fix(sidebar): clear the delete state when a removal is refused (#15187)

* fix(sidebar): clear the delete state when a removal is refused

Callers mark rows deleting up front for immediate sidebar feedback, but a
refusal in beginHostQualifiedRemoval returns before removeWorktree's
try/catch — the only other place that clears the flag. The failure toast
auto-dismisses after 10s, so the workspace was left on a 'Deleting…'
spinner indefinitely with no explanation still on screen.

Also removes workspace-cleanup-removal-host-guard.ts: #14731 refused a
colliding cleanup removal, #15013 replaced refusing with routing to the
confirmed host, and nothing imports the module any more — not even its
own tests.

* test(sidebar): pin the two states that actually reach the removal refusal

The existing case used an empty store, which proved the clear but not that a
user can land there. Cover a stale folder-workspace id and a hostless row under
an ambiguous legacy runtime — both resolve to no route from populated state.

---------

Co-authored-by: QA <qa@local>
This commit is contained in:
Brennan Benson
2026-08-18 14:08:25 -07:00
committed by GitHub
co-authored by QA
parent 12550fcc28
commit f71ef7ee03
3 changed files with 117 additions and 43 deletions
@@ -1,43 +0,0 @@
/**
* STA-4343 fail-closed guard: cleanup must never issue a destructive removal
* against a host other than the one whose row the user confirmed.
*
* A cleanup row's `worktreeId` is `repoId::path`, which two execution hosts can
* both own, while selection, confirmation and preflight all key on `worktreeId`
* alone and removal routing prefers the ACTIVE workspace's host. This is the
* minimal safety property: refuse whenever the confirmed owner is unknown, is
* not the only owner the refreshed scan reports, or is not where the removal
* would actually land. Routing a colliding row to its right host is #14606.
*/
import type { ExecutionHostId } from '../../../../shared/execution-host'
import { resolveWorkspaceCleanupRemovalHostId } from '../../../../shared/workspace-cleanup-host-identity'
import type { WorkspaceCleanupCandidate } from '../../../../shared/workspace-cleanup'
/** True only when the removal is provably about to land on the confirmed row's own host. */
export function isWorkspaceCleanupRemovalHostCertain(args: {
/** The row the user confirmed; internal callers without one fall back to the scanned row. */
confirmedCandidate: WorkspaceCleanupCandidate | undefined
scannedCandidate: WorkspaceCleanupCandidate
/** Every owner the refreshed scan reported for this `worktreeId`, null where a row carries none. */
scannedHostIds: readonly (ExecutionHostId | null)[]
/**
* The host `removeWorktree` would route the destructive IPC to. Null is not a
* refusal here: `removeWorktree` already fails an unroutable removal closed
* with WORKTREE_REMOVAL_AMBIGUOUS_ERROR before it touches any transport.
*/
routeHostId: ExecutionHostId | null
}): boolean {
const confirmedHostId = resolveWorkspaceCleanupRemovalHostId(
args.confirmedCandidate ?? args.scannedCandidate
)
if (!confirmedHostId) {
return false
}
// Why: one id owned by two hosts cannot say which row was confirmed, and a
// confirmed owner the refreshed scan no longer reports cannot be verified either.
const scannedHostIds = new Set(args.scannedHostIds)
if (scannedHostIds.size !== 1 || !scannedHostIds.has(confirmedHostId)) {
return false
}
return args.routeHostId === null || args.routeHostId === confirmedHostId
}
@@ -0,0 +1,110 @@
import { describe, expect, it, vi } from 'vitest'
import { beginHostQualifiedRemoval } from './host-qualified-worktree-removal'
import { folderWorkspaceKey } from '../../../../../../shared/workspace-scope'
const WORKTREE_ID = 'repo1::/shared/workspace/path'
/**
* Callers mark rows deleting up front for immediate sidebar feedback
* (worktree-delete-execution.ts). These refusals return BEFORE removeWorktree's
* try/catch, which is the only other place that clears the flag — so without an
* explicit clear the workspace keeps its "Deleting…" spinner indefinitely, long
* after the 10s failure toast has gone.
*/
// Note: with a confirmed host the route resolves from the host id alone, so this refusal is
// reached on the UNQUALIFIED path — a caller that names no host and has no resolvable owner.
describe('beginHostQualifiedRemoval refusals clear the delete state', () => {
function makeGet(clearWorktreeDeleteState: ReturnType<typeof vi.fn>) {
// Minimal store surface: no worktrees, so no route can resolve for MISSING_HOST.
return () =>
({
clearWorktreeDeleteState,
allWorktrees: () => [],
worktreesByRepo: {},
repos: [],
detectedWorktreesByRepo: {},
settings: {},
sshConnectionStates: new Map(),
sshTargetLabels: new Map(),
workspaceCleanupScan: null
}) as never
}
it('clears when no route resolves and no host was confirmed', () => {
const clearWorktreeDeleteState = vi.fn()
const start = beginHostQualifiedRemoval(
makeGet(clearWorktreeDeleteState),
WORKTREE_ID,
null,
false
)
expect(start.ok).toBe(false)
expect(clearWorktreeDeleteState).toHaveBeenCalledWith(WORKTREE_ID)
})
// The store above is deliberately empty, which proves the clear but not that a real user can
// land here. These two cases route through populated state a user actually has, and still
// resolve to no route — the states that leave the row spinning in the product.
function makeRoutedGet(
clearWorktreeDeleteState: ReturnType<typeof vi.fn>,
overrides: Record<string, unknown>
) {
return () =>
({
clearWorktreeDeleteState,
allWorktrees: () => [],
worktreesByRepo: {},
repos: [],
detectedWorktreesByRepo: {},
settings: {},
runtimeEnvironments: [],
folderWorkspaces: [],
projectGroups: [],
sshConnectionStates: new Map(),
sshTargetLabels: new Map(),
workspaceCleanupScan: null,
...overrides
}) as never
}
// A hostless row (folder-workspace meta never sets hostId, and runtime rows omit the field
// when the repo is unresolved) plus more than one saved runtime environment trips the legacy
// single-runtime gate in resolveWorktreeOperationRouteResult, which returns `missing`.
it('clears when a known worktree has no host and the legacy runtime is ambiguous', () => {
const clearWorktreeDeleteState = vi.fn()
const start = beginHostQualifiedRemoval(
makeRoutedGet(clearWorktreeDeleteState, {
repos: [{ id: 'repo1', connectionId: null, executionHostId: undefined }],
worktreesByRepo: { repo1: [{ id: WORKTREE_ID, repoId: 'repo1' }] },
settings: { activeRuntimeEnvironmentId: 'env-a' },
runtimeEnvironments: [{ id: 'env-a' }, { id: 'env-b' }]
}),
WORKTREE_ID,
null,
false
)
expect(start.ok).toBe(false)
expect(clearWorktreeDeleteState).toHaveBeenCalledWith(WORKTREE_ID)
})
// Folder workspaces fail closed on a stale id by design, so a row whose folder record is gone
// refuses instead of routing.
it('clears when a folder workspace id no longer has an owner', () => {
const clearWorktreeDeleteState = vi.fn()
const folderId = folderWorkspaceKey('fw-removed')
const start = beginHostQualifiedRemoval(
makeRoutedGet(clearWorktreeDeleteState, {
repos: [{ id: 'repo1', connectionId: null, executionHostId: 'local' }],
folderWorkspaces: []
}),
folderId,
null,
false
)
expect(start.ok).toBe(false)
expect(clearWorktreeDeleteState).toHaveBeenCalledWith(folderId)
})
})
@@ -78,6 +78,12 @@ export function beginHostQualifiedRemoval(
resolveHostQualifiedRemovalRoute(get, worktreeId, requiredExecutionHostId)
const removalRoute = resolveRemovalRoute()
if (!removalRoute && (!forgetLocalOnly || !requiredExecutionHostId)) {
// Why: callers mark rows deleting up front for immediate sidebar feedback
// (worktree-delete-execution.ts), and a refusal returns before the try/catch that
// would otherwise clear it. The failure toast auto-dismisses after 10s, so without
// this the workspace sits on a "Deleting…" spinner forever with no explanation left
// on screen.
get().clearWorktreeDeleteState(worktreeId)
return { ok: false, error: WORKTREE_REMOVAL_AMBIGUOUS_ERROR }
}
// Fail closed rather than delete on a host the caller never confirmed.
@@ -86,6 +92,7 @@ export function beginHostQualifiedRemoval(
removalRoute &&
removalRoute.executionHostId !== requiredExecutionHostId
) {
get().clearWorktreeDeleteState(worktreeId)
return { ok: false, error: WORKTREE_REMOVAL_HOST_CHANGED_ERROR }
}
const sameIdSurvivingHostId = resolveSameIdSurvivingHostId(