diff --git a/.devflow/features/mds-cli/KNOWLEDGE.md b/.devflow/features/mds-cli/KNOWLEDGE.md index 3c26741d..bce1b1a4 100644 --- a/.devflow/features/mds-cli/KNOWLEDGE.md +++ b/.devflow/features/mds-cli/KNOWLEDGE.md @@ -14,7 +14,7 @@ referencedFiles: - crates/mds-cli/tests/intrinsic_output.rs - crates/mds-cli/Cargo.toml created: 2026-06-26 -updated: 2026-06-26 +updated: 2026-09-15 --- # MDS CLI (mds-cli) @@ -168,7 +168,7 @@ let exclude_prefix = match &output_base { }; ``` -**AC-M7 path-escape guard** — `output_path_for` has a runtime containment check: if the computed output path somehow escapes `Dir(base)` (e.g. via a malformed strip_prefix result), it falls back to `base/.`. A `debug_assert!(false, ...)` fires in debug builds so tests catch regressions. +**AC-M7 path-escape guard and the flatten report** — both `Dir(_)`-mode oracles now defer to one classifier, `mirror_stem(source, root, d) -> MirroredStem`, whose arms are `Mirrored(path)` (the `strip_prefix` succeeded; the subtree mirror survives) and `Flattened(path)` (`strip_prefix` failed; only the stem survives, joined to the out-dir). `output_base_no_ext` is the silent probe oracle; `output_path_for` is the write oracle and is the **only** site that reports the flattened arm — on stderr, **not** gated on `--quiet`, naming the source, the build root and the flat output. No live caller can reach that arm today (build hands the walker's own prefix back; watch gates event paths on `starts_with(&ctx.root)` and uses canonical keys under a canonical root whose walker skips symlinks), so the message is an invariant report, not user-facing advice. Degenerate stems (`/`, `..`, a bare drive prefix — never a `.mds` file) fall back to the relative name `output`, so `d.join(...)` can never re-root out of the out-dir; the old fallback was `source.as_os_str()`, which was exactly the absolute value that escapes. `output_path_for` keeps its runtime containment check with a `debug_assert!(false, ...)` behind it — deliberately debug-only, because its release fallback is already contained. In `watch.rs`, the ghost-external-dep prune (the vanished-dependency branch of the dir-batch loop, `watch.rs:2858`) skips the output probe via the new `DirWatchState::forget_graph` (graph-only) instead of the full `forget`, so pruning a vanished out-of-root dependency can no longer drop the write-dedup entry of an in-root source that shares its file name. ### Resource limits diff --git a/.devflow/features/mds-fmt/KNOWLEDGE.md b/.devflow/features/mds-fmt/KNOWLEDGE.md index fe69ee58..e39dc8c3 100644 --- a/.devflow/features/mds-fmt/KNOWLEDGE.md +++ b/.devflow/features/mds-fmt/KNOWLEDGE.md @@ -20,7 +20,7 @@ referencedFiles: - crates/mds-cli/src/build.rs - crates/mds-cli/src/watch.rs created: 2026-07-03 -updated: 2026-07-19 +updated: 2026-09-15 --- # mds fmt — Opinionated Safety-Gated Formatter @@ -186,7 +186,7 @@ Notable divergences worth knowing before touching this file: ## Error Handling and Recovery -`MdsError::FormatterInvariant { message: String }` (`error.rs:335-345`, code `mds::formatter_invariant`, constructed via the private `MdsError::formatter_invariant()` helper at `error.rs:686-692`) means **the formatter itself has a bug** — the CLI must never write the file when this occurs. The field name is `message`, matching all other free-form-string variants in `MdsError`. Both `FormatterInvariant` and `Syntax` map to the generic `_ => 1` arm in `exit_code` (`build.rs:372-382`) — no new exit codes were added for `fmt`. The CLI-level contract: `format_str_named` returns `Err`, never a garbled `Ok(String)`, so `fmt.rs` only ever reaches its write call with a value that already passed the gate. +`MdsError::FormatterInvariant { message: String }` (`error.rs:335-345`, code `mds::formatter_invariant`, constructed via the private `MdsError::formatter_invariant()` helper at `error.rs:686-692`) means **the formatter itself has a bug** — the CLI must never write the file when this occurs. The field name is `message`, matching all other free-form-string variants in `MdsError`. Both `FormatterInvariant` and `Syntax` map to the generic `_ => 1` arm in `exit_code` (`build.rs:372-382`) — no new exit codes were added for `fmt`. Since #217, `mds fmt ` on a path that is not valid UTF-8 exits 2 (`mds::io`) like `mds lint` and `mds build`, instead of 1; directory mode is unchanged (per-file failure, exit 1 with the summary). The CLI-level contract: `format_str_named` returns `Err`, never a garbled `Ok(String)`, so `fmt.rs` only ever reaches its write call with a value that already passed the gate. ## Anti-Patterns diff --git a/.devflow/features/mds-lint/KNOWLEDGE.md b/.devflow/features/mds-lint/KNOWLEDGE.md index 6d80906b..4dde7c35 100644 --- a/.devflow/features/mds-lint/KNOWLEDGE.md +++ b/.devflow/features/mds-lint/KNOWLEDGE.md @@ -11,7 +11,7 @@ directories: - crates/mds-python/src - packages/mds/src created: 2026-07-11 -updated: 2026-08-31 +updated: 2026-09-15 --- # mds lint — Static Analysis Engine and Tiered --fix @@ -348,7 +348,7 @@ Source text passed to `NamedSource` uses a different function: `neutralize_sourc - **C1 (U+0080–U+009F) AND U+061C** (both 2-byte UTF-8) → U+00A0 NBSP (2 bytes). U+061C is in the 2-byte branch. - **The other 11 format hazards** (U+200E/U+200F, U+2028/U+2029, U+202A–U+202E, U+2066–U+2069, U+FEFF — all 3-byte) → U+FFFD REPLACEMENT CHARACTER (3 bytes). -The split is implemented via two private predicates: `is_two_byte_format_hazard(ch)` (only U+061C) and `is_three_byte_format_hazard(ch)` (the remaining 11). A `debug_assert_eq!` in `neutralize_source_for_render` catches byte-length violations immediately during development. +The split is implemented via two private predicates: `is_two_byte_format_hazard(ch)` (only U+061C) and `is_three_byte_format_hazard(ch)` (the remaining 11). An unconditional `assert_eq!` (promoted from `debug_assert_eq!`, #220) in `neutralize_source_for_render` catches byte-length violations immediately, in release builds too. ### `named_source_for_render` — The Single NamedSource Builder @@ -491,7 +491,7 @@ LintDiagnostic.fix_removals (FixLineSpan) OR .fix_edits (TextEdit) - **Post-processing a rendered miette frame with any sanitizer** (PF-014): Sanitizing the rendered output escapes miette's own ANSI SGR colour codes into `\u001B[33m` noise on TTYs. CI uses `NO_COLOR=1` and piped stderr so this regression would stay green indefinitely. Pre-sanitize inputs before constructing the `Report`. -- **Putting U+061C in the 3-byte neutralization branch**: U+061C is 2 bytes in UTF-8. Routing it through the 3-byte branch (`U+FFFD`) fires the byte-length `debug_assert_eq!` (13 vs 12 bytes). This was proven, not theorized, during the #176 development. U+061C belongs in `is_two_byte_format_hazard`. +- **Putting U+061C in the 3-byte neutralization branch**: U+061C is 2 bytes in UTF-8. Routing it through the 3-byte branch (`U+FFFD`) fires the byte-length `assert_eq!` (13 vs 12 bytes). This was proven, not theorized, during the #176 development. U+061C belongs in `is_two_byte_format_hazard`. - **Omitting `0xD8` from the fast-path byte scan in `sanitize_with`**: U+061C is encoded as `0xD8 0x9C`. Without `0xD8` in the fast-path, `sanitize_control_chars("a\u{061C}b")` returns `Borrowed` and skips the character entirely. @@ -569,7 +569,7 @@ LintDiagnostic.fix_removals (FixLineSpan) OR .fix_edits (TextEdit) **`packages/mds` prefers the dev WASM artifact**: `packages/mds/src/backend/wasm.ts` resolves to `crates/mds-wasm/pkg/` (the `wasm-pack` dev output) rather than `packages/mds-wasm/dist/node/`. Rebuilding only the `packages/mds-wasm` npm package leaves a STALE backend active, and the cross-surface differential test fails with convincing-looking divergence that isn't a real bug. Always rebuild via `wasm-pack build crates/mds-wasm` when working on WASM output. -**Directory ordering is byte-wise over `/`-normalized paths**: `relative_display` normalizes path separators to `/` via `components().join("/")` before sorting. The fix was declared BREAKING with zero Windows CI executions; it is now covered by a platform-independent ordering test on Ubuntu and a directory case in `packages/mds/__test__/lint.spec.mjs` (runs on windows-latest). The ordering fixture is separator-sensitive by construction (`/` = 0x2F < `[` = 0x5B < `\` = 0x5C) — any separator regression breaks the fixture on Windows. +**Directory ordering is byte-wise over `/`-normalized paths**: `relative_display` normalizes path separators to `/` via `components().join("/")` before sorting. The fix was declared BREAKING with zero Windows CI executions; it is now covered by a platform-independent ordering test on Ubuntu and a directory case in `packages/mds/__test__/lint.spec.mjs` (runs on windows-latest). The ordering fixture is separator-sensitive by construction (`/` = 0x2F < `[` = 0x5B < `\` = 0x5C) — any separator regression breaks the fixture on Windows. Since #217 `relative_display` returns `Result` (Io on strip_prefix failure or a non-UTF-8 component); `run_lint_directory` computes all display paths into `Vec<(PathBuf, String)>` before sorting and fails the run (envelope + exit 2) on the first `Err`; the per-file helpers receive the precomputed `&str` and `LintDirCtx` no longer carries `lint_root`. **`FixOutcome::PartiallyFixed` is silently discarded by `_ => {}`**: `PartiallyFixed` is returned only by `apply_fixes_incremental`. A `_ => {}` wildcard arm compiles clean and discards it without warning. `#[must_use]` does NOT catch this — it fires on a dropped value, not a wildcard arm. Always match `PartiallyFixed` explicitly. diff --git a/.devflow/features/source-map-security/KNOWLEDGE.md b/.devflow/features/source-map-security/KNOWLEDGE.md index 4b85009a..dbd6de1c 100644 --- a/.devflow/features/source-map-security/KNOWLEDGE.md +++ b/.devflow/features/source-map-security/KNOWLEDGE.md @@ -8,7 +8,7 @@ directories: - crates/mds-cli/src - packages/mds/src created: 2026-07-19 -updated: 2026-08-31 +updated: 2026-09-15 --- # Source Map Security and Path Containment @@ -166,7 +166,7 @@ The `Origin.display` field is populated eagerly at module-load time so no absolu - `sources: Vec` — canonical keys; emitted verbatim into SMv3 `sources[]` (byte-identical to ADR-005 contract) - `display_names: Vec` — root-relative display paths; used only for diagnostics, never emitted into `sources[]` -Both `MapBuilder::new(source_name, display_name, source_content)` and `MapBuilder::source_index(file, display, content)` are 3-argument; callers must supply both the canonical key and the display path. A `debug_assert_eq!` enforces strict length parity between the two vecs after every insertion. +Both `MapBuilder::new(source_name, display_name, source_content)` and `MapBuilder::source_index(file, display, content)` are 3-argument; callers must supply both the canonical key and the display path. A `debug_assert_eq!` enforces strict length parity between the two vecs after every insertion; the evaluator's cursor invariant is an unconditional `assert_eq!` since #220. The `sources[]` bytes emitted into produced source maps are **byte-identical** to what they were before R3 — only the diagnostic display path changes. ADR-005 is preserved. @@ -208,7 +208,9 @@ The Windows verbatim lesson is the same on both sides: native backend emits `\\? **Windows verbatim UNC root** (`path_to_unified` fix): `std::fs::canonicalize` on Windows returns verbatim UNC paths (`\\?\C:\proj`). After `replace('\\', "/")` this becomes `//?/C:/proj`, and `normalize_abs` yields components `["?", "C:", "proj", ...]`. But the source path after the same treatment yields `["C:", "proj", ...]`. The prefix `"?"` causes the first-component comparison to fail → containment always fails → EVERY source map entry degrades to its basename. `path_to_unified` now strips `//?/UNC/` then `//?/` before normalizing, so root components match source components. This bug is invisible on Unix CI. -**`source_root()` returns `None` before any `normalize()` call**: `NativeFs::source_root()` returns `None` until at least one `normalize()` or explicit `set_root()` call establishes the project root. The defense-in-depth guard in `resolver.rs` catches this, but external callers that skip `normalize()` and jump straight to `compile_with_deps_opts` will land on the `root = None` branch. +**Empty/non-UTF-8 anchor hazard**: `path_to_unified` is `Option`; `None` root → basename, `None` base → root anchor; `starts_with_comps(x, [])` is vacuously true; root `/` is deliberately still a real root. The `Option` return exists so an unusable anchor can never reach `starts_with_comps` as an empty component list that every path matches — the degradation is chosen at the choke-point (step 6b), not inferred later. + +**`source_root()` returns `None` before any `normalize()` call**: `NativeFs::source_root()` returns `None` until at least one `normalize()` or explicit `set_root()` call establishes the project root. The defense-in-depth guard in `resolver.rs` catches this, but external callers that skip `normalize()` and jump straight to `compile_with_deps_opts` will land on the `root = None` branch — and returns `None` for a non-UTF-8 root (lossy strings are not anchors). **Directory-mode `opts` must be per-file**: In directory mode (`run_build_directory`), each file has a different output directory, so `source_map_base` differs per file. Constructing `opts` as loop-invariant (outside the per-file loop) would give every file the same anchor, producing incorrect relative paths for all but one file. diff --git a/CHANGELOG.md b/CHANGELOG.md index f498cf4c..5ec2e7f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 parent directory is watched non-recursively, so a sibling scratch write by an editor extends a window a real edit has already opened. Either way `npm install` churn or a noisy editor can delay a real edit and the idle tick by up to the cap. +- **`TextEdit::new`, `FixLineSpan::range_inclusive` and `FixLineSpan::range_exclusive` + now panic on a reversed range in every build profile (#220).** The `start <= end` / + `from <= to` precondition was a debug-only assertion; a release caller passing a + reversed range got a value the fix planner later skipped in silence. The rustdoc + `# Panics` sections say so. No in-tree caller constructs a reversed range. ### Fixed @@ -114,6 +119,49 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `crates/mds-python/tests/test_parity.py:201` still says "9 lint rules" — deliberately left as-is here since fixing it would touch the release-surface Python test path; tracked for a later step. +- **Release builds no longer panic on a non-boundary import/extends span offset, and a + missing skeleton block is an internal error instead of silently rendering the base + default (#220).** `attach_import_span` and the `@extends` child-only-blocks check now + compute the underline length through the existing char-boundary-safe helper: a byte + offset that does not land on a UTF-8 character boundary (a compiler defect, not + something a template can cause) yields a zero-length span with the numeric offset and + no source snippet, instead of a `byte index … is not a char boundary` panic in + release. The skeleton-splice walk asserts in every build profile that each `@block` + placeholder has an effective-blocks entry; release builds used to splice the base + default in silence — dropping a child's override — where debug builds panicked. The + three source-map cursor checks in the evaluator and the byte-length check in + `neutralize_source_for_render` are enforced in release now as well; none can be + triggered by template input, only by a defect, and the messages carry no source text. + On the CLI a tripped invariant is a Rust panic (exit code 101); the napi, WASM and + Python bindings convert it to `mds::internal` as before. +- **`mds lint ` fails closed on a path it cannot name (#217).** The directory-mode + `files[].file` key and the sort key are the entry's path relative to the lint root. A + path that is not valid UTF-8, or that is not under the lint root, previously produced + a lossy (U+FFFD) or absolute key silently; it is now `mds::io` (`path is not valid + UTF-8: …` / `path escapes lint root …`) reported before any file is linted — exit 2, + with the analysis-failure envelope under `--format json`. Such a file already exited 2 + as a per-file error; the difference is that the rest of the tree is no longer linted + around it and no lossy key is ever emitted. +- **`mds fmt ` on a path that is not valid UTF-8 exits 2 (#217).** It was a generic + error (exit 1); it is an I/O error (`mds::io`) like `mds lint` and `mds build`. + Directory mode is unchanged (per-file failure, exit 1 with the summary). +- **Source-map `sources[]` anchors fail closed (#217).** An empty project root, or one + that is not valid UTF-8, can no longer make the containment check vacuous: + `NativeFs::source_root()` reports no root for a non-UTF-8 root directory, and the + relativization choke-point treats an unusable root as "not contained" (basename) and + an unusable `source_map_base` as "anchor on the root" — never as an empty prefix every + path matches. No change for any UTF-8 root; not reachable from the CLI or the public + API today (non-UTF-8 entry paths are rejected before compilation) — closed as a latent + hazard. +- **`mds build`/`mds watch` say so when a source is written flat instead of mirrored + (#217).** In `--out-dir` mode a source that is not under the build root is written to + `/.` (contained, unchanged) and now prints `warning: is + outside the build root ; its output is written flat as (…)`, not suppressed + by `--quiet`. No walked source can trigger it; it is a tripwire for a future caller. + Two degenerate fallbacks that could have joined an absolute path into the output + directory now use a fixed relative name. `mds watch` no longer probes output paths for + a vanished out-of-root dependency, which could drop the write-dedup entry of an in-root + source with the same file name. ### Internal diff --git a/SECURITY.md b/SECURITY.md index 0608e375..4b4855ab 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -46,6 +46,11 @@ input. The compiler enforces several defense-in-depth controls: boundary rather than being passed to the OS. - **Non-UTF-8 paths** are rejected at the public API boundary with an explicit error instead of producing corrupted output. +- **Source-map anchors are byte-faithful**: the project root and `source_map_base` + used to decide whether a `sources[]` entry is inside the project are never + derived from a lossy string. A root that is empty or not valid UTF-8 is treated + as "no root" — entries degrade to basenames — so it can never make the + containment check vacuous. - **Replace-by-rename writes**: `mds fmt`, `mds lint --fix`, and `mds build`/`mds watch` outputs and `.map` sidecars are written to a same-directory temp file and renamed over the target after a final symlink re-check (`mds-cli/src/output.rs`, diff --git a/crates/mds-cli/src/fmt.rs b/crates/mds-cli/src/fmt.rs index 1a19315e..6c25e8c1 100644 --- a/crates/mds-cli/src/fmt.rs +++ b/crates/mds-cli/src/fmt.rs @@ -96,9 +96,17 @@ pub(crate) fn run_fmt(args: FmtArgs) -> Result<()> { /// TOCTOU-safe read-then-size-check instead of a bare `std::fs::read`. fn read_source_file(path: &Path) -> Result { let canonical = mds::NativeFs::check_symlink(path).map_err(miette::Error::from)?; + // `MdsError::Io`, not a bare `miette::miette!`: a `miette!` report does not downcast + // to `MdsError`, so `exit_code` fell through to 1 while `check_symlink` one line above + // — the same class of failure on the same argument — already exited 2. The message + // text is identical to `lint.rs`'s `read_source_file` so the two subcommands report + // an undecodable path the same way (#217). let path_str = canonical .to_str() - .ok_or_else(|| miette::miette!("path is not valid UTF-8: {}", path.display()))?; + .ok_or_else(|| mds::MdsError::Io { + message: format!("path is not valid UTF-8: {}", path.display()), + }) + .map_err(miette::Error::from)?; let fs = mds::NativeFs::new(); // R3 / CWE-209: anchor the display root (project-root walk-up from the // file's directory) BEFORE read(), so read-error messages show a diff --git a/crates/mds-cli/src/lint.rs b/crates/mds-cli/src/lint.rs index 15ea2427..389a179d 100644 --- a/crates/mds-cli/src/lint.rs +++ b/crates/mds-cli/src/lint.rs @@ -357,18 +357,41 @@ fn set_diag_display_path(result: &mut mds::LintResult, display: &str) { /// nested path `sub\d.mds` using the native separator (0x5B < 0x5C), but AFTER /// it with the emitted forward slash (0x5B > 0x2F), reversing the array order /// relative to the emitted key order. -fn relative_display(path: &Path, root: &Path) -> String { - let rel = path.strip_prefix(root).unwrap_or(path); - // Hardening: strip_prefix is verified UNREACHABLE today (both call sites pass - // ctx.lint_root and every entry originates from read_dir(dir)). Filter to - // Normal components so the fallback degrades to a relative-looking path rather - // than joining RootDir/Prefix components into `//foo/bar.mds` (Unix) or - // `C:/\/foo/bar.mds` (Windows). - rel.components() - .filter(|c| matches!(c, std::path::Component::Normal(_))) - .map(|c| c.as_os_str().to_string_lossy()) - .collect::>() - .join("/") +/// +/// # Fail-closed contract (#217) +/// +/// The returned string is the directory-mode `files[].file` key and the sort key. +/// A path that is not under `root`, that yields a non-`Normal` component after +/// the strip, or that is not valid UTF-8 is an `Io` error — never a lossy +/// (U+FFFD) or absolute key. +fn relative_display(path: &Path, root: &Path) -> std::result::Result { + let escapes = || MdsError::Io { + message: format!( + "path escapes lint root {}: {}", + root.display(), + path.display() + ), + }; + let rel = path.strip_prefix(root).map_err(|_| escapes())?; + let mut out = String::new(); + for c in rel.components() { + // Only `Normal` can follow a successful strip of a read_dir entry; RootDir / + // Prefix / ParentDir here means the invariant above is broken — reject rather + // than join them into `//foo` (Unix) or `C:/\/foo` (Windows) or drop a `..`. + let std::path::Component::Normal(name) = c else { + return Err(escapes()); + }; + // Same wording as `read_source_file`'s non-UTF-8 rejection, which is what a + // per-file read of this entry would have produced before the run-level check. + let name = name.to_str().ok_or_else(|| MdsError::Io { + message: format!("path is not valid UTF-8: {}", path.display()), + })?; + if !out.is_empty() { + out.push('/'); + } + out.push_str(name); + } + Ok(out) } // ── Read source file ────────────────────────────────────────────────────────── @@ -1200,7 +1223,6 @@ fn run_lint_file( /// The `RefCell` provides interior mutability so per-file helpers can populate the /// caches through a shared `&LintDirCtx` reference. struct LintDirCtx<'a> { - lint_root: &'a Path, flags: LintFlags, runtime_vars: &'a Option>, /// Fast-path-1 cache: `base_dir → config`. Every directory whose files have @@ -1369,7 +1391,7 @@ fn run_lint_directory( // directory and across subdirectories sharing one root mds.json. let walk = collect_mds_files_detailed(dir, MAX_DEPTH, None); - let mut files = walk.files; + let files = walk.files; // AD-216-9: neither early exit below emits a summary — the per-file loop never // runs, so all four counters stay at zero and there is nothing meaningful to @@ -1398,12 +1420,34 @@ fn run_lint_directory( std::process::exit(2); } + // #217: compute every display key BEFORE the sort, so a path that cannot be + // named relative to `dir` fails the whole run instead of contributing a lossy + // or absolute key. Ordering matters twice over: + // * before the sort — the sort key IS the display key, so a lossy key that + // only fails later would already have been built and compared here; + // * before the per-file loop — nothing is linted, so no `files[]` entry can + // carry a key that names no file under `dir`. + // The failure is NOT propagated with `?`: the caller returns `miette::Result`, + // and a bare `?` would render a human diagnostic and drop the JSON + // analysis-failure envelope that `--format json` consumers parse. Same shape + // as the symlinked-root rejection at the top of `run_lint`. + let mut keyed: Vec<(PathBuf, String)> = Vec::with_capacity(files.len()); + for p in files { + match relative_display(&p, dir) { + Ok(display) => keyed.push((p, display)), + Err(e) => { + emit_analysis_failure_json_or_stderr(&e, format, None); + std::process::exit(mds_error_exit_code(&e)); + } + } + } + // F1: sort by (sanitized_display_key, raw_os_path) so that: // 1. Array position is consistent with the sanitized `files[].file` key // emitted by `to_canonical_json` for diagnostic entries (AC-P1-10). - // 2. An OsString secondary key breaks any ties when two different non-UTF-8 - // filenames produce the same `to_string_lossy` string — rare in practice, - // but ensures deterministic order regardless of readdir enumeration order. + // 2. An OsString secondary key breaks any ties when two distinct paths produce + // the same sanitized display key — rare in practice, but ensures + // deterministic order regardless of readdir enumeration order. // // `Path::Ord` (component-wise) diverges from byte-order when a path-separator // character appears WITHIN a filename component — e.g. `api-utils.mds` sorts @@ -1426,9 +1470,9 @@ fn run_lint_directory( // // `sort_by_cached_key` computes each key once — O(n) allocations, not O(n log n) // (AC-P1-22). - files.sort_by_cached_key(|p| { + keyed.sort_by_cached_key(|(p, display)| { ( - mds::sanitize_control_chars_wire(&relative_display(p, dir)).into_owned(), + mds::sanitize_control_chars_wire(display).into_owned(), p.as_os_str().to_os_string(), ) }); @@ -1449,24 +1493,30 @@ fn run_lint_directory( let mut limit_file_count: usize = 0; let ctx = LintDirCtx { - lint_root: dir, flags, runtime_vars: &runtime_vars, base_dir_cache: RefCell::new(HashMap::new()), config_dir_cache: RefCell::new(HashMap::new()), }; - for file in &files { + for (file, display_path) in &keyed { let tally = if format == LintFormat::Json { lint_one_file_accumulating( file, + display_path, &ctx, &mut json_files, &mut any_truncated, &mut any_would_fix, ) } else { - lint_one_file_human(file, &ctx, &mut any_truncated, &mut any_would_fix) + lint_one_file_human( + file, + display_path, + &ctx, + &mut any_truncated, + &mut any_would_fix, + ) }; // AD-216-5: exhaustive match — a future FileTally variant becomes a compile // error here rather than being silently uncounted in the summary. @@ -1487,7 +1537,7 @@ fn run_lint_directory( // suppression, never data corruption. debug_assert_eq!( clean_count + warn_file_count + error_file_count + limit_file_count, - files.len(), + keyed.len(), "AD-216-5: FileTally partition invariant violated" ); @@ -1551,6 +1601,7 @@ fn run_lint_directory( /// Lint one file in directory mode, accumulating results into a JSON array. fn lint_one_file_accumulating( file: &Path, + display_path: &str, ctx: &LintDirCtx<'_>, json_files: &mut Vec, any_truncated: &mut bool, @@ -1565,21 +1616,22 @@ fn lint_one_file_accumulating( .. } = ctx.flags; - // Compute a display path relative to the lint root so JSON `file` keys - // are navigable and unique across the whole directory tree (not just basenames). - // `relative_display` normalises to forward slashes. `to_canonical_json` then - // sanitizes the key via `sanitize_control_chars_wire`; `run_lint_directory` - // sorts on that same sanitized string, so emitted array order and emitted file - // key order are consistent for all inputs including control-byte filenames - // (AC-P1-10). - let display_path = relative_display(file, ctx.lint_root); + // `display_path` is the lint-root-relative key computed once by + // `run_lint_directory`'s pre-pass (#217) — it is never recomputed here, so a + // path this helper could not name has already failed the whole run. It is + // navigable and unique across the whole directory tree (not just basenames) + // and normalised to forward slashes. `to_canonical_json` then sanitizes the + // key via `sanitize_control_chars_wire`; `run_lint_directory` sorts on that + // same sanitized string, so emitted array order and emitted file key order + // are consistent for all inputs including control-byte filenames (AC-P1-10). + // // Error-only entries (`{"file": …, "error": …}`) bypass `to_canonical_json` // and therefore bypass its `sanitize_control_chars_wire` pass. Pre-sanitize // here so the `file` key in error entries is treated identically to the `file` // key in diagnostic entries — hostile filenames cannot inject control, bidi, // or separator characters into either entry type (spec.md §lint-json `file` // contract; ADR-008). - let file_key = mds::sanitize_control_chars_wire(&display_path).into_owned(); + let file_key = mds::sanitize_control_chars_wire(display_path).into_owned(); // `source` is only consumed in the fix branch (below); the report-only/JSON // path does not need it — mds::lint() reads the file independently (I-06). @@ -1615,7 +1667,7 @@ fn lint_one_file_accumulating( } }; // Remap basename-only file field → relative display path. - set_diag_display_path(&mut result, &display_path); + set_diag_display_path(&mut result, display_path); if result.truncated { *any_truncated = true; @@ -1652,7 +1704,7 @@ fn lint_one_file_accumulating( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ); match fix_outcome { FixFileOutcome::Fixed { @@ -1742,7 +1794,7 @@ fn lint_one_file_accumulating( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ) { PreviewOutcome::WouldFix { ref fixed, @@ -1780,6 +1832,7 @@ fn lint_one_file_accumulating( /// Lint one file in directory mode, rendering diagnostics to stderr (human mode). fn lint_one_file_human( file: &Path, + display_path: &str, ctx: &LintDirCtx<'_>, any_truncated: &mut bool, any_would_fix: &mut bool, @@ -1792,11 +1845,11 @@ fn lint_one_file_human( .. } = ctx.flags; - // Compute a display path relative to the lint root for human rendering. - // `relative_display` normalises to forward slashes, matching the unsanitized - // base used by `run_lint_directory`'s sort (AC-P1-10). Human rendering - // shows the real filename bytes rather than sanitized `\uXXXX` escapes. - let display_path = relative_display(file, ctx.lint_root); + // `display_path` is the lint-root-relative key computed once by + // `run_lint_directory`'s pre-pass (#217), normalised to forward slashes and + // identical to the unsanitized base used by that pre-pass's sort (AC-P1-10). + // Human rendering shows the real filename bytes rather than sanitized + // `\uXXXX` escapes. let source = match read_source_file(file) { Ok(s) => s, @@ -1821,7 +1874,7 @@ fn lint_one_file_human( }; // Named source for span rendering: relative display path + source text. - let named_source = (display_path.as_str(), source.as_str()); + let named_source = (display_path, source.as_str()); let mut result = match mds::lint(file, ctx.runtime_vars.clone(), &config) { Ok(r) => r, @@ -1835,7 +1888,7 @@ fn lint_one_file_human( } }; // Remap basename-only file field → relative display path. - set_diag_display_path(&mut result, &display_path); + set_diag_display_path(&mut result, display_path); if result.truncated { *any_truncated = true; @@ -1858,7 +1911,7 @@ fn lint_one_file_human( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ); match fix_outcome { FixFileOutcome::Fixed { @@ -1928,7 +1981,7 @@ fn lint_one_file_human( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ) { PreviewOutcome::WouldFix { ref fixed, @@ -2171,6 +2224,105 @@ mod tests { } } + /// #217: a path that is not under the lint root must be an `Io` error, never a + /// key silently rebuilt out of the path's own components. + /// + /// The old body used `strip_prefix(root).unwrap_or(path)`, so an out-of-root + /// path fell through to a `Normal`-component join of the FULL host path — a + /// `files[].file` key describing a location outside the directory the user + /// asked to lint, with the leading `/` quietly dropped. + /// + /// Positive control: the in-root arm must still return `Ok`, otherwise + /// an unconditional `Err` would satisfy the rejection assertion while breaking + /// every real path. + #[test] + fn relative_display_rejects_path_outside_root() { + use super::relative_display; + use std::path::Path; + + let root = Path::new("/lint-root"); + + // Absolute out-of-root: the strip fails, and the first component of the + // would-be fallback is `RootDir`. + let err = relative_display(Path::new("/other/x.mds"), root) + .expect_err("a path outside the lint root must not produce a display key"); + let message = err.to_string(); + assert!( + message.contains("escapes lint root"), + "out-of-root rejection must name the condition; got: {message:?}" + ); + + // RELATIVE out-of-root: every component is `Normal`, so the component + // guard cannot see this one — only the `strip_prefix` result can reject + // it. Without this arm a reintroduced `strip_prefix(root).unwrap_or(path)` + // still satisfies the absolute arm above and goes unnoticed. + let rel_root = Path::new("lint-root"); + let rel_err = relative_display(Path::new("other/x.mds"), rel_root) + .expect_err("a relative path outside the lint root must not produce a display key"); + let rel_message = rel_err.to_string(); + assert!( + rel_message.contains("escapes lint root"), + "relative out-of-root rejection must name the condition; got: {rel_message:?}" + ); + + // CONTROL ARMS: in-root paths must still succeed with the unchanged key, + // under both an absolute and a relative root. + assert_eq!( + relative_display(Path::new("/lint-root/a/b.mds"), root) + .expect("an in-root path must still produce a display key"), + "a/b.mds", + "control: the in-root key must be byte-identical to the pre-#217 output" + ); + assert_eq!( + relative_display(Path::new("lint-root/a/b.mds"), rel_root) + .expect("an in-root path under a relative root must still produce a key"), + "a/b.mds", + "control: the relative-root in-root key must be byte-identical too" + ); + } + + /// #217: a directory entry whose name is not valid UTF-8 must be an `Io` error, + /// never a lossy key. + /// + /// The old body ran `to_string_lossy` over each component, so an entry named + /// with two invalid bytes became a key of two U+FFFD replacement characters — + /// a `files[].file` value that names no file on disk and collides with every + /// other undecodable name in the tree. + /// + /// The invalid bytes are built at RUNTIME from numeric values; no escape + /// sequence or raw byte appears in this source file (Source hygiene gate). + /// + /// Positive control: the valid-UTF-8 arm must still return `Ok`. + #[cfg(unix)] + #[test] + fn relative_display_rejects_non_utf8_component() { + use super::relative_display; + use std::os::unix::ffi::OsStrExt; + use std::path::Path; + + let root = Path::new("/lint-root"); + + // 0xFF and 0xFE are not legal UTF-8 lead bytes in any position. + let raw: Vec = vec![0xff, 0xfe, b'.', b'm', b'd', b's']; + let hostile = root.join(std::ffi::OsStr::from_bytes(&raw)); + + let err = relative_display(&hostile, root) + .expect_err("a non-UTF-8 entry name must not produce a display key"); + let message = err.to_string(); + assert!( + message.contains("not valid UTF-8"), + "non-UTF-8 rejection must name the condition; got: {message:?}" + ); + + // CONTROL ARM: a valid-UTF-8 sibling in the same root must still succeed. + assert_eq!( + relative_display(&root.join("ok.mds"), root) + .expect("a valid-UTF-8 entry must still produce a display key"), + "ok.mds", + "control: the valid-UTF-8 key must be byte-identical to the pre-#217 output" + ); + } + /// Regression: `relative_display` must NOT treat a literal backslash in a /// Unix filename as a path separator. /// @@ -2204,8 +2356,8 @@ mod tests { // is the byte sequence a, 0x5C, b, ., m, d, s — no control bytes. let backslash_name = Path::new("/lint-root/a\\b.mds"); - let display_subdir = relative_display(real_subdir, root); - let display_backslash = relative_display(backslash_name, root); + let display_subdir = relative_display(real_subdir, root).expect("in-root path"); + let display_backslash = relative_display(backslash_name, root).expect("in-root path"); assert_eq!( display_subdir, "a/b.mds", @@ -2255,8 +2407,8 @@ mod tests { let ctrl_path: &Path = &ctrl_path_buf; let normal_path = Path::new("/lint-root/P.mds"); - let ctrl_raw = relative_display(ctrl_path, root); - let normal_raw = relative_display(normal_path, root); + let ctrl_raw = relative_display(ctrl_path, root).expect("in-root path"); + let normal_raw = relative_display(normal_path, root).expect("in-root path"); // Raw (unsanitized) order: 0x01 < 'P' (0x50) → control-byte file sorts first. assert!( @@ -2311,10 +2463,10 @@ mod tests { let file_z = root.join("z.mds"); // flat: display "z.mds" // Verify display strings first (documents intent and catches platform drift). - let display_a = relative_display(&file_a, &root); - let display_sub_a = relative_display(&file_sub_a, &root); - let display_sub_bracket = relative_display(&file_sub_bracket, &root); - let display_z = relative_display(&file_z, &root); + let display_a = relative_display(&file_a, &root).expect("in-root path"); + let display_sub_a = relative_display(&file_sub_a, &root).expect("in-root path"); + let display_sub_bracket = relative_display(&file_sub_bracket, &root).expect("in-root path"); + let display_z = relative_display(&file_z, &root).expect("in-root path"); assert_eq!( display_a, "a.mds", @@ -2341,10 +2493,14 @@ mod tests { file_sub_a.clone(), ]; paths.sort_by_cached_key(|p| { - mds::sanitize_control_chars_wire(&relative_display(p, &root)).into_owned() + mds::sanitize_control_chars_wire(&relative_display(p, &root).expect("in-root path")) + .into_owned() }); - let sorted: Vec = paths.iter().map(|p| relative_display(p, &root)).collect(); + let sorted: Vec = paths + .iter() + .map(|p| relative_display(p, &root).expect("in-root path")) + .collect(); // Expected byte-wise order: // "a.mds" — 'a' (0x61) @@ -2378,7 +2534,7 @@ mod tests { // Windows absolute path: C:\proj\sub\c.mds with root C:\proj let path = Path::new(r"C:\proj\sub\c.mds"); let root = Path::new(r"C:\proj"); - let display = relative_display(path, root); + let display = relative_display(path, root).expect("in-root path"); assert_eq!( display, "sub/c.mds", diff --git a/crates/mds-cli/src/output.rs b/crates/mds-cli/src/output.rs index 8b7e14a4..1215c024 100644 --- a/crates/mds-cli/src/output.rs +++ b/crates/mds-cli/src/output.rs @@ -20,6 +20,7 @@ //! callers need both single-file and directory logic. use std::borrow::Cow; +use std::ffi::OsStr; use std::io::{IsTerminal, Write as _}; use std::path::{Path, PathBuf}; @@ -205,6 +206,15 @@ pub(crate) fn canonicalize_out_dir(out_dir: Option<&PathBuf>) -> Option let abs = if d.is_absolute() { d.clone() } else { + // Fail-OPEN, deliberately left alone here (#217): when `current_dir()` fails + // — the cwd was deleted, or is unreadable — the relative `--out-dir` is + // anchored at `"."` instead, which resolves against whatever the process's + // cwd actually is. The subsequent `canonicalize()` then usually fails too and + // the non-absolute form is returned, so `starts_with` containment checks + // downstream compare against a path that is not the one they assume. + // Turning this into a hard error changes the signature of an infallible + // helper and every caller with it; tracked as a follow-up rather than folded + // into this change. std::env::current_dir() .unwrap_or_else(|_| PathBuf::from(".")) .join(d) @@ -248,8 +258,9 @@ pub(crate) fn resolve_output_base( /// /// Infallible — no directory creation. /// -/// Defined in terms of [`output_base_no_ext`] so the strip_prefix / AC-M7 -/// path-escape logic is kept in one place (issue 5 — single source of truth). +/// Defined in terms of [`mirror_stem`] so the strip_prefix / AC-M7 path-escape logic is +/// kept in one place (issue 5 — single source of truth), shared with +/// [`output_base_no_ext`]. /// /// - `Dir(base)`: mirrors `source` relative to `root` under `base`. /// If `strip_prefix` fails (source not under root after canonicalization), @@ -258,22 +269,44 @@ pub(crate) fn resolve_output_base( /// - `NextToSource`: `source.with_extension(ext)`. /// /// The `ext` parameter is the output extension without leading `.` (`"md"` or `"json"`). +/// +/// # This is the WRITE oracle +/// +/// It is called once per output path actually computed for a write, so it is where the +/// [`MirroredStem::Flattened`] arm is reported — a warning naming the source, the root +/// and the flat output. [`output_base_no_ext`] computes the same stem for bookkeeping +/// probes and stays silent; moving the report there would fire it on paths that are +/// never written, several times per watch batch (#217). +/// +/// No live caller can reach the flattened arm: `build` walks `root` and hands the walk's +/// own prefix back here; `watch` gates event paths on `starts_with(&ctx.root)` and its +/// startup/baseline loops use canonical keys under a canonical root whose walker skips +/// symlinks. The warning is therefore an invariant report, not a user-facing condition — +/// if it is ever seen, one of those gates has moved. pub(crate) fn output_path_for(source: &Path, root: &Path, base: &OutputBase, ext: &str) -> PathBuf { - let no_ext = output_base_no_ext(source, root, base); match base { OutputBase::Dir(d) => { + let mirrored = mirror_stem(source, root, d); + let flattened = matches!(mirrored, MirroredStem::Flattened(_)); + let no_ext = mirrored.into_path(); + // Invariant: `no_ext` was built by `mirror_stem` as `/`, so + // `file_name()` is `Some`. The literal fallback exists because the previous + // one — `source.as_os_str()` — could be absolute, and an absolute name makes + // the `join` below re-root out of the out-dir. let mut name = no_ext .file_name() - .unwrap_or(source.as_os_str()) + .unwrap_or_else(|| OsStr::new("output")) .to_os_string(); name.push("."); name.push(ext); let out = no_ext.parent().unwrap_or(d.as_path()).join(&name); // AC-M7 containment invariant: the output path must remain inside the out-dir. - // `output_base_no_ext` already guards the strip_prefix escape case by returning + // `mirror_stem` already guards the strip_prefix escape case by returning // `d/` for out-of-root sources; the with-extension step cannot escape. - // The check here is a defence-in-depth belt-and-suspenders assertion. - if out.starts_with(d) { + // The check here is a defence-in-depth belt-and-suspenders assertion, and it + // stays DEBUG-ONLY on purpose: the release fallback below is contained, so + // there is nothing for a release-time check to prevent. + let out = if out.starts_with(d) { out } else { debug_assert!( @@ -281,18 +314,34 @@ pub(crate) fn output_path_for(source: &Path, root: &Path, base: &OutputBase, ext "output_path_for: AC-M7 violated — output {out:?} escaped out-dir {d:?}" ); let flat_name = { + // Invariant: same as above — the join argument must be relative. let mut n = source .file_stem() - .unwrap_or(source.as_os_str()) + .unwrap_or_else(|| OsStr::new("output")) .to_os_string(); n.push("."); n.push(ext); n }; d.join(flat_name) + }; + if flattened { + // Invariant report, not gated on --quiet (like the depth-limit and + // stale-unlink warnings above). Emitted once per output-path computation: + // build — once per file, since its `output_base_no_ext` probes are silent; + // watch — once per rebuild (#217). + eprint_warning(&format!( + "warning: {} is outside the build root {}; its output is written flat \ + as {} (another source outside the root with the same file name would \ + overwrite it)", + safe_path(source), + safe_path(root), + safe_path(&out) + )); } + out } - OutputBase::NextToSource => no_ext.with_extension(ext), + OutputBase::NextToSource => output_base_no_ext(source, root, base).with_extension(ext), } } @@ -392,8 +441,16 @@ pub(crate) fn is_within_default_excluded_dir(root: &Path, path: &Path) -> bool { // rel.parent() returns Some("") whose file_name() is None, and // "".parent() returns None, ending the loop correctly. let rel = match path.strip_prefix(root) { + // `false` IS the closed value here (#217): this predicate answers "is `path` + // inside a default-excluded subdirectory OF `root`", and a path that is not + // under `root` at all has no such subdirectory — the honest answer is no. It is + // not a fail-open default, because neither caller acts on this answer alone: + // both `handle_fs_event_dir` and `process_dir_batch_incremental` evaluate it + // only in conjunction with their own `starts_with(root)` test, so an out-of-root + // path is already classified as an external dep (compile for deps, never emit + // output) before this function's answer is consulted. + Err(_) => return false, Ok(r) => r, - Err(_) => return false, // path is not under root at all }; let mut ancestor = rel.parent(); while let Some(dir) = ancestor { @@ -507,6 +564,13 @@ fn count_mds_in_excluded_dir(dir: &Path, depth: usize, max_depth: usize, count: } /// Return `true` if `path`'s file name starts with `_` (partial convention, DD2). +/// +/// A name that is not valid UTF-8 answers `false` — "not a partial", i.e. a source that +/// should be compiled and written. That is the closed answer, not the open one (#217): +/// such a source never reaches a write, because every read goes through +/// `mds-core`'s `path_to_str`, which rejects a non-UTF-8 path with `MdsError::Io`. The +/// file is reported as a per-file failure (exit 2 in single-file mode, one `failed` in +/// the directory tally) instead of being silently skipped the way `true` would skip it. pub(crate) fn is_partial(path: &Path) -> bool { path.file_name() .and_then(|n| n.to_str()) @@ -553,27 +617,68 @@ pub(crate) fn probe_and_remove_stale(base_no_ext: &Path, kind: OutputKind) { } } +/// Where a `Dir(_)`-mode source landed. +/// +/// `Flattened` is the `strip_prefix` failure arm: contained by construction (the join +/// argument is always a relative `OsStr`) but it abandons the subtree mirror, so two +/// out-of-root sources with the same file name map to the same path. Unreachable from +/// every live caller — see [`output_path_for`] — and the variant exists so the write +/// oracle can *say* so instead of silently degrading (#217). +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum MirroredStem { + /// `source` was below `root`: its relative subtree is preserved under the out-dir. + Mirrored(PathBuf), + /// `source` was not below `root`: only its stem survives, joined to the out-dir. + Flattened(PathBuf), +} + +impl MirroredStem { + /// The extension-less output path, whichever arm produced it. + pub(crate) fn into_path(self) -> PathBuf { + match self { + Self::Mirrored(p) | Self::Flattened(p) => p, + } + } +} + +/// Compute the `Dir(_)`-mode extension-less output stem for `source`, classified by +/// whether the subtree mirror survived. +/// +/// Single source of truth for both [`output_base_no_ext`] (the silent probe oracle) and +/// [`output_path_for`] (the write oracle that reports the flatten). +fn mirror_stem(source: &Path, root: &Path, d: &Path) -> MirroredStem { + match source.strip_prefix(root) { + Ok(rel) => { + // Invariant: `rel` is a non-empty RELATIVE path — `source` is a regular + // `.mds` file strictly below `root` — so `file_stem()` is `Some`. For a + // single-component `rel` (a bare file name) `parent()` is `Some("")`, not + // `None`, and `d.join("")` is `d`, so bare names land directly in the + // out-dir rather than re-rooting. + let stem = rel.file_stem().unwrap_or(rel.as_os_str()).to_os_string(); + MirroredStem::Mirrored(d.join(rel.parent().unwrap_or(Path::new(""))).join(stem)) + } + Err(_) => { + // Invariant: the join argument must be relative, or `d.join` re-roots and + // the result leaves the out-dir entirely (`d.join("/") == "/"`). + // `file_stem()` is `None` only for `/`, `..` and a bare drive prefix — + // never a `.mds` file — and the fallback that used to stand here, + // `source.as_os_str()`, was exactly the absolute value that escapes. A + // literal is the only value guaranteed relative for every input. + let stem = source.file_stem().unwrap_or_else(|| OsStr::new("output")); + MirroredStem::Flattened(d.join(stem)) + } + } +} + /// Return the path stem (path without extension) for a compiled source. /// /// Used to construct the `base_no_ext` argument to [`probe_and_remove_stale`]. /// -/// For `Dir(base)` mode this mirrors the same strip_prefix logic as [`output_path_for`] -/// so the stem is always computed consistently. +/// For `Dir(base)` mode this defers to [`mirror_stem`] so the stem is always computed +/// consistently with [`output_path_for`]. pub(crate) fn output_base_no_ext(source: &Path, root: &Path, base: &OutputBase) -> PathBuf { match base { - OutputBase::Dir(d) => { - let rel = match source.strip_prefix(root) { - Ok(r) => r.to_path_buf(), - Err(_) => { - // Path-escape guard: use filename only (mirrors output_path_for). - let stem = source.file_stem().unwrap_or(source.as_os_str()); - return d.join(stem); - } - }; - // Build the path with no extension. - let stem = rel.file_stem().unwrap_or(rel.as_os_str()).to_os_string(); - d.join(rel.parent().unwrap_or(Path::new(""))).join(stem) - } + OutputBase::Dir(d) => mirror_stem(source, root, d).into_path(), OutputBase::NextToSource => { // source.with_extension("") removes the existing extension. source.with_extension("") @@ -1444,6 +1549,123 @@ mod tests { assert_eq!(result, PathBuf::from("/out/page.md")); } + /// Body of the first `fn` whose header starts with `header`, brace-matched from the + /// first `{` after it. Used by the lexical guard below; `None` when the header is + /// absent, which the caller turns into a non-vacuity failure. + fn fn_body(src: &str, header: &str) -> Option { + let start = src.find(header)?; + let open = start + src[start..].find('{')?; + let mut depth = 0usize; + for (i, c) in src[open..].char_indices() { + match c { + '{' => depth += 1, + '}' => { + depth -= 1; + if depth == 0 { + return Some(src[open..open + i + 1].to_string()); + } + } + _ => {} + } + } + None + } + + /// #217: the `Dir(_)` oracles must be able to say WHICH arm produced a stem — the + /// subtree mirror, or the out-of-root flatten that drops the subtree and lets two + /// sources with the same file name collide. + /// + /// Both arms are asserted: an assertion that only ever observes `Flattened` would be + /// satisfied by a classifier that returns it unconditionally. + #[test] + fn mirror_stem_classifies_out_of_root_as_flattened() { + assert_eq!( + mirror_stem( + Path::new("/other/page.mds"), + Path::new("/root"), + Path::new("/out"), + ), + MirroredStem::Flattened(PathBuf::from("/out/page")), + "a source outside the root loses its subtree and must say so" + ); + assert_eq!( + mirror_stem( + Path::new("/root/a/page.mds"), + Path::new("/root"), + Path::new("/out"), + ), + MirroredStem::Mirrored(PathBuf::from("/out/a/page")), + "a source below the root keeps its subtree and must NOT be reported" + ); + } + + /// #217: a stem-less source must never hand `d.join` an absolute argument. + /// + /// `Path::new("/").file_stem()` is `None`, and the fallback used to be + /// `source.as_os_str()` — `"/"`. `Path::new("/out").join("/")` re-roots to `"/"`, + /// and the write oracle then built `"/.md"`. Neither is inside the out-dir, and + /// neither is a path any source would legitimately compile to. + /// + /// `output_path_for_outside_root_falls_back_to_flat` above is the control: a source + /// that HAS a stem still flattens to `/.`. + #[test] + fn stemless_source_never_escapes_out_dir() { + let source = Path::new("/"); + let root = Path::new("/root"); + let base = OutputBase::Dir(PathBuf::from("/out")); + + assert_eq!( + output_base_no_ext(source, root, &base), + PathBuf::from("/out/output"), + "the probe oracle must keep a stem-less source inside the out-dir" + ); + assert_eq!( + output_path_for(source, root, &base, "md"), + PathBuf::from("/out/output.md"), + "the write oracle must not join an absolute stem" + ); + } + + /// #217: the out-of-root flatten is reported from the WRITE oracle only. + /// + /// `output_base_no_ext` is a probe: watch calls it to guess the output siblings of a + /// source it is about to forget, repeatedly per batch and for sources that are never + /// written. A warning there would fire on bookkeeping rather than on a write. + /// `output_path_for` is called once per output path actually computed for a write, + /// so that is where the report belongs. + /// + /// Lexical, because the property being pinned is exactly "which function contains + /// the call". Both headers must be found, or the two negative assertions would pass + /// on an empty string. + #[test] + fn flatten_warning_lives_in_the_write_oracle_only() { + const SRC: &str = include_str!("output.rs"); + const NEEDLE: &str = "is outside the build root"; + + let oracle = fn_body(SRC, "fn output_path_for(") + .expect("non-vacuity: fn output_path_for must be present in this file"); + let probe = fn_body(SRC, "fn output_base_no_ext(") + .expect("non-vacuity: fn output_base_no_ext must be present in this file"); + + assert!( + oracle.contains("eprint_warning("), + "the write oracle must report the flattened arm; body: {oracle}" + ); + assert!( + oracle.contains(NEEDLE), + "the write oracle's report must name the out-of-root condition; body: {oracle}" + ); + assert!( + !probe.contains("eprint_warning("), + "the probe oracle must stay silent — it runs on bookkeeping, not on writes; \ + body: {probe}" + ); + assert!( + !probe.contains(NEEDLE), + "the probe oracle must not carry the report text either; body: {probe}" + ); + } + /// The `..tmp--` temp files an atomic write leaves in flight must /// never be collected as sources. The suffix sits AFTER the `.mds`, so /// `Path::extension()` is the `tmp-…` component and the walker's extension gate diff --git a/crates/mds-cli/src/watch.rs b/crates/mds-cli/src/watch.rs index 53c2306a..e1a555dd 100644 --- a/crates/mds-cli/src/watch.rs +++ b/crates/mds-cli/src/watch.rs @@ -1655,13 +1655,25 @@ impl DirWatchState { tracked } - /// Remove all state for a deleted source and its output. - fn forget(&mut self, src: &Path, out: &Path) { - self.last_written.remove(out); + /// Remove every GRAPH record of `src` — its forward edges, its error flag and its + /// known-files membership — without touching `last_written`. + /// + /// This is the whole of `forget` for a source that never had an output of its own: + /// an out-of-root dependency, which is a graph node only (DD3). `last_written` is + /// keyed by OUTPUT path, and guessing an output path for such a source means running + /// it through the out-of-root flatten arm, which yields a key that belongs to an + /// in-root source instead (#217). + fn forget_graph(&mut self, src: &Path) { self.forward_deps.remove(src); self.errored.remove(src); self.known_files.remove(src); } + + /// Remove all state for a deleted source and its output. + fn forget(&mut self, src: &Path, out: &Path) { + self.last_written.remove(out); + self.forget_graph(src); + } } /// State for the dir-mode liveness probe (ADR-021). @@ -1744,6 +1756,12 @@ fn compile_one_source( // Derive the output path from the compiled kind (intrinsic extension). // AC-FUNC-23: a @message template writes .json; a plain template writes .md. + // + // Invariant: `src` is strictly below `root`, so this never takes the + // out-of-root flatten arm and never emits its report (#217). Every path that + // reaches here passed `is_in_root` in `process_dir_batch_incremental` or the + // equivalent gate in `process_dir_batch_vars_changed`; out-of-root deps take + // the dep-refresh-only branch above and never call this function. let ext = compiled.kind.extension(); let out = output_path_for(src, root, output_base, ext); @@ -2390,6 +2408,12 @@ fn dir_watch_startup( // Partials (DD2): track in graph but don't emit their own output. if !is_partial(source) { // Derive the output path from the compiled kind (intrinsic extension). + // + // Invariant: `key` is `graph_key(source)` for a source the walker + // collected under the already-canonical `root`, and the walker skips + // symlinked files and directories — so the canonical key is still + // prefixed by `root` and the out-of-root flatten arm cannot fire + // here (#217). let ext = compiled.kind.extension(); let out = output_path_for(&key, &root, &output_base, ext); if let Err(e) = write_output(Some(out.clone()), &compiled.content, quiet, true) @@ -2458,6 +2482,13 @@ fn dir_watch_startup( ) { Ok(compiled) => { // Derive output path from the compiled kind (intrinsic extension). + // + // Invariant: same `key` over the same `all_files` walk as the startup + // loop above — canonical and prefixed by `root` — so this dedup + // baseline computes the same path by the same arm, and the + // out-of-root flatten cannot fire here either (#217). It must agree + // with the startup loop or the `contains_key` check below would miss + // and every source would be rewritten on the first real event. let ext = compiled.kind.extension(); let out = output_path_for(&key, &root, &output_base, ext); if state.last_written.contains_key(&out) { @@ -2811,11 +2842,20 @@ fn process_dir_batch_incremental( // `forward_deps`, and `known_files` now so it doesn't accumulate as a ghost // entry and waste per-batch allocation on every subsequent real-change event. if !deleted.contains(src) { - // Source is gone — probe both .md and .json to clean up either sibling. - let base_no_ext = output_base_no_ext(src, root, output_base); - for ext in &["md", "json"] { - let out = base_no_ext.with_extension(ext); - state.forget(src, &out); + if is_in_root { + // Source is gone — probe both .md and .json to clean up either sibling. + let base_no_ext = output_base_no_ext(src, root, output_base); + for ext in &["md", "json"] { + let out = base_no_ext.with_extension(ext); + state.forget(src, &out); + } + } else { + // An external dep never had an output, so there is no sibling to + // forget. Probing through `output_base_no_ext` would take the + // out-of-root flatten arm and forget `/.md` — + // an entry belonging to the IN-ROOT source with that file name, + // whose next rebuild would then rewrite identical bytes (#217). + state.forget_graph(src); } } continue; @@ -3987,6 +4027,78 @@ mod tests { ); } + /// #217: pruning a ghost EXTERNAL dep must not forget an in-root source's output. + /// + /// A dependency outside the watched root never had an output of its own, so there is + /// no sibling to forget. Probing for one through `output_base_no_ext` takes the + /// out-of-root flatten arm and yields `/`, which is exactly the + /// path an IN-ROOT source with the same file name owns. The prune then dropped that + /// source's `last_written` entry and its next rebuild rewrote identical bytes. + /// + /// Reachable: an importer whose cross-root `@import` target is deleted leaves the + /// vanished dep in `errored`, and every later real-change batch re-seeds `errored`. + #[test] + fn ghost_external_dep_prune_keeps_in_root_last_written() { + let root_dir = tempfile::tempdir().unwrap(); + let out_dir = tempfile::tempdir().unwrap(); + let shared_dir = tempfile::tempdir().unwrap(); + let root = root_dir.path().to_path_buf(); + let out = out_dir.path().to_path_buf(); + + // The in-root source that carries the batch's real change. + let other = root.join("other.mds"); + std::fs::write(&other, "Hello.\n").unwrap(); + + // The in-root source whose bookkeeping is at risk. It is not in this batch, so + // nothing recompiles it — only its `last_written` entry can change. + let victim = root.join("x.mds"); + std::fs::write(&victim, "Victim.\n").unwrap(); + let victim_out = out.join("x.md"); + + // A cross-root dependency with the same file name that no longer exists. + let ghost = shared_dir.path().join("x.mds"); + assert!(!ghost.exists(), "the ghost dep must not exist on disk"); + + let mut state = DirWatchState { + forward_deps: HashMap::new(), + errored: HashSet::new(), + known_files: BTreeSet::new(), + last_written: HashMap::new(), + external_dep_dirs: BTreeSet::new(), + last_mtimes: HashMap::new(), + }; + state.known_files.insert(victim.clone()); + state + .last_written + .insert(victim_out.clone(), "Victim.\n".to_string()); + state.errored.insert(ghost.clone()); + state + .external_dep_dirs + .insert(shared_dir.path().to_path_buf()); + + let changed: BTreeSet = std::iter::once(other).collect(); + process_dir_batch_incremental( + &changed, + &root, + &OutputBase::Dir(out.clone()), + &None, + true, + &mut state, + ); + + assert!( + !state.errored.contains(&ghost), + "control: the ghost prune must actually have run — without it the assertion \ + below would pass on a batch that never reached the branch" + ); + assert!( + state.last_written.contains_key(&victim_out), + "#217: forgetting a ghost external dep must not drop an in-root source's \ + last_written entry; keys: {:?}", + state.last_written.keys().collect::>() + ); + } + /// A source that has never compiled successfully gets an empty dep set, not a panic. #[test] fn record_error_on_unknown_source_inserts_empty_deps() { diff --git a/crates/mds-cli/tests/cli_fmt.rs b/crates/mds-cli/tests/cli_fmt.rs index e0609623..5af22cdb 100644 --- a/crates/mds-cli/tests/cli_fmt.rs +++ b/crates/mds-cli/tests/cli_fmt.rs @@ -1369,3 +1369,68 @@ fn r3_fmt_read_error_names_root_relative_path() { "fmt read error must not leak the absolute path prefix; got: {stderr}" ); } + +/// #217: `mds fmt ` exits 2, like `lint` and `build`. +/// +/// The module doc has promised "2: file not found / not `.mds` / I/O / bad UTF-8" since +/// the subcommand shipped, and `check_symlink` — two statements earlier in the very same +/// function — already exits 2 (`symlinked_input_rejected_exits_two`). The undecodable-path +/// arm raised a bare `miette::miette!`, which does not downcast to `MdsError` and so fell +/// through `exit_code` to 1: one input class produced two different exit codes depending +/// on which of two adjacent checks caught it first. +/// +/// Positive control: the same content under a normally named path exits 0, so the exit-2 +/// assertion cannot be satisfied by a `fmt` that is simply broken. +/// +/// The invalid bytes are built at RUNTIME from numeric values; no escape sequence or raw +/// byte appears in this source file (source hygiene gate). +#[cfg(unix)] +#[test] +fn fmt_non_utf8_path_exits_two() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + // CONTROL ARM — a normally named, already-formatted source. + let control_dir = tempfile::tempdir().unwrap(); + fs::write(control_dir.path().join("ok.mds"), "Hello\n").unwrap(); + let control = fmt_path(&control_dir.path().join("ok.mds"), &[]); + assert_eq!( + control.status.code(), + Some(0), + "control: a source named in UTF-8 must format cleanly; stderr: {}", + String::from_utf8_lossy(&control.stderr) + ); + + // HOSTILE ARM — the same content under a name that is not valid UTF-8. + let dir = tempfile::tempdir().unwrap(); + let raw: Vec = vec![0xff, 0xfe, b'.', b'm', b'd', b's']; + let hostile = dir.path().join(OsString::from_vec(raw)); + + if fs::write(&hostile, "Hello\n").is_err() { + // macOS (APFS / HFS+) enforces valid UTF-8 in filenames and rejects this create + // with EILSEQ, so the ON-DISK half is a Linux-CI gate. The control arm above has + // already run here. Any OTHER unix filesystem must accept the name and reach the + // assertions below — panic rather than skip silently, so a genuine regression + // can never masquerade as a skip. + #[cfg(not(target_os = "macos"))] + panic!( + "fmt_non_utf8_path_exits_two: a non-UTF-8 filename was rejected by the \ + filesystem — unexpected on this platform" + ); + #[cfg(target_os = "macos")] + return; + } + + let output = fmt_path(&hostile, &[]); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!( + output.status.code(), + Some(2), + "#217: an undecodable path is an I/O failure, not a format failure; \ + stderr: {stderr}" + ); + assert!( + stderr.contains("not valid UTF-8"), + "the diagnostic must say why; got: {stderr}" + ); +} diff --git a/crates/mds-cli/tests/cli_lint.rs b/crates/mds-cli/tests/cli_lint.rs index b81ca507..918cfb73 100644 --- a/crates/mds-cli/tests/cli_lint.rs +++ b/crates/mds-cli/tests/cli_lint.rs @@ -2077,6 +2077,143 @@ fn directory_json_file_key_escapes_control_bytes_in_paths() { ); } +// ── #217: a directory entry that cannot be NAMED fails the whole run ────────── + +/// A directory entry whose name is not valid UTF-8 must abort the whole lint run +/// with the analysis-failure envelope, not be linted under a lossy key. +/// +/// Before #217 the key was built with `to_string_lossy`, so the run continued and +/// emitted a `files[].file` of U+FFFD replacement characters — a wire key that +/// names no file on disk and collides with every other undecodable name in the +/// tree. The run now fails BEFORE any file is linted (so no lossy key is ever +/// constructed), with the same exit code it already used for a per-file +/// non-UTF-8 path: 2. +/// +/// Positive controls, both required (an assertion that only ever observes an +/// absence proves nothing until it is shown to detect the presence): +/// - the same directory WITHOUT the hostile entry must exit 0 and emit +/// `files[0].file == "ok.mds"` — otherwise the exit-2 assertion would be +/// satisfied by a lint that is simply broken; +/// - the human-mode control must contain the literal needle `clean,` — otherwise +/// "no summary line in the hostile run" would be indistinguishable from +/// "the needle never matches anything". +/// +/// The invalid bytes are built at RUNTIME from numeric values; no escape sequence +/// or raw byte appears in this source file (Source hygiene gate). +#[cfg(unix)] +#[test] +fn lint_directory_non_utf8_entry_is_an_io_error_exit_2() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + // A warn-only source: it emits a `files[]` entry (a clean file emits none, so + // it cannot pin the key) and exits 1, which is distinct from the hostile arm's + // exit 2 — so the control cannot accidentally satisfy the hostile assertion. + let warn = fs::read_to_string(fixture("lint_warn_only.mds")).unwrap(); + + // CONTROL ARM — the same directory shape without the hostile entry. + let control_dir = tempfile::tempdir().unwrap(); + fs::write(control_dir.path().join("ok.mds"), &warn).unwrap(); + + let control_json = lint_path(control_dir.path(), &["--format", "json"]); + let control_stdout = String::from_utf8_lossy(&control_json.stdout); + assert_eq!( + control_json.status.code(), + Some(1), + "control: a warn-only directory must exit 1, not the hostile arm's 2; \ + stdout: {control_stdout}" + ); + let cv: serde_json::Value = + serde_json::from_str(&control_stdout).expect("control stdout must be valid JSON"); + assert_eq!( + cv["files"] + .as_array() + .and_then(|f| f.first()) + .and_then(|f| f["file"].as_str()), + Some("ok.mds"), + "control: the warn-only file must be linted under its in-root key; \ + got: {control_stdout}" + ); + + let control_human = lint_path(control_dir.path(), &[]); + let control_stderr = String::from_utf8_lossy(&control_human.stderr); + assert!( + control_stderr.contains("clean,"), + "control: a real directory summary must contain the exact needle \"clean,\", \ + otherwise its absence below proves nothing; got: {control_stderr:?}" + ); + + // HOSTILE ARM — same directory plus an entry that cannot be named in UTF-8. + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("ok.mds"), &warn).unwrap(); + + // 0xFF and 0xFE are not legal UTF-8 lead bytes in any position. The `.mds` + // extension is itself valid UTF-8, so the shared walker still collects the entry. + let raw: Vec = vec![0xff, 0xfe, b'.', b'm', b'd', b's']; + let hostile = dir.path().join(OsString::from_vec(raw)); + + if fs::write(&hostile, &warn).is_err() { + // macOS (APFS / HFS+) enforces valid UTF-8 in filenames and rejects this + // create with EILSEQ, so the ON-DISK half of this test is a Linux-CI gate. + // The pure-path rejection it pins is covered on every platform by the + // `relative_display_rejects_non_utf8_component` unit test in lint.rs, and + // the control arm above has already run here. Any OTHER unix filesystem + // must accept the name and reach the assertions below — panic rather than + // skip silently, so a genuine regression can never masquerade as a skip. + #[cfg(not(target_os = "macos"))] + panic!( + "lint_directory_non_utf8_entry_is_an_io_error_exit_2: a non-UTF-8 filename \ + was rejected by the filesystem — unexpected on this platform" + ); + #[cfg(target_os = "macos")] + return; + } + + let out = lint_path(dir.path(), &["--format", "json"]); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + out.status.code(), + Some(2), + "#217: a directory entry that cannot be named must fail the run with exit 2; \ + stdout: {stdout}" + ); + + let v: serde_json::Value = + serde_json::from_str(&stdout).expect("stdout must be the JSON analysis-failure envelope"); + assert_eq!( + v["error"]["code"].as_str(), + Some("mds::io"), + "#217: the envelope must carry the Io error code; got: {stdout}" + ); + let message = v["error"]["message"].as_str().unwrap_or(""); + assert!( + message.contains("not valid UTF-8"), + "#217: the envelope message must name the condition; got: {message:?}" + ); + assert!( + v.get("files").is_none(), + "#217: the analysis-failure envelope must carry no files[] — no file may be \ + linted under a lossy key; got: {stdout}" + ); + + let human = lint_path(dir.path(), &[]); + let stderr = String::from_utf8_lossy(&human.stderr); + assert_eq!( + human.status.code(), + Some(2), + "#217: human mode must fail the run with exit 2; stderr: {stderr}" + ); + assert!( + stderr.contains("not valid UTF-8"), + "#217: human mode must report the condition on stderr; got: {stderr:?}" + ); + assert!( + !stderr.contains("clean,"), + "#217: the run must abort before the per-file loop, so no directory summary \ + may be emitted; got: {stderr:?}" + ); +} + /// Run `mds -` with `input` on stdin. fn run_mds_stdin(subcommand: &str, input: &str) -> std::process::Output { use std::io::Write; @@ -6701,3 +6838,70 @@ fn d1_dir_fix_check_json_body_pre_fix_and_exit_2() { (residual error); stdout: {stdout}" ); } + +/// #217: the SINGLE-FILE arm of `mds lint` also exits 2 for a path that cannot be +/// named in UTF-8. +/// +/// Sibling of `lint_directory_non_utf8_entry_is_an_io_error_exit_2` above, which covers +/// the directory arm. This one is a PIN: `read_source_file` has always raised +/// `MdsError::Io` here, and `exit_code` has always mapped that to 2. It is recorded +/// because `mds fmt` is being brought to the same behaviour and needs a pinned +/// reference to match — the two now emit the identical message text +/// (`path is not valid UTF-8: …`). +/// +/// Positive control: the same invocation on a clean, normally named file exits 0, so +/// the exit-2 assertion cannot be satisfied by a `lint` that is simply broken. +/// +/// The invalid bytes are built at RUNTIME from numeric values; no escape sequence or +/// raw byte appears in this source file (source hygiene gate). +#[cfg(unix)] +#[test] +fn lint_single_file_non_utf8_path_exits_2() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let clean = fs::read_to_string(fixture("lint_clean.mds")).unwrap(); + + // CONTROL ARM — a normally named file with the same content. + let control_dir = tempfile::tempdir().unwrap(); + fs::write(control_dir.path().join("ok.mds"), &clean).unwrap(); + let control = lint_path(&control_dir.path().join("ok.mds"), &[]); + assert_eq!( + control.status.code(), + Some(0), + "control: a clean file named in UTF-8 must exit 0; stderr: {}", + String::from_utf8_lossy(&control.stderr) + ); + + // HOSTILE ARM — the same content under a name that is not valid UTF-8. + let dir = tempfile::tempdir().unwrap(); + let raw: Vec = vec![0xff, 0xfe, b'.', b'm', b'd', b's']; + let hostile = dir.path().join(OsString::from_vec(raw)); + + if fs::write(&hostile, &clean).is_err() { + // macOS (APFS / HFS+) enforces valid UTF-8 in filenames and rejects this create + // with EILSEQ, so the ON-DISK half is a Linux-CI gate. The control arm above has + // already run here. Any OTHER unix filesystem must accept the name and reach the + // assertions below — panic rather than skip silently, so a genuine regression + // can never masquerade as a skip. + #[cfg(not(target_os = "macos"))] + panic!( + "lint_single_file_non_utf8_path_exits_2: a non-UTF-8 filename was rejected \ + by the filesystem — unexpected on this platform" + ); + #[cfg(target_os = "macos")] + return; + } + + let out = lint_path(&hostile, &[]); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!( + out.status.code(), + Some(2), + "#217: a single-file path that cannot be named must exit 2; stderr: {stderr}" + ); + assert!( + stderr.contains("not valid UTF-8"), + "the diagnostic must say why; got: {stderr}" + ); +} diff --git a/crates/mds-cli/tests/dir_build.rs b/crates/mds-cli/tests/dir_build.rs index b81a138b..03cf4e52 100644 --- a/crates/mds-cli/tests/dir_build.rs +++ b/crates/mds-cli/tests/dir_build.rs @@ -51,6 +51,19 @@ fn build_dir(dir: &Path, extra_args: &[&str]) -> std::process::Output { .unwrap() } +/// Run `mds build ` on a single file. Same shape as [`build_dir`]; a separate +/// helper so call sites stay honest about which input form is under test. +fn build_file(path: &Path, extra_args: &[&str]) -> std::process::Output { + mds_bin() + .arg("build") + .arg(path) + .args(extra_args) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap() +} + fn check_dir(dir: &Path, extra_args: &[&str]) -> std::process::Output { mds_bin() .arg("check") @@ -1379,3 +1392,189 @@ fn dir_build_source_map_sidecar_symlink_target_rejected() { "the symlink target must not be written through" ); } + +// ── #217: output-path invariants (flatten visibility, non-UTF-8 paths) ──────── + +/// #217: `mds build` on a path that cannot be named in UTF-8 exits 2 and writes +/// nothing — no compiled output, and no `.map` sidecar. +/// +/// Positive control (a PIN on existing behaviour): the same invocation on a normally +/// named file exits 0 and writes a sidecar whose `sources` names the source. Without +/// it, "no `.map` was written" in the hostile arm would be indistinguishable from a +/// build that never emits sidecars at all. +/// +/// The invalid bytes are built at RUNTIME from numeric values; no escape sequence or +/// raw byte appears in this source file (source hygiene gate). +#[cfg(unix)] +#[test] +fn build_non_utf8_path_exits_2_and_writes_no_map() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + // CONTROL ARM — a normally named source, same flags. + let control = tempfile::tempdir().unwrap(); + create_plain_mds(control.path(), "ok.mds"); + let control_out = build_file(&control.path().join("ok.mds"), &["--source-map"]); + let control_stderr = String::from_utf8_lossy(&control_out.stderr); + assert_eq!( + control_out.status.code(), + Some(0), + "control: a normally named source must build; stderr: {control_stderr}" + ); + let map_path = control.path().join("ok.md.map"); + assert!( + map_path.is_file(), + "control: --source-map must write a sidecar, or the hostile arm's \ + 'no .map' assertion proves nothing" + ); + let map: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&map_path).unwrap()).expect("sidecar is JSON"); + let sources: Vec<&str> = map["sources"] + .as_array() + .expect("sources must be an array") + .iter() + .filter_map(|s| s.as_str()) + .collect(); + assert_eq!( + sources, + vec!["ok.mds"], + "control: the sidecar must name the source it was built from" + ); + + // HOSTILE ARM — a path that is not valid UTF-8. + // 0xFF and 0xFE are not legal UTF-8 lead bytes in any position. The `.mds` + // extension is itself valid UTF-8, so the extension gate still accepts the entry. + let dir = tempfile::tempdir().unwrap(); + let raw: Vec = vec![0xff, 0xfe, b'.', b'm', b'd', b's']; + let hostile = dir.path().join(OsString::from_vec(raw)); + + if fs::write(&hostile, "Hello, world!\n").is_err() { + // macOS (APFS / HFS+) enforces valid UTF-8 in filenames and rejects this create + // with EILSEQ, so the ON-DISK half is a Linux-CI gate. The control arm above has + // already run here. Any OTHER unix filesystem must accept the name and reach the + // assertions below — panic rather than skip silently, so a genuine regression + // can never masquerade as a skip. + #[cfg(not(target_os = "macos"))] + panic!( + "build_non_utf8_path_exits_2_and_writes_no_map: a non-UTF-8 filename was \ + rejected by the filesystem — unexpected on this platform" + ); + #[cfg(target_os = "macos")] + return; + } + + let output = build_file(&hostile, &["--source-map"]); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!( + output.status.code(), + Some(2), + "#217: a path that cannot be named must be an I/O failure; stderr: {stderr}" + ); + assert!( + stderr.contains("not valid UTF-8"), + "the diagnostic must say why; got: {stderr}" + ); + + let names: Vec = fs::read_dir(dir.path()) + .unwrap() + .filter_map(Result::ok) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .collect(); + assert_eq!( + names.len(), + 1, + "neither an output nor a sidecar may be written for a rejected path; got {names:?}" + ); +} + +/// #217 control: a root reached through a symlinked ANCESTOR still mirrors its subtree, +/// and the out-of-root flatten is NOT reported. +/// +/// A symlinked root itself is rejected (`dir_build_symlinked_entry_root_rejected`), so +/// the non-canonical shape has to come from an ancestor. `run_build_directory` hands the +/// same raw `dir` value to the walker and to `output_path_for`, so `strip_prefix` +/// succeeds and the mirror survives. This test is what fails if a future change +/// canonicalizes one of the two and not the other. +#[cfg(unix)] +#[test] +fn dir_build_symlinked_ancestor_root_mirrors_without_warning() { + let tmp = tempfile::tempdir().unwrap(); + let real_src = tmp.path().join("real").join("src"); + fs::create_dir_all(real_src.join("sub")).unwrap(); + create_plain_mds(&real_src, "a.mds"); + create_plain_mds(&real_src.join("sub"), "b.mds"); + std::os::unix::fs::symlink(tmp.path().join("real"), tmp.path().join("link")).unwrap(); + + let out = tempfile::tempdir().unwrap(); + let output = build_dir( + &tmp.path().join("link").join("src"), + &["--out-dir", out.path().to_str().unwrap()], + ); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert_eq!( + output.status.code(), + Some(0), + "a root under a symlinked ancestor must build; stderr: {stderr}" + ); + assert!( + out.path().join("a.md").is_file(), + "top-level source must mirror into the out-dir; stderr: {stderr}" + ); + assert!( + out.path().join("sub").join("b.md").is_file(), + "the subtree must be preserved, not flattened; stderr: {stderr}" + ); + assert!( + stderr.contains("2 built, 0 failed"), + "control needle: the real summary must be present, or the negative assertion \ + below would pass on empty stderr; got: {stderr}" + ); + assert!( + !stderr.contains("is outside the build root"), + "a mirrored build must not report the out-of-root flatten; got: {stderr}" + ); +} + +/// #217 control: a root containing a `..` component mirrors its subtree and does not +/// report the out-of-root flatten. Same property as the symlinked-ancestor test, via +/// the other way a caller can hand in a non-canonical root. +#[test] +fn dir_build_dotdot_root_mirrors_without_warning() { + let tmp = tempfile::tempdir().unwrap(); + fs::create_dir_all(tmp.path().join("sub")).unwrap(); + let src = tmp.path().join("src"); + fs::create_dir_all(src.join("nested")).unwrap(); + create_plain_mds(&src, "a.mds"); + create_plain_mds(&src.join("nested"), "b.mds"); + + let out = tempfile::tempdir().unwrap(); + let output = build_dir( + &tmp.path().join("sub").join("..").join("src"), + &["--out-dir", out.path().to_str().unwrap()], + ); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert_eq!( + output.status.code(), + Some(0), + "a root with a '..' component must build; stderr: {stderr}" + ); + assert!( + out.path().join("a.md").is_file(), + "top-level source must mirror into the out-dir; stderr: {stderr}" + ); + assert!( + out.path().join("nested").join("b.md").is_file(), + "the subtree must be preserved, not flattened; stderr: {stderr}" + ); + assert!( + stderr.contains("2 built, 0 failed"), + "control needle: the real summary must be present, or the negative assertion \ + below would pass on empty stderr; got: {stderr}" + ); + assert!( + !stderr.contains("is outside the build root"), + "a mirrored build must not report the out-of-root flatten; got: {stderr}" + ); +} diff --git a/crates/mds-core/src/evaluator.rs b/crates/mds-core/src/evaluator.rs index a20e8cdc..fec6c685 100644 --- a/crates/mds-core/src/evaluator.rs +++ b/crates/mds-core/src/evaluator.rs @@ -233,10 +233,19 @@ fn evaluate_nodes( if let Some(ref mut map) = ctx.map { if map.suppress == 0 { let abs_out = saved_cursor + output.len() as u32; - debug_assert_eq!( + // Enforced in release builds too (#220): `cursor` is the absolute + // output position every following segment is computed from, so a + // desynchronised cursor mis-attributes the whole rest of the map + // in silence — a source map that points at the wrong bytes is + // worse than no source map. Only a defect in the arms that + // re-anchor the cursor can trip it; no template input can. + // Cost: one u32 compare per node, only while a source map is + // being recorded. + assert_eq!( map.cursor, abs_out, - "cursor invariant violated at Text offset={}", - t.offset + "source-map cursor desynchronised from the output length at \ + a Text node: every following segment would map to the wrong \ + output offset" ); map.push_segment(abs_out, t.offset as u32, t.text.len() as u32); } @@ -250,9 +259,12 @@ fn evaluate_nodes( if let Some(ref mut map) = ctx.map { if map.suppress == 0 { let abs_out = saved_cursor + output.len() as u32; - debug_assert_eq!( + // Enforced in release too (#220); see the Text arm above. + assert_eq!( map.cursor, abs_out, - "cursor invariant violated at EscapedBrace offset={offset}" + "source-map cursor desynchronised from the output length at \ + an EscapedBrace node: every following segment would map to \ + the wrong output offset" ); // Source span: `\{{` is 3 source bytes (backslash + two braces). map.push_segment(abs_out, *offset as u32, 3); @@ -273,10 +285,12 @@ fn evaluate_nodes( // invoke_function will read this anchor as the body's base // output position. let abs_out = saved_cursor + output.len() as u32; - debug_assert_eq!( + // Enforced in release too (#220); see the Text arm above. + assert_eq!( map.cursor, abs_out, - "cursor invariant violated at Interpolation offset={}", - interp.offset + "source-map cursor desynchronised from the output length at \ + an Interpolation node: every following segment would map to \ + the wrong output offset" ); } // Always anchor cursor so inner evaluate_nodes invocations @@ -2532,4 +2546,101 @@ mod tests { "messages: wrong error for object-in-array syntax: {err}" ); } + + // ── #220: the source-map cursor invariant, checked from outside ─────────── + + /// The cursor invariant the record points assert internally is also an observable + /// output property: when `evaluate_with_map` returns, `cursor` is the compiled + /// output length, and every recorded segment points inside that output. + /// + /// Exercised across all three recording node kinds (Text, EscapedBrace, + /// Interpolation) and through the recursive arms (`@if`, `@for`, `@message`, + /// `@block`) that each run their own `evaluate_nodes` invocation with its own + /// local output buffer. + #[test] + fn evaluate_with_map_cursor_tracks_output_length_across_node_kinds() { + let source = concat!( + "Hello {{name}}!\n", + "Escaped: \\{{ raw\n", + "@if flag:\n", + "yes {{name}}\n", + "@end\n", + "@for i in items:\n", + "- {{i}}\n", + "@end\n", + "@message user:\n", + "ask {{name}}\n", + "@end\n", + "@block extra:\n", + "inside {{name}}\n", + "@end\n", + ); + let tokens = crate::lexer::tokenize(source, "t.mds").expect("fixture must tokenize"); + let module = + crate::parser::parse_with_ctx(&tokens, "t.mds", source).expect("fixture must parse"); + + let mut scope = Scope::new(); + scope.set_var("name", Value::String("World".to_string())); + scope.set_var("flag", Value::Boolean(true)); + scope.set_var( + "items", + Value::Array(vec![ + Value::String("a".to_string()), + Value::String("b".to_string()), + ]), + ); + + let builder = crate::sourcemap::MapBuilder::new( + "t.mds".to_string(), + "t.mds".to_string(), + source.to_string(), + ); + let mut warnings = vec![]; + let (output, map) = evaluate_with_map(&module.body, &mut scope, &mut warnings, builder) + .expect("fixture must evaluate"); + + assert_eq!( + map.cursor, + output.len() as u32, + "the cursor must equal the compiled output length when evaluation returns" + ); + assert!( + map.segments.len() >= 12, + "non-vacuity: the fixture must reach many record points; got {} segments", + map.segments.len() + ); + + let mut prev_out = 0u32; + for (i, seg) in map.segments.iter().enumerate() { + assert!( + seg.out >= prev_out, + "segment {i} output offset went backwards: {} < {prev_out}", + seg.out + ); + assert!( + seg.out as usize <= output.len(), + "segment {i} output offset {} is past the end of the compiled output ({})", + seg.out, + output.len() + ); + prev_out = seg.out; + } + + // The escaped brace is the record point with a fixed source width: three source + // bytes (backslash + two braces) collapsing to a two-byte output. + let escape_off = u32::try_from( + source + .find("\\{{") + .expect("fixture must contain an escaped brace"), + ) + .expect("fixture offset fits in u32"); + assert!( + map.segments + .iter() + .any(|s| s.src_off == escape_off && s.len == 3), + "expected an EscapedBrace segment at source offset {escape_off} with len 3; \ + got: {:?}", + map.segments + ); + } } diff --git a/crates/mds-core/src/fs.rs b/crates/mds-core/src/fs.rs index 28405629..a0c1f7d7 100644 --- a/crates/mds-core/src/fs.rs +++ b/crates/mds-core/src/fs.rs @@ -127,6 +127,12 @@ pub trait FileSystem: Send + Sync { /// root found by walking up from the entry-point directory). Returns /// `None` if the root has not been established yet (before any /// `normalize` or `set_root` call). + /// + /// # Contract + /// + /// Implementations must return `None` rather than a lossy string for a root + /// that is not valid UTF-8 — a lossy anchor is not byte-faithful and must not + /// participate in containment (#217). fn source_root(&self) -> Option { None } @@ -556,7 +562,14 @@ impl FileSystem for NativeFs { } fn source_root(&self) -> Option { - self.root_dir.get().map(|p| p.display().to_string()) + // `to_str`, not `display()`: a root that is not valid UTF-8 has no + // byte-faithful string form, and `None` is the documented "no containment + // concept" value every consumer already guards (#217). Containment itself + // is unaffected — `check_path_traversal` compares `Path`s from `root_dir` + // directly and never goes through this string. + self.root_dir + .get() + .and_then(|p| p.to_str().map(str::to_owned)) } } @@ -1497,6 +1510,52 @@ mod tests { ); } + /// #217: a root that is not valid UTF-8 has no byte-faithful string form, so + /// `source_root()` must report `None` — the documented "no containment concept" + /// value, which every consumer already handles with a guarded branch — rather + /// than a lossy stand-in. A lossy anchor names a directory that does not exist + /// and cannot be compared component-wise against a real source path. + /// + /// The containment check itself is unaffected: `check_path_traversal` compares + /// `Path`s from `root_dir` directly and never goes through this string. + /// + /// The invalid byte is built at RUNTIME from a numeric value; no escape sequence + /// or raw byte appears in this source file (Source hygiene gate). + /// + /// Positive control: a valid-UTF-8 root set the same way must still be reported. + #[cfg(unix)] + #[test] + fn source_root_is_none_for_non_utf8_root() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + // 0xFF is not a legal UTF-8 lead byte in any position. + let mut raw = b"/tmp/".to_vec(); + raw.push(0xff); + + let fs = NativeFs::new(); + fs.root_dir + .set(PathBuf::from(OsString::from_vec(raw))) + .expect("root_dir is unset on a fresh NativeFs"); + assert_eq!( + fs.source_root(), + None, + "a root that is not valid UTF-8 must be reported as absent, never lossily" + ); + + // CONTROL ARM: a valid-UTF-8 root set the same way is still reported. + let control = NativeFs::new(); + control + .root_dir + .set(PathBuf::from("/tmp/proj")) + .expect("root_dir is unset on a fresh NativeFs"); + assert_eq!( + control.source_root(), + Some("/tmp/proj".to_string()), + "control: a usable root must still be reported" + ); + } + #[test] fn vfs_source_root_always_none() { // VirtualFs has no containment concept — source_root() always returns None. diff --git a/crates/mds-core/src/lint/diagnostic.rs b/crates/mds-core/src/lint/diagnostic.rs index 354d13ec..e2493876 100644 --- a/crates/mds-core/src/lint/diagnostic.rs +++ b/crates/mds-core/src/lint/diagnostic.rs @@ -249,6 +249,11 @@ impl TextEdit { /// This is the supported construction path for external crates — struct literals /// are not available because this type is `#[non_exhaustive]`. /// + /// # Panics + /// + /// Panics when `start > end`, in every build profile, not only debug builds + /// (promoted from a debug-only assertion in 0.4.3, #220). + /// /// # Examples /// /// ``` @@ -260,7 +265,14 @@ impl TextEdit { /// ``` #[must_use] pub fn new(start: usize, end: usize, new_text: impl Into) -> Self { - debug_assert!(start <= end, "TextEdit::new: start ({start}) > end ({end})"); + // Enforced in release builds too (#220): a reversed byte range is not a + // representable edit, and every applier in the fix pipeline skips one rather + // than failing, so the lint would report a fix it never made. + assert!( + start <= end, + "TextEdit::new: start is greater than end; a reversed byte range is not an \ + edit and the fix applier would skip it in silence" + ); TextEdit { start, end, @@ -323,14 +335,18 @@ impl FixLineSpan { /// Both `from` and `to` are byte offsets within their respective lines. /// The planner translates them to exact line boundaries. /// - /// # Panics (debug) + /// # Panics /// - /// Panics in debug builds when `from > to`. + /// Panics when `from > to`, in every build profile, not only debug builds + /// (promoted from a debug-only assertion in 0.4.3, #220). #[must_use] pub fn range_inclusive(from: usize, to: usize) -> Self { - debug_assert!( + // Enforced in release builds too (#220): a reversed line range cannot be turned + // into a removal, and the planner drops one rather than failing. + assert!( from <= to, - "FixLineSpan::range_inclusive: from ({from}) > to ({to})" + "FixLineSpan::range_inclusive: from is greater than to; a reversed line range \ + cannot be turned into a removal and the planner would skip it in silence" ); FixLineSpan { from, @@ -344,14 +360,18 @@ impl FixLineSpan { /// The line containing `to` is kept; removal stops at the start of that line. /// Use this when a closing token (e.g. `@end`) must remain in the source. /// - /// # Panics (debug) + /// # Panics /// - /// Panics in debug builds when `from > to`. + /// Panics when `from > to`, in every build profile, not only debug builds + /// (promoted from a debug-only assertion in 0.4.3, #220). #[must_use] pub fn range_exclusive(from: usize, to: usize) -> Self { - debug_assert!( + // Enforced in release builds too (#220): a reversed line range cannot be turned + // into a removal, and the planner drops one rather than failing. + assert!( from <= to, - "FixLineSpan::range_exclusive: from ({from}) > to ({to})" + "FixLineSpan::range_exclusive: from is greater than to; a reversed line range \ + cannot be turned into a removal and the planner would skip it in silence" ); FixLineSpan { from, @@ -1149,7 +1169,8 @@ fn is_control_char(ch: char) -> bool { /// length and therefore needs a different replacement per width. Splitting the /// predicate is what keeps that invariant checkable by reading the code rather than /// by trusting a comment: a member added to the wrong helper is a byte-width bug the -/// `debug_assert_eq!` in `neutralize_source_for_render` catches immediately. +/// `assert_eq!` in `neutralize_source_for_render` catches immediately — in release +/// builds too (#220). /// /// See [`is_two_byte_format_hazard`] and [`is_three_byte_format_hazard`] for the /// per-width membership and the rationale for each codepoint. @@ -1239,10 +1260,15 @@ pub fn neutralize_source_for_render(s: &str) -> Cow<'_, str> { out.push(c); } } - debug_assert_eq!( + // Enforced in release builds too (#220): a width mismatch here does not fail, it + // shifts every span offset and caret column that follows the first substitution, so + // the rendered diagnostic underlines the wrong bytes. Only a defect in the per-width + // helpers can trip it — the hazard class and the substitutions are both in-tree. + assert_eq!( out.len(), s.len(), - "neutralize_source_for_render must preserve byte length" + "neutralize_source_for_render changed the byte length of the source: every span \ + offset and caret column after the first substitution would be shifted" ); Cow::Owned(out) } @@ -1501,7 +1527,7 @@ mod tests { /// /// U+061C is 2 bytes in UTF-8 while the other eleven are 3. Routing it through the /// 3-byte branch (→ U+FFFD) would grow the string by one byte per occurrence, - /// desynchronising every following span offset. The `debug_assert_eq!` inside + /// desynchronising every following span offset. The `assert_eq!` inside /// `neutralize_source_for_render` fires on that; this test pins it from outside so /// the guarantee is also checked as an observable output property. #[test] @@ -2554,4 +2580,92 @@ mod tests { assert_eq!(diags[0]["span"]["offset"], 50_u64); assert_eq!(diags[1]["span"]["offset"], 10_u64); } + + // ── #220: width parity and public range checks hold in release too ──────── + + /// The byte-width parity of `neutralize_source_for_render`, checked over the WHOLE + /// class `is_control_char` claims rather than just the twelve bidi controls. + /// + /// The existing bidi test covers the subset most likely to regress; this one closes + /// the class, so a member added to the wrong width helper is caught even when it is + /// a C0 byte or a C1 codepoint rather than a bidi control. + #[test] + fn neutralize_replacement_width_matches_for_every_hazard_codepoint() { + let mut hits = 0usize; + // Bounded: `char` is a finite scalar range and the iterator skips surrogates. + for ch in '\0'..=char::MAX { + if !is_control_char(ch) { + continue; + } + hits += 1; + let raw = ch.to_string(); + let out = neutralize_source_for_render(&raw); + assert_eq!( + out.len(), + ch.len_utf8(), + "U+{:04X} must be replaced by a substitute of identical byte width \ + ({} bytes); got {} bytes", + ch as u32, + ch.len_utf8(), + out.len() + ); + assert_ne!( + &*out, + raw.as_str(), + "U+{:04X} is claimed hostile by is_control_char but was passed through \ + unchanged", + ch as u32 + ); + } + // Non-vacuity: the walk really matched the whole hazard class (30 C0 + DEL + + // 32 C1 + U+061C + 14 three-byte format hazards). + assert_eq!( + hits, 78, + "non-vacuity: expected is_control_char to claim exactly 78 codepoints; a \ + different count means the class changed and this pin must be updated \ + together with the width helpers" + ); + } + + #[test] + #[should_panic(expected = "TextEdit::new: start is greater than end")] + fn text_edit_new_reversed_range_panics() { + let _ = TextEdit::new(5, 4, ""); + } + + #[test] + #[should_panic(expected = "FixLineSpan::range_inclusive: from is greater than to")] + fn fix_line_span_range_inclusive_reversed_panics() { + let _ = FixLineSpan::range_inclusive(5, 4); + } + + #[test] + #[should_panic(expected = "FixLineSpan::range_exclusive: from is greater than to")] + fn fix_line_span_range_exclusive_reversed_panics() { + let _ = FixLineSpan::range_exclusive(5, 4); + } + + /// Positive control for the three checks above: equal and correctly-ordered bounds + /// are legitimate constructions and must never panic. + #[test] + fn range_constructors_accept_equal_and_ordered_bounds() { + let empty = TextEdit::new(4, 4, ""); + assert_eq!(empty.start, 4); + assert_eq!(empty.end, 4); + assert_eq!(empty.new_text, ""); + + let ordered = TextEdit::new(4, 5, ""); + assert_eq!(ordered.start, 4); + assert_eq!(ordered.end, 5); + + let inclusive = FixLineSpan::range_inclusive(4, 4); + assert_eq!(inclusive.from, 4); + assert_eq!(inclusive.to, 4); + assert!(inclusive.to_inclusive); + + let exclusive = FixLineSpan::range_exclusive(4, 5); + assert_eq!(exclusive.from, 4); + assert_eq!(exclusive.to, 5); + assert!(!exclusive.to_inclusive); + } } diff --git a/crates/mds-core/src/resolver.rs b/crates/mds-core/src/resolver.rs index 9ceab923..1af0c49c 100644 --- a/crates/mds-core/src/resolver.rs +++ b/crates/mds-core/src/resolver.rs @@ -2509,15 +2509,12 @@ fn attach_import_span( ) -> MdsError { // Compute the span length as the number of bytes from `offset` to the // end of the `@import` line (not including the newline character itself), - // so the whole directive is underlined. - debug_assert!( - source.is_char_boundary(offset), - "attach_import_span: offset {offset} is not a UTF-8 char boundary in source (len={})", - source.len() - ); - let line_len = source[offset..] - .find('\n') - .unwrap_or(source[offset..].len()); + // so the whole directive is underlined. A non-boundary or out-of-range + // offset can only come from a defect in offset attribution, never from a + // template; `line_len_at` degrades it to a zero-length span instead of + // slicing (#220) — `MdsError::at` then keeps the numeric offset and drops + // the snippet rather than mis-attributing. + let line_len = line_len_at(source, offset); match err { MdsError::FileNotFound { span: None, .. } => { MdsError::file_not_found_at(path, file_str, source, offset, line_len) diff --git a/crates/mds-core/src/resolver/inheritance.rs b/crates/mds-core/src/resolver/inheritance.rs index 04eec416..81090209 100644 --- a/crates/mds-core/src/resolver/inheritance.rs +++ b/crates/mds-core/src/resolver/inheritance.rs @@ -67,6 +67,8 @@ pub(super) fn node_offset(node: &Node) -> usize { /// `@block` overrides and optional whitespace-only text nodes. /// /// Returns `Err(mds::extends)` on the first stray node. +/// +/// A non-boundary offset degrades to a zero-length span (#220). pub(super) fn check_child_only_blocks(body: &[Node], ctx: &ModuleCtx<'_>) -> Result<(), MdsError> { for node in body { match node { @@ -74,15 +76,10 @@ pub(super) fn check_child_only_blocks(body: &[Node], ctx: &ModuleCtx<'_>) -> Res Node::Text(t) if t.text.trim().is_empty() => {} other => { let offset = node_offset(other); - debug_assert!( - ctx.source.is_char_boundary(offset), - "check_child_only_blocks: offset {offset} is not a UTF-8 char boundary \ - in source (len={})", - ctx.source.len() - ); - let line_len = ctx.source[offset..] - .find('\n') - .unwrap_or(ctx.source[offset..].len()); + // Degrade rather than slice on a non-boundary offset (#220): a + // bad offset here is a defect in `node_offset` pairing, not + // something a template can produce. See `line_len_at`. + let line_len = super::line_len_at(ctx.source, offset); return Err(MdsError::extends_error_at( "an extending template may contain only @block overrides", ctx.file_str, @@ -155,9 +152,16 @@ pub(super) fn apply_block_overrides( /// Any other skeleton node yields a single-element slice and the `skeleton_origin` /// (the root-base file). /// -/// Mirrors the missing-block `debug_assert!`/fallback from `splice_skeleton` so both -/// consumers (splice + validate) have identical coverage. This is the single shared -/// walk that prevents text/messages mode validate paths from drifting (PF-004). +/// Every skeleton `@block` is required to have an `effective_blocks` entry, and that +/// requirement is enforced in release builds too (#220). This is the single shared walk +/// that prevents text/messages mode validate paths from drifting (PF-004), so both +/// consumers (splice + validate) have identical coverage of it. +/// +/// # Panics +/// +/// Panics when a skeleton `@block` has no `effective_blocks` entry. That pairing is +/// built from the skeleton itself, so only a defect in the override-map construction +/// can produce it — no template input can. pub(super) fn spliced_regions<'a>( skeleton: &'a [Node], effective_blocks: &'a IndexMap, @@ -166,21 +170,22 @@ pub(super) fn spliced_regions<'a>( let mut regions = Vec::with_capacity(skeleton.len()); for node in skeleton { if let Node::Block(skeleton_block) = node { - if let Some(eff_block) = effective_blocks.get(&skeleton_block.name) { + let eff_block = effective_blocks.get(&skeleton_block.name); + // Enforced in release too, not `debug_assert!` (#220): the map is built + // from this very skeleton by `seed_effective_blocks` / + // `apply_block_overrides`, so a skeleton `@block` with no entry can only + // be a defect in that pairing — no template input can produce it. The + // old release fallback spliced the base default in silence, i.e. dropped + // a child's override from the compiled output with no diagnostic. + assert!( + eff_block.is_some(), + "skeleton @block has no effective_blocks entry: the override map was not \ + built for this skeleton, so a child's @block override would be silently \ + dropped from the compiled output" + ); + if let Some(eff_block) = eff_block { // Block body with its own origin (the winning override file's source). regions.push((eff_block.node.body.as_slice(), &eff_block.origin)); - } else { - // Every block in the skeleton must have an effective_blocks entry. - // A missing entry is a compiler bug (apply_block_overrides was not called). - debug_assert!( - false, - "spliced_regions: block '{}' in skeleton has no effective_blocks entry — \ - this is a compiler bug (apply_block_overrides was not called for this skeleton)", - skeleton_block.name - ); - // Release build: fall back to the skeleton's own default body (same origin - // as the skeleton since this is the base's own node). - regions.push((skeleton_block.body.as_slice(), skeleton_origin)); } } else { // Non-block skeleton nodes: validated against the skeleton origin. diff --git a/crates/mds-core/src/resolver_tests.rs b/crates/mds-core/src/resolver_tests.rs index 0fd44844..d3afa2d2 100644 --- a/crates/mds-core/src/resolver_tests.rs +++ b/crates/mds-core/src/resolver_tests.rs @@ -3190,6 +3190,12 @@ fn parent_dir_drives_nested_file_import_resolution() { // These tests verify `line_len_at` degrades to 0 rather than panicking on // non-boundary or out-of-bounds offsets, matching the guard in // `build_type_mismatch` (evaluator.rs) per ADR-005. +// +// Since #220 `line_len_at` is also the helper behind `attach_import_span` +// (resolver.rs) and `check_child_only_blocks` (resolver/inheritance.rs). Both +// used to slice `source` directly below a `debug_assert!`, so a non-boundary +// offset panicked in release; they now route the underline length through this +// helper and degrade to a zero-length span. #[test] fn line_len_at_out_of_bounds_returns_zero() { @@ -3258,6 +3264,355 @@ fn line_len_at_offset_at_end_returns_zero() { ); } +// ── #220: span attribution must hold in RELEASE builds too ─────────────────── +// +// Two different remedies, chosen per site by what a defect there would cost: +// +// * `attach_import_span` and `check_child_only_blocks` DEGRADE. Both used to slice +// `source[offset..]` one line below a `debug_assert!` on the very condition the +// slice needs, so a bad offset panicked the compiler in release and merely +// changed WHICH assert fired in debug. Routing them through `line_len_at` yields +// a zero-length span: the numeric offset survives, the snippet is dropped, and no +// caret is painted over the wrong bytes. +// * `spliced_regions` FAILS LOUDLY. Its release fallback spliced the base default in +// silence, i.e. dropped a child's `@block` override from the compiled output with +// no diagnostic at all — strictly worse than a panic. + +/// Render a `catch_unwind` payload as text, for both `&'static str` and `String` +/// panic payloads (a formatted `assert!` message is always the latter). +fn panic_message(payload: &(dyn std::any::Any + Send)) -> String { + if let Some(s) = payload.downcast_ref::() { + s.clone() + } else if let Some(s) = payload.downcast_ref::<&'static str>() { + (*s).to_string() + } else { + "".to_string() + } +} + +#[test] +fn attach_import_span_non_boundary_offset_degrades_to_zero_len_span() { + // "é" is a 2-byte UTF-8 sequence; byte offset 1 sits inside it and is therefore + // never a char boundary. + // + // The DEBUG-profile branch deliberately depends on `MdsError::at()` (error.rs) + // keeping its OWN debug-only canary for the cross-source case. That assert is not + // promoted: promoting it would make the release branch below panic as well, which + // is exactly the degradation this test exists to forbid. + let caught = std::panic::catch_unwind(|| { + attach_import_span( + MdsError::file_not_found("./x.mds"), + "./x.mds", + "", + "é\n", + 1, + ) + }); + + if cfg!(debug_assertions) { + let payload = caught.expect_err("debug builds must still trip the MdsError::at() canary"); + let msg = panic_message(&*payload); + assert!( + msg.contains("MdsError::at(): cross-source offset mismatch"), + "the surviving debug canary must be the one in MdsError::at(), which proves \ + attach_import_span degraded to a zero-length span instead of slicing; got: {msg}" + ); + assert!( + !msg.contains("attach_import_span:"), + "attach_import_span must no longer carry a boundary assert of its own; got: {msg}" + ); + } else { + let err = caught.unwrap_or_else(|payload| { + panic!( + "release builds must not panic on a non-boundary offset; got: {}", + panic_message(&*payload) + ) + }); + match err { + MdsError::FileNotFound { span, src, .. } => { + let span = span.expect("the numeric offset must survive the degradation"); + assert_eq!(span.offset(), 1, "the offset must be preserved verbatim"); + assert_eq!(span.len(), 0, "a degraded span must have zero length"); + assert!( + src.is_none(), + "no snippet may be attached when the offset cannot be trusted" + ); + } + other => panic!("expected a FileNotFound error, got: {other:?}"), + } + } +} + +#[test] +fn attach_import_span_out_of_range_offset_on_empty_source_degrades() { + // An empty source is `MdsError::at()`'s documented exemption, so no canary fires in + // either profile: the only thing that could panic here is the raw `source[offset..]` + // slice this site used to perform. + let err = attach_import_span( + MdsError::file_not_found("./x.mds"), + "./x.mds", + "", + "", + 1, + ); + match err { + MdsError::FileNotFound { span, src, .. } => { + let span = span.expect("the numeric offset must survive the degradation"); + assert_eq!(span.offset(), 1, "the offset must be preserved verbatim"); + assert_eq!(span.len(), 0, "a degraded span must have zero length"); + assert!(src.is_none(), "no snippet may be attached"); + } + other => panic!("expected a FileNotFound error, got: {other:?}"), + } +} + +#[test] +fn attach_import_span_valid_offset_spans_whole_directive_and_passes_other_errors() { + // Positive control: a trustworthy offset must still underline the whole directive + // and still carry the snippet, so the degradation above is not a blanket downgrade. + let source = "@import \"./x.mds\"\nnext\n"; + let err = attach_import_span( + MdsError::file_not_found("./x.mds"), + "./x.mds", + "child.mds", + source, + 0, + ); + match err { + MdsError::FileNotFound { span, src, .. } => { + let span = span.expect("a valid offset must produce a span"); + assert_eq!(span.offset(), 0); + assert_eq!( + span.len(), + 17, + "the span must cover `@import \"./x.mds\"` exactly (17 bytes, no newline)" + ); + assert!( + src.is_some(), + "a trustworthy offset must still carry the source snippet" + ); + } + other => panic!("expected a FileNotFound error, got: {other:?}"), + } + + // Control: variants outside the three span-attaching arms are returned unchanged. + let passthrough = attach_import_span(MdsError::syntax("x"), "./x.mds", "child.mds", source, 0); + match passthrough { + MdsError::Syntax { message, span, src } => { + assert_eq!(message, "x", "the message must be untouched"); + assert!(span.is_none(), "no span may be attached to a syntax error"); + assert!( + src.is_none(), + "no snippet may be attached to a syntax error" + ); + } + other => panic!("expected the syntax error to pass through, got: {other:?}"), + } +} + +/// Build the borrowed module context the `@extends` helpers take. +fn extends_ctx<'a>(source: &'a str, vars: &'a HashMap) -> ModuleCtx<'a> { + ModuleCtx { + key: "", + file_str: "", + source, + base_dir: ".", + runtime_vars: vars, + } +} + +#[test] +fn check_child_only_blocks_non_boundary_offset_degrades() { + // Same shape as `attach_import_span`: offset 1 is inside the 2-byte "é". + let caught = std::panic::catch_unwind(|| { + let vars = HashMap::new(); + let ctx = extends_ctx("é\n", &vars); + let body = vec![Node::EscapedBrace { offset: 1 }]; + check_child_only_blocks(&body, &ctx) + }); + + if cfg!(debug_assertions) { + let payload = caught.expect_err("debug builds must still trip the MdsError::at() canary"); + let msg = panic_message(&*payload); + assert!( + msg.contains("MdsError::at(): cross-source offset mismatch"), + "the surviving debug canary must be the one in MdsError::at(), which proves \ + check_child_only_blocks degraded instead of slicing; got: {msg}" + ); + assert!( + !msg.contains("check_child_only_blocks:"), + "check_child_only_blocks must no longer carry a boundary assert of its own; got: {msg}" + ); + } else { + let result = caught.unwrap_or_else(|payload| { + panic!( + "release builds must not panic on a non-boundary offset; got: {}", + panic_message(&*payload) + ) + }); + match result { + Err(MdsError::Extends { span, src, .. }) => { + let span = span.expect("the numeric offset must survive the degradation"); + assert_eq!(span.offset(), 1, "the offset must be preserved verbatim"); + assert_eq!(span.len(), 0, "a degraded span must have zero length"); + assert!(src.is_none(), "no snippet may be attached"); + } + other => panic!("expected an Extends error, got: {other:?}"), + } + } +} + +#[test] +fn check_child_only_blocks_out_of_range_offset_on_empty_source_degrades() { + let vars = HashMap::new(); + let ctx = extends_ctx("", &vars); + let body = vec![Node::EscapedBrace { offset: 1 }]; + match check_child_only_blocks(&body, &ctx) { + Err(MdsError::Extends { span, src, .. }) => { + let span = span.expect("the numeric offset must survive the degradation"); + assert_eq!(span.offset(), 1, "the offset must be preserved verbatim"); + assert_eq!(span.len(), 0, "a degraded span must have zero length"); + assert!(src.is_none(), "no snippet may be attached"); + } + other => panic!("expected an Extends error, got: {other:?}"), + } +} + +#[test] +fn check_child_only_blocks_valid_offset_spans_stray_line_and_accepts_blocks() { + // Positive control: a trustworthy offset still underlines the stray line and still + // carries the snippet; and a well-formed child body is still accepted. + let source = "@block a:\n@end\n\\{{ x\n"; + let vars = HashMap::new(); + let ctx = extends_ctx(source, &vars); + + let stray = vec![ + Node::Block(BlockNode { + name: "a".to_string(), + body: vec![], + offset: 0, + }), + Node::EscapedBrace { offset: 15 }, + ]; + match check_child_only_blocks(&stray, &ctx) { + Err(MdsError::Extends { span, src, .. }) => { + let span = span.expect("a valid offset must produce a span"); + assert_eq!(span.offset(), 15); + assert_eq!( + span.len(), + 5, + "the span must cover the stray `\\{{{{ x` line (5 bytes, no newline)" + ); + assert!( + src.is_some(), + "a trustworthy offset must still carry the source snippet" + ); + } + other => panic!("expected an Extends error, got: {other:?}"), + } + + let clean = vec![ + Node::Block(BlockNode { + name: "a".to_string(), + body: vec![], + offset: 0, + }), + Node::Text(crate::ast::TextNode { + text: "\n".to_string(), + offset: 14, + }), + ]; + assert!( + check_child_only_blocks(&clean, &ctx).is_ok(), + "@block nodes plus whitespace-only text must still be accepted" + ); +} + +#[test] +#[should_panic(expected = "override map was not built for this skeleton")] +fn spliced_regions_missing_effective_block_panics_in_every_profile() { + let skeleton = vec![Node::Block(BlockNode { + name: "content".to_string(), + body: vec![], + offset: 0, + })]; + let effective: IndexMap = IndexMap::new(); + let skeleton_origin = Origin { + file: Arc::from("base.mds"), + display: Arc::from("base.mds"), + source: Arc::from("@block content:\n@end\n"), + }; + let _ = spliced_regions(&skeleton, &effective, &skeleton_origin); +} + +#[test] +fn spliced_regions_pairs_every_skeleton_block_with_its_effective_entry() { + // Positive control for the assert above: when the map IS built, every region must + // carry the origin of the file its offsets index into — the override's for a block + // placeholder, the skeleton's for everything else. + let skeleton = vec![ + Node::Text(crate::ast::TextNode { + text: "intro\n".to_string(), + offset: 0, + }), + Node::Block(BlockNode { + name: "content".to_string(), + body: vec![], + offset: 6, + }), + ]; + let skeleton_origin = Origin { + file: Arc::from("base.mds"), + display: Arc::from("base.mds"), + source: Arc::from("intro\n@block content:\n@end\n"), + }; + let child_origin = Origin { + file: Arc::from("child.mds"), + display: Arc::from("child.mds"), + source: Arc::from("@extends \"./base.mds\"\n@block content:\nhi\n@end\n"), + }; + + let mut effective: IndexMap = IndexMap::new(); + effective.insert( + "content".to_string(), + EffectiveBlock { + node: Arc::new(BlockNode { + name: "content".to_string(), + body: vec![Node::Text(crate::ast::TextNode { + text: "hi\n".to_string(), + offset: 37, + })], + offset: 22, + }), + origin: child_origin, + }, + ); + + let regions = spliced_regions(&skeleton, &effective, &skeleton_origin); + assert_eq!( + regions.len(), + 2, + "one region per skeleton node (text + block placeholder)" + ); + assert_eq!( + regions[1].0.len(), + 1, + "the placeholder must yield the effective block's body nodes, not the skeleton's" + ); + + let entry = effective + .get("content") + .expect("the effective entry must still be in the map"); + assert!( + Arc::ptr_eq(®ions[1].1.source, &entry.origin.source), + "the block region must carry the OVERRIDING file's origin" + ); + assert!( + Arc::ptr_eq(®ions[0].1.source, &skeleton_origin.source), + "non-block regions must carry the skeleton origin" + ); +} + // ── R3 / CWE-209: construction-time display-path relativization ────────────── // // These tests pin the diagnostic display contract on the NativeFs backend: diff --git a/crates/mds-core/src/source_path.rs b/crates/mds-core/src/source_path.rs index b18c6f3d..7d44ac5b 100644 --- a/crates/mds-core/src/source_path.rs +++ b/crates/mds-core/src/source_path.rs @@ -11,6 +11,11 @@ //! information can survive into `sources[]`. The invariant holds in release //! builds (never `debug_assert!`). //! +//! An anchor that cannot be used is never treated as one: an empty or non-UTF-8 +//! root degrades to the basename and an empty or non-UTF-8 base anchors on the +//! root, because an empty component list is a prefix of every path and would +//! otherwise authorise emitting the whole host path (#217). +//! //! # Single choke-point (PF-004) //! //! **All** `sources[]` relativization MUST flow through this function. There @@ -66,6 +71,9 @@ use std::path::Path; /// `./../../` and interior-`..` bypasses. /// 6. If `root = None`: absolute / drive-qualified / lexical-escape checks all /// degrade to basename; otherwise return the unified path. +/// 6b. Anchor validity: an empty or non-UTF-8 root is not an anchor → +/// basename; an empty or non-UTF-8 base → anchor on the root. Root `/` +/// unifies to `"/"`, which is not empty, so it stays a real root. /// 7. If not absolute: resolve against `base` (or `root`) before containment. /// 8. Containment: component-wise descendant of `root`? If not → basename. /// 9. Emit relative to `b` (where `b = base` if `base` is inside `root`, else @@ -135,18 +143,25 @@ pub fn relativize_source(source: &str, base: Option<&Path>, root: Option<&Path>) }; }; + // Step 6b: anchor validity. A root that is empty or not valid UTF-8 is not an + // anchor — `starts_with_comps(x, [])` is vacuously true, so treating it as one + // would pass containment for every source and emit the host path minus its + // leading `/`. Degrade to the basename instead. (Root `/` unifies to `"/"`, + // which is not empty — it stays a real root; see `path_to_unified`.) + let Some(root_unified) = path_to_unified(root) else { + return basename_fallback(&norm_comps); + }; // Normalize root components (absolute component list). - let root_unified = path_to_unified(root); let root_comps = normalize_abs(&root_unified); + // A base that is empty or not valid UTF-8 is likewise not an anchor; both + // uses below fall back to the root, which IS one. + let base_comps: Option> = base.and_then(path_to_unified).map(|b| normalize_abs(&b)); // Step 7: if not absolute, resolve against base (or root) before containment. let abs_comps: Vec = if is_abs { norm_comps.clone() } else { - let anchor = match base { - Some(b) => normalize_abs(&path_to_unified(b)), - None => root_comps.clone(), - }; + let anchor = base_comps.clone().unwrap_or_else(|| root_comps.clone()); match apply_relative(anchor, &norm_comps) { Some(comps) => comps, // Path escapes above anchor root → basename fallback. @@ -167,16 +182,9 @@ pub fn relativize_source(source: &str, base: Option<&Path>, root: Option<&Path>) // root-relative rather than degrading to a basename. It also makes CLI // and binding surfaces run the **same algorithm** — bindings are permanently // in the base-absent case and always receive root-relative paths. - let b_comps = match base { - Some(b) => { - let bc = normalize_abs(&path_to_unified(b)); - if starts_with_comps(&bc, &root_comps) { - bc - } else { - root_comps.clone() - } - } - None => root_comps.clone(), + let b_comps = match base_comps { + Some(bc) if starts_with_comps(&bc, &root_comps) => bc, + _ => root_comps.clone(), }; let result = component_diff(&b_comps, &abs_comps); @@ -220,8 +228,20 @@ fn is_drive_qualified(s: &str) -> bool { } /// Convert a `Path` to a `/`-unified string. -fn path_to_unified(p: &Path) -> String { - let s = p.to_str().unwrap_or("").replace('\\', "/"); +/// +/// Returns `None` when `p` is not valid UTF-8 or unifies to the empty string. +/// +/// Security invariant: a `None` anchor is never treated as a root or base. An +/// empty component list is a prefix of every path (`starts_with_comps(x, [])` is +/// vacuously true), so an empty root would pass containment for every source and +/// `component_diff` would emit the full host path minus its leading `/`. Callers +/// route `None` root → basename fallback, `None` base → root anchor. +/// +/// A root of `/` unifies to `"/"`, which is NOT empty — it is a real root (a +/// container with no WORKDIR resolves there) and keeps root-relative emission +/// even though `normalize_abs("/")` is also the empty component list. +fn path_to_unified(p: &Path) -> Option { + let s = p.to_str()?.replace('\\', "/"); // Strip Windows verbatim-prefix variants (same normalization that // `relativize_source` applies to source strings in step 3), so that the // root component list is comparable to the stripped source component list. @@ -233,7 +253,7 @@ fn path_to_unified(p: &Path) -> String { .strip_prefix("//?/UNC/") .or_else(|| s.strip_prefix("//?/")) .unwrap_or(&s); - stripped.to_string() + (!stripped.is_empty()).then(|| stripped.to_string()) } /// Normalize an **absolute** path string into a component list. @@ -797,6 +817,139 @@ mod tests { result } + // ── #217: an unusable anchor must not be treated as a root ─────────────── + + /// An empty root is not an anchor. + /// + /// `starts_with_comps(x, [])` is vacuously true — an empty component list is a + /// prefix of every path — so an empty root passes containment for every source + /// and `component_diff` then emits the full host path minus its leading `/`. + /// The whole point of the guard is that filesystem layout never reaches + /// `sources[]`, so an anchor that cannot be used must degrade to the basename, + /// not silently authorise everything. + /// + /// Positive control: a real root containing the same source must still emit the + /// root-relative path, otherwise "degrades to basename" would be indistinguishable + /// from "degrades everything to basename". + #[test] + fn empty_root_is_not_a_vacuous_anchor() { + let source = "/Users/alice/proj/src/a.mds"; + + let out = relativize_source(source, None, Some(p(""))); + assert_eq!( + out, "a.mds", + "an empty root is not usable as a containment anchor; it must degrade to \ + the basename" + ); + assert!( + !out.contains("alice"), + "an empty root must not leak the host path into sources[]; got {out:?}" + ); + check_output_invariants(&out, source); + + // CONTROL ARM: a real root that contains the source still emits root-relative. + let control = relativize_source(source, None, Some(p("/Users/alice/proj"))); + assert_eq!( + control, "src/a.mds", + "control: a usable root must still emit the root-relative path" + ); + } + + /// A root that is not valid UTF-8 is not an anchor. + /// + /// It cannot be compared component-wise against a UTF-8 source string, and a + /// lossy stand-in is not byte-faithful — so it must degrade to the basename + /// rather than collapse to an empty (vacuously-containing) component list. + /// + /// The invalid byte is built at RUNTIME from a numeric value; no escape sequence + /// or raw byte appears in this source file (Source hygiene gate). + /// + /// Positive control: the same root spelled in valid UTF-8 must still emit the + /// root-relative path. + #[cfg(unix)] + #[test] + fn non_utf8_root_degrades_to_basename() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let source = "/Users/alice/projX/src/a.mds"; + + // 0xFF is not a legal UTF-8 lead byte in any position. + let mut raw = b"/Users/alice/proj".to_vec(); + raw.push(0xff); + let bad_root = PathBuf::from(OsStr::from_bytes(&raw)); + + let out = relativize_source(source, None, Some(&bad_root)); + assert_eq!( + out, "a.mds", + "a non-UTF-8 root is not usable as a containment anchor; it must degrade \ + to the basename" + ); + assert!( + !out.contains("alice"), + "a non-UTF-8 root must not leak the host path into sources[]; got {out:?}" + ); + check_output_invariants(&out, source); + + // CONTROL ARM: a valid-UTF-8 root containing the source still emits + // root-relative, so the degradation above is specific to the unusable root. + let control = relativize_source(source, None, Some(p("/Users/alice/projX"))); + assert_eq!( + control, "src/a.mds", + "control: a usable root must still emit the root-relative path" + ); + } + + /// A base that is not valid UTF-8 is not a map anchor — emission falls back to + /// the root anchor, which is still a real containment anchor. + /// + /// Positive control: a usable base under the same root must still shift the + /// emitted path by the map-directory offset (`../src/a.mds`), so this test + /// cannot pass by ignoring `base` altogether. + #[cfg(unix)] + #[test] + fn non_utf8_base_falls_back_to_root_anchor() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let source = "/proj/src/a.mds"; + + let mut raw = b"/proj/build".to_vec(); + raw.push(0xff); + let bad_base = PathBuf::from(OsStr::from_bytes(&raw)); + + let out = relativize_source(source, Some(&bad_base), Some(p("/proj"))); + assert_eq!( + out, "src/a.mds", + "an unusable base must anchor on the root, not on an empty component list" + ); + check_output_invariants(&out, source); + + // CONTROL ARM: a usable base inside the root shifts emission by the map offset. + let control = relativize_source(source, Some(p("/proj/build")), Some(p("/proj"))); + assert_eq!( + control, "../src/a.mds", + "control: a usable base must still anchor emission at the map directory" + ); + } + + /// The filesystem root `/` IS a real root and keeps root-relative emission. + /// + /// `normalize_abs("/")` is the empty component list, the same shape an empty or + /// unusable anchor collapses to — but `/` is a legitimate project root (a + /// container with no WORKDIR resolves there), so the distinction the guard must + /// draw is "the unified string is empty or undecodable" versus "the unified + /// string is `/`". This pins the deliberate non-change. + #[test] + fn root_slash_is_a_real_root_not_a_vacuous_one() { + let out = relativize_source("/a/b.mds", None, Some(p("/"))); + assert_eq!( + out, "a/b.mds", + "root `/` must keep root-relative emission, not degrade to the basename" + ); + check_output_invariants(&out, "/a/b.mds"); + } + /// For every test case, the output must never be absolute and never /// drive-qualified, and for non-sentinel outputs the round-trip /// `normalize(effective_b.join(out))` must stay inside `root`. @@ -909,6 +1062,19 @@ mod tests { base: Some("/proj/build"), root: Some("/proj"), }, + // #217: unusable anchors. An empty component list is a prefix of every + // path, so an empty root would pass containment for every source and + // emit the host path minus its leading `/`. + Case { + source: "/Users/alice/proj/src/a.mds", + base: None, + root: Some(""), + }, + Case { + source: "/proj/src/a.mds", + base: Some(""), + root: Some("/proj"), + }, ]; for c in cases { @@ -934,6 +1100,17 @@ mod tests { c.source, ); + // #217: an unusable root must never authorise host-path emission. + // The round-trip check below cannot catch this on its own — an empty + // root is a prefix of every path, so it is satisfied vacuously. + if c.root == Some("") { + assert!( + !out.contains("alice"), + "an empty root must not leak the host path (source={:?}): got {out:?}", + c.source, + ); + } + // Round-trip: normalize(effective_b.join(out)) must be inside root. // // `effective_b` mirrors the production logic (source_path.rs:170-180): diff --git a/crates/mds-core/src/sourcemap.rs b/crates/mds-core/src/sourcemap.rs index 59994aa9..def2b29b 100644 --- a/crates/mds-core/src/sourcemap.rs +++ b/crates/mds-core/src/sourcemap.rs @@ -677,8 +677,8 @@ pub(crate) struct RawSegment { /// `cursor` must always equal the absolute byte count of compiled output /// emitted so far (across all `evaluate_nodes` invocations for this /// compilation). `evaluate_nodes` updates `cursor` after every output- -/// producing node arm. A `debug_assert!` checks the invariant at each -/// leaf-node record point (when `suppress == 0`). +/// producing node arm. An unconditional `assert_eq!` checks the invariant at +/// each leaf-node record point (when `suppress == 0`) (#220). /// /// # Suppression /// diff --git a/crates/mds-core/tests/assert_promotions.rs b/crates/mds-core/tests/assert_promotions.rs new file mode 100644 index 00000000..6dc3d150 --- /dev/null +++ b/crates/mds-core/tests/assert_promotions.rs @@ -0,0 +1,570 @@ +//! Guard (#220): the invariants promoted out of `debug_assert!` stay promoted, their +//! messages stay free of user data, and the two span sites that were changed to degrade +//! stay degraded. +//! +//! The behavioural half of #220 is pinned by unit tests (release-profile runs prove the +//! asserts fire and the degradations do not panic). This file pins the *shape* the +//! behaviour depends on and that a behavioural test cannot see: +//! +//! * an unconditional `assert!`/`assert_eq!`, never a `debug_` form — a demotion would +//! still pass every debug-profile test in CI, which is exactly how these invariants +//! came to be debug-only in the first place; +//! * a message with no `{` in it — a promoted assert is a release-build panic whose text +//! reaches the user's terminal through the CLI's default panic handler, so it must +//! never interpolate a block name, an offset, or any source text; +//! * a justification comment naming the issue within twelve lines above the assert, so +//! the next reader learns why this one is release-critical when the surrounding +//! assertions are not. +//! +//! The scan is a fixed per-site table, not a repo-wide sweep: every OTHER `debug_assert!` +//! in the workspace is deliberately debug-only, so a blanket rule would be wrong. + +use std::path::Path; + +/// A promoted invariant: which file, which anchor, which assertion macro after that +/// anchor (1-based), the macro form it must have, and its exact message. +struct Promoted { + file: &'static str, + anchor: &'static str, + nth: usize, + macro_form: &'static str, + message: &'static str, +} + +/// This table is the single source of truth for the promoted message wording; the +/// `#[should_panic(expected = ...)]` strings in the unit tests are substrings of it. +const PROMOTED: &[Promoted] = &[ + Promoted { + file: "resolver/inheritance.rs", + anchor: "pub(super) fn spliced_regions<'a>(", + nth: 1, + macro_form: "assert!", + message: "skeleton @block has no effective_blocks entry: the override map was not built \ + for this skeleton, so a child's @block override would be silently dropped from \ + the compiled output", + }, + Promoted { + file: "lint/diagnostic.rs", + anchor: "pub fn neutralize_source_for_render(s: &str) -> Cow<'_, str> {", + nth: 1, + macro_form: "assert_eq!", + message: "neutralize_source_for_render changed the byte length of the source: every span \ + offset and caret column after the first substitution would be shifted", + }, + Promoted { + file: "lint/diagnostic.rs", + anchor: "pub fn new(start: usize, end: usize, new_text: impl Into) -> Self {", + nth: 1, + macro_form: "assert!", + message: "TextEdit::new: start is greater than end; a reversed byte range is not an edit \ + and the fix applier would skip it in silence", + }, + Promoted { + file: "lint/diagnostic.rs", + anchor: "pub fn range_inclusive(from: usize, to: usize) -> Self {", + nth: 1, + macro_form: "assert!", + message: "FixLineSpan::range_inclusive: from is greater than to; a reversed line range \ + cannot be turned into a removal and the planner would skip it in silence", + }, + Promoted { + file: "lint/diagnostic.rs", + anchor: "pub fn range_exclusive(from: usize, to: usize) -> Self {", + nth: 1, + macro_form: "assert!", + message: "FixLineSpan::range_exclusive: from is greater than to; a reversed line range \ + cannot be turned into a removal and the planner would skip it in silence", + }, + // The three source-map cursor checks live in one function, in this order: Text, + // EscapedBrace, Interpolation. Addressing them by position also pins that ordering. + Promoted { + file: "evaluator.rs", + anchor: "fn evaluate_nodes(", + nth: 1, + macro_form: "assert_eq!", + message: "source-map cursor desynchronised from the output length at a Text node: every \ + following segment would map to the wrong output offset", + }, + Promoted { + file: "evaluator.rs", + anchor: "fn evaluate_nodes(", + nth: 2, + macro_form: "assert_eq!", + message: "source-map cursor desynchronised from the output length at an EscapedBrace \ + node: every following segment would map to the wrong output offset", + }, + Promoted { + file: "evaluator.rs", + anchor: "fn evaluate_nodes(", + nth: 3, + macro_form: "assert_eq!", + message: "source-map cursor desynchronised from the output length at an Interpolation \ + node: every following segment would map to the wrong output offset", + }, +]; + +/// A span site that must DEGRADE rather than assert: it routes the line length through +/// the shared helper and no longer slices the source itself. +struct Degraded { + file: &'static str, + anchor: &'static str, + needle: &'static str, +} + +const DEGRADED: &[Degraded] = &[ + Degraded { + file: "resolver.rs", + anchor: "fn attach_import_span(", + needle: "line_len_at(", + }, + Degraded { + file: "resolver/inheritance.rs", + anchor: "pub(super) fn check_child_only_blocks(", + needle: "super::line_len_at(", + }, +]; + +/// Forms the degraded sites must NOT regain: a boundary assert that is compiled out in +/// release, or the raw slice that panics one line below it. +const DEGRADED_FORBIDDEN: &[&str] = &["debug_assert", "[offset..]"]; + +#[test] +fn promoted_asserts_are_unconditional_prose_and_justified() { + let src_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut anchors_found = 0usize; + + for site in PROMOTED { + let path = src_dir.join(site.file); + let raw = read(&path); + let anchor_line = unique_anchor_line(&raw, site.anchor, &path); + anchors_found += 1; + + let macros = assertion_macros_after(&raw, anchor_line); + let (macro_line, form, col) = *macros.get(site.nth - 1).unwrap_or_else(|| { + panic!( + "{}: expected at least {} assertion macro(s) after `{}`, found {}", + site.file, + site.nth, + site.anchor, + macros.len() + ) + }); + + assert_eq!( + form, + site.macro_form, + "{}:{}: assertion #{} after `{}` must be an unconditional `{}` — a `debug_` \ + form is compiled out in release, which is the defect #220 removed", + site.file, + macro_line + 1, + site.nth, + site.anchor, + site.macro_form + ); + + let args = macro_arguments(&raw, macro_line, col, site.file); + let literals = string_literals(&args); + assert_eq!( + literals.len(), + 1, + "{}:{}: expected exactly one string literal in the assertion (its message); \ + found {}: {:?}", + site.file, + macro_line + 1, + literals.len(), + literals + ); + let message = &literals[0]; + assert_eq!( + message, + site.message, + "{}:{}: the promoted assertion message must match the table verbatim — the \ + table is the wording the `#[should_panic(expected = ...)]` tests pin", + site.file, + macro_line + 1 + ); + assert!( + !message.contains('{'), + "{}:{}: a promoted assertion message must not interpolate anything — its text \ + is printed to the terminal by the CLI's default panic handler; got: {message}", + site.file, + macro_line + 1 + ); + + assert!( + has_justification(&raw, macro_line), + "{}:{}: no comment naming #220 within the 12 lines above the promoted \ + assertion — a release-critical assert must say why it is one", + site.file, + macro_line + 1 + ); + } + + for site in DEGRADED { + let path = src_dir.join(site.file); + let masked = mask_comments_and_strings(&read(&path)); + let anchor_line = unique_anchor_line(&masked, site.anchor, &path); + anchors_found += 1; + + let body = function_body(&masked, anchor_line); + assert!( + body.contains(site.needle), + "{}: `{}` must compute its span length through `{}` so a bad offset degrades \ + to a zero-length span instead of slicing", + site.file, + site.anchor, + site.needle + ); + for forbidden in DEGRADED_FORBIDDEN { + assert!( + !body.contains(forbidden), + "{}: `{}` must not contain `{}` — that is the debug-only-guard-plus-slice \ + shape #220 replaced", + site.file, + site.anchor, + forbidden + ); + } + } + + // Non-vacuity: a scanner that silently matched nothing would pass every assertion + // above by never entering a loop body. + assert_eq!( + anchors_found, + PROMOTED.len() + DEGRADED.len(), + "non-vacuity: every table anchor must have been located" + ); +} + +// ── Source scanning helpers ────────────────────────────────────────────────── + +fn read(path: &Path) -> String { + std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("{} must be readable: {e}", path.display())) +} + +/// 0-based index of the line containing `anchor`, asserting the anchor is unique. +fn unique_anchor_line(src: &str, anchor: &str, path: &Path) -> usize { + let hits: Vec = src + .lines() + .enumerate() + .filter(|(_, l)| l.contains(anchor)) + .map(|(i, _)| i) + .collect(); + assert_eq!( + hits.len(), + 1, + "{}: anchor `{anchor}` must occur exactly once; found {} occurrence(s) at lines {:?}", + path.display(), + hits.len(), + hits.iter().map(|i| i + 1).collect::>() + ); + hits[0] +} + +/// Every assertion macro at or after `start_line`, as `(line, form, byte column)`. +/// +/// Comment-only lines are skipped so a rustdoc or justification comment naming a macro +/// is never mistaken for the macro itself. +fn assertion_macros_after(src: &str, start_line: usize) -> Vec<(usize, &'static str, usize)> { + let mut out = Vec::new(); + for (i, line) in src.lines().enumerate().skip(start_line) { + if line.trim_start().starts_with("//") { + continue; + } + if let Some((form, col)) = first_assertion_macro(line) { + out.push((i, form, col)); + } + } + out +} + +/// The first assertion macro token in `line`, as `(form, byte column of the macro name)`. +fn first_assertion_macro(line: &str) -> Option<(&'static str, usize)> { + let mut from = 0usize; + // Bounded: each iteration advances `from` past the match it just examined. + while let Some(rel) = line[from..].find("assert") { + let at = from + rel; + let rest = &line[at..]; + let form = if rest.starts_with("assert_eq!(") { + Some("assert_eq!") + } else if rest.starts_with("assert!(") { + Some("assert!") + } else { + None + }; + if let Some(form) = form { + let debug = line[..at].ends_with("debug_"); + let start = if debug { at - "debug_".len() } else { at }; + let full = match (debug, form) { + (true, "assert_eq!") => "debug_assert_eq!", + (true, _) => "debug_assert!", + (false, f) => f, + }; + return Some((full, start)); + } + from = at + "assert".len(); + } + None +} + +/// The text of the macro's argument list, including the enclosing parentheses. +fn macro_arguments(src: &str, line: usize, col: usize, file: &str) -> String { + let line_start = line_start_offset(src, line); + let open = src[line_start + col..] + .find('(') + .map(|r| line_start + col + r) + .unwrap_or_else(|| panic!("{file}:{}: assertion macro has no `(`", line + 1)); + let close = match_paren(src, open) + .unwrap_or_else(|| panic!("{file}:{}: assertion macro has no matching `)`", line + 1)); + src[open..=close].to_string() +} + +/// Byte offset of the start of 0-based `line`. +fn line_start_offset(src: &str, line: usize) -> usize { + let mut offset = 0usize; + for (i, l) in src.lines().enumerate() { + if i == line { + return offset; + } + offset += l.len() + 1; + } + offset +} + +/// Index of the `)` matching the `(` at `open`, skipping string literals, char literals +/// and line comments. +fn match_paren(src: &str, open: usize) -> Option { + let b = src.as_bytes(); + let mut depth = 0i32; + let mut i = open; + // Bounded: `i` advances by at least one on every path. + while i < b.len() { + match b[i] { + b'/' if b.get(i + 1) == Some(&b'/') => { + while i < b.len() && b[i] != b'\n' { + i += 1; + } + continue; + } + b'"' => { + i += 1; + while i < b.len() { + if b[i] == b'\\' { + i += 2; + continue; + } + if b[i] == b'"' { + break; + } + i += 1; + } + } + b'\'' => { + // Char literal `'x'` / `'\n'` — bounded lookahead so a lifetime is not + // mistaken for one. + let mut j = i + 1; + let mut steps = 0; + while j < b.len() && steps < 8 { + if b[j] == b'\\' { + j += 2; + steps += 1; + continue; + } + if b[j] == b'\'' { + i = j; + break; + } + j += 1; + steps += 1; + } + } + b'(' => depth += 1, + b')' => { + depth -= 1; + if depth == 0 { + return Some(i); + } + } + _ => {} + } + i += 1; + } + None +} + +/// Every Rust string literal in `text`, with escapes and `\`-line-continuations resolved +/// to the value the compiler would produce. +fn string_literals(text: &str) -> Vec { + let c: Vec = text.chars().collect(); + let mut out = Vec::new(); + let mut i = 0usize; + // Bounded: `i` advances by at least one on every path. + while i < c.len() { + if c[i] == '/' && c.get(i + 1) == Some(&'/') { + while i < c.len() && c[i] != '\n' { + i += 1; + } + continue; + } + if c[i] != '"' { + i += 1; + continue; + } + i += 1; + let mut lit = String::new(); + while i < c.len() && c[i] != '"' { + if c[i] != '\\' { + lit.push(c[i]); + i += 1; + continue; + } + i += 1; + match c.get(i) { + // Line continuation: the newline and all following whitespace vanish. + Some('\n') => { + i += 1; + while i < c.len() && (c[i] == ' ' || c[i] == '\t') { + i += 1; + } + } + Some('n') => { + lit.push('\n'); + i += 1; + } + Some('t') => { + lit.push('\t'); + i += 1; + } + Some(&ch) => { + lit.push(ch); + i += 1; + } + None => break, + } + } + i += 1; + out.push(lit); + } + out +} + +/// Is there a line comment naming the issue within the 12 lines above `macro_line`? +fn has_justification(src: &str, macro_line: usize) -> bool { + let lines: Vec<&str> = src.lines().collect(); + let start = macro_line.saturating_sub(12); + lines[start..macro_line] + .iter() + .any(|l| l.trim_start().starts_with("//") && l.contains("#220")) +} + +/// The text of a top-level function body, from its signature line to the closing `}` in +/// column zero. +fn function_body(src: &str, signature_line: usize) -> String { + let lines: Vec<&str> = src.lines().collect(); + let end = lines + .iter() + .enumerate() + .skip(signature_line) + .find(|(_, l)| **l == "}") + .map_or(lines.len(), |(i, _)| i); + lines[signature_line..end].join("\n") +} + +/// Replace the CONTENT of line comments, block comments, string literals and char +/// literals with spaces, preserving overall length and newlines, so a needle that only +/// appears in prose does not count as code. +fn mask_comments_and_strings(src: &str) -> String { + let b = src.as_bytes(); + let mut out = vec![b' '; b.len()]; + for (i, &c) in b.iter().enumerate() { + if c == b'\n' { + out[i] = b'\n'; + } + } + let mut i = 0usize; + // Bounded: `i` advances by at least one on every path. + while i < b.len() { + if b[i] == b'/' && b.get(i + 1) == Some(&b'/') { + while i < b.len() && b[i] != b'\n' { + i += 1; + } + continue; + } + if b[i] == b'/' && b.get(i + 1) == Some(&b'*') { + i += 2; + while i < b.len() && !(b[i] == b'*' && b.get(i + 1) == Some(&b'/')) { + i += 1; + } + i = (i + 2).min(b.len()); + continue; + } + if b[i] == b'r' && matches!(b.get(i + 1), Some(&b'"') | Some(&b'#')) && !prev_is_ident(b, i) + { + let mut hashes = 0usize; + let mut j = i + 1; + while b.get(j) == Some(&b'#') { + hashes += 1; + j += 1; + } + if b.get(j) == Some(&b'"') { + j += 1; + while j < b.len() { + if b[j] == b'"' && (0..hashes).all(|k| b.get(j + 1 + k) == Some(&b'#')) { + j += 1 + hashes; + break; + } + j += 1; + } + i = j.min(b.len()); + continue; + } + } + if b[i] == b'"' { + let mut j = i + 1; + while j < b.len() { + if b[j] == b'\\' { + j += 2; + continue; + } + if b[j] == b'"' { + j += 1; + break; + } + j += 1; + } + i = j.min(b.len()); + continue; + } + if b[i] == b'\'' { + let mut j = i + 1; + let mut closed = false; + let mut steps = 0; + while j < b.len() && steps < 8 { + if b[j] == b'\\' { + j += 2; + steps += 1; + continue; + } + if b[j] == b'\'' { + closed = true; + j += 1; + break; + } + j += 1; + steps += 1; + } + if closed { + i = j.min(b.len()); + continue; + } + } + out[i] = b[i]; + i += 1; + } + String::from_utf8(out).expect("masking preserves UTF-8 boundaries on ASCII delimiters") +} + +fn prev_is_ident(b: &[u8], i: usize) -> bool { + i > 0 && (b[i - 1].is_ascii_alphanumeric() || b[i - 1] == b'_') +} diff --git a/spec.md b/spec.md index 1cca27ab..f3ac070a 100644 --- a/spec.md +++ b/spec.md @@ -893,7 +893,7 @@ mds build src/ --out-dir dist # Mirror subtree: src/a/b.mds → dis - `_`-prefixed files are partials and are skipped (not compiled to output). - Symlinked files and symlinked directories inside the tree are skipped; a symlinked entry root is rejected at startup. - Output extension per file is intrinsic (`.md` or `.json`). -- With `--out-dir `, mirrors the source subtree under `/`; without it, writes next to source. +- With `--out-dir `, mirrors the source subtree under `/`; without it, writes next to source. A source that is not under the build root (not reachable for a walked tree; defence in depth) is written flat as `/.` with a warning naming both paths; the warning is not suppressed by `--quiet`. - `-o` is rejected for a directory input. - Continue-on-error: all compilable files are attempted; a summary (`N built, N failed`) is printed when any file fails or when `--quiet` is not passed; non-zero exit when any failed. Under `--quiet`, the summary is suppressed on a fully-successful run and emitted when any file fails, so the non-zero exit is never unexplained. - When the directory contains no `.mds` files at all, exits 1 with `no .mds files found in ; nothing was built` on stderr — emitted even under `--quiet`, like the all-excluded diagnostic — so an empty tree cannot pass a CI gate silently. (Changed in v0.4.3; previously exited 0.) `mds watch ` is unaffected: it starts on an empty tree and compiles files created later. @@ -998,6 +998,7 @@ cat template.mds | mds lint --fix - # Fix from stdin, write fixed source t - Lints every `.mds` file recursively (including `_`-prefixed partials). - Accumulate-and-continue: per-file errors do not abort the run. +- Before any file is linted, every entry is named relative to the lint root; a path that is not valid UTF-8 or that escapes the lint root is an I/O error (`mds::io`, exit 2) for the whole run — no lossy or absolute `file` key is ever emitted. - After processing all files, emits one summary line to stderr: `N clean, N with warnings, N with errors, N resource-limited` Each file falls in exactly one bucket, so the four counts always sum to the number of @@ -1318,7 +1319,7 @@ Maximum config file size: 1 MB. |------|---------| | `0` | Success | | `1` | Template error (syntax, undefined variable, arity mismatch, recursion, etc.); in directory mode, also "nothing to process" (no `.mds` files, or all under default-excluded directories) | -| `2` | I/O or file-system error (file not found, not an MDS file, I/O failure) | +| `2` | I/O or file-system error (file not found, not an MDS file, I/O failure, a path that is not valid UTF-8) | | `3` | Resource limit exceeded (output too large, too many iterations, message count exceeds `MAX_MESSAGE_COUNT` (10,000), cumulative message content exceeds 50 MB, or frontmatter over 1 MiB, over 200,000 YAML nodes, or flow-nesting deeper than 1024 levels) | **`mds lint`** (see §7.5 for per-code meaning): @@ -1327,7 +1328,7 @@ Maximum config file size: 1 MB. |------|---------| | `0` | Clean — no warning- or error-severity findings | | `1` | Warning-severity findings only (no errors) | -| `2` | Error-severity finding, analysis failure, or usage error (including a directory with nothing to lint) | +| `2` | Error-severity finding, analysis failure, or usage error (including a directory with nothing to lint, or a directory entry whose path is not valid UTF-8) | | `3` | Resource limit exceeded | ---