Skip to content

Commit b51793c

Browse files
jedudenclaude
andauthored
Suppress diagnostics inside generated sections during fix (#215)
* Suppress diagnostics inside generated sections during fix The Fixer's pre-fix and post-fix CheckRules calls were running on *lint.File values whose GeneratedRanges had never been populated, so filterGeneratedDiags was a no-op. As a result, `mdsmith fix` surfaced diagnostics inside <?catalog?> / <?include?> bodies that `mdsmith check` correctly hid — the same source bytes produced different results depending on which command ran them, which broke the merge queue's pre-merge-commit hook. Why: the runner sets GeneratedRanges before linting (runner.go:108, :201); the Fixer didn't, leaving range-based filtering inert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Propagate parse-time fields onto post-fix lint.File Address PR #215 review: the post-fix CheckRules call also needs StripFrontMatter and MaxInputBytes from the pre-fix lf, not just FrontMatter/LineOffset. Without them, rules that read secondary files (catalog, include, requiredstructure, crossfilereferenceintegrity) or align cross-file coordinates (duplicatedcontent) silently behave differently between the pre-fix and post-fix passes — the same kind of runner/fixer divergence the GeneratedRanges propagation addressed. Extract the post-fix file construction into buildPostFixFile so all parse-time and resolution context lives in one place, and so the next field added to lint.File doesn't get forgotten by one of the call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Wire GitignoreFunc into Fixer for runner parity Address PR #215 review: the catalog rule's resolveGitignore calls f.GetGitignore() to filter glob hits, but the Fixer never set GitignoreFunc on its lint.File. So a catalog directive whose glob matched gitignored files would silently include them when fix regenerated the body, but exclude them when check ran on the same bytes — the same flavor of fix/check divergence the GeneratedRanges and parse-time-field propagations addressed. Mirror engine.Runner: per-dir cached GitignoreMatcher, closure captured on lf in prepareFile, propagated to finalFile in buildPostFixFile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Hydrate per-file context inside applyFixPasses Address PR #215 review (thread on internal/fix/fix.go:200): the parsedFile that fixable rules see inside applyFixPasses was only getting FS/RootDir wired up, so catalog.Fix (calls f.GetGitignore) and include.Fix (uses f.MaxInputBytes) silently produced different post-fix bytes than `mdsmith check` would have validated against. Extract a hydrateLintFile helper that copies onto a freshly-parsed *lint.File the full per-file context the engine.Runner sets: FS / RootFS / RootDir / FrontMatter / LineOffset / StripFrontMatter / MaxInputBytes / GitignoreFunc / GeneratedRanges. Use it in both applyFixPasses (for parsedFile) and buildPostFixFile (for finalFile) so all three lint passes — pre-fix, fix-pass, post-fix — see the same File contract. Also add a test exercising the prepareFile branch where Fixer.RootDir is set, which the GitignoreFunc-wiring tests had been skipping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop defensive error branches in new fix-path code User feedback: don't write error checks for branches that can't be reached. Mirror the pattern at internal/archetype/gensection/ranges.go:59 (\"NewFile never errors with current implementation\"). - buildPostFixFile: drop the lint.NewFile error return; signature becomes plain *lint.File. Caller no longer needs to handle a parse error after fix. - cachedGitignore: drop the filepath.Abs fallback. On the rare error case Abs returns the input string unchanged, which is still a usable cache key for the inputs the fix pipeline passes (filepath.Dir(path) or f.RootDir). Coverage on the new functions is now 100%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix cachedGitignore cache-key correctness Address PR #215 review (thread on internal/fix/fix.go:65): the previous "drop the Abs error fallback" simplification was wrong — filepath.Abs returns "" on failure (not the input string), so on the rare error path every relative dir would collide on the empty cache key and share one matcher across unrelated directories. My preceding comment also misdescribed Abs's error semantics. Fix the right way: don't normalize the cache key at all. lint.NewGitignoreMatcher does its own filepath.Abs internally to root the matcher, so the cache key only needs to be deterministic across calls within a Fix run, which prepareFile already guarantees by passing the same form (filepath.Dir(path) or f.RootDir). Use the dir string verbatim and update the comment to match what the code actually does. Add TestFixer_CachedGitignore_DistinctKeys documenting the cache contract: distinct inputs yield distinct matchers, repeated input hits the cache, empty-string input is its own entry rather than aliasing with everything else. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Make fix-pass probe actually exercise Fix Address PR #215 review (threads on test lines 258 and 309): the fixPassProbeRule was returning a diagnostic on Check call #1, which is the pre-fix engine.CheckRules pass — applyFixPasses then saw an empty diagnostic list on call #2 and never invoked Fix. So the \"validates hydration during applyFixPasses\" assertion was actually just validating hydration during pre/post-fix CheckRules, which the other tests in this file already cover. Rework: - Track Check and Fix snapshots separately. - Trigger the diagnostic on Check call #2 (the applyFixPasses pass) so Fix actually fires, then assert exactly 1 Fix snapshot. - Pin exact phase counts: 3 Check calls, 1 Fix call. - Assert the Fix call's lint.File also has the per-file context hydrated, not just the Check calls — that's the real regression guard for catalog/include rules whose Fix paths consult these fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use filepath.Clean to normalize cachedGitignore key Address PR #215 review (thread on internal/fix/fix.go:64): the raw-key impl created separate cache entries for equivalent forms like "sub" vs "./sub" vs "sub/", undermining the cache when the caller didn't pre-normalize. Switch the cache key to filepath.Clean(dir): it's total (no error path, so no defensive fallback needed), idempotent, and collapses all the syntactic forms filepath.Clean considers equivalent. Don't use filepath.Abs (which is what Runner does for cross-cwd normalization) because Abs has a stdlib error contract that would require a defensive fallback. The Fixer's prepareFile passes the same form for all files in a Fix() call, so the only normalization that matters in practice is the syntactic-equivalence collapse that Clean already provides — and lint.NewGitignoreMatcher does its own filepath.Abs internally to root the matcher correctly. Extend TestFixer_CachedGitignore (now ...KeyContract) to pin the new equivalence-collapse contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 43c6548 commit b51793c

2 files changed

Lines changed: 505 additions & 17 deletions

File tree

internal/fix/fix.go

Lines changed: 78 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"path/filepath"
99
"sort"
1010

11+
"github.com/jeduden/mdsmith/internal/archetype/gensection"
1112
"github.com/jeduden/mdsmith/internal/config"
1213
"github.com/jeduden/mdsmith/internal/engine"
1314
"github.com/jeduden/mdsmith/internal/explain"
@@ -32,6 +33,38 @@ type Fixer struct {
3233
// remaining diagnostic so output formatters can render an
3334
// explanation trailer.
3435
Explain bool
36+
37+
// gitignoreCache caches GitignoreMatchers by directory so the
38+
// matcher tree is walked once per directory across a fix run,
39+
// matching the engine.Runner cache contract that catalog and
40+
// other gitignore-aware rules expect.
41+
gitignoreCache map[string]*lint.GitignoreMatcher
42+
}
43+
44+
// cachedGitignore returns a GitignoreMatcher for the given directory,
45+
// creating and caching it on first use so the matcher tree is walked
46+
// once per (Fixer, dir). Mirrors engine.Runner so the fix path's
47+
// lint.File values give catalog (and any other rule that calls
48+
// f.GetGitignore()) the same matcher the check path would.
49+
//
50+
// The cache key is filepath.Clean(dir). Clean is total (no error
51+
// path) and idempotent, and it collapses equivalent forms like
52+
// "./sub" and "sub" / "sub/" so callers passing the same logical
53+
// directory in slightly different syntactic forms share one cache
54+
// entry. lint.NewGitignoreMatcher canonicalizes its argument
55+
// internally (filepath.Abs) before walking, so the matcher itself is
56+
// correctly rooted even when the cleaned key is still relative.
57+
func (f *Fixer) cachedGitignore(dir string) *lint.GitignoreMatcher {
58+
if f.gitignoreCache == nil {
59+
f.gitignoreCache = make(map[string]*lint.GitignoreMatcher)
60+
}
61+
key := filepath.Clean(dir)
62+
if m, ok := f.gitignoreCache[key]; ok {
63+
return m
64+
}
65+
m := lint.NewGitignoreMatcher(key)
66+
f.gitignoreCache[key] = m
67+
return m
3568
}
3669

3770
// Result holds the outcome of a fix run.
@@ -118,10 +151,11 @@ func (f *Fixer) fixFile(path string) ([]lint.Diagnostic, []lint.Diagnostic, stri
118151
f.logRules(effective)
119152

120153
fixable, settingsErrs := f.fixableRules(effective)
154+
lf.GeneratedRanges = gensection.FindAllGeneratedRanges(lf)
121155
beforeDiags, checkErrs := engine.CheckRules(lf, f.Rules, effective)
122156
errs = append(errs, append(settingsErrs, checkErrs...)...)
123157

124-
current := f.applyFixPasses(path, lf.Source, fixable, dirFS, &errs)
158+
current := f.applyFixPasses(path, lf.Source, fixable, lf, dirFS, &errs)
125159

126160
var modified string
127161
if !bytes.Equal(lf.Source, current) {
@@ -133,16 +167,7 @@ func (f *Fixer) fixFile(path string) ([]lint.Diagnostic, []lint.Diagnostic, stri
133167
modified = path
134168
}
135169

136-
finalFile, err := lint.NewFile(path, current)
137-
if err != nil {
138-
errs = append(errs, fmt.Errorf("parsing %q after fix: %w", path, err))
139-
return beforeDiags, beforeDiags, modified, errs
140-
}
141-
finalFile.FS = dirFS
142-
finalFile.RootFS = lf.RootFS
143-
finalFile.RootDir = lf.RootDir
144-
finalFile.FrontMatter = lf.FrontMatter
145-
finalFile.LineOffset = lf.LineOffset
170+
finalFile := buildPostFixFile(path, current, lf, dirFS)
146171

147172
diags, checkErrs := engine.CheckRules(finalFile, f.Rules, effective)
148173
errs = append(errs, checkErrs...)
@@ -152,9 +177,41 @@ func (f *Fixer) fixFile(path string) ([]lint.Diagnostic, []lint.Diagnostic, stri
152177
return beforeDiags, diags, modified, errs
153178
}
154179

180+
// hydrateLintFile copies onto a freshly-parsed *lint.File the parse-
181+
// time and resolution context that the engine.Runner sets per-file
182+
// (see runner.go ~line 90-108): FS, RootFS/RootDir, FrontMatter,
183+
// LineOffset, StripFrontMatter, MaxInputBytes, GitignoreFunc, and
184+
// GeneratedRanges (recomputed for the parsed bytes). Used by both
185+
// the post-fix CheckRules call and the parsedFile inside each
186+
// applyFixPasses iteration so rules see the same File regardless of
187+
// which Fixer phase invokes them. Without this, fixable rules like
188+
// catalog (consults GetGitignore for glob filtering) and include
189+
// (consults MaxInputBytes for secondary reads) silently produce
190+
// different post-fix bytes than `mdsmith check` would have validated.
191+
func hydrateLintFile(parsed *lint.File, lf *lint.File, dirFS fs.FS) {
192+
parsed.FS = dirFS
193+
parsed.RootFS = lf.RootFS
194+
parsed.RootDir = lf.RootDir
195+
parsed.FrontMatter = lf.FrontMatter
196+
parsed.LineOffset = lf.LineOffset
197+
parsed.StripFrontMatter = lf.StripFrontMatter
198+
parsed.MaxInputBytes = lf.MaxInputBytes
199+
parsed.GitignoreFunc = lf.GitignoreFunc
200+
parsed.GeneratedRanges = gensection.FindAllGeneratedRanges(parsed)
201+
}
202+
203+
// buildPostFixFile parses post-fix bytes and hydrates them with the
204+
// per-file context from lf so the post-fix CheckRules call sees the
205+
// same lint.File the runner would.
206+
func buildPostFixFile(path string, source []byte, lf *lint.File, dirFS fs.FS) *lint.File {
207+
finalFile, _ := lint.NewFile(path, source) // NewFile never errors with current implementation
208+
hydrateLintFile(finalFile, lf, dirFS)
209+
return finalFile
210+
}
211+
155212
// applyFixPasses repeatedly applies fixable rules until the content stabilizes.
156213
func (f *Fixer) applyFixPasses(
157-
path string, source []byte, fixable []rule.FixableRule, dirFS fs.FS, errs *[]error,
214+
path string, source []byte, fixable []rule.FixableRule, lf *lint.File, dirFS fs.FS, errs *[]error,
158215
) []byte {
159216
const maxPasses = 10
160217
current := source
@@ -167,10 +224,7 @@ func (f *Fixer) applyFixPasses(
167224
*errs = append(*errs, fmt.Errorf("parsing %q: %w", path, err))
168225
break
169226
}
170-
parsedFile.FS = dirFS
171-
if f.RootDir != "" {
172-
parsedFile.SetRootDir(f.RootDir)
173-
}
227+
hydrateLintFile(parsedFile, lf, dirFS)
174228

175229
diags := fr.Check(parsedFile)
176230
if len(diags) == 0 {
@@ -220,10 +274,17 @@ func (f *Fixer) prepareFile(path string, source []byte) (*lint.File, fs.FS, []st
220274
return nil, nil, nil, fmt.Errorf("parsing %q: %w", path, err)
221275
}
222276
lf.MaxInputBytes = f.MaxInputBytes
223-
dirFS := os.DirFS(filepath.Dir(path))
277+
dir := filepath.Dir(path)
278+
dirFS := os.DirFS(dir)
224279
lf.FS = dirFS
280+
gitignoreDir := dir
225281
if f.RootDir != "" {
226282
lf.SetRootDir(f.RootDir)
283+
gitignoreDir = f.RootDir
284+
}
285+
gd := gitignoreDir // capture for closure
286+
lf.GitignoreFunc = func() *lint.GitignoreMatcher {
287+
return f.cachedGitignore(gd)
227288
}
228289
kinds, err := lint.ParseFrontMatterKinds(lf.FrontMatter)
229290
if err != nil {

0 commit comments

Comments
 (0)