From 3f31c0a040ca58b86cea70a8d40ad79d481e45d2 Mon Sep 17 00:00:00 2001 From: cassirer-openai Date: Wed, 21 Oct 2026 00:46:41 +0000 Subject: [PATCH] Resolve MCP backend identity from runtime events in rollout traces (#55710) ## Why MCP callable namespaces and tool names can differ from their backend identities or match built-in tool names. Classifying calls by name can misidentify MCP calls and incorrectly parse namespaced `write_stdin` calls as terminal operations. ## What changed Resolve `ToolCallKind::Mcp` from the runtime invocation's server and tool as soon as the runtime starts, before MCP correlation assignment. Preserve the caller-facing summary and raw invocation, and skip built-in runtime handling for MCP calls. Only create dispatch-based `write_stdin` terminal operations for calls without a namespace. ## Testing Add replay regression coverage for prefixed and unprefixed MCP aliases, names that match built-in tools, and `write_stdin` arguments with and without a session ID. Verify backend classification, preserved callable identity and dispatch lifecycle, and the absence of terminal operations and sessions. GitOrigin-RevId: 71f31335c1eb36b6b7e1a34383d10fc500bd2c4f --- codex-rs/rollout-trace/src/model/runtime.rs | 2 + codex-rs/rollout-trace/src/reducer/tool.rs | 8 ++ .../rollout-trace/src/reducer/tool/mcp.rs | 44 ++++++ .../src/reducer/tool/mcp_tests.rs | 130 ++++++++++++++++++ .../src/reducer/tool/terminal.rs | 20 ++- 5 files changed, 199 insertions(+), 5 deletions(-) create mode 100644 codex-rs/rollout-trace/src/reducer/tool/mcp.rs create mode 100644 codex-rs/rollout-trace/src/reducer/tool/mcp_tests.rs diff --git a/codex-rs/rollout-trace/src/model/runtime.rs b/codex-rs/rollout-trace/src/model/runtime.rs index f0d1d1caf1..3d49b0d98a 100644 --- a/codex-rs/rollout-trace/src/model/runtime.rs +++ b/codex-rs/rollout-trace/src/model/runtime.rs @@ -169,6 +169,8 @@ pub enum ToolCallKind { ExecCommand, WriteStdin, ApplyPatch, + /// Resolved backend identity from an MCP runtime invocation. The callable + /// namespace/name may differ and remain in the summary and raw invocation. Mcp { server: String, tool: String, diff --git a/codex-rs/rollout-trace/src/reducer/tool.rs b/codex-rs/rollout-trace/src/reducer/tool.rs index 43845ea939..20e1e2af47 100644 --- a/codex-rs/rollout-trace/src/reducer/tool.rs +++ b/codex-rs/rollout-trace/src/reducer/tool.rs @@ -19,8 +19,13 @@ use crate::raw_event::RawEventSeq; use crate::raw_event::RawToolCallRequester; mod agents; +mod mcp; mod terminal; +#[cfg(test)] +#[path = "tool/mcp_tests.rs"] +mod mcp_tests; + pub(super) use agents::ObservedAgentResultEdge; pub(super) use agents::PendingAgentInteractionEdge; pub(super) use agents::spawn_edge_id; @@ -255,6 +260,9 @@ impl TraceReducer { tool_call.terminal_operation_id.clone(), ) }; + if self.classify_mcp_runtime_start(&tool_call_id, &runtime_payload)? { + return Ok(()); + } if existing_terminal_operation_id.is_some() && matches!(kind, ToolCallKind::ExecCommand | ToolCallKind::WriteStdin) { diff --git a/codex-rs/rollout-trace/src/reducer/tool/mcp.rs b/codex-rs/rollout-trace/src/reducer/tool/mcp.rs new file mode 100644 index 0000000000..9305b67314 --- /dev/null +++ b/codex-rs/rollout-trace/src/reducer/tool/mcp.rs @@ -0,0 +1,44 @@ +//! Resolve MCP backend identity without replacing the caller-facing invocation. + +use anyhow::Context; +use anyhow::Result; +use codex_protocol::protocol::McpInvocation; +use serde::Deserialize; + +use crate::model::ToolCallKind; +use crate::payload::RawPayloadRef; +use crate::reducer::TraceReducer; + +impl TraceReducer { + /// Returns whether the runtime start identifies an MCP backend call. + pub(super) fn classify_mcp_runtime_start( + &mut self, + tool_call_id: &str, + runtime_payload: &RawPayloadRef, + ) -> Result { + // MCP begin events precede correlation assignment, and callable names + // can match built-ins. Only the runtime payload identifies the backend. + let payload = self.read_payload_json(runtime_payload)?; + let Some(invocation) = payload.get("invocation") else { + return Ok(false); + }; + let invocation = McpInvocation::deserialize(invocation).with_context(|| { + format!( + "parse MCP runtime invocation in {}", + runtime_payload.raw_payload_id + ) + })?; + // Callable namespaces are aliases, not backend names: `mcp__web` can + // route to `web`, while other MCP namespaces have no `mcp__` prefix. + // Keep the original callable in the summary and raw invocation payload. + self.rollout + .tool_calls + .get_mut(tool_call_id) + .with_context(|| format!("MCP runtime start referenced unknown call {tool_call_id}"))? + .kind = ToolCallKind::Mcp { + server: invocation.server, + tool: invocation.tool, + }; + Ok(true) + } +} diff --git a/codex-rs/rollout-trace/src/reducer/tool/mcp_tests.rs b/codex-rs/rollout-trace/src/reducer/tool/mcp_tests.rs new file mode 100644 index 0000000000..5c222b8a39 --- /dev/null +++ b/codex-rs/rollout-trace/src/reducer/tool/mcp_tests.rs @@ -0,0 +1,130 @@ +//! MCP identity regressions for callable aliases and backend names. + +use std::sync::Arc; + +use pretty_assertions::assert_eq; +use serde_json::json; +use tempfile::TempDir; + +use crate::ToolDispatchInvocation; +use crate::ToolDispatchPayload; +use crate::ToolDispatchRequester; +use crate::model::ToolCallKind; +use crate::payload::RawPayloadKind; +use crate::raw_event::RawTraceEventPayload; +use crate::reducer::test_support::ROOT_THREAD_ID; +use crate::reducer::test_support::append_completed_inference; +use crate::reducer::test_support::create_started_writer; +use crate::reducer::test_support::start_turn; +use crate::reducer::test_support::trace_context; +use crate::replay_bundle; +use crate::tool_dispatch::ToolDispatchTraceContext; + +#[test] +fn mcp_calls_keep_callable_identity_and_classify_the_backend() -> anyhow::Result<()> { + // Both naming styles are aliases: neither the namespace nor the callable + // tool name determines the backend identity. All test data is invented. + for (namespace, tool_name, arguments) in [ + ("mcp__catalog", "lookup", "{}"), + ("catalog_alias", "lookup", "{}"), + ("catalog_alias", "exec_command", "{}"), + ("catalog_alias", "send_message", "{}"), + ("catalog_alias", "spawn_agent", "{}"), + ("mcp__catalog", "write_stdin", "{}"), + ("catalog_alias", "write_stdin", r#"{"session_id":123}"#), + ] { + let temp = TempDir::new()?; + let writer = Arc::new(create_started_writer(&temp)?); + start_turn(&writer, "turn-1")?; + append_completed_inference( + &writer, + ROOT_THREAD_ID, + "turn-1", + "inference-1", + vec![], + vec![json!({ + "type": "custom_tool_call", "name": "exec", + "call_id": "call-cell", "input": "await tools.call({})" + })], + )?; + writer.append_with_context( + trace_context("turn-1"), + RawTraceEventPayload::CodeCellStarted { + runtime_cell_id: "cell-1".to_string(), + model_visible_call_id: "call-cell".to_string(), + source_js: "await tools.call({})".to_string(), + }, + )?; + let _dispatch = ToolDispatchTraceContext::start( + Arc::clone(&writer), + ToolDispatchInvocation { + thread_id: ROOT_THREAD_ID.to_string(), + codex_turn_id: "turn-1".to_string(), + tool_call_id: "call-mcp".to_string(), + tool_name: tool_name.to_string(), + tool_namespace: Some(namespace.to_string()), + requester: ToolDispatchRequester::CodeCell { + runtime_cell_id: "cell-1".to_string(), + runtime_tool_call_id: "runtime-tool-1".to_string(), + }, + payload: ToolDispatchPayload::Function { + arguments: arguments.to_string(), + }, + }, + ); + let before = replay_bundle(temp.path())?; + // Namespaced write_stdin calls must neither require terminal arguments + // nor create terminal sessions when those arguments happen to be valid. + assert!(before.terminal_operations.is_empty()); + assert!(before.terminal_sessions.is_empty()); + let mut expected = before.tool_calls["call-mcp"].clone(); + expected.kind = ToolCallKind::Mcp { + server: "inventory_service".to_string(), + tool: "find_item".to_string(), + }; + + let invocation = json!({ + "server": "inventory_service", "tool": "find_item", + "arguments": serde_json::from_str::(arguments)? + }); + let begin = writer.write_json_payload( + RawPayloadKind::ToolRuntimeEvent, + &json!({ + "call_id": "call-mcp", "turn_id": "turn-1", "invocation": invocation + }), + )?; + writer.append(RawTraceEventPayload::ToolCallRuntimeStarted { + tool_call_id: "call-mcp".to_string(), + runtime_payload: begin.clone(), + })?; + expected.raw_runtime_payload_ids.push(begin.raw_payload_id); + // A live prefix must already have backend identity. Everything else, + // including the callable label, invocation reference and parent, stays. + assert_eq!(replay_bundle(temp.path())?.tool_calls["call-mcp"], expected); + + // The producer emits the runtime begin before assigning its MCP + // correlation ID, even when the callable name matches a built-in. + writer.append(RawTraceEventPayload::McpToolCallCorrelationAssigned { + tool_call_id: "call-mcp".to_string(), + mcp_call_id: "mcp-correlation-1".to_string(), + })?; + expected.mcp_call_id = Some("mcp-correlation-1".to_string()); + + let end = writer.write_json_payload( + RawPayloadKind::ToolRuntimeEvent, + &json!({ + "call_id": "call-mcp", "turn_id": "turn-1", "invocation": invocation, + "duration": {"secs": 0, "nanos": 1}, "result": {"Ok": {"content": []}} + }), + )?; + writer.append(RawTraceEventPayload::ToolCallRuntimeEnded { + tool_call_id: "call-mcp".to_string(), + status: crate::ExecutionStatus::Completed, + runtime_payload: end.clone(), + })?; + expected.raw_runtime_payload_ids.push(end.raw_payload_id); + // MCP completion must not overwrite the outer dispatch lifecycle. + assert_eq!(replay_bundle(temp.path())?.tool_calls["call-mcp"], expected); + } + Ok(()) +} diff --git a/codex-rs/rollout-trace/src/reducer/tool/terminal.rs b/codex-rs/rollout-trace/src/reducer/tool/terminal.rs index 20d5a34a4e..f9c6bc3788 100644 --- a/codex-rs/rollout-trace/src/reducer/tool/terminal.rs +++ b/codex-rs/rollout-trace/src/reducer/tool/terminal.rs @@ -54,12 +54,15 @@ impl TraceReducer { }; let payload = self.read_payload_json(invocation_payload)?; - let request = parse_dispatch_terminal_request(payload).with_context(|| { + let Some(request) = parse_dispatch_terminal_request(payload).with_context(|| { format!( "parse terminal invocation payload {} as dispatch payload", invocation_payload.raw_payload_id ) - })?; + })? + else { + return Ok(None); + }; self.insert_terminal_operation(TerminalOperationStart { seq, wall_time_unix_ms, @@ -387,8 +390,14 @@ fn parse_protocol_terminal_request( } } -fn parse_dispatch_terminal_request(value: JsonValue) -> Result { +fn parse_dispatch_terminal_request(value: JsonValue) -> Result> { let payload: DispatchedToolTraceRequestPayload = serde_json::from_value(value)?; + // Recorded kinds are name-based. Only the built-in write_stdin owns terminal + // sessions; core records its default namespace as None. Namespaced aliases + // must wait for runtime classification, even if their arguments look native. + if payload.tool_namespace.is_some() { + return Ok(None); + } if payload.tool_name != "write_stdin" { bail!( "dispatch terminal request is for {}, not write_stdin", @@ -410,14 +419,14 @@ fn parse_dispatch_terminal_request(value: JsonValue) -> Result, payload: DispatchedToolPayload, }