From 54c15b81e195f9404b3207d566e65f957267413c Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 21 Aug 2026 11:47:02 -0700 Subject: [PATCH 1/5] fix(review): omit the full diff rather than truncate it --- README.md | 27 ++++-- docs/claude-pr-review-prompt.md | 10 +- scripts/gather-review-context.sh | 121 ++++++++++++++++++----- tests/context-step-test.sh | 158 +++++++++++++++++++++++++------ 4 files changed, 256 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 9a22805..19db641 100644 --- a/README.md +++ b/README.md @@ -36,12 +36,27 @@ 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. Beneath that total each block holds a share — comment +threads at most half, CI log excerpts a quarter, the PR conversation an eighth, and the two diff +blocks sharing 3,000 patch lines. + +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 block bounded, the tail cut on the assembled context is a backstop rather than the +ordinary path; the PR body is the one block with no cap of its own, and it is what still reaches it. 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: ``, diff --git a/docs/claude-pr-review-prompt.md b/docs/claude-pr-review-prompt.md index 6a998fb..13ef5ac 100644 --- a/docs/claude-pr-review-prompt.md +++ b/docs/claude-pr-review-prompt.md @@ -11,7 +11,15 @@ Everything in `` is already in front of you. Do not spend a tool cal **Unless it is not there.** If `` 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`, 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 --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. diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index fffa38a..c9f084f 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -74,10 +74,11 @@ LOG_WINDOW=120 # subject and the full patch is one allowlisted `gh pr diff` away. Cycle 1 has no # since-diff, so the full diff keeps very nearly the whole budget. # -# No floor is written under the full diff because SINCE_MAX sits below this number: -# the since-diff can spend at most 2,000 of the 3,000 lines, so the full diff always -# keeps the remaining 1,000. A floor would be a branch no input reaches. Raising -# SINCE_MAX to meet or exceed this number is the change that would need one. +# What "yields" means for the full diff is all of it, not a prefix of it: see the +# omission branch below. So this number is a threshold rather than a share -- a full diff +# that does not fit under it is replaced by a notice, and the 1,000 lines the since-diff +# leaves it are not rendered as a partial patch. No floor is written under it for that +# reason: there is nothing for a floor to protect. DIFF_BUDGET_LINES=3000 # Byte budgets. Both of this step's outputs are interpolated into the SAME `prompt:` # string in the review step, and that string reaches the reviewer as one environment @@ -159,6 +160,17 @@ fi # not exist. LOG_BUDGET=$((PROMPT_BUDGET / 4)) LOG_REMAINING=$LOG_BUDGET +# The PR conversation is the last block written, which without a cap of its own makes it +# both the block that overflows the budget and the block the tail cut removes to pay for +# the overflow -- 300 comments rendered 700 KB, and on www.hotdata.dev#332 the heading did +# not render at all. An eighth of the budget, the smallest share of the four, because it is +# the block the reviewer can most afford to lose: the inline threads carry the review +# history and this carries the rest of the discussion. +# +# Bounding it is also what makes the full diff's fit decision below answerable. That +# decision asks "is there room for the whole patch", and the question has no answer while +# an unbounded block is still to come. +CONVO_MAX_BYTES=$((PROMPT_BUDGET / 8)) # What the summary may take of it. The excerpts are written summary first, window # second, but the window is the block worth more: across five real failed job logs the # cause sat immediately above the first ##[error] in four, and the summary is what @@ -178,6 +190,11 @@ NOTICE_CONTEXT='context truncated to fit the review prompt' NOTICE_THREADS='prior review comments truncated' NOTICE_LOG='log excerpt truncated' NOTICE_LINES='truncated: first' +NOTICE_CONVO='PR conversation truncated' +# Not a truncation notice: this block is absent, not short. It is listed with the others +# because the contract is the same -- the prompt document has to quote it, or the reviewer +# reads a diff heading with no patch under it and no idea that a patch exists. +NOTICE_OMITTED='full diff omitted: too large for the review prompt' CTX="${RUNNER_TEMP}/pr-context.md" : > "$CTX" @@ -384,6 +401,23 @@ printf '%s\n' "$THREADS" | strip_block_tags > "$THREADS_FILE" cap_file_escaped "$THREADS_FILE" "$THREADS_MAX_BYTES" \ "${NOTICE_THREADS}; read the rest with gh pr view" +# The bound that keeps the assembled prompt inside MAX_ARG_STRLEN. Context gets what the +# threads block did not spend; threads is capped and written by this point, so its final +# size is known rather than assumed. +# +# Derived here rather than at the cap site at the end of the file, because the full diff +# block needs it: it decides whether to render a patch or a notice, and it cannot make that +# call without knowing what the whole context is allowed to weigh. The cap at the end still +# applies it -- this only moves the arithmetic above its first reader. +THREADS_BYTES=$(escaped_bytes "$THREADS_FILE") +CTX_MAX_BYTES=$((PROMPT_BUDGET - THREADS_BYTES)) +# Unreachable while THREADS_MAX_BYTES is clamped to half the budget. Kept because the +# alternative if that clamp is ever loosened is `head -c` with a negative count, and an +# empty context degrades a review where a failing cap_file loses it entirely. +if [ "$CTX_MAX_BYTES" -lt 0 ]; then + CTX_MAX_BYTES=0 +fi + DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)" { echo "threads<<${DELIMITER}" @@ -583,6 +617,15 @@ if [ "$FULL_DIFF_MAX" -gt "$DIFF_MAX" ]; then FULL_DIFF_MAX=$DIFF_MAX; fi DIFF_FILE="${RUNNER_TEMP}/pr.diff" if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then DIFF_LINES=$(awk 'END {print NR}' "$DIFF_FILE") + # What is left of the byte budget for this block: the whole allowance, less what the + # blocks above already spent, less the conversation still to be written. Measured on the + # stripped text on both sides, because strip_block_tags grows `` from 12 + # bytes to 19 and it is the emitted string that has to fit -- the same reason the cap at + # the end of this file strips before it measures. + strip_block_tags < "$CTX" > "${RUNNER_TEMP}/fit-ctx" + strip_block_tags < "$DIFF_FILE" > "${RUNNER_TEMP}/fit-diff" + DIFF_ALLOWANCE=$((CTX_MAX_BYTES - $(escaped_bytes "${RUNNER_TEMP}/fit-ctx") - CONVO_MAX_BYTES)) + DIFF_ESCAPED=$(escaped_bytes "${RUNNER_TEMP}/fit-diff") { echo echo "## Full diff" @@ -592,13 +635,46 @@ if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then if [ "$DIFF_LINES" -eq 0 ]; then echo "The diff came back empty. That is unusual for a pull request; treat it" echo "as missing rather than as \"nothing changed\" and run gh pr diff." + elif [ "$DIFF_LINES" -le "$FULL_DIFF_MAX" ] && [ "$DIFF_ESCAPED" -le "$DIFF_ALLOWANCE" ]; then + cat "$DIFF_FILE" else - head -n "$FULL_DIFF_MAX" "$DIFF_FILE" - if [ "$DIFF_LINES" -gt "$FULL_DIFF_MAX" ]; then - echo "(${NOTICE_LINES} ${FULL_DIFF_MAX} of ${DIFF_LINES} lines; run gh pr diff for the rest)" - fi + # All or nothing, and this is the nothing. A prefix of a patch is worse than no + # patch: it reads as the whole thing. What survives a cut is whichever files sort + # first rather than whichever matter, and the notice saying so used to be one line + # at the end of a context the reviewer had already read past. 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 only affordable because this is the one diff block the reviewer can + # replace by itself: `gh pr diff` is allowlisted and was refused 0 times in 54 + # attempts over those two weeks. The since-diff above keeps its prefix precisely + # because it has no such escape -- `gh api .../compare` is not allowlisted, so + # trading its prefix for a notice would trade partial information for none. + echo "(${NOTICE_OMITTED}; ${DIFF_LINES} lines)" + echo + echo "The patch is NOT below. Nothing has been shown to you and nothing has been" + echo "summarised. Do not review, approve or draw any conclusion about this pull" + echo "request from the blocks above alone." + echo + echo "Get it before you review:" + echo " - gh pr diff ${PR_NUMBER} --repo ${REPO} for the whole patch." + echo " - If that is too large to read at once, work from the '## Changed files'" + echo " list above -- it names every path with its own +/- counts -- and Read" + echo " those files from the checkout." + echo " - Say in your review that the diff was omitted and which files you read." fi } >> "$CTX" + if [ "$DIFF_LINES" -gt 0 ] \ + && { [ "$DIFF_LINES" -gt "$FULL_DIFF_MAX" ] || [ "$DIFF_ESCAPED" -gt "$DIFF_ALLOWANCE" ]; }; then + # A notice, not a warning, for the same reason the byte cap below uses one: a + # generated-file PR reaches this legitimately and a warning that cried regression on + # every one of them would stop being read. It exists so an operator reading a thin + # review can see the reviewer was never handed the patch -- the failure mode this + # block replaced was invisible, because a line-capped diff said so only in a + # parenthetical and never in an annotation. + echo "::notice::Full diff omitted from the review prompt: ${DIFF_LINES} lines, ${DIFF_ESCAPED} escaped bytes against a ${DIFF_ALLOWANCE} byte allowance and a ${FULL_DIFF_MAX} line cap." + fi else echo "::warning::Could not read the diff." { echo; echo "## Full diff"; echo "Could not read the diff; run gh pr diff."; } >> "$CTX" @@ -614,21 +690,22 @@ else echo "::warning::Could not read PR conversation comments." CONVO="Could not read PR conversation comments." fi -{ echo; echo "## PR conversation"; printf '%s\n' "$CONVO"; } >> "$CTX" +# Capped in its own file rather than appended straight to $CTX, so the cap is on this block +# and not on the whole context: appending first and capping after is the tail cut, which is +# what put this block's heading off the end of the prompt on www.hotdata.dev#332. Stripped +# before the cap for the reason strip_block_tags always runs first -- the substitution grows +# the text, so a cap on the unstripped file bounds a smaller string than the one emitted. +CONVO_FILE="${RUNNER_TEMP}/pr-conversation.md" +printf '%s\n' "$CONVO" | strip_block_tags > "$CONVO_FILE" +cap_file_escaped "$CONVO_FILE" "$CONVO_MAX_BYTES" \ + "${NOTICE_CONVO}; read the rest with gh pr view" +{ echo; echo "## PR conversation"; cat "$CONVO_FILE"; } >> "$CTX" -# The bound that keeps the assembled prompt inside MAX_ARG_STRLEN. Context gets what -# the threads block did not spend; threads is capped and written by this point, so its -# final size is known rather than assumed. The per-block caps above still matter -- they -# decide *what* survives truncation, and they keep any one block from arriving here -# having already crowded out the diff -- but this is what makes the total fit. -THREADS_BYTES=$(escaped_bytes "$THREADS_FILE") -CTX_MAX_BYTES=$((PROMPT_BUDGET - THREADS_BYTES)) -# Unreachable while THREADS_MAX_BYTES is clamped to half the budget. Kept because the -# alternative if that clamp is ever loosened is `head -c` with a negative count, and an -# empty context degrades a review where a failing cap_file loses it entirely. -if [ "$CTX_MAX_BYTES" -lt 0 ]; then - CTX_MAX_BYTES=0 -fi +# CTX_MAX_BYTES is derived above, beside the threads cap it is computed from, because the +# full diff block reads it. The per-block caps still matter -- they decide *what* survives, +# and they keep any one block from arriving here having already crowded out the diff -- but +# what follows is what makes the total fit. +# # Stripped before the cap, for the same reason as the threads block above: the # substitution grows `` from 12 bytes to 19, so a cap applied to the # unstripped file bounds a smaller string than the one actually emitted. This is the diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 5274802..886777f 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -333,6 +333,19 @@ expect_context() { fi } +# expect_no_context -- the inverse, for the blocks that must be +# absent rather than partial. context_has reads from a file, not a pipe, so a negative +# assertion here cannot be inverted into a vacuous pass by SIGPIPE; see CTX_FILE above. +expect_no_context() { + if context_has "$1"; then + echo "FAIL $2: a line matching /$1/ is in the rendered context" + grep -nE -m3 -- "$1" "$CTX_FILE" | sed 's/^/ /' + failures=$((failures + 1)) + else + echo "ok $2" + fi +} + # --- The whole step, nothing failing ---------------------------------------------------- expect "$(run_step)" "0" "step exits 0 with every endpoint answering" @@ -561,13 +574,59 @@ for v in 2.96 2.97; do expect_context 'FAILED \(failures=1' "failing job log reaches the context on gh $v" done -# --- Truncation -------------------------------------------------------------------------- - -# 2998, not 3000: the stub's since-diff is two lines, and the two diff blocks share one -# 3,000-line budget rather than holding independent caps. See the shared-budget section below. +# --- The full diff is all-or-nothing ------------------------------------------------------ +# +# A diff that does not fit is omitted, not trimmed. Two weeks of production runs are the +# argument: 108 runs had their context cut, and only 23% of them re-fetched anything -- +# the other 77% reviewed the prefix they were handed and said nothing about the rest. The +# ones that did re-fetch found 1.91 issues per run against 0.92 for the ones that did not. +# A prefix is worse than an absence because it reads as the whole patch: what survives is +# whichever files sort first, not whichever matter, and the notice announcing the cut was +# one line at the very end of a context the reviewer had already read past. +# +# The reviewer can get the whole patch itself -- `gh pr diff` is allowlisted and was +# refused 0 times in 54 attempts across those two weeks -- so omitting costs it a turn and +# buys back a complete diff. That trade is only available for this block, which is why the +# since-diff above still truncates: `gh api .../compare` is not on the allowlist, so a +# since-diff the reviewer cannot re-fetch is worth more as a prefix than as a notice. expect "$(STUB_DIFF_LINES=4000 run_step)" "0" "step exits 0 on an oversized diff" -expect_context '\(truncated: first 2998 of 4000 lines' "oversized diff truncated with a notice" -expect "$(grep -c '^+line ' "$CTX_FILE")" "2998" "truncated diff carries exactly the cap" +expect_context '^## Full diff' "an omitted diff still renders its heading" +expect_context 'full diff omitted: too large for the review prompt' \ + "a diff over the line budget is omitted with a notice" +expect_context '4000 lines' "the omission notice names the diff's real size" +expect "$(grep -c '^+line ' "$CTX_FILE" || true)" "0" \ + "no partial patch is left behind under the heading" +# The list of paths is what turns the notice into a plan: it is the only block that tells +# the reviewer which files to fetch or read, and it is ordered above the diff so it always +# survives. An omission notice without it sends the reviewer at a 37,000-line PR blind. +expect_context '^## Changed files' "the changed-file list survives a diff omission" +expect_context '^modified \+[0-9]+/-[0-9]+ ' "the changed-file list keeps its per-file counts" + +# The byte half of the same decision. 3,000 lines of quote-dense JSON is the shape that +# broke production: it costs about 1.20x escaped where prose costs 1.02x, so it sits inside +# the line budget and outside the byte budget. Before this it rendered as a prefix cut by +# the tail cap; now it is the notice. +STUB_DIFF_LINES=3000 STUB_DIFF_STYLE=json run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a diff that fits in lines but not bytes" +expect_context 'full diff omitted: too large for the review prompt' \ + "a diff inside the line budget but over the byte budget is omitted" +expect_no_context '"description": "line 1' \ + "no partial patch is left behind when the byte budget is what refused it" +# And the block below it, which is what the tail cut used to eat first. On the production +# run this is drawn from -- www.hotdata.dev#332 -- `## PR conversation` did not render at +# all, because it is written after the diff and the cut works from the end. +expect_context '^## PR conversation' "the conversation survives a diff that will not fit" + +# A diff that fits is still passed whole, with no notice. The omission is a response to the +# budget, not a policy: most PRs are nowhere near it (the median reviewed across the org is +# 161 changed lines) and re-fetching what was already affordable would spend a turn for +# nothing. +STUB_DIFF_LINES=40 run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a diff that fits" +expect "$(grep -c '^+line ' "$CTX_FILE" || true)" "40" "a diff that fits is passed whole" +expect_no_context 'full diff omitted' "a diff that fits carries no omission notice" + +# --- Truncation -------------------------------------------------------------------------- # An empty body under a heading is a claim: "## Full diff" with nothing beneath it reads as # "nothing changed", and the reviewer has been told not to re-fetch what it was given. The @@ -577,12 +636,33 @@ STUB_DIFF_LINES=0 run_step > "$WORK/code.txt" expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on an empty diff" expect_context 'came back empty' "an empty diff says so rather than showing a bare heading" -# The byte cap. Reaching it is a claim too: the cut lands wherever the byte count runs out, -# so the notice has to be appended *after* the cut or it is the first thing removed. And the -# ordering of the blocks is what decides whose content is lost -- the conversation is last -# because it is the block the reviewer can most afford to lose, while the diff and the CI -# status have to survive. +# The conversation block holds a cap of its own now, for the same reason the threads block +# does: it is the last block written, so without one it is both the block that overflows the +# budget and the block the tail cut removes to pay for the overflow. 300 comments rendered +# 700 KB. Bounding it is also what makes the diff's fit decision above answerable -- the +# diff cannot ask "is there room for me" while an unbounded block is still to come. STUB_CONVO_COMMENTS=300 run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a PR with hundreds of conversation comments" +expect_context '^## PR conversation' "the conversation block renders" +expect_context 'PR conversation truncated' "an oversized conversation says it was cut" +expect_context '^## Full diff' "the diff block survives a huge conversation" +expect_context '^\+line 1$' "the diff body survives a huge conversation" +expect_context '^## CI checks' "the CI block survives a huge conversation" +# The cap's real job, as opposed to its notice: without it this block alone rendered 700 KB +# and the only thing standing between that and a failed exec was the tail cut. The diff's +# fit decision reserves CONVO_MAX_BYTES for this block, and a reserve against an unbounded +# writer is not a bound. +expect "$(escaped_of "$CTX_FILE" \ + | awk -v lim="$PROMPT_ARG_LIMIT" '{print ($1 <= lim) ? "bounded" : "over"}')" \ + "bounded" "hundreds of conversation comments stay inside the argument limit" + +# The tail cut, which is now a backstop rather than the ordinary path. Every block above is +# bounded except the PR body, which is author-controlled and has no cap -- a generated +# release-note body is how a context still reaches the limit. Reaching it is a claim too: +# the cut lands wherever the byte count runs out, so the notice has to be appended *after* +# the cut or it is the first thing removed. +BIG_BODY=$(awk 'BEGIN { s = ""; for (i = 0; i < 3000; i++) s = s "release note line with plenty of detail "; print s }') +PR_BODY="$BIG_BODY" run_step > "$WORK/code.txt" expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when the context exceeds the byte cap" # No byte count in this pattern: the cap is derived per run now -- from the argument limit # less the wrapper, the prompt document and the threads block -- so asserting a literal here @@ -590,9 +670,10 @@ expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when the context exceeds the # ship. What has to hold is that the notice is present and names a figure. expect_context '\(context truncated to fit the review prompt' \ "the truncation notice survives the truncation" -expect_context '^## Full diff' "the diff block survives the truncation" -expect_context '^\+line 1$' "the diff body survives the truncation" -expect_context '^## CI checks' "the CI block survives the truncation" +# The head is what the cut keeps, so what has to survive is what was ordered first. The diff +# is no longer among those blocks: on a context this far over budget it has no allowance, so +# it renders as its omission notice and the reviewer is told to fetch it. +expect_context '^## Pull request' "the first block survives the truncation" # The assertion this replaces allowed 210000 bytes, which was above the limit the prompt is # actually bounded by -- it would have passed on a context that could not be handed to the # reviewer at all. Measured escaped, because that is the copy the limit applies to. @@ -738,20 +819,23 @@ ENDPOINTS # not get. # # The since-diff keeps its share, because on cycle 2+ what changed since the last round is -# the reviewer's subject; the full diff yields, and its notice says how to get the rest. +# the reviewer's subject and it is the block the reviewer cannot fetch for itself. The full +# diff yields, and now yields entirely: this is the shape that drove the production numbers +# in the all-or-nothing section above. 74% of runs on PRs over 1,000 lines at cycle 5+ were +# cut, against 25% at cycle 1, because the since-diff is what the later cycles add. STUB_SINCE_LINES=2500 STUB_DIFF_LINES=3000 run_step > "$WORK/code.txt" expect "$(cat "$WORK/code.txt")" "0" "step exits 0 when both diff blocks are oversized" since_rendered=$(awk '/^\+since /' "$CTX_FILE" | wc -l | tr -d ' ') full_rendered=$(awk '/^\+line /' "$CTX_FILE" | wc -l | tr -d ' ') # 1998 of the since-diff's 2,000-line share, the other two lines being its header and the -# "+incremental change" body line; 1,000 for the full diff, which is what the shared 3,000 -# leaves it. +# "+incremental change" body line. expect "$since_rendered" "1998" "the since-diff keeps its full share of the budget" -expect "$full_rendered" "1000" "the full diff takes only what the since-diff left" -expect "$((since_rendered + full_rendered))" "2998" \ - "the two diff blocks together stay inside the shared budget" -expect_context '\(truncated: first 1000 of 3000 lines; run gh pr diff for the rest\)' \ - "the reduced full diff says how much it is showing and how to get the rest" +expect "$full_rendered" "0" \ + "the full diff is omitted rather than reduced to what the since-diff left" +expect_context '\(truncated: first 2000 of 2502 lines' \ + "the since-diff truncates, because the reviewer cannot re-fetch a compare" +expect_context 'full diff omitted: too large for the review prompt' \ + "the full diff says it is absent and how to get it" # --- The assembled prompt fits in one environment string ---------------------------------- # @@ -796,7 +880,7 @@ while IFS= read -r notice; do failures=$((failures + 1)) fi done <&2 @@ -851,18 +935,30 @@ else failures=$((failures + 1)) fi -# Truncating silently would be worse than truncating: the reviewer would report on a diff it -# never saw, with no way to know it had not seen it. -expect_context 'context truncated to fit the review prompt' \ - "an over-budget context says it was truncated" +# Losing the diff silently would be worse than losing it: the reviewer would report on a +# patch it never saw, with no way to know it had not seen it. With every input oversized the +# diff is the block that cannot fit, so what has to be present is the omission notice and +# the instruction that goes with it -- not a truncated patch, and not a bare heading. +expect_context 'full diff omitted: too large for the review prompt' \ + "an over-budget context says the diff is absent" +expect_context 'The patch is NOT below' \ + "the omission says plainly that nothing was shown" +expect_context 'gh pr diff 172 --repo hotdata-dev/dlthubworker' \ + "the omission names the command that gets the patch" # The diff has to keep room. A long enough review history could otherwise spend the whole # budget on prior comments and leave the reviewer with nothing to review -- which is why the -# threads cap is held to half the budget rather than being a fixed number beside it. -if [ "$ctx_bytes" -gt $((PROMPT_ARG_LIMIT / 4)) ]; then - echo "ok the diff keeps room against an oversized review history ($ctx_bytes bytes)" +# threads cap is held to half the budget rather than being a fixed number beside it. Asserted +# on a diff that should fit in what is left rather than on the context total: the total is no +# longer a proxy for it, because a starved allowance now shows up as an omission notice, which +# is small. 2,000 patch lines is about 22 KB escaped against an allowance near 40 KB. +STUB_THREAD_COMMENTS=60 STUB_DIFF_LINES=2000 run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on an oversized review history" +if [ "$(grep -c '^+line ' "$CTX_FILE" || true)" = "2000" ]; then + echo "ok the diff keeps room against an oversized review history" else - echo "FAIL an oversized review history starved the context: only $ctx_bytes bytes left" + echo "FAIL an oversized review history starved the diff's allowance:" \ + "$(grep -c '^+line ' "$CTX_FILE" || true) of 2000 patch lines rendered" failures=$((failures + 1)) fi From f8d4e4f8d02f8a8f022b12eb40a88b0ee4fcd1d1 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 21 Aug 2026 12:02:32 -0700 Subject: [PATCH 2/5] fix(review): bound the blocks written above the diff --- README.md | 13 ++-- scripts/gather-review-context.sh | 102 +++++++++++++++++++-------- tests/context-step-test.sh | 114 +++++++++++++++++++++++++++++-- 3 files changed, 193 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 19db641..472f02f 100644 --- a/README.md +++ b/README.md @@ -41,9 +41,11 @@ Both step outputs are interpolated into one `prompt:` string, so the binding lim "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. Beneath that total each block holds a share — comment -threads at most half, CI log excerpts a quarter, the PR conversation an eighth, and the two diff -blocks sharing 3,000 patch lines. +escaped, and failed on two consecutive pushes. Beneath that total every block holds a byte share — +comment threads and the since-last-review diff at most half each, CI log excerpts a quarter, the PR +conversation and the changed-file list an eighth — with the two diff blocks also sharing 3,000 patch +lines. A line cap is 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 needed both. 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 @@ -55,8 +57,9 @@ can replace itself: `gh pr diff` is allowlisted and was refused 0 times in 54 at 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 block bounded, the tail cut on the assembled context is a backstop rather than the -ordinary path; the PR body is the one block with no cap of its own, and it is what still reaches it. +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: ``, diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index c9f084f..c7c5fa1 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -474,7 +474,16 @@ else echo "::warning::Could not read changed files." FILES="Could not read changed files." fi -{ echo; echo "## Changed files"; printf '%s\n' "$FILES"; } >> "$CTX" +# Capped, because `--paginate` returns up to GitHub's 3,000-file ceiling and a line per +# file is around 60 bytes -- 180 KB, over the whole budget, from a block with no cap of its +# own. It matters more than its size suggests: this is the block the full diff's omission +# notice sends the reviewer to, so it is the last one that should be able to overflow. An +# eighth of the budget, the same share as the conversation. +FILES_FILE="${RUNNER_TEMP}/changed-files.md" +printf '%s\n' "$FILES" | strip_block_tags > "$FILES_FILE" +cap_file_escaped "$FILES_FILE" $((PROMPT_BUDGET / 8)) \ + "changed file list truncated; read the rest with gh pr view --json files" +{ echo; echo "## Changed files"; cat "$FILES_FILE"; } >> "$CTX" # The reviewer cannot run tests -- no dependencies are installed and the allowlist # would refuse anyway -- but CI already ran them. Whether they passed is the one @@ -587,13 +596,30 @@ if [ -n "$LAST_SHA" ] && [ "$LAST_SHA" != "null" ] && [ "$LAST_SHA" != "$HEAD_SH SINCE_LINES=$(awk 'END {print NR}' "$SINCE_FILE") SINCE_USED=$SINCE_LINES if [ "$SINCE_USED" -gt "$SINCE_MAX" ]; then SINCE_USED=$SINCE_MAX; fi + # Line-capped and then byte-capped, because a line cap does not bound bytes: at the + # 1.20x this file measures for quote-dense JSON, 2,000 lines of dashboard patch is + # about 120 KB escaped, which is over CTX_MAX_BYTES on its own. This block is written + # *above* the full diff, so without the byte cap it is the block that drives the tail + # cut on a cycle-2+ generated-file PR -- and what the tail cut then removes is the + # full diff's omission notice and the conversation, not the since-diff that spent the + # budget. Half the budget, the same share the threads block gets, so the pair of + # blocks the reviewer cannot re-fetch can never crowd out everything else together. + # + # cap_file_escaped rather than a smaller SINCE_MAX: the prefix is still what this + # block wants, for the reason the full diff below no longer keeps one -- `gh api + # .../compare` is not allowlisted, so a notice in place of this patch leaves the + # reviewer nothing it can fetch instead. + SINCE_CAPPED="${RUNNER_TEMP}/since-capped.diff" + head -n "$SINCE_MAX" "$SINCE_FILE" > "$SINCE_CAPPED" + if [ "$SINCE_LINES" -gt "$SINCE_MAX" ]; then + echo "(${NOTICE_LINES} ${SINCE_MAX} of ${SINCE_LINES} lines)" >> "$SINCE_CAPPED" + fi + cap_file_escaped "$SINCE_CAPPED" $((PROMPT_BUDGET / 2)) \ + "${NOTICE_LINES} ${SINCE_MAX} of ${SINCE_LINES} lines, then cut to fit the prompt" { echo echo "## Diff since your last review (${LAST_SHA} to ${HEAD_SHA})" - head -n "$SINCE_MAX" "$SINCE_FILE" - if [ "$SINCE_LINES" -gt "$SINCE_MAX" ]; then - echo "(${NOTICE_LINES} ${SINCE_MAX} of ${SINCE_LINES} lines)" - fi + cat "$SINCE_CAPPED" } >> "$CTX" else { @@ -614,6 +640,31 @@ fi FULL_DIFF_MAX=$((DIFF_BUDGET_LINES - SINCE_USED)) if [ "$FULL_DIFF_MAX" -gt "$DIFF_MAX" ]; then FULL_DIFF_MAX=$DIFF_MAX; fi +# Issue comments, not the pull comments above: the PR conversation is a separate +# endpoint from the inline review threads, and only the threads were ever passed. +# +# Fetched and capped here, above the full diff, and written below it -- see the write site +# for why the two are separated. Capped in its own file rather than appended straight to +# $CTX, so the cap is on this block and not on the whole context: appending first and +# capping after is the tail cut, which is what put this block's heading off the end of the +# prompt on www.hotdata.dev#332. Stripped before the cap for the reason strip_block_tags +# always runs first -- the substitution grows the text, so a cap on the unstripped file +# bounds a smaller string than the one emitted. +ISSUE_COMMENTS_JQ='[.[][]] | if length == 0 then "No PR conversation comments." else sort_by(.created_at) | map("--- \(.user.login) at \(.created_at)\n\((.body // "")[0:3000])") | join("\n") end' +if CONVO_JSON=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate); then + CONVO=$(printf '%s' "$CONVO_JSON" | jq -s -r "$ISSUE_COMMENTS_JQ" 2>/dev/null) \ + || CONVO="Could not parse PR conversation comments." +else + echo "::warning::Could not read PR conversation comments." + CONVO="Could not read PR conversation comments." +fi +CONVO_FILE="${RUNNER_TEMP}/pr-conversation.md" +printf '%s\n' "$CONVO" | strip_block_tags > "$CONVO_FILE" +cap_file_escaped "$CONVO_FILE" "$CONVO_MAX_BYTES" \ + "${NOTICE_CONVO}; read the rest with gh pr view" +# What the block will really cost, heading included, rather than what it was allowed to. +CONVO_BYTES=$(($(escaped_bytes "$CONVO_FILE") + 100)) + DIFF_FILE="${RUNNER_TEMP}/pr.diff" if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then DIFF_LINES=$(awk 'END {print NR}' "$DIFF_FILE") @@ -624,8 +675,15 @@ if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then # the end of this file strips before it measures. strip_block_tags < "$CTX" > "${RUNNER_TEMP}/fit-ctx" strip_block_tags < "$DIFF_FILE" > "${RUNNER_TEMP}/fit-diff" - DIFF_ALLOWANCE=$((CTX_MAX_BYTES - $(escaped_bytes "${RUNNER_TEMP}/fit-ctx") - CONVO_MAX_BYTES)) + DIFF_ALLOWANCE=$((CTX_MAX_BYTES - $(escaped_bytes "${RUNNER_TEMP}/fit-ctx") - CONVO_BYTES)) DIFF_ESCAPED=$(escaped_bytes "${RUNNER_TEMP}/fit-diff") + # Set in the omission branch below and read after the group command. `{ ... } >> file` is + # a group, not a subshell, so the assignment survives -- which is the point: spelling the + # condition a second time thirty lines down leaves the two free to drift, and the drift is + # silent in exactly the direction that matters. The block would render the omission text + # while the annotation said nothing, or the reverse, which is the invisible failure this + # annotation exists to end. + DIFF_OMITTED=0 { echo echo "## Full diff" @@ -651,6 +709,7 @@ if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then # attempts over those two weeks. The since-diff above keeps its prefix precisely # because it has no such escape -- `gh api .../compare` is not allowlisted, so # trading its prefix for a notice would trade partial information for none. + DIFF_OMITTED=1 echo "(${NOTICE_OMITTED}; ${DIFF_LINES} lines)" echo echo "The patch is NOT below. Nothing has been shown to you and nothing has been" @@ -665,8 +724,7 @@ if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then echo " - Say in your review that the diff was omitted and which files you read." fi } >> "$CTX" - if [ "$DIFF_LINES" -gt 0 ] \ - && { [ "$DIFF_LINES" -gt "$FULL_DIFF_MAX" ] || [ "$DIFF_ESCAPED" -gt "$DIFF_ALLOWANCE" ]; }; then + if [ "$DIFF_OMITTED" -eq 1 ]; then # A notice, not a warning, for the same reason the byte cap below uses one: a # generated-file PR reaches this legitimately and a warning that cried regression on # every one of them would stop being read. It exists so an operator reading a thin @@ -680,25 +738,15 @@ else { echo; echo "## Full diff"; echo "Could not read the diff; run gh pr diff."; } >> "$CTX" fi -# Issue comments, not the pull comments above: the PR conversation is a separate -# endpoint from the inline review threads, and only the threads were ever passed. -ISSUE_COMMENTS_JQ='[.[][]] | if length == 0 then "No PR conversation comments." else sort_by(.created_at) | map("--- \(.user.login) at \(.created_at)\n\((.body // "")[0:3000])") | join("\n") end' -if CONVO_JSON=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate); then - CONVO=$(printf '%s' "$CONVO_JSON" | jq -s -r "$ISSUE_COMMENTS_JQ" 2>/dev/null) \ - || CONVO="Could not parse PR conversation comments." -else - echo "::warning::Could not read PR conversation comments." - CONVO="Could not read PR conversation comments." -fi -# Capped in its own file rather than appended straight to $CTX, so the cap is on this block -# and not on the whole context: appending first and capping after is the tail cut, which is -# what put this block's heading off the end of the prompt on www.hotdata.dev#332. Stripped -# before the cap for the reason strip_block_tags always runs first -- the substitution grows -# the text, so a cap on the unstripped file bounds a smaller string than the one emitted. -CONVO_FILE="${RUNNER_TEMP}/pr-conversation.md" -printf '%s\n' "$CONVO" | strip_block_tags > "$CONVO_FILE" -cap_file_escaped "$CONVO_FILE" "$CONVO_MAX_BYTES" \ - "${NOTICE_CONVO}; read the rest with gh pr view" +# Written last, prepared above the full diff. The ordering argument for writing it last is +# unchanged -- it is the block the reviewer can most afford to lose. But the diff's fit +# decision has to subtract what this block will actually weigh, and a reserve of +# CONVO_MAX_BYTES is not that: the common case is "No PR conversation comments.", 29 bytes, +# and reserving an eighth of the budget against it hands back around 1,200 patch lines that +# nothing will spend. That over-reserve was cheap while the shortfall cost the diff a +# prefix; now it costs the whole block, so it would convert directly into omissions on pull +# requests whose diff would have fit. Fetching here and measuring the capped file makes the +# reserve exact. This endpoint does not depend on the diff, so nothing else moves. { echo; echo "## PR conversation"; cat "$CONVO_FILE"; } >> "$CTX" # CTX_MAX_BYTES is derived above, beside the threads cap it is computed from, because the diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 886777f..3efeb31 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -134,7 +134,25 @@ case "$args" in fi ;; *"/pulls/"*"/commits"*) fail_if_marked commits; cat "$FIXTURES/pull-commits.json" ;; - *"/pulls/"*"/files"*) fail_if_marked files; cat "$FIXTURES/pull-files.json" ;; + *"/pulls/"*"/files"*) + fail_if_marked files + # STUB_FILES reaches the far end of `--paginate` on this endpoint: GitHub returns up to + # 3,000 files, and the block renders a line each. The shipped fixture is the ordinary + # case and every other file assertion is written against it. + if [ "${STUB_FILES:-0}" -gt 0 ]; then + awk -v n="$STUB_FILES" 'BEGIN { + printf "["; + for (i = 0; i < n; i++) { + if (i) printf ","; + printf "{\"status\":\"modified\",\"additions\":%d,\"deletions\":%d,", i % 90, i % 7; + printf "\"filename\":\"packages/generated/module_%06d/src/deeply/nested/path/component_%06d.ts\"}", i, i; + } + printf "]\n"; + }' + else + cat "$FIXTURES/pull-files.json" + fi + ;; *"/issues/"*"/comments"*) fail_if_marked issue_comments if [ "$STUB_CONVO_COMMENTS" -gt 0 ]; then @@ -198,7 +216,18 @@ case "$args" in # Padding, so the since-diff can be made to compete with the full diff for the # shared line budget. Tagged distinctly from the full diff's "+line" so a test # can tell which block a rendered line came from. - awk -v n="$STUB_SINCE_LINES" 'BEGIN { for (i = 1; i <= n; i++) print "+since " i }' + # + # STUB_SINCE_STYLE is the since-diff's twin of STUB_DIFF_STYLE below, and it has to + # exist separately: SINCE_MAX bounds this block in lines only, so a test that wants + # to reach its *byte* cap needs long lines here, and a short "+since N" cannot get + # there at any line count SINCE_MAX permits. + awk -v n="$STUB_SINCE_LINES" -v style="$STUB_SINCE_STYLE" 'BEGIN { + for (i = 1; i <= n; i++) { + if (style == "json") + printf "+ \"since\": \"line %d, \\\"quoted\\\" text, and enough further payload on this line to make it dense\",\n", i; + else print "+since " i; + } + }' ;; *) printf '{"status":"%s","ahead_by":2,"behind_by":0}\n' "$COMPARE_STATUS" @@ -257,6 +286,8 @@ run_step() { STUB_DIFF_TAGS="${STUB_DIFF_TAGS:-0}" \ STUB_DIFF_STYLE="${STUB_DIFF_STYLE:-plain}" \ STUB_SINCE_LINES="${STUB_SINCE_LINES:-0}" \ + STUB_SINCE_STYLE="${STUB_SINCE_STYLE:-plain}" \ + STUB_FILES="${STUB_FILES:-0}" \ FAIL_ENDPOINT="${FAIL_ENDPOINT:-none}" \ HEAD_SHA="${HEAD_SHA:-1d01475432236aa4fbca722aaaa2687c2b2e4947}" \ BASE_REF=main \ @@ -626,6 +657,81 @@ expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a diff that fits" expect "$(grep -c '^+line ' "$CTX_FILE" || true)" "40" "a diff that fits is passed whole" expect_no_context 'full diff omitted' "a diff that fits carries no omission notice" +# What the diff reserves for the conversation has to be what the conversation will weigh, +# not what it was allowed to weigh. The common case is "No PR conversation comments." at 29 +# bytes; reserving an eighth of the budget against that hands back around 1,200 patch lines +# that nothing will spend. Cheap while the shortfall cost the diff a prefix, and not cheap +# now that it costs the whole block -- an over-reserve converts directly into omissions on +# pull requests whose diff would have fit. +# +# 1,800 lines of quote-dense diff is inside that window: it fits beside an empty +# conversation and not beside a conversation at its cap. A reserve that is a constant +# cannot tell those two runs apart, so this pair is what pins the reserve to the real size. +STUB_DIFF_LINES=1800 STUB_DIFF_STYLE=json run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a diff at the edge of its allowance" +expect "$(grep -c 'description' "$CTX_FILE" || true)" "1800" \ + "a diff inside the allowance renders whole when the conversation is empty" +STUB_DIFF_LINES=1800 STUB_DIFF_STYLE=json STUB_CONVO_COMMENTS=300 run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on the same diff beside a full conversation" +expect_context 'full diff omitted: too large for the review prompt' \ + "the same diff is omitted once the conversation really needs its share" + +# The annotation and the block have to agree. They used to be two spellings of the same +# condition thirty lines apart, free to drift into a context that says the diff is absent +# while the run reports nothing, or the reverse -- which is the invisible failure the +# annotation exists to end. `{ ... } >> file` is a group command, not a subshell, so the +# flag set inside it is what both readers use. +STUB_DIFF_LINES=4000 run_step > /dev/null +if grep -q 'full diff omitted' "$CTX_FILE" \ + && grep -q '::notice::Full diff omitted from the review prompt' "$WORK/step.out"; then + echo "ok an omitted diff is annotated as well as announced in the context" +else + echo "FAIL the omission notice and the ::notice:: annotation disagree:" + printf ' context says omitted: %s, annotation present: %s\n' \ + "$(grep -q 'full diff omitted' "$CTX_FILE" && echo yes || echo no)" \ + "$(grep -q '::notice::Full diff omitted' "$WORK/step.out" && echo yes || echo no)" + failures=$((failures + 1)) +fi +STUB_DIFF_LINES=40 run_step > /dev/null +if grep -q '::notice::Full diff omitted' "$WORK/step.out"; then + echo "FAIL a diff that was passed whole was annotated as omitted" + failures=$((failures + 1)) +else + echo "ok a diff that fits is not annotated as omitted" +fi + +# The two blocks above the diff that had no byte cap of their own. Both are written before +# `## Full diff`, so an overflow in either is paid for by the diff's omission notice and the +# conversation -- the tail cut works from the end, and they do not sit at the end. +# +# The since-diff is the sharper of the two: SINCE_MAX bounds it in lines, and at the 1.20x +# this budget measures for quote-dense patches, 2,000 lines of dashboard JSON is about 120 +# KB escaped, over CTX_MAX_BYTES on its own. So on any cycle-2+ generated-file PR that one +# block drove the tail cut. +STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a since-diff that is huge in bytes" +expect_context '^## Diff since your last review' "the since-diff block renders" +expect_context '^\+ "since": "line 1' "the since-diff keeps a prefix rather than a notice" +expect_context 'cut to fit the prompt' "an over-byte since-diff says it was cut" +# The blocks written after it, which are what the tail cut would have taken instead. +expect_context '^## Full diff' "the full diff heading survives a byte-heavy since-diff" +expect_context '^## PR conversation' "the conversation survives a byte-heavy since-diff" +expect "$(escaped_of "$CTX_FILE" \ + | awk -v lim="$PROMPT_ARG_LIMIT" '{print ($1 <= lim) ? "bounded" : "over"}')" \ + "bounded" "a byte-heavy since-diff stays inside the argument limit" + +# The changed-file list, the other block with no byte cap and the one the omission notice +# sends the reviewer to. `--paginate` returns up to GitHub's 3,000-file ceiling at around +# 60 bytes a line. +STUB_FILES=3000 run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a PR with thousands of changed files" +expect_context '^## Changed files' "the changed-file block renders" +expect_context 'changed file list truncated' "an oversized changed-file list says it was cut" +expect_context '^## Full diff' "the full diff heading survives a huge changed-file list" +expect "$(escaped_of "$CTX_FILE" \ + | awk -v lim="$PROMPT_ARG_LIMIT" '{print ($1 <= lim) ? "bounded" : "over"}')" \ + "bounded" "thousands of changed files stay inside the argument limit" + # --- Truncation -------------------------------------------------------------------------- # An empty body under a heading is a claim: "## Full diff" with nothing beneath it reads as @@ -656,8 +762,8 @@ expect "$(escaped_of "$CTX_FILE" \ | awk -v lim="$PROMPT_ARG_LIMIT" '{print ($1 <= lim) ? "bounded" : "over"}')" \ "bounded" "hundreds of conversation comments stay inside the argument limit" -# The tail cut, which is now a backstop rather than the ordinary path. Every block above is -# bounded except the PR body, which is author-controlled and has no cap -- a generated +# The tail cut, which is now a backstop rather than the ordinary path. Every *fetched* block +# is bounded; the PR body arrives through `env:` rather than an API read and has no cap -- a generated # release-note body is how a context still reaches the limit. Reaching it is a claim too: # the cut lands wherever the byte count runs out, so the notice has to be appended *after* # the cut or it is the first thing removed. From 2028203bcc3dd2a71740c3f72bd5a5c427f23f37 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 21 Aug 2026 12:12:09 -0700 Subject: [PATCH 3/5] fix(review): measure block shares against the context, not the prompt --- docs/claude-pr-review-prompt.md | 2 +- scripts/gather-review-context.sh | 64 ++++++++++++++++++++++---------- tests/context-step-test.sh | 41 +++++++++++++++++++- 3 files changed, 86 insertions(+), 21 deletions(-) diff --git a/docs/claude-pr-review-prompt.md b/docs/claude-pr-review-prompt.md index 13ef5ac..a61d602 100644 --- a/docs/claude-pr-review-prompt.md +++ b/docs/claude-pr-review-prompt.md @@ -11,7 +11,7 @@ Everything in `` is already in front of you. Do not spend a tool cal **Unless it is not there.** If `` 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`, `PR conversation 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`, `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: diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index c7c5fa1..35ec168 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -160,17 +160,6 @@ fi # not exist. LOG_BUDGET=$((PROMPT_BUDGET / 4)) LOG_REMAINING=$LOG_BUDGET -# The PR conversation is the last block written, which without a cap of its own makes it -# both the block that overflows the budget and the block the tail cut removes to pay for -# the overflow -- 300 comments rendered 700 KB, and on www.hotdata.dev#332 the heading did -# not render at all. An eighth of the budget, the smallest share of the four, because it is -# the block the reviewer can most afford to lose: the inline threads carry the review -# history and this carries the rest of the discussion. -# -# Bounding it is also what makes the full diff's fit decision below answerable. That -# decision asks "is there room for the whole patch", and the question has no answer while -# an unbounded block is still to come. -CONVO_MAX_BYTES=$((PROMPT_BUDGET / 8)) # What the summary may take of it. The excerpts are written summary first, window # second, but the window is the block worth more: across five real failed job logs the # cause sat immediately above the first ##[error] in four, and the summary is what @@ -191,6 +180,8 @@ NOTICE_THREADS='prior review comments truncated' NOTICE_LOG='log excerpt truncated' NOTICE_LINES='truncated: first' NOTICE_CONVO='PR conversation truncated' +NOTICE_FILES='changed file list truncated' +NOTICE_SINCE='since-diff cut to fit the review prompt' # Not a truncation notice: this block is absent, not short. It is listed with the others # because the contract is the same -- the prompt document has to quote it, or the reviewer # reads a diff heading with no patch under it and no idea that a patch exists. @@ -418,6 +409,23 @@ if [ "$CTX_MAX_BYTES" -lt 0 ]; then CTX_MAX_BYTES=0 fi +# The PR conversation is the last block written, which without a cap of its own makes it +# both the block that overflows the budget and the block the tail cut removes to pay for +# the overflow -- 300 comments rendered 700 KB, and on www.hotdata.dev#332 the heading did +# not render at all. An eighth of the context, the smallest share, because it is the block +# the reviewer can most afford to lose: the inline threads carry the review history and +# this carries the rest of the discussion. +# +# Denominated in CTX_MAX_BYTES rather than PROMPT_BUDGET, which is why it is derived here +# and not beside the other caps: an eighth of the larger total is a quarter of the context +# once threads is at its own cap, and a share that grows when the review history grows is +# not a share. The same reading applies to the changed-file and since-diff caps below. +# +# Bounding it is also what makes the full diff's fit decision answerable. That decision +# asks "is there room for the whole patch", and the question has no answer while an +# unbounded block is still to come. +CONVO_MAX_BYTES=$((CTX_MAX_BYTES / 8)) + DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)" { echo "threads<<${DELIMITER}" @@ -481,8 +489,8 @@ fi # eighth of the budget, the same share as the conversation. FILES_FILE="${RUNNER_TEMP}/changed-files.md" printf '%s\n' "$FILES" | strip_block_tags > "$FILES_FILE" -cap_file_escaped "$FILES_FILE" $((PROMPT_BUDGET / 8)) \ - "changed file list truncated; read the rest with gh pr view --json files" +cap_file_escaped "$FILES_FILE" $((CTX_MAX_BYTES / 8)) \ + "${NOTICE_FILES}; read the rest with gh pr view --json files" { echo; echo "## Changed files"; cat "$FILES_FILE"; } >> "$CTX" # The reviewer cannot run tests -- no dependencies are installed and the allowlist @@ -594,16 +602,21 @@ if [ -n "$LAST_SHA" ] && [ "$LAST_SHA" != "null" ] && [ "$LAST_SHA" != "$HEAD_SH # awk, not `wc -l`: wc pads its count with spaces on BSD and the number # is interpolated into the notice below, not just compared. SINCE_LINES=$(awk 'END {print NR}' "$SINCE_FILE") - SINCE_USED=$SINCE_LINES - if [ "$SINCE_USED" -gt "$SINCE_MAX" ]; then SINCE_USED=$SINCE_MAX; fi # Line-capped and then byte-capped, because a line cap does not bound bytes: at the # 1.20x this file measures for quote-dense JSON, 2,000 lines of dashboard patch is # about 120 KB escaped, which is over CTX_MAX_BYTES on its own. This block is written # *above* the full diff, so without the byte cap it is the block that drives the tail # cut on a cycle-2+ generated-file PR -- and what the tail cut then removes is the # full diff's omission notice and the conversation, not the since-diff that spent the - # budget. Half the budget, the same share the threads block gets, so the pair of - # blocks the reviewer cannot re-fetch can never crowd out everything else together. + # budget. + # + # Half of CTX_MAX_BYTES, not of PROMPT_BUDGET. Those are the same number only when + # there is no review history: threads has already taken its own half out of + # PROMPT_BUDGET by this point, so on a cycle-5+ PR with a long comment history a share + # denominated in the larger total is the whole of what remains, and this one block can + # fill the context allowance by itself -- the very failure the byte cap is here to + # stop, one level up. The context is what this block writes into, so the context is + # what its share is measured against. # # cap_file_escaped rather than a smaller SINCE_MAX: the prefix is still what this # block wants, for the reason the full diff below no longer keeps one -- `gh api @@ -614,8 +627,21 @@ if [ -n "$LAST_SHA" ] && [ "$LAST_SHA" != "null" ] && [ "$LAST_SHA" != "$HEAD_SH if [ "$SINCE_LINES" -gt "$SINCE_MAX" ]; then echo "(${NOTICE_LINES} ${SINCE_MAX} of ${SINCE_LINES} lines)" >> "$SINCE_CAPPED" fi - cap_file_escaped "$SINCE_CAPPED" $((PROMPT_BUDGET / 2)) \ - "${NOTICE_LINES} ${SINCE_MAX} of ${SINCE_LINES} lines, then cut to fit the prompt" + # Its own notice, rather than a second copy of the line-cap wording, because the two + # caps fire independently and the byte cap fires at a *lower* line count than + # SINCE_MAX on exactly the dense content it exists for. Reusing the line wording there + # printed "truncated: first 2000 of 1500 lines" -- a claim about a cut that did not + # happen, naming a figure the reviewer was not given. These notices are a contract + # with the prompt document; one of them stating a falsehood is worse than none. + cap_file_escaped "$SINCE_CAPPED" $((CTX_MAX_BYTES / 2)) \ + "${NOTICE_SINCE}; read the whole patch with gh pr diff" + # Recounted after both caps, because this is what the full diff is charged for. Fixing + # it at min(SINCE_LINES, SINCE_MAX) before the byte cap charged the full diff for lines + # this block did not end up spending: a dense since-diff cut to 400 rendered lines still + # reserved 2,000, leaving FULL_DIFF_MAX at 1,000 and omitting a 1,500-line full diff + # that the byte allowance had ample room for. The same over-reserve the conversation + # had, in the other currency, and with no notice to explain where the budget went. + SINCE_USED=$(awk 'END {print NR}' "$SINCE_CAPPED") { echo echo "## Diff since your last review (${LAST_SHA} to ${HEAD_SHA})" diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 3efeb31..101b139 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -708,11 +708,50 @@ fi # this budget measures for quote-dense patches, 2,000 lines of dashboard JSON is about 120 # KB escaped, over CTX_MAX_BYTES on its own. So on any cycle-2+ generated-file PR that one # block drove the tail cut. +# With a maxed review history as well, which is the case a share of the wrong total hides. +# Threads takes half of PROMPT_BUDGET, so CTX_MAX_BYTES is the other half -- and a since-diff +# cap denominated in PROMPT_BUDGET is then the whole of the context allowance, letting one +# block fill it and handing the tail cut everything ordered after it: the full diff's +# omission notice, the instructions under it, and the conversation. A cycle-5+ PR with a long +# comment history and a dense patch is exactly that run, and it is the shape the byte cap was +# added to prevent one level up. +# The full diff is oversized here too, so what has to survive is the whole omission block -- +# heading, notice and instructions. That is the text the tail cut took when this share was +# denominated in the wrong total, and it is the text the reviewer needs most on this run. +STUB_THREAD_COMMENTS=60 STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json STUB_DIFF_LINES=4000 \ + run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a dense since-diff beside a long history" +expect_context '^## Full diff' "the full diff heading survives both at once" +expect_context 'full diff omitted: too large for the review prompt' \ + "the omission notice survives both at once" +expect_context 'The patch is NOT below' "the omission instructions survive both at once" +expect_context '^## PR conversation' "the conversation survives both at once" + STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json run_step > "$WORK/code.txt" expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a since-diff that is huge in bytes" expect_context '^## Diff since your last review' "the since-diff block renders" expect_context '^\+ "since": "line 1' "the since-diff keeps a prefix rather than a notice" -expect_context 'cut to fit the prompt' "an over-byte since-diff says it was cut" +expect_context 'since-diff cut to fit the review prompt' "an over-byte since-diff says it was cut" +# And says it in wording that is true. Reusing the line-cap notice here printed "truncated: +# first 2000 of 1500 lines" on a block whose line count was never cut -- the byte cap fires +# independently, and on dense content it fires below SINCE_MAX. A notice that names a figure +# the reviewer was not given is worse than no notice; the document quotes these. +STUB_SINCE_LINES=1500 STUB_SINCE_STYLE=json run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a dense since-diff inside SINCE_MAX" +expect_context 'since-diff cut to fit the review prompt' \ + "a since-diff cut only by bytes says so" +expect_no_context 'truncated: first 2000 of 1500' \ + "no notice claims a line cut that did not happen" + +# What the full diff is charged for has to be what the since-diff actually spent. Fixing +# SINCE_USED at min(SINCE_LINES, SINCE_MAX) before the byte cap ran charged it for lines the +# block did not keep: a dense since-diff cut to a few hundred rendered lines still reserved +# 2,000, leaving FULL_DIFF_MAX at 1,000 and omitting a 1,500-line full diff with the byte +# allowance wide open. The same over-reserve as the conversation's, in the other currency. +STUB_SINCE_LINES=1500 STUB_SINCE_STYLE=json STUB_DIFF_LINES=1500 run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 with a dense since-diff and a full diff" +expect "$(grep -c '^+line ' "$CTX_FILE" || true)" "1500" \ + "the full diff is charged only for the lines the since-diff kept" # The blocks written after it, which are what the tail cut would have taken instead. expect_context '^## Full diff' "the full diff heading survives a byte-heavy since-diff" expect_context '^## PR conversation' "the conversation survives a byte-heavy since-diff" From 749c3bffab0f4185245f6cc04ad10066dd180b0f Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 21 Aug 2026 12:22:58 -0700 Subject: [PATCH 4/5] fix(review): denominate the log allowance in the context too --- README.md | 12 ++++++---- scripts/gather-review-context.sh | 40 +++++++++++++++++++++----------- tests/context-step-test.sh | 22 ++++++++++++++++++ 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 472f02f..57b5eb5 100644 --- a/README.md +++ b/README.md @@ -41,11 +41,13 @@ Both step outputs are interpolated into one `prompt:` string, so the binding lim "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. Beneath that total every block holds a byte share — -comment threads and the since-last-review diff at most half each, CI log excerpts a quarter, the PR -conversation and the changed-file list an eighth — with the two diff blocks also sharing 3,000 patch -lines. A line cap is 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 needed both. +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 at most half, CI log excerpts a quarter, the PR conversation and the changed-file list an eighth +each — with the two diff blocks also sharing 3,000 patch lines. The denominator is the point: a share +of the whole budget is twice the share it claims to be once the review history is long, which is +precisely the case where blocks compete. 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. 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 diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index 35ec168..ef56317 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -155,19 +155,18 @@ fi # tail, so the diff is the block that paid: three verbose failing jobs took it out of # the context entirely. 40000 was sized against the old 200 KB context cap, so the # allowance is scaled off PROMPT_BUDGET like THREADS_MAX_BYTES instead. It is not also -# kept as a per-excerpt cap: a quarter of the budget is around 30 KB, so 40000 could +# kept as a per-excerpt cap: a quarter of the context is around 15 KB, so 40000 could # never be the binding number and stating it would only imply a second bound that does # not exist. -LOG_BUDGET=$((PROMPT_BUDGET / 4)) -LOG_REMAINING=$LOG_BUDGET -# What the summary may take of it. The excerpts are written summary first, window -# second, but the window is the block worth more: across five real failed job logs the -# cause sat immediately above the first ##[error] in four, and the summary is what -# covers the fifth. Sharing an allowance first-come-first-served would invert that -- -# `tail -n 20` bounds the summary in lines, not bytes, so twenty stack-trace or JSON -# lines take everything and the window for the same job renders as its own truncation -# notice. Held to a quarter so the window keeps the larger share of whatever is left. -LOG_SUMMARY_MAX=$((LOG_BUDGET / 4)) +# +# Derived below, beside CTX_MAX_BYTES, rather than here: like the conversation, the +# changed-file list and the since-diff, this share is of the context these excerpts are +# written into and not of the whole prompt budget. A quarter of PROMPT_BUDGET is *half* the +# context once threads is at its own cap, and it is charged raw rather than escaped -- +# cap_log_excerpt bills through wc -c -- so at the 1.20x this file measures for +# quote-dense text those bytes arrive larger than they were counted. Three verbose failing +# jobs beside a long review history is the run where that lands on the tail cut, and what +# the tail cut takes is the conversation and then the full diff's omission instructions. # The truncation notices, as constants rather than literals at their call sites. The # prompt document quotes them and tells the reviewer that seeing one means the block is # incomplete and the rest has to be fetched before drawing conclusions from it -- so a @@ -419,12 +418,27 @@ fi # Denominated in CTX_MAX_BYTES rather than PROMPT_BUDGET, which is why it is derived here # and not beside the other caps: an eighth of the larger total is a quarter of the context # once threads is at its own cap, and a share that grows when the review history grows is -# not a share. The same reading applies to the changed-file and since-diff caps below. +# not a share. Every per-block share of the context reads the same way -- the log allowance +# below, the changed-file list, and the since-diff. Only threads is a share of +# PROMPT_BUDGET, because it is the block CTX_MAX_BYTES is derived by subtracting. # # Bounding it is also what makes the full diff's fit decision answerable. That decision # asks "is there room for the whole patch", and the question has no answer while an # unbounded block is still to come. CONVO_MAX_BYTES=$((CTX_MAX_BYTES / 8)) +# The log allowance, for the reasons given where LOG_WINDOW is set. Same denominator, same +# argument: these excerpts are written into the context, so the context is what they are a +# share of. +LOG_BUDGET=$((CTX_MAX_BYTES / 4)) +LOG_REMAINING=$LOG_BUDGET +# What the summary may take of it. The excerpts are written summary first, window +# second, but the window is the block worth more: across five real failed job logs the +# cause sat immediately above the first ##[error] in four, and the summary is what +# covers the fifth. Sharing an allowance first-come-first-served would invert that -- +# `tail -n 20` bounds the summary in lines, not bytes, so twenty stack-trace or JSON +# lines take everything and the window for the same job renders as its own truncation +# notice. Held to a quarter so the window keeps the larger share of whatever is left. +LOG_SUMMARY_MAX=$((LOG_BUDGET / 4)) DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)" { @@ -486,7 +500,7 @@ fi # file is around 60 bytes -- 180 KB, over the whole budget, from a block with no cap of its # own. It matters more than its size suggests: this is the block the full diff's omission # notice sends the reviewer to, so it is the last one that should be able to overflow. An -# eighth of the budget, the same share as the conversation. +# eighth of the context, the same share as the conversation. FILES_FILE="${RUNNER_TEMP}/changed-files.md" printf '%s\n' "$FILES" | strip_block_tags > "$FILES_FILE" cap_file_escaped "$FILES_FILE" $((CTX_MAX_BYTES / 8)) \ diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 101b139..982da5f 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -896,6 +896,28 @@ fi expect_context '^## Full diff' "the diff block survives three jobs of enormous logs" expect_context '^\+line 1$' "the diff body survives three jobs of enormous logs" +# The same three jobs beside a maxed review history, which is where the log allowance's +# denominator shows. A quarter of PROMPT_BUDGET is *half* the context once threads is at its +# own cap, and cap_log_excerpt charges raw bytes rather than escaped, so the excerpts arrive +# larger than they were counted. The blocks written after them are what pays: the +# conversation, and the full diff's omission instructions -- the text the shares exist to +# protect. The diff is oversized here so that text is what has to be present. +# The since-diff is dense here too, at its own half of the context: a log allowance that is +# a quarter of the *budget* is another half, and the two together are the whole of it before +# the changed-file list and the conversation are written. That sum is the run, and it is why +# both shares have to read off the same denominator -- fixing either alone leaves the other +# free to spend what the first gave back. +STUB_THREAD_COMMENTS=60 STUB_FAILING_JOBS=3 STUB_JOB_LOG="$FAT_LOG" STUB_DIFF_LINES=4000 \ + STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json \ + run_step > "$WORK/code.txt" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on three fat logs beside a long history" +expect_context 'full diff omitted: too large for the review prompt' \ + "the omission notice survives three fat logs and a long history" +expect_context 'The patch is NOT below' \ + "the omission instructions survive three fat logs and a long history" +expect_context '^## PR conversation' \ + "the conversation survives three fat logs and a long history" + # Sharing a budget decides *what* the excerpts spend it on, and the region total above cannot # see that. The summary is written first and the first-error window second, but the window is # the block worth the most: across five real failed logs the cause sat immediately above the From 895cf18e0b0753ac42682792f0e5393d7ce38305 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 21 Aug 2026 12:35:41 -0700 Subject: [PATCH 5/5] fix(review): leave headroom in the sum of the block shares --- README.md | 13 +++++---- docs/claude-pr-review-prompt.md | 2 +- scripts/gather-review-context.sh | 26 +++++++++++++---- tests/context-step-test.sh | 49 ++++++++++++++++++++++++++++---- 4 files changed, 74 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 57b5eb5..7df58e8 100644 --- a/README.md +++ b/README.md @@ -43,11 +43,14 @@ Both step outputs are interpolated into one `prompt:` string, so the binding lim 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 at most half, CI log excerpts a quarter, the PR conversation and the changed-file list an eighth -each — with the two diff blocks also sharing 3,000 patch lines. The denominator is the point: a share -of the whole budget is twice the share it claims to be once the review history is long, which is -precisely the case where blocks compete. 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. +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 diff --git a/docs/claude-pr-review-prompt.md b/docs/claude-pr-review-prompt.md index a61d602..a30f144 100644 --- a/docs/claude-pr-review-prompt.md +++ b/docs/claude-pr-review-prompt.md @@ -11,7 +11,7 @@ Everything in `` is already in front of you. Do not spend a tool cal **Unless it is not there.** If `` 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`, `PR conversation truncated`, `changed file 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 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: diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh index ef56317..513ac42 100755 --- a/scripts/gather-review-context.sh +++ b/scripts/gather-review-context.sh @@ -180,6 +180,7 @@ NOTICE_LOG='log excerpt truncated' NOTICE_LINES='truncated: first' NOTICE_CONVO='PR conversation truncated' NOTICE_FILES='changed file list truncated' +NOTICE_COMMITS='commit list truncated' NOTICE_SINCE='since-diff cut to fit the review prompt' # Not a truncation notice: this block is absent, not short. It is listed with the others # because the contract is the same -- the prompt document has to quote it, or the reviewer @@ -230,9 +231,14 @@ cap_log_excerpt() { echo "($2)" >> "$1" fi else - cap_file "$1" "$limit" "$2" + # Escaped, like every other cap in this file. Billing raw `wc -c` against a share of a + # budget denominated in escaped bytes under-charged by whatever the content's expansion + # is -- about 1.20x for the quote-dense text a CI log is full of -- so an allowance of + # a quarter of the context arrived as nearer a third of it, and the blocks written after + # these excerpts paid the difference. + cap_file_escaped "$1" "$limit" "$2" fi - LOG_REMAINING=$((LOG_REMAINING - $(wc -c < "$1" | tr -d ' '))) + LOG_REMAINING=$((LOG_REMAINING - $(escaped_bytes "$1"))) if [ "$LOG_REMAINING" -lt 0 ]; then LOG_REMAINING=0; fi } @@ -429,7 +435,7 @@ CONVO_MAX_BYTES=$((CTX_MAX_BYTES / 8)) # The log allowance, for the reasons given where LOG_WINDOW is set. Same denominator, same # argument: these excerpts are written into the context, so the context is what they are a # share of. -LOG_BUDGET=$((CTX_MAX_BYTES / 4)) +LOG_BUDGET=$((CTX_MAX_BYTES / 8)) LOG_REMAINING=$LOG_BUDGET # What the summary may take of it. The excerpts are written summary first, window # second, but the window is the block worth more: across five real failed job logs the @@ -482,7 +488,17 @@ else echo "::warning::Could not read commits." COMMITS="Could not read commits." fi -{ echo; echo "## Commits"; printf '%s\n' "$COMMITS"; } >> "$CTX" +# Capped for the same reason as the changed-file list below, and it was the last fetched +# block without one. The endpoint tops out at 250 commits, so the ordinary ceiling is around +# 15 KB -- already a quarter of the context on a maxed-threads run -- but the rendered line +# is a SHA and a git subject, and a git subject has no length bound, so the real ceiling is +# whatever the author wrote. It sits above `## Full diff`, so an overflow here is paid for by +# the omission notice and the conversation. +COMMITS_FILE="${RUNNER_TEMP}/commits.md" +printf '%s\n' "$COMMITS" | strip_block_tags > "$COMMITS_FILE" +cap_file_escaped "$COMMITS_FILE" $((CTX_MAX_BYTES / 8)) \ + "${NOTICE_COMMITS}; read the rest with gh pr view --json commits" +{ echo; echo "## Commits"; cat "$COMMITS_FILE"; } >> "$CTX" # status carries added/modified/removed/renamed, which the raw patch does not spell # out for renames, and the per-file counts let the reviewer budget its reading. @@ -647,7 +663,7 @@ if [ -n "$LAST_SHA" ] && [ "$LAST_SHA" != "null" ] && [ "$LAST_SHA" != "$HEAD_SH # printed "truncated: first 2000 of 1500 lines" -- a claim about a cut that did not # happen, naming a figure the reviewer was not given. These notices are a contract # with the prompt document; one of them stating a falsehood is worse than none. - cap_file_escaped "$SINCE_CAPPED" $((CTX_MAX_BYTES / 2)) \ + cap_file_escaped "$SINCE_CAPPED" $((CTX_MAX_BYTES / 3)) \ "${NOTICE_SINCE}; read the whole patch with gh pr diff" # Recounted after both caps, because this is what the full diff is charged for. Fixing # it at min(SINCE_LINES, SINCE_MAX) before the byte cap charged the full diff for lines diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 982da5f..29670dc 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -907,16 +907,55 @@ expect_context '^\+line 1$' "the diff body survives three jobs of enormous logs" # the changed-file list and the conversation are written. That sum is the run, and it is why # both shares have to read off the same denominator -- fixing either alone leaves the other # free to spend what the first gave back. +# Every capped block at its ceiling simultaneously, which is what makes the shares' *sum* +# the quantity under test rather than any one of them. When they summed to exactly 1 -- +# since-diff CTX/2, logs CTX/4, files CTX/8, conversation CTX/8 -- there was no headroom +# left for the PR body, the commits, the CI list, the headings or the diff, and the log +# share spent more than it was charged because it billed raw where everything else billed +# escaped. `DIFF_ALLOWANCE` goes negative on this run, so the diff correctly omits itself; +# the context was over budget before the omission text and `## PR conversation` were +# written, and the tail cut takes those bottom-up. What it leaves is a `## Full diff` +# heading with the context-truncation notice under it -- a heading with nothing beneath it, +# which the empty-diff branch exists to prevent by another route. STUB_THREAD_COMMENTS=60 STUB_FAILING_JOBS=3 STUB_JOB_LOG="$FAT_LOG" STUB_DIFF_LINES=4000 \ - STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json \ + STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json STUB_FILES=3000 STUB_CONVO_COMMENTS=300 \ run_step > "$WORK/code.txt" -expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on three fat logs beside a long history" +expect "$(cat "$WORK/code.txt")" "0" "step exits 0 with every capped block at its ceiling" expect_context 'full diff omitted: too large for the review prompt' \ - "the omission notice survives three fat logs and a long history" + "the omission notice survives every block at its ceiling" expect_context 'The patch is NOT below' \ - "the omission instructions survive three fat logs and a long history" + "the omission instructions survive every block at its ceiling" expect_context '^## PR conversation' \ - "the conversation survives three fat logs and a long history" + "the conversation survives every block at its ceiling" +# The sum, stated as a margin rather than as a pass/fail against the limit. Boundedness is +# too weak to be the assertion here: with the shares summing to exactly 1 this run measured +# 58,397 escaped against an allowance of 60,356 -- inside it, but by 3%, and only because +# cap_file_escaped hands back 300 bytes a call and undershoots its target by 2%. A worst case +# that clears by rounding error is one comment line from the tail cut, and the argument for +# having per-block shares at all is that the tail cut stays a backstop. The same run now +# measures around 40,600, near 67% of the allowance. +# +# The allowance is derived the way the script derives it, from the same declared constants, +# so a prompt document that grows or a share that is widened moves this number here too +# rather than silently spending the margin. +maxed_ctx=$(escaped_of "$CTX_FILE") +maxed_threads=$(escaped_of "$THREADS_FILE_OUT") +# Read out of the script, not repeated here, for the reason the wrapper assertion further +# down reads its constant the same way: a second copy of a budget term drifts from the first. +maxed_wrapper=$(sed -n 's/^PROMPT_WRAPPER_BYTES=\([0-9]*\)$/\1/p' "$CONTEXT_SCRIPT") +maxed_allowance=$((PROMPT_ARG_LIMIT - ALL_INPUTS_OTHER_BYTES - maxed_wrapper \ + - $(escaped_of docs/claude-pr-review-prompt.md) - 1024 - maxed_threads)) +maxed_pct=$((maxed_ctx * 100 / maxed_allowance)) +if [ "$maxed_pct" -le 85 ]; then + echo "ok every block at its ceiling leaves headroom (${maxed_pct}% of the allowance used)" +else + echo "FAIL every capped block at its ceiling leaves no headroom for the uncapped ones:" + printf ' %s escaped bytes against a %s byte allowance (%s%%), threads at %s\n' \ + "$maxed_ctx" "$maxed_allowance" "$maxed_pct" "$maxed_threads" + printf ' the per-block shares of the context sum too close to 1; the tail cut is the\n' + printf ' ordinary path on this run rather than a backstop\n' + failures=$((failures + 1)) +fi # Sharing a budget decides *what* the excerpts spend it on, and the region total above cannot # see that. The summary is written first and the first-error window second, but the window is