From a04fe0774fcbd3b8eccf7bb3894898f92387d5fc Mon Sep 17 00:00:00 2001 From: asteroida123 <264808420+asteroida123@users.noreply.github.com> Date: Thu, 10 Sep 2026 07:55:07 +0800 Subject: [PATCH 01/15] fix(codex): restore semantic MCP cards from code-mode history --- src-tauri/src/parsers/codex.rs | 580 ++++++++++++++++++++++- src-tauri/src/parsers/codex_code_mode.rs | 7 + 2 files changed, 571 insertions(+), 16 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 745ce604ee..eae327ffbf 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1445,6 +1445,83 @@ fn unwrap_code_mode_script( ) } +#[derive(Debug)] +struct CompletedMcpCall { + id: String, + server: String, + tool: String, + input_preview: Option, + output_preview: Option, + is_error: bool, +} + +fn completed_mcp_call(payload: &serde_json::Value) -> Option { + let item = payload.get("item")?; + if item.get("type").and_then(|v| v.as_str()) != Some("McpToolCall") { + return None; + } + let result = item.get("result"); + let output_preview = result + .and_then(|result| result.get("content")) + .and_then(crate::parsers::pi::tool_result_content_text) + .or_else(|| { + result + .and_then(|result| result.get("structuredContent")) + .and_then(|value| serde_json::to_string(value).ok()) + }); + Some(CompletedMcpCall { + id: item.get("id")?.as_str()?.to_string(), + server: item.get("server")?.as_str()?.to_string(), + tool: item.get("tool")?.as_str()?.to_string(), + input_preview: value_to_preview(item.get("arguments")), + is_error: result + .and_then(|result| result.get("isError")) + .and_then(|value| value.as_bool()) + .unwrap_or(false) + || infer_tool_call_output_is_error(item, result, output_preview.as_deref()), + output_preview, + }) +} + +fn unwrap_completed_mcp_calls( + script: &CodeModeScript, + completed: Vec, +) -> Option<(Vec, Vec)> { + if script.tool_names.len() != completed.len() || script.tool_names.is_empty() { + return None; + } + let names_match = script + .tool_names + .iter() + .zip(&completed) + .all(|(tool_name, item)| { + let server = item.server.replace('-', "_"); + tool_name == &format!("mcp__{server}__{}", item.tool) + }); + if !names_match { + return None; + } + let mut uses = Vec::with_capacity(completed.len()); + let mut results = Vec::with_capacity(completed.len()); + for (index, item) in completed.into_iter().enumerate() { + uses.push(ContentBlock::ToolUse { + tool_use_id: Some(item.id.clone()), + tool_name: script.tool_names[index].clone(), + input_preview: item.input_preview, + status: Some("completed".into()), + meta: None, + }); + results.push(ContentBlock::ToolResult { + tool_use_id: Some(item.id), + output_preview: item.output_preview, + is_error: item.is_error, + agent_stats: None, + images: Vec::new(), + }); + } + Some((uses, results)) +} + /// What the renderer needs to know about a call recovered from a code-mode /// script, as facts rather than prose: the backend states them, the frontend /// words them in the reader's language. @@ -2822,6 +2899,11 @@ impl CodexParser { // that message's blocks once it knows how many `text()` chunks came // back. See `parsers/codex_code_mode.rs`. let mut pending_exec_scripts: HashMap = HashMap::new(); + // App-server persists each MCP call executed inside a code-mode script + // as a semantic `item_completed.McpToolCall`. Keep those authoritative + // ids/results with the sole open script; its output can then replace the + // wrapper even when several results were printed as one JSON chunk. + let mut completed_mcp_by_exec: HashMap> = HashMap::new(); // `exec_command` call_id → the command it ran, and the background shell // sessions that command's output announced (`session id → command`). // A later `wait` / `write_stdin` carries only the session id, so this is @@ -3303,6 +3385,36 @@ impl CodexParser { } } "item_completed" => { + if let Some(call) = completed_mcp_call(payload) { + let exec_id = if deferred_scripts.is_empty() + && pending_exec_scripts.len() == 1 + { + pending_exec_scripts + .keys() + .next() + .expect("one pending exec") + .clone() + } else if pending_exec_scripts.is_empty() { + let mut deferred_exec_ids = deferred_scripts + .values() + .map(|script| script.call_id.as_str()); + let Some(exec_id) = deferred_exec_ids.next() else { + continue; + }; + if deferred_exec_ids.all(|id| id == exec_id) { + exec_id.to_string() + } else { + continue; + } + } else { + continue; + }; + completed_mcp_by_exec + .entry(exec_id) + .or_default() + .push(call); + continue; + } // Plan mode's finished plan document. This is the // ONLY place a plan turn speaks on the canonical // event channel — codex publishes the plan here @@ -3948,14 +4060,26 @@ impl CodexParser { .collect(), note: collected.note, }; - let (uses, results) = unwrap_code_mode_script( - &deferred.call_id, - &deferred.script, - &parsed, - payload, - &mut shell_sessions, - &mut poll_origins, - ); + let semantic = (parsed.status == ScriptStatus::Completed) + .then(|| { + completed_mcp_by_exec.remove(&deferred.call_id) + }) + .flatten(); + let (uses, results) = semantic + .and_then(|calls| { + unwrap_completed_mcp_calls(&deferred.script, calls) + }) + .map(|(uses, results)| (Some(uses), results)) + .unwrap_or_else(|| { + unwrap_code_mode_script( + &deferred.call_id, + &deferred.script, + &parsed, + payload, + &mut shell_sessions, + &mut poll_origins, + ) + }); if let Some(uses) = uses { messages[deferred.use_index].content = uses; } @@ -3976,14 +4100,24 @@ impl CodexParser { } else if let Some((message_index, script)) = pending_script { let call_id = tool_use_id.unwrap_or_default(); let parsed = split_code_mode_output(payload.get("output")); - let (uses, results) = unwrap_code_mode_script( - &call_id, - &script, - &parsed, - payload, - &mut shell_sessions, - &mut poll_origins, - ); + let semantic = (parsed.status == ScriptStatus::Completed) + .then(|| completed_mcp_by_exec.remove(&call_id)) + .flatten(); + let (uses, results) = semantic + .and_then(|calls| { + unwrap_completed_mcp_calls(&script, calls) + }) + .map(|(uses, results)| (Some(uses), results)) + .unwrap_or_else(|| { + unwrap_code_mode_script( + &call_id, + &script, + &parsed, + payload, + &mut shell_sessions, + &mut poll_origins, + ) + }); if let Some(uses) = uses { messages[message_index].content = uses; } @@ -10404,6 +10538,351 @@ mod tests { let _ = fs::remove_file(path); } + #[test] + fn completed_mcp_items_split_a_two_call_one_chunk_script() { + let script = concat!( + "const wd=\"/tmp\";const taskA=\"A\";const taskB=\"B\";", + "const [a,b]=await Promise.all([", + "tools.mcp__codeg_mcp__delegate_to_agent({agent_type:\"codex\",working_dir:wd,task:taskA}),", + "tools.mcp__codeg_mcp__delegate_to_agent({agent_type:\"codex\",working_dir:wd,task:taskB})", + "]);text(JSON.stringify({a,b}));" + ); + assert!( + crate::parsers::codex_code_mode::parse_code_mode_script(script) + .calls + .is_none(), + "the real variable-argument shape cannot be statically evaluated" + ); + let mut lines = code_mode_rollout( + script, + serde_json::json!([ + {"type":"input_text","text":"Script completed\nWall time 0.2 seconds\nOutput:\n"}, + {"type":"input_text","text":"{\"a\":{},\"b\":{}}"}, + ]), + ); + for (offset, (id, task_id, task)) in [ + ("exec-b", "task-b", "B"), + ("exec-a", "task-a", "A"), + ] + .into_iter() + .enumerate() + { + lines.insert( + 2 + offset, + rollout_line( + "2026-07-20T08:40:01Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": id, + "server": "codeg-mcp", + "tool": "delegate_to_agent", + "arguments": {"agent_type":"codex", "task":task}, + "status": "completed", + "result": { + "content": [{"type":"text", "text":format!( + "Delegation successful. task_id={task_id}." + )}], + "structuredContent": {"task_id":task_id, "status":"running"}, + "isError": false + } + } + }), + ), + ); + } + + let detail = parse_lines(&lines, "code-mode-semantic-mcp"); + let uses = tool_uses(&detail); + assert_eq!( + uses.iter() + .map(|(id, name, _)| (id.as_str(), name.as_str())) + .collect::>(), + vec![ + ("exec-b", "mcp__codeg_mcp__delegate_to_agent"), + ("exec-a", "mcp__codeg_mcp__delegate_to_agent"), + ], + "semantic items replace the outer script with real MCP cards" + ); + assert_eq!( + uses[0].2.as_deref(), + Some(r#"{"agent_type":"codex","task":"B"}"#) + ); + assert_eq!( + uses[1].2.as_deref(), + Some(r#"{"agent_type":"codex","task":"A"}"#) + ); + assert_eq!( + tool_results(&detail) + .into_iter() + .map(|(id, output, _)| (id, output)) + .collect::>(), + vec![ + ("exec-b".into(), Some("Delegation successful. task_id=task-b.".into())), + ("exec-a".into(), Some("Delegation successful. task_id=task-a.".into())), + ] + ); + } + + #[test] + fn mixed_native_collaboration_and_semantic_delegation_keep_their_identities() { + // Keep the upstream native team wire in the same rollout as both the + // initial MCP delegation and its continuation delegation. The records are + // deliberately interleaved: each semantic item must stay with its + // own code-mode script while the native spawn keeps its child session. + let sealed = format!("gAAAAAB{}", "qgWsi0g7nV3UTzqL".repeat(30)); + let native = native_team_0153_lines("FINAL_ANSWER", &sealed); + let initial_script = + "const r = await tools.mcp__codeg_mcp__delegate_to_agent({agent_type:\"codex\",working_dir:\"/tmp/mcp-worker\",task:\"semantic initial\"});text(JSON.stringify(r));"; + let continuation_script = + "const r = await tools.mcp__codeg_mcp__delegate_to_agent({agent_type:\"codex\",working_dir:\"/tmp/mcp-worker\",task:\"semantic followup\",continue_from_task_id:\"task-semantic-initial\"});text(JSON.stringify(r));"; + let initial_status = serde_json::json!({ + "task_id": "task-semantic-initial", + "child_conversation_id": 901, + "status": "running", + }); + let continuation_status = serde_json::json!({ + "task_id": "task-semantic-next", + "child_conversation_id": 901, + "status": "running", + }); + let lines = vec![ + native[0].clone(), // session_meta + rollout_line( + "2026-09-08T06:44:10Z", + "response_item", + serde_json::json!({ + "type": "custom_tool_call", + "name": "exec", + "call_id": "exec-semantic-initial", + "input": initial_script, + }), + ), + rollout_line( + "2026-09-08T06:44:11Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "mcp-semantic-initial", + "server": "codeg-mcp", + "tool": "delegate_to_agent", + "arguments": { + "agent_type": "codex", + "working_dir": "/tmp/mcp-worker", + "task": "semantic initial", + }, + "status": "completed", + "result": { + "content": [{ + "type": "text", + "text": format!( + "Delegation successful. task_id={}. child_conversation_id=901.", + initial_status["task_id"] + .as_str() + .expect("initial task id"), + ), + }], + "structuredContent": initial_status, + "isError": false, + }, + }, + }), + ), + native[1].clone(), // native spawn_agent + native[2].clone(), // native SubAgentActivity started + native[3].clone(), // native spawn result + rollout_line( + "2026-09-08T06:44:33Z", + "response_item", + serde_json::json!({ + "type": "custom_tool_call_output", + "call_id": "exec-semantic-initial", + "output": [ + {"type": "input_text", "text": "Script completed\nWall time 0.1 seconds\nOutput:\n"}, + {"type": "input_text", "text": initial_status.to_string()}, + ], + }), + ), + rollout_line( + "2026-09-08T06:44:34Z", + "response_item", + serde_json::json!({ + "type": "custom_tool_call", + "name": "exec", + "call_id": "exec-semantic-continuation", + "input": continuation_script, + }), + ), + rollout_line( + "2026-09-08T06:44:35Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "mcp-semantic-continuation", + "server": "codeg-mcp", + "tool": "delegate_to_agent", + "arguments": { + "agent_type": "codex", + "working_dir": "/tmp/mcp-worker", + "task": "semantic followup", + "continue_from_task_id": "task-semantic-initial", + }, + "status": "completed", + "result": { + "content": [{ + "type": "text", + "text": format!( + "Delegation successful. task_id={}. child_conversation_id=901.", + continuation_status["task_id"] + .as_str() + .expect("continuation task id"), + ), + }], + "structuredContent": continuation_status, + "isError": false, + }, + }, + }), + ), + native[4].clone(), // native agent_message result + rollout_line( + "2026-09-08T06:44:36Z", + "response_item", + serde_json::json!({ + "type": "custom_tool_call_output", + "call_id": "exec-semantic-continuation", + "output": [ + {"type": "input_text", "text": "Script completed\nWall time 0.1 seconds\nOutput:\n"}, + {"type": "input_text", "text": continuation_status.to_string()}, + ], + }), + ), + native[5].clone(), // native SubAgentActivity completed + ]; + + let detail = parse_lines(&lines, "mixed-native-semantic-delegation"); + let uses = tool_uses(&detail); + let semantic_uses: Vec<_> = uses + .iter() + .filter(|(id, _, _)| id.starts_with("mcp-semantic-")) + .map(|(id, name, input)| (id.as_str(), name.as_str(), input.as_deref())) + .collect(); + assert_eq!( + semantic_uses, + vec![ + ( + "mcp-semantic-initial", + "mcp__codeg_mcp__delegate_to_agent", + Some( + r#"{"agent_type":"codex","task":"semantic initial","working_dir":"/tmp/mcp-worker"}"#, + ), + ), + ( + "mcp-semantic-continuation", + "mcp__codeg_mcp__delegate_to_agent", + Some( + r#"{"agent_type":"codex","continue_from_task_id":"task-semantic-initial","task":"semantic followup","working_dir":"/tmp/mcp-worker"}"#, + ), + ), + ], + "semantic MCP cards keep their own item ids, tool names, and inputs" + ); + assert!( + !uses + .iter() + .any(|(id, name, _)| id.starts_with("exec-semantic-") || name == "exec"), + "completed semantic scripts must not remain as generic exec cards: {uses:?}" + ); + + let semantic_results: Vec<_> = tool_results(&detail) + .into_iter() + .filter(|(id, _, _)| id.starts_with("mcp-semantic-")) + .collect(); + assert_eq!( + semantic_results, + vec![ + ( + "mcp-semantic-initial".to_string(), + Some( + "Delegation successful. task_id=task-semantic-initial. child_conversation_id=901." + .to_string(), + ), + false, + ), + ( + "mcp-semantic-continuation".to_string(), + Some( + "Delegation successful. task_id=task-semantic-next. child_conversation_id=901." + .to_string(), + ), + false, + ), + ], + "each semantic result stays on its matching MCP card" + ); + + let (native_input, native_result) = spawn_capsule(&detail); + assert_eq!( + native_input.get("agent_id").and_then(|value| value.as_str()), + Some("01a07fc2-db62-78b3-9762-9cb2540216c2"), + "native activity must keep its own child session id" + ); + assert_eq!( + native_result.as_deref(), + Some("历史与运行预算增强已完成。"), + "native agent_message must stay attached to the native spawn" + ); + } + + #[test] + fn a_deferred_scripts_late_mcp_item_cannot_bind_to_the_next_script() { + let script = "const r=await tools.mcp__codeg_mcp__delegate_to_agent({agent_type:\"codex\",task:\"A\"});text(JSON.stringify(r));"; + let mut lines = code_mode_rollout( + script, + serde_json::json!("Script running with cell ID 34\nWall time 30.0 seconds\nOutput:\n"), + ); + lines.push(rollout_line( + "2026-07-20T08:40:03Z", + "response_item", + serde_json::json!({ + "type":"custom_tool_call", "name":"exec", "call_id":"call_b", + "input":script.replace("task:\"A\"", "task:\"B\"") + }), + )); + lines.push(rollout_line( + "2026-07-20T08:40:04Z", + "event_msg", + serde_json::json!({ + "type":"item_completed", + "item": { + "type":"McpToolCall", "id":"exec-from-a", "server":"codeg-mcp", + "tool":"delegate_to_agent", "arguments":{"task":"A"}, + "status":"completed", "result":{"content":[], "isError":false} + } + }), + )); + lines.push(rollout_line( + "2026-07-20T08:40:05Z", + "response_item", + serde_json::json!({ + "type":"custom_tool_call_output", "call_id":"call_b", + "output":"Script completed\nWall time 0.1 seconds\nOutput:\nB" + }), + )); + + let ids: Vec = tool_uses(&parse_lines(&lines, "deferred-mcp-boundary")) + .into_iter() + .map(|(id, _, _)| id) + .collect(); + assert_eq!(ids, ["call_1", "call_b"]); + } + #[test] fn code_mode_parallel_calls_split_output_per_card() { let lines = code_mode_rollout( @@ -11410,6 +11889,75 @@ mod tests { lines } + #[test] + fn a_deferred_script_completed_mcp_item_replaces_its_wrapper_card() { + let script = concat!( + "const task=\"t1\";", + "const r=await tools.mcp__codeg_mcp__get_delegation_status({task_ids:[task],wait_ms:60000});", + "text(JSON.stringify(r));" + ); + let mut lines = code_mode_rollout( + script, + serde_json::json!("Script running with cell ID 34\nWall time 11.0 seconds\nOutput:\n"), + ); + lines.push(rollout_line( + "2026-07-20T08:40:03Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "mcp-deferred-status", + "server": "codeg-mcp", + "tool": "get_delegation_status", + "arguments": {"task_ids":["t1"], "wait_ms":60000}, + "result": { + "content": [{"type":"text", "text":"status: running"}], + "isError": false, + }, + }, + }), + )); + lines.push(rollout_line( + "2026-07-20T08:40:04Z", + "response_item", + serde_json::json!({ + "type": "function_call", + "name": "wait", + "call_id": "wait-deferred", + "arguments": "{\"cell_id\":\"34\",\"yield_time_ms\":60000}", + }), + )); + lines.push(rollout_line( + "2026-07-20T08:41:04Z", + "response_item", + serde_json::json!({ + "type": "function_call_output", + "call_id": "wait-deferred", + "output": "Script completed\nWall time 60.0 seconds\nOutput:\n{}", + }), + )); + + let detail = parse_lines(&lines, "deferred-semantic-mcp"); + assert_eq!( + tool_uses(&detail), + vec![ ( + "mcp-deferred-status".into(), + "mcp__codeg_mcp__get_delegation_status".into(), + Some(r#"{"task_ids":["t1"],"wait_ms":60000}"#.into()), + ) ], + "the completed semantic item replaces the parked script card" + ); + assert_eq!( + tool_results(&detail), + vec![( + "mcp-deferred-status".into(), + Some("status: running".into()), + false, + )] + ); + } + #[test] fn a_deferred_scripts_result_lands_on_its_own_card() { let lines = deferred_script_rollout(serde_json::json!([ diff --git a/src-tauri/src/parsers/codex_code_mode.rs b/src-tauri/src/parsers/codex_code_mode.rs index 3076833bd9..f54c007741 100644 --- a/src-tauri/src/parsers/codex_code_mode.rs +++ b/src-tauri/src/parsers/codex_code_mode.rs @@ -72,6 +72,11 @@ pub struct CodeModeScript { /// `Some` only when EVERY call site in the script resolved. `None` means /// the caller must fall back to the script card. pub calls: Option>, + /// Tool names found lexically, in execution-source order, even when an + /// argument object contains variables and therefore cannot be resolved. + /// Semantic app-server items can supply those arguments without evaluating + /// the script, while still checking that every detected call is covered. + pub tool_names: Vec, /// Human title for the script card (best effort, survives resolve failure). pub summary: Option, /// Number of `tools.*` call sites detected, resolved or not. @@ -196,6 +201,7 @@ pub fn parse_code_mode_script(src: &str) -> CodeModeScript { if let Some(calls) = expand_table_fanout(src, &sites) { let summary = best_effort_summary(src, &calls, &sites); return CodeModeScript { + tool_names: calls.iter().map(|call| call.tool_name.clone()).collect(), calls: Some(calls), summary, call_sites: sites.len(), @@ -218,6 +224,7 @@ pub fn parse_code_mode_script(src: &str) -> CodeModeScript { let summary = best_effort_summary(src, if resolved { &calls } else { &[] }, &sites); CodeModeScript { + tool_names: sites.iter().map(|site| site.tool_name.clone()).collect(), calls: if resolved { Some(calls) } else { None }, summary, call_sites: sites.len(), From ca923056f6987a7ce986ae359bd401e38a465c50 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 14:41:01 +0800 Subject: [PATCH 02/15] fix(codex): settle a semantic MCP card on the call's own outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A code-mode script whose MCP call was REFUSED still finishes: codeg-mcp answers `isError: true` and the wrapper prints `Script completed` (measured on a real rollout — a `delegate_to_agent` turned down for `depth_limit`). Hardcoding `status: "completed"` on the recovered card therefore contradicted the `is_error: true` result block written right beside it, and `ContentBlock::ToolUse::status` is documented as a claim a reader may act on. Read the outcome off the item instead. Adds the failed-item regression, plus the guard case that keeps every correlation honest: a script mixing an MCP call with a shell call publishes only one semantic item, so the items cannot be zipped onto the call sites and the script's own reading has to stand. --- src-tauri/src/parsers/codex.rs | 149 ++++++++++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index eae327ffbf..fcd20b354d 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1508,7 +1508,14 @@ fn unwrap_completed_mcp_calls( tool_use_id: Some(item.id.clone()), tool_name: script.tool_names[index].clone(), input_preview: item.input_preview, - status: Some("completed".into()), + // Read off the item's OWN outcome, never hardcoded: a code-mode + // script whose MCP call failed still prints `Script completed` + // (measured: a `delegate_to_agent` refused for `depth_limit` + // settles the script fine), so claiming `completed` here would + // contradict the very result block written next to it. This is a + // per-call terminal record — not `ScriptStatus`, which + // `ContentBlock::ToolUse::status` documents as unsafe to copy. + status: Some(if item.is_error { "failed" } else { "completed" }.into()), meta: None, }); results.push(ContentBlock::ToolResult { @@ -10452,6 +10459,27 @@ mod tests { .collect() } + /// `(tool_use_id, status)` per ToolUse block. Separate from `tool_uses` + /// because only the semantic MCP cards carry a status at all — codex + /// leaves it `None` everywhere else (see `ContentBlock::ToolUse::status`). + fn tool_use_statuses( + detail: &crate::models::ConversationDetail, + ) -> Vec<(String, Option)> { + detail + .turns + .iter() + .flat_map(|t| t.blocks.iter()) + .filter_map(|b| match b { + ContentBlock::ToolUse { + tool_use_id, + status, + .. + } => Some((tool_use_id.clone().unwrap_or_default(), status.clone())), + _ => None, + }) + .collect() + } + fn tool_results( detail: &crate::models::ConversationDetail, ) -> Vec<(String, Option, bool)> { @@ -10883,6 +10911,125 @@ mod tests { assert_eq!(ids, ["call_1", "call_b"]); } + /// A refused MCP call still lets the SCRIPT finish, so the wrapper's own + /// `Script completed` says nothing about the call inside it. The card has to + /// settle on the semantic item's outcome or it contradicts the result block + /// written beside it. Shape taken verbatim from a real rollout: codeg-mcp + /// refuses a `delegate_to_agent` past the delegation depth limit. + #[test] + fn a_failed_semantic_mcp_item_settles_its_card_as_failed() { + let script = concat!( + "const dir=\"/tmp/w\";", + "const res=await tools.mcp__codeg_mcp__delegate_to_agent({agent_type:\"codex\",working_dir:dir,task:t});", + "text(res.content[0].text);" + ); + let mut lines = code_mode_rollout( + script, + serde_json::json!([ + {"type":"input_text","text":"Script completed\nWall time 0.0 seconds\nOutput:\n"}, + {"type":"input_text","text":"depth limit exceeded (2 >= 2)"}, + ]), + ); + lines.insert( + 2, + rollout_line( + "2026-07-20T08:40:01Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "exec-depth-limit", + "server": "codeg-mcp", + "tool": "delegate_to_agent", + "arguments": {"agent_type":"codex", "working_dir":"/tmp/w"}, + "status": "failed", + "result": { + "content": [{"type":"text", "text":"depth limit exceeded (2 >= 2)"}], + "structuredContent": {"error_code":"depth_limit", "status":"failed"}, + "isError": true + } + } + }), + ), + ); + + let detail = parse_lines(&lines, "semantic-mcp-failed"); + assert_eq!( + tool_use_statuses(&detail), + vec![("exec-depth-limit".to_string(), Some("failed".to_string()))], + "the card must report the call's own outcome, not the wrapper's" + ); + assert_eq!( + tool_results(&detail), + vec![( + "exec-depth-limit".to_string(), + Some("depth limit exceeded (2 >= 2)".to_string()), + true, + )] + ); + } + + /// The guard that keeps every correlation honest: a script that mixes an + /// MCP call with a shell call publishes only ONE semantic item, so the + /// items cannot be zipped onto the call sites. Real shape — a status poll + /// racing a `write_stdin` — from a rollout on disk. The script card (or its + /// static decomposition) has to keep the turn rather than let the lone item + /// claim a site it may not own. + #[test] + fn a_script_mixing_mcp_and_shell_calls_keeps_its_static_reading() { + let lines_with_item = |item: bool| { + let mut lines = code_mode_rollout( + concat!( + "const rs = await Promise.all([\n", + " tools.mcp__codeg_mcp__get_delegation_status({task_ids:[\"t1\"],wait_ms:30000}),\n", + " tools.write_stdin({session_id:480,chars:\"y\\n\"}),\n", + "]);\ntext(JSON.stringify(rs));" + ), + serde_json::json!([ + {"type":"input_text","text":"Script completed\nWall time 30.0 seconds\nOutput:\n"}, + {"type":"input_text","text":"[{\"tasks\":[]},{}]"}, + ]), + ); + if item { + lines.insert( + 2, + rollout_line( + "2026-07-20T08:40:01Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "exec-poll", + "server": "codeg-mcp", + "tool": "get_delegation_status", + "arguments": {"task_ids":["t1"], "wall_ms":30000}, + "status": "completed", + "result": {"content":[{"type":"text","text":"{\"tasks\":[]}"}], "isError":false} + } + }), + ), + ); + } + tool_uses(&parse_lines( + &lines, + if item { "mixed-with-item" } else { "mixed-baseline" }, + )) + }; + + let with_item = lines_with_item(true); + assert!( + !with_item.iter().any(|(id, _, _)| id == "exec-poll"), + "one item cannot cover two call sites: {with_item:?}" + ); + assert_eq!( + with_item, + lines_with_item(false), + "an uncorrelatable item must leave the script's own reading untouched" + ); + } + #[test] fn code_mode_parallel_calls_split_output_per_card() { let lines = code_mode_rollout( From 290f4db08b11310d3d613bed6dd4f4c3df7d5dfd Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 15:22:03 +0800 Subject: [PATCH 03/15] fix(codex): trust a semantic MCP record's own outcome and content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the recovered card could misreport a call it has the facts for. `infer_tool_call_output_is_error` reads an outcome out of the output TEXT because a script card carries no error field. An app-server `McpToolCall` does — `result.isError`, plus the item's own terminal `status` — so running the heuristic on top of it could only invent failures: a tool that wraps a command and prints `exit code: 1`, or answers with a line opening `Error:`, returned perfectly well. Explicit fields now decide; the heuristic stays as the fallback for a record that states neither. The semantic path also discards the wrapper's printed output, so a result carrying no text of its own — an image-only `content` array, or a transport failure that answered with `error` and no result — left a completed call rendering as an empty card where the script card used to show the run. Fall through to the error string and then to the serialized result, which is what `pi::content_to_text` already does with this same MCP shape. Projecting MCP image blocks onto `ToolResult::images` would be better still, and wants the raw-JSON extractor the typed ACP path has and the parsers do not. --- src-tauri/src/parsers/codex.rs | 148 +++++++++++++++++++++++++++++++-- 1 file changed, 141 insertions(+), 7 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index fcd20b354d..0a310e49de 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1460,7 +1460,14 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { if item.get("type").and_then(|v| v.as_str()) != Some("McpToolCall") { return None; } - let result = item.get("result"); + let result = item.get("result").filter(|value| !value.is_null()); + // Text first, then the structured twin. The last two are for the shapes + // that carry neither — an image-only / resource-only `content` array, or a + // transport failure that answered with `error` and no result. The semantic + // path DISCARDS the wrapper's own printed output, so a `None` here is not a + // quiet degradation, it is a card that says nothing at all where the script + // card used to show the run's text. Serializing rather than dropping is + // what `pi::content_to_text` already does with the same MCP shape. let output_preview = result .and_then(|result| result.get("content")) .and_then(crate::parsers::pi::tool_result_content_text) @@ -1468,17 +1475,37 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { result .and_then(|result| result.get("structuredContent")) .and_then(|value| serde_json::to_string(value).ok()) - }); + }) + .or_else(|| value_to_preview(item.get("error"))) + .or_else(|| result.and_then(|result| serde_json::to_string(result).ok())); + // The record STATES its outcome — `result.isError`, and the item's own + // terminal `status`. Believe it. `infer_tool_call_output_is_error` reads + // tea leaves out of the output text because a script card has no such + // field; run against an authoritative record it can only invent failures, + // and a tool that legitimately PRINTS `exit code: 1` or a line starting + // `Error:` returned perfectly well. Kept as the fallback for a record that + // states neither. + let claimed_failed = result + .and_then(|result| result.get("isError")) + .and_then(serde_json::Value::as_bool) + == Some(true) + || item + .get("status") + .and_then(serde_json::Value::as_str) + .is_some_and(is_failed_status); + let claimed_ok = result + .and_then(|result| result.get("isError")) + .and_then(serde_json::Value::as_bool) + == Some(false) + || item.get("status").and_then(serde_json::Value::as_str) == Some("completed"); Some(CompletedMcpCall { id: item.get("id")?.as_str()?.to_string(), server: item.get("server")?.as_str()?.to_string(), tool: item.get("tool")?.as_str()?.to_string(), input_preview: value_to_preview(item.get("arguments")), - is_error: result - .and_then(|result| result.get("isError")) - .and_then(|value| value.as_bool()) - .unwrap_or(false) - || infer_tool_call_output_is_error(item, result, output_preview.as_deref()), + is_error: claimed_failed + || (!claimed_ok + && infer_tool_call_output_is_error(item, result, output_preview.as_deref())), output_preview, }) } @@ -10970,6 +10997,113 @@ mod tests { ); } + /// A tool whose SUCCESSFUL answer merely reads like a failure — it wraps a + /// command and prints its exit code, or opens with `Error:` — must not be + /// painted as a failed call. The record says `isError: false` outright, and + /// an authoritative field beats the text heuristic that exists only because + /// a script card has none. + #[test] + fn an_explicit_success_survives_output_text_that_reads_like_an_error() { + let script = concat!( + "const cmd=\"pnpm build\";", + "const r=await tools.mcp__shell_srv__run({cmd});", + "text(r.content[0].text);" + ); + let mut lines = code_mode_rollout( + script, + serde_json::json!([ + {"type":"input_text","text":"Script completed\nWall time 0.2 seconds\nOutput:\n"}, + {"type":"input_text","text":"Error: 2 problems\nexit code: 1"}, + ]), + ); + lines.insert( + 2, + rollout_line( + "2026-07-20T08:40:01Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "exec-lint", + "server": "shell-srv", + "tool": "run", + "arguments": {"cmd":"pnpm build"}, + "status": "completed", + "result": { + "content": [{"type":"text", "text":"Error: 2 problems\nexit code: 1"}], + "isError": false + } + } + }), + ), + ); + + let detail = parse_lines(&lines, "semantic-mcp-noisy-success"); + assert_eq!( + tool_use_statuses(&detail), + vec![("exec-lint".to_string(), Some("completed".to_string()))] + ); + assert_eq!( + tool_results(&detail), + vec![( + "exec-lint".to_string(), + Some("Error: 2 problems\nexit code: 1".to_string()), + false, + )], + "the record's own isError is the outcome, not what the output reads like" + ); + } + + /// The semantic path throws the wrapper's printed output away, so a result + /// that carries no text of its own must still be given something to say — + /// otherwise a completed call reloads as a card with an empty body where + /// the script card used to show the run. + #[test] + fn a_textless_semantic_result_still_says_something() { + let script = "const shot=await tools.mcp__shot_srv__capture({url:target});text(\"captured\");"; + let mut lines = code_mode_rollout( + script, + serde_json::json!([ + {"type":"input_text","text":"Script completed\nWall time 0.4 seconds\nOutput:\n"}, + {"type":"input_text","text":"captured"}, + ]), + ); + lines.insert( + 2, + rollout_line( + "2026-07-20T08:40:01Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "exec-shot", + "server": "shot-srv", + "tool": "capture", + "arguments": {"url":"https://example.test"}, + "status": "completed", + "result": { + "content": [{"type":"image", "data":"iVBORw0KGgo=", "mimeType":"image/png"}], + "isError": false + } + } + }), + ), + ); + + let detail = parse_lines(&lines, "semantic-mcp-textless"); + let results = tool_results(&detail); + assert_eq!(results.len(), 1, "one card: {results:?}"); + let (id, output, is_error) = &results[0]; + assert_eq!(id, "exec-shot"); + assert!(!is_error, "an image-only answer is not a failure"); + assert!( + output.as_deref().is_some_and(|text| text.contains("image")), + "a textless result must still carry its content: {output:?}" + ); + } + /// The guard that keeps every correlation honest: a script that mixes an /// MCP call with a shell call publishes only ONE semantic item, so the /// items cannot be zipped onto the call sites. Real shape — a status poll From 43de8bb6f0512e639cb012ca4b8d968af63d45c1 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 16:11:32 +0800 Subject: [PATCH 04/15] fix(codex): bound the semantic result fallback and state its error rule once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three refinements from a second review pass. The serialize fallback took the whole `CallToolResult` — protocol fields, `_meta` and any base64 blob — with no bound, and rendered `{"content":[], "isError":false}` for a call that simply returned nothing. Serialize the `content` alone, only when it carries blocks, capped like `pi` caps its own fallback on the same shape. A call that answered with nothing still says nothing. A record can also state failure through `error` alone. That was reachable only through the text heuristic, which a stated success now suppresses, so a transport failure sitting beside `status: "completed"` would have rendered green. Read it as the stated failure it is. The rule for when an `error` field STATES an error rather than merely existing — `null`, `false` and a blank string are how a record says "no error" — was written out inline twice already; it is `is_stated_error` now, verbatim, and both sites use it. Tests: the outcome precedence is pinned directly against the fields, so the disagreements (`isError` against `status`, either against the output text) are covered rather than implied by four agreeing rollouts; and the mixed shell/MCP guard compares the results as well as the calls. --- src-tauri/src/parsers/codex.rs | 177 +++++++++++++++++++++++++-------- 1 file changed, 135 insertions(+), 42 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 0a310e49de..23ba35d809 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1455,19 +1455,25 @@ struct CompletedMcpCall { is_error: bool, } +/// How much of a serialized MCP result stands in for a call that answered in +/// blocks with no text of its own. Matches `pi`'s cap on the same shape: enough +/// to show what came back, not enough for a base64 blob to swamp the card. +const MCP_RESULT_FALLBACK_CAP: usize = 4000; + fn completed_mcp_call(payload: &serde_json::Value) -> Option { let item = payload.get("item")?; if item.get("type").and_then(|v| v.as_str()) != Some("McpToolCall") { return None; } let result = item.get("result").filter(|value| !value.is_null()); + let is_error = item.get("error").filter(|value| is_stated_error(value)); // Text first, then the structured twin. The last two are for the shapes - // that carry neither — an image-only / resource-only `content` array, or a - // transport failure that answered with `error` and no result. The semantic - // path DISCARDS the wrapper's own printed output, so a `None` here is not a - // quiet degradation, it is a card that says nothing at all where the script - // card used to show the run's text. Serializing rather than dropping is - // what `pi::content_to_text` already does with the same MCP shape. + // that carry neither — a transport failure that answered with `error` and + // no result, or a `content` array holding only blocks this reader cannot + // render (an image, a resource). The semantic path DISCARDS the wrapper's + // own printed output, so a `None` here is not a quiet degradation, it is a + // card that says nothing at all where the script card used to show the + // run's text. let output_preview = result .and_then(|result| result.get("content")) .and_then(crate::parsers::pi::tool_result_content_text) @@ -1476,27 +1482,39 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { .and_then(|result| result.get("structuredContent")) .and_then(|value| serde_json::to_string(value).ok()) }) - .or_else(|| value_to_preview(item.get("error"))) - .or_else(|| result.and_then(|result| serde_json::to_string(result).ok())); - // The record STATES its outcome — `result.isError`, and the item's own - // terminal `status`. Believe it. `infer_tool_call_output_is_error` reads - // tea leaves out of the output text because a script card has no such - // field; run against an authoritative record it can only invent failures, - // and a tool that legitimately PRINTS `exit code: 1` or a line starting + .or_else(|| value_to_preview(is_error)) + .or_else(|| { + // `content` rather than the whole envelope, and bounded: a blob + // must not flood the card with base64 and protocol noise. A call + // that returned NOTHING still says nothing — `{"content":[]}` is + // not worth rendering. + let content = result?.get("content")?; + let carries_blocks = content.as_array().is_some_and(|blocks| !blocks.is_empty()); + carries_blocks + .then(|| serde_json::to_string(content).ok()) + .flatten() + .map(|text| truncate_str(&text, MCP_RESULT_FALLBACK_CAP)) + }); + // The record STATES its outcome — `result.isError`, the item's own terminal + // `status`, and an `error` when the call never reached the server. Believe + // them. `infer_tool_call_output_is_error` reads tea leaves out of the + // output text because a script card has no such field; run against an + // authoritative record it can only invent failures, and a tool that + // legitimately PRINTS `exit code: 1` or answers with a line opening // `Error:` returned perfectly well. Kept as the fallback for a record that - // states neither. - let claimed_failed = result + // states nothing. Stated failure outranks stated success, so a record + // contradicting itself settles as the error it reported. + let stated_is_error = result .and_then(|result| result.get("isError")) - .and_then(serde_json::Value::as_bool) - == Some(true) - || item - .get("status") - .and_then(serde_json::Value::as_str) - .is_some_and(is_failed_status); - let claimed_ok = result - .and_then(|result| result.get("isError")) - .and_then(serde_json::Value::as_bool) - == Some(false) + .and_then(serde_json::Value::as_bool); + let claimed_failed = + stated_is_error == Some(true) + || is_error.is_some() + || item + .get("status") + .and_then(serde_json::Value::as_str) + .is_some_and(is_failed_status); + let claimed_ok = stated_is_error == Some(false) || item.get("status").and_then(serde_json::Value::as_str) == Some("completed"); Some(CompletedMcpCall { id: item.get("id")?.as_str()?.to_string(), @@ -1995,6 +2013,18 @@ fn infer_output_text_is_error(text: &str) -> bool { .is_some_and(|prefix| prefix.eq_ignore_ascii_case("error:")) } +/// Whether an `error` field STATES an error rather than merely existing. +/// `null`, `false` and a blank string are how a record says "no error", and a +/// reader that took their presence for failure would fail every clean call +/// that carries the key. +fn is_stated_error(value: &serde_json::Value) -> bool { + match value { + serde_json::Value::Null | serde_json::Value::Bool(false) => false, + serde_json::Value::String(text) => !text.trim().is_empty(), + _ => true, + } +} + fn infer_output_value_is_error(value: &serde_json::Value, depth: usize) -> bool { if depth > 4 { return false; @@ -2040,13 +2070,8 @@ fn infer_output_value_is_error(value: &serde_json::Value, depth: usize) -> bool } } - if let Some(error) = map.get("error") { - match error { - serde_json::Value::Null => {} - serde_json::Value::Bool(false) => {} - serde_json::Value::String(s) if s.trim().is_empty() => {} - _ => return true, - } + if map.get("error").is_some_and(is_stated_error) { + return true; } for key in ["output", "result", "details", "data"] { @@ -2081,13 +2106,8 @@ fn infer_tool_call_output_is_error( } } - if let Some(error) = payload.get("error") { - match error { - serde_json::Value::Null => {} - serde_json::Value::Bool(false) => {} - serde_json::Value::String(s) if s.trim().is_empty() => {} - _ => return true, - } + if payload.get("error").is_some_and(is_stated_error) { + return true; } if let Some(output) = output_value { @@ -6290,6 +6310,7 @@ mod tests { use super::extract_response_item_user_image_blocks; use super::extract_turn_usage_from_codex_usage; use super::codex_parent_thread_id; + use super::completed_mcp_call; use super::is_encrypted_envelope; use super::merge_codex_context_window_stats; use super::native_team_wait_input; @@ -11146,15 +11167,16 @@ mod tests { ), ); } - tool_uses(&parse_lines( + let detail = parse_lines( &lines, if item { "mixed-with-item" } else { "mixed-baseline" }, - )) + ); + (tool_uses(&detail), tool_results(&detail)) }; let with_item = lines_with_item(true); assert!( - !with_item.iter().any(|(id, _, _)| id == "exec-poll"), + !with_item.0.iter().any(|(id, _, _)| id == "exec-poll"), "one item cannot cover two call sites: {with_item:?}" ); assert_eq!( @@ -11164,6 +11186,77 @@ mod tests { ); } + /// The outcome precedence, stated once against the fields themselves rather + /// than through four rollouts: a stated failure outranks a stated success, + /// a stated success outranks output text that merely reads like a failure, + /// and the text heuristic still decides a record that states nothing. + #[test] + fn a_semantic_records_stated_outcome_outranks_its_output_text() { + let is_error = |status: Option<&str>, result: serde_json::Value| { + let mut item = serde_json::json!({ + "type": "McpToolCall", "id": "i", "server": "s", "tool": "t", + }); + if let Some(status) = status { + item["status"] = status.into(); + } + if !result.is_null() { + item["result"] = result; + } + completed_mcp_call(&serde_json::json!({ "item": item })) + .expect("well-formed item") + .is_error + }; + // Output a successful tool can legitimately return: a wrapped command's + // own complaint. `infer_output_text_is_error` reads it as a failure. + let noisy = serde_json::json!({ + "content": [{"type":"text", "text":"Error: 2 problems\nexit code: 1"}] + }); + let flagged = |flag: bool| { + let mut result = noisy.clone(); + result["isError"] = flag.into(); + result + }; + + assert!( + is_error(None, noisy.clone()), + "a record that states nothing leaves the text to decide" + ); + assert!( + !is_error(Some("completed"), noisy.clone()), + "a stated success outranks output that merely reads like a failure" + ); + assert!( + !is_error(None, flagged(false)), + "isError alone is enough to state that success" + ); + assert!( + is_error(Some("completed"), flagged(true)), + "a stated failure outranks a stated success" + ); + assert!( + is_error(Some("failed"), flagged(false)), + "and does so whichever field states it" + ); + assert!( + !is_error( + Some("completed"), + serde_json::json!({"content":[{"type":"text","text":"fine"}], "isError":false}), + ), + "quiet output with nothing wrong stays clean" + ); + assert!( + completed_mcp_call(&serde_json::json!({ + "item": { + "type":"McpToolCall", "id":"i", "server":"s", "tool":"t", + "status":"completed", "error":"connection refused", "result": null, + } + })) + .expect("well-formed item") + .is_error, + "a transport error is stated too, even beside a completed status" + ); + } + #[test] fn code_mode_parallel_calls_split_output_per_card() { let lines = code_mode_rollout( From 2cb0f7a50c52ab3ed494fc98287eddf03087d413 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 16:12:28 +0800 Subject: [PATCH 05/15] refactor(codex): name the stated error field for what it is `is_error` read as the boolean outcome sitting three lines below it; the local holds the record's `error` VALUE, which is only one of the signals that decide that boolean. --- src-tauri/src/parsers/codex.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 23ba35d809..4ca44e77a0 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1466,7 +1466,7 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { return None; } let result = item.get("result").filter(|value| !value.is_null()); - let is_error = item.get("error").filter(|value| is_stated_error(value)); + let stated_error = item.get("error").filter(|value| is_stated_error(value)); // Text first, then the structured twin. The last two are for the shapes // that carry neither — a transport failure that answered with `error` and // no result, or a `content` array holding only blocks this reader cannot @@ -1482,7 +1482,7 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { .and_then(|result| result.get("structuredContent")) .and_then(|value| serde_json::to_string(value).ok()) }) - .or_else(|| value_to_preview(is_error)) + .or_else(|| value_to_preview(stated_error)) .or_else(|| { // `content` rather than the whole envelope, and bounded: a blob // must not flood the card with base64 and protocol noise. A call @@ -1509,7 +1509,7 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { .and_then(serde_json::Value::as_bool); let claimed_failed = stated_is_error == Some(true) - || is_error.is_some() + || stated_error.is_some() || item .get("status") .and_then(serde_json::Value::as_str) From deb0dc4c9a1062f01e5ac9eb88005cd70db9f14d Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 18:30:21 +0800 Subject: [PATCH 06/15] docs(models): codex does carry a tool status now, from one record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ContentBlock::ToolUse::status` claimed grok was the only parser that could honestly supply a status, and that codex deliberately never does. The semantic MCP path falsifies half of that: a card rebuilt from an `item_completed.McpToolCall` carries that record's own terminal outcome. The warning it was really making still stands and is kept — what must never be copied onto an inner call is the SCRIPT's `ScriptStatus`, which is script-level and would manufacture a permanent spinner. Say that instead of naming codex as a whole. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/models/message.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src-tauri/src/models/message.rs b/src-tauri/src/models/message.rs index 1e020e7104..35bfb91e78 100644 --- a/src-tauri/src/models/message.rs +++ b/src-tauri/src/models/message.rs @@ -116,19 +116,21 @@ pub enum ContentBlock { /// /// OPTIONAL, and `None` means UNKNOWN — never "settled". A reader may /// only act on an affirmative value, so every parser that can't honestly - /// supply one (all of them but grok today) keeps its existing behavior. - /// This exists because absence of output is NOT evidence of liveness: an - /// empty result still writes a `ToolResult`, grok backfills - /// `output_preview` only for non-empty output, and a codex code-mode - /// script that never `text()`s a call settles with none. A viewer - /// polling a RUNNING session's transcript from disk (the grok - /// `spawn_subagent` dialog) has no other way to tell a call that is - /// still working from one that finished. + /// supply one keeps its existing behavior. This exists because absence + /// of output is NOT evidence of liveness: an empty result still writes a + /// `ToolResult`, grok backfills `output_preview` only for non-empty + /// output, and a codex code-mode script that never `text()`s a call + /// settles with none. A viewer polling a RUNNING session's transcript + /// from disk (the grok `spawn_subagent` dialog) has no other way to tell + /// a call that is still working from one that finished. /// - /// Deliberately NOT derived for codex: its `ScriptStatus::Running` is + /// Deliberately NOT derived from codex's `ScriptStatus`: that is /// script-level — a script can still be running after its first inner /// call already completed — so copying it onto recovered inner calls - /// would manufacture a permanent spinner. + /// would manufacture a permanent spinner. The one codex card that does + /// carry a status is an MCP call rebuilt from its OWN semantic + /// `item_completed` record, which states a per-call terminal outcome + /// (`completed` / `failed`) that the wrapper script's status cannot. #[serde(default, skip_serializing_if = "Option::is_none")] status: Option, /// ACP extensibility metadata associated with the tool call. The From 03b05a6f35a3996871152eac52e8b2f03b6eaa8c Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 18:30:21 +0800 Subject: [PATCH 07/15] test(codex): pin the script-status gate on the semantic MCP path A script that threw keeps its own card even when its one MCP call did publish a semantic item, because the wrapper's `Script error:` text is the whole story of that turn and the semantic path discards it. Nothing held that gate down. The count gate cannot stand in for it: a script can throw after its last call already answered, leaving exactly as many items as call sites, so deleting the status check would have passed every other test in the file while silently swallowing the JS error. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 59 ++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 4ca44e77a0..81d53a877c 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -11186,6 +11186,65 @@ mod tests { ); } + /// A script that threw keeps its own card even when its one MCP call did + /// publish a semantic item. The wrapper's `Script error:` text is the whole + /// story of that turn — which line threw, and after which call — and the + /// semantic path DISCARDS it. The count gate cannot stand in for this: a + /// script can throw after its last call answered, leaving exactly as many + /// items as call sites. + #[test] + fn a_thrown_script_keeps_its_own_card_over_a_matching_semantic_item() { + let script = concat!( + "const r=await tools.mcp__codeg_mcp__task_progress({message:m});", + "text(r.content[0].text.toUpperCase());" + ); + let mut lines = code_mode_rollout( + script, + serde_json::json!([ + {"type":"input_text","text":"Script failed\nWall time 0.1 seconds\nOutput:\n"}, + {"type":"input_text","text":"Script error:\nTypeError: Cannot read properties of undefined"}, + ]), + ); + lines.insert( + 2, + rollout_line( + "2026-07-20T08:40:01Z", + "event_msg", + serde_json::json!({ + "type": "item_completed", + "item": { + "type": "McpToolCall", + "id": "exec-progress", + "server": "codeg-mcp", + "tool": "task_progress", + "arguments": {"message":"halfway"}, + "status": "completed", + "result": {"content":[{"type":"text","text":"recorded"}], "isError":false}, + }, + }), + ), + ); + + let detail = parse_lines(&lines, "semantic-mcp-thrown-script"); + assert!( + !tool_uses(&detail) + .iter() + .any(|(id, _, _)| id == "exec-progress"), + "a thrown script must not be replaced by the call that did answer" + ); + let results = tool_results(&detail); + assert_eq!(results.len(), 1, "one card: {results:?}"); + assert!(results[0].2, "a thrown script still renders as an error"); + assert!( + results[0] + .1 + .as_deref() + .is_some_and(|text| text.contains("TypeError")), + "the thrown script's own error must survive: {:?}", + results[0].1 + ); + } + /// The outcome precedence, stated once against the fields themselves rather /// than through four rollouts: a stated failure outranks a stated success, /// a stated success outranks output text that merely reads like a failure, From 4f31bf20e81e87abd84be9eff083d125c0bf0dd1 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 19:12:43 +0800 Subject: [PATCH 08/15] perf(codex): read a semantic MCP preview through a budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two fallbacks that serialize a result — the structured twin, and the `content` array for a call that answered in blocks with no text — called `serde_json::to_string` on the whole value first. For an image-only result that is the entire base64 blob: built in full, then scanned twice by `truncate_str`, to keep four thousand characters of it. Every `McpToolCall` record pays it at capture time, before anything knows whether a script will even accept the correlation. `serialize_preview` writes into a sink that refuses bytes past a budget, which stops the serializer instead of letting it run to the end of its input. UTF-8 spends at most 4 bytes per character, so 4x the character cap always covers the cap, and the partial character a byte cut leaves behind always falls outside the truncation — the output is identical for every input, which is what the equivalence test pins across a value that fits, one landing exactly on the cap, one far past it, and one whose characters are multi-byte. Measured: parsing all 3394 real rollouts on this machine costs the same before and after the whole stack (190.63s vs 190.65s, inside a 0.07% run spread) — but that corpus tops out at a 5.3 KB result, so it cannot exercise this at all. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 127 +++++++++++++++++++++++++++++---- 1 file changed, 113 insertions(+), 14 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 81d53a877c..4e1bb1857c 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1460,6 +1460,54 @@ struct CompletedMcpCall { /// to show what came back, not enough for a base64 blob to swamp the card. const MCP_RESULT_FALLBACK_CAP: usize = 4000; +/// A sink that accepts `budget` bytes and then refuses, so a serializer writing +/// into it stops instead of running to the end of its input. +struct BudgetedSink { + buf: Vec, + budget: usize, +} + +impl std::io::Write for BudgetedSink { + fn write(&mut self, data: &[u8]) -> std::io::Result { + let room = self.budget.saturating_sub(self.buf.len()); + if room == 0 { + return Err(std::io::Error::other("preview budget reached")); + } + let take = room.min(data.len()); + self.buf.extend_from_slice(&data[..take]); + Ok(take) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +/// `value` serialized far enough to fill a `max_chars` preview, and not one +/// byte further. +/// +/// `serde_json::to_string` would materialize the WHOLE value first — for an +/// image-only MCP result that is the entire base64 blob, built and then +/// scanned twice only to keep its first few thousand characters. This is +/// exactly what that produces (UTF-8 spends at most 4 bytes per character, so +/// `4 * max_chars` bytes always cover `max_chars` of them, and the partial +/// character a byte cut can leave at the end always falls outside the +/// truncation) while allocating a bounded buffer instead of an unbounded one. +/// +/// `None` for a value that serializes to nothing at all. +fn serialize_preview(value: &serde_json::Value, max_chars: usize) -> Option { + let mut sink = BudgetedSink { + buf: Vec::new(), + budget: max_chars.saturating_mul(4).saturating_add(1), + }; + // A value that fits reports `Ok`; one that does not aborts with the sink's + // own error. Both leave `buf` holding the prefix, and `serde_json` cannot + // fail on a `Value` for any other reason. + let _ = serde_json::to_writer(&mut sink, value); + let text = String::from_utf8_lossy(&sink.buf); + (!text.is_empty()).then(|| truncate_str(&text, max_chars)) +} + fn completed_mcp_call(payload: &serde_json::Value) -> Option { let item = payload.get("item")?; if item.get("type").and_then(|v| v.as_str()) != Some("McpToolCall") { @@ -1467,33 +1515,34 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { } let result = item.get("result").filter(|value| !value.is_null()); let stated_error = item.get("error").filter(|value| is_stated_error(value)); - // Text first, then the structured twin. The last two are for the shapes - // that carry neither — a transport failure that answered with `error` and - // no result, or a `content` array holding only blocks this reader cannot - // render (an image, a resource). The semantic path DISCARDS the wrapper's - // own printed output, so a `None` here is not a quiet degradation, it is a - // card that says nothing at all where the script card used to show the - // run's text. + // Text first — that is the call's own words, and it is what the script + // card printed, so it is passed through whole. The three below are for the + // shapes carrying no text: a structured-only answer, a transport failure + // that answered with `error` and no result, or a `content` array holding + // only blocks this reader cannot render (an image, a resource). The + // semantic path DISCARDS the wrapper's own printed output, so a `None` here + // is not a quiet degradation, it is a card that says nothing at all where + // the script card used to show the run's text. None of the three is the + // call's own prose, and a serialized one can be arbitrarily large — an + // image block is a base64 blob — so they are read through a budget. let output_preview = result .and_then(|result| result.get("content")) .and_then(crate::parsers::pi::tool_result_content_text) .or_else(|| { result .and_then(|result| result.get("structuredContent")) - .and_then(|value| serde_json::to_string(value).ok()) + .and_then(|value| serialize_preview(value, MCP_RESULT_FALLBACK_CAP)) }) .or_else(|| value_to_preview(stated_error)) .or_else(|| { - // `content` rather than the whole envelope, and bounded: a blob - // must not flood the card with base64 and protocol noise. A call - // that returned NOTHING still says nothing — `{"content":[]}` is - // not worth rendering. + // `content` rather than the whole envelope: a blob must not flood + // the card with protocol noise. A call that returned NOTHING still + // says nothing — `{"content":[]}` is not worth rendering. let content = result?.get("content")?; let carries_blocks = content.as_array().is_some_and(|blocks| !blocks.is_empty()); carries_blocks - .then(|| serde_json::to_string(content).ok()) + .then(|| serialize_preview(content, MCP_RESULT_FALLBACK_CAP)) .flatten() - .map(|text| truncate_str(&text, MCP_RESULT_FALLBACK_CAP)) }); // The record STATES its outcome — `result.isError`, the item's own terminal // `status`, and an `error` when the call never reached the server. Believe @@ -6311,6 +6360,10 @@ mod tests { use super::extract_turn_usage_from_codex_usage; use super::codex_parent_thread_id; use super::completed_mcp_call; + use super::serialize_preview; + use super::truncate_str; + use super::BudgetedSink; + use super::MCP_RESULT_FALLBACK_CAP; use super::is_encrypted_envelope; use super::merge_codex_context_window_stats; use super::native_team_wait_input; @@ -11245,6 +11298,52 @@ mod tests { ); } + /// The sink is what makes a preview bounded: it has to STOP the writer, not + /// grow to fit it. Without the refusal, `serde_json` would keep handing it + /// the rest of a base64 blob. + #[test] + fn a_budgeted_sink_stops_at_its_budget() { + use std::io::Write; + let mut sink = BudgetedSink { + buf: Vec::new(), + budget: 8, + }; + assert!(sink.write_all(&[b'x'; 5]).is_ok(), "room for the first write"); + assert!( + sink.write_all(&[b'x'; 100]).is_err(), + "a write past the budget must fail so serialization aborts" + ); + assert_eq!(sink.buf.len(), 8, "and never buffer more than the budget"); + } + + /// Reading a value through a budget must be INDISTINGUISHABLE from + /// serializing the whole thing and cutting it — otherwise the bound is a + /// behavior change wearing a performance fix's clothes. The cases that can + /// tell them apart: a value that fits, one landing exactly on the cap, one + /// far past it, and one whose characters are multi-byte, where the byte cut + /// lands mid-character and decoding leaves a replacement char behind. + #[test] + fn a_budgeted_preview_reads_exactly_like_an_unbounded_one() { + for (name, value) in [ + ("a small object", serde_json::json!({"a": 1, "b": [true, null]})), + ("empty", serde_json::json!({})), + ("exactly the cap", serde_json::json!("x".repeat(3998))), + ("one past the cap", serde_json::json!("x".repeat(3999))), + ( + "a base64 blob", + serde_json::json!([{"type":"image","mimeType":"image/png","data":"A".repeat(500_000)}]), + ), + ("multi-byte text", serde_json::json!("汉".repeat(6000))), + ] { + let whole = serde_json::to_string(&value).expect("serialize"); + assert_eq!( + serialize_preview(&value, MCP_RESULT_FALLBACK_CAP), + Some(truncate_str(&whole, MCP_RESULT_FALLBACK_CAP)), + "{name}" + ); + } + } + /// The outcome precedence, stated once against the fields themselves rather /// than through four rollouts: a stated failure outranks a stated success, /// a stated success outranks output text that merely reads like a failure, From ae848f3e8c4e2f442566e5fcc8906e59c78526ee Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 19:12:43 +0800 Subject: [PATCH 09/15] docs(models): a record without stated fields yields, not states MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `status`, `result.isError` and `error` are all absent, the outcome is inferred from the result text rather than stated by the record — the precedence test calls that case "a record that states nothing". Say "yields" so the sentence covers every supported case. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/models/message.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src-tauri/src/models/message.rs b/src-tauri/src/models/message.rs index 35bfb91e78..ec0c2e2313 100644 --- a/src-tauri/src/models/message.rs +++ b/src-tauri/src/models/message.rs @@ -129,7 +129,7 @@ pub enum ContentBlock { /// call already completed — so copying it onto recovered inner calls /// would manufacture a permanent spinner. The one codex card that does /// carry a status is an MCP call rebuilt from its OWN semantic - /// `item_completed` record, which states a per-call terminal outcome + /// `item_completed` record, which yields a per-call terminal outcome /// (`completed` / `failed`) that the wrapper script's status cannot. #[serde(default, skip_serializing_if = "Option::is_none")] status: Option, From 1253bea6a0eeb136d99594832db04c9b7fd4e03b Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 19:52:25 +0800 Subject: [PATCH 10/15] fix(codex): a structured answer must not be cut into a success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Budgeting `structuredContent` was wrong. This preview is read TWICE: the card shows it, and `infer_output_text_is_error` re-parses a preview opening with `{` and searches the result for a failed `status`. Cut the JSON and that parse fails silently — so a record stating no `status`, no `isError` and no `error`, whose structured answer says `{…,"status":"failed"}`, settled GREEN. The previous commit introduced that; this restores the whole serialization and pins it with a test that fails the moment a cap goes back on. The budget stays where it belongs: the last fallback, the `content` array that can hold a base64 blob. That branch was already truncated before it was made cheap, so nothing about what it shows changed — and the comment now says which branch is truncated and why the others are not, instead of claiming all of them are. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 68 +++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 4e1bb1857c..aec61e40f1 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1515,29 +1515,35 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { } let result = item.get("result").filter(|value| !value.is_null()); let stated_error = item.get("error").filter(|value| is_stated_error(value)); - // Text first — that is the call's own words, and it is what the script - // card printed, so it is passed through whole. The three below are for the - // shapes carrying no text: a structured-only answer, a transport failure - // that answered with `error` and no result, or a `content` array holding - // only blocks this reader cannot render (an image, a resource). The - // semantic path DISCARDS the wrapper's own printed output, so a `None` here - // is not a quiet degradation, it is a card that says nothing at all where - // the script card used to show the run's text. None of the three is the - // call's own prose, and a serialized one can be arbitrarily large — an - // image block is a base64 blob — so they are read through a budget. + // Text first, then the structured twin. The last two are for the shapes + // that carry neither — a transport failure that answered with `error` and + // no result, or a `content` array holding only blocks this reader cannot + // render (an image, a resource). The semantic path DISCARDS the wrapper's + // own printed output, so a `None` here is not a quiet degradation, it is a + // card that says nothing at all where the script card used to show the + // run's text. + // + // Only the LAST one is truncated, and that asymmetry is deliberate. This + // preview is read twice: once by the card, and once by the error heuristic + // below, which re-parses a preview that opens with `{` or `[` and looks for + // a failed `status` inside it (`infer_output_text_is_error`). Truncating + // valid JSON makes that parse fail, and a structured answer of + // `{…,"status":"failed"}` would then settle GREEN. The last branch already + // accepted that trade before it was made cheap — it is the shape that can + // be a base64 blob, and a card must not be flooded with one. let output_preview = result .and_then(|result| result.get("content")) .and_then(crate::parsers::pi::tool_result_content_text) .or_else(|| { result .and_then(|result| result.get("structuredContent")) - .and_then(|value| serialize_preview(value, MCP_RESULT_FALLBACK_CAP)) + .and_then(|value| serde_json::to_string(value).ok()) }) .or_else(|| value_to_preview(stated_error)) .or_else(|| { - // `content` rather than the whole envelope: a blob must not flood - // the card with protocol noise. A call that returned NOTHING still - // says nothing — `{"content":[]}` is not worth rendering. + // `content` rather than the whole envelope. A call that returned + // NOTHING still says nothing — `{"content":[]}` is not worth + // rendering. let content = result?.get("content")?; let carries_blocks = content.as_array().is_some_and(|blocks| !blocks.is_empty()); carries_blocks @@ -11344,6 +11350,40 @@ mod tests { } } + /// Why the structured answer is the one preview that is NOT truncated: it + /// is read twice. The card shows it, and the error heuristic re-parses it + /// — a preview opening with `{` is parsed back into JSON and searched for + /// a failed `status`. Cut that JSON and the parse fails silently, and a + /// call that reported failure settles GREEN. The padding is what makes the + /// record longer than any cap worth applying, and it sorts before `status` + /// so a cut would take the status with it. + #[test] + fn a_long_structured_failure_is_not_cut_into_a_success() { + let call = completed_mcp_call(&serde_json::json!({ + "item": { + "type": "McpToolCall", "id": "i", "server": "s", "tool": "t", + "result": { + "content": [], + "structuredContent": { + "padding": "p".repeat(MCP_RESULT_FALLBACK_CAP * 2), + "status": "failed", + }, + }, + } + })) + .expect("well-formed item"); + assert!( + call.output_preview + .as_deref() + .is_some_and(|text| serde_json::from_str::(text).is_ok()), + "a structured answer must reach the heuristic still parseable" + ); + assert!( + call.is_error, + "a record that states nothing but reports a failed structured status is a failure" + ); + } + /// The outcome precedence, stated once against the fields themselves rather /// than through four rollouts: a stated failure outranks a stated success, /// a stated success outranks output text that merely reads like a failure, From fa5273b7876d11a2200250c908031a7fa317ea1a Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 20:27:26 +0800 Subject: [PATCH 11/15] fix(codex): truncate what a card shows, never what decides its outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The block fallback is the one preview that is cut, and its outcome was still being read back out of the cut string. Same failure as the structured branch: `infer_output_text_is_error` re-parses a preview opening with `[`, a truncated document does not parse, and a result whose blocks reported a failure settled GREEN. Capping the base64 a card displays is right; letting that cap decide the call is not. So the fallback now hands back the blocks it cut, and the outcome is inferred from those. That is exactly what parsing an untruncated preview would have produced — the same `infer_output_value_is_error` over the same value — so the cap costs the card characters and never costs the call its verdict. Verified by removing the arm and watching the new test fail. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 76 +++++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 20 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index aec61e40f1..0153a9ef07 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1523,14 +1523,14 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { // card that says nothing at all where the script card used to show the // run's text. // - // Only the LAST one is truncated, and that asymmetry is deliberate. This - // preview is read twice: once by the card, and once by the error heuristic - // below, which re-parses a preview that opens with `{` or `[` and looks for - // a failed `status` inside it (`infer_output_text_is_error`). Truncating - // valid JSON makes that parse fail, and a structured answer of - // `{…,"status":"failed"}` would then settle GREEN. The last branch already - // accepted that trade before it was made cheap — it is the shape that can - // be a base64 blob, and a card must not be flooded with one. + // Only the LAST one is truncated, and only for what the card SHOWS — it is + // the shape that can be a base64 blob, and a card must not be flooded with + // one. Nothing may decide an OUTCOME from a cut string: the heuristic below + // re-parses a preview that opens with `{` or `[` and looks for a failed + // `status` inside it (`infer_output_text_is_error`), and truncating valid + // JSON makes that parse fail silently, settling a call that reported + // failure GREEN. So the cut branch also hands back the value it cut, and + // the outcome is read from that instead. let output_preview = result .and_then(|result| result.get("content")) .and_then(crate::parsers::pi::tool_result_content_text) @@ -1539,17 +1539,16 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { .and_then(|result| result.get("structuredContent")) .and_then(|value| serde_json::to_string(value).ok()) }) - .or_else(|| value_to_preview(stated_error)) - .or_else(|| { - // `content` rather than the whole envelope. A call that returned - // NOTHING still says nothing — `{"content":[]}` is not worth - // rendering. - let content = result?.get("content")?; - let carries_blocks = content.as_array().is_some_and(|blocks| !blocks.is_empty()); - carries_blocks - .then(|| serialize_preview(content, MCP_RESULT_FALLBACK_CAP)) - .flatten() - }); + .or_else(|| value_to_preview(stated_error)); + // `content` rather than the whole envelope. A call that returned NOTHING + // still says nothing — `{"content":[]}` is not worth rendering. + let blocks = output_preview + .is_none() + .then(|| result?.get("content")) + .flatten() + .filter(|content| content.as_array().is_some_and(|blocks| !blocks.is_empty())); + let output_preview = output_preview + .or_else(|| blocks.and_then(|content| serialize_preview(content, MCP_RESULT_FALLBACK_CAP))); // The record STATES its outcome — `result.isError`, the item's own terminal // `status`, and an `error` when the call never reached the server. Believe // them. `infer_tool_call_output_is_error` reads tea leaves out of the @@ -1578,7 +1577,12 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { input_preview: value_to_preview(item.get("arguments")), is_error: claimed_failed || (!claimed_ok - && infer_tool_call_output_is_error(item, result, output_preview.as_deref())), + && (infer_tool_call_output_is_error(item, result, output_preview.as_deref()) + // The blocks the preview above was cut out of. Reading them + // directly is exactly what parsing an UNTRUNCATED preview + // would have produced, so the cap costs the card characters + // and never costs the call its outcome. + || blocks.is_some_and(|content| infer_output_value_is_error(content, 0)))), output_preview, }) } @@ -11350,6 +11354,38 @@ mod tests { } } + /// The block fallback IS truncated, so its outcome must not be read back + /// out of the cut string — the blocks themselves decide. Same failure as + /// the structured case: parse a truncated document and you get nothing, + /// and nothing reads as success. + #[test] + fn a_long_block_failure_survives_its_own_truncation() { + let call = completed_mcp_call(&serde_json::json!({ + "item": { + "type": "McpToolCall", "id": "i", "server": "s", "tool": "t", + "result": { + "content": [{ + "type": "resource", + "padding": "p".repeat(MCP_RESULT_FALLBACK_CAP * 2), + "status": "failed", + }], + }, + } + })) + .expect("well-formed item"); + assert!( + call.output_preview + .as_deref() + .is_some_and(|text| text.ends_with("...")), + "the card's copy is still cut: {:?}", + call.output_preview.as_deref().map(str::len) + ); + assert!( + call.is_error, + "a failure reported inside the blocks survives the cut" + ); + } + /// Why the structured answer is the one preview that is NOT truncated: it /// is read twice. The card shows it, and the error heuristic re-parses it /// — a preview opening with `{` is parsed back into JSON and searched for From 1edca458dca25719f081d31392ab528d170cf051 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 21:05:53 +0800 Subject: [PATCH 12/15] fix(codex): a block's payload is bytes, never a verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit read a cut result's outcome off the blocks instead of the cut string — right, but through a full walk, and `infer_output_value_is_error` follows `data`. In a tool-output envelope `data` is a nested result; on an MCP content block it is the PAYLOAD. So an outcome-less image result walked straight into the base64 the cap was added to avoid touching, and `infer_output_text_is_error` lowercased it into a second copy of itself — the bounded preview undone by the inference standing next to it. `blocks_report_failure` reads each block's OWN keys and refuses every descent. That is all a block can honestly report: a marker is a key on the block, a payload is bytes. Pinned by a test with a payload past the cap that reads like an error, which fails against the full walk. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 63 +++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 0153a9ef07..097217a65c 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1578,15 +1578,35 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { is_error: claimed_failed || (!claimed_ok && (infer_tool_call_output_is_error(item, result, output_preview.as_deref()) - // The blocks the preview above was cut out of. Reading them - // directly is exactly what parsing an UNTRUNCATED preview - // would have produced, so the cap costs the card characters - // and never costs the call its outcome. - || blocks.is_some_and(|content| infer_output_value_is_error(content, 0)))), + || blocks_report_failure(blocks))), output_preview, }) } +/// Whether any block in a result's `content` array REPORTS a failure. +/// +/// The blocks are what the preview above was cut out of, and the cut string +/// no longer re-parses, so the outcome has to be read here or not at all — +/// truncation may cost a card characters, never a call its verdict. +/// +/// Own keys only, deliberately. A full `infer_output_value_is_error` walk +/// follows `data`, which in a tool-output envelope is a nested result but on +/// an MCP block is the PAYLOAD — the base64 the cap above exists to avoid +/// touching, and which `infer_output_text_is_error` would lowercase into a +/// second copy of itself. A payload that happens to read like an error is +/// still just bytes. Depth 4 is how that is said to a walker whose own limit +/// is 4: every own key is read, every descent refuses. +fn blocks_report_failure(blocks: Option<&serde_json::Value>) -> bool { + blocks + .and_then(serde_json::Value::as_array) + .is_some_and(|blocks| { + blocks + .iter() + .filter(|block| block.is_object()) + .any(|block| infer_output_value_is_error(block, 4)) + }) +} + fn unwrap_completed_mcp_calls( script: &CodeModeScript, completed: Vec, @@ -11354,6 +11374,39 @@ mod tests { } } + /// The marker search reads a block's OWN keys and stops there. Walking on + /// into `data` would read the PAYLOAD — the base64 the cap exists to avoid + /// touching, and which the text heuristic lowercases into a second copy of + /// itself. This is the case that arm was added for, a payload too long to + /// keep, and it must come back cheap and quiet: a payload that happens to + /// read like an error is still just bytes. + /// + /// (Under the cap nothing is cut, so the ordinary preview path parses the + /// whole thing exactly as it always has — that is not this arm's business.) + #[test] + fn an_oversized_block_payload_is_never_read_as_an_outcome() { + let call = completed_mcp_call(&serde_json::json!({ + "item": { + "type": "McpToolCall", "id": "i", "server": "s", "tool": "t", + "result": { + "content": [{ + "type": "image", + "mimeType": "image/png", + "data": format!("error: {}", "A".repeat(MCP_RESULT_FALLBACK_CAP * 2)), + }], + }, + } + })) + .expect("well-formed item"); + assert!( + call.output_preview + .as_deref() + .is_some_and(|text| text.ends_with("...")), + "the payload is past the cap, so the preview is cut" + ); + assert!(!call.is_error, "a payload is bytes, not a verdict"); + } + /// The block fallback IS truncated, so its outcome must not be read back /// out of the cut string — the blocks themselves decide. Same failure as /// the structured case: parse a truncated document and you get nothing, From d11fd95b0ba3bdaa2d28704bd4105a8a93e24b17 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 21:36:15 +0800 Subject: [PATCH 13/15] docs(codex): say what the budget bounds, and what it does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three claims around the semantic preview promised more than the code delivers. `serialize_preview` said "not one byte further", but `serde_json` walks a string looking for escapes before offering any of it to the writer, so one oversized string is still read through once. `blocks_report_failure` said the cap "avoids touching" the payload when what it avoids is COPYING it, and said "every own key" when it reads the seven outcome fields the walker recognizes. Its depth-4 stop is also not free in every shape — a recognized field can itself be megabytes, and a `trim` on one costs a pass. What is bounded is MEMORY, which is the part that can fail; a pass over bytes already resident is not a second copy of them, and stopping even that would mean replacing the serializer. Say that, rather than leaving a reader to discover it the way this review did. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 58 +++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index 097217a65c..efc21ac4e8 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1483,16 +1483,22 @@ impl std::io::Write for BudgetedSink { } } -/// `value` serialized far enough to fill a `max_chars` preview, and not one -/// byte further. +/// `value` serialized into a `max_chars` preview without ever building the +/// whole serialization. /// -/// `serde_json::to_string` would materialize the WHOLE value first — for an -/// image-only MCP result that is the entire base64 blob, built and then -/// scanned twice only to keep its first few thousand characters. This is -/// exactly what that produces (UTF-8 spends at most 4 bytes per character, so -/// `4 * max_chars` bytes always cover `max_chars` of them, and the partial -/// character a byte cut can leave at the end always falls outside the -/// truncation) while allocating a bounded buffer instead of an unbounded one. +/// `serde_json::to_string` materializes all of it first — for an image-only +/// MCP result that is the entire base64 blob, allocated and then walked twice +/// more by `truncate_str`, to keep a few thousand characters of it. This +/// produces exactly the same string while the buffer stays at `4 * max_chars` +/// bytes: UTF-8 spends at most 4 bytes per character, so that always covers +/// `max_chars` of them, and the partial character a byte cut can leave behind +/// always falls outside the truncation. +/// +/// What it bounds is the MEMORY, which is the part that can fail. Time is only +/// mostly bounded: `serde_json` walks a string looking for escapes before +/// offering any of it to the writer, so one oversized string is still read +/// through once — a pass over bytes already resident, not a second copy of +/// them. Stopping even that would mean replacing the serializer. /// /// `None` for a value that serializes to nothing at all. fn serialize_preview(value: &serde_json::Value, max_chars: usize) -> Option { @@ -1589,13 +1595,21 @@ fn completed_mcp_call(payload: &serde_json::Value) -> Option { /// no longer re-parses, so the outcome has to be read here or not at all — /// truncation may cost a card characters, never a call its verdict. /// -/// Own keys only, deliberately. A full `infer_output_value_is_error` walk -/// follows `data`, which in a tool-output envelope is a nested result but on -/// an MCP block is the PAYLOAD — the base64 the cap above exists to avoid -/// touching, and which `infer_output_text_is_error` would lowercase into a -/// second copy of itself. A payload that happens to read like an error is -/// still just bytes. Depth 4 is how that is said to a walker whose own limit -/// is 4: every own key is read, every descent refuses. +/// A block's own report, deliberately, and no descent. A full +/// `infer_output_value_is_error` walk follows `data`, which in a tool-output +/// envelope is a nested result but on an MCP block is the PAYLOAD — the base64 +/// the cap above refuses to copy, and which `infer_output_text_is_error` would +/// lowercase into a second copy of itself anyway. A payload that happens to +/// read like an error is still just bytes. Depth 4 is how that is said to a +/// walker whose own limit is 4: it reads the outcome fields it recognizes and +/// then every descent refuses. Only OBJECT blocks are asked, because only they +/// can carry such a field — MCP `content` holds typed blocks, and a bare +/// string among them is not a shape this can read a verdict out of. +/// +/// Still not free in the worst case: a recognized field can itself be huge +/// (`{"stderr": ""}` costs a `trim`). That is a pass over +/// one already-resident string, not a copy of it, and unlike `data` it is a +/// field a block would have to have gone out of its way to carry. fn blocks_report_failure(blocks: Option<&serde_json::Value>) -> bool { blocks .and_then(serde_json::Value::as_array) @@ -11374,12 +11388,12 @@ mod tests { } } - /// The marker search reads a block's OWN keys and stops there. Walking on - /// into `data` would read the PAYLOAD — the base64 the cap exists to avoid - /// touching, and which the text heuristic lowercases into a second copy of - /// itself. This is the case that arm was added for, a payload too long to - /// keep, and it must come back cheap and quiet: a payload that happens to - /// read like an error is still just bytes. + /// The marker search reads a block's own outcome fields and stops there. + /// Walking on into `data` would read the PAYLOAD — the base64 the cap + /// refuses to copy, which the text heuristic would then lowercase into a + /// second copy of itself. This pins the VERDICT that follows from that (a + /// payload reading like an error is still just bytes); what it cannot see + /// is the cost, so the reasoning lives on `blocks_report_failure`. /// /// (Under the cap nothing is cut, so the ordinary preview path parses the /// whole thing exactly as it always has — that is not this arm's business.) From e59beea3dbb66f3a55941d75deade98e40670875 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 22:09:00 +0800 Subject: [PATCH 14/15] docs(codex): name the `+ 1` in the preview budget for what it does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sizing claims on `serialize_preview` were loose. "Without ever building the whole serialization" reads as if a small value were also streamed, when a value that fits is written out whole — into a buffer that just cannot grow. And the budget was written `4 * max_chars` when it is `4 * max_chars + 1`, which is not a rounding detail: the `+ 1` is what makes a full buffer decode to STRICTLY more than `max_chars` characters, and strictness is the entire equivalence proof. At `4 * max_chars` the property would still hold for JSON, because JSON always opens with an ASCII byte and so cannot land a full buffer on exactly `max_chars` — but that is a fact about the format, not about this function, and nothing here says so. The `+ 1` makes it arithmetic. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index efc21ac4e8..ab2686f977 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1483,16 +1483,23 @@ impl std::io::Write for BudgetedSink { } } -/// `value` serialized into a `max_chars` preview without ever building the -/// whole serialization. +/// `value` serialized into a `max_chars` preview without ever building an +/// UNBOUNDED serialization. A value that fits is still written out whole — +/// into a buffer that cannot grow past the budget. /// /// `serde_json::to_string` materializes all of it first — for an image-only /// MCP result that is the entire base64 blob, allocated and then walked twice /// more by `truncate_str`, to keep a few thousand characters of it. This -/// produces exactly the same string while the buffer stays at `4 * max_chars` -/// bytes: UTF-8 spends at most 4 bytes per character, so that always covers -/// `max_chars` of them, and the partial character a byte cut can leave behind -/// always falls outside the truncation. +/// produces exactly the same string while the buffer stays at +/// `4 * max_chars + 1` bytes. UTF-8 spends at most 4 bytes per character, so +/// that many always cover `max_chars` of them — and the `+ 1` is what makes it +/// STRICTLY more, which is the whole proof: a full buffer therefore always +/// decodes to more than `max_chars` characters, so it is always truncated, so +/// the partial character a byte cut leaves behind is always among the ones +/// dropped. At `4 * max_chars` alone the strictness would rest on JSON always +/// opening with an ASCII byte and so never letting a full buffer land on +/// exactly `max_chars` — true, but a fact about the format rather than about +/// this function. The `+ 1` is what makes it a property of the arithmetic. /// /// What it bounds is the MEMORY, which is the part that can fail. Time is only /// mostly bounded: `serde_json` walks a string looking for escapes before From 31bc552ed309ddab68131ae62ede4f20f5a2c516 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Thu, 10 Sep 2026 22:29:45 +0800 Subject: [PATCH 15/15] docs(codex): the ASCII-opening argument has a zero-length exception `max_chars = 0` is the one case where a hypothetical `4 * max_chars` budget would be an EMPTY buffer, which decodes to exactly zero characters without ever reaching the opening ASCII byte the argument leans on. The parenthetical said "true" flatly. It is true for every positive `max_chars`, which is what it now says. Co-Authored-By: Claude Opus 5 (1M context) --- src-tauri/src/parsers/codex.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src-tauri/src/parsers/codex.rs b/src-tauri/src/parsers/codex.rs index ab2686f977..eedb7f08cd 100644 --- a/src-tauri/src/parsers/codex.rs +++ b/src-tauri/src/parsers/codex.rs @@ -1498,8 +1498,9 @@ impl std::io::Write for BudgetedSink { /// the partial character a byte cut leaves behind is always among the ones /// dropped. At `4 * max_chars` alone the strictness would rest on JSON always /// opening with an ASCII byte and so never letting a full buffer land on -/// exactly `max_chars` — true, but a fact about the format rather than about -/// this function. The `+ 1` is what makes it a property of the arithmetic. +/// exactly `max_chars` — which holds for every `max_chars` but zero, and even +/// then is a fact about the format rather than about this function. The `+ 1` +/// is what makes it a property of the arithmetic. /// /// What it bounds is the MEMORY, which is the part that can fail. Time is only /// mostly bounded: `serde_json` walks a string looking for escapes before