Skip to content

Commit 272be13

Browse files
committed
fix(pm): the dispatch-gates wrapper's kill branch exits 3, so its code says what its text says
The `result.signal` branch printed "⛔ Do not record it as a run" and then exited 2. `dispatch-gates.mjs --ran` classifies a recorded exit FROM THE CODE: 3 is the repo-wide NOT MEASURED code, 124 and the 128 + signum floor are kills, and everything else is a run. 2 is none of those, so a cap-killed battery recorded honestly as `:: exit 2` reconciled inside the `run` total — a family counted as measured on a run that measured nothing. The branch now exits EXIT_PREREQUISITE_NOT_MET and names the code in its own remedy text. A child that RAN keeps `result.status`; the spawn-failure branch keeps 2, because "the tool is gone" and "the box could not fork" reach it wearing the same error and only the second is a not-measured reading. Pinned by this file's first self-test: three stub children (one that signals its own pid, one red, one green) driven through a `--self-test-child` flag, asserting the wrapper's exit for each AND how `runReconciliation` reads a record of it — including a regression pin that 2 still reconciles as a run. Wired the sibling way (`--self-test && <bare>`), so its row leaves check-self-test-wired's SELF_TEST_RUN_OTHERWISE ledger. Claude-Session: https://claude.ai/code/session_01HZfg2AwVX191qCizp88gQr Co-authored-by: Claude <noreply@anthropic.com>
1 parent e592b4b commit 272be13

3 files changed

Lines changed: 229 additions & 12 deletions

File tree

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@
7373
"check:pm-skill-ratchet": "node scripts/pm/check-skill-line-ratchet.mjs --self-test && node scripts/pm/check-skill-line-ratchet.mjs",
7474
"check:pm-skill-id-lint": "node scripts/pm/check-skill-id-lint.mjs --self-test && node scripts/pm/check-skill-id-lint.mjs",
7575
"check:pm-label-desc-cap": "node scripts/pm/check-label-desc-cap.mjs --self-test && node scripts/pm/check-label-desc-cap.mjs",
76-
"check:pm-dispatch-gates": "node scripts/pm/check-dispatch-gates.mjs",
76+
"check:pm-dispatch-gates": "node scripts/pm/check-dispatch-gates.mjs --self-test && node scripts/pm/check-dispatch-gates.mjs",
7777
"check:watch-hint-literal": "node scripts/check-watch-hint-literal.mjs --self-test && node scripts/check-watch-hint-literal.mjs",
7878
"check:declared-population-live": "node scripts/check-declared-population-live.mjs --self-test && node scripts/check-declared-population-live.mjs",
7979
"check:pm-half-states": "node scripts/pm/check-half-states.mjs --self-test",

‎scripts/check-self-test-wired.mjs‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -233,12 +233,6 @@ const INVOCATION_RE =
233233
* unrun self-test gets wired instead.
234234
*/
235235
const SELF_TEST_RUN_OTHERWISE = [
236-
{
237-
script: 'scripts/pm/check-dispatch-gates.mjs',
238-
via: 'drives',
239-
evidence: 'scripts/pm/dispatch-gates.mjs',
240-
why: 'its whole body spawns the tool with --self-test; lint.yml runs the wrapper bare',
241-
},
242236
{
243237
script: 'scripts/docs-audit/check-affected-docs.mjs',
244238
via: 'drives',

‎scripts/pm/check-dispatch-gates.mjs‎

Lines changed: 228 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,47 @@
5353
* on a quiet box reaches a verdict at all, where before it could only ever be
5454
* killed.
5555
*
56+
* ## The exit contract, and why the kill branch does not keep its old code
57+
*
58+
* Four endings, four codes, and what fixes them is not this file's taste — it
59+
* is what READS them. A dev records a gate's exit beside the command it ran
60+
* (a `:: exit N` tail) and `dispatch-gates.mjs --ran` reconciles that record
61+
* against the families it derives, classifying each line FROM THE CODE. A code
62+
* here is therefore a claim addressed to a reconciler, and this file had one of
63+
* them wrong:
64+
*
65+
* the battery passed result.status (0) a run that passed
66+
* the battery failed result.status a run that failed
67+
* the battery was KILLED EXIT_PREREQUISITE_NOT_MET NOT MEASURED
68+
* the tool could not spawn 2 neither, on purpose
69+
*
70+
* The kill row used to exit 2, and 2 is in none of the reconciler's classes —
71+
* not its NOT-MEASURED code, and not its kill set (coreutils `timeout`'s 124
72+
* and the 128 + signum floor), so it landed inside the `run` total. A family
73+
* counted as measured on the strength of a run that measured nothing is the
74+
* exact false green that reconciliation exists to refuse. The branch's own text
75+
* said the opposite in the same breath — do not record it as a run — and was
76+
* the only carrier saying it, so a reader who copied the number rather than the
77+
* sentence filed a red run over a gate that never reached a verdict.
78+
*
79+
* ⛔ The fix is NOT 143, the shape a shell reports for a SIGTERM'd child. The
80+
* reconciler reads a kill code as UNRUN unless the runner ALSO writes a
81+
* NOT-MEASURED claim with a stated reason beside it — deliberately, because an
82+
* unexplained kill is where an unfinished run hides. That is the right default
83+
* for a runner relaying a kill it did not diagnose. It is the wrong one here,
84+
* where this branch has already identified the kill, printed the reason and
85+
* named the remedy before it exits: everything the claim line would carry is
86+
* already on stderr, and requiring a second line to be remembered is how the
87+
* reading gets lost again. EXIT_PREREQUISITE_NOT_MET carries it in the code
88+
* itself, and it is the repo-wide code for this reading.
89+
*
90+
* The spawn-failure row keeps 2 deliberately; its reason sits at that branch.
91+
*
92+
* ⛔ None of this moves CI. lint.yml runs this gate as an ordinary step with no
93+
* `continue-on-error`, so every non-zero code above is equally red there. These
94+
* codes are read by devs and by `--ran`, and the contract is pinned by this
95+
* file's own `--self-test` rather than by this paragraph.
96+
*
5697
* ## Why the gate exists
5798
*
5899
* scripts/pm/dispatch-gates.mjs derives the "local gates for this card" line of
@@ -183,8 +224,13 @@
183224
*/
184225

185226
import { spawnSync } from 'node:child_process';
186-
import { join } from 'node:path';
227+
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
228+
import { tmpdir } from 'node:os';
229+
import { join, resolve } from 'node:path';
187230
import process from 'node:process';
231+
import { fileURLToPath } from 'node:url';
232+
233+
import { EXIT_PREREQUISITE_NOT_MET } from '../import-prerequisite.mjs';
188234

189235
const ROOT = new URL('../..', import.meta.url).pathname;
190236

@@ -223,8 +269,54 @@ const SURFACE_MODULE = 'scripts/i18n-bundle-surface.mjs';
223269
*/
224270
const FRAME_MODULE = 'scripts/check-skill-frame-sync.mjs';
225271

272+
/**
273+
* The flag under which this file drives a child OTHER than TOOL, spelled so
274+
* that any invocation carrying it reads as a test invocation wherever it is
275+
* written down.
276+
*
277+
* ⛔ Not `--tool`, and not an `OS_TEST_*` environment variable. The exit
278+
* contract below cannot be pinned without standing the 435-second battery
279+
* down, and the battery is named by a module-body constant, so SOMETHING has
280+
* to be substitutable. What that something must never be is quiet: a run
281+
* against a stub grades nothing about the real tool, so the one failure this
282+
* affordance could introduce is a green gate over a child that is not TOOL.
283+
* An environment variable is the shape that fails that way — it is inherited
284+
* from whatever shell the runner was started in, so it can arrive without
285+
* appearing in any invocation anyone reads. A flag cannot: it is spelled at
286+
* the call site, `package.json` holds the only invocation CI runs, and the
287+
* word `self-test` is in the flag itself. The substituted run also announces
288+
* itself on stderr, and the self-test asserts that it does — so the loudness
289+
* is live rather than promised.
290+
*
291+
* The right boundary matters to a sibling gate: `check-self-test-wired`
292+
* matches `--self-test` with one, so this longer flag is not read as an
293+
* invocation of the self-test, and no row is credited for it.
294+
*/
295+
const SELF_TEST_CHILD_FLAG = '--self-test-child';
296+
297+
/** The flag that runs this file's own battery instead of the tool's. */
298+
const SELF_TEST_FLAG = '--self-test';
299+
300+
const argv = process.argv.slice(2);
301+
302+
if (argv.includes(SELF_TEST_FLAG)) await selfTest();
303+
304+
const childAt = argv.indexOf(SELF_TEST_CHILD_FLAG);
305+
const child = childAt < 0 ? TOOL : argv[childAt + 1];
306+
if (child === undefined || child === '') {
307+
console.error(`✗ check:pm-dispatch-gates: ${SELF_TEST_CHILD_FLAG} needs a path after it.`);
308+
process.exit(2);
309+
}
310+
if (child !== TOOL) {
311+
console.error(
312+
`⛔ check:pm-dispatch-gates: SUBSTITUTED CHILD — this run spawned ${child}, not ${TOOL}, so it grades` +
313+
" NOTHING about the tool. It exists so this file's own self-test can drive the exit contract below" +
314+
' without standing up the real battery. A production run never prints this line.',
315+
);
316+
}
317+
226318
const started = Date.now();
227-
const result = spawnSync(process.execPath, [join(ROOT, TOOL), '--self-test'], { stdio: 'inherit' });
319+
const result = spawnSync(process.execPath, [resolve(ROOT, child), '--self-test'], { stdio: 'inherit' });
228320
/**
229321
* What the battery cost on THIS box, printed rather than frozen anywhere.
230322
*
@@ -237,7 +329,13 @@ const result = spawnSync(process.execPath, [join(ROOT, TOOL), '--self-test'], {
237329
const seconds = ((Date.now() - started) / 1000).toFixed(1);
238330

239331
if (result.error) {
240-
console.error(`✗ check:pm-dispatch-gates: could not run ${TOOL} — ${result.error.message}`);
332+
// ⛔ NOT the kill branch's code, and the difference is argued in the header's
333+
// exit-contract section: a spawn that never started has two causes this file
334+
// cannot tell apart — the tool is gone from the tree, which is a finding
335+
// about the tree, and the box could not fork, which is not. 3 would assert
336+
// the second reading over both. Until one measurement separates them this
337+
// stays the code that claims neither.
338+
console.error(`✗ check:pm-dispatch-gates: could not run ${child} — ${result.error.message}`);
241339
process.exit(2);
242340
}
243341
if (result.signal) {
@@ -246,18 +344,143 @@ if (result.signal) {
246344
// the question a killed caller actually has — what do I do now — rather
247345
// than naming the signal and stopping, which is what it used to do.
248346
console.error(
249-
`✗ check:pm-dispatch-gates: ${TOOL} --self-test was killed by ${result.signal} after ${seconds}s — NOTHING was measured.`,
347+
`✗ check:pm-dispatch-gates: ${child} --self-test was killed by ${result.signal} after ${seconds}s — NOTHING was measured.`,
250348
);
251349
console.error(
252350
' Every case decided before the kill is in the output above and every case after it is unjudged, so this run' +
253351
' grades neither the tool nor your diff. ⛔ Do not record it as a run.',
254352
);
353+
console.error(
354+
` This exits ${EXIT_PREREQUISITE_NOT_MET}, the repo-wide NOT MEASURED code, so a record line` +
355+
` \`<command> :: exit ${EXIT_PREREQUISITE_NOT_MET}\` reconciles as NOT-MEASURED rather than as a run that failed.`,
356+
);
255357
console.error(
256358
" Remedy — detach it and read the log, the invocation this file's header prescribes for a capped container:",
257359
);
258360
console.error(' nohup pnpm check:pm-dispatch-gates > pm-dispatch-gates.log 2>&1 &');
259361
console.error(' then tail that log until it stops growing. CI runs this step with no such cap.');
260-
process.exit(2);
362+
process.exit(EXIT_PREREQUISITE_NOT_MET);
261363
}
262364
console.error(`check:pm-dispatch-gates: the battery took ${seconds}s on this box.`);
263365
process.exit(result.status ?? 2);
366+
367+
/**
368+
* This file's own battery — the exit contract, driven against stub children.
369+
*
370+
* ## Why it cannot simply run the gate
371+
*
372+
* Every other assertion about this file would be a reading of the 435-second
373+
* tool battery, and the branch under test only fires when that battery is
374+
* KILLED. A self-test that reproduced the real conditions would have to stand
375+
* the battery up and then race it, which is the one shape that cannot be run
376+
* on every PR. So the child is substituted and the three ways a child can end
377+
* are written directly: killed by a signal, exited red, exited green.
378+
*
379+
* ## Why the killed stub kills ITSELF
380+
*
381+
* Measured both ways on this box. Signalling from outside — spawn the wrapper,
382+
* find its child, send it a SIGTERM — needs the grandchild's pid, so it races
383+
* the spawn and reads the process table to get it; and `timeout -s TERM` on
384+
* the wrapper does not exercise this branch AT ALL, because the wrapper has no
385+
* SIGTERM handler and dies with the child, leaving `timeout`'s own 124 and no
386+
* `result.signal` anywhere. A stub that signals its own pid has no race and no
387+
* pid lookup: node with no SIGTERM listener takes the default disposition, so
388+
* `spawnSync` reports `signal: 'SIGTERM'` and `status: null` — the exact shape
389+
* a foreground-cap kill produces, reached deterministically.
390+
*
391+
* ## The half that is NOT about this file
392+
*
393+
* A number is only a contract if something reads it that way, so each exit is
394+
* also pushed through the reconciler that consumes it, on a derivation of one
395+
* family. That is what makes `3` mean NOT MEASURED here rather than merely
396+
* being three — and the old `2` is pinned alongside it, still reconciling as a
397+
* run, so the case that motivated the change cannot quietly come back.
398+
*/
399+
async function selfTest() {
400+
let failures = 0;
401+
const t = (name, ok) => {
402+
console.log(`${ok ? '✓' : '✗'} ${name}`);
403+
if (!ok) failures += 1;
404+
};
405+
406+
const SELF = fileURLToPath(import.meta.url);
407+
const dir = mkdtempSync(join(tmpdir(), 'check-dispatch-gates-selftest-'));
408+
try {
409+
const stub = (name, body) => {
410+
const at = join(dir, name);
411+
writeFileSync(at, body);
412+
return at;
413+
};
414+
// Signals its own pid: no listener is registered, so the default
415+
// disposition ends the process and the timer only keeps the loop alive in
416+
// case delivery is not synchronous.
417+
const killedChild = stub('killed.mjs', "process.kill(process.pid, 'SIGTERM');\nsetTimeout(() => {}, 5000);\n");
418+
const redChild = stub('red.mjs', 'process.exit(1);\n');
419+
const greenChild = stub('green.mjs', 'process.exit(0);\n');
420+
421+
const drive = (childPath) =>
422+
spawnSync(process.execPath, [SELF, SELF_TEST_CHILD_FLAG, childPath], { encoding: 'utf8' });
423+
424+
const killed = drive(killedChild);
425+
const red = drive(redChild);
426+
const green = drive(greenChild);
427+
428+
t(
429+
'CONTROL: the killed stub really died by SIGNAL, so the branch under test is the one that ran',
430+
killed.stderr.includes('was killed by SIGTERM'),
431+
);
432+
t(
433+
`⭐ a child killed by a signal exits ${EXIT_PREREQUISITE_NOT_MET} — the code this branch's own text asks for`,
434+
killed.status === EXIT_PREREQUISITE_NOT_MET,
435+
);
436+
t(
437+
"…and the text that asks for it is still printed, so the two carriers cannot drift apart silently",
438+
killed.stderr.includes('Do not record it as a run') && killed.stderr.includes('NOTHING was measured'),
439+
);
440+
t('a child that RAN and failed keeps its own status', red.status === 1);
441+
t('a child that RAN and passed keeps its own status', green.status === 0);
442+
t(
443+
'⛔ and a substituted child announces itself on every one of those runs — a stub run is never quiet',
444+
[killed, red, green].every((r) => r.stderr.includes('SUBSTITUTED CHILD')),
445+
);
446+
447+
// The reconciler's own reading of those three codes. Imported rather than
448+
// spawned: what is under test is how a RECORDED code classifies, not how
449+
// the derivation finds this family, and a spawned derivation would add a
450+
// full workflow-tree walk per case to every run of this gate.
451+
const { parseRunRecord, runReconciliation, RUN_RECORD_EXIT_PREFIX, RUN_RECORD_REASON_SEPARATOR } = await import(
452+
resolve(ROOT, TOOL)
453+
);
454+
const COMMAND = 'pnpm check:pm-dispatch-gates';
455+
const reconcile = (code) =>
456+
runReconciliation({
457+
derived: [COMMAND],
458+
record: parseRunRecord(`${COMMAND}${RUN_RECORD_REASON_SEPARATOR}${RUN_RECORD_EXIT_PREFIX}${code}`),
459+
});
460+
461+
const onKill = reconcile(killed.status);
462+
t(
463+
`⭐ …and a record of that exit reconciles as NOT-MEASURED, which is the whole contract`,
464+
onKill.notMeasured.length === 1 && onKill.ran.length === 0 && onKill.unrun.length === 0,
465+
);
466+
t(
467+
'…derived from the CODE, not claimed by the runner — so a killed battery cannot be recorded as a run by hand',
468+
onKill.notMeasured[0]?.source === 'exit-code',
469+
);
470+
t('a red run reconciles as a RUN, and the reconciliation holds', reconcile(red.status).ran.length === 1);
471+
t('a green run reconciles as a RUN too', reconcile(green.status).ran.length === 1);
472+
t(
473+
'⛔ REGRESSION PIN: the code this branch used to exit still reconciles as a RUN — which is why it moved',
474+
reconcile(2).ran.length === 1 && reconcile(2).notMeasured.length === 0,
475+
);
476+
} finally {
477+
rmSync(dir, { recursive: true, force: true });
478+
}
479+
480+
console.log(
481+
failures === 0
482+
? '✓ check:pm-dispatch-gates --self-test: the exit contract holds in all three directions.'
483+
: `✗ check:pm-dispatch-gates --self-test: ${failures} case(s) failed.`,
484+
);
485+
process.exit(failures === 0 ? 0 : 1);
486+
}

0 commit comments

Comments
 (0)