diff --git a/.changeset/curious-finches-travel.md b/.changeset/curious-finches-travel.md new file mode 100644 index 000000000..4b5ff4255 --- /dev/null +++ b/.changeset/curious-finches-travel.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4769 +--- +**Deferred UAT follow-ups stop blocking phase completion** — `/gsd-verify-work` deferrals (skipped with a "Deferred follow-up" reason) no longer fail the phase-completion predicate, and completing a session with deferrals now offers to promote them into a `999.x` ROADMAP backlog entry; plain unresolved skips still block as before. (#4546) diff --git a/gsd-core/workflows/verify-work.md b/gsd-core/workflows/verify-work.md index f4af00cda..a7c998936 100644 --- a/gsd-core/workflows/verify-work.md +++ b/gsd-core/workflows/verify-work.md @@ -542,6 +542,42 @@ Commit the UAT file: gsd_run query commit "test({phase_num}): complete UAT - {passed} passed, {issues} issues" --files ".planning/phases/XX-name/{phase_num}-UAT.md" ``` +**If the UAT file has a non-empty `## Deferred Follow-Ups` section,** those items are currently visible only inside this phase's `*-UAT.md` — offer to promote them to the roadmap backlog so they stay visible at the project level (#4546; reuses the exact entry mechanism `next.md`'s `prior_phase_completeness` step uses for plans-without-summaries): + +``` +Deferred follow-ups recorded: {N} + +They currently live only in {phase_num}-UAT.md. Promote them to the ROADMAP.md backlog? + + [P] Promote to ROADMAP.md 999.x backlog + [K] Keep them in the UAT file only + +Choice [K]: +``` + +(TEXT_MODE: present this as a plain-text numbered list per the text-mode convention and wait for the typed choice.) + +**If the user chooses [P]:** +1. Compute the next backlog number: `{backlog_number}` = the smallest positive integer not already used by an existing `### Phase 999.{n}` heading in `.planning/ROADMAP.md` — scan the headings rather than counting them, since numbering may be non-contiguous. If `.planning/ROADMAP.md` does not exist, create it containing only a `## Backlog` section and use `1`. +2. Append to that `## Backlog` section one backlog entry per deferred follow-up (each with its own `999.{backlog_number}` heading, incrementing per entry), with `test`/`idea`/`deferred_at` verbatim from the section's YAML and `{idea}` flattened to a single line (newlines → spaces — a multi-line response would corrupt the single-line entry; this mirrors `next.md`'s use of a slug for the same reason): + +```markdown +### Phase 999.{backlog_number}: Follow-up — Phase {phase_num} deferred UAT follow-up: Test {test} (BACKLOG) + +**Goal:** Resolve the UAT checkpoint deferred during Phase {phase_num} verification +**Source phase:** {phase_num} +**Deferred at:** {date} during /gsd:verify-work {phase} session completion +**Follow-ups:** +- [ ] Test {test}: {idea} (deferred {deferred_at}) +``` + +3. Commit the deferral record: +```bash +gsd_run query commit "docs: defer Phase {phase_num} UAT follow-ups to backlog" --files .planning/ROADMAP.md +``` + +**If the user chooses [K]:** continue to the summary unchanged — the deferred items remain in the UAT file's `## Deferred Follow-Ups` section. + Present summary: ``` ## UAT Complete: Phase {phase} diff --git a/scripts/lib/macos-conformance-tier.generated.cjs b/scripts/lib/macos-conformance-tier.generated.cjs index cf1615a32..da205a4ce 100644 --- a/scripts/lib/macos-conformance-tier.generated.cjs +++ b/scripts/lib/macos-conformance-tier.generated.cjs @@ -193,6 +193,7 @@ module.exports = { "tests/state.test.cjs", "tests/todos-workstream-scope.test.cjs", "tests/tracer-bullet.test.cjs", + "tests/uat-predicate.test.cjs", "tests/uat.test.cjs", "tests/ui-safety-gate.test.cjs", "tests/ui-spec-inventory-provenance.test.cjs", diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index f81d44ed5..d51dae86f 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -137,10 +137,11 @@ "verify-npm-publish.test.cjs", "verify-test-quality.test.cjs", "verify-work-auto-transition.test.cjs", + "verify-work-deferred-promotion.test.cjs", "verify.test.cjs" ], "issue": "#3813 \u2014 the live-path #1883 suite drives buildPlanningSnapshot in-process with an fs fault; the verify module suites are CLI-driving and cannot host an fs monkeypatch", - "justification": "verify-command-grounding added by #2401/#3678; allowlist follow-up landed with #3606 (base was red on this lane)." + "justification": "verify-command-grounding added by #2401/#3678; allowlist follow-up landed with #3606 (base was red on this lane). #4546 adds verify-work-deferred-promotion (deferred-follow-up promotion writer contract + parity)." }, "install": { "files": [ diff --git a/src/uat-predicate.cts b/src/uat-predicate.cts index b260f465e..4e949e7c1 100644 --- a/src/uat-predicate.cts +++ b/src/uat-predicate.cts @@ -37,6 +37,13 @@ interface UatCheckItem { name: string; result: string; passing: boolean; + /** + * #4546: true when this item is a `skipped` whose reason matches the + * verify-work writer's "Deferred follow-up:" template — a deliberately + * deferred follow-up (#1921) counts as passing and never blocks. Additive + * report field: consumers read `passed`/`blockers`. + */ + deferred: boolean; } interface UatPassedReport { @@ -83,6 +90,22 @@ const BLOCKING_VERIFICATION_FM_STATUSES = new Set([ // UAT test-item `result` values that count as passing const PASSING_RESULTS = new Set(['passed', 'pass']); +// #4546 — a `skipped` test-item whose reason carries the verify-work writer's +// deferral template prefix ("Deferred follow-up: …", #1921) is a deliberately +// deferred follow-up: non-blocking. Quote-tolerant (the writer wraps the value +// in double quotes) and case-insensitive (human-edited files vary). Anything +// else — a reasonless skip, a non-deferral reason — still blocks. PARITY: this +// matcher and the writer template in gsd-core/workflows/verify-work.md +// (process_response) are two halves of one contract, pinned together by +// tests/verify-work-deferred-promotion.test.cjs. +const DEFERRED_REASON_RE = /^["']?deferred follow-up\b/i; +// Trust note (#4546 review): the reason line is user-authored state — an +// author could equally write `result: passed` — so this prefix is an +// AUTHORING contract with the verify-work writer, not a security boundary. +// A hand-written deferral that skips the UAT file's ## Deferred Follow-Ups +// section also bypasses the complete_session promotion offer; the section is +// the durable project-level record. Variant spellings that do not match +// ("Deferred follow-ups:", "followup") block — fail-closed by design. // ─── stripFalsePositiveContexts ─────────────────────────────────────────────── @@ -156,8 +179,8 @@ function analyzeMarkdown(raw: string): { unterminatedFence: boolean; unterminate * - Support bracketed [passed] and bare passed (#2273). * - Returns ALL items (both passing and non-passing). */ -function parseUatResultItems(cleanContent: string): Array<{ test: number; name: string; result: string }> { - const items: Array<{ test: number; name: string; result: string }> = []; +function parseUatResultItems(cleanContent: string): Array<{ test: number; name: string; result: string; reason: string }> { + const items: Array<{ test: number; name: string; result: string; reason: string }> = []; // Find all ### N. Name headings. // #3078-CR MEDIUM (security review follow-up): STRUCTURE and ATTRIBUTION @@ -212,16 +235,28 @@ function parseUatResultItems(cleanContent: string): Array<{ test: number; name: // ambiguity counting, matching src/uat.cts's contract. // Uses [ \t]* (not \s*) so the captured value must sit on the SAME line as result:. // A result: key with the value on a subsequent line yields no match → 'missing' (blocker). + // #4546: the `reason:` line is captured with the same frame and FIRST-match + // rule — it is the deferral signal the evaluator needs (a `skipped` item + // whose reason is the verify-work writer's "Deferred follow-up:" template + // is non-blocking). Quoted values are captured with their quotes so the + // evaluator's matcher can tolerate them exactly as written. const RESULT_LINE_RE = /^result:[ \t]*\[?([\w-]+)\]?/i; const resultMatch = blockContent .split('\n') .map((line) => line.match(RESULT_LINE_RE)) .find((m): m is RegExpMatchArray => m !== null) ?? null; + const REASON_LINE_RE = /^reason:[ \t]*(.*)$/i; + const reasonMatch = blockContent + .split('\n') + .map((line) => line.match(REASON_LINE_RE)) + .find((m): m is RegExpMatchArray => m !== null) ?? null; + const reason = reasonMatch ? reasonMatch[1].trim() : ''; if (resultMatch) { items.push({ test: h.test, name: h.name, result: resultMatch[1].toLowerCase(), + reason, }); } else { // No column-0 result line → emit 'missing' (a non-passing state) @@ -229,6 +264,7 @@ function parseUatResultItems(cleanContent: string): Array<{ test: number; name: test: h.test, name: h.name, result: 'missing', + reason, }); } } @@ -333,13 +369,20 @@ function evaluateUatPassed( const items = parseUatResultItems(cleanContent); for (const item of items) { - const passing = PASSING_RESULTS.has(item.result); + // #4546: a `skipped` item whose reason matches the verify-work writer's + // "Deferred follow-up:" template is a deliberately deferred follow-up + // (#1921) — non-blocking. Quote-tolerant because the writer emits the + // reason WITH its wrapping quotes. Everything else — pending, blocked, + // issue, missing, and a plain or non-deferral skipped — still blocks. + const deferred = item.result === 'skipped' && DEFERRED_REASON_RE.test(item.reason); + const passing = PASSING_RESULTS.has(item.result) || deferred; checks.push({ file, test: item.test, name: item.name, result: item.result, passing, + deferred, }); if (!passing) { blockers.push(`${file}: test ${item.test} (${item.result})`); diff --git a/src/uat.cts b/src/uat.cts index 8a77f53e4..88c300e84 100644 --- a/src/uat.cts +++ b/src/uat.cts @@ -3973,6 +3973,13 @@ function categorizeItem(rawResult: string, reason?: string, blockedBy?: string): return 'blocked'; } if (result === 'skipped') { + // #4546 — a deliberately deferred follow-up (the verify-work writer's + // "Deferred follow-up:" template, #1921) is its own category, checked + // BEFORE the keyword families so e.g. "…on the release build next + // version" is not misfiled as build_needed. Must agree with + // uat-predicate.cts's DEFERRED_REASON_RE (gate/audit agreement, + // #3078-CR), pinned by tests/uat-predicate.test.cjs. + if (reason && /^["']?deferred follow-up\b/i.test(reason)) return 'deferred'; if (reason) { if (/server|not running|not available/i.test(reason)) return 'server_blocked'; if (/simulator|physical|device/i.test(reason)) return 'device_needed'; diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index 093ee7676..58a8520fa 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": 12113, - "onTokens": 10140, - "reductionPct": 16.29 + "offTokens": 12664, + "onTokens": 10691, + "reductionPct": 15.58 } }, "aggregate": { - "offTokens": 107596, - "onTokens": 90948, - "reductionPct": 15.47 + "offTokens": 108147, + "onTokens": 91499, + "reductionPct": 15.39 } } diff --git a/tests/uat-predicate.test.cjs b/tests/uat-predicate.test.cjs index b2a38c886..ceafc315c 100644 --- a/tests/uat-predicate.test.cjs +++ b/tests/uat-predicate.test.cjs @@ -1654,3 +1654,147 @@ describe('#3078-CR: evaluateUatPassed agrees with the audit surface (parseUatIte assert.equal(auditReport.items.length, 0, 'audit: a clean file must have no outstanding rows'); }); }); + + +// ─── #4546 — deferred follow-up skips are non-blocking ──────────────────────── + +describe('#4546 — deferred follow-up skips', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = makeTmpDir(); + }); + + afterEach(() => { + rmDir(tmpDir); + }); + + function makeDeferredUatItem(n, name, reason) { + const lines = [`### ${n}. ${name}`, `expected: ${name} works`]; + if (reason === null) { + lines.push('result: skipped'); + } else { + lines.push('result: skipped', `reason: ${reason}`); + } + return lines; + } + + test('deferred follow-up skip is non-blocking (#4546)', () => { + const content = [ + '---', 'status: complete', '---', '', + '### 1. Test A', 'expected: A', 'result: passed', '', + ...makeDeferredUatItem(2, 'Test B', '"Deferred follow-up: nice to have, next version"'), '', + ].join('\n'); + writeFile(tmpDir, 'phase-UAT.md', content); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.passed, true, + `a deliberately deferred follow-up must not block: ${JSON.stringify(report.blockers)}`); + assert.strictEqual(report.blockers.length, 0); + const deferred = report.checks.find(c => c.test === 2); + assert.ok(deferred, 'deferred check present'); + assert.strictEqual(deferred.passing, true, 'deferred skip counts as passing'); + assert.strictEqual(deferred.deferred, true, 'deferred skip is flagged deferred in the report'); + const passing = report.checks.find(c => c.test === 1); + assert.strictEqual(passing.deferred, false, 'a real pass is not flagged deferred'); + }); + + test('plain skipped without a reason still blocks (#4546 negative space)', () => { + const content = [ + '---', 'status: complete', '---', '', + '### 1. Test A', 'expected: A', 'result: passed', '', + ...makeDeferredUatItem(2, 'Test B', null), '', + ].join('\n'); + writeFile(tmpDir, 'phase-UAT.md', content); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.passed, false, + 'a skipped test with no reason is unresolved and must block'); + assert.ok(report.blockers.some(b => /test 2/.test(b)), + `expected a blocker for test 2, got: ${JSON.stringify(report.blockers)}`); + }); + + test('skipped with a non-deferral reason still blocks (#4546 negative space)', () => { + const content = [ + '---', 'status: complete', '---', '', + '### 1. Test A', 'expected: A', 'result: passed', '', + ...makeDeferredUatItem(2, 'Test B', '"waiting on credentials"'), '', + ].join('\n'); + writeFile(tmpDir, 'phase-UAT.md', content); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.passed, false, + 'a skipped test whose reason is not a deferral must block'); + }); + + test('deferred reason match is case-insensitive (#4546 boundary)', () => { + const content = [ + '---', 'status: complete', '---', '', + ...makeDeferredUatItem(1, 'Test A', '"deferred follow-up: later"'), '', + ].join('\n'); + writeFile(tmpDir, 'phase-UAT.md', content); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.passed, true, + 'the matcher anchors on the shipped template text case-insensitively'); + }); + + test('a deferred skip does not mask other blockers (#4546 independence)', () => { + const content = [ + '---', 'status: complete', '---', '', + ...makeDeferredUatItem(1, 'Test A', '"Deferred follow-up: later"'), '', + '### 2. Test B', 'expected: B', 'result: pending', '', + '### 3. Test C', 'expected: C', 'result: blocked', 'blocked_by: server', '', + '### 4. Test D', 'expected: D', 'result: issue', 'reported: "crashes"', '', + ].join('\n'); + writeFile(tmpDir, 'phase-UAT.md', content); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.passed, false, + 'deferred skip must not mask pending/blocked/issue blockers'); + assert.ok(report.blockers.some(b => /test 2/.test(b)), 'pending still blocks'); + assert.ok(report.blockers.some(b => /test 3/.test(b)), 'blocked still blocks'); + assert.ok(report.blockers.some(b => /test 4/.test(b)), 'issue still blocks'); + assert.ok(!report.blockers.some(b => /test 1/.test(b)), + `the deferred item must not appear among blockers: ${JSON.stringify(report.blockers)}`); + }); + + test('property: deferred-skip acceptance drives the real gate over arbitrary result/reason pairs (#4546)', () => { + // Gate-driven: expectations are derived from the INPUT (the spec sentence + // in #4546), then asserted against evaluateUatPassed's report — the + // property never restates the implementation's regex. The generated + // classes include the no-result-line shape (the parser's 'missing' + // branch) and reasonless skips. + const deferredRe = /^["']?deferred follow-up\b/i; + fc.assert( + fc.property( + fc.constantFrom('passed', 'pass', 'skipped', 'pending', 'blocked', 'issue', 'missing'), + fc.option(fc.stringMatching(/^["']?[a-z ]{0,30}$/), { nil: undefined }), + (result, reason) => { + const itemLines = [`### 1. Test X`, 'expected: X works']; + if (result !== 'missing') { + itemLines.push(`result: ${result}`); + if (reason !== undefined) itemLines.push(`reason: ${reason}`); + } + const content = [ + '---', 'status: complete', '---', '', + ...itemLines, '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, 'phase-UAT.md'), content, 'utf-8'); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.checks.length, 1); + const check = report.checks[0]; + + const isDeferral = result === 'skipped' && + typeof reason === 'string' && deferredRe.test(reason); + const specPassing = result === 'passed' || result === 'pass' || isDeferral; + + assert.strictEqual(check.result, result === 'missing' ? 'missing' : result); + assert.strictEqual(check.passing, specPassing, + `result=${result} reason=${JSON.stringify(reason)}: gate must ${specPassing ? 'pass' : 'block'}`); + assert.strictEqual(check.deferred, isDeferral, + `result=${result} reason=${JSON.stringify(reason)}: deferred flag`); + assert.strictEqual(report.passed, specPassing, + 'a single-item file passes exactly when the item passes'); + assert.deepStrictEqual(report.blockers, specPassing ? [] : [report.blockers[0]]); + } + ), + { numRuns: 120, seed: 4546 } + ); + }); +}); diff --git a/tests/verify-work-deferred-promotion.test.cjs b/tests/verify-work-deferred-promotion.test.cjs new file mode 100644 index 000000000..0ed17b5ca --- /dev/null +++ b/tests/verify-work-deferred-promotion.test.cjs @@ -0,0 +1,106 @@ +'use strict'; + +/** + * Writer-contract + parity tests for the #4546 deferred-follow-up promotion. + * + * Two parallel surfaces own the deferral contract: + * - the WRITER: gsd-core/workflows/verify-work.md, whose process_response step + * writes `reason: "Deferred follow-up: {verbatim user response}"` and whose + * complete_session step must offer 999.x promotion of the Deferred + * Follow-Ups section (reusing next.md's prior_phase_completeness entry + * shape); + * - the READER: the UAT predicate (src/uat-predicate.cts), which must treat + * that exact template text as non-blocking. + * + * Row 8 drives the writer's OWN template text (extracted from the shipped + * workflow, placeholder substituted) through the real predicate — if either + * surface changes its half of the contract, this fails. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { evaluateUatPassed } = require('../gsd-core/bin/lib/uat-predicate.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); +const { cleanup, createTempDir } = require('./helpers.cjs'); + +const ROOT = path.join(__dirname, '..'); +const VERIFY_WORK_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'verify-work.md'); +const NEXT_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'next.md'); + +function readWorkflow(p) { + return fs.readFileSync(p, 'utf8').replace(/\r\n/g, '\n'); +} + +/** Extract the body of a named block from a workflow, or throw. */ +function stepBlock(src, stepName, label) { + const start = src.indexOf(``); + assert.ok(start !== -1, `${label} must contain `); + const end = src.indexOf('', start); + assert.ok(end !== -1, ` must be closed`); + return src.slice(start, end); +} + +describe('verify-work deferred follow-up promotion (#4546)', () => { + const verifyWork = readWorkflow(VERIFY_WORK_PATH); + + test('writer reason template matches the predicate matcher (#4546 parity)', () => { + // Extract the shipped reason template from the writer (process_response + // step): `reason: "Deferred follow-up: {verbatim user response}"`. + const m = verifyWork.match(/reason: "(Deferred follow-up: \{[^}]+\})"/); + assert.ok(m, 'verify-work.md process_response must write the deferred reason template'); + const templateText = m[1]; // e.g. `Deferred follow-up: {verbatim user response}` + const sampleReason = templateText.replace(/\{[^}]+\}/, 'nice to have, next version'); + + // Drive the writer's own template text through the real predicate: a UAT + // file whose only non-passing item carries this reason must pass. + // allow-test-rule: source-text-is-the-product (#4546) — the readFileSync + // in readWorkflow above is this marker's other suppression site: the + // workflow text is the deployed contract. + const tmpDir = createTempDir('gsd-4546-parity-'); + const content = [ + '---', 'status: complete', '---', '', + '### 1. Test A', 'expected: A', 'result: passed', '', + '### 2. Test B', 'expected: B', 'result: skipped', + `reason: "${sampleReason}"`, '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, 'phase-UAT.md'), content, 'utf8'); + const report = evaluateUatPassed(tmpDir); + assert.strictEqual(report.passed, true, + `the writer's own deferred template text must be non-blocking to the reader: ${JSON.stringify(report.blockers)}`); + cleanup(tmpDir); + }); + + test('complete_session offers 999.x promotion of deferred follow-ups (#4546)', () => { + const block = stepBlock(verifyWork, 'complete_session', 'verify-work.md'); + + // Detection: the step must read the Deferred Follow-Ups section the + // process_response step writes (same section name, exact). + assert.match(block, /Deferred Follow-Ups/, + 'complete_session must consult the Deferred Follow-Ups section'); + + // An OFFER (not silent auto-mutation): the literal [P]/[K] choice pair + // must exist — a bare prose 'offer' would satisfy nothing testable. + assert.match(block, /\[P\] Promote to ROADMAP\.md 999\.x backlog/, + 'complete_session must present the [P] promote choice'); + assert.match(block, /\[K\] Keep them in the UAT file only/, + 'complete_session must present the [K] keep choice'); + + // The promoted entry reuses next.md's exact mechanism shape. + const nextWork = readWorkflow(NEXT_PATH); + const nextBlock = stepBlock(nextWork, 'prior_phase_completeness', 'next.md'); + const nextShapeMarkers = ['### Phase 999.', '**Goal:**', '**Source phase:**', '**Deferred at:**']; + for (const marker of nextShapeMarkers) { + assert.ok(nextBlock.includes(marker), + `next.md prior_phase_completeness entry shape must contain ${marker} (fixture sanity)`); + assert.match(block, new RegExp(escapeRegex(marker)), + `complete_session promotion entry must reuse the next.md shape marker: ${marker}`); + } + + // The deferral record is committed scoped to the roadmap. + assert.match(block, /--files[^\n]*ROADMAP\.md/, + 'the promotion commit must be scoped to .planning/ROADMAP.md via --files'); + }); +});