fix(rebase): surface Ctrl-C during rebase as interrupt, not conflict - #3539
Conversation
worktrunk-bot
added
the
nightly-cleanup
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
This fix classifies the wrong error type, so it won't fire on the path it targets.
handle_rebase runs the rebase with repo.run_command(&["rebase", …]), which is Cmd::run() capture mode. On any non-zero/signal exit, run_command returns CommandError::from_failed_output(...) — see run_command in src/git/repository/mod.rs (return Err(super::error::CommandError::from_failed_output("git", args, &output).into())). It never produces a WorktrunkError::ChildProcessExited. Only Cmd::stream() constructs ChildProcessExited { signal } — and only stream() installs the signal forwarder and cmd.process_group(0); capture mode does neither.
interrupt_exit_code() only matches WorktrunkError::ChildProcessExited (self.downcast_ref::<WorktrunkError>() in src/git/error.rs). So in the real interrupt path e is a CommandError, interrupt_exit_code() returns None, and control still falls through to the is_rebasing → RebaseConflict branch — the exact behavior the PR says it fixes.
Worse, the signal number can't even be recovered downstream as written: CommandError::from_failed_output stores exit_code: output.status.code(), which is None for a signal-killed child, and drops output.status.signal() entirely. So teaching interrupt_exit_code() to also inspect CommandError wouldn't help — the fix has to live at the capture layer (preserve status.signal() when building the CommandError, or route the rebase through a path that yields ChildProcessExited).
The unit test passes only because child_exit() fabricates a WorktrunkError::ChildProcessExited { signal: Some(2) } directly — an error shape run_command never returns for this path. The test green-lights a change that doesn't alter runtime behavior. A regression test that actually pins the bug would need to drive handle_rebase (or at least feed classify_rebase_failure the CommandError that run_command really produces on a signal exit) and observe that it's not surfaced as a RebaseConflict.
Flagging for @max-sixty since this touches the rebase/merge failure path. I haven't reproduced the end-to-end interrupt in CI (no TTY), but the error-type mismatch is verifiable from the code above — worth confirming the intended fix layer before this lands, since CI is green on an inert change.
…tion actually fires The prior fix checked interrupt_exit_code() in classify_rebase_failure, but run_command (capture mode) returns a CommandError, not a ChildProcessExited, and from_failed_output dropped status.signal() entirely — so interrupt_exit_code() always returned None on the real path and control still fell through to RebaseConflict. Preserve status.signal() on CommandError (Unix) in from_failed_output, and teach ErrorExt::interrupt_exit_code() to walk the anyhow chain for a CommandError carrying a signal (128 + sig). This makes signal classification correct for every capture-mode command, not just rebase, per the structured-signal policy in CLAUDE.md. Replace the regression test that fabricated a ChildProcessExited (a shape run_command never returns for this path) with one that feeds the real CommandError-with-signal, plus a capture-layer test asserting from_failed_output preserves the signal and interrupt_exit_code recovers it through a .context(...) wrapper.
|
Confirmed — the review was right: the original change was inert on the real path. Fixed at the capture layer, per the review's suggestion:
The regression test that fabricated a One honest caveat I added to the PR description: the common terminal Ctrl-C case is subtler than the original framing. Capture mode doesn't isolate the child in its own process group and |
worktrunk-bot
left a comment
worktrunk-bot
left a comment
There was a problem hiding this comment.
The capture-layer fix looks right and the tests now pin the real error shape. One consequence worth a conscious decision, since it's broader than rebase:
interrupt_exit_code() now maps any capture-mode signal death to Some(128 + sig), not just SIGINT/SIGTERM. Because classify_rebase_failure converts that into a silent AlreadyDisplayed, a git process killed by a non-interrupt signal mid-rebase — SIGSEGV (139), SIGKILL (137), SIGPIPE (141) — now exits silently with 128 + sig instead of surfacing git's stderr as a RebaseConflict/Other error. And since the recovery lives in the shared ErrorExt method, this reclassification applies to every capture-mode (Cmd::run) command, not only rebase.
This is symmetric with the pre-existing stream-mode ChildProcessExited { signal } arm (which already treats every signal as an interrupt), so it's arguably the intended generalization — but the function name reads as "interrupt", and swallowing a git crash's diagnostics is a real (if rare) behavior change. Flagging for @max-sixty to confirm that "all signals ⇒ silent 128+sig" is the desired semantics for capture mode, or whether it should narrow to SIGINT/SIGTERM.
|
CI status for the record (opened by the nightly sweep):
|
|
CI note: the red |
…IGTERM
interrupt_exit_code() classified any capture-mode signal death as an
interrupt, so a git killed by SIGSEGV or an OOM SIGKILL mid-rebase
exited silently with 128+sig, discarding the captured stderr and leaving
the REBASING worktree unexplained. Capture mode has no signal forwarding
or escalation, so a non-SIGINT/SIGTERM death is never wt's own doing;
those now fall through to the visible error path (RebaseConflict
mid-rebase, whose rendering carries the --abort hint, else Other).
Stream mode keeps counting any signal: output already streamed to the
terminal, and user-initiated kills are normalized upstream to the
originating SIGINT/SIGTERM before classification reads the raw signal.
Also name the signal in CommandError's summary ("git fetch failed
(signal 11)") so the surfaced error says what happened, and record both
structured channels in CLAUDE.md's signal-handling section.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Resolved the open question by narrowing, but only the capture-mode branch: The asymmetry is principled, and it turns on two facts the review flagged around but didn't pin down:
Also in the commit: a signal-killed
|
A passing positive matches! never runs its generated false arm; when the macro opens on its own line, llvm-cov attributes that arm's region to the opener line, which then reads as missed and failed codecov/patch (97.70% vs 97.79% target, three such lines). Binding the downcast and importing the variant keeps each call within rustfmt's fn_call_width so the opener shares a line with executed regions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Resolved — the narrowing is exactly the semantics I was flagging for. Verified at the PR head ( |
A SIGTERM against wt previously exited silently: wt traps SIGINT/SIGTERM
to forward them to children, so the shell never sees wt die and never
prints its own "Terminated" line — and the interrupt classification
funneled every signal-derived child exit into
AlreadyDisplayed { exit_code: 128 + sig }, erasing the signal identity
exactly where the exit path would need it.
Interrupt exits are now typed: WorktrunkError::Interrupted { signal, hint }
replaces AlreadyDisplayed at every classification site
(handle_command_error, for_each, classify_rebase_failure, run_custom's
signal branch) and renders once at exit, per shell convention — silent
for SIGINT (the terminal already echoed ^C), "Terminated" for SIGTERM,
"Killed by signal N" otherwise. Exit codes stay 128 + signal.
The hint field carries a recovery line for state the interrupt left
behind: an interrupted rebase (capture mode, so none of git's output was
shown) now prints "Rebase onto <target> left in progress; to abort, run
git rebase --abort" under the message instead of silently stranding the
worktree in REBASING state.
ErrorExt::interrupt_exit_code() is renamed interrupt_signal() and
returns the signal; the 128 + signal convention lives in one place
(WorktrunkError::exit_code). interrupt_signal() also recognizes
Interrupted itself, so an outer loop re-checking a bubbled error can't
demote it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves src/commands/step/rebase.rs against #3539, which extracted the failed-rebase classification into `classify_rebase_failure`. Its `is_rebasing` check reads the structured `InProgressOperation` this branch introduces instead of matching "REBASING" on a display string. > _This was written by Claude Code on behalf of Maximilian_ Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
> _This was written by Claude Code on behalf of max_
…3558) ## The bug With a rebase in progress (a conflicted stop, or a killed `git`), `wt step rebase main` reported success over a conflicted tree: ```console $ git rebase main # stops on a conflict; HEAD detached mid-replay $ wt step rebase main ○ Already up to date with main $ echo $? 0 ``` `handle_rebase` asked `is_rebased_onto` first. Mid-rebase, HEAD sits on a linear extension of the target, so that returns true and the function returns `UpToDate` before anything consults `worktree_state`. `wt merge` from the same state failed differently but no better: ```console ✗ Cannot merge: not on a branch (detached HEAD) ↳ To switch to a branch, run git switch <branch> ``` The hint names the one command that throws the rebase away instead of finishing it. ## The fix Both commit-replaying commands now check for an open operation before doing anything else. `wt merge` runs the check ahead of its branch check, so the open operation is what gets named rather than the detached HEAD it causes. ```console $ wt step rebase main ✗ Cannot rebase: a git operation is already in progress ``` The gate covers every state detection reports, not just rebase. Measured on the others beforehand: a rebase started mid-bisect ran and stacked a second operation on the first, and one started mid-merge or mid-cherry-pick died inside git's autostash (`Cannot save the current index state`, `f: needs merge`). None told the user what to do next. ## Why it prescribes no remedy An earlier draft of this branch carried a per-operation table of `--continue`/`--abort` hints. It was wrong on first contact with the long tail: `git am` borrows the `rebase-apply` directory, so an open am session was reported as a rebase and pointed at `git rebase --continue`, which git refuses outright: ```console $ git rebase --continue fatal: It looks like 'git am' is in progress. Cannot rebase. ``` Adding an `Am` arm would have treated the enumeration as the fixable part. `git status` already answers this better than the table did, in git's own words and translations, for states worktrunk has no variant for: ```console $ git status interactive rebase in progress; onto b162044 You are currently rebasing branch 'feat' on 'b162044'. (fix conflicts and then run "git rebase --continue") (use "git rebase --skip" to skip this patch) (use "git rebase --abort" to check out the original branch) ``` It names the operation, the branch, the progress, and every way out including `--skip`, which the table never offered. So the table is gone rather than extended, and the refusal is a single line: it says what is blocked and why, and leaves the rest to the command every git user reaches for next. Detection still returns a structured `InProgressOperation` rather than a display string, but now purely as detection: the only caller that reads a variant is the one classifying a rebase worktrunk itself just started. The neighbouring `RebaseConflict` error took the same treatment. It forwards git's stderr in the gutter when it has it, and otherwise substituted two hint lines of its own: ```console ↳ To continue after resolving conflicts, run git rebase --continue ↳ To abort, run git rebase --abort ``` That is the operation table again in miniature — one operation's remedies, minus `--skip` — and the one path that reaches it is the safety check for a rebase git *reported as finished*, where worktrunk knows least about what is actually open. It now prints its title, plus git's own words when git said any. ## One detector Two places probed the same files for the same answer: `Repository::worktree_state` for the gate, and `WorkingTree::is_rebasing`/`is_merging` for the `wt list` gutter symbols. They are now one method, `WorkingTree::operation_in_progress`, which `Repository::operation_in_progress` delegates to — the rename also retires a name that collided with `wt list`'s unrelated `WorktreeState`. Detection now runs in the order `git status` consults the state files, which is a change from the old `wt list` path: it checked rebase before merge. Nothing can reach the difference — no rebase backend writes `MERGE_HEAD`, checked against all three — but matching git's order is one less thing to reason about. `wt list` maps the operations without a gutter symbol (cherry-pick, revert, bisect) to `None`, so its output is unchanged. ## The queued sequencer, now closed Having one detector is what made the last gap worth closing. `git cherry-pick A B` that stops on A writes `CHERRY_PICK_HEAD`; resolving that stop with `git commit` instead of `git cherry-pick --continue` removes the file and leaves B queued: ```console $ git status On branch main Cherry-pick currently in progress. (run "git cherry-pick --continue" to continue) $ ls .git/CHERRY_PICK_HEAD ls: .git/CHERRY_PICK_HEAD: No such file or directory $ cat .git/sequencer/todo pick 1f07533 side1 pick 6b32540 side2 ``` Git reads that same file to keep calling the sequence in progress, so detection reads it too: the first instruction word, `pick` or `revert`, exactly as git's own `sequencer_get_last_command` does. The reason this waited for its own change is the risk of refusing a `wt merge` the user can't un-refuse, so the lifecycle is measured rather than assumed. The directory is gone once the sequence finishes, and gone after `--quit` or `--abort`; a single-commit cherry-pick never creates one; a rebase keeps its todo elsewhere, under `rebase-merge/`. An empty or unrecognized todo reports nothing, so the failure direction is the old behavior rather than a refusal with no way out. ## Verification The repro above, plus the merge, am, cherry-pick, revert, and bisect states, checked by hand against real repos: all five refuse, including am, which is now correct precisely because nothing here claims to know what am needs. Three integration tests drive the real binary from a genuinely stopped operation (`test_step_rebase_refuses_mid_rebase`, `test_merge_refuses_mid_rebase`, `test_merge_refuses_mid_merge`). `test_merge_refuses_mid_merge` is what holds the broadened scope: a conflicted merge keeps HEAD on the branch, so it is the case the detached-HEAD check waves through. Narrowing the gate back to rebase-only turns it red, which I verified by making that edit and re-running. The sequencer case is driven end to end from a real stopped pick — `test_operation_in_progress_reads_the_queued_sequencer` commits the resolution by hand, asserts `CHERRY_PICK_HEAD` is gone, and still expects a cherry-pick, so it can only pass through the sequencer read. It then runs `git cherry-pick --quit` and expects the state to clear, which is the false-refusal half. Running the same sequence against the built binary refuses where it previously reported `○ Already up to date with main`. Confirmed unchanged: already-up-to-date, fast-forward, true-rebase, and rebase-conflict output; a plain detached HEAD still gets the `git switch` hint, which is the right remedy when no operation is open; a sibling worktree mid-rebase does not block a clean one; `wt list`'s gutter symbols. Full gate green (4494/4494 tests, lints, fmt), plus `cargo clippy --all-targets --features shell-integration-tests`, which the gate doesn't compile. ## Merged with main `main` gained #3539, which extracted the failed-rebase classification into `classify_rebase_failure`, and #3549, which adds `span_upstream` — both inside `handle_rebase`. The conflict was semantic as well as textual: #3539's `is_rebasing` check still read the old string API (`worktree_state()?.is_some_and(|s| s.starts_with("REBASING"))`), which this branch removes, so resolving the markers alone left a type error. The resolution keeps #3539's classification and reads the structured enum instead: ```rust let state = repo.operation_in_progress()?; let is_rebasing = matches!(state, Some(InProgressOperation::Rebase)); return Err(classify_rebase_failure(e, is_rebasing, &integration_target)); ``` All five of #3539's classification tests pass alongside the gate. > _This was written by Claude Code on behalf of Maximilian_ --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Surfaced by the nightly code-quality survey; the fix layer was corrected after review, and the review discussion then drove the
Interruptedrendering work (see below).Problem
When a
git rebasechild is killed by a signal (SIGINT/SIGTERM) but leaves the worktree inREBASINGstate,handle_rebaseclassified the failure byworktree_state()alone (nowREBASING) and returnedGitError::RebaseConflict, printing conflict-resolution guidance and a non-130 exit code instead of a clean interrupt. This violates the signal-handling policy inCLAUDE.md: signal-derived child exits must be detected via the structured signal channel before any other classification.The original attempt added an interrupt check to
classify_rebase_failure, but it was inert on the real path:handle_rebaserunsrepo.run_command(["rebase", …]), which isCmd::run()capture mode. On failure that returns aCommandError(viafrom_failed_output), never aWorktrunkError::ChildProcessExited, and the interrupt detector only matchedChildProcessExited. Worse,from_failed_outputstored onlyoutput.status.code()(which isNonefor a signal kill) and droppedoutput.status.signal()entirely, so the signal number wasn't recoverable downstream at all.A second problem fell out of the review discussion: every interrupt classification funneled into
AlreadyDisplayed { exit_code: 128 + sig }, erasing the signal identity — so a SIGTERM-killedwtexited silently. Because wt traps SIGINT/SIGTERM to forward them to children, the shell never sees wt die and never prints its ownTerminatedline, so the standard shell convention was suppressed with nothing in its place. And an interrupt that left the worktree mid-rebase said nothing about theREBASINGstate it left behind (git ran in capture mode, so none of its output was ever shown).Fix
The capture layer preserves the signal:
CommandErrorgains asignal: Option<i32>field, populated infrom_failed_outputfromoutput.status.signal()on Unix (Noneelsewhere). A signal-killed child has no exit code, so the display summary names the signal instead:git rebase failed (signal 11).ErrorExt::interrupt_signal()(néinterrupt_exit_code()) walks the anyhow chain for aCommandErrorcarrying SIGINT or SIGTERM, so it works throughrun_command's.context("Failed to execute: git …")wrapper.Only SIGINT/SIGTERM classify as interrupts in capture mode. Capture children get no signal forwarding or escalation, so a death by any other signal (SIGSEGV, an OOM SIGKILL) is never wt's own doing, and a silent exit would discard the captured stderr the user never saw. Those stay on the visible error path: mid-rebase, the
REBASINGstate classifies them asRebaseConflict, whose rendering carries the--aborthint; otherwiseGitError::Other. Stream mode'sChildProcessExitedbranch counts any signal: its output already streamed to the terminal, and user-initiated kills are normalized upstream to the originating SIGINT/SIGTERM (theseen_signalbranch inshell_exec, the concurrent runner's originating-signal override).Interrupt exits are now typed and announced. A new
WorktrunkError::Interrupted { signal, hint }replacesAlreadyDisplayed { exit_code: 128 + sig }at every interrupt-classification site (handle_command_error,for_each,classify_rebase_failure,run_custom's signal branch). It still exits128 + signal, but renders once, at exit, per shell convention:^C, and shells print nothing for an interrupted job.✗ Terminated— the line the shell would have printed had wt not trapped the signal.✗ Killed by signal N.hintcarries an optional recovery line rendered under the message.classify_rebase_failureuses it for the state an interrupt leaves behind:(For SIGINT the hint stands alone under the user's own
^C.)Reachability caveat
The common terminal Ctrl-C case is subtler than the original description implied: on paths where wt has not installed its signal handler, a terminal Ctrl-C is delivered to the whole foreground group and terminates
wtitself at 130 before classification runs. The capture-mode fix is reachable, and correct, when the signal reaches only thegitchild whilewtsurvives (e.g. withinwt mergeafter the handler is installed, a targeted kill, or a supervisor signalling the child), and it also closes the latent, broader gap where capture-mode signal deaths were classified without consulting the structured signal channel at all.Tests
capture_mode_interrupt_is_not_classified_as_conflictfeedsclassify_rebase_failurethe realCommandError-with-signal shape (wrapped in the.context(...)layerrun_commandadds) and assertsInterrupted { signal: 2, hint: Some(…) }with thegit rebase --aborthint, notRebaseConflict.capture_mode_crash_signal_stays_a_visible_errorasserts a SIGSEGV death mid-rebase classifies asRebaseConflictand a SIGKILL death outsideREBASINGasOther, never a silent interrupt.interrupt_signal_capture_mode_narrows_to_sigint_sigtermpins the capture-mode signal set: 2 and 15 classify, 9 and 11 don't.from_failed_output_preserves_signal_and_interrupt_signal_recovers_itpins the capture layer directly: a signal-killedOutputyieldssignal: Some(2)/exit_code: None, recovered through a.context(...)wrapper.interrupted_renders_by_signal_and_exits_128_plus_signalpins the exit-time rendering: silent SIGINT,✗ Terminatedfor SIGTERM,✗ Killed by signal 9otherwise, hint placement, and the128 + signalexit codes.test_interrupt_signalcovers the stream-mode channel plus idempotence (anInterruptederror re-checked by an outer loop stays an interrupt).test_pre_merge_pipeline_aborts_on_signal_exit(hook pipeline),test_for_each_aborts_on_signal_exit(worktree loop),run_custom_propagates_signal_exit_code(custom subcommands) — all now flowing throughInterrupted.