Commit graph

5 commits

Author SHA1 Message Date
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