mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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`.
This commit is contained in:
@@ -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::<HashSet<_>>();
|
||||
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<ToolName>,
|
||||
) -> Option<ToolSpec> {
|
||||
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,
|
||||
|
||||
@@ -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::<HashSet<_>>();
|
||||
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<ToolName>,
|
||||
) -> Option<ToolSpec> {
|
||||
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;
|
||||
|
||||
@@ -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<String> {
|
||||
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()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user