[fettpl]

ELI5

A workspace id is repoId::path with no host. Local and SSH can publish the same id for two different checkouts. An unqualified lookup used to pick the first row, so deleting the SSH card could wipe the local directory. If two hosts share an id and nobody names the host, we now refuse to guess.

What Changed

getWorktreeOnHostFromState fail-closes when hostId is omitted and more than one host publishes the id:

• 0 rows → undefined
• 1 row → that row (unambiguous)

• 1 rows → undefined (not rows[0])

• named hostId → rows.find(row => row.hostId === hostId) as before

runWorktreeDelete already looks up through this selector, so a colliding unqualified delete shows the existing "Workspace list changed" toast and does not call remove. Sidebar/row callers already pass expectedHostId: worktree.hostId.

Why

STA-4343: deleting the SSH row without a host destroyed the local checkout at the same path. First-wins is silent-wrong inventory for a destructive action. Fail-closed is the honest outcome until the caller names the host. Follow-up (not this PR): key worktreeMap on composeWorktreeHostIdentity.

Related open #17799 is host-blind folder connectionId, not this map. Do not merge them.

Linked Issue

Fixes #18279

Visual Proof

N/A — no UI or interaction change. The user-visible difference is a stale-list toast instead of deleting the wrong host's checkout when two hosts share an id and the caller omitted the host.

Testing

• pnpm test src/renderer/src/store/selectors.test.ts src/renderer/src/store/worktree-host-collision-index.test.ts src/renderer/src/components/sidebar — 290 files, 2479 passed

• New selector tests failed on current rows[0], then passed after the fail-close

• New delete-funnel test: two-host collision with no expectedHostId leaves both disk markers, routes to no host, toasts, keeps both store rows

• Existing qualification tests still delete only the named SSH or local row

• pnpm tc:web — exit 0

• pnpm run check:code-quality:changed — 0 findings

• Platforms actually exercised: unit tests on macOS. Collision fixtures use local vs ssh:ssh-1 / ssh:build-box and path.join/os.tmpdir. No Electron UI session.

• I manually tested these changes locally

• Automated tests added/updated, or explained why not below

AI Disclosure

Internal contributor — skip.

Review

Code-reviewer subagent: ready to merge. Selector fail-closes exactly as specified; delete funnel reuses the existing stale-list toast; tests would fail on rows[0].

• Cross-platform: no OS-specific paths, shortcuts, or spawn. Fixture dirs use path.join and os.tmpdir.
• SSH/remote: named SSH host still deletes only the SSH checkout; unqualified collision deletes neither; local checkout survives an SSH-card delete (existing qualification tests).
• Security: fail-closed identity for a destructive action; does not authorize a delete from an ambiguous id.
• Performance: one extra rows.length check on an already-indexed list.
• UI: no visual change.
• worktree-runtime-owner.ts left alone: delete does not use it; no test showed a destructive fall-through to 'local'.

Agent skill upstream boundary

• Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

A collision must not delete the local checkout when the user clicked the SSH card.

Checklist

• This PR is small and focused
• I explained what changed and why (including ELI5)
• Before/after screenshots or videos attached for UI changes, or N/A with reason
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
• pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Local verification used the plan commands (pnpm tc:web, targeted tests, check:code-quality:changed). Full pnpm lint / pnpm build skipped per executor instructions.

[coderabbitai]

Warning

Review limit reached

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: de2689ee-19c7-499f-a199-37979eedde5d

📥 Commits

Reviewing files that changed from the base of the PR and between 337e768 and 54bc21f.

📒 Files selected for processing (4)

• src/renderer/src/components/sidebar/delete-worktree-flow.ts
• src/renderer/src/components/sidebar/worktree-delete-host-qualification.test.ts
• src/renderer/src/store/selectors.ts
• src/renderer/src/store/worktree-host-collision-index.test.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[pullfrog]

✅ No new issues found.

Reviewed changes

• Selector fail-close — getWorktreeOnHostFromState returns undefined when hostId is omitted and two hosts share the id, instead of first-wins rows[0].
• Delete funnel comment — runWorktreeDelete's STA-4343 rationale updated to reflect the fail-closed unqualified path.
• Selector tests — four tests pinning each branch (unqualified collision → undefined, named host → that row, single unambiguous → row, empty → undefined).
• Funnel test — new end-to-end case asserting both disk markers survive, nothing is routed, the stale-list toast fires, and both store rows remain when no host is named.

The change is correct and well-scoped. worktreeId is repoId::path with no host, hostId is optional on legacy rows, and worktreesById buckets distinct hosts per id, so rows.length !== 1 reliably means a genuine two-host collision rather than a same-host duplicate. Every caller of this selector is a destructive path, so fail-closed is the right behavior everywhere; row callers that conditionally omit expectedHostId (e.g. HostedReviewActions.tsx:158) only land here for legacy unqualified rows, which fail-closing correctly refuses. Named-host deletes (rows.find) are untouched. I traced the one subtle secondary effect — the focus-commit check at worktree-delete-execution.ts:176 now sees activeRow as undefined on an unqualified collision — and it converges safely because focusNextWorktreeAfterActiveDelete no-ops unless activeWorktreeId is already null.

The new tests would fail against the old rows[0] behavior (the unqualified collision would resolve to the first-seeded local row and delete it), so the coverage is meaningful rather than theatre.

  ^{｜ View workflow run ｜ Using DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏}

[nwparker]

Thanks @fettpl for catching this and for the clear fail-closed write-up! This is already fixed on main by #33818 (0efc989), which carries your approach forward (you're credited as co-author): getWorktreeOnHostFromState now resolves through findSoleHostCopy, so an unqualified id that matches copies on more than one host returns undefined instead of the first row, and runWorktreeDelete refuses rather than removing the other host's checkout. If you can still reproduce a wrong-host delete on the latest build, please comment or reopen.