voyage/lib/validators/plan-validator.mjs
Kjell Tore Guttormsen bef9c5b3ac
fix: the findings of the release review of 6.3.0
The review of v6.2.0..f669b87 (BLOCK: 1 BLOCKER, 12 MAJOR, 1 MINOR), each
fixed behind a test that failed first, or, for the two missing tests, a test
shown to fail on a stub of the branch it covers.

- Review steps appended by /trekexecute Phase 7.2 are held to the step rules
  on any plan version: plan-validator --steps-from <N> (on a plan below 1.8 a
  template review step validated and was executed).
- The guard refuses where no one can be asked also when its rules will not
  load; the wave merge path names that refusal instead of MERGE_REFUSED.
  Tests for the classifier-error refusal and post-approval-log's own check.
- release-check: the version files may change only in their version after
  the review (JSON compared without it), README diff lines are read inside
  hunks only, an unreadable diff is exit 2, and npm test runs without
  NODE_TEST_CONTEXT.
- What counts as a step of only tests: a note in parentheses is not a path,
  and a data file under specs/ is not a test. End-to-end: rides on the last
  step that builds behaviour (validator hint, trekplan, template).
- A removed line is cited as <rev>:<path>:<line> (findings-schema hint,
  trekreview re-ask).
- skill-lint counts a trailing newline as no line; regression-check takes
  commandEnv from lib/util/command-env.mjs; CLAUDE.md names four mode-read
  exemptions.
- /security-review, older than v6.2.0: the criteria runner refuses git
  --output, which writes a file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-10 22:12:12 +02:00

405 lines
19 KiB
JavaScript

// lib/validators/plan-validator.mjs
// Wraps plan-schema (heading shape) + manifest-yaml (per-step Manifest blocks).
// This is the JS equivalent of Phase 5.5 grep checks in planning-orchestrator.
//
// Schema is forward-compatible: unknown top-level frontmatter keys are
// tolerated silently. Strict-key checks are intentionally avoided so new
// optional fields (jf. the source_findings precedent) can be added without
// a plan_version bump.
//
// plan_version 1.8 rules (apply only to plans declaring >= 1.8; 1.7 plans
// validate exactly as before):
// - every step has `Test first:` naming a File and what it Verifies;
// - the plan has a "run the thing" step: an `End-to-end:` field with a
// backticked command (without it a result is "not verified end to end");
// - the optional step fields `Interface in:`, `Interface out:`,
// `Review focus:` are never empty when present;
// - `ceremony:` is small | bounded | architectural (absent = architectural);
// an architectural plan carries `## Design` (see checkDesign);
// - every check the run will execute can run (checkRunnable): each
// `## Verification` criterion, each step's `Verify:` (its first backticked
// span is the step's test command) and each `End-to-end:` command pass the
// criteria runner's allowlist, and each `End-to-end:` command also stands
// under `## Verification`, where Phase 7 runs it.
// Strict mode reports these as errors; --soft downgrades them to warnings.
//
// Every plan version (the fields are new; a plan without them is unchanged):
// - K10: the optional step field `Regression:` holds ONE command the
// criteria runner's allowlist runs (first backticked span, no pipe, no
// `&&`); the executor runs it green before the step's checkpoint. One that
// can never run is an error (a warning under --soft).
// - K11: `On failure:` opens with revert | retry | skip | escalate, the words
// /trekexecute reads; any other opening is a warning.
// - A step whose `Files:` are all tests (plan-schema `stepTestsOnly`) is the
// warning PLAN_STEP_TESTS_ONLY: its tests pass at its start point, so it can
// never be proven red (voyage-lekerepo trial 3, 2026-10-09).
import { readFileSync, existsSync, realpathSync } from 'node:fs';
import { fileURLToPath } from 'node:url';
import {
sliceSteps, validatePlanHeadings, extractPlanVersion, extractPlanCeremony,
parseStepFields, findSection, splitSections, findModuleCycle, parseMermaid, fencedBlocks,
versionAtLeast, PLAN_VERSION_CURRENT, PLAN_VERSIONS_SUPPORTED, CEREMONY_VALUES,
ON_FAILURE_ACTIONS, onFailureAction, stepTestsOnly,
} from '../parsers/plan-schema.mjs';
import { validateAllManifests } from '../parsers/manifest-yaml.mjs';
import { parseDocument } from '../util/frontmatter.mjs';
import { issue, fail } from '../util/result.mjs';
import { parsePlanVerification, firstCommand, runnableCheck } from '../verification/criteria-runner.mjs';
export const OPTIONAL_STEP_FIELDS = Object.freeze(['Interface in', 'Interface out', 'Review focus']);
export const DESIGN_SUBSECTIONS = Object.freeze([
'Architecture', 'Data Flow', 'Interfaces', 'Data Models', 'Error Handling',
'Module Map', 'Architecture Rules', 'Requirements Check',
]);
const BACKTICKED = /`[^`\n]+`/;
function checkSteps(sections) {
const out = [];
let endToEnd = 0;
for (const s of sections) {
const f = parseStepFields(s.body);
const tf = f.get('Test first');
const names = tf && /\bFile:\**[ \t]*`[^`\n]+`/i.test(tf.text) && /\bVerifies:\**[ \t]*\S/i.test(tf.text);
if (!names) {
out.push(issue(
'PLAN_STEP_NO_TEST_FIRST',
`Step ${s.n}: ${tf ? '`Test first:` does not name a File (backticked path) and what it Verifies' : 'no `Test first:` field'}`,
'plan_version 1.8 requires `Test first:` on every step, with `File:` and `Verifies:` sub-bullets.',
));
}
for (const name of OPTIONAL_STEP_FIELDS) {
if (f.has(name) && f.get(name).text === '') {
out.push(issue('PLAN_STEP_FIELD_EMPTY', `Step ${s.n}: \`${name}:\` is present but empty`,
'Fill the field or remove it; it is optional.'));
}
}
if (f.has('End-to-end') && BACKTICKED.test(f.get('End-to-end').text)) endToEnd += 1;
}
if (sections.length > 0 && endToEnd === 0) {
out.push(issue(
'PLAN_NO_END_TO_END',
'No "run the thing" step: no step carries `End-to-end:` with a backticked command',
'Add a step that runs the product the way a user does (`End-to-end: `<command>` → expected: …`). ' +
'Without it the result is "not verified end to end", never green.',
));
}
return out;
}
const WAY_IN = 'A check the allowlist has no name for (a pipe, `jq`, `grep`, the product\'s own binary) is ' +
'wrapped in a script under tests/ and declared as `bash tests/<script>.sh`.';
/**
* Every check the run will execute can run. Without this a plan passed the
* validator, the plan gate and three critic rounds with a `git grep` criterion
* the runner refuses, and Phase 7 felled a run whose steps had all passed
* (voyage-lekerepo trial, 2026-10-06).
*/
function checkRunnable(text, sections, opts) {
const out = [];
const criteria = parsePlanVerification(text);
for (const c of criteria) {
const r = runnableCheck(c, opts);
if (!r.runnable) {
out.push(issue('PLAN_VERIFICATION_NOT_RUNNABLE',
`## Verification ${c.label} can never run (${r.reason}): ${c.text}`,
`Phase 7 runs every criterion and fells the run on one it cannot run. ${WAY_IN}`));
}
}
const verified = new Set(criteria.filter(c => c.command).map(c => c.command.trim().replace(/\s+/g, ' ')));
for (const s of sections) {
const f = parseStepFields(s.body);
if (f.has('Verify')) {
const v = firstCommand(f.get('Verify').text);
const r = runnableCheck(v, opts);
if (!r.runnable) {
out.push(issue('PLAN_STEP_VERIFY_NOT_RUNNABLE',
`Step ${s.n}: \`Verify:\` does not open with one runnable test command (${r.reason})`,
'The first backticked span of `Verify:` is the step\'s test command: proof of red and the done line run it, ' +
`and nothing after it is run. ${WAY_IN}`));
}
}
if (f.has('End-to-end') && BACKTICKED.test(f.get('End-to-end').text)) {
const e = firstCommand(f.get('End-to-end').text);
const r = runnableCheck(e, opts);
if (!r.runnable) {
out.push(issue('PLAN_END_TO_END_NOT_RUNNABLE',
`Step ${s.n}: \`End-to-end:\` command can never run (${r.reason})`,
`Run the product from a script: \`bash tests/e2e.sh\`. ${WAY_IN}`));
} else if (!verified.has(e.command.trim().replace(/\s+/g, ' '))) {
out.push(issue('PLAN_END_TO_END_NOT_VERIFIED',
`Step ${s.n}: \`End-to-end:\` command \`${e.command}\` does not stand under ## Verification`,
'Phase 7 runs `## Verification`; list the same command there, or the result is "not verified end to end".'));
}
}
}
return out;
}
/**
* K10 and K11. `Verify:` is both the step's one test command and its proof of
* red, so a step whose red test is a shell script could not also run the frozen
* suite before its checkpoint; the plan wrote "run npm test before the
* checkpoint" in prose that nothing enforced (voyage-lekerepo trial 2,
* 2026-10-08). `Regression:` is that command, checked like `Verify:`.
*/
function checkRecovery(sections, opts) {
const errors = [];
const warnings = [];
for (const s of sections) {
const f = parseStepFields(s.body);
if (f.has('Regression')) {
const r = runnableCheck(firstCommand(f.get('Regression').text), opts);
if (!r.runnable) {
errors.push(issue('PLAN_STEP_REGRESSION_NOT_RUNNABLE',
`Step ${s.n}: \`Regression:\` is not one runnable command (${r.reason})`,
'Backtick ONE command the criteria runner allows, unpiped and without `&&` (`npm test`); ' +
`the executor runs it green before the checkpoint. ${WAY_IN}`));
}
}
if (f.has('On failure') && onFailureAction(f.get('On failure').text) === null) {
warnings.push(issue('PLAN_STEP_ON_FAILURE_UNKNOWN',
`Step ${s.n}: \`On failure:\` does not open with a recovery word the executor reads`,
`Open with one of ${ON_FAILURE_ACTIONS.join(' | ')}, then the specifics (\`retry once, then escalate\`).`));
}
if (stepTestsOnly(s.body).testsOnly) {
// The run-the-thing script alone is such a step too: `End-to-end:` rides on
// the last step that builds behaviour (release review of 6.3.0, 2026-10-10).
const e2e = f.has('End-to-end')
? ' `End-to-end:` rides on the last step that builds behaviour, with its script in that step\'s `Files:`, ' +
'not on a step of its own.'
: '';
warnings.push(issue('PLAN_STEP_TESTS_ONLY',
`Step ${s.n}: every file in \`Files:\` is a test; a step of only tests can never be proven red`,
'The test for a behaviour belongs in the step that builds it: at this step\'s start point the behaviour ' +
'already exists (an earlier step built it, or it was there before the plan), so its test passes there and ' +
'red proof rejects it (`not-red-at-base`). Move the test into the step that builds the behaviour; a test ' +
'meant to pass from the start (a golden or characterization test) is the `Regression:` of the step that ' +
'changes the code, not a step of its own.' + e2e));
}
}
return { errors, warnings };
}
/** Table body rows (skips the header and the |---| separator). */
function tableRows(text) {
const rows = text.split('\n').filter(l => l.trim().startsWith('|'));
return rows.slice(2).map(r => r.trim().replace(/^\||\|$/g, '').split('|').map(c => c.trim()));
}
function checkDesign(text, stepNumbers) {
const out = [];
const design = findSection(text, 'Design', 2);
if (!design) {
return [issue('PLAN_DESIGN_MISSING', 'Architectural plan without a `## Design` section',
'Add the design part (templates/design-template.md), or plan under `ceremony: small|bounded` if the brief says so.')];
}
const subs = splitSections(design.text, 3);
const titles = new Set(subs.filter(s => s.title).map(s => s.title.toLowerCase()));
const missing = DESIGN_SUBSECTIONS.filter(t => !titles.has(t.toLowerCase()));
if (missing.length > 0) {
out.push(issue('PLAN_DESIGN_SECTION_MISSING', `## Design lacks: ${missing.join(', ')}`,
`Required subsections: ${DESIGN_SUBSECTIONS.join(', ')}.`));
}
if (!findSection(text, 'Test Strategy', 2)) {
out.push(issue('PLAN_DESIGN_SECTION_MISSING', 'Architectural plan lacks `## Test Strategy`',
'The design part\'s test strategy lives in the plan\'s `## Test Strategy` section.'));
}
const sub = t => subs.find(s => s.title && s.title.toLowerCase() === t.toLowerCase());
const map = sub('Module Map');
if (map) {
const blocks = fencedBlocks(map.text, 'mermaid');
if (blocks.length === 0) {
out.push(issue('PLAN_MODULE_MAP_EMPTY', '### Module Map has no ```mermaid block',
'Draw the modules and their dependency direction as a mermaid graph.'));
}
// All blocks form one map: a cycle may run across two of them.
const parsed = blocks.map(b => parseMermaid(b));
const unparsed = parsed.flatMap(p => p.unparsed);
if (unparsed.length > 0) {
out.push(issue('PLAN_MODULE_MAP_UNPARSED', `Module map lines that look like links but give no edge: ${unparsed.join(' | ')}`,
'Write links as `a --> b` (ids of letters, digits, _ . - /); the validator cannot check what it cannot read.'));
}
const cycle = findModuleCycle(parsed.flatMap(p => p.edges));
if (cycle) {
out.push(issue('PLAN_MODULE_CYCLE', `Module map has a dependency cycle: ${cycle.join(' -> ')}`,
'Dependencies point one way. Break the cycle (extract the shared part, or invert one edge).'));
}
}
const rules = sub('Architecture Rules');
if (rules) {
const rows = tableRows(rules.text);
if (rows.length === 0) {
out.push(issue('PLAN_ARCH_RULE_NO_ENFORCER', '### Architecture Rules lists no rule',
'List at least one rule as a table row: | Rule | Enforced by | Proof |.'));
}
for (const row of rows) {
const line = row.join(' | ');
const checked = /checked by\s+`[^`]+`/i.test(line);
const advisory = /\badvisory\b/i.test(line) && /\.claude\/rules\/[^\s`|]+\.md/.test(line);
if (!checked && !advisory) {
out.push(issue('PLAN_ARCH_RULE_NO_ENFORCER', `Rule without an enforcer: ${row[0] || line}`,
'Each rule says "checked by `<command>`" or "advisory — `.claude/rules/<file>.md`".'));
continue;
}
if (checked) {
const proof = (row[2] ?? '').match(/Step (\d+)/);
if (!proof || !stepNumbers.includes(Number(proof[1]))) {
out.push(issue('PLAN_ARCH_RULE_NO_PROOF',
`Checked rule without an existing proving step: ${row[0] || line}`,
'Name the step (Proof column, "Step N") that shows the check pass, fail on a planted violation, and pass again.'));
}
}
}
}
return out;
}
export function validatePlanContent(rawText, opts = {}) {
const strict = opts.strict !== false;
// CRLF plans (an editor on Windows) used to fail every manifest with
// FM_SYNTAX: the YAML line parser does not expect a trailing \r.
const text = typeof rawText === 'string' ? rawText.replace(/\r\n?/g, '\n') : rawText;
const headRes = validatePlanHeadings(text, { strict });
const errors = [...headRes.errors];
const warnings = [...headRes.warnings];
const steps = headRes.parsed?.steps || [];
const sections = sliceSteps(text);
const manRes = validateAllManifests(sections);
errors.push(...manRes.errors);
warnings.push(...manRes.warnings);
if (steps.length > 0 && manRes.parsed.length !== steps.length) {
errors.push(issue(
'PLAN_MANIFEST_COUNT_MISMATCH',
`Step count (${steps.length}) does not equal manifest count (${manRes.parsed.length})`,
));
}
const planVersion = extractPlanVersion(text);
if (planVersion === null) {
warnings.push(issue('PLAN_NO_VERSION', `No plan_version detected; current target is ${PLAN_VERSION_CURRENT}`));
} else if (!PLAN_VERSIONS_SUPPORTED.includes(planVersion)) {
warnings.push(issue('PLAN_VERSION_MISMATCH', `plan_version=${planVersion}, current target is ${PLAN_VERSION_CURRENT}`));
}
const recovery = checkRecovery(sections, { cwd: opts.cwd });
if (strict) errors.push(...recovery.errors);
else warnings.push(...recovery.errors);
warnings.push(...recovery.warnings);
// Steps a run appended (the review loop's remediation steps, /trekexecute
// Phase 7.2) must be runnable steps whatever the plan's version: below 1.8
// the step rules never ran, and a review step whose Verify is "re-run
// /trekreview" validated and was executed (release review of 6.3.0,
// 2026-10-10). From 1.8 the rules below already cover every step.
const stepsFrom = Number.isInteger(opts.stepsFrom) && opts.stepsFrom > 0 ? opts.stepsFrom : null;
if (stepsFrom !== null && !versionAtLeast(planVersion, '1.8')) {
const appended = sections.filter((s) => Number(s.n) >= stepsFrom);
const stepIssues = [
...checkSteps(appended).filter((i) => i.code !== 'PLAN_NO_END_TO_END'),
...checkRunnable('', appended, { cwd: opts.cwd }),
];
for (const s of appended) {
if (!parseStepFields(s.body).has('Verify')) {
stepIssues.push(issue('PLAN_STEP_VERIFY_NOT_RUNNABLE', `Step ${s.n}: no \`Verify:\` field`,
'An appended step needs one test command: proof of red and the done line run it.'));
}
}
errors.push(...stepIssues);
}
const ceremony = extractPlanCeremony(text);
if (versionAtLeast(planVersion, '1.8')) {
const v18 = [...checkSteps(sections), ...checkRunnable(text, sections, { cwd: opts.cwd })];
if (!ceremony.valid) {
v18.push(issue('PLAN_CEREMONY_INVALID', `ceremony=${ceremony.raw}`,
`Allowed: ${CEREMONY_VALUES.join(' | ')} (absent = architectural).`));
} else if (ceremony.value === 'architectural') {
v18.push(...checkDesign(text, steps.map(s => s.n)));
}
if (strict) errors.push(...v18);
else warnings.push(...v18);
}
// v4.1 SC #20 — MANIFEST_PROFILE_DRIFT detection. Strict-mode only.
// If plan frontmatter declares `profile: <name>` and any step manifest
// declares `profile_used: <other>`, emit a warning (NOT an error) so
// operators see drift but parsing remains forward-compat.
if (strict) {
const planFm = parseDocument(text).parsed?.frontmatter;
const planProfile =
planFm && typeof planFm.profile === 'string' ? planFm.profile : null;
if (planProfile) {
for (const m of manRes.parsed) {
const stepProfile = m.manifest && m.manifest.profile_used;
if (typeof stepProfile === 'string' && stepProfile !== planProfile) {
warnings.push(issue(
'MANIFEST_PROFILE_DRIFT',
`step ${m.n}: profile_used = ${stepProfile}, plan profile = ${planProfile}`,
'A step manifest declares a different profile than the plan frontmatter; ' +
'verify whether this is intentional (manual override) or accidental drift.',
));
}
}
}
}
return {
valid: errors.length === 0,
errors,
warnings,
parsed: { steps, manifests: manRes.parsed, planVersion, ceremony: ceremony.value },
};
}
export function validatePlan(filePath, opts = {}) {
if (!existsSync(filePath)) return fail(issue('PLAN_NOT_FOUND', `File not found: ${filePath}`));
let text;
try { text = readFileSync(filePath, 'utf-8'); }
catch (e) { return fail(issue('PLAN_READ_ERROR', `Cannot read ${filePath}: ${e.message}`)); }
const r = validatePlanContent(text, opts);
return { ...r, parsed: { ...r.parsed, filePath } };
}
// realpath on both sides: `file://${argv[1]}` silently no-ops under a symlink
// or a path with a space (the CLI then exits 0 having checked nothing).
function isMain() {
try {
return realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url));
} catch {
return false;
}
}
if (isMain()) {
const args = process.argv.slice(2);
const strict = !args.includes('--soft');
const at = args.indexOf('--steps-from');
const stepsFrom = at >= 0 ? Number(args[at + 1]) : null;
const filePath = args.find((a, i) => !a.startsWith('--') && !(at >= 0 && i === at + 1));
if (!filePath || (at >= 0 && !(Number.isInteger(stepsFrom) && stepsFrom > 0))) {
process.stderr.write('Usage: plan-validator.mjs [--strict|--soft] [--json] [--steps-from <N>] <plan.md>\n');
process.exit(2);
}
const r = validatePlan(filePath, { strict, stepsFrom });
if (args.includes('--json')) {
process.stdout.write(JSON.stringify({
valid: r.valid,
errors: r.errors,
warnings: r.warnings,
steps: r.parsed?.steps?.length ?? 0,
planVersion: r.parsed?.planVersion ?? null,
ceremony: r.parsed?.ceremony ?? null,
}, null, 2) + '\n');
} else {
process.stdout.write(`plan-validator: ${r.valid ? 'READY' : 'FAIL'} ${filePath} (${r.parsed?.steps?.length ?? 0} steps)\n`);
for (const e of r.errors) process.stderr.write(` ERROR [${e.code}] ${e.message}\n`);
for (const w of r.warnings) process.stderr.write(` WARN [${w.code}] ${w.message}\n`);
}
process.exit(r.valid ? 0 : 1);
}