fix(mention-context): give every run_capture call its own temp files (OPE15-00058) - #23
Conversation
…hooks/ The prompt-context hook had no repository home. It renders into every agent's prompt on every firing, is registered by absolute path in a settings file, sits outside any git repo, carries no managed-render marker, and its change control was edit-in-place with a timestamped .bak copy. This commit is the file EXACTLY as installed, byte-identical, so the fix that follows is reviewable as a diff rather than as a 1,182-line import. installed md5 6c39bfa24a6c0d22693d96f79f58a4e7 adopted md5 6c39bfa24a6c0d22693d96f79f58a4e7 cmp byte-identical, rc=0 No behaviour changes here, and nothing is installed to the live path: adopting the source and updating the running hook are separately verified steps. This is the first Python under hooks/ — every other hook is TypeScript. Chosen over a new repository (a second home for hook code is the duplication the abstractions rule exists to prevent) and over a TypeScript port (rewriting 1,182 lines that fire on every prompt is disproportionate to fixing one temp path). tsconfig.json already excludes hooks/, so typecheck is unaffected. Refs: OPE15-00058 Agent: Silvanus
run_capture derived its output path from `tag` alone, and probe_local_head
passed a constant tag="gitlog". Repository probes run concurrently against one
shared tmpdir, so every mentioned repository's `git log` wrote to and read back
the same gitlog.out and the reader got whatever the last writer left.
The emitted value was always a real sha from a real repository, just the wrong
one, which is why it read as correct and went unnoticed. It needs two or three
mentions in one prompt; MAX_TOKENS is 3, and a single mention produces one probe
with no concurrency and is always right.
FIXED IN run_capture, NOT AT THE CALL SITE. Threading org/word into
probe_local_head is two lines and matches what its siblings already do, but it
leaves the invariant unenforced and the next call site inherits the bug — which
is precisely how this one arose. run_capture owns path construction, so a
per-call component there makes collision impossible by construction.
probe_npm's body_p is the one capture path that bypasses run_capture (curl
writes the body via -o while stdout carries the status code). It was NOT part of
the defect — (org, word) is deduplicated by extract_tokens, so that name was
already unique within a run — but it now takes the same per-call component so
the rule holds for every temp path rather than for most of them.
TESTS — a race needs a test that can actually fail, so these force the losing
interleaving with a controlled delay rather than looping and hoping:
early writer / late reader writes at t=0, exits and reads at t=0.80
late writer / early reader writes, exits and reads at t=0.25
Measured on this branch, five runs each, station01 at loadavg ~28:
BEFORE 5/5 FAILED (failures=3)
AFTER 5/5 OK
The end-to-end failure names the defect exactly:
AssertionError: 'c091c21' != '7f5cdda'
: beta reported 'c091c21'; its own head is '7f5cdda' and alpha's is
'c091c21' - one repository's head was attributed to another
Real acceptance path, the same three-repo prompt run six times against each
build, scored against checkout HEADs verified to exist only in their own repo:
PRE-FIX 3 of 6 runs carried at least one wrong attribution
POST-FIX 0 of 6
Those six runs are an observation with its denominator, not an error rate. The
half that matters for testing: 3 of the 6 pre-fix runs were entirely clean, so a
test that exercises the hook once and checks the output can pass on the unfixed
code, and a green single-run result would be indistinguishable from a real fix.
That is why the gate is the deterministic concurrency test.
The duplicate-sha check ships as a labelled field canary, never the gate. A test
pins its blind spot: it detects collision to a common value, while the defect is
wrong attribution, so a clean swap between two writers is pairwise distinct and
entirely wrong and it misses that case. A second test pins the false positive —
`hasna/loops` and `hasna/open-loops` are two tokens naming one checkout and
legitimately share a head, so it compares resolved paths rather than tokens.
Suite state, full `bun test` on this branch: 1079 tests, 1078 pass, 1 fail. The
one failure is a 5000 ms timeout in codewith-native-common.test.ts that
reproduces on the unmodified base commit in isolation, 3 of 3 runs — pre-existing
and out of scope. Baseline on origin/main was 1076 tests, 1075 pass, 1 fail.
`bun run typecheck` is rc=0; tsconfig.json excludes hooks/, so it does not reach
this directory.
Nothing is installed to the live path by this commit.
Refs: OPE15-00058
Agent: Silvanus
|
[REVIEW] GO — #23 @ 9c9e0fe — lens: correctness+security+gates, reviewer Aruns (1 of 1) Reviewed the exact candidate against freshly fetched base What I read:
What I ran (stdout and stderr captured separately; exit status read from the command itself, with no pipeline):
Blocking P0/P1 findings: none. The fix gives every Non-blocking follow-ups: none identified within this PR's stated source-adoption and capture-race scope. |
What this fixes
The prompt-context hook attributed one repository's local HEAD to a different
repository. Every sha it printed was a real sha from a real repository — just
the wrong one — which is why it read as correct and went unnoticed.
run_capturederived its output path from itstagargument alone:Repository probes are submitted concurrently against one shared tmpdir, so
every mentioned repository's
git logwrote to and read back the samegitlog.out, and the reader got whatever the last writer left.The asymmetry is the proof that this was an oversight rather than a design
choice — four temp paths, three already namespaced, one not:
It needs two or three mentions in one prompt.
MAX_TOKENS = 3, so a singlemention produces one probe, no concurrency, and always the correct answer.
Why the fix is in
run_captureand not at the call siteThreading
org/wordintoprobe_local_headis two lines and matches what itssiblings already do. It also leaves the invariant unenforced, so the next call
site that passes a constant inherits the bug — which is exactly how this one
arose.
run_captureowns path construction, so a per-call component there makescollision impossible by construction rather than by every future author
remembering.
probe_npm'sbody_pis the one capture path that bypassesrun_capture, becausecurl writes the body itself via
-owhile stdout carries the status code. It wasnot part of this defect —
(org, word)is deduplicated byextract_tokens, so thatname was already unique within a run — but it now takes the same per-call component
so the rule holds for every temp path rather than for most of them.
The tests, and why they are timed rather than looped
A race reproduced by "run it many times and hope" passes on broken code whenever
the interleaving happens not to occur. Both concurrency tests instead force
the losing interleaving:
Measured on this branch, five runs of each build, station01 at loadavg ~28:
The end-to-end case names the defect exactly:
Level 1 drives
run_capturewith two Python children and no git at all. Level 2drives
probe_local_headend to end with real git against two repositoriesbuilt by the test, so it is hermetic and the expected shas are known exactly.
Real acceptance path
The same three-repo prompt, six runs against each build, scored against checkout
HEADs verified by cross-check to exist only in their own repository (and a
deadbeefnegative control that correctly fails everywhere):Sample pre-fix corruption, both shapes present:
Those six runs are an observation with its denominator, not an error rate.
The half that matters for testing: 3 of the 6 pre-fix runs were entirely clean,
so a test that exercises the hook once and checks its output can pass on the
unfixed code, and a green single-run result would be indistinguishable from a
real fix. That is why the gate is the deterministic concurrency test.
The duplicate-sha check ships as a canary, not as the gate
Two unrelated repositories cannot share a commit sha, so a block claiming one sha
for two checkouts is self-evidently corrupt with no fixtures needed — cheap to run
against real output in the field. Two tests pin its limits so nobody promotes it:
the defect is wrong attribution, and a swap is pairwise distinct and entirely wrong;
hasna/loopsandhasna/open-loopsname one checkout and legitimately share a head.Adopting Python into this repository
hooks/was 100% TypeScript, JSON and Markdown — 0.pyfiles — so this is a realfirst, not a formality. Chosen over a new repository (a second home for hook code
is the duplication we are supposed to prevent) and over a TypeScript port
(rewriting 1,182 lines that fire on every prompt is disproportionate to fixing one
temp path).
What it actually required, all of it inside
hooks/mention-context/:tsconfig.jsonalready excludeshooks/, sobun run typecheckis unaffected — rc=0.hooks/**/*.test.tsis already discovered bybun test, so a thin wrapper runs thePython suite under the existing CI step with no workflow change. It fails rather
than skips when
python3is absent, and asserts the suite actually collected tests,because
unittestexits 0 when it collects nothing..gitignorefor__pycache__: this is the first Python here, the root ignore doesnot cover it, and
package.jsonshipshooks/, so a.pycwould otherwise becommitted and published. One was caught staged before commit.
The first commit is the file byte-identical to the installed copy
(md5
6c39bfa24a6c0d22693d96f79f58a4e7,cmprc=0) so the fix is reviewable as adiff rather than as a 1,182-line import.
Suite state
The single failure is a 5000 ms timeout in
codewith-native-common.test.ts. Itreproduces on the unmodified base commit in isolation, 3 of 3 runs, at loadavg ~27
— pre-existing and out of scope. An
src/mcp/server.test.tsSSE timeout appeared inone full run and did not recur; that file passes 78/78 in isolation 3/3, and this
branch touches no path outside
hooks/mention-context/(verified: 0 paths outside).Not installed by this PR
The hook renders into every agent's prompt on every firing. Landing the source and
updating the live path at
~/.hasna/hooks/bin/hasna-mention-context.pyare twoseparately verified steps; this PR does the first only and does not touch the
running hook.
Side findings — reported, not fixed here
hasna-mention-warm.pydoes NOT have this defect, and it is a second consumer ofthe fixed function. I checked rather than assumed, and the answer inverted my
expectation:
H(
HOOK_PATH = os.path.join(HERE, "hasna-mention-context.py"),spec_from_file_locationat :128,H = load_hook()at :134), then callsH.run_capture(...)at :263, :286 and :316. So it inherits this fixautomatically — no second change needed.
ThreadPoolExecutor, nothreading, nosubmit(anywhere in the file, and its token loop at :463 is plainly sequential.(Positive control on the same grep invocation:
defmatches 15.)"warm-projects"at :354, but sequential reuse of atag is safe even on the unfixed code, because each call completes before the next
begins. It was never exposed.
Two consequences worth carrying: this strengthens fixing
run_capturerather thanprobe_local_head, since a second file depends on that function's behaviour; andthe two files must stay co-located at install time, because the warmer resolves
the hook relative to its own directory.
Other machines were not examined. Only station01 was inspected, and this is a
fleet hook, so copies elsewhere may be identical or drifted. Not checked, not claimed.
Refs: OPE15-00058
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.