Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/factory-sdk/src/fleet/internal-fleet-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,25 @@ describe('InternalFleetClient', () => {
})
})

it('resumes with the per-agent capability when provided', async () => {
const harness = new FakeHarnessDriverClient()
const fleet = new InternalFleetClient({ client: harness, cwd: '/worktree' })

await fleet.resume({
name: 'ar-1-review',
sessionRef: 'review-session',
node: 'self',
capability: 'spawn:claude',
})

expect(harness.spawned[0]).toMatchObject({
name: 'ar-1-review',
cli: 'claude',
cwd: '/worktree',
continueFrom: 'review-session',
})
})

it('rejects non-self placement for the internal single-node backend', async () => {
const fleet = new InternalFleetClient({ client: new FakeHarnessDriverClient() })

Expand Down
5 changes: 3 additions & 2 deletions packages/factory-sdk/src/fleet/internal-fleet-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,13 @@ export class InternalFleetClient implements FleetClient {
return { name: handle.name, sessionRef: sessionRefFrom(handle) }
}

async resume(input: { name?: string; sessionRef: string; node?: 'self' | string }): Promise<SpawnResult> {
async resume(input: { name?: string; sessionRef: string; node?: 'self' | string; capability?: Capability }): Promise<SpawnResult> {
assertSelfNode(input.node)

const handle = await this.#client.spawnPty({
name: input.name ?? input.sessionRef,
cli: capabilityCli[this.#resumeCapability],
// followups [fleet→W6]: W6 owns resume-vs-respawn and passes the per-agent capability.
cli: capabilityCli[input.capability ?? this.#resumeCapability],
cwd: this.#cwd,
continueFrom: input.sessionRef,
})
Expand Down
4 changes: 2 additions & 2 deletions packages/factory-sdk/src/fleet/relay-fleet-client.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { FleetClient, RosterEntry, SendInput, SpawnInput, SpawnResult } from '../ports'
import type { Capability, FleetClient, RosterEntry, SendInput, SpawnInput, SpawnResult } from '../ports'

const notImplemented = () => new Error('RelayFleetClient not implemented — see relay#1056')

Expand All @@ -8,7 +8,7 @@ export class RelayFleetClient implements FleetClient {
throw notImplemented()
}

async resume(_input: { name?: string; sessionRef: string; node?: 'self' | string }): Promise<SpawnResult> {
async resume(_input: { name?: string; sessionRef: string; node?: 'self' | string; capability?: Capability }): Promise<SpawnResult> {
throw notImplemented()
}

Expand Down
12 changes: 12 additions & 0 deletions packages/factory-sdk/src/github/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export {
GhCliGithubMergeGate,
GithubMergeGate,
evaluateGithubMergeGate,
} from './merge-gate'
export type {
GhRunner,
GhRunResult,
GithubMergeGateInput,
GithubMergeGateVerdict,
GithubMergeGate as GithubMergeGatePort,
} from './merge-gate'
121 changes: 121 additions & 0 deletions packages/factory-sdk/src/github/merge-gate.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { describe, expect, it } from 'vitest'

import { GhCliGithubMergeGate, evaluateGithubMergeGate, type GhRunner } from './merge-gate'

const input = {
repo: 'AgentWorkforce/pear',
number: 123,
expectedHeadSha: 'abc123',
}

const live = (overrides: Record<string, unknown> = {}) => ({
mergeable: 'MERGEABLE',
mergeStateStatus: 'CLEAN',
headRefOid: 'abc123',
statusCheckRollup: [
{ name: 'test', conclusion: 'SUCCESS' },
],
...overrides,
})

describe('GithubMergeGate', () => {
it('returns READY only for MERGEABLE+CLEAN, matching head, and no blocking checks', async () => {
const gate = new GhCliGithubMergeGate(async () => ({ stdout: JSON.stringify(live()) }))

await expect(gate.check(input)).resolves.toMatchObject({
verdict: 'READY',
ready: true,
})
})

it('returns READY for MERGEABLE+CLEAN with neutral, skipped, or expected advisory checks', () => {
expect(evaluateGithubMergeGate(input, live({
statusCheckRollup: [
{ name: 'required', conclusion: 'SUCCESS' },
{ name: 'advisory-neutral', conclusion: 'NEUTRAL' },
{ name: 'advisory-skipped', conclusion: 'SKIPPED' },
{ name: 'expected-but-nonblocking', conclusion: 'EXPECTED' },
],
}))).toMatchObject({
verdict: 'READY',
ready: true,
})
})

it('refuses when the live head differs from the expected head sha', () => {
expect(evaluateGithubMergeGate(input, live({ headRefOid: 'different-sha' }))).toMatchObject({
verdict: 'REFUSE',
ready: false,
reason: expect.stringMatching(/head moved/),
})
})

it('refuses stale mount-clean snapshots when live GitHub contradicts readiness', () => {
const staleMountSnapshot = {
mergeable: 'MERGEABLE',
mergeStateStatus: 'CLEAN',
headRefOid: 'abc123',
statusCheckRollup: [{ conclusion: 'SUCCESS' }],
}
void staleMountSnapshot

expect(evaluateGithubMergeGate(input, live({
mergeable: 'CONFLICTING',
mergeStateStatus: 'UNSTABLE',
headRefOid: 'def456',
statusCheckRollup: [{ conclusion: 'FAILURE' }],
}))).toMatchObject({
verdict: 'REFUSE',
ready: false,
})
})

it('fails closed when gh returns UNKNOWN, errors, or partial output', async () => {
const unknown = new GhCliGithubMergeGate(async () => ({
stdout: JSON.stringify(live({ mergeable: 'UNKNOWN', mergeStateStatus: 'UNKNOWN' })),
}))
await expect(unknown.check(input)).resolves.toMatchObject({
verdict: 'REFUSE',
ready: false,
})

const errorRunner: GhRunner = async () => {
throw new Error('gh timed out')
}
await expect(new GhCliGithubMergeGate(errorRunner).check(input)).resolves.toMatchObject({
verdict: 'REFUSE',
ready: false,
})

const partial = new GhCliGithubMergeGate(async () => ({
stdout: JSON.stringify({ mergeable: 'MERGEABLE', mergeStateStatus: 'CLEAN' }),
}))
await expect(partial.check(input)).resolves.toMatchObject({
verdict: 'REFUSE',
ready: false,
})
})

it('refuses missing, blocking, pending, or unknown status checks', () => {
expect(evaluateGithubMergeGate(input, live({ statusCheckRollup: [] }))).toMatchObject({
verdict: 'REFUSE',
ready: false,
})
expect(evaluateGithubMergeGate(input, live({ statusCheckRollup: [{ conclusion: 'FAILURE' }] }))).toMatchObject({
verdict: 'REFUSE',
ready: false,
})
expect(evaluateGithubMergeGate(input, live({ statusCheckRollup: [{ status: 'IN_PROGRESS' }] }))).toMatchObject({
verdict: 'REFUSE',
ready: false,
})
expect(evaluateGithubMergeGate(input, live({ statusCheckRollup: [{ conclusion: 'UNKNOWN' }] }))).toMatchObject({
verdict: 'REFUSE',
ready: false,
})
expect(evaluateGithubMergeGate(input, live({ statusCheckRollup: [{ status: 'COMPLETED' }] }))).toMatchObject({
verdict: 'REFUSE',
ready: false,
})
})
})
172 changes: 172 additions & 0 deletions packages/factory-sdk/src/github/merge-gate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
import { execFile } from 'node:child_process'
import { promisify } from 'node:util'

const execFileAsync = promisify(execFile)

export interface GhRunResult {
stdout: string
stderr?: string
}

export type GhRunner = (args: string[]) => Promise<GhRunResult>

export interface GithubMergeGateInput {
repo: string
number: number
expectedHeadSha: string
}

export interface GithubMergeGateVerdict {
verdict: 'READY' | 'REFUSE'
ready: boolean
reason: string
live: {
mergeable?: string
mergeStateStatus?: string
headRefOid?: string
checkStates: string[]
}
}

export interface GithubMergeGate {
check(input: GithubMergeGateInput): Promise<GithubMergeGateVerdict>
}

export class GhCliGithubMergeGate implements GithubMergeGate {
readonly #run: GhRunner

constructor(run: GhRunner = defaultGhRunner) {
this.#run = run
}

async check(input: GithubMergeGateInput): Promise<GithubMergeGateVerdict> {
try {
const result = await this.#run([
'pr',
'view',
String(input.number),
'--repo',
input.repo,
'--json',
'mergeable,mergeStateStatus,statusCheckRollup,headRefOid',
])
if (result.stdout.trim().length === 0) {
return refuse('gh returned empty output', { checkStates: [] })
}

return evaluateGithubMergeGate(input, parseGhJson(result.stdout))
} catch (error) {
return refuse(`gh merge gate failed: ${error instanceof Error ? error.message : String(error)}`, {
checkStates: [],
})
}
}
}

export const GithubMergeGate = GhCliGithubMergeGate

export function evaluateGithubMergeGate(
input: GithubMergeGateInput,
live: unknown,
): GithubMergeGateVerdict {
const record = asRecord(live)
const mergeable = stringValue(record.mergeable)
const mergeStateStatus = stringValue(record.mergeStateStatus)
const headRefOid = stringValue(record.headRefOid)
const statusCheckRollup = Array.isArray(record.statusCheckRollup) ? record.statusCheckRollup : undefined
const checkStates = statusCheckRollup ? checkStatesFromRollup(statusCheckRollup) : []

if (!mergeable || !mergeStateStatus || !headRefOid || !statusCheckRollup) {
return refuse('missing required live GitHub merge fields', {
mergeable,
mergeStateStatus,
headRefOid,
checkStates,
})
}

if (mergeable === 'UNKNOWN' || mergeStateStatus === 'UNKNOWN') {
return refuse('GitHub mergeability is still unknown', { mergeable, mergeStateStatus, headRefOid, checkStates })
}

if (headRefOid !== input.expectedHeadSha) {
return refuse(`head moved: expected ${input.expectedHeadSha}, live ${headRefOid ?? 'unknown'}`, {
mergeable,
mergeStateStatus,
headRefOid,
checkStates,
})
}

if (mergeable !== 'MERGEABLE') {
return refuse(`mergeable is ${mergeable ?? 'unknown'}`, { mergeable, mergeStateStatus, headRefOid, checkStates })
}

if (mergeStateStatus !== 'CLEAN') {
return refuse(`merge state is ${mergeStateStatus ?? 'unknown'}`, { mergeable, mergeStateStatus, headRefOid, checkStates })
}

if (checkStates.length === 0) {
return refuse('no successful status checks observed', { mergeable, mergeStateStatus, headRefOid, checkStates })
}

const blocking = checkStates.filter(isBlockingCheckState)
if (blocking.length > 0) {
return refuse(`checks not merge-ready: ${blocking.join(', ')}`, { mergeable, mergeStateStatus, headRefOid, checkStates })
}

return {
verdict: 'READY',
ready: true,
reason: 'MERGEABLE+CLEAN with matching head and no blocking checks',
live: { mergeable, mergeStateStatus, headRefOid, checkStates },
}
}

const defaultGhRunner: GhRunner = async (args) => {
const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 })
return { stdout, stderr }
Comment on lines +126 to +128

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a timeout to the gh subprocess call.

Line 127 runs an external CLI without a timeout, so a stuck gh process can block merge-gate evaluation indefinitely and stall the loop.

Suggested fix
 const defaultGhRunner: GhRunner = async (args) => {
-  const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 })
+  const { stdout, stderr } = await execFileAsync('gh', args, {
+    maxBuffer: 1024 * 1024,
+    timeout: 30_000,
+  })
   return { stdout, stderr }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const defaultGhRunner: GhRunner = async (args) => {
const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 })
return { stdout, stderr }
const defaultGhRunner: GhRunner = async (args) => {
const { stdout, stderr } = await execFileAsync('gh', args, {
maxBuffer: 1024 * 1024,
timeout: 30_000,
})
return { stdout, stderr }
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/factory-sdk/src/github/merge-gate.ts` around lines 126 - 128, The gh
subprocess call in defaultGhRunner lacks a timeout and can hang; update the
execFileAsync invocation inside defaultGhRunner to include a timeout option
(e.g. { timeout: 30_000, maxBuffer: 1024 * 1024 }) or wire in a configurable
constant (e.g. DEFAULT_GH_TIMEOUT) and pass it as { timeout: DEFAULT_GH_TIMEOUT,
maxBuffer: ... } so the child process is killed after the timeout; keep the
returned shape ({ stdout, stderr }) unchanged and surface the timeout error
as-is.

}

const parseGhJson = (stdout: string): unknown => JSON.parse(stdout)

const refuse = (reason: string, live: GithubMergeGateVerdict['live']): GithubMergeGateVerdict => ({
verdict: 'REFUSE',
ready: false,
reason,
live,
})

const checkStatesFromRollup = (value: unknown): string[] => {
if (!Array.isArray(value)) {
return []
}

return value.map((entry) => {
const record = asRecord(entry)
const conclusion = stringValue(record.conclusion)
if (conclusion) {
return conclusion
}

const state = stringValue(record.state)
if (state) {
return state
}

const status = stringValue(record.status)
return status ?? 'UNKNOWN'
})
}

const nonBlockingCheckStates = new Set(['SUCCESS', 'NEUTRAL', 'SKIPPED', 'EXPECTED'])

const isBlockingCheckState = (state: string): boolean => !nonBlockingCheckStates.has(state)

const asRecord = (value: unknown): Record<string, unknown> =>
value !== null && typeof value === 'object' && !Array.isArray(value)
? value as Record<string, unknown>
: {}

const stringValue = (value: unknown): string | undefined =>
typeof value === 'string' ? value : undefined
Loading
Loading