Fix #143: hermetic MCP stdio tests

[AlexMikhalev]

Fixes #143: test_tools_list and test_all_mcp_tools previously
depended on a sibling terraphim_settings/ repository path and a
host .terraphim/ ancestor directory. CI never had that sibling,
so the tests fell over trying to load a missing settings file or
unexpected project config.

This change makes the tests hermetic:

• Spawn the server with cwd set to a unique temp dir under
  std::env::temp_dir() so terraphim_config::project::discover()
  cannot walk up to a host .terraphim/. The server falls back to
  its embedded default config in that case.
• Drain stderr on a background thread (Arc<Mutex>) so the OS
  pipe buffer never fills and SIGPIPEs the server mid-test. The
  captured log is exposed for post-mortem diagnostics.
• Drop the leading -- separator before --verbose. clap
Args::parse() rejects --, which was the root cause of the
  BrokenPipe failures observed in earlier runs.
• Send the notifications/initialized frame between initialize
  and tools/list; the MCP server requires it before dispatching
  subsequent requests.
• Switch test_all_mcp_tools to lightweight tools (json_decode,
  find_files, grep_files) instead of build_autocomplete_index,
  autocomplete_terms, and search, each of which triggers
  ensure_thesaurus_loaded and walks the default KG path - that walk
  hangs in CI when the path is empty. The KG-backed tools already
  have dedicated coverage in the agent / cli test suites.
• Move shared helpers into tests/support/mod.rs and silence the
  per-binary dead-code warnings that arise because each integration
  test compiles its own copy of the module.

Local verification:

cargo test -p terraphim_mcp_server --test test_tools_list
cargo test -p terraphim_mcp_server --test test_all_mcp_tools

both report 1 passed; 0 failed after this change.

[AlexMikhalev]

Summary

test_tools_list and test_all_mcp_tools previously failed in CI for a layered set of reasons: they depended on a sibling terraphim_settings/ repo (which CI never had), the spawned server inherited the host's .terraphim/ and tried to load an unrelated project config, the clap parser rejected the leading -- separator before --verbose, and the MCP server requires a notifications/initialized frame before it will dispatch tools/list. test_all_mcp_tools additionally called KG-backed tools whose ensure_thesaurus_loaded walk hangs in CI because the default KG path is empty.

This PR rewrites both tests to be hermetic and deterministic:

• Spawns the server with cwd set to a unique temp dir under std::env::temp_dir(), so terraphim_config::project::discover() cannot walk up to a host .terraphim/.
• Drains stderr on a background thread into an Arc<Mutex<String>> so the OS pipe buffer cannot fill and SIGPIPE the server mid-test.
• Drops the leading -- from args(["--", "--verbose"]); the args are now ["--verbose"] only.
• Sends notifications/initialized between initialize and tools/list.
• Switches test_all_mcp_tools to the lightweight tools (json_decode, find_files, grep_files); the KG-backed tools have dedicated coverage in the agent and CLI crates.
• Moves shared helpers into a new tests/support/mod.rs and silences the per-binary dead_code warnings that arise because each integration-test binary compiles its own copy.

What was done well: the rationale is exhaustively documented at module scope, the test-specific failures are explained (not just papered over), and the changes leave no #[ignore], no mocks, and no timeout extensions. Both tests report 1 passed; 0 failed locally.

What remains problematic: the hermetic temp dirs created by create_hermetic_root() are never cleaned up (no Drop guard, no tempfile::TempDir); on a long-lived CI runner this leaves terraphim-mcp-server-hermetic-tests-<pid>-<ts>-<n> directories behind indefinitely. Also, the captured stderr_log is only surfaced on the BrokenPipe panic; if a later assertion fails (e.g., the tools/list JSON parse), the stderr is captured but not printed, hampering post-mortem debugging.

Confidence Score: 4/5

• Safe to merge with awareness of the two P2 items below (temp-dir leak and stderr visibility).
• The two findings are hygiene-grade and do not affect test correctness. They should be addressed shortly after merge.
• No files require special attention beyond tests/support/mod.rs and tests/test_tools_list.rs.

Important Files Changed

Filename	Overview
crates/terraphim_mcp_server/tests/support/mod.rs	New module providing create_hermetic_root() and mcp_server_binary(). Each integration-test binary compiles its own copy, hence the per-helper #[allow(dead_code)]. Minor: temp dir is not cleaned up.
crates/terraphim_mcp_server/tests/test_tools_list.rs	Rewritten to spawn the server in a hermetic cwd, drain stderr on a background thread, drop the leading --, and send notifications/initialized. Captured stderr is exposed only on BrokenPipe; should be surfaced on every failure.
crates/terraphim_mcp_server/tests/test_all_mcp_tools.rs	Rewritten to use the same hermetic pattern. Module doc explains the rationale for switching to json_decode / find_files / grep_files instead of the KG-backed tools. No issues found.

Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Test as cargo test (test_tools_list.rs)
    participant Cmd as std::process::Command
    participant Srv as terraphim_mcp_server (child)
    participant Drain as stderr drain thread

    Note over Test,Srv: cwd = /tmp/terraphim-mcp-server-hermetic-tests-<pid>-<ts>-<n>
    Test->>Cmd: spawn(binary, args=["--verbose"], cwd=root)
    Cmd->>Srv: fork + execve
    Srv-->>Drain: stderr pipe (lines drained into Arc<Mutex<String>>)
    Test->>Srv: write {"method":"initialize", "id":1}\n
    Srv-->>Test: {"result":{...}}
    Test->>Srv: write {"method":"notifications/initialized"}\n
    Note over Srv: now ready to dispatch subsequent requests
    Test->>Srv: write {"method":"tools/list", "id":2}\n
    Srv-->>Test: {"result":{"tools":[...]}}
    Test->>Srv: child.kill() / wait()
    Drain-->>Test: thread.join() (log captured but not printed)

Loading

Inline Findings

P2 crates/terraphim_mcp_server/tests/support/mod.rs, lines 26-39: Hermetic temp dirs leak across test runs

create_unique_test_root() calls fs::create_dir_all(&root) but never registers a Drop guard and never returns a guard struct. Each successful or failing test invocation leaves a terraphim-mcp-server-hermetic-tests-<pid>-<ts>-<n> directory under std::env::temp_dir(). On a long-lived CI runner this accumulates; over weeks it adds up to megabytes of empty directories.

Suggested fix: change the return type to a small guard struct whose Drop impl removes the directory, or use tempfile::TempDir::new() (already a dev-dep in neighbouring crates) and return its path wrapped in a guard. The test callers can read files from the guard while it is alive; once the test function returns, Drop cleans up.

pub struct HermeticRoot { path: PathBuf }
impl Drop for HermeticRoot {
    fn drop(&mut self) { let _ = fs::remove_dir_all(&self.path); }
}
impl HermeticRoot {
    pub fn path(&self) -> &Path { &self.path }
}

P2 crates/terraphim_mcp_server/tests/test_tools_list.rs, lines 41-58 and 90-105: Captured stderr is not surfaced on assertion failure

stderr_log is built up on a background thread but only printed in the BrokenPipe branch (panic!("broken pipe writing initialize request ({e}); server stderr:\n{log}")). If a later assertion fails -- e.g., serde_json::from_str(&response) fails because the server emitted an unexpected payload, or tools is unexpectedly empty -- the test panics with a Debug-formatted response string and the captured stderr is silently discarded.

Suggested fix: at minimum, log the captured stderr on expect failures (e.g., wrap the assertion sites in a helper that prints the log). A more thorough fix is to install a Drop guard that dumps the log on any panic by stashing the Arc into a thread-local or by relying on std::panic::catch_unwind and printing the log on unwind.

_{Last reviewed commit: bb3dbf9 | Reviews (1)}