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

[zfarrell]

A diff that does not fit the prompt budget is now replaced by a notice telling the reviewer to run gh pr diff, instead of being cut to a prefix that reads as the whole patch. The PR conversation gets a cap of its own, so it is no longer the block the tail cut pays with — and so the diff can ask whether there is room for it.

Across the last two weeks, 108 runs 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.

[claude]

nit: the reserve is CONVO_MAX_BYTES (the cap) rather than the conversation's actual size, and the common case is that the conversation is ~35 bytes (No PR conversation comments.). With the current constants that hands back PROMPT_BUDGET / 8 ≈ 14.7 KB — roughly 1,200 patch lines — that nothing will ever spend. (not blocking)

That over-reserve was cheap when the shortfall cost the diff a prefix; now it costs the whole block, so it converts directly into omissions on PRs that would have fit. The test at tests/context-step-test.sh:955 shows the margin: an allowance near 40 KB against a diff of 22 KB, where without the reserve it would be near 52 KB.

The conversation is a plain gh api read with no dependency on the diff, so it can be fetched and capped above this block and still be written last:

# ... fetch CONVO, strip, cap_file_escaped into $CONVO_FILE ...
CONVO_BYTES=$(escaped_bytes "$CONVO_FILE")
DIFF_ALLOWANCE=$((CTX_MAX_BYTES - $(escaped_bytes "${RUNNER_TEMP}/fit-ctx") - CONVO_BYTES))

which keeps the block ordering the comment at line 693 argues for while making the reserve exact.

[claude]

nit: this is the exact logical inverse of the elif at line 638, spelled out a second time thirty lines later. It is correct today, but the two are free to drift, and the drift is silent in the direction that matters: the block renders the omission text while the annotation says nothing, or the reverse. That is precisely the "invisible failure mode" the comment below says this notice exists to end. (not blocking)

Since { ... } >> "$CTX" is a group command, not a subshell, a flag set in the else branch survives:

DIFF_OMITTED=0
{
  ...
  elif [ "$DIFF_LINES" -le "$FULL_DIFF_MAX" ] && [ "$DIFF_ESCAPED" -le "$DIFF_ALLOWANCE" ]; then
    cat "$DIFF_FILE"
  else
    DIFF_OMITTED=1
    ...
  fi
} >> "$CTX"
if [ "$DIFF_OMITTED" -eq 1 ]; then
  echo "::notice::..."
fi

That also drops the DIFF_LINES -gt 0 guard, which only exists here to re-exclude the empty-diff branch.

[claude]

nit: "the PR body is the one block with no cap of its own" is not quite true, and the same claim is repeated at tests/context-step-test.sh:659–660. Two other blocks reach the tail cut with nothing bounding them: (not blocking)

• The since-diff is line-capped at SINCE_MAX=2000 but has no byte cap. By this PR's own 1.20x figure, 2,000 lines of dashboard JSON is ~120 KB escaped, which is over CTX_MAX_BYTES on its own — so on cycle 2+ that block alone drives the tail cut, and because it is written before ## Full diff the cut eats the omission notice and the conversation rather than the since-diff. A byte cap there is compatible with the "keep the prefix" argument in this same paragraph; it just needs cap_file_escaped with the existing NOTICE_LINES wording instead of a bare head -n.
• ## Changed files has no cap at all, and --paginate returns up to GitHub's 3,000-file limit. That is the block the omission notice at scripts/gather-review-context.sh:662 sends the reviewer to, so it is worth it not being the block that overflows.

Either bound them or narrow the sentence — it is load-bearing for "the tail cut is a backstop rather than the ordinary path", and a dashboard PR at cycle 2+ is exactly the case where it is not.

[claude]

nit: this half is a share of PROMPT_BUDGET, but what this block is actually writing into is CTX_MAX_BYTES, which is PROMPT_BUDGET - THREADS_BYTES. Threads already took its half out of the same total, so with a maxed review history the two halves are not siblings — the second one is the whole of what remains. (not blocking)

With the doc measuring 9,650 escaped: PROMPT_BUDGET ≈ 117,798, THREADS_MAX_BYTES = 58,899, so CTX_MAX_BYTES = 58,899 — and this cap is 58,899 too. A cycle-5+ PR with a long comment history and a dense since-diff lets this one block fill the entire context allowance, and the tail cut then takes everything ordered after it: ## Full diff's omission notice, the "get it before you review" instructions, and ## PR conversation. That is the case the comment above says cannot happen ("so the pair of blocks the reviewer cannot re-fetch can never crowd out everything else together"), and it is the same shape the byte cap was added to prevent, one level up.

CTX_MAX_BYTES is in scope by here (derived at line 412), so $((CTX_MAX_BYTES / 2)) makes the share mean what the comment says. The same reading applies to CONVO_MAX_BYTES and the changed-file cap: an eighth of PROMPT_BUDGET is a quarter of the context once threads is at its cap.

The suite does not cover it — STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json runs with an empty threads block, where CTX_MAX_BYTES is the full budget and the since-diff fits in half of it. Adding STUB_THREAD_COMMENTS=60 to that run is the case.

[claude]

nit: this notice asserts a line cut that may not have happened. It is unconditional, but SINCE_MAX and SINCE_LINES only describe a real cut when SINCE_LINES > SINCE_MAX — the byte cap fires independently of the line cap, and it fires at a lower line count on exactly the content this cap exists for. (not blocking)

1,500 lines of dashboard JSON is under SINCE_MAX and roughly 165 KB raw, well over the byte budget, so the block renders as:

(truncated: first 2000 of 1500 lines, then cut to fit the prompt)

"first 2000 of 1500" is not a number the reviewer can act on, and the figure it names is not what it was given — the file was cut to a few hundred lines. The notices are load-bearing per the NOTICE_* block at line 182, and this is the one path where one of them states a falsehood rather than a partial truth.

The wording only needs to depend on which cap fired:

if [ "$SINCE_LINES" -gt "$SINCE_MAX" ]; then
  SINCE_CUT_NOTICE="${NOTICE_LINES} ${SINCE_MAX} of ${SINCE_LINES} lines, then cut to fit the prompt"
else
  SINCE_CUT_NOTICE="${NOTICE_LINES} lines of ${SINCE_LINES}, cut to fit the prompt"
fi

The STUB_SINCE_LINES=2000 STUB_SINCE_STYLE=json test only exercises the over-SINCE_MAX half; STUB_SINCE_LINES=1500 reaches this one.

[claude]

nit: SINCE_USED is fixed before the byte cap runs, so it charges the full diff for lines the since-diff did not end up spending — the same over-reserve you just removed for the conversation, in the other currency. (not blocking)

A dense since-diff byte-capped down to, say, 400 rendered lines still sets SINCE_USED=2000, so FULL_DIFF_MAX becomes 1,000 and a 1,500-line full diff is omitted on the line test even though the context it would join is small and the byte allowance is wide open. By the argument at line 741, that converts directly into an omission on a PR whose diff would have fit, and unlike the conversation case there is no notice explaining that the budget went to a block that no longer holds it.

Recounting after the cap is the fix, since SINCE_CAPPED is the file that was actually written:

SINCE_USED=$(awk 'END {print NR}' "$SINCE_CAPPED")

(after the cap_file_escaped call, replacing the min(SINCE_LINES, SINCE_MAX) computed here).

[claude]

All three prior nits are addressed: the conversation reserve is now measured rather than assumed, the omission branch and the ::notice:: annotation share one DIFF_OMITTED flag, and the since-diff and changed-file blocks are bounded with the README sentence narrowed to match.

Three new nits inline, all on the since-diff block the second commit added — a cap share that is a fraction of the wrong total, a truncation notice that names line numbers no cut used, and a line reservation taken before the byte cap reduces it. None block.

[claude]

nit: the comment above enumerates the caps this re-denomination reaches — "the changed-file and since-diff caps below" — and LOG_BUDGET (line 161) is the one it does not. It is now the only per-block share still measured against PROMPT_BUDGET, and it is written above ## Full diff like the other two. (not blocking)

With threads at its cap, CTX_MAX_BYTES ≈ PROMPT_BUDGET / 2 ≈ 58.9 KB, so a quarter of the budget is half the context. It is also charged raw — cap_log_excerpt bills through wc -c, not escaped_bytes — so at the 1.20x this file measures for quote-dense text those 29.4 KB arrive as ~35 KB escaped. Beside a since-diff at its own new half (29.4 KB) that is ~64 KB before the changed-file list and the conversation are written, i.e. the tail cut, and what the tail cut takes is the conversation and then the full diff's omission instructions — the text this commit moved these shares to protect. Three verbose failing jobs plus a maxed review history is a narrow run, but it is the same shape as the one the commit fixes, and CTX_MAX_BYTES is in scope by the job loop at line 528 ($((CTX_MAX_BYTES / 4))).

Same reading applies to README.md:44–45: it presents all five shares as sitting "beneath that total", and after this commit only threads and the log allowance are shares of PROMPT_BUDGET — the other three are shares of what is left after threads.

[claude]

super nit: "An eighth of the budget" is what this cap stopped being one commit ago — the call below is $((CTX_MAX_BYTES / 8)), and the conversation's comment now spells the distinction out ("An eighth of the context"). Worth the one word here too, since the block at line 419 names this cap as one the new reading covers. (not blocking)

[claude]

Prior cycle's five nits are all addressed: the conversation reserve is measured rather than assumed, the omission annotation reads a flag set inside the group command, the changed-file and since-diff blocks are bounded, the shares are denominated in CTX_MAX_BYTES, the since-diff notice depends on which cap fired, and SINCE_USED is recounted after both caps. Tests cover each. One remaining nit inline on the block that did not get the new denominator; not blocking.

[claude]

nit: with this line the per-block shares of the context now sum to exactly 1 — since-diff CTX/2, logs CTX/4, changed files CTX/8, conversation CTX/8 — so there is no headroom left for the PR body, ## Commits, the CI check list, the block headings, or the diff itself. (not blocking)

cap_file_escaped gives back 300 bytes per call, so the three escaped shares land ~900 under; the log share spends that back and more, because cap_log_excerpt bills through wc -c while everything else is charged escaped — at the 1.20x this file measures for quote-dense text, CTX/4 raw arrives as roughly 0.3 * CTX.

The run is a dense since-diff at its cap, three verbose failing jobs, --paginate returning near GitHub's 3,000-file ceiling, and a conversation at its own cap. DIFF_ALLOWANCE goes negative, so the diff correctly omits itself, but the context is already over budget before the omission text and ## PR conversation are written — and those are exactly what the tail cut takes, bottom-up: the conversation, then the instructions, then the (full diff omitted...) line, leaving the ## Full diff heading with the NOTICE_CONTEXT line under it. That is the "heading with nothing under it is a claim" case at line 730, reached through the tail cut instead of through the empty-diff branch.

The suite's nearest run is line 910, which clears it only because the changed-file list and the conversation are both near-empty there; adding STUB_FILES=3000 STUB_CONVO_COMMENTS=300 to that line is the case. Whichever way it is closed — shares summing under 1, or charging the log excerpts escaped like the rest — it is worth the sum having slack, since the argument for these shares is that the tail cut stays a backstop.

[claude]

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

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

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

[claude]

All prior threads are addressed in code: the conversation reserve is now measured off the capped file, the omission branch and the ::notice:: share one DIFF_OMITTED flag, the since-diff has a byte cap with its own notice wording, SINCE_USED is recounted after both caps, and the changed-file list and log allowance are denominated in CTX_MAX_BYTES. Two nits left inline, both about the "every fetched block is bounded" claim rather than about the omission behaviour.

[claude]

Cycle 5: all ten prior threads are resolved in code — the conversation reserve is now exact, DIFF_OMITTED is a single flag read by both the block and the annotation, the since-diff/log/changed-file/commit shares all read off CTX_MAX_BYTES, SINCE_USED is recounted after both caps, NOTICE_SINCE replaces the false line-cut wording, cap_log_excerpt bills escaped, and the shares now leave headroom with a test asserting the margin. No blocking issues.