From 3e84dcef6c71203aa103ef1090eb6eee34055f07 Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Sat, 20 Jun 2026 21:38:28 +0100 Subject: [PATCH 1/2] fix(remove): suppress remote-delete prompt for --yes/--non-interactive/--dry-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running `wt remove --delete-branch --yes` still prompted "Also delete remote branch?" because resolveDeleteRemoteBranch only short- circuited on ui.nonInteractive. That blocks agent/CI callers. --dry-run had the same problem: the prompt fired before the plan was printed. Resolution rule (now documented at the resolvers and in CLI help): 1. explicit --delete-branch / --delete-remote-branch wins 2. config value (remove.deleteBranch / remove.deleteRemoteBranch) 3. --yes, global --non-interactive, or --dry-run → default to false (destructive defaults must require explicit opt-in) 4. otherwise prompt Applied symmetrically to resolveDeleteBranch — same non-interactive contract for the local-branch prompt. Closes Vikunja #49 (id 311). --- src/cli/commands/remove.test.ts | 118 +++++++++++++++++++++++++++++++- src/cli/commands/remove.ts | 12 ++-- src/cli/resolve-params.ts | 20 ++++-- 3 files changed, 137 insertions(+), 13 deletions(-) diff --git a/src/cli/commands/remove.test.ts b/src/cli/commands/remove.test.ts index 91da1ab..4a23775 100644 --- a/src/cli/commands/remove.test.ts +++ b/src/cli/commands/remove.test.ts @@ -38,10 +38,12 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi log: FakeUiLog; multiSpinnerLog: FakeMultiSpinnerLog; spinnerLog: FakeSpinnerLog; + confirmMessages: string[]; } { const log: FakeUiLog = { info: [], success: [], warn: [], error: [], outro: [] }; const multiSpinnerLog: FakeMultiSpinnerLog = { complete: [], fail: [] }; const spinnerLog: FakeSpinnerLog = { start: [], message: [], stop: [] }; + const confirmMessages: string[] = []; const ui = { nonInteractive: opts.nonInteractive ?? false, @@ -92,7 +94,8 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi async text() { return ""; }, - async confirm() { + async confirm(options: { message: string }) { + confirmMessages.push(options.message); return opts.confirm ?? true; }, async select() { @@ -107,7 +110,7 @@ function createFakeUi(opts: { nonInteractive?: boolean; confirm?: boolean; multi cancel() {}, } satisfies UiPort; - return { ui, log, multiSpinnerLog, spinnerLog }; + return { ui, log, multiSpinnerLog, spinnerLog, confirmMessages }; } function buildContainer( @@ -270,6 +273,117 @@ describe("remove — multi path branch delete failures", () => { }); }); +describe("remove — non-interactive suppression of remote-delete prompt", () => { + // Regression for Vikunja #49: `wt remove --delete-branch --force --yes` + // must NOT prompt "Also delete remote branch?". The same applies to global + // --non-interactive and to --dry-run. Default decision in all these cases: + // do NOT delete the remote branch (explicit opt-in required via flag or config). + + function singleRemoveFs() { + return createFakeFilesystem({ + files: { [`${ROOT}/${CONFIG_FILENAME}`]: JSON.stringify({ rootDir: ".worktrees" }) }, + directories: [ROOT, `${ROOT}/.worktrees`, featureWt.path], + }); + } + + function singleRemoveGit() { + return createFakeGit({ + root: ROOT, + mainRoot: ROOT, + worktrees: [mainWt, featureWt], + branches: ["main", "feature"], + mergedBranches: ["feature"], + }); + } + + test("--yes with --delete-branch but no --delete-remote-branch → no prompt, remote NOT deleted", async () => { + const fs = singleRemoveFs(); + const git = singleRemoveGit(); + const { ui, spinnerLog, confirmMessages } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + // "delete-remote-branch" intentionally omitted + yes: true, + force: false, + "dry-run": false, + }); + + // No prompt of any kind. + expect(confirmMessages.length).toBe(0); + // Local branch deleted, remote NOT touched. + expect(spinnerLog.stop.some((m) => m.includes("deleted (local)"))).toBe(true); + expect(spinnerLog.stop.some((m) => m.includes("local & remote"))).toBe(false); + expect(code).toBe(0); + }); + + test("--non-interactive with --delete-branch but no --delete-remote-branch → no prompt, remote NOT deleted", async () => { + const fs = singleRemoveFs(); + const git = singleRemoveGit(); + const { ui, spinnerLog, confirmMessages } = createFakeUi({ nonInteractive: true }); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + yes: false, + force: false, + "dry-run": false, + }); + + expect(confirmMessages.length).toBe(0); + expect(spinnerLog.stop.some((m) => m.includes("deleted (local)"))).toBe(true); + expect(spinnerLog.stop.some((m) => m.includes("local & remote"))).toBe(false); + expect(code).toBe(0); + }); + + test("--dry-run with --delete-branch (no --delete-remote-branch) → no prompt, plan reflects local-only", async () => { + const fs = singleRemoveFs(); + const git = singleRemoveGit(); + const { ui, log, confirmMessages } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + yes: false, + force: false, + "dry-run": true, + }); + + expect(confirmMessages.length).toBe(0); + // Plan: local-only, not "local & remote". + const planLine = log.info.find((m) => m.includes("Would delete branch")); + expect(planLine).toBeDefined(); + expect(planLine).toContain("local"); + expect(planLine).not.toContain("local & remote"); + expect(log.outro.some((m) => m.includes("Dry run"))).toBe(true); + expect(code).toBe(0); + }); + + test("--yes with explicit --delete-remote-branch=true → no prompt, remote DELETED", async () => { + const fs = singleRemoveFs(); + const git = singleRemoveGit(); + const { ui, spinnerLog, confirmMessages } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + "delete-remote-branch": true, + yes: true, + force: false, + "dry-run": false, + }); + + expect(confirmMessages.length).toBe(0); + expect(spinnerLog.stop.some((m) => m.includes("local & remote"))).toBe(true); + 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 diff --git a/src/cli/commands/remove.ts b/src/cli/commands/remove.ts index 86f265e..9293c39 100644 --- a/src/cli/commands/remove.ts +++ b/src/cli/commands/remove.ts @@ -27,12 +27,13 @@ export function removeCommand(container: Container) { }, "delete-branch": { type: "boolean", - description: "Delete the branch after removing worktree", + description: + "Delete the branch after removing worktree (defaults to false in --yes/--non-interactive/--dry-run)", required: false, }, "delete-remote-branch": { type: "boolean", - description: "Delete the remote branch", + description: "Delete the remote branch (defaults to false in --yes/--non-interactive/--dry-run)", required: false, }, force: { @@ -42,7 +43,8 @@ export function removeCommand(container: Container) { }, yes: { type: "boolean", - description: "Confirm destructive operations without prompting", + description: + "Skip confirmation prompts. Safe defaults: branch/remote-branch deletion stays off unless explicitly opted in via flag or config", required: false, }, "dry-run": { @@ -80,7 +82,7 @@ export function removeCommand(container: Container) { ? await resolveDeleteBranch( parsed["delete-branch"], config?.remove.deleteBranch, - { ui }, + { ui, yes, dryRun }, { branches: branchCapableNames, }, @@ -92,7 +94,7 @@ export function removeCommand(container: Container) { ? await resolveDeleteRemoteBranch( parsed["delete-remote-branch"], config?.remove.deleteRemoteBranch, - { ui }, + { ui, yes, dryRun }, { branches: branchCapableNames }, ) : false; diff --git a/src/cli/resolve-params.ts b/src/cli/resolve-params.ts index 328a92c..91a5de6 100644 --- a/src/cli/resolve-params.ts +++ b/src/cli/resolve-params.ts @@ -243,19 +243,27 @@ export async function resolveWorktreesToRemove( }); } +// Resolution order for both resolvers below: +// 1. explicit CLI flag (--delete-branch / --delete-remote-branch) wins +// 2. config value (remove.deleteBranch / remove.deleteRemoteBranch) +// 3. non-interactive contexts (`--yes`, global `--non-interactive`, `--dry-run`) +// → default to `false`. Destructive defaults must require explicit opt-in, +// so agents and CI never accidentally delete branches or remote refs. +// 4. interactive fallback: prompt the user. + export async function resolveDeleteRemoteBranch( flag: boolean | undefined, configValue: boolean | undefined, - deps: { ui: UiPort }, + deps: { ui: UiPort; yes: boolean; dryRun: boolean }, context: { branches: string[] }, ): Promise { if (flag !== undefined) return flag; if (configValue !== undefined) return configValue; - const { ui } = deps; + const { ui, yes, dryRun } = deps; - if (ui.nonInteractive) return false; + if (ui.nonInteractive || yes || dryRun) return false; const message = context.branches.length > 1 @@ -268,16 +276,16 @@ export async function resolveDeleteRemoteBranch( export async function resolveDeleteBranch( flag: boolean | undefined, configValue: boolean | undefined, - deps: { ui: UiPort }, + deps: { ui: UiPort; yes: boolean; dryRun: boolean }, context: { branches: string[] }, ): Promise { if (flag !== undefined) return flag; if (configValue !== undefined) return configValue; - const { ui } = deps; + const { ui, yes, dryRun } = deps; - if (ui.nonInteractive) return false; + if (ui.nonInteractive || yes || dryRun) return false; const message = context.branches.length > 1 From 73198f0222fab77683869ab521613a7c6d12bd3e Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Sat, 20 Jun 2026 21:52:34 +0100 Subject: [PATCH 2/2] =?UTF-8?q?refactor(remove):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20cover=20yes-gate,=20tighten=20help=20text?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of PR #21 (Vikunja #49) flagged two follow-ups: 1. Test gap: all four new tests in the non-interactive describe block passed `delete-branch: true`, so resolution short-circuited at step 1 (explicit flag wins) and the new yes-gate in `resolveDeleteBranch` was never exercised. Added three cases: - `--yes` alone (no flag, no config) → no prompt, no branch delete - `--yes` with explicit `--delete-remote-branch=false` (opt-out beats yes-gate) - `--yes` with config `remove.deleteRemoteBranch=true` (config beats yes-gate) 2. Help-text drift: the three new `--help` strings stuffed the same `--yes/--non-interactive/--dry-run` triplet into every flag, breaking the codebase's short-imperative style. Hoisted the caveat once into the `--yes` description and reverted `--delete-branch` / `--delete-remote-branch` to their original one-liners. Also tweaked the resolution-order comment: bullet 3 said "default to false" but the resolvers actually return false AND skip the prompt; reworded to match. --- src/cli/commands/remove.test.ts | 74 +++++++++++++++++++++++++++++++++ src/cli/commands/remove.ts | 8 ++-- src/cli/resolve-params.ts | 5 ++- 3 files changed, 80 insertions(+), 7 deletions(-) diff --git a/src/cli/commands/remove.test.ts b/src/cli/commands/remove.test.ts index 4a23775..2742544 100644 --- a/src/cli/commands/remove.test.ts +++ b/src/cli/commands/remove.test.ts @@ -382,6 +382,80 @@ describe("remove — non-interactive suppression of remote-delete prompt", () => expect(spinnerLog.stop.some((m) => m.includes("local & remote"))).toBe(true); expect(code).toBe(0); }); + + test("--yes alone (no --delete-branch flag, no config) → no prompt, branch NOT deleted", async () => { + // Exercises the new yes-gate in resolveDeleteBranch: with neither flag nor config, + // --yes must default to false instead of prompting "Also delete branch?". + const fs = singleRemoveFs(); + const git = singleRemoveGit(); + const { ui, spinnerLog, confirmMessages } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + // "delete-branch" omitted on purpose + yes: true, + force: false, + "dry-run": false, + }); + + expect(confirmMessages.length).toBe(0); + // Worktree removed, but no branch-delete spinner fired. + expect(spinnerLog.stop.some((m) => m.includes("removed"))).toBe(true); + expect(spinnerLog.start.some((m) => m.includes("Deleting branch"))).toBe(false); + expect(code).toBe(0); + }); + + test("--yes with explicit --delete-remote-branch=false → no prompt, remote NOT deleted", async () => { + // Explicit opt-out must beat the yes-gate (step 1 of resolution order). + const fs = singleRemoveFs(); + const git = singleRemoveGit(); + const { ui, spinnerLog, confirmMessages } = 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(confirmMessages.length).toBe(0); + expect(spinnerLog.stop.some((m) => m.includes("deleted (local)"))).toBe(true); + expect(spinnerLog.stop.some((m) => m.includes("local & remote"))).toBe(false); + expect(code).toBe(0); + }); + + test("--yes with config remove.deleteRemoteBranch=true → no prompt, remote DELETED", async () => { + // Config opt-in (step 2) must beat the yes-gate (step 3). + const fs = createFakeFilesystem({ + files: { + [`${ROOT}/${CONFIG_FILENAME}`]: JSON.stringify({ + rootDir: ".worktrees", + remove: { deleteRemoteBranch: true }, + }), + }, + directories: [ROOT, `${ROOT}/.worktrees`, featureWt.path], + }); + const git = singleRemoveGit(); + const { ui, spinnerLog, confirmMessages } = createFakeUi(); + const container = buildContainer(ui, git, fs); + + const code = await runRemove(container, { + branch: "feature", + "delete-branch": true, + // "delete-remote-branch" omitted — config decides + yes: true, + force: false, + "dry-run": false, + }); + + expect(confirmMessages.length).toBe(0); + expect(spinnerLog.stop.some((m) => m.includes("local & remote"))).toBe(true); + expect(code).toBe(0); + }); }); describe("remove — single path force on unmerged branch", () => { diff --git a/src/cli/commands/remove.ts b/src/cli/commands/remove.ts index 9293c39..7de8f4d 100644 --- a/src/cli/commands/remove.ts +++ b/src/cli/commands/remove.ts @@ -27,13 +27,12 @@ export function removeCommand(container: Container) { }, "delete-branch": { type: "boolean", - description: - "Delete the branch after removing worktree (defaults to false in --yes/--non-interactive/--dry-run)", + description: "Delete the branch after removing worktree", required: false, }, "delete-remote-branch": { type: "boolean", - description: "Delete the remote branch (defaults to false in --yes/--non-interactive/--dry-run)", + description: "Delete the remote branch", required: false, }, force: { @@ -43,8 +42,7 @@ export function removeCommand(container: Container) { }, yes: { type: "boolean", - description: - "Skip confirmation prompts. Safe defaults: branch/remote-branch deletion stays off unless explicitly opted in via flag or config", + description: "Skip confirmation prompts (branch deletion stays off unless opted in via flag or config)", required: false, }, "dry-run": { diff --git a/src/cli/resolve-params.ts b/src/cli/resolve-params.ts index 91a5de6..e826907 100644 --- a/src/cli/resolve-params.ts +++ b/src/cli/resolve-params.ts @@ -247,8 +247,9 @@ export async function resolveWorktreesToRemove( // 1. explicit CLI flag (--delete-branch / --delete-remote-branch) wins // 2. config value (remove.deleteBranch / remove.deleteRemoteBranch) // 3. non-interactive contexts (`--yes`, global `--non-interactive`, `--dry-run`) -// → default to `false`. Destructive defaults must require explicit opt-in, -// so agents and CI never accidentally delete branches or remote refs. +// → return `false` and skip the prompt. Destructive defaults must require +// explicit opt-in, so agents and CI never accidentally delete branches or +// remote refs. // 4. interactive fallback: prompt the user. export async function resolveDeleteRemoteBranch(