open/voyage
1
0
Fork 0
Code Issues Releases 13 Activity

refactor(plan): the plan validator parses each step once

Browse source 
Review finding 93382464.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-13 14:47:33 +02:00
commit 045191f51d
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 125 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

20
lib/validators/plan-validator.mjs
View file

@ -70,6 +70,12 @@ export const DESIGN_SUBSECTIONS = Object.freeze([
]);
const BACKTICKED = /`[^`\n]+`/;
// Each step's fields are parsed once, here; every check reads `s.fields`
// (review finding 93382464).
function withFields(sections) {
return sections.map((s) => ({ ...s, fields: parseStepFields(s.body) }));
}
// From 1.9 the step names its test the way the executor reads it
// (testFirstFiles): only through a line that opens with `File:` and its
// backticked path, so a `File:` mid-line names no test. Below 1.9 the
@ -79,7 +85,7 @@ function checkSteps(sections, planVersion) {
let endToEnd = 0;
const grammar = usesFilesGrammar(planVersion);
for (const s of sections) {
const f = parseStepFields(s.body);
const f = s.fields;
const tf = f.get('Test first');
const file = grammar
? testFirstFiles(f, planVersion).length > 0
@ -138,7 +144,7 @@ function checkRunnable(text, sections, opts) {
}
const verified = new Set(criteria.filter(c => !refusal(c)).map(c => c.command.trim().replace(/\s+/g, ' ')));
for (const s of sections) {
const f = parseStepFields(s.body);
const f = s.fields;
if (f.has('Verify')) {
const v = firstCommand(f.get('Verify').text);
const r = runnableCheck(v, opts);
@ -177,7 +183,7 @@ function checkRecovery(sections, opts, planVersion) {
const errors = [];
const warnings = [];
for (const s of sections) {
const f = parseStepFields(s.body);
const f = s.fields;
if (f.has('Regression')) {
const r = runnableCheck(firstCommand(f.get('Regression').text), opts);
if (!r.runnable) {
@ -244,7 +250,7 @@ function pathGrammarIssue(n, code, shown, w) {
function checkPathGrammar(sections) {
const out = [];
for (const s of sections) {
const f = parseStepFields(s.body);
const f = s.fields;
for (const i of entryPaths(f.has('Files') ? f.get('Files').text : '').issues) {
out.push(pathGrammarIssue(s.n, i.code, i.entry, FILES_WORDING));
}
@ -341,7 +347,7 @@ export function validatePlanContent(rawText, opts = {}) {
const warnings = [...headRes.warnings];
const steps = headRes.parsed?.steps || [];
const sections = sliceSteps(text);
const sections = withFields(sliceSteps(text));
const manRes = validateAllManifests(sections);
errors.push(...manRes.errors);
warnings.push(...manRes.warnings);
@ -372,7 +378,7 @@ export function validatePlanContent(rawText, opts = {}) {
else warnings.push(...grammar);
} else {
for (const s of sections) {
const f = parseStepFields(s.body);
const f = s.fields;
const unticked = f.has('Files') ? [...new Set(untickedPaths(f.get('Files').text))] : [];
if (unticked.length > 0) {
warnings.push(issue('PLAN_STEP_PATH_UNTICKED',
@ -398,7 +404,7 @@ export function validatePlanContent(rawText, opts = {}) {
...checkRunnable(text, appended, { cwd: opts.cwd, stepsOnly: true }),
];
for (const s of appended) {
if (!parseStepFields(s.body).has('Verify')) {
if (!s.fields.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.'));
}

112
tests/validators/plan-validator-parse-once.test.mjs Normal file
View file

@ -0,0 +1,112 @@
// tests/validators/plan-validator-parse-once.test.mjs
// The plan validator parses each step's fields once (review finding 93382464):
// one private `withFields(sections)` maps each section to its parsed fields,
// and every check reads `s.fields` instead of calling `parseStepFields` again.
// The output must not change: the expected codes below were recorded from
// `validatePlanContent` at the start point of the change (1621352), each with
// the step number its message names, in order.
import { test } from 'node:test';
import { strict as assert } from 'node:assert';
import { readFileSync } from 'node:fs';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
import { buildPlan } from '../helpers/plan-v18-builder.mjs';
import { validatePlanContent } from '../../lib/validators/plan-validator.mjs';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const SOURCE = readFileSync(join(ROOT, 'lib', 'validators', 'plan-validator.mjs'), 'utf8');
// The body of a top-level function: from its declaration to the next
// top-level `function` or `export`. '' when the function is not there.
function bodyOf(name) {
const start = SOURCE.search(new RegExp(`^function ${name}\\(`, 'm'));
if (start < 0) return '';
const rest = SOURCE.slice(start + 1);
const next = rest.search(/^(?:async )?function |^export /m);
return next < 0 ? SOURCE.slice(start) : SOURCE.slice(start, start + 1 + next);
}
// Each issue as CODE@step (the step its message names), so order and place are pinned.
const tag = (x) => {
const m = /\b[Ss]tep (\d+)/.exec(x.message);
return m ? `${x.code}@${m[1]}` : x.code;
};
const codes = (r) => ({ errors: r.errors.map(tag), warnings: r.warnings.map(tag) });
const E2E = { endToEnd: 'bash tests/e2e.sh' };
const TESTS_ONLY = { file: 'tests/only.test.mjs', test: 'tests/only.test.mjs', manifestPaths: ['tests/only.test.mjs'] };
const STEP3 = '### Step 3: Fix it\n\n- **Files:** `src/fix.mjs`, `tests/fix.test.mjs`\n- **Changes:** fix it\n' +
'- **On failure:** escalate\n';
const STEP4 = '### Step 4: Check it\n\n- **Files:** `src/check.mjs`\n- **Changes:** check it\n' +
'- **Verify:** `curl http://localhost/x` → expected: 200\n- **Regression:** `npm test | cat` → expected: exit 0\n' +
'- **On failure:** maybe later\n';
const appended = (plan, ...steps) => plan.replace('\n## Test Strategy', `\n${steps.join('\n')}\n## Test Strategy`);
test('parse once: the validator source holds one parseStepFields( call, inside function withFields', () => {
const calls = SOURCE.match(/parseStepFields\(/g) ?? [];
assert.equal(calls.length, 1, `parseStepFields( occurs ${calls.length} times in lib/validators/plan-validator.mjs`);
const body = bodyOf('withFields');
assert.ok(body !== '', 'lib/validators/plan-validator.mjs declares function withFields');
assert.match(body, /parseStepFields\(/, 'the one parseStepFields( call stands inside function withFields');
});
for (const name of ['checkSteps', 'checkRunnable', 'checkRecovery', 'checkPathGrammar']) {
test(`parse once: ${name} reads .fields in its body`, () => {
const body = bodyOf(name);
assert.ok(body !== '', `function ${name} is declared`);
assert.match(body, /\.fields\b/, `${name} reads the parsed fields (.fields) instead of parsing again`);
});
}
test('parse once: a 1.7 and a 1.8 plan with a tests-only step give the codes recorded at the start point', () => {
for (const version of ['1.7', '1.8']) {
const plan = buildPlan({ version, steps: [TESTS_ONLY, E2E] });
assert.deepEqual(codes(validatePlanContent(plan)), { errors: [], warnings: ['PLAN_STEP_TESTS_ONLY@1'] }, version);
}
});
test('parse once: a 1.8 plan with an unticked path gives the codes recorded at the start point', () => {
const plan = buildPlan({ version: '1.8', steps: [{ filesRaw: '`src/mod1.mjs`, src/plain.mjs - the parser' }, E2E] });
const want = { errors: [], warnings: ['PLAN_STEP_PATH_UNTICKED@1'] };
assert.deepEqual(codes(validatePlanContent(plan)), want);
assert.deepEqual(codes(validatePlanContent(plan, { strict: false })), want);
});
test('parse once: a 1.9 plan with refused entries gives the codes recorded at the start point', () => {
const plan = buildPlan({
version: '1.9',
steps: [{ filesRaw: 'the parser `src/mod1.mjs`', testFirstRaw: ' - File: `parse()` then `tests/mod1.test.mjs`' }, E2E],
});
const refused = ['PLAN_STEP_PATH_NOT_FIRST@1', 'PLAN_STEP_PATH_NOT_A_PATH@1', 'PLAN_STEP_NO_TEST_FIRST@1'];
assert.deepEqual(codes(validatePlanContent(plan)), { errors: refused, warnings: [] });
assert.deepEqual(codes(validatePlanContent(plan, { strict: false })), { errors: [], warnings: refused });
});
test('parse once: --steps-from gives the codes recorded at the start point', () => {
assert.deepEqual(codes(validatePlanContent(appended(buildPlan({ version: '1.7' }), STEP3), { strict: true, stepsFrom: 3 })), {
errors: ['MANIFEST_MISSING@3', 'PLAN_STEP_NO_TEST_FIRST@3', 'PLAN_STEP_VERIFY_NOT_RUNNABLE@3'],
warnings: [],
});
assert.deepEqual(codes(validatePlanContent(appended(buildPlan({ version: '1.8' }), STEP3), { strict: true, stepsFrom: 3 })), {
errors: ['MANIFEST_MISSING@3', 'PLAN_STEP_VERIFY_NOT_RUNNABLE@3', 'PLAN_STEP_NO_TEST_FIRST@3'],
warnings: [],
});
const two = appended(buildPlan({ version: '1.7' }), STEP3, STEP4);
assert.deepEqual(codes(validatePlanContent(two, { strict: true, stepsFrom: 3 })), {
errors: [
'MANIFEST_MISSING@3', 'MANIFEST_MISSING@4', 'PLAN_STEP_REGRESSION_NOT_RUNNABLE@4',
'PLAN_STEP_NO_TEST_FIRST@3', 'PLAN_STEP_NO_TEST_FIRST@4',
'PLAN_STEP_VERIFY_NOT_RUNNABLE@4', 'PLAN_STEP_VERIFY_NOT_RUNNABLE@3',
],
warnings: ['PLAN_STEP_ON_FAILURE_UNKNOWN@4'],
});
assert.deepEqual(codes(validatePlanContent(two, { strict: false, stepsFrom: 3 })), {
errors: ['MANIFEST_MISSING@3', 'MANIFEST_MISSING@4'],
warnings: [
'PLAN_STEP_REGRESSION_NOT_RUNNABLE@4', 'PLAN_STEP_ON_FAILURE_UNKNOWN@4',
'PLAN_STEP_NO_TEST_FIRST@3', 'PLAN_STEP_NO_TEST_FIRST@4',
'PLAN_STEP_VERIFY_NOT_RUNNABLE@4', 'PLAN_STEP_VERIFY_NOT_RUNNABLE@3',
],
});
});