fix(review): the workflow floor and put-back match each finding to its own row
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
a5fcb5e8b4
commit
773aa44dc9
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 194 additions and 30 deletions
|
|
@ -56,6 +56,10 @@ const sevRank = (s) => (SEVERITIES.includes(s) ? SEVERITIES.indexOf(s) : SEVERIT
|
|||
const tierOf = (key) => (typeof key === 'string' && Object.prototype.hasOwnProperty.call(RULE_TIERS, key) ? RULE_TIERS[key] : null);
|
||||
// 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}`;
|
||||
// The row a finding is matched to after the coordinator: its triplet plus its
|
||||
// title, normalized as `sameTitle` below reads it (trimmed, lower case). An
|
||||
// untitled finding gets a key no titled row has.
|
||||
const rowOf = (f) => `${tripletOf(f)}\u0000${f.title === undefined ? '\u0001' : String(f.title).trim().toLowerCase()}`;
|
||||
|
||||
// Findings schema — mirrors lib/review/findings-schema.mjs (NW1). The enum on
|
||||
// rule_key enforces the catalogue at the StructuredOutput layer (stronger than
|
||||
|
|
@ -144,7 +148,8 @@ const sameTitle = (a, b) => a.title === undefined || b.title === undefined ||
|
|||
// findings in canonical order, the untitled findings last), so a full tie on
|
||||
// the survivor never follows arrival order either; a triplet's groups keep the
|
||||
// order of their earliest input index. With `perSource: false` everything on a
|
||||
// triplet is one group, as before.
|
||||
// triplet is one group, as before. A `rows` Map, when given, is filled with
|
||||
// each row written -> the findings of its group (the put-back reads it).
|
||||
const sourceOf = new Map();
|
||||
const cmpStr = (a, b) => (a < b ? -1 : a > b ? 1 : 0);
|
||||
function groupTriplet(entries) {
|
||||
|
|
@ -180,7 +185,7 @@ function groupTriplet(entries) {
|
|||
.sort((a, b) => a.first - b.first)
|
||||
.map((g) => g.members);
|
||||
}
|
||||
function dedupByTriplet(findings, { perSource = true } = {}) {
|
||||
function dedupByTriplet(findings, { perSource = true, rows = null } = {}) {
|
||||
const byTriplet = new Map();
|
||||
findings.forEach((f, i) => {
|
||||
const key = tripletOf(f);
|
||||
|
|
@ -213,7 +218,9 @@ function dedupByTriplet(findings, { perSource = true } = {}) {
|
|||
for (const f of group) {
|
||||
if (sevRank(f.severity) < sevRank(severity)) severity = f.severity;
|
||||
}
|
||||
out.push(severity === survivor.severity ? survivor : { ...survivor, severity });
|
||||
const row = severity === survivor.severity ? survivor : { ...survivor, severity };
|
||||
if (rows) rows.set(row, group);
|
||||
out.push(row);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
|
@ -315,7 +322,8 @@ const allFindings = [
|
|||
...enginePayloads.flatMap((p) => (Array.isArray(p?.findings) ? p.findings : [])
|
||||
.map((f) => tagged(withEngine(f, engineOf(p?.reviewer)), `engine:${p?.reviewer}`))),
|
||||
];
|
||||
const merged = dedupByTriplet(allFindings);
|
||||
const rowMembers = new Map();
|
||||
const merged = dedupByTriplet(allFindings, { rows: rowMembers });
|
||||
log(`Arm B: ${reviewerResults.length} reviewer(s); ${allFindings.length} raw → ${merged.length} after triplet-dedup`);
|
||||
|
||||
phase('Coordinate');
|
||||
|
|
@ -337,28 +345,51 @@ if (coordinatorMissing) missingReviewers += 1;
|
|||
// coordinator returned, so an LLM-written ALLOW over a BLOCKER cannot pass.
|
||||
const returned = coordinatorMissing ? merged : coord.findings;
|
||||
// The floor reads two inputs, never the labels the coordinator wrote:
|
||||
// - `merged` (the coordinator's input, same file/line/rule_key) for each
|
||||
// returned finding's counterpart: (a) an engine counterpart raises it to the
|
||||
// - `merged` (the coordinator's input, one finding per row) for each returned
|
||||
// finding's counterpart, its own row: the row with its `rowOf` key (triplet
|
||||
// plus normalized title); with no title match, the only row on its triplet,
|
||||
// else the strictest row there (highest tier, then highest severity; the
|
||||
// first in `merged` on a tie), never a lower one. `engine` is taken from
|
||||
// that row only. (a) an engine counterpart raises it to the
|
||||
// engine's severity, unless the one legal lowering; (b) a reviewer
|
||||
// counterpart, labelled or not, raises it to the lower of the counterpart's
|
||||
// asserted severity and its catalogue tier (the coordinator may reset a
|
||||
// finding to its tier, never below; for an unknown key the bound is the
|
||||
// counterpart's severity), and a labelled finding is raised as before.
|
||||
// - `allFindings` (before dedup, so an engine finding a reviewer finding
|
||||
// shadowed in its triplet counts too) for the put-back: (c) every engine
|
||||
// finding with no returned finding at its exact triplet is appended at its
|
||||
// own severity, one copy per triplet (the higher severity, else the first
|
||||
// seen) (operator decision 23, review ab1b5d9f). So the coordinator's drop,
|
||||
// move or relabel of an engine finding no longer lowers the verdict.
|
||||
// - the rows' members (before dedup, so an engine finding a reviewer finding
|
||||
// shadowed in its row counts too) for the put-back: (c) each engine finding
|
||||
// in a row whose `rowOf` key no returned finding carries is
|
||||
// put back at its own severity, one copy per row (the higher severity, else
|
||||
// the first seen) (operator decision 23, review ab1b5d9f). So the coordinator's drop,
|
||||
// move, rename or relabel of an engine finding no longer lowers the verdict
|
||||
// (a renamed row is put back beside the renamed finding: a duplicate, the
|
||||
// stricter side).
|
||||
// The raises and put-backs are written into `returned`, the marks kept, so the
|
||||
// rendered review.md carries them.
|
||||
// A coordinator that failed (no verdict and findings) is not trusted with a
|
||||
// partial list: `returned` is then every merged finding, each first raised to
|
||||
// its catalogue tier when the key is known (never lowered), as the default
|
||||
// path's Pass 3 would reset it; that raise stands after `raiseTo` is declared.
|
||||
const counterparts = new Map(merged
|
||||
.filter((m) => m && typeof m === 'object')
|
||||
.map((m) => [tripletOf(m), m]));
|
||||
const rowsOn = (map, key, m) => {
|
||||
if (!map.has(key)) map.set(key, []);
|
||||
map.get(key).push(m);
|
||||
};
|
||||
const counterparts = new Map();
|
||||
const rowsOnTriplet = new Map();
|
||||
for (const m of merged) {
|
||||
if (!m || typeof m !== 'object') continue;
|
||||
rowsOn(counterparts, rowOf(m), m);
|
||||
rowsOn(rowsOnTriplet, tripletOf(m), m);
|
||||
}
|
||||
const rowTier = (m) => sevRank(tierOf(m.rule_key) ?? m.severity);
|
||||
const strictest = (rows) => rows.reduce((best, m) => (rowTier(m) < rowTier(best) ||
|
||||
(rowTier(m) === rowTier(best) && sevRank(m.severity) < sevRank(best.severity)) ? m : best));
|
||||
const counterpartOf = (f) => {
|
||||
const own = counterparts.get(rowOf(f));
|
||||
if (own) return strictest(own);
|
||||
const onTriplet = rowsOnTriplet.get(tripletOf(f));
|
||||
return onTriplet ? strictest(onTriplet) : null;
|
||||
};
|
||||
const present = (f, field) => f[field] !== undefined && f[field] !== null;
|
||||
const raiseTo = (f, sev) => { if (sevRank(sev) < sevRank(f.severity)) f.severity = sev; };
|
||||
const lowerOf = (s1, s2) => (sevRank(s1) >= sevRank(s2) ? s1 : s2);
|
||||
|
|
@ -371,7 +402,7 @@ if (coordinatorMissing) {
|
|||
}
|
||||
for (const f of returned) {
|
||||
if (!f || typeof f !== 'object') continue;
|
||||
const c = counterparts.get(tripletOf(f));
|
||||
const c = counterpartOf(f);
|
||||
const labelled = present(f, 'engine') || present(f, 'contradicts_brief') || present(f, 'brief_lowered_from');
|
||||
if (c && c.engine) {
|
||||
// (a) an engine counterpart: raised to its severity, unless the one legal lowering.
|
||||
|
|
@ -395,11 +426,14 @@ for (const f of returned) {
|
|||
}
|
||||
}
|
||||
// (c) the put-back (decision 23).
|
||||
const returnedTriplets = new Set(returned.filter((f) => f && typeof f === 'object').map(tripletOf));
|
||||
// Engine findings only, so dedupByTriplet keeps the higher severity, else the
|
||||
// first seen: one copy per triplet (decision 23), so not split per source here.
|
||||
const putBack = dedupByTriplet(allFindings.filter((f) =>
|
||||
f && typeof f === 'object' && f.engine && !returnedTriplets.has(tripletOf(f))), { perSource: false });
|
||||
const returnedRows = new Set(returned.filter((f) => f && typeof f === 'object').map(rowOf));
|
||||
// Each row not returned: its engine findings only, so dedupByTriplet keeps the
|
||||
// higher severity, else the first seen: one copy per row, so not split per
|
||||
// source here.
|
||||
const putBack = merged
|
||||
.filter((m) => m && typeof m === 'object' && !returnedRows.has(rowOf(m)))
|
||||
.flatMap((m) => dedupByTriplet((rowMembers.get(m) ?? [m]).filter((f) =>
|
||||
f && typeof f === 'object' && f.engine), { perSource: false }));
|
||||
for (const f of putBack) {
|
||||
const { brief_lowered_from: _lowered, ...copy } = f;
|
||||
returned.push(copy);
|
||||
|
|
|
|||
|
|
@ -483,10 +483,11 @@ test('armB brief rule (15) — a tie between a reviewer MAJOR and a /code-review
|
|||
});
|
||||
|
||||
// ---- Decision 23 (operator 13.10, review ab1b5d9f): no engine finding leaves ----
|
||||
// the workflow review unreturned. Every engine finding of the input (before
|
||||
// dedup) with no returned finding at its exact (file, line, rule_key) is put
|
||||
// back at its own severity; a returned finding is never below the lower of its
|
||||
// reviewer counterpart's severity and its catalogue tier.
|
||||
// the workflow review unreturned. Each engine finding in a row (its triplet
|
||||
// (file, line, rule_key) plus its normalized title) that no returned finding
|
||||
// carries is put back at its own severity, one copy per row; a returned finding
|
||||
// is never below the lower of its reviewer counterpart's severity and its
|
||||
// catalogue tier.
|
||||
|
||||
const at = (findings, file, line, rule_key) =>
|
||||
findings.filter((f) => f && f.file === file && f.line === line && f.rule_key === rule_key);
|
||||
|
|
@ -559,7 +560,7 @@ test('decision 23 — a /code-review MAJOR dropped → WARN', async () => {
|
|||
});
|
||||
|
||||
for (const order of ['MAJOR first', 'BLOCKER first']) {
|
||||
test(`decision 23 — two /code-review findings on one triplet (${order}), returned empty → BLOCK, one copy, at BLOCKER`, async () => {
|
||||
test(`decision 23 — two /code-review findings on one triplet (${order}), returned empty → BLOCK, both rows put back, the BLOCKER row present`, async () => {
|
||||
const pair = [F('CODE_REVIEW_DEFECT', 'MAJOR', { title: 'major' }), F('CODE_REVIEW_DEFECT', 'BLOCKER', { title: 'blocker' })];
|
||||
const c = await floorCase({
|
||||
codeFindings: order === 'MAJOR first' ? pair : [pair[1], pair[0]],
|
||||
|
|
@ -568,10 +569,14 @@ for (const order of ['MAJOR first', 'BLOCKER first']) {
|
|||
});
|
||||
assert.equal(c.r.verdict, 'BLOCK');
|
||||
const back = at(c.r.findings, FILE, LINE, 'CODE_REVIEW_DEFECT');
|
||||
assert.equal(back.length, 1, 'exactly one finding at the triplet');
|
||||
assert.equal(back[0].severity, 'BLOCKER', 'the higher-severity copy is put back');
|
||||
assert.equal(back[0].title, 'blocker');
|
||||
assert.equal(back[0].engine, 'code-review');
|
||||
assert.equal(back.length, 2, 'one source under two titles is two rows, and each is put back');
|
||||
const got = back
|
||||
.map((f) => ({ title: f.title, severity: f.severity, engine: f.engine }))
|
||||
.sort((x, y) => x.title.localeCompare(y.title));
|
||||
assert.deepEqual(got, [
|
||||
{ title: 'blocker', severity: 'BLOCKER', engine: 'code-review' },
|
||||
{ title: 'major', severity: 'MAJOR', engine: 'code-review' },
|
||||
], 'each row is put back at its own severity');
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -698,3 +703,128 @@ test('[ORD] sibling: workflow — the title classes are placed in alphabetical o
|
|||
], `engine order ${name}`);
|
||||
}
|
||||
});
|
||||
|
||||
// ---- WF: the floor and the put-back match each returned finding to its own row ----
|
||||
//
|
||||
// A row is a merged finding; its key is its triplet plus its normalized title.
|
||||
// A returned finding is floored against the row with its own key; with no title
|
||||
// match, against the only row on its triplet, else the strictest one there,
|
||||
// never a lower one. Every row whose key was not returned has its engine
|
||||
// findings put back at their own severity.
|
||||
|
||||
test('[WF] two /code-review MAJORs A and B on one triplet: A returned legally lowered with ALLOW → WARN, B put back at MAJOR', async () => {
|
||||
const K = 'CODE_REVIEW_DEFECT';
|
||||
const c = await floorCase({
|
||||
codeFindings: [F(K, 'MAJOR', { title: 'A' }), F(K, 'MAJOR', { title: 'B' })],
|
||||
returned: [F(K, 'MINOR', { title: 'A', contradicts_brief: MARK, brief_lowered_from: 'MAJOR' })],
|
||||
verdict: 'ALLOW',
|
||||
});
|
||||
assert.equal(c.merged.length, 2, 'one source under two titles is two rows');
|
||||
assert.equal(c.r.verdict, 'WARN');
|
||||
assert.equal(c.r.findings[0].severity, 'MINOR', 'the legal lowering of A stands');
|
||||
assert.equal(c.r.findings[0].engine, 'code-review');
|
||||
const b = at(c.r.findings, FILE, LINE, K).filter((f) => f.title === 'B');
|
||||
assert.equal(b.length, 1, 'B is put back');
|
||||
assert.equal(b[0].severity, 'MAJOR');
|
||||
assert.equal(b[0].engine, 'code-review');
|
||||
});
|
||||
|
||||
test('[WF] two /code-review MAJORs A and B on one triplet: A returned at MAJOR with WARN → B present at MAJOR', async () => {
|
||||
const K = 'CODE_REVIEW_DEFECT';
|
||||
const c = await floorCase({
|
||||
codeFindings: [F(K, 'MAJOR', { title: 'A' }), F(K, 'MAJOR', { title: 'B' })],
|
||||
returned: [F(K, 'MAJOR', { title: 'A' })],
|
||||
verdict: 'WARN',
|
||||
});
|
||||
assert.equal(c.r.verdict, 'WARN');
|
||||
const titles = at(c.r.findings, FILE, LINE, K).map((f) => `${f.title}:${f.severity}`).sort();
|
||||
assert.deepEqual(titles, ['A:MAJOR', 'B:MAJOR'], 'B is present beside the returned A');
|
||||
});
|
||||
|
||||
test('[WF] a reviewer MAJOR returned as MINOR next to engine rows is floored against its own row and keeps engine unset', async () => {
|
||||
const K = 'CODE_REVIEW_DEFECT';
|
||||
const c = await floorCase({
|
||||
reviewerFindings: [F(K, 'MAJOR', { title: 'R' })],
|
||||
codeFindings: [F(K, 'MINOR', { title: 'E1' }), F(K, 'MINOR', { title: 'E2' })],
|
||||
returned: [F(K, 'MINOR', { title: 'R' })],
|
||||
verdict: 'ALLOW',
|
||||
});
|
||||
// Rows: {E1, R} kept as the reviewer's R at MAJOR, and {E2}, an engine row at MINOR.
|
||||
const rows = c.merged.map((m) => `${m.title}:${m.severity}:${m.engine ?? 'none'}`).sort();
|
||||
assert.deepEqual(rows, ['E2:MINOR:code-review', 'R:MAJOR:none']);
|
||||
assert.equal(c.r.findings[0].title, 'R');
|
||||
assert.equal(c.r.findings[0].severity, 'MAJOR', 'floored against its own row, not the engine row E2');
|
||||
assert.equal(c.r.findings[0].engine, undefined, 'engine comes from its own row only');
|
||||
assert.equal(c.r.verdict, 'WARN');
|
||||
const e2 = at(c.r.findings, FILE, LINE, K).filter((f) => f.title === 'E2');
|
||||
assert.equal(e2.length, 1, 'the engine row E2 was not returned and is put back');
|
||||
assert.equal(e2[0].severity, 'MINOR');
|
||||
});
|
||||
|
||||
test('[WF] a returned finding whose title matches no row is floored against the strictest row on its triplet, never a lower one', async () => {
|
||||
const K = 'CODE_REVIEW_DEFECT';
|
||||
const blocker = () => F(K, 'BLOCKER', { title: 'B' });
|
||||
const minor = () => F(K, 'MINOR', { title: 'A' });
|
||||
// Both input orders: the strictest row is first in one and last in the other.
|
||||
for (const [name, codeFindings] of [['BLOCKER first', [blocker(), minor()]], ['BLOCKER last', [minor(), blocker()]]]) {
|
||||
const c = await floorCase({
|
||||
codeFindings,
|
||||
returned: [F(K, 'MINOR', { title: 'renamed' })],
|
||||
verdict: 'ALLOW',
|
||||
});
|
||||
assert.equal(c.r.findings[0].title, 'renamed', name);
|
||||
assert.equal(c.r.findings[0].severity, 'BLOCKER', `the strictest row on the triplet, not the first or last one (${name})`);
|
||||
assert.equal(c.r.verdict, 'BLOCK', name);
|
||||
}
|
||||
});
|
||||
|
||||
// Sample titles, each pair with whether the title rule calls them the same.
|
||||
const TITLE_PAIRS = [
|
||||
['Foo', 'Foo', true],
|
||||
['Foo', 'foo', true],
|
||||
[' Foo bar ', 'foo BAR', true],
|
||||
['Foo\t', 'FOO', true],
|
||||
['Foo', 'Bar', false],
|
||||
['foo bar', 'foo bar', false],
|
||||
['foo-bar', 'foo bar', false],
|
||||
];
|
||||
|
||||
test('[WF] the row key reads titles as sameTitle does: on sample titles a returned finding matches a row exactly where the grouping calls the titles the same', async () => {
|
||||
const K = 'CODE_REVIEW_DEFECT';
|
||||
for (const [a, b, same] of TITLE_PAIRS) {
|
||||
const name = `${JSON.stringify(a)} / ${JSON.stringify(b)}`;
|
||||
// sameTitle in the grouping: one source under titles a and b is one row exactly when they are the same.
|
||||
const g = await floorCase({
|
||||
codeFindings: [F(K, 'MAJOR', { title: a }), F(K, 'MAJOR', { title: b })],
|
||||
returned: [F(K, 'MAJOR', { title: a })],
|
||||
verdict: 'WARN',
|
||||
});
|
||||
assert.equal(g.merged.length, same ? 1 : 2, `grouping, ${name}`);
|
||||
// The row key: a row titled a, returned titled b, is matched (nothing put back) exactly when they are the same.
|
||||
const k = await floorCase({
|
||||
codeFindings: [F(K, 'MAJOR', { title: a })],
|
||||
returned: [F(K, 'MAJOR', { title: b })],
|
||||
verdict: 'WARN',
|
||||
});
|
||||
assert.equal(at(k.r.findings, FILE, LINE, K).length, same ? 1 : 2, `row key, ${name}`);
|
||||
}
|
||||
});
|
||||
|
||||
// ---- Architecture rule: the workflow script imports nothing (it runs inside the Workflow tool) ----
|
||||
|
||||
// Every module specifier a source text imports, statically or dynamically.
|
||||
function importSpecifiers(text) {
|
||||
const found = [];
|
||||
for (const m of text.matchAll(/^\s*import\s*(?:[\w*{][^;]*?\sfrom\s*)?['"]([^'"]+)['"]/gm)) found.push(m[1]);
|
||||
for (const m of text.matchAll(/\bimport\s*\(\s*['"`]([^'"`]+)['"`]/g)) found.push(m[1]);
|
||||
return found;
|
||||
}
|
||||
|
||||
test('arch: scripts/trekreview-armB.workflow.mjs holds no import statement, and the scan reports a planted import (control)', () => {
|
||||
const raw = readFileSync(join(ROOT, 'scripts', 'trekreview-armB.workflow.mjs'), 'utf-8');
|
||||
assert.deepEqual(importSpecifiers(raw), []);
|
||||
const planted = importSpecifiers(`${raw}\nimport { sameTitle } from '../lib/review/coordinator-contract.mjs';\n`);
|
||||
assert.deepEqual(planted, ['../lib/review/coordinator-contract.mjs'], 'control: the scan does not report a planted import');
|
||||
const dynamic = importSpecifiers(`${raw}\nconst m = await import('node:fs');\n`);
|
||||
assert.deepEqual(dynamic, ['node:fs'], 'control: the scan does not report a planted dynamic import');
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue