Skip to content

ci: review up to 10 batches, five per wave - #239

Merged
Atom-oh merged 2 commits into
devfrom
ci/pr-review-10-batches-20261106
Nov 6, 2026
Merged

Atom-oh merged 2 commits into
devfrom
ci/pr-review-10-batches-20261106

Conversation

@Atom-oh

@Atom-oh Atom-oh commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • review-limits.json: max_batches 8 → 10, max_parallel_batches 4 → 5, so a review still takes at most two waves per phase (two fresh one-hour AWS sessions) inside the existing 190-minute job timeout.
  • Docs, CHANGELOG (amends the existing batching bullet), CLAUDE.md/AGENTS.md (markers regenerated) and the bounds table updated; the pre-read cap becomes 1,128 KiB.

Why

#227 (dependency/runtime refresh plus the gated six-pillar diagnosis) now packs into 9 batches and is rejected before any model call.

Resource note

Each wave runs up to 5 batches: 10 reviewer CLIs or 5 chairs on the sample-awsops runner pod (2 CPU, 8 GiB request, 12 GiB limit), up from 8/4. Wall-clock per wave is unchanged (bounded by one batch).

Test plan

  • python3 -m pytest -q scripts/v2/test_pr_review_batches.py scripts/v2/test_pr_review_pipeline.py (89 passed, TMPDIR=/tmp)
  • Local workflow simulation with 20 files → 10 batches in 2 waves, PASS and FAIL paths
  • check_ai_context.py for scripts/ and docs/runbooks/

🤖 Generated with Claude Code

Large promotions (dependency refresh plus a gated feature) need nine file-boundary
batches. Raising max_batches to 10 and max_parallel_batches to 5 keeps every review
within the existing two-wave, two-session workflow and its 190-minute timeout; each
wave still runs one batch-length of model time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

🤖 AI Code Review (two independent reviewers)

Reviewer responses: codex/ALL claude/ALL

Status: PASSED — No blocking issues found

All verification is complete. Every old-number occurrence in the repo corresponds exactly to a hunk this diff rewrites (the "eight panel calls → two" sentence at pr-review-head-images.md:279 refers to the lens consolidation, not batch count, so it's correctly untouched), the limits validation and wave/test arithmetic all hold at 10/5, and I confirmed the concurrency facts behind the one contested finding. Here is the synthesized review.


Chair Review — PR #239: ci: review up to 10 batches, five per wave

IMAGE_COVERAGE: NOT_REQUIRED

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

1. Summary

This PR raises the PR-review batching limits from 8 batches / 4 per wave to 10 batches / 5 per wave: the only executable change is two integers in scripts/pr-review/review-limits.json, with the workflow budget comment, five runbooks, two generated AGENTS.md files, scripts/CLAUDE.md and both CHANGELOG language sections updated to match. I verified the derived arithmetic (admission refusal 10 × 102,400 + 131,072 = 1,155,072 B = exactly the documented 1,128 KiB), the limits validation (1 ≤ 10 ≤ 16, 1 ≤ 5 ≤ 10 passes at review_batches.py:33-36), the two-wave structure ((index-1)//5 + 1 keeps batches 6–10 in wave 2; the workflow's fixed panel_review_2/chair_review_2 steps and fresh per-wave 3600 s sessions still cover it), and the test invariants (ceil(10/5) = 2 ≤ 2; 190×60 − 2×(2×1210 + 2×1030) = 2,440 s ≥ 1,800 s). No AWS-mutating path, IAM policy, or review-integrity control is touched.

2. Issues per lens

L2 — Code correctness

MINOR (downgraded from one model's MAJOR) — peak reviewer-CLI concurrency rises 25 % on an unchanged runner pod. Claude flagged this MAJOR; Codex found no issue. I verified the facts: run-batches.sh:20 admits max_parallel_batches cells at once and each panel cell (run-panel.sh) forks both the codex and claude CLIs concurrently, so the panel phase now peaks at 10 CLIs (and the chair phase at 5) on the pod the runbook describes as 2 CPU / 8 GiB request / 12 GiB limit, which this PR does not resize. However, I do not adopt it as MAJOR: there is no concrete failure trigger in the resulting code — no evidence the previous 8-CLI peak was near the pod's limits, the CLIs are network-bound on model calls, and the failure mode if the pod is OOM-killed is fail-closed (no verdict → review BLOCKED → re-run), never an incorrect PASS. It is a real operational-risk and doc-accuracy gap (the "sized for the pod" claim now covers a peak nobody measured), not a verified defect. Note the coupling is forced: with only two wave steps in the workflow, 10 batches structurally requires max_parallel_batches ≥ 5.

MINOR (one model) — the two-wave invariant is test-guarded but not runtime-guarded. review_batches.py validates max_parallel ≤ max_batches but not ceil(max_batches/max_parallel) ≤ 2, which the workflow's fixed wave steps assume. Verified fail-closed: a hypothetical 3-wave config would leave ready unset and the gate blocks with "Panel coverage incomplete" — confusing but safe.

MINOR (one model) — up to 10 concurrent Bedrock conversations per wave raises throttling/retry-exhaustion odds. The worst-case wall-clock bound is per-cell, not per-count, so the 190-minute budget still holds; this only increases re-run probability.

Both models agree all limit-consumer arithmetic (packing, admission refusal size, wave assignment, chair stdin 252,096 B ≤ 262,144 B, test invariants) is correct at the new values; I independently confirmed each.

L3 — Security / AWS mutation safety

No blocking findings — both models agree. No AWS-mutating path, IAM policy, secret, or ADR-005-governed surface is touched. I confirmed the credential-lifetime invariant survives the wider wave (wave duration is bounded by the slowest cell, not cell count, so 40m20s panel / 34m20s chair stay inside each fresh 3600 s session), and the review-integrity properties are preserved: batch-1 sensitive-file capacity is still one BATCH_BYTES budget with overflow rejected before any model call, the fail-closed sensitivity classifier is unchanged, the trusted per-batch note carries no PR-derived text, and aggregate_env() still ORs failure flags so a later batch cannot clear an earlier one.

MINOR (one model, accepted-design observation) — admission now accepts up to 1,128 KiB of diff while each reviewer still sees ~100 KiB plus a path list, so the non-sensitive cross-batch blind spot grows proportionally. The sensitive-files-fit-batch-1 guarantee and the no-downgrade rule for security findings are unchanged.

L4 — Observability / data-integration correctness

No blocking findings — both models agree. No Steampipe, CIS, collector, or AgentCore diagnosis code is in the diff. Per-batch log grouping, ::stop-commands:: fencing with fresh tokens, per-cell response labels and chair_failed_any all scale to 10 batches unchanged.

MINOR (one model) — with 10 batches the combined comment will almost certainly exceed comment_bytes (60,000) and fit() truncates tail batches from the PR comment. Verified non-gating: the verdict is computed from the untruncated file before fit() runs, truncation cuts from the end (preserving batch 1's security content first), and the full text remains in the workflow log.

L5 — Docs/ADR consistency

No blocking findings — both models agree. I swept the repo for every old-number phrasing variant (8 batches, 8 complete, 4 in flight, 928 KiB, Batches 5, eight reviewer, four chairs, file-boundary batches): every occurrence is exactly a hunk this diff rewrites; nothing stale survives. The 1,128 KiB figure matches the computed 1,155,072 bytes; "Batches 6–10 form a second wave" and "ten reviewer CLIs or five chairs" match the code. CHANGELOG hygiene is correct per CLAUDE.md: both the English and Korean [Unreleased] batching entries are amended in place, 1:1, with no PR/round numbers and no duplicate bullet. No ADR governs review batching, and ADR-005 is unaffected. Unverified (both the panel and I lack the tooling): the regenerated claude-md-sha header digests in the two AGENTS.md files — plausible, no check recomputes them, not a defect claim.

3. Suggestions

  1. Validate or resize the runner for the new 10-CLI peak: either record a measurement at 5-wide parallelism in pr-review-head-images.md (whose "sized for the pod" sentence currently asserts this without evidence) or bump the pod CPU/memory request alongside the concurrency.
  2. Add ceil(max_batches / max_parallel_batches) ≤ 2 to the limits validation in review_batches.py so a future max_batches bump fails loudly at admission instead of as an opaque coverage failure.
  3. Consider noting in the runbook that with near-maximal batch counts, tail-batch review text is likely to be log-only in the published comment (the verdict is unaffected).

4. Verdict

The single contested MAJOR (runner capacity) is verified factual in its concurrency math but speculative in its failure claim, fail-closed in effect, and flagged by only one of two independent models — downgraded to MINOR. All adopted findings are MINOR; arithmetic, tests, security boundaries and documentation are consistent at the new limits.


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

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Atom-oh
Atom-oh merged commit 5d8ea67 into dev Nov 6, 2026
6 checks passed

This branch was previously deployed

1 inactive deployment
ci-review-auto — 30779492 Deployed Nov 6, 2026 by Atom-oh via AI Code Review #918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant