Skip to content

fix(step): gate every staging path on an unmerged index - #3588

Merged
max-sixty merged 3 commits into
mainfrom
fix-relocate-unmerged-paths-gate
Jul 25, 2026
Merged

fix(step): gate every staging path on an unmerged index#3588
max-sixty merged 3 commits into
mainfrom
fix-relocate-unmerged-paths-gate

Conversation

@max-sixty

@max-sixty max-sixty commented Jul 25, 2026

Copy link
Copy Markdown
Owner

wt step relocate --commit committed conflict markers, the bug #3579 fixed everywhere else. It stages with git add -A and calls CommitGenerator::commit_staged_changes directly, so it reached neither of that PR's two gates:

$ git merge side          # CONFLICT (content): Merge conflict in f
$ wt step relocate --commit
◎ Committing changes in feature...
✓ Committed changes @ eb4cf19
$ git show HEAD:f
<<<<<<< HEAD
feature
=======
side
>>>>>>> side

MERGE_HEAD is gone and the working tree is clean afterwards, so the broken merge reads as finished.

The gate belongs at the staging step, not the commit

The obvious funnel is commit_staged_changes, the one call all three staging paths share. It cannot work, and the reason rules the shape out rather than merely disfavoring it: every caller stages before that call, and git add -A collapses an unmerged path's three index stages into one entry. After it, git diff --diff-filter=U returns nothing and git itself stops refusing.

$ git ls-files -u
100644 df967b9… 1	f
100644 ba2906d… 2	f
100644 2299c37… 3	f
$ git add -A
$ git diff --name-only --diff-filter=U    # empty
$ git commit -m x                         # succeeds

A check there passes vacuously in exactly the case it exists for.

So the funnel sits one step earlier. WorkingTree::stage(StageMode, action) refuses, then stages, with nothing in between. It is now the only path to git add against a worktree's real index (every other git add writes a throwaway index via temp_index), so a fifth entrypoint can neither forget the gate nor order it after the staging that destroys the evidence. It also replaces three hand-rolled copies of the StageMode to git add match, so the callers come out smaller than they went in.

The second gate is not belt-and-suspenders

The callers keep their early refusals, which guard a different window: those run before hooks, prompts, and the LLM call, so a doomed commit runs no project commands. That placement leaves a hole the early refusal structurally cannot see, because a pre-commit hook is arbitrary project code, free to conflict the index after the gate has passed.

$ cat .config/wt.toml
pre-commit = "git merge side"
$ wt step commit --yes
◎ Running pre-commit project hook
✓ Committed changes @ 9e89c76        # markers and all

So wt step commit carried the same bug, reachable through any project config. That path isn't experimental, which may make it the more consequential half of this PR. test_step_commit_refuses_hook_created_conflict pins it.

relocate skips rather than aborting

An unresolved conflict is a per-worktree blocker the user has to fix by hand, like a locked worktree. relocate is a bulk command whose every other blocker (locked, uncommitted, template_error, occupied target) skips one worktree and carries on. Aborting the run would leave the other N-1 unrelocated, and would make passing --commit strictly less capable than omitting it.

$ wt step relocate --commit
▲ Skipping feature (1 path with unresolved conflicts)
  conflict.txt
✓ Relocated other: ~/code/wrong-clean → ~/code/myproject.other

--format=json records it as reason: "unmerged", alongside the existing codes.

Relocating a conflicted worktree without --commit is unchanged. Confirmed by hand that git worktree move carries the unmerged index, MERGE_HEAD, and the markers across, and that git still refuses to commit at the new path. Nothing stages there, so nothing is lost.

Detection stays git's

Nothing greps file contents for <<<<<<<. The whole chain is one structured question, git diff --name-only --diff-filter=U -z, which asks git which paths its index records as unmerged: the same question git commit asks itself. A content grep would false-positive on this very repo, whose docstrings contain the markers. The only marker-content checks in the diff are two test assertions, that the markers stay on disk for the user and never reach a commit.

Merged with #3587

#3587 landed while this was in flight (as did #3578, whose wt step push refusal test shares the same spot in merge.rs) and hoisted ensure_no_unmerged_paths("merge") to wt merge's entry, so merge refuses in its own name. The two are complementary: that PR gives each entry point a gate in the name the user typed, and this one guarantees the staging step itself is gated no matter who reaches it. Both sides edited the same docstring and added a test at the same spot; the merge keeps both paragraphs, lists all four entry points, and keeps both tests.

The interaction leaves one narrow seam. wt merge --no-squash, with a pre-commit hook that conflicts the index, now refuses with Cannot commit rather than Cannot merge, because the entry gate already passed and the funnel names the step it fired in. Fixing the wording would mean threading a caller-supplied noun into CommitOptions, which #3587 considered and rejected as the wrong shape, so it stays. The default squash path isn't affected: squash stages before its hooks, so the funnel fires ahead of them.

Verification

Three integration tests, each asserting HEAD is unmoved rather than only matching the message:

  • test_relocate_refuses_unmerged_paths, the reported bug.
  • test_relocate_unmerged_skips_only_that_worktree: one conflicted worktree, one merely dirty. The sibling still relocates, and JSON carries reason: "unmerged".
  • test_step_commit_refuses_hook_created_conflict, driven from a pre-commit hook, so it can only pass through the check inside stage.

Gate green on the merged head (4557/4557, lints, fmt, doc sync), plus cargo nextest run --features shell-integration-tests (4546/4546 pre-merge) and cargo clippy --all-targets --features shell-integration-tests, which the gate doesn't compile.

This was written by Claude Code on behalf of Maximilian

max-sixty and others added 2 commits July 24, 2026 18:12
`wt step relocate --commit` committed conflict markers — the bug #3579 fixed
everywhere else. It stages with `git add -A` and calls
`CommitGenerator::commit_staged_changes` directly, so it reached neither of
that PR's two gates:

    $ git merge side          # CONFLICT in f
    $ wt step relocate --commit
    ✓ Committed changes @ eb4cf19
    $ git show HEAD:f
    <<<<<<< HEAD
    …

`MERGE_HEAD` is gone and the tree is clean afterwards, so the broken merge
reads as finished.

## The gate belongs at the staging step, not the commit

The obvious funnel is `commit_staged_changes`, the one call all three paths
share. It cannot work: every caller stages *before* it, and `git add -A`
collapses an unmerged path's three index stages into one entry — after which
`git diff --diff-filter=U` is empty and git itself no longer refuses. A check
there passes vacuously in exactly the case it exists for.

So the funnel is `WorkingTree::stage(StageMode, action)`: it refuses, then
stages, with nothing in between. It is now the only path to `git add` against
a worktree's real index (the remaining calls write throwaway indexes via
`temp_index`), so a fifth entrypoint cannot forget the gate, and cannot order
it after the staging that destroys the evidence. It also replaces three
hand-rolled copies of the `StageMode` → `git add` match.

## It is not belt-and-suspenders

The callers keep their early refusals, which guard a different window: they
run before hooks, prompts, and the LLM call, so a doomed commit runs no
project commands. That placement leaves a hole only the funnel can close — a
`pre-commit` hook is arbitrary project code, free to conflict the index after
the early gate passed:

    pre-commit = "git merge side"     # in .config/wt.toml
    $ wt step commit --yes
    ✓ Committed changes @ 9e89c76     # markers and all

`test_step_commit_refuses_hook_created_conflict` pins it.

## relocate skips, rather than aborting

An unresolved conflict is a per-worktree blocker the user must fix by hand,
like a locked worktree — and relocate is a bulk command whose every other
blocker (locked, uncommitted, template_error, occupied target) skips one
worktree and carries on. Aborting the run would leave the other N-1
unrelocated, and adding `--commit` would make the command strictly less
capable than omitting it. So it warns, names the paths, records
`reason: "unmerged"`, and continues.

Relocating a conflicted worktree *without* `--commit` stays untouched:
verified that `git worktree move` carries the unmerged index, `MERGE_HEAD`,
and the markers across, and git still refuses to commit at the new path.
Nothing is staged there, so nothing is lost.

Gate green (4546/4546, lints, fmt, doc sync), plus `cargo nextest run
--features shell-integration-tests` (4546/4546) and
`cargo clippy --all-targets --features shell-integration-tests`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#3587 hoisted `ensure_no_unmerged_paths("merge")` to `wt merge`'s entry so it
refuses in its own name. Both sides edited the same docstring and added a test
at the same spot.

Docstring: keeps both paragraphs and lists all four entry points. The two
additions are complementary — #3587's explains why `action` is caller-supplied,
this branch's explains that every entry gate is the *early* refusal and points
at `stage` for the one adjacent to `git add`.

Tests: keeps both, entry-point refusals grouped before the funnel test.
main added test_step_push_refuses_mid_rebase at the same spot as this branch's
test_step_commit_refuses_hook_created_conflict. Both kept, entry-point refusals
grouped ahead of the funnel test.
@max-sixty
max-sixty merged commit 321ffe8 into main Jul 25, 2026
37 checks passed
@max-sixty
max-sixty deleted the fix-relocate-unmerged-paths-gate branch July 25, 2026 02:34
max-sixty added a commit that referenced this pull request Jul 25, 2026
Eleven commits landed on `main` while #3590 sat in CI, so they ship in
v0.69.2 with no changelog entry. Most are user-facing, and three are
data-loss fixes — exactly the drift the release skill's step-12 check
exists to catch. The tag is held until this lands.

Entries added, most-impactful first:

- **#3589** — two ways shell integration deleted user data on a
substring guess: an rc line that only *quotes* the init command, and a
user's own `conf.d/wt.fish` deleted outright by `install`'s legacy
cleanup. Plus forge detection moving from `host.contains("github")` to
label-wise matching.
- **#3585 + #3591** — truncate-in-place rc writes replaced by
write-temp-then-rename, then generalized to every user-file write.
- **#3578** — despite its `docs(step):` subject this carries three
behavior fixes, including `wt step push` succeeding mid-rebase and
moving the target branch onto a half-replayed history.
- **#3588 + #3587** — conflict markers can no longer reach a commit via
`wt step relocate --commit`, and `wt merge` refuses in its own name.
- **#3554** — OpenCode marker writes could land in another session's
worktree (thanks @4i3n6, who both reported and fixed it).

Not documented, per convention: #3574 (doc comments only, no runtime or
docs-site surface) and the three dependency bumps.

Also corrects `.github/CLAUDE.md`, which lists three required status
checks; there are four — `fast-checks` is required too, confirmed
against the branch-protection API.

Entries verified against the diffs by subagent. Two corrections came
back and are folded in: the #3589 entry originally promised "two of them
deleted" and described only one, and the #3578 entry mis-quoted the
success line as `✓ Pushed to main` when it carries a commit count.

> _This was written by Claude Code on behalf of max_
max-sixty added a commit that referenced this pull request Jul 25, 2026
…pping (#3598)

Three simplifications to the staging gate #3588 introduced, all
deletions.

## `stage` names the commit instead of taking a caller's name

`WorkingTree::stage` took an `action` string purely so its refusal could
name the command the user typed. But staging on the user's behalf
happens for exactly one reason — a commit is about to run — so the
string was a constant wearing a parameter's clothes. It now names the
commit, and three call-site strings go with it.

The entry gates keep their `action`, and the reason is worth stating
because it's what stopped me deleting the parameter outright. An entry
point often doesn't yet know it will commit at all:

```rust
current_wt.ensure_no_unmerged_paths("merge")?;   // src/commands/merge.rs:179
...
let ResolvedMergeFlags { commit, .. } = flags.resolve(&resolved.merge);   // :187
```

`wt merge --no-commit` resolves its flags *after* that gate, and
requires a clean tree — which an unmerged path is not. So a conflicted
`--no-commit` run hits the entry gate, and `Cannot commit` would be
actively wrong for a command explicitly told not to commit. The two
gates know different things, and now say different things for that
reason.

This also settles the seam #3588 documented as a tradeoff: `wt merge
--no-squash` reporting `Cannot commit` isn't a wrong label to be fixed
later, it's the funnel correctly naming the only step it guards, at a
point where the flags have resolved and the commit is certain.

## `CommitOptions::warn_about_untracked` was dead

It defaulted to `true`, and both assignments set it to `stage_mode ==
StageMode::All` immediately after assigning `stage_mode`:

```rust
options.stage_mode = stage_mode;
options.warn_about_untracked = stage_mode == super::commit::StageMode::All;
```

while the consumer already ANDs with the same condition:

```rust
if self.warn_about_untracked && self.stage_mode == StageMode::All {
```

No caller could make it `false` while the mode was `All`, so the field
never decided anything.

## One home for the `StageMode` → `git add` mapping

The mapping existed as two matches: one in `stage`, one in
`preview_commit` for the `--dry-run` temp index. Adding a variant meant
finding both, and missing the preview would silently produce a
`--dry-run` that disagreed with the real run. `StageMode::add_args` now
owns it. `stage`'s two per-arm `.context()` strings collapse into one,
which costs nothing the args don't already say.

## Also

`src/commands/step/squash.rs` justified its entry gate as refusing
"ahead of the hooks and the LLM call" — but `wt.stage` is ahead of both
too, so that reason no longer distinguished it. The gate is still worth
keeping; what it uniquely precedes is the pre-commit approval prompt, so
the comment says that now.

## Verification

No user-visible message changes, and the tests confirm it rather than
assert it: the pre-merge gate reports **no snapshots to review**. Every
refusal a test pins comes from an entry gate, which kept its name, and
the one funnel-driven snapshot
(`step_commit_refuses_hook_created_conflict`) already read `Cannot
commit`.

`cargo run -- hook pre-merge --yes` green (4562/4562, clippy, fmt,
doctests, doc sync). Also `cargo nextest run --features
shell-integration-tests` (4562/4562) and `cargo clippy --all-targets
--features shell-integration-tests`, which the gate doesn't compile —
the blind spot that matters for removing a struct field.
`RUSTDOCFLAGS=-Dwarnings cargo doc` clean for the new intra-doc link.

> _This was written by Claude Code on behalf of Maximilian_

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants