fix(mcp): isolate socket integration tests from process environment

[QinXi-ai]

Closes #356.

The MCP socket integration and doctor-label tests mutate SYSKNIFE_SOCKET without the runner tests' ENV_LOCK. Ordinary cargo test therefore lets other tests change their daemon target between requests. Pass an explicit DaemonClient/socket to the shared implementation helpers and remove both MCP environment mutations. Production entry points still resolve their daemon target normally; tests continue to exercise real Unix-socket framing and the stale-approval rejection.

At 46ef3a9, all five single-threaded runs passed, while 14 of 40 ordinary parallel cargo test runs failed in the MCP integration test (271 tests per run): before. With this fix, the same five single-threaded and forty parallel runs all passed: after. These runs use cargo test, not nextest. Upstream Rust, docs/hygiene, frontend, and Postgres jobs also pass: CI.

I inspected the CLI's other set_var/remove_var callers: runner.rs also changes SYSKNIFE_SOCKET and SYSKNIFE_LISTEN_URI, under its existing ENV_LOCK. MCP no longer writes either variable. No tests were added or removed, and no suite-wide serialization or retry policy was introduced. Rustfmt and diff checks passed; local compilation on the Linux host was blocked by DNS resolution of its dependency mirror.

Implemented with Codex assistance.

[vladimirrott]

You found the actual mechanism, and the fix is the right one: inject the client instead of serialising the suite. #356 has been the most expensive open bug here because it makes every other pull request's board untrustworthy, and a retry policy or a global lock would have hidden it rather than removed it.

Head 835a9a16, one file, 50 insertions.

The proof, run here

I built the test binary offline in a container and ran it twenty times, then rebuilt the same tree with only mcp_server.rs reverted to its parent and ran that twenty times.

$ out="$(timeout 900 podman run --rm --network=none -v /tmp/r22p470:/repo:ro -v /tmp/flake470.sh:/flake.sh:ro -w /repo docker.io/library/rust:1-slim bash /flake.sh /repo/.container-target/debug/deps/sysknife-a88d48402304d890 20 2>&1)"; rc=$?; echo "FIXED-TREE loop rc=$rc"; printf '%s\n' "$out" | grep -v 'level=warning' | tail -8
FIXED-TREE loop rc=0
runs=20 failures=0
test result: ok. 271 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

$ out="$(timeout 900 podman run --rm --network=none -v /tmp/r22p470rev:/repo:ro -v /tmp/flake470.sh:/flake.sh:ro -w /repo docker.io/library/rust:1-slim bash /flake.sh /repo/.container-target/debug/deps/sysknife-a88d48402304d890 20 2>&1)"; rc=$?; echo "REVERTED-TREE loop rc=$rc"; printf '%s\n' "$out" | grep -v 'level=warning' | tail -20
REVERTED-TREE loop rc=0
run 20 FAILED:
test mcp_server::tests::mcp_tools_integrate_with_a_daemon_over_the_socket ... FAILED
runs=20 failures=20
test result: FAILED. 270 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

Twenty out of twenty on this host, where your fork saw 14 of 40. My container reports 12 CPUs and the scheduling comes out the same way every time, so the race resolves the same direction on every run. The single-threaded control separates the race from a plain bug:

$ out="$(timeout 600 podman run --rm --network=none -v /tmp/r22p470rev:/repo:ro -w /repo docker.io/library/rust:1-slim /repo/.container-target/debug/deps/sysknife-a88d48402304d890 --test-threads=1 2>&1)"; rc=$?; echo "REVERTED single-threaded rc=$rc"; printf '%s\n' "$out" | grep -E '^test result' | tail -1
REVERTED single-threaded rc=0
test result: ok. 271 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.17s

Same binary, same tree, 271 passing with one thread and one failing with twelve. That is the shape #356 described and nobody had pinned to a line.

The coupling I checked that the diff cannot show

mcp_server.rs losing its two set_var calls only helps if nothing else in the same test binary writes that variable unguarded. After your change, every remaining writer is in runner.rs, and all of them hold the one lock:

$ grep -rn 'set_var("SYSKNIFE_SOCKET"\|set_var("SYSKNIFE_LISTEN_URI"\|remove_var("SYSKNIFE_SOCKET"' apps/ crates/ --include=*.rs | head -20; echo "--- ENV_LOCK sites:"; grep -rn 'ENV_LOCK' apps/sysknife-cli/src/*.rs | head -8
apps/sysknife-cli/src/runner.rs:2603:        std::env::set_var("SYSKNIFE_SOCKET", "unix:///tmp/sysknife-web01.sock");
[16 more runner.rs lines trimmed here, all in the same module]
crates/sysknife-core/src/lib.rs:256:            std::env::set_var("SYSKNIFE_LISTEN_URI", "unix:///explicit.sock");
--- ENV_LOCK sites:
apps/sysknife-cli/src/runner.rs:2602:        let _guard = ENV_LOCK.lock().unwrap();
apps/sysknife-cli/src/runner.rs:3171:    static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

$ grep -rn 'static ENV_LOCK' /tmp/r22p470/apps/sysknife-cli/src/
/tmp/r22p470/apps/sysknife-cli/src/runner.rs:3171:    static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

One declaration, one mod tests in that file, every site under it. crates/sysknife-core/src/lib.rs:256 writes SYSKNIFE_LISTEN_URI and lives in a different test binary, so it cannot reach these.

Gates

cargo fmt --all --check is clean. Clippy on the crate you touched is clean:

$ timeout 1800 podman run --rm --network=none -v /tmp/r22p470:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true localhost/sk-rustgate:1 sh -c 'cargo clippy -p sysknife-cli --all-targets --locked --offline -- -D warnings >/tmp/c.log 2>&1; echo "CLIPPY(-p sysknife-cli) rc=$?"; tail -6 /tmp/c.log' 2>&1 | grep -v 'level=warning'
CLIPPY(-p sysknife-cli) rc=0
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 50.91s

The workspace-wide clippy that CI runs did not complete in my container: the Tauri shell app wants pkg-config and gtk headers that image does not carry, and it fails at build failed, waiting for other jobs to finish. That is my image, not your change, and CI runs the full one. I would rather say which command I could not finish than imply I ran it.

Production is untouched by design: direct_query_inner, execute_steps_inner, history_inner and doctor_inner still call resolve_socket_target() and pass the result down. The new *_with_client and doctor_at_socket functions each have a production caller, so nothing here is reachable only from tests.

Approving at 835a9a16. The CHANGELOG entry is mine at merge; a test-only fix normally gets none, and this one gets a line under Fixed anyway, because contributors have been re-running red boards over it for three weeks.

I am working through your other three. #469 is already past its gates on this host.