Commit df4fcfa
Plan 185: Expose extended-syntax parsers and the flavor model in pkg/markdown (#409)
* Start plan 185: Expose extended-syntax parsers and the flavor model in pkg/markdown
* Plan 185: expose extended-syntax parsers and flavor model in pkg/markdown
Promotes every custom goldmark parser and the per-flavor Feature support
model into a new public pkg/markdown/flavor sub-package. The MDS034 rule
is reduced to a thin adapter; internal/schema's hand-rolled goldmark
config and the rule's own dual parser both fold into the single
goldmark.New call site in pkg/markdown/flavor/parser.go.
- pkg/markdown/flavor: Flavor and Feature types, the support table,
Detect(doc *markdown.Document, accept func(Feature) bool) []Finding,
Finding/HeadingIDExtra shapes, four NewParser*/NewPooledParser*
constructors, and small rewriter helpers (FindHeadingID,
IsGitHubAlert, LineCol).
- pkg/markdown/flavor/ext: the five custom extensions
(Superscript, Subscript, MathBlock, MathInline, Abbreviation) and
their AST node kinds, moved wholesale from
internal/rules/markdownflavor/ext.
- internal/convention: Flavor is now a type alias for
pkg/markdown/flavor.Flavor; constants and ParseFlavor are
re-exported so internal/config compiles unchanged.
- internal/rules/markdownflavor: Check builds a *markdown.Document
from *lint.File, calls flavor.Detect, and maps findings to
diagnostics. Fix retains its byte-range edit pipeline but uses
flavor.NewPooledParser instead of a private singleton.
- internal/schema/validate_content.go: replaces the local
goldmark.New(extension.Table) with flavor.NewPooledParserWith.
- internal/integration/rules_test.go: drops the goldmark-frontmatter
test helper in favour of lint.StripFrontMatter + yamlutil.UnmarshalSafe
so no goldmark.New remains under internal/.
- Contract test pins the public flavor API shape.
- Docs (markdown-library.md, architecture/index.md, cross-system.md,
go.md) updated to drop the "CommonMark only" wording and document
the new sub-package.
Verifies all acceptance criteria: pkg/markdown imports no internal/,
no goldmark.New under internal/ or cmd/, no custom AST node types or
parsers outside pkg/markdown, and MDS034 / schema diagnostics remain
byte-identical (existing tests pass unchanged).
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Fix broken links to moved ext package and lift FindHeadingID coverage
The pinned-version and source mdsmith-check jobs both failed: three
plan/185 references and one architecture-audit reference still pointed
at the now-empty internal/rules/markdownflavor/ext path. Locally the
references resolved because git mv left an empty directory behind;
CI's fresh checkout sees them as dead links.
- plan/185_public-markdown-flavor-library.md: retarget every
internal/rules/markdownflavor/ext link at pkg/markdown/flavor/ext
(the new home).
- docs/development/architecture-audit.md: rewrite the
"markdownflavor/ext sub-package" finding as resolved by plan/185,
drop the broken link, and trim to stay under the 300-line cap.
- pkg/markdown/flavor/detect.go: drop the defensive nil-AST guards I
added in detectBareURLs / detectGitHubAlerts; flavor.Detect already
short-circuits on doc == nil and the original *lint.File variant
carried no such check.
- pkg/markdown/flavor/detect_edge_test.go: exercise both branches of
the public FindHeadingID wrapper so codecov/patch sees full coverage
on the new helper.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Fix duplicate link-ref transformer in newParserInternal
Copilot review caught a real bug: goldmark.New() calls DefaultParser()
which already installs DefaultParagraphTransformers(), so the previous
goldmark.WithParserOptions(parser.WithParagraphTransformers(defaults...))
call appended a second link-reference transformer on top. The reset
closure only touched the appended instance; the one inside the default
parser kept pinning the last parsed document's bytes.
Build the parser explicitly with parser.NewParser (one set of block,
inline, and paragraph parsers including the lrp captured for reset)
and install it via goldmark.WithParser, then let goldmark.New's
extension Extend hooks register the additional block / inline parsers
they need. After this change there is exactly one link-ref transformer
in the resulting parser, and the closure returned to NewPooledParser
callers resets that instance.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Pool the dual parser in flavor.Detect; inline onlyAccept
Self-review found two issues from the plan-185 changes:
1. Detect built a fresh goldmark parser per call via NewPooledParser,
running the full Extend hook chain on every Check. The previous
singleton in internal/rules/markdownflavor avoided this; the move
to a stateless public Detect regressed it. Add a sync.Pool inside
the flavor package that hands each Detect goroutine its own
parser-with-reset pair, mirroring internal/schema's
contentParserPool. The pool resets the link-reference transformer
before Put so idle slots do not pin document bytes.
2. fix.go used a one-line onlyAccept helper that was only ever called
from one site. Inline the closure literal at the call site and
drop the helper.
A new BenchmarkDetectReusesPool exercises the dual-parser code path
repeatedly; coverage in pkg/markdown/flavor stays at 100%.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Expose NearestBlockAncestor, add byte-identical pin test, fill pyramid
Addresses the two remaining self-review concerns and aligns the
package's tests with the test-pyramid rule that every production
function ships its dedicated unit test by name.
Concern #2 — drop the nearestBlockAncestor duplicate:
- Expose NearestBlockAncestor from pkg/markdown/flavor so external
rewriters (and the rule adapter) share the helper instead of
duplicating it. Replace the rule's private copy in fix.go with
flavor.NearestBlockAncestor.
- Add it to the contract test, the markdown-library stable surface
list, and a dedicated TestNearestBlockAncestorPublic test.
Concern #3 — byte-identical pin test:
- pkg/markdown/flavor/detect_pin_test.go adds a corpus-driven table
test (pinCorpus) that maps each input to the exact Finding stream
(feature + 1-based line + 1-based column, in document order).
Plan 185 acceptance criterion "Table tests pin this" is now an
explicit gate; any subtle reorder, drop, or shift in MDS034
diagnostics will break the test with a side-by-side diff.
Test-pyramid alignment:
- TestNearestBlockAncestor (subtests for the skip-non-block and
orphan branches) plus TestNearestBlockAncestorPublic for the
exported wrapper.
- TestIsGitHubAlertPublic exercises both branches of IsGitHubAlert
(alert blockquote / heading-first-child).
- TestLineColPublic pins the documented 1-based semantics of the
exported LineCol wrapper.
- TestDualFindings covers the dualFindings helper I extracted from
Detect in the previous commit, asserting both the keep-filter and
the still-emits-other-features path.
Coverage in pkg/markdown/flavor stays at 100%; mdsmith check and
golangci-lint are clean.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Address recall-mode review findings (14 of 15)
Acts on every concrete finding from the local max-effort review pass
except #5 (linkRefResetter interface duplication), which I'm keeping
local: exposing it from pkg/markdown would surface a goldmark fork
detail on the public surface for less duplication value than it costs.
Bugs
- fix.go (#1): MDS034 Fix was calling flavor.NewPooledParser() per
invocation — same regression I fixed in flavor.Detect last commit,
mirrored at the parallel Fix call site. Move the pool to package
level in flavor and expose a callback API, flavor.WithSharedParser,
used by both Detect's dualFindings and the rule's
fixByteRangeFeatures.
- rule.go fixGitHubAlerts (#7): the type assertion
bq.FirstChild().(*ast.Paragraph) plus lines.At(0) was relying on a
cross-package contract with flavor.IsGitHubAlert. Re-check the
shape locally so a future relax of IsGitHubAlert cannot turn the
walk into a panic.
- fix.go taskCheckBoxEdits (#9): nil-check the
flavor.NearestBlockAncestor return and the block's Lines() before
calling At(0).
- detect.go (#11, #15): IsGitHubAlert nil-guards bq and the
paragraph's Lines.Len(); findHeadingID nil-guards h. Both are
public-API entry points now.
- rule.go ApplySettings (#12): iterate settings keys in sorted order
so the error for multiple unknown settings is deterministic across
Go map randomisation.
Simplifications
- detect.go (#2): drop the taskCheckBoxFinding specialisation; the
TaskCheckBox case in builtinFindingFor calls inlineExtFinding
directly.
- detect.go (#3): promote isGitHubAlert / lineCol /
nearestBlockAncestor to the exported names. The previous private +
one-line public-wrapper pair was duplication; the lowercase
versions are now gone.
- detect.go dualFindings (#4): return nil rather than an
always-allocated empty slice on no findings (CLAUDE.md
allocation-budget rule).
- parser.go (#6): drop NewParser and NewParserWith — they were
one-line wrappers around the pooled forms. The public surface is
now NewPooledParser, NewPooledParserWith, and WithSharedParser.
- contract_test.go (#13): move signature pins to package-scope `var
_ = ...` declarations so the staticcheck "could omit type" rule
does not fight the explicit-type contract.
Reuse
- pkg/markdown.Edit + Splice (#8): added an optional `Repl []byte`
field to Edit; Splice now supports replacement in addition to
deletion. The rule's bespoke `edit` struct and `applyEdits` are
gone; fix.go composes a []markdown.Edit and feeds it through
markdown.Splice. Adjacent-edits-with-Repl behaviour is now pinned
in pkg/markdown's TestSplice.
- lint.UnmarshalFrontMatter (#14): extracted the StripFrontMatter →
trim `---\n` delimiters → yamlutil.UnmarshalSafe pipeline into one
helper in internal/lint/frontmatter.go. internal/integration's
rules_test.go switched to it, dropping its open-coded copy and the
goldmark-frontmatter import.
Test pyramid
- Added unit tests for IsGitHubAlert's nil/empty-Lines branches,
FindHeadingID's nil-heading branch, and TestWithSharedParser for
the new pool callback. Coverage in pkg/markdown/flavor is 100%.
- TestApplyEditsHandlesAdjacentEdits moved into pkg/markdown's
TestSplice as a sub-test covering the new Repl behaviour.
All tests pass, golangci-lint clean, mdsmith check clean.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Refresh bench comment after pool move
The pool moved from inside Detect to package level in commit 08446eb
(WithSharedParser). Update the bench's docstring to point at the new
home so the next reader does not look in the wrong file.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Address second-round recall review (F1, F3, F4, F5, F6)
Second focused review of the previous fix-up commit surfaced seven
findings. Fixing five; F2 (atomic ApplySettings) is pre-existing and
masked by clone-before-apply at every call site, F7 (recover boundary
in WithSharedParser) is hypothetical with no current trigger.
F1 + F4: lint.UnmarshalFrontMatter conflated "no front matter" with
"front matter that decoded into a zero struct". A misspelled key
(dropping a letter from "diagnostics", a schema-mismatched field) or
an empty `---\n---\n` block left fm.Settings == nil && fm.Diagnostics
== nil, so the integration fixture loader silently accepted malformed
bad fixtures.
- UnmarshalFrontMatter now returns (body, hadFrontMatter, err).
Callers that want to enforce schema check hadFrontMatter rather
than inspect v's zero state.
- integration/rules_test.go's parseFixtureFrontMatter uses the
new bool. A bad fixture with malformed FM now fails loudly with
the correct "missing front matter" message.
- Unit tests in lint/frontmatter_test.go pin all four cases (valid
block, no block, empty fences, unrecognised keys, decode error).
F3: markdown.Splice's docstring promised "ascending and
non-overlapping" but the implementation enforced neither — a
violating edit list crashed inside body[prev:e.Start] with an opaque
"slice bounds out of range" panic. Added an entry-point precondition
check that panics with a descriptive message naming the offending
edit's index, Start, and End. Three new sub-tests in TestSpliceInvariantViolation
pin the message text so any future change to the check surfaces here.
F5: taskCheckBoxEdits's comment claimed "a malformed AST cannot
panic the fix". Technically true after the nil/empty-Lines guards
landed in commit 08446eb, but the guards do NOT close the
silent-corruption case where NearestBlockAncestor skips an
empty-Lines TextBlock and returns the enclosing ListItem — start+3
then deletes the bullet instead of the checkbox. Documented the
goldmark TextBlock invariant the fix relies on and the failure mode
when a hand-built AST violates it.
F6: fixGitHubAlerts's local (Paragraph, non-empty Lines) re-check was
dead code today and would silently skip a fix if flavor.IsGitHubAlert
ever drifted — the worst failure mode for a fix path (diagnostic
flagged but fix did nothing). Removed the local check; the comment
documents why trusting IsGitHubAlert is the right call and which
test pins the contract.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Close the F5 silent-corruption path in taskCheckBoxEdits
The previous round's nil/empty-Lines guards did NOT prevent the
silent-corruption case the F5 finding pointed at: NearestBlockAncestor
SKIPS ancestors with empty Lines() and keeps walking up, so a
TaskCheckBox under a Paragraph-with-empty-Lines under a
ListItem-with-populated-Lines yields block=ListItem and start =
bullet-position, not '['-position. The guards both pass; start+3
deletes three bytes from the wrong block.
Add an explicit `f.Source[start] != '['` check that declines the
edit when the byte at Lines.At(0).Start is not the bracket the
task-list parser's invariant promises, plus a bounds check on
start+3 against len(f.Source) for the truly-short-source case.
Three new red/green tests pin the guard:
- non-bracket start (paragraph with arbitrary Lines)
- nil block ancestor (orphan TaskCheckBox)
- bracket runs past EOF (2-byte source)
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Address round-3 review: Splice negative-Start, IsGitHubAlert contract pin
Recall agent surfaced three findings after commit 17cca70; one was
the F5 silent-corruption path already closed in commit 131a9f1. The
remaining two land here.
#2: markdown.Splice's precondition check fired for negative-Start
edits via the generic "overlaps previous edit ending at 0" panic. A
producer that subtracts past 0 and emits Start=-1 sent the debugger
chasing a non-existent previous edit. Add a dedicated `Start < 0`
guard that names the actual fault, plus a pin in
TestSpliceInvariantViolation.
#3: fixGitHubAlerts removed the local (Paragraph, non-empty Lines)
re-check and trusts flavor.IsGitHubAlert's contract — but no
behavior test pinned that contract. The four existing fix tests
exercise inputs where IsGitHubAlert returns true via the real
parser; they would not catch a future relaxation of IsGitHubAlert
that returns true for a non-Paragraph first child. Add
TestIsGitHubAlertContractPostcondition: walk a corpus of alert /
non-alert / degenerate blockquotes, and on every IsGitHubAlert==true
case assert FirstChild is *ast.Paragraph with non-empty Lines.
Coverage in pkg/markdown and pkg/markdown/flavor stays at 100%;
mdsmith check and golangci-lint clean.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
* Mark plan/185 done
Every task and acceptance criterion is checked off and the
implementation has passed three rounds of recall review with all
follow-up fixes landed. Flip the status from 🔳 to ✅ and refresh
the PLAN.md catalog.
https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent db3a705 commit df4fcfa
46 files changed
Lines changed: 1884 additions & 773 deletions
File tree
- docs/development
- architecture
- internal
- convention
- integration
- lint
- rules/markdownflavor
- schema
- pkg/markdown
- flavor
- ext
- plan
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
111 | 111 | | |
112 | 112 | | |
113 | 113 | | |
114 | | - | |
| 114 | + | |
115 | 115 | | |
116 | 116 | | |
117 | 117 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
244 | 244 | | |
245 | 245 | | |
246 | 246 | | |
247 | | - | |
248 | | - | |
249 | | - | |
| 247 | + | |
| 248 | + | |
250 | 249 | | |
251 | | - | |
252 | | - | |
253 | | - | |
254 | | - | |
255 | | - | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
256 | 255 | | |
257 | 256 | | |
258 | 257 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
48 | 49 | | |
49 | 50 | | |
50 | 51 | | |
| |||
166 | 167 | | |
167 | 168 | | |
168 | 169 | | |
169 | | - | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
170 | 179 | | |
171 | 180 | | |
172 | 181 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
52 | 52 | | |
53 | 53 | | |
54 | 54 | | |
55 | | - | |
56 | | - | |
57 | | - | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
97 | 97 | | |
98 | 98 | | |
99 | 99 | | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
100 | 113 | | |
101 | 114 | | |
102 | | - | |
| 115 | + | |
| 116 | + | |
103 | 117 | | |
104 | | - | |
105 | | - | |
106 | | - | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
107 | 129 | | |
108 | 130 | | |
109 | 131 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
75 | 75 | | |
76 | 76 | | |
77 | 77 | | |
78 | | - | |
79 | | - | |
80 | | - | |
81 | | - | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
82 | 89 | | |
83 | 90 | | |
84 | 91 | | |
| |||
213 | 220 | | |
214 | 221 | | |
215 | 222 | | |
216 | | - | |
| 223 | + | |
217 | 224 | | |
218 | 225 | | |
219 | 226 | | |
220 | 227 | | |
221 | 228 | | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
222 | 245 | | |
223 | 246 | | |
224 | 247 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
2 | | - | |
3 | | - | |
4 | | - | |
5 | | - | |
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
6 | 7 | | |
7 | 8 | | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
8 | 13 | | |
9 | 14 | | |
10 | | - | |
11 | | - | |
| 15 | + | |
12 | 16 | | |
13 | | - | |
14 | | - | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
15 | 26 | | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
20 | | - | |
21 | | - | |
22 | | - | |
23 | | - | |
24 | | - | |
25 | | - | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
45 | 35 | | |
46 | 36 | | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | | - | |
77 | | - | |
78 | | - | |
79 | | - | |
80 | | - | |
| 37 | + | |
| 38 | + | |
81 | 39 | | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | | - | |
88 | | - | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
| 40 | + | |
101 | 41 | | |
0 commit comments