feat(plugin): a Claude Code plugin for context-guru, with the #141 review's blockers fixed

[amiddavid]

The Claude Code plugin: /plugin marketplace add rossoctl/context-guru → /plugin install → /context-guru:install.

#141 has merged, so this is no longer stacked — it is two commits on main, 17 files, and the diff is the plugin and its docs. 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 wherever it is routed when the proxy is down, so blast radius is the thing the default optimises for.

Read this first: it cannot work yet, and that is not a defect in the diff

install.sh resolves a GitHub release, and no tag has published assets. On main today the third command ends at no_release_found.

So the merge order matters more than the code review: run release.yaml via workflow_dispatch (snapshot, publishes nothing — it has never executed, and #141 landed wrap_in_directory: true whose archive layout only a real goreleaser run confirms), cut v0.1.0, then merge this. Landing it first puts a README on main telling strangers to run three commands, the last of which fails.

The plugin has therefore never run end to end in a real session. Everything below was verified by unit tests, by driving the scripts directly, and by reading — not by installing it.

What the review of the combined PR found here, and what changed

All six blocking findings were in this half:

#	Finding	Fix
1	uninstall killed the user's own session and left the proxy running	pidfile + socket-owner fallback; the port is in argv via --listen; no pattern matching
2	install.sh could not install, and its documented go install fallback was absent	implemented, plus download_failed as a reported outcome and a key=value contract curl no longer breaks
3	A dead proxy hangs silently and status cannot diagnose it	UserPromptSubmit hook — the only thing that runs without a model call
4	cache advertised context_guru_expand	fixed in #141 where the gate belongs; the claims here corrected
5	The backup destroyed the user's undo; uninstall did not restore what it replaced	O_EXCL + microsecond stamps; the replaced value is recorded and restored
6	The atomic write widened a credential file's mode, and replaced symlinks	mode preserved, path resolved first

On finding 1 the mechanism is worth stating, because the fix follows from it: the port was passed through LISTEN_ADDR in the environment, so pkill -f "context-guru-proxy.*$PORT" matched no proxy — and did match the shell running it, which is the session's own Bash tool. The narrow pattern the skill offered as the safe option was the one that bit. So the fix is a handle, not a better pattern.

On is_ours I did not take the review's suggestion. A URL-shape rule ((127.0.0.1|localhost|[::1]):\d+/anthropic) makes litellm's default read as ours, so uninstall would delete somebody else's routing — the existing test failed the moment I tried it. add now records the URL it wrote and later runs read that record; anything unrecorded stays a conflict, for add and remove alike.

One change since that review

install.sh finds the binary in the tarball rather than assuming the archive root. #141 set wrap_in_directory: true — deliberately, because a flat archive plus the documented tar xzf with no -C overwrites the README.md and LICENSE of whatever directory the user is standing in, which for an evaluator is their own project. Without this change the first install anyone attempted would have failed with binary_not_in_tarball, reading as a broken release rather than a moved file. Found by checking #141's packaging change against this script, not by waiting for it to fail.

Verification

The shell and Python helpers are tested from Go (context-guru-plugin/plugin_test.go) so go test ./... and CI cover them. Seven mutations, each proven to have landed in the source before its result counted, each failing its named test and passing when restored — full output in the commit body. Coverage includes: settings merge/conflict/removal/backup/mode/symlink behaviour, the hook's silence in unrouted projects, its idempotence, its non-failure when the binary is missing, and its wait for /healthz.

One process note kept in the open: my first attempt at the backup mutation reverted only the timestamp granularity and left the O_EXCL loop, so the filename was still unique and the test passed — proving nothing. Reverting half a fix is its own route to a vacuous result.

Worth a reviewer's attention

• The SessionStart hook runs in every project the user has, because the plugin installs at user scope. It self-gates on $ANTHROPIC_BASE_URL naming its own port — matching the port, not "localhost", so somebody routing to litellm on 4000 is not hijacked. It is synchronous (closes the race with the session's first request), idempotent (SessionStart also fires on clear/compact/resume/fork), and exits 0 on every path: a hook that fails here is a plugin that can brick every session on the machine.
• settings.py edits the user's real settings.json. It merges exactly one key, backs the file up first (and prunes to 10), refuses to overwrite a base URL it did not write, restores what it replaced on removal, preserves mode, and follows symlinks. It refuses rather than rewrites a file it cannot parse.
• The zero-value cases are documented where a first-run user reads them, and status checks the commonest one at runtime: outside a git repository there is no environment snapshot, so cachesplit skips and the saving is exactly zero. status also no longer reads acted: 0 / savings_pct: 0 as a verdict — those count content removal, and this component relocates a breakpoint.

🤖 Generated with Claude Code

[amiddavid]

Review of #160 — plugin only, 2 commits, 17 files

Reviewed at 5d37e0c in a separate worktree, rebased scope (origin/main...HEAD), so #141's
changes are excluded. I took your steer and spent the depth on settings.py and the
SessionStart/UserPromptSubmit hooks rather than the docs.

The six blocking findings from #141 are genuinely fixed, and the tests are not vacuous — each
one drives the real script and asserts an observable (the sentinel file, the recorded argv, the
pidfile naming a live PID, the file mode, the symlink target). TestInstallRefusesAnUnverified Download in particular tests the fail-closed property rather than the happy path. The
is_ours-by-record decision is right, and your reasoning for declining the URL-shape suggestion
holds: litellm's 127.0.0.1:4000/anthropic really does make a shape rule unsafe.

Two findings below are, I think, blocking, and both are on the surfaces you named. I verified each
by running the scripts rather than by reading — measurements are in the comments.

1 — the UserPromptSubmit hook cannot finish inside its own timeout. Measured 18s on the exact
dead-proxy path it exists for, against a timeout: 10. The hook is the whole fix for blocking
finding #3, and in the scenario it was written for it is killed before it prints anything.

2 — settings.py remove without --url deletes any base URL, including a stranger's. Measured:
a corp gateway with no context-guru record was removed with result=removed, exit 0. The safety
property currently lives only in the uninstall skill's prompt — a model remembering a flag — while
this script is the layer that is supposed to be deterministic and to "refuse to guess."

Four medium items and a tail of small ones follow. Nothing here needs a real install to close
except the go install PATH item, which I could not exercise: there is no Go toolchain on this
machine, so I could not run go test ./.... CI's build-test was still pending when I checked;
make cover does run ./... so the new package is genuinely gated (the piped go test | sed step
at ci.yaml:34 is only the coverage log, not the gate).

[amiddavid]

Blocking — this timeout is shorter than the work the hook does, on the path the hook exists for.

Measured, with routing configured, no proxy listening, and a binary present that never binds:

$ time check-proxy.sh   # via CLAUDE_PLUGIN_ROOT, routed port, CONTEXT_GURU_BIN=a sleep script
ELAPSED=18s   (UserPromptSubmit timeout is 10s)

The 18s is structural, not a fluke: check-proxy.sh probes /healthz, then invokes
start-proxy.sh, which probes again and then runs its own 15s health-wait loop. The
diagnostic cat <<EOF block is the last thing in the script, so at 10s the hook is killed and
the user sees nothing at all — the identical symptom (a prompt that produces no output) that
this hook was added to replace with an explanation.

The comment at start-proxy.sh:99 says "the hook's own timeout (60s in hooks.json) is the real
backstop." That is true for SessionStart and false here; this path has 10s.

Two fixes, and I think you want both:

1. Print the note before attempting recovery, not after. The note is the deliverable; the
   restart is opportunistic. Emitting it first means a kill at any point still leaves the user
   informed.
2. Bound the wait when invoked from this hook — e.g. have start-proxy.sh honour a
   CONTEXT_GURU_HEALTH_BUDGET (seconds) that check-proxy.sh sets to ~5, and raise this
   timeout to 30 so a slow-but-successful recovery still gets to report success.

A test here would have caught it; see my comment on check-proxy.sh.

[amiddavid]

Blocking — remove with no --url deletes a base URL that is not ours, and reports success.

args.url defaults to "" (line 286), and this guard is if args.url and .... With --url
omitted the entire conflict check is skipped and the key is deleted unconditionally. Measured
against a file with someone else's gateway and no context-guru record:

$ settings.py remove --file settings.json      # note: no --url
result=removed
was=https://gateway.corp.example.com
restored=
exit=0

The gateway is gone, restored= is empty because there was no record to restore from, and the
exit code says success. The module docstring advertises exactly this invocation as supported —
settings.py remove --file PATH [--url URL] — and states two lines later that a base URL that is
not ours "is a CONFLICT and exits non-zero."

What makes this blocking rather than cosmetic is where the safety currently lives. uninstall/SKILL.md
does pass --url, and its prose says "Passing --url is what keeps this safe" — so the property
that protects the user's gateway is a model remembering a flag in a prompt. This script is the
deterministic half precisely so that it is not. It is also the failure mode with the worst blast
radius you named: it edits ~/.claude/settings.json.

Fix, either way round:

• make --url required for remove (mirroring the add check in main()), or
• treat a missing --url as "remove only if is_ours(data, current)" — i.e. fail toward leaving
  the user's configuration alone, which is what is_ours' own docstring says it is for.

Worth a test: remove with no --url over a foreign URL must exit 2 and leave the file byte-identical.

[amiddavid]

Medium — worst-case loop duration is ~138s, against a 60s SessionStart timeout.

The comment says "Up to ~15s", which holds only when the port is refused — then curl returns
instantly and 60 × 0.25s ≈ 15s. If something is listening but not answering (a hung proxy, a
half-open socket, a port taken by an unrelated service that accepts and stalls), each curl burns
its full --max-time 2. Measured against a listener that accepts and never replies:

one curl took 2054ms  ->  60 iterations ≈ 138s   (SessionStart timeout is 60s)

Consequence: the hook is killed at 60s, so the block at lines 109-116 never runs — the user gets
no log path, no /context-guru:status pointer, no tail of the log. And a hung port is one of the
likelier ways to reach that block, since a cleanly-absent proxy exits the loop fast either way.

Budget on wall clock rather than iteration count, and drop the per-probe timeout:

deadline=$(( $(date +%s) + ${CONTEXT_GURU_HEALTH_BUDGET:-15} ))
while [ "$(date +%s)" -lt "$deadline" ]; do
  curl -fsS --max-time 1 "$HEALTH" >/dev/null 2>&1 && { ...; exit 0; }
  sleep 0.25
done

That also gives you the knob check-proxy.sh needs for the 10s-timeout finding on hooks.json.

[amiddavid]

Medium — the go install fallback skips both the mkdir and the on_path report that the
tarball path does.

try_source_build returns 0 and line 126 exits 0 immediately, so everything from line 178 down is
bypassed on this path:

• mkdir -p "$DEST" (line 178) never runs, yet GOBIN="$DEST" is set here. I could not test
  whether go install creates a missing GOBIN — there is no Go toolchain on this machine — so
  this needs confirming rather than taking from me; if it does not, the fallback fails on a clean
  box where ~/.local/bin does not exist yet, which is the common case for the audience this
  whole script targets.
• on_path is never emitted (lines 193-197). So a user who lands on the fallback gets no
  warning when $DEST is not on PATH — and ~/.local/bin frequently is not. They then hit
  start-proxy.sh:56: "routing is configured for port N but the proxy binary is not on PATH",
  from a hook, in a later session, with nothing connecting it back to the install. install/SKILL.md
  reads on_path and tells the user about it, so the skill is silent here too.

Both are fixed by making the fallback fall through to the shared tail instead of exiting: have
try_source_build set the path and break/skip to line 188, or hoist mkdir -p "$DEST" above
line 121 and factor the on_path case into a function both paths call.

Minor, same function: emit "fallback=go_install" is printed before the attempt, so it appears
even when the build fails. install/SKILL.md documents reading it together with the result, which
is correct — just noting the line is "attempted", not "used".

[amiddavid]

Medium — this manual-start command reintroduces the dead dashboard link the PR lists as fixed.

It omits --dashboard and --dashboard-db, which start-proxy.sh:89-90 passes deliberately.
--dashboard defaults to false (cmd/context-guru-proxy/main.go:105), so a proxy started this
way does not serve /dashboard/ — and step 6, ~25 lines below, tells the user

Dashboard: http://127.0.0.1:<port>/dashboard/ — the four billed token tiers are where the cache
effect is visible.

which is a 404. That is the same defect as the "dead dashboard link — it advertised /dashboard/
and never passed --dashboard" item in the PR description.

It also does not self-heal, and that is the part worth fixing rather than documenting:
start-proxy.sh is idempotent on /healthz (line 51), so once this hand-started proxy is up the
hook will never replace it — and with --idle-exit 24h it holds the port for a day. The user's
dashboard is broken for that whole window with no signal as to why.

check-proxy.sh:48 has the same omission in the command it prints to the user.

Simplest fix: make both places invoke start-proxy.sh rather than restating the command line —
the script already self-gates on ANTHROPIC_BASE_URL, which step 5 correctly explains is unset in
the installing session, so it would need an override (CONTEXT_GURU_FORCE=1, or just documenting
ANTHROPIC_BASE_URL=http://127.0.0.1:$PORT/anthropic start-proxy.sh). Failing that, add the two
dashboard flags in both places.

Credit where due: step 5's explanation of why the hook does nothing in the installing session is
exactly right, and it is the kind of thing that usually gets left out.

[amiddavid]

Low — glob.glob treats [, ? and * in the path as pattern syntax, so pruning silently does
nothing for a settings file under a directory containing them (~/projects/foo[1]/.claude/...).
Backups then accumulate forever, which is the thing KEEP_BACKUPS exists to stop, and the failure
is invisible because prune_backups is best-effort by design.

glob.glob(glob.escape(path) + ".context-guru-backup-*") fixes it.

[amiddavid]

Low, process rather than code: requireTool uses t.Skipf, so on a runner without python3 or
bash every test in this file skips and go test is green. ubuntu-latest has both today, so
this is latent — but the whole point of the package header is that these scripts get the coverage
their blast radius warrants, and a skip is indistinguishable from a pass in CI output.

For python3 and bash specifically I'd t.Fatalf instead: they are guaranteed on the CI image,
so an absence means the image changed and you want to hear about it. Keep t.Skipf for anything
genuinely optional.

[amiddavid]

Low, and unverified — please confirm before acting on it, I could not. On Linux, opening a
currently-executing binary with O_TRUNC fails with ETXTBSY, and coreutils install truncates
rather than replacing the inode. If that holds here, the upgrade path (CONTEXT_GURU_UPGRADE=1)
fails with install_failed_to_$DEST in exactly the situation upgrades happen in — a proxy that is
running, which with --idle-exit 24h is most of the time. macOS permits the write, so this would
not reproduce on a dev box.

I could not test it: no Go toolchain and no Linux here.

If it does hold, install -m 755 "$found" "$DEST/$BIN.new" && mv -f "$DEST/$BIN.new" "$DEST/$BIN"
avoids it — rename swaps the directory entry and leaves the running image alone — and is also
atomic for anyone starting the proxy concurrently.

[amiddavid]

Low — the comment above claims the starter "detaches the proxy from this hook's process group so
it survives the hook returning and is not killed with the session's process tree." setsid does
that; the macOS fallback nohup does not — it only makes the child ignore SIGHUP, leaving it in
the caller's process group. So on macOS, which is presumably where most of this was developed, a
signal delivered to the session's process group still reaches the proxy.

disown at line 93 does not help either — that is bash job-table bookkeeping, not a process-group
change.

In practice nohup + disown usually survives, so I would not change the mechanism on my say-so.
But the comment states a property the fallback does not provide, and the next person to read it
will trust it. Either say "setsid where available; nohup only blocks SIGHUP", or get the real
property portably with (trap '' HUP; exec "$BIN" ... &) in a subshell.

[amiddavid]

Nit — "two commands" sits directly above a three-command block, and docs/how-to/install-plugin.md
spells out why it is three ("The first two are once per machine. The third is once per repo").
marketplace.json compresses it further to "in one command."

Since the split is the genuinely reassuring part of the design — the machine-wide step and the
per-repo routing decision are separate on purpose, and the routing one is the one with blast
radius — I would say so rather than round the count: "two commands to install, one per repo to
route."

[amiddavid]

All thirteen. Two of them were blocking, both on the two surfaces I asked you to go deep on — and the fact that I named those surfaces as the highest-risk ones and had still not tested them is the honest summary of this round.

Pushed as 6577320. Every claim below is measured, before and after.

The 19s hook with a 10s timeout

Reproduced exactly, and it is worse than a slow hook: check-proxy.sh exists only for the case where a prompt produces nothing at all, and its explanation is the last thing it prints. So on precisely that path it was killed first and the user got the original silent hang — with a hook that looked like it had handled it.

Your diagnosis of the shared cause was right, and I confirmed the mechanism separately: the seq 1 60 + --max-time 2 loop is only ~15s when the port is refused. Against a socket that accepts and never answers I measure 2046ms per probe → ~122s against SessionStart's 60s, so the failure report at the bottom of start-proxy.sh never ran either. Your "a hung port is one of the likelier ways to reach that block" is the part that makes it a real defect rather than a slow path.

Wall-clock budget via CONTEXT_GURU_HEALTH_BUDGET (default 15s), check-proxy.sh asks for 5s, hooks.json raised to 30s. Measured after: 5s dead-proxy, 11.1s worst-shape, and the hung-port start path is 17s with the failure report printing.

I took one half of your fix and not the other. The note stays last. Printing first guarantees it survives a kill, but the common case here is a silent successful recovery after an idle-exit between two prompts, and a paragraph about a dead proxy on every one of those is noise on a working path — it also lands in the model's context for that turn. The budget is what makes the ordering safe. If you still think the guarantee beats the noise, say so and I will flip it; it is a judgement call, not a disagreement about the facts.

remove with no --url

Reproduced: result=removed, restored= empty, exit 0, corporate gateway gone.

Took your second option (missing --url means "remove only if is_ours") rather than making the flag required, because it fails toward leaving the user's configuration alone even when a caller forgets. Your framing is the part I want to keep on the record — the property lived in a prompt, in a file whose prose asserted it. That prose is corrected too, since it was actively misleading about where the safety came from.

The test asserts the file comes back byte-identical, not merely that a base URL is still there: a refusal that rewrites the file has already done the thing it refused.

Your two unverified items — both are false, and I tested them

You were right to flag rather than assert these. Go lives on a Linux box here, so I ran both:

• go install DOES create a missing GOBIN (Linux, Go 1.26.4). No mkdir needed on the fallback path.
• install -m 755 over a running binary SUCCEEDS on Linux — no ETXTBSY. coreutils install unlinks the destination first, which is why.

I still install-then-rename, but for the reason your false premise was sitting next to: that unlink is a window in which $DEST/$BIN does not exist, so a SessionStart hook firing in another project mid-upgrade reports "not on PATH". rename(2) closes it.

The on_path half of that finding was entirely real and is fixed — both paths call a shared report_path now.

One suggestion declined on its mechanics

O_EXCL on the fixed name .context-guru-tmp closes the mode window, but a leftover temp from a crash or a full disk then makes every later save fail until somebody deletes it by hand — trading a mode window for a permanent lockout of the file this script exists to edit. mkstemp gets the 0600-at-creation property with a unique name and no lockout.

The test you said was missing

Six properties for check-proxy.sh, and two details from your note that I would not have chosen unprompted:

• the timeout is read out of hooks.json, not retyped, so the pair cannot drift again — the defect was a mismatch between the two files;
• the timing assertion measures the accept-and-stall shape, not the refused-port one. My first version used a free port, measured 5s, and would have passed at almost any timeout — sensitive to nothing. On the worst shape it is 11.1s against a 15s bound, and a drop back to 10s fails clearly.

requireTool now fails rather than skips for python3/bash, per your last point.

Nine mutations, each proven to have landed before its result counted, each failing its named test, each file restored byte-identical, suite green after the sweep. Full table in the commit body; it includes reverting hooks.json to 10s and dropping the dashboard flags from the printed command.

One gap, stated rather than papered over

The 0600 temp-file window is fixed but not tested. It is a window between two syscalls, and a test asserting on it would be asserting on a race it cannot observe — you noted TestSettingsPreservesFileMode passes either way, and that is still true. Verified by reading.

Also worth recording: my first attempt at the timeout test failed for a reason that had nothing to do with the script — my assertion string spanned a hard line break in the heredoc. Matching on collapsed whitespace now, since that prose will be rewrapped again.

On the merge order

Unchanged and still the thing gating usefulness rather than correctness: release.yaml via workflow_dispatch, then v0.1.0, then this. Until a tag publishes assets, /context-guru:install ends at no_release_found — so the end-to-end path these fixes are on remains unexercised by anything except tests.

Ready for re-review.

[amiddavid]

Re-review of 6577320 — all thirteen verified fixed; two new findings, one of them mine to own

I re-ran the measurements rather than reading the diff. Everything I filed is genuinely fixed, and
the three places you did not do what I suggested are all better than what I suggested. Detail below,
then two new items — the first of which I should have caught in round one and did not.

The two blocking findings, measured before and after

Path	Before	After	Bound
check-proxy.sh, dead proxy	18s	5s	30s timeout
check-proxy.sh, accept-and-stall	—	5s	30s timeout
start-proxy.sh, accept-and-stall	~138s	16s	60s timeout

The diagnostic is delivered in every case now, including the stall shape. I measure the stall
shape at 5s rather than your 11.1s — probably harness differences, and in the safe direction either
way; the assertion has margin regardless.

settings.py remove verified on three shapes:

• no --url, foreign gateway, no record → result=conflict, exit 2, file byte-identical ✓
• no --url, our own recorded URL → still result=removed ✓ (the fix did not overshoot into
  making remove useless without the flag)
• --url mismatching a foreign URL → still result=conflict, exit 2 ✓

Your three pushbacks: all accepted, and one correction to my own record

1. My two unverified items were wrong, and you tested what I could not. go install creating a
missing GOBIN, and install not hitting ETXTBSY because coreutils unlinks first — I flagged
both as needing confirmation precisely because I had no Linux box and no toolchain, and you went and
got the answer. Thank you for testing rather than deferring. The install-then-mv you kept is
better justified than my version of it: the unlink window is real, rename(2) closes it, and the
comment in the script now records the corrected reasoning instead of my wrong premise, which is the
right thing to leave behind for the next reader.

2. Diagnostic staying last: you are right, and TestCheckHookRecoversSilently is what settles
it. My reasoning was "the kill-survival guarantee is unconditional, ordering makes it free." Yours
is that the guarantee is only worth anything on the failure path, while the common path after an
idle-exit is a silent successful recovery — and a dead-proxy paragraph on every one of those lands
in the model's context on a turn where nothing is wrong. That cost is certain and recurring; mine
was hypothetical once the budget exists. Budget first, ordering second is the correct dependency,
and the test now pins the silence so nobody "helpfully" moves the note up later. No flip needed.

3. mkstemp over O_EXCL: correct, and I missed the failure mode. A leftover
.context-guru-tmp from a crash or a full disk would have made every subsequent save fail
permanently — turning a brief mode window into a permanent lockout of the one file this script
exists to edit, recoverable only by hand. Trading a transient defect for a persistent one is
strictly worse. The except BaseException unlink is a good addition I had not asked for.

I also accept the untested-window gap as stated: a race between two syscalls is not honestly
assertable, and "verified by reading" is the right label rather than inventing a test that would
pass either way.

On the test I asked you to check

TestCheckHookFinishesInsideItsOwnTimeout is right, and the change you describe is the one that
matters: measuring the accept-and-stall shape instead of a free port is what makes it sensitive to
anything. Reading the bound from hooks.json via hookTimeout and asserting against limit/2
rather than limit is better than what I suggested — the half-margin means it fails on a real
regression instead of only on a loaded runner, and it cannot drift from the config. Asserting
--dashboard appears in the printed recovery command is a nice touch: it pins finding 5 in the same
place, so the 404 cannot come back through the text.

TestCheckHookIsSilentWhereRoutingIsNotConfigured covering 127.0.0.1:87871 as a prefix of 8787
pins the gate fix at the level it actually broke. That is the table-driven version I would have
written.

Nothing further on the other eleven — report_path shared by both install paths, glob.escape,
t.Fatalf for python3/bash, the nohup comment, and the "two commands to install, one per repo
to route" wording all check out.

Merge order

Agreed and unchanged: release.yaml via workflow_dispatch → v0.1.0 → this. I am not treating
"has never run end to end" as a finding, for the reason you gave — it is gated on a tag existing,
not on anything in this diff. Worth saying plainly in the PR when it merges, so the first person to
run /context-guru:install against a real release knows they are the first.

Two new comments below. The first is a genuine miss on my part from round one — same defect class
as #141's finding 1, which you noted is not hypothetical here. It is unchanged by this commit, so it
is not a regression; I simply did not see it. The second is a small test-hygiene item on new code.

[amiddavid]

Medium, and my miss from round one — the ownership check is printed after the kill it is
supposed to gate.

This is not a regression from 6577320; it has been here since 0abd995 and I did not catch it. Same
defect class as #141's finding 1, which is why I want it fixed rather than left as prose ordering.

Read the skill top to bottom, which is how it gets executed:

• lines 74-78 — if [ -n "$pid" ]; then kill "$pid" && rm -f "$PIDFILE"
• line 81 — "Before killing anything, confirm the PID is ours — the port may be held by
  something else entirely, and this step must not kill a stranger's process"
• line 85 — the ps -p "$pid" -o command= | grep -q context-guru-proxy check

The instruction says "before killing anything" and sits after the block that kills. An agent working
through this file in order has already sent the signal by the time it reads the guard, and the check
then reports on a process that is already dying. The safety property is stated but structurally
unreachable.

It matters most on the path that produces an untrustworthy PID in the first place. The pidfile is
ours by construction, but the fallback at lines 66-72 is lsof/ss on "whoever holds the port" —
which is explicitly there to cover a stale pidfile and a hand-started proxy, i.e. exactly
the cases where the PID may belong to something else. A recycled PID from a stale pidfile passes
kill -0 too.

The fix is to make it one unskippable block rather than two blocks and a warning, so the ordering
cannot be got wrong by reading:

if [ -z "$pid" ]; then
  echo "(nothing listening on ${PORT})"
elif ps -p "$pid" -o command= | grep -q context-guru-proxy; then
  kill "$pid" && rm -f "$PIDFILE"
else
  # The port is held by something that is not ours — a hand-started service, or a recycled PID
  # from a stale pidfile. Report it and stop; do not kill it.
  echo "NOT OURS — pid $pid holds ${PORT} and is not context-guru-proxy; left alone"
fi

Then keep the liveness re-check at lines 91-93 as it is, since "did the kill work" is a separate
question from "is it ours".

One consequence worth stating in the skill while you are there: if the check says NOT OURS, step 1
has already removed the routing, so the user is safe and unblocked — the leftover is a port
conflict to report, not something to escalate on. That is the reassurance that stops the next reader
reaching for kill -9.

[amiddavid]

Low, new-code hygiene: held is appended here on the listener goroutine and iterated by the
t.Cleanup closure at line 812, which runs on the test goroutine. That is an unsynchronised
read/write of the same slice, and make cover runs go test -race — so if a connection is being
accepted while cleanup runs, this is a race-detector failure rather than a test failure, which is a
confusing thing to land on.

Registering t.Cleanup from inside the goroutine is also a hazard in its own right: t.Cleanup
called after the test has finished panics, and nothing here orders the goroutine's registration
before the test's end.

Cleanup order makes it narrow rather than safe — the held cleanup is registered second so it runs
first (LIFO), while the goroutine is still blocked in Accept and can still append. In practice
curl is done by then, so I would not expect this to fire often; "not often" is the bad frequency
for a race in CI.

Simplest fix is to drop held entirely — closing the listener is what ends the goroutine, and the
accepted connections die with the process at test end:

t.Cleanup(func() { ln.Close() })
go func() {
    for {
        c, err := ln.Accept()
        if err != nil {
            return
        }
        defer c.Close() // held open for the life of the goroutine, answers nothing
    }
}()

If you want the connections explicitly closed, register the cleanup on the test goroutine before
starting the listener goroutine and guard the slice with a mutex. Either way the property the helper
exists for — accept, never answer — is unaffected.

Worth saying: stallingPort is the right helper to have added. The shape it produces is what turned
both timeout tests from assertions that would pass at any value into ones that catch the defect.

[amiddavid]

Correcting my own severity: this is not "low", it is the build-test failure on 6577320. It
fired on the first CI run, not rarely.

--- FAIL: TestCheckHookFinishesInsideItsOwnTimeout (11.14s)
FAIL  github.com/rossoctl/context-guru/context-guru-plugin  20.047s

Three WARNING: DATA RACE reports, all the same shape — Read at ... by goroutine 65 inside
testing.go:1317 (that is tRunner running the registered cleanups) against Previous write at ... by goroutine 66, the accept loop. The frames are plugin_test.go:812, :813, :817 and :821:
the held iteration and c.Close() in the cleanup, versus the append and Accept in the
goroutine. Run: https://github.com/rossoctl/context-guru/actions/runs/33815232946/job/100845795572

I said "not often is the bad frequency for a race in CI" and then underrated it anyway — I reasoned
that curl would be finished by cleanup time, but this test holds three probes against the
stalling port, and ln.Close() runs after the held cleanup (LIFO), so the accept loop is still
live and writing while the cleanup reads. Sorry for filing it as a nit; the fix in my comment above
is unchanged and is what unblocks CI.

Two things worth taking from the failure beyond the fix.

Your 11.1s was right and my 5s was the outlier — the run measures 11.14s, so that was Linux/CI
versus my macOS box, and your figure is the one to design the margin against. That also means the
timing assertion passed: 11.14s against a limit/2 bound of 15s. The defect I filed on
hooks.json is fixed and the test proves it on the platform that matters; the only thing red here
is the helper's bookkeeping. Good news for the substance, and an argument for the half-margin you
chose over my limit — at 30s the bound is 15s and 11.14s fits, but it is not roomy, so I would
resist any future trimming of that timeout.

Second: this is test-only code. No production path touches held, so nothing about the plugin's
behaviour is implicated — worth stating in the commit message that fixes it, so the next reader of
the history does not go hunting for a proxy defect behind a red build-test.

Everything else in the run is green (DCO, build, purego, trivy), and this is the only failing
test in the only failing package.

[amiddavid]

Both done, pushed as 82b8f6f. go test -race ./... and gofmt -l clean.

The race — yours to flag, mine to have caused

You were right to lead with it, and right that it is test-only: nothing in the proxy touches stallingPort. The commit says so explicitly, so a red build-test on 6577320 does not get read later as a proxy defect.

I had it fixed from the same CI log before your message arrived, and the diagnosis matches: t.Cleanup registered from inside the listener goroutine, so the cleanup closure read held on the test goroutine while the accept loop appended on another. Fixed by ownership rather than a mutex — the goroutine that appends is the only thing that reads, and it closes the connections in its own deferred func; the test goroutine only closes the listener, which is what makes Accept return.

Why I did not catch it is the part worth recording. I ran that suite repeatedly on a box without -race, where this is invisible — a real race with no symptom until the scheduler interleaves badly. CI runs -race through make cover, so CI was the first thing that could see it. I had also just finished reporting nine green mutations, which verified that my tests catch defects in the code under test and said nothing whatsoever about whether the test code was correct. New concurrent test code gets -race locally now.

On underrating it as "low hygiene": I would not spend much on that. You filed the right observation with a wrong severity, and the severity was recoverable in one CI run. Filing it at all is what made the fix a five-minute job.

uninstall killing before the check

Real, and the diagnosis is exact — the guard was decoration, because these files execute the way they read. Your point that it bites hardest on the lsof/ss fallback is the part that makes it a defect rather than untidiness: that fallback exists for a stale pidfile or a hand-started proxy, i.e. precisely when the PID may not be ours, and a recycled PID satisfies kill -0 perfectly well.

Collapsed into one if/elif/else as you suggested, with one addition you did not ask for: when the PID is not ours the pidfile survives. Removing it would strand a proxy of ours that is still running under a different pid — and "not ours" plus "delete our handle to it" is the worse of the two failure modes.

Given an executing test rather than better prose, because this class has now recurred twice in this PR's lineage and reading is what missed it both times. TestUninstallDoesNotSignalAProcessThatIsNotOurs extracts the fenced block straight out of the skill and runs it against a stubbed ps: a stranger's process must not be signalled and its pidfile must survive; ours must be signalled. Mutation-checked — with the two-block ordering restored, the block sends SIGTERM to /usr/bin/postgres.

That test has a process note of its own, and it is the same trap as the race: my first version stubbed kill as a file on PATH. kill is a bash builtin, so the stub was never consulted — the block's kill -0 probe failed for a pid that does not exist, it fell through to the socket-owner lookup, and the run exercised none of the branch under test. It failed loudly only because one of the two cases asserts a positive outcome (kill WAS invoked); the negative case alone would have passed for exactly the wrong reason. A shell function overrides the builtin.

Your measurements

Taken, and both recorded in the commit body rather than just agreed to here:

• 11.14s on Linux/CI is the number to design against, not my macOS reading or your 5s. Your table and the CI run agree.
• That leaves 11.14s against a limit/2 bound of 15s — real margin, not roomy. Noted in the commit that the 30s UserPromptSubmit timeout should not be trimmed later without re-measuring on Linux, since the next person to see "30s for a hook?" will be tempted.

Thanks for verifying the two blocking fixes by measurement rather than by reading the diff — the three remove shapes in particular, including that the fix did not overshoot into refusing our own recorded URL. That is the failure mode I would have shipped if I had only tested the refusal.

Merge order unchanged: release.yaml via workflow_dispatch, then v0.1.0, then this.

[amiddavid]

Re-review of 82b8f6f — both items fixed and independently verified; no blocking findings left

The uninstall ordering fix, verified by re-running it rather than reading it

I extracted the fenced block from uninstall/SKILL.md myself and ran it against a stubbed ps, in
both shapes and then with your old two-block ordering restored:

Block	ps reports	signalled	pidfile survives	said NOT OURS
82b8f6f	/usr/bin/postgres -D /var/lib/postgres	no	yes	yes
82b8f6f	context-guru-proxy --listen 127.0.0.1:8787	yes	no (removed, correctly)	no
mutation: old two-block order	/usr/bin/postgres …	yes	no	yes

So the mutation does signal postgres, and the fix does not. Your report matches exactly, and the
test is sensitive to the defect rather than to its surroundings.

The pidfile surviving on NOT OURS is a better call than what I proposed, and I had not thought it
through. I only said "do not remove it"; you gave the reason that makes it load-bearing — a proxy of
ours may still be running under a different pid, and "refuse to touch this process" plus "throw away
our own handle" combine into a proxy nobody can stop by the documented route. Deleting the handle is
the more expensive half of that pair.

skillBlock is the right shape for testing prose that gets executed, and it fails on zero or more
than one match rather than silently taking the first — which is the failure mode that would otherwise
make it test the wrong block after an edit. I confirmed exactly one of the three bash blocks in that
skill contains the needle.

The race, fixed by ownership

Correct, and better than the mutex I offered as the alternative: held is now touched only by the
listener goroutine and closed by its own defer, with t.Cleanup(ln.Close) registered on the test
goroutine as the thing that unblocks Accept. No shared state, so nothing to synchronise — and
t.Cleanup is no longer called from a non-test goroutine, which was the second hazard in the same
five lines. Noting in the commit that no production path touches stallingPort is the right thing to
leave in the history.

Agreed on the process point, and it is the more transferable lesson: a non-race run cannot see this
class at all, so -race locally before pushing is the cheap version of what CI told you.

The trap you flagged — I went looking, and there are exactly two instances left

Thank you for naming the shape; it made this a mechanical search rather than a guess. Your
kill-as-a-file version is a textbook vacuous pass, and the reason it surfaced is exactly what you
said: the positive row asserted a positive outcome, so the stub had to actually be consulted for
the test to go green. That is the property worth generalising.

By that standard the stub-based tests come out clean. TestInstallRefusesAnUnverifiedDownload is
refusal-only on its face, but it asserts checksum_unavailable — a string only reachable if the curl
stub successfully served the tarball first, since real curl would have produced download_failed
instead. So the stub is proven consulted by the assertion itself.
TestInstallReportsPATHFromTheSourceFallbackToo asserts built_from=source and covers both
on_path values, so the go stub is likewise pinned.

Two tests remain in the shape you told me to distrust, and I filed the detail on one of them below.
Neither is wrong today — both are saved by a positive control that lives in a different test — and
that is the fragility worth one line each.

CI

DCO, build, purego, trivy green on 82b8f6f; build-test was still running when I posted, and
I am watching it. Merge order unchanged and still the real gate: release.yaml via
workflow_dispatch → v0.1.0 → this.

Nothing here blocks. From my side this is ready once build-test is green.

[amiddavid]

Following up on the trap you flagged — this is one of the two tests still in that shape, and the
only reason I am filing it is that you asked me to look for it.

Every assertion here is an absence: exit 0, and empty output. Nothing in the test requires the
script to have done anything, so it cannot distinguish "the gate correctly declined" from "the
script exited before reaching the gate." Demonstrated by running its four rows against a
check-proxy.sh gutted to a bare exit 0:

all four rows pass: yes    (exit 0 + silence are the only assertions)

TestHookIsSilentAndInertWhereRoutingIsNotConfigured at line 291 is the same shape — the sentinel's
absence plus silence, with no in-test evidence the sentinel mechanism works at all.

Both are saved today, but from a distance: the positive control for this one is
TestCheckHookFinishesInsideItsOwnTimeout (routed → it speaks), and for line 291 it is
TestHookStartsTheProxyAndWaitsForHealthz (routed → the sentinel appears). That is real coverage, so
neither test is wrong. The fragility is that the control is somewhere else: a future change that
skips, renames or narrows the positive test takes the meaning out of the silence test without
touching it or making it fail, and a silence test that has quietly stopped proving anything is the
exact failure you just spent a round on.

Cheapest fix is to make the control local — one more row in each table asserting the routed case
does act:

{"routed to our port", "http://127.0.0.1:8787/anthropic", wantSilent: false},

with the assertion inverted for that row (non-empty output here; sentinel present at line 291). Then
the table itself proves the mechanism is live, and the silence rows mean something on their own.

Low priority and entirely your call — the coverage exists either way. It is the "would this still
fail if the thing under test were removed?" question, which is the same question your kill stub
answered the hard way.

[amiddavid]

build-test is red on 82b8f6f, and it is not this PR — it is a pre-existing main defect

The plugin package passed:

ok  github.com/rossoctl/context-guru/context-guru-plugin  21.137s

So the stallingPort race fix is confirmed green in CI, which was the open question from my last
review. The sole failure is elsewhere:

--- FAIL: TestArriveCancelsAndReports (0.03s)
    keepalive_test.go:413: refreshed = 0, want the ping's own cache_read of 48576
FAIL  github.com/rossoctl/context-guru/proxy  220.284s

This PR touches no file under proxy/ — git diff --name-only origin/main...HEAD matches nothing
for proxy/ or keepalive. And main is green on its last eight CI runs, so this does not
reproduce on main on demand.

It is a real synchronisation defect in the test, not an unexplained flake

I read the path rather than filing it as "timing-sensitive and probably fine". The test waits on one
signal and then asserts on state written by a different, later one:

• proxy/keepalive.go:899 — k.pings.Add(1), immediately after k.send(...) returns. This is the
  counter waitPings polls (keepalive_test.go: for ... if k.pings.Load() >= n { return }).
• proxy/keepalive.go:989 — e.refreshed = u.CacheRead, under k.mu, roughly ninety lines
  later in the same flow: after the error check, the dash.Event construction, model/provider/
  route resolution and ev.Price(...).

TestArriveCancelsAndReports calls waitPings(t, k, 1) and then immediately k.arrive(...), which
reads e.refreshed (line 548). So the wait returns as soon as the ping is sent and counted, while
the value being asserted is not written until the response has been priced. The window is
deterministic and always present; it only becomes visible when the writer goroutine is descheduled
inside it. refreshed = 0 is exactly the pre-write value.

That makes the assertion sound and the wait wrong: k.pings fires strictly before the state under
test exists.

Why it likely surfaced here rather than on main

Not a code dependency — a contention one. make cover runs go test -race ./..., which schedules
packages concurrently on a 2-core runner, and this PR adds a package that is deliberately
wall-clock-heavy: 21.137s, most of it the two budget tests holding a stalling socket for ~11s
each while curl waits out its timeouts. That is real added pressure on the same runner, and
widening an existing race's window is exactly what it would do. ci.yaml:81-89 already records this
class — a timing-sensitive test that "flaked twice on unrelated PRs, then passed on re-run and passes
3/3 locally".

So: contributing factor here, root cause on main.

Recommendation

Do not absorb this into #160. Per the convention for a main defect found on a feature branch,
it wants its own worktree, its own branch off main, and its own PR — otherwise a plugin PR carries
an unrelated proxy change and the history stops explaining itself.

The fix belongs on the test's wait, not on the production ordering (pings counting a sent ping
is correct, and refreshed genuinely cannot be known before the response is priced). A wait on the
state actually asserted, e.g. polling e.refreshed under k.mu — or a signal published after line
990 — closes it deterministically for every test in this file that follows waitPings with an
assertion on post-response state. Worth checking whether TestArriveCancelsAndReports is the only
one in that shape; a grep for other waitPings callers is the cheap version of that question.

A re-run will very likely go green and would let #160 merge on its own merits. I would still open the
main issue rather than let a re-run close the subject, since a re-run does not make the window
smaller — and #160 measurably makes it easier to hit from now on, so this will recur on unrelated PRs.

Nothing here changes my assessment of #160 itself: no blocking findings, ready once build-test is
green.

[amiddavid]

Addendum — I ran the grep I suggested, and the blast radius is exactly one test.

There are seven waitPings call sites in proxy/keepalive_test.go (179, 260, 278, 406, 441, 814,
942). Only two are followed by an arrive(), and only one of those asserts state from the late
post-response block:

• 406 — TestArriveCancelsAndReports: asserts refreshed, written at keepalive.go:989, after
  the response is priced. This is the racy one.
• 441: asserts strategyID, i.e. e.appliedStrategy — written at keepalive.go:661 when the
  entry is created, well before the sweep fires. Not affected.

The remaining five assert nothing from that block within their following lines.

So the fix is genuinely one test's wait, not a pattern to unpick across the file — which makes the
separate main PR small. Two shapes would both work: poll e.refreshed under k.mu instead of
k.pings, or publish a signal after keepalive.go:990 and wait on that. The second is preferable if
e.spent (written on the adjacent line, 988) ever grows an assertion in this position, since it
closes the window for the whole block rather than for one field.

Worth noting for whoever picks it up: -race will not catch this one. Both accesses are properly
mutex-guarded — the bug is ordering, not unsynchronised memory, so the only signal is the flake
itself. That is the opposite of the stallingPort race in this PR, which -race caught immediately
and a plain run could not see. Two adjacent failure modes with inverted detection stories, which is
worth a line in whatever fixes this.

[amiddavid]

Review complete — all findings resolved, CI green at eb9eac9

DCO · build · build-test (10m54s) · purego · trivy — all pass. The plugin package ran and
passed on its own line (ok github.com/rossoctl/context-guru/context-guru-plugin 22.142s), so the
suite is genuinely exercised rather than skipped.

Sixteen findings over four rounds, all closed: two blocking (the UserPromptSubmit hook unable to
finish inside its own 10s timeout; settings.py remove deleting a base URL that was not ours when
--url was omitted), four medium, the uninstall kill-before-ownership-check ordering, the
stallingPort data race, and the two silence tests whose positive control lived in another file. I
re-measured or re-ran each fix rather than reading the diff; the numbers are in the individual
reviews.

The build-test failure on 82b8f6f was triaged to #206 — a pre-existing main defect, not this PR —
and the re-run on the same commit going green is consistent with that: same code, red then green,
nothing changed in between. Worth keeping #206 open regardless, since a re-run does not shrink the
window and this PR's wall-clock-heavy test package makes it easier to hit from now on.

One correction on the merge state

The PR reports mergeable=MERGEABLE but mergeStateStatus=BLOCKED, and the block is not the
checks — it is reviewDecision=REVIEW_REQUIRED. Ruleset "main branch" (19779069) requires 1
approving review.

That gate cannot be cleared by an approval from this account: the reviewing credential and the PR
author are the same user, and GitHub refuses a self-approval. So there are two real routes, and it is
worth knowing which one is intended before the release sequence starts:

• an approving review from another account with access — OsherElhadad is already an always-bypass
  actor on that ruleset, so is the natural reviewer; or
• an admin bypass at merge time, which the author's own admin: true permission plus the
  RepositoryRole id=5 … mode=always bypass entry does allow.

I am not taking either action — the first is not available to me, and the second is a merge decision,
not a review one.

Merge order

Unchanged, and still the thing that makes this work rather than merely be correct: release.yaml via
workflow_dispatch → v0.1.0 → merge this. Until a tag publishes assets, /context-guru:install
ends at no_release_found, so the end-to-end path stays unexercised by anything but tests. That
remains the single largest untested surface here, and the PR description says so plainly, which is
the right place for it.

No blocking findings outstanding from this review.