From 4beaccbd872ca34ed3d91e41e08a143b97255ab3 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 01:56:15 -0400 Subject: [PATCH 01/30] request CodeRabbit review on opened PRs --- src/cli/fleet.test.ts | 15 + src/cli/fleet.ts | 11 +- src/github/review-request.test.ts | 42 +++ src/github/review-request.ts | 18 ++ src/index.ts | 1 + .../relayfile-cloud-mount-client.test.ts | 184 +++++++++++ src/mount/relayfile-cloud-mount-client.ts | 62 +++- .../relayfile-github-connection-write.test.ts | 302 ++++++++++++++++++ .../relayfile-github-connection-write.ts | 177 +++++++++- src/orchestrator/factory.test.ts | 38 ++- src/orchestrator/factory.ts | 91 +++++- src/ports/index.ts | 1 + src/ports/mount.ts | 14 +- src/ports/writeback.ts | 4 +- src/writeback/github.ts | 49 +++ src/writeback/writeback.test.ts | 83 +++++ 16 files changed, 1062 insertions(+), 30 deletions(-) create mode 100644 src/github/review-request.test.ts create mode 100644 src/github/review-request.ts diff --git a/src/cli/fleet.test.ts b/src/cli/fleet.test.ts index 9a7caeb..6743aa7 100644 --- a/src/cli/fleet.test.ts +++ b/src/cli/fleet.test.ts @@ -2050,6 +2050,21 @@ describe('fleet CLI runtime', () => { { ref: 'refs/heads/factory/77', sha: 'abc123' }, { guarded: true }, )).toBe(false) + expect(await opts?.isAllowedDraft?.( + `/github/repos/${input.repo}/pulls/42/comments/factory-coderabbit-review.json`, + { body: '@coderabbitai review\n' }, + { guarded: true }, + )).toBe(true) + expect(await opts?.isAllowedDraft?.( + `/github/repos/${input.repo}/pulls/42/comments/factory-coderabbit-review.json`, + { body: 'arbitrary public comment' }, + { guarded: true }, + )).toBe(false) + expect(await opts?.isAllowedDraft?.( + `/github/repos/${input.repo}/pulls/42/comments/arbitrary.json`, + { body: '@coderabbitai review' }, + { guarded: true }, + )).toBe(false) closes.push(input) }, } diff --git a/src/cli/fleet.ts b/src/cli/fleet.ts index 4d6a99c..72a32a9 100644 --- a/src/cli/fleet.ts +++ b/src/cli/fleet.ts @@ -70,6 +70,7 @@ import { import type { FactoryIntegrationProvider } from '../ports' import { checkMountStaleness } from '../mount/relayfile-binary' import { MountAuthScopeError } from '../mount/mount-auth-error' +import { isAllowedFactoryGithubWritebackDraft } from '../github/review-request' interface FleetCliDeps { fleet?: FleetClient @@ -178,7 +179,7 @@ export async function runFleetCli(argv: string[], deps: FleetCliDeps = {}): Prom const workspaceId = (await (deps.resolveWorkspace ?? resolveFactoryWorkspace)()).workspaceId mount = deps.mount ?? await (deps.cloudMountFromConfig ?? RelayfileCloudMountClient.fromConfig)({ workspaceId, - isAllowedDraft: (path, _content, opts) => isAllowedFactoryGithubDraft(path, opts), + isAllowedDraft: (path, content, opts) => isAllowedFactoryGithubDraft(path, content, opts), }) await prepareFactoryIntegrations(command, mount, undefined, globals, deps, workspaceId, err) githubWrite = mount.githubWrite @@ -1489,20 +1490,18 @@ async function isAllowedFactoryDraft( return true } - if (isAllowedFactoryGithubDraft(path, opts)) { + if (isAllowedFactoryGithubDraft(path, content, opts)) { return true } return false } -const isFactoryGithubWritebackPath = (path: string): boolean => - /^\/github\/repos\/[^/]+\/[^/]+\/(?:pull-requests\/factory-[^/]+\.json|refs\/(?:factory\.json|refs%2Fheads%2Ffactory%2F[^/]+\.json)|pulls\/[1-9]\d*\/close\.json)$/iu.test(path) - const isAllowedFactoryGithubDraft = ( path: string, + content: unknown, opts: { guarded?: boolean } | undefined, -): boolean => opts?.guarded === true && isFactoryGithubWritebackPath(path) +): boolean => opts?.guarded === true && isAllowedFactoryGithubWritebackDraft(path, content) const scopeIssueFromDraftContent = (content: unknown) => ({ title: typeof asRecord(content)?.title === 'string' ? asRecord(content)?.title as string : '', diff --git a/src/github/review-request.test.ts b/src/github/review-request.test.ts new file mode 100644 index 0000000..5eb42a2 --- /dev/null +++ b/src/github/review-request.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' + +import { + containsCoderabbitReviewRequest, + FACTORY_CODERABBIT_REVIEW_BODY, + isAllowedFactoryGithubWritebackDraft, + isFactoryGithubWritebackPath, +} from './review-request' + +describe('isFactoryGithubWritebackPath', () => { + it('allows only the deterministic factory review-request comment path', () => { + expect(isFactoryGithubWritebackPath( + '/github/repos/AgentWorkforce/factory/pulls/42/comments/factory-coderabbit-review.json', + )).toBe(true) + expect(isFactoryGithubWritebackPath( + '/github/repos/AgentWorkforce/factory/pulls/42/comments/arbitrary.json', + )).toBe(false) + }) + + it('allows only the fixed review-request body at the public comment path', () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/42/comments/factory-coderabbit-review.json' + expect(isAllowedFactoryGithubWritebackDraft(path, { + body: FACTORY_CODERABBIT_REVIEW_BODY, + })).toBe(true) + expect(isAllowedFactoryGithubWritebackDraft(path, { + body: 'arbitrary public comment', + })).toBe(false) + expect(isAllowedFactoryGithubWritebackDraft(path, { + body: FACTORY_CODERABBIT_REVIEW_BODY, + title: 'unexpected extra field', + })).toBe(false) + }) + + it('does not mistake a public marker-only comment for a review command', () => { + expect(containsCoderabbitReviewRequest( + '', + )).toBe(false) + expect(containsCoderabbitReviewRequest( + FACTORY_CODERABBIT_REVIEW_BODY, + )).toBe(true) + }) +}) diff --git a/src/github/review-request.ts b/src/github/review-request.ts new file mode 100644 index 0000000..36968d8 --- /dev/null +++ b/src/github/review-request.ts @@ -0,0 +1,18 @@ +export const CODERABBIT_REVIEW_REQUEST = '@coderabbitai review' +export const FACTORY_CODERABBIT_REVIEW_MARKER = '' +export const FACTORY_CODERABBIT_REVIEW_BODY = + `${CODERABBIT_REVIEW_REQUEST}\n${FACTORY_CODERABBIT_REVIEW_MARKER}` + +export const containsCoderabbitReviewRequest = (value: string): boolean => + value.includes(CODERABBIT_REVIEW_REQUEST) && value.includes(FACTORY_CODERABBIT_REVIEW_MARKER) + +export const isFactoryGithubWritebackPath = (path: string): boolean => + /^\/github\/repos\/[^/]+\/[^/]+\/(?:pull-requests\/factory-[^/]+\.json|refs\/(?:factory\.json|refs%2Fheads%2Ffactory%2F[^/]+\.json)|pulls\/[1-9]\d*\/(?:close\.json|comments\/factory-coderabbit-review\.json))$/iu.test(path) + +export const isAllowedFactoryGithubWritebackDraft = (path: string, content: unknown): boolean => { + if (!isFactoryGithubWritebackPath(path)) return false + if (!/\/pulls\/[1-9]\d*\/comments\/factory-coderabbit-review\.json$/iu.test(path)) return true + if (content === null || typeof content !== 'object' || Array.isArray(content)) return false + const record = content as Record + return Object.keys(record).length === 1 && record.body === FACTORY_CODERABBIT_REVIEW_BODY +} diff --git a/src/index.ts b/src/index.ts index 0d2e6ee..5eee6e0 100644 --- a/src/index.ts +++ b/src/index.ts @@ -301,6 +301,7 @@ export type { FactoryIntegrationProvider, GithubPublishPullRequestInput, GithubPublishPullRequestResult, + GithubPullRequestRef, LocalMountOptions, MountClient, ProviderSyncStatus, diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index f8050f5..e0920e2 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -63,6 +63,10 @@ class FakeRelayFileClient implements RelayFileClientLike { readonly getEventsCalls: Array<{ workspaceId: string; opts?: { cursor?: string; limit?: number; provider?: string; last?: number } }> = [] readonly listLastNChangesCalls: Array<{ limit: number; context?: { workspaceId: string } }> = [] readonly getOpCalls: Array<{ workspaceId: string; opId: string }> = [] + readonly listOpsCalls: Array<{ + workspaceId: string + options?: { action?: string; provider?: string; cursor?: string; limit?: number } + }> = [] readonly createSubscriptionCalls: CreateOrRenewDurableResourceSubscriptionInput[] = [] readonly claimDeliveryCalls: ClaimDurableSubscriptionDeliveriesInput[] = [] readonly acceptDeliveryCalls: AcceptDurableSubscriptionDeliveryInput[] = [] @@ -196,6 +200,24 @@ class FakeRelayFileClient implements RelayFileClientLike { } } + async listOps( + workspaceId: string, + options?: { action?: string; provider?: string; cursor?: string; limit?: number }, + ) { + this.listOpsCalls.push({ workspaceId, options }) + const matching = [...this.ops.values()].filter((operation) => + (!options?.action || operation.action === options.action) && + (!options?.provider || operation.provider === options.provider)) + const start = Number(options?.cursor ?? 0) + const limit = options?.limit ?? matching.length + const items = matching.slice(start, start + limit) + const next = start + items.length + return { + items, + nextCursor: next < matching.length ? String(next) : null, + } + } + async createOrRenewDurableResourceSubscription(input: CreateOrRenewDurableResourceSubscriptionInput) { this.createSubscriptionCalls.push(input) return { @@ -1265,6 +1287,128 @@ describe('RelayfileCloudMountClient', () => { expect(fake.getOpCalls).toEqual([]) }) + it('recovers the latest write operation by path after a mount restart', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + const content = { body: '@coderabbitai review\n' } + const fake = new FakeRelayFileClient() + fake.files.set(path, { + revision: '2', + content: JSON.stringify(content), + contentType: 'application/json', + }) + fake.ops.set('op-old', { + opId: 'op-old', + path, + action: 'file_upsert', + provider: 'github', + status: 'succeeded', + attemptCount: 1, + createdAt: '2026-07-30T00:00:00.000Z', + providerResult: { status: 201, externalId: '84' }, + }) + fake.ops.set('op-latest', { + opId: 'op-latest', + path, + action: 'file_upsert', + provider: 'github', + status: 'failed', + attemptCount: 1, + createdAt: '2026-07-30T01:00:00.000Z', + }) + const restartedMount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + restartedMount.setDefaultAllowedDeletePredicate((candidatePath, candidateContent) => + candidatePath === path && JSON.stringify(candidateContent) === JSON.stringify(content)) + + await expect(restartedMount.confirmWrite(path, { + timeoutMs: 5, + returnFailed: true, + })).resolves.toBe('failed') + await expect(restartedMount.deleteFile(path)).resolves.toBeUndefined() + + expect(fake.listOpsCalls).toEqual([{ + workspaceId: 'rw_test', + options: { + action: 'file_upsert', + provider: 'github', + cursor: undefined, + limit: 100, + }, + }]) + expect(fake.getOpCalls).toEqual([ + { workspaceId: 'rw_test', opId: 'op-latest' }, + { workspaceId: 'rw_test', opId: 'op-latest' }, + ]) + expect(fake.deleteFileCalls).toHaveLength(1) + }) + + it('fails closed when restarted write operations have the same latest timestamp', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + const fake = new FakeRelayFileClient() + for (const opId of ['op-first', 'op-second']) { + fake.ops.set(opId, { + opId, + path, + action: 'file_upsert', + provider: 'github', + status: opId === 'op-first' ? 'succeeded' : 'failed', + attemptCount: 1, + createdAt: '2026-07-30T01:00:00.000Z', + }) + } + const restartedMount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await expect(restartedMount.confirmWrite(path, { + timeoutMs: 5, + returnFailed: true, + })).resolves.toBe('timeout') + + expect(fake.getOpCalls).toEqual([]) + expect(fake.deleteFileCalls).toEqual([]) + }) + + it('fails closed when one of multiple restarted write operations is undated', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + const fake = new FakeRelayFileClient() + fake.ops.set('op-dated', { + opId: 'op-dated', + path, + action: 'file_upsert', + provider: 'github', + status: 'failed', + attemptCount: 1, + createdAt: '2026-07-30T01:00:00.000Z', + }) + fake.ops.set('op-undated', { + opId: 'op-undated', + path, + action: 'file_upsert', + provider: 'github', + status: 'succeeded', + attemptCount: 1, + }) + const restartedMount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await expect(restartedMount.confirmWrite(path, { + timeoutMs: 5, + returnFailed: true, + })).resolves.toBe('timeout') + + expect(fake.getOpCalls).toEqual([]) + expect(fake.deleteFileCalls).toEqual([]) + }) + it('refuses provider writeback paths when the draft predicate is unset or rejects', async () => { const fake = new FakeRelayFileClient() const unset = new RelayfileCloudMountClient({ workspaceId: 'rw_test', client: fake }) @@ -1523,6 +1667,46 @@ describe('RelayfileCloudMountClient', () => { }]) }) + it('installs a default provider-delete predicate without replacing an injected policy', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + const content = { body: '@coderabbitai review\n' } + const createMount = ( + fake: FakeRelayFileClient, + isAllowedDelete?: () => boolean, + ): RelayfileCloudMountClient => new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + isAllowedDelete, + }) + + const defaultFake = new FakeRelayFileClient() + defaultFake.ops.set('op-1', { + opId: 'op-1', + status: 'failed', + attemptCount: 1, + }) + const defaultMount = createMount(defaultFake) + defaultMount.setDefaultAllowedDeletePredicate((candidatePath, candidateContent) => + candidatePath === path && JSON.stringify(candidateContent) === JSON.stringify(content)) + await defaultMount.writeFile(path, content) + await expect(defaultMount.deleteFile(path)).resolves.toBeUndefined() + expect(defaultFake.deleteFileCalls).toHaveLength(1) + + const injectedFake = new FakeRelayFileClient() + injectedFake.ops.set('op-1', { + opId: 'op-1', + status: 'failed', + attemptCount: 1, + }) + const injectedMount = createMount(injectedFake, () => false) + injectedMount.setDefaultAllowedDeletePredicate(() => true) + await injectedMount.writeFile(path, content) + await expect(injectedMount.deleteFile(path)) + .rejects.toThrow(/delete predicate rejected or is unset/) + expect(injectedFake.deleteFileCalls).toEqual([]) + }) + it('refuses failed unlinked orphan deletes when the injected delete predicate is unset', async () => { const fake = new FakeRelayFileClient() fake.files.set('/linear/issues/AR-E2ECANARY.json', { diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index fb45002..b7dd075 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -12,7 +12,9 @@ import { type EventFeedResponse, type FileReadResponse, type GetEventsOptions, + type GetOperationsOptions, type ListTreeOptions, + type OperationFeedResponse, type ResourceAtEventResult, type OperationStatusResponse, type Subscription, @@ -223,6 +225,7 @@ export type RelayFileClientLike = { listLastNChanges?(limit: number, context?: { workspaceId: string; token?: string }): Promise<{ events: ChangeEvent[] }> getResourceAtEvent(eventId: string, context?: { workspaceId: string; token?: string }): Promise getOp?(workspaceId: string, opId: string): Promise + listOps?(workspaceId: string, options?: GetOperationsOptions): Promise getSyncStatus?(workspaceId: string, options?: { provider?: string }): Promise getToken?(): Promise | string getBaseUrl?(): string @@ -284,7 +287,7 @@ export class RelayfileCloudMountClient implements MountClient { #activeLocalMountOperations = 0 #disposed = false #isAllowedDraft?: (path: string, content: unknown, opts?: { guarded?: boolean }) => boolean | Promise - readonly #isAllowedDelete?: (path: string, currentContent: unknown) => boolean | Promise + #isAllowedDelete?: (path: string, currentContent: unknown) => boolean | Promise readonly #lastOpByPath = new Map() readonly #confirmedExternalIdByPath = new Map() readonly #confirmedFailureReasonByPath = new Map() @@ -336,6 +339,12 @@ export class RelayfileCloudMountClient implements MountClient { this.#isAllowedDraft ??= predicate } + setDefaultAllowedDeletePredicate( + predicate: (path: string, content: unknown) => boolean | Promise, + ): void { + this.#isAllowedDelete ??= predicate + } + static async fromConfig(config: RelayfileCloudMountClientConfig = {}): Promise { if ('credsPath' in config) { throw new Error('RelayfileCloudMountClient no longer accepts credsPath; run `agent-relay login` to use the shared cloud session') @@ -777,9 +786,9 @@ export class RelayfileCloudMountClient implements MountClient { async confirmWrite( path: string, - opts: { timeoutMs?: number } = {}, + opts: { timeoutMs?: number; returnFailed?: boolean } = {}, ): Promise<'acked' | 'pending' | 'failed' | 'timeout'> { - const opId = this.#lastOpByPath.get(path) + const opId = this.#lastOpByPath.get(path) ?? await this.#recoverLatestWriteOperation(path) if (!opId || !this.#client.getOp) return 'timeout' const deadline = Date.now() + (opts.timeoutMs ?? 90_000) @@ -790,6 +799,12 @@ export class RelayfileCloudMountClient implements MountClient { status = mapOperationStatus(operation) } catch (error) { this.#confirmedFailureReasonByPath.set(path, providerResultError(operation)) + if ( + opts.returnFailed && + (operation.status === 'failed' || + operation.status === 'dead_lettered' || + operation.status === 'canceled') + ) return 'failed' throw error } if (status !== 'pending') { @@ -808,6 +823,27 @@ export class RelayfileCloudMountClient implements MountClient { } } + async #recoverLatestWriteOperation(path: string): Promise { + if (!this.#client.listOps) return undefined + let cursor: string | undefined + const matching: OperationStatusResponse[] = [] + do { + const page = await this.#client.listOps(this.workspaceId, { + action: 'file_upsert', + provider: providerForPath(path), + cursor, + limit: 100, + }) + for (const operation of page.items) { + if (operation.path === path) matching.push(operation) + } + cursor = page.nextCursor ?? undefined + } while (cursor) + const latest = uniquelyLatestOperation(matching) + if (latest) this.#lastOpByPath.set(path, latest.opId) + return latest?.opId + } + async getConfirmedWriteFailureReason(path: string): Promise { return this.#confirmedFailureReasonByPath.get(path) } @@ -1053,6 +1089,26 @@ const isProviderWritebackPath = (path: string): boolean => const isProviderPath = (path: string): boolean => path.startsWith('/linear/') || path.startsWith('/github/') || path.startsWith('/slack/') +const providerForPath = (path: string): string | undefined => + /^\/([^/]+)\//u.exec(path)?.[1] + +const uniquelyLatestOperation = ( + operations: OperationStatusResponse[], +): OperationStatusResponse | undefined => { + if (operations.length === 1) return operations[0] + const dated = operations + .map((operation) => ({ operation, time: Date.parse(operation.createdAt ?? '') })) + // Multiple operations can only be ordered when every candidate carries the + // sole authoritative ordering field. An undated retry might be newest. + if (dated.some((candidate) => !Number.isFinite(candidate.time))) return undefined + const latestTime = Math.max(...dated.map((candidate) => candidate.time)) + const latest = dated.filter((candidate) => candidate.time === latestTime) + // createdAt is the only authoritative ordering field exposed by listOps. + // Equal latest timestamps cannot establish which retry came last, so fail + // closed rather than confirming or deleting against an arbitrary operation. + return latest.length === 1 ? latest[0]?.operation : undefined +} + const providerContentLooksLinked = (content: unknown): boolean => { const record = content !== null && typeof content === 'object' && !Array.isArray(content) ? content as Record diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index e76b975..998d732 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -16,6 +16,308 @@ const gitRunnerForBranch = (branch: string): GitCommandRunner => vi.fn(async (ar }) describe('RelayfileGithubConnectionWrite', () => { + it('requests CodeRabbit review once through the connected app write path', async () => { + const mount = new FakeMountClient() + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + const input = { repo: 'AgentWorkforce/factory', number: 85 } + + await Promise.all([ + write.requestPullRequestReview(input), + write.requestPullRequestReview(input), + ]) + + const draftPath = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + expect(mount.writes).toEqual([{ + path: draftPath, + content: { body: '@coderabbitai review\n' }, + }]) + + mount.files.delete(draftPath) + mount.files.set('/github/repos/AgentWorkforce/factory/pulls/85__title/comments/9001.json', { + content: { + payload: { + comment: { + body: '@coderabbitai review\n', + }, + }, + }, + }) + const restarted = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + await restarted.requestPullRequestReview(input) + expect(mount.writes).toHaveLength(1) + }) + + it('treats missing fresh-PR comment trees as empty and posts the first request', async () => { + class MissingCommentTreesMount extends FakeMountClient { + override async listTree(): Promise { + throw Object.assign(new Error('not found'), { status: 404 }) + } + } + const mount = new MissingCommentTreesMount() + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 85, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([{ + path: '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json', + content: { body: '@coderabbitai review\n' }, + }]) + }) + + it('propagates non-not-found tree scan failures without posting a request', async () => { + class FailedCommentTreeMount extends FakeMountClient { + override async listTree(): Promise { + throw Object.assign(new Error('service unavailable'), { status: 503 }) + } + } + const mount = new FailedCommentTreeMount() + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 85, + })).rejects.toThrow('service unavailable') + + expect(mount.writes).toEqual([]) + }) + + it('retries a review request after a failed provider confirmation', async () => { + const draftPath = '/github/repos/AgentWorkforce/factory/pulls/86/comments/factory-coderabbit-review.json' + class FailedThenAcknowledgedMount extends FakeMountClient { + confirmationCount = 0 + + override async confirmWrite(path: string): Promise<'acked' | 'failed'> { + if (path !== draftPath) return 'acked' + this.confirmationCount += 1 + return this.confirmationCount < 3 ? 'failed' : 'acked' + } + } + const mount = new FailedThenAcknowledgedMount() + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + const input = { repo: 'AgentWorkforce/factory', number: 86 } + + await expect(write.requestPullRequestReview(input)) + .rejects.toThrow(`GitHub writeback did not complete for ${draftPath}: failed`) + + await expect(write.requestPullRequestReview(input)).resolves.toBeUndefined() + expect(mount.writes.filter((entry) => entry.path === draftPath)).toHaveLength(2) + expect(mount.deletes).toEqual([draftPath]) + }) + + it('recognizes an acknowledged review draft before provider reconciliation', async () => { + const draftPath = '/github/repos/AgentWorkforce/factory/pulls/86/comments/factory-coderabbit-review.json' + const mount = new FakeMountClient({ + [draftPath]: { + body: '@coderabbitai review\n', + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 86, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.deletes).toEqual([]) + }) + + it('fails closed on an indeterminate review draft instead of posting a duplicate', async () => { + const draftPath = '/github/repos/AgentWorkforce/factory/pulls/86/comments/factory-coderabbit-review.json' + const mount = new FakeMountClient({ + [draftPath]: { + body: '@coderabbitai review\n', + }, + }) + mount.setConfirmWrite(draftPath, 'timeout') + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 86, + })).rejects.toThrow( + `GitHub review request draft has indeterminate provider status for ${draftPath}: timeout`, + ) + + expect(mount.writes).toEqual([]) + expect(mount.deletes).toEqual([]) + }) + + it('fails an indeterminate comment scan instead of posting a duplicate request', async () => { + const stalePath = '/github/repos/AgentWorkforce/factory/pulls/87__renamed/comments/9001.json' + class RacingMount extends FakeMountClient { + readonly listPrefixes: string[] = [] + failRead = true + + override async listTree(prefix: string): Promise { + this.listPrefixes.push(prefix) + if (prefix.endsWith('/pulls')) { + return ['/github/repos/AgentWorkforce/factory/pulls/87__renamed'] + } + if (prefix.endsWith('/pulls/87__renamed/comments')) return [stalePath] + return [] + } + + override async readFile(path: string): Promise<{ content: unknown; revision?: string }> { + if (path === stalePath && this.failRead) throw new Error('reconciled path moved') + return { + content: { + payload: { + comment: { + body: '@coderabbitai review\n', + }, + }, + }, + } + } + } + const mount = new RacingMount() + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + const input = { repo: 'AgentWorkforce/factory', number: 87 } + + await expect(write.requestPullRequestReview(input)).rejects.toThrow('reconciled path moved') + expect(mount.writes).toEqual([]) + + mount.failRead = false + await expect(write.requestPullRequestReview(input)).resolves.toBeUndefined() + expect(mount.writes).toEqual([]) + expect(mount.listPrefixes).toEqual([ + '/github/repos/AgentWorkforce/factory/pulls', + '/github/repos/AgentWorkforce/factory/pulls/87/comments', + '/github/repos/AgentWorkforce/factory/pulls/87__renamed/comments', + '/github/repos/AgentWorkforce/factory/pulls', + '/github/repos/AgentWorkforce/factory/pulls/87/comments', + '/github/repos/AgentWorkforce/factory/pulls/87__renamed/comments', + ]) + }) + + it('finds a reconciled request in nested comment metadata', async () => { + const nestedPath = + '/github/repos/AgentWorkforce/factory/pulls/88__renamed/comments/9002/meta.json' + const mount = new FakeMountClient({ + [nestedPath]: { + payload: { + comment: { + body: '@coderabbitai review\n', + }, + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 88, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.reads).toEqual([nestedPath]) + }) + + it('does not let a marker-only mounted comment suppress the review command', async () => { + const markerOnlyPath = + '/github/repos/AgentWorkforce/factory/pulls/88__renamed/comments/9002.json' + const mount = new FakeMountClient({ + [markerOnlyPath]: { + body: '', + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 88, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([{ + path: '/github/repos/AgentWorkforce/factory/pulls/88/comments/factory-coderabbit-review.json', + content: { body: '@coderabbitai review\n' }, + }]) + }) + + it('finds a reconciled request in the flat repository layout', async () => { + const flatPath = + '/github/repos/AgentWorkforce__factory/pulls/89__renamed/comments/9003.json' + const mount = new FakeMountClient({ + [flatPath]: { + repository: { full_name: 'AgentWorkforce/factory' }, + pull_request: { number: 89 }, + comment: { + body: '@coderabbitai review\n', + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 89, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.reads).toEqual([flatPath]) + }) + + it('finds a request in the canonical repository-level comment layout after restart', async () => { + const canonicalPath = + '/github/repos/AgentWorkforce/factory/comments/9004.json' + const unrelatedPath = + '/github/repos/AgentWorkforce/factory/comments/9003.json' + const mount = new FakeMountClient({ + [unrelatedPath]: { + repository: { full_name: 'AgentWorkforce/other' }, + pull_request: { number: 90 }, + comment: { + body: '@coderabbitai review\n', + }, + }, + [canonicalPath]: { + repository: { full_name: 'AgentWorkforce/factory' }, + pull_request: { number: 90 }, + comment: { + body: '@coderabbitai review\n', + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 90, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.reads).toEqual([unrelatedPath, canonicalPath]) + }) + + it('does not let another pull request canonical comment suppress a request', async () => { + const canonicalPath = + '/github/repos/AgentWorkforce/factory/comments/9006.json' + const mount = new FakeMountClient({ + [canonicalPath]: { + repository: { full_name: 'AgentWorkforce/factory' }, + pull_request: { number: 91 }, + comment: { + body: '@coderabbitai review\n', + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 92, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([{ + path: '/github/repos/AgentWorkforce/factory/pulls/92/comments/factory-coderabbit-review.json', + content: { body: '@coderabbitai review\n' }, + }]) + }) + it('publishes an already-pushed remote branch without reading an orchestrator-local clone', async () => { const pullRequestPath = '/github/repos/AgentWorkforce/factory/pull-requests/factory-factory-ar-85-agentworkforce-factory-pushed.json' class ReceiptMount extends FakeMountClient { diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index f321cee..fb6f164 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -7,6 +7,11 @@ import type { GithubPublishPullRequestResult, MountClient, } from '../ports' +import { + FACTORY_CODERABBIT_REVIEW_BODY, + containsCoderabbitReviewRequest, + isAllowedFactoryGithubWritebackDraft, +} from '../github/review-request' const execFileAsync = promisify(execFile) const WRITE_CONFIRM_TIMEOUT_MS = 90_000 @@ -16,7 +21,7 @@ const RECEIPT_READ_DELAY_MS = 100 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string; stderr?: string }> export interface RelayfileGithubConnectionWriteConfig { - mount: Pick + mount: Pick gitRunner?: GitCommandRunner receiptReadAttempts?: number receiptReadDelayMs?: number @@ -33,6 +38,7 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { readonly #receiptReadAttempts: number readonly #receiptReadDelayMs: number readonly #writesByPath = new Map>() + readonly #reviewRequests = new Map>() constructor(config: RelayfileGithubConnectionWriteConfig) { this.#mount = config.mount @@ -111,6 +117,135 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { } } + async requestPullRequestReview(input: { repo: string; number: number }): Promise { + const { owner, repo } = githubRepoParts(input.repo) + if (!Number.isInteger(input.number) || input.number <= 0) { + throw new Error(`GitHub pull request number must be a positive integer: ${input.number}`) + } + const repoRoots = [ + `/github/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`, + `/github/repos/${encodeURIComponent(owner)}__${encodeURIComponent(repo)}`, + ] + const requestKey = `${input.repo.toLowerCase()}#${input.number}` + const existing = this.#reviewRequests.get(requestKey) + if (existing) return existing + const request = this.#requestPullRequestReview(input.repo, input.number, repoRoots) + .catch((error: unknown) => { + this.#reviewRequests.delete(requestKey) + throw error + }) + this.#reviewRequests.set(requestKey, request) + return request + } + + async #requestPullRequestReview(expectedRepo: string, number: number, repoRoots: string[]): Promise { + for (const repoRoot of repoRoots) { + if (await this.#hasPullRequestReviewRequest(expectedRepo, number, repoRoot)) return + } + const commentsRoot = `${repoRoots[0]}/pulls/${number}/comments` + await this.#writeAndConfirm(`${commentsRoot}/factory-coderabbit-review.json`, { + body: FACTORY_CODERABBIT_REVIEW_BODY, + }) + } + + async #hasPullRequestReviewRequest(expectedRepo: string, number: number, repoRoot: string): Promise { + const pullsRoot = `${repoRoot}/pulls` + const pullDirectoryPattern = new RegExp( + `^${escapeRegExp(pullsRoot)}/(${number}(?:__[^/]+)?)(?:/|$)`, + 'u', + ) + const commentRoots = new Set([`${pullsRoot}/${number}/comments`]) + for (const path of await this.#listTreeOrEmpty(pullsRoot)) { + const pullDirectory = pullDirectoryPattern.exec(path)?.[1] + if (pullDirectory) commentRoots.add(`${pullsRoot}/${pullDirectory}/comments`) + } + for (const commentsRoot of commentRoots) { + const directCommentPattern = new RegExp(`^${escapeRegExp(commentsRoot)}/[^/]+\\.json$`, 'u') + const nestedCommentPattern = new RegExp( + `^${escapeRegExp(commentsRoot)}/[^/]+/(?:meta|metadata)\\.json$`, + 'u', + ) + const directCommentDirectoryPattern = new RegExp(`^${escapeRegExp(commentsRoot)}/[^/.]+$`, 'u') + const commentPaths = new Set() + for (const path of await this.#listTreeOrEmpty(commentsRoot)) { + if (directCommentPattern.test(path) || nestedCommentPattern.test(path)) { + commentPaths.add(path) + continue + } + if (!directCommentDirectoryPattern.test(path)) continue + for (const nestedPath of await this.#listTreeOrEmpty(path)) { + if (nestedCommentPattern.test(nestedPath)) commentPaths.add(nestedPath) + } + } + for (const path of commentPaths) { + // A listed comment becoming unreadable is indeterminate, not evidence + // that the request is absent. Propagate so the caller retries the scan. + const content = record((await this.#mount.readFile(path)).content) + // A failed provider operation can leave our bare local draft in the + // tree. Resolve its provider status rather than treating the draft + // itself as evidence that GitHub received the request. + if (isAllowedFactoryGithubWritebackDraft(path, content)) { + const status = await this.#mount.confirmWrite(path, { + timeoutMs: 1, + returnFailed: true, + }) + if (status === 'acked') return true + if (status === 'failed') { + await this.#mount.deleteFile(path) + continue + } + throw new Error(`GitHub review request draft has indeterminate provider status for ${path}: ${status}`) + } + const payload = record(content.payload) + const comment = record(payload.comment) + const rootComment = record(content.comment) + const body = stringValue(content.body) ?? stringValue(payload.body) ?? stringValue(comment.body) + ?? stringValue(rootComment.body) + if (body && containsCoderabbitReviewRequest(body)) return true + } + } + const canonicalCommentsRoot = `${repoRoot}/comments` + const canonicalDirectPattern = new RegExp( + `^${escapeRegExp(canonicalCommentsRoot)}/[^/]+\\.json$`, + 'u', + ) + const canonicalNestedPattern = new RegExp( + `^${escapeRegExp(canonicalCommentsRoot)}/[^/]+/(?:meta|metadata)\\.json$`, + 'u', + ) + const canonicalDirectoryPattern = new RegExp( + `^${escapeRegExp(canonicalCommentsRoot)}/[^/.]+$`, + 'u', + ) + const canonicalPaths = new Set() + for (const path of await this.#listTreeOrEmpty(canonicalCommentsRoot)) { + if (canonicalDirectPattern.test(path) || canonicalNestedPattern.test(path)) { + canonicalPaths.add(path) + continue + } + if (!canonicalDirectoryPattern.test(path)) continue + for (const nestedPath of await this.#listTreeOrEmpty(path)) { + if (canonicalNestedPattern.test(nestedPath)) canonicalPaths.add(nestedPath) + } + } + for (const path of canonicalPaths) { + const content = record((await this.#mount.readFile(path)).content) + if (!canonicalGithubCommentMatches(content, expectedRepo, number)) continue + const body = githubCommentBody(content) + if (body && containsCoderabbitReviewRequest(body)) return true + } + return false + } + + async #listTreeOrEmpty(prefix: string): Promise { + try { + return await this.#mount.listTree(prefix) + } catch (error) { + if (isMountPathNotFound(error)) return [] + throw error + } + } + async closePullRequest(input: { repo: string; number: number }): Promise { const { owner, repo } = githubRepoParts(input.repo) if (!Number.isInteger(input.number) || input.number <= 0) { @@ -214,9 +349,49 @@ const record = (value: unknown): Record => ? value as Record : {} +const escapeRegExp = (value: string): string => + value.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&') + +const isMountPathNotFound = (error: unknown): boolean => { + const details = record(error) + const response = record(details.response) + const status = details.status ?? details.statusCode ?? response.status ?? response.statusCode + const code = stringValue(details.code)?.toLowerCase() + return status === 404 || status === '404' || code === 'not_found' || code === 'file_not_found' +} + const stringValue = (value: unknown): string | undefined => typeof value === 'string' && value.length > 0 ? value : undefined +const githubCommentBody = (content: Record): string | undefined => { + const payload = record(content.payload) + const comment = record(payload.comment) + const rootComment = record(content.comment) + return stringValue(content.body) ?? stringValue(payload.body) ?? stringValue(comment.body) + ?? stringValue(rootComment.body) +} + +const canonicalGithubCommentMatches = ( + content: Record, + expectedRepo: string, + expectedNumber: number, +): boolean => { + const payload = record(content.payload) + const repository = record( + Object.keys(record(payload.repository)).length > 0 + ? payload.repository + : content.repository, + ) + const pullRequest = record( + Object.keys(record(payload.pull_request)).length > 0 + ? payload.pull_request + : content.pull_request, + ) + const fullName = stringValue(repository.full_name) + const number = positiveInteger(pullRequest.number) + return fullName?.toLowerCase() === expectedRepo.toLowerCase() && number === expectedNumber +} + const positiveInteger = (value: unknown): number | undefined => { const number = typeof value === 'number' ? value : typeof value === 'string' ? Number(value) : Number.NaN return Number.isInteger(number) && number > 0 ? number : undefined diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 83ead19..46be5ed 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -291,12 +291,14 @@ class RecordingGithubWriteback implements GithubWriteback { class PublishingGithubWriteback extends RecordingGithubWriteback { readonly publishInputs: GithubPublishPullRequestInput[] = [] + readonly reviewRequests: Array<{ repo: string; number: number }> = [] constructor( private readonly receipt: { number: number author: string }, + private readonly failReviewRequest = false, ) { super() } @@ -311,6 +313,11 @@ class PublishingGithubWriteback extends RecordingGithubWriteback { author: this.receipt.author, } } + + async requestPullRequestReview(input: { repo: string; number: number }) { + this.reviewRequests.push(input) + if (this.failReviewRequest) throw new Error('transient review request failure') + } } class RecordingFactoryEventReporter { @@ -3698,14 +3705,17 @@ describe('FactoryLoop', () => { expect(githubWriteback.statuses).toEqual([]) }) - it('does not treat an accepted merge command as proof that a GitHub-native PR merged', async () => { + it('does not let an accepted merge command or review-request failure falsely complete a GitHub-native issue', async () => { const path = githubIssuePath('AgentWorkforce', 'pear', 50) const mount = new FakeMountClient({ [path]: githubIssueFile(50, { labels: ['factory'] }), }) mount.setSubRoot('/linear/issues', 'absent') const fleet = new FakeFleetClient() - const githubWriteback = new RecordingGithubWriteback() + const githubWriteback = new PublishingGithubWriteback( + { number: 50, author: 'operator-user' }, + true, + ) const mergeGate = new ScriptedGithubMergeGate([readyMergeVerdict('github-head')]) const factory = createFactory(config({ issueSource: 'github', @@ -3727,6 +3737,7 @@ describe('FactoryLoop', () => { expect(mergeGate.checks).toEqual([{ repo: 'AgentWorkforce/pear', number: 50 }]) expect(mergeGate.merges).toEqual([{ repo: 'AgentWorkforce/pear', number: 50, expectedHeadSha: 'github-head' }]) + expect(githubWriteback.reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number: 50 }]) expect(githubWriteback.closes).toEqual([]) expect(githubWriteback.statuses).toEqual([ { key: '50', status: 'in-progress' }, @@ -6179,8 +6190,12 @@ describe('FactoryLoop', () => { const publishPullRequest = vi.fn(async () => { throw new Error('must reconcile the exact existing branch') }) + const reviewRequests: Array<{ repo: string; number: number }> = [] const mount = new FakeMountClient({ [issuePath(597)]: issue }, { publishPullRequest, + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + }, closePullRequest: async () => undefined, }) const fleet = new DurableRemoteLifecycleFleetClient() @@ -6222,8 +6237,10 @@ describe('FactoryLoop', () => { }, }) }) - expect(ghCalls.length).toBeGreaterThan(0) - expect(ghCalls.every((args) => args.includes('--head') && args.includes(branch))).toBe(true) + const lookupCalls = ghCalls.filter((args) => args[0] === 'pr' && args[1] === 'list') + expect(lookupCalls.length).toBeGreaterThan(0) + expect(lookupCalls.every((args) => args.includes('--head') && args.includes(branch))).toBe(true) + expect(reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number: 1597 }]) expect(publishPullRequest).not.toHaveBeenCalled() await factory.stop() }) @@ -6622,6 +6639,7 @@ describe('FactoryLoop', () => { } }, closePullRequest: async () => undefined, + requestPullRequestReview: async () => undefined, } const mount = new ConfirmingMount({ [issuePath(885)]: issueFile(885), @@ -9943,6 +9961,7 @@ describe('FactoryLoop', () => { async ({ identity, appAvailable, expectedIdentity }) => { const number = identity === 'app' ? 520 : identity === 'user' ? 521 : appAvailable ? 522 : 523 const appInputs: GithubPublishPullRequestInput[] = [] + const appReviewRequests: Array<{ repo: string; number: number }> = [] const appWrite: GithubConnectionWrite = { publishPullRequest: async (input) => { appInputs.push(input) @@ -9954,6 +9973,9 @@ describe('FactoryLoop', () => { author: 'relayfile[bot]', } }, + requestPullRequestReview: async (input) => { + appReviewRequests.push(input) + }, closePullRequest: async () => undefined, } const mount = new FakeMountClient({ @@ -9982,6 +10004,12 @@ describe('FactoryLoop', () => { expect(appInputs).toHaveLength(expectedIdentity === 'app' ? 1 : 0) expect(userWriteback.publishInputs).toHaveLength(expectedIdentity === 'user' ? 1 : 0) + expect(appReviewRequests).toEqual( + expectedIdentity === 'app' ? [{ repo: 'AgentWorkforce/pear', number }] : [], + ) + expect(userWriteback.reviewRequests).toEqual( + expectedIdentity === 'user' ? [{ repo: 'AgentWorkforce/pear', number }] : [], + ) expect(infoLogs).toContainEqual([ '[factory] published PR', expect.objectContaining({ @@ -10059,7 +10087,7 @@ describe('FactoryLoop', () => { }, }, probePrResolver: async () => undefined, - probePrGhRunner: async () => { throw new Error('gh must not be invoked for PR publication') }, + probePrGhRunner: async () => { throw new Error('gh must not be invoked for app publication') }, }) await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(52), issueFile(52)))) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index e4b3f15..40eb972 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -6,6 +6,7 @@ import { FactoryConfigSchema, type FactoryConfig } from '../config/schema' import { linearByStatePath, linearByIdPath, linearByUuidPath } from '../constants/linear' import { stateResolutionFromIds, type FactoryStateResolution } from '../linear/state-resolver' import { GithubMergeGate, closeProbePr, type GhRunner, type GithubMergeGate as GithubMergeGatePort } from '../github' +import { isAllowedFactoryGithubWritebackDraft } from '../github/review-request' import { VerificationPipeline, type VerificationGate } from '../environments/verification-pipeline' import type { AgentMessage, @@ -20,6 +21,7 @@ import type { GithubConnectionWrite, GithubIssueStatus, GithubPublishPullRequestResult, + GithubPullRequestRef, GithubRead, GithubWriteback, LinearWriteback, @@ -518,6 +520,7 @@ export class FactoryLoop implements Factory { // is active, but the PTY submit must never land in that critical window. readonly #babysitterCriticalAgents = new Set() readonly #publishedPullRequests = new Map() + readonly #reviewRequestedPullRequests = new Set() readonly #previewReferences = new Map() readonly #removedPreviewIds = new Set() readonly #probePrGhBackoffUntilMs = new Map() @@ -1561,6 +1564,10 @@ export class FactoryLoop implements Factory { this.#probePrGhBackoffUntilMs.set(issueStateKey(issueRef(issue)), this.#clock.now() + PROBE_PR_GH_BACKOFF_MS) return undefined } + await this.#requestAutomatedPullRequestReview({ + repo: pr.repo, + number: pr.prNumber, + }) if (record.decision.implementers.length > 1 && !await this.#allImplementersHaveCompletionPr(record)) { this.#increment('completionSweepMissingPr') return undefined @@ -6094,10 +6101,12 @@ export class FactoryLoop implements Factory { ): Promise { const key = `${issueKey(record.issue)}:${implementer.spec.repo}` const durable = await this.#state.getDispatchLifecycle(this.#workspaceId, issueKey(record.issue)) - const cached = this.#publishedPullRequests.get(key) - if (cached) return cached - const { identity, publisher } = this.#githubPullRequestPublisher() + const cached = this.#publishedPullRequests.get(key) + if (cached) { + await this.#requestAutomatedPullRequestReview(cached) + return cached + } const remoteBranch = implementer.result?.locality === 'remote' && implementer.spec.branch ? implementer.spec.branch : undefined @@ -6123,11 +6132,16 @@ export class FactoryLoop implements Factory { if ( durableReceipt && (!opts.reconcileExisting || !expectedHeadRef || durableReceipt.headRef === expectedHeadRef) - ) return durableReceipt + ) { + this.#publishedPullRequests.set(key, durableReceipt) + await this.#requestAutomatedPullRequestReview(durableReceipt) + return durableReceipt + } if (opts.reconcileExisting && expectedHeadRef) { const existing = await this.#openPullRequestByHead(repo, expectedHeadRef) if (existing) { this.#publishedPullRequests.set(key, existing) + await this.#requestAutomatedPullRequestReview(existing) this.#increment('githubPullRequestsReconciled') this.#logger.info?.('[factory] reconciled existing PR from implementer branch', { issue: issue.key, @@ -6171,16 +6185,54 @@ export class FactoryLoop implements Factory { identity, author: published.author, }) + await this.#requestAutomatedPullRequestReview(published) return published } + async #requestAutomatedPullRequestReview( + published: GithubPullRequestRef, + ): Promise { + const key = `${published.repo.toLowerCase()}#${published.number}` + if (this.#reviewRequestedPullRequests.has(key)) return + try { + const requestReview = this.#githubPullRequestReviewRequester() + if (!requestReview) return + await requestReview({ repo: published.repo, number: published.number }) + this.#reviewRequestedPullRequests.add(key) + } catch (error) { + this.#increment('githubPullRequestReviewRequestFailures') + this.#logger.warn?.('[factory] automated PR review request failed; lifecycle completion remains independent', { + repo: published.repo, + prNumber: published.number, + error: describeError(error).errorMessage, + }) + } + } + + #githubPullRequestReviewRequester(): ((input: GithubPullRequestRef) => Promise) | undefined { + const configured = this.#config.github.identity + if (configured !== 'user' && this.#mount.githubWrite) { + return this.#mount.githubWrite.requestPullRequestReview?.bind(this.#mount.githubWrite) + } + if (configured === 'app') { + throw new Error( + 'GitHub PR identity "app" requires a connected workspace GitHub App write path; refusing to fall back to the local gh user', + ) + } + if (this.#mount.writebackTransport === 'test' && !this.#githubWritebackProvided) return undefined + return this.#githubWriteback.requestPullRequestReview?.bind(this.#githubWriteback) + } + #githubPullRequestPublisher(): { identity: GithubPullRequestIdentity publisher: GithubPullRequestPublisher } { const configured = this.#config.github.identity if (configured !== 'user' && this.#mount.githubWrite) { - return { identity: 'app', publisher: this.#mount.githubWrite } + return { + identity: 'app', + publisher: this.#mount.githubWrite, + } } if (configured === 'app') { throw new Error( @@ -6804,10 +6856,19 @@ export class FactoryLoop implements Factory { : undefined const repo = normalizeGithubRepo(implementer.spec.repo, this.#config.repos.org ?? sourceOwner) const lifecycle = await this.#state.getDispatchLifecycle(this.#workspaceId, issueKey(record.issue)) - if (publishedPullRequests(lifecycle).some((receipt) => + const durableReceipt = publishedPullRequests(lifecycle).find((receipt) => receipt.repo.toLowerCase() === repo.toLowerCase() - )) return true - return Boolean(await this.#openPullRequestByHead(repo, implementer.spec.branch)) + ) + if (durableReceipt) { + await this.#requestAutomatedPullRequestReview(durableReceipt) + return true + } + const existing = await this.#openPullRequestByHead(repo, implementer.spec.branch) + if (existing) { + await this.#requestAutomatedPullRequestReview(existing) + return true + } + return false } // Only a NON-DRAFT (ready) PR counts as completion. A draft PR means the // work isn't review-ready, so an implementer exiting with only a draft PR @@ -6816,7 +6877,12 @@ export class FactoryLoop implements Factory { const pr = opts.openOnly ? await this.#openPrForIssue(issue) : await this.#completionPrForIssue(issue) - return Boolean(pr && !pr.draft) + if (!pr || pr.draft) return false + await this.#requestAutomatedPullRequestReview({ + repo: pr.repo, + number: pr.prNumber, + }) + return true } catch (error) { this.#logger.warn?.('[factory] PR probe failed after implementer exit; preserving restart behavior', { issue: record.issue.key, @@ -15096,6 +15162,8 @@ const liveHeartbeatIntervalMs = (staleMs: number): number => const installFactoryDraftPredicate = (mount: MountClient, config: FactoryConfig): void => { mount.setDefaultAllowedDraftPredicate?.((path, content, opts) => isAllowedFactoryDraft(path, content, opts, mount, config)) + mount.setDefaultAllowedDeletePredicate?.((path, content) => + isAllowedFactoryGithubWritebackDraft(path, content)) } const isAllowedFactoryDraft = async ( @@ -15123,16 +15191,13 @@ const isAllowedFactoryDraft = async ( return true } - if (isFactoryGithubWritebackPath(path)) { + if (isAllowedFactoryGithubWritebackDraft(path, content)) { return true } return false } -const isFactoryGithubWritebackPath = (path: string): boolean => - /^\/github\/repos\/[^/]+\/[^/]+\/(?:pull-requests\/factory-[^/]+\.json|refs\/(?:factory\.json|refs%2Fheads%2Ffactory%2F[^/]+\.json)|pulls\/[1-9]\d*\/close\.json)$/iu.test(path) - const isIssuePathInFactoryScope = async ( mount: MountClient, path: string, diff --git a/src/ports/index.ts b/src/ports/index.ts index 971bbd8..5569352 100644 --- a/src/ports/index.ts +++ b/src/ports/index.ts @@ -8,6 +8,7 @@ export type { GithubConnectionWrite, GithubPublishPullRequestInput, GithubPublishPullRequestResult, + GithubPullRequestRef, LocalMountOptions, MountClient, ProviderSyncStatus, diff --git a/src/ports/mount.ts b/src/ports/mount.ts index f253201..84d6cf6 100644 --- a/src/ports/mount.ts +++ b/src/ports/mount.ts @@ -65,6 +65,11 @@ export interface GithubPublishPullRequestResult { author?: string } +export interface GithubPullRequestRef { + repo: string + number: number +} + export type FactoryIntegrationProvider = 'github' | 'linear' export interface FactoryIntegrationConnectionStatus { @@ -93,6 +98,7 @@ export interface FactoryIntegrationConnections { */ export interface GithubConnectionWrite { publishPullRequest(input: GithubPublishPullRequestInput): Promise + requestPullRequestReview?(input: GithubPullRequestRef): Promise closePullRequest(input: { repo: string; number: number }): Promise } @@ -124,12 +130,18 @@ export interface MountClient { setDefaultAllowedDraftPredicate?( predicate: (path: string, content: unknown, opts?: { guarded?: boolean }) => boolean | Promise, ): void + setDefaultAllowedDeletePredicate?( + predicate: (path: string, content: unknown) => boolean | Promise, + ): void listTree(prefix: string): Promise subscribe(globs: string[], onChange: (event: ChangeEvent) => void, opts?: SubscribeOptions): Subscription getEvents(opts: { cursor?: string; limit?: number; provider?: string; last?: number }): Promise getEventHighWatermark?(opts?: { provider?: string }): Promise getSyncStatus?(provider: string): Promise - confirmWrite(path: string, opts?: { timeoutMs?: number }): Promise<'acked' | 'pending' | 'failed' | 'timeout'> + confirmWrite( + path: string, + opts?: { timeoutMs?: number; returnFailed?: boolean }, + ): Promise<'acked' | 'pending' | 'failed' | 'timeout'> /** Provider failure detail retained for a completed failed write, when available. */ getConfirmedWriteFailureReason?(path: string): Promise /** Provider object id returned by the acknowledged write operation, when available. */ diff --git a/src/ports/writeback.ts b/src/ports/writeback.ts index d28a27a..edfd615 100644 --- a/src/ports/writeback.ts +++ b/src/ports/writeback.ts @@ -1,4 +1,4 @@ -import type { GithubPublishPullRequestInput, GithubPublishPullRequestResult } from './mount' +import type { GithubPublishPullRequestInput, GithubPublishPullRequestResult, GithubPullRequestRef } from './mount' import type { LinearIssue, PrSummary } from '../types' export interface LinearWriteback { @@ -22,6 +22,8 @@ export type GithubIssueStatus = 'ready' | 'in-progress' | 'human-review' export interface GithubWriteback { /** Optional local-user PR publisher, implemented by the default `gh` writeback. */ publishPullRequest?(input: GithubPublishPullRequestInput): Promise + /** Request automated review through the same GitHub identity used to publish. */ + requestPullRequestReview?(input: GithubPullRequestRef): Promise /** Provider-authoritative fallback when the mounted issue record omits its reporter. */ getIssueAuthor?(issue: LinearIssue): Promise /** Provider-authoritative lifecycle lookup used before recovering stale mounted labels. */ diff --git a/src/writeback/github.ts b/src/writeback/github.ts index a7595de..330b19e 100644 --- a/src/writeback/github.ts +++ b/src/writeback/github.ts @@ -5,6 +5,7 @@ import type { MountClient } from '../ports' import type { GithubPublishPullRequestInput, GithubPublishPullRequestResult } from '../ports/mount' import type { GithubIssueStatus, GithubWriteback } from '../ports/writeback' import { defaultGhRunner, type GhRunner } from '../github/merge-gate' +import { FACTORY_CODERABBIT_REVIEW_BODY, containsCoderabbitReviewRequest } from '../github/review-request' import type { LinearIssue, PrSummary } from '../types' import { asRecord, wrappedPayload } from './shared' @@ -74,6 +75,7 @@ export interface GhCliGithubWritebackConfig { export class GhCliGithubWriteback implements GithubWriteback { readonly #run: GhRunner readonly #git: GhRunner + readonly #reviewRequests = new Map>() constructor(config: GhCliGithubWritebackConfig = {}) { this.#run = config.runner ?? defaultGhRunner @@ -155,6 +157,38 @@ export class GhCliGithubWriteback implements GithubWriteback { } } + async requestPullRequestReview(input: { repo: string; number: number }): Promise { + const requestKey = `${input.repo.toLowerCase()}#${input.number}` + const existing = this.#reviewRequests.get(requestKey) + if (existing) return existing + const request = this.#requestPullRequestReview(input) + .catch((error: unknown) => { + this.#reviewRequests.delete(requestKey) + throw error + }) + this.#reviewRequests.set(requestKey, request) + return request + } + + async #requestPullRequestReview(input: { repo: string; number: number }): Promise { + const comments = await this.#run([ + 'api', + '--paginate', + '--slurp', + `repos/${input.repo}/issues/${input.number}/comments`, + ]) + if (githubCommentBodies(comments.stdout).some(containsCoderabbitReviewRequest)) return + await this.#run([ + 'pr', + 'comment', + String(input.number), + '--repo', + input.repo, + '--body', + FACTORY_CODERABBIT_REVIEW_BODY, + ]) + } + async getIssueAuthor(issue: LinearIssue): Promise { const ref = githubIssueRef(issue) const result = await this.#run([ @@ -296,6 +330,21 @@ export class GhCliGithubWriteback implements GithubWriteback { } } +const githubCommentBodies = (stdout: string): string[] => { + const payload: unknown = JSON.parse(stdout) + const bodies: string[] = [] + const visit = (value: unknown): void => { + if (Array.isArray(value)) { + for (const item of value) visit(item) + return + } + const body = asRecord(value)?.body + if (typeof body === 'string') bodies.push(body) + } + visit(payload) + return bodies +} + const defaultGitRunner: GhRunner = async (args) => { const { stdout, stderr } = await execFileAsync('git', args, { maxBuffer: 1024 * 1024 }) return { stdout, stderr } diff --git a/src/writeback/writeback.test.ts b/src/writeback/writeback.test.ts index a99152b..d87fe03 100644 --- a/src/writeback/writeback.test.ts +++ b/src/writeback/writeback.test.ts @@ -887,6 +887,89 @@ describe('GhCliGithubWriteback', () => { ]) }) + it('requests CodeRabbit review once through the authenticated gh user', async () => { + const calls: string[][] = [] + let comments = '[[]]' + const github = new GhCliGithubWriteback({ + runner: async (args) => { + calls.push(args) + if (args[0] === 'api') return { stdout: comments } + comments = JSON.stringify([[ + { body: '@coderabbitai review\n' }, + ]]) + return { stdout: '' } + }, + }) + + const input = { repo: 'AgentWorkforce/factory', number: 124 } + await Promise.all([ + github.requestPullRequestReview(input), + github.requestPullRequestReview(input), + ]) + await github.requestPullRequestReview(input) + + expect(calls).toEqual([ + [ + 'api', '--paginate', '--slurp', + 'repos/AgentWorkforce/factory/issues/124/comments', + ], + [ + 'pr', 'comment', '124', '--repo', 'AgentWorkforce/factory', + '--body', '@coderabbitai review\n', + ], + ]) + }) + + it('does not let a marker-only public comment suppress the review command', async () => { + const calls: string[][] = [] + const github = new GhCliGithubWriteback({ + runner: async (args) => { + calls.push(args) + return { + stdout: args[0] === 'api' + ? JSON.stringify([[ + { body: '' }, + ]]) + : '', + } + }, + }) + + await github.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 124, + }) + + expect(calls.at(-1)).toEqual([ + 'pr', 'comment', '124', '--repo', 'AgentWorkforce/factory', + '--body', '@coderabbitai review\n', + ]) + }) + + it('does not combine the review command and marker across public comments', async () => { + const calls: string[][] = [] + const github = new GhCliGithubWriteback({ + runner: async (args) => { + calls.push(args) + return { + stdout: args[0] === 'api' + ? JSON.stringify([[ + { body: '@coderabbitai review' }, + { body: '' }, + ]]) + : '', + } + }, + }) + + await github.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 124, + }) + + expect(calls.at(-1)?.slice(0, 2)).toEqual(['pr', 'comment']) + }) + it('resolves the issue reporter from GitHub when the mounted payload omits it', async () => { const calls: string[][] = [] const github = new GhCliGithubWriteback({ From bbee21fc00f5de20172e8afc53365cdc13e5aa4f Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 04:22:09 -0400 Subject: [PATCH 02/30] test: permit guarded review draft cleanup --- src/__tests__/mount-delete-callsite-invariant.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/__tests__/mount-delete-callsite-invariant.test.ts b/src/__tests__/mount-delete-callsite-invariant.test.ts index 58de7fa..873fdee 100644 --- a/src/__tests__/mount-delete-callsite-invariant.test.ts +++ b/src/__tests__/mount-delete-callsite-invariant.test.ts @@ -8,6 +8,8 @@ const SDK_ROOT = resolve(fileURLToPath(new URL('..', import.meta.url))) const ALLOWED_DELETE_CALLSITES = new Set([ 'writeback/linear.ts', 'writeback/slack.ts', + // Deletes only the exact review draft after its publish operation reaches a terminal state. + 'mount/relayfile-github-connection-write.ts', ]) describe('mount.deleteFile callsite invariant', () => { From 067f7f12cd7a8aee4c309cb300d42d2ebec9e073 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 04:48:00 -0400 Subject: [PATCH 03/30] fix: decouple automated review requests --- src/orchestrator/factory.test.ts | 42 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 32 +++++++++++++++--------- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 46be5ed..e80cc11 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10021,6 +10021,48 @@ describe('FactoryLoop', () => { }, ) + it('does not let a pending automated review request stall lifecycle completion', async () => { + const number = 524 + const reviewRequests: Array<{ repo: string; number: number }> = [] + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + await new Promise(() => undefined) + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + }) + + try { + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + + await vi.waitFor(() => expect(reviewRequests).toEqual([{ + repo: 'AgentWorkforce/pear', + number, + }])) + await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + } finally { + await factory.stop() + } + }) + it('publishes an implementer PR through the mount connection on successful completion', async () => { class OrderedPreviewFleetClient extends FakeFleetClient { readonly terminalEvents: string[] = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 40eb972..25c6ae7 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -1564,7 +1564,7 @@ export class FactoryLoop implements Factory { this.#probePrGhBackoffUntilMs.set(issueStateKey(issueRef(issue)), this.#clock.now() + PROBE_PR_GH_BACKOFF_MS) return undefined } - await this.#requestAutomatedPullRequestReview({ + this.#requestAutomatedPullRequestReview({ repo: pr.repo, number: pr.prNumber, }) @@ -6104,7 +6104,7 @@ export class FactoryLoop implements Factory { const { identity, publisher } = this.#githubPullRequestPublisher() const cached = this.#publishedPullRequests.get(key) if (cached) { - await this.#requestAutomatedPullRequestReview(cached) + this.#requestAutomatedPullRequestReview(cached) return cached } const remoteBranch = implementer.result?.locality === 'remote' && implementer.spec.branch @@ -6134,14 +6134,14 @@ export class FactoryLoop implements Factory { (!opts.reconcileExisting || !expectedHeadRef || durableReceipt.headRef === expectedHeadRef) ) { this.#publishedPullRequests.set(key, durableReceipt) - await this.#requestAutomatedPullRequestReview(durableReceipt) + this.#requestAutomatedPullRequestReview(durableReceipt) return durableReceipt } if (opts.reconcileExisting && expectedHeadRef) { const existing = await this.#openPullRequestByHead(repo, expectedHeadRef) if (existing) { this.#publishedPullRequests.set(key, existing) - await this.#requestAutomatedPullRequestReview(existing) + this.#requestAutomatedPullRequestReview(existing) this.#increment('githubPullRequestsReconciled') this.#logger.info?.('[factory] reconciled existing PR from implementer branch', { issue: issue.key, @@ -6185,20 +6185,30 @@ export class FactoryLoop implements Factory { identity, author: published.author, }) - await this.#requestAutomatedPullRequestReview(published) + this.#requestAutomatedPullRequestReview(published) return published } - async #requestAutomatedPullRequestReview( + #requestAutomatedPullRequestReview( published: GithubPullRequestRef, - ): Promise { + ): void { const key = `${published.repo.toLowerCase()}#${published.number}` if (this.#reviewRequestedPullRequests.has(key)) return try { const requestReview = this.#githubPullRequestReviewRequester() if (!requestReview) return - await requestReview({ repo: published.repo, number: published.number }) this.#reviewRequestedPullRequests.add(key) + void Promise.resolve() + .then(() => requestReview({ repo: published.repo, number: published.number })) + .catch((error: unknown) => { + this.#reviewRequestedPullRequests.delete(key) + this.#increment('githubPullRequestReviewRequestFailures') + this.#logger.warn?.('[factory] automated PR review request failed; lifecycle completion remains independent', { + repo: published.repo, + prNumber: published.number, + error: describeError(error).errorMessage, + }) + }) } catch (error) { this.#increment('githubPullRequestReviewRequestFailures') this.#logger.warn?.('[factory] automated PR review request failed; lifecycle completion remains independent', { @@ -6860,12 +6870,12 @@ export class FactoryLoop implements Factory { receipt.repo.toLowerCase() === repo.toLowerCase() ) if (durableReceipt) { - await this.#requestAutomatedPullRequestReview(durableReceipt) + this.#requestAutomatedPullRequestReview(durableReceipt) return true } const existing = await this.#openPullRequestByHead(repo, implementer.spec.branch) if (existing) { - await this.#requestAutomatedPullRequestReview(existing) + this.#requestAutomatedPullRequestReview(existing) return true } return false @@ -6878,7 +6888,7 @@ export class FactoryLoop implements Factory { ? await this.#openPrForIssue(issue) : await this.#completionPrForIssue(issue) if (!pr || pr.draft) return false - await this.#requestAutomatedPullRequestReview({ + this.#requestAutomatedPullRequestReview({ repo: pr.repo, number: pr.prNumber, }) From 7b433399e669cea4a039df13470b06acbfaea0b3 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 04:56:13 -0400 Subject: [PATCH 04/30] fix: close automated review edge cases --- .../relayfile-cloud-mount-client.test.ts | 39 +++++++++ src/mount/relayfile-cloud-mount-client.ts | 38 ++++++-- src/orchestrator/factory.test.ts | 87 ++++++++++++++++++- src/orchestrator/factory.ts | 22 +++-- 4 files changed, 167 insertions(+), 19 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index e0920e2..66a0558 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -5,6 +5,7 @@ import type { ChangeEvent, ClaimDurableSubscriptionDeliveriesInput, CreateOrRenewDurableResourceSubscriptionInput, + OperationFeedResponse, OperationStatusResponse, } from '@relayfile/sdk' import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' @@ -1345,6 +1346,44 @@ describe('RelayfileCloudMountClient', () => { expect(fake.deleteFileCalls).toHaveLength(1) }) + it('bounds restarted operation recovery by the confirmation timeout', async () => { + class HangingListOpsClient extends FakeRelayFileClient { + override async listOps(): Promise { + await new Promise(() => undefined) + } + } + const fake = new HangingListOpsClient() + const restartedMount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await expect(restartedMount.confirmWrite('/github/repos/AgentWorkforce/factory/pulls/85/comments/review.json', { + timeoutMs: 5, + })).resolves.toBe('timeout') + expect(fake.getOpCalls).toEqual([]) + }) + + it('treats restarted operation lookup failures as unavailable recovery data', async () => { + class FailingListOpsClient extends FakeRelayFileClient { + override async listOps(): Promise { + throw new Error('relayfile unavailable') + } + } + const fake = new FailingListOpsClient() + const restartedMount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await expect(restartedMount.confirmWrite('/github/repos/AgentWorkforce/factory/pulls/85/comments/review.json', { + timeoutMs: 5, + })).resolves.toBe('timeout') + expect(fake.getOpCalls).toEqual([]) + }) + it('fails closed when restarted write operations have the same latest timestamp', async () => { const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' const fake = new FakeRelayFileClient() diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index b7dd075..08754ac 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -788,10 +788,10 @@ export class RelayfileCloudMountClient implements MountClient { path: string, opts: { timeoutMs?: number; returnFailed?: boolean } = {}, ): Promise<'acked' | 'pending' | 'failed' | 'timeout'> { - const opId = this.#lastOpByPath.get(path) ?? await this.#recoverLatestWriteOperation(path) + const deadline = Date.now() + (opts.timeoutMs ?? 90_000) + const opId = this.#lastOpByPath.get(path) ?? await this.#recoverLatestWriteOperation(path, deadline) if (!opId || !this.#client.getOp) return 'timeout' - const deadline = Date.now() + (opts.timeoutMs ?? 90_000) for (;;) { const operation = await this.#client.getOp(this.workspaceId, opId) let status: 'acked' | 'pending' | 'failed' @@ -823,17 +823,18 @@ export class RelayfileCloudMountClient implements MountClient { } } - async #recoverLatestWriteOperation(path: string): Promise { + async #recoverLatestWriteOperation(path: string, deadline: number): Promise { if (!this.#client.listOps) return undefined let cursor: string | undefined const matching: OperationStatusResponse[] = [] do { - const page = await this.#client.listOps(this.workspaceId, { - action: 'file_upsert', - provider: providerForPath(path), - cursor, - limit: 100, - }) + const page = await settleBeforeDeadline(this.#client.listOps(this.workspaceId, { + action: 'file_upsert', + provider: providerForPath(path), + cursor, + limit: 100, + }), deadline) + if (!page) return undefined for (const operation of page.items) { if (operation.path === path) matching.push(operation) } @@ -962,6 +963,25 @@ const isHttpStatus = (error: unknown, status: number): boolean => { return record?.status === status || record?.statusCode === status } +const settleBeforeDeadline = async (operation: Promise, deadline: number): Promise => { + const remainingMs = deadline - Date.now() + if (remainingMs <= 0) return undefined + let timer: ReturnType | undefined + try { + return await Promise.race([ + operation, + new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), remainingMs) + timer.unref?.() + }), + ]) + } catch { + return undefined + } finally { + if (timer) clearTimeout(timer) + } +} + const mapOperationStatus = ( response: OperationStatusResponse, ): 'acked' | 'pending' | 'failed' => { diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index e80cc11..05891bd 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -3727,7 +3727,7 @@ describe('FactoryLoop', () => { triage: new StaticTriage(), githubWriteback, mergeGate, - probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 50 }), + probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 50, state: 'OPEN' }), }) await factory.runOnce() @@ -10063,6 +10063,91 @@ describe('FactoryLoop', () => { } }) + it('keeps provider delete authorization aligned with guarded draft authorization', async () => { + class DeletePredicateMount extends FakeMountClient { + deletePredicate?: (path: string, content: unknown) => boolean | Promise + + setDefaultAllowedDeletePredicate( + predicate: (path: string, content: unknown) => boolean | Promise, + ): void { + this.deletePredicate ??= predicate + } + } + const number = 525 + const mount = new DeletePredicateMount({ + [issuePath(number)]: issueFile(number), + }) + const factory = createFactory(config(), { + mount, + fleet: new FakeFleetClient(), + triage: new StaticTriage(), + }) + + try { + expect(await mount.deletePredicate?.( + `/linear/issues/AR-${number}__uuid-${number}/comments/factory-draft.json`, + { body: 'status update' }, + )).toBe(true) + expect(await mount.deletePredicate?.( + '/slack/channels/C123/messages/factory-draft.json', + { text: 'status update' }, + )).toBe(true) + expect(await mount.deletePredicate?.( + '/github/repos/AgentWorkforce/factory/pulls/525/comments/factory-coderabbit-review.json', + { body: '@coderabbitai review\n' }, + )).toBe(true) + expect(await mount.deletePredicate?.('/github/repos/AgentWorkforce/factory/arbitrary.json', {})) + .toBe(false) + } finally { + await factory.stop() + } + }) + + it('does not request automated review for an already-closed completion PR', async () => { + const number = 526 + const reviewRequests: Array<{ repo: string; number: number }> = [] + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + }, { + publishPullRequest: async () => { + throw new Error('must use the existing merged PR') + }, + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + }, + closePullRequest: async () => undefined, + }) + const fleet = new FakeFleetClient() + const factory = createFactory(config({ + github: { identity: 'app' }, + babysitter: { enabled: false }, + }), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => ({ + repo: 'AgentWorkforce/pear', + prNumber: number, + state: 'MERGED', + }), + probeCloser: async (input) => ({ + repo: input.repo, + prNumber: input.prNumber, + state: 'CLOSED', + }), + }) + + try { + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + + await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + expect(reviewRequests).toEqual([]) + } finally { + await factory.stop() + } + }) + it('publishes an implementer PR through the mount connection on successful completion', async () => { class OrderedPreviewFleetClient extends FakeFleetClient { readonly terminalEvents: string[] = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 25c6ae7..b85e6a8 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -1564,10 +1564,12 @@ export class FactoryLoop implements Factory { this.#probePrGhBackoffUntilMs.set(issueStateKey(issueRef(issue)), this.#clock.now() + PROBE_PR_GH_BACKOFF_MS) return undefined } - this.#requestAutomatedPullRequestReview({ - repo: pr.repo, - number: pr.prNumber, - }) + if (normalizePrState(pr.state) === 'OPEN') { + this.#requestAutomatedPullRequestReview({ + repo: pr.repo, + number: pr.prNumber, + }) + } if (record.decision.implementers.length > 1 && !await this.#allImplementersHaveCompletionPr(record)) { this.#increment('completionSweepMissingPr') return undefined @@ -6888,10 +6890,12 @@ export class FactoryLoop implements Factory { ? await this.#openPrForIssue(issue) : await this.#completionPrForIssue(issue) if (!pr || pr.draft) return false - this.#requestAutomatedPullRequestReview({ - repo: pr.repo, - number: pr.prNumber, - }) + if (normalizePrState(pr.state) === 'OPEN') { + this.#requestAutomatedPullRequestReview({ + repo: pr.repo, + number: pr.prNumber, + }) + } return true } catch (error) { this.#logger.warn?.('[factory] PR probe failed after implementer exit; preserving restart behavior', { @@ -15173,7 +15177,7 @@ const installFactoryDraftPredicate = (mount: MountClient, config: FactoryConfig) mount.setDefaultAllowedDraftPredicate?.((path, content, opts) => isAllowedFactoryDraft(path, content, opts, mount, config)) mount.setDefaultAllowedDeletePredicate?.((path, content) => - isAllowedFactoryGithubWritebackDraft(path, content)) + isAllowedFactoryDraft(path, content, { guarded: true }, mount, config)) } const isAllowedFactoryDraft = async ( From 4db9988c36d20221171d682e1439a6f74765121d Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:00:09 -0400 Subject: [PATCH 05/30] fix: verify review targets remain open --- src/mount/relayfile-cloud-mount-client.ts | 10 +++--- .../relayfile-github-connection-write.test.ts | 31 +++++++++++++++++++ .../relayfile-github-connection-write.ts | 4 ++- src/orchestrator/factory.ts | 30 ++++++++++++++++-- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index 08754ac..f7c2075 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -829,11 +829,11 @@ export class RelayfileCloudMountClient implements MountClient { const matching: OperationStatusResponse[] = [] do { const page = await settleBeforeDeadline(this.#client.listOps(this.workspaceId, { - action: 'file_upsert', - provider: providerForPath(path), - cursor, - limit: 100, - }), deadline) + action: 'file_upsert', + provider: providerForPath(path), + cursor, + limit: 100, + }), deadline) if (!page) return undefined for (const operation of page.items) { if (operation.path === path) matching.push(operation) diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index 998d732..7b120fb 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -125,6 +125,37 @@ describe('RelayfileGithubConnectionWrite', () => { expect(mount.deletes).toEqual([]) }) + it('gives restarted cloud operation recovery a realistic bounded confirmation window', async () => { + const draftPath = '/github/repos/AgentWorkforce/factory/pulls/86/comments/factory-coderabbit-review.json' + class ConfirmationOptionsMount extends FakeMountClient { + readonly options: Array<{ timeoutMs?: number; returnFailed?: boolean } | undefined> = [] + + override async confirmWrite( + _path: string, + opts?: { timeoutMs?: number; returnFailed?: boolean }, + ): Promise<'acked'> { + this.options.push(opts) + return 'acked' + } + } + const mount = new ConfirmationOptionsMount({ + [draftPath]: { + body: '@coderabbitai review\n', + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 86, + })).resolves.toBeUndefined() + + expect(mount.options).toEqual([{ + timeoutMs: 10_000, + returnFailed: true, + }]) + }) + it('fails closed on an indeterminate review draft instead of posting a duplicate', async () => { const draftPath = '/github/repos/AgentWorkforce/factory/pulls/86/comments/factory-coderabbit-review.json' const mount = new FakeMountClient({ diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index fb6f164..f549cc2 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -13,6 +13,8 @@ import { isAllowedFactoryGithubWritebackDraft, } from '../github/review-request' +const REVIEW_REQUEST_CONFIRM_TIMEOUT_MS = 10_000 + const execFileAsync = promisify(execFile) const WRITE_CONFIRM_TIMEOUT_MS = 90_000 const RECEIPT_READ_ATTEMPTS = 5 @@ -186,7 +188,7 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { // itself as evidence that GitHub received the request. if (isAllowedFactoryGithubWritebackDraft(path, content)) { const status = await this.#mount.confirmWrite(path, { - timeoutMs: 1, + timeoutMs: REVIEW_REQUEST_CONFIRM_TIMEOUT_MS, returnFailed: true, }) if (status === 'acked') return true diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index b85e6a8..77d4753 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -521,6 +521,7 @@ export class FactoryLoop implements Factory { readonly #babysitterCriticalAgents = new Set() readonly #publishedPullRequests = new Map() readonly #reviewRequestedPullRequests = new Set() + readonly #reviewRequestVerifications = new Set() readonly #previewReferences = new Map() readonly #removedPreviewIds = new Set() readonly #probePrGhBackoffUntilMs = new Map() @@ -6106,7 +6107,7 @@ export class FactoryLoop implements Factory { const { identity, publisher } = this.#githubPullRequestPublisher() const cached = this.#publishedPullRequests.get(key) if (cached) { - this.#requestAutomatedPullRequestReview(cached) + this.#requestAutomatedPullRequestReviewForOpenReceipt(cached) return cached } const remoteBranch = implementer.result?.locality === 'remote' && implementer.spec.branch @@ -6136,7 +6137,7 @@ export class FactoryLoop implements Factory { (!opts.reconcileExisting || !expectedHeadRef || durableReceipt.headRef === expectedHeadRef) ) { this.#publishedPullRequests.set(key, durableReceipt) - this.#requestAutomatedPullRequestReview(durableReceipt) + this.#requestAutomatedPullRequestReviewForOpenReceipt(durableReceipt) return durableReceipt } if (opts.reconcileExisting && expectedHeadRef) { @@ -6221,6 +6222,29 @@ export class FactoryLoop implements Factory { } } + #requestAutomatedPullRequestReviewForOpenReceipt( + published: GithubPublishPullRequestResult, + ): void { + const key = `${published.repo.toLowerCase()}#${published.number}` + if (this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestVerifications.has(key)) return + this.#reviewRequestVerifications.add(key) + void this.#openPullRequestByHead(published.repo, published.headRef) + .then((open) => { + if (open?.number === published.number) { + this.#requestAutomatedPullRequestReview(open) + } + }) + .catch((error: unknown) => { + this.#increment('githubPullRequestReviewRequestFailures') + this.#logger.warn?.('[factory] automated PR review request skipped; unable to verify receipt is still open', { + repo: published.repo, + prNumber: published.number, + error: describeError(error).errorMessage, + }) + }) + .finally(() => this.#reviewRequestVerifications.delete(key)) + } + #githubPullRequestReviewRequester(): ((input: GithubPullRequestRef) => Promise) | undefined { const configured = this.#config.github.identity if (configured !== 'user' && this.#mount.githubWrite) { @@ -6872,7 +6896,7 @@ export class FactoryLoop implements Factory { receipt.repo.toLowerCase() === repo.toLowerCase() ) if (durableReceipt) { - this.#requestAutomatedPullRequestReview(durableReceipt) + this.#requestAutomatedPullRequestReviewForOpenReceipt(durableReceipt) return true } const existing = await this.#openPullRequestByHead(repo, implementer.spec.branch) From f0545bf0dbb8fe6e4486f4fe54df33da28c23893 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:06:01 -0400 Subject: [PATCH 06/30] perf: bound review request reconciliation --- .../relayfile-github-connection-write.test.ts | 15 ++++++++++--- .../relayfile-github-connection-write.ts | 21 ++++++++----------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index 7b120fb..05fea39 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -15,6 +15,12 @@ const gitRunnerForBranch = (branch: string): GitCommandRunner => vi.fn(async (ar throw new Error(`unexpected git args: ${args.join(' ')}`) }) +const githubEvent = (id: string, path: string): Parameters[0] => ({ + id, + type: 'relayfile.changed', + resource: { provider: 'github', path }, +} as Parameters[0]) + describe('RelayfileGithubConnectionWrite', () => { it('requests CodeRabbit review once through the connected app write path', async () => { const mount = new FakeMountClient() @@ -185,7 +191,7 @@ describe('RelayfileGithubConnectionWrite', () => { override async listTree(prefix: string): Promise { this.listPrefixes.push(prefix) - if (prefix.endsWith('/pulls')) { + if (prefix.endsWith('/pulls/87')) { return ['/github/repos/AgentWorkforce/factory/pulls/87__renamed'] } if (prefix.endsWith('/pulls/87__renamed/comments')) return [stalePath] @@ -216,10 +222,10 @@ describe('RelayfileGithubConnectionWrite', () => { await expect(write.requestPullRequestReview(input)).resolves.toBeUndefined() expect(mount.writes).toEqual([]) expect(mount.listPrefixes).toEqual([ - '/github/repos/AgentWorkforce/factory/pulls', + '/github/repos/AgentWorkforce/factory/pulls/87', '/github/repos/AgentWorkforce/factory/pulls/87/comments', '/github/repos/AgentWorkforce/factory/pulls/87__renamed/comments', - '/github/repos/AgentWorkforce/factory/pulls', + '/github/repos/AgentWorkforce/factory/pulls/87', '/github/repos/AgentWorkforce/factory/pulls/87/comments', '/github/repos/AgentWorkforce/factory/pulls/87__renamed/comments', ]) @@ -313,6 +319,8 @@ describe('RelayfileGithubConnectionWrite', () => { }, }, }) + mount.emit(githubEvent('canonical-unrelated', unrelatedPath)) + mount.emit(githubEvent('canonical-request', canonicalPath)) const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) await expect(write.requestPullRequestReview({ @@ -336,6 +344,7 @@ describe('RelayfileGithubConnectionWrite', () => { }, }, }) + mount.emit(githubEvent('canonical-other-pr', canonicalPath)) const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) await expect(write.requestPullRequestReview({ diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index f549cc2..9992305 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -14,6 +14,7 @@ import { } from '../github/review-request' const REVIEW_REQUEST_CONFIRM_TIMEOUT_MS = 10_000 +const REVIEW_REQUEST_RECENT_EVENT_LIMIT = 200 const execFileAsync = promisify(execFile) const WRITE_CONFIRM_TIMEOUT_MS = 90_000 @@ -23,7 +24,7 @@ const RECEIPT_READ_DELAY_MS = 100 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string; stderr?: string }> export interface RelayfileGithubConnectionWriteConfig { - mount: Pick + mount: Pick gitRunner?: GitCommandRunner receiptReadAttempts?: number receiptReadDelayMs?: number @@ -157,7 +158,7 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { 'u', ) const commentRoots = new Set([`${pullsRoot}/${number}/comments`]) - for (const path of await this.#listTreeOrEmpty(pullsRoot)) { + for (const path of await this.#listTreeOrEmpty(`${pullsRoot}/${number}`)) { const pullDirectory = pullDirectoryPattern.exec(path)?.[1] if (pullDirectory) commentRoots.add(`${pullsRoot}/${pullDirectory}/comments`) } @@ -215,19 +216,15 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { `^${escapeRegExp(canonicalCommentsRoot)}/[^/]+/(?:meta|metadata)\\.json$`, 'u', ) - const canonicalDirectoryPattern = new RegExp( - `^${escapeRegExp(canonicalCommentsRoot)}/[^/.]+$`, - 'u', - ) const canonicalPaths = new Set() - for (const path of await this.#listTreeOrEmpty(canonicalCommentsRoot)) { + const recentEvents = await this.#mount.getEvents({ + provider: 'github', + last: REVIEW_REQUEST_RECENT_EVENT_LIMIT, + }) + for (const event of recentEvents.events) { + const path = event.resource.path if (canonicalDirectPattern.test(path) || canonicalNestedPattern.test(path)) { canonicalPaths.add(path) - continue - } - if (!canonicalDirectoryPattern.test(path)) continue - for (const nestedPath of await this.#listTreeOrEmpty(path)) { - if (canonicalNestedPattern.test(nestedPath)) canonicalPaths.add(nestedPath) } } for (const path of canonicalPaths) { From 2ae58213ccf65411b96e197ab0ed86c68065491c Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:16:15 -0400 Subject: [PATCH 07/30] fix: query durable review receipts --- .../relayfile-cloud-mount-client.test.ts | 43 +++++++++++++- src/mount/relayfile-cloud-mount-client.ts | 11 ++++ .../relayfile-github-connection-write.test.ts | 53 ++++++++++++++++- .../relayfile-github-connection-write.ts | 57 +++++++++++++------ src/testing/fakes.ts | 18 ++++++ 5 files changed, 160 insertions(+), 22 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index 66a0558..293a2ec 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -61,6 +61,10 @@ class FakeRelayFileClient implements RelayFileClientLike { baseRevision: string }> = [] readonly listTreeCalls: Array<{ workspaceId: string; options?: { path?: string; depth?: number; cursor?: string } }> = [] + readonly queryFilesCalls: Array<{ + workspaceId: string + options?: { path?: string; provider?: string; cursor?: string; limit?: number } + }> = [] readonly getEventsCalls: Array<{ workspaceId: string; opts?: { cursor?: string; limit?: number; provider?: string; last?: number } }> = [] readonly listLastNChangesCalls: Array<{ limit: number; context?: { workspaceId: string } }> = [] readonly getOpCalls: Array<{ workspaceId: string; opId: string }> = [] @@ -159,6 +163,31 @@ class FakeRelayFileClient implements RelayFileClientLike { } } + async queryFiles( + workspaceId: string, + options?: { path?: string; provider?: string; cursor?: string; limit?: number }, + ) { + this.queryFilesCalls.push({ workspaceId, options }) + const paths = [...this.files.keys()] + .filter((path) => path.startsWith(options?.path ?? '/')) + .sort() + const start = options?.cursor + ? Math.max(0, paths.findIndex((path) => path === options.cursor) + 1) + : 0 + const limit = options?.limit ?? paths.length + const page = paths.slice(start, start + limit) + return { + items: page.map((path) => ({ + path, + revision: this.files.get(path)?.revision ?? '1', + contentType: this.files.get(path)?.contentType ?? 'application/json', + provider: options?.provider, + size: this.files.get(path)?.content.length ?? 0, + })), + nextCursor: page.length >= limit ? page.at(-1) ?? null : null, + } + } + async getEvents(workspaceId: string, opts?: { cursor?: string; limit?: number; provider?: string; last?: number }) { this.getEventsCalls.push({ workspaceId, opts }) return { events: this.events, nextCursor: null } @@ -978,7 +1007,7 @@ describe('RelayfileCloudMountClient', () => { .rejects.toThrow('Relayfile cloud session required; run `agent-relay login`') }) - it('delegates readFile/listTree/getEvents with the configured workspace id', async () => { + it('delegates readFile/listTree/queryFiles/getEvents with the configured workspace id', async () => { const fake = new FakeRelayFileClient() fake.files.set('/linear/issues/AR-1.json', { revision: '7', @@ -992,6 +1021,14 @@ describe('RelayfileCloudMountClient', () => { revision: '7', }) await expect(mount.listTree('/linear/issues')).resolves.toEqual(['/linear/issues/AR-1.json']) + await expect(mount.queryFiles({ + path: '/linear/issues', + provider: 'linear', + limit: 100, + })).resolves.toEqual({ + paths: ['/linear/issues/AR-1.json'], + nextCursor: null, + }) await expect(mount.getEvents({ cursor: 'evt-0', limit: 10 })).resolves.toMatchObject({ events: fake.events, nextCursor: null, @@ -1002,6 +1039,10 @@ describe('RelayfileCloudMountClient', () => { workspaceId: 'rw_test', options: { path: '/linear/issues', cursor: undefined }, }) + expect(fake.queryFilesCalls[0]).toEqual({ + workspaceId: 'rw_test', + options: { path: '/linear/issues', provider: 'linear', limit: 100 }, + }) expect(fake.getEventsCalls[0]).toEqual({ workspaceId: 'rw_test', opts: { cursor: 'evt-0', limit: 10 } }) }) diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index f7c2075..52ebd1a 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -10,6 +10,7 @@ import { type ChangeEvent, type DeleteFileInput, type EventFeedResponse, + type FileQueryResponse, type FileReadResponse, type GetEventsOptions, type GetOperationsOptions, @@ -17,6 +18,7 @@ import { type OperationFeedResponse, type ResourceAtEventResult, type OperationStatusResponse, + type QueryFilesOptions, type Subscription, type TreeResponse, type WriteFileInput, @@ -221,6 +223,7 @@ export type RelayFileClientLike = { writeFile(input: WriteFileInput): Promise deleteFile(input: DeleteFileInput): Promise listTree(workspaceId: string, options?: ListTreeOptions): Promise + queryFiles(workspaceId: string, options?: QueryFilesOptions): Promise getEvents(workspaceId: string, options?: GetEventsOptions): Promise listLastNChanges?(limit: number, context?: { workspaceId: string; token?: string }): Promise<{ events: ChangeEvent[] }> getResourceAtEvent(eventId: string, context?: { workspaceId: string; token?: string }): Promise @@ -770,6 +773,14 @@ export class RelayfileCloudMountClient implements MountClient { } } + async queryFiles(opts: QueryFilesOptions): Promise<{ paths: string[]; nextCursor: string | null }> { + const response = await this.#client.queryFiles(this.workspaceId, opts) + return { + paths: response.items.map((item) => item.path), + nextCursor: response.nextCursor, + } + } + async getEventHighWatermark(opts: { provider?: string } = {}): Promise { if (!this.#client.listLastNChanges) return undefined const response = await this.#client.listLastNChanges(10, { workspaceId: this.workspaceId }) diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index 05fea39..2704917 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -319,8 +319,6 @@ describe('RelayfileGithubConnectionWrite', () => { }, }, }) - mount.emit(githubEvent('canonical-unrelated', unrelatedPath)) - mount.emit(githubEvent('canonical-request', canonicalPath)) const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) await expect(write.requestPullRequestReview({ @@ -332,6 +330,56 @@ describe('RelayfileGithubConnectionWrite', () => { expect(mount.reads).toEqual([unrelatedPath, canonicalPath]) }) + it('paginates the current canonical comment tree and skips a concurrently deleted result', async () => { + const canonicalRoot = '/github/repos/AgentWorkforce/factory/comments' + const stalePath = `${canonicalRoot}/9007.json` + const canonicalPath = `${canonicalRoot}/9008.json` + class PaginatedCanonicalCommentsMount extends FakeMountClient { + readonly queryCalls: Array<{ path: string; provider?: string; cursor?: string; limit?: number }> = [] + + override async queryFiles(opts: { + path: string + provider?: string + cursor?: string + limit?: number + }): Promise<{ paths: string[]; nextCursor: string | null }> { + this.queryCalls.push(opts) + return opts.cursor + ? { paths: [canonicalPath], nextCursor: null } + : { paths: [stalePath], nextCursor: 'canonical-page-2' } + } + + override async readFile(path: string): Promise<{ content: unknown; revision?: string }> { + if (path === stalePath) { + throw Object.assign(new Error(`File not found: ${path}`), { code: 'file_not_found' }) + } + return super.readFile(path) + } + } + const mount = new PaginatedCanonicalCommentsMount({ + [canonicalPath]: { + repository: { full_name: 'AgentWorkforce/factory' }, + pull_request: { number: 90 }, + comment: { + body: '@coderabbitai review\n', + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 90, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.queryCalls).toEqual([ + { path: canonicalRoot, provider: 'github', cursor: undefined, limit: 100 }, + { path: canonicalRoot, provider: 'github', cursor: 'canonical-page-2', limit: 100 }, + ]) + expect(mount.reads).toEqual([canonicalPath]) + }) + it('does not let another pull request canonical comment suppress a request', async () => { const canonicalPath = '/github/repos/AgentWorkforce/factory/comments/9006.json' @@ -344,7 +392,6 @@ describe('RelayfileGithubConnectionWrite', () => { }, }, }) - mount.emit(githubEvent('canonical-other-pr', canonicalPath)) const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) await expect(write.requestPullRequestReview({ diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index 9992305..f74420e 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -14,7 +14,7 @@ import { } from '../github/review-request' const REVIEW_REQUEST_CONFIRM_TIMEOUT_MS = 10_000 -const REVIEW_REQUEST_RECENT_EVENT_LIMIT = 200 +const REVIEW_REQUEST_QUERY_PAGE_LIMIT = 100 const execFileAsync = promisify(execFile) const WRITE_CONFIRM_TIMEOUT_MS = 90_000 @@ -24,7 +24,14 @@ const RECEIPT_READ_DELAY_MS = 100 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string; stderr?: string }> export interface RelayfileGithubConnectionWriteConfig { - mount: Pick + mount: Pick & { + queryFiles(opts: { + path: string + provider?: string + cursor?: string + limit?: number + }): Promise<{ paths: string[]; nextCursor: string | null }> + } gitRunner?: GitCommandRunner receiptReadAttempts?: number receiptReadDelayMs?: number @@ -216,23 +223,37 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { `^${escapeRegExp(canonicalCommentsRoot)}/[^/]+/(?:meta|metadata)\\.json$`, 'u', ) - const canonicalPaths = new Set() - const recentEvents = await this.#mount.getEvents({ - provider: 'github', - last: REVIEW_REQUEST_RECENT_EVENT_LIMIT, - }) - for (const event of recentEvents.events) { - const path = event.resource.path - if (canonicalDirectPattern.test(path) || canonicalNestedPattern.test(path)) { - canonicalPaths.add(path) + let cursor: string | undefined + const visitedCursors = new Set() + do { + const page = await this.#mount.queryFiles({ + path: canonicalCommentsRoot, + provider: 'github', + cursor, + limit: REVIEW_REQUEST_QUERY_PAGE_LIMIT, + }) + for (const path of page.paths) { + if (!canonicalDirectPattern.test(path) && !canonicalNestedPattern.test(path)) continue + let content: Record + try { + content = record((await this.#mount.readFile(path)).content) + } catch (error) { + // A current-tree query can race provider reconciliation. A path that + // was deleted after the query is stale evidence, so continue; other + // read failures remain indeterminate and must be retried by the caller. + if (isMountPathNotFound(error)) continue + throw error + } + if (!canonicalGithubCommentMatches(content, expectedRepo, number)) continue + const body = githubCommentBody(content) + if (body && containsCoderabbitReviewRequest(body)) return true } - } - for (const path of canonicalPaths) { - const content = record((await this.#mount.readFile(path)).content) - if (!canonicalGithubCommentMatches(content, expectedRepo, number)) continue - const body = githubCommentBody(content) - if (body && containsCoderabbitReviewRequest(body)) return true - } + cursor = page.nextCursor ?? undefined + if (cursor && visitedCursors.has(cursor)) { + throw new Error(`Relayfile file query repeated cursor while scanning ${canonicalCommentsRoot}: ${cursor}`) + } + if (cursor) visitedCursors.add(cursor) + } while (cursor) return false } diff --git a/src/testing/fakes.ts b/src/testing/fakes.ts index e4e89c0..8339f23 100644 --- a/src/testing/fakes.ts +++ b/src/testing/fakes.ts @@ -83,6 +83,24 @@ export class FakeMountClient implements MountClient { return [...this.files.keys()].filter((path) => path.startsWith(prefix)).sort() } + async queryFiles(opts: { + path: string + provider?: string + cursor?: string + limit?: number + }): Promise<{ paths: string[]; nextCursor: string | null }> { + const paths = [...this.files.keys()].filter((path) => path.startsWith(opts.path)).sort() + const start = opts.cursor + ? Math.max(0, paths.findIndex((path) => path === opts.cursor) + 1) + : 0 + const limit = opts.limit ?? paths.length + const page = paths.slice(start, start + limit) + return { + paths: page, + nextCursor: page.length >= limit ? page.at(-1) ?? null : null, + } + } + isLocalMountAuthDegraded(): boolean { return this.authDegraded } From f2c05179162c959bcc482fff74f73e9837d6cdf5 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:22:47 -0400 Subject: [PATCH 08/30] fix: recognize canonical review identities --- .../relayfile-github-connection-write.test.ts | 60 +++++++++++++++++-- .../relayfile-github-connection-write.ts | 40 ++++++++++++- 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index 2704917..7335db3 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -15,12 +15,6 @@ const gitRunnerForBranch = (branch: string): GitCommandRunner => vi.fn(async (ar throw new Error(`unexpected git args: ${args.join(' ')}`) }) -const githubEvent = (id: string, path: string): Parameters[0] => ({ - id, - type: 'relayfile.changed', - resource: { provider: 'github', path }, -} as Parameters[0]) - describe('RelayfileGithubConnectionWrite', () => { it('requests CodeRabbit review once through the connected app write path', async () => { const mount = new FakeMountClient() @@ -405,6 +399,60 @@ describe('RelayfileGithubConnectionWrite', () => { }]) }) + it('finds a URL-shaped canonical request after adapter reconciliation', async () => { + const canonicalPath = + '/github/repos/AgentWorkforce/factory/comments/9009.json' + const mount = new FakeMountClient({ + [canonicalPath]: { + payload: { + owner: 'AgentWorkforce', + repo: 'factory', + pull_request_url: 'https://api.github.com/repos/AgentWorkforce/factory/pulls/93', + comment: { + body: '@coderabbitai review\n', + }, + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 93, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.reads).toEqual([canonicalPath]) + }) + + it('fails closed when canonical identity fields conflict with the pull request URL', async () => { + const canonicalPath = + '/github/repos/AgentWorkforce/factory/comments/9010.json' + const mount = new FakeMountClient({ + [canonicalPath]: { + payload: { + owner: 'AgentWorkforce', + repo: 'factory', + pull_request_url: 'https://api.github.com/repos/AgentWorkforce/other/pulls/94', + comment: { + body: '@coderabbitai review\n', + }, + }, + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 94, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([{ + path: '/github/repos/AgentWorkforce/factory/pulls/94/comments/factory-coderabbit-review.json', + content: { body: '@coderabbitai review\n' }, + }]) + }) + it('publishes an already-pushed remote branch without reading an orchestrator-local clone', async () => { const pullRequestPath = '/github/repos/AgentWorkforce/factory/pull-requests/factory-factory-ar-85-agentworkforce-factory-pushed.json' class ReceiptMount extends FakeMountClient { diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index f74420e..300c972 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -397,6 +397,7 @@ const canonicalGithubCommentMatches = ( expectedNumber: number, ): boolean => { const payload = record(content.payload) + const comment = record(payload.comment) const repository = record( Object.keys(record(payload.repository)).length > 0 ? payload.repository @@ -407,9 +408,44 @@ const canonicalGithubCommentMatches = ( ? payload.pull_request : content.pull_request, ) + const identities = new Set() + const numbers = new Set() const fullName = stringValue(repository.full_name) - const number = positiveInteger(pullRequest.number) - return fullName?.toLowerCase() === expectedRepo.toLowerCase() && number === expectedNumber + if (fullName) identities.add(fullName.toLowerCase()) + const owner = stringValue(payload.owner) ?? stringValue(content.owner) + const repo = stringValue(payload.repo) ?? stringValue(content.repo) + if (owner || repo) { + if (!owner || !repo) return false + identities.add(`${owner}/${repo}`.toLowerCase()) + } + const directNumber = positiveInteger(pullRequest.number) + if (directNumber) numbers.add(directNumber) + for (const value of [ + content.pull_request_url, + payload.pull_request_url, + comment.pull_request_url, + comment.html_url, + ]) { + const parsed = githubPullRequestFromUrl(stringValue(value)) + if (!parsed) continue + identities.add(parsed.repo.toLowerCase()) + numbers.add(parsed.number) + } + return identities.size === 1 && + identities.has(expectedRepo.toLowerCase()) && + numbers.size === 1 && + numbers.has(expectedNumber) +} + +const githubPullRequestFromUrl = (value: string | undefined): { repo: string; number: number } | undefined => { + if (!value) return undefined + const match = value.match( + /^https:\/\/(?:api\.)?github\.com\/(?:repos\/)?([^/]+)\/([^/]+)\/pulls?\/(\d+)(?:[#/?].*)?$/iu, + ) + const number = positiveInteger(match?.[3]) + return match?.[1] && match[2] && number + ? { repo: `${match[1]}/${match[2]}`, number } + : undefined } const positiveInteger = (value: unknown): number | undefined => { From 040d8544a1711bd48d99c9f66d0f3c1e7b9088bc Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:26:12 -0400 Subject: [PATCH 09/30] fix: bound recovered operation polling --- .../relayfile-cloud-mount-client.test.ts | 30 +++++++++++++++++++ src/mount/relayfile-cloud-mount-client.ts | 7 ++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index 293a2ec..770b855 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -1406,6 +1406,36 @@ describe('RelayfileCloudMountClient', () => { expect(fake.getOpCalls).toEqual([]) }) + it('bounds recovered operation polling by the confirmation timeout', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + class HangingRecoveredOperationClient extends FakeRelayFileClient { + override async getOp(): Promise { + await new Promise(() => undefined) + } + } + const fake = new HangingRecoveredOperationClient() + fake.ops.set('op-recovered', { + opId: 'op-recovered', + path, + action: 'file_upsert', + provider: 'github', + status: 'pending', + attemptCount: 1, + createdAt: '2026-07-30T01:00:00.000Z', + }) + const restartedMount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await expect(restartedMount.confirmWrite(path, { + timeoutMs: 5, + returnFailed: true, + })).resolves.toBe('timeout') + expect(fake.listOpsCalls).toHaveLength(1) + }) + it('treats restarted operation lookup failures as unavailable recovery data', async () => { class FailingListOpsClient extends FakeRelayFileClient { override async listOps(): Promise { diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index 52ebd1a..db2871d 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -804,7 +804,12 @@ export class RelayfileCloudMountClient implements MountClient { if (!opId || !this.#client.getOp) return 'timeout' for (;;) { - const operation = await this.#client.getOp(this.workspaceId, opId) + if (Date.now() >= deadline) return 'timeout' + const operation = await settleBeforeDeadline( + this.#client.getOp(this.workspaceId, opId), + deadline, + ) + if (!operation) return 'timeout' let status: 'acked' | 'pending' | 'failed' try { status = mapOperationStatus(operation) From eeee1aec045a7d6fcd1e50fee02494468f6d93ea Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:29:52 -0400 Subject: [PATCH 10/30] fix: recognize root-level review comments --- .../relayfile-github-connection-write.test.ts | 20 +++++++++++++++++++ .../relayfile-github-connection-write.ts | 2 ++ 2 files changed, 22 insertions(+) diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index 7335db3..d7c4b80 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -425,6 +425,26 @@ describe('RelayfileGithubConnectionWrite', () => { expect(mount.reads).toEqual([canonicalPath]) }) + it('finds a root-level GitHub issue comment by its pull request URL after restart', async () => { + const canonicalPath = + '/github/repos/AgentWorkforce/factory/comments/9011.json' + const mount = new FakeMountClient({ + [canonicalPath]: { + html_url: 'https://github.com/AgentWorkforce/factory/pull/95#issuecomment-9011', + body: '@coderabbitai review\n', + }, + }) + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 95, + })).resolves.toBeUndefined() + + expect(mount.writes).toEqual([]) + expect(mount.reads).toEqual([canonicalPath]) + }) + it('fails closed when canonical identity fields conflict with the pull request URL', async () => { const canonicalPath = '/github/repos/AgentWorkforce/factory/comments/9010.json' diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index 300c972..8c12d98 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -422,7 +422,9 @@ const canonicalGithubCommentMatches = ( if (directNumber) numbers.add(directNumber) for (const value of [ content.pull_request_url, + content.html_url, payload.pull_request_url, + payload.html_url, comment.pull_request_url, comment.html_url, ]) { From 5dae1d4d21dfdaa08f117c42b818703af469f936 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:37:43 -0400 Subject: [PATCH 11/30] fix: retry automated review requests --- src/orchestrator/factory.test.ts | 45 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 24 ++++++++++++++++- src/types.ts | 2 ++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 05891bd..b39b117 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10063,6 +10063,51 @@ describe('FactoryLoop', () => { } }) + it('retries a rejected automated review request after issue completion', async () => { + const number = 527 + const reviewRequests: Array<{ repo: string; number: number }> = [] + let attempts = 0 + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + attempts += 1 + if (attempts === 1) throw new Error('transient provider failure') + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + reviewRequestRetryMs: 5, + }) + + try { + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + + await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + await vi.waitFor(() => expect(reviewRequests).toEqual([ + { repo: 'AgentWorkforce/pear', number }, + { repo: 'AgentWorkforce/pear', number }, + ])) + } finally { + await factory.stop() + } + }) + it('keeps provider delete authorization aligned with guarded draft authorization', async () => { class DeletePredicateMount extends FakeMountClient { deletePredicate?: (path: string, content: unknown) => boolean | Promise diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 77d4753..92eba19 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -266,6 +266,7 @@ const PROBE_PR_GH_BACKOFF_MS = 60_000 const PROBE_PR_GH_CANDIDATE_LIMIT = 200 const PUBLISHED_PR_CONFIRM_ATTEMPTS = 20 const PUBLISHED_PR_CONFIRM_DELAY_MS = 100 +const AUTOMATED_REVIEW_REQUEST_RETRY_MS = 5_000 const SLACK_REPLY_EVENTS_LIMIT = 100 const SLACK_REPLY_POLL_INTERVAL_MS = 5_000 const SLACK_IDENTITY_MESSAGE_SCAN_LIMIT = 250 @@ -390,6 +391,7 @@ export class FactoryLoop implements Factory { readonly #babysitterWakeUnreachableEscalateMs: number readonly #babysitterWakeUnreachableRetryMs: number readonly #startupAgentExitDrainTimeoutMs: number + readonly #reviewRequestRetryMs: number readonly #state: StateStore readonly #workspaceId: string readonly #relayflows?: FactoryPorts['relayflows'] @@ -522,6 +524,7 @@ export class FactoryLoop implements Factory { readonly #publishedPullRequests = new Map() readonly #reviewRequestedPullRequests = new Set() readonly #reviewRequestVerifications = new Set() + readonly #reviewRequestRetryTimers = new Map>() readonly #previewReferences = new Map() readonly #removedPreviewIds = new Set() readonly #probePrGhBackoffUntilMs = new Map() @@ -595,6 +598,7 @@ export class FactoryLoop implements Factory { this.#babysitterWakeUnreachableEscalateMs = ports.babysitterWakeUnreachableEscalateMs ?? BABYSITTER_WAKE_UNREACHABLE_ESCALATE_MS this.#babysitterWakeUnreachableRetryMs = ports.babysitterWakeUnreachableRetryMs ?? BABYSITTER_WAKE_UNREACHABLE_RETRY_MS this.#startupAgentExitDrainTimeoutMs = ports.startupAgentExitDrainTimeoutMs ?? STARTUP_AGENT_EXIT_DRAIN_TIMEOUT_MS + this.#reviewRequestRetryMs = ports.reviewRequestRetryMs ?? AUTOMATED_REVIEW_REQUEST_RETRY_MS this.#workspaceId = config.workspaceId ?? 'default' this.#relayflows = ports.relayflows this.#worktrees = ports.worktrees @@ -891,6 +895,8 @@ export class FactoryLoop implements Factory { this.#dispatchLifecycleRenewTimer = undefined for (const timer of this.#dispatchLifecycleRetryTimers.values()) clearTimeout(timer) this.#dispatchLifecycleRetryTimers.clear() + for (const timer of this.#reviewRequestRetryTimers.values()) clearTimeout(timer) + this.#reviewRequestRetryTimers.clear() this.#abandonedDispatchReasons.clear() this.#dispatchLifecycleOwnershipWaitLogged.clear() if (this.#completionSweepTimer) clearTimeout(this.#completionSweepTimer) @@ -6196,7 +6202,7 @@ export class FactoryLoop implements Factory { published: GithubPullRequestRef, ): void { const key = `${published.repo.toLowerCase()}#${published.number}` - if (this.#reviewRequestedPullRequests.has(key)) return + if (this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestRetryTimers.has(key)) return try { const requestReview = this.#githubPullRequestReviewRequester() if (!requestReview) return @@ -6211,6 +6217,7 @@ export class FactoryLoop implements Factory { prNumber: published.number, error: describeError(error).errorMessage, }) + this.#scheduleAutomatedPullRequestReviewRetry(published) }) } catch (error) { this.#increment('githubPullRequestReviewRequestFailures') @@ -6222,6 +6229,21 @@ export class FactoryLoop implements Factory { } } + #scheduleAutomatedPullRequestReviewRetry(published: GithubPullRequestRef): void { + const key = `${published.repo.toLowerCase()}#${published.number}` + if ( + this.#stopping || + this.#reviewRequestedPullRequests.has(key) || + this.#reviewRequestRetryTimers.has(key) + ) return + const timer = setTimeout(() => { + this.#reviewRequestRetryTimers.delete(key) + if (!this.#stopping) this.#requestAutomatedPullRequestReview(published) + }, this.#reviewRequestRetryMs) + timer.unref?.() + this.#reviewRequestRetryTimers.set(key, timer) + } + #requestAutomatedPullRequestReviewForOpenReceipt( published: GithubPublishPullRequestResult, ): void { diff --git a/src/types.ts b/src/types.ts index 1c3c05e..c866c23 100644 --- a/src/types.ts +++ b/src/types.ts @@ -55,6 +55,8 @@ export interface FactoryPorts { * active in the background. Test-only override of the built-in default. */ startupAgentExitDrainTimeoutMs?: number + /** Retry cadence for failed automated PR review requests. Test-only override. */ + reviewRequestRetryMs?: number relayflows?: FactoryRelayflowDispatchPort /** Local CLI checkout isolation. Remote fleet nodes own their own checkout lifecycle. */ worktrees?: AgentWorktreeManager From 790fb9c11700614c53c77076b01942ce50850b17 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 05:45:43 -0400 Subject: [PATCH 12/30] fix: verify review retries stay open --- src/orchestrator/factory.test.ts | 64 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 60 +++++++++++++++++++++++++++--- 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index b39b117..9b357fb 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10067,6 +10067,7 @@ describe('FactoryLoop', () => { const number = 527 const reviewRequests: Array<{ repo: string; number: number }> = [] let attempts = 0 + let openLookups = 0 const githubWrite: GithubConnectionWrite = { publishPullRequest: async (input) => ({ repo: input.repo, @@ -10091,6 +10092,18 @@ describe('FactoryLoop', () => { fleet, triage: new StaticTriage(), probePrResolver: async () => undefined, + probePrGhRunner: async () => { + openLookups += 1 + if (openLookups === 1) throw new Error('transient open-state lookup failure') + return { + stdout: JSON.stringify({ + number, + headRefName: `factory/ar-${number}-pear`, + isDraft: false, + state: 'OPEN', + }), + } + }, reviewRequestRetryMs: 5, }) @@ -10103,6 +10116,57 @@ describe('FactoryLoop', () => { { repo: 'AgentWorkforce/pear', number }, { repo: 'AgentWorkforce/pear', number }, ])) + expect(openLookups).toBe(2) + } finally { + await factory.stop() + } + }) + + it('does not retry an automated review request after the pull request closes', async () => { + const number = 528 + const reviewRequests: Array<{ repo: string; number: number }> = [] + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + throw new Error('provider failed before the pull request closed') + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: `factory/ar-${number}-pear`, + isDraft: false, + state: 'CLOSED', + }), + }), + reviewRequestRetryMs: 5, + }) + + try { + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + + await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + await vi.waitFor(() => expect(reviewRequests).toHaveLength(1)) + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number }]) } finally { await factory.stop() } diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 92eba19..244c541 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -135,6 +135,7 @@ type EventHighWatermarkResult = { highWatermark?: string; routeUnavailable: bool type PreparedLiveEvent = { path?: string; dispatchRelayflow: boolean } type GithubPullRequestPublisher = Pick type GithubPullRequestIdentity = 'app' | 'user' +type AutomatedReviewRequestTarget = GithubPullRequestRef & { headRef?: string } type GithubOrphanRecoveryContext = { activeIssueIdentities: Set onlineAgentNames: Set @@ -1575,6 +1576,7 @@ export class FactoryLoop implements Factory { this.#requestAutomatedPullRequestReview({ repo: pr.repo, number: pr.prNumber, + headRef: pr.headRef, }) } if (record.decision.implementers.length > 1 && !await this.#allImplementersHaveCompletionPr(record)) { @@ -6199,7 +6201,7 @@ export class FactoryLoop implements Factory { } #requestAutomatedPullRequestReview( - published: GithubPullRequestRef, + published: AutomatedReviewRequestTarget, ): void { const key = `${published.repo.toLowerCase()}#${published.number}` if (this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestRetryTimers.has(key)) return @@ -6229,7 +6231,7 @@ export class FactoryLoop implements Factory { } } - #scheduleAutomatedPullRequestReviewRetry(published: GithubPullRequestRef): void { + #scheduleAutomatedPullRequestReviewRetry(published: AutomatedReviewRequestTarget): void { const key = `${published.repo.toLowerCase()}#${published.number}` if ( this.#stopping || @@ -6238,19 +6240,26 @@ export class FactoryLoop implements Factory { ) return const timer = setTimeout(() => { this.#reviewRequestRetryTimers.delete(key) - if (!this.#stopping) this.#requestAutomatedPullRequestReview(published) + if (!this.#stopping) this.#requestAutomatedPullRequestReviewForOpenReceipt(published) }, this.#reviewRequestRetryMs) timer.unref?.() this.#reviewRequestRetryTimers.set(key, timer) } #requestAutomatedPullRequestReviewForOpenReceipt( - published: GithubPublishPullRequestResult, + published: AutomatedReviewRequestTarget, ): void { const key = `${published.repo.toLowerCase()}#${published.number}` - if (this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestVerifications.has(key)) return + if ( + this.#reviewRequestedPullRequests.has(key) || + this.#reviewRequestVerifications.has(key) || + this.#reviewRequestRetryTimers.has(key) + ) return this.#reviewRequestVerifications.add(key) - void this.#openPullRequestByHead(published.repo, published.headRef) + const openLookup = published.headRef + ? this.#openPullRequestByHead(published.repo, published.headRef) + : this.#openPullRequestByNumber(published.repo, published.number) + void openLookup .then((open) => { if (open?.number === published.number) { this.#requestAutomatedPullRequestReview(open) @@ -6263,6 +6272,7 @@ export class FactoryLoop implements Factory { prNumber: published.number, error: describeError(error).errorMessage, }) + this.#scheduleAutomatedPullRequestReviewRetry(published) }) .finally(() => this.#reviewRequestVerifications.delete(key)) } @@ -6406,6 +6416,43 @@ export class FactoryLoop implements Factory { return candidates.sort((a, b) => b.number - a.number)[0] } + async #openPullRequestByNumber( + repo: string, + number: number, + ): Promise { + if (this.#hasProbePrGhRunner) { + const result = await this.#probePrGhRunner([ + 'pr', + 'view', + String(number), + '--repo', + repo, + '--json', + 'number,headRefName,isDraft,state', + ]) + const candidate = asRecord(parseJsonContent(result.stdout)) + const candidateNumber = numberValue(candidate?.number) + if ( + candidateNumber !== number || + Boolean(candidate?.isDraft) || + normalizePrState(stringValue(candidate?.state)) !== 'OPEN' + ) return undefined + const headRef = stringValue(candidate?.headRefName) + return { + repo, + number, + ...(headRef ? { headRef } : {}), + } + } + const snapshot = await this.#github.getPr(repo, number) + if (snapshot.number !== number || normalizePrState(snapshot.state) !== 'OPEN') return undefined + return { + repo, + number, + ...(snapshot.headRef ? { headRef: snapshot.headRef } : {}), + } + } + async #prepareAgentWorktree(record: InFlightIssue, spec: AgentSpec): Promise { const worktree = this.#agentWorktree(record, spec) if (!worktree || !this.#worktrees) return @@ -6940,6 +6987,7 @@ export class FactoryLoop implements Factory { this.#requestAutomatedPullRequestReview({ repo: pr.repo, number: pr.prNumber, + headRef: pr.headRef, }) } return true From c16c741b8d873d25b0120c8c56b60238f2896c5b Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 06:04:40 -0400 Subject: [PATCH 13/30] fix: arbitrate review request creation --- .../relayfile-cloud-mount-client.test.ts | 134 ++++++++++++++++++ src/mount/relayfile-cloud-mount-client.ts | 40 ++++++ .../relayfile-github-connection-write.test.ts | 75 ++++++++++ .../relayfile-github-connection-write.ts | 25 +++- src/ports/mount.ts | 10 ++ src/testing/fakes.ts | 11 ++ src/writeback/github.ts | 3 + src/writeback/writeback.test.ts | 2 +- 8 files changed, 296 insertions(+), 4 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index 770b855..7deacb3 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -1276,6 +1276,140 @@ describe('RelayfileCloudMountClient', () => { expect(fake.getOpCalls).toEqual([{ workspaceId: 'rw_test', opId: 'op-1' }]) }) + it('preserves create-only revision conflicts instead of updating the winner', async () => { + class RevisionCheckingClient extends FakeRelayFileClient { + override async writeFile(input: Parameters[0]) { + this.writeFileCalls.push(input) + const current = this.files.get(input.path) + if (current && input.baseRevision === '0') { + throw Object.assign(new Error('revision conflict'), { + status: 409, + expectedRevision: '0', + currentRevision: current.revision, + }) + } + this.files.set(input.path, { + revision: String(Number(input.baseRevision) + 1), + content: input.content, + contentType: input.contentType ?? 'application/json', + }) + return { + opId: `op-${this.writeFileCalls.length}`, + status: 'queued' as const, + targetRevision: 'next', + } + } + } + const fake = new RevisionCheckingClient() + fake.files.set('/github/review-request.json', { + revision: '7', + content: '{"body":"winner"}', + contentType: 'application/json', + }) + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await expect(mount.createFile( + '/github/review-request.json', + { body: 'loser' }, + { guarded: true }, + )).resolves.toBe('exists') + + expect(fake.writeFileCalls).toEqual([expect.objectContaining({ + path: '/github/review-request.json', + baseRevision: '0', + content: '{"body":"loser"}', + })]) + expect(fake.files.get('/github/review-request.json')?.content).toBe('{"body":"winner"}') + }) + + it('recovers the concurrent winner instead of confirming a cached prior attempt', async () => { + class RevisionCheckingClient extends FakeRelayFileClient { + override async writeFile(input: Parameters[0]) { + const current = this.files.get(input.path) + if (current && input.baseRevision === '0') { + this.writeFileCalls.push(input) + throw Object.assign(new Error('revision conflict'), { + status: 409, + expectedRevision: '0', + currentRevision: current.revision, + }) + } + return await super.writeFile(input) + } + } + const path = '/github/review-request.json' + const fake = new RevisionCheckingClient() + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await mount.writeFile(path, { body: 'prior attempt' }) + fake.ops.set('op-1', { + opId: 'op-1', + path, + action: 'file_upsert', + provider: 'github', + status: 'succeeded', + attemptCount: 1, + createdAt: '2026-07-30T00:00:00.000Z', + providerResult: { status: 201, externalId: 'prior' }, + }) + fake.files.set(path, { + revision: '2', + content: '{"body":"concurrent winner"}', + contentType: 'application/json', + }) + fake.ops.set('op-2', { + opId: 'op-2', + path, + action: 'file_upsert', + provider: 'github', + status: 'succeeded', + attemptCount: 1, + createdAt: '2026-07-30T00:00:01.000Z', + providerResult: { status: 201, externalId: 'winner' }, + }) + + await expect(mount.createFile( + path, + { body: 'losing attempt' }, + { guarded: true }, + )).resolves.toBe('exists') + await expect(mount.confirmWrite(path, { timeoutMs: 50 })).resolves.toBe('acked') + + expect(fake.listOpsCalls).toHaveLength(1) + expect(fake.getOpCalls.at(-1)).toEqual({ workspaceId: 'rw_test', opId: 'op-2' }) + await expect(mount.getConfirmedWriteExternalId(path)).resolves.toBe('winner') + }) + + it('does not collapse an unrelated 409 into create-only success', async () => { + class InvalidStateClient extends FakeRelayFileClient { + override async writeFile(_input: Parameters[0]): Promise { + throw Object.assign(new Error('provider state conflict'), { + status: 409, + code: 'invalid_state', + }) + } + } + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: new InvalidStateClient(), + isAllowedDraft: () => true, + }) + + await expect(mount.createFile( + '/github/review-request.json', + { body: 'request' }, + { guarded: true }, + )).rejects.toThrow('provider state conflict') + }) + it('confirms a succeeded draft op with providerResult 201 and externalId', async () => { const fake = new FakeRelayFileClient() fake.ops.set('op-1', { diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index db2871d..18b81c0 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -545,6 +545,38 @@ export class RelayfileCloudMountClient implements MountClient { } } + async createFile( + path: string, + content: unknown, + opts?: { guarded?: boolean }, + ): Promise<'created' | 'exists'> { + if (isProviderWritebackPath(path) && await this.#isAllowedDraft?.(path, content, opts) !== true) { + throw new Error(`Refusing provider writeback draft for ${path}: draft predicate rejected or is unset`) + } + + const serialized = serializeContent(content) + // A conflict means another writer owns the current operation. Do not let a + // prior attempt cached by this mount stand in for that winner during + // confirmation; the caller must recover the current op from Relayfile. + this.#lastOpByPath.delete(path) + this.#confirmedExternalIdByPath.delete(path) + this.#confirmedFailureReasonByPath.delete(path) + try { + const queued = await this.#client.writeFile({ + workspaceId: this.workspaceId, + path, + baseRevision: '0', + content: serialized.content, + contentType: serialized.contentType, + }) + this.#lastOpByPath.set(path, queued.opId) + return 'created' + } catch (error) { + if (!isCreateRevisionConflict(error)) throw error + return 'exists' + } + } + async deleteFile(path: string): Promise { this.#confirmedExternalIdByPath.delete(path) this.#confirmedFailureReasonByPath.delete(path) @@ -1125,6 +1157,14 @@ const isProviderWritebackPath = (path: string): boolean => const isProviderPath = (path: string): boolean => path.startsWith('/linear/') || path.startsWith('/github/') || path.startsWith('/slack/') +const isCreateRevisionConflict = (error: unknown): boolean => { + if (!isHttpStatus(error, 409) || error === null || typeof error !== 'object') return false + const conflict = error as { expectedRevision?: unknown; currentRevision?: unknown } + return conflict.expectedRevision === '0' && + typeof conflict.currentRevision === 'string' && + conflict.currentRevision !== '0' +} + const providerForPath = (path: string): string | undefined => /^\/([^/]+)\//u.exec(path)?.[1] diff --git a/src/mount/relayfile-github-connection-write.test.ts b/src/mount/relayfile-github-connection-write.test.ts index d7c4b80..d20b8e8 100644 --- a/src/mount/relayfile-github-connection-write.test.ts +++ b/src/mount/relayfile-github-connection-write.test.ts @@ -47,6 +47,81 @@ describe('RelayfileGithubConnectionWrite', () => { expect(mount.writes).toHaveLength(1) }) + it('uses atomic create to dedupe two independent connected-app writers', async () => { + class RacingCreateMount extends FakeMountClient { + createArrivals = 0 + confirmations = 0 + readonly #bothArrived: Promise + #releaseBoth!: () => void + + constructor() { + super() + this.#bothArrived = new Promise((resolve) => { + this.#releaseBoth = resolve + }) + } + + override async createFile( + path: string, + content: unknown, + opts?: { guarded?: boolean }, + ): Promise<'created' | 'exists'> { + this.createArrivals += 1 + if (this.createArrivals === 2) this.#releaseBoth() + await this.#bothArrived + return await super.createFile(path, content, opts) + } + + override async confirmWrite( + path: string, + opts?: { timeoutMs?: number; returnFailed?: boolean }, + ): Promise<'acked'> { + this.confirmations += 1 + return await super.confirmWrite(path, opts) as 'acked' + } + } + const mount = new RacingCreateMount() + const input = { repo: 'AgentWorkforce/factory', number: 85 } + const first = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + const second = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await Promise.all([ + first.requestPullRequestReview(input), + second.requestPullRequestReview(input), + ]) + + expect(mount.createArrivals).toBe(2) + expect(mount.writes).toEqual([{ + path: '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json', + content: { body: '@coderabbitai review\n' }, + }]) + // The loser only accepts the conflict after reading the exact winning + // draft and observing the same provider operation reach acknowledgement. + expect(mount.confirmations).toBe(2) + }) + + it('fails a create conflict when the winning draft is not the exact request', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + class AppearingConflictMount extends FakeMountClient { + override async createFile(): Promise<'exists'> { + this.files.set(path, { + content: { body: '@coderabbitai review' }, + revision: '1', + }) + return 'exists' + } + } + const mount = new AppearingConflictMount() + const write = new RelayfileGithubConnectionWrite({ mount, gitRunner: gitRunner() }) + + await expect(write.requestPullRequestReview({ + repo: 'AgentWorkforce/factory', + number: 85, + })).rejects.toThrow(`GitHub review request create conflicted with unexpected content at ${path}`) + + expect(mount.writes).toEqual([]) + }) + it('treats missing fresh-PR comment trees as empty and posts the first request', async () => { class MissingCommentTreesMount extends FakeMountClient { override async listTree(): Promise { diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index 8c12d98..44f85f4 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -24,7 +24,7 @@ const RECEIPT_READ_DELAY_MS = 100 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string; stderr?: string }> export interface RelayfileGithubConnectionWriteConfig { - mount: Pick & { + mount: Pick & { queryFiles(opts: { path: string provider?: string @@ -136,6 +136,9 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { `/github/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}`, `/github/repos/${encodeURIComponent(owner)}__${encodeURIComponent(repo)}`, ] + // The process-local promise handles re-entry here; the Relayfile + // create-if-absent below arbitrates independent Factory processes. Direct + // GitHub comments remain outside that store boundary and can still race. const requestKey = `${input.repo.toLowerCase()}#${input.number}` const existing = this.#reviewRequests.get(requestKey) if (existing) return existing @@ -153,9 +156,25 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { if (await this.#hasPullRequestReviewRequest(expectedRepo, number, repoRoot)) return } const commentsRoot = `${repoRoots[0]}/pulls/${number}/comments` - await this.#writeAndConfirm(`${commentsRoot}/factory-coderabbit-review.json`, { - body: FACTORY_CODERABBIT_REVIEW_BODY, + const path = `${commentsRoot}/factory-coderabbit-review.json` + const content = { body: FACTORY_CODERABBIT_REVIEW_BODY } + const result = await this.#mount.createFile(path, content, { guarded: true }) + if (result === 'exists') { + const existing = (await this.#mount.readFile(path)).content + if (!isAllowedFactoryGithubWritebackDraft(path, existing)) { + throw new Error(`GitHub review request create conflicted with unexpected content at ${path}`) + } + } + const status = await this.#mount.confirmWrite(path, { + timeoutMs: WRITE_CONFIRM_TIMEOUT_MS, + returnFailed: true, }) + if (status !== 'acked') { + const failureReason = status === 'failed' + ? await this.#mount.getConfirmedWriteFailureReason?.(path) + : undefined + throw new Error(`GitHub writeback did not complete for ${path}: ${failureReason ?? status}`) + } } async #hasPullRequestReviewRequest(expectedRepo: string, number: number, repoRoot: string): Promise { diff --git a/src/ports/mount.ts b/src/ports/mount.ts index 84d6cf6..688e4b1 100644 --- a/src/ports/mount.ts +++ b/src/ports/mount.ts @@ -126,6 +126,16 @@ export interface MountClient { dispose?(): Promise readFile(path: string): Promise<{ content: unknown; revision?: string }> writeFile(path: string, content: unknown, opts?: { guarded?: boolean }): Promise + /** + * Atomically create a path without replacing an existing record. + * `exists` means the store observed a revision conflict; callers must still + * verify that the winner wrote the record they intended. + */ + createFile( + path: string, + content: unknown, + opts?: { guarded?: boolean }, + ): Promise<'created' | 'exists'> deleteFile(path: string): Promise setDefaultAllowedDraftPredicate?( predicate: (path: string, content: unknown, opts?: { guarded?: boolean }) => boolean | Promise, diff --git a/src/testing/fakes.ts b/src/testing/fakes.ts index 8339f23..0a73bfe 100644 --- a/src/testing/fakes.ts +++ b/src/testing/fakes.ts @@ -71,6 +71,17 @@ export class FakeMountClient implements MountClient { this.writes.push({ path, content }) } + async createFile( + path: string, + content: unknown, + _opts?: { guarded?: boolean }, + ): Promise<'created' | 'exists'> { + if (this.files.has(path)) return 'exists' + this.files.set(path, { content, revision: '1' }) + this.writes.push({ path, content }) + return 'created' + } + async deleteFile(path: string): Promise { if (!this.files.has(path)) { throw new Error(`File not found: ${path}`) diff --git a/src/writeback/github.ts b/src/writeback/github.ts index 330b19e..e60e8b4 100644 --- a/src/writeback/github.ts +++ b/src/writeback/github.ts @@ -158,6 +158,9 @@ export class GhCliGithubWriteback implements GithubWriteback { } async requestPullRequestReview(input: { repo: string; number: number }): Promise { + // This map only coalesces callers in this process. A human can post the + // same command between the public scan and `gh pr comment`, so this path is + // intentionally at-least-once rather than an unqualified dedupe guarantee. const requestKey = `${input.repo.toLowerCase()}#${input.number}` const existing = this.#reviewRequests.get(requestKey) if (existing) return existing diff --git a/src/writeback/writeback.test.ts b/src/writeback/writeback.test.ts index d87fe03..687a165 100644 --- a/src/writeback/writeback.test.ts +++ b/src/writeback/writeback.test.ts @@ -887,7 +887,7 @@ describe('GhCliGithubWriteback', () => { ]) }) - it('requests CodeRabbit review once through the authenticated gh user', async () => { + it('coalesces CodeRabbit review requests within one authenticated gh process', async () => { const calls: string[][] = [] let comments = '[[]]' const github = new GhCliGithubWriteback({ From ba584f04a8f99f94f1ae7e6370b1e7055b3e0954 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 06:20:18 -0400 Subject: [PATCH 14/30] fix: close review request verification gaps --- .../relayfile-cloud-mount-client.test.ts | 24 ++++++++- src/mount/relayfile-cloud-mount-client.ts | 2 - .../relayfile-github-connection-write.ts | 3 +- src/orchestrator/factory.test.ts | 52 +++++++++++++++++++ src/orchestrator/factory.ts | 12 +++-- src/ports/mount.ts | 2 +- 6 files changed, 85 insertions(+), 10 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index 7deacb3..31ba88d 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -1570,7 +1570,7 @@ describe('RelayfileCloudMountClient', () => { expect(fake.listOpsCalls).toHaveLength(1) }) - it('treats restarted operation lookup failures as unavailable recovery data', async () => { + it('propagates restarted operation feed failures instead of reporting a timeout', async () => { class FailingListOpsClient extends FakeRelayFileClient { override async listOps(): Promise { throw new Error('relayfile unavailable') @@ -1585,10 +1585,30 @@ describe('RelayfileCloudMountClient', () => { await expect(restartedMount.confirmWrite('/github/repos/AgentWorkforce/factory/pulls/85/comments/review.json', { timeoutMs: 5, - })).resolves.toBe('timeout') + })).rejects.toThrow('relayfile unavailable') expect(fake.getOpCalls).toEqual([]) }) + it('propagates operation lookup failures instead of reporting a timeout', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/review.json' + class FailingGetOpClient extends FakeRelayFileClient { + override async getOp(): Promise { + throw new Error('operation lookup unavailable') + } + } + const fake = new FailingGetOpClient() + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await mount.writeFile(path, { body: '@coderabbitai review' }, { guarded: true }) + await expect(mount.confirmWrite(path, { + timeoutMs: 5, + })).rejects.toThrow('operation lookup unavailable') + }) + it('fails closed when restarted write operations have the same latest timestamp', async () => { const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' const fake = new FakeRelayFileClient() diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index 18b81c0..1edd0ea 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -1023,8 +1023,6 @@ const settleBeforeDeadline = async (operation: Promise, deadline: number): timer.unref?.() }), ]) - } catch { - return undefined } finally { if (timer) clearTimeout(timer) } diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index 44f85f4..6460cd4 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -24,7 +24,8 @@ const RECEIPT_READ_DELAY_MS = 100 export type GitCommandRunner = (args: string[]) => Promise<{ stdout: string; stderr?: string }> export interface RelayfileGithubConnectionWriteConfig { - mount: Pick & { + mount: Pick & { + createFile: NonNullable queryFiles(opts: { path: string provider?: string diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 9b357fb..52bfe63 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10122,6 +10122,58 @@ describe('FactoryLoop', () => { } }) + it('does not let stale mounted metadata authorize a review retry when GitHub lookup fails', async () => { + const number = 529 + const reviewRequests: Array<{ repo: string; number: number }> = [] + let openLookups = 0 + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + throw new Error('transient provider failure') + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + [`/github/repos/AgentWorkforce/pear/pulls/${number}/metadata.json`]: { + number, + state: 'open', + head_ref: `factory/ar-${number}-pear`, + isDraft: false, + }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + probePrGhRunner: async () => { + openLookups += 1 + throw new Error('authoritative GitHub lookup unavailable') + }, + reviewRequestRetryMs: 5, + }) + + try { + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + + await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + await vi.waitFor(() => expect(openLookups).toBeGreaterThan(1)) + expect(reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number }]) + } finally { + await factory.stop() + } + }) + it('does not retry an automated review request after the pull request closes', async () => { const number = 528 const reviewRequests: Array<{ repo: string; number: number }> = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 244c541..0e04e6d 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -6256,12 +6256,16 @@ export class FactoryLoop implements Factory { this.#reviewRequestRetryTimers.has(key) ) return this.#reviewRequestVerifications.add(key) - const openLookup = published.headRef - ? this.#openPullRequestByHead(published.repo, published.headRef) - : this.#openPullRequestByNumber(published.repo, published.number) + // Retry authorization must come from an authoritative point lookup. The + // head-based discovery helper may fall back to webhook-fed mount metadata, + // which is useful for reconciliation but cannot prove a PR remains open. + const openLookup = this.#openPullRequestByNumber(published.repo, published.number) void openLookup .then((open) => { - if (open?.number === published.number) { + if ( + open?.number === published.number && + (!published.headRef || open.headRef === published.headRef) + ) { this.#requestAutomatedPullRequestReview(open) } }) diff --git a/src/ports/mount.ts b/src/ports/mount.ts index 688e4b1..eec6506 100644 --- a/src/ports/mount.ts +++ b/src/ports/mount.ts @@ -131,7 +131,7 @@ export interface MountClient { * `exists` means the store observed a revision conflict; callers must still * verify that the winner wrote the record they intended. */ - createFile( + createFile?( path: string, content: unknown, opts?: { guarded?: boolean }, From 47fea626b5d3056b2094f3e44c5fae4bb3e8320a Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 06:41:52 -0400 Subject: [PATCH 15/30] fix: bound review request retry lifecycle --- src/orchestrator/factory.test.ts | 88 ++++++++++++++++--- src/orchestrator/factory.ts | 146 +++++++++++++++++++++++-------- src/types.ts | 2 + 3 files changed, 190 insertions(+), 46 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 52bfe63..21934f9 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10063,7 +10063,7 @@ describe('FactoryLoop', () => { } }) - it('retries a rejected automated review request after issue completion', async () => { + it('backs off rejected automated review requests after issue completion', async () => { const number = 527 const reviewRequests: Array<{ repo: string; number: number }> = [] let attempts = 0 @@ -10078,7 +10078,7 @@ describe('FactoryLoop', () => { requestPullRequestReview: async (input) => { reviewRequests.push(input) attempts += 1 - if (attempts === 1) throw new Error('transient provider failure') + if (attempts <= 2) throw new Error('transient provider failure') }, closePullRequest: async () => undefined, } @@ -10104,7 +10104,7 @@ describe('FactoryLoop', () => { }), } }, - reviewRequestRetryMs: 5, + reviewRequestRetryMs: 10, }) try { @@ -10116,16 +10116,84 @@ describe('FactoryLoop', () => { { repo: 'AgentWorkforce/pear', number }, { repo: 'AgentWorkforce/pear', number }, ])) - expect(openLookups).toBe(2) + await new Promise((resolve) => setTimeout(resolve, 5)) + expect(reviewRequests).toHaveLength(2) + await vi.waitFor(() => expect(reviewRequests).toEqual([ + { repo: 'AgentWorkforce/pear', number }, + { repo: 'AgentWorkforce/pear', number }, + { repo: 'AgentWorkforce/pear', number }, + ])) + expect(openLookups).toBe(3) } finally { await factory.stop() } }) - it('does not let stale mounted metadata authorize a review retry when GitHub lookup fails', async () => { + it('makes an unmet bounded run-once review drain a named terminal failure', async () => { + const number = 530 + const reviewRequests: Array<{ repo: string; number: number }> = [] + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + throw new Error('persistent provider failure') + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: `factory/ar-${number}-pear`, + isDraft: false, + state: 'OPEN', + }), + }), + // A normal timer cannot fire during this test; runOnce must explicitly + // drain the pending obligation. + reviewRequestRetryMs: 60_000, + reviewRequestRunOnceDrainMs: 1_000, + }) + + try { + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + await vi.waitFor(() => expect(reviewRequests).toHaveLength(1)) + + await expect(factory.runOnce({ dryRun: true })) + .rejects.toThrow('Run-once automated PR review request drain failed') + + expect(reviewRequests).toHaveLength(5) + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(reviewRequests).toHaveLength(5) + } finally { + await factory.stop() + } + }) + + it.each([ + ['is unavailable', undefined], + ['fails', async () => { throw new Error('authoritative GitHub lookup unavailable') }], + ] as const)('does not let stale mounted metadata authorize a review retry when GitHub lookup %s', async ( + _condition, + probePrGhRunner, + ) => { const number = 529 const reviewRequests: Array<{ repo: string; number: number }> = [] - let openLookups = 0 const githubWrite: GithubConnectionWrite = { publishPullRequest: async (input) => ({ repo: input.repo, @@ -10155,10 +10223,7 @@ describe('FactoryLoop', () => { fleet, triage: new StaticTriage(), probePrResolver: async () => undefined, - probePrGhRunner: async () => { - openLookups += 1 - throw new Error('authoritative GitHub lookup unavailable') - }, + probePrGhRunner, reviewRequestRetryMs: 5, }) @@ -10167,7 +10232,8 @@ describe('FactoryLoop', () => { fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) - await vi.waitFor(() => expect(openLookups).toBeGreaterThan(1)) + await vi.waitFor(() => + expect(factory.status().counters.githubPullRequestReviewRequestFailures).toBeGreaterThan(1)) expect(reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number }]) } finally { await factory.stop() diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 0e04e6d..e2d128a 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -234,6 +234,7 @@ class ClarificationWakeLeaseLostError extends Error {} class ClarificationQuestionDeliveryLeaseLostError extends Error {} class GithubEscalationReconciliationUnavailableError extends Error {} class GithubEscalationPostAmbiguousError extends Error {} +class AutomatedReviewRequestDrainError extends Error {} type GithubEscalationReconciliation = 'found' | 'absent' | 'unavailable' class ClarificationWakeStoppedError extends Error {} @@ -268,6 +269,9 @@ const PROBE_PR_GH_CANDIDATE_LIMIT = 200 const PUBLISHED_PR_CONFIRM_ATTEMPTS = 20 const PUBLISHED_PR_CONFIRM_DELAY_MS = 100 const AUTOMATED_REVIEW_REQUEST_RETRY_MS = 5_000 +const AUTOMATED_REVIEW_REQUEST_MAX_DELAY_MS = 60_000 +const AUTOMATED_REVIEW_REQUEST_RUN_ONCE_ATTEMPTS = 5 +const AUTOMATED_REVIEW_REQUEST_RUN_ONCE_DRAIN_MS = 10_000 const SLACK_REPLY_EVENTS_LIMIT = 100 const SLACK_REPLY_POLL_INTERVAL_MS = 5_000 const SLACK_IDENTITY_MESSAGE_SCAN_LIMIT = 250 @@ -393,6 +397,7 @@ export class FactoryLoop implements Factory { readonly #babysitterWakeUnreachableRetryMs: number readonly #startupAgentExitDrainTimeoutMs: number readonly #reviewRequestRetryMs: number + readonly #reviewRequestRunOnceDrainMs: number readonly #state: StateStore readonly #workspaceId: string readonly #relayflows?: FactoryPorts['relayflows'] @@ -525,7 +530,12 @@ export class FactoryLoop implements Factory { readonly #publishedPullRequests = new Map() readonly #reviewRequestedPullRequests = new Set() readonly #reviewRequestVerifications = new Set() - readonly #reviewRequestRetryTimers = new Map>() + readonly #reviewRequestAttempts = new Map() + readonly #reviewRequestWork = new Set>() + readonly #reviewRequestRetryTimers = new Map + published: AutomatedReviewRequestTarget + }>() readonly #previewReferences = new Map() readonly #removedPreviewIds = new Set() readonly #probePrGhBackoffUntilMs = new Map() @@ -600,6 +610,8 @@ export class FactoryLoop implements Factory { this.#babysitterWakeUnreachableRetryMs = ports.babysitterWakeUnreachableRetryMs ?? BABYSITTER_WAKE_UNREACHABLE_RETRY_MS this.#startupAgentExitDrainTimeoutMs = ports.startupAgentExitDrainTimeoutMs ?? STARTUP_AGENT_EXIT_DRAIN_TIMEOUT_MS this.#reviewRequestRetryMs = ports.reviewRequestRetryMs ?? AUTOMATED_REVIEW_REQUEST_RETRY_MS + this.#reviewRequestRunOnceDrainMs = + ports.reviewRequestRunOnceDrainMs ?? AUTOMATED_REVIEW_REQUEST_RUN_ONCE_DRAIN_MS this.#workspaceId = config.workspaceId ?? 'default' this.#relayflows = ports.relayflows this.#worktrees = ports.worktrees @@ -896,7 +908,7 @@ export class FactoryLoop implements Factory { this.#dispatchLifecycleRenewTimer = undefined for (const timer of this.#dispatchLifecycleRetryTimers.values()) clearTimeout(timer) this.#dispatchLifecycleRetryTimers.clear() - for (const timer of this.#reviewRequestRetryTimers.values()) clearTimeout(timer) + for (const retry of this.#reviewRequestRetryTimers.values()) clearTimeout(retry.timer) this.#reviewRequestRetryTimers.clear() this.#abandonedDispatchReasons.clear() this.#dispatchLifecycleOwnershipWaitLogged.clear() @@ -1847,6 +1859,10 @@ export class FactoryLoop implements Factory { }) throw error } finally { + // A daemon leaves delay-capped retries on their backoff timers. A true + // one-shot invocation has no later loop iteration, so drain them before + // its caller is allowed to dispose the mount and exit. + if (report && !this.#started) await this.#drainAutomatedPullRequestReviewRetriesForRunOnce() if (report) { this.#logger.info?.('[factory] run-once completed', { dryRun, @@ -6209,8 +6225,11 @@ export class FactoryLoop implements Factory { const requestReview = this.#githubPullRequestReviewRequester() if (!requestReview) return this.#reviewRequestedPullRequests.add(key) - void Promise.resolve() + this.#trackAutomatedPullRequestReviewWork(Promise.resolve() .then(() => requestReview({ repo: published.repo, number: published.number })) + .then(() => { + this.#reviewRequestAttempts.delete(key) + }) .catch((error: unknown) => { this.#reviewRequestedPullRequests.delete(key) this.#increment('githubPullRequestReviewRequestFailures') @@ -6220,7 +6239,7 @@ export class FactoryLoop implements Factory { error: describeError(error).errorMessage, }) this.#scheduleAutomatedPullRequestReviewRetry(published) - }) + })) } catch (error) { this.#increment('githubPullRequestReviewRequestFailures') this.#logger.warn?.('[factory] automated PR review request failed; lifecycle completion remains independent', { @@ -6228,6 +6247,7 @@ export class FactoryLoop implements Factory { prNumber: published.number, error: describeError(error).errorMessage, }) + this.#scheduleAutomatedPullRequestReviewRetry(published) } } @@ -6238,12 +6258,18 @@ export class FactoryLoop implements Factory { this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestRetryTimers.has(key) ) return + const attempt = (this.#reviewRequestAttempts.get(key) ?? 0) + 1 + this.#reviewRequestAttempts.set(key, attempt) + const delayMs = Math.min( + this.#reviewRequestRetryMs * (2 ** (attempt - 1)), + AUTOMATED_REVIEW_REQUEST_MAX_DELAY_MS, + ) const timer = setTimeout(() => { this.#reviewRequestRetryTimers.delete(key) if (!this.#stopping) this.#requestAutomatedPullRequestReviewForOpenReceipt(published) - }, this.#reviewRequestRetryMs) + }, delayMs) timer.unref?.() - this.#reviewRequestRetryTimers.set(key, timer) + this.#reviewRequestRetryTimers.set(key, { timer, published }) } #requestAutomatedPullRequestReviewForOpenReceipt( @@ -6260,13 +6286,15 @@ export class FactoryLoop implements Factory { // head-based discovery helper may fall back to webhook-fed mount metadata, // which is useful for reconciliation but cannot prove a PR remains open. const openLookup = this.#openPullRequestByNumber(published.repo, published.number) - void openLookup + this.#trackAutomatedPullRequestReviewWork(openLookup .then((open) => { if ( open?.number === published.number && (!published.headRef || open.headRef === published.headRef) ) { this.#requestAutomatedPullRequestReview(open) + } else { + this.#reviewRequestAttempts.delete(key) } }) .catch((error: unknown) => { @@ -6278,7 +6306,61 @@ export class FactoryLoop implements Factory { }) this.#scheduleAutomatedPullRequestReviewRetry(published) }) - .finally(() => this.#reviewRequestVerifications.delete(key)) + .finally(() => this.#reviewRequestVerifications.delete(key))) + } + + #trackAutomatedPullRequestReviewWork(work: Promise): void { + this.#reviewRequestWork.add(work) + void work.finally(() => this.#reviewRequestWork.delete(work)) + } + + async #drainAutomatedPullRequestReviewRetriesForRunOnce(): Promise { + const deadline = Date.now() + this.#reviewRequestRunOnceDrainMs + const attempts = new Map() + for (;;) { + const work = [...this.#reviewRequestWork] + if (work.length > 0) { + const remainingMs = deadline - Date.now() + if (remainingMs <= 0) break + let deadlineTimer: ReturnType | undefined + try { + await Promise.race([ + Promise.allSettled(work), + new Promise((resolve) => { + deadlineTimer = setTimeout(resolve, remainingMs) + }), + ]) + } finally { + if (deadlineTimer) clearTimeout(deadlineTimer) + } + } + if (Date.now() >= deadline) break + const retries = [...this.#reviewRequestRetryTimers.entries()] + if (retries.length === 0) { + if (this.#reviewRequestWork.size === 0) return + continue + } + for (const [key, retry] of retries) { + const attempt = (attempts.get(key) ?? 0) + 1 + attempts.set(key, attempt) + if (attempt >= AUTOMATED_REVIEW_REQUEST_RUN_ONCE_ATTEMPTS) break + clearTimeout(retry.timer) + this.#reviewRequestRetryTimers.delete(key) + this.#requestAutomatedPullRequestReviewForOpenReceipt(retry.published) + } + if ([...attempts.values()].some((attempt) => + attempt >= AUTOMATED_REVIEW_REQUEST_RUN_ONCE_ATTEMPTS)) break + } + for (const retry of this.#reviewRequestRetryTimers.values()) clearTimeout(retry.timer) + this.#reviewRequestRetryTimers.clear() + const details = { + pendingWork: this.#reviewRequestWork.size, + attempts: Object.fromEntries(attempts), + } + this.#logger.error?.('[factory] run-once automated PR review request drain failed', details) + throw new AutomatedReviewRequestDrainError( + 'Run-once automated PR review request drain failed; review remains unrequested', + ) } #githubPullRequestReviewRequester(): ((input: GithubPullRequestRef) => Promise) | undefined { @@ -6424,36 +6506,30 @@ export class FactoryLoop implements Factory { repo: string, number: number, ): Promise { - if (this.#hasProbePrGhRunner) { - const result = await this.#probePrGhRunner([ - 'pr', - 'view', - String(number), - '--repo', - repo, - '--json', - 'number,headRefName,isDraft,state', - ]) - const candidate = asRecord(parseJsonContent(result.stdout)) - const candidateNumber = numberValue(candidate?.number) - if ( - candidateNumber !== number || - Boolean(candidate?.isDraft) || - normalizePrState(stringValue(candidate?.state)) !== 'OPEN' - ) return undefined - const headRef = stringValue(candidate?.headRefName) - return { - repo, - number, - ...(headRef ? { headRef } : {}), - } - } - const snapshot = await this.#github.getPr(repo, number) - if (snapshot.number !== number || normalizePrState(snapshot.state) !== 'OPEN') return undefined + if (!this.#hasProbePrGhRunner) { + throw new Error('Authoritative GitHub pull request lookup is unavailable') + } + const result = await this.#probePrGhRunner([ + 'pr', + 'view', + String(number), + '--repo', + repo, + '--json', + 'number,headRefName,isDraft,state', + ]) + const candidate = asRecord(parseJsonContent(result.stdout)) + const candidateNumber = numberValue(candidate?.number) + if ( + candidateNumber !== number || + Boolean(candidate?.isDraft) || + normalizePrState(stringValue(candidate?.state)) !== 'OPEN' + ) return undefined + const headRef = stringValue(candidate?.headRefName) return { repo, number, - ...(snapshot.headRef ? { headRef: snapshot.headRef } : {}), + ...(headRef ? { headRef } : {}), } } diff --git a/src/types.ts b/src/types.ts index c866c23..f15edbf 100644 --- a/src/types.ts +++ b/src/types.ts @@ -57,6 +57,8 @@ export interface FactoryPorts { startupAgentExitDrainTimeoutMs?: number /** Retry cadence for failed automated PR review requests. Test-only override. */ reviewRequestRetryMs?: number + /** Maximum time run-once drains automated PR review retries before returning. Test-only override. */ + reviewRequestRunOnceDrainMs?: number relayflows?: FactoryRelayflowDispatchPort /** Local CLI checkout isolation. Remote fleet nodes own their own checkout lifecycle. */ worktrees?: AgentWorktreeManager From 3c97b771f211ff01175ca9f6cd57cf79684b46dd Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 06:48:53 -0400 Subject: [PATCH 16/30] fix: drain review requests before shutdown --- src/orchestrator/factory.test.ts | 23 ++++++++++++++++++++--- src/orchestrator/factory.ts | 16 +++++++++++++++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 21934f9..0b4072f 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10021,9 +10021,13 @@ describe('FactoryLoop', () => { }, ) - it('does not let a pending automated review request stall lifecycle completion', async () => { + it('does not let a pending automated review request stall lifecycle completion and drains it on stop', async () => { const number = 524 const reviewRequests: Array<{ repo: string; number: number }> = [] + let releaseReviewRequest!: () => void + const reviewRequestSettled = new Promise((resolve) => { + releaseReviewRequest = resolve + }) const githubWrite: GithubConnectionWrite = { publishPullRequest: async (input) => ({ repo: input.repo, @@ -10033,7 +10037,7 @@ describe('FactoryLoop', () => { }), requestPullRequestReview: async (input) => { reviewRequests.push(input) - await new Promise(() => undefined) + await reviewRequestSettled }, closePullRequest: async () => undefined, } @@ -10049,6 +10053,7 @@ describe('FactoryLoop', () => { probePrResolver: async () => undefined, }) + let stopping: Promise | undefined try { await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') @@ -10058,8 +10063,20 @@ describe('FactoryLoop', () => { number, }])) await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + + let stopped = false + stopping = factory.stop().then(() => { + stopped = true + }) + await flush() + expect(stopped).toBe(false) + + releaseReviewRequest() + await stopping + expect(stopped).toBe(true) } finally { - await factory.stop() + releaseReviewRequest() + await (stopping ?? factory.stop()) } }) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index e2d128a..73dd43d 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -910,6 +910,7 @@ export class FactoryLoop implements Factory { this.#dispatchLifecycleRetryTimers.clear() for (const retry of this.#reviewRequestRetryTimers.values()) clearTimeout(retry.timer) this.#reviewRequestRetryTimers.clear() + await this.#drainAutomatedPullRequestReviewWorkForStop() this.#abandonedDispatchReasons.clear() this.#dispatchLifecycleOwnershipWaitLogged.clear() if (this.#completionSweepTimer) clearTimeout(this.#completionSweepTimer) @@ -6220,7 +6221,11 @@ export class FactoryLoop implements Factory { published: AutomatedReviewRequestTarget, ): void { const key = `${published.repo.toLowerCase()}#${published.number}` - if (this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestRetryTimers.has(key)) return + if ( + this.#stopping || + this.#reviewRequestedPullRequests.has(key) || + this.#reviewRequestRetryTimers.has(key) + ) return try { const requestReview = this.#githubPullRequestReviewRequester() if (!requestReview) return @@ -6277,6 +6282,7 @@ export class FactoryLoop implements Factory { ): void { const key = `${published.repo.toLowerCase()}#${published.number}` if ( + this.#stopping || this.#reviewRequestedPullRequests.has(key) || this.#reviewRequestVerifications.has(key) || this.#reviewRequestRetryTimers.has(key) @@ -6314,6 +6320,14 @@ export class FactoryLoop implements Factory { void work.finally(() => this.#reviewRequestWork.delete(work)) } + async #drainAutomatedPullRequestReviewWorkForStop(): Promise { + // A verification can enqueue the request itself as it settles, so drain + // until no tracked generation remains. `#stopping` fences new entry points. + while (this.#reviewRequestWork.size > 0) { + await Promise.allSettled([...this.#reviewRequestWork]) + } + } + async #drainAutomatedPullRequestReviewRetriesForRunOnce(): Promise { const deadline = Date.now() + this.#reviewRequestRunOnceDrainMs const attempts = new Map() From f8d3d88eb69908b094c70d9d09c825ab2eb58b88 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 06:58:03 -0400 Subject: [PATCH 17/30] fix: bound review request shutdown drain --- src/orchestrator/factory.test.ts | 44 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 30 ++++++++++++++++++++-- src/types.ts | 2 ++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 0b4072f..9ede91b 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10080,6 +10080,50 @@ describe('FactoryLoop', () => { } }) + it('bounds stop when a custom automated review requester cannot settle', async () => { + const number = 531 + const warnings: unknown[][] = [] + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async () => { + await new Promise(() => undefined) + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + reviewRequestStopDrainMs: 10, + logger: { + info: () => undefined, + warn: (...args: unknown[]) => warnings.push(args), + error: () => undefined, + }, + }) + + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + await vi.waitFor(() => expect(factory.status().counters.done).toBe(1)) + + await expect(factory.stop()).resolves.toBeUndefined() + expect(warnings).toContainEqual([ + '[factory] automated PR review request drain timed out after 10ms; abandoning execution for restart reconciliation', + { timeoutMs: 10, pendingWork: 1 }, + ]) + }) + it('backs off rejected automated review requests after issue completion', async () => { const number = 527 const reviewRequests: Array<{ repo: string; number: number }> = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 73dd43d..6f1a1a2 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -398,6 +398,7 @@ export class FactoryLoop implements Factory { readonly #startupAgentExitDrainTimeoutMs: number readonly #reviewRequestRetryMs: number readonly #reviewRequestRunOnceDrainMs: number + readonly #reviewRequestStopDrainMs: number readonly #state: StateStore readonly #workspaceId: string readonly #relayflows?: FactoryPorts['relayflows'] @@ -612,6 +613,7 @@ export class FactoryLoop implements Factory { this.#reviewRequestRetryMs = ports.reviewRequestRetryMs ?? AUTOMATED_REVIEW_REQUEST_RETRY_MS this.#reviewRequestRunOnceDrainMs = ports.reviewRequestRunOnceDrainMs ?? AUTOMATED_REVIEW_REQUEST_RUN_ONCE_DRAIN_MS + this.#reviewRequestStopDrainMs = ports.reviewRequestStopDrainMs ?? STOP_TEARDOWN_TIMEOUT_MS this.#workspaceId = config.workspaceId ?? 'default' this.#relayflows = ports.relayflows this.#worktrees = ports.worktrees @@ -6323,8 +6325,32 @@ export class FactoryLoop implements Factory { async #drainAutomatedPullRequestReviewWorkForStop(): Promise { // A verification can enqueue the request itself as it settles, so drain // until no tracked generation remains. `#stopping` fences new entry points. - while (this.#reviewRequestWork.size > 0) { - await Promise.allSettled([...this.#reviewRequestWork]) + // The deadline abandons only execution, not the obligation: publication + // receipts and provider PRs are durable, and restart reconciliation checks + // whether the fixed request landed before issuing another at-least-once try. + const deadline = Date.now() + this.#reviewRequestStopDrainMs + while (this.#reviewRequestWork.size > 0 && Date.now() < deadline) { + let timer: ReturnType | undefined + try { + await Promise.race([ + Promise.allSettled([...this.#reviewRequestWork]), + new Promise((resolve) => { + timer = setTimeout(resolve, Math.max(0, deadline - Date.now())) + timer.unref?.() + }), + ]) + } finally { + if (timer) clearTimeout(timer) + } + } + if (this.#reviewRequestWork.size > 0) { + this.#logger.warn?.( + `[factory] automated PR review request drain timed out after ${this.#reviewRequestStopDrainMs}ms; abandoning execution for restart reconciliation`, + { + timeoutMs: this.#reviewRequestStopDrainMs, + pendingWork: this.#reviewRequestWork.size, + }, + ) } } diff --git a/src/types.ts b/src/types.ts index f15edbf..27a7be5 100644 --- a/src/types.ts +++ b/src/types.ts @@ -59,6 +59,8 @@ export interface FactoryPorts { reviewRequestRetryMs?: number /** Maximum time run-once drains automated PR review retries before returning. Test-only override. */ reviewRequestRunOnceDrainMs?: number + /** Maximum time stop waits before abandoning durable automated PR review work. Test-only override. */ + reviewRequestStopDrainMs?: number relayflows?: FactoryRelayflowDispatchPort /** Local CLI checkout isolation. Remote fleet nodes own their own checkout lifecycle. */ worktrees?: AgentWorktreeManager From 5d057434d6173cecf8b94c043cbdd13e1860b758 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 07:21:44 -0400 Subject: [PATCH 18/30] fix: bound review request reconciliation --- src/github/review-request.ts | 3 + .../relayfile-cloud-mount-client.test.ts | 42 ++++++++++--- src/mount/relayfile-cloud-mount-client.ts | 20 +++++-- .../relayfile-github-connection-write.test.ts | 60 +++++++++++++++++-- .../relayfile-github-connection-write.ts | 12 +++- src/ports/mount.ts | 4 +- src/testing/fakes.ts | 8 ++- 7 files changed, 126 insertions(+), 23 deletions(-) diff --git a/src/github/review-request.ts b/src/github/review-request.ts index 36968d8..0fbdd60 100644 --- a/src/github/review-request.ts +++ b/src/github/review-request.ts @@ -3,6 +3,9 @@ export const FACTORY_CODERABBIT_REVIEW_MARKER = '', + cursor: undefined, + limit: 100, + }, + { + path: canonicalRoot, + provider: 'github', + comment: '', + cursor: 'canonical-page-2', + limit: 100, + }, ]) expect(mount.reads).toEqual([canonicalPath]) }) diff --git a/src/mount/relayfile-github-connection-write.ts b/src/mount/relayfile-github-connection-write.ts index 6460cd4..77a605c 100644 --- a/src/mount/relayfile-github-connection-write.ts +++ b/src/mount/relayfile-github-connection-write.ts @@ -9,7 +9,9 @@ import type { } from '../ports' import { FACTORY_CODERABBIT_REVIEW_BODY, + FACTORY_CODERABBIT_REVIEW_MARKER, containsCoderabbitReviewRequest, + factoryCoderabbitReviewCorrelationId, isAllowedFactoryGithubWritebackDraft, } from '../github/review-request' @@ -29,6 +31,7 @@ export interface RelayfileGithubConnectionWriteConfig { queryFiles(opts: { path: string provider?: string + comment?: string cursor?: string limit?: number }): Promise<{ paths: string[]; nextCursor: string | null }> @@ -159,7 +162,11 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { const commentsRoot = `${repoRoots[0]}/pulls/${number}/comments` const path = `${commentsRoot}/factory-coderabbit-review.json` const content = { body: FACTORY_CODERABBIT_REVIEW_BODY } - const result = await this.#mount.createFile(path, content, { guarded: true }) + const correlationId = factoryCoderabbitReviewCorrelationId(expectedRepo, number) + const result = await this.#mount.createFile(path, content, { + guarded: true, + correlationId, + }) if (result === 'exists') { const existing = (await this.#mount.readFile(path)).content if (!isAllowedFactoryGithubWritebackDraft(path, existing)) { @@ -169,6 +176,7 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { const status = await this.#mount.confirmWrite(path, { timeoutMs: WRITE_CONFIRM_TIMEOUT_MS, returnFailed: true, + correlationId, }) if (status !== 'acked') { const failureReason = status === 'failed' @@ -218,6 +226,7 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { const status = await this.#mount.confirmWrite(path, { timeoutMs: REVIEW_REQUEST_CONFIRM_TIMEOUT_MS, returnFailed: true, + correlationId: factoryCoderabbitReviewCorrelationId(expectedRepo, number), }) if (status === 'acked') return true if (status === 'failed') { @@ -249,6 +258,7 @@ export class RelayfileGithubConnectionWrite implements GithubConnectionWrite { const page = await this.#mount.queryFiles({ path: canonicalCommentsRoot, provider: 'github', + comment: FACTORY_CODERABBIT_REVIEW_MARKER, cursor, limit: REVIEW_REQUEST_QUERY_PAGE_LIMIT, }) diff --git a/src/ports/mount.ts b/src/ports/mount.ts index eec6506..78182bc 100644 --- a/src/ports/mount.ts +++ b/src/ports/mount.ts @@ -134,7 +134,7 @@ export interface MountClient { createFile?( path: string, content: unknown, - opts?: { guarded?: boolean }, + opts?: { guarded?: boolean; correlationId?: string }, ): Promise<'created' | 'exists'> deleteFile(path: string): Promise setDefaultAllowedDraftPredicate?( @@ -150,7 +150,7 @@ export interface MountClient { getSyncStatus?(provider: string): Promise confirmWrite( path: string, - opts?: { timeoutMs?: number; returnFailed?: boolean }, + opts?: { timeoutMs?: number; returnFailed?: boolean; correlationId?: string }, ): Promise<'acked' | 'pending' | 'failed' | 'timeout'> /** Provider failure detail retained for a completed failed write, when available. */ getConfirmedWriteFailureReason?(path: string): Promise diff --git a/src/testing/fakes.ts b/src/testing/fakes.ts index 0a73bfe..81d222b 100644 --- a/src/testing/fakes.ts +++ b/src/testing/fakes.ts @@ -74,7 +74,7 @@ export class FakeMountClient implements MountClient { async createFile( path: string, content: unknown, - _opts?: { guarded?: boolean }, + _opts?: { guarded?: boolean; correlationId?: string }, ): Promise<'created' | 'exists'> { if (this.files.has(path)) return 'exists' this.files.set(path, { content, revision: '1' }) @@ -97,6 +97,7 @@ export class FakeMountClient implements MountClient { async queryFiles(opts: { path: string provider?: string + comment?: string cursor?: string limit?: number }): Promise<{ paths: string[]; nextCursor: string | null }> { @@ -152,7 +153,10 @@ export class FakeMountClient implements MountClient { return events.at(-1)?.id } - async confirmWrite(path: string, _opts?: { timeoutMs?: number }): Promise<'acked' | 'pending' | 'failed' | 'timeout'> { + async confirmWrite( + path: string, + _opts?: { timeoutMs?: number; returnFailed?: boolean; correlationId?: string }, + ): Promise<'acked' | 'pending' | 'failed' | 'timeout'> { return this.#confirmations.get(path) ?? 'acked' } From 80e20e4892e72ecec287ac4c85ad0df68f83b349 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 07:25:38 -0400 Subject: [PATCH 19/30] fix: bind cached write confirmations --- .../relayfile-cloud-mount-client.test.ts | 50 +++++++++++++++++++ src/mount/relayfile-cloud-mount-client.ts | 46 +++++++++++------ 2 files changed, 81 insertions(+), 15 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index 8855ada..dab144b 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -1540,6 +1540,56 @@ describe('RelayfileCloudMountClient', () => { expect(fake.deleteFileCalls).toHaveLength(1) }) + it('does not let a cached delete acknowledge a correlated write after stale draft reappearance', async () => { + const path = '/github/repos/AgentWorkforce/factory/pulls/85/comments/factory-coderabbit-review.json' + const content = { body: '@coderabbitai review\n' } + const fake = new FakeRelayFileClient() + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + mount.setDefaultAllowedDeletePredicate((candidatePath, candidateContent) => + candidatePath === path && JSON.stringify(candidateContent) === JSON.stringify(content)) + + await expect(mount.createFile(path, content, { + guarded: true, + correlationId: reviewCorrelationId, + })).resolves.toBe('created') + fake.ops.set('op-1', { + opId: 'op-1', + path, + action: 'file_upsert', + provider: 'github', + correlationId: reviewCorrelationId, + status: 'failed', + attemptCount: 1, + createdAt: '2026-07-30T01:00:00.000Z', + }) + await expect(mount.confirmWrite(path, { + timeoutMs: 5, + returnFailed: true, + correlationId: reviewCorrelationId, + })).resolves.toBe('failed') + await expect(mount.deleteFile(path)).resolves.toBeUndefined() + + // Provider reconciliation may briefly make the failed local draft visible + // again. The cached deletion op must not satisfy the correlated publish. + fake.files.set(path, { + revision: '2', + content: JSON.stringify(content), + contentType: 'application/json', + }) + await expect(mount.confirmWrite(path, { + timeoutMs: 5, + returnFailed: true, + correlationId: reviewCorrelationId, + })).resolves.toBe('failed') + + expect(fake.listOpsCalls.at(-1)?.options?.correlationId).toBe(reviewCorrelationId) + expect(fake.getOpCalls.at(-1)).toEqual({ workspaceId: 'rw_test', opId: 'op-1' }) + }) + it('bounds restarted operation recovery by the confirmation timeout', async () => { class HangingListOpsClient extends FakeRelayFileClient { override async listOps(): Promise { diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index dccbac5..d596889 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -291,7 +291,10 @@ export class RelayfileCloudMountClient implements MountClient { #disposed = false #isAllowedDraft?: (path: string, content: unknown, opts?: { guarded?: boolean }) => boolean | Promise #isAllowedDelete?: (path: string, currentContent: unknown) => boolean | Promise - readonly #lastOpByPath = new Map() + readonly #lastOpByPath = new Map() readonly #confirmedExternalIdByPath = new Map() readonly #confirmedFailureReasonByPath = new Map() @@ -538,10 +541,10 @@ export class RelayfileCloudMountClient implements MountClient { } try { - this.#lastOpByPath.set(path, (await writeAtCurrentRevision()).opId) + this.#lastOpByPath.set(path, { opId: (await writeAtCurrentRevision()).opId }) } catch (error) { if (!isHttpStatus(error, 409)) throw error - this.#lastOpByPath.set(path, (await writeAtCurrentRevision()).opId) + this.#lastOpByPath.set(path, { opId: (await writeAtCurrentRevision()).opId }) } } @@ -570,7 +573,10 @@ export class RelayfileCloudMountClient implements MountClient { contentType: serialized.contentType, ...(opts?.correlationId ? { correlationId: opts.correlationId } : {}), }) - this.#lastOpByPath.set(path, queued.opId) + this.#lastOpByPath.set(path, { + opId: queued.opId, + ...(opts?.correlationId ? { correlationId: opts.correlationId } : {}), + }) return 'created' } catch (error) { if (!isCreateRevisionConflict(error)) throw error @@ -587,11 +593,13 @@ export class RelayfileCloudMountClient implements MountClient { await this.#assertProviderDeleteAllowed(path, currentContent) } - this.#lastOpByPath.set(path, (await this.#client.deleteFile({ - workspaceId: this.workspaceId, - path, - baseRevision: current.revision, - })).opId) + this.#lastOpByPath.set(path, { + opId: (await this.#client.deleteFile({ + workspaceId: this.workspaceId, + path, + baseRevision: current.revision, + })).opId, + }) } async #assertProviderDeleteAllowed(path: string, currentContent: unknown): Promise { @@ -599,14 +607,14 @@ export class RelayfileCloudMountClient implements MountClient { throw new Error(`Refusing provider delete for ${path}: current record is reconciled or linked`) } - const opId = this.#lastOpByPath.get(path) - if (!opId || !this.#client.getOp) { + const cachedOperation = this.#lastOpByPath.get(path) + if (!cachedOperation || !this.#client.getOp) { throw new Error(`Refusing provider delete for ${path}: create operation is unknown`) } let op: OperationStatusResponse try { - op = await this.#client.getOp(this.workspaceId, opId) + op = await this.#client.getOp(this.workspaceId, cachedOperation.opId) } catch (error) { throw new Error(`Refusing provider delete for ${path}: unable to verify create operation: ${errorMessage(error)}`) } @@ -833,8 +841,11 @@ export class RelayfileCloudMountClient implements MountClient { opts: { timeoutMs?: number; returnFailed?: boolean; correlationId?: string } = {}, ): Promise<'acked' | 'pending' | 'failed' | 'timeout'> { const deadline = Date.now() + (opts.timeoutMs ?? 90_000) - const opId = this.#lastOpByPath.get(path) ?? - await this.#recoverLatestWriteOperation(path, deadline, opts.correlationId) + const cachedOperation = this.#lastOpByPath.get(path) + const opId = cachedOperation && + (!opts.correlationId || cachedOperation.correlationId === opts.correlationId) + ? cachedOperation.opId + : await this.#recoverLatestWriteOperation(path, deadline, opts.correlationId) if (!opId || !this.#client.getOp) return 'timeout' for (;;) { @@ -899,7 +910,12 @@ export class RelayfileCloudMountClient implements MountClient { cursor = page.nextCursor ?? undefined } while (cursor) const latest = uniquelyLatestOperation(matching) - if (latest) this.#lastOpByPath.set(path, latest.opId) + if (latest) { + this.#lastOpByPath.set(path, { + opId: latest.opId, + ...(latest.correlationId ? { correlationId: latest.correlationId } : {}), + }) + } return latest?.opId } From b48a5924274b7df03557fea2499a30bf0cec30f8 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 07:29:45 -0400 Subject: [PATCH 20/30] fix: preserve review backoff in loop mode --- src/orchestrator/factory.test.ts | 50 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 20 +++++++++++-- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 9ede91b..4eaf233 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10190,6 +10190,56 @@ describe('FactoryLoop', () => { } }) + it('does not exhaust automated review retries during normal run-loop iterations', async () => { + const number = 532 + const reviewRequests: Array<{ repo: string; number: number }> = [] + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + throw new Error('persistent provider failure') + }, + closePullRequest: async () => undefined, + } + const mount = new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, githubWrite) + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount, + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: `factory/ar-${number}-pear`, + isDraft: false, + state: 'OPEN', + }), + }), + reviewRequestRetryMs: 60_000, + }) + + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(number), issueFile(number)))) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + await vi.waitFor(() => expect(reviewRequests).toHaveLength(1)) + + const reports = await factory.runLoop({ dryRun: true, maxIterations: 2 }) + + expect(reports).toHaveLength(2) + expect(reports.every((report) => report.error === undefined)).toBe(true) + expect(reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number }]) + expect(factory.status().counters.loopIterationFailures).toBeUndefined() + expect(factory.status().counters.loopCircuitBreaks).toBeUndefined() + }) + it('makes an unmet bounded run-once review drain a named terminal failure', async () => { const number = 530 const reviewRequests: Array<{ repo: string; number: number }> = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 6f1a1a2..9f852f6 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -1109,7 +1109,7 @@ export class FactoryLoop implements Factory { highWatermarkRouteUnavailable: highWatermark.routeUnavailable, }) try { - await this.runOnce() + await this.#runOnce({ drainAutomatedReviewRequests: false }) } catch (error) { // A startup backfill failure must not abort the daemon: log it and fall // back to the live event stream (plus any buffered events) instead of @@ -1692,6 +1692,15 @@ export class FactoryLoop implements Factory { } async runOnce(opts: { dryRun?: boolean } = {}): Promise { + return this.#runOnce({ + ...opts, + drainAutomatedReviewRequests: true, + }) + } + + async #runOnce( + opts: { dryRun?: boolean; drainAutomatedReviewRequests: boolean }, + ): Promise { const dryRun = opts.dryRun ?? this.#config.dryRun const startedAtMs = this.#clock.now() const relayfileWaitWarningsAtStart = this.#counters.relayfileOperationWaitWarnings ?? 0 @@ -1865,7 +1874,9 @@ export class FactoryLoop implements Factory { // A daemon leaves delay-capped retries on their backoff timers. A true // one-shot invocation has no later loop iteration, so drain them before // its caller is allowed to dispose the mount and exit. - if (report && !this.#started) await this.#drainAutomatedPullRequestReviewRetriesForRunOnce() + if (report && opts.drainAutomatedReviewRequests) { + await this.#drainAutomatedPullRequestReviewRetriesForRunOnce() + } if (report) { this.#logger.info?.('[factory] run-once completed', { dryRun, @@ -2361,7 +2372,10 @@ export class FactoryLoop implements Factory { await this.#sweepWaitingClarifications() await this.#drainReadyClarificationWake() await this.#sweepPrStateCompletions('run-loop') - reports.push(await this.runOnce({ dryRun: opts.dryRun })) + reports.push(await this.#runOnce({ + dryRun: opts.dryRun, + drainAutomatedReviewRequests: false, + })) consecutiveFailures = 0 } catch (error) { consecutiveFailures += 1 From ae19188d94d880753c6a32a98a3ea5e9f3ed2b9a Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 07:36:42 -0400 Subject: [PATCH 21/30] fix: preserve immediate write status probes --- .../relayfile-cloud-mount-client.test.ts | 37 +++++++++++++++++++ src/mount/relayfile-cloud-mount-client.ts | 15 +++++--- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/mount/relayfile-cloud-mount-client.test.ts b/src/mount/relayfile-cloud-mount-client.test.ts index dab144b..77dd6d9 100644 --- a/src/mount/relayfile-cloud-mount-client.test.ts +++ b/src/mount/relayfile-cloud-mount-client.test.ts @@ -1281,6 +1281,43 @@ describe('RelayfileCloudMountClient', () => { expect(fake.getOpCalls).toEqual([{ workspaceId: 'rw_test', opId: 'op-1' }]) }) + it('observes an already-acked operation during a zero-timeout status probe', async () => { + const fake = new FakeRelayFileClient() + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await mount.writeFile('/linear/issues/new.json', { title: 'new' }) + + await expect(mount.confirmWrite('/linear/issues/new.json', { timeoutMs: 0 })).resolves.toBe('acked') + expect(fake.getOpCalls).toEqual([{ workspaceId: 'rw_test', opId: 'op-1' }]) + }) + + it('observes an already-failed operation during a zero-timeout status probe', async () => { + const fake = new FakeRelayFileClient() + fake.ops.set('op-1', { + opId: 'op-1', + status: 'failed', + attemptCount: 1, + lastError: 'provider rejected the request', + }) + const mount = new RelayfileCloudMountClient({ + workspaceId: 'rw_test', + client: fake, + isAllowedDraft: () => true, + }) + + await mount.writeFile('/linear/issues/new.json', { title: 'new' }) + + await expect(mount.confirmWrite('/linear/issues/new.json', { + timeoutMs: 0, + returnFailed: true, + })).resolves.toBe('failed') + expect(fake.getOpCalls).toEqual([{ workspaceId: 'rw_test', opId: 'op-1' }]) + }) + it('preserves create-only revision conflicts instead of updating the winner', async () => { class RevisionCheckingClient extends FakeRelayFileClient { override async writeFile(input: Parameters[0]) { diff --git a/src/mount/relayfile-cloud-mount-client.ts b/src/mount/relayfile-cloud-mount-client.ts index d596889..9a05070 100644 --- a/src/mount/relayfile-cloud-mount-client.ts +++ b/src/mount/relayfile-cloud-mount-client.ts @@ -848,12 +848,17 @@ export class RelayfileCloudMountClient implements MountClient { : await this.#recoverLatestWriteOperation(path, deadline, opts.correlationId) if (!opId || !this.#client.getOp) return 'timeout' + let firstPoll = true for (;;) { - if (Date.now() >= deadline) return 'timeout' - const operation = await settleBeforeDeadline( - this.#client.getOp(this.workspaceId, opId), - deadline, - ) + const immediateStatusProbe = firstPoll && opts.timeoutMs === 0 + if (!immediateStatusProbe && Date.now() >= deadline) return 'timeout' + const operation = immediateStatusProbe + ? await this.#client.getOp(this.workspaceId, opId) + : await settleBeforeDeadline( + this.#client.getOp(this.workspaceId, opId), + deadline, + ) + firstPoll = false if (!operation) return 'timeout' let status: 'acked' | 'pending' | 'failed' try { From 803569f1774ba51bbace376b1531298c6e328fb6 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 07:41:38 -0400 Subject: [PATCH 22/30] fix: reverify reconciled review targets --- src/orchestrator/factory.test.ts | 71 +++++++++++++++++++++++++++++++- src/orchestrator/factory.ts | 8 ++-- 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 4eaf233..53c0c24 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -6217,7 +6217,12 @@ describe('FactoryLoop', () => { headRefName: branch, isDraft: false, }]) - : '[]', + : JSON.stringify({ + number: 1597, + headRefName: branch, + isDraft: false, + state: 'OPEN', + }), } }, }) @@ -6245,6 +6250,70 @@ describe('FactoryLoop', () => { await factory.stop() }) + it('reverifies a reconciled PR before requesting review when it closes after head lookup', async () => { + const issue = issueFile(598) + const publishPullRequest = vi.fn(async () => { + throw new Error('must reconcile the existing PR instead of publishing another') + }) + const reviewRequests: Array<{ repo: string; number: number }> = [] + const mount = new FakeMountClient({ [issuePath(598)]: issue }, { + publishPullRequest, + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + }, + closePullRequest: async () => undefined, + }) + const fleet = new DurableRemoteLifecycleFleetClient() + const stateStore = new InMemoryStateStore({ batchSize: 1 }) + const ghCalls: string[][] = [] + let branch = '' + const factory = createFactory(config({ babysitter: { enabled: true } }), { + mount, + fleet, + stateStore, + triage: new StaticTriage(), + probePrGhRunner: async (args) => { + ghCalls.push(args) + return { + stdout: args[1] === 'view' + ? JSON.stringify({ + number: 1598, + headRefName: branch, + isDraft: false, + state: 'CLOSED', + }) + : JSON.stringify([{ + number: 1598, + url: 'https://github.com/AgentWorkforce/pear/pull/1598', + headRefName: branch, + isDraft: false, + }]), + } + }, + }) + const decision = await factory.triageIssue(parseLinearIssue(issuePath(598), issue)) + + await factory.dispatch(decision) + branch = (await stateStore.getDispatchLifecycle('factory-test', issueKey(decision.issue)))! + .decision.implementers[0]!.branch! + fleet.emitAgentExit('ar-598-impl-pear', 'reconciled-missing') + + await vi.waitFor(async () => { + expect(await stateStore.getDispatchLifecycle('factory-test', issueKey(decision.issue))).toMatchObject({ + pullRequest: { + repo: 'AgentWorkforce/pear', + number: 1598, + headRef: branch, + }, + }) + }) + await vi.waitFor(() => + expect(ghCalls.some((args) => args[0] === 'pr' && args[1] === 'view')).toBe(true)) + expect(reviewRequests).toEqual([]) + expect(publishPullRequest).not.toHaveBeenCalled() + await factory.stop() + }) + it('adopts a roster-visible remote spawn after crashing across the ack persistence gap', async () => { class AckGapFleet extends RemoteLifecycleFleetClient { failed = false diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 9f852f6..73e9bcd 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -1588,7 +1588,7 @@ export class FactoryLoop implements Factory { return undefined } if (normalizePrState(pr.state) === 'OPEN') { - this.#requestAutomatedPullRequestReview({ + this.#requestAutomatedPullRequestReviewForOpenReceipt({ repo: pr.repo, number: pr.prNumber, headRef: pr.headRef, @@ -6185,7 +6185,7 @@ export class FactoryLoop implements Factory { const existing = await this.#openPullRequestByHead(repo, expectedHeadRef) if (existing) { this.#publishedPullRequests.set(key, existing) - this.#requestAutomatedPullRequestReview(existing) + this.#requestAutomatedPullRequestReviewForOpenReceipt(existing) this.#increment('githubPullRequestsReconciled') this.#logger.info?.('[factory] reconciled existing PR from implementer branch', { issue: issue.key, @@ -7104,7 +7104,7 @@ export class FactoryLoop implements Factory { } const existing = await this.#openPullRequestByHead(repo, implementer.spec.branch) if (existing) { - this.#requestAutomatedPullRequestReview(existing) + this.#requestAutomatedPullRequestReviewForOpenReceipt(existing) return true } return false @@ -7118,7 +7118,7 @@ export class FactoryLoop implements Factory { : await this.#completionPrForIssue(issue) if (!pr || pr.draft) return false if (normalizePrState(pr.state) === 'OPEN') { - this.#requestAutomatedPullRequestReview({ + this.#requestAutomatedPullRequestReviewForOpenReceipt({ repo: pr.repo, number: pr.prNumber, headRef: pr.headRef, From 75a214df98eec72a174bc79682c4091037756c42 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 08:31:28 -0400 Subject: [PATCH 23/30] test: model authoritative review verification --- src/orchestrator/factory.test.ts | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 53c0c24..ade5cb7 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -3728,6 +3728,14 @@ describe('FactoryLoop', () => { githubWriteback, mergeGate, probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 50, state: 'OPEN' }), + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number: 50, + headRefName: 'github-head', + isDraft: false, + state: 'OPEN', + }), + }), }) await factory.runOnce() @@ -12488,6 +12496,17 @@ describe('FactoryLoop', () => { triage: new StaticTriage(), probePrGhRunner: async (args) => { ghCalls.push(args) + if (args[0] === 'pr' && args[1] === 'view') { + const number = Number(args[2]) + return { + stdout: JSON.stringify({ + number, + headRefName: number === 856 ? 'ar-355-is-odd-v2' : 'ar-356-square', + isDraft: false, + state: 'OPEN', + }), + } + } return { stdout: JSON.stringify([ ghPr(880, { @@ -12526,7 +12545,10 @@ describe('FactoryLoop', () => { await factory.runOnce() await factory.runLoop({ maxIterations: 1 }) - expect(ghCalls).toHaveLength(2) + const listCalls = ghCalls.filter((args) => args[0] === 'pr' && args[1] === 'list') + const viewCalls = ghCalls.filter((args) => args[0] === 'pr' && args[1] === 'view') + expect(listCalls).toHaveLength(2) + expect(viewCalls.map((args) => Number(args[2])).sort((a, b) => a - b)).toEqual([856, 857]) expect(ghCalls.every((args) => args.includes('--repo') && args.includes('AgentWorkforce/pear'))).toBe(true) expect(closeInputs).toEqual([ { repo: 'AgentWorkforce/pear', prNumber: 856, expectedIssueKey: 'AR-355', requireTitleMarker: false }, From b47779f2a4457fdfca3b42d801b6816c2f350f89 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 08:40:47 -0400 Subject: [PATCH 24/30] fix: recover review requests after restart --- src/orchestrator/factory.test.ts | 75 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 21 +++++++-- 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index ade5cb7..c0b48f6 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10201,6 +10201,81 @@ describe('FactoryLoop', () => { ]) }) + it('reconciles a terminal lifecycle review obligation after process restart', async () => { + const number = 533 + const path = issuePath(number) + const issue = issueFile(number) + const stateStore = new InMemoryStateStore({ batchSize: 2 }) + const seedFactory = createFactory(config(), { + mount: new FakeMountClient({ [path]: issue }), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + }) + const decision = await seedFactory.triageIssue(parseLinearIssue(path, issue)) + const receipt = { + repo: 'AgentWorkforce/pear', + number, + url: `https://github.com/AgentWorkforce/pear/pull/${number}`, + headRef: `factory/ar-${number}-pear`, + } + await stateStore.claimDispatchLifecycle( + 'factory-test', + issueKey(decision.issue), + { + runId: 'terminal-review-restart', + issue: { uuid: decision.issue.uuid, key: decision.issue.key, path: decision.issue.path }, + decision, + dryRun: false, + phase: 'complete', + agents: [], + invocationIds: [], + pullRequests: [receipt], + pullRequest: receipt, + updatedAtMs: 0, + }, + 'stopped-owner', + 0, + 1, + ) + + const reviewRequests: Array<{ repo: string; number: number }> = [] + const githubWrite: GithubConnectionWrite = { + publishPullRequest: async () => { + throw new Error('restart recovery must reuse the durable receipt') + }, + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + }, + closePullRequest: async () => undefined, + } + const restarted = createFactory(config(), { + mount: new FakeMountClient({ [path]: issue }, githubWrite), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: receipt.headRef, + isDraft: false, + state: 'OPEN', + }), + }), + }) + + try { + await restarted.start({ mode: 'dispatch-owner' }) + await vi.waitFor(() => expect(reviewRequests).toEqual([{ + repo: receipt.repo, + number, + }])) + } finally { + await restarted.stop() + await seedFactory.stop() + } + }) + it('backs off rejected automated review requests after issue completion', async () => { const number = 527 const reviewRequests: Array<{ repo: string; number: number }> = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 73e9bcd..4816083 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -823,6 +823,7 @@ export class FactoryLoop implements Factory { this.#wireFleetEvents() await this.#adoptInFlightAgents(legacyRegistry) this.#startupAgentAdoptionActive = false + await this.#reconcileTerminalAutomatedReviewRequests() if (opts.mode !== 'dispatch-owner') await this.#reapOrphanedWorktreesOnStartup(legacyRegistry) if (this.#config.babysitter.enabled) { // Re-run the idempotent receipt fold after adoption returns. This @@ -6331,6 +6332,20 @@ export class FactoryLoop implements Factory { .finally(() => this.#reviewRequestVerifications.delete(key))) } + async #reconcileTerminalAutomatedReviewRequests(): Promise { + const lifecycles = await this.#state.listDispatchLifecycles(this.#workspaceId) + const receipts = new Map() + for (const [, lifecycle] of lifecycles) { + if (!isTerminalDispatchLifecycle(lifecycle)) continue + for (const receipt of publishedPullRequests(lifecycle)) { + receipts.set(`${receipt.repo.toLowerCase()}#${receipt.number}`, receipt) + } + } + for (const receipt of receipts.values()) { + this.#requestAutomatedPullRequestReviewForOpenReceipt(receipt) + } + } + #trackAutomatedPullRequestReviewWork(work: Promise): void { this.#reviewRequestWork.add(work) void work.finally(() => this.#reviewRequestWork.delete(work)) @@ -6339,9 +6354,9 @@ export class FactoryLoop implements Factory { async #drainAutomatedPullRequestReviewWorkForStop(): Promise { // A verification can enqueue the request itself as it settles, so drain // until no tracked generation remains. `#stopping` fences new entry points. - // The deadline abandons only execution, not the obligation: publication - // receipts and provider PRs are durable, and restart reconciliation checks - // whether the fixed request landed before issuing another at-least-once try. + // The deadline abandons only execution, not the obligation: terminal + // lifecycle receipts are durable, and startup reconciliation verifies + // every open PR before issuing another at-least-once try. const deadline = Date.now() + this.#reviewRequestStopDrainMs while (this.#reviewRequestWork.size > 0 && Date.now() < deadline) { let timer: ReturnType | undefined From 90344cd5a0e52d11e72f6837077d9dc3d70564aa Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 09:35:39 -0400 Subject: [PATCH 25/30] fix: bound automated review recovery --- src/orchestrator/factory.test.ts | 177 +++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 64 +++++++++-- src/types.ts | 2 + 3 files changed, 235 insertions(+), 8 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index c0b48f6..ff21125 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10276,6 +10276,183 @@ describe('FactoryLoop', () => { } }) + it('contains a failed startup review reconciliation without failing the lifecycle owner', async () => { + class ReconciliationFailingStateStore extends InMemoryStateStore { + listCalls = 0 + + override async listDispatchLifecycles( + workspaceId: string, + ): Promise>> { + this.listCalls += 1 + if (this.listCalls === 2) throw new Error('relayfile lifecycle listing unavailable') + return super.listDispatchLifecycles(workspaceId) + } + } + + const warnings: unknown[][] = [] + const stateStore = new ReconciliationFailingStateStore({ batchSize: 2 }) + const factory = createFactory(config(), { + mount: new FakeMountClient(), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + logger: { + info: () => undefined, + warn: (...args: unknown[]) => warnings.push(args), + error: () => undefined, + }, + }) + + try { + await expect(factory.start({ mode: 'dispatch-owner' })).resolves.toBeUndefined() + expect(warnings).toContainEqual([ + '[factory] automated PR review startup reconciliation deferred; lifecycle completion remains independent', + { error: 'relayfile lifecycle listing unavailable' }, + ]) + } finally { + await factory.stop() + } + }) + + it('serializes startup review reconciliation without blocking lifecycle-owner startup', async () => { + const stateStore = new InMemoryStateStore({ batchSize: 2 }) + const receipts = [534, 535].map((number) => ({ + repo: 'AgentWorkforce/pear', + number, + url: `https://github.com/AgentWorkforce/pear/pull/${number}`, + headRef: `factory/ar-${number}-pear`, + })) + for (const receipt of receipts) { + const issue = parseLinearIssue(issuePath(receipt.number), issueFile(receipt.number)) + const decision = await new StaticTriage().triage(issue) + await stateStore.claimDispatchLifecycle( + 'factory-test', + issueKey(decision.issue), + { + runId: `terminal-review-restart-${receipt.number}`, + issue: { uuid: decision.issue.uuid, key: decision.issue.key, path: decision.issue.path }, + decision, + dryRun: false, + phase: 'complete', + agents: [], + invocationIds: [], + pullRequests: [receipt], + pullRequest: receipt, + updatedAtMs: 0, + }, + 'stopped-owner', + 0, + 1, + ) + } + + let releaseFirstLookup!: () => void + const firstLookup = new Promise((resolve) => { + releaseFirstLookup = resolve + }) + const lookupNumbers: number[] = [] + const reviewRequests: number[] = [] + const factory = createFactory(config(), { + mount: new FakeMountClient({}, { + publishPullRequest: async () => { + throw new Error('restart recovery must reuse durable receipts') + }, + requestPullRequestReview: async ({ number }) => { + reviewRequests.push(number) + }, + closePullRequest: async () => undefined, + }), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + probePrGhRunner: async (args) => { + const number = Number(args[2]) + lookupNumbers.push(number) + if (lookupNumbers.length === 1) await firstLookup + return { + stdout: JSON.stringify({ + number, + headRefName: `factory/ar-${number}-pear`, + isDraft: false, + state: 'OPEN', + }), + } + }, + }) + + try { + await expect(factory.start({ mode: 'dispatch-owner' })).resolves.toBeUndefined() + expect(lookupNumbers).toHaveLength(1) + expect(reviewRequests).toEqual([]) + + releaseFirstLookup() + await vi.waitFor(() => expect(lookupNumbers).toEqual([534, 535])) + await vi.waitFor(() => expect(reviewRequests).toEqual([534, 535])) + } finally { + releaseFirstLookup() + await factory.stop() + } + }) + + it('times out a hung review verification, releases its key, and retries the durable obligation', async () => { + const number = 536 + const reviewRequests: Array<{ repo: string; number: number }> = [] + let reviewAttempts = 0 + let lookupAttempts = 0 + const fleet = new FakeFleetClient() + const factory = createFactory(config(), { + mount: new FakeMountClient({ + [issuePath(number)]: issueFile(number), + '/github/repos/AgentWorkforce/pear/meta.json': { default_branch: 'main' }, + }, { + publishPullRequest: async (input) => ({ + repo: input.repo, + number, + url: `https://github.com/${input.repo}/pull/${number}`, + headRef: input.headRef!, + }), + requestPullRequestReview: async (input) => { + reviewRequests.push(input) + reviewAttempts += 1 + if (reviewAttempts === 1) throw new Error('initial provider failure') + }, + closePullRequest: async () => undefined, + }), + fleet, + triage: new StaticTriage(), + probePrResolver: async () => undefined, + probePrGhRunner: async () => { + lookupAttempts += 1 + if (lookupAttempts === 1) await new Promise(() => undefined) + return { + stdout: JSON.stringify({ + number, + headRefName: `factory/ar-${number}-pear`, + isDraft: false, + state: 'OPEN', + }), + } + }, + reviewRequestRetryMs: 5, + reviewRequestVerificationTimeoutMs: 10, + }) + + try { + await factory.dispatch(await factory.triageIssue( + parseLinearIssue(issuePath(number), issueFile(number)), + )) + fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') + + await vi.waitFor(() => expect(lookupAttempts).toBe(2)) + await vi.waitFor(() => expect(reviewRequests).toEqual([ + { repo: 'AgentWorkforce/pear', number }, + { repo: 'AgentWorkforce/pear', number }, + ])) + } finally { + await factory.stop() + } + }) + it('backs off rejected automated review requests after issue completion', async () => { const number = 527 const reviewRequests: Array<{ repo: string; number: number }> = [] diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 4816083..8360354 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -270,6 +270,7 @@ const PUBLISHED_PR_CONFIRM_ATTEMPTS = 20 const PUBLISHED_PR_CONFIRM_DELAY_MS = 100 const AUTOMATED_REVIEW_REQUEST_RETRY_MS = 5_000 const AUTOMATED_REVIEW_REQUEST_MAX_DELAY_MS = 60_000 +const AUTOMATED_REVIEW_REQUEST_VERIFICATION_TIMEOUT_MS = 30_000 const AUTOMATED_REVIEW_REQUEST_RUN_ONCE_ATTEMPTS = 5 const AUTOMATED_REVIEW_REQUEST_RUN_ONCE_DRAIN_MS = 10_000 const SLACK_REPLY_EVENTS_LIMIT = 100 @@ -397,6 +398,7 @@ export class FactoryLoop implements Factory { readonly #babysitterWakeUnreachableRetryMs: number readonly #startupAgentExitDrainTimeoutMs: number readonly #reviewRequestRetryMs: number + readonly #reviewRequestVerificationTimeoutMs: number readonly #reviewRequestRunOnceDrainMs: number readonly #reviewRequestStopDrainMs: number readonly #state: StateStore @@ -611,6 +613,8 @@ export class FactoryLoop implements Factory { this.#babysitterWakeUnreachableRetryMs = ports.babysitterWakeUnreachableRetryMs ?? BABYSITTER_WAKE_UNREACHABLE_RETRY_MS this.#startupAgentExitDrainTimeoutMs = ports.startupAgentExitDrainTimeoutMs ?? STARTUP_AGENT_EXIT_DRAIN_TIMEOUT_MS this.#reviewRequestRetryMs = ports.reviewRequestRetryMs ?? AUTOMATED_REVIEW_REQUEST_RETRY_MS + this.#reviewRequestVerificationTimeoutMs = + ports.reviewRequestVerificationTimeoutMs ?? AUTOMATED_REVIEW_REQUEST_VERIFICATION_TIMEOUT_MS this.#reviewRequestRunOnceDrainMs = ports.reviewRequestRunOnceDrainMs ?? AUTOMATED_REVIEW_REQUEST_RUN_ONCE_DRAIN_MS this.#reviewRequestStopDrainMs = ports.reviewRequestStopDrainMs ?? STOP_TEARDOWN_TIMEOUT_MS @@ -6296,7 +6300,7 @@ export class FactoryLoop implements Factory { #requestAutomatedPullRequestReviewForOpenReceipt( published: AutomatedReviewRequestTarget, - ): void { + ): Promise | undefined { const key = `${published.repo.toLowerCase()}#${published.number}` if ( this.#stopping || @@ -6308,8 +6312,11 @@ export class FactoryLoop implements Factory { // Retry authorization must come from an authoritative point lookup. The // head-based discovery helper may fall back to webhook-fed mount metadata, // which is useful for reconciliation but cannot prove a PR remains open. - const openLookup = this.#openPullRequestByNumber(published.repo, published.number) - this.#trackAutomatedPullRequestReviewWork(openLookup + const openLookup = this.#openPullRequestByNumberWithinVerificationDeadline( + published.repo, + published.number, + ) + const work = openLookup .then((open) => { if ( open?.number === published.number && @@ -6329,11 +6336,22 @@ export class FactoryLoop implements Factory { }) this.#scheduleAutomatedPullRequestReviewRetry(published) }) - .finally(() => this.#reviewRequestVerifications.delete(key))) + .finally(() => this.#reviewRequestVerifications.delete(key)) + this.#trackAutomatedPullRequestReviewWork(work) + return work } async #reconcileTerminalAutomatedReviewRequests(): Promise { - const lifecycles = await this.#state.listDispatchLifecycles(this.#workspaceId) + let lifecycles: Array<[string, DispatchLifecycle]> + try { + lifecycles = await this.#state.listDispatchLifecycles(this.#workspaceId) + } catch (error) { + this.#increment('githubPullRequestReviewRequestFailures') + this.#logger.warn?.('[factory] automated PR review startup reconciliation deferred; lifecycle completion remains independent', { + error: describeError(error).errorMessage, + }) + return + } const receipts = new Map() for (const [, lifecycle] of lifecycles) { if (!isTerminalDispatchLifecycle(lifecycle)) continue @@ -6341,9 +6359,17 @@ export class FactoryLoop implements Factory { receipts.set(`${receipt.repo.toLowerCase()}#${receipt.number}`, receipt) } } - for (const receipt of receipts.values()) { - this.#requestAutomatedPullRequestReviewForOpenReceipt(receipt) - } + // Reconciliation is intentionally detached from startup and serialized. + // A large durable history must not become an unbounded burst of `gh pr + // view` subprocesses, and a review-request outage must not prevent the + // lifecycle owner from starting. Receipts stay durable for the next pass. + const reconciliation = (async () => { + for (const receipt of receipts.values()) { + if (this.#stopping) return + await this.#requestAutomatedPullRequestReviewForOpenReceipt(receipt) + } + })() + this.#trackAutomatedPullRequestReviewWork(reconciliation) } #trackAutomatedPullRequestReviewWork(work: Promise): void { @@ -6602,6 +6628,28 @@ export class FactoryLoop implements Factory { } } + async #openPullRequestByNumberWithinVerificationDeadline( + repo: string, + number: number, + ): Promise { + let timer: ReturnType | undefined + try { + return await Promise.race([ + this.#openPullRequestByNumber(repo, number), + new Promise((_, reject) => { + timer = setTimeout(() => { + reject(new Error( + `Authoritative GitHub pull request lookup timed out after ${this.#reviewRequestVerificationTimeoutMs}ms`, + )) + }, this.#reviewRequestVerificationTimeoutMs) + timer.unref?.() + }), + ]) + } finally { + if (timer) clearTimeout(timer) + } + } + async #prepareAgentWorktree(record: InFlightIssue, spec: AgentSpec): Promise { const worktree = this.#agentWorktree(record, spec) if (!worktree || !this.#worktrees) return diff --git a/src/types.ts b/src/types.ts index 27a7be5..88cc04b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -57,6 +57,8 @@ export interface FactoryPorts { startupAgentExitDrainTimeoutMs?: number /** Retry cadence for failed automated PR review requests. Test-only override. */ reviewRequestRetryMs?: number + /** Maximum time an authoritative PR-state lookup may hold a review retry lock. Test-only override. */ + reviewRequestVerificationTimeoutMs?: number /** Maximum time run-once drains automated PR review retries before returning. Test-only override. */ reviewRequestRunOnceDrainMs?: number /** Maximum time stop waits before abandoning durable automated PR review work. Test-only override. */ From a6af6a1b2c7730ac3b791bf2be3d7b641789843e Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 09:55:27 -0400 Subject: [PATCH 26/30] fix: cancel timed out review lookups --- src/github/index.ts | 1 + src/github/merge-gate.ts | 14 +++++++-- src/index.ts | 1 + src/orchestrator/factory.test.ts | 41 +++++++++++++++++++++---- src/orchestrator/factory.ts | 52 ++++++++++++++++++++------------ 5 files changed, 80 insertions(+), 29 deletions(-) diff --git a/src/github/index.ts b/src/github/index.ts index 72b6b02..feb621d 100644 --- a/src/github/index.ts +++ b/src/github/index.ts @@ -14,6 +14,7 @@ export { standaloneBabysitterAgentName, } from './standalone-babysitter' export type { + GhRunOptions, GhRunner, GhRunResult, GithubMergeInput, diff --git a/src/github/merge-gate.ts b/src/github/merge-gate.ts index 6b11766..5767310 100644 --- a/src/github/merge-gate.ts +++ b/src/github/merge-gate.ts @@ -8,7 +8,12 @@ export interface GhRunResult { stderr?: string } -export type GhRunner = (args: string[]) => Promise +export interface GhRunOptions { + /** Cancels the underlying `gh` subprocess; callers must await its settlement before retrying. */ + signal?: AbortSignal +} + +export type GhRunner = (args: string[], options?: GhRunOptions) => Promise export interface GithubMergeGateInput { repo: string @@ -172,10 +177,13 @@ export function evaluateGithubMergeGate( } } -export const defaultGhRunner: GhRunner = async (args) => { +export const defaultGhRunner: GhRunner = async (args, options) => { // TODO(issue-52): retire this compatibility runner when merge-gate reads and // guarded merges are fully represented by the mounted GitHub connection. - const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 }) + const { stdout, stderr } = await execFileAsync('gh', args, { + maxBuffer: 1024 * 1024, + signal: options?.signal, + }) return { stdout, stderr } } diff --git a/src/index.ts b/src/index.ts index 5eee6e0..01e9c18 100644 --- a/src/index.ts +++ b/src/index.ts @@ -114,6 +114,7 @@ export { export type { CloseProbePrInput, CloseProbePrResult, + GhRunOptions, GhRunner, GhRunResult, GithubMergeInput, diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index ff21125..91df2be 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10394,11 +10394,14 @@ describe('FactoryLoop', () => { } }) - it('times out a hung review verification, releases its key, and retries the durable obligation', async () => { + it('cancels repeated hung review verifications before retrying the durable obligation', async () => { const number = 536 const reviewRequests: Array<{ repo: string; number: number }> = [] let reviewAttempts = 0 let lookupAttempts = 0 + let activeLookups = 0 + let maximumActiveLookups = 0 + let abortedLookups = 0 const fleet = new FakeFleetClient() const factory = createFactory(config(), { mount: new FakeMountClient({ @@ -10421,9 +10424,32 @@ describe('FactoryLoop', () => { fleet, triage: new StaticTriage(), probePrResolver: async () => undefined, - probePrGhRunner: async () => { + probePrGhRunner: async (_args, options) => { lookupAttempts += 1 - if (lookupAttempts === 1) await new Promise(() => undefined) + if (lookupAttempts <= 3) { + activeLookups += 1 + maximumActiveLookups = Math.max(maximumActiveLookups, activeLookups) + await new Promise((_, reject) => { + const signal = options?.signal + if (!signal) { + activeLookups -= 1 + reject(new Error('verification lookup did not receive a cancellation signal')) + return + } + const abort = () => { + activeLookups -= 1 + abortedLookups += 1 + const error = new Error('synthetic gh lookup aborted') + error.name = 'AbortError' + reject(error) + } + if (signal.aborted) { + abort() + } else { + signal.addEventListener('abort', abort, { once: true }) + } + }) + } return { stdout: JSON.stringify({ number, @@ -10433,8 +10459,8 @@ describe('FactoryLoop', () => { }), } }, - reviewRequestRetryMs: 5, - reviewRequestVerificationTimeoutMs: 10, + reviewRequestRetryMs: 1, + reviewRequestVerificationTimeoutMs: 5, }) try { @@ -10443,11 +10469,14 @@ describe('FactoryLoop', () => { )) fleet.emitAgentExit(`ar-${number}-impl-pear`, 'issue-done') - await vi.waitFor(() => expect(lookupAttempts).toBe(2)) + await vi.waitFor(() => expect(lookupAttempts).toBe(4)) await vi.waitFor(() => expect(reviewRequests).toEqual([ { repo: 'AgentWorkforce/pear', number }, { repo: 'AgentWorkforce/pear', number }, ])) + expect(abortedLookups).toBe(3) + expect(maximumActiveLookups).toBe(1) + expect(activeLookups).toBe(0) } finally { await factory.stop() } diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 8360354..3ec3a53 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -6600,19 +6600,23 @@ export class FactoryLoop implements Factory { async #openPullRequestByNumber( repo: string, number: number, + signal?: AbortSignal, ): Promise { if (!this.#hasProbePrGhRunner) { throw new Error('Authoritative GitHub pull request lookup is unavailable') } - const result = await this.#probePrGhRunner([ - 'pr', - 'view', - String(number), - '--repo', - repo, - '--json', - 'number,headRefName,isDraft,state', - ]) + const result = await this.#probePrGhRunner( + [ + 'pr', + 'view', + String(number), + '--repo', + repo, + '--json', + 'number,headRefName,isDraft,state', + ], + { signal }, + ) const candidate = asRecord(parseJsonContent(result.stdout)) const candidateNumber = numberValue(candidate?.number) if ( @@ -6632,19 +6636,27 @@ export class FactoryLoop implements Factory { repo: string, number: number, ): Promise { + const controller = new AbortController() + let timedOut = false let timer: ReturnType | undefined try { - return await Promise.race([ - this.#openPullRequestByNumber(repo, number), - new Promise((_, reject) => { - timer = setTimeout(() => { - reject(new Error( - `Authoritative GitHub pull request lookup timed out after ${this.#reviewRequestVerificationTimeoutMs}ms`, - )) - }, this.#reviewRequestVerificationTimeoutMs) - timer.unref?.() - }), - ]) + // A logical Promise.race would release the verification key while the + // physical `gh` process kept running. Abort the runner and await that + // operation's rejection so retries cannot accumulate subprocesses. + timer = setTimeout(() => { + timedOut = true + controller.abort() + }, this.#reviewRequestVerificationTimeoutMs) + timer.unref?.() + return await this.#openPullRequestByNumber(repo, number, controller.signal) + } catch (error) { + if (timedOut) { + throw new Error( + `Authoritative GitHub pull request lookup timed out after ${this.#reviewRequestVerificationTimeoutMs}ms`, + { cause: error }, + ) + } + throw error } finally { if (timer) clearTimeout(timer) } From 3fc578fe879c63a9d86ec9c3dd3db0d5d4a5af13 Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 10:15:08 -0400 Subject: [PATCH 27/30] fix: preserve PR publisher identity --- src/orchestrator/factory.test.ts | 107 +++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 36 +++++++--- src/ports/mount.ts | 2 + src/state/file-state-store.test.ts | 3 +- 4 files changed, 137 insertions(+), 11 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 91df2be..ffd8b82 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10276,6 +10276,113 @@ describe('FactoryLoop', () => { } }) + it.each([ + { + publisherIdentity: 'user' as const, + appAvailable: true, + expectedRequester: 'user' as const, + }, + { + publisherIdentity: 'app' as const, + appAvailable: true, + expectedRequester: 'app' as const, + }, + { + publisherIdentity: 'app' as const, + appAvailable: false, + expectedRequester: undefined, + }, + ])( + 'preserves $publisherIdentity publisher identity during auto recovery when appAvailable=$appAvailable', + async ({ publisherIdentity, appAvailable, expectedRequester }) => { + const number = publisherIdentity === 'user' ? 534 : appAvailable ? 535 : 536 + const path = issuePath(number) + const issue = issueFile(number) + const stateStore = new InMemoryStateStore({ batchSize: 2 }) + const seedFactory = createFactory(config(), { + mount: new FakeMountClient({ [path]: issue }), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + }) + const decision = await seedFactory.triageIssue(parseLinearIssue(path, issue)) + const receipt = { + repo: 'AgentWorkforce/pear', + number, + url: `https://github.com/AgentWorkforce/pear/pull/${number}`, + headRef: `factory/ar-${number}-pear`, + publisherIdentity, + } + await stateStore.claimDispatchLifecycle( + 'factory-test', + issueKey(decision.issue), + { + runId: `terminal-review-${publisherIdentity}-${appAvailable}`, + issue: { uuid: decision.issue.uuid, key: decision.issue.key, path: decision.issue.path }, + decision, + dryRun: false, + phase: 'complete', + agents: [], + invocationIds: [], + pullRequests: [receipt], + pullRequest: receipt, + updatedAtMs: 0, + }, + 'stopped-owner', + 0, + 1, + ) + + const appReviewRequests: Array<{ repo: string; number: number }> = [] + const userWriteback = new PublishingGithubWriteback({ number, author: 'operator-user' }) + const appWrite: GithubConnectionWrite = { + publishPullRequest: async () => { + throw new Error('restart recovery must reuse the durable receipt') + }, + requestPullRequestReview: async (input) => { + appReviewRequests.push(input) + }, + closePullRequest: async () => undefined, + } + const restarted = createFactory(config({ github: { identity: 'auto' } }), { + mount: new FakeMountClient({ [path]: issue }, appAvailable ? appWrite : undefined), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + githubWriteback: userWriteback, + reviewRequestRetryMs: 60_000, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: receipt.headRef, + isDraft: false, + state: 'OPEN', + }), + }), + }) + + try { + await restarted.start({ mode: 'dispatch-owner' }) + if (expectedRequester === 'app') { + await vi.waitFor(() => expect(appReviewRequests).toEqual([{ repo: receipt.repo, number }])) + expect(userWriteback.reviewRequests).toEqual([]) + } else if (expectedRequester === 'user') { + await vi.waitFor(() => + expect(userWriteback.reviewRequests).toEqual([{ repo: receipt.repo, number }])) + expect(appReviewRequests).toEqual([]) + } else { + await vi.waitFor(() => + expect(restarted.status().counters.githubPullRequestReviewRequestFailures).toBe(1)) + expect(appReviewRequests).toEqual([]) + expect(userWriteback.reviewRequests).toEqual([]) + } + } finally { + await restarted.stop() + await seedFactory.stop() + } + }, + ) + it('contains a failed startup review reconciliation without failing the lifecycle owner', async () => { class ReconciliationFailingStateStore extends InMemoryStateStore { listCalls = 0 diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 3ec3a53..0566a87 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -135,7 +135,10 @@ type EventHighWatermarkResult = { highWatermark?: string; routeUnavailable: bool type PreparedLiveEvent = { path?: string; dispatchRelayflow: boolean } type GithubPullRequestPublisher = Pick type GithubPullRequestIdentity = 'app' | 'user' -type AutomatedReviewRequestTarget = GithubPullRequestRef & { headRef?: string } +type AutomatedReviewRequestTarget = GithubPullRequestRef & { + headRef?: string + publisherIdentity?: GithubPullRequestIdentity +} type GithubOrphanRecoveryContext = { activeIssueIdentities: Set onlineAgentNames: Set @@ -6209,9 +6212,11 @@ export class FactoryLoop implements Factory { title: `${issue.key}: ${issue.title}`, body: githubPullRequestBody(issue, implementer.spec.preview), }) - const published = result.author - ? result - : { ...result, author: identity } + const published = { + ...result, + ...(result.author ? {} : { author: identity }), + publisherIdentity: identity, + } if ( published.repo.toLowerCase() !== repo.toLowerCase() || published.headRef !== (remoteBranch ?? published.headRef) || @@ -6248,7 +6253,7 @@ export class FactoryLoop implements Factory { this.#reviewRequestRetryTimers.has(key) ) return try { - const requestReview = this.#githubPullRequestReviewRequester() + const requestReview = this.#githubPullRequestReviewRequester(published) if (!requestReview) return this.#reviewRequestedPullRequests.add(key) this.#trackAutomatedPullRequestReviewWork(Promise.resolve() @@ -6322,7 +6327,10 @@ export class FactoryLoop implements Factory { open?.number === published.number && (!published.headRef || open.headRef === published.headRef) ) { - this.#requestAutomatedPullRequestReview(open) + this.#requestAutomatedPullRequestReview({ + ...open, + publisherIdentity: published.publisherIdentity, + }) } else { this.#reviewRequestAttempts.delete(key) } @@ -6458,14 +6466,22 @@ export class FactoryLoop implements Factory { ) } - #githubPullRequestReviewRequester(): ((input: GithubPullRequestRef) => Promise) | undefined { + #githubPullRequestReviewRequester( + published: AutomatedReviewRequestTarget, + ): ((input: GithubPullRequestRef) => Promise) | undefined { const configured = this.#config.github.identity - if (configured !== 'user' && this.#mount.githubWrite) { + const identity = published.publisherIdentity ?? (configured === 'auto' ? undefined : configured) + if (!identity) { + throw new Error( + 'Automated PR review request requires the persisted publisher identity when github.identity is "auto"; refusing to select an identity from current availability', + ) + } + if (identity === 'app' && this.#mount.githubWrite) { return this.#mount.githubWrite.requestPullRequestReview?.bind(this.#mount.githubWrite) } - if (configured === 'app') { + if (identity === 'app') { throw new Error( - 'GitHub PR identity "app" requires a connected workspace GitHub App write path; refusing to fall back to the local gh user', + 'The PR was published with GitHub App identity, but no connected workspace GitHub App write path is available; refusing to fall back to the local gh user', ) } if (this.#mount.writebackTransport === 'test' && !this.#githubWritebackProvided) return undefined diff --git a/src/ports/mount.ts b/src/ports/mount.ts index 78182bc..40ce926 100644 --- a/src/ports/mount.ts +++ b/src/ports/mount.ts @@ -63,6 +63,8 @@ export interface GithubPublishPullRequestResult { headSha?: string /** Provider-confirmed login or identity label used to author the PR. */ author?: string + /** Durable write path that authored the PR; review requests must preserve it. */ + publisherIdentity?: 'app' | 'user' } export interface GithubPullRequestRef { diff --git a/src/state/file-state-store.test.ts b/src/state/file-state-store.test.ts index ee55ff0..5faaa3a 100644 --- a/src/state/file-state-store.test.ts +++ b/src/state/file-state-store.test.ts @@ -39,6 +39,7 @@ describe('FileStateStore', () => { number: 85, url: 'https://github.com/AgentWorkforce/factory/pull/85', headRef: 'factory/ar-85-agentworkforce-factory', + publisherIdentity: 'app', }, })).toBe(true) @@ -46,7 +47,7 @@ describe('FileStateStore', () => { expect(await restarted.getDispatchLifecycle('workspace-1', key)).toMatchObject({ phase: 'published', lease: { owner: 'owner-b', epoch: 2 }, - pullRequest: { number: 85 }, + pullRequest: { number: 85, publisherIdentity: 'app' }, }) } finally { await rm(root, { recursive: true, force: true }) From 16442ee3b2741f88656cc3b2face719079bacb7d Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 10:19:19 -0400 Subject: [PATCH 28/30] fix: terminate unknown review identity --- src/orchestrator/factory.test.ts | 114 +++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 16 ++++- 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index ffd8b82..873fb4f 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10383,6 +10383,120 @@ describe('FactoryLoop', () => { }, ) + it('makes a legacy auto receipt terminal until the original publisher is configured explicitly', async () => { + const number = 537 + const path = issuePath(number) + const issue = issueFile(number) + const stateStore = new InMemoryStateStore({ batchSize: 2 }) + const seedFactory = createFactory(config(), { + mount: new FakeMountClient({ [path]: issue }), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + }) + const decision = await seedFactory.triageIssue(parseLinearIssue(path, issue)) + const receipt = { + repo: 'AgentWorkforce/pear', + number, + url: `https://github.com/AgentWorkforce/pear/pull/${number}`, + headRef: `factory/ar-${number}-pear`, + } + await stateStore.claimDispatchLifecycle( + 'factory-test', + issueKey(decision.issue), + { + runId: 'terminal-review-legacy-auto', + issue: { uuid: decision.issue.uuid, key: decision.issue.key, path: decision.issue.path }, + decision, + dryRun: false, + phase: 'complete', + agents: [], + invocationIds: [], + pullRequests: [receipt], + pullRequest: receipt, + updatedAtMs: 0, + }, + 'stopped-owner', + 0, + 1, + ) + + const appReviewRequests: Array<{ repo: string; number: number }> = [] + const userWriteback = new PublishingGithubWriteback({ number, author: 'operator-user' }) + const appWrite: GithubConnectionWrite = { + publishPullRequest: async () => { + throw new Error('restart recovery must reuse the durable receipt') + }, + requestPullRequestReview: async (input) => { + appReviewRequests.push(input) + }, + closePullRequest: async () => undefined, + } + const errors: unknown[][] = [] + const auto = createFactory(config({ github: { identity: 'auto' } }), { + mount: new FakeMountClient({ [path]: issue }, appWrite), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + githubWriteback: userWriteback, + reviewRequestRetryMs: 5, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: receipt.headRef, + isDraft: false, + state: 'OPEN', + }), + }), + logger: { + info: () => undefined, + warn: () => undefined, + error: (...args: unknown[]) => errors.push(args), + }, + }) + + try { + await auto.start({ mode: 'dispatch-owner' }) + await vi.waitFor(() => + expect(auto.status().counters.githubPullRequestReviewRequestIdentityRequired).toBe(1)) + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(auto.status().counters.githubPullRequestReviewRequestIdentityRequired).toBe(1) + expect(appReviewRequests).toEqual([]) + expect(userWriteback.reviewRequests).toEqual([]) + expect(errors).toContainEqual([ + '[factory] automated PR review request reached terminal publisher-identity refusal; verify the PR original publisher, set github.identity explicitly to "app" or "user", and restart', + expect.objectContaining({ repo: receipt.repo, prNumber: number }), + ]) + } finally { + await auto.stop() + } + + const explicitUser = createFactory(config({ github: { identity: 'user' } }), { + mount: new FakeMountClient({ [path]: issue }, appWrite), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + githubWriteback: userWriteback, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: receipt.headRef, + isDraft: false, + state: 'OPEN', + }), + }), + }) + try { + await explicitUser.start({ mode: 'dispatch-owner' }) + await vi.waitFor(() => + expect(userWriteback.reviewRequests).toEqual([{ repo: receipt.repo, number }])) + expect(appReviewRequests).toEqual([]) + } finally { + await explicitUser.stop() + await seedFactory.stop() + } + }) + it('contains a failed startup review reconciliation without failing the lifecycle owner', async () => { class ReconciliationFailingStateStore extends InMemoryStateStore { listCalls = 0 diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 0566a87..179df35 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -238,6 +238,7 @@ class ClarificationQuestionDeliveryLeaseLostError extends Error {} class GithubEscalationReconciliationUnavailableError extends Error {} class GithubEscalationPostAmbiguousError extends Error {} class AutomatedReviewRequestDrainError extends Error {} +class AutomatedReviewPublisherIdentityRequiredError extends Error {} type GithubEscalationReconciliation = 'found' | 'absent' | 'unavailable' class ClarificationWakeStoppedError extends Error {} @@ -6272,6 +6273,19 @@ export class FactoryLoop implements Factory { this.#scheduleAutomatedPullRequestReviewRetry(published) })) } catch (error) { + if (error instanceof AutomatedReviewPublisherIdentityRequiredError) { + this.#reviewRequestAttempts.delete(key) + this.#increment('githubPullRequestReviewRequestIdentityRequired') + this.#logger.error?.( + '[factory] automated PR review request reached terminal publisher-identity refusal; verify the PR original publisher, set github.identity explicitly to "app" or "user", and restart', + { + repo: published.repo, + prNumber: published.number, + error: describeError(error).errorMessage, + }, + ) + return + } this.#increment('githubPullRequestReviewRequestFailures') this.#logger.warn?.('[factory] automated PR review request failed; lifecycle completion remains independent', { repo: published.repo, @@ -6472,7 +6486,7 @@ export class FactoryLoop implements Factory { const configured = this.#config.github.identity const identity = published.publisherIdentity ?? (configured === 'auto' ? undefined : configured) if (!identity) { - throw new Error( + throw new AutomatedReviewPublisherIdentityRequiredError( 'Automated PR review request requires the persisted publisher identity when github.identity is "auto"; refusing to select an identity from current availability', ) } From dbc00f4ff0b4ef1ceb006c4ce118b7de6987c85a Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 10:25:02 -0400 Subject: [PATCH 29/30] fix: stamp adopted review identity --- src/orchestrator/factory.test.ts | 2 ++ src/orchestrator/factory.ts | 26 +++++++++++++++++++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 873fb4f..23df729 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -10218,6 +10218,7 @@ describe('FactoryLoop', () => { number, url: `https://github.com/AgentWorkforce/pear/pull/${number}`, headRef: `factory/ar-${number}-pear`, + publisherIdentity: 'app' as const, } await stateStore.claimDispatchLifecycle( 'factory-test', @@ -10542,6 +10543,7 @@ describe('FactoryLoop', () => { number, url: `https://github.com/AgentWorkforce/pear/pull/${number}`, headRef: `factory/ar-${number}-pear`, + publisherIdentity: 'app' as const, })) for (const receipt of receipts) { const issue = parseLinearIssue(issuePath(receipt.number), issueFile(receipt.number)) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 179df35..31c242a 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -1601,6 +1601,7 @@ export class FactoryLoop implements Factory { repo: pr.repo, number: pr.prNumber, headRef: pr.headRef, + publisherIdentity: this.#publisherIdentityForNewReviewObligation(), }) } if (record.decision.implementers.length > 1 && !await this.#allImplementersHaveCompletionPr(record)) { @@ -6193,16 +6194,17 @@ export class FactoryLoop implements Factory { if (opts.reconcileExisting && expectedHeadRef) { const existing = await this.#openPullRequestByHead(repo, expectedHeadRef) if (existing) { - this.#publishedPullRequests.set(key, existing) - this.#requestAutomatedPullRequestReviewForOpenReceipt(existing) + const adopted = { ...existing, publisherIdentity: identity } + this.#publishedPullRequests.set(key, adopted) + this.#requestAutomatedPullRequestReviewForOpenReceipt(adopted) this.#increment('githubPullRequestsReconciled') this.#logger.info?.('[factory] reconciled existing PR from implementer branch', { issue: issue.key, - repo: existing.repo, - prNumber: existing.number, - url: existing.url, + repo: adopted.repo, + prNumber: adopted.number, + url: adopted.url, }) - return existing + return adopted } } const baseRef = await this.#githubDefaultBranch(repo) @@ -6530,6 +6532,12 @@ export class FactoryLoop implements Factory { } } + #publisherIdentityForNewReviewObligation(): GithubPullRequestIdentity { + const configured = this.#config.github.identity + if (configured !== 'auto') return configured + return this.#mount.githubWrite ? 'app' : 'user' + } + #shouldAttemptPullRequestPublication(): boolean { if (this.#config.github.identity !== 'auto') return true if (this.#mount.githubWrite) return true @@ -7209,7 +7217,10 @@ export class FactoryLoop implements Factory { } const existing = await this.#openPullRequestByHead(repo, implementer.spec.branch) if (existing) { - this.#requestAutomatedPullRequestReviewForOpenReceipt(existing) + this.#requestAutomatedPullRequestReviewForOpenReceipt({ + ...existing, + publisherIdentity: this.#publisherIdentityForNewReviewObligation(), + }) return true } return false @@ -7227,6 +7238,7 @@ export class FactoryLoop implements Factory { repo: pr.repo, number: pr.prNumber, headRef: pr.headRef, + publisherIdentity: this.#publisherIdentityForNewReviewObligation(), }) } return true From 62684be10da10de6900f8bc37615a37e9cfe373c Mon Sep 17 00:00:00 2001 From: mobile Date: Thu, 30 Jul 2026 10:43:40 -0400 Subject: [PATCH 30/30] fix: recover historical PR publisher identity --- src/orchestrator/factory.test.ts | 122 ++++++++++++++++++++++++++++- src/orchestrator/factory.ts | 94 ++++++++++++++++++---- src/ports/state.ts | 8 ++ src/state/file-state-store.test.ts | 53 +++++++++++++ src/state/file-state-store.ts | 40 ++++++++++ src/state/in-memory-state-store.ts | 36 +++++++++ 6 files changed, 334 insertions(+), 19 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 23df729..d3a2760 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -3727,13 +3727,19 @@ describe('FactoryLoop', () => { triage: new StaticTriage(), githubWriteback, mergeGate, - probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 50, state: 'OPEN' }), + probePrResolver: async () => ({ + repo: 'AgentWorkforce/pear', + prNumber: 50, + author: 'operator-user', + state: 'OPEN', + }), probePrGhRunner: async () => ({ stdout: JSON.stringify({ number: 50, headRefName: 'github-head', isDraft: false, state: 'OPEN', + author: { login: 'operator-user' }, }), }), }) @@ -6154,6 +6160,7 @@ describe('FactoryLoop', () => { probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 1591, + author: 'operator-user', headRef: branch, state: 'OPEN', url: 'https://github.com/AgentWorkforce/pear/pull/1591', @@ -6164,6 +6171,7 @@ describe('FactoryLoop', () => { url: 'https://github.com/AgentWorkforce/pear/pull/1591', headRefName: branch, isDraft: false, + author: { login: 'operator-user' }, }]), }), }) @@ -6181,6 +6189,7 @@ describe('FactoryLoop', () => { repo: 'AgentWorkforce/pear', number: 1591, headRef: branch, + publisherIdentity: 'user', }, }) }) @@ -6208,6 +6217,7 @@ describe('FactoryLoop', () => { }) const fleet = new DurableRemoteLifecycleFleetClient() const stateStore = new InMemoryStateStore({ batchSize: 1 }) + const userWriteback = new PublishingGithubWriteback({ number: 1597, author: 'operator-user' }) const ghCalls: string[][] = [] let branch = '' const factory = createFactory(config({ babysitter: { enabled: true } }), { @@ -6224,15 +6234,18 @@ describe('FactoryLoop', () => { url: 'https://github.com/AgentWorkforce/pear/pull/1597', headRefName: branch, isDraft: false, + author: { login: 'operator-user' }, }]) : JSON.stringify({ number: 1597, headRefName: branch, isDraft: false, state: 'OPEN', + author: { login: 'operator-user' }, }), } }, + githubWriteback: userWriteback, }) const decision = await factory.triageIssue(parseLinearIssue(issuePath(597), issue)) @@ -6253,7 +6266,12 @@ describe('FactoryLoop', () => { const lookupCalls = ghCalls.filter((args) => args[0] === 'pr' && args[1] === 'list') expect(lookupCalls.length).toBeGreaterThan(0) expect(lookupCalls.every((args) => args.includes('--head') && args.includes(branch))).toBe(true) - expect(reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number: 1597 }]) + expect(reviewRequests).toEqual([]) + expect(userWriteback.reviewRequests).toEqual([{ repo: 'AgentWorkforce/pear', number: 1597 }]) + await expect(stateStore.getDispatchLifecycle('factory-test', issueKey(decision.issue))) + .resolves.toMatchObject({ + pullRequest: { publisherIdentity: 'user' }, + }) expect(publishPullRequest).not.toHaveBeenCalled() await factory.stop() }) @@ -10384,6 +10402,106 @@ describe('FactoryLoop', () => { }, ) + it.each([ + { + number: 538, + author: 'relayfile[bot]', + expectedIdentity: 'app' as const, + }, + { + number: 539, + author: 'operator-user', + expectedIdentity: 'user' as const, + }, + ])( + 'recovers and persists $expectedIdentity identity for a legacy receipt from authoritative author $author', + async ({ number, author, expectedIdentity }) => { + const path = issuePath(number) + const issue = issueFile(number) + const stateStore = new InMemoryStateStore({ batchSize: 2 }) + const seedFactory = createFactory(config(), { + mount: new FakeMountClient({ [path]: issue }), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + }) + const decision = await seedFactory.triageIssue(parseLinearIssue(path, issue)) + const receipt = { + repo: 'AgentWorkforce/pear', + number, + url: `https://github.com/AgentWorkforce/pear/pull/${number}`, + headRef: `factory/ar-${number}-pear`, + } + await stateStore.claimDispatchLifecycle( + 'factory-test', + issueKey(decision.issue), + { + runId: `terminal-review-legacy-${expectedIdentity}`, + issue: { uuid: decision.issue.uuid, key: decision.issue.key, path: decision.issue.path }, + decision, + dryRun: false, + phase: 'complete', + agents: [], + invocationIds: [], + pullRequests: [receipt], + pullRequest: receipt, + updatedAtMs: 0, + }, + 'stopped-owner', + 0, + 1, + ) + + const appReviewRequests: Array<{ repo: string; number: number }> = [] + const appWrite: GithubConnectionWrite = { + publishPullRequest: async () => { + throw new Error('restart recovery must reuse the durable receipt') + }, + requestPullRequestReview: async (input) => { + appReviewRequests.push(input) + }, + closePullRequest: async () => undefined, + } + const userWriteback = new PublishingGithubWriteback({ number, author: 'operator-user' }) + const restarted = createFactory(config({ github: { identity: 'auto' } }), { + mount: new FakeMountClient({ [path]: issue }, appWrite), + fleet: new RemoteLifecycleFleetClient(), + stateStore, + triage: new StaticTriage(), + githubWriteback: userWriteback, + probePrGhRunner: async () => ({ + stdout: JSON.stringify({ + number, + headRefName: receipt.headRef, + isDraft: false, + state: 'OPEN', + author: { login: author }, + }), + }), + }) + + try { + await restarted.start({ mode: 'dispatch-owner' }) + if (expectedIdentity === 'app') { + await vi.waitFor(() => expect(appReviewRequests).toEqual([{ repo: receipt.repo, number }])) + expect(userWriteback.reviewRequests).toEqual([]) + } else { + await vi.waitFor(() => + expect(userWriteback.reviewRequests).toEqual([{ repo: receipt.repo, number }])) + expect(appReviewRequests).toEqual([]) + } + await expect(stateStore.getDispatchLifecycle('factory-test', issueKey(decision.issue))) + .resolves.toMatchObject({ + pullRequest: { publisherIdentity: expectedIdentity }, + pullRequests: [expect.objectContaining({ publisherIdentity: expectedIdentity })], + }) + } finally { + await restarted.stop() + await seedFactory.stop() + } + }, + ) + it('makes a legacy auto receipt terminal until the original publisher is configured explicitly', async () => { const number = 537 const path = issuePath(number) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 31c242a..d1cd04a 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -123,6 +123,7 @@ type TerminationRoots = { pids: number[]; status: AgentPidResolution['status'] } type ResolvedIssuePr = { repo: string prNumber: number + author?: string draft?: boolean headRef?: string headRepo?: string @@ -137,6 +138,7 @@ type GithubPullRequestPublisher = Pick 1 && !await this.#allImplementersHaveCompletionPr(record)) { @@ -6194,7 +6196,10 @@ export class FactoryLoop implements Factory { if (opts.reconcileExisting && expectedHeadRef) { const existing = await this.#openPullRequestByHead(repo, expectedHeadRef) if (existing) { - const adopted = { ...existing, publisherIdentity: identity } + const adopted = { + ...existing, + publisherIdentity: this.#publisherIdentityForExistingPullRequest(existing.author), + } this.#publishedPullRequests.set(key, adopted) this.#requestAutomatedPullRequestReviewForOpenReceipt(adopted) this.#increment('githubPullRequestsReconciled') @@ -6321,6 +6326,7 @@ export class FactoryLoop implements Factory { #requestAutomatedPullRequestReviewForOpenReceipt( published: AutomatedReviewRequestTarget, + recoveryLifecycleKeys: string[] = [], ): Promise | undefined { const key = `${published.repo.toLowerCase()}#${published.number}` if ( @@ -6338,14 +6344,33 @@ export class FactoryLoop implements Factory { published.number, ) const work = openLookup - .then((open) => { + .then(async (open) => { if ( open?.number === published.number && (!published.headRef || open.headRef === published.headRef) ) { + const recoveredIdentity = published.publisherIdentity ?? + this.#publisherIdentityForExistingPullRequest(open.author) + if (recoveredIdentity && recoveryLifecycleKeys.length > 0) { + for (const lifecycleKey of recoveryLifecycleKeys) { + const persisted = await this.#state.recordDispatchLifecyclePullRequestPublisherIdentity( + this.#workspaceId, + lifecycleKey, + published.repo, + published.number, + recoveredIdentity, + this.#clock.now(), + ) + if (!persisted) { + throw new Error( + `Unable to persist recovered publisher identity for ${published.repo}#${published.number}`, + ) + } + } + } this.#requestAutomatedPullRequestReview({ ...open, - publisherIdentity: published.publisherIdentity, + publisherIdentity: recoveredIdentity, }) } else { this.#reviewRequestAttempts.delete(key) @@ -6376,11 +6401,20 @@ export class FactoryLoop implements Factory { }) return } - const receipts = new Map() - for (const [, lifecycle] of lifecycles) { + const receipts = new Map() + for (const [lifecycleKey, lifecycle] of lifecycles) { if (!isTerminalDispatchLifecycle(lifecycle)) continue for (const receipt of publishedPullRequests(lifecycle)) { - receipts.set(`${receipt.repo.toLowerCase()}#${receipt.number}`, receipt) + const key = `${receipt.repo.toLowerCase()}#${receipt.number}` + const existing = receipts.get(key) + if (existing) { + if (!existing.lifecycleKeys.includes(lifecycleKey)) existing.lifecycleKeys.push(lifecycleKey) + } else { + receipts.set(key, { receipt, lifecycleKeys: [lifecycleKey] }) + } } } // Reconciliation is intentionally detached from startup and serialized. @@ -6388,9 +6422,9 @@ export class FactoryLoop implements Factory { // view` subprocesses, and a review-request outage must not prevent the // lifecycle owner from starting. Receipts stay durable for the next pass. const reconciliation = (async () => { - for (const receipt of receipts.values()) { + for (const { receipt, lifecycleKeys } of receipts.values()) { if (this.#stopping) return - await this.#requestAutomatedPullRequestReviewForOpenReceipt(receipt) + await this.#requestAutomatedPullRequestReviewForOpenReceipt(receipt, lifecycleKeys) } })() this.#trackAutomatedPullRequestReviewWork(reconciliation) @@ -6532,10 +6566,12 @@ export class FactoryLoop implements Factory { } } - #publisherIdentityForNewReviewObligation(): GithubPullRequestIdentity { + #publisherIdentityForExistingPullRequest( + author: string | undefined, + ): GithubPullRequestIdentity | undefined { const configured = this.#config.github.identity if (configured !== 'auto') return configured - return this.#mount.githubWrite ? 'app' : 'user' + return githubPublisherIdentityFromAuthor(author) } #shouldAttemptPullRequestPublication(): boolean { @@ -6566,7 +6602,7 @@ export class FactoryLoop implements Factory { '--state', 'open', '--json', - 'number,url,headRefName,isDraft', + 'number,url,headRefName,isDraft,author', '--limit', '10', ]) @@ -6578,7 +6614,13 @@ export class FactoryLoop implements Factory { const url = stringValue(candidate?.url) const headRef = stringValue(candidate?.headRefName) if (!number || !url || headRef !== expectedHeadRef || candidate?.isDraft !== false) return [] - return [{ repo, number, url, headRef }] + return [{ + repo, + number, + url, + headRef, + author: githubAuthorLogin(candidate!), + }] }) return candidates.sort((a, b) => b.number - a.number)[0] } @@ -6626,6 +6668,7 @@ export class FactoryLoop implements Factory { number: snapshot.number, url: snapshot.url ?? `https://github.com/${repo}/pull/${snapshot.number}`, headRef: expectedHeadRef, + author: snapshot.author, }) } catch { // A partially materialized PR record cannot prove exact ownership. @@ -6651,7 +6694,7 @@ export class FactoryLoop implements Factory { '--repo', repo, '--json', - 'number,headRefName,isDraft,state', + 'number,headRefName,isDraft,state,author', ], { signal }, ) @@ -6667,6 +6710,7 @@ export class FactoryLoop implements Factory { repo, number, ...(headRef ? { headRef } : {}), + author: githubAuthorLogin(candidate!), } } @@ -7219,7 +7263,7 @@ export class FactoryLoop implements Factory { if (existing) { this.#requestAutomatedPullRequestReviewForOpenReceipt({ ...existing, - publisherIdentity: this.#publisherIdentityForNewReviewObligation(), + publisherIdentity: this.#publisherIdentityForExistingPullRequest(existing.author), }) return true } @@ -7238,7 +7282,7 @@ export class FactoryLoop implements Factory { repo: pr.repo, number: pr.prNumber, headRef: pr.headRef, - publisherIdentity: this.#publisherIdentityForNewReviewObligation(), + publisherIdentity: this.#publisherIdentityForExistingPullRequest(pr.author), }) } return true @@ -14716,6 +14760,7 @@ const resolveIssuePrFromMount = async ( candidates.push({ repo, prNumber: pr.number, + author: pr.author, draft: pr.draft, headRef: pr.headRef, headRepo: pr.headRepo, @@ -14763,7 +14808,7 @@ const resolveIssuePrFromGh = async ( '--state', 'all', '--json', - 'number,title,body,headRefName,headRepository,headRepositoryOwner,isCrossRepository,isDraft,state,url', + 'number,title,body,headRefName,headRepository,headRepositoryOwner,isCrossRepository,isDraft,state,url,author', '--limit', String(PROBE_PR_GH_CANDIDATE_LIMIT), ]) @@ -14802,6 +14847,7 @@ const resolveIssuePrFromGh = async ( candidates.push({ repo, prNumber: pr.number, + author: pr.author, draft: pr.draft, headRef: pr.headRef, headRepo: pr.headRepo, @@ -14927,6 +14973,7 @@ const readProbePrCandidate = async ( path: string, ): Promise<{ number: number + author?: string title: string body: string headRef: string @@ -14948,6 +14995,7 @@ const readProbePrCandidate = async ( if (!Number.isInteger(number) || number <= 0) return undefined return { number, + author: githubAuthorLogin(payload), title: stringValue(payload.title) ?? '', body: stringValue(payload.body) ?? '', headRef: refName(payload.headRef) ?? refName(payload.head) ?? stringValue(payload.head_ref) ?? '', @@ -14968,6 +15016,7 @@ const ghProbePrCandidate = ( value: unknown, ): { number: number + author?: string title: string body: string headRef: string @@ -14990,6 +15039,7 @@ const ghProbePrCandidate = ( })() return { number, + author: githubAuthorLogin(payload), title: stringValue(payload.title) ?? '', body: stringValue(payload.body) ?? '', headRef: stringValue(payload.headRefName) ?? '', @@ -15093,6 +15143,7 @@ const isGithubPullFilePath = (path: string): boolean => type PullSnapshot = { number: number + author?: string state?: string headRef?: string draft?: boolean @@ -15114,6 +15165,7 @@ const parsePullSnapshot = (content: unknown, fallbackNumber: number): PullSnapsh const number = fallbackNumber return { number, + author: githubAuthorLogin(payload), state: stringValue(payload.state), headRef: refName(payload.headRef) ?? refName(payload.head) ?? stringValue(payload.head_ref) ?? stringValue(payload.headRefName), draft: booleanValue(payload.isDraft) ?? booleanValue(payload.draft), @@ -15514,6 +15566,14 @@ const githubAuthorLogin = (payload: Record): string | undefined return undefined } +const githubPublisherIdentityFromAuthor = ( + author: string | undefined, +): GithubPullRequestIdentity | undefined => { + const normalized = author?.trim().toLowerCase() + if (!normalized) return undefined + return normalized.endsWith('[bot]') ? 'app' : 'user' +} + const liveHeartbeatIntervalMs = (staleMs: number): number => Math.min(DEFAULT_LIVE_HEARTBEAT_INTERVAL_MS, Math.max(500, Math.floor(staleMs / 4))) diff --git a/src/ports/state.ts b/src/ports/state.ts index 78dbf98..d377ba3 100644 --- a/src/ports/state.ts +++ b/src/ports/state.ts @@ -308,6 +308,14 @@ export interface StateStore { nowMs: number, lifecycle: DispatchLifecycle, ): Promise + recordDispatchLifecyclePullRequestPublisherIdentity( + workspaceId: string, + key: string, + repo: string, + number: number, + identity: 'app' | 'user', + nowMs: number, + ): Promise getDispatchLifecycle(workspaceId: string, key: string): Promise listDispatchLifecycles(workspaceId: string): Promise> clearQueuedDispatchLifecycle( diff --git a/src/state/file-state-store.test.ts b/src/state/file-state-store.test.ts index 5faaa3a..6c71f41 100644 --- a/src/state/file-state-store.test.ts +++ b/src/state/file-state-store.test.ts @@ -54,6 +54,59 @@ describe('FileStateStore', () => { } }) + it('atomically records a recovered publisher identity on a terminal pull-request receipt', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-lifecycle-publisher-identity-')) + try { + const watchStatePath = join(root, 'state.json') + const store = new FileStateStore({ batchSize: 2, watchStatePath }) + const key = 'AR-86:uuid-86:/linear/issues/AR-86.json' + const seed = dispatchLifecycle(86) + const claim = await store.claimDispatchLifecycle( + 'workspace-1', key, seed, 'owner-a', 1_000, 5_000, + ) + expect(claim.lease).toBeDefined() + expect(await store.saveDispatchLifecycle( + 'workspace-1', + key, + 'owner-a', + claim.lease!.epoch, + 1_001, + { + ...claim.lifecycle, + phase: 'complete', + pullRequest: { + repo: 'AgentWorkforce/factory', + number: 86, + url: 'https://github.com/AgentWorkforce/factory/pull/86', + headRef: 'factory/ar-86-agentworkforce-factory', + }, + pullRequests: [{ + repo: 'AgentWorkforce/factory', + number: 86, + url: 'https://github.com/AgentWorkforce/factory/pull/86', + headRef: 'factory/ar-86-agentworkforce-factory', + }], + }, + )).toBe(true) + + expect(await store.recordDispatchLifecyclePullRequestPublisherIdentity( + 'workspace-1', key, 'agentworkforce/FACTORY', 86, 'app', 1_002, + )).toBe(true) + expect(await store.recordDispatchLifecyclePullRequestPublisherIdentity( + 'workspace-1', key, 'AgentWorkforce/factory', 86, 'user', 1_003, + )).toBe(false) + + await expect(new FileStateStore({ batchSize: 2, watchStatePath }) + .getDispatchLifecycle('workspace-1', key)).resolves.toMatchObject({ + phase: 'complete', + pullRequest: { publisherIdentity: 'app' }, + pullRequests: [expect.objectContaining({ publisherIdentity: 'app' })], + }) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + it('atomically adopts an existing GitHub lifecycle across Relayfile issue aliases', async () => { const root = await mkdtemp(join(tmpdir(), 'factory-lifecycle-github-alias-')) try { diff --git a/src/state/file-state-store.ts b/src/state/file-state-store.ts index f770521..9ecf8c9 100644 --- a/src/state/file-state-store.ts +++ b/src/state/file-state-store.ts @@ -191,6 +191,26 @@ export class FileStateStore extends InMemoryStateStore { })) } + override async recordDispatchLifecyclePullRequestPublisherIdentity( + workspaceId: string, + key: string, + repo: string, + number: number, + identity: 'app' | 'user', + nowMs: number, + ): Promise { + return await this.#exclusive(async () => this.#withMutationLock(async () => { + const document = await this.#loadFromDisk() + const lifecycle = document.workspaces[workspaceId]?.dispatchLifecycles[key] + if (!lifecycle) return false + const updated = recordPullRequestPublisherIdentity(lifecycle, repo, number, identity) + if (!updated) return false + lifecycle.updatedAtMs = nowMs + await this.#persist(document) + return true + })) + } + override async getDispatchLifecycle(workspaceId: string, key: string): Promise { return await this.#exclusive(async () => { const lifecycle = (await this.#loadFromDisk()).workspaces[workspaceId]?.dispatchLifecycles[key] @@ -872,6 +892,26 @@ export class FileStateStore extends InMemoryStateStore { } } +const recordPullRequestPublisherIdentity = ( + lifecycle: DispatchLifecycle, + repo: string, + number: number, + identity: 'app' | 'user', +): boolean => { + const receipts = [ + ...(lifecycle.pullRequest ? [lifecycle.pullRequest] : []), + ...(lifecycle.pullRequests ?? []), + ].filter((receipt) => + githubRepositoriesMatch(receipt.repo, repo) && receipt.number === number) + if ( + receipts.length === 0 || + receipts.some((receipt) => + receipt.publisherIdentity !== undefined && receipt.publisherIdentity !== identity) + ) return false + for (const receipt of receipts) receipt.publisherIdentity = identity + return true +} + const parseDocument = (value: unknown): WatchStateDocument => { if (!isRecord(value) || !isRecord(value.workspaces)) { throw new Error('Factory GitHub watch state file is invalid') diff --git a/src/state/in-memory-state-store.ts b/src/state/in-memory-state-store.ts index 024cde2..5ae6a60 100644 --- a/src/state/in-memory-state-store.ts +++ b/src/state/in-memory-state-store.ts @@ -170,6 +170,22 @@ export class InMemoryStateStore implements StateStore { return true } + async recordDispatchLifecyclePullRequestPublisherIdentity( + workspaceId: string, + key: string, + repo: string, + number: number, + identity: 'app' | 'user', + nowMs: number, + ): Promise { + const lifecycle = this.#workspace(workspaceId).dispatchLifecycles.get(key) + if (!lifecycle) return false + const updated = recordPullRequestPublisherIdentity(lifecycle, repo, number, identity) + if (!updated) return false + lifecycle.updatedAtMs = nowMs + return true + } + async getDispatchLifecycle(workspaceId: string, key: string): Promise { const lifecycle = this.#workspace(workspaceId).dispatchLifecycles.get(key) return lifecycle ? cloneDispatchLifecycle(lifecycle) : undefined @@ -666,6 +682,26 @@ export class InMemoryStateStore implements StateStore { } } +const recordPullRequestPublisherIdentity = ( + lifecycle: DispatchLifecycle, + repo: string, + number: number, + identity: 'app' | 'user', +): boolean => { + const receipts = [ + ...(lifecycle.pullRequest ? [lifecycle.pullRequest] : []), + ...(lifecycle.pullRequests ?? []), + ].filter((receipt) => + githubRepositoriesMatch(receipt.repo, repo) && receipt.number === number) + if ( + receipts.length === 0 || + receipts.some((receipt) => + receipt.publisherIdentity !== undefined && receipt.publisherIdentity !== identity) + ) return false + for (const receipt of receipts) receipt.publisherIdentity = identity + return true +} + const cloneDispatchLifecycle = (lifecycle: DispatchLifecycle): DispatchLifecycle => structuredClone(lifecycle) const dispatchLifecycleLeaseMatches = (