feat: deterministic PreToolUse enforcement of the reviewer read-scope bound (2.3.4)

[apackeer]

Deterministic PreToolUse enforcement of the reviewer read-scope bound

Fixes #539.

Problem

The per-unit reviewer read-scope bound shipped in #538 (2.2.16) is prose on six surfaces. The field evidence on that PR's thread shows why prose alone loses against this persona: the U03 transcript carried 14 recursive greps with cross-unit globs (construction/*/*/*.md) that bypassed the first wording, and the reporter's latency table shows per-call time degrading ~30x once sibling reads push context past the ~100k-token knee - so the damage from each violation is superlinear. Per the framework's own layering (determinism belongs in tools/hooks, knowledge in agents, judgment with humans), the bound needs a deterministic twin.

Design (D1-D5 outcomes)

D1 - how the hook learns the dispatch. The conductor writes <record>/.aidlc-reviewer-dispatch.json ({reviewer, stage, unit, exempt[]}) immediately before invoking a per-unit reviewer (stage-protocol 12a step 1) and deletes it the moment the verdict is read (step 3). The record is the enforcement window. Staleness is bounded by mtime + a 6h TTL with a janitor delete (the compose-marker discipline); the worst observed pre-fix review ran ~3h, so 6h covers the pathological case. Concurrent swarm clones each have their own record tree (state is forked per worktree), so no sharding is needed. A reviewer-agent sighting with NO record is never blocked - it records an advisory drop surfaced by /aidlc --doctor (the conductor forgot the step-1 write).

D2 - enforcement matrix. The matcher is an exported pure function (evaluateReviewerScope, pinned by t220's 20-case decision table). It scans path fields (Read/Edit/Write), pattern/glob + search-root fields (Glob/Grep), and the whole command string (Bash) for construction/<seg> tokens: the dispatched unit passes; a wildcard segment or bare construction/ sweep root blocks; a concrete sibling blocks unless the full token exactly matches an exempt entry's construction/ suffix. Grep's content regex is deliberately NOT scanned (matching file content is not a file access). The spot-check carve-out became dispatch-time data: the conductor resolves the explicitly named integration point and puts that one owning file on exempt when writing the record - the reviewer can no longer self-expand scope, which is exactly the prose-vs-persona contest the issue documents (the reporter's transcripts confirm the conductor already names sibling invariants by ID at dispatch time). A shell variable in the unit segment (construction/$UNIT/...) blocks conservatively (the matcher cannot resolve it; the refusal message says to use the literal unit name).

D3 - hard-block vs advisory per harness (user decision: hard-block where possible):

Harness	Registration	Identity	Posture
Claude Code	settings.json, first PreToolUse entry (matcher Read|Edit|Write|Glob|Grep|Bash, quoted per #522)	payload agent_type (probe-verified)	Hard-block (exit 2 + stderr, live-verified)
Kiro CLI	the adapter's new reviewer-scope target, wired inside BOTH reviewer agents' own JSON configs	the scoped registration IS the identity	Hard-block (exit-2 contract, live-verified incl. graceful in-subagent handling)
Codex CLI	hooks.json PreToolUse row + one new trust entry	payload agent_type (verified on 0.142.5)	Hard-block (live-verified; the duplicate-delivery replay cache now carries stderr so a block replays faithfully)
Kiro IDE	none (deliberate)	n/a	The reworked IDE adapter (2.2.17) receives hook context via USER_PROMPT with toolArgs always empty - a pre-tool matcher has nothing to inspect, so per the porting-guide policy no dead hook ships; the 12a prose bound governs. t220 pins the deliberate absence.

Safety rails everywhere: fail-open on any ambiguity (no/stale/malformed record, unknown tool, non-reviewer agent, malformed stdin, any throw), TTY guard, health heartbeat, recordHookDrop diagnostics, and the deterministic off-switch AIDLC_DISABLE_REVIEWER_SCOPE_HOOK=1 for false-positive storms (e.g. a source tree with its own construction/ directory).

D4 - builder coverage: out of scope. The field evidence is reviewer-specific; builders have a weaker temptation, no transcripts, and a wider legitimate read surface. The record + matcher design generalizes if evidence appears.

D5 - audit. Every refusal emits REVIEWER_SCOPE_BLOCKED (Tool, Target, Stage, Unit) through the standard appendAuditEntry path; taxonomy registered across aidlc-audit.ts, audit-format.md (70 -> 71 events), 12-state-machine.md, and the hooks chapter. Audit failure never changes the block decision.

What the reviewer experiences

The blocked call never runs; the harness hands the reviewer the hook's stderr, which names the scope, the offending token, and the sanctioned alternative ("verify cross-unit claims against the passed contracts"). All three live probes showed the agent reading the reason, not retrying, and continuing the review. The human sees nothing in-session; the audit shard records each refusal.

Test evidence

• t220 (new, unit): 46 tests - the matcher decision table (28 cases incl. the adversarial-review shapes) + parse/reason helpers (in-process, pure), the dispatch-record lifecycle against the SHIPPED hook as a subprocess (block/allow/no-record advisory drop/stale janitor/malformed fail-open/off-switch/garbage stdin/audit emit), and the registration + protocol prose pins across all four harnesses (including the deliberate Kiro IDE absence).
• Collateral pins updated: t01/t02 hook roster 11 -> 12 (+ plan parity 67 -> 68), t150 codex hooks.json events (+PreToolUse), t219 quoting count 13 -> 14, coverage-registry allowlist. t48 (audit taxonomy drift), t68 (version trio), t148, t217 all green with the additions.
• Live probes (design phase, all three CLI harnesses): PreToolUse exit-2 blocking verified end to end on Claude Code (subagent agent_type in payload; block reason relayed), Codex 0.142.5 (subagent agent_type = spawned agent name; block inside a collab subagent handled gracefully), and Kiro CLI 2.7.0 (per-agent hook registration fires for the subagent's own calls only; PreToolHook blocked the tool execution relayed). Probe artifacts under the effort's tmp dir.
• Deterministic tier: smoke+unit+integration slice-evidence block in the PR conversation (run per the repo's test discipline).

Review

Two independent adversarial reviewers ran on the committed diff; all findings were reproduced before fixing.

Round 1 (codex-review, gpt-5.5) found three matcher bypasses, now pinned in t220's decision table:

1. Dot-dot traversal - construction/U03/../U01/design.md was judged on the first segment after construction/ and allowed; path components are now normalized with .. collapsed against parents before judging.
2. Bare search roots - grep -rn TODO construction / find construction name the whole tree with no construction/ literal, so the token scan missed them; a quote-stripped pass now blocks any token ending in the bare construction component while the word inside quoted content regexes stays content.
3. Codex apply_patch Delete/Move - *** Delete File: / *** Move to: directives were not fanned out to the scope check; they now forward as Edit-class mutations.

Round 2 (fresh Claude agent, verdict APPROVE-WITH-FIXES) found no blockers; all minors fixed:

4. Kiro identity precision - each Kiro registration now passes its own agent name as an argv token, forwarded as agent_type, so the record's reviewer field is compared on Kiro too (a stale record naming a different reviewer fails open instead of scoping the wrong agent).
5. Block-reason recovery hints - a reviewer blocked on a shell variable (construction/$U/) or an unquoted bare-word grep is now told exactly how to self-correct (literal unit name / quote the pattern); both are conservative blocks, not sibling reads.
6. SKILL.md antecedent - the verdict sentence now reads "the ## Review section verdict from the primary artifact" (the inserted delete step had left "its" pointing at the dispatch record).
7. Codex replay symmetry - the duplicate-delivery cache persists the answered exit code (2 or 0), never a raw crash code.
8. Two doc-count stragglers - the shipped onboarding template (projects into CLAUDE.md/AGENTS.md) and the architecture doc's test-levels row now say 12 hooks.

Round 1's consolidated report (delivered after round 2) added five more items, all folded in:

9. Troubleshooting discoverability - the guide now documents the refusal message, the off-switch, and the stale-record cleanup path (the false-positive class it names: repos with their own construction/ source directories).
10. Kiro batch writes - the adapter's write side now iterates operations[] like the read side.
11. Bounded audit emit - the blocked-call audit row acquires the lock with a 250ms budget (emit-unlocked inside) instead of the standard 5s retry, so a contended Bolt fan-out cannot slow the refuse; a starved lock drops the row and records a hook drop.
12. Rate-bounded advisory - the missing-dispatch-record drop is deduped to one line per 10 minutes.
13. Parallel-review dispatch sharding - correctly out of reach today (serial conductor loop); follow-up issue drafted to shard the record per unit before any parallel per-unit review lands.

The reviews also flagged pre-existing stale "68-event taxonomy" references in eight doc files (wrong on the v2 base already); left for a follow-up sweep rather than folded into this PR.

Base note

Originally stacked on #538; that PR (and the 2.2.17-2.2.19 wave) merged while this was in flight, so the branch is rebased onto current v2 with re-slots: version 2.2.20, test t220. The Kiro IDE registration present in the pre-rebase design was dropped after the 2.2.17 IDE hook rework made the seam unenforceable (empty toolArgs); the drop is deliberate, documented, and pinned.

Credits

The deterministic-hook proposal comes from the issue-534 reporter's transcript evidence on the #538 thread; the .reviewerignore-style negative filter was independently suggested on the issue thread - this PR is that idea at the framework's native enforcement point.

[apackeer]

Slice evidence - issue 539 final tree (tip 132c44a)

All runs from the worktree, bash tests/run-tests.sh --debug -P 8 <tier>, logs
redirected to ROOT tmp/issue-539-reviewer-scope-hook/.

Slice 1: smoke+unit

STAMP:    tests/logs/2026-07-09T15-34-28Z
TRACES:   none expected (smoke/unit tiers produce no driver ndjson)
SUMMARY:  2026-07-09T15-34-28Z/summary.txt  Result: FAIL, Failed files: 1 / 152, Failed assertions: 1 / 2567
RESULT:   smoke+unit . 151/1 . reds: t205-gate-revision-backstop (scenario 11, two-shard anchor timing) . live vars set: none (none required) . invariant grep hits: 0 (path-excluded)

Red disposition: t205 scenario 11 green-alone (bun test tests/unit/t205-gate-revision-backstop.test.ts -> 11 pass / 0 fail); known contention-sensitive scenario (shard-mtime anchor under -P 8), untouched by this diff (t205 file not in the diff).

Slice 2: integration

STAMP:    tests/logs/2026-07-09T15-34-54Z
TRACES:   tests/logs/2026-07-09T15-34-54Z/*.ndjson  (23 files, sdk-drive)
SUMMARY:  2026-07-09T15-34-54Z/summary.txt  Result: FAIL, Failed files: 2 / 99
RESULT:   integration . 97/2 . reds: t183-codekb-placement-reverify.sdk (Bedrock 'unexpected error during processing' API error at minute ~10), t72-stage-reverse-engineering (live-agent latency, 887s) . live vars set: none (Claude-SDK tier needs only claude on PATH) . invariant grep hits: 0 (path-excluded)

Red disposition: both green-alone in the filtered rerun below; both are live-SDK
tests untouched by this diff (upstream API error + latency flake).

Slice 3: red rerun (filtered, green-alone gate)

STAMP:    tests/logs/2026-07-09T15-58-08Z
TRACES:   tests/logs/2026-07-09T15-58-08Z/*.ndjson  (2 files, sdk-drive)
SUMMARY:  2026-07-09T15-58-08Z/summary.txt  Result: PASS, Failed files: 0 / 2
RESULT:   integration --filter "t72-stage-reverse-engineering|t183-codekb" . 2/0 . reds: none . live vars set: none . invariant grep hits: 0

Earlier full-tier context

An earlier full smoke+unit+integration run on the pre-review-fix tree
(tests/logs/2026-07-09T10-15-18Z, 251 files) had 3 reds: t28/t81 (real
collateral pins, fixed by bumping the event count to 71) and t185 (7/16 red
green-alone AND identically red on a clean origin/v2 baseline worktree -
pre-existing environment issue, not this branch). Files affected by the
post-run fixes were re-run explicitly (223 pass / 0 fail across 17 files)
before the final slices above.

[leandrodamascena]

CHANGES REQUESTED.

I like where this is going — a deterministic backstop for a bound that prose lost is the right idea. But I extracted evaluateReviewerScope from the hook and ran it under bun with a {unit:"U03", exempt:[]} record, and I got a dispatched reviewer to read a sibling unit's construction/ content through about a dozen inputs, several of them the way a reviewer would naturally type the command. The round-1 hardening does hold against the shapes it was written for — I confirmed construction/U01/design.md, construction/*/*/*.md, the single-token .., and the whole-path quote all block. The problem is everything one step to the side of it.

The root cause is one design choice: the matcher is a lexical scan for the literal token construction, but the bound it defends is a path-containment property. Those aren't the same thing, and the gap is every finding below. All of these return block:false on the real matcher (I ran each):

• Recursive search rooted above construction/. grep -rn X ., rg Contract docs/aidlc, find docs/aidlc -name design.md -exec cat {} +. The construction subtree lives under .../intents/<intent>/construction/<unit>/, so any recursion rooted at an ancestor carries no construction token and is allowed — while descending into every sibling. This is the exact "14 recursive greps" the PR names as motivation; the reviewer just roots one level up instead of typing construction/*/.
• Pathless Grep/Glob. Grep {pattern:"PaymentGateway"} with no path/glob contributes zero candidate strings, so the matcher loop never runs and the tool recurses cwd. The reviewer JSONs ship execute_bash/fs_read, so it's reachable.
• Case. Read {file_path:"Construction/U02/design.md"} — one capital letter opens the real lowercase file on macOS/Windows. No toLowerCase anywhere.
• Wildcard in the word / quote-split / cd+relative / bare relative. cat construction*/U01/x, cat "construction"/U01/x, cd construction/U03 && cat ../U01/design.md, Read {file_path:"../U01/design.md"} — all allow. The shell resolves them; the matcher never sees a contiguous construction/<sibling> token.

There's also a fail-closed twin of the case bug: scoped to U01, Read {file_path:"construction/u01/design.md"} returns block:true — a reviewer's own-unit read refused. Lower impact (§12a hands it the literal path), but same missing FS-case normalization, so fix both together.

My suggestion is to move the decision off the string and onto the resolved path: the hook already has the record's unit and path, so judge whether a resolved path or a search root lands inside a sibling construction/<other-unit>/, rather than whether the string spells construction. Add case-insensitive comparison, treat a pathless recursive Grep/Glob as rooted at cwd, and widen the tool allow-list (NotebookRead and anything outside the six-name matcher fail open today). Then re-pin t221 with the ancestor-root, pathless, case-variant, and cd-relative shapes — every current block case names construction explicitly, which is why the suite passed.

One structural note, not a blocker: the only writer of .aidlc-reviewer-dispatch.json is prose in the four SKILL.md orchestrators. If the conductor skips step 1 the hook fails open. I think that's acceptable — it's defense-in-depth over the reviewer's own prose bound, fail-open is the safe direction, and forgetting the write falls back to today's baseline, not worse. But it means this is a second layer, not a replacement, and the matcher coverage is what has to change before it earns the "deterministic twin" framing.

Non-blocking: Kiro IDE prose-only is the right call and well documented; product-lead wires the hook but reviews no per-unit stage so it always fails open (harmless, but dead wiring); the block-reason text tells a blocked reviewer to "quote the pattern," which nudges toward the exact quoting that flips a block to an allow — worth changing regardless.

[apackeer]

Addressed the blocking reviewer-scope findings in 12112403.

What changed:

• Replaced the lexical construction token scan with path/root containment checks keyed off the dispatch record root.
• Blocks ancestor-root recursive searches (grep -rn X ., rg ... aidlc/spaces/default/intents, find ...), pathless Grep/Glob, case variants, wildcard or quote-split construction components, cd + relative sibling traversal, and bare relative sibling reads from the current-unit cwd.
• Keeps current-unit paths and exact exempt files allowed.
• Widened the Claude/core reviewer-scope tool surface to file/search/shell tools (NotebookRead, MultiEdit, NotebookEdit, LS included) and removed the old block-reason nudge to "quote the pattern".
• Updated t221, docs, CHANGELOG, and regenerated dist/.

Verification:

• bun test tests/unit/t221-reviewer-scope-hook.test.ts -> 61 pass
• bun scripts/package.ts --check -> all harness trees in sync
• bun test tests/unit/t150-codex-packaging.test.ts tests/unit/t149-codex-hook-adapter.test.ts tests/unit/t147-kiro-hook-adapter.test.ts -> 42 pass
• bun test tests/unit/t68-version-changelog-sync.test.ts tests/unit/t132-hooks-doc-count-sync.test.ts tests/smoke/t01-file-structure.test.ts tests/smoke/t02-hook-executability.test.ts tests/smoke/t03-settings-json.test.ts -> 62 pass
• bash tests/run-tests.sh --debug -P 8 --smoke --unit -> PASS, 153 files / 2632 assertions, logs at tests/logs/2026-07-12T00-28-21Z