refactor(simplify): one source per rule, shared test helpers, a faster isolation test [skip-docs]
/simplify over a97efb6..HEAD (the plan_version 1.9 / trial-4 delivery). - coordinator-contract: engine names come from engine-findings ENGINES; one markedMajorCodeReview predicate under both lowering checks; isEngineFinding everywhere; the unreachable branch (c) of illegalBriefLowering folded into (e). - armB workflow: RULE_KEYS = Object.keys(RULE_TIERS); the engine map is a literal pinned equal to ENGINES by the workflow test. - plan-schema: usesFilesGrammar() is the one 1.9 check (validator, sweep, testFirstFiles, stepTestsOnlyFromFields); opensEntry built on opensChunk. - plan-validator: one issue builder for the Files and Test first grammar. - plan-scope-sweep: the 1.9 and the 6.2.0 reader are one if/else. - artifact-commit: TRAILER_RE/trailerProblem no longer exported (no importer); the order comment says the CLI finds the repo root first. - tests: the lock drivers in session-id-isolation (d) run side by side per mode (~100 s -> ~30 s); a shared artifact-repo helper; severityRank and extractFindingsBlock reused; childEnv(env) without a spread; one copy of the PLAN_VERSION_CURRENT check. Plan readers: validator errors/warnings, plannedPaths, fieldPaths, testFirstFiles and stepTestsOnlyFromFields compared byte for byte against HEAD over 510 plans / 1692 steps plus 210 synthetic 1.7-1.9 cases: same. Suite 2384 (2382/0/2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
acfd2eb9b6
commit
ed6699da58
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
23 changed files with 201 additions and 204 deletions
|
|
@ -155,6 +155,9 @@ export function versionAtLeast(v, min) {
|
|||
return true;
|
||||
}
|
||||
|
||||
/** True when plan_version `v` reads `Files:` and the `Test first:` `File:` line by the 1.9 grammar. */
|
||||
export const usesFilesGrammar = (v) => versionAtLeast(v, '1.9');
|
||||
|
||||
// ---- ceremony (the brief's size class) -------------------------------------
|
||||
|
||||
export const CEREMONY_VALUES = Object.freeze(['small', 'bounded', 'architectural']);
|
||||
|
|
@ -383,10 +386,6 @@ const isFileWord = (w) => BUILD_FILE.test(w) || (isPathWord(w) && (FILE_OR_DIR.t
|
|||
// plain file with a slash, or a plain file alone on the line or followed by a
|
||||
// colon or a dash ("src/cli.mjs", "package.json", "Makefile — a target"), not
|
||||
// "child_process.spawnSync, path.join" or "Node.js 22 only" (round 10).
|
||||
const opensEntry = (tokens) => {
|
||||
const w = plainWord(tokens[0]);
|
||||
return isFileWord(w) && (w.includes('/') || tokens.length === 1 || tokens[0].endsWith(':') || DESCRIPTION_DASH.test(tokens[1]));
|
||||
};
|
||||
// A comma entry inside a description that opens a new entry: opensEntry's test
|
||||
// without "alone on the line", so "path.join" after a comma stays description
|
||||
// (6.4 / regression A).
|
||||
|
|
@ -394,6 +393,7 @@ const opensChunk = (tokens) => {
|
|||
const w = plainWord(tokens[0]);
|
||||
return isFileWord(w) && (w.includes('/') || tokens[0].endsWith(':') || DESCRIPTION_DASH.test(tokens[1]));
|
||||
};
|
||||
const opensEntry = (tokens) => opensChunk(tokens) || (tokens.length === 1 && isFileWord(plainWord(tokens[0])));
|
||||
|
||||
export function fieldPaths(text) {
|
||||
const spans = [];
|
||||
|
|
@ -420,11 +420,11 @@ export function fieldPaths(text) {
|
|||
if (entry === '') continue;
|
||||
const tokens = entry.split(/\s+/);
|
||||
const first = /^\u0000(\d+)\u0000/.exec(entry);
|
||||
if (first && isTicked[Number(first[1])]) described = false;
|
||||
if (described && opensChunk(tokens)) described = false;
|
||||
// An entry opened by a backticked path: after a word of description a
|
||||
// plain path with a slash counts only where a list starts (6.4 / regression B).
|
||||
const tickedEntry = Boolean(first && isTicked[Number(first[1])]);
|
||||
if (tickedEntry) described = false;
|
||||
if (described && opensChunk(tokens)) described = false;
|
||||
let prose = false;
|
||||
// A backticked capitalised name alone in its entry, before any
|
||||
// description, is a file (rounds 6, 10).
|
||||
|
|
@ -448,8 +448,9 @@ export function fieldPaths(text) {
|
|||
}
|
||||
if (DESCRIPTION_DASH.test(t)) described = true;
|
||||
const w = plainWord(t);
|
||||
if (!isFileWord(w)) prose = true;
|
||||
if (described || !isFileWord(w)) continue;
|
||||
const file = isFileWord(w);
|
||||
if (!file) prose = true;
|
||||
if (described || !file) continue;
|
||||
if ((w.includes('/') && !(tickedEntry && prose)) || i === 0 || LIST_JOINER.test(tokens[i - 1])) out.push(w);
|
||||
if (t.endsWith(':')) described = true;
|
||||
}
|
||||
|
|
@ -622,7 +623,7 @@ export function testFirstFileEntries(fields) {
|
|||
export function testFirstFiles(fields, version) {
|
||||
if (!fields || !fields.has('Test first')) return [];
|
||||
const out = [];
|
||||
if (versionAtLeast(version, '1.9')) {
|
||||
if (usesFilesGrammar(version)) {
|
||||
for (const entry of testFirstFileEntries(fields)) {
|
||||
for (const p of entryPaths(entry).files) if (!out.includes(p)) out.push(p);
|
||||
}
|
||||
|
|
@ -655,7 +656,7 @@ function stepTestFile(f, version) {
|
|||
/** `stepTestsOnly` for a caller that already holds the step's parsed fields. */
|
||||
export function stepTestsOnlyFromFields(f, version) {
|
||||
const text = f.has('Files') ? f.get('Files').text : null;
|
||||
const files = text === null ? [] : versionAtLeast(version, '1.9') ? entryPaths(text).files : fieldPaths(text);
|
||||
const files = text === null ? [] : usesFilesGrammar(version) ? entryPaths(text).files : fieldPaths(text);
|
||||
return { testsOnly: files.length > 0 && files.every(stepTestFile(f, version)), files };
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -34,6 +34,7 @@
|
|||
import { SEVERITY_VALUES, RULE_KEYS, getRule } from './rule-catalogue.mjs';
|
||||
import { computeFindingId } from '../parsers/finding-id.mjs';
|
||||
import { validateFindings } from './findings-schema.mjs';
|
||||
import { ENGINES } from './engine-findings.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
export const JUDGE_TITLE_MAX = 100;
|
||||
|
|
@ -96,16 +97,12 @@ export const UNVERIFIED_REASONS = Object.freeze([
|
|||
// The verdict gate (verdict-gate.mjs) imports `illegalBriefLowering`; the
|
||||
// opt-in workflow review mirrors it. `original_severity` is never read here.
|
||||
|
||||
const ENGINE_NAMES = Object.freeze(['code-review', 'security-review']);
|
||||
|
||||
// The payload `reviewer` names lib/review/engine-findings.mjs writes, and the
|
||||
// engine each one stands for.
|
||||
const ENGINE_OF_PAYLOAD = Object.freeze({
|
||||
'claude-code-review': 'code-review',
|
||||
'claude-security-review': 'security-review',
|
||||
});
|
||||
const ENGINE_OF_PAYLOAD = Object.freeze(Object.fromEntries(
|
||||
Object.entries(ENGINES).map(([engine, spec]) => [spec.reviewer, engine])));
|
||||
|
||||
const isEngineFinding = (f) => ENGINE_NAMES.includes(f.engine);
|
||||
const isEngineFinding = (f) => Object.hasOwn(ENGINES, f.engine ?? '');
|
||||
|
||||
function carries(f, field) {
|
||||
return f?.[field] !== undefined && f?.[field] !== null;
|
||||
|
|
@ -124,16 +121,17 @@ function isMark(v) {
|
|||
* @returns {boolean}
|
||||
*/
|
||||
export function lowerableBriefConflict(f) {
|
||||
return markedMajorCodeReview(f) && f.severity === 'MAJOR';
|
||||
}
|
||||
|
||||
function markedMajorCodeReview(f) {
|
||||
return isMark(f?.contradicts_brief) &&
|
||||
f.engine === 'code-review' &&
|
||||
getRule(f.rule_key)?.severity === 'MAJOR' &&
|
||||
f.severity === 'MAJOR';
|
||||
getRule(f.rule_key)?.severity === 'MAJOR';
|
||||
}
|
||||
|
||||
function isLegalBriefLowering(f) {
|
||||
return isMark(f?.contradicts_brief) &&
|
||||
f.engine === 'code-review' &&
|
||||
getRule(f.rule_key)?.severity === 'MAJOR' &&
|
||||
return markedMajorCodeReview(f) &&
|
||||
f.severity === 'MINOR' &&
|
||||
f.brief_lowered_from === 'MAJOR';
|
||||
}
|
||||
|
|
@ -152,14 +150,13 @@ export function illegalBriefLowering(f) {
|
|||
if (f === null || typeof f !== 'object') return false;
|
||||
if (isLegalBriefLowering(f)) return false;
|
||||
const marked = carries(f, 'contradicts_brief');
|
||||
const engine = ENGINE_NAMES.includes(f.engine);
|
||||
const engine = isEngineFinding(f);
|
||||
const rule = getRule(f.rule_key);
|
||||
const belowTier = rule !== null && severityRank(f.severity) > severityRank(rule.severity);
|
||||
if (carries(f, 'brief_lowered_from')) return true; // (a)
|
||||
if (marked && !engine) return true; // (b)
|
||||
if (marked && belowTier) return true; // (c)
|
||||
if (marked && rule === null) return true; // (d)
|
||||
if (engine && belowTier) return true; // (e)
|
||||
if (engine && belowTier) return true; // (c) and (e): a marked finding is an engine one past (b)
|
||||
return false;
|
||||
}
|
||||
|
||||
|
|
@ -409,7 +406,7 @@ export function reasonablenessFilter(findings) {
|
|||
// `illegalBriefLowering` false.
|
||||
const { brief_lowered_from: _lowered, ...rest } = incoming;
|
||||
let f = rest;
|
||||
if (!ENGINE_NAMES.includes(f.engine) && 'contradicts_brief' in f) {
|
||||
if (!isEngineFinding(f) && 'contradicts_brief' in f) {
|
||||
const { contradicts_brief: _mark, ...unmarked } = f;
|
||||
f = unmarked;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -16,8 +16,9 @@
|
|||
// trailer, a multi-line one, one that does not match (an unfilled `{model}`
|
||||
// placeholder, no `<email>`, another key) or one holding `Claude-Session:` is
|
||||
// refused with exit 1. Order: the `--store` check first (exit 2), then the
|
||||
// trailer (exit 1), both before any git call, so "nothing to commit" cannot
|
||||
// mask a bad trailer.
|
||||
// trailer (exit 1), both before commitArtifacts makes any git call (the CLI
|
||||
// only finds the repo root first), so "nothing to commit" cannot mask a bad
|
||||
// trailer.
|
||||
//
|
||||
// CLI:
|
||||
// node lib/util/artifact-commit.mjs --store <git|local> [--artifact <path>]...
|
||||
|
|
@ -37,10 +38,10 @@ import { isMain } from './is-main.mjs';
|
|||
export const ARTIFACT_STORES = Object.freeze(['git', 'local']);
|
||||
const DEFAULT_MESSAGE = 'docs(voyage): record approval';
|
||||
// One `Co-Authored-By:` line; `{`/`}` are refused so an unfilled `{model}` fails.
|
||||
export const TRAILER_RE = /^Co-Authored-By: [^<>{}\n]+ <[^\s<>@]+@[^\s<>]+>$/;
|
||||
const TRAILER_RE = /^Co-Authored-By: [^<>{}\n]+ <[^\s<>@]+@[^\s<>]+>$/;
|
||||
|
||||
/** @returns {string|null} why `trailer` is refused, or null when it is valid */
|
||||
export function trailerProblem(trailer) {
|
||||
function trailerProblem(trailer) {
|
||||
if (typeof trailer !== 'string' || trailer === '') {
|
||||
return '--trailer "Co-Authored-By: <name> <<email>>" is required — nothing committed';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -53,7 +53,7 @@ import { readFileSync, existsSync } from 'node:fs';
|
|||
import {
|
||||
sliceSteps, validatePlanHeadings, extractPlanVersion, extractPlanCeremony,
|
||||
parseStepFields, findSection, splitSections, findModuleCycle, parseMermaid, fencedBlocks,
|
||||
versionAtLeast, PLAN_VERSION_CURRENT, PLAN_VERSIONS_SUPPORTED, CEREMONY_VALUES,
|
||||
versionAtLeast, usesFilesGrammar, PLAN_VERSION_CURRENT, PLAN_VERSIONS_SUPPORTED, CEREMONY_VALUES,
|
||||
ON_FAILURE_ACTIONS, onFailureAction, stepTestsOnlyFromFields, entryPaths, untickedPaths,
|
||||
testFirstFileEntries, testFirstFiles,
|
||||
} from '../parsers/plan-schema.mjs';
|
||||
|
|
@ -77,7 +77,7 @@ const BACKTICKED = /`[^`\n]+`/;
|
|||
function checkSteps(sections, planVersion) {
|
||||
const out = [];
|
||||
let endToEnd = 0;
|
||||
const grammar = versionAtLeast(planVersion, '1.9');
|
||||
const grammar = usesFilesGrammar(planVersion);
|
||||
for (const s of sections) {
|
||||
const f = parseStepFields(s.body);
|
||||
const tf = f.get('Test first');
|
||||
|
|
@ -217,42 +217,38 @@ function checkRecovery(sections, opts, planVersion) {
|
|||
* names no file. Each `Test first:` `File:` line (plan-schema
|
||||
* `testFirstFileEntries`) is one entry under the same grammar.
|
||||
*/
|
||||
// How checkPathGrammar words an issue in each of the two places the grammar applies.
|
||||
const FILES_WORDING = {
|
||||
subject: 'a `Files:` entry', none: '`Files:` names no file', spans: 'an entry are its files',
|
||||
opens: 'each `Files:` entry opens with its backticked path(s)',
|
||||
example: '`` `src/a.mjs` - the parser ``', tail: ' A step that changes no file names none.',
|
||||
};
|
||||
const TEST_FIRST_WORDING = {
|
||||
subject: 'a `Test first:` `File:` line', none: 'a `Test first:` `File:` line names no file',
|
||||
spans: 'the `File:` line are the test files', opens: 'the `File:` line opens with its backticked test file(s)',
|
||||
example: '`` File: `tests/a.test.mjs` (new) ``', tail: '',
|
||||
};
|
||||
|
||||
function pathGrammarIssue(n, code, shown, w) {
|
||||
if (code === 'PLAN_STEP_PATH_NOT_A_PATH') {
|
||||
return issue(code, `Step ${n}: ${w.subject} opens with a backticked span that is no path: ${shown}`,
|
||||
`plan_version 1.9: the leading backticked spans of ${w.spans}; a call (\`parse()\`), an option ` +
|
||||
'or a URL is description and goes after the path.');
|
||||
}
|
||||
return issue('PLAN_STEP_PATH_NOT_FIRST',
|
||||
shown === '' ? `Step ${n}: ${w.none}` : `Step ${n}: ${w.subject} does not open with a backticked path: ${shown}`,
|
||||
`plan_version 1.9: ${w.opens}, separated by commas or spaces; the description follows (${w.example}).${w.tail}`);
|
||||
}
|
||||
|
||||
function checkPathGrammar(sections) {
|
||||
const out = [];
|
||||
for (const s of sections) {
|
||||
const f = parseStepFields(s.body);
|
||||
const { issues } = entryPaths(f.has('Files') ? f.get('Files').text : '');
|
||||
for (const i of issues) {
|
||||
if (i.code === 'PLAN_STEP_PATH_NOT_A_PATH') {
|
||||
out.push(issue('PLAN_STEP_PATH_NOT_A_PATH',
|
||||
`Step ${s.n}: a \`Files:\` entry opens with a backticked span that is no path: ${i.entry}`,
|
||||
'plan_version 1.9: the leading backticked spans of an entry are its files; a call (`parse()`), an option ' +
|
||||
'or a URL is description and goes after the path.'));
|
||||
} else {
|
||||
out.push(issue('PLAN_STEP_PATH_NOT_FIRST',
|
||||
i.entry === ''
|
||||
? `Step ${s.n}: \`Files:\` names no file`
|
||||
: `Step ${s.n}: a \`Files:\` entry does not open with a backticked path: ${i.entry}`,
|
||||
'plan_version 1.9: each `Files:` entry opens with its backticked path(s), separated by commas or spaces; ' +
|
||||
'the description follows (`` `src/a.mjs` - the parser ``). A step that changes no file names none.'));
|
||||
}
|
||||
for (const i of entryPaths(f.has('Files') ? f.get('Files').text : '').issues) {
|
||||
out.push(pathGrammarIssue(s.n, i.code, i.entry, FILES_WORDING));
|
||||
}
|
||||
for (const entry of testFirstFileEntries(f)) {
|
||||
for (const i of entryPaths(entry).issues) {
|
||||
if (i.code === 'PLAN_STEP_PATH_NOT_A_PATH') {
|
||||
out.push(issue('PLAN_STEP_PATH_NOT_A_PATH',
|
||||
`Step ${s.n}: a \`Test first:\` \`File:\` line opens with a backticked span that is no path: ${entry}`,
|
||||
'plan_version 1.9: the leading backticked spans of the `File:` line are the test files; a call ' +
|
||||
'(`parse()`), an option or a URL is description and goes after the path.'));
|
||||
} else {
|
||||
out.push(issue('PLAN_STEP_PATH_NOT_FIRST',
|
||||
entry === ''
|
||||
? `Step ${s.n}: a \`Test first:\` \`File:\` line names no file`
|
||||
: `Step ${s.n}: a \`Test first:\` \`File:\` line does not open with a backticked path: ${entry}`,
|
||||
'plan_version 1.9: the `File:` line opens with its backticked test file(s), separated by commas or ' +
|
||||
'spaces; the description follows (`` File: `tests/a.test.mjs` (new) ``).'));
|
||||
}
|
||||
}
|
||||
for (const i of entryPaths(entry).issues) out.push(pathGrammarIssue(s.n, i.code, entry, TEST_FIRST_WORDING));
|
||||
}
|
||||
}
|
||||
return out;
|
||||
|
|
@ -369,7 +365,7 @@ export function validatePlanContent(rawText, opts = {}) {
|
|||
warnings.push(...recovery.warnings);
|
||||
|
||||
// The `Files:` grammar (decision 16): refused from 1.9, a warning below it.
|
||||
if (versionAtLeast(planVersion, '1.9')) {
|
||||
if (usesFilesGrammar(planVersion)) {
|
||||
const grammar = checkPathGrammar(sections);
|
||||
if (strict) errors.push(...grammar);
|
||||
else warnings.push(...grammar);
|
||||
|
|
@ -397,7 +393,7 @@ export function validatePlanContent(rawText, opts = {}) {
|
|||
if (stepsFrom !== null) {
|
||||
const appended = sections.filter((s) => Number(s.n) >= stepsFrom);
|
||||
const stepIssues = versionAtLeast(planVersion, '1.8') ? [] : [
|
||||
...checkSteps(appended, planVersion).filter((i) => i.code !== 'PLAN_NO_END_TO_END'),
|
||||
...checkSteps(appended).filter((i) => i.code !== 'PLAN_NO_END_TO_END'),
|
||||
...checkRunnable(text, appended, { cwd: opts.cwd, stepsOnly: true }),
|
||||
];
|
||||
for (const s of appended) {
|
||||
|
|
|
|||
|
|
@ -38,7 +38,7 @@
|
|||
import { readFileSync, existsSync, realpathSync } from 'node:fs';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { resolve, relative, dirname, isAbsolute, sep } from 'node:path';
|
||||
import { sliceSteps, parseStepFields, entryPaths, testFirstFiles, extractPlanVersion, versionAtLeast } from '../parsers/plan-schema.mjs';
|
||||
import { sliceSteps, parseStepFields, entryPaths, testFirstFiles, extractPlanVersion, usesFilesGrammar } from '../parsers/plan-schema.mjs';
|
||||
import { parseManifest } from '../parsers/manifest-yaml.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
|
|
@ -66,7 +66,7 @@ const norm = p => p.replace(/\\/g, '/').replace(/^\.\//, '');
|
|||
export function plannedPaths(planText) {
|
||||
const out = new Set();
|
||||
const version = extractPlanVersion(planText);
|
||||
const grammar = versionAtLeast(version, '1.9');
|
||||
const grammar = usesFilesGrammar(version);
|
||||
for (const s of sliceSteps(planText)) {
|
||||
const f = parseStepFields(s.body);
|
||||
const files = f.get('Files');
|
||||
|
|
@ -74,25 +74,26 @@ export function plannedPaths(planText) {
|
|||
// plan_version 1.9: the grammar decides what each entry names.
|
||||
if (files) for (const p of entryPaths(files.text).files) out.add(norm(p));
|
||||
for (const p of testFirstFiles(f, version)) out.add(norm(p));
|
||||
}
|
||||
// v6.2.0's own reading, kept as it was (operator 2026-10-11): every
|
||||
// backticked span that looks like a path, and plain words only when the
|
||||
// field has none. A field that names its paths in backticks plans those,
|
||||
// never a path its description mentions. Release review of 6.3.0, rounds
|
||||
// 3-5: reading Files: through the tests-only check's reader
|
||||
// (plan-schema.mjs fieldPaths) dropped paths v6.2.0 planned and planned
|
||||
// directories a description named, so K6 has its own reader again; the
|
||||
// two answer different questions (what may change; is every named file a test).
|
||||
if (files && !grammar) {
|
||||
const ticked = pathsIn(files.text);
|
||||
// Older plans write bare paths: `- Files: a.ts, b.ts`.
|
||||
const bare = ticked.length ? [] : files.text.split(/[,\s]+/).filter(looksLikePath);
|
||||
for (const p of [...ticked, ...bare]) out.add(norm(p));
|
||||
}
|
||||
const tf = f.get('Test first');
|
||||
if (tf && !grammar) {
|
||||
for (const line of tf.text.split('\n')) {
|
||||
if (/\bFile:/i.test(line)) for (const p of pathsIn(line)) out.add(norm(p));
|
||||
} else {
|
||||
// v6.2.0's own reading, kept as it was (operator 2026-10-11): every
|
||||
// backticked span that looks like a path, and plain words only when the
|
||||
// field has none. A field that names its paths in backticks plans those,
|
||||
// never a path its description mentions. Release review of 6.3.0, rounds
|
||||
// 3-5: reading Files: through the tests-only check's reader
|
||||
// (plan-schema.mjs fieldPaths) dropped paths v6.2.0 planned and planned
|
||||
// directories a description named, so K6 has its own reader again; the
|
||||
// two answer different questions (what may change; is every named file a test).
|
||||
if (files) {
|
||||
const ticked = pathsIn(files.text);
|
||||
// Older plans write bare paths: `- Files: a.ts, b.ts`.
|
||||
const bare = ticked.length ? [] : files.text.split(/[,\s]+/).filter(looksLikePath);
|
||||
for (const p of [...ticked, ...bare]) out.add(norm(p));
|
||||
}
|
||||
const tf = f.get('Test first');
|
||||
if (tf) {
|
||||
for (const line of tf.text.split('\n')) {
|
||||
if (/\bFile:/i.test(line)) for (const p of pathsIn(line)) out.add(norm(p));
|
||||
}
|
||||
}
|
||||
}
|
||||
const man = parseManifest(s.body);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue