feat(review): the brief's threat model on the review side (decision 164, 167, 218)

The code-correctness reviewer's dimension 6 shrinks to what the engines do
not read: command, skill and agent markdown carrying shell, with Claude
Code's threshold (a concrete attack path, high confidence), the keys
SECURITY_INJECTION or SECURITY_RISK, and threat_model_ref quoting the
brief's Outside: line; the code-level bullets go to the engines.

The coordinator puts a Voyage security finding with a threat_model_ref
into the trailing JSON block's known_outside, never into findings.
review-finalize gains the pass `outside`, right after engine: it only
raises. An entry the brief at the review's brief_path does not back now
(line gone, threat model not usable, an engine finding, not a security
finding) goes back into the findings at its catalogue tier with a Source
line, R-step and raised verdict, as the engine put-back does; the rest are
listed under the new `## Outside the Threat Model` section.

verdict-gate decideOpenFindings (and so scripts/release-check.mjs)
re-checks every entry against brief_path through the new
notBackedOutside and exits 2 for one not backed. review-validator
shape-checks the optional known_outside (REVIEW_KNOWN_OUTSIDE_MALFORMED).
threat-model.mjs gains reviewThreatModel and isKnownEntry for the three.

/trekreview Phase 8.8 lists the known issues outside the threat model
(not answered, not counted); the review template shows the section and
field; Handover 1 names the optional ## Threat Model and its code,
Handover 6 names known_outside and its code.

Part 2, step 2.5 of the E plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-11-08 09:14:47 +01:00
commit c47061191c
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
10 changed files with 585 additions and 30 deletions

View file

@ -34,7 +34,7 @@ You will receive a prompt containing:
pass — declared signatures, exports, top-level wiring — but no deep
semantic analysis.
- **Brief path** (optional) — `{project_dir}/brief.md`. Read for `brief_ref`
context only. The brief is not your contract — it is the conformance
context and for its `## Threat Model`, when it has one (dimension 6), only. The brief is not your contract — it is the conformance
reviewer's contract. You evaluate code correctness regardless of
what the brief promised.
- **Rule catalogue** — the 18-key catalogue in
@ -118,21 +118,30 @@ literals or example fenced blocks):
Cite the exact line. Never accept "not implemented" placeholders as fine
"for now" — they are MAJOR findings until removed.
### 6. Security surface — `SECURITY_INJECTION` (BLOCKER)
### 6. Security surface (command texts) — `SECURITY_INJECTION` / `SECURITY_RISK` (BLOCKER)
- Untrusted input is interpolated into a shell command (`exec`, `spawn`
with `shell: true`, template-literal command construction).
- Untrusted input is interpolated into a SQL query, an HTML template,
or a regex without escaping.
- File paths are constructed from untrusted input without
`path.normalize` + a base-dir containment check (path traversal).
- A new HTTP endpoint accepts user input and renders it back without
output encoding (XSS).
- Process env vars containing secrets are echoed in logs.
The security engine reads the code; this dimension covers only what the
engines do not read (decision 167): the command, skill and agent markdown in
the diff that carries shell (a bash block or a command line in
`commands/*.md`, `skills/**/*.md`, `agents/*.md`) and that a session runs as
written.
Cite the line and explain the injection vector. Never assume something
is safe because "the input is internal" — that's how supply-chain
attacks become RCE.
- An untrusted value (an argument, a file name, a field a reviewer or an
engine wrote, fetched text) is spliced into a shell line unquoted, or into
a heredoc the shell expands.
- A path is built from such a value with no containment check.
- A secret is echoed into a log, a transcript or a committed file.
Raise one only with a concrete attack path (who sends what, through which
step, to what effect) and high confidence, Claude Code's own threshold for a
security finding; quote the line. Use `SECURITY_INJECTION`, or `SECURITY_RISK`
when no narrower key fits; the engines' key is never yours. Never assume
something is safe because "the input is internal". When the brief has a
`## Threat Model` and the attack path is one an `Outside:` line of it leaves
out, put that line's text (after `Outside: `) in `threat_model_ref`, exactly
as the brief writes it: the coordinator then lists the finding as a known
issue outside the threat model instead of counting it, while the brief's
intent approval is current. Otherwise leave `threat_model_ref` out.
### 7. Hidden dependencies — `UNDECLARED_DEPENDENCY` (MAJOR)
@ -176,6 +185,7 @@ Severity is fixed by `rule_key`. Do NOT override the catalogue:
| `MISSING_TEST` | MAJOR |
| `PLACEHOLDER_IN_CODE` | MAJOR |
| `SECURITY_INJECTION` | BLOCKER |
| `SECURITY_RISK` | BLOCKER |
| `UNDECLARED_DEPENDENCY` | MAJOR |
| `COVERAGE_SILENT_SKIP` | MAJOR |
| `CLAUDE_MD_DEVIATION` | MINOR |
@ -206,7 +216,7 @@ parsers find it by reading the last `json` code fence.
| Cross-file regressions | PLAN_EXECUTE_DRIFT | {N} |
| Test coverage gaps | MISSING_TEST | {N} |
| Placeholder code | PLACEHOLDER_IN_CODE | {N} |
| Security surface | SECURITY_INJECTION | {N} |
| Security surface (command texts) | SECURITY_INJECTION / SECURITY_RISK | {N} |
| Hidden dependencies | UNDECLARED_DEPENDENCY | {N} |
| Instruction files | CLAUDE_MD_DEVIATION / CLAUDE_MD_STALE | {N} |
@ -272,6 +282,8 @@ parsers find it by reading the last `json` code fence.
- `rule_key` MUST be in the catalogue. A finding with a rule key outside
it is not dropped: it is routed to `unverified` by the coordinator's
reasonableness filter (Pass 3).
- `threat_model_ref` (optional, dimension 6 only): the text of the brief's
`Outside:` line the finding falls under, exactly; omit it otherwise.
## Rules

View file

@ -229,9 +229,22 @@ vocabulary on purpose.
is reviewer-asserted and was never substantiated; counting it would let an
unexamined finding *raise* the verdict, which is invention.
**Known issues outside the threat model** (E, decisions 164, 218). A finding
from a Voyage reviewer (no `engine`) under a security key (`SECURITY_INJECTION`,
`SECURITY_RISK`) that carries `threat_model_ref` goes into the JSON block's
`known_outside`, as the reviewer gave it with its `threat_model_ref`, never into
`findings`: no `@@ID_<n>@@`, no `### ` entry, not counted, no effect on the
verdict. You do not judge whether the brief backs it:
`lib/review/review-finalize.mjs` checks each entry against the brief's
`## Threat Model`, puts back into the findings any the brief does not back and
lists the rest under `## Outside the Threat Model`, and the verdict gate checks
again. An engine finding never goes there, with or without the field: it
blocks as before.
### Pass 4 — Compute verdict
Count findings by severity AFTER dedup and filtering. Verdict thresholds:
Count findings by severity AFTER dedup and filtering (a `known_outside` entry
is no finding and is not counted). Verdict thresholds:
| Counts | Verdict |
|--------|---------|
@ -364,6 +377,7 @@ The LAST fenced block in the file is a `json` block:
"counts": { "BLOCKER": N, "MAJOR": N, "MINOR": N, "SUGGESTION": N },
"allow_blocked_by": ["unverified:actionability:empty (1)", "missing-reviewer:claude-security-review", "unattributable-payload (1)"],
"declined_to_judge": [{ "file": "lib/x.mjs", "line": 3, "rule_key": "MISSING_TEST", "reason": "actionability:empty" }],
"known_outside": [{ "severity": "BLOCKER", "rule_key": "SECURITY_RISK", "file": "commands/x.md", "line": 12, "brief_ref": "Threat Model", "title": "...", "detail": "...", "recommended_action": "...", "threat_model_ref": "<the text of the brief's Outside: line>" }],
"trimmed": 0,
"findings": [
{