mirror of
https://github.com/openai/codex.git
synced 2026-05-18 10:12:59 +00:00
feat(tui): route /diff through workspace commands (#21001)
Stacked on #20892. ## Why #20892 adds the TUI workspace command abstraction so branch status metadata can run through app-server instead of assuming the CLI process has the active workspace locally. `/diff` still used direct local process execution, which means remote app-server sessions could compute the diff against the wrong machine or fail to see the active workspace at all. This PR moves `/diff` onto that same app-server-backed command path so Git runs wherever the active workspace lives. ## What Changed - Route `/diff` through the TUI `WorkspaceCommandExecutor` using the active chat cwd. - Replace direct `tokio::process::Command` usage in `get_git_diff` with argv-based workspace command requests. - Preserve the existing `/diff` behavior: tracked diff output, untracked file diffs, treating Git diff exit code `1` as success, and showing the existing non-git-repository message. - Extend `WorkspaceCommand` with caller-set timeouts and an explicit uncapped-output opt-out. Metadata probes remain capped by default; `/diff` opts out because its full output is the user-visible payload. ## How to Test Manual reviewer path: 1. Start the Codex TUI from a Git worktree with one tracked file change and one untracked file. 2. Run `/diff`. 3. Confirm the rendered diff includes both the tracked diff and the untracked file diff. 4. Start the TUI outside a Git worktree, or switch to a non-git cwd, then run `/diff`. 5. Confirm it shows the existing `/diff` not-inside-a-git-repository message. Targeted tests run: - `cargo test -p codex-tui get_git_diff -- --nocapture` - `cargo test -p codex-tui branch_summary -- --nocapture` - `cargo test -p codex-tui`
This commit is contained in:
@@ -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));
|
||||
});
|
||||
|
||||
@@ -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<io::Result<String>> = 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<String> {
|
||||
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<String, String> {
|
||||
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<String> {
|
||||
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<String, String> {
|
||||
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<bool> {
|
||||
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<bool, String> {
|
||||
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<WorkspaceCommandOutput, String> {
|
||||
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<Vec<String>> = commands
|
||||
.iter()
|
||||
.map(|command| command.argv.clone())
|
||||
.collect();
|
||||
let expected: Vec<Vec<String>> = 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<String>,
|
||||
output: WorkspaceCommandOutput,
|
||||
}
|
||||
|
||||
struct FakeRunner {
|
||||
responses: Mutex<VecDeque<FakeResponse>>,
|
||||
commands: Mutex<Vec<WorkspaceCommand>>,
|
||||
}
|
||||
|
||||
impl FakeRunner {
|
||||
fn new(responses: Vec<FakeResponse>) -> Self {
|
||||
Self {
|
||||
responses: Mutex::new(responses.into()),
|
||||
commands: Mutex::new(Vec::new()),
|
||||
}
|
||||
}
|
||||
|
||||
fn commands(&self) -> Vec<WorkspaceCommand> {
|
||||
self.commands.lock().expect("commands lock").clone()
|
||||
}
|
||||
}
|
||||
|
||||
impl WorkspaceCommandExecutor for FakeRunner {
|
||||
fn run(
|
||||
&self,
|
||||
command: WorkspaceCommand,
|
||||
) -> Pin<
|
||||
Box<
|
||||
dyn Future<Output = Result<WorkspaceCommandOutput, WorkspaceCommandError>>
|
||||
+ 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)
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<Item = impl Into<String>>) -> 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,
|
||||
|
||||
Reference in New Issue
Block a user