Skip to content

Commit ce3bb39

Browse files
authored
🤖 fix: delete local branch on workspace delete (#1292)
This changes workspace deletion for **local worktree** workspaces to also delete the associated **local git branch**. - Normal delete uses `git branch -d` (safe; only deletes if merged) - Force delete uses `git branch -D` - Adds guardrails to avoid deleting protected branches (e.g. `main`) or branches still checked out elsewhere UI copy is updated (desktop + mobile) so users understand that deleting a workspace also deletes the branch. --- <details> <summary>📋 Implementation Plan</summary> # Plan: Delete workspace also deletes its git branch ## Summary (what will change) - **Archive** remains non-destructive (metadata timestamp only). - **Delete workspace** will remove: 1) the git worktree directory *(existing)* 2) the associated **local git branch** by default *(new)* Behavior details (per decisions captured): - **Safe by default**: normal delete uses `git branch -d <branch>` (only deletes if git considers it merged). - **Force delete** uses `git branch -D <branch>` (matches existing `force: true` semantics). - **No remote deletion**: we will not delete `origin/<branch>`. - **SSH runtimes**: keep simplest safe behavior (directory removal only; no explicit remote branch deletion). **Net LoC estimate (prod code): ~80–140 LoC** (mostly `WorktreeRuntime` + UI copy). ## Current behavior (confirmed in repo) - Delete call flow: - `WorkspaceContext.removeWorkspace()` → `api.workspace.remove()` → `workspaceService.remove()` → `runtime.deleteWorkspace()` - `WorktreeRuntime.deleteWorkspace()` already does **best-effort branch deletion**, but **only** when the workspace name starts with `agent_`. - `SSHRuntime.deleteWorkspace()` deletes the remote directory only (no explicit `git branch -d/-D`). ## Recommended approach ### 1) Backend: generalize branch deletion in `WorktreeRuntime.deleteWorkspace` Files: - `src/node/runtime/WorktreeRuntime.ts` - (optional helper) `src/node/git.ts` Steps: 1. **Change the branch-deletion gate** - Today: - `shouldDeleteBranch = !isInPlace && workspaceName.startsWith("agent_")` - Proposed: - `shouldDeleteBranch = !isInPlace` (delete branch for all worktree workspaces, not just `agent_*`). 2. **Add safety checks before deleting the branch** (defensive, and errs on “skip deletion” rather than “delete the wrong thing”): - If branch does **not** exist locally (`await listLocalBranches(projectPath)`): skip. - If branch is the repo’s **detected trunk** (`detectDefaultTrunkBranch(projectPath)`) or the **current branch** in the main worktree (`getCurrentBranch(projectPath)`): skip. - If branch is still **checked out by another worktree** (parse `git -C <projectPath> worktree list --porcelain` and see if any remaining worktree reports `branch refs/heads/<name>`): skip. 3. **Deletion command** - Keep existing `deleteFlag` behavior: - `force=false` → `git branch -d "<branch>"` - `force=true` → `git branch -D "<branch>"` 4. **Keep deletion best-effort** - Continue treating branch cleanup as **non-blocking** (never fail workspace delete just because `git branch -d` failed). - Upgrade logging so it’s clear whether we skipped due to guardrails vs git failure. ### 2) UI: update delete UX copy to reflect branch deletion Goal: make it obvious that “Delete” now also targets the branch. Files / call sites to update: - Desktop command palette: `src/browser/utils/commands/sources.ts` (`confirm("Remove current workspace…")`) - Archived view: - `src/browser/components/ArchivedWorkspaces.tsx` tooltip/copy around “Delete permanently” - `src/browser/components/ForceDeleteModal.tsx` warning text (“Force delete” should mention branch deletion too) - Mobile: `mobile/src/screens/ProjectsScreen.tsx` delete confirmation + force delete wording Copy guidance: - Mention **local branch** by name where feasible. - Mention that normal delete is “safe” (may not delete branch if unmerged) and that Force Delete is destructive. ### 3) Tests 1. **Unit tests** (`src/node/runtime/WorktreeRuntime.test.ts`) - Add coverage for a non-`agent_` branch: - Force delete (`force=true`) removes the branch. - Add coverage for safe delete (`force=false`) on a merged branch: - Create branch → commit → merge into main → delete workspace → branch removed with `-d`. - Add a guardrail test: - Workspace on `main` can be deleted as a worktree, but **`main` branch must not be deleted**. 2. **Integration test** (optional but valuable) - Extend `tests/ipc/removeWorkspace.test.ts` for `local` runtime: - Create workspace branch, then remove with `{ options: { force: true } }`, assert `git branch --list <branch>` is empty in the main repo. <details> <summary>Alternatives considered (not recommended for this iteration)</summary> - **Add a “keep branch” checkbox / option to workspace.remove** - Pros: avoids deleting user-provided existing branches. - Cons: requires API/schema changes, UI affordances across desktop + mobile, and new persistence semantics. - **Delete only “Mux-created” branches** (e.g. track whether branch existed at creation time) - Pros: safest. - Cons: needs new metadata field + backfill semantics for existing workspaces. Given the explicit desire for “delete means delete” now that archiving exists, the simplest consistent behavior is: delete branch by default, with strong guardrails and clear UI copy. </details> </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `openai:gpt-5.2` • Thinking: `xhigh`_ Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 0c6d24a commit ce3bb39

File tree

7 files changed

+254
-20
lines changed

7 files changed

+254
-20
lines changed

mobile/src/screens/ProjectsScreen.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ export function ProjectsScreen(): JSX.Element {
260260
// Show confirmation dialog
261261
Alert.alert(
262262
"Delete Workspace?",
263-
`This will permanently remove "${metadata.name}" from ${metadata.projectName}.\n\nThis action cannot be undone.`,
263+
`This will permanently remove "${metadata.name}" from ${metadata.projectName} and delete the local branch "${metadata.name}".\n\nThis action cannot be undone.`,
264264
[
265265
{ text: "Cancel", style: "cancel" },
266266
{
@@ -280,7 +280,7 @@ export function ProjectsScreen(): JSX.Element {
280280
// Show force delete option
281281
Alert.alert(
282282
"Workspace Has Changes",
283-
`${errorMsg}\n\nForce delete will discard these changes permanently.`,
283+
`${errorMsg}\n\nForce delete will discard these changes permanently and delete the local branch "${metadata.name}".`,
284284
[
285285
{ text: "Cancel", style: "cancel" },
286286
{

src/browser/components/ArchivedWorkspaces.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,9 @@ export const ArchivedWorkspaces: React.FC<ArchivedWorkspacesProps> = ({
518518
<span className="text-muted text-xs">{selectedIds.size} selected</span>
519519
{bulkDeleteConfirm ? (
520520
<>
521-
<span className="text-muted text-xs">Delete permanently?</span>
521+
<span className="text-muted text-xs">
522+
Delete permanently (also deletes local branches)?
523+
</span>
522524
<button
523525
onClick={() => void handleBulkDelete()}
524526
className="rounded bg-red-600 px-2 py-0.5 text-xs text-white hover:bg-red-700"
@@ -556,7 +558,9 @@ export const ArchivedWorkspaces: React.FC<ArchivedWorkspacesProps> = ({
556558
<Trash2 className="h-4 w-4" />
557559
</button>
558560
</TooltipTrigger>
559-
<TooltipContent>Delete selected permanently</TooltipContent>
561+
<TooltipContent>
562+
Delete selected permanently (local branches too)
563+
</TooltipContent>
560564
</Tooltip>
561565
<button
562566
onClick={() => setSelectedIds(new Set())}
@@ -680,7 +684,7 @@ export const ArchivedWorkspaces: React.FC<ArchivedWorkspacesProps> = ({
680684
<Trash2 className="h-4 w-4" />
681685
</button>
682686
</TooltipTrigger>
683-
<TooltipContent>Delete permanently</TooltipContent>
687+
<TooltipContent>Delete permanently (local branch too)</TooltipContent>
684688
</Tooltip>
685689
</div>
686690
</div>

src/browser/components/ForceDeleteModal.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ export const ForceDeleteModal: React.FC<ForceDeleteModalProps> = ({
7070
<WarningBox>
7171
<WarningTitle>This action cannot be undone</WarningTitle>
7272
<WarningText>
73-
Force deleting will permanently remove the workspace and{" "}
73+
Force deleting will permanently remove the workspace and its local branch, and{" "}
7474
{error.includes("unpushed commits:")
7575
? "discard the unpushed commits shown above"
7676
: "may discard uncommitted work or lose data"}

src/browser/utils/commands/sources.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,13 @@ export function buildCoreSources(p: BuildSourcesParams): Array<() => CommandActi
153153
subtitle: workspaceDisplayName,
154154
section: section.workspaces,
155155
run: async () => {
156-
const ok = confirm("Remove current workspace? This cannot be undone.");
156+
const branchName =
157+
selectedMeta?.name ??
158+
selected.namedWorkspacePath.split("/").pop() ??
159+
selected.namedWorkspacePath;
160+
const ok = confirm(
161+
`Remove current workspace? This will delete the worktree and local branch "${branchName}". This cannot be undone.`
162+
);
157163
if (ok) await p.onRemoveWorkspace(selected.workspaceId);
158164
},
159165
});
@@ -305,7 +311,10 @@ export function buildCoreSources(p: BuildSourcesParams): Array<() => CommandActi
305311
(m) => m.id === vals.workspaceId
306312
);
307313
const workspaceName = meta ? `${meta.projectName}/${meta.name}` : vals.workspaceId;
308-
const ok = confirm(`Remove workspace ${workspaceName}? This cannot be undone.`);
314+
const branchName = meta?.name ?? workspaceName.split("/").pop() ?? workspaceName;
315+
const ok = confirm(
316+
`Remove workspace ${workspaceName}? This will delete the worktree and local branch "${branchName}". This cannot be undone.`
317+
);
309318
if (ok) {
310319
await p.onRemoveWorkspace(vals.workspaceId);
311320
}

src/node/runtime/WorktreeRuntime.test.ts

Lines changed: 128 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ describe("WorktreeRuntime.resolvePath", () => {
9090
});
9191

9292
describe("WorktreeRuntime.deleteWorkspace", () => {
93-
it("deletes agent branches when removing worktrees", async () => {
93+
it("deletes non-agent branches when removing worktrees (force)", async () => {
9494
const rootDir = await fsPromises.realpath(
9595
await fsPromises.mkdtemp(path.join(os.tmpdir(), "worktree-runtime-delete-"))
9696
);
@@ -106,7 +106,7 @@ describe("WorktreeRuntime.deleteWorkspace", () => {
106106
const runtime = new WorktreeRuntime(srcBaseDir);
107107
const initLogger = createNullInitLogger();
108108

109-
const branchName = "agent_explore_aaaaaaaaaa";
109+
const branchName = "feature_aaaaaaaaaa";
110110
const createResult = await runtime.createWorkspace({
111111
projectPath,
112112
branchName,
@@ -116,16 +116,80 @@ describe("WorktreeRuntime.deleteWorkspace", () => {
116116
});
117117
expect(createResult.success).toBe(true);
118118
if (!createResult.success) return;
119+
if (!createResult.workspacePath) {
120+
throw new Error("Expected workspacePath from createWorkspace");
121+
}
122+
const workspacePath = createResult.workspacePath;
119123

120-
const before = execSync(`git branch --list "${branchName}"`, {
124+
// Make the branch unmerged (so -d would fail); force delete should still delete it.
125+
execSync("bash -lc 'echo \"change\" >> README.md'", {
126+
cwd: workspacePath,
127+
stdio: "ignore",
128+
});
129+
execSync("git add README.md", { cwd: workspacePath, stdio: "ignore" });
130+
execSync('git commit -m "change"', { cwd: workspacePath, stdio: "ignore" });
131+
132+
const deleteResult = await runtime.deleteWorkspace(projectPath, branchName, true);
133+
expect(deleteResult.success).toBe(true);
134+
135+
const after = execSync(`git branch --list "${branchName}"`, {
121136
cwd: projectPath,
122137
stdio: ["ignore", "pipe", "ignore"],
123138
})
124139
.toString()
125140
.trim();
126-
expect(before).toContain(branchName);
141+
expect(after).toBe("");
142+
} finally {
143+
await fsPromises.rm(rootDir, { recursive: true, force: true });
144+
}
145+
}, 20_000);
127146

128-
const deleteResult = await runtime.deleteWorkspace(projectPath, branchName, true);
147+
it("deletes merged branches when removing worktrees (safe delete)", async () => {
148+
const rootDir = await fsPromises.realpath(
149+
await fsPromises.mkdtemp(path.join(os.tmpdir(), "worktree-runtime-delete-"))
150+
);
151+
152+
try {
153+
const projectPath = path.join(rootDir, "repo");
154+
await fsPromises.mkdir(projectPath, { recursive: true });
155+
initGitRepo(projectPath);
156+
157+
const srcBaseDir = path.join(rootDir, "src");
158+
await fsPromises.mkdir(srcBaseDir, { recursive: true });
159+
160+
const runtime = new WorktreeRuntime(srcBaseDir);
161+
const initLogger = createNullInitLogger();
162+
163+
const branchName = "feature_merge_aaaaaaaaaa";
164+
const createResult = await runtime.createWorkspace({
165+
projectPath,
166+
branchName,
167+
trunkBranch: "main",
168+
directoryName: branchName,
169+
initLogger,
170+
});
171+
expect(createResult.success).toBe(true);
172+
if (!createResult.success) return;
173+
if (!createResult.workspacePath) {
174+
throw new Error("Expected workspacePath from createWorkspace");
175+
}
176+
const workspacePath = createResult.workspacePath;
177+
178+
// Commit on the workspace branch.
179+
execSync("bash -lc 'echo \"merged-change\" >> README.md'", {
180+
cwd: workspacePath,
181+
stdio: "ignore",
182+
});
183+
execSync("git add README.md", { cwd: workspacePath, stdio: "ignore" });
184+
execSync('git commit -m "merged-change"', {
185+
cwd: workspacePath,
186+
stdio: "ignore",
187+
});
188+
189+
// Merge into main so `git branch -d` succeeds.
190+
execSync(`git merge "${branchName}"`, { cwd: projectPath, stdio: "ignore" });
191+
192+
const deleteResult = await runtime.deleteWorkspace(projectPath, branchName, false);
129193
expect(deleteResult.success).toBe(true);
130194

131195
const after = execSync(`git branch --list "${branchName}"`, {
@@ -139,4 +203,63 @@ describe("WorktreeRuntime.deleteWorkspace", () => {
139203
await fsPromises.rm(rootDir, { recursive: true, force: true });
140204
}
141205
}, 20_000);
206+
207+
it("does not delete protected branches", async () => {
208+
const rootDir = await fsPromises.realpath(
209+
await fsPromises.mkdtemp(path.join(os.tmpdir(), "worktree-runtime-delete-"))
210+
);
211+
212+
try {
213+
const projectPath = path.join(rootDir, "repo");
214+
await fsPromises.mkdir(projectPath, { recursive: true });
215+
initGitRepo(projectPath);
216+
217+
// Move the main worktree off main so we can add a separate worktree on main.
218+
execSync("git checkout -b other", { cwd: projectPath, stdio: "ignore" });
219+
220+
const srcBaseDir = path.join(rootDir, "src");
221+
await fsPromises.mkdir(srcBaseDir, { recursive: true });
222+
223+
const runtime = new WorktreeRuntime(srcBaseDir);
224+
const initLogger = createNullInitLogger();
225+
226+
const branchName = "main";
227+
const createResult = await runtime.createWorkspace({
228+
projectPath,
229+
branchName,
230+
trunkBranch: "main",
231+
directoryName: branchName,
232+
initLogger,
233+
});
234+
expect(createResult.success).toBe(true);
235+
if (!createResult.success) return;
236+
if (!createResult.workspacePath) {
237+
throw new Error("Expected workspacePath from createWorkspace");
238+
}
239+
const workspacePath = createResult.workspacePath;
240+
241+
const deleteResult = await runtime.deleteWorkspace(projectPath, branchName, true);
242+
expect(deleteResult.success).toBe(true);
243+
244+
// The worktree directory should be removed.
245+
let worktreeExists = true;
246+
try {
247+
await fsPromises.access(workspacePath);
248+
} catch {
249+
worktreeExists = false;
250+
}
251+
expect(worktreeExists).toBe(false);
252+
253+
// But protected branches (like main) should never be deleted.
254+
const after = execSync(`git branch --list "${branchName}"`, {
255+
cwd: projectPath,
256+
stdio: ["ignore", "pipe", "ignore"],
257+
})
258+
.toString()
259+
.trim();
260+
expect(after).toBe("main");
261+
} finally {
262+
await fsPromises.rm(rootDir, { recursive: true, force: true });
263+
}
264+
}, 20_000);
142265
});

0 commit comments

Comments
 (0)