feat(review): stop rule in the review rounds (decision 217, 220)
prepare (also --dry-run) judges the previous round's archive and review.md when it would open round 3: blocking = BLOCKER + counting MAJOR with the verdict gate's pre-existing reading, new = no carried_from, class = classOf. Fired -> exit 1, `review-stop-rule: <describeStop line>` and `stop_rule`, nothing archived. The cap stays. A round with no readable JSON block gives no summary and the rule is not judged. countingMajors moves from review-answers.mjs into review-rounds.mjs and is re-exported by review-answers (as isSecurityFinding is); review-rounds still imports nothing from review-answers. Existing test updated for the planned rule: the [CRY] occurrence case adds a third round-1 MAJOR resolved in round 2, so its round-3 control is not stopped by "not fewer". Part 1, step 1.3 of the E plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
a576391e28
commit
6b44be4c43
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 164 additions and 21 deletions
|
|
@ -4,8 +4,8 @@
|
|||
// from git and never trusted as written: `lib/review/review-finalize.mjs`
|
||||
// writes it (`markFindings`), and the verdict gate re-derives it
|
||||
// (`preExistingCheck`) before a MAJOR leaves the answer set. A BLOCKER or a
|
||||
// security finding counts in full whatever the mark (lib/review/review-answers.mjs
|
||||
// `countingMajors`); every uncertain case counts as new.
|
||||
// security finding counts in full whatever the mark (lib/review/review-rounds.mjs
|
||||
// `countingMajors`, re-exported by review-answers.mjs); every uncertain case counts as new.
|
||||
//
|
||||
// The line sets come from one `git diff -U0` of `scope_sha_start..scope_sha_end`
|
||||
// with flags that no repo config changes (`--no-color`, `--no-ext-diff`,
|
||||
|
|
|
|||
|
|
@ -82,7 +82,7 @@ import { basename, dirname, join, resolve } from 'node:path';
|
|||
import { fencedBlocks, isJsonBlock } from './findings-schema.mjs';
|
||||
import { reviewedSha } from './reviewed-commit.mjs';
|
||||
import { preExistingCheck, scopeShas } from './pre-existing.mjs';
|
||||
import { archiveName, frontmatterField, isSecurityFinding, runId, sameFinding } from './review-rounds.mjs';
|
||||
import { archiveName, countingMajors, frontmatterField, isSecurityFinding, runId, sameFinding } from './review-rounds.mjs';
|
||||
import {
|
||||
appendApproval, isHollowAnswer, logPath, normalizeSubject, readLog, verifyLog,
|
||||
} from '../util/approval-log.mjs';
|
||||
|
|
@ -203,21 +203,12 @@ export function parseAnswers(body) {
|
|||
return { rows, errors };
|
||||
}
|
||||
|
||||
/** A security finding (defined in review-rounds.mjs; re-exported here for the existing callers). */
|
||||
export { isSecurityFinding };
|
||||
|
||||
/**
|
||||
* Every MAJOR finding (case-insensitive) that counts toward the lock. A finding
|
||||
* is left out only when `preExisting(f)` is true and it is not a security finding.
|
||||
* @param {{findings?: object[]}} payload
|
||||
* @param {{ preExisting?: (f: object) => boolean }} [opts]
|
||||
* @returns {object[]}
|
||||
* A security finding, and every MAJOR that counts toward the lock (both
|
||||
* defined in review-rounds.mjs, which the stop rule reads them from;
|
||||
* re-exported here for the existing callers).
|
||||
*/
|
||||
export function countingMajors(payload, { preExisting = () => false } = {}) {
|
||||
const findings = Array.isArray(payload?.findings) ? payload.findings : [];
|
||||
return findings.filter((f) => f && typeof f === 'object' && isMajor(f)
|
||||
&& !(preExisting(f) === true && !isSecurityFinding(f)));
|
||||
}
|
||||
export { countingMajors, isSecurityFinding };
|
||||
|
||||
/** The parsed trailing JSON block, or null. */
|
||||
function payloadOf(text) {
|
||||
|
|
|
|||
|
|
@ -21,6 +21,17 @@
|
|||
// disagrees is exit 2. The new round is P + 1. An archive that is a
|
||||
// regular file but cannot be read is exit 2 (not measured).
|
||||
// - Round MAX_REVIEW_ROUNDS + 1 is refused (exit 1).
|
||||
// - The stop rule (decisions 217, 220; lib/review/stop-rule.mjs): from round
|
||||
// 3 on (P >= 2; under the cap only round 3), the previous archive (round
|
||||
// P - 1, the other archive with the highest <n>) and review.md (round P)
|
||||
// are judged. Blocking = BLOCKER + counting MAJOR (`countingMajors`, with
|
||||
// the verdict gate's pre-existing reading: a finding with no
|
||||
// `carried_from` whose mark `preExistingCheck` re-derives); new = no
|
||||
// `carried_from`; class = `classOf`. Fired -> exit 1 with
|
||||
// `review-stop-rule: <describeStop line>` and `stop_rule`; nothing is
|
||||
// archived. A round whose trailing JSON block cannot be read gives no
|
||||
// summary, and the rule is then not judged (as a plan verdict without
|
||||
// history). The cap stays.
|
||||
// - The round base is the previous `scope_sha_end`: a full sha that is an
|
||||
// ancestor of HEAD, else exit 2 (not measured).
|
||||
// - review.md is copied to `review-round-<run>-<P>.md`; an existing file
|
||||
|
|
@ -59,7 +70,8 @@
|
|||
// node lib/review/review-rounds.mjs prepare --project <d> --since <sha>
|
||||
// [--repo <d>] [--dry-run] --json
|
||||
// prints one JSON line {round, round_base, previous, run} or {reason};
|
||||
// exit 0 = the round, 1 = refused (the cap),
|
||||
// exit 0 = the round, 1 = refused (the cap, or the stop rule:
|
||||
// `review-stop-rule: ...`, with `stop_rule`),
|
||||
// 2 = not measured (2 is never green)
|
||||
// node lib/review/review-rounds.mjs --cap prints MAX_REVIEW_ROUNDS
|
||||
|
||||
|
|
@ -70,8 +82,9 @@ import { join } from 'node:path';
|
|||
import { fencedBlocks, isJsonBlock } from './findings-schema.mjs';
|
||||
import { contentOverlap, DEFAULT_THRESHOLD } from './plan-review-dedup.mjs';
|
||||
import { FULL_SHA_RE, reviewedSha } from './reviewed-commit.mjs';
|
||||
import { scopeShas } from './pre-existing.mjs';
|
||||
import { preExistingCheck, scopeShas } from './pre-existing.mjs';
|
||||
import { getRule } from './rule-catalogue.mjs';
|
||||
import { decideStop, describeStop, roundSummary } from './stop-rule.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
|
||||
export const MAX_REVIEW_ROUNDS = 3;
|
||||
|
|
@ -122,6 +135,51 @@ function blockSha(text) {
|
|||
|
||||
const notMeasured = (reason, detail = '') => ({ exit: 2, reason: `${reason} — not measured${detail ? ` (${detail})` : ''}` });
|
||||
|
||||
const isMajor = (f) => String(f?.severity ?? '').toUpperCase() === 'MAJOR';
|
||||
const isBlocker = (f) => String(f?.severity ?? '').toUpperCase() === 'BLOCKER';
|
||||
|
||||
/**
|
||||
* Every MAJOR finding (case-insensitive) that counts toward the lock. A finding
|
||||
* is left out only when `preExisting(f)` is true and it is not a security finding.
|
||||
* Defined here and re-exported by review-answers.mjs, which imports this module.
|
||||
* @param {{findings?: object[]}} payload
|
||||
* @param {{ preExisting?: (f: object) => boolean }} [opts]
|
||||
* @returns {object[]}
|
||||
*/
|
||||
export function countingMajors(payload, { preExisting = () => false } = {}) {
|
||||
const findings = Array.isArray(payload?.findings) ? payload.findings : [];
|
||||
return findings.filter((f) => f && typeof f === 'object' && isMajor(f)
|
||||
&& !(preExisting(f) === true && !isSecurityFinding(f)));
|
||||
}
|
||||
|
||||
/** The parsed trailing JSON block of a review (an object), or null. */
|
||||
function payloadOf(text) {
|
||||
const last = fencedBlocks(String(text ?? '')).at(-1);
|
||||
if (!last || !last.closed || !isJsonBlock(last)) return null;
|
||||
try {
|
||||
const p = JSON.parse(last.body);
|
||||
return p && typeof p === 'object' && !Array.isArray(p) && Array.isArray(p.findings) ? p : null;
|
||||
} catch { return null; }
|
||||
}
|
||||
|
||||
/**
|
||||
* One review round's stop-rule summary: its BLOCKERs and counting MAJORs, read
|
||||
* as the verdict gate reads them (a finding with no `carried_from` leaves the
|
||||
* count only when `preExistingCheck` re-derives its mark), or null when the
|
||||
* review has no readable JSON block.
|
||||
* @returns {{blocking: number, newByClass: Object<string, number>}|null}
|
||||
*/
|
||||
export function reviewRoundSummary(text, repo) {
|
||||
const payload = payloadOf(text);
|
||||
if (!payload) return null;
|
||||
const pre = preExistingCheck(text, repo);
|
||||
const blocking = [
|
||||
...payload.findings.filter((f) => f && typeof f === 'object' && isBlocker(f)),
|
||||
...countingMajors(payload, { preExisting: (f) => !f.carried_from && pre(f) }),
|
||||
];
|
||||
return roundSummary(blocking, { fullCount: false });
|
||||
}
|
||||
|
||||
/**
|
||||
* The round of a run, with its archive and clean slate (see the header).
|
||||
* @returns {{exit: 0, round: number, round_base: string|null, previous: string|null, run: string, archived?: boolean, deleted: string[]} | {exit: 1|2, reason: string}}
|
||||
|
|
@ -179,6 +237,19 @@ export function prepare({ projectDir, since, repo, dryRun = false } = {}) {
|
|||
if (round > MAX_REVIEW_ROUNDS) {
|
||||
return { exit: 1, reason: `review round cap reached (${MAX_REVIEW_ROUNDS}) — the operator re-plans` };
|
||||
}
|
||||
if (P >= 2) {
|
||||
// The stop rule on the pair (P - 1, P): the latest other archive and review.md.
|
||||
const nOf = (name) => Number(archiveRe.exec(name)[1]);
|
||||
const prevName = [...others].sort((a, b) => nOf(b) - nOf(a))[0];
|
||||
const prevBytes = readRegular(join(projectDir, prevName));
|
||||
const at = repo ?? projectDir;
|
||||
const a = prevBytes === null ? null : reviewRoundSummary(prevBytes.toString('utf8'), at);
|
||||
const b = reviewRoundSummary(text, at);
|
||||
if (a && b) {
|
||||
const d = decideStop([{ round: P - 1, ...a }, { round: P, ...b }]);
|
||||
if (d.fired) return { exit: 1, reason: `review-stop-rule: ${describeStop(d)}`, stop_rule: d };
|
||||
}
|
||||
}
|
||||
const base = ownEnd;
|
||||
if (!base || !isAncestorOfHead(repo ?? projectDir, base)) {
|
||||
return notMeasured('round base is not an ancestor of HEAD');
|
||||
|
|
|
|||
|
|
@ -841,22 +841,27 @@ test('[CRY] an answer to one carried occurrence, linked from another carried occ
|
|||
const fx = rndRepo();
|
||||
const Y1 = { severity: 'MAJOR', file: 'a.mjs', line: 3, title: 'Parser drops the last token', detail: 'parse() stops one token early' };
|
||||
const Y2 = { severity: 'MAJOR', file: 'a.mjs', line: 5, title: 'Cache never expires entries', detail: 'ttl ignored in get()' };
|
||||
fx.write(rndReview({ start: fx.start, end: fx.c1, findings: [Y1, Y2] }).text);
|
||||
// Y3 is resolved in round 2, so the blocking count falls (3 -> 2) and the stop
|
||||
// rule (lib/review/stop-rule.mjs, "not fewer") leaves round 3 open for the Control below.
|
||||
const Y3 = { severity: 'MAJOR', file: 'b.mjs', line: 3, rule_key: 'SCOPE_CREEP_BUILT', title: 'Adds a flag nobody asked for', detail: 'a --fast switch the brief never named' };
|
||||
fx.write(rndReview({ start: fx.start, end: fx.c1, findings: [Y1, Y2, Y3] }).text);
|
||||
fx.out(CCR);
|
||||
const f1 = fx.finalize(1);
|
||||
assert.equal(f1.status, 0, f1.stdout + f1.stderr);
|
||||
const [id1, id2] = jsonOf(fx.read()).findings.map((x) => x.id);
|
||||
const [id1, id2] = jsonOf(fx.read()).findings.filter((x) => x.file === 'a.mjs').map((x) => x.id);
|
||||
const id3 = jsonOf(fx.read()).findings.find((x) => x.file === 'b.mjs').id;
|
||||
const rec = run([ANSWERS, 'record', '--review', fx.reviewPath, '--finding', id1, '--answer', 'accepted', '--repo', fx.repo,
|
||||
'--round', '1', '--store', 'local', '--trailer', TRAILER], { input: 'the parser keeps the old contract on purpose' });
|
||||
assert.equal(rec.status, 0, `record: ${rec.stdout}${rec.stderr}`);
|
||||
const p = fx.prepare();
|
||||
assert.equal(p.status, 0, p.stdout + p.stderr);
|
||||
roundTwo(fx, [Y1, Y2], { outs: { [CCR]: [] } });
|
||||
roundTwo(fx, [Y1, Y2], { outs: { [CCR]: [{ id: id3, resolved: true }] } });
|
||||
const f2 = fx.finalize2();
|
||||
assert.equal(f2.status, 0, f2.stdout + f2.stderr);
|
||||
const carry = run([ANSWERS, 'carry', '--review', fx.reviewPath, '--previous', fx.archive(1), '--repo', fx.repo]);
|
||||
assert.equal(carry.status, 0, `review-answers carry: ${carry.stdout}${carry.stderr}`);
|
||||
const good = fx.read();
|
||||
assert.deepEqual(jsonOf(good).resolved, [id3], 'Y3 is resolved in round 2');
|
||||
const g1 = jsonOf(good).findings.find((x) => x.carried_from === id1);
|
||||
const g2 = jsonOf(good).findings.find((x) => x.carried_from === id2);
|
||||
assert.ok(g1 && g2, `both MAJORs are carried: ${JSON.stringify(jsonOf(good).findings)}`);
|
||||
|
|
@ -969,3 +974,79 @@ test('[FXL] Phase 8.8 offers only Fix it for a finding pending marks security: t
|
|||
'--round', '1', '--store', 'local', '--trailer', TRAILER], { input: 'the token is a test token' });
|
||||
assert.equal(acc.status, 1, `Control: a security finding is never accepted: ${acc.stdout}${acc.stderr}`);
|
||||
});
|
||||
|
||||
// ---- [STP] the stop rule (E plan, Part 1 step 1.3; decisions 217, 220) ----------------
|
||||
//
|
||||
// prepare judges the previous round's archive and review.md when it would open
|
||||
// round 3 (lib/review/stop-rule.mjs): one class with new blocking findings in
|
||||
// both rounds (`same-class`), or blocking findings that did not get fewer
|
||||
// (`not-fewer`), refuses the round with exit 1 and `review-stop-rule: ...`,
|
||||
// --dry-run included, and archives nothing. Blocking = BLOCKER + counting
|
||||
// MAJOR, with the verdict gate's pre-existing reading; new = no carried_from.
|
||||
|
||||
const BLK_A5 = { severity: 'BLOCKER', file: 'a.mjs', line: 5, rule_key: 'BROKEN_SUCCESS_CRITERION', title: 'Second build path never flushes either', detail: 'emit() skips the final flush call' };
|
||||
const MAJ_A5 = { severity: 'MAJOR', file: 'a.mjs', line: 5, rule_key: 'SCOPE_CREEP_BUILT', title: 'Adds a cache nobody asked for', detail: 'a memo layer the brief never named' };
|
||||
|
||||
test('[STP] countingMajors lives in review-rounds and review-answers re-exports the same function', async () => {
|
||||
assert.equal(typeof rounds.countingMajors, 'function', 'review-rounds.mjs exports countingMajors');
|
||||
const answers = await import(pathToFileURL(ANSWERS).href);
|
||||
assert.equal(answers.countingMajors, rounds.countingMajors, 'review-answers re-exports it');
|
||||
const payload = { findings: [{ severity: 'major', rule_key: 'MISSING_TEST' }, { severity: 'MINOR' }, { severity: 'MAJOR', rule_key: 'SECURITY_INJECTION', pre_existing: true }, { severity: 'MAJOR', rule_key: 'MISSING_TEST', pre_existing: true }] };
|
||||
assert.equal(rounds.countingMajors(payload, { preExisting: (f) => f.pre_existing === true }).length, 2);
|
||||
});
|
||||
|
||||
test('[STP] a new BLOCKER of the same class in rounds 1 and 2: prepare refuses round 3 with review-stop-rule, --dry-run too, nothing archived', () => {
|
||||
const fx = rndRepo();
|
||||
roundOne(fx, [BLK_B3]);
|
||||
roundTwo(fx, [BLK_A5], { engines: [] });
|
||||
const before = fx.snapshot();
|
||||
for (const extra of [['--dry-run'], []]) {
|
||||
const r = fx.prepare(extra);
|
||||
assert.equal(r.status, 1, `${extra.join(' ')}: ${r.stdout}${r.stderr}`);
|
||||
const reason = String(parse(r).reason);
|
||||
assert.match(reason, /^review-stop-rule: stop rule \(same-class\)/, r.stdout);
|
||||
assert.match(reason, /class: BROKEN_SUCCESS_CRITERION/, r.stdout);
|
||||
assert.match(reason, /round 1: 1 blocking/, r.stdout);
|
||||
assert.match(reason, /round 2: 1 blocking/, r.stdout);
|
||||
assert.doesNotMatch(reason, /one more round/i);
|
||||
assert.deepEqual(fx.snapshot(), before, 'a refused round writes, copies and deletes nothing');
|
||||
}
|
||||
});
|
||||
|
||||
test('[STP] a carried MAJOR and nothing fewer: prepare refuses round 3 (not-fewer)', () => {
|
||||
const fx = rndRepo();
|
||||
const { prev } = roundOne(fx, [MAJ_A3]);
|
||||
roundTwo(fx, [{ ...MAJ_A3, carried_from: prev.findings[0].id }], { engines: [] });
|
||||
const r = fx.prepare(['--dry-run']);
|
||||
assert.equal(r.status, 1, r.stdout + r.stderr);
|
||||
assert.match(String(parse(r).reason), /^review-stop-rule: stop rule \(not-fewer\)/, r.stdout);
|
||||
});
|
||||
|
||||
test('[STP] fewer blocking findings of another class: round 3 opens (a MINOR of the old class is not blocking)', () => {
|
||||
const fx = rndRepo();
|
||||
roundOne(fx, [BLK_B3, MAJ_A3]);
|
||||
roundTwo(fx, [MAJ_A5, { ...BLK_A5, severity: 'MINOR' }], { engines: [] });
|
||||
const r = fx.prepare(['--dry-run']);
|
||||
assert.equal(r.status, 0, r.stdout + r.stderr);
|
||||
assert.equal(parse(r).round, 3);
|
||||
});
|
||||
|
||||
test('[STP] the gate\'s pre-existing reading: a pre-existing MAJOR does not count, a pre-existing security MAJOR does', () => {
|
||||
const fx = rndRepo();
|
||||
// The work on a branch whose fork point from main is `start`, so a mark can be re-derived.
|
||||
g(fx.repo, 'checkout', '-q', '-b', 'feat');
|
||||
g(fx.repo, 'branch', '-f', 'main', fx.start);
|
||||
roundOne(fx, [BLK_B3, MAJ_A3]);
|
||||
const PRE = { severity: 'MAJOR', file: 'a.mjs', line: 8, rule_key: 'PLAN_EXECUTE_DRIFT', title: 'Old helper drifts from the plan', detail: 'an untouched line' };
|
||||
const r2 = roundTwo(fx, [MAJ_A5, PRE], { engines: [] });
|
||||
const mark = (rule) => fx.write(editFindings(r2.text, (x) => (x.line === 8 ? { ...x, rule_key: rule, pre_existing: true } : x)));
|
||||
// Control: a pre-existing MAJOR leaves the count: 1 < 2, round 3 opens.
|
||||
mark('PLAN_EXECUTE_DRIFT');
|
||||
const quiet = fx.prepare(['--dry-run']);
|
||||
assert.equal(quiet.status, 0, `Control: ${quiet.stdout}${quiet.stderr}`);
|
||||
// A security finding counts whatever the mark: 2 >= 2.
|
||||
mark('SECURITY_INJECTION');
|
||||
const r = fx.prepare(['--dry-run']);
|
||||
assert.equal(r.status, 1, r.stdout + r.stderr);
|
||||
assert.match(String(parse(r).reason), /^review-stop-rule: stop rule \(not-fewer\).*round 2: 2 blocking/, r.stdout);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue