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>
648 lines
29 KiB
Markdown
648 lines
29 KiB
Markdown
---
|
||
name: trekreview
|
||
description: |
|
||
Independent post-hoc review of delivered code against the brief. Produces
|
||
review.md with severity-tagged findings (BLOCKER/MAJOR/MINOR/SUGGESTION)
|
||
per Handover 6 (review → plan).
|
||
argument-hint: "--project <dir> [--since <ref>] [--quick] [--validate] [--dry-run]"
|
||
allowed-tools: Agent, Read, Glob, Grep, Write, Edit, Bash, AskUserQuestion
|
||
---
|
||
|
||
# Ultrareview Local v1.0
|
||
|
||
Independent post-hoc review of code delivered by `/trekexecute`
|
||
against the contract in `brief.md`. Produces `review.md` — a structured
|
||
artifact with severity-tagged findings that `/trekplan --brief
|
||
review.md` can consume as plan input (Handover 6).
|
||
|
||
Pipeline position:
|
||
|
||
```
|
||
/trekbrief → brief.md
|
||
/trekresearch → research/*.md
|
||
/trekplan → plan.md
|
||
/trekexecute → progress.json (+ commits)
|
||
/trekreview → review.md (this command)
|
||
```
|
||
|
||
The review is **independent**: each reviewer runs without cross-feeding,
|
||
and the coordinator applies BOUNDED operations only. Synthesis-level
|
||
inference across files is forbidden in v1.0 (Judge Agent pattern).
|
||
|
||
See `agents/review-orchestrator.md` for the canonical workflow this
|
||
command executes inline.
|
||
|
||
## Phase 1 — Parse mode and validate input
|
||
|
||
Parse `$ARGUMENTS` via the shared arg-parser:
|
||
|
||
```bash
|
||
node ${CLAUDE_PLUGIN_ROOT}/lib/parsers/arg-parser.mjs --command trekreview "$@"
|
||
```
|
||
|
||
The parser recognizes these flags (see `lib/parsers/arg-parser.mjs`
|
||
FLAG_SCHEMA `trekreview` entry):
|
||
|
||
| Flag | Type | Purpose |
|
||
|------|------|---------|
|
||
| `--project <dir>` | valued | Required. Path to trekplan project folder containing `brief.md`. |
|
||
| `--since <ref>` | valued | Optional. Override "before" SHA for the diff. Validated via `git rev-parse --verify`. |
|
||
| `--quick` | boolean | Skip the brief-conformance pass; run only the code-correctness reviewer; skip the coordinator's reasonableness filter. |
|
||
| `--validate` | boolean | Schema-only check on existing `{project_dir}/review.md`. No LLM calls. |
|
||
| `--dry-run` | boolean | Print the discovered scope and triage map. Skip writes. |
|
||
| `--fg` | boolean | No-op alias (foreground is default). |
|
||
| `--workflow` | boolean | **(opt-in, NW2)** Run Phase 5–6 on the bake-off-validated Workflow substrate (`scripts/trekreview-armB.workflow.mjs`) instead of the default prose Agent-tool path. Requires **Claude Code 2.1.154+**. Combines with `--quick`. See *§ Phase 5–6 via the Workflow substrate*. |
|
||
|
||
Resolution:
|
||
1. If `--project` is missing, print usage and stop:
|
||
```
|
||
Error: --project <dir> is required.
|
||
Usage: /trekreview --project <dir> [--since <ref>] [--quick] [--validate] [--dry-run]
|
||
```
|
||
2. Trim trailing slash from `{dir}`. Set:
|
||
- `project_dir = {dir}`
|
||
- `brief_path = {dir}/brief.md`
|
||
- `review_path = {dir}/review.md`
|
||
3. If `{dir}` does not exist or `{dir}/brief.md` is missing:
|
||
```
|
||
Error: project directory not initialized. Run /trekbrief first.
|
||
Missing: {dir}/brief.md
|
||
```
|
||
|
||
Set `mode`:
|
||
- `validate` if `--validate` is set (overrides everything else; skip to Phase 8.5).
|
||
- `dry-run` if `--dry-run` is set.
|
||
- `quick` if `--quick` is set.
|
||
- `default` otherwise.
|
||
|
||
Set `workflow_substrate` (orthogonal to `mode` — a substrate choice, not a behavior mode):
|
||
- `true` if `--workflow` is set — Phase 5–6 run on the Workflow substrate (see the Phase 5
|
||
routing gate). The Workflow tool requires **Claude Code 2.1.154+**; if it is unavailable,
|
||
fall back to the prose path and note the fallback in the Executive Summary.
|
||
- `false` otherwise. **Default stays prose**: the substrate is opt-in, so the lower
|
||
portability floor of the prose path is preserved unless the operator opts in.
|
||
|
||
## Phase 2 — Validate brief
|
||
|
||
Run the brief validator in soft mode — the brief is upstream context, not
|
||
something this command produces, so partial grades are acceptable as long
|
||
as the file is parseable:
|
||
|
||
```bash
|
||
node ${CLAUDE_PLUGIN_ROOT}/lib/validators/brief-validator.mjs --soft --json "{brief_path}"
|
||
|
||
# v5.9 — composed phase-model resolution (brief > profile > default) for the
|
||
# review phase. ONE call returns {effort, model, source}; captured as
|
||
# phase_signal_result and used in Phase 7 at the reviewer-launch site to
|
||
# inject the resolved model. Append --profile {profile} when the operator
|
||
# passed --profile.
|
||
node ${CLAUDE_PLUGIN_ROOT}/lib/profiles/resolver.mjs --resolve-phase-model --phase review --brief-path "{brief_path}" [--profile {profile}] --json
|
||
```
|
||
|
||
Read the JSON output. If `valid: false` AND any error has code
|
||
`BRIEF_MISSING_REQUIRED_FIELD` or `FRONTMATTER_PARSE_ERROR`: stop and
|
||
ask the user to re-run `/trekbrief`. Other soft errors become
|
||
warnings in the review's Executive Summary.
|
||
|
||
Read the brief frontmatter. Capture for review.md:
|
||
- `task` → review frontmatter `task`
|
||
- `slug` → review frontmatter `slug`
|
||
- `project_dir` → review frontmatter `project_dir` (defaults to the
|
||
CLI `--project` value when missing)
|
||
|
||
## Phase 3 — Discover scope SHA range
|
||
|
||
Determine the "before" SHA that bounds the review:
|
||
|
||
1. **`--since <ref>` override** — if set, validate via:
|
||
```bash
|
||
git rev-parse --verify "$since_ref"
|
||
```
|
||
On failure: print `Error: --since ref is not a valid git revision: {ref}` and stop.
|
||
Set `before_sha = $(git rev-parse --verify "$since_ref")`.
|
||
|
||
2. **Preferred path** — read `{project_dir}/progress.json` if it exists.
|
||
Extract `session_start_sha`. Validate it via `git rev-parse --verify`.
|
||
Set `before_sha = session_start_sha`.
|
||
|
||
3. **Fallback** — no `progress.json`. Use the brief's mtime to find the
|
||
most recent commit at or before the brief was written:
|
||
```bash
|
||
brief_mtime=$(stat -f %m "{brief_path}") # macOS; on Linux use stat -c %Y
|
||
before_sha=$(git log --until="@$brief_mtime" -n 1 --format=%H)
|
||
```
|
||
Emit a clear warning that gets surfaced in the review's Executive
|
||
Summary: "scope_sha_start unavailable — falling back to brief mtime
|
||
({timestamp}). Coverage may include unrelated commits."
|
||
|
||
Compute the "after" SHA: `after_sha=$(git rev-parse HEAD)`.
|
||
|
||
Capture working-tree changes (uncommitted at review time):
|
||
```bash
|
||
git diff --name-only "$before_sha".."$after_sha"
|
||
git diff --name-only HEAD # uncommitted (annotated [uncommitted])
|
||
```
|
||
|
||
The combined file list is the review scope. Note that the
|
||
`[uncommitted]` annotation is a **brief-level contract** — the brief's
|
||
Assumptions section declares this is allowed; the review surfaces it
|
||
explicitly in the Coverage table.
|
||
|
||
If the file count is `0`, write a one-line review.md noting "No diff
|
||
between {before_sha} and {after_sha}; nothing to review." Verdict: ALLOW.
|
||
Skip Phases 4–7. Continue to Phase 8 (validate + stats).
|
||
|
||
## Phase 4 — Triage gate (deterministic path-pattern classifier)
|
||
|
||
The triage gate is **deterministic** — no LLM judgment. It classifies
|
||
every file from Phase 3 into a treatment bucket:
|
||
|
||
| Treatment | When |
|
||
|-----------|------|
|
||
| `skip` | Matches `*.lock`, `*.svg`, `dist/**`, `build/**`, `node_modules/**`, OR the file's first 3 lines contain a generated-file marker (`@generated`, `Code generated by`, `DO NOT EDIT`). |
|
||
| `deep-review` | Matches `auth/**`, `crypto/**`, `**/security/**`, `hooks/**`. |
|
||
| `summary-only` | Default treatment for everything else. |
|
||
|
||
Hard refuse-with-suggestion gates — use `AskUserQuestion`:
|
||
|
||
```
|
||
if (reviewed_files_count > 100) → ask user
|
||
if (estimated_diff_tokens > 100000) → ask user
|
||
```
|
||
|
||
Token estimation: `wc -c "$diff_file" / 4` (rough proxy). Use
|
||
`AskUserQuestion` with the prompt:
|
||
|
||
> The diff under review is large (`{N}` files / `~{T}` tokens). Continue
|
||
> with the full scope, narrow with `--since <closer-ref>`, or stop?
|
||
|
||
Options:
|
||
1. **Continue** — proceed at this scope.
|
||
2. **Narrow** — print suggested `git log --oneline {before}..HEAD` so the
|
||
user can pick a closer ref, then stop.
|
||
3. **Stop** — cancel.
|
||
|
||
Record the treatment for every file. Files marked `skip` MUST appear in
|
||
the Coverage section of `review.md` — never silently drop them. Silent
|
||
drops are `COVERAGE_SILENT_SKIP` (MAJOR) per the rule catalogue.
|
||
|
||
If `mode == dry-run`: print the triage map and exit.
|
||
|
||
## Phase 4.5 — Run the brief's success-criteria checks
|
||
|
||
**Skipped in `quick` mode** (that mode does not launch
|
||
`brief-conformance-reviewer`, so there is nobody to hand the result to) and in
|
||
`dry-run`.
|
||
|
||
`brief-conformance-reviewer` is asked to decide whether each Success Criterion's
|
||
verification command passes. Its tools are `Read`, `Glob`, `Grep` — it cannot
|
||
run anything, and it stays that way: a reviewer that executes the code it
|
||
reviews is not an independent reviewer. So THIS command runs the commands, and
|
||
the reviewer judges the RESULT.
|
||
|
||
```bash
|
||
# Resolve the plugin root ONCE. ${CLAUDE_PLUGIN_ROOT} is substituted in this
|
||
# command's text but is EMPTY in the Bash tool's process env, and a bare
|
||
# `node ${CLAUDE_PLUGIN_ROOT}/lib/…` then runs `node /lib/…`, which exits 1 —
|
||
# indistinguishable from a criterion that failed.
|
||
VOYAGE_ROOT="${CLAUDE_PLUGIN_ROOT:-}"
|
||
case "$VOYAGE_ROOT" in
|
||
/*) ;;
|
||
*) VOYAGE_ROOT="$(ls -d "$HOME"/.claude/plugins/cache/*/voyage 2>/dev/null | head -1)" ;;
|
||
esac
|
||
if [ ! -f "$VOYAGE_ROOT/lib/verification/criteria-runner.mjs" ]; then
|
||
echo "[voyage] success-criteria checks could not run - plugin root unresolved."
|
||
echo " NOT a pass: hand the reviewer NO results and say so."
|
||
exit 2
|
||
fi
|
||
|
||
# Every command is screened twice before it reaches a shell: the runner's own
|
||
# ALLOWLIST of test runners (npm test, node --test, pytest, bash tests/<script>,
|
||
# a read-only git subcommand) reports anything else as NOT RUN, and the executor
|
||
# denylist (catastrophe) reports BLOCKED. Neither is ever run. Foreground only.
|
||
# The working tree the criteria run in - the same resolution trekexecute
|
||
# Phase 7 uses, so one criterion cannot resolve two ways in the two phases.
|
||
CRITERIA_CWD="$(git rev-parse --show-toplevel 2>/dev/null || pwd)"
|
||
|
||
node "$VOYAGE_ROOT/lib/verification/criteria-runner.mjs" \
|
||
--brief "{brief_path}" --evidence --cwd "$CRITERIA_CWD"
|
||
```
|
||
|
||
Exit 0 means every criterion passed; exit 1 means at least one failed or was
|
||
blocked by the executor denylist; a criterion with no command, or one outside
|
||
the allowlist of test runners, is reported as NOT RUN;
|
||
exit 2 means the runner could not run. **The exit
|
||
code does not stop the review** — a failing criterion is exactly what the review
|
||
exists to find. Capture stdout as `sc_evidence_block`.
|
||
|
||
If the runner exits 2 (or the root could not be resolved), set
|
||
`sc_evidence_block` to a single line naming the failure and stating that no
|
||
criterion was checked. An unrun check is never a pass, and the reviewer must be
|
||
told which of the two it is looking at.
|
||
|
||
`sc_evidence_block` is pasted verbatim into the `brief-conformance-reviewer`
|
||
prompt in Phase 5, and its summary line goes into the Coverage section of
|
||
`review.md`. Do NOT summarise, re-word or re-judge it on the way — the block is
|
||
built by `formatCriteriaEvidence` precisely so the orchestrator cannot narrate a
|
||
pass that never happened.
|
||
|
||
## Phase 5 — Launch parallel reviewers
|
||
|
||
**Substrate routing (opt-in `--workflow`).** When `workflow_substrate == true`, run
|
||
Phases 5–6 via the Workflow substrate documented in *§ Phase 5–6 via the Workflow
|
||
substrate* (below Phase 6), then resume at Phase 7 with the returned
|
||
`{verdict, findings}`. When `false` (the default), run the prose Agent-tool path
|
||
described in the rest of this phase.
|
||
|
||
Launch two reviewer agents **in parallel** via the Agent tool — one
|
||
message, multiple tool calls.
|
||
|
||
> **Never pass the Agent tool's `name` parameter — at this or any other spawn
|
||
> site in this command.** `name` does not label a subagent, it changes its kind:
|
||
> the spawn is recorded as `taskKind: "in_process_teammate"` (`spawnDepth: 0`)
|
||
> instead of a subagent (`spawnDepth: 1`). A teammate's final assistant text is
|
||
> not a return value — it reaches the orchestrator only if the teammate itself
|
||
> calls `SendMessage(to: "main")`, and every agent in `agents/` declares a
|
||
> `tools:` allowlist without SendMessage. The agent still runs and still
|
||
> produces correct output; the result is simply never delivered, so the phase
|
||
> presents as a hung agent that no re-prompting can revive. Measured
|
||
> 2026-08-17: named 0/5 returned, unnamed 3/3. Mechanism, denominators, and the
|
||
> on-disk recovery path: `docs/agent-return-channel-defect.md`.
|
||
|
||
Reviewers run independently. Do NOT pre-feed findings between them.
|
||
|
||
| Agent | Mode-gated | Purpose |
|
||
|-------|------------|---------|
|
||
| `brief-conformance-reviewer` | Skipped in `quick` | Trace each Success Criterion + Non-Goal to delivered code. Emits findings tagged with rule_keys from the conformance/scope categories. |
|
||
| `code-correctness-reviewer` | Always runs | 7-dimension code review. Emits findings tagged with rule_keys from the correctness/security/maintenance/tests categories. |
|
||
|
||
Each reviewer prompt includes:
|
||
- **Diff context** — the unified diff from Phase 3, truncated per file
|
||
for files marked `summary-only`.
|
||
- **Triage map** — full file list with treatments. Reviewers must
|
||
respect `skip` decisions.
|
||
- **Brief path** — `{brief_path}` (read on demand; do not inline).
|
||
- **Rule catalogue** — reference to `lib/review/rule-catalogue.mjs`.
|
||
|
||
`brief-conformance-reviewer` additionally receives `sc_evidence_block` from
|
||
Phase 4.5, pasted verbatim — the command, exit code and first line of output for
|
||
every Success Criterion. It is the ONLY evidence that agent has about whether a
|
||
criterion's verification passes, because it cannot run one.
|
||
|
||
Collect each reviewer's trailing JSON block and **validate it against the
|
||
reviewer-output schema** rather than merely parsing it. Run:
|
||
|
||
```bash
|
||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/findings-schema.mjs --json <reviewer-output-file>
|
||
```
|
||
|
||
`validateReviewerOutput` in `lib/review/findings-schema.mjs` extracts the
|
||
last fenced `json` block, parses it, and schema-checks every finding
|
||
(load-bearing fields: `file`, `rule_key` ∈ catalogue, `severity` ∈ enum,
|
||
`line` integer ≥ 0). Parse failure and schema failure surface through the
|
||
same stable error codes (`FINDINGS_NO_JSON_BLOCK`, `FINDINGS_PARSE_ERROR`,
|
||
`FINDING_*`).
|
||
|
||
On any failure, re-ask **that reviewer** to re-emit a conforming JSON
|
||
block only — quote the reported error codes/locations so the fix is
|
||
targeted. **Bounded retries: N=2.** If the output still fails after 2
|
||
re-asks, stop and report which reviewer produced non-conforming output;
|
||
do not feed unvalidated findings to the coordinator.
|
||
|
||
In `quick` mode, launch only `code-correctness-reviewer`. The Executive
|
||
Summary will note the brief-conformance pass was skipped.
|
||
|
||
### Reviewer accounting — every expected reviewer MUST report
|
||
|
||
Write down the expected reviewer set BEFORE the spawn: both reviewers in
|
||
default mode, `code-correctness-reviewer` alone in `quick` mode. After the
|
||
spawn, account for each one by name.
|
||
|
||
**Zero findings from a silent reviewer is indistinguishable from zero findings
|
||
from a clean diff** — unless you check. A reviewer is *accounted for* only when
|
||
it returned a payload that validated. Three ways it fails to:
|
||
|
||
| Failure | Handling |
|
||
|---------|----------|
|
||
| Output fails the schema after the 2 bounded re-asks | STOP (already specified above) |
|
||
| Returned no final message at all | Re-ask that reviewer **once**. Still nothing → STOP. |
|
||
| Was never launched (spawn error, wrong mode) | STOP. |
|
||
|
||
**On STOP: name the reviewer and the failure, and do not proceed to Phase 6.**
|
||
Do not let the coordinator compute a verdict over a review one of whose
|
||
reviewers never spoke — the count would be complete-looking and wrong. This is
|
||
the same shape as the schema branch above ("do not feed unvalidated findings to
|
||
the coordinator"), applied to the other two ways a reviewer can go missing.
|
||
|
||
A reviewer that ran but never delivered is most often the return-channel
|
||
defect: check `~/.claude/projects/<proj>/<session>/subagents/agent-*.jsonl` for
|
||
its final assistant block before re-asking, and confirm no `name` parameter was
|
||
passed at the spawn (see the warning at the top of this phase).
|
||
|
||
If you proceed anyway under an explicit operator instruction, pass the expected
|
||
set to the coordinator as `expectedReviewers` so the missing reviewer at least
|
||
forbids `ALLOW` (`lib/review/coordinator-contract.mjs`, `missing_reviewers`).
|
||
|
||
## Phase 6 — Coordinator dedup + verdict
|
||
|
||
Launch `review-coordinator` (Agent tool) with the merged findings array
|
||
from Phase 5 plus the triage map, brief metadata, and SHA range.
|
||
|
||
The coordinator runs the 4-pass process documented in
|
||
`agents/review-coordinator.md`:
|
||
|
||
1. **Dedup** by `(file, line, rule_key)` triplet.
|
||
2. **HubSpot Judge filters** — Succinctness, Accuracy, Actionability.
|
||
3. **Cloudflare reasonableness** — remove speculative or catalogue-violating
|
||
findings (skipped in `quick` mode).
|
||
4. **Verdict** — BLOCK / WARN / ALLOW per the threshold table.
|
||
|
||
**Fail-closed.** Every removal in Pass 2 and Pass 3 is either
|
||
**dropped** (the test refuted the finding as a claim about this codebase) or
|
||
**unverified** (the finding was removed without its claim ever being settled).
|
||
A non-empty `unverified` bucket forbids `ALLOW`; the verdict becomes `WARN` and
|
||
the Executive Summary's first sentence must say why. The fail-closed rule never
|
||
raises a verdict — it only withholds the clean one. Fate table, reason
|
||
vocabulary, and the `allow_blocked_by` field: `agents/review-coordinator.md`
|
||
§*Suppression is two-valued*, mirrored deterministically in
|
||
`lib/review/coordinator-contract.mjs`.
|
||
|
||
The coordinator's output is the full review.md content — frontmatter +
|
||
body sections + trailing JSON block. Do NOT re-run the reviewers based
|
||
on the coordinator's output.
|
||
|
||
## Phase 5–6 via the Workflow substrate (opt-in `--workflow`)
|
||
|
||
Runs **only** when `workflow_substrate == true`. This is the **NW2 port**: it expresses
|
||
the SAME Phase 5–6 pipeline (parallel reviewers → triplet-dedup → coordinator verdict) as
|
||
a single Workflow, reusing the NW1 findings schema. The S10 bake-off found it
|
||
**fidelity-equivalent** to the prose path — see `docs/T2-bakeoff-results.md` (verdict
|
||
**POSITIVE**: verdict-match 1.0, issue-coverage 100%, `(file,rule_key)` jaccard ≥
|
||
within-arm, tokens +4.4%). It stays **opt-in**, not the default, because the Workflow tool
|
||
raises the consumer floor to **Claude Code 2.1.154+** (outward-facing; the prose path
|
||
keeps the lower floor).
|
||
|
||
Invoke the port via the **Workflow** tool with the Phase 1–4 output pinned into `args`:
|
||
|
||
```
|
||
Workflow({
|
||
scriptPath: "${CLAUDE_PLUGIN_ROOT}/scripts/trekreview-armB.workflow.mjs",
|
||
args: {
|
||
briefPath: "{brief_path}",
|
||
diffPath: "{path to the unified diff file from Phase 3}",
|
||
triage: "{triage map as 'path → treatment' lines from Phase 4}",
|
||
quick: {true if mode == quick, else false}
|
||
}
|
||
})
|
||
```
|
||
|
||
Contract (verified in S10 part B — follow exactly):
|
||
- **Pass `args` as a JSON object.** The script defensively re-parses a JSON *string*, but
|
||
the object form is the contract.
|
||
- **Reviewers are `StructuredOutput`-schema-forced** — `rule_key` is enum-enforced at the
|
||
tool layer (stronger than the prose path's post-hoc NW1 check), so there is no
|
||
`JSON.parse`/re-ask dance.
|
||
- **Recover the result from the `RESULT_JSON:{…}` line** inside the workflow output logs.
|
||
The script returns `{verdict, findings, ...}` AND logs it as that line; the
|
||
notification's `<result>` may be truncated, so parse the logged line.
|
||
- The reviewer/coordinator agentTypes are namespaced inside the script
|
||
(`voyage:brief-conformance-reviewer`, `voyage:code-correctness-reviewer`,
|
||
`voyage:review-coordinator`).
|
||
|
||
Then continue at **Phase 7** exactly as the prose path does — Phase 7 rendering, Phase 8
|
||
validation, and the operator gate are **shared** and substrate-independent (both paths
|
||
return the same `{verdict, findings}` shape).
|
||
|
||
**Known limitation (per bake-off §Posture, surfaced not hidden).** Classifier interference
|
||
was measured **0** at 9-agent concurrency in the session's default permission mode; an
|
||
explicit `auto`/`bypass`-mode re-run was not performed (the permission mode is operator-set,
|
||
not settable from within a session). trekreview's small fan-out showed 0 interference in S8
|
||
and S10. The large fan-out case (the trekplan swarm) is out of NW2 scope.
|
||
|
||
## Phase 7 — Write review.md
|
||
|
||
Write the coordinator's output verbatim to:
|
||
|
||
```
|
||
{project_dir}/review.md
|
||
```
|
||
|
||
Create parent directories if they do not exist. Atomic write pattern:
|
||
write to a temp file, then rename. The frontmatter `findings:` field
|
||
must use **block-style YAML** (one ID per line, ` - ` prefix). The
|
||
parser at `lib/util/frontmatter.mjs` does not support flow-style arrays.
|
||
|
||
If `mode == dry-run`: skip the write; print the would-be path and the
|
||
first 60 lines of the rendered output.
|
||
|
||
## Phase 8 — Validate output + stats
|
||
|
||
Run the strict validator:
|
||
|
||
```bash
|
||
node ${CLAUDE_PLUGIN_ROOT}/lib/validators/review-validator.mjs --json "{review_path}"
|
||
```
|
||
|
||
If validation fails:
|
||
- For repairable errors (missing required body section, malformed
|
||
finding-ID, REVIEW_VERSION_FORMAT warning): repair in place — re-emit
|
||
the missing section, recompute the finding-ID, fix the version
|
||
string. Re-validate.
|
||
- For unrepairable errors (REVIEW_WRONG_TYPE, malformed frontmatter):
|
||
stop and ask the user to re-run; do not silently produce an invalid
|
||
review.md.
|
||
|
||
Append a stats line to `${CLAUDE_PLUGIN_DATA}/trekreview-stats.jsonl`
|
||
(create the file if it does not exist):
|
||
|
||
```json
|
||
{"ts":"{ISO-8601}","slug":"{slug}","verdict":"BLOCK|WARN|ALLOW","counts":{"BLOCKER":N,"MAJOR":N,"MINOR":N,"SUGGESTION":N},"reviewed_files_count":N,"mode":"default|quick|validate|dry-run","duration_ms":N}
|
||
```
|
||
|
||
If `${CLAUDE_PLUGIN_DATA}` is unset or not writable, skip stats silently.
|
||
Never let stats failures block the main workflow.
|
||
|
||
**Build the operator-annotation HTML.** After stats land, run:
|
||
|
||
```bash
|
||
ANNOT_HTML=$(node ${CLAUDE_PLUGIN_ROOT}/scripts/annotate.mjs "{review_path}" 2>&1)
|
||
```
|
||
|
||
`stdout` is the absolute path to the `.html` on success. The HTML renders
|
||
`review.md` with line numbers, lets the operator click any line to attach
|
||
their own note (not Claude-generated suggestions — the operator drives
|
||
every annotation), keeps a sidebar of all notes, persists state in
|
||
localStorage, and exposes a "Copy Prompt" button. If `annotate.mjs`
|
||
exits non-zero, surface a one-line warning and continue — the annotation
|
||
HTML is a convenience, not a gate.
|
||
|
||
## Phase 8.5 — Validate-only mode (`--validate`)
|
||
|
||
When `mode == validate`:
|
||
1. Skip Phases 3–7 entirely.
|
||
2. Run the strict validator on `{project_dir}/review.md`.
|
||
3. Print a one-line PASS/FAIL summary plus the JSON output on FAIL.
|
||
4. Exit 0 on PASS, 1 on FAIL. Never write to disk. Never call any agent.
|
||
|
||
## Phase 9 — Present summary
|
||
|
||
After the write succeeds, print:
|
||
|
||
```
|
||
## Ultrareview Complete
|
||
|
||
**Task:** {task}
|
||
**Mode:** {default | quick | dry-run}
|
||
**Brief:** {brief_path}
|
||
**Project:** {project_dir}
|
||
**Review:** {review_path}
|
||
**Annotation HTML:** file://{$ANNOT_HTML}
|
||
**Scope:** {before_sha}..{after_sha} ({reviewed_files_count} files)
|
||
**Verdict:** {BLOCK | WARN | ALLOW}
|
||
|
||
### Counts
|
||
- BLOCKER: {N}
|
||
- MAJOR: {N}
|
||
- MINOR: {N}
|
||
- SUGGESTION: {N}
|
||
|
||
### Top findings
|
||
- [{severity}] {title} ({file}:{line})
|
||
...
|
||
{up to 5 highest-severity findings}
|
||
|
||
────────────────────────────────────────────────────────────────────
|
||
To review and annotate the review, open it in a browser:
|
||
|
||
open file://{$ANNOT_HTML}
|
||
|
||
Click any line to add YOUR OWN note. The sidebar collects every note,
|
||
the "Copy Prompt" button gathers them into one structured prompt.
|
||
Paste that prompt back into this chat and Claude revises review.md
|
||
from your notes. Annotations persist in your browser if you close
|
||
the tab and reopen the same file.
|
||
────────────────────────────────────────────────────────────────────
|
||
|
||
You can also:
|
||
- Feed BLOCKER + MAJOR findings into a follow-up plan:
|
||
/trekplan --brief {review_path}
|
||
- Re-run with `--quick` for a faster correctness-only pass
|
||
- Re-run with `--since <ref>` to narrow scope
|
||
```
|
||
|
||
Per **Handover 6**, BLOCKER and MAJOR findings are consumed by
|
||
`/trekplan --brief review.md` to produce a remediation plan. The
|
||
review's frontmatter `findings:` list and the trailing JSON block are
|
||
the contract for that handover (see `docs/HANDOVER-CONTRACTS.md`).
|
||
|
||
## Profile (v4.1)
|
||
|
||
Accepts `--profile <name>` where `<name>` is `economy`, `balanced`, `premium`, `fable`,
|
||
or a custom profile under `voyage-profiles/`. Default: `premium`.
|
||
|
||
Resolution order (per `lib/profiles/resolver.mjs`):
|
||
1. `--profile` flag (source: `flag`)
|
||
2. `VOYAGE_PROFILE` env-var (source: `env`)
|
||
3. `premium` default (source: `default`)
|
||
|
||
The selected profile drives `phase_models.review` — `economy` uses sonnet
|
||
for the brief-conformance + code-correctness reviewers; `balanced` and
|
||
`premium` use opus (review benefits from deeper reasoning).
|
||
|
||
Examples:
|
||
```
|
||
/trekreview --profile balanced --project .claude/projects/2026-05-09-add-auth
|
||
VOYAGE_PROFILE=premium /trekreview --project ...
|
||
```
|
||
|
||
Stats records emit `profile` and `profile_source`.
|
||
|
||
## Composition rule (v5.1)
|
||
|
||
Independent of the profile system. When `brief.md` carries
|
||
`phase_signals` (brief_version ≥ 2.1), each downstream phase resolves
|
||
effort + model as:
|
||
|
||
```
|
||
effort_for_phase = brief.phase_signals[<phase>]?.effort ?? 'standard'
|
||
model_for_phase = brief.phase_signals[<phase>]?.model ?? profile.phase_models[<phase>]
|
||
```
|
||
|
||
The brief signal wins per-phase when present; the profile fills any
|
||
gaps. Both fields are mechanically resolved by the single composed CLI
|
||
`node ${CLAUDE_PLUGIN_ROOT}/lib/profiles/resolver.mjs --resolve-phase-model`
|
||
invoked in Phase 2; the resolved JSON `{effort, model, source}` is captured
|
||
as `phase_signal_result` and passed to `Agent` tool calls explicitly. The
|
||
resolver controls the `model` parameter at Agent-spawn sites only — the
|
||
orchestrator's own model is fixed at invocation time (command frontmatter
|
||
omits `model:`, so it follows the session model) and cannot be switched
|
||
mid-turn. Sub-agents fall back to `model:` in their own `agents/*.md`
|
||
frontmatter when no spawn-site injection happens.
|
||
|
||
For `/trekreview` specifically: `effort == 'low'` activates the existing
|
||
`--quick`-equivalent code-path (skip the brief-conformance reviewer; run
|
||
correctness-only). `effort == 'standard'` (or absent) → no change.
|
||
`effort == 'high'` activates the high-effort behavior documented under
|
||
`### High-effort behavior (v5.1.1)` below.
|
||
|
||
### Sequencing gate surface
|
||
|
||
Phase 1 already calls `brief-validator.mjs --soft` against `{brief_path}`.
|
||
If the validator returns `BRIEF_V51_MISSING_SIGNALS` in `errors`
|
||
(brief_version ≥ 2.1 without `phase_signals` or `phase_signals_partial:
|
||
true`), halt with: `Brief is brief_version 2.1 but does not carry
|
||
phase_signals — re-run /trekbrief to commit them (Phase 3.5).`
|
||
Enforcement is validator-only; this surface just makes the friendly hint
|
||
readable.
|
||
|
||
### High-effort behavior (v5.1.1)
|
||
|
||
When `phase_signal_result.effort == 'high'` for the `review` phase,
|
||
skip Pass 3 (Cloudflare reasonableness filter) in
|
||
`agents/review-coordinator.md`. Passes 1, 2, and 4 still run.
|
||
Rationale: high-effort review trusts the operator to weigh borderline
|
||
findings rather than have the coordinator drop them. To prevent
|
||
unknown `rule_key` values from polluting downstream remediation plans
|
||
(Handover 6), the coordinator applies its v5.1.1 high-effort
|
||
normalization rule — substituting unknown `rule_key` values with the
|
||
literal string `PLAN_EXECUTE_DRIFT` (the most general drift category
|
||
in the 12-entry catalogue) and preserving the original in
|
||
`original_rule_key`. See `agents/review-coordinator.md` § Pass 3
|
||
"High-effort normalization (v5.1.1)" for the full normalization spec.
|
||
|
||
Standard effort (or absent): run all 4 passes as usual.
|
||
Low effort: skip the brief-conformance reviewer entirely (existing
|
||
`--quick`-equivalent code-path).
|
||
|
||
## Hard rules
|
||
|
||
- **Brief is the contract.** Every finding in the review traces to a
|
||
brief section via `brief_ref`, except `SCOPE_CREEP_BUILT` (which
|
||
traces to "no anchor"). Conformance is the conformance reviewer's
|
||
job — code-correctness findings carry generic anchors like
|
||
`"NFR — code correctness"`.
|
||
- **Independent reviewers.** Do NOT cross-feed findings between
|
||
brief-conformance-reviewer and code-correctness-reviewer. The
|
||
coordinator is the only place where outputs combine.
|
||
- **Bounded coordination.** Synthesis-level inference across files is
|
||
forbidden in v1.0. The coordinator dedups, filters, and computes the
|
||
verdict — nothing more.
|
||
- **Triage map respected.** Files marked `skip` MUST appear in the
|
||
Coverage section. Silent drops are `COVERAGE_SILENT_SKIP` (MAJOR).
|
||
- **Block-style YAML for findings list.** The frontmatter parser does
|
||
not support flow-style arrays. `findings: [a, b]` is broken; use
|
||
`findings:\n - a\n - b`.
|
||
- **Refuse-with-suggestion above 100 files / 100K tokens.** Never run
|
||
blind on a giant diff. Use AskUserQuestion to surface the gate.
|
||
- **Cost.** Model resolution at Agent-spawn sites is a three-layer fallback:
|
||
brief `phase_signals[<phase>].model` > `profile.phase_models[<phase>]` >
|
||
agent frontmatter `model:`. The composed resolver returns the first two
|
||
layers as `phase_signal_result.model`; spawn sites inject it, and agent
|
||
frontmatter is the fallback when no injection happens.
|
||
- **Privacy.** Never log secrets, tokens, or credentials in review.md.
|
||
Findings citing files with secret-like content must redact the secret
|
||
in the `detail` field.
|
||
- **Honesty.** If the diff is trivially small or all-skip, say so. Do
|
||
not pad findings to make the review look thorough.
|
||
- **No production code.** This command never runs production code, never
|
||
writes to anything outside `{project_dir}` and `${CLAUDE_PLUGIN_DATA}`.
|