fix(review): group a triplet's findings title first, so arrival order never changes the rows
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
7175783ce5
commit
a5fcb5e8b4
6 changed files with 339 additions and 37 deletions
|
|
@ -79,9 +79,14 @@ re-reading, and fresh finding generation are all forbidden.
|
||||||
Two findings collide when their `(file, line, rule_key)` triplets are
|
Two findings collide when their `(file, line, rule_key)` triplets are
|
||||||
identical, with one exception: **one reviewer never duplicates itself.** Two
|
identical, with one exception: **one reviewer never duplicates itself.** Two
|
||||||
findings from the same reviewer on one triplet under different titles are two
|
findings from the same reviewer on one triplet under different titles are two
|
||||||
findings (an engine's catch-all `rule_key` makes this common): keep both rows,
|
findings (an engine's catch-all `rule_key` makes this common): keep both rows.
|
||||||
in input order. A finding from another reviewer joins the row whose title it
|
Group in two passes, so the order the findings arrive in never changes a row:
|
||||||
shares, else the first one. When findings collide:
|
first every finding joins the row of its title (case and outer spaces
|
||||||
|
ignored), over all of the triplet's findings; then each title row, in
|
||||||
|
alphabetical order of its title, joins the first row that holds no finding of
|
||||||
|
one of its reviewers under another title, else it stays a row of its own. A
|
||||||
|
finding without a title joins the first row it fits, last. When findings
|
||||||
|
collide:
|
||||||
- Rank the colliding findings and keep the first:
|
- Rank the colliding findings and keep the first:
|
||||||
1. the higher catalogue tier: the severity `lib/review/rule-catalogue.mjs`
|
1. the higher catalogue tier: the severity `lib/review/rule-catalogue.mjs`
|
||||||
gives the `rule_key`, or the finding's own severity when the key is
|
gives the `rule_key`, or the finding's own severity when the key is
|
||||||
|
|
@ -93,7 +98,10 @@ shares, else the first one. When findings collide:
|
||||||
3. among findings of one kind, the higher asserted severity;
|
3. among findings of one kind, the higher asserted severity;
|
||||||
4. among reviewer-agent findings, prefer the finding from
|
4. among reviewer-agent findings, prefer the finding from
|
||||||
`brief-conformance-reviewer` (its findings are anchored to the brief);
|
`brief-conformance-reviewer` (its findings are anchored to the brief);
|
||||||
5. then the first seen.
|
5. then the first seen in the row's order: its title groups
|
||||||
|
alphabetically, each title's findings in canonical order (reviewer,
|
||||||
|
detail, the whole finding), findings without a title last in that
|
||||||
|
order, never the order the findings arrived in.
|
||||||
- Write the kept finding with the highest severity any finding in its
|
- Write the kept finding with the highest severity any finding in its
|
||||||
group asserted, so dropping a duplicate never lowers the group and an
|
group asserted, so dropping a duplicate never lowers the group and an
|
||||||
engine's brief-conflict mark never lowers what a reviewer agent asserted.
|
engine's brief-conflict mark never lowers what a reviewer agent asserted.
|
||||||
|
|
|
||||||
|
|
@ -19,7 +19,7 @@
|
||||||
//
|
//
|
||||||
// What IS implemented, purely: Pass 1 (triplet dedup → survivor ranked by
|
// What IS implemented, purely: Pass 1 (triplet dedup → survivor ranked by
|
||||||
// catalogue tier, then reviewer over engine, asserted severity, conformance,
|
// catalogue tier, then reviewer over engine, asserted severity, conformance,
|
||||||
// first seen; the survivor carries its group's highest asserted severity
|
// first in the group's order; the survivor carries its group's highest asserted severity
|
||||||
// (review bc7f4e23) + detail concat + raised_by provenance; when that raises
|
// (review bc7f4e23) + detail concat + raised_by provenance; when that raises
|
||||||
// it, `pass1_raised_from` keeps what the survivor itself asserted, read by
|
// it, `pass1_raised_from` keeps what the survivor itself asserted, read by
|
||||||
// Pass 3 and never returned: `original_severity` is what the kept finding
|
// Pass 3 and never returned: `original_severity` is what the kept finding
|
||||||
|
|
@ -254,9 +254,10 @@ export function ingest(reviewerPayloads) {
|
||||||
* (1) the catalogue tier (`getRule(rule_key)?.severity`, else the finding's own
|
* (1) the catalogue tier (`getRule(rule_key)?.severity`, else the finding's own
|
||||||
* severity; lower `severityRank` wins), (2) a reviewer-agent finding over an
|
* severity; lower `severityRank` wins), (2) a reviewer-agent finding over an
|
||||||
* engine finding, (3) among findings of one kind the higher asserted severity,
|
* engine finding, (3) among findings of one kind the higher asserted severity,
|
||||||
* (4) the conformance reviewer, (5) first seen. It is written with the highest
|
* (4) the conformance reviewer, (5) the first in the group's order (the order
|
||||||
* severity any finding in its group asserted, so dropping a duplicate never
|
* the two passes below place the members in, never the arrival order). It is
|
||||||
* lowers the group (review bc7f4e23; plan 2026-10-11-files-backticks-trial4,
|
* written with the highest severity any finding in its group asserted, so
|
||||||
|
* dropping a duplicate never lowers the group (review bc7f4e23; plan 2026-10-11-files-backticks-trial4,
|
||||||
* ADR "Pass 1 survivor order and group severity"). Carries raised_by
|
* ADR "Pass 1 survivor order and group severity"). Carries raised_by
|
||||||
* provenance, concatenates other reviewers' attribution into detail, and
|
* provenance, concatenates other reviewers' attribution into detail, and
|
||||||
* recomputes the id over the triplet.
|
* recomputes the id over the triplet.
|
||||||
|
|
@ -267,23 +268,34 @@ export function dedupByTriplet(findings) {
|
||||||
// One reviewer never duplicates itself: its second finding on a triplet under
|
// One reviewer never duplicates itself: its second finding on a triplet under
|
||||||
// another title is another finding and opens a group of its own, which gets
|
// another title is another finding and opens a group of its own, which gets
|
||||||
// its own row and the id over `rule_key#2` (voyage-lekerepo trial 5,
|
// its own row and the id over `rule_key#2` (voyage-lekerepo trial 5,
|
||||||
// finding 7). Another reviewer's finding joins the group whose title it
|
// finding 7). Inside one triplet the findings are grouped in two passes, so
|
||||||
// shares, else the first one it can join.
|
// the order the reviewers' findings arrive in never changes which row a
|
||||||
|
// finding joins (plan 2026-10-20-fix-round-serious-findings, ADR "ORD: two
|
||||||
|
// passes over the triplet, title first"): (1) every finding with a title
|
||||||
|
// joins the class of its normalized title, over all of the triplet's
|
||||||
|
// findings; (2) the title classes, in alphabetical order of the normalized
|
||||||
|
// title, each join the first earlier group whose every member fits every
|
||||||
|
// finding of the class (another reviewer, or the same title), else they form
|
||||||
|
// a group of their own; findings without a title come last, in canonical
|
||||||
|
// order (reviewer, detail, the whole finding), each joining the first group
|
||||||
|
// it fits. Members inside a group keep the order the passes place them in:
|
||||||
|
// the classes alphabetically, each class's findings in canonical order, the
|
||||||
|
// untitled findings last in canonical order. Rank (5) of the survivor reads
|
||||||
|
// that order, so a full tie never follows arrival order either: the survivor
|
||||||
|
// decides Pass 2's actionability and Pass 3's brief-conflict lowering, and so
|
||||||
|
// the verdict. A triplet's groups keep the order of their earliest input
|
||||||
|
// index, so the rows and `#k` ids stay in input order.
|
||||||
const tripletGroups = new Map();
|
const tripletGroups = new Map();
|
||||||
for (const f of findings) {
|
findings.forEach((f, i) => {
|
||||||
const key = tripletKey(f);
|
const key = tripletKey(f);
|
||||||
if (!tripletGroups.has(key)) tripletGroups.set(key, []);
|
if (!tripletGroups.has(key)) tripletGroups.set(key, []);
|
||||||
const subs = tripletGroups.get(key);
|
tripletGroups.get(key).push({ f, i });
|
||||||
const fits = (g) => g.every((m) => m.reviewer !== f.reviewer || sameTitle(m, f));
|
});
|
||||||
let sub = subs.find((g) => fits(g) && g.some((m) => sameTitle(m, f) && m.title !== undefined && f.title !== undefined))
|
const groups = [...tripletGroups.values()].flatMap((entries) => groupTriplet(
|
||||||
?? subs.find(fits);
|
entries,
|
||||||
if (!sub) {
|
(a, b) => a.reviewer !== b.reviewer || sameTitle(a, b),
|
||||||
sub = [];
|
(f) => f.reviewer,
|
||||||
subs.push(sub);
|
));
|
||||||
}
|
|
||||||
sub.push(f);
|
|
||||||
}
|
|
||||||
const groups = [...tripletGroups.values()].flat();
|
|
||||||
const out = [];
|
const out = [];
|
||||||
for (const group of groups) {
|
for (const group of groups) {
|
||||||
// Ranked as the JSDoc above says. A brief-conflict mark is not carried over
|
// Ranked as the JSDoc above says. A brief-conflict mark is not carried over
|
||||||
|
|
@ -346,6 +358,56 @@ function sameTitle(a, b) {
|
||||||
return String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
|
return String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Code-unit order of two strings (never the locale's). */
|
||||||
|
function cmpStr(a, b) {
|
||||||
|
return a < b ? -1 : a > b ? 1 : 0;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The groups of one triplet's findings, in the two passes `dedupByTriplet`
|
||||||
|
* describes. `entries` are `{ f, i }` (the finding and its input index);
|
||||||
|
* `fits(m, f)` says member `m` may share a group with `f`; `whoOf(f)` names the
|
||||||
|
* reviewer for the canonical order. Returns arrays of findings, each in the
|
||||||
|
* order the passes place them (classes alphabetically, each class in canonical
|
||||||
|
* order, untitled findings last in canonical order), the groups in the order
|
||||||
|
* of their earliest input index.
|
||||||
|
* @param {Array<{f: object, i: number}>} entries
|
||||||
|
* @param {(m: object, f: object) => boolean} fits
|
||||||
|
* @param {(f: object) => unknown} whoOf
|
||||||
|
* @returns {object[][]}
|
||||||
|
*/
|
||||||
|
function groupTriplet(entries, fits, whoOf) {
|
||||||
|
const canon = (x, y) => cmpStr(String(whoOf(x.f) ?? ''), String(whoOf(y.f) ?? ''))
|
||||||
|
|| cmpStr(String(x.f.detail ?? ''), String(y.f.detail ?? ''))
|
||||||
|
|| cmpStr(JSON.stringify(x.f), JSON.stringify(y.f));
|
||||||
|
const classes = new Map();
|
||||||
|
const untitled = [];
|
||||||
|
for (const e of entries) {
|
||||||
|
if (e.f.title === undefined) {
|
||||||
|
untitled.push(e);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
const t = String(e.f.title).trim().toLowerCase();
|
||||||
|
if (!classes.has(t)) classes.set(t, []);
|
||||||
|
classes.get(t).push(e);
|
||||||
|
}
|
||||||
|
const groups = [];
|
||||||
|
const place = (members) => {
|
||||||
|
let g = groups.find((grp) => grp.every((m) => members.every((e) => fits(m.f, e.f))));
|
||||||
|
if (!g) {
|
||||||
|
g = [];
|
||||||
|
groups.push(g);
|
||||||
|
}
|
||||||
|
g.push(...members);
|
||||||
|
};
|
||||||
|
for (const t of [...classes.keys()].sort(cmpStr)) place(classes.get(t).sort(canon));
|
||||||
|
for (const e of untitled.sort(canon)) place([e]);
|
||||||
|
return groups
|
||||||
|
.map((g) => ({ first: Math.min(...g.map((e) => e.i)), members: g.map((e) => e.f) }))
|
||||||
|
.sort((a, b) => a.first - b.first)
|
||||||
|
.map((g) => g.members);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The tiers whose over-long text may be trimmed. Length is packaging, never a
|
* The tiers whose over-long text may be trimmed. Length is packaging, never a
|
||||||
* claim: a BLOCKER or MAJOR keeps every character, and nothing is ever removed
|
* claim: a BLOCKER or MAJOR keeps every character, and nothing is ever removed
|
||||||
|
|
|
||||||
|
|
@ -123,7 +123,8 @@ const VERDICT_SCHEMA = {
|
||||||
// first (for a catalogue key it always ties inside one triplet, since the
|
// first (for a catalogue key it always ties inside one triplet, since the
|
||||||
// rule_key is shared), then a reviewer finding (no `engine`) over an engine
|
// rule_key is shared), then a reviewer finding (no `engine`) over an engine
|
||||||
// finding (decision 13, operator 12.10), then the higher asserted severity;
|
// finding (decision 13, operator 12.10), then the higher asserted severity;
|
||||||
// the workflow has no conformance rule, so otherwise the first-seen one stays.
|
// the workflow has no conformance rule, so otherwise the first in the group's
|
||||||
|
// order stays (the order the two passes below place the members in).
|
||||||
// The survivor carries the highest severity any finding in its group asserted,
|
// The survivor carries the highest severity any finding in its group asserted,
|
||||||
// so an engine's mark never lowers what a reviewer agent asserted.
|
// so an engine's mark never lowers what a reviewer agent asserted.
|
||||||
// As in coordinator-contract Pass 1, one source never duplicates itself: its
|
// As in coordinator-contract Pass 1, one source never duplicates itself: its
|
||||||
|
|
@ -131,25 +132,64 @@ const VERDICT_SCHEMA = {
|
||||||
// (voyage-lekerepo trial 5, finding 7). `sourceOf` names the reviewer or engine.
|
// (voyage-lekerepo trial 5, finding 7). `sourceOf` names the reviewer or engine.
|
||||||
const sameTitle = (a, b) => a.title === undefined || b.title === undefined ||
|
const sameTitle = (a, b) => a.title === undefined || b.title === undefined ||
|
||||||
String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
|
String(a.title).trim().toLowerCase() === String(b.title).trim().toLowerCase();
|
||||||
|
// As there too, a triplet's findings are grouped in two passes, so the order
|
||||||
|
// they arrive in never changes which row a finding joins: (1) every titled
|
||||||
|
// finding joins the class of its normalized title, over all of the triplet's
|
||||||
|
// findings; (2) the classes, in alphabetical order of the normalized title,
|
||||||
|
// each join the first earlier group whose every member fits every finding of
|
||||||
|
// the class (another source, or the same title), else they form a group of
|
||||||
|
// their own; untitled findings come last, in canonical order (source, detail,
|
||||||
|
// the whole finding), each joining the first group it fits. Members keep the
|
||||||
|
// order the passes place them in (the classes alphabetically, each class's
|
||||||
|
// 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.
|
||||||
const sourceOf = new Map();
|
const sourceOf = new Map();
|
||||||
|
const cmpStr = (a, b) => (a < b ? -1 : a > b ? 1 : 0);
|
||||||
|
function groupTriplet(entries) {
|
||||||
|
const srcOf = (f) => sourceOf.get(f) ?? null;
|
||||||
|
const fits = (m, f) => srcOf(m) !== srcOf(f) || sameTitle(m, f);
|
||||||
|
const canon = (x, y) => cmpStr(String(srcOf(x.f) ?? ''), String(srcOf(y.f) ?? ''))
|
||||||
|
|| cmpStr(String(x.f.detail ?? ''), String(y.f.detail ?? ''))
|
||||||
|
|| cmpStr(JSON.stringify(x.f), JSON.stringify(y.f));
|
||||||
|
const classes = new Map();
|
||||||
|
const untitled = [];
|
||||||
|
for (const e of entries) {
|
||||||
|
if (e.f.title === undefined) {
|
||||||
|
untitled.push(e);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
const t = String(e.f.title).trim().toLowerCase();
|
||||||
|
if (!classes.has(t)) classes.set(t, []);
|
||||||
|
classes.get(t).push(e);
|
||||||
|
}
|
||||||
|
const groups = [];
|
||||||
|
const place = (members) => {
|
||||||
|
let g = groups.find((grp) => grp.every((m) => members.every((e) => fits(m.f, e.f))));
|
||||||
|
if (!g) {
|
||||||
|
g = [];
|
||||||
|
groups.push(g);
|
||||||
|
}
|
||||||
|
g.push(...members);
|
||||||
|
};
|
||||||
|
for (const t of [...classes.keys()].sort(cmpStr)) place(classes.get(t).sort(canon));
|
||||||
|
for (const e of untitled.sort(canon)) place([e]);
|
||||||
|
return groups
|
||||||
|
.map((g) => ({ first: Math.min(...g.map((e) => e.i)), members: g.map((e) => e.f) }))
|
||||||
|
.sort((a, b) => a.first - b.first)
|
||||||
|
.map((g) => g.members);
|
||||||
|
}
|
||||||
function dedupByTriplet(findings, { perSource = true } = {}) {
|
function dedupByTriplet(findings, { perSource = true } = {}) {
|
||||||
const byTriplet = new Map();
|
const byTriplet = new Map();
|
||||||
for (const f of findings) {
|
findings.forEach((f, i) => {
|
||||||
const key = tripletOf(f);
|
const key = tripletOf(f);
|
||||||
if (!byTriplet.has(key)) byTriplet.set(key, []);
|
if (!byTriplet.has(key)) byTriplet.set(key, []);
|
||||||
const subs = byTriplet.get(key);
|
byTriplet.get(key).push({ f, i });
|
||||||
const src = sourceOf.get(f) ?? null;
|
});
|
||||||
const fits = (g) => g.every((m) => (sourceOf.get(m) ?? null) !== src || sameTitle(m, f));
|
const groups = [...byTriplet.values()].flatMap((entries) => (perSource
|
||||||
let sub = !perSource ? subs[0]
|
? groupTriplet(entries)
|
||||||
: subs.find((g) => fits(g) && g.some((m) => m.title !== undefined && f.title !== undefined && sameTitle(m, f)))
|
: [entries.map((e) => e.f)]));
|
||||||
?? subs.find(fits);
|
|
||||||
if (!sub) {
|
|
||||||
sub = [];
|
|
||||||
subs.push(sub);
|
|
||||||
}
|
|
||||||
sub.push(f);
|
|
||||||
}
|
|
||||||
const groups = [...byTriplet.values()].flat();
|
|
||||||
const tier = (f) => sevRank(tierOf(f.rule_key) ?? f.severity);
|
const tier = (f) => sevRank(tierOf(f.rule_key) ?? f.severity);
|
||||||
const out = [];
|
const out = [];
|
||||||
for (const group of groups) {
|
for (const group of groups) {
|
||||||
|
|
|
||||||
|
|
@ -455,3 +455,126 @@ test('runContract — the #k id counts rows of one triplet in the final list, so
|
||||||
assert.equal(result.findings[0].id, computeFindingId('bin/slug.mjs', 46, 'MISSING_ERROR_HANDLING'), 'what fill-finding-ids computes for a review.md holding this row alone');
|
assert.equal(result.findings[0].id, computeFindingId('bin/slug.mjs', 46, 'MISSING_ERROR_HANDLING'), 'what fill-finding-ids computes for a review.md holding this row alone');
|
||||||
assert.equal(result.trimmed_count, 1, 'the trimmed row that Pass 2 set aside is not counted');
|
assert.equal(result.trimmed_count, 1, 'the trimmed row that Pass 2 set aside is not counted');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ---- ORD — the rows a finding joins do not depend on arrival order ---------
|
||||||
|
|
||||||
|
/** Every arrival order of `xs`. */
|
||||||
|
function orders(xs) {
|
||||||
|
if (xs.length <= 1) return [xs];
|
||||||
|
return xs.flatMap((x, i) => orders([...xs.slice(0, i), ...xs.slice(i + 1)]).map((rest) => [x, ...rest]));
|
||||||
|
}
|
||||||
|
|
||||||
|
/** A row's members, rebuilt from its survivor plus each "Also flagged by" line, as sorted `reviewer|title` strings. */
|
||||||
|
function members(row) {
|
||||||
|
const out = [`${row.reviewer}|${row.title}`];
|
||||||
|
for (const m of String(row.detail ?? '').matchAll(/^Also flagged by (.+?): (.+)\.$/gm)) out.push(`${m[1]}|${m[2]}`);
|
||||||
|
return out.sort();
|
||||||
|
}
|
||||||
|
|
||||||
|
const ORD_C = 'code-correctness-reviewer';
|
||||||
|
const ORD_F = 'brief-conformance-reviewer';
|
||||||
|
|
||||||
|
test('[ORD] correctness A, conformance B, correctness B give the rows {correctness A} and {conformance B, correctness B} in every arrival order', () => {
|
||||||
|
const at = { file: 'bin/slug.mjs', line: 46, rule_key: 'MISSING_TEST', detail: 'd' };
|
||||||
|
const cA = { ...at, severity: 'MAJOR', title: 'A', reviewer: ORD_C };
|
||||||
|
const fB = { ...at, severity: 'MAJOR', title: 'B', reviewer: ORD_F };
|
||||||
|
const cB = { ...at, severity: 'MAJOR', title: 'B', reviewer: ORD_C };
|
||||||
|
const expected = [[`${ORD_C}|A`], [`${ORD_F}|B`, `${ORD_C}|B`]];
|
||||||
|
const all = orders([['cA', cA], ['fB', fB], ['cB', cB]]);
|
||||||
|
assert.equal(all.length, 6);
|
||||||
|
for (const order of all) {
|
||||||
|
const name = order.map(([n]) => n).join(', ');
|
||||||
|
const rows = dedupByTriplet(order.map(([, f]) => structuredClone(f)));
|
||||||
|
assert.equal(rows.length, 2, `${name}: ${JSON.stringify(rows.map(members))}`);
|
||||||
|
const got = rows.map(members).sort((x, y) => x.length - y.length || x.join().localeCompare(y.join()));
|
||||||
|
assert.deepEqual(got, expected, `order ${name}`);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[ORD] with an off-catalogue rule_key and cA MINOR, fB MAJOR, cB MINOR, every arrival order gives {cA} MINOR, {fB, cB} MAJOR and one verdict', () => {
|
||||||
|
const at = { file: 'bin/slug.mjs', line: 46, rule_key: 'NOT_IN_THE_CATALOGUE', detail: 'd' };
|
||||||
|
const cA = { ...at, severity: 'MINOR', title: 'A', reviewer: ORD_C };
|
||||||
|
const fB = { ...at, severity: 'MAJOR', title: 'B', reviewer: ORD_F };
|
||||||
|
const cB = { ...at, severity: 'MINOR', title: 'B', reviewer: ORD_C };
|
||||||
|
const expected = { [`${ORD_C}|A`]: 'MINOR', [`${ORD_F}|B+${ORD_C}|B`]: 'MAJOR' };
|
||||||
|
const verdicts = new Set();
|
||||||
|
for (const order of orders([['cA', cA], ['fB', fB], ['cB', cB]])) {
|
||||||
|
const name = order.map(([n]) => n).join(', ');
|
||||||
|
const rows = dedupByTriplet(order.map(([, f]) => structuredClone(f)));
|
||||||
|
const bySet = Object.fromEntries(rows.map((r) => [members(r).join('+'), r.severity]));
|
||||||
|
assert.deepEqual(bySet, expected, `order ${name}`);
|
||||||
|
verdicts.add(JSON.stringify(computeVerdict(rows)));
|
||||||
|
}
|
||||||
|
assert.equal(verdicts.size, 1, [...verdicts].join(' | '));
|
||||||
|
assert.equal(JSON.parse([...verdicts][0]).verdict, 'WARN');
|
||||||
|
});
|
||||||
|
|
||||||
|
/** The kept and unverified rows of a run, as sorted `reviewer|title|severity` strings, so two runs can be compared. */
|
||||||
|
function split(result) {
|
||||||
|
const key = (f) => `${f.reviewer}|${String(f.title ?? '').trim().toLowerCase()}|${f.severity}`;
|
||||||
|
return { kept: result.findings.map(key).sort(), unverified: result.unverified.map(key).sort() };
|
||||||
|
}
|
||||||
|
|
||||||
|
test('[ORD] a full tie on one row: one reviewer\'s two same-title BLOCKERs, one with an empty recommended_action, give one verdict in both arrival orders', () => {
|
||||||
|
const at = { file: 'x.mjs', line: 3, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER' };
|
||||||
|
const empty = { ...at, title: 'Injection', recommended_action: '' };
|
||||||
|
const full = { ...at, title: 'injection ', recommended_action: 'Quote the argument.' };
|
||||||
|
const a = runContract([{ reviewer: ORD_C, findings: [structuredClone(empty), structuredClone(full)] }]);
|
||||||
|
const b = runContract([{ reviewer: ORD_C, findings: [structuredClone(full), structuredClone(empty)] }]);
|
||||||
|
assert.equal(a.verdict, b.verdict, `order (empty, full) gives ${a.verdict}, order (full, empty) gives ${b.verdict}`);
|
||||||
|
assert.deepEqual(split(a), split(b), 'the kept/unverified split');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[ORD] a full tie on one row: two /code-review MAJORs titled T, one marked as contradicting the brief, give one verdict in both arrival orders', () => {
|
||||||
|
const at = { file: 'x.mjs', line: 3, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR', title: 'T' };
|
||||||
|
const marked = { ...at, contradicts_brief: 'Non-Goal: no new runtime dependency' };
|
||||||
|
const plain = { ...at };
|
||||||
|
const a = runContract([{ reviewer: 'claude-code-review', findings: [structuredClone(marked), structuredClone(plain)] }]);
|
||||||
|
const b = runContract([{ reviewer: 'claude-code-review', findings: [structuredClone(plain), structuredClone(marked)] }]);
|
||||||
|
assert.equal(a.verdict, b.verdict, `marked first gives ${a.verdict}, unmarked first gives ${b.verdict}`);
|
||||||
|
assert.deepEqual(split(a), split(b), 'the kept/unverified split');
|
||||||
|
});
|
||||||
|
|
||||||
|
/** A row's member set, rebuilt from its survivor plus each "Also flagged by" line; an untitled member is named by its rule_key, as the line names it. */
|
||||||
|
function memberSet(row) {
|
||||||
|
const out = [`${row.reviewer}|${row.title ?? row.rule_key}`];
|
||||||
|
for (const m of String(row.detail ?? '').matchAll(/^Also flagged by (.+?): (.+)\.$/gm)) out.push(`${m[1]}|${m[2]}`);
|
||||||
|
return out.sort().join('+');
|
||||||
|
}
|
||||||
|
|
||||||
|
test('[ORD] the title classes are placed in alphabetical order: cA, fB, cB and an untitled correctness BLOCKER give {cA, cU} BLOCKER and {fB, cB} MAJOR and one verdict in all 24 orders', () => {
|
||||||
|
const K = 'NOT_IN_THE_CATALOGUE';
|
||||||
|
const at = { file: 'bin/slug.mjs', line: 46, rule_key: K, detail: 'd' };
|
||||||
|
// cA and cU tie on every survivor rule, so the row's first member in the
|
||||||
|
// group's order (class A before the untitled findings) is kept: cA, whose
|
||||||
|
// empty recommended_action sets the row aside as unverified.
|
||||||
|
const cA = { ...at, severity: 'BLOCKER', title: 'A', reviewer: ORD_C, recommended_action: '' };
|
||||||
|
const fB = { ...at, severity: 'MAJOR', title: 'B', reviewer: ORD_F };
|
||||||
|
const cB = { ...at, severity: 'MINOR', title: 'B', reviewer: ORD_C };
|
||||||
|
const cU = { ...at, severity: 'BLOCKER', reviewer: ORD_C };
|
||||||
|
const expected = {
|
||||||
|
[`${ORD_C}|A+${ORD_C}|${K}`]: 'BLOCKER',
|
||||||
|
[`${ORD_F}|B+${ORD_C}|B`]: 'MAJOR',
|
||||||
|
};
|
||||||
|
const all = orders([['cA', cA], ['fB', fB], ['cB', cB], ['cU', cU]]);
|
||||||
|
assert.equal(all.length, 24);
|
||||||
|
const verdicts = new Set();
|
||||||
|
for (const order of all) {
|
||||||
|
const name = order.map(([n]) => n).join(', ');
|
||||||
|
const rows = dedupByTriplet(order.map(([, f]) => structuredClone(f)));
|
||||||
|
const bySet = Object.fromEntries(rows.map((r) => [memberSet(r), r.severity]));
|
||||||
|
assert.deepEqual(bySet, expected, `order ${name}`);
|
||||||
|
const j = judgeFilter(rows);
|
||||||
|
verdicts.add(JSON.stringify({
|
||||||
|
kept: j.kept.map(memberSet).sort(),
|
||||||
|
unverified: j.unverified.map(memberSet).sort(),
|
||||||
|
verdict: computeVerdict(j.kept, { unverified: j.unverified }).verdict,
|
||||||
|
}));
|
||||||
|
}
|
||||||
|
assert.equal(verdicts.size, 1, [...verdicts].join(' | '));
|
||||||
|
assert.deepEqual(JSON.parse([...verdicts][0]), {
|
||||||
|
kept: [`${ORD_F}|B+${ORD_C}|B`],
|
||||||
|
unverified: [`${ORD_C}|A+${ORD_C}|${K}`],
|
||||||
|
verdict: 'WARN',
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -1382,3 +1382,11 @@ test('[DJ] review-coordinator body section 9 copies every reviewer Declined to J
|
||||||
assert.match(body, /then every line a reviewer wrote under its own `### Declined to Judge`, copied as `\{reviewer\}: \{line\}` \(they are not findings: they stay out of the JSON `declined_to_judge` array and do not change the verdict\)/,
|
assert.match(body, /then every line a reviewer wrote under its own `### Declined to Judge`, copied as `\{reviewer\}: \{line\}` \(they are not findings: they stay out of the JSON `declined_to_judge` array and do not change the verdict\)/,
|
||||||
'body section 9 no longer copies the reviewers\' Declined to Judge lines');
|
'body section 9 no longer copies the reviewers\' Declined to Judge lines');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('[ORD] sibling: agents/review-coordinator.md — Pass 1 groups in two passes, title first, with no "else the first one"', () => {
|
||||||
|
const pass1 = flat(section(read(COORD), '### Pass 1 — Dedup by `(file, line, rule_key)` triplet'));
|
||||||
|
assert.doesNotMatch(pass1, /else the first one/, 'Pass 1 still lets a finding fall back to the first row in arrival order');
|
||||||
|
assert.doesNotMatch(pass1, /keep both rows, in input order/, 'Pass 1 still keeps the rows in input order');
|
||||||
|
assert.match(pass1, /Group in two passes, so the order the findings arrive in never changes a row: first every finding joins the row of its title/,
|
||||||
|
'Pass 1 does not name the two passes, title first');
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -637,3 +637,64 @@ test('armB — two different /code-review findings on one triplet stay two rows
|
||||||
});
|
});
|
||||||
assert.deepEqual(c.merged.map((f) => f.title).sort(), ['A lone CR does not split lines', 'EAGAIN on stdin']);
|
assert.deepEqual(c.merged.map((f) => f.title).sort(), ['A lone CR does not split lines', 'EAGAIN on stdin']);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ---- ORD sibling: the workflow's inlined dedup does not depend on arrival order ----
|
||||||
|
|
||||||
|
test('[ORD] sibling: workflow — conformance B with /code-review A and B, in both engine orders, gives one row of A alone and one row of both B findings', async () => {
|
||||||
|
const K = 'CODE_REVIEW_DEFECT';
|
||||||
|
const engineA = F(K, 'MINOR', { title: 'A' });
|
||||||
|
const engineB = F(K, 'MAJOR', { title: 'B' });
|
||||||
|
for (const codeFindings of [[engineA, engineB], [engineB, engineA]]) {
|
||||||
|
const name = codeFindings.map((f) => f.title).join(', ');
|
||||||
|
const c = await floorCase({
|
||||||
|
reviewerFindings: [F(K, 'MINOR', { title: 'B' })],
|
||||||
|
codeFindings,
|
||||||
|
returned: [],
|
||||||
|
verdict: 'WARN',
|
||||||
|
});
|
||||||
|
const rows = c.merged.filter((m) => triplet(m) === triplet(F(K, 'MINOR')));
|
||||||
|
assert.equal(rows.length, 2, `engine order ${name}: ${JSON.stringify(rows)}`);
|
||||||
|
const got = rows
|
||||||
|
.map((m) => ({ title: m.title, engine: m.engine ?? null, severity: m.severity }))
|
||||||
|
.sort((x, y) => x.title.localeCompare(y.title));
|
||||||
|
// A alone: the /code-review A finding survives with its own MINOR. Both B
|
||||||
|
// findings: the conformance B finding survives (a reviewer over an engine)
|
||||||
|
// at the MAJOR the /code-review B finding asserted.
|
||||||
|
assert.deepEqual(got, [
|
||||||
|
{ title: 'A', engine: 'code-review', severity: 'MINOR' },
|
||||||
|
{ title: 'B', engine: null, severity: 'MAJOR' },
|
||||||
|
], `engine order ${name}`);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[ORD] sibling: workflow — the title classes are placed in alphabetical order: conformance B with /code-review A, B and an untitled BLOCKER gives {A, untitled} BLOCKER kept as A and {B, B} MAJOR in all 6 engine orders', async () => {
|
||||||
|
const K = 'CODE_REVIEW_DEFECT';
|
||||||
|
const engineA = F(K, 'BLOCKER', { title: 'A' });
|
||||||
|
const engineB = F(K, 'MINOR', { title: 'B' });
|
||||||
|
const { title: _t, ...engineU } = F(K, 'BLOCKER');
|
||||||
|
// The workflow reads the reviewer findings first, so class B always arrives
|
||||||
|
// first: only the alphabetical order puts the untitled finding in A's row.
|
||||||
|
// A and the untitled finding tie on every survivor rule, so the first in the
|
||||||
|
// group's order (class A before the untitled findings) is kept.
|
||||||
|
const engineOrders = [
|
||||||
|
[engineA, engineB, engineU], [engineA, engineU, engineB], [engineB, engineA, engineU],
|
||||||
|
[engineB, engineU, engineA], [engineU, engineA, engineB], [engineU, engineB, engineA],
|
||||||
|
];
|
||||||
|
for (const codeFindings of engineOrders) {
|
||||||
|
const name = codeFindings.map((f) => f.title ?? 'U').join(', ');
|
||||||
|
const c = await floorCase({
|
||||||
|
reviewerFindings: [F(K, 'MAJOR', { title: 'B' })],
|
||||||
|
codeFindings,
|
||||||
|
returned: [],
|
||||||
|
verdict: 'WARN',
|
||||||
|
});
|
||||||
|
const rows = c.merged.filter((m) => triplet(m) === triplet(F(K, 'MINOR')));
|
||||||
|
const got = rows
|
||||||
|
.map((m) => ({ title: m.title ?? null, engine: m.engine ?? null, severity: m.severity }))
|
||||||
|
.sort((x, y) => String(x.title).localeCompare(String(y.title)));
|
||||||
|
assert.deepEqual(got, [
|
||||||
|
{ title: 'A', engine: 'code-review', severity: 'BLOCKER' },
|
||||||
|
{ title: 'B', engine: null, severity: 'MAJOR' },
|
||||||
|
], `engine order ${name}`);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue