[Atom-oh]

Summary

The required AI review rejects any filtered diff over 6,000 lines / 128 KiB before a model is called ("split the change or implement complete bounded review batches"). #227 now carries ~733 KB of review input (runtime refresh + #233 + #235) and cannot pass at all. This implements the bounded batches the gate message asks for.

• scripts/pr-review/review_batches.py admit: a diff within one call's limit keeps the single review path unchanged (same work dir, diff file and checklists). A larger diff is split at diff --git boundaries into at most 8 batches, each within 7/8 of the byte/line limits (redaction headroom), and every batch is admitted with the existing input_scope.admission() checks. One file over the per-batch budget, or more than 8 batches, is rejected before credentials/models. Stale batch dirs and single-path review.md/github.env are removed on each admission (self-hosted runner /tmp persists).
• Panel and chair steps run one run-panel.sh / synthesize.sh per batch in parallel, so the existing timeout budget and one-hour credential sessions still hold. Each batch gets its own GITHUB_ENV file; review_batches.py env keeps per-batch last-write semantics and ORs every failure flag across batches, so a later batch's =0 can never clear an earlier batch's =1.
• Each batch's prompt and chair prompt get a scope note listing the files in the other batches (cross-batch missing-definition rule; policy violations stay blocking).
• combine writes one section per batch and VERDICT: PASS only if every batch's chair ended with VERDICT: PASS (missing/invalid verdict = FAIL). The gate prints diagnostics for every incomplete batch.
• The comment body is cut at a line boundary to 60,000 bytes (GitHub's 65,536-character cap) with a notice; the verdict is unaffected.

Test plan

• scripts/v2/test_pr_review_batches.py (13 tests): single-path unchanged, lossless ordered split, oversized-file / too-many-batches / omitted-source rejection, stale cleanup, cross-batch flag OR, per-batch last-write, combined verdict, comment fit, workflow wiring, multi-batch gate diagnostics.
• Local simulation of the real workflow steps (extracted from the YAML) with fake CLIs: 1 batch PASS; 4 batches PASS; 4 batches with one FAIL → VERDICT: FAIL.
• test_pr_review_pipeline.py / test_pr_review_head_images.py: same failures as base dev locally (codec sandbox needs Docker/Pillow here); one assertion updated for the new admission command. tests/structure/test-pr-review-*.sh unchanged results (panel-prompt fails on base too).
• CI review of this PR (single path, since this diff is small).

Risk: a batched run starts up to 16 CLI processes at once on the runner pod and issues that many concurrent Bedrock calls.

🤖 Generated with Claude Code

[github-actions]

🤖 AI Code Review (two independent reviewers)

_Reviewer responses: codex/ALL claude/ALL _

Status: PASSED — No blocking issues found

Both panels responded and the key claims are verified. Here is the synthesized review.

PR #237 — ci: review oversized PR diffs in complete file-boundary batches

IMAGE_COVERAGE: NOT_REQUIRED

(The manifest lists no images, no unavailable entries and no omissions; both panel reports also declared NOT_REQUIRED.)

1. Summary

This PR replaces outright rejection of oversized filtered diffs with a fail-closed batched review: at most 8 file-boundary batches, security-sensitive files co-located in batch 1, two waves of at most 4 parallel batches each behind fresh one-hour AWS sessions, and a combined verdict that passes only when every batch passes. Both independent reviewers found no CRITICAL or MAJOR issues; I independently re-derived the budget arithmetic (BATCH_BYTES 102,400 + 12,288-byte header ≤ 128 KiB; 5,600 + 400 lines = 6,000; 928 KiB pre-read cap; 190-minute timeout with ~40 minutes of slack), traced the GitHub Actions waves == '2' && … || … expressions through all ready/outcome states, and confirmed the flag aggregation covers exactly the set of flags the existing shell scripts export. The change adds no AWS mutation, IAM grant or secret; ADR-005 is untouched.

2. Issues per lens

L2 — Code correctness

No CRITICAL/MAJOR. Codex reported no findings; Claude reported only MINORs, which I verified against the diff and adopt:

• MINOR (one model; verified) — The "at most two waves" invariant is enforced only by test_pr_review_batches.py, not by review_batches.py's own limits validation. A max_batches/max_parallel_batches edit producing 3 waves would fail closed in CI ("Panel coverage incomplete" — wave 3 has no workflow step), but with a misleading reason. Adding the wave-count bound to the module's ValueError check is cheap.
• MINOR (one model; verified) — run-batches.sh runs without -e, so a hypothetically empty STOP_TOKEN would emit a malformed ::stop-commands:: line. The sed :: rewrite is a real second layer and /dev/urandom via od effectively never fails, but a length guard would make the primary defense non-optional.
• MINOR (one model; verified) — The second panel wave runs even when a wave-1 batch already set coverage-severe.flag; the result can never pass, so up to eight reviewer calls are spent on an outcome already decided. Fail-closed, purely a cost issue.
• MINOR (one model; verified) — /tmp/pr-review-admit.txt is not removed by the new cleanup step. It contains only the four fixed output lines, so exposure is negligible, but the cleanup comment implies completeness.

I additionally confirmed the fail-closed paths Claude traced: a missing per-batch .rc file scores the wave as failed; combine requires the exact last line VERDICT: PASS from every batch; per-batch GITHUB_ENV aggregation runs before the wave's exit-status check so coverage flags survive a failing batch; and the fit subcommand runs under python3 -I without touching the non-isolated input_scope import (only admit, run from the trusted checkout, imports it).

L3 — Security / AWS mutation safety

No CRITICAL/MAJOR — both models agree. The patch adds no mutating AWS call, IAM policy or secret; the two new credential steps duplicate the existing configure-aws-credentials parameters exactly (same role, 3600s, unset/no-reuse). PR-derived file names reach models only via the sanitized, size-bounded stdin data header; the trusted prompt note is fixed text, pinned by tests. Batch work directories are owner-only (umask 0o077) and removed in an always() step.

• MINOR (one model; verified against BASE) — The batch-1 co-location patterns miss some genuine permission surfaces that exist in the tree: scripts/v2/ci_review_access.py (governs this review role's own access), web/lib/bedrock.ts, web/lib/agentcore.ts match neither SENSITIVE_PREFIX nor SENSITIVE_NAME. The design explicitly disclaims completeness and the cross-batch rule keeps unprovided dependencies blocking, so this is an accepted trade-off, not a violation — but ci_review_access.py deserves a prefix entry.
• Improvement (one model; verified) — the codec_sandbox.NOT_FOUND regex correctly narrows cleanup-error swallowing to object-level not-found while excluding transport/storage errors (no such host, no such file or directory), with tests pinning both directions.

L4 — Observability / data-integration correctness

No Steampipe/CIS/AgentCore logic is touched — both models agree; the lens reduces to the pipeline's own diagnostics. I confirmed the flag algebra: the four blocking flags OR across batches (a later batch cannot clear an earlier failure), chair_failed requires every chair to have failed (absent key = verdict produced), and chair_failed_any selects the partial-infrastructure-failure badge ordered before the CRITICAL/MAJOR badge. I verified the exported-flag surface in BASE shell scripts equals FAIL_FLAGS ∪ {chair_used}, so the contract test is genuine.

• MINOR (one model; verified) — OR-ed flags can produce an over-claiming badge: one batch tripping chair_input_failed yields "chair was not called" even though other chairs ran and their findings are published. Appending the tripping batch identifier would fix it.
• MINOR (one model; verified) — Mid-phase aggregate_env output in GITHUB_ENV is optimistic for not-yet-run wave-2 batches; the final wave overwrites it, but nothing marks the intermediate values as unreliable.
• MINOR (one model; plausible) — Four parallel chairs / eight reviewer CLIs on a 2-CPU pod raises timeout-retry risk; wall-clock is still hard-bounded, and the pre-change behavior was outright rejection, so no regression.
• MINOR (one model; verified) — fit truncation at 60,000 bytes can drop a later batch's MAJOR text from the PR comment. The verdict is computed before truncation and batch 1 sorts first, but a compact per-batch verdict index at the top of combine's body would keep failed batches always visible.

L5 — Docs/ADR consistency

No CRITICAL/MAJOR — both models agree docs are consistent. Numeric claims in pr-review-head-images.md (7/8 byte budget, 12 KiB/400-line header, 928 KiB pre-read refusal, 190-minute timeout, comment_bytes 60,000 vs GitHub's 65,536) all match the implementation; branch-strategy.md honestly updates the promotion stance while keeping the coverage-reuse-not-implemented caveat; the CHANGELOG adds one bullet per language with no PR/round numbers, matching the repo convention; the two regenerated AGENTS.md files correspond to the two changed CLAUDE.md sources. MINORs from one model, adopted: the model-facing _note omits the Markdown/unit-test exclusion the code applies (a reviewer could wrongly assume a terraform/CLAUDE.md change is in its batch); the runbook's "two panel calls plus a chair" paragraph doesn't state the up-to-8× multiplier a batched review implies; the reworded input_scope.py rejection ("split the change") is not actionable when surfaced for an internal batch boundary.

3. Suggestions

1. Enforce ceil(max_batches / max_parallel_batches) ≤ 2 inside review_batches.py's limits validation, next to the existing ValueError.
2. Add scripts/v2/ci_review_access.py (and consider web/lib/bedrock*/web/lib/agentcore*) to SENSITIVE_PREFIX.
3. Have combine emit a per-batch verdict index at the top so comment truncation can never hide which batch failed; include the tripping batch ID in the chair_input_failed/panel_incomplete badge text.
4. Guard STOP_TOKEN length in run-batches.sh; add /tmp/pr-review-admit.txt to the cleanup step.
5. One clause in _note stating that Markdown and unit-test files are not co-located in batch 1; one sentence in the runbook stating the batched call-count multiplier.

4. Verdict

No CRITICAL or MAJOR findings from either independent reviewer survived verification; all adopted findings are MINOR hardening/documentation items. Image evidence is not required for this PR (no staged images). The batching design is fail-closed at every degraded path I traced.

---

Triggered by commit a70d004a5b5af79a6f322bd33ebdd2e372a0d8f0 · workflow: .github/workflows/pr-review.yml