diff --git a/.changeset/3518-uat-path-phase-pinned.md b/.changeset/3518-uat-path-phase-pinned.md new file mode 100644 index 000000000..91b56d2b0 --- /dev/null +++ b/.changeset/3518-uat-path-phase-pinned.md @@ -0,0 +1,6 @@ +--- +type: Fixed +pr: 3525 +--- + +**`uat_path` is now pinned to the phase's own UAT artifact instead of being picked by unsorted directory-listing order** — both `uat_path` projections (`init plan-phase` and `init phase-op`) selected the phase's `*-UAT.md` with a bare first-match `.find()` that had no phase-membership check and no ordering, so a stray or cross-phase `04-UAT.md` sitting in phase 03's directory could become phase 03's `uat_path`, and which file won was filesystem-dependent (creation order on APFS, hash order on ext4/XFS) — meaning two machines on the same commit could emit different `uat_path` values for the same phase, sending downstream workflows to read another phase's UAT state. Both sites now route through a shared phase-pinned resolver (`resolveUatFile`, sibling of the `resolveVerificationFile` rule from #3357/#3492): the phase's own `-UAT.md` always wins, otherwise the alphabetically-first dashed candidate, deterministically on every machine. (#3518) diff --git a/src/init.cts b/src/init.cts index 52f541084..b33cdadbc 100644 --- a/src/init.cts +++ b/src/init.cts @@ -103,7 +103,7 @@ const { const { determinePhaseStatus } = commandsMod; const { extractFrontmatter } = frontmatterMod; -const { isPhaseComplete, resolveVerificationFile } = verificationMod; +const { isPhaseComplete, resolveVerificationFile, resolveUatFile } = verificationMod; const { evaluateUatPassed } = uatPredicateMod; const { resolveLoopHooks } = loopResolverMod; const { loadRegistry } = capabilityLoaderMod; @@ -1151,14 +1151,23 @@ function cmdInitPlanPhase( // #3492: pin selection to THIS phase's own token so a stray cross-phase // or sentinel-numbered canonically-shaped file cannot outrank this // phase's own (possibly non-canonical) report. + const phaseToken = extractPhaseToken(path.basename(phaseDirFull)); const verificationFile = resolveVerificationFile(files, { allowBare: true, - phaseToken: extractPhaseToken(path.basename(phaseDirFull)), + phaseToken, }); if (verificationFile) { result['verification_path'] = toPosixPath(path.join(phaseDirFull, verificationFile)); } - const uatFile = files.find((f) => f.endsWith('-UAT.md') || f === 'UAT.md'); + // #3518: routed through the shared UAT resolver — the prior hand-rolled + // `.find()` over unsorted readdir order had no phase check and no + // ordering, so a stray cross-phase 02-UAT.md could become this phase's + // uat_path, filesystem-dependently. Pinned to this phase's own token + // (same rule as verification_path above). + const uatFile = resolveUatFile(files, { + allowBare: true, + phaseToken, + }); if (uatFile) { result['uat_path'] = toPosixPath(path.join(phaseDirFull, uatFile)); } @@ -1984,14 +1993,23 @@ function cmdInitPhaseOp(cwd: string, phase: string, raw: boolean): void { // #3492: pin selection to THIS phase's own token so a stray cross-phase // or sentinel-numbered canonically-shaped file cannot outrank this // phase's own (possibly non-canonical) report. + const phaseToken = extractPhaseToken(path.basename(phaseDirFull)); const verificationFile = resolveVerificationFile(files, { allowBare: true, - phaseToken: extractPhaseToken(path.basename(phaseDirFull)), + phaseToken, }); if (verificationFile) { result['verification_path'] = toPosixPath(path.join(phaseDirFull, verificationFile)); } - const uatFile = files.find((f) => f.endsWith('-UAT.md') || f === 'UAT.md'); + // #3518: routed through the shared UAT resolver — the prior hand-rolled + // `.find()` over unsorted readdir order had no phase check and no + // ordering, so a stray cross-phase 02-UAT.md could become this phase's + // uat_path, filesystem-dependently. Pinned to this phase's own token + // (same rule as verification_path above). + const uatFile = resolveUatFile(files, { + allowBare: true, + phaseToken, + }); if (uatFile) { result['uat_path'] = toPosixPath(path.join(phaseDirFull, uatFile)); } diff --git a/src/verification.cts b/src/verification.cts index ead8315cf..ac5588b67 100644 --- a/src/verification.cts +++ b/src/verification.cts @@ -315,6 +315,61 @@ interface ResolveVerificationFileOptions { phaseToken?: string; } +/** + * #3518: `resolveUatFile`'s options — same two knobs, same semantics, as + * `ResolveVerificationFileOptions` above (the UAT artifact is selected by the + * identical phase-pinned rule the verification report is; see + * `resolvePhaseArtifactFile` below for the single shared selection core). + */ +type ResolveUatFileOptions = ResolveVerificationFileOptions; + +/** + * #3518: the shared phase-pinned artifact-selection core BOTH single-pick + * resolvers (`resolveVerificationFile` for `*-VERIFICATION.md`, + * `resolveUatFile` for `*-UAT.md`) delegate to — one rule, not two grammars + * that agree today and drift tomorrow (epic #3473 F2's defect class). + * + * `bareName` is the artifact filename WITHOUT the leading dash (`'UAT.md'`); + * a "dashed" candidate is any entry ending `-${bareName}`. + * + * Selection order (identical to resolveVerificationFile's documented tiers, + * generalized off the suffix): + * 1. `options.phaseToken` given and `-${bareName}` is among + * the candidates — that exact file always wins: it is THIS phase's own + * artifact, and no other candidate (whichever phase's token it carries) + * can outrank it (#3492 / #3518). + * 2. Fallback — no exact phase-token match (or no token given): + * alphabetically first of ALL dashed candidates (deterministic — + * `entries` is an unsorted readdir listing whose order is + * filesystem-dependent, so the sort is what makes the answer + * machine-independent). Load-bearing: a phase whose only artifact is + * non-canonically named must keep resolving to it, not to null — the + * fix must not turn "found an artifact" into "found nothing". + * 3. `options.allowBare` only — a bare `${bareName}`, ranked BELOW both of + * the above (a dashed file names its phase; a bare one does not). + * Reached only when (1) and (2) found no dashed candidate at all. + * + * Pure — takes an already-read directory listing and does no I/O of its own, + * so every call site keeps its existing `fsImpl` seam and no-throw contract + * untouched. + */ +function resolvePhaseArtifactFile( + entries: string[], + bareName: string, + options: ResolveVerificationFileOptions = {}, +): string | null { + const candidates = entries.filter((f) => f.endsWith(`-${bareName}`)).sort(); + if (candidates.length > 0) { + if (options.phaseToken) { + const thisPhaseFile = `${options.phaseToken}-${bareName}`; + if (candidates.includes(thisPhaseFile)) return thisPhaseFile; + } + return candidates[0]; + } + if (options.allowBare && entries.includes(bareName)) return bareName; + return null; +} + /** * Resolve which `*-VERIFICATION.md` entry in a phase directory's listing IS * the phase's verification report, when more than one such file exists. @@ -328,40 +383,41 @@ interface ResolveVerificationFileOptions { * (findStaleVerificationSummary and readVerificationStatus) — this is the * single resolver both now call (#3473 F2). * - * Selection order: - * 1. `options.phaseToken` given and `-VERIFICATION.md` is among - * the candidates — that exact file always wins. This is THIS phase's own - * report; no other candidate (canonically-shaped or not, whichever - * phase's token it carries) can outrank it (#3492). - * 2. Fallback — no exact phase-token match (or no token given): alphabetically - * first of ALL `*-VERIFICATION.md` entries, unchanged from the original - * pre-#3357 behavior. Load-bearing: a phase whose only report is - * non-canonically named must keep resolving to it, not to null — this - * fix must not turn "found a report" into "found nothing" for anyone. - * 3. `options.allowBare` only — a bare `VERIFICATION.md`, ranked BELOW both - * of the above. Rationale: a dashed file names its phase, a bare one - * does not, so a dashed file (canonical or not) is always the better - * answer when both exist. Reached only when neither (1) nor (2) found - * any dashed candidate at all. - * - * Pure — takes an already-read directory listing and does no I/O of its own, - * so every call site keeps its existing `fsImpl` seam and no-throw contract - * untouched. + * Selection order: see `resolvePhaseArtifactFile` (the shared core this + * delegates to since #3518) — phase-token-pinned, then alphabetically-first + * dashed fallback, then (allowBare only) a bare `VERIFICATION.md`. Behavior + * is byte-identical to the pre-#3518 standalone implementation. */ function resolveVerificationFile( entries: string[], options: ResolveVerificationFileOptions = {}, ): string | null { - const candidates = entries.filter((f) => f.endsWith('-VERIFICATION.md')).sort(); - if (candidates.length > 0) { - if (options.phaseToken) { - const thisPhaseFile = `${options.phaseToken}-VERIFICATION.md`; - if (candidates.includes(thisPhaseFile)) return thisPhaseFile; - } - return candidates[0]; - } - if (options.allowBare && entries.includes('VERIFICATION.md')) return 'VERIFICATION.md'; - return null; + return resolvePhaseArtifactFile(entries, 'VERIFICATION.md', options); +} + +/** + * #3518: resolve which `*-UAT.md` entry in a phase directory's listing IS + * the phase's UAT artifact, when more than one such file exists — the UAT + * counterpart of `resolveVerificationFile`, sharing its exact selection rule + * via `resolvePhaseArtifactFile`. + * + * The bug this closes: both `uat_path` projectors in `src/init.cts` picked + * with a bare `.find((f) => f.endsWith('-UAT.md') || f === 'UAT.md')` over an + * unsorted `readdir` listing — no phase-membership check and no ordering — so + * a stray or cross-phase `04-UAT.md` sitting in phase 03's directory could + * become phase 03's `uat_path`, and WHICH file won was filesystem-dependent + * (creation order on APFS, hash order on ext4/XFS): two machines on the same + * commit could emit different `uat_path` values for the same phase. `uat_path` + * is consumed downstream by workflows that then read the named file, so a + * wrong path routes UAT state from another phase. + * + * Deterministic by construction: same answer on every machine. + */ +function resolveUatFile( + entries: string[], + options: ResolveUatFileOptions = {}, +): string | null { + return resolvePhaseArtifactFile(entries, 'UAT.md', options); } // ─── Public API ─────────────────────────────────────────────────────────────── @@ -735,6 +791,7 @@ export = { VERIFICATION_ROUTING_TABLE, defaultPhaseCleanCommitTimesMs, resolveVerificationFile, + resolveUatFile, findStaleVerificationSummary, readVerificationStatus, isPhaseComplete, diff --git a/tests/init.test.cjs b/tests/init.test.cjs index becc78743..5c08f8510 100644 --- a/tests/init.test.cjs +++ b/tests/init.test.cjs @@ -143,6 +143,31 @@ describe('init commands', () => { ); }); + // #3518: init plan-phase's uat_path projector must resolve via the shared + // resolveUatFile resolver (phase-pinned, deterministic) instead of a + // hand-rolled `.find()` over unsorted readdir() order. A stray cross-phase + // UAT artifact must never become THIS phase's uat_path, on any filesystem. + // The stray is listed first in the fixture (creation-order filesystems) AND + // sorts before the phase's own file (lexicographic-order filesystems), so + // the pre-fix readdir pick loses on both ordering families. + test('#3518: init plan-phase uat_path is phase-pinned — a stray cross-phase -UAT.md must not win', () => { + seedPhase(tmpDir, '03-api', { + '02-UAT.md': '# Stray cross-phase UAT artifact (belongs to phase 02)', + '03-UAT.md': '# UAT', + }); + writePlanningDocs(tmpDir); + + const result = runGsdTools('init plan-phase 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.uat_path, + absPlanningPath(tmpDir, 'phases', '03-api', '03-UAT.md'), + 'the phase\'s own 03-UAT.md must win over the stray cross-phase 02-UAT.md', + ); + }); + // #2056: normalizePhaseName() strips ANY [A-Z][A-Z0-9_]*- prefix as a project // code, so a foreign-prefixed workstream/task id like "MEM-01" collapsed to // "01" and resolved to the unrelated numeric Phase 01. init plan-phase must @@ -406,6 +431,29 @@ describe('init commands', () => { ); }); + // #3518: init phase-op's uat_path projector — the second of the two + // single-pick UAT sites in src/init.cts — same phase-pinned contract as + // the plan-phase test above. Stray created first AND sorting first, so the + // pre-fix readdir-order pick deterministically chose it on both ordering + // families. + test('#3518: init phase-op uat_path is phase-pinned — a stray cross-phase -UAT.md must not win', () => { + seedPhase(tmpDir, '03-api', { + '02-UAT.md': '# Stray cross-phase UAT artifact (belongs to phase 02)', + '03-UAT.md': '# UAT', + }); + writePlanningDocs(tmpDir); + + const result = runGsdTools('init phase-op 03', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.uat_path, + absPlanningPath(tmpDir, 'phases', '03-api', '03-UAT.md'), + 'the phase\'s own 03-UAT.md must win over the stray cross-phase 02-UAT.md', + ); + }); + test('init plan-phase detects has_reviews and reviews_path when REVIEWS.md exists', () => { seedPhase(tmpDir, '03-api', { '03-REVIEWS.md': '# Cross-AI Reviews', diff --git a/tests/verification-status.test.cjs b/tests/verification-status.test.cjs index 8114616f3..12c209e47 100644 --- a/tests/verification-status.test.cjs +++ b/tests/verification-status.test.cjs @@ -48,6 +48,7 @@ const { VERIFICATION_ROUTING_TABLE, defaultPhaseCleanCommitTimesMs, resolveVerificationFile, + resolveUatFile, readVerificationStatus, findStaleVerificationSummary, } = require('../gsd-core/bin/lib/verification.cjs'); @@ -1124,6 +1125,126 @@ describe('#3473 F2: resolveVerificationFile allowBare option', () => { }); +// ─── #3518: resolveUatFile — phase-pinned, deterministic *-UAT.md pick ─────── +// +// Both uat_path projectors in src/init.cts picked the phase's UAT artifact +// with a bare `.find((f) => f.endsWith('-UAT.md') || f === 'UAT.md')` over an +// unsorted readdir listing: no phase-membership check, no ordering. A stray +// cross-phase 04-UAT.md in phase 03's directory could become phase 03's +// uat_path, and WHICH file won was filesystem-dependent (creation order on +// APFS, hash order on ext4/XFS) — two machines on the same commit could emit +// different uat_path values for the same phase (#3518). +// +// resolveUatFile is the UAT counterpart of resolveVerificationFile, sharing +// the identical selection rule via the resolvePhaseArtifactFile core: the +// phase's own -UAT.md always wins; otherwise alphabetically-first +// dashed candidate (deterministic on every filesystem); a bare UAT.md only +// when allowBare is set and no dashed candidate exists at all. +// +// These unit tests are the RELIABLE ANCHORS for the rule (a real phaseToken +// string, no readdir order involved). The end-to-end red/green repro for the +// two init.cts projector call sites lives in tests/init.test.cjs (#3518). +describe('#3518: resolveUatFile — phase-pinned *-UAT.md resolution', () => { + + test('#3518 regression: a stray cross-phase -UAT.md does not outrank this phase\'s own UAT file', () => { + assert.equal( + resolveUatFile( + ['04-UAT.md', '03-UAT.md'], + { phaseToken: '03' }, + ), + '03-UAT.md', + 'this phase (token "03") owns 03-UAT.md; the stray 04-UAT.md belongs to a different phase', + ); + }); + + test('order-independence: same candidates reversed → same answer', () => { + assert.equal( + resolveUatFile( + ['03-UAT.md', '04-UAT.md'], + { phaseToken: '03' }, + ), + '03-UAT.md', + 'input order must not change which file is selected', + ); + }); + + test('fallback: only a stray cross-phase file present → still returned, never null', () => { + // Load-bearing: a phase whose only UAT artifact is not its own + // canonically-named file must keep resolving to SOMETHING, not to null — + // deterministically (alphabetically-first) rather than by readdir order. + assert.equal( + resolveUatFile(['04-UAT.md', '02-UAT.md'], { phaseToken: '03' }), + '02-UAT.md', + '"02-UAT.md" sorts before "04-UAT.md" — deterministic even when the phase\'s own file is absent', + ); + }); + + test('fallback determinism: several candidates, no phase token given → alphabetically first', () => { + assert.equal(resolveUatFile(['02-UAT.md', '01-a-UAT.md']), '01-a-UAT.md'); + }); + + test('allowBare defaults to false — a bare-only list returns null without the option', () => { + assert.equal(resolveUatFile(['UAT.md']), null); + }); + + test('allowBare:true, bare + dashed → the dashed candidate wins', () => { + assert.equal( + resolveUatFile(['UAT.md', '03-UAT.md'], { allowBare: true, phaseToken: '03' }), + '03-UAT.md', + 'a dashed file names its phase and must win over a bare match', + ); + }); + + test('allowBare:true, bare-only candidate → bare file returned', () => { + assert.equal(resolveUatFile(['UAT.md'], { allowBare: true }), 'UAT.md'); + }); + + test('no matches → null', () => { + assert.equal(resolveUatFile(['03-PLAN.md', '03-SUMMARY.md'], { phaseToken: '03' }), null); + }); +}); + +// ─── #3518: call-site guard — no hand-rolled *-UAT.md single-pick survives ──── +// +// The "Partial Fix Across Call Sites" regression class: a future contributor +// adding a NEW uat_path-style projection (or reverting one of the two fixed +// init.cts sites) would hand-roll `.find((f) => f.endsWith('-UAT.md') || +// f === 'UAT.md')` again — reintroducing the readdir-order, +// no-phase-check pick #3518 closed. This scans src/ for that literal shape +// and fails on any site outside src/verification.cts, whose +// resolvePhaseArtifactFile core is the single owner of the pattern. +// (src/commands.cts's scaffold WRITER builds `${padded}-UAT.md` directly — +// a canonical-name construction, not a discovery pick — and does not match.) +describe('#3518: call-site guard — every *-UAT.md single-pick routes through resolveUatFile', () => { + + test('no hand-rolled -UAT.md discovery pick exists outside src/verification.cts', () => { + const srcDir = path.join(__dirname, '..', 'src'); + const owner = path.join(srcDir, 'verification.cts'); + // The two shapes the pre-#3518 bug appeared as: an endsWith('-UAT.md') + // predicate, or a bare `=== 'UAT.md'` equality, anywhere in src/. + const HAND_ROLLED_RE = /endsWith\(['"`]-UAT\.md['"`]\)|===\s*['"`]UAT\.md['"`]/; + const offenders = []; + for (const file of fs.readdirSync(srcDir)) { + if (!file.endsWith('.cts')) continue; + const fullPath = path.join(srcDir, file); + if (fullPath === owner) continue; + // CRLF-tolerant split (local/no-crlf-fragile-split): Windows + // git-autocrlf checkouts yield \r\n line endings. + const lines = fs.readFileSync(fullPath, 'utf-8').split(/\r?\n/); + lines.forEach((line, i) => { + if (HAND_ROLLED_RE.test(line)) offenders.push(`src/${file}:${i + 1}: ${line.trim()}`); + }); + } + assert.deepStrictEqual( + offenders, + [], + 'hand-rolled *-UAT.md single-pick(s) — route through resolveUatFile ' + + '(src/verification.cts, issue #3518):\n' + + offenders.join('\n'), + ); + }); +}); + // ─── #3057 B3: findStaleVerificationSummary — indeterminate vs not-stale ───── // // The pre-fix catch-all returned `null` on ANY fs / scanPhasePlans / clock