openhuman/tests/mcp_registry_multi_server.rs

//! Regression tests for multi-MCP-server support (#3196).
//!
//! These verify the three risks the issue calls out:
//! 1. Two servers exposing identically-named tools both reach the agent
//!    surface without collision (namespaced tool_id keys via server_id).
//! 2. One server failing to connect does not block another from running.
//! 3. Tool listing aggregates across all connected servers and tolerates
//!    a missing one.
//!
//! All tests are hermetic — they use the `test-mcp-stub` binary built
//! by Cargo (exposed via `CARGO_BIN_EXE_test-mcp-stub`) and a fresh
//! temp-dir workspace so they do not share SQLite state with each other
//! or with `mcp_registry_e2e.rs`.

use openhuman_core::openhuman::config::Config;
use openhuman_core::openhuman::mcp_registry::types::{CommandKind, InstalledServer, Transport};
use openhuman_core::openhuman::mcp_registry::{connections, ops, store};

// ── helpers ──────────────────────────────────────────────────────────────────

fn fresh_workspace_config() -> (tempfile::TempDir, Config) {
    let tmp = tempfile::tempdir().expect("tempdir");
    let mut cfg = Config::default();
    cfg.workspace_dir = tmp.path().to_path_buf();
    (tmp, cfg)
}

fn make_stub_server(qualified: &str) -> InstalledServer {
    let stub = env!("CARGO_BIN_EXE_test-mcp-stub");
    InstalledServer {
        server_id: format!("multi-{}", uuid::Uuid::new_v4()),
        qualified_name: qualified.to_string(),
        display_name: qualified.to_string(),
        description: None,
        icon_url: None,
        command_kind: CommandKind::Binary,
        command: stub.to_string(),
        args: Vec::new(),
        env_keys: Vec::new(),
        config: None,
        installed_at: 0,
        last_connected_at: None,
        transport: Transport::Stdio,
        enabled: true,
    }
}

// ── extract response text (mirrors mcp_registry_e2e.rs convention) ───────────

fn extract_echo_text(result: &serde_json::Value) -> &str {
    result
        .get("content")
        .and_then(|c| c.as_array())
        .and_then(|arr| arr.first())
        .and_then(|first| first.get("text"))
        .and_then(|t| t.as_str())
        .unwrap_or("")
}

// ── Test 1: name collision ────────────────────────────────────────────────────

/// Two stub servers both expose the same tool name (`echo`). After connecting
/// both, `all_connected_tools` returns two entries — one per server — and both
/// respond correctly when called by `server_id`, proving the `server_id` is
/// the disambiguator for identically-named tools.
#[tokio::test]
async fn two_servers_same_tool_name_no_collision() {
    let (_tmp, cfg) = fresh_workspace_config();

    let server_a = make_stub_server("@multi-test/echo-a");
    let server_b = make_stub_server("@multi-test/echo-b");

    // Both server_ids must be distinct (sanity guard for the disambiguation
    // assertions below).
    assert_ne!(
        server_a.server_id, server_b.server_id,
        "two stub servers must have distinct server_ids"
    );

    store::insert_server(&cfg, &server_a).expect("insert server_a");
    store::insert_server(&cfg, &server_b).expect("insert server_b");

    // Connect both — each spawns its own subprocess.
    let tools_a = connections::connect(&cfg, &server_a)
        .await
        .expect("connect server_a");
    let tools_b = connections::connect(&cfg, &server_b)
        .await
        .expect("connect server_b");

    assert_eq!(tools_a.len(), 1, "server_a: stub advertises one tool");
    assert_eq!(tools_b.len(), 1, "server_b: stub advertises one tool");
    assert_eq!(tools_a[0].name, "echo", "server_a tool name is `echo`");
    assert_eq!(tools_b[0].name, "echo", "server_b tool name is `echo`");

    // Both servers are in the connected aggregate.
    let all_tools = connections::all_connected_tools().await;
    let a_tools: Vec<_> = all_tools
        .iter()
        .filter(|(sid, _, _)| sid == &server_a.server_id)
        .collect();
    let b_tools: Vec<_> = all_tools
        .iter()
        .filter(|(sid, _, _)| sid == &server_b.server_id)
        .collect();

    assert_eq!(
        a_tools.len(),
        1,
        "server_a contributes exactly one tool to the aggregate"
    );
    assert_eq!(
        b_tools.len(),
        1,
        "server_b contributes exactly one tool to the aggregate"
    );
    assert_eq!(
        a_tools[0].2.name, "echo",
        "server_a's aggregated tool is `echo`"
    );
    assert_eq!(
        b_tools[0].2.name, "echo",
        "server_b's aggregated tool is `echo`"
    );

    // Both are `connected` in all_status.
    let statuses = connections::all_status(&cfg).await;
    let find = |id: &str| {
        statuses
            .iter()
            .find(|s| s.server_id == id)
            .cloned()
            .unwrap_or_else(|| panic!("status entry missing for {id}"))
    };
    assert_eq!(find(&server_a.server_id).status.as_str(), "connected");
    assert_eq!(find(&server_a.server_id).tool_count, 1);
    assert_eq!(find(&server_b.server_id).status.as_str(), "connected");
    assert_eq!(find(&server_b.server_id).tool_count, 1);

    let _ = connections::disconnect(&server_a.server_id).await;
    let _ = connections::disconnect(&server_b.server_id).await;
}

// ── Test 2: routing ───────────────────────────────────────────────────────────

/// `call_tool` routes by `server_id` even with a shared name — each server
/// subprocess receives and echoes back the exact payload sent to that server_id.
/// Demonstrates the agent → core call path works across multiple servers without
/// cross-wiring.
#[tokio::test]
async fn tool_calls_route_to_the_correct_server() {
    let (_tmp, cfg) = fresh_workspace_config();

    let server_a = make_stub_server("@route-test/echo-a");
    let server_b = make_stub_server("@route-test/echo-b");

    store::insert_server(&cfg, &server_a).expect("insert server_a");
    store::insert_server(&cfg, &server_b).expect("insert server_b");

    connections::connect(&cfg, &server_a)
        .await
        .expect("connect server_a");
    connections::connect(&cfg, &server_b)
        .await
        .expect("connect server_b");

    // Send distinct payloads to each server; verify each echoes its own input.
    let result_a = connections::call_tool(
        &server_a.server_id,
        "echo",
        serde_json::json!({ "message": "payload-for-a" }),
    )
    .await
    .expect("call_tool on server_a should succeed");

    let result_b = connections::call_tool(
        &server_b.server_id,
        "echo",
        serde_json::json!({ "message": "payload-for-b" }),
    )
    .await
    .expect("call_tool on server_b should succeed");

    let text_a = extract_echo_text(&result_a);
    let text_b = extract_echo_text(&result_b);

    assert_eq!(
        text_a, "payload-for-a",
        "server_a must echo back exactly what was sent to it"
    );
    assert_eq!(
        text_b, "payload-for-b",
        "server_b must echo back exactly what was sent to it"
    );

    // Cross-verify: calling server_b's id echoes server_b's payload — not server_a's.
    assert_ne!(
        text_a, text_b,
        "two servers must not produce identical outputs for different inputs"
    );

    let _ = connections::disconnect(&server_a.server_id).await;
    let _ = connections::disconnect(&server_b.server_id).await;
}

// ── Test 3: failure isolation ─────────────────────────────────────────────────

/// One server failing to connect must not prevent another from connecting or
/// being usable in the same session. The bad server records an `error` status
/// with a `last_error` message; the good server is `connected` and responds.
#[tokio::test]
async fn failed_connect_does_not_block_healthy_peer() {
    let (_tmp, cfg) = fresh_workspace_config();

    // "bad" server: non-existent binary.
    let mut bad = make_stub_server("@isolation-test/bad");
    bad.command = "/this/path/does/not/exist/bad-mcp".to_string();

    // "good" server: real stub.
    let good = make_stub_server("@isolation-test/good");

    store::insert_server(&cfg, &bad).expect("insert bad server");
    store::insert_server(&cfg, &good).expect("insert good server");

    // Connecting the bad server must fail.
    let connect_err = connections::connect(&cfg, &bad)
        .await
        .expect_err("connect bad server should fail");
    assert!(
        !connect_err.to_string().is_empty(),
        "bad connect error must not be empty"
    );

    // Connecting the good server must succeed independently.
    let good_tools = connections::connect(&cfg, &good)
        .await
        .expect("connect good server must succeed regardless of the bad peer");
    assert_eq!(good_tools.len(), 1, "good server exposes one tool");

    // all_status: bad → error with last_error, good → connected.
    let statuses = connections::all_status(&cfg).await;
    let find = |id: &str| {
        statuses
            .iter()
            .find(|s| s.server_id == id)
            .cloned()
            .unwrap_or_else(|| panic!("status entry missing for {id}"))
    };

    let bad_status = find(&bad.server_id);
    assert_eq!(
        bad_status.status.as_str(),
        "error",
        "bad server must report `error`"
    );
    assert!(
        bad_status
            .last_error
            .as_deref()
            .map(|s| !s.is_empty())
            .unwrap_or(false),
        "bad server must have a non-empty last_error"
    );

    let good_status = find(&good.server_id);
    assert_eq!(
        good_status.status.as_str(),
        "connected",
        "good server must be `connected`"
    );
    assert!(
        good_status.last_error.is_none(),
        "good server must have no last_error"
    );

    // The good server is still callable after the peer's failure.
    let result = connections::call_tool(
        &good.server_id,
        "echo",
        serde_json::json!({ "message": "isolation-check" }),
    )
    .await
    .expect("call_tool on good server must succeed after peer failure");

    assert_eq!(
        extract_echo_text(&result),
        "isolation-check",
        "good server must echo input correctly after peer failure"
    );

    let _ = connections::disconnect(&good.server_id).await;
}

// ── Test 4: disabled enforcement ──────────────────────────────────────────────

/// A disabled server must not contribute tools to the agent surface and must
/// refuse explicit connect, even when a sibling is connected and active.
#[tokio::test]
async fn disabled_server_contributes_no_tools_to_agent_surface() {
    let (_tmp, cfg) = fresh_workspace_config();

    // "live" server: enabled, real stub.
    let live = make_stub_server("@disabled-test/live");

    // "quiet" server: disabled, also uses the real stub binary (so the
    // bogus-command failure path cannot mask the disabled enforcement check).
    let mut quiet = make_stub_server("@disabled-test/quiet");
    quiet.enabled = false;

    store::insert_server(&cfg, &live).expect("insert live server");
    store::insert_server(&cfg, &quiet).expect("insert quiet server");

    // Connect the live server.
    connections::connect(&cfg, &live)
        .await
        .expect("connect live server");

    // Attempting to connect the disabled server via ops must fail with
    // a clear "disabled" message (matches mcp_registry_e2e::connect_refuses_disabled_server).
    let disabled_err = ops::mcp_clients_connect(&cfg, quiet.server_id.clone())
        .await
        .expect_err("connect must reject a disabled server");
    assert!(
        disabled_err.to_lowercase().contains("disabled"),
        "error must mention 'disabled', got: {disabled_err}"
    );

    // all_status: live → connected, quiet → disabled.
    let statuses = connections::all_status(&cfg).await;
    let find = |id: &str| {
        statuses
            .iter()
            .find(|s| s.server_id == id)
            .cloned()
            .unwrap_or_else(|| panic!("status entry missing for {id}"))
    };

    assert_eq!(find(&live.server_id).status.as_str(), "connected");
    assert_eq!(find(&quiet.server_id).status.as_str(), "disabled");

    // The disabled server must not appear in all_connected_tools.
    let all_tools = connections::all_connected_tools().await;
    let quiet_tools: Vec<_> = all_tools
        .iter()
        .filter(|(sid, _, _)| sid == &quiet.server_id)
        .collect();
    assert!(
        quiet_tools.is_empty(),
        "disabled server must contribute zero tools to the agent surface; found: {quiet_tools:?}"
    );

    // The live server IS present and callable.
    let live_tools: Vec<_> = all_tools
        .iter()
        .filter(|(sid, _, _)| sid == &live.server_id)
        .collect();
    assert_eq!(
        live_tools.len(),
        1,
        "live server must contribute its tool even while disabled peer is present"
    );

    let result = connections::call_tool(
        &live.server_id,
        "echo",
        serde_json::json!({ "message": "live-while-quiet" }),
    )
    .await
    .expect("call_tool on live server must succeed");

    assert_eq!(
        extract_echo_text(&result),
        "live-while-quiet",
        "live server echoes correctly while disabled peer exists"
    );

    let _ = connections::disconnect(&live.server_id).await;
}