docs(review): /trekreview chooses its security engine (E 3.2)
Phase 4.7 of /trekreview runs security-engine.mjs choose first. Claude
Security runs through its own skill (scan changes --base {before_sha}
--effort high); its cost question goes to the operator, and "No", a
question nobody can answer or a missing Workflow tool records it as not
run, never a fallback to /security-review. /security-review runs only
when choose picked it. Its report is converted by security-engine.mjs
convert, never transcribed; below_threshold is declined as
engine:below-threshold. With no engine the expected reviewer
security-engine never reports, so the verdict gate exits 2 and the
summary says "security: not measured" and why, never 0. --quick keeps no
security engine. The Engine summary line, the template's ran line, the
hard rules (the engine's report folder, scan changes only) and the
allowed-tools (Workflow) follow.
Engine prose follows in commands/trekexecute.md, docs/command-modes.md,
docs/architecture.md, CLAUDE.md, README.md, SECURITY.md and Handover 6:
Claude Security recommended, proprietary so called by name and never a
manifest dependency; without it /security-review; with neither "not
measured".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
cb7c13b408
commit
d97e9b5b19
10 changed files with 279 additions and 49 deletions
|
|
@ -2249,13 +2249,13 @@ never typed by the operator, in this order (Cherny's):
|
|||
`simplify = "not-run"`, with the reason. Not a stop: `/simplify` is a
|
||||
cleanup pass, not a gate. Continue.
|
||||
3. **`/trekreview`** — Voyage's review gate: Claude Code's `/code-review` and
|
||||
`/security-review` as the engine, then Voyage's control against the brief,
|
||||
which is what stops. Start it through the Skill tool, with the run's range:
|
||||
one security engine (Claude Security, else `/security-review`; with neither,
|
||||
security is "not measured") as the engine, then Voyage's control against the brief, which is what stops. Start it through the Skill tool, with the run's range:
|
||||
`Skill("voyage:trekreview", args: "--project {project_dir} --since {session_start_sha}")`.
|
||||
A plugin command is a skill the model may start unless it carries
|
||||
`disable-model-invocation`, which `/trekreview` does not (the Claude Code
|
||||
skills documentation, read 2026-10-08). Follow its phases to the end in
|
||||
this session; its own questions (project-memory lines, one by one) are its
|
||||
this session; its own questions (Claude Security's question about the scan's cost, project-memory lines one by one) are its
|
||||
own. **Without `--project`** there is no brief to review against:
|
||||
`review = {round: 1, path: null, gate_exit: 2, verdict: null}`, "review not
|
||||
run — no project directory", and the merge stays closed (Phase 8).
|
||||
|
|
@ -3158,7 +3158,7 @@ pins and quotes in the local practice analysis.
|
|||
- **Claude Code** itself: subagents for the helper and the controller; the
|
||||
`PreToolUse` guard behind the test lock (K8); `/simplify`, `/code-review` and
|
||||
`/security-review` as the review's engine, started through the Skill tool
|
||||
(P11).
|
||||
(P11); Claude Security, Anthropic's plugin, as the security engine where it is installed (E).
|
||||
- **The review at the end (P11, wave 2)** — Cherny: the order — run the
|
||||
thing, simplify, then review.
|
||||
Kiro (`feature-pipeline`, documentation only): the gate — a verdict in a file.
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ description: |
|
|||
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] [--answers]"
|
||||
allowed-tools: Agent, Read, Glob, Grep, Write, Edit, Bash, AskUserQuestion, Skill
|
||||
allowed-tools: Agent, Read, Glob, Grep, Write, Edit, Bash, AskUserQuestion, Skill, Workflow
|
||||
---
|
||||
|
||||
# Ultrareview Local v1.0
|
||||
|
|
@ -30,9 +30,11 @@ and the coordinator applies BOUNDED operations only. Synthesis-level
|
|||
inference across files is forbidden in v1.0 (Judge Agent pattern).
|
||||
|
||||
**The review is a gate (review_version 1.2, ordre 5).** In this order:
|
||||
Claude Code's own `/code-review` and `/security-review` are the engine
|
||||
(Phase 4.7); Voyage's control against the brief runs after them and is the
|
||||
part that stops (Phase 5–6); the verdict is written in `review.md` and one
|
||||
Claude Code's own `/code-review` and one security engine (Claude Security
|
||||
where it is installed, else Claude Code's `/security-review`; with neither,
|
||||
security is "not measured", never 0) are the engine (Phase 4.7); Voyage's
|
||||
control against the brief runs after them and is the part that stops
|
||||
(Phase 5–6); the verdict is written in `review.md` and one
|
||||
command decides on it (`lib/review/verdict-gate.mjs`, Phase 8: 0 = ALLOW, or
|
||||
WARN with every counting MAJOR answered, 1 = BLOCK, 2 = could not decide,
|
||||
never green). No finding is struck for
|
||||
|
|
@ -295,12 +297,12 @@ prompt in Phase 5, and its summary line goes into the Coverage section of
|
|||
built by `formatCriteriaEvidence` precisely so the orchestrator cannot narrate a
|
||||
pass that never happened.
|
||||
|
||||
## Phase 4.7 — Claude Code's review engine (`/code-review`, `/security-review`)
|
||||
## Phase 4.7 — The engine: Claude Code's `/code-review` and the security engine
|
||||
|
||||
**Skipped in `dry-run` and `validate`.** In `quick` mode only `/code-review`
|
||||
runs. Wrap, not a new engine: Claude Code's built-in reviews find the
|
||||
correctness and security defects; Voyage's brief control (Phase 5) runs after
|
||||
them and is what stops.
|
||||
runs: no security engine. Wrap, not a new engine: `/code-review` and the
|
||||
security engine find the correctness and security defects; Voyage's brief
|
||||
control (Phase 5) runs after them and is what stops.
|
||||
|
||||
Verified in Claude Code's live documentation (2026-10-06):
|
||||
- Claude may start `/code-review` itself (`code-review.md`: "Claude can start
|
||||
|
|
@ -314,21 +316,55 @@ Verified in Claude Code's live documentation (2026-10-06):
|
|||
default branch" and "Needs an `origin` remote". The docs do not state its
|
||||
output format or whether it runs in the background.
|
||||
|
||||
Run both through the **Skill tool**, one after the other:
|
||||
**Choose the security engine first** (not in `quick`; E 3.1, decisions 175,
|
||||
196). Claude Security is `available` when the `claude-security:claude-security`
|
||||
skill is in this session's skill list AND the Workflow tool is among the tools
|
||||
you can call right now (look at your tools, do not assume); otherwise it is
|
||||
`missing`.
|
||||
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/security-engine.mjs choose --repo . --base {before_sha} --head {after_sha} --claude-security {available|missing}
|
||||
```
|
||||
|
||||
It prints `{engine, reason}`: `claude-security` when Claude Security is
|
||||
available and `{before_sha}..{after_sha}` has committed changes; else
|
||||
`security-review` when origin's HEAD resolves and `origin/HEAD...HEAD` is
|
||||
non-empty; else `null`, with `no-engine` (no `origin` remote and no Claude
|
||||
Security), `no-origin` (an `origin` remote whose HEAD does not resolve) or
|
||||
`empty-diff` (the engine's range holds no committed change). Exit 2 = it could
|
||||
not run: print the reason in one line and record the security engine as not
|
||||
run. Keep `engine` and `reason` for Phases 5 and 9.
|
||||
|
||||
Then run, through the **Skill tool**, one after the other:
|
||||
|
||||
1. `Skill("code-review", args: "high {before_sha}...{after_sha}")` — the
|
||||
review's own SHA range from Phase 3. `high` is set explicitly; note in the
|
||||
Executive Summary that this also becomes the level later `/code-review`
|
||||
runs in this session reuse.
|
||||
2. `Skill("security-review")` — no arguments; its base is fixed.
|
||||
2. The security engine `choose` picked, and only that one:
|
||||
- `claude-security` —
|
||||
`Skill("claude-security:claude-security", args: "scan changes --base {before_sha} --effort high")`.
|
||||
Claude Security asks one question of its own before the scan starts (the
|
||||
scan's time and token cost): put it to the operator as it stands and never
|
||||
answer it yourself. "No", a question nobody can answer (no operator
|
||||
present), the Workflow tool missing after all, or a scan that ends without
|
||||
a report: record Claude Security as not run, with the reason. **Never fall
|
||||
back to `/security-review`**: the review is then not measured for
|
||||
security.
|
||||
- `security-review` — `Skill("security-review")`, no arguments; its base is
|
||||
fixed (origin's default branch). `/security-review` runs only when
|
||||
`choose` picked it.
|
||||
- `null` — no security engine runs: record it as not run with `choose`'s
|
||||
reason.
|
||||
|
||||
**Never** pass `--fix`, `--comment`, `--post` or `ultra`: a review never edits
|
||||
code, never posts anywhere, and never uploads the repo to a cloud review.
|
||||
|
||||
**Wait for the findings.** A background `/code-review` delivers its findings
|
||||
later in this conversation. Do not start Phase 6 before both engines have
|
||||
delivered or have been recorded as not run. For each engine that delivered,
|
||||
transcribe every finding — file, line (0 when none), the one-sentence summary,
|
||||
later in this conversation, and Claude Security's scan runs as a workflow
|
||||
until it writes its report. Do not start Phase 6 before both engines have
|
||||
delivered or have been recorded as not run. For `/code-review` and
|
||||
`/security-review`, transcribe every finding — file, line (0 when none), the one-sentence summary,
|
||||
the failure scenario, the category if given (it decides the tier:
|
||||
`simplification`, `efficiency`, `reuse`, `style` and `readability` enter as MINOR), the
|
||||
fix the engine proposes (`fix`) only when its text states one — never write one
|
||||
|
|
@ -349,7 +385,7 @@ node ${CLAUDE_PLUGIN_ROOT}/lib/review/engine-findings.mjs \
|
|||
```
|
||||
|
||||
The converted payload is saved to `{project_dir}/.review-engine-payload-{engine}.json`
|
||||
(`code-review` or `security-review`), the same stdout, also written: Phase 7's finalize
|
||||
(`code-review`, `security-review` or `claude-security`), the same stdout, also written: Phase 7's finalize
|
||||
reads it from there. Never edit that file by hand.
|
||||
Exit 0 prints a reviewer payload (`claude-code-review` /
|
||||
`claude-security-review`) for Phase 6 plus `out_of_scope`: engine findings on
|
||||
|
|
@ -371,12 +407,40 @@ at least the engine's own (MAJOR for `/code-review`, BLOCKER for
|
|||
`/security-review`); the adapter refuses to let a transcription lower an
|
||||
engine finding.
|
||||
|
||||
**Claude Security's findings are converted, never transcribed.** When its
|
||||
scan has finished it has written `CLAUDE-SECURITY-<ts>/` in the repo root
|
||||
(behind its own `.gitignore`); convert that report, after the scope file
|
||||
above exists:
|
||||
|
||||
```bash
|
||||
node ${CLAUDE_PLUGIN_ROOT}/lib/review/security-engine.mjs convert --report-dir . \
|
||||
--base {before_sha} --head {after_sha} --scope "{project_dir}/.review-scope.txt" \
|
||||
> "{project_dir}/.review-engine-payload-claude-security.json" \
|
||||
&& cat "{project_dir}/.review-engine-payload-claude-security.json"
|
||||
```
|
||||
|
||||
Exit 0 prints the `claude-security` payload: `findings` and `out_of_scope` as
|
||||
for any engine, and `below_threshold`, the findings Claude Security marked
|
||||
below confidence `high` (decision 219). Pass `below_threshold` to the
|
||||
coordinator as well: each one is listed under `## Declined to Judge`
|
||||
(`engine:below-threshold`), never counted, and it forbids `ALLOW`. Exit 2 =
|
||||
not measured (no report stamp of a changes scan of exactly
|
||||
`{before_sha}..{after_sha}` — an empty diff writes none —, a scan not
|
||||
verified, researchers that did not all return, an empty diff or scope, a
|
||||
malformed record): print the reason and record Claude Security as not run;
|
||||
the empty file the redirect leaves is no payload. Exit 1 = the scope list is
|
||||
empty: rebuild it from Phase 3 and re-run.
|
||||
|
||||
**An engine that did not run** (Claude Code older than v2.1.246, the Skill
|
||||
call refused, no `origin` remote, an `ambiguous argument` error, or no findings
|
||||
ever delivered) is recorded as not run with its reason. It stays in the
|
||||
expected reviewer set, so it forbids `ALLOW` (fail-closed); the Executive
|
||||
Summary's first sentence names it. An engine that ran and found nothing
|
||||
delivers an empty payload — that is a report, not an absence.
|
||||
call refused, no `origin` remote, an `ambiguous argument` error, Claude
|
||||
Security's question answered "No" or left unanswered, a report `convert`
|
||||
reads as not measured, `choose` picking no engine, or no findings ever
|
||||
delivered) is recorded as not run with its reason. It stays in the expected
|
||||
reviewer set, so it forbids `ALLOW` (fail-closed) and the verdict gate exits
|
||||
2; the Executive Summary's first sentence names it. For security that
|
||||
sentence says "security: not measured" and why (`choose`'s reason, or why
|
||||
the engine did not run): never 0, never "no findings". An engine that ran and
|
||||
found nothing delivers an empty payload — that is a report, not an absence.
|
||||
|
||||
The two engine payloads are NOT shown to the Phase 5 reviewers: reviewers stay
|
||||
independent, and the coordinator is the only place the outputs meet.
|
||||
|
|
@ -451,7 +515,7 @@ the round input:
|
|||
Finalize (Phase 7) carries a previous finding until every reviewer that ran in
|
||||
its round marks it resolved, and an engine finding until its engine ran this
|
||||
round and did not raise it again. The engines (Phase 4.7) stay on the run range
|
||||
(`/security-review` takes no range).
|
||||
(`/security-review` takes no range; Claude Security scans from `{before_sha}`).
|
||||
|
||||
`brief-conformance-reviewer` additionally receives `sc_evidence_block` from
|
||||
Phase 4.5, pasted verbatim — the command, exit code and first line of output for
|
||||
|
|
@ -502,8 +566,12 @@ Summary will note the brief-conformance pass was skipped.
|
|||
|
||||
Write down the expected reviewer set BEFORE the spawn: both reviewers in
|
||||
default mode, `code-correctness-reviewer` alone in `quick` mode — plus, in
|
||||
every mode, the Phase 4.7 engines that were due (`claude-code-review`, and
|
||||
`claude-security-review` outside `quick`). After the spawn, account for each
|
||||
every mode, the Phase 4.7 engines that were due: `claude-code-review`, and
|
||||
outside `quick` the security engine `choose` picked (`claude-security` for
|
||||
Claude Security, `claude-security-review` for `/security-review`), or
|
||||
`security-engine` when it picked none or could not run. `security-engine` never reports, so the
|
||||
review carries `missing-reviewer:security-engine` and the verdict gate exits 2:
|
||||
security not measured, never 0. After the spawn, account for each
|
||||
one by name. An engine recorded as not run is passed on as missing, never
|
||||
left out of the set.
|
||||
|
||||
|
|
@ -535,7 +603,8 @@ 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 AND the Phase 4.7 engine payloads, plus the triage map, brief
|
||||
from Phase 5 AND the Phase 4.7 engine payloads (Claude Security's
|
||||
`below_threshold` list with them), plus the triage map, brief
|
||||
metadata, SHA range, the expected reviewer set (engines included), and each
|
||||
not-run engine with its reason. Name `voyage:reviewing-adversarially` in the
|
||||
coordinator's prompt as in Phase 5, and pass each reviewer's
|
||||
|
|
@ -894,7 +963,7 @@ After the write succeeds, print:
|
|||
**Scope:** {before_sha}..{after_sha} ({reviewed_files_count} files)
|
||||
**Verdict:** {BLOCK | WARN | ALLOW}
|
||||
**Gate:** verdict-gate exit {gate_exit} ({0 = ALLOW, or WARN with every counting MAJOR answered | 1 = BLOCK | 2 = could not decide, a control did not run, or a MAJOR without an answer or answered "fixed" and not yet confirmed — not green})
|
||||
**Engine:** /code-review {delivered N | not run: reason} · /security-review {delivered N | not run: reason}
|
||||
**Engine:** /code-review {delivered N | not run: reason} · security {Claude Security delivered N, M below threshold | /security-review delivered N | not measured: reason | none in quick mode}
|
||||
|
||||
### Counts
|
||||
- BLOCKER: {N}
|
||||
|
|
@ -966,9 +1035,12 @@ the contract for that handover (see `docs/HANDOVER-CONTRACTS.md`).
|
|||
writes to anything outside `{project_dir}` —
|
||||
with one exception, only after the operator approved that exact line in
|
||||
Phase 8.7: Voyage's own marked block in `.claude/rules/voyage-review-lessons.md`
|
||||
and a one-line pointer in `CLAUDE.md`.
|
||||
- **The engine never edits code.** `/code-review` and `/security-review` run
|
||||
without `--fix`, `--comment`, `--post` or `ultra`. A review that changes
|
||||
and a one-line pointer in `CLAUDE.md`. Claude Security writes its own report
|
||||
folder `CLAUDE-SECURITY-<ts>/` in the repo root, behind its own `.gitignore`;
|
||||
`/trekreview` only reads it.
|
||||
- **The engine never edits code.** `/code-review`, `/security-review` and Claude Security run
|
||||
without `--fix`, `--comment`, `--post` or `ultra`; Claude Security runs
|
||||
`scan changes` only, never its patch job. A review that changes
|
||||
code is not a review.
|
||||
- **No finding is struck for length.** Only MINOR/SUGGESTION text is trimmed,
|
||||
and every trim is counted; what the coordinator set aside is listed under
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue