Make tool executor specs mandatory (#23870)

## Why

`ToolExecutor` is the runtime contract that keeps a callable tool and
its model-visible spec together. Leaving `spec()` optional lets a
registered runtime silently omit that half of the contract, and it also
overloads a missing spec as an exposure decision for tools that should
stay dispatchable without being shown to the model.

## What

- Make `ToolExecutor::spec()` required and update core, extension, and
test tool executors to return a concrete `ToolSpec`.
- Add `ToolExposure::Hidden` for dispatch-only tools. The legacy
`shell_command` runtime in unified-exec sessions now uses that explicit
exposure instead of hiding itself by omitting a spec.
- Build MCP tool specs when `McpHandler` is constructed so invalid MCP
specs are skipped before the handler is registered.
- Keep tool planning aligned with the new contract for direct, deferred,
hidden, code-mode, dynamic, and namespaced tool paths.

## Testing

- Added tool-plan coverage that invalid MCP tool specs are not
registered.
- Updated shell-family coverage for the hidden legacy `shell_command`
runtime and the affected tool executor test fixtures.
This commit is contained in:
jif-oai authored and GitHub committed 2026-05-21 15:25:56 +02:00
1 parent 94442b7f95
commit 516f134641
50 files changed
+304 -193

No files matched your search

@@ -94,8 +94,8 @@ impl ToolExecutor<ToolInvocation> for CodeModeExecuteHandler {
ToolName::plain(PUBLIC_TOOL_NAME) ToolName::plain(PUBLIC_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(self.spec.clone()) self.spec.clone()
} }
async fn handle( async fn handle(
@@ -47,8 +47,8 @@ impl ToolExecutor<ToolInvocation> for CodeModeWaitHandler {
ToolName::plain(WAIT_TOOL_NAME) ToolName::plain(WAIT_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_wait_tool()) create_wait_tool()
} }
async fn handle( async fn handle(
@@ -19,8 +19,8 @@ impl ToolExecutor<ToolInvocation> for ReportAgentJobResultHandler {
ToolName::plain("report_agent_job_result") ToolName::plain("report_agent_job_result")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_report_agent_job_result_tool()) create_report_agent_job_result_tool()
} }
async fn handle( async fn handle(
@@ -20,8 +20,8 @@ impl ToolExecutor<ToolInvocation> for SpawnAgentsOnCsvHandler {
ToolName::plain("spawn_agents_on_csv") ToolName::plain("spawn_agents_on_csv")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_spawn_agents_on_csv_tool()) create_spawn_agents_on_csv_tool()
} }
async fn handle( async fn handle(
@@ -303,8 +303,8 @@ impl ToolExecutor<ToolInvocation> for ApplyPatchHandler {
ToolName::plain("apply_patch") ToolName::plain("apply_patch")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_apply_patch_freeform_tool(self.multi_environment)) create_apply_patch_freeform_tool(self.multi_environment)
} }
async fn handle( async fn handle(
+4 -4
View File
@@ -31,7 +31,7 @@ use tracing::warn;
pub struct DynamicToolHandler { pub struct DynamicToolHandler {
tool_name: ToolName, tool_name: ToolName,
spec: Option<ToolSpec>, spec: ToolSpec,
exposure: ToolExposure, exposure: ToolExposure,
search_text: String, search_text: String,
} }
@@ -50,7 +50,7 @@ impl DynamicToolHandler {
}; };
Some(Self { Some(Self {
tool_name, tool_name,
spec: Some(spec), spec,
exposure: if tool.defer_loading { exposure: if tool.defer_loading {
ToolExposure::Deferred ToolExposure::Deferred
} else { } else {
@@ -67,7 +67,7 @@ impl ToolExecutor<ToolInvocation> for DynamicToolHandler {
self.tool_name.clone() self.tool_name.clone()
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
self.spec.clone() self.spec.clone()
} }
@@ -130,7 +130,7 @@ impl CoreToolRuntime for DynamicToolHandler {
fn search_info(&self) -> Option<ToolSearchInfo> { fn search_info(&self) -> Option<ToolSearchInfo> {
ToolSearchInfo::from_spec( ToolSearchInfo::from_spec(
self.search_text.clone(), self.search_text.clone(),
self.spec()?, self.spec(),
Some(ToolSearchSourceInfo { Some(ToolSearchSourceInfo {
name: "Dynamic tools".to_string(), name: "Dynamic tools".to_string(),
description: Some("Tools provided by the current Codex thread.".to_string()), description: Some("Tools provided by the current Codex thread.".to_string()),
@@ -37,7 +37,7 @@ impl ToolExecutor<ToolInvocation> for ExtensionToolAdapter {
self.0.tool_name() self.0.tool_name()
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
self.0.spec() self.0.spec()
} }
@@ -130,25 +130,23 @@ mod tests {
codex_tools::ToolName::plain("extension_echo") codex_tools::ToolName::plain("extension_echo")
} }
fn spec(&self) -> Option<codex_tools::ToolSpec> { fn spec(&self) -> codex_tools::ToolSpec {
Some(codex_tools::ToolSpec::Function( codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
codex_tools::ResponsesApiTool { name: "extension_echo".to_string(),
name: "extension_echo".to_string(), description: "Echoes arguments.".to_string(),
description: "Echoes arguments.".to_string(), strict: true,
strict: true, parameters: codex_tools::parse_tool_input_schema(&json!({
parameters: codex_tools::parse_tool_input_schema(&json!({ "type": "object",
"type": "object", "properties": {
"properties": { "message": { "type": "string" },
"message": { "type": "string" }, },
}, "required": ["message"],
"required": ["message"], "additionalProperties": false,
"additionalProperties": false, }))
})) .expect("extension schema should parse"),
.expect("extension schema should parse"), output_schema: None,
output_schema: None, defer_loading: None,
defer_loading: None, })
},
))
} }
async fn handle( async fn handle(
@@ -171,6 +169,17 @@ mod tests {
codex_tools::ToolName::plain("extension_echo") codex_tools::ToolName::plain("extension_echo")
} }
fn spec(&self) -> codex_tools::ToolSpec {
codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
name: "extension_echo".to_string(),
description: "Captures arguments.".to_string(),
strict: false,
parameters: codex_tools::JsonSchema::default(),
output_schema: None,
defer_loading: None,
})
}
async fn handle( async fn handle(
&self, &self,
call: codex_tools::ToolCall, call: codex_tools::ToolCall,
@@ -24,8 +24,8 @@ impl ToolExecutor<ToolInvocation> for CreateGoalHandler {
ToolName::plain(CREATE_GOAL_TOOL_NAME) ToolName::plain(CREATE_GOAL_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_create_goal_tool()) create_create_goal_tool()
} }
async fn handle( async fn handle(
@@ -21,8 +21,8 @@ impl ToolExecutor<ToolInvocation> for GetGoalHandler {
ToolName::plain(GET_GOAL_TOOL_NAME) ToolName::plain(GET_GOAL_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_get_goal_tool()) create_get_goal_tool()
} }
async fn handle( async fn handle(
@@ -26,8 +26,8 @@ impl ToolExecutor<ToolInvocation> for UpdateGoalHandler {
ToolName::plain(UPDATE_GOAL_TOOL_NAME) ToolName::plain(UPDATE_GOAL_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_update_goal_tool()) create_update_goal_tool()
} }
async fn handle( async fn handle(
@@ -60,8 +60,8 @@ impl ToolExecutor<ToolInvocation> for ListAvailablePluginsToInstallHandler {
ToolName::plain(LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME) ToolName::plain(LIST_AVAILABLE_PLUGINS_TO_INSTALL_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_list_available_plugins_to_install_tool()) create_list_available_plugins_to_install_tool()
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
+47 -35
View File
@@ -29,19 +29,25 @@ use serde_json::Value;
pub struct McpHandler { pub struct McpHandler {
tool_info: ToolInfo, tool_info: ToolInfo,
spec: ToolSpec,
exposure: ToolExposure, exposure: ToolExposure,
} }
impl McpHandler { impl McpHandler {
pub fn new(tool_info: ToolInfo) -> Self { pub fn new(tool_info: ToolInfo) -> Result<Self, serde_json::Error> {
Self::with_exposure(tool_info, ToolExposure::Direct) Self::with_exposure(tool_info, ToolExposure::Direct)
} }
pub fn with_exposure(tool_info: ToolInfo, exposure: ToolExposure) -> Self { pub fn with_exposure(
Self { tool_info: ToolInfo,
exposure: ToolExposure,
) -> Result<Self, serde_json::Error> {
let spec = create_tool_spec(&tool_info)?;
Ok(Self {
tool_info, tool_info,
spec,
exposure, exposure,
} })
} }
} }
@@ -51,32 +57,8 @@ impl ToolExecutor<ToolInvocation> for McpHandler {
self.tool_info.canonical_tool_name() self.tool_info.canonical_tool_name()
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
let tool_name = self.tool_name(); self.spec.clone()
let namespace_name = tool_name.namespace.as_ref()?;
let tool = mcp_tool_to_responses_api_tool(&tool_name, &self.tool_info.tool).ok()?;
let description = self
.tool_info
.namespace_description
.as_deref()
.map(str::trim)
.filter(|description| !description.is_empty())
.map(str::to_string)
.or_else(|| {
self.tool_info
.connector_name
.as_deref()
.map(str::trim)
.filter(|connector_name| !connector_name.is_empty())
.map(|connector_name| format!("Tools for working with {connector_name}."))
})
.unwrap_or_default();
Some(ToolSpec::Namespace(ResponsesApiNamespace {
name: namespace_name.clone(),
description,
tools: vec![ResponsesApiNamespaceTool::Function(tool)],
}))
} }
fn exposure(&self) -> ToolExposure { fn exposure(&self) -> ToolExposure {
@@ -152,7 +134,7 @@ impl CoreToolRuntime for McpHandler {
ToolSearchInfo::from_spec( ToolSearchInfo::from_spec(
build_mcp_search_text(&self.tool_info), build_mcp_search_text(&self.tool_info),
self.spec()?, self.spec(),
source_info, source_info,
) )
} }
@@ -223,6 +205,32 @@ impl CoreToolRuntime for McpHandler {
} }
} }
fn create_tool_spec(tool_info: &ToolInfo) -> Result<ToolSpec, serde_json::Error> {
let tool_name = tool_info.canonical_tool_name();
let tool = mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)?;
let description = tool_info
.namespace_description
.as_deref()
.map(str::trim)
.filter(|description| !description.is_empty())
.map(str::to_string)
.or_else(|| {
tool_info
.connector_name
.as_deref()
.map(str::trim)
.filter(|connector_name| !connector_name.is_empty())
.map(|connector_name| format!("Tools for working with {connector_name}."))
})
.unwrap_or_default();
Ok(ToolSpec::Namespace(ResponsesApiNamespace {
name: tool_info.callable_namespace.clone(),
description,
tools: vec![ResponsesApiNamespaceTool::Function(tool)],
}))
}
fn mcp_hook_tool_input(raw_arguments: &str) -> Value { fn mcp_hook_tool_input(raw_arguments: &str) -> Value {
if raw_arguments.trim().is_empty() { if raw_arguments.trim().is_empty() {
return Value::Object(Map::new()); return Value::Object(Map::new());
@@ -306,7 +314,8 @@ mod tests {
.to_string(), .to_string(),
}; };
let (session, turn) = make_session_and_context().await; let (session, turn) = make_session_and_context().await;
let handler = McpHandler::new(tool_info("memory", "mcp__memory__", "create_entities")); let handler = McpHandler::new(tool_info("memory", "mcp__memory__", "create_entities"))
.expect("MCP tool spec should build");
assert_eq!( assert_eq!(
handler.pre_tool_use_payload(&ToolInvocation { handler.pre_tool_use_payload(&ToolInvocation {
session: session.into(), session: session.into(),
@@ -336,7 +345,8 @@ mod tests {
arguments: json!({ "message": "hello" }).to_string(), arguments: json!({ "message": "hello" }).to_string(),
}; };
let (session, turn) = make_session_and_context().await; let (session, turn) = make_session_and_context().await;
let handler = McpHandler::new(tool_info("foo", "mcp__foo__", "exec_command")); let handler = McpHandler::new(tool_info("foo", "mcp__foo__", "exec_command"))
.expect("MCP tool spec should build");
assert_eq!( assert_eq!(
handler.pre_tool_use_payload(&ToolInvocation { handler.pre_tool_use_payload(&ToolInvocation {
@@ -362,7 +372,8 @@ mod tests {
arguments: json!({ "message": "hello" }).to_string(), arguments: json!({ "message": "hello" }).to_string(),
}; };
let (session, turn) = make_session_and_context().await; let (session, turn) = make_session_and_context().await;
let handler = McpHandler::new(tool_info("foo", "mcp__foo__", "exec_command")); let handler = McpHandler::new(tool_info("foo", "mcp__foo__", "exec_command"))
.expect("MCP tool spec should build");
let invocation = handler let invocation = handler
.with_updated_hook_input( .with_updated_hook_input(
@@ -411,7 +422,8 @@ mod tests {
truncation_policy: codex_utils_output_truncation::TruncationPolicy::Bytes(1024), truncation_policy: codex_utils_output_truncation::TruncationPolicy::Bytes(1024),
}; };
let (session, turn) = make_session_and_context().await; let (session, turn) = make_session_and_context().await;
let handler = McpHandler::new(tool_info("filesystem", "mcp__filesystem__", "read_file")); let handler = McpHandler::new(tool_info("filesystem", "mcp__filesystem__", "read_file"))
.expect("MCP tool spec should build");
let invocation = ToolInvocation { let invocation = ToolInvocation {
session: session.into(), session: session.into(),
turn: turn.into(), turn: turn.into(),
@@ -32,8 +32,8 @@ impl ToolExecutor<ToolInvocation> for ListMcpResourceTemplatesHandler {
ToolName::plain("list_mcp_resource_templates") ToolName::plain("list_mcp_resource_templates")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_list_mcp_resource_templates_tool()) create_list_mcp_resource_templates_tool()
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -32,8 +32,8 @@ impl ToolExecutor<ToolInvocation> for ListMcpResourcesHandler {
ToolName::plain("list_mcp_resources") ToolName::plain("list_mcp_resources")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_list_mcp_resources_tool()) create_list_mcp_resources_tool()
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -32,8 +32,8 @@ impl ToolExecutor<ToolInvocation> for ReadMcpResourceHandler {
ToolName::plain("read_mcp_resource") ToolName::plain("read_mcp_resource")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_read_mcp_resource_tool()) create_read_mcp_resource_tool()
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -6,7 +6,7 @@ use serde_json::json;
#[test] #[test]
fn search_info_uses_mcp_tool_metadata_and_parameter_names() { fn search_info_uses_mcp_tool_metadata_and_parameter_names() {
let handler = McpHandler::new(tool_info()); let handler = McpHandler::new(tool_info()).expect("MCP tool spec should build");
let search_info = handler.search_info().expect("MCP search info"); let search_info = handler.search_info().expect("MCP search info");
assert_eq!( assert_eq!(
@@ -26,7 +26,7 @@ fn search_info_uses_mcp_tool_metadata_and_parameter_names() {
fn search_info_uses_connector_name_for_output_namespace_description() { fn search_info_uses_connector_name_for_output_namespace_description() {
let mut tool_info = tool_info(); let mut tool_info = tool_info();
tool_info.namespace_description = None; tool_info.namespace_description = None;
let handler = McpHandler::new(tool_info); let handler = McpHandler::new(tool_info).expect("MCP tool spec should build");
let search_info = handler.search_info().expect("MCP search info"); let search_info = handler.search_info().expect("MCP search info");
let LoadableToolSpec::Namespace(namespace) = search_info.entry.output else { let LoadableToolSpec::Namespace(namespace) = search_info.entry.output else {
@@ -11,8 +11,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "close_agent") ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "close_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_close_agent_tool_v1()) create_close_agent_tool_v1()
} }
async fn handle( async fn handle(
@@ -110,7 +110,7 @@ impl CoreToolRuntime for Handler {
fn search_info(&self) -> Option<ToolSearchInfo> { fn search_info(&self) -> Option<ToolSearchInfo> {
multi_agent_tool_search_info( multi_agent_tool_search_info(
"close_agent close shutdown stop agent subagent thread status target", "close_agent close shutdown stop agent subagent thread status target",
self.spec()?, self.spec(),
) )
} }
@@ -13,8 +13,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "resume_agent") ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "resume_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_resume_agent_tool()) create_resume_agent_tool()
} }
async fn handle( async fn handle(
@@ -138,7 +138,7 @@ impl CoreToolRuntime for Handler {
fn search_info(&self) -> Option<ToolSearchInfo> { fn search_info(&self) -> Option<ToolSearchInfo> {
multi_agent_tool_search_info( multi_agent_tool_search_info(
"resume_agent resume reopen closed agent subagent thread id target", "resume_agent resume reopen closed agent subagent thread id target",
self.spec()?, self.spec(),
) )
} }
@@ -12,8 +12,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "send_input") ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "send_input")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_send_input_tool_v1()) create_send_input_tool_v1()
} }
async fn handle( async fn handle(
@@ -94,7 +94,7 @@ impl CoreToolRuntime for Handler {
fn search_info(&self) -> Option<ToolSearchInfo> { fn search_info(&self) -> Option<ToolSearchInfo> {
multi_agent_tool_search_info( multi_agent_tool_search_info(
"send_input send message existing agent subagent follow up interrupt redirect queue target", "send_input send message existing agent subagent follow up interrupt redirect queue target",
self.spec()?, self.spec(),
) )
} }
@@ -28,8 +28,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "spawn_agent") ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "spawn_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_spawn_agent_tool_v1(self.options.clone())) create_spawn_agent_tool_v1(self.options.clone())
} }
async fn handle( async fn handle(
@@ -206,7 +206,7 @@ impl CoreToolRuntime for Handler {
fn search_info(&self) -> Option<ToolSearchInfo> { fn search_info(&self) -> Option<ToolSearchInfo> {
multi_agent_tool_search_info( multi_agent_tool_search_info(
"spawn_agent spawn agent subagent sub-agent delegate delegation parallel work worker explorer no-apps fork model reasoning", "spawn_agent spawn agent subagent sub-agent delegate delegation parallel work worker explorer no-apps fork model reasoning",
self.spec()?, self.spec(),
) )
} }
@@ -33,8 +33,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "wait_agent") ToolName::namespaced(MULTI_AGENT_V1_NAMESPACE, "wait_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_wait_agent_tool_v1(self.options)) create_wait_agent_tool_v1(self.options)
} }
async fn handle( async fn handle(
@@ -206,7 +206,7 @@ impl CoreToolRuntime for Handler {
fn search_info(&self) -> Option<ToolSearchInfo> { fn search_info(&self) -> Option<ToolSearchInfo> {
multi_agent_tool_search_info( multi_agent_tool_search_info(
"wait_agent wait agent subagent status final result complete timeout targets", "wait_agent wait agent subagent status final result complete timeout targets",
self.spec()?, self.spec(),
) )
} }
@@ -11,8 +11,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::plain("close_agent") ToolName::plain("close_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_close_agent_tool_v2()) create_close_agent_tool_v2()
} }
async fn handle( async fn handle(
@@ -13,8 +13,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::plain("followup_task") ToolName::plain("followup_task")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_followup_task_tool()) create_followup_task_tool()
} }
async fn handle( async fn handle(
@@ -11,8 +11,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::plain("list_agents") ToolName::plain("list_agents")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_list_agents_tool()) create_list_agents_tool()
} }
async fn handle( async fn handle(
@@ -13,8 +13,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::plain("send_message") ToolName::plain("send_message")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_send_message_tool()) create_send_message_tool()
} }
async fn handle( async fn handle(
@@ -30,8 +30,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::plain("spawn_agent") ToolName::plain("spawn_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_spawn_agent_tool_v2(self.options.clone())) create_spawn_agent_tool_v2(self.options.clone())
} }
async fn handle( async fn handle(
@@ -25,8 +25,8 @@ impl ToolExecutor<ToolInvocation> for Handler {
ToolName::plain("wait_agent") ToolName::plain("wait_agent")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_wait_agent_tool_v2(self.options)) create_wait_agent_tool_v2(self.options)
} }
async fn handle( async fn handle(
+2 -2
View File
@@ -51,8 +51,8 @@ impl ToolExecutor<ToolInvocation> for PlanHandler {
ToolName::plain("update_plan") ToolName::plain("update_plan")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_update_plan_tool()) create_update_plan_tool()
} }
async fn handle( async fn handle(
@@ -22,10 +22,8 @@ impl ToolExecutor<ToolInvocation> for RequestPermissionsHandler {
ToolName::plain("request_permissions") ToolName::plain("request_permissions")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_request_permissions_tool( create_request_permissions_tool(request_permissions_tool_description())
request_permissions_tool_description(),
))
} }
async fn handle( async fn handle(
@@ -45,8 +45,8 @@ impl ToolExecutor<ToolInvocation> for RequestPluginInstallHandler {
ToolName::plain(REQUEST_PLUGIN_INSTALL_TOOL_NAME) ToolName::plain(REQUEST_PLUGIN_INSTALL_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_request_plugin_install_tool()) create_request_plugin_install_tool()
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -26,10 +26,8 @@ impl ToolExecutor<ToolInvocation> for RequestUserInputHandler {
ToolName::plain(REQUEST_USER_INPUT_TOOL_NAME) ToolName::plain(REQUEST_USER_INPUT_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_request_user_input_tool( create_request_user_input_tool(request_user_input_tool_description(&self.available_modes))
request_user_input_tool_description(&self.available_modes),
))
} }
async fn handle( async fn handle(
@@ -22,6 +22,7 @@ use crate::tools::registry::CoreToolRuntime;
use crate::tools::registry::PostToolUsePayload; use crate::tools::registry::PostToolUsePayload;
use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::PreToolUsePayload;
use crate::tools::registry::ToolExecutor; use crate::tools::registry::ToolExecutor;
use crate::tools::registry::ToolExposure;
use crate::tools::runtimes::shell::ShellRuntimeBackend; use crate::tools::runtimes::shell::ShellRuntimeBackend;
use codex_tools::ToolSpec; use codex_tools::ToolSpec;
@@ -39,7 +40,8 @@ enum ShellCommandBackend {
pub struct ShellCommandHandler { pub struct ShellCommandHandler {
backend: ShellCommandBackend, backend: ShellCommandBackend,
options: Option<ShellCommandHandlerOptions>, options: ShellCommandHandlerOptions,
exposure: ToolExposure,
} }
#[derive(Clone, Copy)] #[derive(Clone, Copy)]
@@ -51,9 +53,22 @@ pub(crate) struct ShellCommandHandlerOptions {
impl ShellCommandHandler { impl ShellCommandHandler {
pub(crate) fn new(options: ShellCommandHandlerOptions) -> Self { pub(crate) fn new(options: ShellCommandHandlerOptions) -> Self {
Self::with_exposure(options, ToolExposure::Direct)
}
pub(crate) fn hidden(options: ShellCommandHandlerOptions) -> Self {
Self::with_exposure(options, ToolExposure::Hidden)
}
fn with_exposure(options: ShellCommandHandlerOptions, exposure: ToolExposure) -> Self {
let backend = match options.backend_config {
ShellCommandBackendConfig::Classic => ShellCommandBackend::Classic,
ShellCommandBackendConfig::ZshFork => ShellCommandBackend::ZshFork,
};
Self { Self {
options: Some(options), backend,
..Self::from(options.backend_config) options,
exposure,
} }
} }
@@ -114,15 +129,12 @@ impl ShellCommandHandler {
} }
impl From<ShellCommandBackendConfig> for ShellCommandHandler { impl From<ShellCommandBackendConfig> for ShellCommandHandler {
fn from(config: ShellCommandBackendConfig) -> Self { fn from(backend_config: ShellCommandBackendConfig) -> Self {
let backend = match config { Self::hidden(ShellCommandHandlerOptions {
ShellCommandBackendConfig::Classic => ShellCommandBackend::Classic, backend_config,
ShellCommandBackendConfig::ZshFork => ShellCommandBackend::ZshFork, allow_login_shell: false,
}; exec_permission_approvals_enabled: false,
Self { })
backend,
options: None,
}
} }
} }
@@ -132,17 +144,19 @@ impl ToolExecutor<ToolInvocation> for ShellCommandHandler {
ToolName::plain("shell_command") ToolName::plain("shell_command")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
self.options.map(|options| { create_shell_command_tool(CommandToolOptions {
create_shell_command_tool(CommandToolOptions { allow_login_shell: self.options.allow_login_shell,
allow_login_shell: options.allow_login_shell, exec_permission_approvals_enabled: self.options.exec_permission_approvals_enabled,
exec_permission_approvals_enabled: options.exec_permission_approvals_enabled,
})
}) })
} }
fn exposure(&self) -> ToolExposure {
self.exposure
}
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
self.options.is_some() self.exposure != ToolExposure::Hidden
} }
async fn handle( async fn handle(
@@ -63,8 +63,8 @@ impl ToolExecutor<ToolInvocation> for TestSyncHandler {
ToolName::plain("test_sync_tool") ToolName::plain("test_sync_tool")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_test_sync_tool()) create_test_sync_tool()
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -59,11 +59,8 @@ impl ToolExecutor<ToolInvocation> for ToolSearchHandler {
ToolName::plain(TOOL_SEARCH_TOOL_NAME) ToolName::plain(TOOL_SEARCH_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_tool_search_tool( create_tool_search_tool(&self.search_source_infos, TOOL_SEARCH_DEFAULT_LIMIT)
&self.search_source_infos,
TOOL_SEARCH_DEFAULT_LIMIT,
))
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -174,6 +171,7 @@ mod tests {
.iter() .iter()
.map(|tool| { .map(|tool| {
McpHandler::new(tool.clone()) McpHandler::new(tool.clone())
.expect("MCP tool should convert")
.search_info() .search_info()
.expect("MCP handler should return search info") .expect("MCP handler should return search info")
}) })
@@ -74,14 +74,14 @@ impl ToolExecutor<ToolInvocation> for ExecCommandHandler {
ToolName::plain("exec_command") ToolName::plain("exec_command")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_exec_command_tool_with_environment_id( create_exec_command_tool_with_environment_id(
CommandToolOptions { CommandToolOptions {
allow_login_shell: self.options.allow_login_shell, allow_login_shell: self.options.allow_login_shell,
exec_permission_approvals_enabled: self.options.exec_permission_approvals_enabled, exec_permission_approvals_enabled: self.options.exec_permission_approvals_enabled,
}, },
self.options.include_environment_id, self.options.include_environment_id,
)) )
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
@@ -36,8 +36,8 @@ impl ToolExecutor<ToolInvocation> for WriteStdinHandler {
ToolName::plain("write_stdin") ToolName::plain("write_stdin")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_write_stdin_tool()) create_write_stdin_tool()
} }
async fn handle( async fn handle(
@@ -70,8 +70,8 @@ impl ToolExecutor<ToolInvocation> for ViewImageHandler {
ToolName::plain("view_image") ToolName::plain("view_image")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(create_view_image_tool(self.options)) create_view_image_tool(self.options)
} }
fn supports_parallel_tool_calls(&self) -> bool { fn supports_parallel_tool_calls(&self) -> bool {
+11
View File
@@ -250,6 +250,17 @@ mod tests {
self.tool_name.clone() self.tool_name.clone()
} }
fn spec(&self) -> codex_tools::ToolSpec {
codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
name: self.tool_name.name.clone(),
description: "Immediate test tool.".to_string(),
strict: false,
defer_loading: None,
parameters: codex_tools::JsonSchema::default(),
output_schema: None,
})
}
async fn handle( async fn handle(
&self, &self,
_invocation: ToolInvocation, _invocation: ToolInvocation,
+1 -1
View File
@@ -184,7 +184,7 @@ impl ToolExecutor<ToolInvocation> for ExposureOverride {
self.handler.tool_name() self.handler.tool_name()
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
self.handler.spec() self.handler.spec()
} }
+19
View File
@@ -11,6 +11,10 @@ impl ToolExecutor<ToolInvocation> for TestHandler {
self.tool_name.clone() self.tool_name.clone()
} }
fn spec(&self) -> codex_tools::ToolSpec {
test_spec(&self.tool_name)
}
async fn handle( async fn handle(
&self, &self,
_invocation: ToolInvocation, _invocation: ToolInvocation,
@@ -40,6 +44,10 @@ impl ToolExecutor<ToolInvocation> for LifecycleTestHandler {
self.tool_name.clone() self.tool_name.clone()
} }
fn spec(&self) -> codex_tools::ToolSpec {
test_spec(&self.tool_name)
}
async fn handle( async fn handle(
&self, &self,
_invocation: ToolInvocation, _invocation: ToolInvocation,
@@ -60,6 +68,17 @@ impl ToolExecutor<ToolInvocation> for LifecycleTestHandler {
impl CoreToolRuntime for LifecycleTestHandler {} impl CoreToolRuntime for LifecycleTestHandler {}
fn test_spec(tool_name: &codex_tools::ToolName) -> codex_tools::ToolSpec {
codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
name: tool_name.name.clone(),
description: "Test tool.".to_string(),
strict: false,
defer_loading: None,
parameters: codex_tools::JsonSchema::default(),
output_schema: None,
})
}
#[derive(Debug, PartialEq, Eq)] #[derive(Debug, PartialEq, Eq)]
enum RecordedToolLifecycle { enum RecordedToolLifecycle {
Start { Start {
+3 -3
View File
@@ -49,8 +49,8 @@ impl ToolExecutor<ExtensionToolCall> for ExtensionEchoExecutor {
ToolName::namespaced("extension/", "echo") ToolName::namespaced("extension/", "echo")
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(ToolSpec::Namespace(ResponsesApiNamespace { ToolSpec::Namespace(ResponsesApiNamespace {
name: "extension/".to_string(), name: "extension/".to_string(),
description: default_namespace_description("extension/"), description: default_namespace_description("extension/"),
tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool {
@@ -69,7 +69,7 @@ impl ToolExecutor<ExtensionToolCall> for ExtensionEchoExecutor {
output_schema: None, output_schema: None,
defer_loading: None, defer_loading: None,
})], })],
})) })
} }
async fn handle( async fn handle(
+24 -15
View File
@@ -173,10 +173,9 @@ fn build_model_visible_specs_and_registry(
continue; continue;
} }
let exposure = runtime.exposure(); let exposure = runtime.exposure();
if exposure.is_direct() if exposure.is_direct() && !is_hidden_by_code_mode_only(turn_context, &tool_name, exposure)
&& !is_hidden_by_code_mode_only(turn_context, &tool_name, exposure)
&& let Some(spec) = runtime.spec()
{ {
let spec = runtime.spec();
specs.push(spec_for_model_request(turn_context, exposure, spec)); specs.push(spec_for_model_request(turn_context, exposure, spec));
} }
} }
@@ -375,9 +374,10 @@ fn build_code_mode_executors(
continue; continue;
} }
let Some(spec) = executor.spec() else { if exposure == ToolExposure::Hidden {
continue; continue;
}; }
let spec = executor.spec();
if exposure != ToolExposure::Deferred { if exposure != ToolExposure::Deferred {
exec_prompt_tool_specs.push(spec.clone()); exec_prompt_tool_specs.push(spec.clone());
@@ -666,16 +666,25 @@ fn add_collaboration_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mu
fn add_mcp_runtime_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut PlannedTools) { fn add_mcp_runtime_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut PlannedTools) {
if let Some(mcp_tools) = context.mcp_tools { if let Some(mcp_tools) = context.mcp_tools {
for tool in mcp_tools { for tool in mcp_tools {
planned_tools.add_runtime(McpHandler::new(tool.clone())); match McpHandler::new(tool.clone()) {
Ok(handler) => planned_tools.add_runtime(handler),
Err(err) => warn!(
"Skipping MCP tool `{}`: failed to build tool spec: {err}",
tool.canonical_tool_name()
),
}
} }
} }
if let Some(deferred_mcp_tools) = context.deferred_mcp_tools { if let Some(deferred_mcp_tools) = context.deferred_mcp_tools {
for tool in deferred_mcp_tools { for tool in deferred_mcp_tools {
planned_tools.add_runtime(McpHandler::with_exposure( match McpHandler::with_exposure(tool.clone(), ToolExposure::Deferred) {
tool.clone(), Ok(handler) => planned_tools.add_runtime(handler),
ToolExposure::Deferred, Err(err) => warn!(
)); "Skipping deferred MCP tool `{}`: failed to build tool spec: {err}",
tool.canonical_tool_name()
),
}
} }
} }
} }
@@ -815,14 +824,14 @@ impl ToolExecutor<ToolInvocation> for MultiAgentV2NamespaceOverride {
ToolName::namespaced(self.namespace.clone(), self.handler.tool_name().name) ToolName::namespaced(self.namespace.clone(), self.handler.tool_name().name)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
match self.handler.spec()? { match self.handler.spec() {
ToolSpec::Function(tool) => Some(ToolSpec::Namespace(ResponsesApiNamespace { ToolSpec::Function(tool) => ToolSpec::Namespace(ResponsesApiNamespace {
name: self.namespace.clone(), name: self.namespace.clone(),
description: MULTI_AGENT_V2_NAMESPACE_DESCRIPTION.to_string(), description: MULTI_AGENT_V2_NAMESPACE_DESCRIPTION.to_string(),
tools: vec![ResponsesApiNamespaceTool::Function(tool)], tools: vec![ResponsesApiNamespaceTool::Function(tool)],
})), }),
spec => Some(spec), spec => spec,
} }
} }
+28 -1
View File
@@ -288,6 +288,14 @@ fn mcp_tool(server: &str, namespace: &str, name: &str) -> ToolInfo {
} }
} }
fn invalid_mcp_tool(server: &str, namespace: &str, name: &str) -> ToolInfo {
let mut tool = mcp_tool(server, namespace, name);
tool.tool.input_schema = Arc::new(rmcp::model::object(json!({
"type": "null",
})));
tool
}
fn dynamic_tool(namespace: Option<&str>, name: &str, defer_loading: bool) -> DynamicToolSpec { fn dynamic_tool(namespace: Option<&str>, name: &str, defer_loading: bool) -> DynamicToolSpec {
DynamicToolSpec { DynamicToolSpec {
namespace: namespace.map(str::to_string), namespace: namespace.map(str::to_string),
@@ -342,7 +350,7 @@ async fn shell_family_registers_visible_unified_exec_and_hidden_legacy_shell() {
plan.assert_visible_contains(&["exec_command", "write_stdin"]); plan.assert_visible_contains(&["exec_command", "write_stdin"]);
plan.assert_visible_lacks(&["shell_command"]); plan.assert_visible_lacks(&["shell_command"]);
plan.assert_registered_contains(&["exec_command", "write_stdin", "shell_command"]); plan.assert_registered_contains(&["exec_command", "write_stdin", "shell_command"]);
assert_eq!(plan.exposure("shell_command"), ToolExposure::Direct); assert_eq!(plan.exposure("shell_command"), ToolExposure::Hidden);
} }
#[tokio::test] #[tokio::test]
@@ -507,6 +515,25 @@ async fn mcp_and_tool_search_follow_direct_and_deferred_tool_exposure() {
enabled.assert_registered_contains(&["tool_search", "mcp__searchable__lookup"]); enabled.assert_registered_contains(&["tool_search", "mcp__searchable__lookup"]);
} }
#[tokio::test]
async fn invalid_mcp_tools_are_not_registered() {
let plan = probe_with(
|_| {},
ToolPlanInputs {
mcp_tools: Some(vec![invalid_mcp_tool(
"invalid",
"mcp__invalid__",
"lookup",
)]),
..ToolPlanInputs::default()
},
)
.await;
plan.assert_visible_lacks(&["mcp__invalid__"]);
plan.assert_registered_lacks(&["mcp__invalid__lookup"]);
}
#[tokio::test] #[tokio::test]
async fn request_plugin_install_requires_all_discovery_features_and_discoverable_tools() { async fn request_plugin_install_requires_all_discovery_features_and_discoverable_tools() {
let discoverable_tools = Some(vec![discoverable_plugin("github", "GitHub")]); let discoverable_tools = Some(vec![discoverable_plugin("github", "GitHub")]);
@@ -36,6 +36,17 @@ impl ToolExecutor<ToolInvocation> for TestHandler {
self.tool_name.clone() self.tool_name.clone()
} }
fn spec(&self) -> codex_tools::ToolSpec {
codex_tools::ToolSpec::Function(codex_tools::ResponsesApiTool {
name: self.tool_name.name.clone(),
description: "Test tool.".to_string(),
strict: false,
defer_loading: None,
parameters: codex_tools::JsonSchema::default(),
output_schema: None,
})
}
async fn handle( async fn handle(
&self, &self,
_invocation: ToolInvocation, _invocation: ToolInvocation,
+5 -1
View File
@@ -43,7 +43,11 @@ pub(crate) fn build_shell_tools(options: ShellToolsOptions) -> Vec<Arc<dyn CoreT
// unified exec is model-visible. // unified exec is model-visible.
add_runtime( add_runtime(
&mut runtimes, &mut runtimes,
ShellCommandHandler::from(options.shell_command_backend), ShellCommandHandler::hidden(ShellCommandHandlerOptions {
backend_config: options.shell_command_backend,
allow_login_shell: options.allow_login_shell,
exec_permission_approvals_enabled: options.exec_permission_approvals_enabled,
}),
); );
} }
ConfigShellToolType::Disabled => {} ConfigShellToolType::Disabled => {}
+3 -3
View File
@@ -125,12 +125,12 @@ impl ToolExecutor<ToolCall> for GoalToolExecutor {
}) })
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(match self.kind { match self.kind {
GoalToolKind::Get => create_get_goal_tool(), GoalToolKind::Get => create_get_goal_tool(),
GoalToolKind::Create => create_create_goal_tool(), GoalToolKind::Create => create_create_goal_tool(),
GoalToolKind::Update => create_update_goal_tool(), GoalToolKind::Update => create_update_goal_tool(),
}) }
} }
async fn handle(&self, invocation: ToolCall) -> Result<Box<dyn ToolOutput>, FunctionCallError> { async fn handle(&self, invocation: ToolCall) -> Result<Box<dyn ToolOutput>, FunctionCallError> {
+3 -3
View File
@@ -43,11 +43,11 @@ where
memory_tool_name(LIST_TOOL_NAME) memory_tool_name(LIST_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(memory_function_tool::<ListArgs, ListMemoriesResponse>( memory_function_tool::<ListArgs, ListMemoriesResponse>(
LIST_TOOL_NAME, LIST_TOOL_NAME,
"List immediate files and directories under a path in the Codex memories store.", "List immediate files and directories under a path in the Codex memories store.",
)) )
} }
async fn handle( async fn handle(
+3 -3
View File
@@ -42,11 +42,11 @@ where
memory_tool_name(READ_TOOL_NAME) memory_tool_name(READ_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(memory_function_tool::<ReadArgs, ReadMemoryResponse>( memory_function_tool::<ReadArgs, ReadMemoryResponse>(
READ_TOOL_NAME, READ_TOOL_NAME,
"Read a Codex memory file by relative path, optionally starting at a 1-indexed line offset and limiting the number of lines returned.", "Read a Codex memory file by relative path, optionally starting at a 1-indexed line offset and limiting the number of lines returned.",
)) )
} }
async fn handle( async fn handle(
+3 -3
View File
@@ -51,11 +51,11 @@ where
memory_tool_name(SEARCH_TOOL_NAME) memory_tool_name(SEARCH_TOOL_NAME)
} }
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec {
Some(memory_function_tool::<SearchArgs, SearchMemoriesResponse>( memory_function_tool::<SearchArgs, SearchMemoriesResponse>(
SEARCH_TOOL_NAME, SEARCH_TOOL_NAME,
"Search Codex memory files for substring matches, optionally normalizing separators or requiring all query substrings on the same line or within a line window.", "Search Codex memory files for substring matches, optionally normalizing separators or requiring all query substrings on the same line or within a line window.",
)) )
} }
async fn handle( async fn handle(
+4 -3
View File
@@ -21,6 +21,9 @@ pub enum ToolExposure {
/// In code-mode-only sessions, this keeps the tool callable as a normal /// In code-mode-only sessions, this keeps the tool callable as a normal
/// model tool while excluding it from the nested code-mode tool surface. /// model tool while excluding it from the nested code-mode tool surface.
DirectModelOnly, DirectModelOnly,
/// Keep this tool registered for dispatch without exposing it to the model.
Hidden,
} }
impl ToolExposure { impl ToolExposure {
@@ -39,9 +42,7 @@ pub trait ToolExecutor<Invocation>: Send + Sync {
/// The concrete tool name handled by this runtime instance. /// The concrete tool name handled by this runtime instance.
fn tool_name(&self) -> ToolName; fn tool_name(&self) -> ToolName;
fn spec(&self) -> Option<ToolSpec> { fn spec(&self) -> ToolSpec;
None
}
fn exposure(&self) -> ToolExposure { fn exposure(&self) -> ToolExposure {
ToolExposure::Direct ToolExposure::Direct