diff --git a/.changeset/happy-herons-snooze.md b/.changeset/happy-herons-snooze.md new file mode 100644 index 000000000..d450d1b88 --- /dev/null +++ b/.changeset/happy-herons-snooze.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 560 +--- +execute-phase no longer accepts 'approved' at the human_needed checkpoint as a substitute for actual verification — the phase stays pending until /gsd:verify-work completes the UAT. diff --git a/agents/gsd-verifier.md b/agents/gsd-verifier.md index 96aca8b6e..48877f925 100644 --- a/agents/gsd-verifier.md +++ b/agents/gsd-verifier.md @@ -544,7 +544,7 @@ done ``` -Merge those harvested items into the same human verification list as your own analysis. Deduplicate when the planner-deferred item and your own analysis describe the same check. The downstream `human_needed` → HUMAN-UAT.md path in `workflows/execute-phase.md` is the single sink — no separate file is created. +Merge those harvested items into the same human verification list as your own analysis. Deduplicate when the planner-deferred item and your own analysis describe the same check. The downstream `human_needed` → `{phase_num}-UAT.md` path in `workflows/execute-phase.md` is the single sink — no separate file is created. **Format:** diff --git a/get-shit-done/references/checkpoints.md b/get-shit-done/references/checkpoints.md index 10b2cb90b..d63f06707 100644 --- a/get-shit-done/references/checkpoints.md +++ b/get-shit-done/references/checkpoints.md @@ -18,7 +18,7 @@ Plans execute autonomously. Checkpoints formalize interaction points where human **When:** Claude completed automated work, human confirms it works correctly. -> **Default mode (#3309): `workflow.human_verify_mode = end-of-phase`.** New projects do NOT halt mid-flight at `checkpoint:human-verify`. The planner suppresses those task emissions and embeds the verification details into the relevant `auto` task's `` block; the verifier harvests every `` at end-of-phase (Step 8) and consolidates them into the existing `human_needed` → HUMAN-UAT.md flow in `workflows/execute-phase.md`. The user reviews everything in one batch. +> **Default mode (#3309): `workflow.human_verify_mode = end-of-phase`.** New projects do NOT halt mid-flight at `checkpoint:human-verify`. The planner suppresses those task emissions and embeds the verification details into the relevant `auto` task's `` block; the verifier harvests every `` at end-of-phase (Step 8) and consolidates them into the existing `human_needed` → `{phase_num}-UAT.md` flow in `workflows/execute-phase.md`. The user reviews everything in one batch. > > **Why this is the default:** every mid-flight halt costs a full executor cold-start (CLAUDE.md, MEMORY.md, STATE.md, plan re-read on respawn) because subagent context is discarded across the pause. A plan with N human-verify checkpoints pays the cold-start cost N+1 times — measured at "tens of thousands of tokens" per round-trip on real projects. > diff --git a/get-shit-done/references/planner-human-verify-mode.md b/get-shit-done/references/planner-human-verify-mode.md index 466e0a7f9..c8ffb923f 100644 --- a/get-shit-done/references/planner-human-verify-mode.md +++ b/get-shit-done/references/planner-human-verify-mode.md @@ -27,7 +27,7 @@ Instead, fold each would-be verification step into the relevant `auto` task usin ``` -The verifier (Step 8) harvests every `` block at end-of-phase and consolidates them into the existing `human_needed` → HUMAN-UAT.md path in `workflows/execute-phase.md`. The user reviews everything in one batch instead of paying a cold-start cost per item. +The verifier (Step 8) harvests every `` block at end-of-phase and consolidates them into the existing `human_needed` → `{phase_num}-UAT.md` path in `workflows/execute-phase.md`. The user reviews everything in one batch instead of paying a cold-start cost per item. ### `mid-flight` (opt-back-in — pre-#3309 behavior) diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index 850e44136..213d8a574 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -1406,11 +1406,11 @@ grep "^status:" "$PHASE_DIR"/*-VERIFICATION.md | cut -d: -f2 | tr -d ' ' **Step A: Persist human verification items as UAT file.** -Create `{phase_dir}/{phase_num}-HUMAN-UAT.md` using UAT template format: +Create `{phase_dir}/{phase_num}-UAT.md` using UAT template format: ```markdown --- -status: partial +status: testing phase: {phase_num}-{phase_name} source: [{phase_num}-VERIFICATION.md] started: [now ISO] @@ -1419,7 +1419,11 @@ updated: [now ISO] ## Current Test -[awaiting human testing] +number: 1 +name: {first human_verification item description} +expected: | + {expected behavior from VERIFICATION.md} +awaiting: user response ## Tests @@ -1443,26 +1447,32 @@ blocked: 0 Commit the file: ```bash -gsd_run query commit "test({phase_num}): persist human verification items as UAT" --files "{phase_dir}/{phase_num}-HUMAN-UAT.md" +gsd_run query commit "test({phase_num}): persist human verification items as UAT" --files "{phase_dir}/{phase_num}-UAT.md" ``` **Step B: Present to user:** ``` -## ✓ Phase {X}: {Name} — Human Verification Required +## ◷ Phase {X}: {Name} — Human Verification Needed -All automated checks passed. {N} items need human testing: +All automated checks passed. {N} item(s) require human testing before this phase can be marked complete: {From VERIFICATION.md human_verification section} -Items saved to `{phase_num}-HUMAN-UAT.md` — they will appear in `/gsd:progress` and `/gsd:audit-uat`. +Tests saved to `{phase_num}-UAT.md`. -"approved" → continue | Report issues → gap closure +When ready to run the tests: + +`/gsd:verify-work {X} ${GSD_WS}` + +Verify-work will walk you through each item and mark the phase complete when all tests pass. ``` -**If user says "approved":** Proceed to `update_roadmap`. The HUMAN-UAT.md file persists with `status: partial` and will surface in future progress checks until the user runs `/gsd:verify-work` on it. +**Do NOT advance the phase from this branch.** Phase completion is handled by verify-work's auto-transition after UAT passes. -**If user reports issues:** Proceed to gap closure as currently implemented. +**If user acknowledges without reporting issues (including "ok", "noted", "ack", "got it", "approved", "done", "yes", "pass", or similar):** Stop. The phase remains pending. No further orchestrator action — wait for the user to run `/gsd:verify-work`. + +**If user reports issues now (before running verify-work):** Proceed to gap closure as currently implemented. **If gaps_found:** ``` diff --git a/tests/fix-3722-execute-phase-human-needed-checkpoint.test.cjs b/tests/fix-3722-execute-phase-human-needed-checkpoint.test.cjs new file mode 100644 index 000000000..ff0a8b340 --- /dev/null +++ b/tests/fix-3722-execute-phase-human-needed-checkpoint.test.cjs @@ -0,0 +1,128 @@ +// allow-test-rule: source-text-is-the-product +// execute-phase.md IS the runtime contract loaded by the orchestrator. +// Asserting that the "ack-and-advance" path is absent is the only way to verify +// the state machine lie (issue #38) cannot regress at runtime. +'use strict'; + +/** + * execute-phase.md human_needed branch — issue #38 / fix #3722 + * + * The old design offered '"approved" → continue' as a shortcut that advanced + * ROADMAP.md without completing human verification. This is a state machine lie: + * the phase appears complete in the project record while HUMAN-UAT.md items + * remain unresolved. + * + * The correct design: + * - human_needed branch creates a {phase_num}-UAT.md file (not {phase_num}-HUMAN-UAT.md) + * - directs the user to /gsd:verify-work to complete verification + * - does NOT call update_roadmap directly (phase completion goes through verify-work) + * - does NOT offer "approved" → continue as a bypass + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const EXECUTE_PHASE = path.join( + __dirname, + '..', + 'get-shit-done', + 'workflows', + 'execute-phase.md' +); + +describe('execute-phase.md human_needed branch — issue #38', () => { + let content; + + // Read once; all tests share the string. + test('workflow file is readable', () => { + content = fs.readFileSync(EXECUTE_PHASE, 'utf-8'); + assert.ok(content.length > 0, 'execute-phase.md must be non-empty'); + }); + + test('human_needed section exists', () => { + if (!content) content = fs.readFileSync(EXECUTE_PHASE, 'utf-8'); + assert.ok( + content.includes('human_needed'), + 'execute-phase.md must contain a human_needed branch' + ); + }); + + test('"approved" → continue bypass is absent from human_needed branch', () => { + if (!content) content = fs.readFileSync(EXECUTE_PHASE, 'utf-8'); + // The old prompt offered '"approved" → continue' as a shortcut that advanced + // ROADMAP.md without completing verification. That path must not exist. + assert.ok( + !content.includes('"approved" → continue'), + 'human_needed branch must not offer "approved" → continue: it marks the phase complete without verification (issue #38)' + ); + }); + + test('human_needed branch does NOT call update_roadmap directly', () => { + if (!content) content = fs.readFileSync(EXECUTE_PHASE, 'utf-8'); + // Locate the human_needed section and check that update_roadmap does not + // appear before the gaps_found section (i.e. it is not reachable from human_needed). + const humanNeededIdx = content.indexOf('**If human_needed:**'); + const gapsFoundIdx = content.indexOf('**If gaps_found:**'); + const updateRoadmapIdx = content.indexOf('update_roadmap', humanNeededIdx); + + assert.ok(humanNeededIdx !== -1, '**If human_needed:** section must exist'); + assert.ok(gapsFoundIdx !== -1, '**If gaps_found:** section must exist'); + assert.ok( + humanNeededIdx < gapsFoundIdx, + 'human_needed section must appear before gaps_found section' + ); + + // update_roadmap must not appear between human_needed and gaps_found sections + const updateRoadmapBetween = + updateRoadmapIdx !== -1 && + updateRoadmapIdx > humanNeededIdx && + updateRoadmapIdx < gapsFoundIdx; + + assert.ok( + !updateRoadmapBetween, + 'update_roadmap must not be reachable directly from the human_needed branch — phase completion must go through verify-work (issue #38)' + ); + }); + + test('human_needed branch directs user to /gsd:verify-work', () => { + if (!content) content = fs.readFileSync(EXECUTE_PHASE, 'utf-8'); + const humanNeededIdx = content.indexOf('**If human_needed:**'); + const gapsFoundIdx = content.indexOf('**If gaps_found:**'); + assert.ok(humanNeededIdx !== -1, '**If human_needed:** section must exist'); + + const humanNeededSection = content.slice( + humanNeededIdx, + gapsFoundIdx !== -1 ? gapsFoundIdx : undefined + ); + assert.ok( + humanNeededSection.includes('verify-work'), + 'human_needed branch must direct the user to /gsd:verify-work to complete verification' + ); + }); + + test('human_needed branch creates {phase_num}-UAT.md (not HUMAN-UAT.md)', () => { + if (!content) content = fs.readFileSync(EXECUTE_PHASE, 'utf-8'); + const humanNeededIdx = content.indexOf('**If human_needed:**'); + const gapsFoundIdx = content.indexOf('**If gaps_found:**'); + assert.ok(humanNeededIdx !== -1, '**If human_needed:** section must exist'); + + const humanNeededSection = content.slice( + humanNeededIdx, + gapsFoundIdx !== -1 ? gapsFoundIdx : undefined + ); + + // The file should be named {phase_num}-UAT.md so verify-work's glob picks it up + assert.ok( + humanNeededSection.includes('-UAT.md'), + 'human_needed branch must create a {phase_num}-UAT.md file for verify-work to resume' + ); + + // HUMAN-UAT.md causes a naming mismatch with verify-work's create_uat_file step + assert.ok( + !humanNeededSection.includes('HUMAN-UAT.md'), + 'human_needed branch must NOT create HUMAN-UAT.md — use {phase_num}-UAT.md to align with verify-work\'s resume path (issue #38 edge case 3)' + ); + }); +});