fix(mcp): acquire the mutation lock before the confirm-path preflight read (NRN-106)

CHANGELOG.md

@@ -14,6 +14,10 @@ Entries here have landed on `main` but have not yet been cut into a tagged relea
 
 - **`vault.set`'s schema-validation refusals now carry a stable machine-branchable `code`, closing the gap NRN-220 deliberately deferred (NRN-221).** A `norn set` / `vault.set` call that hits a schema or argument refusal — an out-of-schema field value, a value outside `allowed_values`, a `--remove` of a `required_frontmatter` field, a malformed `KEY=VALUE`, a `--push` on a scalar, an unresolved or ambiguous `DOC` target, a cross-class key conflict, or a non-mapping/unparseable frontmatter — previously surfaced as prose only: MCP `vault.set` returned an opaque JSON-RPC error with no `report.error.code`, and the CLI printed prose to stderr and exited 2 with **nothing on stdout**, even under `--format json`. Both surfaces now carry one of twelve new kebab codes — `field-type-invalid`, `field-type-unsupported` (the vault's schema declares a `field_type` norn doesn't support — a config defect, split from `field-type-invalid` so an agent's fix-the-value retry branch never loops against a broken schema), `value-too-long`, `value-not-allowed`, `field-json-invalid`, `required-field-removed`, `target-not-found`, `target-ambiguous`, `assignment-malformed`, `field-conflict`, `push-on-scalar`, `frontmatter-not-mapping` (plus the existing `frontmatter-parse-failed` for a target document with unparseable on-disk frontmatter) — documented in `docs/errors.md`. On MCP, a **confirm** call returns the SAME structured `isError: true` + `report.error.code` shape NRN-220 gave `edit`/`new`, and a **dry-run** (`confirm: false`) forecast of such a refusal converges on the edit/new dry-run contract too: structured `outcome: "refused"` + `error.code` with `isError: false` (previously a bare JSON-RPC `Err`), so an SDK that raises on `isError` does not throw on a preview. On the CLI, `norn set --format json` now emits the NRN-150 `{ code, message, path? }` error envelope on stdout (matching `move`/`delete`), where it previously emitted no stdout at all. Records/TTY behavior is byte-identical (prose on stderr, exit 2), and every error's `Display` prose is unchanged; only the machine-readable surfaces are new. No other command's refusal codes change.
 
+### Fixed
+
+- **MCP mutation tools now acquire the vault mutation lock BEFORE the confirm-path preflight read, closing the same read→lock window NRN-99 fixed for `vault.edit` (NRN-106).** `vault.set`, `vault.delete`, `vault.move`, `vault.new`, `vault.rewrite_wikilink`, and `vault.apply` previously built their plan (loading the graph index, running preflight, resolving the target) and only acquired the per-vault lock afterward — so a concurrent `norn` writer could drift a file in that window and slip past both the plan-time hash checks and the applier's index-snapshot check. On the `confirm: true` path, every mutation tool now locks first, spanning the index load through apply; `confirm: false` (dry-run) stays unlocked, exactly as before. Outputs, exit codes, and envelope shapes are unchanged in the uncontended case; under contention, a caller now sees the lock-timeout refusal ("another norn mutation is in progress against this vault") instead of a plan-validation error that would otherwise fire first. Because the lock is now held across planning and the graph-index load, a concurrent external (non-daemon) writer on a large cold vault is more likely to hit the 5s lock timeout than before — the deliberate correctness tradeoff; a later phase narrows the locked window.
+
 ## v0.46.0 - 2026-07-08
 
 **The daemon-reads-complete release.** Phase 1 of the norn-service initiative exits: every read command (`count`/`find`/`get`, canonical and dynamic-predicate spellings alike) routes through the warm `norn serve` daemon when one is live, the nine graph-index tools build from the daemon's verified-once connection, daemon-side operator notes reach the routed caller, and the new `norn service` verbs adopt the daemon under launchd so it is always warm. The initiative's founding bug — `PRAGMA integrity_check` paid per invocation, O(db-size) — is closed on committed structural evidence at 50k-doc scale (one check per daemon lifetime vs one per call direct; trust never weakened, per ADR 0005).

src/mcp/mutate.rs

@@ -20,10 +20,22 @@ use camino::Utf8Path;
 
 /// Acquire the per-vault mutation lock on an MCP mutation's CONFIRM/apply path.
 ///
-/// Sweeps stale pending markers, then acquires the lock with `is_apply = true` —
-/// every MCP mutation reaches this only after its dry-run early-return, so the
-/// apply is always real. Returns the RAII guard the caller must hold for the
-/// duration of the apply; a timeout or lock error becomes an `anyhow` error.
+/// **THE ordering invariant (NRN-99 / NRN-106), canonical statement:** on
+/// `confirm`, every MCP mutation tool acquires this lock BEFORE any read that
+/// feeds the write — the graph-index load, the query-cache open, preflight,
+/// and plan synthesis all run lock-held — so a concurrent norn writer cannot
+/// drift a file in the read→apply window and slip past both the plan-time
+/// hash checks and the applier's index-snapshot check. Dry-run
+/// (`confirm: false`) NEVER locks: it is read-only by contract. Each handler
+/// guards with `if p.confirm { Some(acquire_mutation_lock(..)?) } else { None }`
+/// at the top of its confirm path, mirroring the CLI arms in `main.rs`, which
+/// lock before their cache load. Cheap, read-free argument validation (plan
+/// parsing, schema-version checks, param-shape refusals) stays BEFORE the
+/// lock so malformed input never contends.
+///
+/// Sweeps stale pending markers, then acquires the lock with `is_apply = true`.
+/// Returns the RAII guard the caller must hold for the duration of the apply;
+/// a timeout or lock error becomes an `anyhow` error.
 ///
 /// This is the MCP analogue of the CLI's lock block in `main.rs`, which differs
 /// deliberately: the CLI maps a timeout to exit code 2 + a stderr line, whereas
@@ -150,6 +162,225 @@ pub(crate) fn open_mutation_event_sink(ctx: &VaultContext) -> EventSink {
     }
 }
 
+#[cfg(test)]
+mod lock_ordering_tests {
+    use crate::mcp::context::VaultContext;
+    use camino::Utf8PathBuf;
+    use fs2::FileExt;
+    use tempfile::TempDir;
+
+    /// RAII cleanup for the per-vault state dir under the REAL XDG state home.
+    ///
+    /// We deliberately do NOT set `XDG_STATE_HOME` in-process:
+    /// `std::env::set_var` is process-global and races other in-binary tests
+    /// that resolve state/cache dirs mid-flight (the same reason
+    /// `server.rs::cold_seeded_vault` documents for the cache dir). Vault
+    /// identity is a hash of the unique tempdir root, so the state dir is
+    /// already test-private; this guard removes it on drop — panic included —
+    /// so the held `.mutation.lock` never outlives the test run.
+    struct StateDirCleanup(Utf8PathBuf);
+    impl Drop for StateDirCleanup {
+        fn drop(&mut self) {
+            let _ = std::fs::remove_dir_all(self.0.as_std_path());
+        }
+    }
+
+    /// Seed a temp vault with one real document, hold its `.mutation.lock`
+    /// exclusively (the same fs2 mechanism `tests/mutation_lock.rs` uses for
+    /// the CLI arms), and return everything the ordering assertions need.
+    fn contended_vault() -> (TempDir, Utf8PathBuf, std::fs::File, StateDirCleanup) {
+        let tmp = tempfile::Builder::new()
+            .prefix("norn-mcp-lock-order-")
+            .tempdir()
+            .unwrap();
+        let root = Utf8PathBuf::from_path_buf(tmp.path().to_path_buf()).unwrap();
+        std::fs::write(root.join("doc.md"), "---\ntype: note\n---\nHello\n").unwrap();
+
+        let (_, state_dir) =
+            crate::cache::state_dir_for(&root).expect("resolve state dir for lock path");
+        std::fs::create_dir_all(state_dir.as_std_path()).unwrap();
+        let cleanup = StateDirCleanup(state_dir.clone());
+        let lock_path = state_dir.join(".mutation.lock");
+        let held = std::fs::OpenOptions::new()
+            .create(true)
+            .truncate(false)
+            .read(true)
+            .write(true)
+            .open(lock_path.as_std_path())
+            .unwrap();
+        held.try_lock_exclusive()
+            .expect("test setup: could not hold the mutation lock");
+        (tmp, root, held, cleanup)
+    }
+
+    /// NRN-106: on the confirm path, EVERY mutation tool acquires the mutation
+    /// lock BEFORE any read that feeds the write. Proven by ORDER, not just
+    /// presence, wherever the harness allows: each case targets a document
+    /// that does not exist, so a preflight-first ordering would fail fast with
+    /// that tool's not-found/preflight error (naming the bogus target) and
+    /// never reach the lock. With the lock held by a simulated concurrent
+    /// writer, every confirm call must instead surface the lock-contention
+    /// refusal — and never a message naming the bogus target.
+    ///
+    /// For `vault.rewrite_wikilink` and `vault.apply` the pre-lock steps
+    /// (index load / plan parse) cannot observably fail on a bogus target, so
+    /// their cases pin contention-refusal-under-held-lock rather than strict
+    /// error precedence.
+    ///
+    /// Runs all seven tools in one test against one contended vault, with the
+    /// debug-only `NORN_MUTATION_LOCK_TIMEOUT_MS` override keeping each
+    /// contended acquire at ~150ms instead of the real 5s.
+    #[test]
+    fn confirm_locks_before_any_preflight_read_across_all_mutation_tools() {
+        // Debug-build-only knob (compiled out of release); other tests never
+        // contend on a mutation lock, so a process-visible short timeout is
+        // inert outside this test.
+        std::env::set_var("NORN_MUTATION_LOCK_TIMEOUT_MS", "150");
+
+        let (_tmp, root, held, _cleanup) = contended_vault();
+        let ctx = VaultContext::open(&root, None).expect("open ctx");
+
+        type Case = (&'static str, Box<dyn Fn(&VaultContext) -> anyhow::Error>);
+        let cases: Vec<Case> = vec![
+            (
+                "set",
+                Box::new(|ctx| {
+                    let mut set = std::collections::BTreeMap::new();
+                    set.insert("status".to_string(), serde_json::json!("active"));
+                    crate::mcp::tools::set::handle(
+                        ctx,
+                        crate::mcp::tools::set::SetParams {
+                            target: "bogus-target".into(),
+                            set,
+                            confirm: true,
+                            ..Default::default()
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.set confirm")
+                }),
+            ),
+            (
+                "edit",
+                Box::new(|ctx| {
+                    crate::mcp::tools::edit::handle(
+                        ctx,
+                        crate::mcp::tools::edit::EditParams {
+                            target: "bogus-target".into(),
+                            edits: vec![crate::edit::ops::EditOp::StrReplace {
+                                old: "Hello".into(),
+                                new: "Goodbye".into(),
+                                replace_all: false,
+                            }],
+                            expected_hash: None,
+                            confirm: true,
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.edit confirm")
+                }),
+            ),
+            (
+                "delete",
+                Box::new(|ctx| {
+                    crate::mcp::tools::delete::handle(
+                        ctx,
+                        crate::mcp::tools::delete::DeleteParams {
+                            target: "bogus-target.md".into(),
+                            rewrite_to: None,
+                            allow_broken_links: true,
+                            confirm: true,
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.delete confirm")
+                }),
+            ),
+            (
+                "move",
+                Box::new(|ctx| {
+                    crate::mcp::tools::move_doc::handle(
+                        ctx,
+                        crate::mcp::tools::move_doc::MoveParams {
+                            from: "bogus-target.md".into(),
+                            to: "dst.md".into(),
+                            confirm: true,
+                            ..Default::default()
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.move confirm")
+                }),
+            ),
+            (
+                "new",
+                Box::new(|ctx| {
+                    crate::mcp::tools::new::handle(
+                        ctx,
+                        crate::mcp::tools::new::NewParams {
+                            // Missing parent dir without `parents` — a
+                            // preflight-first ordering refuses on this path.
+                            path: Some("bogus-target/nested.md".to_string()),
+                            parents: false,
+                            confirm: true,
+                            ..Default::default()
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.new confirm")
+                }),
+            ),
+            (
+                "rewrite_wikilink",
+                Box::new(|ctx| {
+                    crate::mcp::tools::rewrite_wikilink::handle(
+                        ctx,
+                        crate::mcp::tools::rewrite_wikilink::RewriteWikilinkParams {
+                            from: "bogus-target".into(),
+                            to: "doc".into(),
+                            confirm: true,
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.rewrite_wikilink confirm")
+                }),
+            ),
+            (
+                "apply",
+                Box::new(|ctx| {
+                    let plan = serde_json::json!({
+                        "schema_version": 1,
+                        "vault_root": ctx.vault_root.to_string(),
+                        "operations": [{
+                            "kind": "delete_document",
+                            "fields": { "path": "bogus-target.md" }
+                        }]
+                    });
+                    crate::mcp::tools::apply::handle(
+                        ctx,
+                        crate::mcp::tools::apply::ApplyParams {
+                            plan,
+                            confirm: true,
+                            parents: false,
+                        },
+                    )
+                    .expect_err("held lock must refuse vault.apply confirm")
+                }),
+            ),
+        ];
+
+        for (tool, call) in cases {
+            let msg = call(&ctx).to_string();
+            assert!(
+                msg.contains("another norn mutation is in progress"),
+                "[{tool}] expected the lock-contention refusal (proving the \
+                 lock is acquired before the preflight read), got: {msg}"
+            );
+            assert!(
+                !msg.contains("bogus-target"),
+                "[{tool}] no preflight read may run while the lock is held — \
+                 the error must not name the bogus target: {msg}"
+            );
+        }
+
+        drop(held);
+    }
+}
+
 #[cfg(test)]
 mod refusal_tests {
     use super::refusal_from_error;

src/mcp/tools/apply.rs

@@ -118,8 +118,9 @@ pub fn handle_output(ctx: &VaultContext, p: ApplyParams) -> Result<MutationResul
 /// **DRY-RUN (`!confirm`):** no lock, discard sink, applier in `dry_run = true`
 /// mode — forecasts the apply (expansion + preflight) without writing.
 ///
-/// **CONFIRM (`confirm`):** acquire mutation lock, open real event sink, apply
-/// with `dry_run = false` — the same path `norn apply --yes` takes.
+/// **CONFIRM (`confirm`):** acquire the mutation lock BEFORE loading the graph
+/// index, open a real event sink, apply with `dry_run = false` — the same
+/// path `norn apply --yes` takes.
 pub fn handle(ctx: &VaultContext, p: ApplyParams) -> Result<crate::apply_report::ApplyReport> {
     use crate::applier::{apply_migration_plan, ApplyContext};
     use crate::migration_plan::MigrationPlan;
@@ -144,12 +145,21 @@ pub fn handle(ctx: &VaultContext, p: ApplyParams) -> Result<crate::apply_report:
         );
     }
 
+    let dry_run = !p.confirm;
+
+    // CONFIRM locks BEFORE any read that feeds the write (plan parsing above
+    // stays pre-lock — it reads no vault state); dry-run never locks. See
+    // `crate::mcp::mutate::acquire_mutation_lock` for the invariant.
+    let _mutation_lock = if p.confirm {
+        Some(crate::mcp::mutate::acquire_mutation_lock(&cwd)?)
+    } else {
+        None
+    };
+
     // ── Step 3: load graph index (same entry point apply uses) ────────────────
     // Warm-connection reuse under the daemon; fresh open in cold mode (NRN-130).
     let index = ctx.load_graph_index()?;
 
-    let dry_run = !p.confirm;
-
     let apply_ctx = ApplyContext {
         dry_run,
         parents: p.parents,
@@ -171,8 +181,8 @@ pub fn handle(ctx: &VaultContext, p: ApplyParams) -> Result<crate::apply_report:
         return Ok(report);
     }
 
-    // ── CONFIRM: acquire mutation lock, open real sink, apply ──────────────────
-    let _mutation_lock = crate::mcp::mutate::acquire_mutation_lock(&cwd)?;
+    // ── CONFIRM: the mutation lock was already acquired above, before the
+    // index load — open the real sink and apply.
 
     // Open a real, file-backed event sink — the same audit trail `norn apply`
     // writes. `apply_migration_plan` emits the per-op spans and action events