From f6de33f411213b2d9ceee1cbeaeaeb8d6bae84de Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Fri, 19 Jun 2026 14:13:32 +0100 Subject: [PATCH 1/2] refactor(remove): extract branch-deletion policy into a use case with 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). --- .../use-cases/delete-branch.test.ts | 148 ++++++++++++++++++ src/application/use-cases/delete-branch.ts | 81 ++++++++++ src/cli/commands/remove.ts | 134 +++++++--------- 3 files changed, 283 insertions(+), 80 deletions(-) create mode 100644 src/application/use-cases/delete-branch.test.ts create mode 100644 src/application/use-cases/delete-branch.ts diff --git a/src/application/use-cases/delete-branch.test.ts b/src/application/use-cases/delete-branch.test.ts new file mode 100644 index 0000000..b9e73a1 --- /dev/null +++ b/src/application/use-cases/delete-branch.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, test } from "bun:test"; +import { createFakeGit } from "../../test-utils/fake-git.ts"; +import { deleteBranch } from "./delete-branch.ts"; + +describe("deleteBranch", () => { + test("clean delete → deleted/normal, remote skipped", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "deleted", mode: "normal", remote: { status: "skipped" } }); + }); + + test("not-merged + force=false → not-merged (no force fallback)", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + // not in mergedBranches → deleteBranch returns BRANCH_NOT_MERGED + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "not-merged" }); + + // force-delete must NOT have been invoked — branch still listable + const branches = await git.listBranches(); + expect(branches.success && branches.data.includes("feature")).toBe(true); + }); + + test("not-merged + force=true → deleted/forced", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "deleted", mode: "forced", remote: { status: "skipped" } }); + + const branches = await git.listBranches(); + expect(branches.success && branches.data.includes("feature")).toBe(false); + }); + + test("delete fails with non-NOT_MERGED error → failed with message", async () => { + const git = createFakeGit({ + branches: ["main"], // "feature" missing → BRANCH_NOT_FOUND + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); + expect(outcome.status).toBe("failed"); + if (outcome.status === "failed") { + expect(outcome.message).toContain("feature"); + } + }); + + test("delete fails with non-NOT_MERGED error → failed even when force=true", async () => { + const git = createFakeGit({ + branches: ["main"], // BRANCH_NOT_FOUND is NOT recoverable via force + }); + const outcome = await deleteBranch({ branch: "ghost", force: true, deleteRemote: false }, { git }); + // deleteBranch returns BRANCH_NOT_FOUND, which is not BRANCH_NOT_MERGED, + // so force fallback must NOT be attempted. + expect(outcome.status).toBe("failed"); + }); + + test("force fallback fails → failed with force error message", async () => { + // Branch exists but is not merged; force-delete will also fail because + // after deleteBranch the branch is "marked" but still in branchStore; + // to force a failure we use a branch that doesn't exist for force. + // Simpler: stub a git where deleteBranchForce fails. + const baseGit = createFakeGit({ branches: ["main", "feature"] }); + const git = { + ...baseGit, + async deleteBranchForce(_branch: string) { + return { success: false as const, error: { code: "UNKNOWN" as const, message: "force boom" } }; + }, + }; + const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: false }, { git }); + expect(outcome).toEqual({ status: "failed", message: "force boom" }); + }); + + test("deleted + remote delete requested + remote ref exists → remote=deleted", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + remoteBranches: ["feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "deleted", mode: "normal", remote: { status: "deleted" } }); + }); + + test("deleted + remote delete requested + remote ref missing → remote=not-found", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + remoteBranches: [], // empty, but state-tracking is enabled because the option is provided + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "deleted", mode: "normal", remote: { status: "not-found" } }); + }); + + test("deleted + remote delete fails with other error → remote=failed", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + mergedBranches: ["feature"], + deleteRemoteBranchFail: { code: "UNKNOWN", message: "network down" }, + }); + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome.status).toBe("deleted"); + if (outcome.status === "deleted") { + expect(outcome.remote).toEqual({ status: "failed", message: "network down" }); + } + }); + + test("force fallback + remote delete → deleted/forced + remote=deleted", async () => { + const git = createFakeGit({ + branches: ["main", "feature"], + // not merged → force fallback + remoteBranches: ["feature"], + }); + const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "deleted", mode: "forced", remote: { status: "deleted" } }); + }); + + test("not-merged outcome must not attempt remote deletion", async () => { + let remoteCalls = 0; + const baseGit = createFakeGit({ branches: ["main", "feature"] }); + const git = { + ...baseGit, + async deleteRemoteBranch(branch: string, remote?: string) { + remoteCalls += 1; + return baseGit.deleteRemoteBranch(branch, remote); + }, + }; + const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); + expect(outcome).toEqual({ status: "not-merged" }); + expect(remoteCalls).toBe(0); + }); + + test("failed outcome must not attempt remote deletion", async () => { + let remoteCalls = 0; + const baseGit = createFakeGit({ branches: ["main"] }); + const git = { + ...baseGit, + async deleteRemoteBranch(branch: string, remote?: string) { + remoteCalls += 1; + return baseGit.deleteRemoteBranch(branch, remote); + }, + }; + const outcome = await deleteBranch({ branch: "ghost", force: false, deleteRemote: true }, { git }); + expect(outcome.status).toBe("failed"); + expect(remoteCalls).toBe(0); + }); +}); diff --git a/src/application/use-cases/delete-branch.ts b/src/application/use-cases/delete-branch.ts new file mode 100644 index 0000000..7344f4f --- /dev/null +++ b/src/application/use-cases/delete-branch.ts @@ -0,0 +1,81 @@ +import type { GitPort } from "../../domain/ports/git-port.ts"; +import { Result } from "../../shared/result.ts"; + +/** + * Result of attempting to delete the remote-tracking branch, when requested. + * + * - `deleted` — remote ref was removed successfully + * - `not-found` — remote returned REMOTE_REF_NOT_FOUND (treated as success: nothing to delete) + * - `failed` — any other remote failure (rendered as a warning in the CLI) + * - `skipped` — caller did not request remote deletion + */ +export type RemoteDeletionOutcome = + | { status: "deleted" } + | { status: "not-found" } + | { status: "failed"; message: string } + | { status: "skipped" }; + +/** + * Typed outcome of the delete-branch policy. + * + * - `deleted` — local branch was removed. `mode` records whether the normal + * delete succeeded or whether the force fallback was used. + * `remote` carries the remote-deletion sub-outcome. + * - `not-merged` — `deleteBranch` reported BRANCH_NOT_MERGED and the caller did + * not request force; no remote deletion was attempted. + * - `failed` — local deletion failed for any other reason, including a force + * fallback that itself failed. `message` is the git error message. + */ +export type DeleteBranchOutcome = + | { status: "deleted"; mode: "normal" | "forced"; remote: RemoteDeletionOutcome } + | { status: "not-merged" } + | { status: "failed"; message: string }; + +export interface DeleteBranchInput { + branch: string; + /** When true, fall back to deleteBranchForce on BRANCH_NOT_MERGED. */ + force: boolean; + /** When true, attempt to delete the remote-tracking branch after the local delete. */ + deleteRemote: boolean; +} + +export interface DeleteBranchDeps { + git: GitPort; +} + +export async function deleteBranch(input: DeleteBranchInput, deps: DeleteBranchDeps): Promise { + const { git } = deps; + const { branch, force, deleteRemote } = input; + + const deleteResult = await git.deleteBranch(branch); + + let mode: "normal" | "forced"; + if (Result.isOk(deleteResult)) { + mode = "normal"; + } else if (deleteResult.error.code === "BRANCH_NOT_MERGED") { + if (!force) { + return { status: "not-merged" }; + } + const forceResult = await git.deleteBranchForce(branch); + if (Result.isErr(forceResult)) { + return { status: "failed", message: forceResult.error.message }; + } + mode = "forced"; + } else { + return { status: "failed", message: deleteResult.error.message }; + } + + const remote = deleteRemote ? await deleteRemoteBranch(branch, git) : ({ status: "skipped" } as const); + return { status: "deleted", mode, remote }; +} + +async function deleteRemoteBranch(branch: string, git: GitPort): Promise { + const remoteResult = await git.deleteRemoteBranch(branch); + if (Result.isOk(remoteResult)) { + return { status: "deleted" }; + } + if (remoteResult.error.code === "REMOTE_REF_NOT_FOUND") { + return { status: "not-found" }; + } + return { status: "failed", message: remoteResult.error.message }; +} diff --git a/src/cli/commands/remove.ts b/src/cli/commands/remove.ts index 52f17c3..00095be 100644 --- a/src/cli/commands/remove.ts +++ b/src/cli/commands/remove.ts @@ -2,6 +2,7 @@ import { resolve } from "node:path"; import { defineCommand } from "citty"; import pc from "picocolors"; import * as v from "valibot"; +import { deleteBranch } from "../../application/use-cases/delete-branch.ts"; import { listWorktrees } from "../../application/use-cases/list-worktrees.ts"; import { loadConfig } from "../../application/use-cases/load-config.ts"; import { removeWorktree } from "../../application/use-cases/remove-worktree.ts"; @@ -181,53 +182,45 @@ export function removeCommand(container: Container) { if (shouldDeleteBranches && wt.branch) { spinner.start(`Deleting branch "${wt.branch}"...`); - let localDeleted = false; - const deleteResult = await git.deleteBranch(wt.branch); + let outcome = await deleteBranch( + { branch: wt.branch, force: force || yes, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); - if (Result.isErr(deleteResult)) { - if (deleteResult.error.code === "BRANCH_NOT_MERGED") { - spinner.stop(pc.yellow(`Branch "${wt.branch}" not merged`)); + if (outcome.status === "not-merged") { + spinner.stop(pc.yellow(`Branch "${wt.branch}" not merged`)); - let shouldForce = force || yes; - if (!shouldForce && !ui.nonInteractive) { - const forceConfirm = await ui.confirm({ - message: `Branch "${wt.branch}" is not merged. Force delete?`, - initialValue: false, - }); - shouldForce = !ui.isCancel(forceConfirm) && forceConfirm; - } + let shouldForce = false; + if (!ui.nonInteractive) { + const forceConfirm = await ui.confirm({ + message: `Branch "${wt.branch}" is not merged. Force delete?`, + initialValue: false, + }); + shouldForce = !ui.isCancel(forceConfirm) && forceConfirm; + } - if (shouldForce) { - spinner.start(`Force deleting branch "${wt.branch}"...`); - const forceResult = await git.deleteBranchForce(wt.branch); - if (Result.isErr(forceResult)) { - spinner.stop(pc.red(`Failed to delete branch "${wt.branch}"`)); - } else { - localDeleted = true; - } - } else { - ui.info(`Branch "${wt.branch}" was not deleted`); - } + if (shouldForce) { + spinner.start(`Force deleting branch "${wt.branch}"...`); + outcome = await deleteBranch( + { branch: wt.branch, force: true, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); } else { - spinner.stop(pc.red(`Failed to delete branch "${wt.branch}"`)); + ui.info(`Branch "${wt.branch}" was not deleted`); } - } else { - localDeleted = true; } - if (localDeleted && shouldDeleteRemoteBranches) { - spinner.message(`Deleting remote branch "${wt.branch}"...`); - const deleteRemoteResult = await git.deleteRemoteBranch(wt.branch); - if (Result.isErr(deleteRemoteResult)) { + if (outcome.status === "deleted") { + if (outcome.remote.status === "deleted") { + spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local & remote)`)); + } else { spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local)`)); - if (deleteRemoteResult.error.code !== "REMOTE_REF_NOT_FOUND") { - ui.warn(`Failed to delete remote branch: ${deleteRemoteResult.error.message}`); + if (outcome.remote.status === "failed") { + ui.warn(`Failed to delete remote branch: ${outcome.remote.message}`); } - } else { - spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local & remote)`)); } - } else if (localDeleted) { - spinner.stop(pc.green(`Branch "${wt.branch}" deleted (local)`)); + } else if (outcome.status === "failed") { + spinner.stop(pc.red(`Failed to delete branch "${wt.branch}"`)); } } } @@ -275,42 +268,26 @@ export function removeCommand(container: Container) { let branchStatus = ""; if (shouldDeleteBranches && wt.branch) { ms.update(wt.path, "deleting branch"); - let localDeleted = false; - const deleteResult = await git.deleteBranch(wt.branch); - - if (Result.isErr(deleteResult)) { - if (deleteResult.error.code === "BRANCH_NOT_MERGED") { - if (force || yes) { - const forceResult = await git.deleteBranchForce(wt.branch); - localDeleted = Result.isOk(forceResult); - } - if (!localDeleted) { - unmergedBranches.push(wt.branch); - branchStatus = " (branch kept — not fully merged)"; - } - } else { - branchStatus = " (branch delete failed)"; - warnings.push(`Failed to delete branch "${wt.branch}": ${deleteResult.error.message}`); - } - } else { - localDeleted = true; - } + const outcome = await deleteBranch( + { branch: wt.branch, force: force || yes, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); - if (localDeleted && shouldDeleteRemoteBranches) { - ms.update(wt.path, "deleting remote branch"); - const deleteRemoteResult = await git.deleteRemoteBranch(wt.branch); - if (Result.isOk(deleteRemoteResult)) { + if (outcome.status === "deleted") { + if (outcome.remote.status === "deleted") { branchStatus = " + branch deleted (local & remote)"; } else { branchStatus = " + branch deleted (local)"; - if (deleteRemoteResult.error.code !== "REMOTE_REF_NOT_FOUND") { - warnings.push( - `Failed to delete remote branch "${wt.branch}": ${deleteRemoteResult.error.message}`, - ); + if (outcome.remote.status === "failed") { + warnings.push(`Failed to delete remote branch "${wt.branch}": ${outcome.remote.message}`); } } - } else if (localDeleted) { - branchStatus = " + branch deleted (local)"; + } else if (outcome.status === "not-merged") { + unmergedBranches.push(wt.branch); + branchStatus = " (branch kept — not fully merged)"; + } else { + branchStatus = " (branch delete failed)"; + warnings.push(`Failed to delete branch "${wt.branch}": ${outcome.message}`); } } @@ -331,21 +308,18 @@ export function removeCommand(container: Container) { for (const branch of unmergedBranches) { const spinner = ui.createSpinner(); spinner.start(`Force deleting branch "${branch}"...`); - const forceResult = await git.deleteBranchForce(branch); - if (Result.isOk(forceResult)) { - if (shouldDeleteRemoteBranches) { - spinner.message(`Deleting remote branch "${branch}"...`); - const remoteResult = await git.deleteRemoteBranch(branch); - if (Result.isOk(remoteResult)) { - spinner.stop(pc.green(`Branch "${branch}" deleted (local & remote)`)); - } else { - spinner.stop(pc.green(`Branch "${branch}" deleted (local)`)); - if (remoteResult.error.code !== "REMOTE_REF_NOT_FOUND") { - ui.warn(`Failed to delete remote branch: ${remoteResult.error.message}`); - } - } + const outcome = await deleteBranch( + { branch, force: true, deleteRemote: shouldDeleteRemoteBranches }, + { git }, + ); + if (outcome.status === "deleted") { + if (outcome.remote.status === "deleted") { + spinner.stop(pc.green(`Branch "${branch}" deleted (local & remote)`)); } else { spinner.stop(pc.green(`Branch "${branch}" deleted (local)`)); + if (outcome.remote.status === "failed") { + ui.warn(`Failed to delete remote branch: ${outcome.remote.message}`); + } } } else { spinner.stop(pc.red(`Failed to delete branch "${branch}"`)); From 57e70f96b6e0d69f439bda7eda1dd4034c15c25f Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Fri, 19 Jun 2026 23:39:31 +0100 Subject: [PATCH 2/2] =?UTF-8?q?refactor(remove):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20preserve=20single-path=20messaging,=20drop=20dead?= =?UTF-8?q?=20surface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../use-cases/delete-branch.test.ts | 23 ++++--- src/application/use-cases/delete-branch.ts | 21 +++---- src/cli/commands/remove.test.ts | 61 ++++++++++++++++++- src/cli/commands/remove.ts | 10 ++- 4 files changed, 83 insertions(+), 32 deletions(-) diff --git a/src/application/use-cases/delete-branch.test.ts b/src/application/use-cases/delete-branch.test.ts index b9e73a1..97bffc1 100644 --- a/src/application/use-cases/delete-branch.test.ts +++ b/src/application/use-cases/delete-branch.test.ts @@ -3,13 +3,13 @@ import { createFakeGit } from "../../test-utils/fake-git.ts"; import { deleteBranch } from "./delete-branch.ts"; describe("deleteBranch", () => { - test("clean delete → deleted/normal, remote skipped", async () => { + test("clean delete → deleted, remote skipped", async () => { const git = createFakeGit({ branches: ["main", "feature"], mergedBranches: ["feature"], }); const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: false }, { git }); - expect(outcome).toEqual({ status: "deleted", mode: "normal", remote: { status: "skipped" } }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "skipped" } }); }); test("not-merged + force=false → not-merged (no force fallback)", async () => { @@ -25,12 +25,12 @@ describe("deleteBranch", () => { expect(branches.success && branches.data.includes("feature")).toBe(true); }); - test("not-merged + force=true → deleted/forced", async () => { + test("not-merged + force=true → deleted (force fallback)", async () => { const git = createFakeGit({ branches: ["main", "feature"], }); const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: false }, { git }); - expect(outcome).toEqual({ status: "deleted", mode: "forced", remote: { status: "skipped" } }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "skipped" } }); const branches = await git.listBranches(); expect(branches.success && branches.data.includes("feature")).toBe(false); @@ -58,10 +58,9 @@ describe("deleteBranch", () => { }); test("force fallback fails → failed with force error message", async () => { - // Branch exists but is not merged; force-delete will also fail because - // after deleteBranch the branch is "marked" but still in branchStore; - // to force a failure we use a branch that doesn't exist for force. - // Simpler: stub a git where deleteBranchForce fails. + // "feature" is not merged, so the normal delete returns BRANCH_NOT_MERGED and + // triggers the force fallback. Stub deleteBranchForce to fail so we exercise the + // "force fallback itself failed → failed" path. const baseGit = createFakeGit({ branches: ["main", "feature"] }); const git = { ...baseGit, @@ -80,7 +79,7 @@ describe("deleteBranch", () => { remoteBranches: ["feature"], }); const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); - expect(outcome).toEqual({ status: "deleted", mode: "normal", remote: { status: "deleted" } }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "deleted" } }); }); test("deleted + remote delete requested + remote ref missing → remote=not-found", async () => { @@ -90,7 +89,7 @@ describe("deleteBranch", () => { remoteBranches: [], // empty, but state-tracking is enabled because the option is provided }); const outcome = await deleteBranch({ branch: "feature", force: false, deleteRemote: true }, { git }); - expect(outcome).toEqual({ status: "deleted", mode: "normal", remote: { status: "not-found" } }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "not-found" } }); }); test("deleted + remote delete fails with other error → remote=failed", async () => { @@ -106,14 +105,14 @@ describe("deleteBranch", () => { } }); - test("force fallback + remote delete → deleted/forced + remote=deleted", async () => { + test("force fallback + remote delete → deleted + remote=deleted", async () => { const git = createFakeGit({ branches: ["main", "feature"], // not merged → force fallback remoteBranches: ["feature"], }); const outcome = await deleteBranch({ branch: "feature", force: true, deleteRemote: true }, { git }); - expect(outcome).toEqual({ status: "deleted", mode: "forced", remote: { status: "deleted" } }); + expect(outcome).toEqual({ status: "deleted", remote: { status: "deleted" } }); }); test("not-merged outcome must not attempt remote deletion", async () => { diff --git a/src/application/use-cases/delete-branch.ts b/src/application/use-cases/delete-branch.ts index 7344f4f..23cd9aa 100644 --- a/src/application/use-cases/delete-branch.ts +++ b/src/application/use-cases/delete-branch.ts @@ -18,16 +18,16 @@ export type RemoteDeletionOutcome = /** * Typed outcome of the delete-branch policy. * - * - `deleted` — local branch was removed. `mode` records whether the normal - * delete succeeded or whether the force fallback was used. - * `remote` carries the remote-deletion sub-outcome. + * - `deleted` — local branch was removed (either by the normal delete or by + * the force fallback). `remote` carries the remote-deletion + * sub-outcome. * - `not-merged` — `deleteBranch` reported BRANCH_NOT_MERGED and the caller did * not request force; no remote deletion was attempted. * - `failed` — local deletion failed for any other reason, including a force * fallback that itself failed. `message` is the git error message. */ export type DeleteBranchOutcome = - | { status: "deleted"; mode: "normal" | "forced"; remote: RemoteDeletionOutcome } + | { status: "deleted"; remote: RemoteDeletionOutcome } | { status: "not-merged" } | { status: "failed"; message: string }; @@ -49,10 +49,10 @@ export async function deleteBranch(input: DeleteBranchInput, deps: DeleteBranchD const deleteResult = await git.deleteBranch(branch); - let mode: "normal" | "forced"; - if (Result.isOk(deleteResult)) { - mode = "normal"; - } else if (deleteResult.error.code === "BRANCH_NOT_MERGED") { + if (Result.isErr(deleteResult)) { + if (deleteResult.error.code !== "BRANCH_NOT_MERGED") { + return { status: "failed", message: deleteResult.error.message }; + } if (!force) { return { status: "not-merged" }; } @@ -60,13 +60,10 @@ export async function deleteBranch(input: DeleteBranchInput, deps: DeleteBranchD if (Result.isErr(forceResult)) { return { status: "failed", message: forceResult.error.message }; } - mode = "forced"; - } else { - return { status: "failed", message: deleteResult.error.message }; } const remote = deleteRemote ? await deleteRemoteBranch(branch, git) : ({ status: "skipped" } as const); - return { status: "deleted", mode, remote }; + return { status: "deleted", remote }; } async function deleteRemoteBranch(branch: string, git: GitPort): Promise { diff --git a/src/cli/commands/remove.test.ts b/src/cli/commands/remove.test.ts index 7cfa7dc..91da1ab 100644 --- a/src/cli/commands/remove.test.ts +++ b/src/cli/commands/remove.test.ts @@ -27,13 +27,21 @@ interface FakeMultiSpinnerLog { fail: MultiSpinnerCall[]; } +interface FakeSpinnerLog { + start: string[]; + message: string[]; + stop: string[]; +} + function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multiselectResult?: string[] } = {}): { ui: UiPort; log: FakeUiLog; multiSpinnerLog: FakeMultiSpinnerLog; + spinnerLog: FakeSpinnerLog; } { const log: FakeUiLog = { info: [], success: [], warn: [], error: [], outro: [] }; const multiSpinnerLog: FakeMultiSpinnerLog = { complete: [], fail: [] }; + const spinnerLog: FakeSpinnerLog = { start: [], message: [], stop: [] }; const ui = { nonInteractive: opts.nonInteractive ?? false, @@ -57,7 +65,17 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi return fn(); }, createSpinner() { - return { start() {}, message() {}, stop() {} }; + return { + start(message: string) { + spinnerLog.start.push(message); + }, + message(message: string) { + spinnerLog.message.push(message); + }, + stop(message?: string) { + if (message !== undefined) spinnerLog.stop.push(message); + }, + }; }, createMultiSpinner(_keys: string[]) { return { @@ -89,7 +107,7 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi cancel() {}, } satisfies UiPort; - return { ui, log, multiSpinnerLog }; + return { ui, log, multiSpinnerLog, spinnerLog }; } function buildContainer( @@ -251,3 +269,42 @@ describe("remove — multi path branch delete failures", () => { expect(code).toBe(0); }); }); + +describe("remove — single path force on unmerged branch", () => { + // Regression for the branch-deletion-policy extraction: with --force/--yes on an + // unmerged branch the CLI must still surface the "not merged" warning and the + // "Force deleting" step before deleting, not silently force on the first attempt. + test("--yes on unmerged branch → shows 'not merged' + 'Force deleting', then deletes", async () => { + const fs = createFakeFilesystem({ + files: { [`${ROOT}/${CONFIG_FILENAME}`]: JSON.stringify({ rootDir: ".worktrees" }) }, + directories: [ROOT, `${ROOT}/.worktrees`, featureWt.path], + }); + const git = createFakeGit({ + root: ROOT, + mainRoot: ROOT, + worktrees: [mainWt, featureWt], + branches: ["main", "feature"], + mergedBranches: [], // "feature" not merged → BRANCH_NOT_MERGED on the normal delete + }); + const { ui, spinnerLog } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + "delete-remote-branch": false, + yes: true, + force: false, + "dry-run": false, + }); + + expect(spinnerLog.stop.some((m) => m.includes("not merged"))).toBe(true); + expect(spinnerLog.start.some((m) => m.includes("Force deleting"))).toBe(true); + expect(spinnerLog.stop.some((m) => m.includes("deleted (local)"))).toBe(true); + + const branches = await git.listBranches(); + expect(branches.success && branches.data.includes("feature")).toBe(false); + + expect(code).toBe(0); + }); +}); diff --git a/src/cli/commands/remove.ts b/src/cli/commands/remove.ts index 00095be..86f265e 100644 --- a/src/cli/commands/remove.ts +++ b/src/cli/commands/remove.ts @@ -183,15 +183,15 @@ export function removeCommand(container: Container) { spinner.start(`Deleting branch "${wt.branch}"...`); let outcome = await deleteBranch( - { branch: wt.branch, force: force || yes, deleteRemote: shouldDeleteRemoteBranches }, + { branch: wt.branch, force: false, deleteRemote: shouldDeleteRemoteBranches }, { git }, ); if (outcome.status === "not-merged") { spinner.stop(pc.yellow(`Branch "${wt.branch}" not merged`)); - let shouldForce = false; - if (!ui.nonInteractive) { + let shouldForce = force || yes; + if (!shouldForce && !ui.nonInteractive) { const forceConfirm = await ui.confirm({ message: `Branch "${wt.branch}" is not merged. Force delete?`, initialValue: false, @@ -327,9 +327,7 @@ export function removeCommand(container: Container) { } }; - if (yes) { - await forceDeleteUnmerged(); - } else if (!ui.nonInteractive) { + if (!ui.nonInteractive) { const confirmMessage = unmergedBranches.length === 1 ? `Branch "${unmergedBranches[0]}" is not fully merged. Force delete?`