diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index b44922710..325d65745 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -144,7 +144,6 @@ mod tools; pub(crate) mod turn_diff_tracker; mod turn_metadata; mod turn_timing; -mod unavailable_tool; pub use rollout::ARCHIVED_SESSIONS_SUBDIR; pub use rollout::Cursor; pub use rollout::EventPersistenceMode; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 4b847295b..43309ab0e 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -542,7 +542,6 @@ fn test_tool_runtime(session: Arc, turn_context: Arc) -> T crate::tools::router::ToolRouterParams { mcp_tools: None, deferred_mcp_tools: None, - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: turn_context.dynamic_tools.as_slice(), @@ -8607,7 +8606,6 @@ async fn fatal_tool_error_stops_turn_and_reports_error() { crate::tools::router::ToolRouterParams { deferred_mcp_tools, mcp_tools: Some(tools), - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: turn_context.dynamic_tools.as_slice(), diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 87334d1ac..400966420 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -58,7 +58,6 @@ use crate::tools::router::ToolRouterParams; use crate::tools::router::extension_tool_bundles; use crate::turn_diff_tracker::TurnDiffTracker; use crate::turn_timing::record_turn_ttft_metric; -use crate::unavailable_tool::collect_unavailable_called_tools; use crate::util::backoff; use crate::util::error_or_panic; use codex_analytics::AppInvocation; @@ -1245,27 +1244,11 @@ pub(crate) async fn built_tools( ); let mcp_tools = has_mcp_servers.then_some(mcp_tool_exposure.direct_tools); let deferred_mcp_tools = mcp_tool_exposure.deferred_tools; - let unavailable_called_tools = if turn_context - .config - .features - .enabled(Feature::UnavailableDummyTools) - { - let exposed_tool_names = mcp_tools - .iter() - .chain(deferred_mcp_tools.iter()) - .flat_map(|tools| tools.iter().map(codex_mcp::ToolInfo::canonical_tool_name)) - .collect::>(); - collect_unavailable_called_tools(input, &exposed_tool_names) - } else { - Vec::new() - }; - Ok(Arc::new(ToolRouter::from_config( &turn_context.tools_config, ToolRouterParams { mcp_tools, deferred_mcp_tools, - unavailable_called_tools, discoverable_tools, extension_tool_bundles: extension_tool_bundles(sess), dynamic_tools: turn_context.dynamic_tools.as_slice(), diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index d689b0cf3..712ed0f1a 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -26,7 +26,6 @@ mod test_sync; pub(crate) mod test_sync_spec; mod tool_search; pub(crate) mod tool_search_spec; -mod unavailable_tool; pub(crate) mod unified_exec; mod view_image; pub(crate) mod view_image_spec; @@ -70,8 +69,6 @@ pub(crate) use shell::ShellCommandHandlerOptions; pub use shell::ShellHandler; pub use test_sync::TestSyncHandler; pub use tool_search::ToolSearchHandler; -pub use unavailable_tool::UnavailableToolHandler; -pub(crate) use unavailable_tool::unavailable_tool_message; pub use unified_exec::ExecCommandHandler; pub(crate) use unified_exec::ExecCommandHandlerOptions; pub use unified_exec::WriteStdinHandler; diff --git a/codex-rs/core/src/tools/handlers/unavailable_tool.rs b/codex-rs/core/src/tools/handlers/unavailable_tool.rs deleted file mode 100644 index e297374a7..000000000 --- a/codex-rs/core/src/tools/handlers/unavailable_tool.rs +++ /dev/null @@ -1,66 +0,0 @@ -use crate::function_tool::FunctionCallError; -use crate::tools::context::FunctionToolOutput; -use crate::tools::context::ToolInvocation; -use crate::tools::context::ToolPayload; -use crate::tools::registry::ToolHandler; -use codex_tools::ToolName; -use codex_tools::ToolSpec; - -pub struct UnavailableToolHandler { - tool_name: ToolName, - spec: Option, -} - -impl UnavailableToolHandler { - pub fn new(tool_name: ToolName, spec: ToolSpec) -> Self { - Self { - tool_name, - spec: Some(spec), - } - } - - pub fn without_spec(tool_name: ToolName) -> Self { - Self { - tool_name, - spec: None, - } - } -} - -pub(crate) fn unavailable_tool_message( - tool_name: impl std::fmt::Display, - next_step: &str, -) -> String { - format!( - "Tool `{tool_name}` is not currently available. It appeared in earlier tool calls in this conversation, but its implementation is not available in the current request. {next_step}" - ) -} - -impl ToolHandler for UnavailableToolHandler { - type Output = FunctionToolOutput; - - fn tool_name(&self) -> ToolName { - self.tool_name.clone() - } - - fn spec(&self) -> Option { - self.spec.clone() - } - - async fn handle(&self, invocation: ToolInvocation) -> Result { - let ToolInvocation { payload, .. } = invocation; - - match payload { - ToolPayload::Function { .. } => Ok(FunctionToolOutput::from_text( - unavailable_tool_message( - &self.tool_name, - "Retry after the tool becomes available or ask the user to re-enable it.", - ), - Some(false), - )), - _ => Err(FunctionCallError::RespondToModel( - "unavailable tool handler received unsupported payload".to_string(), - )), - } - } -} diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index c981e0059..17774aafb 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -623,10 +623,6 @@ impl ToolRegistryBuilder { self.handlers.insert(tool_name, handler); } - pub(crate) fn specs(&self) -> &[ToolSpec] { - &self.specs - } - pub fn build(self) -> (Vec, ToolRegistry) { let registry = ToolRegistry::new(self.handlers); (self.specs, registry) diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 4bc366bf9..c8a4679d2 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -43,7 +43,6 @@ pub struct ToolRouter { pub(crate) struct ToolRouterParams<'a> { pub(crate) mcp_tools: Option>, pub(crate) deferred_mcp_tools: Option>, - pub(crate) unavailable_called_tools: Vec, pub(crate) discoverable_tools: Option>, pub(crate) extension_tool_bundles: Vec, pub(crate) dynamic_tools: &'a [DynamicToolSpec], @@ -54,7 +53,6 @@ impl ToolRouter { let ToolRouterParams { mcp_tools, deferred_mcp_tools, - unavailable_called_tools, discoverable_tools, extension_tool_bundles, dynamic_tools, @@ -63,7 +61,6 @@ impl ToolRouter { config, mcp_tools, deferred_mcp_tools, - unavailable_called_tools, discoverable_tools, &extension_tool_bundles, dynamic_tools, diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index 4d6119cd5..f662c1909 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -97,7 +97,6 @@ async fn parallel_support_does_not_match_namespaced_local_tool_names() -> anyhow ToolRouterParams { deferred_mcp_tools: None, mcp_tools: Some(mcp_tools), - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: turn.dynamic_tools.as_slice(), @@ -177,7 +176,6 @@ async fn mcp_parallel_support_uses_handler_data() -> anyhow::Result<()> { "query_with_delay", ), ]), - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: turn.dynamic_tools.as_slice(), @@ -213,7 +211,6 @@ async fn tools_without_handlers_do_not_support_parallel() -> anyhow::Result<()> ToolRouterParams { deferred_mcp_tools: None, mcp_tools: None, - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: turn.dynamic_tools.as_slice(), @@ -266,7 +263,6 @@ async fn specs_filter_deferred_dynamic_tools() -> anyhow::Result<()> { ToolRouterParams { deferred_mcp_tools: None, mcp_tools: None, - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: &dynamic_tools, @@ -323,7 +319,6 @@ async fn extension_tool_bundles_are_model_visible_and_dispatchable() -> anyhow:: ToolRouterParams { deferred_mcp_tools: None, mcp_tools: None, - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: extension_tool_bundles(&session), dynamic_tools: turn.dynamic_tools.as_slice(), diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 7bf3a095d..46061adfc 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -1,6 +1,5 @@ use crate::shell::Shell; use crate::shell::ShellType; -use crate::tools::flat_tool_name; use crate::tools::handlers::multi_agents_common::DEFAULT_WAIT_TIMEOUT_MS; use crate::tools::handlers::multi_agents_common::MAX_WAIT_TIMEOUT_MS; use crate::tools::handlers::multi_agents_common::MIN_WAIT_TIMEOUT_MS; @@ -11,15 +10,9 @@ use crate::tools::spec_plan_types::ToolRegistryBuildParams; use codex_mcp::ToolInfo; use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_tool_api::ToolBundle as ExtensionToolBundle; -use codex_tools::AdditionalProperties; use codex_tools::DiscoverableTool; -use codex_tools::JsonSchema; -use codex_tools::ResponsesApiTool; -use codex_tools::ToolName; use codex_tools::ToolUserShellType; use codex_tools::ToolsConfig; -use std::collections::HashSet; -use std::sync::Arc; pub(crate) fn tool_user_shell_type(user_shell: &Shell) -> ToolUserShellType { match user_shell.shell_type { @@ -35,14 +28,10 @@ pub(crate) fn build_specs_with_discoverable_tools( config: &ToolsConfig, mcp_tools: Option>, deferred_mcp_tools: Option>, - unavailable_called_tools: Vec, discoverable_tools: Option>, extension_tool_bundles: &[ExtensionToolBundle], dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { - use crate::tools::handlers::UnavailableToolHandler; - use crate::tools::handlers::unavailable_tool_message; - let default_agent_type_description = crate::agent::role::spawn_tool_spec::build(&std::collections::BTreeMap::new()); let min_wait_timeout_ms = if config.multi_agent_v2 { @@ -55,7 +44,7 @@ pub(crate) fn build_specs_with_discoverable_tools( }; let default_wait_timeout_ms = DEFAULT_WAIT_TIMEOUT_MS.clamp(min_wait_timeout_ms, MAX_WAIT_TIMEOUT_MS); - let mut builder = build_tool_registry_builder( + build_tool_registry_builder( config, ToolRegistryBuildParams { mcp_tools: mcp_tools.as_deref(), @@ -70,42 +59,7 @@ pub(crate) fn build_specs_with_discoverable_tools( max_timeout_ms: MAX_WAIT_TIMEOUT_MS, }, }, - ); - let mut existing_spec_names = builder - .specs() - .iter() - .map(|configured_tool| configured_tool.name().to_string()) - .collect::>(); - - for unavailable_tool in unavailable_called_tools { - let tool_name = flat_tool_name(&unavailable_tool).into_owned(); - if existing_spec_names.insert(tool_name.clone()) { - let spec = codex_tools::ToolSpec::Function(ResponsesApiTool { - name: tool_name.clone(), - description: unavailable_tool_message( - &tool_name, - "Calling this placeholder returns an error explaining that the tool is unavailable.", - ), - strict: false, - parameters: JsonSchema::object( - Default::default(), - /*required*/ None, - Some(AdditionalProperties::Boolean(false)), - ), - output_schema: None, - defer_loading: None, - }); - builder.register_handler(Arc::new(UnavailableToolHandler::new( - unavailable_tool, - spec, - ))); - } else { - builder.register_handler(Arc::new(UnavailableToolHandler::without_spec( - unavailable_tool, - ))); - } - } - builder + ) } #[cfg(test)] diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index b4ef8f994..36eb140ae 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -16,7 +16,6 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::SessionSource; -use codex_tools::AdditionalProperties; use codex_tools::DiscoverableTool; use codex_tools::JsonSchema; use codex_tools::REQUEST_PLUGIN_INSTALL_TOOL_NAME; @@ -270,28 +269,11 @@ fn build_specs( mcp_tools: Option>, deferred_mcp_tools: Option>, dynamic_tools: &[DynamicToolSpec], -) -> ToolRegistryBuilder { - build_specs_with_unavailable_tools( - config, - mcp_tools, - deferred_mcp_tools, - Vec::new(), - dynamic_tools, - ) -} - -fn build_specs_with_unavailable_tools( - config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, - unavailable_called_tools: Vec, - dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { build_specs_with_discoverable_tools( config, mcp_tools, deferred_mcp_tools, - unavailable_called_tools, /*discoverable_tools*/ None, /*extension_tool_bundles*/ &[], dynamic_tools, @@ -352,7 +334,6 @@ async fn assert_model_tools( ToolRouterParams { mcp_tools: None, deferred_mcp_tools: None, - unavailable_called_tools: Vec::new(), discoverable_tools: None, extension_tool_bundles: Vec::new(), dynamic_tools: &[], @@ -822,7 +803,6 @@ async fn request_plugin_install_requires_apps_and_plugins_features() { &tools_config, /*mcp_tools*/ None, /*deferred_mcp_tools*/ None, - Vec::new(), discoverable_tools.clone(), /*extension_tool_bundles*/ &[], &[], @@ -1032,55 +1012,6 @@ async fn direct_mcp_tools_register_namespaced_handlers() { assert!(!registry.has_handler(&ToolName::plain("mcp__test_server__echo"))); } -#[tokio::test] -async fn unavailable_mcp_tools_are_exposed_as_dummy_function_tools() { - let config = test_config().await; - let model_info = construct_model_info_offline("gpt-5.4", &config); - let mut features = Features::with_defaults(); - features.enable(Feature::UnifiedExec); - let available_models = Vec::new(); - let tools_config = ToolsConfig::new(&ToolsConfigParams { - model_info: &model_info, - available_models: &available_models, - features: &features, - image_generation_tool_auth_allowed: true, - web_search_mode: Some(WebSearchMode::Cached), - session_source: SessionSource::Cli, - permission_profile: &PermissionProfile::Disabled, - windows_sandbox_level: WindowsSandboxLevel::Disabled, - }); - - let unavailable_tool = ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"); - let (tools, registry) = build_specs_with_unavailable_tools( - &tools_config, - /*mcp_tools*/ None, - /*deferred_mcp_tools*/ None, - vec![unavailable_tool], - &[], - ) - .build(); - - let tool = find_tool(&tools, "mcp__codex_apps__calendar_create_event"); - let ToolSpec::Function(ResponsesApiTool { - description, - parameters, - .. - }) = tool - else { - panic!("unavailable MCP tool should be exposed as a function tool"); - }; - assert!(description.contains("not currently available")); - assert_eq!( - parameters.additional_properties, - Some(AdditionalProperties::Boolean(false)) - ); - assert!(registry.has_handler(&ToolName::namespaced( - "mcp__codex_apps__calendar", - "_create_event" - ))); - assert!(!registry.has_handler(&ToolName::plain("mcp__codex_apps__calendar_create_event"))); -} - #[tokio::test] async fn test_mcp_tool_property_missing_type_defaults_to_string() { let config = test_config().await; diff --git a/codex-rs/core/src/unavailable_tool.rs b/codex-rs/core/src/unavailable_tool.rs deleted file mode 100644 index 9f71facea..000000000 --- a/codex-rs/core/src/unavailable_tool.rs +++ /dev/null @@ -1,113 +0,0 @@ -use std::collections::BTreeMap; -use std::collections::HashSet; - -use crate::tools::flat_tool_name; -use codex_protocol::models::ResponseItem; -use codex_tools::ToolName; - -pub(crate) fn collect_unavailable_called_tools( - input: &[ResponseItem], - exposed_tool_names: &HashSet, -) -> Vec { - let mut unavailable_tools = BTreeMap::new(); - let exposed_flat_names = exposed_tool_names - .iter() - .map(flat_tool_name) - .collect::>(); - - for item in input { - let ResponseItem::FunctionCall { - name, namespace, .. - } = item - else { - continue; - }; - if !should_collect_unavailable_tool(name, namespace.as_deref()) { - continue; - } - - let tool_name = match namespace { - Some(namespace) => ToolName::namespaced(namespace.clone(), name.clone()), - None => ToolName::plain(name.clone()), - }; - let tool_name_flat = flat_tool_name(&tool_name).into_owned(); - if exposed_flat_names.contains(tool_name_flat.as_str()) { - continue; - } - - unavailable_tools - .entry(tool_name_flat) - .or_insert_with(|| tool_name); - } - - unavailable_tools.into_values().collect() -} - -fn should_collect_unavailable_tool(name: &str, namespace: Option<&str>) -> bool { - namespace.is_some_and(|namespace| namespace.starts_with("mcp__")) || name.starts_with("mcp__") -} - -#[cfg(test)] -mod tests { - use super::*; - use pretty_assertions::assert_eq; - - fn function_call(name: &str, namespace: Option<&str>) -> ResponseItem { - ResponseItem::FunctionCall { - id: None, - name: name.to_string(), - namespace: namespace.map(str::to_string), - arguments: "{}".to_string(), - call_id: format!("call-{name}"), - } - } - - #[test] - fn collect_unavailable_called_tools_detects_mcp_function_calls() { - let input = vec![ - function_call("shell", /*namespace*/ None), - function_call("mcp__server__lookup", /*namespace*/ None), - function_call("_create_event", Some("mcp__codex_apps__calendar")), - ]; - - let tools = collect_unavailable_called_tools(&input, &HashSet::new()); - - assert_eq!( - tools, - vec![ - ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"), - ToolName::plain("mcp__server__lookup"), - ] - ); - } - - #[test] - fn collect_unavailable_called_tools_skips_currently_available_tools() { - let exposed_tool_names = HashSet::from([ - ToolName::plain("mcp__server__lookup"), - ToolName::plain("mcp__server__search"), - ]); - let input = vec![ - function_call("mcp__server__lookup", /*namespace*/ None), - function_call("mcp__server__search", /*namespace*/ None), - function_call("mcp__server__missing", /*namespace*/ None), - ]; - - let tools = collect_unavailable_called_tools(&input, &exposed_tool_names); - - assert_eq!(tools, vec![ToolName::plain("mcp__server__missing")]); - } - - #[test] - fn collect_unavailable_called_tools_matches_exposed_display_names() { - let exposed_tool_names = HashSet::from([ToolName::namespaced("mcp__server__", "lookup")]); - let input = vec![function_call( - "mcp__server__lookup", - /*namespace*/ None, - )]; - - let tools = collect_unavailable_called_tools(&input, &exposed_tool_names); - - assert_eq!(tools, Vec::new()); - } -} diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index 8a0a2270a..b3cf2e3bc 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -24,6 +24,7 @@ use core_test_support::apps_test_server::CALENDAR_CREATE_EVENT_RESOURCE_URI; use core_test_support::responses::ResponsesRequest; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_response_created; use core_test_support::responses::ev_tool_search_call; use core_test_support::responses::mount_sse_once; @@ -1062,6 +1063,161 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn tool_search_surfaced_mcp_tool_errors_are_returned_to_model() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount_searchable(&server).await?; + let search_call_id = "tool-search-rmcp-echo"; + let tool_call_id = "rmcp-echo-error"; + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_tool_search_call( + search_call_id, + &json!({ + "query": "Echo back the provided message and include environment data.", + "limit": 8, + }), + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_function_call_with_namespace(tool_call_id, "mcp__rmcp__", "echo", "{}"), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let mut builder = + configured_builder(apps_server.chatgpt_base_url.clone()).with_config(move |config| { + config + .features + .enable(Feature::ToolSearchAlwaysDeferMcpTools) + .expect("test config should allow feature update"); + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "rmcp".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + experimental_environment: None, + enabled: true, + required: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: Some(vec!["echo".to_string()]), + disabled_tools: None, + scopes: None, + oauth_resource: None, + supports_parallel_tool_calls: false, + tools: HashMap::new(), + }, + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(Op::UserInput { + environments: None, + items: vec![UserInput::Text { + text: "Find the rmcp echo tool and call it.".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + }) + .await?; + + let EventMsg::McpToolCallEnd(end) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::McpToolCallEnd(_)) + }) + .await + else { + unreachable!("event guard guarantees McpToolCallEnd"); + }; + assert_eq!(end.call_id, tool_call_id); + assert!(!end.is_success()); + let tool_error = end + .result + .as_ref() + .expect_err("rmcp echo error should stay in the MCP result"); + assert!( + tool_error.contains("tool call error:") + && tool_error.contains("missing field") + && tool_error.contains("message"), + "MCP invocation should report the execution failure: {tool_error}" + ); + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = mock.requests(); + assert_eq!(requests.len(), 3); + + let first_request_tools = tool_names(&requests[0].body_json()); + assert!( + first_request_tools + .iter() + .any(|name| name == TOOL_SEARCH_TOOL_NAME), + "first request should advertise tool_search: {first_request_tools:?}" + ); + assert!( + !first_request_tools.iter().any(|name| name == "mcp__rmcp__"), + "deferred rmcp namespace should not be directly exposed before search: {first_request_tools:?}" + ); + + assert!( + tool_search_output_has_namespace_child(&requests[1], search_call_id, "mcp__rmcp__", "echo"), + "tool_search should return the rmcp echo tool" + ); + + let output = requests[2].function_call_output(tool_call_id); + let output_text = match output.get("output") { + Some(Value::String(text)) => text.clone(), + Some(Value::Array(items)) => items + .iter() + .filter_map(|item| item.get("text").and_then(Value::as_str)) + .collect::>() + .join("\n"), + other => panic!("unexpected MCP error output payload: {other:?}"), + }; + assert!( + output_text.contains("missing field") && output_text.contains("message"), + "MCP error output should be model visible: {output_text}" + ); + assert!( + !output_text.contains("unsupported call"), + "search-surfaced MCP calls should not fall through to unsupported call: {output_text}" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn tool_search_uses_non_app_mcp_server_instructions_as_namespace_description() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/tools.rs b/codex-rs/core/tests/suite/tools.rs index e69cf35da..737ea122d 100644 --- a/codex-rs/core/tests/suite/tools.rs +++ b/codex-rs/core/tests/suite/tools.rs @@ -1,16 +1,12 @@ #![cfg(not(target_os = "windows"))] #![allow(clippy::unwrap_used, clippy::expect_used)] -use std::collections::HashMap; use std::fs; -use std::sync::Arc; use std::time::Duration; use std::time::Instant; use anyhow::Context; use anyhow::Result; -use codex_config::types::McpServerConfig; -use codex_config::types::McpServerTransportConfig; use codex_core::sandboxing::SandboxPermissions; use codex_features::Feature; use codex_protocol::models::PermissionProfile; @@ -33,13 +29,10 @@ use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; use core_test_support::skip_if_sandbox; -use core_test_support::stdio_server_bin; use core_test_support::test_codex::test_codex; use regex_lite::Regex; use serde_json::Value; use serde_json::json; -use serial_test::serial; -use tempfile::TempDir; fn tool_names(body: &Value) -> Vec { body.get("tools") @@ -58,24 +51,6 @@ fn tool_names(body: &Value) -> Vec { .unwrap_or_default() } -fn ev_namespaced_function_call( - call_id: &str, - namespace: &str, - name: &str, - arguments: &str, -) -> Value { - json!({ - "type": "response.output_item.done", - "item": { - "type": "function_call", - "call_id": call_id, - "namespace": namespace, - "name": name, - "arguments": arguments, - } - }) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn empty_turn_environments_omits_environment_backed_tools() -> Result<()> { skip_if_no_network!(Ok(())); @@ -206,149 +181,6 @@ async fn custom_tool_unknown_returns_custom_output_error() -> Result<()> { Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -#[serial(mcp_test_value)] -async fn historical_unavailable_mcp_call_is_exposed_as_placeholder_tool() -> Result<()> { - skip_if_no_network!(Ok(())); - - let historical_call_id = "historical-mcp-call"; - let retry_call_id = "retry-mcp-call"; - let server_name = "rmcp"; - let unavailable_tool_namespace = "mcp__rmcp__"; - let unavailable_tool_name = "echo"; - let unavailable_tool_display_name = "mcp__rmcp__echo"; - let server = start_mock_server().await; - let rmcp_test_server_bin = match stdio_server_bin() { - Ok(bin) => bin, - Err(err) => { - eprintln!("test_stdio_server binary not available, skipping test: {err}"); - return Ok(()); - } - }; - let codex_home = Arc::new(TempDir::new()?); - let mut builder = test_codex() - .with_model("gpt-5.4") - .with_home(Arc::clone(&codex_home)) - .with_config(move |config| { - config - .features - .enable(Feature::UnavailableDummyTools) - .expect("unavailable dummy tools should be enabled for this test"); - let mut servers = config.mcp_servers.get().clone(); - servers.insert( - server_name.to_string(), - McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: rmcp_test_server_bin, - args: Vec::new(), - env: Some(HashMap::new()), - env_vars: Vec::new(), - cwd: None, - }, - experimental_environment: None, - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: Some(Duration::from_secs(10)), - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }, - ); - config - .mcp_servers - .set(servers) - .expect("test mcp servers should accept any configuration"); - }); - let test = builder.build(&server).await?; - - let first_turn_mock = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-1"), - ev_namespaced_function_call( - historical_call_id, - unavailable_tool_namespace, - unavailable_tool_name, - r#"{"message":"ping"}"#, - ), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "rmcp echo tool completed"), - ev_completed("resp-2"), - ]), - ], - ) - .await; - - test.submit_turn("call the rmcp echo tool").await?; - let rollout_path = test.codex.rollout_path().context("rollout path")?; - assert_eq!(first_turn_mock.requests().len(), 2); - drop(test); - - let retry_mock = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-3"), - ev_namespaced_function_call( - retry_call_id, - unavailable_tool_namespace, - unavailable_tool_name, - r#"{"message":"ping again"}"#, - ), - ev_completed("resp-3"), - ]), - sse(vec![ - ev_response_created("resp-4"), - ev_assistant_message("msg-2", "done"), - ev_completed("resp-4"), - ]), - ], - ) - .await; - - let mut resume_builder = test_codex().with_model("gpt-5.4").with_config(|config| { - config - .features - .enable(Feature::UnavailableDummyTools) - .expect("unavailable dummy tools should be enabled for this test"); - }); - let test = resume_builder - .resume(&server, codex_home, rollout_path) - .await?; - - test.submit_turn("retry the rmcp echo tool").await?; - - let requests = retry_mock.requests(); - assert_eq!(requests.len(), 2); - let first_request_tools = tool_names(&requests[0].body_json()); - assert!( - first_request_tools - .iter() - .any(|name| name == unavailable_tool_display_name), - "historical unavailable MCP call should add a placeholder tool; got {first_request_tools:?}" - ); - let output_text = requests[1] - .function_call_output_text(retry_call_id) - .context("placeholder tool output present")?; - assert!(output_text.contains("not currently available")); - assert!( - !output_text.contains("unsupported call"), - "placeholder handler should answer instead of falling back to unsupported call: {output_text}" - ); - - Ok(()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_escalated_permissions_rejected_then_ok() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 8af3033ae..e019178c2 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -162,7 +162,7 @@ pub enum Feature { ToolSearch, /// Always defer MCP tools behind tool_search instead of exposing small sets directly. ToolSearchAlwaysDeferMcpTools, - /// Expose placeholder tools for unavailable historical tool calls. + /// Removed compatibility flag for the deleted unavailable-tool placeholder backfill. UnavailableDummyTools, /// Enable discoverable tool suggestions for apps. ToolSuggest, @@ -958,8 +958,8 @@ pub const FEATURES: &[FeatureSpec] = &[ FeatureSpec { id: Feature::UnavailableDummyTools, key: "unavailable_dummy_tools", - stage: Stage::Stable, - default_enabled: true, + stage: Stage::Removed, + default_enabled: false, }, FeatureSpec { id: Feature::ToolSuggest,