Skip to content

Commit b409fff

Browse files
baozhoutaoclaude
andauthored
fix(devx): check-comment-mask-corpus no longer drops argv[0] when --masker is absent (#17352)
`argv.indexOf('--masker')` returns -1 when the flag is absent, and -1 + 1 === 0, so the filter computing `rest` treated "no --masker anywhere" as "drop index 0" -- removing the first argument from the very list the unknown-option check reads next. An unknown option in first position (e.g. --no-such-flag) was silently dropped, the usage check never fired, and the script fell through to a full 6,484-file sweep, exiting 0 instead of refusing with EXIT_USAGE. Extracts the computation into argvAfterMasker(argv, maskerFlag), guarded so `rest` is argv itself (not a filtered copy) when --masker is absent. --masker in first position is unaffected and unchanged. Adds 9 --self-test cases (floor 17 -> 26): argvAfterMasker exercised directly for an unknown option at argv[0], a middle position, and the position right after --masker, plus real spawned-process assertions (following the objectui port's approach) pinning the exit code as an actual number returned by the OS. Measured before deciding: the issue's suggested patch carried `&& arg !== '--self-test'`, but --self-test is dispatched by the entrypoint before main() is ever called, so that clause would be dead code here -- not ported. Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU Co-authored-by: Claude <noreply@anthropic.com>
1 parent d755bfa commit b409fff

1 file changed

Lines changed: 88 additions & 2 deletions

File tree

‎scripts/check-comment-mask-corpus.mjs‎

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@
125125

126126
// dispatch-gates: whole-tree-population -- `collectSources` walks every authored JS/TS file from the repo root, so the corpus is the whole tree; the one literal below names the masker this gate exercises, not the files it reads.
127127

128+
import { spawnSync } from 'node:child_process';
128129
import { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
129130
import { tmpdir } from 'node:os';
130131
import { dirname, extname, join, relative, resolve, sep } from 'node:path';
@@ -355,6 +356,26 @@ export function sweep({ root = REPO_ROOT, files = collectSources(root), parse, s
355356

356357
const ROW_LIMIT = 25;
357358

359+
/**
360+
* The rest of `argv` after `--masker <path>` is consumed -- `argv` ITSELF
361+
* (same reference, not a filtered copy) when `--masker` is absent (#16540).
362+
*
363+
* `argv.indexOf('--masker')` returns `-1` for "absent", and `-1 + 1 === 0`:
364+
* a filter written as `index !== maskerFlag && index !== maskerFlag + 1`
365+
* therefore excluded index 0 even with no `--masker` in sight -- dropping
366+
* `argv[0]`, the very argument the unknown-option check below reads. An
367+
* unknown option in first position was silently removed from the list it
368+
* was about to be checked against, so the script fell through to the full
369+
* sweep instead of refusing with a usage error.
370+
*
371+
* @param {string[]} argv
372+
* @param {number} maskerFlag `argv.indexOf('--masker')`
373+
* @returns {string[]}
374+
*/
375+
export function argvAfterMasker(argv, maskerFlag) {
376+
return maskerFlag === -1 ? argv : argv.filter((arg, index) => index !== maskerFlag && index !== maskerFlag + 1);
377+
}
378+
358379
async function main(argv) {
359380
const maskerFlag = argv.indexOf('--masker');
360381
let maskerPath = null;
@@ -365,7 +386,7 @@ async function main(argv) {
365386
process.exit(EXIT_USAGE);
366387
}
367388
}
368-
const rest = argv.filter((arg, index) => index !== maskerFlag && index !== maskerFlag + 1);
389+
const rest = argvAfterMasker(argv, maskerFlag);
369390
const unknown = rest.filter((arg) => arg.startsWith('--'));
370391
if (unknown.length) {
371392
console.error(`unknown option(s): ${unknown.join(', ')}`);
@@ -480,7 +501,8 @@ async function main(argv) {
480501
// not red. A battery BELOW its floor means cases stopped running; the remedy is
481502
// to find what stopped registering.
482503
const SELF_TEST_BATTERIES = Object.freeze({
483-
'check-comment-mask-corpus self-test': 17,
504+
// #16540 added 9 cases pinning the argv[0]-drop fix (direct + spawned).
505+
'check-comment-mask-corpus self-test': 26,
484506
});
485507

486508
// DELETING an entry silences that battery's floor exactly as effectively as
@@ -642,6 +664,70 @@ async function runSelfTestCases(parse) {
642664
ok(`the corpus walk finds at least ${CORPUS_FLOOR} files in this tree`, collectSources().length >= CORPUS_FLOOR);
643665
ok('...and every path it returns carries a known source extension', collectSources().every((file) => SOURCE_EXTENSIONS.has(extname(file))));
644666

667+
// ── #16540: an unknown FIRST option must not vanish from `rest` ───────────
668+
//
669+
// `indexOf('--masker')` returns `-1` when it is absent, and `-1 + 1 === 0`:
670+
// the un-guarded filter treated that as "drop index 0" regardless of
671+
// whether `--masker` was ever seen. Cases below pin the fix at two levels:
672+
// `argvAfterMasker()` directly (fast, no process), and a REAL spawned
673+
// process (the objectui port's approach) so the exit code is pinned as an
674+
// actual NUMBER coming back from the OS, not a value this file made up.
675+
const noSuchFlagArgv = Object.freeze(['--no-such-flag']);
676+
ok(
677+
'#16540 -- with --masker absent, rest is argv ITSELF (same reference), not a copy missing argv[0]',
678+
argvAfterMasker(noSuchFlagArgv, -1) === noSuchFlagArgv,
679+
);
680+
ok(
681+
'#16540 regression -- --masker in FIRST position still drops exactly its flag and value',
682+
JSON.stringify(argvAfterMasker(['--masker', 'x', 'extra'], 0)) === JSON.stringify(['extra']),
683+
);
684+
ok(
685+
'#16540 -- an unknown option is read from argv[0] when --masker is absent',
686+
argvAfterMasker(noSuchFlagArgv, -1).filter((arg) => arg.startsWith('--')).length === 1,
687+
);
688+
ok(
689+
'#16540 -- an unknown option is read from a MIDDLE position, no --masker',
690+
argvAfterMasker(['ok', '--no-such-flag', 'ok'], -1).filter((arg) => arg.startsWith('--')).length === 1,
691+
);
692+
ok(
693+
'#16540 -- an unknown option is read from the LAST position, after --masker consumed its two slots',
694+
argvAfterMasker(['--masker', 'x', '--no-such-flag'], 0).filter((arg) => arg.startsWith('--')).length === 1,
695+
);
696+
697+
// Real spawned processes -- proves the fix end to end, through the actual
698+
// CLI dispatch, not just the extracted helper. `status` below is `number |
699+
// null`; the assertion is on `typeof`, never just truthiness, so a future
700+
// regression to `null` (the child never really exited) cannot read as a
701+
// number by accident.
702+
const SELF = fileURLToPath(import.meta.url);
703+
const spawnCheck = (args) => spawnSync(process.execPath, [SELF, ...args], { encoding: 'utf8' });
704+
705+
const spawnStarted = Date.now();
706+
const firstPosition = spawnCheck(['--no-such-flag']);
707+
const firstPositionMs = Date.now() - spawnStarted;
708+
ok(
709+
'#16540 spawned -- --no-such-flag at argv[0] exits EXIT_USAGE as a NUMBER (not a full sweep)',
710+
typeof firstPosition.status === 'number' && firstPosition.status === EXIT_USAGE,
711+
);
712+
ok(
713+
'...and the usage error names the unknown flag',
714+
(firstPosition.stderr ?? '').includes('unknown option(s): --no-such-flag'),
715+
);
716+
ok(
717+
// Generous bound -- the point is "milliseconds", not the tree's real sweep
718+
// time (tens of SECONDS, per this file's own header). A regression back to
719+
// the pre-#16540 shape would run the full corpus here and blow past it.
720+
'...and it answers in well under a second -- it never reaches the sweep',
721+
firstPositionMs < 5000,
722+
);
723+
724+
const afterMasker = spawnCheck(['--masker', 'scripts/js-comment-mask.mjs', '--no-such-flag']);
725+
ok(
726+
'#16540 spawned -- an unknown option AFTER --masker also exits EXIT_USAGE as a NUMBER',
727+
typeof afterMasker.status === 'number' && afterMasker.status === EXIT_USAGE
728+
&& (afterMasker.stderr ?? '').includes('unknown option(s): --no-such-flag'),
729+
);
730+
645731
// ── The walk's exclusions, on a REAL tree, in both directions ─────────────
646732
//
647733
// `SKIPPED_DIRECTORIES` is the kind of declaration that reads as obviously

0 commit comments

Comments
 (0)