fix(review): the workflow review fails closed when its coordinator returns nothing
Review finding edf2ec56; every reviewer that did not run is named in allow_blocked_by. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
9529df7665
commit
25856f2e9c
4 changed files with 166 additions and 8 deletions
|
|
@ -82,6 +82,136 @@ test('armB — quick mode expects only the /code-review engine', async () => {
|
|||
assert.deepEqual(r.missing_engines, []);
|
||||
});
|
||||
|
||||
// ---- Review edf2ec56: a coordinator that returns nothing fails closed ----------------
|
||||
//
|
||||
// A coordinator that returns no verdict and findings counts as a missing reviewer:
|
||||
// its partial list is not trusted (the floors read every merged finding, each raised
|
||||
// to its catalogue tier, never lowered), ALLOW is forbidden, a BLOCK is never lowered,
|
||||
// and every reviewer, engine and coordinator that did not deliver is named in
|
||||
// `allow_blocked_by` as `missing-reviewer:<name>`.
|
||||
|
||||
const COORD_MISSING = 'missing-reviewer:review-coordinator';
|
||||
const MEH_MINOR = () => ({ severity: 'MINOR', rule_key: 'MISSING_ERROR_HANDLING', file: 'src/r.mjs', line: 4, title: 't', detail: 'd' });
|
||||
const CRI_MINOR = () => ({ severity: 'MINOR', rule_key: 'CODE_REVIEW_IMPROVEMENT', file: 'src/e.mjs', line: 8, title: 't', detail: 'd' });
|
||||
const BOTH_ENGINES = (code = [], security = []) => [engine('claude-code-review', code), engine('claude-security-review', security)];
|
||||
|
||||
async function coordCase({ coordinator, reviewerFindings = [MEH_MINOR()], codeFindings = [CRI_MINOR()] }) {
|
||||
const s = stubs({
|
||||
reviewers: [{ reviewer: 'brief-conformance', findings: reviewerFindings }, { reviewer: 'code-correctness', findings: [] }],
|
||||
coordinator,
|
||||
});
|
||||
const r = await exec({ ...BASE, enginePayloads: BOTH_ENGINES(codeFindings) }, s);
|
||||
return { r, s };
|
||||
}
|
||||
|
||||
const failedCoordinators = [
|
||||
['returns null', () => null],
|
||||
['returns findings (a partial list) and no verdict', () => ({ findings: [CRI_MINOR()] })],
|
||||
['returns ALLOW with no findings array', () => ({ verdict: 'ALLOW' })],
|
||||
];
|
||||
for (const [label, make] of failedCoordinators) {
|
||||
test(`edf2ec56 — a coordinator that ${label} → WARN, every merged finding still MINOR, the coordinator named`, async () => {
|
||||
const { r, s } = await coordCase({ coordinator: make() });
|
||||
assert.ok(typeof s.calls.coordinatorPrompt === 'string', 'the coordinator was called');
|
||||
assert.equal(r.verdict, 'WARN');
|
||||
assert.deepEqual(r.findings.map((f) => f.rule_key).sort(), ['CODE_REVIEW_IMPROVEMENT', 'MISSING_ERROR_HANDLING'],
|
||||
'the floors read every merged finding, not a partial list');
|
||||
for (const f of r.findings) assert.equal(f.severity, 'MINOR');
|
||||
assert.equal(r.missing_reviewers, 1);
|
||||
assert.deepEqual(r.allow_blocked_by, [COORD_MISSING]);
|
||||
});
|
||||
}
|
||||
|
||||
test('edf2ec56 — a coordinator returning an empty findings array and no verdict over a reviewer BLOCKER → BLOCK, the BLOCKER in findings', async () => {
|
||||
const blocker = { severity: 'BLOCKER', rule_key: 'UNIMPLEMENTED_CRITERION', file: 'src/r.mjs', line: 2, title: 't', detail: 'd' };
|
||||
const { r } = await coordCase({ coordinator: { findings: [] }, reviewerFindings: [blocker], codeFindings: [] });
|
||||
assert.equal(r.verdict, 'BLOCK');
|
||||
assert.equal(r.findings.filter((f) => f.rule_key === 'UNIMPLEMENTED_CRITERION' && f.severity === 'BLOCKER').length, 1);
|
||||
assert.deepEqual(r.allow_blocked_by, [COORD_MISSING]);
|
||||
});
|
||||
|
||||
test('edf2ec56 — a coordinator returning BLOCK and no findings array → BLOCK (never lowered)', async () => {
|
||||
const { r } = await coordCase({ coordinator: { verdict: 'BLOCK' } });
|
||||
assert.equal(r.verdict, 'BLOCK');
|
||||
assert.deepEqual(r.allow_blocked_by, [COORD_MISSING]);
|
||||
});
|
||||
|
||||
test('edf2ec56 — a coordinator returning the verdict PASS (outside BLOCK/WARN/ALLOW) → WARN, the coordinator named', async () => {
|
||||
const { r } = await coordCase({ coordinator: { verdict: 'PASS', findings: [MEH_MINOR(), CRI_MINOR()] } });
|
||||
assert.equal(r.verdict, 'WARN');
|
||||
assert.ok(r.allow_blocked_by.includes(COORD_MISSING));
|
||||
});
|
||||
|
||||
test('edf2ec56 — no engine payloads: both engines are named in allow_blocked_by, WARN', async () => {
|
||||
const s = stubs({
|
||||
reviewers: [{ reviewer: 'brief-conformance', findings: [MEH_MINOR()] }, { reviewer: 'code-correctness', findings: [] }],
|
||||
coordinator: { verdict: 'ALLOW', findings: [MEH_MINOR()] },
|
||||
});
|
||||
const r = await exec({ ...BASE }, s);
|
||||
assert.equal(r.verdict, 'WARN');
|
||||
assert.deepEqual(r.allow_blocked_by, ['missing-reviewer:claude-code-review', 'missing-reviewer:claude-security-review']);
|
||||
});
|
||||
|
||||
test('edf2ec56 — a first reviewer that returns null is named missing-reviewer:brief-conformance-reviewer', async () => {
|
||||
const s = stubs({
|
||||
reviewers: [null, { reviewer: 'code-correctness', findings: [MEH_MINOR()] }],
|
||||
coordinator: { verdict: 'ALLOW', findings: [MEH_MINOR()] },
|
||||
});
|
||||
const r = await exec({ ...BASE, enginePayloads: BOTH_ENGINES() }, s);
|
||||
assert.equal(r.verdict, 'WARN');
|
||||
assert.deepEqual(r.allow_blocked_by, ['missing-reviewer:brief-conformance-reviewer']);
|
||||
});
|
||||
|
||||
test('edf2ec56 — quick mode: a correctness reviewer that returns null is named missing-reviewer:code-correctness-reviewer, no brief-conformance-reviewer', async () => {
|
||||
const s = stubs({ reviewers: [null], coordinator: { verdict: 'ALLOW', findings: [CRI_MINOR()] } });
|
||||
const r = await exec({ ...BASE, quick: true, enginePayloads: [engine('claude-code-review', [CRI_MINOR()])] }, s);
|
||||
assert.equal(r.verdict, 'WARN');
|
||||
assert.deepEqual(r.allow_blocked_by, ['missing-reviewer:code-correctness-reviewer']);
|
||||
assert.ok(!r.allow_blocked_by.includes('missing-reviewer:brief-conformance-reviewer'));
|
||||
});
|
||||
|
||||
test('edf2ec56 — a null coordinator: a MISSING_TEST asserted MINOR is read at MAJOR (its tier), a MISSING_ERROR_HANDLING asserted BLOCKER stays BLOCKER → BLOCK', async () => {
|
||||
const mt = { severity: 'MINOR', rule_key: 'MISSING_TEST', file: 'src/r.mjs', line: 5, title: 't', detail: 'd' };
|
||||
const mehBlocker = { severity: 'BLOCKER', rule_key: 'MISSING_ERROR_HANDLING', file: 'src/r.mjs', line: 6, title: 't', detail: 'd' };
|
||||
const { r } = await coordCase({ coordinator: null, reviewerFindings: [mt, mehBlocker], codeFindings: [] });
|
||||
const sevOf = (key) => r.findings.find((f) => f.rule_key === key)?.severity;
|
||||
assert.equal(sevOf('MISSING_TEST'), 'MAJOR', 'raised to its catalogue tier');
|
||||
assert.equal(sevOf('MISSING_ERROR_HANDLING'), 'BLOCKER', 'a raise, never a reset');
|
||||
assert.equal(r.verdict, 'BLOCK');
|
||||
});
|
||||
|
||||
test('edf2ec56 control — a coordinator returning ALLOW with the MINOR findings and both engines → ALLOW, nothing named', async () => {
|
||||
const { r } = await coordCase({ coordinator: { verdict: 'ALLOW', findings: [MEH_MINOR(), CRI_MINOR()] } });
|
||||
assert.equal(r.verdict, 'ALLOW');
|
||||
assert.equal(r.missing_reviewers, 0);
|
||||
assert.deepEqual(r.allow_blocked_by, []);
|
||||
});
|
||||
|
||||
test('edf2ec56 control — no findings at all → ALLOW, the coordinator skipped and not named', async () => {
|
||||
const { r, s } = await coordCase({ coordinator: null, reviewerFindings: [], codeFindings: [] });
|
||||
assert.equal(s.calls.coordinatorPrompt, null, 'the coordinator was skipped');
|
||||
assert.equal(r.verdict, 'ALLOW');
|
||||
assert.deepEqual(r.allow_blocked_by, []);
|
||||
assert.ok(!r.allow_blocked_by.includes(COORD_MISSING));
|
||||
});
|
||||
|
||||
test('edf2ec56 control — a BLOCKER with a null coordinator → BLOCK', async () => {
|
||||
const blocker = { severity: 'BLOCKER', rule_key: 'UNIMPLEMENTED_CRITERION', file: 'src/r.mjs', line: 2, title: 't', detail: 'd' };
|
||||
const { r } = await coordCase({ coordinator: null, reviewerFindings: [blocker], codeFindings: [] });
|
||||
assert.equal(r.verdict, 'BLOCK');
|
||||
assert.deepEqual(r.allow_blocked_by, [COORD_MISSING]);
|
||||
});
|
||||
|
||||
test('edf2ec56 — the workflow section of commands/trekreview.md and Handover 6 name the coordinator; the section names allow_blocked_by', () => {
|
||||
const cmd = readFileSync(join(ROOT, 'commands', 'trekreview.md'), 'utf-8');
|
||||
const handover = readFileSync(join(ROOT, 'docs', 'HANDOVER-CONTRACTS.md'), 'utf-8');
|
||||
const wf = section(cmd, '## Phase 5–6 via the Workflow substrate', '## Phase 7');
|
||||
assert.match(wf, /missing-reviewer:review-coordinator/);
|
||||
assert.match(wf, /allow_blocked_by/);
|
||||
const h6 = section(handover, '## Handover 6 — `review.md` → plan', '\n## Handover 7');
|
||||
assert.match(h6, /missing-reviewer:review-coordinator/);
|
||||
});
|
||||
|
||||
// ---- Step 9: the brief-conflict rule on the Workflow path ---------------------------
|
||||
//
|
||||
// Each case stubs the reviewers' and engines' payloads (so `merged` holds the
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue