mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Classify rollout tool calls by their runtime handler (#57194)
## Why Tool interception and MCP aliases can route a requested callable to a different handler. Rollout traces need to reflect the handler that actually executes, including when an `exec_command` invocation is intercepted as `apply_patch`. ## What changed Carry an explicit `ToolCallKind` in runtime-start events and use it to update the tool call's execution classification. This replaces MCP-specific payload inspection while preserving the original callable summary and invocation for intercepted patch and aliased MCP calls. ## Testing Add a regression test verifying that an intercepted `exec_command` is recorded as `ApplyPatch`, retains its original summary and invocation, completes successfully, and creates no terminal operations or sessions. GitOrigin-RevId: 0e72f3ef55cda2796e779efd16241e4a0dbf0c5b
This commit is contained in:
committed by
copyberry
parent
cfc11bfab3
commit
b6a4378862
@@ -31,6 +31,7 @@ use crate::AgentThreadId;
|
||||
use crate::CodexTurnId;
|
||||
use crate::ExecutionStatus;
|
||||
use crate::RawTraceEventPayload;
|
||||
use crate::ToolCallKind;
|
||||
|
||||
pub(crate) struct CodexTurnTraceEvent {
|
||||
pub context_turn_id: CodexTurnId,
|
||||
@@ -93,6 +94,7 @@ pub(crate) fn codex_turn_trace_event(
|
||||
pub(crate) enum ToolRuntimeTraceEvent<'a> {
|
||||
Started {
|
||||
tool_call_id: &'a str,
|
||||
kind: ToolCallKind,
|
||||
payload: ToolRuntimePayload<'a>,
|
||||
},
|
||||
Ended {
|
||||
@@ -280,6 +282,12 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
EventMsg::ExecCommandBegin(event) if event.source != ExecCommandSource::UserShell => {
|
||||
Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: match event.source {
|
||||
ExecCommandSource::UnifiedExecInteraction => ToolCallKind::WriteStdin,
|
||||
ExecCommandSource::Agent
|
||||
| ExecCommandSource::UserShell
|
||||
| ExecCommandSource::UnifiedExecStartup => ToolCallKind::ExecCommand,
|
||||
},
|
||||
payload: ToolRuntimePayload::ExecCommandBegin(event),
|
||||
})
|
||||
}
|
||||
@@ -292,6 +300,7 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
}
|
||||
EventMsg::PatchApplyBegin(event) => Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: ToolCallKind::ApplyPatch,
|
||||
payload: ToolRuntimePayload::PatchApplyBegin(event),
|
||||
}),
|
||||
EventMsg::PatchApplyEnd(event) => Some(ToolRuntimeTraceEvent::Ended {
|
||||
@@ -301,6 +310,10 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
}),
|
||||
EventMsg::McpToolCallBegin(event) => Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: ToolCallKind::Mcp {
|
||||
server: event.invocation.server.clone(),
|
||||
tool: event.invocation.tool.clone(),
|
||||
},
|
||||
payload: ToolRuntimePayload::McpToolCallBegin(event),
|
||||
}),
|
||||
EventMsg::McpToolCallEnd(event) => Some(ToolRuntimeTraceEvent::Ended {
|
||||
@@ -314,6 +327,7 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
}),
|
||||
EventMsg::CollabAgentSpawnBegin(event) => Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: ToolCallKind::SpawnAgent,
|
||||
payload: ToolRuntimePayload::CollabAgentSpawnBegin(event),
|
||||
}),
|
||||
EventMsg::CollabAgentSpawnEnd(event) => Some(ToolRuntimeTraceEvent::Ended {
|
||||
@@ -329,6 +343,7 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
}),
|
||||
EventMsg::CollabAgentInteractionBegin(event) => Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: ToolCallKind::SendMessage,
|
||||
payload: ToolRuntimePayload::CollabAgentInteractionBegin(event),
|
||||
}),
|
||||
EventMsg::CollabAgentInteractionEnd(event) => Some(ToolRuntimeTraceEvent::Ended {
|
||||
@@ -338,6 +353,7 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
}),
|
||||
EventMsg::CollabWaitingBegin(event) => Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: ToolCallKind::WaitAgent,
|
||||
payload: ToolRuntimePayload::CollabWaitingBegin(event),
|
||||
}),
|
||||
EventMsg::CollabWaitingEnd(event) => Some(ToolRuntimeTraceEvent::Ended {
|
||||
@@ -347,6 +363,7 @@ pub(crate) fn tool_runtime_trace_event(event: &EventMsg) -> Option<ToolRuntimeTr
|
||||
}),
|
||||
EventMsg::CollabCloseBegin(event) => Some(ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id: &event.call_id,
|
||||
kind: ToolCallKind::CloseAgent,
|
||||
payload: ToolRuntimePayload::CollabCloseBegin(event),
|
||||
}),
|
||||
EventMsg::CollabCloseEnd(event) => Some(ToolRuntimeTraceEvent::Ended {
|
||||
|
||||
@@ -156,6 +156,8 @@ pub enum RawTraceEventPayload {
|
||||
},
|
||||
ToolCallRuntimeStarted {
|
||||
tool_call_id: ToolCallId,
|
||||
/// Executed handler, which may differ from the requested callable.
|
||||
kind: ToolCallKind,
|
||||
/// Runtime/protocol observation for how Codex began executing the tool.
|
||||
runtime_payload: RawPayloadRef,
|
||||
},
|
||||
|
||||
@@ -340,12 +340,14 @@ impl TraceReducer {
|
||||
}
|
||||
RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
tool_call_id,
|
||||
kind,
|
||||
runtime_payload,
|
||||
} => {
|
||||
self.start_tool_runtime_observation(
|
||||
event.seq,
|
||||
event.wall_time_unix_ms,
|
||||
tool_call_id,
|
||||
kind,
|
||||
runtime_payload,
|
||||
)?;
|
||||
}
|
||||
|
||||
@@ -19,7 +19,6 @@ use crate::raw_event::RawEventSeq;
|
||||
use crate::raw_event::RawToolCallRequester;
|
||||
|
||||
mod agents;
|
||||
mod mcp;
|
||||
mod terminal;
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -242,26 +241,25 @@ impl TraceReducer {
|
||||
seq: RawEventSeq,
|
||||
wall_time_unix_ms: i64,
|
||||
tool_call_id: ToolCallId,
|
||||
kind: ToolCallKind,
|
||||
runtime_payload: RawPayloadRef,
|
||||
) -> Result<()> {
|
||||
let (thread_id, _requester, kind, existing_terminal_operation_id) = {
|
||||
let (thread_id, existing_terminal_operation_id) = {
|
||||
let Some(tool_call) = self.rollout.tool_calls.get_mut(&tool_call_id) else {
|
||||
bail!("tool runtime start referenced unknown call {tool_call_id}");
|
||||
};
|
||||
// Interception and MCP aliases can route a callable to a different handler.
|
||||
// Keep its original summary/invocation, but classify execution by that handler.
|
||||
tool_call.kind = kind.clone();
|
||||
push_unique(
|
||||
&mut tool_call.raw_runtime_payload_ids,
|
||||
&runtime_payload.raw_payload_id,
|
||||
);
|
||||
(
|
||||
tool_call.thread_id.clone(),
|
||||
tool_call.requester.clone(),
|
||||
tool_call.kind.clone(),
|
||||
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)
|
||||
{
|
||||
|
||||
@@ -329,6 +329,7 @@ fn send_message_runtime_payload_targets_delivered_child_message() -> anyhow::Res
|
||||
writer.append_with_context(
|
||||
trace_context_for_agent("turn-1"),
|
||||
RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
kind: ToolCallKind::SendMessage,
|
||||
tool_call_id: "call-send".to_string(),
|
||||
runtime_payload: begin_payload,
|
||||
},
|
||||
@@ -610,6 +611,7 @@ fn close_agent_runtime_payload_targets_thread() -> anyhow::Result<()> {
|
||||
writer.append_with_context(
|
||||
trace_context_for_agent("turn-1"),
|
||||
RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
kind: ToolCallKind::CloseAgent,
|
||||
tool_call_id: "call-close".to_string(),
|
||||
runtime_payload: begin_payload.clone(),
|
||||
},
|
||||
@@ -918,6 +920,7 @@ fn append_spawn_agent_tool_lifecycle(
|
||||
writer.append_with_context(
|
||||
trace_context_for_agent(turn_id),
|
||||
RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
kind: ToolCallKind::SpawnAgent,
|
||||
tool_call_id: "call-spawn".to_string(),
|
||||
runtime_payload: begin.clone(),
|
||||
},
|
||||
|
||||
@@ -1,44 +0,0 @@
|
||||
//! 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)
|
||||
}
|
||||
}
|
||||
@@ -94,6 +94,7 @@ fn mcp_calls_keep_callable_identity_and_classify_the_backend() -> anyhow::Result
|
||||
}),
|
||||
)?;
|
||||
writer.append(RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
kind: expected.kind.clone(),
|
||||
tool_call_id: "call-mcp".to_string(),
|
||||
runtime_payload: begin.clone(),
|
||||
})?;
|
||||
|
||||
@@ -78,6 +78,7 @@ fn exec_tool_reduces_to_terminal_operation_and_session() -> anyhow::Result<()> {
|
||||
let runtime_start = writer.append_with_context(
|
||||
trace_context("turn-1"),
|
||||
RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
kind: ToolCallKind::ExecCommand,
|
||||
tool_call_id: "tool-1".to_string(),
|
||||
runtime_payload: runtime_start_payload,
|
||||
},
|
||||
@@ -267,7 +268,7 @@ fn reused_process_ids_keep_sessions_and_stdin_joins_distinct() -> anyhow::Result
|
||||
model_visible_call_id: None,
|
||||
code_mode_runtime_tool_id: None,
|
||||
requester: crate::raw_event::RawToolCallRequester::Model,
|
||||
kind,
|
||||
kind: kind.clone(),
|
||||
summary: generic_summary("terminal"),
|
||||
invocation_payload: None,
|
||||
},
|
||||
@@ -285,6 +286,7 @@ fn reused_process_ids_keep_sessions_and_stdin_joins_distinct() -> anyhow::Result
|
||||
let runtime_start = writer.append_with_context(
|
||||
trace_context_for_thread(thread, turn),
|
||||
RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
kind,
|
||||
tool_call_id,
|
||||
runtime_payload: request,
|
||||
},
|
||||
|
||||
@@ -497,12 +497,14 @@ impl EnabledThreadTraceContext {
|
||||
match trace_event {
|
||||
crate::protocol_event::ToolRuntimeTraceEvent::Started {
|
||||
tool_call_id,
|
||||
kind,
|
||||
payload,
|
||||
} => {
|
||||
let runtime_payload = self
|
||||
.write_json_payload_best_effort(RawPayloadKind::ToolRuntimeEvent, &payload)?;
|
||||
Some(RawTraceEventPayload::ToolCallRuntimeStarted {
|
||||
tool_call_id: tool_call_id.to_string(),
|
||||
kind,
|
||||
runtime_payload,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1,15 +1,23 @@
|
||||
use std::cell::Cell;
|
||||
use std::collections::HashMap;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_protocol::AgentPath;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::protocol::AgentStatus;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::FileChange;
|
||||
use codex_protocol::protocol::PatchApplyBeginEvent;
|
||||
use codex_protocol::protocol::PatchApplyEndEvent;
|
||||
use codex_protocol::protocol::PatchApplyStatus;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
|
||||
use super::*;
|
||||
@@ -18,6 +26,10 @@ use crate::CompactionCheckpointTracePayload;
|
||||
use crate::ExecutionStatus;
|
||||
use crate::RawTraceEventPayload;
|
||||
use crate::RolloutStatus;
|
||||
use crate::ToolCallKind;
|
||||
use crate::ToolDispatchPayload;
|
||||
use crate::ToolDispatchRequester;
|
||||
use crate::ToolDispatchResult;
|
||||
use crate::replay_bundle;
|
||||
|
||||
#[test]
|
||||
@@ -225,6 +237,81 @@ fn protocol_wrapper_records_selected_events_as_raw_payloads() -> anyhow::Result<
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn intercepted_exec_records_patch_without_a_terminal() -> anyhow::Result<()> {
|
||||
let temp = TempDir::new()?;
|
||||
let thread_id = ThreadId::new();
|
||||
let trace =
|
||||
ThreadTraceContext::start_root_in_root_for_test(temp.path(), minimal_metadata(thread_id))?;
|
||||
trace.record_codex_turn_started("turn-1");
|
||||
let dispatch = trace.start_tool_dispatch_trace(|| Some(ToolDispatchInvocation {
|
||||
thread_id: thread_id.to_string(),
|
||||
codex_turn_id: "turn-1".to_string(),
|
||||
tool_call_id: "patch-call".to_string(),
|
||||
tool_name: "exec_command".to_string(),
|
||||
tool_namespace: None,
|
||||
requester: ToolDispatchRequester::Model {
|
||||
model_visible_call_id: "patch-call".to_string(),
|
||||
},
|
||||
payload: ToolDispatchPayload::Function {
|
||||
arguments: serde_json::json!({"cmd": "apply_patch <<'EOF'\n*** Begin Patch\n*** Add File: hello.txt\n+hello\n*** End Patch\nEOF"}).to_string(),
|
||||
},
|
||||
}));
|
||||
let bundle = single_bundle_dir(temp.path())?;
|
||||
let before = replay_bundle(&bundle)?.tool_calls["patch-call"].clone();
|
||||
let changes = HashMap::from([(
|
||||
PathBuf::from("hello.txt"),
|
||||
FileChange::Add {
|
||||
content: "hello\n".to_string(),
|
||||
},
|
||||
)]);
|
||||
trace.record_tool_call_event(
|
||||
"turn-1",
|
||||
&EventMsg::PatchApplyBegin(PatchApplyBeginEvent {
|
||||
call_id: "patch-call".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
auto_approved: true,
|
||||
changes: changes.clone(),
|
||||
}),
|
||||
);
|
||||
trace.record_tool_call_event(
|
||||
"turn-1",
|
||||
&EventMsg::PatchApplyEnd(PatchApplyEndEvent {
|
||||
call_id: "patch-call".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
stdout: "Success".to_string(),
|
||||
stderr: String::new(),
|
||||
success: true,
|
||||
changes,
|
||||
status: PatchApplyStatus::Completed,
|
||||
}),
|
||||
);
|
||||
dispatch.record_completed(
|
||||
ExecutionStatus::Completed,
|
||||
ToolDispatchResult::DirectResponse {
|
||||
response_item: ResponseInputItem::FunctionCallOutput {
|
||||
call_id: "patch-call".to_string(),
|
||||
output: FunctionCallOutputPayload::from_text("Success".to_string()),
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
let rollout = replay_bundle(&bundle)?;
|
||||
let call = &rollout.tool_calls["patch-call"];
|
||||
assert_eq!(call.kind, ToolCallKind::ApplyPatch);
|
||||
assert_eq!(call.execution.status, ExecutionStatus::Completed);
|
||||
assert_eq!(call.summary, before.summary);
|
||||
assert_eq!(
|
||||
call.raw_invocation_payload_id,
|
||||
before.raw_invocation_payload_id
|
||||
);
|
||||
assert_eq!(call.raw_runtime_payload_ids.len(), 2);
|
||||
assert!(call.raw_result_payload_id.is_some());
|
||||
assert!(rollout.terminal_operations.is_empty());
|
||||
assert!(rollout.terminal_sessions.is_empty());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn minimal_metadata(thread_id: ThreadId) -> ThreadStartedTraceMetadata {
|
||||
ThreadStartedTraceMetadata {
|
||||
thread_id: thread_id.to_string(),
|
||||
|
||||
Reference in New Issue
Block a user