Skip to content

Add safe worktree inventory, pruning, and revival - #4742

Open
juliusmarminge wants to merge 1 commit into
mainfrom
t3code/plan-worktree-management
Open

Add safe worktree inventory, pruning, and revival#4742
juliusmarminge wants to merge 1 commit into
mainfrom
t3code/plan-worktree-management

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Jul 28, 2026

Copy link
Copy Markdown
Member

What Changed

  • Added server-side worktree inventory and safety classification.
  • Added safe manual and bulk pruning with dirty and active-worktree safeguards.
  • Added periodic auto-pruning with configurable inactivity and orphan policies.
  • Added worktree revival before starting turns when a worktree is missing.
  • Added thread-deletion orphan cleanup and the Worktrees settings page with filtering, status badges, policy controls, and destructive-delete confirmations.
  • Added contracts, RPC support, integration coverage, and real Git-backed service tests.

Why

Worktrees accumulated indefinitely, especially for web users and settled threads, because cleanup was limited to a desktop-only deletion flow. This change provides visibility and controlled cleanup while preserving branches and checkpoint references so pruned worktrees can be recreated safely when needed.

UI Changes

Added a Worktrees settings page showing managed worktree status, linked threads, pruning eligibility, attention blockers, filters, bulk pruning, row-level deletion, and global auto-prune policies.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Agent Transcript

Expand

Assistant

Here's what the codebase actually has today, then a proposed feature set. The short version of the current state: there is almost no worktree management — which matches your 186-worktree situation.

Current state (from a codebase sweep):

  • There's no worktree table or registry. A worktree exists only as a denormalized worktree_path string on projection_threads (ProjectionThreads.ts:41). Nothing records creation time, last use, or dirty state.
  • The only cleanup path is the interactive "delete worktree too?" dialog when you delete the last thread using it (useThreadActions.ts:253-397) — and it's desktop-only (needs localApi), so web users always leak worktrees. Server-side thread.delete never touches the worktree.
  • git worktree prune is never called anywhere; git worktree list is only used to annotate the branch list.
  • Settling is purely a projection-level soft-hide (settled_override/settled_at, decider at decider.ts:448-507) — so as you said, settling means worktrees now live forever.
  • Checkpoint refs (refs/t3/checkpoints/...) live in the shared ref store, not per-worktree, so deleting a worktree does not lose checkpoints — which is what makes revival viable.

Proposed features

1. Worktree inventory (the foundation). A server-side service that scans git worktree list --porcelain per project and joins against thread rows to produce, per worktree: linked threads + their settled state, dirty/clean, ahead/unpushed counts, PR state (merged?), last activity (last thread activity or dir mtime), and orphaned (no thread references it at all — your existing 186 will be full of these from deleted threads). I'd derive this on demand with a short cache rather than adding a persisted table — worktrees are filesystem truth and a table will drift. The status pieces already exist (localStatus/remoteStatus in GitManager.ts).

2. Worktrees settings page (your observability ask). Follows the appearance-settings pattern exactly (route file + panel in SettingsPanels.tsx + nav entry in SettingsSidebarNav.tsx). List with badges (dirty, unpushed, merged, orphaned, settled-only), per-row delete, and a "prune all safe" bulk action.

3. A "safe to prune" classification — this is the crux. A worktree is auto-prunable when: no active (unsettled) thread references it, the working tree is clean, and the branch is merged or fully pushed. Everything else needs explicit user action — with one big enhancement below.

4. Safety snapshot before deletion. Before removing a worktree (even a dirty one, if the user forces it), commit the dirty state to a backup ref like refs/t3/worktree-backup/<branch>. This makes every prune lossless and makes auto-pruning much less scary — you can loosen the policy because deletion is reversible.

5. Auto-prune sweep + policy. A periodic server loop on the ProviderSessionReaper pattern (Effect.forkScoped + Schedule.spaced, registered in serverRuntimeStartup.ts) that applies a configurable policy — Zortos's "delete after N days of inactivity" (say default 14, options like 7/14/30/never), evaluated only against safe-to-prune worktrees whose threads are all settled or deleted. Settling doesn't prune immediately; it just starts the clock. Also runs git worktree prune for stale admin entries.

6. Revival on unsettle. The thread keeps its branch + worktreePath metadata even after prune. When a thread unsettles (explicitly, or via the existing auto-unsettle-on-activity in the decider) and its path is gone, bootstrap.prepareWorktree recreates the worktree at the same path from the still-existing branch, and restores the backup ref if one exists. Since branches and checkpoint refs survive pruning, this should be genuinely lossless.

7. Move orphan cleanup server-side. Replace the desktop-only dialog mechanism with a server-side removal path so web gets it too (keep the dialog as UX, but the server does the work).

8. t3 worktrees list|prune CLI. Net-new (nothing exists in apps/server/src/cli/), but cheap once the inventory service exists — and it's the practical tool for digging out of the current 186-worktree hole headlessly. Worth peeking at worktrunk per Max's suggestion for list-view/status ideas; the safety model above is roughly what it does.

Open decisions for you

  1. Branches too, or worktrees only? Pruning 186 worktrees still leaves 186 t3code/* branches (plus their checkpoint refs). I'd prune worktrees only in v1 — branches are cheap, and keeping them is what makes revival trivial. Merged-branch deletion could be an opt-in second tier later.
  2. Dirty worktrees under auto-prune: never touch them (conservative), or snapshot-then-prune once they've been inactive long past the threshold (my lean, given feature 4 makes it lossless)?
  3. Policy granularity: one global "prune after N days" setting, or per-project? Global feels right for v1.
  4. Does deleting a settled thread now silently prune its orphaned worktree (no dialog, since we have safety snapshots), or keep asking?

My suggested build order: inventory service → settings page (immediate observability win + manual bulk-prune for your 186) → safety snapshot + server-side removal → auto-prune sweep → revival → CLI. The first two alone solve the acute pain.

User

  1. We don't need this. If we only auto-prune safe ones and allow the user to force, it's on them that we cannot restore it later. Should just be clear that it is an unsafe action if they choose to delete it in a dirty state?

  2. If we do this it should be super minimal in v1


  1. Sure, lets start with worktrees v1 and then we can add more advanced options later

  2. never touch dirty worktrees. We can have some UI to emphasize that they have worktrees that "needs action" in the worktrees setting to show like "This worktree has not had activity for N days but is dirty so we wont delete it" kinda thing

  3. Global for now. Per-project settings override is a larger thing we need to support application-wide

  4. Should be configurable I think. User can choose (in settings) to delete "orphan" worktrees immediately or just keep it on the normal schedule

Assistant

Plan is written up at .plans/worktree-management-v1.md with all your decisions folded in. The v1 shape:

  • Inventory service (server, derived not persisted) — scans git worktree list --porcelain per project, joins thread rows, computes dirty/unpushed/merged/orphaned/lastActivity and a safeToPrune flag. Exposed as vcsListWorktrees + vcsPruneWorktrees next to the existing worktree RPCs, with the server re-validating safety before every removal rather than trusting the client's classification.
  • Worktrees settings page — appearance-pattern route/panel/nav. Needs-attention group first ("no activity for N days, but has uncommitted changes so we won't delete it"), badges, per-row delete, bulk "prune N safe worktrees". Dirty/unpushed deletes get a destructive-styled confirm that spells out the state is permanently lost — no snapshot machinery, per your call.
  • Auto-prune sweepProviderSessionReaper-pattern loop; only ever removes safe worktrees past the inactivity window (default 14d, options 7/14/30/off), never dirty ones. Uses non-forced git worktree remove so git itself is a last-line safety net, and finishes with git worktree prune. The "delete orphans immediately" toggle is handled here too: when on, thread.delete orphaning a worktree removes it server-side with no dialog — which also fixes the current gap where that cleanup is desktop-only.
  • Revival — threads keep branch/worktreePath after prune; bootstrap.prepareWorktree recreates the worktree from the still-existing branch on unsettle/turn-start when the path is missing. Branches and checkpoint refs both survive v1 pruning, so this is lossless.
  • CLIt3 worktrees list / prune --dry-run as a stretch, cut first if needed; the settings page's bulk prune already covers digging out of the 186.

One open implementation question I flagged in the doc: the sweep runs server-side, but our existing policy-ish settings (like sidebarAutoSettleAfterDays) are client-side only — so the prune policy needs a server-readable home. I'd resolve that at implementation time (reuse a server-persisted settings mechanism if one exists, otherwise a small settings store + RPC).

Build order: inventory → settings page → sweep/policy → revival → CLI. Steps 1–2 alone give you observability and manual cleanup. Say the word and I'll start on the inventory service.

User

the page probably also needs some filter functionality

Assistant

Added to the plan doc. The filter spec for the settings page is now:

  • Free-text search over branch name, directory name, and linked thread titles — thread title is the important one, since t3code-ab12cd34 means nothing to a human and "which worktree belongs to that auth-refactor thread" is the actual question.
  • Project filter, since the page is global and multiple projects' worktrees would interleave.
  • Status filter chips (multi-select): needs attention, safe to prune, dirty, unpushed, orphaned, settled-only.
  • All client-side over the already-loaded inventory — no new RPC surface needed.
  • One interaction rule worth calling out: when filters are active, the bulk prune operates on the filtered set and the button says so ("Prune 12 safe worktrees (filtered)"), so it never silently deletes rows the user isn't looking at.
  • Filter state is ephemeral in v1, no persistence.

Anything else you want to shape before I start on the inventory service?


Note

Medium Risk
Touches turn-start paths and git worktree removal with policy-driven auto-prune; mistakes could remove worktrees or fail sessions, but server-side safety checks and non-force prune limit blast radius.

Overview
Introduces managed worktree lifecycle so thread worktrees no longer accumulate without visibility or server-side cleanup.

Server: New WorktreeService builds inventory from git worktree list plus thread projections, classifies safe-to-prune (no active thread, clean tree, no unpushed commits), and implements pruneWorktrees (re-validates on the server, non-force remove, git worktree prune) and reviveWorktree (recreate directory from surviving branch). WorktreeReaper runs periodic sweeps from worktrees server settings (autoPruneAfterDays, deleteOrphanedImmediately). ProviderCommandReactor calls revival before turn start when the path is missing. ThreadDeletionReactor can auto-prune orphaned safe worktrees when the immediate-orphan setting is on. WS exposes vcsListWorktrees / vcsPruneWorktrees; contracts add worktree types and ServerSettings.worktrees.

Web: New Settings → Worktrees page lists worktrees with filters, needs-attention grouping, bulk safe prune, row delete (safe vs force), and policy controls. Thread delete skips the desktop orphan dialog when the server handles immediate orphan pruning on the primary environment.

Also adds a v1 design plan doc and Git-backed WorktreeService tests; integration tests mock WorktreeService for revival.

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

Note

Add worktree inventory, pruning, and revival with a settings UI and background reaper

  • Adds WorktreeService exposing listWorktrees, pruneWorktrees, and reviveWorktree with safety checks (dirty files, unpushed commits, orphaned detection) across all managed projects.
  • Adds WorktreeReaper as a background service that periodically prunes safe/orphaned worktrees according to configurable autoPruneAfterDays and deleteOrphanedImmediately server settings.
  • Exposes two new WebSocket RPC methods (vcs.listWorktrees, vcs.pruneWorktrees) with authorization scopes; pruning triggers git status refreshes for affected workspaces.
  • Adds a WorktreesSettingsPanel at /settings/worktrees with a filterable table, bulk safe-prune, per-row delete/force-delete, and an auto-prune configuration section.
  • On thread.turn.start, ProviderCommandReactor now attempts to revive a pruned worktree before starting the provider session; on thread deletion, ThreadDeletionReactor may immediately prune the orphaned worktree when settings permit.
📊 Macroscope summarized de7f960. 17 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

- Add worktree inventory, safety validation, pruning, and reaping
- Expose worktree management through server RPC and settings UI
- Restore missing worktrees when threads resume
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c02353a-5db8-4289-8a06-09d0b5c74a96

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/plan-worktree-management

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

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ changed lines (additions + deletions). labels Jul 28, 2026
});
// A pruned (or manually deleted) worktree is recreated from the thread's
// branch before the provider session starts in it.
if (thread.worktreePath !== null && thread.branch !== null && project) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Layers/ProviderCommandReactor.ts:502

When a thread's worktree directory is manually deleted while a provider session is still active, ensureSessionForThread revives the worktree but then reuses the existing provider session because the path string is unchanged. The provider process keeps its original cwd handle pointing at the deleted directory, so the turn runs against a stale/invalid cwd instead of the revived worktree. The cwdChanged check on line 568 only compares path strings (effectiveCwd === activeSession?.cwd), so it stays false even though the underlying directory was recreated. Track the revived result from worktreeService.reviveWorktree and force a session restart when it is true, so the provider process is respawned with the fresh directory handle.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration/Layers/ProviderCommandReactor.ts around line 502:

When a thread's worktree directory is manually deleted while a provider session is still active, `ensureSessionForThread` revives the worktree but then reuses the existing provider session because the path string is unchanged. The provider process keeps its original cwd handle pointing at the deleted directory, so the turn runs against a stale/invalid cwd instead of the revived worktree. The `cwdChanged` check on line 568 only compares path strings (`effectiveCwd === activeSession?.cwd`), so it stays false even though the underlying directory was recreated. Track the `revived` result from `worktreeService.reviveWorktree` and force a session restart when it is true, so the provider process is respawned with the fresh directory handle.

const reviveWorktree: WorktreeService["Service"]["reviveWorktree"] = Effect.fn(
"WorktreeService.reviveWorktree",
)(function* (input) {
const exists = yield* fs.exists(input.worktreePath).pipe(Effect.orElseSucceed(() => false));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium vcs/WorktreeService.ts:531

reviveWorktree returns { revived: false } whenever input.worktreePath exists on the filesystem, even when that path is an empty directory, a stale worktree registration, or an unrelated directory left behind by a pruned or manually deleted worktree. The caller treats { revived: false } as "the worktree is ready to use," so it continues with a path that has no valid Git worktree, causing the provider session to fail or run in the wrong directory instead of recreating the worktree.

The fs.exists check at line 531 only confirms the path exists on disk — it does not verify the path is a registered Git worktree pointing at the expected branch. Consider checking the Git worktree registration (e.g. via git worktree list) or verifying the .git worktree metadata before treating existence as "already revived."

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/vcs/WorktreeService.ts around line 531:

`reviveWorktree` returns `{ revived: false }` whenever `input.worktreePath` exists on the filesystem, even when that path is an empty directory, a stale worktree registration, or an unrelated directory left behind by a pruned or manually deleted worktree. The caller treats `{ revived: false }` as "the worktree is ready to use," so it continues with a path that has no valid Git worktree, causing the provider session to fail or run in the wrong directory instead of recreating the worktree.

The `fs.exists` check at line 531 only confirms the path exists on disk — it does not verify the path is a registered Git worktree pointing at the expected branch. Consider checking the Git worktree registration (e.g. via `git worktree list`) or verifying the `.git` worktree metadata before treating existence as "already revived."

} else if (aheadOfUpstreamCount > 0) {
blockers.push("unpushed");
}
} else if (sync !== undefined && sync.upstreamGone) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium vcs/WorktreeService.ts:386

When sync.upstreamGone is true, the prune-safety check treats the branch as fully pushed and skips all commit-counting, so any local commits made after the remote ref was deleted are never detected. A clean, inactive worktree with unpushed local-only commits is marked safeToPrune and gets auto-removed, bypassing the advertised no-unpushed-commits safeguard.

The else if (sync !== undefined && sync.upstreamGone) branch returns no blocker without comparing the branch against defaultRef. Consider falling through to the aheadOfDefaultCount check (or another rev-list comparison) so post-deletion local commits still trigger unpushed.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/vcs/WorktreeService.ts around line 386:

When `sync.upstreamGone` is `true`, the prune-safety check treats the branch as fully pushed and skips **all** commit-counting, so any local commits made *after* the remote ref was deleted are never detected. A clean, inactive worktree with unpushed local-only commits is marked `safeToPrune` and gets auto-removed, bypassing the advertised no-unpushed-commits safeguard.

The `else if (sync !== undefined && sync.upstreamGone)` branch returns no blocker without comparing the branch against `defaultRef`. Consider falling through to the `aheadOfDefaultCount` check (or another rev-list comparison) so post-deletion local commits still trigger `unpushed`.

Comment on lines +218 to +220
concurrency: {
mode: "serial",
key: ({ environmentId }) => JSON.stringify([environmentId, "worktrees.prune"]),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium state/vcs.ts:218

pruneWorktrees uses the concurrency key [environmentId, "worktrees.prune"], so the shared vcsCommandScheduler only serializes prune requests against other prune requests. Other VCS commands use [environmentId, input.cwd], so a prune can still run concurrently with createWorktree, removeWorktree, switchRef, etc. in the same environment. The comment says prune should serialize per environment, but the key namespace is disjoint from every other command's key, so there is no actual mutual exclusion. To serialize prune against all other VCS commands in the same environment, the key must overlap with theirs — for example [environmentId].

-        key: ({ environmentId }) => JSON.stringify([environmentId, "worktrees.prune"]),
+        key: ({ environmentId }) => JSON.stringify([environmentId]),
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/state/vcs.ts around lines 218-220:

`pruneWorktrees` uses the concurrency key `[environmentId, "worktrees.prune"]`, so the shared `vcsCommandScheduler` only serializes prune requests against other prune requests. Other VCS commands use `[environmentId, input.cwd]`, so a prune can still run concurrently with `createWorktree`, `removeWorktree`, `switchRef`, etc. in the same environment. The comment says prune should serialize per environment, but the key namespace is disjoint from every other command's key, so there is no actual mutual exclusion. To serialize prune against all other VCS commands in the same environment, the key must overlap with theirs — for example `[environmentId]`.

Comment on lines +449 to +455
const seen = new Set<string>();
const worktrees: WorktreeInfo[] = [];
for (const record of perProject.flat()) {
if (seen.has(record.path)) continue;
seen.add(record.path);
worktrees.push(record);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High vcs/WorktreeService.ts:449

The deduplication loop in listWorktrees keeps the first WorktreeInfo per path and silently discards all later records for the same path. When two projects share a repository and the second project has an active thread on that worktree, the retained first record can lack the active_thread blocker and be marked safeToPrune: true. pruneWorktrees then removes a worktree that an active thread is still using. The fix is to merge thread references and blockers from later records into the kept record instead of dropping them.

Suggested change
const seen = new Set<string>();
const worktrees: WorktreeInfo[] = [];
for (const record of perProject.flat()) {
if (seen.has(record.path)) continue;
seen.add(record.path);
worktrees.push(record);
}
const seen = new Map<string, WorktreeInfo>();
const worktrees: WorktreeInfo[] = [];
for (const record of perProject.flat()) {
const existing = seen.get(record.path);
if (existing === undefined) {
seen.set(record.path, record);
worktrees.push(record);
} else {
existing.threads = [...existing.threads, ...record.threads];
existing.orphaned = existing.orphaned && record.orphaned;
existing.pruneBlockers = [
...new Set([...existing.pruneBlockers, ...record.pruneBlockers]),
] as WorktreePruneBlocker[];
existing.safeToPrune = existing.pruneBlockers.length === 0;
}
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/vcs/WorktreeService.ts around lines 449-455:

The deduplication loop in `listWorktrees` keeps the first `WorktreeInfo` per path and silently discards all later records for the same path. When two projects share a repository and the second project has an active thread on that worktree, the retained first record can lack the `active_thread` blocker and be marked `safeToPrune: true`. `pruneWorktrees` then removes a worktree that an active thread is still using. The fix is to merge thread references and blockers from later records into the kept record instead of dropping them.

if (thread === null || thread.worktreePath === null) {
return;
}
const worktreePath = pathService.resolve(thread.worktreePath);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium Layers/ThreadDeletionReactor.ts:78

pruneOrphanedWorktree resolves the thread's stored path with pathService.resolve, but WorktreeService.listWorktrees returns paths canonicalized via fs.realPath. When the stored path uses a non-canonical spelling (e.g. /var/... on macOS where /private/var is the real path), worktrees.find((worktree) => worktree.path === worktreePath) never matches, so the immediate orphan cleanup is silently skipped and the worktree is left behind. Consider canonicalizing worktreePath with fs.realPath (or the equivalent the service uses) before the comparison.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration/Layers/ThreadDeletionReactor.ts around line 78:

`pruneOrphanedWorktree` resolves the thread's stored path with `pathService.resolve`, but `WorktreeService.listWorktrees` returns paths canonicalized via `fs.realPath`. When the stored path uses a non-canonical spelling (e.g. `/var/...` on macOS where `/private/var` is the real path), `worktrees.find((worktree) => worktree.path === worktreePath)` never matches, so the immediate orphan cleanup is silently skipped and the worktree is left behind. Consider canonicalizing `worktreePath` with `fs.realPath` (or the equivalent the service uses) before the comparison.

const { worktrees } = yield* worktreeService.listWorktrees({
projectId: thread.projectId,
});
const record = worktrees.find((worktree) => worktree.path === worktreePath);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orphan prune path mismatch

High Severity

Immediate orphan worktree cleanup in ThreadDeletionReactor fails to prune eligible worktrees. The path comparison between the stored worktreePath (resolved) and inventory paths (canonicalized with realPath) can mismatch, especially with symlinks or macOS /var paths, preventing removal.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit de7f960. Configure here.

blockers.push("status_unavailable");
} else if (aheadOfDefaultCount > 0) {
blockers.push("unpushed");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream gone skips unpushed check

High Severity

When a branch’s upstream is marked gone, prune safety treats it as fully synced and does not apply the already-computed aheadOfDefaultCount. Worktrees whose branches still have commits not on the default ref can be marked safeToPrune and removed by auto-prune, bulk prune, or orphan cleanup.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit de7f960. Configure here.

const localApi = readLocalApi();
let shouldDeleteWorktree = false;
if (canDeleteWorktree && localApi) {
if (canDeleteWorktree && localApi && !serverPrunesOrphan) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orphan setting skips force cleanup

Medium Severity

When deleteOrphanedImmediately is on for the primary environment, the desktop worktree confirmation and client removeWorktree path are skipped entirely. The server only removes orphans that are safeToPrune, so dirty or unpushed orphan worktrees are neither auto-pruned nor offered for force deletion after thread delete.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit de7f960. Configure here.

@macroscopeapp

macroscopeapp Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

6 blocking correctness issues found. This PR introduces a substantial new worktree management feature with new services, UI, contracts, and integration into core orchestration. Multiple unresolved high-severity review comments identify bugs that could lead to data loss (worktrees with unpushed commits incorrectly marked safe to prune) and require attention before merge.

You can customize Macroscope's approvability policy. Learn more.

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service conventions review. One clear error-handling violation (wrapper drops cause and rebuilds its message from cause.message), plus a curried error-constructor helper that the conventions call out. Details inline.

Posted via Macroscope — Effect Service Conventions

Comment on lines +512 to +516
new ProviderAdapterRequestError({
provider: providerErrorLabel(preferredProvider),
method: "thread.turn.start",
detail: `Could not restore the thread's worktree: ${error.message}`,
}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wrapper drops the underlying error and rebuilds detail from error.message. Per the error conventions, preserve the immediate failure as cause and derive the wrapper's message/detail from its own structural attributes rather than copying cause.message. ProviderAdapterRequestError already accepts an optional cause.

Suggested change
new ProviderAdapterRequestError({
provider: providerErrorLabel(preferredProvider),
method: "thread.turn.start",
detail: `Could not restore the thread's worktree: ${error.message}`,
}),
new ProviderAdapterRequestError({
provider: providerErrorLabel(preferredProvider),
method: "thread.turn.start",
detail: `Could not restore the thread's worktree at ${thread.worktreePath}.`,
cause: error,

Posted via Macroscope — Effect Service Conventions

Comment on lines +194 to +195
const toGitManagerError = (operation: string, cwd: string, detail: string) => (cause: unknown) =>
new GitManagerError({ operation, cwd, detail, cause });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toGitManagerError is a curried helper whose only behavior is (...args) => new GitManagerError({ ...args }), used with mapError. The conventions ask that the error be constructed at the failure boundary so its attributes and cause stay visible, rather than through a generic constructor alias. Consider inlining new GitManagerError({ operation, cwd, detail, cause }) at each mapError site.

Posted via Macroscope — Effect Service Conventions

@juliusmarminge

Copy link
Copy Markdown
Member Author

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 4 total unresolved issues (including 3 from previous reviews).

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit de7f960. Configure here.

const primaryEnvironmentId = usePrimaryEnvironmentId();
const deleteOrphanedWorktreesImmediately = usePrimarySettings(
(settings) => settings.worktrees.deleteOrphanedImmediately,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong environment orphan policy

Medium Severity

Thread delete skips the desktop “delete worktree too?” prompt only when deleteOrphanedImmediately comes from primary server settings and the thread’s environment is the primary one. Deletes on other connected servers ignore that server’s worktree policy and may show the dialog even when that server auto-prunes orphans.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit de7f960. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant