From 114bac1409f6ce498e19dad732d4fc01289aa0ed Mon Sep 17 00:00:00 2001 From: xl-openai Date: Thu, 7 May 2026 10:07:03 -0700 Subject: [PATCH 01/10] feat: Expose plugin share metadata in shareContext (#21495) Extends PluginSummary.shareContext with shareUrl and reader shareTargets --- .../codex_app_server_protocol.schemas.json | 15 ++++++ .../codex_app_server_protocol.v2.schemas.json | 15 ++++++ .../schema/json/v2/PluginListResponse.json | 42 +++++++++++++++ .../schema/json/v2/PluginReadResponse.json | 42 +++++++++++++++ .../json/v2/PluginShareListResponse.json | 42 +++++++++++++++ .../typescript/v2/PluginShareContext.ts | 3 +- .../src/protocol/v2/plugin.rs | 2 + .../src/request_processors/plugins.rs | 9 ++++ .../app-server/tests/suite/v2/plugin_list.rs | 31 +++++++++++ .../app-server/tests/suite/v2/plugin_read.rs | 31 +++++++++++ .../app-server/tests/suite/v2/plugin_share.rs | 2 + codex-rs/core-plugins/src/remote.rs | 25 +++++++++ .../core-plugins/src/remote/share/tests.rs | 54 +++++++++++++++++-- 13 files changed, 309 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index fdabfc9291..5e24845729 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -12187,6 +12187,21 @@ }, "remotePluginId": { "type": "string" + }, + "shareTargets": { + "items": { + "$ref": "#/definitions/v2/PluginSharePrincipal" + }, + "type": [ + "array", + "null" + ] + }, + "shareUrl": { + "type": [ + "string", + "null" + ] } }, "required": [ diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index ae225bd7fe..6153a54eb9 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -8798,6 +8798,21 @@ }, "remotePluginId": { "type": "string" + }, + "shareTargets": { + "items": { + "$ref": "#/definitions/PluginSharePrincipal" + }, + "type": [ + "array", + "null" + ] + }, + "shareUrl": { + "type": [ + "string", + "null" + ] } }, "required": [ diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json index a3ecfd3d42..b759d7a3fe 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json @@ -248,6 +248,21 @@ }, "remotePluginId": { "type": "string" + }, + "shareTargets": { + "items": { + "$ref": "#/definitions/PluginSharePrincipal" + }, + "type": [ + "array", + "null" + ] + }, + "shareUrl": { + "type": [ + "string", + "null" + ] } }, "required": [ @@ -255,6 +270,33 @@ ], "type": "object" }, + "PluginSharePrincipal": { + "properties": { + "name": { + "type": "string" + }, + "principalId": { + "type": "string" + }, + "principalType": { + "$ref": "#/definitions/PluginSharePrincipalType" + } + }, + "required": [ + "name", + "principalId", + "principalType" + ], + "type": "object" + }, + "PluginSharePrincipalType": { + "enum": [ + "user", + "group", + "workspace" + ], + "type": "string" + }, "PluginSource": { "oneOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json index b3ec8dd6ec..fe468884f1 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json @@ -302,6 +302,21 @@ }, "remotePluginId": { "type": "string" + }, + "shareTargets": { + "items": { + "$ref": "#/definitions/PluginSharePrincipal" + }, + "type": [ + "array", + "null" + ] + }, + "shareUrl": { + "type": [ + "string", + "null" + ] } }, "required": [ @@ -309,6 +324,33 @@ ], "type": "object" }, + "PluginSharePrincipal": { + "properties": { + "name": { + "type": "string" + }, + "principalId": { + "type": "string" + }, + "principalType": { + "$ref": "#/definitions/PluginSharePrincipalType" + } + }, + "required": [ + "name", + "principalId", + "principalType" + ], + "type": "object" + }, + "PluginSharePrincipalType": { + "enum": [ + "user", + "group", + "workspace" + ], + "type": "string" + }, "PluginSource": { "oneOf": [ { diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginShareListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/PluginShareListResponse.json index 051918f718..96818dfead 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginShareListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginShareListResponse.json @@ -183,6 +183,21 @@ }, "remotePluginId": { "type": "string" + }, + "shareTargets": { + "items": { + "$ref": "#/definitions/PluginSharePrincipal" + }, + "type": [ + "array", + "null" + ] + }, + "shareUrl": { + "type": [ + "string", + "null" + ] } }, "required": [ @@ -215,6 +230,33 @@ ], "type": "object" }, + "PluginSharePrincipal": { + "properties": { + "name": { + "type": "string" + }, + "principalId": { + "type": "string" + }, + "principalType": { + "$ref": "#/definitions/PluginSharePrincipalType" + } + }, + "required": [ + "name", + "principalId", + "principalType" + ], + "type": "object" + }, + "PluginSharePrincipalType": { + "enum": [ + "user", + "group", + "workspace" + ], + "type": "string" + }, "PluginSource": { "oneOf": [ { diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PluginShareContext.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PluginShareContext.ts index f8a8a5e0d0..f1c5c958d7 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PluginShareContext.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PluginShareContext.ts @@ -1,5 +1,6 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { PluginSharePrincipal } from "./PluginSharePrincipal"; -export type PluginShareContext = { remotePluginId: string, creatorAccountUserId: string | null, creatorName: string | null, }; +export type PluginShareContext = { remotePluginId: string, shareUrl: string | null, creatorAccountUserId: string | null, creatorName: string | null, shareTargets: Array | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/plugin.rs b/codex-rs/app-server-protocol/src/protocol/v2/plugin.rs index 53aa861018..20d68e6e55 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/plugin.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/plugin.rs @@ -539,8 +539,10 @@ pub struct PluginSummary { #[ts(export_to = "v2/")] pub struct PluginShareContext { pub remote_plugin_id: String, + pub share_url: Option, pub creator_account_user_id: Option, pub creator_name: Option, + pub share_targets: Option>, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 3616d1ebbe..cc5de4058a 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -108,8 +108,10 @@ fn share_context_for_source( .cloned() .map(|remote_plugin_id| PluginShareContext { remote_plugin_id, + share_url: None, creator_account_user_id: None, creator_name: None, + share_targets: None, }), MarketplacePluginSource::Git { .. } => None, } @@ -1473,8 +1475,15 @@ fn remote_plugin_share_context_to_info( ) -> PluginShareContext { PluginShareContext { remote_plugin_id: context.remote_plugin_id, + share_url: context.share_url, creator_account_user_id: context.creator_account_user_id, creator_name: context.creator_name, + share_targets: context.share_targets.map(|targets| { + targets + .into_iter() + .map(plugin_share_principal_from_remote) + .collect() + }), } } diff --git a/codex-rs/app-server/tests/suite/v2/plugin_list.rs b/codex-rs/app-server/tests/suite/v2/plugin_list.rs index 176aeebeee..ea8294671b 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_list.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_list.rs @@ -13,6 +13,8 @@ use codex_app_server_protocol::PluginListMarketplaceKind; use codex_app_server_protocol::PluginListParams; use codex_app_server_protocol::PluginListResponse; use codex_app_server_protocol::PluginMarketplaceEntry; +use codex_app_server_protocol::PluginSharePrincipal; +use codex_app_server_protocol::PluginSharePrincipalType; use codex_app_server_protocol::PluginSource; use codex_app_server_protocol::PluginSummary; use codex_app_server_protocol::RequestId; @@ -692,8 +694,10 @@ async fn plugin_list_returns_share_context_for_shared_local_plugin() -> Result<( .as_ref() .expect("expected share context"); assert_eq!(share_context.remote_plugin_id, "plugins_123"); + assert_eq!(share_context.share_url, None); assert_eq!(share_context.creator_account_user_id, None); assert_eq!(share_context.creator_name, None); + assert_eq!(share_context.share_targets, None); Ok(()) } @@ -1735,6 +1739,18 @@ async fn plugin_list_fetches_shared_with_me_kind() -> Result<()> { Some("user-gavin__account-123") ); assert_eq!(share_context.creator_name.as_deref(), Some("Gavin")); + assert_eq!( + share_context.share_url.as_deref(), + Some("https://chatgpt.example/plugins/share/share-key-1") + ); + assert_eq!( + share_context.share_targets, + Some(vec![PluginSharePrincipal { + principal_type: PluginSharePrincipalType::User, + principal_id: "user-ada__account-123".to_string(), + name: "Ada".to_string(), + }]) + ); wait_for_remote_plugin_request_count(&server, "/ps/plugins/list", /*expected_count*/ 0).await?; Ok(()) } @@ -2260,10 +2276,25 @@ fn workspace_remote_plugin_page_body( "name": "{plugin_name}", "scope": "WORKSPACE", "creator_account_user_id": "user-gavin__account-123", + "share_url": "https://chatgpt.example/plugins/share/share-key-1", "installation_policy": "AVAILABLE", "authentication_policy": "ON_USE", "status": "ENABLED", "creator_name": "Gavin", + "share_principals": [ + {{ + "principal_type": "user", + "principal_id": "user-gavin__account-123", + "role": "owner", + "name": "Gavin" + }}, + {{ + "principal_type": "user", + "principal_id": "user-ada__account-123", + "role": "reader", + "name": "Ada" + }} + ], "release": {{ "display_name": "{display_name}", "description": "Track work", diff --git a/codex-rs/app-server/tests/suite/v2/plugin_read.rs b/codex-rs/app-server/tests/suite/v2/plugin_read.rs index bbd7a09cf9..16924b0218 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_read.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_read.rs @@ -23,6 +23,8 @@ use codex_app_server_protocol::PluginAuthPolicy; use codex_app_server_protocol::PluginInstallPolicy; use codex_app_server_protocol::PluginReadParams; use codex_app_server_protocol::PluginReadResponse; +use codex_app_server_protocol::PluginSharePrincipal; +use codex_app_server_protocol::PluginSharePrincipalType; use codex_app_server_protocol::PluginSkillReadParams; use codex_app_server_protocol::PluginSkillReadResponse; use codex_app_server_protocol::PluginSource; @@ -237,6 +239,21 @@ async fn plugin_read_returns_share_context_for_shared_remote_plugin() -> Result< "scope": "WORKSPACE", "creator_account_user_id": "user-gavin__account-123", "creator_name": "Gavin", + "share_url": "https://chatgpt.example/plugins/share/share-key-1", + "share_principals": [ + { + "principal_type": "user", + "principal_id": "user-gavin__account-123", + "role": "owner", + "name": "Gavin" + }, + { + "principal_type": "user", + "principal_id": "user-ada__account-123", + "role": "reader", + "name": "Ada" + } + ], "installation_policy": "AVAILABLE", "authentication_policy": "ON_USE", "release": { @@ -307,6 +324,18 @@ async fn plugin_read_returns_share_context_for_shared_remote_plugin() -> Result< Some("user-gavin__account-123") ); assert_eq!(share_context.creator_name.as_deref(), Some("Gavin")); + assert_eq!( + share_context.share_url.as_deref(), + Some("https://chatgpt.example/plugins/share/share-key-1") + ); + assert_eq!( + share_context.share_targets, + Some(vec![PluginSharePrincipal { + principal_type: PluginSharePrincipalType::User, + principal_id: "user-ada__account-123".to_string(), + name: "Ada".to_string(), + }]) + ); Ok(()) } @@ -766,8 +795,10 @@ async fn plugin_read_returns_share_context_for_shared_local_plugin() -> Result<( .as_ref() .expect("expected share context"); assert_eq!(share_context.remote_plugin_id, "plugins_123"); + assert_eq!(share_context.share_url, None); assert_eq!(share_context.creator_account_user_id, None); assert_eq!(share_context.creator_name, None); + assert_eq!(share_context.share_targets, None); Ok(()) } diff --git a/codex-rs/app-server/tests/suite/v2/plugin_share.rs b/codex-rs/app-server/tests/suite/v2/plugin_share.rs index a59183c1d4..09b39ca241 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_share.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_share.rs @@ -662,8 +662,10 @@ fn expected_plugin_interface() -> PluginInterface { fn expected_share_context(plugin_id: &str) -> PluginShareContext { PluginShareContext { remote_plugin_id: plugin_id.to_string(), + share_url: Some("https://chatgpt.example/plugins/share/share-key-1".to_string()), creator_account_user_id: None, creator_name: None, + share_targets: None, } } diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index 3a17048b49..b2da554fd5 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -98,8 +98,10 @@ pub struct RemotePluginSummary { #[derive(Debug, Clone, PartialEq, Eq)] pub struct RemotePluginShareContext { pub remote_plugin_id: String, + pub share_url: Option, pub creator_account_user_id: Option, pub creator_name: Option, + pub share_targets: Option>, } #[derive(Debug, Clone, PartialEq)] @@ -363,6 +365,8 @@ struct RemotePluginDirectoryItem { creator_name: Option, #[serde(default)] share_url: Option, + #[serde(default)] + share_principals: Option>, installation_policy: PluginInstallPolicy, authentication_policy: PluginAuthPolicy, #[serde(rename = "status", default)] @@ -370,6 +374,15 @@ struct RemotePluginDirectoryItem { release: RemotePluginReleaseResponse, } +#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +struct RemotePluginDirectorySharePrincipal { + principal_type: RemotePluginSharePrincipalType, + principal_id: String, + #[serde(default)] + role: Option, + name: String, +} + #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] struct RemotePluginInstalledItem { #[serde(flatten)] @@ -831,8 +844,20 @@ fn remote_plugin_share_context( RemotePluginScope::Global => None, RemotePluginScope::Workspace => Some(RemotePluginShareContext { remote_plugin_id: plugin.id.clone(), + share_url: plugin.share_url.clone(), creator_account_user_id: plugin.creator_account_user_id.clone(), creator_name: plugin.creator_name.clone(), + share_targets: plugin.share_principals.as_ref().map(|principals| { + principals + .iter() + .filter(|principal| principal.role.as_deref() == Some("reader")) + .map(|principal| RemotePluginSharePrincipal { + principal_type: principal.principal_type, + principal_id: principal.principal_id.clone(), + name: principal.name.clone(), + }) + .collect() + }), }), } } diff --git a/codex-rs/core-plugins/src/remote/share/tests.rs b/codex-rs/core-plugins/src/remote/share/tests.rs index ea48286bad..84e698f9dc 100644 --- a/codex-rs/core-plugins/src/remote/share/tests.rs +++ b/codex-rs/core-plugins/src/remote/share/tests.rs @@ -107,15 +107,17 @@ fn remote_plugin_json(plugin_id: &str) -> serde_json::Value { }) } -fn remote_plugin_json_with_share_url( +fn remote_plugin_json_with_share_url_and_principals( plugin_id: &str, share_url: Option<&str>, + share_principals: serde_json::Value, ) -> serde_json::Value { let mut plugin = remote_plugin_json(plugin_id); let serde_json::Value::Object(fields) = &mut plugin else { unreachable!("plugin json should be an object"); }; fields.insert("share_url".to_string(), json!(share_url)); + fields.insert("share_principals".to_string(), share_principals); plugin } @@ -489,9 +491,23 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() { )) .and(query_param_is_missing("pageToken")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "plugins": [remote_plugin_json_with_share_url( + "plugins": [remote_plugin_json_with_share_url_and_principals( "plugins_123", Some("https://chatgpt.example/plugins/share/share-key-1"), + json!([ + { + "principal_type": "user", + "principal_id": "user-owner", + "role": "owner", + "name": "Owner", + }, + { + "principal_type": "user", + "principal_id": "user-reader", + "role": "reader", + "name": "Reader", + }, + ]), )], "pagination": { "next_page_token": "page-2" @@ -510,7 +526,29 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() { )) .and(query_param("pageToken", "page-2")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "plugins": [remote_plugin_json_with_share_url("plugins_456", /*share_url*/ None)], + "plugins": [remote_plugin_json_with_share_url_and_principals( + "plugins_456", + /*share_url*/ None, + json!([ + { + "principal_type": "user", + "principal_id": "user-owner", + "role": "owner", + "name": "Owner", + }, + { + "principal_type": "user", + "principal_id": "user-editor", + "role": "editor", + "name": "Editor", + }, + { + "principal_type": "user", + "principal_id": "user-missing-role", + "name": "Missing Role", + }, + ]), + )], "pagination": empty_pagination_json(), }))) .expect(1) @@ -540,8 +578,16 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() { name: "demo-plugin".to_string(), share_context: Some(RemotePluginShareContext { remote_plugin_id: "plugins_123".to_string(), + share_url: Some( + "https://chatgpt.example/plugins/share/share-key-1".to_string(), + ), creator_account_user_id: None, creator_name: None, + share_targets: Some(vec![RemotePluginSharePrincipal { + principal_type: RemotePluginSharePrincipalType::User, + principal_id: "user-reader".to_string(), + name: "Reader".to_string(), + }]), }), installed: false, enabled: false, @@ -560,8 +606,10 @@ async fn list_remote_plugin_shares_fetches_created_workspace_plugins() { name: "demo-plugin".to_string(), share_context: Some(RemotePluginShareContext { remote_plugin_id: "plugins_456".to_string(), + share_url: None, creator_account_user_id: None, creator_name: None, + share_targets: Some(Vec::new()), }), installed: true, enabled: true, From 857e731478fe4540cc84a155016a286b877b13a8 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 7 May 2026 10:16:10 -0700 Subject: [PATCH 02/10] [codex] Remove string-keyed MCP tool maps (#21454) ## Summary This PR removes the synthetic `HashMap` keys from MCP tool discovery. `McpConnectionManager::list_all_tools()` now returns normalized `Vec`, and downstream code derives identity from `ToolInfo::canonical_tool_name()`. The motivation is to keep model-visible tool identity on `ToolName`/`ToolInfo` instead of parallel string map keys, so future namespace changes do not have to preserve otherwise-unused lookup keys. ## Changes - Rename the MCP normalization path from `qualify_tools` to `normalize_tools_for_model` and return tool values directly. - Flow MCP tool lists through connectors, plugin injection, router/spec building, code mode, and tool search as vectors/slices. - Keep direct/deferred subtraction local to `mcp_tool_exposure`, using `ToolName` values. - Update tests to compare `ToolName` instances where MCP identity matters. ## Validation - `cargo test -p codex-mcp test_normalize_tools` - `cargo test -p codex-core mcp_tool_exposure` - `cargo test -p codex-core direct_mcp_tools_register_namespaced_handlers` - `cargo test -p codex-core search_tool_registers_namespaced_mcp_tool_aliases` - `just fix -p codex-mcp` - `just fix -p codex-core` --- codex-rs/codex-mcp/src/codex_apps.rs | 11 - codex-rs/codex-mcp/src/connection_manager.rs | 17 +- .../codex-mcp/src/connection_manager_tests.rs | 146 +++++---- codex-rs/codex-mcp/src/lib.rs | 1 - codex-rs/codex-mcp/src/mcp/mod.rs | 2 +- codex-rs/codex-mcp/src/tools.rs | 30 +- codex-rs/core/src/connectors.rs | 8 +- codex-rs/core/src/connectors_tests.rs | 124 +++---- codex-rs/core/src/mcp_tool_call.rs | 4 +- codex-rs/core/src/mcp_tool_exposure.rs | 32 +- codex-rs/core/src/mcp_tool_exposure_test.rs | 142 ++++---- codex-rs/core/src/plugins/injection.rs | 5 +- codex-rs/core/src/session/turn.rs | 6 +- .../core/src/tools/handlers/tool_search.rs | 31 +- codex-rs/core/src/tools/router.rs | 5 +- codex-rs/core/src/tools/spec.rs | 16 +- codex-rs/core/src/tools/spec_plan.rs | 12 +- codex-rs/core/src/tools/spec_tests.rs | 305 ++++++++---------- codex-rs/core/src/tools/tool_search_entry.rs | 7 +- codex-rs/core/src/unavailable_tool.rs | 26 +- 20 files changed, 459 insertions(+), 471 deletions(-) diff --git a/codex-rs/codex-mcp/src/codex_apps.rs b/codex-rs/codex-mcp/src/codex_apps.rs index 0a7981fb0d..81643e6665 100644 --- a/codex-rs/codex-mcp/src/codex_apps.rs +++ b/codex-rs/codex-mcp/src/codex_apps.rs @@ -5,7 +5,6 @@ //! connector allow-list filtering, and the normalization that turns app //! connector/tool metadata into model-visible MCP callable names. -use std::collections::HashMap; use std::path::PathBuf; use std::time::Instant; @@ -38,16 +37,6 @@ pub fn codex_apps_tools_cache_key(auth: Option<&CodexAuth>) -> CodexAppsToolsCac } } -pub fn filter_non_codex_apps_mcp_tools_only( - mcp_tools: &HashMap, -) -> HashMap { - mcp_tools - .iter() - .filter(|(_, tool)| tool.server_name != CODEX_APPS_MCP_SERVER_NAME) - .map(|(name, tool)| (name.clone(), tool.clone())) - .collect() -} - #[derive(Clone)] pub(crate) struct CodexAppsToolsCacheContext { pub(crate) codex_home: PathBuf, diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index f85d28d6de..e02b6094b3 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -34,7 +34,7 @@ use crate::server::EffectiveMcpServer; use crate::server::McpServerMetadata; use crate::tools::ToolInfo; use crate::tools::filter_tools; -use crate::tools::qualify_tools; +use crate::tools::normalize_tools_for_model; use crate::tools::tool_with_model_visible_input_schema; use anyhow::Context; use anyhow::Result; @@ -363,10 +363,9 @@ impl McpConnectionManager { failures } - /// Returns a single map that contains all tools. Each key is the - /// fully-qualified name for the tool. + /// Returns all tools with model-visible names normalized. #[instrument(level = "trace", skip_all)] - pub async fn list_all_tools(&self) -> HashMap { + pub async fn list_all_tools(&self) -> Vec { let mut tools = Vec::new(); for managed_client in self.clients.values() { let Some(server_tools) = managed_client.listed_tools().await else { @@ -374,15 +373,15 @@ impl McpConnectionManager { }; tools.extend(server_tools); } - qualify_tools(tools) + normalize_tools_for_model(tools) } /// Force-refresh codex apps tools by bypassing the in-process cache. /// /// On success, the refreshed tools replace the cache contents and the - /// latest filtered tool map is returned directly to the caller. On + /// latest filtered tools are returned directly to the caller. On /// failure, the existing cache remains unchanged. - pub async fn hard_refresh_codex_apps_tools_cache(&self) -> Result> { + pub async fn hard_refresh_codex_apps_tools_cache(&self) -> Result> { let managed_client = self .clients .get(CODEX_APPS_MCP_SERVER_NAME) @@ -425,7 +424,7 @@ impl McpConnectionManager { tool.tool = tool_with_model_visible_input_schema(&tool.tool); tool }); - Ok(qualify_tools(tools)) + Ok(normalize_tools_for_model(tools)) } /// Returns a single map that contains all resources. Each key is the @@ -664,7 +663,7 @@ impl McpConnectionManager { pub async fn resolve_tool_info(&self, tool_name: &ToolName) -> Option { let all_tools = self.list_all_tools().await; all_tools - .into_values() + .into_iter() .find(|tool| tool.canonical_tool_name() == *tool_name) } diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 2d370fcef1..4835bc5705 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -14,7 +14,7 @@ use crate::rmcp_client::elicitation_capability_for_server; use crate::tools::ToolFilter; use crate::tools::ToolInfo; use crate::tools::filter_tools; -use crate::tools::qualify_tools; +use crate::tools::normalize_tools_for_model; use crate::tools::tool_with_model_visible_input_schema; use codex_config::Constrained; use codex_config::McpServerConfig; @@ -86,6 +86,13 @@ fn create_codex_apps_tools_cache_context( } } +fn model_tool_names(tools: &[ToolInfo]) -> HashSet { + tools + .iter() + .map(ToolInfo::canonical_tool_name) + .collect::>() +} + #[test] fn declared_openai_file_fields_treat_names_literally() { let meta = serde_json::json!({ @@ -273,35 +280,41 @@ async fn disabled_permissions_do_not_auto_accept_elicitation_with_requested_fiel } #[test] -fn test_qualify_tools_short_non_duplicated_names() { +fn test_normalize_tools_short_non_duplicated_names() { let tools = vec![ create_test_tool("server1", "tool1"), create_test_tool("server1", "tool2"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); - assert!(qualified_tools.contains_key("mcp__server1__tool1")); - assert!(qualified_tools.contains_key("mcp__server1__tool2")); + assert_eq!( + model_tool_names(&model_tools), + HashSet::from([ + ToolName::namespaced("mcp__server1__", "tool1"), + ToolName::namespaced("mcp__server1__", "tool2") + ]) + ); } #[test] -fn test_qualify_tools_duplicated_names_skipped() { +fn test_normalize_tools_duplicated_names_skipped() { let tools = vec![ create_test_tool("server1", "duplicate_tool"), create_test_tool("server1", "duplicate_tool"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); // Only the first tool should remain, the second is skipped - assert_eq!(qualified_tools.len(), 1); - assert!(qualified_tools.contains_key("mcp__server1__duplicate_tool")); + assert_eq!( + model_tool_names(&model_tools), + HashSet::from([ToolName::namespaced("mcp__server1__", "duplicate_tool")]) + ); } #[test] -fn test_qualify_tools_long_names_same_server() { +fn test_normalize_tools_long_names_same_server() { let server_name = "my_server"; let tools = vec![ @@ -315,116 +328,131 @@ fn test_qualify_tools_long_names_same_server() { ), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); + assert_eq!(model_tools.len(), 2); - let mut keys: Vec<_> = qualified_tools.keys().cloned().collect(); - keys.sort(); + let names = model_tool_names(&model_tools); - assert!(keys.iter().all(|key| key.len() == 64)); - assert!(keys.iter().all(|key| key.starts_with("mcp__my_server__"))); + assert!(names.iter().all(|name| name.display().len() == 64)); assert!( - keys.iter() - .all(|key| key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), - "qualified names must be code-mode compatible: {keys:?}" + names + .iter() + .all(|name| name.namespace.as_deref() == Some("mcp__my_server__")) + ); + assert!( + names.iter().all(|name| name + .display() + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')), + "model-visible names must be code-mode compatible: {names:?}" ); } #[test] -fn test_qualify_tools_sanitizes_invalid_characters() { +fn test_normalize_tools_sanitizes_invalid_characters() { let tools = vec![create_test_tool("server.one", "tool.two-three")]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 1); - let (qualified_name, tool) = qualified_tools.into_iter().next().expect("one tool"); - assert_eq!(qualified_name, "mcp__server_one__tool_two_three"); + assert_eq!(model_tools.len(), 1); + let tool = model_tools.into_iter().next().expect("one tool"); + let model_name = tool.canonical_tool_name(); + assert_eq!( + model_name, + ToolName::namespaced("mcp__server_one__", "tool_two_three") + ); assert_eq!( format!("{}{}", tool.callable_namespace, tool.callable_name), - qualified_name + model_name.display() ); - // The key and callable parts are sanitized for model-visible tool calls, but - // the raw MCP name is preserved for the actual MCP call. + // The callable parts are sanitized for model-visible tool calls, but the raw + // MCP name is preserved for the actual MCP call. assert_eq!(tool.server_name, "server.one"); assert_eq!(tool.callable_namespace, "mcp__server_one__"); assert_eq!(tool.callable_name, "tool_two_three"); assert_eq!(tool.tool.name, "tool.two-three"); assert!( - qualified_name + model_name + .display() .chars() .all(|c| c.is_ascii_alphanumeric() || c == '_'), - "qualified name must be code-mode compatible: {qualified_name:?}" + "model-visible name must be code-mode compatible: {model_name:?}" ); } #[test] -fn test_qualify_tools_keeps_hyphenated_mcp_tools_callable() { +fn test_normalize_tools_keeps_hyphenated_mcp_tools_callable() { let tools = vec![create_test_tool("music-studio", "get-strudel-guide")]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 1); - let (qualified_name, tool) = qualified_tools.into_iter().next().expect("one tool"); - assert_eq!(qualified_name, "mcp__music_studio__get_strudel_guide"); + assert_eq!(model_tools.len(), 1); + let tool = model_tools.into_iter().next().expect("one tool"); + assert_eq!( + tool.canonical_tool_name(), + ToolName::namespaced("mcp__music_studio__", "get_strudel_guide") + ); assert_eq!(tool.callable_namespace, "mcp__music_studio__"); assert_eq!(tool.callable_name, "get_strudel_guide"); assert_eq!(tool.tool.name, "get-strudel-guide"); } #[test] -fn test_qualify_tools_disambiguates_sanitized_namespace_collisions() { +fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() { let tools = vec![ create_test_tool("basic-server", "lookup"), create_test_tool("basic_server", "query"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); - let mut namespaces = qualified_tools - .values() + assert_eq!(model_tools.len(), 2); + let mut namespaces = model_tools + .iter() .map(|tool| tool.callable_namespace.as_str()) .collect::>(); namespaces.sort(); namespaces.dedup(); assert_eq!(namespaces.len(), 2); - let raw_servers = qualified_tools - .values() + let raw_servers = model_tools + .iter() .map(|tool| tool.server_name.as_str()) .collect::>(); assert_eq!(raw_servers, HashSet::from(["basic-server", "basic_server"])); + let model_names = model_tool_names(&model_tools); assert!( - qualified_tools - .keys() - .all(|key| key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), - "qualified names must be code-mode compatible: {qualified_tools:?}" + model_names.iter().all(|name| name + .display() + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')), + "model-visible names must be code-mode compatible: {model_names:?}" ); } #[test] -fn test_qualify_tools_disambiguates_sanitized_tool_name_collisions() { +fn test_normalize_tools_disambiguates_sanitized_tool_name_collisions() { let tools = vec![ create_test_tool("server", "tool-name"), create_test_tool("server", "tool_name"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); - let raw_tool_names = qualified_tools - .values() + assert_eq!(model_tools.len(), 2); + let raw_tool_names = model_tools + .iter() .map(|tool| tool.tool.name.to_string()) .collect::>(); assert_eq!( raw_tool_names, HashSet::from(["tool-name".to_string(), "tool_name".to_string()]) ); - let callable_tool_names = qualified_tools - .values() + let callable_tool_names = model_tools + .iter() .map(|tool| tool.callable_name.as_str()) .collect::>(); assert_eq!(callable_tool_names.len(), 2); @@ -673,7 +701,11 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { let tools = manager.list_all_tools().await; let tool = tools - .get("mcp__codex_apps__calendar_create_event") + .iter() + .find(|tool| { + tool.canonical_tool_name() + == ToolName::namespaced("mcp__codex_apps__", "calendar_create_event") + }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(tool.callable_name, "calendar_create_event"); @@ -799,7 +831,11 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { let tools = manager.list_all_tools().await; let tool = tools - .get("mcp__codex_apps__calendar_create_event") + .iter() + .find(|tool| { + tool.canonical_tool_name() + == ToolName::namespaced("mcp__codex_apps__", "calendar_create_event") + }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(tool.callable_name, "calendar_create_event"); diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index c9ea326571..1919799697 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -51,7 +51,6 @@ pub use mcp::oauth_login_support; pub use mcp::resolve_oauth_scopes; pub use mcp::should_retry_without_scopes; -pub use codex_apps::filter_non_codex_apps_mcp_tools_only; pub use mcp::McpPermissionPromptAutoApproveContext; pub use mcp::mcp_permission_prompt_is_auto_approved; pub use mcp::qualified_mcp_tool_name_prefix; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 1be06e4bae..71f79b9b4a 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -583,7 +583,7 @@ async fn collect_mcp_server_status_snapshot_from_manager( ); let mut tools_by_server = HashMap::>::new(); - for (_qualified_name, tool_info) in tools { + for tool_info in tools { let raw_tool_name = tool_info.tool.name.to_string(); let Some(tool) = protocol_tool_from_rmcp_tool(&raw_tool_name, &tool_info.tool) else { continue; diff --git a/codex-rs/codex-mcp/src/tools.rs b/codex-rs/codex-mcp/src/tools.rs index 1a3e1a6bc2..cb3d8babae 100644 --- a/codex-rs/codex-mcp/src/tools.rs +++ b/codex-rs/codex-mcp/src/tools.rs @@ -1,4 +1,4 @@ -//! MCP tool metadata, filtering, schema shaping, and name qualification. +//! MCP tool metadata, filtering, schema shaping, and name normalization. //! //! Raw MCP tool identities must be preserved for protocol calls, while //! model-visible tool names must be sanitized, deduplicated, and kept within API @@ -130,12 +130,12 @@ pub(crate) fn filter_tools(tools: Vec, filter: &ToolFilter) -> Vec(tools: I) -> HashMap +/// every model-visible name is unique and <= 64 bytes. +pub(crate) fn normalize_tools_for_model(tools: I) -> Vec where I: IntoIterator, { @@ -213,9 +213,9 @@ where candidates.sort_by(|left, right| left.raw_tool_identity.cmp(&right.raw_tool_identity)); let mut used_names = HashSet::new(); - let mut qualified_tools = HashMap::new(); + let mut model_tools = Vec::new(); for mut candidate in candidates { - let (callable_namespace, callable_name, qualified_name) = unique_callable_parts( + let (callable_namespace, callable_name) = unique_callable_parts( &candidate.callable_namespace, &candidate.callable_name, &candidate.raw_tool_identity, @@ -223,9 +223,9 @@ where ); candidate.tool.callable_namespace = callable_namespace; candidate.tool.callable_name = callable_name; - qualified_tools.insert(qualified_name, candidate.tool); + model_tools.push(candidate.tool); } - qualified_tools + model_tools } #[derive(Debug)] @@ -345,10 +345,10 @@ fn unique_callable_parts( tool_name: &str, raw_identity: &str, used_names: &mut HashSet, -) -> (String, String, String) { - let qualified_name = format!("{namespace}{tool_name}"); - if qualified_name.len() <= MAX_TOOL_NAME_LENGTH && used_names.insert(qualified_name.clone()) { - return (namespace.to_string(), tool_name.to_string(), qualified_name); +) -> (String, String) { + let model_name = format!("{namespace}{tool_name}"); + if model_name.len() <= MAX_TOOL_NAME_LENGTH && used_names.insert(model_name) { + return (namespace.to_string(), tool_name.to_string()); } let mut attempt = 0_u32; @@ -360,9 +360,9 @@ fn unique_callable_parts( }; let (namespace, tool_name) = fit_callable_parts_with_hash(namespace, tool_name, &hash_input); - let qualified_name = format!("{namespace}{tool_name}"); - if used_names.insert(qualified_name.clone()) { - return (namespace, tool_name, qualified_name); + let model_name = format!("{namespace}{tool_name}"); + if used_names.insert(model_name) { + return (namespace, tool_name); } attempt = attempt.saturating_add(1); } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index a2ec6e93c0..4da588edb6 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -165,7 +165,7 @@ pub async fn list_cached_accessible_connectors_from_mcp_tools( pub(crate) fn refresh_accessible_connectors_cache_from_mcp_tools( config: &Config, auth: Option<&CodexAuth>, - mcp_tools: &HashMap, + mcp_tools: &[ToolInfo], ) { if !config.features.enabled(Feature::Apps) { return; @@ -517,12 +517,10 @@ async fn chatgpt_get_request_with_auth_provider( } } -pub(crate) fn accessible_connectors_from_mcp_tools( - mcp_tools: &HashMap, -) -> Vec { +pub(crate) fn accessible_connectors_from_mcp_tools(mcp_tools: &[ToolInfo]) -> Vec { // ToolInfo already carries plugin provenance, so app-level plugin sources // can be derived here instead of requiring a separate enrichment pass. - let tools = mcp_tools.values().filter_map(|tool| { + let tools = mcp_tools.iter().filter_map(|tool| { if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { return None; } diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index 513f677e9b..014ab1cad8 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -172,39 +172,30 @@ fn merge_connectors_replaces_plugin_placeholder_name_with_accessible_name() { #[test] fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() { - let tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_list_events".to_string(), - codex_app_tool( - "calendar_list_events", - "calendar", - /*connector_name*/ None, - &["sample", "sample"], - ), + let tools = vec![ + codex_app_tool( + "calendar_list_events", + "calendar", + /*connector_name*/ None, + &["sample", "sample"], ), - ( - "mcp__codex_apps__calendar_create_event".to_string(), - codex_app_tool( - "calendar_create_event", - "calendar", - Some("Google Calendar"), - &["beta", "sample"], - ), + codex_app_tool( + "calendar_create_event", + "calendar", + Some("Google Calendar"), + &["beta", "sample"], ), - ( - "mcp__sample__echo".to_string(), - ToolInfo { - server_name: "sample".to_string(), - callable_name: "echo".to_string(), - callable_namespace: "sample".to_string(), - namespace_description: None, - tool: test_tool_definition("echo"), - connector_id: None, - connector_name: None, - plugin_display_names: plugin_names(&["ignored"]), - }, - ), - ]); + ToolInfo { + server_name: "sample".to_string(), + callable_name: "echo".to_string(), + callable_namespace: "sample".to_string(), + namespace_description: None, + tool: test_tool_definition("echo"), + connector_id: None, + connector_name: None, + plugin_display_names: plugin_names(&["ignored"]), + }, + ]; let connectors = accessible_connectors_from_mcp_tools(&tools); @@ -238,26 +229,20 @@ async fn refresh_accessible_connectors_cache_from_mcp_tools_writes_latest_instal .expect("config should load"); let _ = config.features.set_enabled(Feature::Apps, /*enabled*/ true); let cache_key = accessible_connectors_cache_key(&config, /*auth*/ None); - let tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_list_events".to_string(), - codex_app_tool( - "calendar_list_events", - "calendar", - Some("Google Calendar"), - &["calendar-plugin"], - ), + let tools = vec![ + codex_app_tool( + "calendar_list_events", + "calendar", + Some("Google Calendar"), + &["calendar-plugin"], ), - ( - "mcp__codex_apps__openai_hidden".to_string(), - codex_app_tool( - "openai_hidden", - "connector_openai_hidden", - Some("Hidden"), - &[], - ), + codex_app_tool( + "openai_hidden", + "connector_openai_hidden", + Some("Hidden"), + &[], ), - ]); + ]; let cached = with_accessible_connectors_cache_cleared(|| { refresh_accessible_connectors_cache_from_mcp_tools(&config, /*auth*/ None, &tools); @@ -315,29 +300,26 @@ fn merge_connectors_unions_and_dedupes_plugin_display_names() { #[test] fn accessible_connectors_from_mcp_tools_preserves_description() { - let mcp_tools = HashMap::from([( - "mcp__codex_apps__calendar_create_event".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "calendar_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: Some("Plan events".to_string()), - tool: Tool { - name: "calendar_create_event".to_string().into(), - title: None, - description: Some("Create a calendar event".into()), - input_schema: Arc::new(JsonObject::default()), - output_schema: None, - annotations: None, - execution: None, - icons: None, - meta: None, - }, - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), + let mcp_tools = vec![ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "calendar_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: Some("Plan events".to_string()), + tool: Tool { + name: "calendar_create_event".to_string().into(), + title: None, + description: Some("Create a calendar event".into()), + input_schema: Arc::new(JsonObject::default()), + output_schema: None, + annotations: None, + execution: None, + icons: None, + meta: None, }, - )]); + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }]; assert_eq!( accessible_connectors_from_mcp_tools(&mcp_tools), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 9c01ad48bc..5dd16acb9a 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1490,7 +1490,7 @@ pub(crate) async fn lookup_mcp_tool_metadata( .list_all_tools() .await; let tool_info = tools - .into_values() + .into_iter() .find(|tool_info| tool_info.server_name == server && tool_info.tool.name == tool_name)?; let connector_description = if server == CODEX_APPS_MCP_SERVER_NAME { let connectors = match connectors::list_cached_accessible_connectors_from_mcp_tools( @@ -1585,7 +1585,7 @@ async fn lookup_mcp_app_usage_metadata( .list_all_tools() .await; - tools.into_values().find_map(|tool_info| { + tools.into_iter().find_map(|tool_info| { if tool_info.server_name == server && tool_info.tool.name == tool_name { Some(McpAppUsageMetadata { connector_id: tool_info.connector_id, diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs index 3917a1d5e7..0bf696acdf 100644 --- a/codex-rs/core/src/mcp_tool_exposure.rs +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -1,10 +1,8 @@ -use std::collections::HashMap; use std::collections::HashSet; use codex_features::Feature; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::ToolInfo as McpToolInfo; -use codex_mcp::filter_non_codex_apps_mcp_tools_only; use codex_tools::ToolsConfig; use crate::config::Config; @@ -13,12 +11,12 @@ use crate::connectors; pub(crate) const DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD: usize = 100; pub(crate) struct McpToolExposure { - pub(crate) direct_tools: HashMap, - pub(crate) deferred_tools: Option>, + pub(crate) direct_tools: Vec, + pub(crate) deferred_tools: Option>, } pub(crate) fn build_mcp_tool_exposure( - all_mcp_tools: &HashMap, + all_mcp_tools: &[McpToolInfo], connectors: Option<&[connectors::AppInfo]>, explicitly_enabled_connectors: &[connectors::AppInfo], config: &Config, @@ -48,9 +46,11 @@ pub(crate) fn build_mcp_tool_exposure( let direct_tools = filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); - for direct_tool_name in direct_tools.keys() { - deferred_tools.remove(direct_tool_name); - } + let direct_tool_names = direct_tools + .iter() + .map(McpToolInfo::canonical_tool_name) + .collect::>(); + deferred_tools.retain(|tool| !direct_tool_names.contains(&tool.canonical_tool_name())); McpToolExposure { direct_tools, @@ -58,11 +58,19 @@ pub(crate) fn build_mcp_tool_exposure( } } +fn filter_non_codex_apps_mcp_tools_only(mcp_tools: &[McpToolInfo]) -> Vec { + mcp_tools + .iter() + .filter(|tool| tool.server_name != CODEX_APPS_MCP_SERVER_NAME) + .cloned() + .collect() +} + fn filter_codex_apps_mcp_tools( - mcp_tools: &HashMap, + mcp_tools: &[McpToolInfo], connectors: &[connectors::AppInfo], config: &Config, -) -> HashMap { +) -> Vec { let allowed: HashSet<&str> = connectors .iter() .map(|connector| connector.id.as_str()) @@ -70,7 +78,7 @@ fn filter_codex_apps_mcp_tools( mcp_tools .iter() - .filter(|(_, tool)| { + .filter(|tool| { if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { return false; } @@ -79,7 +87,7 @@ fn filter_codex_apps_mcp_tools( }; allowed.contains(connector_id) && connectors::codex_app_tool_is_enabled(config, tool) }) - .map(|(name, tool)| (name.clone(), tool.clone())) + .cloned() .collect() } diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index 32707e4f8b..c3f467800a 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -1,7 +1,6 @@ -use std::collections::HashMap; +use std::collections::HashSet; use std::sync::Arc; -use codex_connectors::metadata::sanitize_name; use codex_features::Feature; use codex_features::Features; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; @@ -11,6 +10,7 @@ use codex_protocol::config_types::WebSearchMode; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SessionSource; +use codex_tools::ToolName; use codex_tools::ToolsConfig; use codex_tools::ToolsConfigParams; use pretty_assertions::assert_eq; @@ -42,22 +42,15 @@ fn make_connector(id: &str, name: &str) -> AppInfo { fn make_mcp_tool( server_name: &str, tool_name: &str, + callable_namespace: &str, + callable_name: &str, connector_id: Option<&str>, connector_name: Option<&str>, ) -> ToolInfo { - let tool_namespace = if server_name == CODEX_APPS_MCP_SERVER_NAME { - connector_name - .map(sanitize_name) - .map(|connector_name| format!("mcp__{server_name}__{connector_name}")) - .unwrap_or_else(|| server_name.to_string()) - } else { - format!("mcp__{server_name}__") - }; - ToolInfo { server_name: server_name.to_string(), - callable_name: tool_name.to_string(), - callable_namespace: tool_namespace, + callable_name: callable_name.to_string(), + callable_namespace: callable_namespace.to_string(), namespace_description: None, tool: Tool { name: tool_name.to_string().into(), @@ -76,20 +69,29 @@ fn make_mcp_tool( } } -fn numbered_mcp_tools(count: usize) -> HashMap { +fn numbered_mcp_tools(count: usize) -> Vec { (0..count) .map(|index| { let tool_name = format!("tool_{index}"); - ( - format!("mcp__rmcp__{tool_name}"), - make_mcp_tool( - "rmcp", &tool_name, /*connector_id*/ None, /*connector_name*/ None, - ), + make_mcp_tool( + "rmcp", + &tool_name, + "mcp__rmcp__", + &tool_name, + /*connector_id*/ None, + /*connector_name*/ None, ) }) .collect() } +fn tool_names(tools: &[ToolInfo]) -> HashSet { + tools + .iter() + .map(codex_mcp::ToolInfo::canonical_tool_name) + .collect() +} + async fn tools_config_for_mcp_tool_exposure(search_tool: bool) -> ToolsConfig { let config = test_config().await; let model_info = @@ -124,11 +126,7 @@ async fn directly_exposes_small_effective_tool_sets() { &tools_config, ); - let mut direct_tool_names: Vec<_> = exposure.direct_tools.keys().cloned().collect(); - direct_tool_names.sort(); - let mut expected_tool_names: Vec<_> = mcp_tools.keys().cloned().collect(); - expected_tool_names.sort(); - assert_eq!(direct_tool_names, expected_tool_names); + assert_eq!(tool_names(&exposure.direct_tools), tool_names(&mcp_tools)); assert!(exposure.deferred_tools.is_none()); } @@ -151,11 +149,7 @@ async fn searches_large_effective_tool_sets() { .deferred_tools .as_ref() .expect("large tool sets should be discoverable through tool_search"); - let mut deferred_tool_names: Vec<_> = deferred_tools.keys().cloned().collect(); - deferred_tool_names.sort(); - let mut expected_tool_names: Vec<_> = mcp_tools.keys().cloned().collect(); - expected_tool_names.sort(); - assert_eq!(deferred_tool_names, expected_tool_names); + assert_eq!(tool_names(deferred_tools), tool_names(&mcp_tools)); } #[tokio::test] @@ -163,15 +157,14 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { let config = test_config().await; let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mut mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); - mcp_tools.extend([( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), - )]); + mcp_tools.push(make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_create_event", + "mcp__codex_apps__calendar", + "_create_event", + Some("calendar"), + Some("Calendar"), + )); let connectors = vec![make_connector("calendar", "Calendar")]; let exposure = build_mcp_tool_exposure( @@ -182,28 +175,32 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { &tools_config, ); - let mut tool_names: Vec = exposure.direct_tools.into_keys().collect(); - tool_names.sort(); + let direct_tool_names = tool_names(&exposure.direct_tools); assert_eq!( - tool_names, - vec!["mcp__codex_apps__calendar_create_event".to_string()] + direct_tool_names, + HashSet::from([ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + )]) ); assert_eq!( - exposure.deferred_tools.as_ref().map(HashMap::len), + exposure.deferred_tools.as_ref().map(Vec::len), Some(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1) ); let deferred_tools = exposure .deferred_tools .as_ref() .expect("large tool sets should be discoverable through tool_search"); + let deferred_tool_names = tool_names(deferred_tools); assert!( - tool_names - .iter() - .all(|direct_tool_name| !deferred_tools.contains_key(direct_tool_name)), - "direct tools should not also be deferred: {tool_names:?}" + direct_tool_names.is_disjoint(&deferred_tool_names), + "direct tools should not also be deferred: {direct_tool_names:?}" ); - assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); - assert!(deferred_tools.contains_key("mcp__rmcp__tool_0")); + assert!(!deferred_tool_names.contains(&ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + ))); + assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool_0"))); } #[tokio::test] @@ -214,23 +211,24 @@ async fn always_defer_feature_preserves_explicit_apps() { .enable(Feature::ToolSearchAlwaysDeferMcpTools) .expect("test config should allow feature update"); let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; - let mcp_tools = HashMap::from([ - ( - "mcp__rmcp__tool".to_string(), - make_mcp_tool( - "rmcp", "tool", /*connector_id*/ None, /*connector_name*/ None, - ), + let mcp_tools = vec![ + make_mcp_tool( + "rmcp", + "tool", + "mcp__rmcp__", + "tool", + /*connector_id*/ None, + /*connector_name*/ None, ), - ( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), + make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_create_event", + "mcp__codex_apps__calendar", + "_create_event", + Some("calendar"), + Some("Calendar"), ), - ]); + ]; let connectors = vec![make_connector("calendar", "Calendar")]; let exposure = build_mcp_tool_exposure( @@ -241,16 +239,22 @@ async fn always_defer_feature_preserves_explicit_apps() { &tools_config, ); - let mut direct_tool_names: Vec = exposure.direct_tools.into_keys().collect(); - direct_tool_names.sort(); + let direct_tool_names = tool_names(&exposure.direct_tools); assert_eq!( direct_tool_names, - vec!["mcp__codex_apps__calendar_create_event".to_string()] + HashSet::from([ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + )]) ); let deferred_tools = exposure .deferred_tools .as_ref() .expect("MCP tools should be discoverable through tool_search"); - assert!(deferred_tools.contains_key("mcp__rmcp__tool")); - assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); + let deferred_tool_names = tool_names(deferred_tools); + assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool"))); + assert!(!deferred_tool_names.contains(&ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + ))); } diff --git a/codex-rs/core/src/plugins/injection.rs b/codex-rs/core/src/plugins/injection.rs index 4ce50631fb..48e15247b5 100644 --- a/codex-rs/core/src/plugins/injection.rs +++ b/codex-rs/core/src/plugins/injection.rs @@ -1,5 +1,4 @@ use std::collections::BTreeSet; -use std::collections::HashMap; use codex_connectors::metadata::connector_display_label; use codex_protocol::models::ResponseItem; @@ -14,7 +13,7 @@ use codex_mcp::ToolInfo; pub(crate) fn build_plugin_injections( mentioned_plugins: &[PluginCapabilitySummary], - mcp_tools: &HashMap, + mcp_tools: &[ToolInfo], available_connectors: &[connectors::AppInfo], ) -> Vec { if mentioned_plugins.is_empty() { @@ -27,7 +26,7 @@ pub(crate) fn build_plugin_injections( .iter() .filter_map(|plugin| { let available_mcp_servers = mcp_tools - .values() + .iter() .filter(|tool| { tool.server_name != CODEX_APPS_MCP_SERVER_NAME && tool diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 000bbc8ba6..1723904cff 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -196,10 +196,10 @@ pub(crate) async fn run_turn( { Ok(mcp_tools) => mcp_tools, Err(_) if turn_context.apps_enabled() => return None, - Err(_) => HashMap::new(), + Err(_) => Vec::new(), } } else { - HashMap::new() + Vec::new() }; let available_connectors = if turn_context.apps_enabled() { let connectors = codex_connectors::merge::merge_plugin_connectors_with_accessible( @@ -1253,7 +1253,7 @@ pub(crate) async fn built_tools( let exposed_tool_names = mcp_tools .iter() .chain(deferred_mcp_tools.iter()) - .flat_map(|tools| tools.keys().map(String::as_str)) + .flat_map(|tools| tools.iter().map(codex_mcp::ToolInfo::canonical_tool_name)) .collect::>(); collect_unavailable_called_tools(input, &exposed_tool_names) } else { diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index 59deb54116..bc2e727dfb 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -204,19 +204,11 @@ mod tests { }), defer_loading: true, }]; - let handler = handler_from_tools( - Some(&std::collections::HashMap::from([ - ( - "mcp__calendar__create_event".to_string(), - tool_info("calendar", "create_event", "Create events"), - ), - ( - "mcp__calendar__list_events".to_string(), - tool_info("calendar", "list_events", "List events"), - ), - ])), - &dynamic_tools, - ); + let mcp_tools = vec![ + tool_info("calendar", "create_event", "Create events"), + tool_info("calendar", "list_events", "List events"), + ]; + let handler = handler_from_tools(Some(&mcp_tools), &dynamic_tools); let results = [ &handler.entries[0], &handler.entries[2], @@ -376,18 +368,11 @@ mod tests { assert!(count_results_for_server(&results, "other-server") <= TOOL_SEARCH_DEFAULT_LIMIT); } - fn numbered_tools( - server_name: &str, - description_prefix: &str, - count: usize, - ) -> std::collections::HashMap { + fn numbered_tools(server_name: &str, description_prefix: &str, count: usize) -> Vec { (0..count) .map(|index| { let tool_name = format!("tool_{index:03}"); - ( - format!("mcp__{server_name}__{tool_name}"), - tool_info(server_name, &tool_name, description_prefix), - ) + tool_info(server_name, &tool_name, description_prefix) }) .collect() } @@ -427,7 +412,7 @@ mod tests { } fn handler_from_tools( - mcp_tools: Option<&std::collections::HashMap>, + mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> ToolSearchHandler { ToolSearchHandler::new(build_tool_search_entries(mcp_tools, dynamic_tools)) diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index aeba3b0556..609fedf964 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -21,7 +21,6 @@ use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_tools::ToolsConfig; -use std::collections::HashMap; use std::collections::HashSet; use std::sync::Arc; use tokio_util::sync::CancellationToken; @@ -44,8 +43,8 @@ pub struct ToolRouter { } pub(crate) struct ToolRouterParams<'a> { - pub(crate) mcp_tools: Option>, - pub(crate) deferred_mcp_tools: Option>, + pub(crate) mcp_tools: Option>, + pub(crate) deferred_mcp_tools: Option>, pub(crate) unavailable_called_tools: Vec, pub(crate) parallel_mcp_server_names: HashSet, pub(crate) discoverable_tools: Option>, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index d79fda73a3..b1dfb9f865 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -38,17 +38,17 @@ struct McpToolPlanInputs<'a> { tool_namespaces: HashMap, } -fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanInputs<'_> { +fn map_mcp_tools_for_plan(mcp_tools: &[ToolInfo]) -> McpToolPlanInputs<'_> { McpToolPlanInputs { mcp_tools: mcp_tools - .values() + .iter() .map(|tool| ToolRegistryBuildMcpTool { name: tool.canonical_tool_name(), tool: &tool.tool, }) .collect(), tool_namespaces: mcp_tools - .values() + .iter() .map(|tool| { ( tool.callable_namespace.clone(), @@ -64,8 +64,8 @@ fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanI pub(crate) fn build_specs_with_discoverable_tools( config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, + mcp_tools: Option>, + deferred_mcp_tools: Option>, unavailable_called_tools: Vec, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], @@ -74,10 +74,10 @@ pub(crate) fn build_specs_with_discoverable_tools( use crate::tools::handlers::unavailable_tool_message; use crate::tools::tool_search_entry::build_tool_search_entries_for_config; - let mcp_tool_plan_inputs = mcp_tools.as_ref().map(map_mcp_tools_for_plan); + let mcp_tool_plan_inputs = mcp_tools.as_deref().map(map_mcp_tools_for_plan); let deferred_mcp_tool_sources = deferred_mcp_tools.as_ref().map(|tools| { tools - .values() + .iter() .map(|tool| ToolRegistryBuildDeferredTool { name: tool.canonical_tool_name(), server_name: tool.server_name.as_str(), @@ -105,7 +105,7 @@ pub(crate) fn build_specs_with_discoverable_tools( .collect::>(); let tool_search_entries = build_tool_search_entries_for_config( config, - deferred_mcp_tools.as_ref(), + deferred_mcp_tools.as_deref(), &deferred_dynamic_tools, ); let mut builder = build_tool_registry_builder( diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index 61850c4157..bca9d549a7 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -101,6 +101,7 @@ use codex_tools::default_namespace_description; use codex_tools::dynamic_tool_to_loadable_tool_spec; use codex_tools::mcp_tool_to_responses_api_tool; use std::collections::BTreeMap; +use std::collections::HashSet; use std::sync::Arc; pub fn build_tool_registry_builder( @@ -617,11 +618,14 @@ pub fn build_tool_registry_builder( } if let Some(deferred_mcp_tools) = params.deferred_mcp_tools { + let directly_registered_mcp_tools = params + .mcp_tools + .into_iter() + .flatten() + .map(|direct| direct.name.clone()) + .collect::>(); for tool in deferred_mcp_tools { - let registered_directly = params - .mcp_tools - .is_some_and(|mcp_tools| mcp_tools.iter().any(|direct| direct.name == tool.name)); - if !registered_directly { + if !directly_registered_mcp_tools.contains(&tool.name) { builder.register_handler(Arc::new(McpHandler::new(tool.name.clone()))); } } diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 6bd796508d..24e03fbf8a 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -266,8 +266,8 @@ async fn model_info_from_models_json(slug: &str) -> ModelInfo { /// Builds the tool registry builder while collecting tool specs for later serialization. fn build_specs( config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, + mcp_tools: Option>, + deferred_mcp_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { build_specs_with_unavailable_tools( @@ -281,8 +281,8 @@ fn build_specs( fn build_specs_with_unavailable_tools( config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, + mcp_tools: Option>, + deferred_mcp_tools: Option>, unavailable_called_tools: Vec, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { @@ -630,7 +630,7 @@ async fn test_build_specs_default_shell_present() { }); let (tools, _) = build_specs( &tools_config, - Some(HashMap::new()), + Some(Vec::new()), /*deferred_mcp_tools*/ None, &[], ) @@ -856,7 +856,7 @@ async fn search_tool_description_handles_no_enabled_mcp_tools() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - Some(HashMap::new()), + Some(Vec::new()), &[], ) .build(); @@ -890,23 +890,20 @@ async fn search_tool_description_falls_back_to_connector_name_without_descriptio let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - Some(HashMap::from([( - "mcp__codex_apps__calendar_create_event".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: None, - tool: mcp_tool( - "calendar_create_event", - "Create calendar event", - serde_json::json!({"type": "object"}), - ), - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), - }, - )])), + Some(vec![ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: None, + tool: mcp_tool( + "calendar_create_event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }]), &[], ) .build(); @@ -940,55 +937,46 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() { let (_, registry) = build_specs( &tools_config, /*mcp_tools*/ None, - Some(HashMap::from([ - ( - "mcp__codex_apps__calendar_create_event".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: None, - tool: mcp_tool( - "calendar-create-event", - "Create calendar event", - serde_json::json!({"type": "object"}), - ), - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), - }, - ), - ( - "mcp__codex_apps__calendar_list_events".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_list_events".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: None, - tool: mcp_tool( - "calendar-list-events", - "List calendar events", - serde_json::json!({"type": "object"}), - ), - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), - }, - ), - ( - "mcp__rmcp__echo".to_string(), - ToolInfo { - server_name: "rmcp".to_string(), - callable_name: "echo".to_string(), - callable_namespace: "mcp__rmcp__".to_string(), - namespace_description: None, - tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), - connector_id: None, - connector_name: None, - plugin_display_names: Vec::new(), - }, - ), - ])), + Some(vec![ + ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: None, + tool: mcp_tool( + "calendar-create-event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }, + ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "_list_events".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: None, + tool: mcp_tool( + "calendar-list-events", + "List calendar events", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }, + ToolInfo { + server_name: "rmcp".to_string(), + callable_name: "echo".to_string(), + callable_namespace: "mcp__rmcp__".to_string(), + namespace_description: None, + tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), + connector_id: None, + connector_name: None, + plugin_display_names: Vec::new(), + }, + ]), &[], ) .build(); @@ -1018,14 +1006,11 @@ async fn tool_search_entries_skip_namespace_outputs_when_namespace_tools_are_dis windows_sandbox_level: WindowsSandboxLevel::Disabled, }); tools_config.namespace_tools = false; - let mcp_tools = HashMap::from([( - "mcp__test_server__echo".to_string(), - mcp_tool_info(mcp_tool( - "echo", - "Echo", - serde_json::json!({"type": "object"}), - )), - )]); + let mcp_tools = vec![mcp_tool_info(mcp_tool( + "echo", + "Echo", + serde_json::json!({"type": "object"}), + ))]; let dynamic_tools = vec![ DynamicToolSpec { namespace: Some("codex_app".to_string()), @@ -1077,14 +1062,11 @@ async fn direct_mcp_tools_register_namespaced_handlers() { let (_, registry) = build_specs( &tools_config, - Some(HashMap::from([( - "mcp__test_server__echo".to_string(), - mcp_tool_info(mcp_tool( - "echo", - "Echo", - serde_json::json!({"type": "object"}), - )), - )])), + Some(vec![mcp_tool_info(mcp_tool( + "echo", + "Echo", + serde_json::json!({"type": "object"}), + ))]), /*deferred_mcp_tools*/ None, &[], ) @@ -1163,22 +1145,19 @@ async fn test_mcp_tool_property_missing_type_defaults_to_string() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/search".to_string(), - mcp_tool_info_with_display_name( - "dash/search", - mcp_tool( - "search", - "Search docs", - serde_json::json!({ - "type": "object", - "properties": { - "query": {"description": "search query"} - } - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/search", + mcp_tool( + "search", + "Search docs", + serde_json::json!({ + "type": "object", + "properties": { + "query": {"description": "search query"} + } + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1226,20 +1205,17 @@ async fn test_mcp_tool_preserves_integer_schema() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/paginate".to_string(), - mcp_tool_info_with_display_name( - "dash/paginate", - mcp_tool( - "paginate", - "Pagination", - serde_json::json!({ - "type": "object", - "properties": {"page": {"type": "integer"}} - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/paginate", + mcp_tool( + "paginate", + "Pagination", + serde_json::json!({ + "type": "object", + "properties": {"page": {"type": "integer"}} + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1288,20 +1264,17 @@ async fn test_mcp_tool_array_without_items_gets_default_string_items() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/tags".to_string(), - mcp_tool_info_with_display_name( - "dash/tags", - mcp_tool( - "tags", - "Tags", - serde_json::json!({ - "type": "object", - "properties": {"tags": {"type": "array"}} - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/tags", + mcp_tool( + "tags", + "Tags", + serde_json::json!({ + "type": "object", + "properties": {"tags": {"type": "array"}} + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1352,22 +1325,19 @@ async fn test_mcp_tool_anyof_defaults_to_string() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/value".to_string(), - mcp_tool_info_with_display_name( - "dash/value", - mcp_tool( - "value", - "AnyOf Value", - serde_json::json!({ - "type": "object", - "properties": { - "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} - } - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/value", + mcp_tool( + "value", + "AnyOf Value", + serde_json::json!({ + "type": "object", + "properties": { + "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} + } + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1420,39 +1390,36 @@ async fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { }); let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "test_server/do_something_cool".to_string(), - mcp_tool_info_with_display_name( - "test_server/do_something_cool", - mcp_tool( - "do_something_cool", - "Do something cool", - serde_json::json!({ + Some(vec![mcp_tool_info_with_display_name( + "test_server/do_something_cool", + mcp_tool( + "do_something_cool", + "Do something cool", + serde_json::json!({ + "type": "object", + "properties": { + "string_argument": {"type": "string"}, + "number_argument": {"type": "number"}, + "object_argument": { "type": "object", "properties": { - "string_argument": {"type": "string"}, - "number_argument": {"type": "number"}, - "object_argument": { + "string_property": {"type": "string"}, + "number_property": {"type": "number"} + }, + "required": ["string_property", "number_property"], + "additionalProperties": { "type": "object", "properties": { - "string_property": {"type": "string"}, - "number_property": {"type": "number"} + "addtl_prop": {"type": "string"} }, - "required": ["string_property", "number_property"], - "additionalProperties": { - "type": "object", - "properties": { - "addtl_prop": {"type": "string"} - }, - "required": ["addtl_prop"], - "additionalProperties": false - } + "required": ["addtl_prop"], + "additionalProperties": false } } - }), - ), + } + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) diff --git a/codex-rs/core/src/tools/tool_search_entry.rs b/codex-rs/core/src/tools/tool_search_entry.rs index 06b7efcd06..a0e9a726b9 100644 --- a/codex-rs/core/src/tools/tool_search_entry.rs +++ b/codex-rs/core/src/tools/tool_search_entry.rs @@ -5,7 +5,6 @@ use codex_tools::ToolSearchResultSource; use codex_tools::ToolsConfig; use codex_tools::dynamic_tool_to_loadable_tool_spec; use codex_tools::tool_search_result_source_to_loadable_tool_spec; -use std::collections::HashMap; #[derive(Clone)] pub(crate) struct ToolSearchEntry { @@ -15,13 +14,13 @@ pub(crate) struct ToolSearchEntry { } pub(crate) fn build_tool_search_entries( - mcp_tools: Option<&HashMap>, + mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> Vec { let mut entries = Vec::new(); let mut mcp_tools = mcp_tools - .map(|tools| tools.values().collect::>()) + .map(|tools| tools.iter().collect::>()) .unwrap_or_default(); mcp_tools.sort_by_key(|info| info.canonical_tool_name().display()); for info in mcp_tools { @@ -55,7 +54,7 @@ pub(crate) fn build_tool_search_entries( pub(crate) fn build_tool_search_entries_for_config( config: &ToolsConfig, - mcp_tools: Option<&HashMap>, + mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> Vec { let mcp_tools = if config.namespace_tools { diff --git a/codex-rs/core/src/unavailable_tool.rs b/codex-rs/core/src/unavailable_tool.rs index 39ba21f9b8..aabf1f6058 100644 --- a/codex-rs/core/src/unavailable_tool.rs +++ b/codex-rs/core/src/unavailable_tool.rs @@ -6,9 +6,13 @@ use codex_tools::ToolName; pub(crate) fn collect_unavailable_called_tools( input: &[ResponseItem], - exposed_tool_names: &HashSet<&str>, + exposed_tool_names: &HashSet, ) -> Vec { let mut unavailable_tools = BTreeMap::new(); + let exposed_display_names = exposed_tool_names + .iter() + .map(ToolName::display) + .collect::>(); for item in input { let ResponseItem::FunctionCall { @@ -26,7 +30,7 @@ pub(crate) fn collect_unavailable_called_tools( None => ToolName::plain(name.clone()), }; let display_name = tool_name.display(); - if exposed_tool_names.contains(display_name.as_str()) { + if exposed_display_names.contains(&display_name) { continue; } @@ -78,7 +82,10 @@ mod tests { #[test] fn collect_unavailable_called_tools_skips_currently_available_tools() { - let exposed_tool_names = HashSet::from(["mcp__server__lookup", "mcp__server__search"]); + 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), @@ -89,4 +96,17 @@ mod tests { 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()); + } } From 129401df43647cc94a2be590d04629dbd3619a9a Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Thu, 7 May 2026 10:17:07 -0700 Subject: [PATCH 03/10] add top-level remote-control command (#21424) ## Summary `codex --enable remote_control app-server --listen off` is the current way to start a headless, remote-controllable app-server, but it is hard to remember and exposes implementation details. This adds `codex remote-control` as a friendly top-level wrapper for that flow. The command starts a foreground app-server with local transports disabled and enables `remote_control` only for that invocation. ## Changes - Add a visible `codex remote-control` CLI subcommand. - Launch app-server with `AppServerTransport::Off`. - Append `features.remote_control=true` after root feature toggles so the explicit command wins over `--disable remote_control`. - Reject root `--remote` / `--remote-auth-token-env`, matching other non-TUI subcommands. - Add tests for parsing, launch defaults, override ordering, and remote flag rejection. ## Verification - `cargo test -p codex-cli` - `just fix -p codex-cli` --- codex-rs/cli/src/main.rs | 68 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 70cd0294a7..dbe6b7605c 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -126,6 +126,9 @@ enum Subcommand { /// [experimental] Run the app server or related tooling. AppServer(AppServerCommand), + /// [experimental] Start a headless app-server with remote control enabled. + RemoteControl, + /// Launch the Codex desktop app (opens the app installer if missing). #[cfg(any(target_os = "macos", target_os = "windows"))] App(app_cmd::AppCommand), @@ -725,6 +728,14 @@ struct FeatureSetArgs { feature: String, } +const REMOTE_CONTROL_FEATURE_OVERRIDE: &str = "features.remote_control=true"; + +fn enable_remote_control_for_invocation(config_overrides: &mut CliConfigOverrides) { + config_overrides + .raw_overrides + .push(REMOTE_CONTROL_FEATURE_OVERRIDE.to_string()); +} + fn stage_str(stage: Stage) -> &'static str { match stage { Stage::UnderDevelopment => "under development", @@ -896,6 +907,24 @@ async fn cli_main(arg0_paths: Arg0DispatchPaths) -> anyhow::Result<()> { } } } + Some(Subcommand::RemoteControl) => { + reject_remote_mode_for_subcommand( + root_remote.as_deref(), + root_remote_auth_token_env.as_deref(), + "remote-control", + )?; + enable_remote_control_for_invocation(&mut root_config_overrides); + codex_app_server::run_main_with_transport( + arg0_paths.clone(), + root_config_overrides, + codex_config::LoaderOverrides::default(), + /*default_analytics_enabled*/ false, + codex_app_server::AppServerTransport::Off, + codex_protocol::protocol::SessionSource::Cli, + codex_app_server::AppServerWebsocketAuthSettings::default(), + ) + .await?; + } #[cfg(any(target_os = "macos", target_os = "windows"))] Some(Subcommand::App(app_cli)) => { reject_remote_mode_for_subcommand( @@ -2276,6 +2305,45 @@ mod tests { assert!(app_server.analytics_default_enabled); } + #[test] + fn remote_control_override_is_appended_after_root_toggles() { + let mut config_overrides = CliConfigOverrides::default(); + config_overrides + .raw_overrides + .push("features.remote_control=false".to_string()); + + enable_remote_control_for_invocation(&mut config_overrides); + + assert_eq!( + config_overrides.raw_overrides, + vec![ + "features.remote_control=false".to_string(), + REMOTE_CONTROL_FEATURE_OVERRIDE.to_string(), + ] + ); + } + + #[test] + fn reject_remote_flag_for_remote_control() { + let cli = MultitoolCli::try_parse_from([ + "codex", + "--remote", + "ws://127.0.0.1:1234", + "remote-control", + ]) + .expect("parse"); + assert_matches!(cli.subcommand, Some(Subcommand::RemoteControl)); + + let err = reject_remote_mode_for_subcommand( + cli.remote.remote.as_deref(), + cli.remote.remote_auth_token_env.as_deref(), + "remote-control", + ) + .expect_err("remote-control should reject root --remote"); + + assert!(err.to_string().contains("remote-control")); + } + #[test] fn remote_flag_parses_for_interactive_root() { let cli = MultitoolCli::try_parse_from(["codex", "--remote", "ws://127.0.0.1:4500"]) From eb0462f2af3af595af7e8109a31d799a4d795b2a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Thu, 7 May 2026 19:22:04 +0200 Subject: [PATCH 04/10] app-server: refresh live threads from latest config snapshot (#21187) ## Why App-server config writes were leaving existing threads partially stale. After a config mutation, the app-server told each live thread to run `Op::ReloadUserConfig`, but that path only re-read the user `config.toml` layer. Settings that came from the app-server's materialized config snapshot did not propagate to existing threads until restart. This change prevent a FS access from `core` for CCA. ## What changed - add `CodexThread::refresh_runtime_config()` and `Session::refresh_runtime_config()` so the app-server can push a freshly rebuilt config snapshot into a live thread - rebuild the latest config with each thread's `cwd` after config mutations, then refresh the thread from that snapshot instead of asking it to reload only `config.toml` - keep session-static settings unchanged during refresh, while updating runtime-refreshable state such as the config layer stack, `tool_suggest`, and derived hook/plugin/skill state - keep `reload_user_config_layer()` as the file-backed fallback for legacy local reload flows, but route the shared refresh logic through the new runtime refresh path ## Testing - add a session test that verifies `refresh_runtime_config()` rebuilds hooks from refreshed config - add a session test that verifies runtime-refreshable fields update while session-static settings like `model` and `notify` stay unchanged --------- Co-authored-by: Codex --- .../request_processors/config_processor.rs | 15 +- codex-rs/config/src/state.rs | 52 ++++--- codex-rs/core/src/codex_thread.rs | 7 + codex-rs/core/src/session/mod.rs | 64 +++++--- codex-rs/core/src/session/tests.rs | 138 ++++++++++++++++++ 5 files changed, 227 insertions(+), 49 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index 840542419a..cda2bbe61d 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -46,7 +46,6 @@ use codex_login::AuthManager; use codex_model_provider::create_model_provider; use codex_plugin::PluginId; use codex_protocol::config_types::WebSearchMode; -use codex_protocol::protocol::Op; use serde_json::json; use std::path::PathBuf; @@ -378,14 +377,22 @@ impl ConfigRequestProcessor { } async fn reload_user_config(&self) { + let next_config = match self.load_latest_config(/*fallback_cwd*/ None).await { + Ok(config) => config, + Err(err) => { + tracing::warn!( + "failed to rebuild user config for runtime refresh: {}", + err.message + ); + return; + } + }; let thread_ids = self.thread_manager.list_thread_ids().await; for thread_id in thread_ids { let Ok(thread) = self.thread_manager.get_thread(thread_id).await else { continue; }; - if let Err(err) = thread.submit(Op::ReloadUserConfig).await { - tracing::warn!("failed to request user config reload: {err}"); - } + thread.refresh_runtime_config(next_config.clone()).await; } } diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index fc5ec79971..c409b404d0 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -237,28 +237,32 @@ impl ConfigLayerStack { /// replaced; otherwise, it is inserted into the stack at the appropriate /// position based on precedence rules. pub fn with_user_config(&self, config_toml: &AbsolutePathBuf, user_config: TomlValue) -> Self { - let user_layer = ConfigLayerEntry::new( + self.with_user_layer(Some(ConfigLayerEntry::new( ConfigLayerSource::User { file: config_toml.clone(), }, user_config, - ); + ))) + } + /// Returns a new stack with the user layer copied from `other`, preserving + /// every non-user layer already present in this stack. + pub fn with_user_layer_from(&self, other: &Self) -> Self { + self.with_user_layer(other.get_user_layer().cloned()) + } + + fn with_user_layer(&self, user_layer: Option) -> Self { let mut layers = self.layers.clone(); - match self.user_layer_index { - Some(index) => { + let user_layer_index = match (self.user_layer_index, user_layer) { + (Some(index), Some(user_layer)) => { layers[index] = user_layer; - Self { - layers, - user_layer_index: self.user_layer_index, - requirements: self.requirements.clone(), - requirements_toml: self.requirements_toml.clone(), - ignore_user_and_project_exec_policy_rules: self - .ignore_user_and_project_exec_policy_rules, - startup_warnings: self.startup_warnings.clone(), - } + Some(index) } - None => { + (Some(index), None) => { + layers.remove(index); + None + } + (None, Some(user_layer)) => { let user_layer_index = match layers .iter() .position(|layer| layer.name.precedence() > user_layer.name.precedence()) @@ -272,16 +276,18 @@ impl ConfigLayerStack { layers.len() - 1 } }; - Self { - layers, - user_layer_index: Some(user_layer_index), - requirements: self.requirements.clone(), - requirements_toml: self.requirements_toml.clone(), - ignore_user_and_project_exec_policy_rules: self - .ignore_user_and_project_exec_policy_rules, - startup_warnings: self.startup_warnings.clone(), - } + Some(user_layer_index) } + (None, None) => None, + }; + Self { + layers, + user_layer_index, + requirements: self.requirements.clone(), + requirements_toml: self.requirements_toml.clone(), + ignore_user_and_project_exec_policy_rules: self + .ignore_user_and_project_exec_policy_rules, + startup_warnings: self.startup_warnings.clone(), } } diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 24a976637b..9fe235640a 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -464,6 +464,13 @@ impl CodexThread { self.codex.session.get_config().await } + /// Refresh the thread's layer-backed user config state from a caller-supplied + /// config snapshot. Thread-scoped layers and session-static settings remain + /// unchanged. + pub async fn refresh_runtime_config(&self, next_config: crate::config::Config) { + self.codex.session.refresh_runtime_config(next_config).await; + } + pub async fn read_mcp_resource( &self, server: &str, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 1f40438cd6..4c93315c14 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -1399,10 +1399,49 @@ impl Session { state.session_configuration.provider.clone() } + pub(crate) async fn refresh_runtime_config(&self, next_config: Config) { + // Refresh only the user layer from the incoming snapshot. Preserve thread-local + // layers such as request/session overrides that were present when this session + // was created. + let config = { + let mut state = self.state.lock().await; + let mut config = (*state.session_configuration.original_config_do_not_use).clone(); + config.config_layer_stack = config + .config_layer_stack + .with_user_layer_from(&next_config.config_layer_stack); + config.tool_suggest = + resolve_tool_suggest_config_from_layer_stack(&config.config_layer_stack); + let config = Arc::new(config); + state.session_configuration.original_config_do_not_use = Arc::clone(&config); + config + }; + self.services.skills_manager.clear_cache(); + self.services.plugins_manager.clear_cache(); + let hooks = build_hooks_for_config( + config.as_ref(), + self.services.plugins_manager.as_ref(), + self.services.user_shell.as_ref(), + ) + .await; + + let state = self.state.lock().await; + // A newer refresh may have updated the config while this hook build was in flight. + // Only publish hooks derived from the current config snapshot. + if Arc::ptr_eq( + &state.session_configuration.original_config_do_not_use, + &config, + ) { + self.services.hooks.store(Arc::new(hooks)); + } + } + pub(crate) async fn reload_user_config_layer(&self) { // Refresh layer-backed runtime state for an existing session, including enabled plugin, // skill, and hook state. Derived config fields such as feature gates and legacy notify // settings remain session-static. + // + // Prefer `refresh_runtime_config()` when the host can already provide a materialized + // config snapshot. This file-based path exists for legacy local reload flows. let config_toml_path = { let state = self.state.lock().await; state @@ -1428,36 +1467,17 @@ impl Session { } }; - let config = { - let mut state = self.state.lock().await; + let next_config = { + let state = self.state.lock().await; let mut config = (*state.session_configuration.original_config_do_not_use).clone(); config.config_layer_stack = config .config_layer_stack .with_user_config(&config_toml_path, user_config); config.tool_suggest = resolve_tool_suggest_config_from_layer_stack(&config.config_layer_stack); - let config = Arc::new(config); - state.session_configuration.original_config_do_not_use = Arc::clone(&config); config }; - self.services.skills_manager.clear_cache(); - self.services.plugins_manager.clear_cache(); - let hooks = build_hooks_for_config( - config.as_ref(), - self.services.plugins_manager.as_ref(), - self.services.user_shell.as_ref(), - ) - .await; - - let state = self.state.lock().await; - // A newer reload may have updated the config while this hook build was in flight. - // Only publish hooks derived from the current config snapshot. - if Arc::ptr_eq( - &state.session_configuration.original_config_do_not_use, - &config, - ) { - self.services.hooks.store(Arc::new(hooks)); - } + self.refresh_runtime_config(next_config).await; } async fn build_settings_update_items( diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index b250cadfb0..01a5a2fafe 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1283,6 +1283,78 @@ async fn reload_user_config_layer_refreshes_hooks() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn refresh_runtime_config_refreshes_hooks() -> anyhow::Result<()> { + let (session, _turn_context) = make_session_and_context().await; + { + let mut state = session.state.lock().await; + let mut config = (*state.session_configuration.original_config_do_not_use).clone(); + config + .features + .enable(Feature::CodexHooks) + .expect("enable Codex hooks"); + state.session_configuration.original_config_do_not_use = Arc::new(config); + } + let codex_home = session.codex_home().await; + std::fs::create_dir_all(&codex_home)?; + let config_toml_path = codex_home.join(CONFIG_TOML_FILE); + #[derive(serde::Serialize)] + struct NormalizedHookIdentity { + event_name: &'static str, + #[serde(flatten)] + group: codex_config::MatcherGroup, + } + let trusted_hash = { + let identity = NormalizedHookIdentity { + event_name: "session_start", + group: codex_config::MatcherGroup { + matcher: None, + hooks: vec![codex_config::HookHandlerConfig::Command { + command: "python3 /tmp/user.py".to_string(), + timeout_sec: Some(600), + r#async: false, + status_message: None, + }], + }, + }; + let identity = codex_config::TomlValue::try_from(identity)?; + codex_config::version_for_toml(&identity) + }; + let hook_key = format!("{}:session_start:0:0", config_toml_path.display()); + let trusted_user_config: codex_config::TomlValue = serde_json::from_value(serde_json::json!({ + "hooks": { + "SessionStart": [{ + "hooks": [{ + "type": "command", + "command": "python3 /tmp/user.py", + }], + }], + "state": { + hook_key: { + "trusted_hash": trusted_hash, + }, + }, + }, + }))?; + std::fs::write(&config_toml_path, toml::to_string(&trusted_user_config)?)?; + + let request = codex_hooks::SessionStartRequest { + session_id: session.conversation_id, + cwd: session.get_config().await.cwd.clone(), + transcript_path: None, + model: "gpt-5.2".to_string(), + permission_mode: "default".to_string(), + source: codex_hooks::SessionStartSource::Startup, + }; + assert!(session.hooks().preview_session_start(&request).is_empty()); + + let next_config = load_latest_config_for_session(&session).await; + session.refresh_runtime_config(next_config).await; + + assert_eq!(session.hooks().preview_session_start(&request).len(), 1); + Ok(()) +} + #[tokio::test] async fn reload_user_config_layer_updates_effective_tool_suggest_config() { let (session, _turn_context) = make_session_and_context().await; @@ -1312,6 +1384,62 @@ disabled_tools = [ ); } +#[tokio::test] +async fn refresh_runtime_config_updates_runtime_refreshable_fields_and_keeps_session_static_settings() + { + let (session, _turn_context) = make_session_and_context().await; + let codex_home = session.codex_home().await; + std::fs::create_dir_all(&codex_home).expect("create codex home"); + std::fs::write( + codex_home.join(CONFIG_TOML_FILE), + r#"[apps.calendar] +enabled = false +destructive_enabled = false + +[tool_suggest] +disabled_tools = [ + { type = "connector", id = " calendar " }, + { type = "plugin", id = "slack@openai-curated" }, +] +"#, + ) + .expect("write user config"); + + let original = session.get_config().await; + let mut next_config = load_latest_config_for_session(&session).await; + next_config.model = Some("gpt-5.4".to_string()); + next_config.notify = Some(vec!["echo".to_string()]); + + session.refresh_runtime_config(next_config).await; + + let config = session.get_config().await; + let apps_toml = config + .config_layer_stack + .effective_config() + .as_table() + .and_then(|table| table.get("apps")) + .cloned() + .expect("apps table"); + let apps = codex_config::types::AppsConfigToml::deserialize(apps_toml) + .expect("deserialize apps config"); + let app = apps + .apps + .get("calendar") + .expect("calendar app config exists"); + + assert!(!app.enabled); + assert_eq!(app.destructive_enabled, Some(false)); + assert_eq!(config.model, original.model); + assert_eq!(config.notify, original.notify); + assert_eq!( + config.tool_suggest.disabled_tools, + vec![ + ToolSuggestDisabledTool::connector("calendar"), + ToolSuggestDisabledTool::plugin("slack@openai-curated"), + ] + ); +} + #[test] fn filter_connectors_for_input_skips_duplicate_slug_mentions() { let connectors = vec![ @@ -3837,6 +3965,16 @@ async fn make_session_with_config( Ok(session) } +async fn load_latest_config_for_session(session: &Session) -> Config { + let config = session.get_config().await; + ConfigBuilder::default() + .codex_home(config.codex_home.to_path_buf()) + .fallback_cwd(Some(config.cwd.to_path_buf())) + .build() + .await + .expect("load latest config for session") +} + async fn make_session_with_config_and_rx( mutator: impl FnOnce(&mut Config), ) -> anyhow::Result<(Arc, async_channel::Receiver)> { From 566f2cb61220342871ea63afee2322490ecc5d0e Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 7 May 2026 10:48:36 -0700 Subject: [PATCH 05/10] [codex] Move tool specs onto handlers (#21461) ## Why This is the next stacked step after deleting the tool-handler kind indirection. Specs should come from the registered handlers themselves so registry construction has a single source of truth for handler behavior and exposed tool definitions. ## What changed - Added `ToolHandler::spec()` plus handler-provided parallel/code-mode metadata, and made `ToolRegistryBuilder::register_handler` automatically collect specs from registered handlers. - Moved builtin tool spec construction into the corresponding handlers and their adjacent `_spec` modules, including shell, unified exec, apply patch, view image, request plugin install, tool search, MCP resource, goals, planning, permissions, agent jobs, and multi-agent tools. - Reworked configurable handlers to receive their tool-building options through constructors, with non-optional handler options where the handler is always spec-backed. Shell fallback handlers keep an explicit no-spec mode because they are also registered as hidden dispatch aliases. - Kept `CodeModeExecuteHandler` on the explicit configured wrapper so the code-mode exec spec can still be built from the nested registry. ## Verification - `cargo check -p codex-core` - `cargo test -p codex-core tools::spec_plan::tests` - `cargo test -p codex-core tools::spec::tests` - `cargo test -p codex-core tools::handlers::multi_agents_spec::tests` - `RUST_MIN_STACK=16777216 cargo test -p codex-core tools::handlers::multi_agents::tests` - `cargo test -p codex-core tools::handlers::apply_patch::tests` - `cargo test -p codex-core tools::handlers::unified_exec::tests` - `just fix -p codex-core` - `git diff --check` --- codex-rs/core/src/session/tests.rs | 4 +- .../core/src/session/tests/guardian_tests.rs | 8 +- .../src/tools/code_mode/execute_handler.rs | 13 +- .../core/src/tools/code_mode/wait_handler.rs | 6 + .../agent_jobs/report_agent_job_result.rs | 6 + .../agent_jobs/spawn_agents_on_csv.rs | 6 + .../core/src/tools/handlers/apply_patch.rs | 31 +- .../src/tools/handlers/apply_patch_tests.rs | 6 +- .../src/tools/handlers/goal/create_goal.rs | 6 + .../core/src/tools/handlers/goal/get_goal.rs | 6 + .../src/tools/handlers/goal/update_goal.rs | 6 + .../list_mcp_resource_templates.rs | 10 + .../mcp_resource/list_mcp_resources.rs | 10 + .../mcp_resource/read_mcp_resource.rs | 10 + codex-rs/core/src/tools/handlers/mod.rs | 2 + .../handlers/multi_agents/close_agent.rs | 6 + .../handlers/multi_agents/resume_agent.rs | 6 + .../tools/handlers/multi_agents/send_input.rs | 6 + .../src/tools/handlers/multi_agents/spawn.rs | 18 +- .../src/tools/handlers/multi_agents/wait.rs | 18 +- .../src/tools/handlers/multi_agents_spec.rs | 24 +- .../tools/handlers/multi_agents_spec_tests.rs | 4 +- .../src/tools/handlers/multi_agents_tests.rs | 104 ++--- .../handlers/multi_agents_v2/close_agent.rs | 6 + .../handlers/multi_agents_v2/followup_task.rs | 6 + .../handlers/multi_agents_v2/list_agents.rs | 6 + .../handlers/multi_agents_v2/send_message.rs | 6 + .../tools/handlers/multi_agents_v2/spawn.rs | 18 +- .../tools/handlers/multi_agents_v2/wait.rs | 18 +- codex-rs/core/src/tools/handlers/plan.rs | 6 + .../src/tools/handlers/request_permissions.rs | 9 + .../tools/handlers/request_plugin_install.rs | 25 +- .../src/tools/handlers/request_user_input.rs | 9 + codex-rs/core/src/tools/handlers/shell.rs | 1 + .../src/tools/handlers/shell/local_shell.rs | 21 +- .../src/tools/handlers/shell/shell_command.rs | 36 +- .../src/tools/handlers/shell/shell_handler.rs | 22 +- .../core/src/tools/handlers/shell_tests.rs | 2 +- codex-rs/core/src/tools/handlers/test_sync.rs | 10 + .../core/src/tools/handlers/tool_search.rs | 26 +- .../src/tools/handlers/unavailable_tool.rs | 20 +- .../core/src/tools/handlers/unified_exec.rs | 1 + .../handlers/unified_exec/exec_command.rs | 46 ++- .../handlers/unified_exec/write_stdin.rs | 6 + .../src/tools/handlers/unified_exec_tests.rs | 8 +- .../core/src/tools/handlers/view_image.rs | 31 +- codex-rs/core/src/tools/registry.rs | 49 +-- codex-rs/core/src/tools/registry_tests.rs | 18 + codex-rs/core/src/tools/spec.rs | 12 +- codex-rs/core/src/tools/spec_plan.rs | 360 ++++-------------- codex-rs/core/src/tools/spec_plan_tests.rs | 25 +- 51 files changed, 722 insertions(+), 397 deletions(-) diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 01a5a2fafe..b63b16cbf4 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -8908,7 +8908,7 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { let tool_name = "shell"; let call_id = "test-call".to_string(); - let handler = ShellHandler; + let handler = ShellHandler::default(); let resp = handler .handle(ToolInvocation { session: Arc::clone(&session), @@ -8983,7 +8983,7 @@ async fn unified_exec_rejects_escalated_permissions_when_policy_not_on_request() let turn_context = Arc::new(turn_context_raw); let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); - let handler = ExecCommandHandler; + let handler = ExecCommandHandler::default(); let resp = handler .handle(ToolInvocation { session: Arc::clone(&session), diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 857ec950c2..5c473ef1f9 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -323,7 +323,7 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid arg0: None, }; - let handler = ShellHandler; + let handler = ShellHandler::default(); let resp = handler .handle(ToolInvocation { session: Arc::clone(&session), @@ -437,7 +437,7 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() { let session = Arc::new(session); let turn_context = Arc::new(turn_context_raw); - let handler = ShellHandler; + let handler = ShellHandler::default(); let command = if cfg!(windows) { vec![ "cmd.exe".to_string(), @@ -498,7 +498,7 @@ async fn guardian_allows_unified_exec_additional_permissions_requests_past_polic let turn_context = Arc::new(turn_context_raw); let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); - let handler = ExecCommandHandler; + let handler = ExecCommandHandler::default(); let resp = handler .handle(ToolInvocation { session: Arc::clone(&session), @@ -615,7 +615,7 @@ async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_per let session = Arc::new(session); let turn_context = Arc::new(turn_context_raw); - let handler = ShellHandler; + let handler = ShellHandler::default(); let resp = handler .handle(ToolInvocation { session: Arc::clone(&session), diff --git a/codex-rs/core/src/tools/code_mode/execute_handler.rs b/codex-rs/core/src/tools/code_mode/execute_handler.rs index 42841d2189..0e11cd166f 100644 --- a/codex-rs/core/src/tools/code_mode/execute_handler.rs +++ b/codex-rs/core/src/tools/code_mode/execute_handler.rs @@ -5,6 +5,7 @@ use crate::tools::context::ToolPayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::ExecContext; use super::PUBLIC_TOOL_NAME; @@ -12,9 +13,15 @@ use super::build_enabled_tools; use super::handle_runtime_response; use super::is_exec_tool_name; -pub struct CodeModeExecuteHandler; +pub struct CodeModeExecuteHandler { + spec: ToolSpec, +} impl CodeModeExecuteHandler { + pub(crate) fn new(spec: ToolSpec) -> Self { + Self { spec } + } + async fn execute( &self, session: std::sync::Arc, @@ -83,6 +90,10 @@ impl ToolHandler for CodeModeExecuteHandler { ToolName::plain(PUBLIC_TOOL_NAME) } + fn spec(&self) -> Option { + Some(self.spec.clone()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/code_mode/wait_handler.rs b/codex-rs/core/src/tools/code_mode/wait_handler.rs index 8024c95865..21191be02d 100644 --- a/codex-rs/core/src/tools/code_mode/wait_handler.rs +++ b/codex-rs/core/src/tools/code_mode/wait_handler.rs @@ -7,11 +7,13 @@ use crate::tools::context::ToolPayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::DEFAULT_WAIT_YIELD_TIME_MS; use super::ExecContext; use super::WAIT_TOOL_NAME; use super::handle_runtime_response; +use super::wait_spec::create_wait_tool; pub struct CodeModeWaitHandler; @@ -46,6 +48,10 @@ impl ToolHandler for CodeModeWaitHandler { ToolName::plain(WAIT_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_wait_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/agent_jobs/report_agent_job_result.rs b/codex-rs/core/src/tools/handlers/agent_jobs/report_agent_job_result.rs index 90cde7d446..a7a36a49d5 100644 --- a/codex-rs/core/src/tools/handlers/agent_jobs/report_agent_job_result.rs +++ b/codex-rs/core/src/tools/handlers/agent_jobs/report_agent_job_result.rs @@ -2,9 +2,11 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::agent_jobs_spec::create_report_agent_job_result_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::*; @@ -17,6 +19,10 @@ impl ToolHandler for ReportAgentJobResultHandler { ToolName::plain("report_agent_job_result") } + fn spec(&self) -> Option { + Some(create_report_agent_job_result_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/agent_jobs/spawn_agents_on_csv.rs b/codex-rs/core/src/tools/handlers/agent_jobs/spawn_agents_on_csv.rs index 911f1a5eef..a1d0b5e7ae 100644 --- a/codex-rs/core/src/tools/handlers/agent_jobs/spawn_agents_on_csv.rs +++ b/codex-rs/core/src/tools/handlers/agent_jobs/spawn_agents_on_csv.rs @@ -2,9 +2,11 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::agent_jobs_spec::create_spawn_agents_on_csv_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::*; @@ -17,6 +19,10 @@ impl ToolHandler for SpawnAgentsOnCsvHandler { ToolName::plain("spawn_agents_on_csv") } + fn spec(&self) -> Option { + Some(create_spawn_agents_on_csv_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 104ab9ffb6..2b63c1cb17 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -22,6 +22,8 @@ use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; use crate::tools::handlers::apply_granted_turn_permissions; use crate::tools::handlers::apply_patch_spec::ApplyPatchToolArgs; +use crate::tools::handlers::apply_patch_spec::create_apply_patch_freeform_tool; +use crate::tools::handlers::apply_patch_spec::create_apply_patch_json_tool; use crate::tools::handlers::parse_arguments; use crate::tools::hook_names::HookToolName; use crate::tools::orchestrator::ToolOrchestrator; @@ -41,6 +43,7 @@ use codex_exec_server::ExecutorFileSystem; use codex_features::Feature; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::FileSystemPermissions; +use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::PatchApplyUpdatedEvent; @@ -48,11 +51,30 @@ use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_tools::ToolName; +use codex_tools::ToolSpec; use codex_utils_absolute_path::AbsolutePathBuf; const APPLY_PATCH_ARGUMENT_DIFF_BUFFER_INTERVAL: Duration = Duration::from_millis(500); -pub struct ApplyPatchHandler; +pub struct ApplyPatchHandler { + options: ApplyPatchToolType, +} + +impl Default for ApplyPatchHandler { + fn default() -> Self { + Self { + options: ApplyPatchToolType::Freeform, + } + } +} + +impl ApplyPatchHandler { + pub(crate) fn new(apply_patch_tool_type: ApplyPatchToolType) -> Self { + Self { + options: apply_patch_tool_type, + } + } +} #[derive(Default)] struct ApplyPatchArgumentDiffConsumer { @@ -297,6 +319,13 @@ impl ToolHandler for ApplyPatchHandler { ToolName::plain("apply_patch") } + fn spec(&self) -> Option { + Some(match self.options { + ApplyPatchToolType::Freeform => create_apply_patch_freeform_tool(), + ApplyPatchToolType::Function => create_apply_patch_json_tool(), + }) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 04472e4623..c0d4d17f32 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -49,7 +49,7 @@ async fn pre_tool_use_payload_uses_json_patch_input() { arguments: json!({ "input": patch }).to_string(), }; let invocation = invocation_for_payload(payload).await; - let handler = ApplyPatchHandler; + let handler = ApplyPatchHandler::default(); assert_eq!( handler.pre_tool_use_payload(&invocation), @@ -67,7 +67,7 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() { input: patch.to_string(), }; let invocation = invocation_for_payload(payload).await; - let handler = ApplyPatchHandler; + let handler = ApplyPatchHandler::default(); assert_eq!( handler.pre_tool_use_payload(&invocation), @@ -86,7 +86,7 @@ async fn post_tool_use_payload_uses_patch_input_and_tool_output() { }; let invocation = invocation_for_payload(payload).await; let output = ApplyPatchToolOutput::from_text("Success. Updated files.".to_string()); - let handler = ApplyPatchHandler; + let handler = ApplyPatchHandler::default(); assert_eq!( handler.post_tool_use_payload(&invocation, &output), diff --git a/codex-rs/core/src/tools/handlers/goal/create_goal.rs b/codex-rs/core/src/tools/handlers/goal/create_goal.rs index 18c6c3b010..37ca319ad1 100644 --- a/codex-rs/core/src/tools/handlers/goal/create_goal.rs +++ b/codex-rs/core/src/tools/handlers/goal/create_goal.rs @@ -4,10 +4,12 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::goal_spec::CREATE_GOAL_TOOL_NAME; +use crate::tools::handlers::goal_spec::create_create_goal_tool; use crate::tools::handlers::parse_arguments; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::CompletionBudgetReport; use super::CreateGoalArgs; @@ -23,6 +25,10 @@ impl ToolHandler for CreateGoalHandler { ToolName::plain(CREATE_GOAL_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_create_goal_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/goal/get_goal.rs b/codex-rs/core/src/tools/handlers/goal/get_goal.rs index e70c6d9bf0..f1af6dc5bb 100644 --- a/codex-rs/core/src/tools/handlers/goal/get_goal.rs +++ b/codex-rs/core/src/tools/handlers/goal/get_goal.rs @@ -3,9 +3,11 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::goal_spec::GET_GOAL_TOOL_NAME; +use crate::tools::handlers::goal_spec::create_get_goal_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::CompletionBudgetReport; use super::format_goal_error; @@ -20,6 +22,10 @@ impl ToolHandler for GetGoalHandler { ToolName::plain(GET_GOAL_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_get_goal_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/goal/update_goal.rs b/codex-rs/core/src/tools/handlers/goal/update_goal.rs index 46d6d26a04..bdb2315a68 100644 --- a/codex-rs/core/src/tools/handlers/goal/update_goal.rs +++ b/codex-rs/core/src/tools/handlers/goal/update_goal.rs @@ -5,11 +5,13 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::goal_spec::UPDATE_GOAL_TOOL_NAME; +use crate::tools::handlers::goal_spec::create_update_goal_tool; use crate::tools::handlers::parse_arguments; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_protocol::protocol::ThreadGoalStatus; use codex_tools::ToolName; +use codex_tools::ToolSpec; use super::CompletionBudgetReport; use super::UpdateGoalArgs; @@ -25,6 +27,10 @@ impl ToolHandler for UpdateGoalHandler { ToolName::plain(UPDATE_GOAL_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_update_goal_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resource_templates.rs b/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resource_templates.rs index 2c87edf0c7..5e42bc3c0d 100644 --- a/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resource_templates.rs +++ b/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resource_templates.rs @@ -4,11 +4,13 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::mcp_resource_spec::create_list_mcp_resource_templates_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_protocol::models::function_call_output_content_items_to_text; use codex_protocol::protocol::McpInvocation; use codex_tools::ToolName; +use codex_tools::ToolSpec; use rmcp::model::PaginatedRequestParams; @@ -31,6 +33,14 @@ impl ToolHandler for ListMcpResourceTemplatesHandler { ToolName::plain("list_mcp_resource_templates") } + fn spec(&self) -> Option { + Some(create_list_mcp_resource_templates_tool()) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resources.rs b/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resources.rs index ed62852141..77b224cd1a 100644 --- a/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resources.rs +++ b/codex-rs/core/src/tools/handlers/mcp_resource/list_mcp_resources.rs @@ -4,11 +4,13 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::mcp_resource_spec::create_list_mcp_resources_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_protocol::models::function_call_output_content_items_to_text; use codex_protocol::protocol::McpInvocation; use codex_tools::ToolName; +use codex_tools::ToolSpec; use rmcp::model::PaginatedRequestParams; @@ -31,6 +33,14 @@ impl ToolHandler for ListMcpResourcesHandler { ToolName::plain("list_mcp_resources") } + fn spec(&self) -> Option { + Some(create_list_mcp_resources_tool()) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/mcp_resource/read_mcp_resource.rs b/codex-rs/core/src/tools/handlers/mcp_resource/read_mcp_resource.rs index 91d5a4317e..50944aefb6 100644 --- a/codex-rs/core/src/tools/handlers/mcp_resource/read_mcp_resource.rs +++ b/codex-rs/core/src/tools/handlers/mcp_resource/read_mcp_resource.rs @@ -4,11 +4,13 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; +use crate::tools::handlers::mcp_resource_spec::create_read_mcp_resource_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_protocol::models::function_call_output_content_items_to_text; use codex_protocol::protocol::McpInvocation; use codex_tools::ToolName; +use codex_tools::ToolSpec; use rmcp::model::ReadResourceRequestParams; @@ -31,6 +33,14 @@ impl ToolHandler for ReadMcpResourceHandler { ToolName::plain("read_mcp_resource") } + fn spec(&self) -> Option { + Some(create_read_mcp_resource_tool()) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index a1aa7e139a..24bddc2c5f 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -64,12 +64,14 @@ pub use request_user_input::RequestUserInputHandler; pub use shell::ContainerExecHandler; pub use shell::LocalShellHandler; pub use shell::ShellCommandHandler; +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; pub use view_image::ViewImageHandler; diff --git a/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs index 70d24c4288..7d47290c10 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs @@ -1,5 +1,7 @@ use super::*; +use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v1; use crate::turn_timing::now_unix_timestamp_ms; +use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -10,6 +12,10 @@ impl ToolHandler for Handler { ToolName::plain("close_agent") } + fn spec(&self) -> Option { + Some(create_close_agent_tool_v1()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs index 8fa4622617..0b86c9abdf 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs @@ -1,6 +1,8 @@ use super::*; use crate::agent::next_thread_spawn_depth; +use crate::tools::handlers::multi_agents_spec::create_resume_agent_tool; use crate::turn_timing::now_unix_timestamp_ms; +use codex_tools::ToolSpec; use std::sync::Arc; pub(crate) struct Handler; @@ -12,6 +14,10 @@ impl ToolHandler for Handler { ToolName::plain("resume_agent") } + fn spec(&self) -> Option { + Some(create_resume_agent_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents/send_input.rs b/codex-rs/core/src/tools/handlers/multi_agents/send_input.rs index 0994ba5e2f..cc65a00f41 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/send_input.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/send_input.rs @@ -1,6 +1,8 @@ use super::*; use crate::agent::control::render_input_preview; +use crate::tools::handlers::multi_agents_spec::create_send_input_tool_v1; use crate::turn_timing::now_unix_timestamp_ms; +use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -11,6 +13,10 @@ impl ToolHandler for Handler { ToolName::plain("send_input") } + fn spec(&self) -> Option { + Some(create_send_input_tool_v1()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs index adfb926fe7..40e9cc5d38 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs @@ -6,9 +6,21 @@ use crate::agent::exceeds_thread_spawn_depth_limit; use crate::agent::next_thread_spawn_depth; use crate::agent::role::DEFAULT_ROLE_NAME; use crate::agent::role::apply_role_to_config; +use crate::tools::handlers::multi_agents_spec::SpawnAgentToolOptions; +use crate::tools::handlers::multi_agents_spec::create_spawn_agent_tool_v1; use crate::turn_timing::now_unix_timestamp_ms; +use codex_tools::ToolSpec; -pub(crate) struct Handler; +#[derive(Default)] +pub(crate) struct Handler { + options: SpawnAgentToolOptions, +} + +impl Handler { + pub(crate) fn new(options: SpawnAgentToolOptions) -> Self { + Self { options } + } +} impl ToolHandler for Handler { type Output = SpawnAgentResult; @@ -17,6 +29,10 @@ impl ToolHandler for Handler { ToolName::plain("spawn_agent") } + fn spec(&self) -> Option { + Some(create_spawn_agent_tool_v1(self.options.clone())) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents/wait.rs b/codex-rs/core/src/tools/handlers/multi_agents/wait.rs index 8d6c09193e..9e63450a6a 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/wait.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/wait.rs @@ -1,7 +1,10 @@ use super::*; use crate::agent::status::is_final; +use crate::tools::handlers::multi_agents_spec::WaitAgentTimeoutOptions; +use crate::tools::handlers::multi_agents_spec::create_wait_agent_tool_v1; use crate::turn_timing::now_unix_timestamp_ms; use codex_protocol::error::CodexErr; +use codex_tools::ToolSpec; use futures::FutureExt; use futures::StreamExt; use futures::stream::FuturesUnordered; @@ -13,7 +16,16 @@ use tokio::time::Instant; use tokio::time::timeout_at; -pub(crate) struct Handler; +#[derive(Default)] +pub(crate) struct Handler { + options: WaitAgentTimeoutOptions, +} + +impl Handler { + pub(crate) fn new(options: WaitAgentTimeoutOptions) -> Self { + Self { options } + } +} impl ToolHandler for Handler { type Output = WaitAgentResult; @@ -22,6 +34,10 @@ impl ToolHandler for Handler { ToolName::plain("wait_agent") } + fn spec(&self) -> Option { + Some(create_wait_agent_tool_v1(self.options)) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_spec.rs b/codex-rs/core/src/tools/handlers/multi_agents_spec.rs index 2cbef2104b..233491efa4 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_spec.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_spec.rs @@ -9,9 +9,9 @@ use std::collections::BTreeMap; const SPAWN_AGENT_INHERITED_MODEL_GUIDANCE: &str = "Spawned agents inherit your current model by default. Omit `model` to use that preferred default; set `model` only when an explicit override is needed."; const SPAWN_AGENT_MODEL_OVERRIDE_DESCRIPTION: &str = "Optional model override for the new agent. Leave unset to inherit the same model as the parent, which is the preferred default. Only set this when the user explicitly asks for a different model or the task clearly requires one."; -#[derive(Debug, Clone)] -pub struct SpawnAgentToolOptions<'a> { - pub available_models: &'a [ModelPreset], +#[derive(Debug, Clone, Default)] +pub struct SpawnAgentToolOptions { + pub available_models: Vec, pub agent_type_description: String, pub hide_agent_type_model_reasoning: bool, pub include_usage_hint: bool, @@ -26,9 +26,19 @@ pub struct WaitAgentTimeoutOptions { pub max_timeout_ms: i64, } -pub fn create_spawn_agent_tool_v1(options: SpawnAgentToolOptions<'_>) -> ToolSpec { +impl Default for WaitAgentTimeoutOptions { + fn default() -> Self { + Self { + default_timeout_ms: super::multi_agents_common::DEFAULT_WAIT_TIMEOUT_MS, + min_timeout_ms: super::multi_agents_common::MIN_WAIT_TIMEOUT_MS, + max_timeout_ms: super::multi_agents_common::MAX_WAIT_TIMEOUT_MS, + } + } +} + +pub fn create_spawn_agent_tool_v1(options: SpawnAgentToolOptions) -> ToolSpec { let available_models_description = (!options.hide_agent_type_model_reasoning) - .then(|| spawn_agent_models_description(options.available_models)); + .then(|| spawn_agent_models_description(&options.available_models)); let return_value_description = "Returns the spawned agent id plus the user-facing nickname when available."; let mut properties = spawn_agent_common_properties_v1(&options.agent_type_description); @@ -51,9 +61,9 @@ pub fn create_spawn_agent_tool_v1(options: SpawnAgentToolOptions<'_>) -> ToolSpe }) } -pub fn create_spawn_agent_tool_v2(options: SpawnAgentToolOptions<'_>) -> ToolSpec { +pub fn create_spawn_agent_tool_v2(options: SpawnAgentToolOptions) -> ToolSpec { let available_models_description = (!options.hide_agent_type_model_reasoning) - .then(|| spawn_agent_models_description(options.available_models)); + .then(|| spawn_agent_models_description(&options.available_models)); let mut properties = spawn_agent_common_properties_v2(&options.agent_type_description); if options.hide_agent_type_model_reasoning { hide_spawn_agent_metadata_options(&mut properties); diff --git a/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs index dcba73eada..c8fa1a0f90 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_spec_tests.rs @@ -33,7 +33,7 @@ fn model_preset(id: &str, show_in_picker: bool) -> ModelPreset { #[test] fn spawn_agent_tool_v2_requires_task_name_and_lists_visible_models() { let tool = create_spawn_agent_tool_v2(SpawnAgentToolOptions { - available_models: &[ + available_models: vec![ model_preset("visible", /*show_in_picker*/ true), model_preset("hidden", /*show_in_picker*/ false), ], @@ -99,7 +99,7 @@ fn spawn_agent_tool_v2_requires_task_name_and_lists_visible_models() { #[test] fn spawn_agent_tool_v1_keeps_legacy_fork_context_field() { let tool = create_spawn_agent_tool_v1(SpawnAgentToolOptions { - available_models: &[], + available_models: Vec::new(), agent_type_description: "role help".to_string(), hide_agent_type_model_reasoning: false, include_usage_hint: true, diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index 6f44673f13..43503be8c1 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -180,7 +180,7 @@ async fn handler_rejects_non_function_payloads() { input: "hello".to_string(), }, ); - let Err(err) = SpawnAgentHandler.handle(invocation).await else { + let Err(err) = SpawnAgentHandler::default().handle(invocation).await else { panic!("payload should be rejected"); }; assert_eq!( @@ -200,7 +200,7 @@ async fn spawn_agent_rejects_empty_message() { "spawn_agent", function_payload(json!({"message": " "})), ); - let Err(err) = SpawnAgentHandler.handle(invocation).await else { + let Err(err) = SpawnAgentHandler::default().handle(invocation).await else { panic!("empty message should be rejected"); }; assert_eq!( @@ -221,7 +221,7 @@ async fn spawn_agent_rejects_when_message_and_items_are_both_set() { "items": [{"type": "mention", "name": "drive", "path": "app://drive"}] })), ); - let Err(err) = SpawnAgentHandler.handle(invocation).await else { + let Err(err) = SpawnAgentHandler::default().handle(invocation).await else { panic!("message+items should be rejected"); }; assert_eq!( @@ -268,7 +268,7 @@ async fn spawn_agent_uses_explorer_role_and_preserves_approval_policy() { "agent_type": "explorer" })), ); - let output = SpawnAgentHandler + let output = SpawnAgentHandler::default() .handle(invocation) .await .expect("spawn_agent should succeed"); @@ -303,7 +303,7 @@ async fn spawn_agent_fork_context_rejects_agent_type_override() { .expect("root thread should start"); session.services.agent_control = manager.agent_control(); session.conversation_id = root.thread_id; - let err = SpawnAgentHandler + let err = SpawnAgentHandler::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -336,7 +336,7 @@ async fn spawn_agent_fork_context_rejects_child_model_overrides() { session.services.agent_control = manager.agent_control(); session.conversation_id = root.thread_id; - let err = SpawnAgentHandler + let err = SpawnAgentHandler::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -380,7 +380,7 @@ async fn multi_agent_v2_spawn_fork_turns_all_rejects_agent_type_override() { ..turn }; - let err = SpawnAgentHandlerV2 + let err = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -420,7 +420,7 @@ async fn multi_agent_v2_spawn_defaults_to_full_fork_and_rejects_child_model_over .expect("test config should allow feature update"); turn.config = Arc::new(config); - let err = SpawnAgentHandlerV2 + let err = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -464,7 +464,7 @@ async fn multi_agent_v2_spawn_partial_fork_turns_allows_agent_type_override() { ..turn }; - let output = SpawnAgentHandlerV2 + let output = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -506,7 +506,7 @@ async fn spawn_agent_returns_agent_id_without_task_name() { let manager = thread_manager(); session.services.agent_control = manager.agent_control(); - let output = SpawnAgentHandler + let output = SpawnAgentHandler::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -552,7 +552,7 @@ async fn multi_agent_v2_spawn_requires_task_name() { "message": "inspect this repo" })), ); - let Err(err) = SpawnAgentHandlerV2.handle(invocation).await else { + let Err(err) = SpawnAgentHandlerV2::default().handle(invocation).await else { panic!("missing task_name should be rejected"); }; let FunctionCallError::RespondToModel(message) = err else { @@ -588,7 +588,7 @@ async fn multi_agent_v2_spawn_rejects_legacy_items_field() { "task_name": "worker" })), ); - let Err(err) = SpawnAgentHandlerV2.handle(invocation).await else { + let Err(err) = SpawnAgentHandlerV2::default().handle(invocation).await else { panic!("legacy items field should be rejected"); }; let FunctionCallError::RespondToModel(message) = err else { @@ -606,7 +606,7 @@ async fn spawn_agent_errors_when_manager_dropped() { "spawn_agent", function_payload(json!({"message": "hello"})), ); - let Err(err) = SpawnAgentHandler.handle(invocation).await else { + let Err(err) = SpawnAgentHandler::default().handle(invocation).await else { panic!("spawn should fail without a manager"); }; assert_eq!( @@ -640,7 +640,7 @@ async fn multi_agent_v2_spawn_returns_path_and_send_message_accepts_relative_pat let session = Arc::new(session); let turn = Arc::new(turn); - let spawn_output = SpawnAgentHandlerV2 + let spawn_output = SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -735,7 +735,7 @@ async fn multi_agent_v2_spawn_rejects_legacy_fork_context() { .expect("test config should allow feature update"); turn.config = Arc::new(config); - let err = SpawnAgentHandlerV2 + let err = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -774,7 +774,7 @@ async fn multi_agent_v2_spawn_rejects_invalid_fork_turns_string() { .expect("test config should allow feature update"); turn.config = Arc::new(config); - let err = SpawnAgentHandlerV2 + let err = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -813,7 +813,7 @@ async fn multi_agent_v2_spawn_rejects_zero_fork_turns() { .expect("test config should allow feature update"); turn.config = Arc::new(config); - let err = SpawnAgentHandlerV2 + let err = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -1008,7 +1008,7 @@ async fn multi_agent_v2_list_agents_returns_completed_status_and_last_task_messa let session = Arc::new(session); let turn = Arc::new(turn); - let spawn_output = SpawnAgentHandlerV2 + let spawn_output = SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1189,7 +1189,7 @@ async fn multi_agent_v2_list_agents_omits_closed_agents() { let session = Arc::new(session); let turn = Arc::new(turn); - let spawn_output = SpawnAgentHandlerV2 + let spawn_output = SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1253,7 +1253,7 @@ async fn multi_agent_v2_send_message_rejects_legacy_items_field() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1309,7 +1309,7 @@ async fn multi_agent_v2_send_message_rejects_interrupt_parameter() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1382,7 +1382,7 @@ async fn multi_agent_v2_followup_task_completion_notifies_parent_on_every_turn() let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1517,7 +1517,7 @@ async fn multi_agent_v2_followup_task_rejects_legacy_items_field() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1570,7 +1570,7 @@ async fn multi_agent_v2_interrupted_turn_does_not_notify_parent() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -1648,7 +1648,7 @@ async fn multi_agent_v2_spawn_omits_agent_id_when_named() { .expect("test config should allow feature update"); turn.config = Arc::new(config); - let output = SpawnAgentHandlerV2 + let output = SpawnAgentHandlerV2::default() .handle(invocation( Arc::new(session), Arc::new(turn), @@ -1696,7 +1696,7 @@ async fn multi_agent_v2_spawn_surfaces_task_name_validation_errors() { "task_name": "BadName" })), ); - let Err(err) = SpawnAgentHandlerV2.handle(invocation).await else { + let Err(err) = SpawnAgentHandlerV2::default().handle(invocation).await else { panic!("invalid agent name should be rejected"); }; assert_eq!( @@ -1754,7 +1754,7 @@ async fn spawn_agent_reapplies_runtime_sandbox_after_role_config() { "agent_type": "explorer" })), ); - let output = SpawnAgentHandler + let output = SpawnAgentHandler::default() .handle(invocation) .await .expect("spawn_agent should succeed"); @@ -1815,7 +1815,7 @@ async fn spawn_agent_rejects_when_depth_limit_exceeded() { "spawn_agent", function_payload(json!({"message": "hello"})), ); - let Err(err) = SpawnAgentHandler.handle(invocation).await else { + let Err(err) = SpawnAgentHandler::default().handle(invocation).await else { panic!("spawn should fail when depth limit exceeded"); }; assert_eq!( @@ -1855,7 +1855,7 @@ async fn spawn_agent_allows_depth_up_to_configured_max_depth() { "spawn_agent", function_payload(json!({"message": "hello"})), ); - let output = SpawnAgentHandler + let output = SpawnAgentHandler::default() .handle(invocation) .await .expect("spawn should succeed within configured depth"); @@ -1914,7 +1914,7 @@ async fn multi_agent_v2_spawn_agent_ignores_configured_max_depth() { "fork_turns": "none" })), ); - let output = SpawnAgentHandlerV2 + let output = SpawnAgentHandlerV2::default() .handle(invocation) .await .expect("multi-agent v2 spawn should ignore max depth"); @@ -2306,7 +2306,7 @@ async fn wait_agent_rejects_non_positive_timeout() { "timeout_ms": 0 })), ); - let Err(err) = WaitAgentHandler.handle(invocation).await else { + let Err(err) = WaitAgentHandler::default().handle(invocation).await else { panic!("non-positive timeout should be rejected"); }; assert_eq!( @@ -2324,7 +2324,7 @@ async fn wait_agent_rejects_invalid_target() { "wait_agent", function_payload(json!({"targets": ["invalid"]})), ); - let Err(err) = WaitAgentHandler.handle(invocation).await else { + let Err(err) = WaitAgentHandler::default().handle(invocation).await else { panic!("invalid id should be rejected"); }; let FunctionCallError::RespondToModel(msg) = err else { @@ -2342,7 +2342,7 @@ async fn wait_agent_rejects_empty_targets() { "wait_agent", function_payload(json!({"targets": []})), ); - let Err(err) = WaitAgentHandler.handle(invocation).await else { + let Err(err) = WaitAgentHandler::default().handle(invocation).await else { panic!("empty ids should be rejected"); }; assert_eq!( @@ -2370,7 +2370,7 @@ async fn multi_agent_v2_wait_agent_accepts_timeout_only_argument() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -2400,7 +2400,7 @@ async fn multi_agent_v2_wait_agent_accepts_timeout_only_argument() { let session = session.clone(); let turn = turn.clone(); async move { - WaitAgentHandlerV2 + WaitAgentHandlerV2::default() .handle(invocation( session, turn, @@ -2452,7 +2452,7 @@ async fn multi_agent_v2_wait_agent_uses_configured_min_timeout() { let early = timeout( Duration::from_millis(/*millis*/ 20), - WaitAgentHandlerV2.handle(invocation( + WaitAgentHandlerV2::default().handle(invocation( session.clone(), turn.clone(), "wait_agent", @@ -2467,7 +2467,7 @@ async fn multi_agent_v2_wait_agent_uses_configured_min_timeout() { let output = timeout( Duration::from_secs(/*secs*/ 1), - WaitAgentHandlerV2.handle(invocation( + WaitAgentHandlerV2::default().handle(invocation( session, turn, "wait_agent", @@ -2506,7 +2506,7 @@ async fn wait_agent_returns_not_found_for_missing_agents() { "timeout_ms": 1000 })), ); - let output = WaitAgentHandler + let output = WaitAgentHandler::default() .handle(invocation) .await .expect("wait_agent should succeed"); @@ -2546,7 +2546,7 @@ async fn wait_agent_times_out_when_status_is_not_final() { "timeout_ms": MIN_WAIT_TIMEOUT_MS })), ); - let output = WaitAgentHandler + let output = WaitAgentHandler::default() .handle(invocation) .await .expect("wait_agent should succeed"); @@ -2592,7 +2592,7 @@ async fn wait_agent_clamps_short_timeouts_to_minimum() { let early = timeout( Duration::from_millis(50), - WaitAgentHandler.handle(invocation), + WaitAgentHandler::default().handle(invocation), ) .await; assert!( @@ -2642,7 +2642,7 @@ async fn wait_agent_returns_final_status_without_timeout() { "timeout_ms": 1000 })), ); - let output = WaitAgentHandler + let output = WaitAgentHandler::default() .handle(invocation) .await .expect("wait_agent should succeed"); @@ -2678,7 +2678,7 @@ async fn multi_agent_v2_wait_agent_returns_summary_for_mailbox_activity() { let session = Arc::new(session); let turn = Arc::new(turn); - let spawn_output = SpawnAgentHandlerV2 + let spawn_output = SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -2713,7 +2713,7 @@ async fn multi_agent_v2_wait_agent_returns_summary_for_mailbox_activity() { let session = session.clone(); let turn = turn.clone(); async move { - WaitAgentHandlerV2 + WaitAgentHandlerV2::default() .handle(invocation( session, turn, @@ -2769,7 +2769,7 @@ async fn multi_agent_v2_wait_agent_returns_for_already_queued_mail() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -2805,7 +2805,7 @@ async fn multi_agent_v2_wait_agent_returns_for_already_queued_mail() { let output = timeout( Duration::from_millis(500), - WaitAgentHandlerV2.handle(invocation( + WaitAgentHandlerV2::default().handle(invocation( session, turn, "wait_agent", @@ -2848,7 +2848,7 @@ async fn multi_agent_v2_wait_agent_wakes_on_any_mailbox_notification() { let turn = Arc::new(turn); for task_name in ["worker_a", "worker_b"] { - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -2879,7 +2879,7 @@ async fn multi_agent_v2_wait_agent_wakes_on_any_mailbox_notification() { let session = session.clone(); let turn = turn.clone(); async move { - WaitAgentHandlerV2 + WaitAgentHandlerV2::default() .handle(invocation( session, turn, @@ -2935,7 +2935,7 @@ async fn multi_agent_v2_wait_agent_does_not_return_completed_content() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -2964,7 +2964,7 @@ async fn multi_agent_v2_wait_agent_does_not_return_completed_content() { let session = session.clone(); let turn = turn.clone(); async move { - WaitAgentHandlerV2 + WaitAgentHandlerV2::default() .handle(invocation( session, turn, @@ -3021,7 +3021,7 @@ async fn multi_agent_v2_close_agent_accepts_task_name_target() { let session = Arc::new(session); let turn = Arc::new(turn); - SpawnAgentHandlerV2 + SpawnAgentHandlerV2::default() .handle(invocation( session.clone(), turn.clone(), @@ -3176,7 +3176,7 @@ async fn tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtr let parent_thread_id = parent.thread_id; let parent_session = parent.thread.codex.session.clone(); - let child_spawn_output = SpawnAgentHandler + let child_spawn_output = SpawnAgentHandler::default() .handle(invocation( parent_session.clone(), parent_session.new_default_turn().await, @@ -3201,7 +3201,7 @@ async fn tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtr .await .expect("child thread should exist"); let child_session = child_thread.codex.session.clone(); - let grandchild_spawn_output = SpawnAgentHandler + let grandchild_spawn_output = SpawnAgentHandler::default() .handle(invocation( child_session.clone(), child_session.new_default_turn().await, diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs index f09bc7f343..d3a290d363 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/close_agent.rs @@ -1,5 +1,7 @@ use super::*; +use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v2; use crate::turn_timing::now_unix_timestamp_ms; +use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -10,6 +12,10 @@ impl ToolHandler for Handler { ToolName::plain("close_agent") } + fn spec(&self) -> Option { + Some(create_close_agent_tool_v2()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/followup_task.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/followup_task.rs index a5dfcb09dd..147d238b7e 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/followup_task.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/followup_task.rs @@ -3,6 +3,8 @@ use super::message_tool::MessageDeliveryMode; use super::message_tool::handle_message_string_tool; use super::*; use crate::tools::context::FunctionToolOutput; +use crate::tools::handlers::multi_agents_spec::create_followup_task_tool; +use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -13,6 +15,10 @@ impl ToolHandler for Handler { ToolName::plain("followup_task") } + fn spec(&self) -> Option { + Some(create_followup_task_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/list_agents.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/list_agents.rs index dabfe72a7d..37365f2a5d 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/list_agents.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/list_agents.rs @@ -1,5 +1,7 @@ use super::*; use crate::agent::control::ListedAgent; +use crate::tools::handlers::multi_agents_spec::create_list_agents_tool; +use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -10,6 +12,10 @@ impl ToolHandler for Handler { ToolName::plain("list_agents") } + fn spec(&self) -> Option { + Some(create_list_agents_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/send_message.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/send_message.rs index e814c69f5f..38a0cc3ab8 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/send_message.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/send_message.rs @@ -3,6 +3,8 @@ use super::message_tool::SendMessageArgs; use super::message_tool::handle_message_string_tool; use super::*; use crate::tools::context::FunctionToolOutput; +use crate::tools::handlers::multi_agents_spec::create_send_message_tool; +use codex_tools::ToolSpec; pub(crate) struct Handler; @@ -13,6 +15,10 @@ impl ToolHandler for Handler { ToolName::plain("send_message") } + fn spec(&self) -> Option { + Some(create_send_message_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs index 8f09dbcaf2..fc44d3df47 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs @@ -5,12 +5,24 @@ use crate::agent::control::render_input_preview; use crate::agent::next_thread_spawn_depth; use crate::agent::role::DEFAULT_ROLE_NAME; use crate::agent::role::apply_role_to_config; +use crate::tools::handlers::multi_agents_spec::SpawnAgentToolOptions; +use crate::tools::handlers::multi_agents_spec::create_spawn_agent_tool_v2; use crate::turn_timing::now_unix_timestamp_ms; use codex_protocol::AgentPath; use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::Op; +use codex_tools::ToolSpec; -pub(crate) struct Handler; +#[derive(Default)] +pub(crate) struct Handler { + options: SpawnAgentToolOptions, +} + +impl Handler { + pub(crate) fn new(options: SpawnAgentToolOptions) -> Self { + Self { options } + } +} impl ToolHandler for Handler { type Output = SpawnAgentResult; @@ -19,6 +31,10 @@ impl ToolHandler for Handler { ToolName::plain("spawn_agent") } + fn spec(&self) -> Option { + Some(create_spawn_agent_tool_v2(self.options.clone())) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs index 706c0ad6ac..d5fbe49c7e 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/wait.rs @@ -1,11 +1,23 @@ use super::*; +use crate::tools::handlers::multi_agents_spec::WaitAgentTimeoutOptions; +use crate::tools::handlers::multi_agents_spec::create_wait_agent_tool_v2; use crate::turn_timing::now_unix_timestamp_ms; +use codex_tools::ToolSpec; use std::collections::HashMap; use std::time::Duration; use tokio::time::Instant; use tokio::time::timeout_at; -pub(crate) struct Handler; +#[derive(Default)] +pub(crate) struct Handler { + options: WaitAgentTimeoutOptions, +} + +impl Handler { + pub(crate) fn new(options: WaitAgentTimeoutOptions) -> Self { + Self { options } + } +} impl ToolHandler for Handler { type Output = WaitAgentResult; @@ -14,6 +26,10 @@ impl ToolHandler for Handler { ToolName::plain("wait_agent") } + fn spec(&self) -> Option { + Some(create_wait_agent_tool_v2(self.options)) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/plan.rs b/codex-rs/core/src/tools/handlers/plan.rs index ce217f457d..2995fbbc25 100644 --- a/codex-rs/core/src/tools/handlers/plan.rs +++ b/codex-rs/core/src/tools/handlers/plan.rs @@ -2,6 +2,7 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; +use crate::tools::handlers::plan_spec::create_update_plan_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_protocol::config_types::ModeKind; @@ -10,6 +11,7 @@ use codex_protocol::models::ResponseInputItem; use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::EventMsg; use codex_tools::ToolName; +use codex_tools::ToolSpec; use serde_json::Value as JsonValue; pub struct PlanHandler; @@ -49,6 +51,10 @@ impl ToolHandler for PlanHandler { ToolName::plain("update_plan") } + fn spec(&self) -> Option { + Some(create_update_plan_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/request_permissions.rs b/codex-rs/core/src/tools/handlers/request_permissions.rs index 7b49ec5803..01d1820578 100644 --- a/codex-rs/core/src/tools/handlers/request_permissions.rs +++ b/codex-rs/core/src/tools/handlers/request_permissions.rs @@ -6,9 +6,12 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments_with_base_path; +use crate::tools::handlers::shell_spec::create_request_permissions_tool; +use crate::tools::handlers::shell_spec::request_permissions_tool_description; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; pub struct RequestPermissionsHandler; @@ -19,6 +22,12 @@ impl ToolHandler for RequestPermissionsHandler { ToolName::plain("request_permissions") } + fn spec(&self) -> Option { + Some(create_request_permissions_tool( + request_permissions_tool_description(), + )) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/request_plugin_install.rs b/codex-rs/core/src/tools/handlers/request_plugin_install.rs index 879a8955b4..7a9ace3848 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install.rs @@ -12,10 +12,13 @@ use codex_tools::REQUEST_PLUGIN_INSTALL_PERSIST_ALWAYS_VALUE; use codex_tools::REQUEST_PLUGIN_INSTALL_PERSIST_KEY; use codex_tools::REQUEST_PLUGIN_INSTALL_TOOL_NAME; use codex_tools::RequestPluginInstallArgs; +use codex_tools::RequestPluginInstallEntry; use codex_tools::RequestPluginInstallResult; use codex_tools::ToolName; +use codex_tools::ToolSpec; use codex_tools::all_requested_connectors_picked_up; use codex_tools::build_request_plugin_install_elicitation_request; +use codex_tools::collect_request_plugin_install_entries; use codex_tools::filter_request_plugin_install_discoverable_tools_for_client; use codex_tools::verified_connector_install_completed; use rmcp::model::RequestId; @@ -30,10 +33,22 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments; +use crate::tools::handlers::request_plugin_install_spec::create_request_plugin_install_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; -pub struct RequestPluginInstallHandler; +#[derive(Default)] +pub struct RequestPluginInstallHandler { + discoverable_tools: Vec, +} + +impl RequestPluginInstallHandler { + pub(crate) fn new(discoverable_tools: &[DiscoverableTool]) -> Self { + Self { + discoverable_tools: collect_request_plugin_install_entries(discoverable_tools), + } + } +} impl ToolHandler for RequestPluginInstallHandler { type Output = FunctionToolOutput; @@ -42,6 +57,14 @@ impl ToolHandler for RequestPluginInstallHandler { ToolName::plain(REQUEST_PLUGIN_INSTALL_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_request_plugin_install_tool(&self.discoverable_tools)) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/request_user_input.rs b/codex-rs/core/src/tools/handlers/request_user_input.rs index a30fe9a074..6d26234858 100644 --- a/codex-rs/core/src/tools/handlers/request_user_input.rs +++ b/codex-rs/core/src/tools/handlers/request_user_input.rs @@ -4,13 +4,16 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::request_user_input_spec::REQUEST_USER_INPUT_TOOL_NAME; +use crate::tools::handlers::request_user_input_spec::create_request_user_input_tool; use crate::tools::handlers::request_user_input_spec::normalize_request_user_input_args; +use crate::tools::handlers::request_user_input_spec::request_user_input_tool_description; use crate::tools::handlers::request_user_input_spec::request_user_input_unavailable_message; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_protocol::config_types::ModeKind; use codex_protocol::request_user_input::RequestUserInputArgs; use codex_tools::ToolName; +use codex_tools::ToolSpec; pub struct RequestUserInputHandler { pub available_modes: Vec, @@ -23,6 +26,12 @@ impl ToolHandler for RequestUserInputHandler { ToolName::plain(REQUEST_USER_INPUT_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_request_user_input_tool( + request_user_input_tool_description(&self.available_modes), + )) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index a0d744305e..f6960bca41 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -38,6 +38,7 @@ mod shell_handler; pub use container_exec::ContainerExecHandler; pub use local_shell::LocalShellHandler; pub use shell_command::ShellCommandHandler; +pub(crate) use shell_command::ShellCommandHandlerOptions; pub use shell_handler::ShellHandler; fn shell_function_payload_command(payload: &ToolPayload) -> Option { diff --git a/codex-rs/core/src/tools/handlers/shell/local_shell.rs b/codex-rs/core/src/tools/handlers/shell/local_shell.rs index bdb70e9368..1a75fb998e 100644 --- a/codex-rs/core/src/tools/handlers/shell/local_shell.rs +++ b/codex-rs/core/src/tools/handlers/shell/local_shell.rs @@ -12,13 +12,24 @@ use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; +use codex_tools::ToolSpec; +use super::super::shell_spec::create_local_shell_tool; use super::RunExecLikeArgs; use super::local_shell_payload_command; use super::run_exec_like; use super::shell_handler::ShellHandler; -pub struct LocalShellHandler; +#[derive(Default)] +pub struct LocalShellHandler { + include_spec: bool, +} + +impl LocalShellHandler { + pub(crate) fn new() -> Self { + Self { include_spec: true } + } +} impl ToolHandler for LocalShellHandler { type Output = FunctionToolOutput; @@ -27,6 +38,14 @@ impl ToolHandler for LocalShellHandler { ToolName::plain("local_shell") } + fn spec(&self) -> Option { + self.include_spec.then(create_local_shell_tool) + } + + fn supports_parallel_tool_calls(&self) -> bool { + self.include_spec + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index 69f965b51e..3b5e4e77e5 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -23,7 +23,10 @@ use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; +use codex_tools::ToolSpec; +use super::super::shell_spec::CommandToolOptions; +use super::super::shell_spec::create_shell_command_tool; use super::RunExecLikeArgs; use super::run_exec_like; use super::shell_command_payload_command; @@ -36,9 +39,24 @@ enum ShellCommandBackend { pub struct ShellCommandHandler { backend: ShellCommandBackend, + options: Option, +} + +#[derive(Clone, Copy)] +pub(crate) struct ShellCommandHandlerOptions { + pub(crate) backend_config: ShellCommandBackendConfig, + pub(crate) allow_login_shell: bool, + pub(crate) exec_permission_approvals_enabled: bool, } impl ShellCommandHandler { + pub(crate) fn new(options: ShellCommandHandlerOptions) -> Self { + Self { + options: Some(options), + ..Self::from(options.backend_config) + } + } + fn shell_runtime_backend(&self) -> ShellRuntimeBackend { match self.backend { ShellCommandBackend::Classic => ShellRuntimeBackend::ShellCommandClassic, @@ -99,7 +117,10 @@ impl From for ShellCommandHandler { ShellCommandBackendConfig::Classic => ShellCommandBackend::Classic, ShellCommandBackendConfig::ZshFork => ShellCommandBackend::ZshFork, }; - Self { backend } + Self { + backend, + options: None, + } } } @@ -110,6 +131,19 @@ impl ToolHandler for ShellCommandHandler { ToolName::plain("shell_command") } + fn spec(&self) -> Option { + self.options.map(|options| { + create_shell_command_tool(CommandToolOptions { + allow_login_shell: options.allow_login_shell, + exec_permission_approvals_enabled: options.exec_permission_approvals_enabled, + }) + }) + } + + fn supports_parallel_tool_calls(&self) -> bool { + self.options.is_some() + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/shell/shell_handler.rs b/codex-rs/core/src/tools/handlers/shell/shell_handler.rs index 30220d3db0..34ba8a2a8e 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_handler.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_handler.rs @@ -18,15 +18,27 @@ use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::runtimes::shell::ShellRuntimeBackend; +use codex_tools::ToolSpec; +use super::super::shell_spec::ShellToolOptions; +use super::super::shell_spec::create_shell_tool; use super::RunExecLikeArgs; use super::run_exec_like; use super::shell_function_post_tool_use_payload; use super::shell_function_pre_tool_use_payload; -pub struct ShellHandler; +#[derive(Default)] +pub struct ShellHandler { + options: Option, +} impl ShellHandler { + pub(crate) fn new(options: ShellToolOptions) -> Self { + Self { + options: Some(options), + } + } + pub(super) fn to_exec_params( params: &ShellToolCallParams, turn_context: &TurnContext, @@ -58,6 +70,14 @@ impl ToolHandler for ShellHandler { ToolName::plain("shell") } + fn spec(&self) -> Option { + self.options.map(create_shell_tool) + } + + fn supports_parallel_tool_calls(&self) -> bool { + self.options.is_some() + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index a7e6dae35c..ce97b8317e 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -221,7 +221,7 @@ async fn local_shell_pre_tool_use_payload_uses_joined_command() { }, }; let (session, turn) = make_session_and_context().await; - let handler = LocalShellHandler; + let handler = LocalShellHandler::default(); assert_eq!( handler.pre_tool_use_payload(&ToolInvocation { diff --git a/codex-rs/core/src/tools/handlers/test_sync.rs b/codex-rs/core/src/tools/handlers/test_sync.rs index e04400d17e..6254b94829 100644 --- a/codex-rs/core/src/tools/handlers/test_sync.rs +++ b/codex-rs/core/src/tools/handlers/test_sync.rs @@ -13,9 +13,11 @@ use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments; +use crate::tools::handlers::test_sync_spec::create_test_sync_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; pub struct TestSyncHandler; @@ -61,6 +63,14 @@ impl ToolHandler for TestSyncHandler { ToolName::plain("test_sync_tool") } + fn spec(&self) -> Option { + Some(create_test_sync_tool()) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index bc2e727dfb..70410db3ae 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -2,6 +2,7 @@ use crate::function_tool::FunctionCallError; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::ToolSearchOutput; +use crate::tools::handlers::tool_search_spec::create_tool_search_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use crate::tools::tool_search_entry::ToolSearchEntry; @@ -13,6 +14,8 @@ use codex_tools::LoadableToolSpec; use codex_tools::TOOL_SEARCH_DEFAULT_LIMIT; use codex_tools::TOOL_SEARCH_TOOL_NAME; use codex_tools::ToolName; +use codex_tools::ToolSearchSourceInfo; +use codex_tools::ToolSpec; use codex_tools::coalesce_loadable_tool_specs; use std::collections::HashMap; @@ -21,11 +24,15 @@ const COMPUTER_USE_TOOL_SEARCH_LIMIT: usize = 20; pub struct ToolSearchHandler { entries: Vec, + search_source_infos: Vec, search_engine: SearchEngine, } impl ToolSearchHandler { - pub(crate) fn new(entries: Vec) -> Self { + pub(crate) fn new( + entries: Vec, + search_source_infos: Vec, + ) -> Self { let documents: Vec> = entries .iter() .map(|entry| entry.search_text.clone()) @@ -37,6 +44,7 @@ impl ToolSearchHandler { Self { entries, + search_source_infos, search_engine, } } @@ -49,6 +57,17 @@ impl ToolHandler for ToolSearchHandler { ToolName::plain(TOOL_SEARCH_TOOL_NAME) } + fn spec(&self) -> Option { + Some(create_tool_search_tool( + &self.search_source_infos, + TOOL_SEARCH_DEFAULT_LIMIT, + )) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } @@ -415,6 +434,9 @@ mod tests { mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> ToolSearchHandler { - ToolSearchHandler::new(build_tool_search_entries(mcp_tools, dynamic_tools)) + ToolSearchHandler::new( + build_tool_search_entries(mcp_tools, dynamic_tools), + Vec::new(), + ) } } diff --git a/codex-rs/core/src/tools/handlers/unavailable_tool.rs b/codex-rs/core/src/tools/handlers/unavailable_tool.rs index 64bb200581..b0a41e9fa7 100644 --- a/codex-rs/core/src/tools/handlers/unavailable_tool.rs +++ b/codex-rs/core/src/tools/handlers/unavailable_tool.rs @@ -5,14 +5,26 @@ use crate::tools::context::ToolPayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; pub struct UnavailableToolHandler { tool_name: ToolName, + spec: Option, } impl UnavailableToolHandler { - pub fn new(tool_name: ToolName) -> Self { - Self { tool_name } + 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, + } } } @@ -32,6 +44,10 @@ impl ToolHandler for UnavailableToolHandler { self.tool_name.clone() } + fn spec(&self) -> Option { + self.spec.clone() + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 80e85ccd47..c97f5bb6f2 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -22,6 +22,7 @@ mod exec_command; mod write_stdin; pub use exec_command::ExecCommandHandler; +pub(crate) use exec_command::ExecCommandHandlerOptions; pub use write_stdin::WriteStdinHandler; #[derive(Debug, Deserialize)] diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 75ae3fea29..351fb2e983 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -27,15 +27,45 @@ use codex_otel::SessionTelemetry; use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ToolName; +use codex_tools::ToolSpec; use codex_utils_output_truncation::approx_token_count; +use super::super::shell_spec::CommandToolOptions; +use super::super::shell_spec::create_exec_command_tool_with_environment_id; use super::ExecCommandArgs; use super::ExecCommandEnvironmentArgs; use super::effective_max_output_tokens; use super::get_command; use super::post_unified_exec_tool_use_payload; -pub struct ExecCommandHandler; +#[derive(Clone, Copy)] +pub(crate) struct ExecCommandHandlerOptions { + pub(crate) allow_login_shell: bool, + pub(crate) exec_permission_approvals_enabled: bool, + pub(crate) include_environment_id: bool, +} + +pub struct ExecCommandHandler { + options: ExecCommandHandlerOptions, +} + +impl Default for ExecCommandHandler { + fn default() -> Self { + Self { + options: ExecCommandHandlerOptions { + allow_login_shell: false, + exec_permission_approvals_enabled: false, + include_environment_id: false, + }, + } + } +} + +impl ExecCommandHandler { + pub(crate) fn new(options: ExecCommandHandlerOptions) -> Self { + Self { options } + } +} impl ToolHandler for ExecCommandHandler { type Output = ExecCommandToolOutput; @@ -44,6 +74,20 @@ impl ToolHandler for ExecCommandHandler { ToolName::plain("exec_command") } + fn spec(&self) -> Option { + Some(create_exec_command_tool_with_environment_id( + CommandToolOptions { + allow_login_shell: self.options.allow_login_shell, + exec_permission_approvals_enabled: self.options.exec_permission_approvals_enabled, + }, + self.options.include_environment_id, + )) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs index 1e9c68f227..b7be04f2a3 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs @@ -10,8 +10,10 @@ use crate::unified_exec::WriteStdinRequest; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::TerminalInteractionEvent; use codex_tools::ToolName; +use codex_tools::ToolSpec; use serde::Deserialize; +use super::super::shell_spec::create_write_stdin_tool; use super::effective_max_output_tokens; use super::post_unified_exec_tool_use_payload; @@ -36,6 +38,10 @@ impl ToolHandler for WriteStdinHandler { ToolName::plain("write_stdin") } + fn spec(&self) -> Option { + Some(create_write_stdin_tool()) + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 8818b2e344..02123c4b64 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -184,7 +184,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() { arguments: serde_json::json!({ "cmd": "printf exec command" }).to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = ExecCommandHandler; + let handler = ExecCommandHandler::default(); assert_eq!( handler.pre_tool_use_payload(&ToolInvocation { @@ -244,7 +244,7 @@ async fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_s hook_command: Some("echo three".to_string()), }; let invocation = invocation_for_payload("exec_command", "call-43", payload).await; - let handler = ExecCommandHandler; + let handler = ExecCommandHandler::default(); assert_eq!( handler.post_tool_use_payload(&invocation, &output), Some(crate::tools::registry::PostToolUsePayload { @@ -273,7 +273,7 @@ async fn exec_command_post_tool_use_payload_uses_output_for_interactive_completi hook_command: Some("echo three".to_string()), }; let invocation = invocation_for_payload("exec_command", "call-44", payload).await; - let handler = ExecCommandHandler; + let handler = ExecCommandHandler::default(); assert_eq!( handler.post_tool_use_payload(&invocation, &output), @@ -303,7 +303,7 @@ async fn exec_command_post_tool_use_payload_skips_running_sessions() { hook_command: Some("echo three".to_string()), }; let invocation = invocation_for_payload("exec_command", "call-45", payload).await; - let handler = ExecCommandHandler; + let handler = ExecCommandHandler::default(); assert_eq!(handler.post_tool_use_payload(&invocation, &output), None); } diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index a7cbe7d975..d52a844159 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -17,11 +17,32 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments; +use crate::tools::handlers::view_image_spec::ViewImageToolOptions; +use crate::tools::handlers::view_image_spec::create_view_image_tool; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use codex_tools::ToolName; +use codex_tools::ToolSpec; -pub struct ViewImageHandler; +pub struct ViewImageHandler { + options: ViewImageToolOptions, +} + +impl Default for ViewImageHandler { + fn default() -> Self { + Self { + options: ViewImageToolOptions { + can_request_original_image_detail: false, + }, + } + } +} + +impl ViewImageHandler { + pub(crate) fn new(options: ViewImageToolOptions) -> Self { + Self { options } + } +} const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = "view_image is not allowed because you do not support image inputs"; @@ -44,6 +65,14 @@ impl ToolHandler for ViewImageHandler { ToolName::plain("view_image") } + fn spec(&self) -> Option { + Some(create_view_image_tool(self.options)) + } + + fn supports_parallel_tool_calls(&self) -> bool { + true + } + fn kind(&self) -> ToolKind { ToolKind::Function } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 98e8e227aa..c1b5854b68 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -18,6 +18,7 @@ use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::hook_names::HookToolName; use crate::tools::tool_dispatch_trace::ToolDispatchTrace; +use crate::util::error_or_panic; use codex_hooks::HookEvent; use codex_hooks::HookEventAfterToolUse; use codex_hooks::HookPayload; @@ -47,6 +48,14 @@ pub trait ToolHandler: Send + Sync { /// The concrete tool name handled by this handler instance. fn tool_name(&self) -> ToolName; + fn spec(&self) -> Option { + None + } + + fn supports_parallel_tool_calls(&self) -> bool { + false + } + fn kind(&self) -> ToolKind; fn matches_kind(&self, payload: &ToolPayload) -> bool { @@ -512,37 +521,26 @@ impl ToolRegistry { pub struct ToolRegistryBuilder { handlers: HashMap>, specs: Vec, + code_mode_enabled: bool, } impl ToolRegistryBuilder { - pub fn new() -> Self { + pub fn new(code_mode_enabled: bool) -> Self { Self { handlers: HashMap::new(), specs: Vec::new(), + code_mode_enabled, } } - pub fn push_spec_with_parallel_support( - &mut self, - spec: ToolSpec, - supports_parallel_tool_calls: bool, - ) { - self.specs - .push(ConfiguredToolSpec::new(spec, supports_parallel_tool_calls)); - } - - pub(crate) fn push_spec( - &mut self, - spec: ToolSpec, - supports_parallel_tool_calls: bool, - code_mode_enabled: bool, - ) { - let spec = if code_mode_enabled { + pub(crate) fn push_spec(&mut self, spec: ToolSpec, supports_parallel_tool_calls: bool) { + let spec = if self.code_mode_enabled { codex_tools::augment_tool_spec_for_code_mode(spec) } else { spec }; - self.push_spec_with_parallel_support(spec, supports_parallel_tool_calls); + self.specs + .push(ConfiguredToolSpec::new(spec, supports_parallel_tool_calls)); } pub fn register_handler(&mut self, handler: Arc) @@ -550,11 +548,18 @@ impl ToolRegistryBuilder { H: ToolHandler + 'static, { let name = handler.tool_name(); - let display_name = name.display(); - let handler: Arc = handler; - if self.handlers.insert(name, handler).is_some() { - warn!("overwriting handler for tool {display_name}"); + if self.handlers.contains_key(&name) { + error_or_panic(format!("handler for tool {name} already registered")); + return; } + + if let Some(spec) = handler.spec() { + let supports_parallel_tool_calls = handler.supports_parallel_tool_calls(); + self.push_spec(spec, supports_parallel_tool_calls); + } + + let handler: Arc = handler; + self.handlers.insert(name, handler); } pub(crate) fn specs(&self) -> &[ConfiguredToolSpec] { diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index ef7273999d..d445b196a2 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -1,4 +1,7 @@ use super::*; +use crate::tools::handlers::GetGoalHandler; +use crate::tools::handlers::goal_spec::GET_GOAL_TOOL_NAME; +use crate::tools::handlers::goal_spec::create_get_goal_tool; use pretty_assertions::assert_eq; struct TestHandler { @@ -62,3 +65,18 @@ fn handler_looks_up_namespaced_aliases_explicitly() { .is_some_and(|handler| Arc::ptr_eq(handler, &namespaced_handler)) ); } + +#[test] +fn register_handler_adds_handler_and_augments_specs_for_code_mode() { + let mut builder = ToolRegistryBuilder::new(/*code_mode_enabled*/ true); + builder.register_handler(Arc::new(GetGoalHandler)); + + let (specs, registry) = builder.build(); + + assert_eq!(specs.len(), 1); + assert_eq!( + specs[0].spec, + codex_tools::augment_tool_spec_for_code_mode(create_get_goal_tool()) + ); + assert!(registry.has_handler(&codex_tools::ToolName::plain(GET_GOAL_TOOL_NAME))); +} diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index b1dfb9f865..a3e93f8436 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -153,13 +153,15 @@ pub(crate) fn build_specs_with_discoverable_tools( output_schema: None, defer_loading: None, }); - builder.push_spec( + builder.register_handler(Arc::new(UnavailableToolHandler::new( + unavailable_tool, spec, - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); + ))); + } else { + builder.register_handler(Arc::new(UnavailableToolHandler::without_spec( + unavailable_tool, + ))); } - builder.register_handler(Arc::new(UnavailableToolHandler::new(unavailable_tool))); } builder } diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index bca9d549a7..6445099618 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -1,5 +1,4 @@ use crate::tools::code_mode::execute_spec::create_code_mode_tool; -use crate::tools::code_mode::wait_spec::create_wait_tool; use crate::tools::handlers::ApplyPatchHandler; use crate::tools::handlers::CodeModeExecuteHandler; use crate::tools::handlers::CodeModeWaitHandler; @@ -7,6 +6,7 @@ use crate::tools::handlers::ContainerExecHandler; use crate::tools::handlers::CreateGoalHandler; use crate::tools::handlers::DynamicToolHandler; use crate::tools::handlers::ExecCommandHandler; +use crate::tools::handlers::ExecCommandHandlerOptions; use crate::tools::handlers::GetGoalHandler; use crate::tools::handlers::ListMcpResourceTemplatesHandler; use crate::tools::handlers::ListMcpResourcesHandler; @@ -18,6 +18,7 @@ use crate::tools::handlers::RequestPermissionsHandler; use crate::tools::handlers::RequestPluginInstallHandler; use crate::tools::handlers::RequestUserInputHandler; use crate::tools::handlers::ShellCommandHandler; +use crate::tools::handlers::ShellCommandHandlerOptions; use crate::tools::handlers::ShellHandler; use crate::tools::handlers::TestSyncHandler; use crate::tools::handlers::ToolSearchHandler; @@ -26,67 +27,29 @@ use crate::tools::handlers::ViewImageHandler; use crate::tools::handlers::WriteStdinHandler; use crate::tools::handlers::agent_jobs::ReportAgentJobResultHandler; use crate::tools::handlers::agent_jobs::SpawnAgentsOnCsvHandler; -use crate::tools::handlers::agent_jobs_spec::create_report_agent_job_result_tool; -use crate::tools::handlers::agent_jobs_spec::create_spawn_agents_on_csv_tool; -use crate::tools::handlers::apply_patch_spec::create_apply_patch_freeform_tool; -use crate::tools::handlers::apply_patch_spec::create_apply_patch_json_tool; -use crate::tools::handlers::goal_spec::create_create_goal_tool; -use crate::tools::handlers::goal_spec::create_get_goal_tool; -use crate::tools::handlers::goal_spec::create_update_goal_tool; -use crate::tools::handlers::mcp_resource_spec::create_list_mcp_resource_templates_tool; -use crate::tools::handlers::mcp_resource_spec::create_list_mcp_resources_tool; -use crate::tools::handlers::mcp_resource_spec::create_read_mcp_resource_tool; use crate::tools::handlers::multi_agents::CloseAgentHandler; use crate::tools::handlers::multi_agents::ResumeAgentHandler; use crate::tools::handlers::multi_agents::SendInputHandler; use crate::tools::handlers::multi_agents::SpawnAgentHandler; use crate::tools::handlers::multi_agents::WaitAgentHandler; use crate::tools::handlers::multi_agents_spec::SpawnAgentToolOptions; -use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v1; -use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v2; -use crate::tools::handlers::multi_agents_spec::create_followup_task_tool; -use crate::tools::handlers::multi_agents_spec::create_list_agents_tool; -use crate::tools::handlers::multi_agents_spec::create_resume_agent_tool; -use crate::tools::handlers::multi_agents_spec::create_send_input_tool_v1; -use crate::tools::handlers::multi_agents_spec::create_send_message_tool; -use crate::tools::handlers::multi_agents_spec::create_spawn_agent_tool_v1; -use crate::tools::handlers::multi_agents_spec::create_spawn_agent_tool_v2; -use crate::tools::handlers::multi_agents_spec::create_wait_agent_tool_v1; -use crate::tools::handlers::multi_agents_spec::create_wait_agent_tool_v2; use crate::tools::handlers::multi_agents_v2::CloseAgentHandler as CloseAgentHandlerV2; use crate::tools::handlers::multi_agents_v2::FollowupTaskHandler as FollowupTaskHandlerV2; use crate::tools::handlers::multi_agents_v2::ListAgentsHandler as ListAgentsHandlerV2; use crate::tools::handlers::multi_agents_v2::SendMessageHandler as SendMessageHandlerV2; use crate::tools::handlers::multi_agents_v2::SpawnAgentHandler as SpawnAgentHandlerV2; use crate::tools::handlers::multi_agents_v2::WaitAgentHandler as WaitAgentHandlerV2; -use crate::tools::handlers::plan_spec::create_update_plan_tool; -use crate::tools::handlers::request_plugin_install_spec::create_request_plugin_install_tool; -use crate::tools::handlers::request_user_input_spec::create_request_user_input_tool; -use crate::tools::handlers::request_user_input_spec::request_user_input_tool_description; -use crate::tools::handlers::shell_spec::CommandToolOptions; use crate::tools::handlers::shell_spec::ShellToolOptions; -use crate::tools::handlers::shell_spec::create_exec_command_tool_with_environment_id; -use crate::tools::handlers::shell_spec::create_local_shell_tool; -use crate::tools::handlers::shell_spec::create_request_permissions_tool; -use crate::tools::handlers::shell_spec::create_shell_command_tool; -use crate::tools::handlers::shell_spec::create_shell_tool; -use crate::tools::handlers::shell_spec::create_write_stdin_tool; -use crate::tools::handlers::shell_spec::request_permissions_tool_description; -use crate::tools::handlers::test_sync_spec::create_test_sync_tool; -use crate::tools::handlers::tool_search_spec::create_tool_search_tool; use crate::tools::handlers::view_image_spec::ViewImageToolOptions; -use crate::tools::handlers::view_image_spec::create_view_image_tool; use crate::tools::hosted_spec::WebSearchToolOptions; use crate::tools::hosted_spec::create_image_generation_tool; use crate::tools::hosted_spec::create_web_search_tool; use crate::tools::registry::ToolRegistryBuilder; use crate::tools::spec_plan_types::ToolRegistryBuildParams; use crate::tools::spec_plan_types::agent_type_description; -use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use codex_tools::ResponsesApiNamespace; use codex_tools::ResponsesApiNamespaceTool; -use codex_tools::TOOL_SEARCH_DEFAULT_LIMIT; use codex_tools::ToolEnvironmentMode; use codex_tools::ToolName; use codex_tools::ToolSearchSource; @@ -95,7 +58,6 @@ use codex_tools::ToolSpec; use codex_tools::ToolsConfig; use codex_tools::coalesce_loadable_tool_specs; use codex_tools::collect_code_mode_exec_prompt_tool_definitions; -use codex_tools::collect_request_plugin_install_entries; use codex_tools::collect_tool_search_source_infos; use codex_tools::default_namespace_description; use codex_tools::dynamic_tool_to_loadable_tool_spec; @@ -108,7 +70,7 @@ pub fn build_tool_registry_builder( config: &ToolsConfig, params: ToolRegistryBuildParams<'_>, ) -> ToolRegistryBuilder { - let mut builder = ToolRegistryBuilder::new(); + let mut builder = ToolRegistryBuilder::new(config.code_mode_enabled); let exec_permission_approvals_enabled = config.exec_permission_approvals_enabled; if config.code_mode_enabled { @@ -142,7 +104,7 @@ pub fn build_tool_registry_builder( ); enabled_tools .sort_by(|left, right| compare_code_mode_tools(left, right, &namespace_descriptions)); - builder.push_spec( + builder.register_handler(Arc::new(CodeModeExecuteHandler::new( create_code_mode_tool( &enabled_tools, &namespace_descriptions, @@ -152,15 +114,7 @@ pub fn build_tool_registry_builder( .deferred_mcp_tools .is_some_and(|tools| !tools.is_empty()), ), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.register_handler(Arc::new(CodeModeExecuteHandler)); - builder.push_spec( - create_wait_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); + ))); builder.register_handler(Arc::new(CodeModeWaitHandler)); } @@ -169,51 +123,32 @@ pub fn build_tool_registry_builder( matches!(config.environment_mode, ToolEnvironmentMode::Multiple); match &config.shell_type { ConfigShellToolType::Default => { - builder.push_spec( - create_shell_tool(ShellToolOptions { - exec_permission_approvals_enabled, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); + builder.register_handler(Arc::new(ShellHandler::new(ShellToolOptions { + exec_permission_approvals_enabled, + }))); } ConfigShellToolType::Local => { - builder.push_spec( - create_local_shell_tool(), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); + builder.register_handler(Arc::new(LocalShellHandler::new())); } ConfigShellToolType::UnifiedExec => { - builder.push_spec( - create_exec_command_tool_with_environment_id( - CommandToolOptions { - allow_login_shell: config.allow_login_shell, - exec_permission_approvals_enabled, - }, + builder.register_handler(Arc::new(ExecCommandHandler::new( + ExecCommandHandlerOptions { + allow_login_shell: config.allow_login_shell, + exec_permission_approvals_enabled, include_environment_id, - ), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - builder.push_spec( - create_write_stdin_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.register_handler(Arc::new(ExecCommandHandler)); + }, + ))); builder.register_handler(Arc::new(WriteStdinHandler)); } ConfigShellToolType::Disabled => {} ConfigShellToolType::ShellCommand => { - builder.push_spec( - create_shell_command_tool(CommandToolOptions { + builder.register_handler(Arc::new(ShellCommandHandler::new( + ShellCommandHandlerOptions { + backend_config: config.shell_command_backend, allow_login_shell: config.allow_login_shell, exec_permission_approvals_enabled, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); + }, + ))); } } } @@ -221,79 +156,56 @@ pub fn build_tool_registry_builder( if config.environment_mode.has_environment() && config.shell_type != ConfigShellToolType::Disabled { - builder.register_handler(Arc::new(ShellHandler)); - builder.register_handler(Arc::new(ContainerExecHandler)); - builder.register_handler(Arc::new(LocalShellHandler)); - builder.register_handler(Arc::new(ShellCommandHandler::from( - config.shell_command_backend, - ))); + match &config.shell_type { + ConfigShellToolType::Default => { + builder.register_handler(Arc::new(ContainerExecHandler)); + builder.register_handler(Arc::new(LocalShellHandler::default())); + builder.register_handler(Arc::new(ShellCommandHandler::from( + config.shell_command_backend, + ))); + } + ConfigShellToolType::Local => { + builder.register_handler(Arc::new(ShellHandler::default())); + builder.register_handler(Arc::new(ContainerExecHandler)); + builder.register_handler(Arc::new(ShellCommandHandler::from( + config.shell_command_backend, + ))); + } + ConfigShellToolType::UnifiedExec => { + builder.register_handler(Arc::new(ShellHandler::default())); + builder.register_handler(Arc::new(ContainerExecHandler)); + builder.register_handler(Arc::new(LocalShellHandler::default())); + builder.register_handler(Arc::new(ShellCommandHandler::from( + config.shell_command_backend, + ))); + } + ConfigShellToolType::ShellCommand => { + builder.register_handler(Arc::new(ShellHandler::default())); + builder.register_handler(Arc::new(ContainerExecHandler)); + builder.register_handler(Arc::new(LocalShellHandler::default())); + } + ConfigShellToolType::Disabled => {} + } } if params.mcp_tools.is_some() { - builder.push_spec( - create_list_mcp_resources_tool(), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - builder.push_spec( - create_list_mcp_resource_templates_tool(), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - builder.push_spec( - create_read_mcp_resource_tool(), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(ListMcpResourcesHandler)); builder.register_handler(Arc::new(ListMcpResourceTemplatesHandler)); builder.register_handler(Arc::new(ReadMcpResourceHandler)); } - builder.push_spec( - create_update_plan_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(PlanHandler)); if config.goal_tools { - builder.push_spec( - create_get_goal_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(GetGoalHandler)); - builder.push_spec( - create_create_goal_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(CreateGoalHandler)); - builder.push_spec( - create_update_goal_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(UpdateGoalHandler)); } - builder.push_spec( - create_request_user_input_tool(request_user_input_tool_description( - &config.request_user_input_available_modes, - )), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(RequestUserInputHandler { available_modes: config.request_user_input_available_modes.clone(), })); if config.request_permissions_tool_enabled { - builder.push_spec( - create_request_permissions_tool(request_permissions_tool_description()), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(RequestPermissionsHandler)); } @@ -330,13 +242,9 @@ pub fn build_tool_registry_builder( }); } - builder.push_spec( - create_tool_search_tool(&search_source_infos, TOOL_SEARCH_DEFAULT_LIMIT), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(ToolSearchHandler::new( params.tool_search_entries.to_vec(), + search_source_infos, ))); } @@ -344,36 +252,17 @@ pub fn build_tool_registry_builder( && let Some(discoverable_tools) = params.discoverable_tools.filter(|tools| !tools.is_empty()) { - builder.push_spec( - create_request_plugin_install_tool(&collect_request_plugin_install_entries( - discoverable_tools, - )), - /*supports_parallel_tool_calls*/ true, - /*code_mode_enabled*/ false, - ); - builder.register_handler(Arc::new(RequestPluginInstallHandler)); + builder.register_handler(Arc::new(RequestPluginInstallHandler::new( + discoverable_tools, + ))); } if config.environment_mode.has_environment() && let Some(apply_patch_tool_type) = &config.apply_patch_tool_type { - match apply_patch_tool_type { - ApplyPatchToolType::Freeform => { - builder.push_spec( - create_apply_patch_freeform_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - } - ApplyPatchToolType::Function => { - builder.push_spec( - create_apply_patch_json_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - } - } - builder.register_handler(Arc::new(ApplyPatchHandler)); + builder.register_handler(Arc::new(ApplyPatchHandler::new( + apply_patch_tool_type.clone(), + ))); } if config @@ -381,11 +270,6 @@ pub fn build_tool_registry_builder( .iter() .any(|tool| tool == "test_sync_tool") { - builder.push_spec( - create_test_sync_tool(), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(TestSyncHandler)); } @@ -394,135 +278,62 @@ pub fn build_tool_registry_builder( web_search_config: config.web_search_config.as_ref(), web_search_tool_type: config.web_search_tool_type, }) { - builder.push_spec( - web_search_tool, - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); + builder.push_spec(web_search_tool, /*supports_parallel_tool_calls*/ false); } if config.image_gen_tool { builder.push_spec( create_image_generation_tool("png"), /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, ); } if config.environment_mode.has_environment() { - builder.push_spec( - create_view_image_tool(ViewImageToolOptions { - can_request_original_image_detail: config.can_request_original_image_detail, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - builder.register_handler(Arc::new(ViewImageHandler)); + builder.register_handler(Arc::new(ViewImageHandler::new(ViewImageToolOptions { + can_request_original_image_detail: config.can_request_original_image_detail, + }))); } if config.collab_tools { if config.multi_agent_v2 { let agent_type_description = agent_type_description(config, params.default_agent_type_description); - builder.push_spec( - create_spawn_agent_tool_v2(SpawnAgentToolOptions { - available_models: &config.available_models, - agent_type_description, - hide_agent_type_model_reasoning: config.hide_spawn_agent_metadata, - include_usage_hint: config.spawn_agent_usage_hint, - usage_hint_text: config.spawn_agent_usage_hint_text.clone(), - max_concurrent_threads_per_session: config.max_concurrent_threads_per_session, - }), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_send_message_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_followup_task_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_wait_agent_tool_v2(params.wait_agent_timeouts), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_close_agent_tool_v2(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_list_agents_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.register_handler(Arc::new(SpawnAgentHandlerV2)); + builder.register_handler(Arc::new(SpawnAgentHandlerV2::new(SpawnAgentToolOptions { + available_models: config.available_models.clone(), + agent_type_description, + hide_agent_type_model_reasoning: config.hide_spawn_agent_metadata, + include_usage_hint: config.spawn_agent_usage_hint, + usage_hint_text: config.spawn_agent_usage_hint_text.clone(), + max_concurrent_threads_per_session: config.max_concurrent_threads_per_session, + }))); builder.register_handler(Arc::new(SendMessageHandlerV2)); builder.register_handler(Arc::new(FollowupTaskHandlerV2)); - builder.register_handler(Arc::new(WaitAgentHandlerV2)); + builder.register_handler(Arc::new(WaitAgentHandlerV2::new( + params.wait_agent_timeouts, + ))); builder.register_handler(Arc::new(CloseAgentHandlerV2)); builder.register_handler(Arc::new(ListAgentsHandlerV2)); } else { let agent_type_description = agent_type_description(config, params.default_agent_type_description); - builder.push_spec( - create_spawn_agent_tool_v1(SpawnAgentToolOptions { - available_models: &config.available_models, - agent_type_description, - hide_agent_type_model_reasoning: config.hide_spawn_agent_metadata, - include_usage_hint: config.spawn_agent_usage_hint, - usage_hint_text: config.spawn_agent_usage_hint_text.clone(), - max_concurrent_threads_per_session: config.max_concurrent_threads_per_session, - }), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_send_input_tool_v1(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_resume_agent_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.register_handler(Arc::new(ResumeAgentHandler)); - builder.push_spec( - create_wait_agent_tool_v1(params.wait_agent_timeouts), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.push_spec( - create_close_agent_tool_v1(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.register_handler(Arc::new(SpawnAgentHandler)); + builder.register_handler(Arc::new(SpawnAgentHandler::new(SpawnAgentToolOptions { + available_models: config.available_models.clone(), + agent_type_description, + hide_agent_type_model_reasoning: config.hide_spawn_agent_metadata, + include_usage_hint: config.spawn_agent_usage_hint, + usage_hint_text: config.spawn_agent_usage_hint_text.clone(), + max_concurrent_threads_per_session: config.max_concurrent_threads_per_session, + }))); builder.register_handler(Arc::new(SendInputHandler)); - builder.register_handler(Arc::new(WaitAgentHandler)); + builder.register_handler(Arc::new(ResumeAgentHandler)); + builder.register_handler(Arc::new(WaitAgentHandler::new(params.wait_agent_timeouts))); builder.register_handler(Arc::new(CloseAgentHandler)); } } if config.agent_jobs_tools { - builder.push_spec( - create_spawn_agents_on_csv_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(SpawnAgentsOnCsvHandler)); if config.agent_jobs_worker_tools { - builder.push_spec( - create_report_agent_job_result_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); builder.register_handler(Arc::new(ReportAgentJobResultHandler)); } } @@ -584,7 +395,6 @@ pub fn build_tool_registry_builder( tools, }), /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, ); } } @@ -609,11 +419,7 @@ pub fn build_tool_registry_builder( for spec in coalesce_loadable_tool_specs(dynamic_tool_specs) { let spec = spec.into(); if config.namespace_tools || !matches!(spec, ToolSpec::Namespace(_)) { - builder.push_spec( - spec, - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); + builder.push_spec(spec, /*supports_parallel_tool_calls*/ false); } } diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index f41060906b..a77c5c2e19 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -1,8 +1,29 @@ use super::*; +use crate::tools::handlers::apply_patch_spec::create_apply_patch_freeform_tool; +use crate::tools::handlers::goal_spec::create_create_goal_tool; +use crate::tools::handlers::goal_spec::create_get_goal_tool; +use crate::tools::handlers::goal_spec::create_update_goal_tool; use crate::tools::handlers::multi_agents_spec::WaitAgentTimeoutOptions; +use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v1; +use crate::tools::handlers::multi_agents_spec::create_close_agent_tool_v2; +use crate::tools::handlers::multi_agents_spec::create_resume_agent_tool; +use crate::tools::handlers::multi_agents_spec::create_send_input_tool_v1; +use crate::tools::handlers::multi_agents_spec::create_send_message_tool; +use crate::tools::handlers::multi_agents_spec::create_spawn_agent_tool_v1; +use crate::tools::handlers::multi_agents_spec::create_spawn_agent_tool_v2; +use crate::tools::handlers::multi_agents_spec::create_wait_agent_tool_v1; +use crate::tools::handlers::multi_agents_spec::create_wait_agent_tool_v2; +use crate::tools::handlers::plan_spec::create_update_plan_tool; use crate::tools::handlers::request_user_input_spec::REQUEST_USER_INPUT_TOOL_NAME; +use crate::tools::handlers::request_user_input_spec::create_request_user_input_tool; +use crate::tools::handlers::request_user_input_spec::request_user_input_tool_description; use crate::tools::handlers::shell_spec::CommandToolOptions; use crate::tools::handlers::shell_spec::create_exec_command_tool; +use crate::tools::handlers::shell_spec::create_request_permissions_tool; +use crate::tools::handlers::shell_spec::create_write_stdin_tool; +use crate::tools::handlers::shell_spec::request_permissions_tool_description; +use crate::tools::handlers::view_image_spec::ViewImageToolOptions; +use crate::tools::handlers::view_image_spec::create_view_image_tool; use crate::tools::registry::ToolRegistry; use crate::tools::spec_plan_types::ToolNamespace; use crate::tools::spec_plan_types::ToolRegistryBuildDeferredTool; @@ -2423,9 +2444,9 @@ fn request_user_input_tool_spec(available_modes: &[ModeKind]) -> ToolSpec { create_request_user_input_tool(request_user_input_tool_description(available_modes)) } -fn spawn_agent_tool_options(config: &ToolsConfig) -> SpawnAgentToolOptions<'_> { +fn spawn_agent_tool_options(config: &ToolsConfig) -> SpawnAgentToolOptions { SpawnAgentToolOptions { - available_models: &config.available_models, + available_models: config.available_models.clone(), agent_type_description: agent_type_description(config, DEFAULT_AGENT_TYPE_DESCRIPTION), hide_agent_type_model_reasoning: config.hide_spawn_agent_metadata, include_usage_hint: config.spawn_agent_usage_hint, From 0dc1885a5caf608209ee8293076218d7a2dad55b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 7 May 2026 11:07:18 -0700 Subject: [PATCH 06/10] Upgrade `cargo-shear` to 1.11.2 (#21547) ## Summary Catches a few additional dependencies (`sha2`, `url`) that should be in `dev-dependencies`. --- .github/workflows/rust-ci-full.yml | 2 +- .github/workflows/rust-ci.yml | 2 +- codex-rs/app-server/Cargo.toml | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index 7e4d3a8949..12e68b2487 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -36,7 +36,7 @@ jobs: - uses: taiki-e/install-action@44c6d64aa62cd779e873306675c7a58e86d6d532 # v2 with: tool: cargo-shear - version: 1.5.1 + version: 1.11.2 - name: cargo shear run: cargo shear diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index 42a3ca8764..6b221af572 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -82,7 +82,7 @@ jobs: - uses: taiki-e/install-action@44c6d64aa62cd779e873306675c7a58e86d6d532 # v2 with: tool: cargo-shear - version: 1.5.1 + version: 1.11.2 - name: cargo shear run: cargo shear diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index c9031fd0ac..55481b6d5d 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -71,7 +71,6 @@ clap = { workspace = true, features = ["derive"] } futures = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } -sha2 = { workspace = true } tempfile = { workspace = true } thiserror = { workspace = true } time = { workspace = true } @@ -87,20 +86,19 @@ tokio = { workspace = true, features = [ tokio-util = { workspace = true } tracing = { workspace = true, features = ["log"] } tracing-subscriber = { workspace = true, features = ["env-filter", "fmt", "json"] } -url = { workspace = true } uuid = { workspace = true, features = ["serde", "v7"] } [dev-dependencies] app_test_support = { workspace = true } -base64 = { workspace = true } axum = { workspace = true, default-features = false, features = [ "http1", "json", "tokio", ] } -core_test_support = { workspace = true } +base64 = { workspace = true } codex-model-provider-info = { workspace = true } codex-utils-cargo-bin = { workspace = true } +core_test_support = { workspace = true } flate2 = { workspace = true } hmac = { workspace = true } opentelemetry = { workspace = true } @@ -113,8 +111,10 @@ rmcp = { workspace = true, default-features = false, features = [ "transport-streamable-http-server", ] } serial_test = { workspace = true } +sha2 = { workspace = true } +shlex = { workspace = true } tar = { workspace = true } tokio-tungstenite = { workspace = true } tracing-opentelemetry = { workspace = true } +url = { workspace = true } wiremock = { workspace = true } -shlex = { workspace = true } From 56823ec46bd1bf8d73646d21b256ca83455ab994 Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 11:12:22 -0700 Subject: [PATCH 07/10] Move thread name edits to ThreadStore (#21264) - Route live thread renames through `ThreadStore` metadata updates. - Read resumed thread names from store metadata with legacy local fallback preserved in the store. --- codex-rs/app-server/src/request_processors.rs | 1 - .../request_processors/thread_processor.rs | 46 +++++-------------- codex-rs/core/src/session/mod.rs | 39 ++++++++++------ codex-rs/core/src/session/session.rs | 2 +- 4 files changed, 37 insertions(+), 51 deletions(-) diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index dc80494b67..9bbfdf4f2e 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -274,7 +274,6 @@ use codex_core::exec::ExecCapturePolicy; use codex_core::exec::ExecExpiration; use codex_core::exec::ExecParams; use codex_core::exec_env::create_env; -use codex_core::find_thread_name_by_id; use codex_core::find_thread_path_by_id_str; use codex_core::path_utils; #[cfg(test)] diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index deeb2e9d62..cb1b9d31b8 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -2914,10 +2914,19 @@ impl ThreadRequestProcessor { } async fn attach_thread_name(&self, thread_id: ThreadId, thread: &mut Thread) { - if let Some(title) = - title_from_state_db(&self.config, self.state_db.as_ref(), thread_id).await + if let Ok(stored_thread) = self + .thread_store + .read_thread(StoreReadThreadParams { + thread_id, + include_archived: true, + include_history: false, + }) + .await + && let Some(title) = stored_thread.name.as_deref().map(str::trim) + && !title.is_empty() + && stored_thread.preview.trim() != title { - set_thread_name_from_title(thread, title); + set_thread_name_from_title(thread, title.to_string()); } } @@ -3625,37 +3634,6 @@ fn thread_store_archive_error(operation: &str, err: ThreadStoreError) -> JSONRPC } } -async fn title_from_state_db( - config: &Config, - state_db_ctx: Option<&StateDbHandle>, - thread_id: ThreadId, -) -> Option { - if let Some(state_db_ctx) = state_db_ctx - && let Some(metadata) = state_db_ctx.get_thread(thread_id).await.ok().flatten() - && let Some(title) = distinct_title(&metadata) - { - return Some(title); - } - find_thread_name_by_id(&config.codex_home, &thread_id) - .await - .ok() - .flatten() -} - -fn non_empty_title(metadata: &ThreadMetadata) -> Option { - let title = metadata.title.trim(); - (!title.is_empty()).then(|| title.to_string()) -} - -fn distinct_title(metadata: &ThreadMetadata) -> Option { - let title = non_empty_title(metadata)?; - if metadata.first_user_message.as_deref().map(str::trim) == Some(title.as_str()) { - None - } else { - Some(title) - } -} - fn set_thread_name_from_title(thread: &mut Thread, title: String) { if title.trim().is_empty() || thread.preview.trim() == title.trim() { return; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 4c93315c14..6d4b275421 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -35,7 +35,6 @@ use crate::exec_policy::ExecPolicyManager; use crate::parse_turn_item; use crate::path_utils::normalize_for_native_workdir; use crate::realtime_conversation::RealtimeConversationManager; -use crate::rollout::find_thread_name_by_id; use crate::session_prefix::format_subagent_notification_message; use crate::skills::SkillRenderSideEffects; use crate::skills_load_input_from_config; @@ -133,6 +132,7 @@ use codex_thread_store::CreateThreadParams; use codex_thread_store::LiveThread; use codex_thread_store::LiveThreadInitGuard; use codex_thread_store::LocalThreadStore; +use codex_thread_store::ReadThreadParams; use codex_thread_store::ResumeThreadParams; use codex_thread_store::ThreadEventPersistenceMode; use codex_thread_store::ThreadPersistenceMetadata; @@ -829,24 +829,33 @@ pub(crate) fn session_loop_termination_from_handle( .shared() } -async fn thread_title_from_state_db( - state_db: Option<&state_db::StateDbHandle>, - codex_home: &AbsolutePathBuf, +async fn thread_title_from_thread_store( + live_thread: Option<&LiveThread>, + thread_store: &Arc, conversation_id: ThreadId, ) -> Option { - if let Some(metadata) = state_db - && let Some(metadata) = metadata.get_thread(conversation_id).await.ok().flatten() - { - let title = metadata.title.trim(); - if !title.is_empty() && metadata.first_user_message.as_deref().map(str::trim) != Some(title) - { - return Some(title.to_string()); + let thread = match live_thread { + Some(live_thread) => { + live_thread + .read_thread( + /*include_archived*/ true, /*include_history*/ false, + ) + .await + } + None => { + thread_store + .read_thread(ReadThreadParams { + thread_id: conversation_id, + include_archived: true, + include_history: false, + }) + .await } } - find_thread_name_by_id(codex_home, &conversation_id) - .await - .ok() - .flatten() + .ok()?; + + let title = thread.name.as_deref()?.trim(); + (!title.is_empty() && thread.preview.trim() != title).then(|| title.to_string()) } impl Session { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 11fa4109bf..f72a173c80 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -717,7 +717,7 @@ impl Session { tx }; let thread_name = - thread_title_from_state_db(state_db_ctx.as_ref(), &config.codex_home, thread_id) + thread_title_from_thread_store(live_thread_init.as_ref(), &thread_store, thread_id) .instrument(info_span!( "session_init.thread_name_lookup", otel.name = "session_init.thread_name_lookup", From 02743989018a68d1fbeb7a3f51a3670a7b8dcfdc Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 11:18:16 -0700 Subject: [PATCH 08/10] [codex] Fix pathless thread summaries (#21266) ## Summary Fix `getConversationSummary` so thread-id summaries work for stored threads that do not have a local rollout path, such as remote thread stores. The root cause was that `summary_from_stored_thread` returned `None` when `StoredThread.rollout_path` was absent, and `get_thread_summary_response_inner` treated that as an internal error. This made conversation-id lookups depend on a local-only field even though the thread store can address the thread by id. --- codex-rs/app-server/src/request_processors.rs | 3 +- .../request_processors/thread_processor.rs | 22 +-- .../thread_processor_tests.rs | 3 +- .../src/request_processors/thread_summary.rs | 10 +- .../tests/suite/conversation_summary.rs | 142 +++++++++++++++- .../tests/suite/v2/thread_unarchive.rs | 153 ++++++++++++++++++ codex-rs/docs/codex_mcp_interface.md | 2 +- 7 files changed, 313 insertions(+), 22 deletions(-) diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index 9bbfdf4f2e..795b5d4c32 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -13,10 +13,8 @@ use crate::outgoing_message::RequestContext; use crate::outgoing_message::ThreadScopedOutgoingMessageSender; use crate::thread_status::ThreadWatchManager; use crate::thread_status::resolve_thread_status; -use chrono::DateTime; use chrono::Duration as ChronoDuration; use chrono::SecondsFormat; -use chrono::Utc; use codex_analytics::AnalyticsEventsClient; use codex_analytics::AnalyticsJsonRpcError; use codex_analytics::InputError; @@ -495,6 +493,7 @@ pub(crate) use self::thread_lifecycle::populate_thread_turns_from_history; pub(crate) use self::thread_processor::thread_from_stored_thread; #[cfg(test)] pub(crate) use self::thread_summary::read_summary_from_rollout; +#[cfg(test)] pub(crate) use self::thread_summary::summary_to_thread; pub(crate) fn build_api_turns_from_rollout_items(items: &[RolloutItem]) -> Vec { diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index cb1b9d31b8..d83f5e631f 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -1605,11 +1605,8 @@ impl ThreadRequestProcessor { .unarchive_thread(StoreArchiveThreadParams { thread_id }) .await .map_err(|err| thread_store_archive_error("unarchive", err))?; - let summary = summary_from_stored_thread(stored_thread, fallback_provider.as_str()) - .ok_or_else(|| { - internal_error(format!("failed to read unarchived thread {thread_id}")) - })?; - let mut thread = summary_to_thread(summary, &self.config.cwd); + let (mut thread, _) = + thread_from_stored_thread(stored_thread, fallback_provider.as_str(), &self.config.cwd); thread.status = resolve_thread_status( self.thread_watch_manager @@ -3215,12 +3212,7 @@ impl ThreadRequestProcessor { }; let stored_thread = read_result?; - let summary = - summary_from_stored_thread(stored_thread, fallback_provider).ok_or_else(|| { - internal_error( - "failed to load conversation summary: thread is missing rollout path", - ) - })?; + let summary = summary_from_stored_thread(stored_thread, fallback_provider); Ok(GetConversationSummaryResponse { summary }) } @@ -3697,8 +3689,8 @@ pub(crate) fn thread_from_stored_thread( fn summary_from_stored_thread( thread: StoredThread, fallback_provider: &str, -) -> Option { - let path = thread.rollout_path?; +) -> ConversationSummary { + let path = thread.rollout_path.unwrap_or_default(); let source = with_thread_spawn_agent_metadata( thread.source, thread.agent_nickname.clone(), @@ -3709,7 +3701,7 @@ fn summary_from_stored_thread( branch: git.branch, origin_url: git.repository_url, }); - Some(ConversationSummary { + ConversationSummary { conversation_id: thread.thread_id, path, preview: thread.first_user_message.unwrap_or(thread.preview), @@ -3734,7 +3726,7 @@ fn summary_from_stored_thread( cli_version: thread.cli_version, source, git_info, - }) + } } #[allow(clippy::too_many_arguments)] diff --git a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs index 5a18c09db9..5642dbbe81 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs @@ -409,8 +409,7 @@ mod thread_processor_behavior_tests { history: None, }; - let summary = - summary_from_stored_thread(stored_thread, "fallback").expect("summary should exist"); + let summary = summary_from_stored_thread(stored_thread, "fallback"); assert_eq!( summary.timestamp.as_deref(), diff --git a/codex-rs/app-server/src/request_processors/thread_summary.rs b/codex-rs/app-server/src/request_processors/thread_summary.rs index f2de590a9a..875bd3deaf 100644 --- a/codex-rs/app-server/src/request_processors/thread_summary.rs +++ b/codex-rs/app-server/src/request_processors/thread_summary.rs @@ -1,5 +1,10 @@ use super::*; +#[cfg(test)] +use chrono::DateTime; +#[cfg(test)] +use chrono::Utc; + #[cfg(test)] pub(crate) async fn read_summary_from_rollout( path: &Path, @@ -203,6 +208,7 @@ pub(super) fn thread_response_sandbox_policy( sandbox_policy.into() } +#[cfg(test)] fn parse_datetime(timestamp: Option<&str>) -> Option> { timestamp.and_then(|ts| { chrono::DateTime::parse_from_rfc3339(ts) @@ -229,6 +235,7 @@ pub(super) fn thread_started_notification(mut thread: Thread) -> ThreadStartedNo ThreadStartedNotification { thread } } +#[cfg(test)] pub(crate) fn summary_to_thread( summary: ConversationSummary, fallback_cwd: &AbsolutePathBuf, @@ -257,6 +264,7 @@ pub(crate) fn summary_to_thread( AbsolutePathBuf::relative_to_current_dir(path_utils::normalize_for_native_workdir(cwd)) .unwrap_or_else(|err| { warn!( + conversation_id = %conversation_id, path = %path.display(), "failed to normalize thread cwd while summarizing thread: {err}" ); @@ -274,7 +282,7 @@ pub(crate) fn summary_to_thread( created_at: created_at.map(|dt| dt.timestamp()).unwrap_or(0), updated_at: updated_at.map(|dt| dt.timestamp()).unwrap_or(0), status: ThreadStatus::NotLoaded, - path: Some(path), + path: (!path.as_os_str().is_empty()).then_some(path), cwd, cli_version, agent_nickname: source.get_nickname(), diff --git a/codex-rs/app-server/tests/suite/conversation_summary.rs b/codex-rs/app-server/tests/suite/conversation_summary.rs index bb938d9ae7..c06ff8599a 100644 --- a/codex-rs/app-server/tests/suite/conversation_summary.rs +++ b/codex-rs/app-server/tests/suite/conversation_summary.rs @@ -3,20 +3,41 @@ use app_test_support::McpProcess; use app_test_support::create_fake_rollout; use app_test_support::rollout_path; use app_test_support::to_response; +use codex_app_server::in_process; +use codex_app_server::in_process::InProcessStartArgs; +use codex_app_server_protocol::ClientInfo; +use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::ConversationSummary; use codex_app_server_protocol::GetConversationSummaryParams; use codex_app_server_protocol::GetConversationSummaryResponse; +use codex_app_server_protocol::InitializeCapabilities; +use codex_app_server_protocol::InitializeParams; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; +use codex_arg0::Arg0DispatchPaths; +use codex_config::CloudRequirementsLoader; +use codex_config::LoaderOverrides; +use codex_core::config::ConfigBuilder; +use codex_exec_server::EnvironmentManager; +use codex_feedback::CodexFeedback; use codex_protocol::ThreadId; +use codex_protocol::models::BaseInstructions; use codex_protocol::protocol::SessionSource; +use codex_protocol::protocol::ThreadMemoryMode; +use codex_thread_store::CreateThreadParams; +use codex_thread_store::InMemoryThreadStore; +use codex_thread_store::ThreadEventPersistenceMode; +use codex_thread_store::ThreadPersistenceMetadata; +use codex_thread_store::ThreadStore; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use tempfile::TempDir; use tokio::time::timeout; +use uuid::Uuid; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); const FILENAME_TS: &str = "2025-01-02T12-00-00"; @@ -47,7 +68,9 @@ fn normalized_canonical_path(path: impl AsRef) -> Result { } fn normalized_summary_path(mut summary: ConversationSummary) -> Result { - summary.path = normalized_canonical_path(&summary.path)?; + if !summary.path.as_os_str().is_empty() { + summary.path = normalized_canonical_path(summary.path)?; + } Ok(summary) } @@ -122,6 +145,87 @@ async fn get_conversation_summary_by_rollout_path_rejects_remote_thread_store() Ok(()) } +#[tokio::test] +async fn get_conversation_summary_by_thread_id_reads_pathless_store_thread() -> Result<()> { + let codex_home = TempDir::new()?; + let store_id = Uuid::new_v4().to_string(); + create_config_toml_with_in_memory_thread_store(codex_home.path(), &store_id)?; + let store = InMemoryThreadStore::for_id(store_id.clone()); + let _in_memory_store = InMemoryThreadStoreId { store_id }; + let thread_id = ThreadId::from_string("00000000-0000-4000-8000-000000000125")?; + store + .create_thread(CreateThreadParams { + thread_id, + forked_from_id: None, + source: SessionSource::Cli, + thread_source: None, + base_instructions: BaseInstructions::default(), + dynamic_tools: Vec::new(), + metadata: ThreadPersistenceMetadata { + cwd: None, + model_provider: "test-provider".to_string(), + memory_mode: ThreadMemoryMode::Disabled, + }, + event_persistence_mode: ThreadEventPersistenceMode::default(), + }) + .await?; + + let loader_overrides = LoaderOverrides::without_managed_config_for_tests(); + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .loader_overrides(loader_overrides.clone()) + .build() + .await?; + let client = in_process::start(InProcessStartArgs { + arg0_paths: Arg0DispatchPaths::default(), + config: Arc::new(config), + cli_overrides: Vec::new(), + loader_overrides, + cloud_requirements: CloudRequirementsLoader::default(), + thread_config_loader: Arc::new(codex_config::NoopThreadConfigLoader), + feedback: CodexFeedback::new(), + log_db: None, + state_db: None, + environment_manager: Arc::new(EnvironmentManager::default_for_tests()), + config_warnings: Vec::new(), + session_source: SessionSource::Cli, + enable_codex_api_key_env: false, + initialize: InitializeParams { + client_info: ClientInfo { + name: "codex-app-server-tests".to_string(), + title: None, + version: "0.1.0".to_string(), + }, + capabilities: Some(InitializeCapabilities { + experimental_api: true, + ..Default::default() + }), + }, + channel_capacity: in_process::DEFAULT_IN_PROCESS_CHANNEL_CAPACITY, + }) + .await?; + + let result = client + .request(ClientRequest::GetConversationSummary { + request_id: RequestId::Integer(1), + params: GetConversationSummaryParams::ThreadId { + conversation_id: thread_id, + }, + }) + .await? + .expect("getConversationSummary should succeed"); + let GetConversationSummaryResponse { summary } = serde_json::from_value(result)?; + + assert_eq!(summary.conversation_id, thread_id); + assert_eq!(summary.path, PathBuf::new()); + assert_eq!(summary.cwd, PathBuf::new()); + assert_eq!(summary.model_provider, "test"); + + client.shutdown().await?; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn get_conversation_summary_by_relative_rollout_path_resolves_from_codex_home() -> Result<()> { @@ -157,3 +261,39 @@ async fn get_conversation_summary_by_relative_rollout_path_resolves_from_codex_h assert_eq!(normalized_summary_path(received.summary)?, expected); Ok(()) } + +struct InMemoryThreadStoreId { + store_id: String, +} + +impl Drop for InMemoryThreadStoreId { + fn drop(&mut self) { + InMemoryThreadStore::remove_id(&self.store_id); + } +} + +fn create_config_toml_with_in_memory_thread_store( + codex_home: &Path, + store_id: &str, +) -> std::io::Result<()> { + std::fs::write( + codex_home.join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" +experimental_thread_store = {{ type = "in_memory", id = "{store_id}" }} + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "http://127.0.0.1:1/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} diff --git a/codex-rs/app-server/tests/suite/v2/thread_unarchive.rs b/codex-rs/app-server/tests/suite/v2/thread_unarchive.rs index 588764edb8..5b421dcec5 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_unarchive.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_unarchive.rs @@ -2,6 +2,12 @@ use anyhow::Result; use app_test_support::McpProcess; use app_test_support::create_mock_responses_server_repeating_assistant; use app_test_support::to_response; +use codex_app_server::in_process; +use codex_app_server::in_process::InProcessStartArgs; +use codex_app_server_protocol::ClientInfo; +use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::InitializeCapabilities; +use codex_app_server_protocol::InitializeParams; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ThreadArchiveParams; @@ -15,17 +21,36 @@ use codex_app_server_protocol::ThreadUnarchivedNotification; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::UserInput; +use codex_arg0::Arg0DispatchPaths; +use codex_config::CloudRequirementsLoader; +use codex_config::LoaderOverrides; +use codex_core::config::ConfigBuilder; use codex_core::find_archived_thread_path_by_id_str; use codex_core::find_thread_path_by_id_str; +use codex_exec_server::EnvironmentManager; +use codex_feedback::CodexFeedback; +use codex_protocol::ThreadId; +use codex_protocol::models::BaseInstructions; +use codex_protocol::protocol::SessionSource; +use codex_protocol::protocol::ThreadMemoryMode; +use codex_thread_store::CreateThreadParams; +use codex_thread_store::InMemoryThreadStore; +use codex_thread_store::ThreadEventPersistenceMode; +use codex_thread_store::ThreadMetadataPatch; +use codex_thread_store::ThreadPersistenceMetadata; +use codex_thread_store::ThreadStore; +use codex_thread_store::UpdateThreadMetadataParams; use pretty_assertions::assert_eq; use serde_json::Value; use std::fs::FileTimes; use std::fs::OpenOptions; use std::path::Path; +use std::sync::Arc; use std::time::Duration; use std::time::SystemTime; use tempfile::TempDir; use tokio::time::timeout; +use uuid::Uuid; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); @@ -172,11 +197,139 @@ async fn thread_unarchive_moves_rollout_back_into_sessions_directory() -> Result Ok(()) } +#[tokio::test] +async fn thread_unarchive_preserves_pathless_store_metadata() -> Result<()> { + let codex_home = TempDir::new()?; + let store_id = Uuid::new_v4().to_string(); + create_config_toml_with_in_memory_thread_store(codex_home.path(), &store_id)?; + let store = InMemoryThreadStore::for_id(store_id.clone()); + let _in_memory_store = InMemoryThreadStoreId { store_id }; + let thread_id = ThreadId::from_string("00000000-0000-4000-8000-000000000126")?; + let parent_thread_id = ThreadId::from_string("00000000-0000-4000-8000-000000000127")?; + store + .create_thread(CreateThreadParams { + thread_id, + forked_from_id: Some(parent_thread_id), + source: SessionSource::Cli, + thread_source: None, + base_instructions: BaseInstructions::default(), + dynamic_tools: Vec::new(), + metadata: ThreadPersistenceMetadata { + cwd: None, + model_provider: "test-provider".to_string(), + memory_mode: ThreadMemoryMode::Disabled, + }, + event_persistence_mode: ThreadEventPersistenceMode::default(), + }) + .await?; + store + .update_thread_metadata(UpdateThreadMetadataParams { + thread_id, + patch: ThreadMetadataPatch { + name: Some("named pathless thread".to_string()), + ..Default::default() + }, + include_archived: true, + }) + .await?; + + let loader_overrides = LoaderOverrides::without_managed_config_for_tests(); + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .fallback_cwd(Some(codex_home.path().to_path_buf())) + .loader_overrides(loader_overrides.clone()) + .build() + .await?; + let client = in_process::start(InProcessStartArgs { + arg0_paths: Arg0DispatchPaths::default(), + config: Arc::new(config), + cli_overrides: Vec::new(), + loader_overrides, + cloud_requirements: CloudRequirementsLoader::default(), + thread_config_loader: Arc::new(codex_config::NoopThreadConfigLoader), + feedback: CodexFeedback::new(), + log_db: None, + state_db: None, + environment_manager: Arc::new(EnvironmentManager::default_for_tests()), + config_warnings: Vec::new(), + session_source: SessionSource::Cli, + enable_codex_api_key_env: false, + initialize: InitializeParams { + client_info: ClientInfo { + name: "codex-app-server-tests".to_string(), + title: None, + version: "0.1.0".to_string(), + }, + capabilities: Some(InitializeCapabilities { + experimental_api: true, + ..Default::default() + }), + }, + channel_capacity: in_process::DEFAULT_IN_PROCESS_CHANNEL_CAPACITY, + }) + .await?; + + let result = client + .request(ClientRequest::ThreadUnarchive { + request_id: RequestId::Integer(1), + params: ThreadUnarchiveParams { + thread_id: thread_id.to_string(), + }, + }) + .await? + .expect("thread/unarchive should succeed"); + let ThreadUnarchiveResponse { thread } = serde_json::from_value(result)?; + + assert_eq!(thread.id, thread_id.to_string()); + assert_eq!(thread.path, None); + assert_eq!(thread.forked_from_id, Some(parent_thread_id.to_string())); + assert_eq!(thread.name, Some("named pathless thread".to_string())); + + client.shutdown().await?; + Ok(()) +} + fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { let config_toml = codex_home.join("config.toml"); std::fs::write(config_toml, config_contents(server_uri)) } +struct InMemoryThreadStoreId { + store_id: String, +} + +impl Drop for InMemoryThreadStoreId { + fn drop(&mut self) { + InMemoryThreadStore::remove_id(&self.store_id); + } +} + +fn create_config_toml_with_in_memory_thread_store( + codex_home: &Path, + store_id: &str, +) -> std::io::Result<()> { + std::fs::write( + codex_home.join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" +experimental_thread_store = {{ type = "in_memory", id = "{store_id}" }} + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "http://127.0.0.1:1/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} + fn config_contents(server_uri: &str) -> String { format!( r#"model = "mock-model" diff --git a/codex-rs/docs/codex_mcp_interface.md b/codex-rs/docs/codex_mcp_interface.md index 7e3d4c6843..bf2def2407 100644 --- a/codex-rs/docs/codex_mcp_interface.md +++ b/codex-rs/docs/codex_mcp_interface.md @@ -52,7 +52,7 @@ Use the separate `codex mcp` subcommand to manage configured MCP server launcher Use the v2 thread and turn APIs for all new integrations. `thread/start` creates a thread, `turn/start` submits user input, `turn/interrupt` stops an in-flight turn, and `thread/list` / `thread/read` expose persisted history. -`getConversationSummary` remains as a compatibility helper for clients that still need a summary lookup by `conversationId` or `rolloutPath`. +`getConversationSummary` remains as a compatibility helper for clients that still need a summary lookup by `conversationId` or `rolloutPath`. Lookups by `conversationId` are preferred; lookups by `rolloutPath` won't work with non-local thread stores. For complete request and response shapes, see the app-server README and the protocol definitions in `app-server-protocol/src/protocol/v2.rs`. From 4242bba2ebc618253bb43032309daa42cfa39ac3 Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 7 May 2026 11:25:25 -0700 Subject: [PATCH 09/10] Route ThreadManager rollout path reads through thread store (#21265) - Route ThreadManager rollout-path resume/fork through ThreadStore history reads. - Add in-memory store coverage proving path-addressed reads are used. This isn't strictly necessary for the ThreadStore migration, since these ThreadManager methods _only_ work for path-based lookups, but I'm trying to migrate all the rollout recorder callsites to use the threadstore were possible for consistency. --- codex-rs/core/src/thread_manager.rs | 52 ++++++++++- codex-rs/core/src/thread_manager_tests.rs | 106 ++++++++++++++++++++++ codex-rs/thread-store/src/in_memory.rs | 8 +- 3 files changed, 161 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index feda91ce2f..a19832717d 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -7,7 +7,6 @@ use crate::environment_selection::default_thread_environment_selections; use crate::environment_selection::resolve_environment_selections; use crate::file_watcher::FileWatcher; use crate::mcp::McpManager; -use crate::rollout::RolloutRecorder; use crate::rollout::truncation; use crate::session::Codex; use crate::session::CodexSpawnArgs; @@ -41,6 +40,7 @@ use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::InitialHistory; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ResumedHistory; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::SessionConfiguredEvent; use codex_protocol::protocol::SessionSource; @@ -55,6 +55,7 @@ use codex_state::DirectionalThreadSpawnEdgeStatus; use codex_thread_store::InMemoryThreadStore; use codex_thread_store::LocalThreadStore; use codex_thread_store::LocalThreadStoreConfig; +use codex_thread_store::ReadThreadByRolloutPathParams; use codex_thread_store::ReadThreadParams; use codex_thread_store::RemoteThreadStore; use codex_thread_store::StoredThread; @@ -615,7 +616,7 @@ impl ThreadManager { auth_manager: Arc, parent_trace: Option, ) -> CodexResult { - let initial_history = RolloutRecorder::get_rollout_history(&rollout_path).await?; + let initial_history = self.initial_history_from_rollout_path(rollout_path).await?; Box::pin(self.resume_thread_with_history( config, initial_history, @@ -687,7 +688,7 @@ impl ThreadManager { auth_manager: Arc, user_shell_override: crate::shell::Shell, ) -> CodexResult { - let initial_history = RolloutRecorder::get_rollout_history(&rollout_path).await?; + let initial_history = self.initial_history_from_rollout_path(rollout_path).await?; let environments = default_thread_environment_selections( self.state.environment_manager.as_ref(), &config.cwd, @@ -784,7 +785,7 @@ impl ThreadManager { S: Into, { let snapshot = snapshot.into(); - let history = RolloutRecorder::get_rollout_history(&path).await?; + let history = self.initial_history_from_rollout_path(path).await?; self.fork_thread_from_history( snapshot, config, @@ -796,6 +797,24 @@ impl ThreadManager { .await } + async fn initial_history_from_rollout_path( + &self, + rollout_path: PathBuf, + ) -> CodexResult { + let requested_rollout_path = rollout_path.clone(); + let stored_thread = self + .state + .thread_store + .read_thread_by_rollout_path(ReadThreadByRolloutPathParams { + rollout_path, + include_archived: true, + include_history: true, + }) + .await + .map_err(thread_store_rollout_read_error)?; + stored_thread_to_initial_history(stored_thread, Some(requested_rollout_path)) + } + /// Fork an existing thread from already-loaded store history. pub async fn fork_thread_from_history( &self, @@ -1280,6 +1299,31 @@ impl ThreadManagerState { } } +fn stored_thread_to_initial_history( + stored_thread: StoredThread, + rollout_path: Option, +) -> CodexResult { + let thread_id = stored_thread.thread_id; + let history = stored_thread.history.ok_or_else(|| { + CodexErr::Fatal(format!( + "thread {thread_id} did not include persisted history" + )) + })?; + Ok(InitialHistory::Resumed(ResumedHistory { + conversation_id: thread_id, + history: history.items, + rollout_path: rollout_path.or(stored_thread.rollout_path), + })) +} + +fn thread_store_rollout_read_error(err: ThreadStoreError) -> CodexErr { + match err { + ThreadStoreError::ThreadNotFound { thread_id } => CodexErr::ThreadNotFound(thread_id), + ThreadStoreError::InvalidRequest { message } => CodexErr::InvalidRequest(message), + err => CodexErr::Fatal(format!("failed to read thread by rollout path: {err}")), + } +} + /// Return a fork snapshot cut strictly before the nth user message (0-based). /// /// Out-of-range values keep the full committed history at a turn boundary, but diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 17ac81a93b..0834c18e21 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -16,6 +16,7 @@ use codex_protocol::openai_models::ModelsResponse; use codex_protocol::protocol::AgentMessageEvent; use codex_protocol::protocol::InitialHistory; use codex_protocol::protocol::InternalSessionSource; +use codex_protocol::protocol::ResumedHistory; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::ThreadSource; use codex_protocol::protocol::TurnStartedEvent; @@ -730,6 +731,111 @@ async fn resume_stopped_thread_from_rollout_preserves_thread_source() { .expect("shutdown resumed thread"); } +#[tokio::test] +async fn rollout_path_resume_and_fork_read_history_through_thread_store() { + let temp_dir = tempdir().expect("tempdir"); + let mut config = test_config().await; + config.codex_home = temp_dir.path().join("codex-home").abs(); + config.cwd = config.codex_home.abs(); + config.experimental_thread_store = ThreadStoreConfig::InMemory { + id: format!("thread-manager-{}", uuid::Uuid::new_v4()), + }; + std::fs::create_dir_all(&config.codex_home).expect("create codex home"); + + let auth_manager = + AuthManager::from_auth_for_testing(CodexAuth::create_dummy_chatgpt_auth_for_testing()); + let state_db = init_state_db(&config).await; + let thread_store = thread_store_from_config(&config, state_db.clone()); + let in_memory_store = thread_store + .as_any() + .downcast_ref::() + .expect("configured in-memory store"); + let manager = ThreadManager::new( + &config, + auth_manager.clone(), + SessionSource::Exec, + Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), + /*analytics_events_client*/ None, + thread_store.clone(), + state_db, + TEST_INSTALLATION_ID.to_string(), + ); + + let source = manager + .start_thread(config.clone()) + .await + .expect("start source thread"); + source + .thread + .shutdown_and_wait() + .await + .expect("shutdown source thread"); + let _ = manager.remove_thread(&source.thread_id).await; + + let rollout_path = config + .codex_home + .join("rollouts/source.jsonl") + .to_path_buf(); + let resumed = manager + .resume_thread_with_history( + config.clone(), + InitialHistory::Resumed(ResumedHistory { + conversation_id: source.thread_id, + history: vec![RolloutItem::ResponseItem(user_msg("hello"))], + rollout_path: Some(rollout_path.clone()), + }), + auth_manager.clone(), + /*persist_extended_history*/ false, + /*parent_trace*/ None, + ) + .await + .expect("seed rollout path in store"); + resumed + .thread + .shutdown_and_wait() + .await + .expect("shutdown seeded resumed thread"); + let _ = manager.remove_thread(&resumed.thread_id).await; + + let resumed_from_path = manager + .resume_thread_from_rollout( + config.clone(), + rollout_path.clone(), + auth_manager, + /*parent_trace*/ None, + ) + .await + .expect("resume from rollout path"); + assert_eq!(resumed_from_path.thread_id, resumed.thread_id); + + let forked = manager + .fork_thread( + ForkSnapshot::Interrupted, + config, + rollout_path, + /*thread_source*/ None, + /*persist_extended_history*/ false, + /*parent_trace*/ None, + ) + .await + .expect("fork from rollout path"); + assert_ne!(forked.thread_id, resumed.thread_id); + + let calls = in_memory_store.calls().await; + assert_eq!(calls.read_thread_by_rollout_path, 2); + + resumed_from_path + .thread + .shutdown_and_wait() + .await + .expect("shutdown path-resumed thread"); + forked + .thread + .shutdown_and_wait() + .await + .expect("shutdown forked thread"); +} + #[tokio::test] async fn new_uses_active_provider_for_model_refresh() { let server = MockServer::start().await; diff --git a/codex-rs/thread-store/src/in_memory.rs b/codex-rs/thread-store/src/in_memory.rs index fca3d21e62..77739260b8 100644 --- a/codex-rs/thread-store/src/in_memory.rs +++ b/codex-rs/thread-store/src/in_memory.rs @@ -256,10 +256,16 @@ fn stored_thread_from_state( items: history_items.clone(), }); let name = state.names.get(&thread_id).cloned().flatten(); + let rollout_path = state + .rollout_paths + .iter() + .find_map(|(path, mapped_thread_id)| { + (*mapped_thread_id == thread_id).then(|| path.clone()) + }); Ok(StoredThread { thread_id, - rollout_path: None, + rollout_path, forked_from_id: created.forked_from_id, preview: String::new(), name, From 163eac9306e86b38c5ab3986eefd5fd3be616b06 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Thu, 7 May 2026 11:38:10 -0700 Subject: [PATCH 10/10] Grant sandbox users access to desktop runtime bin (#21564) ## Why Codex desktop copies bundled Windows binaries out of `WindowsApps` into a LocalAppData runtime cache before launching `codex.exe`. Sandboxed commands can then need to execute helpers from that cache, but the sandbox user group may not have read/execute access to the runtime bin directory. This makes the Windows sandbox refresh path repair that access directly so the packaged desktop runtime remains usable from sandboxed sessions. ## What changed - Added `setup_runtime_bin` to locate `%LOCALAPPDATA%\OpenAI\Codex\bin`, matching the desktop bundled-binaries destination path, with the same `USERPROFILE\AppData\Local` fallback shape. - During refresh setup, check whether `CodexSandboxUsers` already has read/execute access to the runtime bin directory. - If access is missing, grant `CodexSandboxUsers` `OI/CI/RX` inheritance on that directory. - If the runtime bin directory does not exist, no-op cleanly. ## Verification - `cargo build -p codex-windows-sandbox --bin codex-windows-sandbox-setup` - `cargo test -p codex-windows-sandbox --bin codex-windows-sandbox-setup` - Manual Windows ACL exercise against the installed packaged runtime bin: - existing inherited `CodexSandboxUsers:(I)(OI)(CI)(RX)` no-ops without changing SDDL - after disabling inheritance and removing the group ACE, setup adds `CodexSandboxUsers:(OI)(CI)(RX)` - with `LOCALAPPDATA` pointed at a fake location without `OpenAI\Codex\bin`, setup exits successfully and does not create the directory - restored the real runtime bin with inherited ACLs and confirmed the final SDDL matched the baseline exactly --- .../windows-sandbox-rs/src/setup_main_win.rs | 13 ++- .../src/setup_runtime_bin.rs | 92 +++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 codex-rs/windows-sandbox-rs/src/setup_runtime_bin.rs diff --git a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs index ca3fc1e444..5df1e37a07 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -69,6 +69,8 @@ const DENY_ACCESS: i32 = 3; mod read_acl_mutex; mod sandbox_users; +#[path = "setup_runtime_bin.rs"] +mod setup_runtime_bin; use read_acl_mutex::acquire_read_acl_mutex; use read_acl_mutex::read_acl_mutex_exists; use sandbox_users::provision_sandbox_users; @@ -510,8 +512,7 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<()> { let refresh_only = payload.refresh_only; - if refresh_only { - } else { + if !refresh_only { let provision_result = provision_sandbox_users( &payload.codex_home, &payload.offline_username, @@ -647,6 +648,14 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } + if refresh_only { + setup_runtime_bin::ensure_codex_app_runtime_bin_readable( + sandbox_group_psid, + &mut refresh_errors, + log, + )?; + } + let cap_sid_str = caps.workspace; let sandbox_group_sid_str = string_from_sid_bytes(&sandbox_group_sid).map_err(anyhow::Error::msg)?; diff --git a/codex-rs/windows-sandbox-rs/src/setup_runtime_bin.rs b/codex-rs/windows-sandbox-rs/src/setup_runtime_bin.rs new file mode 100644 index 0000000000..be8b0c67e7 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/setup_runtime_bin.rs @@ -0,0 +1,92 @@ +use std::ffi::c_void; +use std::fs::File; +use std::path::PathBuf; + +use anyhow::Result; +use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; +use codex_windows_sandbox::path_mask_allows; +use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE; +use windows_sys::Win32::Security::OBJECT_INHERIT_ACE; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; + +pub(super) fn ensure_codex_app_runtime_bin_readable( + sandbox_group_psid: *mut c_void, + refresh_errors: &mut Vec, + log: &mut File, +) -> Result<()> { + let local_app_data = std::env::var_os("LOCALAPPDATA") + .map(PathBuf::from) + .or_else(|| { + std::env::var_os("USERPROFILE") + .map(PathBuf::from) + .map(|profile| profile.join("AppData").join("Local")) + }); + let Some(local_app_data) = local_app_data else { + return Ok(()); + }; + + // Codex desktop copies bundled Windows binaries out of WindowsApps to this + // fixed LocalAppData cache before launching codex.exe. + let runtime_bin_dir = local_app_data.join("OpenAI").join("Codex").join("bin"); + if !runtime_bin_dir.is_dir() { + return Ok(()); + } + + let read_execute_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; + let has_access = match path_mask_allows( + &runtime_bin_dir, + &[sandbox_group_psid], + read_execute_mask, + /*require_all_bits*/ true, + ) { + Ok(has_access) => has_access, + Err(err) => { + refresh_errors.push(format!( + "runtime bin read/execute mask check failed on {} for sandbox_group: {err}", + runtime_bin_dir.display() + )); + super::log_line( + log, + &format!( + "runtime bin read/execute mask check failed on {} for sandbox_group: {err}; continuing", + runtime_bin_dir.display() + ), + )?; + false + } + }; + if has_access { + return Ok(()); + } + + super::log_line( + log, + &format!( + "granting read/execute ACE to {} for sandbox users", + runtime_bin_dir.display() + ), + )?; + let result = unsafe { + ensure_allow_mask_aces_with_inheritance( + &runtime_bin_dir, + &[sandbox_group_psid], + read_execute_mask, + OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, + ) + }; + if let Err(err) = result { + refresh_errors.push(format!( + "grant read/execute ACE failed on {} for sandbox_group: {err}", + runtime_bin_dir.display() + )); + super::log_line( + log, + &format!( + "grant read/execute ACE failed on {} for sandbox_group: {err}", + runtime_bin_dir.display() + ), + )?; + } + Ok(()) +}