fix(validate): a check that can never run is caught when the brief or plan is written
voyage-lekerepo end-to-end trial (2026-10-06), report points 2, 3, 4, 9: 11 of 13 brief criteria were NOT RUN at review with no warning in /trekbrief; a plan `git grep` criterion passed validator, gate and three critic rounds, then felled the run in Phase 7; the End-to-end field and Phase 7's text search never met; a prose Verify left the test command undefined. - criteria-runner: runnableCheck() screens a declared check without running it (same allowlist; an npm run script is not looked up at write time); /bin/bash tests/<script> allowed; the plan report carries end_to_end, verified when a passed criterion runs a step's End-to-end command (the older text form still counts). - plan-validator 1.8: PLAN_VERIFICATION_NOT_RUNNABLE, PLAN_STEP_VERIFY_NOT_RUNNABLE, PLAN_END_TO_END_NOT_RUNNABLE, PLAN_END_TO_END_NOT_VERIFIED (the gate inherits them). - brief-validator: BRIEF_SC_NOT_RUNNABLE warning; /trekbrief offers a tests/ script; brief-reviewer no longer proposes unrunnable commands. - trekexecute: Phase 7 copies end_to_end from the runner; Verify's test command is its first backticked span. Measured on the trial's own brief and plan: 11 warnings (the 11 NOT RUN), and the plan stops on V5 git grep and the unlisted End-to-end command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
ca6ee8779a
commit
c56f852ab6
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
14 changed files with 422 additions and 38 deletions
|
|
@ -11,6 +11,7 @@ import { fileURLToPath } from 'node:url';
|
|||
import { parseDocument } from '../util/frontmatter.mjs';
|
||||
import { issue, ok, fail } from '../util/result.mjs';
|
||||
import { BASE_ALLOWED_MODELS } from './profile-validator.mjs';
|
||||
import { parseSuccessCriteria, runnableCheck } from '../verification/criteria-runner.mjs';
|
||||
|
||||
export const BRIEF_REQUIRED_FRONTMATTER = ['type', 'brief_version', 'task', 'slug', 'research_topics', 'research_status'];
|
||||
export const REVIEW_AS_BRIEF_REQUIRED_FRONTMATTER = ['type', 'task', 'slug', 'project_dir', 'findings'];
|
||||
|
|
@ -262,6 +263,26 @@ export function validateBriefContent(text, opts = {}) {
|
|||
}
|
||||
}
|
||||
|
||||
// A criterion that opens with a command the review's criteria runner can
|
||||
// never run is reported NOT RUN at /trekreview, long after anyone could fix
|
||||
// it: 11 of 13 in the voyage-lekerepo trial (2026-10-06), and nothing in
|
||||
// /trekbrief warned. A warning, not an error: a criterion may be one a human
|
||||
// judges by reading, and that stays allowed. Prose-only criteria (no
|
||||
// command) are not flagged.
|
||||
if (fm.type !== 'trekreview') {
|
||||
for (const c of parseSuccessCriteria(body)) {
|
||||
if (!c.command) continue;
|
||||
const r = runnableCheck(c, { cwd: opts.cwd });
|
||||
if (r.runnable) continue;
|
||||
warnings.push(issue(
|
||||
'BRIEF_SC_NOT_RUNNABLE',
|
||||
`Success criterion ${c.label} opens with \`${c.command}\`, which the review's criteria runner will never run (${r.reason})`,
|
||||
'Wrap the check in a script under tests/ and write the criterion as `bash tests/<script>.sh` exits 0, ' +
|
||||
'or name a test runner (`npm test`, `node --test <file>`); otherwise it is reported NOT RUN at review.',
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
if (typeof fm.brief_version === 'string' || typeof fm.brief_version === 'number') {
|
||||
const m = String(fm.brief_version).match(/^(\d+)\.(\d+)$/);
|
||||
if (!m) {
|
||||
|
|
|
|||
|
|
@ -15,7 +15,12 @@
|
|||
// - 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).
|
||||
// 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.
|
||||
|
||||
import { readFileSync, existsSync, realpathSync } from 'node:fs';
|
||||
|
|
@ -28,6 +33,7 @@ import {
|
|||
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([
|
||||
|
|
@ -69,6 +75,56 @@ function checkSteps(sections) {
|
|||
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;
|
||||
}
|
||||
|
||||
/** Table body rows (skips the header and the |---| separator). */
|
||||
function tableRows(text) {
|
||||
const rows = text.split('\n').filter(l => l.trim().startsWith('|'));
|
||||
|
|
@ -176,7 +232,7 @@ export function validatePlanContent(rawText, opts = {}) {
|
|||
|
||||
const ceremony = extractPlanCeremony(text);
|
||||
if (versionAtLeast(planVersion, '1.8')) {
|
||||
const v18 = [...checkSteps(sections)];
|
||||
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).`));
|
||||
|
|
|
|||
|
|
@ -38,6 +38,7 @@ import { readFileSync } from 'node:fs';
|
|||
import { resolve, dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { sliceSteps, parseStepFields } from '../parsers/plan-schema.mjs';
|
||||
|
||||
const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
|
||||
|
|
@ -135,7 +136,8 @@ export function looksLikeCommand(span) {
|
|||
|
||||
// The first backtick-delimited span on the line is the command by convention —
|
||||
// both templates put it first and a second span holds the expected output.
|
||||
function firstCommand(text) {
|
||||
// A plan step's `Verify:` and `End-to-end:` fields follow the same convention.
|
||||
export function firstCommand(text) {
|
||||
const m = text.match(/`([^`]+)`/);
|
||||
if (!m) return { command: null, reason: 'no-command' };
|
||||
const raw = m[1].trim();
|
||||
|
|
@ -273,10 +275,12 @@ function npmScripts(cwd) {
|
|||
// One entry per runner: given the command's tokens (and the working tree the
|
||||
// criteria run in), either allow it or say why not.
|
||||
const RUNNERS = {
|
||||
npm: (t, cwd) => {
|
||||
npm: (t, cwd, writeTime) => {
|
||||
if (t[1] === 'test') return permit();
|
||||
if (t[1] !== 'run') return deny(`npm, but not \`npm test\` or \`npm run\` (\`npm ${t[1] ?? ''}\`)`);
|
||||
if (t[2] === undefined) return deny('npm run names no script');
|
||||
// When a brief or plan is written, the script may be one the plan adds.
|
||||
if (writeTime) return permit();
|
||||
return npmScripts(cwd).has(t[2])
|
||||
? permit()
|
||||
: deny(`npm run names a script package.json does not declare (${t[2]})`);
|
||||
|
|
@ -292,6 +296,8 @@ const RUNNERS = {
|
|||
go: (t) => (t[1] === 'test' ? permit() : deny('go, but not `go test`')),
|
||||
make: (t) => (t[1] === 'test' ? permit() : deny('make, but not `make test`')),
|
||||
bash: (t) => (isTestScript(t[1]) ? permit() : deny('bash, but not a script under tests/')),
|
||||
// The system bash by its path (macOS: 3.2), so a check can pin the shell.
|
||||
'/bin/bash': (t) => (isTestScript(t[1]) ? permit() : deny('/bin/bash, but not a script under tests/')),
|
||||
git: (t) =>
|
||||
READONLY_GIT.has(t[1])
|
||||
? permit()
|
||||
|
|
@ -304,6 +310,25 @@ const RUNNERS = {
|
|||
* the package.json found there.
|
||||
*/
|
||||
export function allowedCommand(command, opts = {}) {
|
||||
return screenShape(command, opts, false);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a declared check could ever run, decided WITHOUT running it: the
|
||||
* same allowlist, used when a brief or a plan is written instead of only when
|
||||
* the run is judged. Measured in the voyage-lekerepo trial (2026-10-06): 11 of
|
||||
* 13 brief criteria were NOT RUN at review, and a plan criterion (`git grep`)
|
||||
* passed the validator, the plan gate and three critic rounds, then felled the
|
||||
* run in Phase 7. An `npm run` script is not looked up here: the plan may be
|
||||
* what adds it. Returns `{runnable, reason}`.
|
||||
*/
|
||||
export function runnableCheck(criterion, opts = {}) {
|
||||
if (!criterion?.command) return { runnable: false, reason: criterion?.reason || 'no command' };
|
||||
const r = screenShape(criterion.command, opts, true);
|
||||
return { runnable: r.allowed, reason: r.reason };
|
||||
}
|
||||
|
||||
function screenShape(command, opts, writeTime) {
|
||||
const cwd = resolve(opts.cwd ?? process.cwd());
|
||||
const text = String(command ?? '');
|
||||
if (text.trim() === '') return deny('the command is empty');
|
||||
|
|
@ -312,9 +337,8 @@ export function allowedCommand(command, opts = {}) {
|
|||
if (operator) return deny(`a shell operator (${operator[0]}) — one criterion declares one command, unpiped`);
|
||||
|
||||
const tokens = text.trim().split(/\s+/);
|
||||
const runner = RUNNERS[tokens[0]];
|
||||
if (!runner) return deny(`\`${tokens[0]}\` is not a known test runner`);
|
||||
return runner(tokens, cwd);
|
||||
if (!Object.hasOwn(RUNNERS, tokens[0])) return deny(`\`${tokens[0]}\` is not a known test runner`);
|
||||
return RUNNERS[tokens[0]](tokens, cwd, writeTime);
|
||||
}
|
||||
|
||||
// --- running ----------------------------------------------------------------
|
||||
|
|
@ -398,17 +422,52 @@ export function summarize(results, opts = {}) {
|
|||
return summary;
|
||||
}
|
||||
|
||||
function report(kind, source, criteria, opts, error) {
|
||||
/** The commands a plan's steps declare in `End-to-end:` (first backticked span each). */
|
||||
export function endToEndCommands(markdown) {
|
||||
return sliceSteps(String(markdown ?? '').replace(/\r\n?/g, '\n'))
|
||||
.map((s) => parseStepFields(s.body).get('End-to-end'))
|
||||
.filter(Boolean)
|
||||
.map((f) => firstCommand(f.text).command)
|
||||
.filter(Boolean);
|
||||
}
|
||||
|
||||
const normalizeCommand = (c) => String(c).trim().replace(/\s+/g, ' ');
|
||||
const SAYS_END_TO_END = /end to end|end-to-end|run the thing/i;
|
||||
|
||||
/**
|
||||
* Whether the run was verified end to end (P9 (6)). A passed criterion
|
||||
* verifies it when it runs a step's `End-to-end:` command — the field the plan
|
||||
* validator requires — or, the older form, when its text says it runs the
|
||||
* thing end to end. Searching the text alone missed a plan that had the field
|
||||
* and ran it green (voyage-lekerepo trial, 2026-10-06). A verification that
|
||||
* did not pass is never verified end to end.
|
||||
*/
|
||||
export function endToEndStatus(results, summary, e2eCommands = []) {
|
||||
const no = { status: 'not-verified', by: null, criterion: null };
|
||||
if (!summary.ok) return no;
|
||||
const passed = results.filter((r) => r.status === 'passed');
|
||||
const want = new Set(e2eCommands.map(normalizeCommand));
|
||||
const byField = passed.find((r) => r.command && want.has(normalizeCommand(r.command)));
|
||||
if (byField) return { status: 'verified', by: 'end-to-end-field', criterion: byField.label };
|
||||
const byText = passed.find((r) => SAYS_END_TO_END.test(r.text));
|
||||
if (byText) return { status: 'verified', by: 'criterion-text', criterion: byText.label };
|
||||
return no;
|
||||
}
|
||||
|
||||
function report(kind, source, criteria, opts, error, e2eCommands = []) {
|
||||
const requireCommand = kind === 'plan';
|
||||
const results = error ? [] : runCriteria(criteria, opts);
|
||||
return {
|
||||
const summary = summarize(results, { requireCommand });
|
||||
const rep = {
|
||||
kind,
|
||||
source,
|
||||
heading: kind === 'plan' ? PLAN_HEADING : BRIEF_HEADING,
|
||||
results,
|
||||
summary: summarize(results, { requireCommand }),
|
||||
summary,
|
||||
error: error ?? null,
|
||||
};
|
||||
if (kind === 'plan') rep.end_to_end = endToEndStatus(results, summary, e2eCommands);
|
||||
return rep;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -460,7 +519,7 @@ export function runPlanVerification(planPath, opts = {}) {
|
|||
if (criteria.length === 0) {
|
||||
return report('plan', planPath, [], opts, noCriteria(planPath, PLAN_HEADING, section.line));
|
||||
}
|
||||
return report('plan', planPath, criteria, opts);
|
||||
return report('plan', planPath, criteria, opts, null, endToEndCommands(md));
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -502,6 +561,12 @@ export function render(rep) {
|
|||
for (const line of r.output.split('\n').slice(0, 20)) out.push(` ${line}`);
|
||||
}
|
||||
}
|
||||
if (rep.end_to_end) {
|
||||
const e = rep.end_to_end;
|
||||
out.push(e.status === 'verified'
|
||||
? ` end to end: verified (${e.criterion}, ${e.by === 'end-to-end-field' ? "runs a step's End-to-end command" : 'its text says it runs the thing'})`
|
||||
: ' end to end: not verified end to end');
|
||||
}
|
||||
return out.join('\n');
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue