fix(strip-non-invoking-markup): open an indented code block after a heading or thematic break - #403
Conversation
…graph block (closes #356)
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eading 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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…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).
ARD — review round 1 (claude-review: "Needs more work")Finding 1 (blocking): setext heading Finding 2 (non-blocking, convention): missing changelog fragment → Addressed — added Verification
Ruled-out from round 1 stand (mawk |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…raph 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).
ARD — review round 2 (claude-review: "Needs more work")Finding (blocking): shellcheck clean; no banned punctuation. Re-requesting review. |
|
Claude finished review — View run Review: PR #403 —
|
…ha#403 cite claude-review (#1131) round 1: - Reworded the discharge sentence: the hook demands a Copilot request (the one action the override forbids), so honoring the override never satisfies it -- replacing the imprecise 'lands a refusal not a verdict' framing (the hook keys on a recognized request call, not on a posted review). - Dropped the 'Morrison-Lab/gha#403' citation: #403 is a Markdown-parsing fix, not a record of the hook collision, so citing it there was misleading.
… is out (#1131) * memory: while Copilot quota is out, unregister the no-unreviewed-pr Stop hook Extends the #1129 note in memories/github.md. The no-unreviewed-pr.py Stop hook (ai-config#1041) enforces the opposite instruction -- request Copilot every turn -- so while the quota is out it fires each turn a PR awaits review and cannot be discharged (the request lands the quota refusal, not a verdict). Unregister it from ~/.claude/settings.json's Stop hooks until September 2026; the script stays, only the local registration goes. A Morrison-Lab/gha session spent over a dozen turns in this loop before it was recognized. * review: reword hook-collision mechanism accurately; drop misleading gha#403 cite claude-review (#1131) round 1: - Reworded the discharge sentence: the hook demands a Copilot request (the one action the override forbids), so honoring the override never satisfies it -- replacing the imprecise 'lands a refusal not a verdict' framing (the hook keys on a recognized request call, not on a posted review). - Dropped the 'Morrison-Lab/gha#403' citation: #403 is a Markdown-parsing fix, not a record of the hook collision, so citing it there was misleading.
Closes #356
WIP — opened up front to claim the issue; implementing now.
Plan
strip-non-invoking-markup.shopens an indented code block only when thepreceding line was blank (
prev_blank). Per CommonMark, the blank line isonly required so an indented block cannot interrupt a paragraph — following
a thematic break or an ATX heading, an indented block opens with no
blank line at all. So a quoted request in that position is not stripped and
dispatches a review off quoted text.
Fix: widen the icode-open precondition from "previous line blank" to "previous
line does not leave an open paragraph/list to lazily continue" — blank, ATX
heading, or thematic break. Deliberately not handling the list-item
predecessor (an indented line after a list item is a list continuation, not
code — the over-stripping direction #345 added guards for).