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

Author: claude

docs/development/markdown-library.md

@@ -230,9 +230,11 @@ The stable surface:
   `Flavor` type and constants, the `Feature`
   type and constants, `AllFeatures`, `Supports`,
   `ParseFlavor`, the `Finding` and
-  `HeadingIDExtra` shapes, `Detect`, the four
-  `NewParser*` / `NewPooledParser*`
-  constructors, and the small rewriter helpers
+  `HeadingIDExtra` shapes, `Detect`, the
+  `NewPooledParser` / `NewPooledParserWith`
+  constructors, the `WithSharedParser` callback
+  for borrowing from the package-shared pool,
+  and the small rewriter helpers
   (`FindHeadingID`, `IsGitHubAlert`, `LineCol`,
   `NearestBlockAncestor`).
 - Sub-package `pkg/markdown/flavor/ext`: the

internal/integration/rules_test.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/jeduden/mdsmith/internal/lint"
 	"github.com/jeduden/mdsmith/internal/rule"
-	"github.com/jeduden/mdsmith/internal/yamlutil"
 
 	_ "github.com/jeduden/mdsmith/internal/rules/ambiguousemphasis"
 	_ "github.com/jeduden/mdsmith/internal/rules/atxheadingwhitespace"
@@ -114,23 +113,17 @@ func parseFixtureFrontMatter(
 ) (map[string]any, []expectedDiag, []byte) {
 	t.Helper()
 
-	prefix, content := lint.StripFrontMatter(data)
-	if prefix == nil {
-		require.False(t, requireDiagnostics, "bad fixture is missing front matter with expected diagnostics")
-		return nil, nil, data
-	}
-
-	// lint.StripFrontMatter returns the prefix including its --- fences;
-	// trim them so the remainder is plain YAML.
-	delim := []byte("---\n")
-	body := bytes.TrimPrefix(prefix, delim)
-	body = bytes.TrimSuffix(body, delim)
-
 	var fm fixtureFrontMatter
-	if err := yamlutil.UnmarshalSafe(body, &fm); err != nil {
+	content, err := lint.UnmarshalFrontMatter(data, &fm)
+	if err != nil {
 		t.Fatalf("decoding front matter: %v", err)
 	}
-
+	if fm.Diagnostics == nil && fm.Settings == nil {
+		// lint.UnmarshalFrontMatter returns the original bytes when no
+		// front matter exists; treat that as "no diagnostics declared".
+		require.False(t, requireDiagnostics, "bad fixture is missing front matter with expected diagnostics")
+		return nil, nil, content
+	}
 	if requireDiagnostics && len(fm.Diagnostics) == 0 {
 		t.Fatal("bad fixture front matter must contain a non-empty diagnostics key")
 	}

internal/lint/frontmatter.go

@@ -17,6 +17,26 @@ func StripFrontMatter(source []byte) (prefix, content []byte) {
 	return markdown.StripFrontMatter(source)
 }
 
+// UnmarshalFrontMatter strips the leading YAML front matter block off
+// source, decodes it into v via yamlutil.UnmarshalSafe, and returns
+// the body with the block removed. body equals source when there is
+// no front matter; in that case v is left untouched and err is nil.
+// Centralises the "---\n" delimiter trim that several call sites
+// were repeating after StripFrontMatter.
+func UnmarshalFrontMatter(source []byte, v any) (body []byte, err error) {
+	prefix, content := markdown.StripFrontMatter(source)
+	if prefix == nil {
+		return content, nil
+	}
+	delim := []byte("---\n")
+	yamlBody := bytes.TrimPrefix(prefix, delim)
+	yamlBody = bytes.TrimSuffix(yamlBody, delim)
+	if err := yamlutil.UnmarshalSafe(yamlBody, v); err != nil {
+		return content, err
+	}
+	return content, nil
+}
+
 // CountLines returns the number of newline-terminated lines in b,
 // forwarded from pkg/markdown.
 func CountLines(b []byte) int {

internal/rules/markdownflavor/fix.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/yuin/goldmark/ast"
 	extast "github.com/yuin/goldmark/extension/ast"
+	gparser "github.com/yuin/goldmark/parser"
 	"github.com/yuin/goldmark/text"
 
 	"github.com/jeduden/mdsmith/internal/lint"
@@ -13,32 +14,24 @@ import (
 	"github.com/jeduden/mdsmith/pkg/markdown/flavor/ext"
 )
 
-// edit describes a single byte-range substitution to apply to source.
-// applyEdits assumes non-overlapping spans and rewrites the buffer in
-// one pass.
-type edit struct {
-	start, end int
-	repl       []byte
-}
-
 // fixByteRangeFeatures collects edits for the six byte-range features
 // (heading IDs, strikethrough, task lists, superscript, subscript, and
 // bare-URL autolinks) and returns the rewritten source. Features that
-// the configured flavor accepts are skipped. The function returns the
-// source unchanged when no edit applies.
+// the configured flavor accepts are skipped. Returns f.Source unchanged
+// when no edit applies.
 func (r *Rule) fixByteRangeFeatures(f *lint.File) []byte {
-	var edits []edit
+	var edits []markdown.Edit
 
 	if r.needsAnyDualFix() {
-		dualParser, reset := flavor.NewPooledParser()
-		defer reset()
-		doc := dualParser.Parse(text.NewReader(f.Source))
-		_ = ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) {
-			if !entering {
+		flavor.WithSharedParser(func(p gparser.Parser) {
+			doc := p.Parse(text.NewReader(f.Source))
+			_ = ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) {
+				if !entering {
+					return ast.WalkContinue, nil
+				}
+				edits = append(edits, r.dualNodeEdits(f, n)...)
 				return ast.WalkContinue, nil
-			}
-			edits = append(edits, r.dualNodeEdits(f, n)...)
-			return ast.WalkContinue, nil
+			})
 		})
 	}
 
@@ -55,7 +48,14 @@ func (r *Rule) fixByteRangeFeatures(f *lint.File) []byte {
 	if len(edits) == 0 {
 		return f.Source
 	}
-	return applyEdits(f.Source, edits)
+	// markdown.Splice expects ascending, non-overlapping edits; the
+	// detection layer never produces overlapping fixes, but a dual-AST
+	// walk and a bare-URL pass merged here can land out of source order,
+	// so sort before handing off.
+	sort.SliceStable(edits, func(i, j int) bool {
+		return edits[i].Start < edits[j].Start
+	})
+	return markdown.Splice(f.Source, edits)
 }
 
 // needsAnyDualFix reports whether any dual-parser fixable feature is
@@ -78,7 +78,7 @@ func (r *Rule) needsAnyDualFix() bool {
 // dualNodeEdits returns the edits to remove an unsupported feature
 // produced from a dual-parser AST node. Returns nil when the node is
 // either supported or not a fixable feature.
-func (r *Rule) dualNodeEdits(f *lint.File, n ast.Node) []edit {
+func (r *Rule) dualNodeEdits(f *lint.File, n ast.Node) []markdown.Edit {
 	switch node := n.(type) {
 	case *ast.Heading:
 		if flavor.Supports(r.Flavor, flavor.FeatureHeadingIDs) {
@@ -112,7 +112,7 @@ func (r *Rule) dualNodeEdits(f *lint.File, n ast.Node) []edit {
 // headingIDEdits returns the edit that drops a "{#id}" attribute block
 // plus any whitespace separating it from the heading text. Returns nil
 // when the heading carries no id attribute.
-func headingIDEdits(f *lint.File, h *ast.Heading) []edit {
+func headingIDEdits(f *lint.File, h *ast.Heading) []markdown.Edit {
 	hx, ok := flavor.FindHeadingID(f.Source, h)
 	if !ok {
 		return nil
@@ -121,7 +121,7 @@ func headingIDEdits(f *lint.File, h *ast.Heading) []edit {
 	for start > 0 && (f.Source[start-1] == ' ' || f.Source[start-1] == '\t') {
 		start--
 	}
-	return []edit{{start: start, end: hx.AttrEnd}}
+	return []markdown.Edit{{Start: start, End: hx.AttrEnd}}
 }
 
 // delimiterPairEdits returns edits removing the opening and closing
@@ -131,62 +131,48 @@ func headingIDEdits(f *lint.File, h *ast.Heading) []edit {
 // markup like `~~*bold*~~` or a softbreak inside the wrapper, where
 // reconstructing each child's own marker span is brittle. The fix
 // declines and the diagnostic remains for the user to resolve.
-func delimiterPairEdits(n ast.Node, markerLen int) []edit {
+func delimiterPairEdits(n ast.Node, markerLen int) []markdown.Edit {
 	t, ok := n.FirstChild().(*ast.Text)
 	if !ok || t.NextSibling() != nil {
 		return nil
 	}
-	return []edit{
-		{start: t.Segment.Start - markerLen, end: t.Segment.Start},
-		{start: t.Segment.Stop, end: t.Segment.Stop + markerLen},
+	return []markdown.Edit{
+		{Start: t.Segment.Start - markerLen, End: t.Segment.Start},
+		{Start: t.Segment.Stop, End: t.Segment.Stop + markerLen},
 	}
 }
 
 // taskCheckBoxEdits removes the "[X]" run plus a single trailing
 // space when present. Per the plan, the bullet itself is preserved.
 // The dual parser places every TaskCheckBox at the start of a
-// TextBlock so block.Lines().At(0).Start always points at '['.
-func taskCheckBoxEdits(f *lint.File, n *extast.TaskCheckBox) []edit {
+// TextBlock so block.Lines().At(0).Start always points at '['; we
+// still guard against a nil block ancestor (degenerate orphan node)
+// and an empty Lines list so a malformed AST cannot panic the fix.
+func taskCheckBoxEdits(f *lint.File, n *extast.TaskCheckBox) []markdown.Edit {
 	block := flavor.NearestBlockAncestor(n)
-	start := block.Lines().At(0).Start
+	if block == nil {
+		return nil
+	}
+	lines := block.Lines()
+	if lines == nil || lines.Len() == 0 {
+		return nil
+	}
+	start := lines.At(0).Start
 	end := start + 3
 	if end < len(f.Source) && f.Source[end] == ' ' {
 		end++
 	}
-	return []edit{{start: start, end: end}}
+	return []markdown.Edit{{Start: start, End: end}}
 }
 
 // wrapBareURL wraps a bare URL in angle brackets so the renderer
 // treats it as a CommonMark autolink. The detector reports a precise
 // span via fin.Start / fin.End.
-func wrapBareURL(source []byte, fin flavor.Finding) edit {
+func wrapBareURL(source []byte, fin flavor.Finding) markdown.Edit {
 	url := source[fin.Start:fin.End]
 	repl := make([]byte, 0, len(url)+2)
 	repl = append(repl, '<')
 	repl = append(repl, url...)
 	repl = append(repl, '>')
-	return edit{start: fin.Start, end: fin.End, repl: repl}
-}
-
-// applyEdits rewrites src by appending unchanged spans and replacement
-// bytes in a single pass. Edits are sorted by ascending start offset;
-// the detection layer never produces overlapping edits for the
-// features we fix, so applyEdits assumes non-overlapping spans.
-func applyEdits(src []byte, edits []edit) []byte {
-	sort.SliceStable(edits, func(i, j int) bool {
-		return edits[i].start < edits[j].start
-	})
-	size := len(src)
-	for _, e := range edits {
-		size += len(e.repl) - (e.end - e.start)
-	}
-	out := make([]byte, 0, size)
-	cursor := 0
-	for _, e := range edits {
-		out = append(out, src[cursor:e.start]...)
-		out = append(out, e.repl...)
-		cursor = e.end
-	}
-	out = append(out, src[cursor:]...)
-	return out
+	return markdown.Edit{Start: fin.Start, End: fin.End, Repl: repl}
 }

internal/rules/markdownflavor/fix_test.go

@@ -174,17 +174,6 @@ func TestRuleDualNodeEditsSupportedFeaturesReturnNil(t *testing.T) {
 	assert.Nil(t, r.dualNodeEdits(nil, &ext.SubscriptNode{}))
 }
 
-// TestApplyEditsHandlesAdjacentEdits guards the single-pass build in
-// applyEdits: adjacent edits (e.g. an opening and closing delimiter
-// of a strikethrough) must compose into a contiguous output without
-// dropping or duplicating bytes between them.
-func TestApplyEditsHandlesAdjacentEdits(t *testing.T) {
-	src := []byte("ab~~xy~~cd")
-	edits := []edit{
-		{start: 2, end: 4},                     // opening "~~"
-		{start: 6, end: 8},                     // closing "~~"
-		{start: 0, end: 0, repl: []byte("> ")}, // pure insertion at start
-	}
-	got := applyEdits(src, edits)
-	assert.Equal(t, "> abxycd", string(got))
-}
+// Splice's single-pass behaviour (adjacent edits, pure insertion,
+// replacement bytes) is exercised in pkg/markdown's TestSplice; the
+// rule layer just feeds edits into markdown.Splice.

internal/rules/markdownflavor/rule.go

@@ -12,6 +12,7 @@ package markdownflavor
 import (
 	"bytes"
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/yuin/goldmark/ast"
@@ -49,9 +50,18 @@ func (r *Rule) Category() string { return "structural" }
 // EnabledByDefault implements rule.Defaultable. MDS034 is opt-in.
 func (r *Rule) EnabledByDefault() bool { return false }
 
-// ApplySettings implements rule.Configurable.
+// ApplySettings implements rule.Configurable. Keys are processed in
+// sorted order so the error reported for multiple unknown settings is
+// deterministic across runs (Go's map iteration order is randomised,
+// which would otherwise produce flaky fixture goldens).
 func (r *Rule) ApplySettings(settings map[string]any) error {
-	for k, v := range settings {
+	keys := make([]string, 0, len(settings))
+	for k := range settings {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for _, k := range keys {
+		v := settings[k]
 		switch k {
 		case "flavor":
 			s, ok := v.(string)
@@ -160,8 +170,18 @@ func (r *Rule) fixGitHubAlerts(f *lint.File) []byte {
 		if !flavor.IsGitHubAlert(bq, f.Source) {
 			return ast.WalkContinue, nil
 		}
-		para := bq.FirstChild().(*ast.Paragraph)
+		// flavor.IsGitHubAlert validates the (Paragraph, non-empty Lines)
+		// shape, but the assertion + At(0) couple us to that contract
+		// across a package boundary. Re-check locally so a future relax
+		// of IsGitHubAlert cannot turn this walk into a panic.
+		para, ok := bq.FirstChild().(*ast.Paragraph)
+		if !ok {
+			return ast.WalkContinue, nil
+		}
 		lines := para.Lines()
+		if lines == nil || lines.Len() == 0 {
+			return ast.WalkContinue, nil
+		}
 		seg := lines.At(0)
 		markerLine, _ := flavor.LineCol(f.Source, seg.Start)
 		skip[markerLine] = true