diff --git a/codex-rs/tui/src/chatwidget/slash_dispatch.rs b/codex-rs/tui/src/chatwidget/slash_dispatch.rs index 30169904d7..6d1278ea2d 100644 --- a/codex-rs/tui/src/chatwidget/slash_dispatch.rs +++ b/codex-rs/tui/src/chatwidget/slash_dispatch.rs @@ -328,16 +328,25 @@ impl ChatWidget { SlashCommand::Diff => { self.add_diff_in_progress(); let tx = self.app_event_tx.clone(); + let runner = self.workspace_command_runner.clone(); + let cwd = self + .current_cwd + .clone() + .unwrap_or_else(|| self.config.cwd.to_path_buf()); tokio::spawn(async move { - let text = match get_git_diff().await { - Ok((is_git_repo, diff_text)) => { - if is_git_repo { - diff_text - } else { - "`/diff` — _not inside a git repository_".to_string() + let text = match runner { + Some(runner) => match get_git_diff(runner.as_ref(), &cwd).await { + Ok((is_git_repo, diff_text)) => { + if is_git_repo { + diff_text + } else { + "`/diff` — _not inside a git repository_".to_string() + } } - } - Err(e) => format!("Failed to compute diff: {e}"), + Err(e) => format!("Failed to compute diff: {e}"), + }, + None => "Failed to compute diff: workspace command runner unavailable" + .to_string(), }; tx.send(AppEvent::DiffResult(text)); }); diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index 78ab53d92f..a7b4b668fb 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -5,25 +5,32 @@ //! untracked files. When the current directory is not inside a Git //! repository, the function returns `Ok((false, String::new()))`. -use std::io; use std::path::Path; -use std::process::Stdio; -use tokio::process::Command; +use std::time::Duration; + +use crate::workspace_command::WorkspaceCommand; +use crate::workspace_command::WorkspaceCommandExecutor; +use crate::workspace_command::WorkspaceCommandOutput; + +const DIFF_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); /// Return value of [`get_git_diff`]. /// /// * `bool` – Whether the current working directory is inside a Git repo. /// * `String` – The concatenated diff (may be empty). -pub(crate) async fn get_git_diff() -> io::Result<(bool, String)> { +pub(crate) async fn get_git_diff( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, +) -> Result<(bool, String), String> { // First check if we are inside a Git repository. - if !inside_git_repo().await? { + if !inside_git_repo(runner, cwd).await? { return Ok((false, String::new())); } // Run tracked diff and untracked file listing in parallel. let (tracked_diff_res, untracked_output_res) = tokio::join!( - run_git_capture_diff(&["diff", "--color"]), - run_git_capture_stdout(&["ls-files", "--others", "--exclude-standard"]), + run_git_capture_diff(runner, cwd, &["diff", "--color"]), + run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]), ); let tracked_diff = tracked_diff_res?; let untracked_output = untracked_output_res?; @@ -35,27 +42,15 @@ pub(crate) async fn get_git_diff() -> io::Result<(bool, String)> { Path::new("/dev/null") }; - let null_path = null_device.to_str().unwrap_or("/dev/null").to_string(); - let mut join_set: tokio::task::JoinSet> = tokio::task::JoinSet::new(); + let null_path = null_device.to_str().unwrap_or("/dev/null"); for file in untracked_output .split('\n') .map(str::trim) .filter(|s| !s.is_empty()) { - let null_path = null_path.clone(); - let file = file.to_string(); - join_set.spawn(async move { - let args = ["diff", "--color", "--no-index", "--", &null_path, &file]; - run_git_capture_diff(&args).await - }); - } - while let Some(res) = join_set.join_next().await { - match res { - Ok(Ok(diff)) => untracked_diff.push_str(&diff), - Ok(Err(err)) if err.kind() == io::ErrorKind::NotFound => {} - Ok(Err(err)) => return Err(err), - Err(_) => {} - } + let args = ["diff", "--color", "--no-index", "--", null_path, file]; + let diff = run_git_capture_diff(runner, cwd, &args).await?; + untracked_diff.push_str(&diff); } Ok((true, format!("{tracked_diff}{untracked_diff}"))) @@ -63,57 +58,282 @@ pub(crate) async fn get_git_diff() -> io::Result<(bool, String)> { /// Helper that executes `git` with the given `args` and returns `stdout` as a /// UTF-8 string. Any non-zero exit status is considered an *error*. -async fn run_git_capture_stdout(args: &[&str]) -> io::Result { - let output = Command::new("git") - .args(args) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .output() - .await?; - - if output.status.success() { - Ok(String::from_utf8_lossy(&output.stdout).into_owned()) +async fn run_git_capture_stdout( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, + args: &[&str], +) -> Result { + let output = run_git_command(runner, cwd, args).await?; + if output.success() { + Ok(output.stdout) } else { - Err(io::Error::other(format!( + Err(format!( "git {:?} failed with status {}", - args, output.status - ))) + args, output.exit_code + )) } } /// Like [`run_git_capture_stdout`] but treats exit status 1 as success and /// returns stdout. Git returns 1 for diffs when differences are present. -async fn run_git_capture_diff(args: &[&str]) -> io::Result { - let output = Command::new("git") - .args(args) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .output() - .await?; - - if output.status.success() || output.status.code() == Some(1) { - Ok(String::from_utf8_lossy(&output.stdout).into_owned()) +async fn run_git_capture_diff( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, + args: &[&str], +) -> Result { + let output = run_git_command(runner, cwd, args).await?; + if output.success() || output.exit_code == 1 { + Ok(output.stdout) } else { - Err(io::Error::other(format!( + Err(format!( "git {:?} failed with status {}", - args, output.status - ))) + args, output.exit_code + )) } } /// Determine if the current directory is inside a Git repository. -async fn inside_git_repo() -> io::Result { - let status = Command::new("git") - .args(["rev-parse", "--is-inside-work-tree"]) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .await; +async fn inside_git_repo( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, +) -> Result { + let output = run_git_command(runner, cwd, &["rev-parse", "--is-inside-work-tree"]).await?; + Ok(output.success()) +} - match status { - Ok(s) if s.success() => Ok(true), - Ok(_) => Ok(false), - Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(false), // git not installed - Err(e) => Err(e), +async fn run_git_command( + runner: &dyn WorkspaceCommandExecutor, + cwd: &Path, + args: &[&str], +) -> Result { + let mut argv = Vec::with_capacity(args.len() + 1); + argv.push("git".to_string()); + argv.extend(args.iter().map(|arg| (*arg).to_string())); + runner + .run( + WorkspaceCommand::new(argv) + .cwd(cwd.to_path_buf()) + .timeout(DIFF_COMMAND_TIMEOUT) + .disable_output_cap(), + ) + .await + .map_err(|err| err.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::workspace_command::WorkspaceCommandError; + use pretty_assertions::assert_eq; + use std::collections::VecDeque; + use std::future::Future; + use std::path::PathBuf; + use std::pin::Pin; + use std::sync::Mutex; + + #[tokio::test] + async fn get_git_diff_returns_not_git_for_non_git_cwd() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![response( + &["git", "rev-parse", "--is-inside-work-tree"], + /*exit_code*/ 128, + "", + )]); + + let result = get_git_diff(&runner, &cwd).await; + + assert_eq!(result, Ok((false, String::new()))); + assert_commands( + &runner.commands(), + &[&["git", "rev-parse", "--is-inside-work-tree"]], + &cwd, + ); + } + + #[tokio::test] + async fn get_git_diff_concatenates_tracked_and_untracked_diffs() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![ + response( + &["git", "rev-parse", "--is-inside-work-tree"], + /*exit_code*/ 0, + "true\n", + ), + response( + &["git", "diff", "--color"], + /*exit_code*/ 1, + "tracked\n", + ), + response( + &["git", "ls-files", "--others", "--exclude-standard"], + /*exit_code*/ 0, + "new.txt\n", + ), + response( + &[ + "git", + "diff", + "--color", + "--no-index", + "--", + null_device(), + "new.txt", + ], + /*exit_code*/ 1, + "untracked\n", + ), + ]); + + let result = get_git_diff(&runner, &cwd).await; + + assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string()))); + assert_commands( + &runner.commands(), + &[ + &["git", "rev-parse", "--is-inside-work-tree"], + &["git", "diff", "--color"], + &["git", "ls-files", "--others", "--exclude-standard"], + &[ + "git", + "diff", + "--color", + "--no-index", + "--", + null_device(), + "new.txt", + ], + ], + &cwd, + ); + } + + #[tokio::test] + async fn get_git_diff_accepts_diff_exit_code_one() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![ + response( + &["git", "rev-parse", "--is-inside-work-tree"], + /*exit_code*/ 0, + "true\n", + ), + response( + &["git", "diff", "--color"], + /*exit_code*/ 1, + "tracked\n", + ), + response( + &["git", "ls-files", "--others", "--exclude-standard"], + /*exit_code*/ 0, + "", + ), + ]); + + let result = get_git_diff(&runner, &cwd).await; + + assert_eq!(result, Ok((true, "tracked\n".to_string()))); + } + + #[tokio::test] + async fn get_git_diff_rejects_unexpected_git_diff_status() { + let cwd = PathBuf::from("/workspace"); + let runner = FakeRunner::new(vec![ + response( + &["git", "rev-parse", "--is-inside-work-tree"], + /*exit_code*/ 0, + "true\n", + ), + response(&["git", "diff", "--color"], /*exit_code*/ 2, ""), + response( + &["git", "ls-files", "--others", "--exclude-standard"], + /*exit_code*/ 0, + "", + ), + ]); + + let error = get_git_diff(&runner, &cwd) + .await + .expect_err("unexpected git diff status should fail"); + + assert!( + error.contains("git [\"diff\", \"--color\"] failed with status 2"), + "unexpected error: {error}", + ); + } + + fn response(argv: &[&str], exit_code: i32, stdout: &str) -> FakeResponse { + FakeResponse { + argv: argv.iter().map(|arg| (*arg).to_string()).collect(), + output: WorkspaceCommandOutput { + exit_code, + stdout: stdout.to_string(), + stderr: String::new(), + }, + } + } + + fn null_device() -> &'static str { + if cfg!(windows) { "NUL" } else { "/dev/null" } + } + + fn assert_commands(commands: &[WorkspaceCommand], expected: &[&[&str]], cwd: &Path) { + let actual: Vec> = commands + .iter() + .map(|command| command.argv.clone()) + .collect(); + let expected: Vec> = expected + .iter() + .map(|argv| argv.iter().map(|arg| (*arg).to_string()).collect()) + .collect(); + assert_eq!(actual, expected); + + for command in commands { + assert_eq!(command.cwd.as_deref(), Some(cwd)); + assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT); + assert!(command.disable_output_cap); + } + } + + struct FakeResponse { + argv: Vec, + output: WorkspaceCommandOutput, + } + + struct FakeRunner { + responses: Mutex>, + commands: Mutex>, + } + + impl FakeRunner { + fn new(responses: Vec) -> Self { + Self { + responses: Mutex::new(responses.into()), + commands: Mutex::new(Vec::new()), + } + } + + fn commands(&self) -> Vec { + self.commands.lock().expect("commands lock").clone() + } + } + + impl WorkspaceCommandExecutor for FakeRunner { + fn run( + &self, + command: WorkspaceCommand, + ) -> Pin< + Box< + dyn Future> + + Send + + '_, + >, + > { + Box::pin(async move { + let mut responses = self.responses.lock().expect("responses lock"); + let response = responses.pop_front().expect("missing fake response"); + assert_eq!(command.argv, response.argv); + self.commands.lock().expect("commands lock").push(command); + Ok(response.output) + }) + } } } diff --git a/codex-rs/tui/src/workspace_command.rs b/codex-rs/tui/src/workspace_command.rs index f0267699a2..c6b2e770e6 100644 --- a/codex-rs/tui/src/workspace_command.rs +++ b/codex-rs/tui/src/workspace_command.rs @@ -1,14 +1,14 @@ //! App-server-backed workspace command execution for TUI-owned background lookups. //! -//! This module is the TUI boundary for short, non-interactive commands that need to run wherever +//! This module is the TUI boundary for non-interactive commands that need to run wherever //! the active workspace lives. Callers describe a command in terms of argv, cwd, environment //! overrides, timeout, and output cap; the runner translates that request to app-server //! `command/exec`. Keeping this as a TUI-local abstraction lets status surfaces avoid knowing //! whether the current app-server is embedded or remote. //! -//! Commands sent through this path are best-effort metadata probes. They should not prompt for -//! stdin, should tolerate failure by omitting optional UI, and should keep output bounded so a -//! status-line refresh cannot grow into an unbounded background process. +//! Commands sent through this path should not prompt for stdin. Most callers should keep output +//! bounded so metadata refreshes cannot grow into unbounded background processes; callers that own a +//! full user-visible payload, such as `/diff`, can explicitly opt out of output capping. use std::collections::HashMap; use std::future::Future; @@ -45,17 +45,20 @@ pub(crate) struct WorkspaceCommand { pub(crate) timeout: Duration, /// Maximum captured stdout/stderr bytes returned by app-server. pub(crate) output_bytes_cap: usize, + /// Whether app-server should return uncapped stdout/stderr. + pub(crate) disable_output_cap: bool, } impl WorkspaceCommand { - /// Creates a workspace command with conservative defaults for status-style metadata probes. + /// Creates a workspace command with conservative defaults for metadata probes. pub(crate) fn new(argv: impl IntoIterator>) -> Self { Self { argv: argv.into_iter().map(Into::into).collect(), cwd: None, env: HashMap::new(), - timeout: Duration::from_secs(5), + timeout: Duration::from_secs(/*secs*/ 5), output_bytes_cap: 64 * 1024, + disable_output_cap: false, } } @@ -70,6 +73,18 @@ impl WorkspaceCommand { self.env.insert(key.into(), Some(value.into())); self } + + /// Sets the maximum wall-clock duration before app-server cancels the command. + pub(crate) fn timeout(mut self, timeout: Duration) -> Self { + self.timeout = timeout; + self + } + + /// Requests uncapped stdout/stderr capture from app-server. + pub(crate) fn disable_output_cap(mut self) -> Self { + self.disable_output_cap = true; + self + } } /// Captured result from a completed workspace command. @@ -176,8 +191,9 @@ impl WorkspaceCommandExecutor for AppServerWorkspaceCommandRunner { tty: false, stream_stdin: false, stream_stdout_stderr: false, - output_bytes_cap: Some(command.output_bytes_cap), - disable_output_cap: false, + output_bytes_cap: (!command.disable_output_cap) + .then_some(command.output_bytes_cap), + disable_output_cap: command.disable_output_cap, disable_timeout: false, timeout_ms: Some(timeout_ms), cwd: command.cwd,