refactor(review): address xhigh code-review pass 3 findings on PR #731

Final review pass, three findings:

1. internal/linkgraph/wikilinks.go's isMarkdownName duplicated the
   exact ToLower-instead-of-EqualFold anti-pattern this PR already
   fixed, on the same feature's call path: crossfilereferenceintegrity's
   checkWikilinkAnchor (touched by this PR's earlier commits) resolves
   wikilinks through NewWikilinkIndex's fs.WalkDir callback, which
   called isMarkdownName once per workspace file — the same O(files)
   shape as the duplicatedcontent fix. Migrated to
   mdpath.IsMarkdownPath and removed the now-redundant local test (its
   cases are covered by internal/mdpath/mdpath_test.go).

   Declined as out of scope: a broader sweep of ~10 other unrelated
   hand-rolled .md/.markdown checks across internal/lint, internal/corpus,
   internal/lsp, internal/githooks, and internal/release — none of them
   sit on a call path this PR's 5 original fixes touch, and migrating
   all of them is a separate cleanup beyond "fix the top 5 issues the
   audit found."

2. fieldPatternCache (type + get/put + buildFieldPattern, added in the
   pass-2 commit) sat in the middle of the already-2600-line rule.go,
   unlike this package's existing convention of splitting cohesive
   concerns into their own files (runcache_wiring.go, scope_rules.go).
   Moved to fieldpatterncache.go / fieldpatterncache_test.go.

3. fieldPatternCache.get and .put were only exercised indirectly
   inside one combined test; split into TestFieldPatternCache_Get and
   TestFieldPatternCache_Put, each covering their nil-receiver,
   empty-cache, and populated-cache paths by name.

All fixes verified: go build, go vet, go test ./... (including
-race), golangci-lint (0 issues), gofmt, mdsmith check . (544 files,
0 failures).

Author: claude

internal/linkgraph/wikilinks.go

@@ -10,6 +10,7 @@ import (
 	"github.com/jeduden/mdsmith/pkg/goldmark/ast"
 
 	"github.com/jeduden/mdsmith/internal/lint"
+	"github.com/jeduden/mdsmith/internal/mdpath"
 )
 
 // WikiLink is one parsed Obsidian-style wikilink occurrence.
@@ -216,7 +217,7 @@ func NewWikilinkIndex(root fs.FS) *WikilinkIndex {
 		base := path.Base(p)
 		lcName := strings.ToLower(base)
 		idx.names[lcName] = append(idx.names[lcName], p)
-		if isMarkdownName(base) {
+		if mdpath.IsMarkdownPath(base) {
 			stem := strings.TrimSuffix(base, path.Ext(base))
 			lcStem := strings.ToLower(stem)
 			idx.stems[lcStem] = append(idx.stems[lcStem], p)
@@ -361,7 +362,7 @@ func ResolveWikiLink(root fs.FS, from, target string) (string, bool) {
 		base := path.Base(p)
 		if stemMode {
 			stem := strings.TrimSuffix(base, path.Ext(base))
-			if strings.EqualFold(stem, wantStem) && isMarkdownName(base) {
+			if strings.EqualFold(stem, wantStem) && mdpath.IsMarkdownPath(base) {
 				matches = append(matches, p)
 			}
 			return nil
@@ -407,8 +408,3 @@ func wikilinkSearchKey(target string) (wantName, wantStem string, stemMode bool)
 		return base, "", false
 	}
 }
-
-func isMarkdownName(name string) bool {
-	ext := strings.ToLower(path.Ext(name))
-	return ext == ".md" || ext == ".markdown"
-}

internal/linkgraph/wikilinks_test.go

@@ -491,14 +491,6 @@ func TestWikilinkSearchKey(t *testing.T) {
 	}
 }
 
-func TestIsMarkdownName(t *testing.T) {
-	assert.True(t, isMarkdownName("page.md"))
-	assert.True(t, isMarkdownName("page.MD"))
-	assert.True(t, isMarkdownName("page.markdown"))
-	assert.False(t, isMarkdownName("page.txt"))
-	assert.False(t, isMarkdownName("page"))
-}
-
 func TestSortByDepthThenName(t *testing.T) {
 	// Mixed depths and matching depths exercise both keys of the
 	// sort: shorter paths come first, ties break alphabetically.

internal/rules/requiredstructure/fieldpatterncache.go

@@ -0,0 +1,73 @@
+package requiredstructure
+
+import (
+	"regexp"
+	"strings"
+
+	"github.com/jeduden/mdsmith/internal/fieldinterp"
+)
+
+// fieldPatternCache memoizes buildFieldPattern's compiled regexes by
+// body text, scoped to a single parseSchemaWithRootFS call: within that
+// one call, buildSchemaHeading and collectBodySyncPoints can both hit
+// the same {field} template text (e.g. a repeated table-row pattern),
+// so a call-scoped cache avoids rebuilding an identical NFA.
+//
+// It is deliberately not a package-level var: bodyText is schema-
+// authored Markdown, which churns during interactive editing in
+// mdsmith lsp, unlike the bounded, static config patterns
+// compiledPatterns caches in maxsectionlength and requiredtextpatterns
+// — a process-lifetime cache here would grow with edit history instead
+// of workspace size. parseSchemaWithCache already caches and
+// invalidates the *parsedSchema this produces, so a fresh per-call
+// cache costs nothing extra on the (already cached) common path.
+//
+// The zero value is ready to use and allocates its backing map lazily
+// on the first miss, so a schema with no {field} text costs nothing.
+// A nil *fieldPatternCache is also valid (get/put are no-ops), so
+// callers that don't want caching can pass nil.
+type fieldPatternCache struct {
+	m map[string]*regexp.Regexp
+}
+
+func (c *fieldPatternCache) get(bodyText string) (*regexp.Regexp, bool) {
+	if c == nil {
+		return nil, false
+	}
+	re, ok := c.m[bodyText]
+	return re, ok
+}
+
+func (c *fieldPatternCache) put(bodyText string, re *regexp.Regexp) {
+	if c == nil {
+		return
+	}
+	if c.m == nil {
+		c.m = make(map[string]*regexp.Regexp)
+	}
+	c.m[bodyText] = re
+}
+
+// buildFieldPattern compiles a regex that matches a body line whose
+// {field} placeholders have been replaced by any non-empty run. The
+// pattern is always valid: each part is regexp.QuoteMeta'd and joined
+// with ".+", so regexp.MustCompile never panics here. See
+// fieldPatternCache's doc comment for cache's scope and lifetime.
+func buildFieldPattern(bodyText string, cache *fieldPatternCache) *regexp.Regexp {
+	if re, ok := cache.get(bodyText); ok {
+		return re
+	}
+	parts := fieldinterp.SplitOnFields(bodyText)
+	var patBuf strings.Builder
+	patBuf.WriteString("^")
+	for i, part := range parts {
+		patBuf.WriteString(regexp.QuoteMeta(part))
+		if i < len(parts)-1 {
+			patBuf.WriteString(".+")
+		}
+	}
+	patBuf.WriteString("$")
+	compiled := regexp.MustCompile(patBuf.String())
+	cache.put(bodyText, compiled)
+	return compiled
+}

…iredstructure/fieldpattern_cache_test.go …uiredstructure/fieldpatterncache_test.gointernal/rules/requiredstructure/fieldpattern_cache_test.go renamed to internal/rules/requiredstructure/fieldpatterncache_test.go internal/rules/requiredstructure/fieldpattern_cache_test.go renamed to internal/rules/requiredstructure/fieldpatterncache_test.go

@@ -55,22 +55,39 @@ func TestBuildFieldPattern_CacheHitZeroAllocs(t *testing.T) {
 	}
 }
 
-// TestFieldPatternCache_LazyAllocation confirms the zero-value cache
-// never allocates a backing map until the first put — a schema with no
-// {field} text must not pay for a map that's never populated.
-func TestFieldPatternCache_LazyAllocation(t *testing.T) {
-	var cache fieldPatternCache
-	if cache.m != nil {
-		t.Fatal("zero-value fieldPatternCache must not pre-allocate its map")
+// TestFieldPatternCache_Get covers get's three paths: a nil receiver
+// (the "no cache" contract buildFieldPattern relies on), a miss on a
+// zero-value cache, and a miss must not allocate the backing map — a
+// schema with no {field} text must not pay for a map that's never
+// populated.
+func TestFieldPatternCache_Get(t *testing.T) {
+	var nilCache *fieldPatternCache
+	if _, ok := nilCache.get("anything"); ok {
+		t.Fatal("get on a nil *fieldPatternCache must report a miss")
 	}
+
+	var cache fieldPatternCache
 	if _, ok := cache.get("anything"); ok {
 		t.Fatal("get on an empty cache must report a miss")
 	}
 	if cache.m != nil {
 		t.Fatal("get on a miss must not allocate the backing map")
 	}
+}
+
+// TestFieldPatternCache_Put covers put's three paths: a nil receiver is
+// a no-op (never panics), the first put lazily allocates the backing
+// map, and a put entry is retrievable via get.
+func TestFieldPatternCache_Put(t *testing.T) {
+	var nilCache *fieldPatternCache
+	nilCache.put("key", nil) // must not panic
+
+	var cache fieldPatternCache
 	cache.put("key", nil)
 	if cache.m == nil {
 		t.Fatal("put must lazily allocate the backing map on first use")
 	}
+	if _, ok := cache.get("key"); !ok {
+		t.Fatal("a put entry must be retrievable via get")
+	}
 }

internal/rules/requiredstructure/rule.go

@@ -787,71 +787,6 @@ func resolveBodySyncLine(
 	return patchedLine{}, false
 }
 
-// fieldPatternCache memoizes buildFieldPattern's compiled regexes by
-// body text, scoped to a single parseSchemaWithRootFS call: within that
-// one call, buildSchemaHeading and collectBodySyncPoints can both hit
-// the same {field} template text (e.g. a repeated table-row pattern),
-// so a call-scoped cache avoids rebuilding an identical NFA.
-//
-// It is deliberately not a package-level var: bodyText is schema-
-// authored Markdown, which churns during interactive editing in
-// mdsmith lsp, unlike the bounded, static config patterns
-// compiledPatterns caches in maxsectionlength and requiredtextpatterns
-// — a process-lifetime cache here would grow with edit history instead
-// of workspace size. parseSchemaWithCache already caches and
-// invalidates the *parsedSchema this produces, so a fresh per-call
-// cache costs nothing extra on the (already cached) common path.
-//
-// The zero value is ready to use and allocates its backing map lazily
-// on the first miss, so a schema with no {field} text costs nothing.
-// A nil *fieldPatternCache is also valid (get/put are no-ops), so
-// callers that don't want caching can pass nil.
-type fieldPatternCache struct {
-	m map[string]*regexp.Regexp
-}
-
-func (c *fieldPatternCache) get(bodyText string) (*regexp.Regexp, bool) {
-	if c == nil {
-		return nil, false
-	}
-	re, ok := c.m[bodyText]
-	return re, ok
-}
-
-func (c *fieldPatternCache) put(bodyText string, re *regexp.Regexp) {
-	if c == nil {
-		return
-	}
-	if c.m == nil {
-		c.m = make(map[string]*regexp.Regexp)
-	}
-	c.m[bodyText] = re
-}
-
-// buildFieldPattern compiles a regex that matches a body line whose
-// {field} placeholders have been replaced by any non-empty run. The
-// pattern is always valid: each part is regexp.QuoteMeta'd and joined
-// with ".+", so regexp.MustCompile never panics here. See
-// fieldPatternCache's doc comment for cache's scope and lifetime.
-func buildFieldPattern(bodyText string, cache *fieldPatternCache) *regexp.Regexp {
-	if re, ok := cache.get(bodyText); ok {
-		return re
-	}
-	parts := fieldinterp.SplitOnFields(bodyText)
-	var patBuf strings.Builder
-	patBuf.WriteString("^")
-	for i, part := range parts {
-		patBuf.WriteString(regexp.QuoteMeta(part))
-		if i < len(parts)-1 {
-			patBuf.WriteString(".+")
-		}
-	}
-	patBuf.WriteString("$")
-	compiled := regexp.MustCompile(patBuf.String())
-	cache.put(bodyText, compiled)
-	return compiled
-}
-
 // buildSchemaHeading constructs a schemaHeading from a docHeading,
 // pre-compiling the field-interpolation regex when the text contains
 // {field} references so matchesSchema pays no per-call compile cost.