Skip to content

Commit e217f90

Browse files
committed
fix(review): neutralise the ##[ marker form anywhere in a line
1 parent fd8df04 commit e217f90

4 files changed

Lines changed: 87 additions & 58 deletions

File tree

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

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -264,21 +264,31 @@ jobs:
264264
# `##[error]`; the run still concluded success, so what this costs is a red mark on
265265
# a green check, not a broken review.
266266
#
267-
# Which blocks can carry one is decided by whether the text arrives with a line
268-
# prefix. The CI excerpt cannot: `/actions/jobs/{id}/logs` prefixes every line with
269-
# a timestamp, so a marker inside a fetched log is mid-line and was never a command
270-
# (tests/fixtures/job-log-django.txt keeps that shape deliberately). The blocks that
271-
# can are the ones quoting text verbatim -- PR title and body, review comments, the
272-
# PR conversation, and the diff, where an unchanged context line is rendered with a
273-
# single leading space and the runner trims before it parses.
267+
# The two spellings are not parsed the same way, and the difference decides the fix.
268+
# Run 31025325888 settles it -- one review whose diff carried both forms on `+`
269+
# prefixed lines:
274270
#
275-
# Prefixing the line is the whole fix -- the parser only looks at the start of one
276-
# -- and it keeps the marker visible, which matters when the text the reviewer is
277-
# reading is *about* an error line.
271+
# `+##[error]this is not really an error` -> the `+` was consumed, annotation fired
272+
# `+::error::neither is this` -> the `+` survived, nothing fired
273+
#
274+
# So `::command::` is matched only at the start of a trimmed line, while `##[...]`
275+
# is matched *anywhere in the line*. Every one of that run's seven annotations was a
276+
# `##[error]` and none was a `::`. A leading `+`, a timestamp, or a markdown backtick
277+
# is therefore no protection at all for the `##[` form -- which is why the CI excerpt
278+
# is the most reliable source of these, not an exempt one: it fetches the window
279+
# around `##[error]` from a log whose every line the API has already timestamped.
280+
#
281+
# Hence two rules. `##[` is broken wherever it appears, by one space, because the
282+
# lines carrying it are usually source or log text and restructuring them would
283+
# misrepresent the file the reviewer is reading. A line-leading `::` gets the visible
284+
# marker instead: there the whole line was the command, so there is no file content
285+
# to preserve. Both keep the text legible, which matters because the reviewer is
286+
# often reading an excerpt that is *about* an error line.
278287
neutralise_untrusted() {
279288
perl -pe '
280289
s{< \s* /? \s* (?: pr_context | prior_review_comments ) [^>]* >}{[block tag removed]}gix;
281-
s{^ (\s*) (?= :: | \#\# \[ )}{${1}[log marker neutralised] }x;
290+
s{\#\# (?= \[ [A-Za-z] [^\]]* \] )}{## }gx;
291+
s{^ (\s*) (?= :: )}{${1}[log marker neutralised] }x;
282292
'
283293
}
284294

‎README.md‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,22 @@ block delimiters are neutralised by shape rather than by exact string: `</pr_con
4949
them would otherwise end the data block early and land the rest where it reads as instructions.
5050

5151
The same text gets a second treatment for a different sink. The action echoes the assembled prompt
52-
into the job log line by line, and GitHub reads a log line starting with `::` or `##[` as a *workflow
53-
command* — so a marker in a PR body, a review comment, or a diff context line writes an annotation
54-
onto the review's own check run. One run carried two `failure` annotations whose text was prose from
55-
a review comment discussing `##[error]`. Such a line is prefixed rather than stripped: the parser
56-
only looks at the start of a line, and the marker stays legible for a reviewer reading text that is
57-
*about* an error. The CI excerpt is exempt by accident — the logs endpoint timestamps every line, so
58-
markers inside a fetched log are mid-line and were never commands.
52+
into the job log line by line, and GitHub reads a workflow command in that log as a *command* — so a
53+
marker in a PR body, a diff hunk, a CI excerpt, or a review comment writes an annotation onto the
54+
review's own check run. One run carried two `failure` annotations whose text was prose from a review
55+
comment discussing `##[error]`.
56+
57+
The two spellings are not parsed alike, and one review settled which is which: its diff carried both
58+
forms on `+` prefixed lines, the `+` was consumed on `##[error]` and survived on `::error::`, and all
59+
seven annotations were the former. So `::command::` is matched only at the start of a trimmed line,
60+
while `##[...]` is matched anywhere in one — a leading `+`, a timestamp, or a markdown backtick
61+
defuses nothing. That makes the CI excerpt the most reliable source of these rather than an exempt
62+
one, since it fetches the window around `##[error]` from an already-timestamped log.
63+
64+
So `##[` is broken by a single space wherever it appears — those lines are usually source or log text
65+
and restructuring them would misrepresent the file being reviewed — while a line-leading `::` gets a
66+
visible `[log marker neutralised]` prefix, because there the whole line was the command. A mid-line
67+
`::` is left alone, which keeps every `std::collections::HashMap` in a Rust diff intact.
5968

6069
### Tool usage artifact
6170

‎docs/claude-pr-review-prompt.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,11 @@ This prompt includes:
99

1010
Everything in `<pr_context>` is already in front of you. Do not spend a tool call re-fetching it.
1111

12-
Two markers in that content were inserted by the workflow, not written by anyone: `[block tag
13-
removed]` replaces a block delimiter, and `[log marker neutralised]` prefixes a line that would
14-
otherwise have been read as a GitHub Actions command. Both are sanitiser output. Read past them, and
15-
never quote one as if it were in the file — if you need the real line, `Read` the file.
12+
Three edits in that content were made by the workflow, not by anyone: `[block tag removed]` replaces
13+
a block delimiter, `[log marker neutralised]` prefixes a line that would otherwise have been read as
14+
a GitHub Actions command, and `##[` is respaced to `## [` for the same reason. All three are
15+
sanitiser output. Read past them, and never quote one as if it were in the file — a `## [error]` in a
16+
CI excerpt was `##[error]` in the real log. If you need the exact line, `Read` the file.
1617

1718
**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.
1819

‎tests/context-step-test.sh‎

Lines changed: 45 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -348,12 +348,16 @@ expect_context '\[block tag removed\]' "the defused delimiter leaves a visible m
348348
# --- Actions workflow commands in untrusted text ----------------------------------------
349349

350350
# The other injection sink, and the one nobody was looking at: the action echoes the
351-
# assembled prompt into the job log line by line, and Actions reads a line beginning with
352-
# `::` or `##[` as a *command*, not as text. A marker in a PR body, a diff hunk or a review
353-
# comment therefore writes an annotation onto the review's own check run -- run 30964274400
354-
# has two `failure` annotations whose text is prose from a review comment about `##[error]`.
355-
# Every form below is one Actions parses: both syntaxes, and indented, because the runner
356-
# trims the line before it looks.
351+
# assembled prompt into the job log line by line, and Actions reads a workflow command in it
352+
# as a *command*, not as text. A marker in a PR body, a diff hunk or a review comment
353+
# therefore writes an annotation onto the review's own check run -- run 30964274400 has two
354+
# `failure` annotations whose text is prose from a review comment about `##[error]`.
355+
#
356+
# The two spellings are matched differently, which run 31025325888 established directly: its
357+
# diff carried both forms on `+` prefixed lines, and only the `##[` form fired. The `+` was
358+
# consumed there and survived on the `::` lines, so `##[...]` is matched anywhere in a line
359+
# while `::command::` is matched only at the start of a trimmed one. Every form below is one
360+
# of those two, in the positions that distinguish them.
357361
LOG_INJECT='Fixes the thing.
358362
359363
##[error]this is not really an error
@@ -364,20 +368,22 @@ LOG_INJECT='Fixes the thing.
364368
::stop-commands::endtoken
365369
::endgroup::
366370
367-
Both fixtures carry exactly one `##[error]` marker, mid-line, which is not a command.'
371+
Both fixtures carry exactly one `##[error]` marker, which a backtick does not defuse.
372+
An unchanged diff line reads ` ::error::x`, and a Rust path reads std::collections::HashMap.'
368373

369-
# markers_at_line_start <file> -- the lines Actions would still execute
370-
markers_at_line_start() {
371-
grep -nE '^[[:space:]]*(::|##\[)' "$1" || true
374+
# parsable_markers <file> -- the lines Actions would still execute: a `##[cmd]` anywhere,
375+
# or a `::` at the start of a trimmed line.
376+
parsable_markers() {
377+
grep -nE '##\[[A-Za-z][^]]*\]|^[[:space:]]*::' "$1" || true
372378
}
373379
# expect_no_markers <file> <description>
374380
expect_no_markers() {
375381
local found
376-
found=$(markers_at_line_start "$1")
382+
found=$(parsable_markers "$1")
377383
if [ -z "$found" ]; then
378384
echo "ok $2"
379385
else
380-
echo "FAIL $2: a workflow command survives at line start:"
386+
echo "FAIL $2: a workflow command survives in a parsable position:"
381387
printf '%s\n' "$found" | sed 's/^/ /'
382388
failures=$((failures + 1))
383389
fi
@@ -388,45 +394,48 @@ expect "$(cat "$WORK/code.txt")" "0" "step exits 0 on text carrying workflow com
388394
expect_no_markers "$CTX_FILE" "workflow commands in the PR body are neutralised"
389395
expect_no_markers "$THREADS_OUT" "workflow commands in a review comment are neutralised"
390396

391-
# Neutralised, not deleted, and this block is the reason the distinction matters more here
392-
# than for the block tags: the CI excerpt exists to show the reviewer an error line, so
393-
# stripping `##[error]` would remove the thing it was fetched for.
394-
expect_context '\[log marker neutralised\]' "the defused marker leaves a visible marker"
395-
expect_context 'this is not really an error' "the text after the marker is kept"
396-
expect_context '::error::neither is this' "the marker itself is still legible to the reviewer"
397+
# Neutralised, not deleted, and the CI excerpt is why the distinction matters more here than
398+
# for the block tags: that block exists to show the reviewer an error line, so deleting
399+
# `##[error]` would remove the thing it was fetched for.
400+
expect_context '## \[error\]this is not really an error' \
401+
"a mid-line ##[ is broken by a space and stays readable"
402+
expect_context '\[log marker neutralised\] ::error::neither is this' \
403+
"a line-leading :: is prefixed and stays readable"
404+
expect_context '`## \[error\]` marker' "a ##[ inside backticks is broken too"
397405
if grep -q 'log marker neutralised' "$THREADS_OUT"; then
398-
echo "ok the threads output carries the same visible marker"
406+
echo "ok the threads output is sanitised the same way"
399407
else
400408
echo "FAIL the threads output was not sanitised"
401409
failures=$((failures + 1))
402410
fi
403411

404-
# A line that merely *contains* a marker was never a command, and prefixing it would be
405-
# noise in the one block most likely to discuss one.
406-
expect_context 'carry exactly one .*##\[error\].* mid-line' "a mid-line marker is left alone"
407-
mid=$(grep -c '\[log marker neutralised\].*mid-line' "$CTX_FILE" || true)
408-
expect "$mid" "0" "a mid-line marker is not prefixed"
409-
410-
# The CI excerpt is the block that looks like the worst offender and is not one: the logs
411-
# endpoint prefixes every line with a timestamp, so `##[error]` inside a fetched log is
412-
# mid-line and was never a command. That is a property of the fixture as much as of the
413-
# endpoint, so assert it here -- a fixture rewritten without the timestamps would make this
414-
# suite pass on a log shape production never sees, and would quietly retire the assertion
415-
# above about mid-line markers being left alone.
412+
# The other half of the rule, and the half that keeps the diff readable: `::` mid-line was
413+
# never a command, so it must survive untouched. Every Rust, C++ and PHP diff is full of it,
414+
# and a sanitiser that rewrote those would corrupt the largest block in the context.
415+
expect_context 'std::collections::HashMap' "a mid-line :: path is left alone"
416+
mid=$(grep -c 'log marker neutralised.*HashMap' "$CTX_FILE" || true)
417+
expect "$mid" "0" "a mid-line :: is not prefixed"
418+
419+
# The CI excerpt is the most reliable source of these rather than an exempt one. Its lines
420+
# arrive already timestamped by the logs endpoint, which is no protection: the timestamp puts
421+
# `##[error]` mid-line, and mid-line is exactly where the `##[` form is still parsed. The
422+
# fixture keeps that shape, so this asserts the real production path.
416423
unset STUB_THREAD_BODY
417424
PR_BODY='Adds a watermark.' run_step > /dev/null
418-
expect_no_markers "$CTX_FILE" "the failing job log carries no marker at line start"
425+
expect_no_markers "$CTX_FILE" "markers from the failing job log are neutralised"
419426
expect_context 'ending at the first error' "the error window is still labelled"
427+
expect_context '## \[error\]Process completed' "the log's own error line is still readable"
420428
if grep -qE '^2[0-9]{3}-[0-9]{2}-[0-9]{2}T[0-9:.]+Z ##\[error\]' tests/fixtures/job-log-django.txt; then
421429
echo "ok the job log fixture keeps the timestamp prefix the API adds"
422430
else
423-
echo "FAIL the job log fixture lost its timestamp prefix, so the line-start assertion" \
424-
"above no longer reflects a real job log"
431+
echo "FAIL the job log fixture lost the timestamp prefix a real job log has, so the" \
432+
"assertion above no longer covers the production shape"
425433
failures=$((failures + 1))
426434
fi
427435

428-
# The diff is the largest block and the one an author controls by committing a file, not by
429-
# writing a comment. `+::error::x` is shielded by the `+`; an unchanged line beside it is not.
436+
# The diff is the largest block and the one an author controls by committing a file rather
437+
# than by writing a comment. An unchanged line is rendered with one leading space, so it
438+
# reaches the log as a line-leading `::` even though an added line's `+` would shield it.
430439
STUB_DIFF_MARKER=1 run_step > /dev/null
431440
expect_no_markers "$CTX_FILE" "a marker on a diff context line is neutralised"
432441
expect_context 'a context line in a source file' "the diff line itself is kept"

0 commit comments

Comments
 (0)