/simplify over bcf3adb..HEAD. Pass 3's last else could never run (RULE_KEYS is checked first); entryPaths tracks an open entry the way testFirstFileEntries does; wrapLine drops normalisations its callers already made. Tests: one payload builder, one BLOCKER fixture, one unreleasedBody helper, one 1.9-block loop, one plan-unreadable loop, and make_repo in the e2e script. No behaviour change; suite 2515 (2513/0/2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
683 lines
38 KiB
JavaScript
683 lines
38 KiB
JavaScript
// tests/review/brief-conflict-findings.test.mjs
|
|
// The brief-conflict rule (plan 2026-10-11-files-backticks-trial4, Step 8).
|
|
//
|
|
// A `/code-review` finding at the MAJOR tier whose recommended action
|
|
// contradicts a named brief Non-Goal (or stated choice) is kept, marked with
|
|
// `contradicts_brief: "<the Non-Goal, verbatim>"`, and drops to MINOR with
|
|
// `brief_lowered_from: 'MAJOR'`. Nothing else is ever lowered by the mark: a
|
|
// BLOCKER stays BLOCKER, a `/security-review` finding keeps its severity, a
|
|
// reviewer-agent finding (no `engine`) is never marked. The verdict gate
|
|
// refuses (exit 2) every shape outside the one legal one.
|
|
//
|
|
// The new exports are probed via `import * as m` so a missing export is an
|
|
// assertion failure, never a load-time crash. Fixtures are read inside tests.
|
|
|
|
import { test } from 'node:test';
|
|
import { strict as assert } from 'node:assert';
|
|
import { spawnSync } from 'node:child_process';
|
|
import { readFileSync, existsSync } from 'node:fs';
|
|
import { join, dirname } from 'node:path';
|
|
import { fileURLToPath } from 'node:url';
|
|
import * as m from '../../lib/review/coordinator-contract.mjs';
|
|
import { extractFindingsBlock } from '../../lib/review/findings-schema.mjs';
|
|
import { toReviewerPayload } from '../../lib/review/engine-findings.mjs';
|
|
|
|
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
|
const GATE = join(ROOT, 'lib', 'review', 'verdict-gate.mjs');
|
|
const LEGAL_FIXTURE = join(ROOT, 'tests', 'fixtures', 'review-brief-conflict-legal.md');
|
|
const ILLEGAL_FIXTURE = join(ROOT, 'tests', 'fixtures', 'review-brief-conflict-illegal.md');
|
|
|
|
const NON_GOAL = 'No retry layer around the HTTP client (Non-Goal 2)';
|
|
|
|
function illegal(f) {
|
|
assert.equal(typeof m.illegalBriefLowering, 'function',
|
|
'coordinator-contract.mjs must export illegalBriefLowering(f)');
|
|
return m.illegalBriefLowering(f);
|
|
}
|
|
|
|
function lowerable(f) {
|
|
assert.equal(typeof m.lowerableBriefConflict, 'function',
|
|
'coordinator-contract.mjs must export lowerableBriefConflict(f)');
|
|
return m.lowerableBriefConflict(f);
|
|
}
|
|
|
|
function codeReviewDefect(extra = {}) {
|
|
return {
|
|
file: 'lib/http.mjs', line: 42, rule_key: 'CODE_REVIEW_DEFECT', severity: 'MAJOR',
|
|
title: 'No retry on transient failure', detail: 'A 503 fails the call.',
|
|
recommended_action: 'Wrap the call in a retry with backoff.', engine: 'code-review',
|
|
...extra,
|
|
};
|
|
}
|
|
|
|
function payload(reviewer, findings) {
|
|
return [{ reviewer, findings }];
|
|
}
|
|
|
|
function legalShape(extra = {}) {
|
|
return codeReviewDefect({
|
|
id: 'd3c22f4b1309d6001a0f3948e377a8e80a4b9c55',
|
|
severity: 'MINOR', contradicts_brief: NON_GOAL, brief_lowered_from: 'MAJOR', ...extra,
|
|
});
|
|
}
|
|
|
|
function assertAllOutputLegal(result) {
|
|
for (const f of result.findings) {
|
|
assert.equal(illegal(f), false,
|
|
`runContract wrote a finding illegalBriefLowering refuses: ${JSON.stringify(f)}`);
|
|
}
|
|
}
|
|
|
|
// ---- runContract (Pass 3) ----------------------------------------------------
|
|
|
|
test('runContract — a marked /code-review MAJOR drops to MINOR, keeps the Non-Goal, verdict ALLOW, not unverified', () => {
|
|
const result = m.runContract(payload('claude-code-review', [codeReviewDefect({ contradicts_brief: NON_GOAL })]));
|
|
assert.equal(result.findings.length, 1, 'the brief-conflicting finding is kept, never removed');
|
|
const f = result.findings[0];
|
|
assert.equal(f.contradicts_brief, NON_GOAL, 'the Non-Goal is named verbatim');
|
|
assert.equal(f.severity, 'MINOR');
|
|
assert.equal(f.brief_lowered_from, 'MAJOR');
|
|
assert.equal(result.verdict, 'ALLOW');
|
|
assert.equal(result.unverified.length, 0, 'the mark is not a suppression reason');
|
|
assert.deepEqual(result.allow_blocked_by, []);
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('runContract — the same /code-review MAJOR without the mark stays MAJOR and WARN', () => {
|
|
const result = m.runContract(payload('claude-code-review', [codeReviewDefect()]));
|
|
assert.equal(result.findings[0].severity, 'MAJOR');
|
|
assert.equal('brief_lowered_from' in result.findings[0], false);
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('runContract — a marked /code-review BLOCKER-tier finding stays BLOCKER and BLOCK', () => {
|
|
const f = codeReviewDefect({ rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', contradicts_brief: NON_GOAL });
|
|
const result = m.runContract(payload('claude-code-review', [f]));
|
|
assert.equal(result.findings[0].severity, 'BLOCKER');
|
|
assert.equal('brief_lowered_from' in result.findings[0], false);
|
|
assert.equal(result.findings[0].contradicts_brief, NON_GOAL, 'the mark is kept on an engine finding');
|
|
assert.equal(result.verdict, 'BLOCK');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('runContract — a marked /security-review finding keeps BLOCKER', () => {
|
|
const f = {
|
|
file: 'lib/auth.mjs', line: 9, rule_key: 'SECURITY_REVIEW_VULNERABILITY', severity: 'BLOCKER',
|
|
title: 'Token in log', detail: 'd', recommended_action: 'Remove the log line.',
|
|
engine: 'security-review', contradicts_brief: NON_GOAL,
|
|
};
|
|
const result = m.runContract(payload('claude-security-review', [f]));
|
|
assert.equal(result.findings[0].severity, 'BLOCKER');
|
|
assert.equal('brief_lowered_from' in result.findings[0], false);
|
|
assert.equal(result.verdict, 'BLOCK');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('runContract — a marked MINOR-tier CODE_REVIEW_IMPROVEMENT stays MINOR with no brief_lowered_from', () => {
|
|
const f = codeReviewDefect({ rule_key: 'CODE_REVIEW_IMPROVEMENT', severity: 'MINOR', contradicts_brief: NON_GOAL });
|
|
const result = m.runContract(payload('claude-code-review', [f]));
|
|
assert.equal(result.findings[0].severity, 'MINOR');
|
|
assert.equal('brief_lowered_from' in result.findings[0], false);
|
|
assert.equal(result.findings[0].contradicts_brief, NON_GOAL);
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('runContract — a reviewer finding (no engine) that arrives marked loses the mark and keeps today\'s reset', () => {
|
|
const f = {
|
|
file: 'lib/b.mjs', line: 7, rule_key: 'MISSING_TEST', severity: 'BLOCKER',
|
|
title: 'No test', detail: 'd', recommended_action: 'Add a test.', contradicts_brief: NON_GOAL,
|
|
};
|
|
const result = m.runContract(payload('code-correctness-reviewer', [f]));
|
|
const out = result.findings[0];
|
|
assert.equal('contradicts_brief' in out, false, 'only an engine finding is ever marked');
|
|
assert.equal(out.severity, 'MAJOR');
|
|
assert.equal(out.original_severity, 'BLOCKER');
|
|
assert.equal('brief_lowered_from' in out, false);
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('runContract — an incoming brief_lowered_from is removed from every finding', () => {
|
|
const reviewer = {
|
|
file: 'lib/b.mjs', line: 7, rule_key: 'MISSING_TEST', severity: 'MAJOR',
|
|
title: 'No test', detail: 'd', recommended_action: 'Add a test.', brief_lowered_from: 'MAJOR',
|
|
};
|
|
const engineBlocker = codeReviewDefect({ line: 50, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER', brief_lowered_from: 'BLOCKER' });
|
|
const engineUnmarked = codeReviewDefect({ line: 60, brief_lowered_from: 'MAJOR' });
|
|
const lowerableOne = codeReviewDefect({ line: 70, contradicts_brief: NON_GOAL, brief_lowered_from: 'BLOCKER' });
|
|
const result = m.runContract([
|
|
{ reviewer: 'code-correctness-reviewer', findings: [reviewer] },
|
|
{ reviewer: 'claude-code-review', findings: [engineBlocker, engineUnmarked, lowerableOne] },
|
|
]);
|
|
const byLine = new Map(result.findings.map((f) => [f.line, f]));
|
|
assert.equal('brief_lowered_from' in byLine.get(7), false);
|
|
assert.equal('brief_lowered_from' in byLine.get(50), false);
|
|
assert.equal(byLine.get(50).severity, 'BLOCKER');
|
|
assert.equal('brief_lowered_from' in byLine.get(60), false);
|
|
assert.equal(byLine.get(60).severity, 'MAJOR');
|
|
assert.equal(byLine.get(70).severity, 'MINOR');
|
|
assert.equal(byLine.get(70).brief_lowered_from, 'MAJOR', 'only the lowering itself writes the field');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('lowerableBriefConflict — only a marked /code-review finding at MAJOR on a MAJOR-tier key', () => {
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: NON_GOAL })), true);
|
|
assert.equal(lowerable(codeReviewDefect()), false, 'no mark');
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: '' })), false, 'empty mark');
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: true })), false, 'non-string mark');
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: NON_GOAL, engine: 'security-review' })), false);
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: NON_GOAL, engine: undefined })), false, 'missing engine');
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: NON_GOAL, rule_key: 'NOPE' })), false, 'unknown key');
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: NON_GOAL, rule_key: 'SECURITY_INJECTION', severity: 'BLOCKER' })), false);
|
|
assert.equal(lowerable(codeReviewDefect({ contradicts_brief: NON_GOAL, severity: 'BLOCKER' })), false, 'severity must be MAJOR');
|
|
});
|
|
|
|
// ---- effectiveSeverity (Pass 2 trimming) -------------------------------------
|
|
|
|
test('judgeFilter — a marked /code-review MAJOR with a long detail is trimmed, its mark intact; unmarked it is kept whole', () => {
|
|
const long = 'x'.repeat(900);
|
|
const marked = codeReviewDefect({ detail: long, contradicts_brief: NON_GOAL });
|
|
const unmarked = codeReviewDefect({ detail: long });
|
|
const a = m.judgeFilter([marked]);
|
|
assert.equal(a.trimmed.length, 1, 'the marked finding is judged MINOR, so its text may be trimmed');
|
|
assert.equal(a.trimmed[0].contradicts_brief, NON_GOAL, 'trimming never touches the mark');
|
|
assert.ok(a.trimmed[0].detail.length <= m.JUDGE_DETAIL_MAX);
|
|
const b = m.judgeFilter([unmarked]);
|
|
assert.equal(b.trimmed.length, 0, 'an unmarked MAJOR keeps every character');
|
|
assert.equal(b.kept[0].detail, long);
|
|
|
|
const result = m.runContract(payload('claude-code-review', [marked]));
|
|
assert.equal(result.trimmed_count, 1);
|
|
assert.equal(result.findings[0].contradicts_brief, NON_GOAL);
|
|
assert.equal(result.findings[0].severity, 'MINOR');
|
|
});
|
|
|
|
// ---- illegalBriefLowering ----------------------------------------------------
|
|
|
|
test('illegalBriefLowering — the legal shape is false', () => {
|
|
assert.equal(illegal(legalShape()), false);
|
|
});
|
|
|
|
test('illegalBriefLowering — a marked BLOCKER-tier engine finding below BLOCKER is true, with and without brief_lowered_from', () => {
|
|
const base = { rule_key: 'SECURITY_INJECTION', contradicts_brief: NON_GOAL };
|
|
assert.equal(illegal(codeReviewDefect({ ...base, severity: 'MINOR', brief_lowered_from: 'MAJOR' })), true);
|
|
assert.equal(illegal(codeReviewDefect({ ...base, severity: 'MAJOR', brief_lowered_from: 'BLOCKER' })), true);
|
|
assert.equal(illegal(codeReviewDefect({ ...base, severity: 'MAJOR' })), true);
|
|
assert.equal(illegal(codeReviewDefect({ ...base, severity: 'MINOR' })), true);
|
|
});
|
|
|
|
test('illegalBriefLowering — a marked /security-review finding below its tier is true', () => {
|
|
const f = {
|
|
id: 'a'.repeat(40), file: 'lib/auth.mjs', line: 9, rule_key: 'SECURITY_REVIEW_VULNERABILITY',
|
|
engine: 'security-review', contradicts_brief: NON_GOAL,
|
|
};
|
|
assert.equal(illegal({ ...f, severity: 'MAJOR' }), true);
|
|
assert.equal(illegal({ ...f, severity: 'MINOR', brief_lowered_from: 'MAJOR' }), true);
|
|
});
|
|
|
|
test('illegalBriefLowering — a marked reviewer finding (no engine) is true at any severity', () => {
|
|
const f = { id: 'b'.repeat(40), file: 'lib/b.mjs', line: 7, rule_key: 'MISSING_TEST', contradicts_brief: NON_GOAL };
|
|
assert.equal(illegal({ ...f, severity: 'MINOR' }), true, 'below its tier');
|
|
assert.equal(illegal({ ...f, severity: 'MAJOR' }), true, 'at its tier');
|
|
assert.equal(illegal({ ...f, severity: 'BLOCKER' }), true, 'above its tier');
|
|
});
|
|
|
|
test('illegalBriefLowering — brief_lowered_from without a mark is true', () => {
|
|
assert.equal(illegal(legalShape({ contradicts_brief: undefined })), true,
|
|
'brief_lowered_from MAJOR on an otherwise legal finding without a mark');
|
|
const reviewer = { id: 'b'.repeat(40), file: 'lib/b.mjs', line: 7, rule_key: 'MISSING_TEST', severity: 'MAJOR', brief_lowered_from: 'MAJOR' };
|
|
assert.equal(illegal(reviewer), true, 'brief_lowered_from on an unmarked reviewer finding');
|
|
});
|
|
|
|
test('illegalBriefLowering — a marked engine finding with an unknown or missing rule_key is true', () => {
|
|
assert.equal(illegal(codeReviewDefect({ rule_key: 'NOT_A_KEY', contradicts_brief: NON_GOAL })), true);
|
|
assert.equal(illegal(codeReviewDefect({ rule_key: undefined, contradicts_brief: NON_GOAL })), true);
|
|
});
|
|
|
|
test('illegalBriefLowering — contradicts_brief: true (not a string) on the otherwise legal shape is true', () => {
|
|
assert.equal(illegal(legalShape({ contradicts_brief: true })), true);
|
|
});
|
|
|
|
test('illegalBriefLowering — an unmarked engine finding below its tier is true', () => {
|
|
const sec = {
|
|
id: 'c'.repeat(40), file: 'lib/auth.mjs', line: 9, rule_key: 'SECURITY_REVIEW_VULNERABILITY',
|
|
severity: 'MINOR', engine: 'security-review',
|
|
};
|
|
assert.equal(illegal(sec), true, 'unmarked /security-review SECURITY_REVIEW_VULNERABILITY at MINOR');
|
|
assert.equal(illegal(codeReviewDefect({ severity: 'MINOR' })), true, 'unmarked /code-review CODE_REVIEW_DEFECT at MINOR');
|
|
});
|
|
|
|
test('illegalBriefLowering — false for engine findings at their tier and for unmarked reviewer findings', () => {
|
|
assert.equal(illegal(codeReviewDefect({ contradicts_brief: NON_GOAL })), false, 'a marked engine finding at its tier');
|
|
assert.equal(illegal(codeReviewDefect()), false, 'an unmarked engine finding at its tier');
|
|
const reviewer = { id: 'b'.repeat(40), file: 'lib/b.mjs', line: 7, rule_key: 'MISSING_TEST' };
|
|
assert.equal(illegal({ ...reviewer, severity: 'MINOR' }), false,
|
|
'an unmarked reviewer MISSING_TEST at MINOR without brief_lowered_from (the quick-mode shape)');
|
|
assert.equal(illegal({ ...reviewer, severity: 'BLOCKER' }), false,
|
|
'an unmarked reviewer MISSING_TEST asserted BLOCKER (above its tier)');
|
|
});
|
|
|
|
// ---- review → plan handover --------------------------------------------------
|
|
|
|
test('review → plan handover — the lowered finding is left out by /trekplan\'s severity filter', () => {
|
|
const prose = readFileSync(join(ROOT, 'commands', 'trekplan.md'), 'utf-8');
|
|
const line = prose.split('\n').find((l) => l.includes('Filter findings to severity'));
|
|
assert.ok(line, 'commands/trekplan.md carries the "Filter findings to severity" line');
|
|
const set = line.match(/\{([^}]*)\}/);
|
|
assert.ok(set, 'the filter line names a {…} severity set');
|
|
const severities = new Set(set[1].split(',').map((s) => s.trim()));
|
|
assert.deepEqual([...severities].sort(), ['BLOCKER', 'MAJOR']);
|
|
|
|
const lowered = m.runContract(payload('claude-code-review', [codeReviewDefect({ contradicts_brief: NON_GOAL })]));
|
|
assert.equal(lowered.findings.filter((f) => severities.has(f.severity)).length, 0,
|
|
'a brief-lowered finding does not become a plan goal');
|
|
const unmarked = m.runContract(payload('claude-code-review', [codeReviewDefect()]));
|
|
assert.equal(unmarked.findings.filter((f) => severities.has(f.severity)).length, 1,
|
|
'known-positive control: the same finding unmarked does');
|
|
});
|
|
|
|
// ---- the verdict gate --------------------------------------------------------
|
|
|
|
function runGate(path) {
|
|
return spawnSync(process.execPath, [GATE, path], { encoding: 'utf-8' });
|
|
}
|
|
|
|
test('verdict gate — exit 0 on the legal brief lowering', () => {
|
|
assert.ok(existsSync(LEGAL_FIXTURE), 'tests/fixtures/review-brief-conflict-legal.md exists');
|
|
const r = runGate(LEGAL_FIXTURE);
|
|
assert.equal(r.status, 0, `legal lowering must pass: ${r.stdout}${r.stderr}`);
|
|
});
|
|
|
|
test('verdict gate — exit 2 on the illegal brief lowering, the reason naming the finding id', () => {
|
|
assert.ok(existsSync(ILLEGAL_FIXTURE), 'tests/fixtures/review-brief-conflict-illegal.md exists');
|
|
const text = readFileSync(ILLEGAL_FIXTURE, 'utf-8');
|
|
const json = JSON.parse(extractFindingsBlock(text));
|
|
assert.equal(json.verdict, 'ALLOW', 'the fixture claims ALLOW');
|
|
const id = json.findings[0].id;
|
|
assert.match(id, /^[0-9a-f]{40}$/);
|
|
const r = runGate(ILLEGAL_FIXTURE);
|
|
assert.equal(r.status, 2, `a marked reviewer finding must be refused: ${r.stdout}${r.stderr}`);
|
|
assert.ok(r.stdout.includes(id), `the reason names the finding id ${id}: ${r.stdout}`);
|
|
});
|
|
|
|
test('verdict gate — imports illegalBriefLowering from coordinator-contract.mjs (the rule lives once)', () => {
|
|
const src = readFileSync(GATE, 'utf-8');
|
|
assert.match(src,
|
|
/^import\s*\{[^}]*\billegalBriefLowering\b[^}]*\}\s*from\s*['"]\.\/coordinator-contract\.mjs['"]/m,
|
|
'verdict-gate.mjs must import illegalBriefLowering from ./coordinator-contract.mjs');
|
|
assert.doesNotMatch(src, /function\s+illegalBriefLowering\b/,
|
|
'verdict-gate.mjs must not carry its own copy of the rule');
|
|
});
|
|
|
|
// ---- the prose states the same rule (Step 10) --------------------------------
|
|
//
|
|
// The coordinator is a model reading agents/review-coordinator.md; the gate
|
|
// above enforces the rule on what it writes. These pins keep the prose saying
|
|
// the same rule the code enforces. Every pin reads a bounded cut, never the
|
|
// whole file, so a phrase elsewhere in the file cannot satisfy it.
|
|
|
|
const readDoc = (...rel) => readFileSync(join(ROOT, ...rel), 'utf-8');
|
|
|
|
/** The text from `startMarker` up to (not including) the first `endMarker` after it. */
|
|
function cut(text, startMarker, endMarker, label) {
|
|
const start = text.indexOf(startMarker);
|
|
assert.ok(start >= 0, `${label}: start marker missing: ${JSON.stringify(startMarker)}`);
|
|
const end = text.indexOf(endMarker, start + startMarker.length);
|
|
assert.ok(end > start, `${label}: end marker ${JSON.stringify(endMarker)} missing after the start`);
|
|
return text.slice(start, end);
|
|
}
|
|
|
|
/** The sentence from `startPhrase` to its full stop (the first '.' after it). */
|
|
function sentenceFrom(text, startPhrase, label) {
|
|
const start = text.indexOf(startPhrase);
|
|
assert.ok(start >= 0, `${label}: sentence start missing: ${JSON.stringify(startPhrase)}`);
|
|
const stop = text.indexOf('.', start + startPhrase.length);
|
|
assert.ok(stop > start, `${label}: the sentence has no full stop`);
|
|
return text.slice(start, stop + 1);
|
|
}
|
|
|
|
/** One hard-rule bullet, from its bold lead to the next bullet. */
|
|
function hardRule(coord, lead) {
|
|
const at = coord.indexOf('\n## Hard rules');
|
|
assert.ok(at >= 0, 'the ## Hard rules section exists');
|
|
const rules = coord.slice(at);
|
|
return cut(`${rules}\n- **`, `- **${lead}**`, '\n- **', `hard rule "${lead}"`);
|
|
}
|
|
|
|
test('trekreview Phase 6 — passes the brief\'s Non-Goals, Constraints, Preferences and Open Questions decisions to the coordinator', () => {
|
|
const cmd = readDoc('commands', 'trekreview.md');
|
|
const p6 = cut(cmd, '## Phase 6 — Coordinator dedup + verdict', '\n## ', 'Phase 6');
|
|
for (const section of ['`## Non-Goals`', '`## Constraints`', '`## Preferences`', '`## Open Questions / Assumptions`']) {
|
|
assert.ok(p6.includes(section), `Phase 6 names the brief section ${section}`);
|
|
}
|
|
assert.match(p6, /operator decisions in `## Open Questions \/ Assumptions`/, 'only the operator decisions of Open Questions');
|
|
assert.match(p6, /verbatim/, 'the choices are passed verbatim');
|
|
assert.match(p6, /brief choices/i, 'Phase 6 names the input the coordinator reads');
|
|
});
|
|
|
|
test('trekreview workflow path — engine, contradicts_brief and brief_lowered_from are carried into review.md', () => {
|
|
const cmd = readDoc('commands', 'trekreview.md');
|
|
const wf = cut(cmd, 'Then continue at **Phase 7**', '\n\n', 'workflow paragraph');
|
|
for (const field of ['`engine`', '`contradicts_brief`', '`brief_lowered_from`']) {
|
|
assert.ok(wf.includes(field), `the workflow paragraph names ${field}`);
|
|
}
|
|
assert.match(wf, /review\.md/, 'the fields are written into review.md');
|
|
});
|
|
|
|
test('review-coordinator Input — names "Brief choices" (the four sections) and the mode line names the exception', () => {
|
|
const coord = readDoc('agents', 'review-coordinator.md');
|
|
const input = cut(coord, '## Input', '\n## Your 4-pass process', 'Input');
|
|
assert.match(input, /\*\*Brief choices\*\*/);
|
|
const choices = cut(input, '**Brief choices**', '\n- **', 'Brief choices bullet');
|
|
for (const section of ['`## Non-Goals`', '`## Constraints`', '`## Preferences`', '`## Open Questions / Assumptions`']) {
|
|
assert.ok(choices.includes(section), `Brief choices names ${section}`);
|
|
}
|
|
assert.match(choices, /verbatim/);
|
|
const mode = cut(input, '- **Mode**', '\n- **', 'mode line');
|
|
assert.match(mode, /except the brief-conflict rule, which applies in every mode/);
|
|
});
|
|
|
|
test('review-coordinator Pass 3 — only an engine finding is marked, and the one lowering needs severity MAJOR', () => {
|
|
const coord = readDoc('agents', 'review-coordinator.md');
|
|
const p3 = cut(coord, '### Pass 3', 'In `quick` mode', 'Pass 3');
|
|
assert.match(p3, /`contradicts_brief: "<verbatim>"`/, 'the mark carries the Non-Goal or choice verbatim');
|
|
assert.match(p3, /engine finding \(`engine: code-review` or `security-review`\)/, 'only an engine finding is marked');
|
|
assert.match(p3, /only an engine finding is ever marked/);
|
|
assert.match(p3, /only a `\/code-review` finding at the MAJOR tier and at severity MAJOR then drops to MINOR, with `brief_lowered_from: MAJOR`/,
|
|
'the one lowering, severity MAJOR required');
|
|
assert.match(p3, /a BLOCKER-tier, a MINOR-tier and any `\/security-review` finding only get the mark/);
|
|
assert.match(p3, /no engine finding is otherwise written below its catalogue tier/);
|
|
assert.match(p3, /`engine` is written only on a finding whose kept Pass 1 survivor itself came from an engine payload/);
|
|
assert.match(p3, /a reviewer-agent finding \(no `engine`\) and a finding whose `rule_key` is not in the catalogue are never marked/);
|
|
assert.match(p3, /not the rule key `NON_GOAL_VIOLATED`/);
|
|
assert.ok(p3.includes('the brief-conflict reason (`contradicts_brief`), a field, never a `suppressed_reason`'),
|
|
'the mark is named as a field, never a suppression reason');
|
|
assert.match(p3, /verdict gate refuses \(exit 2\) every other marked or lowered shape/);
|
|
});
|
|
|
|
test('review-coordinator — quick mode and high-effort normalization each still apply the brief-conflict rule', () => {
|
|
const coord = readDoc('agents', 'review-coordinator.md');
|
|
const pass3 = coord.indexOf('### Pass 3');
|
|
assert.ok(pass3 >= 0, 'Pass 3 heading');
|
|
const quick = cut(coord.slice(pass3), 'In `quick` mode, skip this pass entirely.', '\n\n', 'quick-mode paragraph');
|
|
assert.match(quick, /brief-conflict rule[^.]*still applies/, 'the quick-mode paragraph says the rule still applies');
|
|
const high = cut(coord, '**High-effort normalization (v5.1.1):**', '\n### ', 'high-effort paragraph');
|
|
assert.match(high, /brief-conflict rule[^.]*still applies/, 'the high-effort paragraph says the rule still applies');
|
|
});
|
|
|
|
test('review-coordinator Pass 2 — the trim rule judges a lowerable brief conflict as MINOR, inside its own sentence', () => {
|
|
const coord = readDoc('agents', 'review-coordinator.md');
|
|
const s = sentenceFrom(coord, 'The trim decision reads the **catalogue** tier', 'trim rule');
|
|
assert.match(s, /except a lowerable brief conflict, which is judged as MINOR \(decide the mark before trimming\)/);
|
|
assert.match(s, /so a mislabelled MAJOR is never cut\.$/, 'the sentence still ends on its own guarantee');
|
|
});
|
|
|
|
test('review-coordinator hard rules — the brief re-evaluation, the severity floor and quick mode each name the exception', () => {
|
|
const coord = readDoc('agents', 'review-coordinator.md');
|
|
assert.match(hardRule(coord, 'Bounded operations only.'), /brief-conflict rule/,
|
|
'"You do NOT re-evaluate findings against the brief" names its one exception');
|
|
assert.match(hardRule(coord, 'Severity floor is the catalogue.'), /except a lowerable brief conflict, which is MINOR/);
|
|
assert.match(hardRule(coord, 'Quick mode is documented.'), /except the brief-conflict rule, which applies in every mode/);
|
|
});
|
|
|
|
test('review-coordinator output — the field list and the trailing JSON example carry engine, contradicts_brief, brief_lowered_from', () => {
|
|
const coord = readDoc('agents', 'review-coordinator.md');
|
|
const fields = cut(coord, 'Each Findings subsection uses the', '### Trailing JSON block', 'field list');
|
|
assert.ok(fields.includes('- `- engine: {code-review | security-review, when the finding came from an engine}`'));
|
|
assert.ok(fields.includes('- `- contradicts_brief: {verbatim Non-Goal or choice, only when set}`'));
|
|
assert.ok(fields.includes('- `- brief_lowered_from: {MAJOR, only when lowered for the brief}`'));
|
|
assert.ok(!fields.includes('original_severity'), 'the brief rule does not read original_severity');
|
|
|
|
const block = coord.match(/### Trailing JSON block\n\nThe LAST fenced block in the file is a `json` block:\n\n```json\n([\s\S]*?)\n```/);
|
|
assert.ok(block, 'the trailing JSON example is present');
|
|
const objects = block[1].match(/\{[^{}]*\}/g) ?? [];
|
|
const lowered = objects.find((o) => /"brief_lowered_from":\s*"MAJOR"/.test(o));
|
|
assert.ok(lowered, 'the JSON example shows a finding with brief_lowered_from');
|
|
assert.match(lowered, /"engine":\s*"code-review"/);
|
|
assert.match(lowered, /"contradicts_brief":\s*"[^"]+"/);
|
|
assert.match(lowered, /"severity":\s*"MINOR"/);
|
|
assert.match(lowered, /"rule_key":\s*"CODE_REVIEW_DEFECT"/);
|
|
});
|
|
|
|
test('Handover 6 — names the fields, engine findings only, the one lowering and the five shapes the gate refuses', () => {
|
|
const contracts = readDoc('docs', 'HANDOVER-CONTRACTS.md');
|
|
const h6 = cut(contracts, '## Handover 6 — `review.md` → plan', '\n## Handover 7', 'Handover 6');
|
|
for (const field of ['`engine`', '`contradicts_brief`', '`brief_lowered_from`']) {
|
|
assert.ok(h6.includes(field), `Handover 6 names ${field}`);
|
|
}
|
|
assert.match(h6, /only an engine finding is (?:ever )?marked/i, 'engine findings only');
|
|
assert.match(h6, /`\/code-review` MAJOR-tier finding[^.]*MINOR[^.]*`brief_lowered_from: MAJOR`/, 'the one legal lowering');
|
|
assert.match(h6, /exits 2/);
|
|
const shapes = [
|
|
/any other `brief_lowered_from`/,
|
|
/a mark on a reviewer-agent finding/,
|
|
/a marked finding below its tier/,
|
|
/a marked finding with an unknown key/,
|
|
/an engine finding below its tier, marked or not/,
|
|
];
|
|
for (const shape of shapes) assert.match(h6, shape, `Handover 6 names the refused shape ${shape}`);
|
|
assert.match(h6, /cannot check an `engine` label on the prose path/);
|
|
assert.match(h6, /skips a lowered finding/, 'a plan made from the review skips a lowered (MINOR) finding');
|
|
});
|
|
|
|
// ---- Decision 13 (operator 12.10): the source decides, a tie keeps the reviewer --
|
|
//
|
|
// "Came from an engine" is read from the payload a finding arrived in, never
|
|
// from an `engine` field a reviewer wrote on its own finding; and Pass 1 ranks
|
|
// a triplet's findings by catalogue tier first (for a catalogue key the tier
|
|
// always ties inside one triplet), then a reviewer-agent finding over an engine
|
|
// finding, and the survivor carries the highest severity its group asserted,
|
|
// so an engine's mark can never lower what a reviewer agent asserted, whatever
|
|
// the order (review bc7f4e23).
|
|
|
|
function reviewerCopy(extra = {}) {
|
|
const { engine: _e, ...rest } = codeReviewDefect({ title: 'Reviewer: no retry', ...extra });
|
|
return rest;
|
|
}
|
|
|
|
test('decision 13 — a reviewer payload whose finding labels itself engine code-review is not lowered', () => {
|
|
const f = codeReviewDefect({ contradicts_brief: NON_GOAL });
|
|
const result = m.runContract(payload('code-correctness-reviewer', [f]));
|
|
const out = result.findings[0];
|
|
assert.equal(out.severity, 'MAJOR', 'a reviewer MAJOR is never lowered by the brief rule');
|
|
assert.equal('brief_lowered_from' in out, false);
|
|
assert.equal('engine' in out, false, 'engine comes from the payload source, not the field');
|
|
assert.equal('contradicts_brief' in out, false, 'only an engine finding is ever marked');
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test('decision 13 — an engine payload decides the engine: a /security-review payload finding labelled code-review keeps its severity', () => {
|
|
const f = codeReviewDefect({ contradicts_brief: NON_GOAL });
|
|
const result = m.runContract(payload('claude-security-review', [f]));
|
|
const out = result.findings[0];
|
|
assert.equal(out.engine, 'security-review');
|
|
assert.equal(out.severity, 'MAJOR');
|
|
assert.equal('brief_lowered_from' in out, false);
|
|
assert.equal(out.contradicts_brief, NON_GOAL, 'the mark is kept on an engine finding');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
// One reviewer finding and one /code-review finding as two payloads, in `order`.
|
|
function reviewerAndEnginePayloads(order, reviewerFinding, engineFinding) {
|
|
const reviewerPayload = { reviewer: 'code-correctness-reviewer', findings: [reviewerFinding] };
|
|
const enginePayload = { reviewer: 'claude-code-review', findings: [engineFinding] };
|
|
return order === 'reviewer first' ? [reviewerPayload, enginePayload] : [enginePayload, reviewerPayload];
|
|
}
|
|
|
|
// A reviewer MISSING_TEST and a marked /code-review MAJOR on the same triplet, in `order`.
|
|
function reviewerAndMarkedEngine(order, reviewerExtra = {}) {
|
|
return reviewerAndEnginePayloads(order,
|
|
reviewerCopy({ rule_key: 'MISSING_TEST', ...reviewerExtra }),
|
|
codeReviewDefect({ rule_key: 'MISSING_TEST', contradicts_brief: NON_GOAL }));
|
|
}
|
|
|
|
for (const order of ['reviewer first', 'engine first']) {
|
|
test(`decision 13 — a tie between a marked /code-review MAJOR and a reviewer MAJOR keeps the reviewer (${order}) → WARN`, () => {
|
|
const result = m.runContract(reviewerAndMarkedEngine(order));
|
|
assert.equal(result.findings.length, 1);
|
|
const out = result.findings[0];
|
|
assert.equal(out.severity, 'MAJOR', 'the reviewer agent asserted MAJOR; the engine mark cannot lower it');
|
|
assert.equal('brief_lowered_from' in out, false);
|
|
assert.equal('engine' in out, false, 'the survivor is the reviewer finding');
|
|
assert.equal('contradicts_brief' in out, false);
|
|
assert.deepEqual([...out.raised_by].sort(), ['claude-code-review', 'code-correctness-reviewer']);
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
}
|
|
|
|
for (const order of ['reviewer first', 'engine first']) {
|
|
test(`bc7f4e23 — a marked /code-review MAJOR over a reviewer MINOR on the same triplet keeps the reviewer at MAJOR (${order}) → WARN`, () => {
|
|
const result = m.runContract(reviewerAndMarkedEngine(order, { severity: 'MINOR' }));
|
|
assert.equal(result.findings.length, 1);
|
|
const out = result.findings[0];
|
|
assert.equal('engine' in out, false, 'the survivor is the reviewer finding');
|
|
assert.equal(out.severity, 'MAJOR', 'the group asserted MAJOR; dropping the engine duplicate never lowers it');
|
|
assert.equal('brief_lowered_from' in out, false, 'the engine mark never lowers what a reviewer agent asserted');
|
|
assert.equal('contradicts_brief' in out, false);
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
|
|
test(`bc7f4e23 — dedupByTriplet(ingest(...)) keeps the reviewer finding at the group's MAJOR over a marked /code-review MAJOR (${order})`, () => {
|
|
const { findings } = m.ingest(reviewerAndMarkedEngine(order, { severity: 'MINOR' }));
|
|
const out = m.dedupByTriplet(findings);
|
|
assert.equal(out.length, 1);
|
|
assert.equal('engine' in out[0], false, 'the reviewer-agent finding survives the tier tie');
|
|
assert.equal(out[0].reviewer, 'code-correctness-reviewer');
|
|
assert.equal(out[0].severity, 'MAJOR', 'the survivor carries the highest severity its group asserted');
|
|
assert.equal('contradicts_brief' in out[0], false, 'the engine mark is not carried to the survivor');
|
|
assert.deepEqual([...out[0].raised_by].sort(), ['claude-code-review', 'code-correctness-reviewer']);
|
|
});
|
|
}
|
|
|
|
test('decision 13 — the prose states the tie rule (coordinator Pass 1, Handover 6)', () => {
|
|
const coordinator = readDoc('agents', 'review-coordinator.md');
|
|
const pass1 = cut(coordinator, '### Pass 1', '### Pass 2', 'coordinator Pass 1');
|
|
assert.match(pass1, /prefer\s+a\s+reviewer-agent\s+finding\s+over\s+an\s+engine\s+finding/,
|
|
'Pass 1: a tie keeps the reviewer-agent finding');
|
|
assert.match(pass1, /never\s+lowers\s+what\s+a\s+reviewer\s+agent\s+asserted/);
|
|
const h6 = cut(readDoc('docs', 'HANDOVER-CONTRACTS.md'), '## Handover 6 — `review.md` → plan', '\n## Handover 7', 'Handover 6');
|
|
assert.match(h6, /takes? `engine` from the payload a finding arrived in/);
|
|
assert.match(h6, /ranked by catalogue tier[^.]*a reviewer-agent finding survives over an engine finding[^.]*highest severity the group asserted/i,
|
|
'Handover 6: Pass 1 ranks by catalogue tier and the survivor carries the group severity (bc7f4e23)');
|
|
});
|
|
|
|
test('b406a628 — coordinator Pass 1 ranks in the library order and writes the group severity (decision 24)', () => {
|
|
const coordinator = readDoc('agents', 'review-coordinator.md');
|
|
const pass1 = cut(coordinator, '### Pass 1', '### Pass 2', 'coordinator Pass 1');
|
|
assert.doesNotMatch(pass1, /highest\s+catalogue\s+severity/, 'Pass 1 no longer keeps the highest catalogue severity');
|
|
const ranks = [
|
|
/higher\s+catalogue\s+tier/,
|
|
/prefer\s+a\s+reviewer-agent\s+finding\s+over\s+an\s+engine\s+finding/,
|
|
/higher\s+asserted\s+severity/,
|
|
/brief-conformance-reviewer/,
|
|
/first\s+seen/,
|
|
];
|
|
let previous = -1;
|
|
for (const rank of ranks) {
|
|
const at = pass1.search(rank);
|
|
assert.ok(at >= 0, `Pass 1 names the rank ${rank}`);
|
|
assert.ok(at > previous, `Pass 1 names the rank ${rank} after the one before it`);
|
|
previous = at;
|
|
}
|
|
assert.match(pass1, /not\s+in\s+the\s+catalogue/, 'Pass 1 names the unknown-key fallback');
|
|
assert.match(pass1, /highest\s+severity\s+any\s+finding\s+in\s+its\s+group\s+asserted/,
|
|
'Pass 1 writes the group severity');
|
|
assert.doesNotMatch(coordinator, /not\s+the\s+reviewer's\s+emitted\s+value/,
|
|
'no Hard rule still says the tiebreak ignores the emitted value');
|
|
assert.match(coordinator, /Pass\s+1\s+ranks\s+by\s+the\s+catalogue\s+tier\s+first/);
|
|
});
|
|
|
|
// ---- The engine of a payload is read from an own property (review 3799ea6c) --
|
|
//
|
|
// A payload `reviewer` named after an Object.prototype member must not pick up
|
|
// an inherited function or object as its engine: such a payload is a reviewer
|
|
// payload, its findings carry no `engine`, and the contract's output is plain
|
|
// JSON. The same lookup in engine-findings.mjs refuses an engine named after
|
|
// one with the existing "engine must be one of" error.
|
|
|
|
for (const name of ['toString', 'constructor', 'hasOwnProperty', 'valueOf', '__proto__']) {
|
|
test(`own property — a payload reviewer named ${name} gives findings no engine, and runContract's output survives a JSON round trip`, () => {
|
|
const { engine: _e, ...finding } = codeReviewDefect();
|
|
const ingested = m.ingest(payload(name, [finding]));
|
|
assert.equal(ingested.findings.length, 1);
|
|
assert.equal('engine' in ingested.findings[0], false,
|
|
`a payload reviewer named ${name} is not an engine payload`);
|
|
const result = m.runContract(payload(name, [finding]));
|
|
assert.deepEqual(JSON.parse(JSON.stringify(result)), result,
|
|
'runContract output is plain JSON (no inherited function or object as engine)');
|
|
assertAllOutputLegal(result);
|
|
});
|
|
}
|
|
|
|
test('own property — toReviewerPayload refuses engine toString with the "engine must be one of" error', () => {
|
|
const r = toReviewerPayload({ engine: 'toString', findings: [] });
|
|
assert.equal(r.ok, false, 'toString is not an engine');
|
|
assert.equal(r.errors.length, 1);
|
|
assert.match(r.errors[0], /^engine must be one of code-review\|security-review, got "toString"$/);
|
|
});
|
|
|
|
// ---- original_severity names what the kept finding asserted (review 08deae68) --
|
|
//
|
|
// Pass 1 writes the survivor at its group's highest asserted severity. When
|
|
// Pass 3 then resets it to the catalogue tier, `original_severity` must name
|
|
// what the kept finding itself asserted, never the severity a duplicate lent
|
|
// it. The Pass-1 bookkeeping never leaves the contract.
|
|
|
|
// A reviewer copy at `reviewerSeverity` and an engine CODE_REVIEW_DEFECT at
|
|
// `engineSeverity` on one triplet, in `order`.
|
|
function reviewerAndEngine(order, reviewerSeverity, engineSeverity, reviewerExtra = {}) {
|
|
return reviewerAndEnginePayloads(order,
|
|
reviewerCopy({ severity: reviewerSeverity, ...reviewerExtra }),
|
|
codeReviewDefect({ severity: engineSeverity }));
|
|
}
|
|
|
|
function assertNoPass1Field(result) {
|
|
for (const f of [...result.findings, ...result.suppressed, ...result.unverified]) {
|
|
assert.equal('pass1_raised_from' in f, false, `pass1_raised_from leaked: ${JSON.stringify(f)}`);
|
|
}
|
|
assert.equal(JSON.stringify(result).includes('pass1_raised_from'), false,
|
|
'no part of the result carries pass1_raised_from');
|
|
}
|
|
|
|
for (const order of ['reviewer first', 'engine first']) {
|
|
test(`08deae68 — a reviewer MAJOR raised by an engine BLOCKER ends at MAJOR with no original_severity (${order}) → WARN`, () => {
|
|
const result = m.runContract(reviewerAndEngine(order, 'MAJOR', 'BLOCKER'));
|
|
assert.equal(result.findings.length, 1);
|
|
const out = result.findings[0];
|
|
assert.equal('engine' in out, false, 'the survivor is the reviewer finding');
|
|
assert.equal(out.severity, 'MAJOR', 'the catalogue tier of CODE_REVIEW_DEFECT');
|
|
assert.equal('original_severity' in out, false,
|
|
'the kept finding asserted MAJOR, its tier; the BLOCKER was the engine duplicate\'s');
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertAllOutputLegal(result);
|
|
assertNoPass1Field(result);
|
|
});
|
|
|
|
test(`08deae68 — a reviewer MINOR raised by an engine MAJOR ends at MAJOR with original_severity MINOR (${order})`, () => {
|
|
const result = m.runContract(reviewerAndEngine(order, 'MINOR', 'MAJOR'));
|
|
assert.equal(result.findings.length, 1);
|
|
const out = result.findings[0];
|
|
assert.equal('engine' in out, false, 'the survivor is the reviewer finding');
|
|
assert.equal(out.severity, 'MAJOR');
|
|
assert.equal(out.original_severity, 'MINOR', 'the kept finding itself asserted MINOR');
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertAllOutputLegal(result);
|
|
assertNoPass1Field(result);
|
|
});
|
|
|
|
test(`08deae68 — a raised survivor set aside by Pass 2 (empty recommended_action) carries no pass1_raised_from (${order})`, () => {
|
|
const result = m.runContract(reviewerAndEngine(order, 'MAJOR', 'BLOCKER', { recommended_action: '' }));
|
|
assert.equal(result.findings.length, 0);
|
|
assert.equal(result.unverified.length, 1);
|
|
const u = result.unverified[0];
|
|
assert.equal(u.suppressed_reason, 'actionability:empty');
|
|
assert.equal(u.reviewer, 'code-correctness-reviewer', 'the raised survivor is the reviewer finding');
|
|
assert.equal(u.severity, 'BLOCKER', 'Pass 1 raised it to its group\'s severity');
|
|
assert.equal(result.suppressed.length, 1);
|
|
assert.equal(result.verdict, 'WARN');
|
|
assertNoPass1Field(result);
|
|
});
|
|
}
|