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/Cargo.lock b/codex-rs/Cargo.lock index e59dbfa255..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", @@ -683,6 +693,7 @@ dependencies = [ "tree-sitter", "tree-sitter-bash", "uuid", + "walkdir", "wildmatch", "wiremock", ] @@ -932,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" @@ -1006,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" @@ -1156,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" @@ -1645,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" @@ -3944,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" @@ -4851,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 c55d7d395d..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"] } @@ -65,4 +66,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/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..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; @@ -61,7 +60,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 +104,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 +1122,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 +1166,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) @@ -1283,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 6ae1865f16..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 { @@ -79,9 +111,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 +159,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 @@ -132,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)) } @@ -140,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. @@ -162,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 @@ -185,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); } } @@ -208,3 +261,97 @@ 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 == '-') +} + +#[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" + ); + } +} 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/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 df3fedfd48..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; @@ -71,8 +76,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; } @@ -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" + ); +} 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/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/event_processor.rs b/codex-rs/exec/src/event_processor.rs index 540e014298..5ab09994b1 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; @@ -21,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 @@ -50,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(); @@ -70,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 { @@ -84,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, } } } @@ -184,12 +193,45 @@ impl EventProcessor { EventMsg::TokenCount(TokenUsage { total_tokens, .. }) => { ts_println!(self, "tokens used: {total_tokens}"); } + 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 }) => { + 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, @@ -343,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 } => { @@ -441,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..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,11 +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.hide_agent_reasoning); - // 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."); @@ -216,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; 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/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/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 33297ad372..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, @@ -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/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 diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 51fdfc3e8a..860439ffb6 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; @@ -49,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)] @@ -135,6 +139,8 @@ impl ChatWidget<'_> { initial_images, ), token_usage: TokenUsage::default(), + reasoning_buffer: String::new(), + answer_buffer: String::new(), } } @@ -240,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(); 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 {