Merge pull request #36 from hotdata-dev/fix/omit-oversized-diff

fix(review): omit the full diff rather than truncate it

Author: zfarrell

README.md

@@ -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
+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 >`,

docs/claude-pr-review-prompt.md

@@ -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.