diff --git a/docs/REVIEW_LOG.md b/docs/REVIEW_LOG.md index 1996e6b..53110d4 100644 --- a/docs/REVIEW_LOG.md +++ b/docs/REVIEW_LOG.md @@ -7791,3 +7791,47 @@ rounds"* fails, the ratchet trips, **the cited-figure-with-duplicate case is now the clean tree is silent. **State:** 21 guards, one merged reader, CLEAN GROUND v0.24. + +--- + +## R-309 — a guard that stopped another guard from running + +**The fold exported `scanDocument` so the readers could be tested on strings, and for two +commits importing it ran the whole guard.** `check-behaviour` imports that module. So: + +``` +$ # with the historical 24 restored +$ node tools/check-behaviour.mjs +check-figures: FAILED — measured figures are printed with nothing holding them + docs/scenarios/CLEAN_GROUND.md:1277 — "**24**" carries no citation marker … +exit=1 +``` + +**A figure defect called `process.exit(1)` inside check-behaviour, which reported another +guard's failure under its own name having run zero of its 108 tests.** The build still went +red, which is the only reason this was survivable — but it went red in the wrong place, and +every behavioural test in the repo was silently not running while appearing to. + +**This is the sharpest form yet of the fault this log keeps recording.** R-300 found a check +that passed for a reason unrelated to what it claimed. This is a check that *did not run at +all* and could not say so, because the thing that stopped it was a different guard's verdict +arriving through an import. A guard is a claim we are still checking; this one had quietly +stopped being checked, and the evidence that it had was indistinguishable from a failure. + +**Fixed with the idiom already in `step5-split.mjs`** — the executable body sits behind +`import.meta.main || process.argv[1]?.endsWith(...)`, so importing the module yields the four +readers and nothing else. + +**The test is a spawn, not a source assertion.** The property is *importing has no effect*, +and only a fresh process can tell you that; asserting the source contains the idiom would +pass on a file that had grown a second side effect elsewhere. Importing must print nothing +and exit 0. + +**Proved both ways.** Reverting the guard to `if (true)` fails exactly one test — the new one. +With a figure defect planted, `check-behaviour` now runs all 108 tests green and +`check-figures` fails in its own slot with its own message. + +**It arrived with the fold and it is mine.** It exists because R-307 needed an export for the +string tests, and it was found because those same tests print their own name — the second +time in one evening that the string tests paid for themselves in a way nobody designed. The +general rule for this repo: **a module a guard imports must not be a guard that runs.** diff --git a/tools/check-behaviour.mjs b/tools/check-behaviour.mjs index 03a7fa9..4115c2b 100644 --- a/tools/check-behaviour.mjs +++ b/tools/check-behaviour.mjs @@ -33,6 +33,7 @@ import { STARTER_AUTHORITY, STARTER_GROUPS, STARTER_CASE, STARTER_TEAM } from "./scenario-starter.mjs"; import { castMarkersIn, castLikeIn } from "./declared-cast.mjs"; import { scanDocument } from "./check-figures.mjs"; +import { spawnSync } from "node:child_process"; import { tagsIn, classifiedFiles, playableFiles } from "./check-scenarios.mjs"; import { beatsIn, beatLikeIn } from "./outcome-coverage.mjs"; import { step5Split, split as step5Of, THING_RATING } from "./step5-split.mjs"; @@ -999,6 +1000,22 @@ test("a document that cites nothing is held by the ratchet, not the strict regim assert.ok(r.unmarked >= 1, "but its bare figures are still counted for the ratchet"); }); +test("importing check-figures runs the readers and NOT the guard", () => { + /* THE REGRESSION THIS EXISTS FOR: for one commit the guard's body ran at import, so a + figure defect called process.exit(1) inside THIS file — check-behaviour reporting + check-figures' failure under its own name, having run none of its tests. Spawned rather + than asserted on the source, because the property is "importing has no effect", and only + a fresh process can tell you that. */ + const mod = new URL("./check-figures.mjs", import.meta.url).href; + const r = spawnSync(process.execPath, + ["--input-type=module", "-e", `await import(${JSON.stringify(mod)});`], + { encoding: "utf8" }); + assert.equal(r.status, 0, "importing check-figures must not exit non-zero"); + assert.equal((r.stdout + r.stderr).trim(), "", + "importing check-figures must print nothing — the guard's body belongs behind the " + + "import.meta.main check, or a figure defect stops this file mid-run"); +}); + test("the reader reports the line the figure is actually on", () => assert.match(firstFault(doc("one\ntwo\nwipes **8.3%** here")), /x\.md:3/)); diff --git a/tools/check-figures.mjs b/tools/check-figures.mjs index c6cc718..b7853a0 100644 --- a/tools/check-figures.mjs +++ b/tools/check-figures.mjs @@ -370,82 +370,92 @@ export function scanDocument(label, src) { return { rows, strict, waived, unmarked, optedIn }; } -const strict = []; -const loose = {}; -const rows = []; -const waived = []; +/* ── running the guard ───────────────────────────────────────────────────────────────── + EVERYTHING BELOW RUNS ONLY WHEN THIS FILE IS THE ENTRY POINT, and the reason is not tidiness. + `scanDocument` is exported so check-behaviour can test the readers on strings (R-292), and + for one commit importing it ALSO ran the whole guard: a figure defect called `process.exit(1)` + inside check-behaviour, which then reported check-figures' failure under its own name having + run ZERO of its 107 tests. A guard that stops another guard from running while appearing to + participate is the worst version of the fault this file exists to catch. Same idiom as + step5-split.mjs. */ +if (import.meta.main || process.argv[1]?.endsWith("check-figures.mjs")) { + const strict = []; + const loose = {}; + const rows = []; + const waived = []; -for (const [label, abs] of playableFiles()) { - const r = scanDocument(label, readFileSync(abs, "utf8")); - rows.push(...r.rows); strict.push(...r.strict); waived.push(...r.waived); - if (!r.optedIn) loose[label] = r.unmarked; -} + for (const [label, abs] of playableFiles()) { + const r = scanDocument(label, readFileSync(abs, "utf8")); + rows.push(...r.rows); strict.push(...r.strict); waived.push(...r.waived); + if (!r.optedIn) loose[label] = r.unmarked; + } -/* ── the ratchet, for documents outside the citation regime ────────────────────────────── */ + /* ── the ratchet, for documents outside the citation regime ────────────────────────────── */ -const base = existsSync(BASELINE) ? JSON.parse(readFileSync(BASELINE, "utf8")) : null; + const base = existsSync(BASELINE) ? JSON.parse(readFileSync(BASELINE, "utf8")) : null; -if (UPDATE) { - writeFileSync(BASELINE, JSON.stringify({ - note: "Unmarked measurement figures in scenarios that use no citations. May fall, never rise. " - + "Generated by tools/check-figures.mjs --update.", - uncited: loose - }, null, 1) + "\n"); - console.log(`check-figures: recorded ${Object.keys(loose).length} document(s) outside the citation regime`); - process.exit(0); -} + if (UPDATE) { + writeFileSync(BASELINE, JSON.stringify({ + note: "Unmarked measurement figures in scenarios that use no citations. May fall, never rise. " + + "Generated by tools/check-figures.mjs --update.", + uncited: loose + }, null, 1) + "\n"); + console.log(`check-figures: recorded ${Object.keys(loose).length} document(s) outside the citation regime`); + process.exit(0); + } -const problems = [...strict]; + const problems = [...strict]; -if (!base) { - problems.push(` no ratchet at tools/figures-baseline.json — run: node tools/check-figures.mjs --update`); -} else { - for (const [label, n] of Object.entries(loose)) { - const was = base.uncited?.[label]; - if (was === undefined) { - problems.push(` ${label} uses no citations and is not in the ratchet. Re-record with --update, ` - + `or give it its first citation marker and it joins the strict regime.`); - } else if (n > was) { - problems.push(` ${label}: unmarked measurement figures went ${was} -> ${n}. This document cites ` - + `nothing, so the ratchet is all that holds it: the count may fall, never rise.`); + if (!base) { + problems.push(` no ratchet at tools/figures-baseline.json — run: node tools/check-figures.mjs --update`); + } else { + for (const [label, n] of Object.entries(loose)) { + const was = base.uncited?.[label]; + if (was === undefined) { + problems.push(` ${label} uses no citations and is not in the ratchet. Re-record with --update, ` + + `or give it its first citation marker and it joins the strict regime.`); + } else if (n > was) { + problems.push(` ${label}: unmarked measurement figures went ${was} -> ${n}. This document cites ` + + `nothing, so the ratchet is all that holds it: the count may fall, never rise.`); + } + } + /* A document that LEAVES the loose set has gained its first citation, which is good news + and must not be reported as a missing row. A document that vanishes entirely has been + deleted or reclassified, and that is worth saying out loud. */ + for (const label of Object.keys(base.uncited ?? {})) { + if (!(label in loose) && !rows.some(r => r.label === label)) { + problems.push(` ${label} is in the ratchet but no longer in the corpus. If it was renamed or ` + + `deleted, re-record with --update.`); + } } } - /* A document that LEAVES the loose set has gained its first citation, which is good news - and must not be reported as a missing row. A document that vanishes entirely has been - deleted or reclassified, and that is worth saying out loud. */ - for (const label of Object.keys(base.uncited ?? {})) { - if (!(label in loose) && !rows.some(r => r.label === label)) { - problems.push(` ${label} is in the ratchet but no longer in the corpus. If it was renamed or ` - + `deleted, re-record with --update.`); + + if (LIST) { + for (const r of rows) { + console.log(` ${r.marked ? "cited " : "BARE "} ${r.label}:${r.line} [${r.kind}] ${r.text}`); } + problems.forEach(p => console.log(p)); + process.exit(0); } -} - -if (LIST) { - for (const r of rows) { - console.log(` ${r.marked ? "cited " : "BARE "} ${r.label}:${r.line} [${r.kind}] ${r.text}`); + if (problems.length) { + console.error("check-figures: FAILED — measured figures are printed with nothing holding them"); + problems.forEach(p => console.error(p)); + console.error(` check-cited resolves the markers that exist; it cannot see a figure that has none. ` + + `That is how "a median 24 rounds" survived ten passes against a baseline holding 21.`); + process.exit(1); + } + /* WAIVERS ARE PRINTED ON EVERY GREEN BUILD. A figure nobody checks is the defect this guard + exists for, and a waived figure is one we have decided to stop checking — so the decision + stays on screen rather than becoming the default. Six of them is a short list; sixty would + be a finding. */ + if (waived.length) { + console.log(`check-figures: ${waived.length} figure(s) waived, each on the record:`); + for (const w of waived) console.log(` ${w.label}:${w.line} ${w.text} — ${w.reason}`); } - problems.forEach(p => console.log(p)); - process.exit(0); -} -if (problems.length) { - console.error("check-figures: FAILED — measured figures are printed with nothing holding them"); - problems.forEach(p => console.error(p)); - console.error(` check-cited resolves the markers that exist; it cannot see a figure that has none. ` - + `That is how "a median 24 rounds" survived ten passes against a baseline holding 21.`); - process.exit(1); -} -/* WAIVERS ARE PRINTED ON EVERY GREEN BUILD. A figure nobody checks is the defect this guard - exists for, and a waived figure is one we have decided to stop checking — so the decision - stays on screen rather than becoming the default. Six of them is a short list; sixty would - be a finding. */ -if (waived.length) { - console.log(`check-figures: ${waived.length} figure(s) waived, each on the record:`); - for (const w of waived) console.log(` ${w.label}:${w.line} ${w.text} — ${w.reason}`); -} -const marked = rows.filter(r => r.marked).length; -const looseTotal = Object.values(loose).reduce((a, b) => a + b, 0); -console.log(`check-figures: OK — ${rows.length} measurement-shaped figures across ${new Set(rows.map(r => r.label)).size} ` - + `scenarios, ${marked} marked and ${waived.length} waived; ${looseTotal} bare in ` - + `${Object.keys(loose).length} document(s) that cite nothing, held at the ratchet`); + const marked = rows.filter(r => r.marked).length; + const looseTotal = Object.values(loose).reduce((a, b) => a + b, 0); + console.log(`check-figures: OK — ${rows.length} measurement-shaped figures across ${new Set(rows.map(r => r.label)).size} ` + + `scenarios, ${marked} marked and ${waived.length} waived; ${looseTotal} bare in ` + + `${Object.keys(loose).length} document(s) that cite nothing, held at the ratchet`); +}