fix: round 5 of the release review of 6.3.0, the structural fix
Round 5 (v6.2.0..d37e538, WARN: 11 MAJOR, 2 MINOR). The patching did not converge (the same two places every round), so the operator chose a structural fix on 2026-10-11. Each change behind a test that failed first. - A check runs with no shell. criteria-runner's runCommand starts the runner with the very words the screen checked (spawnSync(words[0], words.slice(1)), without NODE_TEST_CONTEXT); Phase 7 criteria, Regression: (its exec copy is gone, MINOR 0e98d5c7), proof of red, the done line and the test lock all run through it. No quote, glob, tilde or variable can rewrite a checked word any more. The screen stays and says why: a word that means something else in a shell (quote, backslash, $, brace, leading ~) is refused; a glob reaches only node --test, which expands its own patterns (sh read `tests/.?/` as `tests/../`); no path climbs out with .. or starts at /; a word starting with @ is refused (pytest reads @file as more arguments). - The K6 plan-file sweep has v6.2.0's own Files: reader again (516dffa): backticked paths, and plain words only in a field without them. The shared reader from round 3 planned directories a description named and dropped paths v6.2.0 planned. fieldPaths stays for the tests-only check, with a path test on spans (`parse()`, `p.parse()` are no paths), plain descriptions ignored when the field has backticked paths, dotfiles, backslash paths and glued spans read. - Skill frontmatter reads a value as YAML does: trailing space is nothing, and a comment ends a plain value, a flow list or a block indicator (`model: opus # pinned`). A comment that cuts a plain description, or is the whole value, is still a hazard. - release-check reads the diff with --no-renames: a test moved into an allowed place after the review shows as removed. - The example plan's step 2 heading no longer says "must FAIL initially"; steps 1-2 keep the tests-only warning (operator 2026-10-09,73262b6). Open: MINOR c1fef1ad (one shared isMain helper) is not in this round. Measured: 0 of 463 Files: fields with backticked paths (520 fields in 182 plans across the marketplace repos) carry another path-like word. Suite 2185 (2183/0/2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
d37e538ddc
commit
c3641ffa42
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
17 changed files with 409 additions and 111 deletions
|
|
@ -7,7 +7,7 @@
|
|||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { cpSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
|
@ -139,11 +139,14 @@ test('review 630 / 2261016d: a SKILL.md of 499 lines ending in a newline passes
|
|||
|
||||
// ---- cb34c32e: regression-check takes commandEnv from the shared util ------------
|
||||
|
||||
test('review 630 / cb34c32e: regression-check imports commandEnv from lib/util/command-env.mjs', () => {
|
||||
test('review 630 / cb34c32e: regression-check takes commandEnv from lib/util/command-env.mjs', () => {
|
||||
// Round 5 (MINOR 0e98d5c7): regression-check has no exec of its own any more;
|
||||
// it runs through the criteria runner's runCommand, which takes commandEnv
|
||||
// from lib/util/command-env.mjs.
|
||||
const src = read('lib/verification/regression-check.mjs');
|
||||
assert.match(src, /import \{ commandEnv \} from '\.\.\/util\/command-env\.mjs';/);
|
||||
assert.doesNotMatch(src, /from '\.\/red-proof\.mjs'/);
|
||||
assert.ok(src.indexOf('function exec(') > src.lastIndexOf('\nimport '), 'exec is defined after the imports');
|
||||
assert.match(src, /runCriteria\(c, o\)/);
|
||||
assert.match(read('lib/verification/criteria-runner.mjs'), /import \{ commandEnv \} from '\.\.\/util\/command-env\.mjs';/);
|
||||
});
|
||||
|
||||
// ---- CLAUDE.md names the mode-read exemptions as they are -----------------------
|
||||
|
|
@ -331,11 +334,15 @@ test('review 630 r3: a fixture document under tests/ is a test file, and "e.g."
|
|||
assert.deepEqual(stepTestsOnly(step('`./tests/a.test.mjs`')).files, ['tests/a.test.mjs'], './ is stripped');
|
||||
});
|
||||
|
||||
test('review 630 r3: the plan-file sweep reads Files: with the same reader as the tests-only check', async () => {
|
||||
test('review 630 r3: the plan-file sweep and the tests-only check both drop a leading ./', async () => {
|
||||
// Round 3 had the sweep read Files: through the tests-only reader; the
|
||||
// operator reversed that on 2026-10-11 (the K6 sweep keeps v6.2.0's reader,
|
||||
// round 5). What the round-3 finding named still holds: `./` is dropped by both.
|
||||
const { plannedPaths } = await import('../../lib/verification/plan-scope-sweep.mjs');
|
||||
const plan = '## Implementation Plan\n\n### Step 1: x\n\n- **Files:** `./src/a.mjs`, tests/b.test.mjs + src/c.mjs\n- **Changes:** x\n';
|
||||
const plan = '## Implementation Plan\n\n### Step 1: x\n\n- **Files:** `./src/a.mjs`, `./tests/b.test.mjs`\n- **Changes:** x\n';
|
||||
const body = plan.slice(plan.indexOf('### Step 1'));
|
||||
assert.deepEqual(plannedPaths(plan), [...stepTestsOnly(body).files].sort());
|
||||
assert.deepEqual(plannedPaths(plan), ['src/a.mjs', 'tests/b.test.mjs']);
|
||||
assert.deepEqual(stepTestsOnly(body).files, ['src/a.mjs', 'tests/b.test.mjs']);
|
||||
});
|
||||
|
||||
test('review 630 r3: --steps-from refuses an appended step with no Verify: on a 1.8 plan too', () => {
|
||||
|
|
@ -423,3 +430,159 @@ test('review 630 r4: no tilde word, and nothing after `make test`', async () =>
|
|||
assert.equal(allowedCommand(cmd, { cwd: ROOT }).allowed, true, `the control: ${cmd}`);
|
||||
}
|
||||
});
|
||||
|
||||
// ==== round 5 of the release review (2026-10-11), each red on d37e538 ===========
|
||||
// Operator 2026-10-11: no more patching of the same two places. A check runs
|
||||
// with no shell (its words reach the runner as written), and the K6 sweep has
|
||||
// its own v6.2.0 reader again; the shared reader stays for the tests-only check.
|
||||
|
||||
const PASS_SCREENS = { allow: () => ({ allowed: true, reason: '' }), screen: () => ({ allowed: true, rule: '' }) };
|
||||
|
||||
test('review 630 r5 / da7584e7: a check runs without a shell - its words reach the runner as written', async () => {
|
||||
const { runCriteria } = await import('../../lib/verification/criteria-runner.mjs');
|
||||
const dir = mkdtempSync(join(tmpdir(), 'r630-noshell-'));
|
||||
try {
|
||||
mkdirSync(join(dir, 'tests'));
|
||||
writeFileSync(join(dir, 'tests', 'args.sh'), 'printf "%s\\n" "$@"\n');
|
||||
const [r] = runCriteria([{ label: 'X1', text: 'x', command: 'bash tests/args.sh ~/x *.sh $HOME' }], { cwd: dir, ...PASS_SCREENS });
|
||||
assert.equal(r.status, 'passed', r.output);
|
||||
assert.equal(r.output, '~/x\n*.sh\n$HOME\n', 'no shell expanded a tilde, a glob or a variable');
|
||||
const [missing] = runCriteria([{ label: 'X2', text: 'x', command: 'no-such-runner-r630 --x' }], { cwd: dir, ...PASS_SCREENS });
|
||||
assert.equal(missing.status, 'failed', 'a runner that is not installed is a failure, never a pass');
|
||||
assert.match(missing.output, /ENOENT|not found/i, missing.output);
|
||||
} finally { rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('review 630 r5 / c25f8704: a glob cannot climb out of tests/ or the repo', async () => {
|
||||
const { runCriteria, allowedCommand } = await import('../../lib/verification/criteria-runner.mjs');
|
||||
const base = mkdtempSync(join(tmpdir(), 'r630-escape-'));
|
||||
const repo = join(base, 'repo');
|
||||
try {
|
||||
mkdirSync(join(repo, 'tests'), { recursive: true });
|
||||
mkdirSync(join(repo, 'outside'));
|
||||
writeFileSync(join(repo, 'outside', 'evil.sh'), 'echo ESCAPED > escaped-bash.txt\n');
|
||||
writeFileSync(join(repo, 'tests', 'a.test.mjs'), "import { test } from 'node:test';\ntest('in', () => {});\n");
|
||||
writeFileSync(join(base, 'outside.test.mjs'),
|
||||
`import { test } from 'node:test';\nimport { writeFileSync } from 'node:fs';\ntest('out', () => { writeFileSync(${JSON.stringify(join(base, 'escaped-node.txt'))}, 'x'); });\n`);
|
||||
const results = runCriteria([
|
||||
{ label: 'E1', text: 'x', command: 'bash tests/.?/outside/evil.sh' },
|
||||
{ label: 'E2', text: 'x', command: 'node --test .*' },
|
||||
], { cwd: repo });
|
||||
assert.equal(existsSync(join(repo, 'escaped-bash.txt')), false, `bash ran a script outside tests/: ${JSON.stringify(results[0])}`);
|
||||
assert.equal(existsSync(join(base, 'escaped-node.txt')), false, `node --test ran a test file outside the repo: ${JSON.stringify(results[1])}`);
|
||||
assert.notEqual(results[0].status, 'passed');
|
||||
for (const cmd of ['node --test ../outside.test.mjs', 'node --test tests/../../outside.test.mjs', 'node --test /tmp/x.test.mjs',
|
||||
'pytest ../tests', 'go test ../...', 'npm test -- ../x.test.mjs', 'jest /abs/x.test.js', 'vitest run ../x', 'bash tests/*.sh',
|
||||
'pytest tests/test_*.py', 'jest src/*.test.js', 'npm test -- tests/*.mjs']) {
|
||||
assert.equal(allowedCommand(cmd, { cwd: ROOT }).allowed, false, `${cmd} must not run`);
|
||||
}
|
||||
for (const cmd of ['node --test tests/lib/*.test.mjs', 'node --test tests/**/*.test.mjs', 'go test ./...', '/bin/bash tests/fixtures/criteria-exit-0.sh',
|
||||
'node --test tests/', 'pytest tests/', 'git diff HEAD~1..HEAD --stat']) {
|
||||
const r = allowedCommand(cmd, { cwd: ROOT });
|
||||
assert.equal(r.allowed, true, `the control: ${cmd} (${r.reason})`);
|
||||
}
|
||||
} finally { rmSync(base, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('review 630 r5 / 8f3cf84d: a word starting with @ is refused (pytest reads @file as more arguments)', async () => {
|
||||
const { allowedCommand } = await import('../../lib/verification/criteria-runner.mjs');
|
||||
for (const cmd of ['pytest @tests/args.txt', 'python -m pytest @tests/args.txt', 'python3 -m pytest -q @a', 'uv run pytest @a', 'node --test @a', 'jest @a']) {
|
||||
const r = allowedCommand(cmd, { cwd: ROOT });
|
||||
assert.equal(r.allowed, false, `${cmd} must not run`);
|
||||
assert.match(r.reason, /@/);
|
||||
}
|
||||
});
|
||||
|
||||
test('review 630 r5 / da7584e7: every place that runs a check runs it with no shell', () => {
|
||||
for (const rel of ['lib/verification/criteria-runner.mjs', 'lib/verification/regression-check.mjs', 'lib/verification/red-proof.mjs',
|
||||
'lib/verification/done-line.mjs', 'lib/util/test-lock.mjs']) {
|
||||
const src = read(rel);
|
||||
assert.doesNotMatch(src, /spawnSync\(\s*['"](?:\/bin\/)?(?:ba)?sh['"]/, `${rel} starts a shell`);
|
||||
assert.doesNotMatch(src, /shell:\s*true/, `${rel} asks spawn for a shell`);
|
||||
if (rel !== 'lib/verification/criteria-runner.mjs') assert.match(src, /\brunCommand\b/, `${rel} runs through criteria-runner's runCommand`);
|
||||
}
|
||||
assert.doesNotMatch(read('lib/verification/regression-check.mjs'), /function exec\(/, 'regression-check has no exec of its own (MINOR 0e98d5c7)');
|
||||
});
|
||||
|
||||
test('review 630 r5 / da7584e7: the done line runs its command with no shell', async () => {
|
||||
// sh reads `**` as `*` (one level); node --test expands its own pattern at
|
||||
// every depth. Only with no shell does the top-level test run.
|
||||
const { writeDoneLine } = await import('../../lib/verification/done-line.mjs');
|
||||
const repo = mkdtempSync(join(tmpdir(), 'r630-done-'));
|
||||
try {
|
||||
mkdirSync(join(repo, 'tests', 'sub'), { recursive: true });
|
||||
writeFileSync(join(repo, 'tests', 'sub', 'deep.test.mjs'), "import { test } from 'node:test';\ntest('deep', () => {});\n");
|
||||
writeFileSync(join(repo, 'tests', 'top.test.mjs'),
|
||||
`import { test } from 'node:test';\nimport { writeFileSync } from 'node:fs';\ntest('top', () => { writeFileSync(${JSON.stringify(join(repo, 'top-ran.txt'))}, 'x'); });\n`);
|
||||
for (const args of [['init', '-q'], ['-c', 'user.email=t@example.invalid', '-c', 'user.name=t', 'commit', '-q', '--allow-empty', '-m', 'x']]) {
|
||||
assert.equal(spawnSync('git', args, { cwd: repo }).status, 0);
|
||||
}
|
||||
const r = writeDoneLine({ cwd: repo, step: 1, cmd: 'node --test tests/**/*.test.mjs', report: join(repo, 'report.md') });
|
||||
assert.equal(r.status, 'written', JSON.stringify(r));
|
||||
assert.equal(existsSync(join(repo, 'top-ran.txt')), true, 'the pattern reached node as written');
|
||||
} finally { rmSync(repo, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test('review 630 r5 / ff80c7ef 5dc53c7e 02cea4aa: the K6 sweep plans what v6.2.0 planned, with its own reader', async () => {
|
||||
const { plannedPaths } = await import('../../lib/verification/plan-scope-sweep.mjs');
|
||||
const plan = (files) => `## Implementation Plan\n\n### Step 1: x\n\n- **Files:** ${files}\n- **Changes:** x\n`;
|
||||
const cases = [
|
||||
['`src/a.mjs` — replaces the helper in lib/util.mjs and touches config.json; see Node.js docs', ['src/a.mjs']],
|
||||
['`src/cli.mjs` — follows the pattern in lib/', ['src/cli.mjs']],
|
||||
['.gitignore, src/a.mjs', ['.gitignore', 'src/a.mjs']],
|
||||
['src\\cli.mjs', ['src/cli.mjs']],
|
||||
['`src/a.mjs`/`src/b.mjs`', ['src/a.mjs', 'src/b.mjs']],
|
||||
['`src/a.mjs`—adds', ['src/a.mjs']],
|
||||
['`src/cli.mjs`: add the flag; `src/b.mjs`.', ['src/b.mjs', 'src/cli.mjs']],
|
||||
];
|
||||
for (const [files, want] of cases) assert.deepEqual(plannedPaths(plan(files)), want, files);
|
||||
assert.doesNotMatch(read('lib/verification/plan-scope-sweep.mjs'), /import \{[^}]*\bfieldPaths\b/, 'K6 does not read Files: through the tests-only reader');
|
||||
});
|
||||
|
||||
test('review 630 r5 / a4c8005d 02cea4aa: the tests-only check takes only what passes a path test', () => {
|
||||
const step = (files) => `- **Files:** ${files}\n- **Changes:** x\n`;
|
||||
const cases = [
|
||||
['`tests/parse.test.mjs` (asserts `parse()` throws)', true, ['tests/parse.test.mjs']],
|
||||
['`tests/a.test.mjs` — covers pass/fail', true, ['tests/a.test.mjs']],
|
||||
['`tests/a.test.mjs` — covers JSON, YAML', true, ['tests/a.test.mjs']],
|
||||
['`tests/a.test.mjs` — calls `parser.parse()` twice', true, ['tests/a.test.mjs']],
|
||||
['`tests/a.test.mjs`, .gitignore', false, ['tests/a.test.mjs', '.gitignore']],
|
||||
['tests/a.test.mjs, .gitignore', false, ['tests/a.test.mjs', '.gitignore']],
|
||||
['`tests/a.test.mjs`/`src/b.mjs`', false, ['tests/a.test.mjs', 'src/b.mjs']],
|
||||
['`tests\\a.test.mjs`', true, ['tests/a.test.mjs']],
|
||||
['tests\\a.test.mjs, src\\cli.mjs', false, ['tests/a.test.mjs', 'src/cli.mjs']],
|
||||
['Makefile.', false, ['Makefile']],
|
||||
];
|
||||
for (const [files, testsOnly, want] of cases) assert.deepEqual(stepTestsOnly(step(files)), { testsOnly, files: want }, files);
|
||||
});
|
||||
|
||||
test('review 630 r5 / e1ff583a f582b9c2: frontmatter reads trailing space and a comment after a plain value as YAML does', async () => {
|
||||
const { readFrontmatterFields } = await import('../../lib/skills/skill-frontmatter.mjs');
|
||||
const r = readFrontmatterFields('---\nname: "debugging" \nmodel: opus # operator pin\neffort: high # pinned\ntags: [a] \n' +
|
||||
'allowed-tools: [Read, Grep] # read only\ndescription: | # folded below\n Use when x.\n---\nbody\n');
|
||||
assert.deepEqual(r.hazards, [], JSON.stringify(r.hazards));
|
||||
assert.equal(r.fields.name.value, 'debugging');
|
||||
assert.equal(r.fields.model.value, 'opus');
|
||||
assert.equal(r.fields.effort.value, 'high');
|
||||
assert.deepEqual(r.fields.tags.value, ['a']);
|
||||
assert.deepEqual(r.fields['allowed-tools'].value, ['Read', 'Grep']);
|
||||
assert.equal(r.fields.description.value, 'Use when x.\n');
|
||||
// The control: a description is the text Claude Code routes on, so a comment
|
||||
// that cuts it is still a hazard (quote it), and so is a value that is only a comment.
|
||||
for (const fm of ['description: Use when x happens #1.', 'description: Use when x # note', 'name: # x']) {
|
||||
assert.ok(readFrontmatterFields(`---\n${fm}\n---\nbody\n`).hazards.length > 0, fm);
|
||||
}
|
||||
});
|
||||
|
||||
test('review 630 r5 / 41c43285 (operator 2026-10-09): the example plan keeps steps 1-2, and step 2 no longer says it must fail', () => {
|
||||
const plan = read('examples/01-add-verbose-flag/plan.md');
|
||||
const heading = plan.match(/^### Step 2: .*$/m)[0];
|
||||
assert.doesNotMatch(heading, /must FAIL/i, heading);
|
||||
assert.match(plan.slice(plan.indexOf(heading)), /should PASS today/, 'the step body still says it passes today');
|
||||
const progress = JSON.parse(read('examples/01-add-verbose-flag/progress.json'));
|
||||
const title = JSON.stringify(progress).match(/"title":"(Add stability test[^"]*)"/)?.[1];
|
||||
assert.equal(`### Step 2: ${title}`, heading, 'progress.json carries the same step title');
|
||||
const r = validatePlanContent(plan, { strict: false });
|
||||
const warned = r.warnings.filter((w) => w.code === 'PLAN_STEP_TESTS_ONLY').map((w) => w.message);
|
||||
assert.equal(warned.length, 2, `the operator kept the warning on steps 1-2: ${JSON.stringify(warned)}`);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue