From 880fad6fa03c71899cf2bff3adf3a01a6cc15a79 Mon Sep 17 00:00:00 2001 From: mikeangstadt Date: Sun, 6 Sep 2026 11:43:23 -0500 Subject: [PATCH 1/3] fix(code-review): key signal cache on the diff - ISS-8961: the signals/ namespace key was (diff_tip, taxonomy_hash, prompt_hash), and diff_tip is a ref name -- literally "HEAD" for a local branch review -- so no component varied with the diff. The cache dir is ~/.claude/cr-cache-global-repo-, so two reviews from one pooled directory shared directory and key, and the second was served the first's extracted signals. - Add signal_input_hash: SHA-256 over the canonical serialization of the agent input bundle extract-signals-prepare builds, which is exactly what the extraction agent reads. Key is now (diff_tip, input_hash, taxonomy_hash, prompt_hash). - Build the bundle before computing the key rather than only on a miss. - A bundle with no changed files has no diff identity: emit an empty cache_key plus cache_bypass_reason "no-changed-files", which suppresses the read and (via consolidate's empty-key guard) the write. - Correct the --diff-tip help, which claimed the flag was a SHA. Testing: pytest plugins/ (3 pre-existing failures unrelated to this change: TestOverrideCache/TestPR114ReviewFixes fail on clean main because their override fixture is dated 2026-05-29 and the overrides TTL is 90 days). Counterfactual: reverting code_review_helpers.py turns TestPooledWorktreeSignalCacheIsolation red with lane 2's manifest reading status "cache_hit". ruff check; pyright. Risks: cached extractions written under the old key scheme are misses after this lands, so the first review in each cache directory re-runs one Haiku extraction. --- CHANGELOG.md | 5 + .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/SCHEMA.md | 2 +- .../tools/python/code_review_helpers.py | 104 +++++-- .../code-review/tools/python/config/cli.json | 4 +- .../expected/extract_signals_manifest.json | 1 + .../tools/python/test_code_review_helpers.py | 265 +++++++++++++++--- 7 files changed, 322 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17325ab..f6d2baf 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.1 + +#### Fixed +- **Signal extraction no longer serves one review's signals to a different review that shares a cache directory (ISS-8961).** The `signals/` namespace key was `(diff_tip, taxonomy_hash, prompt_hash)`, and `diff_tip` is a ref *name* — `resolve-scope` leaves it as the literal `"HEAD"` for every local branch review and sets it to `origin/` for a PR — so no component of the key varied with the diff. The cache directory is `~/.claude/cr-cache-global-repo-`, which is the worktree directory name, so any two reviews run from the same directory shared both the directory and the key: the second review was served the first review's extracted signals and `extract_signals.json` described a diff that was never under review. The key now includes `signal_input_hash` — a SHA-256 over the canonical serialization of the agent input bundle `extract-signals-prepare` builds (file list, per-file added/removed excerpts, intent hint), which is exactly what the extraction agent reads — so a different diff produces a different key even at an identical path, branch, and tip. `extract-signals-prepare` builds that bundle before computing the key rather than only on a miss. When the bundle carries no changed files there is no diff identity to key on, so the run bypasses the cache in both directions: it emits 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 empty-key guard, the write. A miss re-runs a single Haiku extraction; a wrong hit silently misdescribes the change under review. + ### code v1.14.10 #### Added diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 684332f..de55d5a 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.0", + "version": "3.7.1", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index f41fc34..3e49503 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -585,7 +585,7 @@ A MAJOR `schema_version` bump invalidates every cache namespace at once. | Namespace | Path | Key inputs | TTL | | ------------------- | ------------------------------------------------- | ------------------------------------------------------------- | ------ | | BHA findings | `/bha/.json` | file_content_hash + prompt_hash + model_id + schema_version | 30 d | -| Signal extraction | `/signals/.json` | diff_tip + taxonomy_hash + signal_prompt_hash | 7 d | +| Signal extraction | `/signals/.json` | diff_tip + agent_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 | | 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 84e9431..a1e728d 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -6223,7 +6223,8 @@ def cmd_detect_injection(args: argparse.Namespace) -> int: # --------------------------------------------------------------------------- # Two-step LLM stage modelled on PLN-722's verifier: # 1. ``extract-signals-prepare`` — read diff_data.json + intent + taxonomy, -# compute the cache key, check the cache. On hit: write the final +# build the agent input bundle, compute the cache key from that +# bundle's content hash, check the cache. On hit: write the final # ``extract_signals.json`` immediately. On miss: write the agent input # bundle (diff summary + taxonomy reference) and the manifest the # orchestrator uses to spawn a single Haiku agent. @@ -6240,6 +6241,10 @@ def cmd_detect_injection(args: argparse.Namespace) -> int: SIGNAL_EXTRACTION_MARKER = "signal-extraction-failed" SIGNAL_TAXONOMY_FILENAME = "signal_taxonomy.json" SIGNAL_EXTRACTION_PROMPT_FILENAME = "signal_extraction_prompt.txt" +# Recorded in the manifest when the agent input carries no changed files: +# there is no diff identity to key on, so the run neither reads nor writes +# the signals cache (ISS-8961 — a miss is always cheaper than a wrong hit). +SIGNAL_CACHE_BYPASS_NO_FILES = "no-changed-files" # Cap on per-file excerpt size injected into the agent input. The taxonomy # is the agent's reference — the diff context is the evidence. We need @@ -6331,21 +6336,47 @@ def _signal_extraction_prompt_hash(path: Path) -> str: return hashlib.sha256(path.read_bytes()).hexdigest() +def signal_input_hash(agent_input: dict[str, Any]) -> str: + """Content fingerprint of the exact bundle the extraction agent reads. + + ``_build_signal_input`` is what the agent consumes — file list, per-file + excerpts, and the intent hint — so hashing its canonical serialization + fingerprints every input the extraction is a function of. Sorted keys + make the serialization order-independent; the builder already emits + files and excerpts in a deterministic order. + + ISS-8961: this is the component that makes the key diff-derived. The + ref-name component (``diff_tip``) is ``"HEAD"`` for every local branch + review, so before this hash existed the key was constant across every + review that shared a cache directory — and a pooled worktree makes the + cache directory shared by construction. + """ + canonical = json.dumps( + agent_input, sort_keys=True, separators=(",", ":"), ensure_ascii=True, + ) + return hashlib.sha256(canonical.encode("utf-8", "replace")).hexdigest() + + def signal_extraction_cache_key( - diff_tip: str, taxonomy_hash: str, prompt_hash: str, + diff_tip: str, input_hash: str, taxonomy_hash: str, prompt_hash: str, ) -> str: """Cache key for the ``signals`` namespace (PLN-725). - Tuple ``(diff_tip, taxonomy_hash, prompt_hash)`` is the complete set - of inputs the extraction is a pure function of. Both - ``taxonomy_hash`` and ``prompt_hash`` are content-addressed hashes of - the on-disk asset bytes (``_taxonomy_hash`` and - ``_signal_extraction_prompt_hash``), computed inside + Tuple ``(diff_tip, input_hash, taxonomy_hash, prompt_hash)`` is the + complete set of inputs the extraction is a pure function of. + ``input_hash`` (``signal_input_hash``), ``taxonomy_hash`` and + ``prompt_hash`` are content-addressed hashes of, respectively, the + agent input bundle and the on-disk asset bytes, all computed inside ``cmd_extract_signals_prepare`` rather than taken on faith from - caller-supplied flags. Editing either asset flips the key for real. + caller-supplied flags. Editing any of them flips the key for real. + + ``diff_tip`` is only a ref *name* (``"HEAD"``, ``origin/``), + never a commit id, so it discriminates nothing on its own — keep it as + a coarse extra component, never as the diff identity. """ payload = ( (diff_tip or "") + "\0" + + (input_hash or "") + "\0" + (taxonomy_hash or "") + "\0" + (prompt_hash or "") ) @@ -6535,8 +6566,10 @@ def fail_closed_signal_set(taxonomy: dict[str, Any]) -> list[dict[str, Any]]: def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: """PLN-725 Stage 1a: prep the signal-extraction agent input + check cache. - Reads ``diff_data.json`` and (optionally) an intent summary, computes - the ``(diff_tip, taxonomy_hash, prompt_hash)`` cache key, and either: + Reads ``diff_data.json`` and (optionally) an intent summary, builds the + agent input bundle, computes the + ``(diff_tip, input_hash, taxonomy_hash, prompt_hash)`` cache key, and + either: - **Cache hit** — writes the cached extraction directly to ``/extract_signals.json`` and emits a manifest with @@ -6548,6 +6581,13 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: ``taxonomy_path``, ``prompt_path`` so the orchestrator can spawn a single Haiku agent. + ISS-8961: the bundle is built *before* the key so the key is derived + from the diff the agent will actually read. A degenerate bundle (no + changed files) has no diff identity to key on, so the cache is bypassed + in both directions — miss now, no cache write later — rather than + keyed on the components that remain, which are identical across every + review sharing a cache directory. + Always exits 0; structural failures (no diff_data, malformed taxonomy) print to stderr and return 1. """ @@ -6599,12 +6639,31 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: return 1 taxonomy_hash = _taxonomy_hash(taxonomy_bytes) - key = signal_extraction_cache_key(diff_tip, taxonomy_hash, prompt_hash) + + intent_summary: dict[str, Any] | None = None + if intent_path is not None: + try: + with open(intent_path) as f: + intent_summary = json.load(f) + except (OSError, json.JSONDecodeError): + intent_summary = None + if not isinstance(intent_summary, dict): + intent_summary = None + + agent_input = _build_signal_input(diff_data, intent_summary) + keyable = bool(agent_input.get("files")) + key = ( + signal_extraction_cache_key( + diff_tip, signal_input_hash(agent_input), taxonomy_hash, prompt_hash, + ) + if keyable + else "" + ) output_path = cr_dir / "extract_signals.json" manifest_path = cr_dir / "extract_signals_manifest.json" - cached = _read_cached_signals(cache_dir, key) + cached = _read_cached_signals(cache_dir, key) if key else None if cached is not None: # Strip cache-only metadata before writing the canonical output. canonical = {k: v for k, v in cached.items() if k != "written_at"} @@ -6623,17 +6682,6 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: "model": model, }) - intent_summary: dict[str, Any] | None = None - if intent_path is not None: - try: - with open(intent_path) as f: - intent_summary = json.load(f) - except (OSError, json.JSONDecodeError): - intent_summary = None - if not isinstance(intent_summary, dict): - intent_summary = None - - agent_input = _build_signal_input(diff_data, intent_summary) input_path = cr_dir / "extract_signals_input.json" with open(input_path, "w") as f: json.dump(agent_input, f, indent=2) @@ -6643,7 +6691,7 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: taxonomy_snapshot_path = cr_dir / "extract_signals_taxonomy.json" taxonomy_snapshot_path.write_bytes(taxonomy_bytes) - return _write_and_emit_manifest(manifest_path, { + manifest: dict[str, Any] = { "status": "needs_agent", "cache_key": key, "taxonomy_hash": taxonomy_hash, @@ -6653,7 +6701,13 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: "prompt_path": str(prompt_path), "output_path": str(output_path), "model": model, - }) + } + if not keyable: + # An empty cache_key also stops consolidate writing this run into + # the cache, so a bundle with no diff identity is neither served + # from the cache nor served to a later lane. + manifest["cache_bypass_reason"] = SIGNAL_CACHE_BYPASS_NO_FILES + return _write_and_emit_manifest(manifest_path, manifest) def cmd_extract_signals_consolidate(args: argparse.Namespace) -> int: diff --git a/plugins/code-review/tools/python/config/cli.json b/plugins/code-review/tools/python/config/cli.json index db7bf3e..fd9269b 100644 --- a/plugins/code-review/tools/python/config/cli.json +++ b/plugins/code-review/tools/python/config/cli.json @@ -284,7 +284,7 @@ "--diff-tip" ], "required": true, - "help": "Diff tip SHA for cache key" + "help": "Diff tip ref name; a coarse cache-key component, not the diff identity." }, { "flags": [ @@ -297,7 +297,7 @@ "flags": [ "--cache-dir" ], - "help": "Optional cache directory; fresh (diff_tip, taxonomy_hash, prompt_hash) tuples are served from the signals/ namespace." + "help": "Optional cache directory; fresh (diff_tip, input_hash, taxonomy_hash, prompt_hash) tuples are served from the signals/ namespace." }, { "flags": [ diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json index 1535b27..03ce9d0 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json @@ -1,4 +1,5 @@ { + "cache_bypass_reason": "no-changed-files", "cache_key": "", "input_path": "/extract_signals_input.json", "model": "haiku", 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 df6bbc8..70efc75 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -12222,30 +12222,79 @@ def test_rejects_invalid_rmc(self, tmp_path: Path) -> None: class TestSignalExtractionCacheKey: - """Cache key contract: tuple of (diff_tip, taxonomy_hash, prompt_hash).""" + """Key contract: tuple of (diff_tip, input_hash, taxonomy_hash, prompt_hash).""" def test_same_inputs_same_key(self) -> None: from code_review_helpers import signal_extraction_cache_key - a = signal_extraction_cache_key("dt", "tax", "ph") - b = signal_extraction_cache_key("dt", "tax", "ph") + a = signal_extraction_cache_key("dt", "ih", "tax", "ph") + b = signal_extraction_cache_key("dt", "ih", "tax", "ph") assert a == b def test_diff_tip_flip_changes_key(self) -> None: from code_review_helpers import signal_extraction_cache_key - a = signal_extraction_cache_key("dt1", "tax", "ph") - b = signal_extraction_cache_key("dt2", "tax", "ph") + a = signal_extraction_cache_key("dt1", "ih", "tax", "ph") + b = signal_extraction_cache_key("dt2", "ih", "tax", "ph") + assert a != b + + def test_input_hash_flip_changes_key(self) -> None: + from code_review_helpers import signal_extraction_cache_key + a = signal_extraction_cache_key("dt", "ih1", "tax", "ph") + b = signal_extraction_cache_key("dt", "ih2", "tax", "ph") assert a != b def test_taxonomy_hash_flip_changes_key(self) -> None: from code_review_helpers import signal_extraction_cache_key - a = signal_extraction_cache_key("dt", "tax1", "ph") - b = signal_extraction_cache_key("dt", "tax2", "ph") + a = signal_extraction_cache_key("dt", "ih", "tax1", "ph") + b = signal_extraction_cache_key("dt", "ih", "tax2", "ph") assert a != b def test_prompt_hash_flip_changes_key(self) -> None: from code_review_helpers import signal_extraction_cache_key - a = signal_extraction_cache_key("dt", "tax", "ph1") - b = signal_extraction_cache_key("dt", "tax", "ph2") + a = signal_extraction_cache_key("dt", "ih", "tax", "ph1") + b = signal_extraction_cache_key("dt", "ih", "tax", "ph2") + assert a != b + + def test_key_composition_is_pinned_to_a_literal_digest(self) -> None: + """ISS-8961: the input-hash slot is pinned, not re-derived. + + The expected value is a frozen literal, deliberately duplicating + what the builder computes. A test that re-derived it through the + same helper would stay green if the input-hash component were + dropped back out of the tuple — which is exactly the regression + this pins. + """ + from code_review_helpers import signal_extraction_cache_key + assert signal_extraction_cache_key("dt", "ih", "tax", "ph") == ( + "41cc81cd82aee219c85f3e3b370b678772d6eb1b45c6c3f6c71cd7ab8db961f9" + ) + + +class TestSignalInputHash: + """ISS-8961: the diff-derived component of the signals cache key.""" + + def test_hash_is_pinned_to_a_literal_digest(self) -> None: + """Frozen literal, not a re-derivation of the canonical form.""" + from code_review_helpers import signal_input_hash + bundle = { + "files": [{"path": "a.ts", "status": "modified"}], + "sample_diff_excerpts": [], + "intent": {}, + } + assert signal_input_hash(bundle) == ( + "cf79f0bd063b1da6299767f7f57e8f765b828daad50e7abf4ea5cdb8fa500c75" + ) + + def test_key_order_does_not_change_the_hash(self) -> None: + from code_review_helpers import signal_input_hash + a = signal_input_hash({"files": [], "intent": {}, "sample_diff_excerpts": []}) + b = signal_input_hash({"intent": {}, "sample_diff_excerpts": [], "files": []}) + assert a == b + + def test_a_changed_excerpt_changes_the_hash(self) -> None: + from code_review_helpers import signal_input_hash + base = {"files": [{"path": "a.ts", "status": "modified"}], "intent": {}} + a = signal_input_hash({**base, "sample_diff_excerpts": [{"path": "a.ts", "added_sample": [{"line": "1", "content": "x"}]}]}) + b = signal_input_hash({**base, "sample_diff_excerpts": [{"path": "a.ts", "added_sample": [{"line": "1", "content": "y"}]}]}) assert a != b @@ -12446,8 +12495,6 @@ def test_cache_hit_serves_directly_without_agent(self, tmp_path: Path) -> None: from code_review_helpers import ( CACHE_NAMESPACE_SIGNALS, cmd_extract_signals_prepare, - load_signal_taxonomy, - signal_extraction_cache_key, ) cr_dir = tmp_path / "cr" cache_dir = tmp_path / "cache" @@ -12455,27 +12502,6 @@ def test_cache_hit_serves_directly_without_agent(self, tmp_path: Path) -> None: (cache_dir / CACHE_NAMESPACE_SIGNALS).mkdir(parents=True) diff_path = _build_diff_data(tmp_path) - _, raw = load_signal_taxonomy() - import hashlib - taxonomy_hash = hashlib.sha256(raw).hexdigest() - key = signal_extraction_cache_key("abcdef1234", taxonomy_hash, "ph0") - cached_payload = { - "status": "ok", - "signals": [ - {"name": "language_typescript", "evidence": "x.ts:1 — TS", "confidence": 0.95}, - ], - "errors": [], - "model": "haiku", - "cache_key": key, - "taxonomy_hash": taxonomy_hash, - "prompt_hash": "ph0", - "generated_at": datetime.now(timezone.utc).isoformat(), - "written_at": datetime.now(timezone.utc).isoformat(), - } - (cache_dir / CACHE_NAMESPACE_SIGNALS / f"{key}.json").write_text( - json.dumps(cached_payload), - ) - args = argparse.Namespace( cr_dir=str(cr_dir), diff_data=str(diff_path), @@ -12487,6 +12513,30 @@ def test_cache_hit_serves_directly_without_agent(self, tmp_path: Path) -> None: intent=None, model="haiku", ) + # First pass misses and reports the key this diff resolves to; seed + # the cache under that key rather than re-deriving it here, so the + # fixture cannot silently track a change to the key composition. + assert cmd_extract_signals_prepare(args) == 0 + miss_manifest = json.loads( + (cr_dir / "extract_signals_manifest.json").read_text(), + ) + assert miss_manifest["status"] == "needs_agent" + key = miss_manifest["cache_key"] + (cache_dir / CACHE_NAMESPACE_SIGNALS / f"{key}.json").write_text(json.dumps({ + "status": "ok", + "signals": [ + {"name": "language_typescript", "evidence": "x.ts:1 — TS", "confidence": 0.95}, + ], + "errors": [], + "model": "haiku", + "cache_key": key, + "taxonomy_hash": miss_manifest["taxonomy_hash"], + "prompt_hash": "ph0", + "generated_at": datetime.now(timezone.utc).isoformat(), + "written_at": datetime.now(timezone.utc).isoformat(), + })) + (cr_dir / "extract_signals_input.json").unlink() + rc = cmd_extract_signals_prepare(args) assert rc == 0 @@ -12501,6 +12551,157 @@ def test_cache_hit_serves_directly_without_agent(self, tmp_path: Path) -> None: assert "written_at" not in output +class TestPooledWorktreeSignalCacheIsolation: + """ISS-8961: a second lane in a reused worktree must not inherit lane 1's signals. + + The production shape this reproduces: pooled worktrees give every lane in + a directory the same ``~/.claude/cr-cache-global-repo-`` cache, + and a local branch review always passes ``diff_tip="HEAD"``. Both lanes + below therefore share a cache directory and a diff tip, and differ only in + the diff — which is precisely what the key must discriminate on. + """ + + def _lane_args( + self, cr_dir: Path, cache_dir: Path, diff_path: Path, + ) -> argparse.Namespace: + return argparse.Namespace( + cr_dir=str(cr_dir), + diff_data=str(diff_path), + # A local branch review never resolves this to a SHA. + diff_tip="HEAD", + prompt_hash="ph0", + cache_dir=str(cache_dir), + taxonomy=None, + prompt=None, + intent=None, + model="haiku", + ) + + def _write_diff_data(self, path: Path, file_path: str, added: str) -> Path: + path.write_text(json.dumps({ + "file_statuses": {file_path: "M"}, + "file_loc": {file_path: {"added": 1, "removed": 0}}, + "patch_lines": { + file_path: {"added_lines": {"1": added}, "removed_lines": {}}, + }, + })) + return path + + def test_second_lane_does_not_receive_first_lanes_signals( + self, tmp_path: Path, + ) -> None: + from code_review_helpers import ( + cmd_extract_signals_consolidate, + cmd_extract_signals_prepare, + ) + cache_dir = tmp_path / "cr-cache-global-repo-pool" + cache_dir.mkdir() + + # Lane 1: reviews an auth diff and caches its extraction. + lane1_dir = tmp_path / "lane1" + lane1_dir.mkdir() + lane1_diff = self._write_diff_data( + lane1_dir / "diff_data.json", + "src/auth/login.ts", + "const session = await issueToken(user);", + ) + assert cmd_extract_signals_prepare( + self._lane_args(lane1_dir, cache_dir, lane1_diff), + ) == 0 + lane1_manifest = json.loads( + (lane1_dir / "extract_signals_manifest.json").read_text(), + ) + assert lane1_manifest["status"] == "needs_agent" + agent_out = lane1_dir / "agent_extract_signals.json" + agent_out.write_text(json.dumps({"signals": [ + { + "name": "auth_touching", + "evidence": "src/auth/login.ts:1 — issues a session token", + "confidence": 0.95, + }, + ]})) + assert cmd_extract_signals_consolidate(argparse.Namespace( + cr_dir=str(lane1_dir), + agent_output=str(agent_out), + manifest=None, + taxonomy=None, + cache_dir=str(cache_dir), + )) == 0 + lane1_signals = json.loads( + (lane1_dir / "extract_signals.json").read_text(), + ) + assert [s["name"] for s in lane1_signals["signals"]] == ["auth_touching"] + + # Lane 2: same pooled cache directory, same diff tip, different diff. + lane2_dir = tmp_path / "lane2" + lane2_dir.mkdir() + lane2_diff = self._write_diff_data( + lane2_dir / "diff_data.json", + "src/billing/invoice.py", + "total = sum(line.amount for line in invoice.lines)", + ) + assert cmd_extract_signals_prepare( + self._lane_args(lane2_dir, cache_dir, lane2_diff), + ) == 0 + lane2_manifest = json.loads( + (lane2_dir / "extract_signals_manifest.json").read_text(), + ) + + assert lane2_manifest["cache_key"] != lane1_manifest["cache_key"] + assert lane2_manifest["status"] == "needs_agent" + # The decisive assertion: lane 2 must not be handed lane 1's output. + assert not (lane2_dir / "extract_signals.json").exists() + + def test_unkeyable_input_bypasses_the_cache_in_both_directions( + self, tmp_path: Path, + ) -> None: + """No changed files means no diff identity: never serve, never store.""" + from code_review_helpers import ( + CACHE_NAMESPACE_SIGNALS, + SIGNAL_CACHE_BYPASS_NO_FILES, + cmd_extract_signals_consolidate, + cmd_extract_signals_prepare, + ) + cache_dir = tmp_path / "cr-cache-global-repo-pool" + cache_dir.mkdir() + cr_dir = tmp_path / "lane" + cr_dir.mkdir() + diff_path = cr_dir / "diff_data.json" + diff_path.write_text(json.dumps({ + "file_statuses": {}, "file_loc": {}, "patch_lines": {}, + })) + + assert cmd_extract_signals_prepare( + self._lane_args(cr_dir, cache_dir, diff_path), + ) == 0 + manifest = json.loads((cr_dir / "extract_signals_manifest.json").read_text()) + assert manifest["status"] == "needs_agent" + assert manifest["cache_key"] == "" + assert manifest["cache_bypass_reason"] == SIGNAL_CACHE_BYPASS_NO_FILES + + agent_out = cr_dir / "agent_extract_signals.json" + agent_out.write_text(json.dumps({"signals": [ + { + "name": "language_typescript", + "evidence": "x.ts:1 — TS file", + "confidence": 0.95, + }, + ]})) + assert cmd_extract_signals_consolidate(argparse.Namespace( + cr_dir=str(cr_dir), + agent_output=str(agent_out), + manifest=None, + taxonomy=None, + cache_dir=str(cache_dir), + )) == 0 + assert json.loads( + (cr_dir / "extract_signals.json").read_text(), + )["status"] == "ok" + # Nothing was written into the shared namespace for a later lane to hit. + namespace = cache_dir / CACHE_NAMESPACE_SIGNALS + assert not namespace.exists() or not list(namespace.glob("*.json")) + + class TestExtractSignalsConsolidate: """Validator-driven consolidate behavior: ok path, fail-closed path, finding emission.""" From 6346f8cd3d9da720b3548189666864c7a6df57e1 Mon Sep 17 00:00:00 2001 From: mikeangstadt Date: Sun, 6 Sep 2026 12:08:58 -0500 Subject: [PATCH 2/3] fix(code-review): close review gaps in the signal cache key - Close a false green in the pooled-worktree regression test: it varied the changed PATH as well as the content, so a key hashing only the file list still passed. Add a lane pair holding path, status, and line counts identical and varying only the excerpt -- the shape two reviews of one branch actually take as it is amended. - Delegate signal_input_hash to _stable_json_hash, the module's existing owner of the deterministic cache-key JSON rule, instead of a byte-identical second copy of it. - Pin the bundle's run-to-run stability: a run-varying field added to _build_signal_input would make every review a permanent miss, and the golden harness cannot catch it because it normalizes cache_key. - Emit input_hash in the manifest beside taxonomy_hash and prompt_hash, so the one component that actually varies is visible when diagnosing a key move. Normalized in the golden harness like its siblings. - State the real reason the no-changed-files bundle bypasses the cache. It is not that such a bundle cannot be keyed -- it is the shape a degraded parse-diff takes, and caching it would persist a degraded extraction for the namespace TTL. Name the cost too: a genuinely empty review now re-dispatches its extraction every run. Testing: uv run pytest plugins/ -- 2113 passed, 3 pre-existing failures (override-TTL fixture dated 2026-05-29 vs a 90-day TTL, red on main). ruff check; uv run pyright 0 errors. Counterfactual: restoring origin/main's helper turns 13 tests red, including both pooled-lane tests. Mutation: narrowing the key to the file list alone is now killed by test_same_file_different_content_is_not_served_a_stale_hit. Risks: manifest gains an additive input_hash field; no consumer reads it, and the golden harness normalizes it. --- CHANGELOG.md | 2 +- plugins/code-review/SCHEMA.md | 2 +- .../tools/python/code_review_helpers.py | 43 +++++++---- .../expected/extract_signals_manifest.json | 1 + .../expected/extract_signals_manifest.json | 1 + .../expected/extract_signals_manifest.json | 1 + .../expected/extract_signals_manifest.json | 1 + .../expected/extract_signals_manifest.json | 1 + .../expected/extract_signals_manifest.json | 1 + .../tools/python/prefix_golden_harness.py | 1 + .../tools/python/test_code_review_helpers.py | 74 +++++++++++++++++++ 11 files changed, 110 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6d2baf..fca8546 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### code-review v3.7.1 #### Fixed -- **Signal extraction no longer serves one review's signals to a different review that shares a cache directory (ISS-8961).** The `signals/` namespace key was `(diff_tip, taxonomy_hash, prompt_hash)`, and `diff_tip` is a ref *name* — `resolve-scope` leaves it as the literal `"HEAD"` for every local branch review and sets it to `origin/` for a PR — so no component of the key varied with the diff. The cache directory is `~/.claude/cr-cache-global-repo-`, which is the worktree directory name, so any two reviews run from the same directory shared both the directory and the key: the second review was served the first review's extracted signals and `extract_signals.json` described a diff that was never under review. The key now includes `signal_input_hash` — a SHA-256 over the canonical serialization of the agent input bundle `extract-signals-prepare` builds (file list, per-file added/removed excerpts, intent hint), which is exactly what the extraction agent reads — so a different diff produces a different key even at an identical path, branch, and tip. `extract-signals-prepare` builds that bundle before computing the key rather than only on a miss. When the bundle carries no changed files there is no diff identity to key on, so the run bypasses the cache in both directions: it emits 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 empty-key guard, the write. A miss re-runs a single Haiku extraction; a wrong hit silently misdescribes the change under review. +- **Signal extraction no longer serves one review's signals to a different review that shares a cache directory (ISS-8961).** The `signals/` namespace key was `(diff_tip, taxonomy_hash, prompt_hash)`, and `diff_tip` is a ref *name* — `resolve-scope` leaves it as the literal `"HEAD"` for every local branch review and sets it to `origin/` for a PR — so no component of the key varied with the diff. The cache directory is `~/.claude/cr-cache-global-repo-`, which is the worktree directory name, so any two reviews run from the same directory shared both the directory and the key: the second review was served the first review's extracted signals and `extract_signals.json` described a diff that was never under review. The key now includes `signal_input_hash` — a SHA-256 over the canonical serialization of the agent input bundle `extract-signals-prepare` builds (file list, per-file added/removed excerpts, intent hint), which is exactly what the extraction agent reads — so a different diff produces a different key even at an identical path, branch, and tip. `extract-signals-prepare` builds that bundle before computing the key rather than only on a miss, so the intent hint — an agent input that the old key also omitted — is now covered too. The manifest reports the new `input_hash` alongside `taxonomy_hash` and `prompt_hash`, so an operator can see which component moved a key. A bundle carrying no changed files bypasses the cache in both directions — an empty `cache_key` plus `cache_bypass_reason: "no-changed-files"` suppresses the read and, through `extract-signals-consolidate`'s existing empty-key guard, the write. Not because such a bundle cannot be keyed, but because it is the shape a degraded `parse-diff` takes, and caching it would persist a degraded extraction for the namespace TTL and serve it to every later empty-looking run; the cost is that a genuinely empty review re-dispatches its extraction every time. A miss re-runs a single Haiku extraction; a wrong hit silently misdescribes the change under review. ### code v1.14.10 diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index 3e49503..f00bb54 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -585,7 +585,7 @@ A MAJOR `schema_version` bump invalidates every cache namespace at once. | Namespace | Path | Key inputs | TTL | | ------------------- | ------------------------------------------------- | ------------------------------------------------------------- | ------ | | BHA findings | `/bha/.json` | file_content_hash + prompt_hash + model_id + schema_version | 30 d | -| Signal extraction | `/signals/.json` | diff_tip + agent_input_hash + taxonomy_hash + signal_prompt_hash | 7 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 | | 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 a1e728d..c9e4fcf 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -6241,8 +6241,8 @@ def cmd_detect_injection(args: argparse.Namespace) -> int: SIGNAL_EXTRACTION_MARKER = "signal-extraction-failed" SIGNAL_TAXONOMY_FILENAME = "signal_taxonomy.json" SIGNAL_EXTRACTION_PROMPT_FILENAME = "signal_extraction_prompt.txt" -# Recorded in the manifest when the agent input carries no changed files: -# there is no diff identity to key on, so the run neither reads nor writes +# Recorded in the manifest when the agent input carries no changed files — +# the shape a degraded parse-diff takes, so the run neither reads nor writes # the signals cache (ISS-8961 — a miss is always cheaper than a wrong hit). SIGNAL_CACHE_BYPASS_NO_FILES = "no-changed-files" @@ -6340,10 +6340,14 @@ def signal_input_hash(agent_input: dict[str, Any]) -> str: """Content fingerprint of the exact bundle the extraction agent reads. ``_build_signal_input`` is what the agent consumes — file list, per-file - excerpts, and the intent hint — so hashing its canonical serialization - fingerprints every input the extraction is a function of. Sorted keys - make the serialization order-independent; the builder already emits - files and excerpts in a deterministic order. + excerpts, and the intent hint — so fingerprinting the whole bundle + covers every input the extraction is a function of. The hash MUST be + taken over the entire bundle, not the file list alone: two reviews of + the same branch touch the same paths with the same line counts and + differ only in excerpt content. Canonicalization is delegated to + ``_stable_json_hash``, the module's single owner of the deterministic + cache-key JSON rule (the builder already emits files and excerpts in a + deterministic order, and sorted keys make the rest order-independent). ISS-8961: this is the component that makes the key diff-derived. The ref-name component (``diff_tip``) is ``"HEAD"`` for every local branch @@ -6351,10 +6355,7 @@ def signal_input_hash(agent_input: dict[str, Any]) -> str: review that shared a cache directory — and a pooled worktree makes the cache directory shared by construction. """ - canonical = json.dumps( - agent_input, sort_keys=True, separators=(",", ":"), ensure_ascii=True, - ) - return hashlib.sha256(canonical.encode("utf-8", "replace")).hexdigest() + return _stable_json_hash(agent_input) def signal_extraction_cache_key( @@ -6582,11 +6583,18 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: a single Haiku agent. ISS-8961: the bundle is built *before* the key so the key is derived - from the diff the agent will actually read. A degenerate bundle (no - changed files) has no diff identity to key on, so the cache is bypassed - in both directions — miss now, no cache write later — rather than - keyed on the components that remain, which are identical across every - review sharing a cache directory. + from the diff the agent will actually read. + + A bundle with no changed files bypasses the cache in both directions — + miss now, no cache write later. Not because such a bundle is unkeyable + (it hashes fine, and two genuinely-empty reviews would legitimately + share an entry) but because it is the shape a *degraded* run takes: a + parse-diff that emitted empty ``file_statuses`` for a review that did + have changes is indistinguishable here from a real empty diff, and + caching it would persist that degraded extraction for the namespace TTL + and serve it to every later empty-looking run. The cost is that a real + zero-file review re-dispatches its Haiku extraction every time; a miss + is cheap and bounded, a wrong hit is neither. Always exits 0; structural failures (no diff_data, malformed taxonomy) print to stderr and return 1. @@ -6652,9 +6660,10 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: agent_input = _build_signal_input(diff_data, intent_summary) keyable = bool(agent_input.get("files")) + input_hash = signal_input_hash(agent_input) key = ( signal_extraction_cache_key( - diff_tip, signal_input_hash(agent_input), taxonomy_hash, prompt_hash, + diff_tip, input_hash, taxonomy_hash, prompt_hash, ) if keyable else "" @@ -6676,6 +6685,7 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: return _write_and_emit_manifest(manifest_path, { "status": "cache_hit", "cache_key": key, + "input_hash": input_hash, "taxonomy_hash": taxonomy_hash, "prompt_hash": prompt_hash, "output_path": str(output_path), @@ -6694,6 +6704,7 @@ def cmd_extract_signals_prepare(args: argparse.Namespace) -> int: manifest: dict[str, Any] = { "status": "needs_agent", "cache_key": key, + "input_hash": input_hash, "taxonomy_hash": taxonomy_hash, "prompt_hash": prompt_hash, "input_path": str(input_path), diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_cache_hit/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_cache_hit/expected/extract_signals_manifest.json index 1535b27..05cd6e7 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_cache_hit/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_cache_hit/expected/extract_signals_manifest.json @@ -1,5 +1,6 @@ { "cache_key": "", + "input_hash": "", "input_path": "/extract_signals_input.json", "model": "haiku", "output_path": "/extract_signals.json", diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/extract_signals_manifest.json index 1535b27..05cd6e7 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_coverage_critic/expected/extract_signals_manifest.json @@ -1,5 +1,6 @@ { "cache_key": "", + "input_hash": "", "input_path": "/extract_signals_input.json", "model": "haiku", "output_path": "/extract_signals.json", diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json index 03ce9d0..a2130da 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_empty_diff/expected/extract_signals_manifest.json @@ -1,6 +1,7 @@ { "cache_bypass_reason": "no-changed-files", "cache_key": "", + "input_hash": "", "input_path": "/extract_signals_input.json", "model": "haiku", "output_path": "/extract_signals.json", diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_fast_path/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_fast_path/expected/extract_signals_manifest.json index 1535b27..05cd6e7 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_fast_path/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_fast_path/expected/extract_signals_manifest.json @@ -1,5 +1,6 @@ { "cache_key": "", + "input_hash": "", "input_path": "/extract_signals_input.json", "model": "haiku", "output_path": "/extract_signals.json", diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_since_last_review/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_since_last_review/expected/extract_signals_manifest.json index 1535b27..05cd6e7 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_since_last_review/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_since_last_review/expected/extract_signals_manifest.json @@ -1,5 +1,6 @@ { "cache_key": "", + "input_hash": "", "input_path": "/extract_signals_input.json", "model": "haiku", "output_path": "/extract_signals.json", diff --git a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_standard/expected/extract_signals_manifest.json b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_standard/expected/extract_signals_manifest.json index 1535b27..05cd6e7 100644 --- a/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_standard/expected/extract_signals_manifest.json +++ b/plugins/code-review/tools/python/prefix_fixtures/golden_prefix_standard/expected/extract_signals_manifest.json @@ -1,5 +1,6 @@ { "cache_key": "", + "input_hash": "", "input_path": "/extract_signals_input.json", "model": "haiku", "output_path": "/extract_signals.json", diff --git a/plugins/code-review/tools/python/prefix_golden_harness.py b/plugins/code-review/tools/python/prefix_golden_harness.py index 59babc3..1f53fb6 100644 --- a/plugins/code-review/tools/python/prefix_golden_harness.py +++ b/plugins/code-review/tools/python/prefix_golden_harness.py @@ -674,6 +674,7 @@ def walk_prefix( "review_id": "", "prompt_hash": "", "cache_key": "", + "input_hash": "", "taxonomy_hash": "", # Cache-keying hashes computed over extract_signals.json, whose generated_at # is wall-clock — so these vary run-to-run. The underlying .initial plan and 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 70efc75..9edc191 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -12290,6 +12290,36 @@ def test_key_order_does_not_change_the_hash(self) -> None: b = signal_input_hash({"intent": {}, "sample_diff_excerpts": [], "files": []}) assert a == b + def test_bundle_from_identical_diff_data_is_stable_across_builds(self) -> None: + """ISS-8961: a run-varying field in the bundle is a permanent miss. + + The key is only useful if an unchanged diff rebuilds a byte-identical + bundle. If someone later adds a timestamp, absolute path, or run id + to ``_build_signal_input``, every review misses forever and re-runs + the Haiku extraction — a silent cost regression the golden harness + cannot catch, because it normalizes ``cache_key`` to a placeholder. + """ + from code_review_helpers import _build_signal_input, signal_input_hash + diff_data = { + "file_statuses": {"b.ts": "M", "a.ts": "A"}, + "file_loc": {"a.ts": {"added": 1, "removed": 0}, "b.ts": {"added": 1, "removed": 1}}, + "patch_lines": { + "a.ts": {"added_lines": {"2": "x", "10": "y"}, "removed_lines": {}}, + "b.ts": {"added_lines": {"1": "z"}, "removed_lines": {"4": "w"}}, + }, + } + intent = {"intent": "fix"} + first = signal_input_hash(_build_signal_input(diff_data, intent)) + second = signal_input_hash(_build_signal_input(diff_data, intent)) + assert first == second + # Key order in the source data must not move the digest either. + reordered = { + "patch_lines": diff_data["patch_lines"], + "file_loc": diff_data["file_loc"], + "file_statuses": {"a.ts": "A", "b.ts": "M"}, + } + assert signal_input_hash(_build_signal_input(reordered, intent)) == first + def test_a_changed_excerpt_changes_the_hash(self) -> None: from code_review_helpers import signal_input_hash base = {"files": [{"path": "a.ts", "status": "modified"}], "intent": {}} @@ -12652,6 +12682,50 @@ def test_second_lane_does_not_receive_first_lanes_signals( # The decisive assertion: lane 2 must not be handed lane 1's output. assert not (lane2_dir / "extract_signals.json").exists() + def test_same_file_different_content_is_not_served_a_stale_hit( + self, tmp_path: Path, + ) -> None: + """The realistic pooled-lane shape: only the excerpt content differs. + + The sibling test above varies the changed path too, so a key built + from the file list alone would still separate those two lanes. This + pair holds path, status, and line counts identical and varies only + the added line — which is what successive reviews of one branch look + like as it is amended. It fails unless the key covers the whole + agent-input bundle, excerpts included. + """ + from code_review_helpers import cmd_extract_signals_prepare + cache_dir = tmp_path / "cr-cache-global-repo-pool" + cache_dir.mkdir() + + keys: list[str] = [] + for lane, added in ( + ("lane1", "const s = await issueToken(user);"), + ("lane2", "const s = await issueToken(user, { ttl: 60 });"), + ): + lane_dir = tmp_path / lane + lane_dir.mkdir() + diff = self._write_diff_data( + lane_dir / "diff_data.json", "src/auth/login.ts", added, + ) + assert cmd_extract_signals_prepare( + self._lane_args(lane_dir, cache_dir, diff), + ) == 0 + manifest = json.loads( + (lane_dir / "extract_signals_manifest.json").read_text(), + ) + # Same path, same status, same 1-added/0-removed counts. + bundle = json.loads( + (lane_dir / "extract_signals_input.json").read_text(), + ) + assert [ + (f["path"], f["status"], f["lines_added"], f["lines_removed"]) + for f in bundle["files"] + ] == [("src/auth/login.ts", "M", 1, 0)] + keys.append(manifest["cache_key"]) + + assert keys[0] != keys[1] + def test_unkeyable_input_bypasses_the_cache_in_both_directions( self, tmp_path: Path, ) -> None: From 73d830e234265b315be6642596de12c1d4b7ccd0 Mon Sep 17 00:00:00 2001 From: mikeangstadt Date: Sun, 6 Sep 2026 12:17:36 -0500 Subject: [PATCH 3/3] fix(code-review): pin the clock in override-cache tests Mechanics repair, not an expectation change. The behaviour under test is unchanged and still asserted: a valid, within-TTL override short-circuits verification, routes through consolidate as RE_ASSERTED, and lands in the by-reviewer stats. No assertion was weakened, widened, skipped, or deleted. 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. The fixture was therefore a time bomb: it went red on 2026-08-27 with no code change, and the suite has been red on main ever since (main last ran CI 2026-08-17, green). A test that fails on a calendar date rather than a behaviour change was never pinning what it claimed to. Both sides of the comparison are now literals -- the fixture timestamp and the clock -- so the gap stays a constant 3 days forever. The TTL gate itself is untouched, and the tests whose subject IS the TTL keep offsetting from the real clock on purpose. Testing: uv run pytest plugins/ -- 2116 passed, 0 failed (first green run since 2026-08-27). Mutation: forcing _override_is_expired to return True turns all three repaired tests red, so the pin did not neuter the gate they depend on. ruff check; uv run pyright 0 errors. Risks: none identified. Test-only change. --- .../tools/python/test_code_review_helpers.py | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) 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 9edc191..74f7c58 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -9986,6 +9986,34 @@ def test_cache_hit_skips_spawn_and_materializes_verdict( assert materialized["verifier_verdict"] == "CONFIRMED" +# Fixture instant for override tests whose subject is *not* the TTL gate. +# The override namespace has a 90-day TTL enforced sweep-on-read against the +# wall clock, so a bare literal here silently becomes an expired override once +# the calendar passes it — which is exactly what happened: these fixtures were +# written on 2026-05-29 and the suite went red on 2026-08-27 with no code +# change. Pinning BOTH the assertion timestamp and the clock keeps the gap a +# constant 3 days, so these tests exercise the override-honoring path forever. +# Tests whose subject IS the TTL gate deliberately do the opposite and offset +# from the real clock (see test_override_dropped_when_ttl_exceeded). +_OVERRIDE_FIXTURE_ASSERTED_AT = "2026-05-29T22:00:00+00:00" +_OVERRIDE_FIXTURE_NOW = datetime(2026, 6, 1, 22, 0, 0, tzinfo=timezone.utc) + + +@pytest.fixture +def pinned_override_clock(monkeypatch: pytest.MonkeyPatch) -> None: + """Freeze the helper module's clock inside the overrides TTL window.""" + import code_review_helpers + + class _PinnedClock(datetime): + @classmethod + def now(cls, tz: Any = None) -> datetime: # type: ignore[override] + if tz is None: + return _OVERRIDE_FIXTURE_NOW.replace(tzinfo=None) + return _OVERRIDE_FIXTURE_NOW.astimezone(tz) + + monkeypatch.setattr(code_review_helpers, "datetime", _PinnedClock) + + class TestOverrideCache: """PLN-773 Phase 3 — overrides/ cache namespace + content-hash invalidation.""" @@ -10068,7 +10096,7 @@ def test_write_override_roundtrip(self, tmp_path: Path) -> None: "override": "RE_ASSERT", "reason": "operator says fine", "verified_against": "REJECTED", - "asserted_at": "2026-05-29T22:00:00+00:00", + "asserted_at": _OVERRIDE_FIXTURE_ASSERTED_AT, "asserted_by": "kris.wong@closedloop.ai", } path = _write_override(cache, payload) @@ -10117,7 +10145,7 @@ def test_override_invalid_on_hash_drift(self, tmp_path: Path) -> None: assert _override_is_valid(override, finding, cr) is False def test_verify_prepare_short_circuits_on_valid_override( - self, tmp_path: Path, + self, tmp_path: Path, pinned_override_clock: None, ) -> None: from code_review_helpers import _file_content_hash, _write_override cr = self._cr_dir(tmp_path) @@ -10133,7 +10161,7 @@ def test_verify_prepare_short_circuits_on_valid_override( "finding_id": "bha_p0_f0", "file_content_hash": _file_content_hash(cr, "src/x.py", 3), "override": "RE_ASSERT", - "asserted_at": "2026-05-29T22:00:00+00:00", + "asserted_at": _OVERRIDE_FIXTURE_ASSERTED_AT, }) # PR #114 review fix — delegate to the shared helper with an # explicit cr_dir override so the per-test stdout/Namespace dance @@ -11528,7 +11556,7 @@ def _write_target_file(repo_root: Path, rel: str, content: str) -> None: full.write_text(content) def test_prepare_then_consolidate_routes_override_to_verified( - self, tmp_path: Path, + self, tmp_path: Path, pinned_override_clock: None, ) -> None: """End-to-end: override → RE_ASSERTED in verified[]. @@ -11555,7 +11583,7 @@ def test_prepare_then_consolidate_routes_override_to_verified( "finding_id": "bha_p0_f0", "file_content_hash": _file_content_hash(cr, "src/x.py", 3), "override": "RE_ASSERT", - "asserted_at": "2026-05-29T22:00:00+00:00", + "asserted_at": _OVERRIDE_FIXTURE_ASSERTED_AT, }) # Phase 1 — prepare. Should record the fid in override_hits and @@ -11583,7 +11611,7 @@ def test_prepare_then_consolidate_routes_override_to_verified( assert envelope["pending_verification"] == [] def test_prepare_then_consolidate_writes_re_asserted_to_stats( - self, tmp_path: Path, + self, tmp_path: Path, pinned_override_clock: None, ) -> None: """The per-reviewer ``re_asserted`` counter — the whole point of the PR — must count this finding.""" @@ -11604,7 +11632,7 @@ def test_prepare_then_consolidate_writes_re_asserted_to_stats( "finding_id": "bha_p0_f0", "file_content_hash": _file_content_hash(cr, "src/x.py", 3), "override": "RE_ASSERT", - "asserted_at": "2026-05-29T22:00:00+00:00", + "asserted_at": _OVERRIDE_FIXTURE_ASSERTED_AT, }) _, manifest = _run_verify_prepare(