Merge pull request #1454 from odmrs/fix/skip-advance-on-gaps-found

fix(sdk): skip advance step when verification finds gaps

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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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', () => {

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);
 
@@ -250,9 +252,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;
@@ -296,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();
   }
@@ -838,6 +847,7 @@ export class PhaseRunner {
         });
 
         if (decision === 'accept') {
+          outcome = 'passed';
           break; // Treat as passed
         } else if (decision === 'retry' && gapRetryCount < maxGapRetries) {
           gapRetryCount++;
@@ -912,6 +922,7 @@ export class PhaseRunner {
     }
 
     const durationMs = Date.now() - stepStart;
+    const verifySuccess = outcome === 'passed';
 
     this.eventStream.emitEvent({
       type: GSDEventType.PhaseStepComplete,
@@ -919,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}` }),
     };
   }