mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
codex-tools: extract configured tool specs (#16129)
## Why This continues the `codex-tools` migration by moving another passive tool-spec layer out of `codex-core`. After `ToolSpec` moved into `codex-tools`, `codex-core` still owned `ConfiguredToolSpec` and `create_tools_json_for_responses_api()`. Both are data-model and serialization helpers rather than runtime orchestration, so keeping them in `core/src/tools/registry.rs` and `core/src/tools/spec.rs` left passive tool-definition code coupled to `codex-core` longer than necessary. ## What changed - moved `ConfiguredToolSpec` into `codex-rs/tools/src/tool_spec.rs` - moved `create_tools_json_for_responses_api()` into `codex-rs/tools/src/tool_spec.rs` - re-exported the new surface from `codex-rs/tools/src/lib.rs`, which remains exports-only - updated `core/src/client.rs`, `core/src/tools/registry.rs`, and `core/src/tools/router.rs` to consume the extracted types and serializer from `codex-tools` - moved the tool-list serialization test into `codex-rs/tools/src/tool_spec_tests.rs` - added focused unit coverage for `ConfiguredToolSpec::name()` - simplified `core/src/tools/spec_tests.rs` to use the extracted `ConfiguredToolSpec::name()` directly and removed the now-redundant local `tool_name()` helper - updated `codex-rs/tools/README.md` so the crate boundary reflects the newly extracted tool-spec wrapper and serialization helper ## Test plan - `cargo test -p codex-tools` - `CARGO_TARGET_DIR=/tmp/codex-core-configured-spec cargo test -p codex-core --lib tools::spec::` - `CARGO_TARGET_DIR=/tmp/codex-core-configured-spec cargo test -p codex-core --lib client::` - `just fix -p codex-tools -p codex-core` - `just argument-comment-lint` ## References - #15923 - #15928 - #15944 - #15953 - #16031 - #16047
This commit is contained in:
committed by
GitHub
Unverified
parent
ae8a3be958
commit
4e27a87ec6
@@ -72,6 +72,7 @@ use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::W3cTraceContext;
|
||||
use codex_tools::create_tools_json_for_responses_api;
|
||||
use eventsource_stream::Event;
|
||||
use eventsource_stream::EventStreamError;
|
||||
use futures::StreamExt;
|
||||
@@ -107,7 +108,6 @@ use crate::response_debug_context::extract_response_debug_context;
|
||||
use crate::response_debug_context::extract_response_debug_context_from_api_error;
|
||||
use crate::response_debug_context::telemetry_api_error_message;
|
||||
use crate::response_debug_context::telemetry_transport_error_message;
|
||||
use crate::tools::spec::create_tools_json_for_responses_api;
|
||||
use crate::util::FeedbackRequestTags;
|
||||
use crate::util::emit_feedback_auth_recovery_tags;
|
||||
use crate::util::emit_feedback_request_tags_with_auth_env;
|
||||
|
||||
@@ -24,6 +24,7 @@ use codex_hooks::HookToolInput;
|
||||
use codex_hooks::HookToolInputLocalShell;
|
||||
use codex_hooks::HookToolKind;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_tools::ConfiguredToolSpec;
|
||||
use codex_utils_readiness::Readiness;
|
||||
use serde_json::Value;
|
||||
use tracing::warn;
|
||||
@@ -436,21 +437,6 @@ impl ToolRegistry {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct ConfiguredToolSpec {
|
||||
pub spec: ToolSpec,
|
||||
pub supports_parallel_tool_calls: bool,
|
||||
}
|
||||
|
||||
impl ConfiguredToolSpec {
|
||||
pub fn new(spec: ToolSpec, supports_parallel_tool_calls: bool) -> Self {
|
||||
Self {
|
||||
spec,
|
||||
supports_parallel_tool_calls,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub struct ToolRegistryBuilder {
|
||||
handlers: HashMap<String, Arc<dyn AnyToolHandler>>,
|
||||
specs: Vec<ConfiguredToolSpec>,
|
||||
|
||||
@@ -9,7 +9,6 @@ use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolPayload;
|
||||
use crate::tools::discoverable::DiscoverableTool;
|
||||
use crate::tools::registry::AnyToolResult;
|
||||
use crate::tools::registry::ConfiguredToolSpec;
|
||||
use crate::tools::registry::ToolRegistry;
|
||||
use crate::tools::spec::ToolsConfig;
|
||||
use crate::tools::spec::build_specs_with_discoverable_tools;
|
||||
@@ -18,6 +17,7 @@ use codex_protocol::models::LocalShellAction;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::models::SearchToolCallParams;
|
||||
use codex_protocol::models::ShellToolCallParams;
|
||||
use codex_tools::ConfiguredToolSpec;
|
||||
use rmcp::model::Tool;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
@@ -66,7 +66,7 @@ impl ToolRouter {
|
||||
specs
|
||||
.iter()
|
||||
.filter_map(|configured_tool| {
|
||||
if !codex_code_mode::is_code_mode_nested_tool(configured_tool.spec.name()) {
|
||||
if !codex_code_mode::is_code_mode_nested_tool(configured_tool.name()) {
|
||||
Some(configured_tool.spec.clone())
|
||||
} else {
|
||||
None
|
||||
@@ -101,7 +101,7 @@ impl ToolRouter {
|
||||
pub fn find_spec(&self, tool_name: &str) -> Option<ToolSpec> {
|
||||
self.specs
|
||||
.iter()
|
||||
.find(|config| config.spec.name() == tool_name)
|
||||
.find(|config| config.name() == tool_name)
|
||||
.map(|config| config.spec.clone())
|
||||
}
|
||||
|
||||
@@ -109,7 +109,7 @@ impl ToolRouter {
|
||||
self.specs
|
||||
.iter()
|
||||
.filter(|config| config.supports_parallel_tool_calls)
|
||||
.any(|config| config.spec.name() == tool_name)
|
||||
.any(|config| config.name() == tool_name)
|
||||
}
|
||||
|
||||
#[instrument(level = "trace", skip_all, err)]
|
||||
|
||||
@@ -2351,22 +2351,6 @@ pub(crate) struct ApplyPatchToolArgs {
|
||||
pub(crate) input: String,
|
||||
}
|
||||
|
||||
/// Returns JSON values that are compatible with Function Calling in the
|
||||
/// Responses API:
|
||||
/// https://platform.openai.com/docs/guides/function-calling?api-mode=responses
|
||||
pub fn create_tools_json_for_responses_api(
|
||||
tools: &[ToolSpec],
|
||||
) -> crate::error::Result<Vec<serde_json::Value>> {
|
||||
let mut tools_json = Vec::new();
|
||||
|
||||
for tool in tools {
|
||||
let json = serde_json::to_value(tool)?;
|
||||
tools_json.push(json);
|
||||
}
|
||||
|
||||
Ok(tools_json)
|
||||
}
|
||||
|
||||
fn push_tool_spec(
|
||||
builder: &mut ToolRegistryBuilder,
|
||||
spec: ToolSpec,
|
||||
|
||||
@@ -4,13 +4,13 @@ use crate::models_manager::model_info::with_config_overrides;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell::ShellType;
|
||||
use crate::tools::ToolRouter;
|
||||
use crate::tools::registry::ConfiguredToolSpec;
|
||||
use crate::tools::router::ToolRouterParams;
|
||||
use codex_app_server_protocol::AppInfo;
|
||||
use codex_protocol::openai_models::InputModality;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_tools::AdditionalProperties;
|
||||
use codex_tools::ConfiguredToolSpec;
|
||||
use codex_tools::FreeformTool;
|
||||
use codex_tools::ResponsesApiWebSearchFilters;
|
||||
use codex_tools::ResponsesApiWebSearchUserLocation;
|
||||
@@ -107,16 +107,12 @@ fn deferred_responses_api_tool_serializes_with_defer_loading() {
|
||||
);
|
||||
}
|
||||
|
||||
fn tool_name(tool: &ToolSpec) -> &str {
|
||||
tool.name()
|
||||
}
|
||||
|
||||
// Avoid order-based assertions; compare via set containment instead.
|
||||
fn assert_contains_tool_names(tools: &[ConfiguredToolSpec], expected_subset: &[&str]) {
|
||||
use std::collections::HashSet;
|
||||
let mut names = HashSet::new();
|
||||
let mut duplicates = Vec::new();
|
||||
for name in tools.iter().map(|t| tool_name(&t.spec)) {
|
||||
for name in tools.iter().map(ConfiguredToolSpec::name) {
|
||||
if !names.insert(name) {
|
||||
duplicates.push(name);
|
||||
}
|
||||
@@ -136,7 +132,7 @@ fn assert_contains_tool_names(tools: &[ConfiguredToolSpec], expected_subset: &[&
|
||||
fn assert_lacks_tool_name(tools: &[ConfiguredToolSpec], expected_absent: &str) {
|
||||
let names = tools
|
||||
.iter()
|
||||
.map(|tool| tool_name(&tool.spec))
|
||||
.map(ConfiguredToolSpec::name)
|
||||
.collect::<Vec<_>>();
|
||||
assert!(
|
||||
!names.contains(&expected_absent),
|
||||
@@ -157,7 +153,7 @@ fn shell_tool_name(config: &ToolsConfig) -> Option<&'static str> {
|
||||
fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a ConfiguredToolSpec {
|
||||
tools
|
||||
.iter()
|
||||
.find(|tool| tool_name(&tool.spec) == expected_name)
|
||||
.find(|tool| tool.name() == expected_name)
|
||||
.unwrap_or_else(|| panic!("expected tool {expected_name}"))
|
||||
}
|
||||
|
||||
@@ -289,7 +285,7 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() {
|
||||
let mut actual: BTreeMap<String, ToolSpec> = BTreeMap::from([]);
|
||||
let mut duplicate_names = Vec::new();
|
||||
for t in &tools {
|
||||
let name = tool_name(&t.spec).to_string();
|
||||
let name = t.name().to_string();
|
||||
if actual.insert(name.clone(), t.spec.clone()).is_some() {
|
||||
duplicate_names.push(name);
|
||||
}
|
||||
@@ -318,7 +314,7 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() {
|
||||
},
|
||||
create_view_image_tool(config.can_request_original_image_detail),
|
||||
] {
|
||||
expected.insert(tool_name(&spec).to_string(), spec);
|
||||
expected.insert(spec.name().to_string(), spec);
|
||||
}
|
||||
let collab_specs = if config.multi_agent_v2 {
|
||||
vec![
|
||||
@@ -336,16 +332,16 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() {
|
||||
]
|
||||
};
|
||||
for spec in collab_specs {
|
||||
expected.insert(tool_name(&spec).to_string(), spec);
|
||||
expected.insert(spec.name().to_string(), spec);
|
||||
}
|
||||
if !config.multi_agent_v2 {
|
||||
let spec = create_resume_agent_tool();
|
||||
expected.insert(tool_name(&spec).to_string(), spec);
|
||||
expected.insert(spec.name().to_string(), spec);
|
||||
}
|
||||
|
||||
if config.exec_permission_approvals_enabled {
|
||||
let spec = create_request_permissions_tool();
|
||||
expected.insert(tool_name(&spec).to_string(), spec);
|
||||
expected.insert(spec.name().to_string(), spec);
|
||||
}
|
||||
|
||||
// Exact name set match — this is the only test allowed to fail when tools change.
|
||||
@@ -1679,11 +1675,7 @@ fn test_test_model_info_includes_sync_tool() {
|
||||
)
|
||||
.build();
|
||||
|
||||
assert!(
|
||||
tools
|
||||
.iter()
|
||||
.any(|tool| tool_name(&tool.spec) == "test_sync_tool")
|
||||
);
|
||||
assert!(tools.iter().any(|tool| tool.name() == "test_sync_tool"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1817,7 +1809,7 @@ fn test_build_specs_mcp_tools_sorted_by_name() {
|
||||
// Only assert that the MCP tools themselves are sorted by fully-qualified name.
|
||||
let mcp_names: Vec<_> = tools
|
||||
.iter()
|
||||
.map(|t| tool_name(&t.spec).to_string())
|
||||
.map(|t| t.name().to_string())
|
||||
.filter(|n| n.starts_with("test_server/"))
|
||||
.collect();
|
||||
let expected = vec![
|
||||
@@ -2064,7 +2056,7 @@ fn tool_suggest_is_not_registered_without_feature_flag() {
|
||||
assert!(
|
||||
!tools
|
||||
.iter()
|
||||
.any(|tool| tool_name(&tool.spec) == TOOL_SUGGEST_TOOL_NAME)
|
||||
.any(|tool| tool.name() == TOOL_SUGGEST_TOOL_NAME)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -2155,7 +2147,7 @@ fn tool_suggest_requires_apps_and_plugins_features() {
|
||||
assert!(
|
||||
!tools
|
||||
.iter()
|
||||
.any(|tool| tool_name(&tool.spec) == TOOL_SUGGEST_TOOL_NAME),
|
||||
.any(|tool| tool.name() == TOOL_SUGGEST_TOOL_NAME),
|
||||
"tool_suggest should be absent when {disabled_feature:?} is disabled"
|
||||
);
|
||||
}
|
||||
@@ -3106,38 +3098,3 @@ fn code_mode_exec_description_omits_nested_tool_details_when_not_code_mode_only(
|
||||
assert!(!description.contains("### `update_plan` (`update_plan`)"));
|
||||
assert!(!description.contains("### `view_image` (`view_image`)"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn chat_tools_include_top_level_name() {
|
||||
let properties =
|
||||
BTreeMap::from([("foo".to_string(), JsonSchema::String { description: None })]);
|
||||
let tools = vec![ToolSpec::Function(ResponsesApiTool {
|
||||
name: "demo".to_string(),
|
||||
description: "A demo tool".to_string(),
|
||||
strict: false,
|
||||
defer_loading: None,
|
||||
parameters: JsonSchema::Object {
|
||||
properties,
|
||||
required: None,
|
||||
additional_properties: None,
|
||||
},
|
||||
output_schema: None,
|
||||
})];
|
||||
|
||||
let responses_json = create_tools_json_for_responses_api(&tools).unwrap();
|
||||
assert_eq!(
|
||||
responses_json,
|
||||
vec![json!({
|
||||
"type": "function",
|
||||
"name": "demo",
|
||||
"description": "A demo tool",
|
||||
"strict": false,
|
||||
"parameters": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"foo": { "type": "string" }
|
||||
},
|
||||
},
|
||||
})]
|
||||
);
|
||||
}
|
||||
|
||||
@@ -12,6 +12,7 @@ schema and Responses API tool primitives that no longer need to live in
|
||||
- `AdditionalProperties`
|
||||
- `ToolDefinition`
|
||||
- `ToolSpec`
|
||||
- `ConfiguredToolSpec`
|
||||
- `ResponsesApiTool`
|
||||
- `FreeformTool`
|
||||
- `FreeformToolFormat`
|
||||
@@ -23,6 +24,7 @@ schema and Responses API tool primitives that no longer need to live in
|
||||
- `parse_tool_input_schema()`
|
||||
- `parse_dynamic_tool()`
|
||||
- `parse_mcp_tool()`
|
||||
- `create_tools_json_for_responses_api()`
|
||||
- `mcp_call_tool_result_output_schema()`
|
||||
- `tool_definition_to_responses_api_tool()`
|
||||
- `dynamic_tool_to_responses_api_tool()`
|
||||
|
||||
@@ -25,6 +25,8 @@ pub use responses_api::mcp_tool_to_deferred_responses_api_tool;
|
||||
pub use responses_api::mcp_tool_to_responses_api_tool;
|
||||
pub use responses_api::tool_definition_to_responses_api_tool;
|
||||
pub use tool_definition::ToolDefinition;
|
||||
pub use tool_spec::ConfiguredToolSpec;
|
||||
pub use tool_spec::ResponsesApiWebSearchFilters;
|
||||
pub use tool_spec::ResponsesApiWebSearchUserLocation;
|
||||
pub use tool_spec::ToolSpec;
|
||||
pub use tool_spec::create_tools_json_for_responses_api;
|
||||
|
||||
@@ -6,6 +6,7 @@ use codex_protocol::config_types::WebSearchFilters as ConfigWebSearchFilters;
|
||||
use codex_protocol::config_types::WebSearchUserLocation as ConfigWebSearchUserLocation;
|
||||
use codex_protocol::config_types::WebSearchUserLocationType;
|
||||
use serde::Serialize;
|
||||
use serde_json::Value;
|
||||
|
||||
/// When serialized as JSON, this produces a valid "Tool" in the OpenAI
|
||||
/// Responses API.
|
||||
@@ -60,6 +61,41 @@ impl ToolSpec {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub struct ConfiguredToolSpec {
|
||||
pub spec: ToolSpec,
|
||||
pub supports_parallel_tool_calls: bool,
|
||||
}
|
||||
|
||||
impl ConfiguredToolSpec {
|
||||
pub fn new(spec: ToolSpec, supports_parallel_tool_calls: bool) -> Self {
|
||||
Self {
|
||||
spec,
|
||||
supports_parallel_tool_calls,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn name(&self) -> &str {
|
||||
self.spec.name()
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns JSON values that are compatible with Function Calling in the
|
||||
/// Responses API:
|
||||
/// https://platform.openai.com/docs/guides/function-calling?api-mode=responses
|
||||
pub fn create_tools_json_for_responses_api(
|
||||
tools: &[ToolSpec],
|
||||
) -> Result<Vec<Value>, serde_json::Error> {
|
||||
let mut tools_json = Vec::new();
|
||||
|
||||
for tool in tools {
|
||||
let json = serde_json::to_value(tool)?;
|
||||
tools_json.push(json);
|
||||
}
|
||||
|
||||
Ok(tools_json)
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, PartialEq)]
|
||||
pub struct ResponsesApiWebSearchFilters {
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use super::ConfiguredToolSpec;
|
||||
use super::ResponsesApiWebSearchFilters;
|
||||
use super::ResponsesApiWebSearchUserLocation;
|
||||
use super::ToolSpec;
|
||||
@@ -6,6 +7,7 @@ use crate::FreeformTool;
|
||||
use crate::FreeformToolFormat;
|
||||
use crate::JsonSchema;
|
||||
use crate::ResponsesApiTool;
|
||||
use crate::create_tools_json_for_responses_api;
|
||||
use codex_protocol::config_types::WebSearchContextSize;
|
||||
use codex_protocol::config_types::WebSearchFilters as ConfigWebSearchFilters;
|
||||
use codex_protocol::config_types::WebSearchUserLocation as ConfigWebSearchUserLocation;
|
||||
@@ -79,6 +81,29 @@ fn tool_spec_name_covers_all_variants() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_tool_spec_name_delegates_to_tool_spec() {
|
||||
assert_eq!(
|
||||
ConfiguredToolSpec::new(
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: "lookup_order".to_string(),
|
||||
description: "Look up an order".to_string(),
|
||||
strict: false,
|
||||
defer_loading: None,
|
||||
parameters: JsonSchema::Object {
|
||||
properties: BTreeMap::new(),
|
||||
required: None,
|
||||
additional_properties: None,
|
||||
},
|
||||
output_schema: None,
|
||||
}),
|
||||
/*supports_parallel_tool_calls*/ true,
|
||||
)
|
||||
.name(),
|
||||
"lookup_order"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn web_search_config_converts_to_responses_api_types() {
|
||||
assert_eq!(
|
||||
@@ -107,6 +132,40 @@ fn web_search_config_converts_to_responses_api_types() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn create_tools_json_for_responses_api_includes_top_level_name() {
|
||||
assert_eq!(
|
||||
create_tools_json_for_responses_api(&[ToolSpec::Function(ResponsesApiTool {
|
||||
name: "demo".to_string(),
|
||||
description: "A demo tool".to_string(),
|
||||
strict: false,
|
||||
defer_loading: None,
|
||||
parameters: JsonSchema::Object {
|
||||
properties: BTreeMap::from([(
|
||||
"foo".to_string(),
|
||||
JsonSchema::String { description: None },
|
||||
)]),
|
||||
required: None,
|
||||
additional_properties: None,
|
||||
},
|
||||
output_schema: None,
|
||||
})])
|
||||
.expect("serialize tools"),
|
||||
vec![json!({
|
||||
"type": "function",
|
||||
"name": "demo",
|
||||
"description": "A demo tool",
|
||||
"strict": false,
|
||||
"parameters": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"foo": { "type": "string" }
|
||||
},
|
||||
},
|
||||
})]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn web_search_tool_spec_serializes_expected_wire_shape() {
|
||||
assert_eq!(
|
||||
|
||||
Reference in New Issue
Block a user