fix(review): the Arm B workflow keeps the contract's row on a trimmed title and a tie

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-26 15:06:23 +01:00
commit 878871a6ad
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 164 additions and 8 deletions

View file

@ -724,7 +724,8 @@ test('[ORD] sibling: workflow — the title classes are placed in alphabetical o
// The script cannot import, so its groupTriplet is a hand-kept copy of
// lib/review/coordinator-contract.mjs's. Both run on the ORD fixtures of this
// file and of tests/lib/coordinator-contract.test.mjs, plus the WF floor
// fixture below (the one where two titles from two sources share a row), in
// fixture below (the one where two titles from two sources share a row) and
// the S3 tie of a conformance and a correctness finding, in
// every arrival order within a payload, and must give the same rows in the
// same order. Each finding
// carries its workflow source as `reviewer` (what the contract groups and
@ -780,6 +781,13 @@ function parityFixtures() {
add('conformance B with /code-review A, B and an untitled BLOCKER', {
f: [F(K, 'MAJOR', { title: 'B' })], e: [F(K, 'BLOCKER', { title: 'A' }), F(K, 'MINOR', { title: 'B' }), untitled],
});
// S3: one row of a conformance Z and a correctness A, a full tie on tier,
// source kind and severity; the alphabetical class order places A first, so
// only the rank "conformance wins a tie" keeps Z, as the contract does.
add('a conformance Z and a correctness A tied on one triplet (S3)', {
f: [{ ...slug, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 'Z' }],
c: [{ ...slug, rule_key: 'MISSING_TEST', severity: 'MAJOR', title: 'A' }],
});
return out;
}
@ -800,7 +808,7 @@ test('parity: the workflow dedup and coordinator-contract dedupByTriplet give th
runs++;
}
}
assert.equal(runs, 24, 'control: every fixture ran in each of its arrival orders');
assert.equal(runs, 25, 'control: every fixture ran in each of its arrival orders');
});
// ---- WF: the floor and the put-back match each returned finding to its own row ----
@ -909,6 +917,98 @@ test('[WF] the row key reads titles as sameTitle does: on sample titles a return
}
});
// ---- M5 / S3 / E20: the workflow keeps the contract's row ----
//
// M5: the coordinator trims an over-long MINOR/SUGGESTION title as the
// contract's Pass 2 does; the returned finding still matches its own row, so
// the put-back adds no copy of it. S3: on a conformance/correctness tie the
// workflow keeps the row the contract keeps. E20: the canonical order of
// untitled findings makes the kept row, and so the verdict, independent of
// the order the payloads arrive in.
test('[M5] a coordinator-trimmed title gives no duplicate put-back row', async () => {
const contract = await import('../../lib/review/coordinator-contract.mjs');
assert.equal(typeof contract.judgeFilter, 'function', 'the contract exports judgeFilter');
const K = 'CODE_REVIEW_IMPROVEMENT';
const long = `The cache key ignores the locale ${'x'.repeat(120)}`;
const finding = F(K, 'MINOR', { title: long });
// The title as the coordinator's Judge pass writes it (the contract's trim).
const trimmed = contract.judgeFilter([structuredClone(finding)]).kept[0].title;
assert.notEqual(trimmed, long, 'control: the contract trims this title');
const c = await floorCase({
codeFindings: [finding],
returned: [{ ...finding, title: trimmed }],
verdict: 'ALLOW',
});
assert.equal(c.merged.length, 1, 'one row');
const rows = at(c.r.findings, FILE, LINE, K);
assert.equal(rows.length, 1, `the trimmed finding is its own row's, nothing put back: ${JSON.stringify(rows.map((f) => f.title))}`);
assert.equal(rows[0].title, trimmed);
});
test('[M5] the workflow\'s inlined TRIM_MARKER and JUDGE_TITLE_MAX equal the contract\'s', async () => {
const contract = await import('../../lib/review/coordinator-contract.mjs');
assert.equal(typeof contract.TRIM_MARKER, 'string', 'the contract exports TRIM_MARKER');
assert.equal(typeof contract.JUDGE_TITLE_MAX, 'number', 'the contract exports JUDGE_TITLE_MAX');
const marker = SRC.match(/^const TRIM_MARKER = '([^']*)';$/m);
assert.ok(marker, 'the script carries a TRIM_MARKER literal');
assert.equal(marker[1], contract.TRIM_MARKER);
const max = SRC.match(/^const JUDGE_TITLE_MAX = (\d+);$/m);
assert.ok(max, 'the script carries a JUDGE_TITLE_MAX literal');
assert.equal(Number(max[1]), contract.JUDGE_TITLE_MAX);
});
test('[S3] on a conformance/correctness tie with different titles the workflow keeps the contract\'s row', async () => {
const K = 'MISSING_TEST';
const fZ = F(K, 'MAJOR', { title: 'Z' });
const cA = F(K, 'MAJOR', { title: 'A' });
const s = stubs({
reviewers: [
{ reviewer: 'brief-conformance-reviewer', findings: [structuredClone(fZ)] },
{ reviewer: 'code-correctness-reviewer', findings: [structuredClone(cA)] },
],
coordinator: { verdict: 'WARN', findings: [] },
});
await exec({ ...BASE, enginePayloads: BOTH_ENGINES() }, s);
const prompt = s.calls.coordinatorPrompt;
assert.ok(typeof prompt === 'string', 'the coordinator ran');
const merged = JSON.parse(prompt.slice(prompt.indexOf(PROMPT_JSON_MARK) + PROMPT_JSON_MARK.length));
const contractRows = dedupByTriplet([
{ ...structuredClone(fZ), reviewer: 'brief-conformance-reviewer' },
{ ...structuredClone(cA), reviewer: 'code-correctness-reviewer' },
]);
assert.deepEqual(contractRows.map((r) => r.title), ['Z'], 'control: the contract keeps the conformance finding');
assert.deepEqual(merged.map((r) => r.title), ['Z'], 'the workflow keeps the conformance finding, as the contract does');
});
test('[E20] untitled findings give one verdict in every arrival order', async () => {
// Two untitled engine MAJORs on one triplet, one from each engine: a full tie
// on tier, source kind and severity, so the first in the group's order is
// kept. The coordinator returns the row legally lowered with ALLOW; that
// stands only when the kept row is the /code-review one.
const K = 'CODE_REVIEW_DEFECT';
const { title: _t, ...untitled } = F(K, 'MAJOR');
const code = () => engine('claude-code-review', [structuredClone(untitled)]);
const security = () => engine('claude-security-review', [structuredClone(untitled)]);
const outcomes = new Set();
for (const [name, payloads] of [['code, security', [code(), security()]], ['security, code', [security(), code()]]]) {
const s = stubs({
reviewers: [{ reviewer: 'brief-conformance-reviewer', findings: [] }, { reviewer: 'code-correctness-reviewer', findings: [] }],
coordinator: {
verdict: 'ALLOW',
findings: [{ ...structuredClone(untitled), severity: 'MINOR', contradicts_brief: MARK, brief_lowered_from: 'MAJOR' }],
},
});
const r = await exec({ ...BASE, enginePayloads: payloads }, s);
assert.equal(r.missing_reviewers, 0, `${name}: every reviewer and engine delivered`);
outcomes.add(JSON.stringify({
verdict: r.verdict,
findings: r.findings.map((f) => `${f.engine ?? 'none'}|${f.severity}`).sort(),
}));
}
assert.equal(outcomes.size, 1, [...outcomes].join(' | '));
});
// ---- Architecture rule: the workflow script imports nothing (it runs inside the Workflow tool) ----
// Every module specifier a source text imports, statically or dynamically.