Skip to content

cleanup's safety gate skips the merge check for a pushed branch, and the finish prompt's comment claims the opposite of what it does #1032

Description

@thomasweld

Version: 0.14.2 (read from the published bundle; line numbers are dist/)

1. A branch that is pushed and level with its remote skips the merge check

validateWorktreeSafety (dist/chunk-2V2XIIY6.js) branches three ways on remote status, and the middle branch is empty:

} else if (remoteStatus.exists && remoteStatus.localAhead) {
  // blocks: "The remote branch exists but your local branch is ahead"
} else if (remoteStatus.exists && !remoteStatus.localAhead) {
  // nothing
} else if (!remoteStatus.exists) {
  const isMerged = await isBranchMergedIntoMain(worktree.branch, mainBranch, worktree.path);
}

checkRemoteBranchStatus (dist/index.js:2912) returns { exists: true, localAhead: false } when the remote head equals local, so a pushed, level branch lands in the empty branch and clears the gate with no merge test at all.

A branch existing on the remote says nothing about whether its work was merged. Today it is treated as though it does.

How a user reaches it

The worktree lookups differ between commands: il finish matches on branch name, il cleanup -i <id> matches on worktree path. So anything that changes the checked-out branch inside a worktree (a tool that runs git checkout -b and commits, for instance) leaves cleanup still finding the worktree, but targeting the new branch. That branch is pushed and level, so the gate passes, and the worktree is deleted while the original branch may be neither merged nor pushed.

2. The finish prompt's comment states the reverse of its behaviour

dist/cli.js:1293:

const shouldCleanup = await promptConfirmation(
  "Clean up worktree now?",
  true
  // Default to keeping worktree - won't delete if unmerged changes
);

promptConfirmation(message, defaultValue) renders [Y/n] when defaultValue is true (dist/chunk-NPVA65KS.js:134), so pressing Enter answers yes and the worktree is removed. The default is to delete, not to keep.

The second clause is also unreliable, for the reason in part 1: the merge check does not run on the empty branch. So a comment asserting safety sits on the line that sets a destructive default, and is wrong in both directions.

What already works, for contrast

The uncommitted-changes guard is solid: hasUncommittedChanges is an unconditional blocker inside validateWorktreeSafety (:726) and the finish prompt passes force: options.force ?? false (cli.js:1322), so answering y with a dirty tree fails safely with Cannot cleanup: Worktree has uncommitted changes. Worth noting because it shows the intended shape; the merge side just has a hole in it.

Also worth flagging: the entire validateWorktreeSafety call sits behind if (!options.force) (:135), so --force skips every check rather than only the merge test. The blocker text says "will discard changes", which covers it, but documentation describing --force as merely overriding the merge check would be understating it.

Suggested direction

Run the same merge check in the exists && !localAhead branch as in the !exists branch, and correct or remove the comment at cli.js:1296.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions