mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Emit MCP tool calls as turn items (#20677)
## Why `McpToolCall` was still an app-server item synthesized from deprecated legacy begin/end events. Recent item migrations moved this ownership into core `TurnItem`s, so MCP tool calls now follow the same canonical lifecycle and leave legacy events as compatibility fanout. Keeping the core item close to the v2 `ThreadItem::McpToolCall` shape also avoids spreading MCP result semantics across app-server conversion code. Core now owns whether a completed call is `completed` or `failed`, and whether the payload is a tool result or an error. ## What changed - Added core `TurnItem::McpToolCall` with flattened `server`, `tool`, `arguments`, `status`, `result`, and `error` fields. - Updated MCP tool call emitters, including MCP resource tools, to emit `ItemStarted`/`ItemCompleted` around directly constructed core MCP items. - Updated app-server v2 conversion to project the core MCP item into `ThreadItem::McpToolCall` without deriving status or splitting `Result` locally. - Ignored live deprecated MCP legacy fanout in app-server v2 to avoid duplicate item notifications, while keeping thread history replay on the legacy event path. ## Verification - `cargo test -p codex-protocol` - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-core --lib mcp_tool_call` - `cargo check -p codex-app-server` - `cargo test -p codex-app-server mcp_tool_call_completion_notification_contains_truncated_large_result`
This commit is contained in:
pakrym-oai
committed by
GitHub
Unverified
GitHub
parent
9ddfda9db7
commit
c8c30d9d75
@@ -1,3 +1,4 @@
|
||||
use crate::mcp::CallToolResult;
|
||||
use crate::memory_citation::MemoryCitation;
|
||||
use crate::models::ContentItem;
|
||||
use crate::models::MessagePhase;
|
||||
@@ -10,6 +11,9 @@ use crate::protocol::ContextCompactedEvent;
|
||||
use crate::protocol::EventMsg;
|
||||
use crate::protocol::FileChange;
|
||||
use crate::protocol::ImageGenerationEndEvent;
|
||||
use crate::protocol::McpInvocation;
|
||||
use crate::protocol::McpToolCallBeginEvent;
|
||||
use crate::protocol::McpToolCallEndEvent;
|
||||
use crate::protocol::PatchApplyBeginEvent;
|
||||
use crate::protocol::PatchApplyEndEvent;
|
||||
use crate::protocol::PatchApplyStatus;
|
||||
@@ -27,8 +31,10 @@ use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
use ts_rs::TS;
|
||||
|
||||
#[allow(clippy::large_enum_variant)]
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
|
||||
#[serde(tag = "type")]
|
||||
#[ts(tag = "type")]
|
||||
@@ -42,6 +48,7 @@ pub enum TurnItem {
|
||||
ImageView(ImageViewItem),
|
||||
ImageGeneration(ImageGenerationItem),
|
||||
FileChange(FileChangeItem),
|
||||
McpToolCall(McpToolCallItem),
|
||||
ContextCompaction(ContextCompactionItem),
|
||||
}
|
||||
|
||||
@@ -160,6 +167,45 @@ pub struct FileChangeItem {
|
||||
pub stderr: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema, PartialEq)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
pub struct McpToolCallItem {
|
||||
pub id: String,
|
||||
pub server: String,
|
||||
pub tool: String,
|
||||
pub arguments: serde_json::Value,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub mcp_app_resource_uri: Option<String>,
|
||||
pub status: McpToolCallStatus,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub result: Option<CallToolResult>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub error: Option<McpToolCallError>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(type = "string", optional)]
|
||||
pub duration: Option<Duration>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, Deserialize, Serialize, TS, JsonSchema, PartialEq, Eq)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
pub enum McpToolCallStatus {
|
||||
InProgress,
|
||||
Completed,
|
||||
Failed,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema, PartialEq, Eq)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
pub struct McpToolCallError {
|
||||
pub message: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)]
|
||||
pub struct ContextCompactionItem {
|
||||
pub id: String,
|
||||
@@ -438,6 +484,40 @@ impl FileChangeItem {
|
||||
}
|
||||
}
|
||||
|
||||
impl McpToolCallItem {
|
||||
pub fn as_legacy_begin_event(&self) -> EventMsg {
|
||||
EventMsg::McpToolCallBegin(McpToolCallBeginEvent {
|
||||
call_id: self.id.clone(),
|
||||
invocation: McpInvocation {
|
||||
server: self.server.clone(),
|
||||
tool: self.tool.clone(),
|
||||
arguments: (!self.arguments.is_null()).then(|| self.arguments.clone()),
|
||||
},
|
||||
mcp_app_resource_uri: self.mcp_app_resource_uri.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
pub fn as_legacy_end_event(&self) -> Option<EventMsg> {
|
||||
let result = match (&self.result, &self.error) {
|
||||
(Some(result), _) => Ok(result.clone()),
|
||||
(None, Some(error)) => Err(error.message.clone()),
|
||||
(None, None) => return None,
|
||||
};
|
||||
|
||||
Some(EventMsg::McpToolCallEnd(McpToolCallEndEvent {
|
||||
call_id: self.id.clone(),
|
||||
invocation: McpInvocation {
|
||||
server: self.server.clone(),
|
||||
tool: self.tool.clone(),
|
||||
arguments: (!self.arguments.is_null()).then(|| self.arguments.clone()),
|
||||
},
|
||||
mcp_app_resource_uri: self.mcp_app_resource_uri.clone(),
|
||||
duration: self.duration?,
|
||||
result,
|
||||
}))
|
||||
}
|
||||
}
|
||||
|
||||
impl TurnItem {
|
||||
pub fn id(&self) -> String {
|
||||
match self {
|
||||
@@ -450,6 +530,7 @@ impl TurnItem {
|
||||
TurnItem::ImageView(item) => item.id.clone(),
|
||||
TurnItem::ImageGeneration(item) => item.id.clone(),
|
||||
TurnItem::FileChange(item) => item.id.clone(),
|
||||
TurnItem::McpToolCall(item) => item.id.clone(),
|
||||
TurnItem::ContextCompaction(item) => item.id.clone(),
|
||||
}
|
||||
}
|
||||
@@ -472,6 +553,7 @@ impl TurnItem {
|
||||
.as_legacy_end_event(String::new())
|
||||
.into_iter()
|
||||
.collect(),
|
||||
TurnItem::McpToolCall(item) => item.as_legacy_end_event().into_iter().collect(),
|
||||
TurnItem::Reasoning(item) => item.as_legacy_events(show_raw_agent_reasoning),
|
||||
TurnItem::ContextCompaction(item) => vec![item.as_legacy_event()],
|
||||
}
|
||||
|
||||
@@ -1843,6 +1843,7 @@ impl HasLegacyEvent for ItemStartedEvent {
|
||||
})]
|
||||
}
|
||||
TurnItem::FileChange(item) => vec![item.as_legacy_begin_event(self.turn_id.clone())],
|
||||
TurnItem::McpToolCall(item) => vec![item.as_legacy_begin_event()],
|
||||
_ => Vec::new(),
|
||||
}
|
||||
}
|
||||
@@ -3938,8 +3939,11 @@ mod tests {
|
||||
use super::*;
|
||||
use crate::items::FileChangeItem;
|
||||
use crate::items::ImageGenerationItem;
|
||||
use crate::items::McpToolCallItem;
|
||||
use crate::items::McpToolCallStatus;
|
||||
use crate::items::UserMessageItem;
|
||||
use crate::items::WebSearchItem;
|
||||
use crate::mcp::CallToolResult;
|
||||
use crate::permissions::FileSystemAccessMode;
|
||||
use crate::permissions::FileSystemPath;
|
||||
use crate::permissions::FileSystemSandboxEntry;
|
||||
@@ -4674,6 +4678,40 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn item_started_event_from_mcp_tool_call_emits_begin_event() {
|
||||
let event = ItemStartedEvent {
|
||||
thread_id: ThreadId::new(),
|
||||
turn_id: "turn-1".into(),
|
||||
item: TurnItem::McpToolCall(McpToolCallItem {
|
||||
id: "mcp-1".into(),
|
||||
server: "server".into(),
|
||||
tool: "tool".into(),
|
||||
arguments: json!({"arg": "value"}),
|
||||
mcp_app_resource_uri: Some("app://connector".into()),
|
||||
status: McpToolCallStatus::InProgress,
|
||||
result: None,
|
||||
error: None,
|
||||
duration: None,
|
||||
}),
|
||||
};
|
||||
|
||||
let legacy_events = event.as_legacy_events(/*show_raw_agent_reasoning*/ false);
|
||||
assert_eq!(legacy_events.len(), 1);
|
||||
match &legacy_events[0] {
|
||||
EventMsg::McpToolCallBegin(event) => {
|
||||
assert_eq!(event.call_id, "mcp-1");
|
||||
assert_eq!(event.invocation.server, "server");
|
||||
assert_eq!(event.invocation.tool, "tool");
|
||||
assert_eq!(
|
||||
event.mcp_app_resource_uri.as_deref(),
|
||||
Some("app://connector")
|
||||
);
|
||||
}
|
||||
_ => panic!("expected McpToolCallBegin event"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn item_completed_event_from_image_generation_emits_end_event() {
|
||||
let event = ItemCompletedEvent {
|
||||
@@ -4742,6 +4780,47 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn item_completed_event_from_mcp_tool_call_emits_end_event() {
|
||||
let event = ItemCompletedEvent {
|
||||
thread_id: ThreadId::new(),
|
||||
turn_id: "turn-1".into(),
|
||||
item: TurnItem::McpToolCall(McpToolCallItem {
|
||||
id: "mcp-1".into(),
|
||||
server: "server".into(),
|
||||
tool: "tool".into(),
|
||||
arguments: json!({"arg": "value"}),
|
||||
mcp_app_resource_uri: Some("app://connector".into()),
|
||||
status: McpToolCallStatus::Completed,
|
||||
result: Some(CallToolResult {
|
||||
content: vec![json!({"type": "text", "text": "ok"})],
|
||||
structured_content: None,
|
||||
is_error: Some(false),
|
||||
meta: None,
|
||||
}),
|
||||
error: None,
|
||||
duration: Some(Duration::from_millis(42)),
|
||||
}),
|
||||
};
|
||||
|
||||
let legacy_events = event.as_legacy_events(/*show_raw_agent_reasoning*/ false);
|
||||
assert_eq!(legacy_events.len(), 1);
|
||||
match &legacy_events[0] {
|
||||
EventMsg::McpToolCallEnd(event) => {
|
||||
assert_eq!(event.call_id, "mcp-1");
|
||||
assert_eq!(event.invocation.server, "server");
|
||||
assert_eq!(event.invocation.tool, "tool");
|
||||
assert_eq!(
|
||||
event.mcp_app_resource_uri.as_deref(),
|
||||
Some("app://connector")
|
||||
);
|
||||
assert_eq!(event.duration, Duration::from_millis(42));
|
||||
assert!(event.is_success());
|
||||
}
|
||||
_ => panic!("expected McpToolCallEnd event"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rollback_failed_error_does_not_affect_turn_status() {
|
||||
let event = ErrorEvent {
|
||||
|
||||
Reference in New Issue
Block a user