Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions tests/agent_harness_e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3116,6 +3116,16 @@ async fn model_call_ceiling_bounds_a_wedged_call_below_the_turn_deadline_inner()
// fires is unambiguous. Both are read per turn by `run_policy_for`.
let _per_call = EnvVarGuard::set("OPENHUMAN_MODEL_CALL_TIMEOUT_SECS", "2");
let _turn = EnvVarGuard::set("OPENHUMAN_AGENT_TURN_TIMEOUT_SECS", "600");
// There is a THIRD wall clock, and pinning only the two above leaves this
// test's conclusion resting on the ambient environment. `web_turn_deadline()`
// (`web_chat/ops_part_01.rs:79`) applies its own backstop from
// `OPENHUMAN_WEB_TURN_TIMEOUT_SECS`, and the web layer maps every harness
// `Timeout` onto the same generic `turn_timeout` copy — so an inherited
// value below the 8s bound asserted here would fire first, produce an
// identical terminal, and let the whole test pass with the per-call ceiling
// unwired. Cleared, not set, so it falls back to its 900s default and can
// play no part in the outcome.
let _web_backstop = EnvVarGuard::unset("OPENHUMAN_WEB_TURN_TIMEOUT_SECS");

// Every upstream reply is held for 25s — comfortably past the 2s per-call
// ceiling and comfortably short of the 600s turn deadline. Armed globally
Expand Down
110 changes: 110 additions & 0 deletions tests/mcp_registry_e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -850,3 +850,113 @@ fn supervisor_default_probe_window_stays_eight_seconds() {
regression b44b958d reverted"
);
}

/// An HTTP-remote install pointed at `url`.
///
/// The stdio fixture above cannot reach the auth path at all: a 401 is an HTTP
/// fact, so the credential hints only exist for this transport.
fn make_http_remote_server(url: &str) -> InstalledServer {
InstalledServer {
server_id: format!("test-http-{}", uuid::Uuid::new_v4()),
qualified_name: "@openhuman-test/remote".to_string(),
display_name: "Test Remote".to_string(),
description: Some("Stub HTTP-remote MCP server used by mcp_registry_e2e tests.".into()),
icon_url: None,
// Carried but unread for this transport — callers route off `transport`.
command_kind: CommandKind::Binary,
command: String::new(),
args: Vec::new(),
env_keys: Vec::new(),
config: None,
installed_at: 0,
last_connected_at: None,
transport: Transport::HttpRemote {
url: url.to_string(),
},
enabled: true,
}
}

/// The failure side of the per-config lookups (#5701).
///
/// `per_config_lookups_see_a_per_config_connection` covers the success side and
/// asserts `last_error_for_config` is `None` after a clean connect. That leaves
/// the half that matters for diagnosis untested: `auth_hint_for_config` had no
/// reference in any e2e lane at all, and nothing anywhere asserted that a real
/// failure is actually *surfaced* rather than folded into the same `None` a
/// missing workspace returns.
///
/// The two-workspace precondition is what makes this discriminate. With two
/// hosts open and no default, `host::try_service()` refuses to guess, so the
/// ambient forms answer `None` for a server that genuinely has both a hint and
/// an error recorded — which is exactly the confusion #5701 was filed about.
#[tokio::test]
async fn per_config_failure_lookups_surface_the_reason() {
use openhuman_core::openhuman::mcp::host;
use openhuman_core::openhuman::mcp::registry::connections;
use wiremock::matchers::any;
use wiremock::{Mock, MockServer, ResponseTemplate};

// A server that refuses everything for want of credentials.
let upstream = MockServer::start().await;
Mock::given(any())
.respond_with(ResponseTemplate::new(401))
.mount(&upstream)
.await;

let (_tmp_a, cfg_a) = fresh_workspace_config();
let (_tmp_b, cfg_b) = fresh_workspace_config();
let _host_b = host(&cfg_b);

let h = host(&cfg_a);
let server = make_http_remote_server(&upstream.uri());
h.dynamic()
.store()
.insert_server(&server)
.expect("insert installed server");

let outcome = connections::connect(&cfg_a, &server).await;
assert!(
outcome.is_err(),
"a 401 from the endpoint must fail the connect, not report success"
);

assert!(
host::try_service().is_none(),
"two workspaces open and no default: the ambient service must refuse to guess, \
which is what makes the per-config forms the only correct ones here"
);

let hint = connections::auth_hint_for_config(&cfg_a, &server.server_id).await;
assert_eq!(
hint,
Some("credential_required"),
"the workspace that attempted the connect must see why its 401 happened"
);

let last_error = connections::last_error_for_config(&cfg_a, &server.server_id).await;
// Non-empty after trimming, not merely `Some`: the contract this pins is a
// *readable* reason, and `Some(String::new())` satisfies `is_some()` while
// telling a caller polling status precisely nothing.
assert!(
last_error
.as_deref()
.is_some_and(|error| !error.trim().is_empty()),
"a failed attempt must leave a readable reason behind — `connect`'s own contract \
is that a caller polling status sees it without re-attempting; got {last_error:?}"
);

// A failure belongs to the workspace that hit it, not to every workspace.
assert!(
connections::auth_hint_for_config(&cfg_b, &server.server_id)
.await
.is_none(),
"an auth hint must not leak across workspaces"
);
assert!(
connections::last_error_for_config(&cfg_b, &server.server_id)
.await
.is_none(),
"a failure must not leak across workspaces"
);
}
Loading