fix(execute): one definition of the step's test files, read as decision 20 sets it
Review finding cf241526. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
734b4a23ae
commit
0142034979
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 135 additions and 18 deletions
|
|
@ -35,14 +35,16 @@ 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).
|
||||
The step's test files are every file on the `Test first:` `File:` line:
|
||||
the backticked paths that stand right after `File:`, separated only by
|
||||
commas or spaces; a path in the description after them is not a test file
|
||||
(every plan_version; operator decision 20, 2026-10-12).
|
||||
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.)
|
||||
(`--tests` lists the step's test files. Red proof refuses a list that
|
||||
differs from the plan's and prints the plan's list as `plan_tests`.)
|
||||
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.
|
||||
|
|
@ -51,10 +53,14 @@ and never touch a file the step does not list.
|
|||
`Files:` alone decides this, not what you have changed: the behaviour
|
||||
already exists at the start point, and no test can be red here. Do not rewrite the test and do not build; say so in
|
||||
the report and stop. The executor sends the step up to the plan.
|
||||
Exit 2 = it could not run (the Verify command is not one test-runner
|
||||
invocation, or a file is missing): on `plan_version` ≥ 1.8 do not build;
|
||||
say so in the report and stop. Below 1.8, write "red proof not run:
|
||||
{reason}" in the report and go on to step 4 without a lock.
|
||||
Exit 2 = it could not run. When its reason starts `tests-mismatch` (the
|
||||
`--tests` you gave differs from the plan's list), run the gate once more
|
||||
with `plan_tests` as `--tests`, whatever the plan_version; only a second
|
||||
exit 2 goes on to the two branches that follow. Exit 2 otherwise (the
|
||||
Verify command is not one test-runner invocation, or a file is missing):
|
||||
on `plan_version` ≥ 1.8 do not build; say so in the report and stop.
|
||||
Below 1.8, write "red proof not run: {reason}" in the report and go on to
|
||||
step 4 without a lock.
|
||||
|
||||
**A step with no `Test first:`** (allowed only below plan_version 1.8):
|
||||
skip steps 1–3, write "no Test first: no test written, no proof of red" in
|
||||
|
|
@ -62,8 +68,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). `--files` lists every file on the `Test first:`
|
||||
`File:` line, the same list as `--tests`.
|
||||
mark the step green). `--files` is the list red proof accepted: the
|
||||
`--tests` it ran without `tests-mismatch` (after a rerun, `plan_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). 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).
|
||||
plan_version ≥ 1.8, see below). Its files are the step's test files, as Sub-step B defines them.
|
||||
- **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 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
|
||||
**The step's test files** are every file on the `Test first:` `File:` line: the backticked paths that stand right after `File:`, separated only by commas or spaces; a path in the description after them is not a test file (every plan_version; operator decision 20, 2026-10-12). `red-proof.mjs --plan --step` reads the same list from the plan and refuses a `--tests` that differs (exit 2, `tests-mismatch`); on that answer the executor runs it once more with the list it printed (`plan_tests`), and a second exit 2 is read as the exit-2 row below says. The lock's `--files` is the list red proof accepted. 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`. 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).
|
||||
step up to the plan. The plan's `Files:` alone decides `tests_only`. `--tests` names the step's test files, as Sub-step B defines them.
|
||||
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 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
|
||||
`--tests` names the step's test files, as Sub-step B defines them. 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
|
||||
|
|
@ -1489,7 +1489,7 @@ already rejects such a step (`PLAN_STEP_VERIFY_NOT_RUNNABLE`).
|
|||
| 1, `not-red-at-base`, `tests_only: true` | `"rejected"` | every file in the step's `Files:` is a test (the plan validator's `PLAN_STEP_TESTS_ONLY`, the same reading). The plan's `Files:` alone decides: the step's diff is not read, and a path changed outside `Files:` is the controller's scope finding (Sub-step C). The behaviour its tests cover already exists at the start point (an earlier step built it, or it was there before the plan), so the step can never be red and no rewrite of the test can fix that. The plan is what failed: the step goes straight up to the plan (Sub-step F, `fix_rounds = 4`, stop class 4), with no fix round — the planner moves the test into the step that builds the behaviour, or makes a test meant to pass from the start the `Regression:` of the step that changes the code |
|
||||
| 1, `not-red-at-base`, otherwise | `"rejected"` | the test passes without the change, so the TEST is wrong: a finding for Sub-step F, and that fix round is the one exception to the lock — the executor releases the step's test lock for it, the fix helper rewrites the test, passes the `--red-only` gate and locks again. `tests_only: null` (the plan or the step could not be read) counts here too |
|
||||
| 1, `not-green-at-result` | `"rejected"` | the change does not make the test pass: a finding for Sub-step F |
|
||||
| 2 | `"not-run"` | never a pass and never proven. **plan_version ≥ 1.8:** when the reason is the command (the `Verify:` is not one test-runner invocation), the plan is what failed: the step goes straight up to the plan (Sub-step F, `fix_rounds = 4`, stop class 4); any other reason (a missing test file, an unresolvable start point, a control run that stays red) is a finding for Sub-step F. **Below 1.8:** a step with no `Test first:`, or whose `Verify:` is not a test-runner command, is tolerated as `"not-run"` — counted in the final report among the steps without proof of red, never called proven; any other reason is a finding |
|
||||
| 2 | `"not-run"` | never a pass and never proven. **`tests-mismatch` first:** a reason that starts `tests-mismatch` (the `--tests` given differs from the plan's `Test first:` `File:` line) is neither the plan's failure nor a finding at first: the executor reruns once with the list red proof printed (`plan_tests`) and reads that answer; only a second exit 2 goes on below. **plan_version ≥ 1.8:** when the reason is the command (the `Verify:` is not one test-runner invocation), the plan is what failed: the step goes straight up to the plan (Sub-step F, `fix_rounds = 4`, stop class 4); any other reason (a missing test file, an unresolvable start point, a control run that stays red) is a finding for Sub-step F. **Below 1.8:** a step with no `Test first:`, or whose `Verify:` is not a test-runner command, is tolerated as `"not-run"` — counted in the final report among the steps without proof of red, never called proven; any other reason is a finding |
|
||||
|
||||
The exit code is the verdict, the output text is not. `base.tap` (when the
|
||||
runner prints TAP) says what the red was: `fail 1 of 1` on a node test file is
|
||||
|
|
@ -1497,7 +1497,7 @@ 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}"`.
|
||||
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
|
||||
`--files` names the step's test files, as Sub-step B defines them (the list red proof accepted). 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.
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
#!/usr/bin/env bash
|
||||
# tests/e2e-files-trial4.sh
|
||||
# End to end for the trial-4 plan (step 13): the three CLIs run the way a
|
||||
# command runs them.
|
||||
# End to end for the trial-4 plan (steps 11 and 13): the four CLIs run the
|
||||
# way a command runs them.
|
||||
# 1. lib/validators/plan-validator.mjs --strict --json on three plans built
|
||||
# with tests/helpers/plan-v18-builder.mjs: a valid 1.9 plan (exit 0), a
|
||||
# 1.9 plan with an unticked Files entry (exit 1, PLAN_STEP_PATH_NOT_FIRST),
|
||||
|
|
@ -10,6 +10,13 @@
|
|||
# the trailer is the message's last line), without (exit 1, HEAD unchanged).
|
||||
# 3. lib/review/verdict-gate.mjs on the legal (exit 0) and the illegal
|
||||
# (exit 2) brief-conflict review fixtures.
|
||||
# 4. plan-validator on a 1.8 plan whose Test first File line names a source
|
||||
# path in its description (exit 0, no PLAN_STEP_TESTS_ONLY: decision 20).
|
||||
# 5. plan-validator on a 1.9 plan whose Files is a backticked None (exit 1,
|
||||
# PLAN_STEP_PATH_NOT_A_PATH).
|
||||
# 6. lib/verification/red-proof.mjs with --plan/--step and a --tests that
|
||||
# differs from the plan's list (exit 2, tests-mismatch).
|
||||
# 7. artifact-commit with an empty-model trailer (exit 1, HEAD unchanged).
|
||||
# Prints "e2e: ok" and exits 0, or names the failed check and exits 1.
|
||||
# bash 3.2 compatible, ASCII only.
|
||||
|
||||
|
|
@ -104,5 +111,51 @@ out="$(node lib/review/verdict-gate.mjs tests/fixtures/review-brief-conflict-ill
|
|||
rc=$?
|
||||
[ "$rc" -eq 2 ] || fail "verdict-gate on review-brief-conflict-illegal.md: exit $rc, expected 2: $out"
|
||||
|
||||
# ---- 4. and 5. the plan validator on decision 20 and on a placeholder ----
|
||||
|
||||
node --input-type=module -e '
|
||||
import { writeFileSync } from "node:fs";
|
||||
import { buildPlan } from "./tests/helpers/plan-v18-builder.mjs";
|
||||
const dir = process.argv[1];
|
||||
const e2e = { endToEnd: "bash tests/e2e.sh" };
|
||||
writeFileSync(dir + "/plan-1-8-description.md", buildPlan({ steps: [{
|
||||
file: "src/parser.mjs", test: "tests/parser.test.mjs",
|
||||
filesRaw: "`src/parser.mjs`, `tests/parser.test.mjs`",
|
||||
testFirstRaw: " - File: `tests/parser.test.mjs` (new) - covers `parse()` in `src/parser.mjs`",
|
||||
}, e2e] }));
|
||||
writeFileSync(dir + "/plan-1-9-none.md",
|
||||
buildPlan({ version: "1.9", steps: [{ filesRaw: "`None`" }, e2e] }));
|
||||
' "$TMP" || fail "could not write the plans for checks 4 and 5"
|
||||
|
||||
out="$(node lib/validators/plan-validator.mjs --strict --json "$TMP/plan-1-8-description.md" 2>&1)"
|
||||
rc=$?
|
||||
[ "$rc" -eq 0 ] || fail "1.8 plan with a source path in the File line description: plan-validator exit $rc, expected 0: $out"
|
||||
if echo "$out" | grep -q 'PLAN_STEP_TESTS_ONLY'; then
|
||||
fail "1.8 plan with a source path in the File line description: PLAN_STEP_TESTS_ONLY in: $out"
|
||||
fi
|
||||
|
||||
out="$(node lib/validators/plan-validator.mjs --strict --json "$TMP/plan-1-9-none.md" 2>&1)"
|
||||
rc=$?
|
||||
[ "$rc" -eq 1 ] || fail "1.9 plan whose Files is a backticked None: plan-validator exit $rc, expected 1: $out"
|
||||
echo "$out" | grep -q 'PLAN_STEP_PATH_NOT_A_PATH' \
|
||||
|| fail "1.9 plan whose Files is a backticked None: no PLAN_STEP_PATH_NOT_A_PATH in: $out"
|
||||
|
||||
# ---- 6. red proof refuses a --tests that differs from the plan ----
|
||||
|
||||
out="$(node lib/verification/red-proof.mjs --red-only --base HEAD --cwd "$REPO" --cmd 'node --test tests/parser.test.mjs' --tests tests/other.test.mjs --plan "$TMP/plan-1-8-description.md" --step 1 --json 2>&1)"
|
||||
rc=$?
|
||||
[ "$rc" -eq 2 ] || fail "red-proof with a --tests that differs from the plan: exit $rc, expected 2: $out"
|
||||
echo "$out" | grep -q 'tests-mismatch' \
|
||||
|| fail "red-proof with a --tests that differs from the plan: no tests-mismatch in: $out"
|
||||
|
||||
# ---- 7. the approval commit refuses an empty-model trailer ----
|
||||
|
||||
before="$(git -C "$REPO" rev-parse HEAD)"
|
||||
out="$(node lib/util/artifact-commit.mjs --repo "$REPO" --store local --message 'docs(voyage): approve intent e2e' --trailer 'Co-Authored-By: Claude <noreply@anthropic.com>' 2>&1)"
|
||||
rc=$?
|
||||
[ "$rc" -eq 1 ] || fail "artifact-commit with an empty-model trailer: exit $rc, expected 1: $out"
|
||||
after="$(git -C "$REPO" rev-parse HEAD)"
|
||||
[ "$before" = "$after" ] || fail "artifact-commit with an empty-model trailer: HEAD moved from $before to $after"
|
||||
|
||||
echo 'e2e: ok'
|
||||
exit 0
|
||||
|
|
|
|||
|
|
@ -194,3 +194,61 @@ test('decision 20: a 1.8 line with two File: markers, each followed by a test, g
|
|||
assert.deepEqual(testFirstFiles(body, '1.8'), ['tests/a.test.mjs', 'tests/b.test.mjs']);
|
||||
assert.deepEqual(testFirstFiles(body, null), ['tests/a.test.mjs', 'tests/b.test.mjs']);
|
||||
});
|
||||
|
||||
// ---- one definition in the prose (plan step 11, review cf241526) -----------
|
||||
// The executor and the step helper define the step's test files once, as
|
||||
// decision 20 reads them; every other place points at that definition. Red
|
||||
// proof refuses a --tests that differs from the plan (`tests-mismatch`) and
|
||||
// prints the plan's list (`plan_tests`): the executor and the helper run it
|
||||
// once more with that list, and the lock follows the list red proof accepted.
|
||||
|
||||
const EXEC = ['commands', 'trekexecute.md'];
|
||||
const HELPER = ['agents', 'step-implementer.md'];
|
||||
const readRel = (rel) => readFileSync(join(ROOT, ...rel), 'utf8');
|
||||
const count = (text, s) => text.split(s).length - 1;
|
||||
|
||||
test('prose: neither file reads every backticked span on the File: line any more', () => {
|
||||
for (const rel of [EXEC, HELPER]) {
|
||||
const text = readRel(rel);
|
||||
assert.equal(count(text, 'every backticked span on that line'), 0, `${rel.join('/')} still says "every backticked span on that line"`);
|
||||
}
|
||||
});
|
||||
|
||||
test('prose: the decision-20 definition stands exactly once in each file and keeps the pinned phrase', () => {
|
||||
for (const rel of [EXEC, HELPER]) {
|
||||
const text = readRel(rel);
|
||||
assert.equal((text.match(/operator decision 20(?!\d)/g) || []).length, 1, `${rel.join('/')} holds the decision-20 definition once`);
|
||||
assert.ok(text.includes('every file on the `Test first:` `File:` line'), `${rel.join('/')} keeps the pinned phrase`);
|
||||
}
|
||||
});
|
||||
|
||||
test('prose: the executor definition names tests-mismatch and plan_tests and keeps its line shape', () => {
|
||||
const lines = readRel(EXEC).split('\n');
|
||||
const i = lines.findIndex((l) => l.startsWith("**The step's test files** are"));
|
||||
assert.ok(i >= 0, 'the definition line is there');
|
||||
const def = lines[i];
|
||||
assert.ok(def.includes('operator decision 20'), 'the definition line carries decision 20');
|
||||
assert.ok(def.includes('`tests-mismatch`'), 'the definition names tests-mismatch');
|
||||
assert.ok(def.includes('`plan_tests`'), 'the definition names plan_tests');
|
||||
assert.ok(def.includes('`--files` is the list red proof accepted'), 'the definition says the lock follows the accepted list');
|
||||
assert.ok(def.endsWith(' The'), `the definition line still ends with " The": ${def.slice(-40)}`);
|
||||
assert.ok(lines[i + 1].startsWith('plan template lists them'), `the next line: ${lines[i + 1]}`);
|
||||
});
|
||||
|
||||
test('prose: the executor exit-2 row names tests-mismatch and the rerun with plan_tests', () => {
|
||||
const row = readRel(EXEC).split('\n').find((l) => l.startsWith('| 2 | `"not-run"`'));
|
||||
assert.ok(row, 'the exit-2 row is there');
|
||||
assert.ok(row.includes('`tests-mismatch`'), 'the exit-2 row names tests-mismatch');
|
||||
assert.ok(row.includes('`plan_tests`'), 'the exit-2 row names plan_tests');
|
||||
});
|
||||
|
||||
test('prose: the helper reruns once with plan_tests on tests-mismatch, before the 1.8 branches, and locks the accepted list', () => {
|
||||
const text = readRel(HELPER);
|
||||
const mm = text.match(/tests-mismatch[^.]*once\s+more[^.]*plan_tests/);
|
||||
assert.ok(mm, 'the helper runs the gate once more with plan_tests on tests-mismatch');
|
||||
assert.match(text, /whatever\s+the\s+plan_version/);
|
||||
const below = text.indexOf('Below 1.8');
|
||||
assert.ok(below > 0, 'the helper still has its Below 1.8 branch');
|
||||
assert.ok(mm.index < below, 'the mismatch sentence stands before "Below 1.8"');
|
||||
assert.match(text, /`--files` is the list red proof accepted/);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue