Repository navigation
fix(worktree): require host-qualified identity for workspace deletion (STA-4448) - #14930
brennanb2025 wants to merge 3 commits into
Conversation
… (STA-4448) A workspace id is `repoId::path` with no host component, so the local host, an SSH host and a paired runtime can all publish the SAME id. Removal routing short-circuits to the ACTIVE workspace's host, and `activeWorktreeId === id` holds for a colliding id no matter which row was confirmed — so confirming the remote row issued the destructive IPC against the LOCAL checkout and destroyed its uncommitted work, while the row the user picked survived. #14731 guarded a caller (the cleanup preflight), which is why every other delete surface sailed past it. Make host qualification MANDATORY at the one chokepoint they all share instead: `removeWorktree` now takes a host-qualified `WorktreeRemovalTarget` and fails closed whenever the confirmed host is not where the removal would actually land. Omitting the host is a type error, so a future entry point cannot be written unguarded. Fail closed, never reroute — rerouting a colliding row to its right host is #14606's job, and it can adopt this contract rather than reconcile with it. forget-local stays exempt: it removes no files or Git state, and it must keep working precisely when the owning host is gone and routing cannot resolve. Refusal copy reuses the strings #14731 added, so both delete surfaces explain a wrong-host refusal in the same words. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 6 remain after this review. 📝 WalkthroughWalkthroughWorktree removal now uses structured targets with execution-host identity. Shared validation rejects host collisions and unresolved routing before destructive removal, while Merge Risk: 🟡 Moderate · up to The PR hardens host qualification for deletion, but batch deletion still has a bounded correctness risk: using only the worktree ID can drop the selected row when two hosts share that ID. This should be fixed or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/shared/worktree/removal.ts (1)
127-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the host-qualification and collision-test comments. Keep the non-obvious host-collision invariant and filesystem/transport-routing rationale, but remove step-by-step walkthroughs and fixture details that are already expressed by the code, test names, and assertions. Prefer brief comments focused on non-obvious behavior.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4094f30-a842-46a8-8ade-9c13f2be104f
📒 Files selected for processing (29)
src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsxsrc/renderer/src/components/sidebar/ForgetSshWorkspaceDialog.tsxsrc/renderer/src/components/sidebar/delete-worktree-flow.test.tssrc/renderer/src/components/sidebar/delete-worktree-flow.tssrc/renderer/src/components/sidebar/delete-worktree-parallel-flow.test.tssrc/renderer/src/components/sidebar/ssh-host-remove-workspaces.tssrc/renderer/src/components/sidebar/worktree-delete-execution.tssrc/renderer/src/components/sidebar/worktree-delete-host-qualification.test.tssrc/renderer/src/components/sidebar/worktree-delete-request.tssrc/renderer/src/components/status-bar/WorkspaceSpaceManagerPanel.tsxsrc/renderer/src/lib/worktree-operation-route.tssrc/renderer/src/store/slices/editor-state-worktree-purge-leak.test.tssrc/renderer/src/store/slices/generation-records-worktree-removal-leak.test.tssrc/renderer/src/store/slices/native-chat-launch-draft-teardown.test.tssrc/renderer/src/store/slices/store-worktree-removal-cascade.test.tssrc/renderer/src/store/slices/tab-worktree-orphan-map-purge-leak.test.tssrc/renderer/src/store/slices/workspace-cleanup-removal-preflight.test.tssrc/renderer/src/store/slices/workspace-cleanup-wrong-host-removal-guard.test.tssrc/renderer/src/store/slices/workspace-cleanup.tssrc/renderer/src/store/slices/worktree-helpers.tssrc/renderer/src/store/slices/worktree-removal-host-collision-fixture.tssrc/renderer/src/store/slices/worktree-removal-host-qualification.test.tssrc/renderer/src/store/slices/worktree-removal-maps-leak.test.tssrc/renderer/src/store/slices/worktree-removal-result.tssrc/renderer/src/store/slices/worktrees-remote-runtime-removal.test.tssrc/renderer/src/store/slices/worktrees-removal-state-cleanup.test.tssrc/renderer/src/store/slices/worktrees/teardown/remove-worktree-host-guard.tssrc/renderer/src/store/slices/worktrees/teardown/remove-worktree.tssrc/shared/worktree/removal.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review.
| targets: readonly Pick< | ||
| Worktree, | ||
| 'id' | 'instanceId' | 'displayName' | 'repoId' | 'path' | 'hostId' | ||
| >[], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use host-qualified identity for batch deletion.
hostId is now part of the deletion identity. However, Line 46 still deduplicates by target.id, and Line 82 still resolves the current row by that ID. If two confirmed targets have the same ID on different hosts, the later target replaces the earlier target. One selected workspace is then skipped.
Key deduplication and freshness lookup by both id and hostId. Add a regression test with two hosts that share one worktree ID.
… host Adds the end-to-end sidebar case #14606 had no coverage for at all: the reported user path, driven through the real runWorktreeDelete / runWorktreeDeletesInParallel funnel into the real store action, with real per-host temp directories that the transports actually rm -rf. The decisive assertion is two-sided — the confirmed SSH checkout is GONE and the ACTIVE local one SURVIVES. #14930 could only prove the second half. Two production gaps this surfaced: - the two sidebar row entry points (card quick action, context menu) passed a bare id, so with two rows rendered either one deleted the same workspace. They now name their row's host via expectedHostId. - the batch path re-checked staleness through the id-keyed map, so on a collision it read the other host's row, found a mismatched instanceId, and silently dropped a delete the user confirmed. It now revalidates on (id, host) via getWorktreeOnHostFromState. Cleanup guard gains force coverage via describe.each — force is the least re-checked destructive path — plus a no-false-refusal control proving an ordinary single-host delete still works under force.
Visual proof — what the refusal looks like, and a finding that changes the case for this PRRig: isolated dev instance per branch ( This PR (
|
| case | main | this PR |
|---|---|---|
| route unresolvable | refuses ✅ | refuses ✅ |
| route resolves, but to a different host than the user confirmed | cannot detect — main takes no confirmed host at all; removeWorktree(worktreeId, force, options) has nothing to compare against |
refuses ✅ |
The second row is the data-loss case: the delete lands on a real host that isn't the one the user pointed at. verifyWorktreeRemovalHost is exactly that comparison:
if (args.routeExecutionHostId === args.confirmedExecutionHostId) return { kind: 'allowed' }
return { kind: 'refused', reason: collides ? 'collision' : 'unresolved' }My fixture cannot exercise it. The synthetic ssh:qa-linux host does not resolve, so routing returns null and main's pre-existing guard fires first. Reaching the real case needs two genuinely reachable hosts — a local checkout plus a live SSH host at the same path. That leg is unrun, and it is the leg that actually justifies this PR.
Pre-existing defect, NOT introduced here
On both branches the refused row is left permanently showing a greyed-out spinner and "Deleting…" — deleteStateByWorktreeId keeps { isDeleting: true, phase: 'deleting' }, still stuck 40s later. The toast that carries the explanation auto-dismisses, so the end state a user is left with is a workspace that looks like it is mid-delete forever. Reproduced identically on main, so it is not a regression from this PR — but it is what the refusal actually looks like once the toast goes.
Fixture caveat
An earlier malformed seed of mine (a twin row spread from an undefined source, so no path) crashed WorktreeCard via parseWorkspaceKey and latched the sidebar error boundary. That was my fixture's fault, not a product defect — recorded so nobody chases it. All results above are from a clean re-seed after a reload.
Follow-up: the wording above is being replaced — #15178The toast in the screenshots above is bad copy and it is now fixed in #15178 (off It had three problems: "Workspace identity is ambiguous across hosts" is implementation vocabulary; "Refresh projects and try again" is a remedy that cannot work, because refreshing does not disambiguate two real checkouts; and The new toast
The suggested remedy is verified rather than assumed: removing the checkout on the machine clears the collision on Orca's next scan, with no orphaned row and no Force Delete needed. The stuck "Deleting…" row noted above is still unaddressed and still reproduces on |
The refusal said 'Workspace identity is ambiguous across hosts. Refresh projects and try again.' — implementation vocabulary, and the remedy does not work: refreshing cannot disambiguate two real checkouts, and the View action opens the diff by an id both machines share. Say what happened to their workspace, confirm nothing was lost, and give the one step that does resolve it. Verified: removing the checkout on the machine clears the collision on the next scan, with no orphaned row.
The refusal wording is fixed in this PR now — fd52aefThe toast in my screenshots above was bad copy. Rewritten and pushed to this branch. Three problems with the old text:
The new toast
The remedy is verified, not assumed: I removed the checkout on the machine and Orca's row resolved on the next scan — no orphaned row, no Force Delete needed. Worth checking before putting an instruction in shipped copy. What changed
typecheck 0 · sidebar + shared/worktree suites 262 files / 2,337 tests green · copy confirmed in a live dev instance (screenshot above). Still openThe stuck "Deleting…" row from my earlier comment is unchanged and still reproduces on |
Callers mark rows deleting up front for immediate sidebar feedback, but both refusals return before removeWorktree's try/catch, so nothing ever cleared the flag. The toast auto-dismisses after 10s and the workspace was left on a 'Deleting…' spinner indefinitely — with the explanation already gone. Pre-existing on main; the new copy made it more visible.
|
Superseded by #15013, which merged as 64de8dd. This PR's core was a fail-closed refusal when a colliding id could not be routed. #15013 does the better thing per Brennan's ruling — it routes the delete to the confirmed host instead of refusing — so the refusal logic here is redundant. The two things on this branch that #15013 did not include are handled separately:
Branch kept for history; nothing here is stranded. |
ELI5
A workspace's id is just
repoId::path. It has no host in it. So your laptop, an SSH box and a pairedruntime can all publish the same id for what are three different checkouts. When you clicked delete
on the remote row, Orca looked up "which host?" by asking the active workspace — and got your local
checkout. It deleted that instead, uncommitted work and all, while the row you actually picked stayed on
screen. This PR makes the delete carry the host you confirmed, and refuses when the delete would land
somewhere else.
What Changed
Host qualification is now mandatory at the single chokepoint every delete surface shares,
removeWorktree.src/shared/worktree/removal.ts— newWorktreeRemovalTarget({ id, executionHostId }),toWorktreeRemovalTarget(), and the pure verdict functionverifyWorktreeRemovalHost().executionHostIdis required but nullable:nullis a caller stating "this row declares nohost" (legacy/snapshot rows, folder workspaces), not a caller forgetting the field.
remove-worktree.ts—removeWorktree(target, …)now takes the qualified target and calls the newrefuseWrongHostWorktreeRemoval()guard before any destructive work. The confirmed host outranks theroute.
worktree-operation-route.ts—collectWorktreeOwnerExecutionHostIds()enumerates every host thatclaims an id (unqualified rows contribute nothing, so migration can't invent collisions).
worktree-delete-request.ts—WorktreeDeleteIdentitycarrieshostId, and batch-target resolutionrejects a request whose confirmed
hostIdno longer matches the row a refresh swapped in.worktree-removal-result.ts— extractedRendererRemoveWorktreeResultout ofworktree-helpers.ts(import-cycle avoidance; no behavior change).
Omitting the host is a type error, so a future delete entry point cannot be written unguarded.
Why
P0 data loss. A workspace id is
repoId::pathwith no host component, so local, SSH and pairedruntime rows can share one id. Removal routing short-circuits to the ACTIVE workspace's host, and
activeWorktreeId === idholds for a colliding id no matter which row was confirmed — so confirming theremote row issued the destructive IPC against the local checkout and destroyed its uncommitted
work, while the row the user picked survived.
Why not another caller-side guard. #14731 guarded a caller (the cleanup preflight), which is
exactly why every other delete surface sailed straight past it. Guarding one more caller would have the
same shape and the same failure mode. This puts qualification at the one chokepoint they all share and
lets the type system enforce it.
Approach (a): fail closed, never reroute. When the confirmed host is not where the removal would
land, refuse. Rerouting a colliding row to its correct host is #14606's job and needs evidence this
layer does not have — and #14606 can now adopt this contract rather than reconcile with it.
forget-localis deliberately exempt. It removes no files and no Git state; it drops the workspacefrom Orca's own records. It must keep working precisely when the owning host is gone and routing can
no longer resolve — which is the one situation the guard would refuse in.
Refusal copy reuses #14731's existing strings (
auto.store.slices.workspace.cleanup.hostCollision/.hostUnresolved). No new user-facing strings;en.jsonis untouched.Per-caller verdict table (all 8 destructive callers)
delete-worktree-flow.ts→ skip-confirm deletetoWorktreeRemovalTarget(target)from the resolved rowworktree-delete-execution.ts→runWorktreeDeleteWithToast(confirmed delete, and the batch path)WorktreeRemovalTarget; batch usestoWorktreeRemovalTarget(target)worktree-delete-execution.ts→ in-toast Force Delete retrytargetDeleteWorktreeDialog.tsx→ in-dialog Force DeletecurrentWorktrees; returns early if the row is gone rather than force-deleting a bare idworkspace-cleanup.ts→ cleanup removalscannedHostId— the host the scan actually inspectedWorkspaceSpaceManagerPanel.tsx→ Space-manager force recovery (not in the ticket)worktree.executionHostIdfrom the scan row (the panel lists one row per host)ssh-host-remove-workspaces.ts→ clear all workspaces for an SSH host (not in the ticket)toSshExecutionHostId(resolution.targetId)— every row in the resolution is pinned to that targetForgetSshWorkspaceDialog.tsx→ forget / delete for a disconnected SSH host (not in the ticket)Callers 6, 7 and 8 are the three the ticket does not mention. They were reachable by the same mechanism.
Out of scope, called out so it isn't mistaken for covered: the mobile client calls
worktree.rmdirectly over the wire and is not covered by this renderer guard. It is not exposed to this defect's
mechanism — a mobile session is bound to one host, so the transport is the host selection — and
qualifying it would be a wire change requiring capability negotiation per
docs/reference/remote-wire-compatibility.md.Linked Issue
STA-4448. Related: #14731 (the caller-side guard this generalizes), #14606 (rerouting — can adopt this
contract).
Fixes #
Visual Proof
N/A— no new UI and no new copy. The only user-visible change is that a delete which would have hitthe wrong host now refuses using #14731's existing refusal strings instead of destroying the wrong
checkout. See KNOWN GAPS below regarding rendered-UI validation.
Testing
Red-first evidence (recorded by the implementing agent, on this exact code):
the ACTIVE local checkout must survive … removal was routed to hostId=local: expected false to be true— i.e. the test observes the actual data-loss mechanism, not a message string.
tests fail for the real reason, not because a type changed shape.
Automated (all run locally in this worktree):
pnpm run typecheck— clean across all 3 projects (node, cli, web).worktree-removal-host-qualification.test.ts(12 new),worktree-delete-host-qualification.test.ts(4 new),
worktrees-remote-runtime-removal.test.ts,store-worktree-removal-cascade.test.ts,workspace-cleanup-wrong-host-removal-guard.test.ts,delete-worktree-parallel-flow.test.ts.main:src/renderer/src/store/slices— 263 files / 2624 tests passed;src/renderer/src/components/sidebar— 249 files / 2179 tests passed. Zero failures, sonothing to classify as pre-existing.
oxlint --type-awareandoxfmton all 29 changed files. Note: thecheck:code-quality:changed/check:react-doctor:changedwrappers crash on this box — it runsnode v26 against the repo's required node 24, so pnpm's engine
WARNlands inside oxlint's JSONstdout and breaks the parse. That is pre-existing and unrelated to this change, so the underlying
oxlint scans were run directly instead. The one oxlint finding
(
workspace-cleanup.ts:217 no-unnecessary-boolean-literal-compare) is on a line this PR does nottouch and blames to fix(workspace-cleanup): refuse removal when the owning host is not certain (STA-4343) #14731 on
main— pre-existing.Rebase note, and it doubles as evidence the approach works: rebasing onto current
mainpulled intwo test call sites
mainhad added while this branch was based on an older commit. They were stillpassing a bare string id. The type system caught both (
TS2345) instead of letting them reachremoveWorktreeand crash onremovalTarget.id. That is exactly the failure mode approach (a) waschosen to make impossible for future entry points. Both were migrated to their row's actual host
(
localandssh:shared-target) rather thannull, preserving the two-distinct-hosts scenario thetest is named for.
Platforms: the change is renderer-side host-identity logic with no path or shell handling, so it is
platform-agnostic; verified on Linux. SSH and paired-runtime rows are covered directly by the new
tests, and folder workspaces are covered by the unqualified-target path (still deletes while the id has
at most one declared owner).
KNOWN GAPS — stated plainly
$electronskill is not installed on this box and there is nodesktop session. Per standing orders, computer-use was not substituted. Risk is low because
there is no new UI and no new copy, but this is a real gap, not a formality.
test suite, not from two live hosts sharing a path.
AI Disclosure
Review
Ensure no issues in: Security, Cross-platform support (Linux, Windows, Mac), Remote SSH, Mobile, general
backwards compatibility, performance.
host's uncommitted work.
executionHostId: null) targets still delete while the idhas at most one declared owner, so pre-host-qualified snapshot rows and folder workspaces keep
working. No wire change; no IPC signature change.
delete path only.
Checklist
N/Awith reasonpnpm typecheckand the relevant test suites pass locally; lint run directly (see Testing).pnpm buildnot run locally — left to CI.Author
🤖 Generated with Claude Code