From dc003610e220d4a8c8d5e508a620ddb890f2589d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 9 May 2026 23:28:25 -0400 Subject: [PATCH] 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 --- ...-3339-human-needed-verification-pending.md | 5 ++ get-shit-done/workflows/execute-phase.md | 2 +- get-shit-done/workflows/ship.md | 4 +- sdk/src/phase-runner.test.ts | 66 +++++++++++++++++-- sdk/src/phase-runner.ts | 11 ++-- sdk/src/query/check-ship-ready.test.ts | 34 ++++++++++ sdk/src/query/check-ship-ready.ts | 5 +- 7 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 .changeset/fix-3339-human-needed-verification-pending.md diff --git a/.changeset/fix-3339-human-needed-verification-pending.md b/.changeset/fix-3339-human-needed-verification-pending.md new file mode 100644 index 000000000..7814a1ef8 --- /dev/null +++ b/.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. diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index f8202128a..4a1be0eac 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/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:** diff --git a/get-shit-done/workflows/ship.md b/get-shit-done/workflows/ship.md index 5cc276df6..d233a891d 100644 --- a/get-shit-done/workflows/ship.md +++ b/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 diff --git a/sdk/src/phase-runner.test.ts b/sdk/src/phase-runner.test.ts index baf2bc5af..19fe502f6 100644 --- a/sdk/src/phase-runner.test.ts +++ b/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).mockResolvedValue(phaseOp); + (deps.tools.exec as ReturnType).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).mockResolvedValue(phaseOp); + (deps.tools.exec as ReturnType).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 () => { diff --git a/sdk/src/phase-runner.ts b/sdk/src/phase-runner.ts index 7a9847107..748925fd5 100644 --- a/sdk/src/phase-runner.ts +++ b/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. } } } diff --git a/sdk/src/query/check-ship-ready.test.ts b/sdk/src/query/check-ship-ready.test.ts index eb4021a93..0a7ea9b6a 100644 --- a/sdk/src/query/check-ship-ready.test.ts +++ b/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; + + 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; + + expect(d.verification_passed).toBe(false); + expect(d.ready).toBe(false); + expect(d.blockers).toContain('verification status is not passed'); + }); }); diff --git a/sdk/src/query/check-ship-ready.ts b/sdk/src/query/check-ship-ready.ts index 047e18fb2..7711a792e 100644 --- a/sdk/src/query/check-ship-ready.ts +++ b/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; - 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');