From ea8ea0811685d39af6b2cd579deeb061b2164865 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 4 May 2026 12:10:53 -0700 Subject: [PATCH] =?UTF-8?q?feat(orchestration):=20bundled=20improvements?= =?UTF-8?q?=20=E2=80=94=20check-wait,=20stale-base,=20preamble+ask,=20QoL?= =?UTF-8?q?=20(#1403)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(orchestration): transport keepalive + delivered_at split for check --wait Implements the four §3 fixes from the check-wait design doc: - §3.1 Transport keepalive: long-poll RPCs (orchestration.check --wait) emit `{"_keepalive":true}` frames every 10s so neither server nor client tears the socket down on idle. A `longPoll` admission counter capped at 16 fails fast with `runtime_busy` when saturated; an AbortController wired through the RPC dispatcher cancels the inner waiter the moment the socket closes. - §3.2 delivered_at split: push-on-idle now stamps `delivered_at` instead of flipping `read`, so the check caller remains the sole consumer of its queue. Adds a synchronous idempotent schema migration that hard-fails on error. - §3.3 inbox/check parity: `orchestration inbox --terminal ` and `orchestration check --all` agree on the same rows (sequence DESC, no mark-read). `check --unread=false` kept for one release as a compat shim. - §3.4 CLI heartbeat: `orca orchestration check --wait` emits JSON heartbeat lines to stderr every 15s so Claude Code's Bash tool sees continuous output and doesn't auto-background the subprocess. Tests: extends runtime-rpc, orca-runtime, envelope-schema, orchestration method, and formatter suites; adds a subprocess test that spawns the built CLI and verifies stderr line-flushing, heartbeat cadence, and stdout cleanliness end-to-end. Co-authored-by: Orca * feat(orchestration): preamble rules + heartbeat schema - Preamble (#7, #15, #9): worker_done body ("3-sentence summary" + reportPath), BEHAVIOR RULE #1 forbidding AskUserQuestion, heartbeat every 5 minutes with taskId+dispatchId payload, AFTER YOU SEND grace window. - Schema v2 migration: adds 'heartbeat' to messages.type CHECK, adds dispatch_contexts.last_heartbeat_at, gated by user_version PRAGMA with transactional rebuild + explicit CREATE INDEX to avoid silent perf regress. - DB helpers: recordHeartbeat (dispatched-only), getStaleDispatches, getThreadMessagesFor (thread+handle scoped for ask). Co-authored-by: Orca * feat(orchestration): coordinator heartbeat + stale detector Handle incoming 'heartbeat' messages by calling recordHeartbeat keyed on payload.dispatchId (strict — log-and-skip if missing, no taskId fallback so a straggler heartbeat from a previously-failed dispatch cannot mask a hung retry per §5.3.4). On every tick after the 10-minute threshold, emit one log per stale dispatched row — no auto-fail. Also threads dispatchId through buildDispatchPreamble so workers can attribute their heartbeats back to the correct dispatch context. Co-authored-by: Orca * feat(orchestration): orca orchestration ask verb Adds a CLI verb that sends a decision_gate message and blocks on the coordinator's reply, scoped to the outbound message's thread. Group addresses (@all, @idle, …) are rejected — fan-out questions must use send --type decision_gate explicitly. --json emits bare single-line JSON (bypassing printResult) so workers can pipe `orca orchestration ask … --json | jq -r .answer` without unwrapping an RPC envelope; human mode prints just the answer. On timeout the verb exits 1 and returns {answer: null, timedOut: true}. This is the CLI surface BEHAVIOR RULE #1 in the dispatch preamble points workers at instead of AskUserQuestion. Co-authored-by: Orca * feat(orchestration): QoL bundle — preamble visibility, status enum, dispatch cross-ref, inbox --full Addresses four items from ORCHESTRATOR_FEEDBACK: - #5 preamble visibility: `dispatch-show --preamble` regenerates the preamble text for a task; `dispatch --inject --dry-run` previews without mutating state; `dispatch --return-preamble` echoes the injected preamble in the JSON response so coordinators can audit what a worker received. - #6 status enum validation: CLI rejects unknown `task-update --status` values with `invalid status '', expected one of: pending, ready, dispatched, completed, failed, blocked` before the RPC's generic Zod message. Valid statuses are listed under Notes in `task-update --help`. - #13 task-list dispatch cross-ref: `task-list --json` now includes `assignee_handle` and `dispatch_id` for tasks in status=dispatched via a read-only LEFT JOIN on dispatch_contexts. Non-dispatched rows keep their legacy shape so existing consumers are unaffected. - #14 inbox body visibility: `inbox --full` prints body + payload verbatim; default output is unchanged (id/from/to/subject only). No DB migrations; join-only change on dispatch_contexts so the sibling preamble PR's `last_heartbeat_at` column addition will not conflict. Co-authored-by: Orca * fix(worktree): prevent stale-base worktree creation and dispatch Addresses feedback #16 per DESIGN_DOC_STALE_BASE_FIX.md §0. Four v1 components coordinated by a single shared fetch cache on the runtime: 1. Concurrent-fetch-with-gate in UI create path: `createLocalWorktree` fires `git fetch` BEFORE the suffix loop / PR probe / path resolution, then awaits right before `addWorktree` so the new branch always spawns from a fresh remote tip. Renderer sees a two-phase spinner via the new `createWorktree:progress` IPC event. The cache is a `Map>` + 30s success-only timestamp on `OrcaRuntimeService` (§7.1 — shared with dispatch). 2. Dispatch pre-flight drift guard in `Coordinator.dispatchTask`: probes `rev-list --left-right --count` against the target worktree and silently returns (preserves `ready`, no circuit-breaker burn) when `behind > 20` unless the task spec carries `allow-stale-base: true`. Parsing strips the flag so it never leaks into the worker's `--- TASK ---` block. 3. Preamble drift section: populated only when dispatch detected drift. Workers see `--- BASE DRIFT ---` with the N-most-recent subjects they don't have, so they can pull them in before running. 4. §3.3 Lifecycle: `.finally()` evicts Map entries on BOTH success and rejection; timestamp is written ONLY on success. Prevents a single DNS hiccup from wedging every future create on the repo until restart, and keeps the freshness window honest. Defers the DB `allow_stale_base` column (§0.2) and the create-time warn toast; both can layer in later without migration. Tests: 35 new/updated unit tests covering drift preamble, dispatch refusal, spec-text flag parsing, fetch Map eviction after rejection, freshness-window short-circuit, and concurrent-caller serialization. Co-authored-by: Orca * test(orchestration): seed v2 DB in migration hard-fail test After consolidating the schema bump, fresh DBs are initialized directly at v3 via createTables(), so the v2→v3 ALTER TABLE is skipped on new installs and the prior test's stub never fired. Seed a v2-shape file on disk so the guarded ALTER actually runs and the "simulated migration failure" stub propagates as intended. Co-authored-by: Orca --------- Co-authored-by: Orca --- src/cli/handlers/orchestration.ts | 247 +++++++++-- src/cli/index.test.ts | 25 ++ src/cli/orchestration.subprocess.test.ts | 158 +++++++ src/cli/runtime/client.ts | 41 +- src/cli/runtime/envelope-schema.test.ts | 35 +- src/cli/runtime/envelope-schema.ts | 25 +- src/cli/runtime/transport.ts | 145 +++++-- src/cli/specs/orchestration.ts | 44 +- src/main/git/repo.ts | 57 +++ src/main/ipc/worktree-remote.ts | 93 +++-- src/main/ipc/worktrees-windows.test.ts | 9 +- src/main/ipc/worktrees.test.ts | 11 +- src/main/ipc/worktrees.ts | 2 +- src/main/runtime/fetch-remote-cache.test.ts | 106 +++++ src/main/runtime/orca-runtime.test.ts | 58 ++- src/main/runtime/orca-runtime.ts | 172 +++++++- .../__snapshots__/preamble.test.ts.snap | 83 ++++ .../runtime/orchestration/coordinator.test.ts | 395 +++++++++++++++++- src/main/runtime/orchestration/coordinator.ts | 153 ++++++- src/main/runtime/orchestration/db.test.ts | 308 ++++++++++++++ src/main/runtime/orchestration/db.ts | 264 +++++++++++- .../runtime/orchestration/formatter.test.ts | 1 + .../runtime/orchestration/preamble.test.ts | 211 ++++++++-- src/main/runtime/orchestration/preamble.ts | 136 +++++- src/main/runtime/orchestration/types.ts | 3 + src/main/runtime/rpc/core.ts | 7 + src/main/runtime/rpc/dispatcher.ts | 7 +- .../runtime/rpc/methods/orchestration.test.ts | 356 +++++++++++++++- src/main/runtime/rpc/methods/orchestration.ts | 223 +++++++++- src/main/runtime/runtime-rpc.test.ts | 297 ++++++++++++- src/main/runtime/runtime-rpc.ts | 162 ++++++- 31 files changed, 3569 insertions(+), 265 deletions(-) create mode 100644 src/cli/orchestration.subprocess.test.ts create mode 100644 src/main/runtime/fetch-remote-cache.test.ts create mode 100644 src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap diff --git a/src/cli/handlers/orchestration.ts b/src/cli/handlers/orchestration.ts index be807c28a2f..4520d33888a 100644 --- a/src/cli/handlers/orchestration.ts +++ b/src/cli/handlers/orchestration.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- Why: orchestration CLI handlers share flag-parsing helpers and dispatch/preamble logic; splitting by verb would fragment the RuntimeClient call shape without reducing complexity. */ import type { CommandHandler } from '../dispatch' import { printResult } from '../format' import { @@ -5,14 +6,70 @@ import { getOptionalStringFlag, getRequiredStringFlag } from '../flags' +import { RuntimeClientError } from '../runtime-client' import { getTerminalHandle } from '../selectors' +// Why: 15 s is well under Claude Code's empirical ~2 min Bash-tool silence +// budget and generates only ~40 lines per 10 min wait — enough to assure the +// parent process the subprocess is alive without flooding logs. See design +// doc §3.4. +const DEFAULT_HEARTBEAT_INTERVAL_MS = 15_000 + +// Why: test-only escape hatch so subprocess tests can verify the feature in +// under 10 s rather than needing a full 15 s silence window. Production users +// should never set this — there is no surface documentation. A bogus value +// falls back to the default rather than disabling the heartbeat. +function resolveHeartbeatIntervalMs(): number { + const raw = process.env.ORCA_HEARTBEAT_INTERVAL_MS + if (!raw) { + return DEFAULT_HEARTBEAT_INTERVAL_MS + } + const parsed = Number(raw) + if (!Number.isFinite(parsed) || parsed <= 0) { + return DEFAULT_HEARTBEAT_INTERVAL_MS + } + return parsed +} + +function startCheckHeartbeat(deadlineMs: number | undefined): () => void { + const startedAt = Date.now() + const interval = setInterval(() => { + const payload = { + _heartbeat: true, + elapsedMs: Date.now() - startedAt, + deadlineMs: deadlineMs ?? null + } + // Why: `process.stderr.write` is line-flushed per-call in Node, whereas a + // fully-buffered writer would hold all heartbeat lines until exit and + // silently defeat the whole point of the ping. Subprocess test asserts + // this by reading stderr incrementally. See §3.4. + process.stderr.write(`${JSON.stringify(payload)}\n`) + }, resolveHeartbeatIntervalMs()) + if (typeof interval.unref === 'function') { + interval.unref() + } + return () => clearInterval(interval) +} + +// Why: mirrors TaskStatus (orchestration/types.ts) so the CLI can surface a +// clear enum-aware error before the generic RPC Zod "Missing --status" message. +const TASK_STATUS_VALUES = [ + 'pending', + 'ready', + 'dispatched', + 'completed', + 'failed', + 'blocked' +] as const + type MessageSummary = { id: string from_handle: string to_handle?: string subject: string type?: string + body?: string + payload?: string | null } async function resolveOrchestrationTerminalHandle( @@ -62,18 +119,36 @@ export const ORCHESTRATION_HANDLERS: Record = { 'orchestration check': async ({ flags, client, cwd, json }) => { const terminal = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'terminal') - const result = await client.call<{ + const wait = flags.has('wait') + const timeoutMs = flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : undefined + + // Why: Claude Code's Bash tool auto-backgrounds subprocesses that produce + // no output for ~2 min (shorter on the non-interactive path). Emit a + // heartbeat line to stderr every HEARTBEAT_INTERVAL_MS while the wait is + // active so the parent process can see the subprocess is still alive. + // Stderr rather than stdout so stdout stays a single final JSON payload, + // and JSON-shaped rather than `# …` so `2>&1 | jq` pipelines still work + // (jq refuses `#`-prefixed lines). See design doc §3.4. + const stopHeartbeat = wait ? startCheckHeartbeat(timeoutMs) : null + type CheckResult = { messages: MessageSummary[] count: number formatted?: string - }>('orchestration.check', { - terminal, - unread: flags.has('unread') ? true : undefined, - types: getOptionalStringFlag(flags, 'types'), - inject: flags.has('inject') ? true : undefined, - wait: flags.has('wait') ? true : undefined, - timeoutMs: flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : undefined - }) + } + let result: Awaited>> + try { + result = await client.call('orchestration.check', { + terminal, + unread: flags.has('unread') ? true : undefined, + all: flags.has('all') ? true : undefined, + types: getOptionalStringFlag(flags, 'types'), + inject: flags.has('inject') ? true : undefined, + wait: wait ? true : undefined, + timeoutMs + }) + } finally { + stopHeartbeat?.() + } printResult(result, json, (r) => { if (r.formatted) { return r.formatted @@ -98,19 +173,36 @@ export const ORCHESTRATION_HANDLERS: Record = { }, 'orchestration inbox': async ({ flags, client, json }) => { + const full = flags.has('full') const result = await client.call<{ messages: MessageSummary[] count: number }>('orchestration.inbox', { - limit: getOptionalPositiveIntegerFlag(flags, 'limit') + limit: getOptionalPositiveIntegerFlag(flags, 'limit'), + terminal: getOptionalStringFlag(flags, 'terminal') }) printResult(result, json, (r) => { if (r.count === 0) { return 'No messages.' } + // Why: default output omits body/payload for at-a-glance sweeps; --full + // prints them verbatim so callers can audit without parsing --json. return r.messages - .map((m) => `${m.id} ${m.from_handle} -> ${m.to_handle ?? '?'}: "${m.subject}"`) - .join('\n') + .map((m) => { + const head = `${m.id} ${m.from_handle} -> ${m.to_handle ?? '?'}: "${m.subject}"` + if (!full) { + return head + } + const parts = [head] + if (m.body && m.body.length > 0) { + parts.push(m.body) + } + if (m.payload) { + parts.push(`[payload] ${m.payload}`) + } + return parts.join('\n') + }) + .join(full ? '\n\n' : '\n') }) }, @@ -128,7 +220,13 @@ export const ORCHESTRATION_HANDLERS: Record = { 'orchestration task-list': async ({ flags, client, json }) => { const result = await client.call<{ - tasks: { id: string; spec: string; status: string }[] + tasks: { + id: string + spec: string + status: string + assignee_handle?: string | null + dispatch_id?: string | null + }[] count: number }>('orchestration.taskList', { status: getOptionalStringFlag(flags, 'status'), @@ -138,16 +236,31 @@ export const ORCHESTRATION_HANDLERS: Record = { if (r.count === 0) { return 'No tasks.' } - return r.tasks.map((t) => `${t.id} [${t.status}] ${t.spec.slice(0, 60)}`).join('\n') + return r.tasks + .map((t) => { + const head = `${t.id} [${t.status}] ${t.spec.slice(0, 60)}` + if (t.status === 'dispatched' && t.assignee_handle) { + return `${head} -> ${t.assignee_handle} (${t.dispatch_id ?? '?'})` + } + return head + }) + .join('\n') }) }, 'orchestration task-update': async ({ flags, client, json }) => { + const status = getRequiredStringFlag(flags, 'status') + if (!TASK_STATUS_VALUES.includes(status as (typeof TASK_STATUS_VALUES)[number])) { + throw new RuntimeClientError( + 'invalid_argument', + `invalid status '${status}', expected one of: ${TASK_STATUS_VALUES.join(', ')}` + ) + } const result = await client.call<{ task: { id: string; status: string } }>( 'orchestration.taskUpdate', { id: getRequiredStringFlag(flags, 'id'), - status: getRequiredStringFlag(flags, 'status'), + status, result: getOptionalStringFlag(flags, 'result') } ) @@ -156,29 +269,95 @@ export const ORCHESTRATION_HANDLERS: Record = { 'orchestration dispatch': async ({ flags, client, cwd, json }) => { const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from') - const result = await client.call<{ - dispatch: { id: string; task_id: string; status: string } - }>('orchestration.dispatch', { - task: getRequiredStringFlag(flags, 'task'), - to: getRequiredStringFlag(flags, 'to'), - from, - inject: flags.has('inject') ? true : undefined, - devMode: isDevCliInvocation() - }) - printResult( - result, - json, - (r) => `Dispatched ${r.dispatch.task_id} -> ${r.dispatch.id} [${r.dispatch.status}]` - ) - }, - - 'orchestration dispatch-show': async ({ flags, client, json }) => { + const dryRun = flags.has('dry-run') ? true : undefined + const returnPreamble = flags.has('return-preamble') ? true : undefined + // Why: --to is only required for non-dry-run; the RPC handler re-enforces. + const to = dryRun ? getOptionalStringFlag(flags, 'to') : getRequiredStringFlag(flags, 'to') const result = await client.call<{ dispatch: { id: string; task_id: string; status: string } | null - }>('orchestration.dispatchShow', { - task: getRequiredStringFlag(flags, 'task') + injected?: boolean + dryRun?: boolean + preamble?: string + }>('orchestration.dispatch', { + task: getRequiredStringFlag(flags, 'task'), + to, + from, + inject: flags.has('inject') ? true : undefined, + dryRun, + returnPreamble, + devMode: isDevCliInvocation() }) printResult(result, json, (r) => { + if (r.dryRun) { + return r.preamble ?? '' + } + const base = `Dispatched ${r.dispatch?.task_id} -> ${r.dispatch?.id} [${r.dispatch?.status}]` + return r.preamble ? `${base}\n\n--- Preamble ---\n${r.preamble}` : base + }) + }, + + 'orchestration ask': async ({ flags, client, cwd, json }) => { + const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from') + const timeoutMs = flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : 600_000 + const result = await client.call<{ + answer: string | null + messageId: string | null + threadId: string + timedOut: boolean + }>( + 'orchestration.ask', + { + to: getRequiredStringFlag(flags, 'to'), + question: getRequiredStringFlag(flags, 'question'), + options: getOptionalStringFlag(flags, 'options'), + timeoutMs: flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : undefined, + from + }, + // Why: the runtime's `waitForMessage` can block up to `timeoutMs`, but + // the RPC transport has its own 60s default timeout that would fire + // first. Extend the per-call timeout by a small grace window so the + // RPC doesn't abort before the runtime's internal timeout resolves. + { timeoutMs: timeoutMs + 5_000 } + ) + // Why: deliberate bypass of `printResult`. `--json` on `ask` emits a + // single-line bare JSON object (no RPC envelope, no multi-line pretty- + // print) so workers can pipe `orca orchestration ask … --json | jq -r + // .answer` without reaching into a `result` envelope. This diverges from + // every other orchestration verb; called out in the commit message and + // guarded by a unit test in orchestration.test.ts. + if (json) { + console.log(JSON.stringify(result.result)) + } else if (result.result.answer !== null) { + console.log(result.result.answer) + } + if (result.result.timedOut) { + if (!json) { + console.error(`ask timeout after ${timeoutMs}ms (thread ${result.result.threadId})`) + } + process.exitCode = 1 + } + }, + + 'orchestration dispatch-show': async ({ flags, client, cwd, json }) => { + const showPreamble = flags.has('preamble') ? true : undefined + // Why: resolve --from when previewing so the preamble embeds a real + // coordinator handle, matching what an actual dispatch would produce. + const from = showPreamble + ? await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from') + : undefined + const result = await client.call<{ + dispatch: { id: string; task_id: string; status: string } | null + preamble?: string + }>('orchestration.dispatchShow', { + task: getRequiredStringFlag(flags, 'task'), + preamble: showPreamble, + from, + devMode: isDevCliInvocation() + }) + printResult(result, json, (r) => { + if (r.preamble && showPreamble) { + return r.preamble + } if (!r.dispatch) { return 'No dispatch context found.' } diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 8783968bf24..675c1105932 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -197,6 +197,31 @@ describe('orca cli worktree awareness', () => { expect(logSpy).toHaveBeenCalledWith('Sent 2 messages to 2 recipients') }) + it('rejects unknown task-update status with an enum-aware error', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_coord' + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const priorExitCode = process.exitCode + + await main( + ['orchestration', 'task-update', '--id', 'task_x', '--status', 'complete'], + '/tmp/repo' + ) + + const output = [...errSpy.mock.calls, ...logSpy.mock.calls] + .flat() + .map((v) => (typeof v === 'string' ? v : JSON.stringify(v))) + .join('\n') + expect(output).toContain("invalid status 'complete'") + expect(output).toContain('pending, ready, dispatched, completed, failed, blocked') + expect(callMock).not.toHaveBeenCalled() + expect(process.exitCode).toBe(1) + + // Reset exitCode so subsequent tests don't inherit the failure. + process.exitCode = priorExitCode + errSpy.mockRestore() + }) + it('passes dev mode to injected orchestration dispatches', async () => { process.env.ORCA_TERMINAL_HANDLE = 'term_sender' process.env.ORCA_USER_DATA_PATH = '/tmp/orca-dev' diff --git a/src/cli/orchestration.subprocess.test.ts b/src/cli/orchestration.subprocess.test.ts new file mode 100644 index 00000000000..c8648ceafe6 --- /dev/null +++ b/src/cli/orchestration.subprocess.test.ts @@ -0,0 +1,158 @@ +// Why: subprocess-level test for the CLI heartbeat behavior described in +// design doc §3.4. Spawns the real compiled CLI with no TTY, points it at a +// real in-process runtime via ORCA_USER_DATA_PATH, and asserts: +// - the first heartbeat line appears on stderr well under Claude Code's +// ~2 min Bash-tool silence budget (we verify with a shortened interval; +// production uses 15 s via the same code path) +// - ≥3 heartbeats arrive during the wait window +// - stderr is line-flushed (we observe each heartbeat as a separate chunk +// before the process exits — not in one burst at the end) +// - stdout stays a single clean JSON payload (no heartbeats leak to stdout) +// - a `jq "select(._heartbeat|not)"` filter on the merged stream would +// yield exactly the final result +// +// This test is skipped if the CLI hasn't been built yet (out/cli/index.js +// missing) so `pnpm test` works on a fresh checkout without requiring a prior +// `pnpm run build:cli`. The verification gate explicitly builds the CLI +// before running this file. +import { spawn } from 'child_process' +import { existsSync, mkdtempSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import { describe, expect, it } from 'vitest' +import { OrcaRuntimeService } from '../main/runtime/orca-runtime' +import { OrchestrationDb } from '../main/runtime/orchestration/db' +import { OrcaRuntimeRpcServer } from '../main/runtime/runtime-rpc' + +// Why: Vitest runs tests with `process.cwd()` pinned to the repo root, so +// join against it to locate the compiled CLI regardless of where this test +// file itself lives. +const CLI_PATH = join(process.cwd(), 'out', 'cli', 'index.js') + +const describeIfBuilt = existsSync(CLI_PATH) ? describe : describe.skip + +describeIfBuilt('orca orchestration check --wait subprocess (§3.4)', () => { + it('emits newline-flushed JSON heartbeats to stderr while waiting', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-cli-sub-')) + const runtime = new OrcaRuntimeService() + const db = new OrchestrationDb(':memory:') + runtime.setOrchestrationDb(db) + const server = new OrcaRuntimeRpcServer({ runtime, userDataPath }) + await server.start() + + try { + // Why: use the ORCA_HEARTBEAT_INTERVAL_MS escape hatch to shrink the + // test to ~1 s wall time. Production callers never set this; the + // production default (15 s) is exercised by §3.4's own unit tests + // and by the fact that this same code path runs with the real + // constant when the env var is absent. + const heartbeatMs = 200 + const waitTimeoutMs = 1200 + + const child = spawn( + process.execPath, + [ + CLI_PATH, + 'orchestration', + 'check', + '--wait', + '--timeout-ms', + String(waitTimeoutMs), + '--json' + ], + { + env: { + ...process.env, + ORCA_USER_DATA_PATH: userDataPath, + ORCA_TERMINAL_HANDLE: 'term_nobody', + ORCA_HEARTBEAT_INTERVAL_MS: String(heartbeatMs) + }, + // Why: explicit pipe for all three fds so we can watch stderr + // in real time; no TTY attached (Bash-tool parity). + stdio: ['ignore', 'pipe', 'pipe'] + } + ) + + const stderrChunks: { at: number; data: string }[] = [] + const stdoutChunks: { at: number; data: string }[] = [] + const startedAt = Date.now() + child.stderr.setEncoding('utf8') + child.stdout.setEncoding('utf8') + child.stderr.on('data', (d) => stderrChunks.push({ at: Date.now() - startedAt, data: d })) + child.stdout.on('data', (d) => stdoutChunks.push({ at: Date.now() - startedAt, data: d })) + + const exitCode = await new Promise((resolveExit, rejectExit) => { + child.once('exit', (code) => resolveExit(code ?? 1)) + child.once('error', rejectExit) + }) + + expect(exitCode).toBe(0) + + const stderr = stderrChunks.map((c) => c.data).join('') + const stdout = stdoutChunks.map((c) => c.data).join('') + + const heartbeatLines = stderr + .split('\n') + .filter((line) => line.trim().length > 0) + .map((line) => { + try { + return JSON.parse(line) as Record + } catch { + return null + } + }) + .filter((p): p is Record => p !== null && p._heartbeat === true) + + // ≥3 heartbeats in a 1.2s window with a 200ms interval + expect(heartbeatLines.length).toBeGreaterThanOrEqual(3) + expect(heartbeatLines[0]).toHaveProperty('elapsedMs') + expect(heartbeatLines[0]).toHaveProperty('deadlineMs', waitTimeoutMs) + + // Why: first heartbeat must arrive within one interval + scheduler + // slack (300ms is generous); if the stream were fully buffered we'd + // see everything only after exit. + const firstHeartbeatChunk = stderrChunks.find((c) => c.data.includes('_heartbeat')) + expect(firstHeartbeatChunk).toBeDefined() + expect(firstHeartbeatChunk!.at).toBeLessThan(heartbeatMs + 300) + + // Why: line-flushing proof — the *first* heartbeat chunk must arrive + // strictly before the exit chunk; i.e. we got at least two separate + // stderr data events (heartbeat + final). A single-chunk delivery + // would indicate stderr was buffered until exit. + const lastStderrAt = stderrChunks.at(-1)?.at ?? 0 + const firstStderrAt = stderrChunks.at(0)?.at ?? 0 + expect(lastStderrAt).toBeGreaterThan(firstStderrAt) + + // Stdout: exactly one JSON payload, the terminal result. No heartbeats + // leak, and the content parses as valid JSON. + const stdoutTrimmed = stdout.trim() + const stdoutPayload = JSON.parse(stdoutTrimmed) as Record + expect(stdoutPayload).not.toHaveProperty('_heartbeat') + expect(stdoutTrimmed).not.toContain('_heartbeat') + // Why: result should be an RPC success envelope with the expected + // shape. `count: 0` and `messages: []` because the wait timed out + // with no message for term_nobody. + expect(stdoutPayload).toMatchObject({ ok: true }) + + // Why: the heartbeats-on-stderr design is meant to pair with shell + // filters like `2>&1 | jq "select(._heartbeat|not)"`. jq is + // line-oriented by default, but also accepts pretty-printed JSON + // across multiple lines. What matters here is that every + // heartbeat line on stderr is a standalone JSON object (so jq can + // match it) and doesn't span multiple lines — assert that each + // heartbeat is a single-line JSON with no embedded newlines. + for (const line of stderr.split('\n')) { + if (line.trim().length === 0) { + continue + } + if (line.includes('_heartbeat')) { + expect(() => JSON.parse(line)).not.toThrow() + expect(line).not.toContain('\n') + } + } + } finally { + db.close() + await server.stop() + } + }, 30_000) +}) diff --git a/src/cli/runtime/client.ts b/src/cli/runtime/client.ts index e256bcdabd0..ae65cd4f34c 100644 --- a/src/cli/runtime/client.ts +++ b/src/cli/runtime/client.ts @@ -5,6 +5,14 @@ import { getCliStatus } from './status' import { sendRequest } from './transport' import { RuntimeClientError, RuntimeRpcFailureError, type RuntimeRpcSuccess } from './types' +// Why: for `orchestration.check --wait` the caller's method-level +// `params.timeoutMs` is the inner waiter budget; we extend the client-side +// socket timeout to `timeoutMs + GRACE_MS` so the client's own idle timer +// never fires before the server-side waiter has had a chance to resolve and +// emit its terminal frame. The 10 s grace absorbs round-trip + one final +// keepalive window. See design doc §3.1. +const LONG_POLL_CLIENT_GRACE_MS = 10_000 + export class RuntimeClient { private readonly userDataPath: string private readonly requestTimeoutMs: number @@ -25,18 +33,30 @@ export class RuntimeClient { } ): Promise> { const metadata = readMetadata(this.userDataPath) - const response = await sendRequest( - metadata, - method, - params, - options?.timeoutMs ?? this.requestTimeoutMs - ) + const effectiveTimeoutMs = options?.timeoutMs ?? this.resolveMethodTimeoutMs(method, params) + const response = await sendRequest(metadata, method, params, effectiveTimeoutMs) if (!response.ok) { throw new RuntimeRpcFailureError(response) } return response } + // Why: centralises the per-method timeout policy. `orchestration.check` with + // `wait: true` is the only long-poll today, and its inner waiter budget + // lives in `params.timeoutMs`. We widen the client-side socket timeout to + // `timeoutMs + grace` so it doesn't fire before the server has a chance to + // resolve. Without this, a 5 min wait would still die at the 60 s default. + // See design doc §3.1. + private resolveMethodTimeoutMs(method: string, params?: unknown): number { + if (method === 'orchestration.check' && isWaitingCheck(params)) { + const inner = Number((params as { timeoutMs?: unknown }).timeoutMs) + if (Number.isFinite(inner) && inner > 0) { + return Math.max(inner + LONG_POLL_CLIENT_GRACE_MS, this.requestTimeoutMs) + } + } + return this.requestTimeoutMs + } + async getCliStatus(): Promise> { return getCliStatus(this.userDataPath) } @@ -67,3 +87,12 @@ export class RuntimeClient { function delay(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)) } + +function isWaitingCheck(params: unknown): boolean { + return ( + typeof params === 'object' && + params !== null && + 'wait' in params && + (params as { wait: unknown }).wait === true + ) +} diff --git a/src/cli/runtime/envelope-schema.test.ts b/src/cli/runtime/envelope-schema.test.ts index 57f92ada298..b7315644f4b 100644 --- a/src/cli/runtime/envelope-schema.test.ts +++ b/src/cli/runtime/envelope-schema.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { RuntimeRpcEnvelopeSchema } from './envelope-schema' +import { RuntimeRpcEnvelopeSchema, isKeepaliveFrame } from './envelope-schema' describe('RuntimeRpcEnvelopeSchema', () => { it('accepts a well-formed success envelope', () => { @@ -57,4 +57,37 @@ describe('RuntimeRpcEnvelopeSchema', () => { const parsed = RuntimeRpcEnvelopeSchema.safeParse({ hello: 'world' }) expect(parsed.success).toBe(false) }) + + it('accepts a keepalive frame', () => { + const parsed = RuntimeRpcEnvelopeSchema.safeParse({ _keepalive: true }) + expect(parsed.success).toBe(true) + }) + + it('rejects a keepalive frame with _keepalive !== true', () => { + const parsed = RuntimeRpcEnvelopeSchema.safeParse({ _keepalive: false }) + expect(parsed.success).toBe(false) + }) +}) + +describe('isKeepaliveFrame', () => { + it('returns true for a well-formed keepalive', () => { + expect(isKeepaliveFrame({ _keepalive: true })).toBe(true) + }) + + it('returns false for a success envelope', () => { + expect(isKeepaliveFrame({ id: 'x', ok: true, result: {}, _meta: { runtimeId: 'r' } })).toBe( + false + ) + }) + + it('returns false for non-object inputs', () => { + expect(isKeepaliveFrame(null)).toBe(false) + expect(isKeepaliveFrame(undefined)).toBe(false) + expect(isKeepaliveFrame('keepalive')).toBe(false) + }) + + it('returns false when _keepalive is not strictly true', () => { + expect(isKeepaliveFrame({ _keepalive: 1 })).toBe(false) + expect(isKeepaliveFrame({ _keepalive: 'true' })).toBe(false) + }) }) diff --git a/src/cli/runtime/envelope-schema.ts b/src/cli/runtime/envelope-schema.ts index eeec3536005..f674aab11af 100644 --- a/src/cli/runtime/envelope-schema.ts +++ b/src/cli/runtime/envelope-schema.ts @@ -36,4 +36,27 @@ const Failure = z.object({ _meta: MetaFailure }) -export const RuntimeRpcEnvelopeSchema = z.discriminatedUnion('ok', [Success, Failure]) +// Why: transport-layer keepalive frame (server→client only). Not a terminal +// frame — the client reads past it and keeps waiting for the real +// success/failure. `id` and `_meta` are deliberately absent: keepalives carry +// no method-level semantics and aren't tied to a particular request (one +// connection handles one request today). See design doc §3.1. +const Keepalive = z.object({ + _keepalive: z.literal(true) +}) + +// Why: switched from z.discriminatedUnion('ok', …) to z.union because +// keepalives have no `ok` field. Client code must branch on +// `'_keepalive' in frame` before treating the frame as Success/Failure. +export const RuntimeRpcEnvelopeSchema = z.union([Success, Failure, Keepalive]) + +export type RuntimeRpcKeepaliveFrame = z.infer + +export function isKeepaliveFrame(frame: unknown): frame is RuntimeRpcKeepaliveFrame { + return ( + typeof frame === 'object' && + frame !== null && + '_keepalive' in frame && + (frame as { _keepalive: unknown })._keepalive === true + ) +} diff --git a/src/cli/runtime/transport.ts b/src/cli/runtime/transport.ts index bc39de0d332..7209a274c0f 100644 --- a/src/cli/runtime/transport.ts +++ b/src/cli/runtime/transport.ts @@ -1,7 +1,7 @@ import { createConnection } from 'net' import { randomUUID } from 'crypto' import type { RuntimeMetadata, RuntimeTransportMetadata } from '../../shared/runtime-bootstrap' -import { RuntimeRpcEnvelopeSchema } from './envelope-schema' +import { isKeepaliveFrame, RuntimeRpcEnvelopeSchema } from './envelope-schema' import { RuntimeClientError, type RuntimeRpcResponse } from './types' export async function sendRequest( @@ -13,9 +13,14 @@ export async function sendRequest( return await new Promise((resolve, reject) => { const socket = createConnection(getTransportEndpoint(metadata.transport!)) let buffer = '' + let settled = false const requestId = randomUUID() const timeout = setTimeout(() => { + if (settled) { + return + } + settled = true socket.destroy() reject( new RuntimeClientError( @@ -25,28 +30,72 @@ export async function sendRequest( ) }, timeoutMs) + const finish = ( + result: { ok: true; response: RuntimeRpcResponse } | { ok: false; error: Error } + ): void => { + if (settled) { + return + } + settled = true + clearTimeout(timeout) + socket.end() + if (result.ok) { + resolve(result.response) + } else { + reject(result.error) + } + } + socket.setEncoding('utf8') socket.once('error', () => { - clearTimeout(timeout) - reject( - new RuntimeClientError( + finish({ + ok: false, + error: new RuntimeClientError( 'runtime_unavailable', 'Could not connect to the running Orca app. Restart Orca and try again.' ) - ) + }) }) socket.on('data', (chunk) => { buffer += chunk - const newlineIndex = buffer.indexOf('\n') - if (newlineIndex === -1) { - return - } - const message = buffer.slice(0, newlineIndex) - socket.end() - clearTimeout(timeout) - let response: RuntimeRpcResponse - try { - const raw: unknown = JSON.parse(message) + // Why: the server may interleave `{"_keepalive":true}\n` frames with the + // final success/failure frame to keep both idle timers alive during a + // long-poll (see design doc §3.1). Read frames in a loop until we see a + // terminal frame. Each keepalive refreshes the client-side timer so a + // 10 min wait doesn't trip the 60 s default ceiling. + let newlineIndex = buffer.indexOf('\n') + while (newlineIndex !== -1 && !settled) { + const line = buffer.slice(0, newlineIndex) + buffer = buffer.slice(newlineIndex + 1) + if (line.trim().length === 0) { + newlineIndex = buffer.indexOf('\n') + continue + } + + let raw: unknown + try { + raw = JSON.parse(line) + } catch { + finish({ + ok: false, + error: new RuntimeClientError( + 'invalid_runtime_response', + 'The Orca runtime returned an invalid response frame.' + ) + }) + return + } + + // Fast-path: ignore keepalives without running the full schema. + // setTimeout().refresh() is stable since Node 10 (Orca ships on + // Node 20+ via Electron and the standalone CLI targets the same + // major). See §7 risk #9. + if (isKeepaliveFrame(raw)) { + timeout.refresh() + newlineIndex = buffer.indexOf('\n') + continue + } + // Why: validate the envelope shape (id, ok, result/error, _meta) at // the decode boundary so version skew between the CLI and the Orca // main runtime surfaces as a single invalid_runtime_response instead @@ -54,43 +103,49 @@ export async function sendRequest( // unknown — the TResult generic is the caller's responsibility. const parsed = RuntimeRpcEnvelopeSchema.safeParse(raw) if (!parsed.success) { - reject( - new RuntimeClientError( + finish({ + ok: false, + error: new RuntimeClientError( 'invalid_runtime_response', 'The Orca runtime returned an invalid response frame.' ) - ) + }) return } - response = parsed.data as RuntimeRpcResponse - } catch { - reject( - new RuntimeClientError( - 'invalid_runtime_response', - 'The Orca runtime returned an invalid response frame.' - ) - ) + + // Narrow out keepalive (already filtered above) so TS can see a + // Success|Failure shape here. + const frame = parsed.data + if ('_keepalive' in frame) { + timeout.refresh() + newlineIndex = buffer.indexOf('\n') + continue + } + + const response = frame as RuntimeRpcResponse + if (response.id !== requestId) { + finish({ + ok: false, + error: new RuntimeClientError( + 'invalid_runtime_response', + 'The Orca runtime returned a mismatched response id.' + ) + }) + return + } + if (response._meta?.runtimeId && response._meta.runtimeId !== metadata.runtimeId) { + finish({ + ok: false, + error: new RuntimeClientError( + 'runtime_unavailable', + 'The Orca runtime changed while the request was in flight. Retry the command.' + ) + }) + return + } + finish({ ok: true, response }) return } - if (response.id !== requestId) { - reject( - new RuntimeClientError( - 'invalid_runtime_response', - 'The Orca runtime returned a mismatched response id.' - ) - ) - return - } - if (response._meta?.runtimeId && response._meta.runtimeId !== metadata.runtimeId) { - reject( - new RuntimeClientError( - 'runtime_unavailable', - 'The Orca runtime changed while the request was in flight. Retry the command.' - ) - ) - return - } - resolve(response) }) socket.on('connect', () => { socket.write( diff --git a/src/cli/specs/orchestration.ts b/src/cli/specs/orchestration.ts index e527fe8b953..ac79fd00f92 100644 --- a/src/cli/specs/orchestration.ts +++ b/src/cli/specs/orchestration.ts @@ -23,8 +23,23 @@ export const ORCHESTRATION_COMMAND_SPECS: CommandSpec[] = [ path: ['orchestration', 'check'], summary: 'Check messages for a terminal', usage: - 'orca orchestration check [--terminal ] [--unread] [--types ] [--inject] [--wait] [--timeout-ms ] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'terminal', 'unread', 'types', 'inject', 'wait', 'timeout-ms'] + 'orca orchestration check [--terminal ] [--unread | --all] [--types ] [--inject] [--wait] [--timeout-ms ] [--json]\n' + + ' --unread (default): return only unread messages and mark them read.\n' + + ' --all: return every message for the handle; does not mark read.\n' + + ' --wait: block until a matching message arrives or --timeout-ms expires.\n' + + ' Emits JSON heartbeat lines to stderr every 15s so the caller can\n' + + ' tell the process is alive. Filter with `grep -v _heartbeat` or\n' + + ' `jq "select(._heartbeat|not)"` when merging streams with 2>&1.', + allowedFlags: [ + ...GLOBAL_FLAGS, + 'terminal', + 'unread', + 'all', + 'types', + 'inject', + 'wait', + 'timeout-ms' + ] }, { path: ['orchestration', 'reply'], @@ -34,9 +49,9 @@ export const ORCHESTRATION_COMMAND_SPECS: CommandSpec[] = [ }, { path: ['orchestration', 'inbox'], - summary: 'Show all messages across recipients', - usage: 'orca orchestration inbox [--limit ] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'limit'] + summary: 'Show messages across (or for) recipients', + usage: 'orca orchestration inbox [--limit ] [--terminal ] [--full] [--json]', + allowedFlags: [...GLOBAL_FLAGS, 'limit', 'terminal', 'full'] }, { path: ['orchestration', 'task-create'], @@ -56,20 +71,29 @@ export const ORCHESTRATION_COMMAND_SPECS: CommandSpec[] = [ summary: 'Update a task status', usage: 'orca orchestration task-update --id --status [--result ] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'id', 'status', 'result'] + allowedFlags: [...GLOBAL_FLAGS, 'id', 'status', 'result'], + notes: ['Valid --status values: pending, ready, dispatched, completed, failed, blocked.'] }, { path: ['orchestration', 'dispatch'], summary: 'Dispatch a task to a terminal', usage: - 'orca orchestration dispatch --task --to [--from ] [--inject] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'task', 'to', 'from', 'inject'] + 'orca orchestration dispatch --task --to [--from ] [--inject] [--dry-run] [--return-preamble] [--json]', + allowedFlags: [...GLOBAL_FLAGS, 'task', 'to', 'from', 'inject', 'dry-run', 'return-preamble'] }, { path: ['orchestration', 'dispatch-show'], summary: 'Show dispatch context for a task', - usage: 'orca orchestration dispatch-show --task [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'task'] + usage: + 'orca orchestration dispatch-show --task [--preamble] [--from ] [--json]', + allowedFlags: [...GLOBAL_FLAGS, 'task', 'preamble', 'from'] + }, + { + path: ['orchestration', 'ask'], + summary: 'Ask the coordinator a question and block until answered', + usage: + 'orca orchestration ask --to --question [--options ] [--timeout-ms ] [--from ] [--json]', + allowedFlags: [...GLOBAL_FLAGS, 'to', 'question', 'options', 'timeout-ms', 'from'] }, { path: ['orchestration', 'run'], diff --git a/src/main/git/repo.ts b/src/main/git/repo.ts index bdf8f7a7042..762ca6dd09f 100644 --- a/src/main/git/repo.ts +++ b/src/main/git/repo.ts @@ -223,6 +223,63 @@ export async function getBaseRefDefault(path: string): Promise { return getDefaultBaseRefAsync(path) } +/** + * Return { ahead, behind } for localRef vs remoteRef, or null on git failure. + * + * Why: `rev-list --left-right --count A...B` emits `\t` — + * ahead = commits on A not reachable from B; behind = commits on B not + * reachable from A. This is the merge-base-symmetric delta used by the + * stale-base dispatch guard (§3.1). Returning null on any failure (bad + * ref, corrupt repo, non-numeric output) lets callers degrade gracefully + * instead of failing dispatch on a probe error. + */ +export function getRemoteDrift( + repoPath: string, + localRef: string, + remoteRef: string +): { ahead: number; behind: number } | null { + try { + const stdout = gitExecFileSync( + ['rev-list', '--left-right', '--count', `${localRef}...${remoteRef}`], + { cwd: repoPath } + ) + const [aheadStr, behindStr] = stdout.trim().split(/\s+/) + const ahead = Number(aheadStr) + const behind = Number(behindStr) + if (!Number.isFinite(ahead) || !Number.isFinite(behind)) { + return null + } + return { ahead, behind } + } catch { + return null + } +} + +/** + * Up to `limit` commit subjects present on remoteRef but not localRef, in + * recency order. Returns [] on git failure. + * + * Why: powers the preamble drift section (§3.2) so a worker dispatched + * against an acknowledged-stale base can see at a glance whether the + * drift touches their task area. + */ +export function getRecentDriftSubjects( + repoPath: string, + localRef: string, + remoteRef: string, + limit: number +): string[] { + try { + const stdout = gitExecFileSync( + ['log', '--format=%s', '-n', String(limit), `${localRef}..${remoteRef}`], + { cwd: repoPath } + ) + return stdout.split('\n').filter((s) => s.trim().length > 0) + } catch { + return [] + } +} + /** * Parse `git remote` stdout into a count of configured remotes. * diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 09ef934fd8c..49385ef2f37 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -21,6 +21,7 @@ import { getPRForBranch } from '../github/client' import { listWorktrees, addWorktree, addSparseWorktree } from '../git/worktree' import { getGitUsername, getDefaultBaseRef, getBranchConflictKind } from '../git/repo' import { gitExecFileAsync } from '../git/runner' +import type { OrcaRuntimeService } from '../runtime/orca-runtime' import { isWslPath, parseWslPath, getWslHome } from '../wsl' import { createSetupRunnerScript, getEffectiveHooks, shouldRunSetupForCreate } from '../hooks' import { getSshGitProvider } from '../providers/ssh-git-dispatch' @@ -44,6 +45,20 @@ export function notifyWorktreesChanged(mainWindow: BrowserWindow, repoId: string } } +// Why (§3.3): two-phase spinner. Main process fires `'fetching'` immediately +// after kicking off `git fetch` and `'creating'` after that fetch resolves +// (or is determined to be cache-fresh). Renderer swaps its spinner label in +// response; fallback is the static "Creating worktree..." label if no event +// arrives (e.g. renderer races destruction of the window). +export function emitCreateWorktreeProgress( + mainWindow: BrowserWindow, + phase: 'fetching' | 'creating' +): void { + if (!mainWindow.isDestroyed()) { + mainWindow.webContents.send('createWorktree:progress', { phase }) + } +} + export async function createRemoteWorktree( args: CreateWorktreeArgs, repo: Repo, @@ -205,13 +220,55 @@ export async function createLocalWorktree( args: CreateWorktreeArgs, repo: Repo, store: Store, - mainWindow: BrowserWindow + mainWindow: BrowserWindow, + runtime?: OrcaRuntimeService ): Promise { const settings = store.getSettings() const username = getGitUsername(repo.path) const requestedName = args.name const sanitizedName = sanitizeWorktreeName(args.name) + + // Why (§3.3): determine the base branch (and therefore the remote we need to + // fetch) FIRST, so the fetch can overlap all pre-create work below. Neither + // of these calls depends on the suffix loop / PR probe / branch-conflict + // resolution, so they are safe to hoist. + const baseBranch = args.baseBranch || repo.worktreeBaseRef || getDefaultBaseRef(repo.path) + if (!baseBranch) { + // Why: getDefaultBaseRef may return null when none of origin/HEAD, + // origin/main, origin/master, local main, or local master exist. Don't + // fall back to a hardcoded 'origin/main' — passing a non-existent ref to + // `git worktree add` produces an opaque error. Fail here with a clear + // message so the UI can prompt the user to pick a base branch explicitly. + throw new Error( + 'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.' + ) + } + + // Why (§3.3 Lifecycle): fire fetch via the shared 30s-window cache on the + // runtime so repeat creates on the same repo reuse the in-flight promise + // and dispatch probes benefit from the freshness window. Kicked off BEFORE + // the suffix loop / PR probe / path resolution so those operations overlap + // the network round-trip — the `await` right before `addWorktree` is the + // only point that actually requires fetch completion. + // + // Why `runtime` is optional: a handful of legacy IPC test harnesses still + // call createLocalWorktree without the runtime. In that case we fall back + // to the old fire-and-forget behavior (which those tests already expect). + // Production `worktrees.ts` always passes runtime, so the happy path + // always gets the cache. + const remote = baseBranch.includes('/') ? baseBranch.split('/')[0] : 'origin' + const fetchPromise: Promise = runtime + ? runtime.fetchRemoteWithCache(repo.path, remote) + : gitExecFileAsync(['fetch', remote], { cwd: repo.path }) + .then(() => undefined) + .catch(() => undefined) + + // Why: emit a progress event so the renderer dialog can switch its spinner + // label to "Checking for updates..." while the fetch is in flight, then + // "Creating worktree..." after we await it. Renderer falls back to the + // static "Creating worktree..." label if no event arrives. + emitCreateWorktreeProgress(mainWindow, 'fetching') // Why: WSL worktrees live under ~/orca/workspaces inside the WSL // filesystem. Validate against that root, not the Windows workspace dir. // If WSL home lookup fails, keep using the configured workspace root so @@ -298,20 +355,6 @@ export async function createLocalWorktree( ) } - // Determine base branch. - // - // Why: getDefaultBaseRef may return null when none of origin/HEAD, - // origin/main, origin/master, local main, or local master exist. In that - // case we must not fall back to a hardcoded 'origin/main' — passing a - // non-existent ref to `git worktree add` produces an opaque error. Fail - // here with a clear message so the UI can prompt the user to pick a base - // branch explicitly. - const baseBranch = args.baseBranch || repo.worktreeBaseRef || getDefaultBaseRef(repo.path) - if (!baseBranch) { - throw new Error( - 'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.' - ) - } // Why: `ask` is a pre-create choice gate, not a post-create side effect. // Resolve it before mutating git state so missing UI input cannot strand // a real worktree on disk while the renderer reports "create failed". The @@ -348,16 +391,16 @@ export async function createLocalWorktree( } } - // Why: `git fetch` previously blocked worktree creation for 1–5s on every - // click, even though the fetch result isn't actually required — the - // subsequent `git worktree add` uses whatever local ref `baseBranch` points - // at. Kicking fetch off in parallel lets the worktree be created off the - // last-known tip while the fetch completes in the background; the next - // user action (pull, diff, PR create) will see the refreshed remote state. - const remote = baseBranch.includes('/') ? baseBranch.split('/')[0] : 'origin' - void gitExecFileAsync(['fetch', remote], { cwd: repo.path }).catch(() => { - // Fetch is best-effort — don't block worktree creation if offline - }) + // Why (§3.3): gate on the fetch we fired at the top of this function. + // Pre-create probes (branch-conflict, PR probe, path resolution, sparse + // prep) already ran concurrently with the fetch; in the warm case this + // await is a no-op. In the cold case the spinner has already shown + // "Checking for updates..." so the user sees the wait is legible. + // + // `fetchRemoteWithCache` never rejects (log-and-proceed on offline + // failure), so the bare `await` does not need a try/catch here. + await fetchPromise + emitCreateWorktreeProgress(mainWindow, 'creating') await (sparseDirectories.length > 0 ? addSparseWorktree( diff --git a/src/main/ipc/worktrees-windows.test.ts b/src/main/ipc/worktrees-windows.test.ts index 0a92ba44e04..2ce4d4accb4 100644 --- a/src/main/ipc/worktrees-windows.test.ts +++ b/src/main/ipc/worktrees-windows.test.ts @@ -181,7 +181,14 @@ describe('registerWorktreeHandlers – Windows path handling', () => { ensurePathWithinWorkspaceMock.mockReturnValue('C:\\workspaces\\improve-dashboard') listWorktreesMock.mockResolvedValue([]) - registerWorktreeHandlers(mainWindow as never, store as never, {} as never) + // Why: createLocalWorktree routes `git fetch` through + // `runtime.fetchRemoteWithCache` (§3.3 Lifecycle). Stub it for path tests. + const runtimeStub = { + fetchRemoteWithCache: async () => { + /* noop */ + } + } + registerWorktreeHandlers(mainWindow as never, store as never, runtimeStub as never) }) it('accepts a newly created Windows worktree when git lists the same path with different separators', async () => { diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 1179fce02f1..12547caecde 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -248,7 +248,16 @@ describe('registerWorktreeHandlers', () => { ensurePathWithinWorkspaceMock.mockImplementation((targetPath: string) => targetPath) listWorktreesMock.mockResolvedValue([]) - registerWorktreeHandlers(mainWindow as never, store as never, {} as never) + // Why: createLocalWorktree routes `git fetch` through + // `runtime.fetchRemoteWithCache` (§3.3 Lifecycle). A minimal stub + // keeps these tests focused on create-flow semantics; the full + // cache behavior is covered by fetch-remote-cache.test.ts. + const runtimeStub = { + fetchRemoteWithCache: async () => { + /* noop — fetch mocked at gitExecFileAsync level via gitExecFileAsyncMock */ + } + } + registerWorktreeHandlers(mainWindow as never, store as never, runtimeStub as never) }) it('auto-suffixes the branch name when the first choice collides with a remote branch', async () => { diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 6789f441eb0..1cf22e79ef7 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -146,7 +146,7 @@ export function registerWorktreeHandlers( return createRemoteWorktree(args, repo, store, mainWindow) } - return createLocalWorktree(args, repo, store, mainWindow) + return createLocalWorktree(args, repo, store, mainWindow, runtime) } ) diff --git a/src/main/runtime/fetch-remote-cache.test.ts b/src/main/runtime/fetch-remote-cache.test.ts new file mode 100644 index 00000000000..4ae3a516db4 --- /dev/null +++ b/src/main/runtime/fetch-remote-cache.test.ts @@ -0,0 +1,106 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Why: these tests cover the §3.3 Lifecycle rules on +// `OrcaRuntimeService.fetchRemoteWithCache` — in particular that a rejected +// fetch evicts its Map entry AND does not advance the freshness timestamp, +// and that two concurrent callers serialize on a single underlying fetch. +// They live in a dedicated file so we can mock `gitExecFileAsync` cleanly +// without disturbing the large orca-runtime.test.ts mock surface. + +const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) + +vi.mock('../git/runner', async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + gitExecFileAsync: gitExecFileAsyncMock + } +}) + +// Why: orca-runtime.ts imports heavy modules (hooks, ipc/*, etc.) at top +// level. We only exercise the fetch cache, so we let those imports load +// normally — none of them trigger IO until a runtime method is called. +import { OrcaRuntimeService } from './orca-runtime' + +describe('OrcaRuntimeService.fetchRemoteWithCache', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('evicts the in-flight Map entry on rejection so the next caller re-fetches', async () => { + // First call rejects, second call resolves. Without §3.3 Lifecycle + // `.finally()` eviction, the second caller would await the rejected + // promise forever (or throw the same error) — the regression pattern + // described in §3.3. + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('network down')) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + const runtime = new OrcaRuntimeService(null) + + await runtime.fetchRemoteWithCache('/repo/a', 'origin') + await runtime.fetchRemoteWithCache('/repo/a', 'origin') + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not advance the freshness timestamp when the fetch rejects', async () => { + // A rejected fetch that wrote the timestamp would make the 30s freshness + // cache "lie" — the next caller would skip the fetch on a repo whose + // last real sync is unknown. §3.3 mandates success-only writes. + gitExecFileAsyncMock + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + const runtime = new OrcaRuntimeService(null) + + await runtime.fetchRemoteWithCache('/repo/b', 'origin') + // Immediately call again — if the freshness window were armed we would + // short-circuit and skip the fetch. It must still dispatch a real fetch. + await runtime.fetchRemoteWithCache('/repo/b', 'origin') + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('serializes two concurrent callers onto a single git fetch', async () => { + // Two callers hitting the same repo+remote at the same time must share + // one underlying fetch. Without the in-flight Map they would each + // dispatch an independent `git fetch`, tripling the network load in the + // worst case (renderer create + dispatch probe + CLI create). + let resolveFetch!: () => void + const pending = new Promise<{ stdout: string; stderr: string }>((resolve) => { + resolveFetch = () => resolve({ stdout: '', stderr: '' }) + }) + gitExecFileAsyncMock.mockReturnValueOnce(pending) + + const runtime = new OrcaRuntimeService(null) + + const first = runtime.fetchRemoteWithCache('/repo/c', 'origin') + const second = runtime.fetchRemoteWithCache('/repo/c', 'origin') + + // Allow both callers to register before we resolve. + await Promise.resolve() + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + + resolveFetch() + await Promise.all([first, second]) + + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('skips the fetch inside the 30s freshness window after a successful fetch', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + const runtime = new OrcaRuntimeService(null) + + await runtime.fetchRemoteWithCache('/repo/d', 'origin') + await runtime.fetchRemoteWithCache('/repo/d', 'origin') + + // Second call must short-circuit on the freshness window (no new exec). + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 744568a7a64..463b778b709 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -471,32 +471,44 @@ describe('OrcaRuntimeService', () => { it('delivers pending orchestration messages to an already-idle agent', async () => { vi.useFakeTimers() - const runtime = new OrcaRuntimeService(store) - const db = new OrchestrationDb(':memory:') - const write = vi.fn().mockReturnValue(true) - runtime.setOrchestrationDb(db) - runtime.setPtyController({ - write, - kill: vi.fn(), - getForegroundProcess: async () => null - }) - syncSinglePty(runtime) + try { + const runtime = new OrcaRuntimeService(store) + const db = new OrchestrationDb(':memory:') + const write = vi.fn().mockReturnValue(true) + runtime.setOrchestrationDb(db) + runtime.setPtyController({ + write, + kill: vi.fn(), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime) - const [terminal] = (await runtime.listTerminals()).terminals - runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) - runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) - db.insertMessage({ from: 'term_sender', to: terminal.handle, subject: 'hello' }) + const [terminal] = (await runtime.listTerminals()).terminals + runtime.onPtyData('pty-1', '\x1b]0;Codex working\x07', 100) + runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) + db.insertMessage({ from: 'term_sender', to: terminal.handle, subject: 'hello' }) - runtime.deliverPendingMessagesForHandle(terminal.handle) + runtime.deliverPendingMessagesForHandle(terminal.handle) - expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: hello')) - // Why: markAsRead is deferred until the 500ms delayed Enter is confirmed, - // so we must advance timers past the split-write delay. - await vi.advanceTimersByTimeAsync(500) - expect(write).toHaveBeenCalledWith('pty-1', '\r') - expect(db.getUnreadMessages(terminal.handle)).toHaveLength(0) - db.close() - vi.useRealTimers() + expect(write).toHaveBeenCalledWith('pty-1', expect.stringContaining('Subject: hello')) + // Why: the split Enter write lands after the 500ms delay, so we advance + // past it before asserting on delivered_at. + await vi.advanceTimersByTimeAsync(500) + expect(write).toHaveBeenCalledWith('pty-1', '\r') + + // Why: design doc §3.2 splits delivered vs. read — push-on-idle stamps + // `delivered_at` but must *not* flip `read`, since only the check caller + // (the agent) is authorized to consume messages from its queue. The + // injected banner is a courtesy; the rows stay unread so the agent can + // still observe them via `check` and resolve the consumption race. + const unread = db.getUnreadMessages(terminal.handle) + expect(unread).toHaveLength(1) + expect(unread[0].read).toBe(0) + expect(unread[0].delivered_at).not.toBeNull() + db.close() + } finally { + vi.useRealTimers() + } }) it('adopts preallocated ORCA_TERMINAL_HANDLE as a valid runtime handle', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 3ba6281df65..3d2492bf173 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -7,7 +7,7 @@ import { isShellProcess } from '../../shared/agent-detection' import type { AgentStatus } from '../../shared/agent-detection' -import { gitExecFileAsync, gitExecFileSync } from '../git/runner' +import { gitExecFileAsync } from '../git/runner' import { isWslPath, parseWslPath, getWslHome } from '../wsl' import { randomUUID } from 'crypto' import { join } from 'path' @@ -84,7 +84,9 @@ import { getBranchConflictKind, isGitRepo, getRepoName, - searchBaseRefs + searchBaseRefs, + getRemoteDrift, + getRecentDriftSubjects } from '../git/repo' import { listWorktrees, addWorktree, removeWorktree } from '../git/worktree' import { createSetupRunnerScript, getEffectiveHooks, runHook } from '../hooks' @@ -242,6 +244,23 @@ export class OrcaRuntimeService { private agentDetector: AgentDetector | null = null private _orchestrationDb: OrchestrationDb | null = null private messageWaitersByHandle = new Map>() + // Why (§3.3 + §7.1): the renderer-create path and coordinator + // `probeWorktreeDrift` share this cache so a create that already fetched + // `origin` within the last 30s does not re-fetch during dispatch, and + // vice-versa. Keyed by `::` so multi-remote repos (even + // though v1 only uses `origin`) don't cross-contaminate. The in-flight Map + // also provides serialization — two concurrent callers share a single + // underlying `git fetch`. Lifecycle rules are enforced in + // `fetchRemoteWithCache` and MUST NOT be duplicated elsewhere: + // - entry inserted BEFORE await, + // - `.finally()` removes the entry on BOTH success and rejection, + // - timestamp written ONLY on success (rejection must not make the + // 30s freshness cache lie). + // A literal "insert before await / read-back after await" without these + // three rules wedges all future creates on the same repo after a single + // DNS hiccup until process restart (see §3.3 Lifecycle). + private fetchInflight = new Map>() + private fetchLastCompletedAt = new Map() private readonly getLocalProviderFn: (() => IPtyProvider) | null constructor( @@ -1022,10 +1041,18 @@ export class OrcaRuntimeService { } const remote = baseBranch.includes('/') ? baseBranch.split('/')[0] : 'origin' + // Why (§3.3 Lifecycle): route through the shared fetch cache so back-to-back + // CLI creates on the same repo don't each pay the round-trip, and so a + // subsequent dispatch probe within the 30s window reuses this result. The + // helper swallows rejection (log-and-proceed) so a DNS hiccup never wedges + // future creates and CLI creation stays usable offline — same intent as + // the previous try/catch around gitExecFileSync. try { - gitExecFileSync(['fetch', remote], { cwd: repo.path }) + await this.fetchRemoteWithCache(repo.path, remote) } catch { - // Why: matching the editor behavior keeps CLI creation usable offline. + // Why: belt-and-suspenders. fetchRemoteWithCache already logs and does + // not throw; the outer try/catch guarantees create-path tolerance even + // if future refactors change that contract. } await addWorktree( @@ -1105,6 +1132,96 @@ export class OrcaRuntimeService { } } + /** + * Fetch `remote` in `repoPath`, sharing the 30s freshness window + in-flight + * serialization with all other callers (renderer-create path, CLI create, + * dispatch drift probe). Never rejects — callers log-and-proceed on offline + * failures (§3.3 Lifecycle). + * + * Why a shared cache on the runtime instead of module-scoped: §7.1 relies on + * one cache for BOTH the renderer create path and `probeWorktreeDrift`. A + * dispatch tick that reuses a just-completed create-path fetch is the + * primary telemetry target; splitting the cache by call-site would double + * the fetch load on warm repos. + */ + async fetchRemoteWithCache(repoPath: string, remote: string): Promise { + const key = `${repoPath}::${remote}` + const lastAt = this.fetchLastCompletedAt.get(key) + if (lastAt !== undefined && Date.now() - lastAt < FETCH_FRESHNESS_MS) { + // Why: freshness window hit — skip the fetch entirely. Do NOT reuse any + // in-flight promise here; the timestamp is only written on success, so + // hitting this branch means a previous fetch did succeed recently. + return + } + + const existing = this.fetchInflight.get(key) + if (existing) { + // Why: genuine serialization (not check-then-set). Two callers racing + // on the same repo+remote share the single underlying `git fetch`. + return existing + } + + const promise = gitExecFileAsync(['fetch', remote], { cwd: repoPath }) + .then(() => { + // Why (§3.3 Lifecycle): timestamp on success ONLY. Writing on rejection + // would make the freshness cache lie about the last known remote state. + this.fetchLastCompletedAt.set(key, Date.now()) + }) + .catch((err) => { + // Why: swallow here so awaiters don't throw at the await site. Outer + // create/dispatch paths are already tolerant of offline fetch failure; + // this is the behavioral contract of this helper. + console.warn(`[fetchRemoteWithCache] ${remote} fetch failed for ${repoPath}:`, err) + }) + .finally(() => { + // Why (§3.3 Lifecycle): evict on BOTH success and rejection. A + // rejected entry that survived in the Map would wedge every future + // create on this repo until Orca restarted (the F2 bug §3.3 pins). + this.fetchInflight.delete(key) + }) + + this.fetchInflight.set(key, promise) + return promise + } + + /** + * Probe how far the worktree's HEAD is behind its tracking remote. Returns + * null when the probe cannot establish a signal (no default base ref, or + * git failure). Dispatch treats null as "unknown — proceed" (§3.1); only + * knowing-and-stale refuses. + */ + async probeWorktreeDrift(worktreeSelector: string): Promise<{ + base: string + behind: number + recentSubjects: string[] + } | null> { + const wt = await this.resolveWorktreeSelector(worktreeSelector) + if (!this.store) { + return null + } + const repo = this.store.getRepos().find((r) => r.id === wt.repoId) + if (!repo) { + return null + } + const base = getDefaultBaseRef(repo.path) + if (!base) { + // Why: brand-new repo with no remote primary — nothing to compare + // against, so there's no meaningful drift to report. Dispatch should + // not block on a probe that cannot form an opinion. + return null + } + const remote = base.includes('/') ? base.split('/')[0] : 'origin' + // Why: fetch failures are non-fatal; we proceed with whatever the + // last-known remote ref points at. `fetchRemoteWithCache` never throws. + await this.fetchRemoteWithCache(wt.path, remote) + const drift = getRemoteDrift(wt.path, 'HEAD', base) + if (!drift) { + return null + } + const recentSubjects = getRecentDriftSubjects(wt.path, 'HEAD', base, DRIFT_PROBE_SUBJECT_LIMIT) + return { base, behind: drift.behind, recentSubjects } + } + async updateManagedWorktreeMeta( worktreeSelector: string, updates: { @@ -1733,7 +1850,7 @@ export class OrcaRuntimeService { waitForMessage( handle: string, - options?: { typeFilter?: string[]; timeoutMs?: number } + options?: { typeFilter?: string[]; timeoutMs?: number; signal?: AbortSignal } ): Promise { return new Promise((resolve) => { const timeoutMs = options?.timeoutMs ?? MESSAGE_WAIT_DEFAULT_TIMEOUT_MS @@ -1745,7 +1862,27 @@ export class OrcaRuntimeService { timeout: null } + // Why: if the caller aborts (socket closed on the RPC side — see design + // doc §3.1 counter-lifecycle), resolve immediately so the long-poll slot + // is released instead of counting down the full timeoutMs with a dead + // client on the other end. + const signal = options?.signal + const onAbort = (): void => { + this.removeMessageWaiter(waiter) + resolve() + } + if (signal) { + if (signal.aborted) { + resolve() + return + } + signal.addEventListener('abort', onAbort, { once: true }) + } + waiter.timeout = setTimeout(() => { + if (signal) { + signal.removeEventListener('abort', onAbort) + } this.removeMessageWaiter(waiter) resolve() }, timeoutMs) @@ -2002,8 +2139,15 @@ export class OrcaRuntimeService { // Why: Claude Code treats large single PTY writes as paste events and // swallows a \r included in the same write. Send Enter separately after - // a delay so the agent processes the pasted message first. Mark messages - // as read only after \r is confirmed, so failed deliveries stay queued. + // a delay so the agent processes the pasted message first. Stamp + // `delivered_at` only after \r is confirmed, so failed deliveries stay + // queued. + // + // Important (design doc §3.2, feedback #2): we stamp `delivered_at` here + // instead of flipping `read`. `read` is reserved for "a check-caller + // consumed this message." Flipping `read` on push-on-idle would hide the + // message from the coordinator's next `check --unread`, which is the + // exact bug feedback #2 reported. The two bits must stay independent. const ptyId = leaf.ptyId setTimeout(() => { try { @@ -2012,11 +2156,12 @@ export class OrcaRuntimeService { } const submitted = this.ptyController?.write(ptyId, '\r') ?? false if (submitted) { - this._orchestrationDb?.markAsRead(unread.map((m) => m.id)) + this._orchestrationDb?.markAsDelivered(unread.map((m) => m.id)) } } catch { - // Terminal may have closed during the delay — messages stay unread - // and will be re-delivered on the next idle transition. + // Terminal may have closed during the delay — messages stay queued + // (delivered_at still NULL) and will be re-delivered on the next + // idle transition. } }, 500) } @@ -3133,6 +3278,13 @@ const DEFAULT_TERMINAL_LIST_LIMIT = 200 const DEFAULT_WORKTREE_LIST_LIMIT = 200 const DEFAULT_WORKTREE_PS_LIMIT = 200 const RESOLVED_WORKTREE_CACHE_TTL_MS = 1000 +// Why (§3.3): 30s freshness window. A second worktree-create or dispatch-probe +// against the same repo+remote within this window reuses the previous successful +// fetch instead of repeating the round-trip. Chosen so rapid "new worktree" +// clicks and successive coordinator dispatches feel snappy, while still being +// short enough that a genuinely-changed remote is observed on the next action. +const FETCH_FRESHNESS_MS = 30_000 +const DRIFT_PROBE_SUBJECT_LIMIT = 5 function buildPreview(lines: string[], partialLine: string): string { const previewLines = buildTailLines(lines, partialLine) .map((line) => line.trim()) diff --git a/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap b/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap new file mode 100644 index 00000000000..8c8fd9f09f5 --- /dev/null +++ b/src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap @@ -0,0 +1,83 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`buildDispatchPreamble > renders a stable snapshot of the full preamble 1`] = ` +"You are working inside Orca, a multi-agent IDE. You are a dispatched worker. +Your coordinator's terminal handle is: term_COORD +Your task ID is: task_SNAP + +You talk to the coordinator only through the CLI commands below. Do not use +Slack, GitHub comments, or any other channel to reach a human during the run. + +=== CLI COMMANDS === + + # Report task completion (REQUIRED when done — even on failure). + # + # RULE: --body must be a 3-sentence executive summary (what you did, + # what you found, what's left). Never send an empty body; the coordinator + # reads the body first and only opens artifacts if it needs more detail. + # If you produced a long-form artifact, include its path as + # payload.reportPath so the coordinator can find it without a file search. + # + # RULE: send worker_done exactly once. Failure is still a worker_done + # with subject like "Failed: " — never silently exit. + orca orchestration send --to term_COORD \\ + --type worker_done --subject "" \\ + --body "<3-sentence summary: what you did, what you found, what's left>" \\ + --payload '{"taskId":"task_SNAP","filesModified":["path/a","path/b"],"reportPath":""}' + + # BEHAVIOR RULE: send a heartbeat every 5 minutes + # while actively working on the task. The coordinator uses this to + # distinguish "still thinking" from "hung / crashed." Skip heartbeats only + # while blocked inside \`check --wait\` or \`ask\` — those calls are + # themselves liveness signals. + # + # Include BOTH taskId and dispatchId in the payload: the coordinator + # attributes the heartbeat to the specific dispatch context, not just + # the task, so a straggler heartbeat from a previously-failed dispatch + # cannot mask a hung retry. + orca orchestration send --to term_COORD \\ + --type heartbeat --subject "alive" \\ + --payload '{"taskId":"task_SNAP","dispatchId":"ctx_SNAP","phase":""}' + + # Ask the coordinator a question and block until it answers. + # + # BEHAVIOR RULE #1 (MUST NOT VIOLATE): + # NEVER use AskUserQuestion; use \`orca orchestration ask\` or send + # --type decision_gate. AskUserQuestion opens a local TUI prompt that the + # coordinator cannot see and cannot answer — your session will hang forever + # waiting on a human. Every interactive question goes through \`ask\` below. + # + # The \`ask\` verb is a thin wrapper: it sends a decision_gate message and + # blocks on \`check --wait\` until the coordinator replies, then prints the + # reply body. Use it anywhere you would otherwise have reached for + # AskUserQuestion. + orca orchestration ask --to term_COORD \\ + --question "" \\ + --options "" \\ + --timeout-ms 600000 + + # Escalate a blocker or failure (pre-completion, when you need the + # coordinator to do something before you can continue): + orca orchestration send --to term_COORD \\ + --type escalation --subject "Blocked: " \\ + --body "
" \\ + --payload '{"taskId":"task_SNAP"}' + + # Check for messages from the coordinator: + orca orchestration check + +=== AFTER YOU SEND worker_done === + +Keep the shell session open for a grace period (10 minutes) in case the +coordinator sends a follow-up or re-dispatches you. Poll with +\`orca orchestration check\` every 2 minutes during that window. If no +follow-up arrives, you may exit after the grace period — the coordinator +will not expect further output from you. + +If the coordinator re-dispatches you (you will receive a fresh preamble + +TASK block), reset your polling and start the new task. Do not respond +to the previous task's follow-ups after a re-dispatch. + +=== TASK === +TASK_BODY" +`; diff --git a/src/main/runtime/orchestration/coordinator.test.ts b/src/main/runtime/orchestration/coordinator.test.ts index ac0ebbb4974..5c1ff4e22f7 100644 --- a/src/main/runtime/orchestration/coordinator.test.ts +++ b/src/main/runtime/orchestration/coordinator.test.ts @@ -1,12 +1,27 @@ /* eslint-disable max-lines -- Why: coordinator tests cover dispatch, DAG ordering, escalation, decision gates, concurrency, and stop — splitting by category would scatter shared setup without improving clarity. */ import { afterEach, describe, expect, it } from 'vitest' import { OrchestrationDb } from './db' -import { Coordinator, type CoordinatorRuntime } from './coordinator' +import { + Coordinator, + DISPATCH_STALE_THRESHOLD, + parseAllowStaleBaseFromSpec, + type CoordinatorRuntime +} from './coordinator' + +type DriftResult = { + base: string + behind: number + recentSubjects: string[] +} | null function createMockRuntime(): CoordinatorRuntime & { sentMessages: { handle: string; text: string }[] terminals: { handle: string; worktreeId: string; connected: boolean; writable: boolean }[] createdTerminals: string[] + probeDriftCalls: string[] + probeDriftResult: DriftResult + setProbeDrift(result: DriftResult): void + throwProbeDrift: Error | null } { const mock = { sentMessages: [] as { handle: string; text: string }[], @@ -17,6 +32,12 @@ function createMockRuntime(): CoordinatorRuntime & { writable: boolean }[], createdTerminals: [] as string[], + probeDriftCalls: [] as string[], + probeDriftResult: null as DriftResult, + throwProbeDrift: null as Error | null, + setProbeDrift(result: DriftResult): void { + mock.probeDriftResult = result + }, async sendTerminal(handle: string, action: { text?: string }) { mock.sentMessages.push({ handle, text: action.text ?? '' }) return { handle, accepted: true, bytesWritten: 0 } @@ -32,6 +53,13 @@ function createMockRuntime(): CoordinatorRuntime & { }, async waitForTerminal(handle: string) { return { handle, condition: 'exit' } + }, + async probeWorktreeDrift(worktreeSelector: string): Promise { + mock.probeDriftCalls.push(worktreeSelector) + if (mock.throwProbeDrift) { + throw mock.throwProbeDrift + } + return mock.probeDriftResult } } return mock @@ -355,6 +383,88 @@ describe('Coordinator', () => { expect(result.status).toBe('completed') }) + it('logs a stale warning for dispatched rows past the threshold and does not auto-fail', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + // No terminals available so dispatchReadyTasks creates one and we can + // drive the stale-scan deterministically via SQL backdating. + const task = db.createTask({ spec: 'work' }) + const ctx = db.createDispatchContext(task.id, 'term_stale') + + // Backdate dispatched_at and last_heartbeat_at beyond the 10-min threshold + // so getStaleDispatches returns this row on the first tick. + const sqlite = ( + db as unknown as { db: { prepare: (s: string) => { run: (...a: unknown[]) => void } } } + ).db + const iso = (ms: number) => new Date(Date.now() - ms).toISOString() + sqlite + .prepare('UPDATE dispatch_contexts SET dispatched_at = ?, last_heartbeat_at = ? WHERE id = ?') + .run(iso(60 * 60 * 1000), iso(30 * 60 * 1000), ctx.id) + + const logs: string[] = [] + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 20, + onLog: (m) => logs.push(m) + }) + + // Drive one tick then stop — we only need the stale warning to have fired. + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 80) + }) + coordinator.stop() + await runPromise + + expect(logs.some((l) => /has not sent a heartbeat/.test(l) && l.includes(task.id))).toBe(true) + // Task status must NOT have been auto-failed — logging only. + expect(db.getTask(task.id)?.status).toBe('dispatched') + }) + + it('records heartbeat by dispatchId on worker heartbeat messages', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + + const task = db.createTask({ spec: 'work' }) + const ctx = db.createDispatchContext(task.id, 'term_a') + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 20 + }) + + const runPromise = coordinator.run() + + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'alive', + type: 'heartbeat', + payload: JSON.stringify({ taskId: task.id, dispatchId: ctx.id, phase: 'implementing' }) + }) + + await new Promise((r) => { + setTimeout(r, 80) + }) + + expect(db.getDispatchContext(task.id)?.last_heartbeat_at).toBeTruthy() + + // Complete the task so the coordinator run finishes cleanly. + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id }) + }) + + const result = await runPromise + expect(result.status).toBe('completed') + }) + it('can be stopped', async () => { db = new OrchestrationDb(':memory:') const runtime = createMockRuntime() @@ -376,4 +486,287 @@ describe('Coordinator', () => { const result = await runPromise expect(result.status).toBe('failed') }) + + describe('stale-base dispatch guard', () => { + it('threads drift into the preamble when behind > 0 and under threshold', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + runtime.setProbeDrift({ + base: 'origin/main', + behind: 5, + recentSubjects: ['fix A', 'fix B', 'fix C'] + }) + + const task = db.createTask({ spec: 'do the work' }) + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 50, + worktree: 'wt1' + }) + + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 100) + }) + + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id }) + }) + + const result = await runPromise + expect(result.status).toBe('completed') + expect(runtime.probeDriftCalls).toContain('wt1') + const sent = runtime.sentMessages.find((m) => m.handle === 'term_a') + expect(sent).toBeDefined() + expect(sent!.text).toContain('--- BASE DRIFT ---') + expect(sent!.text).toContain('5 commits behind origin/main') + expect(sent!.text).toContain('fix A') + }) + + it('silently skips dispatch when drift > threshold and allow-stale-base is absent', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + runtime.setProbeDrift({ + base: 'origin/main', + behind: DISPATCH_STALE_THRESHOLD + 10, + recentSubjects: ['fix A'] + }) + + const task = db.createTask({ spec: 'do the work' }) + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 50, + worktree: 'wt1' + }) + + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 250) + }) + coordinator.stop() + const result = await runPromise + + // Why: silent-skip must NOT burn the circuit-breaker budget. Task must + // stay in `ready`; failDispatch must NOT be called; sendTerminal must + // NOT be called; no dispatch context should exist. + expect(runtime.sentMessages).toHaveLength(0) + expect(db.getTask(task.id)?.status).toBe('ready') + expect(db.getDispatchContext(task.id)).toBeUndefined() + // Coordinator was stopped externally, so overall status is 'failed' + // because tasks are not complete — but the task itself never dispatched. + expect(result.status).toBe('failed') + expect(result.failedTasks).not.toContain(task.id) + }) + + it('proceeds with stripped spec + drift section when allow-stale-base overrides', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + runtime.setProbeDrift({ + base: 'origin/main', + behind: 200, + recentSubjects: ['commit 1', 'commit 2'] + }) + + const spec = `Investigate issue #42 +allow-stale-base: true` + const task = db.createTask({ spec }) + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 50, + worktree: 'wt1' + }) + + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 100) + }) + + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id }) + }) + + const result = await runPromise + expect(result.status).toBe('completed') + const sent = runtime.sentMessages.find((m) => m.handle === 'term_a') + expect(sent).toBeDefined() + expect(sent!.text).toContain('--- BASE DRIFT ---') + expect(sent!.text).toContain('200 commits behind origin/main') + // Why (§3.4): stripped spec must not contain the infra flag line. + expect(sent!.text).toContain('Investigate issue #42') + expect(sent!.text).not.toContain('allow-stale-base: true') + }) + + it('proceeds without drift section when probeWorktreeDrift returns null', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + runtime.setProbeDrift(null) + + const task = db.createTask({ spec: 'do the work' }) + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 50, + worktree: 'wt1' + }) + + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 100) + }) + + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id }) + }) + + const result = await runPromise + expect(result.status).toBe('completed') + const sent = runtime.sentMessages.find((m) => m.handle === 'term_a') + expect(sent).toBeDefined() + expect(sent!.text).not.toContain('--- BASE DRIFT ---') + }) + + it('does not call probeWorktreeDrift when coordinator has no worktree selector', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + const logs: string[] = [] + + const task = db.createTask({ spec: 'do the work' }) + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 50, + // worktree deliberately omitted + onLog: (msg) => logs.push(msg) + }) + + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 100) + }) + + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id }) + }) + + const result = await runPromise + expect(result.status).toBe('completed') + expect(runtime.probeDriftCalls).toHaveLength(0) + expect(logs.some((m) => m.includes('stale-base guard inert'))).toBe(true) + // Dispatch still went through normally. + expect(runtime.sentMessages.length).toBeGreaterThan(0) + }) + + it('proceeds without drift when probeWorktreeDrift throws', async () => { + db = new OrchestrationDb(':memory:') + const runtime = createMockRuntime() + runtime.terminals = [{ handle: 'term_a', worktreeId: 'wt1', connected: true, writable: true }] + runtime.throwProbeDrift = new Error('boom') + + const task = db.createTask({ spec: 'do the work' }) + + const coordinator = new Coordinator(db, runtime, { + spec: 'go', + coordinatorHandle: 'coord', + pollIntervalMs: 50, + worktree: 'wt1' + }) + + const runPromise = coordinator.run() + await new Promise((r) => { + setTimeout(r, 100) + }) + + db.insertMessage({ + from: 'term_a', + to: 'coord', + subject: 'Done', + type: 'worker_done', + payload: JSON.stringify({ taskId: task.id }) + }) + + const result = await runPromise + expect(result.status).toBe('completed') + const sent = runtime.sentMessages.find((m) => m.handle === 'term_a') + expect(sent!.text).not.toContain('--- BASE DRIFT ---') + }) + }) +}) + +describe('parseAllowStaleBaseFromSpec', () => { + it('matches canonical form on its own line and strips it', () => { + const spec = `Do the work +allow-stale-base: true` + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(spec) + expect(allowStale).toBe(true) + expect(strippedSpec).toBe('Do the work\n') + expect(strippedSpec).not.toContain('allow-stale-base') + }) + + it('matches case-insensitively', () => { + const spec = `Do the work +Allow-Stale-Base: TRUE` + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(spec) + expect(allowStale).toBe(true) + expect(strippedSpec).not.toMatch(/[Aa]llow-[Ss]tale-[Bb]ase/) + }) + + it('does not match allow-stale-base: false', () => { + const spec = `Do the work +allow-stale-base: false` + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(spec) + expect(allowStale).toBe(false) + expect(strippedSpec).toBe(spec) + }) + + it('does not match allow-stale-base: truthy', () => { + const spec = `Do the work +allow-stale-base: truthy` + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(spec) + expect(allowStale).toBe(false) + expect(strippedSpec).toBe(spec) + }) + + it('does not match the flag embedded inside a sentence', () => { + const spec = 'we allow-stale-base: true sometimes' + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(spec) + expect(allowStale).toBe(false) + expect(strippedSpec).toBe(spec) + }) + + it('handles the flag as the last line with no trailing newline', () => { + const spec = 'line 1\nallow-stale-base: true' + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(spec) + expect(allowStale).toBe(true) + expect(strippedSpec).toBe('line 1\n') + expect(strippedSpec.endsWith('allow-stale-base: true')).toBe(false) + }) }) diff --git a/src/main/runtime/orchestration/coordinator.ts b/src/main/runtime/orchestration/coordinator.ts index 541102f4ae5..2dc02867887 100644 --- a/src/main/runtime/orchestration/coordinator.ts +++ b/src/main/runtime/orchestration/coordinator.ts @@ -19,6 +19,45 @@ export type CoordinatorRuntime = { handle: string, options?: { condition?: string; timeoutMs?: number } ): Promise<{ handle: string; condition: string }> + // Why (§3.1): dispatch pre-flight drift check lives on the runtime because + // it needs to resolve a worktree selector, load the repo, and fetch. The + // coordinator only knows about handles + specs; resolving a git worktree + // from this layer would leak transport details here. + probeWorktreeDrift(worktreeSelector: string): Promise<{ + base: string + behind: number + recentSubjects: string[] + } | null> +} + +// Why (§3.1): single threshold, no warn/refuse split. Coordinator picked 20 +// in msg_eff3a646110d — lets normal day-of-velocity on active monorepos pass +// while still tripping on the 168-commit harm observed in ORCHESTRATOR_FEEDBACK.md. +export const DISPATCH_STALE_THRESHOLD = 20 + +// Why (§3.4): the flag is stashed in the task spec text rather than a DB +// column in v1. The regex is intentionally narrow — only the canonical form +// matches, so typos fail closed (dispatch refuses). Returning the stripped +// spec alongside the boolean keeps this infra line out of the worker's +// `--- TASK ---` block (workers would otherwise read it as an instruction). +// +// Trade-off (§7.9): the regex matches any line of the spec including lines +// inside fenced code blocks. Acceptable v1 limitation — the failure mode is +// "dispatches through when the author didn't intend to," which the preamble +// drift section surfaces to the worker. Skill doc directs authors to place +// the flag as the last line and avoid the literal flag in code examples. +const ALLOW_STALE_BASE_RE = /^[ \t]*allow-stale-base:[ \t]*true[ \t]*\r?$/im +const ALLOW_STALE_BASE_STRIP_RE = /^[ \t]*allow-stale-base:[ \t]*true[ \t]*\r?\n?/im + +export function parseAllowStaleBaseFromSpec(spec: string): { + allowStale: boolean + strippedSpec: string +} { + if (!ALLOW_STALE_BASE_RE.test(spec)) { + return { allowStale: false, strippedSpec: spec } + } + const strippedSpec = spec.replace(ALLOW_STALE_BASE_STRIP_RE, '') + return { allowStale: true, strippedSpec } } export type CoordinatorOptions = { @@ -41,6 +80,13 @@ type CoordinatorState = { const DEFAULT_POLL_MS = 2000 const MAX_CONCURRENT_DEFAULT = 4 +// Why: 10 min matches the preamble's documented heartbeat cadence (5 min) × +// 2, so a single missed heartbeat is the earliest a dispatch can look stale. +// Keeping this in one place (not a per-call arg) ensures the preamble copy +// and the detector logic stay aligned; moving it to a config would multiply +// the places this constant must be kept in sync. +const HUNG_THRESHOLD_MS = 10 * 60 * 1000 + export class Coordinator { private db: OrchestrationDb private runtime: CoordinatorRuntime @@ -173,10 +219,27 @@ export class Coordinator { this.processMessages() this.processEscalations() this.processDecisionGates() + this.warnStaleDispatches() await this.dispatchReadyTasks() return this.checkConvergence() } + // Why: emit a single warning per stale dispatch per tick. This intentionally + // does NOT auto-fail the dispatch — the false-positive cost (a slow worker + // producing correct output) is higher than the false-negative cost (a hung + // worker keeps its terminal slot until a human notices). Auto-fail policy + // is a separate decision documented in R6 of DESIGN_DOC_PREAMBLE_FIX.md. + private warnStaleDispatches(): void { + const thresholdIso = new Date(Date.now() - HUNG_THRESHOLD_MS).toISOString() + const stale = this.db.getStaleDispatches(thresholdIso) + for (const ctx of stale) { + const minutes = Math.round(HUNG_THRESHOLD_MS / 60000) + this.opts.onLog( + `Warning: worker ${ctx.assignee_handle ?? ''} on task ${ctx.task_id} has not sent a heartbeat in ~${minutes} min (dispatch ${ctx.id})` + ) + } + } + private processMessages(): void { const messages = this.db.getUnreadMessages(this.opts.coordinatorHandle) if (messages.length === 0) { @@ -194,6 +257,9 @@ export class Coordinator { case 'decision_gate': this.handleDecisionGateMessage(msg) break + case 'heartbeat': + this.handleHeartbeat(msg) + break case 'status': this.opts.onLog(`Status from ${msg.from_handle}: ${msg.subject}`) break @@ -205,6 +271,35 @@ export class Coordinator { this.db.markAsRead(messages.map((m) => m.id)) } + // Why: attribute heartbeats to the specific dispatchId, not a + // (taskId, from_handle) lookup. A task that gets retried after a failed + // dispatch has multiple rows in dispatch_contexts — a late heartbeat from + // the previous (failed) assignee arriving while the new dispatch is active + // would falsely bump the new row's last_heartbeat_at if we resolved by + // "latest dispatch for this task" (§5.3.4). If the worker drops dispatchId + // from the payload, log-and-skip is the preferred failure mode: the stale + // detector will correctly flag the dispatch as hung because nothing + // refreshed last_heartbeat_at. + private handleHeartbeat(msg: MessageRow): void { + if (!msg.payload) { + this.opts.onLog(`Heartbeat from ${msg.from_handle} missing payload; ignored`) + return + } + let payload: { dispatchId?: unknown } = {} + try { + payload = JSON.parse(msg.payload) + } catch { + this.opts.onLog(`Heartbeat from ${msg.from_handle} has invalid JSON payload; ignored`) + return + } + const dispatchId = payload.dispatchId + if (typeof dispatchId !== 'string' || dispatchId.length === 0) { + this.opts.onLog(`Heartbeat from ${msg.from_handle} missing dispatchId; ignored`) + return + } + this.db.recordHeartbeat(dispatchId, msg.created_at) + } + private handleWorkerDone(msg: MessageRow): void { this.opts.onLog(`Worker done: ${msg.from_handle} — ${msg.subject}`) @@ -383,15 +478,69 @@ export class Coordinator { } private async dispatchTask(task: TaskRow, targetHandle: string): Promise { + // Why (§3.1): pre-flight drift check BEFORE `createDispatchContext` so a + // refusal does NOT increment failure_count. createDispatchContext carries + // `MAX(failure_count)` forward across contexts (db.ts:301-306), so burning + // the circuit-breaker budget here would convert a recoverable "fetch and + // retry" into a hard `failed` task within ~6s of polling. Silent return + // leaves the task in `ready`; the next `dispatchReadyTasks` tick retries + // naturally, and once the coordinator's worktree has been refreshed + // dispatch proceeds cleanly. + const { allowStale, strippedSpec } = parseAllowStaleBaseFromSpec(task.spec) + let baseDrift: { + base: string + behind: number + recentSubjects: string[] + } | null = null + + if (!this.opts.worktree) { + // Why (§7.4): CoordinatorOptions.worktree is optional. When undefined, + // probeWorktreeDrift cannot resolve a selector; log once so operators + // can see the guard did not run for this task and proceed. v2 may + // always resolve a worktree via the coordinator-terminal handle. + this.opts.onLog(`stale-base guard inert for ${task.id}: coordinator has no worktree selector`) + } else { + baseDrift = await this.runtime.probeWorktreeDrift(this.opts.worktree).catch((err) => { + this.opts.onLog(`probeWorktreeDrift failed for ${this.opts.worktree}: ${err}`) + return null + }) + + if (baseDrift && baseDrift.behind > DISPATCH_STALE_THRESHOLD && !allowStale) { + // Why (§3.1): silent-return, NOT failDispatch (which would burn the + // circuit-breaker budget). The message lists three remediations so + // the operator can recover via any of them. + this.opts.onLog( + `Skipping dispatch of ${task.id}: worktree is ${baseDrift.behind} commits ` + + `behind ${baseDrift.base}. Pull/rebase the worktree, recreate it with ` + + `--base-branch ${baseDrift.base}, or include 'allow-stale-base: true' ` + + `in the task spec to override. Task remains in 'ready'; coordinator ` + + `will retry on the next tick.` + ) + return + } + } + const dispatch = this.db.createDispatchContext(task.id, targetHandle) // Why: agents dispatched by the coordinator must use orca-dev in dev mode // so they talk to the dev runtime's socket, not production (Section 6.4). + // Why (§3.4): `strippedSpec` drops the `allow-stale-base: true` line so + // the worker's `--- TASK ---` block does not contain the infra flag (which + // the worker would otherwise read as part of its instructions). const preamble = buildDispatchPreamble({ taskId: task.id, - taskSpec: task.spec, + dispatchId: dispatch.id, + // Why (§3.4, stale-base PR): use `strippedSpec` not `task.spec` so the + // `allow-stale-base: true` line isn't rendered into the worker's + // --- TASK --- block (worker would otherwise treat the infra flag as + // part of its instructions). + taskSpec: strippedSpec, coordinatorHandle: this.opts.coordinatorHandle, - devMode: process.env.ORCA_USER_DATA_PATH?.includes('orca-dev') + devMode: process.env.ORCA_USER_DATA_PATH?.includes('orca-dev'), + // Why (§3.2): drift section fires only when behind > 0. The preamble + // builder gates on this itself; passing the object unconditionally lets + // the coordinator stay dumb about the display rule. + ...(baseDrift ? { baseDrift } : {}) }) // Why: check if the task was previously blocked by a decision gate that diff --git a/src/main/runtime/orchestration/db.test.ts b/src/main/runtime/orchestration/db.test.ts index d412255fdae..9e9d5d941b3 100644 --- a/src/main/runtime/orchestration/db.test.ts +++ b/src/main/runtime/orchestration/db.test.ts @@ -1,4 +1,8 @@ /* eslint-disable max-lines -- Why: DB tests cover messages, tasks, dispatch contexts, decision gates, coordinator runs, and lifecycle in one suite to share the createDb() helper and afterEach cleanup. */ +import Database from 'better-sqlite3' +import { mkdtempSync, rmSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' import { afterEach, describe, expect, it } from 'vitest' import { OrchestrationDb } from './db' import type { MessageType } from './db' @@ -198,6 +202,36 @@ describe('OrchestrationDb', () => { expect(d.listTasks()).toHaveLength(2) }) + it('listTasksWithDispatch joins active dispatch metadata', () => { + const d = createDb() + const ready = d.createTask({ spec: 'ready task' }) + const dispatched = d.createTask({ spec: 'active task' }) + const ctx = d.createDispatchContext(dispatched.id, 'term_worker') + + const rows = d.listTasksWithDispatch() + const readyRow = rows.find((r) => r.id === ready.id) + const dispatchedRow = rows.find((r) => r.id === dispatched.id) + + expect(readyRow?.assignee_handle).toBeNull() + expect(readyRow?.dispatch_id).toBeNull() + expect(dispatchedRow?.assignee_handle).toBe('term_worker') + expect(dispatchedRow?.dispatch_id).toBe(ctx.id) + }) + + it('listTasksWithDispatch does not surface completed dispatches', () => { + const d = createDb() + const task = d.createTask({ spec: 'work' }) + d.createDispatchContext(task.id, 'term_worker') + d.updateTaskStatus(task.id, 'completed') + + const rows = d.listTasksWithDispatch() + const row = rows.find((r) => r.id === task.id) + // Task is completed — its dispatch is terminal and should not appear as + // an "active" assignee. + expect(row?.assignee_handle).toBeNull() + expect(row?.dispatch_id).toBeNull() + }) + it('supports parent_id for task decomposition', () => { const d = createDb() const parent = d.createTask({ spec: 'parent' }) @@ -454,4 +488,278 @@ describe('OrchestrationDb', () => { expect(d.listTasks()).toHaveLength(0) }) }) + + describe('heartbeat + thread helpers (fresh schema)', () => { + it('insertMessage accepts type = heartbeat', () => { + const d = createDb() + const msg = d.insertMessage({ + from: 'worker', + to: 'coord', + subject: 'alive', + type: 'heartbeat', + payload: JSON.stringify({ taskId: 'task_x', dispatchId: 'ctx_x' }) + }) + expect(msg.type).toBe('heartbeat') + }) + + it('recordHeartbeat updates last_heartbeat_at on dispatched rows', () => { + const d = createDb() + const task = d.createTask({ spec: 'work' }) + const ctx = d.createDispatchContext(task.id, 'term_a') + + d.recordHeartbeat(ctx.id, '2026-05-04T00:00:00.000Z') + const after = d.getDispatchContext(task.id) + expect(after?.last_heartbeat_at).toBe('2026-05-04T00:00:00.000Z') + }) + + it('recordHeartbeat is a no-op for completed rows (straggler ignored)', () => { + const d = createDb() + const task = d.createTask({ spec: 'work' }) + const ctx = d.createDispatchContext(task.id, 'term_a') + d.completeDispatch(ctx.id) + + d.recordHeartbeat(ctx.id, '2026-05-04T00:00:00.000Z') + const after = d.getDispatchContext(task.id) + expect(after?.last_heartbeat_at).toBeNull() + }) + + it('getStaleDispatches returns only dispatched rows past the grace window', () => { + const d = createDb() + // Fixture: four rows, SQL-backdated timestamps (no fake clock): + // (a) dispatched, heartbeated 5 min ago → not stale + // (b) dispatched, heartbeated 12 min ago → STALE (expected result) + // (c) dispatched, never heartbeated, dispatched 30s ago → not stale (grace) + // (d) completed, heartbeated 30 min ago → not stale (status filter) + const taskA = d.createTask({ spec: 'a' }) + const taskB = d.createTask({ spec: 'b' }) + const taskC = d.createTask({ spec: 'c' }) + const taskD = d.createTask({ spec: 'd' }) + const ctxA = d.createDispatchContext(taskA.id, 'term_a') + const ctxB = d.createDispatchContext(taskB.id, 'term_b') + const ctxC = d.createDispatchContext(taskC.id, 'term_c') + const ctxD = d.createDispatchContext(taskD.id, 'term_d') + d.completeDispatch(ctxD.id) + + const now = Date.now() + const iso = (ms: number) => new Date(now - ms).toISOString() + + // Backdate dispatched_at for a, b, d to long ago so the grace doesn't + // shield them. c keeps its default (≈now). + const sqlite = (d as unknown as { db: Database.Database }).db + sqlite + .prepare( + 'UPDATE dispatch_contexts SET dispatched_at = ?, last_heartbeat_at = ? WHERE id = ?' + ) + .run(iso(60 * 60 * 1000), iso(5 * 60 * 1000), ctxA.id) + sqlite + .prepare( + 'UPDATE dispatch_contexts SET dispatched_at = ?, last_heartbeat_at = ? WHERE id = ?' + ) + .run(iso(60 * 60 * 1000), iso(12 * 60 * 1000), ctxB.id) + sqlite + .prepare('UPDATE dispatch_contexts SET dispatched_at = ? WHERE id = ?') + .run(iso(30_000), ctxC.id) + sqlite + .prepare( + 'UPDATE dispatch_contexts SET dispatched_at = ?, last_heartbeat_at = ? WHERE id = ?' + ) + .run(iso(60 * 60 * 1000), iso(30 * 60 * 1000), ctxD.id) + + const stale = d.getStaleDispatches(iso(10 * 60 * 1000)) + expect(stale.map((s) => s.id)).toEqual([ctxB.id]) + }) + + it('getThreadMessagesFor returns only same-thread replies to a handle', () => { + const d = createDb() + const outbound = d.insertMessage({ + from: 'worker', + to: 'coord', + subject: 'Question', + type: 'decision_gate', + body: 'yes or no?' + }) + // Reply in the same thread addressed to the worker + const reply = d.insertMessage({ + from: 'coord', + to: 'worker', + subject: 'Re: Question', + body: 'yes', + threadId: outbound.id + }) + // Distractor: different thread, same recipient + d.insertMessage({ + from: 'coord', + to: 'worker', + subject: 'other', + body: 'unrelated', + threadId: 'thread_other' + }) + // Distractor: same thread but not addressed to worker + d.insertMessage({ + from: 'coord', + to: 'someone_else', + subject: 'cc', + body: 'not yours', + threadId: outbound.id + }) + + const replies = d.getThreadMessagesFor(outbound.id, 'worker', outbound.sequence) + expect(replies).toHaveLength(1) + expect(replies[0].id).toBe(reply.id) + }) + }) + + describe('schema migration from v1 → v2', () => { + let dbPath: string + let tempDir: string + + afterEach(() => { + if (tempDir) { + rmSync(tempDir, { recursive: true, force: true }) + } + }) + + function createV1Snapshot(): string { + tempDir = mkdtempSync(join(tmpdir(), 'orca-db-migrate-')) + dbPath = join(tempDir, 'test.db') + const raw = new Database(dbPath) + // v1 schema: pre-heartbeat CHECK, no last_heartbeat_at column. + raw.exec(` + CREATE TABLE messages ( + id TEXT NOT NULL, + from_handle TEXT NOT NULL, + to_handle TEXT NOT NULL, + subject TEXT NOT NULL, + body TEXT NOT NULL DEFAULT '', + type TEXT NOT NULL DEFAULT 'status' + CHECK(type IN ( + 'status', 'dispatch', 'worker_done', 'merge_ready', + 'escalation', 'handoff', 'decision_gate' + )), + priority TEXT NOT NULL DEFAULT 'normal' + CHECK(priority IN ('normal', 'high', 'urgent')), + thread_id TEXT, + payload TEXT, + read INTEGER NOT NULL DEFAULT 0, + sequence INTEGER PRIMARY KEY AUTOINCREMENT, + created_at TEXT NOT NULL DEFAULT (datetime('now')) + ); + CREATE UNIQUE INDEX idx_messages_id ON messages(id); + CREATE INDEX idx_inbox ON messages(to_handle, read); + CREATE INDEX idx_thread ON messages(thread_id); + + CREATE TABLE tasks ( + id TEXT PRIMARY KEY, parent_id TEXT, spec TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'pending' + CHECK(status IN ('pending','ready','dispatched','completed','failed','blocked')), + deps TEXT NOT NULL DEFAULT '[]', result TEXT, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + completed_at TEXT + ); + + CREATE TABLE dispatch_contexts ( + id TEXT PRIMARY KEY, task_id TEXT NOT NULL, assignee_handle TEXT, + status TEXT NOT NULL DEFAULT 'pending' + CHECK(status IN ('pending','dispatched','completed','failed','circuit_broken')), + failure_count INTEGER NOT NULL DEFAULT 0, last_failure TEXT, + dispatched_at TEXT, completed_at TEXT, + created_at TEXT NOT NULL DEFAULT (datetime('now')) + ); + + CREATE TABLE decision_gates ( + id TEXT PRIMARY KEY, task_id TEXT NOT NULL, question TEXT NOT NULL, + options TEXT NOT NULL DEFAULT '[]', + status TEXT NOT NULL DEFAULT 'pending' + CHECK(status IN ('pending','resolved','timeout')), + resolution TEXT, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + resolved_at TEXT + ); + + CREATE TABLE coordinator_runs ( + id TEXT PRIMARY KEY, spec TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'idle' + CHECK(status IN ('idle','running','completed','failed')), + coordinator_handle TEXT NOT NULL, + poll_interval_ms INTEGER NOT NULL DEFAULT 2000, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + completed_at TEXT + ); + `) + // Seed a pre-existing v1 message so migration must preserve data. + raw + .prepare( + `INSERT INTO messages (id, from_handle, to_handle, subject, type) VALUES ('msg_v1', 'a', 'b', 'pre-migration', 'status')` + ) + .run() + raw.pragma('user_version = 0') + raw.close() + return dbPath + } + + it('migrates a v1 snapshot to v2, accepts heartbeat, preserves indexes', () => { + const path = createV1Snapshot() + const d = new OrchestrationDb(path) + db = d + + // (a) INSERT type='heartbeat' now succeeds + expect(() => + d.insertMessage({ + from: 'w', + to: 'c', + subject: 'alive', + type: 'heartbeat', + payload: '{"taskId":"t","dispatchId":"ctx"}' + }) + ).not.toThrow() + + // (b) last_heartbeat_at column exists on dispatch_contexts + const task = d.createTask({ spec: 'work' }) + const ctx = d.createDispatchContext(task.id, 'term_a') + d.recordHeartbeat(ctx.id, '2026-05-04T00:00:00.000Z') + expect(d.getDispatchContext(task.id)?.last_heartbeat_at).toBe('2026-05-04T00:00:00.000Z') + + // (c) Indexes still attached to messages post-rebuild. + const sqlite = (d as unknown as { db: Database.Database }).db + const indexes = sqlite + .prepare( + `SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'messages' AND name NOT LIKE 'sqlite_%'` + ) + .all() as { name: string }[] + const names = new Set(indexes.map((r) => r.name)) + expect(names.has('idx_messages_id')).toBe(true) + expect(names.has('idx_inbox')).toBe(true) + expect(names.has('idx_thread')).toBe(true) + + // v1 data preserved + expect(d.getMessageById('msg_v1')?.subject).toBe('pre-migration') + }) + + it('is idempotent: opening an already-migrated DB is a no-op', () => { + const path = createV1Snapshot() + const first = new OrchestrationDb(path) + first.insertMessage({ + from: 'w', + to: 'c', + subject: 'alive', + type: 'heartbeat', + payload: '{}' + }) + first.close() + + const second = new OrchestrationDb(path) + db = second + expect(() => + second.insertMessage({ + from: 'w', + to: 'c', + subject: 'again', + type: 'heartbeat', + payload: '{}' + }) + ).not.toThrow() + const inbox = second.getInbox(10) + expect(inbox.length).toBeGreaterThanOrEqual(2) + }) + }) }) diff --git a/src/main/runtime/orchestration/db.ts b/src/main/runtime/orchestration/db.ts index b7c74bee72e..e8daa9f1708 100644 --- a/src/main/runtime/orchestration/db.ts +++ b/src/main/runtime/orchestration/db.ts @@ -33,6 +33,14 @@ function generateId(prefix: string): string { return `${prefix}_${randomBytes(6).toString('hex')}` } +// Why: v1 → v2 added `'heartbeat'` to messages.type CHECK + `last_heartbeat_at` +// column (preamble-hardening PR). v2 → v3 adds `delivered_at` column so +// push-on-idle can distinguish queued-but-undelivered from user-acknowledged +// messages without touching the `read` bit (check-wait PR). Bumping together +// since both PRs ship in the same release — existing on-disk DBs must clear +// both gaps in one atomic migration. +const SCHEMA_VERSION = 3 + export class OrchestrationDb { private db: Database.Database @@ -42,6 +50,7 @@ export class OrchestrationDb { this.db.pragma('synchronous = NORMAL') this.db.pragma('busy_timeout = 5000') this.createTables() + this.migrate() } private createTables(): void { @@ -55,7 +64,7 @@ export class OrchestrationDb { type TEXT NOT NULL DEFAULT 'status' CHECK(type IN ( 'status', 'dispatch', 'worker_done', 'merge_ready', - 'escalation', 'handoff', 'decision_gate' + 'escalation', 'handoff', 'decision_gate', 'heartbeat' )), priority TEXT NOT NULL DEFAULT 'normal' CHECK(priority IN ('normal', 'high', 'urgent')), @@ -63,7 +72,8 @@ export class OrchestrationDb { payload TEXT, read INTEGER NOT NULL DEFAULT 0, sequence INTEGER PRIMARY KEY AUTOINCREMENT, - created_at TEXT NOT NULL DEFAULT (datetime('now')) + created_at TEXT NOT NULL DEFAULT (datetime('now')), + delivered_at TEXT ); CREATE UNIQUE INDEX IF NOT EXISTS idx_messages_id ON messages(id); @@ -89,16 +99,17 @@ export class OrchestrationDb { CREATE INDEX IF NOT EXISTS idx_tasks_parent ON tasks(parent_id); CREATE TABLE IF NOT EXISTS dispatch_contexts ( - id TEXT PRIMARY KEY, - task_id TEXT NOT NULL, - assignee_handle TEXT, - status TEXT NOT NULL DEFAULT 'pending' + id TEXT PRIMARY KEY, + task_id TEXT NOT NULL, + assignee_handle TEXT, + status TEXT NOT NULL DEFAULT 'pending' CHECK(status IN ('pending', 'dispatched', 'completed', 'failed', 'circuit_broken')), - failure_count INTEGER NOT NULL DEFAULT 0, - last_failure TEXT, - dispatched_at TEXT, - completed_at TEXT, - created_at TEXT NOT NULL DEFAULT (datetime('now')) + failure_count INTEGER NOT NULL DEFAULT 0, + last_failure TEXT, + dispatched_at TEXT, + completed_at TEXT, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + last_heartbeat_at TEXT ); CREATE INDEX IF NOT EXISTS idx_dispatch_task ON dispatch_contexts(task_id); @@ -132,6 +143,115 @@ export class OrchestrationDb { `) } + // Why: `CREATE TABLE IF NOT EXISTS` is a no-op against an existing on-disk + // DB, so new schema shapes (added columns, widened CHECK constraints) do + // not reach an upgraded user unless we migrate explicitly. The transaction + // guarantees atomicity — a mid-migration crash leaves the DB at the prior + // version because `user_version` is bumped only on success. Idempotent + // re-invocation is a no-op (current >= SCHEMA_VERSION short-circuit). + private migrate(): void { + const current = this.db.pragma('user_version', { simple: true }) as number + if (current >= SCHEMA_VERSION) { + return + } + + this.db.exec('BEGIN') + try { + // v1 → v2: add last_heartbeat_at column; widen messages.type CHECK to + // include 'heartbeat'. SQLite cannot ALTER a CHECK constraint, so we + // rebuild the messages table. We also include `delivered_at` in the + // rebuilt schema so DBs migrating from v1 pick up the v3 column in a + // single table-rewrite pass (avoids a second messages-rebuild later). + if (current < 2) { + if (!this.hasColumn('dispatch_contexts', 'last_heartbeat_at')) { + this.db.exec(`ALTER TABLE dispatch_contexts ADD COLUMN last_heartbeat_at TEXT`) + } + + if (!this.messagesTypeCheckAllowsHeartbeat()) { + // Why — index list is not optional. createTables() already attached + // idx_messages_id / idx_inbox / idx_thread to the old messages table; + // DROP TABLE removes those indexes with it. CREATE INDEX IF NOT + // EXISTS in createTables() only runs on the next process startup, + // so skipping explicit recreation here would leave every + // getUnreadMessages / getMessageById call full-scanning for the + // rest of this process's lifetime — a silent O(N) perf regression. + // The three CREATE INDEX statements below mirror createTables() + // verbatim so the two definitions cannot drift. + this.db.exec(` + CREATE TABLE messages_new ( + id TEXT NOT NULL, + from_handle TEXT NOT NULL, + to_handle TEXT NOT NULL, + subject TEXT NOT NULL, + body TEXT NOT NULL DEFAULT '', + type TEXT NOT NULL DEFAULT 'status' + CHECK(type IN ( + 'status', 'dispatch', 'worker_done', 'merge_ready', + 'escalation', 'handoff', 'decision_gate', 'heartbeat' + )), + priority TEXT NOT NULL DEFAULT 'normal' + CHECK(priority IN ('normal', 'high', 'urgent')), + thread_id TEXT, + payload TEXT, + read INTEGER NOT NULL DEFAULT 0, + sequence INTEGER PRIMARY KEY AUTOINCREMENT, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + delivered_at TEXT + ); + INSERT INTO messages_new ( + id, from_handle, to_handle, subject, body, type, priority, + thread_id, payload, read, sequence, created_at + ) + SELECT + id, from_handle, to_handle, subject, body, type, priority, + thread_id, payload, read, sequence, created_at + FROM messages; + DROP TABLE messages; + ALTER TABLE messages_new RENAME TO messages; + + CREATE UNIQUE INDEX idx_messages_id ON messages(id); + CREATE INDEX idx_inbox ON messages(to_handle, read); + CREATE INDEX idx_thread ON messages(thread_id); + `) + } + } + + // v2 → v3: add `delivered_at` column to messages. A DB that reached v2 + // via the v1 → v2 rebuild above already has the column (we included + // it in messages_new); this handles DBs that were at v2 before this + // release shipped (preamble PR deployed standalone, then check-wait + // merged). ALTER TABLE is idempotent via the hasColumn probe — a + // duplicate-column error would abort the whole transaction. + if (current < 3) { + if (!this.hasColumn('messages', 'delivered_at')) { + this.db.exec(`ALTER TABLE messages ADD COLUMN delivered_at TEXT`) + } + } + + this.db.pragma(`user_version = ${SCHEMA_VERSION}`) + this.db.exec('COMMIT') + } catch (err) { + this.db.exec('ROLLBACK') + throw err + } + } + + private hasColumn(table: string, column: string): boolean { + const rows = this.db.pragma(`table_info(${table})`) as { name: string }[] + return rows.some((r) => r.name === column) + } + + // Why: sqlite_master stores the original CREATE TABLE SQL including the + // CHECK clause. Inspecting that text is the cheapest reliable way to tell + // whether the pre-rebuild schema already knows about 'heartbeat' without + // needing a dedicated schema_meta row. + private messagesTypeCheckAllowsHeartbeat(): boolean { + const row = this.db + .prepare("SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'messages'") + .get() as { sql: string } | undefined + return !!row && row.sql.includes("'heartbeat'") + } + // ── Messages ── insertMessage(msg: { @@ -195,12 +315,56 @@ export class OrchestrationDb { this.db.prepare(`UPDATE messages SET read = 1 WHERE id IN (${placeholders})`).run(...ids) } + // Why: `delivered_at` is stamped via SQLite's datetime('now') rather than a + // JS ISO string so it uses the same 'YYYY-MM-DD HH:MM:SS' UTC shape as the + // other SQL-default timestamps on this table. A future ORDER BY or + // comparison against created_at relies on this format consistency. + // See design doc §3.2. + markAsDelivered(ids: string[]): void { + if (ids.length === 0) { + return + } + const placeholders = ids.map(() => '?').join(',') + this.db + .prepare(`UPDATE messages SET delivered_at = datetime('now') WHERE id IN (${placeholders})`) + .run(...ids) + } + getInbox(limit = 20): MessageRow[] { return this.db .prepare('SELECT * FROM messages ORDER BY sequence DESC LIMIT ?') .all(limit) as MessageRow[] } + // Why: used by `check --all` and `inbox --terminal ` — returns every + // message for a handle regardless of read/delivered state; never touches the + // read bit. Stale-handle safe: if the handle no longer exists, the query + // just returns whatever historical rows remain (§3.3). + getAllMessagesForHandle(toHandle: string, limit = 100): MessageRow[] { + return this.db + .prepare('SELECT * FROM messages WHERE to_handle = ? ORDER BY sequence DESC LIMIT ?') + .all(toHandle, limit) as MessageRow[] + } + + // Why: thread-scoped read for the `orchestration.ask` wait loop. Filtered + // by `to_handle` so a worker only sees replies addressed to it (not + // messages it sent), and ordered by `sequence` so the first post-ask + // reply is returned first. `afterSequence` lets the caller resume past an + // already-seen marker without re-reading the outbound ask itself. Uses + // the existing idx_thread index (see createTables) — no new index. + getThreadMessagesFor(threadId: string, toHandle: string, afterSequence?: number): MessageRow[] { + if (afterSequence !== undefined) { + return this.db + .prepare( + 'SELECT * FROM messages WHERE thread_id = ? AND to_handle = ? AND sequence > ? ORDER BY sequence ASC' + ) + .all(threadId, toHandle, afterSequence) as MessageRow[] + } + return this.db + .prepare('SELECT * FROM messages WHERE thread_id = ? AND to_handle = ? ORDER BY sequence ASC') + .all(threadId, toHandle) as MessageRow[] + } + // ── Tasks ── createTask(task: { spec: string; deps?: string[]; parentId?: string }): TaskRow { @@ -232,6 +396,50 @@ export class OrchestrationDb { return this.db.prepare('SELECT * FROM tasks ORDER BY created_at').all() as TaskRow[] } + // Why: surfaces the active dispatch (assignee handle + dispatch context id) + // alongside each task so coordinators can answer "who is working on task X?" + // from a single query. The LEFT JOIN keeps non-dispatched tasks in the result + // with NULL assignee/dispatch fields so non-dispatched output stays stable. + // The inner subquery picks the most recent active dispatch per task to match + // the semantics of getDispatchContext for dispatched tasks. + listTasksWithDispatch(filter?: { status?: TaskStatus; ready?: boolean }): (TaskRow & { + assignee_handle: string | null + dispatch_id: string | null + })[] { + const whereClauses: string[] = [] + const params: unknown[] = [] + if (filter?.ready) { + whereClauses.push("t.status = 'ready'") + } else if (filter?.status) { + whereClauses.push('t.status = ?') + params.push(filter.status) + } + const where = whereClauses.length > 0 ? `WHERE ${whereClauses.join(' AND ')}` : '' + const sql = ` + SELECT + t.*, + d.assignee_handle AS assignee_handle, + d.id AS dispatch_id + FROM tasks t + LEFT JOIN ( + SELECT dc.* + FROM dispatch_contexts dc + INNER JOIN ( + SELECT task_id, MAX(rowid) AS max_rowid + FROM dispatch_contexts + WHERE status IN ('pending', 'dispatched') + GROUP BY task_id + ) latest ON latest.task_id = dc.task_id AND latest.max_rowid = dc.rowid + ) d ON d.task_id = t.id + ${where} + ORDER BY t.created_at + ` + return this.db.prepare(sql).all(...params) as (TaskRow & { + assignee_handle: string | null + dispatch_id: string | null + })[] + } + updateTaskStatus(id: string, status: TaskStatus, result?: string): TaskRow | undefined { const completedAt = status === 'completed' || status === 'failed' ? new Date().toISOString() : null @@ -362,6 +570,40 @@ export class OrchestrationDb { return active ? this.failDispatch(active.id, error) : undefined } + // Why: only touch rows that are currently dispatched. A straggler heartbeat + // from a dispatch that already transitioned to `completed` / `failed` / + // `circuit_broken` MUST NOT retroactively bump `last_heartbeat_at`, because + // the stale-dispatch detector is the signal the coordinator uses to know a + // newer dispatch for the same task has hung. Silently no-op'ing keeps the + // zombie-heartbeat race from masking a hung retry (§5.3.4). + recordHeartbeat(dispatchId: string, at: string): void { + this.db + .prepare( + "UPDATE dispatch_contexts SET last_heartbeat_at = ? WHERE id = ? AND status = 'dispatched'" + ) + .run(at, dispatchId) + } + + // Why: the query restricts to currently-dispatched contexts AND respects a + // dispatched-at grace. Without `status = 'dispatched'`, every completed / + // failed / circuit_broken row with an old-or-null last_heartbeat_at would + // warn every tick (warning storm). Without `dispatched_at < :threshold`, + // a freshly-dispatched worker would trip the warning during its first + // heartbeat interval (false positive). Callers supply the threshold as an + // ISO timestamp so the SQLite string-compare ordering works correctly + // (ISO-8601 compares lexicographically in time order). + getStaleDispatches(thresholdIso: string): DispatchContextRow[] { + return this.db + .prepare( + `SELECT * FROM dispatch_contexts + WHERE status = 'dispatched' + AND dispatched_at IS NOT NULL + AND dispatched_at < ? + AND (last_heartbeat_at IS NULL OR last_heartbeat_at < ?)` + ) + .all(thresholdIso, thresholdIso) as DispatchContextRow[] + } + failDispatch(ctxId: string, error: string): DispatchContextRow | undefined { const ctx = this.db.prepare('SELECT * FROM dispatch_contexts WHERE id = ?').get(ctxId) as | DispatchContextRow diff --git a/src/main/runtime/orchestration/formatter.test.ts b/src/main/runtime/orchestration/formatter.test.ts index 2b47d40273e..28182e3dbfb 100644 --- a/src/main/runtime/orchestration/formatter.test.ts +++ b/src/main/runtime/orchestration/formatter.test.ts @@ -16,6 +16,7 @@ function makeMessage(overrides: Partial = {}): MessageRow { read: 0, sequence: 1, created_at: '2026-01-01T00:00:00Z', + delivered_at: null, ...overrides } } diff --git a/src/main/runtime/orchestration/preamble.test.ts b/src/main/runtime/orchestration/preamble.test.ts index 71c27d50f66..aeb51e4e9e4 100644 --- a/src/main/runtime/orchestration/preamble.test.ts +++ b/src/main/runtime/orchestration/preamble.test.ts @@ -1,67 +1,118 @@ +import { spawnSync } from 'child_process' import { describe, expect, it } from 'vitest' import { buildDispatchPreamble } from './preamble' +function baseParams(overrides: Partial[0]> = {}) { + return { + taskId: 'task_abc123', + dispatchId: 'ctx_def456', + taskSpec: 'Implement the login form', + coordinatorHandle: 'term_coord', + ...overrides + } +} + describe('buildDispatchPreamble', () => { it('substitutes template variables', () => { - const result = buildDispatchPreamble({ - taskId: 'task_abc123', - taskSpec: 'Implement the login form', - coordinatorHandle: 'term_coord' - }) + const result = buildDispatchPreamble(baseParams()) expect(result).toContain('task_abc123') + expect(result).toContain('ctx_def456') expect(result).toContain('term_coord') expect(result).toContain('Implement the login form') expect(result).not.toContain('{{') }) - it('includes worker_done command', () => { - const result = buildDispatchPreamble({ - taskId: 'task_x', - taskSpec: 'do stuff', - coordinatorHandle: 'term_c' - }) + it('includes worker_done command with --body 3-sentence summary prompt and reportPath', () => { + const result = buildDispatchPreamble(baseParams()) expect(result).toContain('worker_done') expect(result).toContain('orchestration send') expect(result).toContain('orchestration check') + expect(result).toContain('--body') + expect(result).toMatch(/3-sentence summary/) + expect(result).toContain('reportPath') }) - it('includes the task spec after separator', () => { - const result = buildDispatchPreamble({ - taskId: 'task_x', - taskSpec: 'refactor the auth module', - coordinatorHandle: 'term_c' - }) + it('CLI examples parse as valid shell (bash -n on the extracted block)', () => { + const result = buildDispatchPreamble(baseParams()) + // Why: feeding `bash -n` the full preamble falsely fails on apostrophes + // in the surrounding prose. Slice between the CLI markers and strip + // shell-style comment lines so we only syntax-check the commands. + const cliStart = result.indexOf('=== CLI COMMANDS ===') + const cliEnd = result.indexOf('=== AFTER YOU SEND worker_done ===') + expect(cliStart).toBeGreaterThan(-1) + expect(cliEnd).toBeGreaterThan(cliStart) + const block = result.slice(cliStart, cliEnd) + const stripped = block + .split('\n') + .filter((line) => !line.trim().startsWith('#')) + .filter((line) => !line.trim().startsWith('===')) + .join('\n') - expect(result).toContain('--- TASK ---') + const check = spawnSync('bash', ['-n'], { input: stripped, encoding: 'utf8' }) + expect(check.status).toBe(0) + }) + + it('includes heartbeat CLI block with taskId and dispatchId and 5-minute cadence', () => { + const result = buildDispatchPreamble(baseParams()) + expect(result).toContain('--type heartbeat') + expect(result).toContain('--subject "alive"') + expect(result).toMatch(/5 minutes/) + // Both taskId and dispatchId are rendered inside the payload template + // (regression guard for §5.3.4 attribution — dispatchId attribution + // prevents the zombie-heartbeat-masks-hung-retry race). + const heartbeatLine = result + .split('\n') + .find((line) => line.includes('"taskId"') && line.includes('dispatchId')) + expect(heartbeatLine).toBeTruthy() + expect(heartbeatLine).toContain('task_abc123') + expect(heartbeatLine).toContain('ctx_def456') + }) + + it('includes ask block with BEHAVIOR RULE #1 forbidding AskUserQuestion', () => { + const result = buildDispatchPreamble(baseParams()) + expect(result).toContain('orchestration ask') + expect(result).toContain('--question') + expect(result).toContain('--timeout-ms 600000') + // Why: the exact phrase is asserted so the rule can't be trimmed away by + // accident. BEHAVIOR RULE #1 is the only place AskUserQuestion appears. + expect(result).toContain('BEHAVIOR RULE #1') + expect(result).toContain('NEVER use AskUserQuestion') + // AskUserQuestion must appear ONLY inside the rule text, not anywhere + // else (e.g., not in an example payload or header). Count occurrences + // of the exact token as a sanity check. + const occurrences = (result.match(/AskUserQuestion/g) ?? []).length + // Three mentions: the one-liner ban, the TUI-prompt rationale, and the + // "when tempted to reach for AskUserQuestion" closing line. + expect(occurrences).toBe(3) + }) + + it('includes AFTER YOU SEND block with 2-minute poll cadence and release signal', () => { + const result = buildDispatchPreamble(baseParams()) + expect(result).toContain('=== AFTER YOU SEND worker_done ===') + expect(result).toMatch(/2 minutes/) + expect(result).toMatch(/may exit/) + }) + + it('uses === TASK === separator with the task spec appended', () => { + const result = buildDispatchPreamble(baseParams({ taskSpec: 'refactor the auth module' })) + expect(result).toContain('=== TASK ===') expect(result).toContain('refactor the auth module') }) it('uses orca CLI by default when devMode is not set', () => { - const result = buildDispatchPreamble({ - taskId: 'task_x', - taskSpec: 'do stuff', - coordinatorHandle: 'term_c' - }) - + const result = buildDispatchPreamble(baseParams()) expect(result).toContain('orca orchestration send') expect(result).toContain('orca orchestration check') + expect(result).toContain('orca orchestration ask') }) it('uses orca-dev CLI when devMode is true', () => { - const result = buildDispatchPreamble({ - taskId: 'task_x', - taskSpec: 'do stuff', - coordinatorHandle: 'term_c', - devMode: true - }) - + const result = buildDispatchPreamble(baseParams({ devMode: true })) expect(result).toContain('orca-dev orchestration send') expect(result).toContain('orca-dev orchestration check') - // Ensure no bare "orca " (without -dev) appears as a CLI command. - // We split on "orca-dev" first so those occurrences don't produce - // false positives, then check the remaining fragments. + expect(result).toContain('orca-dev orchestration ask') const fragments = result.split('orca-dev') for (const fragment of fragments) { expect(fragment).not.toMatch(/orca orchestration/) @@ -69,14 +120,92 @@ describe('buildDispatchPreamble', () => { }) it('uses orca CLI when devMode is false', () => { - const result = buildDispatchPreamble({ - taskId: 'task_x', - taskSpec: 'do stuff', - coordinatorHandle: 'term_c', - devMode: false - }) - + const result = buildDispatchPreamble(baseParams({ devMode: false })) expect(result).toContain('orca orchestration send') expect(result).toContain('orca orchestration check') }) + + it('appends a BASE DRIFT section when baseDrift.behind > 0', () => { + const result = buildDispatchPreamble({ + taskId: 'task_x', + dispatchId: 'ctx_x', + taskSpec: 'do stuff', + coordinatorHandle: 'term_c', + baseDrift: { + base: 'origin/main', + behind: 7, + recentSubjects: ['fix: A', 'feat: B', 'chore: C'] + } + }) + + expect(result).toContain('--- BASE DRIFT ---') + expect(result).toContain('7 commits behind origin/main') + expect(result).toContain(' - fix: A') + expect(result).toContain(' - feat: B') + expect(result).toContain(' - chore: C') + // drift section must appear before the task spec + expect(result.indexOf('--- BASE DRIFT ---')).toBeLessThan(result.indexOf('=== TASK ===')) + }) + + it('omits the drift section when baseDrift.behind is 0', () => { + const result = buildDispatchPreamble({ + taskId: 'task_x', + dispatchId: 'ctx_x', + taskSpec: 'do stuff', + coordinatorHandle: 'term_c', + baseDrift: { + base: 'origin/main', + behind: 0, + recentSubjects: [] + } + }) + + expect(result).not.toContain('--- BASE DRIFT ---') + expect(result).not.toContain('commits behind') + }) + + it('omits the drift section when baseDrift is undefined', () => { + const result = buildDispatchPreamble({ + taskId: 'task_x', + dispatchId: 'ctx_x', + taskSpec: 'do stuff', + coordinatorHandle: 'term_c' + }) + + expect(result).not.toContain('--- BASE DRIFT ---') + expect(result).not.toContain('commits behind') + }) + + it('lists drift subjects in the order provided, each prefixed with two spaces and dash', () => { + const result = buildDispatchPreamble({ + taskId: 'task_x', + dispatchId: 'ctx_x', + taskSpec: 'do stuff', + coordinatorHandle: 'term_c', + baseDrift: { + base: 'origin/main', + behind: 3, + recentSubjects: ['first', 'second', 'third'] + } + }) + + const firstIdx = result.indexOf(' - first') + const secondIdx = result.indexOf(' - second') + const thirdIdx = result.indexOf(' - third') + expect(firstIdx).toBeGreaterThanOrEqual(0) + expect(secondIdx).toBeGreaterThan(firstIdx) + expect(thirdIdx).toBeGreaterThan(secondIdx) + }) + + it('renders a stable snapshot of the full preamble', () => { + // Why: single strict snapshot catches any accidental regression in + // formatting or rule presence in one line. + const result = buildDispatchPreamble({ + taskId: 'task_SNAP', + dispatchId: 'ctx_SNAP', + taskSpec: 'TASK_BODY', + coordinatorHandle: 'term_COORD' + }) + expect(result).toMatchSnapshot() + }) }) diff --git a/src/main/runtime/orchestration/preamble.ts b/src/main/runtime/orchestration/preamble.ts index 3c61f0be56a..6223dc94aa8 100644 --- a/src/main/runtime/orchestration/preamble.ts +++ b/src/main/runtime/orchestration/preamble.ts @@ -1,41 +1,147 @@ export type PreambleParams = { taskId: string + // Why: the heartbeat payload attributes liveness to a specific dispatch + // context (not just a task). A retried task has multiple dispatch_contexts + // rows; keying heartbeats on dispatchId prevents a straggler heartbeat from + // a previously-failed dispatch from masking a hung retry. Both call sites + // already have this value in scope (coordinator.ts dispatch.id, rpc + // orchestration.ts ctx.id). + dispatchId: string taskSpec: string coordinatorHandle: string devMode?: boolean + // Why: populated by the coordinator's dispatch pre-flight (§3.1) only + // when the target worktree is behind its tracking remote. When absent + // or when `behind === 0`, the preamble emits no drift section. Callers + // must NOT pre-populate this with empty data; the drift section is a + // loud-but-rare signal tied to the `allow-stale-base: true` override + // path, and polluting it for fresh worktrees would train workers to + // ignore it. + baseDrift?: { + base: string + behind: number + recentSubjects: string[] + } } +// Why: 5 minutes is frequent enough that the coordinator's stale-heartbeat +// check (threshold 10 min) catches a hung worker within one tick, and +// infrequent enough to avoid inbox spam on long tasks. One constant so +// cadence tuning is a single-line change (Q1 in DESIGN_DOC_PREAMBLE_FIX.md). +const HEARTBEAT_INTERVAL_MIN = 5 + // Why: the dispatch preamble teaches agents about Orca's CLI commands for -// structured communication. Agents don't need prior knowledge of Orca — they -// treat these as shell tools the same way they use git or npm. +// structured communication. Behavioral rules (body summary, heartbeat cadence, +// no-AskUserQuestion) live as inline comments above the relevant CLI example, +// not as a separate prose block — LLM readers anchor on examples and skim +// trailing prose, so rules must land at the point of use. export function buildDispatchPreamble(params: PreambleParams): string { // Why: in dev mode, agents must use orca-dev to connect to the dev runtime's // socket. Without this, agents inside the dev Electron app would call the // production CLI and talk to the wrong Orca instance (Section 6.4). const cli = params.devMode ? 'orca-dev' : 'orca' - return `You are working inside Orca, a multi-agent IDE. You have access to these -CLI commands for communicating with the coordinator: + const header = `You are working inside Orca, a multi-agent IDE. You are a dispatched worker. +Your coordinator's terminal handle is: ${params.coordinatorHandle} +Your task ID is: ${params.taskId} - # Report task completion (REQUIRED when done): +You talk to the coordinator only through the CLI commands below. Do not use +Slack, GitHub comments, or any other channel to reach a human during the run. + +=== CLI COMMANDS === + + # Report task completion (REQUIRED when done — even on failure). + # + # RULE: --body must be a 3-sentence executive summary (what you did, + # what you found, what's left). Never send an empty body; the coordinator + # reads the body first and only opens artifacts if it needs more detail. + # If you produced a long-form artifact, include its path as + # payload.reportPath so the coordinator can find it without a file search. + # + # RULE: send worker_done exactly once. Failure is still a worker_done + # with subject like "Failed: " — never silently exit. ${cli} orchestration send --to ${params.coordinatorHandle} \\ - --type worker_done --subject "Done" \\ - --payload '{"taskId":"${params.taskId}","filesModified":[...]}' + --type worker_done --subject "" \\ + --body "<3-sentence summary: what you did, what you found, what's left>" \\ + --payload '{"taskId":"${params.taskId}","filesModified":["path/a","path/b"],"reportPath":""}' - # Report a blocker or failure: + # BEHAVIOR RULE: send a heartbeat every ${HEARTBEAT_INTERVAL_MIN} minutes + # while actively working on the task. The coordinator uses this to + # distinguish "still thinking" from "hung / crashed." Skip heartbeats only + # while blocked inside \`check --wait\` or \`ask\` — those calls are + # themselves liveness signals. + # + # Include BOTH taskId and dispatchId in the payload: the coordinator + # attributes the heartbeat to the specific dispatch context, not just + # the task, so a straggler heartbeat from a previously-failed dispatch + # cannot mask a hung retry. + ${cli} orchestration send --to ${params.coordinatorHandle} \\ + --type heartbeat --subject "alive" \\ + --payload '{"taskId":"${params.taskId}","dispatchId":"${params.dispatchId}","phase":""}' + + # Ask the coordinator a question and block until it answers. + # + # BEHAVIOR RULE #1 (MUST NOT VIOLATE): + # NEVER use AskUserQuestion; use \`${cli} orchestration ask\` or send + # --type decision_gate. AskUserQuestion opens a local TUI prompt that the + # coordinator cannot see and cannot answer — your session will hang forever + # waiting on a human. Every interactive question goes through \`ask\` below. + # + # The \`ask\` verb is a thin wrapper: it sends a decision_gate message and + # blocks on \`check --wait\` until the coordinator replies, then prints the + # reply body. Use it anywhere you would otherwise have reached for + # AskUserQuestion. + ${cli} orchestration ask --to ${params.coordinatorHandle} \\ + --question "" \\ + --options "" \\ + --timeout-ms 600000 + + # Escalate a blocker or failure (pre-completion, when you need the + # coordinator to do something before you can continue): ${cli} orchestration send --to ${params.coordinatorHandle} \\ --type escalation --subject "Blocked: " \\ - --body "
" + --body "
" \\ + --payload '{"taskId":"${params.taskId}"}' - # Check for messages from the coordinator or other agents: + # Check for messages from the coordinator: ${cli} orchestration check -Your assigned task ID is: ${params.taskId} +=== AFTER YOU SEND worker_done === -When you finish your task, run the worker_done command above with the -list of files you modified. If you are blocked or need help, send an -escalation. Do not exit the session. +Keep the shell session open for a grace period (10 minutes) in case the +coordinator sends a follow-up or re-dispatches you. Poll with +\`${cli} orchestration check\` every 2 minutes during that window. If no +follow-up arrives, you may exit after the grace period — the coordinator +will not expect further output from you. ---- TASK --- +If the coordinator re-dispatches you (you will receive a fresh preamble + +TASK block), reset your polling and start the new task. Do not respond +to the previous task's follow-ups after a re-dispatch.` + + // Why: the drift section fires only when the coordinator allowed dispatch + // against a stale worktree (via `allow-stale-base: true` in the task spec, + // see §3.4) OR when behind>0 but under the refusal threshold. Either way + // it is defense-in-depth: the worker sees the drift from line 1 instead + // of discovering it via stale line numbers in artifacts later. + const drift = + params.baseDrift && params.baseDrift.behind > 0 ? buildDriftSection(params.baseDrift) : '' + + return `${header}${drift} + +=== TASK === ${params.taskSpec}` } + +function buildDriftSection(drift: NonNullable): string { + const subjects = drift.recentSubjects.map((s) => ` - ${s}`).join('\n') + return ` + +--- BASE DRIFT --- +Your worktree HEAD is ${drift.behind} commits behind ${drift.base}. The 5 most recent +subjects on ${drift.base} NOT in your worktree: +${subjects} + +If any look relevant to your task, either pull them in (\`git pull --rebase +${drift.base}\` or equivalent) or escalate to the coordinator before starting. +---` +} diff --git a/src/main/runtime/orchestration/types.ts b/src/main/runtime/orchestration/types.ts index 1d0ae9a3914..03a31391cc9 100644 --- a/src/main/runtime/orchestration/types.ts +++ b/src/main/runtime/orchestration/types.ts @@ -6,6 +6,7 @@ export type MessageType = | 'escalation' | 'handoff' | 'decision_gate' + | 'heartbeat' export type MessagePriority = 'normal' | 'high' | 'urgent' @@ -30,6 +31,7 @@ export type MessageRow = { read: number sequence: number created_at: string + delivered_at: string | null } export type TaskRow = { @@ -53,6 +55,7 @@ export type DispatchContextRow = { dispatched_at: string | null completed_at: string | null created_at: string + last_heartbeat_at: string | null } export type DecisionGateRow = { diff --git a/src/main/runtime/rpc/core.ts b/src/main/runtime/rpc/core.ts index 9b4bae3d4e3..a91745cc87a 100644 --- a/src/main/runtime/rpc/core.ts +++ b/src/main/runtime/rpc/core.ts @@ -38,6 +38,13 @@ export type RpcRequest = { export type RpcContext = { runtime: OrcaRuntimeService + // Why: long-poll handlers (e.g. orchestration.check with wait=true) need to + // observe the underlying socket's lifetime so they can release their slot + // and resolve their inner waiters immediately when a client disconnects + // instead of running down the configured timeoutMs. Undefined outside the + // runtime-rpc transport (direct in-process callers don't need it). + // See design doc §3.1 counter-lifecycle. + signal?: AbortSignal } export type RpcHandler = (params: TParams, ctx: RpcContext) => Promise | unknown diff --git a/src/main/runtime/rpc/dispatcher.ts b/src/main/runtime/rpc/dispatcher.ts index 105fba0bc55..8d0d1dc5cd3 100644 --- a/src/main/runtime/rpc/dispatcher.ts +++ b/src/main/runtime/rpc/dispatcher.ts @@ -30,7 +30,7 @@ export class RpcDispatcher { this.registry = buildRegistry(methods) } - async dispatch(request: RpcRequest): Promise { + async dispatch(request: RpcRequest, options?: { signal?: AbortSignal }): Promise { const meta = this.meta() const method = this.registry.get(request.method) if (!method) { @@ -55,7 +55,10 @@ export class RpcDispatcher { } try { - const result = await method.handler(parsedParams, { runtime: this.runtime }) + const result = await method.handler(parsedParams, { + runtime: this.runtime, + signal: options?.signal + }) return successResponse(request.id, meta, result) } catch (error) { // Why: browser methods throw BrowserError with a structured `code`; diff --git a/src/main/runtime/rpc/methods/orchestration.test.ts b/src/main/runtime/rpc/methods/orchestration.test.ts index 64a2507d0f1..3ae1d213586 100644 --- a/src/main/runtime/rpc/methods/orchestration.test.ts +++ b/src/main/runtime/rpc/methods/orchestration.test.ts @@ -38,7 +38,7 @@ describe('orchestration RPC methods', () => { it('registers all expected methods', () => { const registry = buildRegistry(ORCHESTRATION_METHODS) - expect(registry.size).toBe(15) + expect(registry.size).toBe(16) expect(registry.has('orchestration.send')).toBe(true) expect(registry.has('orchestration.check')).toBe(true) expect(registry.has('orchestration.reply')).toBe(true) @@ -48,6 +48,7 @@ describe('orchestration RPC methods', () => { expect(registry.has('orchestration.taskUpdate')).toBe(true) expect(registry.has('orchestration.dispatch')).toBe(true) expect(registry.has('orchestration.dispatchShow')).toBe(true) + expect(registry.has('orchestration.ask')).toBe(true) expect(registry.has('orchestration.run')).toBe(true) expect(registry.has('orchestration.runStop')).toBe(true) expect(registry.has('orchestration.gateCreate')).toBe(true) @@ -275,6 +276,79 @@ describe('orchestration RPC methods', () => { }) ).rejects.toThrow('Invalid --types') }) + + it('default (unread only) marks returned rows as read', async () => { + setup() + db.insertMessage({ from: 'a', to: 'b', subject: 'one' }) + db.insertMessage({ from: 'a', to: 'b', subject: 'two' }) + + const first = (await call('orchestration.check', { terminal: 'b' })) as { + count: number + } + expect(first.count).toBe(2) + + const second = (await call('orchestration.check', { terminal: 'b' })) as { + count: number + } + expect(second.count).toBe(0) + }) + + it('--all returns every message for the handle without marking read', async () => { + setup() + db.insertMessage({ from: 'a', to: 'b', subject: 'one' }) + const second = db.insertMessage({ from: 'a', to: 'b', subject: 'two' }) + db.markAsRead([second.id]) + + const result = (await call('orchestration.check', { + terminal: 'b', + all: true + })) as { messages: { read: number }[]; count: number } + + expect(result.count).toBe(2) + // Must not have flipped the remaining unread row + const stillUnread = db.getUnreadMessages('b') + expect(stillUnread).toHaveLength(1) + }) + + it('--all returns rows with delivered_at set after push-on-idle stamped them', async () => { + setup() + const msg = db.insertMessage({ from: 'a', to: 'b', subject: 'hi' }) + // Why: simulate push-on-idle stamping delivered_at without the runtime loop. + db.markAsDelivered([msg.id]) + + const result = (await call('orchestration.check', { + terminal: 'b', + all: true + })) as { messages: { id: string; delivered_at: string | null }[]; count: number } + + expect(result.count).toBe(1) + expect(result.messages[0].delivered_at).not.toBeNull() + }) + + it('--all --terminal returns empty list', async () => { + setup() + db.insertMessage({ from: 'a', to: 'b', subject: 'one' }) + + const result = (await call('orchestration.check', { + terminal: 'does_not_exist', + all: true + })) as { count: number } + expect(result.count).toBe(0) + }) + + it('unread:false compat shim behaves like --all (one-release bridge)', async () => { + setup() + db.insertMessage({ from: 'a', to: 'b', subject: 'one' }) + + const result = (await call('orchestration.check', { + terminal: 'b', + unread: false + })) as { count: number } + expect(result.count).toBe(1) + + // Must not have marked read + expect(db.getUnreadMessages('b')).toHaveLength(1) + }) }) describe('orchestration.reply', () => { @@ -310,6 +384,38 @@ describe('orchestration RPC methods', () => { const result = (await call('orchestration.inbox', {})) as { count: number } expect(result.count).toBe(2) }) + + it('--terminal matches check --all output for the same handle', async () => { + setup() + db.insertMessage({ from: 'a', to: 'b', subject: 'one' }) + db.insertMessage({ from: 'a', to: 'b', subject: 'two' }) + db.insertMessage({ from: 'a', to: 'c', subject: 'other' }) + + const inbox = (await call('orchestration.inbox', { terminal: 'b' })) as { + messages: { id: string; to_handle: string }[] + count: number + } + const check = (await call('orchestration.check', { + terminal: 'b', + all: true + })) as { messages: { id: string; to_handle: string }[]; count: number } + + expect(inbox.count).toBe(2) + expect(check.count).toBe(2) + // Same rows in the same order — both use sequence DESC + expect(inbox.messages.map((m) => m.id)).toEqual(check.messages.map((m) => m.id)) + expect(inbox.messages.every((m) => m.to_handle === 'b')).toBe(true) + }) + + it('--terminal returns empty list without erroring', async () => { + setup() + db.insertMessage({ from: 'a', to: 'b', subject: 'one' }) + + const result = (await call('orchestration.inbox', { + terminal: 'does_not_exist' + })) as { count: number } + expect(result.count).toBe(0) + }) }) describe('orchestration.taskCreate', () => { @@ -369,6 +475,33 @@ describe('orchestration RPC methods', () => { const method = findMethod('orchestration.taskList') expect(() => method.params!.parse({ status: 'done-ish' })).toThrow() }) + + it('includes assignee_handle and dispatch_id for dispatched tasks', async () => { + setup() + const t1 = db.createTask({ spec: 'ready work' }) + const t2 = db.createTask({ spec: 'active work' }) + const ctx = db.createDispatchContext(t2.id, 'term_worker') + + const result = (await call('orchestration.taskList', {})) as { + tasks: { + id: string + status: string + assignee_handle?: string | null + dispatch_id?: string | null + }[] + } + + const ready = result.tasks.find((t) => t.id === t1.id) + const dispatched = result.tasks.find((t) => t.id === t2.id) + expect(ready).toBeDefined() + expect(dispatched).toBeDefined() + // Non-dispatched tasks keep the legacy shape — no assignee/dispatch fields. + expect(ready).not.toHaveProperty('assignee_handle') + expect(ready).not.toHaveProperty('dispatch_id') + // Dispatched tasks surface the active dispatch. + expect(dispatched?.assignee_handle).toBe('term_worker') + expect(dispatched?.dispatch_id).toBe(ctx.id) + }) }) describe('orchestration.taskUpdate', () => { @@ -496,6 +629,50 @@ describe('orchestration RPC methods', () => { /already has an active dispatch/ ) }) + + it('dry-run returns the preamble without mutating state', async () => { + setup() + const task = db.createTask({ spec: 'work' }) + + const result = (await call('orchestration.dispatch', { + task: task.id, + to: 'term_a', + inject: true, + dryRun: true, + from: 'term_coord' + })) as { + dispatch: null + dryRun: boolean + preamble: string + injected: boolean + } + + expect(result.dryRun).toBe(true) + expect(result.dispatch).toBeNull() + expect(result.injected).toBe(false) + expect(result.preamble).toContain('work') + expect(result.preamble).toContain(task.id) + expect(result.preamble).toContain('term_coord') + // Task state must not change on dry-run. + expect(db.getTask(task.id)?.status).toBe('ready') + expect(db.getDispatchContext(task.id)).toBeUndefined() + }) + + it('returnPreamble includes preamble in the response', async () => { + setup() + const task = db.createTask({ spec: 'work' }) + + const result = (await call('orchestration.dispatch', { + task: task.id, + to: 'term_a', + returnPreamble: true, + from: 'term_coord' + })) as { dispatch: { id: string }; preamble: string } + + expect(result.dispatch.id).toMatch(/^ctx_/) + expect(result.preamble).toContain(task.id) + expect(result.preamble).toContain('term_coord') + }) }) describe('orchestration.dispatchShow', () => { @@ -519,6 +696,44 @@ describe('orchestration RPC methods', () => { expect(result.dispatch).toBeNull() }) + + it('--preamble returns the preamble text', async () => { + setup() + const task = db.createTask({ spec: 'refactor auth' }) + db.createDispatchContext(task.id, 'term_a') + + const result = (await call('orchestration.dispatchShow', { + task: task.id, + preamble: true, + from: 'term_coord' + })) as { dispatch: { task_id: string } | null; preamble: string } + + expect(result.preamble).toContain('refactor auth') + expect(result.preamble).toContain(task.id) + expect(result.preamble).toContain('term_coord') + expect(result.dispatch?.task_id).toBe(task.id) + }) + + it('--preamble works when no dispatch exists yet', async () => { + setup() + const task = db.createTask({ spec: 'build feature' }) + + const result = (await call('orchestration.dispatchShow', { + task: task.id, + preamble: true, + from: 'term_coord' + })) as { dispatch: null; preamble: string } + + expect(result.dispatch).toBeNull() + expect(result.preamble).toContain('build feature') + }) + + it('--preamble throws for unknown task', async () => { + setup() + await expect( + call('orchestration.dispatchShow', { task: 'task_fake', preamble: true }) + ).rejects.toThrow('Task not found') + }) }) describe('orchestration.gateCreate', () => { @@ -621,6 +836,145 @@ describe('orchestration RPC methods', () => { }) }) + describe('orchestration.ask', () => { + it('sends a decision_gate and returns the first thread reply', async () => { + setup() + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + vi.spyOn(runtime, 'waitForMessage').mockImplementation(async () => { + // Simulate coordinator replying in the thread during the wait + const outbound = db.getInbox(10).find((m) => m.type === 'decision_gate') + if (outbound) { + db.insertMessage({ + from: 'term_coord', + to: 'term_worker', + subject: 'Re: Question', + body: 'go ahead', + threadId: outbound.id + }) + } + }) + + const result = (await call('orchestration.ask', { + from: 'term_worker', + to: 'term_coord', + question: 'proceed?', + options: 'yes, no', + timeoutMs: 500 + })) as { + answer: string + messageId: string + threadId: string + timedOut: boolean + } + + expect(result.timedOut).toBe(false) + expect(result.answer).toBe('go ahead') + expect(result.messageId).toMatch(/^msg_/) + + // Outbound decision_gate message was persisted with parsed options. + const outbound = db.getInbox(10).find((m) => m.type === 'decision_gate') + expect(outbound).toBeTruthy() + expect(outbound?.subject).toBe('Question') + expect(outbound?.body).toBe('proceed?') + const payload = JSON.parse(outbound!.payload ?? '{}') + expect(payload.question).toBe('proceed?') + expect(payload.options).toEqual(['yes', 'no']) + }) + + it('returns timedOut when no reply arrives in the window', async () => { + setup() + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + vi.spyOn(runtime, 'waitForMessage').mockResolvedValue() + + const result = (await call('orchestration.ask', { + from: 'term_worker', + to: 'term_coord', + question: 'still there?', + timeoutMs: 1 + })) as { answer: string | null; timedOut: boolean; messageId: string | null } + + expect(result.timedOut).toBe(true) + expect(result.answer).toBeNull() + expect(result.messageId).toBeNull() + // Outbound message still persisted (coordinator can still see it). + const outbound = db.getInbox(10).find((m) => m.type === 'decision_gate') + expect(outbound).toBeTruthy() + }) + + it('rejects group addresses with a dedicated error (no message persisted)', async () => { + setup() + await expect( + call('orchestration.ask', { + from: 'term_worker', + to: '@reviewers', + question: 'ok?' + }) + ).rejects.toThrow(/does not support group addresses/) + expect(db.getInbox(10)).toHaveLength(0) + }) + + it('does not return distractor messages on a different thread', async () => { + setup() + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + let wakeCount = 0 + vi.spyOn(runtime, 'waitForMessage').mockImplementation(async () => { + wakeCount++ + const outbound = db.getInbox(20).find((m) => m.type === 'decision_gate') + if (wakeCount === 1 && outbound) { + // First wake: distractor in a DIFFERENT thread — must be ignored. + db.insertMessage({ + from: 'term_coord', + to: 'term_worker', + subject: 'unrelated', + body: 'other', + threadId: 'thread_other' + }) + } else if (wakeCount === 2 && outbound) { + // Second wake: correct thread reply. + db.insertMessage({ + from: 'term_coord', + to: 'term_worker', + subject: 'Re: Question', + body: 'correct answer', + threadId: outbound.id + }) + } + }) + + const result = (await call('orchestration.ask', { + from: 'term_worker', + to: 'term_coord', + question: 'filter?', + timeoutMs: 2_000 + })) as { answer: string; timedOut: boolean } + + expect(result.timedOut).toBe(false) + expect(result.answer).toBe('correct answer') + }) + + it('parses options CSV with whitespace and empty entries', async () => { + setup() + vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) + vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) + vi.spyOn(runtime, 'waitForMessage').mockResolvedValue() + + await call('orchestration.ask', { + from: 'w', + to: 'c', + question: 'q', + options: 'a, b ,,c', + timeoutMs: 1 + }) + + const outbound = db.getInbox(10).find((m) => m.type === 'decision_gate') + const payload = JSON.parse(outbound!.payload ?? '{}') + expect(payload.options).toEqual(['a', 'b', 'c']) + }) + }) + describe('orchestration.reset', () => { it('resets all state', async () => { setup() diff --git a/src/main/runtime/rpc/methods/orchestration.ts b/src/main/runtime/rpc/methods/orchestration.ts index 5c467e369b0..e1a2d68b943 100644 --- a/src/main/runtime/rpc/methods/orchestration.ts +++ b/src/main/runtime/rpc/methods/orchestration.ts @@ -15,7 +15,8 @@ const MESSAGE_TYPES: MessageType[] = [ 'merge_ready', 'escalation', 'handoff', - 'decision_gate' + 'decision_gate', + 'heartbeat' ] const TASK_STATUSES: TaskStatus[] = [ @@ -40,7 +41,8 @@ const SendParams = z.object({ 'merge_ready', 'escalation', 'handoff', - 'decision_gate' + 'decision_gate', + 'heartbeat' ]) .optional(), priority: z.enum(['normal', 'high', 'urgent']).optional(), @@ -52,6 +54,10 @@ const SendParams = z.object({ const CheckParams = z.object({ terminal: OptionalString, unread: OptionalBoolean, + // Why: `all` surfaces every message for the handle and skips mark-read. + // Previously the only way to ask for "all" was the hidden RPC trick + // `{unread: false}`. See design doc §3.2 / §3.3. + all: OptionalBoolean, types: OptionalString, inject: OptionalBoolean, wait: OptionalBoolean, @@ -65,7 +71,11 @@ const ReplyParams = z.object({ }) const InboxParams = z.object({ - limit: OptionalFiniteNumber + limit: OptionalFiniteNumber, + // Why: filters the inbox listing to a specific handle so coordinators can + // ask "everything for this handle" with either `inbox` or `check --all` + // and get agreeing results. See design doc §3.3. + terminal: OptionalString }) const TaskCreateParams = z.object({ @@ -99,14 +109,30 @@ const TaskUpdateParams = z.object({ const DispatchParams = z.object({ task: requiredString('Missing --task'), - to: requiredString('Missing --to'), + // Why: --to is only required for real dispatches. When --dry-run is set the + // caller is previewing the preamble and no terminal is targeted, so allow it + // to be absent. The handler enforces presence before any side-effecting work. + to: OptionalString, from: OptionalString, inject: OptionalBoolean, + dryRun: OptionalBoolean, + returnPreamble: OptionalBoolean, devMode: OptionalBoolean }) const DispatchShowParams = z.object({ - task: OptionalString + task: OptionalString, + preamble: OptionalBoolean, + from: OptionalString, + devMode: OptionalBoolean +}) + +const AskParams = z.object({ + to: requiredString('Missing --to'), + question: requiredString('Missing --question'), + options: OptionalString, + timeoutMs: OptionalFiniteNumber, + from: OptionalString }) const ResetParams = z.object({ @@ -177,7 +203,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ defineMethod({ name: 'orchestration.check', params: CheckParams, - handler: async (params, { runtime }) => { + handler: async (params, { runtime, signal }) => { const db = runtime.getOrchestrationDb() const handle = params.terminal ?? 'unknown' const typeFilter = params.types @@ -191,12 +217,17 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ throw new Error(`Invalid --types: ${invalidTypes.join(',')}`) } - const showUnread = params.unread !== false + // Why: `all` short-circuits to "everything for the handle, no marking." + // Explicit `unread: false` is also honored for one release as a compat + // shim so in-flight callers don't break (see design doc §5). Otherwise + // today's behavior is preserved: default is unread-only + mark-read. + const showAll = params.all === true || params.unread === false + const showUnread = !showAll const readAndReturn = () => { const messages = showUnread ? db.getUnreadMessages(handle, typeFilter) - : db.getAllMessages(handle) + : db.getAllMessagesForHandle(handle) if (showUnread && messages.length > 0) { db.markAsRead(messages.map((m) => m.id)) @@ -216,10 +247,15 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ } // Why: blocking wait lets coordinators replace sleep+poll loops with a - // single call that resolves when a message arrives or the timeout expires. + // single call that resolves when a message arrives or the timeout + // expires. The `signal` plumbed from the RPC transport aborts this + // waiter the moment the client socket closes, so a killed client + // releases its long-poll slot immediately rather than after the full + // timeoutMs. See design doc §3.1 counter-lifecycle. await runtime.waitForMessage(handle, { typeFilter: typeFilter as string[] | undefined, - timeoutMs: params.timeoutMs ?? undefined + timeoutMs: params.timeoutMs ?? undefined, + signal }) return readAndReturn() } @@ -255,7 +291,13 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ params: InboxParams, handler: (params, { runtime }) => { const db = runtime.getOrchestrationDb() - const messages = db.getInbox(params.limit) + // Why: when `terminal` is provided, mirror `check --all` output for that + // handle (same rows in the same sequence order). Stale/unknown handles + // return an empty list instead of erroring, matching the "historical + // rows survive handle deletion" rule in design doc §3.3. + const messages = params.terminal + ? db.getAllMessagesForHandle(params.terminal, params.limit) + : db.getInbox(params.limit) return { messages, count: messages.length } } }), @@ -291,10 +333,21 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ params: TaskListParams, handler: (params, { runtime }) => { const db = runtime.getOrchestrationDb() - const tasks = db.listTasks({ + // Why: listTasksWithDispatch returns the same rows as listTasks plus + // assignee_handle + dispatch_id joined in for tasks that currently have an + // active dispatch. Non-dispatched tasks get NULL for those fields, so + // consumers reading the legacy shape are unaffected. + const joined = db.listTasksWithDispatch({ status: params.status as TaskStatus, ready: params.ready }) + const tasks = joined.map((row) => { + const { assignee_handle, dispatch_id, ...base } = row + if (base.status === 'dispatched') { + return { ...base, assignee_handle, dispatch_id } + } + return base + }) return { tasks, count: tasks.length } } }), @@ -321,6 +374,30 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ if (!task) { throw new Error(`Task not found: ${params.task}`) } + + // Why: --inject --dry-run lets a coordinator preview the exact preamble + // text that would be injected without mutating task state or touching the + // target terminal. Skips the ready-status check so coordinators can inspect + // the preamble for already-dispatched or blocked tasks too. No dispatch + // context exists yet (that happens after the ready-status check), so + // dispatchId is a placeholder — the real injected preamble gets a real + // ctx.id below. + if (params.dryRun) { + const preamble = buildDispatchPreamble({ + taskId: task.id, + dispatchId: 'ctx_dryrun', + taskSpec: task.spec, + coordinatorHandle: params.from ?? 'coordinator', + devMode: params.devMode + }) + return { dispatch: null, injected: false, dryRun: true, preamble } + } + + if (!params.to) { + throw new Error('Missing --to') + } + const to = params.to + if (task.status !== 'ready') { throw new Error(`Task ${params.task} is ${task.status}; only ready tasks can be dispatched`) } @@ -330,28 +407,34 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ // status and foreground process — Claude Code doesn't emit recognized OSC // titles on startup, so title-only detection misses freshly spawned agents. if (params.inject) { - const hasAgent = await runtime.isTerminalRunningAgent(params.to) + const hasAgent = await runtime.isTerminalRunningAgent(to) if (!hasAgent) { throw new Error( - `Cannot dispatch --inject to terminal ${params.to}: no recognized agent detected. ` + + `Cannot dispatch --inject to terminal ${to}: no recognized agent detected. ` + 'Start an agent CLI (e.g. claude, codex, gemini) in the terminal first, ' + 'or dispatch without --inject and send the prompt manually.' ) } } - const ctx = db.createDispatchContext(params.task, params.to) + const ctx = db.createDispatchContext(params.task, to) + + // Why: preamble is built here (not before ctx) so `dispatchId` can be + // the real ctx.id — the preamble-hardening PR made dispatchId required + // so heartbeats can attribute liveness to a specific dispatch context, + // not just a task. + const preamble = buildDispatchPreamble({ + taskId: task.id, + dispatchId: ctx.id, + taskSpec: task.spec, + coordinatorHandle: params.from ?? 'coordinator', + devMode: params.devMode + }) let injected = false if (params.inject) { try { - const preamble = buildDispatchPreamble({ - taskId: task.id, - taskSpec: task.spec, - coordinatorHandle: params.from ?? 'coordinator', - devMode: params.devMode - }) - await runtime.sendTerminal(params.to, { text: preamble, enter: true }) + await runtime.sendTerminal(to, { text: preamble, enter: true }) injected = true } catch (err) { db.failDispatch(ctx.id, err instanceof Error ? err.message : String(err)) @@ -359,6 +442,12 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ } } + // Why: returnPreamble is opt-in because the preamble is several hundred + // bytes and most callers don't need it in the response. Exposing it + // supports coordinators that want to log what was injected for auditing. + if (params.returnPreamble) { + return { dispatch: ctx, injected, preamble } + } return { dispatch: ctx, injected } } }), @@ -372,10 +461,100 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ throw new Error('Missing --task') } const ctx = db.getDispatchContext(params.task) + + // Why: --preamble lets callers inspect the exact preamble text that was + // (or would be) injected for this task. The preamble is derived from the + // current task spec, so even after dispatch completes the text can be + // regenerated deterministically. + if (params.preamble) { + const task = db.getTask(params.task) + if (!task) { + throw new Error(`Task not found: ${params.task}`) + } + const preamble = buildDispatchPreamble({ + taskId: task.id, + // Why: prefer the existing dispatch context's id if we have one + // (so the preview matches what was actually injected); fall back + // to a placeholder when no dispatch has occurred yet. + dispatchId: ctx?.id ?? 'ctx_preview', + taskSpec: task.spec, + coordinatorHandle: params.from ?? 'coordinator', + devMode: params.devMode + }) + return { dispatch: ctx ?? null, preamble } + } + return { dispatch: ctx ?? null } } }), + defineMethod({ + name: 'orchestration.ask', + params: AskParams, + handler: async (params, { runtime }) => { + // Why: group addresses have no unambiguous answer semantics (whose + // reply wins? first? consensus?) and the ~60-LOC scope is not the + // place to design that. Rejecting here closes the silent-timeout + // footgun where a worker passing `--to @reviewers` would have the + // decision_gate inserted against a literal string no one subscribes + // to. Workers that need fan-out fall back to `send --type decision_gate`. + if (isGroupAddress(params.to)) { + throw new Error( + 'ask does not support group addresses; use send --type decision_gate for fan-out questions' + ) + } + + const db = runtime.getOrchestrationDb() + const from = params.from ?? 'unknown' + const timeoutMs = params.timeoutMs ?? 600_000 + const options = + params.options + ?.split(',') + .map((s) => s.trim()) + .filter(Boolean) ?? [] + + const payload = JSON.stringify({ question: params.question, options }) + const outbound = db.insertMessage({ + from, + to: params.to, + subject: 'Question', + body: params.question, + type: 'decision_gate', + payload + }) + runtime.deliverPendingMessagesForHandle(params.to) + runtime.notifyMessageArrived(params.to) + + const threadId = outbound.id + const deadline = Date.now() + timeoutMs + const afterSequence = outbound.sequence + + // Why: loop with a remaining-budget guard so an unrelated distractor + // message that wakes waitForMessage does not cause indefinite iteration. + // waitForMessage is handle-scoped, so we re-query by thread on every + // wake-up to separate "reply in my thread arrived" from "something + // else was delivered to this handle." + while (true) { + const replies = db.getThreadMessagesFor(threadId, from, afterSequence) + if (replies.length > 0) { + const reply = replies[0] + db.markAsRead([reply.id]) + return { + answer: reply.body, + messageId: reply.id, + threadId, + timedOut: false + } + } + const remainingMs = deadline - Date.now() + if (remainingMs <= 0) { + return { answer: null, messageId: null, threadId, timedOut: true } + } + await runtime.waitForMessage(from, { timeoutMs: remainingMs }) + } + } + }), + ...ORCHESTRATION_GATE_METHODS, defineMethod({ diff --git a/src/main/runtime/runtime-rpc.test.ts b/src/main/runtime/runtime-rpc.test.ts index 58fff491f44..77857c19abd 100644 --- a/src/main/runtime/runtime-rpc.test.ts +++ b/src/main/runtime/runtime-rpc.test.ts @@ -2,9 +2,11 @@ import { existsSync, mkdtempSync } from 'fs' import { tmpdir } from 'os' import { join } from 'path' -import { createConnection } from 'net' +import { createConnection, type Socket } from 'net' import { describe, expect, it, vi } from 'vitest' +import Database from 'better-sqlite3' import { OrcaRuntimeService } from './orca-runtime' +import { OrchestrationDb } from './orchestration/db' import * as runtimeMetadataModule from './runtime-metadata' import { readRuntimeMetadata } from './runtime-metadata' import { createRuntimeTransportMetadata, OrcaRuntimeRpcServer } from './runtime-rpc' @@ -46,6 +48,62 @@ async function sendRequest( }) } +// Why: long-poll keepalive tests need every frame, not just the first, because +// we need to count `_keepalive` frames before the terminal success/failure. +// Also exposes the socket so tests can close it mid-wait to exercise the +// long-poll counter decrement path. +type FramedSession = { + socket: Socket + frames: Record[] + done: Promise +} + +function openFramedSession(endpoint: string, request: Record): FramedSession { + const frames: Record[] = [] + const socket = createConnection(endpoint) + let buffer = '' + socket.setEncoding('utf8') + const done = new Promise((resolve, reject) => { + socket.once('error', (err) => { + // Why: ECONNRESET is expected when we deliberately destroy the socket + // mid-wait to probe the counter decrement; surface other errors. + if ((err as NodeJS.ErrnoException).code === 'ECONNRESET') { + resolve() + return + } + reject(err) + }) + socket.on('close', () => resolve()) + socket.on('data', (chunk: string) => { + buffer += chunk + let newlineIndex = buffer.indexOf('\n') + while (newlineIndex !== -1) { + const raw = buffer.slice(0, newlineIndex).trim() + buffer = buffer.slice(newlineIndex + 1) + if (raw) { + const frame = JSON.parse(raw) as Record + frames.push(frame) + // Why: the server leaves the socket open after writing the terminal + // frame (short RPCs expect the client to close); close the client + // side so `done` resolves once we've captured the response. + if (frame._keepalive !== true) { + socket.end() + } + } + newlineIndex = buffer.indexOf('\n') + } + }) + socket.on('connect', () => { + socket.write(`${JSON.stringify(request)}\n`) + }) + }) + return { socket, frames, done } +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + describe('OrcaRuntimeRpcServer', () => { const makeStore = (overrides?: { isUnread?: boolean }) => ({ getRepo: (id: string) => @@ -502,4 +560,241 @@ describe('OrcaRuntimeRpcServer', () => { await server.stop() }) + + // Why: §6 tests for the transport keepalive + long-poll counter path in §3.1. + // Exercise the real socket (not a mock) so we catch buffer/flush regressions + // that a unit-level test would miss. + describe('long-poll transport (§3.1)', () => { + it('emits keepalive frames while a check --wait handler blocks', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-runtime-rpc-')) + const runtime = new OrcaRuntimeService() + const db = new OrchestrationDb(':memory:') + runtime.setOrchestrationDb(db) + // Why: 50ms keepalive lets us collect ≥3 frames within a 300ms wait + // window without slowing the suite. + const server = new OrcaRuntimeRpcServer({ + runtime, + userDataPath, + keepaliveIntervalMs: 50 + }) + await server.start() + + try { + const metadata = readRuntimeMetadata(userDataPath) + const session = openFramedSession(metadata!.transport!.endpoint, { + id: 'req_wait', + authToken: metadata!.authToken, + method: 'orchestration.check', + params: { + terminal: 'term_nobody', + wait: true, + timeoutMs: 300 + } + }) + await session.done + + const keepalives = session.frames.filter((f) => f._keepalive === true) + const terminals = session.frames.filter((f) => f.ok !== undefined) + expect(terminals).toHaveLength(1) + expect(terminals[0]).toMatchObject({ id: 'req_wait', ok: true }) + // Why: 300ms wait with 50ms keepalive → expect roughly 5 keepalives; + // assert ≥3 to tolerate scheduler jitter without flaking. + expect(keepalives.length).toBeGreaterThanOrEqual(3) + } finally { + db.close() + await server.stop() + } + }) + + it('releases long-poll slot when client closes mid-wait', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-runtime-rpc-')) + const runtime = new OrcaRuntimeService() + const db = new OrchestrationDb(':memory:') + runtime.setOrchestrationDb(db) + const server = new OrcaRuntimeRpcServer({ + runtime, + userDataPath, + keepaliveIntervalMs: 1000, + longPollCap: 2 + }) + await server.start() + + try { + const metadata = readRuntimeMetadata(userDataPath) + const endpoint = metadata!.transport!.endpoint + + // Fill the cap with two long waits (10s each — we'll kill them). + const a = openFramedSession(endpoint, { + id: 'req_a', + authToken: metadata!.authToken, + method: 'orchestration.check', + params: { terminal: 'term_a', wait: true, timeoutMs: 10_000 } + }) + const b = openFramedSession(endpoint, { + id: 'req_b', + authToken: metadata!.authToken, + method: 'orchestration.check', + params: { terminal: 'term_b', wait: true, timeoutMs: 10_000 } + }) + // Let the two waits land in the handler and increment the counter. + await sleep(100) + expect(server['activeLongPolls']).toBe(2) + + // Kill one client mid-wait; counter must drop to 1. + a.socket.destroy() + await a.done + // Give Node one tick to fire the close event on the server socket. + await sleep(50) + expect(server['activeLongPolls']).toBe(1) + + // The freed slot must admit a new long-poll immediately. + const c = openFramedSession(endpoint, { + id: 'req_c', + authToken: metadata!.authToken, + method: 'orchestration.check', + params: { terminal: 'term_c', wait: true, timeoutMs: 100 } + }) + await c.done + const cTerminal = c.frames.find((f) => f.ok !== undefined) + expect(cTerminal).toMatchObject({ ok: true, id: 'req_c' }) + + b.socket.destroy() + await b.done + } finally { + db.close() + await server.stop() + } + }) + + it('responds runtime_busy once the long-poll cap is saturated', async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-runtime-rpc-')) + const runtime = new OrcaRuntimeService() + const db = new OrchestrationDb(':memory:') + runtime.setOrchestrationDb(db) + const server = new OrcaRuntimeRpcServer({ + runtime, + userDataPath, + keepaliveIntervalMs: 1000, + longPollCap: 1 + }) + await server.start() + + try { + const metadata = readRuntimeMetadata(userDataPath) + const endpoint = metadata!.transport!.endpoint + + const a = openFramedSession(endpoint, { + id: 'req_a', + authToken: metadata!.authToken, + method: 'orchestration.check', + params: { terminal: 'term_a', wait: true, timeoutMs: 5_000 } + }) + await sleep(100) + expect(server['activeLongPolls']).toBe(1) + + // Second long-poll overflows the cap → runtime_busy. + const overflow = await sendRequest(endpoint, { + id: 'req_overflow', + authToken: metadata!.authToken, + method: 'orchestration.check', + params: { terminal: 'term_b', wait: true, timeoutMs: 5_000 } + }) + expect(overflow).toMatchObject({ + id: 'req_overflow', + ok: false, + error: { code: 'runtime_busy' } + }) + // The failing request must not have counted against the cap. + expect(server['activeLongPolls']).toBe(1) + + // Short RPCs still succeed even when the long-poll cap is full. + const short = await sendRequest(endpoint, { + id: 'req_short', + authToken: metadata!.authToken, + method: 'status.get' + }) + expect(short).toMatchObject({ id: 'req_short', ok: true }) + + a.socket.destroy() + await a.done + } finally { + db.close() + await server.stop() + } + }) + }) + + // Why: §6 test for the idempotent + hard-fail schema migration. A broken + // migration must crash startup loudly rather than serve traffic against a + // schema missing the delivered_at column. + describe('orchestration DB migration (§3.2)', () => { + it('is idempotent when delivered_at already exists', () => { + // First open creates the column; second open should be a no-op. + const db1 = new OrchestrationDb(':memory:') + db1.close() + // File path reuse is meaningless with :memory:, so use a tmp file. + const tmpPath = join(mkdtempSync(join(tmpdir(), 'orca-orch-mig-')), 'orch.sqlite') + const a = new OrchestrationDb(tmpPath) + a.close() + // Second construction must not throw "duplicate column name". + expect(() => { + const b = new OrchestrationDb(tmpPath) + b.close() + }).not.toThrow() + }) + + it('hard-fails startup when the migration cannot be applied', () => { + // Simulate a migration error by monkey-patching better-sqlite3's exec. + // If ALTER TABLE throws for any reason (e.g. disk full, permissions), + // the constructor must propagate — not swallow and serve half-broken. + // + // Why the pre-seeded v2 DB: after the schema bundle, fresh DBs are + // initialized directly at v3 via createTables() (which already includes + // `delivered_at`), so the v2 → v3 ALTER is a no-op for new installs. + // To exercise the hard-fail path we need a DB that actually has work + // to migrate — a v2-shape file without the delivered_at column — so + // the guarded ALTER runs and the stub can fire. + const tmpPath = join(mkdtempSync(join(tmpdir(), 'orca-orch-mig-')), 'orch.sqlite') + const seed = new Database(tmpPath) + seed.exec(` + CREATE TABLE messages ( + id TEXT NOT NULL, + from_handle TEXT NOT NULL, + to_handle TEXT NOT NULL, + subject TEXT NOT NULL, + body TEXT NOT NULL DEFAULT '', + type TEXT NOT NULL DEFAULT 'status' + CHECK(type IN ( + 'status', 'dispatch', 'worker_done', 'merge_ready', + 'escalation', 'handoff', 'decision_gate', 'heartbeat' + )), + priority TEXT NOT NULL DEFAULT 'normal' + CHECK(priority IN ('normal', 'high', 'urgent')), + thread_id TEXT, + payload TEXT, + read INTEGER NOT NULL DEFAULT 0, + sequence INTEGER PRIMARY KEY AUTOINCREMENT, + created_at TEXT NOT NULL DEFAULT (datetime('now')) + ); + `) + seed.pragma('user_version = 2') + seed.close() + + const realPrototype = Database.prototype as unknown as { + exec: (sql: string) => unknown + } + const originalExec = realPrototype.exec + realPrototype.exec = function (sql: string) { + if (sql.includes('ALTER TABLE messages ADD COLUMN delivered_at')) { + throw new Error('simulated migration failure') + } + return originalExec.call(this, sql) + } + try { + expect(() => new OrchestrationDb(tmpPath)).toThrow('simulated migration failure') + } finally { + realPrototype.exec = originalExec + } + }) + }) }) diff --git a/src/main/runtime/runtime-rpc.ts b/src/main/runtime/runtime-rpc.ts index 8ebb8778736..70dbd4af1f3 100644 --- a/src/main/runtime/runtime-rpc.ts +++ b/src/main/runtime/runtime-rpc.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- Why: this file is the single security boundary for the bundled CLI — transport setup, auth-token enforcement, admission control, keepalive framing, and orphan-socket sweeping all co-locate deliberately so a reviewer can audit the boundary in one sitting. Splitting this across files would scatter the invariants without reducing complexity. */ // Why: this is the single security boundary for the bundled CLI. It owns // transport setup (unix socket / named pipe), auth-token enforcement, and // bootstrap-metadata publication so a running runtime is always discoverable @@ -19,12 +20,49 @@ type OrcaRuntimeRpcServerOptions = { userDataPath: string pid?: number platform?: NodeJS.Platform + // Why: test-only overrides for the two time-bound constants below. + // Production callers must not pass these — defaults are set by the design + // doc (§3.1) and changing them in production would weaken the admission + // fence or flood the socket with keepalive frames. + keepaliveIntervalMs?: number + longPollCap?: number } const MAX_RUNTIME_RPC_MESSAGE_BYTES = 1024 * 1024 const RUNTIME_RPC_SOCKET_IDLE_TIMEOUT_MS = 30_000 const MAX_RUNTIME_RPC_CONNECTIONS = 32 +// Why: after 10 s of a pending dispatch we emit a tiny `{"_keepalive":true}` +// frame every 10 s until the handler resolves. Each write resets both the +// server's own socket idle timer (30 s) and — once §3.1 ships on the client — +// the client's idle timer, because any byte counts as socket activity. This +// is the transport-layer fix for feedback #1: long-poll RPCs (i.e. +// orchestration.check --wait) can now run past the 30 s/60 s idle caps +// without either end tearing the socket down. See design doc §3.1. +const KEEPALIVE_INTERVAL_MS = 10_000 + +// Why: long-poll slot cap. With keepalives a `check --wait --timeout-ms +// 600000` can hold a connection for up to 10 minutes; unbounded that would +// saturate MAX_RUNTIME_RPC_CONNECTIONS (32) with 32 waiting coordinators +// and lock out normal short RPCs. Capping at half the connection budget +// leaves the other half for short traffic. On overflow the server responds +// immediately with `runtime_busy` (CLI exit 75) — fail fast, not silent +// queuing. See design doc §3.1 + §7 risk #2. +const LONG_POLL_CAP = 16 + +// Why: a long-poll request is one whose handler blocks for an unbounded +// amount of time waiting for an external event (today, only +// `orchestration.check` with `wait === true`). This function is the single +// place that classifies it — the long-poll counter, abort wiring, and +// runtime_busy admission check all share this decision. See §3.1. +function isLongPollRequest(request: RpcRequest): boolean { + if (request.method !== 'orchestration.check') { + return false + } + const params = request.params as { wait?: unknown } | undefined + return params?.wait === true +} + export class OrcaRuntimeRpcServer { private readonly runtime: OrcaRuntimeService private readonly dispatcher: RpcDispatcher @@ -32,20 +70,30 @@ export class OrcaRuntimeRpcServer { private readonly pid: number private readonly platform: NodeJS.Platform private readonly authToken = randomBytes(24).toString('hex') + private readonly keepaliveIntervalMs: number + private readonly longPollCap: number private server: Server | null = null private transport: RuntimeTransportMetadata | null = null + // Why: separate from Node's server.maxConnections because we need to count + // only long-running dispatches, not every in-flight short RPC. See §3.1 + + // §7 risk #2. + private activeLongPolls = 0 constructor({ runtime, userDataPath, pid = process.pid, - platform = process.platform + platform = process.platform, + keepaliveIntervalMs = KEEPALIVE_INTERVAL_MS, + longPollCap = LONG_POLL_CAP }: OrcaRuntimeRpcServerOptions) { this.runtime = runtime this.dispatcher = new RpcDispatcher({ runtime }) this.userDataPath = userDataPath this.pid = pid this.platform = platform + this.keepaliveIntervalMs = keepaliveIntervalMs + this.longPollCap = longPollCap } async start(): Promise { @@ -174,37 +222,129 @@ export class OrcaRuntimeRpcServer { const rawMessage = buffer.slice(0, newlineIndex).trim() buffer = buffer.slice(newlineIndex + 1) if (rawMessage) { - void this.handleMessage(rawMessage).then((response) => { - socket.write(`${JSON.stringify(response)}\n`) - }) + void this.handleRequest(socket, rawMessage) } newlineIndex = buffer.indexOf('\n') } }) } - private async handleMessage(rawMessage: string): Promise { + // Why: a single entry point per inbound request so keepalive + long-poll + // admission + AbortController wiring + response write all live in one + // place. See design doc §3.1. + private async handleRequest(socket: Socket, rawMessage: string): Promise { + const parsed = this.parseAndAuth(rawMessage) + if ('error' in parsed) { + this.safeWrite(socket, `${JSON.stringify(parsed.error)}\n`) + return + } + const request = parsed.request + + // Why: long-poll admission fence. Short RPCs bypass the counter entirely + // — it only guards handlers that can block for minutes. See §7 risk #2. + const longPoll = isLongPollRequest(request) + if (longPoll) { + if (this.activeLongPolls >= this.longPollCap) { + const busy = this.buildError( + request.id, + 'runtime_busy', + 'long-poll capacity reached; retry with backoff' + ) + this.safeWrite(socket, `${JSON.stringify(busy)}\n`) + socket.end() + return + } + this.activeLongPolls += 1 + } + + // Why: `decremented` must guard against double-decrement when both + // `close` and a post-resolve cleanup path fire. `socket.on('close')` is + // the only path that fires for every termination (normal end, destroy, + // idle timer, client kill -9, OS reset), so it carries the decrement. + // Tying it to `.finally` alone would leak a slot any time a client dies + // mid-wait because the inner waitForMessage can keep counting down for + // minutes after the socket is gone. See §3.1 counter-lifecycle. + let decremented = !longPoll + const abortController = new AbortController() + const onClose = (): void => { + if (!decremented) { + decremented = true + this.activeLongPolls = Math.max(0, this.activeLongPolls - 1) + } + abortController.abort() + } + socket.on('close', onClose) + + // Why: for long-poll requests we start a keepalive ticker after 10 s. The + // first frame at 10 s resets both the server's 30 s idle timer and the + // client's configured timeout. Short RPCs never see a keepalive — the + // ticker never fires because the handler resolves first. + let keepaliveTimer: NodeJS.Timeout | null = null + if (longPoll) { + keepaliveTimer = setInterval(() => { + if (socket.writable && !socket.destroyed) { + socket.write('{"_keepalive":true}\n') + } + }, this.keepaliveIntervalMs) + // Why: don't hold the process open solely on the keepalive interval — + // .unref() lets the event loop exit when nothing else is pending. + if (typeof keepaliveTimer.unref === 'function') { + keepaliveTimer.unref() + } + } + + try { + const response = await this.dispatcher.dispatch(request, { + signal: longPoll ? abortController.signal : undefined + }) + if (!socket.destroyed) { + this.safeWrite(socket, `${JSON.stringify(response)}\n`) + } + } finally { + if (keepaliveTimer) { + clearInterval(keepaliveTimer) + } + // Why: the close-handler path is still what decrements the counter (the + // socket may be closed by the client before the response write flushes). + // We don't remove the listener here — `once` semantics are handled by + // the boolean guard. + } + } + + private parseAndAuth(rawMessage: string): { request: RpcRequest } | { error: RpcResponse } { let request: RpcRequest try { request = JSON.parse(rawMessage) as RpcRequest } catch { - return this.buildError('unknown', 'bad_request', 'Invalid JSON request') + return { error: this.buildError('unknown', 'bad_request', 'Invalid JSON request') } } if (typeof request.id !== 'string' || request.id.length === 0) { - return this.buildError('unknown', 'bad_request', 'Missing request id') + return { error: this.buildError('unknown', 'bad_request', 'Missing request id') } } if (typeof request.method !== 'string' || request.method.length === 0) { - return this.buildError(request.id, 'bad_request', 'Missing RPC method') + return { error: this.buildError(request.id, 'bad_request', 'Missing RPC method') } } if (typeof request.authToken !== 'string' || request.authToken.length === 0) { - return this.buildError(request.id, 'unauthorized', 'Missing auth token') + return { error: this.buildError(request.id, 'unauthorized', 'Missing auth token') } } if (request.authToken !== this.authToken) { - return this.buildError(request.id, 'unauthorized', 'Invalid auth token') + return { error: this.buildError(request.id, 'unauthorized', 'Invalid auth token') } } - return this.dispatcher.dispatch(request) + return { request } + } + + private safeWrite(socket: Socket, payload: string): void { + if (socket.destroyed || !socket.writable) { + return + } + try { + socket.write(payload) + } catch { + // Socket was closed in between the writable check and the write — + // nothing we can do; the client already disconnected. + } } private buildError(id: string, code: string, message: string): RpcResponse {