feat(review): a REVIEW.md policy caps and skips MINOR findings only
This commit is contained in:
parent
9e7e8d1516
commit
8d399fddfb
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
8 changed files with 1056 additions and 7 deletions
|
|
@ -74,6 +74,23 @@ export const REFUTING_REASONS = Object.freeze(new Set([
|
|||
'file-existence:refuted',
|
||||
]));
|
||||
|
||||
/**
|
||||
* Reasons that NOTE: a MINOR or SUGGESTION finding declined, listed under
|
||||
* `## Declined to Judge`, and not holding ALLOW back, as `outOfScopeNoted`
|
||||
* below is. `policy:skip` and `policy:cap` are written by the `REVIEW.md`
|
||||
* policy (lib/review/review-policy.mjs, run by review-finalize.mjs);
|
||||
* `instruction-out-of-set` by the instruction-file filter. None of them may
|
||||
* move a BLOCKER, a MAJOR or a security finding: the verdict gate
|
||||
* (verdict-gate.mjs `decideOpenFindings`) exits 2 on a `declined_to_judge`
|
||||
* entry of that kind under one of these reasons. `classifySuppression` does
|
||||
* not read this set: as a suppression reason each still fails closed.
|
||||
*/
|
||||
export const NOTED_REASONS = Object.freeze(new Set([
|
||||
'policy:skip',
|
||||
'policy:cap',
|
||||
'instruction-out-of-set',
|
||||
]));
|
||||
|
||||
/**
|
||||
* The reason vocabulary on the unverified side. Length is not in it: since
|
||||
* ordre 5 (P11) succinctness trims MINOR/SUGGESTION text and never removes a
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@
|
|||
// lib/review/fill-finding-ids.mjs, on both paths (review_version 1.2, fix
|
||||
// round 4.5b2 part B). Its passes run in the Architecture order (engine,
|
||||
// instruction, carry, pre-existing, policy); later steps insert theirs at
|
||||
// their places in PASSES. Today it runs:
|
||||
// their places in PASSES. Today it runs carry, pre-existing and policy:
|
||||
//
|
||||
// - carry (round 2 and later, `--previous <archive>`): `carry`
|
||||
// (lib/review/review-rounds.mjs) keeps every previous finding that this
|
||||
|
|
@ -26,6 +26,21 @@
|
|||
// never moves or lowers a finding. A finding with `carried_from` keeps its
|
||||
// line from the previous round's end, so it is never pre-existing
|
||||
// (`pre_existing: false`, not judged against this range).
|
||||
// - policy, always the last pass: `readPolicy` (lib/review/review-policy.mjs)
|
||||
// reads `REVIEW.md` at `scope_sha_start` from git and `applyPolicy` caps
|
||||
// and skips MINOR and SUGGESTION findings only (never a BLOCKER, a MAJOR
|
||||
// or a security finding). A moved finding leaves the JSON findings, the
|
||||
// frontmatter list, `counts` and its `### ` entry under
|
||||
// `## Findings (MINOR|SUGGESTION)`, and goes to `declined_to_judge` with
|
||||
// `policy:skip` or `policy:cap`; one whose entry is not found exactly
|
||||
// once there, or that an R-step names, stays reported (stricter). The
|
||||
// `## Declined to Judge` section gets one line per moved finding, by
|
||||
// title, and `Over the policy cap: N` when a cap is set; the frontmatter
|
||||
// gets `review_policy`, `review_policy_note` and `review_policy_sha256`
|
||||
// (`none` without the file); `pre_existing_counts` and `## Pre-existing`
|
||||
// keep only the findings still reported. A malformed, symlinked or changed file, or
|
||||
// a git failure, applies no policy (`review_policy: not-applied`, the
|
||||
// note says why). Coverage is not touched.
|
||||
//
|
||||
// Then, always last, the id recomputation: every id is computed again with
|
||||
// `computeFindingIds` (lib/parsers/finding-id.mjs) over the final array, and
|
||||
|
|
@ -64,6 +79,7 @@ import { FULL_SHA_RE } from './reviewed-commit.mjs';
|
|||
import { markFindings, scopeShas } from './pre-existing.mjs';
|
||||
import { ENGINES } from './engine-findings.mjs';
|
||||
import { carry, frontmatterField, MAX_REVIEW_ROUNDS, runId } from './review-rounds.mjs';
|
||||
import { applyPolicy, readPolicy } from './review-policy.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
const SEVERITIES = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
|
||||
|
|
@ -231,10 +247,140 @@ function passPreExisting(payload, { repo, text }) {
|
|||
};
|
||||
}
|
||||
|
||||
// ---- pass: policy ----------------------------------------------------------------
|
||||
|
||||
const MOVABLE_SECTION_RE = /^##\s+Findings\s*\((?:MINOR|SUGGESTION)\)\s*$/;
|
||||
const DECLINED_RE = /^##\s+Declined to Judge\b/;
|
||||
const OVER_CAP_RE = /^Over the policy cap:\s*\d+\s*$/;
|
||||
|
||||
/** A frontmatter string value: one line, no quote or backslash, at most 200 chars, double-quoted. */
|
||||
const quoted = (s) => `"${oneLine(s).replace(/["\\]/g, "'").slice(0, 200)}"`;
|
||||
|
||||
/**
|
||||
* Where the `### <id>` entry of `id` sits in `lines` (after the frontmatter):
|
||||
* {at, end} when there is exactly one and it stands under `## Findings (MINOR)`
|
||||
* or `## Findings (SUGGESTION)`, else null.
|
||||
*/
|
||||
function movableEntry(lines, fmEnd, id) {
|
||||
const heading = new RegExp(`^###\\s+${id}\\s*$`);
|
||||
const hits = [];
|
||||
for (let i = fmEnd + 1; i < lines.length; i++) if (heading.test(lines[i])) hits.push(i);
|
||||
if (hits.length !== 1) return null;
|
||||
const at = hits[0];
|
||||
let h = at - 1;
|
||||
while (h > fmEnd && !/^##\s/.test(lines[h])) h--;
|
||||
if (!MOVABLE_SECTION_RE.test(lines[h] ?? '')) return null;
|
||||
let end = at + 1;
|
||||
while (end < lines.length && !/^#{2,3}\s/.test(lines[end])) end++;
|
||||
return { at, end, section: h };
|
||||
}
|
||||
|
||||
/** `lines` with every `## Findings (MINOR|SUGGESTION)` section left without an entry saying `None.`. */
|
||||
function emptySectionsSayNone(lines) {
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
if (!MOVABLE_SECTION_RE.test(lines[i])) continue;
|
||||
const end = sectionEnd(lines, i);
|
||||
const body = lines.slice(i + 1, end);
|
||||
if (body.some((l) => /^###\s/.test(l))) continue;
|
||||
if (body.some((l) => l.trim() !== '')) continue;
|
||||
lines.splice(i + 1, end - i - 1, '', 'None.', '');
|
||||
}
|
||||
return lines;
|
||||
}
|
||||
|
||||
/** One `## Declined to Judge` line for a finding the policy moved. */
|
||||
function declinedLine(d) {
|
||||
return `- ${oneLine(d.file) || '(no file)'}:${d.line ?? '?'} ${oneLine(d.rule_key) || '(no rule_key)'} — ${d.reason} (${oneLine(d.title) || '(no title)'})`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Pass `policy`, always the last pass: the `REVIEW.md` policy read at
|
||||
* `scope_sha_start` (lib/review/review-policy.mjs). A finding it moves leaves
|
||||
* the JSON findings (so the frontmatter list and counts), and its `### `
|
||||
* entry; a finding whose entry is not found exactly once under
|
||||
* `## Findings (MINOR|SUGGESTION)`, or that an R-step names, stays reported.
|
||||
* The moved ones are listed under `## Declined to Judge`, with
|
||||
* `Over the policy cap: N` when a cap is set. Coverage is not touched.
|
||||
*/
|
||||
function passPolicy(payload, ctx) {
|
||||
const { start, end } = scopeShas(ctx.text);
|
||||
let policy = readPolicy(ctx.repo, start, end);
|
||||
if (policy.state === 'applied' && payload.declined_to_judge !== undefined && !Array.isArray(payload.declined_to_judge)) {
|
||||
policy = { ...policy, state: 'not-applied', reason: 'declined_to_judge in the JSON block is not an array', cap: null, skip: [] };
|
||||
}
|
||||
ctx.policyFields = {
|
||||
review_policy: policy.state,
|
||||
review_policy_note: quoted(policy.reason),
|
||||
review_policy_sha256: `"${policy.sha256}"`,
|
||||
};
|
||||
// The reason of each one is the one the whole set gives it: the cap counts over every finding.
|
||||
const all = applyPolicy(payload, policy);
|
||||
const reasonOf = new Map(all.moved.map((f, i) => [f, all.declined[i]]));
|
||||
const lines = ctx.head.split('\n');
|
||||
const fmEnd = frontmatterEnd(lines);
|
||||
const named = new Set(remediationSteps(lines).steps.map((st) => st.source).filter(Boolean));
|
||||
const take = [];
|
||||
for (const f of all.moved) {
|
||||
if (typeof f.id !== 'string' || !HEX40.test(f.id) || named.has(f.id) || ctx.added.has(f)) continue;
|
||||
if (movableEntry(lines, fmEnd, f.id)) take.push(f);
|
||||
}
|
||||
const declined = take.map((f) => reasonOf.get(f));
|
||||
const takeSet = new Set(take);
|
||||
payload.findings = payload.findings.filter((f) => !takeSet.has(f));
|
||||
let preSection = null;
|
||||
if (take.length > 0) {
|
||||
payload.declined_to_judge = [...(payload.declined_to_judge ?? []), ...declined];
|
||||
ctx.recount = true;
|
||||
// The pre-existing pass ran first: its counts and list keep only the findings still reported.
|
||||
if (isPlainObject(payload.pre_existing_counts)) {
|
||||
const counts = Object.fromEntries(SEVERITIES.map((sv) => [sv, 0]));
|
||||
for (const f of payload.findings) if (f.pre_existing === true && sevOf(f) in counts) counts[sevOf(f)] += 1;
|
||||
payload.pre_existing_counts = counts;
|
||||
preSection = preExistingSection(payload.findings, counts);
|
||||
}
|
||||
}
|
||||
// Every policy:cap entry the review holds, also those an earlier finalize moved.
|
||||
const overCap = (Array.isArray(payload.declined_to_judge) ? payload.declined_to_judge : [])
|
||||
.filter((d) => d !== null && typeof d === 'object' && d.reason === 'policy:cap').length;
|
||||
ctx.policy = { state: policy.state, moved: take.length, over_cap: overCap };
|
||||
const ids = take.map((f) => f.id);
|
||||
return {
|
||||
headEdit: (head) => {
|
||||
const pre = preSection === null ? head
|
||||
: replaceSection(head, /^##\s+Pre-existing\s*$/, preSection, [/^##\s+Resolved since round\b/, DECLINED_RE]);
|
||||
let out = pre.split('\n');
|
||||
const fe = frontmatterEnd(out);
|
||||
for (const id of ids) {
|
||||
const e = movableEntry(out, fe, id);
|
||||
if (!e) continue;
|
||||
out.splice(e.at, e.end - e.at);
|
||||
}
|
||||
out = emptySectionsSayNone(out);
|
||||
const block = declined.map(declinedLine);
|
||||
if (policy.state === 'applied' && policy.cap !== null) block.push(...(block.length > 0 ? [''] : []), `Over the policy cap: ${overCap}`);
|
||||
if (block.length === 0) return out.join('\n');
|
||||
const at = out.findIndex((l) => DECLINED_RE.test(l));
|
||||
if (at !== -1) {
|
||||
const stop = sectionEnd(out, at);
|
||||
for (let i = stop - 1; i > at; i--) {
|
||||
if (OVER_CAP_RE.test(out[i]) || (declined.length > 0 && out[i].trim() === 'None.')) {
|
||||
out.splice(i, 1);
|
||||
if (out[i] === '' && out[i - 1] === '') out.splice(i, 1);
|
||||
}
|
||||
}
|
||||
}
|
||||
out = appendToSection(out, DECLINED_RE, '## Declined to Judge', block,
|
||||
[/^##\s+Remediation Summary\b/, /^##\s+Remediation Steps\b/, /^##\s+Answers\s*$/]);
|
||||
return out.join('\n');
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
/** The passes, in the Architecture order; the id recomputation runs after them, always last. */
|
||||
export const PASSES = Object.freeze([
|
||||
{ name: 'carry', run: passCarry },
|
||||
{ name: 'pre-existing', run: passPreExisting },
|
||||
{ name: 'policy', run: passPolicy },
|
||||
]);
|
||||
|
||||
// ---- after the ids ---------------------------------------------------------------
|
||||
|
|
@ -425,6 +571,7 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
|
|||
let head = parts.head;
|
||||
const out = { passes: [] };
|
||||
for (const pass of PASSES) {
|
||||
ctx.head = head;
|
||||
const r = pass.run(payload, ctx);
|
||||
if (r.headEdit) head = r.headEdit(head);
|
||||
if (r.counts) out.pre_existing_counts = r.counts;
|
||||
|
|
@ -444,7 +591,7 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
|
|||
out.passes.push('ids');
|
||||
|
||||
head = placeEntries(head, payload.findings, ctx.added);
|
||||
if (ctx.carried) {
|
||||
if (ctx.carried || ctx.recount) {
|
||||
payload.counts = Object.fromEntries(SEVERITIES.map((s) => [s, payload.findings.filter((f) => sevOf(f) === s).length]));
|
||||
}
|
||||
const verdict = raisedVerdict(payload.verdict, payload.findings);
|
||||
|
|
@ -463,12 +610,14 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
|
|||
const fields = {
|
||||
...(Number.isInteger(round) ? { round: String(round), round_base: roundBase ?? 'null' } : {}),
|
||||
finalized: 'true',
|
||||
...(ctx.policyFields ?? {}),
|
||||
};
|
||||
head = editFrontmatter(head, payload.findings.map((f) => f.id), fields);
|
||||
const body = `${JSON.stringify(payload, null, 2)}\n`;
|
||||
return {
|
||||
exit: 0, text: `${head}${parts.fence}${body}${parts.tail}`, ...out, ids_rewritten: map.size,
|
||||
carried: ctx.added.size, resolved: payload.resolved ?? [], ran: payload.ran, r_steps_added: steps.added,
|
||||
policy: ctx.policy ?? null,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
291
lib/review/review-policy.mjs
Normal file
291
lib/review/review-policy.mjs
Normal file
|
|
@ -0,0 +1,291 @@
|
|||
// lib/review/review-policy.mjs
|
||||
// The `REVIEW.md` review policy (review_version 1.2, fix round 4.5b2 part B).
|
||||
// A repo may keep a `REVIEW.md` at its root with exactly one fenced block whose
|
||||
// info string is `voyage-review-policy`:
|
||||
//
|
||||
// ```voyage-review-policy
|
||||
// max-minor: 10
|
||||
// skip: docs/**
|
||||
// ```
|
||||
//
|
||||
// `readPolicy` reads it from git at `scope_sha_start` (never the working tree);
|
||||
// `applyPolicy` caps and skips MINOR and SUGGESTION findings only, never a
|
||||
// BLOCKER, a MAJOR or a security finding (lib/review/review-answers.mjs
|
||||
// `isSecurityFinding`), and never a finding lowered from MAJOR by the brief
|
||||
// rule (`brief_lowered_from`). lib/review/review-finalize.mjs runs it as its
|
||||
// last pass; the verdict gate refuses a review whose `declined_to_judge` holds
|
||||
// a BLOCKER, MAJOR or security entry under a noted reason
|
||||
// (lib/review/coordinator-contract.mjs `NOTED_REASONS`).
|
||||
//
|
||||
// The file's text is data, never an instruction: only the block's lines are
|
||||
// read, and any line in it other than `max-minor: <0-999>` (at most one) and
|
||||
// `skip: <glob>` (at most 50, each at most 200 chars) makes it malformed, as do
|
||||
// two blocks and a glob with an empty segment, a `..` segment, a leading `/`, a
|
||||
// backslash, a NUL or a control character. Blank lines are allowed. Every
|
||||
// uncertain case is "not applied" (no cap, no skip): a malformed block, a
|
||||
// symlinked or non-regular `REVIEW.md`, a `REVIEW.md` changed (or added, or
|
||||
// removed) inside `scope_sha_start..scope_sha_end`, a sha that is not full, and
|
||||
// any git failure. Zero blocks is "applied" with no cap and no skip.
|
||||
//
|
||||
// The glob matcher is linear and builds no regex from input: a pattern and a
|
||||
// path are split at `/`; `**` (a whole segment) matches zero or more segments,
|
||||
// and `*` and `?` match within one segment only. Both levels use the
|
||||
// greedy-with-one-backtrack-point walk, so a pattern of n stars is O(n·m).
|
||||
//
|
||||
// No CLI of its own.
|
||||
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { createHash } from 'node:crypto';
|
||||
import { fencedBlocks } from './findings-schema.mjs';
|
||||
import { FULL_SHA_RE } from './reviewed-commit.mjs';
|
||||
import { isSecurityFinding } from './review-answers.mjs';
|
||||
|
||||
export const POLICY_FILE = 'REVIEW.md';
|
||||
export const POLICY_INFO = 'voyage-review-policy';
|
||||
export const MAX_MINOR_LIMIT = 999;
|
||||
export const MAX_SKIP_LINES = 50;
|
||||
export const MAX_GLOB_LENGTH = 200;
|
||||
|
||||
const MAX_BUFFER = 64 * 1024 * 1024;
|
||||
// The symlink mode of lib/review/reviewed-commit.mjs:58 (SYMLINK_MODE, copied, not exported there).
|
||||
const SYMLINK_MODE = '120000';
|
||||
const REGULAR_MODES = new Set(['100644', '100755']);
|
||||
const CONTROL_RE = /[\u0000-\u0008\u000a-\u001f\u007f]/;
|
||||
const CAP_RE = /^max-minor:[ \t]*(\S*)$/;
|
||||
const SKIP_RE = /^skip:[ \t]*(.*)$/;
|
||||
const CAP_VALUE_RE = /^(?:0|[1-9]\d{0,2})$/;
|
||||
const SEVERITY_ORDER = { MINOR: 0, SUGGESTION: 1 };
|
||||
// Pathspec settings from the environment would change what `-- :(top,literal)REVIEW.md` names.
|
||||
const PATHSPEC_ENV = ['GIT_LITERAL_PATHSPECS', 'GIT_GLOB_PATHSPECS', 'GIT_NOGLOB_PATHSPECS', 'GIT_ICASE_PATHSPECS'];
|
||||
|
||||
// The argv git helper of lib/review/reviewed-commit.mjs:78-80 (copied, not exported there),
|
||||
// with the pathspec variables taken out of the child's environment.
|
||||
function git(repo, args, encoding = 'utf8') {
|
||||
const env = { ...process.env };
|
||||
for (const k of PATHSPEC_ENV) delete env[k];
|
||||
return spawnSync('git', args, { cwd: repo, encoding, maxBuffer: MAX_BUFFER, env });
|
||||
}
|
||||
|
||||
function gitNote(r) {
|
||||
return r.error?.code === 'ENOBUFS' ? 'output past the 64 MiB buffer'
|
||||
: (r.error?.message ?? String(r.stderr ?? '').trim().split('\n')[0]) || `exit ${r.status}`;
|
||||
}
|
||||
|
||||
// The blob reader of lib/review/reviewed-commit.mjs:150-159 (blobAt, copied, not exported there):
|
||||
// it returns the tree entry's mode and the blob as bytes, read by its object id.
|
||||
function blobAt(repo, rev, path) {
|
||||
const ls = git(repo, ['-c', 'core.quotePath=false', 'ls-tree', '--full-tree', '-z', rev, '--', path]);
|
||||
if (ls.status !== 0) return { ok: false, note: `git ls-tree ${rev} failed (${gitNote(ls)})` };
|
||||
const entries = ls.stdout.split('\0').filter(Boolean);
|
||||
const entry = entries.find((e) => e.slice(e.indexOf('\t') + 1) === path);
|
||||
if (!entry) return { ok: true, mode: null, bytes: null };
|
||||
const m = /^(\d{6}) ([a-z]+) ([0-9a-f]{40}|[0-9a-f]{64})\t/.exec(entry);
|
||||
if (!m) return { ok: false, note: `cannot read the tree entry of ${path}` };
|
||||
const [, mode, type, oid] = m;
|
||||
if (type !== 'blob') return { ok: true, mode, bytes: null };
|
||||
const blob = git(repo, ['cat-file', 'blob', oid], 'buffer');
|
||||
if (blob.status !== 0) return { ok: false, note: `git cat-file ${oid} failed (${gitNote(blob)})` };
|
||||
return { ok: true, mode, bytes: blob.stdout };
|
||||
}
|
||||
|
||||
const notApplied = (reason, sha256 = 'none') => ({ state: 'not-applied', reason, sha256, cap: null, skip: [] });
|
||||
|
||||
/** Why a skip glob is malformed, or null. */
|
||||
function badGlob(glob) {
|
||||
if (glob === '') return 'an empty skip glob';
|
||||
if (glob.length > MAX_GLOB_LENGTH) return `a skip glob over ${MAX_GLOB_LENGTH} chars`;
|
||||
if (glob.includes('\\')) return 'a skip glob with a backslash';
|
||||
if (/[\u0000-\u001f\u007f]/.test(glob)) return 'a skip glob with a control character';
|
||||
if (glob.startsWith('/')) return 'a skip glob with a leading /';
|
||||
const segs = glob.split('/');
|
||||
if (segs.some((s) => s === '')) return 'a skip glob with an empty segment';
|
||||
if (segs.some((s) => s === '..')) return 'a skip glob with a .. segment';
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The policy of a `REVIEW.md` text: the one `voyage-review-policy` block, parsed.
|
||||
* @param {string} text
|
||||
* @returns {{ ok: true, cap: number|null, skip: string[], blocks: number } | { ok: false, reason: string }}
|
||||
*/
|
||||
export function parsePolicy(text) {
|
||||
const blocks = fencedBlocks(String(text)).filter((b) => {
|
||||
const info = b.info.trim();
|
||||
return info === POLICY_INFO || info.split(/[ \t]/)[0] === POLICY_INFO;
|
||||
});
|
||||
if (blocks.length === 0) return { ok: true, cap: null, skip: [], blocks: 0 };
|
||||
const malformed = (why) => ({ ok: false, reason: `the ${POLICY_INFO} block in ${POLICY_FILE} is malformed: ${why}` });
|
||||
if (blocks.length > 1) return malformed(`${blocks.length} blocks, exactly one is read`);
|
||||
const [block] = blocks;
|
||||
if (block.info.trim() !== POLICY_INFO) return malformed('its info string is not exactly voyage-review-policy');
|
||||
if (!block.closed) return malformed('the block is never closed');
|
||||
let cap = null;
|
||||
const skip = [];
|
||||
const lines = block.body.split('\n');
|
||||
if (lines.at(-1) === '') lines.pop();
|
||||
for (const [i, raw] of lines.entries()) {
|
||||
const line = raw.replace(/\r$/, '');
|
||||
if (line.includes('\0') || CONTROL_RE.test(line.replace(/\t/g, ' '))) return malformed(`line ${i + 1} has a control character`);
|
||||
const l = line.trim();
|
||||
if (l === '') continue;
|
||||
const c = CAP_RE.exec(l);
|
||||
if (c) {
|
||||
if (cap !== null) return malformed('more than one max-minor line');
|
||||
if (!CAP_VALUE_RE.test(c[1])) return malformed(`max-minor must be an integer 0-${MAX_MINOR_LIMIT}`);
|
||||
cap = Number(c[1]);
|
||||
continue;
|
||||
}
|
||||
const s = SKIP_RE.exec(l);
|
||||
if (s) {
|
||||
const why = badGlob(s[1]);
|
||||
if (why) return malformed(why);
|
||||
skip.push(s[1]);
|
||||
if (skip.length > MAX_SKIP_LINES) return malformed(`more than ${MAX_SKIP_LINES} skip lines`);
|
||||
continue;
|
||||
}
|
||||
return malformed(`line ${i + 1} is neither max-minor: <n> nor skip: <glob>`);
|
||||
}
|
||||
return { ok: true, cap, skip, blocks: 1 };
|
||||
}
|
||||
|
||||
/**
|
||||
* Read the `REVIEW.md` policy at `sha` (the review's `scope_sha_start`) from git.
|
||||
* With `end` (`scope_sha_end`), a `REVIEW.md` changed inside `sha..end` is not
|
||||
* applied; `end` given but not a full sha is not applied either.
|
||||
* @param {string} repo
|
||||
* @param {string} sha
|
||||
* @param {string} [end]
|
||||
* @returns {{ state: 'absent'|'applied'|'not-applied', reason: string, sha256: string, cap: number|null, skip: string[] }}
|
||||
*/
|
||||
export function readPolicy(repo, sha, end) {
|
||||
if (typeof repo !== 'string' || repo === '') return notApplied('no repository to read REVIEW.md from');
|
||||
if (typeof sha !== 'string' || !FULL_SHA_RE.test(sha)) return notApplied('the review has no full scope_sha_start');
|
||||
if (end !== undefined) {
|
||||
if (typeof end !== 'string' || !FULL_SHA_RE.test(end)) return notApplied('the review has no full scope_sha_end');
|
||||
const d = git(repo, ['-c', 'core.quotePath=false', 'diff', '--name-only', '-z', '--no-renames', '--no-relative',
|
||||
'--no-ext-diff', '--no-textconv', sha, end, '--', `:(top,literal)${POLICY_FILE}`]);
|
||||
if (d.status !== 0) return notApplied(`REVIEW.md changed inside the reviewed range (git diff failed: ${gitNote(d)})`);
|
||||
if (d.stdout.split('\0').some(Boolean)) return notApplied('REVIEW.md changed inside the reviewed range');
|
||||
}
|
||||
const b = blobAt(repo, sha, POLICY_FILE);
|
||||
if (!b.ok) return notApplied(`cannot read REVIEW.md (${b.note})`);
|
||||
if (b.mode === null) return { state: 'absent', reason: 'no REVIEW.md at the repo root', sha256: 'none', cap: null, skip: [] };
|
||||
if (b.mode === SYMLINK_MODE) return notApplied('REVIEW.md is a symlink');
|
||||
if (!REGULAR_MODES.has(b.mode) || b.bytes === null) return notApplied('REVIEW.md is not a regular file');
|
||||
const sha256 = createHash('sha256').update(b.bytes).digest('hex');
|
||||
const p = parsePolicy(b.bytes.toString('utf8'));
|
||||
if (!p.ok) return notApplied(p.reason, sha256);
|
||||
const parts = [p.cap === null ? 'no cap' : `max-minor ${p.cap}`, `${p.skip.length} skip path${p.skip.length === 1 ? '' : 's'}`];
|
||||
return {
|
||||
state: 'applied', reason: p.blocks === 0 ? `no ${POLICY_INFO} block: no cap, no skip` : parts.join(', '),
|
||||
sha256, cap: p.cap, skip: p.skip,
|
||||
};
|
||||
}
|
||||
|
||||
// ---- the glob matcher ---------------------------------------------------------------
|
||||
|
||||
/** One segment against one glob segment (`*`, `?`, literal); linear, no regex. */
|
||||
function segmentMatch(pat, str) {
|
||||
let p = 0;
|
||||
let s = 0;
|
||||
let star = -1;
|
||||
let mark = 0;
|
||||
while (s < str.length) {
|
||||
// `*` first: a literal `*` in the path must not use the glob's `*` up as a literal.
|
||||
if (p < pat.length && pat[p] === '*') { star = p++; mark = s; continue; }
|
||||
if (p < pat.length && (pat[p] === '?' || pat[p] === str[s])) { p++; s++; continue; }
|
||||
if (star !== -1) { p = star + 1; s = ++mark; continue; }
|
||||
return false;
|
||||
}
|
||||
while (p < pat.length && pat[p] === '*') p++;
|
||||
return p === pat.length;
|
||||
}
|
||||
|
||||
/**
|
||||
* A repo path against a skip glob: `**` (a whole segment) crosses segments,
|
||||
* `*` and `?` stay in one. A path that is not plain (empty, a leading `/`, a
|
||||
* backslash, an empty or `.`/`..` segment) never matches.
|
||||
* @param {string} glob
|
||||
* @param {string} path
|
||||
* @returns {boolean}
|
||||
*/
|
||||
export function globMatch(glob, path) {
|
||||
if (typeof path !== 'string' || path === '' || path.startsWith('/') || path.includes('\\')) return false;
|
||||
const segs = path.split('/');
|
||||
if (segs.some((s) => s === '' || s === '.' || s === '..')) return false;
|
||||
const pats = String(glob).split('/');
|
||||
let p = 0;
|
||||
let s = 0;
|
||||
let star = -1;
|
||||
let mark = 0;
|
||||
while (s < segs.length) {
|
||||
if (p < pats.length && pats[p] !== '**' && segmentMatch(pats[p], segs[s])) { p++; s++; continue; }
|
||||
if (p < pats.length && pats[p] === '**') { star = p++; mark = s; continue; }
|
||||
if (star !== -1) { p = star + 1; s = ++mark; continue; }
|
||||
return false;
|
||||
}
|
||||
while (p < pats.length && pats[p] === '**') p++;
|
||||
return p === pats.length;
|
||||
}
|
||||
|
||||
// ---- applying the policy ------------------------------------------------------------
|
||||
|
||||
/** A finding the policy may move: MINOR or SUGGESTION, not security, not lowered from MAJOR. */
|
||||
function movable(f) {
|
||||
return f !== null && typeof f === 'object' && Object.hasOwn(SEVERITY_ORDER, f.severity)
|
||||
&& !isSecurityFinding(f) && f.brief_lowered_from === undefined;
|
||||
}
|
||||
|
||||
const cmp = (a, b) => (a < b ? -1 : a > b ? 1 : 0);
|
||||
|
||||
/** The cap's order: severity, file, line, rule_key (code-point order, so the same on every run). */
|
||||
function capOrder(a, b) {
|
||||
return SEVERITY_ORDER[a.f.severity] - SEVERITY_ORDER[b.f.severity]
|
||||
|| cmp(String(a.f.file ?? ''), String(b.f.file ?? ''))
|
||||
|| (Number(a.f.line) || 0) - (Number(b.f.line) || 0)
|
||||
|| cmp(String(a.f.rule_key ?? ''), String(b.f.rule_key ?? ''))
|
||||
|| a.i - b.i;
|
||||
}
|
||||
|
||||
/** A `declined_to_judge` entry in the coordinator's shape (coordinator-contract `runContract`). */
|
||||
function declinedEntry(f, reason) {
|
||||
return {
|
||||
file: f.file ?? null,
|
||||
line: f.line ?? null,
|
||||
rule_key: f.rule_key ?? null,
|
||||
severity: f.severity ?? null,
|
||||
title: f.title ?? null,
|
||||
...(f.engine !== undefined ? { engine: f.engine } : {}),
|
||||
reason,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply a policy to a payload's findings. Only an `applied` policy moves
|
||||
* anything: a movable finding on a skip path goes to `declined` with
|
||||
* `policy:skip`; of the rest, sorted by (severity, file, line, rule_key), those
|
||||
* past `cap` go with `policy:cap`. `kept` keeps the input order.
|
||||
* @param {{findings?: object[]}} payload
|
||||
* @param {{state: string, cap: number|null, skip: string[]}} policy
|
||||
* @returns {{ kept: object[], declined: object[], moved: object[] }}
|
||||
*/
|
||||
export function applyPolicy(payload, policy) {
|
||||
const findings = Array.isArray(payload?.findings) ? payload.findings : [];
|
||||
if (policy?.state !== 'applied') return { kept: [...findings], declined: [], moved: [] };
|
||||
const skip = Array.isArray(policy.skip) ? policy.skip : [];
|
||||
const cap = Number.isInteger(policy.cap) && policy.cap >= 0 ? policy.cap : null;
|
||||
const reasonOf = new Map();
|
||||
const candidates = [];
|
||||
findings.forEach((f, i) => {
|
||||
if (!movable(f)) return;
|
||||
if (skip.some((g) => globMatch(g, f.file))) reasonOf.set(f, 'policy:skip');
|
||||
else candidates.push({ f, i });
|
||||
});
|
||||
if (cap !== null) {
|
||||
for (const { f } of candidates.sort(capOrder).slice(cap)) reasonOf.set(f, 'policy:cap');
|
||||
}
|
||||
const kept = [];
|
||||
const moved = [];
|
||||
for (const f of findings) (reasonOf.has(f) ? moved : kept).push(f);
|
||||
return { kept, declined: moved.map((f) => declinedEntry(f, reasonOf.get(f))), moved };
|
||||
}
|
||||
|
|
@ -35,7 +35,13 @@
|
|||
// `finalized: true` while an archive `review-round-<run>-<n>.md` of its
|
||||
// own run (<run> = the first 12 hex of `scope_sha_start`) sits beside
|
||||
// it; the previous findings reach a later round only through
|
||||
// finalize's carry.
|
||||
// finalize's carry. A review whose `declined_to_judge` holds a
|
||||
// BLOCKER, a MAJOR or a security entry under a noted reason
|
||||
// (`policy:skip`, `policy:cap`, `instruction-out-of-set`;
|
||||
// coordinator-contract `NOTED_REASONS`) is exit 2 as well (reason
|
||||
// `a policy or filter moved a BLOCKER, MAJOR or security finding`;
|
||||
// `movedSerious`): a policy or filter only ever moves MINOR and
|
||||
// SUGGESTION findings.
|
||||
// Exit 2 is NEVER green.
|
||||
//
|
||||
// The pure `decideVerdict` stays text-only: it never reads the answers. The
|
||||
|
|
@ -60,11 +66,11 @@ import { spawnSync } from 'node:child_process';
|
|||
import { dirname } from 'node:path';
|
||||
import { validateReviewContent, VERDICT_VALUES } from '../validators/review-validator.mjs';
|
||||
import { fencedBlocks, isJsonBlock } from './findings-schema.mjs';
|
||||
import { illegalBriefLowering } from './coordinator-contract.mjs';
|
||||
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, reviewSubject } from './review-answers.mjs';
|
||||
import { answerRecords, archivedReader, decideAnswers, isSecurityFinding, reviewSubject } from './review-answers.mjs';
|
||||
import { preExistingCheck, scopeShas } from './pre-existing.mjs';
|
||||
import { frontmatterField, runId } from './review-rounds.mjs';
|
||||
|
||||
|
|
@ -193,6 +199,32 @@ function blockerSectionEntries(body) {
|
|||
}
|
||||
|
||||
const NOT_FINALIZED = 'review-not-measured: round 2+ review not finalized';
|
||||
const MOVED_SERIOUS = 'a policy or filter moved a BLOCKER, MAJOR or security finding';
|
||||
const NOTED_SEVERITIES = new Set(['MINOR', 'SUGGESTION']);
|
||||
|
||||
/** One field of a declined entry for the reason line: one line, at most 120 chars. */
|
||||
const field = (v) => (v === null || v === undefined ? null : String(v).replace(/\s+/g, ' ').slice(0, 120));
|
||||
|
||||
/**
|
||||
* The `declined_to_judge` entries a policy or filter may not have written: a
|
||||
* reason in `NOTED_REASONS` (coordinator-contract) on an entry that is not a
|
||||
* MINOR or SUGGESTION, or that is a security finding (review-answers
|
||||
* `isSecurityFinding`). An unreadable block gives none (decideVerdict and
|
||||
* decideAnswers judge it).
|
||||
* @param {string} text - review.md
|
||||
* @returns {Array<{severity, rule_key, file, line, reason}>}
|
||||
*/
|
||||
export function movedSerious(text) {
|
||||
const last = fencedBlocks(String(text ?? '')).at(-1);
|
||||
if (!last || !last.closed || !isJsonBlock(last)) return [];
|
||||
let payload;
|
||||
try { payload = JSON.parse(last.body); } catch { return []; }
|
||||
const declined = Array.isArray(payload?.declined_to_judge) ? payload.declined_to_judge : [];
|
||||
return declined
|
||||
.filter((d) => d !== null && typeof d === 'object' && NOTED_REASONS.has(d.reason)
|
||||
&& (!NOTED_SEVERITIES.has(String(d.severity ?? '').toUpperCase()) || isSecurityFinding(d)))
|
||||
.map((d) => ({ severity: field(d.severity), rule_key: field(d.rule_key), file: field(d.file), line: field(d.line), reason: d.reason }));
|
||||
}
|
||||
|
||||
/**
|
||||
* A round 2+ review that finalize did not finalize, or null. From round 2 the
|
||||
|
|
@ -230,7 +262,8 @@ export function unfinalizedRound(text, reviewPath) {
|
|||
* against its archive beside the review file (`archivedReader`); with no
|
||||
* review path no carried row answers. A finding with `carried_from` is never
|
||||
* pre-existing: it keeps its line from the previous round's end. A round 2+
|
||||
* review that was not finalized (`unfinalizedRound`) is exit 2 first.
|
||||
* review that was not finalized (`unfinalizedRound`) is exit 2 first, then a
|
||||
* BLOCKER, MAJOR or security finding moved by a policy or filter (`movedSerious`).
|
||||
* @param {string} text - review.md
|
||||
* @param {{ repo?: string, reviewPath: string }} opts
|
||||
* @returns {{ exit: 0|2, reason: string, unanswered: string[], fixed_pending: string[] }}
|
||||
|
|
@ -238,6 +271,15 @@ export function unfinalizedRound(text, reviewPath) {
|
|||
export function decideOpenFindings(text, { repo, reviewPath } = {}) {
|
||||
const notFinal = unfinalizedRound(text, reviewPath);
|
||||
if (notFinal) return { exit: EXIT.UNDECIDED, reason: notFinal, unanswered: [], fixed_pending: [] };
|
||||
const hidden = movedSerious(text);
|
||||
if (hidden.length > 0) {
|
||||
return {
|
||||
exit: EXIT.UNDECIDED,
|
||||
reason: `${MOVED_SERIOUS}: declined_to_judge ${hidden.map((d) => `${d.severity ?? '(no severity)'} ${d.rule_key ?? '(no rule_key)'} at ${d.file ?? '?'}:${d.line ?? '?'} (${d.reason})`).join(', ')}`,
|
||||
unanswered: [],
|
||||
fixed_pending: [],
|
||||
};
|
||||
}
|
||||
const { records, top, problem } = answerRecords(repo ?? (reviewPath ? dirname(reviewPath) : undefined));
|
||||
const subject = top && reviewPath ? reviewSubject(top, reviewPath) : undefined;
|
||||
// A carried `## Answers` row (Carried from review-round-<run>-<n>.md#<id>)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue