fix: round 3 of the release review of 6.3.0
Round 3 (v6.2.0..96cb977, BLOCK: 1 BLOCKER, 8 MAJOR, 2 MINOR), each fixed behind a test that failed first. - Every allowlisted runner, not only git, takes plain words and a short list of options that only read: an option that writes a file or loads code (--test-reporter-destination, --outputFile, --junitxml, --basetemp, --import, --config, -p, -o, -exec, a reporter path) is on no list; after `npm test --` only paths follow; `make test` takes no VAR=value. A built-in node test reporter (tap, spec, dot, junit, lcov) stays allowed. - /trekexecute: a restore the guard refuses (destructive git, in the mode that shows no prompts) or the operator declines is never recorded as reverted: simplify = "revert-refused" and the run stops before review; a refused step revert is reported with its paths. - stepTestsOnly: a fixture document under tests/ is a test file, "e.g." is not a path, ./ is dropped; the plan-file sweep reads Files: with the same reader (fieldPaths); the validator hands its parsed fields over. - plan-validator --steps-from asks for Verify: on a 1.8 plan too. - skill frontmatter: a value starting with # is a hazard; a quote after an escaped backslash closes the value. Routing reads kind tags with hasOwn. - release-check reads scope_sha_end as written (an all-digit sha keeps its leading zero), and Voyage's own artifacts under .claude/projects/ and .claude/voyage/ committed after the review do not count as unreviewed. Suite 2170 (2168/0/2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
96cb977856
commit
0e612c88ac
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
14 changed files with 284 additions and 40 deletions
|
|
@ -293,7 +293,15 @@ export function onFailureAction(text) {
|
|||
* so a bare `Makefile` is still a path. Release review of 6.3.0, rounds 1-2.
|
||||
*/
|
||||
const PATH_WORD = /^(?:[\w@.-]+\/)+[\w@.-]+$|^[\w@-][\w@.-]*\.[A-Za-z0-9]+$/;
|
||||
function fieldPaths(text) {
|
||||
// Abbreviations that PATH_WORD would read as a name with an extension.
|
||||
const NOT_A_PATH = /^(?:e\.g|i\.e|n\.b|cf|etc|vs)$/i;
|
||||
const dotSlash = (p) => p.replace(/^\.\//, '');
|
||||
/**
|
||||
* The one reader of a step's `Files:` field: this file's tests-only check and
|
||||
* the plan-file sweep (lib/verification/plan-scope-sweep.mjs) both use it, so
|
||||
* the two never judge different lists for one step. A leading `./` is dropped.
|
||||
*/
|
||||
export function fieldPaths(text) {
|
||||
const spans = [];
|
||||
const masked = String(text ?? '').replace(/`([^`\n]+)`/g, (_, p) => `\u0000${spans.push(p.trim()) - 1}\u0000`);
|
||||
const out = [];
|
||||
|
|
@ -304,17 +312,20 @@ function fieldPaths(text) {
|
|||
for (const word of entry.split(/\s+/)) {
|
||||
const span = /^\u0000(\d+)\u0000$/.exec(word);
|
||||
if (span) found.push(spans[Number(span[1])]);
|
||||
else if (PATH_WORD.test(word.replace(/[.;:]+$/, ''))) found.push(word.replace(/[.;:]+$/, ''));
|
||||
else {
|
||||
const bare = word.replace(/[.;:]+$/, '');
|
||||
if (PATH_WORD.test(bare) && !NOT_A_PATH.test(bare)) found.push(bare);
|
||||
}
|
||||
}
|
||||
out.push(...(found.length > 0 ? found : [entry.split(/\s+/)[0]]));
|
||||
}
|
||||
return out.filter((p) => p && !p.includes('\u0000'));
|
||||
return out.filter((p) => p && !p.includes('\u0000')).map(dotSlash);
|
||||
}
|
||||
|
||||
// A document is never the test here: `docs/specs/api.md` sits under a `specs/`
|
||||
// folder, which lib/util/test-path.mjs reads as tests for the floor guard.
|
||||
// Nor is a data file under a `spec/` or `specs/` folder (`api/specs/openapi.yaml`
|
||||
// is an API spec); under tests/ it is a fixture, and a fixture is a test file.
|
||||
// A document or a data file outside tests/ is never the test here:
|
||||
// `docs/specs/api.md` and `api/specs/openapi.yaml` sit under a `specs/` folder,
|
||||
// which lib/util/test-path.mjs reads as tests for the floor guard. Under tests/
|
||||
// either one is a fixture, and a fixture is a test file.
|
||||
const DOC_RE = /\.(md|markdown|mdx|txt|rst|adoc)$/i;
|
||||
const DATA_RE = /\.(ya?ml|json|toml|xml|csv)$/i;
|
||||
const UNDER_TESTS_RE = /(^|\/)(tests?|__tests__)\//;
|
||||
|
|
@ -333,13 +344,17 @@ const UNDER_TESTS_RE = /(^|\/)(tests?|__tests__)\//;
|
|||
* @returns {{testsOnly: boolean, files: string[]}}
|
||||
*/
|
||||
export function stepTestsOnly(body) {
|
||||
const f = parseStepFields(body);
|
||||
return stepTestsOnlyFromFields(parseStepFields(body));
|
||||
}
|
||||
|
||||
/** `stepTestsOnly` for a caller that already holds the step's parsed fields. */
|
||||
export function stepTestsOnlyFromFields(f) {
|
||||
const files = f.has('Files') ? fieldPaths(f.get('Files').text) : [];
|
||||
const testFirst = f.has('Test first')
|
||||
? [...f.get('Test first').text.matchAll(/\bFile:\**[ \t]*`([^`\n]+)`/gi)].map((m) => m[1].trim())
|
||||
? [...f.get('Test first').text.matchAll(/\bFile:\**[ \t]*`([^`\n]+)`/gi)].map((m) => dotSlash(m[1].trim()))
|
||||
: [];
|
||||
const isTest = (p) => testFirst.includes(p) ||
|
||||
(isTestPath(p) && !DOC_RE.test(p) && !(DATA_RE.test(p) && !UNDER_TESTS_RE.test(p)));
|
||||
(isTestPath(p) && (UNDER_TESTS_RE.test(p) || !(DOC_RE.test(p) || DATA_RE.test(p))));
|
||||
return { testsOnly: files.length > 0 && files.every(isTest), files };
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -190,7 +190,7 @@ export function loadCases(root) {
|
|||
const text = readFileSync(f, 'utf8');
|
||||
const tagsField = readFrontmatterFields(text).fields.tags;
|
||||
const tags = tagsField && Array.isArray(tagsField.value) ? tagsField.value.map(String) : [];
|
||||
const kindTag = tags.find((t) => t in KIND_TAGS);
|
||||
const kindTag = tags.find((t) => Object.hasOwn(KIND_TAGS, t));
|
||||
if (!kindTag) continue;
|
||||
const val = (k) => { const t = tags.find((x) => x.startsWith(`${k}=`)); return t ? t.slice(k.length + 1) : null; };
|
||||
const body = text.replace(/^---\n[\s\S]*?\n---\n?/, '').trim();
|
||||
|
|
|
|||
|
|
@ -31,6 +31,7 @@ export function splitFile(text) {
|
|||
export function plainScalarHazards(value) {
|
||||
const out = [];
|
||||
if (/:\s/.test(value) || /:$/.test(value)) out.push('unquoted ": " (YAML reads a mapping and the line fails)');
|
||||
if (/^#/.test(value)) out.push('"#" at the start is a YAML comment: the value is empty');
|
||||
if (/\s#/.test(value)) out.push('" #" starts a YAML comment: the rest of the value is cut');
|
||||
if (/^[`@%*&!|>,?{]/.test(value)) out.push(`a value may not start with "${value[0]}" unquoted`);
|
||||
if (/^\[/.test(value) && !/\]$/.test(value)) out.push('an unclosed flow list');
|
||||
|
|
@ -40,7 +41,9 @@ export function plainScalarHazards(value) {
|
|||
|
||||
function unquote(v) {
|
||||
if (v.startsWith('"')) {
|
||||
if (v.length < 2 || !v.endsWith('"') || v.endsWith('\\"')) return { error: 'unclosed double quote' };
|
||||
// The last quote closes the value unless an odd run of backslashes escapes it.
|
||||
const escaped = (v.slice(1, -1).match(/\\*$/)[0].length % 2) === 1;
|
||||
if (v.length < 2 || !v.endsWith('"') || escaped) return { error: 'unclosed double quote' };
|
||||
try { return { value: JSON.parse(v) }; } catch { return { error: 'bad escape in a double-quoted value' }; }
|
||||
}
|
||||
if (v.startsWith("'")) {
|
||||
|
|
|
|||
|
|
@ -40,7 +40,7 @@ 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,
|
||||
ON_FAILURE_ACTIONS, onFailureAction, stepTestsOnlyFromFields,
|
||||
} from '../parsers/plan-schema.mjs';
|
||||
import { validateAllManifests } from '../parsers/manifest-yaml.mjs';
|
||||
import { parseDocument } from '../util/frontmatter.mjs';
|
||||
|
|
@ -165,7 +165,7 @@ function checkRecovery(sections, opts) {
|
|||
`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) {
|
||||
if (stepTestsOnlyFromFields(f).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')
|
||||
|
|
@ -298,11 +298,13 @@ export function validatePlanContent(rawText, opts = {}) {
|
|||
// 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.
|
||||
// 2026-10-10). From 1.8 the rules below cover every step except one: none of
|
||||
// them asks for a `Verify:`, so an appended step needs one on any version
|
||||
// (round 3).
|
||||
const stepsFrom = Number.isInteger(opts.stepsFrom) && opts.stepsFrom > 0 ? opts.stepsFrom : null;
|
||||
if (stepsFrom !== null && !versionAtLeast(planVersion, '1.8')) {
|
||||
if (stepsFrom !== null) {
|
||||
const appended = sections.filter((s) => Number(s.n) >= stepsFrom);
|
||||
const stepIssues = [
|
||||
const stepIssues = versionAtLeast(planVersion, '1.8') ? [] : [
|
||||
...checkSteps(appended).filter((i) => i.code !== 'PLAN_NO_END_TO_END'),
|
||||
...checkRunnable(text, appended, { cwd: opts.cwd, stepsOnly: true }),
|
||||
];
|
||||
|
|
|
|||
|
|
@ -40,7 +40,7 @@ export const PROGRESS_REGRESSION_VALUES = ['passed', 'failed', 'unrunnable', 'no
|
|||
export const PROGRESS_PLAN_APPROVAL_VALUES = ['approved', 'not-checked']; // K2; not-checked = a session spec
|
||||
export const PROGRESS_ARCH_RULES_VALUES = ['pass', 'violation', 'not-measured']; // K5 exit 0 / 1 / 2
|
||||
export const PROGRESS_SCOPE_SWEEP_VALUES = ['pass', 'deviation', 'not-measured']; // K6 exit 0 / 1 / 2
|
||||
export const PROGRESS_SIMPLIFY_VALUES = ['applied', 'clean', 'reverted', 'not-run'];
|
||||
export const PROGRESS_SIMPLIFY_VALUES = ['applied', 'clean', 'reverted', 'revert-refused', 'not-run'];
|
||||
export const PROGRESS_REVIEW_GATE_EXITS = [0, 1, 2]; // K7: ALLOW/WARN, BLOCK, could not decide
|
||||
|
||||
function checkStepFields(k, s, errors) {
|
||||
|
|
|
|||
|
|
@ -257,7 +257,13 @@ export function screenCommand(command, opts = {}) {
|
|||
// - It is not a sandbox. `npm test`, `npm run <script>` and `make test` run
|
||||
// whatever the repo's own package.json/Makefile says they run, including a
|
||||
// script that pushes. That is the repo's responsibility, not this file's.
|
||||
// - It has no opinion about arguments beyond the shape rules below.
|
||||
// - It is not an option parser. Each runner takes only plain words (no
|
||||
// quote, backslash, `$` or brace; a glob only inside a path) and a short
|
||||
// list of options that only read (RUNNER_ARGS); an option that writes a
|
||||
// file or loads code (`--test-reporter-destination`, `--outputFile`,
|
||||
// `--junitxml`, `--basetemp`, `--import`, `--config`, `-o`, `-exec`) is on
|
||||
// no list (release review of 6.3.0, round 3: rounds 1-2 closed this for
|
||||
// git only).
|
||||
// - It rejects plenty of honest commands: an env prefix (`CI=1 npm test`), a
|
||||
// project's own binary (`./build/app --check`), anything piped. A check
|
||||
// that needs one of those is declared through a runner instead —
|
||||
|
|
@ -279,6 +285,60 @@ function writesOutput(word) {
|
|||
const deny = (reason) => ({ allowed: false, reason });
|
||||
const permit = () => ({ allowed: true, reason: '' });
|
||||
|
||||
// A word /bin/sh hands on unchanged, or a path with a glob in it: a glob can
|
||||
// only expand to names that start with the word's first character, so it
|
||||
// cannot make an option out of a path.
|
||||
const PLAIN_WORD = /^(?:[A-Za-z0-9_@%^~:.,=+/-]+|[A-Za-z0-9_./][A-Za-z0-9_@%^~:.,=+/*?-]*)$/;
|
||||
|
||||
// After each runner's fixed words: the options it may take (`name` or
|
||||
// `name=value`; `value` lists the ones whose value may be the next word, and
|
||||
// `only` the ones whose value must be one of a few words: a built-in node test
|
||||
// reporter only formats, a reporter path would load code),
|
||||
// whether `--` may hand the rest to the repo's own script (`dashDash`: then
|
||||
// only paths follow), and whether a plain word may carry `=` (make reads
|
||||
// `VAR=value` as an override). The lists hold options that only read.
|
||||
const RUNNER_ARGS = {
|
||||
npm: { fixed: (t) => (t[1] === 'run' ? 3 : 2), opts: [], dashDash: true },
|
||||
node: {
|
||||
fixed: 2,
|
||||
opts: ['--test-name-pattern', '--test-skip-pattern', '--test-only', '--test-concurrency', '--test-timeout', '--test-reporter'],
|
||||
value: ['--test-reporter'],
|
||||
only: { '--test-reporter': ['spec', 'tap', 'dot', 'junit', 'lcov'] },
|
||||
},
|
||||
vitest: { fixed: 1, opts: ['--run', '--passWithNoTests', '--silent', '--bail'] },
|
||||
jest: { fixed: 1, opts: ['--ci', '--runInBand', '-i', '--silent', '--passWithNoTests', '--bail', '--verbose'] },
|
||||
pytest: { fixed: 1, opts: ['-q', '-qq', '-v', '-vv', '-x', '-s', '--quiet', '--verbose', '--exitfirst', '--tb', '--maxfail'] },
|
||||
cargo: { fixed: 2, opts: ['--release', '--workspace', '--all', '--lib', '--doc', '-q', '--quiet', '--no-fail-fast'] },
|
||||
go: { fixed: 2, opts: ['-v', '-short', '-race', '-failfast', '-cover', '-count', '-run'], value: ['-count', '-run'] },
|
||||
make: { fixed: 2, opts: [], noAssign: true },
|
||||
};
|
||||
RUNNER_ARGS.python = RUNNER_ARGS.python3 = RUNNER_ARGS.uv = { ...RUNNER_ARGS.pytest, fixed: 3 };
|
||||
|
||||
function screenArgs(t, spec) {
|
||||
const fixed = typeof spec.fixed === 'function' ? spec.fixed(t) : spec.fixed;
|
||||
for (let i = fixed, paths = false; i < t.length; i++) {
|
||||
const w = t[i];
|
||||
if (paths) {
|
||||
if (w.startsWith('-')) return deny(`an option after \`--\` (${w}): only paths may follow it`);
|
||||
continue;
|
||||
}
|
||||
if (w === '--' && spec.dashDash) { paths = true; continue; }
|
||||
if (w.startsWith('-')) {
|
||||
const name = w.split('=')[0];
|
||||
if (!spec.opts.includes(name)) return deny(`\`${t[0]}\` with an option that is not on its read-only list (${name})`);
|
||||
const inline = w.includes('=');
|
||||
const value = inline ? w.slice(name.length + 1) : spec.value?.includes(name) ? t[i + 1] : undefined;
|
||||
if (!inline && spec.value?.includes(name)) i++;
|
||||
if (spec.only?.[name] && !spec.only[name].includes(value)) {
|
||||
return deny(`\`${t[0]}\` with ${name} ${value ?? '(no value)'}: only ${spec.only[name].join('|')}`);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (spec.noAssign && w.includes('=')) return deny(`\`${t[0]}\` with a variable override (${w})`);
|
||||
}
|
||||
return permit();
|
||||
}
|
||||
|
||||
// `bash <path>` is the way a repo declares a check the allowlist has no name
|
||||
// for. The path must live under tests/, be relative, and not climb out of it.
|
||||
function isTestScript(path) {
|
||||
|
|
@ -371,7 +431,11 @@ function screenShape(command, opts, writeTime) {
|
|||
|
||||
const tokens = text.trim().split(/\s+/);
|
||||
if (!Object.hasOwn(RUNNERS, tokens[0])) return deny(`\`${tokens[0]}\` is not a known test runner`);
|
||||
return RUNNERS[tokens[0]](tokens, cwd, writeTime);
|
||||
const form = RUNNERS[tokens[0]](tokens, cwd, writeTime);
|
||||
if (!form.allowed || tokens[0] === 'git') return form;
|
||||
const odd = tokens.find((w) => !PLAIN_WORD.test(w));
|
||||
if (odd !== undefined) return deny(`a word the shell would rewrite (${odd}): only plain words are checked`);
|
||||
return Object.hasOwn(RUNNER_ARGS, tokens[0]) ? screenArgs(tokens, RUNNER_ARGS[tokens[0]]) : permit();
|
||||
}
|
||||
|
||||
// --- running ----------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -33,7 +33,7 @@ import { readFileSync, existsSync, realpathSync } from 'node:fs';
|
|||
import { spawnSync } from 'node:child_process';
|
||||
import { resolve, relative, dirname, isAbsolute, sep } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { sliceSteps, parseStepFields } from '../parsers/plan-schema.mjs';
|
||||
import { sliceSteps, parseStepFields, fieldPaths } from '../parsers/plan-schema.mjs';
|
||||
import { parseManifest } from '../parsers/manifest-yaml.mjs';
|
||||
|
||||
// Files Voyage's own process writes while planning and executing.
|
||||
|
|
@ -44,9 +44,8 @@ export const PROCESS_FILES = Object.freeze([
|
|||
]);
|
||||
|
||||
const BACKTICK_SPAN = /`([^`\n]+)`/g;
|
||||
// Backticked: spaces allowed (`docs/my file.md`); bare: no whitespace.
|
||||
// A backticked path in `Test first:` (spaces allowed: `docs/my file.md`).
|
||||
const looksLikeTickedPath = s => /[./]/.test(s) && !/^https?:/.test(s) && !s.startsWith('-') && s.trim() === s;
|
||||
const looksLikePath = s => !/\s/.test(s) && looksLikeTickedPath(s);
|
||||
|
||||
function pathsIn(text) {
|
||||
const out = [];
|
||||
|
|
@ -62,12 +61,9 @@ export function plannedPaths(planText) {
|
|||
for (const s of sliceSteps(planText)) {
|
||||
const f = parseStepFields(s.body);
|
||||
const files = f.get('Files');
|
||||
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));
|
||||
}
|
||||
// One reader with the tests-only check (plan-schema.mjs fieldPaths):
|
||||
// backticked spans as written, else every path-like word.
|
||||
if (files) for (const p of fieldPaths(files.text)) out.add(norm(p));
|
||||
const tf = f.get('Test first');
|
||||
if (tf) {
|
||||
for (const line of tf.text.split('\n')) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue