feat(review): the standard path puts back engine findings the coordinator dropped
This commit is contained in:
parent
8d399fddfb
commit
349a4fc789
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
6 changed files with 559 additions and 21 deletions
89
lib/review/engine-compare.mjs
Normal file
89
lib/review/engine-compare.mjs
Normal file
|
|
@ -0,0 +1,89 @@
|
|||
// lib/review/engine-compare.mjs
|
||||
// The standard-path put-back (fix round 4.5b2 part B, Step 9): on the prose
|
||||
// path, an engine finding at MAJOR or BLOCKER that the coordinator dropped is
|
||||
// put back at its own severity, the way the workflow path does
|
||||
// (scripts/trekreview-armB.workflow.mjs, operator decision 23).
|
||||
//
|
||||
// A finding is matched on its row: its (file, line, rule_key) triplet plus
|
||||
// its title, trimmed and in lower case. An engine finding's row is returned
|
||||
// when a finding of review.md carries its key, or the key of its title as the
|
||||
// coordinator trims it, so a trimmed title puts back no copy. Each row not
|
||||
// returned is put back once, its highest severity first (on a tie, the first
|
||||
// seen), with `engine` taken from the payload it came from (the `reviewer`
|
||||
// field mapped back through ENGINES, never the finding's own label) and no id
|
||||
// (lib/review/review-finalize.mjs recomputes every id). The verdict is only
|
||||
// raised: a BLOCKER gives BLOCK, a MAJOR at least WARN.
|
||||
//
|
||||
// Only a payload's `findings` are compared. An engine finding in
|
||||
// `out_of_scope` stays declined by the existing rule
|
||||
// (lib/review/coordinator-contract.mjs runContract, `engine:out-of-scope`).
|
||||
//
|
||||
// The row rule below is a lib copy of the armB workflow's (scripts cannot be
|
||||
// imported); tests/lib/engine-findings-compare.test.mjs pins both equal.
|
||||
|
||||
import { ENGINES } from './engine-findings.mjs';
|
||||
|
||||
// Copied from scripts/trekreview-armB.workflow.mjs:60-73 (rowOf, trimmedTitle).
|
||||
// The (file,line,rule_key) triplet a finding is deduped and matched on.
|
||||
const tripletOf = (f) => `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
|
||||
// A finding's title as the grouping compares it: trimmed, lower case.
|
||||
const normTitle = (f) => String(f.title).trim().toLowerCase();
|
||||
// The row a finding is matched to after the coordinator: its triplet plus its
|
||||
// `normTitle`. An untitled finding gets a key no titled row has.
|
||||
const rowOf = (f) => `${tripletOf(f)}\u0000${f.title === undefined ? '\u0001' : normTitle(f)}`;
|
||||
// The coordinator trims an over-long MINOR/SUGGESTION title as
|
||||
// lib/review/coordinator-contract.mjs judgeFilter does; its marker and limit
|
||||
// are inlined (scripts cannot import) and pinned equal by the workflow test.
|
||||
const TRIM_MARKER = ' […trimmed]';
|
||||
const JUDGE_TITLE_MAX = 100;
|
||||
// A title as that trim writes it: over JUDGE_TITLE_MAX characters, cut to
|
||||
// leave room for the marker, then the marker; any other title unchanged.
|
||||
const trimmedTitle = (t) => (t.length > JUDGE_TITLE_MAX ? t.slice(0, JUDGE_TITLE_MAX - TRIM_MARKER.length) + TRIM_MARKER : t);
|
||||
|
||||
export { tripletOf, normTitle, rowOf, trimmedTitle };
|
||||
|
||||
const PUT_BACK_SEVERITIES = ['BLOCKER', 'MAJOR'];
|
||||
const VERDICT_RANK = { ALLOW: 0, WARN: 1, BLOCK: 2 };
|
||||
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 findings at MAJOR or BLOCKER whose row no finding of `payload`
|
||||
* carries, as copies to append, and the verdict raised by them.
|
||||
* @param {{ verdict?: string, findings: object[] }} payload - review.md's trailing JSON block
|
||||
* @param {Array<{ reviewer: string, findings: object[], out_of_scope?: object[] }>} enginePayloads
|
||||
* @returns {{ added: object[], raisedVerdict: string|undefined }}
|
||||
*/
|
||||
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;
|
||||
if (!engine) continue;
|
||||
for (const f of p.findings) {
|
||||
if (!isPlainObject(f) || !PUT_BACK_SEVERITIES.includes(f.severity)) continue;
|
||||
candidates.push({ f, engine, rank: PUT_BACK_SEVERITIES.indexOf(f.severity) });
|
||||
}
|
||||
}
|
||||
// Highest severity first; Array.prototype.sort is stable, so a tie keeps the first seen.
|
||||
candidates.sort((a, b) => a.rank - b.rank);
|
||||
const added = [];
|
||||
for (const { f, engine } of candidates) {
|
||||
if (rowReturned(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);
|
||||
rows.add(rowOf(copy));
|
||||
}
|
||||
const current = payload?.verdict;
|
||||
let raised = current;
|
||||
if (Object.hasOwn(VERDICT_RANK, current)) {
|
||||
const need = added.some((f) => f.severity === 'BLOCKER') ? 'BLOCK' : added.length > 0 ? 'WARN' : 'ALLOW';
|
||||
if (VERDICT_RANK[need] > VERDICT_RANK[current]) raised = need;
|
||||
}
|
||||
return { added, raisedVerdict: raised };
|
||||
}
|
||||
|
|
@ -3,8 +3,21 @@
|
|||
// 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 carry, pre-existing and policy:
|
||||
// their places in PASSES. Today it runs engine, carry, pre-existing and policy:
|
||||
//
|
||||
// - engine, always the first pass: `putBack` (lib/review/engine-compare.mjs)
|
||||
// compares the engine payloads Phase 4.7 saved in the project folder
|
||||
// (each regular `.review-engine-payload-*.json` whose top-level `reviewer`
|
||||
// maps back through `ENGINES`, read with readdirSync) with the review's
|
||||
// findings. Each engine finding at MAJOR or BLOCKER whose row no finding
|
||||
// carries (its own row, or the row of its trimmed title) is appended at
|
||||
// its own severity with `engine` kept and no id (the id recomputation
|
||||
// gives it its computed id). It gets a `### ` entry under its
|
||||
// `## Findings (<SEV>)` section with the line
|
||||
// `**Source:** engine {name}, put back by the standard-path comparison`,
|
||||
// the frontmatter list, counts, a verdict that is only raised, and, as a
|
||||
// BLOCKER, its `### Step R<n>`. An engine finding in `out_of_scope` is
|
||||
// not compared (it stays declined, `engine:out-of-scope`).
|
||||
// - carry (round 2 and later, `--previous <archive>`): `carry`
|
||||
// (lib/review/review-rounds.mjs) keeps every previous finding that this
|
||||
// round gives no positive evidence of being resolved, with `carried_from`;
|
||||
|
|
@ -80,6 +93,7 @@ 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 { putBack } from './engine-compare.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
const SEVERITIES = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
|
||||
|
|
@ -185,6 +199,27 @@ function appendToSection(lines, headingRe, heading, block, before) {
|
|||
return lines;
|
||||
}
|
||||
|
||||
// ---- pass: engine ------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Pass `engine`, always the first pass: the engine findings at MAJOR or
|
||||
* BLOCKER the coordinator dropped are put back (lib/review/engine-compare.mjs).
|
||||
*/
|
||||
function passEngine(payload, ctx) {
|
||||
const r = putBack(payload, ctx.evidence.payloads ?? []);
|
||||
for (const f of r.added) {
|
||||
payload.findings.push(f);
|
||||
ctx.added.add(f);
|
||||
ctx.putBack.add(f);
|
||||
}
|
||||
ctx.engine = { put_back: r.added.length };
|
||||
if (r.added.length === 0) return {};
|
||||
ctx.recount = true;
|
||||
if (r.raisedVerdict === payload.verdict) return {};
|
||||
payload.verdict = r.raisedVerdict;
|
||||
return { headEdit: (head) => withVerdict(head, r.raisedVerdict) };
|
||||
}
|
||||
|
||||
// ---- pass: carry -------------------------------------------------------------------
|
||||
|
||||
const LATER_SECTIONS = [/^##\s+Suppressed Findings\b/, /^##\s+Pre-existing\s*$/, /^##\s+Resolved since round\b/,
|
||||
|
|
@ -201,9 +236,20 @@ function resolvedSection(n, list) {
|
|||
function passCarry(payload, ctx) {
|
||||
if (!ctx.previous) return {};
|
||||
const before = payload.findings.length;
|
||||
const r = carry(ctx.previous.findings, payload.findings, {
|
||||
const current = payload.findings;
|
||||
const r = carry(ctx.previous.findings, current, {
|
||||
marks: ctx.evidence.marks, ran: ctx.evidence.ran, previousRan: ctx.previous.ran,
|
||||
});
|
||||
// carry returns a copy of each current finding at its index: a finding an
|
||||
// earlier pass added (an engine put-back) stays added under its copy.
|
||||
for (let i = 0; i < before; i++) {
|
||||
for (const set of [ctx.added, ctx.putBack]) {
|
||||
if (set.has(current[i])) {
|
||||
set.delete(current[i]);
|
||||
set.add(r.findings[i]);
|
||||
}
|
||||
}
|
||||
}
|
||||
payload.findings = r.findings;
|
||||
for (const f of r.findings.slice(before)) ctx.added.add(f);
|
||||
payload.resolved = r.resolved;
|
||||
|
|
@ -378,6 +424,7 @@ function passPolicy(payload, ctx) {
|
|||
|
||||
/** The passes, in the Architecture order; the id recomputation runs after them, always last. */
|
||||
export const PASSES = Object.freeze([
|
||||
{ name: 'engine', run: passEngine },
|
||||
{ name: 'carry', run: passCarry },
|
||||
{ name: 'pre-existing', run: passPreExisting },
|
||||
{ name: 'policy', run: passPolicy },
|
||||
|
|
@ -433,9 +480,14 @@ function editFrontmatter(head, ids, fields) {
|
|||
return [lines[0], ...out, ...lines.slice(fmEnd)].join('\n');
|
||||
}
|
||||
|
||||
/** A body entry for a finding finalize added, in the coordinator's form. */
|
||||
function entryLines(f) {
|
||||
const lines = [`### ${f.id}`, '', `- file: ${oneLine(f.file)}`, `- line: ${f.line}`, `- rule_key: ${oneLine(f.rule_key)}`];
|
||||
/**
|
||||
* A body entry for a finding finalize added, in the coordinator's form; an
|
||||
* engine put-back names its source first.
|
||||
*/
|
||||
function entryLines(f, putBackSet) {
|
||||
const lines = [`### ${f.id}`, ''];
|
||||
if (putBackSet.has(f)) lines.push(`**Source:** engine ${oneLine(f.engine)}, put back by the standard-path comparison`, '');
|
||||
lines.push(`- file: ${oneLine(f.file)}`, `- line: ${f.line}`, `- rule_key: ${oneLine(f.rule_key)}`);
|
||||
for (const k of ['brief_ref', 'title', 'detail', 'recommended_action', 'engine', 'carried_from']) {
|
||||
if (f[k] !== undefined && f[k] !== null && oneLine(f[k]) !== '') lines.push(`- ${k}: ${oneLine(f[k])}`);
|
||||
}
|
||||
|
|
@ -453,14 +505,14 @@ 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) {
|
||||
function placeEntries(head, findings, added, putBackSet) {
|
||||
let lines = head.split('\n');
|
||||
const fmEnd = frontmatterEnd(lines);
|
||||
for (const f of findings) {
|
||||
const sev = sevOf(f);
|
||||
if (!SEVERITIES.includes(sev) || typeof f.id !== 'string') continue;
|
||||
if (added.has(f)) {
|
||||
lines = appendToSection(lines, findingsHeading(sev), `## Findings (${sev})`, entryLines(f), findingsBefore(sev));
|
||||
lines = appendToSection(lines, findingsHeading(sev), `## Findings (${sev})`, entryLines(f, putBackSet), findingsBefore(sev));
|
||||
continue;
|
||||
}
|
||||
const at = lines.findIndex((l, i) => i > fmEnd && new RegExp(`^###\\s+${f.id}\\s*$`).test(l));
|
||||
|
|
@ -566,8 +618,8 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
|
|||
const dup = before.find((id, i) => before.indexOf(id) !== i);
|
||||
if (dup !== undefined) return { exit: 1, reason: `two findings carry the id ${JSON.stringify(dup)}` };
|
||||
|
||||
const ev = evidence ?? { ran: { engines: [], reviewers: [] }, marks: [] };
|
||||
const ctx = { repo, text, round, previous, evidence: ev, added: new Set(), carried: false };
|
||||
const ev = evidence ?? { ran: { engines: [], reviewers: [] }, marks: [], payloads: [] };
|
||||
const ctx = { repo, text, round, previous, evidence: ev, added: new Set(), putBack: new Set(), carried: false };
|
||||
let head = parts.head;
|
||||
const out = { passes: [] };
|
||||
for (const pass of PASSES) {
|
||||
|
|
@ -590,7 +642,7 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
|
|||
head = rewriteIds(head, map);
|
||||
out.passes.push('ids');
|
||||
|
||||
head = placeEntries(head, payload.findings, ctx.added);
|
||||
head = placeEntries(head, payload.findings, ctx.added, ctx.putBack);
|
||||
if (ctx.carried || ctx.recount) {
|
||||
payload.counts = Object.fromEntries(SEVERITIES.map((s) => [s, payload.findings.filter((f) => sevOf(f) === s).length]));
|
||||
}
|
||||
|
|
@ -617,22 +669,24 @@ export function finalizeReview(text, { repo, round, roundBase = null, previous =
|
|||
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,
|
||||
policy: ctx.policy ?? null, put_back: ctx.engine?.put_back ?? 0,
|
||||
};
|
||||
}
|
||||
|
||||
// ---- evidence and the previous review ------------------------------------------------
|
||||
|
||||
/**
|
||||
* What ran this round, from the project folder (positive evidence only), and
|
||||
* the reviewers' marks of previous findings.
|
||||
* What ran this round, from the project folder (positive evidence only), the
|
||||
* reviewers' marks of previous findings, and the engine payloads that prove
|
||||
* an engine ran (for the engine pass).
|
||||
* @param {string} dir
|
||||
* @returns {{ ran: {engines: string[], reviewers: string[]}, marks: Array<{id, resolved, reviewer}> }}
|
||||
* @returns {{ ran: {engines: string[], reviewers: string[]}, marks: Array<{id, resolved, reviewer}>, payloads: object[] }}
|
||||
*/
|
||||
export function roundEvidence(dir) {
|
||||
const engines = new Set();
|
||||
const reviewers = new Set();
|
||||
const marks = [];
|
||||
const payloads = [];
|
||||
let names = [];
|
||||
try { names = readdirSync(dir).sort(); } catch { names = []; }
|
||||
for (const name of names) {
|
||||
|
|
@ -643,7 +697,10 @@ export function roundEvidence(dir) {
|
|||
try { payload = JSON.parse(readFileSync(p, 'utf8')); } catch { continue; }
|
||||
const engine = isPlainObject(payload) && typeof payload.reviewer === 'string' && Object.hasOwn(ENGINE_OF_REVIEWER, payload.reviewer)
|
||||
? ENGINE_OF_REVIEWER[payload.reviewer] : null;
|
||||
if (engine && Array.isArray(payload.findings)) engines.add(engine);
|
||||
if (engine && Array.isArray(payload.findings)) {
|
||||
engines.add(engine);
|
||||
payloads.push(payload);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const m = OUT_RE.exec(name);
|
||||
|
|
@ -659,7 +716,7 @@ export function roundEvidence(dir) {
|
|||
}
|
||||
}
|
||||
}
|
||||
return { ran: { engines: [...engines].sort(), reviewers: [...reviewers].sort() }, marks };
|
||||
return { ran: { engines: [...engines].sort(), reviewers: [...reviewers].sort() }, marks, payloads };
|
||||
}
|
||||
|
||||
/** A `ran` object of the 1.2 shape, else null. */
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue