diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e774ad9..d91d4e33 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/src/mcp/mutate.rs b/src/mcp/mutate.rs index a519cd2b..b7d49abb 100644 --- a/src/mcp/mutate.rs +++ b/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 anyhow::Error>); + let cases: Vec = 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; diff --git a/src/mcp/tools/apply.rs b/src/mcp/tools/apply.rs index 89ce82a8..5fe33465 100644 --- a/src/mcp/tools/apply.rs +++ b/src/mcp/tools/apply.rs @@ -118,8 +118,9 @@ pub fn handle_output(ctx: &VaultContext, p: ApplyParams) -> Result Result { use crate::applier::{apply_migration_plan, ApplyContext}; use crate::migration_plan::MigrationPlan; @@ -144,12 +145,21 @@ pub fn handle(ctx: &VaultContext, p: ApplyParams) -> Result Result Result Result { use crate::applier::{apply_migration_plan, ApplyContext}; use crate::migration_plan::{MigrationOp, MigrationPlan, MIGRATION_PLAN_SCHEMA_VERSION}; let cwd = ctx.vault_root.clone(); + // The MCP contract: `confirm` drives apply vs dry-run. + let dry_run = !p.confirm; + + // CONFIRM locks BEFORE any read that feeds the write; 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 + }; + // Load the graph index honoring files.ignore, exactly like the CLI delete path. // Warm-connection reuse under the daemon; fresh open in cold mode (NRN-130). let index = ctx.load_graph_index()?; @@ -162,9 +174,6 @@ pub fn handle(ctx: &VaultContext, p: DeleteParams) -> Result Result Result { anyhow::bail!("edits array is empty"); } - // CONFIRM acquires the per-vault mutation lock BEFORE the preflight read — - // matching `norn edit` (main.rs), which locks before `preflight_and_plan`. - // The lock must span the body read + transform + apply so a concurrent norn - // writer can't drift the file in the read→apply window and slip past both - // the `expected_hash` CAS and the applier's index-snapshot hash check. The - // DRY-RUN path is read-only and takes NO lock. + // CONFIRM locks BEFORE any read that feeds the write (NRN-99); 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 { diff --git a/src/mcp/tools/move_doc.rs b/src/mcp/tools/move_doc.rs index 6a6608c0..dfe8c9e3 100644 --- a/src/mcp/tools/move_doc.rs +++ b/src/mcp/tools/move_doc.rs @@ -128,14 +128,23 @@ pub fn handle_output(ctx: &VaultContext, p: MoveParams) -> Result Result { use crate::applier::{apply_migration_plan, ApplyContext}; use crate::migration_plan::{MigrationOp, MigrationPlan, MIGRATION_PLAN_SCHEMA_VERSION}; let cwd = ctx.vault_root.clone(); + // CONFIRM locks BEFORE any read that feeds the write; 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 + }; + // Load the graph index honoring files.ignore, exactly like the CLI move path. // Warm-connection reuse under the daemon; fresh open in cold mode (NRN-130). let index = ctx.load_graph_index()?; @@ -237,8 +246,8 @@ pub fn handle(ctx: &VaultContext, p: MoveParams) -> Result Result Result { let cwd = ctx.vault_root.clone(); + // CONFIRM locks BEFORE any read that feeds the write; 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 1: Load config + graph index ───────────────────────────────────── // `preflight_and_plan` does this internally; we replicate it so we can // short-circuit at preflight without a real sink and without touching stdin. @@ -310,10 +320,8 @@ pub fn handle(ctx: &VaultContext, p: NewParams) -> Result { .map_err(|e| anyhow::anyhow!("render_json: {e}")); } - // ── CONFIRM: acquire mutation lock, open sink, apply ─────────────────────── - - // Acquire mutation lock — same pattern as `Command::New` in main.rs. - let _mutation_lock = crate::mcp::mutate::acquire_mutation_lock(&cwd)?; + // ── CONFIRM: the mutation lock was already acquired above, before the + // config/index load — open the real sink and apply. // Open a real, file-backed event sink — the same audit trail `norn new --yes` // writes via `apply_and_render` → `open_event_sink`. The MCP analogue is diff --git a/src/mcp/tools/rewrite_wikilink.rs b/src/mcp/tools/rewrite_wikilink.rs index 5b07555f..0a701204 100644 --- a/src/mcp/tools/rewrite_wikilink.rs +++ b/src/mcp/tools/rewrite_wikilink.rs @@ -105,8 +105,9 @@ pub fn handle_output( /// WITHOUT touching any file (and still refuses, via `Err`, when OLD is /// unresolvable). /// -/// CONFIRM (`confirm`): same plan, but acquire the mutation lock, open a real -/// event sink, and apply with `dry_run = false`. +/// CONFIRM (`confirm`): acquire the mutation lock FIRST — before the index +/// load — then build the same plan, open a real event sink, and apply with +/// `dry_run = false`. pub fn handle( ctx: &VaultContext, p: RewriteWikilinkParams, @@ -116,6 +117,14 @@ pub fn handle( let cwd = ctx.vault_root.clone(); + // CONFIRM locks BEFORE any read that feeds the write; 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 + }; + // Load the graph index honoring files.ignore, exactly like the CLI path. // Warm-connection reuse under the daemon; fresh open in cold mode (NRN-130). let index = ctx.load_graph_index()?; @@ -160,8 +169,8 @@ pub fn handle( 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 rewrite-wikilink` writes via `open_event_sink`. `apply_migration_plan` diff --git a/src/mcp/tools/set.rs b/src/mcp/tools/set.rs index fbff5a26..56bb3054 100644 --- a/src/mcp/tools/set.rs +++ b/src/mcp/tools/set.rs @@ -236,6 +236,26 @@ pub fn handle_output(ctx: &VaultContext, p: SetParams) -> Result Result { let cwd = ctx.vault_root.clone(); + // Pure argument validation runs BEFORE the lock so malformed push/pop + // input never contends (the same parse-before-lock discipline vault.apply + // uses for its plan). `push` / `pop` maps route through the CLI's + // string-coercing --push/--pop seam (infer_scalar), so each value renders + // as a bare KEY=VALUE string (not JSON-quoted) — matching + // `norn set --push status=done`. An array value explodes into N sequential + // entries (matching repeated CLI flags); an object, or a nested + // array/object element, is refused rather than stringified into a literal + // list element. + let push = expand_list_ops(&p.push, "push")?; + let pop = expand_list_ops(&p.pop, "pop")?; + + // CONFIRM locks BEFORE any read that feeds the write; 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 + }; + // ONE query_cache call serves both needs: the graph index (honoring // files.ignore, applied at cache-build time) and the cache handle for target // resolution come from the same snapshot — the pipeline (ground-shift, @@ -258,15 +278,6 @@ pub fn handle(ctx: &VaultContext, p: SetParams) -> Result { .map(|(k, v)| Ok(format!("{k}={}", serde_json::to_string(v)?))) .collect::>>()?; - // `push` / `pop` maps route through the CLI's string-coercing --push/--pop - // seam (infer_scalar), so each value renders as a bare KEY=VALUE string (not - // JSON-quoted) — matching `norn set --push status=done`. An array value - // explodes into N sequential entries (matching repeated CLI flags); an - // object, or a nested array/object element, is refused rather than - // stringified into a literal list element. - let push = expand_list_ops(&p.push, "push")?; - let pop = expand_list_ops(&p.pop, "pop")?; - let args = SetArgs { target: p.target.clone(), // `field` is the coercing --field path (string coercion); `set` is the @@ -306,8 +317,8 @@ pub fn handle(ctx: &VaultContext, p: SetParams) -> Result { return Ok(build_report(&outcome, false, "")); } - // CONFIRM: acquire the per-vault mutation lock, then apply. - let _mutation_lock = crate::mcp::mutate::acquire_mutation_lock(&cwd)?; + // CONFIRM: the mutation lock was already acquired above, before the + // preflight read — apply now. // Open a REAL, file-backed event sink on the apply path — the same audit // trail `norn set --yes` writes (lifecycle → op_planned → action → finished). diff --git a/src/mutation_lock/mod.rs b/src/mutation_lock/mod.rs index 4b964370..328df2b3 100644 --- a/src/mutation_lock/mod.rs +++ b/src/mutation_lock/mod.rs @@ -6,6 +6,38 @@ use std::time::Duration; pub const MUTATION_LOCK_TIMEOUT: Duration = Duration::from_secs(5); +/// Test-only override for the mutation-lock acquire timeout, in milliseconds. +/// Lets contention tests hit `CacheError::MutationLockTimeout` in ~100–200ms +/// instead of stalling the suite the full 5s per contended acquire. DEBUG +/// BUILDS ONLY: the env read is compiled out of release builds (see +/// [`mutation_lock_timeout`]), so an inherited value in a production +/// environment (a dev shell, a baked plist env) can never shrink the real 5s +/// budget. Same pattern as the cache write lock's `NORN_CACHE_LOCK_TIMEOUT_MS` +/// (src/cache/lock.rs). +#[cfg(debug_assertions)] +const MUTATION_LOCK_TIMEOUT_ENV: &str = "NORN_MUTATION_LOCK_TIMEOUT_MS"; + +/// The mutation-lock acquire timeout used by every mutating command surface. +/// +/// **Release builds always return [`MUTATION_LOCK_TIMEOUT`] (5s)** — the env +/// read below is `#[cfg(debug_assertions)]`, compiled out entirely, so no +/// environment can alter the production timeout. Debug builds (what +/// `cargo test` builds and what its integration tests spawn) honor +/// `NORN_MUTATION_LOCK_TIMEOUT_MS` when it parses to a positive integer, read +/// at every acquire (not cached) so a test child process scopes the override +/// to its own environment. +fn mutation_lock_timeout() -> Duration { + #[cfg(debug_assertions)] + if let Ok(raw) = std::env::var(MUTATION_LOCK_TIMEOUT_ENV) { + if let Ok(ms) = raw.parse::() { + if ms > 0 { + return Duration::from_millis(ms); + } + } + } + MUTATION_LOCK_TIMEOUT +} + #[derive(Debug)] pub struct MutationLock { _file: std::fs::File, @@ -16,7 +48,7 @@ impl MutationLock { state_dir: &Utf8Path, is_apply: bool, ) -> Result, CacheError> { - Self::acquire_with_timeout(state_dir, is_apply, MUTATION_LOCK_TIMEOUT) + Self::acquire_with_timeout(state_dir, is_apply, mutation_lock_timeout()) } fn acquire_with_timeout(