F2 slice 1: coordinator creates lineage-visible worktrees (#13)

[zaridan]

F2 slice 1 — the bridge proof (#13)

Implements slice 1 of the green-lit F2-DESIGN.md §7: the single-task, opt-in, default-off proof that a coordinator run's worker shows up in Mission Control. Part of #5, builds on F1 (#12).

Problem

The coordinator dispatched tasks to bare terminals in one shared worktree and never created worktrees, while Mission Control discovers workers only by lineage (selectSpawnedWorktreeIds: parentWorktreeId === directorWorktreeId). So coordinator-driven work was invisible — "No worktrees yet" forever.

What this does

• createWorktree on CoordinatorRuntime (optional method) implemented on OrcaRuntimeService as a thin adapter over the existing createManagedWorktree, stamping lineage parent = the director worktree via orchestrationContext.parentWorktreeId. No git logic forked (base-ref handling, SSH/relay parity, lineage recording all inherited). No DB schema change.
• Opt-in worktreeBacked coordinator option, default false. When off, the legacy bare-terminal dispatch path is byte-for-byte unchanged (verified by test). When on, dispatchReadyTasks creates one child worktree per task (trackKey = task id), launches the worker agent in it via the startup option, and sends the existing preamble unchanged; the drift pre-flight + dispatch target move to the new track worktree.
• Lineage parent = director ⇒ selectSpawnedWorktreeIds finds the worker with no Mission Control change.
• Wired --worktree-backed / --worker-agent through orchestration.run (additive, default off).

Upstream-friendliness (stablyai#6201)

Additive by construction: optional interface method, opt-in default-off flag, no schema change, no forked git logic. The one behavior change (worker-in-worktree vs bare-terminal) is gated behind the default-off flag, so existing coordinator semantics are preserved unless a caller opts in.

Deferred to slice 2 (explicitly NOT built here)

track: spec hint parsing, same-track worktree reuse (the implement→review handoff), per-track serialization lock, multi-track concurrency under maxConcurrent, base-ref-from-predecessor-tip, and resume/adopt-existing-worktree (F3).

Test evidence

• Coordinator (main) test — worktreeBacked ON: asserts the created worker worktree's lineage.parentWorktreeId === directorWorktreeId, the preamble is dispatched into the child worktree's terminal, and the drift probe targets the new track worktree (not the director). Plus a default-off test proving the legacy bare-terminal path is unchanged, and a guard test that no parentless worktrees are created when the director worktree is unset.
• Mission Control (renderer) test — selectSpawnedWorktreeIds discovers a coordinator-created worktree-backed worker given the exact lineage shape the adapter emits (consumer half of the bridge; kept in the renderer project to respect the main↔renderer boundary).
• Unit test for the deterministic worktreeNameForTask slug.

All green locally:

• vitest orchestration + MC suites: 247 passed
• typecheck: node + web + cli projects all clean
• electron-vite build: ✓ built
• oxlint: clean

🤖 Generated with Claude Code

[zaridan]

Adversarial review — F2 slice 1 (worktree-backed dispatch)

Reviewed only the slice-1 delta (git diff d58eb158a..HEAD). Slice-2 absence not faulted. Read-only review.

Attack #1 — default-off "byte-for-byte unchanged" claim: HOLDS. ✅

Verified against git show d58eb158a:…/coordinator.ts. The legacy dispatchReadyTasks body (coordinator.ts:537-583) is identical below the new opt-in guard (coordinator.ts:532-535). The only change reaching the legacy path is inside dispatchTask, where this.opts.worktree became driftSelector (coordinator.ts:683, 691, 698-699). At the legacy callsite dispatchTask(task, targetHandle) (coordinator.ts:578) the 3rd arg is omitted, so driftSelector = undefined ?? this.opts.worktree === this.opts.worktree. No shared-variable, ordering, or terminal-selection leak. The off path is genuinely unaffected.

🔴 BLOCKER — startup-agent → preamble cold-boot race (attack #4)

dispatchReadyTasksInWorktrees launches the agent via startup: { agent } (coordinator.ts:631), then synchronously calls dispatchTask → sendTerminal(targetHandle, { text: preamble, enter: true }) (coordinator.ts:752) with no readiness wait. createWorktree returns as soon as the startup PTY is spawned, not when the agent TUI accepts input — createManagedWorktree returns startupTerminal.handle the moment createTerminal resolves (orca-runtime.ts:12595-12611), and sendTerminal writes immediately with zero gating (orca-runtime.ts:8046-8061). The runtime's own startup path guards exactly this race with sendStartupFollowupWhenReady/pasteStartupDraftWhenReady, which poll up to 30×150ms for the agent to become the foreground process before writing (orca-runtime.ts:11733-11775, 12603-12608). The coordinator bypasses that mechanism entirely. Failure mode: preamble keystrokes fired into a still-booting claude/codex TUI are dropped or mangled → worker silently never receives its task. This is the pasting-into-a-not-yet-ready-TUI bug class, newly introduced by this slice (legacy dispatches to already-idle terminals).

🟠 SHOULD-FIX — no circuit-breaker + unbounded retry + worktree leak on creation failure (attack #2)

When createWorktree (coordinator.ts:633-636) or the fallback createTerminal (coordinator.ts:649-652) throws, the path logs and continues, leaving the task in ready. Unlike the eventual sendTerminal failure — which runs through failDispatch → circuit-breaker → failed (coordinator.ts:756-764) — worktree/terminal-creation failures never touch the breaker and never give up. Two consequences:

• A task that can never get a worktree (disk full, stale director selector, disabled/invalid agent) retries every pollIntervalMs forever; the run never converges. Inconsistent with F1's careful breaker accounting.
• createManagedWorktree only throws after the worktree is created on disk + lineage recorded if a later step fails; the startup-terminal failure is caught internally (orca-runtime.ts:12612-12618) so createWorktree returns with terminalHandle undefined → coordinator hits the createTerminal fallback → if that throws, the just-created worktree is orphaned and the next tick creates another (new conflict-suffixed branch). Repeated failures ⇒ accumulating orphan worktrees with no cleanup. Recommend: bound retries (burn the breaker after N worktree-create failures) and/or tear down the worktree when the post-create terminal step fails.

🟠 SHOULD-FIX — adapter↔createManagedWorktree integration is entirely untested (attack #3)

The real adapter (OrcaRuntimeService.createWorktree, orca-runtime.ts:12754) has no test — coordinator.test.ts mocks it and orchestrator-mission-control-data.test.ts hand-builds the lineage map. Good news: selectSpawnedWorktreeIds keys only on parentWorktreeId/isLive (orchestrator-mission-control-data.ts:14-19), and the adapter sets parentWorktreeId: parent.id (orca-runtime.ts:2772), so the bridge contract is sound. But the mock diverges from the real adapter in ways that hide bugs:

• Mock always returns terminalHandle (coordinator.test.ts:~108); the real adapter returns it only when didSpawnStartup (orca-runtime.ts:12733-12742, gated on effectiveStartup && ptyController?.spawn). So the no-workerAgent fallback branch (coordinator.ts:643-653) is never exercised — and in that real case createManagedWorktree already spawned a plain initial terminal (orca-runtime.ts:12678) so the coordinator's fallback creates a second terminal in the worktree. Untested double-terminal.
• Mock never throws → the entire attack-fix(jira): non-browser User-Agent (XSRF 403) + surface search failures #2 failure path above has zero coverage.
• Mock returns branch: opts.name; the real adapter returns result.worktree.git?.branch ?? opts.name (orca-runtime.ts:2791), which may be a sanitized/conflict-suffixed branch. The test's child.branch === opts.name assertion would not hold against prod.

🟡 NIT — workerAgent unvalidated at the RPC boundary (attack #2 corollary)

orchestration-gates.ts:113 casts params.workerAgent as TuiAgent from a bare OptionalString with no enum validation. An invalid agent string flows to createManagedWorktree, which throws 'Selected agent is disabled…' (orca-runtime.ts:11925-11927) → createWorktree throws → the infinite-retry trap above. Validate against the TuiAgent set at the boundary and reject fast.

🟡 NIT — silent downgrade to legacy

dispatchReadyTasks (coordinator.ts:532) falls through to bare-terminal dispatch when worktreeBacked is set but the runtime lacks createWorktree, with no log. An operator who asked for worktree-backed dispatch gets the legacy path and no signal. One onLog line would close the surprise.

Clean

• Attack Epic: Smart Orcastrators — unify coordinator+lineage into one orchestration system #5 (F1 isolation): adapter leaves the run's target_key as the director and gives children their own ids (orca-runtime.ts:2748-2751 comment + 2772); coordinator_run_id/task_id are passed through orchestrationContext. No mis-stamping. ✅
• Attack Sync orchestration task DAG + dispatch detail to the renderer #6 (worktreeNameForTask): deterministic, slug restricted to [a-z0-9-] so no //\ and provider-neutral; empty-slug fallback handled (coordinator.ts:84-93). Minor: cross-run uniqueness leans on per-run-unique task ids — with F1 per-run DB isolation two runs could both hold task_1 → same orch-…-1 name; createManagedWorktree resolves the residual branch conflict so non-fatal, and per-run namespacing is explicitly slice 2. ✅ (nit-level)
• Attack Control Panel: live DAG render in WorkspaceCard/AgentRow/AgentStateDot vocabulary #7 (maxConcurrent>1): the dispatch loop awaits createWorktree/dispatchTask sequentially (coordinator.ts:618-663) and createDispatchContext holds the dispatch-lock, so no double-creation or terminal-pool race within or across ticks. ✅

Bottom line: default-off regression claim holds. The opt-in path ships two real correctness gaps (preamble race, no-breaker/leak on failure) plus a genuine test-honesty gap (the adapter itself is unexercised).

[zaridan]

Round 2 — review blocker + should-fixes closed

Thanks for the adversarial pass. All three substantive findings + both nits are addressed; the default-off path the review confirmed clean is untouched. Pushed in a5e82bc (fixes) + db06a68 (tests).

🔴 BLOCKER — preamble cold-boot race → fixed

dispatchTask now gates the preamble send on agent readiness by reusing the runtime's existing signal — waitForTerminal(handle, { condition: 'tui-idle', timeoutMs }) — before sendTerminal, not a hand-rolled poller. It runs only when an agent was launched (worktree-backed with a workerAgent); the legacy path passes the default awaitTerminalReady = false and is byte-for-byte unchanged. A readiness timeout rejects and is routed through the same failDispatch breaker as a send failure (bounded, not a hang).

• Test: happy path now asserts wait:term_wt_0 precedes send:term_wt_0; a new test proves that when the agent never reaches tui-idle, the preamble is never sent and the task is breaker-failed (fails without the gate).

🟠 No breaker + infinite retry + orphan leak → fixed

• Bounded retry: worktree/terminal provisioning failures (which happen before a real dispatch context exists) now burn the same F1 circuit breaker via a per-task sentinel dispatch context (createDispatchContext → failDispatch, carrying failure_count forward). After 3 strikes the task goes failed instead of retrying every pollIntervalMs forever.
• No orphans: a new optional removeWorktree capability tears down the just-created worktree when a post-create step fails (no usable terminal, readiness timeout, send failure, or a drift-skip that didn't dispatch). The adapter implements it via removeManagedWorktree(…, force=true).
• Tests: createWorktree-throws and no-usable-terminal cases both end failed with removedWorktrees.length === createdWorktrees.length (no leak); the throw-before-create case removes nothing.

🟠 Adapter mock divergence / untested failure path → fixed

The mock is now faithful to the real adapter: it can throw, can return no terminalHandle, and returns a resolved (possibly different) branch rather than the raw opts.name — so it can't hide these paths. Crucially, the double-terminal is removed at the source: when no workerAgent is set, the adapter now reuses the plain initial terminal createManagedWorktree already opened (found via listTerminals) instead of the coordinator creating a second one. New test asserts the no-agent path dispatches with zero coordinator-created terminals and no readiness wait (a plain shell accepts input immediately).

🟡 Nits

• --worker-agent is now validated against the TuiAgent set at the RPC boundary (orchestration-gates.ts) and rejected fast, so an invalid id can't feed the retry path.
• The worktreeBacked→legacy downgrade now logs exactly once (guarded flag); new test asserts the single line.

Verification

• orchestration + MC vitest: 252 passed (was 247; +5 round-2 tests)
• typecheck node/web/cli: clean · electron-vite build: ✓ · oxlint: clean

Still slice 1 only — track reuse/serialization/multi-track/resume remain deferred to slice 2. Not merged.

[zaridan]

Round-2 re-review

Re-reviewed only the round-2 delta (git diff b6d86c6cd..HEAD = a5e82bcbd fix + db06a6879 test). Read-only.

Round-1 BLOCKER (preamble cold-boot race): CLOSED. ✅

dispatchTask now gates the preamble on waitForTerminal(targetHandle, { condition: 'tui-idle', timeoutMs: 60_000 }) before sendTerminal (coordinator.ts:862-875), and only when awaitTerminalReady — set to workerAgent !== undefined (coordinator.ts:707). waitForTerminal genuinely rejects on timeout (new Error('timeout'), orca-runtime.ts:8370-8373), and that rejection is breaker-accounted in the existing failDispatch catch (coordinator.ts:868-877) then torn down. The test asserts both wait-before-send ordering and preamble-not-sent-on-timeout — honest, fails without the gate. Verified closed.

Mechanism 1 — sentinel dispatch context: SOUND. ✅

recordWorktreeProvisionFailure (coordinator.ts:707-733) uses assignee_handle = orch-provision:${task.id} and calls createDispatchContext then failDispatch synchronously (both are sync better-sqlite3 calls, no await between). So the row never lingers in dispatched past the call, and the F1 uniqueness guard (db.ts:752-762, scoped to assignee_handle + coordinator_run_id + status IN pending/dispatched) cannot collide: the handle is unique per task, disjoint from real term_* handles and from other tasks' sentinels, and is already failed/circuit_broken by next tick. failure_count carries forward via MAX(failure_count) (db.ts:766-769), so sentinel strikes and real-dispatch strikes accumulate into the same 3-strike budget — no double-count, no reset, no confusion with worker_done/heartbeat (the sentinel's dispatchId is never transmitted to any worker; stale-detection and heartbeats filter on status='dispatched'). At <3 strikes the task returns to ready; at 3 it's failed and pushed to failedTasks so the run converges. This correctly closes round-1 should-fix #2.

• 🟡 NIT (theoretical): if failDispatch threw between the create and the fail (coordinator.ts:711-712), the task would strand in dispatched forever (never re-dispatched). It's a plain UPDATE so practically can't throw, but the two writes aren't wrapped in a transaction.

Mechanism 2 — removeWorktree teardown: one real gap.

Teardown is correctly scoped — teardownWorktree (coordinator.ts:736-746) only fires on genuine failure: no-usable-terminal (coordinator.ts:679-687), dispatch-throw (coordinator.ts:735-741), and drift-skip (coordinator.ts:751-757). It never runs on a successful live dispatch. removeManagedWorktree(force=true) is fine here (fresh unpublished worktree, no work to protect), dedups concurrent removals by id (orca-runtime.ts:14114-14120), and teardown errors are swallowed not thrown. No double-teardown (each id is created+possibly-removed within one sequential loop iteration).

• 🟠 SHOULD-FIX — drift-skip teardown can loop forever with no breaker (coordinator.ts:751-757). When dispatchTask returns false (drift pre-flight skipped: behind > threshold && !allow-stale), the fresh worktree is torn down, the task stays ready, and no breaker strike is recorded. probeWorktreeDrift measures the worktree HEAD against the remote-tracking base after a fetch (orca-runtime.ts:13496-13519), so a child branched from a stale local base (origin/base >20 ahead) reports behind > 20 on every freshly created worktree. Result: create → drift-skip → teardown, repeated every poll tick forever — never converges, never fails, unbounded git worktree add/remove churn. The code comment (coordinator.ts:752-755) calls this "effectively unreachable (it branches from base, so drift ≈ 0)" — that's wrong whenever the local base the worktree branches from is itself behind origin by more than the threshold. Narrow precondition (stale local base, allow-stale-base overrides it), but it reopens the exact "never converges" failure mode round 2 set out to kill — just for the drift path instead of the provisioning path. Suggest: breaker-account the worktree-mode drift-skip, or cap recreate attempts, or probe drift before createWorktree.

Mechanism 3 — terminal reuse via listTerminals (no-agent path): untested + a selection hazard.

The adapter reuses an existing terminal via listTerminals(id:${worktreeId}, 1) → terminals[0]?.handle (orca-runtime.ts:12798-12802). The worktree scope is a brand-new id, so it can't grab another run's/stale terminal, and an empty list → undefined handle → coordinator tears down + breaker-accounts (covered). But:

• 🟠 SHOULD-FIX — positional [0] can grab the Setup terminal. On the no-agent path, createManagedWorktree opens a plain initial terminal and, when a setup script is configured, a separate "Setup" terminal (orca-runtime.ts:12678 + 12699-12703). listTerminals returns leaves then PTYs in Map/spawn order and slices to limit (orca-runtime.ts:7667-7703); [0] is whichever registered first. If that's the Setup runner, the agent preamble is dispatched into the setup process, not an interactive shell. This selection is not exercised by any test — the coordinator mock returns the handle directly, so the adapter's real listTerminals pick is untested (same adapter-untested gap as round 1, now extended to reuse). Suggest selecting by role/title rather than position, or returning the known initial-terminal handle from createManagedWorktree directly.
• Readiness of the reused plain shell: low risk — a shell PTY buffers stdin, so input isn't lost the way a redrawing TUI drops it. The reply's "accepts input immediately" is close enough. (Separately: sending an agent preamble into a bare shell on the no-agent path means the shell tries to execute it — but that's the slice-1 deferred-agent design, out of scope.)

Default-off still byte-for-byte after +167 lines: CONFIRMED. ✅

awaitTerminalReady defaults false (coordinator.ts:782), the legacy callsite passes 2 args (coordinator.ts:604) so the if (awaitTerminalReady) wait block is skipped and sendTerminal runs exactly as before; the Promise<void>→Promise<boolean> return is ignored at the legacy callsite. The new missing-createWorktree warn-guard (coordinator.ts:552-563) is gated on this.opts.worktreeBacked, so it's inert when off. The legacy dispatchReadyTasks body below it is unchanged.

New negative tests honest? Mostly yes.

preamble-not-sent-on-timeout, disk-full breaker-fail, no-terminal teardown, and no-agent-zero-extra-terminals each genuinely fail without their fix (they assert empty sentMessages / status==='failed' / removedWorktrees.length === createdWorktrees.length), and the mock was tightened to throw and to omit the handle — good. Gaps: all are mock-level, so the two real-adapter behaviors above (positional listTerminals reuse, and the drift-skip teardown loop) have no coverage; the drift-skip dispatched===false branch in particular is never driven in a worktree-backed test.

Verdict

Round-1 blocker is closed. Sentinel breaker (mechanism 1) is sound. Teardown (mechanism 2) and terminal-reuse (mechanism 3) each ship one should-fix: the drift-skip teardown can churn-loop forever with no breaker, and the adapter's positional terminal pick can land the preamble in a Setup terminal — both currently untested.

[zaridan]

Round 3 — both re-review should-fixes closed

Thanks for the re-review. Mechanism 1 (sentinel breaker) and the default-off path are untouched, as asked. Both should-fixes closed with real-adapter coverage. Pushed in 1ca5bb4 (fixes) + 5776ccd (tests).

🟠 #1 — drift-skip teardown churn loop → fixed (probe before create)

Took the clean option: the drift pre-flight is extracted into resolveDispatchDrift and, in worktree-backed mode, runs on the director worktree BEFORE createWorktree. A stale base now skips without ever creating (or removing) a worktree — recoverable and retried next tick exactly like the legacy path, with zero git worktree add/remove churn. The resolved drift is threaded into dispatchTask so the fresh child is never re-probed (the old behind>threshold-on-every-child trigger is gone). The legacy callsite still computes drift internally → byte-for-byte unchanged (re-confirmed: dispatchTask(task, targetHandle) → resolveDispatchDrift(task, opts.worktree), same probe/log/skip/preamble; the if (drift.skip) return false is ignored at the legacy callsite).

• Test (fails without the fix): stale base over a few ticks → createdWorktrees.length === 0, removedWorktrees.length === 0, nothing sent, drift probed on the director (not a child), task stays ready. The old path would create→teardown every tick.

🟠 #2 — positional terminal pick could grab the Setup terminal → fixed (return the known handle)

Took the clean option: createManagedWorktree now surfaces the plain initial interactive terminal's handle on its result (initialTerminal). The adapter uses startupTerminal?.handle ?? initialTerminal?.handle and no longer re-discovers positionally via listTerminals[0], so the preamble can't land in the "Setup" runner.

• Tests at the real adapter (createManagedWorktree stubbed): asserts createWorktree returns term-initial (not the Setup-first term-setup) and never calls listTerminals; returns the startup-agent terminal when one was launched; returns no handle when none was opened. The existing setup-policy test confirms initialTerminal is spawn feat(orchestrators): Orcastrator — director sidebar, Send to Orcastrator, and Mission Control #1 (interactive, command: undefined), not spawn fix(jira): non-browser User-Agent (XSRF 403) + surface search failures #2 (Setup).

Untouched

• Mechanism 1 sentinel breaker — unchanged. (The theoretical "failDispatch throws between the two writes" NIT is left as-is: a plain UPDATE that can't realistically throw; out of scope for these two should-fixes.)
• Default-off path — re-confirmed byte-for-byte after the resolveDispatchDrift extraction.

Verification

• orchestration + MC + orca-runtime vitest: 726 passed (+4 new round-3 tests; 1 existing strict-equality test updated for the additive initialTerminal field)
• typecheck node/web/cli: clean (shared-type change checked across all three) · electron-vite build: ✓ · oxlint: clean

Still slice 1 only. Not merged.