Skip to content

docs(orchestration): clean up created_child worktrees after worker-release - #19404

Open
LesleyMurfin wants to merge 1 commit into
stablyai:mainfrom
LesleyMurfin:fix/worktree-cleanup-after-worker-release
Open

LesleyMurfin wants to merge 1 commit into
stablyai:mainfrom
LesleyMurfin:fix/worktree-cleanup-after-worker-release

Conversation

@LesleyMurfin

@LesleyMurfin LesleyMurfin commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

ELI5

When a coordinator fans work out to several helper agents, each in its own new Git checkout ("child worktree"), finishing that work only closes the helper's terminal — the checkout itself is left on disk forever. This teaches the orchestration guide the coordinator reads to also delete that checkout once the helper's work is safely wrapped up, unless there's a good reason not to (like the user wanting to keep it).

What Changed

Documentation-only change to the orchestration skill guide (no product code):

  • skill-guides/orchestration/references/recovery-and-cleanup.md: new "Worktree cleanup after release" section. After an accepted worker-release, if the Dispatch's own effects recorded created_child, the coordinator runs orca worktree rm --force --worktree path:<worktreePath> --json, using terminal.worktreePath from worker-show --dispatch <id> --json. Documents five fail-closed conditions where the worktree must be left in place and reported instead: user asked to keep the workspace or worker-retain was recorded, the terminal was user-taken-over, the start effect was reused/--worktree current/an exact pre-existing workspace, the worker stopped or failed with unpushed local-only commits, or worktree rm itself errors.
  • skill-guides/orchestration.md: "Completion accounting" now flags this follow-on check after release, and the "Conditional references" table gains a row routing to the same reference.
  • src/cli/bundled-skill-guides.ts: regenerated via node config/scripts/generate-bundled-skill-guides.mjs --write so orca skills get orchestration serves the updated guide immediately.

Why

worker-release archives output and closes the agent terminal only; a worker-start --worktree new-child checkout stays on disk indefinitely. A coordinator that fans out many workers can leave dozens of stale checkouts behind after every worker settles cleanly, and Workspace Cleanup is suggestion-only and not orchestration-aware. This is a coordinator-contract fix (a MUST-follow step added to the guide the coordinator already reads) rather than a new product flag, so it lands immediately without a schema or RPC change.

Linked Issue

Architectural Context (RFC #21556)

  • Roadmap Phase: Step 6: Reap & Clean
  • Severity / Priority: P2 (Medium)
  • Why It Ties into the Lifecycle: Ephemeral child worktree directories created for orchestration workers were left on disk after worker release.
  • System Impact: Guarantees clean filesystem removal of child worktrees upon worker release, stopping disk accumulation and inode exhaustion.
  • Master Tracking RFC: Part of stablyai/orca#21556

Fixes #18411

Visual Proof

N/A — this is a Markdown documentation change to a skill guide with no rendered UI or runtime behavior change.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Manual verification: ran node config/scripts/generate-bundled-skill-guides.mjs --check before and after regenerating the bundled artifact — clean both times, confirming src/cli/bundled-skill-guides.ts stays byte-consistent with the edited source Markdown. Verified balanced code fences and consistent table-column padding in both edited files by direct inspection. No automated test added: this changes only prose guidance consumed by an LLM coordinator, not a parsed data structure or code path that a unit test could exercise; the existing generate-bundled-skill-guides.test.mjs / --check step already guards the one mechanical invariant (guide ↔ bundled-constant consistency) and passes.

AI Disclosure

This change was authored with AI assistance (Anthropic Claude) and reviewed by the human maintainer before submission.

Review

Self-reviewed: the added guidance matches the exact fail-closed conditions and command proposed by the issue author, reuses existing field names (effects[].action, terminal.worktreePath) verified against the current src/main/runtime/rpc/methods/orchestration/worker implementation rather than guessed, and follows the surrounding guide's section structure, table formatting, and voice.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Residual risk: this is a MUST-follow addition to the coordinator's documented contract, not product-code enforcement. A coordinator can still skip the step or misjudge a fail-closed condition, and nothing here makes it impossible to leave a worktree behind — it makes the expected, documented behavior clean one up instead of silently leaving it. A structural fix (e.g. a worker-release --remove-worktree flag, or auto-cleanup wired into release itself) remains out of scope, as the issue itself notes.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A) — N/A, prose guidance only, no platform-specific behavior
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred) — not run locally (fresh clone without full dependency install); generate-bundled-skill-guides.mjs --check was run directly and passes, and CI will cover the rest

…lease

Adds a coordinator-contract step to the orchestration skill guide: after an
accepted worker-release, if the Dispatch's start effect was created_child,
run 'orca worktree rm --force --worktree path:<worktreePath> --json' using
terminal.worktreePath from worker-show. Documents the fail-closed conditions
(user retention, user-taken-over terminal, reused/current placement, unpushed
local commits, worktree rm errors). Regenerates the bundled skill-guide
artifact so 'orca skills get orchestration' serves it.

Fixes stablyai#18411
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The orchestration guide adds a release-time obligation for Dispatches that recorded a created_child worktree. The recovery reference documents checking the Dispatch effects, removing the exact worktree after worker-release closes the terminal, and reporting conditions that require the worktree to remain.

Merge Risk: 🟠 High · up to 0b1fa

The new cleanup guidance can remove a child worktree while its terminal remains retained and can discard uncommitted or otherwise unverified local work. The release-state and work-preservation guards should be strengthened before merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR addresses issue #18411 by documenting cleanup after accepted worker release for created-child worktrees, using the required command and terminal.worktreePath. It also documents all specified fa…
Out of Scope Changes check ✅ Passed The changes are limited to the orchestration guide, its recovery-and-cleanup reference, and the generated bundled guide artifact. These changes directly support issue #18411 and do not introduce unrel…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Title check ✅ Passed The title clearly and concisely identifies the main change: cleanup of created_child worktrees after worker-release.
Description check ✅ Passed The description is complete and follows the template. It explains the change, rationale, linked issue, testing, AI disclosure, review notes, scope, and checklist status. It also documents that the cha…

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

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

@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: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 0218f15d-d523-43db-b880-63eb7c42820e

📥 Commits

Reviewing files that changed from the base of the PR and between 5882400 and 0b1fa79.

📒 Files selected for processing (3)
  • skill-guides/orchestration.md
  • skill-guides/orchestration/references/recovery-and-cleanup.md
  • src/cli/bundled-skill-guides.ts

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

Comment on lines +163 to +164
After `worker-release` accepts, check whether that Dispatch's own `effects`
recorded `{ "kind": "worktree", "action": "created_child" }`:

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Require a terminal-closed release state before cleanup.

orchestration.workerRelease can return state: "retained" while the terminal remains retained. Command acceptance is not proof that the terminal closed. Require state: "released" or "already_released" before worker-show and worktree rm; route retained, release_pending, and release_unknown through recovery. This guard is necessary because worktree rm --force also sets allowUnverifiedPtyStop.

Also applies to: 177-177

that same response's `terminal.worktreePath`:

```text
ORCA worktree rm --force --worktree path:<worktreePath> --json

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve local work before force removal.

--force removes a dirty checkout without confirmation. The current rule protects only unpushed commits after STOP or failure and only says to run git status; it does not require checking the exact terminal.worktreePath, and it does not cover uncommitted or untracked changes or a successful report. The worker contract does not require a commit when --files-modified is applicable. Inspect the exact child worktree before every removal and leave/report whenever it is dirty or commit visibility is unproven.

Also applies to: 187-188

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

Important

The cleanup recipe as written won't work for the very case it targets: worker-show's terminal field is null after release, so terminal.worktreePath is unavailable from the response the coordinator is told to read. Details inline.

Reviewed changes

  • recovery-and-cleanup.md — new "Worktree cleanup after release" section: after an accepted worker-release, check the Dispatch's effects for a created_child worktree and worktree rm --force it via terminal.worktreePath, with five fail-closed leave-in-place conditions.
  • orchestration.md: "Completion accounting" flags the follow-on cleanup check, and the "Conditional references" table routes to the same reference.
  • src/cli/bundled-skill-guides.ts: regenerated so the bundled guide matches the edited Markdown.

ℹ️ Nitpicks

  • "--force skips the dirty-tree confirmation prompt" — there is no prompt to skip. Non-force worktree rm refuses (errors) a dirty tree via git worktree remove; --force maps to git worktree remove --force and also waives PTY-stop proof (src/cli/handlers/worktree.ts:311). Worth rewording. Relatedly, worktree rm does delete the branch by default (safely, -d, not -D), so the "never force-deletes the underlying Git branch" reassurance is correct but easy to misread as "the branch is never touched".

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

```

If it did, remove the checkout Orca created for it, using the exact path from
that same response's `terminal.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.

terminal.worktreePath won't be present in the response this step is told to read. worker-show returns terminal: observation.exact ? observation.terminal : null (src/main/runtime/rpc/methods/orchestration/worker/worker-control.ts:78), and exact requires the Dispatch's recorded process incarnation to still match (isDispatchProcessCurrent). Release's job is to close/kill that process, so terminal is null afterwards — and for a structured worker it's null unconditionally (inspectWorkerTerminal returns terminal: null on the structured branch). Use the always-exposed worker.worktreeId from that same response instead and drive removal with id:<worktreeId>; worktree rm accepts id: as a first-class selector.

Technical details
# Worktree cleanup path source

## Affected sites
- skill-guides/orchestration/references/recovery-and-cleanup.md:171 — reads terminal.worktreePath from a post-release worker-show response

## Required outcome
- The recipe must obtain the created_child worktree identity from a field that survives release. Prefer `worker.worktreeId` (and/or the `id` on the `created_child` effect) from the `worker-show --json` response, then `ORCA worktree rm --force --worktree id:<worktreeId> --json`.
- If keeping `terminal.worktreePath`, note it must be read from an inspection run before release (when the terminal is still exact), and is never available for structured workers.

## Suggested approach (optional)
- `exposeWorker` already publishes `worktreeId` (parsed from `worker.worktree_id`) and `effects`; both are durable DB values, independent of process liveness. The `id:` selector resolves via `orca-runtime-resolve-worktree-selector.ts`.

@fettpl

fettpl commented Sep 17, 2026

Copy link
Copy Markdown

Heads-up on the path source: after worker-release, worker-show sets terminal to null because observation.exact fails once the process incarnation no longer matches (orchestration.workerShow). terminal.worktreePath is gone at the point this recipe reads it.

Capture <worktreePath> from the worker-start created_child effect before release (path after :: in effects[].id, or residualResources of kind worktree). #18412 has that recipe rebased onto the current split guide, plus fail-closed for unconfirmed release, follow-up Dispatch reuse, and unpushed commits on every outcome — not only STOP/fail.

Happy to fold those into this PR instead if that's the preferred landing path.

@fettpl

fettpl commented Sep 18, 2026

Copy link
Copy Markdown

Overlaps #18412 (opened 2026-09-03). That PR already captures <worktreePath> from the worker-start created_child effect before worker-release (after release, worker-show returns terminal: null, so terminal.worktreePath is not a path source), fail-closes on unpushed commits for every outcome, and pins the contract in orchestration-skill-guidance.test.mjs. Prefer landing #18412.

This branch has not been deployed

No deployments
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.

Orchestration coordinators should remove created-child worktrees after worker-release

2 participants