From 4565b944e4a539f28230c1c7fdf211dca6cacbc3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 18:52:26 -0700 Subject: [PATCH] fix(space): route the ordinary Space delete to the row's host (STA-4343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkspaceSpaceManagerPanel's force path already carried executionHostId; the ordinary Delete one line above it passed a bare worktree id, as did deleteSelected. The Space scan lists one row per host, so clicking Delete on the SSH row resolved through the id-keyed lookup and destroyed the LOCAL checkout — the exact defect this PR exists to fix, still reachable from a second surface. A Space row carries no instanceId, so a hand-built identity would be rejected as stale by the confirmed-target check and turn a wrong-host delete into a total refusal. The row is resolved on its own host first and the STORE row's identity is what travels. Selection keeps the rows, not just their ids, so a multi-select spanning two hosts no longer collapses to one. The delete-selection helpers move to their own module to stay under the 300-line cap (no max-lines disable). Also tightens the cleanup batch ambiguity gate: it keyed on the display identity, which defaults a hostless row to `local`, so a hostless row and a genuine local row for the same id shared an identity and the gate could not fire. It now keys on the strict resolver, where an unqualified row is its own bucket. --- .../status-bar/WorkspaceSpaceManagerPanel.tsx | 49 ++++++-- ...orkspace-space-delete-host-routing.test.ts | 117 ++++++++++++++++++ .../workspace-space-delete-selection.ts | 44 +++++++ .../workspace-space-presentation.test.ts | 6 +- .../workspace-space-presentation.ts | 25 ---- .../workspace-cleanup-removal-targets.ts | 14 ++- 6 files changed, 217 insertions(+), 38 deletions(-) create mode 100644 src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts create mode 100644 src/renderer/src/components/status-bar/workspace-space-delete-selection.ts diff --git a/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx b/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx index aa7a8526829..7831c6c48e7 100644 --- a/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx +++ b/src/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsx @@ -46,7 +46,14 @@ import { getRepoMapFromState, getWorktreeMapFromState } from '../../store/select import { getHostedReviewCacheKey } from '../../store/slices/hosted-review' import { issueCacheKey as getIssueCacheKey } from '../../store/slices/github' import { refreshGitStatusForWorktree } from '../right-sidebar/git-status-refresh' +import { + getSelectedDeletableWorkspaceRows, + getVisibleDeletableWorkspaceIds +} from './workspace-space-delete-selection' import { runWorktreeBatchDelete } from '../sidebar/delete-worktree-flow' +import { toWorktreeDeleteIdentities } from '../sidebar/worktree-delete-request' +import { showWorkspaceListChangedToast } from '../sidebar/stale-workspace-list-toast' +import { getWorktreeOnHostFromState } from '../../store/selectors' import { prepareActiveWorktreeFocusAfterDelete } from '../sidebar/active-worktree-focus-after-delete' import { branchDisplayName } from '../sidebar/WorktreeCardHelpers' import { Badge } from '../ui/badge' @@ -75,8 +82,6 @@ import { countWorkspaceSpaceActiveAgents, getLargestWorkspaceSpaceItemSize, getLargestWorkspaceSpaceRowSize, - getSelectedDeletableWorkspaceIds, - getVisibleDeletableWorkspaceIds, getWorkspaceSpaceGitStatusRefreshCandidates, isWorkspaceSpaceRowReadyToDelete, pruneWorkspaceSpaceSelectedIds, @@ -1491,10 +1496,14 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { sourceRows.find((row) => row.worktreeId === nextTreemapZoomWorktreeId && row.status === 'ok') ?? null const maxSize = getLargestWorkspaceSpaceRowSize(rows) - const selectedDeletableIds = useMemo( - () => getSelectedDeletableWorkspaceIds(rows, nextSelectedIds, isWorktreeUnavailableForDelete), + const selectedDeletableRows = useMemo( + () => getSelectedDeletableWorkspaceRows(rows, nextSelectedIds, isWorktreeUnavailableForDelete), [isWorktreeUnavailableForDelete, nextSelectedIds, rows] ) + const selectedDeletableIds = useMemo( + () => selectedDeletableRows.map((row) => row.worktreeId), + [selectedDeletableRows] + ) const selectedDeletableIdSet = useMemo( () => new Set(selectedDeletableIds), [selectedDeletableIds] @@ -1606,11 +1615,31 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { ) const deleteWorktrees = useCallback( - (worktreeIds: readonly string[]): void => { - if (worktreeIds.length === 0) { + (targets: readonly WorkspaceSpaceWorktree[]): void => { + if (targets.length === 0) { return } - runWorktreeBatchDelete(worktreeIds, { + // Why (STA-4343): the Space scan lists one row per host, so a bare id would + // route the delete at whichever host the id-keyed lookup happens to hold. + // Resolve each row on ITS host and hand over the store row's own identity — + // a Space row carries no instanceId, so a hand-built one would be rejected + // as stale by the confirmed-target check. + const state = useAppStore.getState() + const identities = toWorktreeDeleteIdentities( + targets.flatMap((target) => { + const row = getWorktreeOnHostFromState( + state, + target.worktreeId, + target.executionHostId ?? undefined + ) + return row ? [row] : [] + }) + ) + if (identities.length !== targets.length) { + showWorkspaceListChangedToast() + return + } + runWorktreeBatchDelete(identities, { forceConfirm: true, onDeleted: handleDeletedWorktrees }) @@ -1663,10 +1692,10 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { ) const deleteSelected = (): void => { - if (selectedDeletableIds.length === 0) { + if (selectedDeletableRows.length === 0) { return } - deleteWorktrees(selectedDeletableIds) + deleteWorktrees(selectedDeletableRows) } return ( @@ -2082,7 +2111,7 @@ export function WorkspaceSpaceManagerPanel(): React.JSX.Element { onToggleSelected={() => toggleSelection(worktree.worktreeId)} onInspect={() => setInspectedWorktreeId(worktree.worktreeId)} onOpenWorkspace={() => activateAndRevealWorktree(worktree.worktreeId)} - onDelete={() => deleteWorktrees([worktree.worktreeId])} + onDelete={() => deleteWorktrees([worktree])} onForceDelete={() => forceDeleteWorktree(worktree)} /> )) diff --git a/src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts b/src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts new file mode 100644 index 00000000000..a3cd1f46865 --- /dev/null +++ b/src/renderer/src/components/status-bar/workspace-space-delete-host-routing.test.ts @@ -0,0 +1,117 @@ +/** + * STA-4343: the Space Manager's ORDINARY delete must route to the row's host. + * + * The Space scan lists one row per execution host, so two rows can share a + * `repoId::path` worktree id. The force-delete path carried `executionHostId`; + * the ordinary Delete one line above it passed a bare id, which resolves through + * the id-keyed lookup and lands on whichever host happens to be first. Clicking + * Delete on the SSH row therefore deleted the LOCAL checkout. + * + * A Space row carries no `instanceId` (see WorkspaceSpaceWorktree), so the fix + * cannot hand-build an identity — it has to resolve the store row on that host + * and pass the store row's own identity, or the confirmed-target check rejects + * every Space delete as stale. + */ +import { describe, expect, it } from 'vitest' +import type { ExecutionHostId } from '../../../../shared/execution-host' +import type { WorkspaceSpaceWorktree } from '../../../../shared/workspace-space-types' +import type { Worktree } from '../../../../shared/worktree/types' +import { getSelectedDeletableWorkspaceRows } from './workspace-space-delete-selection' +import { + resolveWorktreeBatchDeleteTargets, + toWorktreeDeleteIdentities +} from '../sidebar/worktree-delete-request' + +const SHARED_ID = 'repo-1::/work/orca' +const LOCAL: ExecutionHostId = 'local' +const SSH: ExecutionHostId = 'ssh:build-box' + +function spaceRow(executionHostId: ExecutionHostId): WorkspaceSpaceWorktree { + return { + worktreeId: SHARED_ID, + executionHostId, + displayName: 'orca', + path: '/work/orca', + status: 'ok', + canDelete: true, + sizeBytes: 1 + } as WorkspaceSpaceWorktree +} + +function storeRow(hostId: ExecutionHostId): Worktree { + return { + id: SHARED_ID, + // A Space row has no instanceId; the store row does, and that is the one + // the confirmed-target check compares against. + instanceId: `instance-${hostId}`, + repoId: 'repo-1', + path: '/work/orca', + isMainWorktree: false, + hostId + } as Worktree +} + +const localStoreRow = storeRow(LOCAL) +const sshStoreRow = storeRow(SSH) + +/** Stands in for getWorktreeOnHostFromState: resolve on the named host. */ +function lookupOnHost(worktreeId: string, hostId: ExecutionHostId | undefined) { + const matches = [localStoreRow, sshStoreRow].filter((row) => row.id === worktreeId) + return hostId ? matches.find((row) => row.hostId === hostId) : matches[0] +} + +/** The panel's delete path: Space row -> store row on ITS host -> identity. */ +function identitiesForSpaceDelete(targets: readonly WorkspaceSpaceWorktree[]) { + return toWorktreeDeleteIdentities( + targets.flatMap((target) => { + const row = lookupOnHost(target.worktreeId, target.executionHostId ?? undefined) + return row ? [row] : [] + }) + ) +} + +describe('Space Manager delete routes to the row host', () => { + it('deletes the SSH row when the SSH row is the one clicked', () => { + const targets = resolveWorktreeBatchDeleteTargets( + identitiesForSpaceDelete([spaceRow(SSH)]), + lookupOnHost + ) + + // The local row is listed first, so a bare-id delete would resolve to it. + expect(targets?.map((row) => row.hostId)).toEqual([SSH]) + }) + + it('deletes the local row when the local row is the one clicked', () => { + const targets = resolveWorktreeBatchDeleteTargets( + identitiesForSpaceDelete([spaceRow(LOCAL)]), + lookupOnHost + ) + + expect(targets?.map((row) => row.hostId)).toEqual([LOCAL]) + }) + + it('carries the store row instanceId so the confirmed-target check accepts it', () => { + // Regression guard: hand-building {id, hostId} with no instanceId would make + // resolveWorktreeBatchDeleteTargets refuse every Space delete instead. + const identities = identitiesForSpaceDelete([spaceRow(SSH)]) + + expect(identities).toEqual([{ id: SHARED_ID, instanceId: `instance-${SSH}`, hostId: SSH }]) + expect(resolveWorktreeBatchDeleteTargets(identities, lookupOnHost)).not.toBeNull() + }) + + it('keeps each selected row on its own host in a multi-select delete', () => { + const selected = getSelectedDeletableWorkspaceRows( + [spaceRow(LOCAL), spaceRow(SSH)], + new Set([SHARED_ID]) + ) + // Both rows share the id, so a selection keyed by id alone would collapse them. + expect(selected).toHaveLength(2) + + const targets = resolveWorktreeBatchDeleteTargets( + identitiesForSpaceDelete(selected), + lookupOnHost + ) + + expect(targets?.map((row) => row.hostId)).toEqual([LOCAL, SSH]) + }) +}) diff --git a/src/renderer/src/components/status-bar/workspace-space-delete-selection.ts b/src/renderer/src/components/status-bar/workspace-space-delete-selection.ts new file mode 100644 index 00000000000..3ba84aebc5b --- /dev/null +++ b/src/renderer/src/components/status-bar/workspace-space-delete-selection.ts @@ -0,0 +1,44 @@ +import type { WorkspaceSpaceWorktree } from '../../../../shared/workspace-space-types' + +/** + * Which Space rows a delete action applies to. + * + * Rows are host-partitioned: two rows can share a `repoId::path` worktree id + * when the same repo is registered on two execution hosts. Anything feeding a + * destructive action therefore hands back ROWS, not ids — an id alone cannot say + * which host's checkout the user pointed at (STA-4343). + */ + +/** The selected rows themselves — each keeps its own host (STA-4343). */ +export function getSelectedDeletableWorkspaceRows( + rows: readonly WorkspaceSpaceWorktree[], + selectedIds: ReadonlySet, + isWorktreeDeleting: (worktreeId: string) => boolean = () => false +): WorkspaceSpaceWorktree[] { + return rows.filter( + (row) => + row.canDelete && + row.status === 'ok' && + selectedIds.has(row.worktreeId) && + !isWorktreeDeleting(row.worktreeId) + ) +} + +export function getSelectedDeletableWorkspaceIds( + rows: readonly WorkspaceSpaceWorktree[], + selectedIds: ReadonlySet, + isWorktreeDeleting: (worktreeId: string) => boolean = () => false +): string[] { + return getSelectedDeletableWorkspaceRows(rows, selectedIds, isWorktreeDeleting).map( + (row) => row.worktreeId + ) +} + +export function getVisibleDeletableWorkspaceIds( + rows: readonly WorkspaceSpaceWorktree[], + isWorktreeDeleting: (worktreeId: string) => boolean = () => false +): string[] { + return rows + .filter((row) => row.canDelete && row.status === 'ok' && !isWorktreeDeleting(row.worktreeId)) + .map((row) => row.worktreeId) +} diff --git a/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts b/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts index 1d600faed2a..e643749ff37 100644 --- a/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts +++ b/src/renderer/src/components/status-bar/workspace-space-presentation.test.ts @@ -1,3 +1,7 @@ +import { + getSelectedDeletableWorkspaceIds, + getVisibleDeletableWorkspaceIds +} from './workspace-space-delete-selection' import { describe, expect, it } from 'vitest' import type { WorkspaceSpaceWorktree } from '../../../../shared/workspace-space-types' import { @@ -6,8 +10,6 @@ import { filterWorkspaceSpaceRows, getLargestWorkspaceSpaceItemSize, getLargestWorkspaceSpaceRowSize, - getSelectedDeletableWorkspaceIds, - getVisibleDeletableWorkspaceIds, getWorkspaceSpaceGitStatusRefreshCandidates, isWorkspaceSpaceFilterQueryTooLarge, isWorkspaceSpaceRowReadyToDelete, diff --git a/src/renderer/src/components/status-bar/workspace-space-presentation.ts b/src/renderer/src/components/status-bar/workspace-space-presentation.ts index 5aab2dcefc5..2b6d5847569 100644 --- a/src/renderer/src/components/status-bar/workspace-space-presentation.ts +++ b/src/renderer/src/components/status-bar/workspace-space-presentation.ts @@ -262,31 +262,6 @@ export function getWorkspaceSpaceGitStatusRefreshCandidates( ) } -export function getSelectedDeletableWorkspaceIds( - rows: readonly WorkspaceSpaceWorktree[], - selectedIds: ReadonlySet, - isWorktreeDeleting: (worktreeId: string) => boolean = () => false -): string[] { - return rows - .filter( - (row) => - row.canDelete && - row.status === 'ok' && - selectedIds.has(row.worktreeId) && - !isWorktreeDeleting(row.worktreeId) - ) - .map((row) => row.worktreeId) -} - -export function getVisibleDeletableWorkspaceIds( - rows: readonly WorkspaceSpaceWorktree[], - isWorktreeDeleting: (worktreeId: string) => boolean = () => false -): string[] { - return rows - .filter((row) => row.canDelete && row.status === 'ok' && !isWorktreeDeleting(row.worktreeId)) - .map((row) => row.worktreeId) -} - export function resolveWorkspaceSpaceInspectedWorktreeId( rows: readonly WorkspaceSpaceWorktree[], currentWorktreeId: string | null diff --git a/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts b/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts index 726341eb65e..c04d5c737f9 100644 --- a/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts +++ b/src/renderer/src/store/slices/workspace-cleanup-removal-targets.ts @@ -26,6 +26,9 @@ import { getWorktreeOperationOwnerHostIds } from '@/lib/worktree-operation-route import { translate } from '@/i18n/i18n' import type { WorkspaceCleanupFailure } from './workspace-cleanup' +/** Distinct from every ExecutionHostId, so a hostless row cannot alias one. */ +const UNQUALIFIED_HOST_BUCKET = Symbol('unqualified-cleanup-host') + export type WorkspaceCleanupRemovalTarget = { kind: 'target' worktreeId: string @@ -90,8 +93,17 @@ export function resolveWorkspaceCleanupRemovalTargets( const displayName = approved[0]?.displayName ?? worktreeId // Why: one id confirmed for two hosts in a single batch cannot say which // row the user meant, and picking either one may delete the other's work. + // + // Why the STRICT resolver: the display identity defaults a hostless row to + // `local`, so a hostless row and a genuine local row for the same id share an + // identity and this gate would not fire. Destructive code may not guess a + // host — an unqualified row is its own bucket here. if ( - new Set(approved.map((candidate) => getWorkspaceCleanupCandidateIdentity(candidate))).size > 1 + new Set( + approved.map( + (candidate) => resolveWorkspaceCleanupRemovalHostId(candidate) ?? UNQUALIFIED_HOST_BUCKET + ) + ).size > 1 ) { return ambiguousHostFailure(worktreeId, displayName) }