From b42cb4fb296f5940fc0e913cb25d07d5277b8701 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 18 Aug 2026 07:26:28 -0400 Subject: [PATCH] fix(#3597): count scenario expectation failures in the QA gate, and fix the workstream scope split it exposed (#3607) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3597): count scenario expectation failures in the QA ratchet gate buildReport counts totals.violations as oracle violations PLUS scenario expectFailures, but collectFindings read only step.violations. A scenario whose declared expect failed therefore produced ok:false and violations:1 in the report while the ratchet printed "0 violations" and exited 0. multi-workstream has failed that way on every CI run since 2026-08-10, when #3217 (PR #3318) made computeProgressPercent withhold a percentage whose scope is not COMPLETE. The walk detected the change the day it landed; nothing was listening. - collectFindings returns a third bucket, expectationFailures, carrying no fingerprint so it can never be baselined or acked away - both modes of main() print and gate on it; the summary line reports it - guard runMain(main) behind require.main === module, so the QA suite can require the script to test collectFindings without running a real walk (that import side effect is why the gate logic had no test) - multi-workstream now asserts the true contract: phase_scope unreadable and percent null, per ADR-3180 7.6 rule 4 - the perturbation test asserts scenario ok, closing the test-side half Closes #3597 * fix(#3597): resolve the milestone window against the active workstream listMilestonePhaseDirs defaulted its ws option to null. planningDir treats undefined as "resolve the ambient workstream" and null as "force the project root", so that default suppressed the ambient resolution every other planning-path read uses. All 18 call sites derive phasesDir ambiently via planningPaths(cwd), so the counts came from the workstream while the milestone window came from the root .planning/ROADMAP.md — the exact numerator/denominator scope split ADR-3180 7.6 rule 3 forbids. workstream create migrates that root roadmap away, so the read threw and scope stayed UNREADABLE, and rule 4 then correctly withheld the percentage. Proof: with a workstream tree byte-unchanged, copying its own ROADMAP to the project root flipped --ws alpha progress from phase_scope:unreadable/percent:null to complete/100. This is the defect the loop QA walk was pointing at all along; the scenario expectation is restored to percent:100 rather than bent to match the bug. - pass ws through as undefined so ambient resolution applies - multi-workstream asserts phase_scope complete + percent 100 - regression test in completion-ratio-scope-withholding covers a workstream-only project with no root ROADMAP - replace the vacuous require.main test: runMain defers through a promise, so the in-process timing check passed against the unguarded file too; a child-process spawn now observes the guard for real - tie the oracle-violation test to expectationFailures, and cover the absent-key, multi-scenario and zero-step report shapes in parity - flatten scenario-authored strings before rendering them into the step summary and CI logs (forged markdown / ANSI injection) - widen the scenario contract assertions past perturbation-* so multi-workstream is actually covered test-side Closes #3597 * fix(#3597): flatten scenario-authored strings on the CI-log output path The step-summary path already routed findings through flattenUntrusted; the check-mode NEW-smell and STALE-entry console.error blocks, and the repro line in both printers, still interpolated raw. detail carries a scenario-authored expect[].path verbatim, and reason/scenario/id come from contributor-authored baseline and ack fragments validated only as non-empty strings. A crafted path could print a forged summary line into the CI log directly above the real one, plus ANSI repaint and unbounded length. Exit codes are unaffected — this is log spoofing, not gate bypass. * fix(#3597): refuse to archive on an unreadable milestone window; close review gaps Resolving the milestone window against the active workstream can leave the window UNREADABLE when that workstream has no ROADMAP of its own. getMilestonePhaseFilter throws, the window degrades to a pass-all fallback, and milestone complete would then move every phase dir -- breaking the guarantee stated at the archive site that no out-of-window directory is touched. milestone complete now refuses to archive when the window is UNREADABLE and reports the refusal; --dry-run previews the same refusal from the same shared derivation. The guard is scoped to UNREADABLE, not to every non-COMPLETE scope. A broader condition regressed ordinary root projects: the QA walk caught milestone-rollover leaving 01-parser on disk, which then tripped the #1447 abort in phases clear. UNSCOPED and TRUNCATED are pre-existing classifications and keep their existing behavior. Review fixes: - the workstream regression test asserted complete/100 but its fixture wrote no workstream STATE.md, so it resolved unscoped/null and the test failed; it now asserts a milestone and genuinely fails-first - the parity test hand-supplied totals.violations, hardcoding the very formula under test; at least one case now goes through the real buildReport - drop a vacuous qa-report.json assertion (jsonOut defaults to null, so no report is written by either shape) - buildRepro emitted a repo-relative binary path after cd-ing into a temp project, so every repro died with MODULE_NOT_FOUND; it now resolves an absolute path - flattenUntrusted truncated the repro to 300 chars, handing reviewers a command that looks complete and is not; length capping is now opt-out for repro while newline/control/backtick stripping still applies * chore(#3597): backfill changeset pr number (#3607) --------- Co-authored-by: sim --- .changeset/fierce-quails-cheer.md | 5 + docs/TESTING-SUITES.md | 31 +- scripts/qa-smell-ratchet.cjs | 185 +++++++-- src/milestone.cts | 91 ++++- src/phase-locator.cts | 17 +- ...ompletion-ratio-scope-withholding.test.cjs | 56 +++ tests/loop-walk.qa.test.cjs | 357 +++++++++++++++++- tests/milestone-archive.test.cjs | 161 ++++++++ tests/qa/report.cjs | 29 +- tests/qa/scenarios/multi-workstream.json | 3 +- 10 files changed, 865 insertions(+), 70 deletions(-) create mode 100644 .changeset/fierce-quails-cheer.md diff --git a/.changeset/fierce-quails-cheer.md b/.changeset/fierce-quails-cheer.md new file mode 100644 index 000000000..b82ea4098 --- /dev/null +++ b/.changeset/fierce-quails-cheer.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3607 +--- +**`progress`, `stats`, and `query progress` now report a real percentage inside a workstream** — under `--ws`, these commands counted the workstream's own phases and plans but read the milestone window from the project root, which `workstream create` has already migrated away. The scope resolved as unreadable and the percentage was withheld, so a fully-complete workstream reported no progress at all. **`milestone complete` no longer archives every phase directory when its milestone window is unreadable** — it previously fell back to moving everything on disk in that case; it now declines to archive and reports why, leaving the phase directories in place. (#3597) diff --git a/docs/TESTING-SUITES.md b/docs/TESTING-SUITES.md index 0eb209629..e8315f657 100644 --- a/docs/TESTING-SUITES.md +++ b/docs/TESTING-SUITES.md @@ -185,7 +185,8 @@ self-test. Separately, `scripts/qa-smell-ratchet.cjs` drives that same harness end to end against the real `gsd-tools` binary and turns its findings into a CI gate — run it with `npm run lint:qa-smells`. -The harness's oracles (`tests/qa/oracles.cjs`) distinguish two severities: +The gate has three independent inputs. The harness's oracles +(`tests/qa/oracles.cjs`) supply the first two: - A **violation** is the engine breaking a documented contract. It always fails the build — baseline or no baseline, acknowledged or not. @@ -196,6 +197,34 @@ The harness's oracles (`tests/qa/oracles.cjs`) distinguish two severities: shrink-only, so a fixed or changed scenario must be pruned, not left behind). +The third comes from the scenarios themselves, not from an oracle: + +- A **scenario expectation failure** is a step's declared `expect` not + holding — the scenario asserted `percent: 100` and the engine returned + something else. Like a violation, it is **never acknowledgeable**: it + carries no fingerprint, so there is no `key` to put in a baseline entry or + an ack fragment. Fix the engine, or correct the expectation. + +Expectation failures were invisible to the gate until +[#3597](https://github.com/open-gsd/gsd-core/issues/3597): `buildReport` +counted them in `totals.violations` while `collectFindings` read only oracle +violations, so a failing scenario printed `0 violations` and exited 0. The +`multi-workstream` scenario failed on every CI run for three weeks without +reddening a build. The invariant that keeps the two honest — asserted in +`tests/loop-walk.qa.test.cjs` — is: + +``` +collectFindings().violations.length + + collectFindings().expectationFailures.length + === report.totals.violations +``` + +Note also that `scripts/qa-smell-ratchet.cjs` only invokes its own `main()` +under `require.main === module`. That guard is what lets the QA suite +`require()` the script to test `collectFindings` without kicking off a real +20-scenario walk as an import side effect — the reason the gate's own logic +had no test before #3597. + Every smell must terminate in exactly one of TWO states — there is no third "accepted with a good explanation" state: diff --git a/scripts/qa-smell-ratchet.cjs b/scripts/qa-smell-ratchet.cjs index 20a0db622..28a53bcae 100644 --- a/scripts/qa-smell-ratchet.cjs +++ b/scripts/qa-smell-ratchet.cjs @@ -35,6 +35,10 @@ * contract) is a completely different thing and is NEVER acknowledgeable * through this mechanism: it always fails, baseline or no baseline. This * script's whole ratchet apparatus applies to smells alone. + * - A scenario EXPECTATION FAILURE (a step's declared `expect` did not + * hold — `step.expectFailures`) is, like a VIOLATION, NEVER + * acknowledgeable through this ratchet: it has no fingerprint and no + * baseline/fragment path, and it always fails the build. * * WHY A BASELINE FILE *AND* A FRAGMENTS DIRECTORY (not just one) * ────────────────────────────────────────────────────────────── @@ -56,8 +60,9 @@ * # (real repro commands; see * # `report.cjs`'s buildRepro) * - * Exit code 0 only when: zero violations, zero NEW smells, zero STALE - * baseline/fragment entries. Exit code 1 otherwise. + * Exit code 0 only when: zero violations, zero scenario expectation + * failures, zero NEW smells, zero STALE baseline/fragment entries. Exit + * code 1 otherwise. */ const fs = require('node:fs'); @@ -323,20 +328,38 @@ function mergeKnown(baselineEntries, fragmentEntries) { } /** - * Walk `reportObject.scenarios[].steps[]` and split every finding into - * `smells` (fingerprinted) and `violations` (never acknowledgeable — see - * this file's header). Both carry the step's `repro` command for later use - * in failure messages / the GitHub step summary. + * Walk `reportObject.scenarios[].steps[]` and split every finding into three + * buckets: `smells` (fingerprinted, ratcheted against the baseline), + * `violations` (never acknowledgeable — see this file's header), and + * `expectationFailures` (a step's declared `expect` did not hold; also never + * acknowledgeable — see this file's header). All three carry the step's + * `repro` command for later use in failure messages / the GitHub step + * summary. + * + * `expectationFailures` entries deliberately carry NO `key` field and are + * never passed through `fingerprint()`: unlike a smell, an expectation + * failure has no baseline/fragment acknowledgment path at all, so giving it + * a fingerprint would invite exactly the laundering this ratchet exists to + * prevent (#3597). + * + * INVARIANT: `violations.length + expectationFailures.length` must always + * equal `reportObject.totals.violations` — see `tests/qa/report.cjs`'s + * `buildReport()`, which computes that total the same way. This is the + * parity that broke in #3597: this function used to read only + * `step.violations`, so a scenario whose `expect` failed produced + * `totals.violations: 1` while this script counted (and printed) 0. * * @param {ReturnType} reportObject * @returns {{ * smells: Array<{key:string,id:string,scenario:string,argv:string[],detail:string,at:string,repro:string}>, * violations: Array<{id:string,scenario:string,argv:string[],detail:string,at:string,repro:string}>, + * expectationFailures: Array<{scenario:string,argv:string[],detail:string,at:string,repro:string}>, * }} */ function collectFindings(reportObject) { const smells = []; const violations = []; + const expectationFailures = []; for (const scenario of reportObject.scenarios) { for (const step of scenario.steps) { for (const v of step.violations || []) { @@ -344,6 +367,11 @@ function collectFindings(reportObject) { id: v.id, scenario: scenario.name, argv: step.argv, detail: v.detail, at: step.at, repro: step.repro, }); } + for (const detail of step.expectFailures || []) { + expectationFailures.push({ + scenario: scenario.name, argv: step.argv, detail, at: step.at, repro: step.repro, + }); + } for (const smell of step.smells || []) { const key = fingerprint(scenario.name, { id: smell.id, subject: smell.subject, argv: step.argv }); smells.push({ @@ -358,7 +386,7 @@ function collectFindings(reportObject) { } } } - return { smells, violations }; + return { smells, violations, expectationFailures }; } /** Lowercase, hyphenate, and strip anything that isn't `[a-z0-9-]`, for a fragment-filename skeleton. */ @@ -388,6 +416,42 @@ function fragmentSkeleton(finding) { return `${suggestedName}:\n${JSON.stringify(doc, null, 2)}`; } +/** + * Flatten one untrusted, scenario-authored string for safe single-line + * rendering into CI logs and the GitHub step summary. + * + * `detail` / `scenario` / `at` values originate in scenario JSON + * (`expect[].path` reaches `detail` verbatim via `evaluateExpectations`) + * and are validated only as non-empty strings. Rendered raw into + * `$GITHUB_STEP_SUMMARY` — which GitHub renders as markdown — a newline + * plus a forged heading or a fake "0 expectation failures" line lets a + * red run present a green-looking summary; a backtick breaks out of the + * code span it is rendered inside; an ANSI escape repaints the CI log. + * + * The 300-char truncation is a SEPARATE concern from the neutralization + * above and is controllable via `maxLen`: every existing caller keeps the + * default (a long `detail`/`scenario`/`at` value is fine to summarize), but + * the `repro` field is a copy-pasteable command — truncating it produces a + * string that *looks* like a complete, runnable command but silently isn't + * (it dies mid-argv or mid-path), which is worse than no repro at all. Pass + * `{ maxLen: Infinity }` at those call sites to lift the cap while keeping + * every other neutralization (newlines/control chars/backticks) intact. + * + * @param {unknown} value + * @param {{maxLen?: number}} [opts] + * @returns {string} + */ +function flattenUntrusted(value, { maxLen = 300 } = {}) { + let s = String(value) + // eslint-disable-next-line no-control-regex -- deliberately stripping C0/C1 control chars (incl. CR/LF/ANSI escapes) + .replace(/[\x00-\x1f\x7f-\x9f]/g, ' ') + .replace(/`/g, "'") + .replace(/\s+/g, ' ') + .trim(); + if (s.length > maxLen) s = `${s.slice(0, maxLen)}…`; + return s; +} + /** * Build the markdown block appended to `GITHUB_STEP_SUMMARY`, when set — * kept intentionally compact (a PR reviewer's first read, not a log dump). @@ -395,28 +459,38 @@ function fragmentSkeleton(finding) { * @param {{ * smells: ReturnType['smells'], * violations: ReturnType['violations'], + * expectationFailures: ReturnType['expectationFailures'], * newKeys: string[], * staleEntries: Array<{key:string,id:string,scenario:string,source:string}>, * smellSummary: Array<{id:string,count:number,examples:string[]}>, * }} data * @returns {string} */ -function buildStepSummaryMarkdown({ smells, violations, newKeys, staleEntries, smellSummary }) { +function buildStepSummaryMarkdown({ smells, violations, expectationFailures, newKeys, staleEntries, smellSummary }) { const lines = []; lines.push('## QA smell ratchet'); lines.push(''); lines.push( `**${smells.length} smells** (${newKeys.length} new, ${staleEntries.length} stale) · ` - + `**${violations.length} violations**`, + + `**${violations.length} violations** · **${expectationFailures.length} expectation failures**`, ); lines.push(''); + if (expectationFailures.length) { + lines.push('### ❌ Scenario expectation failures'); + lines.push(''); + for (const f of expectationFailures) { + lines.push(`- \`${flattenUntrusted(f.scenario)}\` at **${flattenUntrusted(f.at)}** — ${flattenUntrusted(f.detail)}`); + } + lines.push(''); + } + if (newKeys.length) { lines.push('### 🚨 NEW (unacknowledged) smells'); lines.push(''); for (const key of newKeys) { const f = smells.find((s) => s.key === key); - lines.push(`- \`${f.id}\` in **${f.scenario}** — ${f.detail}`); + lines.push(`- \`${flattenUntrusted(f.id)}\` in **${flattenUntrusted(f.scenario)}** — ${flattenUntrusted(f.detail)}`); } lines.push(''); } @@ -425,7 +499,7 @@ function buildStepSummaryMarkdown({ smells, violations, newKeys, staleEntries, s lines.push('### Stale baseline/fragment entries (no longer produced)'); lines.push(''); for (const e of staleEntries) { - lines.push(`- \`${e.id}\` in **${e.scenario}** (${e.source})`); + lines.push(`- \`${flattenUntrusted(e.id)}\` in **${flattenUntrusted(e.scenario)}** (${flattenUntrusted(e.source)})`); } lines.push(''); } @@ -436,18 +510,23 @@ function buildStepSummaryMarkdown({ smells, violations, newKeys, staleEntries, s lines.push('| oracle id | count |'); lines.push('|---|---|'); for (const entry of smellSummary) { - lines.push(`| \`${entry.id}\` | ${entry.count} |`); + lines.push(`| \`${flattenUntrusted(entry.id)}\` | ${entry.count} |`); } lines.push(''); } const firstFailingRepro = (violations[0] && violations[0].repro) + || (expectationFailures[0] && expectationFailures[0].repro) || (newKeys.length && smells.find((s) => s.key === newKeys[0]).repro); if (firstFailingRepro) { lines.push('### Repro (first failing step)'); lines.push(''); lines.push('```sh'); - lines.push(firstFailingRepro); + // Backticks are replaced with `'` by flattenUntrusted, so the flattened + // value can never contain a ``` run that would close this fence early. + // maxLen: Infinity — a truncated repro looks runnable and isn't, which + // is worse than no repro at all (see flattenUntrusted's JSDoc). + lines.push(flattenUntrusted(firstFailingRepro, { maxLen: Infinity })); lines.push('```'); lines.push(''); } @@ -475,7 +554,7 @@ function main() { fs.writeFileSync(jsonOut, `${JSON.stringify(reportObject, null, 2)}\n`, 'utf8'); } - const { smells, violations } = collectFindings(reportObject); + const { smells, violations, expectationFailures } = collectFindings(reportObject); const runKeys = new Set(smells.map((s) => s.key)); const baseline = readBaseline({ allowMissing: update }); @@ -555,6 +634,7 @@ function main() { const md = buildStepSummaryMarkdown({ smells, violations, + expectationFailures, newKeys: added, staleEntries: removed.map((key) => ({ key, id: '(pruned)', scenario: '(pruned)', source: BASELINE_REL_PATH })), smellSummary: reportObject.smellSummary, @@ -564,12 +644,17 @@ function main() { console.log( `\nqa-smell-ratchet: ${smells.length} smells (${added.length} new, ${removed.length} stale), ` - + `${violations.length} violations`, + + `${violations.length} violations, ${expectationFailures.length} expectation failures`, ); - if (violations.length) { - printViolations(violations); - throw new ExitError(1, 'qa-smell-ratchet --update: baseline regenerated, but VIOLATIONS remain (never acknowledgeable — see above)'); + if (violations.length || expectationFailures.length) { + if (violations.length) printViolations(violations); + if (expectationFailures.length) printExpectationFailures(expectationFailures); + throw new ExitError( + 1, + 'qa-smell-ratchet --update: baseline regenerated, but VIOLATIONS and/or SCENARIO EXPECTATION FAILURES remain ' + + '(neither is ever acknowledgeable — see above)', + ); } return; } @@ -589,14 +674,18 @@ function main() { printViolations(violations); } + if (expectationFailures.length) { + printExpectationFailures(expectationFailures); + } + if (newKeys.length) { console.error(`\nqa-smell-ratchet: ${newKeys.length} NEW (unacknowledged) smell(s):\n`); for (const key of newKeys) { const f = smells.find((s) => s.key === key); - console.error(`NEW smell: ${f.key}`); - console.error(` oracle: ${f.id}`); - console.error(` scenario: ${f.scenario}`); - console.error(` detail: ${f.detail}`); + console.error(`NEW smell: ${flattenUntrusted(f.key)}`); + console.error(` oracle: ${flattenUntrusted(f.id)}`); + console.error(` scenario: ${flattenUntrusted(f.scenario)}`); + console.error(` detail: ${flattenUntrusted(f.detail)}`); console.error(' remedy: exactly two options — no third "accepted with an explanation" state:'); console.error(' 1. fix the detector if this is a FALSE POSITIVE (the oracle is wrong; make it stop firing);'); console.error(' 2. file a defect and add an entry citing its issue number (REAL) — a fragment:\n'); @@ -607,12 +696,12 @@ function main() { if (staleKeys.length) { console.error(`\nqa-smell-ratchet: ${staleKeys.length} STALE baseline/fragment entr${staleKeys.length === 1 ? 'y' : 'ies'} (no longer produced by the run):\n`); for (const e of staleEntries) { - console.error(`STALE entry: ${e.key}`); - console.error(` source: ${e.source}`); - console.error(` oracle: ${e.id}`); - console.error(` scenario: ${e.scenario}`); - console.error(` issue: ${e.issue}`); - if (e.reason !== undefined) console.error(` reason: ${e.reason}`); + console.error(`STALE entry: ${flattenUntrusted(e.key)}`); + console.error(` source: ${flattenUntrusted(e.source)}`); + console.error(` oracle: ${flattenUntrusted(e.id)}`); + console.error(` scenario: ${flattenUntrusted(e.scenario)}`); + console.error(` issue: ${flattenUntrusted(e.issue)}`); + if (e.reason !== undefined) console.error(` reason: ${flattenUntrusted(e.reason)}`); } console.error('\n remedy: node scripts/qa-smell-ratchet.cjs --update'); } @@ -628,6 +717,7 @@ function main() { const md = buildStepSummaryMarkdown({ smells, violations, + expectationFailures, newKeys, staleEntries, smellSummary: reportObject.smellSummary, @@ -637,10 +727,10 @@ function main() { console.log( `\nqa-smell-ratchet: ${smells.length} smells (${newKeys.length} new, ${staleKeys.length} stale), ` - + `${violations.length} violations`, + + `${violations.length} violations, ${expectationFailures.length} expectation failures`, ); - if (sourceErrors.length || violations.length || newKeys.length || staleKeys.length) { + if (sourceErrors.length || violations.length || expectationFailures.length || newKeys.length || staleKeys.length) { throw new ExitError(1); } } @@ -651,15 +741,38 @@ function main() { function printViolations(violations) { console.error(`qa-smell-ratchet: ${violations.length} VIOLATION(s) — never acknowledgeable, always fail:\n`); for (const v of violations) { - console.error(`VIOLATION: ${v.id}`); - console.error(` scenario: ${v.scenario}`); - console.error(` argv: ${v.argv.join(' ')}`); - console.error(` detail: ${v.detail}`); - console.error(` repro: ${v.repro}`); + console.error(`VIOLATION: ${flattenUntrusted(v.id)}`); + console.error(` scenario: ${flattenUntrusted(v.scenario)}`); + console.error(` argv: ${flattenUntrusted(v.argv.join(' '))}`); + console.error(` detail: ${flattenUntrusted(v.detail)}`); + // maxLen: Infinity — a truncated repro looks runnable and isn't. + console.error(` repro: ${flattenUntrusted(v.repro, { maxLen: Infinity })}`); } } -runMain(main); +/** + * @param {ReturnType['expectationFailures']} expectationFailures + */ +function printExpectationFailures(expectationFailures) { + console.error(`qa-smell-ratchet: ${expectationFailures.length} SCENARIO EXPECTATION FAILURE(S) — never acknowledgeable, always fail:\n`); + for (const f of expectationFailures) { + console.error('EXPECTATION FAILURE:'); + console.error(` scenario: ${flattenUntrusted(f.scenario)}`); + console.error(` at: ${flattenUntrusted(f.at)}`); + console.error(` argv: ${flattenUntrusted(f.argv.join(' '))}`); + console.error(` detail: ${flattenUntrusted(f.detail)}`); + // maxLen: Infinity — a truncated repro looks runnable and isn't. + console.error(` repro: ${flattenUntrusted(f.repro, { maxLen: Infinity })}`); + } +} + +// `require()`ing this module (from `tests/loop-walk.qa.test.cjs`) must not +// trigger a real 20-scenario walk as a side effect — that's what made +// `collectFindings` untestable before #3597. Guard `runMain` so it only +// fires when this file is executed directly (`node scripts/qa-smell-ratchet.cjs`). +if (require.main === module) { + runMain(main); +} module.exports = { parseArgs, diff --git a/src/milestone.cts b/src/milestone.cts index 773a9357f..2b97f3cd2 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -749,15 +749,23 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo let totalTasks = 0; const accomplishments: string[] = []; - try { - // #3185 (ADR-3180 Decision 1): "which phase directories belong to the - // CURRENT milestone" — routed through the canonical owner (with the - // explicit `version` this command already resolved) instead of a - // hand-rolled readdirSync + isDirInMilestone filter, which also never - // excluded sentinels, unlike the owner. - const dirs = listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: version }).value; + // #3597 (ADR-3180 Decision 2): SINGLE resolution of "which phase + // directories belong to the current milestone" AND the SCOPE discriminator + // that resolution came from — shared verbatim by the read-only stats loop + // immediately below, the --dry-run preview, and the real archive pass, so + // none of the three can ever disagree. `listMilestonePhaseDirs` never + // throws (its own doc comment), so this is safe to call unguarded ahead of + // the try/catch that scopes the stats roll-up below. + // + // The stats loop's own ENUMERATION behavior is intentionally left + // unaffected by a non-COMPLETE scope — it reports what is actually on + // disk, same as before #3597. Only the destructive archive pass (and its + // --dry-run preview) refuses to act on a non-COMPLETE (non-answer) scope; + // see the guard built from `milestonePhaseScope` further down. + const { value: milestonePhaseDirs, scope: milestonePhaseScope } = listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: version }); - for (const dir of dirs) { + try { + for (const dir of milestonePhaseDirs) { phaseCount++; // #3183: canonical plan/summary sets (root+nested, superseded-excluded) // from the single owner, rather than a root-only hand-rolled readdirSync @@ -803,15 +811,41 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo * accomplishments=[] rather than crash `milestone complete`. */ } + // #3597 (ADR-3180 Decision 2): `SCOPE.UNREADABLE` is the ONE classification + // where `getMilestonePhaseFilter` throws (no workstream ROADMAP of its + // own), leaves `milestonePhaseNums` empty, and the window degrades to a + // pass-all fallback — that fallback is what silently WIDENS the archive + // set past the single-derivation guarantee this block exists to protect + // (confirmed empirically: a workstream with phase dirs but no workstream + // ROADMAP.md reports UNREADABLE and used to enumerate every directory on + // disk). `SCOPE.TRUNCATED` already refuses the WHOLE command above. + // `SCOPE.UNSCOPED` is a DIFFERENT, pre-existing classification — e.g. a + // root project with no milestone asserted in STATE.md — whose + // `listMilestonePhaseDirs` resolution is a real (non-degraded) answer, and + // its rollover archive behavior predates this branch and must not change. + // The guard therefore refuses ONLY on UNREADABLE, not on "not COMPLETE" — + // widening to every non-COMPLETE scope was itself a regression (a root + // project with no active workstream resolves UNSCOPED, and refusing to + // archive there broke the ordinary `milestone complete` -> `phases clear` + // rollover). Computed once here from the single + // `milestonePhaseDirs`/`milestonePhaseScope` resolution above, so the + // --dry-run preview below and the real archive pass further down can never + // disagree about whether (or what) to archive. + const phasesArchiveSkippedForScope = options.archivePhases !== false && milestonePhaseScope === SCOPE.UNREADABLE; + const phasesArchiveSkipReason = phasesArchiveSkippedForScope + ? `milestone window scope is "${milestonePhaseScope}" — refusing to archive phase directories until the window can be resolved (ADR-3180)` + : null; + // #2118: --dry-run preview — compute what WOULD happen without mutating. // The stats above are read-only; all mutations start at the archive section below. if (options.dryRun) { const phaseDirsToArchive: string[] = []; - if (options.archivePhases !== false) { - // #3185 (ADR-3180 Decision 1): same routed derivation as the stats loop - // above — the dry-run preview must list exactly what the real archive - // pass below would move. - phaseDirsToArchive.push(...listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: version }).value); + if (options.archivePhases !== false && !phasesArchiveSkippedForScope) { + // #3185 (ADR-3180 Decision 1) / #3597: same single routed derivation as + // the stats loop above — the dry-run preview must list exactly what + // the real archive pass below would move, including refusing to list + // anything when the window scope is not COMPLETE. + phaseDirsToArchive.push(...milestonePhaseDirs); } // #2142 MAJOR 5 (review): dry-run preview of quick-task archival — // read-only, routed through the SAME `listQuickTaskDirsForArchive` @@ -838,6 +872,8 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo ? { source: path.relative(cwd, path.join(planningBase, `${version}-MILESTONE-AUDIT.md`)).split(path.sep).join('/'), target: path.relative(cwd, path.join(archiveDir, `${version}-MILESTONE-AUDIT.md`)).split(path.sep).join('/') } : null, phases: phaseDirsToArchive, + phases_archive_skipped: phasesArchiveSkippedForScope, + phases_archive_skip_reason: phasesArchiveSkipReason, quick: quickDirsToArchive, }, would_update: { @@ -1051,7 +1087,15 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo // Archive phase directories if requested let phasesArchived = false; // #1871: archive phase dirs by default on milestone complete (opt out via --no-archive-phases). - if (options.archivePhases !== false) { + // #3597: `phasesArchiveSkippedForScope` (computed once, above, from the SAME + // `milestonePhaseDirs`/`milestonePhaseScope` resolution the --dry-run preview + // consumed) refuses this destructive rename loop entirely when the window + // scope is UNREADABLE — the one scope whose resolution degrades to a + // pass-all fallback (ADR-3180). UNSCOPED/TRUNCATED are real, pre-existing + // answers and archive exactly as they did before this branch. + // `phasesArchived` stays false and the refusal is surfaced on `result` below; + // nothing on disk moves, and `phaseArchiveDir` is never even created. + if (options.archivePhases !== false && !phasesArchiveSkippedForScope) { // #2245 audit (was ERROR-HIDING): retryRenameSync moves one phase dir at a // time — a mid-loop failure (e.g. the Nth rename) used to leave // `phasesArchived` at its `false` default even though the first N-1 dirs @@ -1064,12 +1108,12 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo const phaseArchiveDir = path.join(archiveDir, `${version}-phases`); platformEnsureDir(phaseArchiveDir); - // #3185 (ADR-3180 Decision 1): same routed derivation as the stats - // loop above — only the CURRENT milestone's phase directories move, - // never a sentinel or an out-of-window directory left for a later - // milestone. - const phaseDirNames = listMilestonePhaseDirs(phasesDir, { cwd, versionOverride: version }).value; - for (const dir of phaseDirNames) { + // #3185 (ADR-3180 Decision 1) / #3597: same single routed derivation as + // the stats loop and the --dry-run preview above — only the CURRENT + // milestone's phase directories move, never a sentinel or an + // out-of-window directory left for a later milestone, and never a + // pass-all degrade from a non-COMPLETE scope (refused above). + for (const dir of milestonePhaseDirs) { retryRenameSync(path.join(phasesDir, dir), path.join(phaseArchiveDir, dir)); archivedCount++; } @@ -1095,6 +1139,13 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo requirements: fs.existsSync(path.join(archiveDir, `${version}-REQUIREMENTS.md`)), audit: fs.existsSync(path.join(archiveDir, `${version}-MILESTONE-AUDIT.md`)), phases: phasesArchived, + // #3597: machine-readable refusal signal — distinguishes "nothing to + // archive because the milestone genuinely has no phase directories" + // (phases: false, phases_archive_skipped: false) from "refused to + // archive because the milestone window scope was not COMPLETE" + // (phases: false, phases_archive_skipped: true, with a reason). + phases_archive_skipped: phasesArchiveSkippedForScope, + phases_archive_skip_reason: phasesArchiveSkipReason, quick: !!quickArchiveResult && quickArchiveResult.archived > 0, }, milestones_updated: true, diff --git a/src/phase-locator.cts b/src/phase-locator.cts index 126eb2b81..c83483ca8 100644 --- a/src/phase-locator.cts +++ b/src/phase-locator.cts @@ -347,6 +347,11 @@ function findPhaseInternal(cwd: string, phase: unknown): PhaseSearchResult | nul * an absent `phasesDir` is a real empty (a new project genuinely has no * phases) and inherits the window's scope, whereas a `phasesDir` that exists * but cannot be read is UNREADABLE. + * + * `opts.ws` is tri-state, matching `planningDir`'s own contract: `undefined` + * (the default — do not pass `ws` at all) resolves the AMBIENT workstream + * from `GSD_WORKSTREAM`; `null` FORCES the project root regardless of any + * ambient workstream; a string forces that specific workstream. */ function listMilestonePhaseDirs( phasesDir: string, @@ -357,7 +362,17 @@ function listMilestonePhaseDirs( phaseIdConvention?: string | null; } = {}, ): { value: string[]; scope: Scope } { - const { cwd, ws = null, versionOverride = null, phaseIdConvention = null } = opts; + // #3597: `ws` must default to `undefined`, NOT `null`. `undefined` means + // "resolve the ambient workstream" (mirrors planningDir's own contract, + // src/planning-workspace.cts:124); `null` means "force the project root". + // Every cwd-bearing caller derives `phasesDir` ambiently (planningPaths(cwd) + // / planningDir(cwd) with no explicit ws), so defaulting `ws` to `null` here + // forced the milestone WINDOW to the root ROADMAP while the caller's + // `phasesDir` stayed workstream-scoped — numerator and denominator drawn + // from different scoped sets (ADR-3180 §7.6 rule 3 violation). That is what + // made `--ws progress` read `phase_scope: "unreadable"` and withhold + // `percent` once `workstream create` migrated the root ROADMAP away. + const { cwd, ws, versionOverride = null, phaseIdConvention = null } = opts; // Without a cwd there is nothing to scope AGAINST — the caller asked for an // unscoped read, which is a real answer (mirrors extractCurrentMilestoneScoped's diff --git a/tests/completion-ratio-scope-withholding.test.cjs b/tests/completion-ratio-scope-withholding.test.cjs index 538d06ca7..dd21bb7bc 100644 --- a/tests/completion-ratio-scope-withholding.test.cjs +++ b/tests/completion-ratio-scope-withholding.test.cjs @@ -804,6 +804,62 @@ describe('H. self-consistency — after state sync, STATE.md body and frontmatte }); }); +// ═════════════════════════════════════════════════════════════════════════ +// I. #3597 regression — `--ws progress` scope must come from the +// WORKSTREAM's own ROADMAP, not the root one. +// +// listMilestonePhaseDirs (src/phase-locator.cts) destructured its `ws` option +// with `= null`, an EXPLICIT "force project root" request, and forwarded that +// to getMilestonePhaseFilter -> planningDir(cwd, null) -> always the root +// .planning/ROADMAP.md — while every phasesDir passed in by callers (e.g. +// cmdProgressRender, src/commands.cts) is derived AMBIENTLY via +// planningPaths(cwd)/planningDir(cwd) with no explicit ws, i.e. already +// workstream-scoped. Once `workstream create` migrates the root ROADMAP away, +// that root read throws, scope collapses to UNREADABLE, and +// computeProgressPercent correctly (ADR-3180 §7.6 rule 4) withholds the +// percentage — but only because the numerator (workstream-scoped phase dirs) +// and the window (root-scoped) were drawn from two different scoped sets +// (rule 3 violation). The fix defaults `ws` to `undefined` so it inherits the +// same ambient GSD_WORKSTREAM resolution as phasesDir. +// ═════════════════════════════════════════════════════════════════════════ + +describe('I. workstream scoping (#3597) — --ws progress reads the WORKSTREAM roadmap, not root', () => { + test('I1: workstream with its own ROADMAP and no root ROADMAP.md -> phase_scope COMPLETE, percent numeric', (t) => { + const cwd = createTempDir('gsd-3597-i1-'); + t.after(() => cleanup(cwd)); + + // No root .planning/ROADMAP.md at all — mirrors `workstream create` + // migrating it away. Only the workstream's own planning tree exists. + const wsPlanningDir = path.join(cwd, '.planning', 'workstreams', 'alpha'); + // The workstream's own STATE.md must assert the milestone (mirrors every + // other fixture in this file via writeState) — without it, no milestone + // is asserted at all and classifyMilestoneWindow reports UNSCOPED + // regardless of the ws-forwarding fix under test here. + writeFile(cwd, '.planning/workstreams/alpha/STATE.md', ['---', 'milestone: v1.0', '---', ''].join('\n')); + writeFile(cwd, '.planning/workstreams/alpha/ROADMAP.md', [ + '## v1.0 Current 🚧', + '', + '### Phase 1: Foo', + ].join('\n')); + writeFile(cwd, '.planning/workstreams/alpha/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/workstreams/alpha/phases/01-foo/01-01-SUMMARY.md', '# Summary\n'); + assert.ok(fs.existsSync(wsPlanningDir), 'sanity: workstream planning dir was created'); + assert.ok(!fs.existsSync(path.join(planningDirOf(cwd), 'ROADMAP.md')), 'sanity: no root ROADMAP.md exists'); + + const result = runGsdTools(['--ws', 'alpha', 'progress', '--cwd', cwd, '--raw'], cwd); + assert.strictEqual(result.success, true, result.error); + const rendered = JSON.parse(result.output); + + // This is the failing-first assertion: pre-fix, `ws` defaulted to `null` + // inside listMilestonePhaseDirs, forcing the milestone window to the + // (nonexistent) root ROADMAP.md -> phase_scope: "unreadable", percent: null. + assert.strictEqual(rendered.phase_scope, SCOPE.COMPLETE); + assert.strictEqual(rendered.percent, 100); + assert.strictEqual(rendered.total_plans, 1); + assert.strictEqual(rendered.total_summaries, 1); + }); +}); + // ═════════════════════════════════════════════════════════════════════════ // The guard — narrow syntactic check considered and DROPPED (documented) // ═════════════════════════════════════════════════════════════════════════ diff --git a/tests/loop-walk.qa.test.cjs b/tests/loop-walk.qa.test.cjs index 448d40f86..508f8715e 100644 --- a/tests/loop-walk.qa.test.cjs +++ b/tests/loop-walk.qa.test.cjs @@ -16,7 +16,7 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { execFile } = require('node:child_process'); +const { execFile, spawnSync } = require('node:child_process'); const { promisify } = require('node:util'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -27,6 +27,7 @@ const { ORACLES, runOracles, SEVERITY } = require('./qa/oracles.cjs'); const { LoopWalk } = require('./qa/loop-walk.cjs'); const { MUTATIONS, apply, NOOP } = require('./qa/mutations.cjs'); const { loadScenario, runScenario, assertWiringIsLive } = require('./qa/scenario.cjs'); +const { buildReport } = require('./qa/report.cjs'); const { resolveRef } = require('./qa/fixtures/index.cjs'); const { resolveWithin, resolveForCompare } = require('./qa/paths.cjs'); const { LOOP_HOST_CONTRACT } = require('../gsd-core/bin/lib/loop-host-contract.cjs'); @@ -1084,10 +1085,13 @@ describe('scenario discovery (mutations wired for real)', () => { assert.strictEqual(names.includes('_selftest-must-fail.json'), false); }); - test('every discovered perturbation scenario applies its mutation and runs to completion without a harness crash', () => { + test('every discovered scenario holds its own expectations and runs to completion; every perturbation applies its mutation', () => { const liveCommands = [...getLiveCommandTokens()]; - const perturbationFiles = discoverScenarioFiles().filter((p) => path.basename(p).startsWith('perturbation-')); + const allFiles = discoverScenarioFiles(); + const perturbationFiles = allFiles.filter((p) => path.basename(p).startsWith('perturbation-')); + const nonPerturbationFiles = allFiles.filter((p) => !path.basename(p).startsWith('perturbation-')); assert.ok(perturbationFiles.length >= 3, 'expected at least the crlf, truncated-frontmatter, and delete-artifact scenarios'); + assert.ok(nonPerturbationFiles.length >= 1, 'expected at least one non-perturbation scenario — otherwise this widening silently narrows back to a perturbation-only loop'); // Anti-vacuity for perturbations specifically: a mutation that changes // nothing observable in ANY scenario is indistinguishable from a @@ -1099,7 +1103,13 @@ describe('scenario discovery (mutations wired for real)', () => { // over by weakening this assertion. let anyMutationObserved = false; - for (const file of perturbationFiles) { + // Widened to EVERY discovered scenario, not just perturbations: scoping + // the `expectFailures`/`report.ok` contract checks to `perturbation-*` + // was itself the #3597 blind spot in miniature — `multi-workstream`, the + // exact scenario this PR fixes, is not a perturbation scenario, so the + // old perturbation-only loop could never have seen it fail. + for (const file of allFiles) { + const isPerturbation = path.basename(file).startsWith('perturbation-'); const scenario = loadScenario(file); const report = runScenario(scenario, { LoopWalk, runOracles, liveCommands }); @@ -1112,6 +1122,28 @@ describe('scenario discovery (mutations wired for real)', () => { ); } + // A scenario reporting `ok: false` used to be invisible here — this + // loop only ever checked that the harness did not CRASH, never that + // the scenario's own declared expectations held (#3597, the test-side + // half; the gate-side half is `collectFindings` collecting + // `expectFailures` in `scripts/qa-smell-ratchet.cjs`). That blind spot + // is exactly what let `multi-workstream` fail on every CI run since + // 2026-08-10 without reddening anything. The `expectFailures` + // assertion comes first deliberately because it names WHICH + // expectation broke, whereas `report.ok` alone only says something did. + assert.deepStrictEqual( + report.steps.flatMap((s) => s.expectFailures), + [], + `${scenario.name}: a declared expectation failed — the scenario's own contract is broken`, + ); + assert.strictEqual( + report.ok, + true, + `${scenario.name}: scenario reported ok:false`, + ); + + if (!isPerturbation) continue; + const mutatedStep = report.steps.find((s) => s.mutation); assert.ok(mutatedStep, `${scenario.name}: no step recorded a mutation — mutations remain unwired`); assert.strictEqual(mutatedStep.mutationNoop, false, `${scenario.name}: mutation no-oped on a real roadmap artifact`); @@ -1334,3 +1366,320 @@ describe('worktree-concurrency (dedicated — trajectory 9 is not expressible as assert.strictEqual(jsonB.project_exists, true); }); }); + +describe('qa-smell-ratchet gate (#3597)', () => { + /** + * Build a one-scenario, one-step report object that mirrors the shape + * `tests/qa/report.cjs`'s `buildReport()` produces, so `collectFindings` + * (from `scripts/qa-smell-ratchet.cjs`) can be exercised directly without + * running a real 20-scenario walk against the CLI. Every field present on + * a real report is present here — including `totals.violations`, which + * `buildReport` computes as `violations.length + expectFailures.length` + * per step, not `violations.length` alone. + * + * @param {{expectFailures?: string[], violations?: Array<{id:string,detail:string}>, smells?: Array<{id:string,subject?:string,detail:string}>}} params + * @returns {object} a synthetic report object + */ + function reportWithStep({ expectFailures = [], violations = [], smells = [] }) { + const step = { + at: 'execute:post', + argv: ['--ws', 'alpha', 'progress'], + kind: 'json', + expectFailures, + violations, + smells, + mutation: null, + mutationNoop: false, + mutationObserved: false, + repro: 'cd /tmp/x && node gsd-core/bin/gsd-tools.cjs --ws alpha progress', + }; + return { + reportVersion: 1, + meta: { nodeVersion: 'v24.0.0', platform: 'linux', generatedAt: '2026-01-01T00:00:00.000Z' }, + totals: { + scenarios: 1, + steps: 1, + violations: expectFailures.length + violations.length, + smells: smells.length, + mutationsApplied: 0, + mutationsObserved: 0, + }, + scenarios: [ + { + name: 'synthetic-scenario', + ok: (expectFailures.length + violations.length) === 0, + fixture: 'greenfield', + steps: [step], + }, + ], + smellSummary: [], + }; + } + + /** + * Build a single step object, EXACTLY like `reportWithStep`'s inline step — + * except `expectFailures`/`violations`/`smells` are only set on `step` when + * explicitly present in `fields`. Passing `{}` produces a step where all + * three keys are ABSENT entirely (not empty arrays), which is what + * exercises `collectFindings`'s `step.violations || []` / + * `step.expectFailures || []` / `step.smells || []` defensive fallbacks — + * `reportWithStep` alone can never omit a key, since it always assigns all + * three with `= []` defaults. + * + * @param {{expectFailures?: string[], violations?: Array, smells?: Array, at?: string, argv?: string[]}} [fields] + * @returns {object} + */ + function buildStep(fields = {}) { + const step = { + at: fields.at || 'execute:post', + argv: fields.argv || ['--ws', 'alpha', 'progress'], + kind: 'json', + mutation: null, + mutationNoop: false, + mutationObserved: false, + repro: 'cd /tmp/x && node gsd-core/bin/gsd-tools.cjs --ws alpha progress', + }; + if (fields.expectFailures !== undefined) step.expectFailures = fields.expectFailures; + if (fields.violations !== undefined) step.violations = fields.violations; + if (fields.smells !== undefined) step.smells = fields.smells; + return step; + } + + /** + * Build a full synthetic report spanning one or more scenarios (each with + * its own, possibly empty, `steps` array) — a sibling to `reportWithStep` + * for shapes it cannot express (multiple scenarios; zero-step scenarios). + * `totalViolations` is supplied explicitly by the caller so each PARITY + * case can assert against the exact combined count it intends + * `report.totals.violations` to carry, mirroring how `buildReport()` + * computes that field for real reports. + * + * @param {Array<{name: string, steps: object[]}>} scenarioDefs + * @param {number} totalViolations + * @returns {object} + */ + function reportWithScenarios(scenarioDefs, totalViolations) { + return { + reportVersion: 1, + meta: { nodeVersion: 'v24.0.0', platform: 'linux', generatedAt: '2026-01-01T00:00:00.000Z' }, + totals: { + scenarios: scenarioDefs.length, + steps: scenarioDefs.reduce((sum, s) => sum + s.steps.length, 0), + violations: totalViolations, + smells: 0, + mutationsApplied: 0, + mutationsObserved: 0, + }, + scenarios: scenarioDefs.map((s) => ({ + name: s.name, + ok: true, + fixture: 'greenfield', + steps: s.steps, + })), + smellSummary: [], + }; + } + + /** + * Build a report via the REAL `buildReport()` (tests/qa/report.cjs) rather + * than hand-computing `totals.violations`. Every other PARITY case above + * hardcodes `expectFailures.length + violations.length` itself — the same + * formula `buildReport` uses internally — so a change to `buildReport`'s + * own violation-counting formula would leave those cases green while the + * gate silently drifted from the report it reads (exactly the #3597 + * defect class). Routing at least one case through the real builder is + * what makes this test capable of catching that drift. + * + * @param {Array<{name: string, steps: Array<{at?: string, argv?: string[], kind?: string, oracleFailures?: object[], expectFailures?: string[], smells?: object[]}>}>} scenarioDefs + * @returns {object} a real report document produced by `buildReport()` + */ + function realBuiltReport(scenarioDefs) { + const scenarioReports = scenarioDefs.map((s) => ({ + name: s.name, + ok: true, + fixture: 'greenfield', + steps: s.steps.map((step) => ({ + at: step.at || 'execute:post', + argv: step.argv || ['--ws', 'alpha', 'progress'], + kind: step.kind || 'json', + expectFailures: step.expectFailures || [], + oracleFailures: step.oracleFailures || [], + smells: step.smells || [], + mutation: null, + mutationNoop: false, + mutationObserved: false, + })), + })); + return buildReport(scenarioReports, { + nodeVersion: 'v24.0.0', + platform: 'linux', + generatedAt: '2026-01-01T00:00:00.000Z', + }); + } + + test('requiring the ratchet does NOT run a full walk — proven by running require() to completion in a CHILD PROCESS (#3597)', () => { + // WHY AN IN-PROCESS ELAPSED-TIME CHECK IS INSUFFICIENT (and banned): + // `runMain` (`scripts/lib/cli-exit.cjs:38`) defers through + // `Promise.resolve().then(() => main())` — a microtask — so a synchronous + // `require()` call ALWAYS returns before `main()` has had any chance to + // run, REGARDLESS of whether the `require.main === module` guard exists + // at all. Proven with a synthetic module whose `main()` blocks for + // 4000ms: `require()` of it still returns in ~2ms. Both "typeof + // collectFindings === 'function'" and "elapsed < Nms" pass identically + // against the UNGUARDED file, so an in-process timing assertion proves + // nothing about the guard — it is vacuous. It is also a wall-clock + // assertion, which CLAUDE.md's test rules ban outright ("Clock Seams: Do + // not assert on wall-clock time"). + // + // WHY A CHILD PROCESS IS THE ONLY THING THAT ACTUALLY OBSERVES THE GUARD: + // spawning `node -e "require()"` and letting the event loop drain + // to natural completion (rather than measuring how fast `require()` + // returns) lets whatever `runMain` scheduled actually run. Loaded via + // `-e`, the required module's own `module` object is never + // `require.main` (that identity belongs to the `-e` pseudo-module), so a + // genuinely guarded file never calls `runMain(main)` and its process + // never prints `main()`'s "qa-smell-ratchet: ..." summary line. Against + // the pre-#3597 unguarded shape, `runMain(main)` always fires and that + // line DOES appear in the child's output. That presence/absence is the + // only thing that actually distinguishes guarded from unguarded. + const scriptPath = path.join(__dirname, '..', 'scripts', 'qa-smell-ratchet.cjs'); + const repoRoot = path.join(__dirname, '..'); + const result = spawnSync( + process.execPath, + ['-e', `require(${JSON.stringify(scriptPath)})`], + { cwd: repoRoot, timeout: 120000, encoding: 'utf-8' }, + ); + assert.strictEqual( + result.status, + 0, + `child process exited non-zero (status=${result.status}): stdout=${result.stdout} stderr=${result.stderr}`, + ); + const combined = `${result.stdout || ''}${result.stderr || ''}`; + // This is the discriminating assertion: `jsonOut` defaults to `null` in + // `parseArgs` (scripts/qa-smell-ratchet.cjs), so no report is written + // even under the unguarded pre-fix shape run via `node -e` — asserting + // the ABSENCE of qa-report.json would be vacuously true either way. The + // presence/absence of the "qa-smell-ratchet:" summary line is the only + // thing that actually distinguishes a guarded require() from an + // unguarded one (see the WHY comments above). + assert.strictEqual( + combined.includes('qa-smell-ratchet:'), + false, + 'require()ing the ratchet printed main()\'s summary line in a child process — the require.main guard did not prevent a full walk', + ); + + // `collectFindings` remains reachable in-process, as documented. + const ratchet = require('../scripts/qa-smell-ratchet.cjs'); + assert.strictEqual(typeof ratchet.collectFindings, 'function'); + }); + + test('collectFindings surfaces a scenario expectation failure — a failing scenario must reach the gate', () => { + const { collectFindings } = require('../scripts/qa-smell-ratchet.cjs'); + // This is the #3597 defect: `multi-workstream` failed this exact way on + // every CI run from 2026-08-10 onward while the ratchet printed + // "0 violations" and exited 0 — because `collectFindings` never read + // `step.expectFailures` at all. + const report = reportWithStep({ expectFailures: ['path "percent": expected 100, got null'] }); + const found = collectFindings(report); + assert.strictEqual(found.expectationFailures.length, 1); + const [entry] = found.expectationFailures; + assert.strictEqual(entry.scenario, 'synthetic-scenario'); + assert.strictEqual(entry.at, 'execute:post'); + assert.strictEqual(entry.detail, 'path "percent": expected 100, got null'); + assert.deepStrictEqual(entry.argv, ['--ws', 'alpha', 'progress']); + assert.ok(entry.repro); + }); + + test('an expectation failure is NOT reported as an oracle violation (the two stay distinguishable)', () => { + const { collectFindings } = require('../scripts/qa-smell-ratchet.cjs'); + const report = reportWithStep({ expectFailures: ['path "percent": expected 100, got null'] }); + const found = collectFindings(report); + // Tied to the new bucket: pre-fix, `collectFindings` returned + // `{smells: [], violations: []}` for this exact input (it never read + // `step.expectFailures` at all), so both `deepStrictEqual([])` arms below + // passed unchanged whether the fix was present or not. Asserting the + // expectation failure was actually collected is what makes this test + // capable of failing against that pre-fix shape. + assert.strictEqual(found.expectationFailures.length, 1); + assert.deepStrictEqual(found.violations, []); + assert.deepStrictEqual(found.smells, []); + }); + + test('an expectation failure is never fingerprinted, so it can never be baselined or acked away', () => { + const { collectFindings } = require('../scripts/qa-smell-ratchet.cjs'); + const report = reportWithStep({ expectFailures: ['path "percent": expected 100, got null'] }); + const found = collectFindings(report); + for (const failure of found.expectationFailures) { + assert.strictEqual( + failure.key, + undefined, + 'an expectation failure must not carry a fingerprint `key` — a key would make it acknowledgeable via smell-baseline.json / smell-acks/, letting a real scenario failure be silenced like a smell', + ); + } + }); + + test('PARITY: the gate counts exactly what report.totals.violations counts', () => { + // The #3597 root cause was a silent disagreement between these two + // counters: `report.totals.violations` (built by `buildReport`) already + // counted `expectFailures`, but `collectFindings` (consumed by the gate) + // did not — so the gate under-counted relative to the report it was + // reading. + const { collectFindings } = require('../scripts/qa-smell-ratchet.cjs'); + const cases = [ + reportWithStep({}), + reportWithStep({ expectFailures: ['a'] }), + reportWithStep({ violations: [{ id: 'exit-contract', detail: 'boom' }] }), + reportWithStep({ expectFailures: ['a', 'b'], violations: [{ id: 'exit-contract', detail: 'boom' }] }), + // A step whose expectFailures/violations/smells keys are ABSENT + // entirely (not empty arrays) — exercises collectFindings's `|| []` + // defensive fallbacks. + reportWithScenarios([{ name: 'absent-keys-scenario', steps: [buildStep({})] }], 0), + // Two scenarios, each contributing at least one expectation failure + // and/or violation, with totals.violations set to the true combined + // count across BOTH scenarios. + reportWithScenarios( + [ + { + name: 'scenario-one', + steps: [ + buildStep({ expectFailures: ['a'] }), + buildStep({ violations: [{ id: 'exit-contract', detail: 'boom' }] }), + ], + }, + { name: 'scenario-two', steps: [buildStep({ expectFailures: ['b', 'c'] })] }, + ], + 4, + ), + // A scenario whose steps array is EMPTY. + reportWithScenarios([{ name: 'empty-steps-scenario', steps: [] }], 0), + // Built via the REAL buildReport() (see realBuiltReport's doc comment + // above for why this case, specifically, is load-bearing). + realBuiltReport([{ name: 'real-scenario', steps: [{ expectFailures: ['a'] }] }]), + realBuiltReport([ + { + name: 'real-scenario-one', + steps: [ + { expectFailures: ['a'] }, + { oracleFailures: [{ id: 'exit-contract', detail: 'boom' }] }, + ], + }, + { name: 'real-scenario-two', steps: [{ expectFailures: ['b', 'c'] }] }, + ]), + ]; + for (const report of cases) { + const found = collectFindings(report); + assert.strictEqual( + found.violations.length + found.expectationFailures.length, + report.totals.violations, + ); + } + }); + + test('a clean report yields no findings at all (anti-vacuity: the assertions above can pass honestly)', () => { + const { collectFindings } = require('../scripts/qa-smell-ratchet.cjs'); + const found = collectFindings(reportWithStep({})); + assert.deepStrictEqual(found.violations, []); + assert.deepStrictEqual(found.expectationFailures, []); + assert.deepStrictEqual(found.smells, []); + }); +}); diff --git a/tests/milestone-archive.test.cjs b/tests/milestone-archive.test.cjs index d02b08fdc..c0ecc3c7e 100644 --- a/tests/milestone-archive.test.cjs +++ b/tests/milestone-archive.test.cjs @@ -17,6 +17,7 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); const { createTempProject, cleanup, runGsdTools, toPosixPath } = require('./helpers.cjs'); +const { seedWorkstream } = require('./fixtures/index.cjs'); const { findTableBySchema } = require('../gsd-core/bin/lib/markdown-table.cjs'); const { buildQuickArchiveIndex } = require('../gsd-core/bin/lib/milestone.cjs'); @@ -1191,3 +1192,163 @@ describe('#2142 review: README injection, symlink escape, dry-run/real-run parit } }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3597: milestone complete refuses to archive phase directories when the +// milestone window's scope is not SCOPE.COMPLETE (ADR-3180 "a non-answer must +// not be acted on"). Regression coverage for the specific widening #3597 +// introduced when listMilestonePhaseDirs stopped forcing `ws: null` — a +// workstream with phase directories but NO workstream-local ROADMAP.md now +// resolves its milestone window against the ACTIVE workstream (fixing --ws +// progress), but getMilestonePhaseFilter throws internally when it cannot +// read that workstream's ROADMAP.md, degrading scope to SCOPE.UNREADABLE with +// a pass-all directory fallback. Before this guard, `milestone complete` +// archived every phase directory on disk in that shape; after it, the archive +// step refuses and reports why, while the surrounding command (ROADMAP/ +// REQUIREMENTS archival, STATE.md closure) still completes — matching the +// pre-existing UNREADABLE/UNSCOPED "legitimately handled" posture documented +// at the TRUNCATED-only whole-command refusal above it in src/milestone.cts. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3597: milestone complete refuses to archive on a non-COMPLETE window scope', () => { + let tmpDir; + + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + function seedUnreadableWorkstream(cwd) { + // Root ROADMAP.md declares only phase 1 — irrelevant to the workstream's + // OWN window once the workstream is active, but included to mirror the + // exact reproduction shape (a root ROADMAP that could otherwise mislead a + // naive root-scoped read). + fs.writeFileSync( + path.join(cwd, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n### Phase 1: Root\n\n**Goal:** Root-only work.\n', + ); + // Workstream `alpha`: STATE.md declares milestone v1.0, but NO + // ROADMAP.md of its own — this is what makes getMilestonePhaseFilter + // throw internally and degrade to SCOPE.UNREADABLE for this workstream's + // window. + seedWorkstream(cwd, { + name: 'alpha', + state: '---\nmilestone: v1.0\n---\n\n# GSD State\n', + active: true, + }); + const alphaPhases = path.join(cwd, '.planning', 'workstreams', 'alpha', 'phases'); + for (const dir of ['01-a', '02-b', '03-c']) { + fs.mkdirSync(path.join(alphaPhases, dir), { recursive: true }); + } + return alphaPhases; + } + + test('archives NOTHING and leaves every phase dir on disk when the workstream has no ROADMAP.md', () => { + const alphaPhases = seedUnreadableWorkstream(tmpDir); + + const result = runSdkQuery(['milestone.complete', 'v1.0'], tmpDir); + assert.ok(result.success, `milestone.complete should still succeed (UNREADABLE is not a whole-command refusal): ${result.error}`); + + assert.strictEqual(result.data.archived.phases, false, 'phases must NOT be reported as archived'); + assert.strictEqual(result.data.archived.phases_archive_skipped, true, 'the refusal must be surfaced as machine-readable'); + assert.ok( + typeof result.data.archived.phases_archive_skip_reason === 'string' + && result.data.archived.phases_archive_skip_reason.length > 0, + `expected a non-empty skip reason, got: ${JSON.stringify(result.data.archived.phases_archive_skip_reason)}`, + ); + + const onDisk = fs.readdirSync(alphaPhases, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name) + .sort(); + assert.deepStrictEqual(onDisk, ['01-a', '02-b', '03-c'], 'all three phase directories must still be on disk, untouched'); + + // Negative proof: no phase-archive directory was even created. + assert.strictEqual( + fs.existsSync(path.join(tmpDir, '.planning', 'workstreams', 'alpha', 'milestones', 'v1.0-phases')), + false, + 'the archive destination must never be created on a refused archive pass', + ); + }); + + test('--dry-run previews an empty archive list and the same refusal on the unreadable-window workstream', () => { + seedUnreadableWorkstream(tmpDir); + + const result = runSdkQuery(['milestone.complete', 'v1.0', '--dry-run'], tmpDir); + assert.ok(result.success, `milestone.complete --dry-run should succeed: ${result.error}`); + assert.deepStrictEqual(result.data.would_archive.phases, [], 'dry-run must preview an EMPTY archive list'); + assert.strictEqual(result.data.would_archive.phases_archive_skipped, true); + assert.ok( + typeof result.data.would_archive.phases_archive_skip_reason === 'string' + && result.data.would_archive.phases_archive_skip_reason.length > 0, + ); + }); + + test('the guard is not a blanket refusal — a normal COMPLETE-scope workstream still archives exactly its in-window phase dirs', () => { + // Workstream `beta` HAS its own ROADMAP.md declaring phase 1 only — a + // real, resolvable (SCOPE.COMPLETE) window. `02-out-of-window` has no + // matching ROADMAP entry and must NOT be archived, proving this exercises + // real window scoping and not merely "archive everything present". + seedWorkstream(tmpDir, { + name: 'beta', + state: '---\nmilestone: v1.0\n---\n\n# GSD State\n', + // #3597: a versioned `## v1.0 ...` heading is required for the window + // to resolve SCOPE.COMPLETE against the explicit `version` argument — + // a free-form roadmap (no versioned heading at all) resolves UNSCOPED + // instead once an explicit version is requested (verified empirically + // against the built CLI), which would silently defeat this "guard is + // not a blanket refusal" proof. + roadmap: '# Roadmap\n\n## v1.0 Current\n\n### Phase 1: Foo\n\n**Goal:** Do foo.\n', + active: true, + }); + const betaPhases = path.join(tmpDir, '.planning', 'workstreams', 'beta', 'phases'); + fs.mkdirSync(path.join(betaPhases, '01-foo'), { recursive: true }); + fs.mkdirSync(path.join(betaPhases, '02-out-of-window'), { recursive: true }); + + const result = runSdkQuery(['milestone.complete', 'v1.0'], tmpDir); + assert.ok(result.success, `milestone.complete should succeed: ${result.error}`); + + assert.strictEqual(result.data.archived.phases, true, 'phases must be reported as archived'); + assert.strictEqual(result.data.archived.phases_archive_skipped, false, 'a resolvable (COMPLETE) window must not be reported as skipped'); + assert.strictEqual(result.data.archived.phases_archive_skip_reason, null); + + const archiveDir = path.join(tmpDir, '.planning', 'workstreams', 'beta', 'milestones', 'v1.0-phases'); + assert.ok(fs.existsSync(path.join(archiveDir, '01-foo')), 'the in-window phase dir must be archived'); + assert.ok(!fs.existsSync(path.join(archiveDir, '02-out-of-window')), 'the out-of-window phase dir must NOT be archived'); + assert.ok(fs.existsSync(path.join(betaPhases, '02-out-of-window')), 'the out-of-window phase dir must remain on disk, untouched'); + }); + + // #3597 regression: the guard originally shipped as "refuse whenever scope + // !== SCOPE.COMPLETE", which also caught SCOPE.UNSCOPED — a DIFFERENT, + // pre-existing classification whose archive behavior predates this branch. + // A root project (no active workstream) with a free-form ROADMAP.md (no + // versioned `## vX.Y` heading) resolves UNSCOPED once an explicit version + // is requested — exactly the `milestone-rollover` QA scenario shape + // (tests/qa/scenarios/milestone-rollover.json, fixture "greenfield": + // .planning/ROADMAP.md from @roadmap/three-phase, no workstreams at all). + // Under the too-broad guard, `milestone complete 1.0 --force` refused to + // archive `01-parser`, leaving it on disk and causing the QA walk's + // following `phases clear --confirm` step to abort on the #1447 + // uncommitted-change safety check. Narrowing the guard to UNREADABLE-only + // must restore this exact rollover: the phase directories archive. + test('a root project with an unscoped (non-versioned) roadmap still archives phase dirs like before the guard', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n### Phase 1: Parser\n**Goal:** Parse input.\n\n### Phase 2: Printable Output\n**Goal:** Render output.\n', + ); + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + fs.mkdirSync(path.join(phasesDir, '01-parser'), { recursive: true }); + fs.mkdirSync(path.join(phasesDir, '02-printable-output'), { recursive: true }); + + const result = runSdkQuery(['milestone.complete', 'v1.0', '--force'], tmpDir); + assert.ok(result.success, `milestone.complete should succeed: ${result.error}`); + + assert.strictEqual(result.data.archived.phases, true, 'phases must still be archived for an UNSCOPED (not UNREADABLE) window'); + assert.strictEqual(result.data.archived.phases_archive_skipped, false, 'UNSCOPED must not trigger the refusal — only UNREADABLE does'); + assert.strictEqual(result.data.archived.phases_archive_skip_reason, null); + + const archiveDir = path.join(tmpDir, '.planning', 'milestones', 'v1.0-phases'); + assert.ok(fs.existsSync(path.join(archiveDir, '01-parser')), '01-parser must be archived'); + assert.ok(fs.existsSync(path.join(archiveDir, '02-printable-output')), '02-printable-output must be archived'); + assert.ok(!fs.existsSync(path.join(phasesDir, '01-parser')), '01-parser must no longer be on disk at its original location'); + assert.ok(!fs.existsSync(path.join(phasesDir, '02-printable-output')), '02-printable-output must no longer be on disk at its original location'); + }); +}); diff --git a/tests/qa/report.cjs b/tests/qa/report.cjs index 37c56fdf3..5ae3505a5 100644 --- a/tests/qa/report.cjs +++ b/tests/qa/report.cjs @@ -21,12 +21,14 @@ * different documents, which defeats diffing/reviewing a report in CI. * * REPRO LINES ARE ALWAYS STRINGS: `step.repro` is either a real, - * copy-pasteable `cd && node gsd-core/bin/gsd-tools.cjs ...` command, - * or a string clearly prefixed `NOT RUNNABLE: ...` explaining why (the tree - * was not preserved, or the step declared no CLI invocation at all). A - * repro line that *looks* runnable but points at a directory that was - * already deleted is worse than no repro line, so the two cases are never - * conflated into one shape that "sometimes has a command". + * copy-pasteable `cd && node ...` + * command (plus a trailing `#` shell-comment note about the pinned clock and + * cleared env — see `buildRepro`), or a string clearly prefixed + * `NOT RUNNABLE: ...` explaining why (the tree was not preserved, or the + * step declared no CLI invocation at all). A repro line that *looks* + * runnable but points at a directory that was already deleted is worse than + * no repro line, so the two cases are never conflated into one shape that + * "sometimes has a command". */ const fs = require('node:fs'); @@ -38,6 +40,12 @@ const REPORT_VERSION = 1; /** * Build a single copy-pasteable repro command/explanation for one step. * + * The emitted `node` path is ABSOLUTE (resolved from this file's own + * location via `__dirname`), never the repo-relative + * `gsd-core/bin/gsd-tools.cjs` — `preservedDir` is a temp project directory + * unrelated to the repo checkout, so `cd`ing into it first and then + * resolving a repo-relative path throws `MODULE_NOT_FOUND` (#3597). + * * @param {{preservedDir?: string, argv: string[]}} params * @returns {string} */ @@ -48,7 +56,14 @@ function buildRepro({ preservedDir, argv }) { if (!preservedDir) { return 'NOT RUNNABLE: the scenario tree was not preserved for this run — re-run with `--keep` (or `GSD_QA_KEEP=1`) to get a reproducible command.'; } - return `cd ${preservedDir} && node gsd-core/bin/gsd-tools.cjs --json-errors ${argv.join(' ')}`; + const gsdToolsPath = path.resolve(__dirname, '..', '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); + // The trailing `# ...` is a shell comment, not part of the command: pasting + // the whole line (including the note) into a shell still runs correctly, + // since everything from `#` to end-of-line is ignored. This keeps the + // string a single copy-pasteable line while telling a human reader the + // walk also pins a clock and clears ambient env vars this repro cannot. + return `cd ${preservedDir} && node ${gsdToolsPath} --json-errors ${argv.join(' ')}` + + ' # note: the walk also pins GSD_NOW_MS and clears ambient GSD_* vars, so results may differ slightly'; } /** diff --git a/tests/qa/scenarios/multi-workstream.json b/tests/qa/scenarios/multi-workstream.json index 205f95d7e..4b85da4a5 100644 --- a/tests/qa/scenarios/multi-workstream.json +++ b/tests/qa/scenarios/multi-workstream.json @@ -1,7 +1,7 @@ { "name": "multi-workstream", "fixture": "greenfield", - "notes": "The point of this scenario is workstream isolation: create two workstreams, populate only one, and prove progress stays isolated per-`--ws` in both directions (populated vs empty, and switching back and forth). No payload field names the active workstream, so monotonic-progress's oracle derives scope from the invocation's own `--ws ` argv token — a workstream switch is a scope change, so an apparent counter reset when switching to an untouched workstream is a SMELL, never a VIOLATION. Do not trim/reorder these steps to dodge the oracle — the isolation crossing IS the scenario.", + "notes": "The point of this scenario is workstream isolation: create two workstreams, populate only one, and prove progress stays isolated per-`--ws` in both directions (populated vs empty, and switching back and forth). No payload field names the active workstream, so monotonic-progress's oracle derives scope from the invocation's own `--ws ` argv token — a workstream switch is a scope change, so an apparent counter reset when switching to an untouched workstream is a SMELL, never a VIOLATION. Do not trim/reorder these steps to dodge the oracle — the isolation crossing IS the scenario. The final step asserts a real percentage because the workstream's own ROADMAP (`.planning/workstreams/alpha/ROADMAP.md`) scopes the milestone window for `--ws alpha`; `phase_scope` is asserted alongside `percent` so a regression to the #3597 root/workstream scope mismatch — which made `listMilestonePhaseDirs` read the milestone window against the ROOT ROADMAP while counting phase directories from the WORKSTREAM's own phases dir, reporting `phase_scope: unreadable` and withholding `percent` — is caught immediately.", "steps": [ { "at": "discuss:post", @@ -58,6 +58,7 @@ "expect": [ { "path": "total_plans", "is": 1 }, { "path": "total_summaries", "is": 1 }, + { "path": "phase_scope", "is": "complete" }, { "path": "percent", "is": 100 } ] },