Commit graph

6 commits

Author SHA1 Message Date
02243c6365
fix(verification): the criteria runner screens with an ALLOWLIST, not a denylist
A denylist in front of /bin/sh is whack-a-mole. Measured 2026-09-18, end to
end through both screens: 5 of 11 named evasions ran with real effect - a
`command` prefix reached git, an escaped `rm` inside a shell fence deleted a
directory, `find -delete` deleted a file, `>|` and `tee` wrote outside the
working tree, a python one-liner deleted the whole tree - and 19 of 28 got
past the refusal list on its own. Every quoting, aliasing and indirection form
of the shell is another mole.

So the screen is now an ALLOWLIST. A criterion runs only when its first word
is a known test runner (npm test, npm run <script package.json declares>,
node --test, vitest, jest, pytest, python -m pytest, uv run pytest,
cargo test, go test, make test, bash <script under tests/>, a read-only git
subcommand) AND the command carries no shell operator and no newline.
Everything else is NOT RUN with the reason said out loud: never run, and never
reported as a failure either - an absent measurement is not a finding. That
also closes the smaller hole in the same file: a bare word a sentence merely
names (`whoami`, `login`, `package.json`) is no longer executed, because it is
not a runner.

REFUSED_BY_POLICY is gone with the list that produced it; a command outside
the allowlist is `unrunnable`, which in plan mode still fells the run and in
brief mode is reported to the reviewer as an absent measurement.

What the allowlist deliberately does NOT do, said in the file and in the
reviewer's rubric: it is not a sandbox. `npm test`, `npm run <script>` and
`make test` run whatever the repo's own package.json/Makefile says they run,
including a script that pushes - that is the repo's responsibility. And it
rejects honest commands too: an env prefix, a project's own binary, anything
piped. A check that needs one of those is declared through
`bash tests/<script>.sh`, the documented way in.

Red first: 6 of the new tests fail against the previous runner (measured with
an always-allow shim so the module still loads), including the end-to-end one
where the canary directory was deleted and files were written outside the
tree. The fixtures move from `true`/`false` to two allowlisted shell fixtures,
because `false` is no longer a runner - the fail case must still be a real
non-zero exit, not an unrun criterion.

Suite 1148 (1146/0/2).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 02:51:40 +02:00
52b87978cb
fix(verification): the runner refuses writes a brief may not perform
/trekreview now runs the commands a BRIEF declares, and a brief is an artifact
that can arrive from outside the repo. Measured 2026-09-18 on 6cafb4c: the
executor denylist stopped a download piped into a shell, but the remote-writing
git subcommand and a recursive delete of a path both RAN. The denylist screens
catastrophe (root deletion, fork bombs, mkfs); it was never meant to screen an
artifact under review.

A second screen, in the runner and ahead of the denylist, refuses four classes:

- a remote-writing git subcommand. The subcommand is found by walking git's own
  options (`-C`, `-c`, `--git-dir`, ... take a value), so `git status` and
  `git log` still run and `git -C sub push` does not.
- a recursive delete: any `rm` carrying `-r`/`-rf`/`--recursive`. A plain
  `rm build/artifact.txt` still runs.
- a download piped straight into a shell (also caught by the denylist; pinned
  here so the runner does not depend on another file for it).
- a write outside the working tree. `/dev/null`-class devices are fine, and so
  is anything under the working tree; `~/...`, an absolute path elsewhere, and
  a target carrying an unexpanded `$VAR` are refused - the runner cannot know
  where a variable points, and guessing is how a screen stops screening.

A refusal is its own outcome, REFUSED_BY_POLICY: the command never reaches a
shell, and `summary.ok` is false in both plan and brief mode. For the reviewer,
REFUSED is like NOT RUN - the absence of a measurement, never on its own a
finding - and the rubric and the evidence block both say so.

Chosen deliberately, and it is stricter than today's habit: writing scratch to
/tmp is refused too. The repo's own example plan does `> /tmp/out`. Verification
output belongs in the working tree; exempting the whole system temp dir would
have made the rule unstatable, since a working tree created under /tmp then
contains its own escape hatch.

NOT covered, stated rather than implied:
- other writing git subcommands (tag, remote, config, gc) - only push is listed
- writes through a wrapper: `sh -c '...'`, `xargs`, `find -exec`, a Makefile
  target, a script the criterion invokes. The screen reads the command it is
  given, not what that command goes on to do.
- `>` inside a quoted string reads as a redirect, so a criterion echoing a
  literal `>` is refused. Fail-closed, on purpose.
- the whole surface still runs with the invoking process's permissions; this is
  a refusal list, not a sandbox.

The denylist-layer test now uses a stand-in command with a screen double: the
refusal list catches a recursive delete first, so naming one there would have
stopped exercising the denylist layer at all.

Red first: the 6 new tests failed before this change (`refuseCommand` did not
exist), and the fixture brief's four writes ran.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 02:14:01 +02:00
106dcb0091
fix(verification): a backticked span is only run when it IS a command
"The first backtick span is the command" is right for a plan, whose template
puts the command first, and wrong for a brief, whose criterion usually opens
by NAMING the thing under discussion. Measured 2026-09-18 on the repo's own
example brief: 5 of 6 criteria FAILED, 3 of them parse artifacts - `--verbose`
run as a command gave exit 2 ("invalid option"), `tests/` gave exit 126 ("is a
directory"). The rubric reads a FAILED result as decisive, so each one became
a BROKEN_SUCCESS_CRITERION BLOCKER about prose.

looksLikeCommand() screens the span by SHAPE only - no filesystem lookup, so a
span parses the same everywhere. Refused: a leading flag, a directory, a token
carrying quotes/braces/prose, and a lone relative path with a slash (an
explicit ./, ../, / or ~/ still runs, as do env-var prefixes). A refused span
is `unrunnable` with reason `not-a-command` - its own outcome, never FAILED,
and it never reaches a shell.

It deliberately does NOT scan on to a later span. "The first span that LOOKS
like a command" invents commands out of prose: in that same example brief it
would have run `whoami` and `login`, two real binaries a sentence happens to
name. An absent measurement is honest; a guessed one is not.

The shape check applies to prose spans only. Inside a shell-tagged fence the
author has already declared shell, so `[ -f x ] || exit 1` still runs.

The rubric follows: a NOT RUN result is never on its own a finding. The
Partial row now describes half-built DELIVERED CODE, and the reviewer gets a
table of the three reason strings - no-command, placeholder, not-a-command -
with what each says about the sentence rather than about the code.

Not covered, stated for the record: a multi-token span whose first token is a
non-executable file (`tests/golden/login.stdout --check`) still runs, and a
criterion whose command is real but whose binary is absent still reports the
shell's exit 127 - that is a true measurement of a missing binary, not a
parse artifact.

Red first: 4 runner tests + 1 doc-consistency pin failed before this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 02:04:46 +02:00
0013d292d6
fix(verification): the criteria runner reads the repo's own plan format
The runner read bullet lines only. The repo's own example plan writes its
whole acceptance run as a fenced bash block, so `## Verification` parsed to
ZERO criteria, the runner exited 1, and Phase 7 forbade `result: completed`
- a correct plan felled every single-session run. Measured 2026-09-18 on
6cafb4c: none of the repo's plan artifacts exited 0.

Fences are now read twice over, for two opposite reasons:

- a `## ` heading INSIDE a fence is quoted text and no longer opens a
  section. examples/02-real-cli/REGENERATED.md is a report that quotes a
  plan outline inside one fence; it used to yield an empty section that
  read as "0 of 0", and now yields the honest NO_VERIFICATION_SECTION.
- a shell-tagged fence inside the section holds the commands. The tag list
  is closed (bash/sh/shell/zsh/console/shell-session): an untagged fence is
  more often expected OUTPUT than input, and inventing a criterion from
  output is the failure this file exists to prevent.

Blank and comment-only lines inside the block declare nothing. A `$`/`>`
console prompt is stripped; a `#` root prompt is NOT, because it cannot be
told from a comment and running a comment is the worse mistake.

Measured after the fix (parse only - one example names a fictional CLI):
examples/01 6 criteria, plan-template 2 (both placeholders, correctly NOT
RUN), the two runner fixtures 2 each, plan-run-C 1, REGENERATED.md 0 with
NO_VERIFICATION_SECTION.

Divergence from the order's premise, stated for the record: it said 2 of 3
example plans write `## Verification` as a fenced bash block. Ground truth
is 1 of 3 - REGENERATED.md has no section of its own at all - and the
example plan's block holds 6 command lines, not 5.

Red first: 4 of the 6 new tests failed before this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 02:02:14 +02:00
c23b009738
fix(review): run the success-criteria commands and hand the reviewer the result (D-04)
The rubric required `brief-conformance-reviewer` to classify a Success
Criterion as Full only when "its verification command/test exists and passes".
Its tools are `Read`, `Glob`, `Grep`. It cannot run anything, so "passes" was
either guessed from the command's mere existence or quietly downgraded to
"exists" — a BLOCKER-tier rule key resting on an impression.

The reviewer stays read-only — a reviewer that executes the code it reviews is
not an independent reviewer. The command does the running instead:

- `/trekreview` Phase 4.5 runs the brief's `## Success Criteria` commands
  through `lib/verification/criteria-runner.mjs --brief --evidence` and captures
  the block as `sc_evidence_block`, pasted verbatim into the reviewer prompt in
  Phase 5. The exit code does not stop the review — a failing criterion is
  exactly what the review exists to find.
- `formatCriteriaEvidence` builds that block in code: one row per criterion with
  the command, the exit code and the first output line. Chose a code-built block
  over an orchestrator-written summary so the orchestrator cannot narrate a pass
  that never happened.
- The rubric now judges the supplied result: `PASS` supports Full, `FAILED` /
  `BLOCKED` is `Broken` with the exit code cited, and `NOT RUN` is the absence
  of a measurement — never evidence in either direction.
- Phase 4.5 is skipped in `quick` mode: that mode does not launch the
  conformance reviewer, so there is nobody to hand the result to.

Red first: seven tests in `tests/lib/criteria-runner.test.mjs` against a
committed brief fixture whose three criteria pass, fail, and are prose-only.
The two doc pins were verified red against the pre-fix files (rubric asked
"exists and passes"; no Phase 4.5; the block reached nobody).

Suite: 1117 (1115/0/2), up 9.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 01:34:18 +02:00
e55ca9fc89
fix(execute): run the plan's Verification on the single-session path (D-03)
A trekplan's `## Verification` section is where the brief's success criteria
land. Phase 7 opened with "**Skip for trekplans.**", and only the multi-session
wave path (Phase 2.6 Step 3) ran master verification. A plan executed in ONE
session therefore reported `completed` without ever running the criteria it was
measured against — the executor's own belief was the only evidence.

The check now exists as code, not as an instruction:

- `lib/verification/criteria-runner.mjs` parses the criteria an artifact
  DECLARES (a plan's `## Verification`, a brief's `## Success Criteria`), runs
  each command, and returns a verdict built from exit codes. Fail-closed
  throughout: a placeholder, a prose-only criterion, or an unavailable screen
  is `unrunnable`/`blocked`, never `passed`. A plan with no `## Verification`
  section exits 1 — a plan that promises no end-to-end check cannot be reported
  as verified.
- Every command is screened through the plugin's own PreToolUse denylist
  (`hooks/scripts/pre-bash-executor.mjs`) before it reaches a shell. Chose
  invoking that hook over its documented stdin protocol rather than copying its
  rules, because a command spawned from node never passes through the Bash tool
  and so the hook cannot fire by itself — this keeps exactly one denylist.
- Phase 7 is now "Exit / verification check": session specs run the exit
  condition, trekplans run the criteria runner. Phase 4's entry-condition skip
  for trekplans stands — a plan carries no entry condition; the exit side is
  not symmetrical.
- A failing criterion FELLS the run: `plan_verification.status != "passed"`
  forbids `result: completed`. That is clause 2 of the stop-signal contract,
  now enforced on the single-session path too.

Red first: `tests/lib/criteria-runner.test.mjs` (26 tests) against two committed
fixture plans, one of which declares a criterion that fails on purpose. The
doc pin in `tests/lib/doc-consistency.test.mjs` guards the wiring — a capability
no phase calls is the same defect wearing a lib/ file; verified red against the
pre-fix Phase 7 (skip present, runner absent, no fell-the-run clause).

Suite: 1108 (1106/0/2), up 27 from 1081.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 01:30:57 +02:00