From 76d688289e299428d541ab1555730a5b0594329a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 14:34:21 +0000 Subject: [PATCH 01/10] chore(metrics): record plan-approval audit entry for #2166+#2171 batch Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .claude/metrics/config-changelog.jsonl | 1 + 1 file changed, 1 insertion(+) diff --git a/.claude/metrics/config-changelog.jsonl b/.claude/metrics/config-changelog.jsonl index 5199a8c98..f4b97a2f1 100644 --- a/.claude/metrics/config-changelog.jsonl +++ b/.claude/metrics/config-changelog.jsonl @@ -84,3 +84,4 @@ {"timestamp": "2026-09-17T15:36:16.556298+00:00", "type": "approval", "proposed": "acceptance-criteria set: plans/measure-rereview-duplication.md (issue #2165)", "evidence_shown": "plans/measure-rereview-duplication.md", "risks_surfaced": ["AC6: cross-reference to step 1.4 checkpoint list not inlined", "AC8: additive-alias pattern not defined in AC text"]} {"timestamp": "2026-09-21T18:49:13Z", "type": "approval", "proposed": "Approve plan for Agent lifecycle improvements (#2172 batch: #2187-#2190)", "evidence_shown": "plans/2172-agent-lifecycle-improvements.md", "risks_surfaced": ["Strategic critic recommended splitting into up to 3 PRs; acknowledged, not adopted (user pre-decided single-batch)", "Slice 2 malformed-hand-back scenario is provisional pending Step 2.1a transcript-shape confirmation", "Slice 4 JS/TS fast-check test may need network-exempt fallback if vendoring proves impractical"], "description": "Auto-approved (non-interactive) - no usable TTY in this remote session"} {"timestamp": "2026-09-21T18:51:16Z", "type": "approval", "proposed": "Acceptance-criteria gate for plans/2172-agent-lifecycle-improvements.md", "evidence_shown": "plans/2172-agent-lifecycle-improvements.md", "risks_surfaced": ["Step 1.4 latency check has no quantitative SLA threshold", "Step 1.2 missing idempotent-double-fire test case", "Step 2.1a conditional AC pass/fail ambiguity if no hand-back signal found", "Step 2.1b missing invalid-JSON (not just unreadable) transcript test", "Step 3.1 claim-extraction heuristics under-specified with only 2 of 5 pinned", "Step 3.2 missing malformed-but-parseable WebFetch response case", "Step 3.3 doc-review integration test is structural-only, not behavioral", "Step 4.1 property-derivation edge cases (cross-module pair, ambiguous invariant) untested", "Step 4.2 missing language-specific runtime-failure cases (Hypothesis/fast-check install failure)", "Step 4.3 vendoring-impractical threshold undefined"], "description": "Acceptance-criteria gate auto-passed with 10 flagged (0 blocker, 2 warning, 8 suggestion/minor) criterion findings (non-interactive) - no human gate. Trigger: --yes flag. Findings will be resolved as implementation-time decisions during Step 4, per the plan own assumptions-recording convention."} +{"ts": "2026-09-22T14:33:36Z", "type": "approval", "proposed": "Approve plan for #2166+#2171 verdict-ledger writer batch (part of #2164)", "evidence_shown": "plans/2164-verdict-ledger-writer.md", "risks_surfaced": ["Bash write-shape detection is a heuristic, not a sandbox", "override-audit.jsonl/config-changelog.jsonl remain unguarded (Decision 5 scope-narrowing)", "Step 2.1 marker/Step 2.3 parser format drift risk", "No consumer exists yet for review-verdicts.jsonl until #2167", "plan-review-strategic suggested splitting into two PRs (not adopted)", "Decision 4a: verdict trust boundary relies on the orchestrating session's own declared scope marker, not independently re-verified"], "description": "Auto-approved (non-interactive, --yes via /ship)"} From 0a10340a1aafb9cbdd62c4b4dc3cb086f9c62ce1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 14:45:18 +0000 Subject: [PATCH 02/10] feat(hooks): block direct Write/Edit writes to the boundary-events ledger Part of #2171, part of #2164. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .claude/metrics/config-changelog.jsonl | 1 + .../hooks/boundary_events_write_guard.py | 141 +++++++++++++ .../hooks/test_boundary_events_write_guard.py | 186 ++++++++++++++++++ .../hooks/test_boundary_events_write_guard.py | 169 ++++++++++++++++ 4 files changed, 497 insertions(+) create mode 100755 plugins/dev-team/hooks/boundary_events_write_guard.py create mode 100644 plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py create mode 100644 tests/hooks/test_boundary_events_write_guard.py diff --git a/.claude/metrics/config-changelog.jsonl b/.claude/metrics/config-changelog.jsonl index f4b97a2f1..6839e4b4d 100644 --- a/.claude/metrics/config-changelog.jsonl +++ b/.claude/metrics/config-changelog.jsonl @@ -85,3 +85,4 @@ {"timestamp": "2026-09-21T18:49:13Z", "type": "approval", "proposed": "Approve plan for Agent lifecycle improvements (#2172 batch: #2187-#2190)", "evidence_shown": "plans/2172-agent-lifecycle-improvements.md", "risks_surfaced": ["Strategic critic recommended splitting into up to 3 PRs; acknowledged, not adopted (user pre-decided single-batch)", "Slice 2 malformed-hand-back scenario is provisional pending Step 2.1a transcript-shape confirmation", "Slice 4 JS/TS fast-check test may need network-exempt fallback if vendoring proves impractical"], "description": "Auto-approved (non-interactive) - no usable TTY in this remote session"} {"timestamp": "2026-09-21T18:51:16Z", "type": "approval", "proposed": "Acceptance-criteria gate for plans/2172-agent-lifecycle-improvements.md", "evidence_shown": "plans/2172-agent-lifecycle-improvements.md", "risks_surfaced": ["Step 1.4 latency check has no quantitative SLA threshold", "Step 1.2 missing idempotent-double-fire test case", "Step 2.1a conditional AC pass/fail ambiguity if no hand-back signal found", "Step 2.1b missing invalid-JSON (not just unreadable) transcript test", "Step 3.1 claim-extraction heuristics under-specified with only 2 of 5 pinned", "Step 3.2 missing malformed-but-parseable WebFetch response case", "Step 3.3 doc-review integration test is structural-only, not behavioral", "Step 4.1 property-derivation edge cases (cross-module pair, ambiguous invariant) untested", "Step 4.2 missing language-specific runtime-failure cases (Hypothesis/fast-check install failure)", "Step 4.3 vendoring-impractical threshold undefined"], "description": "Acceptance-criteria gate auto-passed with 10 flagged (0 blocker, 2 warning, 8 suggestion/minor) criterion findings (non-interactive) - no human gate. Trigger: --yes flag. Findings will be resolved as implementation-time decisions during Step 4, per the plan own assumptions-recording convention."} {"ts": "2026-09-22T14:33:36Z", "type": "approval", "proposed": "Approve plan for #2166+#2171 verdict-ledger writer batch (part of #2164)", "evidence_shown": "plans/2164-verdict-ledger-writer.md", "risks_surfaced": ["Bash write-shape detection is a heuristic, not a sandbox", "override-audit.jsonl/config-changelog.jsonl remain unguarded (Decision 5 scope-narrowing)", "Step 2.1 marker/Step 2.3 parser format drift risk", "No consumer exists yet for review-verdicts.jsonl until #2167", "plan-review-strategic suggested splitting into two PRs (not adopted)", "Decision 4a: verdict trust boundary relies on the orchestrating session's own declared scope marker, not independently re-verified"], "description": "Auto-approved (non-interactive, --yes via /ship)"} +{"ts": "2026-09-22T14:37:03Z", "type": "approval", "proposed": "Acceptance criteria set for #2166+#2171 verdict-ledger batch (part of #2164)", "evidence_shown": "plans/2164-verdict-ledger-writer.md", "risks_surfaced": [], "description": "spec-compliance-review criteria-verification pass: all 10 criteria PASS, no flags"} diff --git a/plugins/dev-team/hooks/boundary_events_write_guard.py b/plugins/dev-team/hooks/boundary_events_write_guard.py new file mode 100755 index 000000000..e8c1333d1 --- /dev/null +++ b/plugins/dev-team/hooks/boundary_events_write_guard.py @@ -0,0 +1,141 @@ +#!/usr/bin/env python3 +"""boundary_events_write_guard.py — Claude Code PreToolUse hook (#2171). + +`.claude/metrics/boundary-events.jsonl` is the plugin's boundary-level +accountability ledger (`hooks/lib/boundary_events.py`, #859) — every guard +hook's block/warn/bypass decision is recorded there, and that module's own +docstring states rows must never carry free text (command text, prompt +text, file paths, reasons), only rule IDs from closed vocabularies. A +direct `Write` or `Edit` tool call targeting that file bypasses +`emit_boundary_event()` entirely and could forge or corrupt that record +from inside the session — this hook raises the cost of that forgery the +same way `pre_tool_guard.py` raises it for other sensitive paths, by +blocking a Write/Edit whose `file_path`/`path` resolves to the ledger. + +Step 1.1 of plans/2164-verdict-ledger-writer.md — Write/Edit only. Bash +write-shaped command detection and `settings.json` registration are a +separate, later step (Step 1.2) and are NOT part of this module yet. + +Contract (docs/python-hook-contract.md): + Input : PreToolUse JSON on stdin — `tool_input.file_path` or + `tool_input.path`, resolved relative to `cwd` when not absolute + Output: block message on stdout naming `emit_boundary_event()` + (`hooks/lib/boundary_events.py`) as the remedy + Exit : 2 to block, 0 to allow. Fail-open on any exception (malformed + payload, unreadable cwd, unresolvable repo root) — never raises. + +Path matching is lexical only (`os.path.abspath` — `.`/`..` collapsing, no +symlink following): a symlink-escape sandbox is explicitly out of scope +(plan Step 1.1 TEST note) — this guard raises the cost of forgery, it does +not sandbox the filesystem. + +Stdlib-only. See ADR 0014. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +_HOOK_DIR = Path(__file__).resolve().parent +_LIB_DIR = _HOOK_DIR / "lib" +if str(_LIB_DIR) not in sys.path: + sys.path.insert(0, str(_LIB_DIR)) + +import artifact_paths +from boundary_events import emit_boundary_event as _emit_boundary_event +from stdin_json import read_stdin_json # type: ignore[import-not-found] + +_LEDGER_NAME = "boundary-events.jsonl" + + +def emit_boundary_event(*args, **kwargs) -> None: + """Local safety net (#859): even a misbehaving helper must never affect + this hook's exit code, stdout, or stderr.""" + try: + _emit_boundary_event(*args, **kwargs) + except Exception: # noqa: BLE001, S110 - fail-open by design + pass + + +def _extract_file_path(tool_input: object) -> str: + """Return `tool_input.file_path`, `tool_input.path`, or empty string — + mirrors `pre_tool_guard._extract_file_path`'s field-preference order.""" + if not isinstance(tool_input, dict): + return "" + file_path = tool_input.get("file_path") + if isinstance(file_path, str) and file_path: + return file_path + other = tool_input.get("path") + if isinstance(other, str) and other: + return other + return "" + + +def targets_ledger(file_path: str, cwd: str) -> bool: + """True when `file_path` (joined against `cwd` if not absolute) names + the same on-disk path `emit_boundary_event()` itself resolves and + writes to — `artifact_paths.resolve_file("metrics", ...)` under the + repo root, not a bare `.claude/metrics/` prefix match (so + `review-verdicts.jsonl`, Slice 2's own store, is unaffected).""" + if not file_path: + return False + base = Path(cwd) if cwd else Path.cwd() + candidate = Path(file_path) + if not candidate.is_absolute(): + candidate = base / candidate + candidate_norm = os.path.abspath(str(candidate)) + + ledger = artifact_paths.resolve_file( + "metrics", _LEDGER_NAME, root=cwd, migrate=False + ) + ledger_norm = os.path.abspath(str(ledger)) + + return candidate_norm == ledger_norm + + +def main() -> int: + try: + payload = read_stdin_json() + if payload is None: + return 0 + + file_path = _extract_file_path(payload.get("tool_input")) + if not file_path: + return 0 + + raw_cwd = payload.get("cwd") + cwd = ( + raw_cwd + if isinstance(raw_cwd, str) and raw_cwd and "\0" not in raw_cwd + else "." + ) + + if not targets_ledger(file_path, cwd): + return 0 + + raw_tool = payload.get("tool_name") + tool = raw_tool if isinstance(raw_tool, str) and raw_tool else "Write" + raw_session_id = payload.get("session_id") + session_id = raw_session_id if isinstance(raw_session_id, str) else None + + emit_boundary_event( + cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id + ) + print(f"BLOCKED: Direct write to '{file_path}' is not allowed.") + print( + "This file is the boundary-events accountability ledger (#859) — " + "it is append-only from the session's perspective." + ) + print( + "Use emit_boundary_event() in plugins/dev-team/hooks/lib/boundary_events.py " + "instead of writing to it directly." + ) + return 2 + except Exception: # noqa: BLE001 - fail-open by design, see module docstring + return 0 + + +if __name__ == "__main__": # pragma: no cover + sys.exit(main()) diff --git a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py new file mode 100644 index 000000000..2288265ed --- /dev/null +++ b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py @@ -0,0 +1,186 @@ +"""Unit tests for hooks/boundary_events_write_guard.py (#2171, plan Step +1.1 — Write/Edit path-match guard only; Bash write-shaped command detection +and settings.json registration are Step 1.2, not covered here). + +In-process, stdin-monkeypatched, with `emit_boundary_event` stubbed (same +split `test_destructive_guard.py` uses) — real subprocess + real-ledger +end-to-end coverage lives in `tests/hooks/test_boundary_events_write_guard.py`. +""" + +from __future__ import annotations + +import json +import sys + +import pytest + +from _repo_root import REPO_ROOT as _REPO_ROOT + +_PLUGIN_DIR = _REPO_ROOT / "plugins" / "dev-team" + +for _p in (_PLUGIN_DIR / "hooks",): + if str(_p) not in sys.path: + sys.path.insert(0, str(_p)) + +import boundary_events_write_guard as guard + + +@pytest.fixture(autouse=True) +def _no_boundary_events(monkeypatch): + """Unit-level tests call `main()`/`targets_ledger()` in-process — stub + the emit so no real ledger write happens as a side effect of testing + the guard itself. `tests/hooks/test_boundary_events_write_guard.py` + exercises the real subprocess + real emit path (matches + pre_tool_guard.py's own test split).""" + monkeypatch.setattr(guard, "emit_boundary_event", lambda *a, **k: None) + + +# --------------------------------------------------------------------------- +# _extract_file_path +# --------------------------------------------------------------------------- + + +def test_extract_file_path_prefers_file_path(): + assert guard._extract_file_path({"file_path": "a.py", "path": "b.py"}) == "a.py" + + +def test_extract_file_path_falls_back_to_path(): + assert guard._extract_file_path({"path": "b.py"}) == "b.py" + + +def test_extract_file_path_empty_when_neither(): + assert guard._extract_file_path({}) == "" + + +def test_extract_file_path_empty_when_not_a_dict(): + assert guard._extract_file_path("a.py") == "" + + +# --------------------------------------------------------------------------- +# targets_ledger — path-form matrix (relative, absolute, "./"-prefixed) +# --------------------------------------------------------------------------- + + +def test_targets_ledger_relative_path(tmp_path): + assert guard.targets_ledger( + ".claude/metrics/boundary-events.jsonl", str(tmp_path) + ) + + +def test_targets_ledger_dot_slash_prefixed_path(tmp_path): + assert guard.targets_ledger( + "./.claude/metrics/boundary-events.jsonl", str(tmp_path) + ) + + +def test_targets_ledger_absolute_path(tmp_path): + absolute = str(tmp_path / ".claude" / "metrics" / "boundary-events.jsonl") + assert guard.targets_ledger(absolute, str(tmp_path)) + + +def test_targets_ledger_false_for_unrelated_file_under_metrics(tmp_path): + assert not guard.targets_ledger( + ".claude/metrics/session-digest.jsonl", str(tmp_path) + ) + + +def test_targets_ledger_false_for_review_verdicts_store(tmp_path): + """Slice 2's own store — matches the exact ledger name/path, not a + `.claude/metrics/` prefix (plan Step 1.1 implementation note).""" + assert not guard.targets_ledger( + ".claude/metrics/review-verdicts.jsonl", str(tmp_path) + ) + + +def test_targets_ledger_false_for_empty_path(tmp_path): + assert not guard.targets_ledger("", str(tmp_path)) + + +# --------------------------------------------------------------------------- +# main() — in-process, stdin-monkeypatched (Write and Edit tool shapes) +# --------------------------------------------------------------------------- + + +def _stdin(monkeypatch, payload): + import io + + monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(payload))) + + +def test_main_blocks_write_to_ledger(monkeypatch, tmp_path, capsys): + _stdin( + monkeypatch, + { + "tool_name": "Write", + "tool_input": {"file_path": ".claude/metrics/boundary-events.jsonl"}, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 2 + out = capsys.readouterr().out + assert "emit_boundary_event()" in out + assert "BLOCKED" in out + + +def test_main_blocks_edit_to_ledger(monkeypatch, tmp_path, capsys): + _stdin( + monkeypatch, + { + "tool_name": "Edit", + "tool_input": {"file_path": ".claude/metrics/boundary-events.jsonl"}, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 2 + out = capsys.readouterr().out + assert "emit_boundary_event()" in out + + +def test_main_allows_write_to_unrelated_file(monkeypatch, tmp_path, capsys): + _stdin( + monkeypatch, + { + "tool_name": "Write", + "tool_input": {"file_path": ".claude/metrics/session-digest.jsonl"}, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 0 + assert capsys.readouterr().out == "" + + +def test_main_allows_write_to_review_verdicts_store(monkeypatch, tmp_path, capsys): + _stdin( + monkeypatch, + { + "tool_name": "Write", + "tool_input": {"file_path": ".claude/metrics/review-verdicts.jsonl"}, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 0 + assert capsys.readouterr().out == "" + + +def test_main_fails_open_on_missing_tool_input(monkeypatch, tmp_path): + _stdin(monkeypatch, {"tool_name": "Write", "cwd": str(tmp_path)}) + assert guard.main() == 0 + + +def test_main_fails_open_on_non_json_stdin(monkeypatch): + import io + + monkeypatch.setattr(sys, "stdin", io.StringIO("not-json")) + assert guard.main() == 0 + + +def test_main_fails_open_on_empty_stdin(monkeypatch): + import io + + monkeypatch.setattr(sys, "stdin", io.StringIO("")) + assert guard.main() == 0 + + +def test_main_silent_pass_when_file_path_absent(monkeypatch, tmp_path): + _stdin(monkeypatch, {"tool_name": "Write", "tool_input": {}, "cwd": str(tmp_path)}) + assert guard.main() == 0 diff --git a/tests/hooks/test_boundary_events_write_guard.py b/tests/hooks/test_boundary_events_write_guard.py new file mode 100644 index 000000000..3d7a71392 --- /dev/null +++ b/tests/hooks/test_boundary_events_write_guard.py @@ -0,0 +1,169 @@ +"""End-to-end tests for hooks/boundary_events_write_guard.py (#2171, plan +Step 1.1 — Write/Edit path-match guard only; Bash write-shaped command +detection and settings.json registration are Step 1.2, not covered here). + +Drives main() via subprocess with representative PreToolUse JSON payloads, +using the real `emit_boundary_event()` write path (mirrors +tests/hooks/test_agent_dispatch_ledger.py) — confirms the guard both blocks +AND allows for real, and that a block leaves its own boundary-events trace +(plan Step 1.1 IMPLEMENT note: "every sibling guard ... records its own +decision"). Unit-level coverage of the guard's internals lives in +plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py (matches +tests/hooks/test_destructive_guard.py's own e2e-vs-unit split). +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +from _repo_root import REPO_ROOT as _REPO_ROOT + +_HOOK_PY = _REPO_ROOT / "plugins" / "dev-team" / "hooks" / "boundary_events_write_guard.py" + + +def _run(payload: dict) -> subprocess.CompletedProcess: + env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "LANG": "C.UTF-8"} + return subprocess.run( + [sys.executable, str(_HOOK_PY)], + input=json.dumps(payload).encode(), + env=env, + capture_output=True, + timeout=10, + check=False, + ) + + +def _read_jsonl(path: Path) -> list: + if not path.is_file(): + return [] + lines = [ln for ln in path.read_text(encoding="utf-8").splitlines() if ln.strip()] + return [json.loads(ln) for ln in lines] + + +def test_write_to_ledger_is_blocked_and_records_its_own_event(tmp_path: Path) -> None: + ledger = tmp_path / ".claude" / "metrics" / "boundary-events.jsonl" + + result = _run( + { + "tool_name": "Write", + "tool_input": { + "file_path": ".claude/metrics/boundary-events.jsonl", + "content": '{"forged": true}\n', + }, + "cwd": str(tmp_path), + "session_id": "sess-1", + } + ) + + assert result.returncode == 2 + assert b"BLOCKED" in result.stdout + assert b"emit_boundary_event()" in result.stdout + + events = _read_jsonl(ledger) + assert len(events) == 1 + event = events[0] + assert event["hook"] == "boundary_events_write_guard" + assert event["tool"] == "Write" + assert event["decision"] == "block" + assert event["matched_rule"] == "ledger-write-blocked" + assert event["session_id"] == "sess-1" + + +def test_edit_to_ledger_is_blocked(tmp_path: Path) -> None: + result = _run( + { + "tool_name": "Edit", + "tool_input": { + "file_path": ".claude/metrics/boundary-events.jsonl", + "old_string": "a", + "new_string": "b", + }, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + assert b"emit_boundary_event()" in result.stdout + + +def test_write_to_unrelated_file_is_allowed_and_records_nothing(tmp_path: Path) -> None: + result = _run( + { + "tool_name": "Write", + "tool_input": {"file_path": "README.md", "content": "hello\n"}, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 0 + assert result.stdout == b"" + assert not (tmp_path / ".claude" / "metrics" / "boundary-events.jsonl").exists() + + +def test_write_to_review_verdicts_store_is_allowed(tmp_path: Path) -> None: + """Slice 2's own store (#2166) — not affected by this guard, which + matches the exact ledger name/path, not a `.claude/metrics/` prefix.""" + result = _run( + { + "tool_name": "Write", + "tool_input": { + "file_path": ".claude/metrics/review-verdicts.jsonl", + "content": "{}\n", + }, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 0 + assert result.stdout == b"" + + +def test_absolute_path_to_ledger_is_blocked(tmp_path: Path) -> None: + absolute = str(tmp_path / ".claude" / "metrics" / "boundary-events.jsonl") + result = _run( + { + "tool_name": "Write", + "tool_input": {"file_path": absolute, "content": "{}\n"}, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + + +def test_dot_slash_prefixed_path_to_ledger_is_blocked(tmp_path: Path) -> None: + result = _run( + { + "tool_name": "Write", + "tool_input": { + "file_path": "./.claude/metrics/boundary-events.jsonl", + "content": "{}\n", + }, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + + +def test_missing_tool_input_is_silent_pass(tmp_path: Path) -> None: + result = _run({"tool_name": "Write", "cwd": str(tmp_path)}) + assert result.returncode == 0 + assert result.stdout == b"" + + +def test_malformed_stdin_is_silent_pass() -> None: + result = subprocess.run( + [sys.executable, str(_HOOK_PY)], + input=b"not json", + env={"PATH": os.environ.get("PATH", "/usr/bin:/bin")}, + capture_output=True, + timeout=10, + check=False, + ) + assert result.returncode == 0 + assert result.stdout == b"" From 11e6914462574e699bcadfbd71fafb602ef5cd36 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:01:31 +0000 Subject: [PATCH 03/10] feat(hooks): extend boundary-events write guard to Bash write-shaped commands Part of #2171, part of #2164. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .../hooks/boundary_events_write_guard.py | 127 ++++++++++++-- plugins/dev-team/hooks/hooks.json | 8 + plugins/dev-team/settings.json | 8 + .../hooks/test_boundary_events_write_guard.py | 105 +++++++++++- .../hooks/test_boundary_events_write_guard.py | 157 +++++++++++++++++- 5 files changed, 384 insertions(+), 21 deletions(-) diff --git a/plugins/dev-team/hooks/boundary_events_write_guard.py b/plugins/dev-team/hooks/boundary_events_write_guard.py index e8c1333d1..1618fff8b 100755 --- a/plugins/dev-team/hooks/boundary_events_write_guard.py +++ b/plugins/dev-team/hooks/boundary_events_write_guard.py @@ -12,15 +12,27 @@ same way `pre_tool_guard.py` raises it for other sensitive paths, by blocking a Write/Edit whose `file_path`/`path` resolves to the ledger. -Step 1.1 of plans/2164-verdict-ledger-writer.md — Write/Edit only. Bash -write-shaped command detection and `settings.json` registration are a -separate, later step (Step 1.2) and are NOT part of this module yet. +Step 1.1 of plans/2164-verdict-ledger-writer.md added the Write/Edit +path-match guard. Step 1.2 extends this same module to also inspect Bash +`tool_input.command` text for write-shaped commands (redirect, `tee`, +`sed -i`, `cp`/`mv`/`rm`/`truncate`/`dd`, or a write/append/exclusive-mode +Python `open()`) that target the ledger by filename, in any path form — +see `bash_command_writes_to_ledger()` and `_BASH_WRITE_SHAPE_PATTERNS` +below. The hook is registered in `settings.json`'s existing `Write|Edit` +and `Bash` `PreToolUse` matcher groups. Contract (docs/python-hook-contract.md): - Input : PreToolUse JSON on stdin — `tool_input.file_path` or - `tool_input.path`, resolved relative to `cwd` when not absolute - Output: block message on stdout naming `emit_boundary_event()` - (`hooks/lib/boundary_events.py`) as the remedy + Input : PreToolUse JSON on stdin — for Write/Edit, `tool_input.file_path` + or `tool_input.path`, resolved relative to `cwd` when not + absolute; for Bash, `tool_input.command` + Output: block message on stdout. The Write/Edit-path message names + `emit_boundary_event()` (`hooks/lib/boundary_events.py`) as the + remedy — a Python-only function unreachable from a shell + command. The Bash-path message instead names the + `hooks/lib/boundary_events.py` CLI (`python3 + hooks/lib/boundary_events.py ...`, #1461) — the only + remedy actually reachable from a Bash command (plan-review-ux + finding, Step 1.2). Exit : 2 to block, 0 to allow. Fail-open on any exception (malformed payload, unreadable cwd, unresolvable repo root) — never raises. @@ -35,6 +47,7 @@ from __future__ import annotations import os +import re import sys from pathlib import Path @@ -95,31 +108,115 @@ def targets_ledger(file_path: str, cwd: str) -> bool: return candidate_norm == ledger_norm +def _extract_command(tool_input: object) -> str: + """Return `tool_input.command`, or empty string — mirrors + `destructive_guard._extract_command`'s field access.""" + if not isinstance(tool_input, dict): + return "" + command = tool_input.get("command") + return command if isinstance(command, str) else "" + + +# Any prefix of path characters (word chars, dots, slashes, hyphens) ending +# in the ledger's literal filename. Matches the filename in every path form +# the plan requires (relative, absolute, "./"-prefixed, or bare after a +# `cd .claude/metrics`-shaped prefix) with a single suffix, since all four +# forms literally end in this substring. +_LEDGER_PATH_SUFFIX = r"[\w./-]*" + re.escape(_LEDGER_NAME) + +# Bash write-shaped patterns targeting the ledger (Step 1.2, #2171). Each +# pattern embeds `_LEDGER_PATH_SUFFIX` directly, so a match always means the +# command's write operation targets the ledger specifically — not merely +# that the ledger's filename appears somewhere unrelated in the command +# (e.g. `cat boundary-events.jsonl | tee /tmp/copy.txt` reads the ledger and +# writes elsewhere; it does not match the `tee` pattern below because +# `tee`'s own argument is `/tmp/copy.txt`, not the ledger). Heuristic, not a +# sandbox — see the plan's Risks note ("raises the cost of forgery," per +# #2171's Out of Scope). Mirrors destructive_guard.py's own pattern-table +# idiom (a module-level constant, one comment per pattern). +_BASH_WRITE_SHAPE_PATTERNS: tuple[re.Pattern, ...] = ( + # `>`/`>>` redirect whose target is the ledger. Also catches a + # heredoc's trailing redirect operator (`cat <<'EOF' >> ...`) — the + # heredoc `<<` marker itself is never parsed or matched (plan Decision + # note); detection is via this same trailing operator, like any other + # write-shaped command. + re.compile(rf">{{1,2}}\s*['\"]?{_LEDGER_PATH_SUFFIX}"), + # `tee` writing to the ledger. + re.compile(rf"\btee\b(?:\s+-{{1,2}}\S+)*\s+['\"]?{_LEDGER_PATH_SUFFIX}"), + # `sed -i` (in-place edit) targeting the ledger, within the same shell + # statement (stops at `;`/`|`/`&` so an unrelated later statement that + # happens to also mention the ledger doesn't false-positive). + re.compile(rf"\bsed\b[^;|&\n]*-i\b[^;|&\n]*['\"]?{_LEDGER_PATH_SUFFIX}"), + # cp/mv/rm/truncate/dd targeting the ledger, within the same shell + # statement (same same-statement scoping as the sed pattern above). + re.compile(rf"\b(?:cp|mv|rm|truncate|dd)\b[^;|&\n]*{_LEDGER_PATH_SUFFIX}"), + # A Python `open(...)` call on the ledger using a write/append/ + # exclusive mode ("w"/"a"/"x", optionally suffixed e.g. "wb"/"a+"/"x+"). + # A read-mode or mode-omitted (default "r") `open()` never matches this + # pattern, so it is allowed. + re.compile( + rf"open\(\s*['\"]{_LEDGER_PATH_SUFFIX}['\"]\s*,\s*['\"](?:w|a|x)[\w+]*['\"]" + ), +) + + +def bash_command_writes_to_ledger(command: str) -> bool: + """True when `command` is write-shaped AND targets the ledger by + filename, in any path form — see `_BASH_WRITE_SHAPE_PATTERNS`. A + read-shaped command referencing the same filename (`cat`, `grep`, + `tail`, `head`, a read-mode `open()`) never matches any pattern here, + so it is allowed without a separate read-allowlist check.""" + if not command: + return False + return any(pattern.search(command) for pattern in _BASH_WRITE_SHAPE_PATTERNS) + + def main() -> int: try: payload = read_stdin_json() if payload is None: return 0 - file_path = _extract_file_path(payload.get("tool_input")) - if not file_path: - return 0 - raw_cwd = payload.get("cwd") cwd = ( raw_cwd if isinstance(raw_cwd, str) and raw_cwd and "\0" not in raw_cwd else "." ) - - if not targets_ledger(file_path, cwd): - return 0 - raw_tool = payload.get("tool_name") tool = raw_tool if isinstance(raw_tool, str) and raw_tool else "Write" raw_session_id = payload.get("session_id") session_id = raw_session_id if isinstance(raw_session_id, str) else None + if tool == "Bash": + command = _extract_command(payload.get("tool_input")) + if not bash_command_writes_to_ledger(command): + return 0 + + emit_boundary_event( + cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id + ) + print( + f"BLOCKED: This Bash command writes to '.claude/metrics/{_LEDGER_NAME}', " + "which is not allowed." + ) + print( + "This file is the boundary-events accountability ledger (#859) — " + "it is append-only from the session's perspective." + ) + print( + "Use 'python3 hooks/lib/boundary_events.py ...' " + "(plugins/dev-team/hooks/lib/boundary_events.py) instead of writing to it from Bash." + ) + return 2 + + file_path = _extract_file_path(payload.get("tool_input")) + if not file_path: + return 0 + + if not targets_ledger(file_path, cwd): + return 0 + emit_boundary_event( cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id ) diff --git a/plugins/dev-team/hooks/hooks.json b/plugins/dev-team/hooks/hooks.json index 81ac8be2c..1d1564c45 100644 --- a/plugins/dev-team/hooks/hooks.json +++ b/plugins/dev-team/hooks/hooks.json @@ -47,6 +47,10 @@ { "type": "command", "command": "sh \"${CLAUDE_PLUGIN_ROOT}/hooks/py.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/refactor_test_freeze_guard.py\"" + }, + { + "type": "command", + "command": "sh \"${CLAUDE_PLUGIN_ROOT}/hooks/py.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/boundary_events_write_guard.py\"" } ] }, @@ -105,6 +109,10 @@ { "type": "command", "command": "sh \"${CLAUDE_PLUGIN_ROOT}/hooks/py.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/scan_bash_command_for_banned_scripts.py\"" + }, + { + "type": "command", + "command": "sh \"${CLAUDE_PLUGIN_ROOT}/hooks/py.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/boundary_events_write_guard.py\"" } ] }, diff --git a/plugins/dev-team/settings.json b/plugins/dev-team/settings.json index 262edd748..92d641310 100644 --- a/plugins/dev-team/settings.json +++ b/plugins/dev-team/settings.json @@ -63,6 +63,10 @@ { "type": "command", "command": "sh hooks/py.sh hooks/refactor_test_freeze_guard.py" + }, + { + "type": "command", + "command": "sh hooks/py.sh hooks/boundary_events_write_guard.py" } ] }, @@ -121,6 +125,10 @@ { "type": "command", "command": "sh hooks/py.sh hooks/scan_bash_command_for_banned_scripts.py" + }, + { + "type": "command", + "command": "sh hooks/py.sh hooks/boundary_events_write_guard.py" } ] }, diff --git a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py index 2288265ed..019857be4 100644 --- a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py +++ b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py @@ -1,6 +1,8 @@ -"""Unit tests for hooks/boundary_events_write_guard.py (#2171, plan Step -1.1 — Write/Edit path-match guard only; Bash write-shaped command detection -and settings.json registration are Step 1.2, not covered here). +"""Unit tests for hooks/boundary_events_write_guard.py (#2171). + +Plan Step 1.1 covers the Write/Edit path-match guard. Plan Step 1.2 adds +Bash `tool_input.command` write-shape detection (`bash_command_writes_to_ledger`) +and its `main()` dispatch — covered below. In-process, stdin-monkeypatched, with `emit_boundary_event` stubbed (same split `test_destructive_guard.py` uses) — real subprocess + real-ledger @@ -184,3 +186,100 @@ def test_main_fails_open_on_empty_stdin(monkeypatch): def test_main_silent_pass_when_file_path_absent(monkeypatch, tmp_path): _stdin(monkeypatch, {"tool_name": "Write", "tool_input": {}, "cwd": str(tmp_path)}) assert guard.main() == 0 + + +# --------------------------------------------------------------------------- +# bash_command_writes_to_ledger — write-shaped commands (Step 1.2) +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "command", + [ + # Redirect, across all four path forms (Examples Outline). + "echo '{}' >> .claude/metrics/boundary-events.jsonl", + "echo '{}' >> /repo/.claude/metrics/boundary-events.jsonl", + "echo '{}' >> ./.claude/metrics/boundary-events.jsonl", + "cd .claude/metrics && echo '{}' >> boundary-events.jsonl", + # Heredoc, caught via its trailing redirect operator, not `<<`. + "cat <<'EOF' >> .claude/metrics/boundary-events.jsonl\n{}\nEOF", + # tee + "echo '{}' | tee -a .claude/metrics/boundary-events.jsonl", + # sed -i + "sed -i 's/a/b/' .claude/metrics/boundary-events.jsonl", + # cp/mv/rm/truncate/dd + "rm .claude/metrics/boundary-events.jsonl", + "mv .claude/metrics/boundary-events.jsonl /tmp/moved.jsonl", + "cp .claude/metrics/boundary-events.jsonl /tmp/copy.jsonl", + "truncate -s 0 .claude/metrics/boundary-events.jsonl", + "dd if=/dev/null of=.claude/metrics/boundary-events.jsonl", + # python3 -c with a write/append-mode open() + "python3 -c \"open('.claude/metrics/boundary-events.jsonl', 'w').write('{}')\"", + "python3 -c \"open('.claude/metrics/boundary-events.jsonl', 'a').write('{}')\"", + ], +) +def test_bash_command_writes_to_ledger_true_for_write_shaped_commands(command): + assert guard.bash_command_writes_to_ledger(command) is True + + +@pytest.mark.parametrize( + "command", + [ + "tail -20 .claude/metrics/boundary-events.jsonl", + "cat .claude/metrics/boundary-events.jsonl", + "grep foo .claude/metrics/boundary-events.jsonl", + "head .claude/metrics/boundary-events.jsonl", + # read-mode (mode omitted, defaults to "r") open() + "python3 -c \"print(open('.claude/metrics/boundary-events.jsonl').read())\"", + # reads the ledger, writes elsewhere — tee's own target is not the ledger + "cat .claude/metrics/boundary-events.jsonl | tee /tmp/copy.jsonl", + # write-shaped, but targets an unrelated file + "echo '{}' >> .claude/metrics/session-digest.jsonl", + "", + ], +) +def test_bash_command_writes_to_ledger_false_for_read_or_unrelated_commands(command): + assert guard.bash_command_writes_to_ledger(command) is False + + +# --------------------------------------------------------------------------- +# main() — Bash tool shape (Step 1.2) +# --------------------------------------------------------------------------- + + +def test_main_blocks_bash_redirect_to_ledger(monkeypatch, tmp_path, capsys): + _stdin( + monkeypatch, + { + "tool_name": "Bash", + "tool_input": { + "command": "echo '{}' >> .claude/metrics/boundary-events.jsonl" + }, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 2 + out = capsys.readouterr().out + assert "BLOCKED" in out + assert "hooks/lib/boundary_events.py" in out + # Bash-path message names the CLI, not the Python-only function + # (plan-review-ux finding, Step 1.2). + assert "emit_boundary_event()" not in out + + +def test_main_allows_bash_read_of_ledger(monkeypatch, tmp_path, capsys): + _stdin( + monkeypatch, + { + "tool_name": "Bash", + "tool_input": {"command": "tail -20 .claude/metrics/boundary-events.jsonl"}, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 0 + assert capsys.readouterr().out == "" + + +def test_main_allows_bash_command_with_no_command_field(monkeypatch, tmp_path): + _stdin(monkeypatch, {"tool_name": "Bash", "tool_input": {}, "cwd": str(tmp_path)}) + assert guard.main() == 0 diff --git a/tests/hooks/test_boundary_events_write_guard.py b/tests/hooks/test_boundary_events_write_guard.py index 3d7a71392..460123691 100644 --- a/tests/hooks/test_boundary_events_write_guard.py +++ b/tests/hooks/test_boundary_events_write_guard.py @@ -1,6 +1,7 @@ -"""End-to-end tests for hooks/boundary_events_write_guard.py (#2171, plan -Step 1.1 — Write/Edit path-match guard only; Bash write-shaped command -detection and settings.json registration are Step 1.2, not covered here). +"""End-to-end tests for hooks/boundary_events_write_guard.py (#2171). + +Plan Step 1.1 covers the Write/Edit path-match guard. Plan Step 1.2 adds +Bash `tool_input.command` write-shape detection — covered below. Drives main() via subprocess with representative PreToolUse JSON payloads, using the real `emit_boundary_event()` write path (mirrors @@ -20,6 +21,8 @@ import sys from pathlib import Path +import pytest + from _repo_root import REPO_ROOT as _REPO_ROOT _HOOK_PY = _REPO_ROOT / "plugins" / "dev-team" / "hooks" / "boundary_events_write_guard.py" @@ -167,3 +170,151 @@ def test_malformed_stdin_is_silent_pass() -> None: ) assert result.returncode == 0 assert result.stdout == b"" + + +# --------------------------------------------------------------------------- +# Bash write-shaped commands (Step 1.2) +# --------------------------------------------------------------------------- + + +def test_bash_redirect_to_ledger_is_blocked_and_records_its_own_event( + tmp_path: Path, +) -> None: + ledger = tmp_path / ".claude" / "metrics" / "boundary-events.jsonl" + + result = _run( + { + "tool_name": "Bash", + "tool_input": { + "command": "echo '{}' >> .claude/metrics/boundary-events.jsonl" + }, + "cwd": str(tmp_path), + "session_id": "sess-2", + } + ) + + assert result.returncode == 2 + assert b"BLOCKED" in result.stdout + assert b"hooks/lib/boundary_events.py" in result.stdout + # Bash-path message names the CLI, not the Python-only function + # (plan-review-ux finding, Step 1.2). + assert b"emit_boundary_event()" not in result.stdout + + events = _read_jsonl(ledger) + assert len(events) == 1 + event = events[0] + assert event["hook"] == "boundary_events_write_guard" + assert event["tool"] == "Bash" + assert event["decision"] == "block" + assert event["matched_rule"] == "ledger-write-blocked" + assert event["session_id"] == "sess-2" + + +def test_bash_heredoc_with_trailing_redirect_to_ledger_is_blocked( + tmp_path: Path, +) -> None: + result = _run( + { + "tool_name": "Bash", + "tool_input": { + "command": "cat <<'EOF' >> .claude/metrics/boundary-events.jsonl\n" + '{"forged": true}\n' + "EOF" + }, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + assert b"BLOCKED" in result.stdout + + +def test_bash_tee_to_ledger_is_blocked(tmp_path: Path) -> None: + result = _run( + { + "tool_name": "Bash", + "tool_input": { + "command": "echo '{}' | tee -a .claude/metrics/boundary-events.jsonl" + }, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + + +@pytest.mark.parametrize( + "command", + [ + # relative + "echo '{}' >> .claude/metrics/boundary-events.jsonl", + # absolute (constructed per-test below instead, see next test) + # "./"-prefixed + "echo '{}' >> ./.claude/metrics/boundary-events.jsonl", + # bare filename after a `cd .claude/metrics`-shaped prefix + "cd .claude/metrics && echo '{}' >> boundary-events.jsonl", + ], +) +def test_bash_write_blocked_regardless_of_relative_path_form( + tmp_path: Path, command: str +) -> None: + result = _run( + { + "tool_name": "Bash", + "tool_input": {"command": command}, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + + +def test_bash_write_blocked_for_absolute_path_form(tmp_path: Path) -> None: + absolute = str(tmp_path / ".claude" / "metrics" / "boundary-events.jsonl") + result = _run( + { + "tool_name": "Bash", + "tool_input": {"command": f"echo '{{}}' >> {absolute}"}, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 2 + + +@pytest.mark.parametrize( + "command", + [ + "tail -20 .claude/metrics/boundary-events.jsonl", + "cat .claude/metrics/boundary-events.jsonl", + "grep foo .claude/metrics/boundary-events.jsonl", + "python3 -c \"print(open('.claude/metrics/boundary-events.jsonl').read())\"", + ], +) +def test_bash_reads_of_ledger_are_allowed(tmp_path: Path, command: str) -> None: + result = _run( + { + "tool_name": "Bash", + "tool_input": {"command": command}, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 0 + assert result.stdout == b"" + assert not (tmp_path / ".claude" / "metrics" / "boundary-events.jsonl").exists() + + +def test_bash_write_to_unrelated_file_is_allowed(tmp_path: Path) -> None: + result = _run( + { + "tool_name": "Bash", + "tool_input": { + "command": "echo '{}' >> .claude/metrics/session-digest.jsonl" + }, + "cwd": str(tmp_path), + } + ) + + assert result.returncode == 0 + assert result.stdout == b"" From 1a3b26575b87ae89fd8bbcb78b4dbeeb731eae7e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:27:53 +0000 Subject: [PATCH 04/10] fix(hooks): harden boundary-events guard against write-shape regex gaps Slice-1 review-fix loop: widen the Bash write-shape regexes (Python r+/mode= open(), tee/sed non-first-operand forms, ~/$VAR/quoted-space path prefixes), symmetrize targets_ledger()'s Write/Edit path comparison against pre_tool_guard.py's own established idiom, correct the Bash remedy message to a real invocable CLI shape, extract a shared _block() helper plus per-tool handlers, and address naming/test-smell/test-review findings (double-waiver markers, custom assertion/run_raw helpers, safety-net and NUL-byte cwd coverage, empty-stdout assertions, de-hardcoded path constant). Part of #2171, part of #2164. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .../hooks/boundary_events_write_guard.py | 199 +++++++++++------ .../hooks/test_boundary_events_write_guard.py | 205 ++++++++++++++---- .../hooks/test_boundary_events_write_guard.py | 108 +++++---- 3 files changed, 370 insertions(+), 142 deletions(-) diff --git a/plugins/dev-team/hooks/boundary_events_write_guard.py b/plugins/dev-team/hooks/boundary_events_write_guard.py index 1618fff8b..8979983e4 100755 --- a/plugins/dev-team/hooks/boundary_events_write_guard.py +++ b/plugins/dev-team/hooks/boundary_events_write_guard.py @@ -29,17 +29,30 @@ `emit_boundary_event()` (`hooks/lib/boundary_events.py`) as the remedy — a Python-only function unreachable from a shell command. The Bash-path message instead names the - `hooks/lib/boundary_events.py` CLI (`python3 - hooks/lib/boundary_events.py ...`, #1461) — the only - remedy actually reachable from a Bash command (plan-review-ux - finding, Step 1.2). + `hooks/lib/boundary_events.py` CLI's actual invocable shape + (`python3 plugins/dev-team/hooks/lib/boundary_events.py --event + + --subject-hash ...`, #1461) — `--event` is a flag drawn + from a closed `choices` set, not a positional, and most events + require `--subject-hash`; the CLI cannot construct an + arbitrary row by design — a genuinely custom row still needs + `emit_boundary_event()` called from a hook (plan-review-ux + finding, Step 1.2, corrected by review). Exit : 2 to block, 0 to allow. Fail-open on any exception (malformed payload, unreadable cwd, unresolvable repo root) — never raises. Path matching is lexical only (`os.path.abspath` — `.`/`..` collapsing, no symlink following): a symlink-escape sandbox is explicitly out of scope (plan Step 1.1 TEST note) — this guard raises the cost of forgery, it does -not sandbox the filesystem. +not sandbox the filesystem. One narrow exception: `targets_ledger()` +resolves the `cwd` anchor itself via `os.path.realpath` before joining a +relative `file_path` against it (review finding — see `targets_ledger()`'s +own docstring) so that anchor stays symmetric with the ledger side, which +already passes through `git rev-parse --show-toplevel`'s own symlink +resolution via `artifact_paths.resolve_file()`/`project_root()`. Everything +beyond that single anchor — the rest of the joined path, and all Bash +command-text matching below — stays purely lexical, no further symlink +following. Stdlib-only. See ADR 0014. """ @@ -80,9 +93,9 @@ def _extract_file_path(tool_input: object) -> str: file_path = tool_input.get("file_path") if isinstance(file_path, str) and file_path: return file_path - other = tool_input.get("path") - if isinstance(other, str) and other: - return other + path = tool_input.get("path") + if isinstance(path, str) and path: + return path return "" @@ -91,10 +104,23 @@ def targets_ledger(file_path: str, cwd: str) -> bool: the same on-disk path `emit_boundary_event()` itself resolves and writes to — `artifact_paths.resolve_file("metrics", ...)` under the repo root, not a bare `.claude/metrics/` prefix match (so - `review-verdicts.jsonl`, Slice 2's own store, is unaffected).""" + `review-verdicts.jsonl`, Slice 2's own store, is unaffected). + + The `cwd` anchor is realpath'd before the join (review finding): the + ledger side already resolves symlinks transparently, because + `artifact_paths.resolve_file()` -> `project_root()` finds the repo root + via `git rev-parse --show-toplevel`, and git itself resolves a + symlinked `cwd` to its real location. Joining `file_path` against the + *unresolved* `cwd` string, then comparing both sides with only + `os.path.abspath` (no symlink resolution), left the two sides + comparing a symlinked-form candidate against a resolved-form ledger — + a real, if narrow, false negative on a symlinked `cwd`. Resolving only + the `cwd` anchor keeps this guard's `os.path.abspath` comparison the + same symmetric idiom `pre_tool_guard.py` uses elsewhere, without + resolving symlinks anywhere else in the path (module docstring).""" if not file_path: return False - base = Path(cwd) if cwd else Path.cwd() + base = Path(os.path.realpath(cwd)) if cwd else Path.cwd() candidate = Path(file_path) if not candidate.is_absolute(): candidate = base / candidate @@ -117,12 +143,16 @@ def _extract_command(tool_input: object) -> str: return command if isinstance(command, str) else "" -# Any prefix of path characters (word chars, dots, slashes, hyphens) ending -# in the ledger's literal filename. Matches the filename in every path form -# the plan requires (relative, absolute, "./"-prefixed, or bare after a -# `cd .claude/metrics`-shaped prefix) with a single suffix, since all four -# forms literally end in this substring. -_LEDGER_PATH_SUFFIX = r"[\w./-]*" + re.escape(_LEDGER_NAME) +# Any prefix of path characters ending in the ledger's literal filename. +# Matches the filename in every ordinary path form the plan requires +# (relative, absolute, "./"-prefixed, bare after a `cd .claude/metrics` +# prefix, `~/`-prefixed, `$VAR`/`${VAR}`-expanded, or a quoted path +# containing a space), since all of those literally end in this substring. +# Widening this class is not an attempt at obfuscation-grade path matching +# (shell escapes, `$(...)` substitution, base64-encoded paths, etc. remain +# out of scope — see the module docstring's "heuristic, not a sandbox" +# note); it only covers path forms a real command would ordinarily write. +_LEDGER_PATH_SUFFIX = r"[\w./~${} -]*" + re.escape(_LEDGER_NAME) # Bash write-shaped patterns targeting the ledger (Step 1.2, #2171). Each # pattern embeds `_LEDGER_PATH_SUFFIX` directly, so a match always means the @@ -141,21 +171,32 @@ def _extract_command(tool_input: object) -> str: # note); detection is via this same trailing operator, like any other # write-shaped command. re.compile(rf">{{1,2}}\s*['\"]?{_LEDGER_PATH_SUFFIX}"), - # `tee` writing to the ledger. - re.compile(rf"\btee\b(?:\s+-{{1,2}}\S+)*\s+['\"]?{_LEDGER_PATH_SUFFIX}"), - # `sed -i` (in-place edit) targeting the ledger, within the same shell - # statement (stops at `;`/`|`/`&` so an unrelated later statement that - # happens to also mention the ledger doesn't false-positive). - re.compile(rf"\bsed\b[^;|&\n]*-i\b[^;|&\n]*['\"]?{_LEDGER_PATH_SUFFIX}"), + # `tee` writing to the ledger, within the same shell statement (same + # same-statement scoping as the cp/mv/rm pattern below) — the ledger + # can be any of tee's operands (`tee /tmp/log boundary-events.jsonl`), + # not just its first. + re.compile(rf"\btee\b[^;|&\n]*['\"]?{_LEDGER_PATH_SUFFIX}"), + # `sed -i`/`sed --in-place` (in-place edit) targeting the ledger, within + # the same shell statement (stops at `;`/`|`/`&` so an unrelated later + # statement that happens to also mention the ledger doesn't + # false-positive). + re.compile( + rf"\bsed\b[^;|&\n]*(?:-i\b|--in-place\b)[^;|&\n]*['\"]?{_LEDGER_PATH_SUFFIX}" + ), # cp/mv/rm/truncate/dd targeting the ledger, within the same shell # statement (same same-statement scoping as the sed pattern above). re.compile(rf"\b(?:cp|mv|rm|truncate|dd)\b[^;|&\n]*{_LEDGER_PATH_SUFFIX}"), - # A Python `open(...)` call on the ledger using a write/append/ - # exclusive mode ("w"/"a"/"x", optionally suffixed e.g. "wb"/"a+"/"x+"). - # A read-mode or mode-omitted (default "r") `open()` never matches this - # pattern, so it is allowed. + # A Python `open(...)` call on the ledger using any mode that permits + # writing: "w"/"a"/"x" (optionally suffixed, e.g. "wb"/"a+"/"x+"), or a + # "+" read-write mode such as "r+"/"rb+"/"r+b" — any mode string + # containing w/a/x/+ can write. The mode may be positional or the + # keyword form (`mode='a'`). "Allowed" means read-ONLY: a mode-omitted + # (default "r") or explicit pure-"r" `open()` never matches this + # pattern — not "read-mode" generically, since "r+" is a read-*and*- + # write mode despite starting with "r". re.compile( - rf"open\(\s*['\"]{_LEDGER_PATH_SUFFIX}['\"]\s*,\s*['\"](?:w|a|x)[\w+]*['\"]" + rf"open\(\s*['\"]{_LEDGER_PATH_SUFFIX}['\"]\s*,\s*" + r"(?:mode\s*=\s*)?['\"][^'\"]*[wax+][^'\"]*['\"]" ), ) @@ -171,6 +212,68 @@ def bash_command_writes_to_ledger(command: str) -> bool: return any(pattern.search(command) for pattern in _BASH_WRITE_SHAPE_PATTERNS) +def _block( + cwd: str, + tool: str, + session_id: str | None, + blocked_message: str, + remedy_message: str, +) -> int: + """Shared block sequence for both `main()` branches: record the guard's + own decision (every sibling guard does — see the module docstring), + print the block explanation, and return the block exit code.""" + emit_boundary_event( + cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id + ) + print(blocked_message) + print( + "This file is the boundary-events accountability ledger (#859) — " + "it is append-only from the session's perspective." + ) + print(remedy_message) + return 2 + + +def _handle_bash_tool(payload: dict, cwd: str, session_id: str | None) -> int: + command = _extract_command(payload.get("tool_input")) + if not bash_command_writes_to_ledger(command): + return 0 + + return _block( + cwd, + "Bash", + session_id, + f"BLOCKED: This Bash command writes to '.claude/metrics/{_LEDGER_NAME}', " + "which is not allowed.", + "Use 'python3 plugins/dev-team/hooks/lib/boundary_events.py " + "--event " + "--subject-hash ...' instead of writing to it from Bash — " + "an arbitrary row isn't CLI-constructible by design (that CLI only " + "accepts a closed --event vocabulary); this exact row needs a " + "Python-side hook change instead.", + ) + + +def _handle_write_edit_tool( + payload: dict, cwd: str, tool: str, session_id: str | None +) -> int: + file_path = _extract_file_path(payload.get("tool_input")) + if not file_path: + return 0 + + if not targets_ledger(file_path, cwd): + return 0 + + return _block( + cwd, + tool, + session_id, + f"BLOCKED: Direct write to '{file_path}' is not allowed.", + "Use emit_boundary_event() in plugins/dev-team/hooks/lib/boundary_events.py " + "instead of writing to it directly.", + ) + + def main() -> int: try: payload = read_stdin_json() @@ -189,47 +292,9 @@ def main() -> int: session_id = raw_session_id if isinstance(raw_session_id, str) else None if tool == "Bash": - command = _extract_command(payload.get("tool_input")) - if not bash_command_writes_to_ledger(command): - return 0 - - emit_boundary_event( - cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id - ) - print( - f"BLOCKED: This Bash command writes to '.claude/metrics/{_LEDGER_NAME}', " - "which is not allowed." - ) - print( - "This file is the boundary-events accountability ledger (#859) — " - "it is append-only from the session's perspective." - ) - print( - "Use 'python3 hooks/lib/boundary_events.py ...' " - "(plugins/dev-team/hooks/lib/boundary_events.py) instead of writing to it from Bash." - ) - return 2 - - file_path = _extract_file_path(payload.get("tool_input")) - if not file_path: - return 0 - - if not targets_ledger(file_path, cwd): - return 0 + return _handle_bash_tool(payload, cwd, session_id) - emit_boundary_event( - cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id - ) - print(f"BLOCKED: Direct write to '{file_path}' is not allowed.") - print( - "This file is the boundary-events accountability ledger (#859) — " - "it is append-only from the session's perspective." - ) - print( - "Use emit_boundary_event() in plugins/dev-team/hooks/lib/boundary_events.py " - "instead of writing to it directly." - ) - return 2 + return _handle_write_edit_tool(payload, cwd, tool, session_id) except Exception: # noqa: BLE001 - fail-open by design, see module docstring return 0 diff --git a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py index 019857be4..59c1e319e 100644 --- a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py +++ b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py @@ -12,6 +12,8 @@ from __future__ import annotations import json +import os +import subprocess import sys import pytest @@ -19,12 +21,23 @@ from _repo_root import REPO_ROOT as _REPO_ROOT _PLUGIN_DIR = _REPO_ROOT / "plugins" / "dev-team" +_TESTS_LIB = _PLUGIN_DIR / "tests" / "lib" -for _p in (_PLUGIN_DIR / "hooks",): +for _p in (_PLUGIN_DIR / "hooks", _TESTS_LIB): if str(_p) not in sys.path: sys.path.insert(0, str(_p)) import boundary_events_write_guard as guard +from hermetic import hermetic_git_env # type: ignore[import-not-found] + +# Test-file-local constant (not `guard._LEDGER_NAME` — importing the code +# under test's own constant would make the test oracle circular). +_LEDGER_REL_PATH = ".claude/metrics/boundary-events.jsonl" + +# Captured before any per-test monkeypatching can shadow it, so +# `test_main_swallows_exception_from_real_emit_boundary_event` can restore +# the real wrapper regardless of `_no_boundary_events`'s autouse stub. +_REAL_EMIT_BOUNDARY_EVENT = guard.emit_boundary_event @pytest.fixture(autouse=True) @@ -34,6 +47,7 @@ def _no_boundary_events(monkeypatch): the guard itself. `tests/hooks/test_boundary_events_write_guard.py` exercises the real subprocess + real emit path (matches pre_tool_guard.py's own test split).""" + # double-waiver: B1 — emit_boundary_event opens a real file handle monkeypatch.setattr(guard, "emit_boundary_event", lambda *a, **k: None) @@ -64,15 +78,11 @@ def test_extract_file_path_empty_when_not_a_dict(): def test_targets_ledger_relative_path(tmp_path): - assert guard.targets_ledger( - ".claude/metrics/boundary-events.jsonl", str(tmp_path) - ) + assert guard.targets_ledger(_LEDGER_REL_PATH, str(tmp_path)) def test_targets_ledger_dot_slash_prefixed_path(tmp_path): - assert guard.targets_ledger( - "./.claude/metrics/boundary-events.jsonl", str(tmp_path) - ) + assert guard.targets_ledger("./" + _LEDGER_REL_PATH, str(tmp_path)) def test_targets_ledger_absolute_path(tmp_path): @@ -98,6 +108,36 @@ def test_targets_ledger_false_for_empty_path(tmp_path): assert not guard.targets_ledger("", str(tmp_path)) +def test_targets_ledger_true_for_relative_path_via_symlinked_cwd(tmp_path): + """Review finding (Step 1.1 correction): `targets_ledger()` compared an + `os.path.abspath`-normalized candidate against a + `artifact_paths.resolve_file()`/git-resolved ledger path — an + asymmetric comparison. `resolve_file()` -> `project_root()` finds the + repo root via `git rev-parse --show-toplevel`, which resolves a + symlinked `cwd` to its real location; joining `file_path` against the + *unresolved* `cwd` then comparing only with `os.path.abspath` (no + symlink resolution) left a genuine false negative whenever `cwd` + itself is reached through a symlink. Reproduced here with a real git + repo and a real symlink (not mocked — this is exactly the git/ + filesystem interaction that produced the asymmetry) to prove the fix: + before the fix this returned False; after realpath'ing the `cwd` + anchor, it correctly returns True.""" + real_repo = tmp_path / "real-repo" + real_repo.mkdir() + subprocess.run( + ["git", "init", "-q"], + cwd=str(real_repo), + env=hermetic_git_env(home=tmp_path), + check=True, + ) + link_parent = tmp_path / "link-parent" + link_parent.mkdir() + symlinked_cwd = link_parent / "repo-link" + os.symlink(str(real_repo), str(symlinked_cwd), target_is_directory=True) + + assert guard.targets_ledger(_LEDGER_REL_PATH, str(symlinked_cwd)) + + # --------------------------------------------------------------------------- # main() — in-process, stdin-monkeypatched (Write and Edit tool shapes) # --------------------------------------------------------------------------- @@ -114,7 +154,7 @@ def test_main_blocks_write_to_ledger(monkeypatch, tmp_path, capsys): monkeypatch, { "tool_name": "Write", - "tool_input": {"file_path": ".claude/metrics/boundary-events.jsonl"}, + "tool_input": {"file_path": _LEDGER_REL_PATH}, "cwd": str(tmp_path), }, ) @@ -129,7 +169,7 @@ def test_main_blocks_edit_to_ledger(monkeypatch, tmp_path, capsys): monkeypatch, { "tool_name": "Edit", - "tool_input": {"file_path": ".claude/metrics/boundary-events.jsonl"}, + "tool_input": {"file_path": _LEDGER_REL_PATH}, "cwd": str(tmp_path), }, ) @@ -164,28 +204,93 @@ def test_main_allows_write_to_review_verdicts_store(monkeypatch, tmp_path, capsy assert capsys.readouterr().out == "" -def test_main_fails_open_on_missing_tool_input(monkeypatch, tmp_path): +def test_main_fails_open_on_missing_tool_input(monkeypatch, tmp_path, capsys): _stdin(monkeypatch, {"tool_name": "Write", "cwd": str(tmp_path)}) assert guard.main() == 0 + assert capsys.readouterr().out == "" -def test_main_fails_open_on_non_json_stdin(monkeypatch): +def test_main_fails_open_on_non_json_stdin(monkeypatch, capsys): import io monkeypatch.setattr(sys, "stdin", io.StringIO("not-json")) assert guard.main() == 0 + assert capsys.readouterr().out == "" -def test_main_fails_open_on_empty_stdin(monkeypatch): +def test_main_fails_open_on_empty_stdin(monkeypatch, capsys): import io monkeypatch.setattr(sys, "stdin", io.StringIO("")) assert guard.main() == 0 + assert capsys.readouterr().out == "" -def test_main_silent_pass_when_file_path_absent(monkeypatch, tmp_path): +def test_main_silent_pass_when_file_path_absent(monkeypatch, tmp_path, capsys): _stdin(monkeypatch, {"tool_name": "Write", "tool_input": {}, "cwd": str(tmp_path)}) assert guard.main() == 0 + assert capsys.readouterr().out == "" + + +def test_main_fails_open_on_non_string_cwd_with_embedded_nul(monkeypatch, capsys): + """`cwd` falls back to "." when it isn't a usable string — including a + string containing an embedded NUL byte, which would otherwise reach + `subprocess.run(cwd=...)` (inside `artifact_paths.project_root`, via + `targets_ledger` -> `resolve_file`) and raise `ValueError`. This exact + hazard was a previously-fixed real defect elsewhere in this codebase: + #1904 item 16 in `artifact_paths.project_root`'s own docstring + documents a `cwd` with an embedded NUL byte raising `ValueError` from + `subprocess.run`, not `OSError` — escaping that function's "never + raises" contract. `file_path` here names a file that is never the + ledger regardless of what "." resolves to for the ambient test + process, so this test stays deterministic across environments while + still proving the NUL-byte `cwd` was normalized away before reaching + any subprocess call (main() doesn't raise, and evaluation proceeds to + an ordinary "not blocked" verdict rather than crashing).""" + _stdin( + monkeypatch, + { + "tool_name": "Write", + "tool_input": {"file_path": ".claude/metrics/session-digest.jsonl"}, + "cwd": "bad\0cwd", + }, + ) + assert guard.main() == 0 + assert capsys.readouterr().out == "" + + +def test_main_swallows_exception_from_real_emit_boundary_event( + monkeypatch, tmp_path, capsys +): + """WARNING-severity coverage gap (review finding): `main()`'s local + `emit_boundary_event` safety-net wrapper (#859 — "even a misbehaving + helper must never affect this hook's exit code/stdout/stderr") was + never exercised with a raising underlying emit; every other test stubs + the wrapper itself via the autouse fixture, never proving the wrapper's + own `try`/`except` actually does anything. This test restores the real + wrapper and forces the underlying `_emit_boundary_event` to raise, + confirming `main()`'s exit code and stdout are unaffected.""" + # double-waiver: B1 — emit_boundary_event opens a real file handle + monkeypatch.setattr(guard, "emit_boundary_event", _REAL_EMIT_BOUNDARY_EVENT) + + def _raise(*_args, **_kwargs): + raise RuntimeError("simulated emit failure") + + # double-waiver: B1 — the underlying real emit also opens a file handle + monkeypatch.setattr(guard, "_emit_boundary_event", _raise) + + _stdin( + monkeypatch, + { + "tool_name": "Write", + "tool_input": {"file_path": _LEDGER_REL_PATH}, + "cwd": str(tmp_path), + }, + ) + assert guard.main() == 2 + out = capsys.readouterr().out + assert "BLOCKED" in out + assert "emit_boundary_event()" in out # --------------------------------------------------------------------------- @@ -197,25 +302,50 @@ def test_main_silent_pass_when_file_path_absent(monkeypatch, tmp_path): "command", [ # Redirect, across all four path forms (Examples Outline). - "echo '{}' >> .claude/metrics/boundary-events.jsonl", - "echo '{}' >> /repo/.claude/metrics/boundary-events.jsonl", - "echo '{}' >> ./.claude/metrics/boundary-events.jsonl", + "echo '{}' >> " + _LEDGER_REL_PATH, + "echo '{}' >> /repo/" + _LEDGER_REL_PATH, + "echo '{}' >> ./" + _LEDGER_REL_PATH, "cd .claude/metrics && echo '{}' >> boundary-events.jsonl", # Heredoc, caught via its trailing redirect operator, not `<<`. - "cat <<'EOF' >> .claude/metrics/boundary-events.jsonl\n{}\nEOF", + "cat <<'EOF' >> " + _LEDGER_REL_PATH + "\n{}\nEOF", # tee - "echo '{}' | tee -a .claude/metrics/boundary-events.jsonl", + "echo '{}' | tee -a " + _LEDGER_REL_PATH, + # tee with the ledger NOT as its first operand (review finding — + # previously a false negative: the pattern required the ledger to + # be tee's first argument). + "tee /tmp/log " + _LEDGER_REL_PATH, # sed -i - "sed -i 's/a/b/' .claude/metrics/boundary-events.jsonl", + "sed -i 's/a/b/' " + _LEDGER_REL_PATH, + # sed --in-place (GNU long form — review finding: previously only + # `-i` matched). + "sed --in-place 's/a/b/' " + _LEDGER_REL_PATH, # cp/mv/rm/truncate/dd - "rm .claude/metrics/boundary-events.jsonl", - "mv .claude/metrics/boundary-events.jsonl /tmp/moved.jsonl", - "cp .claude/metrics/boundary-events.jsonl /tmp/copy.jsonl", - "truncate -s 0 .claude/metrics/boundary-events.jsonl", - "dd if=/dev/null of=.claude/metrics/boundary-events.jsonl", + "rm " + _LEDGER_REL_PATH, + "mv " + _LEDGER_REL_PATH + " /tmp/moved.jsonl", + "cp " + _LEDGER_REL_PATH + " /tmp/copy.jsonl", + "truncate -s 0 " + _LEDGER_REL_PATH, + "dd if=/dev/null of=" + _LEDGER_REL_PATH, # python3 -c with a write/append-mode open() - "python3 -c \"open('.claude/metrics/boundary-events.jsonl', 'w').write('{}')\"", - "python3 -c \"open('.claude/metrics/boundary-events.jsonl', 'a').write('{}')\"", + "python3 -c \"open('" + _LEDGER_REL_PATH + "', 'w').write('{}')\"", + "python3 -c \"open('" + _LEDGER_REL_PATH + "', 'a').write('{}')\"", + # open() in read+write mode ("r+") — review finding: previously a + # false negative, since only w/a/x-prefixed modes matched despite + # "r+" permitting writes. + "python3 -c \"open('" + _LEDGER_REL_PATH + "', 'r+').write('{}')\"", + # open() with the keyword form of mode= — review finding: + # previously a false negative, since the pattern only matched a + # positional mode argument. + "python3 -c \"open('" + _LEDGER_REL_PATH + "', mode='a').write('{}')\"", + # `~/`-prefixed path — review finding: previously a false + # negative, since the prefix character class excluded `~`. + "echo '{}' >> ~/" + _LEDGER_REL_PATH, + # Quoted path expanding an env var, with characters ($, {, }) + # previously excluded from the prefix character class (review + # finding). + 'echo \'{}\' >> "$CLAUDE_PROJECT_DIR/' + _LEDGER_REL_PATH + '"', + # Quoted path containing a space in a directory component (review + # finding: previously excluded from the prefix character class). + 'echo \'{}\' >> "My Notes/' + _LEDGER_REL_PATH + '"', ], ) def test_bash_command_writes_to_ledger_true_for_write_shaped_commands(command): @@ -225,14 +355,14 @@ def test_bash_command_writes_to_ledger_true_for_write_shaped_commands(command): @pytest.mark.parametrize( "command", [ - "tail -20 .claude/metrics/boundary-events.jsonl", - "cat .claude/metrics/boundary-events.jsonl", - "grep foo .claude/metrics/boundary-events.jsonl", - "head .claude/metrics/boundary-events.jsonl", + "tail -20 " + _LEDGER_REL_PATH, + "cat " + _LEDGER_REL_PATH, + "grep foo " + _LEDGER_REL_PATH, + "head " + _LEDGER_REL_PATH, # read-mode (mode omitted, defaults to "r") open() - "python3 -c \"print(open('.claude/metrics/boundary-events.jsonl').read())\"", + "python3 -c \"print(open('" + _LEDGER_REL_PATH + "').read())\"", # reads the ledger, writes elsewhere — tee's own target is not the ledger - "cat .claude/metrics/boundary-events.jsonl | tee /tmp/copy.jsonl", + "cat " + _LEDGER_REL_PATH + " | tee /tmp/copy.jsonl", # write-shaped, but targets an unrelated file "echo '{}' >> .claude/metrics/session-digest.jsonl", "", @@ -252,9 +382,7 @@ def test_main_blocks_bash_redirect_to_ledger(monkeypatch, tmp_path, capsys): monkeypatch, { "tool_name": "Bash", - "tool_input": { - "command": "echo '{}' >> .claude/metrics/boundary-events.jsonl" - }, + "tool_input": {"command": "echo '{}' >> " + _LEDGER_REL_PATH}, "cwd": str(tmp_path), }, ) @@ -262,6 +390,8 @@ def test_main_blocks_bash_redirect_to_ledger(monkeypatch, tmp_path, capsys): out = capsys.readouterr().out assert "BLOCKED" in out assert "hooks/lib/boundary_events.py" in out + assert "--event" in out + assert "--subject-hash" in out # Bash-path message names the CLI, not the Python-only function # (plan-review-ux finding, Step 1.2). assert "emit_boundary_event()" not in out @@ -272,7 +402,7 @@ def test_main_allows_bash_read_of_ledger(monkeypatch, tmp_path, capsys): monkeypatch, { "tool_name": "Bash", - "tool_input": {"command": "tail -20 .claude/metrics/boundary-events.jsonl"}, + "tool_input": {"command": "tail -20 " + _LEDGER_REL_PATH}, "cwd": str(tmp_path), }, ) @@ -280,6 +410,7 @@ def test_main_allows_bash_read_of_ledger(monkeypatch, tmp_path, capsys): assert capsys.readouterr().out == "" -def test_main_allows_bash_command_with_no_command_field(monkeypatch, tmp_path): +def test_main_allows_bash_command_with_no_command_field(monkeypatch, tmp_path, capsys): _stdin(monkeypatch, {"tool_name": "Bash", "tool_input": {}, "cwd": str(tmp_path)}) assert guard.main() == 0 + assert capsys.readouterr().out == "" diff --git a/tests/hooks/test_boundary_events_write_guard.py b/tests/hooks/test_boundary_events_write_guard.py index 460123691..9c44085c6 100644 --- a/tests/hooks/test_boundary_events_write_guard.py +++ b/tests/hooks/test_boundary_events_write_guard.py @@ -27,12 +27,21 @@ _HOOK_PY = _REPO_ROOT / "plugins" / "dev-team" / "hooks" / "boundary_events_write_guard.py" +# Test-file-local constant (not `boundary_events_write_guard._LEDGER_NAME` — +# importing the code under test's own constant would make the test oracle +# circular against the code under test). +_LEDGER_REL_PATH = ".claude/metrics/boundary-events.jsonl" -def _run(payload: dict) -> subprocess.CompletedProcess: + +def _run_raw(input_bytes: bytes) -> subprocess.CompletedProcess: + """Lower-level subprocess wiring shared by every test here — `_run()` + is the payload-shaped convenience wrapper; a test exercising a + non-JSON/malformed stdin body calls this directly instead of + hand-rolling its own `subprocess.run()`.""" env = {"PATH": os.environ.get("PATH", "/usr/bin:/bin"), "LANG": "C.UTF-8"} return subprocess.run( [sys.executable, str(_HOOK_PY)], - input=json.dumps(payload).encode(), + input=input_bytes, env=env, capture_output=True, timeout=10, @@ -40,6 +49,10 @@ def _run(payload: dict) -> subprocess.CompletedProcess: ) +def _run(payload: dict) -> subprocess.CompletedProcess: + return _run_raw(json.dumps(payload).encode()) + + def _read_jsonl(path: Path) -> list: if not path.is_file(): return [] @@ -47,6 +60,26 @@ def _read_jsonl(path: Path) -> list: return [json.loads(ln) for ln in lines] +def assert_boundary_event( + event: dict, + *, + hook: str, + tool: str, + decision: str, + matched_rule: str, + session_id: str | None = None, +) -> None: + """Shared assertion for a single recorded boundary-events row — used in + place of the duplicated 5-field inline assertion block at each of this + file's two "blocked and records its own event" tests.""" + assert event["hook"] == hook + assert event["tool"] == tool + assert event["decision"] == decision + assert event["matched_rule"] == matched_rule + if session_id is not None: + assert event["session_id"] == session_id + + def test_write_to_ledger_is_blocked_and_records_its_own_event(tmp_path: Path) -> None: ledger = tmp_path / ".claude" / "metrics" / "boundary-events.jsonl" @@ -54,7 +87,7 @@ def test_write_to_ledger_is_blocked_and_records_its_own_event(tmp_path: Path) -> { "tool_name": "Write", "tool_input": { - "file_path": ".claude/metrics/boundary-events.jsonl", + "file_path": _LEDGER_REL_PATH, "content": '{"forged": true}\n', }, "cwd": str(tmp_path), @@ -68,12 +101,14 @@ def test_write_to_ledger_is_blocked_and_records_its_own_event(tmp_path: Path) -> events = _read_jsonl(ledger) assert len(events) == 1 - event = events[0] - assert event["hook"] == "boundary_events_write_guard" - assert event["tool"] == "Write" - assert event["decision"] == "block" - assert event["matched_rule"] == "ledger-write-blocked" - assert event["session_id"] == "sess-1" + assert_boundary_event( + events[0], + hook="boundary_events_write_guard", + tool="Write", + decision="block", + matched_rule="ledger-write-blocked", + session_id="sess-1", + ) def test_edit_to_ledger_is_blocked(tmp_path: Path) -> None: @@ -81,7 +116,7 @@ def test_edit_to_ledger_is_blocked(tmp_path: Path) -> None: { "tool_name": "Edit", "tool_input": { - "file_path": ".claude/metrics/boundary-events.jsonl", + "file_path": _LEDGER_REL_PATH, "old_string": "a", "new_string": "b", }, @@ -143,7 +178,7 @@ def test_dot_slash_prefixed_path_to_ledger_is_blocked(tmp_path: Path) -> None: { "tool_name": "Write", "tool_input": { - "file_path": "./.claude/metrics/boundary-events.jsonl", + "file_path": "./" + _LEDGER_REL_PATH, "content": "{}\n", }, "cwd": str(tmp_path), @@ -160,14 +195,7 @@ def test_missing_tool_input_is_silent_pass(tmp_path: Path) -> None: def test_malformed_stdin_is_silent_pass() -> None: - result = subprocess.run( - [sys.executable, str(_HOOK_PY)], - input=b"not json", - env={"PATH": os.environ.get("PATH", "/usr/bin:/bin")}, - capture_output=True, - timeout=10, - check=False, - ) + result = _run_raw(b"not json") assert result.returncode == 0 assert result.stdout == b"" @@ -185,9 +213,7 @@ def test_bash_redirect_to_ledger_is_blocked_and_records_its_own_event( result = _run( { "tool_name": "Bash", - "tool_input": { - "command": "echo '{}' >> .claude/metrics/boundary-events.jsonl" - }, + "tool_input": {"command": "echo '{}' >> " + _LEDGER_REL_PATH}, "cwd": str(tmp_path), "session_id": "sess-2", } @@ -196,18 +222,26 @@ def test_bash_redirect_to_ledger_is_blocked_and_records_its_own_event( assert result.returncode == 2 assert b"BLOCKED" in result.stdout assert b"hooks/lib/boundary_events.py" in result.stdout + # Message names the CLI's actual invocable shape — a flag-based + # `--event` drawn from a closed vocabulary plus `--subject-hash`, not + # the unusable positional form the message previously printed (review + # finding, Step 1.2 correction). + assert b"--event" in result.stdout + assert b"--subject-hash" in result.stdout # Bash-path message names the CLI, not the Python-only function # (plan-review-ux finding, Step 1.2). assert b"emit_boundary_event()" not in result.stdout events = _read_jsonl(ledger) assert len(events) == 1 - event = events[0] - assert event["hook"] == "boundary_events_write_guard" - assert event["tool"] == "Bash" - assert event["decision"] == "block" - assert event["matched_rule"] == "ledger-write-blocked" - assert event["session_id"] == "sess-2" + assert_boundary_event( + events[0], + hook="boundary_events_write_guard", + tool="Bash", + decision="block", + matched_rule="ledger-write-blocked", + session_id="sess-2", + ) def test_bash_heredoc_with_trailing_redirect_to_ledger_is_blocked( @@ -217,7 +251,7 @@ def test_bash_heredoc_with_trailing_redirect_to_ledger_is_blocked( { "tool_name": "Bash", "tool_input": { - "command": "cat <<'EOF' >> .claude/metrics/boundary-events.jsonl\n" + "command": "cat <<'EOF' >> " + _LEDGER_REL_PATH + "\n" '{"forged": true}\n' "EOF" }, @@ -233,9 +267,7 @@ def test_bash_tee_to_ledger_is_blocked(tmp_path: Path) -> None: result = _run( { "tool_name": "Bash", - "tool_input": { - "command": "echo '{}' | tee -a .claude/metrics/boundary-events.jsonl" - }, + "tool_input": {"command": "echo '{}' | tee -a " + _LEDGER_REL_PATH}, "cwd": str(tmp_path), } ) @@ -247,10 +279,10 @@ def test_bash_tee_to_ledger_is_blocked(tmp_path: Path) -> None: "command", [ # relative - "echo '{}' >> .claude/metrics/boundary-events.jsonl", + "echo '{}' >> " + _LEDGER_REL_PATH, # absolute (constructed per-test below instead, see next test) # "./"-prefixed - "echo '{}' >> ./.claude/metrics/boundary-events.jsonl", + "echo '{}' >> ./" + _LEDGER_REL_PATH, # bare filename after a `cd .claude/metrics`-shaped prefix "cd .claude/metrics && echo '{}' >> boundary-events.jsonl", ], @@ -285,10 +317,10 @@ def test_bash_write_blocked_for_absolute_path_form(tmp_path: Path) -> None: @pytest.mark.parametrize( "command", [ - "tail -20 .claude/metrics/boundary-events.jsonl", - "cat .claude/metrics/boundary-events.jsonl", - "grep foo .claude/metrics/boundary-events.jsonl", - "python3 -c \"print(open('.claude/metrics/boundary-events.jsonl').read())\"", + "tail -20 " + _LEDGER_REL_PATH, + "cat " + _LEDGER_REL_PATH, + "grep foo " + _LEDGER_REL_PATH, + "python3 -c \"print(open('" + _LEDGER_REL_PATH + "').read())\"", ], ) def test_bash_reads_of_ledger_are_allowed(tmp_path: Path, command: str) -> None: From e00aba02b199448cdeb6332c33c3b194d70619e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:39:20 +0000 Subject: [PATCH 05/10] fix(hooks): close legacy-path and ReDoS gaps in the boundary-events guard Opus-tier checkpoint on #2171's guard hook (security/domain/arch-review): targets_ledger() now also blocks the pre-migration legacy path (/metrics/boundary-events.jsonl), which emit_boundary_event()'s default migrate=True would otherwise silently promote into ledger history from a forged Write/Edit. bash_command_writes_to_ledger() gains an O(n) literal fast-path ahead of the write-shape regexes to remove a quadratic-backtracking hang/bypass on long non-matching commands. Dedupes the ledger stream name/path resolution onto hooks/lib/review_dispatch_ledger.py instead of re-declaring it locally, adds the missing telemetry-schema.md registration, mirrors block messages to stderr per the documented exit-2 hook contract, and drives the Bash remedy's --event vocabulary from a new boundary_events.cli_event_names() accessor instead of a hand-copied literal. Part of #2164 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .../hooks/boundary_events_write_guard.py | 88 ++++++++++++++----- plugins/dev-team/hooks/lib/boundary_events.py | 13 ++- .../dev-team/knowledge/telemetry-schema.md | 4 +- .../hooks/test_boundary_events_write_guard.py | 40 +++++++++ 4 files changed, 122 insertions(+), 23 deletions(-) diff --git a/plugins/dev-team/hooks/boundary_events_write_guard.py b/plugins/dev-team/hooks/boundary_events_write_guard.py index 8979983e4..08060e177 100755 --- a/plugins/dev-team/hooks/boundary_events_write_guard.py +++ b/plugins/dev-team/hooks/boundary_events_write_guard.py @@ -31,10 +31,13 @@ command. The Bash-path message instead names the `hooks/lib/boundary_events.py` CLI's actual invocable shape (`python3 plugins/dev-team/hooks/lib/boundary_events.py --event - - --subject-hash ...`, #1461) — `--event` is a flag drawn - from a closed `choices` set, not a positional, and most events - require `--subject-hash`; the CLI cannot construct an + --subject-hash ...`, #1461) — the message + interpolates the live `` choice set from + `boundary_events.cli_event_names()` rather than restating it + here, so this docstring never drifts the way the message + itself used to (review finding, #2171). `--event` is a flag + drawn from a closed `choices` set, not a positional, and most + events require `--subject-hash`; the CLI cannot construct an arbitrary row by design — a genuinely custom row still needs `emit_boundary_event()` called from a hook (plan-review-ux finding, Step 1.2, corrected by review). @@ -70,11 +73,12 @@ sys.path.insert(0, str(_LIB_DIR)) import artifact_paths +from boundary_events import cli_event_names as _cli_event_names from boundary_events import emit_boundary_event as _emit_boundary_event +from review_dispatch_ledger import LEDGER_STREAM as _LEDGER_NAME +from review_dispatch_ledger import resolve_stream as _resolve_ledger_stream from stdin_json import read_stdin_json # type: ignore[import-not-found] -_LEDGER_NAME = "boundary-events.jsonl" - def emit_boundary_event(*args, **kwargs) -> None: """Local safety net (#859): even a misbehaving helper must never affect @@ -104,7 +108,17 @@ def targets_ledger(file_path: str, cwd: str) -> bool: the same on-disk path `emit_boundary_event()` itself resolves and writes to — `artifact_paths.resolve_file("metrics", ...)` under the repo root, not a bare `.claude/metrics/` prefix match (so - `review-verdicts.jsonl`, Slice 2's own store, is unaffected). + `review-verdicts.jsonl`, Slice 2's own store, is unaffected) — OR the + pre-migration legacy path `/metrics/boundary-events.jsonl` + (review finding, #2171): `resolve_file(..., migrate=True)`, the default + every `emit_boundary_event()` call uses, `shutil.move`s an untracked + file at that legacy path into the ledger the *next* time anything + emits, whenever the new-location file does not yet exist. Matching + only the new-location path would let a Write/Edit plant a forged file + at the legacy path — invisible to this guard — that a later, entirely + legitimate `emit_boundary_event()` call then silently promotes into + ledger history. Both candidates must be blocked for the guard's + forgery-cost claim to hold. The `cwd` anchor is realpath'd before the join (review finding): the ledger side already resolves symlinks transparently, because @@ -126,12 +140,22 @@ def targets_ledger(file_path: str, cwd: str) -> bool: candidate = base / candidate candidate_norm = os.path.abspath(str(candidate)) - ledger = artifact_paths.resolve_file( - "metrics", _LEDGER_NAME, root=cwd, migrate=False - ) + # Cheap lexical pre-check (performance review finding, #2171): every + # candidate this function can match ends in `_LEDGER_NAME` — skip the + # `git rev-parse` subprocess `resolve_stream()`/`project_root()` incur + # for the overwhelming majority of Write/Edit calls that plainly don't. + if os.path.basename(candidate_norm) != _LEDGER_NAME: + return False + + ledger = _resolve_ledger_stream("metrics", _LEDGER_NAME, Path(cwd) if cwd else Path.cwd()) ledger_norm = os.path.abspath(str(ledger)) + if candidate_norm == ledger_norm: + return True + + legacy_ledger = artifact_paths.project_root(start=cwd) / "metrics" / _LEDGER_NAME + legacy_norm = os.path.abspath(str(legacy_ledger)) - return candidate_norm == ledger_norm + return candidate_norm == legacy_norm def _extract_command(tool_input: object) -> str: @@ -206,8 +230,17 @@ def bash_command_writes_to_ledger(command: str) -> bool: filename, in any path form — see `_BASH_WRITE_SHAPE_PATTERNS`. A read-shaped command referencing the same filename (`cat`, `grep`, `tail`, `head`, a read-mode `open()`) never matches any pattern here, - so it is allowed without a separate read-allowlist check.""" - if not command: + so it is allowed without a separate read-allowlist check. + + O(n) fast path first (security review finding, #2171): every pattern in + `_BASH_WRITE_SHAPE_PATTERNS` requires the literal `_LEDGER_NAME` + substring, so a command that lacks it cannot match any of them — this + is semantically equivalent to running the patterns, not a heuristic + shortcut. Skipping straight to `False` on a long non-matching command + (e.g. a `rm` of padding data) avoids the patterns' overlapping + `[^;|&\\n]*`/path-suffix character classes backtracking quadratically + on input that was never going to match.""" + if not command or _LEDGER_NAME not in command: return False return any(pattern.search(command) for pattern in _BASH_WRITE_SHAPE_PATTERNS) @@ -221,16 +254,30 @@ def _block( ) -> int: """Shared block sequence for both `main()` branches: record the guard's own decision (every sibling guard does — see the module docstring), - print the block explanation, and return the block exit code.""" + print the block explanation, and return the block exit code. + + Mirrors every line to stderr in addition to stdout (docs/python-hook- + contract.md § stderr, "Exception — exit-2 (block) messages"): some + Claude Code hook-error wrappers surface only stderr on a nonzero hook + exit, so a stdout-only block message can go unseen there. Stdout stays + the canonical channel; stderr is additive duplication for this exit-2 + path only — this is a new hook, so it converges to the documented + standard from the start rather than joining the stdout-only legacy list.""" emit_boundary_event( cwd, "boundary_events_write_guard", tool, "block", "ledger-write-blocked", session_id ) - print(blocked_message) - print( - "This file is the boundary-events accountability ledger (#859) — " - "it is append-only from the session's perspective." + lines = ( + blocked_message, + ( + "This file is the boundary-events accountability ledger (#859) — " + "it is append-only from the session's perspective." + ), + remedy_message, ) - print(remedy_message) + for line in lines: + print(line) + for line in lines: + print(line, file=sys.stderr) return 2 @@ -239,6 +286,7 @@ def _handle_bash_tool(payload: dict, cwd: str, session_id: str | None) -> int: if not bash_command_writes_to_ledger(command): return 0 + events = "|".join(_cli_event_names()) return _block( cwd, "Bash", @@ -246,7 +294,7 @@ def _handle_bash_tool(payload: dict, cwd: str, session_id: str | None) -> int: f"BLOCKED: This Bash command writes to '.claude/metrics/{_LEDGER_NAME}', " "which is not allowed.", "Use 'python3 plugins/dev-team/hooks/lib/boundary_events.py " - "--event " + f"--event <{events}> " "--subject-hash ...' instead of writing to it from Bash — " "an arbitrary row isn't CLI-constructible by design (that CLI only " "accepts a closed --event vocabulary); this exact row needs a " diff --git a/plugins/dev-team/hooks/lib/boundary_events.py b/plugins/dev-team/hooks/lib/boundary_events.py index 2ad031111..9bef08cd2 100644 --- a/plugins/dev-team/hooks/lib/boundary_events.py +++ b/plugins/dev-team/hooks/lib/boundary_events.py @@ -248,6 +248,17 @@ def emit_boundary_event( } +def cli_event_names() -> list[str]: + """The full, sorted `--event` choice set this CLI accepts — the same + union `_main()`'s own `argparse` `choices=` computes. Public so a + caller that needs to *describe* the CLI's invocable shape (e.g. + `boundary_events_write_guard.py`'s Bash-path remedy message, #2171) + reads this one source of truth instead of re-enumerating the three + closed-vocabulary dicts above by hand, which would silently go stale + the next time an event is added to any of them.""" + return sorted({*_CLI_EVENTS, *_CLI_AGENT_EVENTS, *_CLI_VERDICT_EVENTS}) + + def _main() -> int: """CLI entry point (#1461): lets a *skill's* bash-block prose emit one of a small, fixed set of exemption events, the same way @@ -315,7 +326,7 @@ def _main() -> int: parser.add_argument( "--event", required=True, - choices=sorted({*_CLI_EVENTS, *_CLI_AGENT_EVENTS, *_CLI_VERDICT_EVENTS}), + choices=cli_event_names(), ) # Required for every event EXCEPT `gate-ran` (#2037), which has no diff # content to bind to — a real git hook has no staged/branch diff to hash diff --git a/plugins/dev-team/knowledge/telemetry-schema.md b/plugins/dev-team/knowledge/telemetry-schema.md index 848620d52..0f8dc1a5d 100644 --- a/plugins/dev-team/knowledge/telemetry-schema.md +++ b/plugins/dev-team/knowledge/telemetry-schema.md @@ -79,7 +79,7 @@ common `clean` case and the unexplainable `unreadable` case write nothing. | `hook` | string | Emitting hook's module name, e.g. `destructive_guard`, `verify_guard`, `pre_pr_review` (the review-corroboration gate, #1886; `pre_commit_review` is now a documented no-op and emits nothing), `telemetry`, `agent_dispatch_ledger` — or `code-review` for the CLI-emitted events (`--event doc-only`/`single-agent`/`dispatch-failure`), which carry the invoking skill's name rather than a hook module name | | `tool` | string | Hooked tool/event: `Bash`, `Write`, `Edit`, `Skill`, `Agent`, `UserPromptSubmit`, `SubagentStop` (#2188) | | `decision` | string enum | `block` \| `warn` \| `bypass` \| `intervention` \| `revert` \| `record` \| `dispatch-failure` | -| `matched_rule` | string | Rule ID from a closed vocabulary (pattern ID, hook-defined constant, bypass flag name, intervention keyword, or — for `record`/`dispatch-failure` — the dispatched review-agent's registered name, or — for `subagent_completion_guard.py`'s `record` rows — `empty-final-turn`/`truncated-final-turn`, #2188) — never free text | +| `matched_rule` | string | Rule ID from a closed vocabulary (pattern ID, hook-defined constant, bypass flag name, intervention keyword, or — for `record`/`dispatch-failure` — the dispatched review-agent's registered name, or — for `subagent_completion_guard.py`'s `record` rows — `empty-final-turn`/`truncated-final-turn`, #2188, or — for `boundary_events_write_guard.py`'s `block` rows — `ledger-write-blocked`, #2171) — never free text | | `plugin_version` | string | From `.claude-plugin/plugin.json` | | `session_id` | string, optional | Opaque per-session ID, when present in the hook payload — enables joins with `session-digest.jsonl` | | `subject_hash` | string, optional | `review_gate_hash()` value (#1461) binding this event to the staged content it corroborates. A hex digest, not free text | @@ -96,7 +96,7 @@ PR-creation time, against the branch's cumulative diff, does not have that problem. `hooks/pre_pr_review.py` never emits this event. Existing rows in `boundary-events.jsonl` from before the migration remain valid history. -- **Emitter:** `hooks/lib/boundary_events.py::emit_boundary_event()`, called from `destructive_guard.py`, `verify_guard.py`, `pre_pr_review.py` (#1886), `telemetry.py` (intervention keywords), `agent_dispatch_ledger.py` (decision `record`, #1461), `subagent_completion_guard.py` (decision `record`, `tool` `SubagentStop`, #2188), the mechanically-adopted guards (`pre_tool_guard.py`, `context_ceiling_guard.py`, `bash_retry_guard.py`, `refactor_test_freeze_guard.py`, `refactor_test_bash_guard.py`, `refactor_test_revert_guard.py` (decision `revert`, #906), `contract_version_guard.py`, `mutation_testing_smoke_gate.py`, `mutation_gate.py`, `tdd_guard.py`), and `boundary_events.py`'s own CLI (`--event dispatch-failure`, decision `dispatch-failure`, #1763) invoked from `skills/code-review/SKILL.md` Step 4. `--event gate-ran --verdict {allow,block,errored}` (decision `record`, `matched_rule` of `gate-ran-`, #2037) is invoked from the repo-root `.husky/pre-commit` git hook — the real, git-native pre-commit gate (distinct from `pre_pr_review.py`, a Claude-Code-level PreToolUse hook gating `gh pr create`) — at every exit point, success or failure alike, so `${CLAUDE_PLUGIN_ROOT}/scripts/session_report.py --profile maintainer` can correlate a commit-attempt Bash record against a nearby `gate_ran` event and classify the previously-unmeasured "the gate silently never ran" population (`gate_ran_absent`) apart from a genuine internal failure (`gate_ran_errored`). This event carries no `session_id` in practice — a real git hook has no Claude Code session_id to attach — so correlation is by time proximity, not session join; see `session_report.py`'s "gate-run correlation (#2037)" section. +- **Emitter:** `hooks/lib/boundary_events.py::emit_boundary_event()`, called from `destructive_guard.py`, `verify_guard.py`, `pre_pr_review.py` (#1886), `telemetry.py` (intervention keywords), `agent_dispatch_ledger.py` (decision `record`, #1461), `subagent_completion_guard.py` (decision `record`, `tool` `SubagentStop`, #2188), `boundary_events_write_guard.py` (decision `block`, `tool` `Write`\|`Edit`\|`Bash`, `matched_rule` `ledger-write-blocked` — the PreToolUse guard blocking a direct Write/Edit/Bash write to this same ledger, #2171), the mechanically-adopted guards (`pre_tool_guard.py`, `context_ceiling_guard.py`, `bash_retry_guard.py`, `refactor_test_freeze_guard.py`, `refactor_test_bash_guard.py`, `refactor_test_revert_guard.py` (decision `revert`, #906), `contract_version_guard.py`, `mutation_testing_smoke_gate.py`, `mutation_gate.py`, `tdd_guard.py`), and `boundary_events.py`'s own CLI (`--event dispatch-failure`, decision `dispatch-failure`, #1763) invoked from `skills/code-review/SKILL.md` Step 4. `--event gate-ran --verdict {allow,block,errored}` (decision `record`, `matched_rule` of `gate-ran-`, #2037) is invoked from the repo-root `.husky/pre-commit` git hook — the real, git-native pre-commit gate (distinct from `pre_pr_review.py`, a Claude-Code-level PreToolUse hook gating `gh pr create`) — at every exit point, success or failure alike, so `${CLAUDE_PLUGIN_ROOT}/scripts/session_report.py --profile maintainer` can correlate a commit-attempt Bash record against a nearby `gate_ran` event and classify the previously-unmeasured "the gate silently never ran" population (`gate_ran_absent`) apart from a genuine internal failure (`gate_ran_errored`). This event carries no `session_id` in practice — a real git hook has no Claude Code session_id to attach — so correlation is by time proximity, not session join; see `session_report.py`'s "gate-run correlation (#2037)" section. - **Consent:** ALWAYS-ON — not gated by `DEV_TEAM_TELEMETRY`. Local-only, rule-IDs-only safety/accountability channel; no observability holes by design. - **Fail-open:** every exception in the emit helper is swallowed — never changes the calling hook's exit code, stdout, or stderr. - **Consumers:** `skills/session-review/SKILL.md`, `skills/harness-audit/SKILL.md`, `agents/session-analysis.md`, `skills/cost-report/`, `skills/run-report/SKILL.md` (#1167), `hooks/lib/review_gate_corroboration.py` (#1461 `record` rows; #1763 also reads `dispatch-failure` rows as negative evidence for the gate veto), future `agent-telemetry` cross-machine aggregation (#178). diff --git a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py index 59c1e319e..fab0d6618 100644 --- a/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py +++ b/plugins/dev-team/tests/hooks/test_boundary_events_write_guard.py @@ -15,6 +15,7 @@ import os import subprocess import sys +import time import pytest @@ -138,6 +139,25 @@ def test_targets_ledger_true_for_relative_path_via_symlinked_cwd(tmp_path): assert guard.targets_ledger(_LEDGER_REL_PATH, str(symlinked_cwd)) +def test_targets_ledger_true_for_legacy_pre_migration_path(tmp_path): + """Domain-review finding (#2171): `emit_boundary_event()` resolves the + ledger with `resolve_file(..., migrate=True)` (the writer default), + which `shutil.move`s an untracked `/metrics/ + boundary-events.jsonl` into `.claude/metrics/boundary-events.jsonl` + the next time anything emits, whenever the new-location file does not + yet exist. Matching only the new location would let a Write/Edit plant + a forged file at the legacy path — unguarded — that a later, + legitimate emit then silently promotes into ledger history. Both + locations must be blocked.""" + legacy = str(tmp_path / "metrics" / "boundary-events.jsonl") + assert guard.targets_ledger(legacy, str(tmp_path)) + + +def test_targets_ledger_false_for_legacy_unrelated_file(tmp_path): + legacy_unrelated = str(tmp_path / "metrics" / "session-digest.jsonl") + assert not guard.targets_ledger(legacy_unrelated, str(tmp_path)) + + # --------------------------------------------------------------------------- # main() — in-process, stdin-monkeypatched (Write and Edit tool shapes) # --------------------------------------------------------------------------- @@ -372,6 +392,26 @@ def test_bash_command_writes_to_ledger_false_for_read_or_unrelated_commands(comm assert guard.bash_command_writes_to_ledger(command) is False +def test_bash_command_writes_to_ledger_fast_path_on_long_non_matching_command(): + """Security-review finding (#2171): the write-shape patterns' `[^;|&\\n]*` + classes overlap with the path-suffix class, giving a long non-matching + command quadratic backtracking — a plausible hang/bypass (a padded `rm` + ahead of the real write could stall the scan past a timeout). Every + pattern requires the literal `_LEDGER_NAME` substring, so an `in` + fast-path is semantically equivalent and turns this from O(n^2) into + O(n). Bounded timing assertion (generous — this is a regression guard, + not a benchmark) proves the fast path is actually taken.""" + long_command = "rm " + ("a" * 200_000) + " ; echo done" + assert guard._LEDGER_NAME not in long_command + + start = time.monotonic() + result = guard.bash_command_writes_to_ledger(long_command) + elapsed = time.monotonic() - start + + assert result is False + assert elapsed < 1.0 + + # --------------------------------------------------------------------------- # main() — Bash tool shape (Step 1.2) # --------------------------------------------------------------------------- From 61516e6507e9ce572e94185707aec34b554873fd Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:42:43 +0000 Subject: [PATCH 06/10] feat(code-review): declare target-file scope in each agent dispatch prompt Add a structured "Files in scope for this review: , ..." marker to skills/code-review/SKILL.md step 4's dispatch-prompt construction, so a future verdict recorder (Step 2.3, #2166) can recover which files an agent was asked to review from its own transcript. No behavior change to what gets reviewed or reported. Adds hooks/lib/review_verdicts.py with just SCOPE_MARKER_PREFIX, the shared constant this step's test and Step 2.3's future parser both import, so the marker's literal format can't silently drift between renderer and parser. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- plugins/dev-team/hooks/lib/review_verdicts.py | 18 +++++ plugins/dev-team/skills/code-review/SKILL.md | 1 + .../test_code_review_dispatch_marker.py | 78 +++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 plugins/dev-team/hooks/lib/review_verdicts.py create mode 100644 plugins/dev-team/tests/skills/test_code_review_dispatch_marker.py diff --git a/plugins/dev-team/hooks/lib/review_verdicts.py b/plugins/dev-team/hooks/lib/review_verdicts.py new file mode 100644 index 000000000..17e9942d9 --- /dev/null +++ b/plugins/dev-team/hooks/lib/review_verdicts.py @@ -0,0 +1,18 @@ +"""review_verdicts.py — per-lens review verdict store (#2166). + +Step 2.1 scope: this module currently holds only `SCOPE_MARKER_PREFIX`, the +literal text prefix `skills/code-review/SKILL.md` step 4 renders into each +per-agent dispatch prompt (`Files in scope for this review: , ...`) +and that Step 2.3's SubagentStop verdict recorder will parse back out of the +transcript. Sharing one constant between the renderer's test (this step) and +the future parser (Step 2.3) keeps the marker format and its parser from +silently drifting apart. + +Step 2.2 adds `emit_review_verdict()`/`load_verdicts()` (the writer/reader +for `.claude/metrics/review-verdicts.jsonl`) to this module. Nothing else +belongs here yet. +""" + +from __future__ import annotations + +SCOPE_MARKER_PREFIX = "Files in scope for this review: " diff --git a/plugins/dev-team/skills/code-review/SKILL.md b/plugins/dev-team/skills/code-review/SKILL.md index a0fe35a26..5b005be08 100644 --- a/plugins/dev-team/skills/code-review/SKILL.md +++ b/plugins/dev-team/skills/code-review/SKILL.md @@ -496,6 +496,7 @@ Prints a manifest naming the pack path, its byte size, and `files_omitted`. Pass - **`files_omitted` is not optional to relay.** When the manifest reports omissions (a file over the per-file cap, a binary, a body that would exhaust the budget), name those paths in each agent's prompt and tell it to open them directly. The pack body says so too, but a silently skipped file is a coverage hole that reads as a clean review. - **File scope**: pass only files matching each agent's declared scope. Skip the agent if no files match. +- **Scope marker (#2166)**: append one structured, single-line marker to every dispatch prompt, listing the exact files passed under File scope above, comma-separated: `Files in scope for this review: , , ...`. This is metadata for the (not-yet-built) verdict recorder — it changes nothing about what gets reviewed or reported this slice. - **Context payload** (controlled by the agent's `Context needs`): - `diff-only` → diff output only (for auto-scope or `--since` only) - `full-file` → complete files diff --git a/plugins/dev-team/tests/skills/test_code_review_dispatch_marker.py b/plugins/dev-team/tests/skills/test_code_review_dispatch_marker.py new file mode 100644 index 000000000..56c577324 --- /dev/null +++ b/plugins/dev-team/tests/skills/test_code_review_dispatch_marker.py @@ -0,0 +1,78 @@ +"""Content checks for code-review/SKILL.md step 4's dispatch-prompt scope +marker (#2166 Step 2.1). +""" + +from __future__ import annotations + +import sys + +from _repo_root import REPO_ROOT + +_LIB_DIR = REPO_ROOT / "plugins" / "dev-team" / "hooks" / "lib" +if str(_LIB_DIR) not in sys.path: + sys.path.insert(0, str(_LIB_DIR)) + +from review_verdicts import SCOPE_MARKER_PREFIX + +SKILL = REPO_ROOT / "plugins" / "dev-team" / "skills" / "code-review" / "SKILL.md" + +_SECTION_MARKER = "### 4. Run each enabled agent" + + +def _dispatch_section() -> str: + text = SKILL.read_text(encoding="utf-8") + assert _SECTION_MARKER in text, "step 4 heading not found" + section = text.split(_SECTION_MARKER, 1)[1].split("\n### ", 1)[0] + return section + + +def test_dispatch_section_declares_scope_marker_template() -> None: + section = _dispatch_section() + assert "Files in scope for this review: , , ..." in section, ( + "step 4 is missing the dispatch-prompt scope marker template" + ) + + +def test_dispatch_section_marker_prose_uses_shared_prefix_constant() -> None: + section = _dispatch_section() + assert SCOPE_MARKER_PREFIX in section, ( + "step 4's marker prose has drifted from review_verdicts.SCOPE_MARKER_PREFIX" + ) + + +def _render_marker_line(files: list[str]) -> str: + """Render a dispatch-prompt marker line the way SKILL.md step 4 describes: + SCOPE_MARKER_PREFIX followed by the comma-separated in-scope file list. + """ + return SCOPE_MARKER_PREFIX + ", ".join(files) + + +def _parse_marker_line(line: str) -> list[str]: + """Local, throwaway extraction for this step's round-trip test only. + + Step 2.3 owns the real parser (reading it out of a subagent transcript); + this is not that parser, just a same-format check that renderer output + is machine-parseable via the shared SCOPE_MARKER_PREFIX constant. + """ + assert line.startswith(SCOPE_MARKER_PREFIX), "line missing scope marker prefix" + remainder = line[len(SCOPE_MARKER_PREFIX) :] + return remainder.split(", ") + + +def test_marker_round_trip_recovers_identical_file_list() -> None: + files = ["src/foo.py", "src/bar/baz.py", "tests/test_foo.py"] + + dispatch_prompt = ( + "Review the following files for correctness issues.\n\n" + f"{_render_marker_line(files)}\n\n" + "Return findings per the standard output contract." + ) + + marker_line = next( + line + for line in dispatch_prompt.splitlines() + if line.startswith(SCOPE_MARKER_PREFIX) + ) + recovered = _parse_marker_line(marker_line) + + assert recovered == files From 94af4f426d2e32721055c8000fd5320612eff32a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:46:17 +0000 Subject: [PATCH 07/10] feat(hooks): add review_verdicts.py per-lens verdict store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends hooks/lib/review_verdicts.py (Step 2.1 landed just SCOPE_MARKER_PREFIX) with emit_review_verdict()/load_verdicts() for .claude/metrics/review-verdicts.jsonl — Step 2.2 of #2166's plan. - emit_review_verdict(): unconditional (no consent gate, Decision 2), fail-open, appends one JSON line via atomic_state.append_line_locked, stamped with plugin_version.shipped_version(). Keyed per (lens, file_path, file_content_hash), not per-diff (Decision 3). - load_verdicts(): returns no usable rows (never raises) for an absent file, a corrupted/non-object line, or a plugin_version older than current. No consumer yet this slice — review_verdict_recorder.py lands in Step 2.3. Tests cover the write round-trip, each reader no-usable-rows case, an unwritable-metrics-dir fail-open case, an explicit consent-off case (DEV_TEAM_TELEMETRY unset + ~/.claude/telemetry.json {"enabled": false}), and a mechanical content-guard confirming load_verdicts has no consumer outside review_verdict_recorder.py and this module's own tests. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- plugins/dev-team/hooks/lib/review_verdicts.py | 177 ++++++++++++- .../tests/hooks/test_review_verdicts.py | 237 ++++++++++++++++++ 2 files changed, 403 insertions(+), 11 deletions(-) create mode 100644 plugins/dev-team/tests/hooks/test_review_verdicts.py diff --git a/plugins/dev-team/hooks/lib/review_verdicts.py b/plugins/dev-team/hooks/lib/review_verdicts.py index 17e9942d9..be7aad24d 100644 --- a/plugins/dev-team/hooks/lib/review_verdicts.py +++ b/plugins/dev-team/hooks/lib/review_verdicts.py @@ -1,18 +1,173 @@ """review_verdicts.py — per-lens review verdict store (#2166). -Step 2.1 scope: this module currently holds only `SCOPE_MARKER_PREFIX`, the -literal text prefix `skills/code-review/SKILL.md` step 4 renders into each -per-agent dispatch prompt (`Files in scope for this review: , ...`) -and that Step 2.3's SubagentStop verdict recorder will parse back out of the -transcript. Sharing one constant between the renderer's test (this step) and -the future parser (Step 2.3) keeps the marker format and its parser from -silently drifting apart. - -Step 2.2 adds `emit_review_verdict()`/`load_verdicts()` (the writer/reader -for `.claude/metrics/review-verdicts.jsonl`) to this module. Nothing else -belongs here yet. +Records, per genuine review-agent dispatch, an outcome (`pass` | `findings`) +bound to `(lens, file_path, file_content_hash)` — NOT per-diff (Decision 3, +plans/2164-verdict-ledger-writer.md): a verdict row answers "did this lens +pass this exact file content", so it can be looked up again the next time the +same file content recurs, regardless of which diff produced it. + +A deliberate sibling of `hooks/lib/boundary_events.py`, not an extension of +it (Decision 1): `boundary_events.py`'s own docstring forbids ever writing a +real `file_path` into that stream ("Never write free text ... file paths ... +must never appear"), so a per-file verdict needs its own store, +`.claude/metrics/review-verdicts.jsonl`, written via the same +`atomic_state.append_line_locked` primitive `boundary_events.py` uses. + +ALWAYS-ON (Decision 2): unlike `telemetry.py`, this stream is not gated by +`DEV_TEAM_TELEMETRY`/`~/.claude/telemetry.json` consent — same posture as +`boundary-events.jsonl` itself, for the same reason (local-only, mechanical +accountability data: lens/path/hash/outcome, no prose). + +Fail-open: every exception in `emit_review_verdict()` is swallowed. A full +disk, read-only `.claude/metrics/`, or malformed state must never change the +calling hook's stdout, stderr, or exit code. `load_verdicts()` never raises +either — an absent file, a corrupted line, or a stale `plugin_version` row +all degrade to "no usable rows" rather than an exception. + +`load_verdicts()` has no consumer in this slice (Step 2.2) — `#2167`'s +`review_verdict_recorder.py` is the first one; see this module's own test +`test_load_verdicts_has_no_other_consumers` for the mechanical check that +enforces that boundary. + +Stdlib only. See ADR 0014 / ADR 0015. """ from __future__ import annotations +import json +import sys +from datetime import datetime, timezone +from pathlib import Path + +_LIB_DIR = Path(__file__).resolve().parent +if str(_LIB_DIR) not in sys.path: + sys.path.insert(0, str(_LIB_DIR)) + +import artifact_paths +import atomic_state +import plugin_version + +_LOG_NAME = "review-verdicts.jsonl" + +# The literal text prefix `skills/code-review/SKILL.md` step 4 renders into +# each per-agent dispatch prompt (`Files in scope for this review: , +# ...`) and that Step 2.3's SubagentStop verdict recorder parses back out of +# the transcript. Sharing one constant between the renderer's test (Step 2.1) +# and the future parser (Step 2.3) keeps the marker format and its parser +# from silently drifting apart. SCOPE_MARKER_PREFIX = "Files in scope for this review: " + + +def _isoformat_utc() -> str: + return datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + + +def emit_review_verdict( + cwd, + lens: str, + file_path: str, + file_content_hash: str, + outcome: str, + session_id: str | None = None, +) -> None: + """Append one compact JSON line to + `/.claude/metrics/review-verdicts.jsonl`. + + Unconditional (Decision 2) — no consent check. Fail-open: any error (bad + `cwd`, unwritable `.claude/metrics/`, disk full, etc.) is swallowed + silently, matching `emit_boundary_event`'s own contract. + + Args: + cwd: Directory whose `.claude/metrics/` subdirectory receives the + row. Accepts `str` or `Path`. + lens: The review agent's name (e.g. "security-review"). + file_path: The in-scope file this verdict is about. + file_content_hash: Content hash of `file_path` at review time. + outcome: `"pass"` or `"findings"`. + session_id: Optional opaque session ID from the hook payload. + """ + try: + base = Path(cwd) if cwd else Path.cwd() + log = artifact_paths.resolve_file("metrics", _LOG_NAME, base) + log.parent.mkdir(parents=True, exist_ok=True) + + payload = { + "ts": _isoformat_utc(), + "lens": lens, + "file_path": file_path, + "file_content_hash": file_content_hash, + "outcome": outcome, + "plugin_version": plugin_version.shipped_version(), + } + if session_id: + payload["session_id"] = session_id + + line = json.dumps(payload, separators=(",", ":")) + "\n" + atomic_state.append_line_locked(log, line) + except Exception: # noqa: BLE001, S110 — fail-open by design, see module docstring + pass + + +def _version_tuple(version: object) -> tuple[int, ...] | None: + """Parse a dotted numeric version string into a comparable tuple, or + `None` when it isn't one (e.g. the `"unknown"` fallback + `plugin_version.shipped_version()` can return).""" + if not isinstance(version, str) or not version: + return None + parts: list[int] = [] + for segment in version.split("."): + if not segment.isdigit(): + return None + parts.append(int(segment)) + return tuple(parts) if parts else None + + +def _is_usable_version(row_version: object, current_version: str) -> bool: + """A row is usable when its `plugin_version` is the current one, or a + numerically parseable version no older than it. Anything else — a + strictly older version, or a value that fails to parse and isn't an + exact string match — is treated as stale/unusable (fail toward + excluding, never toward raising).""" + if row_version == current_version: + return True + row_tuple = _version_tuple(row_version) + current_tuple = _version_tuple(current_version) + if row_tuple is None or current_tuple is None: + return False + return row_tuple >= current_tuple + + +def load_verdicts(cwd) -> list[dict]: + """Read `/.claude/metrics/review-verdicts.jsonl` and return its + usable rows. + + "Usable" excludes: an absent file, a line that isn't valid JSON, a line + that isn't a JSON object, and a row whose `plugin_version` is older than + `plugin_version.shipped_version()`. Never raises — every failure mode + degrades to an empty (or partial) list. + """ + try: + base = Path(cwd) if cwd else Path.cwd() + log = artifact_paths.resolve_file("metrics", _LOG_NAME, base, migrate=False) + if not log.is_file(): + return [] + text = log.read_text(encoding="utf-8") + except Exception: # noqa: BLE001 — fail-open by design, see module docstring + return [] + + current_version = plugin_version.shipped_version() + rows: list[dict] = [] + for raw_line in text.splitlines(): + raw_line = raw_line.strip() + if not raw_line: + continue + try: + row = json.loads(raw_line) + except ValueError: + continue + if not isinstance(row, dict): + continue + if not _is_usable_version(row.get("plugin_version"), current_version): + continue + rows.append(row) + return rows diff --git a/plugins/dev-team/tests/hooks/test_review_verdicts.py b/plugins/dev-team/tests/hooks/test_review_verdicts.py new file mode 100644 index 000000000..a6afc0a48 --- /dev/null +++ b/plugins/dev-team/tests/hooks/test_review_verdicts.py @@ -0,0 +1,237 @@ +"""Unit tests for hooks/lib/review_verdicts.py (#2166, Step 2.2). + +Covers: + - `emit_review_verdict()`: append, dir creation, compact single-line JSON, + optional session_id, fail-open on OSError / arbitrary exceptions. + - Consent posture (Decision 2): writes happen even with + `DEV_TEAM_TELEMETRY` unset and an explicit `{"enabled": false}` in + `~/.claude/telemetry.json`. + - `load_verdicts()`: write round-trip, and its "no usable rows, never an + exception" contract for an absent file, a corrupted line, and a + version-mismatched row. + - A mechanical content-guard: `load_verdicts` has no consumer yet other + than `review_verdict_recorder.py` (not built in this slice) and this + module's own tests. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +from _repo_root import REPO_ROOT as _REPO_ROOT + +_PLUGIN_DIR = _REPO_ROOT / "plugins" / "dev-team" +_HOOKS_DIR = _PLUGIN_DIR / "hooks" +_LIB_DIR = _HOOKS_DIR / "lib" +_TESTS_LIB = _PLUGIN_DIR / "tests" / "lib" + +for _p in (_HOOKS_DIR, _LIB_DIR, _TESTS_LIB): + if str(_p) not in sys.path: + sys.path.insert(0, str(_p)) + +import plugin_version # type: ignore[import-not-found] +import review_verdicts # type: ignore[import-not-found] +import telemetry_consent # type: ignore[import-not-found] + +_LOG_REL = Path(".claude") / "metrics" / "review-verdicts.jsonl" + + +# --------------------------------------------------------------------------- +# emit_review_verdict() — the writer +# --------------------------------------------------------------------------- + + +def test_emit_appends_one_compact_jsonl_line_with_expected_fields( + tmp_path: Path, +) -> None: + review_verdicts.emit_review_verdict( + tmp_path, "security-review", "src/foo.py", "abc123", "pass" + ) + log = tmp_path / _LOG_REL + assert log.is_file() + raw = log.read_text(encoding="utf-8") + lines = raw.splitlines() + assert len(lines) == 1 + assert raw.endswith("\n") + # Compact separators: no space after ',' or ':'. + assert ", " not in lines[0] + assert '": ' not in lines[0] + event = json.loads(lines[0]) + assert event["lens"] == "security-review" + assert event["file_path"] == "src/foo.py" + assert event["file_content_hash"] == "abc123" + assert event["outcome"] == "pass" + assert "ts" in event + assert event["plugin_version"] == plugin_version.shipped_version() + assert "session_id" not in event + + +def test_emit_creates_metrics_dir_when_absent(tmp_path: Path) -> None: + assert not (tmp_path / ".claude" / "metrics").exists() + review_verdicts.emit_review_verdict(tmp_path, "security-review", "f.py", "h", "pass") + assert (tmp_path / ".claude" / "metrics").is_dir() + + +def test_emit_includes_session_id_when_given(tmp_path: Path) -> None: + review_verdicts.emit_review_verdict( + tmp_path, "security-review", "f.py", "h", "pass", session_id="sess-1" + ) + event = json.loads((tmp_path / _LOG_REL).read_text(encoding="utf-8").splitlines()[0]) + assert event["session_id"] == "sess-1" + + +def test_emit_appends_not_overwrites_across_two_calls(tmp_path: Path) -> None: + review_verdicts.emit_review_verdict(tmp_path, "security-review", "a.py", "h1", "pass") + review_verdicts.emit_review_verdict( + tmp_path, "structure-review", "b.py", "h2", "findings" + ) + lines = (tmp_path / _LOG_REL).read_text(encoding="utf-8").splitlines() + assert len(lines) == 2 + events = [json.loads(ln) for ln in lines] + assert events[0]["file_path"] == "a.py" + assert events[1]["file_path"] == "b.py" + + +def test_emit_fails_open_on_unwritable_metrics_dir(tmp_path: Path) -> None: + """A `.claude/` that can't hold a `metrics/` subdirectory (e.g. a file + occupying its path) must not raise — the caller's exit code must never + be affected.""" + (tmp_path / ".claude").write_text("not a directory") + review_verdicts.emit_review_verdict(tmp_path, "security-review", "f.py", "h", "pass") + + +def test_emit_fails_open_on_arbitrary_exception(tmp_path: Path, monkeypatch) -> None: + def _boom(*_a, **_k): + raise RuntimeError("disk is on fire") + + monkeypatch.setattr(review_verdicts, "_isoformat_utc", _boom) + review_verdicts.emit_review_verdict(tmp_path, "security-review", "f.py", "h", "pass") + assert not (tmp_path / _LOG_REL).exists() + + +# --------------------------------------------------------------------------- +# Consent posture (Decision 2): unconditional, like boundary-events.jsonl. +# --------------------------------------------------------------------------- + + +def test_emit_writes_even_when_telemetry_consent_is_off( + tmp_path: Path, monkeypatch +) -> None: + """Consent-off, demonstrated by test (plan AC): `DEV_TEAM_TELEMETRY` is + unset AND `~/.claude/telemetry.json` explicitly says `{"enabled": + false}` — `emit_review_verdict` must still write a row, since this store + sits outside consent gating entirely (Decision 2).""" + monkeypatch.delenv("DEV_TEAM_TELEMETRY", raising=False) + fake_home = tmp_path / "fake-home" + (fake_home / ".claude").mkdir(parents=True) + (fake_home / ".claude" / "telemetry.json").write_text( + json.dumps({"enabled": False}), encoding="utf-8" + ) + monkeypatch.setattr(Path, "home", lambda: fake_home) + # Sanity: confirm consent really does read as off under this setup — + # otherwise this test wouldn't prove what it claims to. + assert telemetry_consent.is_enabled() is False + + project = tmp_path / "project" + review_verdicts.emit_review_verdict(project, "security-review", "f.py", "h", "pass") + log = project / _LOG_REL + assert log.is_file() + assert len(log.read_text(encoding="utf-8").splitlines()) == 1 + + +# --------------------------------------------------------------------------- +# load_verdicts() — the reader +# --------------------------------------------------------------------------- + + +def test_load_verdicts_round_trips_an_emitted_row(tmp_path: Path) -> None: + review_verdicts.emit_review_verdict( + tmp_path, "security-review", "src/foo.py", "abc123", "findings" + ) + rows = review_verdicts.load_verdicts(tmp_path) + assert len(rows) == 1 + assert rows[0]["lens"] == "security-review" + assert rows[0]["file_path"] == "src/foo.py" + assert rows[0]["file_content_hash"] == "abc123" + assert rows[0]["outcome"] == "findings" + assert rows[0]["plugin_version"] == plugin_version.shipped_version() + + +def test_load_verdicts_absent_file_returns_empty(tmp_path: Path) -> None: + assert review_verdicts.load_verdicts(tmp_path) == [] + + +def test_load_verdicts_corrupted_line_yields_no_usable_rows(tmp_path: Path) -> None: + log = tmp_path / _LOG_REL + log.parent.mkdir(parents=True) + log.write_text("not json at all\n", encoding="utf-8") + assert review_verdicts.load_verdicts(tmp_path) == [] + + +def test_load_verdicts_version_mismatch_yields_no_usable_rows( + tmp_path: Path, monkeypatch +) -> None: + log = tmp_path / _LOG_REL + log.parent.mkdir(parents=True) + row = { + "ts": "2026-01-01T00:00:00Z", + "lens": "security-review", + "file_path": "f.py", + "file_content_hash": "h", + "outcome": "pass", + "plugin_version": "0.0.1", + } + log.write_text(json.dumps(row) + "\n", encoding="utf-8") + monkeypatch.setattr(review_verdicts.plugin_version, "shipped_version", lambda: "99.0.0") + assert review_verdicts.load_verdicts(tmp_path) == [] + + +def test_load_verdicts_never_raises_on_a_non_object_json_line(tmp_path: Path) -> None: + """A structurally-valid-JSON-but-not-object line (e.g. a bare list) is + another shape of "corrupted" this reader must not choke on.""" + log = tmp_path / _LOG_REL + log.parent.mkdir(parents=True) + log.write_text("[1, 2, 3]\n", encoding="utf-8") + assert review_verdicts.load_verdicts(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Mechanical content-guard (acceptance-critic finding): `load_verdicts` has +# no consumer yet beyond `review_verdict_recorder.py` (Step 2.3, not built +# in this slice) and this module's own tests — enforces the plan's "no +# consumer of review-verdicts.jsonl changes behavior in this slice" AC. +# --------------------------------------------------------------------------- + +_ALLOWED_LOAD_VERDICTS_CONSUMERS = { + _HOOKS_DIR / "review_verdict_recorder.py", +} + + +def test_load_verdicts_has_no_other_consumers() -> None: + hits: list[Path] = [] + for path in _PLUGIN_DIR.rglob("*.py"): + if "__pycache__" in path.parts: + continue + if path == _LIB_DIR / "review_verdicts.py": + continue + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + if "load_verdicts" not in text: + continue + hits.append(path) + + assert hits, "expected at least this test file to reference load_verdicts" + for hit in hits: + allowed = hit in _ALLOWED_LOAD_VERDICTS_CONSUMERS or hit.name.startswith( + "test_review_verdicts" + ) + assert allowed, ( + f"{hit} imports/references load_verdicts — only " + "review_verdict_recorder.py (Step 2.3, not built in this slice) " + "and this module's own test files may, per the plan's 'no " + "consumer changes behavior in this slice' AC." + ) From 4cc7e9b23daf6f34b324aae4b2c62ad4144a8483 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:57:46 +0000 Subject: [PATCH 08/10] feat(hooks): record per-lens review verdicts on SubagentStop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds hooks/review_verdict_recorder.py (#2166 Step 2.3): a SubagentStop hook that identifies the dispatched review agent via the harness-native attributionAgent field (hooks/lib/cost_meter.py's documented attribution mechanism, reused via scripts/lib/session_log.records rather than reimplemented), falling back to the Task/Agent dispatch join only when that field is absent. Checks subagent_type against review_agent_registry's closed set, parses the Step 2.1 dispatch-prompt scope marker for the in-scope file list, cross-references the agent's final JSON result's issues[].file, and writes one review-verdicts.jsonl row per in-scope file via review_verdicts.emit_review_verdict(). Fail-open throughout; a single unreadable in-scope file is skipped without affecting the others. A pre-implementation spike against 124 real subagent transcripts (recorded in the module's own docstring) confirmed attributionAgent is present on 100% of usage-bearing records, one consistent value per file — no fallback needed in practice, though it's still implemented per the plan. Registers the hook in both settings.json and hooks.json (parity required by tests/hooks/test_plugin_hooks_json.py), documents review-verdicts.jsonl in knowledge/telemetry-schema.md (fixing test_schema_doc_covers_all_metrics_paths, red since Steps 2.1/2.2 landed), and allowlists the new module's own attributionAgent-naming docstring in repo_invariants.py's transcript-parsing-confined-to-session-log check. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- plugins/dev-team/hooks/hooks.json | 4 + .../dev-team/hooks/review_verdict_recorder.py | 342 ++++++++++++ plugins/dev-team/knowledge/index.json | 4 + .../dev-team/knowledge/telemetry-schema.md | 45 ++ plugins/dev-team/settings.json | 4 + .../code-review/scripts/repo_invariants.py | 10 + .../hooks/test_review_verdict_recorder.py | 486 ++++++++++++++++++ 7 files changed, 895 insertions(+) create mode 100755 plugins/dev-team/hooks/review_verdict_recorder.py create mode 100644 plugins/dev-team/tests/hooks/test_review_verdict_recorder.py diff --git a/plugins/dev-team/hooks/hooks.json b/plugins/dev-team/hooks/hooks.json index 1d1564c45..7bb629232 100644 --- a/plugins/dev-team/hooks/hooks.json +++ b/plugins/dev-team/hooks/hooks.json @@ -301,6 +301,10 @@ { "type": "command", "command": "sh \"${CLAUDE_PLUGIN_ROOT}/hooks/py.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/subagent_completion_guard.py\"" + }, + { + "type": "command", + "command": "sh \"${CLAUDE_PLUGIN_ROOT}/hooks/py.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/review_verdict_recorder.py\"" } ] } diff --git a/plugins/dev-team/hooks/review_verdict_recorder.py b/plugins/dev-team/hooks/review_verdict_recorder.py new file mode 100755 index 000000000..ebfe0d460 --- /dev/null +++ b/plugins/dev-team/hooks/review_verdict_recorder.py @@ -0,0 +1,342 @@ +#!/usr/bin/env python3 +"""hooks/review_verdict_recorder.py — SubagentStop per-lens verdict recorder +(#2166, Step 2.3; plan: plans/2164-verdict-ledger-writer.md, Decisions 3/4/4a). + +## Contract (docs/python-hook-contract.md) + + Input : SubagentStop JSON on stdin (`transcript_path`, `session_id`, `cwd`) + Output: zero or more `.claude/metrics/review-verdicts.jsonl` rows via + `hooks/lib/review_verdicts.emit_review_verdict()` — one per in-scope + file the dispatch prompt's Step 2.1 scope marker declared, each + carrying `outcome: "pass"` or `"findings"`. + Posture: fail-open throughout. Any error, or any signal this hook can't + resolve confidently (missing/unreadable/non-JSON transcript, an + unresolvable `subagent_type`, an unregistered or registered-but- + non-review `subagent_type`, a missing/reformatted scope marker, an + unparseable/malformed final JSON result) -> zero rows written, exit + 0. A per-file read failure (a deleted/unreadable in-scope file) + skips only that file — the rest still get their rows. + +## Spike finding: `attributionAgent` reliability, checked against real +## transcripts before writing the rest of this hook (per this step's own +## instruction — not recalled/assumed) + +`hooks/lib/cost_meter.py`'s "Attribution dimensions" docstring claims the +native top-level `attributionAgent` field is "present on every usage-bearing +sidechain record in real transcripts". Checked here directly against every +real subagent transcript from this session +(`~/.claude/projects/-home-user-agentic-dev-team//subagents/agent-*.jsonl`, +124 files, 10,795 total rows, 4,279 usage-bearing assistant rows): + + * 124/124 files carried an `attributionAgent` value on 100% (4,279/4,279) + of their usage-bearing (assistant) records — no exceptions. + * Exactly one distinct value per file, always — never absent-then-present + partway through, never two different values in one file. The field + identifies the whole dispatch, not just one turn. + * Values are plugin-qualified for this plugin's own agents (e.g. + `dev-team:structure-review`, `dev-team:correctness-review`, + `dev-team:software-engineer`) and bare for harness-builtin agent types + (`general-purpose`, `claude-code-guide`) — `strip_plugin_prefix` + (`hooks/lib/review_agent_registry.py`) already normalizes exactly this, + reused below rather than reimplemented. + * The field is NOT stamped on every record in a subagent transcript — only + the usage-bearing (assistant) ones. The transcript's first record (the + dispatch-prompt `user` turn this hook also needs, for the Decision 3 + scope marker) never carries it, confirming as a side effect that "the + dispatch prompt is the subagent's initial user message" holds in + practice, not just in the plan's own claim. + +**Conclusion: the primary signal is fully reliable in this corpus — no scope +change to this step.** The documented Task/Agent-dispatch-join fallback +(main-thread `tool_use.input.subagent_type` + `toolUseResult.agentId`, +matched against the subagent transcript's own `agentId`, with the parent +transcript path derived from `transcript_path`'s own directory structure) is +still implemented below, per the plan's explicit instruction — but the spike +found no real transcript that ever needed it. + +Stdlib-only (hashlib/json/pathlib/sys). See ADR 0014, ADR 0015. +""" + +from __future__ import annotations + +import hashlib +import json +import sys +from pathlib import Path + +_HOOK_DIR = Path(__file__).resolve().parent +_LIB_DIR = _HOOK_DIR / "lib" +if str(_LIB_DIR) not in sys.path: + sys.path.insert(0, str(_LIB_DIR)) + +# hooks/ -> scripts/lib/session_log/ is a documented reverse-dependency +# exception (see hooks/lib/cost_meter.py's own module docstring for the +# full rationale): session_log/ ships INSIDE this same plugin package, +# always present wherever this hook runs, and session_log itself imports +# nothing from hooks/lib/ (no cycle). Mirrors cost_meter.py's own +# sys.path.insert + bare-package-import MECHANISM, not its directionality. +_SCRIPTS_LIB_DIR = _HOOK_DIR.parent / "scripts" / "lib" +if str(_SCRIPTS_LIB_DIR) not in sys.path: + sys.path.insert(0, str(_SCRIPTS_LIB_DIR)) + +from review_agent_registry import ( # type: ignore[import-not-found] + default_agents_dir, + read_registered_review_agent_names, + strip_plugin_prefix, +) +from review_verdicts import ( # type: ignore[import-not-found] + SCOPE_MARKER_PREFIX, + emit_review_verdict, +) +from session_log import records as _records # type: ignore[import-not-found] +from stdin_json import read_stdin_json, resolve_cwd # type: ignore[import-not-found] + + +def _read_transcript_records(path: Path) -> list[dict] | None: + """Every JSON-object row from `path`, or `None` when the transcript + itself can't be used at all — missing, unreadable, or containing zero + valid JSON lines. The three "malformed/unreadable transcript" Gherkin + scenarios all collapse onto this one fail-open signal; a caller treats + `None` and an empty-but-readable transcript identically (fail open, + write nothing).""" + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + return None + records: list[dict] = [] + saw_json = False + for line in text.splitlines(): + line = line.strip() + if not line: + continue + try: + row = json.loads(line) + except json.JSONDecodeError: + continue + saw_json = True + if isinstance(row, dict): + records.append(row) + return records if saw_json else None + + +def _attribution_subagent_type(records: list[dict]) -> str | None: + """Primary signal (see module docstring spike finding): the first + non-empty native attribution value found on any record in the subagent's + own transcript.""" + for rec in records: + agent = _records.attribution_agent_of(rec) + if agent: + return agent + return None + + +def _own_agent_id(records: list[dict]) -> str | None: + for rec in records: + agent_id = rec.get("agentId") + if isinstance(agent_id, str) and agent_id: + return agent_id + return None + + +def _parent_transcript_path(subagent_transcript: Path) -> Path | None: + """`//subagents/agent-.jsonl` implies + `/.jsonl` (this step's own documented derivation). + `None` when `subagent_transcript` doesn't match that layout.""" + subagents_dir = subagent_transcript.parent + if subagents_dir.name != "subagents": + return None + session_dir = subagents_dir.parent + return session_dir.parent / f"{session_dir.name}.jsonl" + + +def _fallback_subagent_type(subagent_transcript: Path, records: list[dict]) -> str | None: + """The documented Task/Agent dispatch join, used only when the primary + `attributionAgent` signal is absent from every record (see module + docstring: the spike found no real transcript that needed this path).""" + agent_id = _own_agent_id(records) + if not agent_id: + return None + parent_path = _parent_transcript_path(subagent_transcript) + if parent_path is None: + return None + parent_records = _read_transcript_records(parent_path) + if not parent_records: + return None + dispatch_types: dict[str, str] = {} + agent_types: dict[str, str] = {} + for rec in parent_records: + _records.join_dispatch_agent_ids(rec, dispatch_types, agent_types) + return agent_types.get(agent_id) + + +def _resolve_subagent_type(subagent_transcript: Path, records: list[dict]) -> str | None: + raw = _attribution_subagent_type(records) or _fallback_subagent_type( + subagent_transcript, records + ) + return strip_plugin_prefix(raw) if raw else None + + +def _message_text(message: object) -> str | None: + """Assistant/user message `content` as plain text, whether it's a bare + string or a Messages-API content-block list (only `text` blocks + contribute; a block list with no text block returns `None`).""" + if not isinstance(message, dict): + return None + content = message.get("content") + if isinstance(content, str): + return content + if isinstance(content, list): + parts = [ + block.get("text", "") + for block in content + if isinstance(block, dict) and block.get("type") == "text" + ] + return "\n".join(parts) if parts else None + return None + + +def _first_turn_text(records: list[dict]) -> str | None: + if not records: + return None + return _message_text(records[0].get("message")) + + +def _last_turn_text(records: list[dict]) -> str | None: + if not records: + return None + return _message_text(records[-1].get("message")) + + +def _parse_scope_marker(text: str) -> list[str] | None: + """The in-scope file list from the Step 2.1 `SCOPE_MARKER_PREFIX` line, + or `None` when no line starts with it — a missing/reformatted marker, + which the caller treats as fail-open (zero rows).""" + for line in text.splitlines(): + if line.startswith(SCOPE_MARKER_PREFIX): + remainder = line[len(SCOPE_MARKER_PREFIX) :] + return [f.strip() for f in remainder.split(",") if f.strip()] + return None + + +def _extract_json_object(text: str) -> dict | None: + """A small, tolerant JSON-object extractor for an agent's final-turn + text: a clean parse first, then the first `{` to the last `}` span + (recovers a fenced ```json block or a prose preamble/trailing + sentence). Returns `None` for anything that isn't recoverable as a JSON + object — the caller treats that as a malformed final result and fails + open (zero rows), never guesses a verdict from a result it couldn't + parse.""" + stripped = text.strip() + if not stripped: + return None + try: + parsed = json.loads(stripped) + except json.JSONDecodeError: + parsed = None + if isinstance(parsed, dict): + return parsed + start = stripped.find("{") + end = stripped.rfind("}") + if start == -1 or end == -1 or end <= start: + return None + try: + parsed = json.loads(stripped[start : end + 1]) + except json.JSONDecodeError: + return None + return parsed if isinstance(parsed, dict) else None + + +def _issues_list(result: dict | None) -> list | None: + """`None` means "can't trust any verdict from this result" (missing or + schema-drifted `issues`) — the caller fails open. A genuine empty list + is a usable clean result (every in-scope file gets `pass`).""" + if result is None: + return None + issues = result.get("issues") + return issues if isinstance(issues, list) else None + + +def _findings_files(issues: list) -> set[str]: + files: set[str] = set() + for issue in issues: + if isinstance(issue, dict): + file_path = issue.get("file") + if isinstance(file_path, str) and file_path: + files.add(file_path) + return files + + +def _hash_file(path: Path) -> str | None: + """Current-content sha256 hex digest of `path`, or `None` on any read + failure (deleted, unreadable, or a directory) — the caller skips just + that one in-scope file rather than aborting the whole batch.""" + try: + data = path.read_bytes() + except OSError: + return None + return hashlib.sha256(data).hexdigest() + + +def process(payload: dict) -> None: + """Fail-open SubagentStop processing (see module docstring Contract). + + Writes zero or more rows via `emit_review_verdict`; never raises — + every input this function can't resolve confidently degrades to "write + nothing" rather than a guess (Decision 4a: this hook trusts the dispatch + prompt's declared scope, it does not independently re-verify it).""" + transcript_path = payload.get("transcript_path") + if not isinstance(transcript_path, str) or not transcript_path: + return + transcript = Path(transcript_path) + records = _read_transcript_records(transcript) + if not records: + return + + subagent_type = _resolve_subagent_type(transcript, records) + if not subagent_type: + return + + registered = read_registered_review_agent_names(default_agents_dir()) + if not registered or subagent_type not in registered: + return + + first_text = _first_turn_text(records) + in_scope = _parse_scope_marker(first_text) if first_text else None + if not in_scope: + return + + last_text = _last_turn_text(records) + result = _extract_json_object(last_text) if last_text else None + issues = _issues_list(result) + if issues is None: + return + findings_files = _findings_files(issues) + + cwd = resolve_cwd(payload) + session_id = payload.get("session_id") + + for file_path in in_scope: + target = Path(file_path) + if not target.is_absolute(): + target = Path(cwd) / target + file_hash = _hash_file(target) + if file_hash is None: + continue # deleted/unreadable in-scope file -- skip this one only + outcome = "findings" if file_path in findings_files else "pass" + emit_review_verdict( + cwd, subagent_type, file_path, file_hash, outcome, session_id=session_id + ) + + +def main() -> int: + """Fail-open SubagentStop entry point — see module docstring Contract.""" + try: + payload = read_stdin_json() or {} + process(payload) + except Exception: # noqa: BLE001, S110 — fail-open by design, see module docstring + pass + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/plugins/dev-team/knowledge/index.json b/plugins/dev-team/knowledge/index.json index 1fbea9983..f171a86ed 100644 --- a/plugins/dev-team/knowledge/index.json +++ b/plugins/dev-team/knowledge/index.json @@ -1504,6 +1504,10 @@ "summary": "**Added by #859.** The boundary-level (policy-gateway) channel: every guard", "anchor": "boundary-eventsjsonl" }, + "`review-verdicts.jsonl`": { + "summary": "**Added by #2166** (plan: `plans/2164-verdict-ledger-writer.md`, Slice 2).", + "anchor": "review-verdictsjsonl" + }, "`telemetry.jsonl`": { "summary": "Opt-in usage beacon: which slash commands / skills get invoked, and whether", "anchor": "telemetryjsonl" diff --git a/plugins/dev-team/knowledge/telemetry-schema.md b/plugins/dev-team/knowledge/telemetry-schema.md index 0f8dc1a5d..9c0accb3e 100644 --- a/plugins/dev-team/knowledge/telemetry-schema.md +++ b/plugins/dev-team/knowledge/telemetry-schema.md @@ -103,6 +103,51 @@ problem. `hooks/pre_pr_review.py` never emits this event. Existing rows in --- +## `review-verdicts.jsonl` + +**Added by #2166** (plan: `plans/2164-verdict-ledger-writer.md`, Slice 2). A +**new, separate** store from `boundary-events.jsonl` (Decision 1) — not an +overload of that stream's `record` decision — because a per-file verdict +needs a real `file_path`, which `boundary_events.py`'s own "never write free +text ... file paths ... must never appear" invariant forbids. Records, per +genuine review-agent dispatch, an outcome (`pass` \| `findings`) bound to +`(lens, file_path, file_content_hash)` — a verdict about *this exact file +content*, not about any one diff, so it can be looked up again the next time +the same content recurs regardless of which diff produced it. + +The recorder identifies which lens dispatched via the native +`attributionAgent` field the harness stamps on the subagent's own transcript +records (`hooks/lib/cost_meter.py`'s "Attribution dimensions" mechanism, +reused via `scripts/lib/session_log.records`), falling back to the +documented Task/Agent-dispatch join only when that field is absent. It reads +the in-scope file list from a structured marker +(`skills/code-review/SKILL.md` step 4: `Files in scope for this review: +, ...`) in the dispatch prompt — the subagent transcript's own first +turn — and cross-references it against the agent's final JSON result's +`issues[].file` list (`knowledge/review-agent-output-contract.md`). +**Disclosed trust boundary (Decision 4a):** the in-scope list is the +orchestrating session's own declared scope, not independently re-verified +against any diff — the property this store adds is that a *real* +`SubagentStop` event occurred for a *registered* review agent, not +omniscient verification of review depth. + +| Field | Type | Values / source | +| --- | --- | --- | +| `ts` | string | ISO-8601 UTC `%Y-%m-%dT%H:%M:%SZ` | +| `lens` | string | The dispatched review agent's registered name (e.g. `structure-review`), plugin-prefix-stripped | +| `file_path` | string | One file the dispatch prompt's scope marker declared in scope | +| `file_content_hash` | string | sha256 hex digest of `file_path`'s content at the time the recorder ran (current content, not the content at dispatch time) | +| `outcome` | string enum | `pass` \| `findings` — whether `file_path` appears in the agent's final `issues[]` | +| `plugin_version` | string | From `.claude-plugin/plugin.json` | +| `session_id` | string, optional | Opaque per-session ID, when present in the hook payload | + +- **Emitter:** `hooks/review_verdict_recorder.py` (a `SubagentStop` hook) via `hooks/lib/review_verdicts.emit_review_verdict()`. No-op (zero rows) for any `subagent_type` outside `hooks/lib/review_agent_registry`'s closed set of registered `agents/*-review.md` names, and fail-open throughout (missing/unreadable transcript, unresolved `subagent_type`, a missing/reformatted scope marker, or an unparseable final JSON result all degrade to zero rows, never an exception); a single deleted/unreadable in-scope file is skipped without affecting the other rows. +- **Consent:** ALWAYS-ON — same posture as `boundary-events.jsonl` (Decision 2), not gated by `DEV_TEAM_TELEMETRY`/`~/.claude/telemetry.json`. Local-only, mechanical accountability data (lens/path/hash/outcome), no prose. +- **Fail-open:** every exception in `emit_review_verdict()` is swallowed — never changes the calling hook's exit code, stdout, or stderr. `hooks/lib/review_verdicts.load_verdicts()` mirrors this on the read side: an absent file, a corrupted line, or a stale `plugin_version` row all degrade to "no usable rows", never an exception. +- **Consumers:** none yet — this slice is deliberately writer-only (#2167 is the queued consumer slice). + +--- + ## `telemetry.jsonl` Opt-in usage beacon: which slash commands / skills get invoked, and whether diff --git a/plugins/dev-team/settings.json b/plugins/dev-team/settings.json index 92d641310..066386c9b 100644 --- a/plugins/dev-team/settings.json +++ b/plugins/dev-team/settings.json @@ -317,6 +317,10 @@ { "type": "command", "command": "sh hooks/py.sh hooks/subagent_completion_guard.py" + }, + { + "type": "command", + "command": "sh hooks/py.sh hooks/review_verdict_recorder.py" } ] } diff --git a/plugins/dev-team/skills/code-review/scripts/repo_invariants.py b/plugins/dev-team/skills/code-review/scripts/repo_invariants.py index cbfdba0d5..cbad32641 100755 --- a/plugins/dev-team/skills/code-review/scripts/repo_invariants.py +++ b/plugins/dev-team/skills/code-review/scripts/repo_invariants.py @@ -609,6 +609,16 @@ def check_contract_failure_shapes_documented(changed_files=None) -> list[dict]: "narrower concern than the four-identifier duplication this " "invariant targets, not zero" ), + "plugins/dev-team/hooks/review_verdict_recorder.py": ( + "reads attributionAgent/agentId only through session_log.records " + "(attribution_agent_of/join_dispatch_agent_ids) -- never a raw " + "field access; the 'attributionAgent' occurrences are all in this " + "file's own module docstring, recording #2166 Step 2.3's own " + "pre-implementation spike finding against 124 real subagent " + "transcripts (mirrors cost_meter.py's entry above: prose " + "documenting the harness field this hook's decisions are based on, " + "not a second parsing implementation)" + ), "plugins/dev-team/hooks/lib/pricing.py": ( "reads a pre-extracted usage dict's known numeric fields " "(cache_creation_input_tokens/cache_read_input_tokens) for cost " diff --git a/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py b/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py new file mode 100644 index 000000000..946331772 --- /dev/null +++ b/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py @@ -0,0 +1,486 @@ +"""Tests for hooks/review_verdict_recorder.py (#2166, Step 2.3). + +Fixture-transcript style, mirroring `test_subagent_completion_guard.py`'s own +convention: a subagent transcript is a list of JSONL rows written to a temp +file, `main()` is exercised end-to-end via `subprocess` (so the real +`read_stdin_json()` path is exercised, not a monkeypatched stand-in), and the +written `review-verdicts.jsonl` rows are read back and asserted on. + +Covers every Gherkin scenario in the plan's Slice 2 feature block +(plans/2164-verdict-ledger-writer.md), one test per scenario. +""" + +from __future__ import annotations + +import hashlib +import json +import os +import subprocess +import sys +from pathlib import Path + +_HOOK_DIR = Path(__file__).resolve().parents[2] / "hooks" +_HOOK_PY = _HOOK_DIR / "review_verdict_recorder.py" +_LIB_DIR = _HOOK_DIR / "lib" +for _p in (_HOOK_DIR, _LIB_DIR): + if str(_p) not in sys.path: + sys.path.insert(0, str(_p)) + +import review_verdict_recorder as recorder +from review_verdicts import SCOPE_MARKER_PREFIX # type: ignore[import-not-found] + +_VERDICTS_REL = Path(".claude") / "metrics" / "review-verdicts.jsonl" + +# A real registered review agent (agents/structure-review.md exists). +_REVIEW_AGENT = "structure-review" +# A real, registered TEAM agent that is not a review lens +# (agents/software-engineer.md exists, but doesn't match agents/*-review.md). +_NON_REVIEW_AGENT = "software-engineer" +# Not a real agent file at all (no agents/phantom-review.md). +_UNREGISTERED_AGENT = "phantom-review" + + +# --------------------------------------------------------------------------- +# Fixture builders +# --------------------------------------------------------------------------- + + +def _write_transcript(tmp_path: Path, rows: list[dict], name: str = "agent-test.jsonl") -> str: + path = tmp_path / "subagents" + path.mkdir(parents=True, exist_ok=True) + file_path = path / name + with file_path.open("w", encoding="utf-8") as fh: + for row in rows: + fh.write(json.dumps(row) + "\n") + return str(file_path) + + +def _dispatch_row(in_scope_files: list[str], agent_id: str = "agent-1") -> dict: + """The subagent transcript's first (dispatch-prompt) turn: role `user`, + plain-string content carrying the Step 2.1 scope marker.""" + marker = SCOPE_MARKER_PREFIX + ", ".join(in_scope_files) + return { + "type": "user", + "agentId": agent_id, + "message": { + "role": "user", + "content": f"Review the following files for issues.\n\n{marker}\n", + }, + } + + +def _result_row(result: dict, attribution_agent: str, agent_id: str = "agent-1") -> dict: + """The subagent transcript's final (result) turn: an assistant row + carrying the native `attributionAgent` field and the agent's JSON + result as text.""" + return { + "type": "assistant", + "isSidechain": True, + "agentId": agent_id, + "attributionAgent": attribution_agent, + "message": { + "role": "assistant", + "content": [{"type": "text", "text": json.dumps(result)}], + }, + } + + +def _write_file(tmp_path: Path, rel_path: str, content: str = "hello\n") -> None: + target = tmp_path / rel_path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + + +def _sha256(text: str) -> str: + return hashlib.sha256(text.encode("utf-8")).hexdigest() + + +def _run_hook(raw_stdin: bytes) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(_HOOK_PY)], + input=raw_stdin, + env={"PATH": os.environ.get("PATH", "/usr/bin:/bin")}, + capture_output=True, + timeout=10, + check=False, + ) + + +def _run_main(tmp_path: Path, transcript_path: str | None, session_id: str | None = "sess-1") -> int: + payload: dict = {"cwd": str(tmp_path)} + if transcript_path is not None: + payload["transcript_path"] = transcript_path + if session_id is not None: + payload["session_id"] = session_id + result = _run_hook(json.dumps(payload).encode()) + assert result.stdout == b"" + assert result.stderr == b"" + return result.returncode + + +def _read_rows(tmp_path: Path) -> list[dict]: + path = tmp_path / _VERDICTS_REL + if not path.is_file(): + return [] + return [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines() if line] + + +# --------------------------------------------------------------------------- +# Scenario: clean review agent result records a pass row per in-scope file +# --------------------------------------------------------------------------- + + +def test_clean_result_records_a_pass_row_per_in_scope_file(tmp_path: Path) -> None: + files = ["a.py", "b.py", "c.py"] + for f in files: + _write_file(tmp_path, f, content=f"content of {f}\n") + + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(files), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + assert len(rows) == 3 + by_path = {r["file_path"]: r for r in rows} + assert set(by_path) == set(files) + for f in files: + row = by_path[f] + assert row["outcome"] == "pass" + assert row["lens"] == _REVIEW_AGENT + assert row["file_content_hash"] == _sha256(f"content of {f}\n") + assert row["session_id"] == "sess-1" + assert "plugin_version" in row + + +# --------------------------------------------------------------------------- +# Scenario: a result with findings records a mixed verdict per file +# --------------------------------------------------------------------------- + + +def test_findings_result_records_mixed_verdict_per_file(tmp_path: Path) -> None: + files = ["a.py", "b.py", "c.py"] + for f in files: + _write_file(tmp_path, f) + + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(files), + _result_row( + { + "status": "warn", + "issues": [ + { + "severity": "warning", + "confidence": "medium", + "file": "b.py", + "line": 3, + "message": "something", + } + ], + "summary": "1 issue", + }, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + by_path = {r["file_path"]: r["outcome"] for r in rows} + assert by_path == {"a.py": "pass", "b.py": "findings", "c.py": "pass"} + + +# --------------------------------------------------------------------------- +# Scenario: an unregistered subagent_type is ignored +# --------------------------------------------------------------------------- + + +def test_unregistered_subagent_type_writes_no_rows(tmp_path: Path) -> None: + _write_file(tmp_path, "a.py") + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py"]), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + _UNREGISTERED_AGENT, + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Scenario: a registered but non-review subagent_type is ignored +# --------------------------------------------------------------------------- + + +def test_registered_non_review_subagent_type_writes_no_rows(tmp_path: Path) -> None: + _write_file(tmp_path, "a.py") + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py"]), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_NON_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Scenario: a dispatch with an empty scope marker writes no rows +# --------------------------------------------------------------------------- + + +def test_empty_scope_marker_writes_no_rows(tmp_path: Path) -> None: + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row([]), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Scenario: a dispatch with exactly one file in scope writes exactly one row +# --------------------------------------------------------------------------- + + +def test_single_file_scope_writes_exactly_one_row(tmp_path: Path) -> None: + _write_file(tmp_path, "only.py") + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["only.py"]), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + assert len(rows) == 1 + assert rows[0]["file_path"] == "only.py" + assert rows[0]["outcome"] == "pass" + + +# --------------------------------------------------------------------------- +# Scenario: an in-scope file that no longer exists is skipped, not fatal +# --------------------------------------------------------------------------- + + +def test_deleted_in_scope_file_is_skipped_others_still_written(tmp_path: Path) -> None: + _write_file(tmp_path, "a.py") + _write_file(tmp_path, "c.py") + # "b.py" is declared in scope but never created on disk. + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py", "b.py", "c.py"]), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + assert {r["file_path"] for r in rows} == {"a.py", "c.py"} + + +# --------------------------------------------------------------------------- +# Scenario Outline: a malformed or unreadable transcript fails open +# --------------------------------------------------------------------------- + + +def test_missing_transcript_path_key_writes_no_rows(tmp_path: Path) -> None: + assert _run_main(tmp_path, None) == 0 + assert _read_rows(tmp_path) == [] + + +def test_unreadable_transcript_path_writes_no_rows(tmp_path: Path) -> None: + # A directory can never be read as transcript text -- OSError on read. + unreadable = tmp_path / "not-a-file" + unreadable.mkdir() + assert _run_main(tmp_path, str(unreadable)) == 0 + assert _read_rows(tmp_path) == [] + + +def test_non_json_transcript_content_writes_no_rows(tmp_path: Path) -> None: + bad = tmp_path / "garbage.jsonl" + bad.write_text("this is not json\nneither is this\n", encoding="utf-8") + assert _run_main(tmp_path, str(bad)) == 0 + assert _read_rows(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Scenario: a well-formed transcript with a missing/reformatted scope marker +# fails open +# --------------------------------------------------------------------------- + + +def test_missing_scope_marker_writes_no_rows(tmp_path: Path) -> None: + transcript = _write_transcript( + tmp_path, + [ + { + "type": "user", + "agentId": "agent-1", + "message": { + "role": "user", + "content": "Review these files: a.py, b.py\n", + }, + }, + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Scenario: the recorder trusts the dispatch prompt's declared scope; it is +# not independently re-verified against the diff +# --------------------------------------------------------------------------- + + +def test_trusts_declared_scope_without_reverifying_against_diff(tmp_path: Path) -> None: + """The marker declares "trusted.py" in scope; nothing in this test ever + constructs or checks a diff/git state for that path -- the recorder + writes a pass row purely from the marker + the final JSON result, + exactly the trust boundary Decision 4a documents.""" + _write_file(tmp_path, "trusted.py") + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["trusted.py"]), + _result_row( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + assert len(rows) == 1 + assert rows[0]["file_path"] == "trusted.py" + assert rows[0]["outcome"] == "pass" + + +# --------------------------------------------------------------------------- +# Malformed final JSON result also fails open (Step 2.3 IMPLEMENT text, +# not a separate Gherkin scenario but part of this step's own contract). +# --------------------------------------------------------------------------- + + +def test_malformed_final_json_result_writes_no_rows(tmp_path: Path) -> None: + _write_file(tmp_path, "a.py") + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py"]), + { + "type": "assistant", + "isSidechain": True, + "agentId": "agent-1", + "attributionAgent": f"dev-team:{_REVIEW_AGENT}", + "message": { + "role": "assistant", + "content": [{"type": "text", "text": "not json output at all"}], + }, + }, + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Unit-level coverage of the documented Task/Agent dispatch-join fallback +# (used only when attributionAgent is absent from every record -- the spike +# recorded in this hook's own module docstring found no real transcript that +# needed it, but the fallback is still implemented per the plan and exercised +# here directly rather than left dead). +# --------------------------------------------------------------------------- + + +def test_fallback_resolves_subagent_type_via_task_agent_join(tmp_path: Path) -> None: + session_dir = tmp_path / "session-1" + subagents_dir = session_dir / "subagents" + subagents_dir.mkdir(parents=True) + + subagent_transcript = subagents_dir / "agent-abc123.jsonl" + subagent_rows = [ + { + "type": "user", + "agentId": "abc123", + "message": {"role": "user", "content": "Review a.py"}, + }, + { + "type": "assistant", + "isSidechain": True, + "agentId": "abc123", + # No attributionAgent field at all on any record. + "message": {"role": "assistant", "content": "done"}, + }, + ] + with subagent_transcript.open("w", encoding="utf-8") as fh: + for row in subagent_rows: + fh.write(json.dumps(row) + "\n") + + parent_transcript = tmp_path / "session-1.jsonl" + parent_rows = [ + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "toolu_1", + "name": "Task", + "input": {"subagent_type": f"dev-team:{_REVIEW_AGENT}"}, + } + ], + }, + }, + { + "type": "user", + "toolUseResult": {"agentId": "abc123"}, + "message": { + "role": "user", + "content": [{"type": "tool_result", "tool_use_id": "toolu_1"}], + }, + }, + ] + with parent_transcript.open("w", encoding="utf-8") as fh: + for row in parent_rows: + fh.write(json.dumps(row) + "\n") + + records = recorder._read_transcript_records(subagent_transcript) + resolved = recorder._resolve_subagent_type(subagent_transcript, records) + assert resolved == _REVIEW_AGENT From 10d784241da82bdf3f52f83db63d62caea437589 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 16:19:44 +0000 Subject: [PATCH 09/10] fix(hooks): fix handback parsing and path normalization in verdict recorder Fixes nine review findings from spec-compliance/correctness/security/test checkpoints against review_verdict_recorder.py: - CRITICAL: the hook read the transcript's literal last row as the agent's JSON result, but a real completed subagent's last row is a short wrap-up turn AFTER the SubagentHandback tool_use/tool_result exchange -- the real JSON lives in the handback's own input.message field. Verified against a real transcript in this session's own corpus before fixing. The hook wrote zero rows in production before this fix. - Path-form mismatch between the scope marker and issues[].file (relative vs. absolute) caused false "pass" verdicts; both sides now resolve against cwd before comparison. - Degenerate exits past subagent_type confirmation (missing scope marker, unparseable result) now record a boundary event naming the reason, instead of being silently indistinguishable from a legitimate no-op. - _hash_file rejects non-regular files and caps read size, closing a FIFO/device hang past the hook's fail-open wrapper. - Scope-marker paths are contained to cwd before being read/hashed. - repo_invariants.py's transcript-parsing allowlist entry for this hook is corrected to accurately describe its agentId/attributionAgent reads, and its transcript reader now delegates to session_log's shared streaming iterator instead of a second whole-file reader. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .../dev-team/hooks/review_verdict_recorder.py | 222 ++++++++++--- .../dev-team/knowledge/telemetry-schema.md | 17 +- .../code-review/scripts/repo_invariants.py | 24 +- .../hooks/test_review_verdict_recorder.py | 302 ++++++++++++++++-- 4 files changed, 485 insertions(+), 80 deletions(-) diff --git a/plugins/dev-team/hooks/review_verdict_recorder.py b/plugins/dev-team/hooks/review_verdict_recorder.py index ebfe0d460..c693adde0 100755 --- a/plugins/dev-team/hooks/review_verdict_recorder.py +++ b/plugins/dev-team/hooks/review_verdict_recorder.py @@ -8,7 +8,13 @@ Output: zero or more `.claude/metrics/review-verdicts.jsonl` rows via `hooks/lib/review_verdicts.emit_review_verdict()` — one per in-scope file the dispatch prompt's Step 2.1 scope marker declared, each - carrying `outcome: "pass"` or `"findings"`. + carrying `outcome: "pass"` or `"findings"`. Once `subagent_type` is + confirmed a registered review lens, a degenerate exit (missing/ + reformatted scope marker, unparseable/malformed final result) also + writes one `.claude/metrics/boundary-events.jsonl` `"record"`-decision + row via `hooks/lib/boundary_events.emit_boundary_event()`, naming the + reason (`missing-scope-marker` | `unparseable-result`) (Fix #3, #2166 + correctness review). Posture: fail-open throughout. Any error, or any signal this hook can't resolve confidently (missing/unreadable/non-JSON transcript, an unresolvable `subagent_type`, an unregistered or registered-but- @@ -79,6 +85,7 @@ if str(_SCRIPTS_LIB_DIR) not in sys.path: sys.path.insert(0, str(_SCRIPTS_LIB_DIR)) +from boundary_events import emit_boundary_event # type: ignore[import-not-found] from review_agent_registry import ( # type: ignore[import-not-found] default_agents_dir, read_registered_review_agent_names, @@ -98,21 +105,21 @@ def _read_transcript_records(path: Path) -> list[dict] | None: valid JSON lines. The three "malformed/unreadable transcript" Gherkin scenarios all collapse onto this one fail-open signal; a caller treats `None` and an empty-but-readable transcript identically (fail open, - write nothing).""" - try: - text = path.read_text(encoding="utf-8", errors="replace") - except OSError: - return None + write nothing). + + Delegates to `session_log.records.iter_file_records` (imported above as + `_records`, Fix #9 / #2166 correctness review) rather than + `read_text()`-ing the whole transcript into memory: this hook has no + need to distinguish "the trailing line is malformed JSON" as its own + outcome the way `subagent_completion_guard.py`'s private `_tail_lines`/ + `_last_row` reader does (see that module's own docstring for why IT + keeps a private reader) — `_read_transcript_records` only ever needs + "did this transcript yield at least one usable JSON object", which the + shared streaming iterator answers just as well, at a fraction of the + peak memory on a multi-MB transcript.""" records: list[dict] = [] saw_json = False - for line in text.splitlines(): - line = line.strip() - if not line: - continue - try: - row = json.loads(line) - except json.JSONDecodeError: - continue + for row in _records.iter_file_records(path): saw_json = True if isinstance(row, dict): records.append(row) @@ -201,10 +208,64 @@ def _first_turn_text(records: list[dict]) -> str | None: return _message_text(records[0].get("message")) -def _last_turn_text(records: list[dict]) -> str | None: - if not records: - return None - return _message_text(records[-1].get("message")) +def _handback_message_text(records: list[dict]) -> str | None: + """PRIMARY result-extraction path (Fix #1, #2166 correctness review — + CRITICAL: this hook wrote zero rows in production before this fix). + + Verified directly against a real transcript in this session's own + corpus before writing this (not assumed): + `~/.claude/projects/-home-user-agentic-dev-team//subagents/ + agent-a006af1f32a025449.jsonl`, line 17 — a completed subagent's real + final JSON result lives inside its OWN `SubagentHandback` tool_use + block's `input.message` field (prose plus a fenced ```json block), never + as the transcript's literal last row. The literal last row is a short + wrap-up assistant turn ("Report delivered...") that comes AFTER the + handback's own `tool_result` ack — exactly the shape + `subagent_completion_guard.py`'s module docstring documents and confirms + across 70 real transcripts ("Finding 3 — SubagentHandback is a real, + distinguishable tool_use block"). Scans backward so the LAST handback + call wins if a transcript ever carries more than one.""" + for rec in reversed(records): + message = rec.get("message") + if not isinstance(message, dict): + continue + content = message.get("content") + if not isinstance(content, list): + continue + for block in content: + if ( + isinstance(block, dict) + and block.get("type") == "tool_use" + and block.get("name") == "SubagentHandback" + ): + handback_input = block.get("input") + if isinstance(handback_input, dict): + text = handback_input.get("message") + if isinstance(text, str) and text: + return text + return None + + +def _fallback_last_parseable_turn_text(records: list[dict]) -> str | None: + """FALLBACK ONLY (Fix #1): used when no `SubagentHandback` call is found + anywhere in the transcript. This session's own corpus never needed this + path (see `_handback_message_text` above) — kept per the plan's explicit + instruction for a transcript shape that never showed up in the sample. + The last turn (scanning backward) whose text itself recovers as a JSON + object via `_extract_json_object`, rather than blindly trusting the + transcript's literal last row the way the pre-fix code did.""" + for rec in reversed(records): + text = _message_text(rec.get("message")) + if text and _extract_json_object(text) is not None: + return text + return None + + +def _final_result_text(records: list[dict]) -> str | None: + """The text handed to `_extract_json_object` for the agent's final JSON + result: `_handback_message_text` first, `_fallback_last_parseable_turn_text` + only when no handback call is present at all (Fix #1).""" + return _handback_message_text(records) or _fallback_last_parseable_turn_text(records) def _parse_scope_marker(text: str) -> list[str] | None: @@ -256,25 +317,77 @@ def _issues_list(result: dict | None) -> list | None: return issues if isinstance(issues, list) else None -def _findings_files(issues: list) -> set[str]: - files: set[str] = set() +def _resolve_under_cwd(file_path: str, cwd) -> Path | None: + """Resolve `file_path` (a scope-marker entry or an `issues[].file` + entry) against `cwd` to an absolute, symlink-resolved `Path`. + + This is the single normalized form both: + * the path-traversal containment check (Fix #5, security review: + a scope marker declaring `../../../../etc/passwd`-style paths must + not be read/hashed outside the repo), and + * the scope-marker/`issues[].file` membership comparison (Fix #2, + correctness review: real review-agent transcripts in this session's + own corpus report the SAME file in different path forms — + relative vs. absolute — across different agents, so raw string + equality silently missed genuine matches) + key off of, so the two concerns share one resolution instead of two + that could drift. Local replica of `finding_signature.py`'s own + `_normalize_path` intent rather than an import of it: + `hooks/lib/doc_classification.py`'s module docstring documents the + established direction as `skills/*/scripts/` importing FROM `hooks/lib/` + (`change_shape.py` reaches into `doc_classification.py`, never the + reverse) — a hook importing `skills/code-review/scripts/finding_signature.py` + would invert that. + + `None` on any resolution failure — rare, since `Path.resolve()` without + `strict=` doesn't require the path to exist, but a broken symlink chain + can still raise `OSError`.""" + try: + target = Path(file_path) + if not target.is_absolute(): + target = Path(cwd) / target + return target.resolve() + except OSError: + return None + + +def _findings_files(issues: list, cwd) -> set[Path]: + files: set[Path] = set() for issue in issues: if isinstance(issue, dict): file_path = issue.get("file") if isinstance(file_path, str) and file_path: - files.add(file_path) + resolved = _resolve_under_cwd(file_path, cwd) + if resolved is not None: + files.add(resolved) return files +# 50 MiB cap (Fix #4, security review): bounds `_hash_file`'s read against a +# FIFO/device path or a multi-GB file hanging past this hook's fail-open +# exception wrapper in `main()` — a hang is not an exception `main()` can +# catch. +_MAX_HASH_FILE_BYTES = 50 * 1024 * 1024 +_HASH_CHUNK_BYTES = 1 << 20 # 1 MiB incremental read + + def _hash_file(path: Path) -> str | None: """Current-content sha256 hex digest of `path`, or `None` on any read - failure (deleted, unreadable, or a directory) — the caller skips just - that one in-scope file rather than aborting the whole batch.""" + failure (deleted, unreadable, not a regular file — a directory, FIFO, or + device — or over `_MAX_HASH_FILE_BYTES`) — the caller skips just that + one in-scope file rather than aborting the whole batch.""" try: - data = path.read_bytes() + if not path.is_file(): + return None + if path.stat().st_size > _MAX_HASH_FILE_BYTES: + return None + digest = hashlib.sha256() + with path.open("rb") as fh: + while chunk := fh.read(_HASH_CHUNK_BYTES): + digest.update(chunk) + return digest.hexdigest() except OSError: return None - return hashlib.sha256(data).hexdigest() def process(payload: dict) -> None: @@ -283,7 +396,23 @@ def process(payload: dict) -> None: Writes zero or more rows via `emit_review_verdict`; never raises — every input this function can't resolve confidently degrades to "write nothing" rather than a guess (Decision 4a: this hook trusts the dispatch - prompt's declared scope, it does not independently re-verify it).""" + prompt's declared scope, it does not independently re-verify it). + + Fix #3 (correctness review): the early returns below the point where + `subagent_type` is confirmed to be a REGISTERED REVIEW LENS are + observationally degenerate (zero rows, zero stderr, exit 0) the same way + a legitimate no-op is — but unlike a legitimate no-op, they mean a + dispatch that SHOULD have produced rows didn't. Those two exits + (missing/reformatted scope marker, unparseable/malformed final result) + each record one `boundary_events` `"record"`-decision row naming the + reason, mirroring `subagent_completion_guard.py`'s own posture of + surfacing a non-clean, explainable classification instead of staying + silently indistinguishable from the happy path. The earlier, + PRE-resolution early returns (missing transcript, unreadable transcript, + unresolvable `subagent_type`, an entirely unregistered or a + registered-but-non-review `subagent_type`) stay silent — those are + legitimate no-ops: this dispatch was never confirmed to be a review + lens that should have produced rows at all.""" transcript_path = payload.get("transcript_path") if not isinstance(transcript_path, str) or not transcript_path: return @@ -300,29 +429,48 @@ def process(payload: dict) -> None: if not registered or subagent_type not in registered: return + # Past this point `subagent_type` is a confirmed, registered review + # lens -- this dispatch SHOULD produce rows (Fix #3). + cwd = resolve_cwd(payload) + session_id = payload.get("session_id") + first_text = _first_turn_text(records) in_scope = _parse_scope_marker(first_text) if first_text else None if not in_scope: + emit_boundary_event( + cwd, + "review_verdict_recorder", + "SubagentStop", + "record", + "missing-scope-marker", + session_id=session_id, + ) return - last_text = _last_turn_text(records) - result = _extract_json_object(last_text) if last_text else None + final_text = _final_result_text(records) + result = _extract_json_object(final_text) if final_text else None issues = _issues_list(result) if issues is None: + emit_boundary_event( + cwd, + "review_verdict_recorder", + "SubagentStop", + "record", + "unparseable-result", + session_id=session_id, + ) return - findings_files = _findings_files(issues) - - cwd = resolve_cwd(payload) - session_id = payload.get("session_id") + findings_files = _findings_files(issues, cwd) + cwd_resolved = Path(cwd).resolve() for file_path in in_scope: - target = Path(file_path) - if not target.is_absolute(): - target = Path(cwd) / target + target = _resolve_under_cwd(file_path, cwd) + if target is None or not target.is_relative_to(cwd_resolved): + continue # unresolvable, or outside cwd containment (Fix #5) file_hash = _hash_file(target) if file_hash is None: - continue # deleted/unreadable in-scope file -- skip this one only - outcome = "findings" if file_path in findings_files else "pass" + continue # deleted/unreadable/non-regular/oversized -- skip this one only + outcome = "findings" if target in findings_files else "pass" emit_review_verdict( cwd, subagent_type, file_path, file_hash, outcome, session_id=session_id ) diff --git a/plugins/dev-team/knowledge/telemetry-schema.md b/plugins/dev-team/knowledge/telemetry-schema.md index 9c0accb3e..31981621f 100644 --- a/plugins/dev-team/knowledge/telemetry-schema.md +++ b/plugins/dev-team/knowledge/telemetry-schema.md @@ -73,13 +73,26 @@ the two non-clean, explainable outcomes, with `matched_rule` set to the classification itself (`empty-final-turn` or `truncated-final-turn`); the common `clean` case and the unexplainable `unreadable` case write nothing. +**#2166 Fix #3** adds `review_verdict_recorder.py` as a `SubagentStop` +emitter, same `record` decision, same non-verdict posture: once this hook +has confirmed a dispatch's `subagent_type` IS a registered review lens (so +the dispatch SHOULD produce `review-verdicts.jsonl` rows), a degenerate exit +that would otherwise be silently indistinguishable from a legitimate no-op +instead writes one `record` row naming why, via `matched_rule` of +`missing-scope-marker` (the dispatch prompt's Step 2.1 scope marker is +missing or reformatted) or `unparseable-result` (the agent's final JSON +result couldn't be recovered, even by the tolerant extractor). The +PRE-resolution exits (unreadable transcript, unresolvable `subagent_type`, +an unregistered or registered-but-non-review `subagent_type`) stay silent — +those are legitimate no-ops, not degenerate states. + | Field | Type | Values / source | | --- | --- | --- | | `ts` | string | ISO-8601 UTC `%Y-%m-%dT%H:%M:%SZ` | | `hook` | string | Emitting hook's module name, e.g. `destructive_guard`, `verify_guard`, `pre_pr_review` (the review-corroboration gate, #1886; `pre_commit_review` is now a documented no-op and emits nothing), `telemetry`, `agent_dispatch_ledger` — or `code-review` for the CLI-emitted events (`--event doc-only`/`single-agent`/`dispatch-failure`), which carry the invoking skill's name rather than a hook module name | | `tool` | string | Hooked tool/event: `Bash`, `Write`, `Edit`, `Skill`, `Agent`, `UserPromptSubmit`, `SubagentStop` (#2188) | | `decision` | string enum | `block` \| `warn` \| `bypass` \| `intervention` \| `revert` \| `record` \| `dispatch-failure` | -| `matched_rule` | string | Rule ID from a closed vocabulary (pattern ID, hook-defined constant, bypass flag name, intervention keyword, or — for `record`/`dispatch-failure` — the dispatched review-agent's registered name, or — for `subagent_completion_guard.py`'s `record` rows — `empty-final-turn`/`truncated-final-turn`, #2188, or — for `boundary_events_write_guard.py`'s `block` rows — `ledger-write-blocked`, #2171) — never free text | +| `matched_rule` | string | Rule ID from a closed vocabulary (pattern ID, hook-defined constant, bypass flag name, intervention keyword, or — for `record`/`dispatch-failure` — the dispatched review-agent's registered name, or — for `subagent_completion_guard.py`'s `record` rows — `empty-final-turn`/`truncated-final-turn`, #2188, or — for `review_verdict_recorder.py`'s `record` rows — `missing-scope-marker`/`unparseable-result`, #2166 Fix #3, or — for `boundary_events_write_guard.py`'s `block` rows — `ledger-write-blocked`, #2171) — never free text | | `plugin_version` | string | From `.claude-plugin/plugin.json` | | `session_id` | string, optional | Opaque per-session ID, when present in the hook payload — enables joins with `session-digest.jsonl` | | `subject_hash` | string, optional | `review_gate_hash()` value (#1461) binding this event to the staged content it corroborates. A hex digest, not free text | @@ -96,7 +109,7 @@ PR-creation time, against the branch's cumulative diff, does not have that problem. `hooks/pre_pr_review.py` never emits this event. Existing rows in `boundary-events.jsonl` from before the migration remain valid history. -- **Emitter:** `hooks/lib/boundary_events.py::emit_boundary_event()`, called from `destructive_guard.py`, `verify_guard.py`, `pre_pr_review.py` (#1886), `telemetry.py` (intervention keywords), `agent_dispatch_ledger.py` (decision `record`, #1461), `subagent_completion_guard.py` (decision `record`, `tool` `SubagentStop`, #2188), `boundary_events_write_guard.py` (decision `block`, `tool` `Write`\|`Edit`\|`Bash`, `matched_rule` `ledger-write-blocked` — the PreToolUse guard blocking a direct Write/Edit/Bash write to this same ledger, #2171), the mechanically-adopted guards (`pre_tool_guard.py`, `context_ceiling_guard.py`, `bash_retry_guard.py`, `refactor_test_freeze_guard.py`, `refactor_test_bash_guard.py`, `refactor_test_revert_guard.py` (decision `revert`, #906), `contract_version_guard.py`, `mutation_testing_smoke_gate.py`, `mutation_gate.py`, `tdd_guard.py`), and `boundary_events.py`'s own CLI (`--event dispatch-failure`, decision `dispatch-failure`, #1763) invoked from `skills/code-review/SKILL.md` Step 4. `--event gate-ran --verdict {allow,block,errored}` (decision `record`, `matched_rule` of `gate-ran-`, #2037) is invoked from the repo-root `.husky/pre-commit` git hook — the real, git-native pre-commit gate (distinct from `pre_pr_review.py`, a Claude-Code-level PreToolUse hook gating `gh pr create`) — at every exit point, success or failure alike, so `${CLAUDE_PLUGIN_ROOT}/scripts/session_report.py --profile maintainer` can correlate a commit-attempt Bash record against a nearby `gate_ran` event and classify the previously-unmeasured "the gate silently never ran" population (`gate_ran_absent`) apart from a genuine internal failure (`gate_ran_errored`). This event carries no `session_id` in practice — a real git hook has no Claude Code session_id to attach — so correlation is by time proximity, not session join; see `session_report.py`'s "gate-run correlation (#2037)" section. +- **Emitter:** `hooks/lib/boundary_events.py::emit_boundary_event()`, called from `destructive_guard.py`, `verify_guard.py`, `pre_pr_review.py` (#1886), `telemetry.py` (intervention keywords), `agent_dispatch_ledger.py` (decision `record`, #1461), `subagent_completion_guard.py` (decision `record`, `tool` `SubagentStop`, #2188), `review_verdict_recorder.py` (decision `record`, `tool` `SubagentStop`, `matched_rule` `missing-scope-marker`\|`unparseable-result`, #2166 Fix #3), `boundary_events_write_guard.py` (decision `block`, `tool` `Write`\|`Edit`\|`Bash`, `matched_rule` `ledger-write-blocked` — the PreToolUse guard blocking a direct Write/Edit/Bash write to this same ledger, #2171), the mechanically-adopted guards (`pre_tool_guard.py`, `context_ceiling_guard.py`, `bash_retry_guard.py`, `refactor_test_freeze_guard.py`, `refactor_test_bash_guard.py`, `refactor_test_revert_guard.py` (decision `revert`, #906), `contract_version_guard.py`, `mutation_testing_smoke_gate.py`, `mutation_gate.py`, `tdd_guard.py`), and `boundary_events.py`'s own CLI (`--event dispatch-failure`, decision `dispatch-failure`, #1763) invoked from `skills/code-review/SKILL.md` Step 4. `--event gate-ran --verdict {allow,block,errored}` (decision `record`, `matched_rule` of `gate-ran-`, #2037) is invoked from the repo-root `.husky/pre-commit` git hook — the real, git-native pre-commit gate (distinct from `pre_pr_review.py`, a Claude-Code-level PreToolUse hook gating `gh pr create`) — at every exit point, success or failure alike, so `${CLAUDE_PLUGIN_ROOT}/scripts/session_report.py --profile maintainer` can correlate a commit-attempt Bash record against a nearby `gate_ran` event and classify the previously-unmeasured "the gate silently never ran" population (`gate_ran_absent`) apart from a genuine internal failure (`gate_ran_errored`). This event carries no `session_id` in practice — a real git hook has no Claude Code session_id to attach — so correlation is by time proximity, not session join; see `session_report.py`'s "gate-run correlation (#2037)" section. - **Consent:** ALWAYS-ON — not gated by `DEV_TEAM_TELEMETRY`. Local-only, rule-IDs-only safety/accountability channel; no observability holes by design. - **Fail-open:** every exception in the emit helper is swallowed — never changes the calling hook's exit code, stdout, or stderr. - **Consumers:** `skills/session-review/SKILL.md`, `skills/harness-audit/SKILL.md`, `agents/session-analysis.md`, `skills/cost-report/`, `skills/run-report/SKILL.md` (#1167), `hooks/lib/review_gate_corroboration.py` (#1461 `record` rows; #1763 also reads `dispatch-failure` rows as negative evidence for the gate veto), future `agent-telemetry` cross-machine aggregation (#178). diff --git a/plugins/dev-team/skills/code-review/scripts/repo_invariants.py b/plugins/dev-team/skills/code-review/scripts/repo_invariants.py index cbad32641..5b9512989 100755 --- a/plugins/dev-team/skills/code-review/scripts/repo_invariants.py +++ b/plugins/dev-team/skills/code-review/scripts/repo_invariants.py @@ -610,14 +610,22 @@ def check_contract_failure_shapes_documented(changed_files=None) -> list[dict]: "invariant targets, not zero" ), "plugins/dev-team/hooks/review_verdict_recorder.py": ( - "reads attributionAgent/agentId only through session_log.records " - "(attribution_agent_of/join_dispatch_agent_ids) -- never a raw " - "field access; the 'attributionAgent' occurrences are all in this " - "file's own module docstring, recording #2166 Step 2.3's own " - "pre-implementation spike finding against 124 real subagent " - "transcripts (mirrors cost_meter.py's entry above: prose " - "documenting the harness field this hook's decisions are based on, " - "not a second parsing implementation)" + "attributionAgent is read only through session_log.records " + "(attribution_agent_of); the 'attributionAgent' occurrences this " + "check's own regex matches are all in this file's own module " + "docstring, recording #2166 Step 2.3's own pre-implementation spike " + "finding against 124 real subagent transcripts (mirrors " + "cost_meter.py's entry above: prose documenting the harness field " + "this hook's decisions are based on, not a second parsing " + "implementation). `agentId` is NOT one of this check's own scanned " + "identifiers (see _TRANSCRIPT_FIELD_RE above) and is read directly " + "as a plain top-level field (`_own_agent_id`'s `rec.get(\"agentId\")`) " + "rather than through session_log -- correcting this entry's prior, " + "inaccurate 'never a raw field access' claim about it (#2166 Fix " + "#9, correctness review). This file's own transcript reader, " + "`_read_transcript_records`, delegates to " + "session_log.records.iter_file_records (Fix #9) rather than " + "carrying a second whole-file reader" ), "plugins/dev-team/hooks/lib/pricing.py": ( "reads a pre-extracted usage dict's known numeric fields " diff --git a/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py b/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py index 946331772..0b7ba2aeb 100644 --- a/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py +++ b/plugins/dev-team/tests/hooks/test_review_verdict_recorder.py @@ -26,6 +26,7 @@ if str(_p) not in sys.path: sys.path.insert(0, str(_p)) +import plugin_version # type: ignore[import-not-found] import review_verdict_recorder as recorder from review_verdicts import SCOPE_MARKER_PREFIX # type: ignore[import-not-found] @@ -69,20 +70,65 @@ def _dispatch_row(in_scope_files: list[str], agent_id: str = "agent-1") -> dict: } -def _result_row(result: dict, attribution_agent: str, agent_id: str = "agent-1") -> dict: - """The subagent transcript's final (result) turn: an assistant row - carrying the native `attributionAgent` field and the agent's JSON - result as text.""" - return { - "type": "assistant", - "isSidechain": True, - "agentId": agent_id, - "attributionAgent": attribution_agent, - "message": { - "role": "assistant", - "content": [{"type": "text", "text": json.dumps(result)}], +def _handback_tail(message_text: str, attribution_agent: str, agent_id: str = "agent-1") -> list[dict]: + """The subagent transcript's REAL result-bearing tail (Fix #1, #2166 + correctness review) -- verified against a real transcript in this + session's own corpus, not fabricated: a `SubagentHandback` tool_use + block whose OWN `input.message` carries `message_text`, followed by its + `tool_result` ack, followed by a short wrap-up assistant turn whose text + is deliberately NOT the result (mirrors real completions -- "Report + delivered."). The pre-fix hook read the transcript's literal last row as + the JSON result and so mishandled this exact shape; these three rows are + what `_handback_message_text` (`hooks/review_verdict_recorder.py`) now + scans backward for.""" + tool_use_id = f"toolu_{agent_id}" + return [ + { + "type": "assistant", + "isSidechain": True, + "agentId": agent_id, + "attributionAgent": attribution_agent, + "message": { + "role": "assistant", + "stop_reason": "tool_use", + "content": [ + { + "type": "tool_use", + "id": tool_use_id, + "name": "SubagentHandback", + "input": {"message": message_text}, + } + ], + }, }, - } + { + "type": "user", + "agentId": agent_id, + "message": { + "role": "user", + "content": [{"type": "tool_result", "tool_use_id": tool_use_id}], + }, + }, + { + "type": "assistant", + "isSidechain": True, + "agentId": agent_id, + "attributionAgent": attribution_agent, + "message": { + "role": "assistant", + "stop_reason": "end_turn", + "content": [{"type": "text", "text": "Report delivered."}], + }, + }, + ] + + +def _result_rows(result: dict, attribution_agent: str, agent_id: str = "agent-1") -> list[dict]: + """The realistic transcript tail (`_handback_tail`) carrying `result` as + the handback's JSON message -- the drop-in replacement for every call + site that used to build a single fabricated final row whose text WAS the + JSON (a shape the real harness never produces, see `_handback_tail`).""" + return _handback_tail(json.dumps(result), attribution_agent, agent_id) def _write_file(tmp_path: Path, rel_path: str, content: str = "hello\n") -> None: @@ -125,6 +171,16 @@ def _read_rows(tmp_path: Path) -> list[dict]: return [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines() if line] +_BOUNDARY_EVENTS_REL = Path(".claude") / "metrics" / "boundary-events.jsonl" + + +def _read_boundary_events(tmp_path: Path) -> list[dict]: + path = tmp_path / _BOUNDARY_EVENTS_REL + if not path.is_file(): + return [] + return [json.loads(line) for line in path.read_text(encoding="utf-8").splitlines() if line] + + # --------------------------------------------------------------------------- # Scenario: clean review agent result records a pass row per in-scope file # --------------------------------------------------------------------------- @@ -139,7 +195,7 @@ def test_clean_result_records_a_pass_row_per_in_scope_file(tmp_path: Path) -> No tmp_path, [ _dispatch_row(files), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_REVIEW_AGENT}", ), @@ -157,7 +213,7 @@ def test_clean_result_records_a_pass_row_per_in_scope_file(tmp_path: Path) -> No assert row["lens"] == _REVIEW_AGENT assert row["file_content_hash"] == _sha256(f"content of {f}\n") assert row["session_id"] == "sess-1" - assert "plugin_version" in row + assert row["plugin_version"] == plugin_version.shipped_version() # --------------------------------------------------------------------------- @@ -174,7 +230,7 @@ def test_findings_result_records_mixed_verdict_per_file(tmp_path: Path) -> None: tmp_path, [ _dispatch_row(files), - _result_row( + *_result_rows( { "status": "warn", "issues": [ @@ -210,7 +266,7 @@ def test_unregistered_subagent_type_writes_no_rows(tmp_path: Path) -> None: tmp_path, [ _dispatch_row(["a.py"]), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, _UNREGISTERED_AGENT, ), @@ -231,7 +287,7 @@ def test_registered_non_review_subagent_type_writes_no_rows(tmp_path: Path) -> N tmp_path, [ _dispatch_row(["a.py"]), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_NON_REVIEW_AGENT}", ), @@ -251,7 +307,7 @@ def test_empty_scope_marker_writes_no_rows(tmp_path: Path) -> None: tmp_path, [ _dispatch_row([]), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_REVIEW_AGENT}", ), @@ -272,7 +328,7 @@ def test_single_file_scope_writes_exactly_one_row(tmp_path: Path) -> None: tmp_path, [ _dispatch_row(["only.py"]), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_REVIEW_AGENT}", ), @@ -298,7 +354,7 @@ def test_deleted_in_scope_file_is_skipped_others_still_written(tmp_path: Path) - tmp_path, [ _dispatch_row(["a.py", "b.py", "c.py"]), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_REVIEW_AGENT}", ), @@ -307,6 +363,7 @@ def test_deleted_in_scope_file_is_skipped_others_still_written(tmp_path: Path) - assert _run_main(tmp_path, transcript) == 0 rows = _read_rows(tmp_path) assert {r["file_path"] for r in rows} == {"a.py", "c.py"} + assert all(r["outcome"] == "pass" for r in rows) # --------------------------------------------------------------------------- @@ -352,7 +409,7 @@ def test_missing_scope_marker_writes_no_rows(tmp_path: Path) -> None: "content": "Review these files: a.py, b.py\n", }, }, - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_REVIEW_AGENT}", ), @@ -378,7 +435,7 @@ def test_trusts_declared_scope_without_reverifying_against_diff(tmp_path: Path) tmp_path, [ _dispatch_row(["trusted.py"]), - _result_row( + *_result_rows( {"status": "pass", "issues": [], "summary": "clean"}, f"dev-team:{_REVIEW_AGENT}", ), @@ -398,27 +455,61 @@ def test_trusts_declared_scope_without_reverifying_against_diff(tmp_path: Path) def test_malformed_final_json_result_writes_no_rows(tmp_path: Path) -> None: + """The `SubagentHandback` call is present (the primary Fix #1 path), but + its own `input.message` isn't recoverable as JSON, and the wrap-up + turn's "Report delivered." text isn't JSON either -- so the fallback + also finds nothing usable.""" _write_file(tmp_path, "a.py") transcript = _write_transcript( tmp_path, [ _dispatch_row(["a.py"]), - { - "type": "assistant", - "isSidechain": True, - "agentId": "agent-1", - "attributionAgent": f"dev-team:{_REVIEW_AGENT}", - "message": { - "role": "assistant", - "content": [{"type": "text", "text": "not json output at all"}], - }, - }, + *_handback_tail("not json output at all", f"dev-team:{_REVIEW_AGENT}"), ], ) assert _run_main(tmp_path, transcript) == 0 assert _read_rows(tmp_path) == [] +# --------------------------------------------------------------------------- +# Tolerant JSON-recovery path (Fix #7, test review): `_extract_json_object`'s +# fenced-block/prose-preamble recovery branches had zero test coverage -- +# only the "no JSON at all" failure path (above) was tested. +# --------------------------------------------------------------------------- + + +def test_fenced_json_result_in_handback_message_is_recovered(tmp_path: Path) -> None: + """A real handback message is prose PLUS a ```json fenced block (per + `_handback_message_text`'s own docstring citation of a real transcript), + sometimes with a trailing sentence after the fence too. The tolerant + extractor must recover the JSON object from that, not just from a + message that IS raw JSON with nothing else.""" + files = ["a.py", "b.py"] + for f in files: + _write_file(tmp_path, f) + result = { + "status": "warn", + "issues": [{"severity": "warning", "file": "b.py", "message": "something"}], + "summary": "1 issue", + } + fenced_message = ( + "Reviewed the following files for issues.\n\n" + "```json\n" + json.dumps(result) + "\n```\n\n" + "Findings delivered above." + ) + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(files), + *_handback_tail(fenced_message, f"dev-team:{_REVIEW_AGENT}"), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + by_path = {r["file_path"]: r["outcome"] for r in rows} + assert by_path == {"a.py": "pass", "b.py": "findings"} + + # --------------------------------------------------------------------------- # Unit-level coverage of the documented Task/Agent dispatch-join fallback # (used only when attributionAgent is absent from every record -- the spike @@ -484,3 +575,148 @@ def test_fallback_resolves_subagent_type_via_task_agent_join(tmp_path: Path) -> records = recorder._read_transcript_records(subagent_transcript) resolved = recorder._resolve_subagent_type(subagent_transcript, records) assert resolved == _REVIEW_AGENT + + +# --------------------------------------------------------------------------- +# Scenario: a finding is matched to its scope-marker file across different +# path forms (Fix #2, correctness review). +# --------------------------------------------------------------------------- + + +def test_findings_match_across_differing_path_forms(tmp_path: Path) -> None: + """Real review-agent transcripts in this session's own corpus report the + SAME file in different path forms (relative vs. absolute) across + different agents. The scope marker here declares the relative form; + `issues[].file` reports the absolute form of the SAME file -- raw + string equality would silently miss this and record a false `pass`.""" + _write_file(tmp_path, "a.py") + absolute_a = str((tmp_path / "a.py").resolve()) + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py"]), + *_result_rows( + { + "status": "warn", + "issues": [{"severity": "warning", "file": absolute_a, "message": "x"}], + "summary": "1 issue", + }, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + rows = _read_rows(tmp_path) + assert len(rows) == 1 + assert rows[0]["outcome"] == "findings" + # The marker's own spelling is preserved in the row -- only the + # comparison is normalized. + assert rows[0]["file_path"] == "a.py" + + +# --------------------------------------------------------------------------- +# Scenario: degenerate exits past subagent_type confirmation record a +# boundary event naming the reason; pre-resolution exits stay silent +# (Fix #3, correctness review). +# --------------------------------------------------------------------------- + + +def test_missing_scope_marker_emits_boundary_event(tmp_path: Path) -> None: + transcript = _write_transcript( + tmp_path, + [ + { + "type": "user", + "agentId": "agent-1", + "message": {"role": "user", "content": "Review these files: a.py, b.py\n"}, + }, + *_result_rows( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + events = _read_boundary_events(tmp_path) + assert len(events) == 1 + assert events[0]["hook"] == "review_verdict_recorder" + assert events[0]["tool"] == "SubagentStop" + assert events[0]["decision"] == "record" + assert events[0]["matched_rule"] == "missing-scope-marker" + + +def test_unparseable_result_emits_boundary_event(tmp_path: Path) -> None: + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py"]), + *_handback_tail("not json output at all", f"dev-team:{_REVIEW_AGENT}"), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + events = _read_boundary_events(tmp_path) + assert len(events) == 1 + assert events[0]["matched_rule"] == "unparseable-result" + + +def test_unregistered_subagent_type_emits_no_boundary_event(tmp_path: Path) -> None: + """PRE-resolution exits stay silent -- an unregistered `subagent_type` + is a legitimate no-op, not a degenerate state (Fix #3).""" + _write_file(tmp_path, "a.py") + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row(["a.py"]), + *_result_rows( + {"status": "pass", "issues": [], "summary": "clean"}, + _UNREGISTERED_AGENT, + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_boundary_events(tmp_path) == [] + + +# --------------------------------------------------------------------------- +# Scenario: `_hash_file` bounds its read (Fix #4, security review). +# --------------------------------------------------------------------------- + + +def test_hash_file_skips_files_over_max_size_cap(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setattr(recorder, "_MAX_HASH_FILE_BYTES", 10) + big = tmp_path / "big.py" + big.write_bytes(b"x" * 11) + assert recorder._hash_file(big) is None + + +def test_hash_file_skips_non_regular_files(tmp_path: Path) -> None: + fifo = tmp_path / "fifo" + os.mkfifo(str(fifo)) + assert recorder._hash_file(fifo) is None + + +# --------------------------------------------------------------------------- +# Scenario: a scope-marker path that resolves outside `cwd` is never +# read/hashed (Fix #5, security review). +# --------------------------------------------------------------------------- + + +def test_scope_marker_path_traversal_is_not_read(tmp_path: Path) -> None: + outside = tmp_path.parent / f"{tmp_path.name}-outside-verdict-recorder.py" + outside.write_text("secret\n", encoding="utf-8") + try: + transcript = _write_transcript( + tmp_path, + [ + _dispatch_row([f"../{outside.name}"]), + *_result_rows( + {"status": "pass", "issues": [], "summary": "clean"}, + f"dev-team:{_REVIEW_AGENT}", + ), + ], + ) + assert _run_main(tmp_path, transcript) == 0 + assert _read_rows(tmp_path) == [] + finally: + outside.unlink(missing_ok=True) From 56775d877c5fb54b288a2bc686abcee488375f51 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 16:40:54 +0000 Subject: [PATCH 10/10] fix(hooks): converge ledger-name ownership, canonicalize verdict paths, dedupe lens check Backstop review of #2166 + #2171 surfaced six findings: - boundary_events.py's LOG_NAME (promoted from private _LOG_NAME) is now the single source of truth for the ledger filename; review_dispatch_ledger.LEDGER_STREAM is an alias of it, and boundary_events_write_guard.py + review_gate_corroboration.py import from the true owner instead of re-declaring/importing a narrower copy. repo_invariants.check_ledger_filename_single_sourced asserts the alias stays an identity going forward (4/8 reviewers independently reported this). - review_verdict_recorder.py now emits the canonical cwd-relative POSIX file_path (falling back to an absolute resolved form) instead of the raw scope-marker path, so the same file groups under one lookup key for the future #2167 reader; telemetry-schema.md documents the new field shape. - Renamed process()'s `target` to `resolved` to match _findings_files(). - Corrected _resolve_under_cwd's docstring from "outside the repo" to "outside cwd", matching its actual containment logic. - Extracted review_agent_registry.is_registered_review_lens() to replace the strip-prefix + registry-read + membership check hand-rolled independently in agent_dispatch_ledger.py, boundary_events.py, and review_verdict_recorder.py. - Fixed stale docstrings: review_verdicts.py no longer misnames review_verdict_recorder.py as load_verdicts()'s consumer; the cost_meter.py citation now points at its inline comment, not its module docstring; SKILL.md's scope-marker section names its live consumer. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n --- .../dev-team/hooks/agent_dispatch_ledger.py | 28 ++++++------- .../hooks/boundary_events_write_guard.py | 7 ++-- plugins/dev-team/hooks/lib/boundary_events.py | 37 +++++++++-------- .../hooks/lib/review_agent_registry.py | 26 ++++++++++++ .../hooks/lib/review_dispatch_ledger.py | 13 ++++-- .../hooks/lib/review_gate_corroboration.py | 2 +- plugins/dev-team/hooks/lib/review_verdicts.py | 6 ++- .../dev-team/hooks/review_verdict_recorder.py | 36 +++++++++++------ .../dev-team/knowledge/telemetry-schema.md | 2 +- plugins/dev-team/skills/code-review/SKILL.md | 2 +- .../code-review/scripts/repo_invariants.py | 40 +++++++++++++++++++ .../tests/hooks/test_review_agent_registry.py | 28 +++++++++++++ .../hooks/test_xunit_v3_operator_gate.py | 2 +- .../tests/scripts/test_repo_invariants.py | 27 +++++++++++++ 14 files changed, 199 insertions(+), 57 deletions(-) diff --git a/plugins/dev-team/hooks/agent_dispatch_ledger.py b/plugins/dev-team/hooks/agent_dispatch_ledger.py index 583fea805..f155c19bb 100755 --- a/plugins/dev-team/hooks/agent_dispatch_ledger.py +++ b/plugins/dev-team/hooks/agent_dispatch_ledger.py @@ -87,11 +87,7 @@ sys.path.insert(0, str(_LIB_DIR)) from boundary_events import emit_boundary_event as _emit_boundary_event -from review_agent_registry import ( - default_agents_dir, - read_registered_review_agent_names, - strip_plugin_prefix, -) +from review_agent_registry import is_registered_review_lens, strip_plugin_prefix from review_gate_hash import ( EMPTY_DIGEST, branch_diff_gate_hash, @@ -124,24 +120,24 @@ def main() -> int: if not isinstance(subagent_type, str) or not subagent_type: return 0 - # Normalize the plugin-qualified dispatch form ("dev-team:doc-review") to - # the bare name the registry's closed set uses, so the plugin's normal, - # installed invocation form is recognized identically to a bare-named one. - subagent_type = strip_plugin_prefix(subagent_type) - - # #1904 item 1: `read_registered_review_agent_names()` returns `None` on - # a registry read failure, distinct from a genuine `frozenset()` — but - # this is the WRITE/POSITIVE-evidence side (recording that a dispatch - # happened), where collapsing `None` to "don't record" is the safe + # `is_registered_review_lens()` (review_agent_registry.py) owns the + # strip-prefix + registry-read + membership check, including the + # "unreadable registry collapses to skip" posture — this is the + # WRITE/POSITIVE-evidence side (recording that a dispatch happened), + # where collapsing an unreadable registry to "don't record" is the safe # direction: narrowing corroboration can only narrow, never widen, what # counts as a passing gate later. - registered = read_registered_review_agent_names(default_agents_dir()) - if not registered or subagent_type not in registered: + if not is_registered_review_lens(subagent_type): # Not a real, registered review agent, or the registry could not be # read at all — never recorded, not even as a rejected/flagged entry # (module docstring). return 0 + # Normalize the plugin-qualified dispatch form ("dev-team:doc-review") to + # the bare name the registry's closed set uses, so the plugin's normal, + # installed invocation form is recognized identically to a bare-named one. + subagent_type = strip_plugin_prefix(subagent_type) + cwd = payload.get("cwd") or "." session_id = payload.get("session_id") tool_name = payload.get("tool_name") or "Agent" diff --git a/plugins/dev-team/hooks/boundary_events_write_guard.py b/plugins/dev-team/hooks/boundary_events_write_guard.py index 08060e177..64a634e0d 100755 --- a/plugins/dev-team/hooks/boundary_events_write_guard.py +++ b/plugins/dev-team/hooks/boundary_events_write_guard.py @@ -73,10 +73,9 @@ sys.path.insert(0, str(_LIB_DIR)) import artifact_paths +from boundary_events import LOG_NAME as _LEDGER_NAME from boundary_events import cli_event_names as _cli_event_names from boundary_events import emit_boundary_event as _emit_boundary_event -from review_dispatch_ledger import LEDGER_STREAM as _LEDGER_NAME -from review_dispatch_ledger import resolve_stream as _resolve_ledger_stream from stdin_json import read_stdin_json # type: ignore[import-not-found] @@ -147,7 +146,9 @@ def targets_ledger(file_path: str, cwd: str) -> bool: if os.path.basename(candidate_norm) != _LEDGER_NAME: return False - ledger = _resolve_ledger_stream("metrics", _LEDGER_NAME, Path(cwd) if cwd else Path.cwd()) + ledger = artifact_paths.resolve_file( + "metrics", _LEDGER_NAME, Path(cwd) if cwd else Path.cwd(), migrate=False + ) ledger_norm = os.path.abspath(str(ledger)) if candidate_norm == ledger_norm: return True diff --git a/plugins/dev-team/hooks/lib/boundary_events.py b/plugins/dev-team/hooks/lib/boundary_events.py index 9bef08cd2..a78b666f5 100644 --- a/plugins/dev-team/hooks/lib/boundary_events.py +++ b/plugins/dev-team/hooks/lib/boundary_events.py @@ -34,7 +34,14 @@ import atomic_state import plugin_version -_LOG_NAME = "boundary-events.jsonl" +#: This stream's filename — the single source of truth every other reader +#: or guard that needs to name it (`review_dispatch_ledger.LEDGER_STREAM`, +#: `boundary_events_write_guard.py`, `review_gate_corroboration.py`) must +#: import from here rather than re-declaring its own literal (backstop +#: review finding, #2166 + #2171: this filename previously had three +#: independent homes). Public because this module is the one that actually +#: writes the ledger and therefore owns its name. +LOG_NAME = "boundary-events.jsonl" # Test-only injection point (see `_write_jsonl_line` below and # `atomic_state.race_window_delay`'s own docstring): unset in production, a @@ -142,7 +149,7 @@ def emit_boundary_event( """ try: base = Path(cwd) if cwd else Path.cwd() - log = artifact_paths.resolve_file("metrics", _LOG_NAME, base) + log = artifact_paths.resolve_file("metrics", LOG_NAME, base) log.parent.mkdir(parents=True, exist_ok=True) payload = { @@ -382,27 +389,23 @@ def _main() -> int: import review_agent_registry except Exception: # noqa: BLE001 - fail-open: an unavailable registry module never records return 0 - # #1904 item 1: `read_registered_review_agent_names()` returns `None` - # on a registry read failure, distinct from a genuine `frozenset()` - # — but this is the "should this get recorded at all" gate, so - # collapsing `None` to "don't record" is the safe direction here too - # (matches `agent_dispatch_ledger.py`'s own posture; see module - # comment): a lost write only means less evidence is recorded, never - # more. - registered = review_agent_registry.read_registered_review_agent_names( - review_agent_registry.default_agents_dir() - ) + # `is_registered_review_lens()` (review_agent_registry.py) owns the + # strip-prefix + registry-read + membership check, including the + # "unreadable registry collapses to skip" posture — previously + # hand-rolled independently here, in `agent_dispatch_ledger.py`, and + # in `review_verdict_recorder.py` (backstop review finding, #2166 + + # #2171 consolidation). + if not review_agent_registry.is_registered_review_lens(args.agent): + # Unregistered name, or the registry could not be read at all -> + # silently NOT recorded (matches agent_dispatch_ledger.py's own + # posture; see module comment). + return 0 # Normalize the plugin-qualified form ("dev-team:security-review") # to the bare stem the registry's closed set uses, matching # agent_dispatch_ledger.py's own normalization exactly — otherwise # the plugin's normal, installed invocation form would be silently # dropped as "unregistered". agent = review_agent_registry.strip_plugin_prefix(args.agent) - if not registered or agent not in registered: - # Unregistered name, or the registry could not be read at all -> - # silently NOT recorded (matches agent_dispatch_ledger.py's own - # posture; see module comment). - return 0 hook, tool, decision = _CLI_AGENT_EVENTS[args.event] emit_boundary_event( args.cwd, diff --git a/plugins/dev-team/hooks/lib/review_agent_registry.py b/plugins/dev-team/hooks/lib/review_agent_registry.py index 0e1e30d5d..d4c165733 100644 --- a/plugins/dev-team/hooks/lib/review_agent_registry.py +++ b/plugins/dev-team/hooks/lib/review_agent_registry.py @@ -80,6 +80,32 @@ def default_agents_dir() -> Path: return Path(__file__).resolve().parents[2] / "agents" +def is_registered_review_lens(subagent_type: str) -> bool: + """True when `subagent_type` (raw, or plugin-qualified like + `dev-team:security-review`) names a registered `agents/*-review.md` + review lens. + + Encapsulates the repeated three-step check — `strip_plugin_prefix` -> + read the registered review-agent set via `default_agents_dir()` -> + membership test — previously hand-rolled independently in + `hooks/agent_dispatch_ledger.py`, `hooks/lib/boundary_events.py`, and + `hooks/review_verdict_recorder.py` (backstop review finding, #2166 + + #2171 — this repo's own `hooks/lib/review_dispatch_ledger.py` module + docstring already applied the identical consolidation lesson to a + sibling predicate). + + An unreadable registry (`read_registered_review_agent_names()` returns + `None`) collapses to `False`, matching every prior call site's own + "don't record"/"skip" posture exactly: a lost registry read only + narrows what counts as a registered review lens, never widens it. + """ + if not subagent_type: + return False + name = strip_plugin_prefix(subagent_type) + registered = read_registered_review_agent_names(default_agents_dir()) + return bool(registered) and name in registered + + def read_registered_review_agent_names(agents_dir: Path) -> frozenset[str] | None: """Checked variant of `registered_review_agent_names()` that owns the read-failure-vs-empty distinction (#1904 item 1) instead of forcing every diff --git a/plugins/dev-team/hooks/lib/review_dispatch_ledger.py b/plugins/dev-team/hooks/lib/review_dispatch_ledger.py index c1fceaba3..1b5dbd797 100644 --- a/plugins/dev-team/hooks/lib/review_dispatch_ledger.py +++ b/plugins/dev-team/hooks/lib/review_dispatch_ledger.py @@ -30,9 +30,16 @@ sys.path.insert(0, str(_LIB_DIR)) import artifact_paths - -#: The stream every review dispatch is deterministically recorded to. -LEDGER_STREAM = "boundary-events.jsonl" +import boundary_events + +#: The stream every review dispatch is deterministically recorded to. An +#: alias of `boundary_events.LOG_NAME` (the module that actually writes the +#: ledger and owns its filename), not a fresh literal — kept as this +#: module's own public name for its existing callers (backstop review +#: finding, #2166 + #2171: this filename previously had three independent +#: homes; `repo_invariants.check_ledger_filename_single_sourced` asserts +#: this stays an identity, not just an equal value). +LEDGER_STREAM = boundary_events.LOG_NAME #: The ledger rows that denote a review dispatch. _LEDGER_HOOK = "agent_dispatch_ledger" diff --git a/plugins/dev-team/hooks/lib/review_gate_corroboration.py b/plugins/dev-team/hooks/lib/review_gate_corroboration.py index 40135868b..3ac844e7a 100644 --- a/plugins/dev-team/hooks/lib/review_gate_corroboration.py +++ b/plugins/dev-team/hooks/lib/review_gate_corroboration.py @@ -75,9 +75,9 @@ import artifact_paths import metrics_query import review_agent_registry +from boundary_events import LOG_NAME as _LEDGER_STREAM_NAME from boundary_events import TS_FORMAT as _TS_FORMAT -_LEDGER_STREAM_NAME = "boundary-events.jsonl" _EVENT_TYPE = "agent_dispatch_ledger" _DECISION = "record" diff --git a/plugins/dev-team/hooks/lib/review_verdicts.py b/plugins/dev-team/hooks/lib/review_verdicts.py index be7aad24d..96636b1d7 100644 --- a/plugins/dev-team/hooks/lib/review_verdicts.py +++ b/plugins/dev-team/hooks/lib/review_verdicts.py @@ -24,8 +24,10 @@ either — an absent file, a corrupted line, or a stale `plugin_version` row all degrade to "no usable rows" rather than an exception. -`load_verdicts()` has no consumer in this slice (Step 2.2) — `#2167`'s -`review_verdict_recorder.py` is the first one; see this module's own test +`load_verdicts()` remains unconsumed in this slice (Step 2.2) — Step 2.3's +`hooks/review_verdict_recorder.py` is a writer only, it never calls +`load_verdicts()`; `#2167` (a not-yet-built slice) is the first intended +consumer; see this module's own test `test_load_verdicts_has_no_other_consumers` for the mechanical check that enforces that boundary. diff --git a/plugins/dev-team/hooks/review_verdict_recorder.py b/plugins/dev-team/hooks/review_verdict_recorder.py index c693adde0..702a1d34d 100755 --- a/plugins/dev-team/hooks/review_verdict_recorder.py +++ b/plugins/dev-team/hooks/review_verdict_recorder.py @@ -76,8 +76,8 @@ sys.path.insert(0, str(_LIB_DIR)) # hooks/ -> scripts/lib/session_log/ is a documented reverse-dependency -# exception (see hooks/lib/cost_meter.py's own module docstring for the -# full rationale): session_log/ ships INSIDE this same plugin package, +# exception (see hooks/lib/cost_meter.py's identical import for the full +# rationale): session_log/ ships INSIDE this same plugin package, # always present wherever this hook runs, and session_log itself imports # nothing from hooks/lib/ (no cycle). Mirrors cost_meter.py's own # sys.path.insert + bare-package-import MECHANISM, not its directionality. @@ -87,8 +87,7 @@ from boundary_events import emit_boundary_event # type: ignore[import-not-found] from review_agent_registry import ( # type: ignore[import-not-found] - default_agents_dir, - read_registered_review_agent_names, + is_registered_review_lens, strip_plugin_prefix, ) from review_verdicts import ( # type: ignore[import-not-found] @@ -324,7 +323,7 @@ def _resolve_under_cwd(file_path: str, cwd) -> Path | None: This is the single normalized form both: * the path-traversal containment check (Fix #5, security review: a scope marker declaring `../../../../etc/passwd`-style paths must - not be read/hashed outside the repo), and + not be read/hashed outside `cwd`), and * the scope-marker/`issues[].file` membership comparison (Fix #2, correctness review: real review-agent transcripts in this session's own corpus report the SAME file in different path forms — @@ -425,8 +424,7 @@ def process(payload: dict) -> None: if not subagent_type: return - registered = read_registered_review_agent_names(default_agents_dir()) - if not registered or subagent_type not in registered: + if not is_registered_review_lens(subagent_type): return # Past this point `subagent_type` is a confirmed, registered review @@ -464,15 +462,29 @@ def process(payload: dict) -> None: cwd_resolved = Path(cwd).resolve() for file_path in in_scope: - target = _resolve_under_cwd(file_path, cwd) - if target is None or not target.is_relative_to(cwd_resolved): + resolved = _resolve_under_cwd(file_path, cwd) + if resolved is None or not resolved.is_relative_to(cwd_resolved): continue # unresolvable, or outside cwd containment (Fix #5) - file_hash = _hash_file(target) + file_hash = _hash_file(resolved) if file_hash is None: continue # deleted/unreadable/non-regular/oversized -- skip this one only - outcome = "findings" if target in findings_files else "pass" + outcome = "findings" if resolved in findings_files else "pass" + # Canonical, cwd-relative POSIX form (Fix #2, backstop review, + # #2166 + #2171) -- not the raw scope-marker `file_path`, which can + # name the same file in different forms (relative vs. absolute) + # across different dispatch prompts. Decision 3 makes + # `(lens, file_path, file_content_hash)` the future #2167 reader's + # lookup key, so the same file's rows must consistently group under + # one canonical path. Falls back to the absolute resolved form only + # if `relative_to` fails -- not expected here, since the + # containment check above already excludes anything outside + # `cwd_resolved`. + try: + canonical_path = resolved.relative_to(cwd_resolved).as_posix() + except ValueError: + canonical_path = resolved.as_posix() emit_review_verdict( - cwd, subagent_type, file_path, file_hash, outcome, session_id=session_id + cwd, subagent_type, canonical_path, file_hash, outcome, session_id=session_id ) diff --git a/plugins/dev-team/knowledge/telemetry-schema.md b/plugins/dev-team/knowledge/telemetry-schema.md index 31981621f..1663659fe 100644 --- a/plugins/dev-team/knowledge/telemetry-schema.md +++ b/plugins/dev-team/knowledge/telemetry-schema.md @@ -148,7 +148,7 @@ omniscient verification of review depth. | --- | --- | --- | | `ts` | string | ISO-8601 UTC `%Y-%m-%dT%H:%M:%SZ` | | `lens` | string | The dispatched review agent's registered name (e.g. `structure-review`), plugin-prefix-stripped | -| `file_path` | string | One file the dispatch prompt's scope marker declared in scope | +| `file_path` | string | One file the dispatch prompt's scope marker declared in scope, in its canonical form: `cwd`-relative POSIX (forward-slash) path, not the raw form the scope marker carried — falls back to an absolute resolved POSIX path only when the file can't be expressed relative to `cwd` | | `file_content_hash` | string | sha256 hex digest of `file_path`'s content at the time the recorder ran (current content, not the content at dispatch time) | | `outcome` | string enum | `pass` \| `findings` — whether `file_path` appears in the agent's final `issues[]` | | `plugin_version` | string | From `.claude-plugin/plugin.json` | diff --git a/plugins/dev-team/skills/code-review/SKILL.md b/plugins/dev-team/skills/code-review/SKILL.md index 5b005be08..9bda4ae00 100644 --- a/plugins/dev-team/skills/code-review/SKILL.md +++ b/plugins/dev-team/skills/code-review/SKILL.md @@ -496,7 +496,7 @@ Prints a manifest naming the pack path, its byte size, and `files_omitted`. Pass - **`files_omitted` is not optional to relay.** When the manifest reports omissions (a file over the per-file cap, a binary, a body that would exhaust the budget), name those paths in each agent's prompt and tell it to open them directly. The pack body says so too, but a silently skipped file is a coverage hole that reads as a clean review. - **File scope**: pass only files matching each agent's declared scope. Skip the agent if no files match. -- **Scope marker (#2166)**: append one structured, single-line marker to every dispatch prompt, listing the exact files passed under File scope above, comma-separated: `Files in scope for this review: , , ...`. This is metadata for the (not-yet-built) verdict recorder — it changes nothing about what gets reviewed or reported this slice. +- **Scope marker (#2166)**: append one structured, single-line marker to every dispatch prompt, listing the exact files passed under File scope above, comma-separated: `Files in scope for this review: , , ...`. This is metadata for `hooks/review_verdict_recorder.py` (#2166 Step 2.3), the `SubagentStop` verdict recorder that parses it back out of the transcript — it changes nothing about what gets reviewed or reported this slice. - **Context payload** (controlled by the agent's `Context needs`): - `diff-only` → diff output only (for auto-scope or `--since` only) - `full-file` → complete files diff --git a/plugins/dev-team/skills/code-review/scripts/repo_invariants.py b/plugins/dev-team/skills/code-review/scripts/repo_invariants.py index 5b9512989..e0100fd32 100755 --- a/plugins/dev-team/skills/code-review/scripts/repo_invariants.py +++ b/plugins/dev-team/skills/code-review/scripts/repo_invariants.py @@ -69,6 +69,8 @@ # review_agent_registry.py's own docstring). Import rather than re-deriving # `_PLUGIN_ROOT / "agents"` locally. sys.path.insert(0, str(_PLUGIN_ROOT / "hooks" / "lib")) +import boundary_events +import review_dispatch_ledger from review_agent_registry import ( default_agents_dir, find_review_agent_files, @@ -869,6 +871,43 @@ def check_normative_content_single_sourced(changed_files=None) -> list[dict]: return findings +def check_ledger_filename_single_sourced(changed_files=None) -> list[dict]: + """`boundary-events.jsonl`'s filename must stay single-sourced from + `hooks/lib/boundary_events.LOG_NAME` — the module that actually writes + the ledger and therefore owns its name. + + Backstop review against #2166 + #2171 found this filename independently + hand-rolled in three homes (`boundary_events._LOG_NAME`, + `review_dispatch_ledger.LEDGER_STREAM`, and + `boundary_events_write_guard.py`'s own import), reported by 4 of 8 + reviewers (arch, domain, naming, structure) — clearing this repo's own + ratchet rule ("a mechanical finding reported twice becomes a check") + decisively. `review_dispatch_ledger.LEDGER_STREAM` is now an alias of + `boundary_events.LOG_NAME`, not a fresh literal — this check asserts + that stays an *identity*, not just an equal value, so a future edit + can't silently reintroduce a fourth independent copy with a green test + suite. + + Corpus-wide by design: this is a standing structural invariant, not a + changeset-scoped one. + """ + if review_dispatch_ledger.LEDGER_STREAM is not boundary_events.LOG_NAME: + return [ + { + "invariant": "ledger-filename-single-sourced", + "file": "plugins/dev-team/hooks/lib/review_dispatch_ledger.py", + "message": ( + "review_dispatch_ledger.LEDGER_STREAM must remain the " + "identical object as boundary_events.LOG_NAME (an alias, " + "not a fresh literal) — boundary_events.py is the module " + "that actually writes the boundary-events ledger and " + "owns its filename." + ), + } + ] + return [] + + # Registered checks. Each entry takes an optional `changed_files` list and # returns findings. See the module docstring for why that argument exists. CHECKS = [ @@ -880,6 +919,7 @@ def check_normative_content_single_sourced(changed_files=None) -> list[dict]: check_transcript_parsing_confined_to_session_log, check_churn_report_window_key_safe_access, check_normative_content_single_sourced, + check_ledger_filename_single_sourced, ] diff --git a/plugins/dev-team/tests/hooks/test_review_agent_registry.py b/plugins/dev-team/tests/hooks/test_review_agent_registry.py index 62da45d6b..155e9ff81 100644 --- a/plugins/dev-team/tests/hooks/test_review_agent_registry.py +++ b/plugins/dev-team/tests/hooks/test_review_agent_registry.py @@ -173,3 +173,31 @@ def test_strip_plugin_prefix_leaves_a_different_plugins_qualifier_unchanged() -> review_agent_registry.strip_plugin_prefix("other-plugin:doc-review") == "other-plugin:doc-review" ) + + +# --------------------------------------------------------------------------- +# is_registered_review_lens() -- consolidates the strip-prefix + registry- +# read + membership check previously hand-rolled independently in +# agent_dispatch_ledger.py, boundary_events.py, and review_verdict_recorder.py +# (backstop review finding, #2166 + #2171) +# --------------------------------------------------------------------------- + + +def test_is_registered_review_lens_true_for_a_bare_registered_name() -> None: + assert review_agent_registry.is_registered_review_lens("security-review") is True + + +def test_is_registered_review_lens_true_for_a_plugin_qualified_name() -> None: + assert review_agent_registry.is_registered_review_lens("dev-team:security-review") is True + + +def test_is_registered_review_lens_false_for_an_unregistered_name() -> None: + assert review_agent_registry.is_registered_review_lens("not-a-real-agent") is False + + +def test_is_registered_review_lens_false_for_a_non_review_team_agent() -> None: + assert review_agent_registry.is_registered_review_lens("orchestrator") is False + + +def test_is_registered_review_lens_false_for_empty_string() -> None: + assert review_agent_registry.is_registered_review_lens("") is False diff --git a/plugins/dev-team/tests/hooks/test_xunit_v3_operator_gate.py b/plugins/dev-team/tests/hooks/test_xunit_v3_operator_gate.py index 146a23b0c..a0fd1e8cf 100644 --- a/plugins/dev-team/tests/hooks/test_xunit_v3_operator_gate.py +++ b/plugins/dev-team/tests/hooks/test_xunit_v3_operator_gate.py @@ -26,7 +26,7 @@ def _events(tmp_path) -> list[dict]: - log = tmp_path / ".claude" / "metrics" / boundary_events._LOG_NAME + log = tmp_path / ".claude" / "metrics" / boundary_events.LOG_NAME if not log.is_file(): return [] return [json.loads(ln) for ln in log.read_text(encoding="utf-8").splitlines() if ln] diff --git a/plugins/dev-team/tests/scripts/test_repo_invariants.py b/plugins/dev-team/tests/scripts/test_repo_invariants.py index 5609f57b2..a80e7bdb5 100644 --- a/plugins/dev-team/tests/scripts/test_repo_invariants.py +++ b/plugins/dev-team/tests/scripts/test_repo_invariants.py @@ -684,3 +684,30 @@ def test_corpus_wide_regardless_of_changed_files(self): repo_invariants.check_churn_report_window_key_safe_access(["some/file.py"]) == repo_invariants.check_churn_report_window_key_safe_access(None) ) + + +class TestLedgerFilenameSingleSourced: + """#2166 + #2171 backstop review: the ledger filename was independently + hand-rolled in three homes (reported by 4 of 8 reviewers), converging + under this repo's own ratchet rule into this check.""" + + def test_clean_in_the_real_repo(self): + assert repo_invariants.check_ledger_filename_single_sourced() == [] + + def test_flags_when_ledger_stream_is_no_longer_the_same_object(self, monkeypatch): + # Round-tripped through bytes (not a literal) so it is guaranteed to + # be a distinct object from boundary_events.LOG_NAME, even though + # the value is equal — the check asserts identity, not equality. + drifted = repo_invariants.boundary_events.LOG_NAME.encode("utf-8").decode("utf-8") + monkeypatch.setattr(repo_invariants.review_dispatch_ledger, "LEDGER_STREAM", drifted) + + findings = repo_invariants.check_ledger_filename_single_sourced() + + assert len(findings) == 1 + assert findings[0]["invariant"] == "ledger-filename-single-sourced" + + def test_corpus_wide_regardless_of_changed_files(self): + assert ( + repo_invariants.check_ledger_filename_single_sourced(["some/file.py"]) + == repo_invariants.check_ledger_filename_single_sourced(None) + )