fix(review): neutralise log markers, flag command substitution

[zfarrell]

Two follow-ups from reviewing the first day of runs under the frontloaded context.

Untrusted text reaching the prompt could write annotations onto the review's own check run — the action echoes the prompt into the job log, and GitHub parses a line starting with :: or ##[ as a workflow command. Such lines are now prefixed rather than stripped, so the marker stays legible. Separately, has_subst joins compound on the tool usage artifact to test the hypothesis that markdown backticks in a review body are what still gets gh pr review refused.

[claude]

nit: the byte caps no longer bound the step outputs, because prefixing happens after cap_file (not blocking).

cap_file runs at line 287 for threads and line 521 for the context, but neutralise_untrusted runs here (293) and at 526 — i.e. after the budget is enforced. The block-tag substitution was size-neutral-ish, so that ordering never mattered before; the new prefix adds 25 bytes ([log marker neutralised] ) per matched line, and the shortest matching line is ::\n at 3 bytes. That is ~9x on pathological input: 100 KB of threads becomes ~930 KB, 200 KB of context becomes ~1.9 MB.

It is reachable without any privilege — a PR body or an inline review comment made of ::-only lines, or a committed file whose lines start with :: (context lines in the diff carry a single leading space, which is exactly the vector the new test covers). The failure mode is degraded rather than dangerous: the step is continue-on-error, and the prompt already handles an empty pr_context. But it does contradict README.md:39-44, which states the caps are "deliberately far below any plausible runner limit", and in the cases that stay under the limit it multiplies prompt tokens by the same factor.

Neutralising into the file before capping keeps the budget authoritative and needs no new machinery:

THREADS_FILE="${RUNNER_TEMP}/threads.md"
printf '%s\n' "$THREADS" | neutralise_untrusted > "$THREADS_FILE"
cap_file "$THREADS_FILE" "$THREADS_MAX_BYTES" "..."

and the same for $CTX before the cap_file at line 521, leaving the two heredoc blocks as plain cat. Truncating after the substitution is safe in the direction that matters — head -c only drops the tail, so it cannot expose a :: that the prefix was covering.

[claude]

Approving — one non-blocking note inline.

The neutralisation itself checks out: the two substitutions run in the safe order (the block-tag replacement is non-empty, so it cannot promote a mid-line :: to line start), \# is escaped so /x does not eat the pattern as a comment, and /x does not apply to the replacement so the trailing space survives. Both untrusted step outputs go through the same function, and the test suite asserts that rather than assuming it.

has_subst is grouped and computed on the raw command, distinct from compound's unquoted form, and the existing fixture has no backtick or $(, so none of the pinned .commands rows split.

[claude]

nit: the ordering point from thread #3722321975 still stands after the two-rule split — flagging so the split isn't mistaken for the fix. (not blocking)

The ##[ rule now adds 1 byte per match rather than 25, so that half of the amplification is gone. The :: half is unchanged: cap_file runs at line 297 (and 531 for the context) but neutralise_untrusted runs here (303) and at 536, so the prefix lands after the budget is enforced. ::\n is 3 bytes in and 28 bytes out, i.e. still ~9x — 100 KB of threads → ~930 KB, 200 KB of context → ~1.9 MB, from a comment body or a committed file of ::-only lines.

Same fix as before: neutralise into the file before capping, and leave the two heredocs as plain cat. head -c only drops the tail, so truncating after the substitution cannot re-expose a :: the prefix was covering.

[claude]

super nit: the input this asserts on is line-leading, not mid-line. (not blocking)

LOG_INJECT line 3 is ##[error]this is not really an error at column 1 — it is the case the old rule would have prefixed and the new rule respaces instead. The genuinely mid-line assertions are the backtick one two lines below and the timestamped job-log one at 427. As written the description makes the suite read as if line-leading ##[ were untested.

Suggested change

	expect_context '## \[error\]this is not really an error' \
	"a mid-line ##[ is broken by a space and stays readable"
	expect_context '## \[error\]this is not really an error' \
	"a line-leading ##[ is broken by a space rather than prefixed"

[claude]

super nit: this paragraph is inserted between "already in front of you" and the "Unless it is not there." that qualifies it. (not blocking)

Read straight through, "it" now attaches to the nearest antecedent — the respaced ##[error] line — rather than to the frontloaded context, and that next paragraph is the one that stops a reviewer reading an empty CI block as a green one. Moving the sanitiser paragraph below line 18 keeps the pair adjacent and costs nothing, since nothing in it depends on being read first. (Same nit applies to the identical insertion in README, which is prose and not prompt.)

[claude]

No blocking issues. Three non-blocking comments inline.

The two-rule split is well evidenced and the tests cover the positions that distinguish the forms: line-leading :: prefixed, mid-line :: untouched (std::collections::HashMap), ##[ respaced anywhere including inside backticks and behind an API timestamp. has_subst is correctly tested against the raw command where compound is tested unquoted, and the two are asserted apart on the same gh pr review row.

Note on CI: only Tests / test is reported as passing in the context I was given; I did not run anything.

[claude]

nit: moving the pass here makes every per-block cap non-authoritative, and the PR body has no cap of its own — so the diff can now be evicted where it used to survive. (not blocking)

The step output is bounded again, which was the point, and the threads half is fully fixed because its only cap is the one that now runs last. But cap_file "$CTX" at line 542 is the sole cap downstream of the substitutions, and the blocks are appended in order, so an amplifying early block no longer just inflates the output — it consumes the budget the later blocks were going to use.

The PR body at line 340 is the reachable one, and it is the block your own new test already sizes: 65,536 characters is ~21,800 :: lines, 28 bytes out each, ~610 KB against a 200 KB cap. The header above it is ~150 bytes, so head -c 200000 cuts ~1/3 of the way into the body and ## Commits, ## Changed files, ## CI checks, the failing-job excerpt, the since-diff and ## Full diff are all gone. Before this commit the same body was ~64 KB at cap time, so every one of those blocks made it in and the cost was output size instead.

That reading is testable, and I think it is why tests/context-step-test.sh:655-661 asserts only boundedness where the three sibling budget tests (613, 632, 676) each also assert ^## Full diff. Adding the same line there should fail today:

expect_context '^## Full diff' "the diff block survives an amplifying PR body"

The other pre-cap blocks turn out to be fine, for what it is worth — the log excerpts are line-bounded (LOG_WINDOW 120 + 20 summary lines, so ~4 KB even all-::), and DIFF_MAX/SINCE_MAX cap out around 145 KB combined. The body is the only uncapped one.

Two shapes of fix. Narrow: give the description its own block cap on the sanitised bytes, like the log excerpts get.

BODY_FILE="${RUNNER_TEMP}/pr-body.md"
printf '%s\n' "${PR_BODY}" | neutralise_untrusted > "$BODY_FILE"
cap_file "$BODY_FILE" "$BODY_MAX_BYTES" "description truncated at ${BODY_MAX_BYTES} bytes"

General: sanitise each untrusted value at the point it enters $CTX rather than in one pass at the end, which restores the invariant the per-block caps were written under and lets this whole-file pass go away. PR_TITLE is untrusted on the same footing and is a single line, so it costs nothing either way.

Degradation, not a wrong review — the truncation notice survives the cut and the prompt handles absent blocks by fetching. But README.md:53-56 now says the per-block caps keep the file "far below" the total, and with the substitutions downstream of them that is no longer what the code does.

[claude]

Prior nits addressed: the sanitiser now runs before both byte caps, the test description reads line-leading, and the prompt's "Unless it is not there." is adjacent to the paragraph it qualifies again. One non-blocking note inline about the PR body being the one uncapped block downstream of the new ordering. has_subst looks right — raw command for the flag, unquoted for compound, and both grouped so the write-path denials separate.

[claude]

nit: this line puts a parsable ##[error] into the prompt itself, which is the one place the sanitiser cannot reach. (not blocking)

neutralise_untrusted runs over the two step outputs. This document arrives separately, through steps.prompt.outputs.content at .github/workflows/claude-pr-review.yml:610, and is never passed through it. By the mechanism this PR establishes — ##[...] is matched anywhere in a line and a backtick does not defuse it (run 31025325888, and the `## [error]` marker assertion at tests/context-step-test.sh:412) — the `##[error]` here has its closing ] on the same line, so it is a workflow command and becomes an error annotation on the review's own check run.

Line 16's `##[` is fine, for what it's worth: no ] follows it on that line, so there is nothing to close the command. Line 17's `## [error]` is the already-spaced form. This one is the only reachable spelling in the file.

The difference from the case the PR fixes is that this one is unconditional — it fires on every run once the doc lands on main, not only when an author writes a marker. Same cost as the incident (a red mark on a green check), but permanent. Breaking the ##[ adjacency keeps the contrast the sentence needs:

Suggested change

	CI excerpt was `##[error]` in the real log. If you need the exact line, `Read` the file.
	CI excerpt was `##` + `[error]`, unspaced, in the real log. If you need the exact line, `Read` the file.

[claude]

nit: neither of these two assertions would have caught the }} truncation the comment credits them with — both pass against the old default. (not blocking)

The old form was PR_BODY="${PR_BODY:-... \${{ github.token }} are literal text here.}". Bash ends the expansion at the first }, i.e. the one in github.token }} — but the remaining } are literal text here. is still inside the outer double quotes, so it was concatenated literally. That is what the comment at line 210-216 says happened ("the text after the }} was concatenated literally"), and it means the buggy value was:

Adds a watermark. `$(touch /tmp/pwned)` and ${{ github.token } are literal text here.

So are literal text here\.$ matched, github\.token matched, and \$\(touch /tmp/pwned\) matched. The single byte that differs is }} collapsing to } — nothing else. The fragment the old code delivered was a fragment of the expansion, not of the line, which is why an end-anchored assertion cannot see it.

One assertion that does discriminate, and it subsumes both:

Suggested change

	# The whole default, not a prefix of it. This is the assertion that would have caught the
	# `}}` truncation in run_step's default: the Actions expression sits after the point where the
	# parameter expansion used to end, so its arrival proves the body reached the step intact.
	expect_context 'are literal text here\.$' "the whole PR body reaches the context, not a prefix"
	expect_context 'github\.token' "an Actions expression in the PR body survives as text"
	# The whole default, including the `}}` that the old inline form ate. `${PR_BODY:-...}` ended at
	# the first `}` of `${{ github.token }}`, and the tail after it was still concatenated literally --
	# so an end-anchored match and a bare `github.token` match both survived the bug. The doubled
	# brace is the only byte that distinguishes the fragment from the whole, so it is what to pin.
	expect_context 'github\.token \}\} are literal text here\.$' \
	"the whole PR body reaches the context, not a prefix"

[claude]

All four prior threads are addressed: the sanitiser now runs before both byte caps, the two-rule split stands, the description has its own 40 KB cap on sanitised bytes so it can no longer evict the diff, and the two doc/test wording nits were applied. The added budget assertions cover the eviction case directly. Two non-blocking nits inline; no blocking issues.

[claude]

Cycle 5: no blocking issues. All six prior threads are addressed in code, not just in replies -- the two step outputs are sanitised into their files before the caps, the description has its own 40 KB budget on sanitised bytes with tests asserting the diff and CI blocks still survive an amplifying body, the live log marker is out of the prompt document and now guarded by a lint scan, and the body-integrity assertion pins the doubled brace that discriminated the old bug.