Repository navigation
fix(persistence): repo-row store writes take the row, not the id; ratchet the rest (R4-2a) - #30756
Conversation
…chet the rest (R4-2a)
…-only write ratchet (review)
…s on a paired host
…new main test to the target shape
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0cf07499-1d5e-4c2c-be4a-e20ba453e933
📒 Files selected for processing: 45
.github/workflows/pr.ymlconfig/id-only-store-write-baseline.txtconfig/scripts/check-id-only-store-write-ratchet.mjsconfig/scripts/check-id-only-store-write-ratchet.test.mjspackage.jsonsrc/main/ipc/repos-execution-host-catalog.test.tssrc/main/ipc/repos-local-add-and-project-setup.test.tssrc/main/ipc/repos-remote-client-events.test.tssrc/main/ipc/repos/nested-repo-import-handler.tssrc/main/ipc/repos/project-group-handlers.tssrc/main/ipc/repos/project-host-setup-handlers.tssrc/main/ipc/repos/repo-catalog-handlers.tssrc/main/ipc/repos/repo-ipc-arg-schemas.tssrc/main/persistence-duplicate-repo-id-host-scope.test.tssrc/main/persistence-orcad-migration-catalog.test.tssrc/main/persistence-orcad-migration-terminal-restore-hints.test.tssrc/main/persistence-remote-session-startup.test.tssrc/main/persistence-repo-lifecycle.test.tssrc/main/persistence-worktree-lineage-and-backups.test.tssrc/main/persistence-worktree-name-retirement.test.tssrc/main/persistence/loading-store/project-collection-operations.tssrc/main/persistence/loading-store/repo-lifecycle-operations.tssrc/main/persistence/restoring-sessions/folder-workspace-operations.tssrc/main/persistence/tracking-repos/project-host-operations.tssrc/main/runtime/orca-runtime-tests/project-setup-folder-remote-identity.spec.tssrc/main/runtime/orca-runtime-tests/repository-project-operations.spec.tssrc/main/runtime/project-removal-structured-sessions.test.tssrc/main/runtime/project-removal-terminal-stop.test.tssrc/main/runtime/runtime-nested-repo-import-host-twin.test.tssrc/main/runtime/runtime-nested-repo-import.tssrc/main/runtime/runtime-project-group-controller.tssrc/main/runtime/runtime-project-host-setup-controller.tssrc/main/runtime/runtime-remove-project-host-scope.spec.tssrc/main/runtime/runtime-repository-settings-controller.tssrc/main/ssh/orcad-migration-delta-move.test.tssrc/main/ssh/orcad-migration-repository-link.test.tssrc/main/worktree-removal-close-records.test.tssrc/preload/api/repository-api.tssrc/renderer/src/components/sidebar/project-header-drag-commit.test.tssrc/renderer/src/components/sidebar/project-header-drag-commit.tssrc/renderer/src/components/sidebar/project-header-drag-contract.tssrc/renderer/src/components/sidebar/worktree-list/drag/use-header-drag.tssrc/renderer/src/store/project-groups/project-group-mutations.tssrc/renderer/src/store/slices/repos-project-group-cross-host-move.test.tssrc/renderer/src/store/slices/repos-project-groups.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| const BASELINE_PATH = 'config/id-only-store-write-baseline.txt' | ||
| const COMPOSITION = 'src/main/persistence/loading-store/store-domain-composition.ts' | ||
| // The only type names that make an id-named parameter host-carrying. | ||
| const HOST_TARGET_TYPE = /^(?:RepoHostTarget|HostWorktreeTarget)\b/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,145p' config/scripts/check-id-only-store-write-ratchet.mjs
sed -n '1,85p' config/scripts/check-id-only-store-write-ratchet.test.mjsRepository: stablyai/orca
Length of output: 8731
Match the complete host-target type.
MEMBER captures RepoHostTarget | string as the first parameter type. HOST_TARGET_TYPE checks only the prefix, so findIdOnlyWrites skips this method even though callers can pass a bare string ID. This creates a false negative in the ratchet; it does not establish an existing wrong-row write.
Suggested checker and regression test
-const HOST_TARGET_TYPE = /^(?:RepoHostTarget|HostWorktreeTarget)\b/
+const HOST_TARGET_TYPE = /^(?:RepoHostTarget|HostWorktreeTarget)$/ updateFolderWorkspace(
id: string,
updates: object
): void {}
+ updateRepoById(repoId: RepoHostTarget | string): void {}
+
removeWorktreeMeta(worktreeId?: string): void {}
@@
'RowOps.moveProjectToGroup',
'RowOps.updateFolderWorkspace',
+ 'RowOps.updateRepoById',
'RowOps.removeWorktreeMeta',| args.session.repoId, | ||
| draggedRepo.projectGroupId ?? null, | ||
| order, | ||
| getRepoExecutionHostId(draggedRepo) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n -C 8 'useWorktreeSidebarHeaderDrag\s*\(' src/renderer/src
rg -n -C 6 'repoMap|sidebarRepoHeaderIdsByBucket|bucketKey' src/renderer/src/components/sidebar/worktree-listRepository: stablyai/orca
Length of output: 41935
🏁 Script executed:
set -eu
printf '%s\n' '--- commit path ---'
nl -ba src/renderer/src/components/sidebar/project-header-drag-commit.ts | sed -n '1,180p'
printf '%s\n' '--- header drag hook ---'
nl -ba src/renderer/src/components/sidebar/worktree-list/drag/use-header-drag.ts | sed -n '1,280p'
printf '%s\n' '--- viewport caller ---'
nl -ba src/renderer/src/components/sidebar/worktree-list/viewport/VirtualizedWorktreeViewport.tsx | sed -n '1,180p'
printf '%s\n' '--- repoMap construction ---'
rg -n -C 8 --glob '*.{ts,tsx}' -e 'new Map\(.*repo' -e 'repoMap\s*=' -e 'repoMap:' src/renderer/src/components/sidebar src/renderer/src | head -n 320
printf '%s\n' '--- bucket/header identity ---'
rg -n -C 8 --glob '*.{ts,tsx}' -e 'getSidebarOrderedRepoHeaderIdsByBucket' -e 'sidebarRepoHeaderIdsByBucket' -e 'bucketKey' src/renderer/src/components/sidebar/worktree-list src/renderer/src/components/sidebar | head -n 420Repository: stablyai/orca
Length of output: 41953
🏁 Script executed:
set -eu
printf '%s\n' '--- bucket implementation ---'
nl -ba src/renderer/src/components/sidebar/project-header-drop.ts | sed -n '1,260p'
printf '%s\n' '--- repo header drag controller ---'
nl -ba src/renderer/src/components/sidebar/project-header-drag.ts | sed -n '1,320p'
printf '%s\n' '--- drag contract and callers ---'
nl -ba src/renderer/src/components/sidebar/project-header-drag-contract.ts | sed -n '1,220p'
rg -n -C 12 --glob '*.{ts,tsx}' -e 'commitProjectHeaderDragDrop' -e 'ProjectHeaderDragSession' -e 'repoById' src/renderer/src/components/sidebar
printf '%s\n' '--- row/repo source and map construction ---'
rg -n -C 12 --glob '*.{ts,tsx}' -e 'Map<string, Repo>' -e 'new Map.*repos' -e 'repoMap.*new Map' -e 'repos.map' -e 'repoMap' src/renderer/src/components/sidebar | head -n 500Repository: stablyai/orca
Length of output: 42145
🏁 Script executed:
set -eu
printf '%s\n' '--- drag session creation ---'
nl -ba src/renderer/src/components/sidebar/project-header-drag-start.ts | sed -n '1,180p'
printf '%s\n' '--- drag contract types ---'
nl -ba src/renderer/src/components/sidebar/project-header-drag-contract.ts | sed -n '1,130p'
printf '%s\n' '--- section header props and drag hookup ---'
nl -ba src/renderer/src/components/sidebar/worktree-list/rows/SectionHeader.tsx | sed -n '1,240p'
printf '%s\n' '--- row constructors and header repo selection ---'
rg -n -C 10 --glob '*.{ts,tsx}' -e 'type: .header' -e 'repo:' -e 'projectGroupId' src/renderer/src/components/sidebar/worktree-list | head -n 500
printf '%s\n' '--- sidebar repo map ownership ---'
rg -n -C 10 --glob '*.{ts,tsx}' -e 'const repoMap' -e 'repoMap =' -e 'new Map.*repo' -e 'repos.*map' src/renderer/src | head -n 500Repository: stablyai/orca
Length of output: 41641
🏁 Script executed:
set -eu
printf '%s\n' '--- SectionHeader drag callback ---'
nl -ba src/renderer/src/components/sidebar/worktree-list/rows/SectionHeader.tsx | sed -n '240,430p'
printf '%s\n' '--- header drag pointer callback ---'
rg -n -C 16 --glob '*.{ts,tsx}' -e 'onHandlePointerDown' -e 'data-repo-header-drag-handle' src/renderer/src/components/sidebar
printf '%s\n' '--- host-qualified duplicate repo evidence ---'
rg -n -C 12 --glob '*.{ts,tsx}' -e 'same.*id' -e 'duplicate.*repo' -e 'duplicate.*host' -e 'executionHostId.*repo' -e 'connectionId.*executionHostId' src/renderer/src | head -n 600
printf '%s\n' '--- sidebar row builder entrypoints ---'
rg -n -C 12 --glob '*.{ts,tsx}' -e 'planSidebarRows' -e 'buildSidebarRow' -e 'GroupHeaderRow' src/renderer/src/components/sidebar/worktree-list src/shared/sidebar-list | head -n 600Repository: stablyai/orca
Length of output: 41709
🏁 Script executed:
set -eu
printf '%s\n' '--- row types ---'
nl -ba src/shared/sidebar-list/row-types.ts | sed -n '1,240p'
printf '%s\n' '--- row planner repo/header logic ---'
rg -n -C 14 --glob '*.ts' -e 'row.repo' -e 'RepoHeader' -e 'repoMap.get' -e 'header.*repo' src/shared/sidebar-list/sidebar-row-plan.ts src/shared/sidebar-list/project-group-sections.ts src/shared/sidebar-list
printf '%s\n' '--- host-scoped feed and planner inputs ---'
nl -ba src/shared/sidebar-list/sidebar-feed-rows.ts | sed -n '180,270p'
rg -n -C 12 --glob '*.{ts,tsx}' -e 'hostScoped' -e 'visibleHostIds' -e 'executionHostId' src/shared/sidebar-list/sidebar-row-plan.ts src/shared/sidebar-list/project-group-sections.ts src/shared/sidebar-list/visible-worktrees.ts src/renderer/src/components/sidebar | head -n 600
printf '%s\n' '--- targeted duplicate/drag tests ---'
rg -n -C 18 --glob '*.{ts,tsx}' -e 'paired-host' -e 'same-id' -e 'same id' -e 'cross-host' -e 'ssh:ssh' src/renderer/src/components/sidebar src/shared/sidebar-list | head -n 700Repository: stablyai/orca
Length of output: 42383
🏁 Script executed:
set -eu
printf '%s\n' '--- sidebar row plan outline and header construction ---'
rg -n -C 20 --glob '*.ts' -e 'function planSidebarRows' -e 'export function planSidebarRows' -e 'type.*Section' -e 'repo:' -e 'hostId:' src/shared/sidebar-list/sidebar-row-plan.ts src/shared/sidebar-list/project-group-sections.ts src/shared/sidebar-list/*.ts | head -n 900
printf '%s\n' '--- exact planner blocks ---'
nl -ba src/shared/sidebar-list/sidebar-row-plan.ts | sed -n '1,420p'
printf '%s\n' '--- exact project-group section blocks ---'
nl -ba src/shared/sidebar-list/project-group-sections.ts | sed -n '1,260p'Repository: stablyai/orca
Length of output: 42481
🏁 Script executed:
set -eu
printf '%s\n' '--- ordered repo grouping ---'
nl -ba src/shared/sidebar-list/worktree-grouping.ts | sed -n '1,340p'
nl -ba src/shared/sidebar-list/worktree-group-entry.ts | sed -n '1,220p'
printf '%s\n' '--- host section rows ---'
nl -ba src/shared/sidebar-list/host-section-rows.ts | sed -n '1,360p'
printf '%s\n' '--- renderer section-row assembly ---'
rg -n -C 18 --glob '*.{ts,tsx}' -e 'HostSectionRow' -e 'build.*Section.*Rows' -e 'host-header' -e 'appendHost' src/renderer/src/components/sidebar src/shared/sidebar-list | head -n 700Repository: stablyai/orca
Length of output: 42463
🏁 Script executed:
set -eu
printf '%s\n' '--- repo header row construction ---'
rg -n -C 18 --glob '*.ts' -e 'hostWorktreeCounts' -e 'getLaneHostWorktreeCounts' -e 'build.*Header' -e 'type: .header' src/shared/sidebar-list/group-sections.ts src/shared/sidebar-list/row-builders.ts src/shared/sidebar-list/host-labels.ts src/shared/sidebar-list/pinned-group-rows.ts
printf '%s\n' '--- complete group section emitter ---'
nl -ba src/shared/sidebar-list/group-sections.ts | sed -n '1,300p'
printf '%s\n' '--- host count helpers ---'
nl -ba src/shared/sidebar-list/host-labels.ts | sed -n '1,180p'Repository: stablyai/orca
Length of output: 33121
🏁 Script executed:
set -eu
printf '%s\n' '--- project section grouping ---'
rg -n -C 20 --glob '*.ts' -e 'function buildProjectSections' -e 'export function buildProjectSections' -e 'repoIdOf' -e 'displayRepoIdOf' src/shared/project-section-grouping.ts src/shared/project-section-order.ts src/shared/sidebar-list
nl -ba src/shared/project-section-grouping.ts | sed -n '1,360p'Repository: stablyai/orca
Length of output: 26047
🏁 Script executed:
set -eu
printf '%s\n' '--- move project group persistence path ---'
rg -n -C 18 --glob '*.{ts,tsx}' -e 'moveProjectToGroup' -e 'projectGroup.moveProject' -e 'moveProject.*hostId' -e 'hostId.*moveProject' src | head -n 700
printf '%s\n' '--- host context into row rendering ---'
rg -n -C 16 --glob '*.{ts,tsx}' -e 'renderWorktreeSectionHeaderRow' -e 'renderWorktreeVirtualRow' -e 'sectionRows' -e 'hostId' src/renderer/src/components/sidebar/worktree-list | head -n 700Repository: stablyai/orca
Length of output: 42902
🏁 Script executed:
set -eu
nl -ba src/renderer/src/store/project-groups/project-group-mutations.ts | sed -n '90,205p'Repository: stablyai/orca
Length of output: 4963
🏁 Script executed:
set -eu
rg -n -C 8 --glob '*.ts' 'moveProjectToGroup' src/renderer/src/store/project-groups/project-group-mutations.tsRepository: stablyai/orca
Length of output: 1467
Carry the host identity into the project-header drag session.
Host-localized sections can reuse the same repository header under different hosts, but SectionHeader passes only row.repo.id. The ID-keyed repoMap can resolve the other host’s repository, and line 73 then sends that repository’s host. moveProjectToGroup uses the supplied host to select the persisted row, so the move can target the wrong host twin.
Pass the rendered host’s ExecutionHostId through the drag session and use it for the host-qualified repository lookup. Do not derive the host from repoById or bucketKey; the bucket key contains only projectGroupId.
… SSH twin is not WSL-fenced
128ee90 to
d3985f0
Compare
ELI5
Orca allows the same project id to exist twice: once for a project on this computer and once for a project on an SSH host. Some save operations were given only the id, and they changed whichever copy came first in the list. This PR makes the project-row saves name the exact copy they mean. It also adds a CI check that lists every remaining id-only save, and that list is only allowed to get shorter.
What Changed
Before: say a local project and an SSH project share the id
dup, with the local one listed first. You move the SSH project into a group from the sidebar. The renderer finds the SSH copy, but the local IPC call it sends carries only the id. Main then moves the local project into the group and leaves the SSH one where it was. TheprojectGroup.moveProjectRPC had the same problem: it resolved the right row from the selector, then passed onlyrepo.idto the store.After: the SSH project moves and the local one stays put.
Mechanism:
Store.moveProjectToGroupandStore.removeProjectForHostnow take aRepoHostTarget, which is the row's ownid,connectionIdandexecutionHostId(the shapeupdateRepoalready takes since fix(persistence): repo writes take the row's identity, never a bare id #30616). The lookup goes throughfindRepoRowForHostScopedWrite, so the store never picks the first row with a matching id.repo.rm.connectionId). A paired host's own rows are stampedruntime:<env>, so they still match and are grouped instead of being imported a second time.projectGroups:moveProjectIPC gains an optionalhostId, and the renderer sends the host of the row it picked. If a caller sends nohostId, it gets today's behaviour (the first row with that id), the same fallbackrepos:updateuses.repos:removeForHostalready carried a requiredhostId; it now builds the target from that.config/scripts/check-id-only-store-write-ratchet.mjswith baselineconfig/id-only-store-write-baseline.txt. It reads the Store's operation classes fromstore-domain-composition.tsand lists every public write (methods or arrow properties, any ofpublic/override/async, generics included) whose first parameter is named like an id (id,*Id,*Ids,*WorktreeKey/*WorkspaceKey) and isn't typedRepoHostTarget/HostWorktreeTarget. It goes by the parameter's name, so retyping it (Repo['id'], a brand) doesn't get a write off the list. Known limitation: it doesn't see an id inside an object parameter ({ projectId }). A new entry fails CI, and a fixed entry must be pruned. It runs in the static-analysis job and inpnpm lint. Each exemption has its reason written in the script: automation ids, ids that name a host (targetId,environmentId,hostId), feature ids, the cross-hostProjectrow,reorderRepos(it refuses a duplicated id) andreorderReposForHost. Retired worktree names and sparse presets do have twins; the twins share one bucket keyed by repo id, and re-keying it is a non-goal (D7).The baseline goes from 15 to 14 entries: 2 fixed here, plus
removeWorkspaceLineage, which the wider match now finds. The remaining entries are the next two PRs:removeWorkspaceLineage,setWorktreeMeta,setWorktreeMetaForHost,removeWorktreeMeta,setWorktreeLineage,removeWorktreeLineage,migrateWorktreeIdentity,installDeferredOrcadMigrationShellHistoryupdateFolderWorkspace,removeFolderWorkspace,updateProjectGroup,deleteProjectGroup,removeWorkspaceSessionStateForWorktree,snapshotAutomationRunWorkspaceDisplayNameWhy
Point fixes have kept missing call sites: each one fixes the caller someone noticed, and the next id-only caller ships later. With the type change, a caller can no longer compile while handing over a bare id. It has to pass a row, and that row comes from its own selection or from a boundary that knows the host. A required
hostIdparameter would still let a caller fill it from a first-match lookup (decision D-A). The ratchet makes the remaining surface visible, keeps it from growing while families (b) and (c) land, and shows exactly what is left.reorderRepos(string[])is not on the list. With twins present, its permutation check fails and it refuses the reorder rather than writing a row, andreorderReposForHostexists for the host-scoped drag.Follow-up, not in this PR: a header drag reads its host from the id-keyed
repoByIdmap. Carrying the host in the drag session needs the header rows to pass it in, and a merged multi-host header has no single host.Follow-up, not in this PR: for a paired runtime, the renderer still sends
projectGroup.moveProjectas{ repo: projectId }, which keeps the RPC schema unchanged. That host resolves the selector and now writes the row it resolved, but a twin on that host would answerselector_ambiguous. Sending a host-qualified selector there is a separate change.Remote compatibility: no RPC param changed. The IPC field is optional, and an old renderer without it gets today's behaviour. Folder projects follow the same path (
moveProjectToGroupcovers git and folder repos alike).ref-oss check
The review needed more than 3 rounds, so the design was checked against how two widely used editors with remote support store per-host workspace records. Both make the host part of the record's identity: one keys rows on (connection, paths) and gives every record a globally unique id, and the other keys on the full URI, which includes the remote authority. Their writes and removals are therefore addressed by that full identity, or by a globally unique id, and never by a bare id two hosts can share. Orca can't re-key its stores (round 1 D7, D8 and D51), so making every write carry the row's (id, host) identity is the matching approach. The approach doesn't change. The review findings were separate call-site edge cases, not one recurring design flaw. Switching a method's signature to
RepoHostTargetturns every caller that still passes a bare id into a compile error, so callers are found by the compiler rather than hunted by hand; the ratchet only holds the 14 methods not yet switched, and each family PR switches one group and shrinks the baseline (switching all of them at once would work the same way, as a single PR of roughly 1,500 lines).Linked Issue
Part of R4-2 (round-4 SSH hardening, "R4-2 follow-up: other id-only store writes"). No GitHub issue.
Visual Proof
N/A: no UI change. The only effect a user sees is that the right twin moves, and the tests below cover that.
Testing
Twin tests (a local and an SSH row with the same id), each failing before the fix:
persistence-duplicate-repo-id-host-scope.test.ts:moveProjectToGroupmoves the addressed twin;removeProjectForHostgiven the SSH row leaves the local twin. Run against the old store code, 5 tests in this file fail, including the existing host-scoped removal tests.ipc/repos-execution-host-catalog.test.ts: the IPC withhostIdtargets that host's row; withouthostIdit keeps the old first-row behaviour; an invalidhostIdis rejected.runtime-remove-project-host-scope.spec.ts: throughprojectGroup.moveProject, a path selector moves the SSH row, a name selector moves the local row, and a bare duplicated id is refused. The store there is a stub; the real-Store test above covers the store's own lookup.runtime-nested-repo-import-host-twin.test.ts: with an SSH row at the same path, the runtime nested import adds the local repo and moves no SSH row; a paired host's own runtime-stamped row is grouped, not imported again.project-header-drag-commit.test.ts: a drag reorder carries the dragged row's host.repos-project-group-cross-host-move.test.ts: the renderer names the SSH twin's host in the IPC.Ratchet unit test:
config/scripts/check-id-only-store-write-ratchet.test.mjs.Local runs, single worker: src/main/persistence**, src/main/ipc/repos, the runtime project/repository tests, renderer repo and project-group slices, and host-twin isolation. Result: 322 files and 3300 tests pass.
pnpm tcis clean. The full static-analysis job, mirrored locally with the new ratchet added, is clean.orca-runtime.test.ts"sets up an existing folder…" fails locally on main as well, because of the macOS/private/vartmpdir; this PR doesn't cause it.Verified live: nothing. This is an in-process store change, and it is covered by unit tests only.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Codex (
gpt-6.1-sol, high effort, base pinned at the merge-base):runtime:<env>-stamped rows, which would import the same checkout twice. Fixed by excluding SSH rows only, with a regression test.projectGroups:moveProjectchecked the WSL-move fence by repo id before resolving the host, so a fenced local repo also blocked moving its SSH twin. Fixed: the handler resolves the target first and checksassertWslMoveRepoWritable, which exempts non-local hosts. Tests cover both twins under a seeded fence.Agent skill upstream boundary
Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)