diff --git a/crates/agentic-server-core/src/tool/codex.rs b/crates/agentic-server-core/src/tool/codex.rs index ea04bd2..099ee6b 100644 --- a/crates/agentic-server-core/src/tool/codex.rs +++ b/crates/agentic-server-core/src/tool/codex.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use serde_json::Value; @@ -13,10 +13,30 @@ use super::registry::{ToolEntry, ToolType}; // unlikely to collide with user functions, and can be restored to // `{ namespace, name }` on the way back to the client. pub const MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX: &str = "agentic_ns__"; +pub const MAX_MODEL_VISIBLE_TOOL_NAME_LEN: usize = 64; + +const HASHED_NAMESPACE_MEMBER_SUFFIX_LEN: usize = 18; + +fn stable_name_hash(value: &str) -> u64 { + const FNV_OFFSET_BASIS: u64 = 0xcbf2_9ce4_8422_2325; + const FNV_PRIME: u64 = 0x0000_0100_0000_01b3; + + value.bytes().fold(FNV_OFFSET_BASIS, |hash, byte| { + (hash ^ u64::from(byte)).wrapping_mul(FNV_PRIME) + }) +} #[must_use] pub fn model_visible_namespace_member_name(namespace: &str, member: &str) -> String { - format!("{MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX}{namespace}__{member}") + let full_name = format!("{MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX}{namespace}__{member}"); + if full_name.chars().count() <= MAX_MODEL_VISIBLE_TOOL_NAME_LEN { + return full_name; + } + + let hash = stable_name_hash(&full_name); + let readable_len = MAX_MODEL_VISIBLE_TOOL_NAME_LEN - HASHED_NAMESPACE_MEMBER_SUFFIX_LEN; + let readable_prefix = full_name.chars().take(readable_len).collect::(); + format!("{readable_prefix}__{hash:016x}") } /// Registers one `ToolEntry` per `Function` member of `p`, keyed by the @@ -91,14 +111,14 @@ impl NamespaceMap { #[derive(Default)] struct NamespaceMapBuilder { - top_level_names: HashSet, + top_level_registry_keys: HashMap, map: NamespaceMap, } impl NamespaceMapBuilder { - fn new(top_level_names: HashSet) -> Self { + fn new(top_level_registry_keys: HashMap) -> Self { Self { - top_level_names, + top_level_registry_keys, ..Self::default() } } @@ -109,9 +129,10 @@ impl NamespaceMapBuilder { member_name: &str, ) -> Result { let flat_name = model_visible_namespace_member_name(namespace_name, member_name); - if self.top_level_names.contains(&flat_name) { + if let Some(tool_kind) = self.top_level_registry_keys.get(&flat_name) { return Err(ToolError::Config(format!( - "codex namespace member {namespace_name}.{member_name} collides with top-level function {flat_name}" + "codex namespace member {namespace_name}.{member_name} generates name {flat_name}, which collides with a declared {}", + tool_kind.description() ))); } if let Some(existing) = self.map.calls.get(&flat_name) { @@ -177,7 +198,7 @@ pub struct CodexNamespaceHandler; impl CodexNamespaceHandler { /// Rewrites every `Namespace` tool's function members to their flat, /// model-visible names (see [`model_visible_namespace_member_name`]), - /// given real collision detection against sibling top-level tool names. + /// with collision detection against sibling function-call registry keys. /// /// Tools stay `ResponsesTool::Namespace` — only the nested members' /// `name` fields change — so [`ResponsesTool::to_function_tools`] and @@ -190,10 +211,10 @@ impl CodexNamespaceHandler { /// # Errors /// /// Returns [`ToolError::Config`] when a generated namespace member name - /// collides with a top-level function tool or with another namespace - /// member. + /// collides with another declared function-call tool or with another + /// namespace member. pub fn resolve_namespace_members(&self, tools: &[ResponsesTool]) -> Result, ToolError> { - let mut builder = NamespaceMapBuilder::new(typed_top_level_tool_names(tools)); + let mut builder = NamespaceMapBuilder::new(typed_top_level_registry_keys(tools)); tools .iter() .map(|tool| match tool { @@ -211,8 +232,8 @@ impl CodexNamespaceHandler { /// # Errors /// /// Returns [`ToolError::Config`] when a generated namespace member name - /// collides with a top-level function tool or with another namespace - /// member. + /// collides with another declared function-call tool or with another + /// namespace member. pub fn build_namespace_map(&self, tools: Option<&[ResponsesTool]>) -> Result, ToolError> { namespace_map_from_tools(tools) } @@ -227,13 +248,13 @@ impl CodexNamespaceHandler { /// # Errors /// /// Returns [`ToolError::Config`] when a generated namespace member name - /// collides with a top-level function tool or with another namespace - /// member. + /// collides with another declared function-call tool or with another + /// namespace member. pub fn validate_namespace_collisions(&self, tools: Option<&[ResponsesTool]>) -> Result<(), ToolError> { let Some(tools) = tools else { return Ok(()); }; - let mut builder = NamespaceMapBuilder::new(typed_top_level_tool_names(tools)); + let mut builder = NamespaceMapBuilder::new(typed_top_level_registry_keys(tools)); for tool in tools { let ResponsesTool::Namespace(namespace) = tool else { continue; @@ -318,7 +339,7 @@ fn namespace_map_from_tools(tools: Option<&[ResponsesTool]>) -> Result) -> Result HashSet { +fn typed_top_level_registry_keys(tools: &[ResponsesTool]) -> HashMap { tools .iter() - .filter_map(|tool| match tool { - ResponsesTool::Function(function) => Some(function.name.as_str().to_string()), - ResponsesTool::Mcp(_) - | ResponsesTool::WebSearch(_) - | ResponsesTool::FileSearch(_) - | ResponsesTool::CodeInterpreter(_) - | ResponsesTool::Namespace(_) - | ResponsesTool::Custom(_) - | ResponsesTool::Unknown => None, + .filter_map(|tool| { + let registry_key = match tool { + ResponsesTool::Function(function) => function.name.as_str().to_owned(), + ResponsesTool::Mcp(mcp) => mcp.name.as_str().to_owned(), + ResponsesTool::WebSearch(_) => "web_search".to_owned(), + ResponsesTool::FileSearch(_) => "file_search".to_owned(), + ResponsesTool::CodeInterpreter(_) => "code_interpreter".to_owned(), + ResponsesTool::Namespace(_) | ResponsesTool::Custom(_) | ResponsesTool::Unknown => return None, + }; + tool.tool_type().map(|tool_type| (registry_key, tool_type)) }) .collect() } @@ -587,6 +610,106 @@ mod tests { ); } + #[test] + fn long_namespace_member_name_is_stable_and_within_upstream_limit() { + let namespace = "mcp__codex_apps__github"; + let member = "_remove_reaction_from_pr_review_comment"; + + let shortened = model_visible_namespace_member_name(namespace, member); + + assert_eq!(shortened.chars().count(), MAX_MODEL_VISIBLE_TOOL_NAME_LEN); + assert_eq!( + shortened, + "agentic_ns__mcp__codex_apps__github___remove_r__2e989f39f22daf41" + ); + assert_eq!(shortened, model_visible_namespace_member_name(namespace, member)); + assert_ne!( + shortened, + model_visible_namespace_member_name(namespace, "_remove_reaction_from_issue_comment") + ); + } + + #[test] + fn namespace_member_name_preserves_exact_limit_and_shortens_next_character() { + let namespace = "n"; + let fixed_len = MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX.chars().count() + namespace.chars().count() + 2; + let member_at_limit = "m".repeat(MAX_MODEL_VISIBLE_TOOL_NAME_LEN - fixed_len); + let full_name_at_limit = format!("{MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX}{namespace}__{member_at_limit}"); + + assert_eq!(full_name_at_limit.chars().count(), MAX_MODEL_VISIBLE_TOOL_NAME_LEN); + assert_eq!( + model_visible_namespace_member_name(namespace, &member_at_limit), + full_name_at_limit + ); + + let member_over_limit = format!("{member_at_limit}m"); + let shortened = model_visible_namespace_member_name(namespace, &member_over_limit); + assert_eq!(shortened.chars().count(), MAX_MODEL_VISIBLE_TOOL_NAME_LEN); + assert_ne!( + shortened, + format!("{MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX}{namespace}__{member_over_limit}") + ); + } + + #[test] + fn long_unicode_namespace_member_name_stays_valid_utf8() { + let namespace = "工具箱"; + let member = "工具".repeat(30); + + let shortened = model_visible_namespace_member_name(namespace, &member); + + assert_eq!(shortened.chars().count(), MAX_MODEL_VISIBLE_TOOL_NAME_LEN); + assert!(shortened.starts_with("agentic_ns__工具箱__")); + } + + #[test] + fn long_namespace_member_round_trips_through_shortened_name() { + let namespace = "mcp__codex_apps__github"; + let member = "_remove_reaction_from_pr_review_comment"; + let tools: Vec = serde_json::from_value(serde_json::json!([ + { + "type": "namespace", + "name": namespace, + "tools": [{"type": "function", "name": member}] + } + ])) + .unwrap(); + let upstream_name = model_visible_namespace_member_name(namespace, member); + let mut output = vec![completed_call(&upstream_name, "{}")]; + + let resolved = CodexNamespaceHandler + .resolve_namespace_members(&tools) + .expect("valid namespace members"); + assert!(matches!( + resolved.as_slice(), + [ResponsesTool::Namespace(namespace)] + if matches!(&namespace.tools[0], CodexNamespaceMember::Function(function) + if function.name.as_str() == upstream_name) + )); + + let map = CodexNamespaceHandler + .build_namespace_map(Some(&tools)) + .expect("valid namespace map"); + let choice = ToolChoice::Function { + namespace: Some(namespace.to_string()), + name: NonEmptyToolName::try_from(member).unwrap(), + }; + assert_eq!( + CodexNamespaceHandler.resolve_tool_choice(map.as_ref(), Some(&choice)), + ToolChoice::Function { + namespace: None, + name: NonEmptyToolName::try_from(upstream_name).unwrap(), + } + ); + CodexNamespaceHandler.restore_output_items(&mut output, map.as_ref()); + + let OutputItem::FunctionCall(call) = &output[0] else { + panic!("expected function call"); + }; + assert_eq!(call.namespace.as_deref(), Some(namespace)); + assert_eq!(call.name, member); + } + #[test] fn validate_namespace_collisions_rejects_top_level_flat_name_collision() { let tools: Vec = serde_json::from_value(serde_json::json!([ @@ -603,7 +726,7 @@ mod tests { .validate_namespace_collisions(Some(&tools)) .unwrap_err(); - assert!(err.to_string().contains("collides with top-level function")); + assert!(err.to_string().contains("collides with a declared function tool")); } #[test] @@ -620,7 +743,32 @@ mod tests { let err = CodexNamespaceHandler.resolve_namespace_members(&tools).unwrap_err(); - assert!(err.to_string().contains("collides with top-level function")); + assert!(err.to_string().contains("collides with a declared function tool")); + } + + #[test] + fn resolve_namespace_members_rejects_shortened_name_collision_with_later_mcp_tool() { + let namespace = "mcp__codex_apps__github"; + let member = "_remove_reaction_from_pr_review_comment"; + let shortened_name = model_visible_namespace_member_name(namespace, member); + let tools: Vec = serde_json::from_value(serde_json::json!([ + { + "type": "namespace", + "name": namespace, + "tools": [{"type": "function", "name": member}] + }, + { + "type": "mcp", + "name": shortened_name, + "server_label": "fixture", + "server_url": "http://127.0.0.1:1/mcp" + } + ])) + .unwrap(); + + let err = CodexNamespaceHandler.resolve_namespace_members(&tools).unwrap_err(); + + assert!(err.to_string().contains("collides with a declared MCP tool")); } #[test] @@ -650,7 +798,7 @@ mod tests { #[cfg(debug_assertions)] #[should_panic(expected = "namespace collisions must be validated before recording namespace members")] fn namespace_map_builder_debug_asserts_when_member_collision_validation_is_skipped() { - let mut builder = NamespaceMapBuilder::new(HashSet::new()); + let mut builder = NamespaceMapBuilder::new(HashMap::new()); assert_eq!( builder.record_flat_member_with_flat_name("a__b", "c", "agentic_ns__a__b__c".to_owned()), diff --git a/crates/agentic-server-core/src/tool/registry.rs b/crates/agentic-server-core/src/tool/registry.rs index 0e11b29..17c0e8e 100644 --- a/crates/agentic-server-core/src/tool/registry.rs +++ b/crates/agentic-server-core/src/tool/registry.rs @@ -30,6 +30,18 @@ pub enum ToolType { } impl ToolType { + #[must_use] + pub(crate) const fn description(self) -> &'static str { + match self { + Self::Function => "function tool", + Self::CodexNamespace => "Codex namespace tool", + Self::Mcp => "MCP tool", + Self::WebSearch => "web search tool", + Self::FileSearch => "file search tool", + Self::CodeInterpreter => "code interpreter tool", + } + } + #[must_use] pub const fn is_gateway_owned(self) -> bool { !matches!(self, Self::Function | Self::CodexNamespace) diff --git a/crates/agentic-server-core/src/types/request_response.rs b/crates/agentic-server-core/src/types/request_response.rs index ce18768..3648bd2 100644 --- a/crates/agentic-server-core/src/types/request_response.rs +++ b/crates/agentic-server-core/src/types/request_response.rs @@ -513,7 +513,7 @@ mod tests { panic!("colliding namespace member should be rejected"); }; - assert!(err.to_string().contains("collides with top-level function")); + assert!(err.to_string().contains("collides with a declared function tool")); } #[test] diff --git a/crates/agentic-server-core/tests/stateful_responses_integration.rs b/crates/agentic-server-core/tests/stateful_responses_integration.rs index 040781c..4e19ad2 100644 --- a/crates/agentic-server-core/tests/stateful_responses_integration.rs +++ b/crates/agentic-server-core/tests/stateful_responses_integration.rs @@ -7,6 +7,7 @@ mod support; use agentic_core::executor::execute; use agentic_core::executor::request::RequestContext; +use agentic_core::tool::model_visible_namespace_member_name; use agentic_core::types::request_response::RequestPayload; use agentic_core::types::tools::{FunctionToolParam, NonEmptyToolName}; use agentic_core::{FunctionToolResultMessage, InputItem, ResponsesInput, ResponsesTool, ToolChoice}; @@ -367,7 +368,45 @@ async fn test_codex_namespace_collision_with_top_level_function_is_rejected() { }; assert!( - err.to_string().contains("collides with top-level function"), + err.to_string().contains("collides with a declared function tool"), + "unexpected error: {err}" + ); + assert!( + fixture.request_bodies().await.is_empty(), + "invalid request must fail before calling upstream" + ); +} + +#[tokio::test] +async fn test_shortened_namespace_name_collision_with_later_mcp_tool_is_rejected() { + let fixture = TestFixture::new_with_responses(vec![text_response("should not be called")]).await; + let namespace = "mcp__codex_apps__github"; + let member = "_remove_reaction_from_pr_review_comment"; + let shortened_name = model_visible_namespace_member_name(namespace, member); + let tools: Vec = serde_json::from_value(serde_json::json!([ + { + "type": "namespace", + "name": namespace, + "tools": [{"type": "function", "name": member}] + }, + { + "type": "mcp", + "name": shortened_name, + "server_label": "fixture", + "server_url": "http://127.0.0.1:1/mcp" + } + ])) + .unwrap(); + + let mut request = make_request("remove a reaction", true, false, None, None); + request.tools = Some(tools); + + let Err(err) = execute(request, Arc::clone(&fixture.exec_ctx)).await else { + panic!("colliding shortened namespace member should be rejected"); + }; + + assert!( + err.to_string().contains("collides with a declared MCP tool"), "unexpected error: {err}" ); assert!( diff --git a/crates/agentic-server/tests/responses_websocket_test.rs b/crates/agentic-server/tests/responses_websocket_test.rs index d8b8037..e0f6fff 100644 --- a/crates/agentic-server/tests/responses_websocket_test.rs +++ b/crates/agentic-server/tests/responses_websocket_test.rs @@ -25,7 +25,7 @@ use tokio_util::sync::CancellationToken; use agentic_core::executor::{ConversationHandler, ExecutionContext, ResponseHandler}; use agentic_core::proxy::ProxyState; use agentic_core::storage::{ConversationStore, ResponseStore, create_pool_with_schema}; -use agentic_core::tool::WebSearchHandler; +use agentic_core::tool::{WebSearchHandler, model_visible_namespace_member_name}; use agentic_server::app::{AppState, WebSocketTracker}; use common::{spawn_gateway, test_config}; @@ -823,6 +823,65 @@ async fn test_websocket_restores_namespace_tool_call_events() { ); } +#[tokio::test] +async fn test_websocket_bounds_and_restores_long_namespace_tool_name() { + let namespace = "mcp__codex_apps__github"; + let member = "_remove_reaction_from_pr_review_comment"; + let upstream_name = model_visible_namespace_member_name(namespace, member); + let mock = MockResponsesServer::start(vec![sse_function_call_response( + "resp_upstream_long_namespace", + &upstream_name, + )]) + .await; + let fixture = storage_backed_state(&mock.url).await; + let (gateway_url, _gateway) = spawn_gateway(fixture.state.clone()).await; + let mut ws = connect_responses_ws(&gateway_url).await; + + send_json( + &mut ws, + json!({ + "type": "response.create", + "model": "test-model", + "input": "use the long namespace tool", + "tools": [{ + "type": "namespace", + "name": namespace, + "tools": [{ + "type": "function", + "name": member, + "parameters": {"type": "object"} + }] + }], + "store": true, + "stream": true + }), + ) + .await; + + let events = recv_until_completed(&mut ws).await; + let added = events + .iter() + .find(|event| event["type"] == "response.output_item.added") + .unwrap(); + let done = events + .iter() + .find(|event| event["type"] == "response.output_item.done") + .unwrap(); + assert_eq!(added["item"]["namespace"], namespace); + assert_eq!(added["item"]["name"], member); + assert_eq!(done["item"]["namespace"], namespace); + assert_eq!(done["item"]["name"], member); + + let completed = events.last().unwrap(); + assert_eq!(completed["response"]["output"][0]["namespace"], namespace); + assert_eq!(completed["response"]["output"][0]["name"], member); + + let requests = mock.request_bodies().await; + let forwarded_name = requests[0]["tools"][0]["name"].as_str().unwrap(); + assert_eq!(forwarded_name, upstream_name); + assert_eq!(forwarded_name.chars().count(), 64); +} + #[tokio::test] async fn test_websocket_custom_tool_round_trip_and_continuation() { let mock = MockResponsesServer::start(vec![ diff --git a/docs/design/codex-integration.md b/docs/design/codex-integration.md index 45b782b..e4b8e31 100644 --- a/docs/design/codex-integration.md +++ b/docs/design/codex-integration.md @@ -56,6 +56,8 @@ The model-visible namespace member format is: agentic_ns__{namespace}__{member} ``` +Names at or below the upstream 64-character function-name limit retain that exact form. Longer generated names keep a readable prefix and replace the tail with a deterministic 16-hex fingerprint, producing exactly 64 characters. The request-scoped namespace map records the result, so restoration never depends on parsing either form. + For example, Codex can send: ```json @@ -91,10 +93,7 @@ When the model calls that flat function, the gateway restores: ## Collision Handling -The `agentic_ns__` prefix marks gateway-generated namespace member names. If a declared top-level function already uses -the generated name for a namespace member, or if two distinct namespace members generate the same -flat name, the typed executor rejects the request as invalid. Forwarding either shape would make a later model call -ambiguous and impossible to restore reliably to `{ namespace, name }`. +The `agentic_ns__` prefix marks gateway-generated namespace member names. If any other declared tool registers the same function-call name as a namespace member (including a function, MCP tool, or normalized built-in), or if two distinct namespace members generate the same flat name, the typed executor rejects the request as invalid before upstream inference or gateway tool setup. Forwarding either shape would make a later model call ambiguous and impossible to restore reliably to `{ namespace, name }`. Custom tools are excluded from this check because they use `custom_tool_call`, not `function_call`. ---