From 39d8688245a701f11958c6069a03fbe6cb10025d Mon Sep 17 00:00:00 2001 From: odmrs Date: Sat, 28 Mar 2026 11:50:41 -0300 Subject: [PATCH 1/2] fix(sdk): skip advance step when verification finds gaps Previously, the advance step ran unconditionally after verify, marking phases as complete in ROADMAP.md even when gaps_found. This caused subsequent auto runs to skip unfinished phases. Now checks if all verify steps passed before advancing. When verification fails, the phase remains incomplete so the next auto run re-attempts it. --- sdk/src/phase-runner.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sdk/src/phase-runner.ts b/sdk/src/phase-runner.ts index 37dc9051b..361e4efd5 100644 --- a/sdk/src/phase-runner.ts +++ b/sdk/src/phase-runner.ts @@ -250,9 +250,13 @@ export class PhaseRunner { } // ── Step 6: Advance ── - if (!halted) { + // Only advance if verify passed — never mark a phase complete when gaps were found. + const verifyPassed = steps.every(s => s.step !== PhaseStepType.Verify || s.success); + if (!halted && verifyPassed) { const advanceResult = await this.runAdvanceStep(phaseNumber, sessionOpts, callbacks); steps.push(advanceResult); + } else if (!halted && !verifyPassed) { + this.logger?.warn(`Skipping advance for phase ${phaseNumber}: verification found gaps`); } const totalDurationMs = Date.now() - startTime; From c5e4fea697db8814d6aef011bd8ca1d55950160d Mon Sep 17 00:00:00 2001 From: odmrs Date: Sat, 28 Mar 2026 14:13:49 -0300 Subject: [PATCH 2/2] fix(sdk): verify outcome gates advance correctly + regression tests Address review findings from #1454: 1. runVerifyStep now returns success:false when gaps persist after exhausting retries (was always returning success:true) 2. human_needed + callback accept correctly sets outcome to passed 3. retryOnce skips retry for verification outcomes (gaps_found, human_needed) which have their own internal retry logic 4. Updated 3 existing tests to expect success:false on exhausted gaps 5. Added 3 regression tests: - persistent gaps_found does NOT append Advance step - persistent gaps_found does NOT call phaseComplete - verifier disabled still advances normally --- sdk/src/phase-runner.test.ts | 74 +++++++++++++++++++++++++++++++++--- sdk/src/phase-runner.ts | 13 ++++++- 2 files changed, 80 insertions(+), 7 deletions(-) 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}` }), }; }