mirror of
https://github.com/openai/codex.git
synced 2026-10-21 17:48:44 +00:00
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
This commit is contained in:
committed by
copyberry
parent
b48277327d
commit
3f31c0a040
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
{
|
||||
|
||||
@@ -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<bool> {
|
||||
// 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)
|
||||
}
|
||||
}
|
||||
@@ -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::<serde_json::Value>(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(())
|
||||
}
|
||||
@@ -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<ParsedTerminalRequest> {
|
||||
fn parse_dispatch_terminal_request(value: JsonValue) -> Result<Option<ParsedTerminalRequest>> {
|
||||
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<ParsedTerminalReq
|
||||
let terminal_id = terminal_id_from_json(&args.session_id)
|
||||
.context("write_stdin dispatch payload omitted session_id")?;
|
||||
|
||||
Ok(ParsedTerminalRequest {
|
||||
Ok(Some(ParsedTerminalRequest {
|
||||
terminal_id: Some(terminal_id),
|
||||
request: TerminalRequest::WriteStdin {
|
||||
stdin: args.chars,
|
||||
yield_time_ms: args.yield_time_ms,
|
||||
max_output_tokens: args.max_output_tokens,
|
||||
},
|
||||
})
|
||||
}))
|
||||
}
|
||||
|
||||
fn parse_terminal_response_payload(
|
||||
@@ -565,6 +574,7 @@ struct ExecCommandEndPayload {
|
||||
#[derive(Deserialize)]
|
||||
struct DispatchedToolTraceRequestPayload {
|
||||
tool_name: String,
|
||||
tool_namespace: Option<String>,
|
||||
payload: DispatchedToolPayload,
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user