feat(hooks): boundary-events write guard and per-lens verdict ledger - #2195
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
…dger Part of #2171, part of #2164. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
…commands Part of #2171, part of #2164. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
…uard Opus-tier checkpoint on #2171's guard hook (security/domain/arch-review): targets_ledger() now also blocks the pre-migration legacy path (<root>/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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
…rompt Add a structured "Files in scope for this review: <path>, ..." 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
…corder 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
…s, 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
boundary_events_write_guard.py, a PreToolUse hook blocking directWrite/Edit/Bashwrites to.claude/metrics/boundary-events.jsonl(including its pre-migration legacy path) — the ledger is now writable only via the sanctionedemit_boundary_event()path, closing a self-certification gap (feat(hooks): block direct writes to the boundary-events ledger outside the sanctioned emitter #2171).hooks/lib/review_verdicts.py(an unconditional, fail-open per-lens verdict store keyed(lens, file_path, file_content_hash)) andhooks/review_verdict_recorder.py(a SubagentStop hook that resolves a completed subagent'ssubagent_typevia the harness's nativeattributionAgentsignal, parses a new dispatch-prompt scope marker, and records apass/findingsverdict per in-scope file) — this slice only writes the store; nothing reads it yet (feat(hooks): record per-lens review verdicts bound to a content hash #2166).Files in scope for this review: <path>, ...marker to/code-review's per-agent dispatch prompts (skills/code-review/SKILL.md), the shared contract between the writer and the recorder.Test Plan
10922 passed, 46 skipped(local CI mirror,scripts/ci-local.sh), plus a final10999 passed, 48 skippedfull-directory run before push.boundary_events_write_guard.py: Write/Edit/Bash write-shaped commands (redirect, heredoc,tee,sed -i, Pythonopen()) targeting the ledger — including relative/absolute/./-prefixed/symlinked-cwd/legacy-path forms — are blocked; reads (cat,grep,tail,python3 -cread-mode) are unaffected. Guard demonstrated live to both block and allow.review_verdict_recorder.py: every Slice 2 Gherkin scenario covered by a fixture-transcript test (clean pass, mixed findings, unregistered/non-reviewsubagent_type, empty/single-file scope, deleted in-scope file, malformed/missing-marker transcripts, trust-boundary). Verified live against a real subagent transcript from this session that theSubagentHandbackmessage — not the transcript's literal last row — is correctly parsed for the JSON result (a defect caught and fixed mid-review, with RED→GREEN evidence).pre_pr_review.py/review_gate_corroboration.pysuites pass unmodified.knowledge/telemetry-schema.mdupdated for both new emitters;test_schema_doc_covers_all_metrics_pathsgreen.review-verdicts.jsonlis unconditional (ALWAYS-ON), matchingboundary-events.jsonl's own posture.Closes #2166
Closes #2171
Part of #2164
🤖 Generated with Claude Code
https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n
Generated by Claude Code