fix(code-review): key coverage-critic cache on the diff too

[mikeangstadt]

Stacked on #192 (base is fix/iss-8961-review-signal-cache-key, not main). Retarget to main after #192 merges. ISS-9674.

Summary

• This is ISS-8961 in the coverage_critic/ namespace, and it was safe only by accident. coverage_critic_cache_key was (coverage_plan_initial_hash, signals_hash, diff_tip, prompt_hash, available_reviewers_hash). _build_coverage_critic_input returns (main_input, diff_summary) where diff_summary is literally _build_signal_input(diff_data, intent_summary=None) — the bundle the critic reads — and it appeared in no key component. As in fix(code-review): key signal cache on the diff, not the worktree #192, diff_tip is a ref name and never a commit id, so nothing in the tuple varied with the diff, while the cache directory is the pooled worktree's basename.
• The trap this closes. The namespace did not misfire only because signals_hash hashes extract_signals.json, which carries a wall-clock generated_at. The key therefore changed every run and the cache never hit. Anyone looking at a namespace with a 0% hit rate would find that timestamp, remove it, measure a real speedup, and ship a critic that serves one review's coverage plan to a different review — with no test failing. That is the whole reason this is a separate, filed ticket rather than a footnote.
• The key now includes diff_summary_hash (signal_input_hash over that bundle), and cmd_coverage_critic_prepare builds the agent input before computing the key rather than after. The manifest reports diff_summary_hash beside its sibling component hashes.
• Corrects the key docstring's false claim that "All five are content-addressed" (diff_tip is not), and the SCHEMA.md coverage-critic row, which documented the path as <diff_tip>.json when the code writes <key>.json and listed three of the five components. These are the two doc errors fix(code-review): key signal cache on the diff, not the worktree #192 deliberately left on the adjacent table row to keep that PR to one subject.

Breaking changes

None — additive. One-time invalidation of the coverage_critic/ namespace, which in practice never hit. coverage_critic_cache_key() gains a positional 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

• test_pooled_lane_with_a_different_diff_misses_prior_cache is the regression test: one shared cache directory, and plan_initial, signals, prompt, roster, and diff_tip all held constant so that only the diff varies — which is precisely what the old key could not see.
• Counterfactual: dropping diff_summary_hash back out of the key payload turns 3 tests red, including that one. Restored: green.
• Key composition pinned to a frozen literal digest, not re-derived through the builder under test.
• test_cache_hit_serves_directly no longer re-derives the key to seed its fixture; it prepares once and seeds under the key the manifest reports — the false-green shape caught in fix(code-review): key signal cache on the diff, not the worktree #192's review.
• diff_summary_hash is normalized in the golden harness like its sibling hashes: it is fixture-coupled rather than wall-clock, so pinning it in goldens would force a hand-updated digest on every fixture diff edit. Composition is pinned by the unit test instead.
• ruff check clean; uv run pyright 0 errors; uv run pytest plugins/ 2119 passed, 0 failed.

[shafty023]

Beginning a code-review. Please allow time for me to finish before merging the PR

[shafty023]

P1: This hashes diff_summary, but the critic is still dispatched only manifest.input_path. That JSON contains the plan, signals, and roster; the separately written coverage_critic_diff_summary.json is never passed through singleton dispatch, even though the prompt says the critic receives it.

What happens is a new diff gets a new cache key, then the cache miss still runs a diff-blind critic. Please include the summary (or explicitly pass its path) in the dispatched input contract, hash that exact delivered content, and add a contract test that proves the critic receives it.

[shafty023]

P2: This only proves that the two computed keys differ. _lane() never writes a lane-1 entry under lane1_key, so both calls return needs_agent even if the cache read/reuse path is broken.

Seed a valid lane-1 cache payload in the shared namespace, then assert lane 2 is a miss and does not consume it. That is the regression this ticket needs: a wrong prior hit must be rejected, not just a different key calculated.