From 85c1500569f4d72a06c4aac173ec4b59a85a05b3 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Mon, 27 Apr 2026 12:09:02 -0700 Subject: [PATCH] fix: filter dynamic deferred tools from model_visible_specs (#19771) fixes #19486 ### Problem Right now dynamic deferred tools are filtered at normal-turn prompt building time, rather than upstream while building the `ToolRouter` itself. This causes issues because dynamic deferred tools are then wrongly included in the router's `model_visible_specs`, which is what the compaction request-building flow relies on. ### Fix Move the dynamic deferred tool filtering to `ToolRouter` creation time to solve this problem for every request that relies on `ToolRouter` for `model_visible_specs`, which solves the issue generically. ### Tests Added unit + integration tests to ensure dynamic deferred tools are omitted from `model_visible_specs` and compaction request respectively. Tested against live `/compact` endpoint; raw deferred dynamic tools without `tool_search` returned `400` (current bug), while the filtered payload (this fix) returns `200`. --- codex-rs/core/src/session/turn.rs | 49 +------ codex-rs/core/src/tools/router.rs | 70 +++++++--- codex-rs/core/src/tools/router_tests.rs | 88 ++++++++++++ codex-rs/core/tests/suite/compact_remote.rs | 143 ++++++++++++++++++++ codex-rs/core/tests/suite/search_tool.rs | 12 +- 5 files changed, 293 insertions(+), 69 deletions(-) diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 6383b892e..ff3d37912 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -95,9 +95,7 @@ use codex_protocol::protocol::ReasoningRawContentDeltaEvent; use codex_protocol::protocol::TurnDiffEvent; use codex_protocol::protocol::WarningEvent; use codex_protocol::user_input::UserInput; -use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; -use codex_tools::ToolSpec; use codex_tools::filter_tool_suggest_discoverable_tools_for_client; use codex_utils_stream_parser::AssistantTextChunk; use codex_utils_stream_parser::AssistantTextStreamParser; @@ -946,25 +944,9 @@ pub(crate) fn build_prompt( turn_context: &TurnContext, base_instructions: BaseInstructions, ) -> Prompt { - let deferred_dynamic_tools = turn_context - .dynamic_tools - .iter() - .filter(|tool| tool.defer_loading) - .map(|tool| ToolName::new(tool.namespace.clone(), tool.name.clone())) - .collect::>(); - let tools = if deferred_dynamic_tools.is_empty() { - router.model_visible_specs() - } else { - router - .model_visible_specs() - .into_iter() - .filter_map(|spec| filter_deferred_dynamic_tool_spec(spec, &deferred_dynamic_tools)) - .collect() - }; - Prompt { input, - tools, + tools: router.model_visible_specs(), parallel_tool_calls: turn_context.model_info.supports_parallel_tool_calls, base_instructions, personality: turn_context.personality, @@ -975,35 +957,6 @@ pub(crate) fn build_prompt( } } -fn filter_deferred_dynamic_tool_spec( - spec: ToolSpec, - deferred_dynamic_tools: &HashSet, -) -> Option { - match spec { - ToolSpec::Function(tool) => { - if deferred_dynamic_tools.contains(&ToolName::plain(tool.name.as_str())) { - None - } else { - Some(ToolSpec::Function(tool)) - } - } - ToolSpec::Namespace(mut namespace) => { - let namespace_name = namespace.name.clone(); - namespace.tools.retain(|tool| match tool { - ResponsesApiNamespaceTool::Function(tool) => !deferred_dynamic_tools.contains( - &ToolName::namespaced(namespace_name.as_str(), tool.name.as_str()), - ), - }); - if namespace.tools.is_empty() { - None - } else { - Some(ToolSpec::Namespace(namespace)) - } - } - spec => Some(spec), - } -} - #[allow(clippy::too_many_arguments)] #[instrument(level = "trace", skip_all, diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 1f0dec692..aeba3b055 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -71,23 +71,26 @@ impl ToolRouter { dynamic_tools, ); let (specs, registry) = builder.build(); - let model_visible_specs = if config.code_mode_only_enabled { - specs - .iter() - .filter_map(|configured_tool| { - if !codex_code_mode::is_code_mode_nested_tool(configured_tool.name()) { - Some(configured_tool.spec.clone()) - } else { - None - } - }) - .collect() - } else { - specs - .iter() - .map(|configured_tool| configured_tool.spec.clone()) - .collect() - }; + let deferred_dynamic_tools = dynamic_tools + .iter() + .filter(|tool| tool.defer_loading) + .map(|tool| ToolName::new(tool.namespace.clone(), tool.name.clone())) + .collect::>(); + let model_visible_specs = specs + .iter() + .filter_map(|configured_tool| { + if config.code_mode_only_enabled + && codex_code_mode::is_code_mode_nested_tool(configured_tool.name()) + { + return None; + } + + filter_deferred_dynamic_tool_spec( + configured_tool.spec.clone(), + &deferred_dynamic_tools, + ) + }) + .collect(); Self { registry, @@ -293,6 +296,39 @@ impl ToolRouter { self.registry.dispatch_any(invocation).await } } + +fn filter_deferred_dynamic_tool_spec( + spec: ToolSpec, + deferred_dynamic_tools: &HashSet, +) -> Option { + if deferred_dynamic_tools.is_empty() { + return Some(spec); + } + + match spec { + ToolSpec::Function(tool) => { + if deferred_dynamic_tools.contains(&ToolName::plain(tool.name.as_str())) { + None + } else { + Some(ToolSpec::Function(tool)) + } + } + ToolSpec::Namespace(mut namespace) => { + let namespace_name = namespace.name.clone(); + namespace.tools.retain(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => !deferred_dynamic_tools.contains( + &ToolName::namespaced(namespace_name.as_str(), tool.name.as_str()), + ), + }); + if namespace.tools.is_empty() { + None + } else { + Some(ToolSpec::Namespace(namespace)) + } + } + spec => Some(spec), + } +} #[cfg(test)] #[path = "router_tests.rs"] mod tests; diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index e8c098c41..ac859d9aa 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -3,8 +3,13 @@ use std::sync::Arc; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolPayload; +use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_protocol::models::ResponseItem; +use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; +use codex_tools::ToolSpec; +use pretty_assertions::assert_eq; +use serde_json::json; use super::ToolCall; use super::ToolRouter; @@ -133,3 +138,86 @@ async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> Ok(()) } + +#[tokio::test] +async fn model_visible_specs_filter_deferred_dynamic_tools() -> anyhow::Result<()> { + let (_, turn) = make_session_and_context().await; + let hidden_tool = "hidden_dynamic_tool"; + let visible_tool = "visible_dynamic_tool"; + let dynamic_tools = vec![ + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: hidden_tool.to_string(), + description: "Hidden until discovered.".to_string(), + input_schema: json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }), + defer_loading: true, + }, + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: visible_tool.to_string(), + description: "Visible immediately.".to_string(), + input_schema: json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }), + defer_loading: false, + }, + ]; + + let router = ToolRouter::from_config( + &turn.tools_config, + ToolRouterParams { + deferred_mcp_tools: None, + mcp_tools: None, + unavailable_called_tools: Vec::new(), + parallel_mcp_server_names: HashSet::new(), + discoverable_tools: None, + dynamic_tools: &dynamic_tools, + }, + ); + + assert!( + router + .find_spec(&ToolName::namespaced("codex_app", hidden_tool)) + .is_some() + ); + assert_eq!( + namespace_function_names(&router.specs(), "codex_app"), + vec![hidden_tool.to_string(), visible_tool.to_string()] + ); + assert_eq!( + namespace_function_names(&router.model_visible_specs(), "codex_app"), + vec![visible_tool.to_string()] + ); + + Ok(()) +} + +fn namespace_function_names(specs: &[ToolSpec], namespace_name: &str) -> Vec { + specs + .iter() + .find_map(|spec| match spec { + ToolSpec::Namespace(namespace) if namespace.name == namespace_name => Some( + namespace + .tools + .iter() + .map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => tool.name.clone(), + }) + .collect(), + ), + ToolSpec::Function(_) + | ToolSpec::Freeform(_) + | ToolSpec::ToolSearch { .. } + | ToolSpec::LocalShell {} + | ToolSpec::ImageGeneration { .. } + | ToolSpec::WebSearch { .. } + | ToolSpec::Namespace(_) => None, + }) + .unwrap_or_default() +} diff --git a/codex-rs/core/tests/suite/compact_remote.rs b/codex-rs/core/tests/suite/compact_remote.rs index b70cc6f38..24e86881b 100644 --- a/codex-rs/core/tests/suite/compact_remote.rs +++ b/codex-rs/core/tests/suite/compact_remote.rs @@ -6,6 +6,7 @@ use std::path::PathBuf; use anyhow::Result; use codex_core::compact::SUMMARY_PREFIX; use codex_login::CodexAuth; +use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_protocol::items::TurnItem; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; @@ -36,6 +37,7 @@ use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; use core_test_support::wait_for_event_with_timeout; use pretty_assertions::assert_eq; +use serde_json::Value; use serde_json::json; use tokio::time::Duration; use wiremock::ResponseTemplate; @@ -55,6 +57,53 @@ fn estimate_compact_payload_tokens(request: &responses::ResponsesRequest) -> i64 .saturating_add(approx_token_count(&request.instructions_text())) } +fn assert_tools_payload_does_not_defer(body: &Value) { + if let Some(tools) = body.get("tools") { + assert!( + !contains_defer_loading(tools), + "model-visible tools should not include deferred declarations: {tools}" + ); + } +} + +fn namespace_child_tool_names(body: &Value, namespace: &str) -> Vec { + body.get("tools") + .and_then(Value::as_array) + .and_then(|tools| { + tools.iter().find_map(|tool| { + if tool.get("type").and_then(Value::as_str) == Some("namespace") + && tool.get("name").and_then(Value::as_str) == Some(namespace) + { + tool.get("tools").and_then(Value::as_array).map(|children| { + children + .iter() + .filter_map(|child| { + child + .get("name") + .and_then(Value::as_str) + .map(str::to_string) + }) + .collect() + }) + } else { + None + } + }) + }) + .unwrap_or_default() +} + +fn contains_defer_loading(value: &Value) -> bool { + match value { + Value::Object(map) => { + map.get("defer_loading").and_then(Value::as_bool) == Some(true) + || map.values().any(contains_defer_loading) + } + Value::Array(values) => values.iter().any(contains_defer_loading), + Value::Null | Value::Bool(_) | Value::Number(_) | Value::String(_) => false, + } +} + const PRETURN_CONTEXT_DIFF_CWD: &str = "/tmp/PRETURN_CONTEXT_DIFF_CWD"; const DUMMY_FUNCTION_NAME: &str = "test_tool"; const REMOTE_COMPACT_TURN_COMPLETE_TIMEOUT: Duration = Duration::from_secs(30); @@ -358,6 +407,100 @@ async fn remote_compact_replaces_history_for_followups() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_compact_filters_deferred_dynamic_tools() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let mut builder = test_codex().with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()); + let mut test = builder.build(&server).await?; + let hidden_tool = "hidden_dynamic_tool"; + let visible_tool = "visible_dynamic_tool"; + let input_schema = json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }); + let dynamic_tools = vec![ + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: hidden_tool.to_string(), + description: "Hidden until discovered.".to_string(), + input_schema: input_schema.clone(), + defer_loading: true, + }, + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: visible_tool.to_string(), + description: "Visible immediately.".to_string(), + input_schema, + defer_loading: false, + }, + ]; + let new_thread = test + .thread_manager + .start_thread_with_tools( + test.config.clone(), + dynamic_tools, + /*persist_extended_history*/ false, + ) + .await?; + test.codex = new_thread.thread; + test.session_configured = new_thread.session_configured; + let codex = test.codex.clone(); + + let responses_mock = mount_sse_once( + &server, + sse(vec![ + responses::ev_assistant_message("m1", "FIRST_REMOTE_REPLY"), + responses::ev_completed("resp-1"), + ]), + ) + .await; + let compact_mock = responses::mount_compact_json_once( + &server, + serde_json::json!({ + "output": compacted_summary_only_output("compact summary"), + }), + ) + .await; + + codex + .submit(Op::UserInput { + environments: None, + items: vec![UserInput::Text { + text: "hello remote compact".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + }) + .await?; + wait_for_turn_complete(&codex).await; + + codex.submit(Op::Compact).await?; + wait_for_turn_complete(&codex).await; + + let first_response_body = responses_mock.single_request().body_json(); + let compact_body = compact_mock.single_request().body_json(); + assert_eq!( + compact_body["tools"], first_response_body["tools"], + "compact requests should send the same model-visible tools payload as /v1/responses" + ); + assert_tools_payload_does_not_defer(&first_response_body); + assert_tools_payload_does_not_defer(&compact_body); + assert_eq!( + namespace_child_tool_names(&first_response_body, "codex_app"), + vec![visible_tool.to_string()] + ); + assert_eq!( + namespace_child_tool_names(&compact_body, "codex_app"), + vec![visible_tool.to_string()] + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn remote_compact_runs_automatically() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index fa2eca9b2..1794f2fd3 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -605,7 +605,8 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - "apps tools/call should include turn metadata turn_id: {apps_tool_call:?}" ); - let first_request_tools = tool_names(&requests[0].body_json()); + let first_request_body = requests[0].body_json(); + let first_request_tools = tool_names(&first_request_body); assert!( first_request_tools .iter() @@ -823,7 +824,8 @@ async fn tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call() - let requests = mock.requests(); assert_eq!(requests.len(), 3); - let first_request_tools = tool_names(&requests[0].body_json()); + let first_request_body = requests[0].body_json(); + let first_request_tools = tool_names(&first_request_body); assert!( first_request_tools .iter() @@ -853,7 +855,8 @@ async fn tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call() - })] ); - let second_request_tools = tool_names(&requests[1].body_json()); + let second_request_body = requests[1].body_json(); + let second_request_tools = tool_names(&second_request_body); assert!( !second_request_tools.iter().any(|name| name == tool_name), "follow-up request should rely on tool_search_output history, not tool injection: {second_request_tools:?}" @@ -870,7 +873,8 @@ async fn tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call() - FunctionCallOutputPayload::from_text("dynamic-search-ok".to_string()) ); - let third_request_tools = tool_names(&requests[2].body_json()); + let third_request_body = requests[2].body_json(); + let third_request_tools = tool_names(&third_request_body); assert!( !third_request_tools.iter().any(|name| name == tool_name), "post-tool follow-up should rely on tool_search_output history, not tool injection: {third_request_tools:?}"