open/voyage
1
0
Fork 0
Code Issues Releases 13 Activity

fix(review): the workflow review floors an illegally lowered brief-conflict finding

Browse source 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-12 18:16:07 +02:00
commit 8cc6ba0181
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
2 changed files with 331 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

82
scripts/trekreview-armB.workflow.mjs
View file

@ -43,6 +43,20 @@ const RULE_KEYS = [
'CODE_REVIEW_DEFECT', 'CODE_REVIEW_IMPROVEMENT', 'SECURITY_REVIEW_VULNERABILITY',
];
const SEVERITIES = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
// Every catalogue rule_key -> its tier (lib/review/rule-catalogue.mjs; scripts
// cannot import, so it is inlined and pinned equal by the workflow test).
const RULE_TIERS = {
MISSING_BRIEF_REF: 'MAJOR', UNIMPLEMENTED_CRITERION: 'BLOCKER', SCOPE_CREEP_BUILT: 'MAJOR',
NON_GOAL_VIOLATED: 'BLOCKER', MISSING_TEST: 'MAJOR', SECURITY_INJECTION: 'BLOCKER',
PLACEHOLDER_IN_CODE: 'MAJOR', MISSING_ERROR_HANDLING: 'MINOR', UNDECLARED_DEPENDENCY: 'MAJOR',
PLAN_EXECUTE_DRIFT: 'MAJOR', BROKEN_SUCCESS_CRITERION: 'BLOCKER', COVERAGE_SILENT_SKIP: 'MAJOR',
CODE_REVIEW_DEFECT: 'MAJOR', CODE_REVIEW_IMPROVEMENT: 'MINOR', SECURITY_REVIEW_VULNERABILITY: 'BLOCKER',
};
// Lower number = higher severity; an unknown severity ranks last (as
// lib/review/coordinator-contract.mjs severityRank does).
const sevRank = (s) => (SEVERITIES.includes(s) ? SEVERITIES.indexOf(s) : SEVERITIES.length);
// The catalogue tier of a rule_key, or null when the key is missing or unknown.
const tierOf = (key) => (typeof key === 'string' && Object.prototype.hasOwnProperty.call(RULE_TIERS, key) ? RULE_TIERS[key] : null);
// Findings schema — mirrors lib/review/findings-schema.mjs (NW1). The enum on
// rule_key enforces the catalogue at the StructuredOutput layer (stronger than
@ -94,6 +108,9 @@ const VERDICT_SCHEMA = {
line: { type: 'integer', minimum: 0 },
title: { type: 'string' },
detail: { type: 'string' },
engine: { type: 'string', enum: ['code-review', 'security-review'] },
contradicts_brief: { type: 'string' },
brief_lowered_from: { type: 'string', enum: ['MAJOR'] },
},
},
},
@ -102,12 +119,18 @@ const VERDICT_SCHEMA = {
// Inlined dedup by (file,line,rule_key) — the JS-side replacement for the
// coordinator's pass-1 triplet dedup (T2 §5 / lib/review/plan-review-dedup.mjs
// pattern; scripts cannot import, so the triplet key is inlined).
// pattern; scripts cannot import, so the triplet key is inlined). On a
// repeated triplet the higher severity stays; at equal severity the finding
// with an `engine` stays; otherwise the first-seen one stays.
function dedupByTriplet(findings) {
const seen = new Map();
for (const f of findings) {
const key = `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
if (!seen.has(key)) seen.set(key, f);
if (!seen.has(key)) { seen.set(key, f); continue; }
const kept = seen.get(key);
const higher = sevRank(f.severity) < sevRank(kept.severity);
const tieToEngine = sevRank(f.severity) === sevRank(kept.severity) && Boolean(f.engine) && !kept.engine;
if (higher || tieToEngine) seen.set(key, f);
}
return [...seen.values()];
}
@ -137,6 +160,16 @@ function coordinatorPrompt(merged, a) {
'Return {verdict, findings} via StructuredOutput. Synthesis across files is forbidden.',
`The brief (the contract) is at: ${a.briefPath} — Read it if needed for context.`,
'',
'=== BRIEF CHOICES (the brief-conflict rule) ===',
`Read the brief's choices at ${a.briefPath}: \`## Non-Goals\`, \`## Constraints\`, \`## Preferences\`,`,
'and the operator decisions in `## Open Questions / Assumptions`.',
'Only an engine finding (`engine`: code-review or security-review) whose recommended action',
'contradicts one of those choices is marked, with `contradicts_brief: "<the Non-Goal or choice, verbatim>"`.',
'Only a /code-review finding at the MAJOR tier and at severity MAJOR then drops to MINOR, with',
"`brief_lowered_from: 'MAJOR'`. Every other marked finding keeps its severity. A reviewer-agent finding",
'(no `engine`) and a finding whose rule_key is not in the catalogue are never marked. A brief-conflicting',
'finding is kept and marked, never removed. Each returned finding keeps its `engine` and `rule_key`.',
'',
'=== PRE-DEDUPED FINDINGS (JSON) ===',
JSON.stringify(merged, null, 2),
].join('\n');
@ -171,8 +204,22 @@ const expectedEngines = a.quick ? ['claude-code-review'] : ['claude-code-review'
const arrivedEngines = new Set(enginePayloads.map((p) => p.reviewer));
const missingEngines = expectedEngines.filter((name) => !arrivedEngines.has(name));
const missingReviewers = (rawResults.length - reviewerResults.length) + missingEngines.length;
const allFindings = [...reviewerResults, ...enginePayloads]
.flatMap((r) => (Array.isArray(r?.findings) ? r.findings : []));
// Provenance, before dedup: "came from an engine" is read from the source,
// never from an `engine` field. A reviewer finding loses any `engine` it
// carries; an engine payload's findings get `engine` from its `reviewer`
// (the names lib/review/engine-findings.mjs writes); any other payload gives
// its findings no `engine`. So an `engine` in `merged` means an engine payload.
const engineOf = (reviewer) => (reviewer === 'claude-code-review' ? 'code-review'
: reviewer === 'claude-security-review' ? 'security-review' : null);
const withEngine = (f, eng) => {
if (!f || typeof f !== 'object') return f;
const { engine: _dropped, ...rest } = f;
return eng ? { ...rest, engine: eng } : rest;
};
const allFindings = [
...reviewerResults.flatMap((r) => (Array.isArray(r?.findings) ? r.findings : []).map((f) => withEngine(f, null))),
...enginePayloads.flatMap((p) => (Array.isArray(p?.findings) ? p.findings : []).map((f) => withEngine(f, engineOf(p?.reviewer)))),
];
const merged = dedupByTriplet(allFindings);
log(`Arm B: ${reviewerResults.length} reviewer(s); ${allFindings.length} raw → ${merged.length} after triplet-dedup`);
@ -190,6 +237,33 @@ if (merged.length === 0) {
// The verdict is mechanical: recompute its floor from the findings the
// coordinator returned, so an LLM-written ALLOW over a BLOCKER cannot pass.
const returned = Array.isArray(coord?.findings) ? coord.findings : merged;
// The brief-conflict floor: every returned finding is judged from its
// counterpart in the coordinator's input (`merged`, same file/line/rule_key),
// never from the labels the coordinator wrote. The raise is written into
// `returned`, the mark kept, so the rendered review.md carries it.
const counterparts = new Map(merged
.filter((m) => m && typeof m === 'object')
.map((m) => [`${m.file}\u0000${m.line}\u0000${m.rule_key}`, m]));
const present = (f, field) => f[field] !== undefined && f[field] !== null;
const raiseTo = (f, sev) => { if (sevRank(sev) < sevRank(f.severity)) f.severity = sev; };
for (const f of returned) {
if (!f || typeof f !== 'object') continue;
const c = counterparts.get(`${f.file}\u0000${f.line}\u0000${f.rule_key}`);
const labelled = present(f, 'engine') || present(f, 'contradicts_brief') || present(f, 'brief_lowered_from');
if (c && c.engine) {
// (1) an engine counterpart: raised to its severity, unless the one legal lowering.
const legal = c.engine === 'code-review' && c.severity === 'MAJOR' && tierOf(c.rule_key) === 'MAJOR' &&
f.severity === 'MINOR' && typeof f.contradicts_brief === 'string' && f.contradicts_brief.trim().length > 0 &&
f.brief_lowered_from === 'MAJOR';
if (!legal) raiseTo(f, c.severity);
f.engine = c.engine;
} else if (labelled) {
// (2) a reviewer counterpart, (3) no counterpart: an unknown key raises to BLOCKER.
raiseTo(f, tierOf(f.rule_key) ?? 'BLOCKER');
if (c) raiseTo(f, c.severity);
delete f.engine;
}
}
const has = (sev) => returned.some((f) => f && f.severity === sev);
let verdict = coord?.verdict ?? null;
if (has('BLOCKER')) verdict = 'BLOCK';

254
tests/scripts/trekreview-armb-workflow.test.mjs
View file

@ -9,6 +9,8 @@ import { strict as assert } from 'node:assert';
import { readFileSync } from 'node:fs';
import { join, dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
import { illegalBriefLowering } from '../../lib/review/coordinator-contract.mjs';
import { RULE_CATALOGUE } from '../../lib/review/rule-catalogue.mjs';
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
const SRC = readFileSync(join(ROOT, 'scripts', 'trekreview-armB.workflow.mjs'), 'utf-8')
@ -21,8 +23,17 @@ const engine = (reviewer, findings = []) => ({ reviewer, findings });
function stubs({ reviewers = [], coordinator = null } = {}) {
let i = 0;
const calls = { coordinatorPrompt: null, coordinatorOpts: null };
return {
agent: async (_prompt, opts) => (opts.label === 'coordinator' ? coordinator : reviewers[i++] ?? null),
calls,
agent: async (prompt, opts) => {
if (opts.label === 'coordinator') {
calls.coordinatorPrompt = prompt;
calls.coordinatorOpts = opts;
return coordinator;
}
return reviewers[i++] ?? null;
},
parallel: async (thunks) => Promise.all(thunks.map((t) => t())),
phase: () => {},
log: () => {},
@ -69,3 +80,244 @@ test('armB — quick mode expects only the /code-review engine', async () => {
assert.equal(r.verdict, 'ALLOW');
assert.deepEqual(r.missing_engines, []);
});
// ---- Step 9: the brief-conflict rule on the Workflow path ---------------------------
//
// Each case stubs the reviewers' and engines' payloads (so `merged` holds the
// input), the coordinator's returned findings and its verdict, then asserts the
// workflow's verdict. `merged` is read from the prompt the coordinator stub
// receives. For every case the test states the value of `illegalBriefLowering`
// on the subject finding as returned, with `engine` set from its counterpart's
// source (removed when the counterpart is a reviewer finding or absent), and
// checks the floor raised a finding exactly where that value is true — plus
// case 7, where a false `engine` label alone raises.
const RANK = ['BLOCKER', 'MAJOR', 'MINOR', 'SUGGESTION'];
const rank = (s) => (RANK.indexOf(s) === -1 ? RANK.length : RANK.indexOf(s));
const MARK = 'Non-Goal: no new runtime dependency';
const FILE = 'src/a.mjs';
const LINE = 10;
const F = (rule_key, severity, extra = {}) => ({ severity, rule_key, file: FILE, line: LINE, title: 't', detail: 'd', ...extra });
const PROMPT_JSON_MARK = '=== PRE-DEDUPED FINDINGS (JSON) ===';
const triplet = (f) => `${f.file}\u0000${f.line}\u0000${f.rule_key}`;
async function floorCase({ reviewerFindings = [], codeFindings = [], securityFindings = [], quick = false, returned, verdict }) {
const before = structuredClone(returned);
const coordinator = { verdict, findings: structuredClone(returned) };
const reviewers = quick
? [{ reviewer: 'code-correctness', findings: structuredClone(reviewerFindings) }]
: [{ reviewer: 'brief-conformance', findings: structuredClone(reviewerFindings) }, { reviewer: 'code-correctness', findings: [] }];
const enginePayloads = quick
? [engine('claude-code-review', structuredClone(codeFindings))]
: [engine('claude-code-review', structuredClone(codeFindings)), engine('claude-security-review', structuredClone(securityFindings))];
const s = stubs({ reviewers, coordinator });
const r = await exec({ ...BASE, quick, enginePayloads }, 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 raised = r.findings.some((f, i) => rank(f.severity) < rank(before[i].severity));
return { r, merged, before, raised, s };
}
// The subject (index 0 of `returned`) with `engine` from its counterpart's source.
function subjectIllegal(before, merged, idx = 0) {
const subject = { ...before[idx] };
const counterpart = merged.find((m) => triplet(m) === triplet(subject));
if (counterpart && counterpart.engine) subject.engine = counterpart.engine;
else delete subject.engine;
return illegalBriefLowering(subject);
}
function checkRaise({ before, merged, raised }, statedIllegal, extraRaise = false) {
assert.equal(subjectIllegal(before, merged), statedIllegal, 'illegalBriefLowering on the subject');
assert.equal(raised, statedIllegal || extraRaise, 'the floor raised a finding exactly where the shape is illegal');
}
test('armB brief rule (1) — an unlabelled SECURITY_REVIEW_VULNERABILITY returned at MINOR over a /security-review BLOCKER → BLOCK', async () => {
const c = await floorCase({
securityFindings: [F('SECURITY_REVIEW_VULNERABILITY', 'BLOCKER')],
returned: [F('SECURITY_REVIEW_VULNERABILITY', 'MINOR')],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'BLOCK');
assert.equal(c.r.findings[0].engine, 'security-review');
checkRaise(c, true);
});
test('armB brief rule (2) — an unlabelled CODE_REVIEW_DEFECT returned at MINOR over a /code-review MAJOR with ALLOW → WARN', async () => {
const c = await floorCase({
codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
returned: [F('CODE_REVIEW_DEFECT', 'MINOR')],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'WARN');
checkRaise(c, true);
});
for (const withEngine of [true, false]) {
test(`armB brief rule (3) — the legal lowering keeps ALLOW (returned ${withEngine ? 'with' : 'without'} engine) and carries engine code-review`, async () => {
const lowered = F('CODE_REVIEW_DEFECT', 'MINOR', { contradicts_brief: MARK, brief_lowered_from: 'MAJOR' });
if (withEngine) lowered.engine = 'code-review';
const c = await floorCase({
codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
returned: [lowered],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'ALLOW');
assert.equal(c.r.findings[0].engine, 'code-review');
assert.equal(c.r.findings[0].severity, 'MINOR');
checkRaise(c, false);
});
}
test('armB brief rule (4) — the legal shape over a reviewer CODE_REVIEW_DEFECT asserted BLOCKER (relabelled engine) → BLOCK, no engine', async () => {
const c = await floorCase({
reviewerFindings: [F('CODE_REVIEW_DEFECT', 'BLOCKER')],
returned: [F('CODE_REVIEW_DEFECT', 'MINOR', { engine: 'code-review', contradicts_brief: MARK, brief_lowered_from: 'MAJOR' })],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'BLOCK');
assert.equal(c.r.findings[0].engine, undefined);
checkRaise(c, true);
});
test('armB brief rule (5) — a reviewer finding that carries engine code-review is not an engine finding: the source decides → WARN', async () => {
const c = await floorCase({
reviewerFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR', { engine: 'code-review' })],
returned: [F('CODE_REVIEW_DEFECT', 'MINOR', { engine: 'code-review', contradicts_brief: MARK, brief_lowered_from: 'MAJOR' })],
verdict: 'ALLOW',
});
assert.equal(c.merged[0].engine, undefined, 'a reviewer engine field never reaches merged');
assert.equal(c.r.verdict, 'WARN');
checkRaise(c, true);
});
test('armB brief rule (6) — a marked reviewer MISSING_TEST asserted BLOCKER, returned at MAJOR with WARN → BLOCK', async () => {
const c = await floorCase({
reviewerFindings: [F('MISSING_TEST', 'BLOCKER')],
returned: [F('MISSING_TEST', 'MAJOR', { contradicts_brief: MARK })],
verdict: 'WARN',
});
assert.equal(c.r.verdict, 'BLOCK');
checkRaise(c, true);
});
test('armB brief rule (7) — a reviewer MISSING_TEST asserted BLOCKER, returned at MAJOR with only engine security-review → BLOCK, no engine', async () => {
const c = await floorCase({
reviewerFindings: [F('MISSING_TEST', 'BLOCKER')],
returned: [F('MISSING_TEST', 'MAJOR', { engine: 'security-review' })],
verdict: 'WARN',
});
assert.equal(c.r.verdict, 'BLOCK');
assert.equal(c.r.findings[0].engine, undefined);
checkRaise(c, false, true);
});
test('armB brief rule (8) — a stray brief_lowered_from on a reviewer finding asserted BLOCKER, returned at MAJOR with WARN → BLOCK', async () => {
const c = await floorCase({
reviewerFindings: [F('MISSING_TEST', 'BLOCKER')],
returned: [F('MISSING_TEST', 'MAJOR', { brief_lowered_from: 'MAJOR' })],
verdict: 'WARN',
});
assert.equal(c.r.verdict, 'BLOCK');
checkRaise(c, true);
});
test('armB brief rule (9) — a marked finding with no counterpart and an unknown rule_key, returned at MINOR with ALLOW → BLOCK', async () => {
const other = { severity: 'MINOR', rule_key: 'MISSING_ERROR_HANDLING', file: 'src/b.mjs', line: 3, title: 't', detail: 'd' };
const c = await floorCase({
reviewerFindings: [other],
returned: [{ severity: 'MINOR', rule_key: 'NOT_IN_THE_CATALOGUE', file: 'src/ghost.mjs', line: 7, title: 't', detail: 'd', contradicts_brief: MARK }, other],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'BLOCK');
checkRaise(c, true);
});
test('armB brief rule (10) — a marked BLOCKER-tier /code-review finding returned at MINOR → BLOCK', async () => {
const c = await floorCase({
codeFindings: [F('SECURITY_INJECTION', 'BLOCKER')],
returned: [F('SECURITY_INJECTION', 'MINOR', { engine: 'code-review', contradicts_brief: MARK })],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'BLOCK');
checkRaise(c, true);
});
test('armB brief rule (11) — dedup keeps the /code-review MAJOR over a first-seen reviewer MINOR on the same triplet → WARN', async () => {
const c = await floorCase({
reviewerFindings: [F('CODE_REVIEW_DEFECT', 'MINOR')],
codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
returned: [F('CODE_REVIEW_DEFECT', 'MINOR')],
verdict: 'ALLOW',
});
assert.equal(c.merged.length, 1);
assert.equal(c.merged[0].severity, 'MAJOR');
assert.equal(c.merged[0].engine, 'code-review');
assert.equal(c.r.deduped_count, 1);
assert.equal(c.r.raw_finding_count, 2);
assert.equal(c.r.verdict, 'WARN');
checkRaise(c, true);
});
test('armB brief rule (12) — an unlabelled reviewer MISSING_TEST asserted BLOCKER, returned at MAJOR (catalogue reset) with WARN stays WARN', async () => {
const c = await floorCase({
reviewerFindings: [F('MISSING_TEST', 'BLOCKER')],
returned: [F('MISSING_TEST', 'MAJOR')],
verdict: 'WARN',
});
assert.equal(c.r.verdict, 'WARN');
checkRaise(c, false);
});
test('armB brief rule (13) — a marked MINOR-tier engine finding at its tier (CODE_REVIEW_IMPROVEMENT) keeps ALLOW', async () => {
const c = await floorCase({
codeFindings: [F('CODE_REVIEW_IMPROVEMENT', 'MINOR')],
returned: [F('CODE_REVIEW_IMPROVEMENT', 'MINOR', { engine: 'code-review', contradicts_brief: MARK })],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'ALLOW');
assert.equal(c.r.findings[0].engine, 'code-review');
checkRaise(c, false);
});
test('armB brief rule (14) — an unlabelled reviewer MISSING_TEST at MINOR (quick mode) keeps the coordinator verdict', async () => {
const c = await floorCase({
quick: true,
reviewerFindings: [F('MISSING_TEST', 'MINOR')],
returned: [F('MISSING_TEST', 'MINOR')],
verdict: 'ALLOW',
});
assert.equal(c.r.verdict, 'ALLOW');
checkRaise(c, false);
});
test('armB brief rule — RULE_TIERS in the script equals the catalogue (same keys, same tiers)', () => {
const start = SRC.indexOf('const RULE_TIERS = {');
assert.ok(start !== -1, 'the script carries a RULE_TIERS literal block');
const block = SRC.slice(start, SRC.indexOf('};', start));
const tiers = Object.fromEntries([...block.matchAll(/([A-Z_]+):\s*'([A-Z]+)'/g)].map((m) => [m[1], m[2]]));
const catalogue = Object.fromEntries(RULE_CATALOGUE.map((r) => [r.rule_key, r.severity]));
assert.deepEqual(tiers, catalogue);
});
test('armB brief rule — VERDICT_SCHEMA declares engine, contradicts_brief and brief_lowered_from; the prompt states the rule', async () => {
const c = await floorCase({
codeFindings: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
returned: [F('CODE_REVIEW_DEFECT', 'MAJOR')],
verdict: 'WARN',
});
const props = c.s.calls.coordinatorOpts.schema.properties.findings.items.properties;
assert.deepEqual(props.engine, { type: 'string', enum: ['code-review', 'security-review'] });
assert.deepEqual(props.contradicts_brief, { type: 'string' });
assert.deepEqual(props.brief_lowered_from, { type: 'string', enum: ['MAJOR'] });
const prompt = c.s.calls.coordinatorPrompt;
assert.ok(prompt.includes('contradicts_brief'));
assert.ok(prompt.includes('brief_lowered_from'));
assert.ok(prompt.includes('## Non-Goals'));
assert.ok(prompt.includes('## Constraints'));
assert.ok(prompt.includes('## Preferences'));
assert.ok(prompt.includes('## Open Questions / Assumptions'));
assert.ok(/only an engine finding/i.test(prompt), 'engine findings only');
assert.ok(/kept and marked, never removed/i.test(prompt));
});