From 2bd3314886cb1fe341778f00fc0f0537f6a1c976 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Wed, 16 Jul 2025 15:11:18 -0700 Subject: [PATCH 1/8] support deltas in core (#1587) - Added support for message and reasoning deltas - Skipped adding the support in the cli and tui for later - Commented a failing test (wrong merge) that needs fix in a separate PR. Side note: I think we need to disable merge when the CI don't pass. --- codex-rs/core/src/chat_completions.rs | 8 +++++- codex-rs/core/src/client.rs | 24 ++++++++++++++--- codex-rs/core/src/client_common.rs | 2 ++ codex-rs/core/src/codex.rs | 27 +++++++++++++------- codex-rs/core/src/protocol.rs | 16 ++++++++++++ codex-rs/core/tests/cli_stream.rs | 4 +-- codex-rs/core/tests/stream_no_completed.rs | 2 ++ codex-rs/exec/src/event_processor.rs | 8 ++++++ codex-rs/mcp-server/src/codex_tool_runner.rs | 6 +++++ codex-rs/tui/src/chatwidget.rs | 8 ++++++ 10 files changed, 89 insertions(+), 16 deletions(-) diff --git a/codex-rs/core/src/chat_completions.rs b/codex-rs/core/src/chat_completions.rs index 816fc80f9b..ad7b55952a 100644 --- a/codex-rs/core/src/chat_completions.rs +++ b/codex-rs/core/src/chat_completions.rs @@ -134,7 +134,7 @@ pub(crate) async fn stream_chat_completions( match res { Ok(resp) if resp.status().is_success() => { - let (tx_event, rx_event) = mpsc::channel::>(16); + let (tx_event, rx_event) = mpsc::channel::>(1600); let stream = resp.bytes_stream().map_err(CodexErr::Reqwest); tokio::spawn(process_chat_sse(stream, tx_event)); return Ok(ResponseStream { rx_event }); @@ -426,6 +426,12 @@ where // will never appear in a Chat Completions stream. continue; } + Poll::Ready(Some(Ok(ResponseEvent::OutputTextDelta(_)))) + | Poll::Ready(Some(Ok(ResponseEvent::ReasoningSummaryDelta(_)))) => { + // Deltas are ignored here since aggregation waits for the + // final OutputItemDone. + continue; + } } } } diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 2fa182cf7f..8ec68d02e8 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -125,6 +125,7 @@ impl ModelClient { reasoning, previous_response_id: prompt.prev_id.clone(), store: prompt.store, + // TODO: make this configurable stream: true, }; @@ -148,7 +149,7 @@ impl ModelClient { let res = req_builder.send().await; match res { Ok(resp) if resp.status().is_success() => { - let (tx_event, rx_event) = mpsc::channel::>(16); + let (tx_event, rx_event) = mpsc::channel::>(1600); // spawn task to process SSE let stream = resp.bytes_stream().map_err(CodexErr::Reqwest); @@ -205,6 +206,7 @@ struct SseEvent { kind: String, response: Option, item: Option, + delta: Option, } #[derive(Debug, Deserialize)] @@ -337,6 +339,22 @@ where return; } } + "response.output_text.delta" => { + if let Some(delta) = event.delta { + let event = ResponseEvent::OutputTextDelta(delta); + if tx_event.send(Ok(event)).await.is_err() { + return; + } + } + } + "response.reasoning_summary_text.delta" => { + if let Some(delta) = event.delta { + let event = ResponseEvent::ReasoningSummaryDelta(delta); + if tx_event.send(Ok(event)).await.is_err() { + return; + } + } + } "response.created" => { if event.response.is_some() { let _ = tx_event.send(Ok(ResponseEvent::Created {})).await; @@ -360,10 +378,8 @@ where | "response.function_call_arguments.delta" | "response.in_progress" | "response.output_item.added" - | "response.output_text.delta" | "response.output_text.done" | "response.reasoning_summary_part.added" - | "response.reasoning_summary_text.delta" | "response.reasoning_summary_text.done" => { // Currently, we ignore these events, but we handle them // separately to skip the logging message in the `other` case. @@ -375,7 +391,7 @@ where /// used in tests to stream from a text SSE file async fn stream_from_fixture(path: impl AsRef) -> Result { - let (tx_event, rx_event) = mpsc::channel::>(16); + let (tx_event, rx_event) = mpsc::channel::>(1600); let f = std::fs::File::open(path.as_ref())?; let lines = std::io::BufReader::new(f).lines(); diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index f9a816a7a9..3e3c2e7efa 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -57,6 +57,8 @@ pub enum ResponseEvent { response_id: String, token_usage: Option, }, + OutputTextDelta(String), + ReasoningSummaryDelta(String), } #[derive(Debug, Serialize)] diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 52c37c51ee..5227f93c8e 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -61,7 +61,9 @@ use crate::models::ResponseInputItem; use crate::models::ResponseItem; use crate::models::ShellToolCallParams; use crate::project_doc::get_user_instructions; +use crate::protocol::AgentMessageDeltaEvent; use crate::protocol::AgentMessageEvent; +use crate::protocol::AgentReasoningDeltaEvent; use crate::protocol::AgentReasoningEvent; use crate::protocol::ApplyPatchApprovalRequestEvent; use crate::protocol::AskForApproval; @@ -103,7 +105,7 @@ impl Codex { /// submitted to start the session. pub async fn spawn(config: Config, ctrl_c: Arc) -> CodexResult<(Codex, String)> { let (tx_sub, rx_sub) = async_channel::bounded(64); - let (tx_event, rx_event) = async_channel::bounded(64); + let (tx_event, rx_event) = async_channel::bounded(1600); let instructions = get_user_instructions(&config).await; let configure_session = Op::ConfigureSession { @@ -1121,15 +1123,8 @@ async fn try_run_turn( let mut stream = sess.client.clone().stream(&prompt).await?; - // Buffer all the incoming messages from the stream first, then execute them. - // If we execute a function call in the middle of handling the stream, it can time out. - let mut input = Vec::new(); - while let Some(event) = stream.next().await { - input.push(event?); - } - let mut output = Vec::new(); - for event in input { + while let Some(Ok(event)) = stream.next().await { match event { ResponseEvent::Created => { let mut state = sess.state.lock().unwrap(); @@ -1172,6 +1167,20 @@ async fn try_run_turn( state.previous_response_id = Some(response_id); break; } + ResponseEvent::OutputTextDelta(delta) => { + let event = Event { + id: sub_id.to_string(), + msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta }), + }; + sess.tx_event.send(event).await.ok(); + } + ResponseEvent::ReasoningSummaryDelta(delta) => { + let event = Event { + id: sub_id.to_string(), + msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta }), + }; + sess.tx_event.send(event).await.ok(); + } } } Ok(output) diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index fa25a2fe38..b233d4f27b 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -282,9 +282,15 @@ pub enum EventMsg { /// Agent text output message AgentMessage(AgentMessageEvent), + /// Agent text output delta message + AgentMessageDelta(AgentMessageDeltaEvent), + /// Reasoning event from agent. AgentReasoning(AgentReasoningEvent), + /// Agent reasoning delta event from agent. + AgentReasoningDelta(AgentReasoningDeltaEvent), + /// Ack the client's configure message. SessionConfigured(SessionConfiguredEvent), @@ -340,11 +346,21 @@ pub struct AgentMessageEvent { pub message: String, } +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct AgentMessageDeltaEvent { + pub delta: String, +} + #[derive(Debug, Clone, Deserialize, Serialize)] pub struct AgentReasoningEvent { pub text: String, } +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct AgentReasoningDeltaEvent { + pub delta: String, +} + #[derive(Debug, Clone, Deserialize, Serialize)] pub struct McpToolCallBeginEvent { /// Identifier so this can be paired with the McpToolCallEnd event. diff --git a/codex-rs/core/tests/cli_stream.rs b/codex-rs/core/tests/cli_stream.rs index df3fedfd48..9ef042eb1a 100644 --- a/codex-rs/core/tests/cli_stream.rs +++ b/codex-rs/core/tests/cli_stream.rs @@ -71,8 +71,8 @@ async fn chat_mode_stream_cli() { println!("Stderr:\n{}", String::from_utf8_lossy(&output.stderr)); assert!(output.status.success()); let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("hi")); - assert_eq!(stdout.matches("hi").count(), 1); + let hi_lines = stdout.lines().filter(|line| line.trim() == "hi").count(); + assert_eq!(hi_lines, 1, "Expected exactly one line with 'hi'"); server.verify().await; } diff --git a/codex-rs/core/tests/stream_no_completed.rs b/codex-rs/core/tests/stream_no_completed.rs index da2736aa77..8883eff373 100644 --- a/codex-rs/core/tests/stream_no_completed.rs +++ b/codex-rs/core/tests/stream_no_completed.rs @@ -32,6 +32,8 @@ fn sse_completed(id: &str) -> String { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// this test is flaky (has race conditions), so we ignore it for now +#[ignore] async fn retries_on_early_close() { #![allow(clippy::unwrap_used)] diff --git a/codex-rs/exec/src/event_processor.rs b/codex-rs/exec/src/event_processor.rs index 540e014298..2a7c4c621b 100644 --- a/codex-rs/exec/src/event_processor.rs +++ b/codex-rs/exec/src/event_processor.rs @@ -3,7 +3,9 @@ use codex_common::summarize_sandbox_policy; use codex_core::WireApi; use codex_core::config::Config; use codex_core::model_supports_reasoning_summaries; +use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; +use codex_core::protocol::AgentReasoningDeltaEvent; use codex_core::protocol::BackgroundEventEvent; use codex_core::protocol::ErrorEvent; use codex_core::protocol::Event; @@ -184,6 +186,12 @@ impl EventProcessor { EventMsg::TokenCount(TokenUsage { total_tokens, .. }) => { ts_println!(self, "tokens used: {total_tokens}"); } + EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: _ }) => { + // TODO: think how we want to support this in the CLI + } + EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta: _ }) => { + // TODO: think how we want to support this in the CLI + } EventMsg::AgentMessage(AgentMessageEvent { message }) => { ts_println!( self, diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 7c3b02fe5e..88dcf649dc 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -171,6 +171,12 @@ pub async fn run_codex_tool_session( EventMsg::SessionConfigured(_) => { tracing::error!("unexpected SessionConfigured event"); } + EventMsg::AgentMessageDelta(_) => { + // TODO: think how we want to support this in the MCP + } + EventMsg::AgentReasoningDelta(_) => { + // TODO: think how we want to support this in the MCP + } EventMsg::Error(_) | EventMsg::TaskStarted | EventMsg::TokenCount(_) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 51fdfc3e8a..28014c6e40 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -3,7 +3,9 @@ use std::sync::Arc; use codex_core::codex_wrapper::init_codex; use codex_core::config::Config; +use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; +use codex_core::protocol::AgentReasoningDeltaEvent; use codex_core::protocol::AgentReasoningEvent; use codex_core::protocol::ApplyPatchApprovalRequestEvent; use codex_core::protocol::ErrorEvent; @@ -375,6 +377,12 @@ impl ChatWidget<'_> { self.bottom_pane .on_history_entry_response(log_id, offset, entry.map(|e| e.text)); } + EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: _ }) => { + // TODO: think how we want to support this in the TUI + } + EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta: _ }) => { + // TODO: think how we want to support this in the TUI + } event => { self.conversation_history .add_background_event(format!("{event:?}")); From 0bc7ee91937d5cb9ff0e6a77e35cc89a91a81892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Preet=20=F0=9F=9A=80?= <114741744+preetDev004@users.noreply.github.com> Date: Wed, 16 Jul 2025 19:00:39 -0400 Subject: [PATCH 2/8] Added mcp-server name validation (#1591) This PR implements server name validation for MCP (Model Context Protocol) servers to ensure they conform to the required pattern ^[a-zA-Z0-9_-]+$. This addresses the TODO comment in mcp_connection_manager.rs:82. + Added validation before spawning MCP client tasks + Invalid server names are added to errors map with descriptive messages I have read the CLA Document and I hereby sign the CLA --------- Co-authored-by: Michael Bolin --- codex-rs/core/src/mcp_connection_manager.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 6ae1865f16..7cf6762752 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -79,9 +79,19 @@ impl McpConnectionManager { // Launch all configured servers concurrently. let mut join_set = JoinSet::new(); + let mut errors = ClientStartErrors::new(); for (server_name, cfg) in mcp_servers { - // TODO: Verify server name: require `^[a-zA-Z0-9_-]+$`? + // Validate server name before spawning + if !is_valid_mcp_server_name(&server_name) { + let error = anyhow::anyhow!( + "invalid server name '{}': must match pattern ^[a-zA-Z0-9_-]+$", + server_name + ); + errors.insert(server_name, error); + continue; + } + join_set.spawn(async move { let McpServerConfig { command, args, env } = cfg; let client_res = McpClient::new_stdio_client(command, args, env).await; @@ -117,7 +127,6 @@ impl McpConnectionManager { let mut clients: HashMap> = HashMap::with_capacity(join_set.len()); - let mut errors = ClientStartErrors::new(); while let Some(res) = join_set.join_next().await { let (server_name, client_res) = res?; // JoinError propagation @@ -208,3 +217,10 @@ pub async fn list_all_tools( Ok(aggregated) } + +fn is_valid_mcp_server_name(server_name: &str) -> bool { + !server_name.is_empty() + && server_name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') +} From d3dbc104798eb15ab88c71ce8a33d42f78668c96 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 16 Jul 2025 16:35:29 -0700 Subject: [PATCH 3/8] fix: update bin/codex.js so it listens for exit on the child process (#1590) When Codex CLI is installed via `npm`, we use a `.js` wrapper script to launch the Rust binary. - Previously, we were not listening for signals to ensure that killing the Node.js process would also kill the underlying Rust process. - We also did not have a proper `exit` handler in place on the child process to ensure we exited from the Node.js process. This PR fixes these things and hopefully addresses https://github.com/openai/codex/issues/1570. This also adds logic so that Windows falls back to the TypeScript CLI again, which should address https://github.com/openai/codex/issues/1573. --- codex-cli/bin/codex.js | 79 +++++++++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 13 deletions(-) diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index 54b99078e4..ae1fb9593c 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -15,7 +15,6 @@ * current platform / architecture, an error is thrown. */ -import { spawnSync } from "child_process"; import fs from "fs"; import path from "path"; import { fileURLToPath, pathToFileURL } from "url"; @@ -35,7 +34,7 @@ const wantsNative = fs.existsSync(path.join(__dirname, "use-native")) || : false); // Try native binary if requested. -if (wantsNative) { +if (wantsNative && process.platform !== 'win32') { const { platform, arch } = process; let targetTriple = null; @@ -74,22 +73,76 @@ if (wantsNative) { } const binaryPath = path.join(__dirname, "..", "bin", `codex-${targetTriple}`); - const result = spawnSync(binaryPath, process.argv.slice(2), { + + // Use an asynchronous spawn instead of spawnSync so that Node is able to + // respond to signals (e.g. Ctrl-C / SIGINT) while the native binary is + // executing. This allows us to forward those signals to the child process + // and guarantees that when either the child terminates or the parent + // receives a fatal signal, both processes exit in a predictable manner. + const { spawn } = await import("child_process"); + + const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", }); - const exitCode = typeof result.status === "number" ? result.status : 1; - process.exit(exitCode); -} + child.on("error", (err) => { + // Typically triggered when the binary is missing or not executable. + // Re-throwing here will terminate the parent with a non-zero exit code + // while still printing a helpful stack trace. + // eslint-disable-next-line no-console + console.error(err); + process.exit(1); + }); -// Fallback: execute the original JavaScript CLI. + // Forward common termination signals to the child so that it shuts down + // gracefully. In the handler we temporarily disable the default behavior of + // exiting immediately; once the child has been signaled we simply wait for + // its exit event which will in turn terminate the parent (see below). + const forwardSignal = (signal) => { + if (child.killed) { + return; + } + try { + child.kill(signal); + } catch { + /* ignore */ + } + }; -// Resolve the path to the compiled CLI bundle -const cliPath = path.resolve(__dirname, "../dist/cli.js"); -const cliUrl = pathToFileURL(cliPath).href; + ["SIGINT", "SIGTERM", "SIGHUP"].forEach((sig) => { + process.on(sig, () => forwardSignal(sig)); + }); -// Load and execute the CLI -(async () => { + // When the child exits, mirror its termination reason in the parent so that + // shell scripts and other tooling observe the correct exit status. + // Wrap the lifetime of the child process in a Promise so that we can await + // its termination in a structured way. The Promise resolves with an object + // describing how the child exited: either via exit code or due to a signal. + const childResult = await new Promise((resolve) => { + child.on("exit", (code, signal) => { + if (signal) { + resolve({ type: "signal", signal }); + } else { + resolve({ type: "code", exitCode: code ?? 1 }); + } + }); + }); + + if (childResult.type === "signal") { + // Re-emit the same signal so that the parent terminates with the expected + // semantics (this also sets the correct exit code of 128 + n). + process.kill(process.pid, childResult.signal); + } else { + process.exit(childResult.exitCode); + } +} else { + // Fallback: execute the original JavaScript CLI. + + // Resolve the path to the compiled CLI bundle + const cliPath = path.resolve(__dirname, "../dist/cli.js"); + const cliUrl = pathToFileURL(cliPath).href; + + // Load and execute the CLI try { await import(cliUrl); } catch (err) { @@ -97,4 +150,4 @@ const cliUrl = pathToFileURL(cliPath).href; console.error(err); process.exit(1); } -})(); +} From 643ab1f582a248a9f995bf94110d28fe9677d387 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Wed, 16 Jul 2025 22:26:31 -0700 Subject: [PATCH 4/8] Add streaming to exec and tui (#1594) Added support for streaming in `tui` Added support for streaming in `exec` https://github.com/user-attachments/assets/4215892e-d940-452c-a1d0-416ed0cf14eb --- codex-rs/exec/src/event_processor.rs | 77 ++++++++++++++----- codex-rs/exec/src/lib.rs | 3 +- codex-rs/tui/src/app.rs | 2 + codex-rs/tui/src/chatwidget.rs | 53 ++++++++++--- .../tui/src/conversation_history_widget.rs | 46 ++++++++++- 5 files changed, 149 insertions(+), 32 deletions(-) diff --git a/codex-rs/exec/src/event_processor.rs b/codex-rs/exec/src/event_processor.rs index 2a7c4c621b..5ab09994b1 100644 --- a/codex-rs/exec/src/event_processor.rs +++ b/codex-rs/exec/src/event_processor.rs @@ -23,6 +23,7 @@ use owo_colors::OwoColorize; use owo_colors::Style; use shlex::try_join; use std::collections::HashMap; +use std::io::Write; use std::time::Instant; /// This should be configurable. When used in CI, users may not want to impose @@ -52,10 +53,12 @@ pub(crate) struct EventProcessor { /// Whether to include `AgentReasoning` events in the output. show_agent_reasoning: bool, + answer_started: bool, + reasoning_started: bool, } impl EventProcessor { - pub(crate) fn create_with_ansi(with_ansi: bool, show_agent_reasoning: bool) -> Self { + pub(crate) fn create_with_ansi(with_ansi: bool, config: &Config) -> Self { let call_id_to_command = HashMap::new(); let call_id_to_patch = HashMap::new(); let call_id_to_tool_call = HashMap::new(); @@ -72,7 +75,9 @@ impl EventProcessor { green: Style::new().green(), cyan: Style::new().cyan(), call_id_to_tool_call, - show_agent_reasoning, + show_agent_reasoning: !config.hide_agent_reasoning, + answer_started: false, + reasoning_started: false, } } else { Self { @@ -86,7 +91,9 @@ impl EventProcessor { green: Style::new(), cyan: Style::new(), call_id_to_tool_call, - show_agent_reasoning, + show_agent_reasoning: !config.hide_agent_reasoning, + answer_started: false, + reasoning_started: false, } } } @@ -186,18 +193,45 @@ impl EventProcessor { EventMsg::TokenCount(TokenUsage { total_tokens, .. }) => { ts_println!(self, "tokens used: {total_tokens}"); } - EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: _ }) => { - // TODO: think how we want to support this in the CLI + EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta }) => { + if !self.answer_started { + ts_println!(self, "{}\n", "codex".style(self.italic).style(self.magenta)); + self.answer_started = true; + } + print!("{delta}"); + #[allow(clippy::expect_used)] + std::io::stdout().flush().expect("could not flush stdout"); } - EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta: _ }) => { - // TODO: think how we want to support this in the CLI + EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta }) => { + if !self.show_agent_reasoning { + return; + } + if !self.reasoning_started { + ts_println!( + self, + "{}\n", + "thinking".style(self.italic).style(self.magenta), + ); + self.reasoning_started = true; + } + print!("{delta}"); + #[allow(clippy::expect_used)] + std::io::stdout().flush().expect("could not flush stdout"); } EventMsg::AgentMessage(AgentMessageEvent { message }) => { - ts_println!( - self, - "{}\n{message}", - "codex".style(self.bold).style(self.magenta) - ); + // if answer_started is false, this means we haven't received any + // delta. Thus, we need to print the message as a new answer. + if !self.answer_started { + ts_println!( + self, + "{}\n{}", + "codex".style(self.italic).style(self.magenta), + message, + ); + } else { + println!(); + self.answer_started = false; + } } EventMsg::ExecCommandBegin(ExecCommandBeginEvent { call_id, @@ -351,7 +385,7 @@ impl EventProcessor { ); // Pretty-print the patch summary with colored diff markers so - // it’s easy to scan in the terminal output. + // it's easy to scan in the terminal output. for (path, change) in changes.iter() { match change { FileChange::Add { content } => { @@ -449,12 +483,17 @@ impl EventProcessor { } EventMsg::AgentReasoning(agent_reasoning_event) => { if self.show_agent_reasoning { - ts_println!( - self, - "{}\n{}", - "thinking".style(self.italic).style(self.magenta), - agent_reasoning_event.text - ); + if !self.reasoning_started { + ts_println!( + self, + "{}\n{}", + "codex".style(self.italic).style(self.magenta), + agent_reasoning_event.text, + ); + } else { + println!(); + self.reasoning_started = false; + } } } EventMsg::SessionConfigured(session_configured_event) => { diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 44dddd4d0f..afefed1a93 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -115,8 +115,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any }; let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides)?; - let mut event_processor = - EventProcessor::create_with_ansi(stdout_with_ansi, !config.hide_agent_reasoning); + let mut event_processor = EventProcessor::create_with_ansi(stdout_with_ansi, &config); // Print the effective configuration and prompt so users can see what Codex // is using. event_processor.print_config_summary(&config, &prompt); diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 33297ad372..883250400d 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -297,6 +297,8 @@ impl<'a> App<'a> { } fn draw_next_frame(&mut self, terminal: &mut tui::Tui) -> Result<()> { + // TODO: add a throttle to avoid redrawing too often + match &mut self.app_state { AppState::Chat { widget } => { terminal.draw(|frame| frame.render_widget_ref(&**widget, frame.area()))?; diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 28014c6e40..860439ffb6 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -51,6 +51,8 @@ pub(crate) struct ChatWidget<'a> { config: Config, initial_user_message: Option, token_usage: TokenUsage, + reasoning_buffer: String, + answer_buffer: String, } #[derive(Clone, Copy, Eq, PartialEq)] @@ -137,6 +139,8 @@ impl ChatWidget<'_> { initial_images, ), token_usage: TokenUsage::default(), + reasoning_buffer: String::new(), + answer_buffer: String::new(), } } @@ -242,16 +246,51 @@ impl ChatWidget<'_> { self.request_redraw(); } EventMsg::AgentMessage(AgentMessageEvent { message }) => { + // if the answer buffer is empty, this means we haven't received any + // delta. Thus, we need to print the message as a new answer. + if self.answer_buffer.is_empty() { + self.conversation_history + .add_agent_message(&self.config, message); + } else { + self.conversation_history + .replace_prev_agent_message(&self.config, message); + } + self.answer_buffer.clear(); + self.request_redraw(); + } + EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta }) => { + if self.answer_buffer.is_empty() { + self.conversation_history + .add_agent_message(&self.config, "".to_string()); + } + self.answer_buffer.push_str(&delta.clone()); self.conversation_history - .add_agent_message(&self.config, message); + .replace_prev_agent_message(&self.config, self.answer_buffer.clone()); + self.request_redraw(); + } + EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta }) => { + if self.reasoning_buffer.is_empty() { + self.conversation_history + .add_agent_reasoning(&self.config, "".to_string()); + } + self.reasoning_buffer.push_str(&delta.clone()); + self.conversation_history + .replace_prev_agent_reasoning(&self.config, self.reasoning_buffer.clone()); self.request_redraw(); } EventMsg::AgentReasoning(AgentReasoningEvent { text }) => { - if !self.config.hide_agent_reasoning { + // if the reasoning buffer is empty, this means we haven't received any + // delta. Thus, we need to print the message as a new reasoning. + if self.reasoning_buffer.is_empty() { self.conversation_history - .add_agent_reasoning(&self.config, text); - self.request_redraw(); + .add_agent_reasoning(&self.config, "".to_string()); + } else { + // else, we rerender one last time. + self.conversation_history + .replace_prev_agent_reasoning(&self.config, text); } + self.reasoning_buffer.clear(); + self.request_redraw(); } EventMsg::TaskStarted => { self.bottom_pane.clear_ctrl_c_quit_hint(); @@ -377,12 +416,6 @@ impl ChatWidget<'_> { self.bottom_pane .on_history_entry_response(log_id, offset, entry.map(|e| e.text)); } - EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: _ }) => { - // TODO: think how we want to support this in the TUI - } - EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta: _ }) => { - // TODO: think how we want to support this in the TUI - } event => { self.conversation_history .add_background_event(format!("{event:?}")); diff --git a/codex-rs/tui/src/conversation_history_widget.rs b/codex-rs/tui/src/conversation_history_widget.rs index c0e5031d70..01a8dc6834 100644 --- a/codex-rs/tui/src/conversation_history_widget.rs +++ b/codex-rs/tui/src/conversation_history_widget.rs @@ -202,6 +202,14 @@ impl ConversationHistoryWidget { self.add_to_history(HistoryCell::new_agent_reasoning(config, text)); } + pub fn replace_prev_agent_reasoning(&mut self, config: &Config, text: String) { + self.replace_last_agent_reasoning(config, text); + } + + pub fn replace_prev_agent_message(&mut self, config: &Config, text: String) { + self.replace_last_agent_message(config, text); + } + pub fn add_background_event(&mut self, message: String) { self.add_to_history(HistoryCell::new_background_event(message)); } @@ -249,6 +257,42 @@ impl ConversationHistoryWidget { }); } + pub fn replace_last_agent_reasoning(&mut self, config: &Config, text: String) { + if let Some(idx) = self + .entries + .iter() + .rposition(|entry| matches!(entry.cell, HistoryCell::AgentReasoning { .. })) + { + let width = self.cached_width.get(); + let entry = &mut self.entries[idx]; + entry.cell = HistoryCell::new_agent_reasoning(config, text); + let height = if width > 0 { + entry.cell.height(width) + } else { + 0 + }; + entry.line_count.set(height); + } + } + + pub fn replace_last_agent_message(&mut self, config: &Config, text: String) { + if let Some(idx) = self + .entries + .iter() + .rposition(|entry| matches!(entry.cell, HistoryCell::AgentMessage { .. })) + { + let width = self.cached_width.get(); + let entry = &mut self.entries[idx]; + entry.cell = HistoryCell::new_agent_message(config, text); + let height = if width > 0 { + entry.cell.height(width) + } else { + 0 + }; + entry.line_count.set(height); + } + } + pub fn record_completed_exec_command( &mut self, call_id: String, @@ -454,7 +498,7 @@ impl WidgetRef for ConversationHistoryWidget { { // Choose a thumb color that stands out only when this pane has focus so that the - // user’s attention is naturally drawn to the active viewport. When unfocused we show + // user's attention is naturally drawn to the active viewport. When unfocused we show // a low-contrast thumb so the scrollbar fades into the background without becoming // invisible. let thumb_style = if self.has_input_focus { From fcbcc40f517dfcd1ef96e252eb821e5b630e78a3 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Thu, 17 Jul 2025 10:12:15 -0700 Subject: [PATCH 5/8] Storing the sessions in a more organized way for easier look up. (#1596) now storing the sessions in `~/.codex/sessions/YYYY/MM/DD/` --- codex-rs/Cargo.lock | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/rollout.rs | 12 ++- codex-rs/core/tests/cli_stream.rs | 156 ++++++++++++++++++++++++++++++ 4 files changed, 165 insertions(+), 5 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e59dbfa255..b1aa13a12c 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -683,6 +683,7 @@ dependencies = [ "tree-sitter", "tree-sitter-bash", "uuid", + "walkdir", "wildmatch", "wiremock", ] diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index c55d7d395d..ff066cc534 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -65,4 +65,5 @@ predicates = "3" pretty_assertions = "1.4.1" tempfile = "3" tokio-test = "0.4" +walkdir = "2.5.0" wiremock = "0.6" diff --git a/codex-rs/core/src/rollout.rs b/codex-rs/core/src/rollout.rs index c18a58df06..0ff2e94a3a 100644 --- a/codex-rs/core/src/rollout.rs +++ b/codex-rs/core/src/rollout.rs @@ -153,13 +153,15 @@ struct LogFileInfo { } fn create_log_file(config: &Config, session_id: Uuid) -> std::io::Result { - // Resolve ~/.codex/sessions and create it if missing. - let mut dir = config.codex_home.clone(); - dir.push(SESSIONS_SUBDIR); - fs::create_dir_all(&dir)?; - + // Resolve ~/.codex/sessions/YYYY/MM/DD and create it if missing. let timestamp = OffsetDateTime::now_local() .map_err(|e| IoError::other(format!("failed to get local time: {e}")))?; + let mut dir = config.codex_home.clone(); + dir.push(SESSIONS_SUBDIR); + dir.push(timestamp.year().to_string()); + dir.push(format!("{:02}", u8::from(timestamp.month()))); + dir.push(format!("{:02}", timestamp.day())); + fs::create_dir_all(&dir)?; // Custom format for YYYY-MM-DDThh-mm-ss. Use `-` instead of `:` for // compatibility with filesystems that do not allow colons in filenames. diff --git a/codex-rs/core/tests/cli_stream.rs b/codex-rs/core/tests/cli_stream.rs index 9ef042eb1a..3669b93f51 100644 --- a/codex-rs/core/tests/cli_stream.rs +++ b/codex-rs/core/tests/cli_stream.rs @@ -2,7 +2,12 @@ use assert_cmd::Command as AssertCommand; use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use serde_json::Value; +use std::time::Duration; +use std::time::Instant; use tempfile::TempDir; +use uuid::Uuid; +use walkdir::WalkDir; use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -117,3 +122,154 @@ async fn responses_api_stream_cli() { let stdout = String::from_utf8_lossy(&output.stdout); assert!(stdout.contains("fixture hello")); } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn integration_creates_and_checks_session_file() { + // Honor sandbox network restrictions for CI parity with the other tests. + if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + + // 1. Temp home so we read/write isolated session files. + let home = TempDir::new().unwrap(); + + // 2. Unique marker we'll look for in the session log. + let marker = format!("integration-test-{}", Uuid::new_v4()); + let prompt = format!("echo {marker}"); + + // 3. Use the same offline SSE fixture as responses_api_stream_cli so the test is hermetic. + let fixture = + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/cli_responses_fixture.sse"); + + // 4. Run the codex CLI through cargo (ensures the right bin is built) and invoke `exec`, + // which is what records a session. + let mut cmd = AssertCommand::new("cargo"); + cmd.arg("run") + .arg("-p") + .arg("codex-cli") + .arg("--quiet") + .arg("--") + .arg("exec") + .arg("--skip-git-repo-check") + .arg("-C") + .arg(env!("CARGO_MANIFEST_DIR")) + .arg(&prompt); + cmd.env("CODEX_HOME", home.path()) + .env("OPENAI_API_KEY", "dummy") + .env("CODEX_RS_SSE_FIXTURE", &fixture) + // Required for CLI arg parsing even though fixture short-circuits network usage. + .env("OPENAI_BASE_URL", "http://unused.local"); + + let output = cmd.output().unwrap(); + assert!( + output.status.success(), + "codex-cli exec failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // 5. Sessions are written asynchronously; wait briefly for the directory to appear. + let sessions_dir = home.path().join("sessions"); + let start = Instant::now(); + while !sessions_dir.exists() && start.elapsed() < Duration::from_secs(2) { + std::thread::sleep(Duration::from_millis(50)); + } + + // 6. Scan all session files and find the one that contains our marker. + let mut matching_files = vec![]; + for entry in WalkDir::new(&sessions_dir) { + let entry = entry.unwrap(); + if entry.file_type().is_file() && entry.file_name().to_string_lossy().ends_with(".jsonl") { + let path = entry.path(); + let content = std::fs::read_to_string(path).unwrap(); + let mut lines = content.lines(); + // Skip SessionMeta (first line) + let _ = lines.next(); + for line in lines { + let item: Value = serde_json::from_str(line).unwrap(); + if let Some("message") = item.get("type").and_then(|t| t.as_str()) { + if let Some(content) = item.get("content") { + if content.to_string().contains(&marker) { + matching_files.push(path.to_owned()); + break; + } + } + } + } + } + } + assert_eq!( + matching_files.len(), + 1, + "Expected exactly one session file containing the marker, found {}", + matching_files.len() + ); + let path = &matching_files[0]; + + // 7. Verify directory structure: sessions/YYYY/MM/DD/filename.jsonl + let rel = match path.strip_prefix(&sessions_dir) { + Ok(r) => r, + Err(_) => panic!("session file should live under sessions/"), + }; + let comps: Vec = rel + .components() + .map(|c| c.as_os_str().to_string_lossy().into_owned()) + .collect(); + assert_eq!( + comps.len(), + 4, + "Expected sessions/YYYY/MM/DD/, got {rel:?}" + ); + let year = &comps[0]; + let month = &comps[1]; + let day = &comps[2]; + assert!( + year.len() == 4 && year.chars().all(|c| c.is_ascii_digit()), + "Year dir not 4-digit numeric: {year}" + ); + assert!( + month.len() == 2 && month.chars().all(|c| c.is_ascii_digit()), + "Month dir not zero-padded 2-digit numeric: {month}" + ); + assert!( + day.len() == 2 && day.chars().all(|c| c.is_ascii_digit()), + "Day dir not zero-padded 2-digit numeric: {day}" + ); + // Range checks (best-effort; won't fail on leading zeros) + if let Ok(m) = month.parse::() { + assert!((1..=12).contains(&m), "Month out of range: {m}"); + } + if let Ok(d) = day.parse::() { + assert!((1..=31).contains(&d), "Day out of range: {d}"); + } + + // 8. Parse SessionMeta line and basic sanity checks. + let content = std::fs::read_to_string(path).unwrap(); + let mut lines = content.lines(); + let meta: Value = serde_json::from_str(lines.next().unwrap()).unwrap(); + assert!(meta.get("id").is_some(), "SessionMeta missing id"); + assert!( + meta.get("timestamp").is_some(), + "SessionMeta missing timestamp" + ); + + // 9. Confirm at least one message contains the marker. + let mut found_message = false; + for line in lines { + let item: Value = serde_json::from_str(line).unwrap(); + if item.get("type").map(|t| t == "message").unwrap_or(false) { + if let Some(content) = item.get("content") { + if content.to_string().contains(&marker) { + found_message = true; + break; + } + } + } + } + assert!( + found_message, + "No message found in session file containing the marker" + ); +} From b95a010e86c9edf22626a7d41ed65e1c76fd8bd8 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 17 Jul 2025 11:35:38 -0700 Subject: [PATCH 6/8] fix: trim MCP tool names to fit into tool name length limit (#1571) Store fully qualified names along with tool entries so we don't have to re-parse them. Fixes: https://github.com/openai/codex/issues/1289 --- codex-rs/Cargo.lock | 66 ++++++++ codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/codex.rs | 3 +- codex-rs/core/src/mcp_connection_manager.rs | 171 +++++++++++++++++--- 4 files changed, 219 insertions(+), 22 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b1aa13a12c..a25e0f8be0 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -399,6 +399,15 @@ version = "2.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6099cdc01846bc367c4e7dd630dc5966dccf36b652fae7a74e17b640411a91b2" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bstr" version = "1.12.0" @@ -671,6 +680,7 @@ dependencies = [ "seccompiler", "serde", "serde_json", + "sha1", "strum_macros 0.27.1", "tempfile", "thiserror 2.0.12", @@ -933,6 +943,15 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + [[package]] name = "crc32fast" version = "1.4.2" @@ -1007,6 +1026,16 @@ version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "460fbee9c2c2f33933d720630a6a0bac33ba7053db5344fac858d4b8952d77d5" +[[package]] +name = "crypto-common" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "ctor" version = "0.1.26" @@ -1157,6 +1186,16 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "dirs" version = "6.0.0" @@ -1646,6 +1685,16 @@ dependencies = [ "byteorder", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "getopts" version = "0.2.23" @@ -3945,6 +3994,17 @@ dependencies = [ "syn 2.0.104", ] +[[package]] +name = "sha1" +version = "0.10.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3bf829a2d51ab4a5ddf1352d8470c140cadc8301b2ae1789db023f01cedd6ba" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -4852,6 +4912,12 @@ dependencies = [ "unicode-width 0.2.0", ] +[[package]] +name = "typenum" +version = "1.18.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1dccffe3ce07af9386bfd29e80c0ab1a8205a2fc34e4bcd40364df902cfa8f3f" + [[package]] name = "unicase" version = "2.8.1" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index ff066cc534..e192a71f39 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -28,6 +28,7 @@ rand = "0.9" reqwest = { version = "0.12", features = ["json", "stream"] } serde = { version = "1", features = ["derive"] } serde_json = "1" +sha1 = "0.10.6" strum_macros = "0.27.1" thiserror = "2.0.12" time = { version = "0.3", features = ["formatting", "local-offset", "macros"] } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5227f93c8e..d4e73b2ebf 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -51,7 +51,6 @@ use crate::exec::process_exec_tool_call; use crate::exec_env::create_env; use crate::flags::OPENAI_STREAM_MAX_RETRIES; use crate::mcp_connection_manager::McpConnectionManager; -use crate::mcp_connection_manager::try_parse_fully_qualified_tool_name; use crate::mcp_tool_call::handle_mcp_tool_call; use crate::models::ContentItem; use crate::models::FunctionCallOutputPayload; @@ -1292,7 +1291,7 @@ async fn handle_function_call( handle_container_exec_with_params(params, sess, sub_id, call_id).await } _ => { - match try_parse_fully_qualified_tool_name(&name) { + match sess.mcp_connection_manager.parse_tool_name(&name) { Some((server, tool_name)) => { // TODO(mbolin): Determine appropriate timeout for tool call. let timeout = None; diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 7cf6762752..c8161c9b90 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -7,6 +7,7 @@ //! `""` as the key. use std::collections::HashMap; +use std::collections::HashSet; use std::time::Duration; use anyhow::Context; @@ -16,8 +17,12 @@ use codex_mcp_client::McpClient; use mcp_types::ClientCapabilities; use mcp_types::Implementation; use mcp_types::Tool; + +use sha1::Digest; +use sha1::Sha1; use tokio::task::JoinSet; use tracing::info; +use tracing::warn; use crate::config_types::McpServerConfig; @@ -26,7 +31,8 @@ use crate::config_types::McpServerConfig; /// /// OpenAI requires tool names to conform to `^[a-zA-Z0-9_-]+$`, so we must /// choose a delimiter from this character set. -const MCP_TOOL_NAME_DELIMITER: &str = "__OAI_CODEX_MCP__"; +const MCP_TOOL_NAME_DELIMITER: &str = "__"; +const MAX_TOOL_NAME_LENGTH: usize = 64; /// Timeout for the `tools/list` request. const LIST_TOOLS_TIMEOUT: Duration = Duration::from_secs(10); @@ -35,16 +41,42 @@ const LIST_TOOLS_TIMEOUT: Duration = Duration::from_secs(10); /// spawned successfully. pub type ClientStartErrors = HashMap; -fn fully_qualified_tool_name(server: &str, tool: &str) -> String { - format!("{server}{MCP_TOOL_NAME_DELIMITER}{tool}") +fn qualify_tools(tools: Vec) -> HashMap { + let mut used_names = HashSet::new(); + let mut qualified_tools = HashMap::new(); + for tool in tools { + let mut qualified_name = format!( + "{}{}{}", + tool.server_name, MCP_TOOL_NAME_DELIMITER, tool.tool_name + ); + if qualified_name.len() > MAX_TOOL_NAME_LENGTH { + let mut hasher = Sha1::new(); + hasher.update(qualified_name.as_bytes()); + let sha1 = hasher.finalize(); + let sha1_str = format!("{sha1:x}"); + + // Truncate to make room for the hash suffix + let prefix_len = MAX_TOOL_NAME_LENGTH - sha1_str.len(); + + qualified_name = format!("{}{}", &qualified_name[..prefix_len], sha1_str); + } + + if used_names.contains(&qualified_name) { + warn!("skipping duplicated tool {}", qualified_name); + continue; + } + + used_names.insert(qualified_name.clone()); + qualified_tools.insert(qualified_name, tool); + } + + qualified_tools } -pub(crate) fn try_parse_fully_qualified_tool_name(fq_name: &str) -> Option<(String, String)> { - let (server, tool) = fq_name.split_once(MCP_TOOL_NAME_DELIMITER)?; - if server.is_empty() || tool.is_empty() { - return None; - } - Some((server.to_string(), tool.to_string())) +struct ToolInfo { + server_name: String, + tool_name: String, + tool: Tool, } /// A thin wrapper around a set of running [`McpClient`] instances. @@ -57,7 +89,7 @@ pub(crate) struct McpConnectionManager { clients: HashMap>, /// Fully qualified tool name -> tool instance. - tools: HashMap, + tools: HashMap, } impl McpConnectionManager { @@ -141,7 +173,9 @@ impl McpConnectionManager { } } - let tools = list_all_tools(&clients).await?; + let all_tools = list_all_tools(&clients).await?; + + let tools = qualify_tools(all_tools); Ok((Self { clients, tools }, errors)) } @@ -149,7 +183,10 @@ impl McpConnectionManager { /// Returns a single map that contains **all** tools. Each key is the /// fully-qualified name for the tool. pub fn list_all_tools(&self) -> HashMap { - self.tools.clone() + self.tools + .iter() + .map(|(name, tool)| (name.clone(), tool.tool.clone())) + .collect() } /// Invoke the tool indicated by the (server, tool) pair. @@ -171,13 +208,19 @@ impl McpConnectionManager { .await .with_context(|| format!("tool call failed for `{server}/{tool}`")) } + + pub fn parse_tool_name(&self, tool_name: &str) -> Option<(String, String)> { + self.tools + .get(tool_name) + .map(|tool| (tool.server_name.clone(), tool.tool_name.clone())) + } } /// Query every server for its available tools and return a single map that /// contains **all** tools. Each key is the fully-qualified name for the tool. -pub async fn list_all_tools( +async fn list_all_tools( clients: &HashMap>, -) -> Result> { +) -> Result> { let mut join_set = JoinSet::new(); // Spawn one task per server so we can query them concurrently. This @@ -194,18 +237,19 @@ pub async fn list_all_tools( }); } - let mut aggregated: HashMap = HashMap::with_capacity(join_set.len()); + let mut aggregated: Vec = Vec::with_capacity(join_set.len()); while let Some(join_res) = join_set.join_next().await { let (server_name, list_result) = join_res?; let list_result = list_result?; for tool in list_result.tools { - // TODO(mbolin): escape tool names that contain invalid characters. - let fq_name = fully_qualified_tool_name(&server_name, &tool.name); - if aggregated.insert(fq_name.clone(), tool).is_some() { - panic!("tool name collision for '{fq_name}': suspicious"); - } + let tool_info = ToolInfo { + server_name: server_name.clone(), + tool_name: tool.name.clone(), + tool, + }; + aggregated.push(tool_info); } } @@ -224,3 +268,90 @@ fn is_valid_mcp_server_name(server_name: &str) -> bool { .chars() .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') } + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + use mcp_types::ToolInputSchema; + + fn create_test_tool(server_name: &str, tool_name: &str) -> ToolInfo { + ToolInfo { + server_name: server_name.to_string(), + tool_name: tool_name.to_string(), + tool: Tool { + annotations: None, + description: Some(format!("Test tool: {tool_name}")), + input_schema: ToolInputSchema { + properties: None, + required: None, + r#type: "object".to_string(), + }, + name: tool_name.to_string(), + }, + } + } + + #[test] + fn test_qualify_tools_short_non_duplicated_names() { + let tools = vec![ + create_test_tool("server1", "tool1"), + create_test_tool("server1", "tool2"), + ]; + + let qualified_tools = qualify_tools(tools); + + assert_eq!(qualified_tools.len(), 2); + assert!(qualified_tools.contains_key("server1__tool1")); + assert!(qualified_tools.contains_key("server1__tool2")); + } + + #[test] + fn test_qualify_tools_duplicated_names_skipped() { + let tools = vec![ + create_test_tool("server1", "duplicate_tool"), + create_test_tool("server1", "duplicate_tool"), + ]; + + let qualified_tools = qualify_tools(tools); + + // Only the first tool should remain, the second is skipped + assert_eq!(qualified_tools.len(), 1); + assert!(qualified_tools.contains_key("server1__duplicate_tool")); + } + + #[test] + fn test_qualify_tools_long_names_same_server() { + let server_name = "my_server"; + + let tools = vec![ + create_test_tool( + server_name, + "extremely_lengthy_function_name_that_absolutely_surpasses_all_reasonable_limits", + ), + create_test_tool( + server_name, + "yet_another_extremely_lengthy_function_name_that_absolutely_surpasses_all_reasonable_limits", + ), + ]; + + let qualified_tools = qualify_tools(tools); + + assert_eq!(qualified_tools.len(), 2); + + let mut keys: Vec<_> = qualified_tools.keys().cloned().collect(); + keys.sort(); + + assert_eq!(keys[0].len(), 64); + assert_eq!( + keys[0], + "my_server__extremely_lena02e507efc5a9de88637e436690364fd4219e4ef" + ); + + assert_eq!(keys[1].len(), 64); + assert_eq!( + keys[1], + "my_server__yet_another_e1c3987bd9c50b826cbe1687966f79f0c602d19ca" + ); + } +} From 6949329a7fc66589b3355cf18c3cff4d94d21b1b Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 17 Jul 2025 11:40:00 -0700 Subject: [PATCH 7/8] chore: auto format code on save and add more details to AGENTS.md (#1582) Adds a default vscode config with generally applicable settings. Adds more entrypoints to justfile both for environment setup and to help agents better verify changes. --- .vscode/launch.json | 18 ++++++++++++++++++ .vscode/settings.json | 10 ++++++++++ AGENTS.md | 4 ++++ codex-rs/justfile | 6 ++++++ codex-rs/toolchain.toml | 3 +++ codex-rs/tui/src/app.rs | 2 +- .../src/bottom_pane/status_indicator_view.rs | 2 +- 7 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 .vscode/launch.json create mode 100644 .vscode/settings.json create mode 100644 codex-rs/toolchain.toml diff --git a/.vscode/launch.json b/.vscode/launch.json new file mode 100644 index 0000000000..618207f301 --- /dev/null +++ b/.vscode/launch.json @@ -0,0 +1,18 @@ +{ + "version": "0.2.0", + "configurations": [ + { + "type": "lldb", + "request": "launch", + "name": "Cargo launch", + "cargo": { + "cwd": "${workspaceFolder}/codex-rs", + "args": [ + "build", + "--bin=codex-tui" + ] + }, + "args": [] + } + ] +} diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000000..f66a12583c --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,10 @@ +{ + "rust-analyzer.checkOnSave": true, + "rust-analyzer.check.command": "clippy", + "rust-analyzer.check.extraArgs": ["--all-features", "--tests"], + "rust-analyzer.rustfmt.extraArgs": ["--config", "imports_granularity=Item"], + "[rust]": { + "editor.defaultFormatter": "rust-lang.rust-analyzer", + "editor.formatOnSave": true, + } +} diff --git a/AGENTS.md b/AGENTS.md index 1348e57824..4188cc9873 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,3 +3,7 @@ In the codex-rs folder where the rust code lives: - Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR`. You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. + +After making changes to the rust code run `just fmt` (in `codex-rs` directory) to format the code and `just fix` (in `codex-rs` directory) to fix any linter issues in the code. + +Ensure the test suite passes by running `cargo test --all-features` in the `codex-rs` directory. diff --git a/codex-rs/justfile b/codex-rs/justfile index 83a390ec56..6c8e9f9e4d 100644 --- a/codex-rs/justfile +++ b/codex-rs/justfile @@ -23,3 +23,9 @@ file-search *args: # format code fmt: cargo fmt -- --config imports_granularity=Item + +fix: + cargo clippy --fix --all-features --tests --allow-dirty + +install: + cargo fetch diff --git a/codex-rs/toolchain.toml b/codex-rs/toolchain.toml new file mode 100644 index 0000000000..72bafdf4b6 --- /dev/null +++ b/codex-rs/toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "1.88.0" +components = [ "clippy", "rustfmt", "rust-src"] diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 883250400d..ac69bef2e9 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -60,7 +60,7 @@ struct ChatWidgetArgs { initial_images: Vec, } -impl<'a> App<'a> { +impl App<'_> { pub(crate) fn new( config: Config, initial_prompt: Option, diff --git a/codex-rs/tui/src/bottom_pane/status_indicator_view.rs b/codex-rs/tui/src/bottom_pane/status_indicator_view.rs index d9ac57d7b9..de46ac2709 100644 --- a/codex-rs/tui/src/bottom_pane/status_indicator_view.rs +++ b/codex-rs/tui/src/bottom_pane/status_indicator_view.rs @@ -24,7 +24,7 @@ impl StatusIndicatorView { } } -impl<'a> BottomPaneView<'a> for StatusIndicatorView { +impl BottomPaneView<'_> for StatusIndicatorView { fn update_status_text(&mut self, text: String) -> ConditionalUpdate { self.update_text(text); ConditionalUpdate::NeedsRedraw From a825cc8f5ac44b3182ed9282ca2dcf87324ed5de Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 17 Jul 2025 12:14:27 -0700 Subject: [PATCH 8/8] feat: add --json flag to `codex exec` --- codex-rs/exec/src/cli.rs | 4 ++++ codex-rs/exec/src/lib.rs | 30 +++++++++++++++++++++++++----- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/codex-rs/exec/src/cli.rs b/codex-rs/exec/src/cli.rs index 613fedf0a1..53af25c7e9 100644 --- a/codex-rs/exec/src/cli.rs +++ b/codex-rs/exec/src/cli.rs @@ -51,6 +51,10 @@ pub struct Cli { #[arg(long = "color", value_enum, default_value_t = Color::Auto)] pub color: Color, + /// Print events to stdout as JSONL. + #[arg(long = "json", default_value_t = false)] + pub json: bool, + /// Specifies file where the last message from the agent should be written. #[arg(long = "output-last-message")] pub last_message_file: Option, diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index afefed1a93..257dbbfb00 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -36,6 +36,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any skip_git_repo_check, color, last_message_file, + json: json_mode, sandbox_mode: sandbox_mode_cli_arg, prompt, config_overrides, @@ -115,10 +116,15 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any }; let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides)?; - let mut event_processor = EventProcessor::create_with_ansi(stdout_with_ansi, &config); - // Print the effective configuration and prompt so users can see what Codex - // is using. - event_processor.print_config_summary(&config, &prompt); + let mut event_processor = if !json_mode { + let mut event_processor = EventProcessor::create_with_ansi(stdout_with_ansi, &config); + // Print the effective configuration and prompt so users can see what Codex + // is using. + event_processor.print_config_summary(&config, &prompt); + Some(event_processor) + } else { + None + }; if !skip_git_repo_check && !is_inside_git_repo(&config) { eprintln!("Not inside a Git repo and --skip-git-repo-check was not specified."); @@ -215,7 +221,21 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any } _ => (false, None), }; - event_processor.process_event(event); + if let Some(ref mut event_processor) = event_processor { + event_processor.process_event(event); + } else if json_mode { + // Skip streaming delta events; wait for full message. + match &event.msg { + EventMsg::AgentMessageDelta(_) | EventMsg::AgentReasoningDelta(_) => { + // Ignore streaming deltas in JSON mode. + } + _ => { + if let Ok(json_line) = serde_json::to_string(&event) { + println!("{json_line}"); + } + } + } + } if is_last_event { handle_last_message(last_assistant_message, last_message_file.as_deref())?; break;