codex-rs: make tool calls prettier #1211

rgwood-dd · 2025-06-03T18:48:39Z

This PR overhauls how active tool calls and completed tool calls are displayed:

More use of colour to indicate success/failure and distinguish between components like tool name+arguments
Previously, the entire CallToolResult was serialized to JSON and pretty-printed. Now, we extract each individual CallToolResultContent and print those
1. The previous solution was wasting space by unnecessarily showing details of the CallToolResult struct to users, without formatting the actual tool call results nicely
2. We're now able to show users more information from tool results in less space, with nicer formatting when tools return JSON results

Before:

After:

Future Work

Integrate image tool result handling better. We should be able to display images even if they're not the first CallToolResultContent
Users should have some way to view the full version of truncated tool results
It would be nice to add some left padding for tool results, make it more clear that they are results. This is doable, just a little fiddly due to the way first_visible_line scrolling works
There's almost certainly a better way to format JSON than "all on 1 line with spaces to make Ratatui wrapping work". But I think that works OK for now.

bolinfest · 2025-06-03T19:00:18Z

codex-rs/tui/Cargo.toml

 tui-markdown = "0.3.3"
 tui-textarea = "0.7.0"
 uuid = "1"
+unicode-segmentation = "1.12.0"


nit: alpha sort?

bolinfest · 2025-06-03T19:01:06Z

codex-rs/tui/src/history_cell.rs

                ("approval", format!("{:?}", config.approval_policy)),
                ("sandbox", format!("{:?}", config.sandbox_policy)),
            ];
-            if config.model_provider.wire_api == WireApi::Responses


bad rebase maybe? did you mean to remove this?

Oops, sorry for not catching this. Yes, I messed this up when moving from this repo to my own fork 🙇

bolinfest · 2025-06-03T19:08:28Z

codex-rs/tui/src/history_cell.rs

+    // try to parse the text as json
+    let json = serde_json::from_str::<serde_json::Value>(text);
+    if let Ok(json) = json {
+        let json_pretty = serde_json::to_string_pretty(&json).unwrap_or_else(|_| json.to_string());


Hmm, I guess PrettyFormatter isn't configurable to support what you're trying to do here?

https://docs.rs/serde_json/1.0.140/serde_json/ser/struct.PrettyFormatter.html

I don't believe so, it looks like only indentation is configurable in PrettyFormatter – no matter what it will always split JSON onto multiple newlines, i.e.

{ "foo": "bar" }

Instead of {"foo": "bar"}.

I think there's a tradeoff here between readability and compactness. serde_json pretty-printing looks good but it leaves a lot of horizontal space unused; not much info can be shown in 5 lines if every field and bracket gets its own line.

bolinfest · 2025-06-03T19:21:27Z

codex-rs/tui/src/history_cell.rs

    pub(crate) fn new_completed_mcp_tool_call(
-        fq_tool_name: String,
-        invocation: String,
+        terminal_width: u16,


Maybe num_cols is a more appropriate name? Because the terminal is technically a bit wider because of borders and whatnot, right?

Yes, good point

bolinfest · 2025-06-03T19:23:42Z

codex-rs/tui/src/history_cell.rs

 }
+
+/// Truncate a tool result to fit within the given width. If the text is valid JSON, we format it in a compact way before truncating
+fn format_and_truncate_tool_result(text: &str, width: u16) -> String {


Could you please add a couple of unit tests for these functions? Or maybe move them into a helper file (truncate.rs?) since this file is getting a bit long...

rgwood-dd · 2025-06-03T21:05:37Z

codex-rs/mcp-types/src/lib.rs


 #[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
 #[serde(untagged)]
+#[allow(clippy::large_enum_variant)]


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.

rgwood-dd · 2025-06-03T21:11:00Z

@bolinfest Thank you for the detailed review and sorry for missing a few obvious things! I've addressed your comments, including moving the truncation+formatting logic out to text_formatting.rs and adding several tests.

With tests in place I made some improvements to the JSON formatting (preserving original order of fields, more opinionated compact formatting). It's a little complex but I think with the comments+tests in place it's worthwhile.

rgwood-dd · 2025-06-03T21:16:40Z

How it looks after the latest PR:

bolinfest

This looks great and thanks for the thorough tests! I'll merge once CI is green.

bolinfest · 2025-06-03T21:20:13Z

codex-rs/tui/src/text_formatting.rs

+            '\n' | '\r' if !in_string => {
+                // Skip newlines when not in a string
+            }
+            ' ' | '\t' if !in_string => {


to_string_pretty() should never include a literal \r or \n, should it?

Make tool calls prettier

c824ac4

rgwood-dd force-pushed the pretty-tool-calls branch from aef101c to c824ac4 Compare June 3, 2025 18:52

bolinfest requested changes Jun 3, 2025

View reviewed changes

Add reasoning back

33accad

rgwood-dd commented Jun 3, 2025

View reviewed changes

Refactor+test truncation+JSON formatting code

6f67caf

rgwood-dd force-pushed the pretty-tool-calls branch from 530905b to 6f67caf Compare June 3, 2025 21:09

Allow unwrap() in text_formatting tests

a2f9a49

bolinfest approved these changes Jun 3, 2025

View reviewed changes

bolinfest merged commit a67a67f into openai:main Jun 3, 2025
9 checks passed

github-actions bot locked and limited conversation to collaborators Jun 3, 2025

codex-rs: make tool calls prettier #1211

codex-rs: make tool calls prettier #1211

Uh oh!

Conversation

rgwood-dd commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before:

After:

Future Work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgwood-dd commented Jun 3, 2025

Uh oh!

rgwood-dd commented Jun 3, 2025

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rgwood-dd commented Jun 3, 2025 •

edited

Loading