diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 067f67d..d0dc1ce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,10 @@ jobs: steps: - name: Checkout uses: actions/checkout@v6 + with: + # PR checkouts otherwise use GitHub's synthetic merge commit, while + # the attestation is required to bind the exact reviewed head SHA. + ref: ${{ github.event.pull_request.head.sha || github.sha }} - name: Setup Node uses: actions/setup-node@v6 diff --git a/scripts/verify-packed-e2e.mjs b/scripts/verify-packed-e2e.mjs index a82cec7..051d39e 100644 --- a/scripts/verify-packed-e2e.mjs +++ b/scripts/verify-packed-e2e.mjs @@ -21,6 +21,11 @@ const checks = [] try { assert.match(headSha, /^[0-9a-f]{40}$/u, 'head SHA must be a full Git object ID') assert.match(testedCheckoutSha, /^[0-9a-f]{40}$/u, 'tested checkout SHA must be a full Git object ID') + assert.equal( + headSha, + testedCheckoutSha, + 'FACTORY_E2E_HEAD_SHA must match the checkout being packed and tested', + ) checks.push('head-sha-bound') mkdirSync(packDir, { recursive: true }) diff --git a/src/fleet/internal-fleet-client.test.ts b/src/fleet/internal-fleet-client.test.ts index 82124e6..2e4a026 100644 --- a/src/fleet/internal-fleet-client.test.ts +++ b/src/fleet/internal-fleet-client.test.ts @@ -38,6 +38,7 @@ class FakeHarnessDriverClient implements HarnessDriverClientLike { if (this.throwOnRelease) { throw new Error('release failed') } + this.agents = this.agents.filter((agent) => agent.name !== name) return { name } } @@ -532,6 +533,7 @@ describe('InternalFleetClient', () => { data: { error: 'internal reply dropped', success: false }, }) } + this.agents = this.agents.filter((agent) => agent.name !== name) return { name } } } @@ -905,11 +907,47 @@ describe('InternalFleetClient', () => { expect(harness.released).toEqual([{ name: 'ar-1-impl', reason: 'done' }]) await expect(fleet.roster()).resolves.toEqual({ - agents: [{ name: 'ar-1-impl' }], + agents: [], nodes: [{ name: 'self', capabilities: ['spawn:claude', 'spawn:codex', 'workflow:run'], live: true }], }) }) + it('retries a successful release acknowledgement until the broker name disappears', async () => { + vi.useFakeTimers() + try { + class RetainedFirstReleaseHarnessDriverClient extends FakeHarnessDriverClient { + attempts = 0 + + override async release(name: string, reason?: string): Promise<{ name: string }> { + this.attempts += 1 + this.released.push({ name, reason }) + if (this.attempts > 1) { + this.agents = this.agents.filter((agent) => agent.name !== name) + } + return { name } + } + } + + const harness = new RetainedFirstReleaseHarnessDriverClient() + harness.agents = [{ name: 'ar-stale-review', cli: 'codex' }] + const logger = { warn: vi.fn() } + const fleet = new InternalFleetClient({ client: harness, logger }) + + const released = fleet.release('ar-stale-review', 'reconciled-missing') + await vi.advanceTimersByTimeAsync(250) + await released + + expect(harness.attempts).toBe(2) + expect(harness.agents).toEqual([]) + expect(logger.warn).toHaveBeenCalledWith( + '[factory-sdk] released broker name is still present; retrying release', + expect.objectContaining({ name: 'ar-stale-review', attempt: 1, maxAttempts: 3 }), + ) + } finally { + vi.useRealTimers() + } + }) + it('re-adopts tracked workers and synthesizes exits for workers missing after restart', async () => { const harness = new FakeHarnessDriverClient() harness.agents = [{ name: 'ar-1-impl' }] diff --git a/src/fleet/internal-fleet-client.ts b/src/fleet/internal-fleet-client.ts index 0c46979..f001bdb 100644 --- a/src/fleet/internal-fleet-client.ts +++ b/src/fleet/internal-fleet-client.ts @@ -283,7 +283,6 @@ export class InternalFleetClient implements FleetClient { for (let attempt = 1; attempt <= RELEASE_RETRY_MAX_ATTEMPTS; attempt += 1) { try { await this.#client.release(name, reason) - return } catch (error) { if (attempt >= RELEASE_RETRY_MAX_ATTEMPTS || !isRetryableReleaseError(error)) { throw error @@ -295,7 +294,47 @@ export class InternalFleetClient implements FleetClient { error, }) await sleep(RELEASE_RETRY_BACKOFF_MS * attempt) + continue } + + // A successful HTTP acknowledgement is not sufficient proof that the + // broker name is reusable. Under concurrent startup reconciliation the + // worker can disappear while a stale inventory row remains, making an + // immediate same-name resume fail with `agent already exists`. Verify the + // raw Harness Driver inventory and repeat the public release until the + // broker itself no longer advertises the name. + let retained: boolean + try { + const agents = await this.#client.listAgents() + if (!Array.isArray(agents)) { + throw new TypeError('Expected Harness Driver listAgents() to return an array') + } + retained = agents.some((agent) => agent?.name === name) + } catch (error) { + if (attempt >= RELEASE_RETRY_MAX_ATTEMPTS) throw error + this.#logger?.warn?.('[factory-sdk] unable to verify released broker name; retrying release', { + name, + attempt, + maxAttempts: RELEASE_RETRY_MAX_ATTEMPTS, + error, + }) + await sleep(RELEASE_RETRY_BACKOFF_MS * attempt) + continue + } + if (!retained) return + + const retainedError = Object.assign( + new Error(`Broker still reports agent ${name} after a successful release acknowledgement`), + { code: 'release_not_observed', retryable: true }, + ) + if (attempt >= RELEASE_RETRY_MAX_ATTEMPTS) throw retainedError + this.#logger?.warn?.('[factory-sdk] released broker name is still present; retrying release', { + name, + attempt, + maxAttempts: RELEASE_RETRY_MAX_ATTEMPTS, + error: retainedError, + }) + await sleep(RELEASE_RETRY_BACKOFF_MS * attempt) } } diff --git a/src/packed-e2e-attestation.test.ts b/src/packed-e2e-attestation.test.ts new file mode 100644 index 0000000..75e6738 --- /dev/null +++ b/src/packed-e2e-attestation.test.ts @@ -0,0 +1,23 @@ +import { spawnSync } from 'node:child_process' +import { resolve } from 'node:path' + +import { describe, expect, it } from 'vitest' + +describe('packed E2E attestation', () => { + it('rejects an attestation head that differs from the tested checkout', () => { + const root = resolve(import.meta.dirname, '..') + const result = spawnSync(process.execPath, [resolve(root, 'scripts/verify-packed-e2e.mjs')], { + cwd: root, + encoding: 'utf8', + env: { + ...process.env, + FACTORY_E2E_HEAD_SHA: '0000000000000000000000000000000000000000', + }, + }) + + expect(result.status).not.toBe(0) + expect(result.stderr).toContain( + 'FACTORY_E2E_HEAD_SHA must match the checkout being packed and tested', + ) + }) +})