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 } ] },