fix(code-review): key signal cache on the diff, not the worktree

[mikeangstadt]

Summary

• ISS-8961. /code-review's signal-extraction cache served one review's signals to a different review. The signals/ namespace key was (diff_tip, taxonomy_hash, prompt_hash) and diff_tip is a ref name, never a commit id — cmd_resolve_scope leaves it as the literal "HEAD" for every local branch review, and sets it to origin/<head_ref> for a PR. So no component of the key varied with the diff. The cache directory is ~/.claude/cr-cache-global-repo-<basename of git rev-parse --show-toplevel>, i.e. the worktree directory name. Any two reviews run from the same directory therefore shared both the directory and the key, and the second was handed the first's extract_signals.json — describing a diff that was never under review. Pooled worktrees make that the normal case, not an edge case.
• The key now includes signal_input_hash: a SHA-256 over the canonical serialization of the agent input bundle extract-signals-prepare builds (files, per-file added/removed excerpts, intent hint). That bundle is exactly what the extraction agent reads, so the key covers every input the extraction is a function of, and a different diff produces a different key even at an identical path, branch, and tip.
• extract-signals-prepare now builds the bundle before computing the key rather than only on a miss.
• A miss is preferred to a wrong hit. When the bundle carries no changed files there is no diff identity to key on, so the run bypasses the cache in both directions: an empty cache_key plus cache_bypass_reason: "no-changed-files" in the manifest, which suppresses the read and — through extract-signals-consolidate's existing if cache_key: guard — the write.
• Corrected the --diff-tip help, which claimed the flag was a SHA.

Breaking changes

None for callers — --diff-tip is still accepted and still a key component. Cached extractions written under the old key scheme become misses, so the first review in each cache directory re-runs one Haiku extraction. signal_extraction_cache_key() takes a fourth argument; it has no callers outside this module and its tests.

UI Feature Flag

[ui-flag:N/A] — tooling-only change in a Python helper; no UI surface in this repo.

Test plan

• TestPooledWorktreeSignalCacheIsolation::test_second_lane_does_not_receive_first_lanes_signals reproduces production shape: one cache directory, diff_tip="HEAD" for both lanes, two different diffs. Lane 1 prepares + consolidates real signals into the cache; lane 2 must get a different key, status: "needs_agent", and no extract_signals.json.
• Counterfactual: with code_review_helpers.py reverted (git stash push -- <that file>), that test fails and lane 2's manifest reads "status": "cache_hit" — the production bug, reproduced. Restored, green.
• test_unkeyable_input_bypasses_the_cache_in_both_directions drives the empty-diff path through prepare and consolidate and asserts nothing lands in the signals/ namespace for a later lane to hit.
• Key composition is pinned to frozen literal digests (test_key_composition_is_pinned_to_a_literal_digest, test_hash_is_pinned_to_a_literal_digest) rather than re-derived through the helper under test, so dropping the input-hash component back out of the tuple cannot stay green.
• test_cache_hit_serves_directly_without_agent no longer re-derives the key to seed its fixture; it prepares once and seeds under the key the manifest reports.
• Golden golden_prefix_empty_diff/expected/extract_signals_manifest.json updated: that fixture's prefix run has no changed files, so it now records cache_bypass_reason. Deliberate behavior change in this diff.
• ruff check . clean; uv run pyright 0 errors.
• uv run pytest plugins/: 3 failures, all pre-existing on clean main and unrelated — TestOverrideCache::test_verify_prepare_short_circuits_on_valid_override and two TestPR114ReviewFixes cases pin an override fixture dated 2026-05-29T22:00:00+00:00 against a 90-day overrides TTL, so they became a wall-clock time bomb around 2026-08-27.

[mikeangstadt]

Follow-up commit 6346f8c addresses findings from two review passes on 880fad6.

Closed a false green in my own regression test. test_second_lane_does_not_receive_first_lanes_signals varied the changed path as well as the content, so a key hashing only the file list would still have passed it. Confirmed by mutation: narrowing the call site to signal_input_hash({"files": agent_input["files"]}) left all 18 signal-extraction tests green. That mutation is the ISS-8961 bug in its most likely real shape — successive reviews of one branch touch the same paths with the same line counts and differ only in content. test_same_file_different_content_is_not_served_a_stale_hit now holds path, status, and line counts identical and varies only the added line; it kills that mutation.

SSOT. signal_input_hash was a byte-identical second copy of _stable_json_hash, the module's existing owner of the deterministic cache-key JSON rule (equivalence checked on ASCII, non-ASCII, and empty payloads). It now delegates.

Run-to-run stability is now pinned. The highest-consequence regression this design admits is someone adding a run-varying field to _build_signal_input — every review becomes a permanent miss and re-runs Haiku forever, and the golden harness cannot catch it because it normalizes cache_key to a placeholder. test_bundle_from_identical_diff_data_is_stable_across_builds covers it.

Diagnosability. The manifest emitted taxonomy_hash and prompt_hash so an operator can see which component moved a key, but omitted the one component that now actually varies. It carries input_hash too, normalized in the golden harness like its siblings.

Corrected the bypass rationale. The docstring claimed a zero-file bundle "has no diff identity to key on". Not true — it hashes fine, and two genuinely empty reviews would legitimately share an entry. The real reason is that it is the shape a degraded parse-diff takes, where caching would persist a degraded extraction for the namespace TTL and serve it to every later empty-looking run. The cost is now named as well: a genuinely empty review re-dispatches its extraction every run.

Also noted, not changed here: a review pass observed that coverage_critic_cache_key omits its own _build_signal_input bundle (_build_coverage_critic_input passes it as diff_summary) and is safe today only because signals_hash transitively covers extract_signals.json's wall-clock generated_at. Stripping that timestamp to make the critic cache actually hit would reproduce this bug in the coverage_critic/ namespace with no test failing. Different namespace and different key, so it is out of scope for this PR rather than folded in silently.

Gates on 6346f8c: ruff check clean, uv run pyright 0 errors, uv run pytest plugins/ 2113 passed. The 3 failures are pre-existing and red on main — TestOverrideCache and two TestPR114ReviewFixes cases pin an override fixture dated 2026-05-29T22:00:00+00:00 against the 90-day overrides TTL, so they went red on 2026-08-27; main's last CI run was 2026-08-17. They block this PR and are unrelated to it.

[mikeangstadt]

73d830e unblocks this PR by repairing a time-bomb fixture. This is a test-mechanics repair, not an expectation change — flagging that explicitly so it is not read as manufacturing a green.

The behaviour under test is unchanged and still asserted. A valid, within-TTL override still short-circuits verification (override_hits populated, to_verify empty), still routes through consolidate as RE_ASSERTED, and still lands in stats.verification.by_reviewer. No assertion was weakened, widened, skipped, or deleted; the TTL gate itself is untouched.

What was actually wrong. Three tests hardcoded asserted_at: "2026-05-29T22:00:00+00:00" while _override_is_expired sweeps the overrides namespace against the real wall clock with a 90-day TTL. So the fixture aged out on 2026-08-27 and the suite has been red on main ever since with no code change — main's last CI run was 2026-08-17, green, which is why nobody saw it. A test that goes red on a calendar date rather than on a behaviour change was never pinning what it claimed to.

The repair. Both sides of the comparison are now literals — the fixture timestamp and the clock (_OVERRIDE_FIXTURE_ASSERTED_AT / _OVERRIDE_FIXTURE_NOW, pinned via a pinned_override_clock fixture) — so the gap stays a constant 3 days forever. Pinned rather than made relative, so the fixture cannot drift into some other time-based contract later. The tests whose subject is the TTL gate (test_override_dropped_when_ttl_exceeded, test_override_honored_when_ttl_within_bounds) deliberately keep offsetting from the real clock and are untouched.

Proof the pin did not neuter the gate. Forcing _override_is_expired to return True turns all three repaired tests red (plus two others). They still fail when the honoring behaviour breaks — they simply no longer fail on the calendar.

uv run pytest plugins/: 2116 passed, 0 failed — the first fully green run since 2026-08-27. ruff check clean, uv run pyright 0 errors.