mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Handle required MCP startup failures across components (#10902)
Summary - add a `required` flag for MCP servers everywhere config/CLI data is touched so mandatory helpers can be round-tripped - have `codex exec` and `codex app-server` thread start/resume fail fast when required MCPs fail to initialize
This commit is contained in:
committed by
GitHub
Unverified
parent
3800173459
commit
aab61934af
@@ -5,6 +5,7 @@ use app_test_support::create_mock_responses_server_repeating_assistant;
|
||||
use app_test_support::rollout_path;
|
||||
use app_test_support::to_response;
|
||||
use chrono::Utc;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::SessionSource;
|
||||
@@ -261,6 +262,44 @@ async fn thread_resume_with_overrides_defers_updated_at_until_turn_start() -> Re
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_fails_when_required_mcp_server_fails_to_initialize() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
let rollout = setup_rollout_fixture(codex_home.path(), &server.uri())?;
|
||||
create_config_toml_with_required_broken_mcp(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let resume_id = mcp
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id: rollout.conversation_id,
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let err: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(resume_id)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
assert!(
|
||||
err.error
|
||||
.message
|
||||
.contains("required MCP servers failed to initialize"),
|
||||
"unexpected error message: {}",
|
||||
err.error.message
|
||||
);
|
||||
assert!(
|
||||
err.error.message.contains("required_broken"),
|
||||
"unexpected error message: {}",
|
||||
err.error.message
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_prefers_path_over_thread_id() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
@@ -481,6 +520,40 @@ stream_max_retries = 0
|
||||
)
|
||||
}
|
||||
|
||||
fn create_config_toml_with_required_broken_mcp(
|
||||
codex_home: &std::path::Path,
|
||||
server_uri: &str,
|
||||
) -> std::io::Result<()> {
|
||||
let config_toml = codex_home.join("config.toml");
|
||||
std::fs::write(
|
||||
config_toml,
|
||||
format!(
|
||||
r#"
|
||||
model = "gpt-5.2-codex"
|
||||
approval_policy = "never"
|
||||
sandbox_mode = "read-only"
|
||||
|
||||
model_provider = "mock_provider"
|
||||
|
||||
[features]
|
||||
remote_models = false
|
||||
personality = true
|
||||
|
||||
[model_providers.mock_provider]
|
||||
name = "Mock provider for test"
|
||||
base_url = "{server_uri}/v1"
|
||||
wire_api = "responses"
|
||||
request_max_retries = 0
|
||||
stream_max_retries = 0
|
||||
|
||||
[mcp_servers.required_broken]
|
||||
command = "codex-definitely-not-a-real-binary"
|
||||
required = true
|
||||
"#
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
fn set_rollout_mtime(path: &Path, updated_at_rfc3339: &str) -> Result<()> {
|
||||
let parsed = chrono::DateTime::parse_from_rfc3339(updated_at_rfc3339)?.with_timezone(&Utc);
|
||||
let times = FileTimes::new().set_modified(parsed.into());
|
||||
|
||||
@@ -2,6 +2,7 @@ use anyhow::Result;
|
||||
use app_test_support::McpProcess;
|
||||
use app_test_support::create_mock_responses_server_repeating_assistant;
|
||||
use app_test_support::to_response;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCNotification;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
@@ -113,6 +114,42 @@ model_reasoning_effort = "high"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_fails_when_required_mcp_server_fails_to_initialize() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml_with_required_broken_mcp(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let req_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams::default())
|
||||
.await?;
|
||||
|
||||
let err: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(req_id)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
assert!(
|
||||
err.error
|
||||
.message
|
||||
.contains("required MCP servers failed to initialize"),
|
||||
"unexpected error message: {}",
|
||||
err.error.message
|
||||
);
|
||||
assert!(
|
||||
err.error.message.contains("required_broken"),
|
||||
"unexpected error message: {}",
|
||||
err.error.message
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// Helper to create a config.toml pointing at the mock model server.
|
||||
fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> {
|
||||
let config_toml = codex_home.join("config.toml");
|
||||
@@ -136,3 +173,33 @@ stream_max_retries = 0
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
fn create_config_toml_with_required_broken_mcp(
|
||||
codex_home: &Path,
|
||||
server_uri: &str,
|
||||
) -> std::io::Result<()> {
|
||||
let config_toml = codex_home.join("config.toml");
|
||||
std::fs::write(
|
||||
config_toml,
|
||||
format!(
|
||||
r#"
|
||||
model = "mock-model"
|
||||
approval_policy = "never"
|
||||
sandbox_mode = "read-only"
|
||||
|
||||
model_provider = "mock_provider"
|
||||
|
||||
[model_providers.mock_provider]
|
||||
name = "Mock provider for test"
|
||||
base_url = "{server_uri}/v1"
|
||||
wire_api = "responses"
|
||||
request_max_retries = 0
|
||||
stream_max_retries = 0
|
||||
|
||||
[mcp_servers.required_broken]
|
||||
command = "codex-definitely-not-a-real-binary"
|
||||
required = true
|
||||
"#
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -243,6 +243,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re
|
||||
let new_entry = McpServerConfig {
|
||||
transport: transport.clone(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -791,6 +791,10 @@
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"required": {
|
||||
"default": null,
|
||||
"type": "boolean"
|
||||
},
|
||||
"scopes": {
|
||||
"default": null,
|
||||
"items": {
|
||||
|
||||
@@ -1115,6 +1115,12 @@ impl Session {
|
||||
sandbox_cwd: session_configuration.cwd.clone(),
|
||||
use_linux_sandbox_bwrap: config.features.enabled(Feature::UseLinuxSandboxBwrap),
|
||||
};
|
||||
let mut required_mcp_servers: Vec<String> = mcp_servers
|
||||
.iter()
|
||||
.filter(|(_, server)| server.enabled && server.required)
|
||||
.map(|(name, _)| name.clone())
|
||||
.collect();
|
||||
required_mcp_servers.sort();
|
||||
let cancel_token = sess.mcp_startup_cancellation_token().await;
|
||||
|
||||
sess.services
|
||||
@@ -1130,6 +1136,25 @@ impl Session {
|
||||
sandbox_state,
|
||||
)
|
||||
.await;
|
||||
if !required_mcp_servers.is_empty() {
|
||||
let failures = sess
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.required_startup_failures(&required_mcp_servers)
|
||||
.await;
|
||||
if !failures.is_empty() {
|
||||
let details = failures
|
||||
.iter()
|
||||
.map(|failure| format!("{}: {}", failure.server, failure.error))
|
||||
.collect::<Vec<_>>()
|
||||
.join("; ");
|
||||
return Err(anyhow::anyhow!(
|
||||
"required MCP servers failed to initialize: {details}"
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// record_initial_history can emit events. We record only after the SessionConfiguredEvent is emitted.
|
||||
sess.record_initial_history(initial_history).await;
|
||||
|
||||
@@ -169,6 +169,9 @@ mod document_helpers {
|
||||
if !config.enabled {
|
||||
entry["enabled"] = value(false);
|
||||
}
|
||||
if config.required {
|
||||
entry["required"] = value(true);
|
||||
}
|
||||
if let Some(timeout) = config.startup_timeout_sec {
|
||||
entry["startup_timeout_sec"] = value(timeout.as_secs_f64());
|
||||
}
|
||||
@@ -1391,6 +1394,7 @@ gpt-5 = "gpt-5.1"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -1414,6 +1418,7 @@ gpt-5 = "gpt-5.1"
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: false,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(std::time::Duration::from_secs(5)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -1480,6 +1485,7 @@ foo = { command = "cmd" }
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -1525,6 +1531,7 @@ foo = { command = "cmd" } # keep me
|
||||
cwd: None,
|
||||
},
|
||||
enabled: false,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -1569,6 +1576,7 @@ foo = { command = "cmd", args = ["--flag"] } # keep me
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -1614,6 +1622,7 @@ foo = { command = "cmd" }
|
||||
cwd: None,
|
||||
},
|
||||
enabled: false,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -1916,6 +1916,7 @@ mod tests {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -1934,6 +1935,7 @@ mod tests {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -2800,6 +2802,7 @@ profile = "project"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(3)),
|
||||
tool_timeout_sec: Some(Duration::from_secs(5)),
|
||||
@@ -2956,6 +2959,7 @@ bearer_token = "secret"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -3026,6 +3030,7 @@ ZIG_VAR = "3"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -3076,6 +3081,7 @@ ZIG_VAR = "3"
|
||||
cwd: Some(cwd_path.clone()),
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -3124,6 +3130,7 @@ ZIG_VAR = "3"
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(2)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -3188,6 +3195,7 @@ startup_timeout_sec = 2.0
|
||||
)])),
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(2)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -3264,6 +3272,7 @@ X-Auth = "DOCS_AUTH"
|
||||
)])),
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(2)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -3293,6 +3302,7 @@ X-Auth = "DOCS_AUTH"
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -3360,6 +3370,7 @@ url = "https://example.com/mcp"
|
||||
)])),
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(2)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -3379,6 +3390,7 @@ url = "https://example.com/mcp"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -3461,6 +3473,7 @@ url = "https://example.com/mcp"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: false,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -3490,6 +3503,51 @@ url = "https://example.com/mcp"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn replace_mcp_servers_serializes_required_flag() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
let servers = BTreeMap::from([(
|
||||
"docs".to_string(),
|
||||
McpServerConfig {
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: "docs-server".to_string(),
|
||||
args: Vec::new(),
|
||||
env: None,
|
||||
env_vars: Vec::new(),
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: true,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
},
|
||||
)]);
|
||||
|
||||
apply_blocking(
|
||||
codex_home.path(),
|
||||
None,
|
||||
&[ConfigEdit::ReplaceMcpServers(servers.clone())],
|
||||
)?;
|
||||
|
||||
let config_path = codex_home.path().join(CONFIG_TOML_FILE);
|
||||
let serialized = std::fs::read_to_string(&config_path)?;
|
||||
assert!(
|
||||
serialized.contains("required = true"),
|
||||
"serialized config missing required flag:\n{serialized}"
|
||||
);
|
||||
|
||||
let loaded = load_global_mcp_servers(codex_home.path()).await?;
|
||||
let docs = loaded.get("docs").expect("docs entry");
|
||||
assert!(docs.required);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn replace_mcp_servers_serializes_tool_filters() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
@@ -3505,6 +3563,7 @@ url = "https://example.com/mcp"
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -50,6 +50,10 @@ pub struct McpServerConfig {
|
||||
#[serde(default = "default_enabled")]
|
||||
pub enabled: bool,
|
||||
|
||||
/// When `true`, `codex exec` exits with an error if this MCP server fails to initialize.
|
||||
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
|
||||
pub required: bool,
|
||||
|
||||
/// Reason this server was disabled after applying requirements.
|
||||
#[serde(skip)]
|
||||
pub disabled_reason: Option<McpServerDisabledReason>,
|
||||
@@ -114,6 +118,8 @@ pub(crate) struct RawMcpServerConfig {
|
||||
#[serde(default)]
|
||||
pub enabled: Option<bool>,
|
||||
#[serde(default)]
|
||||
pub required: Option<bool>,
|
||||
#[serde(default)]
|
||||
pub enabled_tools: Option<Vec<String>>,
|
||||
#[serde(default)]
|
||||
pub disabled_tools: Option<Vec<String>>,
|
||||
@@ -138,6 +144,7 @@ impl<'de> Deserialize<'de> for McpServerConfig {
|
||||
};
|
||||
let tool_timeout_sec = raw.tool_timeout_sec;
|
||||
let enabled = raw.enabled.unwrap_or_else(default_enabled);
|
||||
let required = raw.required.unwrap_or_default();
|
||||
let enabled_tools = raw.enabled_tools.clone();
|
||||
let disabled_tools = raw.disabled_tools.clone();
|
||||
let scopes = raw.scopes.clone();
|
||||
@@ -192,6 +199,7 @@ impl<'de> Deserialize<'de> for McpServerConfig {
|
||||
startup_timeout_sec,
|
||||
tool_timeout_sec,
|
||||
enabled,
|
||||
required,
|
||||
disabled_reason: None,
|
||||
enabled_tools,
|
||||
disabled_tools,
|
||||
@@ -697,6 +705,7 @@ mod tests {
|
||||
}
|
||||
);
|
||||
assert!(cfg.enabled);
|
||||
assert!(!cfg.required);
|
||||
assert!(cfg.enabled_tools.is_none());
|
||||
assert!(cfg.disabled_tools.is_none());
|
||||
}
|
||||
@@ -803,6 +812,20 @@ mod tests {
|
||||
.expect("should deserialize disabled server config");
|
||||
|
||||
assert!(!cfg.enabled);
|
||||
assert!(!cfg.required);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deserialize_required_server_config() {
|
||||
let cfg: McpServerConfig = toml::from_str(
|
||||
r#"
|
||||
command = "echo"
|
||||
required = true
|
||||
"#,
|
||||
)
|
||||
.expect("should deserialize required server config");
|
||||
|
||||
assert!(cfg.required);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -99,6 +99,7 @@ fn codex_apps_mcp_server_config(config: &Config, auth: Option<&CodexAuth>) -> Mc
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(30)),
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -379,6 +379,7 @@ fn mcp_dependency_to_server_config(
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -402,6 +403,7 @@ fn mcp_dependency_to_server_config(
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -455,6 +457,7 @@ mod tests {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -502,6 +505,7 @@ mod tests {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -474,6 +474,31 @@ impl McpConnectionManager {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn required_startup_failures(
|
||||
&self,
|
||||
required_servers: &[String],
|
||||
) -> Vec<McpStartupFailure> {
|
||||
let mut failures = Vec::new();
|
||||
for server_name in required_servers {
|
||||
let Some(async_managed_client) = self.clients.get(server_name).cloned() else {
|
||||
failures.push(McpStartupFailure {
|
||||
server: server_name.clone(),
|
||||
error: format!("required MCP server `{server_name}` was not initialized"),
|
||||
});
|
||||
continue;
|
||||
};
|
||||
|
||||
match async_managed_client.client().await {
|
||||
Ok(_) => {}
|
||||
Err(error) => failures.push(McpStartupFailure {
|
||||
server: server_name.clone(),
|
||||
error: startup_outcome_error_message(error),
|
||||
}),
|
||||
}
|
||||
}
|
||||
failures
|
||||
}
|
||||
|
||||
/// Returns a single map that contains all tools. Each key is the
|
||||
/// fully-qualified name for the tool.
|
||||
#[instrument(level = "trace", skip_all)]
|
||||
@@ -1131,6 +1156,13 @@ fn is_mcp_client_startup_timeout_error(error: &StartupOutcomeError) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
fn startup_outcome_error_message(error: StartupOutcomeError) -> String {
|
||||
match error {
|
||||
StartupOutcomeError::Cancelled => "MCP startup cancelled".to_string(),
|
||||
StartupOutcomeError::Failed { error } => error,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod mcp_init_error_display_tests {}
|
||||
|
||||
@@ -1325,6 +1357,7 @@ mod tests {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -1371,6 +1404,7 @@ mod tests {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -87,6 +87,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -228,6 +229,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -376,6 +378,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -534,6 +537,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -724,6 +728,7 @@ async fn streamable_http_with_oauth_round_trip() -> anyhow::Result<()> {
|
||||
env_http_headers: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -426,6 +426,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()>
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(std::time::Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -519,6 +520,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
@@ -783,6 +785,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(std::time::Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
|
||||
@@ -304,6 +304,13 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
last_message_file.clone(),
|
||||
)),
|
||||
};
|
||||
let required_mcp_servers: HashSet<String> = config
|
||||
.mcp_servers
|
||||
.get()
|
||||
.iter()
|
||||
.filter(|(_, server)| server.enabled && server.required)
|
||||
.map(|(name, _)| name.clone())
|
||||
.collect();
|
||||
|
||||
if oss {
|
||||
// We're in the oss section, so provider_id should be Some
|
||||
@@ -516,6 +523,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
// Track whether a fatal error was reported by the server so we can
|
||||
// exit with a non-zero status for automation-friendly signaling.
|
||||
let mut error_seen = false;
|
||||
let mut shutdown_requested = false;
|
||||
while let Some(envelope) = rx.recv().await {
|
||||
let ThreadEventEnvelope {
|
||||
thread_id,
|
||||
@@ -532,6 +540,20 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
})
|
||||
.await?;
|
||||
}
|
||||
if let EventMsg::McpStartupUpdate(update) = &event.msg
|
||||
&& required_mcp_servers.contains(&update.server)
|
||||
&& let codex_core::protocol::McpStartupStatus::Failed { error } = &update.status
|
||||
{
|
||||
error_seen = true;
|
||||
eprintln!(
|
||||
"Required MCP server '{}' failed to initialize: {error}",
|
||||
update.server
|
||||
);
|
||||
if !shutdown_requested {
|
||||
thread.submit(Op::Shutdown).await?;
|
||||
shutdown_requested = true;
|
||||
}
|
||||
}
|
||||
if matches!(event.msg, EventMsg::Error(_)) {
|
||||
error_seen = true;
|
||||
}
|
||||
@@ -545,7 +567,10 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
match shutdown {
|
||||
CodexStatus::Running => continue,
|
||||
CodexStatus::InitiateShutdown => {
|
||||
thread.submit(Op::Shutdown).await?;
|
||||
if !shutdown_requested {
|
||||
thread.submit(Op::Shutdown).await?;
|
||||
shutdown_requested = true;
|
||||
}
|
||||
}
|
||||
CodexStatus::Shutdown if thread_id == primary_thread_id => break,
|
||||
CodexStatus::Shutdown => continue,
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
#![cfg(not(target_os = "windows"))]
|
||||
#![allow(clippy::expect_used, clippy::unwrap_used)]
|
||||
|
||||
use core_test_support::responses;
|
||||
use core_test_support::test_codex_exec::test_codex_exec;
|
||||
use predicates::str::contains;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn exits_non_zero_when_required_mcp_server_fails_to_initialize() -> anyhow::Result<()> {
|
||||
let test = test_codex_exec();
|
||||
|
||||
let config_toml = r#"
|
||||
[mcp_servers.required_broken]
|
||||
command = "codex-definitely-not-a-real-binary"
|
||||
required = true
|
||||
"#;
|
||||
std::fs::write(test.home_path().join("config.toml"), config_toml)?;
|
||||
|
||||
let server = responses::start_mock_server().await;
|
||||
let body = responses::sse(vec![
|
||||
responses::ev_response_created("resp_1"),
|
||||
responses::ev_assistant_message("msg_1", "hello"),
|
||||
responses::ev_completed("resp_1"),
|
||||
]);
|
||||
responses::mount_sse_once(&server, body).await;
|
||||
|
||||
test.cmd_with_server(&server)
|
||||
.arg("--skip-git-repo-check")
|
||||
.arg("--experimental-json")
|
||||
.arg("tell me something")
|
||||
.assert()
|
||||
.code(1)
|
||||
.stderr(contains(
|
||||
"required MCP servers failed to initialize: required_broken",
|
||||
));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -3,6 +3,7 @@ mod add_dir;
|
||||
mod apply_patch;
|
||||
mod auth_env;
|
||||
mod ephemeral;
|
||||
mod mcp_required_exit;
|
||||
mod originator;
|
||||
mod output_schema;
|
||||
mod resume;
|
||||
|
||||
@@ -2525,6 +2525,7 @@ mod tests {
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
@@ -2547,6 +2548,7 @@ mod tests {
|
||||
env_http_headers: Some(env_headers),
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
|
||||
Reference in New Issue
Block a user