diff --git a/packages/app/e2e/regression/settings.spec.ts b/packages/app/e2e/regression/settings.spec.ts index 133c22f45b9b..e118deee5044 100644 --- a/packages/app/e2e/regression/settings.spec.ts +++ b/packages/app/e2e/regression/settings.spec.ts @@ -1,6 +1,7 @@ import { expect, test, type Page } from "@playwright/test" import type { ConfigEntry, OpenCodeEvent, SessionMessageInfo, WorktreeDirectory } from "@opencode/client/promise" import { NO_PROVIDER, REMOTE_SERVER, SERVER, holdRoute, project, seed, session } from "../utils/app" +import { MockWorktreeError } from "../utils/mock-api" import { mockMissingLocation, mockOpenCodeServer } from "../utils/mock-server" import { mockRemoteServer, mockWorkspace, openSession, openSettings, type WorkspaceInput } from "../utils/workspace" @@ -864,6 +865,69 @@ test("worktrees follow inventory events, wait for session counts, and delete by await expect(settings.getByLabel(sandboxes[0]!, { exact: true })).toHaveCount(0) }) +test("bulk delete offers force for worktrees git refuses to remove", async ({ page }) => { + const submodule = `${directory}/submodule-workspace` + const clean = `${directory}/clean-workspace` + + const inventory: WorktreeDirectory[] = [ + { directory }, + { directory: submodule, strategy: "git" }, + { directory: clean, strategy: "git" }, + ] + + const view = await open(page, { + project: { sandboxes: [submodule, clean] }, + sessions: [], + worktrees: () => inventory, + onWorktreeRemoveFailure: (input) => + input.directory === submodule && !input.force + ? new MockWorktreeError({ + name: "WorktreeError", + data: { + message: "fatal: working trees containing submodules cannot be moved or removed", + forceRequired: true, + }, + }) + : undefined, + onWorktreeRemove: (input) => { + inventory.splice( + inventory.findIndex((item) => item.directory === input.directory), + 1, + ) + }, + }) + + const settings = view.settings + await settings.getByRole("tab", { name: "Worktrees", exact: true }).click() + await settings.getByRole("button", { name: "More options", exact: true }).click() + + const deleteWithoutSessions = page.getByRole("menuitem", { + name: "Delete worktrees without sessions", + exact: true, + }) + + await expect(deleteWithoutSessions).toBeVisible() + await deleteWithoutSessions.click() + + const confirmation = page.getByRole("dialog", { name: "Delete worktrees without sessions", exact: true }) + await confirmation.getByRole("button", { name: "Delete worktrees", exact: true }).click() + + const force = page.getByRole("dialog", { name: "Force delete worktrees?", exact: true }) + await expect(force).toContainText("Git refused to remove 1 worktree without force.") + + const forced = page.waitForRequest((request) => { + if (new URL(request.url()).pathname !== "/api/worktree" || request.method() !== "DELETE") return false + + const body = request.postDataJSON() + + return body.directory === submodule && body.force === true + }) + + await force.getByRole("button", { name: "Force delete", exact: true }).click() + await forced + await expect(settings.getByLabel(submodule, { exact: true })).toHaveCount(0) +}) + test.describe("worktrees prefetch", () => { const other = project({ id: "proj_other", directory: "/repo/other", name: "Other project" }) diff --git a/packages/app/e2e/utils/mock-api.ts b/packages/app/e2e/utils/mock-api.ts index 1f236218f81b..832016e8f048 100644 --- a/packages/app/e2e/utils/mock-api.ts +++ b/packages/app/e2e/utils/mock-api.ts @@ -79,6 +79,19 @@ export class MockUnsupported extends Schema.TaggedError()("Mock message: Schema.String, }) {} +// The server's error when git refuses to remove a worktree without force. +export class MockWorktreeError extends Schema.TaggedError()( + "WorktreeError", + { + name: Schema.Literal("WorktreeError"), + data: Schema.Struct({ + message: Schema.String, + forceRequired: Schema.optional(Schema.Boolean), + }), + }, + { httpApiStatus: 400 }, +) {} + const Unsupported = MockUnsupported.pipe(HttpApiSchema.status(501)) const PtyMissing = [MockNotFound.pipe(HttpApiSchema.status(404)), MockPtyNotFound.pipe(HttpApiSchema.status(404))] @@ -197,7 +210,7 @@ const Group = HttpApiGroup.make("mock") HttpApiEndpoint.delete("worktreeRemove", "/api/worktree", { payload: Worktree.RemoveInput, success: NoContent, - error: Unsupported, + error: [Unsupported, MockWorktreeError], }), ) .add( diff --git a/packages/app/e2e/utils/mock-server.ts b/packages/app/e2e/utils/mock-server.ts index 0ccbbcdf5c00..847e8525cf9f 100644 --- a/packages/app/e2e/utils/mock-server.ts +++ b/packages/app/e2e/utils/mock-server.ts @@ -16,6 +16,7 @@ import { MockShellNotFound, MockUnauthorized, MockUnsupported, + MockWorktreeError, } from "./mock-api" import { installSseTransport } from "./sse-transport" @@ -125,6 +126,8 @@ export interface MockServerConfig { // and return the answer; by default it creates `/`. A created directory joins the project's sandboxes. onWorktreeCreate?: (input: Schema.Json) => void | MockAnswer | Promise onWorktreeRemove?: (input: Worktree.RemoveInput) => void | Promise + // Return an error to fail the removal before `onWorktreeRemove` runs. + onWorktreeRemoveFailure?: (input: Worktree.RemoveInput) => MockWorktreeError | undefined // POST /api/session keeps the client-reserved `id` and `location`. Return an answer to fail the attempt (1-based). onSessionCreate?: (body: Schema.JsonObject, attempt: number) => void | MockAnswer // Title of created sessions (default: the request's title, else "New session"). @@ -1021,6 +1024,10 @@ function mockHandlers( if (!remove) return unsupported("remove worktrees", "onWorktreeRemove") + const failure = config.onWorktreeRemoveFailure?.(ctx.payload) + + if (failure) return Effect.fail(failure) + return Effect.promise(async () => remove(ctx.payload)).pipe(Effect.andThen(noContent)) }, // Discovery against a static inventory changes nothing, and the app refreshes whenever worktree settings open. diff --git a/packages/app/src/runtime/i18n/en.ts b/packages/app/src/runtime/i18n/en.ts index 7be03ef2e11e..e87bbbe56b08 100644 --- a/packages/app/src/runtime/i18n/en.ts +++ b/packages/app/src/runtime/i18n/en.ts @@ -1180,6 +1180,14 @@ export const dict = { "settings.workspaces.delete.warning": "This permanently deletes the worktree and its branch.", "settings.workspaces.deleteAll.warning": "Worktrees with unmerged changes, linked sessions, or active locations will be kept.", + "settings.workspaces.deleteForce.title": "Force delete worktrees?", + "settings.workspaces.deleteForce.confirm.one": + "Git refused to remove {{count}} worktree without force. It may contain initialized submodules or files that changed during deletion.", + "settings.workspaces.deleteForce.confirm.other": + "Git refused to remove {{count}} worktrees without force. They may contain initialized submodules or files that changed during deletion.", + "settings.workspaces.deleteForce.warning": + "Force deletion also removes uncommitted changes and unpushed commits inside those worktrees, including their submodules. This cannot be undone.", + "settings.workspaces.deleteForce.button": "Force delete", "settings.workspaces.delete.blocked.active": "The active worktree cannot be deleted.", "settings.workspaces.delete.blocked.linked": "Linked sessions will remain, but their working directory will no longer exist.", diff --git a/packages/app/src/settings/workspaces/workspaces.tsx b/packages/app/src/settings/workspaces/workspaces.tsx index ca52263a2cd2..2d9be098a402 100644 --- a/packages/app/src/settings/workspaces/workspaces.tsx +++ b/packages/app/src/settings/workspaces/workspaces.tsx @@ -3,7 +3,7 @@ import { For, Show, createEffect, createMemo, createSignal } from "solid-js" import { createStore } from "solid-js/store" import { createResizeObserver } from "@solid-primitives/resize-observer" import { Key } from "@solid-primitives/keyed" -import type { SessionInfo } from "@opencode/client/promise" +import { isWorktreeError, type SessionInfo } from "@opencode/client/promise" import { useQuery, useQueryClient } from "@tanstack/solid-query" import { Button } from "@opencode/ui/button" import { Dialog, DialogFooter, DialogHeader, DialogTitleGroup } from "@opencode/ui/dialog" @@ -45,6 +45,8 @@ type Workspace = { project: Project } +type RemoveResult = "removed" | "blocked" | "forceRequired" | "failed" + export const SettingsWorkspaces: Component<{ activeDirectory?: string resetProjectFilter?: () => number @@ -246,7 +248,11 @@ export const SettingsWorkspaces: Component<{ }) } - const remove = async (workspace: Workspace, force = false, context = captureDeleteContext()) => { + const remove = async ( + workspace: Workspace, + force = false, + context = captureDeleteContext(), + ): Promise => { const key = String(pathKey(workspace.directory)) setStore("deleting", (items) => [...items, key]) @@ -256,7 +262,7 @@ export const SettingsWorkspaces: Component<{ if (!force && (preflight.result.active || preflight.result.linked || preflight.result.dirty)) { blocked(preflight.result) - return + return "blocked" } const removed = await context.sdk.api.worktree @@ -265,18 +271,20 @@ export const SettingsWorkspaces: Component<{ directory: workspace.directory, force, }) - .then(() => true) + .then(() => "removed" as const) .catch((error) => { + if (!force && isWorktreeError(error) && error.data.forceRequired) return "forceRequired" as const + showToast({ variant: "error", title: language.t("workspace.delete.failed.title"), description: error instanceof Error ? error.message : language.t("common.requestFailed"), }) - return false + return "failed" as const }) - if (!removed) return + if (removed !== "removed") return removed setStore("removing", (items) => [...items, key]) await new Promise((resolve) => setTimeout(resolve, 150)) tabs.store.forEach((tab) => { @@ -295,6 +303,8 @@ export const SettingsWorkspaces: Component<{ queryKey: worktreeInventoryKey(context.sdk.scope, workspace.project.id), }) await queryClient.invalidateQueries({ queryKey: [context.sdk.scope, "settings-workspace-inventory"] }) + + return "removed" } finally { setStore("deleting", (items) => items.filter((item) => item !== key)) setStore("removing", (items) => items.filter((item) => item !== key)) @@ -307,18 +317,20 @@ export const SettingsWorkspaces: Component<{ if (store.transaction === "confirm") setStore("transaction", undefined) } - const transact = async (task: () => Promise) => { - if (store.transaction !== "confirm") return + const transact = async (task: () => Promise): Promise => { + if (store.transaction !== "confirm") return undefined setStore("transaction", "running") try { - await task() + return await task() } catch (error) { showToast({ variant: "error", title: language.t("workspace.delete.failed.title"), description: error instanceof Error ? error.message : language.t("common.requestFailed"), }) + + return undefined } finally { setStore("transaction", undefined) } @@ -337,7 +349,7 @@ export const SettingsWorkspaces: Component<{ inspectionID={current} inspect={() => inspect(workspace, context)} inspectionMessages={inspectionMessages} - onDelete={() => transact(() => remove(workspace, true, context))} + onDelete={() => transact(() => remove(workspace, true, context)).then(() => undefined)} /> ), releaseConfirmation, @@ -345,7 +357,51 @@ export const SettingsWorkspaces: Component<{ } const removeAll = async (inventory: Workspace[], context: ReturnType) => { - await removeWorkspacesSequentially(inventory, (workspace) => remove(workspace, false, context)) + const forceRequired: Workspace[] = [] + await removeWorkspacesSequentially(inventory, async (workspace) => { + if ((await remove(workspace, false, context)) === "forceRequired") forceRequired.push(workspace) + }) + + return forceRequired + } + + const forceDelete = async (inventory: Workspace[], context: ReturnType) => { + if (store.transaction) return + setStore("transaction", "running") + + try { + await removeWorkspacesSequentially(inventory, async (workspace) => { + await remove(workspace, true, context) + }) + } catch (error) { + showToast({ + variant: "error", + title: language.t("workspace.delete.failed.title"), + description: error instanceof Error ? error.message : language.t("common.requestFailed"), + }) + } finally { + setStore("transaction", undefined) + } + } + + const confirmForceDelete = (inventory: Workspace[], context: ReturnType) => { + if (store.transaction) return + void dialog.push(() => ( + forceDelete(inventory, context)} + /> + )) + } + + const removeAllConfirmed = async (inventory: Workspace[], context: ReturnType) => { + const forceRequired = await transact(() => removeAll(inventory, context)) + + if (!forceRequired?.length) return + confirmForceDelete(forceRequired, context) } const confirmDeleteAll = () => { @@ -359,7 +415,7 @@ export const SettingsWorkspaces: Component<{ title={language.t("settings.workspaces.deleteAll")} confirmation={language.plural("settings.workspaces.deleteAll.confirm", inventory.length)} warning={language.t("settings.workspaces.deleteAll.warning")} - onDelete={() => transact(() => removeAll(inventory, context))} + onDelete={() => removeAllConfirmed(inventory, context)} /> ), releaseConfirmation, @@ -377,7 +433,7 @@ export const SettingsWorkspaces: Component<{ title={language.t("settings.workspaces.deleteWithoutSessions")} confirmation={language.plural("settings.workspaces.deleteWithoutSessions.confirm", inventory.length)} warning={language.t("settings.workspaces.deleteWithoutSessions.warning")} - onDelete={() => transact(() => removeAll(inventory, context))} + onDelete={() => removeAllConfirmed(inventory, context)} /> ), releaseConfirmation, @@ -574,6 +630,7 @@ function DialogDeleteWorkspaces(props: { title: string confirmation: string warning: string + confirmLabel?: string onDelete: () => Promise }) { const dialog = useDialog() @@ -603,7 +660,7 @@ function DialogDeleteWorkspaces(props: { {language.t("common.cancel")} diff --git a/packages/core/src/git.ts b/packages/core/src/git.ts index 1778879facea..f116f3f7aae9 100644 --- a/packages/core/src/git.ts +++ b/packages/core/src/git.ts @@ -1417,7 +1417,15 @@ const layer = Layer.effect( cwd = repository.worktree, ) { const result = yield* proc - .run(ChildProcess.make(gitExecutable(), args, { cwd, extendEnv: true, stdin: "ignore" })) + .run( + ChildProcess.make(gitExecutable(), args, { + cwd, + // Git localizes diagnostics, and `git worktree` has no machine-readable output. + env: { LC_ALL: "C" }, + extendEnv: true, + stdin: "ignore", + }), + ) .pipe( Effect.mapError( (cause) => new WorktreeError({ operation, directory: worktreeDirectory, message: cause.message, cause }), @@ -1429,7 +1437,9 @@ const layer = Layer.effect( operation, directory: worktreeDirectory, message, - forceRequired: operation === "remove" && /contains modified or untracked files|is dirty/i.test(message), + forceRequired: + operation === "remove" && + /contains modified or untracked files|is dirty|containing submodules/i.test(message), }) }) diff --git a/packages/core/src/tool/plugin/opencode.ts b/packages/core/src/tool/plugin/opencode.ts index d18bb45e41d1..85add5199df0 100644 --- a/packages/core/src/tool/plugin/opencode.ts +++ b/packages/core/src/tool/plugin/opencode.ts @@ -301,6 +301,6 @@ function removeFailure(directory: string, error: unknown) { if (error instanceof Worktree.StrategyUnavailableError) return `Unable to remove worktree ${directory}: its worktree strategy ${error.strategy} is not available.` if ((error instanceof Git.WorktreeError || error instanceof Worktree.OperationError) && error.forceRequired) - return `Unable to remove worktree ${directory}: it has modified or untracked files.` + return `Unable to remove worktree ${directory}: it has modified or untracked files, or initialized submodules. Retry with force: true to delete it anyway.` return `Unable to remove worktree ${directory}.` } diff --git a/packages/core/test/git.test.ts b/packages/core/test/git.test.ts index d040949fce58..6bd315ffd8f7 100644 --- a/packages/core/test/git.test.ts +++ b/packages/core/test/git.test.ts @@ -105,6 +105,52 @@ describe("Git worktrees", () => { }), ) + it.live("requires force to remove a worktree with initialized submodules", () => + Effect.gen(function* () { + const root = yield* Effect.acquireRelease( + Effect.promise(() => tmpdir()), + (dir) => Effect.promise(() => dir[Symbol.asyncDispose]()), + ) + const submodule = `${root.path}-submodule` + yield* Effect.addFinalizer(() => + Effect.promise(() => fs.rm(submodule, { recursive: true, force: true })).pipe(Effect.ignore), + ) + yield* Effect.promise(async () => { + await initRepo(submodule) + await initRepo(root.path) + await $`git -c protocol.file.allow=always submodule add ${submodule} modules/submodule`.cwd(root.path).quiet() + await $`git add .`.cwd(root.path).quiet() + await $`git commit -m "add submodule"`.cwd(root.path).quiet() + }) + const directory = AbsolutePath.make(yield* Effect.promise(() => fs.realpath(root.path))) + const worktree = AbsolutePath.make(`${root.path}-git-worktree-submodule`) + yield* Effect.addFinalizer(() => + Effect.promise(() => fs.rm(worktree, { recursive: true, force: true })).pipe(Effect.ignore), + ) + const git = yield* Git.Service + const repo = yield* git.repo.discover(directory) + if (!repo) throw new Error("Repository not found") + + yield* git.worktree.create({ repository: repo, directory: worktree }) + yield* Effect.promise(() => + $`git -c protocol.file.allow=always submodule update --init`.cwd(worktree).quiet(), + ) + + const error = yield* git.worktree + .remove({ repository: repo, directory: worktree, force: false }) + .pipe(Effect.flip) + + expect(error).toBeInstanceOf(Git.WorktreeError) + if (!(error instanceof Git.WorktreeError)) throw new Error("Expected a worktree error") + expect(error.operation).toBe("remove") + expect(error.forceRequired).toBe(true) + expect(yield* Effect.promise(() => Bun.file(path.join(worktree, ".git")).exists())).toBe(true) + + yield* git.worktree.remove({ repository: repo, directory: worktree, force: true }) + expect(yield* Effect.promise(() => fs.stat(worktree).then(() => true, () => false))).toBe(false) + }), + ) + it.live("reads no remote and no root commits from an unborn repository", () => Effect.gen(function* () { const root = yield* Effect.acquireRelease( diff --git a/packages/tui/src/component/dialog-workspaces.tsx b/packages/tui/src/component/dialog-workspaces.tsx index 70aaa8ff4bf2..df9d1720579e 100644 --- a/packages/tui/src/component/dialog-workspaces.tsx +++ b/packages/tui/src/component/dialog-workspaces.tsx @@ -259,7 +259,8 @@ export function DialogWorkspaces(props: DialogWorkspacesProps) { .catch(() => undefined) const choice = await DialogWorkspaceFileChanges.show(dialog, status?.data ?? [], { title: "Delete worktree?", - message: "This worktree has file changes. Do you want to delete it anyway?", + message: + "This worktree needs force to delete. Force also discards uncommitted changes and unpushed commits in its submodules. Delete it anyway?", confirm: { title: "delete", key: "d" }, }) if (choice !== "yes") { diff --git a/services/www/src/docs/content/tools.mdx b/services/www/src/docs/content/tools.mdx index 88e29cd319fa..96e79a048283 100644 --- a/services/www/src/docs/content/tools.mdx +++ b/services/www/src/docs/content/tools.mdx @@ -264,7 +264,8 @@ The same namespace manages worktrees for the session's project: strategy. - `worktree_list` lists the project's worktrees. - `worktree_remove` removes a worktree OpenCode recorded for the project. Set - `force` to discard uncommitted changes. + `force` to discard uncommitted changes or remove a worktree with initialized + submodules. ```text Create a worktree named "login" and move this session into it.