Reduce retained MCP result data in TUI history (#39023)

## Why MCP tool results can contain multi-megabyte image, audio, or resource bodies, even though TUI history renders only summaries for those content types. ## What changed - Project completed MCP results into width-independent display data, dropping media and resource bodies that history does not render. - Preserve text output, JSON fallbacks, error state, Node REPL rendering, and the fully decoded image-output marker. - Add tests covering width-dependent rendering, image validation, and Node REPL text fields on non-text and unknown content blocks. GitOrigin-RevId: 2cfddf0864093e6837a4431e541f7a9cab544a68
2026-10-30 19:18:49 +00:00 · 2026-08-17 16:46:26 +00:00
parent 0841d9bba5
commit f0904922f6
4 changed files with 353 additions and 121 deletions
@@ -2,6 +2,16 @@

 use super::*;

+#[path = "mcp_result.rs"]
+mod result;
+
+use result::McpResultKind;
+use result::McpToolResult;
+
+#[cfg(test)]
+#[path = "mcp_tests.rs"]
+mod tests;
+
 #[derive(Debug)]
 struct McpImageOutputCell;

@@ -29,7 +39,7 @@ pub(crate) struct McpToolCallCell {
    invocation: McpInvocation,
    start_time: Instant,
    duration: Option<Duration>,
-    result: Option<Result<codex_protocol::mcp::CallToolResult, String>>,
+    result: Option<Result<McpToolResult, String>>,
    animations_enabled: bool,
 }

@@ -79,8 +89,12 @@ impl McpToolCallCell {
        duration: Duration,
        result: Result<codex_protocol::mcp::CallToolResult, String>,
    ) -> Option<Box<dyn HistoryCell>> {
-        let image_cell = try_new_completed_mcp_tool_call_with_image_output(&result)
-            .map(|cell| Box::new(cell) as Box<dyn HistoryCell>);
+        let result = result.map(|result| McpToolResult::new(result, self.result_kind()));
+        let image_cell = result
+            .as_ref()
+            .ok()
+            .filter(|result| result.has_image)
+            .map(|_| Box::new(McpImageOutputCell) as Box<dyn HistoryCell>);
        self.duration = Some(duration);
        self.result = Some(result);
        image_cell
@@ -88,7 +102,7 @@ impl McpToolCallCell {

    fn success(&self) -> Option<bool> {
        match self.result.as_ref() {
-            Some(Ok(result)) => Some(!result.is_error.unwrap_or(false)),
+            Some(Ok(result)) => Some(!result.is_error),
            Some(Err(_)) => Some(false),
            None => None,
        }
@@ -100,40 +114,18 @@ impl McpToolCallCell {
        self.result = Some(Err("interrupted".to_string()));
    }

-    fn render_content_block(block: &serde_json::Value, width: usize) -> String {
-        let content = match serde_json::from_value::<rmcp::model::ContentBlock>(block.clone()) {
-            Ok(content) => content,
-            Err(_) => {
-                return format_and_truncate_tool_result(
-                    &block.to_string(),
-                    TOOL_CALL_MAX_LINES,
-                    width,
-                );
-            }
-        };
-
-        match content {
-            rmcp::model::ContentBlock::Text(text) => {
-                format_and_truncate_tool_result(&text.text, TOOL_CALL_MAX_LINES, width)
-            }
-            rmcp::model::ContentBlock::Image(_) => "<image content>".to_string(),
-            rmcp::model::ContentBlock::Audio(_) => "<audio content>".to_string(),
-            rmcp::model::ContentBlock::Resource(resource) => {
-                let uri = match resource.resource {
-                    rmcp::model::ResourceContents::TextResourceContents { uri, .. } => uri,
-                    rmcp::model::ResourceContents::BlobResourceContents { uri, .. } => uri,
-                    _ => return "<unknown embedded resource>".to_string(),
-                };
-                format!("embedded resource: {uri}")
-            }
-            rmcp::model::ContentBlock::ResourceLink(link) => format!("link: {}", link.uri),
-            _ => format_and_truncate_tool_result(&block.to_string(), TOOL_CALL_MAX_LINES, width),
+    fn result_kind(&self) -> McpResultKind {
+        if self.invocation.server == "node_repl" && self.invocation.tool == "js" {
+            McpResultKind::NodeRepl
+        } else {
+            McpResultKind::Standard
        }
    }
+
    fn render_lines(&self, width: u16, mode: McpToolCallRenderMode) -> Vec<Line<'static>> {
        let mut lines: Vec<Line<'static>> = Vec::new();
        let status = self.success();
-        let node_repl = self.invocation.server == "node_repl" && self.invocation.tool == "js";
+        let node_repl = self.result_kind() == McpResultKind::NodeRepl;
        let compact = node_repl && mode == McpToolCallRenderMode::Display;
        let bullet = match status {
            Some(true) => "•".green().bold(),
@@ -194,24 +186,21 @@ impl McpToolCallCell {

        if let Some(result) = &self.result {
            match result {
-                Ok(codex_protocol::mcp::CallToolResult { content, .. }) => {
+                Ok(McpToolResult { content, .. }) => {
                    if !content.is_empty() {
                        for block in content {
                            let text = if compact && status == Some(true) {
-                                let meaningful_output = block
-                                    .get("text")
-                                    .and_then(serde_json::Value::as_str)
-                                    .and_then(|text| {
-                                        if text.starts_with("Script completed\n") {
-                                            return text
-                                                .split_once("\nOutput:\n")
-                                                .map(|(_, output)| output.to_string());
-                                        }
-                                        serde_json::from_str::<NodeReplExecOutput>(text)
-                                            .ok()
-                                            .filter(|output| output.exit_code == 0)
-                                            .map(|output| output.output)
-                                    });
+                                let meaningful_output = block.text().and_then(|text| {
+                                    if text.starts_with("Script completed\n") {
+                                        return text
+                                            .split_once("\nOutput:\n")
+                                            .map(|(_, output)| output.to_string());
+                                    }
+                                    serde_json::from_str::<NodeReplExecOutput>(text)
+                                        .ok()
+                                        .filter(|output| output.exit_code == 0)
+                                        .map(|output| output.output)
+                                });
                                match meaningful_output {
                                    Some(output) if output.is_empty() => continue,
                                    Some(output) => format_and_truncate_tool_result(
@@ -219,16 +208,15 @@ impl McpToolCallCell {
                                        TOOL_CALL_MAX_LINES,
                                        detail_wrap_width,
                                    ),
-                                    None => Self::render_content_block(block, detail_wrap_width),
+                                    None => block.render(detail_wrap_width),
                                }
                            } else if node_repl
                                && mode == McpToolCallRenderMode::Transcript
-                                && let Some(output) =
-                                    block.get("text").and_then(serde_json::Value::as_str)
+                                && let Some(output) = block.text()
                            {
                                output.trim_end_matches('\n').to_string()
                            } else {
-                                Self::render_content_block(block, detail_wrap_width)
+                                block.render(detail_wrap_width)
                            };
                            for segment in text.split('\n') {
                                let line = Line::from(segment.to_string().dim());
@@ -301,9 +289,9 @@ impl HistoryCell for McpToolCallCell {

        if let Some(result) = &self.result {
            match result {
-                Ok(codex_protocol::mcp::CallToolResult { content, .. }) => {
+                Ok(McpToolResult { content, .. }) => {
                    for block in content {
-                        let text = Self::render_content_block(block, RAW_TOOL_OUTPUT_WIDTH);
+                        let text = block.render(RAW_TOOL_OUTPUT_WIDTH);
                        lines.extend(raw_lines_from_source(&text));
                    }
                }
@@ -329,68 +317,6 @@ pub(crate) fn new_active_mcp_tool_call(
 ) -> McpToolCallCell {
    McpToolCallCell::new(call_id, invocation, animations_enabled)
 }
-/// Returns an additional history cell if an MCP tool result includes a decodable image.
-///
-/// This intentionally returns at most one cell: the first image in `CallToolResult.content` that
-/// successfully base64-decodes and parses as an image. This is used as a lightweight “image output
-/// exists” affordance separate from the main MCP tool call cell.
-///
-/// Manual testing tip:
-/// - Run the rmcp stdio test server (`codex-rs/rmcp-client/src/bin/test_stdio_server.rs`) and
-///   register it as an MCP server via `codex mcp add`.
-/// - Use its `image_scenario` tool with cases like `text_then_image`,
-///   `invalid_base64_then_image`, or `invalid_image_bytes_then_image` to ensure this path triggers
-///   even when the first block is not a valid image.
-fn try_new_completed_mcp_tool_call_with_image_output(
-    result: &Result<codex_protocol::mcp::CallToolResult, String>,
-) -> Option<McpImageOutputCell> {
-    result
-        .as_ref()
-        .ok()?
-        .content
-        .iter()
-        .find_map(decode_mcp_image)?;
-
-    Some(McpImageOutputCell)
-}
-
-/// Decodes an MCP `ImageContent` block into an in-memory image.
-///
-/// Returns `None` when the block is not an image, when base64 decoding fails, when the format
-/// cannot be inferred, or when the image decoder rejects the bytes.
-fn decode_mcp_image(block: &serde_json::Value) -> Option<DynamicImage> {
-    let content = serde_json::from_value::<rmcp::model::ContentBlock>(block.clone()).ok()?;
-    let rmcp::model::ContentBlock::Image(image) = content else {
-        return None;
-    };
-    let base64_data = if let Some(data_url) = image.data.strip_prefix("data:") {
-        data_url.split_once(',')?.1
-    } else {
-        image.data.as_str()
-    };
-    let raw_data = base64::engine::general_purpose::STANDARD
-        .decode(base64_data)
-        .map_err(|e| {
-            error!("Failed to decode image data: {e}");
-            e
-        })
-        .ok()?;
-    let reader = ImageReader::new(Cursor::new(raw_data))
-        .with_guessed_format()
-        .map_err(|e| {
-            error!("Failed to guess image format: {e}");
-            e
-        })
-        .ok()?;
-
-    reader
-        .decode()
-        .map_err(|e| {
-            error!("Image decoding failed: {e}");
-            e
-        })
-        .ok()
-}
 /// Render a summary of configured MCP servers from the current `Config`.
 pub(crate) fn empty_mcp_output() -> WebHyperlinkHistoryCell {
    let mut docs_line = HyperlinkLine::new(Line::from("    See the "));
@@ -0,0 +1,169 @@
+//! Width-independent display content retained after an MCP call completes.
+//!
+//! The wire result can contain multi-megabyte image, audio, or resource bodies. Validate each
+//! block once with the MCP model, then retain only what history rendering actually displays.
+
+use crate::exec_cell::TOOL_CALL_MAX_LINES;
+use crate::text_formatting::format_and_truncate_tool_result;
+use base64::Engine;
+use codex_protocol::mcp::CallToolResult;
+use image::DynamicImage;
+use image::ImageReader;
+use rmcp::model::ContentBlock;
+use rmcp::model::ResourceContents;
+use serde::Deserialize;
+use std::borrow::Cow;
+use std::io::Cursor;
+use tracing::error;
+
+#[derive(Clone, Copy, Eq, PartialEq)]
+pub(super) enum McpResultKind {
+    Standard,
+    NodeRepl,
+}
+
+#[derive(Debug)]
+pub(super) struct McpToolResult {
+    pub(super) content: Vec<McpContentBlock>,
+    pub(super) is_error: bool,
+    pub(super) has_image: bool,
+}
+
+#[derive(Debug)]
+pub(super) struct McpContentBlock {
+    display: McpContentDisplay,
+    /// Code mode uses a top-level `text` field even on malformed or non-text blocks.
+    original_text: Option<String>,
+}
+
+#[derive(Debug)]
+enum McpContentDisplay {
+    Text(String),
+    Summary(Cow<'static, str>),
+    Json(String),
+}
+
+impl McpToolResult {
+    /// Consumes a wire result, dropping bodies represented only by media or resource summaries.
+    ///
+    /// Canonical MCP deserialization preserves full text and the exact JSON fallback for malformed
+    /// or unknown blocks. Every block is projected, but image decoding stops at the first fully
+    /// valid image.
+    pub(super) fn new(result: CallToolResult, kind: McpResultKind) -> Self {
+        let mut has_image = false;
+        let content = result
+            .content
+            .into_iter()
+            .map(|block| {
+                // Deserialize by reference so malformed blocks remain available for the exact
+                // JSON fallback. Successful blocks no longer retain their wire representation.
+                let parsed = ContentBlock::deserialize(&block);
+                let original_text = match (&parsed, kind) {
+                    (Ok(ContentBlock::Text(_)), _) | (_, McpResultKind::Standard) => None,
+                    (_, McpResultKind::NodeRepl) => block
+                        .get("text")
+                        .and_then(serde_json::Value::as_str)
+                        .map(str::to_owned),
+                };
+                let display = match parsed {
+                    Ok(ContentBlock::Text(text)) => McpContentDisplay::Text(text.text),
+                    Ok(ContentBlock::Image(image)) => {
+                        // Keep the marker's existing full-decode validity check, and stop
+                        // decoding after the first valid image, just as the old search did.
+                        if !has_image {
+                            has_image = decode_mcp_image(&image.data).is_some();
+                        }
+                        McpContentDisplay::Summary("<image content>".into())
+                    }
+                    Ok(ContentBlock::Audio(_)) => {
+                        McpContentDisplay::Summary("<audio content>".into())
+                    }
+                    Ok(ContentBlock::Resource(resource)) => {
+                        let summary = match resource.resource {
+                            ResourceContents::TextResourceContents { uri, .. }
+                            | ResourceContents::BlobResourceContents { uri, .. } => {
+                                format!("embedded resource: {uri}").into()
+                            }
+                            _ => "<unknown embedded resource>".into(),
+                        };
+                        McpContentDisplay::Summary(summary)
+                    }
+                    Ok(ContentBlock::ResourceLink(link)) => {
+                        McpContentDisplay::Summary(format!("link: {}", link.uri).into())
+                    }
+                    Ok(_) | Err(_) => McpContentDisplay::Json(block.to_string()),
+                };
+                McpContentBlock {
+                    display,
+                    original_text,
+                }
+            })
+            .collect();
+
+        Self {
+            content,
+            is_error: result.is_error.unwrap_or(false),
+            has_image,
+        }
+    }
+}
+
+impl McpContentBlock {
+    /// Returns the untruncated top-level text used by node_repl's compact and transcript views.
+    ///
+    /// Valid text blocks reuse their display storage. Only node_repl retains this extra field on
+    /// malformed or non-text blocks, where it takes precedence over the usual display content.
+    pub(super) fn text(&self) -> Option<&str> {
+        match &self.display {
+            McpContentDisplay::Text(text) => Some(text),
+            McpContentDisplay::Summary(_) | McpContentDisplay::Json(_) => {
+                self.original_text.as_deref()
+            }
+        }
+    }
+
+    /// Applies width-dependent formatting to full text or fallback JSON without reparsing the MCP
+    /// block. Media and resource summaries retain their existing untruncated display form.
+    pub(super) fn render(&self, width: usize) -> String {
+        match &self.display {
+            McpContentDisplay::Text(text) | McpContentDisplay::Json(text) => {
+                format_and_truncate_tool_result(text, TOOL_CALL_MAX_LINES, width)
+            }
+            McpContentDisplay::Summary(summary) => summary.to_string(),
+        }
+    }
+}
+
+/// Fully decodes an MCP image before exposing the separate image-output marker.
+///
+/// A header-only check would accept images whose decoder rejects their pixel data. Preserve the
+/// existing behavior for invalid base64, unknown formats, corrupt images, and data URLs.
+fn decode_mcp_image(data: &str) -> Option<DynamicImage> {
+    let base64_data = if let Some(data_url) = data.strip_prefix("data:") {
+        data_url.split_once(',')?.1
+    } else {
+        data
+    };
+    let raw_data = base64::engine::general_purpose::STANDARD
+        .decode(base64_data)
+        .map_err(|e| {
+            error!("Failed to decode image data: {e}");
+            e
+        })
+        .ok()?;
+    let reader = ImageReader::new(Cursor::new(raw_data))
+        .with_guessed_format()
+        .map_err(|e| {
+            error!("Failed to guess image format: {e}");
+            e
+        })
+        .ok()?;
+
+    reader
+        .decode()
+        .map_err(|e| {
+            error!("Image decoding failed: {e}");
+            e
+        })
+        .ok()
+}
@@ -0,0 +1,142 @@
+use super::*;
+use base64::Engine;
+use codex_protocol::mcp::CallToolResult;
+use pretty_assertions::assert_eq;
+use serde_json::Value;
+use serde_json::json;
+
+const PNG: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==";
+
+fn result(content: Vec<Value>) -> CallToolResult {
+    CallToolResult {
+        content,
+        structured_content: None,
+        is_error: None,
+        meta: None,
+    }
+}
+
+#[test]
+fn projected_content_preserves_width_dependent_rendering() {
+    let text = "{\"result\": [1, 2, 3], \"text\": \"long output 🦀\"}";
+    let malformed = json!({"type": "image", "data": PNG});
+    let invalid_metadata =
+        json!({"type": "text", "text": "not a valid block", "annotations": {"priority": "high"}});
+    let unknown = json!({"type": "future_block", "text": "unknown output 🦀"});
+    let projected = McpToolResult::new(
+        result(vec![
+            json!({"type": "text", "text": text}),
+            json!({"type": "image", "mimeType": "image/png", "data": PNG}),
+            json!({"type": "audio", "mimeType": "audio/wav", "data": "audio data"}),
+            json!({"type": "resource", "resource": {"uri": "file:///text.txt", "text": "resource body"}}),
+            json!({"type": "resource", "resource": {"uri": "file:///blob.bin", "blob": "binary data"}}),
+            json!({"type": "resource_link", "uri": "file:///linked.txt", "name": "linked"}),
+            malformed.clone(),
+            invalid_metadata.clone(),
+            unknown.clone(),
+        ]),
+        McpResultKind::Standard,
+    );
+
+    for width in [1, 8, 40, 120, RAW_TOOL_OUTPUT_WIDTH] {
+        let format_text =
+            |text: &str| format_and_truncate_tool_result(text, TOOL_CALL_MAX_LINES, width);
+        assert_eq!(
+            projected
+                .content
+                .iter()
+                .map(|block| block.render(width))
+                .collect::<Vec<_>>(),
+            vec![
+                format_text(text),
+                "<image content>".to_string(),
+                "<audio content>".to_string(),
+                "embedded resource: file:///text.txt".to_string(),
+                "embedded resource: file:///blob.bin".to_string(),
+                "link: file:///linked.txt".to_string(),
+                format_text(&malformed.to_string()),
+                format_text(&invalid_metadata.to_string()),
+                format_text(&unknown.to_string()),
+            ],
+        );
+    }
+}
+
+#[test]
+fn projected_image_marker_still_requires_a_complete_image() {
+    let bytes = base64::engine::general_purpose::STANDARD
+        .decode(PNG)
+        .expect("decode PNG fixture");
+    let truncated = base64::engine::general_purpose::STANDARD.encode(&bytes[..33]);
+    let invalid = json!({"type": "image", "mimeType": "image/png", "data": truncated});
+    let valid = json!({"type": "image", "mimeType": "image/png", "data": format!("data:image/png;base64,{PNG}")});
+
+    let projected = McpToolResult::new(result(vec![invalid.clone()]), McpResultKind::Standard);
+    assert!(!projected.has_image);
+    assert_eq!(projected.content[0].render(/*width*/ 80), "<image content>");
+
+    let projected = McpToolResult::new(result(vec![invalid, valid]), McpResultKind::Standard);
+    assert!(projected.has_image);
+}
+
+#[test]
+fn code_mode_preserves_text_fields_on_nontext_and_unknown_blocks() {
+    let mut cell = new_active_mcp_tool_call(
+        "call-code-mode".to_string(),
+        McpInvocation {
+            server: "node_repl".to_string(),
+            tool: "js".to_string(),
+            arguments: Some(json!({"title": "Inspect results"})),
+        },
+        /*animations_enabled*/ false,
+    );
+    let unknown =
+        json!({"type": "future_block", "text": "Script completed\nOutput:\nunknown-side output"});
+    cell.complete(
+        Duration::ZERO,
+        Ok(result(vec![
+            json!({"type": "image", "mimeType": "image/png", "data": PNG, "text": "Script completed\nOutput:\nimage-side output"}),
+            unknown.clone(),
+        ])),
+    );
+
+    let display = cell
+        .display_lines(/*width*/ 200)
+        .iter()
+        .map(ToString::to_string)
+        .collect::<Vec<_>>()
+        .join("\n");
+    let transcript = cell
+        .transcript_lines(/*width*/ 200)
+        .iter()
+        .map(ToString::to_string)
+        .collect::<Vec<_>>()
+        .join("\n");
+    insta::assert_snapshot!(format!("history:\n{display}\n\ntranscript:\n{transcript}"), @r#"
+    history:
+    • Called Inspect results
+      └ image-side output
+        unknown-side output
+
+    transcript:
+    • Called node_repl.js({"title":"Inspect results"})
+      └ Script completed
+        Output:
+        image-side output
+        Script completed
+        Output:
+        unknown-side output
+    "#);
+    assert_eq!(
+        cell.raw_lines(),
+        vec![
+            Line::from("Called node_repl.js({\"title\":\"Inspect results\"})"),
+            Line::from("<image content>"),
+            Line::from(format_and_truncate_tool_result(
+                &unknown.to_string(),
+                TOOL_CALL_MAX_LINES,
+                RAW_TOOL_OUTPUT_WIDTH,
+            )),
+        ],
+    );
+}
@@ -51,7 +51,6 @@ use crate::version::CODEX_CLI_VERSION;
 use crate::wrapping::RtOptions;
 use crate::wrapping::adaptive_wrap_line;
 use crate::wrapping::adaptive_wrap_lines;
-use base64::Engine;
 use codex_app_server_protocol::AskForApproval;
 use codex_app_server_protocol::McpAuthStatus;
 use codex_app_server_protocol::McpServerStatus;
@@ -83,8 +82,6 @@ use codex_protocol::user_input::TextElement;
 use codex_utils_absolute_path::AbsolutePathBuf;
 #[cfg(test)]
 use codex_utils_cli::format_env_display;
-use image::DynamicImage;
-use image::ImageReader;
 use ratatui::prelude::*;
 use ratatui::style::Color;
 use ratatui::style::Modifier;
@@ -96,12 +93,10 @@ use ratatui::widgets::Paragraph;
 use ratatui::widgets::Wrap;
 use std::any::Any;
 use std::collections::HashMap;
-use std::io::Cursor;
 use std::path::Path;
 use std::path::PathBuf;
 use std::time::Duration;
 use std::time::Instant;
-use tracing::error;
 use unicode_segmentation::UnicodeSegmentation;
 use url::Url;