From 0d97532a5790bd1fb3064d9eb0af450b6c573141 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 3 Jun 2026 20:51:53 -0400 Subject: [PATCH] fix(#586): make ship PHASE_VERIFICATION_INCOMPLETE actionable, drop dead `pass` status branch (#650) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#586): make ship PHASE_VERIFICATION_INCOMPLETE actionable, drop dead `pass` arm The ship preflight gate blocked with PHASE_VERIFICATION_INCOMPLETE but named no next step, and accepted a `pass` status the verifier never emits. Capture the verification status and route per value (gaps_found / human_needed / missing), mirroring execute-phase's status table; accept only `passed`. Closes #586 Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#586): backfill changeset PR number 650 Co-Authored-By: Claude Opus 4.8 (1M context) * fix(#586): scope ship verification status to frontmatter only Codex adversarial review of PR #650 flagged that the status gate grepped `^status:` over the entire VERIFICATION.md, so a `status:` line in the report body (a code block / copied artifact) concatenates into a non-matching value and blocks a genuinely-passed phase with the wrong next action. Restrict extraction to the leading YAML frontmatter block, first match only. Adds a behavioral regression test that runs the gate's own bash pipeline against a passing report whose body contains decoy `status:` lines. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#586): drop manual PR ref from changeset body The changelog renderer auto-appends `(#)` from the fragment's pr: field (scripts/changeset/serialize.cjs, github-release-notes.cjs). The manual trailing `(#586)` produced a double, mismatched ref (issue #586 + auto PR #650); remove it to match the sibling-fragment convention. Co-Authored-By: Claude Opus 4.8 (1M context) * test(#586): make ship-586 bash-fence regex Windows-safe (CRLF) The behavioral test extracted the gate's bash block with /```bash\n.../ — a literal \n that fails to match Windows CRLF checkouts and trips the windows-test-parity-guard (fenceRegexLiteralNewline). Use ```bash\r?\n and normalize the captured block to LF before running it. Full unit suite: 0 fail. Co-Authored-By: Claude Opus 4.8 (1M context) * test(#586): run ship-586 bash-pipeline tests on POSIX only On Windows CI the behavioral tests failed: git-bash is present (so the old hasBash guard ran them) but receives a Windows-style tmpdir path it cannot glob, so extraction returned empty. The extraction logic is platform-independent and the gate's bash only runs in a POSIX workflow context, so skip the pipeline execution on win32. POSIX (macOS/Linux) still runs and asserts it. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .changeset/ship-verification-actionable.md | 5 + gsd-core/workflows/ship.md | 11 +- tests/ship-586-verification-routing.test.cjs | 112 +++++++++++++++++++ 3 files changed, 125 insertions(+), 3 deletions(-) create mode 100644 .changeset/ship-verification-actionable.md create mode 100644 tests/ship-586-verification-routing.test.cjs diff --git a/.changeset/ship-verification-actionable.md b/.changeset/ship-verification-actionable.md new file mode 100644 index 000000000..a66fb68f0 --- /dev/null +++ b/.changeset/ship-verification-actionable.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 650 +--- +**ship now tells you how to clear a blocked verification** — the `PHASE_VERIFICATION_INCOMPLETE` block names the exact next action per status (`gaps_found`, `human_needed`, or missing), and the dead `pass` status arm is gone. diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index 19d29565d..f91aeef26 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -41,10 +41,15 @@ Verify the work is ready to ship: 1. **Verification passed?** ```bash - VERIFICATION=$(cat ${PHASE_DIR}/*-VERIFICATION.md 2>/dev/null) + VERIFICATION_FILE=$(ls ${PHASE_DIR}/*-VERIFICATION.md 2>/dev/null | head -1) + STATUS=$(sed -n '/^---$/,/^---$/p' "${VERIFICATION_FILE}" 2>/dev/null | grep -m1 "^status:" | cut -d: -f2 | tr -d ' ') ``` - 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. + The verifier emits exactly `passed`, `gaps_found`, or `human_needed` (see the status table in `execute-phase.md`); only `passed` may ship. Route on `${STATUS}` — on any non-`passed` value, block with `PHASE_VERIFICATION_INCOMPLETE` and state the matching next action: + - `passed` → verification complete; continue to the next preflight check. + - `gaps_found` → run `/gsd:plan-phase ${PHASE_NUMBER} --gaps` to plan the fixes, then re-run `/gsd:execute-phase` before shipping. + - `human_needed` → complete the manual tests in `${PHASE_DIR}/*-UAT.md`, then re-run the verify step until status is `passed`. + - empty (no `*-VERIFICATION.md`) → the verify step never completed; re-run `/gsd:execute-phase`. + - any other value → unexpected status `${STATUS}`; re-run `/gsd:execute-phase` verification. 2. **Clean working tree?** ```bash diff --git a/tests/ship-586-verification-routing.test.cjs b/tests/ship-586-verification-routing.test.cjs new file mode 100644 index 000000000..5dc7d319b --- /dev/null +++ b/tests/ship-586-verification-routing.test.cjs @@ -0,0 +1,112 @@ +'use strict'; +// allow-test-rule: runtime-contract-is-the-product +// The ship.md verification gate is LLM-executed prose; its routing message text +// IS the user-facing product surface (RULESET.TESTS.no-source-grep.exemption: +// "reserved for tests where the file content IS the product surface ... agent .md"). +// The behavioral tests below additionally EXECUTE the gate's own bash extraction +// pipeline (parsed out of ship.md) against fixture reports, so the extraction +// contract is verified, not just asserted as text. +const test = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); +const helpers = require('./helpers.cjs'); + +const SHIP_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'ship.md'); +const ship = fs.readFileSync(SHIP_MD, 'utf8'); + +const start = ship.indexOf('**Verification passed?**'); +const end = ship.indexOf('**Clean working tree?**'); +assert.ok(start !== -1 && end !== -1 && end > start, 'could not locate the verification gate block'); +const gate = ship.slice(start, end); + +// ---- content assertions (the routing message IS the product surface) ---- + +test('gate captures the status value with a single first-match grep', () => { + assert.match(gate, /grep -m1 "\^status:"/, 'gate must extract status via grep -m1 "^status:"'); +}); + +test('gate scopes status extraction to the YAML frontmatter only', () => { + assert.match(gate, /sed -n '\/\^---\$\/,\/\^---\$\/p'/, 'gate must restrict extraction to the frontmatter block'); +}); + +test('gate routes gaps_found to /gsd:plan-phase --gaps', () => { + assert.match(gate, /gaps_found/); + assert.match(gate, /\/gsd:plan-phase[^\n]*--gaps/); +}); + +test('gate routes human_needed to the UAT manual-test step', () => { + assert.match(gate, /human_needed/); + assert.match(gate, /UAT\.md/); +}); + +test('gate routes a missing VERIFICATION.md to re-running execute-phase', () => { + assert.match(gate, /\/gsd:execute-phase/); +}); + +test('gate still blocks with PHASE_VERIFICATION_INCOMPLETE', () => { + assert.match(gate, /PHASE_VERIFICATION_INCOMPLETE/); +}); + +test('the dead `pass` status arm is gone — only `passed` is accepted', () => { + assert.doesNotMatch(gate, /status:\s*pass(?!ed)/i, 'no bare `status: pass` arm may remain'); + assert.doesNotMatch(gate, /`pass`\s*\/\s*`passed`/, 'the `pass` / `passed` either-arm must be removed'); + assert.match(gate, /passed/); +}); + +// ---- behavioral tests: run the gate's OWN bash pipeline against fixtures ---- + +const bashBlock = (() => { + // `\r?\n` (not a literal `\n`) so the fence matches on Windows CRLF checkouts; + // normalize the captured block to LF before handing it to bash. + const m = gate.match(/```bash\r?\n([\s\S]*?)```/); + assert.ok(m, 'gate must contain a bash block'); + return m[1].replace(/\r\n/g, '\n'); +})(); + +const hasBash = (() => { + try { execFileSync('bash', ['-c', 'true'], { stdio: 'ignore' }); return true; } + catch { return false; } +})(); + +// The extraction logic is platform-independent; the bash *pipeline* is executed +// only where the gate's shell actually runs (POSIX). On Windows, git-bash exists +// but receives Windows-style tmpdir paths it cannot glob, so skip execution there. +const skipBashPipeline = (process.platform === 'win32' || !hasBash) && 'bash pipeline runs on POSIX only'; + +function runGateExtraction(verificationContents) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ship586-')); + try { + if (verificationContents !== null) { + fs.writeFileSync(path.join(dir, '01-VERIFICATION.md'), verificationContents); + } + const script = `PHASE_DIR='${dir}'\n${bashBlock}\nprintf '%s' "$STATUS"`; + return execFileSync('bash', ['-c', script], { encoding: 'utf8' }); + } finally { + helpers.cleanup(dir); + } +} + +const FM = (status) => + `---\nphase: 01-demo\nverified: 2026-01-01T00:00:00Z\nstatus: ${status}\nscore: 3/3 must-haves verified\n---\n\n# Verification\n`; + +test('extraction yields passed for a passing frontmatter', { skip: skipBashPipeline }, () => { + assert.strictEqual(runGateExtraction(FM('passed')), 'passed'); +}); + +test('extraction yields gaps_found / human_needed verbatim', { skip: skipBashPipeline }, () => { + assert.strictEqual(runGateExtraction(FM('gaps_found')), 'gaps_found'); + assert.strictEqual(runGateExtraction(FM('human_needed')), 'human_needed'); +}); + +test('REGRESSION: a body `status:` line does not corrupt a passing report (Codex PR #650 finding)', { skip: skipBashPipeline }, () => { + const withBodyStatus = FM('passed') + + '\n## Example\n\n```yaml\nstatus: gaps_found\n```\n\nstatus: human_needed\n'; + assert.strictEqual(runGateExtraction(withBodyStatus), 'passed'); +}); + +test('extraction yields empty when no VERIFICATION.md exists', { skip: skipBashPipeline }, () => { + assert.strictEqual(runGateExtraction(null), ''); +});