Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,35 @@ comments", which are claims rather than gaps, so those failures are disclosed to
there once skipped the review step and the notify step with it, leaving the PR with no review and no
explanation.

Both step outputs are byte-bounded (100 KB of comment threads, 200 KB of context), with per-block
caps beneath that — 3,000 diff lines, 40 KB per CI log excerpt, 3,000 characters per comment. The
caps are deliberately far below any plausible runner limit: 400 inline comments rendered 1.1 MB of
threads before they existed, and the runner accounts for output size in UTF-16, so a byte count here
is not the number it checks against. Blocks are ordered so that truncation sacrifices the PR
conversation before the diff or the CI status.
Both step outputs are interpolated into one `prompt:` string, so the binding limit is the kernel's
`MAX_ARG_STRLEN` (131,072) on the SUM of them, not a per-output cap — past it `exec` fails with
"Argument list too long" while the action still reports success. The budget is denominated in
*escaped* bytes, because the action carries the prompt a second time inside `toJson(inputs)` and the
escaped copy is the larger one: a Grafana dashboard PR measured 123,401 raw bytes and 135,366
escaped, and failed on two consecutive pushes. Comment threads take at most half of that total, and
what is left is the context allowance; every other block holds a share of *that* — the since-last-review
diff a third, and CI log excerpts, the PR conversation, the changed-file list and the commit list an
eighth each — with the two diff blocks also sharing 3,000 patch lines. Two things about those shares
are load-bearing. The denominator: a share of the whole budget is twice the share it claims to be once
the review history is long, which is precisely when blocks compete. And the sum: it is around
two-thirds, because shares adding to exactly 1 leave nothing for the PR body, the CI list or the
headings, and a worst case that clears the limit by rounding error is not a backstop. A line cap is
also not a byte cap — at the 1.20x a quote-dense patch costs, 2,000 lines of dashboard JSON is about
120 KB escaped — so the since-diff needs both, and every block is charged escaped.

The full diff is all-or-nothing. It renders whole or it is replaced by a notice naming its size and
telling the reviewer to run `gh pr diff`. A prefix reads as the whole patch: what survives a cut is
whichever files sort first rather than whichever matter. Across two weeks of production runs, 108 had
their context cut and only 23% re-fetched anything — the ones that did found 1.91 issues per run
against 0.92 for the ones that did not, and 40% of the cut runs on PRs over 1,000 lines posted no
finding at all. Omitting is affordable for this block alone, because it is the only diff the reviewer
can replace itself: `gh pr diff` is allowlisted and was refused 0 times in 54 attempts. The
since-last-review diff keeps its prefix for the same reason inverted — `gh api .../compare` is not
allowlisted, so trading its prefix for a notice would trade partial information for none.

With every fetched block bounded, the tail cut on the assembled context is a backstop rather than the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "every fetched block bounded" is one block short, and the same claim is now in tests/context-step-test.sh:804. ## Commits (scripts/gather-review-context.sh:485) is written straight from $COMMITS with no cap — no cap_file_escaped, no line cap. (not blocking)

It is the last fetched block in that position. The pulls/{n}/commits endpoint tops out at 250 commits, so the ordinary ceiling is ~15 KB at a 60-byte line — already a quarter of the context on a maxed-threads run — but the line is \(.sha[0:8]) \(.commit.message | split("\n")[0]) and a git subject has no length bound, so the real ceiling is whatever the author writes. It sits above ## Full diff like the changed-file list did, so an overflow there is paid for by the omission notice and the conversation, which is the argument this PR just used to cap the other two.

Either give it the same treatment as FILES_FILE — it is three lines and NOTICE_FILES has a sibling shape — or narrow the sentence to name it alongside the PR body. The claim is load-bearing for "the tail cut is a backstop rather than the ordinary path", so it is worth being exactly true.

ordinary path. The PR body is what still reaches it: it arrives through `env:` rather than an API
read, and a generated release-note body is the remaining way for a context to exceed the budget.

Everything reaching the prompt is attacker-controlled — title, body, diff, CI logs, comments — so the
block delimiters are neutralised by shape rather than by exact string: `</pr_context >`,
Expand Down
10 changes: 9 additions & 1 deletion docs/claude-pr-review-prompt.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ Everything in `<pr_context>` is already in front of you. Do not spend a tool cal

**Unless it is not there.** If `<pr_context>` is empty, or a block inside it says it could not be read, then that block is genuinely missing — fetch what you need yourself with `gh pr diff` or `gh pr view`, and say in your review that you reviewed without it. Never treat a missing block as evidence: an absent CI block does not mean CI is clean, and an absent diff does not mean nothing changed.

**Or if it was cut short.** A block may end with a notice that it was truncated — `context truncated to fit the review prompt`, `prior review comments truncated`, `log excerpt truncated`, or `(truncated: first N of M lines`. The part you were given is real, but the rest of that block exists and you have not seen it. Do not review as though you had. Fetch the remainder with `gh pr diff` or `gh pr view` before drawing any conclusion about the code that was cut, and **state plainly at the top of your review that your context was truncated and what you did about it.** A truncated diff is the one case where the instruction above not to re-fetch does not apply.
**Or if it was cut short.** A block may end with a notice that it was truncated — `context truncated to fit the review prompt`, `prior review comments truncated`, `log excerpt truncated`, `PR conversation truncated`, `changed file list truncated`, `commit list truncated`, `since-diff cut to fit the review prompt`, or `(truncated: first N of M lines`. The part you were given is real, but the rest of that block exists and you have not seen it. Do not review as though you had. Fetch the remainder with `gh pr diff` or `gh pr view` before drawing any conclusion about the code that was cut, and **state plainly at the top of your review that your context was truncated and what you did about it.** A truncated diff is the one case where the instruction above not to re-fetch does not apply.

**Or if the diff is not there at all.** When the patch is too large for this prompt, `## Full diff` holds `full diff omitted: too large for the review prompt` and no patch. This is not a summary and not a sample — you have been shown none of the change. Get it before you review anything:

1. Run `gh pr diff <number> --repo <owner/repo>` for the whole patch. It is allowlisted and it is one turn.
2. If that is too large to read in one go, use the `## Changed files` block, which names every path with its own `+`/`-` counts, and `Read` those files from the checkout. The checkout is the merge result at the head SHA, so what you read is the post-change file.
3. Say in your review that the diff was omitted and name the files you read.

**Never approve, and never state that a change is correct, on the strength of the surrounding blocks alone.** The title, the commits, the file list and the CI status describe the change; they are not the change. An approval formed without the patch is a false claim about coverage no matter how carefully the rest of the context is read.

This matters most when you approve. An approval formed on a partial diff, presented as though it were formed on the whole one, is worse than no review — a human reads it as coverage it does not have. If you could not see all of the change and could not fetch the rest, say so and do not approve on the strength of what you did see.

Expand Down
Loading
Loading