Skip to content

Commit 87fb773

Browse files
os-steveclaude
andauthored
fix(pm): dispatch-gates runs its CLI only when invoked directly (#9757) (#10084)
`scripts/pm/dispatch-gates.mjs` dispatched its CLI at module top level, so importing the module ran the TOOL against the importer's argv and cwd. Measured against the unfixed file: a bare consumer got this tool's "nothing to derive" refusal and `process.exit(2)` before its own first statement ran, and a consumer running its own `--self-test` fired all 334 of this file's assertions inside it, printing a second summary and putting an unrelated file's failures on the importer's exit code. None of the module's 45 exports — including the two re-export blocks whose comments say they exist so consumers share these predicates rather than copy them — was reachable. Same defect class and same repair as PR #9897 on `check-governed-merges.mjs`, which cites its own line 810 as precedent. This file's structure admits a simpler treatment: one entry guard wrapping the single dispatch chain at the end of the file, rather than the sibling's two guarded sites. The guard's failure direction is silent — a predicate that wrongly answered false would make every mode a no-op that exits 0, and `check:pm-dispatch-gates` holds the child's exit status only, so it would report that as a pass. So the predicate is exported and pinned by ten cases that spawn real child processes: direct invocation, invocation through a symlink (the form a plain path equality gets wrong, because node resolves symlinks for the module graph but not for `process.argv[1]`), and import by a consumer whose own argv carries `--tier` and `--self-test`. Comment and guard only: no verdict, population, tier answer or exit code moves. Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4639cec commit 87fb773

1 file changed

Lines changed: 198 additions & 50 deletions

File tree

scripts/pm/dispatch-gates.mjs

Lines changed: 198 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,21 @@
132132
*/
133133

134134
import { spawnSync } from 'node:child_process';
135-
import { readFileSync, readdirSync, existsSync, mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
135+
import {
136+
readFileSync,
137+
readdirSync,
138+
existsSync,
139+
mkdtempSync,
140+
mkdirSync,
141+
writeFileSync,
142+
rmSync,
143+
realpathSync,
144+
symlinkSync,
145+
} from 'node:fs';
136146
import { tmpdir } from 'node:os';
137-
import { join, dirname } from 'node:path';
147+
import { join, dirname, resolve } from 'node:path';
138148
import process from 'node:process';
149+
import { fileURLToPath, pathToFileURL } from 'node:url';
139150
import {
140151
anyConfigExtractsMetadataForms,
141152
findExtractConfigs,
@@ -3686,6 +3697,81 @@ function selfTest() {
36863697
rmSync(gitTmp, { recursive: true, force: true });
36873698
}
36883699

3700+
// ── The entry guard (#9757) ───────────────────────────────────────────────
3701+
//
3702+
// Both directions are measured by really spawning node, because the guard's
3703+
// own failure direction is silent in BOTH of them. If the predicate wrongly
3704+
// answered false, every CLI mode would print nothing and exit 0, and
3705+
// `check:pm-dispatch-gates` — which holds the child's exit status only —
3706+
// would report that no-op as a pass. If it wrongly answered true, the defect
3707+
// this guard exists to remove is simply still here. Reasoning about argv
3708+
// cannot tell those apart on the invocation forms that actually occur; a
3709+
// child process can.
3710+
const SELF = fileURLToPath(import.meta.url);
3711+
t('the entry predicate answers true for this module named by its own path', invokedAs(SELF, SELF));
3712+
t('and for the same file named relatively from the repo root, as the gate spells it', invokedAs(join(ROOT, 'scripts/pm/dispatch-gates.mjs'), SELF));
3713+
t('a different file in the same directory is not this module', !invokedAs(join(ROOT, 'scripts/pm/check-dispatch-gates.mjs'), SELF));
3714+
t('an absent argv[1] is not this module — the `node --eval` importer', !invokedAs(undefined, SELF) && !invokedAs('', SELF));
3715+
3716+
const entryTmp = mkdtempSync(join(tmpdir(), 'dispatch-gates-entry-'));
3717+
try {
3718+
// RUN DIRECTLY the modes must all still reach their branches. `--tier`
3719+
// stands in for every one of them: the guard is a SINGLE site wrapping the
3720+
// whole chain, so a form that reaches this branch reaches `--self-test`
3721+
// too — and spawning `--self-test` from inside `--self-test` would recurse.
3722+
const direct = spawnSync(process.execPath, [SELF, '--tier', 'packages/spec/src/data/filter.zod.ts'], {
3723+
encoding: 'utf8',
3724+
cwd: ROOT,
3725+
});
3726+
t(
3727+
'invoked directly, --tier still answers rather than exiting 0 in silence',
3728+
direct.status === 0 && (direct.stdout ?? '').trim().length > 0,
3729+
);
3730+
3731+
// REACHED THROUGH A SYMLINK — the form a plain path equality gets wrong.
3732+
// Node resolves the link for the module graph, so `import.meta.url` names
3733+
// the real file while argv[1] names the link. Under the precedent's
3734+
// one-comparison spelling this run goes inert, exit 0, no output: the
3735+
// false-green the gate cannot see.
3736+
const link = join(entryTmp, 'linked-dispatch-gates.mjs');
3737+
symlinkSync(SELF, link);
3738+
const viaLink = spawnSync(process.execPath, [link, '--tier', 'packages/spec/src/data/filter.zod.ts'], {
3739+
encoding: 'utf8',
3740+
cwd: ROOT,
3741+
});
3742+
t(
3743+
'invoked through a symlink to this file, --tier still answers',
3744+
viaLink.status === 0 && (viaLink.stdout ?? '').trim().length > 0,
3745+
);
3746+
t('and it answers the SAME thing as the direct invocation', (viaLink.stdout ?? '') === (direct.stdout ?? ''));
3747+
3748+
// IMPORTED the module must do nothing at all. The importer's argv carries
3749+
// this tool's own flags on purpose: that is the shape that fired an
3750+
// unrelated file's assertions inside the importer's self-test.
3751+
const consumer = join(entryTmp, 'consumer.mjs');
3752+
const REACHED = 'CONSUMER-REACHED function function function';
3753+
writeFileSync(
3754+
consumer,
3755+
`const m = await import(${JSON.stringify(pathToFileURL(SELF).href)});\n` +
3756+
`console.log('CONSUMER-REACHED', typeof m.maskComments, typeof m.isExtractConfigPath, typeof m.deriveTier);\n`,
3757+
);
3758+
const imported = spawnSync(process.execPath, [consumer, '--self-test', '--tier', 'packages/spec/src/index.ts'], {
3759+
encoding: 'utf8',
3760+
cwd: entryTmp,
3761+
});
3762+
t(
3763+
'imported, the importer reaches its own first statement and the re-exports are there',
3764+
imported.status === 0 && (imported.stdout ?? '').trim() === REACHED,
3765+
);
3766+
t('imported, this module prints nothing of its own on either stream', (imported.stderr ?? '').trim() === '');
3767+
t(
3768+
"imported by a consumer whose own argv says --self-test, THIS file's self-test does not fire",
3769+
!(imported.stdout ?? '').includes('dispatch-gates self-test:'),
3770+
);
3771+
} finally {
3772+
rmSync(entryTmp, { recursive: true, force: true });
3773+
}
3774+
36893775
let failed = 0;
36903776
for (const [name, cond] of cases) {
36913777
if (!cond) failed++;
@@ -3698,58 +3784,120 @@ function selfTest() {
36983784
console.log(`✓ dispatch-gates self-test: ${cases.length} cases pass.`);
36993785
}
37003786

3701-
const argvPaths = process.argv.slice(2).filter((a) => !a.startsWith('--'));
3702-
const wantsChanged = process.argv.includes('--changed');
3703-
if (process.argv.includes('--self-test')) {
3704-
selfTest();
3705-
} else if (wantsChanged && argvPaths.length > 0) {
3706-
// The two input modes answer different questions and must never be blended:
3707-
// silently preferring one would make the other's arguments vanish without a
3708-
// word, which is the class of failure this whole file is about.
3709-
console.error('dispatch-gates: --changed derives the paths itself — do not pass paths with it.');
3710-
process.exit(2);
3711-
} else {
3712-
let paths;
3713-
if (argvPaths.length > 0) {
3714-
paths = argvPaths.map((p) => p.replace(/^\.\//, ''));
3787+
// ── CLI ─────────────────────────────────────────────────────────────────────
3788+
3789+
/**
3790+
* Is `entryArg` — a `process.argv[1]` — this very module?
3791+
*
3792+
* Exported so the predicate the entry guard stands on is pinned by cases rather
3793+
* than trusted by reading. Its failure direction is SILENT: an `invokedDirectly`
3794+
* that wrongly answered `false` would turn every mode of this tool into a no-op
3795+
* that prints nothing and exits 0, and `check:pm-dispatch-gates` holds the
3796+
* child's exit STATUS only (see that gate's header) — so the no-op would report
3797+
* as a pass, which is the silent-success direction this tree treats as worse
3798+
* than no check at all.
3799+
*
3800+
* Two comparisons, because node resolves symlinks for the module graph but
3801+
* leaves `process.argv[1]` as the caller typed it. The plain `resolve` equality
3802+
* is the spelling of the landed precedent one file over
3803+
* (`scripts/pm/check-governed-merges.mjs`, whose header carries this shape's
3804+
* incident history). The realpath comparison is the half that keeps a checkout
3805+
* REACHED THROUGH A SYMLINK from reading as "imported": `import.meta.url` would
3806+
* name the real file while `argv[1]` named the link, the equality would answer
3807+
* false, and the tool would go quietly inert for whoever ran it that way. It
3808+
* falls back to `false` rather than throwing — an unreadable entry path is not
3809+
* this module.
3810+
*/
3811+
export function invokedAs(entryArg, selfPath) {
3812+
if (!entryArg) return false;
3813+
const entry = resolve(entryArg);
3814+
const self = resolve(selfPath);
3815+
if (entry === self) return true;
3816+
try {
3817+
return realpathSync(entry) === realpathSync(self);
3818+
} catch {
3819+
return false;
3820+
}
3821+
}
3822+
3823+
const invokedDirectly = invokedAs(process.argv[1], fileURLToPath(import.meta.url));
3824+
3825+
/**
3826+
* Executed only as a CLI. Importing this module must have NO side effect.
3827+
*
3828+
* Everything above this line is exported — the two re-export blocks with their
3829+
* stated rationales, and the derivation functions the self-test drives — and
3830+
* none of it was reachable while this dispatch ran at module top level. An
3831+
* `import { maskComments } from './dispatch-gates.mjs'` ran the TOOL against the
3832+
* IMPORTER's argv and cwd, and on most paths reached `process.exit(2)` before
3833+
* the importer's own first statement: measured here, a bare consumer printed
3834+
* this tool's "nothing to derive" refusal and exited 2, its own `console.log`
3835+
* never having run. On the other branch it is worse than an exit — a consumer
3836+
* running its own `--self-test` fired all of THIS file's assertions inside it,
3837+
* printing a second summary line and putting an unrelated file's failures on
3838+
* the importer's exit code. That is the same defect PR #9897 fixed in
3839+
* `check-governed-merges.mjs` at 77 assertions; this file carries it at 334. A
3840+
* self-test is a mode of the file being RUN, never a side effect of importing
3841+
* it, and a shared module that exits on import is a shared module nobody can
3842+
* share.
3843+
*
3844+
* The guard is ONE site wrapping the whole chain, not a condition repeated per
3845+
* branch: a branch added inside it later cannot forget to carry it.
3846+
*/
3847+
if (invokedDirectly) {
3848+
const argvPaths = process.argv.slice(2).filter((a) => !a.startsWith('--'));
3849+
const wantsChanged = process.argv.includes('--changed');
3850+
if (process.argv.includes('--self-test')) {
3851+
selfTest();
3852+
} else if (wantsChanged && argvPaths.length > 0) {
3853+
// The two input modes answer different questions and must never be blended:
3854+
// silently preferring one would make the other's arguments vanish without a
3855+
// word, which is the class of failure this whole file is about.
3856+
console.error('dispatch-gates: --changed derives the paths itself — do not pass paths with it.');
3857+
process.exit(2);
37153858
} else {
3716-
// No paths: derive them. This is the dev-side form — "the gates my ACTUAL
3717-
// diff implicates" — and it is the default because the caller-supplied
3718-
// list was the thing getting it wrong (#9320). `--changed` spells the same
3719-
// thing out for a caller that would rather say it than imply it.
3720-
let derived;
3859+
let paths;
3860+
if (argvPaths.length > 0) {
3861+
paths = argvPaths.map((p) => p.replace(/^\.\//, ''));
3862+
} else {
3863+
// No paths: derive them. This is the dev-side form — "the gates my ACTUAL
3864+
// diff implicates" — and it is the default because the caller-supplied
3865+
// list was the thing getting it wrong (#9320). `--changed` spells the same
3866+
// thing out for a caller that would rather say it than imply it.
3867+
let derived;
3868+
try {
3869+
derived = changedPathsFromGit();
3870+
} catch (err) {
3871+
console.error(`dispatch-gates: could not derive the change set — ${err.message}`);
3872+
console.error('usage: node scripts/pm/dispatch-gates.mjs [--residue] [--tier] [<path> ...] | --changed | --self-test');
3873+
process.exit(2);
3874+
}
3875+
if (derived.paths.length === 0) {
3876+
// An empty derivation is an input problem far more often than an answer,
3877+
// and "no gates" is the most expensive thing this tool could say wrongly
3878+
// (#4690: an unreadable input must never look like an empty answer).
3879+
console.error(
3880+
`dispatch-gates: this branch changes nothing against '${derived.base}' (merge base ${derived.mergeBase.slice(0, 9)}) — ` +
3881+
'nothing to derive. On the base branch already, or in the wrong checkout? Pass explicit paths to ask about a hypothetical surface.',
3882+
);
3883+
process.exit(2);
3884+
}
3885+
for (const line of derivationProvenance(derived)) console.error(line);
3886+
console.error('');
3887+
paths = derived.paths;
3888+
}
37213889
try {
3722-
derived = changedPathsFromGit();
3890+
// `--tier` answers the claim-time question alone: it reads no workflow and
3891+
// no check script, so it still answers on a tree where the gate derivation
3892+
// cannot run — and a claim comment is written before any of that matters.
3893+
if (process.argv.includes('--tier')) {
3894+
for (const line of tierLines(deriveTier(paths))) console.log(line);
3895+
} else {
3896+
derive(paths, { showResidue: process.argv.includes('--residue') });
3897+
}
37233898
} catch (err) {
3724-
console.error(`dispatch-gates: could not derive the change set — ${err.message}`);
3725-
console.error('usage: node scripts/pm/dispatch-gates.mjs [--residue] [--tier] [<path> ...] | --changed | --self-test');
3726-
process.exit(2);
3727-
}
3728-
if (derived.paths.length === 0) {
3729-
// An empty derivation is an input problem far more often than an answer,
3730-
// and "no gates" is the most expensive thing this tool could say wrongly
3731-
// (#4690: an unreadable input must never look like an empty answer).
3732-
console.error(
3733-
`dispatch-gates: this branch changes nothing against '${derived.base}' (merge base ${derived.mergeBase.slice(0, 9)}) — ` +
3734-
'nothing to derive. On the base branch already, or in the wrong checkout? Pass explicit paths to ask about a hypothetical surface.',
3735-
);
3899+
console.error(`dispatch-gates: derivation failed — ${err.message}`);
37363900
process.exit(2);
37373901
}
3738-
for (const line of derivationProvenance(derived)) console.error(line);
3739-
console.error('');
3740-
paths = derived.paths;
3741-
}
3742-
try {
3743-
// `--tier` answers the claim-time question alone: it reads no workflow and
3744-
// no check script, so it still answers on a tree where the gate derivation
3745-
// cannot run — and a claim comment is written before any of that matters.
3746-
if (process.argv.includes('--tier')) {
3747-
for (const line of tierLines(deriveTier(paths))) console.log(line);
3748-
} else {
3749-
derive(paths, { showResidue: process.argv.includes('--residue') });
3750-
}
3751-
} catch (err) {
3752-
console.error(`dispatch-gates: derivation failed — ${err.message}`);
3753-
process.exit(2);
37543902
}
37553903
}

0 commit comments

Comments
 (0)