diff --git a/src/apply_report.rs b/src/apply_report.rs index aa459a6..a92f9a5 100644 --- a/src/apply_report.rs +++ b/src/apply_report.rs @@ -238,6 +238,17 @@ impl ApplyError { }; } } + // A committing routed mutation whose daemon call failed after the + // request was sent (NRN-228): the vault state is UNKNOWN, so a consumer + // must branch to inspect/re-read, never blind-retry. No path — the + // uncertainty spans whatever the tool call targeted. + if let Some(uncertain) = e.downcast_ref::() { + return Self { + code: uncertain.code().to_string(), + message: uncertain.to_string(), + path: None, + }; + } Self { code: "internal-error".to_string(), // `{:#}` renders the full anyhow context chain, matching the prose the @@ -260,6 +271,30 @@ pub struct ApplyWarning { mod tests { use super::*; + /// NRN-228: the routed-mutation uncertainty error keeps its stable code + /// through the anyhow seam — a JSON consumer sees `post-send-uncertain`, + /// not a laundered `internal-error`. + #[test] + fn from_anyhow_recovers_the_post_send_uncertain_code() { + let error = anyhow::Error::new(crate::service::PostSendUncertainError { + tool: "vault.set".to_string(), + cause: anyhow::anyhow!("connection reset"), + }); + let envelope = ApplyError::from_anyhow(&error); + assert_eq!(envelope.code, "post-send-uncertain"); + assert!( + envelope + .message + .contains("the daemon may have applied the change"), + "the message keeps the uncertainty prose; got: {}", + envelope.message + ); + assert_eq!( + envelope.path, None, + "the uncertainty carries no single path" + ); + } + #[test] fn apply_report_serializes_with_per_op_status() { let report = ApplyReport { diff --git a/src/lib.rs b/src/lib.rs index b304fe1..713cb1c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -247,58 +247,160 @@ fn try_route_read( } } -/// The generic read-routing skeleton behind `count`/`find`/`get` (NRN-222). +/// What a routed tool call does when the call fails AFTER the `tools/call` +/// frame has been sent to the daemon (NRN-228). Decided at send time and passed +/// down into [`execute_routed_call`], which shares one body between reads and +/// mutations. A failure BEFORE the send always falls back to Direct regardless +/// of this policy (the tool never ran, so a Direct retry cannot double-apply). +/// +/// [NRN-151/CAS seam: a future safe-retry policy — e.g. a compare-and-swap +/// precondition that makes a post-send retry provably safe — slots in here as +/// another variant without reshaping the seam.] +#[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FallbackAfterSend { + /// Any post-send failure falls back to Direct silently. The request is + /// idempotent — a read, or a dry-run mutation that writes nothing — so a + /// second verified direct open is safe and byte-identical. + Fallback, + /// A post-send failure is NOT retried: the daemon may have applied the + /// mutation, so a Direct re-run could double-apply. Surface an explicit + /// uncertainty error (exit 1) naming the inspect / `--dry-run` remedy. + Commit, +} + +/// The generic routing skeleton shared by reads (`count`/`find`/`get`, NRN-222) +/// and mutations (NRN-228). /// /// Computes the canonical vault root ONCE (threaded into the preamble — NRN-92 -/// review F5), probes the well-known socket, delegates `tool` with `arguments`, -/// then hands the `structuredContent` to `reconstruct` (rebuild the command's -/// native result type) and `emit` (render it exactly like the direct path). Every -/// failure mode — un-canonicalizable root, no/stale daemon, preamble mismatch, -/// transport error, tool error, or an unreadable envelope — returns `None`, so -/// the direct dispatch serves the read (and re-produces any error canonically). -/// The `reconstruct`-then-`emit` split keeps stdout untouched until -/// reconstruction succeeds, so a fall-back to Direct never double-writes. +/// review F5), probes the well-known socket, and delegates to +/// [`execute_routed_call`], which runs `tool` with `arguments` and applies the +/// `after_send` policy. Every pre-flight miss — an un-canonicalizable root or no +/// live daemon — returns `None`, so the direct dispatch serves the request (and +/// re-produces any error canonically). /// -/// `on_tool_error` decides what a result flagged `isError: true` means for -/// this tool (see [`crate::service::OnToolError`]): `vault.get` accepts the -/// payload (its `isError` is the semantic not-found signal — falling back -/// would execute the failing read twice); `count`/`find` fall back to Direct. +/// `on_tool_error` decides what a result flagged `isError: true` means for this +/// tool (see [`crate::service::OnToolError`]): `vault.get` accepts the payload +/// (its `isError` is the semantic not-found signal — falling back would execute +/// the failing read twice); `count`/`find` fall back to Direct. #[cfg(unix)] -fn route_read( +#[allow(clippy::too_many_arguments)] +fn route_tool_call( cwd: &camino::Utf8Path, tool: &str, arguments: serde_json::Value, on_tool_error: crate::service::OnToolError, + after_send: FallbackAfterSend, verbose: bool, reconstruct: impl FnOnce(&serde_json::Value) -> Result, emit: impl FnOnce(T) -> Result, ) -> Option> { // A root that cannot canonicalize cannot be served warm either — fall back to - // Direct, which reports the failure canonically. + // Direct, which reports the failure canonically. A pre-send miss, so both + // policies fall back. let (canonical, _hash) = crate::cache::vault_identity(cwd).ok()?; // Probe the well-known control socket. No daemon => a cheap stat => Direct, // with zero added latency (the common case pays nothing beyond the stat). + // Also a pre-send miss (the request never left this process). let client = match crate::service::probe(crate::service::handshake_timeout()) { crate::service::RouteDecision::Route(client) => client, crate::service::RouteDecision::Direct => return None, }; - let structured = match client.call_tool_structured(&canonical, tool, arguments, on_tool_error) { - Ok(structured) => structured, - Err(error) => { - if verbose { - eprintln!("norn: routed {tool} failed ({error}); using direct execution"); + execute_routed_call( + &canonical, + &client, + tool, + arguments, + on_tool_error, + after_send, + verbose, + reconstruct, + emit, + ) +} + +/// The body shared by every routed call once a live daemon is proven (NRN-228): +/// invoke `tool` on `client`, apply the send-commit `after_send` policy to a +/// failure, then hand a success to `reconstruct` (rebuild the command's native +/// result type) and `emit` (render it exactly like the direct path). +/// +/// The failure branch is the whole point of the send-commit split. A pre-send +/// failure (socket trust, connect, preamble, version gate, MCP initialize) +/// always falls back to Direct — the tool never ran. A post-send failure +/// (timeout, dropped connection, unreadable response) falls back only under +/// `Fallback`; under `Commit` it is surfaced as an explicit uncertainty error +/// (the daemon may have applied the change) instead of risking a double-apply. +/// +/// The `reconstruct`-then-`emit` split keeps stdout untouched until +/// reconstruction succeeds, so a fall-back to Direct never double-writes. +/// +/// Split from [`route_tool_call`] so unit tests can drive it with a stub +/// `ServiceClient` on a temp socket, without touching the process-global env the +/// well-known-socket probe reads. +#[cfg(unix)] +#[allow(clippy::too_many_arguments)] +fn execute_routed_call( + canonical: &camino::Utf8Path, + client: &crate::service::ServiceClient, + tool: &str, + arguments: serde_json::Value, + on_tool_error: crate::service::OnToolError, + after_send: FallbackAfterSend, + verbose: bool, + reconstruct: impl FnOnce(&serde_json::Value) -> Result, + emit: impl FnOnce(T) -> Result, +) -> Option> { + // A committing mutation must use `AcceptWithPayload`: under `FallBackDirect` + // a clean daemon-side refusal (`isError: true`) surfaces as a post-send + // `Err`, which Commit would misreport as a false "may have applied" alarm + // instead of rendering the refusal envelope the payload carries. + debug_assert!( + !(on_tool_error == crate::service::OnToolError::FallBackDirect + && after_send == FallbackAfterSend::Commit), + "a committing routed mutation must use OnToolError::AcceptWithPayload" + ); + let structured = + match client.call_tool_structured_phased(canonical, tool, arguments, on_tool_error) { + Ok(structured) => structured, + Err(error) => { + // Send-commit refusal (NRN-228): a committing mutation whose call + // failed AFTER the request crossed to the daemon must NOT fall + // back to a Direct re-run — the change may already be applied, so + // a retry could double-apply. Surface the uncertainty (exit 1). + // Every other case — any `Fallback` failure, or a PRE-send + // failure in either policy (the tool never ran) — falls back to + // Direct exactly like a read, with the byte-identical verbose note. + // Exhaustive match, NOT `==`: a future policy variant (the + // NRN-151/CAS retry seam) must fail to compile here rather than + // silently falling through to an unguarded Direct re-run. + match after_send { + FallbackAfterSend::Commit => { + if error.phase == crate::service::CallPhase::PostSend { + return Some(Err(post_send_uncertainty_error(tool, error.source))); + } + } + FallbackAfterSend::Fallback => {} + } + if verbose { + eprintln!( + "norn: routed {tool} failed ({}); using direct execution", + error.source + ); + } + return None; } - return None; - } - }; + }; // NRN-218: a daemon-side field-universe gate refusal (an unknown dynamic-field // predicate) crosses back in the envelope. Commit to routing — re-emit the // byte-identical error the direct gate would (exit 1 at the top level), NOT a // fall-back to Direct (which would re-execute the read). Forward any operator // notes FIRST so the stderr ordering matches the direct path, where the cache - // open prints its contention note before the gate error (NRN-215). + // open prints its contention note before the gate error (NRN-215). Ignoring + // `after_send` here is safe under any policy: the gate is pre-execution + // validation, so a refusal is proof the daemon did NOT mutate — surfacing + // the byte-identical refusal (not the uncertainty error) is always correct. if let Some(message) = crate::grammar::dynamic_field_refusal_message(&structured) { for note in crate::mcp::notes::operator_notes_from_structured(&structured) { eprintln!("{note}"); @@ -308,6 +410,16 @@ fn route_read( let value = match reconstruct(&structured) { Ok(value) => value, Err(error) => { + // The call itself SUCCEEDED — the daemon executed the tool — so an + // unreadable envelope here is post-send uncertainty for a committing + // mutation: falling back would re-run an already-applied change. + // Same exhaustive-match rule as the failure branch above. + match after_send { + FallbackAfterSend::Commit => { + return Some(Err(post_send_uncertainty_error(tool, error))); + } + FallbackAfterSend::Fallback => {} + } if verbose { eprintln!( "norn: routed {tool} envelope unreadable ({error}); using direct execution" @@ -332,6 +444,121 @@ fn route_read( Some(emit(value)) } +/// The explicit error a committing routed mutation surfaces when the daemon call +/// fails AFTER the request was sent (NRN-228). Unlike a read, the seam does NOT +/// fall back to a Direct re-run (which could double-apply a change the daemon may +/// already have made); it names the remedy an operator can act on. Maps to exit +/// 1 at the top level — the "vault may be partially mutated" code +/// (`docs/errors.md`), distinct from a clean pre-flight refusal (exit 2). Typed +/// ([`crate::service::PostSendUncertainError`], code `post-send-uncertain`) so +/// the structured failure envelope recovers a machine-branchable code via +/// `ApplyError::from_anyhow` instead of laundering it to `internal-error`. +#[cfg(unix)] +fn post_send_uncertainty_error(tool: &str, source: anyhow::Error) -> anyhow::Error { + anyhow::Error::new(crate::service::PostSendUncertainError { + tool: tool.to_string(), + cause: source, + }) +} + +/// The read-routing skeleton behind `count`/`find`/`get` (NRN-222): route the +/// read, and on ANY failure fall back to Direct. A read is idempotent, so both a +/// pre-send and a post-send failure fall back safely and byte-identically — +/// hence the [`FallbackAfterSend::Fallback`] policy. +#[cfg(unix)] +fn route_read( + cwd: &camino::Utf8Path, + tool: &str, + arguments: serde_json::Value, + on_tool_error: crate::service::OnToolError, + verbose: bool, + reconstruct: impl FnOnce(&serde_json::Value) -> Result, + emit: impl FnOnce(T) -> Result, +) -> Option> { + route_tool_call( + cwd, + tool, + arguments, + on_tool_error, + FallbackAfterSend::Fallback, + verbose, + reconstruct, + emit, + ) +} + +/// The mutation sibling of [`route_read`] (NRN-228): route a mutating tool call +/// to the warm daemon under a send-commit fallback policy. +/// +/// A routed **dry-run** is a read in mutation clothing — it writes nothing — so +/// it uses [`FallbackAfterSend::Fallback`]: any failure, pre- or post-send, +/// silently falls back to Direct, exactly like a read. A routed **apply** uses +/// [`FallbackAfterSend::Commit`]: a failure BEFORE the tool call is sent +/// (forced-direct flags, probe miss, handshake / version-gate failure) falls +/// back to Direct like a read, but a failure AFTER send (timeout, connection +/// drop, unreadable response) does NOT — the daemon may have applied the change, +/// so the seam surfaces an explicit uncertainty error (exit 1) rather than +/// risking a double-apply. +/// +/// Mutation callers pass [`crate::service::OnToolError::AcceptWithPayload`]: +/// the NRN-220 refusal envelopes ride `structuredContent` and flow through +/// `reconstruct` into the CLI's refusal rendering, whereas `FallBackDirect` +/// would turn every clean daemon-side refusal (`isError: true` → a post-send +/// `Err`) into a false "may have applied" alarm under Commit (debug-asserted +/// in `execute_routed_call`). +/// +/// `pub` so it is part of the routing seam a future integration test (and the +/// mutation commands wired in NRN-229+) can reach; no production command routes +/// through it yet, so it is exercised today by the in-crate routing tests. Two +/// notes for the NRN-229 wiring: +/// +/// - Like the whole seam, this is `cfg(unix)`-only (the daemon rides UDS). A +/// cfg-agnostic caller must add a `#[cfg(not(unix))]` stub returning `None`, +/// mirroring `try_route_read`'s Direct stub — always-Direct is safe there +/// because no daemon can exist to have half-applied anything. +/// - The composed path here (well-known-socket probe + policy selection) is +/// deliberately untested: the probe reads the process-global +/// `XDG_CACHE_HOME`, which in-process tests cannot set without racing other +/// tests (the same rule the service-module tests follow). The in-crate seam +/// tests inject a stub `ServiceClient` into `execute_routed_call` instead; +/// the probe half is covered by the e2e `serve_*_routing` suites and will be +/// for mutations once NRN-229 gives them a routable command. +#[cfg(unix)] +#[allow(clippy::too_many_arguments)] +pub fn route_call( + cwd: &camino::Utf8Path, + tool: &str, + arguments: serde_json::Value, + on_tool_error: crate::service::OnToolError, + dry_run: bool, + verbose: bool, + reconstruct: impl FnOnce(&serde_json::Value) -> Result, + emit: impl FnOnce(T) -> Result, +) -> Option> { + route_tool_call( + cwd, + tool, + arguments, + on_tool_error, + after_send_for(dry_run), + verbose, + reconstruct, + emit, + ) +} + +/// The send-commit policy a routed mutation runs under: a dry-run writes nothing +/// (a read in mutation clothing), so it may fall back after send; an apply +/// commits, so a post-send failure is surfaced, never silently retried. +#[cfg(unix)] +fn after_send_for(dry_run: bool) -> FallbackAfterSend { + if dry_run { + FallbackAfterSend::Fallback + } else { + FallbackAfterSend::Commit + } +} + /// Route a `count` to the warm daemon, or `None` to run Direct. #[cfg(unix)] fn route_count( @@ -2056,7 +2283,13 @@ fn exit_code_for(index: &GraphIndex) -> i32 { #[cfg(all(test, unix))] mod tests { - use super::routing_forced_direct; + use super::{after_send_for, execute_routed_call, routing_forced_direct, FallbackAfterSend}; + use crate::service::{bind_trusted, CallPhase, OnToolError, ServiceClient, CONTROL_PROTOCOL}; + use anyhow::Result; + use camino::{Utf8Path, Utf8PathBuf}; + use std::io::{BufRead, BufReader, Write}; + use std::os::unix::net::UnixListener; + use std::thread; /// F1 seam-level gate: BOTH `--config` and `--no-cache-refresh` force a /// routable read onto the Direct path, independent of any live daemon. The @@ -2075,4 +2308,315 @@ mod tests { ); assert!(routing_forced_direct(true, true), "both flags force Direct"); } + + // ── NRN-228 route_call send-commit seam ────────────────────────────────── + // + // These drive `execute_routed_call` (the body `route_call` and `route_read` + // share once a daemon is proven live) against a stub UDS daemon on a temp + // socket, so the send-commit policy is observed end-to-end without touching + // the process-global env the well-known-socket probe reads. + + /// How far the stub daemon drives the request wire before dropping (or + /// completing) the connection. + enum Stub { + /// PRE-send: read `hello`, then drop without a `ready` frame — the + /// client fails at the preamble, before the `tools/call` frame is ever + /// written. + DropBeforeReady, + /// POST-send: complete `hello`/`ready` and MCP `initialize`, read the + /// `tools/call`, then drop WITHOUT a response — the request has crossed + /// to the daemon, but no result comes back. + DropAfterCall, + /// Success: like `DropAfterCall`, but answer the `tools/call` with a + /// valid success envelope carrying `structuredContent` — the daemon + /// EXECUTED the tool; any later failure (e.g. `reconstruct`) is + /// post-send by construction. + RespondOk, + } + + /// Spawn a stub daemon behaving per [`Stub`]. + fn spawn_stub(listener: UnixListener, mode: Stub) -> thread::JoinHandle<()> { + thread::spawn(move || { + let (conn, _) = listener.accept().unwrap(); + let mut reader = BufReader::new(conn.try_clone().unwrap()); + let mut w = conn; + + let mut hello = String::new(); + reader.read_line(&mut hello).unwrap(); + if matches!(mode, Stub::DropBeforeReady) { + // Pre-send: never answer `ready`. Dropping both fds (w now, the + // reader clone at return) gives the client a prompt EOF. + return; + } + writeln!( + w, + "{}", + serde_json::json!({ + "norn_control": "ready", "protocol": CONTROL_PROTOCOL, + "version": env!("CARGO_PKG_VERSION"), + }) + ) + .unwrap(); + w.flush().unwrap(); + + let mut init = String::new(); + reader.read_line(&mut init).unwrap(); + writeln!( + w, + "{}", + serde_json::json!({"jsonrpc": "2.0", "id": 1, "result": {"ok": true}}) + ) + .unwrap(); + w.flush().unwrap(); + + // Read the tools/call (so the client's write completes)... + let mut call = String::new(); + reader.read_line(&mut call).unwrap(); + if matches!(mode, Stub::DropAfterCall) { + // ...then drop without responding — the post-send failure. + return; + } + // ...and answer it with a valid success envelope (`RespondOk`). + writeln!( + w, + "{}", + serde_json::json!({ + "jsonrpc": "2.0", "id": 2, + "result": { + "content": [{"type": "text", "text": "{\"ok\":true}"}], + "structuredContent": {"ok": true}, + "isError": false, + } + }) + ) + .unwrap(); + w.flush().unwrap(); + }) + } + + /// `reconstruct`/`emit` must never run on a FAILED routed call — the failure + /// branch returns before them. Panicking closures prove it. + fn reconstruct_never(_: &serde_json::Value) -> Result<()> { + panic!("reconstruct must not run on a failed routed call") + } + fn emit_never(_: ()) -> Result { + panic!("emit must not run on a failed routed call") + } + + /// The policy mapping: a dry-run writes nothing (a read in mutation + /// clothing) so it may fall back after send; an apply commits, so a + /// post-send failure is surfaced, never silently retried. + #[test] + fn after_send_for_maps_dry_run_and_apply() { + assert_eq!( + after_send_for(true), + FallbackAfterSend::Fallback, + "a dry-run may fall back after send" + ); + assert_eq!( + after_send_for(false), + FallbackAfterSend::Commit, + "an apply commits — no post-send fallback" + ); + } + + /// Commit mode, PRE-send failure: the tool never ran, so the seam falls back + /// to Direct (returns `None`) exactly like a read. + #[test] + fn commit_falls_back_to_direct_on_a_pre_send_failure() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("s.sock")).unwrap(); + let stub = spawn_stub(bind_trusted(&path), Stub::DropBeforeReady); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let out = execute_routed_call( + Utf8Path::new("/vaults/atlas"), + &client, + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + FallbackAfterSend::Commit, + false, + reconstruct_never, + emit_never, + ); + assert!( + out.is_none(), + "a pre-send failure under Commit must fall back to Direct (None)" + ); + stub.join().unwrap(); + } + + /// Commit mode, POST-send failure: the daemon may have applied the change, + /// so the seam does NOT fall back. It returns `Some(Err(..))` — an error the + /// top level maps to exit 1 — whose message names the inspect / `--dry-run` + /// remedy. `Some` (not `None`) is the no-fallback proof. + #[test] + fn commit_surfaces_uncertainty_on_a_post_send_failure() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("s.sock")).unwrap(); + let stub = spawn_stub(bind_trusted(&path), Stub::DropAfterCall); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let out = execute_routed_call( + Utf8Path::new("/vaults/atlas"), + &client, + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + FallbackAfterSend::Commit, + false, + reconstruct_never, + emit_never, + ); + let result = out.expect("Commit must NOT fall back after send (Some, not None)"); + let error = result.expect_err("a post-send Commit failure is an error, not a success"); + let message = format!("{error}"); + assert!( + message.contains("the daemon may have applied the change"), + "the error names the uncertainty; got: {message}" + ); + assert!( + message.contains("--dry-run"), + "the error names the --dry-run remedy; got: {message}" + ); + assert!( + message.contains("norn get"), + "the error names the inspect remedy; got: {message}" + ); + // NRN-220: the error is typed, so the structured failure envelope + // recovers the stable machine code from the seam's actual error value. + assert_eq!( + crate::apply_report::ApplyError::from_anyhow(&error).code, + "post-send-uncertain" + ); + stub.join().unwrap(); + } + + /// Fallback mode (a routed dry-run), the SAME post-send failure: because a + /// dry-run writes nothing, the seam silently falls back to Direct (`None`). + /// The only difference from the Commit case above is the policy. + #[test] + fn dry_run_fallback_is_silent_on_a_post_send_failure() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("s.sock")).unwrap(); + let stub = spawn_stub(bind_trusted(&path), Stub::DropAfterCall); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let out = execute_routed_call( + Utf8Path::new("/vaults/atlas"), + &client, + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + FallbackAfterSend::Fallback, + false, + reconstruct_never, + emit_never, + ); + assert!( + out.is_none(), + "a dry-run (Fallback) post-send failure must silently fall back to Direct (None)" + ); + stub.join().unwrap(); + } + + /// Commit mode, `reconstruct` failure AFTER a successful call: the daemon + /// EXECUTED the tool (the envelope came back fine — this process just can't + /// read it), so falling back to Direct would double-apply. The seam must + /// surface the same uncertainty error as a dropped response, not `None`. + #[test] + fn commit_surfaces_uncertainty_on_a_reconstruct_failure() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("s.sock")).unwrap(); + let stub = spawn_stub(bind_trusted(&path), Stub::RespondOk); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let out = execute_routed_call( + Utf8Path::new("/vaults/atlas"), + &client, + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + FallbackAfterSend::Commit, + false, + |_| -> Result<()> { Err(anyhow::anyhow!("envelope shape mismatch")) }, + emit_never, + ); + let result = out.expect("Commit must NOT fall back on a reconstruct failure (Some)"); + let error = result.expect_err("a post-success reconstruct failure is an error"); + let message = format!("{error}"); + assert!( + message.contains("the daemon may have applied the change"), + "the error names the uncertainty; got: {message}" + ); + assert!( + message.contains("--dry-run") && message.contains("norn get"), + "the error names the inspect / --dry-run remedy; got: {message}" + ); + stub.join().unwrap(); + } + + /// Fallback mode (a routed dry-run), the SAME post-success `reconstruct` + /// failure: a dry-run writes nothing, so the seam keeps today's silent + /// fall-back to Direct (`None`). + #[test] + fn dry_run_fallback_is_silent_on_a_reconstruct_failure() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("s.sock")).unwrap(); + let stub = spawn_stub(bind_trusted(&path), Stub::RespondOk); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let out = execute_routed_call( + Utf8Path::new("/vaults/atlas"), + &client, + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + FallbackAfterSend::Fallback, + false, + |_| -> Result<()> { Err(anyhow::anyhow!("envelope shape mismatch")) }, + emit_never, + ); + assert!( + out.is_none(), + "a dry-run (Fallback) reconstruct failure must silently fall back to Direct (None)" + ); + stub.join().unwrap(); + } + + /// The stub's PRE-send drop really does fail the client before the tool call + /// is sent — proven at the phase-tagged layer so the fallback tests above + /// can't pass on a mis-tagged phase. (POST-send tagging is proven by the + /// service-module unit tests.) + #[test] + fn stub_pre_send_drop_is_tagged_pre_send() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("s.sock")).unwrap(); + let stub = spawn_stub(bind_trusted(&path), Stub::DropBeforeReady); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let error = client + .call_tool_structured_phased( + Utf8Path::new("/vaults/atlas"), + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + ) + .expect_err("a dropped preamble must be Err"); + assert_eq!(error.phase, CallPhase::PreSend); + stub.join().unwrap(); + } } diff --git a/src/service/mod.rs b/src/service/mod.rs index 2b195c7..09b9a90 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -51,7 +51,7 @@ //! //! This lands the *client half*: host-socket-path derivation, the probe, the //! control-frame protocol, and (NRN-94) the request path — -//! [`ServiceClient::call_tool_structured`] sends the `hello` vault preamble and +//! [`ServiceClient::call_tool_structured_phased`] sends the `hello` vault preamble and //! runs the MCP `initialize` + `tools/call` exchange over the same stream. The //! daemon that answers is NRN-93. The routing seam in `src/lib.rs` decides which //! commands actually route (only `count` today — see `try_route_read`); every @@ -274,7 +274,7 @@ pub enum RouteDecision { #[cfg(unix)] pub struct ServiceClient { /// The socket a handshake just succeeded against, and where the request - /// connection ([`ServiceClient::call_tool_structured`]) reconnects. + /// connection ([`ServiceClient::call_tool_structured_phased`]) reconnects. pub socket_path: Utf8PathBuf, } @@ -809,6 +809,110 @@ fn read_line_capped( } } +/// Where a routed tool call failed, relative to the moment the `tools/call` +/// frame crosses to the daemon (NRN-228). +/// +/// For a read this distinction is invisible (every failure falls back to a +/// verified direct open either way). For a *mutation* it is the whole ballgame: +/// a pre-send failure never reached the tool, so a Direct retry is safe; a +/// post-send failure may have applied, so a Direct retry could double-apply. +/// [`ServiceClient::call_tool_structured_phased`] tags every failure with one of +/// these so a send-commit policy (see the routing seam's `route_call`) can decide. +#[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CallPhase { + /// The `tools/call` frame was never written — the failure happened during + /// the socket-trust check, the request connect, the `hello`/`ready` + /// preamble, the request-connection version gate, or the MCP `initialize`. + /// The tool never ran, so re-running Direct cannot double-apply. + PreSend, + /// The `tools/call` frame was written; the daemon may have executed the + /// tool. The failure is a lost/garbled/timed-out response — NOT proof the + /// tool did not run — so a mutation must not silently retry Direct. + PostSend, +} + +/// A routed tool-call failure carrying its [`CallPhase`] (NRN-228). The read +/// path collapses this to a plain `anyhow::Error` (it falls back regardless); +/// the mutation path branches on `phase`. +#[cfg(unix)] +#[derive(Debug)] +pub struct CallToolError { + /// Whether the tool request had already been sent when the failure hit. + pub phase: CallPhase, + /// The underlying failure, preserved verbatim for the operator-facing message. + pub source: anyhow::Error, +} + +#[cfg(unix)] +impl CallToolError { + /// Tag a failure that struck before the `tools/call` frame was written. + fn pre_send(source: impl Into) -> Self { + Self { + phase: CallPhase::PreSend, + source: source.into(), + } + } + + /// Tag a failure that struck at or after the `tools/call` frame was written. + fn post_send(source: impl Into) -> Self { + Self { + phase: CallPhase::PostSend, + source: source.into(), + } + } +} + +#[cfg(unix)] +impl std::fmt::Display for CallToolError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.source) + } +} + +#[cfg(unix)] +impl std::error::Error for CallToolError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(self.source.as_ref()) + } +} + +/// The typed error a committing routed mutation surfaces when the daemon call +/// fails AFTER the request was sent (NRN-228): the seam refuses to fall back to +/// a Direct re-run (which could double-apply a change the daemon may already +/// have made) and names the remedy instead. +/// +/// Typed (not bare anyhow prose) so [`code`](Self::code) rides the +/// `ApplyError::from_anyhow` downcast chain into the structured `{code, +/// message}` failure envelope (NRN-150/220) — a JSON consumer branches on +/// `post-send-uncertain`, never the prose. Cross-platform on purpose: the +/// downcast chain references it on every target, though only the unix routing +/// seam ever constructs it. +/// +/// `cause` is deliberately NOT the std `source` (its text already rides the +/// Display message; chaining it as `source` too would double-print it under +/// anyhow's `{:#}` rendering). +#[derive(Debug, thiserror::Error)] +#[error( + "routed {tool} failed after the request was sent ({cause}); the daemon may have \ + applied the change. Inspect the target (e.g. `norn get `) or re-run with \ + --dry-run before retrying." +)] +pub struct PostSendUncertainError { + /// The MCP tool name of the mutation whose outcome is unknown. + pub tool: String, + /// The underlying failure (transport drop, timeout, unreadable envelope). + pub cause: anyhow::Error, +} + +impl PostSendUncertainError { + /// The stable, machine-branchable kebab code (NRN-220), mirroring the + /// `.code()` convention of `standards::apply::ApplyError` / `EditError`. + pub fn code(&self) -> &'static str { + "post-send-uncertain" + } +} + /// The request half of the routing seam (NRN-94). /// /// A [`ServiceClient`] is proof that a daemon answered the liveness ping on a @@ -844,21 +948,32 @@ impl ServiceClient { /// `tool` is the MCP tool name (e.g. `"vault.count"`); `arguments` is the /// tool's parameter object; `on_tool_error` decides what a result flagged /// `isError: true` means (see [`OnToolError`]). - pub fn call_tool_structured( + /// Route one tool call to the warm daemon and return its `structuredContent` + /// JSON, tagging every failure with the [`CallPhase`] about the moment the + /// `tools/call` frame is written (the send boundary). A read discards the + /// phase (it falls back to Direct on any failure — see `route_read`); a + /// mutation branches on it, refusing to fall back once the request has + /// crossed to the daemon (see `route_call`). NRN-228. + pub fn call_tool_structured_phased( &self, vault_root: &Utf8Path, tool: &str, arguments: serde_json::Value, on_tool_error: OnToolError, - ) -> anyhow::Result { + ) -> Result { use std::io::BufReader; + // ── PRE-SEND: nothing below has written the tool call yet, so every + // failure here is `CallPhase::PreSend` — the tool never ran. ── + // TRUST (NRN-94 review F4): before naming the vault, verify the socket is // ours. A squatter who binds the well-known path must not learn vault // paths (from the `hello`) or serve forged counts. On any mismatch, bail // so the caller falls back to a verified direct open — trust over speed. if !socket_is_trusted(&self.socket_path) { - anyhow::bail!("service socket failed the ownership check; using direct execution"); + return Err(CallToolError::pre_send(anyhow::anyhow!( + "service socket failed the ownership check; using direct execution" + ))); } // One overall wall-clock deadline for the whole routed read (F3): a @@ -867,12 +982,17 @@ impl ServiceClient { let deadline = std::time::Instant::now() + ROUTED_READ_BUDGET; // Fresh request connection, bounded by the same budget. - let stream = connect_control(&self.socket_path, ROUTED_READ_BUDGET)?; + let stream = connect_control(&self.socket_path, ROUTED_READ_BUDGET) + .map_err(CallToolError::pre_send)?; // `SO_RCVTIMEO` is the per-read poll interval (short, so the reader wakes // to re-check the deadline); the deadline bounds the cumulative time. Set // once — per-read `setsockopt` trips EINVAL on macOS under load. - stream.set_read_timeout(Some(REQUEST_POLL_INTERVAL))?; - stream.set_write_timeout(Some(ROUTED_READ_BUDGET))?; + stream + .set_read_timeout(Some(REQUEST_POLL_INTERVAL)) + .map_err(CallToolError::pre_send)?; + stream + .set_write_timeout(Some(ROUTED_READ_BUDGET)) + .map_err(CallToolError::pre_send)?; // Read via a BufReader over `&stream`; write via `&stream` directly. Both // are shared borrows, so no `try_clone` (whose dup'd-fd churn trips the // macOS `SO_RCVTIMEO` EINVAL noted on the probe path). @@ -885,12 +1005,19 @@ impl ServiceClient { protocol: CONTROL_PROTOCOL, vault_root: vault_root.as_str().to_string(), }; - write_json_line(&stream, &hello)?; - let ready = read_json_value(&mut reader, deadline)? - .ok_or_else(|| anyhow::anyhow!("daemon closed before answering hello"))?; + write_json_line(&stream, &hello).map_err(CallToolError::pre_send)?; + let ready = read_json_value(&mut reader, deadline) + .map_err(CallToolError::pre_send)? + .ok_or_else(|| { + CallToolError::pre_send(anyhow::anyhow!("daemon closed before answering hello")) + })?; match ready.get("norn_control").and_then(|v| v.as_str()) { Some("ready") => {} - _ => anyhow::bail!("daemon did not answer hello with ready: {ready}"), + _ => { + return Err(CallToolError::pre_send(anyhow::anyhow!( + "daemon did not answer hello with ready: {ready}" + ))) + } } // Version gate on the REQUEST connection too (NRN-222 review): the // probe's ping is version-gated, but this is a separate connection — a @@ -903,13 +1030,20 @@ impl ServiceClient { Some(version) if version == client_version => {} Some(server) => { warn_version_skew(server, client_version); - anyhow::bail!("service/client version skew on the request connection"); + return Err(CallToolError::pre_send(anyhow::anyhow!( + "service/client version skew on the request connection" + ))); + } + None => { + return Err(CallToolError::pre_send(anyhow::anyhow!( + "ready frame carries no version; refusing to route" + ))) } - None => anyhow::bail!("ready frame carries no version; refusing to route"), } // 2. MCP handshake. The daemon serves rmcp over the remainder of the - // stream; `initialize` must succeed before any `tools/call`. + // stream; `initialize` must succeed before any `tools/call`. Still + // pre-send — `initialize` mutates nothing. let init = json_rpc_request( 1, "initialize", @@ -919,28 +1053,41 @@ impl ServiceClient { "clientInfo": { "name": "norn-cli", "version": env!("CARGO_PKG_VERSION") }, }), ); - write_json_line(&stream, &init)?; - let init_resp = read_rpc_response(&mut reader, 1, deadline)?; + write_json_line(&stream, &init).map_err(CallToolError::pre_send)?; + let init_resp = + read_rpc_response(&mut reader, 1, deadline).map_err(CallToolError::pre_send)?; if let Some(err) = init_resp.get("error") { - anyhow::bail!("MCP initialize failed: {err}"); + return Err(CallToolError::pre_send(anyhow::anyhow!( + "MCP initialize failed: {err}" + ))); } - // 3. The actual tool call. A JSON-RPC `error` OR a `result` flagged - // `isError` is a tool-level failure — bail so the caller falls back to - // Direct, which re-produces the identical error and exit code. + // ── SEND BOUNDARY: writing the `tools/call` frame commits the request to + // the daemon. From here every failure is `CallPhase::PostSend` — the + // tool may have run, so a mutation caller must NOT silently retry + // Direct. A JSON-RPC `error` OR a `result` flagged `isError` is a + // tool-level failure; for a read the caller falls back to Direct + // (which re-produces the identical error and exit code). ── let call = json_rpc_request( 2, "tools/call", serde_json::json!({ "name": tool, "arguments": arguments }), ); - write_json_line(&stream, &call)?; - let resp = read_rpc_response(&mut reader, 2, deadline)?; + // The WRITE itself is tagged post-send even though, at today's newline + // framing, a write failure provably means the daemon never parsed the + // frame. The tag deliberately does not depend on that framing invariant: + // mutation safety must not couple to a separate component's parser + // behavior, so the conservative tag holds across daemon changes. + write_json_line(&stream, &call).map_err(CallToolError::post_send)?; + let resp = read_rpc_response(&mut reader, 2, deadline).map_err(CallToolError::post_send)?; if let Some(err) = resp.get("error") { - anyhow::bail!("tool '{tool}' failed on the daemon: {err}"); + return Err(CallToolError::post_send(anyhow::anyhow!( + "tool '{tool}' failed on the daemon: {err}" + ))); } - let result = resp - .get("result") - .ok_or_else(|| anyhow::anyhow!("tool '{tool}' returned no result"))?; + let result = resp.get("result").ok_or_else(|| { + CallToolError::post_send(anyhow::anyhow!("tool '{tool}' returned no result")) + })?; // F5: a successful JSON-RPC envelope can still carry a tool-level failure // via `isError: true` (an MCP CallToolResult convention). Under // `FallBackDirect`, treat it as a failure so we do not render a @@ -951,13 +1098,19 @@ impl ServiceClient { if on_tool_error == OnToolError::FallBackDirect && result.get("isError").and_then(serde_json::Value::as_bool) == Some(true) { - anyhow::bail!("tool '{tool}' reported isError; using direct execution"); + return Err(CallToolError::post_send(anyhow::anyhow!( + "tool '{tool}' reported isError; using direct execution" + ))); } let structured = result .get("structuredContent") .cloned() .filter(|v| !v.is_null()) - .ok_or_else(|| anyhow::anyhow!("tool '{tool}' returned no structuredContent"))?; + .ok_or_else(|| { + CallToolError::post_send(anyhow::anyhow!( + "tool '{tool}' returned no structuredContent" + )) + })?; Ok(structured) } } @@ -1077,6 +1230,19 @@ fn read_rpc_response( anyhow::bail!("no JSON-RPC response for request {id}") } +/// Bind a stub listener and pin the socket to 0600, so the request path's +/// F4 trust gate ([`socket_is_trusted`]) accepts it regardless of the ambient +/// umask (which could otherwise leave it group/other-writable). Shared by this +/// module's request-path tests and the crate-root routing-seam tests (NRN-228) +/// — hence hoisted out of the inner `tests` module. +#[cfg(all(test, unix))] +pub(crate) fn bind_trusted(path: &Utf8Path) -> std::os::unix::net::UnixListener { + use std::os::unix::fs::PermissionsExt; + let listener = std::os::unix::net::UnixListener::bind(path.as_std_path()).unwrap(); + std::fs::set_permissions(path.as_std_path(), std::fs::Permissions::from_mode(0o600)).unwrap(); + listener +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -1084,17 +1250,6 @@ mod tests { use std::os::unix::net::UnixListener; use std::thread; - /// Bind a stub listener and pin the socket to 0600, so the request path's - /// F4 trust gate (`socket_is_trusted`) accepts it regardless of the ambient - /// umask (which could otherwise leave it group/other-writable). - fn bind_trusted(path: &Utf8PathBuf) -> UnixListener { - use std::os::unix::fs::PermissionsExt; - let listener = UnixListener::bind(path).unwrap(); - std::fs::set_permissions(path.as_std_path(), std::fs::Permissions::from_mode(0o600)) - .unwrap(); - listener - } - /// The host socket path is deterministic (same process env => same /// answer every call) and lands at the well-known `norn/run/norn.sock` /// suffix under the ambient cache tree. @@ -1193,7 +1348,7 @@ mod tests { }; let vault_root = Utf8PathBuf::from("/vaults/atlas"); let structured = client - .call_tool_structured( + .call_tool_structured_phased( &vault_root, "vault.count", serde_json::json!({"by": "type"}), @@ -1236,7 +1391,7 @@ mod tests { let client = ServiceClient { socket_path: path.clone(), }; - let result = client.call_tool_structured( + let result = client.call_tool_structured_phased( &Utf8PathBuf::from("/vaults/atlas"), "vault.count", serde_json::json!({}), @@ -1288,7 +1443,7 @@ mod tests { socket_path: path.clone(), }; let error = client - .call_tool_structured( + .call_tool_structured_phased( &Utf8PathBuf::from("/vaults/atlas"), "vault.count", serde_json::json!({}), @@ -1364,7 +1519,7 @@ mod tests { let client = ServiceClient { socket_path: path.clone(), }; - let result = client.call_tool_structured( + let result = client.call_tool_structured_phased( &Utf8PathBuf::from("/vaults/atlas"), "vault.count", serde_json::json!({}), @@ -1434,7 +1589,7 @@ mod tests { socket_path: path.clone(), }; let structured = client - .call_tool_structured( + .call_tool_structured_phased( &Utf8PathBuf::from("/vaults/atlas"), "vault.get", serde_json::json!({"targets": ["nope"]}), @@ -1448,6 +1603,108 @@ mod tests { server.join().unwrap(); } + /// NRN-228: a failure BEFORE the `tools/call` frame is written is tagged + /// `CallPhase::PreSend` — the tool never ran. The stub accepts `hello` and + /// then drops the connection without a `ready` frame, so the client fails at + /// the preamble. A mutation caller can safely fall back to Direct here. + #[test] + fn call_tool_structured_phased_tags_a_preamble_failure_pre_send() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("service.sock")).unwrap(); + let listener = bind_trusted(&path); + + let server = thread::spawn(move || { + let (conn, _) = listener.accept().unwrap(); + let mut reader = BufReader::new(conn.try_clone().unwrap()); + let mut hello_line = String::new(); + reader.read_line(&mut hello_line).unwrap(); + // Drop without answering `ready`: the tool call is never written. + drop(conn); + }); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let error = client + .call_tool_structured_phased( + &Utf8PathBuf::from("/vaults/atlas"), + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + ) + .expect_err("a dropped preamble must be Err"); + assert_eq!( + error.phase, + CallPhase::PreSend, + "a pre-`tools/call` failure must be tagged PreSend (got {error})" + ); + server.join().unwrap(); + } + + /// NRN-228: a failure AFTER the `tools/call` frame is written is tagged + /// `CallPhase::PostSend` — the daemon may have run the tool. The stub + /// completes `hello`/`ready` and `initialize`, reads the `tools/call`, then + /// drops WITHOUT a response, so the client fails reading the tool result. + /// A mutation caller must NOT silently retry Direct here. + #[test] + fn call_tool_structured_phased_tags_a_dropped_response_post_send() { + let dir = tempfile::tempdir().unwrap(); + let path = Utf8PathBuf::from_path_buf(dir.path().join("service.sock")).unwrap(); + let listener = bind_trusted(&path); + + let server = thread::spawn(move || { + let (conn, _) = listener.accept().unwrap(); + let mut reader = BufReader::new(conn.try_clone().unwrap()); + let mut w = conn; + // hello → ready + let mut hello_line = String::new(); + reader.read_line(&mut hello_line).unwrap(); + writeln!( + w, + "{}", + serde_json::json!({ + "norn_control": "ready", "protocol": CONTROL_PROTOCOL, + "version": env!("CARGO_PKG_VERSION"), + }) + ) + .unwrap(); + w.flush().unwrap(); + // initialize → result + let mut init_line = String::new(); + reader.read_line(&mut init_line).unwrap(); + writeln!( + w, + "{}", + serde_json::json!({"jsonrpc": "2.0", "id": 1, "result": {"ok": true}}) + ) + .unwrap(); + w.flush().unwrap(); + // Read the tools/call, then DROP before responding: the request has + // crossed to the daemon, but no result comes back. + let mut call_line = String::new(); + reader.read_line(&mut call_line).unwrap(); + drop(w); + }); + + let client = ServiceClient { + socket_path: path.clone(), + }; + let error = client + .call_tool_structured_phased( + &Utf8PathBuf::from("/vaults/atlas"), + "vault.set", + serde_json::json!({}), + OnToolError::AcceptWithPayload, + ) + .expect_err("a dropped response must be Err"); + assert_eq!( + error.phase, + CallPhase::PostSend, + "a post-`tools/call` failure must be tagged PostSend (got {error})" + ); + server.join().unwrap(); + } + /// F4: the request path refuses to route through a socket it does not own. /// A normally-created socket (owned by us, not world-writable) passes; /// making it world-writable fails the trust gate.