fix: round 2 of the release review of 6.3.0
Round 2 (v6.2.0..0d044f6, BLOCK: 2 BLOCKER, 11 MAJOR, 1 MINOR), each fixed behind a test that failed first. - The git runner checks only words /bin/sh passes through unchanged (no quote, backslash, $, glob or brace) and refuses --output and its abbreviations: round 1's --output refusal was bypassed by quoting. - fieldPaths: backticked paths are kept as written, parentheses and all; in plain text every path-like word counts, and a parenthesised note does not. - plan-validator --steps-from reads the plan's ## Verification for an appended End-to-end step, and warns instead of failing under --soft. - regression-check prints progress_status, the value progress.json takes; /trekexecute copies it. - release-check: a plugin in a subdirectory is checked in its own directory, the suite is not run when a cheaper check already makes the answer exit 2, and a bad argument or a crash is exit 2. - Skill routing looks a skill up among skills only, and builds the corpus vectors once. - /trekexecute Phase 7.2 says plainly that remediation steps in the review template's own form stop with a re-plan; making them runnable is a later order (operator 2026-10-10). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
0d044f6b6f
commit
ed87cc668d
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
10 changed files with 263 additions and 70 deletions
|
|
@ -285,22 +285,30 @@ export function onFailureAction(text) {
|
|||
// ---- a step that holds nothing but tests -----------------------------------
|
||||
|
||||
/**
|
||||
* The paths a field names. Each entry (comma- or line-separated, a sub-bullet's
|
||||
* `-` dropped) gives its backticked spans, or else its first word: a plain
|
||||
* path beside a backticked one is still a path (independent review, order 10).
|
||||
* A parenthesised note is not an entry: "`a.test.mjs` (new: covers x, y)"
|
||||
* names one path, not "y)" too (release review of 6.3.0, 2026-10-10).
|
||||
* The paths a field names, in order. A backticked span is a path as written,
|
||||
* parentheses and all (`app/(auth)/x.sh`). In the plain text around the spans
|
||||
* a parenthesised note is not a path ("(new: covers x, y)"), and every word
|
||||
* that looks like a path is one ("tests/a.test.mjs + src/b.mjs" names both);
|
||||
* an entry (comma- or line-separated) with no such word gives its first word,
|
||||
* 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) {
|
||||
const spans = [];
|
||||
const masked = String(text ?? '').replace(/`([^`\n]+)`/g, (_, p) => `\u0000${spans.push(p.trim()) - 1}\u0000`);
|
||||
const out = [];
|
||||
for (const raw of String(text ?? '').replace(/\([^()\n]*\)/g, ' ').split(/[,\n]/)) {
|
||||
for (const raw of masked.replace(/\([^()\n]*\)/g, ' ').split(/[,\n]/)) {
|
||||
const entry = raw.trim().replace(/^[-*]\s+/, '');
|
||||
if (entry === '') continue;
|
||||
const spans = [...entry.matchAll(/`([^`\n]+)`/g)].map((m) => m[1].trim());
|
||||
if (spans.length > 0) out.push(...spans);
|
||||
else out.push(entry.split(/\s+/)[0]);
|
||||
const found = [];
|
||||
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(/[.;:]+$/, ''));
|
||||
}
|
||||
out.push(...(found.length > 0 ? found : [entry.split(/\s+/)[0]]));
|
||||
}
|
||||
return out.filter(Boolean);
|
||||
return out.filter((p) => p && !p.includes('\u0000'));
|
||||
}
|
||||
|
||||
// A document is never the test here: `docs/specs/api.md` sits under a `specs/`
|
||||
|
|
|
|||
|
|
@ -102,7 +102,10 @@ export function buildCorpus(entries) {
|
|||
const df = new Map();
|
||||
for (const tf of docs.values()) for (const term of tf.keys()) df.set(term, (df.get(term) || 0) + 1);
|
||||
const n = docs.size;
|
||||
return { docs, idf: (term) => Math.log(1 + n / (1 + (df.get(term) || 0))), entries };
|
||||
const idf = (term) => Math.log(1 + n / (1 + (df.get(term) || 0)));
|
||||
// The corpus never changes: its vectors are built once, not per prompt or pair.
|
||||
const vecs = new Map([...docs].map(([id, tf]) => [id, vec(tf, idf)]));
|
||||
return { docs, idf, vecs, entries };
|
||||
}
|
||||
|
||||
function vec(tf, idf) {
|
||||
|
|
@ -122,7 +125,7 @@ function cosine(a, b) {
|
|||
export function rank(prompt, corpus) {
|
||||
const pv = vec(termFreq(tokenize(prompt)), corpus.idf);
|
||||
const out = [];
|
||||
for (const [id, tf] of corpus.docs) out.push({ id, score: cosine(pv, vec(tf, corpus.idf)) });
|
||||
for (const [id, v] of corpus.vecs) out.push({ id, score: cosine(pv, v) });
|
||||
return out.sort((a, b) => b.score - a.score || a.id.localeCompare(b.id));
|
||||
}
|
||||
|
||||
|
|
@ -132,7 +135,7 @@ export function collisions(corpus, t = THRESHOLDS) {
|
|||
const out = [];
|
||||
for (let i = 0; i < ids.length; i++) {
|
||||
for (let j = i + 1; j < ids.length; j++) {
|
||||
const sim = cosine(vec(corpus.docs.get(ids[i]), corpus.idf), vec(corpus.docs.get(ids[j]), corpus.idf));
|
||||
const sim = cosine(corpus.vecs.get(ids[i]), corpus.vecs.get(ids[j]));
|
||||
if (sim >= t.COLLISION_ERROR) out.push({ a: ids[i], b: ids[j], sim, level: 'error' });
|
||||
else if (sim >= t.COLLISION_WARN) out.push({ a: ids[i], b: ids[j], sim, level: 'warn' });
|
||||
}
|
||||
|
|
@ -205,8 +208,11 @@ export function loadCases(root) {
|
|||
export function checkRouting(root, t = THRESHOLDS) {
|
||||
const entries = loadEntries(root);
|
||||
const corpus = buildCorpus(entries);
|
||||
const byName = new Map(entries.map((e) => [e.name, e]));
|
||||
const skills = entries.filter((e) => e.kind === 'skill');
|
||||
// A skill is looked up among skills only: an agent or command with the same
|
||||
// name must not take its place. An owner may be any kind; a skill wins a tie.
|
||||
const skillByName = new Map(skills.map((e) => [e.name, e]));
|
||||
const byName = new Map([...entries.filter((e) => e.kind !== 'skill'), ...skills].map((e) => [e.name, e]));
|
||||
const cases = loadCases(root);
|
||||
const errors = [];
|
||||
const warnings = [];
|
||||
|
|
@ -220,7 +226,7 @@ export function checkRouting(root, t = THRESHOLDS) {
|
|||
|
||||
for (const c of cases) {
|
||||
if (!c.from) errors.push({ code: 'ROUTING_CASE_NO_SOURCE', case: c.case, message: `${c.case}: tag the event the case came from (from=...)` });
|
||||
const self = c.skill && byName.get(c.skill);
|
||||
const self = c.skill && skillByName.get(c.skill);
|
||||
if (!self || self.kind !== 'skill') {
|
||||
errors.push({ code: 'ROUTING_UNKNOWN_SKILL', case: c.case, message: `${c.case}: skill=${c.skill} is not a skill in skills/` });
|
||||
continue;
|
||||
|
|
|
|||
|
|
@ -99,7 +99,9 @@ const WAY_IN = 'A check the allowlist has no name for (a pipe, `jq`, `grep`, the
|
|||
function checkRunnable(text, sections, opts) {
|
||||
const out = [];
|
||||
const criteria = parsePlanVerification(text);
|
||||
for (const c of criteria) {
|
||||
// opts.stepsOnly: only the given steps are judged; ## Verification is read
|
||||
// for the commands it lists, never reported on (the --steps-from path).
|
||||
for (const c of opts.stepsOnly ? [] : criteria) {
|
||||
const r = runnableCheck(c, opts);
|
||||
if (!r.runnable) {
|
||||
out.push(issue('PLAN_VERIFICATION_NOT_RUNNABLE',
|
||||
|
|
@ -302,7 +304,7 @@ export function validatePlanContent(rawText, opts = {}) {
|
|||
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 }),
|
||||
...checkRunnable(text, appended, { cwd: opts.cwd, stepsOnly: true }),
|
||||
];
|
||||
for (const s of appended) {
|
||||
if (!parseStepFields(s.body).has('Verify')) {
|
||||
|
|
@ -310,7 +312,8 @@ export function validatePlanContent(rawText, opts = {}) {
|
|||
'An appended step needs one test command: proof of red and the done line run it.'));
|
||||
}
|
||||
}
|
||||
errors.push(...stepIssues);
|
||||
if (strict) errors.push(...stepIssues);
|
||||
else warnings.push(...stepIssues);
|
||||
}
|
||||
|
||||
const ceremony = extractPlanCeremony(text);
|
||||
|
|
|
|||
|
|
@ -268,6 +268,13 @@ const SHELL_OPERATORS = /[;&|<>]|\$\(|`/;
|
|||
|
||||
// Read-only git subcommands: they report, they do not write a repository.
|
||||
const READONLY_GIT = new Set(['status', 'log', 'diff', 'show', 'ls-files']);
|
||||
// A word /bin/sh hands to git unchanged (refs like HEAD~1 and HEAD^, formats like %h, paths).
|
||||
const GIT_LITERAL_WORD = /^[A-Za-z0-9_@%^~:.,=+/-]+$/;
|
||||
// `--output`, `--output=<file>`, or an abbreviation of it git may accept.
|
||||
function writesOutput(word) {
|
||||
const name = word.split('=')[0];
|
||||
return name.length >= 5 && '--output'.startsWith(name);
|
||||
}
|
||||
|
||||
const deny = (reason) => ({ allowed: false, reason });
|
||||
const permit = () => ({ allowed: true, reason: '' });
|
||||
|
|
@ -317,9 +324,15 @@ const RUNNERS = {
|
|||
'/bin/bash': (t) => (isTestScript(t[1]) ? permit() : deny('/bin/bash, but not a script under tests/')),
|
||||
git: (t) => {
|
||||
if (!READONLY_GIT.has(t[1])) return deny(`git, but not a read-only subcommand first (${[...READONLY_GIT].join('|')})`);
|
||||
// `--output` makes diff/log/show write their output to a file (release
|
||||
// review of 6.3.0, 2026-10-10: a plan's check could overwrite any file).
|
||||
if (t.some((w) => w === '--output' || w.startsWith('--output='))) return deny('git with --output writes a file, so it is not a read-only check');
|
||||
// The command runs through /bin/sh, which unquotes and expands what it is
|
||||
// given, so a check on the words here holds only for words the shell passes
|
||||
// through unchanged: no quote, backslash, `$`, glob or brace. Then
|
||||
// `--output` (and an abbreviation of it) is refused: it makes diff/log/show
|
||||
// write to a file (release review of 6.3.0, 2026-10-10: a plan's check
|
||||
// could overwrite any file, first plainly, then through quoting).
|
||||
const odd = t.find((w) => !GIT_LITERAL_WORD.test(w));
|
||||
if (odd !== undefined) return deny(`git with a word the shell would rewrite (${odd}): only plain words are checked`);
|
||||
if (t.some(writesOutput)) return deny('git with --output writes a file, so it is not a read-only check');
|
||||
return permit();
|
||||
},
|
||||
};
|
||||
|
|
|
|||
|
|
@ -10,8 +10,11 @@
|
|||
// (no command, or a command outside the test-runner allowlist): the
|
||||
// step is not done, and that is a finding for the fix rounds
|
||||
// 2 = could not run: bad usage, a plan that cannot be read, no step N
|
||||
// JSON on stdout: {step, status, command, exit_code, output}
|
||||
// JSON on stdout: {step, status, progress_status, command, exit_code, output}
|
||||
// status: passed | failed | blocked | unrunnable | absent | error
|
||||
// progress_status: the value progress.json takes for steps.N.regression
|
||||
// (blocked counts as unrunnable, error as not-run), so the executor
|
||||
// copies one field and never translates
|
||||
//
|
||||
// The command is the field's first backticked span — the same reading as the
|
||||
// runner's `## Verification` bullets — and it goes through the same two
|
||||
|
|
@ -44,6 +47,8 @@ function exec(command, { cwd, timeoutMs }) {
|
|||
|
||||
export const REGRESSION_FIELD = 'Regression';
|
||||
const EXIT = Object.freeze({ passed: 0, absent: 0, failed: 1, blocked: 1, unrunnable: 1, error: 2 });
|
||||
const PROGRESS = Object.freeze({ passed: 'passed', absent: 'absent', failed: 'failed', blocked: 'unrunnable', unrunnable: 'unrunnable', error: 'not-run' });
|
||||
const withProgress = (r) => ({ ...r, progress_status: PROGRESS[r.status] ?? 'not-run' });
|
||||
|
||||
/**
|
||||
* @param {{ planText: string, step: number, cwd?: string, run?: Function }} o
|
||||
|
|
@ -52,19 +57,19 @@ const EXIT = Object.freeze({ passed: 0, absent: 0, failed: 1, blocked: 1, unrunn
|
|||
export function checkRegression({ planText, step, cwd = process.cwd(), run = (c, o) => runCriteria(c, { ...o, exec }) }) {
|
||||
const base = { step, command: null, exit_code: null, output: '' };
|
||||
const found = sliceSteps(planText).find((s) => Number(s.n) === step);
|
||||
if (!found) return { ...base, status: 'error', output: `no "### Step ${step}:" in the plan`, exit: EXIT.error };
|
||||
if (!found) return withProgress({ ...base, status: 'error', output: `no "### Step ${step}:" in the plan`, exit: EXIT.error });
|
||||
const field = parseStepFields(found.body).get(REGRESSION_FIELD);
|
||||
if (!field) return { ...base, status: 'absent', output: 'no Regression: field — nothing to run', exit: EXIT.absent };
|
||||
if (!field) return withProgress({ ...base, status: 'absent', output: 'no Regression: field — nothing to run', exit: EXIT.absent });
|
||||
const { command, reason } = firstCommand(field.text);
|
||||
const [r] = run([{ label: `R${step}`, text: field.text, command, reason }], { cwd });
|
||||
return {
|
||||
return withProgress({
|
||||
step,
|
||||
status: r.status,
|
||||
command: r.command ?? null,
|
||||
exit_code: r.exitCode ?? null,
|
||||
output: r.output ?? '',
|
||||
exit: EXIT[r.status] ?? EXIT.error,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
export function main(argv) {
|
||||
|
|
@ -92,7 +97,7 @@ export function main(argv) {
|
|||
}
|
||||
let planText;
|
||||
try { planText = readFileSync(plan, 'utf8'); } catch (e) {
|
||||
return say({ step, status: 'error', command: null, exit_code: null, output: `cannot read ${plan}: ${e.message}`, exit: 2 });
|
||||
return say(withProgress({ step, status: 'error', command: null, exit_code: null, output: `cannot read ${plan}: ${e.message}`, exit: 2 }));
|
||||
}
|
||||
return say(checkRegression({ planText, step, cwd }));
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue