From 2517413baca74826d48aa761199fd379ef5deff7 Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Sat, 20 Jun 2026 21:37:28 +0100 Subject: [PATCH 1/2] refactor(cleanup): use deleteBranch use case instead of hand-rolled ladder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../use-cases/cleanup-worktrees.test.ts | 25 ++++++++++++++++ .../use-cases/cleanup-worktrees.ts | 29 ++++++------------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/application/use-cases/cleanup-worktrees.test.ts b/src/application/use-cases/cleanup-worktrees.test.ts index 9b1f0e6..090084c 100644 --- a/src/application/use-cases/cleanup-worktrees.test.ts +++ b/src/application/use-cases/cleanup-worktrees.test.ts @@ -330,6 +330,31 @@ describe("cleanupWorktrees", () => { }); }); + test("branch deletion failure surfaces as error report", async () => { + // Unmerged branch → force fallback. Stub deleteBranchForce to fail so the + // delete-branch use case returns `failed`, which cleanup must surface as + // an `error` report. + const baseGit = createFakeGit({ + worktrees: [mainWt, featureA], + branches: ["main", "feature-a"], + goneBranches: ["feature-a"], + mergedBranches: [], + commitCountMap: new Map([["main..feature-a", 3]]), + }); + const git = { + ...baseGit, + async deleteBranchForce(_branch: string) { + return { success: false as const, error: { code: "UNKNOWN" as const, message: "force boom" } }; + }, + }; + const output = expectOk(await cleanupWorktrees({ force: true, dryRun: false }, { git })); + + expect(output.reports[0]).toMatchObject({ + branch: "feature-a", + result: { status: "error", message: "force boom" }, + }); + }); + test("default branch is never cleaned", async () => { const git = createFakeGit({ worktrees: [mainWt], diff --git a/src/application/use-cases/cleanup-worktrees.ts b/src/application/use-cases/cleanup-worktrees.ts index 09cd0e9..267a8f0 100644 --- a/src/application/use-cases/cleanup-worktrees.ts +++ b/src/application/use-cases/cleanup-worktrees.ts @@ -1,6 +1,7 @@ import type { GitPort } from "../../domain/ports/git-port.ts"; import { Result as R, type Result } from "../../shared/result.ts"; import { classifyGoneBranch } from "./classify-gone-branch.ts"; +import { deleteBranch } from "./delete-branch.ts"; import { isFullyMerged } from "./is-fully-merged.ts"; export interface CleanupWorktreesInput { @@ -86,26 +87,14 @@ export async function cleanupWorktrees( } } - const deleteResult = await git.deleteBranch(branch); - if (!deleteResult.success) { - if (deleteResult.error.code === "BRANCH_NOT_MERGED") { - const forceResult = await git.deleteBranchForce(branch); - if (!forceResult.success) { - reports.push({ - branch, - worktreePath, - result: { status: "error", message: forceResult.error.message }, - }); - return; - } - } else { - reports.push({ - branch, - worktreePath, - result: { status: "error", message: deleteResult.error.message }, - }); - return; - } + const outcome = await deleteBranch({ branch, force: true, deleteRemote: false }, { git }); + if (outcome.status === "failed") { + reports.push({ + branch, + worktreePath, + result: { status: "error", message: outcome.message }, + }); + return; } reports.push({ From 7b512b38b256daa1b403b28b6d937e6599c7875b Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Sat, 20 Jun 2026 21:49:46 +0100 Subject: [PATCH 2/2] refactor(cleanup): handle delete-branch outcomes exhaustively Reviewers flagged that the `not-merged` outcome was silently treated as success (fell through to the `cleaned` / `branch-only` report) because `force: true` makes it statically unreachable today. That invariant lived only in the PR description. Replace the `if status === "failed"` check with an exhaustive `switch` on `outcome.status` so: - a future contract change in `deleteBranch` (new outcome, weakened force semantics) fails the typecheck instead of silently misreporting a non-deleted branch as `cleaned`; - the mapping (`deleted` / `failed` / `not-merged`) is documented in code, not only in the PR body. Observable behavior unchanged. Tests stay green. --- .../use-cases/cleanup-worktrees.ts | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/application/use-cases/cleanup-worktrees.ts b/src/application/use-cases/cleanup-worktrees.ts index 267a8f0..837ff84 100644 --- a/src/application/use-cases/cleanup-worktrees.ts +++ b/src/application/use-cases/cleanup-worktrees.ts @@ -87,21 +87,37 @@ export async function cleanupWorktrees( } } + // Mapping: + // - `deleted` → existing `cleaned` / `branch-only` report. + // - `failed` → `error` report carrying the git error message. + // - `not-merged` → unreachable because we pass `force: true`. Handled + // defensively so a future contract change in + // `deleteBranch` doesn't silently report a non-deleted + // branch as `cleaned`. const outcome = await deleteBranch({ branch, force: true, deleteRemote: false }, { git }); - if (outcome.status === "failed") { - reports.push({ - branch, - worktreePath, - result: { status: "error", message: outcome.message }, - }); - return; + switch (outcome.status) { + case "deleted": + reports.push({ + branch, + worktreePath, + result: { status: worktree ? "cleaned" : "branch-only" }, + }); + return; + case "failed": + reports.push({ + branch, + worktreePath, + result: { status: "error", message: outcome.message }, + }); + return; + case "not-merged": + reports.push({ + branch, + worktreePath, + result: { status: "error", message: `Branch "${branch}" is not fully merged` }, + }); + return; } - - reports.push({ - branch, - worktreePath, - result: { status: worktree ? "cleaned" : "branch-only" }, - }); }; if (goneBranches.length > 0) {