fix(review): bound the assembled prompt below MAX_ARG_STRLEN - #31
Conversation
This step's two outputs are interpolated into one `prompt:` value, which reaches the reviewer as a single environment string. That makes the kernel's MAX_ARG_STRLEN -- 32 * PAGE_SIZE, 131072 on the runners -- the binding limit, not the step-output limit the previous budgets reasoned about. Those budgets were 100 KB for threads and 200 KB for context, applied independently and free to sum to 300 KB. Past the limit exec fails with "Argument list too long", and the failure is near-silent: the action still reports success, the tool-usage steps skip for want of an execution log, no review of any kind is posted, and the only trace is the generic "review unavailable" notice. A pull request assembling 186 KB of prompt failed exactly that way with both blocks inside their own caps. So budget the sum. Context gets what the threads block did not spend, out of the argument limit less the wrapper, the prompt document and 1 KB of slack. The document is measured rather than hard-coded, so editing it shrinks the context budget instead of silently overflowing the limit. threads keeps a cap of its own -- 400 inline comments rendered 1.1 MB on their own -- held to half the budget so a long review history cannot starve the diff either. The budget lands near 122 KB, close enough to the limit that a pull request assembling under it today is not newly truncated: only those that already fail outright change behaviour. The new assertion oversizes every input at once, which is what the old pair of caps could not be caught by -- each block sat inside its own limit while the total did not fit. Two existing assertions were stale against a derived cap: one pinned the literal 200000 in the truncation notice, and one allowed 210000 bytes of context, which is past the limit the prompt is bounded by and so would have passed on a context that could not be handed to the reviewer at all.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Blocking Issues
-
scripts/gather-review-context.sh:444-456(and the threads cap at:210) — the budget is enforced beforestrip_block_tags, which is what actually produces the emitted output at:216and:461. That substitution replaces a 12-byte openingpr_contexttag with a 19-byte[block tag removed], so the string handed to the reviewer can be up to 1.58x the size the cap measured.cap_filealso appends its notice afterhead -c, putting the assembled total at aboutPROMPT_BUDGET + 103before any substitution fires, which leaves roughly 920 of the 1024 bytes of slack — about 130 tag occurrences across the diff, PR body, log excerpt and threads — before the total crossesMAX_ARG_STRLENagain. What it crosses back into is the near-silent failure this PR exists to close, and it is reachable on purpose:strip_block_tagsexists because an author may write a closingpr_contexttag into text this script renders, so flooding it now also costs the PR its review behind a green check.The new all-oversized assertion cannot catch this. The stub's diff is
+line Nand its comment bodies are padding text, so no substitution fires and the measured total equals the capped total.
Action Required
- Make the cap measure what is emitted: run
strip_block_tagsinto the file beforecap_file, for both$THREADS_FILEand$CTX, and have:216/:461copy the already-stripped file out. Truncating mid-[block tag removed]is harmless, and no unstripped tag can survive a cut. - Give the new assertion an input that exercises it — a stub diff or comment body carrying the closing tag repeated — otherwise the bound stays untested against the one input shape that can break it.
The rest of the change is sound: budgeting the sum, measuring the prompt document rather than pinning it, and replacing the 210,000-byte assertion that sat above the limit. One non-blocking note inline on PROMPT_WRAPPER_BYTES.
I have not run anything — CI was still queued or in progress when this review started, so nothing here is a claim about the suite's result.
The bound the previous commit added did not hold. Both blocks were capped, but what reached the output was `strip_block_tags < "$FILE"`, and that substitution grows the text: the shortest tag it matches, `<pr_context>`, is 12 bytes and becomes a 19-byte placeholder. So the cap measured a smaller string than the one emitted -- 1.58x smaller at worst. With ~920 bytes of slack left after cap_file appends its notice, about 130 tag occurrences anywhere in the diff, the PR body, a CI log excerpt or the inline comment threads put the assembled prompt back over MAX_ARG_STRLEN, and back into the near-silent failure this is meant to close. It is reachable on purpose: strip_block_tags exists precisely because author text can contain these tags. Strip into the file first, then cap, then emit the already-stripped file. A cut can bisect `[block tag removed]`, which is inert; it can no longer leave half of a live tag behind, because none are left to bisect. The all-oversized assertion could not catch this -- padding text fires no substitution, so the emitted size equalled the capped size. STUB_DIFF_TAGS puts a tag on every added line, which is the only input shape that separates the two. Verified: with the strip back in its old position that assertion fails. Also compare the measured wrapper against PROMPT_WRAPPER_BYTES, extracted from the script rather than repeated in the test. It is the only one of the three subtracted terms that is stated as a constant rather than measured, so it is the only one that can go stale, and nothing was checking it -- the wrapper could have grown by the whole remaining slack before the total assertion went red, naming the symptom instead of the cause. And tell the reviewer what to do with a truncated block. The prompt document said not to re-fetch what it was given, with an exception only for a block that was empty or unreadable -- nothing for one that was cut short. A truncated diff would have been reviewed as though complete, and an approval formed on a partial diff reads to a human as coverage it does not have. It is now told to fetch the remainder, to say at the top of the review that its context was truncated, and not to approve on the strength of a change it could not fully see.
|
Both addressed in Blocking — cap measured before the size-changing substitution. Correct, and the bound genuinely did not hold. Verified the mechanism rather than taking it on faith: Taken exactly as suggested: Also right that the existing assertion could not catch it — padding text fires no substitution, so the measured total equalled the capped total. Nit — One thing you did not raise, which your reasoning led me to. The prompt document told the reviewer not to re-fetch what it was given, with an exception only for a block that was empty or unreadable — nothing for one that was truncated. So this PR would have introduced a reviewer that reads a shortened diff and reports as though it were complete. It is now told to fetch the remainder, to state at the top of the review that its context was truncated, and not to approve on the strength of a change it could not fully see. That mattered more than the byte arithmetic: a silent partial review is worse than a loud failure. Full suite passes. |
There was a problem hiding this comment.
Both cycle-1 findings are fixed in the code. strip_block_tags now runs into $THREADS_FILE and $CTX before cap_file, with the emit sites reduced to cat, so the cap measures what is emitted; PROMPT_WRAPPER_BYTES is extracted from the script with sed and compared against the measurement rather than repeated in the test.
Re-derived the bound against the doc's new size rather than taking it on faith: budget 121124, threads clamped to 60562, the two truncation notices appended after the cut at ~80 and ~89 bytes, measured literal wrapper 493 against a declared 600. Total lands near 130058 against 131072.
The unprompted addition to the prompt document is the more valuable half of this round. A cap that truncates silently would have turned every overflow into a review reporting on a diff it never saw — worse than the exec failure, because it carries a green check and an approval. Telling the reviewer to fetch the remainder, to state the truncation at the top, and not to approve on a change it could not fully see is what makes the truncation path safe to reach at all.
Two nits inline, both non-blocking: the threads half of the reorder has no test that can see it (the stubbed comment bodies fire no substitution), and LOG_MAX_BYTES was sized against the old 200 KB context cap — at 40000 per excerpt, up to three jobs and two excerpts each, the CI log block can now exceed the whole budget and cut the diff entirely.
Not asserting anything about CI: Tests / test was still unreported when this review started.
|
Hey @anoop-narang — thanks for opening this. I had Claude look into it yesterday but didn't get a chance to finish. Looks like we both caught the root cause, and you found a case I'd missed (strip-before-cap — my branch had that bug too). But I think this PR is missing one thing: the budget counts raw bytes, and the limit applies to the escaped copy.
hotdata-dev/monopoly#1670 is the case that slips through. 123,401 raw bytes — inside the limit — 135,366 escaped. Failed twice. I ran your script against it: Your cap never even fires — the context lands at ~115 KB, under your 121 KB budget — so it passes through untouched and still dies. Your 186 KB example doesn't show this because 186 KB blows the raw limit on And it can't be corrected with a fixed factor: prose escapes at ~1.02x, quote-dense JSON at ~1.20x, so the same 3,000 lines land either side of the limit depending only on content. I put the delta in #32, based on your branch, so you can pull it in here:
Same PR with those changes: One thing worth your call: that leaves 1,881 bytes of margin, which follows from the 1 KB slack you chose deliberately. If |
fix(review): measure the prompt budget in escaped bytes
# Conflicts: # tests/context-step-test.sh
There was a problem hiding this comment.
Both prior blocking items are fixed in code, and the escaped-byte work in 65f3553 holds up:
strip_block_tagsnow runs into the file before the cap for both blocks (scripts/gather-review-context.sh:324,:580), and both emit sitescatthe stripped file, so the cap measures what is emitted.THREADS_BYTESis measured aftercap_file_escapedreturns, so even a non-converged threads cap is absorbed intoCTX_MAX_BYTESrather than escaping the total.cap_file_escapedre-derives the ratio from the surviving prefix each pass, so the expensive-prefix case converges well inside the 8-pass bound; the 0.98 undershoot plus the 300-byte notice reserve covers the landing.SINCE_USEDstays 0 on every no-patch path andSINCE_MAX < DIFF_BUDGET_LINESkeepsFULL_DIFF_MAX >= 1000, sohead -nnever sees 0 or a negative.- Summing
escaped_ofper block over-counts by ~3 bytes each against the concatenation, which is the safe direction.
One non-blocking nit inline on the truncation paragraph in docs/claude-pr-review-prompt.md: it quotes a notice string the script stopped emitting in c05858c.
Not verified: CI had not reported when this review started, so I am not claiming the suite passes.
There was a problem hiding this comment.
All five prior threads are resolved in the code: strip-before-cap for both blocks, PROMPT_WRAPPER_BYTES extracted from the script and compared, STUB_THREAD_TAGS plus a threads-side block-tag assertion, a shared LOG_BUDGET across failing jobs, and the truncation notices as NOTICE_* constants the suite ties to the prompt document.
Verified the notice contract holds in both directions I can check statically: all four constants at scripts/gather-review-context.sh:171-174 are quoted verbatim in docs/claude-pr-review-prompt.md:14, and the sed extraction at tests/context-step-test.sh:777 matches all four, so notice_count reaches 4.
One nit left inline on the log budget — the per-excerpt clamp is unreachable for every value PROMPT_BUDGET can take, so the summary excerpt can spend the window's share. Not blocking.
CI: Tests / test and Claude PR Review / review had not reported at the time this context was gathered, so I am not claiming the suite passes.
There was a problem hiding this comment.
Prior blocking issue is fixed: strip_block_tags runs before both caps (scripts/gather-review-context.sh:383, :637-638) and the emit sites now copy the stripped file, so the bound measures what is actually emitted.
This cycle's change holds up. cap_log_excerpt's optional third argument gives the summary min(LOG_BUDGET/4, LOG_REMAINING) and leaves the window the larger share, the zero-limit branch still routes around head -c 0, and the charge is taken after the cap so the notice counts against the allowance. Log excerpts are measured raw against a budget derived in escaped bytes, but cap_file_escaped "$CTX" bounds the total regardless, so that only shifts what survives rather than whether the prompt fits.
Sum arithmetic checks out: PROMPT_WRAPPER_BYTES=600 against ~503 escaped actual, ALL_INPUTS_OTHER_BYTES=2000 against 1,356 measured, leaving roughly 1.7 KB under the limit on top of the declared 1 KB slack. PROMPT_DOC resolves to the same .github-workflows/docs/claude-pr-review-prompt.md the workflow interpolates, so measuring it is measuring the right file.
CI checks were queued or in progress in the snapshot, so I have no result for the new window_bytes assertion.
No blocking issues.
|
@zfarrell Thanks for taking over this. I was running out of time yesterday and couldn't finish this. |
2 participants
The problem
The gather step's two outputs —
threadsandpr_context— are interpolated into a singleprompt:value, which reaches the reviewer as one environment string. That makes the kernel'sMAX_ARG_STRLEN(32 * PAGE_SIZE, 131072 on the runners) the binding limit.The budgets did not account for it.
THREADS_MAX_BYTES=100000andCTX_MAX_BYTES=200000were applied independently and were free to sum to 300 KB. The comment above them reasoned about the runner's UTF-16 accounting of step outputs — a real limit, but not the one that fails first.Past the limit,
execfails and the failure is near-silent:264 ms — the reviewer never starts. The action reports success, the tool-usage steps skip for want of an execution log, no review of any kind is submitted, and the only trace is the generic "review unavailable, please review manually" notice. A pull request assembling 186,505 bytes of prompt failed exactly that way, with both blocks inside their own caps:
threads## Diff since your last review## Full diffBecause the cap that would have truncated sat above the limit, the existing
cap_filemachinery and its::warning::notice never fired.The fix
Budget the sum rather than two independent blocks:
PROMPT_BUDGET = 131072 − wrapper − prompt document − 1 KB slackpr_contextgets whateverthreadsdid not spend.threadskeeps a cap of its own, since 400 inline comments rendered 1.1 MB on their own, held to half the budget so a long review history cannot starve the diff either.Blast radius
The budget lands near 122 KB — deliberately close to the limit rather than comfortably below it. Every byte held back is context the reviewer doesn't get, and the three terms are measured rather than estimated. The consequence is that a pull request assembling under the limit today is not newly truncated; only those that already fail outright change behaviour.
Tests
The new assertion oversizes every input at once — long review history and oversized diff and long conversation. That combination is the bug: oversizing one input at a time is exactly what let the old pair of caps look safe, each block inside its own limit while the total did not fit.
Verified against the unfixed script:
The wrapper is measured out of the workflow rather than hard-coded, so adding a header line or another do-not-follow notice fails the test instead of quietly spending budget the script believes it has.
Two existing assertions were stale against a derived cap and are fixed:
200000in the truncation notice;Full suite passes (
context-step,pr-context,review-cycle,tool-usage,workflow-lint). The workflow YAML is unchanged.Not addressed here
On review cycle 2+ the prompt carries the diff twice —
## Full diffplus## Diff since your last review(119 KB + 40 KB in the case above). The bound now keeps that within the limit, but the duplication is its own concern and is left alone.