mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
Stabilize plugin MCP tools test (#19191)
## Summary The plugin MCP tool-listing test could hide MCP startup failures by polling `ListMcpTools` until its own 30s deadline. If the plugin MCP server startup had already failed or timed out, the session-owned MCP manager would keep returning an empty tool list, so CI only reported `discovered tools: []` instead of the startup state that mattered. This makes the test synchronize on `McpStartupComplete` for the sample plugin MCP server before asserting listed tools, and gives the Bazel-launched test server a larger startup window. ## Notes Confidence is about 80%. The source path strongly supports the RCA: a failed MCP startup is represented as an empty tool list through `ListMcpTools`, so the old polling contract could not distinguish "not ready yet" from "startup already failed." I could not retrieve the CI execution-log artifact to confirm the exact hidden startup error, but the observed Ubuntu Bazel failure matches this path: repeated `ListMcpTools` responses with no tools until the test-local timeout fired. I think this is the right solution because it keeps plugin behavior unchanged and fixes only the test contract. Future startup failures should now report the `McpStartupComplete` failure/cancellation instead of timing out on an empty tool snapshot. This test was introduced in https://github.com/openai/codex/pull/12864.
This commit is contained in:
@@ -6,6 +6,7 @@ use std::time::Duration;
|
|||||||
use std::time::Instant;
|
use std::time::Instant;
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
|
use anyhow::bail;
|
||||||
use codex_features::Feature;
|
use codex_features::Feature;
|
||||||
use codex_login::CodexAuth;
|
use codex_login::CodexAuth;
|
||||||
use codex_protocol::protocol::EventMsg;
|
use codex_protocol::protocol::EventMsg;
|
||||||
@@ -72,7 +73,8 @@ fn write_plugin_mcp_plugin(home: &TempDir, command: &str) {
|
|||||||
r#"{{
|
r#"{{
|
||||||
"mcpServers": {{
|
"mcpServers": {{
|
||||||
"sample": {{
|
"sample": {{
|
||||||
"command": "{command}"
|
"command": "{command}",
|
||||||
|
"startup_timeout_sec": 60.0
|
||||||
}}
|
}}
|
||||||
}}
|
}}
|
||||||
}}"#
|
}}"#
|
||||||
@@ -415,30 +417,58 @@ async fn plugin_mcp_tools_are_listed() -> Result<()> {
|
|||||||
write_plugin_mcp_plugin(codex_home.as_ref(), &rmcp_test_server_bin);
|
write_plugin_mcp_plugin(codex_home.as_ref(), &rmcp_test_server_bin);
|
||||||
let codex = build_plugin_test_codex(&server, codex_home).await?;
|
let codex = build_plugin_test_codex(&server, codex_home).await?;
|
||||||
|
|
||||||
let tools_ready_deadline = Instant::now() + Duration::from_secs(30);
|
let startup_event = wait_for_event_with_timeout(
|
||||||
loop {
|
&codex,
|
||||||
codex.submit(Op::ListMcpTools).await?;
|
|ev| match ev {
|
||||||
let list_event = wait_for_event_with_timeout(
|
EventMsg::McpStartupComplete(summary) => {
|
||||||
&codex,
|
summary.ready.iter().any(|server| server == "sample")
|
||||||
|ev| matches!(ev, EventMsg::McpListToolsResponse(_)),
|
|| summary
|
||||||
Duration::from_secs(10),
|
.failed
|
||||||
)
|
.iter()
|
||||||
.await;
|
.any(|failure| failure.server == "sample")
|
||||||
let EventMsg::McpListToolsResponse(tool_list) = list_event else {
|
|| summary.cancelled.iter().any(|server| server == "sample")
|
||||||
unreachable!("event guard guarantees McpListToolsResponse");
|
}
|
||||||
};
|
_ => false,
|
||||||
if tool_list.tools.contains_key("mcp__sample__echo")
|
},
|
||||||
&& tool_list.tools.contains_key("mcp__sample__image")
|
Duration::from_secs(70),
|
||||||
{
|
)
|
||||||
break;
|
.await;
|
||||||
}
|
let EventMsg::McpStartupComplete(startup) = startup_event else {
|
||||||
|
unreachable!("event guard guarantees McpStartupComplete");
|
||||||
let available_tools: Vec<&str> = tool_list.tools.keys().map(String::as_str).collect();
|
};
|
||||||
if Instant::now() >= tools_ready_deadline {
|
if let Some(failure) = startup
|
||||||
panic!("timed out waiting for plugin MCP tools; discovered tools: {available_tools:?}");
|
.failed
|
||||||
}
|
.iter()
|
||||||
tokio::time::sleep(Duration::from_millis(200)).await;
|
.find(|failure| failure.server == "sample")
|
||||||
|
{
|
||||||
|
let error = &failure.error;
|
||||||
|
bail!("plugin MCP server failed to start: {error}");
|
||||||
}
|
}
|
||||||
|
if startup.cancelled.iter().any(|server| server == "sample") {
|
||||||
|
bail!("plugin MCP server startup was cancelled");
|
||||||
|
}
|
||||||
|
assert!(
|
||||||
|
startup.ready.iter().any(|server| server == "sample"),
|
||||||
|
"expected plugin MCP server to be ready; startup summary: {startup:?}"
|
||||||
|
);
|
||||||
|
|
||||||
|
codex.submit(Op::ListMcpTools).await?;
|
||||||
|
let list_event = wait_for_event_with_timeout(
|
||||||
|
&codex,
|
||||||
|
|ev| matches!(ev, EventMsg::McpListToolsResponse(_)),
|
||||||
|
Duration::from_secs(10),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
let EventMsg::McpListToolsResponse(tool_list) = list_event else {
|
||||||
|
unreachable!("event guard guarantees McpListToolsResponse");
|
||||||
|
};
|
||||||
|
let mut available_tools: Vec<&str> = tool_list.tools.keys().map(String::as_str).collect();
|
||||||
|
available_tools.sort_unstable();
|
||||||
|
assert!(
|
||||||
|
tool_list.tools.contains_key("mcp__sample__echo")
|
||||||
|
&& tool_list.tools.contains_key("mcp__sample__image"),
|
||||||
|
"expected plugin MCP tools to be listed; discovered tools: {available_tools:?}"
|
||||||
|
);
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user