Skip to content

fix(cli): skip non-Markdown files named explicitly on check/fix - #760

Merged
jeduden merged 2 commits into
mainfrom
claude/issue-759-review-eb3226
Jul 25, 2026
Merged

fix(cli): skip non-Markdown files named explicitly on check/fix#760
jeduden merged 2 commits into
mainfrom
claude/issue-759-review-eb3226

Conversation

@jeduden

@jeduden jeduden commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes #759. When a non-Markdown file was named explicitly on the command line, mdsmith check linted it as Markdown and mdsmith fix rewrote it. The directory walk and glob expansion already skip such files; only the explicit-path branch was missing the gate.

The damage was worst on .gitattributes, because mdsmith generates the merge-driver block there — so fix rewrote a file it owns (inserting blank lines around # comment lines it mis-parsed as headings), and the result was not even idempotent-to-clean.

Root cause

internal/lint/files.go routes each positional argument three ways, but only two gated on isMarkdown:

  • directory walk (walkDir) — gated ✅
  • glob expansion (resolveGlob) — gated ✅
  • explicit file path (resolveArg) — added the file unconditionally

Change

Commit 1 — skip the file. Gate the explicit-file branch of resolveArg on isMarkdown, so an explicit path behaves exactly like the walk. Extension is the only signal available at resolve time (.md, .markdown, from the mdpath single-source-of-truth). All check/fix/list/metrics callers share this resolver, so the behavior is uniform.

Commit 2 — warn instead of a silent no-op. Skipping was correct but silent: mdsmith fix .gitattributes resolved to zero files and exited 0 with no output, so an explicit argument that did nothing looked like a clean pass. Added ResolveOpts.OnSkipNonMarkdown, a hook fired only in the explicit-file branch (never for walk/glob filtering, which stays silent by design). check/fix wire it to a stderr warning:

mdsmith: skipping ".gitattributes": not a Markdown file (expected .md, .markdown)

The warning is de-duplicated per path, gated to the text format, and suppressed under --quiet — because check/fix emit their diagnostics (including --format json/sarif) on stderr, so a prose line on that stream would corrupt structured output.

MDS048 is unaffected. The .gitattributes managed-block rule reads .gitattributes from disk itself (bytelimit.ReadFileLimited) while linting a Markdown file; the file never needs to be in the linted set.

Behavior before / after

# before
$ mdsmith check .gitattributes
stats: checked=1 fixed=0 failures=14 unfixed=14
$ mdsmith fix .gitattributes      # rewrites the file, still fails

# after (text)
$ mdsmith fix .gitattributes
mdsmith: skipping ".gitattributes": not a Markdown file (expected .md, .markdown)
$ echo $?      # 0, and the file is byte-for-byte unchanged

# after (machine-readable / quiet — warning suppressed, no stderr noise)
$ mdsmith check -f json .gitattributes   # no warning, clean stream
$ mdsmith check --quiet  .gitattributes  # no warning

A mixed run (check README.md .gitattributes) still lints README.md, drops the non-Markdown path, and warns about it.

Tests

  • Unit (internal/lint/files_test.go): explicit non-Markdown file skipped; mixed args keep only Markdown; OnSkipNonMarkdown fires for the explicit file only, never for a walked entry.
  • Unit (cmd/mdsmith/main_unit_test.go): the warner emits + de-dupes in text mode; returns nil (suppressed) for quiet/json/sarif.
  • e2e (cmd/mdsmith/e2e_nonmarkdown_test.go): fix leaves .gitattributes byte-for-byte unchanged and warns; check exits clean and warns; the warning is absent under --quiet and -f json; a mixed run reports the Markdown file and never anchors a diagnostic to the config file.

Verification

  • go test ./..., go vet, golangci-lint — all clean
  • mdsmith check .checked=563 failures=0
  • Confirmed via probe that removing the guard makes all three e2e tests fail (real regression guards, not tautologies)

Out of scope

The issue's secondary observation (an MDS048 scoped-vs-unscoped block disagreement) is explicitly marked not currently reproducible by the reporter. Not addressed here.

Docs

docs/reference/cli/check.md and fix.md document that non-Markdown paths are skipped, that naming one explicitly prints a warning, and that --quiet/json/sarif suppress it.

🤖 Generated with Claude Code

https://claude.ai/code/session_011giDUosuDeqRDVWgmWyaj5

The directory walk and glob expansion both gate on isMarkdown, so a
non-Markdown file like .gitattributes is skipped when discovered. An
explicitly named path bypassed that gate: resolveArg added the file
unconditionally after the symlink/regular-file checks.

The result was that `mdsmith check .gitattributes` lints a git-config
file as Markdown (parsing `#` comments as ATX headings, scoring comment
prose for readability) and `mdsmith fix .gitattributes` rewrites it —
inserting blank lines around headings, including inside the merge-driver
block mdsmith generates there. The rewrite is not even idempotent-to-
clean, so the file ends up modified and still failing.

Gate the explicit-file branch of resolveArg on isMarkdown, matching
walkDir and resolveGlob. MDS048's .gitattributes managed-block check is
unaffected: it reads .gitattributes from disk itself while linting a
Markdown file, so the file never needs to be in the linted set.

Adds unit coverage (explicit non-Markdown skipped; mixed args keep only
Markdown) and e2e coverage (fix leaves a non-Markdown file byte-for-byte
unchanged; check exits clean; a mixed run still lints the Markdown file).

Fixes #759

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011giDUosuDeqRDVWgmWyaj5
@jeduden
jeduden requested a review from Copilot July 20, 2026 07:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.69%. Comparing base (d4af5d5) to head (a6ed778).
⚠️ Report is 38 commits behind head on main.

Additional details and impacted files
Components Coverage Δ
Go 98.69% <100.00%> (+<0.01%) ⬆️
TypeScript 99.54% <ø> (ø)

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Skipping a directly named non-Markdown path (issue #759) was correct but
silent: `mdsmith check .gitattributes` / `mdsmith fix .gitattributes`
resolved to zero files and exited 0 with no output, so an explicit
argument that did nothing looked like a clean pass.

Add ResolveOpts.OnSkipNonMarkdown, a hook fired only in resolveArg's
explicit-file branch — never for entries filtered out by a directory
walk or glob expansion, where silent filtering is the intended,
pre-existing behavior and the user named the directory or pattern rather
than the file.

check and fix wire the hook to a stderr warning
(`mdsmith: skipping "X": not a Markdown file (expected .md, .markdown)`),
de-duplicated per path. The warning is gated to the text format and
suppressed under --quiet: check and fix emit their diagnostics —
including --format json and --format sarif — on stderr, so a prose line
on that stream would corrupt the structured output.

Adds a unit test for the resolver hook (fires for the explicit file
only, not walked entries), unit tests for the warner (text emits + dedup;
suppressed for quiet/json/sarif), and e2e coverage for the warning being
present in text mode and absent under --quiet and --format json.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011giDUosuDeqRDVWgmWyaj5
@jeduden
jeduden requested a review from Copilot July 25, 2026 19:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jeduden
jeduden marked this pull request as ready for review July 25, 2026 19:11
@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Jul 25, 2026
@jeduden

jeduden commented Jul 25, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Jul 25, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-760-1785006745 alongside #763, #765, #766. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Jul 25, 2026
@jeduden
jeduden merged commit 02fc9f8 into main Jul 25, 2026
36 checks passed
@jeduden

jeduden commented Jul 25, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit 94af540. CI run that validated the merge.

Next: Done — nothing more to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check/fix lint non-Markdown files given as explicit paths, and fix edits .gitattributes

3 participants