Repository navigation
fix(cli): orca worktree rm reports the delete's outcome, not its acceptance - #35459
Conversation
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
|
[Medium impact] The PR appears safe to merge; no new actionable issues were found. Summary
|
…ceptance The host answers a worktree delete as soon as it accepts it and lets Git delete the checkout in the background. Clients that show a "removing" marker ask it to wait for Git, but the CLI could not, so it printed `removed: true` before anything was deleted, and a delete Git then refused (for example a checkout with submodules) failed only in the host's log. Scripts saw success, and every retry repeated the same silent failure. `worktree.rm` takes an optional `waitForBackgroundRemoval` flag, and the CLI sets it, so the command exits with Git's real result or error. It waits as long as the desktop and web clients do and, if it stops waiting first, says the delete may still be running and how to check. A reply sent on acceptance now says `removed: false` alongside `removing: true`, so callers that still get the early answer are not told the checkout is gone. Older hosts ignore the flag, and the CLI then says the delete is still running. Fixes #35135
6a82f2d to
3538969
Compare
There was a problem hiding this comment.
Actionable comments posted: 1

Review info
-
Run configuration-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c0f29a83-35bb-4456-9528-2ba1fda03eec
Files selected for processing: 7-
src/cli/handlers/worktree-removal-request.tssrc/cli/handlers/worktree.tssrc/cli/index.test.tssrc/main/runtime/rpc/methods/worktree-runtime-routing.test.tssrc/main/runtime/rpc/methods/worktree.tssrc/main/runtime/rpc/worktree-removal-marker-projection.tssrc/shared/rpc-contract/worktree-params.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
| ) | ||
| return { | ||
| removed: true, | ||
| removed: result.removing !== true, |
There was a problem hiding this comment.
Do not infer successful deletion from an absent removing field.
If Git fails before finishAcceptedWorktreeRemoval looks up the pending removal, the settlement code has already deleted that entry. The helper then drops removing without awaiting the failed promise. This line returns removed: true, so orca worktree rm reports success for a failed deletion. Capture the settlement promise when removal is accepted, and await that promise before reporting completion. (raw.githubusercontent.com)
ELI5
orca worktree rmused to say "removed: true" as soon as Orca agreed to delete a workspace, before anything was actually deleted. If the delete then failed, only the host's log knew. The CLI now waits for the real result, so it succeeds when the workspace is gone and fails with Git's error when it isn't.What Changed
worktree.rmonce it accepts the delete, and Git deletes the checkout in the background. The desktop, paired desktop and web clients advertiseworktree.background-removal.v1, so the host waits for Git before answering them. The CLI advertises no capabilities, so it got{ removed: true, removing: true }right away. A background failure (for example a checkout with submodules) went only to the host's stdout, the CLI exited 0, and every retry silently repeated the failure.worktree.rmtakes an optionalwaitForBackgroundRemovalflag. The host waits for Git when the client reads removal markers or sets the flag. This uses the same wait and join paths the desktop already uses.worktreeRemovalReplyTimeoutMs, which also covers--run-hooks). It exits with Git's real result or error. If it stops waiting first, it says the delete may still be running and points atorca worktree show, which lists a failed delete'sremovalError.removed: falsealongsideremoving: true, so a caller that still gets the early answer (older CLIs, mobile) is never told the checkout is gone. No client readsremovedfrom this reply. I checked the renderer, web, mobile and the skill guides.removed: falseand that the delete is still running.Why
CLI and automation callers act on the exit status and the JSON reply, so they need the outcome, not acceptance. A "pending" status they would have to poll would push the waiting into every script. The host already keeps waiters for exactly this. The CLI just couldn't ask for it, because the wait was tied to a client capability that also changes how listings look. A per-request flag lets the CLI ask for the outcome without changing what
worktree listshows it. Mobile keeps reading acceptance, as it did before.Linked Issue
Fixes #35135
The submodule refusal itself now happens before teardown in the companion PR #35458.
Thanks to MrClean73 for the detailed report.
Visual Proof
N/A. CLI and RPC behaviour only, with no UI change.
Testing
src/main/runtime/rpc/methods/worktree-runtime-routing.test.ts: a capability-less caller that sets the flag gets the wait and receives Git's failure as an RPC error. An accepted, still-running delete repliesremoved: false. Both fail on origin/main.src/cli/index.test.ts:worktree rmsends the flag with the removal reply timeout and printsremoved: falsewhen an older host answers on acceptance. Both fail on origin/main.pnpm check:changed --maxWorkers=2: everything passes except tests unrelated to this change.electron-serve-browser-processfails the same way on origin/main (the real-home write guard). Three cross-version agent-session downgrade tests timed out under machine load and pass when run alone.Not run: a live
orca worktree rmagainst a running desktop ororca serve.I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
removed: falsewhile a delete is pending, which is accurate.--json, a new CLI talking to an older host prints that host's reply unchanged (removed: true, removing: true).removalError. That is a separate mobile feature.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)