Skip to content

fix(cli): wait out background worktree removal and report the real outcome - #35212

Closed
Pr1p wants to merge 1 commit into
stablyai:mainfrom
Pr1p:fix/cli-worktree-rm-honest-removal
Closed

Pr1p wants to merge 1 commit into
stablyai:mainfrom
Pr1p:fix/cli-worktree-rm-honest-removal

Conversation

@Pr1p

@Pr1p Pr1p commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

ELI5

orca worktree rm used to say removed: true right away, while the checkout was still being deleted in the background. If that background delete failed — for example on a checkout with an initialised submodule, which Git refuses to remove without --force — the failure only ever reached the runtime's own log. The CLI now declares it can wait, so worktree rm waits out the deletion and reports the real outcome: success, or the actual Git failure.

What Changed

Before: the CLI never advertised worktree.background-removal.v1, so hosts treated it as a legacy client: worktree.rm returned an acceptance reply (removed: true, removing: true) before any deletion happened, and pending-removal rows were omitted from worktree list/ps so a stuck checkout was invisible. The hardcoded acceptance removed: true in the RPC method is kept — genuinely old clients depend on it to drop the row.

After:

  • The CLI capability list (new shared client-capabilities.ts) advertises worktree.background-removal.v1 on both the local socket frame and the remote WebSocket handshake.
  • Local socket frames now carry clientCapabilities: the frame is owner-token authenticated and parsed directly as the RpcRequest, so the field rides it like a per-connection negotiation. The capability only changes what the host reports back to this client (waited results, visible removing rows); it grants no authority and unlocks no gate.
  • With the capability, worktree.rm waits for the background settlement: success resolves truthfully, and a failed deletion rejects so the Git error reaches the CLI instead of only a console.warn in the runtime log.
  • Pending-removal rows stop being omitted from CLI worktree list/ps, so a checkout stuck mid-deletion stays visible.

Requests keep the default 60s timeout: a deletion slower than that reports runtime_timeout while the background job keeps running, and a subsequent worktree rm joins the same pending settlement to report its outcome.

Why

#35135: scripts calling orca worktree rm in a loop received removed: true per call while nothing was being removed, and every failure was invisible.

Linked Issue

Addresses the acceptance-reply half of #35135. The reporter's retry-loop/backoff observations are deliberately not in scope here.

Visual Proof

N/A — no rendered UI change.

Testing

  • transport.test.ts: the local frame now asserts clientCapabilities contains worktree.background-removal.v1 alongside the owner auth token.
  • websocket-transport.test.ts: the handshake capability array gains the same entry.
  • CI: typecheck, static analysis, targeted tests.

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

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: cd616252-6b10-4e17-98df-e8718ad208a6
Commits

Reviewing files that changed from the base of the PR and between 3e94a6c and cb1ed80.

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

No actionable comments were generated in the recent review.

Walkthrough

The CLI defines a shared tuple of agent-launch and worktree-background-removal capabilities. Local RPC frames now include that tuple, and WebSocket transports use it instead of a locally defined capability array. A local-socket test checks the capability field, authentication token, and successful response handling. Worktree formatters display removal status when removing is exactly true.

Merge Risk: Moderate · previous review 3e94a

Local CLI listings can still hide worktrees being deleted, so the capability-forwarding fix should land before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the fix (waiting out background worktree removal and reporting the real outcome) in the CLI, which matches the changeset: CLI_CLIENT_CAPABILITIES advertises worktree.background-removal…
Description check Passed The description follows the repository template closely. It has ELI5, What Changed (before/after and mechanism), Why, Linked Issue, Visual Proof marked N/A with reason, Testing, and Notes. It explains…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the fix (waiting out background worktree removal and reporting the real outcome) in the CLI, which matches the changeset: CLI_CLIENT_CAPABILITIES advertises worktree.background-removal.v1, worktree rm waits for background settlement, and removing rows are shown in worktree list/ps. The title is specific, one sentence, and follows the conventional-commit style. It does not cover the capability plumbing or the list/ps visibility change, which is acceptable.

Full details: Description check

Explanation

The description follows the repository template closely. It has ELI5, What Changed (before/after and mechanism), Why, Linked Issue, Visual Proof marked N/A with reason, Testing, and Notes. It explains the mechanism and the timeout behaviour, and it scopes out the retry-loop part of #35135. Gaps: the Risk section is missing entirely, although the template asks for a risk level, what could break, and how to back it out. The Review, Agent skill upstream boundary, and AI Disclosure sections are also absent. The Linked Issue section says it addresses only part of #35135 and uses 'Fixes #' with no number, so it does not link a closing issue. The Testing checklist does not mark the Every added test fails without this change item or the manual test item. These are important omissions but the core content is complete, so the check passes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 5 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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
Recent 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: fcfb8ae3-b265-4976-895a-70c6d60ea396
Commits

Reviewing files that changed from the base of the PR and between f103d38 and 3e94a6c.

Files selected for processing: 3
  • config/tsconfig.node.json
  • src/cli/runtime/client-capabilities.ts
  • src/cli/runtime/websocket-transport.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@coderabbitai coderabbitai 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.

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: fbeb584d-fe8c-4fed-8095-60212d5f7fd2
Commits

Reviewing files that changed from the base of the PR and between 66b2478 and 5855c4d.

Files selected for processing: 5
  • src/cli/runtime/client-capabilities.ts
  • src/cli/runtime/transport.test.ts
  • src/cli/runtime/transport.ts
  • src/cli/runtime/websocket-transport.test.ts
  • src/cli/runtime/websocket-transport.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment thread src/cli/runtime/client-capabilities.ts Resolved

@coderabbitai coderabbitai 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.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

Minor · Forward local client capabilities into dispatch. · transport.ts:184-199

src/cli/runtime/transport.ts:184-199
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Forward local client capabilities into dispatch.

The local frame includes clientCapabilities, but RuntimeRpcRequestAdmission passes only signal and callerScope to dispatcher.dispatch. Local CLI worktree.rm, worktree list, and worktree ps therefore receive no RpcContext.clientCapabilities and use legacy behavior.

Keep parseAndAuth and the existing response flow unchanged:

Suggested fix
         signal:
           longPoll ||
           request.method === 'linear.commentEdit' ||
           request.method === 'linear.commentDelete'
             ? context?.signal
             : undefined,
-        callerScope
+        callerScope,
+        clientCapabilities: request.clientCapabilities

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: f1fec242-6935-4b6a-97e5-5834c074ce83
Commits

Reviewing files that changed from the base of the PR and between 5855c4d and a83bc0c.

Files selected for processing: 2
  • src/cli/format.test.ts
  • src/cli/workspace-format.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

@Pr1p
Pr1p force-pushed the fix/cli-worktree-rm-honest-removal branch from f103d38 to 3e94a6c Compare November 4, 2026 20:16
…tcome

Advertise worktree.background-removal.v1 on the CLI client frames so a
capable host waits out the delete instead of answering early, and share
the CLI capability list between the local socket and paired transports.
@Pr1p
Pr1p force-pushed the fix/cli-worktree-rm-honest-removal branch from 3e94a6c to cb1ed80 Compare November 4, 2026 20:21
@Pr1p

Pr1p commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #35459 (with #35458), merged on 2026-10-29, which closed #35135. orca worktree rm now waits out the background removal and reports the real outcome, so the core change here is already on main. The only unique bits left are the removing: display in worktree list/worktree ps and the local-transport capability advertisement; neither is needed for the fix. Closing as superseded — if the display is still wanted, I can refile it as a small focused PR.

@Pr1p Pr1p closed this Nov 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant