From 2238c16a914c9b7337b2bd87bc65972f8daf06d1 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 28 Mar 2026 15:32:35 -0700 Subject: [PATCH] codex-tools: extract code mode tool spec adapters (#16132) ## Why The longer-term `codex-tools` migration is to move pure tool-definition and tool-spec plumbing out of `codex-core` while leaving session- and runtime-coupled orchestration behind. The remaining code-mode adapter layer in `core/src/tools/code_mode_description.rs` was a good next extraction seam because it only transformed `ToolSpec` values for code mode and already delegated the low-level description rendering to `codex-code-mode`. ## What Changed - added `codex-rs/tools/src/code_mode.rs` with `augment_tool_spec_for_code_mode()` and `tool_spec_to_code_mode_tool_definition()` - added focused unit coverage in `codex-rs/tools/src/code_mode_tests.rs` - rewired `core/src/tools/spec.rs` and `core/src/tools/code_mode/mod.rs` to use the extracted adapters from `codex-tools` - removed the old `core/src/tools/code_mode_description.rs` shim and its test file from `codex-core` - added the `codex-code-mode` dependency to `codex-tools`, updated `Cargo.lock`, and refreshed the `codex-tools` README to reflect the expanded boundary ## Test Plan - `cargo test -p codex-tools` - `CARGO_TARGET_DIR=/tmp/codex-core-code-mode-adapters cargo test -p codex-core --lib tools::spec::` - `CARGO_TARGET_DIR=/tmp/codex-core-code-mode-adapters cargo test -p codex-core --lib tools::code_mode::` - `just bazel-lock-update` - `just bazel-lock-check` - `just argument-comment-lint` ## References - #15923 - #15928 - #15944 - #15953 - #16031 - #16047 - #16129 --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/tools/code_mode/mod.rs | 33 +---- .../core/src/tools/code_mode_description.rs | 49 ------- .../src/tools/code_mode_description_tests.rs | 104 --------------- codex-rs/core/src/tools/mod.rs | 1 - codex-rs/core/src/tools/spec.rs | 21 ++- codex-rs/tools/Cargo.toml | 1 + codex-rs/tools/README.md | 3 + codex-rs/tools/src/code_mode.rs | 60 +++++++++ codex-rs/tools/src/code_mode_tests.rs | 123 ++++++++++++++++++ codex-rs/tools/src/lib.rs | 3 + 11 files changed, 202 insertions(+), 197 deletions(-) delete mode 100644 codex-rs/core/src/tools/code_mode_description.rs delete mode 100644 codex-rs/core/src/tools/code_mode_description_tests.rs create mode 100644 codex-rs/tools/src/code_mode.rs create mode 100644 codex-rs/tools/src/code_mode_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3327b8c87..92e0c2592 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2613,6 +2613,7 @@ dependencies = [ name = "codex-tools" version = "0.0.0" dependencies = [ + "codex-code-mode", "codex-protocol", "pretty_assertions", "rmcp", diff --git a/codex-rs/core/src/tools/code_mode/mod.rs b/codex-rs/core/src/tools/code_mode/mod.rs index a1f21a622..c8f16ca60 100644 --- a/codex-rs/core/src/tools/code_mode/mod.rs +++ b/codex-rs/core/src/tools/code_mode/mod.rs @@ -19,7 +19,6 @@ use crate::codex::Session; use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; use crate::tools::ToolRouter; -use crate::tools::code_mode_description::augment_tool_spec_for_code_mode; use crate::tools::context::FunctionToolOutput; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::context::ToolPayload; @@ -29,6 +28,7 @@ use crate::tools::router::ToolCallSource; use crate::tools::router::ToolRouterParams; use crate::unified_exec::resolve_max_tokens; use codex_features::Feature; +use codex_tools::tool_spec_to_code_mode_tool_definition; use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::formatted_truncate_text_content_items_with_policy; use codex_utils_output_truncation::truncate_function_output_items_with_policy; @@ -247,42 +247,13 @@ pub(super) async fn build_enabled_tools( let mut out = router .specs() .into_iter() - .map(|spec| augment_tool_spec_for_code_mode(spec, /*code_mode_enabled*/ true)) - .filter_map(enabled_tool_from_spec) + .filter_map(|spec| tool_spec_to_code_mode_tool_definition(&spec)) .collect::>(); out.sort_by(|left, right| left.name.cmp(&right.name)); out.dedup_by(|left, right| left.name == right.name); out } -fn enabled_tool_from_spec(spec: ToolSpec) -> Option { - let tool_name = spec.name().to_string(); - if !codex_code_mode::is_code_mode_nested_tool(&tool_name) { - return None; - } - - match spec { - ToolSpec::Function(tool) => Some(codex_code_mode::ToolDefinition { - name: tool_name, - description: tool.description, - kind: codex_code_mode::CodeModeToolKind::Function, - input_schema: serde_json::to_value(&tool.parameters).ok(), - output_schema: tool.output_schema, - }), - ToolSpec::Freeform(tool) => Some(codex_code_mode::ToolDefinition { - name: tool_name, - description: tool.description, - kind: codex_code_mode::CodeModeToolKind::Freeform, - input_schema: None, - output_schema: None, - }), - ToolSpec::LocalShell {} - | ToolSpec::ImageGeneration { .. } - | ToolSpec::ToolSearch { .. } - | ToolSpec::WebSearch { .. } => None, - } -} - async fn build_nested_router(exec: &ExecContext) -> ToolRouter { let nested_tools_config = exec.turn.tools_config.for_code_mode_nested_tools(); let mcp_tools = exec diff --git a/codex-rs/core/src/tools/code_mode_description.rs b/codex-rs/core/src/tools/code_mode_description.rs deleted file mode 100644 index fb4fc1f51..000000000 --- a/codex-rs/core/src/tools/code_mode_description.rs +++ /dev/null @@ -1,49 +0,0 @@ -use crate::client_common::tools::ToolSpec; - -#[allow(unused_imports)] -#[cfg(test)] -pub(crate) use codex_code_mode::append_code_mode_sample; -#[allow(unused_imports)] -#[cfg(test)] -pub(crate) use codex_code_mode::render_json_schema_to_typescript; - -pub(crate) fn augment_tool_spec_for_code_mode(spec: ToolSpec, code_mode_enabled: bool) -> ToolSpec { - if !code_mode_enabled { - return spec; - } - - match spec { - ToolSpec::Function(mut tool) => { - let input_type = serde_json::to_value(&tool.parameters) - .ok() - .map(|schema| codex_code_mode::render_json_schema_to_typescript(&schema)) - .unwrap_or_else(|| "unknown".to_string()); - let output_type = tool - .output_schema - .as_ref() - .map(codex_code_mode::render_json_schema_to_typescript) - .unwrap_or_else(|| "unknown".to_string()); - tool.description = codex_code_mode::append_code_mode_sample( - &tool.description, - &tool.name, - "args", - input_type, - output_type, - ); - ToolSpec::Function(tool) - } - ToolSpec::Freeform(mut tool) => { - if tool.name != codex_code_mode::PUBLIC_TOOL_NAME { - tool.description = codex_code_mode::append_code_mode_sample( - &tool.description, - &tool.name, - "input", - "string".to_string(), - "unknown".to_string(), - ); - } - ToolSpec::Freeform(tool) - } - other => other, - } -} diff --git a/codex-rs/core/src/tools/code_mode_description_tests.rs b/codex-rs/core/src/tools/code_mode_description_tests.rs deleted file mode 100644 index 034a1e3f3..000000000 --- a/codex-rs/core/src/tools/code_mode_description_tests.rs +++ /dev/null @@ -1,104 +0,0 @@ -use super::append_code_mode_sample; -use super::render_json_schema_to_typescript; -use pretty_assertions::assert_eq; -use serde_json::json; - -#[test] -fn render_json_schema_to_typescript_renders_object_properties() { - let schema = json!({ - "type": "object", - "properties": { - "path": {"type": "string"}, - "recursive": {"type": "boolean"} - }, - "required": ["path"], - "additionalProperties": false - }); - - assert_eq!( - render_json_schema_to_typescript(&schema), - "{ path: string; recursive?: boolean; }" - ); -} - -#[test] -fn render_json_schema_to_typescript_renders_anyof_unions() { - let schema = json!({ - "anyOf": [ - {"const": "pending"}, - {"const": "done"}, - {"type": "number"} - ] - }); - - assert_eq!( - render_json_schema_to_typescript(&schema), - "\"pending\" | \"done\" | number" - ); -} - -#[test] -fn render_json_schema_to_typescript_renders_additional_properties() { - let schema = json!({ - "type": "object", - "properties": { - "tags": { - "type": "array", - "items": {"type": "string"} - } - }, - "additionalProperties": {"type": "integer"} - }); - - assert_eq!( - render_json_schema_to_typescript(&schema), - "{ tags?: Array; [key: string]: number; }" - ); -} - -#[test] -fn render_json_schema_to_typescript_sorts_object_properties() { - let schema = json!({ - "type": "object", - "properties": { - "structuredContent": {"type": "string"}, - "_meta": {"type": "string"}, - "isError": {"type": "boolean"}, - "content": {"type": "array", "items": {"type": "string"}} - }, - "required": ["content"] - }); - - assert_eq!( - render_json_schema_to_typescript(&schema), - "{ _meta?: string; content: Array; isError?: boolean; structuredContent?: string; }" - ); -} - -#[test] -fn append_code_mode_sample_uses_global_tools_for_valid_identifiers() { - assert_eq!( - append_code_mode_sample( - "desc", - "mcp__ologs__get_profile", - "args", - "{ foo: string }".to_string(), - "unknown".to_string(), - ), - "desc\n\nexec tool declaration:\n```ts\ndeclare const tools: { mcp__ologs__get_profile(args: { foo: string }): Promise; };\n```" - ); -} - -#[test] -fn append_code_mode_sample_normalizes_invalid_identifiers() { - assert_eq!( - append_code_mode_sample( - "desc", - "mcp__rmcp__echo-tool", - "args", - "{ foo: string }".to_string(), - "unknown".to_string(), - ), - "desc\n\nexec tool declaration:\n```ts\ndeclare const tools: { mcp__rmcp__echo_tool(args: { foo: string }): Promise; };\n```" - ); -} diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 87a01c8e3..57859bba4 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -1,5 +1,4 @@ pub mod code_mode; -pub(crate) mod code_mode_description; pub mod context; pub(crate) mod discoverable; pub mod events; diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index d5b4b2921..e478c55e4 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -8,7 +8,6 @@ use crate::shell::Shell; use crate::shell::ShellType; use crate::tools::code_mode::PUBLIC_TOOL_NAME; use crate::tools::code_mode::WAIT_TOOL_NAME; -use crate::tools::code_mode_description::augment_tool_spec_for_code_mode; use crate::tools::discoverable::DiscoverablePluginInfo; use crate::tools::discoverable::DiscoverableTool; use crate::tools::discoverable::DiscoverableToolAction; @@ -46,8 +45,10 @@ use codex_protocol::protocol::SubAgentSource; use codex_tools::FreeformTool; use codex_tools::FreeformToolFormat; use codex_tools::ResponsesApiTool; +use codex_tools::augment_tool_spec_for_code_mode; use codex_tools::dynamic_tool_to_responses_api_tool; use codex_tools::mcp_tool_to_responses_api_tool; +use codex_tools::tool_spec_to_code_mode_tool_definition; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_template::Template; use serde::Deserialize; @@ -2357,7 +2358,11 @@ fn push_tool_spec( supports_parallel_tool_calls: bool, code_mode_enabled: bool, ) { - let spec = augment_tool_spec_for_code_mode(spec, code_mode_enabled); + let spec = if code_mode_enabled { + augment_tool_spec_for_code_mode(spec) + } else { + spec + }; if supports_parallel_tool_calls { builder.push_spec_with_parallel_support(spec, /*supports_parallel_tool_calls*/ true); } else { @@ -2455,16 +2460,8 @@ pub(crate) fn build_specs_with_discoverable_tools( .build(); let mut enabled_tools = nested_specs .into_iter() - .filter_map(|spec| { - let (name, description) = match augment_tool_spec_for_code_mode( - spec.spec, /*code_mode_enabled*/ true, - ) { - ToolSpec::Function(tool) => (tool.name, tool.description), - ToolSpec::Freeform(tool) => (tool.name, tool.description), - _ => return None, - }; - codex_code_mode::is_code_mode_nested_tool(&name).then_some((name, description)) - }) + .filter_map(|spec| tool_spec_to_code_mode_tool_definition(&spec.spec)) + .map(|tool| (tool.name, tool.description)) .collect::>(); enabled_tools.sort_by(|left, right| left.0.cmp(&right.0)); enabled_tools.dedup_by(|left, right| left.0 == right.0); diff --git a/codex-rs/tools/Cargo.toml b/codex-rs/tools/Cargo.toml index 1dc9b1c2f..9e2d23fb5 100644 --- a/codex-rs/tools/Cargo.toml +++ b/codex-rs/tools/Cargo.toml @@ -8,6 +8,7 @@ version.workspace = true workspace = true [dependencies] +codex-code-mode = { workspace = true } codex-protocol = { workspace = true } rmcp = { workspace = true, default-features = false, features = [ "base64", diff --git a/codex-rs/tools/README.md b/codex-rs/tools/README.md index 66d342cdb..29eeea266 100644 --- a/codex-rs/tools/README.md +++ b/codex-rs/tools/README.md @@ -21,6 +21,7 @@ schema and Responses API tool primitives that no longer need to live in - `ResponsesApiWebSearchUserLocation` - `ResponsesApiNamespace` - `ResponsesApiNamespaceTool` +- code-mode `ToolSpec` adapters - `parse_tool_input_schema()` - `parse_dynamic_tool()` - `parse_mcp_tool()` @@ -30,6 +31,8 @@ schema and Responses API tool primitives that no longer need to live in - `dynamic_tool_to_responses_api_tool()` - `mcp_tool_to_responses_api_tool()` - `mcp_tool_to_deferred_responses_api_tool()` +- `augment_tool_spec_for_code_mode()` +- `tool_spec_to_code_mode_tool_definition()` That extraction is the first step in a longer migration. The goal is not to move all of `core/src/tools` into this crate in one shot. Instead, the plan is diff --git a/codex-rs/tools/src/code_mode.rs b/codex-rs/tools/src/code_mode.rs new file mode 100644 index 000000000..ba6f68ae2 --- /dev/null +++ b/codex-rs/tools/src/code_mode.rs @@ -0,0 +1,60 @@ +use crate::ToolSpec; +use codex_code_mode::CodeModeToolKind; +use codex_code_mode::ToolDefinition as CodeModeToolDefinition; + +/// Augment tool descriptions with code-mode-specific exec samples. +pub fn augment_tool_spec_for_code_mode(spec: ToolSpec) -> ToolSpec { + let Some(description) = code_mode_tool_definition_for_spec(&spec) + .map(codex_code_mode::augment_tool_definition) + .map(|definition| definition.description) + else { + return spec; + }; + + match spec { + ToolSpec::Function(mut tool) => { + tool.description = description; + ToolSpec::Function(tool) + } + ToolSpec::Freeform(mut tool) => { + tool.description = description; + ToolSpec::Freeform(tool) + } + other => other, + } +} + +/// Convert a supported nested tool spec into the code-mode runtime shape, +/// including the code-mode-specific description sample. +pub fn tool_spec_to_code_mode_tool_definition(spec: &ToolSpec) -> Option { + let definition = code_mode_tool_definition_for_spec(spec)?; + codex_code_mode::is_code_mode_nested_tool(&definition.name) + .then(|| codex_code_mode::augment_tool_definition(definition)) +} + +fn code_mode_tool_definition_for_spec(spec: &ToolSpec) -> Option { + match spec { + ToolSpec::Function(tool) => Some(CodeModeToolDefinition { + name: tool.name.clone(), + description: tool.description.clone(), + kind: CodeModeToolKind::Function, + input_schema: serde_json::to_value(&tool.parameters).ok(), + output_schema: tool.output_schema.clone(), + }), + ToolSpec::Freeform(tool) => Some(CodeModeToolDefinition { + name: tool.name.clone(), + description: tool.description.clone(), + kind: CodeModeToolKind::Freeform, + input_schema: None, + output_schema: None, + }), + ToolSpec::LocalShell {} + | ToolSpec::ImageGeneration { .. } + | ToolSpec::ToolSearch { .. } + | ToolSpec::WebSearch { .. } => None, + } +} + +#[cfg(test)] +#[path = "code_mode_tests.rs"] +mod tests; diff --git a/codex-rs/tools/src/code_mode_tests.rs b/codex-rs/tools/src/code_mode_tests.rs new file mode 100644 index 000000000..07e0a3378 --- /dev/null +++ b/codex-rs/tools/src/code_mode_tests.rs @@ -0,0 +1,123 @@ +use super::augment_tool_spec_for_code_mode; +use super::tool_spec_to_code_mode_tool_definition; +use crate::AdditionalProperties; +use crate::FreeformTool; +use crate::FreeformToolFormat; +use crate::JsonSchema; +use crate::ResponsesApiTool; +use crate::ToolSpec; +use pretty_assertions::assert_eq; +use serde_json::json; +use std::collections::BTreeMap; + +#[test] +fn augment_tool_spec_for_code_mode_augments_function_tools() { + assert_eq!( + augment_tool_spec_for_code_mode(ToolSpec::Function(ResponsesApiTool { + name: "lookup_order".to_string(), + description: "Look up an order".to_string(), + strict: false, + defer_loading: Some(true), + parameters: JsonSchema::Object { + properties: BTreeMap::from([( + "order_id".to_string(), + JsonSchema::String { description: None }, + )]), + required: Some(vec!["order_id".to_string()]), + additional_properties: Some(AdditionalProperties::Boolean(false)), + }, + output_schema: Some(json!({ + "type": "object", + "properties": { + "ok": {"type": "boolean"} + }, + "required": ["ok"], + })), + })), + ToolSpec::Function(ResponsesApiTool { + name: "lookup_order".to_string(), + description: "Look up an order\n\nexec tool declaration:\n```ts\ndeclare const tools: { lookup_order(args: { order_id: string; }): Promise<{ ok: boolean; }>; };\n```".to_string(), + strict: false, + defer_loading: Some(true), + parameters: JsonSchema::Object { + properties: BTreeMap::from([( + "order_id".to_string(), + JsonSchema::String { description: None }, + )]), + required: Some(vec!["order_id".to_string()]), + additional_properties: Some(AdditionalProperties::Boolean(false)), + }, + output_schema: Some(json!({ + "type": "object", + "properties": { + "ok": {"type": "boolean"} + }, + "required": ["ok"], + })), + }) + ); +} + +#[test] +fn augment_tool_spec_for_code_mode_preserves_exec_tool_description() { + assert_eq!( + augment_tool_spec_for_code_mode(ToolSpec::Freeform(FreeformTool { + name: codex_code_mode::PUBLIC_TOOL_NAME.to_string(), + description: "Run code".to_string(), + format: FreeformToolFormat { + r#type: "grammar".to_string(), + syntax: "lark".to_string(), + definition: "start: \"exec\"".to_string(), + }, + })), + ToolSpec::Freeform(FreeformTool { + name: codex_code_mode::PUBLIC_TOOL_NAME.to_string(), + description: "Run code".to_string(), + format: FreeformToolFormat { + r#type: "grammar".to_string(), + syntax: "lark".to_string(), + definition: "start: \"exec\"".to_string(), + }, + }) + ); +} + +#[test] +fn tool_spec_to_code_mode_tool_definition_returns_augmented_nested_tools() { + let spec = ToolSpec::Freeform(FreeformTool { + name: "apply_patch".to_string(), + description: "Apply a patch".to_string(), + format: FreeformToolFormat { + r#type: "grammar".to_string(), + syntax: "lark".to_string(), + definition: "start: \"patch\"".to_string(), + }, + }); + + assert_eq!( + tool_spec_to_code_mode_tool_definition(&spec), + Some(codex_code_mode::ToolDefinition { + name: "apply_patch".to_string(), + description: "Apply a patch\n\nexec tool declaration:\n```ts\ndeclare const tools: { apply_patch(input: string): Promise; };\n```".to_string(), + kind: codex_code_mode::CodeModeToolKind::Freeform, + input_schema: None, + output_schema: None, + }) + ); +} + +#[test] +fn tool_spec_to_code_mode_tool_definition_skips_unsupported_variants() { + assert_eq!( + tool_spec_to_code_mode_tool_definition(&ToolSpec::ToolSearch { + execution: "sync".to_string(), + description: "Search".to_string(), + parameters: JsonSchema::Object { + properties: BTreeMap::new(), + required: None, + additional_properties: None, + }, + }), + None + ); +} diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 527424c27..08352704c 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -1,6 +1,7 @@ //! Shared tool definitions and Responses API tool primitives that can live //! outside `codex-core`. +mod code_mode; mod dynamic_tool; mod json_schema; mod mcp_tool; @@ -8,6 +9,8 @@ mod responses_api; mod tool_definition; mod tool_spec; +pub use code_mode::augment_tool_spec_for_code_mode; +pub use code_mode::tool_spec_to_code_mode_tool_definition; pub use dynamic_tool::parse_dynamic_tool; pub use json_schema::AdditionalProperties; pub use json_schema::JsonSchema;