From 2979f2a994b157b003b231fe41a6db4f28794dce Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 5 Aug 2026 18:22:14 -0400 Subject: [PATCH] fix(#2978): add structural validation to roadmap validate (#3092) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2978): roadmap validate must perform structural validation Failing-first: roadmap validate returns {"warnings":[]} (exit 0) for every input — empty file, garbage, missing file, truncated frontmatter — because it performs no structural validation and its one opt-in check (W021 milestone- prefix) is off by default. Six cases: empty, garbage, missing, truncated frontmatter, well-formed (no false positive), BOM-prefixed (not corruption). * fix(#2978): add structural validation to roadmap validate roadmap validate returned {"warnings":[]} (exit 0) for every input — empty file, garbage, missing file, truncated frontmatter — because it performed no structural validation and its one opt-in check (W021 milestone-prefix) is off by default. A verb named validate that cannot produce a negative result provides false assurance. Add four structural checks, each producing a coded warning {code, message}: - V001: file missing/unreadable (was silent success) - V002: empty/whitespace-only - V003: malformed frontmatter (unterminated --- fence; BOM-tolerant per #3057) - V004: no recognizable phase entries (no ### Phase N: heading) Keep the existing W021 milestone-prefix check as-is. Exit non-zero via ExitError(1) when warnings are non-empty, per the documented contract ('exits non-zero on any error or warning'). Well-formed roadmaps (incl. BOM-prefixed, CRLF) still validate cleanly with warnings: [] and exit 0. * test(#2978): update W021 tests for non-zero exit on warnings Two existing W021 tests asserted roadmap validate exits 0 even with warnings ('roadmap validate should exit 0 even with warnings') — that was the bug. #2978 made validate exit non-zero on any warning (per its documented contract). Updated both mismatch-case tests to expect success===false and parse the JSON output from the failure path (stdout is written before the ExitError throw). * chore(#2978): add changeset fragment * chore(#2978): backfill changeset PR number 3092 --------- Co-authored-by: sim --- .changeset/silly-moles-rally.md | 5 ++ src/roadmap-command-router.cts | 53 ++++++++++++-- tests/milestone-prefixed-convention.test.cjs | 6 +- tests/roadmap.test.cjs | 76 ++++++++++++++++++++ 4 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 .changeset/silly-moles-rally.md diff --git a/.changeset/silly-moles-rally.md b/.changeset/silly-moles-rally.md new file mode 100644 index 000000000..f1a88560b --- /dev/null +++ b/.changeset/silly-moles-rally.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3092 +--- +**`roadmap validate` now performs real structural validation** — it previously returned `{"warnings":[]}` (exit 0) for every input including empty files, garbage text, and missing files, providing false assurance. It now checks file existence/readability, emptiness, frontmatter well-formedness, and the presence of at least one phase entry, exiting non-zero on any warning (per its documented contract). The existing opt-in milestone-prefix consistency check is preserved. (#2978) diff --git a/src/roadmap-command-router.cts b/src/roadmap-command-router.cts index 7ef26d5a1..7b54d8cf4 100644 --- a/src/roadmap-command-router.cts +++ b/src/roadmap-command-router.cts @@ -21,6 +21,9 @@ const { planningDir } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import configLoaderMod = require('./config-loader.cjs'); const { loadConfig } = configLoaderMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import cliExitMod = require('./cli-exit.cjs'); +const { ExitError } = cliExitMod; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -143,11 +146,43 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }: RouteRoadmapCom 'annotate-dependencies': () => roadmap.cmdRoadmapAnnotateDependencies(cwd, args[2], raw), 'validate': () => { const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); - let roadmapContent = ''; + const warnings: Array<{ code: string; message: string }> = []; + + // #2978: structural validation. A verb named "validate" that cannot + // produce a negative result provides false assurance. Before the + // opt-in milestone-prefix check, verify the file is structurally a + // roadmap at all. + let roadmapContent: string; try { roadmapContent = fs.readFileSync(roadmapPath, 'utf8'); } catch { - // ROADMAP.md missing — return empty warnings + // ROADMAP.md missing — not silent success. + warnings.push({ code: 'V001', message: 'ROADMAP.md not found or unreadable' }); + const result = { warnings }; + process.stdout.write(raw ? JSON.stringify(result) : JSON.stringify(result, null, 2)); + throw new ExitError(1); + } + + // Empty or whitespace-only. + if (roadmapContent.trim() === '') { + warnings.push({ code: 'V002', message: 'ROADMAP.md is empty' }); + } + + // Malformed frontmatter — a `---` opener with no matching closer. + // Tolerate a leading BOM (#3057) before the fence. + const contentAfterBom = roadmapContent.replace(/^\uFEFF/, ''); + if (contentAfterBom.startsWith('---')) { + const closeMatch = contentAfterBom.slice(3).match(/\r?\n---\s*(\r?\n|$)/); + if (!closeMatch) { + warnings.push({ code: 'V003', message: 'ROADMAP.md frontmatter is malformed (unterminated --- fence)' }); + } + } + + // No recognizable phase structure — at least one `### Phase N:` heading. + // Mirrors the phase-heading pattern used across roadmap-parser.cts. + const hasPhaseEntry = /^#{2,4}\s*Phase\s+\S/im.test(roadmapContent); + if (!hasPhaseEntry && !warnings.some((w) => w.code === 'V002')) { + warnings.push({ code: 'V004', message: 'ROADMAP.md contains no recognizable phase entries (no "### Phase N:" headings)' }); } // W021 only fires when phase_id_convention is explicitly 'milestone-prefixed'. @@ -173,13 +208,17 @@ function routeRoadmapCommand({ roadmap, args, cwd, raw, error }: RouteRoadmapCom } } } - const warnings = (convention === 'milestone-prefixed') - ? checkW021(roadmapContent) - : []; + if (convention === 'milestone-prefixed') { + warnings.push(...checkW021(roadmapContent)); + } const result = { warnings }; - if (raw) process.stdout.write(JSON.stringify(result)); - else process.stdout.write(JSON.stringify(result, null, 2)); + process.stdout.write(raw ? JSON.stringify(result) : JSON.stringify(result, null, 2)); + // #2978: exit non-zero on any warning, per the documented contract + // ("exits non-zero on any error or warning"). + if (warnings.length > 0) { + throw new ExitError(1); + } }, 'upgrade': () => { const dryRun = !args.includes('--apply'); diff --git a/tests/milestone-prefixed-convention.test.cjs b/tests/milestone-prefixed-convention.test.cjs index f54e42a1a..455b42632 100644 --- a/tests/milestone-prefixed-convention.test.cjs +++ b/tests/milestone-prefixed-convention.test.cjs @@ -89,7 +89,8 @@ describe('W021 — milestone-prefixed phase ID convention', () => { ]); const result = runGsdTools(['roadmap', 'validate'], tmpDir); - assert.ok(result.success, `roadmap validate should exit 0 even with warnings: ${result.error}`); + // #2978: validate now exits non-zero on any warning (per its documented contract). + assert.strictEqual(result.success, false, `roadmap validate must exit non-zero on W021 warnings: ${result.error}`); const out = JSON.parse(result.output); assert.ok(Array.isArray(out.warnings), 'output.warnings should be an array'); @@ -219,7 +220,8 @@ describe('W021 — milestone-prefixed phase ID convention', () => { ]); const result = runGsdTools(['roadmap', 'validate'], tmpDir); - assert.ok(result.success, `roadmap validate failed: ${result.error}`); + // #2978: validate now exits non-zero on any warning (per its documented contract). + assert.strictEqual(result.success, false, `roadmap validate must exit non-zero on W021 warnings: ${result.error}`); const out = JSON.parse(result.output); const w021 = (out.warnings || []).filter(w => w.code === 'W021'); diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index 8b9e60d6f..9b518f39b 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -3612,3 +3612,79 @@ describe('bug #1103 — annotate-dependencies preserves newline before Plans: he }); }); } + +// ───────────────────────────────────────────────────────────────────────────── +// bug #2978: roadmap validate returns {"warnings":[]} for every input +// ───────────────────────────────────────────────────────────────────────────── + +describe('bug #2978: roadmap validate performs structural validation', () => { + test('empty (zero-byte) ROADMAP.md → non-empty warnings, non-zero exit', () => { + const tmpDir = createTempProject('gsd-2978-empty-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), ''); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'empty file must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(Array.isArray(payload.warnings) && payload.warnings.length > 0, + `empty file must produce non-empty warnings; got: ${JSON.stringify(payload)}`); + } finally { cleanup(tmpDir); } + }); + + test('garbage/non-roadmap text → non-empty warnings, non-zero exit', () => { + const tmpDir = createTempProject('gsd-2978-garbage-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'not a roadmap at all'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'garbage must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(payload.warnings.length > 0, 'garbage must produce warnings'); + } finally { cleanup(tmpDir); } + }); + + test('missing ROADMAP.md → non-empty warnings, non-zero exit', () => { + const tmpDir = createTempProject('gsd-2978-missing-'); + try { + // createTempProject creates .planning/phases but no ROADMAP.md — don't write one + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'missing file must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(payload.warnings.length > 0, 'missing file must produce warnings'); + } finally { cleanup(tmpDir); } + }); + + test('truncated frontmatter (unterminated ---) → non-empty warnings', () => { + const tmpDir = createTempProject('gsd-2978-trunc-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '---\nmilestone: v1.0\n### Phase 1: Setup\n'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.strictEqual(result.success, false, 'truncated frontmatter must exit non-zero'); + const payload = JSON.parse(result.output); + assert.ok(payload.warnings.length > 0, 'truncated frontmatter must produce warnings'); + } finally { cleanup(tmpDir); } + }); + + test('well-formed roadmap → warnings: [], exit 0 (no false positive)', () => { + const tmpDir = createTempProject('gsd-2978-good-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n## v1.0\n\n### Phase 1: Foundation\n**Goal:** setup\n'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.ok(result.success, `well-formed roadmap must exit 0; got: ${result.error}`); + const payload = JSON.parse(result.output); + assert.deepStrictEqual(payload.warnings, [], 'well-formed roadmap must have no warnings'); + } finally { cleanup(tmpDir); } + }); + + test('BOM-prefixed well-formed roadmap → warnings: [], exit 0 (not corruption)', () => { + const tmpDir = createTempProject('gsd-2978-bom-'); + try { + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), + '\uFEFF# Roadmap\n\n### Phase 1: Foundation\n**Goal:** setup\n'); + const result = runGsdTools(['roadmap', 'validate', '--raw'], tmpDir); + assert.ok(result.success, `BOM-prefixed roadmap must exit 0; got: ${result.error}`); + const payload = JSON.parse(result.output); + assert.deepStrictEqual(payload.warnings, [], 'BOM is not corruption'); + } finally { cleanup(tmpDir); } + }); +});