From 9d0a9253efd505ea97039d184d77536c93bf58fa Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 6 May 2026 20:32:04 -0700 Subject: [PATCH] Remove MCP prefixes from hook display names --- codex-rs/core/src/mcp_tool_call.rs | 8 ++--- codex-rs/core/src/mcp_tool_call_tests.rs | 36 ++++++++++++-------- codex-rs/core/src/tools/handlers/mcp.rs | 43 +++++++++++++++++------- codex-rs/core/src/tools/hook_names.rs | 9 +++++ codex-rs/core/src/tools/mod.rs | 6 ++-- codex-rs/core/src/tools/registry.rs | 8 +++-- codex-rs/core/src/unavailable_tool.rs | 7 ++-- codex-rs/core/tests/suite/hooks_mcp.rs | 2 +- codex-rs/protocol/src/tool_name.rs | 19 ++++++++--- 9 files changed, 93 insertions(+), 45 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 74acdc330f..9f7f45e57a 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -115,7 +115,7 @@ pub(crate) async fn handle_mcp_tool_call( call_id: String, server: String, tool_name: String, - hook_tool_name: String, + hook_tool_name: HookToolName, arguments: String, ) -> HandledMcpToolCall { // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON @@ -220,7 +220,7 @@ pub(crate) async fn handle_mcp_tool_call( turn_context, &call_id, &invocation, - &hook_tool_name, + hook_tool_name, metadata.as_ref(), approval_mode, ) @@ -1144,7 +1144,7 @@ async fn maybe_request_mcp_tool_approval( turn_context: &Arc, call_id: &str, invocation: &McpInvocation, - hook_tool_name: &str, + hook_tool_name: HookToolName, metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, ) -> Option { @@ -1204,7 +1204,7 @@ async fn maybe_request_mcp_tool_approval( turn_context, call_id, PermissionRequestPayload { - tool_name: HookToolName::new(hook_tool_name), + tool_name: hook_tool_name, tool_input: invocation .arguments .clone() diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index a556e228ac..dd98e98d72 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -55,6 +55,14 @@ fn annotations( } } +fn hook_tool_name(name: &str) -> HookToolName { + HookToolName::new(name) +} + +fn hook_tool_name_with_legacy_alias(name: &str, legacy_name: &str) -> HookToolName { + HookToolName::new_with_aliases(name, vec![legacy_name.to_string()]) +} + fn approval_metadata( connector_id: Option<&str>, connector_name: Option<&str>, @@ -2186,7 +2194,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &turn_context, "call-1", &invocation, - "mcp__test__tool", + hook_tool_name("read_only_tool"), Some(&metadata), AppToolApproval::Approve, ) @@ -2259,7 +2267,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &turn_context, "call-guardian", &invocation, - "mcp__test__tool", + hook_tool_name("read_only_tool"), Some(&metadata), AppToolApproval::Auto, ) @@ -2315,7 +2323,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &turn_context, "call-mcp-hook", &invocation, - "mcp__memory__create_entities", + hook_tool_name_with_legacy_alias("create_entities", "mcp__memory__create_entities"), Some(&metadata), AppToolApproval::Auto, ) @@ -2336,7 +2344,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { "transcript_path": null, "model": turn_context.model_info.slug, "permission_mode": "default", - "tool_name": "mcp__memory__create_entities", + "tool_name": "create_entities", "hook_event_name": "PermissionRequest", "tool_input": { "entities": [{ @@ -2375,7 +2383,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { &turn_context, "call-mcp-hook-no-metadata", &invocation, - "mcp__memory__create_entities", + hook_tool_name_with_legacy_alias("create_entities", "mcp__memory__create_entities"), /*metadata*/ None, AppToolApproval::Auto, ) @@ -2396,7 +2404,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { "transcript_path": null, "model": turn_context.model_info.slug, "permission_mode": "default", - "tool_name": "mcp__memory__create_entities", + "tool_name": "create_entities", "hook_event_name": "PermissionRequest", "tool_input": { "entities": [] } })] @@ -2452,7 +2460,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &turn_context, "call-mcp-remembered", &invocation, - "mcp__memory__create_entities", + hook_tool_name_with_legacy_alias("create_entities", "mcp__memory__create_entities"), Some(&metadata), AppToolApproval::Auto, ) @@ -2532,7 +2540,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { &turn_context, "call-guardian-deny", &invocation, - "mcp__test__tool", + hook_tool_name("dangerous_tool"), Some(&metadata), AppToolApproval::Auto, ) @@ -2589,7 +2597,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &turn_context, "call-prompt", &invocation, - "mcp__test__tool", + hook_tool_name("read_only_tool"), Some(&metadata), AppToolApproval::Prompt, ) @@ -2664,7 +2672,7 @@ async fn approve_mode_skips_arc_interrupt_for_model() { &turn_context, "call-2", &invocation, - "mcp__test__tool", + hook_tool_name("dangerous_tool"), Some(&metadata), AppToolApproval::Approve, ) @@ -2731,7 +2739,7 @@ async fn custom_approve_mode_skips_arc_interrupt_for_model() { &turn_context, "call-2-custom", &invocation, - "mcp__test__tool", + hook_tool_name("dangerous_tool"), Some(&metadata), AppToolApproval::Approve, ) @@ -2798,7 +2806,7 @@ async fn approve_mode_skips_arc_interrupt_without_annotations() { &turn_context, "call-3", &invocation, - "mcp__test__tool", + hook_tool_name("dangerous_tool"), Some(&metadata), AppToolApproval::Approve, ) @@ -2875,7 +2883,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { &turn_context, "call-2", &invocation, - "mcp__test__tool", + hook_tool_name("dangerous_tool"), Some(&metadata), approval_mode, ) @@ -2978,7 +2986,7 @@ async fn approve_mode_skips_arc_and_guardian_in_every_permission_mode() { &turn_context, "call-3", &invocation, - "mcp__test__tool", + hook_tool_name("dangerous_tool"), Some(&metadata), AppToolApproval::Approve, ) diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 35dbc3bc01..55b3e81e24 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -39,13 +39,16 @@ impl ToolHandler for McpHandler { } fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { - let ToolPayload::Mcp { raw_arguments, .. } = &invocation.payload else { + let ToolPayload::Mcp { + tool, + raw_arguments, + .. + } = &invocation.payload + else { return None; }; - - let tool_name = &self.tool_name; Some(PreToolUsePayload { - tool_name: HookToolName::new(flat_tool_name(tool_name).into_owned()), + tool_name: mcp_hook_tool_name(tool, &self.tool_name), tool_input: mcp_hook_tool_input(raw_arguments), }) } @@ -55,15 +58,14 @@ impl ToolHandler for McpHandler { invocation: &ToolInvocation, result: &Self::Output, ) -> Option { - let ToolPayload::Mcp { .. } = &invocation.payload else { + let ToolPayload::Mcp { tool, .. } = &invocation.payload else { return None; }; let tool_response = result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; - let tool_name = &self.tool_name; Some(PostToolUsePayload { - tool_name: HookToolName::new(flat_tool_name(tool_name).into_owned()), + tool_name: mcp_hook_tool_name(tool, &self.tool_name), tool_use_id: invocation.call_id.clone(), tool_input: result.tool_input.clone(), tool_response, @@ -96,14 +98,14 @@ impl ToolHandler for McpHandler { let arguments_str = raw_arguments; let started = Instant::now(); - let hook_tool_name = flat_tool_name(&self.tool_name); + let hook_tool_name = mcp_hook_tool_name(&tool, &self.tool_name); let result = handle_mcp_tool_call( Arc::clone(&session), &turn, call_id.clone(), server, tool, - hook_tool_name.into_owned(), + hook_tool_name, arguments_str, ) .await; @@ -118,6 +120,15 @@ impl ToolHandler for McpHandler { } } +fn mcp_hook_tool_name(raw_tool_name: &str, canonical_tool_name: &ToolName) -> HookToolName { + let legacy_name = flat_tool_name(canonical_tool_name); + if legacy_name.as_ref() == raw_tool_name { + HookToolName::new(raw_tool_name.to_string()) + } else { + HookToolName::new_with_aliases(raw_tool_name.to_string(), vec![legacy_name.into_owned()]) + } +} + fn mcp_hook_tool_input(raw_arguments: &str) -> Value { if raw_arguments.trim().is_empty() { return Value::Object(serde_json::Map::new()); @@ -138,7 +149,7 @@ mod tests { use tokio::sync::Mutex; #[tokio::test] - async fn mcp_pre_tool_use_payload_uses_model_tool_name_and_raw_args() { + async fn mcp_pre_tool_use_payload_uses_raw_mcp_tool_name_and_args() { let payload = ToolPayload::Mcp { server: "memory".to_string(), tool: "create_entities".to_string(), @@ -168,7 +179,10 @@ mod tests { payload, }), Some(PreToolUsePayload { - tool_name: HookToolName::new("mcp__memory__create_entities"), + tool_name: HookToolName::new_with_aliases( + "create_entities", + vec!["mcp__memory__create_entities".to_string()], + ), tool_input: json!({ "entities": [{ "name": "Ada", @@ -180,7 +194,7 @@ mod tests { } #[tokio::test] - async fn mcp_post_tool_use_payload_uses_model_tool_name_args_and_result() { + async fn mcp_post_tool_use_payload_uses_raw_mcp_tool_name_args_and_result() { let payload = ToolPayload::Mcp { server: "filesystem".to_string(), tool: "read_file".to_string(), @@ -223,7 +237,10 @@ mod tests { assert_eq!( handler.post_tool_use_payload(&invocation, &output), Some(PostToolUsePayload { - tool_name: HookToolName::new("mcp__filesystem__read_file"), + tool_name: HookToolName::new_with_aliases( + "read_file", + vec!["mcp__filesystem__read_file".to_string()], + ), tool_use_id: "call-mcp-post".to_string(), tool_input: json!({ "path": { diff --git a/codex-rs/core/src/tools/hook_names.rs b/codex-rs/core/src/tools/hook_names.rs index 9d3b6c2409..327da2c862 100644 --- a/codex-rs/core/src/tools/hook_names.rs +++ b/codex-rs/core/src/tools/hook_names.rs @@ -25,6 +25,15 @@ impl HookToolName { } } + /// Builds a hook tool name with compatibility aliases used for matcher + /// selection only. + pub(crate) fn new_with_aliases(name: impl Into, matcher_aliases: Vec) -> Self { + Self { + name: name.into(), + matcher_aliases, + } + } + /// Returns the hook identity for file edits performed through `apply_patch`. /// /// The serialized name remains `apply_patch` so logs and policies can key diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 3073d9f9da..25a05e5553 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -33,9 +33,9 @@ pub(crate) const TELEMETRY_PREVIEW_MAX_LINES: usize = 64; // lines pub(crate) const TELEMETRY_PREVIEW_TRUNCATION_NOTICE: &str = "[... telemetry preview truncated ...]"; -/// Legacy boundaries such as hook payloads, telemetry tags, and Responses tool -/// names still require a single flattened string. Keep comparisons and sorting -/// on `ToolName` itself; use this only when crossing those boundaries. +/// Legacy boundaries such as telemetry tags and Responses tool names still +/// require a single flattened string. Keep comparisons and sorting on +/// `ToolName` itself; use this only when crossing those boundaries. pub(crate) fn flat_tool_name(tool_name: &ToolName) -> Cow<'_, str> { match tool_name.namespace.as_deref() { Some(namespace) => { diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 0b734ecdb9..e7b0558c4e 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -627,8 +627,10 @@ async fn dispatch_after_tool_use_hook( let session = invocation.session.as_ref(); let turn = invocation.turn.as_ref(); let tool_input = HookToolInput::from(&invocation.payload); - let tool_name = &invocation.tool_name; - let hook_tool_name = flat_tool_name(tool_name); + let hook_tool_name = match &invocation.payload { + ToolPayload::Mcp { tool, .. } => tool.clone(), + _ => flat_tool_name(&invocation.tool_name).into_owned(), + }; let hook_outcomes = session .hooks() .dispatch(HookPayload { @@ -640,7 +642,7 @@ async fn dispatch_after_tool_use_hook( event: HookEventAfterToolUse { turn_id: turn.sub_id.clone(), call_id: invocation.call_id.clone(), - tool_name: hook_tool_name.into_owned(), + tool_name: hook_tool_name, tool_kind: hook_tool_kind(&tool_input), tool_input, executed: dispatch.executed, diff --git a/codex-rs/core/src/unavailable_tool.rs b/codex-rs/core/src/unavailable_tool.rs index 2c4c1feadf..b76c4d57f9 100644 --- a/codex-rs/core/src/unavailable_tool.rs +++ b/codex-rs/core/src/unavailable_tool.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use std::collections::HashSet; +use crate::tools::flat_tool_name; use codex_protocol::models::ResponseItem; use codex_tools::ToolName; @@ -11,7 +12,7 @@ pub(crate) fn collect_unavailable_called_tools( let mut unavailable_tools = BTreeMap::new(); let exposed_display_names = exposed_tool_names .iter() - .map(ToolName::display) + .map(|name| flat_tool_name(name).into_owned()) .collect::>(); for item in input { @@ -29,8 +30,8 @@ pub(crate) fn collect_unavailable_called_tools( Some(namespace) => ToolName::namespaced(namespace.clone(), name.clone()), None => ToolName::plain(name.clone()), }; - let display_name = tool_name.display(); - if exposed_display_names.contains(&display_name) { + let display_name = flat_tool_name(&tool_name); + if exposed_display_names.contains(display_name.as_ref()) { continue; } diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 26e3053189..20a93cd295 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -27,7 +27,7 @@ use serde_json::json; const RMCP_SERVER: &str = "rmcp"; const RMCP_NAMESPACE: &str = "mcp__rmcp__"; -const RMCP_ECHO_TOOL_NAME: &str = "mcp__rmcp__echo"; +const RMCP_ECHO_TOOL_NAME: &str = "echo"; const RMCP_HOOK_MATCHER: &str = "mcp__rmcp__.*"; const RMCP_ECHO_MESSAGE: &str = "hook e2e ping"; diff --git a/codex-rs/protocol/src/tool_name.rs b/codex-rs/protocol/src/tool_name.rs index 3d7219abe8..36183248d5 100644 --- a/codex-rs/protocol/src/tool_name.rs +++ b/codex-rs/protocol/src/tool_name.rs @@ -36,10 +36,7 @@ impl ToolName { impl fmt::Display for ToolName { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match &self.namespace { - Some(namespace) => write!(f, "{namespace}{}", self.name), - None => f.write_str(&self.name), - } + f.write_str(&self.name) } } @@ -74,3 +71,17 @@ impl From<&str> for ToolName { Self::plain(name) } } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn display_omits_namespace_prefix() { + assert_eq!( + ToolName::namespaced("mcp__server__", "lookup").to_string(), + "lookup" + ); + } +}