refactor(simplify): drop a dead branch and repeated fixtures from the round-2 remediation

/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>
This commit is contained in:
Kjell Tore Guttormsen 2026-10-14 13:43:24 +02:00
commit 4d5c927c5d
Signed by: ktg
SSH key fingerprint: SHA256:JakMjO6FTBBzN0Bhfj9saOoEjaFxlSdYuZQQpM/lF9Q
7 changed files with 76 additions and 85 deletions

View file

@ -562,10 +562,8 @@ function leadingSpans(text) {
* @returns {string|null} the entry's new text, or null when the line is description
*/
function wrapLine(text, line) {
const so = String(text ?? '');
if (so.trim() === '') return line.trim();
if (so.trimEnd().endsWith(',')) return `${so} ${line.trim()}`.trim();
return null;
if (text === '') return line.trim();
return text.endsWith(',') ? `${text} ${line.trim()}` : null;
}
/**
@ -579,21 +577,23 @@ export function entryPaths(text) {
const files = [];
const issues = [];
const entries = [];
// Whether the last entry still takes wrapped lines (operator decision 29).
let open = false;
String(text ?? '').split('\n').forEach((line, i) => {
const item = LIST_ITEM.exec(line);
const rest = line.trim();
if (item) entries.push({ line: i + 1, entry: line.slice(item[0].length).trim(), closed: false });
if (item) { entries.push({ line: i + 1, entry: line.slice(item[0].length).trim() }); open = true; }
else if (rest === '') return;
else if (entries.length === 0) entries.push({ line: i + 1, entry: rest, closed: false });
else if (entries.length === 0) { entries.push({ line: i + 1, entry: rest }); open = true; }
else {
// A wrapped line joins the entry above it only after a comma, or when
// the entry is empty (wrapLine); otherwise it closes the entry, and it
// and every later wrapped line of that entry are description, never read
// (operator decision 29).
if (!open) return;
const last = entries[entries.length - 1];
if (last.closed) return;
const joined = wrapLine(last.entry, rest);
if (joined === null) last.closed = true;
if (joined === null) open = false;
else last.entry = joined;
}
});

View file

@ -101,9 +101,8 @@ export const UNVERIFIED_REASONS = Object.freeze([
// to MINOR, writing `brief_lowered_from: 'MAJOR'`. A BLOCKER and any
// `/security-review` finding keep their severity and only carry the mark.
// The verdict gate (verdict-gate.mjs) imports `illegalBriefLowering`; the
// opt-in workflow review mirrors it. `original_severity` is never read here.
// `original_severity` is what the kept finding itself asserted, before Pass 1
// raised it to its group's severity (review 08deae68).
// opt-in workflow review mirrors it. `original_severity` (what the kept finding
// itself asserted before Pass 1 raised it, review 08deae68) is never read here.
// The payload `reviewer` names lib/review/engine-findings.mjs writes, and the
// engine each one stands for.
@ -456,11 +455,11 @@ export function reasonablenessFilter(findings) {
suppress(f, 'unknown-rule_key', dropped, unverified);
continue;
}
const rule = getRule(f.rule_key);
const rule = getRule(f.rule_key); // never null: RULE_KEYS checked above
if (lowerableBriefConflict(f)) {
// The one legal lowering: a marked /code-review MAJOR on a MAJOR-tier key.
kept.push({ ...f, severity: 'MINOR', brief_lowered_from: 'MAJOR' });
} else if (rule) {
} else {
// Today's catalogue reset. A marked engine finding arrives at its tier
// (engine-findings.mjs), so it lands here only when its severity was
// changed on the way, and the reset puts it back with its mark kept.
@ -470,8 +469,6 @@ export function reasonablenessFilter(findings) {
...f, severity: rule.severity,
...(own !== rule.severity ? { original_severity: own } : {}),
});
} else {
kept.push(f);
}
}
return { kept, dropped, unverified };

View file

@ -80,15 +80,20 @@ echo "$out" | grep -q 'PLAN_STEP_PATH_UNTICKED' \
# ---- 2. the approval commit in a temp git repo ----
# make_repo <dir>: a temp git repo with one commit, no hooks, no signing.
make_repo() {
mkdir -p "$1" "$TMP/hooks-none" || fail "mkdir $1"
git -C "$1" init -q || fail "git init $1"
git -C "$1" config user.name 'E2E Test'
git -C "$1" config user.email 'test@example.invalid'
git -C "$1" config commit.gpgsign false
git -C "$1" config core.hooksPath "$TMP/hooks-none"
echo 'x' > "$1/README.md"
git -C "$1" add README.md && git -C "$1" commit -q -m 'init' || fail "initial commit in $1"
}
REPO="$TMP/repo"
mkdir -p "$REPO" "$TMP/hooks-none" || fail "mkdir $REPO"
git -C "$REPO" init -q || fail "git init"
git -C "$REPO" config user.name 'E2E Test'
git -C "$REPO" config user.email 'test@example.invalid'
git -C "$REPO" config commit.gpgsign false
git -C "$REPO" config core.hooksPath "$TMP/hooks-none"
echo 'x' > "$REPO/README.md"
git -C "$REPO" add README.md && git -C "$REPO" commit -q -m 'init' || fail "initial commit in the temp repo"
make_repo "$REPO"
echo '# brief' > "$REPO/brief.md"
H1='cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc'
@ -205,14 +210,8 @@ if echo "$out" | grep -q 'PLAN_STEP_TESTS_ONLY'; then
fi
R="$TMP/repo-red"
make_repo "$R"
mkdir -p "$R/src" "$R/tests" || fail "mkdir $R"
git -C "$R" init -q || fail "git init (second repo)"
git -C "$R" config user.name 'E2E Test'
git -C "$R" config user.email 'test@example.invalid'
git -C "$R" config commit.gpgsign false
git -C "$R" config core.hooksPath "$TMP/hooks-none"
echo 'x' > "$R/README.md"
git -C "$R" add README.md && git -C "$R" commit -q -m 'init' || fail "initial commit in the second temp repo"
echo 'export const parse = (s) => Number(s);' > "$R/src/parser.mjs"
for t in parser a b; do
cat > "$R/tests/$t.test.mjs" <<'EOT'

View file

@ -267,9 +267,7 @@ for (const [label, slice] of [['the /trekplan 1.9 block', trekplan19Block], ['th
assert.match(text, /takes\s+its\s+first\s+wrapped\s+line/,
`${label} says a \`File:\` line with nothing after it takes its first wrapped line: ${JSON.stringify(text)}`);
});
}
for (const [label, slice] of [['the /trekplan 1.9 block', trekplan19Block], ['the template 1.9 paragraph', template19Paragraph]]) {
test(`${label} names the placeholders, \`./Name\` and both nones`, () => {
const text = slice();
assert.ok(text.includes('`TBD`') && text.includes('`N/A`'), `${label} names the placeholders (\`TBD\`, \`N/A\`): ${JSON.stringify(text)}`);
@ -304,14 +302,20 @@ function firstChangelogSection() {
return { heading: lines[start], body: lines.slice(start + 1, end).join('\n') };
}
// The Unreleased section's body, after checking it is the first section.
function unreleasedBody() {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
return body;
}
test('CHANGELOG: the first "## " section is the Unreleased plan_version 1.9 section', () => {
assert.equal(firstChangelogSection().heading, UNRELEASED);
});
test('CHANGELOG: parseDoors reads the Unreleased section\'s door lines, none bad, one naming plan_version 1.9', () => {
assert.equal(typeof releaseCheck.parseDoors, 'function', 'scripts/release-check.mjs exports parseDoors');
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const d = releaseCheck.parseDoors(body);
assert.equal(d.found, true, 'the Unreleased section has a "### Doors" heading');
assert.ok(d.entries.length > 0, 'the Doors list has door lines');
@ -333,8 +337,7 @@ const knownIssues = (body) => {
};
test('CHANGELOG: the Unreleased section\'s Known issues name the session split', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const known = knownIssues(body);
assert.match(known, /session split/, `Known issues name the session split: ${JSON.stringify(known)}`);
});
@ -347,8 +350,7 @@ test('CHANGELOG: the Unreleased section\'s Known issues name the session split',
// the intro's measurement names the second remediation's Step 8 commit. Word
// boundaries keep "operator decision 2026-10-12" from counting as decision 20.
test('CHANGELOG: the door lines name operator decisions 20 and 23, and none says "every test file on the Test first File line"', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const texts = releaseCheck.parseDoors(body).entries.map((e) => e.text);
assert.ok(texts.some((t) => /\boperator decision 20\b/.test(t)),
`a door line names operator decision 20: ${JSON.stringify(texts)}`);
@ -359,14 +361,12 @@ test('CHANGELOG: the door lines name operator decisions 20 and 23, and none says
});
test('CHANGELOG: the Unreleased section no longer says "on a severity tie"', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
assert.doesNotMatch(body, /on\s+a\s+severity\s+tie/);
});
test('CHANGELOG: Known issues name the verdict gate, the engine payloads and operator decision 25', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const known = knownIssues(body);
assert.match(known, /verdict\s+gate/, `Known issues name the verdict gate: ${JSON.stringify(known)}`);
assert.match(known, /engine\s+payloads/, `Known issues name the engine payloads: ${JSON.stringify(known)}`);
@ -374,8 +374,7 @@ test('CHANGELOG: Known issues name the verdict gate, the engine payloads and ope
});
test('CHANGELOG: the intro names the second remediation\'s Step 8 commit, and no longer acd0e3d, 5b94952 or the Step 13 commit', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const firstSub = body.search(/^### /m);
const intro = firstSub < 0 ? body : body.slice(0, firstSub);
assert.ok(!intro.includes('acd0e3d'), `the intro no longer names acd0e3d: ${JSON.stringify(intro)}`);
@ -396,8 +395,7 @@ test('CHANGELOG: the intro names the second remediation\'s Step 8 commit, and no
const oneSpace = (text) => text.replace(/\s+/g, ' ');
test('CHANGELOG: door lines name operator decisions 29, 30 and 31, none bad, and the decision-29 line says "takes its first wrapped line"', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const d = releaseCheck.parseDoors(body);
assert.deepEqual(d.bad, [], 'every top-level door line reads "- one-way: ..." or "- two-way: ..."');
const texts = d.entries.map((e) => e.text);
@ -410,16 +408,14 @@ test('CHANGELOG: door lines name operator decisions 29, 30 and 31, none bad, and
});
test('CHANGELOG: the section names 08deae68, edf2ec56 and operator decision 27', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
assert.ok(body.includes('08deae68'), 'the Unreleased section names 08deae68');
assert.ok(body.includes('edf2ec56'), 'the Unreleased section names edf2ec56');
assert.match(body, /\boperator decision 27\b/, 'the Unreleased section names operator decision 27');
});
test('CHANGELOG: the b4fa1927 bullet follows operator decision 29', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const b = bullet(body, /b4fa1927/, 'Fixed bullet b4fa1927');
assert.match(b, /\boperator decision 29\b/, `the b4fa1927 bullet names operator decision 29: ${JSON.stringify(b)}`);
assert.ok(b.includes('4b6460e5'), `the b4fa1927 bullet names 4b6460e5: ${JSON.stringify(b)}`);
@ -432,8 +428,7 @@ test('CHANGELOG: the b4fa1927 bullet follows operator decision 29', () => {
});
test('CHANGELOG: the 6c31212c bullet names operator decision 31 below 1.9 and keeps decision 20 at every plan_version', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const b = bullet(body, /6c31212c/, 'Fixed bullet 6c31212c');
assert.match(b, /\boperator decision 31\b/, `the 6c31212c bullet names operator decision 31: ${JSON.stringify(b)}`);
assert.match(b, /\bplus\b/, `the 6c31212c bullet names plus: ${JSON.stringify(b)}`);
@ -444,8 +439,7 @@ test('CHANGELOG: the 6c31212c bullet names operator decision 31 below 1.9 and ke
});
test('CHANGELOG: the edf2ec56, 08deae68 and 7961a700 bullets say what was fixed', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const e = bullet(body, /edf2ec56/, 'Fixed bullet edf2ec56');
assert.match(e, /allow_blocked_by/, `the edf2ec56 bullet names allow_blocked_by: ${JSON.stringify(e)}`);
assert.match(e, /catalogue\s+tier\s+or\s+higher/, `the edf2ec56 bullet says "catalogue tier or higher": ${JSON.stringify(e)}`);
@ -457,8 +451,7 @@ test('CHANGELOG: the edf2ec56, 08deae68 and 7961a700 bullets say what was fixed'
});
test('CHANGELOG: the cf241526 bullet follows operator decision 30', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const b = bullet(body, /cf241526/, 'Fixed bullet cf241526');
assert.match(b, /\boperator decision 30\b/, `the cf241526 bullet names operator decision 30: ${JSON.stringify(b)}`);
assert.match(b, /pass\s+no\s+`--tests`/, `the cf241526 bullet says the callers pass no \`--tests\`: ${JSON.stringify(b)}`);
@ -471,8 +464,7 @@ test('CHANGELOG: the cf241526 bullet follows operator decision 30', () => {
});
test('CHANGELOG: the old rerun wording is gone from the Unreleased section', () => {
const { heading, body } = firstChangelogSection();
assert.equal(heading, UNRELEASED);
const body = unreleasedBody();
const flat = oneSpace(body);
for (const phrase of ['reruns red proof once with `plan_tests`', 'reruns the same way', 'the list red proof accepted']) {
assert.ok(!flat.includes(phrase), `the Unreleased section no longer says "${phrase}"`);

View file

@ -410,23 +410,20 @@ test('red proof --plan: a plan that cannot be read is not-run, exit 2 (plan-unre
const { dir, base } = repo('a - b');
try {
addTest(dir); // red at the start point: a missing plan must not read as proven
const missing = join(dir, 'nope.md');
for (const given of [[], ['--tests', 'tests/sum.test.mjs']]) {
const c = cli(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, ...given, '--plan', missing, '--step', '1', '--json']);
assert.equal(c.status, 2, `${given.join(' ')}: ${c.stdout}${c.stderr}`);
const out = JSON.parse(c.stdout);
assert.equal(out.status, 'not-run', JSON.stringify(out));
assert.equal(typeof out.reason, 'string', JSON.stringify(out));
assert.match(out.reason, /^plan-unreadable: cannot read the plan: /, JSON.stringify(out));
}
// A readable plan without the step named.
const plan = planFile(dir);
for (const given of [[], ['--tests', 'tests/sum.test.mjs']]) {
const c = cli(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, ...given, '--plan', plan, '--step', '9', '--json']);
assert.equal(c.status, 2, `${given.join(' ')}: ${c.stdout}${c.stderr}`);
const out = JSON.parse(c.stdout);
assert.equal(out.status, 'not-run', JSON.stringify(out));
assert.equal(out.reason, 'plan-unreadable: no "### Step 9:" in the plan', JSON.stringify(out));
const cases = [
[join(dir, 'nope.md'), '1', /^plan-unreadable: cannot read the plan: /],
// A readable plan without the step named.
[planFile(dir), '9', /^plan-unreadable: no "### Step 9:" in the plan$/],
];
for (const [plan, step, reason] of cases) {
for (const given of [[], ['--tests', 'tests/sum.test.mjs']]) {
const c = cli(['--red-only', '--cwd', dir, '--base', base, '--cmd', TEST_CMD, ...given, '--plan', plan, '--step', step, '--json']);
assert.equal(c.status, 2, `${given.join(' ')}: ${c.stdout}${c.stderr}`);
const out = JSON.parse(c.stdout);
assert.equal(out.status, 'not-run', JSON.stringify(out));
assert.equal(typeof out.reason, 'string', JSON.stringify(out));
assert.match(out.reason, reason, JSON.stringify(out));
}
}
} finally { rmSync(dir, { recursive: true, force: true }); }
});

View file

@ -498,11 +498,18 @@ test('decision 13 — an engine payload decides the engine: a /security-review p
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 = {}) {
const reviewerPayload = { reviewer: 'code-correctness-reviewer', findings: [reviewerCopy({ rule_key: 'MISSING_TEST', ...reviewerExtra })] };
const enginePayload = { reviewer: 'claude-code-review', findings: [codeReviewDefect({ rule_key: 'MISSING_TEST', contradicts_brief: NON_GOAL })] };
return order === 'reviewer first' ? [reviewerPayload, enginePayload] : [enginePayload, reviewerPayload];
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']) {
@ -622,9 +629,9 @@ test('own property — toReviewerPayload refuses engine toString with the "engin
// A reviewer copy at `reviewerSeverity` and an engine CODE_REVIEW_DEFECT at
// `engineSeverity` on one triplet, in `order`.
function reviewerAndEngine(order, reviewerSeverity, engineSeverity, reviewerExtra = {}) {
const reviewerPayload = { reviewer: 'code-correctness-reviewer', findings: [reviewerCopy({ severity: reviewerSeverity, ...reviewerExtra })] };
const enginePayload = { reviewer: 'claude-code-review', findings: [codeReviewDefect({ severity: engineSeverity })] };
return order === 'reviewer first' ? [reviewerPayload, enginePayload] : [enginePayload, reviewerPayload];
return reviewerAndEnginePayloads(order,
reviewerCopy({ severity: reviewerSeverity, ...reviewerExtra }),
codeReviewDefect({ severity: engineSeverity }));
}
function assertNoPass1Field(result) {

View file

@ -93,6 +93,7 @@ test('armB — quick mode expects only the /code-review engine', async () => {
const COORD_MISSING = 'missing-reviewer:review-coordinator';
const MEH_MINOR = () => ({ severity: 'MINOR', rule_key: 'MISSING_ERROR_HANDLING', file: 'src/r.mjs', line: 4, title: 't', detail: 'd' });
const CRI_MINOR = () => ({ severity: 'MINOR', rule_key: 'CODE_REVIEW_IMPROVEMENT', file: 'src/e.mjs', line: 8, title: 't', detail: 'd' });
const UC_BLOCKER = () => ({ severity: 'BLOCKER', rule_key: 'UNIMPLEMENTED_CRITERION', file: 'src/r.mjs', line: 2, title: 't', detail: 'd' });
const BOTH_ENGINES = (code = [], security = []) => [engine('claude-code-review', code), engine('claude-security-review', security)];
async function coordCase({ coordinator, reviewerFindings = [MEH_MINOR()], codeFindings = [CRI_MINOR()] }) {
@ -123,8 +124,7 @@ for (const [label, make] of failedCoordinators) {
}
test('edf2ec56 — a coordinator returning an empty findings array and no verdict over a reviewer BLOCKER → BLOCK, the BLOCKER in findings', async () => {
const blocker = { severity: 'BLOCKER', rule_key: 'UNIMPLEMENTED_CRITERION', file: 'src/r.mjs', line: 2, title: 't', detail: 'd' };
const { r } = await coordCase({ coordinator: { findings: [] }, reviewerFindings: [blocker], codeFindings: [] });
const { r } = await coordCase({ coordinator: { findings: [] }, reviewerFindings: [UC_BLOCKER()], codeFindings: [] });
assert.equal(r.verdict, 'BLOCK');
assert.equal(r.findings.filter((f) => f.rule_key === 'UNIMPLEMENTED_CRITERION' && f.severity === 'BLOCKER').length, 1);
assert.deepEqual(r.allow_blocked_by, [COORD_MISSING]);
@ -196,8 +196,7 @@ test('edf2ec56 control — no findings at all → ALLOW, the coordinator skipped
});
test('edf2ec56 control — a BLOCKER with a null coordinator → BLOCK', async () => {
const blocker = { severity: 'BLOCKER', rule_key: 'UNIMPLEMENTED_CRITERION', file: 'src/r.mjs', line: 2, title: 't', detail: 'd' };
const { r } = await coordCase({ coordinator: null, reviewerFindings: [blocker], codeFindings: [] });
const { r } = await coordCase({ coordinator: null, reviewerFindings: [UC_BLOCKER()], codeFindings: [] });
assert.equal(r.verdict, 'BLOCK');
assert.deepEqual(r.allow_blocked_by, [COORD_MISSING]);
});