refactor(errors): finish routing command-failure bails through CommandError - #3483
Merged
Conversation
…dError 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 <noreply@anthropic.com>
worktrunk-bot
approved these changes
Jul 15, 2026
worktrunk-bot
left a comment
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
Clean refactor — the routing through CommandError::from_failed_output is correct, imports balance, and the three new error-path tests are deterministic and cross-platform (invalid pathspec / corrupt .git/config / sh -c all fail identically under sh and Git Bash). One non-blocking observation on the summary preview inline.
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 <noreply@anthropic.com>
worktrunk-bot
approved these changes
Jul 15, 2026
max-sixty
added a commit
that referenced
this pull request
Jul 15, 2026
) Step-12 drift fold for the v0.68.0 release: #3483 (the follow-up completing #3474) merged during the release PR CI window, so it ships in 0.68.0 but the changelog entry only covered the eight #3474 sites. The entry now covers the full cutover and cites #3483. > _This was written by Claude Code on behalf of max_
1 task
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
2 participants
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.
Follow-up to #3474: converts the remaining raw-output
bail!("{stderr}")sites to typedCommandError, so every failed-command display goes through the one normalizing constructor (\rstripping, canonical✗ <command> failed (exit N)header + output gutter).Sites converted:
step/shared.rs(git ls-filesin ignored-entry listing) andstep/commit.rs(git addinto the temp index) — the two commands-layer git sites.commands/config/{plugins,codex}.rs, collapsed into one privaterun_plugin_clihelper incommands/config/mod.rs. Their failure snapshots now show the canonical header + stderr gutter instead of✗ claude plugin marketplace add failed: <stderr>.execute_llm_commandinllm.rs— its hand-rolled stderr/stdout/exit-code fallback ladder wasfrom_failed_output's job; deleted in favor of the constructor. TheLlmCommandFailedwrappers read the detail viaErrorExt::display_message(), which extracts the captured output for command failures and matches the old text in the common cases; the picker summary pane guards on the error type (format_summary_error) — command failures surface their captured output, everything else keeps the full anyhow chain the pane always showed (folded in from review). The shell program is recorded by basename only (same rule asrender_llm_invocation), so a no-output failure doesn't leak the Git Bash install path on Windows.Deliberately left: the two
gh pr list/glab mr listbails inpicker/prs.rs. Their consumer is the skim-footer warning sink where the current one-line text is the intended display, and picker text sanitization was explicitly dropped (#3177).Behavior note (intended, but visible): an LLM command that emits on both streams before failing now shows both in the error block (
combined_output()joins stderr + stdout); the old ladder showed stderr only.Testing: three new error-path unit tests (
list_ignored_entries/stage_to_temp_indexvia corrupt-config and bad-pathspec,execute_llm_commandexit-code + basename assertion); the five plugin sites are covered by the existing failing-mock integration tests whose snapshots this updates;test_generate_summary_llm_errorcovers the summary-pane command-failure arm andformat_summary_error_keeps_chain_for_non_command_errorsthe chain-preserving arm.