[kyle-sexton]

Refs: #11000

Summary

pr-review-main used to stop on a diff over its 300000-byte cap. It recorded verdict=none reason=too-large and left the PR evidence-unbound, so a lane-source PR with a large diff could never merge. Now it reviews that diff in chunks, up to 4 of them. This PR follows the issue #11000 body, which is the spec, except for the multi-file over-cap stop below, which stays open in #11000.

• The diff splits into at most 4 chunks of at most 300000 bytes each. A chunk ends at a file boundary, and a file is never split, so every file, lane-source files included, is reviewed whole in one chunk (split-diff.py).
• Each chunk gets its own run-claude-cli review with the same prompt and inputs. The prompt lists that chunk's parts and says which chunk of how many it is.
• The lane writes one verdict. findings is the sum over the chunks, and critical is the most CRITICAL findings any one chunk returned, as the issue says ("the max CRITICAL count across chunks wins"). It is uploaded under the same artifact name in the same verdict.json shape and binds exactly as a single-pass verdict does. The gate holds on critical > 0, and the max is above 0 exactly when the sum is, so binding is the same either way. Each listed finding of a chunked verdict names its chunk.
• It fails closed. If any chunk's review fails, leaves no result, is off the schema or skips one of its parts, no verdict is written. The leg then records verdict=none reason=chunk-failed on the head, binds nothing and fails.
• A single file over 300000 bytes still stops as too-large, however small its hunks, with perf(pr-pipeline): record main-review stops and cut merge-job lane reads #11084's record as before. So does a diff that needs more than 4 chunks (see Remaining work in feat(pr-pipeline): review an oversize main-run diff in chunks instead of holding it for a person #11000).
• A diff under the cap is one chunk, and its prompt is byte-for-byte the old single-pass prompt.
• No person hold is added. chunk-failed is a lane cause, like the other stops (main-review-stop:<reason> under the MED evidence-unbound:<lane>).

Remaining work in #11000

A multi-file diff over MAX_CHUNKS × 300000 bytes (about 1.2 MB) still stops as too-large, while #11000 asks for the size stop to fire only for a single file over the cap. Lifting that needs a dynamic chunk count (for example, a planning job that emits a chunk matrix), which this PR does not build: any fixed slot count leaves some multi-file diff over the cap. The convention (docs/conventions/pr-pipeline/README.md) and the pr-pipeline changelog fragment state this stop and point at #11000.

Fix

Design: unrolled review slots. Each review leg (key, subscription, fallback) has 4 Review chunk k steps. Chunk k runs only when the diff has at least k chunks and chunk k-1 succeeded without handing off. This is the repository's own pattern for several run-claude-cli calls in one job: pr-unblock's adjudicate-feedback job runs 3 adjudication slots this way. Actions cannot loop a uses: step, and each run-claude-cli call makes its own work directory with mktemp -d, so calls in the same job do not collide.

• .github/workflows/pr-review-main.yml. The edits are identical in all three legs, and workflow.test.mjs holds the legs equal.
  • Write the diff. The binary and long-line checks still run on the whole patch. split-diff.py then writes the chunks, and exit status 3 (a file over the cap, or more chunks than MAX_CHUNKS) means stop too-large. Each chunk is cut into Read-sized parts under diff/chunk-<k>/. The step outputs chunks and logs the chunks notice. MAX_CHUNKS: "4" is the knob.
  • Write the prompt and schema. One prompt per chunk, pr-review-main-prompt-<k>.md. A new CHUNK_NOTE placeholder renders empty when there is one chunk; otherwise it says the diff is split at file boundaries, each file whole.
  • Slots. Each slot has its own lane record (Report chunk k's outcome). Slots after the first have a Time chunk k step that gives step-started-at. Each slot uploads its own sealed transcript, and slots 2 to 4 add -<k> to the transcript job-id.
  • Hand the lane on. A handoff from any chunk hands the whole lane on, and the next leg reviews every chunk again.
  • Write the verdict. It runs only when every chunk's review succeeded and nothing was handed off, and it passes EXECUTION_FILES and CHUNKS.
  • Record a failed chunk (new). It covers a chunked leg that reviewed, handed nothing on and wrote no verdict. It writes <lane>-stop=chunk-failed and <lane>-head. The job outputs read steps.diff.outputs.X || steps.chunk-stop.outputs.X.
  • Record job. The closed reason regex adds chunk-failed.
  • Job timeout. timeout-minutes goes from 30 to 100 per review leg: 4 chunk steps at 22 minutes each, plus the leg's other steps. The legs run on subscription usage, so a longer run spends subscription usage, not billing.
  • Not changed: on:, every permissions: block, token requests and the concurrency group.
• write-verdict.mjs.
  • buildChunkedVerdict checks each chunk's output as a whole review, with FINDINGS_MAX per chunk. It sums findings and takes the max of critical. One chunk gives the single-pass verdict. Each item of a chunked verdict carries its 1-based chunk.
  • main reads DIFF_DIR/chunk-<k>/ and runs readCoverage for each chunk against its own parts. It requires the number of chunk directories to equal CHUNKS.
  • It prints one pr-review-main chunk notice per chunk.
• split-diff.py (new, stdlib only). It cuts at file boundaries and packs whole files greedily in order. A file over the cap, or more chunks than the most, writes nothing and exits 3.
• babysit_gh.py. MAIN_REVIEW_STOP_TITLE adds chunk-failed. Without this, that title would read as None, which counts 1 CRITICAL carried on every later head. Check runs cannot be deleted, so that hold would be permanent.
• maintenance-reconcile-lost-runs/reconcile.mjs. REVIEW_STEP becomes Review chunk 1, the step name the reconciler reads to hold a lost run. A cut step list that reaches a later chunk also reaches Write the prompt and schema, so the predecessor check still holds the run. The verdict write still starts only after a true success, which keeps the reconciler's rule: a run is re-run once only if it was lost before its write.
• Docs and changelogs. docs/conventions/pr-pipeline/README.md is updated. There are fragments in .changes/pr-pipeline/ and .changes/source-control/.

Alternatives considered

1. A dynamic chunk matrix: a planning job emits a lane × chunk matrix. It needs a new job with its own permissions: block, and per-chunk outputs would collide in the matrix-output merge, which would mean rebuilding the handoff routes per chunk. Not built here; it is the remaining work in feat(pr-pipeline): review an oversize main-run diff in chunks instead of holding it for a person #11000.
2. A static chunk axis on the existing matrix. About 16 VMs per run would mostly start only to exit, each repeating the trigger gate's API reads. Rejected for the runner and API cost.
3. A loop inside one step that calls the CLI directly. It bypasses run-claude-cli's isolation, credential scrub and handoff contract. Rejected.
4. One session reading every chunk. It does not remove the reason for the per-review byte cap. Rejected.

Known limits

• The 4-chunk cap. See Remaining work in feat(pr-pipeline): review an oversize main-run diff in chunks instead of holding it for a person #11000. Each chunk is a sequential step of up to 22 minutes, and each slot adds about 70 YAML lines per leg; 4 slots fit a 100-minute job. workflow.test.mjs requires MAX_CHUNKS to equal the number of review steps in every leg.
• A reviewer sees only its own chunk. A change in one chunk that depends on a file in another is judged without that file. write-diff.py still prints whole any move that an added line names.
• items against critical. verdict.json's items lists every chunk's findings, so it can hold more CRITICAL items than critical counts. Binding reads only the counts (findings and critical, matched against the check-run title), never items. Each chunked item names its chunk, so a reader can see which chunk's CRITICAL findings the max counts.
• A handoff at chunk k spends the earlier chunks' reviews, because the next leg re-reviews every chunk.
• The per-chunk findings notices print only when the verdict is written. On chunk-failed, the verdict step's error names the chunk instead.

Metrics

Each run makes the chunk count and the per-chunk results measurable (§9), as workflow notices:

• The diff step prints ::notice title=pr-review-main chunks::lane=<lane> pr=<n> chunks=<k> bytes=<b1>,<b2>,..., for one chunk too.
• The verdict step prints ::notice title=pr-review-main chunk::lane=<lane> chunk=<i>/<k> bytes=<b> findings=<f> critical=<c> for each chunk.
• A stop prints ::notice title=pr-review-main stop::reason=<too-large|chunk-failed> ... and posts the check run pr-review-main: <lane> titled verdict=none reason=<reason>.

Verification

Tests came first. Each was red before the change and is green after.

Test	Before	After
workflow.test.mjs, 35 tests	at the keep-whole change: 2 fail (the single-file case gets status 0 with two chunks, and the runDiff case below gets chunks=2 instead of stop=too-large)	35 pass
write-verdict.test.mjs, 11 tests	at the max change: 2 fail (critical: 3, the sum, where 2 is expected); at the chunk-label change: the aggregation test fails (chunk undefined)	11 pass
test_babysit_gh.py (106) with test_a_failed_chunk_reads_as_a_stop_that_counts_no_critical	fail	pass
reconcile.test.mjs	the pr-review-main step-name test fails after the rename	102 pass
deny-rules.test.mjs		5 pass

workflow.test.mjs, write-verdict.test.mjs and deny-rules.test.mjs together: 51 pass, 0 fail on the current head, after merging main.

The keep-whole change replaces the hunk-split expectations, so split-diff.py's single-file test now expects exit 3 where it expected 0. The spec asks for that, and it is not a weakened test.

The cases:

• Splitting at file boundaries. Files of 60, 70 and 50 bytes with a 150-byte cap give two chunks, a+b and c.
• A single file over the cap. A 43-byte header and three 30-byte hunks (133 bytes) with a 110-byte cap exit 3 and write nothing, alone or beside a small file. At a 133-byte cap the file is one whole chunk.
• The acceptance case. In the Write the diff step, one file of 303 bytes over a 300-byte MAX_DIFF_BYTES, spread over 12 hunks of under 30 bytes each, records stop=too-large and writes no part. A 612-byte one-hunk file does the same.
• At the cap. One file of exactly the cap is one chunk, and exactly max-chunks chunks still split.
• Over the cap. Three cap-sized files with a limit of 2 chunks exit 3 and write nothing. Five 200-byte files with a 300-byte cap stop as too-large. Four give chunks=4 and a notice with bytes=200,200,200,200.
• Aggregation. A chunk with two CRITICAL and one SUGGESTION, then a chunk with one CRITICAL and one IMPORTANT, give findings=5 critical=2, so neither the last chunk nor the sum wins. The items carry chunk 1, 1, 1, 2, 2, and no chunk lists more CRITICAL items than critical. The end-to-end run, in the other order (1 then 2 CRITICAL), writes findings=4 critical=2.
• Failing closed. A chunk that never ran, left no result, is off the schema or read another chunk's parts throws, writes no verdict.json and leaves GITHUB_OUTPUT empty. So does a chunk count that does not match the chunk directories.

Checks:

• actionlint exits clean.
• zizmor --offline reports no findings (34 ignored, 25 suppressed, all from the slot steps' self-repository ignores and secret inputs).
• ruff check and ruff format --check pass through scripts/run-ruff.sh.
• markdownlint-cli2 reports no issues on the edited docs and fragment.
• check-changelog-fragments.sh --check and --check-required origin/main both pass.

Security review. An independent review:security-reviewer (Opus) ran on feature commit fa3f3a4 with verdict CLEAN: no token or secret leak, no account compromise, no billing; on:, permissions:, token requests and concurrency unchanged. The follow-up changes (no hunk split, max CRITICAL, chunk-labeled items, docs) touched only split-diff.py, the prompt text, tests, write-verdict.mjs and docs, and the claude-security-review lane passed on 5c76ba9.

Related

• feat(pr-pipeline): review an oversize main-run diff in chunks instead of holding it for a person #11000 stays open for the multi-file over-cap stop (Remaining work in feat(pr-pipeline): review an oversize main-run diff in chunks instead of holding it for a person #11000).
• perf(pr-pipeline): record main-review stops and cut merge-job lane reads #11084 made the size stop a recorded outcome. This PR keeps its records for the over-cap case.
• fix(pr-pipeline): record the main-run review's size stop as an outcome and collapse exact renames #10961 is the main-run designed-stop work this builds on.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-30T06:42:07.689842Z	a93c119	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

claude-review reviewed d40517b8b162aaa40076a6b713ee5f712036e694: 1 finding(s), 1 as inline comments.

I read the whole diff (.claude-lane/pr.diff) and the PR context. I ran no code, tests or workflows and have no Bash here, so I checked everything by reading. The PR's own test counts are author-claimed and unverified.

What I checked, and found sound

• split-diff.py: it cuts only at diff --git lines. A content line always has a prefix byte, so it cannot be mistaken for a header. A file over the cap, or more than MAX_CHUNKS chunks, exits 3 and writes nothing. A trailing newline and a patch with no trailing newline both split correctly.
• write-verdict.mjs:
  • Each chunk is checked as a whole review, with its own readCoverage against its own parts.
  • The chunk-directory count must equal CHUNKS.
  • A missing, unreadable or off-schema chunk throws before verdict.json is written.
  • The verdict satisfies the gate's critical <= findings check (babysit_review_evidence.py:587).
  • Taking the max of critical keeps the "binds on critical > 0" behaviour identical to a single pass.
• Workflow slots:
  • Chunk k runs only when the diff has at least k chunks and chunk k-1 succeeded without a handoff.
  • The verdict step needs every needed chunk to have succeeded and none to have handed off.
  • The Record a failed chunk condition excludes a handoff and a skipped first review, so a handoff is never recorded as chunk-failed.
  • A single-chunk failure still goes through Fail without a verdict as before.
  • The record job's regex and MAIN_REVIEW_STOP_TITLE both accept chunk-failed, so a failed chunk is not read as an unknown reason that counts 1 CRITICAL.
• Reconciler: a lost run in chunk 2 to 4 also lists Write the prompt and schema. The cut-list check therefore still holds it, and Write the verdict still marks the line past which a run is not re-run.
• Trusted-path scrutiny:
  • No on:, permissions: or token-request lines changed.
  • Every slot repeats chunk 1's read-only tool grants: tools: Read, with Bash, Edit, Write, Agent and mcp__* denied.
  • The model's text still reaches no shell; it goes through a file and write-verdict.mjs.
  • I found no security issue.

Spend (not a hold, not counted): a diff over the cap now makes up to 4 run-claude-cli reviews per leg instead of stopping. Each runs up to 22 minutes, and the job timeout-minutes goes from 30 to 100. This spends prepaid API credit or subscription usage and Actions minutes within the existing runners; it enables no paid plan or billing setting.

Finding (1): the closing issue's size stop is only partly delivered (inline comment on pr-review-main.yml:226).

Embedded text: neither the PR body nor the issue contains an instruction aimed at this review.

[github-actions]

claude-security-review: 1 inline finding(s) on a93c119c34b25ff1bc086f5a504641ab58f40af6; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

claude-security-review reviewed d40517b8b162aaa40076a6b713ee5f712036e694: 1 finding(s), 1 as inline comments.

Security review: PR #11160, chunked main-run review

critical-findings: 0

I read the whole diff (13 files) and the PR context. The workflow, split-diff.py, write-verdict.mjs and the babysit_gh.py change all touch trusted paths.

What I checked

• Token and permission scope. The on: trigger and every permissions: block are unchanged. Each chunk step copies chunk 1's inputs. The tests require the steps to be identical, and the leg-specific credentials stay one per step. The tools stay Read(./pr-review-main-data/**) with dontAsk, and Bash, Agent, WebFetch and mcp__* stay denied. No scope grew and no secret is newly exposed.
• Untrusted text reaching a shell or the prompt.
  • CHUNKS and the chunk number k reach the prompt only after the regex ^[1-9]$ and seq.
  • The chunk note carries only integers.
  • Model output still goes through buildVerdict, which is unchanged, including its credential-pattern scan.
  • The record job's closed set ^(too-large|binary|long-line|chunk-failed)$ and the 40-hex head check still guard the gh api call.
• Verdict binding. write-verdict.mjs fails closed. It checks the chunk count against the chunk directories, requires an execution file per chunk, and runs readCoverage against each chunk's own parts. It sums findings and takes the maximum critical, which is above 0 exactly when any chunk has a CRITICAL, so the gate's critical > 0 hold is unchanged.
• chunk-failed stop.
  • MAIN_REVIEW_STOP_TITLE adds chunk-failed, so it reads as a stop with no CRITICAL count, like too-large. It counts no CRITICAL but leaves the lanes unbound, so the merge hold stays.
  • A forged stop title only renames the reason. That matches the existing README claim for the other stops.
• Handoff. The "Hand the lane on" step handles every chunk's handoff reason and its closed set is unchanged. "Record a failed chunk" is skipped when any chunk handed off.
• Reconciler. REVIEW_STEP now names "Review chunk 1". That keeps the lost-run hold for a run cut at any chunk, because its predecessor step is also in the cut list.
• Manifests. No plugin.json or marketplace.json change.

Finding (IMPORTANT)

• A diff the lane used to refuse is now reviewed in isolated pieces and can bind. See the inline comment on .github/workflows/pr-review-main.yml. Before this change, any diff over 300000 bytes got verdict=none reason=too-large and stayed unbound. Now a diff of up to 4 chunks is reviewed chunk by chunk. Each prompt tells the model "the other chunks are not yours to judge". A change split across files in different chunks (a workflow or lane-source edit in one chunk calling code added in another) is never judged as a whole. Each chunk can return 0 CRITICAL, and the verdict binds. The README already admits this ("A reviewer sees only its own chunk"), but it does not name it as a weakened hold.
  • Who can use it. An author whose pull request reaches this lane. I could not see that trusted-actor check in this diff, and the owner threat model limits the authors to the owner's own actors.
  • What stops it. The over-cap single-file and more-than-4-chunk cases still stop. Every file is read whole in one chunk. The other merge gates are unchanged. I did not verify whether they cover the cross-file case, so this is not rated CRITICAL.

Spend, not a hold

Each of the three legs now allows up to 4 sequential review runs of 22 minutes, and the job timeout goes from 30 to 100 minutes. This draws only on prepaid API credits or subscription usage, so it is no hold and not counted as a finding.

[github-actions]

The pr-verify verifier failed head 5c76ba94598aeff83754046cfb5c6d95268fa21c. What it wrote, as data:

What it wrote (25 lines)

Summary: Issue #11000 asks that an oversize diff be reviewed in chunks so that "the size stop then
  fires only for a single file over the cap". This pull request says it closes the issue, but it
  adds a hard cap of 4 chunks (MAX_CHUNKS). Any multi-file diff over about 1.2 MB still stops as
  `too-large`, which leaves the permanent person hold the issue exists to remove. The PR body admits
  this and lists it as remaining work, yet still uses `Closes #11000`, so the spec is only partly
  delivered. The splitting, the fail-closed handling (`chunk-failed`), the CRITICAL max across
  chunks and the chunk metrics look correct and are tested.
- `.github/workflows/pr-review-main.yml`: `MAX_CHUNKS: "4"` together with `split-diff.py` exiting 3
  when a diff needs more than 4 chunks keeps `stop too-large` for multi-file diffs. The issue says
  the size stop should fire only for a single file over the cap. Make a multi-file diff of any size
  reviewable: add review slots, or chunk across legs or jobs, so a diff never stops `too-large` for
  its chunk count. If that is not done here, change `Closes #11000` to a `Refs: #11000` line that
  names the delivered part and the remaining work.
- `.github/scripts/pr-review-main/split-diff.py:140`: the `len(chunks) > max_chunks` branch raises
  TooLarge, so a multi-file diff stops `too-large`. The issue says this stop should fire only for a
  single file over the cap. The tests at `.github/scripts/pr-review-main/workflow.test.mjs` (the
  five-file `tooMany` case) lock in the stop the spec wants removed.
- Spec acceptance item 'An independent security review of the change finds no HIGH/CRITICAL' is not
  in `person_checks`, and the PR body shows no record of such a review. Attach one, or have the
  issue's refinement record it as a person check.
- `.github/workflows/pr-review-main.yml`: the job `timeout-minutes` goes from 30 to 100 in each
  review leg. This spends within the subscription, not a real charge; named for a human's attention.
- `.github/scripts/pr-review-main/write-verdict.mjs`: in a chunked verdict, `items` can list more
  CRITICAL items than `critical` counts (the max over chunks). The PR body says the gate reads only
  `findings` and `critical`; confirm that no consumer reads `items` severities against `critical`.

This pull request leaves the merge queue, and ci-status holds it out while it carries pr-verify-failed. A pr-verify pass at a new commit removes the label.

[github-actions]

claude-security-review: 1 inline finding(s) on 5c76ba94598aeff83754046cfb5c6d95268fa21c; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

claude-security-review: 1 inline finding(s) on d40517b8b162aaa40076a6b713ee5f712036e694; the summary is in this lane's pull request comment.

claude-lane: claude-security-review

[github-actions]

claude-review: 1 inline finding(s) on d40517b8b162aaa40076a6b713ee5f712036e694; the summary is in this lane's pull request comment.

claude-lane: claude-review