feat: mds fmt auto-formatter

[dean0x]

Summary

Implements mds fmt — an opinionated, safety-gated auto-formatter for MDS templates.

Closes #60

Engine (mds-core): format_str / format_str_with

A span-guided, line-oriented source rewriter — never a token/AST reconstructor, since the
lexer is lossy (interpolation inner whitespace trimmed, the newline after a directive line /
code fence discarded, frontmatter \r filtered) and reconstructing from tokens would invent or
drop whitespace the author actually wrote. Tokenizes only to locate protected byte-ranges and
directive-line offsets, then rewrites the original source string in a single left-to-right pass.

Ruleset (v1 — R1-R4):

Rule	What	Why it's safe
R1	CRLF/CR → LF, everywhere, including inside frontmatter/code fences	clean_output strips every \r from the final compiled string unconditionally, frontmatter lexing already filters \r, and directive matching trims \r before comparison — no downstream consumer can ever observe it
R2	Exactly one final newline; empty/whitespace-only input → ""	Matches clean_output's own leading/trailing-trim (verified against the live compiler)
R3	Runs of 3+ blank lines collapse to one; leading blank lines in the body are removed entirely; never inside a protected region	Mirrors clean_output's own newline-run cap exactly — verified empirically, not just inferred from its doc comment
R4	Trailing whitespace stripped on directive lines	The parser calls dir.trim() before matching, so trailing whitespace is discarded pre-parse and never reaches output

Two things were discovered empirically (not anticipated in the plan) and reshaped the design:

1. A whitespace-only "blank" line is NOT inert mid-document. clean_output's per-character
   loop treats a bare space as ordinary content (resets its newline-run counter, pushed through
   verbatim) — only a truly empty line participates in run-collapsing, and only the absolute
   start/end of the document get unconditional stripping. printf 'Hello\n \nWorld\n' | mds build -
   preserves the three spaces byte-for-byte. Stripping such a line in the formatter would silently
   break compile-equivalence, so this formatter leaves body-text trailing whitespace — including
   whitespace-only "blank" lines — completely untouched, everywhere.
2. @message/@define bodies bypass clean_output entirely. Message content is built via
   evaluate_nodes(...).trim() with no clean_output pass (confirmed by reading collect_single_message
   in evaluator.rs and reproducing it: a \r inside a message body survives verbatim into the
   compiled JSON, and an uncollapsed 4-newline run stays uncollapsed). These bodies — and, conservatively,
   @define bodies too (a function can be called from a markdown-mode site or a message-mode site,
   and the formatter can't tell which without a full call-graph analysis) — get a third "raw content"
   region alongside frontmatter/code fences: copied byte-for-byte, including any nested code fence,
   with only directive lines (R4) still safe to trim inside them.

Safety gate (assert_equivalent): re-compiles both the original and formatted source with the
same base_dir and requires identical CompiledOutput. When the original doesn't compile standalone
(a minority case — typically an undefined runtime variable; imports still resolve via base_dir),
falls back to a structural, rule-aware token comparison (Text compared via clean_output itself,
outside raw-content spans; byte-exact inside them). Any divergence returns MdsError::FormatterInvariant
rather than silently writing a wrong result — the CLI never writes on this error.

Deferred to a future version: interpolation { x } → {x} trimming, frontmatter key sorting,
@import grouping/reordering, blank-line insertion around directive blocks, body hard-break
normalization, directive internal spacing.

CLI (mds-cli): mds fmt [FILE|DIR|-] [--check] [--diff]

• File / directory (recursive, including _-prefixed partials — formatting rewrites source,
  not compiled output, so partials are just as much a candidate as anything else) / stdin (-, as
  a filter) / omit to auto-detect.
• Writes only when content changes (preserves mtime on unchanged files).
• --check: read-only, exits non-zero if anything would change. --diff: read-only, prints a
  unified diff via the similar crate (colorized with raw ANSI only when stdout is a TTY, matching
  watch.rs's own TTY-gated ANSI convention — no color crate dependency). They combine: --diff
  controls what's printed, --check controls the exit code.
• Channel discipline: diff / stdin-filter content → stdout; all status, summaries, errors → stderr;
  --quiet suppresses status but never errors.
• mds fmt - into a closed pipe handles a broken pipe gracefully (no panic).
• Directory-mode summary: N formatted, M unchanged, K failed (N would reformat, K failed under
  --check), continue-on-error, matching the run_check_directory precedent.
• FmtConfig (mds.json → {"fmt": {"sort_frontmatter_keys": true}}) is added as forward-compat
  scaffolding only — validated on load, not yet consulted by any formatting behavior (no matching
  CLI flag; a no-op flag would be a UX/clippy liability).

Exit codes (reuses the existing mapping — no new codes)

Code	Meaning
0	Formatted OK / nothing to change / diff preview
1	--check found something that would change, or a format/parse error (incl. FormatterInvariant)
2	File not found / not .mds / I/O / bad UTF-8
3	Oversized source

Test summary

• Engine: 41 integration tests (crates/mds-core/tests/fmt.rs) + 12 white-box unit tests in
  formatter.rs — idempotence and compile-equivalence across a corpus, every rule individually,
  the safety gate (both the real-compile path and the structural fallback), the @message/@define
  raw-content exemption (including a nested-code-fence-inside-a-message-body regression test), and
  edge cases (empty/whitespace-only, missing trailing newline, frontmatter-only, mid-directive EOF,
  unclosed fence/interpolation/frontmatter, UTF-8 BOM, multi-byte UTF-8, mixed CRLF/LF, a ~1MB
  linear-time perf test). cargo test -p mds-core: 638 lib tests + 41 fmt.rs + 60 api_surface.rs,
  all green (was 626/59 before this PR).
• CLI: 34 integration tests (crates/mds-cli/tests/cli_fmt.rs) + 6 exit-code rows added to
  cli_commands.rs + 6 unit tests in fmt.rs — every behavior above, including mtime preservation,
  channel-separation assertions, symlink rejection, oversized/bad-UTF8/non-.mds handling, and a
  partial+parent round-trip proving a formatted partial still compiles its parent to unchanged output.
• Full workspace: cargo test --workspace (1300+ tests, 0 failures), cargo fmt --all --check,
  cargo clippy --workspace --all-targets -- -D warnings (zero warnings, including the wasm/napi/python
  binding crates — none of their public surfaces were touched), cargo check -p mds-core -p mds-cli -p mds-python, node scripts/verify-versions.mjs (unchanged — no version bump for a feature PR).
• Manual smoke tests (both from the plan, reproduced exactly):
  • printf 'a \r\n\n\n\nb\n' | mds fmt - → a \n\nb\n (CRLF→LF, blank-collapse, body two-space
    break preserved)
  • mds fmt --check --diff crates/mds-cli/tests/fixtures/fmt_unformatted.mds → diff on stdout,
    exit 1
• Security: Snyk snyk_code_scan on the new/modified Rust — 0 issues on the first pass. The
  similar dependency (3.1.1, Apache-2.0, MSRV 1.85 ≤ workspace's 1.88, released 2026-05-23 — past
  the repo's 30-day soak policy) was confirmed via cargo tree to compile with zero transitive
  dependencies (a bstr entry appears in Cargo.lock's full resolution graph but is feature-gated
  off and never actually compiled or linked — verified via cargo tree --all-features and checking
  target/ for build artifacts). Snyk's SCA tool doesn't support Cargo/Rust package-manager
  detection in this environment, so the dependency review was done manually rather than via
  snyk_sca_scan.

Deviations from the plan (with justification)

1. Blank-line whitespace stripping (part of R4 as originally described) was not implemented.
   Verified unsound against the live compiler (see above) — stripping it would silently violate
   compile-equivalence, which the plan itself states is the non-negotiable, overriding constraint.
   The one concrete test case in the plan (" \n" whole-document input) is still covered, via R2.
2. A third region category (raw_content_spans) was added for @message/@define bodies,
   beyond the plan's literal "protected = Frontmatter* ∪ Code*" definition — required once the
   clean_output-bypass was discovered; otherwise R1/R3 would silently break message JSON content.
3. rewrite's internal signature takes a body_start: usize (and the new raw_content
   spans) beyond the plan's literal 3-argument sketch — needed to correctly distinguish "leading
   frontmatter" from "a leading code fence with no frontmatter" for the leading-blank-elision rule.
   All of these are private, non-public-API functions.
4. R3's precise threshold: the plan's prose says "3+ blank lines" but its own parenthetical
   ("clean_output collapses 3+\n→2") describes what's actually a 2+-blank-line threshold. This PR
   implements a direct, empirically-verified mirror of clean_output's real per-character algorithm
   (documented in formatter.rs's module docs) rather than a hand-derived "3+" reading — this is a
   precision fix per the plan's own instruction to verify claims against live code, not a behavior
   gap (it's a superset of what the literal "3+" wording would require, so AC-EF-5 still holds).
5. MdsError::FormatterInvariant reachability is covered structurally (the variant exists, has
   the right diagnostic code, is exercised via mds_error_variants_exist / a dedicated
   formatter_invariant_has_diagnostic_code test) rather than via a forced-bad-transform test hook,
   per the plan's own fallback allowance ("if not cleanly feasible, cover it structurally and say so").
6. Fixtures: the five named fixtures (fmt_unformatted.mds, fmt_formatted.mds,
   _fmt_partial.mds, fmt_body_hardbreak.mds, fmt_malformed.mds) live only under
   crates/mds-cli/tests/fixtures/ (consumed by the CLI's subprocess-based tests). The engine's
   "corpus" tests use inline string constants instead of on-disk fixtures, since mds-core has no
   existing fixture-directory corpus walker to match, and inline strings avoid fragile cross-crate
   fixture-path coupling.

Reviewer focus areas

• crates/mds-core/src/formatter.rs's module docs and the raw_content_spans / rewrite_body
  priority ordering (directive > raw-content > protected > ordinary) — this is the trickiest part
  of the engine.
• The exact CLI channel-discipline and exit-code semantics in crates/mds-cli/src/fmt.rs,
  especially how --check and --diff combine.

[dean0x]

🔴 Consistency Issue: FormatterInvariant.detail should be message

File: crates/mds-core/src/error.rs:345
Confidence: 82%

Every other MdsError variant that carries a single free-form explanatory string names that field message — Syntax, BuiltinError, Io, ResourceLimit, YamlError, JsonError, ImportError, ExportError, Extends (9 variants). FormatterInvariant { detail: String } is the lone deviation.

This is public API and the field name is baked into the surface. Since we're still in the pre-release window and this change is free now (no published users), renaming to message removes this inconsistency:

FormatterInvariant { message: String },

After release, this becomes a breaking change. Recommend fixing before shipping.

[dean0x]

🔴 Testing: Safety Gate Failure Paths Never Exercised

File: crates/mds-core/src/formatter.rs:515-606
Confidence: 88% (HIGH)

The safety gate is the core invariant that makes this feature safe to ship. However, no test ever makes it fire:

• assert_equivalent path 1 (divergent output): untested
• assert_equivalent path 3 (structural mismatch): untested
• structural_equivalent itself: zero direct unit tests

The suite has only one indirect integration test that happens to avoid the subtlest branches (no raw-span, no code fence). This means a refactor that made assert_equivalent unconditionally return Ok would pass the entire suite — the highest-value regression this feature can suffer.

Fix: Add direct unit tests for structural_equivalent covering:

1. A matching pair with a raw-content span present
2. A diverging pair inside a raw span returning false
3. A diverging pair outside a raw span

Plus one integration test forcing FormatterInvariant with a golden pair where outputs genuinely differ.

[dean0x]

🟡 Consistency: MdsError::FormatterInvariant.detail should be message

File: crates/mds-core/src/error.rs:345
Confidence: 82%

Every other MdsError variant with a single free-form string names it message — Syntax, BuiltinError, Io, ResourceLimit, YamlError, JsonError, ImportError, ExportError, Extends. FormatterInvariant { detail: String } is the lone deviation.

This is public API (baked into the enum surface). The rename is free now pre-release, but becomes breaking after shipping.

Fix:

FormatterInvariant { message: String },

Update the constructor helper and test sites accordingly.

[dean0x]

🟡 Documentation: Blank Line Collapse Threshold is Understated

Files: README.md:175, CHANGELOG.md:16, crates/mds-cli/src/main.rs:80
Confidence: 82%

The docs state "Runs of 3+ blank lines collapse to one." The actual behavior collapses runs of 2 or more blank lines to one (verified in formatter.rs:468 — if newline_run <= 2).

Concretely: a\n\n\nb (two blank lines) formats to a\n\nb (one blank line). Only a single blank line is preserved.

This contradicts the internal module docs (formatter.rs:40-46) which correctly states: "a run of 3+ raw newlines caps to 2 (one blank line survives)."

Fix: Use unambiguous wording in all three locations:

"Collapses runs of two or more consecutive blank lines to a single blank line (mirrors the compiler's own output normalizer)"

or in newline terms:

"3+ consecutive newlines collapse to 2"

[dean0x]

🟡 Documentation: Byte-for-Byte Claim Contradicts CRLF Normalization

Files: README.md:174,184, CHANGELOG.md:15-19
Confidence: 82%

The same document claims:

• Under "What it normalizes": "CRLF → LF, everywhere (including inside frontmatter and code fences)"
• Under "What it deliberately leaves untouched": "The byte-for-byte content of frontmatter, code fences, and @message/@define bodies ... untouched"

These are in direct tension for frontmatter and code fences: R1 strips \r inside those regions (formatter.rs:30-36), so their content is not byte-for-byte preserved. Only @message/@define bodies are truly byte-for-byte (R1/R3 are excluded there).

Fix: Scope the claim to be precise:

"The content of frontmatter and code fences (apart from CRLF→LF normalization), and the fully byte-for-byte content of @message/@define bodies (including their line endings)."

This removes the contradiction and preserves the meaningful raw-content distinction.

[dean0x]

🟡 Complexity: Positional Boolean-Flag Parameters (Transposition Hazard)

File: crates/mds-cli/src/fmt.rs:128,150,199
Confidence: 82%

run_fmt_stdin, run_fmt_file, and run_fmt_directory each take three bare bool parameters in the same order: (check, diff, quiet). Because all three are bool, transposing any two (e.g., diff/quiet) compiles cleanly and silently misbehaves — a maintenance hazard.

Failure scenario: A future edit swaps two bools at one call site; the compiler accepts it and --quiet silently starts controlling --diff.

Fix: Bundle into a struct:

#[derive(Clone, Copy)]
struct FmtFlags { check: bool, diff: bool, quiet: bool }

fn run_fmt_file(path: &Path, flags: FmtFlags) -> Result<()> { ... }

The code already has FmtArgs at fmt.rs:37-42; reuse that pattern here. Named-field access removes the transposition hazard and shrinks all three signatures.

[dean0x]

📋 Summary: Code Review Findings (All Reports)

Overall Assessment: This is an exceptionally well-engineered PR that should be approved after addressing the highlighted items above. The formatter is safe by construction, the test coverage of the happy path is exemplary, and the code quality is consistently high. Regression risk is zero; the reviewer score spans 7-10/10 across all nine review dimensions.

Scores by Dimension

• Regression: 10/10 ✅ (zero removed exports, zero changed signatures, zero regressions)
• Security: 9/10 ✅ (no injection flaws, TOCTOU residual <70% confidence)
• Rust: 9/10 ✅ (memory-safe by construction, UTF-8 proven safe, idiomatic error handling)
• Reliability: 9/10 ✅ (all loops bounded, safety gate unconditional, I/O limits enforced)
• Dependencies: 9/10 ✅ (minimal addition, zero transitive compiled deps, MSRV safe)
• Performance: 8/10 ✅ (main path O(n) verified, quadratic only on cold fallback path)
• Complexity: 8/10 ✅ (well-documented, essential domain complexity not over-abstracted)
• Documentation: 7/10 ⚠️ (strong module-level and API docs; user-facing text has precision gaps)
• Testing: 7/10 ⚠️ (strong behavior-focused happy-path coverage; safety-gate failure paths untested)

High-Confidence Findings (80%+) Requiring Fixes Before Release

1. [Testing 88%] Safety gate failure paths untested — Posted above; top priority
2. [Consistency 82%] FormatterInvariant.detail → message — Posted above; quick win
3. [Documentation 82%] Blank line collapse threshold incorrect — Posted above
4. [Documentation 82%] Byte-for-byte claim contradicts CRLF strip — Posted above

Important-But-Optional Polish (80%+ confidence)

5. [Complexity 82%] Positional bool triple — Posted above; silent transposition hazard
6. [Complexity 80%] run_fmt_directory concern-mixing — Extract per-file work into a helper returning FileOutcome { Changed, Unchanged, Failed }; reduces 85-line function and removes tally-update scatter
7. [Consistency 80%] Directory --check --quiet inconsistency — Drop the changed_count > 0 disjunct in read-only summary gate so it matches single-file/stdin paths
8. [Reliability 80%] Slice-based rewrite char-boundary assertions missing — Add debug_assert!s for body_start, line_start/line_end, and span-ordering invariants (repo rules mandate it; zero-cost in release, documents contract)
9. [Performance 88%] structural_equivalent fallback quadratic via .any() — Replace per-token linear scan with partition_point binary search on sorted spans (O(T·S) → O(T log S))
10. [Testing 82%] Malformed fmt config never tested — Add one test: write mds.json with bad-typed sort_frontmatter_keys and assert mds fmt fails config loading
11. [Testing 97%] Unused fixture fmt_body_hardbreak.mds — Either wire to a CLI test or delete (dead code rule)

Lower-Confidence But Notable Items (60-79%)

• [Testing 85%] Perf test uses wall-clock assertion — Replace with scaling check (two sizes, assert ratio stays linear; tests fallback path too)
• [Testing 80%] Windows @import gate gap — Known follow-up (KB notes it); separate issue OK, not a blocker
• [Testing 85%] Thin adversarial coverage — Deep directive nesting, pathological blank-line runs, BOM+CRLF combo untested
• [Rust 85%] Unified-diff colorizer mis-colors --/++ prefixes — TTY-only cosmetic; key off ChangeTag instead of line prefixes
• [Reliability 72%] Broken-stderr asymmetry — stdout is pipe-safe; stderr status/summary can panic on closed pipe (accepted convention)

Pre-existing / Informational (Not Blocking)

• [Dependencies 95% pre-existing] No cargo-deny/audit in CI — Both RUSTSEC + license policy gaps; recommend adding
• [Consistency 90% pre-existing] build_directory summary ignores --quiet — Documented pre-existing divergence

Recommendation: APPROVE (with conditions)

Merge when the four HIGH-CONFIDENCE findings above are fixed:

1. Add safety-gate failure-path tests
2. Rename FormatterInvariant.detail → message
3. Fix blank-line documentation threshold
4. Fix byte-for-byte contradiction

The remaining 7 polish items are low-risk pre-release refactors worth doing (all touched code is new), but they do not block merge — they can ship as is and be addressed in follow-ups if preferred.

---

Review Method: 11 specialized sub-reviews (testing, performance, consistency, complexity, rust, reliability, security, regression, dependencies, architecture, documentation) read and deduplicated. All findings verified against source code.

Claude Code Review |