From cd7cf2fd17b8d30b4453e21ab865ca249394f417 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 10 Aug 2026 13:00:54 -0700 Subject: [PATCH] fix(sidebar): fail closed on ambiguous folder owners --- issue-12532-reduced-candidate-report.md | 59 ++-- issue-12532-reduced-retention-table.md | 60 ++-- src/main/ipc/filesystem-auth.test.ts | 28 ++ src/main/ipc/filesystem-auth.ts | 325 +++++++++++------- src/main/ipc/repos-remote.test.ts | 26 ++ src/main/ipc/repos.ts | 17 +- src/main/persistence.test.ts | 73 ++++ src/main/persistence.ts | 63 ++-- .../folder-workspace-path-status.test.ts | 65 ++++ .../folder-workspace-path-status.ts | 15 +- src/preload/api-types.ts | 3 +- .../sidebar/project-group-sidebar-identity.ts | 3 +- .../sidebar/worktree-list-groups.ts | 12 +- .../slices/project-group-removal-targets.ts | 28 +- ...-same-id-folder-workspace-identity.test.ts | 102 +++--- src/renderer/src/store/slices/repos.ts | 122 ++++--- src/renderer/src/store/slices/worktrees.ts | 210 ++--------- src/shared/folder-workspaces.ts | 25 +- src/shared/types.ts | 2 +- src/shared/workspace-scope.test.ts | 87 ----- src/shared/workspace-scope.ts | 102 +----- 21 files changed, 695 insertions(+), 732 deletions(-) delete mode 100644 src/shared/workspace-scope.test.ts diff --git a/issue-12532-reduced-candidate-report.md b/issue-12532-reduced-candidate-report.md index d12a4d6be14..e6a5a197039 100644 --- a/issue-12532-reduced-candidate-report.md +++ b/issue-12532-reduced-candidate-report.md @@ -1,29 +1,46 @@ -# Reduced #12532 owner-identity candidate +# #12532 reduced candidate final review + +## Verdict + +**NOT CLEAN.** The reviewed candidate `d83f9aae932c2d0eddd46e70cfd03e0cb78ac1fa` contained correctness, scope, and bounded-performance defects. Issue-scoped fixes are committed locally for another fresh review; PR #13599 was not pushed or retargeted. ## Gate -| Metric | Limit | Actual | -| --- | --- | --- | -| Total files | ≤45 | **28** | -| Production | ≤28 | **15** | -| Tests | — | **12** | -| Docs | — | **1** retention table | + +| Metric | Limit | Actual | +| ----------- | ----- | ------ | +| Total files | ≤45 | **26** | +| Production | ≤28 | **13** | +| Tests | — | **11** | +| Docs | — | **2** | ## SHAs -| Role | SHA | -| --- | --- | -| Base (#13611) | `d0271cb46f2871c201f39c51e35356efd4658eac` | -| Archived broad 66-file tip | `7c64936ce7` / docs `39d2585a90` on `archive/issue-12532-broad-66file-7c64936-20260810-122250` | -| Reduced candidate | `253f8c3f9e48dfec505d7ec96e104f099cf365a1` on `brennanb2025/issue-12532-owner-identity-narrow` (unpushed) | -## Retention table -`issue-12532-reduced-retention-table.md` +| Role | SHA | +| --------------------- | ---------------------------------------------------------------------------------------------- | +| Base (#13611) | `d0271cb46f2871c201f39c51e35356efd4658eac` | +| Reviewed candidate | `d83f9aae932c2d0eddd46e70cfd03e0cb78ac1fa` | +| Candidate code commit | `253f8c3f9e48dfec505d7ec96e104f099cf365a1` | +| Archived broad tip | `7c64936ce7` / docs `39d2585a90` on `archive/issue-12532-broad-66file-7c64936-20260810-122250` | + +## Findings fixed + +- Removed owner-qualified folder identity propagation from `WorkspaceScope`, worktree/tab/session maps, active workspace IDs, and editor/file routing. Unambiguous folder activation keeps `folder:folder-workspace-1`; same-id ambiguity fails closed. +- Added optional owner selectors only at folder CRUD and path-status preload/IPC/persistence seams. Legacy unambiguous calls keep their prior payload shape, while ambiguous mutations require an exact owner. +- Made persistence folder lookup/update/delete and folder path-status selection fail closed on bare same-id ambiguity without purging a surviving sibling's shared bare session state. +- Scoped project-group removal, move, path auth, and sidebar row/collapse identity by owner without changing unrelated repo/worktree IDs or #13611 header geometry. +- Replaced quadratic filesystem-auth subtree materialization with a linear owner-indexed aggregation plus sorted path indexes; cycles fail closed and a 4,096-level adversarial chain remains bounded. +- Removed the sidebar's per-row full catalog scan by precomputing ambiguous group IDs. +- Corrected owner-qualified folder path-status cache eviction to use the same canonical identity codec as cache creation. ## Validation -- Focused + legacy identity/sticky suite: **1110 passed** (20 files) -- typecheck web/node: pass -- oxlint production files: 0 errors -- max-lines ratchet / reliability gates: pass -- #13611 virtual-rows / host-section-rows: **unchanged** -## Remaining QA -Independent review; multi-host UI smoke for same-id collapse/CRUD; full monorepo CI optional. +- Focused identity, CRUD, path-status, auth, persistence, activity/session, dashboard, and legacy suites: **866 passed** across **23 unique files**; the corrected owner-cache fixture was rerun separately (**5 passed**). +- Node `v24.19.0` node/CLI/web typechecks: pass. +- Changed-code quality: 0 findings; max-lines ratchet and reliability gates: pass. +- `git diff --check` and formatting: pass. +- Filesystem-auth adversarial 4,096-level chain and cycle fixtures: pass. +- #13611 `worktree-list-virtual-rows`, `host-section-rows`, and `WorktreeList` geometry: byte-identical to `d0271cb46f2871c201f39c51e35356efd4658eac`. + +## Remaining work + +Another independent reviewer must review the fixed local commit. Because this review found actionable defects, do not push or retarget PR #13599 yet; Electron and SSH multi-host QA remain post-review/post-push work. diff --git a/issue-12532-reduced-retention-table.md b/issue-12532-reduced-retention-table.md index 415131d0aa4..0d544eb60a2 100644 --- a/issue-12532-reduced-retention-table.md +++ b/issue-12532-reduced-retention-table.md @@ -1,7 +1,7 @@ # #12532 reduced candidate — retained production file table -**Base:** `d0271cb46f` (#13611 geometry locked) -**Archived broad tip:** `7c64936ce7` / docs `39d2585a90` on `archive/issue-12532-broad-66file-7c64936-20260810-122250` +**Base:** `d0271cb46f` (#13611 geometry locked) +**Archived broad tip:** `7c64936ce7` / docs `39d2585a90` on `archive/issue-12532-broad-66file-7c64936-20260810-122250` **Hard gate:** ≤45 total / ≤28 production ## Retention policy @@ -13,40 +13,38 @@ ## Retained production files -| # | File | Failing behavior without it | Why a narrower seam cannot absorb it | -| --- | --- | --- | --- | -| 1 | `src/shared/project-groups.ts` | Same bare `group.id` on local+SSH collapses into one row on normalize; subtree delete/update walks bare parent edges across hosts | Canonical owner identity + fail-closed `resolveProjectGroupOwner` must live in shared code used by store, IPC, auth | -| 2 | `src/shared/folder-workspaces.ts` | Same bare folder id dropped on normalize; row keys collide in multi-host catalogs | Catalog identity for folders is parallel to groups; must stay co-located with normalize | -| 3 | `src/shared/workspace-scope.ts` | No dual-read for `folder:folder-workspace-1` vs optional owner-qualified form; parse cannot recover owner segment | Session key codec is shared main↔renderer; dual-read helpers belong at the codec, not duplicated | -| 4 | `src/shared/types.ts` | `WorkspaceScope` cannot carry optional `ownerHostId` from parse | One-line additive field; no alternative without a parallel type | -| 5 | `src/shared/folder-workspace-path-status.ts` | Path-status requests cannot name owner → cross-owner cache/RPC mix | Request type is the IPC/renderer contract boundary | -| 6 | `src/renderer/src/store/slices/repos.ts` | `updateProjectGroup`/`deleteProjectGroup` map by bare id and replace/delete both hosts; folder delete purges wrong session; path-status routes by focus | Single store owns CRUD orchestration; IPC alone cannot fix optimistic local state | -| 7 | `src/renderer/src/store/slices/project-group-removal-targets.ts` | Contained-project removal enumerates bare subtree across hosts | Delete-with-projects path must use owner-scoped subtree before IPC | -| 8 | `src/main/ipc/repos.ts` | Main IPC drops optional `ownerHostId` → host store cannot disambiguate | Wire entry for project-group/folder selectors | -| 9 | `src/preload/api-types.ts` | Renderer types omit optional `ownerHostId` | Type contract for preload; additive only | -| 10 | `src/main/persistence.ts` | Host-local group update/delete by bare id mutates wrong row; session maps need dual-read/migrate for bare folder keys | Persistence is the durable store for groups/folders/sessions on this host | -| 11 | `src/main/ipc/filesystem-auth.ts` | Authorize uses bare group/folder membership → same-id remote folder can authorize local path (or fail open) | Security boundary must index by owner once per rebuild (linear) | -| 12 | `src/main/project-groups/folder-workspace-path-status.ts` | Path status resolves bare folder/group to wrong connection | Main path probe must fail-closed on ambiguous owner | -| 13 | `src/renderer/src/components/sidebar/worktree-list-groups.ts` | Same-id groups share collapse key `project-group:`; folder rows share list keys | Row model is where collapse/render keys are minted; virtualizer host stamp alone does not fix collapse Set | -| 14 | `src/renderer/src/components/sidebar/project-group-sidebar-identity.ts` | Call sites reimplement owner resolve inconsistently | Thin shared index for sidebar owner lookup (kept small; avoids bloating WorktreeList) | -| 15 | `src/renderer/src/store/slices/worktrees.ts` | `setActiveFolderWorkspace` always used bare `folder:` so same-id hosts share session maps; no dual-read migrate of `folder:folder-workspace-1` | Activation/session map ownership lives in worktrees slice; cannot dual-read tabs without this seam | +| # | File | Failing behavior without it | Why a narrower seam cannot absorb it | +| --- | ----------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | +| 1 | `src/shared/project-groups.ts` | Same bare `group.id` on local+SSH collapses into one row on normalize; subtree delete/update walks bare parent edges across hosts | Canonical owner identity + fail-closed `resolveProjectGroupOwner` must live in shared code used by store, IPC, auth | +| 2 | `src/shared/folder-workspaces.ts` | Same bare folder id dropped on normalize; row keys collide in multi-host catalogs | Catalog identity for folders is parallel to groups; must stay co-located with normalize | +| 3 | `src/shared/folder-workspace-path-status.ts` | Path-status requests cannot name owner → cross-owner cache/RPC mix | Request type is the IPC/renderer contract boundary | +| 4 | `src/renderer/src/store/slices/repos.ts` | Group/folder CRUD and path status can select or purge the wrong owner | Single store owns routing and optimistic catalog state | +| 5 | `src/renderer/src/store/slices/project-group-removal-targets.ts` | Contained-project removal enumerates bare subtree across hosts | Delete-with-projects needs the selected owner subtree before IPC | +| 6 | `src/main/ipc/repos.ts` | Main IPC drops optional `ownerHostId` | Wire entry for group/folder selectors | +| 7 | `src/preload/api-types.ts` | Renderer types omit optional `ownerHostId` | Additive preload contract boundary | +| 8 | `src/main/persistence.ts` | Host-local group/folder CRUD mutates the first bare-id row | Durable owner-aware mutation boundary | +| 9 | `src/main/ipc/filesystem-auth.ts` | Same-id remote folders can authorize local paths | Security boundary uses precomputed owner indexes | +| 10 | `src/main/project-groups/folder-workspace-path-status.ts` | Path status resolves bare folder/group to the wrong connection | Main path probe must fail closed on ambiguity | +| 11 | `src/renderer/src/components/sidebar/worktree-list-groups.ts` | Same-id groups share render and collapse keys | Row model mints both identities | +| 12 | `src/renderer/src/components/sidebar/project-group-sidebar-identity.ts` | Sidebar callers otherwise resolve owners inconsistently | One precomputed sidebar lookup index | +| 13 | `src/renderer/src/store/slices/worktrees.ts` | Bare same-id folder activation selects one owner while tabs still share `folder:` | A bounded guard fails closed without changing workspace/tab identity | -**Production count: 15** (≤28). WorktreeList left on base: collapse keys come from `worktree-list-groups`; store fail-closes bare ambiguous mutate; optional `ownerHostId` on APIs covers explicit callers/tests. +**Production count: 13** (≤28). WorktreeList and all header geometry stay on the dependency base; owner selectors stop at sidebar, store, IPC, persistence, path-status, and auth seams. ## Explicitly dropped vs broad 38-file tip (and why) -| Dropped | Reason | -| --- | --- | -| WorktreeCard / WorktreeContextMenu | Store fail-closed + optional owner covers delete; card can keep bare id when unambiguous | -| dashboard-snapshot-workspaces | Dual-read in session codec/store activation; dashboard not required for issue #12532 same-id group headers | -| worktrees.ts bulk dual-read | Prefer bare session keys when unambiguous; avoid global session routing rewrite | -| folder-workspace-session-owner, workspace-session-host-persistence, worktree-runtime-owner | Cascade from always-qualified keys; not needed if default stays bare | -| folder-workspace-worktree | Keep bare synthetic `folder:` when unambiguous | -| folder-workspace-connection, composer helpers/path-status | Not required for adversarial same-id group/folder CRUD | +| Dropped | Reason | +| --------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | +| WorktreeCard / WorktreeContextMenu | Store fail-closed + optional owner covers delete; card can keep bare id when unambiguous | +| dashboard-snapshot-workspaces | Dashboard identity is unrelated to project-group catalog ownership | +| worktrees.ts bulk dual-read | Prefer bare session keys when unambiguous; avoid global session routing rewrite | +| folder-workspace-session-owner, workspace-session-host-persistence, worktree-runtime-owner | Cascade from always-qualified keys; not needed if default stays bare | +| folder-workspace-worktree | Keep bare synthetic `folder:` when unambiguous | +| folder-workspace-connection, composer helpers/path-status | Not required for adversarial same-id group/folder CRUD | | project-group-header-drag/drop/contract, move-targets, header-section-boundaries, folder-reveal, host-filtering, rendered-order, virtual-rows | Geometry/drag cascade; #13611 host stamp already unique for virtualizer; drag can follow store owner later if proven | -| ui.ts draft owner field | Optional convenience; not required for fail-closed CRUD | -| repos-runtime-routing-fixture | Test-only support (count under tests if reintroduced) | +| ui.ts draft owner field | Optional convenience; not required for fail-closed CRUD | +| repos-runtime-routing-fixture | Test-only support (count under tests if reintroduced) | ## Test budget (focused only) -Same-id / fail-closed / dual-read / auth-index / path-status / owner update-delete / normalize — prefer **≤20** test files, no mass expectation rewrites of sticky geometry or bare-key consumers. +Same-id / fail-closed / auth-index / path-status / owner update-delete / normalize — no mass expectation rewrites of sticky geometry or bare-key consumers. diff --git a/src/main/ipc/filesystem-auth.test.ts b/src/main/ipc/filesystem-auth.test.ts index 6d8731bd3de..c1113426f37 100644 --- a/src/main/ipc/filesystem-auth.test.ts +++ b/src/main/ipc/filesystem-auth.test.ts @@ -348,6 +348,34 @@ describe('filesystem-auth path containment', () => { expect(elapsedMs).toBeLessThan(250) }) + it('bounds deep legacy folder hierarchies without materializing every subtree', () => { + const groupCount = 4_096 + const projectGroups = Array.from({ length: groupCount }, (_, index) => + makeProjectGroup({ + id: `group-${index}`, + parentPath: index === 0 ? '/legacy/root' : null, + parentGroupId: index === 0 ? null : `group-${index - 1}` + }) + ) + const repos = [ + { + ...repo, + id: 'remote-leaf', + path: '/outside/repo', + projectGroupId: `group-${groupCount - 1}`, + connectionId: 'builder' + } as Repo + ] + const store = makeStore(repos, { projectGroups, folderWorkspaces: [] }) + + const started = performance.now() + const allowed = isPathAllowed('/legacy/root', store) + const elapsedMs = performance.now() - started + + expect(allowed).toBe(false) + expect(elapsedMs).toBeLessThan(250) + }) + it.skipIf(process.platform === 'win32')( 'rejects missing descendants under a symlinked ancestor outside the repo', async () => { diff --git a/src/main/ipc/filesystem-auth.ts b/src/main/ipc/filesystem-auth.ts index d8cc22afbf0..7daa7bae61a 100644 --- a/src/main/ipc/filesystem-auth.ts +++ b/src/main/ipc/filesystem-auth.ts @@ -5,16 +5,15 @@ import { realpath } from 'node:fs/promises' import type { Store } from '../persistence' import { isRepoRoot, listRepoWorktrees } from '../repo-worktrees' import { computeWorkspaceRoot, getWorktreePathSettings } from './worktree-logic' -import { isPathInsideOrEqual } from '../../shared/cross-platform-path' +import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' import { buildProjectGroupOwnerIndex, getProjectGroupOwnerHostId, - getProjectGroupSubtreeIds, resolveFolderWorkspaceProjectGroup } from '../../shared/project-groups' import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' import type { ProjectGroup, Repo } from '../../shared/types' -import { getFolderWorkspaceCatalogOwnerHostId } from '../../shared/folder-workspaces' +import { resolveFolderWorkspaceCatalogOwnerHostId } from '../../shared/folder-workspaces' export const PATH_ACCESS_DENIED_MESSAGE = 'Access denied: path resolves outside allowed directories. If this blocks a legitimate workflow, please file a GitHub issue.' @@ -67,13 +66,18 @@ function getLocalRepos(store: Store) { type FolderAuthIndex = { projectGroupIndex: ReturnType - ownerGroupIdsByOwner: Map - reposByOwner: Map - reposByOwnerAndGroupId: Map - ownerSubtreeIds: Map>> - unscopedSubtreeIds: Map> - repos: readonly Repo[] - projectGroups: readonly ProjectGroup[] + groupMembership: ReadonlyMap + legacyGroupMembership: ReadonlyMap + allRepoPaths: readonly string[] + localOwnerRepoPaths: readonly string[] + unconnectedRepoPathsByOwner: ReadonlyMap +} + +type GroupMembershipSummary = { + repoCount: number + hasLocalOwner: boolean + hasUnconnectedRepo: boolean + unsafeCycle: boolean } function ownerGroupKey(ownerHostId: string, groupId: string): string { @@ -85,118 +89,183 @@ function buildFolderAuthIndex( repos: readonly Repo[] ): FolderAuthIndex { const projectGroupIndex = buildProjectGroupOwnerIndex(projectGroups) - const ownerGroupIdsByOwner = new Map() - const childrenByOwnerParent = new Map() + const parentByGroup = new Map() + const remainingChildren = new Map() + const groupMembership = new Map() + const legacyParentsByGroup = new Map>() + const legacyChildrenByGroup = new Map>() + const legacyGroupMembership = new Map() for (const group of projectGroups) { const ownerHostId = getProjectGroupOwnerHostId(group) - const ownerGroups = ownerGroupIdsByOwner.get(ownerHostId) ?? [] - ownerGroups.push(group) - ownerGroupIdsByOwner.set(ownerHostId, ownerGroups) + const key = ownerGroupKey(ownerHostId, group.id) + remainingChildren.set(key, 0) + groupMembership.set(key, emptyGroupMembershipSummary()) + if (!legacyGroupMembership.has(group.id)) { + legacyGroupMembership.set(group.id, emptyGroupMembershipSummary()) + } + if (group.parentGroupId) { + const parents = legacyParentsByGroup.get(group.id) ?? new Set() + parents.add(group.parentGroupId) + legacyParentsByGroup.set(group.id, parents) + const children = legacyChildrenByGroup.get(group.parentGroupId) ?? new Set() + children.add(group.id) + legacyChildrenByGroup.set(group.parentGroupId, children) + } + } + for (const group of projectGroups) { if (!group.parentGroupId) { continue } + const ownerHostId = getProjectGroupOwnerHostId(group) + const key = ownerGroupKey(ownerHostId, group.id) const parentKey = ownerGroupKey(ownerHostId, group.parentGroupId) - const children = childrenByOwnerParent.get(parentKey) ?? [] - children.push(group.id) - childrenByOwnerParent.set(parentKey, children) - } - - const ownerSubtreeIds = new Map>>() - for (const [ownerHostId, groups] of ownerGroupIdsByOwner) { - const subtreeByRoot = new Map>() - for (const group of groups) { - if (subtreeByRoot.has(group.id)) { - continue - } - const subtree = new Set() - const pending = [group.id] - while (pending.length > 0) { - const groupId = pending.pop()! - if (subtree.has(groupId)) { - continue - } - subtree.add(groupId) - for (const childId of childrenByOwnerParent.get(ownerGroupKey(ownerHostId, groupId)) ?? - []) { - pending.push(childId) - } - } - subtreeByRoot.set(group.id, subtree) - } - ownerSubtreeIds.set(ownerHostId, subtreeByRoot) - } - - const unscopedSubtreeIds = new Map>() - for (const group of projectGroups) { - if (!unscopedSubtreeIds.has(group.id)) { - unscopedSubtreeIds.set(group.id, getProjectGroupSubtreeIds(projectGroups, group.id)) + if (remainingChildren.has(parentKey)) { + parentByGroup.set(key, parentKey) + remainingChildren.set(parentKey, (remainingChildren.get(parentKey) ?? 0) + 1) } } - const reposByOwner = new Map() - const reposByOwnerAndGroupId = new Map() + const allRepoPaths: string[] = [] + const localOwnerRepoPaths: string[] = [] + const unconnectedRepoPathsByOwner = new Map() for (const repo of repos) { const ownerHostId = getRepoExecutionHostId(repo) - const ownerRepos = reposByOwner.get(ownerHostId) ?? [] - ownerRepos.push(repo) - reposByOwner.set(ownerHostId, ownerRepos) + const normalizedPath = normalizeRuntimePathForComparison(repo.path) + allRepoPaths.push(normalizedPath) + if (ownerHostId === LOCAL_EXECUTION_HOST_ID) { + localOwnerRepoPaths.push(normalizedPath) + } + if (!repo.connectionId) { + const ownerPaths = unconnectedRepoPathsByOwner.get(ownerHostId) ?? [] + ownerPaths.push(normalizedPath) + unconnectedRepoPathsByOwner.set(ownerHostId, ownerPaths) + } if (typeof repo.projectGroupId === 'string') { const key = ownerGroupKey(ownerHostId, repo.projectGroupId) - const groupRepos = reposByOwnerAndGroupId.get(key) ?? [] - groupRepos.push(repo) - reposByOwnerAndGroupId.set(key, groupRepos) - } - } - - return { - projectGroupIndex, - ownerGroupIdsByOwner, - reposByOwner, - reposByOwnerAndGroupId, - ownerSubtreeIds, - unscopedSubtreeIds, - repos, - projectGroups - } -} - -function collectOwnerGroupMembershipRepos( - index: FolderAuthIndex, - projectGroupId: string, - ownerHostId: string -): Repo[] { - const groupIds = - index.ownerSubtreeIds.get(ownerHostId)?.get(projectGroupId) ?? new Set([projectGroupId]) - const seen = new Set() - const candidates: Repo[] = [] - for (const groupId of groupIds) { - for (const repo of index.reposByOwnerAndGroupId.get(ownerGroupKey(ownerHostId, groupId)) ?? - []) { - if (!seen.has(repo)) { - seen.add(repo) - candidates.push(repo) + const summary = groupMembership.get(key) + if (summary) { + summary.repoCount++ + summary.hasLocalOwner ||= ownerHostId === LOCAL_EXECUTION_HOST_ID + summary.hasUnconnectedRepo ||= !repo.connectionId + } + const legacySummary = legacyGroupMembership.get(repo.projectGroupId) + if (legacySummary) { + legacySummary.repoCount++ + legacySummary.hasLocalOwner ||= ownerHostId === LOCAL_EXECUTION_HOST_ID + legacySummary.hasUnconnectedRepo ||= !repo.connectionId } } } - return candidates -} -function collectOwnerPathMembershipRepos( - index: FolderAuthIndex, - folderPath: string, - ownerHostId: string, - exclude: ReadonlySet -): Repo[] { - const candidates: Repo[] = [] - for (const repo of index.reposByOwner.get(ownerHostId) ?? []) { - if (exclude.has(repo)) { + const pending = [...remainingChildren] + .filter(([, childCount]) => childCount === 0) + .map(([key]) => key) + let processed = 0 + while (pending.length > 0) { + const key = pending.pop()! + processed++ + const parentKey = parentByGroup.get(key) + if (!parentKey) { continue } - if (isPathInsideOrEqual(folderPath, repo.path)) { - candidates.push(repo) + mergeGroupMembership(groupMembership.get(parentKey)!, groupMembership.get(key)!) + const nextChildCount = (remainingChildren.get(parentKey) ?? 1) - 1 + remainingChildren.set(parentKey, nextChildCount) + if (nextChildCount === 0) { + pending.push(parentKey) } } - return candidates + if (processed !== groupMembership.size) { + for (const [key, childCount] of remainingChildren) { + if (childCount > 0) { + groupMembership.get(key)!.unsafeCycle = true + } + } + } + + const legacyRemainingChildren = new Map( + [...legacyGroupMembership.keys()].map((groupId) => [ + groupId, + legacyChildrenByGroup.get(groupId)?.size ?? 0 + ]) + ) + const legacyPending = [...legacyRemainingChildren] + .filter(([, childCount]) => childCount === 0) + .map(([groupId]) => groupId) + let legacyProcessed = 0 + while (legacyPending.length > 0) { + const groupId = legacyPending.pop()! + legacyProcessed++ + for (const parentId of legacyParentsByGroup.get(groupId) ?? []) { + const parentSummary = legacyGroupMembership.get(parentId) + if (!parentSummary) { + continue + } + mergeGroupMembership(parentSummary, legacyGroupMembership.get(groupId)!) + const nextChildCount = (legacyRemainingChildren.get(parentId) ?? 1) - 1 + legacyRemainingChildren.set(parentId, nextChildCount) + if (nextChildCount === 0) { + legacyPending.push(parentId) + } + } + } + if (legacyProcessed !== legacyGroupMembership.size) { + for (const [groupId, childCount] of legacyRemainingChildren) { + if (childCount > 0) { + legacyGroupMembership.get(groupId)!.unsafeCycle = true + } + } + } + + allRepoPaths.sort() + localOwnerRepoPaths.sort() + for (const paths of unconnectedRepoPathsByOwner.values()) { + paths.sort() + } + return { + projectGroupIndex, + groupMembership, + legacyGroupMembership, + allRepoPaths, + localOwnerRepoPaths, + unconnectedRepoPathsByOwner + } +} + +function emptyGroupMembershipSummary(): GroupMembershipSummary { + return { + repoCount: 0, + hasLocalOwner: false, + hasUnconnectedRepo: false, + unsafeCycle: false + } +} + +function mergeGroupMembership(target: GroupMembershipSummary, child: GroupMembershipSummary): void { + target.repoCount += child.repoCount + target.hasLocalOwner ||= child.hasLocalOwner + target.hasUnconnectedRepo ||= child.hasUnconnectedRepo + target.unsafeCycle ||= child.unsafeCycle +} + +function hasIndexedPathInside(rootPath: string, sortedPaths: readonly string[]): boolean { + const root = normalizeRuntimePathForComparison(rootPath) + let low = 0 + let high = sortedPaths.length + while (low < high) { + const middle = (low + high) >>> 1 + if (sortedPaths[middle] < root) { + low = middle + 1 + } else { + high = middle + } + } + const candidate = sortedPaths[low] + if (!candidate) { + return false + } + const prefix = root === '/' || /^[a-z]:\/$/i.test(root) ? root : `${root}/` + return candidate === root || candidate.startsWith(prefix) } function isRemoteOnlyFolderScopeWithIndex( @@ -209,42 +278,34 @@ function isRemoteOnlyFolderScopeWithIndex( if (ownerHostId !== LOCAL_EXECUTION_HOST_ID) { return true } + const summary = + index.groupMembership.get(ownerGroupKey(ownerHostId, projectGroupId)) ?? + emptyGroupMembershipSummary() + if (summary.unsafeCycle) { + return true + } if (inferLegacyOwner) { - const groupIds = index.unscopedSubtreeIds.get(projectGroupId) ?? new Set([projectGroupId]) - let sawLegacyCandidate = false - let sawLocalLegacyCandidate = false - for (const repo of index.repos) { - const inGroup = typeof repo.projectGroupId === 'string' && groupIds.has(repo.projectGroupId) - // Why: only pay path reads when group membership alone cannot classify the scope. - if (!inGroup && !isPathInsideOrEqual(folderPath, repo.path)) { - continue - } - sawLegacyCandidate = true - if (getRepoExecutionHostId(repo) === LOCAL_EXECUTION_HOST_ID) { - sawLocalLegacyCandidate = true - break - } + const legacySummary = + index.legacyGroupMembership.get(projectGroupId) ?? emptyGroupMembershipSummary() + if (legacySummary.unsafeCycle) { + return true } - if (sawLegacyCandidate && !sawLocalLegacyCandidate) { + const hasAnyCandidate = + legacySummary.repoCount > 0 || hasIndexedPathInside(folderPath, index.allRepoPaths) + const hasLocalCandidate = + legacySummary.hasLocalOwner || hasIndexedPathInside(folderPath, index.localOwnerRepoPaths) + if (hasAnyCandidate && !hasLocalCandidate) { return true } } - const groupCandidates = collectOwnerGroupMembershipRepos(index, projectGroupId, ownerHostId) - if (groupCandidates.length === 0) { + if (summary.repoCount === 0) { // Why: empty membership means no remote-linked evidence; authorize local folder scopes without path scans. return false } - if (groupCandidates.some((repo) => !repo.connectionId)) { + if (summary.hasUnconnectedRepo) { return false } - // Why: path containment only matters when every group-linked repo is remote-linked. - const pathCandidates = collectOwnerPathMembershipRepos( - index, - folderPath, - ownerHostId, - new Set(groupCandidates) - ) - if (pathCandidates.some((repo) => !repo.connectionId)) { + if (hasIndexedPathInside(folderPath, index.unconnectedRepoPathsByOwner.get(ownerHostId) ?? [])) { return false } return true @@ -276,10 +337,16 @@ function getLocalFolderScopeRoots(store: Store): string[] { } for (const workspace of scopeStore.getFolderWorkspaces?.() ?? []) { const group = resolveFolderWorkspaceProjectGroup(index.projectGroupIndex, workspace) - // Why: narrow Store doubles may omit projectGroups; folder roots stay additive from the workspace catalog. + if (!group && index.projectGroupIndex.byId.has(workspace.projectGroupId)) { + continue + } + // Why: narrow Store doubles may omit groups; folder roots stay additive from the workspace catalog. const ownerHostId = group ? getProjectGroupOwnerHostId(group) - : getFolderWorkspaceCatalogOwnerHostId(workspace, projectGroups) + : resolveFolderWorkspaceCatalogOwnerHostId(workspace, projectGroups) + if (!ownerHostId) { + continue + } if ( !isRemoteOnlyFolderScopeWithIndex( index, diff --git a/src/main/ipc/repos-remote.test.ts b/src/main/ipc/repos-remote.test.ts index 4fe5a780e55..ab13ce5bb58 100644 --- a/src/main/ipc/repos-remote.test.ts +++ b/src/main/ipc/repos-remote.test.ts @@ -38,6 +38,8 @@ const { updateProjectGroup: vi.fn(), deleteProjectGroup: vi.fn(), moveProjectToGroup: vi.fn(), + updateFolderWorkspace: vi.fn(), + removeFolderWorkspace: vi.fn(), getSshTarget: vi.fn() }, mockGitProvider: { @@ -179,6 +181,8 @@ describe('projectGroups IPC validation', () => { mockStore.updateProjectGroup.mockReset() mockStore.deleteProjectGroup.mockReset() mockStore.moveProjectToGroup.mockReset() + mockStore.updateFolderWorkspace.mockReset() + mockStore.removeFolderWorkspace.mockReset() mockStore.addRepo.mockReset() mockStore.getProjects.mockReset().mockReturnValue([]) mockStore.getProjectHostSetups.mockReset().mockReturnValue([]) @@ -412,6 +416,28 @@ describe('projectGroups IPC validation', () => { expect(mockStore.moveProjectToGroup).not.toHaveBeenCalled() }) + it('forwards optional folder workspace mutation ownership to persistence', () => { + mockStore.updateFolderWorkspace.mockReturnValue({ id: 'same-folder' }) + mockStore.removeFolderWorkspace.mockReturnValue(true) + + handlers.get('folderWorkspaces:update')!(null, { + folderWorkspaceId: 'same-folder', + ownerHostId: 'ssh:builder', + updates: { name: 'Remote' } + }) + handlers.get('folderWorkspaces:delete')!(null, { + folderWorkspaceId: 'same-folder', + ownerHostId: 'local' + }) + + expect(mockStore.updateFolderWorkspace).toHaveBeenCalledWith( + 'same-folder', + { name: 'Remote' }, + 'ssh:builder' + ) + expect(mockStore.removeFolderWorkspace).toHaveBeenCalledWith('same-folder', 'local') + }) + it('scans nested repositories over a connected SSH filesystem', async () => { mockGitProvider.isGitRepoAsync.mockImplementation(async (path: string) => ({ isRepo: path === '/srv/platform/api', diff --git a/src/main/ipc/repos.ts b/src/main/ipc/repos.ts index 7e7812e4719..444d3f69d3c 100644 --- a/src/main/ipc/repos.ts +++ b/src/main/ipc/repos.ts @@ -949,6 +949,7 @@ const FolderWorkspaceCreateArgs = z const FolderWorkspaceUpdateArgs = z.object({ folderWorkspaceId: z.string().min(1), + ownerHostId: ProjectGroupOwnerHostId.optional(), updates: z .object({ name: z.string().optional(), @@ -971,13 +972,15 @@ const FolderWorkspaceUpdateArgs = z.object({ }) const FolderWorkspaceSelectorArgs = z.object({ - folderWorkspaceId: z.string().min(1) + folderWorkspaceId: z.string().min(1), + ownerHostId: ProjectGroupOwnerHostId.optional() }) const FolderWorkspacePathStatusArgs = z.discriminatedUnion('scope', [ z.object({ scope: z.literal('folder-workspace'), - folderWorkspaceId: z.string().min(1) + folderWorkspaceId: z.string().min(1), + ownerHostId: ProjectGroupOwnerHostId.optional() }), z.object({ scope: z.literal('project-group'), @@ -1562,7 +1565,7 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v typeof args.updates.folderPath === 'string' && args.updates.folderPath.trim().length > 0 ) { - const workspace = store.getFolderWorkspace(args.folderWorkspaceId) + const workspace = store.getFolderWorkspace(args.folderWorkspaceId, args.ownerHostId) if (!workspace) { return null } @@ -1587,7 +1590,11 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v ) assertFolderWorkspacePathUsable(status) } - const updated = store.updateFolderWorkspace(args.folderWorkspaceId, args.updates) + const updated = store.updateFolderWorkspace( + args.folderWorkspaceId, + args.updates, + args.ownerHostId + ) if (updated) { notifyReposChanged(mainWindow) } @@ -1601,7 +1608,7 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v rawArgs, 'invalid_folder_workspace_delete_args' ) - const deleted = store.removeFolderWorkspace(args.folderWorkspaceId) + const deleted = store.removeFolderWorkspace(args.folderWorkspaceId, args.ownerHostId) if (deleted) { notifyReposChanged(mainWindow) } diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 1efa5e1a690..30de36d17a4 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -3736,6 +3736,79 @@ describe('Store', () => { ) }) + it('updates and removes same-id folder workspaces only for the selected owner', async () => { + const folderWorkspace = { + id: 'same-folder', + projectGroupId: 'same-id', + name: 'Folder', + folderPath: '/folder', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 1, + lastActivityAt: 1, + createdAt: 1, + updatedAt: 1 + } + writeDataFile({ + schemaVersion: 1, + repos: [], + worktreeMeta: {}, + settings: {}, + ui: {}, + githubCache: { pr: {}, issue: {} }, + projectGroups: [ + { + id: 'same-id', + name: 'Local', + parentPath: '/local', + connectionId: null, + parentGroupId: null, + createdFrom: 'folder-scan', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + }, + { + id: 'same-id', + name: 'SSH', + parentPath: '/remote', + connectionId: 'builder', + parentGroupId: null, + createdFrom: 'folder-scan', + tabOrder: 1, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + } + ], + folderWorkspaces: [ + { ...folderWorkspace, name: 'Local', folderPath: '/local/folder', connectionId: null }, + { + ...folderWorkspace, + name: 'SSH', + folderPath: '/remote/folder', + connectionId: 'builder' + } + ] + }) + const store = await createStore() + + expect(store.getFolderWorkspace('same-folder')).toBeUndefined() + expect(store.updateFolderWorkspace('same-folder', { name: 'Ambiguous' })).toBeNull() + expect( + store.updateFolderWorkspace('same-folder', { name: 'Remote' }, 'ssh:builder')?.name + ).toBe('Remote') + expect(store.removeFolderWorkspace('same-folder')).toBe(false) + expect(store.removeFolderWorkspace('same-folder', 'local')).toBe(true) + expect(store.getFolderWorkspace('same-folder', 'ssh:builder')?.name).toBe('Remote') + }) + it('adapts flat folder-scan groups into sparse nested folder scopes on load', async () => { writeDataFile({ schemaVersion: 1, diff --git a/src/main/persistence.ts b/src/main/persistence.ts index df56de55ffa..864a3e3ea3a 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -263,6 +263,7 @@ import { normalizeBrowserPageZoomLevel } from '../shared/browser-page-zoom' import { persistedUIValuesEqual } from '../shared/persisted-ui-equality' import { ActiveViewPreference } from './active-view-preference' import { + resolveFolderWorkspaceCatalogOwnerHostId, normalizeFolderWorkspaceName, normalizeFolderWorkspaces } from '../shared/folder-workspaces' @@ -4488,10 +4489,20 @@ export class Store { ? { ...repo, projectGroupId: null } : repo }) - const removedFolderWorkspaceKeys = new Set() - for (const workspace of this.state.folderWorkspaces ?? []) { + const folderWorkspaces = this.state.folderWorkspaces ?? [] + const removedFolderWorkspaces = folderWorkspaces.filter((workspace) => { const group = resolveFolderWorkspaceProjectGroup(projectGroupIndex, workspace) - if (group && deletedGroupIdentities.has(getProjectGroupOwnerIdentity(group))) { + return group && deletedGroupIdentities.has(getProjectGroupOwnerIdentity(group)) + }) + this.state.folderWorkspaces = folderWorkspaces.filter( + (workspace) => !removedFolderWorkspaces.includes(workspace) + ) + const removedFolderWorkspaceKeys = new Set() + for (const workspace of removedFolderWorkspaces) { + const hasSameIdSibling = this.state.folderWorkspaces.some( + (candidate) => candidate.id === workspace.id + ) + if (!hasSameIdSibling) { removedFolderWorkspaceKeys.add(folderWorkspaceKey(workspace.id)) this.state.workspaceSession = removeWorkspaceSessionOwner( this.state.workspaceSession, @@ -4500,10 +4511,6 @@ export class Store { this.removeWorkspaceLineageForFolderParent(workspace.id) } } - this.state.folderWorkspaces = (this.state.folderWorkspaces ?? []).filter((workspace) => { - const group = resolveFolderWorkspaceProjectGroup(projectGroupIndex, workspace) - return !group || !deletedGroupIdentities.has(getProjectGroupOwnerIdentity(group)) - }) this.pruneMobileClientTabSelections((worktreeId) => removedFolderWorkspaceKeys.has(worktreeId)) this.scheduleSave() return true @@ -4515,8 +4522,15 @@ export class Store { ) } - getFolderWorkspace(id: string): FolderWorkspace | undefined { - return (this.state.folderWorkspaces ?? []).find((workspace) => workspace.id === id) + getFolderWorkspace(id: string, ownerHostId?: ExecutionHostId): FolderWorkspace | undefined { + const matches = (this.state.folderWorkspaces ?? []).filter( + (workspace) => + workspace.id === id && + (!ownerHostId || + resolveFolderWorkspaceCatalogOwnerHostId(workspace, this.state.projectGroups ?? []) === + ownerHostId) + ) + return matches.length === 1 ? matches[0] : undefined } createFolderWorkspace(input: { @@ -4598,9 +4612,10 @@ export class Store { | 'firstAgentMessageRenameError' | 'lastActivityAt' > - > + >, + ownerHostId?: ExecutionHostId ): FolderWorkspace | null { - const workspace = this.getFolderWorkspace(id) + const workspace = this.getFolderWorkspace(id, ownerHostId) if (!workspace) { return null } @@ -4675,20 +4690,22 @@ export class Store { return workspace } - removeFolderWorkspace(id: string): boolean { - const before = this.state.folderWorkspaces?.length ?? 0 - this.state.folderWorkspaces = (this.state.folderWorkspaces ?? []).filter( - (workspace) => workspace.id !== id - ) - if ((this.state.folderWorkspaces?.length ?? 0) === before) { + removeFolderWorkspace(id: string, ownerHostId?: ExecutionHostId): boolean { + const workspace = this.getFolderWorkspace(id, ownerHostId) + if (!workspace) { return false } - this.state.workspaceSession = removeWorkspaceSessionOwner( - this.state.workspaceSession, - folderWorkspaceKey(id) - )! - this.removeWorkspaceLineageForFolderParent(id) - this.pruneMobileClientTabSelections((worktreeId) => worktreeId === folderWorkspaceKey(id)) + this.state.folderWorkspaces = (this.state.folderWorkspaces ?? []).filter( + (candidate) => candidate !== workspace + ) + if (!this.state.folderWorkspaces.some((candidate) => candidate.id === id)) { + this.state.workspaceSession = removeWorkspaceSessionOwner( + this.state.workspaceSession, + folderWorkspaceKey(id) + )! + this.removeWorkspaceLineageForFolderParent(id) + this.pruneMobileClientTabSelections((worktreeId) => worktreeId === folderWorkspaceKey(id)) + } this.scheduleSave() return true } diff --git a/src/main/project-groups/folder-workspace-path-status.test.ts b/src/main/project-groups/folder-workspace-path-status.test.ts index 8e88f26aba6..0b6833c65b9 100644 --- a/src/main/project-groups/folder-workspace-path-status.test.ts +++ b/src/main/project-groups/folder-workspace-path-status.test.ts @@ -67,6 +67,71 @@ it('resolves same-id project-group paths by exact owner', () => { ).toThrow('folder_workspace_path_scope_not_found') }) +it('resolves same-id folder-workspace paths by exact owner and rejects a bare selector', () => { + const local = makeGroup({ id: 'same-id', parentPath: '/local' }) + const ssh = makeGroup({ id: 'same-id', parentPath: '/remote', connectionId: 'builder' }) + const store = { + getRepos: () => [], + getProjectGroups: () => [local, ssh], + getFolderWorkspaces: () => [ + { + id: 'same-folder', + projectGroupId: 'same-id', + name: 'Local', + folderPath: '/local/folder', + connectionId: null, + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 1, + lastActivityAt: 1, + createdAt: 1, + updatedAt: 1 + }, + { + id: 'same-folder', + projectGroupId: 'same-id', + name: 'SSH', + folderPath: '/remote/folder', + connectionId: 'builder', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 2, + lastActivityAt: 1, + createdAt: 1, + updatedAt: 1 + } + ] + } + + expect( + resolveFolderWorkspaceStatusPath({ + store, + request: { + scope: 'folder-workspace', + folderWorkspaceId: 'same-folder', + ownerHostId: 'ssh:builder' + } + }) + ).toEqual({ + folderPath: '/remote/folder', + projectGroupId: 'same-id', + connectionId: 'builder', + ownerHostId: 'ssh:builder' + }) + expect(() => + resolveFolderWorkspaceStatusPath({ + store, + request: { scope: 'folder-workspace', folderWorkspaceId: 'same-folder' } + }) + ).toThrow('folder_workspace_path_scope_not_found') +}) + describe('folder workspace path status', () => { it('reports existing local directories and local files', async () => { const root = await mkdtemp(join(tmpdir(), 'orca-folder-status-')) diff --git a/src/main/project-groups/folder-workspace-path-status.ts b/src/main/project-groups/folder-workspace-path-status.ts index 0350cabff88..7422b7ea5fd 100644 --- a/src/main/project-groups/folder-workspace-path-status.ts +++ b/src/main/project-groups/folder-workspace-path-status.ts @@ -14,6 +14,7 @@ import { } from '../../shared/project-groups' import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import type { FolderWorkspace, ProjectGroup, Repo } from '../../shared/types' +import { resolveFolderWorkspaceCatalogOwnerHostId } from '../../shared/folder-workspaces' import type { IFilesystemProvider } from '../providers/types' type FolderWorkspacePathStatusStore = { @@ -194,14 +195,20 @@ export function resolveFolderWorkspaceStatusPath(args: { } } - const workspace = args.store - .getFolderWorkspaces?.() - .find((entry) => entry.id === request.folderWorkspaceId) + const matches = (args.store.getFolderWorkspaces?.() ?? []).filter( + (entry) => + entry.id === request.folderWorkspaceId && + (!request.ownerHostId || + resolveFolderWorkspaceCatalogOwnerHostId(entry, projectGroups) === request.ownerHostId) + ) + const workspace = matches.length === 1 ? matches[0] : null if (!workspace) { throw new Error('folder_workspace_path_scope_not_found') } const group = resolveFolderWorkspaceProjectGroup(projectGroupIndex, workspace) - // Why: fail closed only when the workspace row is missing; group may lag hydrate on Store doubles. + if (!group && projectGroupIndex.byId.has(workspace.projectGroupId)) { + throw new Error('folder_workspace_path_scope_not_found') + } const ownerHostId = getFolderWorkspaceProjectGroupOwnerHostId(workspace, projectGroupIndex) return { folderPath: workspace.folderPath, diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 77c00313891..10080b2f179 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1388,6 +1388,7 @@ export type PreloadApi = { }) => Promise update: (args: { folderWorkspaceId: string + ownerHostId?: ExecutionHostId updates: Partial< Pick< FolderWorkspace, @@ -1408,7 +1409,7 @@ export type PreloadApi = { > > }) => Promise - delete: (args: { folderWorkspaceId: string }) => Promise + delete: (args: { folderWorkspaceId: string; ownerHostId?: ExecutionHostId }) => Promise } sparsePresets: { list: (args: { repoId: string }) => Promise diff --git a/src/renderer/src/components/sidebar/project-group-sidebar-identity.ts b/src/renderer/src/components/sidebar/project-group-sidebar-identity.ts index 5afba9bdb32..1015c679e09 100644 --- a/src/renderer/src/components/sidebar/project-group-sidebar-identity.ts +++ b/src/renderer/src/components/sidebar/project-group-sidebar-identity.ts @@ -66,6 +66,7 @@ export function parseProjectGroupSidebarHeaderKey( export type ProjectGroupSidebarIndex = { byIdentity: ReadonlyMap byUnambiguousId: ReadonlyMap + ambiguousIds: ReadonlySet } export function buildProjectGroupSidebarIndex( @@ -90,7 +91,7 @@ export function buildProjectGroupSidebarIndex( byUnambiguousId.set(group.id, group) } } - return { byIdentity, byUnambiguousId } + return { byIdentity, byUnambiguousId, ambiguousIds } } export function findProjectGroupForSidebarOwner( diff --git a/src/renderer/src/components/sidebar/worktree-list-groups.ts b/src/renderer/src/components/sidebar/worktree-list-groups.ts index def461aabd5..26fe90abdeb 100644 --- a/src/renderer/src/components/sidebar/worktree-list-groups.ts +++ b/src/renderer/src/components/sidebar/worktree-list-groups.ts @@ -1555,10 +1555,7 @@ export function buildRows( const repoEntries = sortRepoEntriesWithinGroup(groupByProjectGroupIdentity.get(identity) ?? []) const childGroups = childGroupsByParentIdentity.get(identity) ?? [] // Why: bare project-group: stays when the bare id is unique; only same-id multi-owner rows need owner stamps. - const keyOwner = - projectGroups.filter((group) => group.id === projectGroup.id).length > 1 - ? ownerHostId - : undefined + const keyOwner = projectGroupIndex.ambiguousIds.has(projectGroup.id) ? ownerHostId : undefined const key = getProjectGroupHeaderKey(projectGroup.id, keyOwner) // Why: same-id groups on distinct owners collapse independently in persisted sidebar state. const collapseKey = getProjectGroupHeaderKey(projectGroup.id, keyOwner) @@ -1685,10 +1682,9 @@ export function getGroupKeysForWorktree( break } visited.add(identity) - const revealOwner = - projectGroups.filter((entry) => entry.id === group!.id).length > 1 - ? getProjectGroupOwnerHostId(group) - : undefined + const revealOwner = projectGroupIndex.ambiguousIds.has(group.id) + ? getProjectGroupOwnerHostId(group) + : undefined groupKeys.unshift(getProjectGroupHeaderKey(group.id, revealOwner)) group = findProjectGroupParentForSidebar(projectGroupIndex, group) } 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 1a8b5e0d212..6fac496b239 100644 --- a/src/renderer/src/store/slices/project-group-removal-targets.ts +++ b/src/renderer/src/store/slices/project-group-removal-targets.ts @@ -1,16 +1,12 @@ import type { FolderWorkspace, ProjectGroup, Repo } from '../../../../shared/types' -import { - getRepoExecutionHostId, - normalizeExecutionHostId, - toSshExecutionHostId, - type ExecutionHostId -} from '../../../../shared/execution-host' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' import { buildProjectGroupOwnerIndex, getProjectGroupIdentity, getProjectGroupOwnerHostId, getProjectGroupOwnerIdentity, getProjectGroupOwnerSubtreeIdentities, + resolveFolderWorkspaceProjectGroup, resolveProjectGroupOwner, type ProjectGroupOwnerIndex } from '../../../../shared/project-groups' @@ -32,26 +28,6 @@ export type ProjectGroupRemovalTargets = { folderWorkspaceIdentities: Set } -function getFolderWorkspacePreferredOwnerHostId( - workspace: Pick -): ExecutionHostId | undefined { - return ( - normalizeExecutionHostId(workspace.executionHostId) ?? - (workspace.connectionId ? toSshExecutionHostId(workspace.connectionId) : undefined) - ) -} - -function resolveFolderWorkspaceProjectGroup( - index: ProjectGroupOwnerIndex, - workspace: Pick -): ProjectGroup | null { - return resolveProjectGroupOwner( - index, - workspace.projectGroupId, - getFolderWorkspacePreferredOwnerHostId(workspace) - ) -} - export function getProjectGroupRemovalFolderWorkspaceIdentity( index: ProjectGroupOwnerIndex, workspace: Pick diff --git a/src/renderer/src/store/slices/repos-same-id-folder-workspace-identity.test.ts b/src/renderer/src/store/slices/repos-same-id-folder-workspace-identity.test.ts index 2d467f62aba..42481cd3cc3 100644 --- a/src/renderer/src/store/slices/repos-same-id-folder-workspace-identity.test.ts +++ b/src/renderer/src/store/slices/repos-same-id-folder-workspace-identity.test.ts @@ -67,12 +67,12 @@ describe('same-id cross-host folder workspace identity', () => { ], folderWorkspaces: [local, remote], folderWorkspacePathStatuses: { - 'local:folder-workspace:local:same-id': { + 'local:folder-workspace:["local","same-id"]': { status: { path: '/local/folder', exists: true }, checkedAt: 1, requestSnapshot: 'local' }, - 'local:folder-workspace:runtime:env-1:same-id': { + 'local:folder-workspace:["runtime:env-1","same-id"]': { status: { path: '/remote/folder', exists: true }, checkedAt: 1, requestSnapshot: 'remote' @@ -81,10 +81,11 @@ describe('same-id cross-host folder workspace identity', () => { purgeWorktreeTerminalState }) + const deleteFolderWorkspace = vi.fn().mockResolvedValue(true) vi.stubGlobal('window', { api: { folderWorkspaces: { - delete: vi.fn().mockResolvedValue(true) + delete: deleteFolderWorkspace } } }) @@ -96,11 +97,13 @@ describe('same-id cross-host folder workspace identity', () => { const remaining = store.getState().folderWorkspaces expect(remaining).toHaveLength(1) expect(remaining[0]?.executionHostId).toBe('runtime:env-1') - expect(purgeWorktreeTerminalState).toHaveBeenCalledWith([ - folderWorkspaceKey('same-id', 'local') - ]) + expect(deleteFolderWorkspace).toHaveBeenCalledWith({ + folderWorkspaceId: 'same-id', + ownerHostId: 'local' + }) + expect(purgeWorktreeTerminalState).not.toHaveBeenCalled() expect(Object.keys(store.getState().folderWorkspacePathStatuses)).toEqual([ - 'local:folder-workspace:runtime:env-1:same-id' + 'local:folder-workspace:["runtime:env-1","same-id"]' ]) }) @@ -119,16 +122,13 @@ describe('same-id cross-host folder workspace identity', () => { { ownerHostId: 'runtime:env-1' } ) expect(localKey).not.toBe(remoteKey) - expect(localKey).toContain('local:') - expect(localKey).toContain('folder-workspace:same-id') - expect(remoteKey).toContain('runtime:env-1:') - expect(remoteKey).toContain('folder-workspace:same-id') + expect(localKey).toContain('["local","same-id"]') + expect(remoteKey).toContain('["runtime:env-1","same-id"]') }) - it('migrates legacy bare folder session tabs for the default owner on activation', () => { + it('keeps the legacy bare folder session identity on unambiguous activation', () => { const store = createTestStore() const bareKey = folderWorkspaceKey('folder-workspace-1') - const qualifiedKey = folderWorkspaceKey('folder-workspace-1', 'local') const folder = makeFolder({ id: 'folder-workspace-1', projectGroupId: 'group-a', @@ -161,18 +161,16 @@ describe('same-id cross-host folder workspace identity', () => { store.getState().setActiveFolderWorkspace('folder-workspace-1', 'local') const state = store.getState() - expect(state.activeWorkspaceKey).toBe(qualifiedKey) - expect(state.activeWorktreeId).toBe(qualifiedKey) - expect(state.tabsByWorktree[qualifiedKey]?.[0]?.id).toBe('legacy-tab') - expect(state.tabsByWorktree[bareKey]).toBeUndefined() - expect(state.activeTabIdByWorktree[qualifiedKey]).toBe('legacy-tab') + expect(state.activeWorkspaceKey).toBe(bareKey) + expect(state.activeWorktreeId).toBe(bareKey) + expect(state.tabsByWorktree[bareKey]?.[0]?.id).toBe('legacy-tab') + expect(state.activeTabIdByWorktree[bareKey]).toBe('legacy-tab') expect(state.activeTabId).toBe('legacy-tab') }) - it('keeps owner-distinct same-id session tabs isolated across hosts', () => { + it('fails closed instead of owner-qualifying tabs for ambiguous folder ids', () => { const store = createTestStore() - const localKey = folderWorkspaceKey('same-id', 'local') - const remoteKey = folderWorkspaceKey('same-id', 'runtime:env-1') + const bareKey = folderWorkspaceKey('same-id') store.setState({ projectGroups: [ makeGroup({ id: 'same-group', executionHostId: 'local', parentPath: '/local' }), @@ -193,24 +191,12 @@ describe('same-id cross-host folder workspace identity', () => { }) ], tabsByWorktree: { - [localKey]: [ + [bareKey]: [ { - id: 'local-tab', - ptyId: 'pty-local', - worktreeId: localKey, - title: 'Local', - customTitle: null, - color: null, - sortOrder: 0, - createdAt: 1 - } - ], - [remoteKey]: [ - { - id: 'remote-tab', - ptyId: 'pty-remote', - worktreeId: remoteKey, - title: 'Remote', + id: 'legacy-tab', + ptyId: 'pty-legacy', + worktreeId: bareKey, + title: 'Legacy', customTitle: null, color: null, sortOrder: 0, @@ -218,20 +204,38 @@ describe('same-id cross-host folder workspace identity', () => { } ] }, - ptyIdsByTabId: { - 'local-tab': ['pty-local'], - 'remote-tab': ['pty-remote'] - } + ptyIdsByTabId: { 'legacy-tab': ['pty-legacy'] } }) store.getState().setActiveFolderWorkspace('same-id', 'local') - expect(store.getState().activeWorkspaceKey).toBe(localKey) - expect(store.getState().tabsByWorktree[localKey]?.[0]?.id).toBe('local-tab') - expect(store.getState().tabsByWorktree[remoteKey]?.[0]?.id).toBe('remote-tab') + expect(store.getState().activeWorkspaceKey).toBeNull() + expect(store.getState().activeWorktreeId).toBeNull() + expect(store.getState().tabsByWorktree[bareKey]?.[0]?.id).toBe('legacy-tab') + }) - store.getState().setActiveFolderWorkspace('same-id', 'runtime:env-1') - expect(store.getState().activeWorkspaceKey).toBe(remoteKey) - expect(store.getState().tabsByWorktree[localKey]?.[0]?.id).toBe('local-tab') - expect(store.getState().tabsByWorktree[remoteKey]?.[0]?.id).toBe('remote-tab') + it('forwards the selected folder owner through local update IPC', async () => { + const local = makeFolder({ executionHostId: 'local' }) + const remote = makeFolder({ executionHostId: 'runtime:env-1' }) + const update = vi.fn().mockResolvedValue({ ...local, name: 'Updated' }) + const store = createTestStore() + store.setState({ + projectGroups: [ + makeGroup({ executionHostId: 'local' }), + makeGroup({ executionHostId: 'runtime:env-1' }) + ], + folderWorkspaces: [local, remote] + }) + vi.stubGlobal('window', { api: { folderWorkspaces: { update } } }) + + await expect( + store + .getState() + .updateFolderWorkspace('same-id', { name: 'Updated' }, { executionHostId: 'local' }) + ).resolves.toBe(true) + expect(update).toHaveBeenCalledWith({ + folderWorkspaceId: 'same-id', + ownerHostId: 'local', + updates: { name: 'Updated' } + }) }) }) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 1b038ff1729..989b99744a7 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -46,6 +46,7 @@ import { applyManualRepoOrder, getManualRepoOrder } from '../../../../shared/man import { buildProjectGroupOwnerIndex, getFolderWorkspaceProjectGroupOwnerHostId, + getProjectGroupIdentity, getProjectGroupOwnerHostId, getProjectGroupOwnerIdentity, getProjectGroupSubtreeIds, @@ -98,15 +99,11 @@ import { } from '../../../../shared/execution-host' import { isRemovedRuntimeHostId } from './stale-runtime-host-rows' import { cleanupEphemeralVmRuntimesForDeleted } from '@/lib/ephemeral-vm-runtime-cleanup' +import { folderWorkspaceKey, parseWorkspaceKey } from '../../../../shared/workspace-scope' import { - folderWorkspaceKey, - folderWorkspaceSessionKeys, - parseWorkspaceKey -} from '../../../../shared/workspace-scope' -import { - getFolderWorkspaceCatalogOwnerHostId, getFolderWorkspaceIdentity, - getFolderWorkspaceOwnerIdentity + getFolderWorkspaceOwnerIdentity, + resolveFolderWorkspaceCatalogOwnerHostId } from '../../../../shared/folder-workspaces' import { formatFolderWorkspaceCreateError } from '../../lib/folder-workspace-path-status' import { getEnvironmentSshStateGeneration } from './runtime-environment-ssh' @@ -1232,15 +1229,8 @@ function clearRestoredFolderWorkspaceSessionOwners( next[key] = hostId continue } - const workspace = state.folderWorkspaces.find((entry) => { - if (entry.id !== scope.folderWorkspaceId) { - return false - } - if (!scope.ownerHostId) { - return true - } - return getFolderWorkspaceCatalogOwnerHostId(entry, state.projectGroups) === scope.ownerHostId - }) + const matches = state.folderWorkspaces.filter((entry) => entry.id === scope.folderWorkspaceId) + const workspace = matches.length === 1 ? matches[0] : undefined if (workspace && !state.projectGroups.some((group) => group.id === workspace.projectGroupId)) { // Why: ownership resolves via the project group; if that catalog is still missing, keep the restored host owner so a session write doesn't move runtime tabs local. next[key] = hostId @@ -1380,14 +1370,9 @@ async function reconcileFailedFolderWorkspaceUpdate(args: { const remainingSameId = args .get() .folderWorkspaces.some((workspace) => workspace.id === args.folderWorkspaceId) - args.get().purgeWorktreeTerminalState( - remainingSameId - ? [folderWorkspaceKey(args.folderWorkspaceId, args.ownerHostId)] - : folderWorkspaceSessionKeys({ - folderWorkspaceId: args.folderWorkspaceId, - ownerHostId: args.ownerHostId - }) - ) + if (!remainingSameId) { + args.get().purgeWorktreeTerminalState([folderWorkspaceKey(args.folderWorkspaceId)]) + } } } catch (err) { console.warn('Failed to reconcile folder workspace after update failure:', err) @@ -1433,14 +1418,14 @@ function settingsForRepoOwner( function getFolderWorkspacePathStatusScopeKey(request: FolderWorkspacePathStatusRequest): string { if (request.scope === 'project-group') { return request.ownerHostId - ? `project-group:${request.ownerHostId}:${request.projectGroupId}` + ? `project-group:${getProjectGroupIdentity(request.projectGroupId, request.ownerHostId)}` : `project-group:${request.projectGroupId}` } if (request.scope === 'path') { return `path:${request.connectionId ?? ''}:${request.path}` } return request.ownerHostId - ? `folder-workspace:${request.ownerHostId}:${request.folderWorkspaceId}` + ? `folder-workspace:${getFolderWorkspaceIdentity(request.folderWorkspaceId, request.ownerHostId)}` : `folder-workspace:${request.folderWorkspaceId}` } @@ -1576,21 +1561,22 @@ function getFolderWorkspaceStatusRequestSnapshot( const catalog = getFolderPathStatusCatalog(state.projectGroups, state.repos) const projectGroupIndex = catalog.projectGroupIndex + const folderMatches = + request.scope === 'folder-workspace' + ? state.folderWorkspaces.filter( + (workspace) => + workspace.id === request.folderWorkspaceId && + (!request.ownerHostId || + resolveFolderWorkspaceCatalogOwnerHostId(workspace, state.projectGroups) === + request.ownerHostId) + ) + : [] const scope = request.scope === 'project-group' ? resolveProjectGroupOwner(projectGroupIndex, request.projectGroupId, request.ownerHostId) - : state.folderWorkspaces.find((workspace) => { - if (workspace.id !== request.folderWorkspaceId) { - return false - } - if (!request.ownerHostId) { - return true - } - return ( - getFolderWorkspaceCatalogOwnerHostId(workspace, state.projectGroups) === - request.ownerHostId - ) - }) + : folderMatches.length === 1 + ? folderMatches[0] + : null const projectGroup = request.scope === 'project-group' ? scope && 'parentPath' in scope @@ -2593,10 +2579,12 @@ export const createRepoSlice: StateCreator = (set, } }, - getFolderWorkspacePathStatusCacheKey: (request, options) => - `${getRuntimeTargetCachePrefix( + getFolderWorkspacePathStatusCacheKey: (request, options) => { + const ownedRequest = getOwnerQualifiedFolderWorkspacePathStatusRequest(request, options) + return `${getRuntimeTargetCachePrefix( getFolderWorkspacePathStatusRouteSettings(options, get().settings) - )}:${options?.ownerHostId ? `${options.ownerHostId}:` : ''}${getFolderWorkspacePathStatusScopeKey(request)}`, + )}:${getFolderWorkspacePathStatusScopeKey(ownedRequest)}` + }, getFreshFolderWorkspacePathStatus: (request, options) => { const state = get() @@ -2840,7 +2828,15 @@ export const createRepoSlice: StateCreator = (set, (state.activeWorktreeId === folderWorkspaceKey(folderWorkspaceId) ? (state.activeWorkspaceExecutionHostId ?? undefined) : undefined) - if (!findFolderWorkspaceOwner(state, folderWorkspaceId, executionHostId)) { + const owner = findFolderWorkspaceOwner(state, folderWorkspaceId, executionHostId) + if (!owner) { + return false + } + const ownerHostId = + executionHostId ?? + resolveFolderWorkspaceCatalogOwnerHostId(owner, state.projectGroups) ?? + undefined + if (!ownerHostId) { return false } const runtimeEnvironmentId = getRuntimeEnvironmentIdForFolderWorkspace( @@ -2850,8 +2846,10 @@ export const createRepoSlice: StateCreator = (set, ) // Why: owner-scoped mutations must not follow whichever runtime happens to be focused. const target = getActiveRuntimeTarget({ activeRuntimeEnvironmentId: runtimeEnvironmentId }) - const ownerHostId = executionHostId ?? getRuntimeTargetHostId(target) const updateIdentity = getFolderWorkspaceUpdateIdentity(ownerHostId, folderWorkspaceId) + const includeOwnerHostId = + executionHostId !== undefined || + state.folderWorkspaces.filter((workspace) => workspace.id === folderWorkspaceId).length > 1 // Why: same gate as folderWorkspace.create — an older paired runtime would drop the Jira link silently. if ( target.kind === 'environment' && @@ -2871,7 +2869,11 @@ export const createRepoSlice: StateCreator = (set, try { const updated = target.kind === 'local' - ? await window.api.folderWorkspaces.update({ folderWorkspaceId, updates }) + ? await window.api.folderWorkspaces.update({ + folderWorkspaceId, + ...(includeOwnerHostId ? { ownerHostId } : {}), + updates + }) : ( await callRuntimeRpc<{ folderWorkspace: FolderWorkspace | null }>( target, @@ -2936,9 +2938,17 @@ export const createRepoSlice: StateCreator = (set, return false } const ownerHostId = - options?.ownerHostId ?? getFolderWorkspaceCatalogOwnerHostId(owner, state.projectGroups) + options?.ownerHostId ?? + resolveFolderWorkspaceCatalogOwnerHostId(owner, state.projectGroups) ?? + undefined + if (!ownerHostId) { + return false + } // Why: catalog owner can come from the project group even when the folder row itself is still unstamped. const ownerRuntime = parseExecutionHostId(ownerHostId) + const includeOwnerHostId = + options?.ownerHostId !== undefined || + state.folderWorkspaces.filter((workspace) => workspace.id === folderWorkspaceId).length > 1 const runtimeEnvironmentId = ownerRuntime?.kind === 'runtime' ? ownerRuntime.environmentId @@ -2948,7 +2958,10 @@ export const createRepoSlice: StateCreator = (set, const target = getActiveRuntimeTarget({ activeRuntimeEnvironmentId: runtimeEnvironmentId }) const deleted = target.kind === 'local' - ? await window.api.folderWorkspaces.delete({ folderWorkspaceId }) + ? await window.api.folderWorkspaces.delete({ + folderWorkspaceId, + ...(includeOwnerHostId ? { ownerHostId } : {}) + }) : ( await callRuntimeRpc<{ deleted: boolean }>( target, @@ -2968,21 +2981,22 @@ export const createRepoSlice: StateCreator = (set, ), folderWorkspacePathStatuses: Object.fromEntries( Object.entries(s.folderWorkspacePathStatuses).filter(([cacheKey]) => { - const ownerScoped = `folder-workspace:${ownerHostId}:${folderWorkspaceId}` + const ownerScoped = getFolderWorkspacePathStatusScopeKey({ + scope: 'folder-workspace', + folderWorkspaceId, + ownerHostId + }) const bareScoped = `folder-workspace:${folderWorkspaceId}` - return !cacheKey.includes(ownerScoped) && !cacheKey.endsWith(bareScoped) + return !cacheKey.endsWith(ownerScoped) && !cacheKey.endsWith(bareScoped) }) ) })) const remainingSameId = get().folderWorkspaces.some( (workspace) => workspace.id === folderWorkspaceId ) - // Why: same-id cross-host rows share the legacy bare key; only drop it when no sibling remains. - get().purgeWorktreeTerminalState( - remainingSameId - ? [folderWorkspaceKey(folderWorkspaceId, ownerHostId)] - : folderWorkspaceSessionKeys({ folderWorkspaceId, ownerHostId }) - ) + if (!remainingSameId) { + get().purgeWorktreeTerminalState([folderWorkspaceKey(folderWorkspaceId)]) + } return true } catch (err) { console.error('Failed to delete folder workspace:', err) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index c427d49943a..bf68f10bd3f 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -97,16 +97,13 @@ import { captureWorktreeOperationGenerationGuard } from '@/lib/worktree-operatio import { getEnvironmentSshStateGeneration } from './runtime-environment-ssh' import { getRuntimeEnvironmentConnectionGeneration } from './runtime-status' import { - dualReadFolderWorkspaceKeyedValue, folderWorkspaceKey, getActiveSidebarWorkspaceId, isWorkspaceKey, - migrateFolderWorkspaceKeyedRecord, parseWorkspaceKey, worktreeWorkspaceKey } from '../../../../shared/workspace-scope' import { folderWorkspaceToWorktree } from '../../../../shared/folder-workspace-worktree' -import { getFolderWorkspaceCatalogOwnerHostId } from '../../../../shared/folder-workspaces' import { CLIENT_WORKTREE_CREATE_MAX_ATTEMPTS, getClientWorktreeCreateCandidate, @@ -878,110 +875,6 @@ function folderWorkspaceMatchesHost( ) } -/** Promote legacy bare `folder:` session maps to owner-qualified keys for one unambiguous owner. */ -function migrateLegacyFolderWorkspaceSessionMaps( - s: AppState, - folderWorkspaceId: string, - ownerHostId: ExecutionHostId -): Partial | null { - const resolveOwner = (id: string): ExecutionHostId | null => - id === folderWorkspaceId ? ownerHostId : null - const tabs = migrateFolderWorkspaceKeyedRecord(s.tabsByWorktree, resolveOwner) - const activeFileIdByWorktree = migrateFolderWorkspaceKeyedRecord( - s.activeFileIdByWorktree, - resolveOwner - ) - const activeBrowserTabIdByWorktree = migrateFolderWorkspaceKeyedRecord( - s.activeBrowserTabIdByWorktree, - resolveOwner - ) - const activeTabTypeByWorktree = migrateFolderWorkspaceKeyedRecord( - s.activeTabTypeByWorktree, - resolveOwner - ) - const activeTabIdByWorktree = migrateFolderWorkspaceKeyedRecord( - s.activeTabIdByWorktree, - resolveOwner - ) - const browserTabsByWorktree = migrateFolderWorkspaceKeyedRecord( - s.browserTabsByWorktree, - resolveOwner - ) - const groupsByWorktree = migrateFolderWorkspaceKeyedRecord(s.groupsByWorktree, resolveOwner) - const unifiedTabsByWorktree = migrateFolderWorkspaceKeyedRecord( - s.unifiedTabsByWorktree, - resolveOwner - ) - const activeGroupIdByWorktree = migrateFolderWorkspaceKeyedRecord( - s.activeGroupIdByWorktree, - resolveOwner - ) - // Why: openFiles rows also stamp worktreeId; rewrite legacy bare folder stamps for this owner. - let openFilesChanged = false - const nextOpenFiles = s.openFiles.map((file) => { - const scope = parseWorkspaceKey(file.worktreeId) - if ( - scope?.type === 'folder' && - !scope.ownerHostId && - scope.folderWorkspaceId === folderWorkspaceId - ) { - openFilesChanged = true - return { ...file, worktreeId: folderWorkspaceKey(folderWorkspaceId, ownerHostId) } - } - return file - }) - const bareKey = folderWorkspaceKey(folderWorkspaceId) - const qualifiedKey = folderWorkspaceKey(folderWorkspaceId, ownerHostId) - const activeMigrated = - s.activeWorkspaceKey === bareKey || s.activeWorktreeId === bareKey - ? { activeWorkspaceKey: qualifiedKey, activeWorktreeId: qualifiedKey } - : null - const changed = - tabs.migratedKeys.length > 0 || - activeFileIdByWorktree.migratedKeys.length > 0 || - activeBrowserTabIdByWorktree.migratedKeys.length > 0 || - activeTabTypeByWorktree.migratedKeys.length > 0 || - activeTabIdByWorktree.migratedKeys.length > 0 || - browserTabsByWorktree.migratedKeys.length > 0 || - groupsByWorktree.migratedKeys.length > 0 || - unifiedTabsByWorktree.migratedKeys.length > 0 || - activeGroupIdByWorktree.migratedKeys.length > 0 || - openFilesChanged || - activeMigrated != null - if (!changed) { - return null - } - return { - ...(tabs.migratedKeys.length > 0 ? { tabsByWorktree: tabs.record } : {}), - ...(activeFileIdByWorktree.migratedKeys.length > 0 - ? { activeFileIdByWorktree: activeFileIdByWorktree.record } - : {}), - ...(activeBrowserTabIdByWorktree.migratedKeys.length > 0 - ? { activeBrowserTabIdByWorktree: activeBrowserTabIdByWorktree.record } - : {}), - ...(activeTabTypeByWorktree.migratedKeys.length > 0 - ? { activeTabTypeByWorktree: activeTabTypeByWorktree.record } - : {}), - ...(activeTabIdByWorktree.migratedKeys.length > 0 - ? { activeTabIdByWorktree: activeTabIdByWorktree.record } - : {}), - ...(browserTabsByWorktree.migratedKeys.length > 0 - ? { browserTabsByWorktree: browserTabsByWorktree.record } - : {}), - ...(groupsByWorktree.migratedKeys.length > 0 - ? { groupsByWorktree: groupsByWorktree.record } - : {}), - ...(unifiedTabsByWorktree.migratedKeys.length > 0 - ? { unifiedTabsByWorktree: unifiedTabsByWorktree.record } - : {}), - ...(activeGroupIdByWorktree.migratedKeys.length > 0 - ? { activeGroupIdByWorktree: activeGroupIdByWorktree.record } - : {}), - ...(openFilesChanged ? { openFiles: nextOpenFiles } : {}), - ...(activeMigrated != null ? activeMigrated : {}) - } -} - function findKnownWorktreeById( state: Pick, worktreeId: string, @@ -989,18 +882,12 @@ function findKnownWorktreeById( ): Worktree | DetectedWorktreeListResult['worktrees'][number] | undefined { const workspaceScope = parseWorkspaceKey(worktreeId) if (workspaceScope?.type === 'folder') { - const preferredHostId = executionHostId ?? workspaceScope.ownerHostId const matches = state.folderWorkspaces.filter( (workspace) => workspace.id === workspaceScope.folderWorkspaceId && - (!preferredHostId || folderWorkspaceMatchesHost(workspace, preferredHostId)) + (!executionHostId || folderWorkspaceMatchesHost(workspace, executionHostId)) ) - // Why: bare folder keys dual-read only when the owner is unambiguous; multi-owner same-id must use owner-qualified keys. - const folderWorkspace = preferredHostId - ? (matches[0] ?? null) - : matches.length === 1 - ? matches[0] - : null + const folderWorkspace = matches.length === 1 ? matches[0] : null if (!folderWorkspace) { return undefined } @@ -3787,9 +3674,6 @@ export const createWorktreeSlice: StateCreator validIds.add(FLOATING_TERMINAL_WORKTREE_ID) // Why: folder workspaces persist tabs under `folder:` keys that authoritative repo scans never return. for (const workspace of get().folderWorkspaces ?? []) { - const ownerHostId = getFolderWorkspaceCatalogOwnerHostId(workspace, get().projectGroups) - validIds.add(folderWorkspaceKey(workspace.id, ownerHostId)) - // Why: pre-owner session rows still use bare folder ids. validIds.add(folderWorkspaceKey(workspace.id)) } for (const key of Object.keys(get().restoredRuntimeHostIdByWorkspaceSessionKey ?? {})) { @@ -4120,7 +4004,7 @@ export const createWorktreeSlice: StateCreator const activeScope = parseWorkspaceKey(get().activeWorkspaceKey ?? '') const parentWorkspace = activeScope?.type === 'folder' - ? folderWorkspaceKey(activeScope.folderWorkspaceId, activeScope.ownerHostId) + ? folderWorkspaceKey(activeScope.folderWorkspaceId) : undefined const createArgs = { repoId, @@ -6041,25 +5925,16 @@ export const createWorktreeSlice: StateCreator }, setActiveFolderWorkspace: (folderWorkspaceId, executionHostId) => { - const workspace = findKnownWorktreeById( - get(), - folderWorkspaceKey(folderWorkspaceId, executionHostId), - executionHostId - ) - if (!workspace) { + // Why: tabs keep the legacy bare workspace id, so a same-id catalog cannot activate safely. + if ( + get().folderWorkspaces.filter((workspace) => workspace.id === folderWorkspaceId).length !== 1 + ) { return } - // Why: synthetic folder worktrees stamp hostId from catalog ownership; prefer the caller host when provided. - const ownerHostId = executionHostId ?? workspace.hostId ?? LOCAL_EXECUTION_HOST_ID - const workspaceKey = folderWorkspaceKey(folderWorkspaceId, ownerHostId) - // Why: promote legacy bare `folder:` session maps before activation so dual-read becomes a one-time migration. - const migratedSession = migrateLegacyFolderWorkspaceSessionMaps( - get(), - folderWorkspaceId, - ownerHostId - ) - if (migratedSession) { - set(migratedSession) + const workspaceKey = folderWorkspaceKey(folderWorkspaceId) + const workspace = findKnownWorktreeById(get(), workspaceKey, executionHostId) + if (!workspace) { + return } if (shouldDeferActivationTerminalPrep()) { markInputQuietSchedulerInput() @@ -6070,71 +5945,32 @@ export const createWorktreeSlice: StateCreator const reconciledActiveTabId = get().reconcileWorktreeTabModel(workspaceKey).activeRenderableTabId set((s) => { - const dualTabs = dualReadFolderWorkspaceKeyedValue((key) => s.tabsByWorktree[key], { - folderWorkspaceId, - ownerHostId - }) - const dualActiveFile = dualReadFolderWorkspaceKeyedValue( - (key) => s.activeFileIdByWorktree[key], - { folderWorkspaceId, ownerHostId } - ) - const dualActiveBrowser = dualReadFolderWorkspaceKeyedValue( - (key) => s.activeBrowserTabIdByWorktree[key], - { folderWorkspaceId, ownerHostId } - ) - const dualActiveTabType = dualReadFolderWorkspaceKeyedValue( - (key) => s.activeTabTypeByWorktree[key], - { folderWorkspaceId, ownerHostId } - ) - const dualActiveTab = dualReadFolderWorkspaceKeyedValue( - (key) => s.activeTabIdByWorktree[key], - { folderWorkspaceId, ownerHostId } - ) - const dualBrowserTabs = dualReadFolderWorkspaceKeyedValue( - (key) => s.browserTabsByWorktree[key], - { folderWorkspaceId, ownerHostId } - ) - const dualGroups = dualReadFolderWorkspaceKeyedValue((key) => s.groupsByWorktree[key], { - folderWorkspaceId, - ownerHostId - }) - const dualUnified = dualReadFolderWorkspaceKeyedValue((key) => s.unifiedTabsByWorktree[key], { - folderWorkspaceId, - ownerHostId - }) - const dualActiveGroup = dualReadFolderWorkspaceKeyedValue( - (key) => s.activeGroupIdByWorktree[key], - { folderWorkspaceId, ownerHostId } - ) - const restoredFileId = dualActiveFile?.value ?? null - const restoredBrowserTabId = dualActiveBrowser?.value ?? null - const restoredTabType = dualActiveTabType?.value ?? 'terminal' - const activeGroupId = dualActiveGroup?.value ?? dualGroups?.value?.[0]?.id ?? null + const restoredFileId = s.activeFileIdByWorktree[workspaceKey] ?? null + const restoredBrowserTabId = s.activeBrowserTabIdByWorktree[workspaceKey] ?? null + const restoredTabType = s.activeTabTypeByWorktree[workspaceKey] ?? 'terminal' + const activeGroupId = + s.activeGroupIdByWorktree[workspaceKey] ?? s.groupsByWorktree[workspaceKey]?.[0]?.id ?? null const activeGroup = activeGroupId - ? ((dualGroups?.value ?? []).find((group) => group.id === activeGroupId) ?? null) + ? ((s.groupsByWorktree[workspaceKey] ?? []).find((group) => group.id === activeGroupId) ?? + null) : null const activeUnifiedTabId = reconciledActiveTabId ?? activeGroup?.activeTabId ?? null const activeUnifiedTab = activeUnifiedTabId != null - ? ((dualUnified?.value ?? []).find( + ? ((s.unifiedTabsByWorktree[workspaceKey] ?? []).find( (tab) => tab.id === activeUnifiedTabId && (!activeGroup || tab.groupId === activeGroup.id) ) ?? null) : null const fileStillOpen = restoredFileId - ? s.openFiles.some( - (file) => - file.id === restoredFileId && - (file.worktreeId === workspaceKey || - file.worktreeId === folderWorkspaceKey(folderWorkspaceId)) - ) + ? s.openFiles.some((file) => file.id === restoredFileId && file.worktreeId === workspaceKey) : false - const browserTabs = dualBrowserTabs?.value ?? [] + const browserTabs = s.browserTabsByWorktree[workspaceKey] ?? [] const browserTabStillOpen = restoredBrowserTabId ? browserTabs.some((tab) => tab.id === restoredBrowserTabId) : false - const worktreeTabs = dualTabs?.value ?? [] - const restoredTabId = dualActiveTab?.value ?? null + const worktreeTabs = s.tabsByWorktree[workspaceKey] ?? [] + const restoredTabId = s.activeTabIdByWorktree[workspaceKey] ?? null const tabStillExists = restoredTabId ? worktreeTabs.some((tab) => tab.id === restoredTabId) : false diff --git a/src/shared/folder-workspaces.ts b/src/shared/folder-workspaces.ts index b1bb74bc50a..26260212cf6 100644 --- a/src/shared/folder-workspaces.ts +++ b/src/shared/folder-workspaces.ts @@ -20,23 +20,34 @@ export function getFolderWorkspaceCatalogOwnerHostId( workspace: Pick, projectGroups: readonly ProjectGroup[] = [] ): ExecutionHostId { + return ( + resolveFolderWorkspaceCatalogOwnerHostId(workspace, projectGroups) ?? LOCAL_EXECUTION_HOST_ID + ) +} + +export function resolveFolderWorkspaceCatalogOwnerHostId( + workspace: Pick, + projectGroups: readonly ProjectGroup[] = [] +): ExecutionHostId | null { const executionHostId = normalizeExecutionHostId(workspace.executionHostId) if (executionHostId) { return executionHostId } + if (projectGroups.length > 0) { + const index = buildProjectGroupOwnerIndex(projectGroups) + const group = resolveFolderWorkspaceProjectGroup(index, workspace) + if (group) { + return getFolderWorkspaceProjectGroupOwnerHostId(workspace, index) + } + return null + } if (workspace.connectionId) { return toSshExecutionHostId(workspace.connectionId) } if (workspace.connectionId === null) { return LOCAL_EXECUTION_HOST_ID } - if (projectGroups.length === 0) { - return LOCAL_EXECUTION_HOST_ID - } - return getFolderWorkspaceProjectGroupOwnerHostId( - workspace, - buildProjectGroupOwnerIndex(projectGroups) - ) + return LOCAL_EXECUTION_HOST_ID } export function getFolderWorkspaceOwnerIdentity( diff --git a/src/shared/types.ts b/src/shared/types.ts index 7fb4099d1b2..cd874d94037 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -326,7 +326,7 @@ export type ProjectGroup = { export type WorkspaceScope = | { type: 'worktree'; worktreeId: string } - | { type: 'folder'; folderWorkspaceId: string; ownerHostId?: ExecutionHostId } + | { type: 'folder'; folderWorkspaceId: string } export type WorkspaceKey = `worktree:${string}` | `folder:${string}` diff --git a/src/shared/workspace-scope.test.ts b/src/shared/workspace-scope.test.ts deleted file mode 100644 index 6dda189661a..00000000000 --- a/src/shared/workspace-scope.test.ts +++ /dev/null @@ -1,87 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { - dualReadFolderWorkspaceKeyedValue, - folderWorkspaceKey, - folderWorkspaceSessionKeys, - migrateFolderWorkspaceKeyedRecord, - parseWorkspaceKey -} from './workspace-scope' - -describe('owner-qualified folder workspace keys', () => { - it('round-trips owner-qualified keys for same-id multi-host rows', () => { - const localKey = folderWorkspaceKey('same-id', 'local') - const sshKey = folderWorkspaceKey('same-id', 'ssh:builder-1') - - expect(localKey).not.toBe(sshKey) - expect(parseWorkspaceKey(localKey)).toEqual({ - type: 'folder', - folderWorkspaceId: 'same-id', - ownerHostId: 'local' - }) - expect(parseWorkspaceKey(sshKey)).toEqual({ - type: 'folder', - folderWorkspaceId: 'same-id', - ownerHostId: 'ssh:builder-1' - }) - }) - - it('keeps legacy bare folder keys parseable', () => { - expect(parseWorkspaceKey(folderWorkspaceKey('legacy-folder'))).toEqual({ - type: 'folder', - folderWorkspaceId: 'legacy-folder' - }) - }) - - it('lists both owner-qualified and legacy session keys for cleanup', () => { - expect( - folderWorkspaceSessionKeys({ folderWorkspaceId: 'same-id', ownerHostId: 'local' }) - ).toEqual([folderWorkspaceKey('same-id', 'local'), folderWorkspaceKey('same-id')]) - }) - - it('dual-reads owner-qualified first, then legacy bare alias', () => { - const bare = folderWorkspaceKey('folder-workspace-1') - const qualified = folderWorkspaceKey('folder-workspace-1', 'local') - const record: Record = { [bare]: 'legacy-tabs' } - - expect( - dualReadFolderWorkspaceKeyedValue((key) => record[key], { - folderWorkspaceId: 'folder-workspace-1', - ownerHostId: 'local' - }) - ).toEqual({ value: 'legacy-tabs', key: bare, viaLegacyAlias: true }) - - record[qualified] = 'canonical-tabs' - expect( - dualReadFolderWorkspaceKeyedValue((key) => record[key], { - folderWorkspaceId: 'folder-workspace-1', - ownerHostId: 'local' - }) - ).toEqual({ value: 'canonical-tabs', key: qualified, viaLegacyAlias: false }) - }) - - it('migrates unambiguous legacy bare folder keys for the default owner', () => { - const bare = folderWorkspaceKey('folder-workspace-1') - const qualified = folderWorkspaceKey('folder-workspace-1', 'local') - const { record, migratedKeys } = migrateFolderWorkspaceKeyedRecord( - { [bare]: [{ id: 'tab-1' }], [folderWorkspaceKey('ambiguous')]: [{ id: 'tab-2' }] }, - (folderWorkspaceId) => (folderWorkspaceId === 'folder-workspace-1' ? 'local' : null) - ) - - expect(migratedKeys).toEqual([bare]) - expect(record[qualified]).toEqual([{ id: 'tab-1' }]) - expect(record[bare]).toBeUndefined() - // Multi-owner / unresolved bare keys stay put so callers fail closed. - expect(record[folderWorkspaceKey('ambiguous')]).toEqual([{ id: 'tab-2' }]) - }) - - it('does not overwrite an existing owner-qualified row when migrating bare aliases', () => { - const bare = folderWorkspaceKey('folder-workspace-1') - const qualified = folderWorkspaceKey('folder-workspace-1', 'local') - const { record } = migrateFolderWorkspaceKeyedRecord( - { [bare]: 'legacy', [qualified]: 'canonical' }, - () => 'local' - ) - expect(record[qualified]).toBe('canonical') - expect(record[bare]).toBeUndefined() - }) -}) diff --git a/src/shared/workspace-scope.ts b/src/shared/workspace-scope.ts index 60ac6f607e0..4993c0599d7 100644 --- a/src/shared/workspace-scope.ts +++ b/src/shared/workspace-scope.ts @@ -1,18 +1,10 @@ -import type { ExecutionHostId } from './execution-host' import type { WorkspaceKey, WorkspaceScope } from './types' export function worktreeWorkspaceKey(worktreeId: string): WorkspaceKey { return `worktree:${worktreeId}` } -/** Owner-qualified folder session keys keep same-id multi-host rows independent. */ -export function folderWorkspaceKey( - folderWorkspaceId: string, - ownerHostId?: ExecutionHostId -): WorkspaceKey { - if (ownerHostId) { - return `folder:${encodeURIComponent(ownerHostId)}:${encodeURIComponent(folderWorkspaceId)}` - } +export function folderWorkspaceKey(folderWorkspaceId: string): WorkspaceKey { return `folder:${folderWorkspaceId}` } @@ -22,24 +14,8 @@ export function parseWorkspaceKey(value: string): WorkspaceScope | null { return worktreeId.length > 0 ? { type: 'worktree', worktreeId } : null } if (value.startsWith('folder:')) { - const rest = value.slice('folder:'.length) - if (rest.length === 0) { - return null - } - // Why: owner-qualified keys encode both segments so host ids with colons stay unambiguous. - const separator = rest.indexOf(':') - if (separator > 0) { - try { - const ownerHostId = decodeURIComponent(rest.slice(0, separator)) as ExecutionHostId - const folderWorkspaceId = decodeURIComponent(rest.slice(separator + 1)) - if (ownerHostId.length > 0 && folderWorkspaceId.length > 0) { - return { type: 'folder', folderWorkspaceId, ownerHostId } - } - } catch { - // Fall through to legacy id-only form when decoding fails. - } - } - return { type: 'folder', folderWorkspaceId: rest } + const folderWorkspaceId = value.slice('folder:'.length) + return folderWorkspaceId.length > 0 ? { type: 'folder', folderWorkspaceId } : null } return null } @@ -56,80 +32,10 @@ export function getActiveSidebarWorkspaceId( ): string | null { const scope = activeWorkspaceKey ? parseWorkspaceKey(activeWorkspaceKey) : null if (scope?.type === 'folder') { - return folderWorkspaceKey(scope.folderWorkspaceId, scope.ownerHostId) + return folderWorkspaceKey(scope.folderWorkspaceId) } if (scope?.type === 'worktree') { return scope.worktreeId } return activeWorktreeId } - -export function folderWorkspaceSessionKeys(args: { - folderWorkspaceId: string - ownerHostId: ExecutionHostId -}): string[] { - return [ - folderWorkspaceKey(args.folderWorkspaceId, args.ownerHostId), - // Why: older sessions still key by bare folder id; purge both so one owner cleanup cannot leave the legacy alias behind for the deleted row only when it was sole occupant is handled by callers. - folderWorkspaceKey(args.folderWorkspaceId) - ] -} - -/** - * Dual-read contract for owner-qualified folder session keys: - * prefer the canonical owner-qualified key, then fall back to the legacy bare - * `folder:` alias so pre-migration tabs/state keep resolving for that owner. - */ -export function dualReadFolderWorkspaceKeyedValue( - get: (key: string) => T | undefined, - args: { folderWorkspaceId: string; ownerHostId: ExecutionHostId } -): { value: T; key: string; viaLegacyAlias: boolean } | null { - const qualified = folderWorkspaceKey(args.folderWorkspaceId, args.ownerHostId) - const fromQualified = get(qualified) - if (fromQualified !== undefined) { - return { value: fromQualified, key: qualified, viaLegacyAlias: false } - } - const bare = folderWorkspaceKey(args.folderWorkspaceId) - const fromBare = get(bare) - if (fromBare !== undefined) { - return { value: fromBare, key: bare, viaLegacyAlias: true } - } - return null -} - -/** - * Migrate legacy bare `folder:` map keys to owner-qualified form when the - * catalog can resolve an unambiguous owner. Multi-owner same-id bare keys stay - * untouched so callers can fail closed instead of guessing. - */ -export function migrateFolderWorkspaceKeyedRecord( - record: Record, - resolveUnambiguousOwner: (folderWorkspaceId: string) => ExecutionHostId | null -): { record: Record; migratedKeys: string[] } { - const migratedKeys: string[] = [] - let next: Record | null = null - for (const [key, value] of Object.entries(record)) { - const scope = parseWorkspaceKey(key) - if (scope?.type !== 'folder' || scope.ownerHostId) { - continue - } - const ownerHostId = resolveUnambiguousOwner(scope.folderWorkspaceId) - if (!ownerHostId) { - continue - } - const qualified = folderWorkspaceKey(scope.folderWorkspaceId, ownerHostId) - if (qualified === key) { - continue - } - if (!next) { - next = { ...record } - } - // Why: if both forms already exist, keep the canonical owner-qualified row. - if (!(qualified in next)) { - next[qualified] = value - } - delete next[key] - migratedKeys.push(key) - } - return { record: next ?? record, migratedKeys } -}