diff --git a/codex-rs/core/src/bash.rs b/codex-rs/core/src/bash.rs index 5b94daf252..05b5436ff3 100644 --- a/codex-rs/core/src/bash.rs +++ b/codex-rs/core/src/bash.rs @@ -1,3 +1,4 @@ +use tree_sitter::Node; use tree_sitter::Parser; use tree_sitter::Tree; use tree_sitter_bash::LANGUAGE as BASH; @@ -59,9 +60,6 @@ pub fn try_parse_word_only_commands_sequence(tree: &Tree, src: &str) -> Option Option Option Option> { +/// Extract the plain words of a simple command node, normalizing quoted +/// strings into their contents. Returns None if the node contains unsupported +/// constructs for a word-only command. +pub(crate) fn extract_words_from_command_node(cmd: Node, src: &str) -> Option> { if cmd.kind() != "command" { return None; } @@ -130,6 +131,48 @@ fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option Option> { + let root = tree.root_node(); + let mut cursor = root.walk(); + let mut stack = vec![root]; + let mut best: Option = None; + while let Some(node) = stack.pop() { + if node.is_named() + && node.kind() == "command" + && best.is_none_or(|b| node.start_byte() < b.start_byte()) + { + best = Some(node); + } + for child in node.children(&mut cursor) { + stack.push(child); + } + } + best +} + +/// Given the first command node, return the byte index in `src` at which the +/// remainder script starts, only if the next non-whitespace token is an allowed +/// sequencing operator for dropping the leading `cd`/`pushd`. +/// +/// Allowed operators: `&&` (conditional on success) and `;` (unconditional). +/// Disallowed: `||`, `|` — removing `cd` would change semantics. +pub(crate) fn remainder_start_after_wrapper_operator(first_cmd: Node, src: &str) -> Option { + let mut sib = first_cmd.next_sibling()?; + while !sib.is_named() && sib.kind().trim().is_empty() { + sib = sib.next_sibling()?; + } + if sib.is_named() || (sib.kind() != "&&" && sib.kind() != ";") { + return None; + } + let mut idx = sib.end_byte(); + let bytes = src.as_bytes(); + while idx < bytes.len() && bytes[idx].is_ascii_whitespace() { + idx += 1; + } + if idx >= bytes.len() { None } else { Some(idx) } +} + #[cfg(test)] mod tests { use super::*; @@ -215,4 +258,45 @@ mod tests { fn rejects_trailing_operator_parse_error() { assert!(parse_seq("ls &&").is_none()); } + + #[test] + fn find_first_command_node_finds_cd() { + let src = "cd foo && ls; git status"; + let tree = try_parse_bash(src).unwrap(); + let first = find_first_command_node(&tree).unwrap(); + let words = extract_words_from_command_node(first, src).unwrap(); + assert_eq!(words, vec!["cd".to_string(), "foo".to_string()]); + } + + #[test] + fn remainder_after_wrapper_operator_allows_and_and_semicolon() { + // Allows && + let src = "cd foo && ls; git status"; + let tree = try_parse_bash(src).unwrap(); + let first = find_first_command_node(&tree).unwrap(); + let idx = remainder_start_after_wrapper_operator(first, src).unwrap(); + assert_eq!(&src[idx..], "ls; git status"); + + // Allows ; + let src2 = "cd foo; ls"; + let tree2 = try_parse_bash(src2).unwrap(); + let first2 = find_first_command_node(&tree2).unwrap(); + let idx2 = remainder_start_after_wrapper_operator(first2, src2).unwrap(); + assert_eq!(&src2[idx2..], "ls"); + } + + #[test] + fn remainder_after_wrapper_operator_rejects_or_and_pipe() { + // Rejects || + let src = "cd foo || echo hi"; + let tree = try_parse_bash(src).unwrap(); + let first = find_first_command_node(&tree).unwrap(); + assert!(remainder_start_after_wrapper_operator(first, src).is_none()); + + // Rejects | + let src2 = "cd foo | rg bar"; + let tree2 = try_parse_bash(src2).unwrap(); + let first2 = find_first_command_node(&tree2).unwrap(); + assert!(remainder_start_after_wrapper_operator(first2, src2).is_none()); + } } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 375183ab82..e34b3f55dc 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -11,6 +11,7 @@ use std::time::Duration; use crate::AuthManager; use crate::client_common::REVIEW_PROMPT; use crate::event_mapping::map_response_item_to_event_messages; +use crate::normalize_command::try_normalize_command; use async_channel::Receiver; use async_channel::Sender; use codex_apply_patch::ApplyPatchAction; @@ -2774,6 +2775,25 @@ async fn handle_container_exec_with_params( ) } None => { + // Normalize a classic bash -lc "cd ... && ..." wrapper into updated + // cwd and a cleaner script for display, if applicable. Once we have + // confidence in this approach, we will use the normalized command + // for execution as well. + let (params, command_for_display) = + match try_normalize_command(¶ms.command, ¶ms.cwd) { + Some(norm) => { + let mut p = params; + p.command = norm.command; + p.cwd = norm.cwd; + (p, norm.command_for_display) + } + None => { + let p = params; + let command_for_display = p.command.clone(); + (p, command_for_display) + } + }; + let safety = { let state = sess.state.lock_unchecked(); assess_command_safety( @@ -2784,7 +2804,6 @@ async fn handle_container_exec_with_params( params.with_escalated_permissions.unwrap_or(false), ) }; - let command_for_display = params.command.clone(); (params, safety, command_for_display) } }; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 7b9c3dc9f0..d0c99d08c4 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -35,6 +35,7 @@ mod mcp_connection_manager; mod mcp_tool_call; mod message_history; mod model_provider_info; +mod normalize_command; pub mod parse_command; mod truncate; mod unified_exec; diff --git a/codex-rs/core/src/normalize_command.rs b/codex-rs/core/src/normalize_command.rs new file mode 100644 index 0000000000..1add8dfe30 --- /dev/null +++ b/codex-rs/core/src/normalize_command.rs @@ -0,0 +1,401 @@ +use crate::bash::extract_words_from_command_node; +use crate::bash::find_first_command_node; +use crate::bash::remainder_start_after_wrapper_operator; +use crate::bash::try_parse_bash; +use std::path::Path; +use std::path::PathBuf; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct NormalizedCommand { + pub command: Vec, + pub cwd: PathBuf, + pub command_for_display: Vec, +} + +/// Normalize a shell command for execution and display. +/// +/// For classic wrappers like `bash -lc "cd repo && git status"` returns: +/// - command: ["bash", "-lc", "git status"] +/// - cwd: original cwd joined with "repo" +/// - command_for_display: ["git status"] +/// +/// Returns None when no normalization is needed. +pub(crate) fn try_normalize_command(command: &[String], cwd: &Path) -> Option { + let invocation = parse_shell_script_from_shell_invocation(command)?; + let shell_script = invocation.command.clone(); + + let tree = try_parse_bash(&shell_script)?; + + let first_cmd = find_first_command_node(&tree)?; + let words = extract_words_from_command_node(first_cmd, &shell_script)?; + if !is_command_cd_to_directory(&words) { + return None; + } + + let idx = remainder_start_after_wrapper_operator(first_cmd, &shell_script)?; + let remainder = shell_script[idx..].to_string(); + + // Compute new cwd only after confirming the wrapper/operator shape + let dir = &words[1]; + let new_cwd = if Path::new(dir).is_absolute() { + PathBuf::from(dir) + } else { + // TODO(mbolin): Is there anything to worry about if `dir` is a + // symlink or contains `..` components? + cwd.join(dir) + }; + + Some(NormalizedCommand { + command: vec![invocation.executable, invocation.flag, remainder.clone()], + cwd: new_cwd, + command_for_display: vec![remainder], + }) +} + +/// This is similar to [`crate::shell::strip_bash_lc`] and should potentially +/// be unified with it. +fn parse_shell_script_from_shell_invocation(command: &[String]) -> Option { + // Allowed shells + fn is_allowed_exe(s: &str) -> bool { + matches!(s, "bash" | "/bin/bash" | "zsh" | "/bin/zsh") + } + match command { + // exactly three items: + [exe, flag, cmd] if is_allowed_exe(exe) && (flag == "-lc" || flag == "-c") => { + Some(ShellScriptInvocation { + executable: exe.clone(), + flag: flag.clone(), + command: cmd.clone(), + }) + } + // exactly four items: -l -c + [exe, flag1, flag2, cmd] if is_allowed_exe(exe) && flag1 == "-l" && flag2 == "-c" => { + Some(ShellScriptInvocation { + executable: exe.clone(), + flag: "-lc".to_string(), + command: cmd.clone(), + }) + } + _ => None, + } +} + +fn is_command_cd_to_directory(command: &[String]) -> bool { + matches!(command, [first, _dir] if first == "cd") +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct ShellScriptInvocation { + executable: String, + /// Single arg, so `-l -c` must be collapsed to `-lc`. + /// Must be written so `command` follows `flag`, so must + /// be `-lc` and not `-cl`. + flag: String, + command: String, +} + +#[cfg(test)] +mod tests { + use super::NormalizedCommand; + use super::try_normalize_command; + use pretty_assertions::assert_eq; + use std::path::PathBuf; + + #[test] + fn cd_prefix_in_bash_script_is_hidden() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo && echo hi".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "echo hi".into()], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["echo hi".into()], + }) + ); + } + + #[test] + fn cd_prefix_with_quoted_path_is_hidden() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + " cd 'foo bar' && ls".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "ls".into()], + cwd: PathBuf::from("/baz/foo bar"), + command_for_display: vec!["ls".into()], + }) + ); + } + + #[test] + fn cd_prefix_with_additional_cd_is_preserved() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo && cd bar && ls".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "cd bar && ls".into()], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["cd bar && ls".into()], + }) + ); + } + + #[test] + fn cd_prefix_with_or_connector_is_preserved() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd and_and || echo \"couldn't find the dir for &&\"".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + // Not a classic wrapper (uses ||), so no normalization. + assert_eq!(norm, None); + } + + #[test] + fn cd_prefix_preserves_operators_between_remaining_commands() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo && ls && git status".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "ls && git status".into()], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["ls && git status".into()], + }) + ); + } + + #[test] + fn cd_prefix_preserves_pipelines() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo && ls | rg foo".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "ls | rg foo".into()], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["ls | rg foo".into()], + }) + ); + } + + #[test] + fn cd_prefix_preserves_sequence_operator() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo && ls; git status".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "ls; git status".into()], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["ls; git status".into()], + }) + ); + } + + #[test] + fn cd_prefix_with_semicolon_is_hidden() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo; ls".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "ls".into()], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["ls".into()], + }) + ); + } + + #[test] + fn pushd_prefix_is_not_normalized() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "pushd foo && ls".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + // We do not normalize pushd because omitting pushd from the execution + // would have different semantics than the original command. + let norm = try_normalize_command(&command, &cwd); + assert_eq!(norm, None); + } + + #[test] + fn supports_zsh_and_bin_bash_and_split_flags() { + // zsh -lc + let zsh = vec!["zsh".into(), "-lc".into(), "cd foo && ls".into()]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&zsh, &cwd).unwrap(); + assert_eq!( + norm.command, + ["zsh", "-lc", "ls"] + .iter() + .map(|s| s.to_string()) + .collect::>() + ); + assert_eq!(norm.cwd, PathBuf::from("/baz/foo")); + assert_eq!(norm.command_for_display, vec!["ls".to_string()]); + + // /bin/bash -l -c + let bash_split = vec![ + "/bin/bash".into(), + "-l".into(), + "-c".into(), + "cd foo && ls".into(), + ]; + let norm2 = try_normalize_command(&bash_split, &cwd).unwrap(); + assert_eq!( + norm2.command, + ["/bin/bash", "-lc", "ls"] + .iter() + .map(|s| s.to_string()) + .collect::>() + ); + assert_eq!(norm2.cwd, PathBuf::from("/baz/foo")); + assert_eq!(norm2.command_for_display, vec!["ls".to_string()]); + } + + #[test] + fn supports_dash_c_flag() { + let command = vec!["bash".into(), "-c".into(), "cd foo && ls".into()]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd).unwrap(); + assert_eq!( + norm.command, + ["bash", "-c", "ls"] + .iter() + .map(|s| s.to_string()) + .collect::>() + ); + assert_eq!(norm.cwd, PathBuf::from("/baz/foo")); + assert_eq!(norm.command_for_display, vec!["ls".to_string()]); + } + + #[test] + fn rejects_pipe_operator_immediately_after_cd() { + let command = vec!["bash".into(), "-lc".into(), "cd foo | rg bar".into()]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!(norm, None); + } + + #[test] + fn rejects_unknown_shell_and_bad_flag_order() { + // Unknown shell + let unknown = vec!["sh".into(), "-lc".into(), "cd foo && ls".into()]; + let cwd = PathBuf::from("/baz"); + assert_eq!(try_normalize_command(&unknown, &cwd), None); + + // Bad flag order -cl (unsupported) + let bad_flag = vec!["bash".into(), "-cl".into(), "cd foo && ls".into()]; + assert_eq!(try_normalize_command(&bad_flag, &cwd), None); + } + + #[test] + fn absolute_directory_sets_absolute_cwd() { + let command = vec!["bash".into(), "-lc".into(), "cd /tmp && ls".into()]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd).unwrap(); + assert_eq!(norm.cwd, PathBuf::from("/tmp")); + assert_eq!( + norm.command, + ["bash", "-lc", "ls"] + .iter() + .map(|s| s.to_string()) + .collect::>() + ); + assert_eq!(norm.command_for_display, vec!["ls".to_string()]); + } + + #[test] + fn non_shell_command_is_returned_unmodified() { + let command = vec!["rg".to_string(), "--files".to_string()]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + // Not a shell wrapper, so no normalization. + assert_eq!(norm, None); + } + + #[test] + fn cd_prefix_with_or_operator_is_preserved() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd missing && ls || echo 'fallback'".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec!["bash".into(), "-lc".into(), "ls || echo 'fallback'".into()], + cwd: PathBuf::from("/baz/missing"), + command_for_display: vec!["ls || echo 'fallback'".into()], + }) + ); + } + + #[test] + fn cd_prefix_with_ampersands_in_string_is_hidden() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "cd foo && echo \"checking && markers\"".to_string(), + ]; + let cwd = PathBuf::from("/baz"); + let norm = try_normalize_command(&command, &cwd); + assert_eq!( + norm, + Some(NormalizedCommand { + command: vec![ + "bash".into(), + "-lc".into(), + "echo \"checking && markers\"".into() + ], + cwd: PathBuf::from("/baz/foo"), + command_for_display: vec!["echo \"checking && markers\"".into()], + }) + ); + } +}