mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[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`
This commit is contained in:
committed by
GitHub
Unverified
parent
bbcfed8ac2
commit
42ad752f36
@@ -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<String>, PathBuf)> {
|
||||
fn shell_script_for_invocation(invocation: &ToolInvocation) -> Option<String> {
|
||||
let ToolPayload::Function { arguments } = &invocation.payload else {
|
||||
return None;
|
||||
};
|
||||
@@ -42,39 +37,10 @@ fn shell_command_for_invocation(invocation: &ToolInvocation) -> Option<(Vec<Stri
|
||||
) {
|
||||
(None, "shell_command") => serde_json::from_str::<ShellCommandToolCallParams>(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::<ExecCommandArgs>(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,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,9 +26,7 @@ pub use write_stdin::WriteStdinHandler;
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
pub(crate) struct ExecCommandArgs {
|
||||
cmd: String,
|
||||
#[serde(default)]
|
||||
pub(crate) workdir: Option<String>,
|
||||
pub(crate) cmd: String,
|
||||
#[serde(default)]
|
||||
shell: Option<String>,
|
||||
#[serde(default)]
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<MemoriesUsageKind> {
|
||||
if !is_known_safe_command(command) {
|
||||
pub fn memories_usage_kinds_from_command(command: &str) -> Vec<MemoriesUsageKind> {
|
||||
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()),
|
||||
|
||||
@@ -94,6 +94,12 @@ pub fn try_parse_word_only_commands_sequence(tree: &Tree, src: &str) -> Option<V
|
||||
Some(commands)
|
||||
}
|
||||
|
||||
/// Parses a shell script consisting only of plain commands joined by safe operators.
|
||||
pub fn parse_shell_script_into_commands(script: &str) -> Option<Vec<Vec<String>>> {
|
||||
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<Vec<Vec<String>>> {
|
||||
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<Vec<Vec<String>>> {
|
||||
let tree = try_parse_shell(src)?;
|
||||
try_parse_word_only_commands_sequence(&tree, src)
|
||||
parse_shell_script_into_commands(src)
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -1818,7 +1818,11 @@ fn parse_find_query_and_path(tail: &[String]) -> (Option<String>, Option<String>
|
||||
fn parse_shell_lc_commands(original: &[String]) -> Option<Vec<ParsedCommand>> {
|
||||
// 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<ParsedCommand> {
|
||||
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<Vec<ParsedCommand>> {
|
||||
// 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<ParsedCommand> = Vec::new();
|
||||
@@ -1939,11 +1943,11 @@ fn parse_shell_lc_commands(original: &[String]) -> Option<Vec<ParsedCommand>> {
|
||||
})
|
||||
.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.
|
||||
|
||||
Reference in New Issue
Block a user