diff --git a/sdk/src/phase-runner.test.ts b/sdk/src/phase-runner.test.ts index 876e09e52..a31bd7a92 100644 --- a/sdk/src/phase-runner.test.ts +++ b/sdk/src/phase-runner.test.ts @@ -577,9 +577,9 @@ describe('PhaseRunner', () => { // 1 initial + 1 retry = 2 calls (not 3) expect(verifyCallCount).toBe(2); - // Verify step still succeeds (gap closure exhausted → proceed) + // Verify step fails when gaps persist after exhausting retries const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); - expect(verifyStep!.success).toBe(true); + expect(verifyStep!.success).toBe(false); }); it('gaps_found triggers plan → execute → re-verify cycle', async () => { @@ -659,9 +659,9 @@ describe('PhaseRunner', () => { expect(afterVerify).not.toContain(PhaseStepType.Plan); expect(afterVerify.filter(s => s === PhaseStepType.Execute)).toHaveLength(0); - // Verify step still reports success (exhausted retries → proceed) + // Verify step fails when gaps persist (no retries allowed) const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); - expect(verifyStep!.success).toBe(true); + expect(verifyStep!.success).toBe(false); }); it('gap closure plan step failure proceeds to re-verify without executing', async () => { @@ -724,8 +724,9 @@ describe('PhaseRunner', () => { // 1 initial + 3 retries = 4 verify calls expect(verifyCallCount).toBe(4); + // Verify step fails when gaps persist after all retries exhausted const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify); - expect(verifyStep!.success).toBe(true); + expect(verifyStep!.success).toBe(false); }); it('gap closure results are included in the final verify step planResults', async () => { @@ -774,6 +775,69 @@ describe('PhaseRunner', () => { }); }); + // ─── Advance gate on persistent gaps ────────────────────────────────── + + describe('advance gate on persistent gaps', () => { + it('persistent gaps_found does NOT append Advance step', async () => { + const phaseOp = makePhaseOp({ has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + mockRunPhaseStepSession.mockImplementation(async (_prompt, step) => { + if (step === PhaseStepType.Verify) { + return makePlanResult({ + success: false, + error: { subtype: 'verification_failed', messages: ['Gaps persist'] }, + }); + } + return makePlanResult(); + }); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + const stepTypes = result.steps.map(s => s.step); + expect(stepTypes).not.toContain(PhaseStepType.Advance); + }); + + it('persistent gaps_found does NOT call phaseComplete', async () => { + const phaseOp = makePhaseOp({ has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + mockRunPhaseStepSession.mockImplementation(async (_prompt, step) => { + if (step === PhaseStepType.Verify) { + return makePlanResult({ + success: false, + error: { subtype: 'verification_failed', messages: ['Gaps persist'] }, + }); + } + return makePlanResult(); + }); + + const runner = new PhaseRunner(deps); + await runner.run('1'); + + expect(deps.tools.phaseComplete).not.toHaveBeenCalled(); + }); + + it('verifier disabled still advances normally', async () => { + const phaseOp = makePhaseOp({ has_context: true, has_plans: true, plan_count: 1 }); + const config = makeConfig({ workflow: { research: false, verifier: false, skip_discuss: true, plan_check: false } as any }); + const deps = makeDeps({ config }); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); + + const stepTypes = result.steps.map(s => s.step); + expect(stepTypes).toContain(PhaseStepType.Advance); + expect(result.success).toBe(true); + }); + }); + // ─── Phase lifecycle events ──────────────────────────────────────────── describe('phase lifecycle events', () => { diff --git a/sdk/src/phase-runner.ts b/sdk/src/phase-runner.ts index 361e4efd5..2e78139e8 100644 --- a/sdk/src/phase-runner.ts +++ b/sdk/src/phase-runner.ts @@ -239,6 +239,8 @@ export class PhaseRunner { if (!this.config.workflow.verifier) { this.logger?.debug('Skipping verify: config.workflow.verifier=false'); } else { + // Verify has its own internal retry logic (gap closure). retryOnce only + // retries on unexpected session throws, not on verification outcomes like gaps_found. const verifyResult = await this.retryOnce('verify', () => this.runVerifyStep(phaseNumber, sessionOpts, callbacks, options)); steps.push(verifyResult); @@ -300,6 +302,9 @@ export class PhaseRunner { const result = await fn(); if (result.success) return result; + // Don't retry verify outcomes (gaps_found, human_needed) — they have their own retry logic. + if (result.error?.startsWith('verification_')) return result; + this.logger?.warn(`Step "${label}" failed, retrying once...`); return fn(); } @@ -842,6 +847,7 @@ export class PhaseRunner { }); if (decision === 'accept') { + outcome = 'passed'; break; // Treat as passed } else if (decision === 'retry' && gapRetryCount < maxGapRetries) { gapRetryCount++; @@ -916,6 +922,7 @@ export class PhaseRunner { } const durationMs = Date.now() - stepStart; + const verifySuccess = outcome === 'passed'; this.eventStream.emitEvent({ type: GSDEventType.PhaseStepComplete, @@ -923,15 +930,17 @@ export class PhaseRunner { sessionId: lastResult?.sessionId ?? '', phaseNumber, step: PhaseStepType.Verify, - success: true, + success: verifySuccess, durationMs, + ...(!verifySuccess && { error: `verification_${outcome}` }), }); return { step: PhaseStepType.Verify, - success: true, + success: verifySuccess, durationMs, planResults: allPlanResults, + ...(!verifySuccess && { error: `verification_${outcome}` }), }; }