From 70bd9b8f6378d8de20bb7271f6bb9f8a326e4266 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 9 Sep 2026 16:04:22 +0300 Subject: [PATCH 1/6] test(vars): RED specs for duplicate --vars keys (#326); fix lint_str rustdoc and tier tables (#329) --- crates/mds-cli/src/build.rs | 99 ++++++++++++ crates/mds-cli/tests/cli_build.rs | 203 ++++++++++++++++++++++- crates/mds-cli/tests/cli_watch.rs | 218 ++++++++++++++++++++++++- crates/mds-cli/tests/common/mod.rs | 48 +++++- crates/mds-cli/tests/warnings.rs | 222 ++++++++++++++++++++++++- crates/mds-core/src/lib.rs | 234 ++++++++++++++++++++++++++- crates/mds-core/src/lint/fix.rs | 12 +- crates/mds-core/src/lint/tier.rs | 100 +++++++++++- crates/mds-core/src/vars_json.rs | 207 ++++++++++++++++++++++++ crates/mds-core/tests/api_surface.rs | 14 ++ 10 files changed, 1339 insertions(+), 18 deletions(-) create mode 100644 crates/mds-core/src/vars_json.rs diff --git a/crates/mds-cli/src/build.rs b/crates/mds-cli/src/build.rs index 8660eebe..3d6b9fc5 100644 --- a/crates/mds-cli/src/build.rs +++ b/crates/mds-cli/src/build.rs @@ -2262,4 +2262,103 @@ mod tests { assert_eq!(map.get("num"), Some(&mds::Value::Number(42.0))); assert_eq!(map.get("id"), Some(&mds::Value::String("007".to_string()))); } + + // ── #326: duplicate --vars file keys surface on RuntimeVars ─────────────── + + #[test] + fn build_runtime_vars_vars_file_duplicate_key_is_reported() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"x": 1, "x": 2}"#).unwrap(); + + let resolved = build_runtime_vars(RuntimeVarArgs { + vars: Some(path.clone()), + set_vars: vec![], + set_string_vars: vec![], + }) + .expect("duplicate vars-file key must not error"); + assert_eq!(resolved.duplicate_vars_file_keys, vec!["x".to_string()]); + assert_eq!(resolved.vars_file, Some(path)); + let map = resolved.vars.expect("non-empty vars"); + assert_eq!(map.get("x"), Some(&mds::Value::Number(2.0))); + } + + #[test] + fn build_runtime_vars_vars_file_nested_duplicate_reports_dotted_path() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"cfg":{"a":1,"a":2}}"#).unwrap(); + + let resolved = build_runtime_vars(RuntimeVarArgs { + vars: Some(path), + set_vars: vec![], + set_string_vars: vec![], + }) + .expect("nested duplicate vars-file key must not error"); + assert_eq!(resolved.duplicate_vars_file_keys, vec!["cfg.a".to_string()]); + } + + /// Positive control (PF-013): a clean vars file reports no duplicates, while + /// still populating `vars_file`. + #[test] + fn build_runtime_vars_vars_file_without_duplicates_reports_none() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"name": "World"}"#).unwrap(); + + let resolved = build_runtime_vars(RuntimeVarArgs { + vars: Some(path.clone()), + set_vars: vec![], + set_string_vars: vec![], + }) + .expect("clean vars file must not error"); + assert!( + resolved.duplicate_vars_file_keys.is_empty(), + "expected no duplicates, got: {:?}", + resolved.duplicate_vars_file_keys + ); + assert_eq!(resolved.duplicate_vars_file_keys_omitted, 0); + assert!(resolved.vars_file.is_some(), "vars_file must be populated"); + } + + #[test] + fn build_runtime_vars_no_vars_file_reports_no_duplicates_and_no_path() { + let resolved = build_runtime_vars(RuntimeVarArgs { + vars: None, + set_vars: vec![("a".to_string(), "1".to_string())], + set_string_vars: vec![], + }) + .expect("no vars file must not error"); + assert!( + resolved.duplicate_vars_file_keys.is_empty(), + "expected no duplicates when no vars file was given, got: {:?}", + resolved.duplicate_vars_file_keys + ); + assert_eq!(resolved.duplicate_vars_file_keys_omitted, 0); + assert_eq!( + resolved.vars_file, None, + "vars_file must be None when --vars was not given" + ); + } + + /// U5 (extended): the cross-flag hard error must precede the vars-file read + /// entirely — a nonexistent vars path must not surface a file-not-found error + /// when --set and --set-string also collide. + #[test] + fn build_runtime_vars_cross_flag_error_precedes_the_vars_file_read() { + let result = build_runtime_vars(RuntimeVarArgs { + vars: Some(PathBuf::from("/does/not/exist/vars.json")), + set_vars: vec![("x".to_string(), "1".to_string())], + set_string_vars: vec![("x".to_string(), "2".to_string())], + }); + assert!( + result.is_err(), + "cross-flag collision must be a hard error even with a nonexistent vars path" + ); + let msg = format!("{}", result.unwrap_err()); + assert!( + msg.contains("variable 'x' is set by both --set and --set-string"), + "error must be the cross-flag collision, not a file-read error; got: {msg}" + ); + } } diff --git a/crates/mds-cli/tests/cli_build.rs b/crates/mds-cli/tests/cli_build.rs index e682983f..2dc1c612 100644 --- a/crates/mds-cli/tests/cli_build.rs +++ b/crates/mds-cli/tests/cli_build.rs @@ -1,5 +1,5 @@ mod common; -use common::{fixture, mds_bin}; +use common::{count_occurrences, dup_vars_file_omitted, dup_vars_file_warning, fixture, mds_bin}; #[test] fn build_to_file() { @@ -939,6 +939,207 @@ fn vars_file_non_object_json_error_names_the_file() { ); } +// ── #326: duplicate --vars file keys warn, last value wins ─────────────────── + +/// A duplicated top-level key in a `--vars` JSON file warns exactly once, the +/// last value wins, and the run still exits 0. +#[test] +fn vars_file_duplicate_key_warns_and_last_value_wins() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "x={{x}}\n").unwrap(); + std::fs::write(&vars, r#"{"x": 1, "x": 2}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!( + output.status.success(), + "a duplicate vars-file key must warn, not error" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("x=2"), + "the last value must win; got stdout: {stdout}" + ); + assert!( + !stdout.contains("x=1"), + "the first value must not survive; got stdout: {stdout}" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + let expected = dup_vars_file_warning("x", &vars); + let count = count_occurrences(&stderr, &expected); + assert_eq!( + count, 1, + "expected the duplicate-key warning exactly once, found {count} times; stderr:\n{stderr}" + ); +} + +/// A duplicated key nested one level deep warns with the dotted path. +#[test] +fn vars_file_nested_duplicate_key_warns_with_a_dotted_path() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello!\n").unwrap(); + std::fs::write(&vars, r#"{"cfg": {"a": 1, "a": 2}}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + let expected = dup_vars_file_warning("cfg.a", &vars); + assert_eq!( + count_occurrences(&stderr, &expected), + 1, + "expected the dotted-path warning exactly once; stderr:\n{stderr}" + ); +} + +/// A duplicated key inside an array element warns with the bracket-index path. +#[test] +fn vars_file_array_nested_duplicate_key_warns_with_a_bracket_index() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello!\n").unwrap(); + std::fs::write(&vars, r#"{"items": [{"x": 0}, {"a": 1, "a": 2}]}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + let expected = dup_vars_file_warning("items[1].a", &vars); + assert_eq!( + count_occurrences(&stderr, &expected), + 1, + "expected the bracket-index warning exactly once; stderr:\n{stderr}" + ); +} + +/// Positive control (PF-013) for the three tests above: a vars file with no +/// duplicates must emit no duplicate-key warning at all, and the value renders +/// first (i.e. the run really did use this vars file). +#[test] +fn vars_file_without_duplicates_emits_no_duplicate_warning() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "x={{x}}\n").unwrap(); + std::fs::write(&vars, r#"{"x": 1}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("x=1"), + "expected the single value to render first; got stdout: {stdout}" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("is set more than once in vars file"), + "a clean vars file must not warn; got stderr: {stderr}" + ); +} + +/// More than [`mds::VarsLoad::duplicate_keys_omitted`]'s cap (1 000) distinct +/// duplicate paths prints exactly 1 000 warning lines plus one omitted-count tail +/// line naming the remainder (D6). +#[test] +fn vars_file_more_than_1000_duplicates_prints_a_tail() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello!\n").unwrap(); + + let mut json = String::from("{"); + for i in 0..1_003usize { + if i > 0 { + json.push(','); + } + json.push_str(&format!(r#""k{i}":0,"k{i}":1"#)); + } + json.push('}'); + std::fs::write(&vars, json).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + let warning_count = count_occurrences(&stderr, "is set more than once in vars file"); + assert_eq!( + warning_count, 1_000, + "expected exactly 1000 warning lines; stderr had {warning_count}" + ); + let tail = dup_vars_file_omitted(3, &vars); + assert_eq!( + count_occurrences(&stderr, &tail), + 1, + "expected the omitted-count tail line exactly once; stderr:\n{stderr}" + ); +} + /// `mds build --vars ` exits 2 with `mds::file_not_found` (C1/F2). /// /// Before this fix, `load_optional_vars_file` used `miette::miette!("{e}")` which diff --git a/crates/mds-cli/tests/cli_watch.rs b/crates/mds-cli/tests/cli_watch.rs index adece3b5..a88dacea 100644 --- a/crates/mds-cli/tests/cli_watch.rs +++ b/crates/mds-cli/tests/cli_watch.rs @@ -20,7 +20,9 @@ //! - Always kill+wait child in `ChildGuard::drop`. mod common; -use common::{mds_bin, spawn_watch_ready, spawn_watch_unsynchronized, StderrTap}; +use common::{ + dup_vars_file_warning, mds_bin, spawn_watch_ready, spawn_watch_unsynchronized, StderrTap, +}; use std::path::Path; use std::process::{Child, Command, Stdio}; @@ -4238,3 +4240,217 @@ fn i9_dir_watch_duplicate_set_warns_exactly_once_at_startup() { drop(child); } + +// ── I16-I18: duplicate --vars file key warnings under `mds watch` (#326) ───── +// +// Unlike I8/I9 (--set/--set-string warn once per SESSION, at startup), a +// duplicate in the --vars FILE warns at startup AND on every rebuild: ADR-016 +// reloads the vars file on every rebuild, so a duplicate present in it is +// re-reported each time (D9). + +/// I16: mds watch (file mode) with a duplicated top-level key in the vars file +/// warns at STARTUP and on EVERY rebuild. Guards `watch.rs:936`. +#[test] +fn i16_file_watch_vars_file_duplicate_warns_at_startup_and_on_every_rebuild() { + let base = tempfile::tempdir().unwrap(); + let src_dir = base.path().join("src"); + let vars_dir = base.path().join("vars_dir"); + std::fs::create_dir_all(&src_dir).unwrap(); + std::fs::create_dir_all(&vars_dir).unwrap(); + + let src = src_dir.join("t.mds"); + std::fs::write(&src, "version 1").unwrap(); + let vars_file = vars_dir.join("vars.json"); + std::fs::write(&vars_file, r#"{"x": 1, "x": 2}"#).unwrap(); + let out = src_dir.join("t.md"); + + let expected = dup_vars_file_warning("x", &vars_file); + + let (child, stderr_tap) = spawn_ready( + mds_bin() + .args([ + "watch", + src.to_str().unwrap(), + "--vars", + vars_file.to_str().unwrap(), + "--debounce", + "0", + ]) + .stdout(Stdio::null()), + ); + + let stderr_after_start = wait_for_stderr_contains_str(&stderr_tap, &expected, TIMEOUT); + assert_eq!( + count_occurrences(&stderr_after_start, &expected), + 1, + "I16: expected exactly 1 warning at startup; stderr:\n{stderr_after_start}" + ); + + // Edit 1: trigger a rebuild — ADR-016 reloads the vars file, re-reporting the + // duplicate. + std::fs::write(&src, "version 2").unwrap(); + assert!( + wait_for_file_contains(&out, "version 2", TIMEOUT), + "I16: rebuild after edit 1 must complete" + ); + let after_edit_1 = stderr_tap.text(); + assert_eq!( + count_occurrences(&after_edit_1, &expected), + 2, + "I16: one rebuild must re-report the vars-file duplicate; stderr:\n{after_edit_1}" + ); + + // Edit 2: trigger another rebuild. + std::fs::write(&src, "version 3").unwrap(); + assert!( + wait_for_file_contains(&out, "version 3", TIMEOUT), + "I16: rebuild after edit 2 must complete" + ); + let after_edit_2 = stderr_tap.text(); + assert_eq!( + count_occurrences(&after_edit_2, &expected), + 3, + "I16: a second rebuild must report the duplicate again; stderr:\n{after_edit_2}" + ); + + drop(child); +} + +/// I17: mds watch (dir mode) reports the vars-file duplicate exactly once per +/// rebuild: once at startup (proving the `:2196` dedup-baseline second read does +/// NOT double-print), and once more per subsequent rebuild (proving exactly one +/// of `:1793`/`:1919` emits, not both). +#[test] +fn i17_dir_watch_vars_file_duplicate_warns_once_per_rebuild() { + let base = tempfile::tempdir().unwrap(); + let src_dir = base.path().join("src"); + let vars_dir = base.path().join("vars_dir"); + std::fs::create_dir_all(&src_dir).unwrap(); + std::fs::create_dir_all(&vars_dir).unwrap(); + + let src = src_dir.join("t.mds"); + std::fs::write(&src, "version 1").unwrap(); + let vars_file = vars_dir.join("vars.json"); + std::fs::write(&vars_file, r#"{"x": 1, "x": 2}"#).unwrap(); + let out = src_dir.join("t.md"); + + let expected = dup_vars_file_warning("x", &vars_file); + + let (child, stderr_tap) = spawn_ready( + mds_bin() + .args([ + "watch", + src_dir.to_str().unwrap(), + "--vars", + vars_file.to_str().unwrap(), + "--debounce", + "0", + ]) + .stdout(Stdio::null()), + ); + + // No edits yet: the startup count must be exactly 1, proving the dedup-baseline + // second read at :2196 does not also emit. + let stderr_startup = wait_for_stderr_contains_str(&stderr_tap, &expected, TIMEOUT); + assert_eq!( + count_occurrences(&stderr_startup, &expected), + 1, + "I17: dir-watch startup must emit the vars-file warning exactly once \ + (guards :2196); stderr:\n{stderr_startup}" + ); + + // One rebuild: the count must rise to exactly 2, proving exactly one of + // :1793/:1919 fires per rebuild (not both). + std::fs::write(&src, "version 2").unwrap(); + assert!( + wait_for_file_contains(&out, "version 2", TIMEOUT), + "I17: rebuild after edit must complete" + ); + let stderr_after_edit = stderr_tap.text(); + assert_eq!( + count_occurrences(&stderr_after_edit, &expected), + 2, + "I17: one rebuild must add exactly one more warning (guards a double-emit \ + between :1793 and :1919); stderr:\n{stderr_after_edit}" + ); + + drop(child); +} + +/// I18 (user decision, positive control first): a vars file that starts clean +/// produces no duplicate-key warning at startup or on the first rebuild; a +/// duplicate introduced mid-session is reported on the NEXT rebuild, naming the +/// key. +#[test] +fn i18_duplicate_introduced_mid_session_is_reported_on_the_next_rebuild() { + let base = tempfile::tempdir().unwrap(); + let src_dir = base.path().join("src"); + let vars_dir = base.path().join("vars_dir"); + std::fs::create_dir_all(&src_dir).unwrap(); + std::fs::create_dir_all(&vars_dir).unwrap(); + + let src = src_dir.join("t.mds"); + std::fs::write(&src, "version 1").unwrap(); + let vars_file = vars_dir.join("vars.json"); + std::fs::write(&vars_file, r#"{"x": 1}"#).unwrap(); + let out = src_dir.join("t.md"); + + let expected = dup_vars_file_warning("x", &vars_file); + + let (child, stderr_tap) = spawn_ready( + mds_bin() + .args([ + "watch", + src.to_str().unwrap(), + "--vars", + vars_file.to_str().unwrap(), + "--debounce", + "0", + ]) + .stdout(Stdio::null()), + ); + + assert!( + wait_for_file_contains(&out, "version 1", TIMEOUT), + "I18: startup compile must complete" + ); + + // Positive control (PF-013): no duplicate at startup. + let startup_stderr = stderr_tap.text(); + assert_eq!( + count_occurrences(&startup_stderr, &expected), + 0, + "I18: a clean vars file must not warn at startup; stderr:\n{startup_stderr}" + ); + + // First rebuild, still clean: still no warning. + std::fs::write(&src, "version 2").unwrap(); + assert!( + wait_for_file_contains(&out, "version 2", TIMEOUT), + "I18: first rebuild must complete" + ); + let clean_rebuild_stderr = stderr_tap.text(); + assert_eq!( + count_occurrences(&clean_rebuild_stderr, &expected), + 0, + "I18: the first rebuild must still not warn (vars file is still clean); \ + stderr:\n{clean_rebuild_stderr}" + ); + + // Introduce a duplicate mid-session, then trigger the next rebuild. + std::fs::write(&vars_file, r#"{"x": 1, "x": 2}"#).unwrap(); + std::fs::write(&src, "version 3").unwrap(); + assert!( + wait_for_file_contains(&out, "version 3", TIMEOUT), + "I18: rebuild after introducing the duplicate must complete" + ); + let final_stderr = stderr_tap.text(); + assert_eq!( + count_occurrences(&final_stderr, &expected), + 1, + "I18: the duplicate introduced mid-session must be reported on the next \ + rebuild, naming the key; stderr:\n{final_stderr}" + ); + + drop(child); +} diff --git a/crates/mds-cli/tests/common/mod.rs b/crates/mds-cli/tests/common/mod.rs index 0e49ceff..fe3c1cfd 100644 --- a/crates/mds-cli/tests/common/mod.rs +++ b/crates/mds-cli/tests/common/mod.rs @@ -1,5 +1,5 @@ use std::io::Read; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Child, Command, Stdio}; use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -23,6 +23,52 @@ pub fn mds_bin() -> std::process::Command { cmd } +// ── Duplicate --vars file key warnings (#326) ──────────────────────────────── + +/// USER-FACING CONTRACT (#326). `{key}` = dotted/bracketed path, `{path}` = the +/// `--vars` arg as typed on the command line. +#[allow(dead_code)] +pub const DUP_VARS_FILE_WARNING_FMT: &str = + "warning: key '{key}' is set more than once in vars file {path}; the last value wins"; + +/// Tail line printed when more than [`mds::VarsLoad::duplicate_keys_omitted`] +/// (capped at 1 000) distinct duplicate paths exist (#326). +#[allow(dead_code)] +pub const DUP_VARS_FILE_OMITTED_FMT: &str = + "warning: {n} more duplicate keys in vars file {path} are not listed"; + +/// Render [`DUP_VARS_FILE_WARNING_FMT`] for a given key path and `--vars` path. +#[allow(dead_code)] +pub fn dup_vars_file_warning(key: &str, path: &Path) -> String { + DUP_VARS_FILE_WARNING_FMT + .replace("{key}", key) + .replace("{path}", &path.display().to_string()) +} + +/// Render [`DUP_VARS_FILE_OMITTED_FMT`] for a given omitted count and `--vars` path. +#[allow(dead_code)] +pub fn dup_vars_file_omitted(n: usize, path: &Path) -> String { + DUP_VARS_FILE_OMITTED_FMT + .replace("{n}", &n.to_string()) + .replace("{path}", &path.display().to_string()) +} + +/// Count non-overlapping occurrences of `needle` in `haystack`. +/// +/// Same body as the private `count_occurrences` in `warnings.rs` / `cli_watch.rs` — +/// those files import only the two render helpers above (E0255 otherwise) and keep +/// their own private copy of this one. +#[allow(dead_code)] +pub fn count_occurrences(haystack: &str, needle: &str) -> usize { + let mut count = 0; + let mut start = 0; + while let Some(pos) = haystack[start..].find(needle) { + count += 1; + start += pos + needle.len(); + } + count +} + // ── Watch readiness handshake ──────────────────────────────────────────────── /// Contents `mds watch` writes to the file named by `MDS_TEST_READY`. diff --git a/crates/mds-cli/tests/warnings.rs b/crates/mds-cli/tests/warnings.rs index 28118b37..561e48ae 100644 --- a/crates/mds-cli/tests/warnings.rs +++ b/crates/mds-cli/tests/warnings.rs @@ -1,5 +1,5 @@ mod common; -use common::{assert_no_control_chars, fixture, mds_bin}; +use common::{assert_no_control_chars, dup_vars_file_warning, fixture, mds_bin}; #[test] fn check_collecting_warnings_returns_warnings_for_empty_include() { @@ -434,6 +434,226 @@ fn i7_hostile_key_is_wire_escaped_in_warning() { assert_no_control_chars(&stderr, "I7 stderr"); } +// ── I10-I15: duplicate --vars file key warnings (#326) ─────────────────────── + +#[test] +fn i10_vars_file_duplicate_key_warns_exactly_once_on_build() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + std::fs::write(&vars, r#"{"x": 1, "x": 2}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!( + output.status.success(), + "I10: build with duplicate vars-file key must succeed" + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + let expected = dup_vars_file_warning("x", &vars); + let count = count_occurrences(&stderr, &expected); + assert_eq!( + count, 1, + "I10: expected warning exactly once, found {count} times; stderr:\n{stderr}" + ); +} + +#[test] +fn i11_quiet_suppresses_vars_file_duplicate_warning() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + std::fs::write(&vars, r#"{"x": 1, "x": 2}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + "--quiet", + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success(), "I11: quiet build must succeed"); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + stderr.is_empty(), + "I11: --quiet must suppress the duplicate vars-file warning, got: {stderr}" + ); +} + +#[test] +fn i12_vars_file_duplicate_key_warns_on_check() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + std::fs::write(&vars, r#"{"x": 1, "x": 2}"#).unwrap(); + + let output = mds_bin() + .args([ + "check", + src.to_str().unwrap(), + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success(), "I12: check must succeed"); + let stderr = String::from_utf8(output.stderr).unwrap(); + let expected = dup_vars_file_warning("x", &vars); + assert_eq!( + count_occurrences(&stderr, &expected), + 1, + "I12: check must warn exactly once for a duplicate vars-file key; stderr:\n{stderr}" + ); +} + +#[test] +fn i13_vars_file_duplicate_key_warns_on_lint() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + std::fs::write(&vars, r#"{"x": 1, "x": 2}"#).unwrap(); + + let output = mds_bin() + .args([ + "lint", + src.to_str().unwrap(), + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success(), "I13: lint must succeed"); + let stderr = String::from_utf8(output.stderr).unwrap(); + let expected = dup_vars_file_warning("x", &vars); + assert_eq!( + count_occurrences(&stderr, &expected), + 1, + "I13: lint must warn exactly once for a duplicate vars-file key; stderr:\n{stderr}" + ); +} + +#[test] +fn i14_vars_file_triple_repeat_warns_once() { + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + std::fs::write(&vars, r#"{"x": 1, "x": 2, "x": 3}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!(output.status.success(), "I14: build must succeed"); + let stderr = String::from_utf8(output.stderr).unwrap(); + let expected = dup_vars_file_warning("x", &vars); + assert_eq!( + count_occurrences(&stderr, &expected), + 1, + "I14: triple repeat must still produce exactly one warning; stderr:\n{stderr}" + ); +} + +#[test] +fn i15_hostile_vars_file_key_is_wire_escaped_in_warning() { + // Mirrors i7: a key containing an ESC byte (U+001B) and an RLO (U+202E) must + // appear in the warning with those codepoints replaced by their \uXXXX escape + // sequences, never as raw control bytes. + // + // The vars FILE must spell these as JSON \u escapes (a raw control byte is not + // legal inside a JSON string). The escape text is built via `format!` with a + // hex-formatted integer, never as a literal 4-hex \uXXXX sequence in this + // source file, so the editor tool layer has nothing to decode (PF-018). + let esc_json_escape = format!("\\u{:04x}", 0x1bu32); + let rlo_json_escape = format!("\\u{:04x}", 0x202eu32); + let hostile_key_in_json = format!("{esc_json_escape}[31m{rlo_json_escape}"); + + // The expected ESCAPED forms as they appear in the sanitized warning output. + let expected_esc_form = "\\u001B"; + let expected_rlo_form = "\\u202E"; + + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + let vars_json = format!(r#"{{"{hostile_key_in_json}": 1, "{hostile_key_in_json}": 2}}"#); + std::fs::write(&vars, vars_json).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!( + output.status.success(), + "I15: build with hostile vars-file key must succeed" + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + + // PF-013 NON-VACUITY FIRST: assert the escaped form IS present before asserting + // absence of raw control bytes. + assert!( + stderr.contains(expected_esc_form), + "I15: expected escaped ESC form '{expected_esc_form}' in stderr; got:\n{stderr:?}" + ); + assert!( + stderr.contains(expected_rlo_form), + "I15: expected escaped RLO form '{expected_rlo_form}' in stderr; got:\n{stderr:?}" + ); + + // Now assert the raw control bytes are absent. + assert_no_control_chars(&stderr, "I15 stderr"); +} + // ── R2: @include warning precision ─────────────────────────────────────────── #[test] diff --git a/crates/mds-core/src/lib.rs b/crates/mds-core/src/lib.rs index 211861f0..f691575a 100644 --- a/crates/mds-core/src/lib.rs +++ b/crates/mds-core/src/lib.rs @@ -57,6 +57,7 @@ pub(crate) mod source_path; pub(crate) mod sourcemap; pub(crate) mod validator; pub(crate) mod value; +pub(crate) mod vars_json; pub use formatter::{format_str, format_str_named, format_str_with}; pub use fs::{effective_parent, FileSystem, NativeFs, VirtualFs}; @@ -1162,8 +1163,8 @@ pub fn check_virtual_collecting_warnings( /// Lint an MDS source string with default options. /// /// Runs the check gate (resolve+validate) first: returns `Err(MdsError)` when the -/// template does not compile. On a clean gate, applies the 9 lint rules and returns -/// a `LintResult` (empty in S1 — rules arrive in S2). +/// template does not compile. On a clean gate, applies every registered lint rule +/// ([`KNOWN_LINT_RULES`]) and returns a `LintResult`. /// /// # Examples /// @@ -1771,6 +1772,235 @@ mod tests { assert_eq!(vars.len(), 2); } + // ── load_vars_str_reporting_duplicates (#326) ───────────────────────────── + + #[test] + fn load_vars_str_reporting_duplicates_flat_duplicate() { + let loaded = load_vars_str_reporting_duplicates(r#"{"x": 1, "x": 2}"#) + .expect("should load duplicate-key vars"); + assert_eq!(loaded.duplicate_keys, vec!["x".to_string()]); + assert_eq!(loaded.vars.get("x"), Some(&Value::Number(2.0))); + } + + /// Positive control (PF-013): a clean document reports no duplicates. + #[test] + fn load_vars_str_reporting_duplicates_clean_input_reports_none() { + let loaded = load_vars_str_reporting_duplicates(r#"{"name": "World", "count": 42}"#) + .expect("should load clean vars"); + assert!( + loaded.duplicate_keys.is_empty(), + "expected no duplicates, got {:?}", + loaded.duplicate_keys + ); + assert_eq!(loaded.duplicate_keys_omitted, 0); + } + + #[test] + fn load_vars_str_reporting_duplicates_nested_and_array_paths() { + let loaded = + load_vars_str_reporting_duplicates(r#"{"x":{"a":1,"a":2},"y":[{"b":1,"b":2}]}"#) + .expect("should load duplicate-key vars"); + assert_eq!( + loaded.duplicate_keys, + vec!["x.a".to_string(), "y[0].b".to_string()] + ); + } + + /// Non-vacuity: `vars` matches `load_vars_str` on a 9-key fixture with no + /// duplicates — the reporting variant must not silently drop or reorder keys. + #[test] + fn load_vars_str_reporting_duplicates_vars_matches_load_vars_str() { + let json = r#"{"nul":null,"t":true,"f":false,"neg":-1,"big":18446744073709551615,"flt":1.5e300,"s":"a\nbA","arr":[],"obj":{}}"#; + let loaded = + load_vars_str_reporting_duplicates(json).expect("should load the all-types fixture"); + let plain = load_vars_str(json).expect("should load via the plain API too"); + assert_eq!(loaded.vars, plain); + assert_eq!(loaded.vars.len(), 9, "expected 9 top-level keys"); + } + + #[test] + fn load_vars_str_reporting_duplicates_rejects_oversized_input() { + let oversized = "x".repeat((MAX_FILE_SIZE as usize) + 1); + let err = load_vars_str_reporting_duplicates(&oversized) + .expect_err("expected error for oversized input"); + assert!( + err.to_string().contains("exceeds maximum size"), + "error message should mention size limit, got: {err}" + ); + } + + #[test] + fn load_vars_str_reporting_duplicates_rejects_non_object() { + let err = load_vars_str_reporting_duplicates("[1,2,3]") + .expect_err("expected error for non-object JSON"); + assert!( + err.to_string().contains("vars must be a JSON object"), + "got: {err}" + ); + } + + #[test] + fn load_vars_str_reporting_duplicates_rejects_malformed_json() { + let err = load_vars_str_reporting_duplicates("not json") + .expect_err("expected error for malformed JSON"); + assert!(err.to_string().contains("JSON"), "got: {err}"); + } + + #[test] + fn load_vars_str_reporting_duplicates_omitted_counter_surfaces() { + let mut json = String::from("{"); + for i in 0..1_003usize { + if i > 0 { + json.push(','); + } + json.push_str(&format!(r#""k{i}":0,"k{i}":1"#)); + } + json.push('}'); + let loaded = load_vars_str_reporting_duplicates(&json) + .expect("should load the 1003-duplicate fixture"); + assert_eq!(loaded.duplicate_keys.len(), 1_000); + assert_eq!(loaded.duplicate_keys_omitted, 3); + } + + // ── load_vars_file_reporting_duplicates (#326) ──────────────────────────── + + #[test] + fn load_vars_file_reporting_duplicates_flat_duplicate() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"x": 1, "x": 2}"#).unwrap(); + let loaded = + load_vars_file_reporting_duplicates(&path).expect("should load duplicate-key vars"); + assert_eq!(loaded.duplicate_keys, vec!["x".to_string()]); + assert_eq!(loaded.vars.get("x"), Some(&Value::Number(2.0))); + } + + /// Positive control (PF-013): a clean vars file reports no duplicates. + #[test] + fn load_vars_file_reporting_duplicates_clean_input_reports_none() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"name": "World", "count": 42}"#).unwrap(); + let loaded = load_vars_file_reporting_duplicates(&path).expect("should load clean vars"); + assert!( + loaded.duplicate_keys.is_empty(), + "expected no duplicates, got {:?}", + loaded.duplicate_keys + ); + assert_eq!(loaded.duplicate_keys_omitted, 0); + } + + #[test] + fn load_vars_file_reporting_duplicates_nested_and_array_paths() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"x":{"a":1,"a":2},"y":[{"b":1,"b":2}]}"#).unwrap(); + let loaded = + load_vars_file_reporting_duplicates(&path).expect("should load duplicate-key vars"); + assert_eq!( + loaded.duplicate_keys, + vec!["x.a".to_string(), "y[0].b".to_string()] + ); + } + + #[test] + fn load_vars_file_reporting_duplicates_rejects_oversized_input() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + let oversized = "x".repeat((MAX_FILE_SIZE as usize) + 1); + std::fs::write(&path, oversized).unwrap(); + let err = load_vars_file_reporting_duplicates(&path) + .expect_err("expected error for oversized input"); + let msg = err.to_string(); + assert!(msg.contains("vars file exceeds maximum size"), "got: {msg}"); + } + + #[test] + fn load_vars_file_reporting_duplicates_rejects_non_object() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, "[1,2,3]").unwrap(); + let err = load_vars_file_reporting_duplicates(&path) + .expect_err("expected error for non-object JSON"); + let msg = err.to_string(); + assert!(msg.contains("invalid vars file"), "got: {msg}"); + assert!( + msg.contains("top-level value is not a JSON object"), + "got: {msg}" + ); + } + + #[test] + fn load_vars_file_reporting_duplicates_rejects_malformed_json() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, "not json").unwrap(); + let err = load_vars_file_reporting_duplicates(&path) + .expect_err("expected error for malformed JSON"); + let msg = err.to_string(); + assert!(msg.contains("invalid vars file"), "got: {msg}"); + assert!( + msg.contains(&path.display().to_string()), + "error should name the vars file path, got: {msg}" + ); + } + + #[test] + fn load_vars_file_reporting_duplicates_omitted_counter_surfaces() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + let mut json = String::from("{"); + for i in 0..1_003usize { + if i > 0 { + json.push(','); + } + json.push_str(&format!(r#""k{i}":0,"k{i}":1"#)); + } + json.push('}'); + std::fs::write(&path, json).unwrap(); + let loaded = load_vars_file_reporting_duplicates(&path) + .expect("should load the 1003-duplicate fixture"); + assert_eq!(loaded.duplicate_keys.len(), 1_000); + assert_eq!(loaded.duplicate_keys_omitted, 3); + } + + /// Mirrors `security.rs:400-422` (mds-cli): the same symlink guard applies to + /// the reporting variant, not just the pre-existing `load_vars_file`. + #[test] + #[cfg(unix)] + fn load_vars_file_reporting_duplicates_rejects_symlinked_path() { + let dir = tempfile::tempdir().unwrap(); + let real_vars = dir.path().join("real_vars.json"); + std::fs::write(&real_vars, r#"{"name": "Alice"}"#).unwrap(); + let link_vars = dir.path().join("link_vars.json"); + std::os::unix::fs::symlink(&real_vars, &link_vars).unwrap(); + + let result = load_vars_file_reporting_duplicates(&link_vars); + assert!( + result.is_err(), + "load_vars_file_reporting_duplicates must reject a symlinked vars path" + ); + let err = format!("{}", result.unwrap_err()); + assert!( + err.contains("symlink") || err.contains("not allowed"), + "error must mention symlink restriction; got: {err}" + ); + } + + /// `load_vars_file` must stay a silent, no-frills wrapper: same map as the + /// reporting variant, and it exposes nothing else (no duplicate info). + #[test] + fn load_vars_file_is_a_silent_wrapper_over_the_reporting_variant() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("vars.json"); + std::fs::write(&path, r#"{"x": 1, "x": 2}"#).unwrap(); + + let plain = load_vars_file(&path).expect("load_vars_file should succeed"); + let loaded = + load_vars_file_reporting_duplicates(&path).expect("reporting variant should succeed"); + assert_eq!(plain, loaded.vars); + } + // ── scan_imports tests ──────────────────────────────────────────────────── #[test] diff --git a/crates/mds-core/src/lint/fix.rs b/crates/mds-core/src/lint/fix.rs index 87b35528..371b4333 100644 --- a/crates/mds-core/src/lint/fix.rs +++ b/crates/mds-core/src/lint/fix.rs @@ -2,12 +2,12 @@ //! //! ## Tier contract (T5 / AC-F-18) //! -//! | Tier | Rules | Semantics | -//! |------|---------------------------------------------------|-----------| -//! | A | duplicate-import, duplicate-export, | Auto-fixable (span-removal); gated by reverify | -//! | | unreachable-branch, empty-block | | -//! | B | unused-import, unused-function | Fixable only when structural-standalone (no imports/extends/partial; reverify applies) | -//! | C | unused-variable, redundant-else, shadow-variable | Report-only; never fixed | +//! | Tier | Rules | Semantics | +//! |------|------------------------------------------------------------|-----------| +//! | A | duplicate-import, duplicate-export, unreachable-branch, | Auto-fixable (span-removal); gated by reverify | +//! | | empty-block, legacy-interpolation | | +//! | B | unused-import, unused-function | Fixable only when structural-standalone (no imports/extends/partial; reverify applies) | +//! | C | unused-variable, redundant-else, shadow-variable | Report-only; never fixed | //! //! **Tier B nuance:** //! - `unused-function` sets `fix_removals` to a whole-block [`FixLineSpan`] and is diff --git a/crates/mds-core/src/lint/tier.rs b/crates/mds-core/src/lint/tier.rs index afb792ab..aba1b00b 100644 --- a/crates/mds-core/src/lint/tier.rs +++ b/crates/mds-core/src/lint/tier.rs @@ -5,12 +5,15 @@ //! be a circular dependency: `fix.rs` imports `LintResult` from `diagnostic.rs`, //! so `diagnostic.rs` cannot import from `fix.rs`. //! -//! | Tier | Rules | Semantics | -//! |------|---------------------------------------------------|-----------| -//! | A | duplicate-import, duplicate-export, | Auto-fixable; gated by reverify | -//! | | unreachable-branch, empty-block | | -//! | B | unused-import, unused-function | Fixable only when structural-standalone | -//! | C | unused-variable, redundant-else, shadow-variable | Never fixed | +//! | Tier | Rules | Semantics | +//! |------|--------------------------------------------------------------|-----------| +//! | A | duplicate-import, duplicate-export, unreachable-branch, | Auto-fixable; gated by reverify | +//! | | empty-block, legacy-interpolation | | +//! | B | unused-import, unused-function | Fixable only when structural-standalone | +//! | C | unused-variable, redundant-else, shadow-variable | Never fixed | +//! +//! This table restates `rule_tier`, which is the authoritative source; `legacy-interpolation` +//! is the one Tier A rule whose fix is not output-neutral (see `is_output_neutral`). //! //! ## Terminology (spec §7.5) //! @@ -224,4 +227,89 @@ mod tests { // Original offset is preserved. assert_eq!(map.get("a"), Some(&0)); } + + /// #329: the module-doc Tier table in both `tier.rs` and `fix.rs` must stay in + /// sync with `rule_tier`, the authoritative source. Parses the `//! |` table + /// rows out of each file's own source (via `include_str!`), extracts every + /// token that names a registered rule together with the tier of its row + /// (a continuation row with a blank tier column inherits the tier of the row + /// above), and asserts the extracted map agrees with `rule_tier` in both + /// directions for all 10 rules. + #[test] + fn module_doc_tier_table_matches_rule_tier() { + use super::super::rules::ALL_RULE_NAMES; + + fn extract_tier_table(source: &str) -> std::collections::HashMap { + let mut map = std::collections::HashMap::new(); + let mut current_tier: Option = None; + for line in source.lines() { + let trimmed = line.trim_start(); + let Some(rest) = trimmed.strip_prefix("//! |") else { + continue; + }; + let cells: Vec<&str> = rest.split('|').collect(); + if cells.len() < 2 { + continue; + } + let tier_cell = cells[0].trim(); + if tier_cell.len() == 1 && tier_cell.chars().all(|c| c.is_ascii_uppercase()) { + current_tier = tier_cell.chars().next(); + } + let Some(tier) = current_tier else { continue }; + for token in cells[1].split(',') { + let name = token.trim(); + if ALL_RULE_NAMES.contains(&name) { + map.insert(name.to_string(), tier); + } + } + } + map + } + + fn tier_char(tier: FixTier) -> char { + match tier { + FixTier::A => 'A', + FixTier::B => 'B', + FixTier::C => 'C', + } + } + + for (label, source) in [ + ("tier.rs", include_str!("tier.rs")), + ("fix.rs", include_str!("fix.rs")), + ] { + let extracted = extract_tier_table(source); + // Non-vacuity: the parser must actually find all 10 rule names, or the + // bidirectional checks below would pass on an empty/partial extraction. + assert_eq!( + extracted.len(), + 10, + "{label}: expected all 10 rule names to be extracted from the module-doc \ + Tier table, got {extracted:?}" + ); + // Direction 1: table → rule_tier. + for &name in ALL_RULE_NAMES { + let got = *extracted.get(name).unwrap_or_else(|| { + panic!( + "{label}: rule {name:?} is registered but missing from the \ + module-doc Tier table" + ) + }); + let expected = tier_char(rule_tier(name)); + assert_eq!( + got, expected, + "{label}: module-doc Tier table says {name:?} is Tier {got}, but \ + rule_tier says Tier {expected}" + ); + } + // Direction 2: rule_tier → table (every extracted name is registered). + for name in extracted.keys() { + assert!( + ALL_RULE_NAMES.contains(&name.as_str()), + "{label}: module-doc Tier table names {name:?}, which is not a \ + registered rule" + ); + } + } + } } diff --git a/crates/mds-core/src/vars_json.rs b/crates/mds-core/src/vars_json.rs new file mode 100644 index 00000000..207a6afb --- /dev/null +++ b/crates/mds-core/src/vars_json.rs @@ -0,0 +1,207 @@ +//! Duplicate JSON object key detection for `--vars` files (#326). +//! +//! Implementation arrives in Phase 2 of the v0.4.3 action plan (step C1). This +//! module currently contains only its test specifications — the items the tests +//! reference (`duplicate_json_keys`, `DuplicateKeys`, `MAX_DUPLICATE_KEY_PATHS`) do +//! not exist yet, so the crate's test build is intentionally RED until Phase 2 +//! lands. See `.devflow/docs/handoff-v043-action-plan.md` step C1 for the design. + +#[cfg(test)] +mod tests { + use super::*; + + fn dup(json: &str) -> DuplicateKeys { + duplicate_json_keys(json).expect("expected the fixture to parse") + } + + /// Fixture shared by [`clean_document_reports_no_duplicates`] (T6) and + /// [`every_json_leaf_shape_is_accepted`] (T9): one instance of every JSON leaf + /// shape the visitor must handle, with no duplicates. The `\n` and `A` are + /// JSON string escapes present in the JSON text itself — ASCII backslash + /// sequences inside this Rust raw string, never live bytes (PF-018). + const EVERY_LEAF_SHAPE_FIXTURE: &str = r#"{"nul":null,"t":true,"f":false,"neg":-1,"big":18446744073709551615,"flt":1.5e300,"s":"a\nbA","arr":[],"obj":{}}"#; + + // T1 + #[test] + fn flat_duplicate_is_reported_once() { + let d = dup(r#"{"x":1,"x":2}"#); + assert_eq!(d.paths, vec!["x".to_string()]); + assert_eq!(d.omitted, 0); + } + + // T2 + #[test] + fn nested_duplicate_reports_dotted_path() { + let d = dup(r#"{"x":{"a":1,"a":2}}"#); + assert_eq!(d.paths, vec!["x.a".to_string()]); + } + + // T3 + #[test] + fn duplicate_inside_array_element_reports_bracket_index() { + let d = dup(r#"{"x":[{"k":0},{"k":0},{"a":1,"a":2}]}"#); + assert_eq!(d.paths, vec!["x[2].a".to_string()]); + } + + // T4 + #[test] + fn duplicate_two_levels_under_an_array_element() { + let d = dup(r#"{"x":[{"y":{"a":1,"a":2}}]}"#); + assert_eq!(d.paths, vec!["x[0].y.a".to_string()]); + } + + // T5 + #[test] + fn triple_repeat_reports_one_entry() { + let d = dup(r#"{"x":1,"x":2,"x":3}"#); + assert_eq!(d.paths, vec!["x".to_string()]); + } + + // T6 — positive control (PF-013) for every duplicate-detecting test in this + // module: a document with no duplicates at all must report none. + #[test] + fn clean_document_reports_no_duplicates() { + let d = dup(EVERY_LEAF_SHAPE_FIXTURE); + assert!( + d.paths.is_empty(), + "expected no duplicates, got {:?}", + d.paths + ); + assert_eq!(d.omitted, 0); + } + + // T7 + #[test] + fn duplicates_are_reported_in_encounter_order() { + let d = dup(r#"{"a":1,"b":{"c":1,"c":2},"a":3}"#); + assert_eq!(d.paths, vec!["b.c".to_string(), "a".to_string()]); + } + + // T8 + #[test] + fn same_key_in_two_objects_is_two_paths() { + let d = dup(r#"{"o":{"a":1,"a":2},"p":{"a":1,"a":2}}"#); + assert_eq!(d.paths, vec!["o.a".to_string(), "p.a".to_string()]); + } + + // T9 + #[test] + fn every_json_leaf_shape_is_accepted() { + let d = dup(EVERY_LEAF_SHAPE_FIXTURE); + assert!( + d.paths.is_empty(), + "expected no duplicates, got {:?}", + d.paths + ); + // Non-vacuity: the same text really does have 9 distinct top-level keys — + // otherwise an empty/trivial fixture would trivially pass T6/T9 both. + let value: serde_json::Value = + serde_json::from_str(EVERY_LEAF_SHAPE_FIXTURE).expect("fixture must be valid JSON"); + let serde_json::Value::Object(map) = value else { + panic!("fixture must be a JSON object"); + }; + assert_eq!(map.len(), 9, "fixture must have exactly 9 top-level keys"); + } + + // T10 — pins the documented limitation: a key containing '.' cannot be + // distinguished from a nesting separator in the rendered path. + #[test] + fn a_key_containing_a_dot_renders_ambiguously() { + let d = dup(r#"{"a.b":1,"a.b":2}"#); + assert_eq!(d.paths, vec!["a.b".to_string()]); + } + + /// Generate a flat JSON object with `n` distinct keys (`k0`..`k{n-1}`), each + /// key written twice (duplicated), in a single top-level object. + fn generate_duplicated_keys(n: usize) -> String { + let mut s = String::from("{"); + for i in 0..n { + if i > 0 { + s.push(','); + } + s.push_str(&format!(r#""k{i}":0,"k{i}":1"#)); + } + s.push('}'); + s + } + + // T11a + #[test] + fn recorded_paths_are_capped_and_the_rest_counted() { + let json = generate_duplicated_keys(1_003); + let d = dup(&json); + assert_eq!(d.paths.len(), MAX_DUPLICATE_KEY_PATHS); + assert_eq!(d.omitted, 3); + } + + // T11b — non-vacuity for T11a: below the cap, nothing is omitted and every + // path is kept. + #[test] + fn paths_below_the_cap_are_all_kept() { + let json = generate_duplicated_keys(999); + let d = dup(&json); + assert_eq!(d.paths.len(), 999); + assert_eq!(d.omitted, 0); + } + + /// Build `depth` nested single-key objects (`n0`..`n{depth-1}`) wrapping a + /// duplicated `dup` key at the deepest level. + fn nested_objects_with_duplicate_at_depth(depth: usize) -> String { + let mut open = String::new(); + let mut close = String::new(); + for i in 0..depth { + open.push_str(&format!(r#"{{"n{i}":"#)); + close.push('}'); + } + format!(r#"{open}{{"dup":1,"dup":2}}{close}"#) + } + + // T12 + #[test] + fn nesting_at_the_serde_json_limit_is_scanned() { + // 120 nested objects — below serde_json's 128-level recursion limit — with + // a duplicate at the deepest level. + let json = nested_objects_with_duplicate_at_depth(120); + let d = dup(&json); + assert_eq!(d.paths.len(), 1); + let expected_path = (0..120) + .map(|i| format!("n{i}")) + .collect::>() + .join(".") + + ".dup"; + assert_eq!(d.paths[0], expected_path); + } + + // T13 — proves D7's recursion bound is real: an error, never a panic. + #[test] + fn nesting_beyond_the_serde_json_limit_is_an_error_not_a_panic() { + // 129 nested arrays exceeds serde_json's 128-level recursion bound. + let mut json = "[".repeat(129); + json.push_str(&"]".repeat(129)); + let err = duplicate_json_keys(&json).expect_err("expected a recursion-limit error"); + assert!( + err.to_string().contains("recursion limit exceeded"), + "expected a recursion-limit message, got: {err}" + ); + } + + // T14a + #[test] + fn malformed_json_is_an_error() { + assert!(duplicate_json_keys("{").is_err()); + } + + // T14b — proves `de.end()` is called: trailing garbage after a complete value + // must be rejected, not silently ignored. + #[test] + fn trailing_data_is_an_error() { + assert!(duplicate_json_keys("{} {}").is_err()); + } + + // T15 + #[test] + fn an_array_root_reports_index_prefixed_paths() { + let d = dup(r#"[{"a":1,"a":2}]"#); + assert_eq!(d.paths, vec!["[0].a".to_string()]); + } +} diff --git a/crates/mds-core/tests/api_surface.rs b/crates/mds-core/tests/api_surface.rs index b7f7cd5f..0f4a1ee0 100644 --- a/crates/mds-core/tests/api_surface.rs +++ b/crates/mds-core/tests/api_surface.rs @@ -32,6 +32,20 @@ fn public_functions_exist() { let _ = mds::check_virtual_collecting_warnings(HashMap::new(), "main.mds", None); let _ = mds::load_vars_file(Path::new("nonexistent.json")); let _ = mds::load_vars_str("{}"); + let _ = mds::load_vars_file_reporting_duplicates(Path::new("nonexistent.json")); + let _ = mds::load_vars_str_reporting_duplicates("{}"); +} + +/// #326: `VarsLoad` fields are readable from an external crate. `#[non_exhaustive]` +/// forbids a struct literal, so the type is only obtainable through the load API. +#[test] +fn vars_load_fields_are_readable() { + let loaded = mds::load_vars_str_reporting_duplicates(r#"{"x": 1, "x": 2}"#) + .expect("should load duplicate-key vars"); + let _: &HashMap = &loaded.vars; + let _: &Vec = &loaded.duplicate_keys; + let _: usize = loaded.duplicate_keys_omitted; + assert_eq!(loaded.duplicate_keys, vec!["x".to_string()]); } #[test] From ec4af5535fad7efe640eb03b41d094a47a6cf8cb Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 9 Sep 2026 16:16:51 +0300 Subject: [PATCH 2/6] fix(core): report duplicate keys in JSON vars via a value-free second pass (#326) --- crates/mds-core/src/lib.rs | 139 +++++++++++++- crates/mds-core/src/vars_json.rs | 311 ++++++++++++++++++++++++++++++- 2 files changed, 439 insertions(+), 11 deletions(-) diff --git a/crates/mds-core/src/lib.rs b/crates/mds-core/src/lib.rs index f691575a..089fc6e5 100644 --- a/crates/mds-core/src/lib.rs +++ b/crates/mds-core/src/lib.rs @@ -1390,9 +1390,50 @@ pub fn scan_imports(source: &str) -> Result, MdsError> { Ok(paths.into_iter().collect()) } +/// The result of loading runtime variables from a JSON `--vars` source, along +/// with any duplicate object keys found in that source. +/// +/// Returned by [`load_vars_file_reporting_duplicates`] and +/// [`load_vars_str_reporting_duplicates`]. `vars` holds the fully-parsed +/// variables — JSON permits a repeated object key, so when one occurs, the +/// **last** value for that key wins (the same behavior `load_vars_file` / +/// `load_vars_str` have always had; this type only adds visibility into it). +/// +/// `duplicate_keys` lists the path of each key that repeated within its +/// enclosing object, at any nesting depth, in encounter order: dotted for +/// object nesting (`x.a`), 0-based bracketed for array-element nesting +/// (`x[2].a`, or `[0].a` for an array at the document root). A literal key +/// containing `.`, `[`, or `]` renders ambiguously with a nesting separator — +/// a documented, accepted limitation. **These paths are structured, untrusted +/// text** taken directly from the input JSON: a caller that displays one must +/// escape it first (e.g. with [`sanitize_control_chars_wire`]), exactly as for +/// any other untrusted identifier. +/// +/// At most 1,000 duplicate-key paths are recorded; `duplicate_keys_omitted` +/// counts any further distinct duplicate paths beyond that cap. +/// +/// This type is `#[non_exhaustive]`: new fields may be added in minor releases. +/// Obtain values from the load API above; do not construct via struct literal +/// in external crates. +#[non_exhaustive] +#[derive(Debug, Clone, PartialEq)] +pub struct VarsLoad { + /// The loaded variables. When a key repeated in the source, the last value + /// for that key wins. + pub vars: HashMap, + /// Paths of keys that repeated in the source, in encounter order, one entry + /// per path (however many times the key repeats), capped at 1,000. See the + /// type-level doc for the path grammar and the untrusted-text caveat. + pub duplicate_keys: Vec, + /// Count of distinct duplicate-key paths beyond the 1,000-path cap. + pub duplicate_keys_omitted: usize, +} + /// Load runtime variables from a JSON file. /// -/// The file must contain a JSON object; each key becomes a variable name. +/// The file must contain a JSON object; each key becomes a variable name. A +/// repeated object key is accepted silently (the last value wins) — use +/// [`load_vars_file_reporting_duplicates`] to also learn which keys repeated. /// /// # Examples /// @@ -1406,6 +1447,38 @@ pub fn scan_imports(source: &str) -> Result, MdsError> { /// ``` #[must_use = "the loaded variables should be used"] pub fn load_vars_file(path: &Path) -> Result, MdsError> { + load_vars_file_reporting_duplicates(path).map(|loaded| loaded.vars) +} + +/// Load runtime variables from a JSON file, additionally reporting any +/// duplicate object keys found in it. +/// +/// The file must contain a JSON object; each key becomes a variable name. When +/// a key repeats — at any nesting depth — the last value wins and every +/// repeated key's path is reported in [`VarsLoad::duplicate_keys`]; see that +/// type's doc for the path grammar and cap. +/// +/// # Errors +/// +/// Returns `Err(MdsError)` when: the path contains a symlink; the file cannot +/// be read; the file exceeds the maximum size; the file is not valid UTF-8; the +/// content is not valid JSON; or the top-level JSON value is not an object. +/// +/// # Examples +/// +/// ```rust,no_run +/// use std::path::Path; +/// +/// let loaded = mds::load_vars_file_reporting_duplicates(Path::new("vars.json"))?; +/// for key in &loaded.duplicate_keys { +/// eprintln!("warning: key '{key}' is set more than once; the last value wins"); +/// } +/// let result = mds::compile(Path::new("template.mds"), Some(loaded.vars))?; +/// let md = result.into_markdown()?; +/// # Ok::<(), Box>(()) +/// ``` +#[must_use = "the loaded variables and duplicate keys should be used"] +pub fn load_vars_file_reporting_duplicates(path: &Path) -> Result { let path_str = path_to_str(path)?; // PF-004: guard the vars-file path through the same symlink check that the // resolver applies to every imported file — avoids a raw read that bypasses @@ -1437,14 +1510,29 @@ pub fn load_vars_file(path: &Path) -> Result, MdsError> { ))); }; - map.into_iter() + let vars: HashMap = map + .into_iter() .map(|(key, val)| Value::from_json(val).map(|v| (key, v))) - .collect() + .collect::>()?; + + // D1: the duplicate scan is a second, value-free pass over the same text, + // run LAST — after every existing guard and error above has already had its + // chance to fire, in its existing order. + let dup = vars_json::duplicate_json_keys(&content) + .map_err(|e| MdsError::invalid_vars(format!("{path_str}: {e}")))?; + + Ok(VarsLoad { + vars, + duplicate_keys: dup.paths, + duplicate_keys_omitted: dup.omitted, + }) } /// Load runtime variables from a JSON string. /// -/// The string must contain a JSON object; each key becomes a variable name. +/// The string must contain a JSON object; each key becomes a variable name. A +/// repeated object key is accepted silently (the last value wins) — use +/// [`load_vars_str_reporting_duplicates`] to also learn which keys repeated. /// /// # Examples /// @@ -1462,6 +1550,32 @@ pub fn load_vars_file(path: &Path) -> Result, MdsError> { /// ``` #[must_use = "the loaded variables should be used"] pub fn load_vars_str(json: &str) -> Result, MdsError> { + load_vars_str_reporting_duplicates(json).map(|loaded| loaded.vars) +} + +/// Load runtime variables from a JSON string, additionally reporting any +/// duplicate object keys found in it. +/// +/// The string must contain a JSON object; each key becomes a variable name. +/// When a key repeats — at any nesting depth — the last value wins and every +/// repeated key's path is reported in [`VarsLoad::duplicate_keys`]; see that +/// type's doc for the path grammar and cap. +/// +/// # Errors +/// +/// Returns `Err(MdsError)` when `json` exceeds the maximum size, is not valid +/// JSON, or its top-level value is not an object. +/// +/// # Examples +/// +/// ```rust +/// let loaded = mds::load_vars_str_reporting_duplicates(r#"{"x": 1, "x": 2}"#)?; +/// assert_eq!(loaded.duplicate_keys, vec!["x".to_string()]); +/// assert_eq!(loaded.vars.get("x"), Some(&mds::Value::Number(2.0))); +/// # Ok::<(), Box>(()) +/// ``` +#[must_use = "the loaded variables and duplicate keys should be used"] +pub fn load_vars_str_reporting_duplicates(json: &str) -> Result { if json.len() as u64 > MAX_FILE_SIZE { return Err(MdsError::resource_limit(format!( "vars string exceeds maximum size of {} bytes", @@ -1473,9 +1587,22 @@ pub fn load_vars_str(json: &str) -> Result, MdsError> { let serde_json::Value::Object(map) = parsed else { return Err(MdsError::json_error("vars must be a JSON object")); }; - map.into_iter() + let vars: HashMap = map + .into_iter() .map(|(key, val)| Value::from_json(val).map(|v| (key, v))) - .collect() + .collect::>()?; + + // D1: the duplicate scan is a second, value-free pass over the same text, + // run LAST — after every existing guard and error above has already had its + // chance to fire, in its existing order. + let dup = + vars_json::duplicate_json_keys(json).map_err(|e| MdsError::json_error(e.to_string()))?; + + Ok(VarsLoad { + vars, + duplicate_keys: dup.paths, + duplicate_keys_omitted: dup.omitted, + }) } #[cfg(test)] diff --git a/crates/mds-core/src/vars_json.rs b/crates/mds-core/src/vars_json.rs index 207a6afb..fc0f4254 100644 --- a/crates/mds-core/src/vars_json.rs +++ b/crates/mds-core/src/vars_json.rs @@ -1,10 +1,311 @@ //! Duplicate JSON object key detection for `--vars` files (#326). //! -//! Implementation arrives in Phase 2 of the v0.4.3 action plan (step C1). This -//! module currently contains only its test specifications — the items the tests -//! reference (`duplicate_json_keys`, `DuplicateKeys`, `MAX_DUPLICATE_KEY_PATHS`) do -//! not exist yet, so the crate's test build is intentionally RED until Phase 2 -//! lands. See `.devflow/docs/handoff-v043-action-plan.md` step C1 for the design. +//! # Why a second, value-free pass (D1) +//! +//! `serde_json`'s own `Value` deserializer (`value/de.rs`, `visit_map`) builds an +//! object by repeatedly calling `Map::insert` and discarding the previous value on +//! a repeated key — by the time `serde_json::from_str::` returns, every +//! duplicate has already vanished; there is nothing left in the parsed `Value` to +//! detect a duplicate from. Rather than replace that deserializer with one that +//! tracks duplicates while also building the value (and re-deriving its numeric +//! parsing, non-finite-float-to-`Null` handling, and borrowed-vs-owned string +//! rules along the way), this module runs a SECOND pass over the same JSON text +//! with a value-free (`Self::Value = ()`) visitor. The two passes are independent: +//! the first (unchanged, in `lib.rs`) produces the `Value`; this one only records +//! which key paths repeat. Fidelity of the parsed value is preserved by +//! construction — this module never constructs or approximates a `Value`. +//! +//! # Where the duplicate vanishes +//! +//! `serde_json::Value`'s `Deserialize` impl inserts each key into a `Map` via +//! `Map::insert`, which returns (and drops) the previous value for a repeated +//! key. `load_vars_file`/`load_vars_str` in `lib.rs` call `duplicate_json_keys` +//! (this module) as a second pass over the *same* text to recover exactly the +//! information that first pass already discarded. +//! +//! # Path grammar +//! +//! A reported path names a JSON key by walking from the document root: +//! - Object nesting is dotted: `x.a`. +//! - Array-element nesting uses a 0-based bracket index: `x[2].a`, and an array at +//! the document root renders as `[0].a`. +//! - A literal key containing `.`, `[`, or `]` is **not** escaped — it renders +//! ambiguously with an actual nesting separator. This is a documented, +//! accepted limitation (display-only; the underlying key is never altered). +//! +//! # Cap +//! +//! At most [`MAX_DUPLICATE_KEY_PATHS`] paths are recorded; any further distinct +//! duplicate path is counted in [`DuplicateKeys::omitted`] instead. One path is +//! recorded per key, however many times that key repeats within its enclosing +//! object (a key appearing 3 times still yields one path). +//! +//! # Recursion bound (D7) +//! +//! This module adds no depth cap of its own. `serde_json::Deserializer`'s own +//! recursion guard (`check_recursion!`, limit 128, not configurable in this +//! build) bounds the scan's recursion and returns an `Err`, never panics. +//! `Value::from_json`'s separate `MAX_VALUE_DEPTH = 64` has already rejected any +//! document deep enough to matter for the parsed `Value` before this scan ever +//! runs — this module's 128-level ceiling exists only so the scan itself cannot +//! overflow the stack on adversarial input, and a document between 64 and 128 +//! levels deep fails earlier in `Value::from_json` regardless. + +use std::collections::HashSet; +use std::fmt; +use std::fmt::Write as _; + +use serde::de::{self, DeserializeSeed, MapAccess, SeqAccess, Visitor}; + +/// Maximum number of distinct duplicate-key paths recorded by +/// [`duplicate_json_keys`]. Mirrors the precedent of `MAX_WARNINGS` +/// (`evaluator.rs`) and `MAX_DIAGNOSTICS` (`limits.rs`): a hostile document with +/// many thousands of duplicate keys must not produce an unbounded warning flood. +/// Paths beyond the cap are counted, not recorded — see [`DuplicateKeys::omitted`]. +pub(crate) const MAX_DUPLICATE_KEY_PATHS: usize = 1_000; + +/// Result of scanning a JSON document's text for duplicate object keys. +/// +/// `paths` lists each duplicated key's rendered path (see the module doc's "Path +/// grammar" section), in encounter order, one entry per path regardless of how +/// many times the key repeats, capped at [`MAX_DUPLICATE_KEY_PATHS`]. `omitted` +/// counts any further distinct duplicate paths beyond the cap. +#[derive(Debug)] +pub(crate) struct DuplicateKeys { + pub(crate) paths: Vec, + pub(crate) omitted: usize, +} + +/// One segment of the path to the object currently being scanned: a named object +/// key, or a 0-based array index. +enum Seg { + Key(String), + Index(usize), +} + +/// Scan state threaded through the recursive visitor: the path to the object +/// currently being visited, the duplicate paths found so far, and the count of +/// duplicates omitted past the cap. +struct Scan { + path: Vec, + found: Vec, + omitted: usize, +} + +impl Scan { + /// Record one duplicate occurrence of `key` inside the object at the current + /// `path`. Renders the full path (container path + `key`) into a single + /// `String` via `write!`/`push_str` — no per-segment `format!` allocation + /// chain. Bounded: once [`MAX_DUPLICATE_KEY_PATHS`] paths have been recorded, + /// every further call only increments `omitted`. + fn record(&mut self, key: &str) { + if self.found.len() >= MAX_DUPLICATE_KEY_PATHS { + self.omitted += 1; + return; + } + let mut rendered = String::new(); + for seg in &self.path { + match seg { + Seg::Key(k) => { + if !rendered.is_empty() { + rendered.push('.'); + } + rendered.push_str(k); + } + Seg::Index(i) => { + // write! into an existing String never allocates a throwaway + // intermediate — the digits are appended in place. + let _ = write!(rendered, "[{i}]"); + } + } + } + if !rendered.is_empty() { + rendered.push('.'); + } + rendered.push_str(key); + self.found.push(rendered); + } +} + +/// Value-free visitor/seed pair: recurses through a JSON document recording +/// duplicate object keys, without building a `Value`. Holds a reborrowed `&mut +/// Scan` so the same scan state threads through every recursive call. +struct DupScan<'a> { + scan: &'a mut Scan, +} + +impl<'de> DeserializeSeed<'de> for DupScan<'_> { + type Value = (); + + fn deserialize(self, deserializer: D) -> Result<(), D::Error> + where + D: de::Deserializer<'de>, + { + deserializer.deserialize_any(self) + } +} + +impl<'de> Visitor<'de> for DupScan<'_> { + type Value = (); + + fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("any valid JSON value") + } + + // Every leaf shape serde_json's `deserialize_any` can call for a JSON leaf + // (de.rs): null, bool, signed/unsigned integer, float, string. None of these + // carry nested structure, so each is simply accepted. + fn visit_unit(self) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_bool(self, _v: bool) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_i64(self, _v: i64) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_u64(self, _v: u64) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_f64(self, _v: f64) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_str(self, _v: &str) -> Result + where + E: de::Error, + { + Ok(()) + } + + // Defence only: not reachable via plain serde_json::Deserializer (no + // arbitrary-precision integers, no explicit Option variant in JSON — `null` + // already routes to `visit_unit`), but spelled out so a future serde_json + // configuration change fails loudly via T9 (`invalid_type`) rather than + // silently mis-scanning. + fn visit_i128(self, _v: i128) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_u128(self, _v: u128) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_none(self) -> Result + where + E: de::Error, + { + Ok(()) + } + + fn visit_some(self, deserializer: D) -> Result + where + D: de::Deserializer<'de>, + { + deserializer.deserialize_any(self) + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: SeqAccess<'de>, + { + let Self { scan } = self; + let mut i = 0usize; + loop { + scan.path.push(Seg::Index(i)); + // Reborrow: `&mut *scan` yields a fresh `&mut Scan` for this element + // without moving `scan` out of the outer closure, so the loop can + // keep using it on the next iteration. + let got = seq.next_element_seed(DupScan { scan: &mut *scan }); + scan.path.pop(); + if got?.is_none() { + return Ok(()); + } + // Bounded by the document itself (ultimately by MAX_FILE_SIZE on the + // caller side): a JSON array literal cannot have more elements than + // there are bytes to spell them. + i += 1; + } + } + + fn visit_map(self, mut map: A) -> Result + where + A: MapAccess<'de>, + { + let Self { scan } = self; + let mut seen = HashSet::new(); + let mut reported = HashSet::new(); + while let Some(key) = map.next_key::()? { + // Record once per key per enclosing object: the second occurrence + // trips `reported.insert`, later repeats of the same key find + // `reported.insert` already false and are skipped. + if !seen.insert(key.clone()) && reported.insert(key.clone()) { + scan.record(&key); + } + scan.path.push(Seg::Key(key)); + let v = map.next_value_seed(DupScan { scan: &mut *scan }); + scan.path.pop(); + v?; + } + Ok(()) + } +} + +/// Scan `json` for JSON object keys that repeat within their enclosing object, at +/// any depth, without building a `serde_json::Value`. +/// +/// Returns the rendered path of each duplicated key (see the module doc's "Path +/// grammar" section), in encounter order, capped at [`MAX_DUPLICATE_KEY_PATHS`] +/// with any excess counted in [`DuplicateKeys::omitted`]. +/// +/// # Errors +/// +/// Returns `Err` when `json` is not valid JSON, or when nesting exceeds +/// `serde_json`'s built-in recursion limit (128 levels) — never panics. Callers +/// in this crate pass text a `serde_json::from_str::` call has already +/// accepted, so an error here is a divergence between the two passes and is +/// propagated rather than swallowed. +pub(crate) fn duplicate_json_keys(json: &str) -> Result { + let mut scan = Scan { + path: Vec::new(), + found: Vec::new(), + omitted: 0, + }; + let mut de = serde_json::Deserializer::from_str(json); + DupScan { scan: &mut scan }.deserialize(&mut de)?; + // Reject trailing garbage after a complete value (e.g. "{} {}") — omitting + // this call would silently ignore anything after the first valid value. + de.end()?; + Ok(DuplicateKeys { + paths: scan.found, + omitted: scan.omitted, + }) +} #[cfg(test)] mod tests { From 1d932bf575e3450e4790c2c74f9f6e8002be07b9 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 9 Sep 2026 16:46:15 +0300 Subject: [PATCH 3/6] fix(cli): warn on duplicate --vars file keys at every depth and on every watch rebuild (#326) --- CHANGELOG.md | 40 +++++++++ README.md | 1 + crates/mds-cli/src/build.rs | 71 ++++++++++++++-- crates/mds-cli/src/main.rs | 8 +- crates/mds-cli/src/watch.rs | 160 ++++++++++++++++++++++++++++-------- spec.md | 4 +- 6 files changed, 240 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59417c11..cce0d8ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Warn on duplicate keys in `--vars` JSON files, at every depth, on every `mds watch` rebuild (#326).** + `mds build|check|lint|watch --vars f.json` with a repeated JSON object key (e.g. + `{"x": 1, "x": 2}`) previously compiled silently with the last value winning — + `serde_json`'s map deserializer discards the earlier value with no signal. Now every + subcommand prints `warning: key '' is set more than once in vars file ; + the last value wins` for each duplicate, exit 0, suppressed by `--quiet` (parity with + the existing `--set`/`--set-string` duplicate-key warning, #200). Duplicates are + detected at **every depth** and reported with a dotted/bracketed key path mirroring + `{{a.b}}` interpolation syntax — `x`, `x.a`, `x[2].a`, `[0].a` for an array root + (0-based, display-only; a key that itself contains `.`/`[`/`]` renders ambiguously, + a known and documented limitation). At most 1 000 distinct duplicate key paths are + listed; beyond that a single tail line reports how many more were omitted: + `warning: {n} more duplicate keys in vars file are not listed`. `mds watch` + reloads the vars file from disk on every rebuild (ADR-016), so its duplicate keys are + re-reported on every rebuild too — including a duplicate introduced mid-session by + editing the vars file — while `--set`/`--set-string` duplicate warnings keep their + existing once-at-startup behaviour. New public `mds-core` API: + `mds::VarsLoad { vars, duplicate_keys, duplicate_keys_omitted }` (`#[non_exhaustive]`) + and `mds::load_vars_file_reporting_duplicates` / + `mds::load_vars_str_reporting_duplicates`, implemented as a second, value-free parse + pass over the same JSON text so the already-parsed `vars` map is never re-derived and + stays byte-for-byte what the existing parser produced. `load_vars_file`/`load_vars_str` + are unchanged in every observable way (same signature, same return type, same errors) + and now delegate to the reporting variants. Known limitations: a key containing a + literal `.`, `[`, or `]` renders ambiguously in its reported path; the file-load and + string-load error codes (`mds::invalid_vars` vs `mds::json`) remain deliberately + un-unified (pre-existing split, unchanged). +- **Fix stale `lint_str` rustdoc and lint-rule Tier tables (#329).** `mds-core`'s + `lint_str` rustdoc said "applies the 9 lint rules" after a 10th rule + (`legacy-interpolation`) had shipped; the Tier tables in `lint/tier.rs` and + `lint/fix.rs` both omitted `legacy-interpolation` from Tier A. Fixed all three, and + added a mechanised test (`module_doc_tier_table_matches_rule_tier`) that extracts + every rule name and tier from both module-doc tables and asserts they match + `rule_tier` for all 10 known rules, so the tables can't drift again silently. + `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. + ### Internal - Cargo dependency sweep: napi 3.9.0 → 3.12.2, napi-derive 3.5.6 → 3.6.3, napi-build 2.3.2 → 2.4.1 (napi-sys 3.3.0, napi-derive-backend 6.1.2), pyo3 0.29.0 → 0.29.2, clap 4.6.1 → 4.6.6, similar 3.1.1 → 3.2.0, wasm-bindgen 0.2.121 → 0.2.126 (js-sys 0.3.103, wasm-bindgen-futures 0.4.76, wasm-bindgen-test 0.3.76), serde 1.0.228 → 1.0.229, serde_json 1.0.150 → 1.0.151, thiserror 2.0.18 → 2.0.20, libc 0.2.186 → 0.2.189. Supersedes Dependabot #354 #360 #359 #358 #280 #251 #249 #246 #243. diff --git a/README.md b/README.md index daf2eea9..5b594863 100644 --- a/README.md +++ b/README.md @@ -94,6 +94,7 @@ Build/Watch options: --out-dir Output directory (build/single-file watch: .md or .json; dir-mode watch: mirrors source subtree) --vars JSON file with variable overrides (reloaded each rebuild) + A key repeated at any depth warns with its path; the last value wins. --set KEY=VALUE Set a single variable (repeatable); value coerced to number/bool/null/array when possible Repeating a key warns; the last value wins. --set-string KEY=VALUE Set a single variable as a string, bypassing type coercion (repeatable) diff --git a/crates/mds-cli/src/build.rs b/crates/mds-cli/src/build.rs index 3d6b9fc5..349966fe 100644 --- a/crates/mds-cli/src/build.rs +++ b/crates/mds-cli/src/build.rs @@ -517,10 +517,8 @@ pub(crate) struct RuntimeVarArgs { } /// Load vars from an optional file path, returning None if no file was given. -pub(crate) fn load_optional_vars_file( - path: Option, -) -> Result>> { - path.map(|p| mds::load_vars_file(&p).map_err(miette::Error::from)) +pub(crate) fn load_optional_vars_file(path: Option) -> Result> { + path.map(|p| mds::load_vars_file_reporting_duplicates(&p).map_err(miette::Error::from)) .transpose() } @@ -535,6 +533,16 @@ pub(crate) struct RuntimeVars { pub(crate) duplicate_set_keys: Vec, /// Keys that appeared more than once inside `--set-string` (same contract). pub(crate) duplicate_set_string_keys: Vec, + /// Key paths (dotted/bracketed, e.g. `x.a`, `x[2].a`) that appeared more than + /// once in the `--vars` JSON file, at any depth (#326). Empty when no `--vars` + /// file was given, or when the file had no duplicates. + pub(crate) duplicate_vars_file_keys: Vec, + /// Count of distinct duplicate key paths beyond `mds::VarsLoad`'s cap that were + /// not individually recorded in `duplicate_vars_file_keys` (#326). + pub(crate) duplicate_vars_file_keys_omitted: usize, + /// The `--vars` file path as passed on the command line, for warning messages + /// (#326). `None` when no `--vars` file was given. + pub(crate) vars_file: Option, } /// Collect keys that appear more than once in `pairs`, in first-occurrence order, @@ -587,7 +595,19 @@ pub(crate) fn build_runtime_vars(args: RuntimeVarArgs) -> Result { let duplicate_set_keys = duplicate_keys(&set_vars); let duplicate_set_string_keys = duplicate_keys(&set_string_vars); - let mut runtime_vars = load_optional_vars_file(vars)?; + // Clone the path BEFORE load_optional_vars_file(vars) moves it (#326). + let vars_file = vars.clone(); + let loaded = load_optional_vars_file(vars)?; + let (mut runtime_vars, duplicate_vars_file_keys, duplicate_vars_file_keys_omitted) = + match loaded { + Some(mds::VarsLoad { + vars, + duplicate_keys, + duplicate_keys_omitted, + .. + }) => (Some(vars), duplicate_keys, duplicate_keys_omitted), + None => (None, Vec::new(), 0), + }; for (key, val) in set_vars { runtime_vars .get_or_insert_with(HashMap::new) @@ -602,11 +622,49 @@ pub(crate) fn build_runtime_vars(args: RuntimeVarArgs) -> Result { vars: runtime_vars, duplicate_set_keys, duplicate_set_string_keys, + duplicate_vars_file_keys, + duplicate_vars_file_keys_omitted, + vars_file, }) } +/// Emit `warning: key '…' is set more than once in vars file …; the last value wins` +/// lines for every duplicate key path found in the `--vars` JSON file (#326), at +/// every depth, plus one tail line when the duplicate count exceeds +/// [`mds::VarsLoad`]'s cap. +/// +/// AD-224-3: every untrusted value interpolated into `eprint_warning` must be wrapped +/// in `safe_inline(…)` / `safe_path(…)` **at the interpolation site** — not hoisted +/// into a `let` binding first. `key` is raw, untrusted text straight from the JSON +/// (D4); `Path` is not `Display`, so it goes through `safe_path`, not `safe_inline`. +/// +/// AD-224-5: no-op when `quiet` is true. +pub(crate) fn emit_duplicate_vars_file_warnings(resolved: &RuntimeVars, quiet: bool) { + if quiet { + return; + } + let Some(path) = resolved.vars_file.as_deref() else { + return; + }; + for key in &resolved.duplicate_vars_file_keys { + crate::output::eprint_warning(&format!( + "warning: key '{}' is set more than once in vars file {}; the last value wins", + crate::output::safe_inline(key), + crate::output::safe_path(path) + )); + } + if resolved.duplicate_vars_file_keys_omitted > 0 { + crate::output::eprint_warning(&format!( + "warning: {} more duplicate keys in vars file {} are not listed", + crate::output::safe_inline(resolved.duplicate_vars_file_keys_omitted), + crate::output::safe_path(path) + )); + } +} + /// Emit `warning: variable '…' is set more than once by --set/--set-string` lines -/// for any duplicate keys found by [`build_runtime_vars`]. +/// for any duplicate keys found by [`build_runtime_vars`], plus (D8 order: file → +/// `--set` → `--set-string`) the `--vars` file duplicate-key warnings (#326). /// /// AD-224-3: every untrusted value interpolated into `eprint_warning` must be wrapped /// in `safe_inline(…)` **at the interpolation site** — not hoisted into a `let` binding @@ -618,6 +676,7 @@ pub(crate) fn emit_duplicate_var_warnings(resolved: &RuntimeVars, quiet: bool) { if quiet { return; } + emit_duplicate_vars_file_warnings(resolved, quiet); for key in &resolved.duplicate_set_keys { crate::output::eprint_warning(&format!( "warning: variable '{}' is set more than once by --set; the last value wins", diff --git a/crates/mds-cli/src/main.rs b/crates/mds-cli/src/main.rs index 8ce98566..f3a107ee 100644 --- a/crates/mds-cli/src/main.rs +++ b/crates/mds-cli/src/main.rs @@ -56,7 +56,7 @@ enum Commands { /// Mutually exclusive with -o/--output. #[arg(long = "out-dir", conflicts_with = "output")] out_dir: Option, - /// JSON file with runtime variable overrides + /// JSON file with runtime variable overrides (a repeated key warns; the last value wins) #[arg(long)] vars: Option, /// Set a runtime variable (repeatable, e.g. --set name=Alice --set count=3; repeating a key warns, last value wins) @@ -89,7 +89,7 @@ enum Commands { Check { /// Input .mds file (use "-" for stdin; omit to auto-detect in current directory) input: Option, - /// JSON file with runtime variable overrides + /// JSON file with runtime variable overrides (a repeated key warns; the last value wins) #[arg(long)] vars: Option, /// Set a runtime variable (repeatable, e.g. --set name=Alice --set count=3; repeating a key warns, last value wins) @@ -160,7 +160,7 @@ enum Commands { /// Output format: `human` (default, stderr) or `json` (stdout) #[arg(long = "format", value_name = "FORMAT", default_value = "human")] format: String, - /// JSON file with runtime variable overrides + /// JSON file with runtime variable overrides (a repeated key warns; the last value wins) #[arg(long)] vars: Option, /// Set a runtime variable (repeatable, e.g. --set name=Alice; repeating a key warns, last value wins) @@ -208,7 +208,7 @@ enum Commands { /// Mutually exclusive with -o/--output. #[arg(long = "out-dir", conflicts_with = "output")] out_dir: Option, - /// JSON file with runtime variable overrides (reloaded on each rebuild) + /// JSON file with runtime variable overrides (reloaded on each rebuild; a repeated key warns on each rebuild, last value wins) #[arg(long)] vars: Option, /// Set a runtime variable (repeatable, e.g. --set name=Alice --set count=3; repeating a key warns, last value wins) diff --git a/crates/mds-cli/src/watch.rs b/crates/mds-cli/src/watch.rs index 894fb9ac..c50997ab 100644 --- a/crates/mds-cli/src/watch.rs +++ b/crates/mds-cli/src/watch.rs @@ -377,7 +377,8 @@ pub(crate) fn external_recovery_decision( /// /// Rejects a symlinked vars file at startup (build parity — PF-004). /// Falls back to the raw path when the file does not yet exist (the user may create -/// it later; the per-rebuild `load_vars_file` will catch it then). +/// it later; the vars file is reloaded on every rebuild — ADR-016 — so a duplicate +/// key introduced after startup is caught on the next rebuild, #326). pub(crate) fn canonicalize_vars_path(vars: Option) -> Result, MdsError> { match vars { Some(p) if p.exists() => { @@ -746,7 +747,13 @@ pub(crate) fn run_watch(args: WatchArgs) -> Result<()> { /// kind cannot change without the template itself changing, which triggers a rebuild). struct FileCompileCtx { entry: PathBuf, + /// Canonicalized `--vars` path — matches notify's canonicalized event paths; + /// used for `dirs_to_watch`/`files_of_interest` (never for display, #326). vars_path: Option, + /// The `--vars` path exactly as the user typed it, uncanonicalized (#326, D4). + /// Used for `RuntimeVarArgs.vars` so the vars-file duplicate-key warning displays + /// (and reads) the as-typed path rather than its canonical form. + vars_path_raw: Option, static_set_vars: Vec<(String, String)>, static_set_string_vars: Vec<(String, String)>, /// The `-o ` or `--out-dir` argument passed by the user, if any. @@ -933,19 +940,30 @@ fn rebuild_file( ) { // Soft-error: vars file may be temporarily absent (AC-W7 / AC-C5). // Print the error, settle mtime to avoid re-fire, and keep watching. - let runtime_vars = match build_runtime_vars(RuntimeVarArgs { - vars: ctx.vars_path.clone(), + // + // The vars-file duplicate-key warnings (#326) are NOT emitted here + // unconditionally: `rebuild_file` is called both from a genuine fs-event + // rebuild AND from the liveness probe's unconditional-on-first-tick + // self-heal recompile (a documented "worst case: one redundant compile" + // that normally dedups to no write, see the comment above this function). + // Emitting here would double-report the same duplicate once per session on + // every startup. Instead the resolved vars are held and the warning is + // emitted below, gated on `content_changed` — the same signal that gates + // the "Recompiled" line — so the vars-file duplicate is re-reported exactly + // once per OBSERVABLE rebuild (tests I9, I16, I18). + let resolved = match build_runtime_vars(RuntimeVarArgs { + vars: ctx.vars_path_raw.clone(), set_vars: ctx.static_set_vars.clone(), set_string_vars: ctx.static_set_string_vars.clone(), }) { - // Flags are fixed for the session; warned once at startup — discard here. - Ok(v) => v.vars, + Ok(v) => v, Err(e) => { eprint_error(e); state.last_mtimes = snapshot_state(&state.foi); return; } }; + let runtime_vars = resolved.vars.clone(); let t0 = Instant::now(); match compile_to_content( @@ -990,6 +1008,13 @@ fn rebuild_file( .get(&output_key) .is_none_or(|prev| *prev != compiled.content); + // #326: re-report the vars-file duplicate-key warnings exactly when an + // observable rebuild happens (same gate as the "Recompiled" line below), + // not on the liveness probe's redundant no-op recompile. + if content_changed { + crate::build::emit_duplicate_vars_file_warnings(&resolved, ctx.quiet); + } + // ADR-016: always recompute dep set from fresh output. let new_dirs = dirs_to_watch(&ctx.entry, &compiled.dependencies, ctx.vars_path.as_deref()); @@ -1051,6 +1076,14 @@ fn run_watch_file( quiet: bool, tick: Option, ) -> Result<()> { + // #326: keep the --vars argument as the user typed it, separately from the + // canonicalized form below. `vars_path` (canonical) is used for everything that + // must match notify's canonicalized event paths (dirs_to_watch, files_of_interest, + // event matching); `vars_path_raw` is used only for `RuntimeVarArgs.vars`, so the + // vars-file duplicate-key warning (D4: "{path} = the --vars arg as typed") displays + // and reads through the same path the user gave — reading a valid, possibly + // symlinked path is fine either way, only the DISPLAYED text differs. + let vars_path_raw = vars.clone(); // Canonicalize so path matches notify event paths (resolves /tmp → /private/tmp on macOS). // Also rejects a symlinked vars file at startup (build parity — PF-004). let vars_path = canonicalize_vars_path(vars).map_err(miette::Error::from)?; @@ -1146,7 +1179,7 @@ fn run_watch_file( // For the default case (no explicit flag), the path depends on the output kind, which // is only known after compilation — so we compile first, then derive. let resolved = build_runtime_vars(RuntimeVarArgs { - vars: vars_path.clone(), + vars: vars_path_raw.clone(), set_vars: static_set_vars.clone(), set_string_vars: static_set_string_vars.clone(), })?; @@ -1303,6 +1336,7 @@ fn run_watch_file( let ctx = FileCompileCtx { entry, vars_path, + vars_path_raw, static_set_vars, static_set_string_vars, output_arg: output, @@ -1513,7 +1547,13 @@ struct LivenessState { /// once per batch by `process_dir_batch`, over the whole tracked set (#321). /// /// Compile success/failure is already signalled via `state.errored`; the caller uses -/// that set rather than this function's return value, so the return type is `()`. +/// that set for error tracking. +/// +/// Returns `true` when this call produced an observable, content-changed rebuild +/// (a real write, not a partial/unchanged/errored compile) — used by +/// `process_dir_batch`'s callers to gate the `#326` vars-file duplicate-key +/// warning on an OBSERVABLE rebuild rather than every internal recompute (the +/// same content-based signal `rebuild_file` uses in single-file mode). fn compile_one_source( src: &Path, root: &Path, @@ -1521,7 +1561,7 @@ fn compile_one_source( runtime_vars: &Option>, quiet: bool, state: &mut DirWatchState, -) { +) -> bool { let t0 = Instant::now(); match compile_to_content( src, @@ -1535,7 +1575,7 @@ fn compile_one_source( // Partials (DD2): refresh graph edges but do NOT write output. if is_partial(src) { state.record_success(src, dep_paths, root, None, None); - return; + return false; } // Derive the output path from the compiled kind (intrinsic extension). @@ -1594,20 +1634,24 @@ fn compile_one_source( Some(&out), Some(compiled.content), ); + true } Err(e) => { eprint_error(e); state.record_error(src); + false } } } else { // Content unchanged — still refresh graph edges + known_files. state.record_success(src, dep_paths, root, None, None); + false } } Err(e) => { eprint_error(e); state.record_error(src); + false } } } @@ -1629,7 +1673,13 @@ struct DirStartup { /// from the extracted helper functions (issue #6 / zero-warnings policy). struct DirWatchCtx { root: PathBuf, + /// Canonicalized `--vars` path — matches notify's canonicalized event paths; + /// used for matching/watching (never for display, #326). vars_path: Option, + /// The `--vars` path exactly as the user typed it, uncanonicalized (#326, D4). + /// Used for `RuntimeVarArgs.vars` so the vars-file duplicate-key warning displays + /// (and reads) the as-typed path rather than its canonical form. + vars_path_raw: Option, static_set_vars: Vec<(String, String)>, static_set_string_vars: Vec<(String, String)>, output_base: OutputBase, @@ -1791,11 +1841,18 @@ fn liveness_probe_dir( if !batch.is_empty() { // Soft-error: vars file may be temporarily absent (AC-W7 / AC-C5). let runtime_vars = match build_runtime_vars(RuntimeVarArgs { - vars: ctx.vars_path.clone(), + vars: ctx.vars_path_raw.clone(), set_vars: ctx.static_set_vars.clone(), set_string_vars: ctx.static_set_string_vars.clone(), }) { // Flags are fixed for the session; warned once at startup — discard here. + // The vars-file duplicate-key warnings (#326) are emitted from + // `handle_fs_event_dir` instead, not here: this liveness-probe path is a + // defensive content-backstop/full-reconcile tick that can race the same + // edit's real fs-event delivery (both observing the same changed mtime), + // and emitting from both sites double-counts a single rebuild (test I17 + // guards this — see the module docs on `liveness_probe_dir` and + // `handle_fs_event_dir`). Ok(v) => v.vars, Err(e) => { eprint_error(e); @@ -1916,13 +1973,20 @@ fn handle_fs_event_dir( // ADR-016: reload vars from disk on every rebuild. // Soft-error: vars file may be temporarily absent (AC-W7 / AC-C5). - let runtime_vars = match build_runtime_vars(RuntimeVarArgs { - vars: ctx.vars_path.clone(), + let resolved = match build_runtime_vars(RuntimeVarArgs { + vars: ctx.vars_path_raw.clone(), set_vars: ctx.static_set_vars.clone(), set_string_vars: ctx.static_set_string_vars.clone(), }) { - // Flags are fixed for the session; warned once at startup — discard here. - Ok(v) => v.vars, + // --set/--set-string are fixed for the session and warned once at startup — + // discarded (via `resolved.vars` below). The vars file is reloaded on every + // rebuild (ADR-016), so its duplicate keys are re-reported too — but only when + // this batch produces an OBSERVABLE rebuild (#326, tests I16-I18): at + // `--debounce 0` a single edit can generate more than one raw FS event, each + // reaching this function separately, so the warning is emitted after + // `process_dir_batch` reports whether anything actually changed rather than + // unconditionally here. + Ok(v) => v, Err(e) => { eprint_error(e); // Re-baseline so the idle-tick content backstop does not report the same @@ -1931,8 +1995,9 @@ fn handle_fs_event_dir( return DirEventOutcome::Done; } }; + let runtime_vars = resolved.vars.clone(); - process_dir_batch( + let any_changed = process_dir_batch( &mds_changed, vars_changed, &ctx.root, @@ -1941,6 +2006,9 @@ fn handle_fs_event_dir( ctx.quiet, state, ); + if any_changed { + crate::build::emit_duplicate_vars_file_warnings(&resolved, ctx.quiet); + } DirEventOutcome::Done } @@ -1967,6 +2035,9 @@ fn dir_watch_startup( ) -> Result { // Load config once from the root directory. let config = load_config(&root)?; + // #326: keep the --vars argument as the user typed it (see FileCompileCtx's + // vars_path_raw doc for why) — `vars_path` below stays canonical for matching. + let vars_path_raw = vars.clone(); // Canonicalize so path matches notify event paths (resolves /tmp → /private/tmp on macOS). // Also rejects a symlinked vars file at startup (build parity — PF-004). let vars_path = canonicalize_vars_path(vars).map_err(miette::Error::from)?; @@ -2068,7 +2139,7 @@ fn dir_watch_startup( // Startup compile: compile all .mds files found under root. let all_files = collect_mds_files(&root, MAX_COLLECT_DEPTH, exclude_prefix.as_deref()); let resolved = build_runtime_vars(RuntimeVarArgs { - vars: vars_path.clone(), + vars: vars_path_raw.clone(), set_vars: static_set_vars.clone(), set_string_vars: static_set_string_vars.clone(), })?; @@ -2189,16 +2260,19 @@ fn dir_watch_startup( // Build the dedup baseline for any source whose startup compile did not record // one (partials are skipped above; a failed write leaves no entry). - // dir_watch_startup calls build_runtime_vars twice: once above (emit) and once - // here (discard) — emitting at both sites would double-print the warning on - // directory-watch startup. Test I9 is the sole mechanical guard on this. + // dir_watch_startup calls build_runtime_vars twice: once above (emit, including + // the #326 vars-file duplicate-key warnings) and once here (discard) — emitting + // at both sites would double-print every warning (both the --set/--set-string + // ones and the vars-file ones) on directory-watch startup. Tests I9 and I17 are + // the mechanical guards on this. { let baseline_resolved = build_runtime_vars(RuntimeVarArgs { - vars: vars_path.clone(), + vars: vars_path_raw.clone(), set_vars: static_set_vars.clone(), set_string_vars: static_set_string_vars.clone(), })?; - // Flags are fixed for the session; warned once above at startup — discard here. + // Flags and vars-file duplicates alike are already warned above at startup — + // discard here (this second read only rebuilds the dedup baseline). let baseline_vars = baseline_resolved.vars; for source in &all_files { let key = graph_key(source); @@ -2290,6 +2364,7 @@ fn dir_watch_startup( let ctx = DirWatchCtx { root, vars_path, + vars_path_raw, static_set_vars, static_set_string_vars, output_base, @@ -2388,6 +2463,13 @@ fn run_watch_dir( /// /// Called by both the event path and the reconcile path so the same state /// transitions apply uniformly. +/// +/// Returns `true` when the batch produced at least one observable, content-changed +/// rebuild (see `compile_one_source`) — callers use this to gate the `#326` +/// vars-file duplicate-key warning on an OBSERVABLE rebuild, since a single logical +/// edit can otherwise reach this function more than once (e.g. multiple raw FS +/// events for one write at `--debounce 0`, or a liveness-probe self-heal tick +/// racing a real FS event for the same change) and would otherwise double-warn. fn process_dir_batch( changed: &BTreeSet, vars_changed: bool, @@ -2396,12 +2478,12 @@ fn process_dir_batch( runtime_vars: &Option>, quiet: bool, state: &mut DirWatchState, -) { - if vars_changed { - process_dir_batch_vars_changed(root, output_base, runtime_vars, quiet, state); +) -> bool { + let any_changed = if vars_changed { + process_dir_batch_vars_changed(root, output_base, runtime_vars, quiet, state) } else { - process_dir_batch_incremental(changed, root, output_base, runtime_vars, quiet, state); - } + process_dir_batch_incremental(changed, root, output_base, runtime_vars, quiet, state) + }; // Re-baseline the content backstop over the post-batch tracked set (#321). // @@ -2412,6 +2494,7 @@ fn process_dir_batch( // unchanged broken file does not re-fire every tick) and drops keys for sources the // batch deleted, which `snapshot_state` achieves by replacing the map outright. state.last_mtimes = snapshot_state(&state.tracked_set()); + any_changed } /// Full recompile of all known files triggered by a vars-file change. @@ -2425,13 +2508,17 @@ fn process_dir_batch( /// (rust.md / reliability issue #3 fix). /// /// Uses `compile_one_source` for the shared compile→dedup→write sequence. +/// +/// Returns `true` when at least one source in the batch produced an observable, +/// content-changed rebuild (see `compile_one_source`). fn process_dir_batch_vars_changed( root: &Path, output_base: &OutputBase, runtime_vars: &Option>, quiet: bool, state: &mut DirWatchState, -) { +) -> bool { + let mut any_changed = false; let all_sources: Vec = state.known_files.iter().cloned().collect(); // Determine which known sources no longer exist — their output files must be @@ -2475,13 +2562,14 @@ fn process_dir_batch_vars_changed( state.external_dep_dirs.clear(); for src in &all_sources { - if src.exists() { - compile_one_source(src, root, output_base, runtime_vars, quiet, state); + if src.exists() && compile_one_source(src, root, output_base, runtime_vars, quiet, state) { + any_changed = true; } } // Prune known_files to currently-existing sources. state.known_files = all_sources.into_iter().filter(|p| p.exists()).collect(); + any_changed } /// Incremental recompile: compile only transitive importers of the changed seeds. @@ -2494,6 +2582,9 @@ fn process_dir_batch_vars_changed( /// 5. Delete outputs for removed sources. /// /// Uses `compile_one_source` for the shared compile→dedup→write sequence. +/// +/// Returns `true` when at least one affected source produced an observable, +/// content-changed rebuild (see `compile_one_source`). fn process_dir_batch_incremental( changed: &BTreeSet, root: &Path, @@ -2501,7 +2592,9 @@ fn process_dir_batch_incremental( runtime_vars: &Option>, quiet: bool, state: &mut DirWatchState, -) { +) -> bool { + let mut any_changed = false; + // 1. Partition. let (existing, deleted): (BTreeSet, BTreeSet) = changed.iter().cloned().partition(|p| p.exists()); @@ -2514,7 +2607,7 @@ fn process_dir_batch_incremental( } if seeds.is_empty() { - return; + return false; } // 3. Affected = seeds ∪ transitive importers (uses start-of-batch graph snapshot). @@ -2582,7 +2675,9 @@ fn process_dir_batch_incremental( } // In-root source: full compile→dedup→write via shared helper. - compile_one_source(src, root, output_base, runtime_vars, quiet, state); + if compile_one_source(src, root, output_base, runtime_vars, quiet, state) { + any_changed = true; + } } // 5. Deletions: after importers recompiled, clean up graph + outputs. @@ -2634,6 +2729,7 @@ fn process_dir_batch_incremental( // (watcher is not in scope here; callers call liveness_probe_dir which re-arms only // live dirs — stale dirs simply drop off the set and stop being visited each tick.) state.external_dep_dirs = live_ext_dirs; + any_changed } // ── Unit tests ──────────────────────────────────────────────────────────────── diff --git a/spec.md b/spec.md index 56ad75aa..6c409e85 100644 --- a/spec.md +++ b/spec.md @@ -890,7 +890,7 @@ mds build src/ --out-dir dist # Mirror subtree: src/a/b.mds → dis |--------|-------------| | `-o, --output ` | Output file path, or `-` for stdout. Mutually exclusive with `--out-dir`. Rejected for directory input. Warns if the extension contradicts the template kind. | | `--out-dir ` | Output directory. Mirrors subtree (dir mode) or writes `.` inside it (file mode). Created if absent. | -| `--vars ` | JSON file with runtime variable overrides. | +| `--vars ` | JSON file with runtime variable overrides. A key repeated at any depth warns with its dotted/bracketed path (e.g. `x.a`, `x[2].a`); the last value wins. | | `--set KEY=VALUE` | Set a single variable. Repeatable. Values are coerced to boolean, number, null, or array when possible. Repeating a key emits a warning; the last value wins. | | `--set-string KEY=VALUE` | Set a single variable as a **string**, bypassing type coercion. Repeatable. Use when the value must remain a string (e.g. a numeric-looking ID). Repeating a key emits a warning; the last value wins. | | `--source-map` | Generate a source-map sidecar (`.map`, e.g. `-o out.md` → `out.md.map`). Ignored for messages-mode templates (a warning is emitted and no source map is produced). Conflicts with `--no-source-map`. Also enabled globally via `build.source_map = true` in `mds.json`. | @@ -971,7 +971,7 @@ cat template.mds | mds lint --fix - # Fix from stdin, write fixed source t | `--check` | With `--fix`: never writes; exit is `max(1, residual severity)` — 1 when a fix is pending and the post-fix residual would be clean or warn-only, 2 when findings the fix cannot remove would remain at error severity. The emitted diagnostics stay pre-fix (the preview reports what is wrong now); only the exit code looks through to the residual. Useful for CI. | | `--diff` | With `--fix`: print unified diff of pending changes without writing. Exit follows the same `max(1, residual severity)` preview rule as `--check`. | | `--format ` | Output format: `human` (default, stderr) or `json` (stdout). | -| `--vars ` | JSON file with runtime variable overrides (forwarded to the check gate). | +| `--vars ` | JSON file with runtime variable overrides (forwarded to the check gate). A key repeated at any depth warns with its dotted/bracketed path; the last value wins. | | `--set KEY=VALUE` | Set a single variable. Repeatable. Type coercion applies. Repeating a key emits a warning; the last value wins. | | `--set-string KEY=VALUE` | Set a single variable as a string, bypassing type coercion. Repeatable. Repeating a key emits a warning; the last value wins. | | `-q, --quiet` | Suppress warning/info human diagnostics and the directory summary on clean/warn-only runs; errors still print and the summary still appears when error- or resource-limited files are present. (The directory-depth warning — fires on trees deeper than MAX_DEPTH=64 — is emitted regardless of `--quiet`.) | From 004e3ab28f60ab08602ed4aa29bb5ec1c47af34d Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 9 Sep 2026 17:05:05 +0300 Subject: [PATCH 4/6] fix(cli): warn on vars-file duplicates on the dir-watch self-heal rebuild too; pin --quiet and newline-key contracts (#326) --- crates/mds-cli/src/watch.rs | 35 ++++--- crates/mds-cli/tests/cli_watch.rs | 159 ++++++++++++++++++++++++++++++ crates/mds-cli/tests/warnings.rs | 77 +++++++++++++++ 3 files changed, 257 insertions(+), 14 deletions(-) diff --git a/crates/mds-cli/src/watch.rs b/crates/mds-cli/src/watch.rs index c50997ab..7cb77243 100644 --- a/crates/mds-cli/src/watch.rs +++ b/crates/mds-cli/src/watch.rs @@ -1840,20 +1840,21 @@ fn liveness_probe_dir( if !batch.is_empty() { // Soft-error: vars file may be temporarily absent (AC-W7 / AC-C5). - let runtime_vars = match build_runtime_vars(RuntimeVarArgs { + let resolved = match build_runtime_vars(RuntimeVarArgs { vars: ctx.vars_path_raw.clone(), set_vars: ctx.static_set_vars.clone(), set_string_vars: ctx.static_set_string_vars.clone(), }) { - // Flags are fixed for the session; warned once at startup — discard here. - // The vars-file duplicate-key warnings (#326) are emitted from - // `handle_fs_event_dir` instead, not here: this liveness-probe path is a - // defensive content-backstop/full-reconcile tick that can race the same - // edit's real fs-event delivery (both observing the same changed mtime), - // and emitting from both sites double-counts a single rebuild (test I17 - // guards this — see the module docs on `liveness_probe_dir` and - // `handle_fs_event_dir`). - Ok(v) => v.vars, + // --set/--set-string are fixed for the session and warned once at + // startup — discarded (via `resolved.vars` below). The vars file is + // reloaded on every rebuild (ADR-016); this self-heal path emits under + // the same content-changed gate as `handle_fs_event_dir`, so one + // logical edit observed by both paths still warns once — tests I17 and + // I19. Without this, a self-heal recompile driven purely by this + // content-backstop/full-reconcile tick (no FS event ever delivered, + // e.g. after a root delete+recreate) could print "Recompiled" with no + // vars-file duplicate warning at all. + Ok(v) => v, Err(e) => { eprint_error(e); // Re-baseline so the next tick does not report the same change again @@ -1862,15 +1863,18 @@ fn liveness_probe_dir( return; } }; - process_dir_batch( + let any_changed = process_dir_batch( &batch, false, /* vars_changed */ &ctx.root, &ctx.output_base, - &runtime_vars, + &resolved.vars, ctx.quiet, state, ); + if any_changed { + crate::build::emit_duplicate_vars_file_warnings(&resolved, ctx.quiet); + } } // No baseline refresh here: `process_dir_batch` re-baselines `last_mtimes` over the // post-batch tracked set, and an empty batch means nothing appeared, was removed, or @@ -1995,14 +1999,17 @@ fn handle_fs_event_dir( return DirEventOutcome::Done; } }; - let runtime_vars = resolved.vars.clone(); + // `process_dir_batch` takes the map by reference, so borrow `resolved.vars` + // directly rather than cloning it — `resolved` (and its `.vars_file`, + // `.duplicate_vars_file_keys`, `.duplicate_vars_file_keys_omitted`) is still + // needed below, after this borrow ends, for the warning emission. let any_changed = process_dir_batch( &mds_changed, vars_changed, &ctx.root, &ctx.output_base, - &runtime_vars, + &resolved.vars, ctx.quiet, state, ); diff --git a/crates/mds-cli/tests/cli_watch.rs b/crates/mds-cli/tests/cli_watch.rs index a88dacea..6919308a 100644 --- a/crates/mds-cli/tests/cli_watch.rs +++ b/crates/mds-cli/tests/cli_watch.rs @@ -4454,3 +4454,162 @@ fn i18_duplicate_introduced_mid_session_is_reported_on_the_next_rebuild() { drop(child); } + +// ── I19-I20: liveness self-heal rebuild and --quiet regressions (#326) ─────── +// +// AC-W2 (`watch_dir_mode_root_delete_recreate_recovers`) proves that a root +// delete+recreate kills the recursive watch on the old inode, so the create +// event for a file written into the recreated root is never delivered — only +// `liveness_probe_dir`'s re-arm + full reconcile finds and compiles it. Before +// this fix, that self-heal recompile discarded the resolved `RuntimeVars` and +// never warned about a --vars file duplicate, unlike `handle_fs_event_dir`. + +/// I19: a self-heal rebuild driven by the liveness probe (no FS event ever +/// delivered) must warn about a --vars file duplicate key, same as a genuine +/// FS-event rebuild does. Guards `liveness_probe_dir`'s content-backstop site. +#[test] +fn i19_dir_watch_liveness_self_heal_rebuild_warns_about_vars_file_duplicate() { + let base = tempfile::tempdir().unwrap(); + let root = base.path().join("watched"); + let vars_dir = base.path().join("vars_dir"); + std::fs::create_dir(&root).unwrap(); + std::fs::create_dir(&vars_dir).unwrap(); + std::fs::write(root.join("a.mds"), "---\nname: A\n---\nOld A\n").unwrap(); + let vars_file = vars_dir.join("vars.json"); + std::fs::write(&vars_file, r#"{"x": 1, "x": 2}"#).unwrap(); + let out_dir = base.path().join("out"); + std::fs::create_dir(&out_dir).unwrap(); + + let expected = dup_vars_file_warning("x", &vars_file); + + let (child, stderr_tap) = spawn_ready( + mds_bin() + .args([ + "watch", + root.to_str().unwrap(), + "--out-dir", + out_dir.to_str().unwrap(), + "--vars", + vars_file.to_str().unwrap(), + "--debounce", + "0", + "--poll-interval", + "100", + ]) + .stdout(Stdio::null()), + ); + + // Startup: exactly 1 warning (dir-mode startup, unaffected by this fix). + let startup_stderr = wait_for_stderr_contains_str(&stderr_tap, &expected, TIMEOUT); + assert_eq!( + count_occurrences(&startup_stderr, &expected), + 1, + "I19: dir-watch startup must warn exactly once; stderr:\n{startup_stderr}" + ); + assert!( + wait_for_file_contains(&out_dir.join("a.md"), "Old A", TIMEOUT), + "I19: initial compile should produce 'Old A'" + ); + + // Delete the entire watched root — kills the recursive watch on the old inode + // (same setup as `watch_dir_mode_root_delete_recreate_recovers`). + std::fs::remove_dir_all(&root).unwrap(); + std::thread::sleep(Duration::from_millis(200)); + + // Recreate the root with a brand-new file. TICK-DEPENDENT: the create event + // above is unobservable (new inode, nothing watching it yet) — only the + // liveness probe's re-arm + reconcile self-heal path (`liveness_probe_dir`) + // can find and compile it. + std::fs::create_dir(&root).unwrap(); + std::fs::write( + root.join("new.mds"), + "---\nname: N\n---\nNew file {{name}}\n", + ) + .unwrap(); + + assert!( + wait_for_file_contains(&out_dir.join("new.md"), "New file N", TICK_TIMEOUT), + "I19: watcher must self-heal after root delete+recreate and recompile" + ); + + // The self-heal recompile must ALSO re-warn about the vars-file duplicate — + // proves liveness_probe_dir no longer discards the resolved vars, matching + // handle_fs_event_dir's gate (emit iff the rebuild was observable). + let final_stderr = stderr_tap.text(); + assert_eq!( + count_occurrences(&final_stderr, &expected), + 2, + "I19: the liveness self-heal rebuild must warn about the vars-file \ + duplicate too, not only at startup; stderr:\n{final_stderr}" + ); + + drop(child); +} + +/// I20: `mds watch --quiet` suppresses the vars-file duplicate-key warning on +/// every rebuild, not just at startup. Regression guard for the inner +/// `if quiet { return; }` early-out in `emit_duplicate_vars_file_warnings`: +/// nothing else stops per-rebuild spam under --quiet on the direct watch call +/// sites (`rebuild_file`, `handle_fs_event_dir`, `liveness_probe_dir`), since +/// they call the emitter directly and bypass `emit_duplicate_var_warnings`'s +/// own quiet early-out (that one only guards the startup call sites). +#[test] +fn i20_watch_quiet_suppresses_vars_file_duplicate_warning_on_every_rebuild() { + let base = tempfile::tempdir().unwrap(); + let src_dir = base.path().join("src"); + let vars_dir = base.path().join("vars_dir"); + std::fs::create_dir_all(&src_dir).unwrap(); + std::fs::create_dir_all(&vars_dir).unwrap(); + + let src = src_dir.join("t.mds"); + std::fs::write(&src, "version 1").unwrap(); + let vars_file = vars_dir.join("vars.json"); + std::fs::write(&vars_file, r#"{"x": 1, "x": 2}"#).unwrap(); + let out = src_dir.join("t.md"); + + let expected = dup_vars_file_warning("x", &vars_file); + + let (child, stderr_tap) = spawn_ready( + mds_bin() + .args([ + "watch", + src.to_str().unwrap(), + "--vars", + vars_file.to_str().unwrap(), + "--debounce", + "0", + "--quiet", + ]) + .stdout(Stdio::null()), + ); + + // Positive control (PF-013): the rebuild really happens even though nothing + // warns — otherwise "0 occurrences" below would be vacuous. + assert!( + wait_for_file_contains(&out, "version 1", TIMEOUT), + "I20: startup compile must complete even under --quiet" + ); + let startup_stderr = stderr_tap.text(); + assert_eq!( + count_occurrences(&startup_stderr, &expected), + 0, + "I20: --quiet must suppress the startup vars-file duplicate warning; \ + stderr:\n{startup_stderr}" + ); + + std::fs::write(&src, "version 2").unwrap(); + assert!( + wait_for_file_contains(&out, "version 2", TIMEOUT), + "I20: rebuild after edit must complete even under --quiet" + ); + + let after_edit = stderr_tap.text(); + assert_eq!( + count_occurrences(&after_edit, &expected), + 0, + "I20: --quiet must suppress the vars-file duplicate warning on rebuild \ + too; stderr:\n{after_edit}" + ); + + drop(child); +} diff --git a/crates/mds-cli/tests/warnings.rs b/crates/mds-cli/tests/warnings.rs index 561e48ae..b2997712 100644 --- a/crates/mds-cli/tests/warnings.rs +++ b/crates/mds-cli/tests/warnings.rs @@ -654,6 +654,83 @@ fn i15_hostile_vars_file_key_is_wire_escaped_in_warning() { assert_no_control_chars(&stderr, "I15 stderr"); } +#[test] +fn i15b_hostile_vars_file_key_with_newline_is_wire_escaped() { + // i15 uses ESC (U+001B) and RLO (U+202E), which HUMAN and WIRE escape + // identically (per sanitize_control_chars_wire/sanitize_control_chars: + // "everything else escapes identically in both modes" except the newline + // character) — so i15 cannot tell `safe_inline(key)` apart from a bare + // `key`; the outer `eprint_warning` HUMAN pass masks the difference. The + // newline is the one code point where WIRE differs from HUMAN: HUMAN + // keeps a real newline byte (a forged second stderr line), WIRE escapes + // it to the 6-character uppercase-hex literal form (see + // `sanitize_control_chars_wire`'s own doctest, which pins this exact + // escaped form for a newline). This test pins that WIRE escaping at the + // interpolation site, independently of whatever the outer eprint_warning + // pass does. + // + // The vars file key is written as a raw string containing the JSON escape + // sequence \n (two ASCII characters: backslash, n) inside the JSON text — + // NOT a real newline byte, and never a literal 4-hex \uXXXX sequence in + // this .rs source (PF-018). serde_json decodes that JSON escape to a real + // LF byte in the parsed key. + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("t.mds"); + let vars = dir.path().join("vars.json"); + std::fs::write(&src, "Hello world").unwrap(); + std::fs::write(&vars, r#"{"a\nb": 1, "a\nb": 2}"#).unwrap(); + + let output = mds_bin() + .args([ + "build", + src.to_str().unwrap(), + "-o", + "-", + "--vars", + vars.to_str().unwrap(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .unwrap(); + + assert!( + output.status.success(), + "I15b: build with a newline-in-key vars file must succeed" + ); + let stderr = String::from_utf8(output.stderr).unwrap(); + + // PF-013 NON-VACUITY FIRST: the exact pinned warning line, with the key + // rendered via the 6-character uppercase-hex escape for the newline + // (never a real LF byte), appears exactly once. A weaker check such as + // `stderr.contains("\\u000A")` alone would not be specific enough — this + // asserts the whole pinned line renders correctly, key and all. + let escaped_key = "a\\u000Ab"; + let expected = dup_vars_file_warning(escaped_key, &vars); + assert_eq!( + count_occurrences(&stderr, &expected), + 1, + "I15b: expected the exact pinned warning line with the key WIRE-escaped \ + as {escaped_key:?}; got stderr:\n{stderr:?}" + ); + + // If a raw LF reached eprint_warning unescaped, the key's real newline + // would forge a second stderr line whose text starts with the tail of the + // key ("b' is set more than once..."). Assert no real-newline-delimited + // line looks like that forged fragment — this fails if a raw LF were + // emitted mid-line, independently of the exact-line check above. + for line in stderr.lines() { + assert!( + !line.starts_with("b'"), + "I15b: a stderr line looks like the forged tail of a raw-LF-split \ + hostile key (line: {line:?}); this would only happen if a raw LF \ + reached eprint_warning unescaped; full stderr:\n{stderr:?}" + ); + } + + assert_no_control_chars(&stderr, "I15b stderr"); +} + // ── R2: @include warning precision ─────────────────────────────────────────── #[test] From d03a5f9cf9f63b6bfe44a60156765bae8e8ae651 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 9 Sep 2026 17:23:59 +0300 Subject: [PATCH 5/6] docs(vars): correct self-review comment nits; document empty-key path rendering (#326) --- CHANGELOG.md | 5 +++-- crates/mds-cli/src/watch.rs | 8 ++++---- crates/mds-cli/tests/cli_build.rs | 2 +- crates/mds-cli/tests/cli_watch.rs | 10 +++++++--- crates/mds-cli/tests/common/mod.rs | 5 +++-- crates/mds-core/src/lib.rs | 9 +++++---- crates/mds-core/src/vars_json.rs | 5 +++-- 7 files changed, 26 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cce0d8ec..2ded2982 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,8 +33,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 stays byte-for-byte what the existing parser produced. `load_vars_file`/`load_vars_str` are unchanged in every observable way (same signature, same return type, same errors) and now delegate to the reporting variants. Known limitations: a key containing a - literal `.`, `[`, or `]` renders ambiguously in its reported path; the file-load and - string-load error codes (`mds::invalid_vars` vs `mds::json`) remain deliberately + literal `.`, `[`, or `]` renders ambiguously in its reported path, and an + empty-string key renders as an empty segment; the file-load and string-load + error codes (`mds::invalid_vars` vs `mds::json`) remain deliberately un-unified (pre-existing split, unchanged). - **Fix stale `lint_str` rustdoc and lint-rule Tier tables (#329).** `mds-core`'s `lint_str` rustdoc said "applies the 9 lint rules" after a 10th rule diff --git a/crates/mds-cli/src/watch.rs b/crates/mds-cli/src/watch.rs index 7cb77243..50822a22 100644 --- a/crates/mds-cli/src/watch.rs +++ b/crates/mds-cli/src/watch.rs @@ -950,7 +950,7 @@ fn rebuild_file( // every startup. Instead the resolved vars are held and the warning is // emitted below, gated on `content_changed` — the same signal that gates // the "Recompiled" line — so the vars-file duplicate is re-reported exactly - // once per OBSERVABLE rebuild (tests I9, I16, I18). + // once per OBSERVABLE rebuild (tests I16, I18, I20). let resolved = match build_runtime_vars(RuntimeVarArgs { vars: ctx.vars_path_raw.clone(), set_vars: ctx.static_set_vars.clone(), @@ -1985,7 +1985,7 @@ fn handle_fs_event_dir( // --set/--set-string are fixed for the session and warned once at startup — // discarded (via `resolved.vars` below). The vars file is reloaded on every // rebuild (ADR-016), so its duplicate keys are re-reported too — but only when - // this batch produces an OBSERVABLE rebuild (#326, tests I16-I18): at + // this batch produces an OBSERVABLE rebuild (#326, test I17): at // `--debounce 0` a single edit can generate more than one raw FS event, each // reaching this function separately, so the warning is emitted after // `process_dir_batch` reports whether anything actually changed rather than @@ -2270,8 +2270,8 @@ fn dir_watch_startup( // dir_watch_startup calls build_runtime_vars twice: once above (emit, including // the #326 vars-file duplicate-key warnings) and once here (discard) — emitting // at both sites would double-print every warning (both the --set/--set-string - // ones and the vars-file ones) on directory-watch startup. Tests I9 and I17 are - // the mechanical guards on this. + // ones and the vars-file ones) on directory-watch startup. Test I17 is + // the mechanical guard on this. { let baseline_resolved = build_runtime_vars(RuntimeVarArgs { vars: vars_path_raw.clone(), diff --git a/crates/mds-cli/tests/cli_build.rs b/crates/mds-cli/tests/cli_build.rs index 2dc1c612..84262524 100644 --- a/crates/mds-cli/tests/cli_build.rs +++ b/crates/mds-cli/tests/cli_build.rs @@ -1091,7 +1091,7 @@ fn vars_file_without_duplicates_emits_no_duplicate_warning() { ); } -/// More than [`mds::VarsLoad::duplicate_keys_omitted`]'s cap (1 000) distinct +/// More than [`mds::VarsLoad::duplicate_keys`]'s cap (1 000) distinct /// duplicate paths prints exactly 1 000 warning lines plus one omitted-count tail /// line naming the remainder (D6). #[test] diff --git a/crates/mds-cli/tests/cli_watch.rs b/crates/mds-cli/tests/cli_watch.rs index 6919308a..1d936373 100644 --- a/crates/mds-cli/tests/cli_watch.rs +++ b/crates/mds-cli/tests/cli_watch.rs @@ -4516,10 +4516,14 @@ fn i19_dir_watch_liveness_self_heal_rebuild_warns_about_vars_file_duplicate() { std::fs::remove_dir_all(&root).unwrap(); std::thread::sleep(Duration::from_millis(200)); - // Recreate the root with a brand-new file. TICK-DEPENDENT: the create event - // above is unobservable (new inode, nothing watching it yet) — only the + // Recreate the root with a brand-new file. On Linux/inotify the create event + // above is unobservable (new inode, nothing watching it yet), so only the // liveness probe's re-arm + reconcile self-heal path (`liveness_probe_dir`) - // can find and compile it. + // can find and compile it; on macOS FSEvents watches by path, so the create + // event IS delivered and `handle_fs_event_dir` may service the self-heal + // first instead. Either way, the content-changed gate guarantees exactly one + // warning per observable rebuild, which is what the count assertion below + // pins. std::fs::create_dir(&root).unwrap(); std::fs::write( root.join("new.mds"), diff --git a/crates/mds-cli/tests/common/mod.rs b/crates/mds-cli/tests/common/mod.rs index fe3c1cfd..613b73ae 100644 --- a/crates/mds-cli/tests/common/mod.rs +++ b/crates/mds-cli/tests/common/mod.rs @@ -31,8 +31,9 @@ pub fn mds_bin() -> std::process::Command { pub const DUP_VARS_FILE_WARNING_FMT: &str = "warning: key '{key}' is set more than once in vars file {path}; the last value wins"; -/// Tail line printed when more than [`mds::VarsLoad::duplicate_keys_omitted`] -/// (capped at 1 000) distinct duplicate paths exist (#326). +/// Tail line printed when more distinct duplicate paths exist than the +/// 1 000-path cap on [`mds::VarsLoad::duplicate_keys`] allows; `{n}` is +/// [`mds::VarsLoad::duplicate_keys_omitted`] (#326). #[allow(dead_code)] pub const DUP_VARS_FILE_OMITTED_FMT: &str = "warning: {n} more duplicate keys in vars file {path} are not listed"; diff --git a/crates/mds-core/src/lib.rs b/crates/mds-core/src/lib.rs index 089fc6e5..f38ec028 100644 --- a/crates/mds-core/src/lib.rs +++ b/crates/mds-core/src/lib.rs @@ -1403,8 +1403,9 @@ pub fn scan_imports(source: &str) -> Result, MdsError> { /// enclosing object, at any nesting depth, in encounter order: dotted for /// object nesting (`x.a`), 0-based bracketed for array-element nesting /// (`x[2].a`, or `[0].a` for an array at the document root). A literal key -/// containing `.`, `[`, or `]` renders ambiguously with a nesting separator — -/// a documented, accepted limitation. **These paths are structured, untrusted +/// containing `.`, `[`, or `]` renders ambiguously with a nesting separator, +/// and an empty-string key renders as an empty segment — a documented, +/// accepted limitation. **These paths are structured, untrusted /// text** taken directly from the input JSON: a caller that displays one must /// escape it first (e.g. with [`sanitize_control_chars_wire`]), exactly as for /// any other untrusted identifier. @@ -1413,8 +1414,8 @@ pub fn scan_imports(source: &str) -> Result, MdsError> { /// counts any further distinct duplicate paths beyond that cap. /// /// This type is `#[non_exhaustive]`: new fields may be added in minor releases. -/// Obtain values from the load API above; do not construct via struct literal -/// in external crates. +/// Obtain values from the functions named above; do not construct via struct +/// literal in external crates. #[non_exhaustive] #[derive(Debug, Clone, PartialEq)] pub struct VarsLoad { diff --git a/crates/mds-core/src/vars_json.rs b/crates/mds-core/src/vars_json.rs index fc0f4254..ce149e3f 100644 --- a/crates/mds-core/src/vars_json.rs +++ b/crates/mds-core/src/vars_json.rs @@ -30,8 +30,9 @@ //! - Array-element nesting uses a 0-based bracket index: `x[2].a`, and an array at //! the document root renders as `[0].a`. //! - A literal key containing `.`, `[`, or `]` is **not** escaped — it renders -//! ambiguously with an actual nesting separator. This is a documented, -//! accepted limitation (display-only; the underlying key is never altered). +//! ambiguously with an actual nesting separator, and an empty-string key +//! renders as an empty segment. This is a documented, accepted limitation +//! (display-only; the underlying key is never altered). //! //! # Cap //! From 2a1b996948ee91017d0225d1f5133513022d5ee7 Mon Sep 17 00:00:00 2001 From: Dean Sharon Date: Wed, 9 Sep 2026 17:33:38 +0300 Subject: [PATCH 6/6] refactor(watch): move the vars map instead of cloning it per rebuild; tighten #326 docs (#326) --- CHANGELOG.md | 22 ++++++++++++---------- crates/mds-cli/src/main.rs | 2 +- crates/mds-cli/src/watch.rs | 8 ++++++-- crates/mds-core/src/lib.rs | 5 ++++- crates/mds-core/src/vars_json.rs | 7 ++++++- 5 files changed, 29 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ded2982..e2d49396 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Warn on duplicate keys in `--vars` JSON files, at every depth, on every `mds watch` rebuild (#326).** +- **Warn on duplicate keys in `--vars` JSON files, at every depth, on every `mds watch` rebuild that writes output (#326).** `mds build|check|lint|watch --vars f.json` with a repeated JSON object key (e.g. `{"x": 1, "x": 2}`) previously compiled silently with the last value winning — `serde_json`'s map deserializer discards the earlier value with no signal. Now every @@ -17,15 +17,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the last value wins` for each duplicate, exit 0, suppressed by `--quiet` (parity with the existing `--set`/`--set-string` duplicate-key warning, #200). Duplicates are detected at **every depth** and reported with a dotted/bracketed key path mirroring - `{{a.b}}` interpolation syntax — `x`, `x.a`, `x[2].a`, `[0].a` for an array root - (0-based, display-only; a key that itself contains `.`/`[`/`]` renders ambiguously, - a known and documented limitation). At most 1 000 distinct duplicate key paths are - listed; beyond that a single tail line reports how many more were omitted: - `warning: {n} more duplicate keys in vars file are not listed`. `mds watch` - reloads the vars file from disk on every rebuild (ADR-016), so its duplicate keys are - re-reported on every rebuild too — including a duplicate introduced mid-session by - editing the vars file — while `--set`/`--set-string` duplicate warnings keep their - existing once-at-startup behaviour. New public `mds-core` API: + `{{a.b}}` interpolation syntax — `x`, `x.a`, `x[2].a` (the array-root form `[0].a` + is produced only by the internal scanner and pinned by a unit test; both load + functions reject a non-object root before the scan runs, so a caller of + `mds::load_vars_file`/`load_vars_str` never sees it). At most 1 000 distinct + duplicate key paths are listed; beyond that a single tail line reports how many + more were omitted: `warning: {n} more duplicate keys in vars file are not + listed`. `mds watch` reloads the vars file from disk on every rebuild (ADR-016), + so its duplicate keys are re-reported on every rebuild that writes output too — + including a duplicate introduced mid-session by editing the vars file — while + `--set`/`--set-string` duplicate warnings keep their existing once-at-startup + behaviour. New public `mds-core` API: `mds::VarsLoad { vars, duplicate_keys, duplicate_keys_omitted }` (`#[non_exhaustive]`) and `mds::load_vars_file_reporting_duplicates` / `mds::load_vars_str_reporting_duplicates`, implemented as a second, value-free parse diff --git a/crates/mds-cli/src/main.rs b/crates/mds-cli/src/main.rs index f3a107ee..db9b32a3 100644 --- a/crates/mds-cli/src/main.rs +++ b/crates/mds-cli/src/main.rs @@ -208,7 +208,7 @@ enum Commands { /// Mutually exclusive with -o/--output. #[arg(long = "out-dir", conflicts_with = "output")] out_dir: Option, - /// JSON file with runtime variable overrides (reloaded on each rebuild; a repeated key warns on each rebuild, last value wins) + /// JSON file with runtime variable overrides (reloaded on each rebuild; a repeated key warns on each rebuild that writes output, last value wins) #[arg(long)] vars: Option, /// Set a runtime variable (repeatable, e.g. --set name=Alice --set count=3; repeating a key warns, last value wins) diff --git a/crates/mds-cli/src/watch.rs b/crates/mds-cli/src/watch.rs index 50822a22..acc66f29 100644 --- a/crates/mds-cli/src/watch.rs +++ b/crates/mds-cli/src/watch.rs @@ -951,7 +951,7 @@ fn rebuild_file( // emitted below, gated on `content_changed` — the same signal that gates // the "Recompiled" line — so the vars-file duplicate is re-reported exactly // once per OBSERVABLE rebuild (tests I16, I18, I20). - let resolved = match build_runtime_vars(RuntimeVarArgs { + let mut resolved = match build_runtime_vars(RuntimeVarArgs { vars: ctx.vars_path_raw.clone(), set_vars: ctx.static_set_vars.clone(), set_string_vars: ctx.static_set_string_vars.clone(), @@ -963,7 +963,11 @@ fn rebuild_file( return; } }; - let runtime_vars = resolved.vars.clone(); + // Move the map out instead of cloning it: `compile_to_content` takes + // `runtime_vars` by value, and the emitter below only ever reads + // `resolved.vars_file` / `duplicate_vars_file_keys` / + // `duplicate_vars_file_keys_omitted` — none of which need `.vars`. + let runtime_vars = resolved.vars.take(); let t0 = Instant::now(); match compile_to_content( diff --git a/crates/mds-core/src/lib.rs b/crates/mds-core/src/lib.rs index f38ec028..7e378ad2 100644 --- a/crates/mds-core/src/lib.rs +++ b/crates/mds-core/src/lib.rs @@ -1402,7 +1402,10 @@ pub fn scan_imports(source: &str) -> Result, MdsError> { /// `duplicate_keys` lists the path of each key that repeated within its /// enclosing object, at any nesting depth, in encounter order: dotted for /// object nesting (`x.a`), 0-based bracketed for array-element nesting -/// (`x[2].a`, or `[0].a` for an array at the document root). A literal key +/// (`x[2].a`). Both loading functions above reject a non-object top-level +/// value before the duplicate scan runs, so a caller only ever observes +/// paths that start with a key — never the array-root form (`[0].a`) the +/// internal scanner can otherwise produce. A literal key /// containing `.`, `[`, or `]` renders ambiguously with a nesting separator, /// and an empty-string key renders as an empty segment — a documented, /// accepted limitation. **These paths are structured, untrusted diff --git a/crates/mds-core/src/vars_json.rs b/crates/mds-core/src/vars_json.rs index ce149e3f..db89f08c 100644 --- a/crates/mds-core/src/vars_json.rs +++ b/crates/mds-core/src/vars_json.rs @@ -28,7 +28,12 @@ //! A reported path names a JSON key by walking from the document root: //! - Object nesting is dotted: `x.a`. //! - Array-element nesting uses a 0-based bracket index: `x[2].a`, and an array at -//! the document root renders as `[0].a`. +//! the document root renders as `[0].a`. This scanner produces that array-root +//! form on any JSON text (see the `T15` test below); in practice, though, the +//! public `load_vars_file`/`load_vars_str` (and their `_reporting_duplicates` +//! variants, in `lib.rs`) reject a non-object top-level value before this scan +//! ever runs, so a caller of those functions never observes it — only paths +//! starting with a key are reachable through the public API. //! - A literal key containing `.`, `[`, or `]` is **not** escaped — it renders //! ambiguously with an actual nesting separator, and an empty-string key //! renders as an empty segment. This is a documented, accepted limitation