mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Avoid blocking thread startup on MCP OAuth discovery (#32825)
## Why OAuth metadata discovery for an optional HTTP MCP server can stall, delaying `thread/start` even though that server is not required. ## What changed - Remove eager MCP authentication-status discovery from session and connection manager startup. - When an MCP connection reports an authentication error, inspect configured and stored credentials locally to preserve login and reauthentication failure reasons without making another discovery request. - Continue using server configuration directly for startup error messages and timeout hints. ## Testing Add an app-server regression test with an optional HTTP MCP endpoint that accepts a connection but never responds, and verify that `thread/start` completes without waiting for its configured startup timeout. GitOrigin-RevId: 56fe0188cc1ceaf746b0b65e8b7dd1fd2e13420d
This commit is contained in:
jif authored and copyberry committed
1 parent
4ebc61c0f8
commit
2e156cbe31
12 files changed
+219
-139
No files matched your search
@@ -1492,7 +1492,6 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() {
|
||||
&mcp_servers,
|
||||
OAuthCredentialsStoreMode::default(),
|
||||
AuthKeyringBackendKind::default(),
|
||||
HashMap::new(),
|
||||
&approval_policy,
|
||||
String::new(),
|
||||
tx_event,
|
||||
@@ -1570,35 +1569,32 @@ fn elicitation_capability_advertises_url_support_when_enabled() {
|
||||
#[test]
|
||||
fn mcp_init_error_display_prompts_for_github_pat() {
|
||||
let server_name = "github";
|
||||
let entry = McpAuthStatusEntry {
|
||||
config: Some(McpServerConfig {
|
||||
auth: Default::default(),
|
||||
transport: McpServerTransportConfig::StreamableHttp {
|
||||
url: "https://api.githubcopilot.com/mcp/".to_string(),
|
||||
bearer_token_env_var: None,
|
||||
http_headers: None,
|
||||
env_http_headers: None,
|
||||
},
|
||||
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}),
|
||||
auth_state: McpAuthState::Unsupported,
|
||||
let config = McpServerConfig {
|
||||
auth: Default::default(),
|
||||
transport: McpServerTransportConfig::StreamableHttp {
|
||||
url: "https://api.githubcopilot.com/mcp/".to_string(),
|
||||
bearer_token_env_var: None,
|
||||
http_headers: None,
|
||||
env_http_headers: None,
|
||||
},
|
||||
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
let err: StartupOutcomeError = anyhow::anyhow!("OAuth is unsupported").into();
|
||||
|
||||
let display = mcp_init_error_display(server_name, Some(&entry), &err);
|
||||
let display = mcp_init_error_display(server_name, Some(&config), &err);
|
||||
|
||||
let expected = format!(
|
||||
"GitHub MCP does not support OAuth. Log in by adding a personal access token (https://github.com/settings/personal-access-tokens) to your environment and config.toml:\n[mcp_servers.{server_name}]\nbearer_token_env_var = CODEX_GITHUB_PERSONAL_ACCESS_TOKEN"
|
||||
@@ -1612,7 +1608,7 @@ fn mcp_init_error_display_prompts_for_login_when_auth_required() {
|
||||
let server_name = "example";
|
||||
let err: StartupOutcomeError = anyhow::anyhow!("Auth required for server").into();
|
||||
|
||||
let display = mcp_init_error_display(server_name, /*entry*/ None, &err);
|
||||
let display = mcp_init_error_display(server_name, /*config*/ None, &err);
|
||||
|
||||
let expected = format!(
|
||||
"The {server_name} MCP server is not logged in. Run `codex mcp login {server_name}`."
|
||||
@@ -1648,17 +1644,13 @@ fn mcp_startup_failure_reason_requires_existing_oauth_and_auth_failure() {
|
||||
(Some(McpAuthState::OAuth), true, None),
|
||||
(None, true, None),
|
||||
] {
|
||||
let entry = auth_state.map(|auth_state| McpAuthStatusEntry {
|
||||
config: None,
|
||||
auth_state,
|
||||
});
|
||||
let error = StartupOutcomeError::Failed {
|
||||
error: "startup failed".to_string(),
|
||||
is_authentication_required,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
mcp_startup_failure_reason(entry.as_ref(), &error),
|
||||
mcp_startup_failure_reason(auth_state, &error),
|
||||
expected,
|
||||
"auth_state={auth_state:?}, is_authentication_required={is_authentication_required}"
|
||||
);
|
||||
@@ -1668,35 +1660,32 @@ fn mcp_startup_failure_reason_requires_existing_oauth_and_auth_failure() {
|
||||
#[test]
|
||||
fn mcp_init_error_display_reports_generic_errors() {
|
||||
let server_name = "custom";
|
||||
let entry = McpAuthStatusEntry {
|
||||
config: Some(McpServerConfig {
|
||||
auth: Default::default(),
|
||||
transport: McpServerTransportConfig::StreamableHttp {
|
||||
url: "https://example.com".to_string(),
|
||||
bearer_token_env_var: Some("TOKEN".to_string()),
|
||||
http_headers: None,
|
||||
env_http_headers: None,
|
||||
},
|
||||
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}),
|
||||
auth_state: McpAuthState::Unsupported,
|
||||
let config = McpServerConfig {
|
||||
auth: Default::default(),
|
||||
transport: McpServerTransportConfig::StreamableHttp {
|
||||
url: "https://example.com".to_string(),
|
||||
bearer_token_env_var: Some("TOKEN".to_string()),
|
||||
http_headers: None,
|
||||
env_http_headers: None,
|
||||
},
|
||||
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
let err: StartupOutcomeError = anyhow::anyhow!("boom").into();
|
||||
|
||||
let display = mcp_init_error_display(server_name, Some(&entry), &err);
|
||||
let display = mcp_init_error_display(server_name, Some(&config), &err);
|
||||
|
||||
let expected = format!("MCP client for `{server_name}` failed to start: {err:#}");
|
||||
|
||||
@@ -1712,7 +1701,7 @@ fn mcp_init_error_display_includes_startup_timeout_hint() {
|
||||
] {
|
||||
let err: StartupOutcomeError = anyhow::anyhow!(error).into();
|
||||
|
||||
let display = mcp_init_error_display(server_name, /*entry*/ None, &err);
|
||||
let display = mcp_init_error_display(server_name, /*config*/ None, &err);
|
||||
|
||||
assert_eq!(
|
||||
"MCP client for `slow` timed out after 30 seconds. Add or adjust `startup_timeout_sec` in your config.toml:\n[mcp_servers.slow]\nstartup_timeout_sec = XX",
|
||||
|
||||
Reference in new issue
Block a user