feat(dist): pure-Go releases, a cache preset, --idle-exit, and gateway conformance - #141
feat(dist): pure-Go releases, a cache preset, --idle-exit, and gateway conformance#141amiddavid wants to merge 1 commit into
cache preset, --idle-exit, and gateway conformance#141Conversation
92631bf to
6f10c82
Compare
|
Reviewed against main The headline claim holds, and the compaction core is sound. Almost everything below is in the plugin shell, not the core. 1. Blocking —
|
|
Two additional findings that landed after my main review, both in 11. Blocking — the checksum verification is fail-open, so an unverified binary installs and runs
An unverified binary landed on a PATH directory and executed — and this binary then handles all of Three things make it worse than a missing check:
Fix: 12. Blocking — there is no upgrade path, on the PR that creates the release channel
( For the PR whose purpose is a release channel, "installs once, never upgradable" is the gap that Addendum on the
|
| Zero-value condition | Detected | Documented |
|---|---|---|
| Non-Anthropic backend | no | yes (install-plugin.md:67, status/SKILL.md:71) |
| System prompt under the 1,024-token floor | no | no |
| Not a git repository | no | nowhere |
Row 3 is the common case for a casual trial, and it reproduces live: non-git cwd →
cachesplit mutated=0 verdict=skipped; the same task inside a git repo → mutated=2 verdict=moved.
status/SKILL.md:65-73 lists four honest reasons the numbers may be flat and the one that actually
applies is not among them — so a first-run user outside a git repo is shown a zero and told the cache
warms on later turns. True in general, wrong there. A fifth bullet plus
git rev-parse --is-inside-work-tree in the status skill closes it.
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. ## 1. The toolchain gate was our own documentation `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. ## 2. A `cache` preset: cachesplit alone The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. ## 3. `--idle-exit`: the proxy cleans itself up Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. ## 4. Gateway conformance All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. ## 5. Review of #141: the `cache` preset advertised a tool whose every call must fail Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. ## 6. The rest of the review - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. ## Verification Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
6f10c82 to
5e344d4
Compare
|
Thank you — this was an unusually useful review, and three of the findings were things no test of mine would have caught. Split as you asked, into this PR (core) and #160 (the plugin): every blocking finding was in the plugin, so nothing clean is held behind it. I went with two rather than five because Where each finding went:
I verified finding 4 before fixing it rather than taking it on faith, and got your result exactly:
Two places I did not do what you suggested, both with reasons:
On the −34.1% figure — you were right and it mattered. Both numbers are now stated with their regimes, along with the three zero cases; the old citation pointed at Not fixed, deliberately: Still unverified, and the thing I would not paper over: the plugin has never been installed into a real Claude Code session, because Also from your review: the stale preset tables are #142 (now guarded by a set-equality drift test), and the prose instances the guard cannot reach are #143. |
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. ## 1. The toolchain gate was our own documentation `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. ## 2. A `cache` preset: cachesplit alone The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. ## 3. `--idle-exit`: the proxy cleans itself up Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. ## 4. Gateway conformance All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. ## 5. Review of #141: the `cache` preset advertised a tool whose every call must fail Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. ## 6. The rest of the review - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. ## Verification Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
5e344d4 to
eadc015
Compare
…view's blockers fixed Split out of #141 as its own PR: all six of the review's blocking findings were in the plugin, and the release plumbing and conformance work should not wait behind them. The core lands in #141. `/plugin marketplace add rossoctl/context-guru` → `/plugin install` → `/context-guru:install`. Three skills over four scripts and two hooks. Default routing scope is `.claude/settings.local.json`: one repo, gitignored, `--global` an explicit opt-in — a base URL pointing at localhost breaks Claude Code everywhere a dead proxy is routed. ## The six blocking findings **1. `/context-guru:uninstall` killed the user's own session and left the proxy running.** It ran `pkill -f "context-guru-proxy.*${PORT}"`. The port was passed through `LISTEN_ADDR` in the environment, so it appeared nowhere in the proxy's command line and the pattern matched no proxy — while it DID match the shell running the `pkill`, i.e. the session's own Bash tool. A user runs uninstall *because* their sessions are broken; this killed the session mid-command, reported nothing removed, and left the port held. Fixed with a handle rather than a better pattern: the starter passes `--listen` (so the port is in `argv` and `ps` is honest) and writes a pidfile under `~/.local/state/context-guru`; uninstall kills that PID, falls back to the socket's owner via `lsof`/`ss`, and confirms the process is ours before killing anything. The skill also no longer offers a broader pattern as a fallback — on a host running a production instance or a benchmark arm, that would take those down too. **2. `install.sh` could not install anything, and its documented fallback was missing.** Strict checksums now; `download_failed` (a tag with no assets) is documented as an outcome; the `go install` fallback the header comment described is implemented; curl's stderr no longer breaks the `key=value` contract the skill parses. **3. A dead proxy is a silent, indefinite hang** — no output on either stream — and `/context-guru:status` cannot diagnose it, because invoking a skill needs a model call, which is the broken thing. New `check-proxy.sh` on `UserPromptSubmit`: it probes `/healthz`, tries to restart, and otherwise prints what to do. A hook is the only thing that runs without a model turn. It never blocks a prompt. **4. The `cache` preset advertised `context_guru_expand`.** Fixed in #141 (the gate belongs in the proxy); the docs and the install skill here no longer claim otherwise where they were wrong. **5. `settings.py` destroyed the user's undo, and uninstall did not restore what it replaced.** The backup stamp was second-granularity with an overwriting `copy2`, so an install→uninstall round trip wrote both backups to the same path and the survivor held the POST-install state — the value it existed to protect was gone from the file AND the backup. Now microsecond-stamped and created with `O_EXCL`. And `replaced` was reported then forgotten, so after a `--force` install over somebody's gateway, uninstall left them with no base URL at all; the replaced value is now recorded and restored. `is_ours` deserves a note. The review suggested matching `http://(127.0.0.1|localhost|[::1]):\\d+ /anthropic` as ours, to stop a port change reporting a conflict against context-guru itself. A test caught why that is wrong: litellm's default is `http://127.0.0.1:4000/anthropic`, so a URL-shape rule would let uninstall delete somebody else's routing. Two local proxies are indistinguishable by URL, so `add` records the URL it wrote and later runs read that record. Anything unrecorded stays a conflict — for both add and remove. **6. The atomic write widened a credential-bearing file's mode** from 600 to 644 under the common umask, and `os.replace` onto a symlinked `settings.json` replaced the LINK with a regular file, so a dotfile-managed setup silently never received the edit. Mode is preserved; the path is resolved first. ## Smaller review items - **`start-proxy.sh` printed a dead dashboard link** — it advertised `/dashboard/` and never passed `--dashboard`, so the first line the plugin ever prints was a 404. Now passed, with `--dashboard-db` under the state directory: the default would write `./context-guru-dashboard.db` into the user's repository. - **Backups accumulated forever** (one per add and per remove). Pruned to the newest 10. - **The zero-value cases are now stated** where a first-run user reads them, and `status` checks the one that is both commonest and previously undocumented: **outside a git repository** there is no environment snapshot, so `cachesplit` skips and the saving is exactly zero. The status skill also no longer treats `acted: 0` / `savings_pct: 0` as a verdict — those count content removal, and this component relocates a breakpoint. - **`--idle-exit`'s 24h is the plugin's value, not the flag's default** (which is 0 = never). Said so, along with probes not counting as activity. - Upgrade path documented (`CONTEXT_GURU_UPGRADE=1`, `CONTEXT_GURU_VERSION`). ## Verification The scripts are tested from Go (`context-guru-plugin/plugin_test.go`) so `go test ./...` and CI cover them. Seven mutations, each proven to have landed before its result counted: backup() back to overwriting copy2 -> TestBackupsDoNotClobberEachOther FAIL "both operations reported the same backup path ..., so one overwrote the other" uninstall stops restoring -> TestUninstallRestoresTheBaseURLItReplaced FAIL restored="" want "https://gateway.corp.example/anthropic"; env left {ANTHROPIC_AUTH_TOKEN:keep} mode no longer preserved -> TestSettingsPreservesFileMode FAIL realpath removed -> TestSettingsFollowsASymlink FAIL checksum fail-open again -> TestInstallRefusesAnUnverifiedDownload FAIL port back in the environment -> TestHookMakesTheProxyIdentifiable FAIL pidfile no longer written -> TestHookMakesTheProxyIdentifiable FAIL One of those is worth recording as a process note: my first attempt at the backup mutation reverted only the timestamp granularity and left the `O_EXCL` retry loop in place, so the name was still unique and the test passed — proving nothing. Reverting half a fix is its own way to get a vacuous result. The run above restores the original function whole. Pre-existing coverage still passes: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for `/healthz`. **Still not verified end to end in a real Claude Code session**, because `install.sh` resolves a GitHub release and no tag has published assets yet. That is the first thing to do once #141 merges and a tag exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…view's blockers fixed Split out of #141 as its own PR: all six of the review's blocking findings were in the plugin, and the release plumbing and conformance work should not wait behind them. The core lands in #141. `/plugin marketplace add rossoctl/context-guru` → `/plugin install` → `/context-guru:install`. Three skills over four scripts and two hooks. Default routing scope is `.claude/settings.local.json`: one repo, gitignored, `--global` an explicit opt-in — a base URL pointing at localhost breaks Claude Code everywhere a dead proxy is routed. ## The six blocking findings **1. `/context-guru:uninstall` killed the user's own session and left the proxy running.** It ran `pkill -f "context-guru-proxy.*${PORT}"`. The port was passed through `LISTEN_ADDR` in the environment, so it appeared nowhere in the proxy's command line and the pattern matched no proxy — while it DID match the shell running the `pkill`, i.e. the session's own Bash tool. A user runs uninstall *because* their sessions are broken; this killed the session mid-command, reported nothing removed, and left the port held. Fixed with a handle rather than a better pattern: the starter passes `--listen` (so the port is in `argv` and `ps` is honest) and writes a pidfile under `~/.local/state/context-guru`; uninstall kills that PID, falls back to the socket's owner via `lsof`/`ss`, and confirms the process is ours before killing anything. The skill also no longer offers a broader pattern as a fallback — on a host running a production instance or a benchmark arm, that would take those down too. **2. `install.sh` could not install anything, and its documented fallback was missing.** Strict checksums now; `download_failed` (a tag with no assets) is documented as an outcome; the `go install` fallback the header comment described is implemented; curl's stderr no longer breaks the `key=value` contract the skill parses. **3. A dead proxy is a silent, indefinite hang** — no output on either stream — and `/context-guru:status` cannot diagnose it, because invoking a skill needs a model call, which is the broken thing. New `check-proxy.sh` on `UserPromptSubmit`: it probes `/healthz`, tries to restart, and otherwise prints what to do. A hook is the only thing that runs without a model turn. It never blocks a prompt. **4. The `cache` preset advertised `context_guru_expand`.** Fixed in #141 (the gate belongs in the proxy); the docs and the install skill here no longer claim otherwise where they were wrong. **5. `settings.py` destroyed the user's undo, and uninstall did not restore what it replaced.** The backup stamp was second-granularity with an overwriting `copy2`, so an install→uninstall round trip wrote both backups to the same path and the survivor held the POST-install state — the value it existed to protect was gone from the file AND the backup. Now microsecond-stamped and created with `O_EXCL`. And `replaced` was reported then forgotten, so after a `--force` install over somebody's gateway, uninstall left them with no base URL at all; the replaced value is now recorded and restored. `is_ours` deserves a note. The review suggested matching `http://(127.0.0.1|localhost|[::1]):\\d+ /anthropic` as ours, to stop a port change reporting a conflict against context-guru itself. A test caught why that is wrong: litellm's default is `http://127.0.0.1:4000/anthropic`, so a URL-shape rule would let uninstall delete somebody else's routing. Two local proxies are indistinguishable by URL, so `add` records the URL it wrote and later runs read that record. Anything unrecorded stays a conflict — for both add and remove. **6. The atomic write widened a credential-bearing file's mode** from 600 to 644 under the common umask, and `os.replace` onto a symlinked `settings.json` replaced the LINK with a regular file, so a dotfile-managed setup silently never received the edit. Mode is preserved; the path is resolved first. ## Smaller review items - **`start-proxy.sh` printed a dead dashboard link** — it advertised `/dashboard/` and never passed `--dashboard`, so the first line the plugin ever prints was a 404. Now passed, with `--dashboard-db` under the state directory: the default would write `./context-guru-dashboard.db` into the user's repository. - **Backups accumulated forever** (one per add and per remove). Pruned to the newest 10. - **The zero-value cases are now stated** where a first-run user reads them, and `status` checks the one that is both commonest and previously undocumented: **outside a git repository** there is no environment snapshot, so `cachesplit` skips and the saving is exactly zero. The status skill also no longer treats `acted: 0` / `savings_pct: 0` as a verdict — those count content removal, and this component relocates a breakpoint. - **`--idle-exit`'s 24h is the plugin's value, not the flag's default** (which is 0 = never). Said so, along with probes not counting as activity. - Upgrade path documented (`CONTEXT_GURU_UPGRADE=1`, `CONTEXT_GURU_VERSION`). ## Verification The scripts are tested from Go (`context-guru-plugin/plugin_test.go`) so `go test ./...` and CI cover them. Seven mutations, each proven to have landed before its result counted: backup() back to overwriting copy2 -> TestBackupsDoNotClobberEachOther FAIL "both operations reported the same backup path ..., so one overwrote the other" uninstall stops restoring -> TestUninstallRestoresTheBaseURLItReplaced FAIL restored="" want "https://gateway.corp.example/anthropic"; env left {ANTHROPIC_AUTH_TOKEN:keep} mode no longer preserved -> TestSettingsPreservesFileMode FAIL realpath removed -> TestSettingsFollowsASymlink FAIL checksum fail-open again -> TestInstallRefusesAnUnverifiedDownload FAIL port back in the environment -> TestHookMakesTheProxyIdentifiable FAIL pidfile no longer written -> TestHookMakesTheProxyIdentifiable FAIL One of those is worth recording as a process note: my first attempt at the backup mutation reverted only the timestamp granularity and left the `O_EXCL` retry loop in place, so the name was still unique and the test passed — proving nothing. Reverting half a fix is its own way to get a vacuous result. The run above restores the original function whole. Pre-existing coverage still passes: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for `/healthz`. **Still not verified end to end in a real Claude Code session**, because `install.sh` resolves a GitHub release and no tag has published assets yet. That is the first thing to do once #141 merges and a tag exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
eadc015 to
e624e0c
Compare
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
e624e0c to
61827f6
Compare
…view's blockers fixed Split out of #141 as its own PR: all six of the review's blocking findings were in the plugin, and the release plumbing and conformance work should not wait behind them. The core lands in #141. `/plugin marketplace add rossoctl/context-guru` → `/plugin install` → `/context-guru:install`. Three skills over four scripts and two hooks. Default routing scope is `.claude/settings.local.json`: one repo, gitignored, `--global` an explicit opt-in — a base URL pointing at localhost breaks Claude Code everywhere a dead proxy is routed. ## The six blocking findings **1. `/context-guru:uninstall` killed the user's own session and left the proxy running.** It ran `pkill -f "context-guru-proxy.*${PORT}"`. The port was passed through `LISTEN_ADDR` in the environment, so it appeared nowhere in the proxy's command line and the pattern matched no proxy — while it DID match the shell running the `pkill`, i.e. the session's own Bash tool. A user runs uninstall *because* their sessions are broken; this killed the session mid-command, reported nothing removed, and left the port held. Fixed with a handle rather than a better pattern: the starter passes `--listen` (so the port is in `argv` and `ps` is honest) and writes a pidfile under `~/.local/state/context-guru`; uninstall kills that PID, falls back to the socket's owner via `lsof`/`ss`, and confirms the process is ours before killing anything. The skill also no longer offers a broader pattern as a fallback — on a host running a production instance or a benchmark arm, that would take those down too. **2. `install.sh` could not install anything, and its documented fallback was missing.** Strict checksums now; `download_failed` (a tag with no assets) is documented as an outcome; the `go install` fallback the header comment described is implemented; curl's stderr no longer breaks the `key=value` contract the skill parses. **3. A dead proxy is a silent, indefinite hang** — no output on either stream — and `/context-guru:status` cannot diagnose it, because invoking a skill needs a model call, which is the broken thing. New `check-proxy.sh` on `UserPromptSubmit`: it probes `/healthz`, tries to restart, and otherwise prints what to do. A hook is the only thing that runs without a model turn. It never blocks a prompt. **4. The `cache` preset advertised `context_guru_expand`.** Fixed in #141 (the gate belongs in the proxy); the docs and the install skill here no longer claim otherwise where they were wrong. **5. `settings.py` destroyed the user's undo, and uninstall did not restore what it replaced.** The backup stamp was second-granularity with an overwriting `copy2`, so an install→uninstall round trip wrote both backups to the same path and the survivor held the POST-install state — the value it existed to protect was gone from the file AND the backup. Now microsecond-stamped and created with `O_EXCL`. And `replaced` was reported then forgotten, so after a `--force` install over somebody's gateway, uninstall left them with no base URL at all; the replaced value is now recorded and restored. `is_ours` deserves a note. The review suggested matching `http://(127.0.0.1|localhost|[::1]):\\d+ /anthropic` as ours, to stop a port change reporting a conflict against context-guru itself. A test caught why that is wrong: litellm's default is `http://127.0.0.1:4000/anthropic`, so a URL-shape rule would let uninstall delete somebody else's routing. Two local proxies are indistinguishable by URL, so `add` records the URL it wrote and later runs read that record. Anything unrecorded stays a conflict — for both add and remove. **6. The atomic write widened a credential-bearing file's mode** from 600 to 644 under the common umask, and `os.replace` onto a symlinked `settings.json` replaced the LINK with a regular file, so a dotfile-managed setup silently never received the edit. Mode is preserved; the path is resolved first. ## Smaller review items - **`start-proxy.sh` printed a dead dashboard link** — it advertised `/dashboard/` and never passed `--dashboard`, so the first line the plugin ever prints was a 404. Now passed, with `--dashboard-db` under the state directory: the default would write `./context-guru-dashboard.db` into the user's repository. - **Backups accumulated forever** (one per add and per remove). Pruned to the newest 10. - **The zero-value cases are now stated** where a first-run user reads them, and `status` checks the one that is both commonest and previously undocumented: **outside a git repository** there is no environment snapshot, so `cachesplit` skips and the saving is exactly zero. The status skill also no longer treats `acted: 0` / `savings_pct: 0` as a verdict — those count content removal, and this component relocates a breakpoint. - **`--idle-exit`'s 24h is the plugin's value, not the flag's default** (which is 0 = never). Said so, along with probes not counting as activity. - Upgrade path documented (`CONTEXT_GURU_UPGRADE=1`, `CONTEXT_GURU_VERSION`). ## Verification The scripts are tested from Go (`context-guru-plugin/plugin_test.go`) so `go test ./...` and CI cover them. Seven mutations, each proven to have landed before its result counted: backup() back to overwriting copy2 -> TestBackupsDoNotClobberEachOther FAIL "both operations reported the same backup path ..., so one overwrote the other" uninstall stops restoring -> TestUninstallRestoresTheBaseURLItReplaced FAIL restored="" want "https://gateway.corp.example/anthropic"; env left {ANTHROPIC_AUTH_TOKEN:keep} mode no longer preserved -> TestSettingsPreservesFileMode FAIL realpath removed -> TestSettingsFollowsASymlink FAIL checksum fail-open again -> TestInstallRefusesAnUnverifiedDownload FAIL port back in the environment -> TestHookMakesTheProxyIdentifiable FAIL pidfile no longer written -> TestHookMakesTheProxyIdentifiable FAIL One of those is worth recording as a process note: my first attempt at the backup mutation reverted only the timestamp granularity and left the `O_EXCL` retry loop in place, so the name was still unique and the test passed — proving nothing. Reverting half a fix is its own way to get a vacuous result. The run above restores the original function whole. Pre-existing coverage still passes: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for `/healthz`. **Still not verified end to end in a real Claude Code session**, because `install.sh` resolves a GitHub release and no tag has published assets yet. That is the first thing to do once #141 merges and a tag exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
|
Restructured per your ask: the pre-existing defects are now #161, and this PR keeps only distribution work. #161 carries the expand-tool gate (including It also adds a Two things from doing the split that you should know, both mine:
Also filed #162: #143 duplicates #145 and should be closed in its favour — I filed it without checking the tracker. |
cache preset, --idle-exit, a Claude Code plugin, and gateway conformancecache preset, --idle-exit, and gateway conformance
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
61827f6 to
f1af7f2
Compare
…view's blockers fixed Split out of #141 as its own PR: all six of the review's blocking findings were in the plugin, and the release plumbing and conformance work should not wait behind them. The core lands in #141. `/plugin marketplace add rossoctl/context-guru` → `/plugin install` → `/context-guru:install`. Three skills over four scripts and two hooks. Default routing scope is `.claude/settings.local.json`: one repo, gitignored, `--global` an explicit opt-in — a base URL pointing at localhost breaks Claude Code everywhere a dead proxy is routed. ## The six blocking findings **1. `/context-guru:uninstall` killed the user's own session and left the proxy running.** It ran `pkill -f "context-guru-proxy.*${PORT}"`. The port was passed through `LISTEN_ADDR` in the environment, so it appeared nowhere in the proxy's command line and the pattern matched no proxy — while it DID match the shell running the `pkill`, i.e. the session's own Bash tool. A user runs uninstall *because* their sessions are broken; this killed the session mid-command, reported nothing removed, and left the port held. Fixed with a handle rather than a better pattern: the starter passes `--listen` (so the port is in `argv` and `ps` is honest) and writes a pidfile under `~/.local/state/context-guru`; uninstall kills that PID, falls back to the socket's owner via `lsof`/`ss`, and confirms the process is ours before killing anything. The skill also no longer offers a broader pattern as a fallback — on a host running a production instance or a benchmark arm, that would take those down too. **2. `install.sh` could not install anything, and its documented fallback was missing.** Strict checksums now; `download_failed` (a tag with no assets) is documented as an outcome; the `go install` fallback the header comment described is implemented; curl's stderr no longer breaks the `key=value` contract the skill parses. **3. A dead proxy is a silent, indefinite hang** — no output on either stream — and `/context-guru:status` cannot diagnose it, because invoking a skill needs a model call, which is the broken thing. New `check-proxy.sh` on `UserPromptSubmit`: it probes `/healthz`, tries to restart, and otherwise prints what to do. A hook is the only thing that runs without a model turn. It never blocks a prompt. **4. The `cache` preset advertised `context_guru_expand`.** Fixed in #141 (the gate belongs in the proxy); the docs and the install skill here no longer claim otherwise where they were wrong. **5. `settings.py` destroyed the user's undo, and uninstall did not restore what it replaced.** The backup stamp was second-granularity with an overwriting `copy2`, so an install→uninstall round trip wrote both backups to the same path and the survivor held the POST-install state — the value it existed to protect was gone from the file AND the backup. Now microsecond-stamped and created with `O_EXCL`. And `replaced` was reported then forgotten, so after a `--force` install over somebody's gateway, uninstall left them with no base URL at all; the replaced value is now recorded and restored. `is_ours` deserves a note. The review suggested matching `http://(127.0.0.1|localhost|[::1]):\\d+ /anthropic` as ours, to stop a port change reporting a conflict against context-guru itself. A test caught why that is wrong: litellm's default is `http://127.0.0.1:4000/anthropic`, so a URL-shape rule would let uninstall delete somebody else's routing. Two local proxies are indistinguishable by URL, so `add` records the URL it wrote and later runs read that record. Anything unrecorded stays a conflict — for both add and remove. **6. The atomic write widened a credential-bearing file's mode** from 600 to 644 under the common umask, and `os.replace` onto a symlinked `settings.json` replaced the LINK with a regular file, so a dotfile-managed setup silently never received the edit. Mode is preserved; the path is resolved first. ## Smaller review items - **`start-proxy.sh` printed a dead dashboard link** — it advertised `/dashboard/` and never passed `--dashboard`, so the first line the plugin ever prints was a 404. Now passed, with `--dashboard-db` under the state directory: the default would write `./context-guru-dashboard.db` into the user's repository. - **Backups accumulated forever** (one per add and per remove). Pruned to the newest 10. - **The zero-value cases are now stated** where a first-run user reads them, and `status` checks the one that is both commonest and previously undocumented: **outside a git repository** there is no environment snapshot, so `cachesplit` skips and the saving is exactly zero. The status skill also no longer treats `acted: 0` / `savings_pct: 0` as a verdict — those count content removal, and this component relocates a breakpoint. - **`--idle-exit`'s 24h is the plugin's value, not the flag's default** (which is 0 = never). Said so, along with probes not counting as activity. - Upgrade path documented (`CONTEXT_GURU_UPGRADE=1`, `CONTEXT_GURU_VERSION`). ## Verification The scripts are tested from Go (`context-guru-plugin/plugin_test.go`) so `go test ./...` and CI cover them. Seven mutations, each proven to have landed before its result counted: backup() back to overwriting copy2 -> TestBackupsDoNotClobberEachOther FAIL "both operations reported the same backup path ..., so one overwrote the other" uninstall stops restoring -> TestUninstallRestoresTheBaseURLItReplaced FAIL restored="" want "https://gateway.corp.example/anthropic"; env left {ANTHROPIC_AUTH_TOKEN:keep} mode no longer preserved -> TestSettingsPreservesFileMode FAIL realpath removed -> TestSettingsFollowsASymlink FAIL checksum fail-open again -> TestInstallRefusesAnUnverifiedDownload FAIL port back in the environment -> TestHookMakesTheProxyIdentifiable FAIL pidfile no longer written -> TestHookMakesTheProxyIdentifiable FAIL One of those is worth recording as a process note: my first attempt at the backup mutation reverted only the timestamp granularity and left the `O_EXCL` retry loop in place, so the name was still unique and the test passed — proving nothing. Reverting half a fix is its own way to get a vacuous result. The run above restores the original function whole. Pre-existing coverage still passes: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for `/healthz`. **Still not verified end to end in a real Claude Code session**, because `install.sh` resolves a GitHub release and no tag has published assets yet. That is the first thing to do once #141 merges and a tag exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
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 `<<cg:HASH>>` 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 `<<cg:HASH>>`), 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) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
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) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
f1af7f2 to
b90ad0c
Compare
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…view's blockers fixed Split out of #141 as its own PR: all six of the review's blocking findings were in the plugin, and the release plumbing and conformance work should not wait behind them. The core lands in #141. `/plugin marketplace add rossoctl/context-guru` → `/plugin install` → `/context-guru:install`. Three skills over four scripts and two hooks. Default routing scope is `.claude/settings.local.json`: one repo, gitignored, `--global` an explicit opt-in — a base URL pointing at localhost breaks Claude Code everywhere a dead proxy is routed. **1. `/context-guru:uninstall` killed the user's own session and left the proxy running.** It ran `pkill -f "context-guru-proxy.*${PORT}"`. The port was passed through `LISTEN_ADDR` in the environment, so it appeared nowhere in the proxy's command line and the pattern matched no proxy — while it DID match the shell running the `pkill`, i.e. the session's own Bash tool. A user runs uninstall *because* their sessions are broken; this killed the session mid-command, reported nothing removed, and left the port held. Fixed with a handle rather than a better pattern: the starter passes `--listen` (so the port is in `argv` and `ps` is honest) and writes a pidfile under `~/.local/state/context-guru`; uninstall kills that PID, falls back to the socket's owner via `lsof`/`ss`, and confirms the process is ours before killing anything. The skill also no longer offers a broader pattern as a fallback — on a host running a production instance or a benchmark arm, that would take those down too. **2. `install.sh` could not install anything, and its documented fallback was missing.** Strict checksums now; `download_failed` (a tag with no assets) is documented as an outcome; the `go install` fallback the header comment described is implemented; curl's stderr no longer breaks the `key=value` contract the skill parses. **3. A dead proxy is a silent, indefinite hang** — no output on either stream — and `/context-guru:status` cannot diagnose it, because invoking a skill needs a model call, which is the broken thing. New `check-proxy.sh` on `UserPromptSubmit`: it probes `/healthz`, tries to restart, and otherwise prints what to do. A hook is the only thing that runs without a model turn. It never blocks a prompt. **4. The `cache` preset advertised `context_guru_expand`.** Fixed in #141 (the gate belongs in the proxy); the docs and the install skill here no longer claim otherwise where they were wrong. **5. `settings.py` destroyed the user's undo, and uninstall did not restore what it replaced.** The backup stamp was second-granularity with an overwriting `copy2`, so an install→uninstall round trip wrote both backups to the same path and the survivor held the POST-install state — the value it existed to protect was gone from the file AND the backup. Now microsecond-stamped and created with `O_EXCL`. And `replaced` was reported then forgotten, so after a `--force` install over somebody's gateway, uninstall left them with no base URL at all; the replaced value is now recorded and restored. `is_ours` deserves a note. The review suggested matching `http://(127.0.0.1|localhost|[::1]):\\d+ /anthropic` as ours, to stop a port change reporting a conflict against context-guru itself. A test caught why that is wrong: litellm's default is `http://127.0.0.1:4000/anthropic`, so a URL-shape rule would let uninstall delete somebody else's routing. Two local proxies are indistinguishable by URL, so `add` records the URL it wrote and later runs read that record. Anything unrecorded stays a conflict — for both add and remove. **6. The atomic write widened a credential-bearing file's mode** from 600 to 644 under the common umask, and `os.replace` onto a symlinked `settings.json` replaced the LINK with a regular file, so a dotfile-managed setup silently never received the edit. Mode is preserved; the path is resolved first. - **`start-proxy.sh` printed a dead dashboard link** — it advertised `/dashboard/` and never passed `--dashboard`, so the first line the plugin ever prints was a 404. Now passed, with `--dashboard-db` under the state directory: the default would write `./context-guru-dashboard.db` into the user's repository. - **Backups accumulated forever** (one per add and per remove). Pruned to the newest 10. - **The zero-value cases are now stated** where a first-run user reads them, and `status` checks the one that is both commonest and previously undocumented: **outside a git repository** there is no environment snapshot, so `cachesplit` skips and the saving is exactly zero. The status skill also no longer treats `acted: 0` / `savings_pct: 0` as a verdict — those count content removal, and this component relocates a breakpoint. - **`--idle-exit`'s 24h is the plugin's value, not the flag's default** (which is 0 = never). Said so, along with probes not counting as activity. - Upgrade path documented (`CONTEXT_GURU_UPGRADE=1`, `CONTEXT_GURU_VERSION`). The scripts are tested from Go (`context-guru-plugin/plugin_test.go`) so `go test ./...` and CI cover them. Seven mutations, each proven to have landed before its result counted: backup() back to overwriting copy2 -> TestBackupsDoNotClobberEachOther FAIL "both operations reported the same backup path ..., so one overwrote the other" uninstall stops restoring -> TestUninstallRestoresTheBaseURLItReplaced FAIL restored="" want "https://gateway.corp.example/anthropic"; env left {ANTHROPIC_AUTH_TOKEN:keep} mode no longer preserved -> TestSettingsPreservesFileMode FAIL realpath removed -> TestSettingsFollowsASymlink FAIL checksum fail-open again -> TestInstallRefusesAnUnverifiedDownload FAIL port back in the environment -> TestHookMakesTheProxyIdentifiable FAIL pidfile no longer written -> TestHookMakesTheProxyIdentifiable FAIL One of those is worth recording as a process note: my first attempt at the backup mutation reverted only the timestamp granularity and left the `O_EXCL` retry loop in place, so the name was still unique and the test passed — proving nothing. Reverting half a fix is its own way to get a vacuous result. The run above restores the original function whole. Pre-existing coverage still passes: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for `/healthz`. **Still not verified end to end in a real Claude Code session**, because `install.sh` resolves a GitHub release and no tag has published assets yet. That is the first thing to do once #141 merges and a tag exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…pand-tool gate, count_tokens, C-toolchain claim, preset facts) (#161) * fix: defects that surfaced while building the distribution funnel 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 `<<cg:HASH>>` 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 `<<cg:HASH>>`), 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) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> * ci(purego): run one package binary at a time 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) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> * fix: make this PR's own prose true, and unit-test HasOffload 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) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com> --------- Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
…way conformance Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
b90ad0c to
7281324
Compare
…view's blockers fixed Split out of #141 as its own PR: all six of the review's blocking findings were in the plugin, and the release plumbing and conformance work should not wait behind them. The core lands in #141. `/plugin marketplace add rossoctl/context-guru` → `/plugin install` → `/context-guru:install`. Three skills over four scripts and two hooks. Default routing scope is `.claude/settings.local.json`: one repo, gitignored, `--global` an explicit opt-in — a base URL pointing at localhost breaks Claude Code everywhere a dead proxy is routed. **1. `/context-guru:uninstall` killed the user's own session and left the proxy running.** It ran `pkill -f "context-guru-proxy.*${PORT}"`. The port was passed through `LISTEN_ADDR` in the environment, so it appeared nowhere in the proxy's command line and the pattern matched no proxy — while it DID match the shell running the `pkill`, i.e. the session's own Bash tool. A user runs uninstall *because* their sessions are broken; this killed the session mid-command, reported nothing removed, and left the port held. Fixed with a handle rather than a better pattern: the starter passes `--listen` (so the port is in `argv` and `ps` is honest) and writes a pidfile under `~/.local/state/context-guru`; uninstall kills that PID, falls back to the socket's owner via `lsof`/`ss`, and confirms the process is ours before killing anything. The skill also no longer offers a broader pattern as a fallback — on a host running a production instance or a benchmark arm, that would take those down too. **2. `install.sh` could not install anything, and its documented fallback was missing.** Strict checksums now; `download_failed` (a tag with no assets) is documented as an outcome; the `go install` fallback the header comment described is implemented; curl's stderr no longer breaks the `key=value` contract the skill parses. **3. A dead proxy is a silent, indefinite hang** — no output on either stream — and `/context-guru:status` cannot diagnose it, because invoking a skill needs a model call, which is the broken thing. New `check-proxy.sh` on `UserPromptSubmit`: it probes `/healthz`, tries to restart, and otherwise prints what to do. A hook is the only thing that runs without a model turn. It never blocks a prompt. **4. The `cache` preset advertised `context_guru_expand`.** Fixed in #141 (the gate belongs in the proxy); the docs and the install skill here no longer claim otherwise where they were wrong. **5. `settings.py` destroyed the user's undo, and uninstall did not restore what it replaced.** The backup stamp was second-granularity with an overwriting `copy2`, so an install→uninstall round trip wrote both backups to the same path and the survivor held the POST-install state — the value it existed to protect was gone from the file AND the backup. Now microsecond-stamped and created with `O_EXCL`. And `replaced` was reported then forgotten, so after a `--force` install over somebody's gateway, uninstall left them with no base URL at all; the replaced value is now recorded and restored. `is_ours` deserves a note. The review suggested matching `http://(127.0.0.1|localhost|[::1]):\\d+ /anthropic` as ours, to stop a port change reporting a conflict against context-guru itself. A test caught why that is wrong: litellm's default is `http://127.0.0.1:4000/anthropic`, so a URL-shape rule would let uninstall delete somebody else's routing. Two local proxies are indistinguishable by URL, so `add` records the URL it wrote and later runs read that record. Anything unrecorded stays a conflict — for both add and remove. **6. The atomic write widened a credential-bearing file's mode** from 600 to 644 under the common umask, and `os.replace` onto a symlinked `settings.json` replaced the LINK with a regular file, so a dotfile-managed setup silently never received the edit. Mode is preserved; the path is resolved first. - **`start-proxy.sh` printed a dead dashboard link** — it advertised `/dashboard/` and never passed `--dashboard`, so the first line the plugin ever prints was a 404. Now passed, with `--dashboard-db` under the state directory: the default would write `./context-guru-dashboard.db` into the user's repository. - **Backups accumulated forever** (one per add and per remove). Pruned to the newest 10. - **The zero-value cases are now stated** where a first-run user reads them, and `status` checks the one that is both commonest and previously undocumented: **outside a git repository** there is no environment snapshot, so `cachesplit` skips and the saving is exactly zero. The status skill also no longer treats `acted: 0` / `savings_pct: 0` as a verdict — those count content removal, and this component relocates a breakpoint. - **`--idle-exit`'s 24h is the plugin's value, not the flag's default** (which is 0 = never). Said so, along with probes not counting as activity. - Upgrade path documented (`CONTEXT_GURU_UPGRADE=1`, `CONTEXT_GURU_VERSION`). The scripts are tested from Go (`context-guru-plugin/plugin_test.go`) so `go test ./...` and CI cover them. Seven mutations, each proven to have landed before its result counted: backup() back to overwriting copy2 -> TestBackupsDoNotClobberEachOther FAIL "both operations reported the same backup path ..., so one overwrote the other" uninstall stops restoring -> TestUninstallRestoresTheBaseURLItReplaced FAIL restored="" want "https://gateway.corp.example/anthropic"; env left {ANTHROPIC_AUTH_TOKEN:keep} mode no longer preserved -> TestSettingsPreservesFileMode FAIL realpath removed -> TestSettingsFollowsASymlink FAIL checksum fail-open again -> TestInstallRefusesAnUnverifiedDownload FAIL port back in the environment -> TestHookMakesTheProxyIdentifiable FAIL pidfile no longer written -> TestHookMakesTheProxyIdentifiable FAIL One of those is worth recording as a process note: my first attempt at the backup mutation reverted only the timestamp granularity and left the `O_EXCL` retry loop in place, so the name was still unique and the test passed — proving nothing. Reverting half a fix is its own way to get a vacuous result. The run above restores the original function whole. Pre-existing coverage still passes: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for `/healthz`. **Still not verified end to end in a real Claude Code session**, because `install.sh` resolves a GitHub release and no tag has published assets yet. That is the first thing to do once #141 merges and a tag exists. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Implements the proposal in #130, all three stages. Based on
docs/local-distribution, so this PR's diff is the implementation only — the spec and its two verification scripts come from #130, which should merge first.Four decisions were taken before writing anything, and all four are reviewable:
.claude/settings.local.json),--globalopt-in--idle-exitdefault and floormax(2 × store.ttl_seconds, 1h)— enforced at startupgo install; the tap repo and signing are still unownedStage 1 — the toolchain gate
Our own docs were the largest one.
quickstart-proxy.md,setup.mdandhosted.mdall told evaluators to install a C toolchain;setup.mdadditionally named bifrost's tokenizer as a cgo dependency, which it is not.Re-verified directly on go 1.26.4 rather than taken from the spec:
CGO_ENABLED=0with default tags builds all four release targets (27.1–33.5 MB stripped),filereports "statically linked",lddreports "not a dynamic executable", the binary starts and answers/healthz, and-tags cg_skeletonfails underCGO_ENABLED=0with the build-constraints signature — confirming tree-sitter is the only C dependency..goreleaser.yaml: plain GOOS/GOARCH matrix, no cross-toolchains. Nobrews:block, so nothing depends on a repo that does not exist..github/workflows/release.yaml: a tag publishes;workflow_dispatchbuilds the same matrix as a snapshot and publishes nothing. Its first step asserts the pure-Go claim withCC=/nonexistent-c-compiler, so a cgo dependency escaping the build tag fails in CI rather than at a stranger's install.make build-static. The Makefile keepsCGO_ENABLED=1becausego test -raceneeds it — reading that as a shipping requirement is how the wrong claim reached the docs, and the comment now says so.cachepreset ={cachesplit}. Notsafe: format/textclean/searchfold are lossless in meaning but still rewrite the JSON, so "we do not touch your context" stops being literally checkable. Confirmed end to end — a released-shape binary logspipeline=[cachesplit]underPRESET=cache.Stage 2 —
--idle-exitand the pluginOff by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the same teardown, so the self-killing path cannot drift from the one known to work.
Two properties are load-bearing:
end_turn, where 83.7% of the recoverable dollars sit. A request-only watchdog would kill the feature in its working window. So a pending ping both vetoes the exit and resets the clock: retiring the last ping buys a full fresh threshold rather than exiting moments later.store.ValidateIdleExitrefuses anything belowmax(2 × ttl, 1h)— ~5h34m at the default — at startup, not in a doc comment. 2× because the TTL is a sliding window.NewMemorynow calls the sameOptions.EffectiveTTLthe floor is computed from, so the two cannot drift.The plugin (
/plugin marketplace add rossoctl/context-guru→/context-guru:install) is three skills over three scripts plus aSessionStarthook. The hook self-gates on$ANTHROPIC_BASE_URLmatching its own port: the plugin installs at user scope, so its hooks run in every project, and the env value settings already write into the process environment is the per-project enablement signal — no second copy of the port to drift, and it degrades correctly (remove the key by hand and the hook stops firing). It matches the port, not "localhost", so someone routing to litellm on 4000 is not hijacked. Synchronous (closes the race with the first request), idempotent (SessionStartalso fires on clear/compact/resume/fork), and exit 0 on every path — a hook that fails here is a plugin that can brick every session on the machine.settings.pymerges exactly one key, backs the file up first, refuses to overwrite a base URL the user already set, removes only a URL it installed, and refuses to rewrite a settings file it cannot parse rather than replacing it.Stage 3 — gateway conformance
All five items, under the
cachepreset. Four were already correct and are now pinned; one was missing:cache_controlmakes Claude Code disable prompt caching for the rest of the conversation — so a budget mistake silently switches off what the funnel is selling.CLAUDE_CODE_ATTRIBUTION_HEADER=0is not needed in the installer.POST /anthropic/v1/messages/count_tokens— NEW. Absent it, Claude Code counts context by issuing inference requests: billed calls added by a proxy sold on removing them. Forwarded verbatim with no pipeline, because the client budgets its own transcript from the answer.Verification
Every test was revert-verified, with each mutation asserted to have landed in the source before its result was allowed to count. 14 mutations, each failing with the intended message and passing when restored — full matrix in the commit body.
Doing that properly caught three defects in my own tests:
mainnow stamps the clock at launch rather than relying on goroutine scheduling.json.Marshalof a map, which sorts keys — so re-encoding produced identical bytes. It now carries Claude Code's real key order ({"type":...,"text":...}), and both mutations fail.The plugin's shell and Python helpers are tested from Go (
context-guru-plugin/plugin_test.go) sogo test ./...and CI cover them: settings merge/conflict/removal/backup, and the hook's silence in unrouted projects, idempotence, non-failure when the binary is missing, and its wait for/healthz.go build ./...,go vet ./...,gofmt -land the fullgo test ./...are clean.Not verified, and one thing deliberately elsewhere
install.shresolves a GitHub release and no tag has ever been published. Until one is, it reportsno_release_found. Cutting a tag is the first thing to do after this merges.cacherow, and is fixed in a sibling PR (fix/preset-doc-drift) rather than here, because it is unrelated to distribution: three presets were documented as runningtoon(retired after acting 0 times on 5,752 production requests) and every row omittedtextclean/searchfold/linecap, whiledocs/reference/presets.mdwas correct at the same moment. This PR adds one row and leaves the rest of that table exactly as it is on main, so whichever PR merges second needs a trivial rebase there.Still open for you
userConfig.skeletonomitted from releases, source build documented, per the spec's proposal.-slimbuild, so the demo keeps the dashboard.🤖 Generated with Claude Code