From 802a361a19c798c90903e047b671bbeb9984d498 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Wed, 15 Jul 2026 10:02:05 -0700 Subject: [PATCH 1/2] refactor(errors): finish routing command-failure bails through CommandError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remaining raw-stderr bail sites all become CommandError so failed-command display has one constructor (\r-normalized, typed command/output split): - step/shared.rs ls-files and step/commit.rs stage-to-temp-index (git) - claude/codex plugin CLI calls, collapsed into one run_plugin_cli helper in commands/config — their failure snapshots pick up the canonical header + gutter rendering - execute_llm_command: the stderr/stdout/exit-code fallback ladder is from_failed_output's job; the LlmCommandFailed wrappers and the picker summary pane read the detail via display_message(), which extracts the captured output for command failures and falls back to the same outermost message as before for spawn errors The gh/glab picker-list bails stay: their consumer is the skim footer, where the current one-line text is the intended display and picker text sanitization was deliberately dropped (#3177). Co-Authored-By: Claude Fable 5 --- src/commands/config/codex.rs | 29 +++------ src/commands/config/mod.rs | 17 ++++++ src/commands/config/plugins.rs | 34 ++--------- src/commands/step/commit.rs | 23 ++++++- src/commands/step/shared.rs | 36 ++++++++--- src/llm.rs | 60 ++++++++++++------- src/summary.rs | 6 +- tests/integration_tests/step_copy_ignored.rs | 2 +- ..._plugins_claude_install_command_fails.snap | 5 +- ...gins_claude_install_second_step_fails.snap | 5 +- ...lugins_claude_uninstall_command_fails.snap | 5 +- ...__plugins_codex_install_command_fails.snap | 5 +- 12 files changed, 138 insertions(+), 89 deletions(-) diff --git a/src/commands/config/codex.rs b/src/commands/config/codex.rs index 0ce971aa31..6b89156ba0 100644 --- a/src/commands/config/codex.rs +++ b/src/commands/config/codex.rs @@ -1,8 +1,7 @@ //! Codex plugin marketplace management. -use anyhow::{Context, Result, bail}; +use anyhow::{Result, bail}; use color_print::cformat; -use worktrunk::shell_exec::Cmd; use worktrunk::styling::{eprintln, hint_message, progress_message, success_message}; use super::show::is_codex_available; @@ -29,15 +28,10 @@ pub fn handle_codex_install(yes: bool) -> Result<()> { } eprintln!("{}", progress_message("Adding Codex plugin marketplace...")); - let output = Cmd::new("codex") - .args(["plugin", "marketplace", "add", MARKETPLACE_SOURCE]) - .run() - .context("Failed to run codex CLI")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("codex plugin marketplace add failed: {}", stderr.trim()); - } + super::run_plugin_cli( + "codex", + &["plugin", "marketplace", "add", MARKETPLACE_SOURCE], + )?; eprintln!("{}", success_message("Codex marketplace configured")); eprintln!( @@ -84,15 +78,10 @@ pub fn handle_codex_uninstall(yes: bool) -> Result<()> { "{}", progress_message("Removing Codex plugin marketplace...") ); - let output = Cmd::new("codex") - .args(["plugin", "marketplace", "remove", MARKETPLACE_NAME]) - .run() - .context("Failed to run codex CLI")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("codex plugin marketplace remove failed: {}", stderr.trim()); - } + super::run_plugin_cli( + "codex", + &["plugin", "marketplace", "remove", MARKETPLACE_NAME], + )?; eprintln!("{}", success_message("Codex marketplace removed")); eprintln!("{}", hint_message("Installed plugins are left unchanged")); diff --git a/src/commands/config/mod.rs b/src/commands/config/mod.rs index 18ad243499..cf24b95ec2 100644 --- a/src/commands/config/mod.rs +++ b/src/commands/config/mod.rs @@ -31,6 +31,23 @@ pub use state::{ }; pub use update::handle_config_update; +/// Run a plugin-CLI command (`claude` / `codex`), surfacing a non-zero exit +/// as a typed [`worktrunk::git::CommandError`]. +fn run_plugin_cli(program: &str, args: &[&str]) -> anyhow::Result<()> { + use anyhow::Context; + + let output = worktrunk::shell_exec::Cmd::new(program) + .args(args.iter().copied()) + .run() + .with_context(|| format!("Failed to run {program} CLI"))?; + if !output.status.success() { + return Err( + worktrunk::git::CommandError::from_failed_output(program, args, &output).into(), + ); + } + Ok(()) +} + #[cfg(test)] mod tests { use insta::assert_snapshot; diff --git a/src/commands/config/plugins.rs b/src/commands/config/plugins.rs index ee5183ccc1..1d4d97e026 100644 --- a/src/commands/config/plugins.rs +++ b/src/commands/config/plugins.rs @@ -4,7 +4,6 @@ use std::path::PathBuf; use anyhow::{Context, bail}; use color_print::cformat; -use worktrunk::shell_exec::Cmd; use worktrunk::styling::{eprintln, info_message, progress_message, success_message}; use super::show::{ @@ -35,26 +34,13 @@ pub fn handle_claude_install(yes: bool) -> anyhow::Result<()> { } eprintln!("{}", progress_message("Adding plugin from marketplace...")); - let output = Cmd::new("claude") - .args(["plugin", "marketplace", "add", "max-sixty/worktrunk"]) - .run() - .context("Failed to run claude CLI")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("claude plugin marketplace add failed: {}", stderr.trim()); - } + super::run_plugin_cli( + "claude", + &["plugin", "marketplace", "add", "max-sixty/worktrunk"], + )?; eprintln!("{}", progress_message("Installing plugin...")); - let output = Cmd::new("claude") - .args(["plugin", "install", "worktrunk@worktrunk"]) - .run() - .context("Failed to run claude CLI")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("claude plugin install failed: {}", stderr.trim()); - } + super::run_plugin_cli("claude", &["plugin", "install", "worktrunk@worktrunk"])?; eprintln!("{}", success_message("Plugin installed")); @@ -88,15 +74,7 @@ pub fn handle_claude_uninstall(yes: bool) -> anyhow::Result<()> { } eprintln!("{}", progress_message("Uninstalling plugin...")); - let output = Cmd::new("claude") - .args(["plugin", "uninstall", "worktrunk@worktrunk"]) - .run() - .context("Failed to run claude CLI")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("claude plugin uninstall failed: {}", stderr.trim()); - } + super::run_plugin_cli("claude", &["plugin", "uninstall", "worktrunk@worktrunk"])?; eprintln!("{}", success_message("Plugin uninstalled")); diff --git a/src/commands/step/commit.rs b/src/commands/step/commit.rs index 04438191ed..0eac03f805 100644 --- a/src/commands/step/commit.rs +++ b/src/commands/step/commit.rs @@ -142,8 +142,27 @@ fn stage_to_temp_index(repo: &Repository, add_args: &[&str]) -> anyhow::Result anyhow::Result> { + let args = [ + "ls-files", + "--ignored", + "--exclude-standard", + "-o", + "--directory", + ]; let output = Cmd::new("git") - .args([ - "ls-files", - "--ignored", - "--exclude-standard", - "-o", - "--directory", - ]) + .args(args) .current_dir(worktree_path) .context(context) .run() .context("Failed to run git ls-files")?; if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git ls-files failed: {}", stderr.trim()); + return Err(worktrunk::git::CommandError::from_failed_output("git", &args, &output).into()); } // Parse output: directories end with / @@ -228,3 +228,21 @@ fn list_ignored_entries( Ok(entries) } + +#[cfg(test)] +mod tests { + use worktrunk::git::CommandError; + use worktrunk::testing::TestRepo; + + /// A hard `git ls-files` failure (here: a fatal config error) must + /// surface as a typed `CommandError`. + #[test] + fn list_ignored_entries_failure_is_command_error() { + let test = TestRepo::with_initial_commit(); + std::fs::write(test.root_path().join(".git/config"), "[bad\n").unwrap(); + + let err = super::list_ignored_entries(test.root_path(), "test").unwrap_err(); + let cmd_err = CommandError::find_in(&err).expect("error should carry a CommandError"); + assert!(cmd_err.command_string().starts_with("git ls-files")); + } +} diff --git a/src/llm.rs b/src/llm.rs index 479ab2db84..935a687ee4 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -7,7 +7,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use worktrunk::config::CommitGenerationConfig; -use worktrunk::git::{CommandError, CommitMessageDetail, Repository}; +use worktrunk::git::{CommandError, CommitMessageDetail, ErrorExt, Repository}; use worktrunk::path::format_path_for_display; use worktrunk::shell_exec::{Cmd, ShellConfig}; use worktrunk::styling::{eprintln, warning_message}; @@ -512,32 +512,29 @@ pub(crate) fn execute_llm_command(command: &str, prompt: &str) -> anyhow::Result // entirely. See conversation around PR #2136 for sketch. let shell = ShellConfig::get()?; + let args: Vec<&str> = shell + .args + .iter() + .map(String::as_str) + .chain([command]) + .collect(); let output = Cmd::new(shell.executable.to_string_lossy()) - .args(&shell.args) - .arg(command) + .args(args.iter().copied()) .external("commit.generation") .stdin_bytes(prompt) .run() .context("Failed to spawn LLM command")?; if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - let stderr = stderr.trim(); - if stderr.is_empty() { - // Fall back to stdout or exit code when stderr is empty - let stdout = String::from_utf8_lossy(&output.stdout); - let stdout = stdout.trim(); - if stdout.is_empty() { - anyhow::bail!( - "LLM command failed with exit code {}", - output.status.code().unwrap_or(-1) - ); - } else { - anyhow::bail!("{}", stdout); - } - } else { - anyhow::bail!("{}", stderr); - } + // Shell basename only — the full install path is noise (and on + // Windows leaks the Git Bash location); same rule as + // `render_llm_invocation`. + let shell_name = shell + .executable + .file_name() + .unwrap_or(shell.executable.as_os_str()) + .to_string_lossy(); + return Err(CommandError::from_failed_output(shell_name, &args, &output).into()); } let message = String::from_utf8_lossy(&output.stdout).trim().to_owned(); @@ -779,7 +776,7 @@ pub(crate) fn generate_commit_message( return execute_llm_command(command, &prompt).map_err(|e| { worktrunk::git::GitError::LlmCommandFailed { command: command.clone(), - error: e.to_string(), + error: e.display_message(), reproduction_command: Some(format_reproduction_command( "wt step commit --show-prompt", command, @@ -928,7 +925,7 @@ pub(crate) fn generate_squash_message( return execute_llm_command(command, &prompt).map_err(|e| { worktrunk::git::GitError::LlmCommandFailed { command: command.clone(), - error: e.to_string(), + error: e.display_message(), reproduction_command: Some(format_reproduction_command( "wt step squash --show-prompt", command, @@ -1046,7 +1043,7 @@ pub(crate) fn test_commit_generation( execute_llm_command(command, &prompt).map_err(|e| { worktrunk::git::GitError::LlmCommandFailed { command: command.clone(), - error: e.to_string(), + error: e.display_message(), reproduction_command: None, // Already a test command } .into() @@ -1091,6 +1088,23 @@ mod tests { ); } + /// A failing LLM command must surface as a typed [`CommandError`] carrying + /// the exit code and captured output — `LlmCommandFailed` and the summary + /// pane read the detail via `display_message`. + #[test] + fn test_execute_llm_command_failure_is_command_error() { + let err = execute_llm_command("printf 'oops' >&2; exit 3", "prompt").unwrap_err(); + let cmd_err = CommandError::find_in(&err).expect("error should carry a CommandError"); + assert_eq!(cmd_err.exit_code, Some(3)); + assert_eq!(err.display_message(), "oops"); + // Basename only — no install-path leakage (see render_llm_invocation). + assert!( + !cmd_err.program.contains('/') && !cmd_err.program.contains('\\'), + "shell program has directory components: {}", + cmd_err.program + ); + } + /// Single-quotes in the user's command must be escaped so the displayed string is a /// faithful, copy-pasteable shell invocation. #[test] diff --git a/src/summary.rs b/src/summary.rs index 8700eafb6a..37d5e8eb6b 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -35,7 +35,7 @@ use minijinja::Environment; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use worktrunk::cache; -use worktrunk::git::Repository; +use worktrunk::git::{ErrorExt, Repository}; use worktrunk::path::sanitize_for_filename; use worktrunk::styling::INFO_SYMBOL; use worktrunk::sync::Semaphore; @@ -357,7 +357,9 @@ pub(crate) fn generate_summary( let reset = Reset; cformat!("{INFO_SYMBOL}{reset} {branch}{reset} has no changes to summarize\n") } - Err(e) => format!("Error: {e:#}"), + // `display_message` surfaces the LLM command's captured stderr for + // typed command failures rather than the single-line chain summary. + Err(e) => format!("Error: {}", e.display_message()), } } diff --git a/tests/integration_tests/step_copy_ignored.rs b/tests/integration_tests/step_copy_ignored.rs index 068cf4dc87..e433125fe5 100644 --- a/tests/integration_tests/step_copy_ignored.rs +++ b/tests/integration_tests/step_copy_ignored.rs @@ -624,7 +624,7 @@ fn test_copy_ignored_no_default_branch_worktree(mut repo: TestRepo) { /// Test copy-ignored in a bare repository setup /// /// This test reproduces GitHub issue #598: `wt step copy-ignored` fails in bare repo -/// with error "git ls-files failed: fatal: this operation must be run in a work tree" +/// with git's "fatal: this operation must be run in a work tree" #[test] fn test_copy_ignored_bare_repo() { use crate::common::{BareRepoTest, TestRepoBase, setup_temp_snapshot_settings, wt_command}; diff --git a/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_command_fails.snap b/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_command_fails.snap index b6a0c0845f..5daa76aa49 100644 --- a/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_command_fails.snap +++ b/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_command_fails.snap @@ -10,6 +10,7 @@ info: - "--yes" env: APPDATA: "[TEST_CONFIG_HOME]" + CLAUDE_CONFIG_DIR: "[TEST_CLAUDE_CONFIG]" CLICOLOR_FORCE: "1" COLUMNS: "500" GIT_AUTHOR_DATE: "2025-01-01T00:00:00Z" @@ -38,6 +39,7 @@ info: WORKTRUNK_TEST_GEMINI_INSTALLED: "0" WORKTRUNK_TEST_NUSHELL_ENV: "0" WORKTRUNK_TEST_OPENCODE_INSTALLED: "0" + WORKTRUNK_TEST_PARENT_SHELL: "" WORKTRUNK_TEST_POWERSHELL_ENV: "0" WORKTRUNK_TEST_POWERSHELL_INSTALLED: "0" WORKTRUNK_TEST_SKIP_URL_HEALTH_CHECK: "1" @@ -50,4 +52,5 @@ exit_code: 1 ----- stderr ----- ◎ Adding plugin from marketplace... -✗ claude plugin marketplace add failed: error: network timeout +✗ claude plugin marketplace add max-sixty/worktrunk failed (exit 1) +  error: network timeout diff --git a/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_second_step_fails.snap b/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_second_step_fails.snap index 974527ebbe..341cba7fc1 100644 --- a/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_second_step_fails.snap +++ b/tests/snapshots/integration__integration_tests__config_show__plugins_claude_install_second_step_fails.snap @@ -10,6 +10,7 @@ info: - "--yes" env: APPDATA: "[TEST_CONFIG_HOME]" + CLAUDE_CONFIG_DIR: "[TEST_CLAUDE_CONFIG]" CLICOLOR_FORCE: "1" COLUMNS: "500" GIT_AUTHOR_DATE: "2025-01-01T00:00:00Z" @@ -38,6 +39,7 @@ info: WORKTRUNK_TEST_GEMINI_INSTALLED: "0" WORKTRUNK_TEST_NUSHELL_ENV: "0" WORKTRUNK_TEST_OPENCODE_INSTALLED: "0" + WORKTRUNK_TEST_PARENT_SHELL: "" WORKTRUNK_TEST_POWERSHELL_ENV: "0" WORKTRUNK_TEST_POWERSHELL_INSTALLED: "0" WORKTRUNK_TEST_SKIP_URL_HEALTH_CHECK: "1" @@ -51,4 +53,5 @@ exit_code: 1 ----- stderr ----- ◎ Adding plugin from marketplace... ◎ Installing plugin... -✗ claude plugin install failed: error: install failed +✗ claude plugin install worktrunk@worktrunk failed (exit 1) +  error: install failed diff --git a/tests/snapshots/integration__integration_tests__config_show__plugins_claude_uninstall_command_fails.snap b/tests/snapshots/integration__integration_tests__config_show__plugins_claude_uninstall_command_fails.snap index 422057b81d..9a4519a1d8 100644 --- a/tests/snapshots/integration__integration_tests__config_show__plugins_claude_uninstall_command_fails.snap +++ b/tests/snapshots/integration__integration_tests__config_show__plugins_claude_uninstall_command_fails.snap @@ -10,6 +10,7 @@ info: - "--yes" env: APPDATA: "[TEST_CONFIG_HOME]" + CLAUDE_CONFIG_DIR: "[TEST_CLAUDE_CONFIG]" CLICOLOR_FORCE: "1" COLUMNS: "500" GIT_AUTHOR_DATE: "2025-01-01T00:00:00Z" @@ -38,6 +39,7 @@ info: WORKTRUNK_TEST_GEMINI_INSTALLED: "0" WORKTRUNK_TEST_NUSHELL_ENV: "0" WORKTRUNK_TEST_OPENCODE_INSTALLED: "0" + WORKTRUNK_TEST_PARENT_SHELL: "" WORKTRUNK_TEST_POWERSHELL_ENV: "0" WORKTRUNK_TEST_POWERSHELL_INSTALLED: "0" WORKTRUNK_TEST_SKIP_URL_HEALTH_CHECK: "1" @@ -50,4 +52,5 @@ exit_code: 1 ----- stderr ----- ◎ Uninstalling plugin... -✗ claude plugin uninstall failed: error: uninstall failed +✗ claude plugin uninstall worktrunk@worktrunk failed (exit 1) +  error: uninstall failed diff --git a/tests/snapshots/integration__integration_tests__config_show__plugins_codex_install_command_fails.snap b/tests/snapshots/integration__integration_tests__config_show__plugins_codex_install_command_fails.snap index 9d73b50a51..2dd80cbe2e 100644 --- a/tests/snapshots/integration__integration_tests__config_show__plugins_codex_install_command_fails.snap +++ b/tests/snapshots/integration__integration_tests__config_show__plugins_codex_install_command_fails.snap @@ -10,6 +10,7 @@ info: - "--yes" env: APPDATA: "[TEST_CONFIG_HOME]" + CLAUDE_CONFIG_DIR: "[TEST_CLAUDE_CONFIG]" CLICOLOR_FORCE: "1" COLUMNS: "500" GIT_AUTHOR_DATE: "2025-01-01T00:00:00Z" @@ -38,6 +39,7 @@ info: WORKTRUNK_TEST_GEMINI_INSTALLED: "0" WORKTRUNK_TEST_NUSHELL_ENV: "0" WORKTRUNK_TEST_OPENCODE_INSTALLED: "0" + WORKTRUNK_TEST_PARENT_SHELL: "" WORKTRUNK_TEST_POWERSHELL_ENV: "0" WORKTRUNK_TEST_POWERSHELL_INSTALLED: "0" WORKTRUNK_TEST_SKIP_URL_HEALTH_CHECK: "1" @@ -50,4 +52,5 @@ exit_code: 1 ----- stderr ----- ◎ Adding Codex plugin marketplace... -✗ codex plugin marketplace add failed: error: marketplace add failed +✗ codex plugin marketplace add max-sixty/worktrunk failed (exit 1) +  error: marketplace add failed From e6d2cfbc51688b519f9f3bf40bb2b6014ea2c975 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Wed, 15 Jul 2026 14:18:28 -0700 Subject: [PATCH 2/2] fix(summary): keep the anyhow chain for non-command preview errors display_message() collapses any non-CommandError to its outermost context line, so the preview pane lost the cause chain for spawn failures and template bugs, not just the narrow case the PR body flagged. Guard on the error type instead: command failures surface their captured output, everything else keeps the {:#} chain the pane always showed. Suggested by the automated reviewer on #3483. Co-Authored-By: Claude Fable 5 --- src/summary.rs | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/summary.rs b/src/summary.rs index 37d5e8eb6b..b856171b57 100644 --- a/src/summary.rs +++ b/src/summary.rs @@ -35,7 +35,7 @@ use minijinja::Environment; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use worktrunk::cache; -use worktrunk::git::{ErrorExt, Repository}; +use worktrunk::git::{CommandError, ErrorExt, Repository}; use worktrunk::path::sanitize_for_filename; use worktrunk::styling::INFO_SYMBOL; use worktrunk::sync::Semaphore; @@ -357,9 +357,22 @@ pub(crate) fn generate_summary( let reset = Reset; cformat!("{INFO_SYMBOL}{reset} {branch}{reset} has no changes to summarize\n") } - // `display_message` surfaces the LLM command's captured stderr for - // typed command failures rather than the single-line chain summary. - Err(e) => format!("Error: {}", e.display_message()), + Err(e) => format_summary_error(&e), + } +} + +/// Format a summary-generation error for the preview pane. +/// +/// A typed command failure carries the LLM command's captured output — +/// `display_message` surfaces that rather than the single-line chain +/// summary. For anything else (shell spawn failure, template bug) keep +/// the full anyhow chain, which `display_message` would collapse to its +/// outermost line. +fn format_summary_error(e: &anyhow::Error) -> String { + if CommandError::find_in(e).is_some() { + format!("Error: {}", e.display_message()) + } else { + format!("Error: {e:#}") } } @@ -367,6 +380,22 @@ pub(crate) fn generate_summary( mod tests { use super::*; + /// A non-command error (spawn failure, template bug) keeps its full + /// anyhow chain in the preview pane; command failures are covered by + /// `test_generate_summary_llm_error`. + #[test] + fn format_summary_error_keeps_chain_for_non_command_errors() { + use anyhow::Context; + let err = std::io::Error::new(std::io::ErrorKind::NotFound, "No such file or directory"); + let err = anyhow::Result::<()>::Err(err.into()) + .context("Failed to spawn LLM command") + .unwrap_err(); + assert_eq!( + format_summary_error(&err), + "Error: Failed to spawn LLM command: No such file or directory" + ); + } + #[test] fn test_render_prompt_includes_diff_and_stat() { let result = render_prompt("diff content here", "stat content here").unwrap();