fix(review): plan dedup ranks a merged pair by what rule B counts, so reviewer order never lowers a blocker
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
f70aed5d47
commit
e8ea98747d
SSH key fingerprint:
SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
4 changed files with 166 additions and 47 deletions
|
|
@ -55,10 +55,12 @@
|
|||
import { resolve } from 'node:path';
|
||||
import {
|
||||
normalizeSeverity, contentOverlap, contentTokens, DEFAULT_THRESHOLD, ANCHOR_MIN_CONTENT_WORDS, SEVERITIES,
|
||||
isSecurityKey, keysOf, isSecurity, ruleB,
|
||||
} from './plan-review-dedup.mjs';
|
||||
import { getRule } from './rule-catalogue.mjs';
|
||||
import { computeFindingId } from '../parsers/finding-id.mjs';
|
||||
|
||||
export { isSecurityKey };
|
||||
|
||||
export const RULE_A = 'Rule A: from round 2 on, a new major finding that cites a plan line outside the diff since the last reviewed text counts as minor; a blocker, a security finding and a finding with no plan line count at full severity, and a previous finding stays counted until it is marked resolved.';
|
||||
export const RULE_B = 'Rule B: a major that a named test, `Regression:` command or gate in the plan will fail on during execution is minor — never a blocker, never a security finding.';
|
||||
|
||||
|
|
@ -71,11 +73,6 @@ export const DIFF_FIELDS = Object.freeze([
|
|||
'lowering_refused', 'carried_from',
|
||||
]);
|
||||
|
||||
/** A rule_key whose catalogue entry is in the security category (case and spaces ignored). */
|
||||
export function isSecurityKey(key) {
|
||||
return getRule(String(key).trim().toUpperCase())?.category === 'security';
|
||||
}
|
||||
|
||||
const toLines = (text) => {
|
||||
const t = String(text ?? '').replace(/\r\n?/g, '\n');
|
||||
if (t === '') return [];
|
||||
|
|
@ -225,14 +222,6 @@ const countedOf = s => normalizeSeverity(s) ?? 'blocker';
|
|||
const higher = (a, b) => (rankOf(a) <= rankOf(b) ? a : b);
|
||||
const hasOwn = (o, k) => Object.prototype.hasOwnProperty.call(o, k);
|
||||
|
||||
/** The rule keys of a finding: `rule_keys` when it is a list, else its one `rule_key`. */
|
||||
function keysOf(f) {
|
||||
const keys = Array.isArray(f.rule_keys) ? f.rule_keys : [f.rule_key];
|
||||
return keys.filter(k => typeof k === 'string' && k !== '');
|
||||
}
|
||||
|
||||
const isSecurity = f => keysOf(f).some(isSecurityKey);
|
||||
|
||||
function union(...lists) {
|
||||
const out = [];
|
||||
for (const list of lists) {
|
||||
|
|
@ -241,33 +230,6 @@ function union(...lists) {
|
|||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* The rule-B record of one finding at its rated severity.
|
||||
* A null or absent `lowered_from` is no record at all.
|
||||
* @returns {{severity: string, record: object}}
|
||||
*/
|
||||
function ruleB(f, rated) {
|
||||
if (f.lowered_from === undefined || f.lowered_from === null) return { severity: rated, record: {} };
|
||||
const from = normalizeSeverity(f.lowered_from);
|
||||
const caught = typeof f.caught_by === 'string' && f.caught_by.trim() !== '' ? f.caught_by : null;
|
||||
const security = isSecurity(f);
|
||||
if (from === 'major' && rated === 'minor' && caught && !security) {
|
||||
return { severity: 'minor', record: { original_severity: 'major', lowered_by: `rule B: ${caught}`, caught_by: caught } };
|
||||
}
|
||||
let why;
|
||||
if (from === null) why = 'rule B: lowered_from is not a severity; counted as blocker';
|
||||
else if (from === 'blocker') why = 'rule B never lowers a blocker';
|
||||
else if (from !== 'major') why = `rule B lowers only major to minor, not ${from}`;
|
||||
else if (rated !== 'minor') why = `rule B lowers major to minor only; the finding is rated ${rated}`;
|
||||
else if (security) why = 'rule B never lowers a security finding';
|
||||
else why = 'rule B needs caught_by: the named test, Regression: command or gate that will fail on it';
|
||||
const severity = higher(from ?? 'blocker', rated);
|
||||
const record = { lowering_refused: why };
|
||||
if (caught) record.caught_by = caught;
|
||||
if (severity !== rated) record.original_severity = rated;
|
||||
return { severity, record };
|
||||
}
|
||||
|
||||
/** A current finding in today's verdict shape, plus the pass-through fields. */
|
||||
function project(f) {
|
||||
const rated = normalizeSeverity(f.severity);
|
||||
|
|
|
|||
|
|
@ -33,10 +33,11 @@
|
|||
// `<rule_key>#n` (n = 2, 3, …) in its id.
|
||||
//
|
||||
// Provenance is preserved on the surviving finding's `raised_by` array.
|
||||
// Severity is kept (normalized to blocker | major | minor, else null) and a
|
||||
// merge keeps the more severe of the two: the plan gate (K3,
|
||||
// Severity is kept (normalized to blocker | major | minor, else null). A merge
|
||||
// keeps the copy rule B counts highest on the merged keys, then the higher
|
||||
// rated severity; on a full tie the survivor keeps its own. The plan gate (K3,
|
||||
// lib/validators/plan-gate.mjs) stops on a BLOCKER, so a merge must never
|
||||
// downgrade one.
|
||||
// downgrade one, in either reviewer order.
|
||||
//
|
||||
// CLI shim — two input modes:
|
||||
// file mode: node lib/review/plan-review-dedup.mjs \
|
||||
|
|
@ -55,6 +56,7 @@ import { readFileSync } from 'node:fs';
|
|||
import { jaccardSimilarity, meetsThreshold } from '../parsers/jaccard.mjs';
|
||||
import { computeFindingId, parseFindingId } from '../parsers/finding-id.mjs';
|
||||
import { isMain } from '../util/is-main.mjs';
|
||||
import { getRule } from './rule-catalogue.mjs';
|
||||
|
||||
export const DEFAULT_THRESHOLD = 0.7;
|
||||
export const SEVERITIES = Object.freeze(['blocker', 'major', 'minor']);
|
||||
|
|
@ -68,6 +70,52 @@ export function normalizeSeverity(s) {
|
|||
// Lower rank = more severe; unrated ranks with blocker so a merge never hides it.
|
||||
const rank = s => (s === null ? 0 : SEVERITIES.indexOf(s));
|
||||
|
||||
// ---- rule B (lib/review/critic-diff.mjs imports these back) --------------------
|
||||
|
||||
const higher = (a, b) => (SEVERITIES.indexOf(a) <= SEVERITIES.indexOf(b) ? a : b);
|
||||
|
||||
/** A rule_key whose catalogue entry is in the security category (case and spaces ignored). */
|
||||
export function isSecurityKey(key) {
|
||||
return getRule(String(key).trim().toUpperCase())?.category === 'security';
|
||||
}
|
||||
|
||||
/** The rule keys of a finding: `rule_keys` when it is a list, else its one `rule_key`. */
|
||||
export function keysOf(f) {
|
||||
const keys = Array.isArray(f.rule_keys) ? f.rule_keys : [f.rule_key];
|
||||
return keys.filter(k => typeof k === 'string' && k !== '');
|
||||
}
|
||||
|
||||
export const isSecurity = f => keysOf(f).some(isSecurityKey);
|
||||
|
||||
/**
|
||||
* The rule-B record of one finding at its rated severity.
|
||||
* A null or absent `lowered_from` is no record at all.
|
||||
* @returns {{severity: string, record: object}}
|
||||
*/
|
||||
export function ruleB(f, rated) {
|
||||
if (f.lowered_from === undefined || f.lowered_from === null) return { severity: rated, record: {} };
|
||||
const from = normalizeSeverity(f.lowered_from);
|
||||
const caught = typeof f.caught_by === 'string' && f.caught_by.trim() !== '' ? f.caught_by : null;
|
||||
const security = isSecurity(f);
|
||||
if (from === 'major' && rated === 'minor' && caught && !security) {
|
||||
return { severity: 'minor', record: { original_severity: 'major', lowered_by: `rule B: ${caught}`, caught_by: caught } };
|
||||
}
|
||||
let why;
|
||||
if (from === null) why = 'rule B: lowered_from is not a severity; counted as blocker';
|
||||
else if (from === 'blocker') why = 'rule B never lowers a blocker';
|
||||
else if (from !== 'major') why = `rule B lowers only major to minor, not ${from}`;
|
||||
else if (rated !== 'minor') why = `rule B lowers major to minor only; the finding is rated ${rated}`;
|
||||
else if (security) why = 'rule B never lowers a security finding';
|
||||
else why = 'rule B needs caught_by: the named test, Regression: command or gate that will fail on it';
|
||||
const severity = higher(from ?? 'blocker', rated);
|
||||
const record = { lowering_refused: why };
|
||||
if (caught) record.caught_by = caught;
|
||||
if (severity !== rated) record.original_severity = rated;
|
||||
return { severity, record };
|
||||
}
|
||||
|
||||
// ---- merge ---------------------------------------------------------------------
|
||||
|
||||
function mergeInto(survivor, other) {
|
||||
for (const a of other.raised_by) {
|
||||
if (!survivor.raised_by.includes(a)) survivor.raised_by.push(a);
|
||||
|
|
@ -75,9 +123,13 @@ function mergeInto(survivor, other) {
|
|||
const keys = [...(survivor.rule_keys ?? [])];
|
||||
for (const k of other.rule_keys ?? []) if (!keys.includes(k)) keys.push(k);
|
||||
if (keys.length > 0) survivor.rule_keys = keys;
|
||||
// The more severe partner brings its rule-B record with it (or none); on
|
||||
// equal severity the survivor keeps its own.
|
||||
if (rank(other.severity) < rank(survivor.severity)) {
|
||||
// The partner wins when rule B counts it higher on the merged keys, or, on an
|
||||
// equal count, when it is rated higher; the winner brings its severity and
|
||||
// its rule-B record (or none). On a full tie the survivor keeps its own.
|
||||
const counted = x => ruleB({ ...x, rule_keys: keys }, normalizeSeverity(x.severity) ?? 'blocker').severity;
|
||||
const co = rank(counted(other));
|
||||
const cs = rank(counted(survivor));
|
||||
if (co < cs || (co === cs && rank(other.severity) < rank(survivor.severity))) {
|
||||
survivor.severity = other.severity;
|
||||
for (const field of ['lowered_from', 'caught_by']) {
|
||||
if (field in other) survivor[field] = other[field];
|
||||
|
|
|
|||
|
|
@ -16,6 +16,10 @@
|
|||
# (exit 0, REVISE); the bare gate exits 1 with PLAN_GATE_OPEN_MAJOR.
|
||||
# 8. git status --porcelain --untracked-files=all lists no snapshot; the
|
||||
# round-1 snapshot file exists under .git/voyage/plan-snapshots/.
|
||||
# 9. Round 3 (plan unchanged, counted fully): a minor merged with its copy
|
||||
# that claims a lowering from blocker, and a minor with a lowered_from
|
||||
# that is not a severity (exit 0, BLOCK, two blockers); the bare gate
|
||||
# exits 1 with PLAN_GATE_OPEN_BLOCKER.
|
||||
# Prints "e2e: ok" and exits 0, or names the failed check and exits 1.
|
||||
# bash 3.2 compatible, ASCII only.
|
||||
|
||||
|
|
@ -144,5 +148,21 @@ echo "$st" | grep -q 'plan-snapshots' && fail "git status lists a snapshot: $st"
|
|||
[ "$st" = '?? plan.md' ] || fail "git status lists more than the plan: $st"
|
||||
[ -f "$REPO/.git/voyage/plan-snapshots/$H1.md" ] || fail "no round-1 snapshot under .git/voyage/plan-snapshots/"
|
||||
|
||||
# ---- 9. round 3: a refused and a malformed lowering reach the gate as blockers ----
|
||||
|
||||
M9='Step 1 never names the exit code its verify command expects'
|
||||
G9='The revert for step one is never run against a dirty tree'
|
||||
printf '{"plan_critic":{"findings":[{"file":"plan.md","line":%s,"rule_key":"step-quality","severity":"minor","text":"%s"}]},"scope_guardian":{"findings":[{"file":"plan.md","line":%s,"rule_key":"step-quality","severity":"minor","lowered_from":"blocker","caught_by":"npm test","text":"%s"},{"file":"plan.md","line":%s,"rule_key":"gap","severity":"minor","lowered_from":"high","caught_by":"npm test","text":"%s"}]}}\n' \
|
||||
"$L1" "$M9" "$L1" "$M9" "$LO" "$G9" > "$TMP/r3.json"
|
||||
out="$(round "$TMP/r3.json")"
|
||||
rc=$?
|
||||
[ "$rc" -eq 0 ] || fail "9: round 3 --record: exit $rc, expected 0: $out"
|
||||
echo "$out" | grep -q '"verdict":"BLOCK"' || fail "9: round 3 is not BLOCK: $out"
|
||||
echo "$out" | grep -q '"blocker":2' || fail "9: round 3 does not count two blockers: $out"
|
||||
out="$(gate plan.md)"
|
||||
rc=$?
|
||||
[ "$rc" -eq 1 ] || fail "9: the bare gate exit $rc, expected 1: $out"
|
||||
echo "$out" | grep -q 'PLAN_GATE_OPEN_BLOCKER' || fail "9: no PLAN_GATE_OPEN_BLOCKER in: $out"
|
||||
|
||||
echo "e2e: ok"
|
||||
exit 0
|
||||
|
|
|
|||
|
|
@ -1110,3 +1110,88 @@ test('29: a lowered_from that is not a severity reaches the gate and counts as b
|
|||
assert.equal(g0.lowering_refused, undefined);
|
||||
}
|
||||
});
|
||||
|
||||
// ---- 30 ------------------------------------------------------------------------
|
||||
|
||||
test('30: a merged pair counts the same in either reviewer order, and a blocker claim survives a merge', () => {
|
||||
const p0 = buildPlan();
|
||||
const X = at(p0, S1_CHANGES);
|
||||
const TEXT = 'Step 1 never names the exit code its verify command expects';
|
||||
const CAUGHT = { caught_by: 'tests/mod1.test.mjs' };
|
||||
const cases = [
|
||||
{
|
||||
name: '(a) minor vs a refused blocker lowering',
|
||||
a: f('minor', X, TEXT),
|
||||
b: f('minor', X, TEXT, { lowered_from: 'blocker', ...CAUGHT }),
|
||||
severity: 'blocker', refused: 'rule B never lowers a blocker',
|
||||
},
|
||||
{
|
||||
name: '(b) plain major vs a refused blocker lowering',
|
||||
a: f('major', X, TEXT),
|
||||
b: f('minor', X, TEXT, { lowered_from: 'blocker', ...CAUGHT }),
|
||||
severity: 'blocker', refused: 'rule B never lowers a blocker',
|
||||
},
|
||||
{
|
||||
name: '(c) minor vs a major lowering with no caught_by',
|
||||
a: f('minor', X, TEXT),
|
||||
b: f('minor', X, TEXT, { lowered_from: 'major' }),
|
||||
severity: 'major',
|
||||
},
|
||||
{
|
||||
name: '(d) minor vs a security-keyed major lowering, merged by text',
|
||||
a: f('minor', X, TEXT),
|
||||
b: f('minor', X, TEXT, { rule_key: 'SECURITY_INJECTION', lowered_from: 'major', ...CAUGHT }),
|
||||
severity: 'major', refused: 'rule B never lowers a security finding',
|
||||
},
|
||||
{
|
||||
name: '(e) control: plain major vs a granted major lowering',
|
||||
a: f('major', X, TEXT),
|
||||
b: f('minor', X, TEXT, { lowered_from: 'major', ...CAUGHT }),
|
||||
severity: 'major', refused: undefined,
|
||||
},
|
||||
{
|
||||
name: '(f) minor vs a lowered_from that is not a severity',
|
||||
a: f('minor', X, TEXT),
|
||||
b: f('minor', X, TEXT, { lowered_from: 'high', ...CAUGHT }),
|
||||
severity: 'blocker', refused: 'rule B: lowered_from is not a severity; counted as blocker',
|
||||
},
|
||||
];
|
||||
for (const c of cases) {
|
||||
for (const [order, payload] of [['a first', { pc: [c.a], sg: [c.b] }], ['b first', { pc: [c.b], sg: [c.a] }]]) {
|
||||
const d = dd(payload);
|
||||
assert.equal(d.findings.length, 1, `${c.name}, ${order}: the pair is merged: ${JSON.stringify(d.findings)}`);
|
||||
const v = rec(p0, d).verdict;
|
||||
const g = byText(v, TEXT);
|
||||
assert.equal(g.severity, c.severity, `${c.name}, ${order}: counts ${c.severity}: ${JSON.stringify(g)}`);
|
||||
if ('refused' in c) {
|
||||
assert.equal(g.lowering_refused, c.refused, `${c.name}, ${order}: lowering_refused`);
|
||||
}
|
||||
assert.deepEqual([...g.raised_by].sort(), ['plan-critic', 'scope-guardian'], `${c.name}, ${order}: both raisers kept`);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
// ---- architecture: dedup never imports critic-diff or plan-gate --------------------
|
||||
|
||||
/** Every `from '...'`, `import('...')` and `require('...')` specifier naming critic-diff or plan-gate. */
|
||||
function dedupImportViolations(source) {
|
||||
const re = /\bfrom\s*(['"`])([^'"`]+)\1|\bimport\s*\(\s*(['"`])([^'"`]+)\3\s*\)|\brequire\s*\(\s*(['"`])([^'"`]+)\5\s*\)/g;
|
||||
const out = [];
|
||||
for (const m of String(source).matchAll(re)) {
|
||||
const spec = m[2] ?? m[4] ?? m[6];
|
||||
if (/(^|\/)(critic-diff|plan-gate)\.mjs$/.test(spec)) out.push(spec);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
test('architecture: plan-review-dedup imports neither critic-diff nor plan-gate (planted imports caught)', () => {
|
||||
const DEDUP_SRC = join(ROOT, 'lib', 'review', 'plan-review-dedup.mjs');
|
||||
assert.ok(existsSync(DEDUP_SRC), 'lib/review/plan-review-dedup.mjs exists');
|
||||
const src = readFileSync(DEDUP_SRC, 'utf-8');
|
||||
assert.deepEqual(dedupImportViolations(src), [], 'the real source passes');
|
||||
const plantedDiff = `${src}\nimport { countRound } from './critic-diff.mjs';\n`;
|
||||
assert.deepEqual(dedupImportViolations(plantedDiff), ['./critic-diff.mjs'], 'a planted critic-diff import fails');
|
||||
const plantedGate = `${src}\nawait import('../validators/plan-gate.mjs');\n`;
|
||||
assert.deepEqual(dedupImportViolations(plantedGate), ['../validators/plan-gate.mjs'], 'a planted plan-gate import fails');
|
||||
assert.deepEqual(dedupImportViolations(src), [], 'the real source passes again');
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue