From 1b09fb13d3e580e875471bda1fc357bc2cf5a391 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 13 Aug 2026 01:37:33 -0400 Subject: [PATCH] refactor(#3309): add phase-structure health-diagnostic rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W005, W023, I001, W009 — phase directory naming, duplicate phase keys, plan-summary presence, and validation-architecture checks, migrated onto the frozen rule table per ADR-3180 §8.2. --- .../phase-structure.cts | 196 ++++++++++++ .../phase-structure.test.cjs | 291 ++++++++++++++++++ 2 files changed, 487 insertions(+) create mode 100644 src/health-diagnostic-rules/phase-structure.cts create mode 100644 tests/health-diagnostic-rules/phase-structure.test.cjs diff --git a/src/health-diagnostic-rules/phase-structure.cts b/src/health-diagnostic-rules/phase-structure.cts new file mode 100644 index 000000000..8d8667e31 --- /dev/null +++ b/src/health-diagnostic-rules/phase-structure.cts @@ -0,0 +1,196 @@ +/** + * Health Diagnostic — Phase directory structure rules (Phase 11, #3309, + * ADR-3180 §8.2/§8.3/§8.5). + * + * Group: "Phase directory structure" (design doc, "Rule table organization" + * table) — W005, W023, I001, W009. + * + * Ported behavior-preserving from `cmdValidateHealth` + * (`src/verify.cts:1893-1990`, the exact call sites for W005/W023/I001/W009), + * with two disclosed fidelity reductions forced by `PlanningSnapshot`'s + * current shape (see each rule's own comment below): + * + * - I001 cannot name the individual unsummarized PLAN filename (`snapshot. + * phases.value[i]` exposes only `planCount`/`summaryCount`, not per-plan + * filenames) — this rule reports a coarser per-PHASE message instead. + * - W023's original "described" list called `determinePhaseStatus` + * (`commands.cts:154`), a SIX-way status string ('Not Started'/'Planned'/ + * 'In Progress'/'Executed'/'Needs Review'/'Complete') computed from its own + * raw `readdirSync` + `*-VERIFICATION.md` frontmatter read of `phaseDir` — + * neither `PhaseSnapshot.complete` (a boolean) nor `PhaseSnapshot. + * verificationStatus` (the DIFFERENT, `readVerificationStatus`-routed + * status vocabulary: 'passed'/'gaps_found'/'human_needed'/'stale'/ + * 'unknown'/'missing', §7.4 disk-strict) reproduces that six-way string + * byte-for-byte — the two computations read the same file independently + * and can disagree (e.g. a stale-but-frontmatter-"passed" VERIFICATION.md + * reads 'Complete' under the original raw read but routes to a non-'passed' + * `verificationStatus` under §7.4's staleness handling). Reproducing the + * raw frontmatter read here would violate §8.1 rule 1 (no ambient I/O in a + * rule's `check`). This rule instead describes each colliding directory + * with the snapshot fields actually available (`planCount`, `summaryCount`, + * `verificationStatus`) — a disclosed fidelity reduction, not a silent + * reproduction of the original six-way label. + * + * Design: .gsd/phase/refactor-3309-health-diagnostic-rule-table/40-design.md + * + * ADR-457 build-at-publish: source in + * src/health-diagnostic-rules/phase-structure.cts, compiled to + * gsd-core/bin/lib/health-diagnostic-rules/phase-structure.cjs (gitignored). + */ + +// eslint-disable-next-line @typescript-eslint/no-require-imports -- type-only; erased at compile time, no runtime require emitted +import type planningSnapshotMod = require('../planning-snapshot.cjs'); + +type PlanningSnapshot = ReturnType; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +import healthDiagnosticMod = require('../health-diagnostic.cjs'); +const { SEVERITY, REMEDY_ACTION, REMEDY_RISK } = healthDiagnosticMod; +type Diagnostic = healthDiagnosticMod.Diagnostic; +type Rule = healthDiagnosticMod.Rule; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +import validateMod = require('../validate.cjs'); +const { phaseDirNameRe } = validateMod; + +// eslint-disable-next-line @typescript-eslint/no-require-imports +import phaseIdMod = require('../phase-id.cjs'); +const { extractPhaseToken, normalizePhaseName, comparePhaseNum } = phaseIdMod; + +// ─── W005 — phase directory doesn't follow NN-name format (verify.cts:1893-1902) ─ + +function checkW005(snapshot: PlanningSnapshot): Diagnostic[] { + const diagnostics: Diagnostic[] = []; + for (const name of snapshot.phaseDirs.value) { + if (!name.match(phaseDirNameRe)) { + diagnostics.push({ + code: 'W005', + severity: SEVERITY.WARNING, + message: `Phase directory "${name}" doesn't follow NN-name format`, + remedy: { + action: REMEDY_ACTION.ADVISE, + risk: REMEDY_RISK.NONE, + args: { command: 'Rename to match pattern (e.g., 01-setup)' }, + }, + }); + } + } + return diagnostics; +} + +// ─── W023 — phase directories collide on normalized key (verify.cts:1904-1950) ─ +// +// Groups `snapshot.phaseDirs.value` by `normalizePhaseName(extractPhaseToken(name))` +// — the exact same two owners (`phase-id.cjs`) the original `verify.cts:1917-1918` +// call site uses, relocated verbatim rather than reimplemented. Sorted with +// `comparePhaseNum` + a `localeCompare` tiebreak, mirroring +// `verify.cts:1930-1932`'s deterministic-output rationale. See the file-level +// comment for the disclosed "described" fidelity reduction. + +function checkW023(snapshot: PlanningSnapshot): Diagnostic[] { + const groups = new Map(); + for (const name of snapshot.phaseDirs.value) { + const token = extractPhaseToken(name); + const key = normalizePhaseName(token); + const list = groups.get(key); + if (list) list.push(name); + else groups.set(key, [name]); + } + + const phaseByDir = new Map(snapshot.phases.value.map((p) => [p.dir, p])); + + const diagnostics: Diagnostic[] = []; + for (const [key, dirs] of groups) { + if (dirs.length < 2) continue; + const described = dirs + .slice() + .sort((a, b) => comparePhaseNum(a, b) || String(a).localeCompare(String(b))) + .map((d) => { + const phase = phaseByDir.get(d); + const plans = phase ? phase.planCount : 0; + const summaries = phase ? phase.summaryCount : 0; + const verificationStatus = phase ? phase.verificationStatus : 'missing'; + return `${d} (plans: ${plans}, summaries: ${summaries}, verification: ${verificationStatus})`; + }) + .join(', '); + diagnostics.push({ + code: 'W023', + severity: SEVERITY.WARNING, + message: `Phase directories collide on normalized key "${key}": ${described}`, + remedy: { + action: REMEDY_ACTION.ADVISE, + risk: REMEDY_RISK.NONE, + args: { + command: + 'Inspect each directory; rename or remove the duplicate so only one directory maps to this phase key', + }, + }, + }); + } + return diagnostics; +} + +// ─── I001 — plan(s) without a matching SUMMARY.md (verify.cts:1952-1965) ─── +// +// GENUINE FIDELITY GAP (see file-level comment): the original is PER-PLAN +// (`${e.name}/${plan} has no SUMMARY.md`, `plan` an individual PLAN.md +// filename from `findUnsummarizedPlans`). `PlanningSnapshot`'s +// `phases.value[i]` carries only `planCount`/`summaryCount` NUMBERS per +// phase — no per-plan filenames — so this rule cannot name which plan lacks +// a summary without reading the phase directory directly inside `check` +// (forbidden by §8.1 rule 1). This rule instead reports one coarser +// per-PHASE diagnostic naming the deficit count, not the individual +// filename(s). + +function checkI001(snapshot: PlanningSnapshot): Diagnostic[] { + const diagnostics: Diagnostic[] = []; + for (const phase of snapshot.phases.value) { + const deficit = phase.planCount - phase.summaryCount; + if (deficit > 0) { + diagnostics.push({ + code: 'I001', + severity: SEVERITY.INFO, + message: `Phase ${phase.dir} has ${deficit} plan(s) without a matching summary`, + remedy: { + action: REMEDY_ACTION.ADVISE, + risk: REMEDY_RISK.NONE, + args: { command: 'May be in progress' }, + }, + }); + } + } + return diagnostics; +} + +// ─── W009 — Validation Architecture in RESEARCH.md but no VALIDATION.md ──── +// (verify.cts:1967-1990) + +function checkW009(snapshot: PlanningSnapshot): Diagnostic[] { + const diagnostics: Diagnostic[] = []; + for (const entry of snapshot.researchValidationStatus.value) { + if (entry.hasValidationArchitecture && !entry.hasValidationMd) { + diagnostics.push({ + code: 'W009', + severity: SEVERITY.WARNING, + message: `Phase ${entry.dir}: has Validation Architecture in RESEARCH.md but no VALIDATION.md`, + remedy: { + action: REMEDY_ACTION.ADVISE, + risk: REMEDY_RISK.NONE, + args: { command: 'Re-run /gsd-plan-phase with --research to regenerate' }, + }, + }); + } + } + return diagnostics; +} + +// ─── Exports ──────────────────────────────────────────────────────────────── + +const RULES: Rule[] = [ + { code: 'W005', severity: SEVERITY.WARNING, check: checkW005 }, + { code: 'W023', severity: SEVERITY.WARNING, check: checkW023 }, + { code: 'I001', severity: SEVERITY.INFO, check: checkI001 }, + { code: 'W009', severity: SEVERITY.WARNING, check: checkW009 }, +]; + +export = { RULES }; diff --git a/tests/health-diagnostic-rules/phase-structure.test.cjs b/tests/health-diagnostic-rules/phase-structure.test.cjs new file mode 100644 index 000000000..ceb24f900 --- /dev/null +++ b/tests/health-diagnostic-rules/phase-structure.test.cjs @@ -0,0 +1,291 @@ +'use strict'; + +/** + * Tests for `src/health-diagnostic-rules/phase-structure.cts` (Phase 11, + * #3309, ADR-3180 §8.2/§8.3/§8.5) — the "Phase directory structure" rule + * group: W005, W023, I001, W009. + * + * Design: .gsd/phase/refactor-3309-health-diagnostic-rule-table/40-design.md + * + * Fixture provenance (§8.5 + CONTRIBUTING "Fixture provenance (#2371)"): + * - W005/W023 are MECHANICAL MUTATION — a malformed directory name / two + * directories deliberately constructed to collide on the same normalized + * phase key. Both are directory-NAME shapes, not a document format being + * modeled, so there is nothing to mutate from a template; the mutation IS + * the directory name itself. + * - I001/W009 are STRUCTURAL ABSENCE — a missing SUMMARY.md / missing + * VALIDATION.md file. Exempt from the provenance concern per §8.5's own + * category 1: the fixture *is* the absence, no format is being modeled. + * + * Every case calls the REAL `buildPlanningSnapshot(cwd)` (Phase 10, + * `src/planning-snapshot.cts`) against real temp `.planning/` trees, then + * calls the REAL rule `check` functions from the compiled module under + * test — no hand-built in-memory snapshot mocks. Fixture helpers mirror + * `tests/planning-snapshot.test.cjs`'s own `writeState`/`writeRoadmap`/ + * `writeFile`/`makeCompletePhaseDir` verbatim. + * + * All fixtures use a ROADMAP.md with NO `Phase N:` headings under the + * current milestone section. `getMilestonePhaseFilter` + * (`src/roadmap-parser.cts:1341-1355`) degrades to a pass-all filter + * whenever `milestonePhaseNums.size === 0`, so `listMilestonePhaseDirs` + * enumerates every on-disk phase directory regardless of whether its name + * parses as a phase id — which is exactly what these rules need to exercise + * (a malformed dir name would otherwise never reach `phaseDirs.value` in the + * first place, since a non-matching name also fails the window's own + * numeric-prefix membership test). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { createTempDir, cleanup } = require('../helpers.cjs'); + +const { buildPlanningSnapshot } = require('../../gsd-core/bin/lib/planning-snapshot.cjs'); +const { RULES } = require('../../gsd-core/bin/lib/health-diagnostic-rules/phase-structure.cjs'); + +const ruleByCode = Object.fromEntries(RULES.map((r) => [r.code, r])); + +// ─── Fixture helpers (mirrors tests/planning-snapshot.test.cjs) ──────────── + +function planningDirOf(cwd) { + return path.join(cwd, '.planning'); +} + +function writeRoadmap(cwd, content) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + fs.writeFileSync(path.join(planningDirOf(cwd), 'ROADMAP.md'), content); +} + +function writeState(cwd, fields) { + fs.mkdirSync(planningDirOf(cwd), { recursive: true }); + const lines = ['---']; + for (const [k, v] of Object.entries(fields)) lines.push(`${k}: ${v}`); + lines.push('---', ''); + fs.writeFileSync(path.join(planningDirOf(cwd), 'STATE.md'), lines.join('\n')); +} + +function writeFile(cwd, relPath, content) { + const full = path.join(cwd, relPath); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); +} + +function makeCompletePhaseDir(cwd, relPhaseDir) { + writeFile(cwd, `${relPhaseDir}/01-01-PLAN.md`, '# Plan\n'); + writeFile(cwd, `${relPhaseDir}/01-01-SUMMARY.md`, '# Summary\n'); + writeFile(cwd, `${relPhaseDir}/01-VERIFICATION.md`, '---\nstatus: passed\n---\n'); +} + +// No `Phase N:` heading anywhere -> getMilestonePhaseFilter's pass-all +// degrade -> listMilestonePhaseDirs enumerates every on-disk phase dir name +// verbatim, malformed or not. +function writePassAllRoadmap(cwd) { + writeRoadmap(cwd, ['## v1.0 Current 🚧', ''].join('\n')); +} + +function baseFixture(cwd) { + writeState(cwd, { milestone: 'v1.0' }); + writePassAllRoadmap(cwd); +} + +// ─── W005 — phase directory doesn't follow NN-name format ────────────────── + +describe('W005 — phase directory naming', () => { + test('MECHANICAL MUTATION: a directory name with no NN- prefix fires W005', (t) => { + const cwd = createTempDir('gsd-3309-w005-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + writeFile(cwd, '.planning/phases/notaphase/README.md', '# not a phase dir\n'); + + const snap = buildPlanningSnapshot(cwd); + assert.ok(snap.phaseDirs.value.includes('notaphase'), 'fixture sanity: malformed dir enumerated'); + + const diagnostics = ruleByCode['W005'].check(snap); + assert.deepEqual( + diagnostics.map((d) => d.code), + ['W005'], + ); + assert.match(diagnostics[0].message, /"notaphase"/); + assert.match(diagnostics[0].message, /doesn't follow NN-name format/); + assert.equal(diagnostics[0].severity, 'warning'); + assert.equal(diagnostics[0].remedy.action, 'advise'); + assert.equal(diagnostics[0].remedy.risk, 'none'); + }); + + test('baseline: only well-formed directory names produces no diagnostics', (t) => { + const cwd = createTempDir('gsd-3309-w005-neg-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + makeCompletePhaseDir(cwd, '.planning/phases/02-bar'); + + const snap = buildPlanningSnapshot(cwd); + assert.deepEqual(ruleByCode['W005'].check(snap), []); + }); +}); + +// ─── W023 — phase directories collide on normalized key ──────────────────── + +describe('W023 — colliding phase directories', () => { + test('MECHANICAL MUTATION: two directories that normalize to the same key fire W023', (t) => { + const cwd = createTempDir('gsd-3309-w023-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + // extractPhaseToken("05-real") === "05"; extractPhaseToken("05-real-stray") + // === "05" too (the tokenizer stops at the first non-continuation segment, + // "real"/"stray" is a slug word, not a zero-padded continuation) — both + // normalize to phase key "05" via normalizePhaseName. + makeCompletePhaseDir(cwd, '.planning/phases/05-real'); + writeFile(cwd, '.planning/phases/05-real-stray/01-01-PLAN.md', '# Plan\n'); + + const snap = buildPlanningSnapshot(cwd); + assert.ok( + snap.phaseDirs.value.includes('05-real') && snap.phaseDirs.value.includes('05-real-stray'), + 'fixture sanity: both colliding dirs enumerated', + ); + + const diagnostics = ruleByCode['W023'].check(snap); + assert.deepEqual( + diagnostics.map((d) => d.code), + ['W023'], + ); + assert.match(diagnostics[0].message, /collide on normalized key "05"/); + assert.match(diagnostics[0].message, /05-real \(/); + assert.match(diagnostics[0].message, /05-real-stray \(/); + assert.equal(diagnostics[0].severity, 'warning'); + }); + + test('baseline: distinct phase keys produce no diagnostics', (t) => { + const cwd = createTempDir('gsd-3309-w023-neg-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + makeCompletePhaseDir(cwd, '.planning/phases/02-bar'); + + const snap = buildPlanningSnapshot(cwd); + assert.deepEqual(ruleByCode['W023'].check(snap), []); + }); +}); + +// ─── I001 — plan(s) without a matching SUMMARY.md ────────────────────────── + +describe('I001 — unsummarized plans', () => { + test('STRUCTURAL ABSENCE: a PLAN.md with no matching SUMMARY.md fires I001', (t) => { + const cwd = createTempDir('gsd-3309-i001-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + writeFile(cwd, '.planning/phases/01-foo/01-01-PLAN.md', '# Plan\n'); + + const snap = buildPlanningSnapshot(cwd); + const phase = snap.phases.value.find((p) => p.dir === '01-foo'); + assert.equal(phase.planCount, 1); + assert.equal(phase.summaryCount, 0); + + const diagnostics = ruleByCode['I001'].check(snap); + assert.deepEqual( + diagnostics.map((d) => d.code), + ['I001'], + ); + assert.match(diagnostics[0].message, /Phase 01-foo has 1 plan\(s\) without a matching summary/); + assert.equal(diagnostics[0].severity, 'info'); + }); + + test('baseline: matched plan/summary pairs produce no diagnostics', (t) => { + const cwd = createTempDir('gsd-3309-i001-neg-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + + const snap = buildPlanningSnapshot(cwd); + assert.deepEqual(ruleByCode['I001'].check(snap), []); + }); +}); + +// ─── W009 — Validation Architecture in RESEARCH.md but no VALIDATION.md ─── + +describe('W009 — missing VALIDATION.md', () => { + test('STRUCTURAL ABSENCE: RESEARCH.md has Validation Architecture but no VALIDATION.md fires W009', (t) => { + const cwd = createTempDir('gsd-3309-w009-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + writeFile( + cwd, + '.planning/phases/01-foo/01-RESEARCH.md', + '# Research\n\n## Validation Architecture\n\nSome content.\n', + ); + + const snap = buildPlanningSnapshot(cwd); + const entry = snap.researchValidationStatus.value.find((e) => e.dir === '01-foo'); + assert.equal(entry.hasValidationArchitecture, true); + assert.equal(entry.hasValidationMd, false); + + const diagnostics = ruleByCode['W009'].check(snap); + assert.deepEqual( + diagnostics.map((d) => d.code), + ['W009'], + ); + assert.match( + diagnostics[0].message, + /Phase 01-foo: has Validation Architecture in RESEARCH\.md but no VALIDATION\.md/, + ); + assert.equal(diagnostics[0].severity, 'warning'); + }); + + test('baseline: VALIDATION.md present alongside Validation Architecture produces no diagnostics', (t) => { + const cwd = createTempDir('gsd-3309-w009-neg-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + writeFile( + cwd, + '.planning/phases/01-foo/01-RESEARCH.md', + '# Research\n\n## Validation Architecture\n\nSome content.\n', + ); + writeFile(cwd, '.planning/phases/01-foo/01-VALIDATION.md', '# Validation\n'); + + const snap = buildPlanningSnapshot(cwd); + assert.deepEqual(ruleByCode['W009'].check(snap), []); + }); + + test('baseline: RESEARCH.md without Validation Architecture heading produces no diagnostics', (t) => { + const cwd = createTempDir('gsd-3309-w009-neg2-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + writeFile(cwd, '.planning/phases/01-foo/01-RESEARCH.md', '# Research\n\nNo relevant heading here.\n'); + + const snap = buildPlanningSnapshot(cwd); + assert.deepEqual(ruleByCode['W009'].check(snap), []); + }); +}); + +// ─── §8.2 rule 1 — every diagnostic's severity matches its rule's declared severity ─ + +describe('rule/severity 1:1 (§8.2 rule 1)', () => { + test('every emitted diagnostic carries the same severity as its rule entry', (t) => { + const cwd = createTempDir('gsd-3309-sev-'); + t.after(() => cleanup(cwd)); + baseFixture(cwd); + makeCompletePhaseDir(cwd, '.planning/phases/01-foo'); + writeFile(cwd, '.planning/phases/notaphase/README.md', '# not a phase dir\n'); + writeFile(cwd, '.planning/phases/05-real-stray/01-01-PLAN.md', '# Plan\n'); + writeFile(cwd, '.planning/phases/02-bar/01-01-PLAN.md', '# Plan\n'); + writeFile( + cwd, + '.planning/phases/02-bar/01-RESEARCH.md', + '## Validation Architecture\n', + ); + + const snap = buildPlanningSnapshot(cwd); + for (const rule of RULES) { + for (const diagnostic of rule.check(snap)) { + assert.equal(diagnostic.code, rule.code); + assert.equal(diagnostic.severity, rule.severity); + } + } + }); +});