From 85545a77a5ed8e99ca4b95a45936e1853c6c8742 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 16 Sep 2026 17:26:24 -0400 Subject: [PATCH] fix(#4663): gate the canonicalization on the uat-passed predicate (#4809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4663): add failing-first contract coverage for the blocked-uat canonicalization gate verify-work.md's complete_session step flips VERIFICATION.md to passed on 'zero issues' alone, so a session whose every UAT row is blocked (a session that observed nothing) canonicalizes the report. Pins the deployed contract the fix must satisfy: the flip runs the unflagged phase uat-passed predicate inside the human_needed branch, frontmatter.set sits inside a passed==true guard, a refusal message carries the blocker count and keeps human_needed, and an indeterminate pre-check fails closed. All four new assertions are RED until the workflow grows the guard. * fix(#4663): gate the canonicalization on the uat-passed predicate complete_session flipped VERIFICATION.md to passed whenever the session recorded zero issues and the status was human_needed — but blocked rows are not issues by this workflow's own rule, so a 0-passed / 0-issues / N-blocked session (one that observed nothing) rewrote the canonical report to passed. Every later reader (transition.md's preliminary check, resume paths, validate-phase, verification.status) then inherited the unearned pass while the phase-close predicate correctly refused it. The flip now runs the phase-close predicate in a new --uat-only form before canonicalizing: UAT rows evaluated (at least one pass, no pending/blocked/failed/unexplained-skip row), VERIFICATION-status blockers skipped — they must be, because the report still reads human_needed at pre-check time and that status is itself a blocking verification entry, so the full predicate could never pass there and the flip would deadlock (found by isolated review, probed). The --require-verification call stays the transition gate; the refusal branch reports the blocker count and keeps human_needed; an indeterminate pre-check fails closed. Emitted-Drift-Ack-Growth: verify-work.md — canonicalize block gains the uat-only pre-check and refusal branch (#4663) * test(#4663): align the canonicalize pre-check needles with the shipped line The workflow line carries a 2>/dev/null redirect the needles did not include, so both pre-check assertions fail against the committed fix (fixed-string grep verified). Reviewer-found; needle and message aligned. * fix(#4663): reword the canonicalize prose and refresh its size baseline The rationale paragraph mentioned the flagged transition-gate call by its flag, putting a --require-verification literal before the first phase uat-passed occurrence and breaking the existing ordering pin; the prose now describes it without the literal. verify-work.md's growth also drifted the committed compact-content baseline; regenerated via benchmark-compact-content.cjs --write (derived artifact, report-not-gate contract). Emitted-Drift-Ack-Growth: verify-work.md — canonicalize block gains the uat-only pre-check and refusal branch (#4663) * docs(#4663): backfill changeset PR number --------- Co-authored-by: sim --- .changeset/humble-hawks-munch.md | 5 ++ gsd-core/workflows/verify-work.md | 12 +++- src/phase-command-router.cts | 14 ++++- src/phase.cts | 2 +- src/uat-predicate.cts | 23 ++++++-- .../compact-content-benchmark-baseline.json | 12 ++-- tests/uat-predicate.test.cjs | 56 +++++++++++++++++++ tests/verify-work-auto-transition.test.cjs | 52 +++++++++++++++++ 8 files changed, 161 insertions(+), 15 deletions(-) create mode 100644 .changeset/humble-hawks-munch.md diff --git a/.changeset/humble-hawks-munch.md b/.changeset/humble-hawks-munch.md new file mode 100644 index 000000000..2a48c0a18 --- /dev/null +++ b/.changeset/humble-hawks-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4809 +--- +**verify-work no longer canonicalizes a vacuous pass** — a UAT session with zero logged issues but every row blocked (0 passed) flipped VERIFICATION.md to `passed`, leaving a phase whose central claim was never observed carrying a passed verification. The canonicalize flip now requires the same `phase uat-passed` predicate the phase-close uses (at least one pass, no blocked/pending/failed rows) and, when it refuses, says the verification stays human_needed with the blocker count. (#4663) diff --git a/gsd-core/workflows/verify-work.md b/gsd-core/workflows/verify-work.md index a7c998936..db56f62e1 100644 --- a/gsd-core/workflows/verify-work.md +++ b/gsd-core/workflows/verify-work.md @@ -639,7 +639,7 @@ If an active secure-phase step hook exists AND `SECURITY_FILE` exists: check fro If no active secure-phase step hook exists OR (`SECURITY_FILE` exists AND `threats_open` is `0`): -If execution verification is waiting only on human UAT and this session recorded zero issues, canonicalize the report before the shared completion predicate: +If execution verification is waiting only on human UAT and this session recorded zero issues, canonicalize the report before the shared completion predicate. (#4663) Zero issues is NOT pass evidence on its own — blocked rows are not issues by this workflow's own rule, so a session that observed nothing (0 passed / 0 issues / N blocked) must NOT flip the report. The flip runs the SAME UAT-row predicate the phase-close uses, in its `--uat-only` form: it skips the verification-status blockers (the report still reads `human_needed` at this point — the full predicate could never pass here), and `passed` means at least one UAT check passed with no row pending/blocked/failed or skipped without a reason. The flagged transition-gate call below stays the final say on canonical verification: ```bash PHASE_DIR=$(printf '%s' "$INIT" | jq -r '.phase_dir // empty') @@ -648,7 +648,15 @@ VERIFICATION_STATUS=$(gsd_run query verification.status "$PHASE_DIR" 2>/dev/null VERIFICATION_STATUS_VALUE=$(printf '%s' "$VERIFICATION_STATUS" | jq -r '.status // empty' 2>/dev/null || echo "") PHASE_VERIFICATION_STATUS="$VERIFICATION_STATUS_VALUE" if [ "$VERIFICATION_STATUS_VALUE" = "human_needed" ]; then - gsd_run query frontmatter.set "$VERIFICATION_FILE" --field status --value passed + UAT_PRECHECK=$(gsd_run phase uat-passed "{phase}" --uat-only 2>/dev/null) + UAT_PRECHECK_PASSED=$(printf '%s' "$UAT_PRECHECK" | jq -r '.passed // false' 2>/dev/null || echo "false") + if [ "$UAT_PRECHECK_PASSED" = "true" ]; then + gsd_run query frontmatter.set "$VERIFICATION_FILE" --field status --value passed + else + UAT_BLOCKERS=$(printf '%s' "$UAT_PRECHECK" | jq -r '.blockers | length' 2>/dev/null) + [ -n "$UAT_BLOCKERS" ] || UAT_BLOCKERS="?" + echo "NOT canonicalizing: ${UAT_BLOCKERS} UAT row(s) blocked or not passing; verification stays human_needed. Resolve or pass them, then re-run /gsd:verify-work {phase}." >&2 + fi fi ``` diff --git a/src/phase-command-router.cts b/src/phase-command-router.cts index 1472b67f4..ff38d3166 100644 --- a/src/phase-command-router.cts +++ b/src/phase-command-router.cts @@ -45,7 +45,7 @@ interface PhaseHandlers { ) => void; cmdPhaseRemove: (cwd: string, phaseNum: string, opts: { force: boolean }, raw: boolean) => void; cmdPhaseComplete: (cwd: string, phaseNum: string | undefined, raw: boolean) => void; - cmdPhaseUatPassed: (cwd: string, phaseNum: string | undefined, raw: boolean, opts?: { policy?: { requireVerification?: boolean } }) => void; + cmdPhaseUatPassed: (cwd: string, phaseNum: string | undefined, raw: boolean, opts?: { policy?: { requireVerification?: boolean; uatOnly?: boolean } }) => void; cmdPhaseListPlans: (cwd: string, phaseNum: string | undefined, raw: boolean) => void; } @@ -222,10 +222,14 @@ function routePhaseCommand({ phase, args, cwd, raw, error }: RoutePhaseCommandOp }, 'uat-passed': (_ctx: Record): { ok: true; data: null } => { let requireVerification = false; + let uatOnly = false; const positional: string[] = []; for (const token of args.slice(2)) { if (token === '--require-verification') { requireVerification = true; + } else if (token === '--uat-only') { + // #4663: evaluate UAT rows only (verification-status blockers skipped). + uatOnly = true; } else if (token === '--raw') { // --raw is handled by the outer CLI layer; accepted here silently } else if (token.startsWith('--')) { @@ -234,7 +238,13 @@ function routePhaseCommand({ phase, args, cwd, raw, error }: RoutePhaseCommandOp positional.push(token); } } - phase.cmdPhaseUatPassed(cwd, positional[0], raw, { policy: { requireVerification } }); + if (requireVerification && uatOnly) { + return makeInvalidArgs( + '--uat-only', + '--uat-only and --require-verification are mutually exclusive', + ) as never; + } + phase.cmdPhaseUatPassed(cwd, positional[0], raw, { policy: { requireVerification, uatOnly } }); return { ok: true as const, data: null }; }, // #1437 — list plan files for a phase diff --git a/src/phase.cts b/src/phase.cts index d56e0b8be..ada98269c 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -4693,7 +4693,7 @@ function cmdPhaseUatPassed( cwd: string, phaseNum: string | undefined, raw: boolean, - opts: { policy?: { requireVerification?: boolean } } = {}, + opts: { policy?: { requireVerification?: boolean; uatOnly?: boolean } } = {}, ): void { if (!phaseNum) { error('phase number required for phase uat-passed'); diff --git a/src/uat-predicate.cts b/src/uat-predicate.cts index 4e949e7c1..bde4d4ec4 100644 --- a/src/uat-predicate.cts +++ b/src/uat-predicate.cts @@ -55,6 +55,14 @@ interface UatPassedReport { no_uat_artifacts: boolean; policy: { require_verification: boolean; + /** + * #4663: true when the report was produced in uat-only mode — UAT rows + * evaluated, VERIFICATION-file blockers (and only those) skipped. The + * verify-work canonicalize pre-check uses this form; the flip it gates is + * what removes the human_needed verification status, so the full + * predicate could never pass at pre-check time. + */ + uat_only: boolean; }; /** * #3057 B3: true when the `requireVerification` policy check's own @@ -283,9 +291,15 @@ function parseUatResultItems(cleanContent: string): Array<{ test: number; name: */ function evaluateUatPassed( phaseFullDir: string, - opts?: { policy?: { requireVerification?: boolean } }, + opts?: { policy?: { requireVerification?: boolean; uatOnly?: boolean } }, ): UatPassedReport { - const requireVerification = opts?.policy?.requireVerification === true; + // uatOnly (#4663) takes precedence: it evaluates the UAT rows ONLY, skipping + // the VERIFICATION-file blockers entirely. The verify-work canonicalize + // pre-check needs exactly that — it runs while the report still reads + // `human_needed`, which is itself a blocking verification status, so the + // full predicate could never pass there and the flip would deadlock. + const uatOnly = opts?.policy?.uatOnly === true; + const requireVerification = !uatOnly && opts?.policy?.requireVerification === true; const blockers: string[] = []; const checks: UatCheckItem[] = []; @@ -309,7 +323,7 @@ function evaluateUatPassed( checks: [], blockers, no_uat_artifacts, - policy: { require_verification: requireVerification }, + policy: { require_verification: requireVerification, uat_only: uatOnly }, // readVerificationStatus was never reached on this early-return path. verification_stale_check_indeterminate: false, }; @@ -392,7 +406,7 @@ function evaluateUatPassed( // ── Process VERIFICATION files ───────────────────────────────────────────── let hasPassingVerification = false; - for (const file of verFileNames) { + for (const file of uatOnly ? [] : verFileNames) { verificationFiles.push(file); const verificationFilePath = path.join(phaseFullDir, file); let raw = ''; @@ -451,6 +465,7 @@ function evaluateUatPassed( no_uat_artifacts, policy: { require_verification: requireVerification, + uat_only: uatOnly, }, verification_stale_check_indeterminate: verificationStaleCheckIndeterminate, }; diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index d4f7e934b..e55b42c82 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -33,14 +33,14 @@ "reductionPct": 11.9 }, "verify-work": { - "offTokens": 12664, - "onTokens": 10691, - "reductionPct": 15.58 + "offTokens": 12992, + "onTokens": 11019, + "reductionPct": 15.19 } }, "aggregate": { - "offTokens": 108302, - "onTokens": 91615, - "reductionPct": 15.41 + "offTokens": 108630, + "onTokens": 91943, + "reductionPct": 15.36 } } diff --git a/tests/uat-predicate.test.cjs b/tests/uat-predicate.test.cjs index ceafc315c..6c9067206 100644 --- a/tests/uat-predicate.test.cjs +++ b/tests/uat-predicate.test.cjs @@ -599,6 +599,62 @@ describe('evaluateUatPassed — policy.requireVerification', () => { }); }); +// ─── evaluateUatPassed — policy.uatOnly (#4663) ─────────────────────────────── +// +// The verify-work canonicalize pre-check runs WHILE the report still reads +// `human_needed` — and `human_needed` is itself a blocking verification +// status, so the full predicate can never pass at pre-check time and the +// flip would deadlock. uatOnly evaluates the UAT rows ONLY: verification-file +// blockers (and only those) are skipped, while every UAT-row blocker +// (pending/blocked/failed/skip-without-reason) and the vacuous-pass guard +// still apply. + +describe('evaluateUatPassed — policy.uatOnly (#4663)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = makeTmpDir(); + }); + + afterEach(() => { + rmDir(tmpDir); + }); + + test('uatOnly ignores the human_needed verification blocker the flip is about to remove', () => { + writeFile(tmpDir, 'phase-UAT.md', makePassingUat(1)); + writeFile(tmpDir, 'phase-VERIFICATION.md', '---\nstatus: human_needed\n---\n\nWaiting on hardware.'); + const full = evaluateUatPassed(tmpDir, {}); + assert.strictEqual(full.passed, false, + 'the full predicate must refuse while verification is human_needed (the blocker the flip removes)'); + assert.ok(full.blockers.some(b => /human_needed/.test(b)), + `expected a human_needed blocker, got: ${JSON.stringify(full.blockers)}`); + const report = evaluateUatPassed(tmpDir, { policy: { uatOnly: true } }); + assert.strictEqual(report.passed, true, 'uat-only: all UAT rows pass → the pre-check may flip'); + assert.strictEqual(report.blockers.length, 0); + assert.strictEqual(report.policy.uat_only, true); + assert.strictEqual(report.policy.require_verification, false); + }); + + test('uatOnly still refuses blocked UAT rows — the vacuous pass stays impossible', () => { + writeFile(tmpDir, 'phase-UAT.md', [ + '---', 'status: human_needed', '---', '', '# UAT', '', + '### 1. Test 1', 'expected: It works', 'result: blocked', '', + ].join('\n')); + writeFile(tmpDir, 'phase-VERIFICATION.md', '---\nstatus: human_needed\n---\n\nWaiting on hardware.'); + const report = evaluateUatPassed(tmpDir, { policy: { uatOnly: true } }); + assert.strictEqual(report.passed, false, 'a blocked row is a non-pass result even in uat-only mode'); + assert.ok(report.blockers.length > 0); + }); + + test('uatOnly and requireVerification are mutually exclusive — uatOnly wins', () => { + writeFile(tmpDir, 'phase-UAT.md', makePassingUat(1)); + const report = evaluateUatPassed(tmpDir, { policy: { uatOnly: true, requireVerification: true } }); + assert.strictEqual(report.passed, true, 'uatOnly takes precedence; the verification policy is not evaluated'); + assert.strictEqual(report.policy.require_verification, false); + assert.strictEqual(report.policy.uat_only, true); + }); +}); + // ─── evaluateUatPassed — #3057 B3: staleness-check indeterminate is surfaced ── // // readVerificationStatus's internal staleness check can fail (fs / diff --git a/tests/verify-work-auto-transition.test.cjs b/tests/verify-work-auto-transition.test.cjs index 9bb8ca060..039deb879 100644 --- a/tests/verify-work-auto-transition.test.cjs +++ b/tests/verify-work-auto-transition.test.cjs @@ -129,6 +129,58 @@ describe('verify-work.md — auto-transition after UAT passes with 0 issues', () }); }); +// ── #4663 — the canonicalize flip requires the UAT predicate, not a vacuous zero ── +// "zero issues" is not pass evidence: blocked rows are not issues by this same +// workflow's rule, so a 0-passed / 0-issues / N-blocked session must NOT flip +// VERIFICATION.md to `passed`. The flip now consumes the SAME predicate the +// phase-close uses in its --uat-only form (the verification-status blocker +// is exactly what the flip removes, so the full predicate could never pass +// at pre-check time); the flagged call stays the later transition gate. +describe('verify-work.md — canonicalize flip is gated by the UAT predicate (#4663)', () => { + test('canonicalize flips to passed only when the uat-passed predicate reports passed (#4663)', () => { + const content = fs.readFileSync(VERIFY_WORK, 'utf-8'); + const humanNeededIdx = content.indexOf('if [ "$VERIFICATION_STATUS_VALUE" = "human_needed" ]; then'); + const precheckIdx = content.indexOf('UAT_PRECHECK=$(gsd_run phase uat-passed "{phase}" --uat-only 2>/dev/null)'); + const flipGuardIdx = content.indexOf('if [ "$UAT_PRECHECK_PASSED" = "true" ]; then'); + const setPassedIdx = content.indexOf('gsd_run query frontmatter.set "$VERIFICATION_FILE" --field status --value passed'); + + assert.ok(precheckIdx !== -1, 'the canonicalize block must run the uat-passed predicate before flipping'); + assert.ok(humanNeededIdx !== -1 && precheckIdx > humanNeededIdx, 'the pre-check must sit inside the human_needed branch'); + assert.ok(content.includes(".passed // false"), 'the verdict must be extracted from the typed report with a false default'); + assert.ok(flipGuardIdx !== -1 && flipGuardIdx > precheckIdx, 'the flip must be guarded on the extracted passed verdict'); + assert.ok(setPassedIdx > flipGuardIdx, 'frontmatter.set must sit INSIDE the passed==true guard'); + }); + + test('the canonicalize pre-check runs uat-passed without --require-verification (#4663)', () => { + const content = fs.readFileSync(VERIFY_WORK, 'utf-8'); + const precheckIdx = content.indexOf('UAT_PRECHECK=$(gsd_run phase uat-passed "{phase}" --uat-only 2>/dev/null)'); + const flaggedIdx = content.indexOf('PHASE_COMPLETE=$(gsd_run phase uat-passed "{phase}" --require-verification)'); + + assert.ok(precheckIdx !== -1, 'the --uat-only pre-check must exist'); + assert.ok( + !content.slice(precheckIdx, precheckIdx + 120).includes('--require-verification'), + 'the pre-check is the unflagged predicate - requiring verification there would evaluate the very report being written' + ); + assert.ok(flaggedIdx !== -1 && flaggedIdx > precheckIdx, 'the flagged predicate remains the later transition gate'); + }); + + test('refused canonicalization says the verification stays human_needed (#4663)', () => { + const content = fs.readFileSync(VERIFY_WORK, 'utf-8'); + assert.match(content, /stays human_needed/, 'the refusal message must say verification stays human_needed'); + assert.match(content, /blockers \| length/, 'the refusal must carry the blocking-row count'); + }); + + test('an indeterminate pre-check must not flip the report (fail closed) (#4663)', () => { + const content = fs.readFileSync(VERIFY_WORK, 'utf-8'); + // jq -r '.passed // false' with an `|| echo "false"` fallback: empty or + // failed gsd_run output must yield no-flip, never a flip. + assert.ok( + content.includes(`jq -r '.passed // false' 2>/dev/null || echo "false"`), + 'the extraction must default to false on empty/failed output' + ); + }); +}); + // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-3381-verify-work-workstream.test.cjs — consolidation epic #1969 (B4 #1973)