diff --git a/CHANGELOG.md b/CHANGELOG.md index fca8546..de946f9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index de55d5a..4bc5c51 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/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" diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index f00bb54..b376dff 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -586,7 +586,7 @@ A MAJOR `schema_version` bump invalidates every cache namespace at once. | ------------------- | ------------------------------------------------- | ------------------------------------------------------------- | ------ | | BHA findings | `/bha/.json` | file_content_hash + prompt_hash + model_id + schema_version | 30 d | | Signal extraction | `/signals/.json` | diff_tip + input_hash + taxonomy_hash + signal_prompt_hash | 7 d | -| Coverage critic | `/coverage_critic/.json` | coverage_plan_initial_hash + signals_hash + critic_prompt_hash | 7 d | +| Coverage critic | `/coverage_critic/.json` | coverage_plan_initial_hash + signals_hash + diff_summary_hash + diff_tip + critic_prompt_hash + available_reviewers_hash | 7 d | | Verification | `/verifications/.json` | finding_id + file_content_hash + verifier_model + verifier_prompt_hash | 30 d | | Overrides | `/overrides/.json` | finding_id (file content change invalidates) | 90 d | diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index c9e4fcf..d4e878a 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/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/`` 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), diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/coverage.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/coverage.json index fe7fb92..685b05e 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/coverage.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/coverage.json @@ -4,6 +4,7 @@ "cache_key": "", "coverage_plan_initial_hash": "", "coverage_state": "/coverage.json", + "diff_summary_hash": "", "diff_summary_path": "/coverage_critic_diff_summary.json", "input_path": "/coverage_critic_input.json", "model": "sonnet", diff --git a/plugins/code-review/tools/python/prefix_golden_harness.py b/plugins/code-review/tools/python/prefix_golden_harness.py index 1f53fb6..2055f59 100644 --- a/plugins/code-review/tools/python/prefix_golden_harness.py +++ b/plugins/code-review/tools/python/prefix_golden_harness.py @@ -682,6 +682,11 @@ def walk_prefix( # only nondeterminism, not coverage. "signals_hash": "", "coverage_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": "", "emitted_at": "", "timestamp": "", "generated_at": "", diff --git a/plugins/code-review/tools/python/test_code_review_helpers.py b/plugins/code-review/tools/python/test_code_review_helpers.py index 74f7c58..59f0fa3 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/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": "
"}) + lane2_key = _lane("lane2", {"42": "
"}) + 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