mirror of
https://github.com/openai/codex.git
synced 2026-06-01 19:02:59 +00:00
Remove unavailable MCP placeholder tool backfill (#22439)
## Why `UnavailableDummyTools` kept synthetic placeholder tools alive for historical tool calls whose backing MCP tool was no longer available. That path adds stale model-visible tool specs and special routing at the point where unavailable MCP calls should use ordinary current-tool handling. This removes the runtime backfill instead of preserving a second compatibility lane. ## Is it safe to remove? The unavailable tools were added in #17853 after a CS issue when a previously-called MCP tool failed to load and was omitted from the CS spec. Now that we have tool search, I think this is resolved: - API merges tools from previous TST output into effective tool set so theyre always in CS spec - if an MCP tool surfaced by TST later becomes unavailable, the model can still call it and it will just return model-visible error - both TST output and function call output are dropped on compaction so model will not remember old calls to MCP post compaction ## What changed - Delete unavailable-tool collection, placeholder handler, router/spec plumbing, and obsolete placeholder coverage. - Keep `features.unavailable_dummy_tools` as a removed no-op feature tombstone so existing configs still parse cleanly. - Add an integration-style `tool_search` regression test showing that a deferred MCP tool surfaced through `tool_search` still routes through MCP and returns a model-visible tool-call error rather than `unsupported call`. ## Verification - `cargo test -p codex-core tool_search`
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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<ToolSpec>,
|
||||
}
|
||||
|
||||
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<ToolSpec> {
|
||||
self.spec.clone()
|
||||
}
|
||||
|
||||
async fn handle(&self, invocation: ToolInvocation) -> Result<Self::Output, FunctionCallError> {
|
||||
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(),
|
||||
)),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<ToolSpec>, ToolRegistry) {
|
||||
let registry = ToolRegistry::new(self.handlers);
|
||||
(self.specs, registry)
|
||||
|
||||
@@ -43,7 +43,6 @@ pub struct ToolRouter {
|
||||
pub(crate) struct ToolRouterParams<'a> {
|
||||
pub(crate) mcp_tools: Option<Vec<ToolInfo>>,
|
||||
pub(crate) deferred_mcp_tools: Option<Vec<ToolInfo>>,
|
||||
pub(crate) unavailable_called_tools: Vec<ToolName>,
|
||||
pub(crate) discoverable_tools: Option<Vec<DiscoverableTool>>,
|
||||
pub(crate) extension_tool_bundles: Vec<ExtensionToolBundle>,
|
||||
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,
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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<Vec<ToolInfo>>,
|
||||
deferred_mcp_tools: Option<Vec<ToolInfo>>,
|
||||
unavailable_called_tools: Vec<ToolName>,
|
||||
discoverable_tools: Option<Vec<DiscoverableTool>>,
|
||||
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::<HashSet<_>>();
|
||||
|
||||
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)]
|
||||
|
||||
@@ -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<Vec<ToolInfo>>,
|
||||
deferred_mcp_tools: Option<Vec<ToolInfo>>,
|
||||
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<Vec<ToolInfo>>,
|
||||
deferred_mcp_tools: Option<Vec<ToolInfo>>,
|
||||
unavailable_called_tools: Vec<ToolName>,
|
||||
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;
|
||||
|
||||
Reference in New Issue
Block a user