From 8a78d43a94bbaef0f270451cadc20fc80f864b6c Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 25 Aug 2026 08:52:32 +0200 Subject: [PATCH 1/6] fix(orchestrator): key the in-flight dispatch guard where dispatch writes it (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #367 reported two `FactoryLoop` fields written under one key function and read under another. The first, `#abandonedDispatchReasons`, was converged on `main` by #346 (`bffa2da`) after the issue was verified against `2f32e69` — but incidentally, with no test pinning the invariant. This lands that test pair and fixes the second field, which was still live. `#dispatchInFlight` is the per-work-unit dispatch concurrency gate. `dispatch()` writes it as `::`; orphan recovery read it as `has(issueKey(lifecycle.issue))`, which is `::` — wrong key function AND missing the suffix, so no entry could ever match and `dispatchCallActive` was unconditionally false. That guard is not decorative. At that point in `#githubOrphanRecoveryContext`, `activeIssueIdentities` is seeded only from waiting clarifications — the registry-agents pass runs after — so it is the only in-process signal that a `dispatch()` call owns the row. With it dead, a dispatch parked in `fleet.spawn` (no agent on the roster yet, row still `dispatching`) is orphan-shaped, and `#releaseOrphanedGithubLifecycle` renews the lease with our own cached epoch, so it succeeds and clears the claim underneath the live call. Match the key space the map is actually written under, as a prefix scan — the shape `stop()` already uses at `:1531`. Tests. Two must-fire/must-not-fire pairs whose arms differ in exactly one value. The abandon fence pair holds the `abandoning` save open, so the lease, the cached epoch and a nonterminal persisted row are all intact and the fence is the only thing `#dispatchLifecycleStillOwned` can decide on. The existing #303 late-placement coverage cannot see the defect: there the abandon has already completed, so `isTerminalDispatchLifecycle` answers on its own. Reverting the writer to the pre-#346 `issueKey(record.issue)` fails the must-fire with `expected [] to contain { reason: 'dispatch-released-before-placement' }` and `received [{ reason: 'issue-abandoned' }]` — the production shape: the placement accepted onto a record the reaper had already fenced. The orphan-recovery pair fails under its own ablation with `the readiness sweep never completed`: the sweep recovers the row and re-dispatches the same work unit, joining the `dispatch()` promise still parked on the spawn gate. Bounded by `withDeadline` so it arrives named rather than as a slow test. CI guard. `dispatch-keymap.test.ts` runs the #367 cross-check on the TypeScript AST under `npm test`, so it needs no workflow change: any `this.#field` accessor keyed under more than one key function fails the build. Coverage is stated rather than implied — keys arriving as parameters are unresolved and still need review by hand — and a second test asserts a floor on the resolved-field count, so a scan that stopped resolving anything cannot pass silently. No claim is made here about any production symptom. Co-Authored-By: Claude Opus 5 Session-Id: 65a2c163-9043-4fe7-9414-114e6481941c --- src/orchestrator/dispatch-keymap.test.ts | 151 ++++++++++++ src/orchestrator/factory.test.ts | 278 +++++++++++++++++++++++ src/orchestrator/factory.ts | 24 +- 3 files changed, 452 insertions(+), 1 deletion(-) create mode 100644 src/orchestrator/dispatch-keymap.test.ts diff --git a/src/orchestrator/dispatch-keymap.test.ts b/src/orchestrator/dispatch-keymap.test.ts new file mode 100644 index 00000000..a6eb6759 --- /dev/null +++ b/src/orchestrator/dispatch-keymap.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, it } from 'vitest' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import ts from 'typescript' + +/** + * A whole-file cross-check for one class of defect: a `Map`/`Set` field on + * `FactoryLoop` written under one key function and read under another. + * + * This is the scan that found #367. `#329` rekeyed the *readers* of + * `#abandonedDispatchReasons` to the provider-neutral `dispatchLifecycleKey()` + * and left the only writer on `issueKey()`. Neither side is wrong on its own, + * both compile, and the two key spaces can never collide by construction — + * `issueKey` is `::`, `dispatchLifecycleKey` is + * `github:/#` | `linear:` | `issue:` — so the write + * was simply invisible to every read. Nothing in the type system, the linter or + * a per-call-site review catches that; only comparing the whole field's + * accessors does. + * + * COVERAGE, stated plainly so a green run is not read as more than it is. Only + * arguments this scan can resolve are classified: a direct `keyFn(...)` call, a + * `const` bound to one inside the same function, or a template literal whose + * first interpolation is one (the composite `::` shape + * `dispatch()` uses, which is deliberately a *different* key space from the + * bare key). A key arriving as a function parameter is not resolved and is + * reported as unresolved rather than assumed compatible — those sites still + * need review by hand. `analysedFieldFloor` below is the must-not-fire: a scan + * that stopped resolving anything would otherwise report zero mismatches and + * pass silently. + */ + +const KEY_FNS = new Set([ + 'issueKey', + 'dispatchLifecycleKey', + 'dispatchIssueIdentity', + 'issueStateKey', + 'trackerKey', +]) +const ACCESSORS = new Set(['get', 'set', 'has', 'delete']) + +// Fields resolved on the current file. A refactor may legitimately move this +// number; a collapse towards zero means the scan stopped seeing its subject. +const ANALYSED_FIELD_FLOOR = 25 + +type KeyUse = { keyFn: string; line: number } + +const analyse = (filePath: string): { + fields: Map> + unresolved: string[] +} => { + const source = ts.createSourceFile( + filePath, + readFileSync(filePath, 'utf8'), + ts.ScriptTarget.ESNext, + true, + ) + const lineOf = (node: ts.Node): number => + source.getLineAndCharacterOfPosition(node.getStart(source)).line + 1 + + const fields = new Map>() + const unresolved: string[] = [] + + const classify = (expr: ts.Expression | undefined, scopes: Array>): string | undefined => { + if (!expr) return undefined + if (ts.isCallExpression(expr) && ts.isIdentifier(expr.expression) && KEY_FNS.has(expr.expression.text)) { + return expr.expression.text + } + if (ts.isIdentifier(expr)) { + for (let i = scopes.length - 1; i >= 0; i--) { + const bound = scopes[i]!.get(expr.text) + if (bound) return bound + } + return undefined + } + // `${keyFn(issue)}:...` is its own key space, not the bare key. Reporting + // it as distinct is exactly what surfaced #367's second field. + if (ts.isTemplateExpression(expr) && expr.head.text === '' && expr.templateSpans.length > 0) { + const first = classify(expr.templateSpans[0]!.expression, scopes) + if (first) return `${first}+composite` + } + return undefined + } + + const record = (field: string, use: KeyUse): void => { + const byKeyFn = fields.get(field) ?? new Map() + fields.set(field, byKeyFn) + const lines = byKeyFn.get(use.keyFn) ?? [] + byKeyFn.set(use.keyFn, lines) + lines.push(use.line) + } + + const walk = (node: ts.Node, scopes: Array>): void => { + let scoped = scopes + if (ts.isFunctionLike(node)) scoped = [...scopes, new Map()] + + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { + const keyFn = classify(node.initializer, scoped) + if (keyFn) scoped[scoped.length - 1]!.set(node.name.text, keyFn) + } + + if ( + ts.isCallExpression(node) && + ts.isPropertyAccessExpression(node.expression) && + ACCESSORS.has(node.expression.name.text) && + ts.isPropertyAccessExpression(node.expression.expression) && + node.expression.expression.expression.kind === ts.SyntaxKind.ThisKeyword && + ts.isPrivateIdentifier(node.expression.expression.name) + ) { + const field = node.expression.expression.name.text + const arg = node.arguments[0] + const keyFn = classify(arg, scoped) + if (keyFn) record(field, { keyFn, line: lineOf(node) }) + else if (arg) unresolved.push(`${field} @${lineOf(node)}`) + } + + ts.forEachChild(node, (child) => walk(child, scoped)) + } + + walk(source, [new Map()]) + return { fields, unresolved } +} + +describe('FactoryLoop keyed-field cross-check (#367)', () => { + const factoryPath = fileURLToPath(new URL('./factory.ts', import.meta.url)) + const { fields, unresolved } = analyse(factoryPath) + + it('keys every resolvable field of FactoryLoop under exactly one key function', () => { + const mismatched = [...fields] + .filter(([, byKeyFn]) => byKeyFn.size > 1) + .map(([field, byKeyFn]) => ({ + field, + keys: Object.fromEntries([...byKeyFn].map(([keyFn, lines]) => [keyFn, lines.sort((a, b) => a - b)])), + })) + .sort((a, b) => a.field.localeCompare(b.field)) + + expect(mismatched).toEqual([]) + }) + + // MUST-NOT-FIRE for the check above. Without this, deleting `KEY_FNS` — or + // any change that makes `classify` stop resolving — turns the assertion into + // a vacuous pass while the invariant it guards goes unenforced. + it('still resolves the fields it is meant to be checking', () => { + expect(fields.size).toBeGreaterThanOrEqual(ANALYSED_FIELD_FLOOR) + expect([...fields.keys()]).toContain('#abandonedDispatchReasons') + expect([...fields.keys()]).toContain('#dispatchInFlight') + // The unresolved sites are a known blind spot, not a silent one: keys that + // arrive as parameters are reviewed by hand. Asserting the bucket exists + // keeps that limitation visible in the test output rather than implied. + expect(unresolved.length).toBeGreaterThan(0) + }) +}) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 97d3c411..d27e5f46 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -14215,6 +14215,284 @@ describe('FactoryLoop', () => { } }, 40_000) + /** + * #367. `#abandonedDispatchReasons` is the FIRST thing + * `#dispatchLifecycleStillOwned` consults, and during the window between the + * reaper fencing a dispatch and the durable row actually reaching a terminal + * phase it is the ONLY signal that the dispatch is gone. #329 rekeyed all + * three readers of that map to the provider-neutral `dispatchLifecycleKey` + * and left the only writer on `issueKey`, whose key space it can never + * collide with — `issueKey` is `::`, the lifecycle key is + * `github:/#` | `linear:` | `issue:`. Throughout + * that window the predicate therefore answered "still owned" for a dispatch + * that had been explicitly abandoned, and a placement landing inside it was + * recorded onto a record the reaper had already fenced and would never + * release. + * + * The #303 late-placement coverage above cannot see this: there the abandon + * has already run to completion, so `isTerminalDispatchLifecycle` answers on + * its own and the fence is never reached. These two arms hold the + * `abandoning` save open instead, which keeps the lease, the cached epoch and + * a nonterminal persisted row all intact — every other arm of the predicate + * says "owned", so the fence is the only thing that can decide. + * + * The arms differ in exactly one value: `agentlessHoldTimeoutMs`. Swapping + * them fails both. + */ + describe('the abandon fence and the ownership predicate share a key space (#367)', () => { + const abandonFenceFixture = async (opts: { agentlessHoldTimeoutMs: number; number: number }) => { + const root = await mkdtemp(join(tmpdir(), 'factory-abandon-fence-')) + const watchStatePath = join(root, 'state.json') + const state = () => new FileStateStore({ batchSize: 1, watchStatePath }) + const spawnGate = Promise.withResolvers() + const spawnStarted = Promise.withResolvers() + const abandoningReached = Promise.withResolvers() + const abandoningGate = Promise.withResolvers() + + class HangingSpawnFleetClient extends RemoteLifecycleFleetClient { + override async spawn(input: SpawnInput): Promise { + spawnStarted.resolve() + await spawnGate.promise + return await super.spawn(input) + } + } + + // Parks the lifecycle inside the `abandoning` write. On the must-fire arm + // that is what holds the fence installed while the row is still + // `dispatching` and still leased to us. On the control arm nothing ever + // reaches this branch — the two arms are otherwise identical so that the + // deadline is the only variable. + class PausedAbandonStateStore extends FileStateStore { + override async saveDispatchLifecycle( + workspaceId: string, + key: string, + owner: string, + epoch: number, + nowMs: number, + lifecycle: DispatchLifecycle, + ): Promise { + if (lifecycle.phase === 'abandoning') { + abandoningReached.resolve() + await abandoningGate.promise + } + return await super.saveDispatchLifecycle(workspaceId, key, owner, epoch, nowMs, lifecycle) + } + } + + const fleet = new HangingSpawnFleetClient() + const factory = createFactory(config({ + batchSize: 1, + // The ordinary held-agent deadline stays hours away in both arms: only + // the never-placed deadline can fence this dispatch. + dispatch: { agentHoldTimeoutMs: 4 * 60 * 60_000, agentlessHoldTimeoutMs: opts.agentlessHoldTimeoutMs }, + loop: { heartbeatPath: join(root, 'heartbeat.json'), registryPath: join(root, 'registry.json') }, + }), { + mount: new FakeMountClient({ [issuePath(opts.number)]: issueFile(opts.number) }), + fleet, + stateStore: new PausedAbandonStateStore({ batchSize: 1, watchStatePath }), + triage: new StaticTriage(), + }) + return { root, state, fleet, factory, spawnGate, spawnStarted, abandoningReached, abandoningGate } + } + + // MUST FIRE. Before the fix this asserted `false` — the fence was written + // where no reader could see it, so the late placement was accepted onto a + // fenced record and never released. + it('releases a late placement on the fence alone, before the row goes terminal', async () => { + const { + root, state, fleet, factory, spawnGate, spawnStarted, abandoningReached, abandoningGate, + } = await abandonFenceFixture({ agentlessHoldTimeoutMs: 1_000, number: 367 }) + const dispatched = factory + .triageIssue(parseLinearIssue(issuePath(367), issueFile(367))) + .then((decision) => factory.dispatch(decision)) + .catch(() => undefined) + try { + await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') + // The reaper has now fenced this dispatch and is parked inside its + // `abandoning` write. + await withDeadline(abandoningReached.promise, 20_000, 'the abandoning save was never reached') + + // The precondition that makes this a test of the fence and nothing + // else: the persisted row is still nonterminal, so every other arm of + // `#dispatchLifecycleStillOwned` still answers "owned". + const key = dispatchIssueIdentity(parseLinearIssue(issuePath(367), issueFile(367))) + expect(await state().getDispatchLifecycle('factory-test', key)).toMatchObject({ phase: 'dispatching' }) + + // The placement now lands inside that window. + spawnGate.resolve() + await vi.waitFor(() => expect(fleet.releases).toContainEqual({ + name: 'ar-367-impl-pear', + reason: 'dispatch-released-before-placement', + }), { timeout: 10_000 }) + expect(factory.status().counters.lateSpawnPlacementsReleased).toBeGreaterThanOrEqual(1) + } finally { + spawnGate.resolve() + abandoningGate.resolve() + await dispatched + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }, 60_000) + + // MUST NOT FIRE, and the control for the arm above. Same fixture, same + // late-returning spawn, same paused-abandon state store — only the + // never-placed deadline moves out of reach, so nothing fences this + // dispatch. The placement must be accepted. Without this arm, making + // `#dispatchLifecycleStillOwned` return `false` unconditionally would + // satisfy the must-fire. + it('CONTROL: accepts the same late placement when nothing abandoned the dispatch', async () => { + const { + root, state, fleet, factory, spawnGate, spawnStarted, abandoningGate, + } = await abandonFenceFixture({ agentlessHoldTimeoutMs: 60 * 60_000, number: 368 }) + const dispatched = factory + .triageIssue(parseLinearIssue(issuePath(368), issueFile(368))) + .then((decision) => factory.dispatch(decision)) + try { + await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') + const key = dispatchIssueIdentity(parseLinearIssue(issuePath(368), issueFile(368))) + // The same never-placed shape the must-fire arm reaps: a planned spec + // with no result yet, and no `heldSinceAtMs`. + await vi.waitFor(async () => { + const pending = await state().getDispatchLifecycle('factory-test', key) + expect(pending?.phase).toBe('dispatching') + expect(pending?.heldSinceAtMs).toBeUndefined() + expect(pending?.agents.map((agent) => agent.tracked.result)).toEqual([undefined]) + }, { timeout: 10_000 }) + + spawnGate.resolve() + await dispatched + expect(fleet.spawns.map((spawn) => spawn.name)).toEqual(['ar-368-impl-pear', 'ar-368-review']) + expect(fleet.releases).toEqual([]) + expect(factory.status().counters.lateSpawnPlacementsReleased).toBeUndefined() + expect(factory.status().counters.agentlessSlotPastDeadlineReleases).toBeUndefined() + await vi.waitFor(async () => expect(await state().getDispatchLifecycle('factory-test', key)) + .toMatchObject({ phase: 'running' }), { timeout: 10_000 }) + } finally { + spawnGate.resolve() + abandoningGate.resolve() + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }, 60_000) + }) + + /** + * #367, second field. `#dispatchInFlight` is the per-work-unit dispatch + * concurrency gate, and orphan recovery consults it to answer "is a + * `dispatch()` call for this lifecycle running right now?" before it may + * class the row as an abandoned claim. Its entries are + * `::`; the read was + * `has(issueKey(issue))`, which is `::` — unsatisfiable by + * construction, so the guard was dead and a dispatch that was still mid-spawn + * could have its own claim released underneath it. + * + * The arms differ only in whether that dispatch call is still in flight. + */ + describe('orphan recovery sees a dispatch that is still in flight (#367)', () => { + const inFlightFixture = async (number: number) => { + const root = await mkdtemp(join(tmpdir(), 'factory-orphan-inflight-')) + const path = githubIssuePath('AgentWorkforce', 'pear', number) + const ready = githubIssueFile(number, { labels: ['factory', 'pear'] }) + const mount = new FakeMountClient({ [path]: ready }) + mount.setSubRoot('/linear/issues', 'absent') + const spawnGate = Promise.withResolvers() + const spawnStarted = Promise.withResolvers() + class HangingSpawnFleetClient extends RemoteLifecycleFleetClient { + override async spawn(input: SpawnInput): Promise { + spawnStarted.resolve() + await spawnGate.promise + return await super.spawn(input) + } + } + const fleet = new HangingSpawnFleetClient() + const factory = createFactory(config({ + issueSource: 'github', + // Nothing may reap this row on a deadline: the orphan-recovery + // classification is the only thing under test here. + dispatch: { agentHoldTimeoutMs: 4 * 60 * 60_000, agentlessHoldTimeoutMs: 4 * 60 * 60_000 }, + loop: { registryPath: join(root, 'registry.json') }, + }), { + mount, + fleet, + stateStore: new InMemoryStateStore({ batchSize: 4 }), + triage: new StaticTriage(), + githubWriteback: new RecordingGithubWriteback(), + probePrGhRunner: async () => ({ stdout: '[]' }), + }) + // The dispatch claims the issue on the provider; the fake mount is not + // written through by the writeback, so stamp the resulting + // `factory:in-progress` projection here. Done only once the spawn has + // started, so it cannot race the dispatch's own live re-read. + const markInProgress = (): void => { + mount.files.set(path, { + content: githubIssueFile(number, { labels: ['factory', 'pear', 'factory:in-progress'] }), + }) + } + const startDispatch = (): Promise => factory + .triageIssue(parseGithubFactoryIssue(path, ready)) + .then((decision) => factory.dispatch(decision)) + return { root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch } + } + + // MUST NOT FIRE. The claim belongs to a `dispatch()` this process is still + // inside. Before the fix the guard could not see it and the row was + // classed as an orphan. + it('preserves the claim of a dispatch whose spawn has not returned', async () => { + const { + root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch, + } = await inFlightFixture(369) + const dispatched = startDispatch().catch(() => undefined) + try { + await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') + markInProgress() + + // Bounded on purpose. With the guard dead this sweep classes the row + // as an orphan, recovers it, and re-dispatches the same work unit — + // which joins the very `dispatch()` promise that is still parked on + // the spawn gate, so the failure arrives as a hang rather than an + // assertion. Name it instead of letting it read as a slow test. + await withDeadline(factory.runOnce(), 15_000, 'the readiness sweep never completed') + + expect(factory.status().counters.githubOrphanedLifecycleClaimsReleased).toBeUndefined() + expect(fleet.releases).toEqual([]) + } finally { + spawnGate.resolve() + await dispatched + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }, 40_000) + + // MUST FIRE, and the control for the arm above. Identical fixture and an + // identical orphan-shaped row — the dispatch simply is not in flight any + // more. Without this arm the assertion above would pass for a fixture that + // never reaches the orphan-recovery call site at all. + it('CONTROL: still releases the same claim once no dispatch is in flight', async () => { + const { + root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch, + } = await inFlightFixture(370) + try { + const dispatched = startDispatch().catch(() => undefined) + await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') + spawnGate.resolve() + await dispatched + markInProgress() + // Take the placements back off the roster so the row is agent-less + // again: the same shape the arm above holds, minus the live call. + for (const spawn of [...fleet.spawns]) await fleet.release(spawn.name, 'test-teardown') + fleet.releases.length = 0 + + await factory.runOnce() + + expect(factory.status().counters.githubOrphanedLifecycleClaimsReleased).toBe(1) + } finally { + spawnGate.resolve() + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }, 40_000) + }) + // #303 must-not-fire control. The window between `promoteDispatchLifecycle` // and the first `recordSpawn` legitimately has zero agents. Reaping it on // sight would convert a wedge into a dispatch race. diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 73150a74..3d34d877 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -4103,7 +4103,7 @@ export class FactoryLoop implements Factory { const activeAgents = lifecycle.agents.filter((agent) => agent.releasedAtMs === undefined) const hasLiveAgent = activeAgents.some((agent) => onlineAgents.has(agent.name)) const exitRecoveryActive = activeAgents.some((agent) => this.#agentExitsInFlight.has(agent.name)) - const dispatchCallActive = this.#dispatchInFlight.has(issueKey(lifecycle.issue)) + const dispatchCallActive = this.#hasDispatchCallInFlight(lifecycle.issue) // The provider's `factory:in-progress` transition happens immediately // before the durable lifecycle advances from dispatching to running. // A crash can therefore leave any nonterminal phase behind while the @@ -5003,6 +5003,28 @@ export class FactoryLoop implements Factory { } } + /** + * Is a `dispatch()` call for this work unit running in this process right now? + * + * `#dispatchInFlight` is keyed by work unit *plus* the dry-run and phase the + * call was made under, so membership is a prefix match on the same + * provider-neutral identity — the shape `stop()` already uses to find the + * dispatches a claim fence belongs to. + * + * #367: this was `#dispatchInFlight.has(issueKey(issue))`, which no key in + * the map can ever equal. `issueKey` is `::`, while every + * entry is `::` — so the guard was + * unsatisfiable by construction and orphan recovery could class a lifecycle + * as abandoned while its own dispatch was still mid-flight. + */ + #hasDispatchCallInFlight(issue: IssueRef): boolean { + const prefix = `${dispatchLifecycleKey(issue)}:` + for (const key of this.#dispatchInFlight.keys()) { + if (key.startsWith(prefix)) return true + } + return false + } + async #dispatchUnlocked(decision: TriageDecision, opts: { dryRun?: boolean; labelsValidated?: boolean } = {}): Promise { const dryRun = opts.dryRun ?? this.#config.dryRun const batch = await this.#batch() From 658032b85d41075c6fee52e7cc593a69d5fd2d00 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 25 Aug 2026 09:09:21 +0200 Subject: [PATCH 2/6] fix(orchestrator): restrict the dispatch guard to the live dispatch phase (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #369 review, codex (P2). The prefix scan matched every `#dispatchInFlight` entry for the work unit, including `:dry-run:*` and `:live:escalation`. Neither can own the durable lifecycle the guard protects: `durableDispatch` is `!dryRun && …` (`:5140`), and an escalation returns from `#dispatchUnlocked` at `:5109`, before any lifecycle is created. Matching them would preserve a genuinely orphaned `factory:in-progress` claim for a call that cannot own it, until a later sweep. Look up the exact key `dispatch()` writes instead. That also puts the read back inside the keymap cross-check's resolvable set, so the field is now covered on both sides rather than only on its writes. The ablation is unchanged: reverting to the `issueKey` read still fails `preserves the claim of a dispatch whose spawn has not returned` with `the readiness sweep never completed`. Co-Authored-By: Claude Opus 5 Session-Id: 65a2c163-9043-4fe7-9414-114e6481941c --- src/orchestrator/factory.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 3d34d877..78e7fed3 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -5004,12 +5004,18 @@ export class FactoryLoop implements Factory { } /** - * Is a `dispatch()` call for this work unit running in this process right now? + * Is a `dispatch()` call that could own this work unit's durable lifecycle + * running in this process right now? * - * `#dispatchInFlight` is keyed by work unit *plus* the dry-run and phase the - * call was made under, so membership is a prefix match on the same - * provider-neutral identity — the shape `stop()` already uses to find the - * dispatches a claim fence belongs to. + * `#dispatchInFlight` is keyed by work unit *plus* the dry-run flag and phase + * the call was made under, so this must be built from the same three parts + * `dispatch()` writes rather than from the bare identity. + * + * Only the live dispatch phase qualifies. A dry run never claims a lifecycle + * at all (`durableDispatch` is `!dryRun && …`), and an escalation returns from + * `#dispatchUnlocked` before one is created — so matching those would preserve + * a genuinely orphaned claim for a call that cannot own it (#369 review, + * codex). * * #367: this was `#dispatchInFlight.has(issueKey(issue))`, which no key in * the map can ever equal. `issueKey` is `::`, while every @@ -5018,11 +5024,7 @@ export class FactoryLoop implements Factory { * as abandoned while its own dispatch was still mid-flight. */ #hasDispatchCallInFlight(issue: IssueRef): boolean { - const prefix = `${dispatchLifecycleKey(issue)}:` - for (const key of this.#dispatchInFlight.keys()) { - if (key.startsWith(prefix)) return true - } - return false + return this.#dispatchInFlight.has(`${dispatchLifecycleKey(issue)}:live:dispatch`) } async #dispatchUnlocked(decision: TriageDecision, opts: { dryRun?: boolean; labelsValidated?: boolean } = {}): Promise { From fcd95a97dcdf9dc52de9a32b752ef854258ad084 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 25 Aug 2026 09:24:04 +0200 Subject: [PATCH 3/6] fix(orchestrator): match both live phases in the dispatch guard (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #369 review, cubic (P1), and it is the counter-case to the codex P2 that narrowed this to `:live:dispatch` in 658032b. The phase half of a `#dispatchInFlight` key is not stable across the two functions that produce and consume it. `dispatch()` derives it from the INCOMING decision (`:4983`), while `#dispatchUnlocked` re-derives the escalation reason from the POST-ROUTING decision (`:5109`) — and `authoritativeRoutedDecision` (`:19321`) upgrades a routeless `confidence: 'low'` triage to `'high'` once the live labels resolve a repository. A call keyed `:live:escalation` therefore falls through the escalation return and creates a lifecycle, so matching only `:live:dispatch` reopened #367 for exactly that path. The dry-run half IS stable and `durableDispatch` is `!dryRun && …`, so codex's exclusion of `:dry-run:*` still holds and is kept. Live-only, both phases — the `\`${key}:live:\`` prefix `stop()` already uses at `:1531`. Adds the must-fire for the transition, driven through the real `dispatch()` path with a routeless low-confidence triage: reaching the spawn at all is what proves the upgrade happened, since an un-upgraded escalation returns above it. It fails with `the readiness sweep never completed` against the `:live:dispatch` version, and both in-flight arms fail against the original `issueKey` read. Note against 658032b's message: the prefix scan is not a `Map` accessor, so `dispatch-keymap.test.ts` covers this field on its writes only. The three behavioural arms are what protect the read. Co-Authored-By: Claude Opus 5 Session-Id: 65a2c163-9043-4fe7-9414-114e6481941c --- src/orchestrator/factory.test.ts | 50 ++++++++++++++++++++++++++++++-- src/orchestrator/factory.ts | 26 +++++++++++++---- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index d27e5f46..7480a3a8 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -14389,7 +14389,17 @@ describe('FactoryLoop', () => { * The arms differ only in whether that dispatch call is still in flight. */ describe('orphan recovery sees a dispatch that is still in flight (#367)', () => { - const inFlightFixture = async (number: number) => { + // Low confidence and NO routes. `labelDerivedDispatchDecision` resolves the + // repository from the live `pear` label, and `authoritativeRoutedDecision` + // then upgrades this to `confidence: 'high'` — which is precisely how a call + // keyed `:live:escalation` by `dispatch()` goes on to create a lifecycle. + class RoutelessLowConfidenceTriage extends StaticTriage { + override async triage(issue: LinearIssue): Promise { + return { ...await super.triage(issue), routes: [], confidence: 'low' } + } + } + + const inFlightFixture = async (number: number, triage: TriageEngine = new StaticTriage()) => { const root = await mkdtemp(join(tmpdir(), 'factory-orphan-inflight-')) const path = githubIssuePath('AgentWorkforce', 'pear', number) const ready = githubIssueFile(number, { labels: ['factory', 'pear'] }) @@ -14415,7 +14425,7 @@ describe('FactoryLoop', () => { mount, fleet, stateStore: new InMemoryStateStore({ batchSize: 4 }), - triage: new StaticTriage(), + triage, githubWriteback: new RecordingGithubWriteback(), probePrGhRunner: async () => ({ stdout: '[]' }), }) @@ -14491,6 +14501,42 @@ describe('FactoryLoop', () => { await rm(root, { recursive: true, force: true }) } }, 40_000) + + // MUST NOT FIRE, for the phase half of the key specifically. `dispatch()` + // picks the phase from the INCOMING decision (`:4983`), while + // `#dispatchUnlocked` re-derives the escalation reason from the + // POST-ROUTING decision (`:5109`) — and `authoritativeRoutedDecision` + // (`:19321`) upgrades a routeless `confidence: 'low'` triage to `'high'` + // once the live labels resolve a repository. So a call keyed + // `:live:escalation` does reach lifecycle creation, and a guard that + // matched only `:live:dispatch` would reopen #367 for it. + // + // The CONTROL above is shared: it is the same fixture with no call in + // flight, and it still releases. + it('preserves the claim of a dispatch keyed for escalation before routing upgraded it', async () => { + const { + root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch, + } = await inFlightFixture(371, new RoutelessLowConfidenceTriage()) + const dispatched = startDispatch().catch(() => undefined) + try { + await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') + markInProgress() + + // Reaching the spawn at all is what proves the phase transition + // happened: an escalation returns from `#dispatchUnlocked` above the + // spawn, so a decision still carrying `confidence: 'low'` here would + // never have placed an agent. + await withDeadline(factory.runOnce(), 15_000, 'the readiness sweep never completed') + + expect(factory.status().counters.githubOrphanedLifecycleClaimsReleased).toBeUndefined() + expect(fleet.releases).toEqual([]) + } finally { + spawnGate.resolve() + await dispatched + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }, 40_000) }) // #303 must-not-fire control. The window between `promoteDispatchLifecycle` diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 78e7fed3..f5a36502 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -5011,11 +5011,21 @@ export class FactoryLoop implements Factory { * the call was made under, so this must be built from the same three parts * `dispatch()` writes rather than from the bare identity. * - * Only the live dispatch phase qualifies. A dry run never claims a lifecycle - * at all (`durableDispatch` is `!dryRun && …`), and an escalation returns from - * `#dispatchUnlocked` before one is created — so matching those would preserve - * a genuinely orphaned claim for a call that cannot own it (#369 review, - * codex). + * Live only, but BOTH phases — the same `` `${key}:live:` `` prefix `stop()` + * uses at `:1531`, and for the same reason. + * + * The dry-run half of the key is decided once and is stable across both + * functions, and `durableDispatch` is `!dryRun && …`, so a `:dry-run:*` call + * provably never claims a lifecycle and matching it would preserve a + * genuinely orphaned claim (#369 review, codex). + * + * The phase half is NOT stable. `dispatch()` derives it from the incoming + * decision, while `#dispatchUnlocked` re-derives the escalation reason from + * the post-routing decision — and `authoritativeRoutedDecision` upgrades a + * routeless `confidence: 'low'` triage to `'high'` when the live labels + * resolve a repository. A call keyed `:live:escalation` therefore does reach + * lifecycle creation, so excluding that phase would reopen exactly the defect + * below for it (#369 review, cubic). * * #367: this was `#dispatchInFlight.has(issueKey(issue))`, which no key in * the map can ever equal. `issueKey` is `::`, while every @@ -5024,7 +5034,11 @@ export class FactoryLoop implements Factory { * as abandoned while its own dispatch was still mid-flight. */ #hasDispatchCallInFlight(issue: IssueRef): boolean { - return this.#dispatchInFlight.has(`${dispatchLifecycleKey(issue)}:live:dispatch`) + const livePrefix = `${dispatchLifecycleKey(issue)}:live:` + for (const key of this.#dispatchInFlight.keys()) { + if (key.startsWith(livePrefix)) return true + } + return false } async #dispatchUnlocked(decision: TriageDecision, opts: { dryRun?: boolean; labelsValidated?: boolean } = {}): Promise { From 9bfa58155dd7b97f0ab4ea6eaffcaacd50153c21 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 25 Aug 2026 09:56:04 +0200 Subject: [PATCH 4/6] test(orchestrator): give the escalation-keyed arm its own control (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #369 review, cubic (P3), and the comment it flags was wrong: the escalation must-not-fire did NOT share the existing control. That control runs `StaticTriage` and keys its dispatch `:live:dispatch`, while the escalation arm runs `RoutelessLowConfidenceTriage` and keys `:live:escalation` — a different triage and a different key phase. The hole that leaves: an orphan sweep that silently skipped escalation-keyed lifecycles altogether would release nothing, and the escalation must-not-fire would go green on an assertion that nothing was released. Only a positive control on the same triage rules that out. The CONTROL is now `it.each` over both phases — `dispatch-keyed` (370) and `escalation-keyed` (372) — so each must-not-fire is paired with a must-fire built from the same triage. Verified non-vacuous: ablating the guard to `return true` fails BOTH controls. Co-Authored-By: Claude Opus 5 Session-Id: 65a2c163-9043-4fe7-9414-114e6481941c --- src/orchestrator/factory.test.ts | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 7480a3a8..b46608a3 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -14473,14 +14473,23 @@ describe('FactoryLoop', () => { } }, 40_000) - // MUST FIRE, and the control for the arm above. Identical fixture and an - // identical orphan-shaped row — the dispatch simply is not in flight any - // more. Without this arm the assertion above would pass for a fixture that - // never reaches the orphan-recovery call site at all. - it('CONTROL: still releases the same claim once no dispatch is in flight', async () => { + // MUST FIRE, and the control for BOTH must-not-fire arms — one per key + // phase. Identical fixture and an identical orphan-shaped row; the dispatch + // simply is not in flight any more. + // + // The escalation arm needs its own control rather than borrowing the + // dispatch one (#369 review, cubic). The two use different triages and so + // produce different key phases, and an orphan sweep that silently skipped + // escalation-keyed lifecycles altogether would green the escalation + // must-not-fire while releasing nothing. Only a positive control on the + // same triage rules that out. + it.each([ + { label: 'dispatch-keyed', number: 370, triage: () => new StaticTriage() }, + { label: 'escalation-keyed', number: 372, triage: () => new RoutelessLowConfidenceTriage() }, + ])('CONTROL: still releases the same $label claim once no dispatch is in flight', async ({ number, triage }) => { const { root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch, - } = await inFlightFixture(370) + } = await inFlightFixture(number, triage()) try { const dispatched = startDispatch().catch(() => undefined) await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') @@ -14511,8 +14520,9 @@ describe('FactoryLoop', () => { // `:live:escalation` does reach lifecycle creation, and a guard that // matched only `:live:dispatch` would reopen #367 for it. // - // The CONTROL above is shared: it is the same fixture with no call in - // flight, and it still releases. + // Paired with the `escalation-keyed` CONTROL above, which uses this same + // triage and does release once the call is no longer in flight — so a + // sweep that simply ignored escalation-keyed rows cannot green this. it('preserves the claim of a dispatch keyed for escalation before routing upgraded it', async () => { const { root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch, From 67da8da45308dd925bd623fd4d112c26ce31ef85 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 25 Aug 2026 10:09:08 +0200 Subject: [PATCH 5/6] test(orchestrator): close two blind spots in the keymap cross-check (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #369 review, CodeRabbit. Both are soundness gaps in the guard itself, which matters more than usual here: the whole point of #367 item 5 is a check that does not silently miss things. `ACCESSORS` omitted `add`, so a `Set` written with `.add(k1)` and tested with `.has(k2)` was invisible — the same defect class in a different container. Including it resolves 11 more sites (156 -> 167) and reports no new mismatches, so there is no hidden Set defect today, but the hole is closed. The binding resolver recorded every `VariableDeclaration` initializer and does not track assignments, so a reassigned `let`/`var` kept vouching for a key function it no longer held — a false negative. Restricted to `const`. Measured cost: zero bindings lost, because every key binding in factory.ts is already `const`. Guard re-verified against the defect it exists for: ablating `#hasDispatchCallInFlight` back to `has(issueKey(issue))` fails the check and names `issueKey` against `dispatchLifecycleKey+composite`. Co-Authored-By: Claude Opus 5 Session-Id: 65a2c163-9043-4fe7-9414-114e6481941c --- src/orchestrator/dispatch-keymap.test.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/orchestrator/dispatch-keymap.test.ts b/src/orchestrator/dispatch-keymap.test.ts index a6eb6759..f7ac4703 100644 --- a/src/orchestrator/dispatch-keymap.test.ts +++ b/src/orchestrator/dispatch-keymap.test.ts @@ -36,7 +36,9 @@ const KEY_FNS = new Set([ 'issueStateKey', 'trackerKey', ]) -const ACCESSORS = new Set(['get', 'set', 'has', 'delete']) +// `add` included so a `Set` written under one key function and tested under +// another is caught too, not just a `Map` (#369 review, CodeRabbit). +const ACCESSORS = new Set(['get', 'set', 'has', 'delete', 'add']) // Fields resolved on the current file. A refactor may legitimately move this // number; a collapse towards zero means the scan stopped seeing its subject. @@ -93,7 +95,17 @@ const analyse = (filePath: string): { let scoped = scopes if (ts.isFunctionLike(node)) scoped = [...scopes, new Map()] - if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { + // `const` only. A `let`/`var` can be reassigned after its initializer, and + // this scan does not track assignments — carrying the initializer's key + // function past a reassignment would let it vouch for a binding that no + // longer holds that key, hiding a genuine mismatch (#369 review, + // CodeRabbit). + if ( + ts.isVariableDeclaration(node) && + ts.isIdentifier(node.name) && + ts.isVariableDeclarationList(node.parent) && + (node.parent.flags & ts.NodeFlags.Const) !== 0 + ) { const keyFn = classify(node.initializer, scoped) if (keyFn) scoped[scoped.length - 1]!.set(node.name.text, keyFn) } From 61a251b6edad1bb5c50ce5025a21eaf0ca8396ac Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 25 Aug 2026 10:22:58 +0200 Subject: [PATCH 6/6] test(orchestrator): drain the control arm's dispatch in teardown (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #369 review, CodeRabbit (review-body nitpick). The `it.each` control declared `dispatched` inside its `try`, so `finally` could not drain it — the one arm of the three that did not. A throw at the `withDeadline` above it left that dispatch running across `factory.stop()` and `rm(root)`, and its late rejection would then surface from teardown instead of the assertion that actually failed. Hoisted and awaited in `finally`, matching the two must-not-fire arms. The `.catch` stays so the drain itself cannot throw over a real failure; the `githubOrphanedLifecycleClaimsReleased` assertion is what proves the dispatch landed a lifecycle. All three arms now share one shape: hoist, drain in finally. Co-Authored-By: Claude Opus 5 Session-Id: 65a2c163-9043-4fe7-9414-114e6481941c --- src/orchestrator/factory.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index b46608a3..a3f7ddf7 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -14490,8 +14490,15 @@ describe('FactoryLoop', () => { const { root, fleet, factory, spawnGate, spawnStarted, markInProgress, startDispatch, } = await inFlightFixture(number, triage()) + // Hoisted so `finally` can drain it. Left inside the `try`, a throw at + // the deadline below would leave this dispatch running across + // `factory.stop()` and `rm(root)`, and its late rejection would surface + // from teardown instead of the assertion that actually failed (#369 + // review, CodeRabbit). `.catch` keeps that drain from throwing here for + // the same reason; the assertion below is what proves the dispatch + // actually landed a lifecycle. + const dispatched = startDispatch().catch(() => undefined) try { - const dispatched = startDispatch().catch(() => undefined) await withDeadline(spawnStarted.promise, 12_000, 'the spawn was never entered') spawnGate.resolve() await dispatched @@ -14506,6 +14513,7 @@ describe('FactoryLoop', () => { expect(factory.status().counters.githubOrphanedLifecycleClaimsReleased).toBe(1) } finally { spawnGate.resolve() + await dispatched await factory.stop() await rm(root, { recursive: true, force: true }) }