fix: bound the per-unit reviewer read scope to the unit plus shared contracts (2.2.16)

[apackeer]

fix: bound the per-unit reviewer read scope to the unit plus shared contracts (2.2.16)

Fixes #534

Problem

On for_each: unit-of-work design stages (functional-design, nfr-requirements, nfr-design, infrastructure-design, code-generation), stage-protocol section 12a passes the reviewer a constant-size pass-list but nothing bounds what it may read. The persona ("Cross-reference everything") and the reviewing knowledge checklist ("Cross-unit contract boundaries respected?") actively push it into sibling units' construction/<other-unit>/ directories, so reviewing unit N reads the designs of units 1..N-1 and per-unit review cost grows linearly - multiplied by the NOT-READY iteration loop. The issue's field data: a 12-unit intent spent over 24 hours in functional-design, with reviews of 20-90 minutes each and direct quotes of three sibling units' artifacts in one unit's review.

Change

Cross-unit contracts already live in the shared inception artifacts each stage consumes: (components.md, component-methods.md, services.md, unit-of-work.md), and the engine already resolves those to absolute paths in the directive. This PR routes cross-unit verification through them:

• Stage-protocol 12a step 1: for per-unit stages (directive.unit present) the pass-list now also carries the resolved directive.consumes paths (paths only, per the existing context-budget rule), plus an explicit "Reviewer read scope" paragraph: current unit's artifacts + passed contracts; other units' construction/ directories only to spot-check an integration point the current unit's design explicitly names, and only that file. Step 2's "Reviewer executes" enumeration mirrors the bound.
• Persona (aidlc-architecture-reviewer-agent.md): new ## Review Scope section; "Cross-reference everything" is scoped to the artifacts under review and the passed contracts.
• Knowledge (reviewing.md): Stance gains a scope bullet; the Functional Design checklist item names the shared inception contracts as the verification source, with the one-file spot-check carve-out.
• All four harness orchestrators (Claude Code, Kiro CLI, Kiro IDE, Codex) carry the same two edits in their reviewer-step bullet - these bullets are what the conductor actually executes, so the protocol edit alone would not reach it.
• New t217-reviewer-read-scope.test.ts pins the bound on all four surfaces (protocol, persona, knowledge, orchestrator skill). All 17 assertions fail on the pre-fix bytes and pass post-fix.
• aidlc-product-lead-agent needs no edit: it reviews no per-unit stage, and the protocol wording keys off directive.unit, so the bound applies agent-agnostically if that ever changes.
• Version 2.2.16 + CHANGELOG + README badge; dist/ regenerated, bun scripts/package.ts --check green.

Why this does not lose review coverage

A sweep of sibling designs was verifying claims the shared contracts already pin. Where a unit's design names a sibling's internal identifier, the spot-check carve-out still opens that one file. Where a design hand-waves ("integrates with earlier units") the reviewer now raises a finding instead of doing the reader's work - and a contract detail that exists only in a sibling's design prose is itself a gap in the shared contracts, which surfaces as a finding against the design or the contracts, both in scope.

Evidence

• Three independent adversarial review rounds (two Claude fable passes + a gpt-5.5 second opinion). Round 1: all 17 t217 assertions proven to differentiate pre/post-fix bytes; cross-harness bullet parity verified. Round 2 traced the runtime substrate (directive.consumes is emitted with resolved absolute paths; the persona reaches the reviewer on all four harnesses, including via Kiro's file:// prompt references and the Codex TOML embed) and found four wording/coverage issues, fixed in the second commit: a persona clause that would have flagged other units' shared-contract entries as unreferenced (false NOT-READY generator), an inaccurate characterization of directive.consumes, a spot-check carve-out loose enough to permit sibling-directory browsing, and t217 covering only the Claude SKILL surface when the four harness SKILL files are independently authored.
• Deterministic tier: smoke+unit+integration, 239 files / 3752 assertions; reds t84, t78, t185 (5s timeouts under parallel load) and t183 (SDK internal server error) all green-alone on rerun; none touch the edited surfaces.
• Targeted: t217 + t200 + t15 + t68 = 34 pass / 0 fail.

Notes for reviewers

• Complementary to PR feat: prune per-unit design artifacts by unit kind (2.2.18) #511 (artifact-count pruning) and PR feat: opt-in unit-major iteration for construction design stages (2.2.13) #512 (unit-major iteration) - orthogonal axes on the same wall-clock pain; conflicts only on CHANGELOG/version.
• Sibling PR for issue [Bug]: agent frontmatter uses modelOverride: but Claude Code's field is model: — every delegated agent silently inherits the session model (the sonnet tier never engages) #533 claims 2.2.15; the usual second-to-merge re-bump convention applies.
• Related independent defect: the reviewer also ran on the session model instead of its designated sonnet tier ([Bug]: agent frontmatter uses modelOverride: but Claude Code's field is model: — every delegated agent silently inherits the session model (the sonnet tier never engages) #533, fixed separately).

[jstrunk]

Since filing #534 I ran the same 12-unit intent through Construction nfr-requirements with the #533 model fix applied locally (all six reviews confirmed on claude-sonnet-5 in the subagent transcripts). The scope problem reproduces on this second for_each: unit-of-work stage, independent of the model defect, and the transcripts sharpen both the case for this PR and one limit of it.

1. The growth reproduces, and it's worse than linear. Six nfr-requirements reviews in dispatch order:

Review order	Unit	Duration	Peak input context	Unique API calls
1st	U01	33 min	143k tok	56
2nd	U02	41 min	151k	74
3rd	U04	30 min	143k	55
4th	U06	71 min	161k	83
5th	U03	89 min	203k	113
6th	U05	177 min	214k	103

Duration tracks review order, not unit complexity — each later reviewer has more READY siblings to cross-check, and each sibling artifact has grown (appended ## Review sections).

2. Why the cost compounds: per-call latency scales hard with context. Pooling all ~560 API calls across the six runs and bucketing time-from-tool-result-to-next-response by total input tokens:

Input context	n	Mean latency	Max
50–75k	39	4.5s	22s
75–100k	53	5.2s	22s
100–125k	145	23.0s	230s
125–150k	155	37.7s	374s
150–175k	68	75.6s	366s
175–200k	73	88.0s	1652s
200–225k	32	130.1s	582s

There's a knee near 100k tokens (~5× jump), and ~30× total degradation by 200k. In the U03 review, ~85 of 89 minutes were time-to-first-token on calls emitting ~30 output tokens. Implication for this PR: bounding sibling reads doesn't just save the reads — it keeps every subsequent call in the review below the knee. The benefit is superlinear, which strengthens the case for merging.

3. The gap: the prose bound doesn't cover the access pattern the reviewer actually used. The scope sentence forbids reading "other units' construction/<other-unit>/ directories." But in the U03 transcript, sibling access was dominated by recursive Bash greps with cross-unit globs — 14 commands like:

grep -rn "Realtime instance|instance class.*Realtime|endpoint instance" construction/*/*/*.md inception/*/*.md
grep -rn "ml\.|instance_type|c5\.|m6g\." construction/U01-infra-foundation/*/*.md construction/U05-scoring-service/*/*.md ...

(alongside 32 Reads touching 30 distinct files across five units, and 97 Bash calls total). A construction/*/*/*.md glob never "reads a directory" in the sense the prose forbids, and a diligent reviewer can file each one under the spot-check carve-out. Note the pre-fix persona already contained instructions the reviewer overrode in the other direction ("Cross-reference everything" was doing exactly what it said) — prose bounds have a track record of losing to this persona's diligence.

Suggested follow-up (not a blocker): a deterministic PreToolUse hook in core/hooks/ that, when the active subagent is the reviewer on a per-unit stage, rejects Read/Grep/Bash targeting construction/<unit>/ for any unit other than the dispatched one (with the passed contract paths exempted). The hooks layer already does subagent tracking and has one flow-altering hook (Stop), so the mechanism exists. That would make the bound self-enforcing regardless of how the persona interprets the carve-out — the .reviewerignore idea upthread, but using the framework's native enforcement point.

One more observation supporting the "no lost coverage" section: in my runs the conductor's dispatch prompts already named the specific sibling invariants by ID (e.g. "Sibling consistency: U04 (EP-1 publish envelope, EC-1 footprint formula, ER-7) and U02 (metrics-doc convention, rung mechanics)"). The orchestrator demonstrably knows the contract-level facts without the reviewer sweeping prose to rediscover them — which is the PR's coverage argument, confirmed from the other side. It also validates patching all four SKILL.md bullets: the conductor composes scope-expanding prompt text on its own, so the protocol edit alone wouldn't have reached it.

Methodology

Durations and API-call counts come from the subagent transcripts under <session-dir>/subagents/agent-<id>.jsonl (first→last event timestamps, assistant events deduplicated by message.id). The latency table measures time from each tool_result event to the next assistant event, bucketed by input_tokens + cache_read_input_tokens + cache_creation_input_tokens. The grep examples are verbatim from the U03 reviewer's tool_use inputs.

[apackeer]

Thanks for running this on a second stage and bringing transcript-level numbers - the latency-knee table is the strongest argument for this PR that anyone has produced, and the point that bounding sibling reads keeps every subsequent call below the knee (so the benefit is superlinear) is now part of how I'd frame the change.

On the gap you found: you're right, and it's fixed. The bound as first shipped said "must not read other units' construction/<other-unit>/ directories", and your U03 transcript shows exactly why that wording loses - 14 recursive greps with construction/*/*/*.md globs never "read a directory" as written. As of 631a08e all six surfaces that carry the bound (the 12a read-scope paragraph, the reviewer-executes bullet, the persona, the knowledge checklist, and the reviewer-step bullet in all four harness orchestrator skills) state it tool-agnostically: grep, glob, and shell patterns that span sibling unit paths count as sibling reads, and the spot-check carve-out excludes searching as well as browsing. The unit test (t217) now pins the tool-agnostic wording on the protocol and on each of the four authored skill files, so a future rewording can't quietly narrow it back to file reads.

On the deterministic PreToolUse hook: agreed, and filed as #539 rather than folded in here - the hook needs a deterministic way to know "the active subagent is the reviewer for unit X" (the dispatch is conductor-driven prose today), and the blocking capabilities differ per harness (Kiro CLI can hard-block from its preToolUse seam; Codex likely degrades to advisory), so it deserves its own design pass rather than a rider on a prose PR. Prose-vs-persona is a contest prose eventually loses with this reviewer, as you note - the hook makes the bound self-enforcing and is where this should end up.

Your observation that the conductor's dispatch prompts already name sibling invariants by ID is a useful confirmation of the coverage argument from the side we couldn't test - and yes, it validates patching the four SKILL.md bullets rather than the protocol alone.

[leandrodamascena]

APPROVE

I traced the runtime path: buildRunStageDirective sets consumes: present from splitConsumesByPresence(resolveConsumes(...)) (aidlc-orchestrate.ts:1145), and the consumed inception artifacts resolve to the shared producer dir, not per-unit. So the contract paths the reviewer is told to check against actually reach it — the bound isn't dead prose.

Cross-harness parity holds (package.ts --check green, all four SKILLs carry the same reviewer-step wording). Enforcement is prose-only, but it's restated as RFC-2119 MUST NOT across protocol §12a, the persona, the knowledge file, and the four orchestrators, and it's tool-agnostic (names grep/glob/shell so a construction/*/ glob counts as a sibling read, not a search). There's no way to express "only the current unit" as a static path scope since every unit lives under aidlc/spaces/**, so prose is the only option here. I'm fine with the coverage trade-off: an unresolved cross-unit question becomes a finding against the design or the shared contracts, both in scope.

Two non-blocking test gaps in t217:

• The persona assertions (t217:47-54) pin the ## Review Scope header, the reworded cross-reference line, and spot-check, but not the "do NOT access sibling units' construction" sentence or the grep/glob wording on that surface. So that sentence could be deleted from the persona and t217 stays green. Add a /grep, glob, or shell patterns/ assertion to the persona block.
• Same gap in the knowledge block (t217:57-67): the tool-agnostic clause on reviewing.md:27 is unpinned. Worth an assertion there too.

Neither blocks the merge — the wording is present on all surfaces today, just under-guarded on two of them.