F2 slice 2: worktree-per-track reuse, serialization, multi-track concurrency (#13)

[zaridan]

Part of #5 / #13. Builds on F2 slice 1 (lineage-visible worktrees) which shipped the single-task, opt-in, default-off worktreeBacked bridge. Slice 2 makes the track abstraction (design §3.3) real.

What this does

All under the existing default-off worktreeBacked path:

1. track: spec hint — a task may declare track: <key> in its spec text (same low-friction channel as F1's allow-stale-base). Parsed by parseTrackFromSpec and stripped from the worker's --- TASK --- block. Default when unset = the task's own id → per-task (slice-1) behavior preserved.
2. Same-track worktree reuse — an in-memory Map<trackKey, …>. The first dispatch of a track lazily creates the child worktree (slice-1 path); later same-track tasks reuse it (dispatch a terminal into the existing checkout). This is the implement → review handoff: a review task declaring track: <implement-key> runs in implement's worktree and sees its commits → one branch → one PR.
3. Per-track serialization — one active dispatch per track. A ready task on a busy track waits (stays ready, re-evaluated next tick — same shape as the legacy "no idle terminal" wait), so two agents never edit one checkout. Tracks claimed earlier in a tick count too.
4. Multi-track concurrency under maxConcurrent — distinct tracks run concurrently. Effective parallelism = min(maxConcurrent, #distinct ready tracks with no in-flight dispatch).

Hard constraints honored (these bit us in slice 1)

• Default-off byte-for-byte — all new logic is gated by worktreeBacked; a legacy spec with no track: line strips to itself, so the bare-terminal path is unchanged. Covered by tests.
• Agent-readiness before preamble — the waitForTerminal(tui-idle) gate is applied to reused agent terminals too (same race).
• Breaker accounting — every provisioning/serialization/dispatch failure burns the circuit breaker so retries converge; reuse failures do not tear down the shared worktree (it holds the predecessor's work).
• Real-adapter coverage — a test drives a real Coordinator against the real OrcaRuntimeService.createWorktree adapter (not the coordinator mock) to prove the reuse path.

Tests (fail without the fix)

• Two tasks same trackKey → one worktree, serialized (2nd waits until 1st completes), 2nd runs in the 1st's worktree/terminal.
• Two tasks distinct trackKeys → two worktrees, concurrent under maxConcurrent (plus a maxConcurrent=1 bound check).
• track: hint parsed + stripped from the preamble (worktree-backed and legacy paths).
• parseTrackFromSpec unit tests.
• Real-adapter reuse path: one adapter worktree, serialized, 2nd into the 1st's interactive terminal, no recreate.

Deferred (out of scope)

• Cross-track base-ref-from-predecessor-tip (same-track reuse already gives the handoff since they share a worktree) — design §10 Q5.
• F3 resume / adopt-existing-worktree map seeding — TODOs in the track map reference Bridge F3: Coordinator resume-on-boot (kill the zombie-director failure) #14.

Verification

• orchestration + Mission Control + runtime vitest: green (638 in the orchestration/MC/runtime set; 521 across the two touched files).
• typecheck (node/cli/web): green.
• electron-vite build: green.
• oxlint: clean.

Do not merge — review first.

[zaridan]

Review panel verdict — not merge-ready (2 must-fix)

4-lens adversarial panel + synthesis, findings then verified against source by the coordinator. The two serious ones hit the canonical implement→review flow slice 2 exists for:

🔴 Must-fix

1. Reuse fallback downgrades an agent dispatch to a bare shell → task hangs forever. resolveTrackTerminal (coordinator.ts:903-906): when the cached agent terminal is gone, it createTerminal(id:<worktree>) → isAgent:false, then dispatchIntoExistingTrack sends the agent preamble into a plain shell. No worker_done ever arrives; per-track serialization keeps the track busy on a dead dispatch (stale-detection is warn-only). This is the implement→review path itself (implement's agent exits after its grace window → review reuses → bare shell). Fix: relaunch the worker agent in the reused worktree, or treat "cached agent terminal gone" as a breaker-accounted provision failure — never silently downgrade to a shell.
2. Within-track order non-deterministic → review can dispatch before implement. db.ts:634 ORDER BY created_at, no tie-break; created_at is second-granularity. Same-second implement+review → unspecified order. Per-track serialization gives mutual exclusion, NOT ordering, so review can review an empty branch. Fix: stable tie-break (ORDER BY created_at, id) AND/OR require same-track successors to declare deps:[predecessor] (which promoteReadyTasks honors) — and document it.

🟠 Should-fix

3. track: regex (coordinator.ts:91) matches anywhere incl. fenced code blocks — same limitation F1's allow-stale-base accepted, but track: is more prose-likely. Anchor to a header/top region (or document like F1).
4. Test gaps: reuse-path tui-idle gate + failure-convergence untested; no test that two no-track tasks get distinct worktrees; no test that same-track order is implement-first.

Clean / over-flagged (dropped)

Default-off path confirmed unchanged; F1 isolation respected; the "serialize-wait doesn't burn the breaker" and "default=task-id preserved" items were positive confirmations, not findings.

[zaridan]

Round 2 — all panel findings closed

Pushed 4 commits (82e72887 → f78819e1). Default-off + F1 isolation untouched (panel confirmed clean).

🔴 Must-fix #1 — reuse no longer downgrades an agent dispatch to a bare shell

resolveTrackTerminal now decides agent-vs-shell from the run config, not the cached flag. When a same-track successor reuses a worktree but the cached agent terminal is gone (implement's agent exits after its grace window), it relaunches the worker agent in the same worktree via createTerminal({ launchAgent }) — an agent terminal in the existing checkout, predecessor's commits preserved, never a new worktree. If a terminal can't be obtained it returns null → the caller breaker-accounts it and leaves the shared worktree intact, so the task converges to failed instead of hanging forever. The agent preamble is never sent into a non-agent shell. launchAgent added to the CoordinatorRuntime.createTerminal interface (optional/additive; the real adapter already supports it).

🔴 Must-fix #2 — within-track order is now deterministic

• Stable sort: listTasks/listTasksWithDispatch now ORDER BY created_at, id (second-granularity created_at alone was non-deterministic).
• Correct ordering via deps: same-track successors must declare deps:[predecessor] — promoteReadyTasks keeps review pending until implement finishes, so it never dispatches into an empty branch. Documented in skills/orchestration/SKILL.md (serialization gives mutual exclusion, not ordering). Tie-break is the within-tick backstop; deps is the guarantee.

🟠 Should-fix #3 — track: hint is fence-aware

parseTrackFromSpec scans line-by-line and skips fenced code blocks (``` / ~~~), so a track: line inside a worker-instruction example isn't parsed as the real key or stripped from the preamble.

🟠 Should-fix #4 — test gaps closed (all fail without their fix)

• reuse-path tui-idle gate (2nd wait precedes 2nd send on the reused agent terminal)
• reuse relaunch asserts launchAgent (coordinator mock and real-adapter)
• reuse failure → breaker-fail after strikes, shared worktree not torn down (removedWorktrees empty), no preamble into a shell
• two no-track tasks → distinct worktrees (default trackKey = task id)
• same-track deps:[predecessor] → implement dispatches first, then review reuses
• fenced track: ignored

Also

Wired --worktree-backed / --worker-agent on orchestration run (the RPC validated them but the CLI never forwarded them, so the documented workflow was unreachable). Default-off preserved; CLI test added.

Verification — all green

orchestration + MC + runtime + CLI vitest (767 passed) · typecheck (node/cli/web) · electron-vite build · oxlint. Not merged.

[zaridan]

Round-2 re-review — must-fix #1 NOT closed (blocker), verified against source

A scoped re-review panel + the coordinator's own read of the adapter agree:

🔴 BLOCKER — the reuse-relaunch still spawns a bare shell

resolveTrackTerminal (coordinator.ts ~947) relaunches via createTerminal(id:<worktree>, { launchAgent }) with no command. Verified in the adapter: createTerminal spawns agentTeamsPlan?.command ?? opts.command (orca-runtime.ts:14612); launchAgent is only a metadata tag (:14644) + reveal hint (:14667) and does not make the PTY run an agent. So the relaunched terminal is a plain shell labelled isAgent:true → the agent preamble goes into a shell → no worker_done → the track hangs (or spurious bounded failed). The round-1 symptom was relocated into the runtime layer, not fixed. The real agent-spawn path is the separate launchAgentTerminal (:14741), which CoordinatorRuntime doesn't expose.

🔴 BLOCKER — the "real-adapter" reuse test masks the defect

orca-runtime.test.ts mocks createTerminal and asserts only launchAgent === 'claude' — green precisely because it never exercises the real spawn. It must fail against today's bare-shell code.

🟠 Should-fix

• Same-track ordering not safe-by-default: ORDER BY created_at, id is stable but id is random → arbitrary order; review can sort before implement every tick. Implement-first relies entirely on operator-declared deps, which is unenforced. Enforce/warn for same-track successors without a predecessor dep.
• --worktree-backed without --worker-agent = worktree-backed bare-shell mode (workers can't emit worker_done). Couple the flags.

No merge. Round 3 dispatched: relaunch via launchAgentTerminal (real spawn), a test that fails against the bare shell, ordering safe-by-default, CLI flags coupled.

[zaridan]

Round 3 — the reuse-relaunch is now a REAL agent (confirmed against the adapter)

You were right: round-2's relaunch was still a bare shell. Confirmed against source — createTerminal spawns command: agentTeamsPlan?.command ?? opts.command (orca-runtime.ts:14612) and only stores launchAgent as a metadata tag (:14644) + reveal hint (:14667); it does NOT run an agent. The real spawn is launchAgentTerminal (:14741), which builds an actual launch command via buildStartupForAgent. Pushed 43de5581 → bf234d93.

🔴 Must-fix #1 — relaunch a real agent

resolveTrackTerminal now relaunches via runtime.launchAgentTerminal('id:'+worktreeId, { agent, prompt: '' }) — the real agent spawn, in the EXISTING worktree (predecessor's commits preserved). Empty prompt is fine (buildStartupForAgent uses allowEmptyPromptLaunch); the dispatch preamble follows via sendTerminal, exactly like slice-1's create→send flow. Added launchAgentTerminal to the CoordinatorRuntime interface (optional/additive); removed the obsolete createTerminal({launchAgent}) option and the false "createTerminal already supports launchAgent" comment. A runtime without launchAgentTerminal, or a failed relaunch, returns null → caller breaker-accounts it, shared worktree intact. An agent preamble is never sent into a non-agent terminal.

🔴 Must-fix #2 — the test now fails against the bare-shell code

The reuse-relaunch tests now assert the coordinator invokes launchAgentTerminal (the real spawn), and that the bare-shell createTerminal is not used for the agent — so they fail against round-2's code. Plus a real-adapter test that calls the real OrcaRuntimeService.launchAgentTerminal and asserts a non-empty agent launch command reaches the spawn primitive (proving it's a real agent, not a tagged shell).

🟠 Should-fix #3 — same-track ordering safe by default

id is randomBytes (non-monotonic), so the sort alone can't guarantee implement-first. decompose() now refuses (when worktreeBacked) any DAG where a track has tasks not totally ordered by deps — the run fails fast with an actionable error instead of silently racing review against an empty branch. No longer hinges on the operator remembering deps. Legacy path unaffected.

🟠 Should-fix #4 — couple the flags

orchestration.run now refuses --worktree-backed without --worker-agent (a bare-shell worker never emits worker_done). Enforced at the RPC boundary (authoritative for the CLI and all callers).

Nits

5. Corrected the ORDER BY comment (stabilizes read order; does NOT by itself guarantee implement-first). 6. Documented the fence parser's safe degradation on an unclosed fence (→ task-id default).

Verification — green

typecheck (node/cli/web) · electron-vite build · oxlint · full vitest 20391 passed (the lone failure is src/relay/subprocess.test.ts, an unrelated relay-timing test that passes in isolation). Same-track tests now declare deps:[predecessor] (the safe pattern). Not merged — ready for your independent check that the relaunch spawns a real agent.