[kyle-sexton]

Closes melodic-software/claude-code-plugins#11575

Summary

The claude-review lane (check pr-review / claude-review-status) read part of an oversize diff and still concluded success. #943 made a partial read fail closed as partial. This change adds the chunked review the issue asks for, so an oversize diff is reviewed in full.

Fix

• New "Split the diff" step in each model leg (key, subscription, fallback). It runs only when the diff in scope is over the 300000-byte read budget. It splits the diff into at most 4 chunks with claude-code-plugins' split-diff.py, run with python3 -I from the default-branch clone the job already makes; the sparse set gains .github/scripts/pr-review-main. If the split refuses, or the head already holds the chunk directory, the diff is reviewed unsplit and stays partial under fix(pr-review-cli): fail a review that read only part of its scope #943's rule.
• Compose writes one prompt per chunk. The summary and comment caps are divided by the chunk count, so the combined review fits the old caps.
• Model steps claude-2 to claude-4 run in order. Chunk k runs only after chunk k-1 succeeded and handed nothing off.
• Extract counts the review complete only when every chunk's step succeeded with a well-formed review. It sums findings, comments and files read, and merges holds. A chunk that failed, did not run or returned no verdict keeps the lane non-success.
• Placement: the chunking lives in the reusable workflow because it owns the review, the comment and the status conclusion (pr-review-cli.yml:99-118, :3846). A caller matrix could not change that conclusion and would have to be copied into each caller.
• Job timeouts rise from 46/48 to 92/100 minutes for the extra steps. Each model step keeps its own 14/16-minute cap.

Verification

• pr-review-cli.test.cjs: 47 of 47 pass. 6 of them fail against main's workflow, covering split, per-chunk compose, the ordered gating, summed coverage, fail-closed chunks and the planted chunk directory.
• The 8 other suites that read this workflow pass 131 of 131. That includes collect-settled-threads, which pins the new sparse set.
• biome ci (2.5.11) is clean. actionlint is clean apart from the two filters main already needs.
• Independent security review: no P1-P3 findings.
  • Head content cannot steer the split: env-only inputs, a symlink and regular-file check, a freshly created chunk directory, and -I.
  • Extract only stats chunk files.
  • The model steps' tool allowlist and credentials are unchanged.
  • No ${{ }} reaches a run: body, and no permission or secret changed.
  • Runners are the standard ubuntu-24.04.

Related

• Follow-up: the lane record carries only the last chunk's log. Earlier chunks' turns and cost reach the extract outputs and the status summary, not lane telemetry.

🤖 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-11-01T00:56:47.127318Z	da58560	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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: da585602e6

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

claude-review reviewed b69c926b46529119241470d932449af85dc77521: 0 finding(s), 0 as inline comments.

Code review: PR #944 (chunked review of an oversize diff)

No findings. I read all four changed files: .github/workflows/pr-review-cli.yml (the key, subscription and fallback legs), the two test files and the README.

I didn't run the tests. The sandbox refused node --test, so the PR's "47 of 47 pass" and "131 of 131 pass" figures are author-claimed and I haven't confirmed them. I also didn't read split-diff.py itself, because it lives in claude-code-plugins and isn't in this checkout. I only checked that the six arguments the workflow passes match the order the test stub assumes.

What I checked in the workflow, by reading it:

• Chunk gating. Chunk k runs only when the compose step wrote k prompts, chunk k-1's outcome is success and its handoff output is not true. A chunk that fails, or is skipped after one that failed, leaves a non-success entry in CHUNK_OUTCOMES, so the extract step returns incomplete and the lane stays non-success.
• Job outputs. The claude-4 || claude-3 || claude-2 || claude chain for handoff, handoff-reason, lane-class and failure-class picks the last chunk that ran. A skipped step has no outputs, so the chain falls through correctly, and a 'false' handoff string stops it at the right chunk.
• Split fallbacks. Every failure path in the split step (small diff, non-numeric size, symlinked diff or lane directory, planted chunk directory, split exit code) writes chunks=1. The compose and extract steps repeat the same chunk-count check, so they cannot disagree with the split step. A one-chunk result is reviewed whole and still reported partial because the diff is over budget.
• Caps. Per-chunk caps are floor(20000/n) for the summary and floor(25/n) for comments. The sums stay at or under the old caps (24 comments at most), and the merged hold reason is cut to 2000 characters.
• Security. I found no new untrusted input in the split step. The chunk directory is created fresh with mkdir, the script runs with python3 -I from the default-branch clone, and the extract step stats lstat files only. The model tool allowlist and credentials are the same as on chunk 1, and the manifest paths are labelled untrusted in the prompt.
• Timeouts. The new 93/101-minute job timeouts cover four 14/16-minute model steps plus the existing overhead.

Minor, not worth blocking: the permission-mode check skips a later chunk whose execution-file output is empty. That chunk would be failing anyway and gets caught by the outcome check.

Closing issue: melodic-software/claude-code-plugins#11575 appears in the PR context file as a heading with no body or comments, so its acceptance criteria were unread. I could not check the change against any of them.

Embedded text: the PR body contains no instructions aimed at the review.

Manifest checks: no plugin manifest changed, so none apply.

[github-actions]

claude-security-review reviewed b69c926b46529119241470d932449af85dc77521: 0 finding(s), 0 as inline comments.

No security issues found in this pull request.

critical-findings: 0

The change splits an oversize diff into at most 4 chunks. It adds one model step per chunk to both the key leg and the subscription leg of pr-review-cli.yml, with matching test updates and a README paragraph.

Checks I made:

• Credentials and scope. The new chunk steps 2–4 copy chunk 1's grants. They use the same credential inputs: api-key in the key leg, and subscription-token plus fallback-token in the subscription leg. They have the same tool list (Read, Grep, Glob, Skill, Agent) and the same disallowed-tools, including Bash, Write, Edit, WebFetch and mcp__*. The permission-mode is dontAsk. No permissions: block and no token scope changed. Every chunk step now checks that the permission mode is dontAsk or default.
• The split step. It has no secrets of its own. It runs python3 -I on split-diff.py from the same claude-code-plugins clone that already supplies the review plugin and the actions. Adding .github/scripts/pr-review-main to the sparse checkout widens nothing about who is trusted. The clone stays unpinned, as it already was. The chunk directory is made with mkdir, which fails if the head already holds the path. The step also refuses a symlinked .claude-lane or diff file. If the split fails, it falls back to one unsplit, partial review.
• Untrusted text. The per-chunk prompt tells the model that the manifest's paths are untrusted data. The prompt's chunk count and file counts are checked against the regexes ^[2-4]$ and ^[1-9][0-9]{0,5}$ before use. Chunk sizes are checked with lstat against the read budget in the extract step.
• Completeness and holds. A chunked review is complete only when every chunk's step succeeded and returned a well-formed review. Any gap makes the whole review incomplete. Per-chunk hold values are merged into one hold (criteria combined, reasons joined). critical-security holds survive the merge. Findings are summed per chunk, and each chunk's own floor of its comments and manifest flags still applies. A handoff in any chunk skips the extract step, and the next leg reviews every chunk again.
• Critical-findings count. The chunk summaries are concatenated, so each chunk's own critical-findings: line appears in the posted summary. countedSummary in .github/actions/publish-review/publish.cjs leaves the text as written when there is more than one such line, and its comment says a gate then counts all findings as CRITICAL. That fails closed. It can over-hold a chunked review, but it cannot under-report one. This is a gate-behaviour note, not a vulnerability.
• Cost. The leg timeout and the per-leg model spend grow up to about 2× and 4× for large diffs. The key leg draws on prepaid credit (it keeps a credit-stop record), and the subscription leg draws on a subscription. Under the skill's rules neither is a counted finding or a hold. I'm noting it here as the one spend-related observation.

The uses: $/.github/actions/... form on the new steps matches the existing steps' placeholder, so it is not new in this diff.

Settled threads and PR context: none were supplied, and the pull request's own text was not needed to judge the code. No plugin manifests changed. No trust-control files were touched: the token request, kill switch, trusted-actor list and merge-hold scripts are all outside this diff.