From 3d4356b1f6d2593acfae6cb646fcbd859442fe36 Mon Sep 17 00:00:00 2001 From: d-morrison Date: Mon, 3 Aug 2026 20:36:23 -0700 Subject: [PATCH 1/4] start: strip-non-invoking-markup indented code block after a non-paragraph block (closes #356) From 0cdb57be4025cce86e1200ecc9f746774674d1c5 Mon Sep 17 00:00:00 2001 From: d-morrison Date: Mon, 3 Aug 2026 20:41:53 -0700 Subject: [PATCH 2/4] fix(strip-non-invoking-markup): open an indented code block after a heading or thematic break An indented code block may begin with no blank line after a thematic break or an ATX heading -- neither leaves an open paragraph for the indented line to lazily continue (CommonMark). The prior blank-line precondition missed that, so a quoted request indented under a `---` or `#` heading was not stripped and dispatched a review off quoted text. Widen the icode-open precondition to a new opens_icode() predicate: blank, ATX heading, or thematic break. A list item deliberately does not qualify -- an indented line after one is a list continuation, not code -- preserving the over-stripping guards from #345. Thematic breaks are detected by character count rather than a backreference regex, which POSIX ERE (awk) does not support. Adds four cases to the offline suite (31 total): the thematic-break, ATX, and spaced-thematic-break openers (each fails against the pre-fix script), plus a list-item guard. Closes #356 --- .../scripts/strip-non-invoking-markup.sh | 67 +++++++++++++++---- .../run-strip-non-invoking-markup-tests.sh | 23 +++++++ 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/.github/workflows/scripts/strip-non-invoking-markup.sh b/.github/workflows/scripts/strip-non-invoking-markup.sh index 65f2529..b7b3db0 100755 --- a/.github/workflows/scripts/strip-non-invoking-markup.sh +++ b/.github/workflows/scripts/strip-non-invoking-markup.sh @@ -70,6 +70,43 @@ function run_len(s, ch, n) { return n } +# Whether `bare` is a CommonMark thematic break: three or more of a single one +# of `- * _`, with only spaces or tabs between. Written as a character count +# rather than a regex because the POSIX ERE that awk uses has no backreference, +# so a same-character-repeated pattern (a `\1` in PCRE) silently never matches. +function is_thematic_break(bare, ch, i, c, n) { + ch = substr(bare, 1, 1) + if (ch != "-" && ch != "*" && ch != "_") return 0 + n = 0 + for (i = 1; i <= length(bare); i++) { + c = substr(bare, i, 1) + if (c == ch) n++ + else if (c != " " && c != "\t") return 0 + } + return (n >= 3) +} + +# Whether an indented code block may begin on the line *after* this one. +# `bare` is the line with its indentation removed, `indent` its width. +# +# CommonMark forbids an indented code block only from *interrupting a +# paragraph*, so the blank line the caller checks for is one way -- but not the +# only way -- to know the following indented line is not a lazy paragraph +# continuation. A thematic break and an ATX heading are leaf blocks that leave +# no open paragraph, so an indented block opens after them with no blank line. +# A list item deliberately does NOT qualify: an indented line after one is a +# list continuation, not code, and stripping it is the over-stripping error the +# mention gate must avoid (the list-continuation cases added in #345 guard +# exactly this). Both shapes are only valid at three columns of indentation or +# fewer; deeper, they are themselves code. +function opens_icode(bare, indent) { + if (indent > 3) return 0 + # ATX heading: 1-6 `#` then a space/tab or end of line. + if (bare ~ /^#{1,6}([ \t]|$)/) return 1 + if (is_thematic_break(bare)) return 1 + return 0 +} + # Replace every closed inline code span in `text` with the placeholder. # # This runs once over the WHOLE remaining body rather than per line, because a @@ -121,7 +158,7 @@ function strip_spans(text, out, i, n, run, j, closerun, found) { BEGIN { in_fence = 0; fence_char = ""; fence_len = 0 in_icode = 0 - prev_blank = 1 # start of input behaves like a blank line + prev_opens_icode = 1 # start of input can open an indented code block kept = ""; nkept = 0 } @@ -142,23 +179,25 @@ BEGIN { substr(bare, run_len(bare, fence_char) + 1) ~ /^[ \t]*$/) { in_fence = 0 } - prev_blank = blank + prev_opens_icode = 0 next } # --- indented code block ---------------------------------------------- - # Four columns of indentation after a blank line opens one, and it runs - # until a non-blank line dedents to three columns or fewer. The blank-line - # precondition is what keeps an indented *list continuation* out of this - # branch: those follow their list item directly, so over-stripping them - # would drop a genuine request, which is the expensive error for the - # mention gate sharing this script. + # Four columns of indentation opens one when the preceding line does not + # leave an open paragraph or list for it to lazily continue (a blank line, a + # thematic break, or an ATX heading -- see `opens_icode`). It runs until a + # non-blank line dedents to three columns or fewer. The precondition is what + # keeps an indented *list continuation* out of this branch: those follow + # their list item directly, so over-stripping them would drop a genuine + # request, which is the expensive error for the mention gate sharing this + # script. if (in_icode) { - if (blank || indent >= 4) { prev_blank = blank; next } + if (blank || indent >= 4) { prev_opens_icode = 0; next } in_icode = 0 - } else if (prev_blank && !blank && indent >= 4) { + } else if (prev_opens_icode && !blank && indent >= 4) { in_icode = 1 - prev_blank = blank + prev_opens_icode = 0 next } @@ -167,17 +206,17 @@ BEGIN { fence_char = lead fence_len = run_len(bare, lead) in_fence = 1 - prev_blank = blank + prev_opens_icode = 0 next } - if (lead == ">") { prev_blank = blank; next } + if (lead == ">") { prev_opens_icode = 0; next } } # Kept lines are buffered rather than printed, so the span scan below can # see across line boundaries. kept = (nkept == 0) ? line : kept "\n" line nkept++ - prev_blank = blank + prev_opens_icode = (blank || opens_icode(bare, indent)) } END { diff --git a/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh b/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh index ab4b037..c79e46a 100755 --- a/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh +++ b/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh @@ -167,6 +167,29 @@ check "indentation without a preceding blank line is not code" \ $'some prose\n @claude review' \ $'some prose\n @claude review' +# An indented code block also opens after a thematic break or an ATX heading +# with no blank line: neither leaves an open paragraph for the indented line to +# lazily continue (CommonMark 0.31.2, "Indented code blocks"). A request quoted +# this way must still be stripped (gha#356). +check "indented code block opens after a thematic break" \ + $'---\n @claude review' \ + '---' + +check "indented code block opens after an ATX heading" \ + $'## Accepted phrasing\n @claude review' \ + '## Accepted phrasing' + +check "indented code block opens after a spaced thematic break" \ + $'* * *\n @claude review' \ + '* * *' + +# A list item is NOT such a predecessor: an indented line after it is a list +# continuation, not code, so it must survive -- and a lone-dash item must not +# be mistaken for a thematic break, which would over-strip a genuine request. +check "indented line after a list item is not code" \ + $'- a point\n @claude review' \ + $'- a point\n @claude review' + check "CRLF endings are normalized" \ $'@claude review\r\nthanks\r' \ $'@claude review\nthanks' From e58369e4cb1bf68c04eaa63cc40caff3981a92f4 Mon Sep 17 00:00:00 2001 From: d-morrison Date: Mon, 3 Aug 2026 21:01:44 -0700 Subject: [PATCH 3/4] fix(strip-non-invoking-markup): recognize setext heading underlines; add changelog fragment Review round 1 (claude-review): opens_icode missed =-underlined setext headings, so a request quoted as an indented code block under 'Heading\n======' was not stripped and dispatched a review off quoted text -- the exact bug class #356 exists to close, for a construct ("headings") #356 lists in scope. The - underline worked only by coincidence (>=3 dashes also satisfy is_thematic_break). A setext underline is a heading underline only when the line before it is paragraph text; the same =/- run elsewhere is a paragraph, a thematic break, or a list marker. So add is_setext_underline() plus a prev_para paragraph-context flag, and open an indented code block after a setext underline only when it sits under paragraph text. Rename opens_icode -> heading_or_break (the context-free ATX/thematic part). Covers both underline characters and 1-2 dash underlines. Adds four offline cases (35 total): =-underline and single-dash-underline openers (each fails against the round-1 script), plus two guards that a =-run at start or after a blank is a paragraph and must not over-strip. Also adds the changelog.d/ fragment every prior fix to this file carried (review finding 2). --- .../scripts/strip-non-invoking-markup.sh | 75 ++++++++++++------- .../run-strip-non-invoking-markup-tests.sh | 23 ++++++ ...nvoking-markup-block-predecessors.fixed.md | 14 ++++ 3 files changed, 84 insertions(+), 28 deletions(-) create mode 100644 changelog.d/strip-non-invoking-markup-block-predecessors.fixed.md diff --git a/.github/workflows/scripts/strip-non-invoking-markup.sh b/.github/workflows/scripts/strip-non-invoking-markup.sh index b7b3db0..4d8d744 100755 --- a/.github/workflows/scripts/strip-non-invoking-markup.sh +++ b/.github/workflows/scripts/strip-non-invoking-markup.sh @@ -86,20 +86,28 @@ function is_thematic_break(bare, ch, i, c, n) { return (n >= 3) } -# Whether an indented code block may begin on the line *after* this one. -# `bare` is the line with its indentation removed, `indent` its width. -# -# CommonMark forbids an indented code block only from *interrupting a -# paragraph*, so the blank line the caller checks for is one way -- but not the -# only way -- to know the following indented line is not a lazy paragraph -# continuation. A thematic break and an ATX heading are leaf blocks that leave -# no open paragraph, so an indented block opens after them with no blank line. -# A list item deliberately does NOT qualify: an indented line after one is a -# list continuation, not code, and stripping it is the over-stripping error the -# mention gate must avoid (the list-continuation cases added in #345 guard -# exactly this). Both shapes are only valid at three columns of indentation or -# fewer; deeper, they are themselves code. -function opens_icode(bare, indent) { +# Whether `bare` is a setext heading underline: a run of only `=` or only `-`, +# trailing spaces/tabs allowed. This is a *heading* underline only when the line +# before it is paragraph text, which the caller tracks separately -- the same +# characters are otherwise a thematic break (`- * _`, >=3), a list marker, or +# ordinary text. CommonMark makes the two underline characters symmetric, and an +# indented code block opens after either with no blank line, so the `-` case +# that `is_thematic_break` already covers for >=3 dashes needs its `=` twin here +# (plus 1-2 dash underlines, which are not thematic breaks). +function is_setext_underline(bare) { + return bare ~ /^=+[ \t]*$/ || bare ~ /^-+[ \t]*$/ +} + +# Whether this line is a leaf block -- an ATX heading or a thematic break -- +# after which an indented code block may begin with no blank line. `bare` is the +# line with its indentation removed, `indent` its width. These two shapes need +# no context; a setext underline also qualifies but only after paragraph text, +# so the caller handles it. A list item deliberately does NOT qualify: an +# indented line after one is a list continuation, not code, and stripping it is +# the over-stripping error the mention gate must avoid (the list-continuation +# cases added in #345 guard exactly this). Both shapes are only valid at three +# columns of indentation or fewer; deeper, they are themselves code. +function heading_or_break(bare, indent) { if (indent > 3) return 0 # ATX heading: 1-6 `#` then a space/tab or end of line. if (bare ~ /^#{1,6}([ \t]|$)/) return 1 @@ -159,6 +167,7 @@ BEGIN { in_fence = 0; fence_char = ""; fence_len = 0 in_icode = 0 prev_opens_icode = 1 # start of input can open an indented code block + prev_para = 0 # ... but is not itself paragraph text (no setext under it) kept = ""; nkept = 0 } @@ -179,25 +188,26 @@ BEGIN { substr(bare, run_len(bare, fence_char) + 1) ~ /^[ \t]*$/) { in_fence = 0 } - prev_opens_icode = 0 + prev_opens_icode = 0; prev_para = 0 next } # --- indented code block ---------------------------------------------- - # Four columns of indentation opens one when the preceding line does not - # leave an open paragraph or list for it to lazily continue (a blank line, a - # thematic break, or an ATX heading -- see `opens_icode`). It runs until a - # non-blank line dedents to three columns or fewer. The precondition is what - # keeps an indented *list continuation* out of this branch: those follow - # their list item directly, so over-stripping them would drop a genuine - # request, which is the expensive error for the mention gate sharing this - # script. + # Four columns of indentation opens one when the preceding line does not leave + # an open paragraph or list for it to lazily continue. `prev_opens_icode` + # carries that from the previous iteration: it is set for a blank line, an ATX + # heading, a thematic break (`heading_or_break`), or a setext underline sitting + # under paragraph text. It runs until a non-blank line dedents to three columns + # or fewer. The precondition is what keeps an indented *list continuation* out + # of this branch: those follow their list item directly, so over-stripping them + # would drop a genuine request, which is the expensive error for the mention + # gate sharing this script. if (in_icode) { - if (blank || indent >= 4) { prev_opens_icode = 0; next } + if (blank || indent >= 4) { prev_opens_icode = 0; prev_para = 0; next } in_icode = 0 } else if (prev_opens_icode && !blank && indent >= 4) { in_icode = 1 - prev_opens_icode = 0 + prev_opens_icode = 0; prev_para = 0 next } @@ -206,17 +216,26 @@ BEGIN { fence_char = lead fence_len = run_len(bare, lead) in_fence = 1 - prev_opens_icode = 0 + prev_opens_icode = 0; prev_para = 0 next } - if (lead == ">") { prev_opens_icode = 0; next } + if (lead == ">") { prev_opens_icode = 0; prev_para = 0; next } } # Kept lines are buffered rather than printed, so the span scan below can # see across line boundaries. kept = (nkept == 0) ? line : kept "\n" line nkept++ - prev_opens_icode = (blank || opens_icode(bare, indent)) + # Decide what the NEXT line inherits. A blank line, an ATX heading, or a + # thematic break (`hd`) opens an indented code block after it with no context; + # a setext underline (`ul`) does too, but only when the line before it -- the + # `prev_para` carried into this iteration -- was paragraph text. `prev_para` + # then records whether THIS line is itself paragraph text, i.e. a plain + # non-blank line that a following `=`/`-` run would underline. + hd = heading_or_break(bare, indent) + ul = (indent <= 3 && is_setext_underline(bare)) + prev_opens_icode = (blank || hd || (ul && prev_para)) + prev_para = (!blank && !hd && !ul) } END { diff --git a/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh b/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh index c79e46a..3a43534 100755 --- a/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh +++ b/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh @@ -190,6 +190,29 @@ check "indented line after a list item is not code" \ $'- a point\n @claude review' \ $'- a point\n @claude review' +# A setext heading underline (a run of only `=` or only `-` under paragraph text) +# is also such a predecessor. CommonMark makes the two underline characters +# symmetric; the `-` case is caught by the thematic-break check only for >=3 +# dashes, so the `=` twin and the 1-2 dash underlines are handled here. +check "indented code block opens after an = setext underline" \ + $'Heading\n======\n @claude review' \ + $'Heading\n======' + +check "indented code block opens after a single-dash setext underline" \ + $'Heading\n-\n @claude review' \ + $'Heading\n-' + +# But a run of `=` NOT under paragraph text is an ordinary paragraph, not a +# heading underline, so an indented line after it is a lazy continuation, not +# code -- stripping it would drop a genuine request. +check "= run at start of input is a paragraph, not a setext underline" \ + $'======\n @claude review' \ + $'======\n @claude review' + +check "= run after a blank line is a paragraph, not a setext underline" \ + $'intro\n\n======\n @claude review' \ + $'intro\n\n======\n @claude review' + check "CRLF endings are normalized" \ $'@claude review\r\nthanks\r' \ $'@claude review\nthanks' diff --git a/changelog.d/strip-non-invoking-markup-block-predecessors.fixed.md b/changelog.d/strip-non-invoking-markup-block-predecessors.fixed.md new file mode 100644 index 0000000..102e21c --- /dev/null +++ b/changelog.d/strip-non-invoking-markup-block-predecessors.fixed.md @@ -0,0 +1,14 @@ +- **The `@claude review` matcher no longer fires on a mention quoted as an + indented code block that opens after a heading or a thematic break** + (gha#356). + `strip-non-invoking-markup.sh` only opened an indented code block after a + blank line, but CommonMark bars an indented block only from *interrupting a + paragraph* -- after a thematic break, an ATX heading, or a setext-heading + underline it opens with no blank line at all. + So a request quoted under a `---` or a `#`/`===` heading rendered as code on + GitHub yet slipped through the stripper and dispatched a review off quoted + text -- the same class of false dispatch the code-span and fence handling + already close. + A list item is deliberately still not treated as such a predecessor: an + indented line after one is a list continuation, not code, so stripping it + would drop a genuine request in the mention gate that shares this script. From 57898ce02e37163bb79c1bf230543cce61345d62 Mon Sep 17 00:00:00 2001 From: d-morrison Date: Mon, 3 Aug 2026 21:15:05 -0700 Subject: [PATCH 4/4] fix(strip-non-invoking-markup): track consumed-as-underline for paragraph state Review round 2 (claude-review): prev_para's update used the raw `ul` match instead of whether the line was actually consumed as a setext underline (`ul && prev_para`). A `=`/`-` run with no paragraph above it is not consumed -- CommonMark makes it an ordinary new paragraph -- so prev_para must go to 1 for the next line. With the raw-ul formula, a chain of underline-shaped lines mistracked state: `===` / `===` / indented request formed an H1 whose indented follower was not recognized as code, so the quoted request survived and reached the matcher (verified: detect-review-request returned true; want false). Introduce `consumed = (ul && prev_para)` and key both prev_opens_icode and prev_para on it. Adds a regression case (36 total) for the two-`===`-line chain, confirmed to fail against the round-2 script (e58369e). --- .../scripts/strip-non-invoking-markup.sh | 16 ++++++++++------ .../tests/run-strip-non-invoking-markup-tests.sh | 9 +++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/.github/workflows/scripts/strip-non-invoking-markup.sh b/.github/workflows/scripts/strip-non-invoking-markup.sh index 4d8d744..099942b 100755 --- a/.github/workflows/scripts/strip-non-invoking-markup.sh +++ b/.github/workflows/scripts/strip-non-invoking-markup.sh @@ -228,14 +228,18 @@ BEGIN { nkept++ # Decide what the NEXT line inherits. A blank line, an ATX heading, or a # thematic break (`hd`) opens an indented code block after it with no context; - # a setext underline (`ul`) does too, but only when the line before it -- the - # `prev_para` carried into this iteration -- was paragraph text. `prev_para` - # then records whether THIS line is itself paragraph text, i.e. a plain - # non-blank line that a following `=`/`-` run would underline. + # a setext underline does too, but only when it is actually *consumed* as one + # -- a `=`/`-` run (`ul`) sitting under paragraph text (`prev_para`). A ul-shaped + # run with no paragraph above it is not consumed: CommonMark makes it an + # ordinary new paragraph, so `prev_para` must go to 1 for the next line (a later + # underline could underline it), which is why `prev_para` keys on `consumed` + # rather than the raw `ul` match. Otherwise a chain of underline-shaped lines + # (`===` / `===` / indented request) mistracks state and under-strips. hd = heading_or_break(bare, indent) ul = (indent <= 3 && is_setext_underline(bare)) - prev_opens_icode = (blank || hd || (ul && prev_para)) - prev_para = (!blank && !hd && !ul) + consumed = (ul && prev_para) + prev_opens_icode = (blank || hd || consumed) + prev_para = (!blank && !hd && !consumed) } END { diff --git a/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh b/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh index 3a43534..22a5966 100755 --- a/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh +++ b/.github/workflows/scripts/tests/run-strip-non-invoking-markup-tests.sh @@ -213,6 +213,15 @@ check "= run after a blank line is a paragraph, not a setext underline" \ $'intro\n\n======\n @claude review' \ $'intro\n\n======\n @claude review' +# A chain of underline-shaped lines: the first `===` has no paragraph above it so +# it is an ordinary paragraph, and the second `===` then underlines it into a +# heading -- after which the indented request is code and must be stripped. This +# only works if paragraph state tracks whether a line was actually *consumed* as +# an underline, not the raw match. +check "chain of two = runs forms a heading, stripping the indented request" \ + $'===\n===\n @claude review' \ + $'===\n===' + check "CRLF endings are normalized" \ $'@claude review\r\nthanks\r' \ $'@claude review\nthanks'