[codex] Fix worktree delete cleanup ordering#4801
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughWorkspace deletion is refactored to extract a new ChangesWorkspace deletion refactor: phase reordering and result enrichment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 5be1caf on May 21, 2026 11:23pm UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts (1)
224-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck cloud API precondition before running teardown.
Line 228 can execute teardown side effects before Line 252 rejects on missing
ctx.api. That means deletion can fail after local teardown has already run. Move the precondition check ahead of teardown to keep failure atomic.Suggested fix
// ─── Step 0: Preflight ───────────────────────────────────────── // Block only on dirty worktree (the common "I forgot to commit" // case). Missing/broken local state is handled by the cleanup phase. if (!input.force && local && project) { try { const git = await ctx.git(local.worktreePath); const status = await git.status(); if (!status.isClean()) { throw new TRPCError({ code: "CONFLICT", message: "Worktree has uncommitted changes", }); } } catch (err) { if (err instanceof TRPCError) throw err; // Can't read status (missing worktree dir, etc.) — not a // conflict. Continue; step 3b will skip idempotently. } } + // Ensure we can commit to cloud before any local teardown/cleanup side effects. + if (!ctx.api) { + throw new TRPCError({ + code: "PRECONDITION_FAILED", + message: "Cloud API not configured", + }); + } + // ─── Step 1: Teardown ────────────────────────────────────────── // Script is the user's last chance to stop services / flush state // before the workspace goes away. Failure here is recoverable // via force-retry, which skips this step. if (!input.force && local && project) { const teardown: TeardownResult = await runTeardown({ db: ctx.db, workspaceId: input.workspaceId, worktreePath: local.worktreePath, }); if (teardown.status === "failed") { const cause: TeardownFailureCause = { kind: "TEARDOWN_FAILED", exitCode: teardown.exitCode, signal: teardown.signal, timedOut: teardown.timedOut, outputTail: teardown.outputTail, }; throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Teardown script failed", cause, }); } } - - // Make sure we can commit to cloud before touching local disk. The actual - // cloud mutation happens after the worktree is removed. - if (!ctx.api) { - throw new TRPCError({ - code: "PRECONDITION_FAILED", - message: "Cloud API not configured", - }); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts` around lines 224 - 257, The cloud API precondition (ctx.api) must be validated before running teardown to avoid performing local side effects when cloud commits will later fail; move the existing check that throws the TRPCError with code "PRECONDITION_FAILED" so it executes before the teardown block (i.e., before calling runTeardown), keeping the same error shape and message, and preserve existing conditions (input.force, local, project) so you still skip teardown when forced or non-local/no-project; ensure references to runTeardown, ctx.api, TRPCError, input.force, local, and project are updated accordingly.
🧹 Nitpick comments (1)
packages/mcp-v2/src/tools/workspaces/delete.ts (1)
21-38: ⚡ Quick winInconsistent return type shapes.
The handler returns two different shapes:
- Early return (line 22):
{ success: true, alreadyGone: true }- Normal return (lines 24-28):
{ success: boolean; worktreeRemoved: boolean; warnings: string[] }This creates a union type that forces callers to distinguish between the two cases. For API consistency, consider adding the enriched fields to the early return path with default values:
♻️ Proposed fix for consistent return shape
if (!workspace) { - return { success: true, alreadyGone: true }; + return { + success: true, + alreadyGone: true, + worktreeRemoved: false, + warnings: [], + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp-v2/src/tools/workspaces/delete.ts` around lines 21 - 38, The early-return when workspace is missing returns { success: true, alreadyGone: true } which has a different shape than the hostServiceCall response; update the early return in the delete handler to include the same fields returned by hostServiceCall (success, worktreeRemoved, warnings) plus alreadyGone flag (e.g. success: true, worktreeRemoved: false, warnings: [], alreadyGone: true) so callers always receive a consistent object shape; locate the check using workspace and input.id in this file and adjust that return accordingly to match the hostServiceCall shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts`:
- Around line 224-257: The cloud API precondition (ctx.api) must be validated
before running teardown to avoid performing local side effects when cloud
commits will later fail; move the existing check that throws the TRPCError with
code "PRECONDITION_FAILED" so it executes before the teardown block (i.e.,
before calling runTeardown), keeping the same error shape and message, and
preserve existing conditions (input.force, local, project) so you still skip
teardown when forced or non-local/no-project; ensure references to runTeardown,
ctx.api, TRPCError, input.force, local, and project are updated accordingly.
---
Nitpick comments:
In `@packages/mcp-v2/src/tools/workspaces/delete.ts`:
- Around line 21-38: The early-return when workspace is missing returns {
success: true, alreadyGone: true } which has a different shape than the
hostServiceCall response; update the early return in the delete handler to
include the same fields returned by hostServiceCall (success, worktreeRemoved,
warnings) plus alreadyGone flag (e.g. success: true, worktreeRemoved: false,
warnings: [], alreadyGone: true) so callers always receive a consistent object
shape; locate the check using workspace and input.id in this file and adjust
that return accordingly to match the hostServiceCall shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc878255-3f48-494b-a1e1-d9a34f6f0afe
📒 Files selected for processing (10)
packages/cli/src/commands/workspaces/delete/command.tspackages/host-service/src/trpc/router/workspace-cleanup/index.tspackages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/host-service/test/integration/bug-hunt-2.integration.test.tspackages/host-service/test/integration/bug-hunt-3.integration.test.tspackages/host-service/test/integration/workspace-create-delete.integration.test.tspackages/host-service/test/workspace-cleanup.test.tspackages/mcp-v2/src/tools/workspaces/delete.tspackages/sdk/src/resources/workspaces.ts
Greptile SummaryThis PR fixes a class of workspace delete bugs where cloud rows were removed before local worktree cleanup, leaving disk state orphaned when cleanup failed. It consolidates all delete callers (legacy
Confidence Score: 3/5The worktree-before-cloud reordering is sound for the main case, but branch deletion was also moved before cloud delete, which can leave a workspace visible in the app without its branch if the cloud call fails transiently. The core ordering fix is correct and well-tested. The concern is that branch deletion (step 2c) now runs before cloud delete (step 3). When deleteBranch is true and cloud delete fails transiently, the workspace reappears in the sidebar but the branch pointer is already gone, making the workspace inaccessible until a force-retry removes it. The old design had branch deletion as a best-effort step after the cloud commit point, so this is a step backward for that specific failure scenario. The coverage gap for git-factory failure with an existing worktree path is also worth addressing before merge. packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts — branch deletion ordering around step 2c/3; packages/host-service/test/workspace-cleanup.test.ts — missing coverage for git-factory failure with existing path.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts | Core cleanup saga reordered: local teardown now precedes cloud delete, worktree failure now throws instead of warning, but branch deletion was moved before the cloud commit point which can leave a workspace visible in the app with its branch already deleted on cloud API failure. |
| packages/host-service/src/trpc/router/workspace/workspace.ts | Legacy workspace.delete now delegates to destroyWorkspace with force:true and deleteBranch:false, sharing one cleanup path with v2; main-workspace protection preserved via isMainWorkspace. |
| packages/host-service/test/workspace-cleanup.test.ts | Replaced best-effort warning test with new ordering test; stale "phase 2" label remains in in-flight guard test; git-factory-throw-with-path-present scenario is not covered. |
| packages/cli/src/commands/workspaces/delete/command.ts | CLI now surfaces per-workspace warnings from the structured delete result; clean and correct. |
| packages/mcp-v2/src/tools/workspaces/delete.ts | Type annotation updated to include worktreeRemoved and warnings fields; matches the new return shape. |
| packages/sdk/src/resources/workspaces.ts | WorkspaceDeleteResult interface replaced catch-all with named optional fields; all fields optional so existing callers won't break. |
| packages/host-service/test/integration/bug-hunt-3.integration.test.ts | Test updated to verify legacy workspace.delete routes through v2 saga and force-removes dirty worktrees; assertions updated to new return shape. |
Comments Outside Diff (1)
-
packages/host-service/test/workspace-cleanup.test.ts, line 293 (link)Stale phase reference in test label — after the reorder, cloud delete is now step 3, not step 2. Keeping the old label makes the test suite misleading for anyone debugging a future failure here.
Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/test/workspace-cleanup.test.ts Line: 293 Comment: Stale phase reference in test label — after the reorder, cloud delete is now step 3, not step 2. Keeping the old label makes the test suite misleading for anyone debugging a future failure here. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts:322-336
**Branch deletion before the cloud commit point**
Step 2c (branch deletion) now runs before step 3 (cloud delete). If `deleteBranch: true` and cloud delete fails after a successful branch deletion, the workspace remains visible in the app but the branch is permanently gone. The user sees the workspace in the sidebar, tries to check out or interact with it, and finds the branch missing — with no indication it was already deleted. On retry, the branch-not-found failure is demoted to a warning so cloud delete eventually succeeds, but by then the branch pointer is irretrievably lost (without manual `git reflog` surgery).
The old design had branch deletion in step 3c (after the cloud commit point), where it was safely best-effort. Moving it before cloud delete means a cloud API transient failure converts an optional cleanup into an unrecoverable loss of the branch reference.
### Issue 2 of 3
packages/host-service/test/workspace-cleanup.test.ts:293
Stale phase reference in test label — after the reorder, cloud delete is now step 3, not step 2. Keeping the old label makes the test suite misleading for anyone debugging a future failure here.
```suggestion
test("clears the Set when phase 3 (cloud delete) throws", async () => {
```
### Issue 3 of 3
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts:282-297
**Missing test for git-factory failure when the worktree path still exists**
The removed test ("git-factory failure in phase 3 becomes a warning") used a non-existent `worktreePath`, so `worktreeRemoved` was `true` at the start and the git factory was never invoked. That scenario is now unreachable for `deleteBranch: false`.
The new hard-error path — git factory throws and `existsSync(local.worktreePath)` is `true` — has no explicit unit test. The added "worktree removal failure" test covers the mock `worktreeRemove` throw but leaves git-factory-throw-with-path-present uncovered. A test that sets `gitFactoryThrows: true` on a real temp directory (similar to the new ordering test) would pin this behavior.
Reviews (1): Last reviewed commit: "Fix worktree delete cleanup ordering" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -335,7 +332,10 @@ async function runDestroy(ctx: HostServiceContext, input: DestroyInput) { | |||
| } | |||
| } | |||
|
|
|||
| // 3d. Host sqlite row | |||
| // ─── Step 3: Cloud delete ────────────────────────────────────── | |||
| await ctx.api.v2Workspace.delete.mutate({ id: input.workspaceId }); | |||
There was a problem hiding this comment.
Step 2c (branch deletion) now runs before step 3 (cloud delete). If deleteBranch: true and cloud delete fails after a successful branch deletion, the workspace remains visible in the app but the branch is permanently gone. The user sees the workspace in the sidebar, tries to check out or interact with it, and finds the branch missing — with no indication it was already deleted. On retry, the branch-not-found failure is demoted to a warning so cloud delete eventually succeeds, but by then the branch pointer is irretrievably lost (without manual git reflog surgery).
The old design had branch deletion in step 3c (after the cloud commit point), where it was safely best-effort. Moving it before cloud delete means a cloud API transient failure converts an optional cleanup into an unrecoverable loss of the branch reference.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts
Line: 322-336
Comment:
**Branch deletion before the cloud commit point**
Step 2c (branch deletion) now runs before step 3 (cloud delete). If `deleteBranch: true` and cloud delete fails after a successful branch deletion, the workspace remains visible in the app but the branch is permanently gone. The user sees the workspace in the sidebar, tries to check out or interact with it, and finds the branch missing — with no indication it was already deleted. On retry, the branch-not-found failure is demoted to a warning so cloud delete eventually succeeds, but by then the branch pointer is irretrievably lost (without manual `git reflog` surgery).
The old design had branch deletion in step 3c (after the cloud commit point), where it was safely best-effort. Moving it before cloud delete means a cloud API transient failure converts an optional cleanup into an unrecoverable loss of the branch reference.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts (1)
277-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when the local row exists but the project row is missing.
This block only protects the cloud delete when
local && projectis present. Iflocalexists withoutproject, Step 2 skips theexistsSync(local.worktreePath)check entirely, but Step 3 still deletes the cloud row and Step 5 removes the sqlite row. That reintroduces the orphaned-worktree case this PR is fixing whenever local metadata is partially missing. Treatlocal && !project && worktreePath still existsas a hard stop; only continue if the worktree is already gone.Suggested guard
let worktreeRemoved = false; let branchDeleted = false; let git: Awaited<ReturnType<typeof ctx.git>> | null = null; +if (local && !project) { + worktreeRemoved = !existsSync(local.worktreePath); + if (!worktreeRemoved) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `Missing project metadata for workspace ${input.workspaceId}; refusing cloud delete while worktree still exists at ${local.worktreePath}`, + }); + } +} if (local && project) { worktreeRemoved = !existsSync(local.worktreePath); try { git = await ctx.git(project.repoPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts` around lines 277 - 319, The current logic only runs the existsSync(local.worktreePath) guard when both local and project are present, which lets the cleanup proceed (and delete cloud/sql rows) when local exists but project is missing; change the flow so that if local exists and project is null/undefined you still check the filesystem and abort unless the worktree is already gone: when local && !project, compute worktreeRemoved = !existsSync(local.worktreePath) and if worktreeRemoved is false throw a TRPCError (same shape as other errors) instead of continuing; keep the existing warnings/ctx.git flow for the local && project case (references: local, project, worktreeRemoved, existsSync, ctx.git, local.worktreePath, TRPCError, warnings).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mcp-v2/src/tools/workspaces/delete.ts`:
- Around line 22-27: The early-return object in deleteWorkspace (or the delete
flow in packages/mcp-v2/src/tools/workspaces/delete.ts) returns success,
alreadyGone, worktreeRemoved, and warnings but omits cloudDeleted and
branchDeleted; update that early-return to include cloudDeleted: false (or
appropriate boolean) and branchDeleted: false so the returned shape matches the
normal deletion path and the PR summary’s five-field contract.
---
Outside diff comments:
In
`@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts`:
- Around line 277-319: The current logic only runs the
existsSync(local.worktreePath) guard when both local and project are present,
which lets the cleanup proceed (and delete cloud/sql rows) when local exists but
project is missing; change the flow so that if local exists and project is
null/undefined you still check the filesystem and abort unless the worktree is
already gone: when local && !project, compute worktreeRemoved =
!existsSync(local.worktreePath) and if worktreeRemoved is false throw a
TRPCError (same shape as other errors) instead of continuing; keep the existing
warnings/ctx.git flow for the local && project case (references: local, project,
worktreeRemoved, existsSync, ctx.git, local.worktreePath, TRPCError, warnings).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db17cab0-ac7e-4ae2-9ce2-6434b2ae375a
📒 Files selected for processing (4)
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.tspackages/host-service/test/integration/workspace-cleanup.integration.test.tspackages/host-service/test/workspace-cleanup.test.tspackages/mcp-v2/src/tools/workspaces/delete.ts
1 participant
What changed
workspace.deleteendpoint through the v2workspaceCleanup.destroysaga so desktop, CLI, SDK, and MCP deletes share one cleanup path.Why
Some delete callers were using the legacy
workspace.deleteendpoint, which deleted the cloud row first, attempted plain worktree removal, swallowed removal errors, and then deleted the local row. That could make a workspace disappear from the app while leaving the worktree on disk.The desktop app should not remove the authoritative cloud row until local worktree removal succeeds. If local removal fails while the path still exists, the delete now fails before cloud delete so the workspace can reappear and be retried.
Validation
bun test packages/host-service/test/workspace-cleanup.test.ts packages/host-service/test/integration/workspace-create-delete.integration.test.ts packages/host-service/test/integration/bug-hunt-2.integration.test.ts packages/host-service/test/integration/bug-hunt-3.integration.test.tsbun run lint:fixbun run lintbun run --cwd packages/host-service typecheckbun run --cwd packages/cli typecheckbun run --cwd packages/mcp-v2 typecheckbun run --cwd packages/sdk typecheckgit diff --checkSummary by cubic
Fixes workspace deletion by removing local worktrees before cloud delete and routing all delete entry points through the v2 cleanup path. Calls now return structured results and surface warnings to prevent “vanishing” workspaces and orphaned disk state.
workspace.deletethrough exporteddestroyWorkspacewith an in‑flight guard; v1 callers useforceand returnsuccess,cloudDeleted,worktreeRemoved,branchDeleted, andwarnings.--force) → cloud delete → branch delete → host sqlite; pre‑validates cloud API before touching disk.force, cloud‑delete failure retries, branch delete after cloud, and missing project metadata.Written for commit 5be1caf. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes