From 74d2741729b4e4aee7e34ddf8c30de03e258250b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 Aug 2025 11:25:23 -0700 Subject: [PATCH 1/2] chore: require uninlined_format_args from clippy (#2845) - added `uninlined_format_args` to `[workspace.lints.clippy]` in the `Cargo.toml` for the workspace - ran `cargo clippy --tests --fix` - ran `just fmt` --- codex-rs/Cargo.toml | 1 + codex-rs/core/src/config.rs | 8 ++++---- codex-rs/core/src/environment_context.rs | 10 ++++------ codex-rs/core/src/error.rs | 6 +++--- .../core/src/exec_command/session_manager.rs | 19 +++++-------------- codex-rs/core/tests/suite/client.rs | 4 ++-- codex-rs/core/tests/suite/prompt_caching.rs | 2 +- codex-rs/exec/tests/suite/common.rs | 4 ++-- codex-rs/mcp-server/src/outgoing_message.rs | 2 +- 9 files changed, 23 insertions(+), 33 deletions(-) diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 8a48ef8187..4155992293 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -34,6 +34,7 @@ rust = {} [workspace.lints.clippy] expect_used = "deny" +uninlined_format_args = "deny" unwrap_used = "deny" [profile.release] diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 9b8f288cf3..4d623c3e5b 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -1317,9 +1317,9 @@ disable_response_storage = true let raw_path = project_dir.path().to_string_lossy(); let path_str = if raw_path.contains('\\') { - format!("'{}'", raw_path) + format!("'{raw_path}'") } else { - format!("\"{}\"", raw_path) + format!("\"{raw_path}\"") }; let expected = format!( r#"[projects.{path_str}] @@ -1340,9 +1340,9 @@ trust_level = "trusted" let config_path = codex_home.path().join(CONFIG_TOML_FILE); let raw_path = project_dir.path().to_string_lossy(); let path_str = if raw_path.contains('\\') { - format!("'{}'", raw_path) + format!("'{raw_path}'") } else { - format!("\"{}\"", raw_path) + format!("\"{raw_path}\"") }; // Use a quoted key so backslashes don't require escaping on Windows let initial = format!( diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/environment_context.rs index 1af4c9098a..b7ee862517 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -85,23 +85,21 @@ impl EnvironmentContext { } if let Some(approval_policy) = self.approval_policy { lines.push(format!( - " {}", - approval_policy + " {approval_policy}" )); } if let Some(sandbox_mode) = self.sandbox_mode { - lines.push(format!(" {}", sandbox_mode)); + lines.push(format!(" {sandbox_mode}")); } if let Some(network_access) = self.network_access { lines.push(format!( - " {}", - network_access + " {network_access}" )); } if let Some(shell) = self.shell && let Some(shell_name) = shell.name() { - lines.push(format!(" {}", shell_name)); + lines.push(format!(" {shell_name}")); } lines.push(ENVIRONMENT_CONTEXT_END.to_string()); lines.join("\n") diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index b05ff1a581..00ac145c2e 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -170,15 +170,15 @@ fn format_reset_duration(total_secs: u64) -> String { let mut parts: Vec = Vec::new(); if days > 0 { let unit = if days == 1 { "day" } else { "days" }; - parts.push(format!("{} {}", days, unit)); + parts.push(format!("{days} {unit}")); } if hours > 0 { let unit = if hours == 1 { "hour" } else { "hours" }; - parts.push(format!("{} {}", hours, unit)); + parts.push(format!("{hours} {unit}")); } if minutes > 0 { let unit = if minutes == 1 { "minute" } else { "minutes" }; - parts.push(format!("{} {}", minutes, unit)); + parts.push(format!("{minutes} {unit}")); } if parts.is_empty() { diff --git a/codex-rs/core/src/exec_command/session_manager.rs b/codex-rs/core/src/exec_command/session_manager.rs index 5359024bdd..c547409cd1 100644 --- a/codex-rs/core/src/exec_command/session_manager.rs +++ b/codex-rs/core/src/exec_command/session_manager.rs @@ -359,10 +359,7 @@ fn truncate_middle(s: &str, max_bytes: usize) -> (String, Option) { let est_tokens = (s.len() as u64).div_ceil(4); if max_bytes == 0 { // Cannot keep any content; still return a full marker (never truncated). - return ( - format!("…{} tokens truncated…", est_tokens), - Some(est_tokens), - ); + return (format!("…{est_tokens} tokens truncated…"), Some(est_tokens)); } // Helper to truncate a string to a given byte length on a char boundary. @@ -406,16 +403,13 @@ fn truncate_middle(s: &str, max_bytes: usize) -> (String, Option) { // Refine marker length and budgets until stable. Marker is never truncated. let mut guess_tokens = est_tokens; // worst-case: everything truncated for _ in 0..4 { - let marker = format!("…{} tokens truncated…", guess_tokens); + let marker = format!("…{guess_tokens} tokens truncated…"); let marker_len = marker.len(); let keep_budget = max_bytes.saturating_sub(marker_len); if keep_budget == 0 { // No room for any content within the cap; return a full, untruncated marker // that reflects the entire truncated content. - return ( - format!("…{} tokens truncated…", est_tokens), - Some(est_tokens), - ); + return (format!("…{est_tokens} tokens truncated…"), Some(est_tokens)); } let left_budget = keep_budget / 2; @@ -441,14 +435,11 @@ fn truncate_middle(s: &str, max_bytes: usize) -> (String, Option) { } // Fallback: use last guess to build output. - let marker = format!("…{} tokens truncated…", guess_tokens); + let marker = format!("…{guess_tokens} tokens truncated…"); let marker_len = marker.len(); let keep_budget = max_bytes.saturating_sub(marker_len); if keep_budget == 0 { - return ( - format!("…{} tokens truncated…", est_tokens), - Some(est_tokens), - ); + return (format!("…{est_tokens} tokens truncated…"), Some(est_tokens)); } let left_budget = keep_budget / 2; let right_budget = keep_budget - left_budget; diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 5a1fb35b12..aed34dc3a1 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -418,7 +418,7 @@ async fn prefers_chatgpt_token_when_config_prefers_chatgpt() { match CodexAuth::from_codex_home(codex_home.path(), config.preferred_auth_method) { Ok(Some(auth)) => codex_login::AuthManager::from_auth_for_testing(auth), Ok(None) => panic!("No CodexAuth found in codex_home"), - Err(e) => panic!("Failed to load CodexAuth: {}", e), + Err(e) => panic!("Failed to load CodexAuth: {e}"), }; let conversation_manager = ConversationManager::new(auth_manager); let NewConversation { @@ -499,7 +499,7 @@ async fn prefers_apikey_when_config_prefers_apikey_even_with_chatgpt_tokens() { match CodexAuth::from_codex_home(codex_home.path(), config.preferred_auth_method) { Ok(Some(auth)) => codex_login::AuthManager::from_auth_for_testing(auth), Ok(None) => panic!("No CodexAuth found in codex_home"), - Err(e) => panic!("Failed to load CodexAuth: {}", e), + Err(e) => panic!("Failed to load CodexAuth: {e}"), }; let conversation_manager = ConversationManager::new(auth_manager); let NewConversation { diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index b165c0bca5..999f807286 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -280,7 +280,7 @@ async fn prefixes_context_and_instructions_once_and_consistently_across_requests {}"#, cwd.path().to_string_lossy(), match shell.name() { - Some(name) => format!(" {}\n", name), + Some(name) => format!(" {name}\n"), None => String::new(), } ); diff --git a/codex-rs/exec/tests/suite/common.rs b/codex-rs/exec/tests/suite/common.rs index 49747dca05..8c57e7afcb 100644 --- a/codex-rs/exec/tests/suite/common.rs +++ b/codex-rs/exec/tests/suite/common.rs @@ -28,7 +28,7 @@ impl Respond for SeqResponder { Some(body) => wiremock::ResponseTemplate::new(200) .insert_header("content-type", "text/event-stream") .set_body_raw( - load_sse_fixture_with_id_from_str(body, &format!("request_{}", call_num)), + load_sse_fixture_with_id_from_str(body, &format!("request_{call_num}")), "text/event-stream", ), None => panic!("no response for {call_num}"), @@ -63,7 +63,7 @@ pub(crate) async fn run_e2e_exec_test(cwd: &Path, response_streams: Vec) .current_dir(cwd.clone()) .env("CODEX_HOME", cwd.clone()) .env("OPENAI_API_KEY", "dummy") - .env("OPENAI_BASE_URL", format!("{}/v1", uri)) + .env("OPENAI_BASE_URL", format!("{uri}/v1")) .arg("--skip-git-repo-check") .arg("-s") .arg("danger-full-access") diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index 16241a0899..5f206cb0cb 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -123,7 +123,7 @@ impl OutgoingMessageSender { } pub(crate) async fn send_server_notification(&self, notification: ServerNotification) { - let method = format!("codex/event/{}", notification); + let method = format!("codex/event/{notification}"); let params = match serde_json::to_value(¬ification) { Ok(serde_json::Value::Object(mut map)) => map.remove("data"), _ => None, From 354c072b5cab4cd1c76084aaba1bd80425b5397e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 Aug 2025 12:01:49 -0700 Subject: [PATCH 2/2] chore: try to make it easier to debug the flakiness of test_shell_command_approval_triggers_elicitation --- .../mcp-server/tests/common/mcp_process.rs | 19 +++++----- codex-rs/mcp-server/tests/suite/codex_tool.rs | 35 +++++++++++++------ 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 5788163c10..f350c590a1 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -283,6 +283,7 @@ impl McpProcess { } async fn send_jsonrpc_message(&mut self, message: JSONRPCMessage) -> anyhow::Result<()> { + eprintln!("writing message to stdin: {message:?}"); let payload = serde_json::to_string(&message)?; self.stdin.write_all(payload.as_bytes()).await?; self.stdin.write_all(b"\n").await?; @@ -294,13 +295,15 @@ impl McpProcess { let mut line = String::new(); self.stdout.read_line(&mut line).await?; let message = serde_json::from_str::(&line)?; + eprintln!("read message from stdout: {message:?}"); Ok(message) } pub async fn read_stream_until_request_message(&mut self) -> anyhow::Result { + eprintln!("in read_stream_until_request_message()"); + loop { let message = self.read_jsonrpc_message().await?; - eprint!("message: {message:?}"); match message { JSONRPCMessage::Notification(_) => { @@ -323,10 +326,10 @@ impl McpProcess { &mut self, request_id: RequestId, ) -> anyhow::Result { + eprintln!("in read_stream_until_response_message({request_id:?})"); + loop { let message = self.read_jsonrpc_message().await?; - eprint!("message: {message:?}"); - match message { JSONRPCMessage::Notification(_) => { eprintln!("notification: {message:?}"); @@ -352,8 +355,6 @@ impl McpProcess { ) -> anyhow::Result { loop { let message = self.read_jsonrpc_message().await?; - eprint!("message: {message:?}"); - match message { JSONRPCMessage::Notification(_) => { eprintln!("notification: {message:?}"); @@ -377,10 +378,10 @@ impl McpProcess { &mut self, method: &str, ) -> anyhow::Result { + eprintln!("in read_stream_until_notification_message({method})"); + loop { let message = self.read_jsonrpc_message().await?; - eprint!("message: {message:?}"); - match message { JSONRPCMessage::Notification(notification) => { if notification.method == method { @@ -405,10 +406,10 @@ impl McpProcess { pub async fn read_stream_until_legacy_task_complete_notification( &mut self, ) -> anyhow::Result { + eprintln!("in read_stream_until_legacy_task_complete_notification()"); + loop { let message = self.read_jsonrpc_message().await?; - eprint!("message: {message:?}"); - match message { JSONRPCMessage::Notification(notification) => { let is_match = if notification.method == "codex/event" { diff --git a/codex-rs/mcp-server/tests/suite/codex_tool.rs b/codex-rs/mcp-server/tests/suite/codex_tool.rs index 13866d970c..939637888c 100644 --- a/codex-rs/mcp-server/tests/suite/codex_tool.rs +++ b/codex-rs/mcp-server/tests/suite/codex_tool.rs @@ -30,7 +30,8 @@ use mcp_test_support::create_final_assistant_message_sse_response; use mcp_test_support::create_mock_chat_completions_server; use mcp_test_support::create_shell_sse_response; -const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); +// Allow ample time on slower CI or under load to avoid flakes. +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(20); /// Test that a shell command that is not on the "trusted" list triggers an /// elicitation request to the MCP and that sending the approval runs the @@ -52,9 +53,25 @@ async fn test_shell_command_approval_triggers_elicitation() { } async fn shell_command_approval_triggers_elicitation() -> anyhow::Result<()> { - // We use `git init` because it will not be on the "trusted" list. - let shell_command = vec!["git".to_string(), "init".to_string()]; + // Use a simple, untrusted command that creates a file so we can + // observe a side-effect. + // + // Cross‑platform approach: run a tiny Python snippet to touch the file + // using `python3 -c ...` on all platforms. let workdir_for_shell_function_call = TempDir::new()?; + let created_file = workdir_for_shell_function_call + .path() + .join("created_by_shell_tool.txt"); + let created_file_str = created_file.to_string_lossy().to_string(); + + let shell_command = vec![ + "python3".to_string(), + "-c".to_string(), + format!( + "import pathlib; pathlib.Path(r\"{}\").touch()", + created_file_str + ), + ]; let McpHandle { process: mut mcp_process, @@ -67,7 +84,7 @@ async fn shell_command_approval_triggers_elicitation() -> anyhow::Result<()> { Some(5_000), "call1234", )?, - create_final_assistant_message_sse_response("Enjoy your new git repo!")?, + create_final_assistant_message_sse_response("File created!")?, ]) .await?; @@ -122,8 +139,7 @@ async fn shell_command_approval_triggers_elicitation() -> anyhow::Result<()> { .expect("task_complete_notification timeout") .expect("task_complete_notification resp"); - // Verify the original `codex` tool call completes and that `git init` ran - // successfully. + // Verify the original `codex` tool call completes and that the file was created. let codex_response = timeout( DEFAULT_READ_TIMEOUT, mcp_process.read_stream_until_response_message(RequestId::Integer(codex_request_id)), @@ -136,7 +152,7 @@ async fn shell_command_approval_triggers_elicitation() -> anyhow::Result<()> { result: json!({ "content": [ { - "text": "Enjoy your new git repo!", + "text": "File created!", "type": "text" } ] @@ -145,10 +161,7 @@ async fn shell_command_approval_triggers_elicitation() -> anyhow::Result<()> { codex_response ); - assert!( - workdir_for_shell_function_call.path().join(".git").is_dir(), - ".git folder should have been created" - ); + assert!(created_file.is_file(), "created file should exist"); Ok(()) }