From 7c4475e362e46a32b51b843d95704f2409fb8e93 Mon Sep 17 00:00:00 2001 From: Leo Date: Mon, 13 Jul 2026 12:30:15 -0400 Subject: [PATCH 01/13] fix(mcp): resolve daemon respawn binary deterministically and fail loud Bridge daemon-respawn resolved kkernel from ambient PATH, so a stale PATH binary produced a silent failing respawn loop under version skew. Resolve the binary from a deterministic source and surface respawn failure loudly instead of looping silently. Fixes #898 Co-Authored-By: Claude Fable 5 --- crates/khive-mcp/src/daemon.rs | 252 +++++++++++++++++++++++++-------- 1 file changed, 196 insertions(+), 56 deletions(-) diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index c57dcc98d..abae6e294 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -679,7 +679,7 @@ fn prepare_daemon_log_file(log_path: &std::path::Path) -> Option prepare_daemon_log_file_with_cap(log_path, DAEMON_LOG_MAX_BYTES) } -fn spawn_daemon() -> std::io::Result<()> { +fn spawn_daemon() -> std::io::Result { #[cfg(test)] SPAWN_COUNT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); @@ -709,8 +709,14 @@ fn spawn_daemon() -> std::io::Result<()> { use std::os::unix::process::CommandExt; cmd.process_group(0); } - cmd.spawn()?; - Ok(()) + // #898: return the live `Child` (rather than discarding it) so the caller + // can positively confirm the respawned process is still alive before + // treating recovery as healthy — see `RecoveryOutcome::Spawned` and its + // use in `forward_or_spawn`. A binary that predates or otherwise rejects + // `mcp --daemon` (version skew) exits immediately with a clap parse + // error; without this handle that failure was invisible to everything + // except `khived.log`. + cmd.spawn() } /// Return `true` if `args` (the full `ps -o args=` output for a process) @@ -977,8 +983,13 @@ enum RecoveryOutcome { /// via the normal path (no new spawn occurred). Skipped, /// This client killed the stale daemon and spawned a replacement; caller - /// must wait for readiness then forward the real request. - Spawned, + /// must wait for readiness then forward the real request. Carries the + /// spawned [`std::process::Child`] (#898) so `forward_or_spawn` can, once + /// it is otherwise about to give up and fall back locally, positively + /// confirm whether that specific respawn attempt already exited instead + /// of ever binding the socket — turning a version-skewed binary's silent, + /// forever-repeating respawn failure into a loud, caller-visible error. + Spawned(std::process::Child), /// Could not obtain a positive confirmation either way within the /// deadline-bound recovery window (the recoverer lock or the boot/recovery /// lock stayed contended past its deadline) — #838. The @@ -1262,7 +1273,7 @@ async fn kill_and_respawn(config_id: &str, namespace: &str) -> std::io::Result McpError { ) } +// ── #898: loud, unambiguous respawn-failure error ─────────────────────────── +// +// Root cause (2026-07-12 incident): `spawn_daemon` already resolves the spawn +// target deterministically via `std::env::current_exe()` (never ambient +// `PATH`), so a version-skewed binary reaching `mcp --daemon` means THIS +// process's own on-disk binary predates (or otherwise rejects) that flag — +// respawning via `current_exe()` faithfully relaunches the very same stale +// binary. That relaunch fails immediately with a clap parse error +// (`error: Unrecognized option: 'daemon'`) written only to `khived.log`. +// Because `spawn_daemon` was fire-and-forget (the `Child` was discarded) and +// `forward_or_spawn` cannot distinguish "our own respawn attempt definitely +// already died" from "no daemon is configured to run here at all", every +// request repeated the same failing respawn, burned the full connect +// deadline plus the boot-quiescence wait, and then quietly completed via +// local dispatch (or, in `KHIVE_DAEMON_STRICT=1`, rejected with the generic +// `no_socket` reason) — a silent, forever-repeating failure invisible to the +// caller and to every metric except a `khived.log` grep. +// +// The fix: `spawn_daemon` now returns the live `Child` (see +// `RecoveryOutcome::Spawned`), and `forward_or_spawn` checks — only at the +// point it would otherwise fall back locally, never earlier, so the existing +// connect-retry window and #667's boot-quiescence fence are unchanged — +// whether the respawn attempt IT made has already exited. A confirmed exit is +// unambiguous (this process spawned that exact child); it is never treated as +// the legitimate ADR-049 no-daemon case and never silently swallowed, in +// either strict or non-strict mode. + +/// Best-effort tail of `khived.log` for the loud respawn-failure diagnostic. +/// Returns `None` on any read failure or if the tail is empty — logging is +/// best-effort and must never turn a diagnostic path into a second failure. +fn tail_daemon_log(max_bytes: usize) -> Option { + let path = daemon_log_path()?; + let bytes = std::fs::read(&path).ok()?; + let start = bytes.len().saturating_sub(max_bytes); + let tail = String::from_utf8_lossy(&bytes[start..]).trim().to_string(); + if tail.is_empty() { + None + } else { + Some(tail) + } +} + +/// Build the loud, caller-visible error for a respawn attempt this process +/// made and can now positively confirm failed — `detail` names what was +/// observed (an exit status, or a `Command::spawn` error). Always logs at +/// `error!` and is always returned to the caller regardless of +/// `KHIVE_DAEMON_STRICT`: unlike the ordinary "no daemon reachable" fallback +/// (which may be the legitimate ADR-049 no-daemon deployment), a respawn WE +/// attempted and can prove failed is never a case for quietly completing the +/// request via local dispatch. See #898. +fn respawn_failed_error(detail: &str) -> McpError { + let exe = std::env::current_exe() + .map(|p| p.display().to_string()) + .unwrap_or_else(|_| "".to_string()); + let mut msg = format!( + "daemon respawn failed: {detail} — resolved daemon binary was \ + `{exe} mcp --daemon`; this is most likely binary version skew (an \ + on-disk kkernel predating or otherwise rejecting `mcp --daemon`); \ + rebuild with `make local`." + ); + if let Some(tail) = tail_daemon_log(2048) { + msg.push_str(&format!("\nkhived.log tail:\n{tail}")); + } + tracing::error!( + exe = %exe, + detail, + "daemon respawn failed loudly (#898): respawn attempt confirmed dead" + ); + McpError::internal_error( + msg, + Some(serde_json::json!({"reason": "respawn_failed"})), + ) +} + // ── bridge self-heal: re-exec in place on ProtocolMismatch (#714) ─────────── // // A long-lived stdio bridge process keeps running the OLD on-disk binary @@ -1804,22 +1889,29 @@ pub async fn forward_or_spawn(frame: &DaemonRequestFrame) -> Option = None; match kill_and_respawn(&frame.config_id, &frame.namespace).await { Err(e) => { - tracing::warn!(error = %e, "failed to spawn/recover the daemon; falling back to local dispatch"); - return fallback_or_reject( - FallbackReason::NoSocket, - &frame.config_id, - None, - &frame.namespace, - ); + // #898: `Command::spawn` itself failed to start the child at all — + // an unambiguous, already-fully-diagnosed respawn failure. Loud in + // both strict and non-strict mode; never a silent local fallback. + return Some(Err(respawn_failed_error(&format!( + "failed to start the replacement process: {e}" + )))); } Ok(RecoveryOutcome::Skipped) => { // A concurrent client already has a live matching daemon ready. } - Ok(RecoveryOutcome::Spawned) => { + Ok(RecoveryOutcome::Spawned(child)) => { // Give the kernel a moment to release the socket path and let the // spawned daemon process start. + spawned_child = Some(child); tokio::time::sleep(std::time::Duration::from_millis(50)).await; } Ok(RecoveryOutcome::Uncertain) => { @@ -1852,6 +1944,20 @@ pub async fn forward_or_spawn(frame: &DaemonRequestFrame) -> Option { + // #898: only now — after the full connect-retry window AND + // the #667 boot-quiescence wait, exactly as before — check + // whether the respawn THIS call made has already exited. + // A confirmed exit is unambiguous (this process spawned + // that exact child) and is never treated as the + // legitimate ADR-049 no-daemon case. + if let Some(child) = spawned_child.as_mut() { + if let Ok(Some(status)) = child.try_wait() { + return Some(Err(respawn_failed_error(&format!( + "the spawned process exited with {status} instead \ + of binding the daemon socket" + )))); + } + } return fallback_or_reject( FallbackReason::NoSocket, &frame.config_id, @@ -2575,15 +2681,19 @@ mod tests { clear_daemon_env(); } - // #947: genuine daemon-unreachable fallback (no `KHIVE_NO_DAEMON` opt-out) - // must still complete locally in non-strict mode, and must reject the - // request in strict mode. `spawn_daemon()` really runs here (`SPAWN_COUNT` - // bumps) but the spawned process is this same test binary re-invoked with - // unrecognized args, so it never binds the socket and the daemon - // genuinely never becomes reachable — the same way - // `forward_or_spawn_blocks_on_boot_quiescence_before_local_fallback` below - // forces this path without a fake daemon. Each run pays the ~5s forward - // deadline plus the boot-quiescence reprobe. + // #898: genuine daemon-unreachable fallback (no `KHIVE_NO_DAEMON` opt-out), + // where the respawn THIS call attempted can be positively confirmed dead. + // `spawn_daemon()` really runs here (`SPAWN_COUNT` bumps), spawning this + // same test binary re-invoked with unrecognized `mcp --daemon` args; it + // exits immediately without ever binding the socket — mirroring the + // 2026-07-12 incident's version-skewed binary (`error: Unrecognized + // option: 'daemon'`). This must surface as a loud, caller-visible error in + // BOTH strict and non-strict mode: unlike the ordinary "no daemon + // reachable, cause unknown" fallback, a respawn this process made and can + // prove failed is never eligible for a silent local-dispatch completion. + // Each run pays the ~5s forward deadline plus the boot-quiescence reprobe + // — see `forward_or_spawn_blocks_on_boot_quiescence_before_local_fallback` + // below, which asserts that wait is unaffected by this change. fn unreachable_daemon_frame(config_id: &str) -> DaemonRequestFrame { DaemonRequestFrame { @@ -2605,7 +2715,7 @@ mod tests { #[tokio::test] #[serial] - async fn forward_or_spawn_non_strict_falls_back_locally_when_daemon_unreachable() { + async fn forward_or_spawn_surfaces_loud_error_when_respawn_confirmed_dead_non_strict() { clear_daemon_env(); reset_fallback_counters(); let dir = tempfile::tempdir().expect("tempdir"); @@ -2618,12 +2728,29 @@ mod tests { let frame = unreachable_daemon_frame(CFG); let out = forward_or_spawn(&frame).await; - assert!( - out.is_none(), - "non-strict mode must still complete the request via local dispatch \ - when the daemon is genuinely unreachable, got {out:?}" - ); - assert_eq!(fallback_count(FallbackReason::NoSocket), 1); + match out { + Some(Err(McpError { message, .. })) => { + assert!( + message.contains("respawn failed"), + "must name the respawn failure specifically, not a generic \ + fallback: {message}" + ); + assert!( + message.contains("make local"), + "must point the operator at the fix: {message}" + ); + } + other => panic!( + "a respawn attempt confirmed dead must surface loudly even in \ + non-strict mode (#898) instead of completing the request via \ + silent local dispatch, got {other:?}" + ), + } + // #898's loud respawn-failure path bypasses the ordinary + // fallback/telemetry machinery entirely — a confirmed respawn failure + // is never the legitimate ADR-049 no-daemon case that telemetry + // exists to count. + assert_eq!(fallback_count(FallbackReason::NoSocket), 0); reset_fallback_counters(); clear_daemon_env(); @@ -2632,7 +2759,7 @@ mod tests { #[tokio::test] #[serial] - async fn forward_or_spawn_strict_mode_errors_when_daemon_unreachable() { + async fn forward_or_spawn_surfaces_loud_error_when_respawn_confirmed_dead_strict() { clear_daemon_env(); reset_fallback_counters(); let dir = tempfile::tempdir().expect("tempdir"); @@ -2648,23 +2775,19 @@ mod tests { match out { Some(Err(McpError { message, .. })) => { assert!( - message.contains("no_socket"), - "strict-mode error must name the fallback reason: {message}" - ); - assert!( - message.contains("KHIVE_DAEMON_STRICT"), - "strict-mode error should name the mode that rejected the \ - request: {message}" + message.contains("respawn failed"), + "strict mode must still surface the specific respawn-failure \ + diagnosis, not the generic no_socket reason: {message}" ); } other => panic!( - "KHIVE_DAEMON_STRICT=1 must reject the request instead of completing \ - it locally when the daemon is unreachable, got {other:?}" + "KHIVE_DAEMON_STRICT=1 must reject the request when the daemon is \ + unreachable, got {other:?}" ), } - // Counters/telemetry stay exactly as they were before #947 — this - // change only affects what the caller gets back. - assert_eq!(fallback_count(FallbackReason::NoSocket), 1); + // Strict mode changes nothing here: #898's loud path is unconditional + // and never reaches record_fallback/fallback_or_reject at all. + assert_eq!(fallback_count(FallbackReason::NoSocket), 0); reset_fallback_counters(); clear_daemon_env(); @@ -4330,7 +4453,7 @@ mod tests { ); let spawned_count = [&a, &b] .iter() - .filter(|r| matches!(r, Ok(RecoveryOutcome::Spawned))) + .filter(|r| matches!(r, Ok(RecoveryOutcome::Spawned(_)))) .count(); assert_eq!( spawned_count, 1, @@ -4427,7 +4550,7 @@ mod tests { // must attempt kill+spawn (Spawned outcome — spawn itself fails because there // is no real kkernel binary in test, but KILL_COUNT is checked BEFORE spawn). assert!( - matches!(outcome, Ok(RecoveryOutcome::Spawned) | Err(_)), + matches!(outcome, Ok(RecoveryOutcome::Spawned(_)) | Err(_)), "pre-probe same-protocol daemon must NOT be classified Alive; \ expected Spawned or spawn-error, got Skipped" ); @@ -5063,12 +5186,19 @@ mod tests { // Once the boot thread holds the guard (for `GUARD_HOLD`, deliberately // longer than `forward_or_spawn`'s fixed 5s readiness deadline), the fix // must block inside `wait_for_boot_quiescence_then_reprobe` until that - // guard is released before it is allowed to decide "genuinely no daemon" - // and return `None`. The elapsed-time assertion below is the - // fail-if-reverted oracle: reverting the fence makes `forward_or_spawn` - // return right at the 5s readiness deadline (measured from well before - // the boot thread even starts holding the guard), strictly before - // `GUARD_HOLD` has elapsed. + // guard is released before it is allowed to decide "genuinely no daemon". + // The elapsed-time assertion below is the fail-if-reverted oracle for + // #667: reverting that fence makes `forward_or_spawn` return right at the + // 5s readiness deadline (measured from well before the boot thread even + // starts holding the guard), strictly before `GUARD_HOLD` has elapsed. + // + // #898: by the time that wait finally ends, THIS call's own spawned child + // (the test binary, rejected `mcp --daemon` and exited within + // milliseconds) has long since exited — so the final outcome is now the + // loud, specific respawn-failure error rather than a silent `None`. That + // change is orthogonal to what this test actually guards: the fence must + // still be waited out in full regardless of which terminal outcome + // follows it, which the elapsed-time assertion below continues to prove. #[tokio::test] #[serial] async fn forward_or_spawn_blocks_on_boot_quiescence_before_local_fallback() { @@ -5131,11 +5261,21 @@ mod tests { .join() .expect("boot-holder thread must not panic"); - assert!( - result.is_none(), - "genuinely no daemon after boot quiescence must fall back to local \ - dispatch (None), got {result:?}" - ); + match &result { + Some(Err(McpError { message, .. })) => { + assert!( + message.contains("respawn failed"), + "#898: this call's own spawned child is confirmed dead by \ + now, so the outcome must be the specific loud respawn- \ + failure error, not a generic message: {message}" + ); + } + other => panic!( + "after boot quiescence, a respawn attempt confirmed dead must \ + surface loudly (#898) rather than falling back silently, got \ + {other:?}" + ), + } assert!( elapsed >= GUARD_HOLD, "forward_or_spawn must block until the cold-boot guard is released \ From 83753e395be3991118df5daa9e7320514099c95e Mon Sep 17 00:00:00 2001 From: Leo Date: Mon, 13 Jul 2026 17:48:29 -0400 Subject: [PATCH 02/13] fix(mcp): preserve strict respawn error envelope --- crates/khive-mcp/src/daemon.rs | 17 +++++++++++++---- crates/khive-mcp/src/server.rs | 18 ++++++++++-------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index 7cbf67be8..99bb3ddb0 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -1367,10 +1367,19 @@ fn respawn_failed_error(detail: &str) -> McpError { detail, "daemon respawn failed loudly (#898): respawn attempt confirmed dead" ); - McpError::internal_error( - msg, - Some(serde_json::json!({"reason": "respawn_failed"})), - ) + // Under strict mode this is also a pre-dispatch rejection, so preserve + // #947's request-envelope contract by tagging it for `server::request`. + // Non-strict callers still receive the raw, loud MCP error introduced by + // #898; in neither mode may the request fall through to local dispatch. + let data = if is_daemon_strict_mode() { + serde_json::json!({ + STRICT_FALLBACK_MARKER: true, + "reason": "respawn_failed", + }) + } else { + serde_json::json!({"reason": "respawn_failed"}) + }; + McpError::internal_error(msg, Some(data)) } // ── bridge self-heal: re-exec in place on ProtocolMismatch (#714) ─────────── diff --git a/crates/khive-mcp/src/server.rs b/crates/khive-mcp/src/server.rs index 999977a05..f94347cc5 100644 --- a/crates/khive-mcp/src/server.rs +++ b/crates/khive-mcp/src/server.rs @@ -1492,13 +1492,14 @@ result (e.g. create then link with the new entity's id)."#)] if let Some(res) = crate::daemon::forward_or_spawn(&frame).await { return match res { Ok(s) => Ok(s), - // #947: a strict-mode fallback rejection is tagged with + // #947/#898: a strict-mode pre-dispatch rejection is + // tagged with // `daemon::STRICT_FALLBACK_MARKER` so it can be reshaped // into the normal per-op envelope instead of surfacing as // an RPC-level error. Every other daemon-forward error - // (protocol mismatch, oversized frame, ambiguous - // post-write outcome) is untagged and passes through - // unchanged. + // (non-strict respawn failure, protocol mismatch, + // oversized frame, ambiguous post-write outcome) is + // untagged and passes through unchanged. Err(e) => match strict_fallback_reason(&e) { Some(reason) => strict_fallback_envelope_response(&p, reason), None => Err(e), @@ -2678,8 +2679,9 @@ mod tests { clear_daemon_env(); crate::daemon::reset_fallback_counters(); let dir = tempfile::tempdir().expect("tempdir"); - // Never bound by anything in this test — the daemon is genuinely - // unreachable, exactly like `daemon::forward_or_spawn_strict_mode_errors_when_daemon_unreachable`. + // Never bound by anything in this test. The spawned test harness exits + // immediately on `mcp --daemon`, so #898 classifies this as a confirmed + // respawn failure rather than the older generic `no_socket` fallback. std::env::set_var("KHIVE_SOCKET", dir.path().join("khived.sock")); std::env::set_var("KHIVE_PID", dir.path().join("khived.pid")); std::env::set_var("KHIVE_LOCK", dir.path().join("khived.recovery.lock")); @@ -2719,8 +2721,8 @@ mod tests { "error must name the strict mode that rejected the fallback: {msg}" ); assert!( - msg.contains("no_socket"), - "error must name the fallback reason: {msg}" + msg.contains("respawn_failed"), + "error must name the confirmed respawn failure: {msg}" ); } From ddfdbb1f3a48c2aae4305b82e67f8411c46191ab Mon Sep 17 00:00:00 2001 From: Leo Date: Mon, 13 Jul 2026 17:49:45 -0400 Subject: [PATCH 03/13] test(mcp): keep schedule routing regression hermetic --- crates/khive-mcp/src/serve.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/khive-mcp/src/serve.rs b/crates/khive-mcp/src/serve.rs index 6116fac2f..a9fd7a163 100644 --- a/crates/khive-mcp/src/serve.rs +++ b/crates/khive-mcp/src/serve.rs @@ -5048,11 +5048,21 @@ backend = "kg-backend" ); use clap::Parser; - let args = Args::parse_from(["mcp", "--config", config_path.to_str().expect("utf8 path")]); + let args = Args::parse_from([ + "mcp", + "--config", + config_path.to_str().expect("utf8 path"), + "--no-embed", + ]); let (server, schedule_rt) = build_server(&args).expect("build_server must succeed"); // No `[packs.schedule]` entry above, so it defaults to "main". let rt = schedule_rt.expect("schedule pack is loaded by default"); + assert!( + rt.config().embedding_model.is_none() + && rt.config().additional_embedding_models.is_empty(), + "the backend-routing fixture must remain independent of external embedding models" + ); let marker = "adr106-multi-backend-dispatch-marker"; let action_dsl = format!("create(kind=\"observation\", content=\"{marker}\")"); From 7871897f1dde987658144b9a30e6ed37b94dfe46 Mon Sep 17 00:00:00 2001 From: Leo Date: Mon, 13 Jul 2026 17:02:23 -0400 Subject: [PATCH 04/13] test(khive-db): serialize writer-task tests on tx_registry to fix flaky tx-age sweep run_writer_task registers a writer_task_tx handle in the process-wide tx_registry singleton; the checkpoint tx_age_sweep_* tests read that singleton under #[serial(tx_registry)]. The writer_task.rs tests that spawn a writer task were missing from the group, leaking a longer-lived writer_task_tx into the sweep's oldest() read and intermittently failing the assertion on main CI. Join them to the serial group. No prod change. --- crates/khive-db/src/writer_task.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/khive-db/src/writer_task.rs b/crates/khive-db/src/writer_task.rs index c13590065..17738a9a5 100644 --- a/crates/khive-db/src/writer_task.rs +++ b/crates/khive-db/src/writer_task.rs @@ -420,6 +420,7 @@ async fn run_writer_task( mod tests { use super::*; use crate::pool::PoolConfig; + use serial_test::serial; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use std::time::Duration; @@ -432,7 +433,14 @@ mod tests { ConnectionPool::new(cfg).expect("pool open") } + // `#[serial(tx_registry)]`: `run_writer_task` registers a `writer_task_tx` + // handle in the process-wide `tx_registry` singleton for the life of each + // `BEGIN IMMEDIATE`. Tests that observe the registry (the checkpoint + // `tx_age_sweep_*` group) read `tx_registry::oldest()`; an un-serialized + // spawning test here would leak a longer-lived `writer_task_tx` into that + // read and make the sweep name the wrong transaction. Share the key. #[tokio::test] + #[serial(tx_registry)] async fn begin_immediate_failure_replies_error_without_running_op() { // Real lock contention, not a simulation: hold the database-level // write lock from the pool's own writer connection (the unmigrated @@ -504,7 +512,11 @@ mod tests { ); } + // `#[serial(tx_registry)]`: shares the key with the checkpoint + // `tx_age_sweep_*` tests — see the note on + // `begin_immediate_failure_replies_error_without_running_op`. #[tokio::test] + #[serial(tx_registry)] async fn writer_task_executes_op_and_commits() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("writer_task_commit.db"); @@ -627,7 +639,13 @@ mod tests { first.abort(); } + // `#[serial(tx_registry)]`: this test deliberately keeps a request (and + // thus its `writer_task_tx` registry handle) alive past a timeout, so it is + // the worst polluter of the checkpoint `tx_age_sweep_*` reads if left + // un-serialized. Shares the key — see the note on + // `begin_immediate_failure_replies_error_without_running_op`. #[tokio::test] + #[serial(tx_registry)] async fn send_with_timeout_returns_op_result_when_op_outlives_the_timeout() { // `send_with_timeout`'s timeout must bound ONLY the enqueue step — // never the reply-wait. An accepted request (channel not full) must From 1e789bb75d19bdef651fe5f9b8ecd2b7cc8ee5d3 Mon Sep 17 00:00:00 2001 From: Leo Date: Mon, 13 Jul 2026 17:56:25 -0400 Subject: [PATCH 05/13] test(kkernel): make code-ingest vector test hermetic --- crates/kkernel/src/code_ingest.rs | 81 +++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 3 deletions(-) diff --git a/crates/kkernel/src/code_ingest.rs b/crates/kkernel/src/code_ingest.rs index 4d0bf5d5e..2d49613eb 100644 --- a/crates/kkernel/src/code_ingest.rs +++ b/crates/kkernel/src/code_ingest.rs @@ -104,6 +104,16 @@ pub async fn run_code_ingest(args: CodeIngestArgs) -> Result<()> { /// after validation has already succeeded and a real (non-dry-run) write is /// about to occur. async fn code_ingest_batch(args: CodeIngestArgs) -> Result { + code_ingest_batch_with_runtime_setup(args, |_| Ok(())).await +} + +async fn code_ingest_batch_with_runtime_setup( + args: CodeIngestArgs, + runtime_setup: F, +) -> Result +where + F: FnOnce(&KhiveRuntime) -> Result<()>, +{ let bytes = std::fs::read(&args.findings) .with_context(|| format!("failed to read {}", args.findings.display()))?; @@ -161,6 +171,7 @@ async fn code_ingest_batch(args: CodeIngestArgs) -> Result { } let runtime = KhiveRuntime::new(cfg).map_err(|e| anyhow::anyhow!("{e}"))?; + runtime_setup(&runtime)?; let resolved_ns = runtime.config().default_namespace.clone(); let token = runtime .authorize(resolved_ns) @@ -479,10 +490,63 @@ fn wal_sidecar_path(db_path: &Path) -> PathBuf { #[cfg(test)] mod tests { + use std::sync::Arc; + + use async_trait::async_trait; + use khive_runtime::{EmbedderProvider, RuntimeError}; + use lattice_embed::{EmbedError, EmbeddingModel, EmbeddingService}; use serial_test::serial; use super::*; + struct FixedEmbeddingService { + dimensions: usize, + } + + #[async_trait] + impl EmbeddingService for FixedEmbeddingService { + async fn embed( + &self, + texts: &[String], + _model: EmbeddingModel, + ) -> std::result::Result>, EmbedError> { + Ok(texts + .iter() + .map(|_| vec![1.0_f32; self.dimensions]) + .collect()) + } + + fn supports_model(&self, _model: EmbeddingModel) -> bool { + true + } + + fn name(&self) -> &'static str { + "code-ingest-test" + } + } + + struct FixedEmbeddingProvider { + name: String, + dimensions: usize, + } + + #[async_trait] + impl EmbedderProvider for FixedEmbeddingProvider { + fn name(&self) -> &str { + &self.name + } + + fn dimensions(&self) -> usize { + self.dimensions + } + + async fn build(&self) -> std::result::Result, RuntimeError> { + Ok(Arc::new(FixedEmbeddingService { + dimensions: self.dimensions, + })) + } + } + fn base_args(findings: PathBuf, db: PathBuf) -> CodeIngestArgs { CodeIngestArgs { findings, @@ -847,9 +911,20 @@ mod tests { let findings = write_valid_findings(tmp.path()); let db = tmp.path().join("scratch.db"); - code_ingest_batch(base_args(findings, db.clone())) - .await - .expect("ingest must succeed"); + code_ingest_batch_with_runtime_setup(base_args(findings, db.clone()), |runtime| { + let model_names = runtime.registered_embedding_model_names(); + assert!( + !model_names.is_empty(), + "test requires at least one configured embedding model" + ); + for name in model_names { + let dimensions = runtime.resolve_embedding_model(Some(&name))?.dimensions(); + runtime.register_embedder(FixedEmbeddingProvider { name, dimensions }); + } + Ok(()) + }) + .await + .expect("ingest must succeed"); let cfg = resolve_runtime_config(RuntimeConfigInputs { db: Some(db.to_str().expect("utf8 path")), From 370be1bd349f72d835fcc0e286779f9a913852ba Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 09:01:07 -0400 Subject: [PATCH 06/13] fix(mcp): stop exfiltrating daemon log and executable path in respawn errors (#963) --- crates/khive-mcp/src/daemon.rs | 95 ++++++++++++++++++++++++++++------ 1 file changed, 78 insertions(+), 17 deletions(-) diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index 99bb3ddb0..e5c434891 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -1326,7 +1326,7 @@ fn ambiguous_forward_error() -> McpError { // the legitimate ADR-049 no-daemon case and never silently swallowed, in // either strict or non-strict mode. -/// Best-effort tail of `khived.log` for the loud respawn-failure diagnostic. +/// Best-effort tail of `khived.log` for the local respawn-failure diagnostic. /// Returns `None` on any read failure or if the tail is empty — logging is /// best-effort and must never turn a diagnostic path into a second failure. fn tail_daemon_log(max_bytes: usize) -> Option { @@ -1341,10 +1341,11 @@ fn tail_daemon_log(max_bytes: usize) -> Option { } } -/// Build the loud, caller-visible error for a respawn attempt this process -/// made and can now positively confirm failed — `detail` names what was -/// observed (an exit status, or a `Command::spawn` error). Always logs at -/// `error!` and is always returned to the caller regardless of +/// Build the caller-visible error for a respawn attempt this process made and +/// can now positively confirm failed. The detailed observation, resolved +/// executable path, and shared daemon-log tail stay in local error-level +/// tracing; callers receive only a stable code and safe remediation. The error +/// is returned regardless of /// `KHIVE_DAEMON_STRICT`: unlike the ordinary "no daemon reachable" fallback /// (which may be the legitimate ADR-049 no-daemon deployment), a respawn WE /// attempted and can prove failed is never a case for quietly completing the @@ -1353,18 +1354,11 @@ fn respawn_failed_error(detail: &str) -> McpError { let exe = std::env::current_exe() .map(|p| p.display().to_string()) .unwrap_or_else(|_| "".to_string()); - let mut msg = format!( - "daemon respawn failed: {detail} — resolved daemon binary was \ - `{exe} mcp --daemon`; this is most likely binary version skew (an \ - on-disk kkernel predating or otherwise rejecting `mcp --daemon`); \ - rebuild with `make local`." - ); - if let Some(tail) = tail_daemon_log(2048) { - msg.push_str(&format!("\nkhived.log tail:\n{tail}")); - } + let log_tail = tail_daemon_log(2048); tracing::error!( exe = %exe, detail, + daemon_log_tail = log_tail.as_deref().unwrap_or(""), "daemon respawn failed loudly (#898): respawn attempt confirmed dead" ); // Under strict mode this is also a pre-dispatch rejection, so preserve @@ -1379,7 +1373,10 @@ fn respawn_failed_error(detail: &str) -> McpError { } else { serde_json::json!({"reason": "respawn_failed"}) }; - McpError::internal_error(msg, Some(data)) + McpError::internal_error( + "daemon respawn failed (respawn_failed); rebuild with `make local` and retry", + Some(data), + ) } // ── bridge self-heal: re-exec in place on ProtocolMismatch (#714) ─────────── @@ -2735,6 +2732,62 @@ mod tests { } } + struct RespawnDisclosureFixture { + original_home: Option, + _home: tempfile::TempDir, + sentinel: &'static str, + } + + impl RespawnDisclosureFixture { + fn new(sentinel: &'static str) -> Self { + let original_home = std::env::var_os("HOME"); + let home = tempfile::tempdir().expect("isolated HOME tempdir"); + std::env::set_var("HOME", home.path()); + let log_path = daemon_log_path().expect("HOME resolves daemon log path"); + std::fs::create_dir_all(log_path.parent().expect("daemon log has parent")) + .expect("create daemon log directory"); + std::fs::write(&log_path, format!("{sentinel}\n")).expect("seed daemon log sentinel"); + Self { + original_home, + _home: home, + sentinel, + } + } + + fn assert_caller_output_is_sanitized(&self, error: &McpError) { + let caller_output = serde_json::to_string(error).expect("serialize caller MCP error"); + let executable = std::env::current_exe() + .expect("resolve current test executable") + .display() + .to_string(); + assert!( + !caller_output.contains(self.sentinel), + "shared daemon log content must not reach the caller: {caller_output}" + ); + assert!( + !caller_output.contains(&executable), + "absolute daemon executable path must not reach the caller: {caller_output}" + ); + assert!( + caller_output.contains("respawn_failed"), + "caller must receive the stable respawn_failed code: {caller_output}" + ); + assert!( + caller_output.contains("make local"), + "caller must receive safe remediation text: {caller_output}" + ); + } + } + + impl Drop for RespawnDisclosureFixture { + fn drop(&mut self) { + match self.original_home.take() { + Some(home) => std::env::set_var("HOME", home), + None => std::env::remove_var("HOME"), + } + } + } + #[tokio::test] #[serial] async fn forward_or_spawn_surfaces_loud_error_when_respawn_confirmed_dead_non_strict() { @@ -2746,12 +2799,16 @@ mod tests { std::env::set_var("KHIVE_LOCK", dir.path().join("khived.recovery.lock")); std::env::remove_var("KHIVE_NO_DAEMON"); std::env::remove_var("KHIVE_DAEMON_STRICT"); + let disclosure = + RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_NON_STRICT_4d9813b72e"); let frame = unreachable_daemon_frame(CFG); let out = forward_or_spawn(&frame).await; match out { - Some(Err(McpError { message, .. })) => { + Some(Err(error)) => { + disclosure.assert_caller_output_is_sanitized(&error); + let message = &error.message; assert!( message.contains("respawn failed"), "must name the respawn failure specifically, not a generic \ @@ -2790,12 +2847,16 @@ mod tests { std::env::set_var("KHIVE_LOCK", dir.path().join("khived.recovery.lock")); std::env::remove_var("KHIVE_NO_DAEMON"); std::env::set_var("KHIVE_DAEMON_STRICT", "1"); + let disclosure = + RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_STRICT_7ac60d5391"); let frame = unreachable_daemon_frame(CFG); let out = forward_or_spawn(&frame).await; match out { - Some(Err(McpError { message, .. })) => { + Some(Err(error)) => { + disclosure.assert_caller_output_is_sanitized(&error); + let message = &error.message; assert!( message.contains("respawn failed"), "strict mode must still surface the specific respawn-failure \ From e3a86f16f8726d3a4e2262d1192dc4b0dd63d321 Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 10:08:02 -0400 Subject: [PATCH 07/13] fix(mcp): sanitize respawn failure tracing Keep confirmed failure events limited to stable classifications and numeric codes so bridge stderr cannot replay daemon logs or executable paths. --- crates/khive-mcp/src/daemon.rs | 169 ++++++++++++++++++++++----------- implementation_notes.md | 14 +++ 2 files changed, 128 insertions(+), 55 deletions(-) create mode 100644 implementation_notes.md diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index e5c434891..29e5e9031 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -1326,41 +1326,35 @@ fn ambiguous_forward_error() -> McpError { // the legitimate ADR-049 no-daemon case and never silently swallowed, in // either strict or non-strict mode. -/// Best-effort tail of `khived.log` for the local respawn-failure diagnostic. -/// Returns `None` on any read failure or if the tail is empty — logging is -/// best-effort and must never turn a diagnostic path into a second failure. -fn tail_daemon_log(max_bytes: usize) -> Option { - let path = daemon_log_path()?; - let bytes = std::fs::read(&path).ok()?; - let start = bytes.len().saturating_sub(max_bytes); - let tail = String::from_utf8_lossy(&bytes[start..]).trim().to_string(); - if tail.is_empty() { - None - } else { - Some(tail) - } +#[derive(Clone, Copy)] +enum RespawnFailure { + SpawnError { os_error_code: Option }, + ExitedBeforeBind { exit_code: Option }, } /// Build the caller-visible error for a respawn attempt this process made and -/// can now positively confirm failed. The detailed observation, resolved -/// executable path, and shared daemon-log tail stay in local error-level -/// tracing; callers receive only a stable code and safe remediation. The error -/// is returned regardless of +/// can now positively confirm failed. Both the caller error and bridge tracing +/// expose only stable classifications and non-sensitive numeric status codes. +/// The error is returned regardless of /// `KHIVE_DAEMON_STRICT`: unlike the ordinary "no daemon reachable" fallback /// (which may be the legitimate ADR-049 no-daemon deployment), a respawn WE /// attempted and can prove failed is never a case for quietly completing the /// request via local dispatch. See #898. -fn respawn_failed_error(detail: &str) -> McpError { - let exe = std::env::current_exe() - .map(|p| p.display().to_string()) - .unwrap_or_else(|_| "".to_string()); - let log_tail = tail_daemon_log(2048); - tracing::error!( - exe = %exe, - detail, - daemon_log_tail = log_tail.as_deref().unwrap_or(""), - "daemon respawn failed loudly (#898): respawn attempt confirmed dead" - ); +fn respawn_failed_error(failure: RespawnFailure) -> McpError { + match failure { + RespawnFailure::SpawnError { os_error_code } => tracing::error!( + reason = "respawn_failed", + failure_category = "spawn_error", + ?os_error_code, + "daemon respawn attempt confirmed failed" + ), + RespawnFailure::ExitedBeforeBind { exit_code } => tracing::error!( + reason = "respawn_failed", + failure_category = "exited_before_bind", + ?exit_code, + "daemon respawn attempt confirmed failed" + ), + } // Under strict mode this is also a pre-dispatch rejection, so preserve // #947's request-envelope contract by tagging it for `server::request`. // Non-strict callers still receive the raw, loud MCP error introduced by @@ -1909,9 +1903,9 @@ pub async fn forward_or_spawn(frame: &DaemonRequestFrame) -> Option { // A concurrent client already has a live matching daemon ready. @@ -1960,10 +1954,11 @@ pub async fn forward_or_spawn(frame: &DaemonRequestFrame) -> Option(value: Option<&str>, f: impl FnOnce() -> T) -> T { let prev = std::env::var("KHIVE_DAEMON_STRICT").ok(); @@ -2738,6 +2727,38 @@ mod tests { sentinel: &'static str, } + #[derive(Clone, Default)] + struct CapturedLog(std::sync::Arc>>); + + impl std::io::Write for CapturedLog { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0 + .lock() + .expect("captured log mutex poisoned") + .extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for CapturedLog { + type Writer = CapturedLog; + + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } + } + + impl CapturedLog { + fn contents(&self) -> String { + String::from_utf8(self.0.lock().expect("captured log mutex poisoned").clone()) + .expect("tracing output is UTF-8") + } + } + impl RespawnDisclosureFixture { fn new(sentinel: &'static str) -> Self { let original_home = std::env::var_os("HOME"); @@ -2754,20 +2775,24 @@ mod tests { } } - fn assert_caller_output_is_sanitized(&self, error: &McpError) { - let caller_output = serde_json::to_string(error).expect("serialize caller MCP error"); + fn assert_output_is_sanitized(&self, output_name: &str, output: &str) { let executable = std::env::current_exe() .expect("resolve current test executable") .display() .to_string(); assert!( - !caller_output.contains(self.sentinel), - "shared daemon log content must not reach the caller: {caller_output}" + !output.contains(self.sentinel), + "shared daemon log content must not reach {output_name}: {output}" ); assert!( - !caller_output.contains(&executable), - "absolute daemon executable path must not reach the caller: {caller_output}" + !output.contains(&executable), + "absolute daemon executable path must not reach {output_name}: {output}" ); + } + + fn assert_caller_output_is_sanitized(&self, error: &McpError) { + let caller_output = serde_json::to_string(error).expect("serialize caller MCP error"); + self.assert_output_is_sanitized("the caller", &caller_output); assert!( caller_output.contains("respawn_failed"), "caller must receive the stable respawn_failed code: {caller_output}" @@ -2779,6 +2804,22 @@ mod tests { } } + async fn forward_with_captured_events( + frame: &DaemonRequestFrame, + ) -> (Option>, String) { + let captured = CapturedLog::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(captured.clone()) + .with_ansi(false) + .without_time() + .finish(); + let subscriber_guard = tracing::subscriber::set_default(subscriber); + let output = forward_or_spawn(frame).await; + drop(subscriber_guard); + let events = captured.contents(); + (output, events) + } + impl Drop for RespawnDisclosureFixture { fn drop(&mut self) { match self.original_home.take() { @@ -2803,7 +2844,16 @@ mod tests { RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_NON_STRICT_4d9813b72e"); let frame = unreachable_daemon_frame(CFG); - let out = forward_or_spawn(&frame).await; + let (out, events) = forward_with_captured_events(&frame).await; + disclosure.assert_output_is_sanitized("bridge tracing events", &events); + assert!( + events.contains("reason=\"respawn_failed\""), + "trace must retain the stable reason code: {events}" + ); + assert!( + events.contains("failure_category=\"exited_before_bind\""), + "trace must classify the confirmed failure without raw detail: {events}" + ); match out { Some(Err(error)) => { @@ -2851,7 +2901,16 @@ mod tests { RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_STRICT_7ac60d5391"); let frame = unreachable_daemon_frame(CFG); - let out = forward_or_spawn(&frame).await; + let (out, events) = forward_with_captured_events(&frame).await; + disclosure.assert_output_is_sanitized("strict-mode bridge tracing events", &events); + assert!( + events.contains("reason=\"respawn_failed\""), + "strict-mode trace must retain the stable reason code: {events}" + ); + assert!( + events.contains("failure_category=\"exited_before_bind\""), + "strict-mode trace must classify the failure without raw detail: {events}" + ); match out { Some(Err(error)) => { diff --git a/implementation_notes.md b/implementation_notes.md new file mode 100644 index 000000000..f0e7cde82 --- /dev/null +++ b/implementation_notes.md @@ -0,0 +1,14 @@ +# PR #963 Round 4 implementation notes + +- Removed the confirmed-respawn-failure bridge trace's executable-path field, free-form detail, and `khived.log` tail read from `crates/khive-mcp/src/daemon.rs`. +- Replaced raw diagnostics with typed `spawn_error` / `exited_before_bind` categories, the stable `respawn_failed` reason, and optional numeric OS/exit codes. +- Extended strict and non-strict regression coverage to capture formatted tracing events as well as serialized `McpError` output, proving seeded daemon-log sentinels and the absolute executable path reach neither channel. +- Preserved ADR-049 Amendment 2 behavior: confirmed respawn failures still reject in both modes, with no change to fallback tiering or the strict-mode marker. + +Verification from `crates/`: + +- `cargo fmt --all` +- `cargo test -p khive-mcp` — 215 unit + 113 integration tests passed +- `cargo clippy -p khive-mcp --all-targets -- -D warnings` + +Domain utility: MEDIUM — the composed Rust observability guidance supported structured event capture; ADR-049 Amendment 2 and the review finding determined the security boundary and rejection semantics. From e826b2e10ded4b5018dd418c9f5c4dd04183d02a Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 10:16:48 -0400 Subject: [PATCH 08/13] docs(mcp): correct request-path comment to match respawn_failed contract (#963) --- crates/khive-mcp/src/server.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/khive-mcp/src/server.rs b/crates/khive-mcp/src/server.rs index b09941d58..67081cc95 100644 --- a/crates/khive-mcp/src/server.rs +++ b/crates/khive-mcp/src/server.rs @@ -1502,8 +1502,12 @@ several independent ops can run together; use chain when each op needs the prior result (e.g. create then link with the new entity's id)."#)] async fn request(&self, Parameters(p): Parameters) -> Result { // Forward to the warm daemon when reachable, auto-spawning it - // on first use. Any failure (no socket, spawn failure, namespace - // mismatch, KHIVE_NO_DAEMON) falls through to local dispatch. + // on first use. An ordinary no-socket condition, a namespace + // mismatch, or KHIVE_NO_DAEMON falls through to local dispatch. + // A confirmed respawn failure (spawn error, or the child exits + // before binding the socket) instead returns a caller-visible + // `respawn_failed` error without local dispatch, per ADR-049 + // Amendment 2. // // MCP-AUD-002: the daemon wire frame has no `save_to` field, so // daemon-forwarded requests silently drop the sink and return the From 49a3895b78c73ee7170e803b75f36580bb043e22 Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 10:38:38 -0400 Subject: [PATCH 09/13] chore: remove workflow scratch notes from tree Merge main and drop implementation_notes.md, a stray process-notes file that does not belong in the source tree. --- implementation_notes.md | 14 -------------- 1 file changed, 14 deletions(-) delete mode 100644 implementation_notes.md diff --git a/implementation_notes.md b/implementation_notes.md deleted file mode 100644 index f0e7cde82..000000000 --- a/implementation_notes.md +++ /dev/null @@ -1,14 +0,0 @@ -# PR #963 Round 4 implementation notes - -- Removed the confirmed-respawn-failure bridge trace's executable-path field, free-form detail, and `khived.log` tail read from `crates/khive-mcp/src/daemon.rs`. -- Replaced raw diagnostics with typed `spawn_error` / `exited_before_bind` categories, the stable `respawn_failed` reason, and optional numeric OS/exit codes. -- Extended strict and non-strict regression coverage to capture formatted tracing events as well as serialized `McpError` output, proving seeded daemon-log sentinels and the absolute executable path reach neither channel. -- Preserved ADR-049 Amendment 2 behavior: confirmed respawn failures still reject in both modes, with no change to fallback tiering or the strict-mode marker. - -Verification from `crates/`: - -- `cargo fmt --all` -- `cargo test -p khive-mcp` — 215 unit + 113 integration tests passed -- `cargo clippy -p khive-mcp --all-targets -- -D warnings` - -Domain utility: MEDIUM — the composed Rust observability guidance supported structured event capture; ADR-049 Amendment 2 and the review finding determined the security boundary and rejection semantics. From 411550d6754e6dfd5be7e9d63842acc3c2724fb0 Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 11:01:01 -0400 Subject: [PATCH 10/13] test(mcp): cover daemon spawn error sanitization --- crates/khive-mcp/src/daemon.rs | 95 ++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index 29e5e9031..1d8437d37 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -2727,6 +2727,42 @@ mod tests { sentinel: &'static str, } + #[cfg(unix)] + struct ExecutablePermissionsGuard { + path: std::path::PathBuf, + permissions: std::fs::Permissions, + } + + #[cfg(unix)] + impl ExecutablePermissionsGuard { + fn make_current_exe_unexecutable() -> Self { + use std::os::unix::fs::PermissionsExt; + + let path = std::env::current_exe().expect("resolve current test executable"); + let permissions = std::fs::metadata(&path) + .expect("read current test executable metadata") + .permissions(); + assert_ne!( + permissions.mode() & 0o111, + 0, + "current test executable must initially have an execute bit" + ); + let mut unexecutable = permissions.clone(); + unexecutable.set_mode(permissions.mode() & !0o111); + std::fs::set_permissions(&path, unexecutable) + .expect("make current test executable unexecutable"); + Self { path, permissions } + } + } + + #[cfg(unix)] + impl Drop for ExecutablePermissionsGuard { + fn drop(&mut self) { + std::fs::set_permissions(&self.path, self.permissions.clone()) + .expect("restore current test executable permissions"); + } + } + #[derive(Clone, Default)] struct CapturedLog(std::sync::Arc>>); @@ -2939,6 +2975,65 @@ mod tests { // ── daemon socket round-trip (env-mutating → serial) ───────────────────── + #[cfg(unix)] + #[tokio::test] + #[serial] + async fn forward_or_spawn_sanitizes_spawn_error_without_local_fallback() { + clear_daemon_env(); + reset_fallback_counters(); + let dir = tempfile::tempdir().expect("tempdir"); + std::env::set_var("KHIVE_SOCKET", dir.path().join("khived.sock")); + std::env::set_var("KHIVE_PID", dir.path().join("khived.pid")); + std::env::set_var("KHIVE_LOCK", dir.path().join("khived.recovery.lock")); + std::env::remove_var("KHIVE_NO_DAEMON"); + std::env::remove_var("KHIVE_DAEMON_STRICT"); + let disclosure = + RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_SPAWN_ERROR_f6a81b23c9"); + let _permissions = ExecutablePermissionsGuard::make_current_exe_unexecutable(); + + let frame = unreachable_daemon_frame(CFG); + let (out, events) = forward_with_captured_events(&frame).await; + disclosure.assert_output_is_sanitized("spawn-error bridge tracing events", &events); + assert!( + events.contains("reason=\"respawn_failed\""), + "trace must retain the stable reason code: {events}" + ); + assert!( + events.contains("failure_category=\"spawn_error\""), + "trace must classify the process-start failure without raw detail: {events}" + ); + assert!( + events.contains("os_error_code=Some(13)"), + "trace diagnostic must be the numeric permission-denied code only: {events}" + ); + assert!( + !events.contains("Permission denied") && !events.contains("os error"), + "trace must not expose the raw OS error text: {events}" + ); + + match out { + Some(Err(error)) => { + disclosure.assert_caller_output_is_sanitized(&error); + let caller_output = + serde_json::to_string(&error).expect("serialize caller MCP error"); + assert!( + !caller_output.contains("Permission denied") + && !caller_output.contains("os error"), + "caller must not receive the raw OS error text: {caller_output}" + ); + } + other => panic!( + "a confirmed process-start failure must return respawn_failed instead of \ + permitting local dispatch, got {other:?}" + ), + } + assert_eq!(fallback_total(), 0); + + reset_fallback_counters(); + clear_daemon_env(); + std::env::remove_var("KHIVE_LOCK"); + } + #[tokio::test] #[serial] async fn daemon_round_trip_dispatches_and_enforces_config_id() { From 61bf28dcd7a81204923737cc55ef7eae8b018785 Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 11:37:13 -0400 Subject: [PATCH 11/13] fix(mcp): include respawn remediation in strict errors --- crates/khive-mcp/src/server.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/khive-mcp/src/server.rs b/crates/khive-mcp/src/server.rs index 67081cc95..62d4d0880 100644 --- a/crates/khive-mcp/src/server.rs +++ b/crates/khive-mcp/src/server.rs @@ -1572,7 +1572,8 @@ fn strict_fallback_envelope_response( let total = parsed.ops.len(); let error_msg = format!( "daemon fallback rejected under KHIVE_DAEMON_STRICT=1: reason={reason}; \ - refusing to complete the request via local dispatch" + refusing to complete the request via local dispatch; \ + rebuild with `make local` and retry" ); let results: Vec = match parsed.mode { @@ -2761,6 +2762,10 @@ mod tests { msg.contains("respawn_failed"), "error must name the confirmed respawn failure: {msg}" ); + assert!( + msg.contains("make local"), + "error must include the safe respawn remediation: {msg}" + ); } // ── single op ────────────────────────────────────────────────────── From eab0644a4c26d63a6a8cb6ef4944fdd0d64a42b6 Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 12:35:31 -0400 Subject: [PATCH 12/13] test(mcp): cover respawn error branches for coverage ratchet --- crates/khive-mcp/src/daemon.rs | 91 ++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index 1d8437d37..eeb84c75f 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -2795,6 +2795,18 @@ mod tests { } } + #[test] + fn captured_log_flush_is_a_noop() { + use std::io::Write; + + let mut captured = CapturedLog::default(); + captured + .write_all(b"respawn-event") + .expect("write captured event"); + captured.flush().expect("flush captured event"); + assert_eq!(captured.contents(), "respawn-event"); + } + impl RespawnDisclosureFixture { fn new(sentinel: &'static str) -> Self { let original_home = std::env::var_os("HOME"); @@ -2865,6 +2877,20 @@ mod tests { } } + #[test] + #[serial] + fn respawn_disclosure_fixture_restores_absent_home() { + let home = std::env::var_os("HOME").expect("test process has HOME"); + std::env::remove_var("HOME"); + { + let _fixture = + RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_ABSENT_HOME_62cc1aeb13"); + assert!(std::env::var_os("HOME").is_some()); + } + assert!(std::env::var_os("HOME").is_none()); + std::env::set_var("HOME", home); + } + #[tokio::test] #[serial] async fn forward_or_spawn_surfaces_loud_error_when_respawn_confirmed_dead_non_strict() { @@ -2894,6 +2920,9 @@ mod tests { match out { Some(Err(error)) => { disclosure.assert_caller_output_is_sanitized(&error); + let data = error.data.as_ref().expect("respawn error data"); + assert_eq!(data["reason"], "respawn_failed"); + assert!(data.get(STRICT_FALLBACK_MARKER).is_none()); let message = &error.message; assert!( message.contains("respawn failed"), @@ -2951,6 +2980,9 @@ mod tests { match out { Some(Err(error)) => { disclosure.assert_caller_output_is_sanitized(&error); + let data = error.data.as_ref().expect("strict respawn error data"); + assert_eq!(data["reason"], "respawn_failed"); + assert_eq!(data[STRICT_FALLBACK_MARKER], true); let message = &error.message; assert!( message.contains("respawn failed"), @@ -3034,6 +3066,65 @@ mod tests { std::env::remove_var("KHIVE_LOCK"); } + #[cfg(unix)] + struct SleepingExecutableGuard { + path: std::path::PathBuf, + backup: std::path::PathBuf, + } + + #[cfg(unix)] + impl SleepingExecutableGuard { + fn replace_current_exe() -> Self { + use std::os::unix::fs::PermissionsExt; + + let path = std::env::current_exe().expect("resolve current test executable"); + let backup = + path.with_extension(format!("khive-respawn-backup-{}", std::process::id())); + std::fs::rename(&path, &backup).expect("back up current test executable"); + std::fs::write(&path, b"#!/bin/sh\nsleep 10\n") + .expect("install sleeping executable wrapper"); + let mut permissions = std::fs::metadata(&path) + .expect("read sleeping wrapper metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&path, permissions).expect("make sleeping wrapper executable"); + Self { path, backup } + } + } + + #[cfg(unix)] + impl Drop for SleepingExecutableGuard { + fn drop(&mut self) { + std::fs::remove_file(&self.path).expect("remove sleeping executable wrapper"); + std::fs::rename(&self.backup, &self.path).expect("restore current test executable"); + } + } + + #[cfg(unix)] + #[tokio::test] + #[serial] + async fn forward_or_spawn_falls_back_when_spawned_child_is_still_alive() { + clear_daemon_env(); + reset_fallback_counters(); + let dir = tempfile::tempdir().expect("tempdir"); + std::env::set_var("KHIVE_SOCKET", dir.path().join("khived.sock")); + std::env::set_var("KHIVE_PID", dir.path().join("khived.pid")); + std::env::set_var("KHIVE_LOCK", dir.path().join("khived.recovery.lock")); + std::env::remove_var("KHIVE_NO_DAEMON"); + std::env::remove_var("KHIVE_DAEMON_STRICT"); + let _executable = SleepingExecutableGuard::replace_current_exe(); + + let out = forward_or_spawn(&unreachable_daemon_frame(CFG)).await; + + assert!( + out.is_none(), + "a live spawned child with no socket remains eligible for non-strict local fallback: {out:?}" + ); + assert_eq!(fallback_count(FallbackReason::NoSocket), 1); + clear_daemon_env(); + std::env::remove_var("KHIVE_LOCK"); + } + #[tokio::test] #[serial] async fn daemon_round_trip_dispatches_and_enforces_config_id() { From a2b9d53af10055199b33f42063b66b312249591e Mon Sep 17 00:00:00 2001 From: Leo Date: Tue, 14 Jul 2026 13:08:53 -0400 Subject: [PATCH 13/13] test(mcp): inject daemon exe via seam for respawn coverage --- crates/khive-mcp/src/daemon.rs | 154 +++++++++++++++------------------ 1 file changed, 68 insertions(+), 86 deletions(-) diff --git a/crates/khive-mcp/src/daemon.rs b/crates/khive-mcp/src/daemon.rs index eeb84c75f..0f2a46b9c 100644 --- a/crates/khive-mcp/src/daemon.rs +++ b/crates/khive-mcp/src/daemon.rs @@ -681,10 +681,14 @@ fn prepare_daemon_log_file(log_path: &std::path::Path) -> Option } fn spawn_daemon() -> std::io::Result { + let exe = std::env::current_exe()?; + spawn_daemon_with_exe(&exe) +} + +fn spawn_daemon_with_exe(exe: &std::path::Path) -> std::io::Result { #[cfg(test)] SPAWN_COUNT.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - let exe = std::env::current_exe()?; // The binary is `kkernel`; the MCP server (and its daemon mode) live under // the `mcp` subcommand. let mut cmd = std::process::Command::new(exe); @@ -1182,7 +1186,14 @@ const RECOVERER_LOCK_TIMEOUT_MS: u64 = 8_000; /// never silently conflated with a positive "confirmed alive" result. /// `Dead` (confirmed, recoverer lock held) → kill+spawn; /// `RecoveryOutcome::Spawned`. -async fn kill_and_respawn(config_id: &str, namespace: &str) -> std::io::Result { +async fn kill_and_respawn( + config_id: &str, + namespace: &str, + spawn: &F, +) -> std::io::Result +where + F: Fn() -> std::io::Result + Sync, +{ let initial_probe = { let _lock = acquire_recovery_lock(); probe_daemon_identity(config_id, namespace, 500).await @@ -1275,7 +1286,7 @@ async fn kill_and_respawn(config_id: &str, namespace: &str) -> std::io::Result Bo /// (`ambiguous_forward_error`) instead of killing/respawning/retrying or /// falling back locally. See #644. pub async fn forward_or_spawn(frame: &DaemonRequestFrame) -> Option> { + forward_or_spawn_with(frame, &spawn_daemon).await +} + +#[cfg(test)] +async fn forward_or_spawn_with_exe( + frame: &DaemonRequestFrame, + exe: &std::path::Path, +) -> Option> { + let spawn = || spawn_daemon_with_exe(exe); + forward_or_spawn_with(frame, &spawn).await +} + +async fn forward_or_spawn_with( + frame: &DaemonRequestFrame, + spawn: &F, +) -> Option> +where + F: Fn() -> std::io::Result + Sync, +{ if env_truthy("KHIVE_NO_DAEMON") { return None; } @@ -1898,7 +1928,7 @@ pub async fn forward_or_spawn(frame: &DaemonRequestFrame) -> Option = None; - match kill_and_respawn(&frame.config_id, &frame.namespace).await { + match kill_and_respawn(&frame.config_id, &frame.namespace, spawn).await { Err(e) => { // #898: `Command::spawn` itself failed to start the child at all — // an unambiguous, already-fully-diagnosed respawn failure. Loud in @@ -2727,40 +2757,22 @@ mod tests { sentinel: &'static str, } - #[cfg(unix)] - struct ExecutablePermissionsGuard { - path: std::path::PathBuf, - permissions: std::fs::Permissions, - } + fn daemon_script_fixture( + dir: &tempfile::TempDir, + name: &str, + body: &str, + ) -> std::path::PathBuf { + use std::os::unix::fs::PermissionsExt; - #[cfg(unix)] - impl ExecutablePermissionsGuard { - fn make_current_exe_unexecutable() -> Self { - use std::os::unix::fs::PermissionsExt; - - let path = std::env::current_exe().expect("resolve current test executable"); - let permissions = std::fs::metadata(&path) - .expect("read current test executable metadata") - .permissions(); - assert_ne!( - permissions.mode() & 0o111, - 0, - "current test executable must initially have an execute bit" - ); - let mut unexecutable = permissions.clone(); - unexecutable.set_mode(permissions.mode() & !0o111); - std::fs::set_permissions(&path, unexecutable) - .expect("make current test executable unexecutable"); - Self { path, permissions } - } - } - - #[cfg(unix)] - impl Drop for ExecutablePermissionsGuard { - fn drop(&mut self) { - std::fs::set_permissions(&self.path, self.permissions.clone()) - .expect("restore current test executable permissions"); - } + let path = dir.path().join(name); + std::fs::write(&path, body).expect("write daemon executable fixture"); + let mut permissions = std::fs::metadata(&path) + .expect("read daemon executable fixture metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&path, permissions) + .expect("make daemon executable fixture executable"); + path } #[derive(Clone, Default)] @@ -2852,8 +2864,9 @@ mod tests { } } - async fn forward_with_captured_events( + async fn forward_with_exe_and_captured_events( frame: &DaemonRequestFrame, + exe: &std::path::Path, ) -> (Option>, String) { let captured = CapturedLog::default(); let subscriber = tracing_subscriber::fmt() @@ -2862,7 +2875,7 @@ mod tests { .without_time() .finish(); let subscriber_guard = tracing::subscriber::set_default(subscriber); - let output = forward_or_spawn(frame).await; + let output = forward_or_spawn_with_exe(frame, exe).await; drop(subscriber_guard); let events = captured.contents(); (output, events) @@ -2904,9 +2917,10 @@ mod tests { std::env::remove_var("KHIVE_DAEMON_STRICT"); let disclosure = RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_NON_STRICT_4d9813b72e"); + let exe = daemon_script_fixture(&dir, "exits-before-bind", "#!/bin/sh\nexit 23\n"); let frame = unreachable_daemon_frame(CFG); - let (out, events) = forward_with_captured_events(&frame).await; + let (out, events) = forward_with_exe_and_captured_events(&frame, &exe).await; disclosure.assert_output_is_sanitized("bridge tracing events", &events); assert!( events.contains("reason=\"respawn_failed\""), @@ -2964,9 +2978,10 @@ mod tests { std::env::set_var("KHIVE_DAEMON_STRICT", "1"); let disclosure = RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_STRICT_7ac60d5391"); + let exe = daemon_script_fixture(&dir, "exits-before-bind", "#!/bin/sh\nexit 23\n"); let frame = unreachable_daemon_frame(CFG); - let (out, events) = forward_with_captured_events(&frame).await; + let (out, events) = forward_with_exe_and_captured_events(&frame, &exe).await; disclosure.assert_output_is_sanitized("strict-mode bridge tracing events", &events); assert!( events.contains("reason=\"respawn_failed\""), @@ -3010,7 +3025,7 @@ mod tests { #[cfg(unix)] #[tokio::test] #[serial] - async fn forward_or_spawn_sanitizes_spawn_error_without_local_fallback() { + async fn forward_or_spawn_with_injected_exe_sanitizes_spawn_error_without_local_fallback() { clear_daemon_env(); reset_fallback_counters(); let dir = tempfile::tempdir().expect("tempdir"); @@ -3021,10 +3036,11 @@ mod tests { std::env::remove_var("KHIVE_DAEMON_STRICT"); let disclosure = RespawnDisclosureFixture::new("KHIVE_RESPAWN_LOG_SENTINEL_SPAWN_ERROR_f6a81b23c9"); - let _permissions = ExecutablePermissionsGuard::make_current_exe_unexecutable(); + let exe = dir.path().join("not-executable"); + std::fs::write(&exe, "not an executable").expect("write non-executable fixture"); let frame = unreachable_daemon_frame(CFG); - let (out, events) = forward_with_captured_events(&frame).await; + let (out, events) = forward_with_exe_and_captured_events(&frame, &exe).await; disclosure.assert_output_is_sanitized("spawn-error bridge tracing events", &events); assert!( events.contains("reason=\"respawn_failed\""), @@ -3066,44 +3082,10 @@ mod tests { std::env::remove_var("KHIVE_LOCK"); } - #[cfg(unix)] - struct SleepingExecutableGuard { - path: std::path::PathBuf, - backup: std::path::PathBuf, - } - - #[cfg(unix)] - impl SleepingExecutableGuard { - fn replace_current_exe() -> Self { - use std::os::unix::fs::PermissionsExt; - - let path = std::env::current_exe().expect("resolve current test executable"); - let backup = - path.with_extension(format!("khive-respawn-backup-{}", std::process::id())); - std::fs::rename(&path, &backup).expect("back up current test executable"); - std::fs::write(&path, b"#!/bin/sh\nsleep 10\n") - .expect("install sleeping executable wrapper"); - let mut permissions = std::fs::metadata(&path) - .expect("read sleeping wrapper metadata") - .permissions(); - permissions.set_mode(0o755); - std::fs::set_permissions(&path, permissions).expect("make sleeping wrapper executable"); - Self { path, backup } - } - } - - #[cfg(unix)] - impl Drop for SleepingExecutableGuard { - fn drop(&mut self) { - std::fs::remove_file(&self.path).expect("remove sleeping executable wrapper"); - std::fs::rename(&self.backup, &self.path).expect("restore current test executable"); - } - } - #[cfg(unix)] #[tokio::test] #[serial] - async fn forward_or_spawn_falls_back_when_spawned_child_is_still_alive() { + async fn forward_or_spawn_with_injected_exe_falls_back_when_child_stays_alive() { clear_daemon_env(); reset_fallback_counters(); let dir = tempfile::tempdir().expect("tempdir"); @@ -3112,9 +3094,9 @@ mod tests { std::env::set_var("KHIVE_LOCK", dir.path().join("khived.recovery.lock")); std::env::remove_var("KHIVE_NO_DAEMON"); std::env::remove_var("KHIVE_DAEMON_STRICT"); - let _executable = SleepingExecutableGuard::replace_current_exe(); + let exe = daemon_script_fixture(&dir, "still-running", "#!/bin/sh\nsleep 10\n"); - let out = forward_or_spawn(&unreachable_daemon_frame(CFG)).await; + let out = forward_or_spawn_with_exe(&unreachable_daemon_frame(CFG), &exe).await; assert!( out.is_none(), @@ -4065,7 +4047,7 @@ mod tests { // whose turn arrives after the first recoverer already replaced the stale // daemon. The bounded probe confirms the live daemon; Skipped is returned // without killing. - let outcome = kill_and_respawn(&config_id, "test").await; + let outcome = kill_and_respawn(&config_id, "test", &spawn_daemon).await; assert!( matches!(outcome, Ok(RecoveryOutcome::Skipped)), @@ -4413,7 +4395,7 @@ mod tests { // Simulate the exactly-once scenario: // (a) kill_and_respawn sees a live daemon → returns Skipped (0 dispatches) // (b) call site forwards the real request once → 1 dispatch - let recovery = kill_and_respawn(config_id, "test").await; + let recovery = kill_and_respawn(config_id, "test", &spawn_daemon).await; assert!( matches!(recovery, Ok(RecoveryOutcome::Skipped)), "probe must find the live CountingDispatch daemon and return Skipped" @@ -4792,8 +4774,8 @@ mod tests { }); let (a, b) = tokio::join!( - kill_and_respawn(&config_id, "test"), - kill_and_respawn(&config_id, "test"), + kill_and_respawn(&config_id, "test", &spawn_daemon), + kill_and_respawn(&config_id, "test", &spawn_daemon), ); let spawned_count = [&a, &b] .iter() @@ -4886,7 +4868,7 @@ mod tests { FORCE_PID_IS_DAEMON.store(true, std::sync::atomic::Ordering::SeqCst); reset_counters(); - let outcome = kill_and_respawn(config_id, "test").await; + let outcome = kill_and_respawn(config_id, "test", &spawn_daemon).await; // The fake socket served exactly one response; join it before asserting. let _ = tokio::time::timeout(std::time::Duration::from_secs(2), fake_handle).await;