From f0a516b0c04187d741d5cf5e03cb2babe542f0e5 Mon Sep 17 00:00:00 2001 From: Miya Date: Tue, 25 Aug 2026 09:50:27 +0200 Subject: [PATCH] fix: verify recorded exit codes on every completion path Session-Id: 01a037bb-4e8c-7c20-8962-621515f5e335 --- .../src/__tests__/completion-pipeline.test.ts | 7 ++- .../src/__tests__/e2e-permissions.test.ts | 4 +- .../__tests__/fixtures/permission-test.yaml | 2 + .../core/src/__tests__/step-executor.test.ts | 41 +++++++++++++ .../core/src/__tests__/verification.test.ts | 22 ++++--- .../src/__tests__/workflow-runner.test.ts | 59 ++++++++++++++++++- packages/core/src/runner.ts | 16 +++-- packages/core/src/schema.ts | 2 +- packages/core/src/step-executor.ts | 24 ++++++-- packages/core/src/verification.ts | 16 +++-- 10 files changed, 166 insertions(+), 27 deletions(-) diff --git a/packages/core/src/__tests__/completion-pipeline.test.ts b/packages/core/src/__tests__/completion-pipeline.test.ts index 86df7fa..e2c103a 100644 --- a/packages/core/src/__tests__/completion-pipeline.test.ts +++ b/packages/core/src/__tests__/completion-pipeline.test.ts @@ -70,6 +70,7 @@ vi.mock('@relaycast/sdk', () => ({ let waitForExitFn: (ms?: number) => Promise<'exited' | 'timeout' | 'released'>; let waitForIdleFn: (ms?: number) => Promise<'idle' | 'timeout' | 'exited'>; let mockSpawnOutputs: string[] = []; +let mockSpawnExitCodes: Array = []; vi.mock('node:child_process', async () => { const actual = await vi.importActual('node:child_process'); @@ -106,7 +107,7 @@ function makeMockHandle(name: string) { return { name, runtime: 'pty' as const, - exitCode: undefined as number | undefined, + exitCode: mockSpawnExitCodes.shift(), exitSignal: undefined as string | undefined, waitForExit: (ms?: number) => waitForExitFn(ms).then((reason) => ({ reason })), waitForIdle: (ms?: number) => waitForIdleFn(ms).then((reason) => ({ reason })), @@ -348,6 +349,7 @@ describe('Completion Pipeline', () => { waitForExitFn = vi.fn().mockResolvedValue('exited'); waitForIdleFn = vi.fn().mockImplementation(() => never()); mockSpawnOutputs = []; + mockSpawnExitCodes = []; mockRelayInstance.spawnPty.mockImplementation(defaultSpawnPtyImplementation); eventListeners.clear(); db = makeDb(); @@ -1728,6 +1730,7 @@ describe('Completion Pipeline', () => { // Output has no STEP_COMPLETE, no OWNER_DECISION — just normal work output mockSpawnOutputs = ['Implemented the auth module. All tests pass.']; + mockSpawnExitCodes = [0]; const localDb = makeDb(); runner = new WorkflowRunner({ db: localDb, workspaceId: 'ws-test' }); @@ -1823,6 +1826,7 @@ describe('Completion Pipeline', () => { // Output contains positive conclusion words but no explicit marker mockSpawnOutputs = ['Feature implemented and verified. All artifacts are correct and complete.']; + mockSpawnExitCodes = [0]; const localDb = makeDb(); runner = new WorkflowRunner({ db: localDb, workspaceId: 'ws-test' }); @@ -1925,6 +1929,7 @@ describe('Completion Pipeline', () => { mockSpawnOutputs = [ 'Implemented the feature.\nOWNER_DECISION: INCOMPLETE_RETRY\nREASON: needs more tests\n', ]; + mockSpawnExitCodes = [0]; const localDb = makeDb(); runner = new WorkflowRunner({ db: localDb, workspaceId: 'ws-test' }); diff --git a/packages/core/src/__tests__/e2e-permissions.test.ts b/packages/core/src/__tests__/e2e-permissions.test.ts index 37ef793..dbb1dff 100644 --- a/packages/core/src/__tests__/e2e-permissions.test.ts +++ b/packages/core/src/__tests__/e2e-permissions.test.ts @@ -184,7 +184,9 @@ function makeMockHandle(name: string) { return { name, runtime: 'pty' as const, - exitCode: undefined as number | undefined, + // This fixture models agents that exit cleanly; exit_code verification now + // consumes the recorded code instead of treating process success as implicit. + exitCode: 0 as number | undefined, exitSignal: undefined as string | undefined, waitForExit: (ms?: number) => waitForExitFn(ms).then((reason) => ({ reason })), waitForIdle: (ms?: number) => waitForIdleFn(ms).then((reason) => ({ reason })), diff --git a/packages/core/src/__tests__/fixtures/permission-test.yaml b/packages/core/src/__tests__/fixtures/permission-test.yaml index a82ef62..6ddae0a 100644 --- a/packages/core/src/__tests__/fixtures/permission-test.yaml +++ b/packages/core/src/__tests__/fixtures/permission-test.yaml @@ -34,9 +34,11 @@ workflows: task: 'Verify you have read access. Check RELAY_AGENT_TOKEN is set.' verification: type: exit_code + value: '0' - name: write-step agent: writer dependsOn: [check-env] task: 'Verify you can write to src/tests/. Check RELAY_AGENT_TOKEN is set.' verification: type: exit_code + value: '0' diff --git a/packages/core/src/__tests__/step-executor.test.ts b/packages/core/src/__tests__/step-executor.test.ts index fe1d8d6..d7c2fa3 100644 --- a/packages/core/src/__tests__/step-executor.test.ts +++ b/packages/core/src/__tests__/step-executor.test.ts @@ -94,6 +94,47 @@ describe('StepExecutor — deterministic steps', () => { const result = await executor.executeOne(step, new Map()); expect(result.status).toBe('completed'); }); + + it('processSpawner path runs exit_code verification before accepting terminal success', async () => { + const executor = createExecutor({ + processSpawner: mockSpawner({ + spawnShell: vi.fn(async () => ({ output: 'claim already taken', exitCode: 78 })), + }), + }); + const step = makeStep({ + command: 'claim-work', + failOnError: false, + verification: { type: 'exit_code', value: '0' }, + ...({ terminalSuccessExitCodes: [78] } as Partial), + }); + + const result = await executor.executeOne(step, new Map()); + + expect(result.status).toBe('failed'); + expect(result.error).toContain('recorded exit code "78" did not match "0"'); + }); + + it('injected executeStep path runs exit_code verification before accepting terminal success', async () => { + const executor = createExecutor({ + executeStep: vi.fn(async () => ({ + status: 'completed', + output: 'claim already taken', + exitCode: 78, + completionReason: 'completed_early_exit' as any, + })), + }); + const step = makeStep({ + command: 'claim-work', + verification: { type: 'exit_code', value: '0' }, + ...({ terminalSuccessExitCodes: [78] } as Partial), + }); + + const result = await executor.executeOne(step, new Map()); + + expect(result.status).toBe('failed'); + expect(result.completionReason).toBe('failed_verification'); + expect(result.error).toContain('recorded exit code "78" did not match "0"'); + }); }); // ── 2. Non-interactive agent step ──────────────────────────────────────────── diff --git a/packages/core/src/__tests__/verification.test.ts b/packages/core/src/__tests__/verification.test.ts index 935451a..2267a15 100644 --- a/packages/core/src/__tests__/verification.test.ts +++ b/packages/core/src/__tests__/verification.test.ts @@ -44,18 +44,25 @@ describe('verification logic', () => { vi.clearAllMocks(); }); - // 1. exit_code — pass on exit 0 (implicit success) + // 1. exit_code — compare the recorded exit against the expected value describe('exit_code', () => { - it('should pass when agent exited successfully (exit 0 implicit)', () => { - const result = run({ type: 'exit_code', value: '0' }, 'some output'); + it('should pass when the recorded exit code matches', () => { + const result = run({ type: 'exit_code', value: '0' }, 'some output', 'test-step', { + exitCode: 0, + }); expect(result.passed).toBe(true); expect(result.completionReason).toBe('completed_verified'); }); - it('should still pass for non-zero value (exit_code is implicitly satisfied)', () => { - // per existing logic, exit_code case is a no-op — always passes if we reach it - const result = run({ type: 'exit_code', value: '1' }, 'output'); - expect(result.passed).toBe(true); + it('should fail when the recorded exit code does not match', () => { + expect(() => + run({ type: 'exit_code', value: '0' }, 'output', 'test-step', { exitCode: 78 }) + ).toThrow('recorded exit code "78" did not match "0"'); + + const missing = run({ type: 'exit_code', value: '0' }, 'output', 'test-step', { + allowFailure: true, + }); + expect(missing.passed).toBe(false); }); }); @@ -227,6 +234,7 @@ describe('verification logic', () => { it('should log legacy marker message when completionMarkerFound is false', () => { const result = run({ type: 'exit_code', value: '0' }, 'output', 'my-step', { completionMarkerFound: false, + exitCode: 0, }); expect(result.passed).toBe(true); expect(noopSideEffects.log).toHaveBeenCalledWith( diff --git a/packages/core/src/__tests__/workflow-runner.test.ts b/packages/core/src/__tests__/workflow-runner.test.ts index 0c336da..23d597e 100644 --- a/packages/core/src/__tests__/workflow-runner.test.ts +++ b/packages/core/src/__tests__/workflow-runner.test.ts @@ -68,6 +68,7 @@ vi.mock('@relaycast/sdk', () => ({ let waitForExitFn: (ms?: number) => Promise<'exited' | 'timeout' | 'released'>; let waitForIdleFn: (ms?: number) => Promise<'idle' | 'timeout' | 'exited'>; let mockSpawnOutputs: string[] = []; +let mockSpawnExitCodes: Array = []; const mockHarnessDriverSpawn = vi.fn(async () => mockRelayInstance); // Spawned-agent handle shaped like harness-driver's SpawnedAgentHandle, but @@ -76,7 +77,7 @@ function makeMockHandle(name: string) { return { name, runtime: 'pty' as const, - exitCode: undefined as number | undefined, + exitCode: mockSpawnExitCodes.shift(), exitSignal: undefined as string | undefined, waitForExit: (ms?: number) => waitForExitFn(ms).then((reason) => ({ reason })), waitForIdle: (ms?: number) => waitForIdleFn(ms).then((reason) => ({ reason })), @@ -317,6 +318,7 @@ describe('WorkflowRunner', () => { waitForExitFn = vi.fn().mockResolvedValue('exited'); waitForIdleFn = vi.fn().mockImplementation(() => never()); mockSpawnOutputs = []; + mockSpawnExitCodes = []; mockHarnessDriverSpawn.mockImplementation(async () => mockRelayInstance); mockRelayInstance.spawnPty.mockImplementation(defaultSpawnPtyImplementation); eventListeners.clear(); @@ -1005,6 +1007,48 @@ agents: expect(run.status, run.error).toBe('completed'); }); + it('WorkflowRunner deterministic executor path rejects terminal success when exit_code verification disagrees', async () => { + const executeDeterministicStep = vi.fn(async () => ({ + output: 'claim already taken', + exitCode: 78, + })); + runner = new WorkflowRunner({ + db, + workspaceId: 'ws-test', + executor: { + executeAgentStep: vi.fn(async () => ''), + executeDeterministicStep, + }, + }); + + const run = await runner.execute( + makeConfig({ + errorHandling: { strategy: 'fail-fast' }, + agents: [], + workflows: [ + { + name: 'default', + steps: [ + { + name: 'claim-gate', + type: 'deterministic', + command: 'claim-work', + failOnError: false, + verification: { type: 'exit_code', value: '0' }, + terminalSuccessExitCodes: [78], + } as any, + ], + }, + ], + }), + 'default' + ); + + expect(executeDeterministicStep).toHaveBeenCalledTimes(1); + expect(run.status).toBe('failed'); + expect(run.error).toContain('recorded exit code "78" did not match "0"'); + }); + it('repairs a failed deterministic gate with a workflow agent before retrying', async () => { const tmpDir = mkdtempSync(path.join(os.tmpdir(), 'relay-deterministic-repair-')); const stepDir = path.join(tmpDir, 'step-cwd'); @@ -1140,6 +1184,7 @@ agents: it('should apply verification fallback for self-owned interactive steps', async () => { mockSpawnOutputs = ['LEAD_DONE\n', 'REVIEW_DECISION: APPROVE\nREVIEW_REASON: verified\n']; + mockSpawnExitCodes = [0]; const run = await runner.execute( makeConfig({ @@ -1237,6 +1282,7 @@ agents: try { mockSpawnOutputs = ['LEAD_DONE\n']; + mockSpawnExitCodes = [0]; const run = await runner.execute( makeConfig({ @@ -1281,6 +1327,7 @@ agents: it('should pass canonical bypass args to interactive codex PTY spawns', async () => { mockSpawnOutputs = ['LEAD_DONE\n', 'REVIEW_DECISION: APPROVE\nREVIEW_REASON: verified\n']; + mockSpawnExitCodes = [0]; const run = await runner.execute( makeConfig({ @@ -1441,6 +1488,10 @@ agents: it('should not double release the worker when the owner fails after worker completion', async () => { const workerRelease = vi.fn().mockResolvedValue(undefined); const ownerRelease = vi.fn().mockResolvedValue(undefined); + let markWorkerReleased!: () => void; + const workerReleasedSignal = new Promise((resolve) => { + markWorkerReleased = resolve; + }); mockRelayInstance.spawnPty.mockImplementation( async ({ name, task }: { name: string; task?: string }) => { @@ -1461,7 +1512,10 @@ agents: // WorkflowAgentHandle destructures `reason`, so raw strings would // map to `undefined` and the timeout would go undetected. waitForExit: vi.fn().mockImplementation(async () => { - await Promise.resolve(); + // Model the test's stated ordering explicitly: the worker's + // completion promise settles before the owner timeout fires. + await workerReleasedSignal; + await new Promise((resolve) => setTimeout(resolve, 0)); return { reason: 'timeout' }; }), waitForIdle: vi.fn().mockResolvedValue({ reason: 'timeout' }), @@ -1476,6 +1530,7 @@ agents: exitSignal: undefined, waitForExit: vi.fn().mockImplementation(async () => { await workerRelease(); + markWorkerReleased(); return { reason: 'released' }; }), waitForIdle: vi.fn().mockImplementation(() => never()), diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 1f77d6f..fd1fe88 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -4669,7 +4669,9 @@ export class WorkflowRunner { { exitCode: executorResult.exitCode } ); const verificationResult = step.verification - ? this.runVerification(step.verification, output, step.name) + ? this.runVerification(step.verification, output, step.name, undefined, { + exitCode: executorResult.exitCode, + }) : undefined; return { output, @@ -4780,7 +4782,9 @@ export class WorkflowRunner { ); const verificationResult = step.verification - ? this.runVerification(step.verification, output, step.name) + ? this.runVerification(step.verification, output, step.name, undefined, { + exitCode: lastExitCode, + }) : undefined; lastCommandOutput = [commandStdout || output, commandStderr].filter(Boolean).join('\n'); @@ -5785,7 +5789,8 @@ export class WorkflowRunner { step.verification, specialistOutput, step.name, - promptTaskText + promptTaskText, + { exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode } ); completionReason = verificationResult.completionReason; } @@ -6652,6 +6657,7 @@ export class WorkflowRunner { ? this.runVerification(step.verification, specialistOutput, step.name, verificationTaskText, { allowFailure: true, completionMarkerFound: hasMarker, + exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode, }) : { passed: false }; @@ -6959,7 +6965,7 @@ export class WorkflowRunner { specialistOutput, step.name, verificationTaskText, - { allowFailure: true } + { allowFailure: true, exitCode: evidence?.process.exitCode } ); if (!verificationResult.passed) return null; } @@ -7983,7 +7989,7 @@ export class WorkflowRunner { ptyOutput, step.name, preparedTask.promptTaskText, - { allowFailure: true } + { allowFailure: true, exitCode: agent?.exitCode } ); if (verificationResult.passed) { this.log(`[${step.name}] Agent timed out but verification passed — treating as complete`); diff --git a/packages/core/src/schema.ts b/packages/core/src/schema.ts index 9a62e48..5972c7c 100644 --- a/packages/core/src/schema.ts +++ b/packages/core/src/schema.ts @@ -477,7 +477,7 @@ export interface VerificationCheck { /** * Type-specific value: * - output_contains: token that must appear in the step's output - * - exit_code: expected exit code (currently informational) + * - exit_code: expected recorded process exit code * - file_exists: path that must exist (relative to cwd or absolute) * - custom: shell command to execute, or `regex:` against output * - pr_url: optional `/` qualifier to require the discovered diff --git a/packages/core/src/step-executor.ts b/packages/core/src/step-executor.ts index da254fe..403fe21 100644 --- a/packages/core/src/step-executor.ts +++ b/packages/core/src/step-executor.ts @@ -407,15 +407,24 @@ export class StepExecutor { error: state.row.error ?? result?.error, }; } + const output = result?.output ?? ''; + const status = result?.status ?? 'completed'; + const verificationResult = + status === 'completed' && step.verification + ? this.runVerification(step.verification, output, step.name, undefined, { + allowFailure: true, + exitCode: result?.exitCode, + }) + : undefined; return this.completeStep(step, state, { - status: result?.status ?? 'completed', - output: result?.output ?? '', + status: verificationResult?.passed === false ? 'failed' : status, + output, exitCode: result?.exitCode, exitSignal: result?.exitSignal, - completionReason: result?.completionReason, + completionReason: verificationResult?.completionReason ?? result?.completionReason, retries: result?.retries ?? state.row.retryCount, duration: result?.duration ?? 0, - error: result?.error, + error: verificationResult?.error ?? result?.error, }); } @@ -539,12 +548,19 @@ export class StepExecutor { }; } + const verificationResult = step.verification + ? this.runVerification(step.verification, output, step.name, undefined, { + exitCode: spawnResult.exitCode, + }) + : undefined; + return { status: 'completed' as const, output, exitCode: spawnResult.exitCode, exitSignal: spawnResult.exitSignal, retries: attempt, + completionReason: verificationResult?.completionReason, }; }, }); diff --git a/packages/core/src/verification.ts b/packages/core/src/verification.ts index de9e16d..d20696f 100644 --- a/packages/core/src/verification.ts +++ b/packages/core/src/verification.ts @@ -48,6 +48,8 @@ export interface VerificationOptions { allowFailure?: boolean; completionMarkerFound?: boolean; cwd?: string; + /** Exit code recorded for the process being verified. */ + exitCode?: number; } export class WorkflowCompletionError extends Error { @@ -124,8 +126,11 @@ export function runVerification( } case 'exit_code': - if (!checkExitCode(check.value)) { - return fail(`Verification failed for "${stepName}": exit code did not match "${check.value}"`); + if (!checkExitCode(check.value, options.exitCode)) { + return fail( + `Verification failed for "${stepName}": recorded exit code ` + + `"${options.exitCode ?? 'unavailable'}" did not match "${check.value}"` + ); } break; @@ -223,10 +228,9 @@ export function stripInjectedTaskEcho(output: string, injectedTaskText?: string) return output; } -export function checkExitCode(_expectedExitCode: string): boolean { - // Existing runner semantics treat process success as established before this - // verification hook runs, so this check is currently an unconditional pass. - return true; +export function checkExitCode(expectedExitCode: string, actualExitCode?: number): boolean { + const expected = Number(expectedExitCode); + return Number.isInteger(expected) && actualExitCode !== undefined && actualExitCode === expected; } export function checkOutputContains(output: string, token: string, injectedTaskText?: string): boolean {