refactor(error): introduce CommandError typed leaf for buffered git failures#2580
Merged
refactor(error): introduce CommandError typed leaf for buffered git failures#2580
Conversation
…ailures Building on #2567's `.context(...)` patch, the buffered command wrappers (`Repository::run_command`, `WorkingTree::run_command`) now return `Err(CommandError { program, args, stderr, stdout, exit_code })` instead of `bail!("{stderr}")`. The structured leaf separates the single-line summary (Display) from the captured payload (`combined_output()`), so multi-line stderr renders cleanly regardless of whether `.context(...)` was added at the propagation site, and callers that embed raw stderr in higher-level errors (`RebaseConflict`, `WorktreeRemovalFailed`, `PushFailed`, the list-task error path) read it directly instead of round-tripping through `e.to_string()`. The `debug_assert!` that triggered #2564 in debug builds is now a meaningful invariant: it flags only multi-line bails that aren't typed as `CommandError`, instead of acting as a proxy for "developer remembered `.context(...)`." Key changes: - `worktrunk::git::CommandError` (new) — typed leaf with `combined_output()` and a single-line `Display`. - `worktrunk::git::find_command_error` / `display_message` — chain walkers for renderers and embedders. - `print_command_error` refactored to `format_command_error -> String` (testable) plus a downcast branch that preserves intermediate `.context(...)` entries above the captured body. - `Repository::extract_failed_command` extended to recognize `CommandError` alongside `StreamCommandError`. - 5 user-facing serialization sites migrated to `display_message` so wrappers like `WorktreeRemovalFailed.error` and `PushFailed.error` keep showing git's real stderr. Notes: - `.context(...)` calls added in #2567 are no longer load-bearing for correctness — they remain as semantic UX. Pruning the mechanical ones is a follow-up. - Method named `combined_output` rather than `output` because the `no-direct-cmd-output` pre-commit hook regex flags `.output()` to keep callers off `std::process::Command::output`. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
worktrunk-bot
left a comment
There was a problem hiding this comment.
One missed migration site: handle_detached_removed_worktree_output in src/output/handlers.rs:1268 still does error: err.to_string() for GitError::WorktreeRemovalFailed, while the named-removal sibling at line 1344 was updated to display_message(&err). Both call remove_worktree_with_cleanup, which now produces a CommandError-bearing error via Repository::remove_worktree / delete_branch_if_safe — so for detached-HEAD removals the user now sees "git worktree remove --force /path failed (exit 128)" instead of git's actual stderr. The docstring on display_message even names WorktreeRemovalFailed as the canonical case for this helper.
Happy to push a one-line fix if you'd like.
Reviewer caught one missed migration (`handle_detached_removed_worktree_output` at handlers.rs:1268) — the prior `replace_all` matched only the sibling at L1344 because their indentation differed. Audit also found the branch-deletion error path at L452, which similarly stringifies a `run_command` error for direct gutter rendering. Both now go through `worktrunk::git::display_message` so the user sees git's actual stderr rather than the `CommandError` single-line summary. Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 4, 2026
Pre-commit fmt complained the inlined `eprintln!` with `display_message` exceeded the line limit. Reformat to multi-line. Co-Authored-By: Claude <noreply@anthropic.com>
Two trivial unit tests targeting codecov-flagged lines that didn't have direct coverage: - `command_error_command_string_handles_empty_args` exercises the args-empty branch of `CommandError::command_string()` (returns just the program name). - `renders_command_error_with_empty_body` exercises the gutter assembly's empty-body fallback: a CommandError wrapped in `.context()` with no captured stderr/stdout falls back to the single-line summary rather than rendering an empty gutter. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds on #2567 by replacing the leaf bail at the buffered command boundary with a typed error.
Repository::run_commandandWorkingTree::run_commandnow returnErr(CommandError { program, args, stderr, stdout, exit_code })instead ofbail!(\"{stderr}\"). The structured form lets the renderer separate a single-line summary (Display) from the multi-line captured payload (combined_output()), and lets callers that embed raw stderr in higher-level errors (RebaseConflict,WorktreeRemovalFailed,PushFailed, list-task errors) read it directly instead of round-tripping throughe.to_string().The
debug_assert!that triggered #2564 in debug builds is now a meaningful invariant: it flags only multi-line bails that aren't typed asCommandError, instead of acting as a proxy for "developer remembered.context(...)."What's in the diff
worktrunk::git::CommandError(new) — typed leaf withcombined_output()and a single-lineDisplay.worktrunk::git::find_command_error/display_message— chain walkers for renderers and embedders.print_command_errorrefactored toformat_command_error -> String(testable) with a downcast branch that preserves intermediate.context(...)entries above the captured body.Repository::extract_failed_commandextended to recognizeCommandErroralongsideStreamCommandError.display_messageso wrappers likeWorktreeRemovalFailed.errorandPushFailed.errorkeep showing git's real stderr (rather than the new single-line summary).Reviewer orientation
src/git/error.rs—CommandError,find_command_error,display_message.src/git/repository/mod.rs,src/git/repository/working_tree.rs— leaf cutover.src/main.rs— renderer; the new branch and the trim-then-anstreamprint_command_errorwere both flagged in iteration.src/commands/{step_commands,worktree/push,list/collect/{mod,tasks}}.rs,src/output/handlers.rs— call-site migrations.Notes
.context(...)calls added in fix(prune): wrap multi-line git errors with context to silence debug_assert #2567 are no longer load-bearing for correctness — they remain as semantic UX. Pruning the mechanical ones is a follow-up.combined_output()rather thanoutput()because theno-direct-cmd-outputpre-commit hook regex flags.output()to keep callers offstd::process::Command::output.Test plan
cargo run -- hook pre-merge --yes— 3480 tests pass, all lints, all docs