refactor(remove): extract branch-deletion policy into a use case#17
Merged
Conversation
… typed outcomes src/cli/commands/remove.ts had three diverged copies of the delete-and-classify step (single-worktree path, multi-worktree first pass, multi-worktree deferred force pass). Any fix had to be made three times, and the destructive policy lived in the untestable CLI layer. Extract a single deleteBranch use case that: - attempts the delete and falls back to deleteBranchForce when force is true - reports a typed DeleteBranchOutcome union (deleted/not-merged/failed) with a nested RemoteDeletionOutcome (deleted/not-found/failed/skipped) - relies on the contract-tested git error codes (BRANCH_NOT_MERGED, REMOTE_REF_NOT_FOUND) verified by the fake-vs-real-git contract suite remove.ts now renders the typed result at each former call site; retry-on-error logic is gone from the CLI. Interactive prompting (the single-mode immediate prompt and the multi-mode deferred batch prompt) stays in the CLI because it is a UI concern; both paths invoke the same use case. No user-visible behavior changes. New unit tests cover all five outcome shapes plus negative cases (no remote attempt on not-merged/failed).
…op dead surface Review follow-ups on the deleteBranch extraction: - single-worktree path: pass force:false on the first delete so the "not merged" warning and the "Force deleting" step are still shown under --force/--yes (the prior code force-deleted inside the use case on the first attempt, silently swallowing the unmerged signal — notably under --yes). shouldForce is computed from force||yes as before. - multi-worktree deferred pass: remove the now-unreachable `if (yes)` arm. unmergedBranches is only populated on a not-merged outcome, which requires force:false, i.e. yes===false — so that branch was dead. - drop the unused `mode: "normal" | "forced"` field from DeleteBranchOutcome; no caller reads it and tests already prove the force fallback via status:"deleted" on a known-unmerged branch. Simplifies the use case body. - tests: drop mode assertions, fix a stale comment, and add a single-path CLI regression test (the fake spinner now records its messages) so the --force-on-unmerged messaging can't silently regress again. Behavior change (documented): in the multi path a force-delete that itself fails is now reported as "branch delete failed" instead of being relabelled "kept — not fully merged" and offered a pointless retry.
epodivilov
added a commit
that referenced
this pull request
Jun 20, 2026
…adder PR #17 extracted the local branch-deletion policy (deleteBranch → BRANCH_NOT_MERGED fallback to deleteBranchForce) into a dedicated use case, but cleanup-worktrees.ts still carried a 4th copy of the same ladder that was missed during that refactor. Migrate that call site to deleteBranch({ branch, force: true, deleteRemote: false }) and map the typed outcome onto the existing cleanup reports: - deleted → cleaned / branch-only (unchanged) - failed → error with the git error message cleanup never touches the remote, so deleteRemote stays off; force=true mirrors the previous unconditional fallback, so observable behavior of wt cleanup is identical. Existing cleanup tests stay green. Added one new test exercising the failed-mapping branch.
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
1 participant
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
src/cli/commands/remove.ts(three diverged sites) into a singledeleteBranchuse case returning a typedDeleteBranchOutcomeunion (deleted/not-merged/failed) with a nestedRemoteDeletionOutcomefor the optional remote-delete step.remove.tsis now responsible only for rendering typed results — no retry-on-error logic in the CLI layer. The single-mode interactive prompt and the multi-mode deferred batch prompt remain in the CLI (UI concerns) and both invoke the same use case.FakeGit; the contract-tested git error codes (BRANCH_NOT_MERGED,REMOTE_REF_NOT_FOUND,BRANCH_NOT_FOUND) verified by the fake-vs-real-git contract suite (#35) ensure fidelity to real git.--force/--yes(the use case attempts the plain delete first; only the explicit force step falls back); completion messages and warnings are unchanged. In the multi-worktree path, a force-delete that itself fails is now reported as "branch delete failed" with the git error, instead of being relabelled "kept — not fully merged" and offered a retry that just fails again — the only intentional behavior change. The transient "Deleting remote branch..." spinner hint is gone in the single, multi, and deferred paths (not only multi, as originally noted), since the local and remote deletes are now sequenced inside the use case; final messages are unchanged. Internal: dropped the unusedmode: "normal" | "forced"field fromDeleteBranchOutcome— no caller read it.Test plan
pnpm typecheckpassespnpm lintpassespnpm format(no changes)pnpm test— 478 pass / 0 fail (12 unit tests fordeleteBranch+ 1 single-path CLI regression test)BRANCH_NOT_FOUND), force-fallback failure, remote=deleted, remote=not-found, remote=failed (warning), and that failed/not-merged outcomes never invokedeleteRemoteBranchremove.tsnow delegate to the use case