Sign up
View stargazersSign up

fix(server): refuse worktree removal while another thread references it#9185

closed1
main
GitHub fork: fix/worktree-removal-shared-threads
openedSep 2, 2026byNNurozen
12 files changed
View on GitHubTwo-way sync enabledPR last imported 13d ago

Deleting a thread could remove a git worktree that another thread still uses: the removal path and the checkpoint branch-drift check both consulted only the active shell, so archived threads sharing the worktree were treated as gone, and the legacy sidebar's bulk delete pre-seeded its deleted-set with the whole batch — the first deletion treated still-alive batch mates as deleted and removed a worktree they still pointed at.

The vcsRemoveWorktree handler now refuses while any non-deleted thread (archived included) references the path, branch-drift adoption counts archived siblings, and the legacy sidebar grows its deleted-set only as deletions actually land (matching the current sidebar). Covered by a pure guard unit test, a reactor test proving the archived-sibling case fails on the old logic, and a wire-level server.test.ts case.

Built with Claude Fable 5 on Claude Code.

Note

Medium Risk Changes worktree removal and branch-drift behavior on shared paths; callers that previously removed worktrees while only archived threads referenced them will now get errors, but the guard reduces risk of breaking concurrent thread workspaces.

Overview Fixes cases where a git worktree could be removed or branch metadata updated incorrectly because archived threads (and bulk-delete bookkeeping) were ignored.

Server: Adds ProjectionSnapshotQuery.listThreadIdsByWorktreePath to resolve all non-deleted threads at an exact worktree path (archived included). vcsRemoveWorktree now calls this before git worktree remove and fails with a GitCommandError when any thread still references the path; git status refresh runs only after an allowed removal. CheckpointReactor uses the same lookup instead of the active shell snapshot when deciding whether a worktree is shared, so branch-drift adoption does not run on shared paths when the only sibling is archived.

Web: LegacySidebar bulk delete no longer pre-fills deletedThreadKeys with the whole selection; each thread is added only after its delete succeeds, so orphaned-worktree cleanup does not treat still-live batch mates as already gone.

Tests cover the WS guard (including archived owner), reactor behavior with an archived co-tenant, and updated projection mocks.

Reviewed by Cursor Bugbot for commit 896ed9a0db5fabf4925eeca2fcaba7006d9b2051. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Refuse worktree removal and branch adoption when another thread references the path

  • Adds a ProjectionSnapshotQuery lookup that returns all non-deleted thread ids (including archived threads) matching an exact worktree path, backed by a new SQL query and request schema.
  • The vcsRemoveWorktree RPC in ws.ts now calls this lookup before removing a worktree; if any thread references the path, it fails with a count-based GitCommandError and skips the Git removal workflow.
  • The branch-drift handler in CheckpointReactor.ts replaces its shell-snapshot scan with the same lookup and skips branch adoption when any returned thread id differs from the current thread.
  • Fixes batch thread deletion in LegacySidebar.tsx so the deleted-key set starts empty and grows only after each individual delete succeeds, preventing later deletions from discounting still-alive batch members.
  • Behavioral Change: ProjectionSnapshotQueryShape gains a new required method; all in-tree mocks and fixtures are updated, but out-of-tree implementations of the service interface will need to add it.

Macroscope summarized 896ed9a.

Checks

8 successful · 8 skipped
All checks passed
896ed9a
apps/server/src/ws.tsline 2262Outdated · 2b78d76Open thread
……@@ -2255,7 +2259,29 @@ const makeWsRpcLayer = (
22562260 observeRpcEffect(
22572261 WS_METHODS.vcsRemoveWorktree,
2258- gitWorkflow.removeWorktree(input).pipe(Effect.tap(() => refreshGitStatus(input.cwd))),
2262+ Effect.all([
MmacroscopeappBoton line 2262Sep 2, 2026

🟠 High src/ws.ts:2262

The ownership check can return no matching thread while a thread moves between active and archived, allowing gitWorkflow.removeWorktree(input) to delete a worktree still referenced by that thread. getShellSnapshot() and getArchivedShellSnapshot() are read independently, so an unarchive between those reads makes the thread absent from both results; perform the ownership lookup and removal decision atomically, or use one query covering all non-deleted rows.

✅ Resolved in 896ed9a0db5fabf4925eeca2fcaba7006d9b2051

NNurozenSep 2, 2026

Fixed in 896ed9a0d — the two independent snapshot reads are gone; ownership now comes from a single atomic query (listThreadIdsByWorktreePath) over every non-deleted row, so a thread moving between active and archived can no longer fall between reads. The drift-adoption check in CheckpointReactor uses the same query.

MmacroscopeappBotSep 2, 2026

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

apps/server/src/vcs/worktreeRemovalGuard.tslines 10–38Outdated · 2b78d76Open thread
……@@ -0,0 +1,38 @@
35+ cwd: input.cwd,
36+ detail: `failed to check which threads reference worktree ${input.path}`,
37+ cause,
38+ });
MmacroscopeappBoton lines 10–38Sep 2, 2026

Both exports here only construct a GitCommandError from their arguments, and each has exactly one call site in ws.ts (worktreeOwnershipQueryError is used once inside Effect.mapError). Per the error conventions, a helper whose only behavior is (...args) => new SomeError({ ...args }) — including a one-off alias used with mapError — should not be introduced; construct the error at the failure boundary so its attributes and cause stay visible, and drop this module (and its test) once the construction lives in the handler.

If you want the sharing check itself to remain unit-testable, keep a pure predicate/selector (e.g. returning the referencing thread ids or a boolean) and build the GitCommandError inline in the vcsRemoveWorktree handler, rather than exporting a pre-built error factory.

Posted via Macroscope — Effect Service Conventions

NNurozenSep 2, 2026

Fixed in 896ed9a0d — the helper module is dropped and both GitCommandError constructions now live inline at the vcsRemoveWorktree failure boundary (the lookup failure keeps its cause). The sharing decision reduced to referencingThreadIds.length !== 0, so no separate predicate was needed.

MmacroscopeappBotSep 2, 2026

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

apps/server/src/vcs/worktreeRemovalGuard.test.tslines 9–37Outdated · 2b78d76Open thread
……@@ -0,0 +1,38 @@
34+ const error = worktreeOwnershipQueryError({ cwd: "/repo", path: "/wt" }, cause);
35+ expect(error._tag).toBe("GitCommandError");
36+ expect(error.cause).toBe(cause);
37+ });
MmacroscopeappBoton lines 9–37Sep 2, 2026

These tests only assert that two thin constructors return a GitCommandError; the behavior this PR actually changes is untested: the vcsRemoveWorktree handler refusing removal when an archived thread still references input.path (and allowing it otherwise), and followWorktreeBranchDrift now treating an archived sibling as a shared worktree. Consider replacing these with focused tests at those boundaries — e.g. a ws.ts handler test with a stub ProjectionSnapshotQuery whose archived snapshot contains a thread on the same worktreePath, and a CheckpointReactor test asserting no thread.meta.update dispatch in that case.

Posted via Macroscope — Effect Service Conventions

NNurozenSep 2, 2026

Fixed in 896ed9a0d — the constructor-only tests are gone with the module; the PR now tests the real boundaries instead: a wire-level server test asserting vcsRemoveWorktree refuses (and never invokes the git driver) while a thread still references the path, and a CheckpointReactor test asserting branch-drift adoption is skipped when an archived sibling shares the worktree (that case fails against the pre-fix logic).

MmacroscopeappBotSep 2, 2026

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

MmacroscopeappBotSep 2, 2026
Synced from GitHub

Approvability

Verdict: Approved at 896ed9a

Macroscope's review found this PR approvable — This is a focused bug fix that prevents worktree removal or branch metadata updates when archived threads still reference the same path, while correcting bulk-delete bookkeeping. The change is localized, has targeted boundary tests, and introduces no schema, deployment, security, billing, or default-setting changes.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

Added 1 commit

  • fix(server): refuse worktree removal while another thread references it896ed9a
MMnigosapproved these changesSep 2, 2026
Synced from GitHub

Ran the tests locally and sanity checked the claims. The archived-sibling reactor test is a real regression test: with just the CheckpointReactor change reverted it fails, and it passes again with the fix. I also grepped for callers of vcsRemoveWorktree and the only one is the thread deletion flow in useThreadActions, so the new guard shouldn't block any legitimate removal.

One small thing I noticed, not a blocker: the guard compares worktree paths exactly, so on macOS a /tmp vs /private/tmp mismatch would slip past it. The old shell snapshot check worked the same way, so nothing new here, just worth knowing.

TtheoOct 1, 2026

Note

🤖 GPT-6 Astra responding on behalf of Theo

We're closing this as part of a cleanup of stale PRs. Thank you for the time and care you put into this contribution.

A local merge check found conflicts in 7 files against the current target branch. The latest implementation commit is from 2026-09-02, and there has been no contribution update in over a week.

At this point, regenerating a focused patch against the current intended base would be easier to review and maintain than updating this PR. We appreciate the work and will keep this thread as a reference. If you revisit it, please check the current direction with us and link the fresh PR back here.