fix(3323): keep human-needed verification pending (#3339)

* fix: keep human-needed verification pending

* chore: add changeset for human verification gating

* docs: align ship verification status wording

.changeset/fix-3339-human-needed-verification-pending.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3339
+---
+**Human-needed verification no longer completes phases or passes ship preflight** — SDK phase execution now keeps `human_needed` and missing verification results pending instead of advancing to `phaseComplete`, and `check.ship-ready` only passes explicit `pass` / `passed` verification status. Closes #3323.

get-shit-done/workflows/execute-phase.md

@@ -1463,7 +1463,7 @@ grep "^status:" "$PHASE_DIR"/*-VERIFICATION.md | cut -d: -f2 | tr -d ' '
 | Status | Action |
 |--------|--------|
 | `passed` | → update_roadmap |
-| `human_needed` | Present items for human testing, get approval or feedback |
+| `human_needed` | Persist and present human testing items; keep phase pending until verification reruns as `passed` |
 | `gaps_found` | Present gap summary, offer `/gsd-plan-phase {phase} --gaps ${GSD_WS}` |
 
 **If human_needed:**

get-shit-done/workflows/ship.md

@@ -42,8 +42,8 @@ Verify the work is ready to ship:
    ```bash
    VERIFICATION=$(cat ${PHASE_DIR}/*-VERIFICATION.md 2>/dev/null)
    ```
-   Check for `status: passed` or `status: human_needed` (with human approval).
-   If no VERIFICATION.md or status is `gaps_found`: warn and ask user to confirm.
+   Check for `status: pass` or `status: passed`.
+   If no VERIFICATION.md or status is anything other than `pass` / `passed` (including `human_needed` / `gaps_found`): block with `PHASE_VERIFICATION_INCOMPLETE`; complete or formally re-run verification before shipping.
 
 2. **Clean working tree?**
    ```bash

sdk/src/phase-runner.test.ts

@@ -647,7 +647,7 @@ Use TypeScript.`, 'utf-8');
       expect(result.success).toBe(true);
     });
 
-    it('invokes onVerificationReview when verification returns human_needed', async () => {
+    it('keeps phase pending when verification review is accepted for human_needed', async () => {
       const onVerificationReview = vi.fn().mockResolvedValue('accept');
       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 });
@@ -671,7 +671,61 @@ Use TypeScript.`, 'utf-8');
       });
 
       expect(onVerificationReview).toHaveBeenCalled();
-      expect(result.success).toBe(true); // callback accepted
+      expect(result.success).toBe(false);
+      expect(deps.tools.phaseComplete).not.toHaveBeenCalled();
+      expect(result.steps.map(s => s.step)).not.toContain(PhaseStepType.Advance);
+
+      const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify);
+      expect(verifyStep?.success).toBe(false);
+      expect(verifyStep?.error).toBe('verification_human_needed');
+    });
+
+    it('routes VERIFICATION.md status human_needed through the human review gate', async () => {
+      const onVerificationReview = vi.fn().mockResolvedValue('accept');
+      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);
+      (deps.tools.exec as ReturnType<typeof vi.fn>).mockImplementation((cmd: string) => {
+        if (cmd === 'check.verification-status') return Promise.resolve({ status: 'human_needed' });
+        return Promise.resolve(undefined);
+      });
+
+      const runner = new PhaseRunner(deps);
+      const result = await runner.run('1', {
+        callbacks: { onVerificationReview },
+      });
+
+      expect(onVerificationReview).toHaveBeenCalled();
+      expect(result.success).toBe(false);
+      expect(deps.tools.phaseComplete).not.toHaveBeenCalled();
+      expect(result.steps.map(s => s.step)).not.toContain(PhaseStepType.Advance);
+
+      const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify);
+      expect(verifyStep?.success).toBe(false);
+      expect(verifyStep?.error).toBe('verification_human_needed');
+    });
+
+    it('does not advance when verification status is missing', 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);
+      (deps.tools.exec as ReturnType<typeof vi.fn>).mockImplementation((cmd: string) => {
+        if (cmd === 'check.verification-status') return Promise.resolve({ status: 'missing' });
+        return Promise.resolve(undefined);
+      });
+
+      const runner = new PhaseRunner(deps);
+      const result = await runner.run('1');
+
+      expect(result.success).toBe(false);
+      expect(deps.tools.phaseComplete).not.toHaveBeenCalled();
+      expect(result.steps.map(s => s.step)).not.toContain(PhaseStepType.Advance);
+
+      const verifyStep = result.steps.find(s => s.step === PhaseStepType.Verify);
+      expect(verifyStep?.success).toBe(false);
+      expect(verifyStep?.error).toBe('verification_gaps_found');
     });
 
     it('halts when verification review callback rejects', async () => {
@@ -1258,7 +1312,7 @@ Use TypeScript.`, 'utf-8');
       expect(stepTypes).toContain(PhaseStepType.Research);
     });
 
-    it('auto-accepts when verification callback throws', async () => {
+    it('keeps human verification pending when verification callback throws', 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 });
@@ -1281,9 +1335,11 @@ Use TypeScript.`, 'utf-8');
         },
       });
 
-      // Should auto-accept and proceed to advance
+      // Should acknowledge the callback failure but still avoid advancing.
       const stepTypes = result.steps.map(s => s.step);
-      expect(stepTypes).toContain(PhaseStepType.Advance);
+      expect(stepTypes).not.toContain(PhaseStepType.Advance);
+      expect(result.success).toBe(false);
+      expect(deps.tools.phaseComplete).not.toHaveBeenCalled();
     });
 
     it('auto-approves advance when advance callback throws', async () => {

sdk/src/phase-runner.ts

@@ -902,8 +902,7 @@ export class PhaseRunner {
         });
 
         if (decision === 'accept') {
-          outcome = 'passed';
-          break; // Treat as passed
+          break; // Acknowledged by caller, but still pending human verification.
         } else if (decision === 'retry' && gapRetryCount < maxGapRetries) {
           gapRetryCount++;
           continue;
@@ -1141,10 +1140,10 @@ export class PhaseRunner {
       const status = (data?.status ?? '').toLowerCase();
 
       if (status === 'pass' || status === 'passed') return 'passed';
+      if (status === 'human_needed') return 'human_needed';
       if (status === 'fail' || status === 'gaps_found') return 'gaps_found';
       if (status === 'missing') {
-        // VERIFICATION.md doesn't exist yet — treat session success as passed
-        return 'passed';
+        return 'gaps_found';
       }
       // Unknown status — log and treat as gaps_found to be safe
       this.logger?.warn(`Unknown verification status '${status}' for phase ${phaseNumber}, treating as gaps_found`);
@@ -1219,8 +1218,8 @@ export class PhaseRunner {
       this.logger?.warn(`Unexpected verification callback return value: ${String(decision)}, falling back to accept`);
       return 'accept';
     } catch (err) {
-      this.logger?.warn(`Verification callback threw, auto-accepting: ${err instanceof Error ? err.message : String(err)}`);
-      return 'accept'; // Auto-approve on error
+      this.logger?.warn(`Verification callback threw, keeping human verification pending: ${err instanceof Error ? err.message : String(err)}`);
+      return 'accept'; // Treat as acknowledged; caller remains pending.
     }
   }
 }

sdk/src/query/check-ship-ready.test.ts

@@ -74,4 +74,38 @@ describe('checkShipReady', () => {
     // Per spec: gh_authenticated is advisory — skip actual auth check to avoid slow network call
     expect(d.gh_authenticated).toBe(false);
   });
+
+  it('blocks shipping when VERIFICATION.md is missing', async () => {
+    await mkdir(join(projectDir, '.planning', 'phases', '01-foundation'), { recursive: true });
+
+    const { data } = await checkShipReady(['1'], projectDir);
+    const d = data as Record<string, unknown>;
+
+    expect(d.verification_passed).toBe(false);
+    expect(d.ready).toBe(false);
+    expect(d.blockers).toContain('verification status is not passed');
+  });
+
+  it('blocks shipping when verification status is human_needed', async () => {
+    const phaseDir = join(projectDir, '.planning', 'phases', '02-core');
+    await mkdir(phaseDir, { recursive: true });
+    await writeFile(
+      join(phaseDir, 'VERIFICATION.md'),
+      [
+        '---',
+        'status: human_needed',
+        '---',
+        '',
+        '# Verification',
+      ].join('\n'),
+      'utf-8',
+    );
+
+    const { data } = await checkShipReady(['2'], projectDir);
+    const d = data as Record<string, unknown>;
+
+    expect(d.verification_passed).toBe(false);
+    expect(d.ready).toBe(false);
+    expect(d.blockers).toContain('verification status is not passed');
+  });
 });

sdk/src/query/check-ship-ready.ts

@@ -73,13 +73,14 @@ export const checkShipReady: QueryHandler = async (args, projectDir) => {
   try {
     const verRes = await checkVerificationStatus([raw], projectDir);
     const vdata = verRes.data as Record<string, unknown>;
-    verification_passed = vdata.status !== 'fail';
+    const status = String(vdata.status ?? '').toLowerCase();
+    verification_passed = status === 'pass' || status === 'passed';
   } catch {
     verification_passed = false;
   }
 
   // Collect blockers
-  if (!verification_passed) blockers.push('verification status is fail or missing');
+  if (!verification_passed) blockers.push('verification status is not passed');
   if (!clean_tree) blockers.push('working tree is not clean (uncommitted changes)');
   if (!on_feature_branch) blockers.push('not on a feature branch (currently on main/master or unknown)');
   if (!remote_configured) blockers.push('no git remote configured');