mirror of
https://github.com/openai/codex.git
synced 2026-11-07 08:34:29 +00:00
Expand MCP tool catalog cache regression coverage (#33308)
## Why Cached MCP tool catalogs must not substitute stale session state for the live server connection. ## What changed - Verify `mcpServerStatus/list` waits for replacement stdio server metadata before combining it with cached tools. - Verify configurations with remotely sourced environment variables bypass the tool catalog cache. - Assert that calls made from cached definitions return output from the live replacement process. - Make the stdio test server available to Windows Bazel integration tests. GitOrigin-RevId: 0e518cd46f0d89af28e3a4e256023b5872e6e2fb
This commit is contained in:
1 parent
3307ea8b63
commit
1bbdb32789
4 files changed
+172
-1
No files matched your search
@@ -6,10 +6,10 @@ codex_rust_crate(
|
|||||||
extra_binaries = [
|
extra_binaries = [
|
||||||
"//codex-rs/bwrap:bwrap",
|
"//codex-rs/bwrap:bwrap",
|
||||||
"//codex-rs/code-mode-host:codex-code-mode-host",
|
"//codex-rs/code-mode-host:codex-code-mode-host",
|
||||||
|
"//codex-rs/rmcp-client:test_stdio_server",
|
||||||
],
|
],
|
||||||
extra_binaries_non_windows = [
|
extra_binaries_non_windows = [
|
||||||
"//codex-rs/cli:codex",
|
"//codex-rs/cli:codex",
|
||||||
"//codex-rs/rmcp-client:test_stdio_server",
|
|
||||||
],
|
],
|
||||||
integration_test_timeout = "long",
|
integration_test_timeout = "long",
|
||||||
run_tests_with_wine_exec = True,
|
run_tests_with_wine_exec = True,
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
use std::borrow::Cow;
|
use std::borrow::Cow;
|
||||||
use std::collections::BTreeMap;
|
use std::collections::BTreeMap;
|
||||||
use std::collections::BTreeSet;
|
use std::collections::BTreeSet;
|
||||||
|
use std::path::Path;
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
|
|
||||||
@@ -18,6 +19,7 @@ use codex_app_server_protocol::ThreadStartParams;
|
|||||||
use codex_app_server_protocol::ThreadStartResponse;
|
use codex_app_server_protocol::ThreadStartResponse;
|
||||||
use codex_core::config::set_project_trust_level;
|
use codex_core::config::set_project_trust_level;
|
||||||
use codex_protocol::config_types::TrustLevel;
|
use codex_protocol::config_types::TrustLevel;
|
||||||
|
use core_test_support::stdio_server_bin;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use rmcp::handler::server::ServerHandler;
|
use rmcp::handler::server::ServerHandler;
|
||||||
use rmcp::model::Implementation;
|
use rmcp::model::Implementation;
|
||||||
@@ -38,10 +40,46 @@ use serde_json::json;
|
|||||||
use tempfile::TempDir;
|
use tempfile::TempDir;
|
||||||
use tokio::net::TcpListener;
|
use tokio::net::TcpListener;
|
||||||
use tokio::task::JoinHandle;
|
use tokio::task::JoinHandle;
|
||||||
|
use tokio::time::sleep;
|
||||||
use tokio::time::timeout;
|
use tokio::time::timeout;
|
||||||
|
|
||||||
const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10);
|
const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10);
|
||||||
|
|
||||||
|
async fn wait_for_new_pid(path: &Path, previous_pid: Option<&str>) -> Result<String> {
|
||||||
|
Ok(timeout(DEFAULT_READ_TIMEOUT, async {
|
||||||
|
loop {
|
||||||
|
if let Ok(contents) = std::fs::read_to_string(path) {
|
||||||
|
let pid = contents.trim();
|
||||||
|
if !pid.is_empty() && Some(pid) != previous_pid {
|
||||||
|
return pid.to_string();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
sleep(Duration::from_millis(10)).await;
|
||||||
|
}
|
||||||
|
})
|
||||||
|
.await?)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn assert_dynamic_status(response: &ListMcpServerStatusResponse, process_label: &str) {
|
||||||
|
assert_eq!(response.data.len(), 1);
|
||||||
|
let status = &response.data[0];
|
||||||
|
assert_eq!(status.name, "cached-stdio");
|
||||||
|
assert_eq!(
|
||||||
|
status
|
||||||
|
.server_info
|
||||||
|
.as_ref()
|
||||||
|
.and_then(|info| info.title.as_deref()),
|
||||||
|
Some(process_label)
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
status
|
||||||
|
.tools
|
||||||
|
.get("echo")
|
||||||
|
.and_then(|tool| tool.description.as_deref()),
|
||||||
|
Some(format!("Echo from {process_label}.").as_str())
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn mcp_server_status_list_returns_raw_server_and_tool_names() -> Result<()> {
|
async fn mcp_server_status_list_returns_raw_server_and_tool_names() -> Result<()> {
|
||||||
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
||||||
@@ -118,6 +156,100 @@ url = "{mcp_server_url}/mcp"
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn mcp_server_status_list_waits_for_live_stdio_metadata_before_using_cached_tools()
|
||||||
|
-> Result<()> {
|
||||||
|
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
||||||
|
let codex_home = TempDir::new()?;
|
||||||
|
write_mock_responses_config_toml(
|
||||||
|
codex_home.path(),
|
||||||
|
&server.uri(),
|
||||||
|
&BTreeMap::new(),
|
||||||
|
/*auto_compact_limit*/ 1024,
|
||||||
|
/*requires_openai_auth*/ None,
|
||||||
|
"mock_provider",
|
||||||
|
"compact",
|
||||||
|
)?;
|
||||||
|
|
||||||
|
let barrier_file = codex_home.path().join("allow-initialize");
|
||||||
|
let pid_file = codex_home.path().join("mcp.pid");
|
||||||
|
std::fs::write(&barrier_file, "ready")?;
|
||||||
|
let config_path = codex_home.path().join("config.toml");
|
||||||
|
let mut config_toml = std::fs::read_to_string(&config_path)?;
|
||||||
|
config_toml.push_str(&format!(
|
||||||
|
r#"
|
||||||
|
[mcp_servers.cached-stdio]
|
||||||
|
command = {}
|
||||||
|
enabled_tools = ["echo"]
|
||||||
|
startup_timeout_sec = 10
|
||||||
|
|
||||||
|
[mcp_servers.cached-stdio.env]
|
||||||
|
MCP_TEST_DYNAMIC_SERVER_METADATA = "1"
|
||||||
|
MCP_TEST_INITIALIZE_BARRIER_FILE = {}
|
||||||
|
MCP_TEST_PID_FILE = {}
|
||||||
|
"#,
|
||||||
|
toml::Value::String(stdio_server_bin()?),
|
||||||
|
toml::Value::String(barrier_file.to_string_lossy().into_owned()),
|
||||||
|
toml::Value::String(pid_file.to_string_lossy().into_owned()),
|
||||||
|
));
|
||||||
|
std::fs::write(config_path, config_toml)?;
|
||||||
|
|
||||||
|
let mut mcp = TestAppServer::builder()
|
||||||
|
.with_codex_home(codex_home.path())
|
||||||
|
.without_auto_env()
|
||||||
|
.build()
|
||||||
|
.await?;
|
||||||
|
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||||
|
|
||||||
|
let first_request_id = mcp
|
||||||
|
.send_list_mcp_server_status_request(ListMcpServerStatusParams {
|
||||||
|
cursor: None,
|
||||||
|
limit: None,
|
||||||
|
detail: Some(McpServerStatusDetail::ToolsAndAuthOnly),
|
||||||
|
thread_id: None,
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
let first_response = timeout(
|
||||||
|
DEFAULT_READ_TIMEOUT,
|
||||||
|
mcp.read_stream_until_response_message(RequestId::Integer(first_request_id)),
|
||||||
|
)
|
||||||
|
.await??;
|
||||||
|
let first_response: ListMcpServerStatusResponse = to_response(first_response)?;
|
||||||
|
let first_pid = wait_for_new_pid(&pid_file, /*previous_pid*/ None).await?;
|
||||||
|
assert_dynamic_status(&first_response, &format!("rmcp-test-process-{first_pid}"));
|
||||||
|
|
||||||
|
std::fs::remove_file(&barrier_file)?;
|
||||||
|
let second_request_id = mcp
|
||||||
|
.send_list_mcp_server_status_request(ListMcpServerStatusParams {
|
||||||
|
cursor: None,
|
||||||
|
limit: None,
|
||||||
|
detail: Some(McpServerStatusDetail::ToolsAndAuthOnly),
|
||||||
|
thread_id: None,
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
let second_pid = wait_for_new_pid(&pid_file, Some(&first_pid)).await?;
|
||||||
|
assert!(
|
||||||
|
timeout(
|
||||||
|
Duration::from_millis(200),
|
||||||
|
mcp.read_stream_until_response_message(RequestId::Integer(second_request_id)),
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.is_err(),
|
||||||
|
"status/list should wait for the live stdio server to initialize"
|
||||||
|
);
|
||||||
|
|
||||||
|
std::fs::write(&barrier_file, "ready")?;
|
||||||
|
let second_response = timeout(
|
||||||
|
DEFAULT_READ_TIMEOUT,
|
||||||
|
mcp.read_stream_until_response_message(RequestId::Integer(second_request_id)),
|
||||||
|
)
|
||||||
|
.await??;
|
||||||
|
let second_response: ListMcpServerStatusResponse = to_response(second_response)?;
|
||||||
|
assert_dynamic_status(&second_response, &format!("rmcp-test-process-{second_pid}"));
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> {
|
async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> {
|
||||||
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await;
|
||||||
|
|||||||
@@ -19,6 +19,7 @@ use crate::tools::normalize_tools_for_model_with_prefix;
|
|||||||
use codex_config::AppToolApproval;
|
use codex_config::AppToolApproval;
|
||||||
use codex_config::Constrained;
|
use codex_config::Constrained;
|
||||||
use codex_config::McpServerConfig;
|
use codex_config::McpServerConfig;
|
||||||
|
use codex_config::McpServerEnvVar;
|
||||||
use codex_config::McpServerToolConfig;
|
use codex_config::McpServerToolConfig;
|
||||||
use codex_config::types::AuthKeyringBackendKind;
|
use codex_config::types::AuthKeyringBackendKind;
|
||||||
use codex_config::types::OAuthCredentialsStoreMode;
|
use codex_config::types::OAuthCredentialsStoreMode;
|
||||||
@@ -816,6 +817,36 @@ async fn tool_catalog_cache_sanitizes_tools_and_tracks_environment_generation()
|
|||||||
assert!(!first_context.has_tools());
|
assert!(!first_context.has_tools());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn tool_catalog_cache_bypasses_remote_sourced_environment_variables() {
|
||||||
|
let cache = McpToolCatalogCache::default();
|
||||||
|
let runtime_context = McpRuntimeContext::new(
|
||||||
|
Arc::new(EnvironmentManager::without_environments()),
|
||||||
|
PathBuf::from("/tmp"),
|
||||||
|
);
|
||||||
|
let config: McpServerConfig = serde_json::from_value(serde_json::json!({
|
||||||
|
"command": "docs-mcp",
|
||||||
|
"env_vars": [McpServerEnvVar::Config {
|
||||||
|
name: "DOCS_TOKEN".to_string(),
|
||||||
|
source: Some("remote".to_string()),
|
||||||
|
}],
|
||||||
|
}))
|
||||||
|
.expect("MCP config");
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
cache
|
||||||
|
.context(
|
||||||
|
"docs",
|
||||||
|
&config,
|
||||||
|
&runtime_context,
|
||||||
|
/*resolved_environment*/ None,
|
||||||
|
&ElicitationCapability::default(),
|
||||||
|
/*supports_openai_form_elicitation*/ false,
|
||||||
|
)
|
||||||
|
.is_none()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn list_available_server_infos_uses_cache_while_client_is_pending() {
|
async fn list_available_server_infos_uses_cache_while_client_is_pending() {
|
||||||
let pending_client = futures::future::pending::<Result<ManagedClient, StartupOutcomeError>>()
|
let pending_client = futures::future::pending::<Result<ManagedClient, StartupOutcomeError>>()
|
||||||
|
|||||||
@@ -267,6 +267,14 @@ async fn regular_mcp_definition_cache_preserves_live_session_state() -> anyhow::
|
|||||||
output.contains(&expected_error),
|
output.contains(&expected_error),
|
||||||
"model-visible tool output should contain the live visibility error: {output}"
|
"model-visible tool output should contain the live visibility error: {output}"
|
||||||
);
|
);
|
||||||
|
let output = cached_done_response
|
||||||
|
.single_request()
|
||||||
|
.function_call_output_text("cached-call")
|
||||||
|
.expect("successful tool output should be returned to the model");
|
||||||
|
assert!(
|
||||||
|
output.contains(&second_process),
|
||||||
|
"model-visible tool output should come from the live server: {output}"
|
||||||
|
);
|
||||||
|
|
||||||
second_thread.shutdown_and_wait().await?;
|
second_thread.shutdown_and_wait().await?;
|
||||||
responses_server.verify().await;
|
responses_server.verify().await;
|
||||||
|
|||||||
Reference in new issue
Block a user