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

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to the claude-plugins project will be documented in this fil
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`.
 
+### code-review v3.7.2
+
+#### Fixed
+- **The coverage-critic cache key now covers the diff bundle the critic reads, instead of being correct by accident (ISS-9674).** `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 actually reads — and it was in no key component. As with the `signals/` namespace in v3.7.1, `diff_tip` is a ref *name* and never a commit id, so nothing in the tuple varied with the diff. The namespace did not misfire only because `signals_hash` hashes `extract_signals.json`, which carries a wall-clock `generated_at`: the key changed every run, so the cache never hit. That made the obvious optimization — strip `generated_at` so the coverage-critic cache finally hits — a change that would have served one review's coverage plan to a different review from the same pooled worktree, with no test failing. The key now includes `diff_summary_hash` (`signal_input_hash` over that bundle) and `cmd_coverage_critic_prepare` builds the input before computing the key rather than after. The manifest reports `diff_summary_hash` alongside its sibling component hashes. Adds a pooled-lane regression test that holds plan, signals, prompt, roster, and diff tip constant and varies only the diff; it fails against the previous key. Also corrects the key's docstring, which claimed all five components were content-addressed when `diff_tip` is not, and the `SCHEMA.md` cache-namespace row, which documented the wrong path shape and three of the five components.
+
 ### code-review v3.7.1
 
 #### Fixed

plugins/code-review/.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "code-review",
   "description": "Code review plugin",
-  "version": "3.7.1",
+  "version": "3.7.2",
   "author": {
     "name": "ClosedLoop",
     "email": "support@closedloop.ai"

plugins/code-review/SCHEMA.md

@@ -586,7 +586,7 @@ A MAJOR `schema_version` bump invalidates every cache namespace at once.
 | ------------------- | ------------------------------------------------- | ------------------------------------------------------------- | ------ |
 | BHA findings        | `<CACHE_DIR>/bha/<file_hash>.json`                | file_content_hash + prompt_hash + model_id + schema_version   | 30 d   |
 | Signal extraction   | `<CACHE_DIR>/signals/<key>.json`                  | diff_tip + input_hash + taxonomy_hash + signal_prompt_hash     | 7 d    |
-| Coverage critic     | `<CACHE_DIR>/coverage_critic/<diff_tip>.json`     | coverage_plan_initial_hash + signals_hash + critic_prompt_hash | 7 d   |
+| Coverage critic     | `<CACHE_DIR>/coverage_critic/<key>.json`          | coverage_plan_initial_hash + signals_hash + diff_summary_hash + diff_tip + critic_prompt_hash + available_reviewers_hash | 7 d |
 | Verification        | `<CACHE_DIR>/verifications/<finding_id>.json`     | finding_id + file_content_hash + verifier_model + verifier_prompt_hash | 30 d |
 | Overrides           | `<CACHE_DIR>/overrides/<finding_id>.json`         | finding_id (file content change invalidates)                  | 90 d   |
 

plugins/code-review/tools/python/code_review_helpers.py

@@ -7860,16 +7860,32 @@ def _stable_json_hash(payload: Any) -> str:
 def coverage_critic_cache_key(
     coverage_plan_initial_hash: str,
     signals_hash: str,
+    diff_summary_hash: str,
     diff_tip: str,
     prompt_hash: str,
     available_reviewers_hash: str,
 ) -> str:
     """Cache key for the ``coverage_critic`` namespace (PLN-725).
 
-    Tuple ``(coverage_plan_initial_hash, signals_hash, diff_tip,
-    prompt_hash, available_reviewers_hash)`` is the complete set of
-    inputs the critic is a pure function of. All five are
-    content-addressed.
+    Tuple ``(coverage_plan_initial_hash, signals_hash, diff_summary_hash,
+    diff_tip, prompt_hash, available_reviewers_hash)`` is the complete set
+    of inputs the critic is a pure function of. All are content-addressed
+    except ``diff_tip``, which is only a ref *name* (``"HEAD"`` for every
+    local branch review, ``origin/<branch>`` for a PR) and therefore
+    discriminates nothing on its own — it is a coarse extra component,
+    never the diff identity.
+
+    ``diff_summary_hash`` (ISS-9674) fingerprints the ``diff_summary``
+    half of ``_build_coverage_critic_input`` — which is literally
+    ``_build_signal_input(diff_data, intent_summary=None)``, the bundle
+    the critic reads. Before it was added, no key component varied with
+    the diff at all; the key changed run-to-run only because
+    ``signals_hash`` transitively covered ``extract_signals.json``'s
+    wall-clock ``generated_at``. That made this namespace correct by
+    accident and permanently cold, and it meant the obvious optimization
+    — strip ``generated_at`` so the cache finally hits — would have
+    served one review's coverage plan to another review from the same
+    pooled worktree, which is ISS-8961 in this namespace.
 
     ``available_reviewers_hash`` is the deterministic hash of the
     AVAILABLE roster the agent will actually see (sorted, dedup'd, and
@@ -7882,6 +7898,7 @@ def coverage_critic_cache_key(
     payload = (
         (coverage_plan_initial_hash or "") + "\0"
         + (signals_hash or "") + "\0"
+        + (diff_summary_hash or "") + "\0"
         + (diff_tip or "") + "\0"
         + (prompt_hash or "") + "\0"
         + (available_reviewers_hash or "")
@@ -8351,9 +8368,16 @@ def cmd_coverage_critic_prepare(args: argparse.Namespace) -> int:
     # stale (could propose a now-removed reviewer), so it must key
     # the cache.
     available_reviewers_hash = _available_reviewers_hash(available_reviewers)
+    # ISS-9674: build the agent input before the key, so the key can cover
+    # the diff bundle the critic actually reads. Mirrors the ordering
+    # ``cmd_extract_signals_prepare`` adopted for the same reason.
+    main_input, diff_summary = _build_coverage_critic_input(
+        plan_initial, extract_signals, diff_data, available_reviewers,
+    )
+    diff_summary_hash = signal_input_hash(diff_summary)
     key = coverage_critic_cache_key(
-        plan_initial_hash, signals_hash, diff_tip, prompt_hash,
-        available_reviewers_hash,
+        plan_initial_hash, signals_hash, diff_summary_hash, diff_tip,
+        prompt_hash, available_reviewers_hash,
     )
 
     cached = _read_cached_coverage_critic(cache_dir, key)
@@ -8376,9 +8400,6 @@ def cmd_coverage_critic_prepare(args: argparse.Namespace) -> int:
             return 1
         return _emit_summary(manifest)
 
-    main_input, diff_summary = _build_coverage_critic_input(
-        plan_initial, extract_signals, diff_data, available_reviewers,
-    )
     input_path = cr_dir / "coverage_critic_input.json"
     diff_summary_path = cr_dir / "coverage_critic_diff_summary.json"
     with open(input_path, "w") as f:
@@ -8391,6 +8412,7 @@ def cmd_coverage_critic_prepare(args: argparse.Namespace) -> int:
         "cache_key": key,
         "coverage_plan_initial_hash": plan_initial_hash,
         "signals_hash": signals_hash,
+        "diff_summary_hash": diff_summary_hash,
         "prompt_hash": prompt_hash,
         "available_reviewers_hash": available_reviewers_hash,
         "input_path": str(input_path),

plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/coverage.json

@@ -4,6 +4,7 @@
     "cache_key": "<CACHE_KEY>",
     "coverage_plan_initial_hash": "<PLAN_INITIAL_HASH>",
     "coverage_state": "<CR_DIR>/coverage.json",
+    "diff_summary_hash": "<DIFF_SUMMARY_HASH>",
     "diff_summary_path": "<CR_DIR>/coverage_critic_diff_summary.json",
     "input_path": "<CR_DIR>/coverage_critic_input.json",
     "model": "sonnet",

plugins/code-review/tools/python/prefix_golden_harness.py

@@ -682,6 +682,11 @@ def walk_prefix(
     # only nondeterminism, not coverage.
     "signals_hash": "<SIGNALS_HASH>",
     "coverage_plan_initial_hash": "<PLAN_INITIAL_HASH>",
+    # Fixture-coupled rather than wall-clock: deterministic for a given
+    # fixture, but pinning it would make every edit to a fixture's diff
+    # require a hand-updated digest here. Its composition is pinned by a
+    # literal-digest unit test instead (same treatment as prompt_hash).
+    "diff_summary_hash": "<DIFF_SUMMARY_HASH>",
     "emitted_at": "<TS>",
     "timestamp": "<TS>",
     "generated_at": "<TS>",

plugins/code-review/tools/python/test_code_review_helpers.py

@@ -14083,43 +14083,62 @@ class TestCoverageCriticCacheKey:
     def test_same_inputs_same_key(self) -> None:
         from code_review_helpers import coverage_critic_cache_key
         assert (
-            coverage_critic_cache_key("p", "s", "t", "h", "a")
-            == coverage_critic_cache_key("p", "s", "t", "h", "a")
+            coverage_critic_cache_key("p", "s", "d", "t", "h", "a")
+            == coverage_critic_cache_key("p", "s", "d", "t", "h", "a")
         )
 
     def test_plan_initial_hash_flip_changes_key(self) -> None:
         from code_review_helpers import coverage_critic_cache_key
         assert (
-            coverage_critic_cache_key("p1", "s", "t", "h", "a")
-            != coverage_critic_cache_key("p2", "s", "t", "h", "a")
+            coverage_critic_cache_key("p1", "s", "d", "t", "h", "a")
+            != coverage_critic_cache_key("p2", "s", "d", "t", "h", "a")
         )
 
     def test_signals_hash_flip_changes_key(self) -> None:
         from code_review_helpers import coverage_critic_cache_key
         assert (
-            coverage_critic_cache_key("p", "s1", "t", "h", "a")
-            != coverage_critic_cache_key("p", "s2", "t", "h", "a")
+            coverage_critic_cache_key("p", "s1", "d", "t", "h", "a")
+            != coverage_critic_cache_key("p", "s2", "d", "t", "h", "a")
+        )
+
+    def test_diff_summary_hash_flip_changes_key(self) -> None:
+        """ISS-9674: the only component that varies with the diff itself."""
+        from code_review_helpers import coverage_critic_cache_key
+        assert (
+            coverage_critic_cache_key("p", "s", "d1", "t", "h", "a")
+            != coverage_critic_cache_key("p", "s", "d2", "t", "h", "a")
+        )
+
+    def test_key_composition_is_pinned_to_a_literal_digest(self) -> None:
+        """Frozen literal, not a re-derivation through the builder.
+
+        A test that recomputed the digest with the same formula would stay
+        green if a component were dropped back out of the tuple.
+        """
+        from code_review_helpers import coverage_critic_cache_key
+        assert coverage_critic_cache_key("p", "s", "d", "t", "h", "a") == (
+            "85531ac31b220ae9a731cb4063b13a05177105d9b4c5872fca093b496af030b3"
         )
 
     def test_diff_tip_flip_changes_key(self) -> None:
         from code_review_helpers import coverage_critic_cache_key
         assert (
-            coverage_critic_cache_key("p", "s", "t1", "h", "a")
-            != coverage_critic_cache_key("p", "s", "t2", "h", "a")
+            coverage_critic_cache_key("p", "s", "d", "t1", "h", "a")
+            != coverage_critic_cache_key("p", "s", "d", "t2", "h", "a")
         )
 
     def test_prompt_hash_flip_changes_key(self) -> None:
         from code_review_helpers import coverage_critic_cache_key
         assert (
-            coverage_critic_cache_key("p", "s", "t", "h1", "a")
-            != coverage_critic_cache_key("p", "s", "t", "h2", "a")
+            coverage_critic_cache_key("p", "s", "d", "t", "h1", "a")
+            != coverage_critic_cache_key("p", "s", "d", "t", "h2", "a")
         )
 
     def test_available_reviewers_hash_flip_changes_key(self) -> None:
         from code_review_helpers import coverage_critic_cache_key
         assert (
-            coverage_critic_cache_key("p", "s", "t", "h", "a1")
-            != coverage_critic_cache_key("p", "s", "t", "h", "a2")
+            coverage_critic_cache_key("p", "s", "d", "t", "h", "a1")
+            != coverage_critic_cache_key("p", "s", "d", "t", "h", "a2")
         )
 
 
@@ -14787,31 +14806,20 @@ def test_malformed_roster_still_returns_one(self, tmp_path: Path) -> None:
     def test_cache_hit_serves_directly(self, tmp_path: Path) -> None:
         from code_review_helpers import (
             CACHE_NAMESPACE_COVERAGE_CRITIC,
-            _available_reviewers_hash,
-            _stable_json_hash,
             cmd_coverage_critic_prepare,
-            coverage_critic_cache_key,
-            _coverage_critic_prompt_hash,
-            _default_coverage_critic_prompt_path,
         )
         inputs = _write_coverage_critic_inputs(tmp_path, signals={"signals": []})
         args = self._args(tmp_path, inputs)
         cache_dir = Path(args.cache_dir)
         (cache_dir / CACHE_NAMESPACE_COVERAGE_CRITIC).mkdir(parents=True)
 
-        plan_hash = _stable_json_hash(inputs["plan_initial"])
-        signals_hash = _stable_json_hash({"signals": []})
-        prompt_hash = _coverage_critic_prompt_hash(
-            _default_coverage_critic_prompt_path(),
-        )
-        # Roster hash must mirror what prepare computes (post-filter) —
-        # plan_initial has bug_hunter_a in required, so the filter is a
-        # no-op against the default fixture (which lists
-        # accessibility-expert + i18n-expert).
-        available_hash = _available_reviewers_hash(inputs["available"])
-        key = coverage_critic_cache_key(
-            plan_hash, signals_hash, "abc123", prompt_hash, available_hash,
-        )
+        # Seed under the key prepare itself reports on a miss, rather than
+        # re-deriving it here: a fixture that rebuilt the key through the
+        # builder under test would stay green if the composition changed.
+        assert cmd_coverage_critic_prepare(args) == 0
+        miss_manifest = _read_coverage_section(Path(args.cr_dir), "critic")
+        assert miss_manifest["status"] == "needs_agent"
+        key = miss_manifest["cache_key"]
         cached_payload = {
             "required": inputs["plan_initial"]["required"],
             "best_effort": [
@@ -14836,6 +14844,57 @@ def test_cache_hit_serves_directly(self, tmp_path: Path) -> None:
         # written_at metadata stripped from canonical output
         assert "written_at" not in final
 
+    def test_pooled_lane_with_a_different_diff_misses_prior_cache(
+        self, tmp_path: Path,
+    ) -> None:
+        """ISS-9674: the coverage-critic twin of the ISS-8961 pooled-lane bug.
+
+        Production shape: pooled worktrees give every lane in a directory
+        the same cache dir, and ``diff_tip`` is a ref name (``"HEAD"`` for a
+        local branch review), so it separates nothing. Here everything the
+        old key covered is held constant -- same plan_initial, same signals,
+        same prompt, same roster, same diff_tip -- and only the DIFF varies.
+
+        Against the pre-ISS-9674 key this fails: the two lanes compute the
+        same key and lane 2 is served lane 1's coverage plan. It passes only
+        because the key now covers the diff bundle the critic reads.
+        """
+        from code_review_helpers import (
+            CACHE_NAMESPACE_COVERAGE_CRITIC,
+            cmd_coverage_critic_prepare,
+        )
+
+        def _lane(name: str, added: dict[str, str]) -> str:
+            lane_root = tmp_path / name
+            lane_root.mkdir()
+            inputs = _write_coverage_critic_inputs(
+                lane_root,
+                signals={"signals": []},
+                diff_data={
+                    "files_to_review": ["src/Modal.tsx"],
+                    "file_statuses": {"src/Modal.tsx": "M"},
+                    "patch_lines": {
+                        "src/Modal.tsx": {
+                            "added_lines": added, "removed_lines": {},
+                        },
+                    },
+                },
+            )
+            args = self._args(lane_root, inputs)
+            # One shared pooled cache directory across both lanes.
+            args.cache_dir = str(shared_cache)
+            assert cmd_coverage_critic_prepare(args) == 0
+            manifest = _read_coverage_section(Path(args.cr_dir), "critic")
+            assert manifest["status"] == "needs_agent"
+            return str(manifest["cache_key"])
+
+        shared_cache = tmp_path / "cr-cache-global-repo-pool"
+        (shared_cache / CACHE_NAMESPACE_COVERAGE_CRITIC).mkdir(parents=True)
+
+        lane1_key = _lane("lane1", {"42": "<div role='dialog'>"})
+        lane2_key = _lane("lane2", {"42": "<div onClick={submit}>"})
+        assert lane1_key != lane2_key
+
     def test_roster_shrink_misses_prior_cache(self, tmp_path: Path) -> None:
         """Concrete failure mode the PR comment described: same diff,
         same plan_initial, same signals, same prompt, but the
@@ -14847,9 +14906,11 @@ def test_roster_shrink_misses_prior_cache(self, tmp_path: Path) -> None:
         from code_review_helpers import (
             CACHE_NAMESPACE_COVERAGE_CRITIC,
             _available_reviewers_hash,
+            _build_coverage_critic_input,
             _stable_json_hash,
             cmd_coverage_critic_prepare,
             coverage_critic_cache_key,
+            signal_input_hash,
             _coverage_critic_prompt_hash,
             _default_coverage_critic_prompt_path,
         )
@@ -14869,8 +14930,14 @@ def test_roster_shrink_misses_prior_cache(self, tmp_path: Path) -> None:
         prompt_hash = _coverage_critic_prompt_hash(
             _default_coverage_critic_prompt_path(),
         )
+        _, diff_summary = _build_coverage_critic_input(
+            inputs["plan_initial"], {"signals": []},
+            json.loads(inputs["paths"]["diff_data"].read_text()),
+            ["accessibility-expert", "i18n-expert"],
+        )
+        diff_summary_hash = signal_input_hash(diff_summary)
         old_key = coverage_critic_cache_key(
-            plan_hash, signals_hash, "abc123", prompt_hash,
+            plan_hash, signals_hash, diff_summary_hash, "abc123", prompt_hash,
             _available_reviewers_hash(["accessibility-expert", "i18n-expert"]),
         )
         # Seed a cache entry under the OLD roster that proposes the