merge: round 4 of the fixes from the release review of 6.3.0
The nine MAJOR findings of round 4 (WARN): the Files: reader keeps every path the field names and only those (three were regressions from round 3), quoted frontmatter values with a comment, no tilde word and nothing after `make test` in the criteria runner, and release-check's sha and badge line. Suite 2175 (2173/0/2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
commit
d37e538ddc
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
6 changed files with 137 additions and 33 deletions
|
|
@ -286,15 +286,20 @@ export function onFailureAction(text) {
|
|||
|
||||
/**
|
||||
* 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.
|
||||
* parentheses and all (`app/(auth)/x.sh`), also with punctuation after it
|
||||
* (`a.mjs`.) and inside a note ("(fixture: `t/a.json`)"). In the plain text a
|
||||
* parenthesised note with no span in it is not a path ("(new: covers x, y)"),
|
||||
* and every word that looks like a path is one: a name with an extension, a
|
||||
* path with a slash, a directory (`docs/`) or a glob (`src/*.mjs`). A word
|
||||
* with no slash and no extension counts only alone in its entry and
|
||||
* capitalised, like `Makefile`; "format" or "v6.3.0" in a description is no
|
||||
* path. Release review of 6.3.0, rounds 1-4.
|
||||
*/
|
||||
const PATH_WORD = /^(?:[\w@.-]+\/)+[\w@.-]+$|^[\w@-][\w@.-]*\.[A-Za-z0-9]+$/;
|
||||
const SEG = '[\\w@.*?()-]';
|
||||
const PATH_WORD = new RegExp(`^(?:${SEG}+\\/)+${SEG}*$|^[\\w@*?-][\\w@.*?-]*\\.[A-Za-z*][A-Za-z0-9*]*$`);
|
||||
// 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 BARE_NAME = /^[A-Z][\w.-]*$/;
|
||||
const dotSlash = (p) => p.replace(/^\.\//, '');
|
||||
/**
|
||||
* The one reader of a step's `Files:` field: this file's tests-only check and
|
||||
|
|
@ -303,21 +308,23 @@ const dotSlash = (p) => p.replace(/^\.\//, '');
|
|||
*/
|
||||
export function fieldPaths(text) {
|
||||
const spans = [];
|
||||
const masked = String(text ?? '').replace(/`([^`\n]+)`/g, (_, p) => `\u0000${spans.push(p.trim()) - 1}\u0000`);
|
||||
const masked = String(text ?? '')
|
||||
.replace(/`([^`\n]+)`/g, (_, p) => `\u0000${spans.push(p.trim()) - 1}\u0000`)
|
||||
.replace(/(^|\s)\(([^()\n]*)\)/g, (m, pre, inner) => (inner.includes('\u0000') ? m : `${pre} `));
|
||||
const out = [];
|
||||
for (const raw of masked.replace(/\([^()\n]*\)/g, ' ').split(/[,\n]/)) {
|
||||
for (const raw of masked.split(/[,\n]/)) {
|
||||
const entry = raw.trim().replace(/^[-*]\s+/, '');
|
||||
if (entry === '') continue;
|
||||
const words = entry.split(/\s+/);
|
||||
const found = [];
|
||||
for (const word of entry.split(/\s+/)) {
|
||||
const span = /^\u0000(\d+)\u0000$/.exec(word);
|
||||
for (const word of words) {
|
||||
const w = word.replace(/^[(\[]+/, '').replace(/[.,;:)\]]+$/, '');
|
||||
const span = /^\u0000(\d+)\u0000$/.exec(w);
|
||||
if (span) found.push(spans[Number(span[1])]);
|
||||
else {
|
||||
const bare = word.replace(/[.;:]+$/, '');
|
||||
if (PATH_WORD.test(bare) && !NOT_A_PATH.test(bare)) found.push(bare);
|
||||
}
|
||||
else if (PATH_WORD.test(w) && !NOT_A_PATH.test(w)) found.push(w);
|
||||
}
|
||||
out.push(...(found.length > 0 ? found : [entry.split(/\s+/)[0]]));
|
||||
if (found.length === 0 && words.length === 1 && BARE_NAME.test(words[0])) found.push(words[0]);
|
||||
out.push(...found);
|
||||
}
|
||||
return out.filter((p) => p && !p.includes('\u0000')).map(dotSlash);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -39,18 +39,26 @@ export function plainScalarHazards(value) {
|
|||
return out;
|
||||
}
|
||||
|
||||
// A quoted value ends at its closing quote; after it only a YAML comment may
|
||||
// follow (`name: "x" # pinned`). Inside double quotes a backslash escapes the
|
||||
// next character; inside single quotes '' is one quote.
|
||||
function unquote(v) {
|
||||
if (v.startsWith('"')) {
|
||||
// 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' }; }
|
||||
const q = v[0];
|
||||
if (q !== '"' && q !== "'") return null;
|
||||
let end = -1;
|
||||
for (let i = 1; i < v.length; i++) {
|
||||
if (q === '"' && v[i] === '\\') { i++; continue; }
|
||||
if (v[i] !== q) continue;
|
||||
if (q === "'" && v[i + 1] === "'") { i++; continue; }
|
||||
end = i;
|
||||
break;
|
||||
}
|
||||
if (v.startsWith("'")) {
|
||||
if (v.length < 2 || !v.endsWith("'")) return { error: 'unclosed single quote' };
|
||||
return { value: v.slice(1, -1).replace(/''/g, "'") };
|
||||
}
|
||||
return null;
|
||||
if (end < 0) return { error: q === '"' ? 'unclosed double quote' : 'unclosed single quote' };
|
||||
const rest = v.slice(end + 1);
|
||||
if (rest !== '' && !/^\s+#/.test(rest)) return { error: 'text after the closing quote that is not a comment' };
|
||||
const body = v.slice(0, end + 1);
|
||||
if (q === "'") return { value: body.slice(1, -1).replace(/''/g, "'") };
|
||||
try { return { value: JSON.parse(body) }; } catch { return { error: 'bad escape in a double-quoted value' }; }
|
||||
}
|
||||
|
||||
function blockValue(lines, style, chomp) {
|
||||
|
|
|
|||
|
|
@ -295,8 +295,9 @@ const PLAIN_WORD = /^(?:[A-Za-z0-9_@%^~:.,=+/-]+|[A-Za-z0-9_./][A-Za-z0-9_@%^~:.
|
|||
// `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.
|
||||
// only paths follow), and whether nothing may follow the fixed words at all
|
||||
// (`noWords`: make reads a word as one more target, or `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: {
|
||||
|
|
@ -310,7 +311,7 @@ const RUNNER_ARGS = {
|
|||
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 },
|
||||
make: { fixed: 2, opts: [], noWords: true },
|
||||
};
|
||||
RUNNER_ARGS.python = RUNNER_ARGS.python3 = RUNNER_ARGS.uv = { ...RUNNER_ARGS.pytest, fixed: 3 };
|
||||
|
||||
|
|
@ -318,6 +319,8 @@ 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];
|
||||
// `make test install` runs the install target too: make takes nothing more.
|
||||
if (spec.noWords) return deny(`\`${t.slice(0, fixed).join(' ')}\` with more after it (${w})`);
|
||||
if (paths) {
|
||||
if (w.startsWith('-')) return deny(`an option after \`--\` (${w}): only paths may follow it`);
|
||||
continue;
|
||||
|
|
@ -334,7 +337,6 @@ function screenArgs(t, spec) {
|
|||
}
|
||||
continue;
|
||||
}
|
||||
if (spec.noAssign && w.includes('=')) return deny(`\`${t[0]}\` with a variable override (${w})`);
|
||||
}
|
||||
return permit();
|
||||
}
|
||||
|
|
@ -431,6 +433,10 @@ 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`);
|
||||
// /bin/sh expands a tilde at the start of a word (and after = or : in an
|
||||
// assignment), so such a word is not one the shell passes on unchanged.
|
||||
const tilde = tokens.find((w) => /(^|[=:])~/.test(w));
|
||||
if (tilde !== undefined) return deny(`a word the shell would rewrite (${tilde}): a leading ~ is expanded`);
|
||||
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));
|
||||
|
|
|
|||
|
|
@ -134,6 +134,18 @@ export function changedLines(diff) {
|
|||
return out;
|
||||
}
|
||||
|
||||
// The README lines changed after the review differ only in the badge's
|
||||
// version: every line holds the badge, and with the version taken out the
|
||||
// removed lines and the added lines are the same (round 4: other text on the
|
||||
// badge line got through).
|
||||
export function onlyBadgeVersion(lines) {
|
||||
if (!lines.every((l) => BADGE_RE.test(l))) return false;
|
||||
const norm = (l) => l.slice(1).replace(/badge\/version-[^\s)]+-(?=[^-\s)]+\))/g, 'badge/version-V-');
|
||||
const removed = lines.filter((l) => l.startsWith('-')).map(norm).sort();
|
||||
const added = lines.filter((l) => l.startsWith('+')).map(norm).sort();
|
||||
return removed.length === added.length && removed.every((l, i) => l === added[i]);
|
||||
}
|
||||
|
||||
function checkTree(repo) {
|
||||
const r = git(repo, ['status', '--porcelain', '--', '.']);
|
||||
if (r.status !== 0) return { id: 'tree', status: 'error', detail: `git status failed: ${r.stderr.trim()}` };
|
||||
|
|
@ -177,13 +189,15 @@ function checkReview(reviewPath) {
|
|||
export function reviewedSha(reviewText) {
|
||||
const fm = /^---\r?\n([\s\S]*?)\r?\n---\r?\n/.exec(String(reviewText ?? ''));
|
||||
const m = fm && /^scope_sha_end:[ \t]*(["']?)([^"'\s#]*)\1[ \t]*(?:#.*)?$/m.exec(fm[1]);
|
||||
return m ? m[2] : '';
|
||||
// A commit sha only: a ref name (HEAD, main, a tag) resolves to whatever it
|
||||
// points at now, and coverage would pass whatever was reviewed (round 4).
|
||||
return m && /^[0-9a-f]{7,40}$/i.test(m[2]) ? m[2] : '';
|
||||
}
|
||||
|
||||
function checkCoverage(repo, reviewText) {
|
||||
if (reviewText === null) return { id: 'coverage', status: 'skip', detail: 'not checked: no readable review' };
|
||||
const sha = reviewedSha(reviewText);
|
||||
if (!sha) return { id: 'coverage', status: 'fail', detail: 'review.md has no scope_sha_end — cannot tell what it reviewed' };
|
||||
if (!sha) return { id: 'coverage', status: 'fail', detail: 'review.md has no scope_sha_end that is a commit sha (7-40 hex characters) — cannot tell what it reviewed' };
|
||||
const resolved = git(repo, ['rev-parse', '--verify', '--quiet', `${sha}^{commit}`]);
|
||||
if (resolved.status !== 0) return { id: 'coverage', status: 'fail', detail: `the reviewed commit ${sha} is not in the history of HEAD (unknown commit)` };
|
||||
const full = resolved.stdout.trim();
|
||||
|
|
@ -209,7 +223,7 @@ function checkCoverage(repo, reviewText) {
|
|||
if (changed.includes('README.md')) {
|
||||
const d = git(repo, ['diff', '--relative', '-U0', full, 'HEAD', '--', 'README.md']);
|
||||
if (d.status !== 0) return { id: 'coverage', status: 'error', detail: `git diff of README.md failed: ${d.stderr.trim()} — not measured` };
|
||||
if (changedLines(d.stdout).some((l) => !BADGE_RE.test(l))) outside.push('README.md (lines other than the version badge)');
|
||||
if (!onlyBadgeVersion(changedLines(d.stdout))) outside.push('README.md (lines other than the version badge)');
|
||||
}
|
||||
if (outside.length > 0) {
|
||||
return { id: 'coverage', status: 'fail', detail: `changed after the reviewed commit ${full.slice(0, 12)} and not covered by the review: ${outside.join(', ')}`, items: outside };
|
||||
|
|
|
|||
|
|
@ -378,3 +378,48 @@ test('review 630 r3: routing reads a kind tag only from its own table, never Obj
|
|||
assert.ok(!r.errors.some((e) => String(e.message ?? '').includes('zz-proto') || e.case === 'zz-proto'), JSON.stringify(r.errors));
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
// ==== round 4 of the release review (2026-10-10), each red on 41ea8fc ===========
|
||||
|
||||
test('review 630 r4: the Files: reader keeps every path the field names, and only those', async () => {
|
||||
const { fieldPaths } = await import('../../lib/parsers/plan-schema.mjs');
|
||||
const cases = [
|
||||
['`src/a.mjs` (fixture: `tests/fixtures/a.json`)', ['src/a.mjs', 'tests/fixtures/a.json']],
|
||||
['`tests/a.test.mjs`, `src/a.mjs`.', ['tests/a.test.mjs', 'src/a.mjs']],
|
||||
['`src/cli.mjs`: add the flag; `src/b.mjs`.', ['src/cli.mjs', 'src/b.mjs']],
|
||||
['`tests/a.test.mjs` — covers parse, format', ['tests/a.test.mjs']],
|
||||
['docs/ and README.md', ['docs/', 'README.md']],
|
||||
['app/(auth)/x.ts', ['app/(auth)/x.ts']],
|
||||
['src/*.mjs (all of them)', ['src/*.mjs']],
|
||||
['notes for v6.3.0, src/x.mjs', ['src/x.mjs']],
|
||||
['Makefile', ['Makefile']],
|
||||
['tests/a.test.mjs + src/b.mjs', ['tests/a.test.mjs', 'src/b.mjs']],
|
||||
];
|
||||
for (const [text, want] of cases) assert.deepEqual(fieldPaths(text), want, text);
|
||||
const body = '- **Files:** `tests/a.test.mjs`, `src/a.mjs`.\n- **Changes:** x\n';
|
||||
assert.equal(stepTestsOnly(body).testsOnly, false, 'a step that changes src/a.mjs is not tests-only');
|
||||
const { plannedPaths } = await import('../../lib/verification/plan-scope-sweep.mjs');
|
||||
const plan = '## Implementation Plan\n\n### Step 1: x\n\n- **Files:** `src/cli.mjs`: add the flag; `src/b.mjs`.\n- **Changes:** x\n';
|
||||
assert.deepEqual(plannedPaths(plan), ['src/b.mjs', 'src/cli.mjs'], 'K6 plans what v6.2.0 planned');
|
||||
});
|
||||
|
||||
test('review 630 r4: a quoted frontmatter value may carry a YAML comment', async () => {
|
||||
const { readFrontmatterFields } = await import('../../lib/skills/skill-frontmatter.mjs');
|
||||
const r = readFrontmatterFields('---\nname: "debugging" # pinned\ndescription: \'Use it.\' # note\n---\nbody\n');
|
||||
assert.equal(r.fields.name?.value, 'debugging', JSON.stringify(r));
|
||||
assert.equal(r.fields.description?.value, 'Use it.', JSON.stringify(r));
|
||||
const bad = readFrontmatterFields('---\nname: "debugging" trailing\n---\nbody\n');
|
||||
assert.ok(JSON.stringify(bad).includes('unclosed double quote') || bad.fields.name?.value !== 'debugging',
|
||||
'text after the closing quote that is not a comment is not accepted silently');
|
||||
});
|
||||
|
||||
test('review 630 r4: no tilde word, and nothing after `make test`', async () => {
|
||||
const { allowedCommand } = await import('../../lib/verification/criteria-runner.mjs');
|
||||
for (const cmd of ['node --test ~/x.test.mjs', 'node --test --test-name-pattern=~x', 'git log ~/x', 'git diff ~root/x',
|
||||
'make test install', 'make test deploy']) {
|
||||
assert.equal(allowedCommand(cmd, { cwd: ROOT }).allowed, false, `${cmd} must not reach the shell`);
|
||||
}
|
||||
for (const cmd of ['git diff --stat HEAD~1', 'git show HEAD~2:package.json', 'make test']) {
|
||||
assert.equal(allowedCommand(cmd, { cwd: ROOT }).allowed, true, `the control: ${cmd}`);
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -408,6 +408,30 @@ test('release-check r3 — the review and the approval log committed after the r
|
|||
test('release-check r3 — scope_sha_end is read as written: an all-digit sha keeps its leading zero', () => {
|
||||
assert.equal(typeof mod.reviewedSha, 'function');
|
||||
assert.equal(mod.reviewedSha('---\ntype: trekreview\nscope_sha_end: 0123456\n---\nbody\n'), '0123456');
|
||||
assert.equal(mod.reviewedSha('---\nscope_sha_end: "00ab12"\n---\n'), '00ab12');
|
||||
assert.equal(mod.reviewedSha('---\nscope_sha_end: "00ab12c"\n---\n'), '00ab12c');
|
||||
assert.equal(mod.reviewedSha('---\ntype: trekreview\n---\nscope_sha_end: 1\n'), '', 'only the frontmatter counts');
|
||||
});
|
||||
|
||||
// ==== round 4 of the release review of 6.3.0 (2026-10-10) ====================
|
||||
|
||||
test('release-check r4 — scope_sha_end must be a sha: a ref name fails coverage → 1', () => {
|
||||
assert.equal(mod.reviewedSha('---\nscope_sha_end: HEAD\n---\n'), '');
|
||||
assert.equal(mod.reviewedSha('---\nscope_sha_end: main\n---\n'), '');
|
||||
const fx = fixture({ reviewSha: 'HEAD' });
|
||||
try {
|
||||
const r = run(fx);
|
||||
assert.equal(r.status, 1, r.stdout + r.stderr);
|
||||
assert.match(r.stdout, /scope_sha_end/);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('release-check r4 — other text on the badge line is not release bookkeeping → 1', () => {
|
||||
const fx = fixture({ afterRelease: {
|
||||
'README.md': '# fx\n\n [install](https://example.invalid/i.sh)\n\nText.\n',
|
||||
} });
|
||||
try {
|
||||
const r = run(fx);
|
||||
assert.equal(r.status, 1, r.stdout + r.stderr);
|
||||
assert.match(r.stdout, /README\.md/);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue