refactor(review): simplify pass over E (reuse, simplification, efficiency)
Quality only, no behaviour change (four cleanup reviewers, one implementer; npm test 2480/0 on the working tree): - threat-model.mjs splitBacked(): the one known_outside backing check of plan-gate (record and bare gate), verdict-gate and review-finalize, with one why-text; the engines' security keys derived from ENGINES; isSecurity. - coordinator-contract: "not a security engine" read from ENGINES (a string engine only), not the code-review literal. - review-rounds owns trailingBlock, findingsBlockSha, isMajor, isBlocker; review-answers imports them (review-rounds still imports nothing from it). prepare keeps the archive bytes it read instead of reading the previous archive again. - engine-compare exports rowIn and payloadEngine; finalize uses them. - plan-gate: one accepted_open builder; the stop-rule check hoisted; one top-level lookup for --record --brief. - security-engine: notFullShas(), runCli's usage flag, readScopeList shared with engine-findings; findReport and reviewRoundSummary module-private; two dead defaults gone. - tests: shared plan-gate harness and threat-model brief helpers; the [CNV] table cases call convertReport in-process, one CLI case per exit path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
b94b060035
commit
f820081bdc
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
18 changed files with 332 additions and 373 deletions
|
|
@ -136,6 +136,9 @@ const ENGINE_OF_PAYLOAD = Object.freeze(Object.fromEntries(
|
|||
Object.entries(ENGINES).map(([engine, spec]) => [spec.reviewer, engine])));
|
||||
|
||||
const isEngineFinding = (f) => Object.hasOwn(ENGINES, f.engine ?? '');
|
||||
// From an engine that is not a security engine (/code-review). A string only:
|
||||
// a list or object `engine` would coerce to an ENGINES key.
|
||||
const fromNonSecurityEngine = (f) => typeof f?.engine === 'string' && ENGINES[f.engine]?.security === false;
|
||||
|
||||
function carries(f, field) {
|
||||
return f?.[field] !== undefined && f?.[field] !== null;
|
||||
|
|
@ -159,7 +162,7 @@ export function lowerableBriefConflict(f) {
|
|||
|
||||
function markedMajorCodeReview(f) {
|
||||
return isMark(f?.contradicts_brief) &&
|
||||
f.engine === 'code-review' &&
|
||||
fromNonSecurityEngine(f) &&
|
||||
getRule(f.rule_key)?.severity === 'MAJOR';
|
||||
}
|
||||
|
||||
|
|
@ -667,7 +670,7 @@ export function runContract(reviewerPayloads, options = {}) {
|
|||
// unchanged file (independent review, 2026-10-08).
|
||||
const outOfScope = (options.engineOutOfScope ?? [])
|
||||
.map((f) => ({ ...f, suppressed_reason: 'engine:out-of-scope' }));
|
||||
const minorOnly = (f) => f.engine === 'code-review' && (f.severity === 'MINOR' || f.severity === 'SUGGESTION');
|
||||
const minorOnly = (f) => fromNonSecurityEngine(f) && (f.severity === 'MINOR' || f.severity === 'SUGGESTION');
|
||||
const outOfScopeBlocking = outOfScope.filter((f) => !minorOnly(f));
|
||||
const outOfScopeNoted = outOfScope.filter(minorOnly);
|
||||
// Claude Security's findings below confidence `high` (security-engine.mjs
|
||||
|
|
|
|||
|
|
@ -38,11 +38,19 @@ const trimmedTitle = (t) => (t.length > JUDGE_TITLE_MAX ? trimText(t, JUDGE_TITL
|
|||
|
||||
export { tripletOf, normTitle, rowOf, trimmedTitle };
|
||||
|
||||
/** A finding's row is in `rows` (a Set of rowOf keys): as it is, or with its title as the coordinator trims it. */
|
||||
export const rowIn = (rows, f) => rows.has(rowOf(f)) ||
|
||||
(typeof f.title === 'string' && rows.has(rowOf({ ...f, title: trimmedTitle(f.title) })));
|
||||
|
||||
const PUT_BACK_SEVERITIES = ['BLOCKER', 'MAJOR'];
|
||||
const VERDICT_RANK = { ALLOW: 0, WARN: 1, BLOCK: 2 };
|
||||
export const ENGINE_OF_REVIEWER = Object.freeze(Object.fromEntries(Object.entries(ENGINES).map(([engine, s]) => [s.reviewer, engine])));
|
||||
const isPlainObject = (x) => x !== null && typeof x === 'object' && !Array.isArray(x);
|
||||
|
||||
/** The engine a payload's top-level `reviewer` maps back to through ENGINES, or null. */
|
||||
export const payloadEngine = (p) => (isPlainObject(p) && typeof p.reviewer === 'string' && Object.hasOwn(ENGINE_OF_REVIEWER, p.reviewer)
|
||||
? ENGINE_OF_REVIEWER[p.reviewer] : null);
|
||||
|
||||
/**
|
||||
* The engine findings at MAJOR or BLOCKER whose row no finding of `payload`
|
||||
* carries, as copies to append, and the verdict raised by them.
|
||||
|
|
@ -53,12 +61,10 @@ const isPlainObject = (x) => x !== null && typeof x === 'object' && !Array.isArr
|
|||
export function putBack(payload, enginePayloads) {
|
||||
const returned = (Array.isArray(payload?.findings) ? payload.findings : []).filter(isPlainObject);
|
||||
const rows = new Set(returned.map(rowOf));
|
||||
const rowReturned = (m) => rows.has(rowOf(m)) ||
|
||||
(typeof m.title === 'string' && rows.has(rowOf({ ...m, title: trimmedTitle(m.title) })));
|
||||
const candidates = [];
|
||||
for (const p of Array.isArray(enginePayloads) ? enginePayloads : []) {
|
||||
if (!isPlainObject(p) || !Array.isArray(p.findings)) continue;
|
||||
const engine = typeof p.reviewer === 'string' && Object.hasOwn(ENGINE_OF_REVIEWER, p.reviewer) ? ENGINE_OF_REVIEWER[p.reviewer] : null;
|
||||
const engine = payloadEngine(p);
|
||||
if (!engine) continue;
|
||||
for (const f of p.findings) {
|
||||
if (!isPlainObject(f) || !PUT_BACK_SEVERITIES.includes(f.severity)) continue;
|
||||
|
|
@ -69,7 +75,7 @@ export function putBack(payload, enginePayloads) {
|
|||
candidates.sort((a, b) => a.rank - b.rank);
|
||||
const added = [];
|
||||
for (const { f, engine } of candidates) {
|
||||
if (rowReturned(f)) continue;
|
||||
if (rowIn(rows, f)) continue;
|
||||
const { id: _id, brief_lowered_from: _lowered, contradicts_brief: _contradicts, pre_existing: _pre, carried_from: _cf, ...rest } = f;
|
||||
const copy = { id: null, ...rest, engine };
|
||||
added.push(copy);
|
||||
|
|
|
|||
|
|
@ -102,6 +102,17 @@ function parseLine(v) {
|
|||
|
||||
const tier = (key) => SEVERITY_VALUES.indexOf(getRule(key).severity);
|
||||
|
||||
/**
|
||||
* A scope list file (one path per line; lines trimmed, blank ones dropped), as
|
||||
* `--scope` gives it here and to lib/review/security-engine.mjs. Throws when
|
||||
* the file cannot be read.
|
||||
* @param {string} path
|
||||
* @returns {string[]}
|
||||
*/
|
||||
export function readScopeList(path) {
|
||||
return readFileSync(path, 'utf8').split('\n').map((l) => l.trim()).filter(Boolean);
|
||||
}
|
||||
|
||||
/** /code-review categories that name an improvement, not a defect. */
|
||||
export const IMPROVEMENT_CATEGORIES = Object.freeze(new Set(['simplification', 'efficiency', 'reuse', 'style', 'readability']));
|
||||
|
||||
|
|
@ -188,7 +199,7 @@ if (isMain(import.meta.url)) {
|
|||
let scopeFiles;
|
||||
try {
|
||||
input = JSON.parse(readFileSync(filePath, 'utf-8'));
|
||||
if (scopePath) scopeFiles = readFileSync(scopePath, 'utf-8').split('\n').map((l) => l.trim()).filter(Boolean);
|
||||
if (scopePath) scopeFiles = readScopeList(scopePath);
|
||||
} catch (e) {
|
||||
process.stderr.write(`engine-findings: could not read input: ${e.message}\n`);
|
||||
process.exit(2);
|
||||
|
|
|
|||
|
|
@ -80,10 +80,12 @@ import { createHash } from 'node:crypto';
|
|||
import { spawnSync } from 'node:child_process';
|
||||
import { accessSync, constants as fsConstants, lstatSync, readFileSync, realpathSync, renameSync, writeFileSync } from 'node:fs';
|
||||
import { basename, dirname, join, resolve } from 'node:path';
|
||||
import { fencedBlocks, isJsonBlock } from './findings-schema.mjs';
|
||||
import { reviewedSha } from './reviewed-commit.mjs';
|
||||
import { preExistingCheck, scopeShas } from './pre-existing.mjs';
|
||||
import { archiveName, countingMajors, frontmatterField, isSecurityFinding, runId, sameFinding } from './review-rounds.mjs';
|
||||
import {
|
||||
archiveName, countingMajors, findingsBlockSha, frontmatterField, isBlocker, isMajor, isSecurityFinding, runId,
|
||||
sameFinding, trailingBlock,
|
||||
} from './review-rounds.mjs';
|
||||
import {
|
||||
appendApproval, isHollowAnswer, logPath, normalizeSubject, readLog, verifyLog,
|
||||
} from '../util/approval-log.mjs';
|
||||
|
|
@ -105,26 +107,9 @@ const LINK_RE = /^(review-round-([0-9a-f]{12})-(\d+)\.md)#([0-9a-f]{40})$/;
|
|||
const ARCHIVE_NAME_RE = /^review-round-([0-9a-f]{12})-(\d+)\.md$/;
|
||||
|
||||
const sha256 = (s) => createHash('sha256').update(s).digest('hex');
|
||||
const isMajor = (f) => String(f?.severity ?? '').toUpperCase() === 'MAJOR';
|
||||
const isBlocker = (f) => String(f?.severity ?? '').toUpperCase() === 'BLOCKER';
|
||||
|
||||
/** The trailing fenced block when it is a closed `json` block, else null. */
|
||||
function trailingBlock(text) {
|
||||
const last = fencedBlocks(String(text ?? '')).at(-1);
|
||||
return last && last.closed && isJsonBlock(last) ? last : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* sha256 of the trailing JSON block's body (the one reader the gate uses,
|
||||
* findings-schema `fencedBlocks`), or null when the last block is not a
|
||||
* closed `json` block.
|
||||
* @param {string} text
|
||||
* @returns {string|null}
|
||||
*/
|
||||
export function findingsBlockSha(text) {
|
||||
const last = trailingBlock(text);
|
||||
return last ? sha256(last.body) : null;
|
||||
}
|
||||
/** sha256 of the trailing JSON block's body, or null (defined in review-rounds.mjs). */
|
||||
export { findingsBlockSha };
|
||||
|
||||
/**
|
||||
* The sha256 a K2 `review` record carries for one answer: the canonical JSON
|
||||
|
|
|
|||
|
|
@ -130,8 +130,8 @@ import { isSecurityFinding } from './review-answers.mjs';
|
|||
import { carry, frontmatterField, MAX_REVIEW_ROUNDS, runId } from './review-rounds.mjs';
|
||||
import { applyPolicy, readPolicy } from './review-policy.mjs';
|
||||
import { ENGINES } from './engine-findings.mjs';
|
||||
import { ENGINE_OF_REVIEWER, putBack, rowOf, trimmedTitle } from './engine-compare.mjs';
|
||||
import { backedLine, isKnownEntry, reviewThreatModel } from './threat-model.mjs';
|
||||
import { payloadEngine, putBack, rowIn, rowOf } from './engine-compare.mjs';
|
||||
import { isKnownEntry, reviewThreatModel, splitBacked } from './threat-model.mjs';
|
||||
import { getRule } from './rule-catalogue.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
|
|
@ -287,23 +287,16 @@ function outsideSection(kept, back) {
|
|||
*/
|
||||
function passOutside(payload, ctx) {
|
||||
if (!Array.isArray(payload.known_outside) || payload.known_outside.length === 0) return {};
|
||||
const use = reviewThreatModel(ctx.text, ctx.repo);
|
||||
const tm = use.usable ? use.threatModel : null;
|
||||
const kept = [];
|
||||
const back = [];
|
||||
for (const e of payload.known_outside) {
|
||||
if (isKnownEntry(e) && !(tm && backedLine(e, tm) !== null)) back.push(e);
|
||||
else kept.push(e);
|
||||
}
|
||||
const why = tm ? 'no `Outside:` line of the brief equals its threat_model_ref, or it is not a Voyage security finding' : use.why;
|
||||
// Only a well-formed entry goes back; a malformed one stays listed as such.
|
||||
const { unbacked: back, why } = splitBacked(payload.known_outside.filter(isKnownEntry), reviewThreatModel(ctx.text, ctx.repo));
|
||||
const kept = payload.known_outside.filter((e) => !back.includes(e));
|
||||
// A row a finding already carries (the engine pass put the same engine
|
||||
// finding back) is counted once: the entry leaves the list, no second copy.
|
||||
const rows = new Set(payload.findings.filter(isPlainObject).map(rowOf));
|
||||
for (const e of back) {
|
||||
const rule = getRule(e.rule_key.trim().toUpperCase());
|
||||
const f = { ...e, id: null, rule_key: rule ? rule.rule_key : e.rule_key, severity: rule ? rule.severity : 'BLOCKER' };
|
||||
const held = rows.has(rowOf(f)) || (typeof f.title === 'string' && rows.has(rowOf({ ...f, title: trimmedTitle(f.title) })));
|
||||
if (held) continue;
|
||||
if (rowIn(rows, f)) continue;
|
||||
rows.add(rowOf(f));
|
||||
payload.findings.push(f);
|
||||
ctx.added.add(f);
|
||||
|
|
@ -425,10 +418,6 @@ function resolvedSection(n, list) {
|
|||
return `## Resolved since round ${n}\n\nPrevious findings this round no longer raises: ${list.length}.\n\n${items.join('\n')}\n\n`;
|
||||
}
|
||||
|
||||
/** The engine a payload's top-level `reviewer` maps back to through ENGINES, or null. */
|
||||
const payloadEngine = (p) => (isPlainObject(p) && typeof p.reviewer === 'string' && Object.hasOwn(ENGINE_OF_REVIEWER, p.reviewer)
|
||||
? ENGINE_OF_REVIEWER[p.reviewer] : null);
|
||||
|
||||
/**
|
||||
* The entries each engine raised this round outside its `findings` (decision 222):
|
||||
* `below_threshold` and `out_of_scope`, plain objects only, each with the
|
||||
|
|
@ -721,7 +710,7 @@ function findingsBefore(sev) {
|
|||
* The body entries: one added for each finding in `added`, and a `### <id>`
|
||||
* entry that sits under another severity's `## Findings` section moved to its own.
|
||||
*/
|
||||
function placeEntries(head, findings, added, putBackSet, outsideBack = new Map()) {
|
||||
function placeEntries(head, findings, added, putBackSet, outsideBack) {
|
||||
let lines = head.split('\n');
|
||||
const fmEnd = frontmatterEnd(lines);
|
||||
for (const f of findings) {
|
||||
|
|
|
|||
|
|
@ -134,20 +134,31 @@ function regularFiles(dir, re) {
|
|||
}).sort();
|
||||
}
|
||||
|
||||
/**
|
||||
* sha256 of the trailing fenced block's body when it is a closed `json`
|
||||
* block, else null. A copy of findingsBlockSha (lib/review/review-answers.mjs,
|
||||
* which imports this module, so importing it back would close a cycle).
|
||||
*/
|
||||
function blockSha(text) {
|
||||
// The trailing-block helpers live here and review-answers.mjs imports them:
|
||||
// it imports this module, so this one never imports it back.
|
||||
|
||||
/** The trailing fenced block when it is a closed `json` block, else null. */
|
||||
export function trailingBlock(text) {
|
||||
const last = fencedBlocks(String(text ?? '')).at(-1);
|
||||
return last && last.closed && isJsonBlock(last) ? createHash('sha256').update(last.body).digest('hex') : null;
|
||||
return last && last.closed && isJsonBlock(last) ? last : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* sha256 of the trailing JSON block's body (the one reader the gate uses,
|
||||
* findings-schema `fencedBlocks`), or null when the last block is not a
|
||||
* closed `json` block.
|
||||
* @param {string} text
|
||||
* @returns {string|null}
|
||||
*/
|
||||
export function findingsBlockSha(text) {
|
||||
const last = trailingBlock(text);
|
||||
return last ? createHash('sha256').update(last.body).digest('hex') : null;
|
||||
}
|
||||
|
||||
const notMeasured = (reason, detail = '') => ({ exit: 2, reason: `${reason} — not measured${detail ? ` (${detail})` : ''}` });
|
||||
|
||||
const isMajor = (f) => String(f?.severity ?? '').toUpperCase() === 'MAJOR';
|
||||
const isBlocker = (f) => String(f?.severity ?? '').toUpperCase() === 'BLOCKER';
|
||||
export const isMajor = (f) => String(f?.severity ?? '').toUpperCase() === 'MAJOR';
|
||||
export const isBlocker = (f) => String(f?.severity ?? '').toUpperCase() === 'BLOCKER';
|
||||
|
||||
/**
|
||||
* Every MAJOR finding (case-insensitive) that counts toward the lock. A finding
|
||||
|
|
@ -163,10 +174,10 @@ export function countingMajors(payload, { preExisting = () => false } = {}) {
|
|||
&& !(preExisting(f) === true && !isSecurityFinding(f)));
|
||||
}
|
||||
|
||||
/** The parsed trailing JSON block of a review (an object), or null. */
|
||||
/** The parsed trailing JSON block of a review (an object with a `findings` list), or null. */
|
||||
function payloadOf(text) {
|
||||
const last = fencedBlocks(String(text ?? '')).at(-1);
|
||||
if (!last || !last.closed || !isJsonBlock(last)) return null;
|
||||
const last = trailingBlock(text);
|
||||
if (!last) return null;
|
||||
try {
|
||||
const p = JSON.parse(last.body);
|
||||
return p && typeof p === 'object' && !Array.isArray(p) && Array.isArray(p.findings) ? p : null;
|
||||
|
|
@ -180,7 +191,7 @@ function payloadOf(text) {
|
|||
* review has no readable JSON block.
|
||||
* @returns {{blocking: number, newByClass: Object<string, number>}|null}
|
||||
*/
|
||||
export function reviewRoundSummary(text, repo) {
|
||||
function reviewRoundSummary(text, repo) {
|
||||
const payload = payloadOf(text);
|
||||
if (!payload) return null;
|
||||
const pre = preExistingCheck(text, repo);
|
||||
|
|
@ -188,7 +199,7 @@ export function reviewRoundSummary(text, repo) {
|
|||
...payload.findings.filter((f) => f && typeof f === 'object' && isBlocker(f)),
|
||||
...countingMajors(payload, { preExisting: (f) => !f.carried_from && pre(f) }),
|
||||
];
|
||||
return roundSummary(blocking, { fullCount: false });
|
||||
return roundSummary(blocking);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -226,19 +237,19 @@ export function prepare({ projectDir, since, repo, dryRun = false } = {}) {
|
|||
const roundField = frontmatterField(text, 'round');
|
||||
// review.md's own archive (see the header): byte-equal first, then by content.
|
||||
const ownName = archiveName(run, roundField === null ? 1 : roundField);
|
||||
const ownBlock = blockSha(text);
|
||||
const ownBlock = findingsBlockSha(text);
|
||||
const ownEnd = reviewedSha(text);
|
||||
const isOwn = (name, bytes) => {
|
||||
if (bytes.equals(review)) return true;
|
||||
if (name !== ownName || ownBlock === null || ownEnd === '') return false;
|
||||
const t = bytes.toString('utf8');
|
||||
return blockSha(t) === ownBlock && reviewedSha(t) === ownEnd;
|
||||
return findingsBlockSha(t) === ownBlock && reviewedSha(t) === ownEnd;
|
||||
};
|
||||
const others = [];
|
||||
for (const n of archives) {
|
||||
const bytes = readRegular(join(projectDir, n));
|
||||
if (bytes === null) return notMeasured(`archive ${n} is not readable`);
|
||||
if (!isOwn(n, bytes)) others.push(n);
|
||||
for (const name of archives) {
|
||||
const bytes = readRegular(join(projectDir, name));
|
||||
if (bytes === null) return notMeasured(`archive ${name} is not readable`);
|
||||
if (!isOwn(name, bytes)) others.push({ name, bytes });
|
||||
}
|
||||
const P = 1 + others.length;
|
||||
if (roundField !== null && roundField !== String(P)) {
|
||||
|
|
@ -250,11 +261,10 @@ export function prepare({ projectDir, since, repo, dryRun = false } = {}) {
|
|||
}
|
||||
if (P >= 2) {
|
||||
// The stop rule on the pair (P - 1, P): the latest other archive and review.md.
|
||||
const nOf = (name) => Number(archiveRe.exec(name)[1]);
|
||||
const prevName = [...others].sort((a, b) => nOf(b) - nOf(a))[0];
|
||||
const prevBytes = readRegular(join(projectDir, prevName));
|
||||
const nOf = (o) => Number(archiveRe.exec(o.name)[1]);
|
||||
const prev = [...others].sort((x, y) => nOf(y) - nOf(x))[0];
|
||||
const at = repo ?? projectDir;
|
||||
const a = prevBytes === null ? null : reviewRoundSummary(prevBytes.toString('utf8'), at);
|
||||
const a = reviewRoundSummary(prev.bytes.toString('utf8'), at);
|
||||
const b = reviewRoundSummary(text, at);
|
||||
if (!a || !b) return notMeasured('stop rule: a round has no readable JSON block');
|
||||
const d = decideStop([{ round: P - 1, ...a }, { round: P, ...b }]);
|
||||
|
|
|
|||
|
|
@ -71,7 +71,7 @@
|
|||
import { spawnSync } from 'node:child_process';
|
||||
import { existsSync, lstatSync, readFileSync, readdirSync } from 'node:fs';
|
||||
import { join, posix } from 'node:path';
|
||||
import { toReviewerPayload, ENGINES } from './engine-findings.mjs';
|
||||
import { toReviewerPayload, ENGINES, readScopeList } from './engine-findings.mjs';
|
||||
import { FULL_SHA_RE } from './reviewed-commit.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
|
|
@ -89,6 +89,14 @@ const isPlainObject = (x) => x !== null && typeof x === 'object' && !Array.isArr
|
|||
const couldNotRun = (reason) => ({ exit: 2, reason });
|
||||
const notMeasured = (why) => ({ exit: 2, reason: `Claude Security report not measured: ${why}` });
|
||||
|
||||
/** The first of --base / --head that is not a full commit id, as a could-not-run; else null. */
|
||||
function notFullShas(base, head) {
|
||||
for (const [name, sha] of [['--base', base], ['--head', head]]) {
|
||||
if (!FULL_SHA_RE.test(String(sha ?? ''))) return couldNotRun(`${name} must be a full 40- or 64-hex commit id`);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
// ---- choose ------------------------------------------------------------------------
|
||||
|
||||
function git(repo, args) {
|
||||
|
|
@ -111,8 +119,8 @@ function differs(repo, ...revs) {
|
|||
*/
|
||||
export function chooseEngine({ repo, base, head, claudeSecurity } = {}) {
|
||||
if (typeof repo !== 'string' || repo === '') return couldNotRun('--repo is required');
|
||||
if (!FULL_SHA_RE.test(String(base ?? ''))) return couldNotRun('--base must be a full 40- or 64-hex commit id');
|
||||
if (!FULL_SHA_RE.test(String(head ?? ''))) return couldNotRun('--head must be a full 40- or 64-hex commit id');
|
||||
const bad = notFullShas(base, head);
|
||||
if (bad) return bad;
|
||||
if (claudeSecurity !== 'available' && claudeSecurity !== 'missing') {
|
||||
return couldNotRun('--claude-security must be available or missing');
|
||||
}
|
||||
|
|
@ -165,7 +173,7 @@ function stampIn(folder) {
|
|||
const sameSha = (a, b) => typeof a === 'string' && a.toLowerCase() === b.toLowerCase();
|
||||
|
||||
/** The newest report folder in `dir` whose stamp is a changes scan of exactly base..head, or null. */
|
||||
export function findReport(dir, base, head) {
|
||||
function findReport(dir, base, head) {
|
||||
let names;
|
||||
try { names = readdirSync(dir); } catch { return null; }
|
||||
const folders = names.filter((n) => REPORT_DIR_RE.test(n) && isDir(join(dir, n))).sort().reverse();
|
||||
|
|
@ -253,8 +261,8 @@ function readRecords(folder, prefix) {
|
|||
*/
|
||||
export function convertReport({ reportDir, base, head, scopeFiles } = {}) {
|
||||
if (typeof reportDir !== 'string' || reportDir === '') return couldNotRun('--report-dir is required');
|
||||
if (!FULL_SHA_RE.test(String(base ?? ''))) return couldNotRun('--base must be a full 40- or 64-hex commit id');
|
||||
if (!FULL_SHA_RE.test(String(head ?? ''))) return couldNotRun('--head must be a full 40- or 64-hex commit id');
|
||||
const bad = notFullShas(base, head);
|
||||
if (bad) return bad;
|
||||
const found = findReport(reportDir, base, head);
|
||||
if (!found) {
|
||||
return notMeasured(`no CLAUDE-SECURITY-<ts>/ folder in ${reportDir} holds one stamp of a changes scan of ${base}..${head} (an empty diff writes none)`);
|
||||
|
|
@ -300,28 +308,31 @@ function flags(args, known) {
|
|||
return out;
|
||||
}
|
||||
|
||||
/** A usage error: the CLI prints USAGE after the reason. */
|
||||
const badUsage = (reason) => ({ ...couldNotRun(reason), usage: true });
|
||||
|
||||
function runCli(argv) {
|
||||
const [cmd, ...rest] = argv;
|
||||
if (cmd === 'choose') {
|
||||
const f = flags(rest, ['--repo', '--base', '--head', '--claude-security']);
|
||||
if (!f) return couldNotRun('bad arguments');
|
||||
if (!f) return badUsage('bad arguments');
|
||||
const r = chooseEngine({ repo: f['--repo'], base: f['--base'], head: f['--head'], claudeSecurity: f['--claude-security'] });
|
||||
return r.exit === 0 ? { exit: 0, out: { engine: r.engine, reason: r.reason } } : r;
|
||||
}
|
||||
if (cmd === 'convert') {
|
||||
const f = flags(rest, ['--report-dir', '--base', '--head', '--scope']);
|
||||
if (!f) return couldNotRun('bad arguments');
|
||||
if (!f) return badUsage('bad arguments');
|
||||
let scopeFiles;
|
||||
if (f['--scope'] !== undefined) {
|
||||
if (!existsSync(f['--scope'])) return couldNotRun(`--scope ${f['--scope']} does not exist`);
|
||||
try {
|
||||
scopeFiles = readFileSync(f['--scope'], 'utf8').split('\n').map((l) => l.trim()).filter(Boolean);
|
||||
scopeFiles = readScopeList(f['--scope']);
|
||||
} catch (e) { return couldNotRun(`cannot read --scope: ${e.message}`); }
|
||||
}
|
||||
const r = convertReport({ reportDir: f['--report-dir'], base: f['--base'], head: f['--head'], scopeFiles });
|
||||
return r.exit === 0 ? { exit: 0, out: r.payload } : r;
|
||||
}
|
||||
return couldNotRun('a subcommand is required: choose or convert');
|
||||
return badUsage('a subcommand is required: choose or convert');
|
||||
}
|
||||
|
||||
if (isMain(import.meta.url)) {
|
||||
|
|
@ -331,7 +342,7 @@ if (isMain(import.meta.url)) {
|
|||
process.stdout.write(`${JSON.stringify(r.out, null, 2)}\n`);
|
||||
} else {
|
||||
process.stderr.write(`security-engine: ${r.reason}\n`);
|
||||
if (r.reason === 'bad arguments' || r.reason.startsWith('a subcommand')) process.stderr.write(USAGE);
|
||||
if (r.usage) process.stderr.write(USAGE);
|
||||
}
|
||||
process.exit(r.exit);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -21,6 +21,9 @@
|
|||
// Otherwise every security finding counts, and `why` says why.
|
||||
// backedLine(finding, tm) -> the `Outside:` text the finding's
|
||||
// `threat_model_ref` equals exactly, else null.
|
||||
// splitBacked(entries, use, asFinding) -> {backed, unbacked, why}: the one
|
||||
// backing check of both gates' `known_outside` (backedLine under a usable
|
||||
// threat model; `why` says why the unbacked ones are not backed).
|
||||
// usableInRepo(briefPath, repo) -> usable() for a brief path relative to
|
||||
// the repo's top level (an absolute one as it is); both gates read a
|
||||
// stored `brief_path` this way, whatever directory they run in.
|
||||
|
|
@ -45,10 +48,11 @@ import { parseDocument } from '../util/frontmatter.mjs';
|
|||
import { verifyLog, normalizeHex, normalizeSubject, resolveRepo } from '../util/approval-log.mjs';
|
||||
import { checkIntentApprovalContent } from '../validators/intent-approval.mjs';
|
||||
import { parseThreatModel } from '../validators/brief-validator.mjs';
|
||||
import { isSecurityKey, keysOf } from './plan-review-dedup.mjs';
|
||||
import { isSecurity, keysOf } from './plan-review-dedup.mjs';
|
||||
import { ENGINES, SECURITY_ENGINES } from './engine-findings.mjs';
|
||||
|
||||
/** The engines' catalogue key (/security-review, Claude Security): never Voyage's own. */
|
||||
export const ENGINES_SECURITY_KEY = 'SECURITY_REVIEW_VULNERABILITY';
|
||||
/** The security engines' catalogue keys (/security-review, Claude Security): never Voyage's own. */
|
||||
const ENGINE_SECURITY_KEYS = new Set(SECURITY_ENGINES.map((e) => ENGINES[e].rule_key));
|
||||
|
||||
const EMPTY = Object.freeze({ present: false, inside: [], outside: [], problems: [] });
|
||||
|
||||
|
|
@ -154,9 +158,8 @@ export function isKnownEntry(e) {
|
|||
export function isOwnSecurityFinding(f) {
|
||||
if (!f || typeof f !== 'object') return false;
|
||||
if (typeof f.engine === 'string' && f.engine.trim() !== '') return false;
|
||||
const keys = keysOf(f);
|
||||
if (keys.some((k) => k.trim().toUpperCase() === ENGINES_SECURITY_KEY)) return false;
|
||||
return keys.some(isSecurityKey);
|
||||
if (keysOf(f).some((k) => ENGINE_SECURITY_KEYS.has(k.trim().toUpperCase()))) return false;
|
||||
return isSecurity(f);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -173,3 +176,23 @@ export function backedLine(finding, tm) {
|
|||
if (typeof ref !== 'string' || ref === '') return null;
|
||||
return tm.outside.find((line) => line === ref) ?? null;
|
||||
}
|
||||
|
||||
const NOT_BACKED = 'no `Outside:` line of the brief equals the text it quotes, or it is not a well-formed Voyage security finding';
|
||||
|
||||
/**
|
||||
* Split entries on the threat model `use` (from usable / usableInRepo /
|
||||
* reviewThreatModel): backed when backedLine finds the `Outside:` line of
|
||||
* `asFinding(entry)`; a threat model that is not usable backs nothing.
|
||||
* @param {Array<*>} entries
|
||||
* @param {{usable: boolean, why: string|null, threatModel: object}|undefined} use
|
||||
* @param {(e: *) => *} [asFinding] - the finding backedLine checks for an entry (default: the entry)
|
||||
* @returns {{backed: Array<*>, unbacked: Array<*>, why: string|null}} why = why the unbacked ones are not backed (null when none is)
|
||||
*/
|
||||
export function splitBacked(entries, use, asFinding = (e) => e) {
|
||||
const tm = use?.usable === true ? use.threatModel : null;
|
||||
const backed = [];
|
||||
const unbacked = [];
|
||||
for (const e of entries) (backedLine(asFinding(e), tm) !== null ? backed : unbacked).push(e);
|
||||
if (unbacked.length === 0) return { backed, unbacked, why: null };
|
||||
return { backed, unbacked, why: tm ? NOT_BACKED : (use?.why ?? 'the threat model is not usable') };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -93,7 +93,7 @@ import { frontmatterField, runId } from './review-rounds.mjs';
|
|||
// No cycle: neither module, nor any module they import, imports this gate.
|
||||
import { roundEvidence } from './review-finalize.mjs';
|
||||
import { putBack, normTitle } from './engine-compare.mjs';
|
||||
import { backedLine, isKnownEntry, reviewThreatModel } from './threat-model.mjs';
|
||||
import { isKnownEntry, reviewThreatModel, splitBacked } from './threat-model.mjs';
|
||||
|
||||
export const EXIT = Object.freeze({ PASS: 0, BLOCK: 1, UNDECIDED: 2 });
|
||||
|
||||
|
|
@ -319,7 +319,7 @@ export function droppedEngineFindings(text, reviewPath) {
|
|||
|
||||
/**
|
||||
* The `known_outside` entries of review.md the brief at its `brief_path` does
|
||||
* not back now (lib/review/threat-model.mjs `reviewThreatModel`, `backedLine`):
|
||||
* not back now (lib/review/threat-model.mjs `reviewThreatModel`, `splitBacked`):
|
||||
* a malformed list or entry, an engine finding, not a security finding, an
|
||||
* `Outside:` line gone, or a threat model that is not usable. An unreadable
|
||||
* trailing block gives none (decideVerdict judges it).
|
||||
|
|
@ -333,11 +333,9 @@ export function notBackedOutside(text, repo) {
|
|||
const list = payload.known_outside;
|
||||
if (!Array.isArray(list)) return { entries: [list], why: 'known_outside is not a list' };
|
||||
if (list.length === 0) return { entries: [], why: null };
|
||||
const use = reviewThreatModel(text, repo);
|
||||
const tm = use.usable ? use.threatModel : null;
|
||||
const entries = list.filter((e) => !isKnownEntry(e) || !(tm && backedLine(e, tm) !== null));
|
||||
const why = tm ? 'no `Outside:` line of the brief equals its threat_model_ref, it is not a Voyage security finding, or it is malformed' : use.why;
|
||||
return { entries, why: entries.length > 0 ? why : null };
|
||||
// A malformed entry is checked as no finding, so it is never backed.
|
||||
const { unbacked, why } = splitBacked(list, reviewThreatModel(text, repo), (e) => (isKnownEntry(e) ? e : null));
|
||||
return { entries: unbacked, why };
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -148,8 +148,9 @@ import { splitSections, fenceTracker, extractPlanVersion, versionAtLeast } from
|
|||
import { normalizeSeverity, isSecurity } from '../review/plan-review-dedup.mjs';
|
||||
import { countRound, lineDiff, oneLine } from '../review/critic-diff.mjs';
|
||||
import { roundSummary, decideStop, describeStop, STOP_REASONS } from '../review/stop-rule.mjs';
|
||||
import { usableInRepo, repoRelativeBrief, backedLine } from '../review/threat-model.mjs';
|
||||
import { usableInRepo, repoRelativeBrief, splitBacked } from '../review/threat-model.mjs';
|
||||
import { resolveRepo } from '../verification/repo-tools.mjs';
|
||||
import { resolveRepo as resolveTopLevel } from '../util/approval-log.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
export const MAX_ROUNDS = 3;
|
||||
|
|
@ -410,19 +411,15 @@ export function recordVerdict(text, dedup, opts = {}) {
|
|||
// known_outside; a copy rule A carries from an earlier round stays counted
|
||||
// until a reviewer marks it resolved (decision 221).
|
||||
const use = opts.threatModel;
|
||||
const tm = use && use.usable === true ? use.threatModel : null;
|
||||
const backed = [];
|
||||
const counted = [];
|
||||
for (const f of dedup.findings) {
|
||||
const line = tm ? backedLine(f, tm) : null;
|
||||
if (line === null) { counted.push(f); continue; }
|
||||
// rule_keys kept, so the bare gate re-checks the entry with the same keys (keysOf).
|
||||
backed.push({
|
||||
id: f.id ?? null, rule_key: f.rule_key ?? null,
|
||||
...(Array.isArray(f.rule_keys) ? { rule_keys: f.rule_keys.filter(k => typeof k === 'string') } : {}),
|
||||
text: typeof f.text === 'string' ? f.text : '', line,
|
||||
});
|
||||
}
|
||||
const split = splitBacked(dedup.findings, use);
|
||||
const counted = split.unbacked;
|
||||
// rule_keys kept, so the bare gate re-checks the entry with the same keys
|
||||
// (keysOf); line = the `Outside:` text, which a backed threat_model_ref equals.
|
||||
const backed = split.backed.map(f => ({
|
||||
id: f.id ?? null, rule_key: f.rule_key ?? null,
|
||||
...(Array.isArray(f.rule_keys) ? { rule_keys: f.rule_keys.filter(k => typeof k === 'string') } : {}),
|
||||
text: typeof f.text === 'string' ? f.text : '', line: f.threat_model_ref,
|
||||
}));
|
||||
const backedIds = new Set(backed.map(e => e.id).filter(id => id !== null));
|
||||
const c = countRound({
|
||||
round,
|
||||
|
|
@ -506,32 +503,34 @@ export function acceptOpen(rawText, opts = {}) {
|
|||
const ex = extractVerdict(text);
|
||||
if (!ex.found || ex.error) return { code: 2, message: `no readable verdict: ${ex.error ?? 'no ## Critic Verdict'}` };
|
||||
const v = ex.verdict;
|
||||
const fired = v.stop_rule?.fired === true;
|
||||
if (v.plan_sha256 !== planHash(text)) {
|
||||
return {
|
||||
code: 1,
|
||||
message: v.stop_rule?.fired === true
|
||||
message: fired
|
||||
? 'the verdict is stale: the plan changed after the last round, and the stop rule fired, so there is no next round; restore the reviewed text to accept, or stop and take the direction to the operator'
|
||||
: 'the verdict is stale: the plan changed after the last round; run the next round instead',
|
||||
};
|
||||
}
|
||||
if (v.stop_rule?.fired === true) {
|
||||
// The verdict with `accepted_open` naming `list`, bound to this round and plan text.
|
||||
const accept = (list, under) => ({
|
||||
...v,
|
||||
accepted_open: {
|
||||
round: v.round,
|
||||
plan_sha256: v.plan_sha256,
|
||||
accepted_at: opts.now ?? new Date().toISOString(),
|
||||
...(under ? { under } : {}),
|
||||
findings: list.map(m => ({ id: m.id ?? null, text: m.text ?? '' })),
|
||||
},
|
||||
});
|
||||
if (fired) {
|
||||
const open = v.findings.filter(f => sevOf(f) !== 'minor');
|
||||
if (open.length === 0) return { code: 1, message: 'no BLOCKER or MAJOR is open; nothing to accept' };
|
||||
const security = open.filter(isSecurity);
|
||||
if (security.length > 0) {
|
||||
return { code: 1, message: `PLAN_GATE_SECURITY_OPEN: ${security.length} open security finding(s) count and are never accepted: ${security.map(s => s.text).join(' | ')}` };
|
||||
}
|
||||
const accepted = {
|
||||
...v,
|
||||
accepted_open: {
|
||||
round: v.round,
|
||||
plan_sha256: v.plan_sha256,
|
||||
accepted_at: opts.now ?? new Date().toISOString(),
|
||||
under: 'stop-rule',
|
||||
findings: open.map(m => ({ id: m.id ?? null, text: m.text ?? '' })),
|
||||
},
|
||||
};
|
||||
return { code: 0, text: placeSection(text, renderSection(accepted)), message: `${open.length} open finding(s) of round ${v.round} accepted as known issues under the stop rule` };
|
||||
return { code: 0, text: placeSection(text, renderSection(accept(open, 'stop-rule'))), message: `${open.length} open finding(s) of round ${v.round} accepted as known issues under the stop rule` };
|
||||
}
|
||||
if (v.findings.some(f => sevOf(f) !== 'major' && sevOf(f) !== 'minor')) return { code: 1, message: 'a BLOCKER is open; a blocker is never accepted' };
|
||||
const majors = v.findings.filter(f => sevOf(f) === 'major');
|
||||
|
|
@ -540,16 +539,7 @@ export function acceptOpen(rawText, opts = {}) {
|
|||
// (found by the independent review: acceptance at round 1 passed the gate).
|
||||
const left = v.max_rounds + (v.extra_rounds ?? 0) - v.round;
|
||||
if (left > 0) return { code: 1, message: `${left} critic round(s) left: revise the plan and run the next round; acceptance is only for after the last round` };
|
||||
const accepted = {
|
||||
...v,
|
||||
accepted_open: {
|
||||
round: v.round,
|
||||
plan_sha256: v.plan_sha256,
|
||||
accepted_at: opts.now ?? new Date().toISOString(),
|
||||
findings: majors.map(m => ({ id: m.id ?? null, text: m.text ?? '' })),
|
||||
},
|
||||
};
|
||||
return { code: 0, text: placeSection(text, renderSection(accepted)), message: `${majors.length} open MAJOR(s) of round ${v.round} accepted` };
|
||||
return { code: 0, text: placeSection(text, renderSection(accept(majors))), message: `${majors.length} open MAJOR(s) of round ${v.round} accepted` };
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -559,13 +549,8 @@ export function acceptOpen(rawText, opts = {}) {
|
|||
* @param {{usable: boolean, why: string|null, threatModel: object}} use - threat-model.mjs `usable`
|
||||
*/
|
||||
function checkKnownOutside(entries, use, reasons, warnings) {
|
||||
const tm = use?.usable === true ? use.threatModel : null;
|
||||
const backed = [];
|
||||
const unbacked = [];
|
||||
for (const e of entries) {
|
||||
const asFinding = { rule_key: e.rule_key, ...(Array.isArray(e.rule_keys) ? { rule_keys: e.rule_keys } : {}), threat_model_ref: e.line };
|
||||
(tm && backedLine(asFinding, tm) !== null ? backed : unbacked).push(e);
|
||||
}
|
||||
const { backed, unbacked, why } = splitBacked(entries, use,
|
||||
e => ({ rule_key: e.rule_key, ...(Array.isArray(e.rule_keys) ? { rule_keys: e.rule_keys } : {}), threat_model_ref: e.line }));
|
||||
if (backed.length > 0) {
|
||||
warnings.push({
|
||||
code: 'PLAN_GATE_KNOWN_OUTSIDE',
|
||||
|
|
@ -573,7 +558,6 @@ function checkKnownOutside(entries, use, reasons, warnings) {
|
|||
});
|
||||
}
|
||||
if (unbacked.length > 0) {
|
||||
const why = tm ? 'no `Outside:` line in the brief equals it, or it is not a Voyage security finding' : (use?.why ?? 'the threat model is not usable');
|
||||
reasons.push({
|
||||
code: 'PLAN_GATE_OUTSIDE_NOT_BACKED',
|
||||
message: `${unbacked.length} finding(s) recorded as outside the threat model are not backed by the brief now (${why}), so they count in full: ${unbacked.map(e => e.text).join(' | ')}`,
|
||||
|
|
@ -609,10 +593,11 @@ export function gatePlan(rawText, opts = {}) {
|
|||
} else if (ex.error) {
|
||||
reasons.push({ code: 'PLAN_GATE_VERDICT_MALFORMED', message: ex.error });
|
||||
} else {
|
||||
const stop = v.stop_rule?.fired === true ? v.stop_rule : null;
|
||||
if (v.plan_sha256 !== planHash(text)) {
|
||||
reasons.push({
|
||||
code: 'PLAN_GATE_VERDICT_STALE',
|
||||
message: v.stop_rule?.fired === true
|
||||
message: stop
|
||||
? `The plan changed after critic round ${v.round}, and the stop rule fired, so there is no next round; restore the reviewed text, or stop and take the direction to the operator.`
|
||||
: `The plan changed after critic round ${v.round}; run the next round.`,
|
||||
});
|
||||
|
|
@ -620,7 +605,6 @@ export function gatePlan(rawText, opts = {}) {
|
|||
const sev = f => normalizeSeverity(f.severity);
|
||||
const blockers = v.findings.filter(f => sev(f) !== 'major' && sev(f) !== 'minor');
|
||||
const majors = v.findings.filter(f => sev(f) === 'major');
|
||||
const stop = v.stop_rule?.fired === true ? v.stop_rule : null;
|
||||
if (stop) {
|
||||
// The stop rule replaces the open-blocker and open-major reasons: no new
|
||||
// round, the operator decides (decision 220). Covered = accepted under
|
||||
|
|
@ -844,9 +828,11 @@ if (isMain(import.meta.url)) {
|
|||
const dir = snapshotDir(planPath);
|
||||
const opts = { extraRound: args.includes('--extra-round'), reviewed, planPaths: planPathsOf(planPath) };
|
||||
if (briefArg !== undefined) {
|
||||
// Stored relative to the repo's top level, never absolute; read from there.
|
||||
opts.briefPath = repoRelativeBrief(briefArg);
|
||||
opts.threatModel = usableInRepo(opts.briefPath);
|
||||
// Stored relative to the repo's top level, never absolute; read from there
|
||||
// (one top-level lookup for both).
|
||||
const top = resolveTopLevel();
|
||||
opts.briefPath = repoRelativeBrief(briefArg, top);
|
||||
opts.threatModel = usableInRepo(opts.briefPath, top);
|
||||
}
|
||||
const lb = loadBase(text, dir);
|
||||
if (lb.verdict) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue