fix(cli): warn on duplicate keys in --vars JSON files at every depth; fix stale lint_str rustdoc (#326, #329)

CHANGELOG.md

@@ -7,6 +7,49 @@ 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 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
+  subcommand prints `warning: key '<path>' is set more than once in vars file <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` (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 <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
+  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, 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
+  (`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.

README.md

@@ -94,6 +94,7 @@ Build/Watch options:
   --out-dir <DIR>             Output directory (build/single-file watch: <stem>.md or <stem>.json;
                               dir-mode watch: mirrors source subtree)
   --vars <FILE>               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)

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<PathBuf>,
-) -> Result<Option<HashMap<String, mds::Value>>> {
-    path.map(|p| mds::load_vars_file(&p).map_err(miette::Error::from))
+pub(crate) fn load_optional_vars_file(path: Option<PathBuf>) -> Result<Option<mds::VarsLoad>> {
+    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<String>,
     /// Keys that appeared more than once inside `--set-string` (same contract).
     pub(crate) duplicate_set_string_keys: Vec<String>,
+    /// 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<String>,
+    /// 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<PathBuf>,
 }
 
 /// 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<RuntimeVars> {
     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<RuntimeVars> {
         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",
@@ -2262,4 +2321,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}"
+        );
+    }
 }

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<PathBuf>,
-        /// JSON file with runtime variable overrides
+        /// JSON file with runtime variable overrides (a repeated key warns; the last value wins)
         #[arg(long)]
         vars: Option<PathBuf>,
         /// 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<PathBuf>,
-        /// JSON file with runtime variable overrides
+        /// JSON file with runtime variable overrides (a repeated key warns; the last value wins)
         #[arg(long)]
         vars: Option<PathBuf>,
         /// 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<PathBuf>,
         /// 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<PathBuf>,
-        /// 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 that writes output, last value wins)
         #[arg(long)]
         vars: Option<PathBuf>,
         /// Set a runtime variable (repeatable, e.g. --set name=Alice --set count=3; repeating a key warns, last value wins)