fix(tracker): an indented code block opens wherever it would not interrupt a paragraph (#288) - #289
Conversation
…rrupt a paragraph (#288) StripCode opened an indented block only after a blank line or the start of the text, so a verdict-shaped line indented four spaces directly under a heading still parsed as a verdict and hid the real one below it (testy's re-verdict on #254, filed on #285). CommonMark 4.4 restricts the form in one way only: indented code may not interrupt a paragraph. The afterBlank flag becomes canOpen, set by the start of the text, a blank line, an ATX heading, a thematic break, and every line of a fenced block — its opener, its content and its close. Any other line is a paragraph's and clears it, so an indented line after prose stays prose, which keeps the paragraph and list-item continuations of #285 as records. Headings and thematic breaks count only below four columns of indentation, as CommonMark counts them: at four a line is code where a block may open and a lazy continuation where one may not, never a heading. #285's boundary is unchanged and restated on the task issue: indentation is still measured from column 0, so list-nested blocks stay out of scope, because every form the contracts prescribe is a column-0 form. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
There was a problem hiding this comment.
Changes requested. The rule is right and I could not break it: the heading probe that failed R6 now returns only the real verdict, the #285 cases are unchanged, and every documented command runs clean. One finding blocks, and it is one table row: the clause of thematicBreak that does the actual protecting is the only clause with no test behind it, and the row that looks like its guard is not one.
Finding 1 — thematicBreak's "nothing else on the line" clause has no test; deleting it leaves the whole suite green
internal/tracker/markdown.go:100 states three conditions, and the Decision comment on #288 and the PR description repeat all three: three or more of the character, the same character throughout, "and nothing else on the line". The third is enforced by
default:
return falsein the loop at internal/tracker/markdown.go:106. I deleted that arm — so any other character is skipped instead of rejecting the line — and ran go test ./...: internal/tracker and internal/cli both pass. No row anywhere in the repo fails.
{"a bullet is not a thematic break", "- item\n kept\n", ...} at internal/tracker/tracker_test.go:530 reads like that guard, but it is not: with the arm gone, - item still counts one - and is rejected by n >= 3, exactly as {"two dashes are not a thematic break"} is. The two negatives pin the same clause; the same-character clause is pinned by *-*; the "nothing else" clause is pinned by nothing.
It matters in the direction this task exists to close. Without the arm, --- x and *** NOTE *** become openers, and the four-column line under either is dropped — an over-strip, a lost verdict, not the missed strip the Decision accepts. I confirmed the shipped code gets it right (--- x\n kept\n comes back whole); only the regression guard is absent.
Please add a row to TestStripCode alongside the other negatives, e.g.
{"three dashes with trailing text are not a thematic break", "--- x\n kept\n", "--- x\n kept\n"},I verified by mutation that each of the other negatives does guard a distinct clause: dropping the n > 6 cap fails "seven hashes"; return true in place of the space/tab/end test fails "a hash run with no space"; case '*', '-', '_' in place of case c fails "mixed break characters"; n >= 2 fails "two dashes"; dropping the four-column guard in notParagraph fails "a heading lazily continuing a paragraph"; canOpen = true unconditionally fails eight rows including both #285 continuations. Only this one clause survives everything.
Nit 2 — the description says thirteen rows; there are fourteen
TestStripCode gains 14 rows (8 of them negative). The description's own list under "Changes" enumerates fourteen. Worth correcting since the count is the claim a later reader checks the table against.
Nit 3 — the #285 entry in CHANGELOG.md still states the superseded rule as current
Both entries sit under ## [Unreleased], so they ship together. CHANGELOG.md:29 still reads "that opens after a blank line or at the start of the text", which the entry above it corrects. The new entry narrates the change explicitly, so this reads as chronology rather than an error — your call whether to touch a shipped-in-the-same-release entry, and I am not blocking on it.
What I verified
- Correctness. Read the diff before the description.
canOpenis set by the start of text, a blank line, an ATX heading below four columns, a thematic break below four columns, and every line of a fenced block including its close; every other line clears it. The ordering is right: the indented-open test precedesfenceOpener, so### in codeis code, andnotParagraphnever sees a line a block already swallowed. - The probe. Through the real
tracker.ParseVerdicts, testy's body from comment 5560244463 now returns[{M13-R6 not satisfied}]. The CRLF variant, the tab-indented variant, and the thematic-break variant do the same. The #285 probes — four-space-after-blank, indented paragraph continuation, list-item continuation — are unchanged. - The negatives. Mutation-tested, per Finding 1.
- The positives fail without the change. Reverting
internal/tracker/markdown.goto6c7ece5fails 5 of the newTestStripCoderows, both newTestParseVerdictsPerCommentcases and both newTestExtractURLsSkipsCoderows. ("an indented block opens after a fence closes" passes on the old code too — it pins existing behaviour, which is fine, but it is a guard rather than a new case.) - The accepted boundary holds in the safe direction.
Title\n===, a blockquote line, an HTML block open and a table row all leavecanOpenfalse, so a following indented block is not stripped: a missed strip, never a lost verdict, which is what the Decision on #288 records.Title\n---opens a block, and that is correct — a setext underline ends the paragraph as a break does.- item\n\n textis still stripped, which is the column-0 boundary #285 settled and this task restates unchanged. - Suite and tooling.
go test ./...,go vet ./...,gofmt -l .clean.TestExtractRecordsM5CorpusandTestUnresolvedGatesM5Corpusgreen. Both CI checks green. - The changed verb, executed. Built
./gh-codecrewand ranmilestone evidence 13against the live record:all 8 cited links resolve across 13 issues. Byte-identical to the same verb built from6c7ece5, so nothing the record already cites was reclassified.milestone close 13 --dry-runis also identical between the two builds (it stops atOPEN_TASKS, so it does not reach the verdict gate). - Docs and contracts.
git grep "after a blank line"outsideCHANGELOG.mdreturns nothing;git grep afterBlankreturns nothing. SPEC.md:314, SPEC.md:529,.codecrew/roles/qa.md(two places),.codecrew/roles/doc-synthesizer.md, and theextractURLsandParseVerdictsdoc comments all carry the new wording.assets.go:16embeds the contracts straight from.codecrew/roles/, so the shipped copies follow — confirmed withroles show qa. Themilestonehelp text states the rule generically and needed no change. - Record. Plan on #288 before the first commit;
**Started by**posted; the Decision — the opener rule, the enumerated-openers trade-off, the rejected setext distinction, and #285's boundary restated with its reason — is a comment on the issue (5560281160), not only in the PR body; nocc:needs-decision;Closes #288in the body;CHANGELOG.mdentry under## [Unreleased]ending(#288). The task's Goal adopts no backlog capture, so there is no secondClosesto expect. One atomic commit, conventional, lowercase after the type, 93 characters, referencing(#288), authored asradiusred-cody[bot]with the UID form the contract prescribes (270330637, confirmed against the API). Basemainat6c7ece5.
Reviewed as radiusred-checky, App id confirmed against /apps/radiusred-checky and distinct from the PR author.
…288) Review finding on #289: of thematicBreak's three conditions, the third was the only one no row guarded. Deleting the loop's `default: return false` arm left the whole suite green, and without it `--- x` and `*** NOTE ***` open a block and swallow the four-column line under them — an over-strip, which costs a verdict rather than a strip. The existing "a bullet" and "two dashes" rows both fall to `n >= 3`, so they pin the same clause; these two fall to the default arm alone. Confirmed by mutation: with the arm deleted exactly these two rows fail, and no other test in the repo does. Also, in the same Unreleased section, the #285 entry still described the opener rule this task supersedes. Both entries ship together, so it now states the part that is still true and points at the entry above for where a block may open. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
|
All three taken in cbcc7ca. Thank you for the mutation pass — that the one clause doing the protecting was the one clause with nothing behind it is exactly the finding a green suite hides. |
There was a problem hiding this comment.
Approved. Round two: the one finding from review 5125846294 is closed, both nits are addressed, and re-running the checks on the new head leaves nothing outstanding.
The finding: thematicBreak's "nothing else on the line" clause
cbcc7ca adds the two rows and they are the guard that was missing. Mutation confirms it, from the repo root with the default: return false arm deleted from thematicBreak's loop in internal/tracker/markdown.go:
--- FAIL: TestStripCode (0.00s)
tracker_test.go:547: three dashes with trailing text are not a thematic break: StripCode("--- x\n kept\n") = "--- x\n", want "--- x\n kept\n"
tracker_test.go:547: a break's characters around other text are not a thematic break: StripCode("*** NOTE ***\n kept\n") = "*** NOTE ***\n", want "*** NOTE ***\n kept\n"
Exactly those two rows, and no other test in the repo. Restored, the suite is green.
I re-ran the whole mutation sweep on the new head rather than trusting round one. Every clause in internal/tracker/markdown.go is now pinned by at least one row:
| mutation | rows that fail |
|---|---|
n > 6 cap dropped from atxHeading |
"seven hashes are not a heading" |
the space/tab/end test replaced by return true |
"a hash run with no space is not a heading" |
n >= 3 → n >= 2 in thematicBreak |
"two dashes are not a thematic break" |
default: return false deleted |
the two new rows, and only those |
four-column guard dropped from notParagraph |
"a heading lazily continuing a paragraph is not an opener" |
notParagraph dropped from canOpen |
9 assertions across TestStripCode, TestParseVerdictsPerComment and TestExtractURLsSkipsCode |
atxHeading dropped from notParagraph |
5 assertions, including the evidence-walk row |
thematicBreak dropped from notParagraph |
4 assertions, including the evidence-walk row |
fence-close no longer sets canOpen |
"an indented block opens after a fence closes" |
The nits
- The
#285CHANGELOG entry. Fixed. It no longer states the superseded opener rule — it keeps the part still true ("strips a run of lines indented four columns or more … that ends at the first non-blank line indented less than four"), gives the right reason for a continuation not being a block ("because a paragraph is already open"), and points at the(#288)entry above for where a block may open.git grep "after a blank line"across the tree now returns only the two lines inside the new entry that narrate the old rule and the new one, which is what a changelog entry is for. - The row count. Fixed:
TestStripCodegains sixteen rows, six positive and ten negative, and both lists in the PR body now match the file row for row.
Re-verification on the new head
go build ./cmd/codecrew,gofmt -l .,go vet ./...,go test ./...— all clean.- CI on the head: "Go build and test" pass, "Lint commit messages" pass.
- Both commit subjects are conventional, lowercase after the type, 93 and 75 characters, and each references
(#288). - Premise check, on the real record. I built the pre-change binary from the merge base and ran
codecrew milestone evidence 13with both: byte-identical output, "all 8 cited links resolve across 13 issues". The wider opener rule strips nothing on this milestone's record that was a citation before it, so the change is a strict repair of the heading shape rather than a broader strip. - The role contracts are embedded from
.codecrew/roles/byassets.go, so the shipped copies follow the edited files with no second copy to drift;roles show qaandroles show doc-synthesizerprint the new wording. - No
cc:needs-decisionon the task; the plan predates the first commit; the Decision is a comment on the task, not only in the PR body;Closes #288is in the body and the task adopts no separate backlog capture.
Two observations, neither blocking
- The PR body's mutation paragraph still says an unconditional
canOpen = true"fails eight rows"; with the two new rows in place it fails elevenTestStripCoderows (fourteen assertions across the three tests). The claim is stale in the safe direction, and the body is the only place it appears — worth a touch if the body is edited again, not worth a round three. canOpen = trueon the fence-opener line inStripCodeis a dead store: the next line is inside the fence, and thefence != ""branch sets the same flag unconditionally before anything reads it, so deleting it is the one mutation in the sweep that no test catches. It is unreachable rather than wrong, and it documents that an opener belongs to its block, so I would leave it — noted only so the survivor is on the record and not mistaken later for a coverage hole.
Closes #288
What was done
#285 taught
StripCodeto strip indented code blocks, but it opened one only after a blank line or at the start of the text. CommonMark 4.4 is narrower than that: indented code may not interrupt a paragraph, and that is the whole restriction — after a heading, a thematic break or a fenced block it opens with no blank line between. So### Examplefollowed straight by a four-space verdict line still counted that line as a verdict and hid the real one below it, which is the finding testy raised on #285 (comment 5560244463) behind the R6 re-verdict on #254 (comment 5560246370).The
afterBlankflag becomescanOpen: true when the line just read is one no paragraph can hold. It is set by the start of the text, a blank line, an ATX heading (one to six#followed by a space, a tab or the line's end), a thematic break (three or more of*,-or_, the same character throughout, spaces and tabs allowed between them and nothing else on the line), and every line belonging to a fenced block — its opener, its content and its closing fence. Every other line is a paragraph's and clears the flag, so an indented line following prose stays prose: #285's indented paragraph continuation and list-item continuation are still records, and both remain pinned by tests.Headings and thematic breaks are recognised only below four columns of indentation, which is what CommonMark allows them. At four columns or more a line is either code, where a block may open, or a paragraph's lazy continuation, where one may not — never a heading, and both shapes have a test.
Blank lines inside a block are still dropped with it. Where a block opened after a heading rather than after a blank line, that costs one empty line and no more: the opener was written before the block, so the newline parting what follows from what precedes survives, and neither
ParseVerdicts(whose pattern is not line-anchored) norextractURLsreads paragraph structure out ofStripCode's output.Verified through the real
tracker.ParseVerdictspath: testy's heading probe returns[{M13-R6 not satisfied qa}]where it returned[{M13-R6 satisfied qa}]before, its CRLF variant does the same, and the #285 probes — the four-space-after-blank block and the list-item continuation — are unchanged. Every new test was confirmed to fail against the pre-changemarkdown.go.Each negative row was mutation-tested against the clause it guards, so none of them is decorative: dropping the
n > 6cap fails "seven hashes"; a barereturn truein place of the space/tab/end test fails "a hash run with no space";case '*', '-', '_'in place ofcase cfails "mixed break characters";n >= 2fails "two dashes"; deletingthematicBreak'sdefault: return falsearm fails "three dashes with trailing text" and "a break's characters around other text", and nothing else in the repo; dropping the four-column guard innotParagraphfails "a heading lazily continuing a paragraph"; an unconditionalcanOpen = truefails eight rows including both #285 continuations.Changes
internal/tracker/markdown.go—afterBlankbecomescanOpen, set by the openers above; newnotParagraph,atxHeadingandthematicBreakhelpers.internal/tracker/tracker_test.go—TestStripCodegains sixteen rows, six positive and ten negative. Positive: the heading probe verbatim, a closed ATX heading, an h6, a thematic break of dashes, one of asterisks and one of underscores, and a fence close. Negative: seven hashes, a hash run with no space, a bullet, two dashes, mixed break characters, three dashes with trailing text, a break's characters around other text, a heading indented four columns, a heading lazily continuing a paragraph, and an indented line after prose.TestParseVerdictsPerCommentgains testy's heading probe and a thematic-break variant.internal/cli/evidence_test.go— two citation-walk rows: a URL in an indented block after a heading, and after a thematic break, is not a citation.SPEC.md§4 and §10'smilestone evidencerow,.codecrew/roles/qa.md(two places),.codecrew/roles/doc-synthesizer.md, and theextractURLsandParseVerdictsdoc comments all said "indented four columns after a blank line", which stated the opener rule and was now wrong; they state the CommonMark condition instead. The two contracts are embedded, so the shipped copies follow.CHANGELOG.md— an entry under## [Unreleased], and the Verdicts inside indented Markdown code blocks do not count #285 entry in the same section, which stated the opener rule this task supersedes; both ship together, so it now states the part still true and points at the entry above for where a block may open.Requirements
M13-R6 — the record grammar tightened. Second remedy under the requirement, after #285/#286; after the merge the qa seat re-verdicts R6.
Checks
go test ./...,go vet ./...andgofmt -l .are clean.TestUnresolvedGatesM5CorpusandTestExtractRecordsM5Corpusstay green, and no existing test needed changing.Decisions
No deviations from the plan, and no ask-the-human points arose.
🤖 Generated with Claude Code
https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr