From 40cb8ec1fc32cc58975bd241ad1662b18bd2b238 Mon Sep 17 00:00:00 2001 From: m4air Date: Wed, 7 Oct 2026 14:52:24 -0700 Subject: [PATCH 1/3] fix(projects): remove a project only on the host the user picked removeProject now requires a hostId and matches that host's row exactly. The bare-id fallbacks (unique candidate, focused host) could resolve to another host's live project and delete it. Orphaned SSH purge and group deletion now pass each row's own host. Fixes #13071 --- .../components/sidebar/RemoveFolderDialog.tsx | 13 +- .../sidebar/delete-worktree-flow.ts | 5 +- .../project-groups/project-group-mutations.ts | 19 +-- src/renderer/src/store/repos/repo-removal.ts | 19 ++- src/renderer/src/store/repos/repo-state.ts | 4 +- .../slices/project-removal-owner-host.test.ts | 131 ++++++++++++++++++ ...epos-cross-host-project-collisions.test.ts | 4 +- ...pos-ephemeral-vm-cleanup-retention.test.ts | 6 +- .../repos-host-identity-routing.test.ts | 10 +- .../repos-project-groups-delete.test.ts | 7 +- .../repos-remove-project-purge-leak.test.ts | 5 +- .../store/slices/repos-stale-fetch.test.ts | 5 +- src/renderer/src/store/slices/repos.test.ts | 17 ++- ...se-presets-repo-removal-purge-leak.test.ts | 3 +- .../slices/store-session-cascades.test.ts | 2 +- .../workspace-chat-draft-removal.test.ts | 2 +- .../orphaned-runtime-ssh-project-purge.ts | 18 +-- 17 files changed, 204 insertions(+), 66 deletions(-) create mode 100644 src/renderer/src/store/slices/project-removal-owner-host.test.ts diff --git a/src/renderer/src/components/sidebar/RemoveFolderDialog.tsx b/src/renderer/src/components/sidebar/RemoveFolderDialog.tsx index 0b5bd3cdb46f..f1b8856e14f3 100644 --- a/src/renderer/src/components/sidebar/RemoveFolderDialog.tsx +++ b/src/renderer/src/components/sidebar/RemoveFolderDialog.tsx @@ -13,7 +13,7 @@ import { translate } from '@/i18n/i18n' import { getRepoExecutionHostId, isRuntimeOwnedSshTargetId, - type ExecutionHostId + normalizeExecutionHostId } from '../../../../shared/execution-host' // Why: interpolated into the sentence so locales control where the name sits; @@ -29,7 +29,8 @@ const RemoveFolderDialog = React.memo(function RemoveFolderDialog() { const isOpen = activeModal === 'confirm-remove-folder' const repoId = typeof modalData.repoId === 'string' ? modalData.repoId : '' const displayName = typeof modalData.displayName === 'string' ? modalData.displayName : '' - const hostId = typeof modalData.hostId === 'string' ? (modalData.hostId as ExecutionHostId) : null + const hostId = + typeof modalData.hostId === 'string' ? normalizeExecutionHostId(modalData.hostId) : null // Why: for an SSH project the files live on the remote host's disk, not the // user's — "still on your disk" would be misleading. Name the host (using the @@ -75,11 +76,9 @@ const RemoveFolderDialog = React.memo(function RemoveFolderDialog() { const [descriptionBeforeName, descriptionAfterName] = description.split(NAME_TOKEN) const handleConfirm = useCallback(() => { - if (repoId) { - void removeProject(repoId, { - ...(hostId ? { hostId } : {}), - errorFeedback: 'toast' - }) + // Why: without the opener's host a bare id could remove another host's row (#13071). + if (repoId && hostId) { + void removeProject(repoId, { hostId, errorFeedback: 'toast' }) } closeModal() }, [closeModal, hostId, removeProject, repoId]) diff --git a/src/renderer/src/components/sidebar/delete-worktree-flow.ts b/src/renderer/src/components/sidebar/delete-worktree-flow.ts index dafda9e5f5fd..9d21a0c12126 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-flow.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-flow.ts @@ -10,7 +10,7 @@ import { getWorkspaceDeleteLineage } from './workspace-delete-lineage' import { resolveSshWorkspaceForget } from './ssh-workspace-forget-resolution' import { isPairedWebClientWindow } from '@/lib/desktop-window-chrome' import { parseWorkspaceKey } from '../../../../shared/workspace-scope' -import { getRepoExecutionHostId } from '../../../../shared/execution-host' +import { getWorktreeExecutionHostId } from '../../../../shared/execution-host' import { resolveWorktreeBatchDeleteTargets, toWorktreeDeleteIdentities, @@ -54,12 +54,11 @@ export function runWorktreeDelete(worktreeId: string, options: WorktreeDeleteOpt hostId: target.hostId, settings: state.settings }) - const hostId = repo ? getRepoExecutionHostId(repo) : target.hostId // Why: git refuses to delete the primary checkout; users can still remove the owning project from Orca (disk contents kept). state.openModal('confirm-remove-folder', { repoId: target.repoId, displayName: repo?.displayName ?? target.displayName, - ...(hostId ? { hostId } : {}) + hostId: getWorktreeExecutionHostId(target, repo ?? undefined) }) return } diff --git a/src/renderer/src/store/project-groups/project-group-mutations.ts b/src/renderer/src/store/project-groups/project-group-mutations.ts index c0f64fed8278..81b95a55a95e 100644 --- a/src/renderer/src/store/project-groups/project-group-mutations.ts +++ b/src/renderer/src/store/project-groups/project-group-mutations.ts @@ -188,16 +188,19 @@ export function createProjectGroupMutationActions( repo.id === projectId && (!ownerHostId || catalogOwnsHost(ownerHostId, getRepoExecutionHostId(repo))) ) - for (const projectId of targets.projectIds) { + for (const projectId of new Set(targets.projectIds)) { const ownedProjects = findOwnedProjects(projectId) - const projectHostId = - ownedProjects.length === 1 ? getRepoExecutionHostId(ownedProjects[0]) : undefined - try { - if (ownedProjects.length > 0) { - await get().removeProject(projectId, { hostId: projectHostId }) + // Why: an owned catalog's rows under the id are all members; an unstamped group already routes to the focused host. + const focusedProject = ownerHostId + ? null + : findRepoForHost(ownedProjects, projectId, { settings: get().settings }) + const removable = ownerHostId ? ownedProjects : focusedProject ? [focusedProject] : [] + for (const ownedProject of removable) { + try { + await get().removeProject(projectId, { hostId: getRepoExecutionHostId(ownedProject) }) + } catch (err) { + console.error('Failed to remove contained project:', err) } - } catch (err) { - console.error('Failed to remove contained project:', err) } const stillExists = findOwnedProjects(projectId).length > 0 if (stillExists) { diff --git a/src/renderer/src/store/repos/repo-removal.ts b/src/renderer/src/store/repos/repo-removal.ts index 572e8a78180f..5c73bb033263 100644 --- a/src/renderer/src/store/repos/repo-removal.ts +++ b/src/renderer/src/store/repos/repo-removal.ts @@ -4,7 +4,7 @@ import type { AppState } from '../types' import { getRepoIdFromWorktreeId } from '../../../../shared/worktree/id' import { getWorktreeIdFromVisitKey, getWorktreeVisitKey } from '@/lib/worktree-visit-recency' import { omitSparsePresetsForRepos } from '../slices/sparse-presets' -import { findRepoForHost, repoMatchesHostIdentity } from '../slices/repo-host-identity' +import { repoMatchesHostIdentity } from '../slices/repo-host-identity' import { callRuntimeRpc, getActiveRuntimeTarget, @@ -57,11 +57,10 @@ export function createRepoRemovalActions( return { removeProject: async (projectId, options) => { try { - // Why: pass an explicit hostId so a duplicate id across hosts resolves to the intended row, not the focused-host fallback. - const ownerRepo = findRepoForHost(get().repos, projectId, { - settings: get().settings, - hostId: options?.hostId - }) + // Why: exact host match only; the unique-candidate/focused fallbacks could pick another host's row (#13071). + const ownerRepo = get().repos.find((repo) => + repoMatchesHostIdentity(repo, projectId, options.hostId) + ) if (!ownerRepo) { return } @@ -79,10 +78,8 @@ export function createRepoRemovalActions( ) } } - // Why: derive the target from the owner's settings (via options.hostId) so an SSH host removal never routes repo.rm to the focused runtime. - const target = getActiveRuntimeTarget( - settingsForRepoOwner(get(), projectId, options?.hostId) - ) + // Why: derive the target from the owner row's host so an SSH host removal never routes repo.rm to the focused runtime. + const target = getActiveRuntimeTarget(settingsForRepoOwner(get(), projectId, ownerHostId)) // Why: repos:remove is id-only and would delete every host's row; scope local removal to the owning host so cross-host duplicates keep other rows. const idExistsOnOtherHost = get().repos.some( (repo) => repo.id === projectId && getRepoExecutionHostId(repo) !== ownerHostId @@ -285,7 +282,7 @@ export function createRepoRemovalActions( } catch (err) { console.error('Failed to remove repo:', err) // Why: bulk and background callers aggregate their own failures, so only opted-in single-project entry points toast (#11994). - if (options?.errorFeedback === 'toast') { + if (options.errorFeedback === 'toast') { toast.error( translate('auto.store.slices.repos.removeProjectFailed', 'Failed to remove project'), { diff --git a/src/renderer/src/store/repos/repo-state.ts b/src/renderer/src/store/repos/repo-state.ts index 356db14d9c6d..bc16fc387de9 100644 --- a/src/renderer/src/store/repos/repo-state.ts +++ b/src/renderer/src/store/repos/repo-state.ts @@ -249,11 +249,11 @@ export type RepoSlice = { groupId: string | null, order?: number ) => Promise - // options.hostId disambiguates which host's row to remove when the id exists on multiple hosts; else the focused host is assumed. + // options.hostId is required: a bare id can resolve to another host's row (#13071). Removes nothing when that host has no row. // options.errorFeedback defaults to 'silent' so bulk/background callers keep their own aggregate reporting. removeProject: ( projectId: string, - options?: { hostId?: ExecutionHostId; errorFeedback?: 'toast' | 'silent' } + options: { hostId: ExecutionHostId; errorFeedback?: 'toast' | 'silent' } ) => Promise updateProject: (projectId: string, updates: ProjectUpdate) => Promise // options.hostId targets a specific host's row + RPC target when the id exists on multiple hosts; else the focused host is assumed. diff --git a/src/renderer/src/store/slices/project-removal-owner-host.test.ts b/src/renderer/src/store/slices/project-removal-owner-host.test.ts new file mode 100644 index 000000000000..734531b69957 --- /dev/null +++ b/src/renderer/src/store/slices/project-removal-owner-host.test.ts @@ -0,0 +1,131 @@ +/** + * #13071: a project id can exist on several hosts. Removing one host's project must + * never resolve the bare id to another host's row (the unique-candidate or + * focused-host fallback), or the other host's live project is destroyed. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createTestStore } from './store-test-helpers' +import { purgeOrphanedRuntimeSshProjects } from './worktrees/teardown/orphaned-runtime-ssh-project-purge' +import type { ProjectGroup } from '../../../../shared/project-group-types' +import type { Repo } from '../../../../shared/repo-types' +import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' + +const baseRepo: Repo = { + id: 'same-repo', + path: '/work/same', + displayName: 'Same', + badgeColor: '#000', + addedAt: 1 +} +const localRow: Repo = { ...baseRepo, executionHostId: 'local' } +const sshRow: Repo = { + ...baseRepo, + path: '/remote/same', + connectionId: 'target-1', + executionHostId: 'ssh:target-1' +} +const runtimeRow: Repo = { + ...baseRepo, + path: '/server/same', + executionHostId: 'runtime:env-b' +} + +const group: ProjectGroup = { + id: 'group-1', + name: 'Platform', + parentPath: null, + parentGroupId: null, + createdFrom: 'manual', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1, + executionHostId: 'local' +} + +const reposRemove = vi.fn() +const reposRemoveForHost = vi.fn() +const projectGroupsDelete = vi.fn() +const runtimeCall = vi.fn() + +beforeEach(() => { + clearRuntimeCompatibilityCacheForTests() + for (const mock of [reposRemove, reposRemoveForHost, projectGroupsDelete, runtimeCall]) { + mock.mockReset() + } + reposRemove.mockResolvedValue(undefined) + reposRemoveForHost.mockResolvedValue(undefined) + projectGroupsDelete.mockResolvedValue(true) + vi.stubGlobal('window', { + api: { + repos: { remove: reposRemove, removeForHost: reposRemoveForHost }, + projectGroups: { delete: projectGroupsDelete }, + pty: { kill: vi.fn() }, + runtimeEnvironments: { call: runtimeCall }, + ui: { set: vi.fn().mockResolvedValue(undefined) } + } + }) +}) + +describe('project removal stays on its owner host (#13071)', () => { + it('leaves the only remaining row alone when the named host has no row', async () => { + const store = createTestStore() + // Host A's row dropped from the catalog; host B's row is the unique candidate for the id. + store.setState({ repos: [runtimeRow] }) + + await store.getState().removeProject('same-repo', { hostId: 'local' }) + + expect(store.getState().repos).toEqual([runtimeRow]) + expect(reposRemove).not.toHaveBeenCalled() + expect(reposRemoveForHost).not.toHaveBeenCalled() + expect(runtimeCall).not.toHaveBeenCalled() + }) + + it('purges a destroyed SSH host row without touching the same id on the focused host', async () => { + const store = createTestStore() + store.setState({ repos: [localRow, sshRow] }) + + await purgeOrphanedRuntimeSshProjects(store.getState, ['target-1']) + + expect(store.getState().repos).toEqual([localRow]) + expect(reposRemoveForHost).toHaveBeenCalledWith({ + repoId: 'same-repo', + hostId: 'ssh:target-1' + }) + expect(reposRemove).not.toHaveBeenCalled() + }) + + it('removes every row an owned group holds under the id, each on its own host', async () => { + const store = createTestStore() + store.setState({ + projectGroups: [group], + repos: [ + { ...localRow, projectGroupId: group.id }, + { ...sshRow, projectGroupId: group.id }, + runtimeRow + ] + }) + + const result = await store.getState().deleteProjectGroupWithContainedProjects(group.id, { + removeContainedProjects: true, + hostId: 'local' + }) + + expect(result).toMatchObject({ + removedProjectIds: ['same-repo'], + failedProjectRemovals: [] + }) + // The runtime host's row is outside the local catalog and must survive. + expect(store.getState().repos).toEqual([runtimeRow]) + expect(reposRemoveForHost).toHaveBeenCalledWith({ + repoId: 'same-repo', + hostId: 'local' + }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ + repoId: 'same-repo', + hostId: 'ssh:target-1' + }) + expect(runtimeCall).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts b/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts index d3cac1929aad..02037c11bed4 100644 --- a/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts +++ b/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts @@ -159,7 +159,7 @@ describe('deleting one host copy of a same-named project', () => { it('removes only the remote row when the same name exists locally', async () => { const store = seed([localTwin, remoteATwin], 'env-a') - await store.getState().removeProject('env-a-uuid') + await store.getState().removeProject('env-a-uuid', { hostId: 'runtime:env-a' }) expect(repoRmCalls()).toEqual([ expect.objectContaining({ @@ -185,7 +185,7 @@ describe('deleting one host copy of a same-named project', () => { it('removes only remote A when the same name exists on remote B', async () => { const store = seed([remoteATwin, remoteBTwin], 'env-b') - await store.getState().removeProject('env-a-uuid') + await store.getState().removeProject('env-a-uuid', { hostId: 'runtime:env-a' }) expect(repoRmCalls()).toEqual([ expect.objectContaining({ diff --git a/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts b/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts index afc54b88af16..4cd7c1f62d2a 100644 --- a/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts +++ b/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts @@ -1,5 +1,6 @@ import { expect, it, vi } from 'vitest' import { toast } from 'sonner' +import { getRepoExecutionHostId } from '../../../../shared/execution-host' import type { Repo } from '../../../../shared/repo-types' import { ephemeralVmCleanup, @@ -38,7 +39,10 @@ it('retains a runtime-owned SSH project when VM cleanup fails', async () => { const store = createTestStore() store.setState({ repos: [runtimeRepo], activeRepoId: runtimeRepo.id }) - await store.getState().removeProject(runtimeRepo.id, { errorFeedback: 'toast' }) + await store.getState().removeProject(runtimeRepo.id, { + hostId: getRepoExecutionHostId(runtimeRepo), + errorFeedback: 'toast' + }) expect(store.getState().repos).toEqual([runtimeRepo]) expect(reposRemove).not.toHaveBeenCalled() diff --git a/src/renderer/src/store/slices/repos-host-identity-routing.test.ts b/src/renderer/src/store/slices/repos-host-identity-routing.test.ts index 9b6bf765d437..d04028e44f7f 100644 --- a/src/renderer/src/store/slices/repos-host-identity-routing.test.ts +++ b/src/renderer/src/store/slices/repos-host-identity-routing.test.ts @@ -301,7 +301,7 @@ describe('repo slice host identity routing', () => { ]) }) - it('removes only the focused host row and worktrees for duplicate repo ids', async () => { + it('removes only the named host row and worktrees for duplicate repo ids', async () => { const localWorktree = makeWorktree({ id: 'same-repo::/local/wt', repoId: 'same-repo' @@ -345,7 +345,7 @@ describe('repo slice host identity routing', () => { } }) - await store.getState().removeProject('same-repo') + await store.getState().removeProject('same-repo', { hostId: 'local' }) expect(store.getState().repos).toEqual([remoteDuplicate]) // Current contract: bare-ID UI selections clear even though the sibling host row survives. @@ -408,8 +408,8 @@ describe('repo slice host identity routing', () => { const store = createTestStore() store.setState({ repos: [localDuplicate] }) - const first = store.getState().removeProject(localDuplicate.id) - const second = store.getState().removeProject(localDuplicate.id) + const first = store.getState().removeProject(localDuplicate.id, { hostId: 'local' }) + const second = store.getState().removeProject(localDuplicate.id, { hostId: 'local' }) expect(reposRemove).toHaveBeenCalledTimes(2) resolveSecondRemoval() @@ -553,7 +553,7 @@ describe('repo slice host identity routing', () => { } }) - await store.getState().removeProject('same-repo') + await store.getState().removeProject('same-repo', { hostId: 'runtime:env-1' }) expect(store.getState().repos).toEqual([localDuplicate]) expect(store.getState().worktreesByRepo['same-repo']).toEqual([localWorktree]) diff --git a/src/renderer/src/store/slices/repos-project-groups-delete.test.ts b/src/renderer/src/store/slices/repos-project-groups-delete.test.ts index 8d8093fc8fe2..cf84819499a6 100644 --- a/src/renderer/src/store/slices/repos-project-groups-delete.test.ts +++ b/src/renderer/src/store/slices/repos-project-groups-delete.test.ts @@ -227,12 +227,7 @@ describe('project group deletion store routing', () => { groupId: projectGroup.id, requestedProjectIds: ['shared', 'shared'], removedProjectIds: ['shared'], - failedProjectRemovals: [ - { - projectId: 'shared', - reason: 'Project remained in Orca after removeProject completed.' - } - ] + failedProjectRemovals: [] }) expect(reposRemoveForHost).toHaveBeenCalledWith({ diff --git a/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts b/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts index 2c35bb35fb8a..7606c2b1511e 100644 --- a/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts +++ b/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts @@ -13,6 +13,7 @@ * grew monotonically. The single `removeWorktree` path already routes through * `purgeWorktreeTerminalState`; project removal did not. */ +import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { describe, it, expect, vi, beforeEach } from 'vitest' import { createTestStore, makeWorktree } from './store-test-helpers' import type { Repo } from '../../../../shared/repo-types' @@ -63,7 +64,7 @@ describe('removeProject purges per-worktree state (leak regression)', () => { const store = createTestStore() seedTwoProjects(store) - await store.getState().removeProject(repo1.id) + await store.getState().removeProject(repo1.id, { hostId: getRepoExecutionHostId(repo1) }) const s = store.getState() // Removed project's worktree is purged from every map. @@ -79,7 +80,7 @@ describe('removeProject purges per-worktree state (leak regression)', () => { const store = createTestStore() seedTwoProjects(store) - await store.getState().removeProject(repo1.id) + await store.getState().removeProject(repo1.id, { hostId: getRepoExecutionHostId(repo1) }) const s = store.getState() // Surviving project's worktree state is untouched (guard over-eviction). diff --git a/src/renderer/src/store/slices/repos-stale-fetch.test.ts b/src/renderer/src/store/slices/repos-stale-fetch.test.ts index d3b04b1b078e..86093114ab81 100644 --- a/src/renderer/src/store/slices/repos-stale-fetch.test.ts +++ b/src/renderer/src/store/slices/repos-stale-fetch.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi, beforeEach } from 'vitest' +import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { createTestStore, makeLayout, makeTab } from './store-test-helpers' import type { Repo } from '../../../../shared/repo-types' import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' @@ -119,7 +120,9 @@ describe('repos slice stale-fetch race (#7020)', () => { store.setState({ repos: [localRepo] }) const pending = store.getState().fetchRepos() - await store.getState().removeProject(localRepo.id) + await store + .getState() + .removeProject(localRepo.id, { hostId: getRepoExecutionHostId(localRepo) }) expect(store.getState().repos).toEqual([]) resolveCatalog([localRepo]) diff --git a/src/renderer/src/store/slices/repos.test.ts b/src/renderer/src/store/slices/repos.test.ts index d5a246b5da5e..bbec0a1e4efd 100644 --- a/src/renderer/src/store/slices/repos.test.ts +++ b/src/renderer/src/store/slices/repos.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from 'vitest' +import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { createTestStore, makeWorktree } from './store-test-helpers' import { workItemsCacheKey } from '../github/cache-identity' import type { Project, ProjectHostSetup } from '../../../../shared/project-types' @@ -35,6 +36,8 @@ vi.mock('sonner', () => ({ installReposRuntimeRoutingHarness() +const ownerOf = (repo: Repo) => ({ hostId: getRepoExecutionHostId(repo) }) + describe('repo slice runtime routing', () => { it('fetches repos from local IPC when no remote environment is active', async () => { reposList.mockResolvedValue([localRepo]) @@ -658,7 +661,7 @@ describe('repo slice runtime routing', () => { activeRepoId: remoteRepo.id }) - await store.getState().removeProject(remoteRepo.id) + await store.getState().removeProject(remoteRepo.id, ownerOf(remoteRepo)) expect(store.getState().repos).toEqual([]) expect(store.getState().activeRepoId).toBeNull() @@ -683,7 +686,7 @@ describe('repo slice runtime routing', () => { } }) - await store.getState().removeProject(sshRepo.id) + await store.getState().removeProject(sshRepo.id, ownerOf(sshRepo)) expect(store.getState().repos).toEqual([]) expect(store.getState().activeRepoId).toBeNull() @@ -704,7 +707,7 @@ describe('repo slice runtime routing', () => { } }) - await store.getState().removeProject(sshRepo.id) + await store.getState().removeProject(sshRepo.id, ownerOf(sshRepo)) expect(store.getState().repos).toEqual([localRepo]) expect(store.getState().lastVisitedAtByWorktreeId).toEqual({ [localWorktreeId]: 200 }) @@ -731,7 +734,7 @@ describe('repo slice runtime routing', () => { } }) - await store.getState().removeProject(remoteRepo.id) + await store.getState().removeProject(remoteRepo.id, ownerOf(remoteRepo)) expect(store.getState().repos).toEqual([localRepo]) expect(store.getState().lastVisitedAtByWorktreeId).toEqual({ [localWorktreeId]: 200 }) @@ -761,7 +764,7 @@ describe('repo slice runtime routing', () => { } }) - await store.getState().removeProject(localRepo.id) + await store.getState().removeProject(localRepo.id, ownerOf(localRepo)) expect(Object.keys(store.getState().workItemsCache)).toEqual([ workItemsCacheKey('other-repo', 20, '') @@ -793,7 +796,7 @@ describe('repo slice runtime routing', () => { } }) - await store.getState().removeProject(remoteRepo.id) + await store.getState().removeProject(remoteRepo.id, ownerOf(remoteRepo)) expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ selector: 'env-1', @@ -839,7 +842,7 @@ describe('repo slice runtime routing', () => { activeWorktreeId: hiddenWorktree.id }) - await store.getState().removeProject(localRepo.id) + await store.getState().removeProject(localRepo.id, ownerOf(localRepo)) expect(store.getState().detectedWorktreesByRepo[localRepo.id]).toBeUndefined() expect(store.getState().tabsByWorktree[hiddenWorktree.id]).toBeUndefined() diff --git a/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts b/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts index 429e3e793d8b..421e5b6f972e 100644 --- a/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts +++ b/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts @@ -7,6 +7,7 @@ * so orphaned entries accumulated for the renderer's whole session. repoIds are * random UUIDs (never reused), so re-adding a repo cannot reclaim the old keys. */ +import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { describe, it, expect, vi, beforeEach } from 'vitest' import { createTestStore, makeWorktree } from './store-test-helpers' import type { Repo } from '../../../../shared/repo-types' @@ -54,7 +55,7 @@ describe('removeProject purges the removed repo sparse-preset maps (leak regress const store = createTestStore() seed(store) - await store.getState().removeProject(repo1.id) + await store.getState().removeProject(repo1.id, { hostId: getRepoExecutionHostId(repo1) }) const s = store.getState() // Removed repo: every sparse-preset map key is gone. diff --git a/src/renderer/src/store/slices/store-session-cascades.test.ts b/src/renderer/src/store/slices/store-session-cascades.test.ts index 1cdb873af109..a229a223b36e 100644 --- a/src/renderer/src/store/slices/store-session-cascades.test.ts +++ b/src/renderer/src/store/slices/store-session-cascades.test.ts @@ -58,7 +58,7 @@ describe('removeProject cascade', () => { activeTabId: 'tab1' }) - await store.getState().removeProject('repo1') + await store.getState().removeProject('repo1', { hostId: 'local' }) const s = store.getState() expect(s.repos).toEqual([]) diff --git a/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts b/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts index 0f81e7f6c573..de8fe2dbf693 100644 --- a/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts +++ b/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts @@ -217,7 +217,7 @@ describe('workspace chat drafts on removal', () => { } }) - await store.getState().removeProject('repo1') + await store.getState().removeProject('repo1', { hostId: 'local' }) expect(draftTexts()).toEqual(['', '', 'other']) }) diff --git a/src/renderer/src/store/slices/worktrees/teardown/orphaned-runtime-ssh-project-purge.ts b/src/renderer/src/store/slices/worktrees/teardown/orphaned-runtime-ssh-project-purge.ts index 428479f86230..023889faaf4c 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/orphaned-runtime-ssh-project-purge.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/orphaned-runtime-ssh-project-purge.ts @@ -1,4 +1,8 @@ -import { toSshExecutionHostId, type ExecutionHostId } from '../../../../../../shared/execution-host' +import { + getRepoExecutionHostId, + toSshExecutionHostId, + type ExecutionHostId +} from '../../../../../../shared/execution-host' import type { AppState } from '../../../types' // Why: an SSH per-workspace-env project's host is the runtime-owned SSH target; once that runtime is destroyed, remove the project or it lingers as a dead, never-connectable one. @@ -30,14 +34,12 @@ export async function purgeOrphanedRuntimeSshProjects( } } // A repo whose only host was the destroyed runtime can outlive its setup (pruned first by a projection refresh); remove it directly so no dead project lingers. - const orphanedRepoIds = get() - .repos.filter( - (repo) => destroyedTargetIds.has(repo.connectionId ?? '') && !purgedRepoIds.has(repo.id) - ) - .map((repo) => repo.id) - for (const repoId of orphanedRepoIds) { + const orphanedRepos = get().repos.filter( + (repo) => destroyedTargetIds.has(repo.connectionId ?? '') && !purgedRepoIds.has(repo.id) + ) + for (const repo of orphanedRepos) { try { - await get().removeProject(repoId) + await get().removeProject(repo.id, { hostId: getRepoExecutionHostId(repo) }) } catch (error) { console.error('Failed to purge orphaned per-workspace-env repo:', error) } From 0dfd946bb43530a8e845bcb8b7dd4bb4490f16b0 Mon Sep 17 00:00:00 2001 From: m4air Date: Wed, 7 Oct 2026 15:08:16 -0700 Subject: [PATCH 2/3] fix(projects): group delete removes only the group's member rows Same-id rows on other hosts outside the group were removed because membership was rebuilt from bare ids after the cascade cleared it. Capture (id, host) member pairs before deletion and remove only those. --- .../project-groups/project-group-mutations.ts | 40 ++++++++++-------- .../slices/project-group-removal-targets.ts | 15 +++++-- .../slices/project-removal-owner-host.test.ts | 42 +++++++++++++++++++ 3 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/renderer/src/store/project-groups/project-group-mutations.ts b/src/renderer/src/store/project-groups/project-group-mutations.ts index 81b95a55a95e..3c1e79288cce 100644 --- a/src/renderer/src/store/project-groups/project-group-mutations.ts +++ b/src/renderer/src/store/project-groups/project-group-mutations.ts @@ -4,7 +4,6 @@ import type { ProjectGroup } from '../../../../shared/project-group-types' import type { Repo } from '../../../../shared/repo-types' import { selectProjectGroupRemovalTargets } from '../slices/project-group-removal-targets' import { - catalogOwnsHost, getProjectGroupHostId, projectGroupMatchesOwnerHost, resolveProjectGroupOwnerHostId, @@ -180,29 +179,36 @@ export function createProjectGroupMutationActions( const removedProjectIds: string[] = [] const failedProjectRemovals: ProjectRemovalFailure[] = [] - // Why: the group's catalog can hold rows from several hosts (a local catalog also owns SSH rows), - // so each project is removed on its own host rather than on the group's. - const findOwnedProjects = (projectId: string): Repo[] => - get().repos.filter( - (repo) => - repo.id === projectId && - (!ownerHostId || catalogOwnsHost(ownerHostId, getRepoExecutionHostId(repo))) - ) + // Why: members were captured before the cascade cleared membership; a same-id row on another + // host outside the group must survive, so each member row is removed on its own host. + const membersById = new Map() + for (const member of targets.projects) { + const row = findRepoForHost(get().repos, member.id, { hostId: member.hostId }) + if (row) { + membersById.set(member.id, [...(membersById.get(member.id) ?? []), row]) + } + } for (const projectId of new Set(targets.projectIds)) { - const ownedProjects = findOwnedProjects(projectId) - // Why: an owned catalog's rows under the id are all members; an unstamped group already routes to the focused host. - const focusedProject = ownerHostId + const members = membersById.get(projectId) ?? [] + // Why: an unstamped group already routes to the focused host, so only that host's member is removed. + const focusedMember = ownerHostId ? null - : findRepoForHost(ownedProjects, projectId, { settings: get().settings }) - const removable = ownerHostId ? ownedProjects : focusedProject ? [focusedProject] : [] - for (const ownedProject of removable) { + : findRepoForHost(members, projectId, { settings: get().settings }) + const removable = ownerHostId ? members : focusedMember ? [focusedMember] : [] + const removableHostIds = removable.map((row) => getRepoExecutionHostId(row)) + for (const hostId of removableHostIds) { try { - await get().removeProject(projectId, { hostId: getRepoExecutionHostId(ownedProject) }) + await get().removeProject(projectId, { hostId }) } catch (err) { console.error('Failed to remove contained project:', err) } } - const stillExists = findOwnedProjects(projectId).length > 0 + // Why: an ambiguous unstamped member with no focused-host row was left in place, so it counts as remaining. + const stillExists = + (members.length > 0 && removableHostIds.length === 0) || + removableHostIds.some((hostId) => + get().repos.some((repo) => repoMatchesHostIdentity(repo, projectId, hostId)) + ) if (stillExists) { failedProjectRemovals.push({ projectId, diff --git a/src/renderer/src/store/slices/project-group-removal-targets.ts b/src/renderer/src/store/slices/project-group-removal-targets.ts index 667d74ceb1f2..3ff2e55b3578 100644 --- a/src/renderer/src/store/slices/project-group-removal-targets.ts +++ b/src/renderer/src/store/slices/project-group-removal-targets.ts @@ -4,10 +4,14 @@ import { getProjectGroupSubtreeIds } from '../../../../shared/project-groups' import { getRepoExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' import { catalogOwnsHost, getProjectGroupHostId } from './project-group-owner-routing' +export type ProjectGroupRemovalProject = { id: string; hostId: ExecutionHostId } + export type ProjectGroupRemovalTargets = { groupExists: boolean deletedGroupIds: Set projectIds: string[] + // Why: ids repeat across hosts and membership is per row, so removal must target the exact member rows. + projects: ProjectGroupRemovalProject[] } export function selectProjectGroupRemovalTargets( @@ -27,25 +31,30 @@ export function selectProjectGroupRemovalTargets( return { groupExists: false, deletedGroupIds: new Set(), - projectIds: [] + projectIds: [], + projects: [] } } const deletedGroupIds = getProjectGroupSubtreeIds(ownerGroups, groupId) const projectIds: string[] = [] + const projects: ProjectGroupRemovalProject[] = [] for (const repo of repos) { + const repoHostId = getRepoExecutionHostId(repo) if ( repo.projectGroupId && deletedGroupIds.has(repo.projectGroupId) && - ownsRowHost(getRepoExecutionHostId(repo)) + ownsRowHost(repoHostId) ) { projectIds.push(repo.id) + projects.push({ id: repo.id, hostId: repoHostId }) } } return { groupExists: true, deletedGroupIds, - projectIds + projectIds, + projects } } diff --git a/src/renderer/src/store/slices/project-removal-owner-host.test.ts b/src/renderer/src/store/slices/project-removal-owner-host.test.ts index 734531b69957..21d74829443d 100644 --- a/src/renderer/src/store/slices/project-removal-owner-host.test.ts +++ b/src/renderer/src/store/slices/project-removal-owner-host.test.ts @@ -128,4 +128,46 @@ describe('project removal stays on its owner host (#13071)', () => { }) expect(runtimeCall).not.toHaveBeenCalled() }) + + it('keeps a same-id row on another host that sits outside the deleted group', async () => { + const store = createTestStore() + const otherGroup: ProjectGroup = { ...group, id: 'group-2', name: 'Other' } + const outsideSshRow: Repo = { ...sshRow, projectGroupId: otherGroup.id } + store.setState({ + projectGroups: [group, otherGroup], + repos: [{ ...localRow, projectGroupId: group.id }, outsideSshRow] + }) + + const result = await store.getState().deleteProjectGroupWithContainedProjects(group.id, { + removeContainedProjects: true, + hostId: 'local' + }) + + expect(result).toMatchObject({ + removedProjectIds: ['same-repo'], + failedProjectRemovals: [] + }) + expect(store.getState().repos).toEqual([outsideSshRow]) + expect(reposRemoveForHost).toHaveBeenCalledTimes(1) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'same-repo', hostId: 'local' }) + }) + + it('removes only the member row of an unstamped group, not the focused-host non-member', async () => { + const store = createTestStore() + const unstampedGroup: ProjectGroup = { ...group, executionHostId: undefined } + const memberSshRow: Repo = { ...sshRow, projectGroupId: unstampedGroup.id } + store.setState({ projectGroups: [unstampedGroup], repos: [localRow, memberSshRow] }) + + const result = await store.getState().deleteProjectGroupWithContainedProjects(group.id, { + removeContainedProjects: true + }) + + expect(result).toMatchObject({ + removedProjectIds: ['same-repo'], + failedProjectRemovals: [] + }) + expect(store.getState().repos).toEqual([localRow]) + expect(reposRemoveForHost).toHaveBeenCalledTimes(1) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'same-repo', hostId: 'ssh:target-1' }) + }) }) From 116903f66c4b4db45aeb6f9ddc2a6876295a452f Mon Sep 17 00:00:00 2001 From: m4air Date: Wed, 7 Oct 2026 16:44:51 -0700 Subject: [PATCH 3/3] fix(projects): never let a stale catalog widen a project removal to every host The renderer chose id-only `repos:remove` when its own catalog showed no same-id row on another host. A stale catalog (another window, a paired client, or a pending refresh) hid that row, and main then deleted every host's registration and saved workspace metadata. - The local catalog route now always calls `repos:removeForHost` with the owner row's host, for local, direct SSH, git and folder projects. - `Store.removeProject` refuses an id that rows on more than one host share, so no id-only caller can delete another host's row. - Setup rollbacks remove the just-created row by its host. --- .../repos-local-add-and-project-setup.test.ts | 3 +- src/main/ipc/repos-remote-test-harness.ts | 2 + .../ipc/repos/project-host-setup-handlers.ts | 4 +- ...tence-duplicate-repo-id-host-scope.test.ts | 24 +++++++ .../repo-lifecycle-operations.ts | 11 +++- .../repository-project-operations.spec.ts | 4 +- .../runtime-project-host-setup-controller.ts | 6 +- src/renderer/src/store/repos/repo-removal.ts | 9 +-- .../slices/project-removal-owner-host.test.ts | 65 +++++++++++++++++++ ...epos-cross-host-project-collisions.test.ts | 3 +- .../repos-host-identity-routing.test.ts | 4 +- .../repos-project-groups-delete.test.ts | 13 ++-- .../repos-remove-project-purge-leak.test.ts | 2 +- .../slices/repos-runtime-routing-fixture.ts | 3 + .../store/slices/repos-stale-fetch.test.ts | 2 +- src/renderer/src/store/slices/repos.test.ts | 5 +- ...se-presets-repo-removal-purge-leak.test.ts | 2 +- .../slices/store-session-cascades.test.ts | 2 +- .../slices/store-session-test-harness.ts | 10 ++- .../workspace-chat-draft-removal.test.ts | 4 +- 20 files changed, 147 insertions(+), 31 deletions(-) diff --git a/src/main/ipc/repos-local-add-and-project-setup.test.ts b/src/main/ipc/repos-local-add-and-project-setup.test.ts index 0ff8b48c773e..3be8f6b5b398 100644 --- a/src/main/ipc/repos-local-add-and-project-setup.test.ts +++ b/src/main/ipc/repos-local-add-and-project-setup.test.ts @@ -343,7 +343,8 @@ describe('repos:add + repos:clone', () => { }) ).rejects.toThrow('Imported folder does not match the selected project identity.') - expect(mockStore.removeProject).toHaveBeenCalledWith(added[0]?.id) + expect(mockStore.removeProjectForHost).toHaveBeenCalledWith(added[0]?.id, 'local') + expect(mockStore.removeProject).not.toHaveBeenCalled() expect(invalidateAuthorizedRootsCacheMock).toHaveBeenCalled() }) diff --git a/src/main/ipc/repos-remote-test-harness.ts b/src/main/ipc/repos-remote-test-harness.ts index 3e7cfe783c23..03f06a9561e8 100644 --- a/src/main/ipc/repos-remote-test-harness.ts +++ b/src/main/ipc/repos-remote-test-harness.ts @@ -16,6 +16,7 @@ export type ReposIpcMocks = { | 'getRepos' | 'addRepo' | 'removeProject' + | 'removeProjectForHost' | 'getRepo' | 'getProjects' | 'getProjectHostSetups' @@ -53,6 +54,7 @@ export function createReposIpcMocks(): ReposIpcMocks { getRepos: vi.fn().mockReturnValue([]), addRepo: vi.fn(), removeProject: vi.fn(), + removeProjectForHost: vi.fn(), getRepo: vi.fn(), updateRepo: vi.fn(), getProjects: vi.fn().mockReturnValue([]), diff --git a/src/main/ipc/repos/project-host-setup-handlers.ts b/src/main/ipc/repos/project-host-setup-handlers.ts index 3feb101324c3..8898d5ec07a6 100644 --- a/src/main/ipc/repos/project-host-setup-handlers.ts +++ b/src/main/ipc/repos/project-host-setup-handlers.ts @@ -14,7 +14,7 @@ import type { } from '../../../shared/project-types' import { getProjectIdForProviderIdentity } from '../../../shared/project-host-setup-projection' import { getProjectHostSetupForRepo } from '../../../shared/project-host-setup-lookup' -import { parseExecutionHostId } from '../../../shared/execution-host' +import { getRepoExecutionHostId, parseExecutionHostId } from '../../../shared/execution-host' import { prepareLocalWorktreeRootForRepo } from '../../worktree-root-preparation' import { invalidateAuthorizedRootsCache } from '../registered-worktree-roots-cache' import { emitRepoAdded } from './repo-added-telemetry' @@ -177,7 +177,7 @@ export function registerProjectHostSetupHandlers(mainWindow: BrowserWindow, stor } catch (err) { // Why: an import that cannot be linked must not leave a new repo registration or authorization root behind. if (!result.alreadyExisted) { - store.removeProject(result.repo.id) + store.removeProjectForHost(result.repo.id, getRepoExecutionHostId(result.repo)) invalidateAuthorizedRootsCache() } throw err diff --git a/src/main/persistence-duplicate-repo-id-host-scope.test.ts b/src/main/persistence-duplicate-repo-id-host-scope.test.ts index 48cb826f909d..abbedfe5ab2e 100644 --- a/src/main/persistence-duplicate-repo-id-host-scope.test.ts +++ b/src/main/persistence-duplicate-repo-id-host-scope.test.ts @@ -111,6 +111,30 @@ describe('deleting one host copy of a repo id shared by two hosts', () => { expect(store.getRepos().map((repo) => repo.path)).toEqual(['/laptop/dup', '/remote/dup']) }) + it.each(['git', 'folder'] as const)( + 'refuses an id-only removeProject that would take the other host %s row and its metadata', + async (kind) => { + // A stale renderer catalog once sent id-only removal when it could not see the SSH twin (#13071). + const store = await createStoreFromState({ + repos: duplicateIdRepos().map((repo) => ({ ...repo, kind })) + }) + store.setWorktreeMetaForHost('dup::/remote/dup', 'ssh:ssh-1', { displayName: 'SSH wt' }) + + expect(() => store.removeProject('dup')).toThrow('repo_id_ambiguous') + + expect(store.getRepos().map((repo) => repo.path)).toEqual(['/laptop/dup', '/remote/dup']) + expect(Object.values(store.getAllWorktreeMeta()).map((meta) => meta.displayName)).toContain( + 'SSH wt' + ) + + store.removeProjectForHost('dup', 'local') + expect(store.getRepos().map((repo) => repo.path)).toEqual(['/remote/dup']) + expect(Object.values(store.getAllWorktreeMeta()).map((meta) => meta.displayName)).toContain( + 'SSH wt' + ) + } + ) + it('removeProjectForHost drops only the addressed host row', async () => { const store = await createStoreWithDuplicateRepoId() diff --git a/src/main/persistence/loading-store/repo-lifecycle-operations.ts b/src/main/persistence/loading-store/repo-lifecycle-operations.ts index 1dde30761169..f853e17a3e27 100644 --- a/src/main/persistence/loading-store/repo-lifecycle-operations.ts +++ b/src/main/persistence/loading-store/repo-lifecycle-operations.ts @@ -63,9 +63,16 @@ export class RepoLifecycleOperations { } removeProject(id: string): void { - const repoRemoved = this[repoLifecycleOperationsContext].runtime.state.repos.some( - (repo) => repo.id === id + const matchingHostIds = new Set( + this[repoLifecycleOperationsContext].runtime.state.repos + .filter((repo) => repo.id === id) + .map((repo) => getRepoExecutionHostId(repo)) ) + // Why: an id-only delete would take every host's registration and metadata; callers must name the host (#13071). + if (matchingHostIds.size > 1) { + throw new Error(`repo_id_ambiguous: ${id} exists on more than one host; remove it by host.`) + } + const repoRemoved = matchingHostIds.size > 0 this[repoLifecycleOperationsContext].runtime.state.repos = this[ repoLifecycleOperationsContext ].runtime.state.repos.filter((r) => r.id !== id) diff --git a/src/main/runtime/orca-runtime-tests/repository-project-operations.spec.ts b/src/main/runtime/orca-runtime-tests/repository-project-operations.spec.ts index c9879bf3d7cc..31294118b604 100644 --- a/src/main/runtime/orca-runtime-tests/repository-project-operations.spec.ts +++ b/src/main/runtime/orca-runtime-tests/repository-project-operations.spec.ts @@ -267,7 +267,7 @@ describe('OrcaRuntimeService', () => { Object.assign(repo, updates) return { ...repo } as never }, - removeProject: (id: string) => { + removeProjectForHost: (id: string) => { const index = repos.findIndex((repo) => repo.id === id) if (index !== -1) { repos.splice(index, 1) @@ -309,7 +309,7 @@ describe('OrcaRuntimeService', () => { const runtimeStore = { ...store, getRepos: () => [...repos] as never, - removeProject: (id: string) => { + removeProjectForHost: (id: string) => { const index = repos.findIndex((repo) => repo.id === id) if (index !== -1) { repos.splice(index, 1) diff --git a/src/main/runtime/runtime-project-host-setup-controller.ts b/src/main/runtime/runtime-project-host-setup-controller.ts index cdc3ab1c60f1..bfcbb4e5fe19 100644 --- a/src/main/runtime/runtime-project-host-setup-controller.ts +++ b/src/main/runtime/runtime-project-host-setup-controller.ts @@ -14,6 +14,7 @@ import type { } from '../../shared/project-types' import type { Repo } from '../../shared/repo-types' import { + getRepoExecutionHostId, getSshTargetIdForExecutionHost, parseExecutionHostId, type ExecutionHostId @@ -162,7 +163,10 @@ export class RuntimeProjectHostSetupController { return this.linkRepo(args, initialRepo) } catch (error) { if (repoWasCreated) { - this.deps.getStore()?.removeProject?.(initialRepo.id) + // Why: host-scoped so a rollback never takes a same-id row on another host. + this.deps + .getStore() + ?.removeProjectForHost?.(initialRepo.id, getRepoExecutionHostId(initialRepo)) this.deps.invalidateResolvedWorktrees() this.deps.invalidateWorktreeScan(initialRepo.id) invalidateAuthorizedRootsCache() diff --git a/src/renderer/src/store/repos/repo-removal.ts b/src/renderer/src/store/repos/repo-removal.ts index 5c73bb033263..1943b3dd722f 100644 --- a/src/renderer/src/store/repos/repo-removal.ts +++ b/src/renderer/src/store/repos/repo-removal.ts @@ -80,10 +80,6 @@ export function createRepoRemovalActions( } // Why: derive the target from the owner row's host so an SSH host removal never routes repo.rm to the focused runtime. const target = getActiveRuntimeTarget(settingsForRepoOwner(get(), projectId, ownerHostId)) - // Why: repos:remove is id-only and would delete every host's row; scope local removal to the owning host so cross-host duplicates keep other rows. - const idExistsOnOtherHost = get().repos.some( - (repo) => repo.id === projectId && getRepoExecutionHostId(repo) !== ownerHostId - ) // Why before the host call: its announcement can start a listing refresh that drops these tabs. const chatDraftKeys = captureWorkspaceChatDraftKeys( get(), @@ -93,10 +89,9 @@ export function createRepoRemovalActions( })) ) try { + // Why: always host-scoped; this catalog may be stale and miss a same-id row on another host (#13071). await (target.kind === 'local' - ? idExistsOnOtherHost - ? window.api.repos.removeForHost({ repoId: projectId, hostId: ownerHostId }) - : window.api.repos.remove({ repoId: projectId }) + ? window.api.repos.removeForHost({ repoId: projectId, hostId: ownerHostId }) : callRuntimeRpc(target, 'repo.rm', { repo: projectId }, { timeoutMs: 15_000 })) } catch (err) { // Why: the owner already dropped this project, so purge the local ghost row instead of aborting (#11994). diff --git a/src/renderer/src/store/slices/project-removal-owner-host.test.ts b/src/renderer/src/store/slices/project-removal-owner-host.test.ts index 21d74829443d..4d798fbcf934 100644 --- a/src/renderer/src/store/slices/project-removal-owner-host.test.ts +++ b/src/renderer/src/store/slices/project-removal-owner-host.test.ts @@ -171,3 +171,68 @@ describe('project removal stays on its owner host (#13071)', () => { expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'same-repo', hostId: 'ssh:target-1' }) }) }) + +describe('removal stays host-scoped when this catalog is stale (#13071)', () => { + type MainRow = { id: string; hostId: string; kind: 'git' | 'folder' } + + // Mirrors the main catalog: id-only removal drops every host's row, removeForHost one row. + function stubMainCatalog(rows: MainRow[]): { current: MainRow[] } { + const main = { current: rows } + reposRemove.mockImplementation(async ({ repoId }: { repoId: string }) => { + main.current = main.current.filter((row) => row.id !== repoId) + }) + reposRemoveForHost.mockImplementation( + async ({ repoId, hostId }: { repoId: string; hostId: string }) => { + main.current = main.current.filter((row) => !(row.id === repoId && row.hostId === hostId)) + } + ) + return main + } + + it.each(['git', 'folder'] as const)( + 'keeps an unseen same-id SSH %s project when removing the local one', + async (kind) => { + const main = stubMainCatalog([ + { id: 'same-repo', hostId: 'local', kind }, + { id: 'same-repo', hostId: 'ssh:target-1', kind } + ]) + const store = createTestStore() + // Another window added the SSH twin; this renderer has not refreshed yet. + store.setState({ repos: [{ ...localRow, kind }] }) + + await store.getState().removeProject('same-repo', { hostId: 'local' }) + + expect(reposRemove).not.toHaveBeenCalled() + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'same-repo', hostId: 'local' }) + expect(main.current).toEqual([{ id: 'same-repo', hostId: 'ssh:target-1', kind }]) + } + ) + + it.each(['git', 'folder'] as const)( + 'removes a direct SSH %s project without touching an unseen local twin', + async (kind) => { + const main = stubMainCatalog([ + { id: 'same-repo', hostId: 'local', kind }, + { id: 'same-repo', hostId: 'ssh:target-1', kind } + ]) + const store = createTestStore() + store.setState({ repos: [{ ...sshRow, kind }] }) + + await store.getState().removeProject('same-repo', { hostId: 'ssh:target-1' }) + + expect(reposRemove).not.toHaveBeenCalled() + expect(main.current).toEqual([{ id: 'same-repo', hostId: 'local', kind }]) + } + ) + + it('never falls back to id-only removal when the selected row is already gone in main', async () => { + const main = stubMainCatalog([{ id: 'same-repo', hostId: 'ssh:target-1', kind: 'git' }]) + const store = createTestStore() + store.setState({ repos: [localRow] }) + + await store.getState().removeProject('same-repo', { hostId: 'local' }) + + expect(reposRemove).not.toHaveBeenCalled() + expect(main.current).toEqual([{ id: 'same-repo', hostId: 'ssh:target-1', kind: 'git' }]) + }) +}) diff --git a/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts b/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts index 02037c11bed4..a8e4e0496799 100644 --- a/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts +++ b/src/renderer/src/store/slices/repos-cross-host-project-collisions.test.ts @@ -177,7 +177,8 @@ describe('deleting one host copy of a same-named project', () => { await store.getState().removeProject('local-uuid', { hostId: 'local' }) - expect(reposRemove).toHaveBeenCalledWith({ repoId: 'local-uuid' }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'local-uuid', hostId: 'local' }) + expect(reposRemove).not.toHaveBeenCalled() expect(repoRmCalls()).toEqual([]) expect(remainingRepoIds(store)).toEqual(['env-a-uuid']) }) diff --git a/src/renderer/src/store/slices/repos-host-identity-routing.test.ts b/src/renderer/src/store/slices/repos-host-identity-routing.test.ts index d04028e44f7f..a67543a705ab 100644 --- a/src/renderer/src/store/slices/repos-host-identity-routing.test.ts +++ b/src/renderer/src/store/slices/repos-host-identity-routing.test.ts @@ -404,13 +404,13 @@ describe('repo slice host identity routing', () => { it('allows two removals of the same owner row to overlap', async () => { const { promise: firstRemoval, resolve: resolveFirstRemoval } = Promise.withResolvers() const { promise: secondRemoval, resolve: resolveSecondRemoval } = Promise.withResolvers() - reposRemove.mockReturnValueOnce(firstRemoval).mockReturnValueOnce(secondRemoval) + reposRemoveForHost.mockReturnValueOnce(firstRemoval).mockReturnValueOnce(secondRemoval) const store = createTestStore() store.setState({ repos: [localDuplicate] }) const first = store.getState().removeProject(localDuplicate.id, { hostId: 'local' }) const second = store.getState().removeProject(localDuplicate.id, { hostId: 'local' }) - expect(reposRemove).toHaveBeenCalledTimes(2) + expect(reposRemoveForHost).toHaveBeenCalledTimes(2) resolveSecondRemoval() resolveFirstRemoval() diff --git a/src/renderer/src/store/slices/repos-project-groups-delete.test.ts b/src/renderer/src/store/slices/repos-project-groups-delete.test.ts index cf84819499a6..5e793f03a054 100644 --- a/src/renderer/src/store/slices/repos-project-groups-delete.test.ts +++ b/src/renderer/src/store/slices/repos-project-groups-delete.test.ts @@ -191,8 +191,9 @@ describe('project group deletion store routing', () => { failedProjectRemovals: [] }) - expect(reposRemove).toHaveBeenCalledWith({ repoId: 'direct' }) - expect(reposRemove).toHaveBeenCalledWith({ repoId: 'nested' }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'direct', hostId: 'local' }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: 'nested', hostId: 'local' }) + expect(reposRemove).not.toHaveBeenCalled() expect(store.getState().repos).toEqual([siblingRepo]) }) @@ -234,7 +235,11 @@ describe('project group deletion store routing', () => { repoId: 'shared', hostId: 'local' }) - expect(reposRemove).toHaveBeenCalledWith({ repoId: 'shared' }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ + repoId: 'shared', + hostId: 'ssh:ssh-1' + }) + expect(reposRemove).not.toHaveBeenCalled() expect(store.getState().repos).toEqual([]) }) @@ -265,7 +270,7 @@ describe('project group deletion store routing', () => { it('reports project removal failures by comparing store state after removeProject', async () => { const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) - reposRemove.mockImplementation(async ({ repoId }: { repoId: string }) => { + reposRemoveForHost.mockImplementation(async ({ repoId }: { repoId: string }) => { if (repoId === 'nested') { throw new Error('remove failed') } diff --git a/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts b/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts index 7606c2b1511e..551e4363ee7a 100644 --- a/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts +++ b/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts @@ -29,7 +29,7 @@ beforeEach(() => { ptyKill.mockReset() vi.stubGlobal('window', { api: { - repos: { remove: reposRemove }, + repos: { removeForHost: reposRemove }, pty: { kill: ptyKill }, runtimeEnvironments: { call: vi.fn() } } diff --git a/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts b/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts index f7dca6594ae5..5fd6936eab7e 100644 --- a/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts +++ b/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts @@ -42,6 +42,7 @@ export const reposPickFolder: Mock = vi.fn() export const reposClone: Mock = vi.fn() export const reposCloneRemote: Mock = vi.fn() export const reposRemove: Mock = vi.fn() +export const reposRemoveForHost: Mock = vi.fn() export const reposUpdate: Mock = vi.fn() export const reposReorder: Mock = vi.fn() export const reposReorderForHost: Mock = vi.fn() @@ -73,6 +74,7 @@ export function installReposRuntimeRoutingHarness(): void { reposClone.mockReset() reposCloneRemote.mockReset() reposRemove.mockReset() + reposRemoveForHost.mockReset() reposUpdate.mockReset() reposReorder.mockReset() reposReorderForHost.mockReset() @@ -102,6 +104,7 @@ export function installReposRuntimeRoutingHarness(): void { cloneRemote: reposCloneRemote, pickFolder: reposPickFolder, remove: reposRemove, + removeForHost: reposRemoveForHost, update: reposUpdate, reorder: reposReorder, reorderForHost: reposReorderForHost diff --git a/src/renderer/src/store/slices/repos-stale-fetch.test.ts b/src/renderer/src/store/slices/repos-stale-fetch.test.ts index 86093114ab81..8993c823ec05 100644 --- a/src/renderer/src/store/slices/repos-stale-fetch.test.ts +++ b/src/renderer/src/store/slices/repos-stale-fetch.test.ts @@ -61,7 +61,7 @@ beforeEach(() => { api: { repos: { list: reposList, - remove: vi.fn().mockResolvedValue(undefined) + removeForHost: vi.fn().mockResolvedValue(undefined) } } }) diff --git a/src/renderer/src/store/slices/repos.test.ts b/src/renderer/src/store/slices/repos.test.ts index bbec0a1e4efd..bd253952e22d 100644 --- a/src/renderer/src/store/slices/repos.test.ts +++ b/src/renderer/src/store/slices/repos.test.ts @@ -19,6 +19,7 @@ import { reposList, reposPickFolder, reposRemove, + reposRemoveForHost, reposReorder, reposUpdate, runtimeEnvironmentCall, @@ -690,7 +691,7 @@ describe('repo slice runtime routing', () => { expect(store.getState().repos).toEqual([]) expect(store.getState().activeRepoId).toBeNull() - expect(reposRemove).toHaveBeenCalledWith({ repoId: sshRepo.id }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: sshRepo.id, hostId: 'ssh:ssh-1' }) expect(runtimeEnvironmentCall).not.toHaveBeenCalled() }) @@ -711,7 +712,7 @@ describe('repo slice runtime routing', () => { expect(store.getState().repos).toEqual([localRepo]) expect(store.getState().lastVisitedAtByWorktreeId).toEqual({ [localWorktreeId]: 200 }) - expect(reposRemove).toHaveBeenCalledWith({ repoId: sshRepo.id }) + expect(reposRemoveForHost).toHaveBeenCalledWith({ repoId: sshRepo.id, hostId: 'ssh:ssh-1' }) }) it('drops persisted visit timestamps for removed unhydrated runtime repos', async () => { diff --git a/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts b/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts index 421e5b6f972e..36bd54dff772 100644 --- a/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts +++ b/src/renderer/src/store/slices/sparse-presets-repo-removal-purge-leak.test.ts @@ -22,7 +22,7 @@ beforeEach(() => { reposRemove.mockReset().mockResolvedValue(undefined) vi.stubGlobal('window', { api: { - repos: { remove: reposRemove }, + repos: { removeForHost: reposRemove }, pty: { kill: vi.fn() }, runtimeEnvironments: { call: vi.fn() } } diff --git a/src/renderer/src/store/slices/store-session-cascades.test.ts b/src/renderer/src/store/slices/store-session-cascades.test.ts index a229a223b36e..c3c4c10fd7da 100644 --- a/src/renderer/src/store/slices/store-session-cascades.test.ts +++ b/src/renderer/src/store/slices/store-session-cascades.test.ts @@ -20,7 +20,7 @@ const mockApi = createStoreSessionMockApi() describe('removeProject cascade', () => { beforeEach(() => { vi.clearAllMocks() - mockApi.repos.remove.mockResolvedValue(undefined) + mockApi.repos.removeForHost.mockResolvedValue(undefined) mockApi.pty.kill.mockResolvedValue(undefined) }) diff --git a/src/renderer/src/store/slices/store-session-test-harness.ts b/src/renderer/src/store/slices/store-session-test-harness.ts index 6ef35f6360ed..5840acbe30b6 100644 --- a/src/renderer/src/store/slices/store-session-test-harness.ts +++ b/src/renderer/src/store/slices/store-session-test-harness.ts @@ -14,7 +14,14 @@ type UsageScannerMocks = { export type StoreSessionMockApi = { worktrees: { list: Mock; create: Mock; remove: Mock; updateMeta: Mock } - repos: { list: Mock; add: Mock; remove: Mock; update: Mock; pickFolder: Mock } + repos: { + list: Mock + add: Mock + remove: Mock + removeForHost: Mock + update: Mock + pickFolder: Mock + } pty: { kill: Mock } gh: { prForBranch: Mock; issue: Mock } settings: { get: Mock; set: Mock } @@ -38,6 +45,7 @@ export function createStoreSessionMockApi(): StoreSessionMockApi { list: vi.fn().mockResolvedValue([]), add: vi.fn().mockResolvedValue({}), remove: vi.fn().mockResolvedValue(undefined), + removeForHost: vi.fn().mockResolvedValue(undefined), update: vi.fn().mockResolvedValue({}), pickFolder: vi.fn().mockResolvedValue(null) }, diff --git a/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts b/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts index de8fe2dbf693..90bfb2e773cb 100644 --- a/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts +++ b/src/renderer/src/store/slices/workspace-chat-draft-removal.test.ts @@ -24,7 +24,7 @@ vi.mock('@/lib/agent-status', async (importOriginal) => { const mockApi = { worktrees: { list: vi.fn(), remove: vi.fn(), updateMeta: vi.fn().mockResolvedValue({}) }, - repos: { remove: vi.fn() }, + repos: { remove: vi.fn(), removeForHost: vi.fn() }, folderWorkspaces: { delete: vi.fn() }, pty: { kill: vi.fn() }, runtimeEnvironments: { call: vi.fn().mockResolvedValue({ ok: true, result: {} }) } @@ -106,7 +106,7 @@ describe('workspace chat drafts on removal', () => { beforeEach(() => { vi.clearAllMocks() mockApi.worktrees.remove.mockResolvedValue(undefined) - mockApi.repos.remove.mockResolvedValue(undefined) + mockApi.repos.removeForHost.mockResolvedValue(undefined) clearNativeChatComposerDraftsForTests() })