voyage/tests/lib/review-threat-model.test.mjs
Kjell Tore Guttormsen 1be961aab1
fix(gate): part 2 verifier findings — non-object findings, carried backed copy, rule_keys, marks, approval prose, brief_path, engine put-back once
1. plan-review-dedup skips only a null or undefined finding; any other
   entry that is not an object (a string, a number, an array) goes on as
   before E, a finding with no valid severity that the plan gate counts as
   a blocker (it was dropped, so a string blocker recorded PASS).
2. plan-gate --record: a previous finding that rule A carried as counted and
   whose id is backed this round leaves the counted set, the counts, the
   verdict and the stop rule's summary (it was counted and listed in
   known_outside at once: BLOCK, not-fewer, and --accept-open refused).
3. A known_outside entry keeps rule_keys; extractVerdict accepts it (a list
   of strings), and the bare gate re-checks with the same keys the recorder
   used, so a merged finding whose security key is only in rule_keys is a
   WARN, not a STOP.
4. plan-critic, scope-guardian and /trekplan Phase 9's round-2 sentences say
   the Critic Verdict also lists known_outside and each is marked by id in
   previous. Code already accepts such a mark (dedup passes it; countRound
   ignores ids it does not carry; --record removes the entry on true);
   pinned in diff mode and full count.
5. The approval prose names ## Threat Model where it named only ## Intent +
   ## Goal: skills/annotating/SKILL.md (description, step 4, guard list),
   docs/command-modes.md, Handover 1's intent_approved_hash row, README
   (approval, Approve row, intent approval gate) and CLAUDE.md's /trekbrief
   row.
6. threat-model.mjs gains usableInRepo (a brief path relative to the repo
   top level) and repoRelativeBrief; --record stores brief_path relative to
   the top level, never absolute, and both gates read it from there, so the
   gate run from a subdirectory reads the same brief. reviewThreatModel
   uses usableInRepo. Handover 4 and the gate header say so.
7. review-finalize's outside pass puts an entry back only when no finding
   carries its row yet (engine-compare rowOf): an engine finding listed in
   known_outside while its payload is saved went back twice.
8. A test runs scripts/release-check.mjs on a review whose known_outside
   entry the brief does not back: exit 2 (control: backed passes the review
   check).

Existing tests changed: plan-gate-threat-model's entry-keys assertion now
expects rule_keys (item 3), and its planted non-security entry drops the
security key from rule_keys too, since the gate reads rule_keys (item 3).

Part 2 fix round of the E plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-11-08 10:50:20 +01:00

394 lines
18 KiB
JavaScript

// tests/lib/review-threat-model.test.mjs
// Part 2, step 2.5 of the E plan (decisions 164, 167, 218): the brief's threat
// model on the review side.
//
// - The coordinator puts a Voyage security finding with a `threat_model_ref`
// into the trailing JSON block's `known_outside`, never into `findings`.
// - lib/review/review-finalize.mjs only raises: an entry the brief does not
// back now (its Outside: line gone, the approval stale, an engine finding,
// not a security finding) goes back into `findings` at its catalogue tier and
// raises the verdict, as the engine put-back does; the entries that stay are
// listed under `## Outside the Threat Model`.
// - lib/review/verdict-gate.mjs `decideOpenFindings` re-checks every entry
// against the review's `brief_path` and exits 2 for one not backed.
// - lib/validators/review-validator.mjs shape-checks the optional field.
// - Prose: the code reviewer's dimension 6, the coordinator, /trekreview Phase
// 8.8, the review template, Handover 1 and 6.
// Tests run the real CLIs (finalize, validator, gate) in throwaway git repos
// with no hooks, a stamped brief and its logged approval.
import { after, test } from 'node:test';
import { strict as assert } from 'node:assert';
import { spawnSync } from 'node:child_process';
import { appendFileSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';
import { validateReviewContent } from '../../lib/validators/review-validator.mjs';
import { stampIntentApproval } from '../../lib/validators/intent-approval.mjs';
import { appendApproval, normalizeHex, normalizeSubject, resolveRepo } from '../../lib/util/approval-log.mjs';
import { childEnv } from '../helpers/child-env.mjs';
import { releaseCheck } from '../../scripts/release-check.mjs';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const FINALIZE = join(ROOT, 'lib', 'review', 'review-finalize.mjs');
const VALIDATOR = join(ROOT, 'lib', 'validators', 'review-validator.mjs');
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
const PROJECT_REL = '.claude/projects/2026-11-08-tm';
const read = (rel) => readFileSync(join(ROOT, rel), 'utf8');
const flat = (s) => s.replace(/\s+/g, ' ');
const OUTSIDE = "a local user who can already edit the repo's hooks";
const INSIDE = 'a crafted file name in a pull request reaches the shell';
const BRIEF = `---
type: trekbrief
brief_version: "2.2"
created: 2026-11-08
task: "Add an upload hook"
slug: tm
project_dir: ${PROJECT_REL}/
research_topics: 0
research_status: complete
framing: new-direction
---
# Task
## TL;DR
An upload hook.
## Intent
Uploads arrive from strangers.
## Goal
A hook that checks each upload.
## Threat Model
- Inside: ${INSIDE}
- Outside: ${OUTSIDE}
## Success Criteria
- It checks.
`;
const made = [];
after(() => { for (const b of made) rmSync(b, { recursive: true, force: true }); });
function g(cwd, ...args) {
const r = spawnSync('git', args, { cwd, encoding: 'utf8', env: childEnv() });
if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`);
return r.stdout.trim();
}
const run = (args) => spawnSync(process.execPath, args, { encoding: 'utf8', env: childEnv() });
/** A throwaway repo with a start and an end commit, a stamped brief and its logged intent approval. */
function tmRepo() {
const base = mkdtempSync(join(tmpdir(), 'review-tm-'));
made.push(base);
const repo = join(base, 'repo');
mkdirSync(repo, { recursive: true });
mkdirSync(join(base, 'hooks-none'));
g(repo, 'init', '-q', '-b', 'main');
appendFileSync(join(repo, '.git', 'config'), '[user]\n\tname = Test\n\temail = test@example.invalid\n'
+ `[commit]\n\tgpgsign = false\n[core]\n\thooksPath = "${join(base, 'hooks-none')}"\n`);
appendFileSync(join(repo, '.git', 'info', 'exclude'), '.claude/\n');
mkdirSync(join(repo, 'commands'), { recursive: true });
writeFileSync(join(repo, 'commands', 'up.md'), '# up\n\none\ntwo\n');
g(repo, 'add', '-A');
g(repo, 'commit', '-q', '-m', 'chore: start');
const start = g(repo, 'rev-parse', 'HEAD');
writeFileSync(join(repo, 'commands', 'up.md'), '# up\n\none\n```bash\nrm -rf "$NAME"\n```\n');
g(repo, 'add', '-A');
g(repo, 'commit', '-q', '-m', 'feat: the work');
const end = g(repo, 'rev-parse', 'HEAD');
const project = join(repo, ...PROJECT_REL.split('/'));
mkdirSync(project, { recursive: true });
const briefPath = join(project, 'brief.md');
const s = stampIntentApproval(BRIEF, new Date('2026-11-08T08:00:00Z'));
assert.equal(s.stamped, true);
writeFileSync(briefPath, s.text);
const root = resolveRepo(repo);
assert.equal(appendApproval({ repo: root, kind: 'intent', subject: normalizeSubject(root, briefPath), sha: normalizeHex(s.hash) }).appended, true);
const reviewPath = join(project, 'review.md');
return {
repo, start, end, project, briefPath, reviewPath,
write: (text) => writeFileSync(reviewPath, text),
read: () => readFileSync(reviewPath, 'utf8'),
finalize: () => run([FINALIZE, '--review', reviewPath, '--repo', repo, '--project', project, '--round', '1']),
validate: () => run([VALIDATOR, reviewPath]),
gate: () => run([GATE, '--json', reviewPath, '--repo', repo]),
};
}
/** An entry as the coordinator writes it into `known_outside`. */
const entry = (extra = {}) => ({
severity: 'BLOCKER', rule_key: 'SECURITY_RISK', file: 'commands/up.md', line: 5,
brief_ref: 'Threat Model', title: 'A hook edited by a local user runs the removal',
detail: 'commands/up.md:5 runs rm on a name a local user can set in a hook.',
recommended_action: 'Quote the name and refuse a path outside the project.',
threat_model_ref: OUTSIDE, ...extra,
});
/** A clean ALLOW review_version 1.2 review of start..end with `known` in known_outside. */
function tmReview({ start, end, known }) {
const json = JSON.stringify({
verdict: 'ALLOW', counts: { BLOCKER: 0, MAJOR: 0, MINOR: 0, SUGGESTION: 0 },
allow_blocked_by: [], declined_to_judge: [], trimmed: 0, findings: [],
...(known !== undefined ? { known_outside: known } : {}),
}, null, 2);
return `---
type: trekreview
review_version: "1.2"
task: "t"
slug: tm
project_dir: ${PROJECT_REL}/
brief_path: ${PROJECT_REL}/brief.md
scope_sha_start: ${start}
scope_sha_end: ${end}
reviewed_files_count: 1
verdict: ALLOW
findings: []
---
# Review: t
## Executive Summary
x
## Coverage
| File | Treatment | Reason |
|------|-----------|--------|
| commands/up.md | deep-review | default |
## Declined to Judge
None.
Trimmed: 0
## Remediation Summary
x
\`\`\`json
${json}
\`\`\`
`;
}
const jsonOf = (text) => JSON.parse(text.slice(text.lastIndexOf('```json\n') + 8, text.lastIndexOf('```')));
const fmVerdict = (text) => /^verdict:\s*(\S+)/m.exec(text)?.[1];
function sectionOf(text, heading) {
const start = text.indexOf(`\n${heading}`);
if (start === -1) return '';
const next = text.indexOf('\n## ', start + heading.length + 1);
return text.slice(start + 1, next === -1 ? undefined : next);
}
const codes = (text) => validateReviewContent(text, { strict: true }).errors.map((e) => e.code);
// ---- the validator ------------------------------------------------------------------
test('validator: a well-formed known_outside is valid; a malformed one is REVIEW_KNOWN_OUTSIDE_MALFORMED (strict)', () => {
const ok = tmReview({ start: 'a'.repeat(40), end: 'b'.repeat(40), known: [entry()] });
assert.deepEqual(codes(ok).filter((c) => c === 'REVIEW_KNOWN_OUTSIDE_MALFORMED'), []);
for (const bad of ['x', [null], [entry({ threat_model_ref: '' })], [entry({ threat_model_ref: undefined })],
[entry({ rule_key: 7 })], [entry({ file: undefined })], [entry({ line: -1 })]]) {
const text = tmReview({ start: 'a'.repeat(40), end: 'b'.repeat(40), known: bad });
assert.ok(codes(text).includes('REVIEW_KNOWN_OUTSIDE_MALFORMED'), `malformed: ${JSON.stringify(bad)}`);
}
});
// ---- finalize -----------------------------------------------------------------------
test('finalize: a backed entry stays in known_outside, counts nothing, and is listed under ## Outside the Threat Model', () => {
const fx = tmRepo();
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry()] }));
const r = fx.finalize();
assert.equal(r.status, 0, r.stdout + r.stderr);
const text = fx.read();
const json = jsonOf(text);
assert.deepEqual(json.findings, []);
assert.equal(json.known_outside?.length, 1);
assert.equal(json.verdict, 'ALLOW');
const sec = sectionOf(text, '## Outside the Threat Model');
assert.ok(sec.includes(entry().title), sec);
assert.ok(sec.includes(OUTSIDE), sec);
const gate = fx.gate();
assert.equal(gate.status, 0, gate.stdout + gate.stderr);
});
test('finalize: an entry the brief no longer backs goes back into findings at its catalogue tier and raises the verdict', () => {
const fx = tmRepo();
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry()] }));
// The operator lifts it back in: the Outside line changes (the approval goes stale too).
writeFileSync(fx.briefPath, readFileSync(fx.briefPath, 'utf8').replace(OUTSIDE, 'anyone at all'));
const r = fx.finalize();
assert.equal(r.status, 0, r.stdout + r.stderr);
const text = fx.read();
const json = jsonOf(text);
const back = json.findings.filter((f) => f.rule_key === 'SECURITY_RISK' && f.file === 'commands/up.md');
assert.equal(back.length, 1, `put back: ${JSON.stringify(json.findings)}`);
assert.equal(back[0].severity, 'BLOCKER', 'at its catalogue tier');
assert.match(back[0].id, /^[0-9a-f]{40}$/);
assert.deepEqual(json.known_outside ?? [], []);
assert.equal(json.verdict, 'BLOCK');
assert.equal(fmVerdict(text), 'BLOCK');
assert.equal(json.counts.BLOCKER, 1);
const blockers = sectionOf(text, '## Findings (BLOCKER)');
assert.ok(blockers.includes(`### ${back[0].id}`), blockers);
assert.match(blockers, /\*\*Source:\*\* .*outside the threat model.*put back/i);
assert.ok(sectionOf(text, '## Remediation Steps').includes(back[0].id), 'the put-back BLOCKER gets its R-step');
const v = fx.validate();
assert.equal(v.status, 0, v.stdout + v.stderr);
assert.equal(fx.gate().status, 1, 'BLOCK');
});
test('finalize: an engine finding or a finding that is not a security finding is never backed, whatever its ref', () => {
const fx = tmRepo();
fx.write(tmReview({ start: fx.start, end: fx.end, known: [
entry({ engine: 'security-review', rule_key: 'SECURITY_INJECTION', title: 'engine one' }),
entry({ rule_key: 'MISSING_TEST', severity: 'MAJOR', line: 4, title: 'not security' }),
] }));
const r = fx.finalize();
assert.equal(r.status, 0, r.stdout + r.stderr);
const json = jsonOf(fx.read());
assert.deepEqual(json.findings.map((f) => [f.title, f.severity]).sort(), [['engine one', 'BLOCKER'], ['not security', 'MAJOR']]);
assert.deepEqual(json.known_outside ?? [], []);
assert.equal(json.verdict, 'BLOCK');
});
// ---- the verdict gate ---------------------------------------------------------------
test('gate: an ALLOW review whose known_outside entry the brief does not back is exit 2, never green', () => {
const fx = tmRepo();
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry({ threat_model_ref: INSIDE })] }));
const g2 = fx.gate();
assert.equal(g2.status, 2, g2.stdout + g2.stderr);
assert.match(JSON.parse(g2.stdout).reason, /threat model/i);
// Control: the same review with a backed entry passes.
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry()] }));
const g0 = fx.gate();
assert.equal(g0.status, 0, g0.stdout + g0.stderr);
});
test('gate: a backed entry turns not backed once the brief\'s approval is stale (the brief is re-read every time)', () => {
const fx = tmRepo();
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry()] }));
assert.equal(fx.gate().status, 0);
writeFileSync(fx.briefPath, readFileSync(fx.briefPath, 'utf8').replace(INSIDE, 'a crafted name'));
const g2 = fx.gate();
assert.equal(g2.status, 2, g2.stdout + g2.stderr);
assert.match(JSON.parse(g2.stdout).reason, /BRIEF_INTENT_APPROVAL_STALE/);
});
// ---- Part 2 fix round ---------------------------------------------------------------
test('fix 7: an engine finding the coordinator listed in known_outside while its payload is saved goes back once', () => {
const fx = tmRepo();
const engineFinding = {
id: 'c'.repeat(40), severity: 'BLOCKER', rule_key: 'SECURITY_INJECTION', file: 'commands/up.md', line: 5,
brief_ref: 'NFR — security (Claude Code /security-review)', title: 'Unquoted name reaches rm',
detail: 'rm -rf "$NAME" with a name from the request.', recommended_action: 'Quote and contain the name.', engine: 'security-review',
};
writeFileSync(join(fx.project, '.review-engine-payload-security-review.json'),
JSON.stringify({ reviewer: 'claude-security-review', findings: [engineFinding], out_of_scope: [] }));
const { id: _id, ...asListed } = engineFinding;
fx.write(tmReview({ start: fx.start, end: fx.end, known: [{ ...asListed, threat_model_ref: OUTSIDE }] }));
const r = fx.finalize();
assert.equal(r.status, 0, r.stdout + r.stderr);
const json = jsonOf(fx.read());
const at5 = json.findings.filter((f) => f.file === 'commands/up.md' && f.line === 5);
assert.equal(at5.length, 1, `put back once: ${JSON.stringify(json.findings.map((f) => [f.id, f.rule_key, f.engine]))}`);
assert.equal(at5[0].severity, 'BLOCKER');
assert.deepEqual(json.known_outside ?? [], []);
assert.equal(json.verdict, 'BLOCK');
const v = fx.validate();
assert.equal(v.status, 0, v.stdout + v.stderr);
});
test('fix 8: scripts/release-check.mjs exits 2 on a review whose known_outside entry the brief does not back', () => {
const fx = tmRepo();
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry({ threat_model_ref: INSIDE })] }));
const r = releaseCheck({ repo: fx.repo, review: fx.reviewPath, version: '1.0.0', suiteTimeoutMs: 30000 });
assert.equal(r.exit, 2, JSON.stringify(r.checks));
const rev = r.checks.find((c) => c.id === 'review');
assert.equal(rev.status, 'error');
assert.match(rev.detail, /threat model/i);
// Control: the same review with a backed entry passes the review check.
fx.write(tmReview({ start: fx.start, end: fx.end, known: [entry()] }));
const c = releaseCheck({ repo: fx.repo, review: fx.reviewPath, version: '1.0.0', suiteTimeoutMs: 30000 });
assert.equal(c.checks.find((x) => x.id === 'review').status, 'pass', JSON.stringify(c.checks));
});
test('fix 5: the approval prose says a ## Threat Model is covered too, when the brief has one', () => {
const line = (rel, needle) => read(rel).split('\n').find((l) => l.includes(needle)) ?? '';
assert.match(line('docs/command-modes.md', '| `--approve <project-dir'), /`## Threat Model`/);
assert.match(line('docs/HANDOVER-CONTRACTS.md', '| `intent_approved_hash` |'), /`## Threat Model`/);
assert.match(line('CLAUDE.md', '| `/trekbrief` |'), /`## Threat Model`/);
assert.match(line('README.md', '**Approval, logged and committed.**'), /`## Threat Model`/);
assert.match(line('README.md', '| **Approve** |'), /`## Threat Model`/);
assert.match(line('README.md', '**Intent approval gate.**'), /`## Threat Model`/);
const skill = read('skills/annotating/SKILL.md');
assert.match(skill.split('\n').find((l) => l.startsWith('description:')), /## Threat Model/);
assert.equal((flat(skill).match(/`## Intent` or `## Goal` \(or `## Threat Model`/g) ?? []).length, 2, 'the Process step and the guard list');
});
// ---- prose --------------------------------------------------------------------------
function subsection(text, heading) {
const start = text.indexOf(heading);
assert.ok(start >= 0, `heading missing: ${heading}`);
const rest = text.slice(start + heading.length);
const next = rest.search(/\n#{2,3} /);
return text.slice(start, next === -1 ? undefined : start + heading.length + next);
}
test('code-correctness-reviewer: dimension 6 reads only what the engines do not (markdown carrying shell), with the threshold, the keys and threat_model_ref', () => {
const text = read('agents/code-correctness-reviewer.md');
const dim = flat(subsection(text, '### 6. '));
assert.match(dim, /command, skill and agent markdown/);
assert.match(dim, /concrete attack path/);
assert.match(dim, /high confidence[^.]*Claude Code's (own )?threshold/);
assert.match(dim, /`SECURITY_INJECTION`/);
assert.match(dim, /`SECURITY_RISK`/);
assert.match(dim, /`threat_model_ref`/);
assert.match(dim, /`Outside:`/);
assert.doesNotMatch(dim, /XSS|SQL query/, 'code-level security is the engines\' part now');
assert.ok(!text.includes('SECURITY_REVIEW_VULNERABILITY'), 'the reviewer never names the engines\' key');
assert.ok(text.split('\n').some((l) => l.startsWith('| `SECURITY_RISK` | BLOCKER |')), 'the severity table names SECURITY_RISK');
});
test('review-coordinator: a Voyage security finding with threat_model_ref goes to known_outside, never into findings', () => {
const t = flat(read('agents/review-coordinator.md'));
assert.match(t, /`threat_model_ref`[^.]*`known_outside`[^.]*never into `findings`/);
assert.match(t, /"known_outside"/, 'the JSON template carries known_outside');
});
test('/trekreview Phase 8.8 lists the known issues outside the threat model; the template shows the section and field', () => {
const md = read('commands/trekreview.md');
const p88 = flat(md.slice(md.indexOf('## Phase 8.8'), md.indexOf('## Phase 9')));
assert.match(p88, /## Outside the Threat Model/);
assert.match(p88, /`known_outside`/);
assert.match(p88, /not answered/);
const tpl = read('templates/trekreview-template.md');
assert.match(tpl, /^## Outside the Threat Model$/m);
assert.match(tpl, /"known_outside": \[/);
});
test('Handover 1 names the optional ## Threat Model; Handover 6 names known_outside and its code', () => {
const h = read('docs/HANDOVER-CONTRACTS.md');
const h1 = h.slice(h.indexOf('## Handover 1'), h.indexOf('## Handover 2'));
assert.match(h1, /`## Threat Model`/);
assert.match(h1, /BRIEF_THREAT_MODEL_MALFORMED/);
const h6 = h.slice(h.indexOf('## Handover 6'), h.indexOf('## Handover 7'));
assert.match(h6, /`known_outside`/);
assert.match(h6, /REVIEW_KNOWN_OUTSIDE_MALFORMED/);
assert.match(h6, /## Outside the Threat Model/);
});