From ad23019771a1e8acdfd7b313494d165fd0f9040b Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 12:45:48 +0300 Subject: [PATCH 01/11] =?UTF-8?q?test(core):=20RED=20=E2=80=94=20#220=20si?= =?UTF-8?q?tes=20degrade=20or=20assert=20in=20release;=20pub-API=20range?= =?UTF-8?q?=20checks,=20neutralize=20width=20parity=20and=20source-map=20c?= =?UTF-8?q?ursor=20asserts=20pinned=20(#220)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifteen tests, added BEFORE the fix, that fail for the reason #220 exists. Nothing in production code changes in this commit. Behavioural tests (14, lib target): resolver_tests.rs T-A1..A3 attach_import_span_* (3) resolver_tests.rs T-B1..B3 check_child_only_blocks_* (3) resolver_tests.rs T-C1,C2 spliced_regions_* (2) lint/diagnostic.rs T-D1 neutralize_replacement_width_matches_for_every_hazard_codepoint lint/diagnostic.rs T-E1..E4 text_edit_new_reversed / fix_line_span_range_* / range_constructors_accept evaluator.rs T-F1 evaluate_with_map_cursor_tracks_output_length_across_node_kinds Shape test (1, integration target): tests/assert_promotions.rs T-L1 promoted_asserts_are_unconditional_prose_and_justified — a fixed ten-row table (8 promoted asserts + 2 degrade sites), NOT a repo-wide sweep: every other debug_assert! in the workspace is deliberately debug-only. Per promoted row it pins the macro form (no debug_ prefix), the exact message literal (with backslash-continuations joined), the absence of any '{' in it, and a justification comment naming #220 within 12 lines above. Per degrade row it pins that line_len_at( is called and that neither debug_assert nor [offset..] survives in the function body (comments and string literals masked first). DEV-profile RED run (cargo nextest, --no-fail-fast, 1445 skipped): Summary 15 tests run: 6 passed, 9 failed Failing, with the panic text that proves the premise: attach_import_span_non_boundary_offset_degrades_to_zero_len_span resolver.rs:2513 "attach_import_span: offset 1 is not a UTF-8 char boundary in source (len=3)" — the site's OWN debug assert fires, not the MdsError::at() canary the degraded path must reach. attach_import_span_out_of_range_offset_on_empty_source_degrades resolver.rs:2513 "... (len=0)" — fires even though MdsError::at() exempts an empty source. check_child_only_blocks_non_boundary_offset_degrades inheritance.rs:77 "check_child_only_blocks: offset 1 is not a UTF-8 char boundary in source (len=3)" check_child_only_blocks_out_of_range_offset_on_empty_source_degrades inheritance.rs:77, same shape. spliced_regions_missing_effective_block_panics_in_every_profile should_panic expected "override map was not built for this skeleton"; the current debug_assert!(false, ...) panics with different text. text_edit_new_reversed_range_panics panic message "TextEdit::new: start (5) > end (4)"; expected substring "TextEdit::new: start is greater than end". fix_line_span_range_inclusive_reversed_panics "FixLineSpan::range_inclusive: from (5) > to (4)" vs expected "FixLineSpan::range_inclusive: from is greater than to". fix_line_span_range_exclusive_reversed_panics "FixLineSpan::range_exclusive: from (5) > to (4)" vs expected "FixLineSpan::range_exclusive: from is greater than to". promoted_asserts_are_unconditional_prose_and_justified assert_promotions.rs:153 left: "debug_assert!" right: "assert!" at resolver/inheritance.rs:175 — the scanner really reads the macro form. Passing controls (6): attach_import_span_valid_offset_spans_whole_directive_ and_passes_other_errors, check_child_only_blocks_valid_offset_spans_stray_ line_and_accepts_blocks, spliced_regions_pairs_every_skeleton_block_with_its_ effective_entry, range_constructors_accept_equal_and_ordered_bounds, neutralize_replacement_width_matches_for_every_hazard_codepoint, evaluate_with_map_cursor_tracks_output_length_across_node_kinds. G1 RELEASE-profile RED run (the proof CI cannot give — CI runs Rust tests in dev only). Form: plain `cargo nextest ... --cargo-profile release --lib`, no LTO override needed; 31 s wall for list + build + run (30.45 s of that is the release compile). `cargo nextest list -p mds-core --cargo-profile release --lib -E 'test(/attach_import_span_|check_child_only_blocks_|spliced_regions_| text_edit_new_reversed|fix_line_span_range_|range_constructors_accept| neutralize_replacement_width|evaluate_with_map_cursor/)'` listed exactly 14 tests. Summary 14 tests run: 6 passed, 8 failed, 1149 skipped The four degrade tests fail here for a DIFFERENT and worse reason than in debug — the raw slice, not a compiled-out assert: resolver.rs:2518 "start byte index 1 is not a char boundary; it is inside 'e-acute' (bytes 0..2 of string)" resolver.rs:2518 "start byte index 1 is out of bounds for string of length 0" inheritance.rs:83 both of the above, same two inputs. This is the measured refutation of the issue's premise for sites 1 and 2: they do not "silently degrade" in release, they panic one line below the assert that was supposed to catch the condition. The four promotion tests fail with "test did not panic as expected": diagnostic.rs:2606, :2612, :2618 (the three range constructors) and resolver_tests.rs:3531 (spliced_regions) — i.e. in release the invariant is not enforced at all. T-D1 non-vacuity: the codepoint walk found exactly 78 members of is_control_char (30 C0 + DEL + 32 C1 + U+061C + 14 three-byte format hazards); the pin matches the real count, no adjustment needed. Gates on this commit: cargo fmt --all --check clean; cargo clippy --workspace --all-targets -- -D warnings clean (exit 0). --- crates/mds-core/src/evaluator.rs | 97 ++++ crates/mds-core/src/lint/diagnostic.rs | 88 ++++ crates/mds-core/src/resolver_tests.rs | 349 +++++++++++++ crates/mds-core/tests/assert_promotions.rs | 570 +++++++++++++++++++++ 4 files changed, 1104 insertions(+) create mode 100644 crates/mds-core/tests/assert_promotions.rs diff --git a/crates/mds-core/src/evaluator.rs b/crates/mds-core/src/evaluator.rs index a20e8cdc..251c9fe0 100644 --- a/crates/mds-core/src/evaluator.rs +++ b/crates/mds-core/src/evaluator.rs @@ -2532,4 +2532,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/lint/diagnostic.rs b/crates/mds-core/src/lint/diagnostic.rs index 354d13ec..7affce23 100644 --- a/crates/mds-core/src/lint/diagnostic.rs +++ b/crates/mds-core/src/lint/diagnostic.rs @@ -2554,4 +2554,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_tests.rs b/crates/mds-core/src/resolver_tests.rs index 0fd44844..9276d9e5 100644 --- a/crates/mds-core/src/resolver_tests.rs +++ b/crates/mds-core/src/resolver_tests.rs @@ -3258,6 +3258,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/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'_') +} From 9e23488a1e39ef9dee07ed0ba4d14208f425976c Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:02:17 +0300 Subject: [PATCH 02/11] fix(core): degrade non-boundary import/extends spans via line_len_at; enforce the skeleton-block, neutralize-width, range-check and source-map-cursor invariants in release (#220) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remedies, chosen per site by what a defect there actually costs. DEGRADE (2 sites). Both computed a span length by slicing source[offset..] one line below a debug_assert! on the very condition the slice needs. In release the assert vanishes and the slice panics; in debug the site's own assert fires first and hides the real one. Both now call the existing private helper line_len_at, which returns 0 for a non-boundary or out-of-range offset. MdsError::at then keeps the numeric offset and drops the snippet, so the diagnostic degrades instead of mis-attributing or crashing. resolver.rs attach_import_span resolver/inheritance.rs check_child_only_blocks (super::line_len_at) line_len_at stays private and in place; parser.rs directive_line_len is untouched. ENFORCE (8 asserts). Each is a pure code-shape invariant that no template input can reach, and each had a release behaviour strictly worse than a panic. Every message is prose with no interpolation: a promoted assert is a release panic whose text reaches the terminal through the CLI's default handler (the CLI installs no panic hook), so no block name, offset, or source text may appear in it. resolver/inheritance.rs spliced_regions was debug_assert!(false, "... block '{name}' ...") + a release fallback that spliced the base default in SILENCE — i.e. dropped a child's @block override from the compiled output with no diagnostic at all. Now assert!(eff_block.is_some(), ...) on the real predicate; the fallback is gone. rustdoc gains a # Panics section; the pre-existing (PF-004) text is kept verbatim. lint/diagnostic.rs neutralize_source_for_render byte-length parity: a width mismatch does not fail, it shifts every span offset and caret column after the first substitution. lint/diagnostic.rs TextEdit::new, FixLineSpan::range_inclusive, FixLineSpan::range_exclusive public API range checks; every applier in the fix pipeline skips a reversed range rather than failing, so the lint would report a fix it never made. rustdoc "# Panics (debug)" -> "# Panics"; TextEdit::new gains a # Panics section it did not have. evaluator.rs evaluate_nodes, Text / EscapedBrace / Interpolation arms source-map cursor: a desynchronised cursor mis-attributes the whole rest of the map silently. sourcemap.rs MapBuilder rustdoc updated to say an unconditional assert_eq! checks it. Everything else stays debug_assert! — deliberately, one line each: resolver.rs:1654 tautology pin for a planned refactor, not an invariant error.rs:175 debug-only canary BY DESIGN: promoting it would make the degraded release path panic, undoing this fix sourcemap.rs 775/786/827/886 encoder-internal, hot per-segment evaluator.rs 960/977/1005/1432 evaluator-internal, hot per-node parser_helpers.rs 641/865, parser.rs 76/91, lexer.rs 181 parse-time internals lint/fix.rs 618/653/660/664 planner internals; :631 is already an unconditional assert and stays as it is formatter.rs 312/359/376 formatter internals behind a safety gate lib.rs:720, options.rs:144 API-surface internals mds-cli output.rs:279, lint.rs:1488, watch.rs 1476/2498 CLI internals mds-wasm lib.rs:80 binding internal GATES (all three measured, numbers verbatim). G1 — release-profile proof. CI runs Rust tests in dev only, so this is local and mandatory. Form: plain `cargo nextest ... --cargo-profile release --lib`; no CARGO_PROFILE_RELEASE_LTO override was needed and no [profile.release-test] was added. Wall time 31 s for list + build + run. list -> exactly 14 tests run -> Summary 14 tests run: 14 passed, 1149 skipped (the same 14 were 6 passed / 8 failed at the RED commit). Dev profile, same 14 plus the T-L1 shape test: 15 passed, 1445 skipped. Whole workspace: 2335 tests run: 2335 passed (2320 at the RED commit's parent + 15 new). Doc-tests mds: 53 passed. G2 — evaluator cost, the gate on the three source-map cursor asserts. Fixture: 180,000 lines of "Hello {{name}} \{{ tail text" over a frontmatter name: World (5,220,020 bytes; 4,500,020-byte output, 3,600,083-byte map). Binaries: target/release/mds built at the RED commit (production code then byte-identical to main) vs built from this commit. Command, x5 each, back-to-back: /usr/bin/time -p build big.mds --source-map -o out.md round 1, wall seconds before 0.24 0.21 0.22 0.21 0.21 median 0.21 after 0.69 0.21 0.21 0.22 0.21 median 0.21 (the 0.69 is the first touch of a freshly copied binary; it does not recur) round 2, wall / user seconds before 0.21/0.16 x5 median 0.21 / 0.16 after 0.21/0.16 x5 median 0.21 / 0.16 noise control, x3 each, no --source-map (the asserts are unreachable) before 0.17 0.17 0.17 median 0.17 after 0.17 0.17 0.16 median 0.17 Threshold: median wall(after) <= 1.05 x median wall(before) 0.21 <= 0.2205 PASS (ratio 1.000) Supplementary, higher resolution (the specified fixture runs in ~0.21 s, so /usr/bin/time's 10 ms granularity is 4.8% of the measurement): 320,000 lines, 9,280,020 bytes, 960,000 segments — just under MAX_SOURCEMAP_SEGMENTS. before 0.29 x5 median wall 0.29, median user 0.22 after 0.29 0.29 0.30 0.30 0.31 median wall 0.30, median user 0.22 ratio 1.034 <= 1.05 PASS (user-CPU median unchanged) Verdict: PASS on two independent rounds. The three evaluator assert_eq!s are KEPT; no revert to debug_assert_eq! was needed, and the three evaluator rows stay in the shape-test table. G3 — WASM size. Measured with CI's own command (wasm-pack build crates/mds-wasm --target nodejs --out-dir pkg, and --target web --out-dir pkg-web) against the same two trees as G2. Local wasm-opt is wasm-pack's cached v117, not CI's pinned Binaryen v129, so only the DELTA transfers. before pkg 850,313 pkg-web 850,313 after pkg 850,971 pkg-web 850,971 delta +658 bytes (+0.077%) on both — the eight new message strings. Local sits 6,865 bytes above the CI-measured 843,448 at 33380d7, inside the known 3-7 KB local/CI offset. Projected CI: ~844,106 bytes, leaving ~5,894 bytes under the 850,000 guard. Far below the 5 KB trigger that would have forced shorter messages. ci.yml and the guard are untouched. Gates on this commit: cargo fmt --all --check clean; cargo clippy --workspace --all-targets -- -D warnings clean. --- crates/mds-core/src/evaluator.rs | 30 ++++++++--- crates/mds-core/src/lint/diagnostic.rs | 52 ++++++++++++++----- crates/mds-core/src/resolver.rs | 15 +++--- crates/mds-core/src/resolver/inheritance.rs | 55 +++++++++++---------- crates/mds-core/src/sourcemap.rs | 4 +- 5 files changed, 99 insertions(+), 57 deletions(-) diff --git a/crates/mds-core/src/evaluator.rs b/crates/mds-core/src/evaluator.rs index 251c9fe0..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 diff --git a/crates/mds-core/src/lint/diagnostic.rs b/crates/mds-core/src/lint/diagnostic.rs index 7affce23..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] 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/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 /// From 12b3408e1217a8dc6f647bc81e95766cb68ae544 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:19:34 +0300 Subject: [PATCH 03/11] =?UTF-8?q?test(lint):=20RED=20=E2=80=94=20relative?= =?UTF-8?q?=5Fdisplay=20must=20fail=20closed=20(#217)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mds lint ` builds every `files[].file` wire key (and the directory sort key) with `relative_display`, which fails OPEN twice: `strip_prefix(root) .unwrap_or(path)` rebuilds a key out of an out-of-root path's own components, and `to_string_lossy` turns an entry name that is not valid UTF-8 into U+FFFD replacement characters. Either way a key that names no file under the linted directory reaches stdout. Three tests pin the fail-closed contract; each pairs its rejections with a positive control so it cannot pass by rejecting everything. RED observations (this commit, macOS): - relative_display_rejects_path_outside_root — two rejection arms. The absolute arm `/other/x.mds` under root `/lint-root` expected Err("escapes lint root") and got Ok("other/x.mds"): the leading `/` is dropped and the host path is re-emitted as if it were relative to the lint root. The relative arm `other/x.mds` under root `lint-root` is the arm that is sensitive to the `strip_prefix` result alone — every component there is `Normal`, so a component-shape guard cannot see it — and it likewise got Ok("other/x.mds"). Controls: both roots must still yield Ok("a/b.mds") for an in-root path. - relative_display_rejects_non_utf8_component — expected Err("not valid UTF-8"), got Ok of a key whose first two characters are U+FFFD replacement characters followed by ".mds". - lint_directory_non_utf8_entry_is_an_io_error_exit_2 — the on-disk half is a Linux-CI gate: macOS APFS/HFS+ enforce valid UTF-8 filenames and reject the create with EILSEQ, so the hostile arm cannot be built on this machine. The test runs its two positive controls here (exit 1 and `files[0].file == "ok.mds"` in JSON mode; the literal `clean,` summary needle in human mode) and then returns at an explicitly macOS-gated branch; on any other unix filesystem a rejected create panics rather than skipping silently. Expected RED on Linux: a `files` key present in the envelope and a summary line on stderr. Interface scaffold so the RED tests compile and fail on BEHAVIOUR rather than on a type error: `relative_display` already returns `Result`, with the existing fail-open body wrapped in `Ok` and the three production call sites carrying a temporary `.expect`. The GREEN commit replaces the body, the call sites and that scaffold. The four existing `relative_display` unit tests switch to `.expect("in-root path")` with their assertions byte-identical. Counts: mds-cli 878 tests (875 before), 876 passed, 2 failed (the two RED unit tests above). fmt and clippy clean. Refs #217 --- crates/mds-cli/src/lint.rs | 153 +++++++++++++++++++++++++++---- crates/mds-cli/tests/cli_lint.rs | 137 +++++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 17 deletions(-) diff --git a/crates/mds-cli/src/lint.rs b/crates/mds-cli/src/lint.rs index 15ea2427..11831e7d 100644 --- a/crates/mds-cli/src/lint.rs +++ b/crates/mds-cli/src/lint.rs @@ -357,18 +357,29 @@ 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 { +/// +/// # 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 { + // RED SCAFFOLD (#217): the interface is already the fail-closed one so the + // tests below compile and fail on BEHAVIOUR, not on a type error. The body + // is still the fail-open one; the GREEN commit replaces it. 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() + Ok(rel + .components() .filter(|c| matches!(c, std::path::Component::Normal(_))) .map(|c| c.as_os_str().to_string_lossy()) .collect::>() - .join("/") + .join("/")) } // ── Read source file ────────────────────────────────────────────────────────── @@ -1428,7 +1439,10 @@ fn run_lint_directory( // (AC-P1-22). files.sort_by_cached_key(|p| { ( - mds::sanitize_control_chars_wire(&relative_display(p, dir)).into_owned(), + mds::sanitize_control_chars_wire( + &relative_display(p, dir).expect("relative_display is infallible until #217 lands"), + ) + .into_owned(), p.as_os_str().to_os_string(), ) }); @@ -1572,7 +1586,8 @@ fn lint_one_file_accumulating( // 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); + let display_path = relative_display(file, ctx.lint_root) + .expect("relative_display is infallible until #217 lands"); // 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` @@ -1796,7 +1811,8 @@ fn lint_one_file_human( // `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); + let display_path = relative_display(file, ctx.lint_root) + .expect("relative_display is infallible until #217 lands"); let source = match read_source_file(file) { Ok(s) => s, @@ -2171,6 +2187,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 +2319,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 +2370,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 +2426,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 +2456,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 +2497,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/tests/cli_lint.rs b/crates/mds-cli/tests/cli_lint.rs index b81ca507..9e7c2491 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; From dd54b82d12a5d6efa9c0eaf9831cfc0942fe6d9c Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:24:12 +0300 Subject: [PATCH 04/11] fix(lint): relative_display returns Result; display paths computed before the sort; run exits 2 with the analysis-failure envelope (#217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relative_display` no longer manufactures a key it cannot justify. It returns `Err(MdsError::Io { .. })` when the path is not under `root`, when a component that survives the strip is not `Normal`, or when a component is not valid UTF-8 — instead of `strip_prefix(root).unwrap_or(path)` plus a `to_string_lossy` join. For every in-root UTF-8 path the `Ok` value is byte-identical to the previous output, so no existing assertion moved. `run_lint_directory` now computes every display key in a pre-pass BEFORE the sort and before the per-file loop, and `lint_one_file_accumulating` / `lint_one_file_human` take `display_path: &str` instead of recomputing it. Two orderings are load-bearing and are commented at the pre-pass: - before the sort — the sort key IS the display key, so a lossy key that only failed later would already have been built and compared; - before the per-file loop — nothing is linted, so no `files[]` entry can carry a key that names no file under the linted directory. The pre-pass does NOT use `?`. The caller returns `miette::Result`, so a bare `?` would render a human diagnostic and drop the JSON analysis-failure envelope that `--format json` consumers parse. It follows the existing symlinked-root shape instead: `emit_analysis_failure_json_or_stderr(&e, format, None)` then `std::process::exit(mds_error_exit_code(&e))`. The non-UTF-8 message text is the one `read_source_file` already produced for the same condition, so the user-visible wording is unchanged; the exit code is unchanged at 2. What changes is WHEN: the whole run now fails before any file is linted, instead of the tree being linted with one entry keyed by U+FFFD replacement characters. `LintDirCtx.lint_root` existed only to feed those two per-file calls and is removed; the `debug_assert_eq!` partition invariant now counts `keyed`. Verified locally by forcing `relative_display` to error and running the binary over a real directory: `--format json` emits `{"error":{"code":"mds::io","message":"path is not valid UTF-8: …",…},"version":1}` with no `files` key and exit 2; human mode prints the same message with no summary line and exit 2. That is the exact shape `lint_directory_non_utf8_entry_is_an_io_error_exit_2` asserts on Linux, whose hostile arm cannot be built on macOS — APFS/HFS+ reject a non-UTF-8 filename with EILSEQ at create. The test handles that with an explicitly macOS-gated early return placed AFTER its positive controls have run; on any other unix filesystem a rejected create panics rather than skipping silently. Mutation controls (applied, observed, reverted): - `strip_prefix(root).unwrap_or(path)` reintroduced → relative_display_rejects_path_outside_root fails on its relative arm ("other/x.mds"). Recording the first run of this control honestly: with only the absolute arm the mutant SURVIVED, because `/other/x.mds` is then rejected by the `Component::Normal` guard rather than by the strip. The relative arm and its control were added to the RED commit for exactly that reason, and the mutant now dies. - `to_string_lossy` reintroduced → relative_display_rejects_non_utf8_component fails, reporting a key of two U+FFFD replacement characters. - pre-pass replaced by a per-file lossy key: not observable on macOS, where the hostile arm cannot be created. The forced-error run above is the local stand-in — it exercises the envelope + exit code + absent summary that this mutation would break. Counts: mds-cli 878 tests, 878 passed, 0 failed. fmt, clippy and the source-hygiene gate clean. Refs #217 --- crates/mds-cli/src/lint.rs | 141 +++++++++++++++++++++++-------------- 1 file changed, 89 insertions(+), 52 deletions(-) diff --git a/crates/mds-cli/src/lint.rs b/crates/mds-cli/src/lint.rs index 11831e7d..389a179d 100644 --- a/crates/mds-cli/src/lint.rs +++ b/crates/mds-cli/src/lint.rs @@ -365,21 +365,33 @@ fn set_diag_display_path(result: &mut mds::LintResult, display: &str) { /// 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 { - // RED SCAFFOLD (#217): the interface is already the fail-closed one so the - // tests below compile and fail on BEHAVIOUR, not on a type error. The body - // is still the fail-open one; the GREEN commit replaces it. - 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). - Ok(rel - .components() - .filter(|c| matches!(c, std::path::Component::Normal(_))) - .map(|c| c.as_os_str().to_string_lossy()) - .collect::>() - .join("/")) + 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 ────────────────────────────────────────────────────────── @@ -1211,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 @@ -1380,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 @@ -1409,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 @@ -1437,12 +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).expect("relative_display is infallible until #217 lands"), - ) - .into_owned(), + mds::sanitize_control_chars_wire(display).into_owned(), p.as_os_str().to_os_string(), ) }); @@ -1463,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. @@ -1501,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" ); @@ -1565,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, @@ -1579,22 +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) - .expect("relative_display is infallible until #217 lands"); + // `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). @@ -1630,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; @@ -1667,7 +1704,7 @@ fn lint_one_file_accumulating( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ); match fix_outcome { FixFileOutcome::Fixed { @@ -1757,7 +1794,7 @@ fn lint_one_file_accumulating( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ) { PreviewOutcome::WouldFix { ref fixed, @@ -1795,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, @@ -1807,12 +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) - .expect("relative_display is infallible until #217 lands"); + // `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, @@ -1837,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, @@ -1851,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; @@ -1874,7 +1911,7 @@ fn lint_one_file_human( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ); match fix_outcome { FixFileOutcome::Fixed { @@ -1944,7 +1981,7 @@ fn lint_one_file_human( base_dir, ctx.runtime_vars.clone(), &config, - &display_path, + display_path, ) { PreviewOutcome::WouldFix { ref fixed, From 7e9a5fe6564f4fd2385e2795be164d36071fe5c1 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:26:34 +0300 Subject: [PATCH 05/11] =?UTF-8?q?test(core):=20RED=20=E2=80=94=20empty=20o?= =?UTF-8?q?r=20non-UTF-8=20source-map=20anchors=20must=20not=20be=20vacuou?= =?UTF-8?q?s=20(#217)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relativize_source` converts its `root` and `base` anchors with `path_to_unified`, which is `p.to_str().unwrap_or("")`. An anchor that cannot be decoded therefore becomes the empty string, `normalize_abs("")` becomes the EMPTY component list, and `starts_with_comps(x, [])` is vacuously true — an empty component list is a prefix of every path. So an unusable root passes containment for every source, and `component_diff` then emits the full host path minus its leading `/` into `sources[]`. That is precisely the filesystem layout the guard exists to keep out. `NativeFs::source_root` has the mirror-image problem: it reports `p.display().to_string()`, so a root that is not valid UTF-8 is handed to the guard as a lossy string that names no directory and cannot be compared component-wise against a real source. Six tests, each pairing its degradation with a positive control. RED observations (this commit): - empty_root_is_not_a_vacuous_anchor — root `""`, source `/Users/alice/proj/src/a.mds`: expected `"a.mds"`, got `"Users/alice/proj/src/a.mds"` — the host path with the leading `/` removed. - non_utf8_root_degrades_to_basename — root `/Users/alice/proj` plus one invalid byte, source `/Users/alice/projX/src/a.mds`: expected `"a.mds"`, got `"Users/alice/projX/src/a.mds"`. Control root `/Users/alice/projX` already yields `"src/a.mds"`. - property_outputs_never_absolute_or_drive_qualified — the new `root: Some("")` case leaks `"Users/alice/proj/src/a.mds"`. The matrix's existing round-trip invariant cannot catch this on its own: an empty root is a prefix of every path, so the check is satisfied vacuously. The added assertion is explicit about the leak. - source_root_is_none_for_non_utf8_root — a root of `/tmp/` plus one invalid byte: expected `None`, got `Some` of a string ending in a U+FFFD replacement character. Two of the six are PINS that are green before and after, recorded as such rather than presented as RED: - non_utf8_base_falls_back_to_root_anchor — an unusable BASE already lands on the root anchor today, because step 9 asks whether the base is inside the root and an empty component list is not (`starts_with_comps([], ["proj"])` is false on the length check). The test pins that the behaviour survives the change, with a control proving a usable base still shifts emission to `"../src/a.mds"`. - root_slash_is_a_real_root_not_a_vacuous_one — `normalize_abs("/")` is also the empty component list, but `/` is a legitimate project root (a container with no WORKDIR resolves there) and must keep root-relative emission. This pins the deliberate non-change: the distinction to draw is "the unified string is empty or undecodable" versus "the unified string is `/`". The invalid bytes are built at runtime from numeric values; no escape sequence or raw byte appears in any test source. Counts: mds-core 1465 tests (1460 before), 1461 passed, 4 failed (the four RED observations above). fmt, clippy and the source-hygiene gate clean. Refs #217 --- crates/mds-core/src/fs.rs | 46 +++++++++ crates/mds-core/src/source_path.rs | 157 +++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+) diff --git a/crates/mds-core/src/fs.rs b/crates/mds-core/src/fs.rs index 28405629..3c1bcc00 100644 --- a/crates/mds-core/src/fs.rs +++ b/crates/mds-core/src/fs.rs @@ -1497,6 +1497,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/source_path.rs b/crates/mds-core/src/source_path.rs index b18c6f3d..98442400 100644 --- a/crates/mds-core/src/source_path.rs +++ b/crates/mds-core/src/source_path.rs @@ -797,6 +797,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 +1042,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 +1080,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): From d73cf2f18f23c7a932b503eb485fa19b2ec5d76b Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:33:25 +0300 Subject: [PATCH 06/11] =?UTF-8?q?fix(core):=20path=5Fto=5Funified=20is=20O?= =?UTF-8?q?ption;=20unusable=20root=20=E2=86=92=20basename,=20unusable=20b?= =?UTF-8?q?ase=20=E2=86=92=20root=20anchor;=20source=5Froot=20is=20None=20?= =?UTF-8?q?for=20a=20non-UTF-8=20root=20(#217)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `path_to_unified` now returns `Option`, `None` for a path that is not valid UTF-8 or that unifies to the empty string. The whole hazard was that its old `to_str().unwrap_or("")` fed `normalize_abs("")`, which is the EMPTY component list — and `starts_with_comps(x, [])` is vacuously true. An anchor that could not be decoded therefore authorised containment for every source, and `component_diff` emitted the full host path minus its leading `/` into `sources[]`. `relativize_source` gains step 6b, immediately after the existing `root = None` branch: - root that is `None` from `path_to_unified` → `basename_fallback(&norm_comps)`; - base that is `None` from `path_to_unified` → the root anchor, at BOTH of its uses (the step-7 resolution anchor and the step-9 emission anchor `b`). Root `/` is deliberately NOT affected. It unifies to `"/"`, which is not empty, so it stays a real root and keeps root-relative emission even though `normalize_abs("/")` is also the empty component list — a container with no WORKDIR resolves there. The distinction the guard draws is "the unified string is empty or undecodable", not "the component list came out empty". `NativeFs::source_root` uses `to_str` instead of `display()`, so a root that is not valid UTF-8 is reported as `None` — the documented "no containment concept" value with a guarded branch at every consumer (never absolute, never drive-qualified, `..`-escape → basename) — rather than a lossy string naming a directory that does not exist. A sentinel string would be a fake path leaking through the public trait. Containment itself is untouched: `check_path_traversal` compares `Path`s from `root_dir` directly and never goes through this string. The trait rustdoc states the obligation for third-party implementations, and the `relativize_source` rustdoc and the module's security invariant both gain the anchor-validity rule. No new error variant and no sentinel: the change is entirely in the Option routing. Source-map goldens: NONE moved. The design guarantees zero change for any UTF-8 non-empty root and any base, and that is what the surfaces show — `packages/mds` 304/304 pass (including `source maps (U-SM)`, `source maps — WASM backend (W-SM)` and `source maps — compileFile differential (CF-SM)`) against a freshly rebuilt native addon, and the Python suite is 250 passed / 6 deselected against a freshly built extension. The CLI's own source-map assertions in output.rs and watch.rs are part of the green mds-cli run. The WASM path is structurally unaffected: `VirtualFs::source_root` is the trait default `None`, so it never reaches step 6b. An empty base is likewise unreachable from the CLI — `compute_source_map_base` absolutizes every branch. Mutation controls (applied, observed, reverted): - `path_to_unified(root).unwrap_or_default()` → empty_root_is_not_a_vacuous_anchor, non_utf8_root_degrades_to_basename and the property matrix all fail, each reporting the host path minus its leading `/`. - unusable base mapped to the empty component list instead of `None` → non_utf8_base_falls_back_to_root_anchor stays GREEN, and that is correct: step 9 already asks whether the base is inside the root, and an empty component list is not (the length check fails). The mutation is behaviour-preserving for the tested paths; it is recorded here rather than papered over with a test that would only pin an implementation detail. - `source_root` back to `display().to_string()` → source_root_is_none_for_non_utf8_root fails with `Some` of a string ending in a U+FFFD replacement character. Counts: workspace 2343 tests (mds-core 1465, mds-cli 878), all passing; doctests 53. fmt, clippy, the rustdoc gate (`RUSTDOCFLAGS="-D warnings" cargo doc -p mds-core -p mds-cli --no-deps`) and the source-hygiene gate are clean. Refs #217 --- crates/mds-core/src/fs.rs | 15 +++++++- crates/mds-core/src/source_path.rs | 56 ++++++++++++++++++++---------- 2 files changed, 52 insertions(+), 19 deletions(-) diff --git a/crates/mds-core/src/fs.rs b/crates/mds-core/src/fs.rs index 3c1bcc00..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)) } } diff --git a/crates/mds-core/src/source_path.rs b/crates/mds-core/src/source_path.rs index 98442400..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. From a10a7bcd7b2f1a04224aab3796e989eb37074e22 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:47:36 +0300 Subject: [PATCH 07/11] =?UTF-8?q?test(cli):=20RED=20=E2=80=94=20out-of-roo?= =?UTF-8?q?t=20flatten=20is=20classified,=20warned=20and=20never=20absolut?= =?UTF-8?q?e;=20watch=20ghost=20prune=20keeps=20in-root=20state=20(#217)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six tests over the directory-mode output oracles and the watch bookkeeping that probes them. `MirroredStem` + `mirror_stem` are added here as an INTERFACE SCAFFOLD only: the Err arm reproduces today's fail-open fallback verbatim (`d.join(source.file_stem().unwrap_or(source.as_os_str()))`), so the classification can be pinned before the behaviour changes. The follow-up commit replaces that body and wires the report. RED observations (`cargo nextest run -p mds-cli --bins --test dir_build --no-fail-fast`, 204 tests, 201 passed, 3 failed): - stemless_source_never_escapes_out_dir — FAILED. `output_base_no_ext(Path::new("/"), "/root", Dir("/out"))` returned `"/"`, not `"/out/output"`: `"/".file_stem()` is None, the fallback handed `source.as_os_str()` — `"/"` — to `d.join(...)`, and joining an absolute argument re-roots. Suppressing that first assertion locally to reach the second showed the write oracle's half: `output_path_for` built `"/.md"` and tripped the containment `debug_assert!` at output.rs:279 — `output_path_for: AC-M7 violated — output "/.md" escaped out-dir "/out"`. Neither value is inside the out-dir; neither is a path a source compiles to. - flatten_warning_lives_in_the_write_oracle_only — FAILED. The body of `fn output_path_for(` contains no `eprint_warning(` and no "is outside the build root": the flatten is currently silent. - ghost_external_dep_prune_keeps_in_root_last_written — FAILED. After one batch, `last_written` held only `[/other.md]`; the in-root source's `out/x.md` entry was gone. Pruning a vanished OUT-OF-ROOT dep probes `output_base_no_ext`, takes the flatten arm, and forgets `/x.md` — an entry an in-root `x.mds` owns. Its control (`errored` no longer holds the ghost) passed, so the batch did reach the branch. Green in this commit (pins and controls, not regressions): - mirror_stem_classifies_out_of_root_as_flattened — passes on the scaffold; both arms asserted so a classifier that always says Flattened would fail. - build_non_utf8_path_exits_2_and_writes_no_map — control arm pins `--source-map` on a normal file: exit 0, sidecar present, `sources == ["ok.mds"]`. The hostile arm is a Linux-CI gate: macOS (APFS) rejects creating a non-UTF-8 filename with EILSEQ, so the create-failure path takes an explicit macOS-only `return` with a sibling `panic!` on every other platform — no silent skip. - dir_build_symlinked_ancestor_root_mirrors_without_warning and dir_build_dotdot_root_mirrors_without_warning — non-canonical roots (a symlinked ancestor; a `..` component) still mirror their subtree and must not emit the report. `run_build_directory` hands the same raw `dir` to the walker and to `output_path_for`, so `strip_prefix` succeeds; these fail if a future change canonicalizes one and not the other. Each carries the "2 built, 0 failed" needle so the negative assertion cannot pass on empty stderr. `cargo fmt --all` clean; `cargo clippy --workspace --all-targets -- -D warnings` and `cargo clippy -p mds-cli --all-targets --features startup-race-probe -- -D warnings` both clean. --- crates/mds-cli/src/output.rs | 181 ++++++++++++++++++++++++--- crates/mds-cli/src/watch.rs | 72 +++++++++++ crates/mds-cli/tests/dir_build.rs | 199 ++++++++++++++++++++++++++++++ 3 files changed, 437 insertions(+), 15 deletions(-) diff --git a/crates/mds-cli/src/output.rs b/crates/mds-cli/src/output.rs index 8b7e14a4..172f2ec3 100644 --- a/crates/mds-cli/src/output.rs +++ b/crates/mds-cli/src/output.rs @@ -553,27 +553,61 @@ 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) => { + 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(_) => { + // RED scaffold (#217): reproduces the current fail-open fallback verbatim so + // the classification interface can be pinned before the behaviour changes. + // `source.as_os_str()` is absolute for an absolute `source`, and + // `d.join()` re-roots — replaced in the follow-up commit. + let stem = source.file_stem().unwrap_or(source.as_os_str()); + 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 +1478,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..1d05b732 100644 --- a/crates/mds-cli/src/watch.rs +++ b/crates/mds-cli/src/watch.rs @@ -3987,6 +3987,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/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}" + ); +} From 0be5e56f72110f140aad73f79b13ea970df38287 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:52:31 +0300 Subject: [PATCH 08/11] fix(cli): output_path_for warns on the flattened arm, degenerate stems are relative, watch external-ghost prune skips the output probe (#217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes plus four invariant comments. Nothing about the mirrored (normal) path moves; the flatten stays CONTAINED and stays a flatten. 1. `mirror_stem`'s Err arm no longer joins an absolute value. `file_stem()` is `None` only for `/`, `..` and a bare drive prefix — never a `.mds` file — and the old fallback, `source.as_os_str()`, was exactly the absolute value that escapes: `Path::new("/out").join("/")` re-roots to `"/"`. The literal `"output"` is the only value guaranteed relative for every input. The same substitution is applied to the two `unwrap_or(source.as_os_str())` fallbacks in `output_path_for`. The containment `debug_assert!` is KEPT and stays debug-only on purpose: its release fallback is contained, so there is nothing a release-time check would prevent. 2. `output_path_for` reports the flattened arm — once, inline, with `safe_path` at each interpolation site, not gated on `--quiet` (the depth-limit and stale-unlink warnings in the same file are not either; this is an invariant report, not a user preference). `output_base_no_ext` computes the same stem for bookkeeping probes and stays SILENT: watch calls it several times per batch and for sources that are never written. 3. `DirWatchState::forget` splits into `forget_graph` (the three graph removals) and `forget`. The ghost-prune at the `!src.exists()` branch of `process_dir_batch_incremental` now calls `forget_graph` for an out-of-root source. This was a REACHABLE bookkeeping bug: a vanished external dep has no output, so probing `output_base_no_ext` for one took the flatten arm and forgot `/.md` — an entry owned by the in-root source with that file name, whose next rebuild then rewrote identical bytes. Comment-only, no behaviour change: `canonicalize_out_dir`'s fail-open `current_dir()` fallback is documented as such with the reason it is a follow-up rather than part of this change; `is_within_default_excluded_dir`'s `Err => false` is documented as the CLOSED answer (both callers evaluate it only alongside their own `starts_with(root)` test); `is_partial`'s non-UTF-8 `false` is documented as fail-closed downstream (mds-core's `path_to_str` rejects the path, so the file becomes a per-file failure instead of a silent skip); and the three watch `output_path_for` call sites each state why the flatten arm cannot fire there. Exact warning text as emitted (one line, `{}` filled by `safe_path`): 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) GREEN — `cargo nextest run -p mds-cli --no-fail-fast`: 885 tests run, 885 passed, 0 skipped (was 878 before this phase; +7 new tests). All four RED tests now pass. `print_discipline` both tests green, registry untouched (`every_allowlist_entry_is_live` passes, so no entry went dead). `cargo nextest run -p mds-cli --test cli_watch --no-fail-fast` x3, sequential: 80/80 passed each run (4.54 s / 4.44 s / 5.07 s). No timeout, sleep, debounce or ordering change was made. Mutation controls, each observed then reverted: - report deleted from the write oracle and moved into the probe oracle: flatten_warning_lives_in_the_write_oracle_only fails on the positive half ("the write oracle must report the flattened arm"). - report present in BOTH oracles: the same test fails on the negative half ("the probe oracle must stay silent"), so neither half is vacuous. - `safe_path` dropped at the site and the value hoisted into a `let`: print_discipline reports `output.rs:334: eprint_warning(format!) interpolates unsanitized \`src_display\``. - Err-arm `unwrap_or(source.as_os_str())` reintroduced: stemless_source_never_escapes_out_dir fails, left `"/"` right `"/out/output"`. - watch gate forced to `if true`: ghost_external_dep_prune_keeps_in_root_last_written fails, `last_written` keys reduced to `[/other.md]`. Named weakness: T-P16 pins the report's PRESENCE in `output_path_for` lexically, so it would still pass if the call were wrapped in a `!quiet` gate. `output_path_for` has no `quiet` in scope, so that mutation is not reachable without a signature change; recorded rather than performed. `cargo fmt --all` clean; `cargo clippy --workspace --all-targets -- -D warnings` and `cargo clippy -p mds-cli --all-targets --features startup-race-probe -- -D warnings` both clean. --- crates/mds-cli/src/output.rs | 101 +++++++++++++++++++++++++++++------ crates/mds-cli/src/watch.rs | 56 ++++++++++++++++--- 2 files changed, 134 insertions(+), 23 deletions(-) diff --git a/crates/mds-cli/src/output.rs b/crates/mds-cli/src/output.rs index 172f2ec3..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()) @@ -585,15 +649,22 @@ impl MirroredStem { 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(_) => { - // RED scaffold (#217): reproduces the current fail-open fallback verbatim so - // the classification interface can be pinned before the behaviour changes. - // `source.as_os_str()` is absolute for an absolute `source`, and - // `d.join()` re-roots — replaced in the follow-up commit. - let stem = source.file_stem().unwrap_or(source.as_os_str()); + // 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)) } } diff --git a/crates/mds-cli/src/watch.rs b/crates/mds-cli/src/watch.rs index 1d05b732..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; From aae35a9254b05d46b280ace3840876fd8070cb4f Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:54:10 +0300 Subject: [PATCH 09/11] =?UTF-8?q?test(fmt):=20RED=20=E2=80=94=20non-UTF-8?= =?UTF-8?q?=20path=20exits=202=20(#217)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tests. `fmt_non_utf8_path_exits_two` (cli_fmt.rs) is the RED one; `lint_single_file_non_utf8_path_exits_2` (cli_lint.rs) is the PIN it must match — `lint`'s single-file arm has always raised `MdsError::Io` here and `exit_code` has always mapped that to 2. Its directory sibling (`lint_directory_non_utf8_entry_is_an_io_error_exit_2`) was added earlier in this branch; this is the file arm. `mds fmt`'s 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 same `read_source_file` — already exits 2. The undecodable-path arm raised a bare `miette::miette!`, which does not downcast to `MdsError`, so `exit_code` fell through to 1. One input class, two exit codes, decided by which of two adjacent checks caught it first. RED observation. macOS (APFS) rejects creating a non-UTF-8 filename with EILSEQ, so the on-disk arm cannot run here — it is a Linux-CI gate, taken via an explicit `#[cfg(target_os = "macos")] return` with a sibling `panic!` on every other platform, so no other platform can skip silently. To observe the arm locally the `to_str()` success path was temporarily forced to `None` (`.filter(|_| false)`), the real binary rebuilt, and run over a normally named file: $ mds fmt /ok.mds × path is not valid UTF-8: /ok.mds EXIT=1 # must be 2 The same forced build in DIRECTORY mode: $ mds fmt × path is not valid UTF-8: /ok.mds 0 formatted, 0 unchanged, 1 failed EXIT=1 # correct, and must STAY 1 Directory mode exits 1 through the `Failed` tally, not through `exit_code`, so it is unaffected by the fix and is deliberately not asserted here. The forced branch was reverted before committing. Both tests pass on macOS today — the control arms run, the hostile arms return early. The hostile assertions first execute on Linux CI. `cargo nextest run -p mds-cli --test cli_fmt --test cli_lint --no-fail-fast`: 176 tests run, 176 passed, 0 skipped. `cargo fmt --all --check` clean; `cargo clippy --workspace --all-targets -- -D warnings` and `cargo clippy -p mds-cli --all-targets --features startup-race-probe -- -D warnings` both clean. --- crates/mds-cli/tests/cli_fmt.rs | 65 +++++++++++++++++++++++++++++++ crates/mds-cli/tests/cli_lint.rs | 67 ++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+) 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 9e7c2491..918cfb73 100644 --- a/crates/mds-cli/tests/cli_lint.rs +++ b/crates/mds-cli/tests/cli_lint.rs @@ -6838,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}" + ); +} From f4707d7d788bffc7d46b8963012b8f16e566e545 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 13:55:51 +0300 Subject: [PATCH 10/11] fix(fmt): a non-UTF-8 path is mds::io (exit 2) like lint and build (#217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `read_source_file` raised the undecodable-path failure as a bare `miette::miette!`. That report does not downcast to `MdsError`, so `build::exit_code` fell through to 1 — while `check_symlink`, called one statement earlier on the same argument, already returned an `MdsError` and exited 2. The module doc has documented 2 for this class since the subcommand shipped. The message text is byte-identical to `lint.rs`'s `read_source_file` (`path is not valid UTF-8: {}` over `path.display()`), so the two subcommands now report the same condition the same way. No new `MdsError` variant; no change to `exit_code`. GREEN observation. macOS (APFS) cannot create the on-disk fixture, so the arm was exercised by temporarily forcing the `to_str()` success path to `None` (`.filter(|_| false)`), rebuilding the real binary, and running it — the same procedure that produced the RED reading in the previous commit: $ mds fmt /ok.mds mds::io × path is not valid UTF-8: /ok.mds EXIT=2 # was 1 DIRECTORY mode is UNCHANGED, as intended: $ mds fmt mds::io × path is not valid UTF-8: /ok.mds 0 formatted, 0 unchanged, 1 failed EXIT=1 # was 1 Directory mode exits through the `Failed` tally, not through `exit_code`, so the error class it reports does not reach the exit status. Nothing about it moves. The forced branch was reverted before committing. Full gate battery on the final tree: - `cargo nextest run -p mds-core -p mds-cli --no-fail-fast`: 2352 tests run, 2352 passed, 0 skipped (mds-core 1465 unchanged, mds-cli 887 — was 878 at the start of this phase, +9: 7 in the output/watch RED commit, 2 here). - `cargo clippy --workspace --all-targets -- -D warnings` — clean. - `cargo clippy -p mds-cli --all-targets --features startup-race-probe -- -D warnings` — clean. - `cargo fmt --all --check` — clean. - `RUSTDOCFLAGS="-D warnings" cargo doc -p mds-core -p mds-cli --no-deps` — clean. - `node scripts/verify-no-control-bytes.mjs` — 563 files, 6811840 bytes, clean. --- crates/mds-cli/src/fmt.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 From e1a69d079f37e6948cc64ae9fe5631b63828a00d Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Tue, 15 Sep 2026 14:03:42 +0300 Subject: [PATCH 11/11] =?UTF-8?q?docs:=20release-enforced=20invariants=20a?= =?UTF-8?q?nd=20fail-closed=20path=20helpers=20=E2=80=94=20CHANGELOG,=20sp?= =?UTF-8?q?ec=20=C2=A77.2/=C2=A77.5/=C2=A77.9,=20SECURITY.md,=20rustdoc,?= =?UTF-8?q?=20KBs=20(#220,=20#217)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documents the behaviour landed by the ten preceding commits on this branch. No production code changes; the only .rs edit is a test-group comment. CHANGELOG [Unreleased]: - Changed: the pub-API range-check promotion (TextEdit::new, FixLineSpan::range_inclusive/range_exclusive now panic in release). - Fixed: the #220 release-enforcement entry (non-boundary import/extends spans degrade instead of panicking; skeleton-block, neutralize-width and source-map cursor invariants enforced in every profile), plus four #217 entries — lint directory-mode fails closed on an unnameable path, fmt non-UTF-8 exits 2, source-map anchors fail closed, and the build/watch flatten warning. spec.md: - §7.2 dir-mode --out-dir: the out-of-root flat write and its non-quiet warning. - §7.5 dir-mode lint: entries are named relative to the lint root before any file is linted; an unnameable path fails the whole run. - §7.9: both exit-2 rows name the non-UTF-8 path case. SECURITY.md: new filesystem-boundary bullet — source-map anchors are byte-faithful, and an empty or non-UTF-8 root can never make containment vacuous. rustdoc: the B.6 checklist was verified site by site; every item was already in place from the earlier commits except the line_len_at test-group header in resolver_tests.rs, which now records that the helper also backs attach_import_span and check_child_only_blocks. Feature KBs: mds-lint (neutralize assert_eq! promotion x2, relative_display now fallible and LintDirCtx.lint_root gone), source-map-security (evaluator cursor assert, the empty/non-UTF-8 anchor gotcha, source_root None for a non-UTF-8 root), mds-cli (AC-M7 paragraph rewritten for MirroredStem/mirror_stem, the write-oracle-only flatten warning, relative degenerate stems, the watch forget_graph prune), mds-fmt (single exit-code correction). Refs #220, #217 --- .devflow/features/mds-cli/KNOWLEDGE.md | 4 +- .devflow/features/mds-fmt/KNOWLEDGE.md | 4 +- .devflow/features/mds-lint/KNOWLEDGE.md | 8 ++-- .../features/source-map-security/KNOWLEDGE.md | 8 ++-- CHANGELOG.md | 48 +++++++++++++++++++ SECURITY.md | 5 ++ crates/mds-core/src/resolver_tests.rs | 6 +++ spec.md | 7 +-- 8 files changed, 76 insertions(+), 14 deletions(-) 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-core/src/resolver_tests.rs b/crates/mds-core/src/resolver_tests.rs index 9276d9e5..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() { 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 | ---