feat(review): one shared stop rule for the plan loop and review rounds (decision 217)

New lib/review/stop-rule.mjs: classOf (catalogue key, else the lower-cased
dimension before the first slash, else unclassified), roundSummary
(blocking = blockers + counting majors; new = no carried_from; null map
for a round counted in full), decideStop (same-class / not-fewer on the
last pair, round 1 alone never fires) and describeStop (one operator line
with the class, the counts per round and the two recommendations).

Part 1, step 1.1 of the E plan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lib/review/stop-rule.mjs

@@ -0,0 +1,112 @@
+// lib/review/stop-rule.mjs
+// The stop rule (decisions 217, 220): when a review loop gets no new round.
+// One rule, read by the plan loop (lib/validators/plan-gate.mjs, the critic
+// rounds) and by the review rounds (lib/review/review-rounds.mjs).
+//
+// A new round is worth running only while there is an open blocking finding
+// and the blocking findings get fewer. The rule is judged after round N >= 2,
+// on the pair (N-1, N), and fires on either reason:
+//   - `same-class`: one finding class has new blocking findings in N-1 and in
+//     N (two rounds in a row of the same kind of finding: the loop is finding
+//     instances of a class, not converging on it). Skipped for a pair where
+//     either round was counted in full (newByClass null: no carry, so "new"
+//     cannot be told from "carried").
+//   - `not-fewer`: blocking(N) > 0 and blocking(N) >= blocking(N-1).
+// Round 1 alone never fires, and a history that starts later (an older verdict
+// without one) fires only once it holds two rounds.
+//
+// A finding's class (217): its catalogue rule key when it has one
+// (lib/review/rule-catalogue.mjs), otherwise the reviewer's dimension, the
+// part of `rule_key` before the first `/`, lower-cased; empty -> `unclassified`.
+// Coarse on purpose: it stops early and often, and a loop cannot form.
+//
+// When it fires the operator gets the class and the counts per round with two
+// recommendations, "known issue" or "new mechanism" — never another round.
+//
+// Credit: the `not-fewer` signal covers what Addy Osmani calls "doubt theater"
+// (addyosmani/agent-skills, MIT): rounds of review that keep raising doubt
+// without reducing it.
+//
+// Pure: no I/O.
+
+import { getRule } from './rule-catalogue.mjs';
+
+export const STOP_REASONS = Object.freeze(['same-class', 'not-fewer']);
+
+/** A finding's class (217): the catalogue key, else the lower-cased dimension, else `unclassified`. */
+export function classOf(ruleKey) {
+  if (typeof ruleKey !== 'string') return 'unclassified';
+  const entry = getRule(ruleKey.trim().toUpperCase());
+  if (entry) return entry.rule_key;
+  const dim = ruleKey.split('/')[0].trim().toLowerCase();
+  return dim === '' ? 'unclassified' : dim;
+}
+
+/** Blocking = anything but minor or suggestion (an unrated finding counts, as the plan gate counts it as a blocker). */
+const isBlocking = f => !['minor', 'suggestion'].includes(String(f?.severity ?? '').trim().toLowerCase());
+
+/**
+ * One round's summary.
+ * @param {object[]} findings - the round's counted findings (blockers and
+ *   majors still counting, plus anything else, which is ignored)
+ * @param {{fullCount?: boolean}} [opts] - fullCount: the round was counted in
+ *   full (no diff base, no carry), so newByClass is null
+ * @returns {{blocking: number, newByClass: Object<string, number>|null}}
+ *   newByClass: per class, the blocking findings with no `carried_from`
+ */
+export function roundSummary(findings, { fullCount = false } = {}) {
+  const blocking = (Array.isArray(findings) ? findings : []).filter(f => f && typeof f === 'object' && isBlocking(f));
+  if (fullCount) return { blocking: blocking.length, newByClass: null };
+  const newByClass = {};
+  for (const f of blocking) {
+    if (f.carried_from !== undefined && f.carried_from !== null) continue;
+    const c = classOf(f.rule_key);
+    newByClass[c] = (newByClass[c] ?? 0) + 1;
+  }
+  return { blocking: blocking.length, newByClass };
+}
+
+const count = n => (Number.isInteger(n) && n >= 0 ? n : 0);
+const classesIn = m => (m && typeof m === 'object' ? Object.keys(m).filter(k => count(m[k]) > 0).sort() : []);
+
+/**
+ * Judge the stop rule after the last round of `rounds` (oldest first).
+ * @param {Array<{round?: number, blocking: number, newByClass: Object<string, number>|null}>} rounds
+ * @returns {{fired: boolean, reason: 'same-class'|'not-fewer'|null, classes: string[],
+ *   perRound: Array<{round: number, blocking: number, newByClass: Object<string, number>|null}>}}
+ */
+export function decideStop(rounds) {
+  const list = (Array.isArray(rounds) ? rounds : []).filter(x => x && typeof x === 'object');
+  const perRound = list.map((x, i) => ({
+    round: Number.isInteger(x.round) ? x.round : i + 1,
+    blocking: count(x.blocking),
+    newByClass: x.newByClass && typeof x.newByClass === 'object' ? { ...x.newByClass } : null,
+  }));
+  const none = { fired: false, reason: null, classes: [], perRound };
+  if (perRound.length < 2) return none;
+  const prev = perRound[perRound.length - 2];
+  const cur = perRound[perRound.length - 1];
+  if (prev.newByClass !== null && cur.newByClass !== null) {
+    const before = new Set(classesIn(prev.newByClass));
+    const shared = classesIn(cur.newByClass).filter(c => before.has(c));
+    if (shared.length > 0) return { fired: true, reason: 'same-class', classes: shared, perRound };
+  }
+  if (cur.blocking > 0 && cur.blocking >= prev.blocking) {
+    return { fired: true, reason: 'not-fewer', classes: classesIn(cur.newByClass), perRound };
+  }
+  return none;
+}
+
+/** The one line the operator gets when the stop rule fired (empty string when it did not). */
+export function describeStop(decision) {
+  if (!decision || decision.fired !== true) return '';
+  const rounds = Array.isArray(decision.perRound) ? decision.perRound : [];
+  const fmtNew = m => (m === null ? 'new not counted (full count)' : (classesIn(m).map(c => `${c} ${m[c]}`).join(', ') || 'none') + ' new');
+  const counts = rounds.map(p => `round ${p.round}: ${p.blocking} blocking (${fmtNew(p.newByClass)})`).join('; ');
+  const classes = Array.isArray(decision.classes) && decision.classes.length > 0 ? decision.classes.join(', ') : '(none new)';
+  const why = decision.reason === 'same-class'
+    ? 'new blocking findings of the same class two rounds in a row'
+    : 'the blocking findings did not get fewer';
+  return `stop rule (${decision.reason}): ${why}; class: ${classes}; ${counts}. ` +
+    'Recommendation: known issue (accept the open findings and go on) or new mechanism (stop; the direction goes to the operator).';
+}

tests/lib/stop-rule.test.mjs

@@ -0,0 +1,151 @@
+// tests/lib/stop-rule.test.mjs
+// Part 1, step 1.1 of the E plan (decisions 217, 220): one stop rule shared by
+// the plan loop (lib/validators/plan-gate.mjs) and the review rounds
+// (lib/review/review-rounds.mjs).
+//
+// - classOf: a catalogue key as is, else the lower-cased text before the first
+//   `/`, empty -> `unclassified` (217).
+// - roundSummary: blocking = blockers + counting majors; new = no
+//   `carried_from`; a round counted in full has newByClass null.
+// - decideStop: judged after round N >= 2 on the pair (N-1, N); `same-class`
+//   and `not-fewer`, each at its boundary; round 1 alone, a null pair (for
+//   same-class) and a history-less old verdict never fire on that reason.
+// - describeStop: one line with the class, the counts per round and the two
+//   recommendations, never "one more round".
+
+import { test } from 'node:test';
+import { strict as assert } from 'node:assert';
+import { classOf, roundSummary, decideStop, describeStop } from '../../lib/review/stop-rule.mjs';
+
+const fnd = (severity, rule_key, extra = {}) => ({ severity, rule_key, text: `${severity} ${rule_key}`, ...extra });
+const r = (round, blocking, newByClass) => ({ round, blocking, newByClass });
+
+// ---- classOf -------------------------------------------------------------------
+
+test('classOf: a catalogue key is its own class', () => {
+  assert.equal(classOf('SECURITY_INJECTION'), 'SECURITY_INJECTION');
+  assert.equal(classOf('MISSING_TEST'), 'MISSING_TEST');
+  // Case and spaces are ignored on the catalogue lookup, as isSecurityKey does.
+  assert.equal(classOf(' security_injection '), 'SECURITY_INJECTION');
+});
+
+test('classOf: otherwise the lower-cased dimension before the first slash', () => {
+  assert.equal(classOf('Step-Quality/missing-verify'), 'step-quality');
+  assert.equal(classOf('step-quality/other-instance'), 'step-quality');
+  assert.equal(classOf('creep'), 'creep');
+  assert.equal(classOf('  Gap  '), 'gap');
+  assert.equal(classOf('a/b/c'), 'a');
+});
+
+test('classOf: empty or missing -> unclassified', () => {
+  for (const k of ['', '   ', '/x', null, undefined, 42]) {
+    assert.equal(classOf(k), 'unclassified', `classOf(${JSON.stringify(k)})`);
+  }
+});
+
+// ---- roundSummary -------------------------------------------------------------
+
+test('roundSummary: blocking counts blockers and majors, never minors; new = no carried_from', () => {
+  const s = roundSummary([
+    fnd('blocker', 'dep/a'),
+    fnd('major', 'dep/b'),
+    fnd('major', 'gap', { carried_from: 1 }),
+    fnd('minor', 'dep/c'),
+    fnd('BLOCKER', 'MISSING_TEST'),
+    fnd('SUGGESTION', 'x'),
+  ], { fullCount: false });
+  assert.equal(s.blocking, 4);
+  assert.deepEqual(s.newByClass, { dep: 2, MISSING_TEST: 1 });
+});
+
+test('roundSummary: a round counted in full has newByClass null', () => {
+  const s = roundSummary([fnd('blocker', 'dep/a')], { fullCount: true });
+  assert.equal(s.blocking, 1);
+  assert.equal(s.newByClass, null);
+});
+
+test('roundSummary: no findings -> 0 and an empty map', () => {
+  assert.deepEqual(roundSummary([], {}), { blocking: 0, newByClass: {} });
+});
+
+// ---- decideStop ------------------------------------------------------------------
+
+test('decideStop: round 1 alone never fires', () => {
+  const d = decideStop([r(1, 5, { dep: 5 })]);
+  assert.equal(d.fired, false);
+  assert.equal(d.reason, null);
+});
+
+test('decideStop: an old verdict without history never fires', () => {
+  for (const h of [undefined, null, [], 'x']) {
+    assert.equal(decideStop(h).fired, false, `decideStop(${JSON.stringify(h)})`);
+  }
+});
+
+test('decideStop: same-class fires when one class has new blocking findings in N-1 and N', () => {
+  const d = decideStop([r(1, 3, { dep: 1, gap: 2 }), r(2, 1, { dep: 1 })]);
+  assert.equal(d.fired, true);
+  assert.equal(d.reason, 'same-class');
+  assert.deepEqual(d.classes, ['dep']);
+  assert.deepEqual(d.perRound.map(p => [p.round, p.blocking]), [[1, 3], [2, 1]]);
+});
+
+test('decideStop: same-class does not fire when the classes differ (boundary)', () => {
+  const d = decideStop([r(1, 3, { dep: 1, gap: 2 }), r(2, 1, { creep: 1 })]);
+  assert.equal(d.fired, false);
+  // A class with a zero count is not "new findings of that class".
+  assert.equal(decideStop([r(1, 3, { dep: 0, gap: 3 }), r(2, 1, { dep: 1 })]).fired, false);
+});
+
+test('decideStop: not-fewer fires when blocking(N) > 0 and blocking(N) >= blocking(N-1)', () => {
+  const eq = decideStop([r(1, 2, { dep: 2 }), r(2, 2, { gap: 2 })]);
+  assert.equal(eq.fired, true);
+  assert.equal(eq.reason, 'not-fewer');
+  assert.deepEqual(eq.classes, ['gap']);
+  const up = decideStop([r(1, 0, {}), r(2, 1, { gap: 1 })]);
+  assert.equal(up.fired, true);
+  assert.equal(up.reason, 'not-fewer');
+});
+
+test('decideStop: not-fewer does not fire when blocking falls or reaches 0 (boundary)', () => {
+  assert.equal(decideStop([r(1, 2, { dep: 2 }), r(2, 1, { gap: 1 })]).fired, false);
+  assert.equal(decideStop([r(1, 0, {}), r(2, 0, {})]).fired, false);
+  assert.equal(decideStop([r(1, 3, { dep: 3 }), r(2, 0, {})]).fired, false);
+});
+
+test('decideStop: a null pair judges only not-fewer', () => {
+  // Same class would fire, but round 2 was counted in full: skipped.
+  assert.equal(decideStop([r(1, 3, { dep: 3 }), r(2, 1, null)]).fired, false);
+  assert.equal(decideStop([r(1, 3, null), r(2, 1, { dep: 1 })]).fired, false);
+  const d = decideStop([r(1, 1, { dep: 1 }), r(2, 1, null)]);
+  assert.equal(d.fired, true);
+  assert.equal(d.reason, 'not-fewer');
+});
+
+test('decideStop: only the last pair is judged', () => {
+  // Rounds 1-2 would fire (same class, not fewer); rounds 2-3 do not.
+  const d = decideStop([r(1, 2, { dep: 2 }), r(2, 3, { dep: 1 }), r(3, 1, { gap: 1 })]);
+  assert.equal(d.fired, false);
+  assert.equal(d.perRound.length, 3);
+});
+
+// ---- describeStop ----------------------------------------------------------------
+
+test('describeStop: the class, the counts per round and the two recommendations, never "one more round"', () => {
+  const d = decideStop([r(1, 3, { dep: 1, gap: 2 }), r(2, 1, { dep: 1 })]);
+  const line = describeStop(d);
+  assert.equal(typeof line, 'string');
+  assert.ok(!line.includes('\n'), 'one line');
+  assert.match(line, /same-class/);
+  assert.match(line, /\bdep\b/);
+  assert.match(line, /round 1: 3 blocking/);
+  assert.match(line, /round 2: 1 blocking/);
+  assert.match(line, /known issue/i);
+  assert.match(line, /new mechanism/i);
+  assert.doesNotMatch(line, /one more round/i);
+  const nf = describeStop(decideStop([r(1, 2, { dep: 2 }), r(2, 2, { gap: 2 })]));
+  assert.match(nf, /not-fewer/);
+  assert.match(nf, /round 1: 2 blocking/);
+  assert.match(nf, /round 2: 2 blocking/);
+  assert.doesNotMatch(nf, /one more round/i);
+});