Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codex-rs/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions codex-rs/mcp-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1144,6 +1144,7 @@ pub enum ServerRequest {

#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
#[serde(untagged)]
#[allow(clippy::large_enum_variant)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After enabling the preserve_order feature on serde_json, this Clippy lint started going off:

warning: large size difference between variants
    --> mcp-types/src/lib.rs:1147:1
     |
1147 | / pub enum ServerResult {
1148 | |     Result(Result),
     | |     -------------- the second-largest variant contains at least 72 bytes
1149 | |     InitializeResult(InitializeResult),
     | |     ---------------------------------- the largest variant contains at least 320 bytes
1150 | |     ListResourcesResult(ListResourcesResult),
...    |
1157 | |     CompleteResult(CompleteResult),
1158 | | }
     | |_^ the entire enum is at least 320 bytes
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
     = note: `#[warn(clippy::large_enum_variant)]` on by default
help: consider boxing the large fields to reduce the total size of the enum
     |
1149 -     InitializeResult(InitializeResult),
1149 +     InitializeResult(Box<InitializeResult>),
     |

I don't think this is especially performance-sensitive code, boxing InitializeResults feels like a premature optimization to me.

pub enum ServerResult {
Result(Result),
InitializeResult(InitializeResult),
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/tui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ ratatui = { version = "0.29.0", features = [
] }
ratatui-image = "8.0.0"
regex-lite = "0.1"
serde_json = "1"
serde_json = { version = "1", features = ["preserve_order"] }
shlex = "1.3.0"
strum = "0.27.1"
strum_macros = "0.27.1"
Expand All @@ -51,6 +51,7 @@ tracing-subscriber = { version = "0.3.19", features = ["env-filter"] }
tui-input = "0.11.1"
tui-markdown = "0.3.3"
tui-textarea = "0.7.0"
unicode-segmentation = "1.12.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: alpha sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

uuid = "1"

[dev-dependencies]
Expand Down
3 changes: 1 addition & 2 deletions codex-rs/tui/src/conversation_history_widget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,15 +299,14 @@ impl ConversationHistoryWidget {
for entry in self.entries.iter_mut() {
if let HistoryCell::ActiveMcpToolCall {
call_id: history_id,
fq_tool_name,
invocation,
start,
..
} = &entry.cell
{
if &call_id == history_id {
let completed = HistoryCell::new_completed_mcp_tool_call(
fq_tool_name.clone(),
width,
invocation.clone(),
*start,
success,
Expand Down
125 changes: 78 additions & 47 deletions codex-rs/tui/src/history_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::cell_widget::CellWidget;
use crate::exec_command::escape_command;
use crate::markdown::append_markdown;
use crate::text_block::TextBlock;
use crate::text_formatting::format_and_truncate_tool_result;
use base64::Engine;
use codex_ansi_escape::ansi_escape_line;
use codex_common::elapsed::format_duration;
Expand All @@ -14,6 +15,7 @@ use image::DynamicImage;
use image::GenericImageView;
use image::ImageReader;
use lazy_static::lazy_static;
use mcp_types::EmbeddedResourceResource;
use ratatui::prelude::*;
use ratatui::style::Color;
use ratatui::style::Modifier;
Expand Down Expand Up @@ -73,18 +75,14 @@ pub(crate) enum HistoryCell {
/// An MCP tool call that has not finished yet.
ActiveMcpToolCall {
call_id: String,
/// `server.tool` fully-qualified name so we can show a concise label
fq_tool_name: String,
/// Formatted invocation that mirrors the `$ cmd ...` style of exec
/// commands. We keep this around so the completed state can reuse the
/// exact same text without re-formatting.
invocation: String,
/// Formatted line that shows the command name and arguments
invocation: Line<'static>,
start: Instant,
view: TextBlock,
},

/// Completed MCP tool call where we show the result serialized as JSON.
CompletedMcpToolCallWithTextOutput { view: TextBlock },
CompletedMcpToolCall { view: TextBlock },

/// Completed MCP tool call where the result is an image.
/// Admittedly, [mcp_types::CallToolResult] can have multiple content types,
Expand Down Expand Up @@ -289,8 +287,6 @@ impl HistoryCell {
tool: String,
arguments: Option<serde_json::Value>,
) -> Self {
let fq_tool_name = format!("{server}.{tool}");

// Format the arguments as compact JSON so they roughly fit on one
// line. If there are no arguments we keep it empty so the invocation
// mirrors a function-style call.
Expand All @@ -302,29 +298,30 @@ impl HistoryCell {
})
.unwrap_or_default();

let invocation = if args_str.is_empty() {
format!("{fq_tool_name}()")
} else {
format!("{fq_tool_name}({args_str})")
};
let invocation_spans = vec![
Span::styled(server, Style::default().fg(Color::Blue)),
Span::raw("."),
Span::styled(tool, Style::default().fg(Color::Blue)),
Span::raw("("),
Span::styled(args_str, Style::default().fg(Color::Gray)),
Span::raw(")"),
];
let invocation = Line::from(invocation_spans);

let start = Instant::now();
let title_line = Line::from(vec!["tool".magenta(), " running...".dim()]);
let lines: Vec<Line<'static>> = vec![
title_line,
Line::from(format!("$ {invocation}")),
Line::from(""),
];
let lines: Vec<Line<'static>> = vec![title_line, invocation.clone(), Line::from("")];

HistoryCell::ActiveMcpToolCall {
call_id,
fq_tool_name,
invocation,
start,
view: TextBlock::new(lines),
}
}

/// If the first content is an image, return a new cell with the image.
/// TODO(rgwood-dd): Handle images properly even if they're not the first result.
fn try_new_completed_mcp_tool_call_with_image_output(
result: &Result<mcp_types::CallToolResult, String>,
) -> Option<Self> {
Expand Down Expand Up @@ -370,8 +367,8 @@ impl HistoryCell {
}

pub(crate) fn new_completed_mcp_tool_call(
fq_tool_name: String,
invocation: String,
num_cols: u16,
invocation: Line<'static>,
start: Instant,
success: bool,
result: Result<mcp_types::CallToolResult, String>,
Expand All @@ -384,36 +381,70 @@ impl HistoryCell {
let status_str = if success { "success" } else { "failed" };
let title_line = Line::from(vec![
"tool".magenta(),
format!(" {fq_tool_name} ({status_str}, duration: {})", duration).dim(),
" ".into(),
if success {
status_str.green()
} else {
status_str.red()
},
format!(", duration: {duration}").gray(),
]);

let mut lines: Vec<Line<'static>> = Vec::new();
lines.push(title_line);
lines.push(Line::from(format!("$ {invocation}")));

// Convert result into serde_json::Value early so we don't have to
// worry about lifetimes inside the match arm.
let result_val = result.map(|r| {
serde_json::to_value(r)
.unwrap_or_else(|_| serde_json::Value::String("<serialization error>".into()))
});

if let Ok(res_val) = result_val {
let json_pretty =
serde_json::to_string_pretty(&res_val).unwrap_or_else(|_| res_val.to_string());
let mut iter = json_pretty.lines();
for raw in iter.by_ref().take(TOOL_CALL_MAX_LINES) {
lines.push(Line::from(raw.to_string()).dim());
lines.push(invocation);

match result {
Ok(mcp_types::CallToolResult { content, .. }) => {
if !content.is_empty() {
lines.push(Line::from(""));

for tool_call_result in content {
let line_text = match tool_call_result {
mcp_types::CallToolResultContent::TextContent(text) => {
format_and_truncate_tool_result(
&text.text,
TOOL_CALL_MAX_LINES,
num_cols as usize,
)
}
mcp_types::CallToolResultContent::ImageContent(_) => {
// TODO show images even if they're not the first result, will require a refactor of `CompletedMcpToolCall`
"<image content>".to_string()
}
mcp_types::CallToolResultContent::AudioContent(_) => {
"<audio content>".to_string()
}
mcp_types::CallToolResultContent::EmbeddedResource(resource) => {
let uri = match resource.resource {
EmbeddedResourceResource::TextResourceContents(text) => {
text.uri
}
EmbeddedResourceResource::BlobResourceContents(blob) => {
blob.uri
}
};
format!("embedded resource: {uri}")
}
};
lines.push(Line::styled(line_text, Style::default().fg(Color::Gray)));
}
}

lines.push(Line::from(""));
}
let remaining = iter.count();
if remaining > 0 {
lines.push(Line::from(format!("... {} additional lines", remaining)).dim());
Err(e) => {
lines.push(Line::from(vec![
Span::styled(
"Error: ",
Style::default().fg(Color::Red).add_modifier(Modifier::BOLD),
),
Span::raw(e),
]));
}
}

lines.push(Line::from(""));
};

HistoryCell::CompletedMcpToolCallWithTextOutput {
HistoryCell::CompletedMcpToolCall {
view: TextBlock::new(lines),
}
}
Expand Down Expand Up @@ -520,7 +551,7 @@ impl CellWidget for HistoryCell {
| HistoryCell::ErrorEvent { view }
| HistoryCell::SessionInfo { view }
| HistoryCell::CompletedExecCommand { view }
| HistoryCell::CompletedMcpToolCallWithTextOutput { view }
| HistoryCell::CompletedMcpToolCall { view }
| HistoryCell::PendingPatch { view }
| HistoryCell::ActiveExecCommand { view, .. }
| HistoryCell::ActiveMcpToolCall { view, .. } => view.height(width),
Expand All @@ -541,7 +572,7 @@ impl CellWidget for HistoryCell {
| HistoryCell::ErrorEvent { view }
| HistoryCell::SessionInfo { view }
| HistoryCell::CompletedExecCommand { view }
| HistoryCell::CompletedMcpToolCallWithTextOutput { view }
| HistoryCell::CompletedMcpToolCall { view }
| HistoryCell::PendingPatch { view }
| HistoryCell::ActiveExecCommand { view, .. }
| HistoryCell::ActiveMcpToolCall { view, .. } => {
Expand Down
1 change: 1 addition & 0 deletions codex-rs/tui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ mod scroll_event_helper;
mod slash_command;
mod status_indicator_widget;
mod text_block;
mod text_formatting;
mod tui;
mod user_approval_widget;

Expand Down
Loading