fix(execute): every test file on the Test first line is read, locked and copied
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
6f342a2cdc
commit
8bf97cc834
4 changed files with 129 additions and 13 deletions
|
|
@ -35,10 +35,14 @@ and never touch a file the step does not list.
|
|||
|
||||
1. **Test first.** Read the step's `Test first:` and write that test (and only
|
||||
that test) first. Follow the `Pattern:` file's style when one is named.
|
||||
The step's test files are every file on the `Test first:` `File:` line
|
||||
(below plan_version 1.9 every backticked span on that line with the shape
|
||||
of a path; from 1.9 the backticked paths the line opens with).
|
||||
Then register every new file you created so the executor's diff sees it:
|
||||
`git add --intent-to-add -- <path>` (this stages nothing).
|
||||
2. **Prove it red before you build.** Run, from the repository root:
|
||||
`node {voyage_root}/lib/verification/red-proof.mjs --red-only --base {start_sha} --cmd "<the step's Verify command>" --tests <test files as ONE comma-separated argument: a,b> --plan "{plan}" --step {step}`
|
||||
(`--tests` lists every file on the `Test first:` `File:` line.)
|
||||
Exit 0 = the test fails at the start point: continue. Exit 1 = the test
|
||||
already passes at the start point, so it proves nothing: rewrite the test
|
||||
until it fails for the reason the step exists, and run the check again.
|
||||
|
|
@ -58,7 +62,8 @@ and never touch a file the step does not list.
|
|||
3. **Lock the tests** (when the CLI exists):
|
||||
`node {voyage_root}/lib/util/test-lock.mjs lock --files <test files> --step {step} --cmd "{Verify command}"`
|
||||
(the lock stores the step's test command; only that command can later
|
||||
mark the step green).
|
||||
mark the step green). `--files` lists every file on the `Test first:`
|
||||
`File:` line, the same list as `--tests`.
|
||||
If the file does not exist, write "not-locked (test-lock.mjs missing)" in the
|
||||
report. Never silent. Once locked you cannot edit the tests, and that is the
|
||||
point: the code changes to meet the test, never the reverse.
|
||||
|
|
|
|||
|
|
@ -186,7 +186,7 @@ Extract every `### Step N: {description}` heading (in order). For each step, ext
|
|||
- **Reuses** — existing code to leverage (informational)
|
||||
- **Test first** — the test the step writes before it builds: its `File:`
|
||||
(existing | new), what it `Verifies:`, an optional `Pattern:` (required from
|
||||
plan_version ≥ 1.8, see below)
|
||||
plan_version ≥ 1.8, see below). The step's test files are every file on the `Test first:` `File:` line (below plan_version 1.9 every backticked span on that line with the shape of a path; from 1.9 the backticked paths the line opens with).
|
||||
- **Verify** — command to run after implementation: the first backticked
|
||||
span of the field. It is also the step's **test command**: proof of red
|
||||
(Sub-step B) and the done line (Sub-step G) run it, so it must be one
|
||||
|
|
@ -1451,7 +1451,7 @@ is not proven red at its start point and green at its result does not pass.
|
|||
skill is the advice; the gate below, `red-proof.mjs`, is the guarantee, and it
|
||||
stays a command whatever the skill says.
|
||||
|
||||
**The step's test files** are the `File:` paths under its `Test first:`. The
|
||||
**The step's test files** are every file on the `Test first:` `File:` line (below plan_version 1.9 every backticked span on that line with the shape of a path; from 1.9 the backticked paths the line opens with). The
|
||||
plan template lists them apart from `Files:`, so everywhere below **the step's
|
||||
scope = its `Files:` + its test files**: both are built, both are committed at
|
||||
the checkpoint, and neither is a scope finding.
|
||||
|
|
@ -1469,12 +1469,12 @@ already rejects such a step (`PLAN_STEP_VERIFY_NOT_RUNNABLE`).
|
|||
(the test must FAIL at the start point), locks the test files, and only then
|
||||
implements. A `tests_only: true` answer stops the helper before it builds
|
||||
(it does not rewrite the test); the executor's own run below then sends the
|
||||
step up to the plan. The plan's `Files:` alone decides `tests_only`.
|
||||
step up to the plan. The plan's `Files:` alone decides `tests_only`. The step's test files are every file on the `Test first:` `File:` line (below plan_version 1.9 every backticked span on that line with the shape of a path; from 1.9 the backticked paths the line opens with).
|
||||
2. **The executor proves it both ways after the helper returns:**
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/verification/red-proof.mjs --base {start_sha} --cmd "{Verify command}" --tests {test files, ONE comma-separated argument} --plan "{plan_path}" --step {N} --json
|
||||
```
|
||||
The test must FAIL against the start point and PASS against the result
|
||||
The step's test files are every file on the `Test first:` `File:` line (below plan_version 1.9 every backticked span on that line with the shape of a path; from 1.9 the backticked paths the line opens with). The test must FAIL against the start point and PASS against the result
|
||||
(the working tree), and a **control run** must PASS: the start-point copy
|
||||
with the step's whole change laid on top. The start point is a temp
|
||||
`git clone --shared` checked out at `start_sha` (a real git checkout), with
|
||||
|
|
@ -1496,8 +1496,8 @@ runner prints TAP) says what the red was: `fail 1 of 1` on a node test file is
|
|||
usually a load error, not a named failing test — say so in the step report.
|
||||
|
||||
**The test lock (K8).** After its red gate the helper runs
|
||||
`node ${CLAUDE_PLUGIN_ROOT}/lib/util/test-lock.mjs lock --files {test files} --step {N} --cmd "{Verify command}"`;
|
||||
from then on the guard refuses Write/Edit/NotebookEdit on those files, so the
|
||||
`node ${CLAUDE_PLUGIN_ROOT}/lib/util/test-lock.mjs lock --files {test files} --step {N} --cmd "{Verify command}"`.
|
||||
The step's test files are every file on the `Test first:` `File:` line (below plan_version 1.9 every backticked span on that line with the shape of a path; from 1.9 the backticked paths the line opens with). From then on the guard refuses Write/Edit/NotebookEdit on those files, so the
|
||||
code moves to meet the test and never the reverse. The executor reads
|
||||
`node ${CLAUDE_PLUGIN_ROOT}/lib/util/test-lock.mjs status` and records
|
||||
`steps.{N}.test_lock = "locked"` when it exits 0 and names the step's files.
|
||||
|
|
|
|||
|
|
@ -486,15 +486,44 @@ export function stepTestsOnly(body) {
|
|||
return stepTestsOnlyFromFields(parseStepFields(body));
|
||||
}
|
||||
|
||||
// A line of the `Test first:` field that names its test files (6.3.1's selector).
|
||||
const TEST_FIRST_FILE_LINE = /\bFile:\**[ \t]*`/i;
|
||||
|
||||
/**
|
||||
* Whether a path counts as a test for this step: its own `Test first:` file, or
|
||||
* a path that is clearly a test.
|
||||
* The step's test files: every file on the `Test first:` `File:` line. A line
|
||||
* counts as that line exactly when TEST_FIRST_FILE_LINE matches it. Below
|
||||
* plan_version 1.9 (or without a version) every backticked span on the line,
|
||||
* from the match on, that has the shape of a path is a test file (a call
|
||||
* `parse()`, an option, a URL is not), normalised and deduplicated. A
|
||||
* `Verifies:` line without a `File:` is never read. Every version reads this
|
||||
* way until the 1.9 branch exists.
|
||||
* @param {Map<string, {text: string}>} fields - parseStepFields(body)
|
||||
* @param {string|null} [version] - the plan's plan_version
|
||||
* @returns {string[]}
|
||||
*/
|
||||
export function testFirstFiles(fields, version) {
|
||||
if (!fields || !fields.has('Test first')) return [];
|
||||
const out = [];
|
||||
for (const line of String(fields.get('Test first').text ?? '').split('\n')) {
|
||||
const at = TEST_FIRST_FILE_LINE.exec(line);
|
||||
if (!at) continue;
|
||||
for (const m of line.slice(at.index).matchAll(/`([^`\n]+)`/g)) {
|
||||
const s = m[1].trim();
|
||||
if (!(tickedPath(s) || BUILD_FILE.test(s))) continue;
|
||||
const p = norm(s);
|
||||
if (!out.includes(p)) out.push(p);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a path counts as a test for this step: its own `Test first:` files
|
||||
* (testFirstFiles), or a path that is clearly a test.
|
||||
* @returns {(path: string) => boolean}
|
||||
*/
|
||||
function stepTestFile(f) {
|
||||
const testFirst = f.has('Test first')
|
||||
? [...f.get('Test first').text.matchAll(/\bFile:\**[ \t]*`([^`\n]+)`/gi)].map((m) => norm(m[1].trim()))
|
||||
: [];
|
||||
function stepTestFile(f, version) {
|
||||
const testFirst = testFirstFiles(f, version);
|
||||
return (p) => testFirst.includes(p) ||
|
||||
(isTestPath(p) && (UNDER_TESTS_RE.test(p) || !(DOC_RE.test(p) || DATA_RE.test(p))));
|
||||
}
|
||||
|
|
|
|||
82
tests/lib/test-first-multi-file.test.mjs
Normal file
82
tests/lib/test-first-multi-file.test.mjs
Normal file
|
|
@ -0,0 +1,82 @@
|
|||
// tests/lib/test-first-multi-file.test.mjs
|
||||
// Plan step 3 (files-backticks trial 4): below plan_version 1.9 every test file
|
||||
// on the `Test first:` `File:` line is read, not only the first backticked
|
||||
// span. 6.3.1 read the first span alone, so a step whose `Files:` were its two
|
||||
// `Test first:` files was not seen as tests-only when the second file is not a
|
||||
// test by its path. One reading: testFirstFiles (plan-schema.mjs), used by
|
||||
// stepTestsOnly; the executor and the step helper say the same in prose.
|
||||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import * as m from '../../lib/parsers/plan-schema.mjs';
|
||||
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
const ROOT = join(HERE, '..', '..');
|
||||
|
||||
/** A 1.8-shaped step section: `Files:` and the `Test first:` lines given. */
|
||||
const step = (files, testFirst) => [
|
||||
'### Step 1: x', '',
|
||||
`- **Files:** ${files}`,
|
||||
'- **Changes:** x',
|
||||
'- **Test first:**',
|
||||
...testFirst,
|
||||
'- **Verify:** `node --test tests/a.test.mjs` -> expected: exit 0',
|
||||
].join('\n');
|
||||
|
||||
const TWO = step('`tests/a.test.mjs`, `checks/smoke.mjs`', [
|
||||
' - File: `tests/a.test.mjs`, `checks/smoke.mjs` (new)',
|
||||
' - Verifies: both checks run',
|
||||
]);
|
||||
|
||||
const testFirstFiles = (body, version) => {
|
||||
assert.equal(typeof m.testFirstFiles, 'function', 'plan-schema.mjs exports testFirstFiles');
|
||||
return m.testFirstFiles(m.parseStepFields(body), version);
|
||||
};
|
||||
|
||||
test('a 1.8 step whose Files are its two Test first files is tests-only', () => {
|
||||
// checks/smoke.mjs is not a test by its path: only the Test first line makes it one.
|
||||
assert.equal(m.stepTestsOnly(TWO).testsOnly, true, JSON.stringify(m.stepTestsOnly(TWO)));
|
||||
});
|
||||
|
||||
test('testFirstFiles returns every path on the File: line, below 1.9 and without a version', () => {
|
||||
assert.deepEqual(testFirstFiles(TWO, '1.8'), ['tests/a.test.mjs', 'checks/smoke.mjs']);
|
||||
assert.deepEqual(testFirstFiles(TWO, null), ['tests/a.test.mjs', 'checks/smoke.mjs']);
|
||||
assert.deepEqual(testFirstFiles(TWO), ['tests/a.test.mjs', 'checks/smoke.mjs']);
|
||||
});
|
||||
|
||||
test('testFirstFiles: a call, an option or a URL on the File: line is not read; paths are normalised and deduplicated', () => {
|
||||
const body = step('`tests/a.test.mjs`', [
|
||||
' - File: `tests/a.test.mjs`, `parse()`, `--watch`, `https://example.invalid/x.mjs`, `./tests/a.test.mjs`',
|
||||
' - Verifies: x',
|
||||
]);
|
||||
assert.deepEqual(testFirstFiles(body, '1.8'), ['tests/a.test.mjs']);
|
||||
});
|
||||
|
||||
test('testFirstFiles: a Verifies: line, or a span before File: on the line, is never read', () => {
|
||||
const body = step('`tests/a.test.mjs`', [
|
||||
' - see `lib/before.mjs` File: `tests/a.test.mjs`',
|
||||
' - Verifies: `lib/other.mjs` returns `src/x.mjs`',
|
||||
]);
|
||||
assert.deepEqual(testFirstFiles(body, '1.8'), ['tests/a.test.mjs']);
|
||||
const none = step('`tests/a.test.mjs`', [' - Verifies: `tests/a.test.mjs` runs']);
|
||||
assert.deepEqual(testFirstFiles(none, '1.8'), []);
|
||||
});
|
||||
|
||||
test('a code file beside the test on the File: line still makes the step not tests-only when Files has more', () => {
|
||||
const body = step('`tests/a.test.mjs`, `src/cli.mjs`', [
|
||||
' - File: `tests/a.test.mjs`, `parse()` (new)',
|
||||
' - Verifies: x',
|
||||
]);
|
||||
assert.equal(m.stepTestsOnly(body).testsOnly, false);
|
||||
});
|
||||
|
||||
test('prose: the executor and the step helper name every file on the Test first File: line', () => {
|
||||
const phrase = 'every file on the `Test first:` `File:` line';
|
||||
for (const rel of [['commands', 'trekexecute.md'], ['agents', 'step-implementer.md']]) {
|
||||
const text = readFileSync(join(ROOT, ...rel), 'utf8');
|
||||
assert.ok(text.includes(phrase), `${rel.join('/')} says "${phrase}"`);
|
||||
}
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue