Skip to content

Commit b011847

Browse files
committed
plan 50: address Copilot review on PR #154
- resolveCorpus: handle relative f.Path correctly. In CLI runs ResolveFiles returns './docs/a.md' while RootDir is absolute, so filepath.Rel would fail and the rule quietly fell back to FS-only scope. Split rootRelative() out: absolute paths go through Rel, relative ones are treated as root-relative, and '..' traversal rejects either. - matchesFilters: match include/exclude globs against both the full slash path and basename, matching MDS027's semantics so patterns like 'draft.md' work regardless of directory depth. - README: fix 'Default: enabled' to 'disabled (opt-in via .mdsmith.yml)'. - Fixtures: drop the self-referential 'appears in ref/source.md' wording; neutral phrasing works in both duplicate.md and source.md. - Tests: add coverage for relative f.Path under absolute RootDir, basename-only exclude patterns across directories, and '..' traversal falling through to FS scope.
1 parent 0de3993 commit b011847

5 files changed

Lines changed: 146 additions & 26 deletions

File tree

internal/rules/MDS037-duplicated-content/README.md

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@ Paragraphs should not repeat verbatim across Markdown files.
1111
- **ID**: MDS037
1212
- **Name**: `duplicated-content`
1313
- **Status**: ready
14-
- **Default**: enabled, include: [], exclude: [], min-chars: 200
14+
- **Default**: disabled (opt-in via `.mdsmith.yml`);
15+
include: [], exclude: [], min-chars: 200
1516
- **Fixable**: no
1617
- **Implementation**:
1718
[source](./)
@@ -91,11 +92,12 @@ wrap: markdown
9192
```markdown
9293
# Duplicate Fixture
9394

94-
This fixture deliberately repeats a distinctive paragraph that also
95-
appears in ref/source.md, so MDS037 must report a diagnostic pointing
96-
back at the other file. The wording must cross the default two-hundred
97-
character threshold and stay unique relative to other rule fixtures so
98-
nothing else matches by accident.
95+
A distinctive paragraph appears in this file and in a sibling
96+
fixture, so MDS037 must flag the match and point at the other
97+
location. The wording stays above the default two-hundred character
98+
threshold after normalization. It stays unique relative to the
99+
other rule fixtures so nothing matches by accident across the test
100+
suite.
99101
```
100102

101103
<?/include?>
@@ -110,11 +112,12 @@ wrap: markdown
110112
```markdown
111113
# Source Fixture
112114

113-
This fixture deliberately repeats a distinctive paragraph that also
114-
appears in ref/source.md, so MDS037 must report a diagnostic pointing
115-
back at the other file. The wording must cross the default two-hundred
116-
character threshold and stay unique relative to other rule fixtures so
117-
nothing else matches by accident.
115+
A distinctive paragraph appears in this file and in a sibling
116+
fixture, so MDS037 must flag the match and point at the other
117+
location. The wording stays above the default two-hundred character
118+
threshold after normalization. It stays unique relative to the
119+
other rule fixtures so nothing matches by accident across the test
120+
suite.
118121
```
119122

120123
<?/include?>

internal/rules/MDS037-duplicated-content/bad/duplicate.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ diagnostics:
66
---
77
# Duplicate Fixture
88

9-
This fixture deliberately repeats a distinctive paragraph that also
10-
appears in ref/source.md, so MDS037 must report a diagnostic pointing
11-
back at the other file. The wording must cross the default two-hundred
12-
character threshold and stay unique relative to other rule fixtures so
13-
nothing else matches by accident.
9+
A distinctive paragraph appears in this file and in a sibling
10+
fixture, so MDS037 must flag the match and point at the other
11+
location. The wording stays above the default two-hundred character
12+
threshold after normalization. It stays unique relative to the
13+
other rule fixtures so nothing matches by accident across the test
14+
suite.
Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
# Source Fixture
22

3-
This fixture deliberately repeats a distinctive paragraph that also
4-
appears in ref/source.md, so MDS037 must report a diagnostic pointing
5-
back at the other file. The wording must cross the default two-hundred
6-
character threshold and stay unique relative to other rule fixtures so
7-
nothing else matches by accident.
3+
A distinctive paragraph appears in this file and in a sibling
4+
fixture, so MDS037 must flag the match and point at the other
5+
location. The wording stays above the default two-hundred character
6+
threshold after normalization. It stays unique relative to the
7+
other rule fixtures so nothing matches by accident across the test
8+
suite.

internal/rules/duplicatedcontent/rule.go

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,11 +206,17 @@ func runeLen(s string) int {
206206
// file within it. RootFS (the project root) is preferred; otherwise the
207207
// file's own directory is used. The returned selfName is forward-slash,
208208
// fs.FS-style so it can be compared to fs.WalkDir's path argument.
209+
//
210+
// f.Path may be absolute (CLI runs with a discovered root) or relative
211+
// to the project root (ResolveFiles returns things like "./docs/a.md").
212+
// Absolute paths go through filepath.Rel; relative paths are cleaned
213+
// and slashed in place. Either way, a self-path that escapes RootDir
214+
// (starts with "..") falls through to the FS scope rather than
215+
// walking the whole project root behind the user's back.
209216
func resolveCorpus(f *lint.File) (fs.FS, string) {
210217
if f.RootFS != nil && f.RootDir != "" {
211-
rel, err := filepath.Rel(f.RootDir, f.Path)
212-
if err == nil && !strings.HasPrefix(rel, "..") {
213-
return f.RootFS, filepath.ToSlash(rel)
218+
if selfName, ok := rootRelative(f.RootDir, f.Path); ok {
219+
return f.RootFS, selfName
214220
}
215221
}
216222
if f.FS != nil {
@@ -219,6 +225,29 @@ func resolveCorpus(f *lint.File) (fs.FS, string) {
219225
return nil, ""
220226
}
221227

228+
// rootRelative returns path expressed relative to rootDir using forward
229+
// slashes, or ok=false when path escapes rootDir. Already-relative paths
230+
// are assumed to be rooted at rootDir and only cleaned; absolute paths
231+
// go through filepath.Rel.
232+
func rootRelative(rootDir, path string) (string, bool) {
233+
var rel string
234+
if filepath.IsAbs(path) {
235+
r, err := filepath.Rel(rootDir, path)
236+
if err != nil {
237+
return "", false
238+
}
239+
rel = r
240+
} else {
241+
rel = filepath.Clean(path)
242+
}
243+
slash := filepath.ToSlash(rel)
244+
slash = strings.TrimPrefix(slash, "./")
245+
if slash == ".." || strings.HasPrefix(slash, "../") {
246+
return "", false
247+
}
248+
return slash, true
249+
}
250+
222251
// buildCorpusIndex walks corpus for .md files (excluding selfName) and
223252
// returns a map from paragraph fingerprint to every occurrence found.
224253
// Files that can't be read or parsed are silently skipped — this rule is
@@ -282,17 +311,24 @@ func isMarkdownPath(p string) bool {
282311
return strings.HasSuffix(strings.ToLower(p), ".md")
283312
}
284313

314+
// matchesFilters reports whether path is allowed by include/exclude.
315+
// To stay consistent with MDS027 cross-file-reference-integrity,
316+
// patterns are matched against both the full forward-slash path and
317+
// the basename, so `"draft.md"` excludes a file regardless of which
318+
// directory it sits in.
285319
func matchesFilters(path string, include, exclude []glob.Glob) bool {
320+
slashPath := filepath.ToSlash(path)
321+
base := filepath.Base(path)
286322
for _, g := range exclude {
287-
if g.Match(path) {
323+
if g.Match(slashPath) || g.Match(base) {
288324
return false
289325
}
290326
}
291327
if len(include) == 0 {
292328
return true
293329
}
294330
for _, g := range include {
295-
if g.Match(path) {
331+
if g.Match(slashPath) || g.Match(base) {
296332
return true
297333
}
298334
}

internal/rules/duplicatedcontent/rule_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,85 @@ func TestCheck_ConfigDiagOnBadExcludeGlob(t *testing.T) {
261261
assert.Equal(t, lint.Error, diags[0].Severity)
262262
}
263263

264+
func TestCheck_RootFSWithRelativeFilePath(t *testing.T) {
265+
// Mirrors a normal CLI run: RootDir is absolute (from config
266+
// discovery) while f.Path is the relative path returned by
267+
// ResolveFiles (e.g. "./docs/a.md"). resolveCorpus must use the
268+
// RootFS walk rather than silently falling back to the file's
269+
// own directory and missing duplicates elsewhere in the tree.
270+
dir := t.TempDir()
271+
docs := filepath.Join(dir, "docs")
272+
guides := filepath.Join(dir, "guides")
273+
require.NoError(t, os.MkdirAll(docs, 0o755))
274+
require.NoError(t, os.MkdirAll(guides, 0o755))
275+
276+
p := longParagraph("the quick brown fox jumps over the lazy dog")
277+
writeFile(t, filepath.Join(docs, "a.md"), "# A\n\n"+p+"\n")
278+
writeFile(t, filepath.Join(guides, "b.md"), "# B\n\n"+p+"\n")
279+
280+
data, err := os.ReadFile(filepath.Join(docs, "a.md"))
281+
require.NoError(t, err)
282+
// Relative path, as ResolveFiles would return.
283+
relPath := filepath.Join("docs", "a.md")
284+
f, err := lint.NewFile(relPath, data)
285+
require.NoError(t, err)
286+
f.FS = os.DirFS(docs)
287+
f.SetRootDir(dir)
288+
289+
diags := (&Rule{}).Check(f)
290+
require.Len(t, diags, 1)
291+
assert.Contains(t, diags[0].Message, "guides/b.md",
292+
"RootFS walk should have found the duplicate in a different directory")
293+
}
294+
295+
func TestCheck_RootFSRejectsPathEscapingRoot(t *testing.T) {
296+
// A file whose Path sits outside RootDir (via "../" traversal) must
297+
// not scan the entire RootFS; resolveCorpus falls through to FS so
298+
// the walk stays local.
299+
dir := t.TempDir()
300+
sub := filepath.Join(dir, "sub")
301+
outside := filepath.Join(dir, "outside")
302+
require.NoError(t, os.MkdirAll(sub, 0o755))
303+
require.NoError(t, os.MkdirAll(outside, 0o755))
304+
305+
p := longParagraph("the quick brown fox jumps over the lazy dog")
306+
// File under a nested RootDir, but its recorded path escapes via
307+
// "../outside/dup.md" — which filepath.Rel would also flag.
308+
writeFile(t, filepath.Join(outside, "dup.md"), "# Dup\n\n"+p+"\n")
309+
writeFile(t, filepath.Join(sub, "peer.md"), "# Peer\n\n"+p+"\n")
310+
311+
data, err := os.ReadFile(filepath.Join(outside, "dup.md"))
312+
require.NoError(t, err)
313+
// Relative escape path: "../outside/dup.md" against RootDir=sub.
314+
f, err := lint.NewFile(filepath.Join("..", "outside", "dup.md"), data)
315+
require.NoError(t, err)
316+
f.FS = os.DirFS(outside)
317+
f.SetRootDir(sub)
318+
319+
diags := (&Rule{}).Check(f)
320+
// peer.md is under sub/ which is no longer in scope; FS (=outside)
321+
// only holds dup.md itself. No duplicates reported.
322+
assert.Empty(t, diags)
323+
}
324+
325+
func TestCheck_BasenameExcludePatternMatchesAcrossDirs(t *testing.T) {
326+
// Consistent with MDS027: a basename pattern ("draft.md") excludes
327+
// the file regardless of which directory the walker finds it in.
328+
dir := t.TempDir()
329+
sub := filepath.Join(dir, "nested")
330+
require.NoError(t, os.MkdirAll(sub, 0o755))
331+
332+
p := longParagraph("the quick brown fox jumps over the lazy dog")
333+
writeFile(t, filepath.Join(dir, "a.md"), "# A\n\n"+p+"\n")
334+
writeFile(t, filepath.Join(sub, "draft.md"), "# Draft\n\n"+p+"\n")
335+
336+
f := newLintFileWithRoot(t, filepath.Join(dir, "a.md"), dir)
337+
r := &Rule{Exclude: []string{"draft.md"}}
338+
diags := r.Check(f)
339+
assert.Empty(t, diags,
340+
"basename-only exclude pattern should hide nested/draft.md")
341+
}
342+
264343
func TestCheck_FallsBackToFSWhenRootFSMissing(t *testing.T) {
265344
dir := t.TempDir()
266345
p := longParagraph("the quick brown fox jumps over the lazy dog")

0 commit comments

Comments
 (0)