From 42ad752f36190026b5b697530ada2805eb0e0ef2 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 15 Jun 2026 08:28:02 -0700 Subject: [PATCH] [codex] simplify memory read metrics (#28164) ## Why Memory read telemetry currently reconstructs the executable shell command after a tool call finishes. That duplicates shell, login-policy, and cwd resolution owned by the tool handlers, and can diverge from the environment-specific command that unified exec actually ran. ## What changed - Expose the existing restricted shell-script parser directly for raw script text. - Parse `shell_command` and `exec_command` input into plain command argv before classifying memory reads. - Preserve all-or-nothing safe-command validation for multi-command scripts. - Remove cwd resolution, shell selection, and the unnecessary async boundary from memory read metric emission. ## Testing - `just test -p codex-shell-command` - `cargo check -p codex-core` --- codex-rs/core/src/memory_usage.rs | 46 +++---------------- .../core/src/tools/handlers/unified_exec.rs | 4 +- codex-rs/core/src/tools/registry.rs | 2 +- codex-rs/memories/read/src/usage.rs | 15 ++++-- codex-rs/shell-command/src/bash.rs | 13 ++++-- codex-rs/shell-command/src/parse_command.rs | 14 ++++-- 6 files changed, 36 insertions(+), 58 deletions(-) diff --git a/codex-rs/core/src/memory_usage.rs b/codex-rs/core/src/memory_usage.rs index 9e9601c73..4655b6393 100644 --- a/codex-rs/core/src/memory_usage.rs +++ b/codex-rs/core/src/memory_usage.rs @@ -5,20 +5,15 @@ use crate::tools::handlers::unified_exec::ExecCommandArgs; use codex_memories_read::usage::MEMORIES_USAGE_METRIC; use codex_memories_read::usage::memories_usage_kinds_from_command; use codex_protocol::models::ShellCommandToolCallParams; -use std::path::PathBuf; -pub(crate) async fn emit_metric_for_tool_read(invocation: &ToolInvocation, success: bool) { - let Some((command, _)) = shell_command_for_invocation(invocation) else { +pub(crate) fn emit_metric_for_tool_read(invocation: &ToolInvocation, success: bool) { + let Some(command) = shell_script_for_invocation(invocation) else { return; }; - let kinds = memories_usage_kinds_from_command(&command); - if kinds.is_empty() { - return; - } let success = if success { "true" } else { "false" }; let tool_name = flat_tool_name(&invocation.tool_name); - for kind in kinds { + for kind in memories_usage_kinds_from_command(&command) { invocation.turn.session_telemetry.counter( MEMORIES_USAGE_METRIC, /*inc*/ 1, @@ -31,7 +26,7 @@ pub(crate) async fn emit_metric_for_tool_read(invocation: &ToolInvocation, succe } } -fn shell_command_for_invocation(invocation: &ToolInvocation) -> Option<(Vec, PathBuf)> { +fn shell_script_for_invocation(invocation: &ToolInvocation) -> Option { let ToolPayload::Function { arguments } = &invocation.payload else { return None; }; @@ -42,39 +37,10 @@ fn shell_command_for_invocation(invocation: &ToolInvocation) -> Option<(Vec serde_json::from_str::(arguments) .ok() - .map(|params| { - if !invocation.turn.config.permissions.allow_login_shell - && params.login == Some(true) - { - #[allow(deprecated)] - let cwd = invocation.turn.resolve_path(params.workdir).to_path_buf(); - return (Vec::new(), cwd); - } - let use_login_shell = params - .login - .unwrap_or(invocation.turn.config.permissions.allow_login_shell); - let command = invocation - .session - .user_shell() - .derive_exec_args(¶ms.command, use_login_shell); - #[allow(deprecated)] - let cwd = invocation.turn.resolve_path(params.workdir).to_path_buf(); - (command, cwd) - }), + .map(|params| params.command), (None, "exec_command") => serde_json::from_str::(arguments) .ok() - .and_then(|params| { - let command = crate::tools::handlers::unified_exec::get_command( - ¶ms, - invocation.session.user_shell(), - &invocation.turn.unified_exec_shell_mode, - invocation.turn.config.permissions.allow_login_shell, - ) - .ok()?; - #[allow(deprecated)] - let cwd = invocation.turn.resolve_path(params.workdir).to_path_buf(); - Some((command.command, cwd)) - }), + .map(|params| params.cmd), (Some(_), _) | (None, _) => None, } } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 1c6252a1d..71e776dbf 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -26,9 +26,7 @@ pub use write_stdin::WriteStdinHandler; #[derive(Debug, Deserialize)] pub(crate) struct ExecCommandArgs { - cmd: String, - #[serde(default)] - pub(crate) workdir: Option, + pub(crate) cmd: String, #[serde(default)] shell: Option, #[serde(default)] diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 76e3f5f34..c027bd16b 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -569,7 +569,7 @@ impl ToolRegistry { Ok((_, success)) => *success, Err(_) => false, }; - emit_metric_for_tool_read(&invocation, success).await; + emit_metric_for_tool_read(&invocation, success); let post_tool_use_payload = if success { let guard = response_cell.lock().await; guard diff --git a/codex-rs/memories/read/src/usage.rs b/codex-rs/memories/read/src/usage.rs index 277fb800d..869153148 100644 --- a/codex-rs/memories/read/src/usage.rs +++ b/codex-rs/memories/read/src/usage.rs @@ -1,6 +1,7 @@ use codex_protocol::parse_command::ParsedCommand; +use codex_shell_command::bash::parse_shell_script_into_commands; use codex_shell_command::is_safe_command::is_known_safe_command; -use codex_shell_command::parse_command::parse_command; +use codex_shell_command::parse_command::parse_shell_script; pub use crate::metrics::MEMORIES_USAGE_METRIC; @@ -25,12 +26,18 @@ impl MemoriesUsageKind { } } -pub fn memories_usage_kinds_from_command(command: &[String]) -> Vec { - if !is_known_safe_command(command) { +pub fn memories_usage_kinds_from_command(command: &str) -> Vec { + let Some(commands) = parse_shell_script_into_commands(command) else { + return Vec::new(); + }; + if !commands + .iter() + .all(|command| is_known_safe_command(command)) + { return Vec::new(); } - parse_command(command) + parse_shell_script(command) .into_iter() .filter_map(|command| match command { ParsedCommand::Read { path, .. } => get_memory_kind(path.display().to_string()), diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index b25d3fd37..007fbf956 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -94,6 +94,12 @@ pub fn try_parse_word_only_commands_sequence(tree: &Tree, src: &str) -> Option Option>> { + let tree = try_parse_shell(script)?; + try_parse_word_only_commands_sequence(&tree, script) +} + pub fn extract_bash_command(command: &[String]) -> Option<(&str, &str)> { let [shell, flag, script] = command else { return None; @@ -114,9 +120,7 @@ pub fn extract_bash_command(command: &[String]) -> Option<(&str, &str)> { /// joined by safe operators. pub fn parse_shell_lc_plain_commands(command: &[String]) -> Option>> { let (_, script) = extract_bash_command(command)?; - - let tree = try_parse_shell(script)?; - try_parse_word_only_commands_sequence(&tree, script) + parse_shell_script_into_commands(script) } /// Returns the parsed argv for a single shell command in a here-doc style @@ -322,8 +326,7 @@ mod tests { use pretty_assertions::assert_eq; fn parse_seq(src: &str) -> Option>> { - let tree = try_parse_shell(src)?; - try_parse_word_only_commands_sequence(&tree, src) + parse_shell_script_into_commands(src) } #[test] diff --git a/codex-rs/shell-command/src/parse_command.rs b/codex-rs/shell-command/src/parse_command.rs index 1c8ac9948..72e1aca67 100644 --- a/codex-rs/shell-command/src/parse_command.rs +++ b/codex-rs/shell-command/src/parse_command.rs @@ -1818,7 +1818,11 @@ fn parse_find_query_and_path(tail: &[String]) -> (Option, Option fn parse_shell_lc_commands(original: &[String]) -> Option> { // Only handle bash/zsh here; PowerShell is stripped separately without bash parsing. let (_, script) = extract_bash_command(original)?; + Some(parse_shell_script(script)) +} +/// Parses command metadata from a Bash-compatible shell script. +pub fn parse_shell_script(script: &str) -> Vec { if let Some(tree) = try_parse_shell(script) && let Some(all_commands) = try_parse_word_only_commands_sequence(&tree, script) && !all_commands.is_empty() @@ -1831,9 +1835,9 @@ fn parse_shell_lc_commands(original: &[String]) -> Option> { // Commands arrive in source order; drop formatting helpers while preserving it. let filtered_commands = drop_small_formatting_commands(all_commands); if filtered_commands.is_empty() { - return Some(vec![ParsedCommand::Unknown { + return vec![ParsedCommand::Unknown { cmd: script.to_string(), - }]); + }]; } // Build parsed commands, tracking `cd` segments to compute effective file paths. let mut commands: Vec = Vec::new(); @@ -1939,11 +1943,11 @@ fn parse_shell_lc_commands(original: &[String]) -> Option> { }) .collect(); } - return Some(commands); + return commands; } - Some(vec![ParsedCommand::Unknown { + vec![ParsedCommand::Unknown { cmd: script.to_string(), - }]) + }] } /// Return true if this looks like a small formatting helper in a pipeline.