fix(review): a code-review finding against a brief Non-Goal may drop to MINOR, never a BLOCKER

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-12 18:04:36 +02:00 · 2026-10-12 18:04:36 +02:00 · ce7700b71b
commit ce7700b71b
parent 3009c5eddb
5 changed files with 533 additions and 4 deletions
--- a/tests/fixtures/review-brief-conflict-illegal.md
+++ b/tests/fixtures/review-brief-conflict-illegal.md
@ -0,0 +1,55 @@
+---
+type: trekreview
+review_version: "1.1"
+task: "t"
+slug: s
+project_dir: .claude/projects/2026-10-06-s/
+brief_path: .claude/projects/2026-10-06-s/brief.md
+scope_sha_end: abc
+reviewed_files_count: 1
+verdict: ALLOW
+findings:
+  - 8530de651325215c87cc8ba418c42ba3e8a66ed2
+---
+
+# Review: t
+
+## Executive Summary
+
+One reviewer-agent finding (no engine) carries a brief-conflict mark at MINOR: only an engine finding is ever marked, so the gate refuses it.
+
+## Coverage
+
+| File | Treatment | Reason |
+|------|-----------|--------|
+| lib/b.mjs | summary-only | default |
+
+## Remediation Summary
+
+x
+
+```json
+{
+  "verdict": "ALLOW",
+  "counts": {
+    "BLOCKER": 0,
+    "MAJOR": 0,
+    "MINOR": 1,
+    "SUGGESTION": 0
+  },
+  "allow_blocked_by": [],
+  "findings": [
+    {
+      "id": "8530de651325215c87cc8ba418c42ba3e8a66ed2",
+      "severity": "MINOR",
+      "rule_key": "MISSING_TEST",
+      "file": "lib/b.mjs",
+      "line": 7,
+      "title": "No test for the retry path",
+      "detail": "The retry branch has no test.",
+      "recommended_action": "Add a test for the retry branch.",
+      "contradicts_brief": "No retry layer around the HTTP client (Non-Goal 2)"
+    }
+  ]
+}
+```
--- a/tests/fixtures/review-brief-conflict-legal.md
+++ b/tests/fixtures/review-brief-conflict-legal.md
@ -0,0 +1,57 @@
+---
+type: trekreview
+review_version: "1.1"
+task: "t"
+slug: s
+project_dir: .claude/projects/2026-10-06-s/
+brief_path: .claude/projects/2026-10-06-s/brief.md
+scope_sha_end: abc
+reviewed_files_count: 1
+verdict: ALLOW
+findings:
+  - d3c22f4b1309d6001a0f3948e377a8e80a4b9c55
+---
+
+# Review: t
+
+## Executive Summary
+
+One /code-review finding contradicts a brief Non-Goal; it is kept, marked and lowered from MAJOR to MINOR (the one legal shape).
+
+## Coverage
+
+| File | Treatment | Reason |
+|------|-----------|--------|
+| lib/a.mjs | summary-only | default |
+
+## Remediation Summary
+
+x
+
+```json
+{
+  "verdict": "ALLOW",
+  "counts": {
+    "BLOCKER": 0,
+    "MAJOR": 0,
+    "MINOR": 1,
+    "SUGGESTION": 0
+  },
+  "allow_blocked_by": [],
+  "findings": [
+    {
+      "id": "d3c22f4b1309d6001a0f3948e377a8e80a4b9c55",
+      "severity": "MINOR",
+      "rule_key": "CODE_REVIEW_DEFECT",
+      "file": "lib/a.mjs",
+      "line": 12,
+      "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",
+      "contradicts_brief": "No retry layer around the HTTP client (Non-Goal 2)",
+      "brief_lowered_from": "MAJOR"
+    }
+  ]
+}
+```
--- a/tests/review/brief-conflict-findings.test.mjs
+++ b/tests/review/brief-conflict-findings.test.mjs
@ -0,0 +1,308 @@
+// 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';
+
+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(text.slice(text.lastIndexOf('```json') + 7, text.lastIndexOf('```')));
+  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');
+});