merge: stale-base worktree fixes

This commit is contained in:
Jinwoo-H
2026-05-04 14:32:32 -04:00
11 changed files with 886 additions and 37 deletions
+57
View File
@@ -223,6 +223,63 @@ export async function getBaseRefDefault(path: string): Promise<string | null> {
return getDefaultBaseRefAsync(path)
}
/**
* Return { ahead, behind } for localRef vs remoteRef, or null on git failure.
*
* Why: `rev-list --left-right --count A...B` emits `<ahead>\t<behind>` —
* 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.
*
+68 -25
View File
@@ -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<CreateWorktreeResult> {
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<void> = 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(
+8 -1
View File
@@ -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 () => {
+10 -1
View File
@@ -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 () => {
+1 -1
View File
@@ -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)
}
)
+106
View File
@@ -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<string, unknown>
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)
})
})
+128 -4
View File
@@ -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<string, Set<MessageWaiter>>()
// 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 `<repoPath>::<remote>` 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<string, Promise<void>>()
private fetchLastCompletedAt = new Map<string, number>()
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<void> {
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: {
@@ -3161,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())
@@ -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<DriftResult> {
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)
})
})
+90 -2
View File
@@ -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<void> {
// 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
@@ -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)
})
})
+38 -2
View File
@@ -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<PreambleParams['baseDrift']>): 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.
---`
}