fix(review): the review's fix round - engines' key in rule_key, --brief only with --record, the gate's threat model from the review's work tree
The one narrow fix round after /code-review high on E (three findings of new classes; the between-rounds findings are known issues, decision 226): - threat-model.mjs isOwnSecurityFinding: the engines' key in rule_key is never Voyage's own, also when rule_keys names a key of its own (keysOf reads rule_keys alone when it is a list, so an entry with rule_key SECURITY_REVIEW_VULNERABILITY and rule_keys [SECURITY_RISK] could be backed, against decision 218). - plan-gate.mjs: --brief without --record is a usage error (exit 2); the bare gate and --accept-open read the verdict's brief_path, and a --brief there was silently ignored. - verdict-gate.mjs: the known_outside check reads the threat model from the answers' repo (--repo, else the review file's own work tree); with no work tree nothing is backed. A review outside any work tree, gated from another repo, was backed by that repo's brief (ALLOW). Each with a test seen failing on an assertion first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
f820081bdc
commit
d402b0088b
6 changed files with 73 additions and 5 deletions
|
|
@ -158,7 +158,9 @@ 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;
|
||||
if (keysOf(f).some((k) => ENGINE_SECURITY_KEYS.has(k.trim().toUpperCase()))) return false;
|
||||
// The engines' key in rule_key or in rule_keys: keysOf reads rule_keys alone when it is a list.
|
||||
const named = [f.rule_key, ...(Array.isArray(f.rule_keys) ? f.rule_keys : [])].filter((k) => typeof k === 'string');
|
||||
if (named.some((k) => ENGINE_SECURITY_KEYS.has(k.trim().toUpperCase()))) return false;
|
||||
return isSecurity(f);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -87,7 +87,7 @@ import { illegalBriefLowering, NOTED_REASONS } from './coordinator-contract.mjs'
|
|||
import { reviewedSha, checkReviewedCommit } from './reviewed-commit.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
import { resolveRepo } from '../util/approval-log.mjs';
|
||||
import { answerRecords, archivedReader, decideAnswers, isSecurityFinding, reviewSubject } from './review-answers.mjs';
|
||||
import { answerLogRepo, answerRecords, archivedReader, decideAnswers, isSecurityFinding, reviewSubject } from './review-answers.mjs';
|
||||
import { preExistingCheck, scopeShas } from './pre-existing.mjs';
|
||||
import { frontmatterField, runId } from './review-rounds.mjs';
|
||||
// No cycle: neither module, nor any module they import, imports this gate.
|
||||
|
|
@ -324,7 +324,8 @@ export function droppedEngineFindings(text, reviewPath) {
|
|||
* `Outside:` line gone, or a threat model that is not usable. An unreadable
|
||||
* trailing block gives none (decideVerdict judges it).
|
||||
* @param {string} text - review.md
|
||||
* @param {string|undefined} repo - the repo's top level
|
||||
* @param {string|null|undefined} repo - the repo's top level; none (no work
|
||||
* tree for the review) backs nothing, never the working directory's repo
|
||||
* @returns {{entries: Array<*>, why: string|null}}
|
||||
*/
|
||||
export function notBackedOutside(text, repo) {
|
||||
|
|
@ -333,8 +334,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 = repo ? reviewThreatModel(text, repo) : { usable: false, why: 'no git work tree for the review', threatModel: 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));
|
||||
const { unbacked, why } = splitBacked(list, use, (e) => (isKnownEntry(e) ? e : null));
|
||||
return { entries: unbacked, why };
|
||||
}
|
||||
|
||||
|
|
@ -371,7 +373,9 @@ export function decideOpenFindings(text, { repo, reviewPath } = {}) {
|
|||
fixed_pending: [],
|
||||
};
|
||||
}
|
||||
const outside = notBackedOutside(text, repo);
|
||||
// The threat model is read from the answers' repo: --repo, else the review
|
||||
// file's own work tree, never the working directory's (none: nothing is backed).
|
||||
const outside = notBackedOutside(text, repo ?? (reviewPath ? answerLogRepo(dirname(reviewPath)) : null));
|
||||
if (outside.entries.length > 0) {
|
||||
const what = (e) => (e !== null && typeof e === 'object'
|
||||
? `${field(e.title) ?? '(no title)'} at ${field(e.file) ?? '?'}:${field(e.line) ?? '?'}`
|
||||
|
|
|
|||
|
|
@ -787,6 +787,12 @@ if (isMain(import.meta.url)) {
|
|||
out({ code: 2, status: 'error', message: unknown.length ? `unknown argument: ${unknown.join(' ')}` : 'bad arguments' });
|
||||
process.exit(2);
|
||||
}
|
||||
if (bIdx >= 0 && !args.includes('--record')) {
|
||||
// The bare gate and --accept-open read the verdict's stored brief_path; a --brief there would be ignored.
|
||||
process.stderr.write(`${usage}\n`);
|
||||
out({ code: 2, status: 'error', message: "--brief is read only with --record; the gate reads the verdict's brief_path" });
|
||||
process.exit(2);
|
||||
}
|
||||
let text;
|
||||
try {
|
||||
if (!existsSync(planPath)) throw new Error('file not found');
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue