From 4df4b712889808f564d55aa8b275fbf1ef3d5c7a Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Mon, 4 May 2026 04:08:43 -0400 Subject: [PATCH] fix(worktree): prevent stale-base worktree creation and dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- 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.ts | 132 +++++++- .../runtime/orchestration/coordinator.test.ts | 313 +++++++++++++++++- src/main/runtime/orchestration/coordinator.ts | 92 ++++- .../runtime/orchestration/preamble.test.ts | 68 ++++ src/main/runtime/orchestration/preamble.ts | 40 ++- 11 files changed, 886 insertions(+), 37 deletions(-) create mode 100644 src/main/runtime/fetch-remote-cache.test.ts 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.ts b/src/main/runtime/orca-runtime.ts index 3ba6281df65..99890c07c7c 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: { @@ -3133,6 +3250,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/coordinator.test.ts b/src/main/runtime/orchestration/coordinator.test.ts index ac0ebbb4974..8d89f634d5b 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 @@ -376,4 +404,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..fe8c13f59e3 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 = { @@ -383,15 +422,64 @@ 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, + 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/preamble.test.ts b/src/main/runtime/orchestration/preamble.test.ts index 71c27d50f66..ed6fb0f6bd7 100644 --- a/src/main/runtime/orchestration/preamble.test.ts +++ b/src/main/runtime/orchestration/preamble.test.ts @@ -79,4 +79,72 @@ describe('buildDispatchPreamble', () => { 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', + 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', + 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', + 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', + 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) + }) }) diff --git a/src/main/runtime/orchestration/preamble.ts b/src/main/runtime/orchestration/preamble.ts index 3c61f0be56a..a386ae7aa20 100644 --- a/src/main/runtime/orchestration/preamble.ts +++ b/src/main/runtime/orchestration/preamble.ts @@ -3,6 +3,18 @@ export type PreambleParams = { 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: the dispatch preamble teaches agents about Orca's CLI commands for @@ -14,7 +26,7 @@ export function buildDispatchPreamble(params: PreambleParams): string { // 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 + const header = `You are working inside Orca, a multi-agent IDE. You have access to these CLI commands for communicating with the coordinator: # Report task completion (REQUIRED when done): @@ -34,8 +46,32 @@ Your assigned task ID is: ${params.taskId} 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. +escalation. Do not exit the session.` + + // 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. +---` +}