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); } + }); +});