From 27d873c452bcd6d02d86c1b7d7f18ec3ba677266 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Tue, 1 Sep 2026 13:36:50 +0300 Subject: [PATCH 1/3] fix: defects that surfaced while building the distribution funnel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit None of this is distribution work. Every item is a defect in code or docs that already shipped, found while doing #141, and split out at review request so it can be judged on its own — and so it can land whether or not the funnel does. ## 1. The expand tool was advertised where no marker can exist `expand.Inject` under `auto` gated on "the request declares tools" and "the store persists". Nothing asked whether the pipeline could produce a `<>` marker at all, so an offloader-free pipeline declared `context_guru_expand` to the provider — and every call against it must fail, because there is nothing in the Store to resolve. Measured on the real gateway route: tools SENT by client : [Read Bash] tools FORWARDED upstream: [Read Bash context_guru_expand] Affected `safe` and any cachesplit-only configuration, and — the one that matters most — **`off`, the A/B control arm**. A control that carries an extra tool declaration is not a control, and every measurement taken against it was comparing two arms that differed by more than the pipeline. The cost when it fires is a wasted round trip and a step of the user's turn: on a transcript containing marker-shaped text (this repo's own docs contain literal `<>`), a model calls the tool and gets "[expand: original for id ... is no longer available]". It was also a code-vs-comment contradiction, which is why nobody noticed: `Options.InjectExpand` documented the gate as requiring "an expandable marker", while `expand/inject.go` says "No marker condition, deliberately" three lines from the code. Both now describe what happens. `components.Pipeline.HasOffload()` answers by TYPE ASSERTION, not a list of component names: a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one. `components.Offload` cannot be implemented by accident — it requires returning cache keys proving the original was stashed. Marker independence is preserved (the property that keeps the tools array byte-stable across a session, and hence the prefix cached): a pipeline does not change turn to turn. **Ten existing tests changed fixture.** Every test of the expand loop hand-seeds the Store to simulate an offload, but built its handler with `pipeline: []` — which cannot offload anything. Harmless while injection ignored the pipeline; now they use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies). No assertion was weakened; each fixture now matches its own premise. ## 2. `POST /v1/messages/count_tokens` was not served Absent it, a client asking how big its context is gets a 404 and falls back to working it out with **inference requests** — billed calls, caused by a proxy whose purpose is to reduce them. Cheap to add, and it costs every routed user, not only the funnel. Forwarded verbatim, with no pipeline. Returning the compacted count would be smaller and would be wrong in the dangerous direction: the client budgets its own transcript from this number, and because every component fails open, the next request could forward the full body and take a 400. Over-reporting is recoverable; under-reporting is a failed turn. The cost of that choice is now documented in `docs/reference/routes.md`, where the route was absent entirely — a routed session self-compacts earlier than it needs to (115,933 reported vs 32,802 forwarded on a measured body). The hosted branch has tests, because that branch is the only thing standing between the multi-tenant service and an unmetered open forwarder that would send OUR credential upstream. ## 3. Our own docs said the binary needs a C toolchain `docs/setup.md`, `docs/hosted.md` and `docs/get-started/quickstart-proxy.md` all told evaluators to install one. It is needed for `go test -race` and for the optional `cg_skeleton` tag, not for the binary. setup.md went further and named **bifrost's tokenizer** as a cgo dependency, which it never was — o200k_base is embedded (`internal/tokens/tokens.go`). Asserted rather than re-claimed: a new `purego` CI job builds with `CGO_ENABLED=0` and `CC=/nonexistent-c-compiler`, checks the artifact is statically linked, starts it and probes /healthz. It also runs the packages whose behaviour depends on which components compile in — because `build-test` runs exclusively with `CGO_ENABLED=1` (the race detector needs it), so `TestEveryPresetBuilds` had **never executed in the configuration a user would build**. That guard exists for exactly the `preset: coding` / `unknown component "skeleton"` breakage. ## 4. Preset facts stated outside the guarded files (#143, #145) - The binary defaults to **`house`**; five sites said `codesmart` (README x3, `docs/reference/config.md`, `docs/get-started/quickstart-proxy.md` — the last is step 2 of the first page anyone runs). Anyone running the binary bare while reading those measured a different configuration than the published SWE-bench numbers describe. - README's `codesmart`/`codesafe` pipeline lists and `docs/get-started/connect-ibm-service.md`'s "Default pipeline" were stale — naming `toon`, retired after acting 0 of 5,752 production requests, and omitting components that do run. The IBM page's omission of `toolfilter` matters most: that page is what a prospective hosted tenant reads to decide what the service does to their traffic. All regenerated from the `presets` map. The two tables inside #142's drift guard are untouched here; these are the sites that guard cannot reach. ## Verification Five mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated -> TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist FAIL on cachesplit-only, `safe`, and `off` HasOffload always false -> same test FAIL on `mcp` and the offloader pipeline: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" count_tokens route unregistered -> TestCountTokensIsServed FAIL (404) count_tokens rewrites the body -> TestCountTokensIsServed FAIL hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The second is the mirror-image check: it proves the gate did not trade one silent defect for another, an offloader whose output nothing can expand. Two things I got wrong on the way, recorded because both were caught by tests rather than by me: - I first asserted `mcp` had no offloader. `smartcrush` implements `components.Offload` (`components/offload/smartcrush.go`), so that pipeline genuinely mints markers and genuinely needs the tool. The case now asserts the opposite, with the reason — and it is the argument for asking the interface rather than keeping a hand-written list. - Copying `proxy/proxy.go` wholesale from the older distribution branch onto current main silently reverted #155's `effPreset`/`notePreset` work. `TestCompactRowNamesThePresetThatRan` — a test I had never read — failed with "the dashboard names a pipeline that did not run". The file was restored from main and the two edits re-applied on top; #155's change is intact. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. One unrelated flake seen once and not reproduced: `TestConcurrentCallsDoNotRaceOnTheGateHistogram` failed in a full-suite run with "no single-flight follower ran ... the race was never exercised", then passed 8/8 in isolation and in two further full suites, and passes on clean main. Reported separately rather than papered over. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .github/workflows/ci.yaml | 40 ++++++++ Makefile | 15 ++- README.md | 11 +- components/pipeline.go | 24 +++++ docs/get-started/connect-ibm-service.md | 2 +- docs/get-started/quickstart-proxy.md | 2 +- docs/hosted.md | 2 +- docs/reference/config.md | 2 +- docs/reference/routes.md | 25 +++++ docs/setup.md | 10 +- proxy/agentcompaction_test.go | 7 +- proxy/ccbody_test.go | 60 +++++++++++ proxy/counttokens.go | 99 ++++++++++++++++++ proxy/counttokens_hosted_test.go | 66 ++++++++++++ proxy/counttokens_test.go | 112 +++++++++++++++++++++ proxy/expandgate_test.go | 127 ++++++++++++++++++++++++ proxy/expandsplice_test.go | 18 ++-- proxy/expandunresolved_test.go | 2 +- proxy/proxy.go | 34 ++++++- proxy/proxy_test.go | 21 +++- proxy/tenancy_test.go | 1 + 21 files changed, 647 insertions(+), 33 deletions(-) create mode 100644 proxy/ccbody_test.go create mode 100644 proxy/counttokens.go create mode 100644 proxy/counttokens_hosted_test.go create mode 100644 proxy/counttokens_test.go create mode 100644 proxy/expandgate_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b79f658c..650965a8 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -33,6 +33,46 @@ jobs: - name: Build run: make build + # The binary needs no C toolchain — asserted, not asserted-in-a-comment. + # + # Our own docs told every evaluator to install one (`docs/setup.md` even named bifrost's + # tokenizer as a cgo dependency, which it never was). The only cgo dependency is tree-sitter, + # behind the `cg_skeleton` build tag. Nothing checked that, and the `build-test` job above runs + # exclusively with CGO_ENABLED=1 because `go test -race` requires it — so the configuration a + # user would actually build was never exercised. + # + # CC is pointed at a path that does not exist, so a cgo dependency escaping the build tag fails + # here rather than in a stranger's terminal. + purego: + runs-on: ubuntu-latest + env: + CGO_ENABLED: "0" + CC: /nonexistent-c-compiler + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: '1.25' + check-latest: true + - name: Build with no C compiler available + run: | + go build -o /tmp/cg-purego ./cmd/context-guru-proxy + file /tmp/cg-purego | tee /dev/stderr | grep -q "statically linked" + - name: It has to start, not just link + run: | + LISTEN_ADDR=127.0.0.1:4471 CONTEXT_GURU_KEEPALIVE=off /tmp/cg-purego & + for _ in $(seq 1 40); do + sleep 0.25 + curl -fsS http://127.0.0.1:4471/healthz && break + done + curl -fsS http://127.0.0.1:4471/healthz | grep -q ok + # And the guard that matters most to a CGO-free build: a preset naming a component which is + # not registered without cgo starts the proxy and then fails at pipeline build. That is the + # `preset: coding` / `unknown component "skeleton"` breakage, and TestEveryPresetBuilds + # covers it — but it had never run in this configuration. + - name: Test the packages whose behaviour depends on which components compile in + run: go test ./config/... ./components/... ./apply/... ./proxy/... ./store/... + trivy: runs-on: ubuntu-latest steps: diff --git a/Makefile b/Makefile index 9139f2b7..f0a21637 100644 --- a/Makefile +++ b/Makefile @@ -6,8 +6,13 @@ LDFLAGS := -s -w \ -X $(PKG)/internal/buildinfo.Version=$(VERSION) \ -X $(PKG)/internal/buildinfo.Commit=$(COMMIT) -# CGO is required to compile the tree-sitter binding; a C toolchain (gcc/clang) -# must be present for make test/build/lint. +# CGO is on for DEVELOPMENT, and the reason is `go test -race`, which does not work +# without it. It is NOT a requirement of the shipped binary: tree-sitter is the only cgo +# dependency in the tree and it is behind the `cg_skeleton` build tag, so a default build +# is pure Go. `make build-static` is that build, and CI proves it with no C compiler on PATH +# (the `purego` job). Reading this variable as a shipping requirement is what put "install a C +# toolchain" at the top of our own quickstart, and named bifrost's tokenizer as a cgo dependency +# in docs/setup.md, which it never was. export CGO_ENABLED=1 .DEFAULT_GOAL := help @@ -22,6 +27,12 @@ build: ## Build the context-guru-proxy binary into ./bin @mkdir -p bin go build -ldflags "$(LDFLAGS)" -o bin/$(BINARY) ./cmd/context-guru-proxy +.PHONY: build-static +build-static: ## Build the pure-Go static binary (no C toolchain needed) + @mkdir -p bin + CGO_ENABLED=0 go build -trimpath -ldflags "$(LDFLAGS)" -o bin/$(BINARY) ./cmd/context-guru-proxy + @file bin/$(BINARY) 2>/dev/null || true + .PHONY: test test: ## Run all tests with the race detector go test -race ./... diff --git a/README.md b/README.md index 2382368a..6a487318 100644 --- a/README.md +++ b/README.md @@ -108,7 +108,7 @@ docker build -t context-guru:local . ```sh # 1 — run the proxy (ships with the SWE-bench-winning cache-aware config by default) -./bin/context-guru-proxy # --preset codesmart; listens on :4000 (LISTEN_ADDR to change) +./bin/context-guru-proxy # --preset house (the default); listens on :4000 # 2 — point any agent at it (one port serves both dialects) export ANTHROPIC_BASE_URL=http://localhost:4000/anthropic @@ -129,9 +129,10 @@ curl -s localhost:4000/anthropic/v1/messages \ -d '{"model":"...","max_tokens":64,"messages":[ ... ]}' ``` -Presets: **`codesmart`** (the default — the SWE-bench-winning cache-aware config -`[format, toon, dedup, failed_run, cmdfilter, extract_llm, extract, cachesplit]`), **`codesafe`** (the same -minus the LLM pass — deterministic-only `[format, dedup, failed_run, cmdfilter, extract, collapse, cachesplit]`, +Presets: **`house`** is the binary's default. **`codesmart`** is the SWE-bench-winning +cache-aware config, `[format, textclean, searchfold, dedup, failed_run, cmdfilter, extract_llm, extract, linecap, cachesplit]`, and is what the +published benchmark numbers describe — pass `--preset codesmart` to run it. **`codesafe`** (the same +minus the LLM pass — deterministic-only `[format, textclean, searchfold, dedup, failed_run, cmdfilter, extract, collapse, linecap, cachesplit]`, zero model calls by policy), plus `general`, `agent`, `aggressive`, `coding`, `mcp`, `balanced`, `safe`, `summarize`, `off`. `codesmart`'s LLM relevance-trimmer (`extract_llm`) engages only when a cheap model is configured @@ -140,7 +141,7 @@ See [docs/components.md](docs/components.md) and [docs/reference/presets.md](doc | Flag / env | Default | Purpose | |---|---|---| -| `--preset` / `PRESET` | `codesmart` | pipeline preset when no `--config` | +| `--preset` / `PRESET` | `house` | pipeline preset when no `--config` | | `--config` / `CONFIG` | — | YAML config (overrides preset) | | `LISTEN_ADDR` | `:4000` | listen address | | `--anthropic-upstream` / `ANTHROPIC_UPSTREAM` | `https://api.anthropic.com` | Anthropic upstream base | diff --git a/components/pipeline.go b/components/pipeline.go index 9be77d07..4477e98e 100644 --- a/components/pipeline.go +++ b/components/pipeline.go @@ -286,6 +286,30 @@ func (p *Pipeline) Find(name string) Component { return nil } +// HasOffload reports whether any component in this pipeline is an Offload — i.e. whether a +// request through it can produce a `<>` marker at all. +// +// Asked by the host to decide whether advertising `context_guru_expand` is meaningful. A +// pipeline with no Offload mints no markers, so every expand call a model makes against it MUST +// fail: there is nothing in the Store to resolve. Advertising it there costs a wasted round trip +// and a step of the user's turn to learn that. +// +// A type assertion, deliberately, rather than a list of component names: a name list is a second +// copy of "which components are lossy" that drifts the moment somebody adds one, and the +// interface is the definition. `components.Offload` cannot be implemented by accident — it +// requires returning cache keys that prove the original was stashed. +func (p *Pipeline) HasOffload() bool { + if p == nil { + return false + } + for _, c := range p.comps { + if _, ok := c.(Offload); ok { + return true + } + } + return false +} + // Has reports whether a component with this name is configured in the pipeline. // Hosts use it to gate body-level work that belongs to a component's concern but // cannot be done inside it — e.g. cacheinject's cache-prefix repair, which must diff --git a/docs/get-started/connect-ibm-service.md b/docs/get-started/connect-ibm-service.md index e9906054..a37c4fa0 100644 --- a/docs/get-started/connect-ibm-service.md +++ b/docs/get-started/connect-ibm-service.md @@ -19,7 +19,7 @@ your agent ──▶ https://contextguru.vpc.cloud9.ibm.com ──▶ the model | Your provider key | **stays yours** — your agent keeps sending it, and the proxy forwards it upstream unchanged | | Cost control | none needed: every account's traffic is billed to that account's own provider credential | | Transcript capture | your account consents **on registration** — [what that means, and the off switch](#three-things-worth-knowing-before-you-rely-on-it) | -| Default pipeline | `[format, toon, dedup, failed_run, cmdfilter, extract, cachesplit]`, `mode: sync` | +| Default pipeline | `[format, dedup, toon, cmdfilter, searchfold, textclean, extract, cachesplit, toolfilter]` (the `house` preset), `mode: sync` | The default pipeline is **fully deterministic** — no cheap-model calls anywhere in it. That is why it is the default on a shared box: it adds no upstream spend, contends for no shared diff --git a/docs/get-started/quickstart-proxy.md b/docs/get-started/quickstart-proxy.md index b4bb7255..0d74a651 100644 --- a/docs/get-started/quickstart-proxy.md +++ b/docs/get-started/quickstart-proxy.md @@ -17,7 +17,7 @@ module dependency — build straight from the repo root. 2. Run it. It listens on `:4000`; set `LISTEN_ADDR` to change that. ```sh - ./bin/context-guru-proxy # default preset: codesmart + ./bin/context-guru-proxy # default preset: house ``` 3. Point your agent at it: diff --git a/docs/hosted.md b/docs/hosted.md index fdb38c91..c7bd8ad8 100644 --- a/docs/hosted.md +++ b/docs/hosted.md @@ -86,7 +86,7 @@ Everything below was derived from the scripts in `deploy/service/`, which are th | For | You need | |---|---| -| Building the binary | Go 1.26 and a C toolchain (`CGO_ENABLED=1`) — or an already-installed `/usr/local/bin/context-guru-proxy`, which `install` keeps if there is no fresh build | +| Building the binary | Go 1.26 — no C toolchain: the binary is pure Go and statically linked (`make build-static`, which CI proves needs no C compiler). Or an already-installed `/usr/local/bin/context-guru-proxy`, which `install` keeps if there is no fresh build. A C toolchain is needed only for the optional `cg_skeleton` tag | | Installing | root, and systemd. The scripts are written for **RHEL 9** (`dnf`, and nginx 1.20's config dialect) | | The TLS front end | `nginx`, plus a certificate and key at `/etc/context-guru/tls/{fullchain,privkey}.pem` | | Each upstream | a host to send it to. A key only if you want the server to hold one — the default forwards each caller’s own | diff --git a/docs/reference/config.md b/docs/reference/config.md index 721f0716..a723248d 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -76,7 +76,7 @@ for every component's config block. | Flag / env | Default | Purpose | |---|---|---| -| `--preset` / `PRESET` | `codesmart` | Pipeline preset when no `--config`. | +| `--preset` / `PRESET` | `house` | Pipeline preset when no `--config`. `codesmart` is the SWE-bench arm and must be asked for by name. | | `--config` / `CONFIG` | — | YAML config file (overrides preset). | | `LISTEN_ADDR` | `:4000` | Listen address. | | `--openai-upstream` / `OPENAI_UPSTREAM` | `https://api.openai.com` | OpenAI upstream base. | diff --git a/docs/reference/routes.md b/docs/reference/routes.md index 318c4789..89daa994 100644 --- a/docs/reference/routes.md +++ b/docs/reference/routes.md @@ -8,6 +8,7 @@ The proxy serves both provider dialects on one port (default `:4000`). |---|---| | `POST /openai/v1/chat/completions` | OpenAI chat dialect — runs the pipeline, forwards to the OpenAI upstream. | | `POST /anthropic/v1/messages` | Anthropic Messages dialect — runs the pipeline, forwards to the Anthropic upstream. | +| `POST /anthropic/v1/messages/count_tokens` | Token counting, forwarded **verbatim** — the pipeline does not run. Absent this route a client falls back to counting context with *inference* requests, which a proxy sold on reducing spend must not cause. See the note below on what it costs. | | `POST /compact` | Stateless compaction: run the pipeline and return the rewritten body, no upstream call. `?provider=anthropic` switches dialect; `?preset=` / `x-context-guru-pipeline` override the pipeline; `?cache=on\|off\|auto` overrides cache-awareness. | | `GET /healthz` | Liveness check. | | `GET /stats` | Savings rollups and health counters — see below. | @@ -15,6 +16,30 @@ The proxy serves both provider dialects on one port (default `:4000`). | `GET /expand?id=` | Recover an offloaded original by its `<>` id. Scoped to the caller's session. | | `GET /favicon.ico` | `204`. Present so a browser's unprompted request does not fall through to the Bob catch-all below and answer `401`. | + +### `count_tokens` answers about the ORIGINAL body, and that is deliberate + +The route forwards the client's body unchanged, so the count it returns describes what the client +sent — not what context-guru will forward. That is the safe direction and it is the literal API +answer, but it has a cost worth stating plainly. + +Returning the *compacted* count would be smaller and would look better. It would also be wrong in +the dangerous direction: the client would believe it has more room than it does, and because every +component fails open (a reverted component forwards the full body), the very next request could +send the uncompacted body and take a `400`. Over-reporting is recoverable; under-reporting is a +failed turn. + +The cost: Claude Code uses this number to decide when to run **its own** compaction, so a routed +session self-compacts earlier than it needs to — paying for a summarization call and discarding +transcript the proxy was already handling. On a measured body: `115,933` tokens reported, +`32,802` actually forwarded. The count also excludes any tool declaration the proxy adds. + +Two upstream caveats: on an implicit prefix-cache backend the numbers are unaffected because +`cachesplit` is a no-op there, and at least one LiteLLM-fronted gateway answers this endpoint with +an implausible count (`13` for a body whose system prompt alone is ~7,929 tokens) — the same answer +it gives when called directly, so the undercount is upstream's, but it means the route's +cheap-budgeting justification does not hold on that upstream. + ### Bob (BobShell) gateway routes Mounted only when `--bob-upstream` / `BOB_UPSTREAM` is set, or in hosted mode. Without diff --git a/docs/setup.md b/docs/setup.md index fc5a2460..ae7510e5 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -5,9 +5,13 @@ SWE-bench task driven by Claude Code. ## Prerequisites -- **Go 1.26** and a **C toolchain** — `CGO_ENABLED=1` (bifrost's tokenizer and, with the - `cg_skeleton` tag, tree-sitter, use cgo). bifrost is an ordinary module dependency; nothing - to check out beside this repo. +- **Go 1.26**. A **C toolchain** is needed only for the `cg_skeleton` tag used below + (tree-sitter), and for `make test`'s race detector — *not* for the binary itself, which is + pure Go and statically linked. bifrost's tokenizer does **not** use cgo: o200k_base is + embedded (`internal/tokens/tokens.go`), and a default `CGO_ENABLED=0` build of this repo + succeeds for all four `GOOS`/`GOARCH` targets we care about. CI asserts it on every PR with no C + compiler on `PATH` at all (`.github/workflows/ci.yaml`, the `purego` job), so this cannot rot + back into a false claim. - **Docker** (for the gateway image / eval-containers), and the **eval-containers** repo. ## Build diff --git a/proxy/agentcompaction_test.go b/proxy/agentcompaction_test.go index b4eed3a0..7c586322 100644 --- a/proxy/agentcompaction_test.go +++ b/proxy/agentcompaction_test.go @@ -229,7 +229,12 @@ func TestExpandToolAdvertisedOnEveryTurnButNeverOnACompaction(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + // A pipeline that CAN offload: advertising is gated on that, because a pipeline + // which mints no markers would advertise a tool whose every call must fail. The + // premise of this test is that an offload happened (hence the store seed below), + // so the fixture has to be a pipeline where that is possible. `linecap` does not + // act on these short bodies, so the forwarded bytes are unchanged. + h, st := buildHandler(t, "pipeline: [linecap]\n", upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() diff --git a/proxy/ccbody_test.go b/proxy/ccbody_test.go new file mode 100644 index 00000000..dac15980 --- /dev/null +++ b/proxy/ccbody_test.go @@ -0,0 +1,60 @@ +// Claude-Code-shaped request fixtures, shared by the proxy's wire-level tests. +// +// They live in their own file because several test groups need them and each was inventing its own +// body, which is how a fixture ends up not resembling what the client actually sends. +package proxy_test + +import ( + "encoding/json" + "strings" + "testing" +) + +// cachePipeline is a cachesplit-only pipeline: the shape a cache-focused deployment runs, and the +// one whose promise is that nothing else touches the request. +const cachePipeline = "pipeline: [cachesplit]\n" + +// attributionText is the block Claude Code prepends as the first system block. +const attributionText = "You are Claude Code, Anthropic's official CLI for Claude." + +// jsonStr quotes a Go string as a JSON string. +func jsonStr(v string) string { + b, _ := json.Marshal(v) + return string(b) +} + +// volatileSystemText is a system block over cachesplit's 1024-token floor (~4 chars/token) that +// ends in the environment snapshot the split exists to move out of the hashed prefix. +func volatileSystemText() string { + return strings.Repeat("You are a coding agent. Follow the instructions carefully.\n", 120) + + "\nCurrent branch: main\nRecent commits:\n0898367954 SWE-bench\n" +} + +// claudeCodeBody is a Claude-Code-shaped Anthropic request: a small attribution block as the +// FIRST system block, then a large one ending in the volatile environment snapshot, with the +// cache breakpoint at its end. +// +// The JSON is assembled as TEXT rather than marshalled from a map, and that is not fussiness. +// Go's json.Marshal sorts map keys, so a map-built body arrives pre-sorted — and a proxy bug +// that re-encodes blocks instead of passing their original bytes through would then produce +// byte-identical output and no test would notice. Claude Code sends `{"type":...,"text":...}`, +// unsorted, and the API's positional strip of the attribution block depends on those bytes +// arriving unchanged. So the fixture has to carry the real order. +func claudeCodeBody(t *testing.T, stream bool) []byte { + t.Helper() + return claudeCodeBodyWithFirst(t, stream, attributionText) +} + +func claudeCodeBodyWithFirst(t *testing.T, stream bool, first string) []byte { + t.Helper() + body := `{"model":"claude-sonnet-5","max_tokens":64,"stream":` + + map[bool]string{true: "true", false: "false"}[stream] + + `,"system":[` + + `{"type":"text","text":` + jsonStr(first) + `},` + + `{"type":"text","text":` + jsonStr(volatileSystemText()) + `,"cache_control":{"type":"ephemeral"}}` + + `],"messages":[{"role":"user","content":"hello"}]}` + if !json.Valid([]byte(body)) { + t.Fatalf("test fixture is not valid JSON: %s", body) + } + return []byte(body) +} diff --git a/proxy/counttokens.go b/proxy/counttokens.go new file mode 100644 index 00000000..ea2844b8 --- /dev/null +++ b/proxy/counttokens.go @@ -0,0 +1,99 @@ +package proxy + +import ( + "bytes" + "io" + "log/slog" + "net/http" +) + +// countTokensPath is the upstream route this forwards to, and the suffix Claude Code appends +// to whatever ANTHROPIC_BASE_URL it was given. +const countTokensPath = "/v1/messages/count_tokens" + +// countTokens forwards POST /v1/messages/count_tokens to the Anthropic upstream, verbatim. +// +// **Why it has to exist.** Claude Code asks the API how many tokens its context is worth, to +// decide when to compact. Route the client through a gateway that does not serve this endpoint +// and the fallback is not a warning — the client works it out by issuing INFERENCE requests +// instead. On a funnel whose entire pitch is "this makes your sessions cheaper", the absence of +// a cheap endpoint silently adds billed calls. Cheap to add; expensive to leave out. +// +// **Why the body is forwarded UNCHANGED, with no pipeline.** The client is asking about the +// context IT holds, and it uses the answer to manage its own transcript. Handing back a count +// for a compacted body would answer a question nobody asked and would make the client's own +// budgeting wrong in the direction that hurts — it would believe it has more room than it does, +// and only find out at a 400 on the real request. So: no components, no cache split, no +// rewriting. The count is the truth about what the client sent. +// +// The response is relayed through h.stream, which copies status, headers and body byte for byte +// — the same path the chat routes use, and the reason an upstream error here reaches the client +// with the upstream's own wording intact. +func (h *Handler) countTokens(static upstream) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + up := static + if h.opts.Tenants != nil { + // Hosted mode: authenticate, meter and resolve the tenant's own upstream, exactly + // as the catch-all does. Without this the route would be an unmetered open + // forwarder that also leaked OUR token to the upstream in place of the tenant's. + tn, err := h.authenticate(r) + if err != nil { + h.refuse(w, r, err) + return + } + release, ok := h.meter(w, r, tn) + defer release() + if !ok { + return + } + if up, err = h.upstreamFor(r, tn, pickAnthropic, static); err != nil { + h.refuseRoute(w, r, tn, err) + return + } + } + if up.base == "" { + recordRefusal(refuseNoUpstream, "") + http.Error(w, "no upstream configured", http.StatusBadGateway) + return + } + + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBytes) + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, "count_tokens: unreadable request body", http.StatusBadRequest) + return + } + req, err := http.NewRequestWithContext(r.Context(), http.MethodPost, + up.base+countTokensPath, bytes.NewReader(body)) + if err != nil { + http.Error(w, "proxy: "+err.Error(), http.StatusBadGateway) + return + } + copyHeaders(req.Header, r.Header) + setUpstreamAuth(req.Header, up) + + resp, err := h.client.Do(req) + if err != nil { + recordRefusal(refuseUpstream, "") + // The fixed string, not err.Error(): a *url.Error stringifies as + // `Post "": ...`, which would publish the operator's + // upstream address — and any userinfo in it — to every caller. Detail goes to + // the log, where the operator can see it and the caller cannot. + slog.Warn("context-guru: count_tokens upstream call failed", "err", err) + http.Error(w, "upstream request failed", http.StatusBadGateway) + return + } + h.stream(w, resp) + } +} + +// anthropicCountTokensUpstream is the single-tenant upstream for the route above: the same +// base and the same credential handling as POST /anthropic/v1/messages, so a proxy configured +// with an API key injects it here too, and one configured without forwards the caller's own. +func (h *Handler) anthropicCountTokensUpstream() upstream { + return upstream{ + base: h.opts.AnthropicUpstream, + path: countTokensPath, + setKey: headerKey("x-api-key", h.opts.AnthropicKey), + } +} diff --git a/proxy/counttokens_hosted_test.go b/proxy/counttokens_hosted_test.go new file mode 100644 index 00000000..8baffb7f --- /dev/null +++ b/proxy/counttokens_hosted_test.go @@ -0,0 +1,66 @@ +package proxy + +import ( + "net/http" + "strings" + "testing" +) + +// The hosted branch of the count_tokens route. +// +// This is here because the branch was shipped untested. `Mux()` has one caller, shared between +// the single-tenant binary and the hosted service, so the multi-tenant deployment serves this +// route — and the `h.opts.Tenants != nil` block is the only thing standing between it and an +// unmetered open forwarder that would send OUR credential upstream in place of the caller's. +// The conformance tests build a handler with `Tenants == nil`, so they exercise none of it. + +// TestCountTokensHostedRequiresAuth: no token, no forwarding. The failure this prevents is not +// subtle — an unauthenticated POST that reaches the upstream is an open relay on somebody else's +// credential, and it would also be unmetered. +func TestCountTokensHostedRequiresAuth(t *testing.T) { + f := newHostedFixture(t, "up1", "anthropic") + const path = "/anthropic/v1/messages/count_tokens" + + if w := f.post(path, "", ""); w.Code != http.StatusUnauthorized { + t.Errorf("%s without a token = %d, want 401", path, w.Code) + } + if w := f.post(path, "cg_live_"+strings.Repeat("A", 26), ""); w.Code != http.StatusUnauthorized { + t.Errorf("%s with an unissued token = %d, want 401", path, w.Code) + } + // And nothing reached the upstream on either attempt. + f.mu.Lock() + n := len(f.seen) + f.mu.Unlock() + if n != 0 { + t.Fatalf("%d unauthenticated request(s) reached the upstream", n) + } +} + +// TestCountTokensHostedForwardsWithTheTenantsCredential: an authenticated call resolves the +// TENANT's upstream and injects the configured key, exactly as the chat route does — and the +// caller's own context-guru token must not travel with it. +func TestCountTokensHostedForwardsWithTheTenantsCredential(t *testing.T) { + f := newHostedFixture(t, "up1", "anthropic") + _, tok := f.register(t, "user@ibm.com") + + w := f.post("/anthropic/v1/messages/count_tokens", tok, "") + if w.Code != http.StatusOK { + t.Fatalf("authenticated count_tokens = %d, want 200: %s", w.Code, w.Body.String()) + } + up := f.lastUpstream(t) + if got := up.URL.Path; got != countTokensPath { + t.Errorf("upstream path = %q, want %q", got, countTokensPath) + } + // The server key is injected in the slot the Anthropic dialect uses... + if got := up.Header.Get("x-api-key"); got != "real-upstream-secret" { + t.Errorf("upstream x-api-key = %q, want the injected server key", got) + } + // ...and OUR token does not leak upstream. This is the same rule the catch-all documents: + // the caller's header holds a context-guru credential, which must not leave the box. + if got := up.Header.Get(TokenHeader); got != "" { + t.Errorf("the context-guru token leaked upstream in %s: %q", TokenHeader, got) + } + if got := up.Header.Get("Authorization"); strings.Contains(got, tok) { + t.Errorf("the context-guru token leaked upstream in Authorization: %q", got) + } +} diff --git a/proxy/counttokens_test.go b/proxy/counttokens_test.go new file mode 100644 index 00000000..d1009fe2 --- /dev/null +++ b/proxy/counttokens_test.go @@ -0,0 +1,112 @@ +// The count_tokens route: served at all, forwarded verbatim, errors passed through. +package proxy_test + +import ( + "fmt" + "github.com/tidwall/gjson" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestCountTokensIsServed covers conformance item 5. +// +// Without this route the client gets a 404 and falls back to counting context by issuing +// INFERENCE requests — billed calls added by a proxy sold on removing them. The body must arrive +// unmodified (the client is asking about the context IT holds, and uses the answer to budget its +// own transcript), and the answer must come back verbatim. +func TestCountTokensIsServed(t *testing.T) { + var gotPath string + var gotBody []byte + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + gotBody, _ = io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"input_tokens":4321}`) + })) + defer upstream.Close() + + h, _ := buildHandler(t, cachePipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + body := claudeCodeBody(t, false) + resp, err := http.Post(srv.URL+"/anthropic/v1/messages/count_tokens", "application/json", + strings.NewReader(string(body))) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + out, _ := io.ReadAll(resp.Body) + + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200 (a 404 here sends the client back to counting with "+ + "inference requests): %s", resp.StatusCode, out) + } + if gotPath != "/v1/messages/count_tokens" { + t.Errorf("upstream path = %q, want /v1/messages/count_tokens", gotPath) + } + if string(gotBody) != string(body) { + t.Errorf("the body was modified before counting. The client is asking about the context "+ + "IT holds and budgets its own transcript from the answer, so a count of a compacted "+ + "body would make it believe it has room it does not.\n got: %s\nwant: %s", gotBody, body) + } + if gjson.GetBytes(out, "input_tokens").Int() != 4321 { + t.Errorf("response not relayed verbatim: %s", out) + } +} + +// TestCountTokensForwardsUpstreamErrors: same wording-preservation rule as the chat route. A +// client that cannot read the real error here has no way to tell a malformed request from an +// auth failure. + +// TestCountTokensForwardsUpstreamErrors: same wording-preservation rule as the chat route. A +// client that cannot read the real error here has no way to tell a malformed request from an +// auth failure. +func TestCountTokensForwardsUpstreamErrors(t *testing.T) { + errBody := `{"type":"error","error":{"type":"authentication_error","message":"invalid x-api-key"}}` + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + fmt.Fprint(w, errBody) + })) + defer upstream.Close() + + h, _ := buildHandler(t, cachePipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + resp, err := http.Post(srv.URL+"/anthropic/v1/messages/count_tokens", "application/json", + strings.NewReader(`{"model":"claude-sonnet-5","messages":[]}`)) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + got, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusUnauthorized || string(got) != errBody { + t.Errorf("status/body not forwarded verbatim: %d %s", resp.StatusCode, got) + } +} + +// TestCachePresetAdvertisesNoExtraTool is the test that should have existed when the `cache` +// preset shipped, and did not. +// +// The preset's promise — repeated in `config/config.go`, `docs/reference/presets.md`, +// `docs/how-to/choose-a-preset.md`, `docs/how-to/install-plugin.md` and the install skill — is +// that nothing is added to the user's requests. It was false: `expand.Inject` under `auto` gated +// only on "the request declares tools" and "the store persists", so a cachesplit-only pipeline +// forwarded `context_guru_expand` to the provider. Measured before the fix: +// +// tools SENT by client : [Read Bash] +// tools FORWARDED upstream: [Read Bash context_guru_expand] +// +// That is not cosmetic. A pipeline with no Offload mints no markers, so EVERY expand call against +// it must fail — on a real session with marker-shaped text in a file, the model called it +// unprompted and got "[expand: original for id ... is no longer available]", costing a round trip +// and a step of the user's turn. +// +// TestCachePresetIsCachesplitAlone could not catch this: it asserts the presets MAP, while the +// injection happens in proxy.go after apply returns. This one reads the bytes that leave the +// process, which is the only place the claim is actually true or false. diff --git a/proxy/expandgate_test.go b/proxy/expandgate_test.go new file mode 100644 index 00000000..989c7114 --- /dev/null +++ b/proxy/expandgate_test.go @@ -0,0 +1,127 @@ +// The expand-tool advertising gate. +package proxy_test + +import ( + "encoding/json" + "fmt" + "github.com/tidwall/gjson" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist is the test that should have existed when the `cache` +// preset shipped, and did not. +// +// The preset's promise — repeated in `config/config.go`, `docs/reference/presets.md`, +// `docs/how-to/choose-a-preset.md`, `docs/how-to/install-plugin.md` and the install skill — is +// that nothing is added to the user's requests. It was false: `expand.Inject` under `auto` gated +// only on "the request declares tools" and "the store persists", so a cachesplit-only pipeline +// forwarded `context_guru_expand` to the provider. Measured before the fix: +// +// tools SENT by client : [Read Bash] +// tools FORWARDED upstream: [Read Bash context_guru_expand] +// +// That is not cosmetic. A pipeline with no Offload mints no markers, so EVERY expand call against +// it must fail — on a real session with marker-shaped text in a file, the model called it +// unprompted and got "[expand: original for id ... is no longer available]", costing a round trip +// and a step of the user's turn. +// +// TestCachePresetIsCachesplitAlone could not catch this: it asserts the presets MAP, while the +// injection happens in proxy.go after apply returns. This one reads the bytes that leave the +// process, which is the only place the claim is actually true or false. +func TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist(t *testing.T) { + for _, c := range []struct { + name string + pipeline string + wantAdd bool + }{ + // A cache-only pipeline: no offloader, so no marker can ever exist. + {"cachesplit only", "pipeline: [cachesplit]\n", false}, + // `safe` is a shipped preset with no Offload — lossless folds and the cache split only — + // and it was affected too. + {"safe", "pipeline: [format, textclean, searchfold, cachesplit]\n", false}, + // `mcp` looks similar and is NOT affected, which is the distinction the gate has to get + // right: `smartcrush` implements components.Offload (components/offload/smartcrush.go), + // so that pipeline really can mint a marker and really does need the tool. I had this + // case the wrong way round at first and the test caught it — which is the argument for + // asking the interface rather than keeping a hand-written list of "lossy" component names. + {"mcp (smartcrush IS an Offload)", "pipeline: [format, textclean, smartcrush, cachesplit]\n", true}, + // `off` is the A/B control arm. A control that carries an extra tool declaration is not + // a control — and this was broken too. + {"off (the A/B control)", "pipeline: []\n", false}, + // The feature must still work where markers can exist, or the fix has traded one silent + // defect for another: an offloader whose output nothing can expand. + {"a pipeline with an offloader", "pipeline: [linecap, cachesplit]\n", true}, + } { + t.Run(c.name, func(t *testing.T) { + var forwarded []byte + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + forwarded, _ = io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) + })) + defer upstream.Close() + + h, _ := buildHandler(t, c.pipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + // Tools declared, which is what `auto` keys on, plus a long tool output so the + // offloader case has something to act on. + body := []byte(`{"model":"claude-sonnet-5","max_tokens":64,` + + `"tools":[{"name":"Read","input_schema":{"type":"object"}},` + + `{"name":"Bash","input_schema":{"type":"object"}}],` + + `"messages":[{"role":"user","content":[{"type":"tool_result","tool_use_id":"a",` + + `"content":` + jsonStr(strings.Repeat("output line that is long enough to offload\n", 400)) + + `}]}]}`) + if !json.Valid(body) { + t.Fatalf("fixture is not valid JSON") + } + + resp, err := http.Post(srv.URL+"/anthropic/v1/messages", "application/json", + strings.NewReader(string(body))) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if len(forwarded) == 0 { + t.Fatal("upstream received nothing") + } + + var sent, got []string + for _, v := range gjson.GetBytes(body, "tools").Array() { + sent = append(sent, v.Get("name").String()) + } + for _, v := range gjson.GetBytes(forwarded, "tools").Array() { + got = append(got, v.Get("name").String()) + } + has := false + for _, n := range got { + if n == "context_guru_expand" { + has = true + } + } + if has != c.wantAdd { + if c.wantAdd { + t.Fatalf("pipeline %q mints markers but no longer advertises the expand tool, "+ + "so a model cannot recover what it offloaded: sent %v, forwarded %v", + c.pipeline, sent, got) + } + t.Fatalf("pipeline %q has no offloader, so it mints no markers and every expand "+ + "call against it must fail — but it advertises the tool anyway: sent %v, "+ + "forwarded %v", c.pipeline, sent, got) + } + // The client's own tools must keep their exact identity and order either way: the + // tools array has to be byte-stable across a session or every turn is a full + // prefix-cache miss. + for i, n := range sent { + if got[i] != n { + t.Errorf("client tool %d changed: %q -> %q (full: %v)", i, n, got[i], got) + } + } + }) + } +} diff --git a/proxy/expandsplice_test.go b/proxy/expandsplice_test.go index 86bd7554..36349555 100644 --- a/proxy/expandsplice_test.go +++ b/proxy/expandsplice_test.go @@ -76,7 +76,7 @@ func TestExpandCalledAfterALeadingBlockIsNeverGivenToTheClient(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -155,7 +155,7 @@ func TestTheStreamedPrefixReachesTheClientBeforeTheExpandCall(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -217,7 +217,7 @@ func TestTheClientsNoSuchToolErrorNeverReachesTheModel(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -299,7 +299,7 @@ func TestAFailedContinuationRoundStillEndsTheClientsTurn(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -357,7 +357,7 @@ func TestAJSONContinuationRoundStillEndsTheClientsTurn(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -404,7 +404,7 @@ func TestARoundPastTheRetainBoundCountsTheLeakAndDoesNotContinue(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -467,7 +467,7 @@ func TestEveryContinuationShapeStillClosesTheClientsMessage(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -523,7 +523,7 @@ func TestASplicedRoundThatCallsExpandAgainStaysWellFormed(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("ORIGINAL ONE")) st.Put("HASH2", []byte("ORIGINAL TWO")) srv := httptest.NewServer(h.Mux()) @@ -608,7 +608,7 @@ func TestATruncatedRoundStillGivesTheClientAnEnd(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() diff --git a/proxy/expandunresolved_test.go b/proxy/expandunresolved_test.go index d84ecd95..9c63276e 100644 --- a/proxy/expandunresolved_test.go +++ b/proxy/expandunresolved_test.go @@ -45,7 +45,7 @@ func TestAnExpandCallThatResolvesNothingStillFinishesTheTurn(t *testing.T) { })) defer upstream.Close() - h, _ := buildHandler(t, "pipeline: []\n", upstream.URL) + h, _ := buildHandler(t, offloadCapablePipeline, upstream.URL) srv := httptest.NewServer(h.Mux()) defer srv.Close() diff --git a/proxy/proxy.go b/proxy/proxy.go index bac2858c..e6ee5def 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -82,9 +82,20 @@ type Options struct { } // InjectExpand controls advertising the context_guru_expand tool on outgoing // requests so Offload markers are actually recoverable (expand.InjectAuto | - // InjectAlways | InjectNever). Empty defaults to auto. auto injects only when the - // request already declares tools, carries an expandable marker, and the store - // persists — safe for any agent. + // InjectAlways | InjectNever). Empty defaults to auto. + // + // `auto` injects when the request already declares tools, the store persists, and THE + // PIPELINE CONTAINS AT LEAST ONE OFFLOAD. It does NOT depend on the request carrying a + // marker — that would make the tools array vary turn to turn, and every variation is a + // whole-prefix cache miss (see the note on expand.InjectAuto). + // + // This comment used to claim a marker condition that expand.Inject explicitly disclaims, + // and the pipeline condition was missing entirely. The consequence was live: a + // cachesplit-only pipeline (the `cache` preset) forwarded `context_guru_expand` to the + // provider, a model that saw marker-shaped text in a file called it, and the call could + // only ever fail because nothing in that pipeline mints a marker to resolve. + // + // `always` still injects unconditionally — an operator asking for it by name gets it. InjectExpand string // CacheMode controls cache-aware compaction ("auto"|"on"|"off"; empty=auto). // auto/on keep offloaders from mutating already-cached content on prompt-caching @@ -346,6 +357,10 @@ func (h *Handler) Mux() *http.ServeMux { path: "/v1/messages", setKey: headerKey("x-api-key", h.opts.AnthropicKey), }, pickAnthropic)) + // Token counting, forwarded verbatim with no pipeline. Absent this route, a client that + // asks how big its context is gets a 404 and falls back to working it out with inference + // requests — billed calls, added by a proxy sold on reducing them. See counttokens.go. + m.HandleFunc("POST /anthropic"+countTokensPath, h.countTokens(h.anthropicCountTokensUpstream())) m.HandleFunc("POST /compact", h.compact) m.HandleFunc("GET /healthz", func(w http.ResponseWriter, _ *http.Request) { w.Write([]byte("ok")) }) // A browser asks for /favicon.ico unprompted, and in hosted mode Bob's "/" catch-all @@ -1145,7 +1160,18 @@ func (h *Handler) chat(provider bschemas.ModelProvider, static upstream, pick fu if im == "" { im = expand.InjectAuto } - body, _ = expand.Inject(string(provider), im, body, tn.Store.Persists()) + // Under `auto`, advertise only when THIS request's pipeline can actually mint a + // marker. Without this, an offloader-free pipeline (`cache`, `off`, `safe`, + // `mcp`) declared a tool whose every use is guaranteed to fail — measured: + // `[Read Bash]` in, `[Read Bash context_guru_expand]` out, and on a transcript + // containing marker-shaped text the model duly called it and got + // "[expand: original for id ... is no longer available]". + // + // `off` mattered most: it is the A/B control arm, and a control that carries an + // extra tool declaration is not a control. + if im != expand.InjectAuto || tn.Pipe.HasOffload() { + body, _ = expand.Inject(string(provider), im, body, tn.Store.Persists()) + } } }() // Load the request's one INFO line with everything the pipeline decided. serve diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 53714666..28633356 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -35,6 +35,19 @@ func buildHandler(t *testing.T, yaml string, upstream string) (*proxy.Handler, s return proxy.New(pipe, st, agg, proxy.Options{OpenAIUpstream: upstream, AnthropicUpstream: upstream}), st } +// offloadCapablePipeline is the fixture for any test whose premise is that content was +// OFFLOADED — i.e. every test of the context_guru_expand loop. +// +// Advertising that tool is gated on the pipeline containing an Offload, because a pipeline that +// mints no markers would be declaring a tool whose every call must fail (the defect that shipped +// in the `cache` and `off` presets). A `pipeline: []` fixture therefore no longer advertises it, +// and a test that hand-seeds the Store to simulate an offload has to use a pipeline where the +// offload it is simulating could actually have happened. +// +// `linecap` is an Offload that does not act on the short bodies in these tests, so it leaves the +// asserted bytes alone. +const offloadCapablePipeline = "pipeline: [linecap]\n" + func openAIBody(msgs ...map[string]any) []byte { b, _ := json.Marshal(map[string]any{"model": "gpt-x", "temperature": 0.2, "messages": msgs}) return b @@ -298,7 +311,7 @@ func TestExpandToolLoop(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) // as if a prior turn offloaded it srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -356,7 +369,7 @@ func TestExpandSSELoop(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -428,7 +441,7 @@ func TestExpandPartialResolutionWellFormed(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("GOOD", []byte("RESOLVED ORIGINAL")) // only one of the two resolves srv := httptest.NewServer(h.Mux()) defer srv.Close() @@ -770,7 +783,7 @@ func TestExpandSSEMultiRoundCapped(t *testing.T) { })) defer upstream.Close() - h, st := buildHandler(t, "pipeline: []\n", upstream.URL) + h, st := buildHandler(t, offloadCapablePipeline, upstream.URL) st.Put("HASH", []byte("THE ORIGINAL CONTENT")) srv := httptest.NewServer(h.Mux()) defer srv.Close() diff --git a/proxy/tenancy_test.go b/proxy/tenancy_test.go index b07a1e6b..59a45061 100644 --- a/proxy/tenancy_test.go +++ b/proxy/tenancy_test.go @@ -179,6 +179,7 @@ func TestHostedRejectsUnauthenticated(t *testing.T) { for _, path := range []string{ "/openai/v1/chat/completions", "/anthropic/v1/messages", + "/anthropic/v1/messages/count_tokens", "/inference/v1/chat/completions", "/compact", } { From f656a0bff586212275201be9085bc15847993c29 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Tue, 1 Sep 2026 14:18:32 +0300 Subject: [PATCH 2/3] ci(purego): run one package binary at a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new job runs `go test` over five package trees, and `go test` starts up to GOMAXPROCS package binaries in parallel. On a 2-core CI runner that added a second heavily-parallel run of the proxy package per PR, and under that contention a timing-sensitive control-plane test from #150 (TestCtlGetCampaignAggregatesPredictedAndRealPerTenant) failed on two unrelated PRs — then passed on a re-run of the same commit, and passes 3/3 whole-package on a 16-core box against both main and the affected branch. Filed as #163. Hunting that flake is not this job's business. Not provoking it is: `-p 1` costs about a minute and removes the contention this job introduced, without dropping any coverage. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .github/workflows/ci.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 650965a8..ae8219cd 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -70,8 +70,17 @@ jobs: # not registered without cgo starts the proxy and then fails at pipeline build. That is the # `preset: coding` / `unknown component "skeleton"` breakage, and TestEveryPresetBuilds # covers it — but it had never run in this configuration. + # -p 1: run one package binary at a time. + # + # A CI runner has 2 cores, and `go test` otherwise starts up to GOMAXPROCS package binaries + # in parallel. Adding this job therefore added a SECOND heavily-parallel run of the proxy + # package per PR, and under that contention a timing-sensitive control-plane test + # (TestCtlGetCampaignAggregatesPredictedAndRealPerTenant, see the issue linked below) failed + # twice on unrelated PRs, then passed on re-run and passes 3/3 locally on main and on the + # branch. That flake is not this job's business to hunt, but it IS this job's business not to + # provoke it: serialising costs about a minute and removes the contention this job added. - name: Test the packages whose behaviour depends on which components compile in - run: go test ./config/... ./components/... ./apply/... ./proxy/... ./store/... + run: go test -p 1 ./config/... ./components/... ./apply/... ./proxy/... ./store/... trivy: runs-on: ubuntu-latest From 62234c3b9fd5e95262b136cb0b0d7f75309c941e Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Tue, 1 Sep 2026 19:44:34 +0300 Subject: [PATCH 3/3] fix: make this PR's own prose true, and unit-test HasOffload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #161 found the same defect this PR is themed on — claims that are not true — in seven more places, one of them in shipped code. All seven fixed, plus the `HasOffload` unit test the reviewer raised without asking for. Rebased onto current main first, so #142's preset-table guard and the preset pass below cannot re-fix or re-break each other. ## Merge-blocking **1. `proxy/proxy.go` named `mcp` as offloader-free.** It is not: `smartcrush` implements components.Offload, which this PR's own test asserts (`wantAdd: true`) and its own mirror-image mutation proves. I had corrected the test and left the comment wrong — the copy a future reader actually trusts. The fix deletes the list rather than correcting it. The comment now names the shapes affected (`off`, `safe`, any cachesplit-only configuration) and then says why enumerating presets here is the wrong move: a list in a comment is a second source of truth, and this one was wrong about `mcp` on its first draft. That is the whole argument for gating on the interface. **2. Five sites named the `cache` preset, which does not exist on this base.** It is #141's, and it reached here in the wholesale copy of `proxy/proxy.go` this PR already admits to, then travelled into the test files when they were split out of that branch. Reworded to name configurations that exist here; the underlying defect they describe is unchanged and still reproduces on `off` and `safe`. **3. `proxy/counttokens_test.go` carried copy-paste artifacts vet and gofmt cannot see.** A duplicated 3-line doc comment, and a 20-line orphan documenting a function that lives in `expandgate_test.go` under a different name and citing a test that exists nowhere. Both from the same cause: my splitter took each test's doc comment by scanning back to the previous blank line, which swallowed the FOLLOWING test's comment as a trailing block. A third artifact the review did not list is fixed too — `expandgate_test.go`'s doc comment still described "the preset's promise" and cited `docs/how-to/install-plugin.md` and an install skill, both of which belong to #160. ## The rest **4.** `docs/reference/config.md` and `docs/components.md` said `auto` injection has exactly two conditions. It has three. Both now say so, and say what the third is for. The cache-stability argument those passages make is unaffected — a pipeline does not change turn to turn either — so it gained a member rather than needing a rewrite. **5. `make build` now sets `CGO_ENABLED=0`.** The docs could claim "no C toolchain" all they liked while step 1 of the quickstart was `make build`, which needed one because the Makefile exported `CGO_ENABLED=1` for every target. Pointing readers at `build-static` would have fixed the sentence; making the DEFAULT build pure Go makes the claim true of the command the docs tell people to run. `CGO_ENABLED=1` stays for the test targets, where `-race` requires it, and the comment says exactly that. Verified: `CC=/nonexistent make build` produces a statically linked binary. README, CLAUDE.md and the quickstart no longer require a C toolchain. All five remaining `codesmart`-is-the-default sites are corrected — including two in `config/config.go`, which is how the claim spread to five documents: it sat three lines from the flag that disproves it. **6.** `docs/setup.md` overstated its own evidence, which is the exact sin this PR is about. It claimed CI removes the C compiler from `PATH` (with cgo off the toolchain never consults `CC`; that variable is a tripwire, not the mechanism) and that cross-compilation to four targets is asserted, when CI builds native linux/amd64 only. Now says what CI actually does, and states separately that the other three targets were verified by hand and are asserted at release time. Same overstatement fixed in the `ci.yaml` comment. **7.** `ci.yaml` promised a linked issue and linked nothing, and named a different flake than the PR body did. Both are real; the comment is about the campaign one, and now links #163. ## HasOffload unit tests `./components`: nil-safe, empty pipeline (the A/B control arm), reformatters-only, and an offloader in three positions. Revert-verified both ways — always-true fails the empty and reformatter cases, always-false fails the offloader cases. A registry-walking test was supposed to make it rot-proof, and **it skipped**: registrations happen in `components/all`, so a test inside `components` can neither see them nor import the package that does. A test that skips reads as coverage and is not, so it moved to `components/all`, where it runs — 21 of 21 registered components, 13 implementing Offload. It fails if either count is zero, because an all-false or all-true population would agree with a broken HasOffload. Full `go test ./...`, `go vet ./...` and `gofmt -l` clean; doc link/anchor checker re-run over every document touched. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .github/workflows/ci.yaml | 8 ++-- CLAUDE.md | 4 +- Makefile | 17 ++++--- README.md | 4 +- components/all/hasoffload_test.go | 58 ++++++++++++++++++++++++ components/hasoffload_test.go | 66 ++++++++++++++++++++++++++++ config/config.go | 9 ++-- docs/components.md | 12 ++--- docs/get-started/quickstart-proxy.md | 6 ++- docs/how-to/choose-a-preset.md | 2 +- docs/how-to/use-with-claude-code.md | 2 +- docs/reference/config.md | 2 +- docs/setup.md | 12 +++-- proxy/counttokens_test.go | 25 ----------- proxy/expandgate_test.go | 28 ++++++------ proxy/proxy.go | 20 ++++++--- proxy/proxy_test.go | 3 +- 17 files changed, 204 insertions(+), 74 deletions(-) create mode 100644 components/all/hasoffload_test.go create mode 100644 components/hasoffload_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index ae8219cd..95e5a1e1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -41,8 +41,10 @@ jobs: # exclusively with CGO_ENABLED=1 because `go test -race` requires it — so the configuration a # user would actually build was never exercised. # - # CC is pointed at a path that does not exist, so a cgo dependency escaping the build tag fails - # here rather than in a stranger's terminal. + # CGO_ENABLED=0 is what does the work: with cgo off the toolchain never consults CC at all, so a + # dependency that needs it fails to BUILD here rather than in a stranger's terminal. CC is pointed + # at a nonexistent path anyway, as a tripwire for the day somebody sets CGO_ENABLED=1 in this job + # and expects it to still be testing the same thing. purego: runs-on: ubuntu-latest env: @@ -75,7 +77,7 @@ jobs: # A CI runner has 2 cores, and `go test` otherwise starts up to GOMAXPROCS package binaries # in parallel. Adding this job therefore added a SECOND heavily-parallel run of the proxy # package per PR, and under that contention a timing-sensitive control-plane test - # (TestCtlGetCampaignAggregatesPredictedAndRealPerTenant, see the issue linked below) failed + # (TestCtlGetCampaignAggregatesPredictedAndRealPerTenant — rossoctl/context-guru#163) failed # twice on unrelated PRs, then passed on re-run and passes 3/3 locally on main and on the # branch. That flake is not this job's business to hunt, but it IS this job's business not to # provoke it: serialising costs about a minute and removes the contention this job added. diff --git a/CLAUDE.md b/CLAUDE.md index c56ca73a..3654563f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,9 @@ reference — port its *logic*, re-implement its transport in Go. ## Conventions -- Go 1.26, module `github.com/rossoctl/context-guru`. Build needs `CGO_ENABLED=1` (tree-sitter). +- Go 1.26, module `github.com/rossoctl/context-guru`. The build is pure Go — `make build` sets + `CGO_ENABLED=0`. cgo is needed only for `go test -race` and for `-tags cg_skeleton` (tree-sitter), + which is why the Makefile still exports `CGO_ENABLED=1` for the test targets. - Match the surrounding code's style; keep packages small and single-purpose. - **Commits: DCO sign-off is mandatory** — `git commit -s`. Author as the repo owner. AI attribution uses `Assisted-By:` — never `Co-Authored-By`, never a "Generated with" diff --git a/Makefile b/Makefile index f0a21637..bca99df9 100644 --- a/Makefile +++ b/Makefile @@ -9,10 +9,13 @@ LDFLAGS := -s -w \ # CGO is on for DEVELOPMENT, and the reason is `go test -race`, which does not work # without it. It is NOT a requirement of the shipped binary: tree-sitter is the only cgo # dependency in the tree and it is behind the `cg_skeleton` build tag, so a default build -# is pure Go. `make build-static` is that build, and CI proves it with no C compiler on PATH -# (the `purego` job). Reading this variable as a shipping requirement is what put "install a C -# toolchain" at the top of our own quickstart, and named bifrost's tokenizer as a cgo dependency -# in docs/setup.md, which it never was. +# is pure Go, and `make build` now builds it that way — so the documented "no C toolchain" claim +# is true of the command the docs actually tell you to run, which it was not while this variable +# applied to every target. CI proves it with no compiler on PATH at all (the `purego` job). +# +# Reading this variable as a SHIPPING requirement is what put "install a C toolchain" at the top of +# our own quickstart, and named bifrost's tokenizer as a cgo dependency in docs/setup.md, which it +# never was. It is needed for `go test -race` and for `-tags cg_skeleton`; nothing else. export CGO_ENABLED=1 .DEFAULT_GOAL := help @@ -23,12 +26,12 @@ help: ## Display this help awk 'BEGIN {FS = ":.*?## "}; {printf " \033[36m%-12s\033[0m %s\n", $$1, $$2}' .PHONY: build -build: ## Build the context-guru-proxy binary into ./bin +build: ## Build the context-guru-proxy binary into ./bin (pure Go — no C toolchain needed) @mkdir -p bin - go build -ldflags "$(LDFLAGS)" -o bin/$(BINARY) ./cmd/context-guru-proxy + CGO_ENABLED=0 go build -ldflags "$(LDFLAGS)" -o bin/$(BINARY) ./cmd/context-guru-proxy .PHONY: build-static -build-static: ## Build the pure-Go static binary (no C toolchain needed) +build-static: ## Same as build, plus -trimpath — the exact build releases ship @mkdir -p bin CGO_ENABLED=0 go build -trimpath -ldflags "$(LDFLAGS)" -o bin/$(BINARY) ./cmd/context-guru-proxy @file bin/$(BINARY) 2>/dev/null || true diff --git a/README.md b/README.md index 6a487318..3fd159ec 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,9 @@ flowchart TD ## Install -Requires **Go 1.26** and a **C toolchain** (`CGO_ENABLED=1`). Build from the repo root: +Requires **Go 1.26**; no C toolchain — `make build` builds with cgo off and produces a statically +linked binary. (A C compiler is needed only for `make test`'s race detector and the optional +`cg_skeleton` tag.) Build from the repo root: ```sh CGO_ENABLED=1 go build -tags cg_skeleton -o bin/context-guru-proxy ./cmd/context-guru-proxy diff --git a/components/all/hasoffload_test.go b/components/all/hasoffload_test.go new file mode 100644 index 00000000..085ba96e --- /dev/null +++ b/components/all/hasoffload_test.go @@ -0,0 +1,58 @@ +package all + +import ( + "testing" + + "github.com/rossoctl/context-guru/components" +) + +// Pipeline.HasOffload must agree with the Offload interface for every component the binary can +// build — that agreement is the whole reason the gate asks the interface instead of keeping a list +// of "lossy" component names, and it is what stops the list-shaped version of this from rotting. +// +// This lives in `all` rather than in `components` because the registry is only populated here: the +// blank imports in all.go are what run each component's init(). The same test inside `components` +// skipped for lack of registrations, which looked like coverage and was not. +// +// It walks the registry rather than a fixture, so a new lossy component needs no change here, and a +// component that quietly stops implementing Offload fails immediately. +func TestHasOffloadAgreesWithTheOffloadInterface(t *testing.T) { + names := components.Names() + if len(names) == 0 { + t.Fatal("no components registered: this package's blank imports are what register them, " + + "so an empty registry means the test is checking nothing") + } + var checked, offloaders int + for _, name := range names { + c, err := components.New(name, nil) + if err != nil || c == nil { + // A component that will not build from a nil config block; the proxy-level tests + // exercise those with real configuration. + continue + } + _, isOffload := c.(components.Offload) + if isOffload { + offloaders++ + } + got := components.NewPipeline([]components.Component{c}, nil).HasOffload() + if got != isOffload { + t.Errorf("component %q: implements components.Offload = %v, but a pipeline holding "+ + "only it reports HasOffload() = %v", name, isOffload, got) + } + checked++ + } + if checked == 0 { + t.Fatal("no component could be built from a nil config, so nothing was actually checked") + } + // Both sides must be non-empty or the agreement is trivial: all-false would agree with a + // HasOffload that always returns false, which is the mirror-image defect. + if offloaders == 0 { + t.Errorf("checked %d components and none implement Offload — either the registry is not "+ + "what it should be, or this test would pass against a broken HasOffload", checked) + } + if offloaders == checked { + t.Errorf("all %d checked components implement Offload, so this test would also pass "+ + "against a HasOffload that always returns true", checked) + } + t.Logf("checked %d of %d registered components; %d implement Offload", checked, len(names), offloaders) +} diff --git a/components/hasoffload_test.go b/components/hasoffload_test.go new file mode 100644 index 00000000..7d050490 --- /dev/null +++ b/components/hasoffload_test.go @@ -0,0 +1,66 @@ +package components + +import ( + "testing" + + schemas "github.com/maximhq/bifrost/core/schemas" +) + +// HasOffload's whole selling point is that it cannot rot the way a list of component names would, +// so it is worth a test in its own package rather than only proxy-level integration coverage. +// +// It is a type assertion over the pipeline's components, and the property that matters is that it +// answers by INTERFACE: a component that drops bytes must satisfy components.Offload, and adding +// one must not require anybody to remember to update a second list somewhere. These fakes stand in +// for the three shapes a real component takes. + +type offloadStub struct{ name string } + +func (o offloadStub) Name() string { return o.name } +func (o offloadStub) Enabled(*Ctx) bool { return true } +func (o offloadStub) Offload(*schemas.BifrostChatRequest, *Report, *Ctx) ([]string, error) { + return []string{"key"}, nil +} + +type reformatStub struct{ name string } + +func (r reformatStub) Name() string { return r.name } +func (r reformatStub) Enabled(*Ctx) bool { return true } +func (r reformatStub) Reformat(*schemas.BifrostChatRequest, *Report, *Ctx) error { return nil } + +func TestHasOffload(t *testing.T) { + for _, c := range []struct { + name string + comps []Component + want bool + }{ + {"nil pipeline", nil, false}, + {"empty pipeline — the A/B control arm", []Component{}, false}, + {"reformatters only", []Component{reformatStub{"format"}, reformatStub{"cachesplit"}}, false}, + {"one offloader", []Component{offloadStub{"linecap"}}, true}, + {"an offloader among reformatters", []Component{ + reformatStub{"format"}, offloadStub{"mask"}, reformatStub{"cachesplit"}}, true}, + // Position must not matter: the host asks "can this pipeline ever mint a marker", which is + // a property of the set, not of the order. + {"offloader last", []Component{reformatStub{"format"}, offloadStub{"extract"}}, true}, + } { + t.Run(c.name, func(t *testing.T) { + var p *Pipeline + if c.comps != nil || c.name != "nil pipeline" { + p = NewPipeline(c.comps, nil) + } + if got := p.HasOffload(); got != c.want { + t.Errorf("HasOffload() = %v, want %v", got, c.want) + } + }) + } +} + +// TestHasOffloadIsNilSafe: the host calls this on a per-request pipeline that a fail-open path may +// leave nil, and a panic there would take down a request rather than degrade it. +func TestHasOffloadIsNilSafe(t *testing.T) { + var p *Pipeline + if p.HasOffload() { + t.Error("a nil pipeline reported an offloader") + } +} diff --git a/config/config.go b/config/config.go index 977d0a0c..4bc6c0b0 100644 --- a/config/config.go +++ b/config/config.go @@ -402,8 +402,11 @@ var presets = map[string][]string{ // stacking our offloaders beside it would reduce the same tool outputs first and // there would be nothing left to attribute. "agentdiet": {"format", "agentdiet", "cachesplit"}, - // codesmart / codesafe are the SWE-bench study's winning configs, shipped as the - // recommended defaults (codesmart is the proxy default). Their tuned per-component + // codesmart / codesafe are the SWE-bench study's winning configs and the RECOMMENDED + // pipelines — but neither is the proxy default, which is `house` (see the --preset flag in + // cmd/context-guru-proxy/main.go). Asserting it here is how five documents came to repeat + // it: the claim was copied from three lines away from the flag that disproves it. Their + // tuned per-component // settings live in presetConfigs; the name-lists here keep PresetPipeline (used by // /compact?preset=) resolving them. "codesmart": {"format", "textclean", "searchfold", "dedup", "failed_run", "cmdfilter", "extract_llm", "extract", "linecap", "cachesplit"}, @@ -442,7 +445,7 @@ var presets = map[string][]string{ // reader relies on when deciding whether the published numbers describe the shipped // default. They describe an ancestor of it. Treat any preset change as a reason to // re-measure, not as a documentation edit. -// - codesmart (the winning cache-aware config, and the proxy default): the LLM +// - codesmart (the winning cache-aware config; NOT the proxy default, which is `house`): the LLM // relevance-trimmer extract_llm routed to the CHEAP model (model.source: config, // nil-when-unset ⇒ it silently no-ops to deterministic — see docs), gated at 3000 // tok so most turns make no model call, ≤4 calls/req; the free deterministic extract diff --git a/docs/components.md b/docs/components.md index f5643e5f..410a656d 100644 --- a/docs/components.md +++ b/docs/components.md @@ -31,7 +31,7 @@ messages (`role:"tool"`; for Anthropic, `tool_result` blocks normalized to that | `summarize` | Offload (LLM) | the middle of the transcript → one summary | via expand | long trajectories | `summary_level` (regular), `keep_last` (3), `min_tokens` (500), `resummarize_tokens` (6000), `model.source`, `trigger` | | [`agentdiet`](components/agentdiet.md) | Offload (LLM) | useless/redundant/**expired** content in the step that just aged past the delay | via expand | a step above `min_step_tokens`, `delay_steps` turns back | `delay_steps` (2), `context_steps` (1), `min_step_tokens` (500), `min_saved_tokens` (400), `max_keep_ratio` (0.8), `model.source` | -Presets (`config/config.go`), verbatim: **`codesmart`** (the proxy default) +Presets (`config/config.go`), verbatim: **`house`** (the proxy default), **`codesmart`** (the SWE-bench arm) `[format, textclean, searchfold, dedup, failed_run, cmdfilter, extract_llm, extract, linecap, cachesplit]` · **`codesafe`** `[format, textclean, searchfold, dedup, failed_run, cmdfilter, extract, collapse, linecap, cachesplit]` @@ -79,10 +79,12 @@ Absolutes (`min_request_tokens`, etc.) still win; when the window is unknown, fr absolutes apply (backward compatible). This lets one config generalize across models/benchmarks. **Reversibility in practice.** The `context_guru_expand` tool is advertised on outgoing requests -(`INJECT_EXPAND=auto|always|never`, default `auto` = whenever the request already declares -tools and the store persists), so Offload markers are genuinely recoverable — not just described in marker text. -Both conditions are properties of the **session**, not of the turn, so the `tools` array a session -sends is byte-identical on every request in it. That matters more than it looks: `tools` sits ahead of +(`INJECT_EXPAND=auto|always|never`, default `auto` = whenever the request already declares tools, +the store persists, **and the pipeline contains at least one Offload**), so Offload markers are +genuinely recoverable — not just described in marker text. That third condition is what keeps the +tool off pipelines that mint no markers, where every call to it would have to fail. +All three conditions are properties of the **session**, not of the turn, so the `tools` array a +session sends is byte-identical on every request in it. That matters more than it looks: `tools` sits ahead of `system` and `messages` in the provider's prompt-cache hash, so the first request carrying a **new** tools array re-creates the **entire** prefix at the write rate. `auto` used to also require a marker on the request, which made the array grow on the first offloading turn and shrink again on the next turn diff --git a/docs/get-started/quickstart-proxy.md b/docs/get-started/quickstart-proxy.md index 0d74a651..e82ebb44 100644 --- a/docs/get-started/quickstart-proxy.md +++ b/docs/get-started/quickstart-proxy.md @@ -3,9 +3,13 @@ Run context-guru in front of your provider and point an agent at it. One port serves both the OpenAI and Anthropic dialects. -You need **Go 1.26** and a **C toolchain** (`CGO_ENABLED=1`). Everything else is a normal +You need **Go 1.26**. You do **not** need a C toolchain: `make build` builds with cgo disabled, and +the result is a statically linked binary with no runtime dependencies. Everything else is a normal module dependency — build straight from the repo root. +A C compiler is needed for exactly two things: `make test` (the race detector requires cgo) and the +optional [`skeleton`](../components/skeleton.md) component's `cg_skeleton` build tag. + ## Steps 1. Build: diff --git a/docs/how-to/choose-a-preset.md b/docs/how-to/choose-a-preset.md index 7f3b501f..313f6f15 100644 --- a/docs/how-to/choose-a-preset.md +++ b/docs/how-to/choose-a-preset.md @@ -11,7 +11,7 @@ context-guru-proxy --preset codesmart # or PRESET=codesmart, or preset: in | Your workload | Preset | |---|---| -| **Most agents — the default** | **`codesmart`** | +| **Most agents — the recommended pipeline** | **`codesmart`** (pass `--preset codesmart`; the binary defaults to `house`) | | Same, but no LLM on the hot path | `codesafe` | | A guaranteed-safe, lossless win only | `safe` | | General non-agentic traffic | `balanced` | diff --git a/docs/how-to/use-with-claude-code.md b/docs/how-to/use-with-claude-code.md index 2a7cf217..ff3eaefd 100644 --- a/docs/how-to/use-with-claude-code.md +++ b/docs/how-to/use-with-claude-code.md @@ -81,7 +81,7 @@ ANTHROPIC_BASE_URL=https://127.0.0.1:1/nope ANTHROPIC_AUTH_TOKEN=bogus \ claude -p 'say PONG' --max-turns 1 # must FAIL ``` -**Which preset?** `codesmart` is the default and the cheapest arm in the +**Which preset?** `codesmart` is the recommended pipeline and the cheapest arm in the [benchmarks](../RESULTS.md) at the highest reward. Use `coding` if you want `skeleton` to strip function bodies out of large source reads — it needs a `cg_skeleton` build. See [Choose a preset](choose-a-preset.md). diff --git a/docs/reference/config.md b/docs/reference/config.md index a723248d..51717b72 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -85,7 +85,7 @@ for every component's config block. | `OPENAI_API_KEY` / `ANTHROPIC_API_KEY` | — | Real key injected on forward (gateway mode); empty = pass client auth through. | | `CHEAP_MODEL` (+ `CHEAP_MODEL_BASE` / `_KEY` / `_AUTH` / `_PROVIDER`) | — | Dedicated cheap model for the LLM components (`extract_llm`, `summarize`) — the `model.source: config` client. Without it they no-op. | | `FORCE_MODEL` | — | Overwrite the request `model` (eval-containers uses `EVAL_MODEL`). | -| `INJECT_EXPAND` | `auto` | Whether the `context_guru_expand` tool is advertised: `auto` (whenever the request already declares tools and the store persists — both session-stable, so the `tools` array never changes shape mid-session and the prompt-cache prefix survives) \| `always` (also when the request declares no tools, creating the array) \| `never`. | +| `INJECT_EXPAND` | `auto` | Whether the `context_guru_expand` tool is advertised: `auto` (whenever the request declares tools, the store persists, **and the pipeline contains at least one Offload** — all three session-stable, so the `tools` array never changes shape mid-session and the prompt-cache prefix survives) \| `always` (unconditional, including when the request declares no tools) \| `never`. The pipeline condition exists because a pipeline that mints no markers would be advertising a tool whose every call must fail. | | `CACHE_MODE` | `auto` | Cache-aware compaction: `auto` (on when the agent sets its own breakpoints) \| `on` \| `off`. | | `MODEL_INFO_URL` / `MODEL_INFO` | LiteLLM map | Source for context-window sizes (used by the fractional triggers). `MODEL_INFO=off` disables the lookup; fractions are then ignored and absolutes apply. | | `MODEL_PRICES` | — | Path to an **operator price list**, consulted before the public map. See below. A file that fails to load is fatal. | diff --git a/docs/setup.md b/docs/setup.md index ae7510e5..aa63ef66 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -8,10 +8,14 @@ SWE-bench task driven by Claude Code. - **Go 1.26**. A **C toolchain** is needed only for the `cg_skeleton` tag used below (tree-sitter), and for `make test`'s race detector — *not* for the binary itself, which is pure Go and statically linked. bifrost's tokenizer does **not** use cgo: o200k_base is - embedded (`internal/tokens/tokens.go`), and a default `CGO_ENABLED=0` build of this repo - succeeds for all four `GOOS`/`GOARCH` targets we care about. CI asserts it on every PR with no C - compiler on `PATH` at all (`.github/workflows/ci.yaml`, the `purego` job), so this cannot rot - back into a false claim. + embedded (`internal/tokens/tokens.go`). CI asserts the pure-Go build on every PR — natively, for + linux/amd64 — in the `purego` job (`.github/workflows/ci.yaml`), which builds with + `CGO_ENABLED=0`, checks the artifact is statically linked, starts it and probes `/healthz`. So the + claim cannot rot back into a false one for the platform CI runs on. + + Cross-compilation to the other three release targets (linux/arm64, darwin/amd64, darwin/arm64) is + **not** covered by that job: it was verified by hand on go 1.26.4 and is asserted at release time + by the tag workflow, not per PR. - **Docker** (for the gateway image / eval-containers), and the **eval-containers** repo. ## Build diff --git a/proxy/counttokens_test.go b/proxy/counttokens_test.go index d1009fe2..14e73518 100644 --- a/proxy/counttokens_test.go +++ b/proxy/counttokens_test.go @@ -58,10 +58,6 @@ func TestCountTokensIsServed(t *testing.T) { } } -// TestCountTokensForwardsUpstreamErrors: same wording-preservation rule as the chat route. A -// client that cannot read the real error here has no way to tell a malformed request from an -// auth failure. - // TestCountTokensForwardsUpstreamErrors: same wording-preservation rule as the chat route. A // client that cannot read the real error here has no way to tell a malformed request from an // auth failure. @@ -89,24 +85,3 @@ func TestCountTokensForwardsUpstreamErrors(t *testing.T) { t.Errorf("status/body not forwarded verbatim: %d %s", resp.StatusCode, got) } } - -// TestCachePresetAdvertisesNoExtraTool is the test that should have existed when the `cache` -// preset shipped, and did not. -// -// The preset's promise — repeated in `config/config.go`, `docs/reference/presets.md`, -// `docs/how-to/choose-a-preset.md`, `docs/how-to/install-plugin.md` and the install skill — is -// that nothing is added to the user's requests. It was false: `expand.Inject` under `auto` gated -// only on "the request declares tools" and "the store persists", so a cachesplit-only pipeline -// forwarded `context_guru_expand` to the provider. Measured before the fix: -// -// tools SENT by client : [Read Bash] -// tools FORWARDED upstream: [Read Bash context_guru_expand] -// -// That is not cosmetic. A pipeline with no Offload mints no markers, so EVERY expand call against -// it must fail — on a real session with marker-shaped text in a file, the model called it -// unprompted and got "[expand: original for id ... is no longer available]", costing a round trip -// and a step of the user's turn. -// -// TestCachePresetIsCachesplitAlone could not catch this: it asserts the presets MAP, while the -// injection happens in proxy.go after apply returns. This one reads the bytes that leave the -// process, which is the only place the claim is actually true or false. diff --git a/proxy/expandgate_test.go b/proxy/expandgate_test.go index 989c7114..483a9b5e 100644 --- a/proxy/expandgate_test.go +++ b/proxy/expandgate_test.go @@ -12,26 +12,28 @@ import ( "testing" ) -// TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist is the test that should have existed when the `cache` -// preset shipped, and did not. +// TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist pins the gate this change adds. // -// The preset's promise — repeated in `config/config.go`, `docs/reference/presets.md`, -// `docs/how-to/choose-a-preset.md`, `docs/how-to/install-plugin.md` and the install skill — is -// that nothing is added to the user's requests. It was false: `expand.Inject` under `auto` gated -// only on "the request declares tools" and "the store persists", so a cachesplit-only pipeline -// forwarded `context_guru_expand` to the provider. Measured before the fix: +// `expand.Inject` under `auto` gated on two things — the request declares tools, and the store +// persists. Nothing asked whether the pipeline could produce a `<>` marker at all, so an +// offloader-free pipeline advertised `context_guru_expand` to the provider. Measured before the +// fix, on a cachesplit-only pipeline: // // tools SENT by client : [Read Bash] // tools FORWARDED upstream: [Read Bash context_guru_expand] // // That is not cosmetic. A pipeline with no Offload mints no markers, so EVERY expand call against -// it must fail — on a real session with marker-shaped text in a file, the model called it -// unprompted and got "[expand: original for id ... is no longer available]", costing a round trip -// and a step of the user's turn. +// it must fail — on a real session with marker-shaped text in a file (this repo's own docs contain +// literal `<>`), the model called it unprompted and got "[expand: original for id ... is +// no longer available]", costing a round trip and a step of the user's turn. // -// TestCachePresetIsCachesplitAlone could not catch this: it asserts the presets MAP, while the -// injection happens in proxy.go after apply returns. This one reads the bytes that leave the -// process, which is the only place the claim is actually true or false. +// It has to be asserted HERE, on the bytes that leave the process. Any test of the presets map or +// of the built pipeline would pass throughout: the injection happens in proxy.go, after apply has +// already returned. +// +// The `mcp` case below is the one that keeps this honest. It looks offloader-free and is not, so a +// gate written against a list of preset names would get it wrong — as the first draft of this test +// did. func TestExpandToolIsAdvertisedOnlyWhereMarkersCanExist(t *testing.T) { for _, c := range []struct { name string diff --git a/proxy/proxy.go b/proxy/proxy.go index e6ee5def..5d65a967 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -91,9 +91,9 @@ type Options struct { // // This comment used to claim a marker condition that expand.Inject explicitly disclaims, // and the pipeline condition was missing entirely. The consequence was live: a - // cachesplit-only pipeline (the `cache` preset) forwarded `context_guru_expand` to the - // provider, a model that saw marker-shaped text in a file called it, and the call could - // only ever fail because nothing in that pipeline mints a marker to resolve. + // cachesplit-only pipeline forwarded `context_guru_expand` to the provider, a model that + // saw marker-shaped text in a file called it, and the call could only ever fail because + // nothing in that pipeline mints a marker to resolve. // // `always` still injects unconditionally — an operator asking for it by name gets it. InjectExpand string @@ -1161,12 +1161,18 @@ func (h *Handler) chat(provider bschemas.ModelProvider, static upstream, pick fu im = expand.InjectAuto } // Under `auto`, advertise only when THIS request's pipeline can actually mint a - // marker. Without this, an offloader-free pipeline (`cache`, `off`, `safe`, - // `mcp`) declared a tool whose every use is guaranteed to fail — measured: - // `[Read Bash]` in, `[Read Bash context_guru_expand]` out, and on a transcript - // containing marker-shaped text the model duly called it and got + // marker. Without it, an offloader-free pipeline — `off`, `safe`, or any + // cachesplit-only configuration — declared a tool whose every use is guaranteed + // to fail: measured `[Read Bash]` in, `[Read Bash context_guru_expand]` out, and + // on a transcript containing marker-shaped text the model duly called it and got // "[expand: original for id ... is no longer available]". // + // Which presets those are is NOT a list worth writing down here — `mcp` looks + // offloader-free and is not, because `smartcrush` implements components.Offload. + // That is exactly why the gate asks the interface (Pipeline.HasOffload) instead + // of naming presets: a list in a comment is a second source of truth, and this + // one was wrong about `mcp` in its first draft. + // // `off` mattered most: it is the A/B control arm, and a control that carries an // extra tool declaration is not a control. if im != expand.InjectAuto || tn.Pipe.HasOffload() { diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 28633356..956e5ccf 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -40,7 +40,8 @@ func buildHandler(t *testing.T, yaml string, upstream string) (*proxy.Handler, s // // Advertising that tool is gated on the pipeline containing an Offload, because a pipeline that // mints no markers would be declaring a tool whose every call must fail (the defect that shipped -// in the `cache` and `off` presets). A `pipeline: []` fixture therefore no longer advertises it, +// in `off` — the A/B control arm — and in `safe`). A `pipeline: []` fixture therefore no longer +// advertises it, // and a test that hand-seeds the Store to simulate an offload has to use a pipeline where the // offload it is simulating could actually have happened. //