fix(review): sanitise before the byte caps, not after

Author: zfarrell

.github/workflows/claude-pr-review.yml

@@ -292,15 +292,21 @@ jobs:
             '
           }
 
+          # Neutralise into the file, then cap -- never the other way round. These
+          # substitutions *grow* the text: a bare `::` line goes from 3 bytes to 28, so a cap
+          # enforced before them stops bounding the step output, and 100 KB of `::`-only
+          # comment lines would leave here as ~930 KB. Capping afterwards is safe in the only
+          # direction that matters, because head -c drops the tail and cannot re-expose a
+          # marker the prefix was covering.
           THREADS_FILE="${RUNNER_TEMP}/threads.md"
-          printf '%s\n' "$THREADS" > "$THREADS_FILE"
+          printf '%s\n' "$THREADS" | neutralise_untrusted > "$THREADS_FILE"
           cap_file "$THREADS_FILE" "$THREADS_MAX_BYTES" \
             "prior review comments truncated at ${THREADS_MAX_BYTES} bytes; read the rest with gh pr view"
 
           DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)"
           {
             echo "threads<<${DELIMITER}"
-            neutralise_untrusted < "$THREADS_FILE"
+            cat "$THREADS_FILE"
             echo "${DELIMITER}"
           } >> $GITHUB_OUTPUT
 
@@ -523,6 +529,11 @@ jobs:
           fi
           { echo; echo "## PR conversation"; printf '%s\n' "$CONVO"; } >> "$CTX"
 
+          # Before the size check, for the reason given at the threads file: the substitutions
+          # grow the text, so the budget has to be enforced on what actually leaves the step.
+          neutralise_untrusted < "$CTX" > "${CTX}.clean"
+          mv "${CTX}.clean" "$CTX"
+
           # Last resort against an unbounded block -- the per-block caps above should keep
           # the file far below this, so hitting it means one of them regressed.
           if [ "$(wc -c < "$CTX" | tr -d " ")" -gt "$CTX_MAX_BYTES" ]; then
@@ -533,7 +544,7 @@ jobs:
           CTX_DELIMITER="PR_CONTEXT_$(openssl rand -hex 16)"
           {
             echo "pr_context<<${CTX_DELIMITER}"
-            neutralise_untrusted < "$CTX"
+            cat "$CTX"
             echo "${CTX_DELIMITER}"
           } >> $GITHUB_OUTPUT
         env:

README.md

@@ -66,6 +66,12 @@ and restructuring them would misrepresent the file being reviewed — while a li
 visible `[log marker neutralised]` prefix, because there the whole line was the command. A mid-line
 `::` is left alone, which keeps every `std::collections::HashMap` in a Rust diff intact.
 
+Both substitutions run *before* the byte caps, not after. They are the only thing here that makes text
+longer — a bare `::` line is 3 bytes in and 28 out — so capping first would leave the budgets bounding
+nothing: 100 KB of `::`-only comment lines would leave as ~930 KB. Truncating afterwards is safe in
+the direction that matters, since `head -c` only drops the tail and cannot re-expose a marker the
+prefix was covering.
+
 ### Tool usage artifact
 
 Each run attaches a `claude-tool-usage-pr-<number>` artifact (14-day retention): tool call counts,

docs/claude-pr-review-prompt.md

@@ -9,14 +9,14 @@ This prompt includes:
 
 Everything in `<pr_context>` is already in front of you. Do not spend a tool call re-fetching it.
 
+**Unless it is not there.** If `<pr_context>` is empty, or a block inside it says it could not be read, then that block is genuinely missing — fetch what you need yourself with `gh pr diff` or `gh pr view`, and say in your review that you reviewed without it. Never treat a missing block as evidence: an absent CI block does not mean CI is clean, and an absent diff does not mean nothing changed.
+
 Three edits in that content were made by the workflow, not by anyone: `[block tag removed]` replaces
 a block delimiter, `[log marker neutralised]` prefixes a line that would otherwise have been read as
 a GitHub Actions command, and `##[` is respaced to `## [` for the same reason. All three are
 sanitiser output. Read past them, and never quote one as if it were in the file — a `## [error]` in a
 CI excerpt was `##[error]` in the real log. If you need the exact line, `Read` the file.
 
-**Unless it is not there.** If `<pr_context>` is empty, or a block inside it says it could not be read, then that block is genuinely missing — fetch what you need yourself with `gh pr diff` or `gh pr view`, and say in your review that you reviewed without it. Never treat a missing block as evidence: an absent CI block does not mean CI is clean, and an absent diff does not mean nothing changed.
-
 ## Tools
 
 Available: `Read`, `Grep`, `Glob`, `rg`, and `gh pr diff` / `gh pr view` / `gh pr review` / `gh pr comment`. Nothing else — every other command is refused, and each refusal costs a turn.

tests/context-step-test.sh

@@ -118,12 +118,16 @@ case "$args" in
   *"/pulls/"*"/comments"*)
     fail_if_marked comments
     if [ -n "$STUB_THREAD_BODY" ]; then
-      # One thread, body under the caller's control. The threads block is a separate step
+      # Threads whose body is under the caller's control. The threads block is a separate step
       # output from the context, and the production incident this exists for arrived through
       # it: the marker was prose in a prior review comment, not anything the PR author wrote.
-      jq -n --arg body "$STUB_THREAD_BODY" \
-        '[{id: 1, user: {login: "claude[bot]"}, path: "a.py", line: 1,
-           created_at: "2026-08-01T00:00:00Z", body: $body}]'
+      # The count matters as much as the body, because each body is capped at 3,000 characters
+      # on its own -- one comment cannot reach the block cap no matter what is in it, so a
+      # test that needs the block cap has to ask for many.
+      jq -n --arg body "$STUB_THREAD_BODY" --argjson n "${STUB_THREAD_COMMENTS:-1}" \
+        '[range(if $n > 0 then $n else 1 end)
+          | {id: ., user: {login: "claude[bot]"}, path: "a.py", line: (. + 1),
+             created_at: "2026-08-01T00:00:00Z", body: $body}]'
     elif [ "$STUB_THREAD_COMMENTS" -gt 0 ]; then
       awk -v n="$STUB_THREAD_COMMENTS" 'BEGIN {
         printf "[";
@@ -398,7 +402,7 @@ expect_no_markers "$THREADS_OUT" "workflow commands in a review comment are neut
 # for the block tags: that block exists to show the reviewer an error line, so deleting
 # `##[error]` would remove the thing it was fetched for.
 expect_context '## \[error\]this is not really an error' \
-  "a mid-line ##[ is broken by a space and stays readable"
+  "a line-leading ##[ is broken by a space rather than prefixed"
 expect_context '\[log marker neutralised\] ::error::neither is this' \
   "a line-leading :: is prefixed and stays readable"
 expect_context '`## \[error\]` marker' "a ##[ inside backticks is broken too"
@@ -627,6 +631,35 @@ expect "$(awk -v n="$total_bytes" 'BEGIN { print (n < 400000) ? "bounded" : "unb
   "bounded" "the whole step output is bounded (was $total_bytes bytes)"
 expect_context '^## Full diff' "the diff block survives a huge threads block"
 
+# The budget has to survive the sanitiser, which is the one thing in this step that makes the
+# text *longer*. A bare `::` line is 3 bytes in and 28 out, so a cap enforced before the
+# substitution bounds nothing: 100 KB of `::`-only lines leaves as ~930 KB. The padding used
+# above carries no marker, so only an input made of them holds this ordering in place, and it
+# needs no privilege to produce -- a review comment, or a committed file of `::` lines.
+#
+# 400 comments, not one: each body is capped at 3,000 characters before it reaches the block,
+# so a single comment cannot approach the block cap however long it is. Getting that wrong is
+# what made the first version of this test pass against the bug it was written for.
+COLON_BODY=$(awk 'BEGIN { for (i = 0; i < 2000; i++) print "::" }')
+STUB_THREAD_BODY="$COLON_BODY" STUB_THREAD_COMMENTS=400 run_step > "$WORK/code.txt"
+expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on comments made of bare :: lines"
+colon_bytes=$(wc -c < "$THREADS_OUT" | tr -d ' ')
+expect "$(awk -v n="$colon_bytes" 'BEGIN { print (n < 300000) ? "bounded" : "unbounded" }')" \
+  "bounded" "the sanitiser cannot grow the threads output past its cap (was $colon_bytes bytes)"
+expect_no_markers "$THREADS_OUT" "every :: line in an amplifying comment is still neutralised"
+unset STUB_THREAD_BODY
+
+# The same amplification against the context, through the one block with no per-block cap of
+# its own: the PR body is printed whole. GitHub allows 65,536 characters there, which is
+# ~21,800 `::` lines, or ~610 KB out against a 200 KB budget.
+BODY_COLONS=$(awk 'BEGIN { for (i = 0; i < 21800; i++) print "::" }')
+PR_BODY="$BODY_COLONS" run_step > "$WORK/code.txt"
+expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on a PR body of bare :: lines"
+ctx_colon_bytes=$(wc -c < "$CTX_FILE" | tr -d ' ')
+expect "$(awk -v n="$ctx_colon_bytes" 'BEGIN { print (n < 250000) ? "bounded" : "unbounded" }')" \
+  "bounded" "the sanitiser cannot grow the context past its cap (was $ctx_colon_bytes bytes)"
+expect_no_markers "$CTX_FILE" "every :: line in an amplifying PR body is still neutralised"
+
 # Ordering only means something if an *earlier* block can exhaust the budget. LOG_WINDOW
 # counts lines, and a CI log line has no length limit -- one base64 or JSON dump near the
 # first error marker is enough to eat the budget before the diff heading is ever written.