Skip to content

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

Open
mikeangstadt wants to merge 1 commit into
fix/iss-8961-review-signal-cache-keyfrom
fix/iss-9674-coverage-critic-cache-key
Open

fix(code-review): key coverage-critic cache on the diff too#193
mikeangstadt wants to merge 1 commit into
fix/iss-8961-review-signal-cache-keyfrom
fix/iss-9674-coverage-critic-cache-key

Conversation

@mikeangstadt

Copy link
Copy Markdown
Collaborator

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.

ISS-9674, the coverage-critic twin of ISS-8961.

- 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, None) -- the
  bundle the critic reads -- and it was in no key component. diff_tip is
  a ref name, never a commit id, so nothing varied with the diff.
- It did not misfire only because signals_hash hashes
  extract_signals.json, which carries a wall-clock generated_at, so the
  key changed every run and the namespace never hit. That made the
  obvious optimization -- strip generated_at so the cache finally hits --
  a change that would serve one review's coverage plan to another review
  from the same pooled worktree, with no test failing.
- Add diff_summary_hash to the key; build the input before the key
  rather than after; report the hash in the manifest beside its
  siblings.
- Correct the docstring's false "All five are content-addressed" claim
  and the SCHEMA.md row (wrong path shape, 3 of 5 components listed).
- Reseed the cache-hit test from the key prepare reports instead of
  re-deriving it through the builder under test.

Testing: uv run pytest plugins/ -- 2119 passed, 0 failed. Counterfactual:
dropping diff_summary_hash from the payload turns 3 tests red, including
test_pooled_lane_with_a_different_diff_misses_prior_cache, which holds
plan, signals, prompt, roster and diff tip constant and varies only the
diff. ruff check; uv run pyright 0 errors.

Risks: one-time invalidation of the coverage_critic namespace, which in
practice never hit anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant