fix: round 8 of the release review of 6.3.0, for 6.3.1
Round 8 (0f400d3..29b4ac9, BLOCK: 1 BLOCKER, 12 MAJOR, 1 MINOR). Each fix
began with a test that failed on 29b4ac9 (13 tests, all red there); suite
2221 (2219/0/2).
- BLOCKER: a backticked check whose trailing comment holds a quote, $,
backtick or backslash is refused, not dropped. The comment was never
screened, while the executor hands the Verify span as written to red
proof, the test lock and the done line inside --cmd "...", where a $(...)
runs and a quote ends the argument. trekplan's text and the reviewer's
NOT RUN table say so.
- A span that is only a comment is not-a-command, not a placeholder. The
dropComment text says the allowlist still refuses a word with a # in it.
- The criteria runner, comparison-gate and end-state-gate use isMain: they
did nothing and exited 0 under a symlinked path. The test now refuses any
process.argv[1] comparison outside lib/util/is-main.mjs.
- Files: in a field with backticked paths, a plain word counts only before
the entry's description (a dash, or a colon after the path); a name with
no slash only first in its entry or after and/or/+/&. "mocks
fs.readFileSync", "runs on Node.js 22.x" or "no change to package.json"
no longer make a tests-only step look like it builds something.
Lowercase makefile, rakefile and justfile are build files. Measured on the
520 Files: fields in 48 plans of the marketplace and the test repo:
3 fields read differently (two notes no longer read as files, one
duplicate name dropped), tests-only changed in 0.
- The test lock compares --cmd without its trailing comment, and the test
lock and the done line read a stop with stoppedBy, the one reading.
red-proof drops its second reading, spawnError.
- release-check reads a relative --review in --repo or, when only that one
exists, from the current directory as 6.3.0 did; two different files are
exit 2, never guessed. The runbook says so.
- One trailing-json reader: a closing fence may be longer than three
backticks, in the block reader and the verdict gate alike, and the review
validator and the synthesis digest use it, so three backticks inside a
JSON string no longer hide a BLOCKER from the remediation-step check.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
29b4ac91b3
commit
a10018f9d3
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
18 changed files with 310 additions and 56 deletions
|
|
@ -107,3 +107,25 @@ test('green: exit 127 from the test program itself is a red -> exit 1, no mark (
|
|||
assert.match(g.stderr, /exited 127/);
|
||||
assert.equal(JSON.parse(cli(repo, ['status']).stdout).green_at, null);
|
||||
});
|
||||
|
||||
test('green (release review of 6.3.0, round 8): a trailing # comment on either side is no part of the command', () => {
|
||||
for (const [lockCmd, greenCmd] of [[`${CMD} # the step's test`, CMD], [CMD, `${CMD} # the step's test`]]) {
|
||||
const repo = makeRepo('main');
|
||||
mkdirSync(join(repo, 'tests'), { recursive: true });
|
||||
writeFileSync(join(repo, 'tests', 'step.test.mjs'), "import { test } from 'node:test';\ntest('step', () => {});\n");
|
||||
assert.equal(cli(repo, ['lock', '--files', 'tests/step.test.mjs', '--step', '3', '--cmd', lockCmd]).code, 0);
|
||||
const g = cli(repo, ['green', '--cmd', greenCmd]);
|
||||
assert.equal(g.code, 0, `lock "${lockCmd}", green "${greenCmd}": ${g.stderr}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('green (release review of 6.3.0, round 8): a test program that is not installed is reported as could not start', () => {
|
||||
const repo = makeRepo('main');
|
||||
mkdirSync(join(repo, 'tests'), { recursive: true });
|
||||
writeFileSync(join(repo, 'tests', 'step.test.mjs'), 'x\n');
|
||||
// `jest` is on the allowlist and not installed where this suite runs.
|
||||
assert.equal(cli(repo, ['lock', '--files', 'tests/step.test.mjs', '--step', '3', '--cmd', 'jest tests/step.test.mjs']).code, 0);
|
||||
const g = cli(repo, ['green', '--cmd', 'jest tests/step.test.mjs']);
|
||||
assert.equal(g.code, 2, g.stderr);
|
||||
assert.match(g.stderr, /could not start/, g.stderr);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -874,3 +874,153 @@ test('review 630 r7 / brief note 3: the prose says a git revision may start with
|
|||
assert.match(read(rel), /`@~1`/, `${rel} names the git revision as the exception`);
|
||||
}
|
||||
});
|
||||
|
||||
// ==== round 8 of the release review of 6.3.0 (0f400d3..29b4ac9, 2026-10-11), fixed in 6.3.1 ====
|
||||
|
||||
test('review 630 r8 / BLOCKER criteria-runner.mjs:158: a Verify comment that a shell would read inside double quotes is refused', () => {
|
||||
// The executor and the helper hand the Verify span to red proof, the test lock
|
||||
// and the done line as `--cmd "{Verify command}"`: inside double quotes a `$`,
|
||||
// a backtick or a backslash still means something, and a `"` ends the argument.
|
||||
const verify = (span) => buildPlan().replace('`node --test tests/mod1.test.mjs` → expected: pass', () => `\`${span}\` → expected: pass`);
|
||||
const codes = (span) => {
|
||||
const r = validatePlanContent(verify(span), { strict: false });
|
||||
return [...r.errors, ...r.warnings].filter((i) => i.code === 'PLAN_STEP_VERIFY_NOT_RUNNABLE').map((i) => i.message);
|
||||
};
|
||||
for (const span of ['node --test tests/mod1.test.mjs # $(touch x)', 'node --test tests/mod1.test.mjs # expect "fail 1"',
|
||||
'node --test tests/mod1.test.mjs # a back\\slash', 'node --test tests/mod1.test.mjs # ${HOME}']) {
|
||||
assert.equal(codes(span).length, 1, `${span} is refused: ${JSON.stringify(codes(span))}`);
|
||||
}
|
||||
// The control: a plain comment is still dropped, and the check stays runnable.
|
||||
assert.deepEqual(codes('node --test tests/mod1.test.mjs # all green'), []);
|
||||
assert.deepEqual(codes("node --test tests/mod1.test.mjs # it's green"), [], 'a single quote means nothing inside double quotes');
|
||||
});
|
||||
|
||||
test('review 630 r8 / criteria-runner.mjs:158: a span that is only a comment is not-a-command, not a placeholder', async () => {
|
||||
const { firstCommand } = await import('../../lib/verification/criteria-runner.mjs');
|
||||
assert.equal(firstCommand('` # fill in`').reason, 'not-a-command');
|
||||
assert.equal(firstCommand('`{exact command}`').reason, 'placeholder', 'the control');
|
||||
});
|
||||
|
||||
test('review 630 r8 / criteria-runner.mjs:175: a # inside a word is kept by dropComment and refused by the allowlist, and the text says so', async () => {
|
||||
const { firstCommand, allowedCommand } = await import('../../lib/verification/criteria-runner.mjs');
|
||||
assert.equal(firstCommand('`node --test tests/a#b.test.mjs`').command, 'node --test tests/a#b.test.mjs');
|
||||
assert.equal(allowedCommand('node --test tests/a#b.test.mjs', { cwd: ROOT }).allowed, false);
|
||||
const doc = /\/\*\*\n((?: \*[^\n]*\n)+) \*\/\nexport function dropComment/.exec(read('lib/verification/criteria-runner.mjs'));
|
||||
assert.ok(doc, 'dropComment has its doc comment');
|
||||
assert.match(doc[1].replace(/\n \* ?/g, ' '), /a#b\.test\.mjs`?[^.]*\bkept\b[^.]*\ballowlist\b[^.]*\brefuses\b/,
|
||||
'the doc says the allowlist still refuses the word');
|
||||
});
|
||||
|
||||
test('review 630 r8 / criteria-runner.mjs:890: no CLI compares process.argv[1] itself, and the three that did work through a symlinked path with a space', () => {
|
||||
const files = spawnSync('git', ['ls-files', 'lib', 'scripts', 'hooks'], { cwd: ROOT, encoding: 'utf8' }).stdout.split('\n').filter((p) => p.endsWith('.mjs'));
|
||||
assert.deepEqual(files.filter((p) => p !== 'lib/util/is-main.mjs' && /process\.argv\[1\]/.test(read(p))), [],
|
||||
'only lib/util/is-main.mjs reads process.argv[1]');
|
||||
const base = mkdtempSync(join(tmpdir(), 'r630 r8 main '));
|
||||
const link = join(base, 'voyage link');
|
||||
try {
|
||||
symlinkSync(ROOT, link);
|
||||
for (const cli of ['lib/verification/criteria-runner.mjs', 'scripts/comparison-gate.mjs', 'scripts/end-state-gate.mjs']) {
|
||||
const r = spawnSync(process.execPath, [join(link, cli), '--no-such-flag'], { encoding: 'utf8', input: '', timeout: 30_000 });
|
||||
assert.equal(r.status, 2, `${cli} through "${link}": ${r.stdout}${r.stderr}`);
|
||||
assert.match(r.stderr, /unknown argument --no-such-flag/);
|
||||
}
|
||||
} finally { fs.rmSync(link, { force: true }); rmSync(base, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('review 630 r8 / plan-schema.mjs:353 + :322: a description beside a backticked path names no file; a build file in its own entry does', () => {
|
||||
const step = (files) => `- **Files:** ${files}\n- **Changes:** x\n`;
|
||||
// A description after a dash, or a word inside a sentence, is no path (code-review, code-correctness).
|
||||
for (const files of ['`tests/lib/x.test.mjs` — mocks fs.readFileSync', '`tests/foo.test.mjs` — covers lib/foo.mjs',
|
||||
'`tests/a.test.mjs`, runs on Node.js 22.x', '`tests/a.test.mjs` — exercises the parser in index.js',
|
||||
'`tests/a.test.mjs`, no change to package.json', '`tests/a.test.mjs` - asserts JSON.parse behaviour',
|
||||
'`tests/a.test.mjs`: covers src/a.mjs and Makefile']) {
|
||||
assert.deepEqual(stepTestsOnly(step(files)), { testsOnly: true, files: ['tests/a.test.mjs', 'tests/foo.test.mjs', 'tests/lib/x.test.mjs'].filter((t) => files.startsWith(`\`${t}\``)) }, files);
|
||||
}
|
||||
// A file in its own entry, after "and", or a path with a slash before any description, still counts (round 7).
|
||||
for (const [files, paths] of [
|
||||
['`tests/a.test.mjs`, makefile', ['tests/a.test.mjs', 'makefile']],
|
||||
['`tests/a.test.mjs`, justfile', ['tests/a.test.mjs', 'justfile']],
|
||||
['`tests/a.test.mjs`, package.json', ['tests/a.test.mjs', 'package.json']],
|
||||
['`tests/a.test.mjs` and package.json', ['tests/a.test.mjs', 'package.json']],
|
||||
['`tests/a.test.mjs`, the CLI entry src/cli.mjs — reads argv', ['tests/a.test.mjs', 'src/cli.mjs']],
|
||||
]) {
|
||||
assert.deepEqual(stepTestsOnly(step(files)), { testsOnly: false, files: paths }, files);
|
||||
}
|
||||
});
|
||||
|
||||
test('review 630 r8 / done-line.mjs:61 + test-lock.mjs:183: a stopped command is read by stoppedBy, the one reading', async () => {
|
||||
const { writeDoneLine } = await import('../../lib/verification/done-line.mjs');
|
||||
const repo = gitRepo('r630-r8-done-');
|
||||
try {
|
||||
writeFileSync(join(repo, 'README.md'), 'x\n');
|
||||
commitAll(repo, 'base');
|
||||
mkdirSync(join(repo, 'tests'));
|
||||
writeFileSync(join(repo, 'tests', 'hang.test.mjs'), "import { test } from 'node:test';\ntest('hangs', () => new Promise((r) => setTimeout(r, 6000)));\n");
|
||||
const r = writeDoneLine({ cwd: repo, step: 1, cmd: 'node --test tests/hang.test.mjs', report: join(repo, 'report.md'), timeoutMs: 2000 });
|
||||
assert.equal(r.status, 'not-run', JSON.stringify(r));
|
||||
assert.match(r.reason, /killed .*after the 2 s timeout/, r.reason);
|
||||
} finally { rmSync(repo, { recursive: true, force: true }); }
|
||||
for (const rel of ['lib/verification/done-line.mjs', 'lib/util/test-lock.mjs', 'lib/verification/red-proof.mjs']) {
|
||||
const src = read(rel);
|
||||
assert.match(src, /\bstoppedBy\(/, `${rel} reads a stop with stoppedBy`);
|
||||
assert.doesNotMatch(src, /r\.error \|\| r\.status === null/, `${rel} does not read the spawn result by hand`);
|
||||
}
|
||||
});
|
||||
|
||||
test('review 630 r8 / red-proof.mjs:101: a run reports one reading of a stop, not a second spawnError beside it', async () => {
|
||||
const { proveRed } = await import('../../lib/verification/red-proof.mjs');
|
||||
const repo = gitRepo('r630-r8-spawn-');
|
||||
try {
|
||||
writeFileSync(join(repo, 'README.md'), 'x\n');
|
||||
const base = commitAll(repo, 'base');
|
||||
mkdirSync(join(repo, 'tests'));
|
||||
writeFileSync(join(repo, 'tests', 'a.test.mjs'), 'x\n');
|
||||
// `jest` is on the allowlist and not installed where this suite runs.
|
||||
const r = proveRed({ cwd: repo, base, cmd: 'jest tests/a.test.mjs', tests: ['tests/a.test.mjs'], redOnly: true });
|
||||
assert.equal(r.status, 'not-run', JSON.stringify(r));
|
||||
assert.match(r.reason, /could not start/, r.reason);
|
||||
const runs = [r.base, r.result].filter(Boolean);
|
||||
assert.ok(runs.length > 0, JSON.stringify(r));
|
||||
for (const run of runs) assert.ok(!('spawnError' in run), `no spawnError beside stopped: ${JSON.stringify(run)}`);
|
||||
} finally { rmSync(repo, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('review 630 r8 / findings-schema.mjs:46: the block reader and the verdict gate agree on what a fence is', async () => {
|
||||
const { extractFindingsBlock } = await import('../../lib/review/findings-schema.mjs');
|
||||
const json = '{\n "findings": []\n}';
|
||||
// Markdown: a closing fence may be longer than the opening one.
|
||||
assert.equal(extractFindingsBlock(`x\n\n\`\`\`json\n${json}\n\`\`\`\`\n`), `${json}\n`, 'a ```` closer ends the block');
|
||||
// Not a fence in Markdown, and not in the verdict gate either: three backticks
|
||||
// after text on the same line.
|
||||
assert.equal(extractFindingsBlock(`\`\`\`json\n${json}\`\`\`\n`), null, 'a closer after JSON on its line');
|
||||
assert.equal(extractFindingsBlock(`see \`\`\`json\n${json}\n\`\`\`\n`), null, 'an opener after prose on its line');
|
||||
const { decideVerdict } = await import('../../lib/review/verdict-gate.mjs');
|
||||
const fm = ['---', 'type: trekreview', 'review_version: "1.1"', 'created: 2026-10-11', 'task: "t"', 'slug: s',
|
||||
'project_dir: p/', 'brief_path: p/brief.md', `scope_sha_start: ${'b'.repeat(40)}`, `scope_sha_end: ${'c'.repeat(40)}`,
|
||||
'reviewed_files_count: 1', 'verdict: WARN', 'findings: []', '---', ''].join('\n');
|
||||
const payload = JSON.stringify({ verdict: 'WARN', counts: { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 }, allow_blocked_by: [], findings: [] }, null, 2);
|
||||
for (const closer of ['```', '````']) {
|
||||
const v = decideVerdict(`${fm}\n# R\n\n## Executive Summary\n\nx\n\n## Coverage\n\nx\n\n## Declined to Judge\n\nNone.\n\nTrimmed: 0\n\n` +
|
||||
`## Remediation Summary\n\nx\n\n\`\`\`json\n${payload}\n${closer}\n`);
|
||||
assert.equal(v.exit, 0, `closer ${closer}: ${v.reason}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('review 630 r8 / review-validator.mjs:36 + synthesis-digest-schema.mjs:59: three backticks in a JSON string do not end the block there either', async () => {
|
||||
const { validateReviewContent } = await import('../../lib/validators/review-validator.mjs');
|
||||
const { extractDigestBlock } = await import('../../lib/plan/synthesis-digest-schema.mjs');
|
||||
const id = 'a'.repeat(40);
|
||||
const payload = { verdict: 'BLOCK', counts: { BLOCKER: 1, MAJOR: 0, MINOR: 0, SUGGESTION: 0 }, allow_blocked_by: [],
|
||||
findings: [{ id, severity: 'BLOCKER', rule_key: 'MISSING_TEST', file: 'a.mjs', line: 1, title: 'x',
|
||||
detail: 'write the check as ```bash\nnpm test\n``` and run it', recommended_action: 'Add a test.' }] };
|
||||
const review = ['---', 'type: trekreview', 'review_version: "1.1"', 'created: 2026-10-11', 'task: "t"', 'slug: s',
|
||||
'project_dir: p/', 'brief_path: p/brief.md', 'scope_sha_start: ' + 'b'.repeat(40), 'scope_sha_end: ' + 'c'.repeat(40),
|
||||
'reviewed_files_count: 1', 'verdict: BLOCK', 'findings:', ` - ${id}`, '---', '', '# Review', '',
|
||||
'## Executive Summary', '', 'x', '', '## Coverage', '', 'x', '', '## Declined to Judge', '', 'None.', '', 'Trimmed: 0', '',
|
||||
'## Remediation Summary', '', 'x', '', '```json', JSON.stringify(payload, null, 2), '```', ''].join('\n');
|
||||
const r = validateReviewContent(review);
|
||||
assert.ok([...r.errors, ...r.warnings].some((i) => i.code === 'REVIEW_MISSING_REMEDIATION_STEP'),
|
||||
`a BLOCKER with no ### Step R<n> is found although its detail holds a fence: ${JSON.stringify(r.errors.map((e) => e.code))}`);
|
||||
const digest = '```json\n{"note": "a ```bash fence"}\n```\n';
|
||||
assert.equal(extractDigestBlock(digest), '{"note": "a ```bash fence"}\n');
|
||||
});
|
||||
|
|
|
|||
|
|
@ -489,3 +489,26 @@ test('release-check r7 — a relative --review is read in --repo, like every oth
|
|||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
// ==== round 8 of the release review of 6.3.0 (2026-10-11), fixed in 6.3.1 ====
|
||||
|
||||
test('release-check r8 — a relative --review written from the caller\'s directory, as 6.3.0 read it, still works → 0', () => {
|
||||
const fx = fixture();
|
||||
try {
|
||||
// The review sits beside the repo (fx.base/review.md); run from fx.base.
|
||||
const r = spawnSync(process.execPath, [CHECK, '--repo', 'repo', '--review', 'review.md'], { cwd: fx.base, encoding: 'utf8' });
|
||||
assert.doesNotMatch(r.stdout, /does not exist/, r.stdout);
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
||||
test('release-check r8 — a relative --review that names two different files is not guessed → 2', () => {
|
||||
const fx = fixture();
|
||||
try {
|
||||
writeFileSync(join(fx.repo, 'review.md'), readFileSync(fx.review, 'utf8'));
|
||||
writeFileSync(join(fx.repo, '.git', 'info', 'exclude'), 'review.md\n');
|
||||
const r = spawnSync(process.execPath, [CHECK, '--repo', 'repo', '--review', 'review.md'], { cwd: fx.base, encoding: 'utf8' });
|
||||
assert.equal(r.status, 2, r.stdout + r.stderr);
|
||||
assert.match(r.stdout, /two files/, r.stdout);
|
||||
} finally { fx.cleanup(); }
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue