Skip to content

Commit b9dec72

Browse files
committed
fix(devx): read a quoted blob as an argument, not as a program being run (#10889)
`invokesScript()` in check-shard-attestation added adjacency for #6589 but tokenized with `command.split(/\s+/)` plus a per-token quote strip, so a quoted string handed to `echo` was torn into words and the script name plus its flag INSIDE that string read as a genuine invocation. The reading is now the quote-aware one #10884 built for the sibling gate, moved to the lexer whose output it consumes rather than mirrored: `commandWords` and `invokes` live beside `shellCommands` in check-shard-attestation, and check-required-contexts imports them over the one import edge that already existed between the two files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
1 parent 145ba75 commit b9dec72

2 files changed

Lines changed: 223 additions & 106 deletions

File tree

‎scripts/check-required-contexts.mjs‎

Lines changed: 11 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,13 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync,
181181
import { tmpdir } from 'node:os';
182182
import { dirname, join, resolve } from 'node:path';
183183
import { fileURLToPath } from 'node:url';
184-
import { shellCommands } from './check-shard-attestation.mjs';
184+
// Both layers of the shell lexer, from the one home that owns them. This file
185+
// wrote `invokes` (and the `commandWords` under it) first (#10877), but they
186+
// belong beside `shellCommands`, whose output they consume: the import edge
187+
// between these two files runs THIS way only, so a helper kept here could never
188+
// be reached from check-shard-attestation without a cycle — and that file needed
189+
// the same narrow reading for the same defect one flag over (#10889).
190+
import { invokes, shellCommands } from './check-shard-attestation.mjs';
185191
import { isEntrypoint } from './invoked-as.mjs';
186192

187193
/**
@@ -696,102 +702,6 @@ function runCommands(node) {
696702
return commands;
697703
}
698704

699-
/**
700-
* One shell command, split into words, each marked as QUOTED or not.
701-
*
702-
* ⚠️ Not a third comment stripper — it handles no `#` at all. It runs on
703-
* `shellCommands()` OUTPUT, where comments are already gone, and recovers the
704-
* one thing that function does not expose: where the words are, and which of
705-
* them came out of a quoted region. Shell word-splitting does not happen inside
706-
* quotes, so `echo "pnpm check:required-contexts is below"` is TWO words — the
707-
* program `echo`, and one quoted blob — and no amount of splitting on
708-
* whitespace will make the blob look like a program being run.
709-
*
710-
* That distinction is the whole narrow recognizer. `invokesScript()` in
711-
* check-shard-attestation splits on `/\s+/` and strips the quotes off each
712-
* piece, which is right for the question IT asks and cannot answer this one:
713-
* it would read the `echo` above as running `pnpm`.
714-
*
715-
* @param {string} command one command from `shellCommands()`
716-
* @returns {{ word: string, quoted: boolean }[]}
717-
*/
718-
function commandWords(command) {
719-
const words = [];
720-
let word = '';
721-
let quoted = false;
722-
let started = false;
723-
let quote = '';
724-
const flush = () => {
725-
if (started) words.push({ word, quoted });
726-
word = '';
727-
quoted = false;
728-
started = false;
729-
};
730-
for (let i = 0; i < command.length; i += 1) {
731-
const ch = command[i];
732-
if (quote) {
733-
// Inside double quotes a backslash escapes the next character; inside
734-
// single quotes it is literal. Either way the result stays in this word.
735-
if (ch === '\\' && quote === '"' && i + 1 < command.length) {
736-
word += command[i + 1];
737-
i += 1;
738-
continue;
739-
}
740-
if (ch === quote) {
741-
quote = '';
742-
continue;
743-
}
744-
word += ch;
745-
continue;
746-
}
747-
if (ch === '\\' && i + 1 < command.length) {
748-
word += command[i + 1];
749-
i += 1;
750-
started = true;
751-
continue;
752-
}
753-
if (ch === "'" || ch === '"') {
754-
quote = ch;
755-
quoted = true;
756-
started = true;
757-
continue;
758-
}
759-
if (ch === ' ' || ch === '\t') {
760-
flush();
761-
continue;
762-
}
763-
word += ch;
764-
started = true;
765-
}
766-
flush();
767-
return words;
768-
}
769-
770-
/**
771-
* Does this command RUN `program`, with `arg` among the arguments it passes?
772-
*
773-
* Adjacency, as check-shard-attestation's `invokesScript()` reads it (#6589):
774-
* the argument must belong to a command that runs the program, not merely
775-
* appear near it. Plus one condition that function does not impose — the
776-
* PROGRAM word must be UNQUOTED. A quoted word is a string being passed to
777-
* something else, and the something else is usually `echo`.
778-
*
779-
* Arguments may be quoted freely: `--verify-required-set` and
780-
* `"--verify-required-set"` are the same argument to the same invocation, and
781-
* refusing the quoted spelling would narrow a PRESENCE assertion into a false
782-
* red — the failure mode that gets a pin loosened rather than fixed.
783-
*
784-
* @param {string} command one command from `shellCommands()`
785-
* @param {string} program an executable or script basename, e.g. `pnpm`
786-
* @param {string} arg the argument that makes it THE invocation
787-
*/
788-
function invokes(command, program, arg) {
789-
const words = commandWords(command);
790-
const at = words.findIndex(({ word, quoted }) => !quoted && (word === program || word.endsWith(`/${program}`)));
791-
if (at === -1) return false;
792-
return words.slice(at + 1).some(({ word }) => word === arg || word.startsWith(`${arg}=`));
793-
}
794-
795705
/**
796706
* Does this workflow WIRE UP the live required-set read?
797707
*
@@ -2852,8 +2762,10 @@ async function selfTest() {
28522762
);
28532763
// (l) the same thing one grammar down: a quoted string being handed to
28542764
// `echo`. Splitting the command on whitespace and stripping quotes off each
2855-
// piece — which is what the sibling's `invokesScript()` does — would read
2856-
// this as running `pnpm`, so `commandWords` tracks quoting instead.
2765+
// piece — which is what the sibling's `invokesScript()` did until #10889 —
2766+
// would read this as running `pnpm`, so `invokes` tracks quoting instead.
2767+
// That sibling now shares this exact reading rather than mirroring it: the
2768+
// helper moved to the lexer it consumes and both files import it (#10889).
28572769
assert(
28582770
!pinWiredIn(
28592771
lintJobFixture(' - name: talk about it', ' run: |', ` echo "run: pnpm ${PIN_SCRIPT} is wired below"`),

‎scripts/check-shard-attestation.mjs‎

Lines changed: 212 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,117 @@ export function shellCommands(runText) {
398398
return commands;
399399
}
400400

401+
/**
402+
* One shell command, split into words, each marked as QUOTED or not.
403+
*
404+
* ⚠️ Not a second comment stripper — it handles no `#` at all. It runs on
405+
* `shellCommands()` OUTPUT, where comments are already gone, and recovers the
406+
* one thing that function does not expose: where the words are, and which of
407+
* them came out of a quoted region.
408+
*
409+
* Shell does not word-split inside quotes, so
410+
* `echo "node scripts/check-shard-attestation.mjs --verify is how you check"`
411+
* is TWO words — the program `echo`, and one quoted blob — and no amount of
412+
* splitting on whitespace will make the blob look like a program being run.
413+
* The tokenizer this replaced did exactly that (`command.split(/\s+/)` then
414+
* `token.replace(/^['"]|['"]$/g, '')`), which tore the blob into pieces and
415+
* read the script name plus its flag inside a STRING as an invocation (#10889).
416+
*
417+
* ## Why this lives HERE and not beside its first caller
418+
*
419+
* It arrived in `check-required-contexts.mjs` (#10877/#10884), which needed the
420+
* same narrow reading first. It is defined here because the two files already
421+
* have exactly ONE import edge and it points `check-required-contexts` →
422+
* `check-shard-attestation` (that file imports `shellCommands` from this one).
423+
* A helper kept in the importer cannot be reached from the importee without a
424+
* cycle, so "adopt the sibling's helper" mechanically means moving it DOWN to
425+
* the lexer it already consumes — commands out of a `run:` block, then words
426+
* out of a command, one layer above the other in one home. The alternative,
427+
* re-typing it here, is the hand-mirror #10628 had to undo; the obstacle that
428+
* justified the mirror there (a module that ran its gate on import) does not
429+
* exist here, because this file has been behind `isEntrypoint()` all along.
430+
*
431+
* @param {string} command one command from `shellCommands()`
432+
* @returns {{ word: string, quoted: boolean }[]}
433+
*/
434+
export function commandWords(command) {
435+
const words = [];
436+
let word = '';
437+
let quoted = false;
438+
let started = false;
439+
let quote = '';
440+
const flush = () => {
441+
if (started) words.push({ word, quoted });
442+
word = '';
443+
quoted = false;
444+
started = false;
445+
};
446+
for (let i = 0; i < command.length; i += 1) {
447+
const ch = command[i];
448+
if (quote) {
449+
// Inside double quotes a backslash escapes the next character; inside
450+
// single quotes it is literal. Either way the result stays in this word.
451+
if (ch === '\\' && quote === '"' && i + 1 < command.length) {
452+
word += command[i + 1];
453+
i += 1;
454+
continue;
455+
}
456+
if (ch === quote) {
457+
quote = '';
458+
continue;
459+
}
460+
word += ch;
461+
continue;
462+
}
463+
if (ch === '\\' && i + 1 < command.length) {
464+
word += command[i + 1];
465+
i += 1;
466+
started = true;
467+
continue;
468+
}
469+
if (ch === "'" || ch === '"') {
470+
quote = ch;
471+
quoted = true;
472+
started = true;
473+
continue;
474+
}
475+
if (ch === ' ' || ch === '\t') {
476+
flush();
477+
continue;
478+
}
479+
word += ch;
480+
started = true;
481+
}
482+
flush();
483+
return words;
484+
}
485+
486+
/**
487+
* Does this command RUN `program`, with `arg` among the arguments it passes?
488+
*
489+
* Two conditions, and the ASYMMETRY between them is the whole recognizer:
490+
*
491+
* - the PROGRAM word must be UNQUOTED. A quoted word is a string being
492+
* handed to something else, and the something else is usually `echo`;
493+
* - ARGUMENTS may be quoted freely. `--verify` and `"--verify"` are the same
494+
* argument to the same invocation, and refusing the quoted spelling would
495+
* narrow a PRESENCE assertion into a false red — the failure mode that
496+
* gets a pin loosened rather than fixed.
497+
*
498+
* Plus adjacency (#6589), which the co-occurrence spelling lacked: the argument
499+
* must belong to a command that runs the program, not merely appear near it.
500+
*
501+
* @param {string} command one command from `shellCommands()`
502+
* @param {string} program an executable or script basename, e.g. `pnpm`
503+
* @param {string} arg the argument that makes it THE invocation
504+
*/
505+
export function invokes(command, program, arg) {
506+
const words = commandWords(command);
507+
const at = words.findIndex(({ word, quoted }) => !quoted && (word === program || word.endsWith(`/${program}`)));
508+
if (at === -1) return false;
509+
return words.slice(at + 1).some(({ word }) => word === arg || word.startsWith(`${arg}=`));
510+
}
511+
401512
/**
402513
* Does this `run:` text INVOKE this script with `flag`?
403514
*
@@ -406,16 +517,35 @@ export function shellCommands(runText) {
406517
* flag spelled in another — or in a comment, or as an argument of some other
407518
* program entirely — no longer add up to an invocation.
408519
*
520+
* ## …and the command that "runs" it must not be an `echo` (#10889)
521+
*
522+
* Adjacency alone was still satisfied by PROSE, because the tokenizer under it
523+
* split on whitespace and stripped the quotes off each piece. Measured on the
524+
* tree that had it:
525+
*
526+
* echo "node scripts/check-shard-attestation.mjs --verify is how you check"
527+
*
528+
* read as a genuine `--verify` invocation. The direction is what makes it worth
529+
* fixing: `invokesScript` feeds PRESENCE questions — does an attesting job end
530+
* with the `--emit` pair, does an aggregate gate run `--verify` — and a
531+
* presence assertion fails toward a silent GREEN, so the symptom is this guard
532+
* reporting that a leg attests when it does not.
533+
*
534+
* ⚠️ One spelling of that was green already, and only by ACCIDENT:
535+
* `echo "::error::check-shard-attestation.mjs --verify exited 1"` missed
536+
* because `::error::` fuses onto the basename token, matching neither
537+
* `=== SCRIPT_BASENAME` nor `endsWith('/' + SCRIPT_BASENAME)`. Write the same
538+
* annotation with a SPACE after `::error::` and it counted. Both spellings are
539+
* pinned in the self-test, because a fix that only handles the obvious one
540+
* leaves the fragile one behind — and writing a diagnostic ABOUT this script is
541+
* the #4890 shape that keeps re-arming this family (#6589's own second-order
542+
* trap was a comment; this one is an `echo`).
543+
*
409544
* @param {unknown} runText a step's `run:` string
410545
* @param {string} flag e.g. `--verify` or `--emit`
411546
*/
412547
export function invokesScript(runText, flag) {
413-
return shellCommands(runText).some((command) => {
414-
const tokens = command.split(/\s+/).map((token) => token.replace(/^['"]|['"]$/g, ''));
415-
const at = tokens.findIndex((token) => token === SCRIPT_BASENAME || token.endsWith(`/${SCRIPT_BASENAME}`));
416-
if (at === -1) return false;
417-
return tokens.slice(at + 1).some((token) => token === flag || token.startsWith(`${flag}=`));
418-
});
548+
return shellCommands(runText).some((command) => invokes(command, SCRIPT_BASENAME, flag));
419549
}
420550

421551
/**
@@ -792,6 +922,81 @@ async function selfTest() {
792922
assert(invokesScript('node scripts/check-shard-attestation.mjs --gate "a # b" --verify', '--verify'), 'a quoted `#` is an argument, not a comment — the command must not be truncated');
793923
assert(invokesScript("node scripts/check-shard-attestation.mjs --gate 'a || b' --verify", '--verify'), 'a quoted separator is an argument, not a command boundary');
794924
assert(!invokesScript(undefined, '--verify') && !invokesScript(null, '--verify'), 'a step with no run: is not an invocation');
925+
926+
// ── …and the PROGRAM word must be unquoted (#10889) ───────────────────────
927+
//
928+
// Adjacency (above) closed co-occurrence but not PROSE. The tokenizer under
929+
// it split on whitespace and stripped the quotes off each piece, so a quoted
930+
// string handed to `echo` was torn into "tokens" and the script name plus its
931+
// flag INSIDE that string read as a program being run with an argument.
932+
//
933+
// ⭐ Direction, because it decides which limb is the dangerous one to lose:
934+
// these feed PRESENCE questions (does this job end with the `--emit` pair,
935+
// does this gate run `--verify`), and a presence assertion fails toward a
936+
// silent GREEN — the guard reporting that a leg attests when it does not.
937+
// So every "prose" case below is paired with the same text made REAL, and
938+
// #6589's cases above stay exactly as they were: a recognizer tightened until
939+
// nothing satisfies it is a worse defect than the prose it excluded, because
940+
// what a permanently red pin gets is loosened.
941+
assert(
942+
!invokesScript('echo "node scripts/check-shard-attestation.mjs --verify is how you would check"', '--verify'),
943+
'#10889: an `echo` QUOTING the invocation is prose — the quoted blob is ONE argument, not a program being run',
944+
);
945+
assert(
946+
!invokesScript("echo 'node scripts/check-shard-attestation.mjs --verify is how you would check'", '--verify'),
947+
'#10889: …and single quotes word-split no more than double quotes do',
948+
);
949+
assert(
950+
!invokesScript('echo "run node scripts/check-shard-attestation.mjs --verify to reproduce"', '--verify'),
951+
'#10889: prose WRAPPED AROUND the invocation is still prose',
952+
);
953+
// ⚠️ The two `::error::` spellings are pinned together on purpose. The FUSED
954+
// one was already false before this fix — but only incidentally, because
955+
// `::error::` runs into the basename and the joined token matches neither
956+
// `=== SCRIPT_BASENAME` nor `endsWith('/' + SCRIPT_BASENAME)`. The SPACED one
957+
// is the same annotation, one space different, and it COUNTED. Keeping only
958+
// the fused case would let the accident stand in for the fix.
959+
assert(
960+
!invokesScript('echo "::error::check-shard-attestation.mjs --verify exited 1"', '--verify'),
961+
'#10889: a `::error::` annotation about this script is not an invocation of it',
962+
);
963+
assert(
964+
!invokesScript('echo "::error:: check-shard-attestation.mjs --verify exited 1"', '--verify'),
965+
'#10889: …and the same annotation written with a SPACE, which is the spelling that used to count',
966+
);
967+
// The `--emit` half of the same question: an attesting shard job is judged by
968+
// whether its LAST step invokes `--emit`, so prose there is the same silent
969+
// green one flag over.
970+
assert(
971+
!invokesScript('echo "the attestation step runs check-shard-attestation.mjs --emit --job test"', '--emit'),
972+
'#10889: the same defect on the `--emit` presence question, which decides whether a shard ATTESTS',
973+
);
974+
// ⭐ The asymmetry, stated as a fact and not left to the docblock: only the
975+
// PROGRAM word has to be unquoted. Refusing a quoted ARGUMENT would turn a
976+
// presence assertion into a false red.
977+
assert(
978+
invokesScript('node scripts/check-shard-attestation.mjs "--verify" --gate x', '--verify'),
979+
'#10889: quoting the ARGUMENT does not un-invoke the script — only the PROGRAM word must be unquoted',
980+
);
981+
assert(
982+
invokesScript("node scripts/check-shard-attestation.mjs '--verify' --gate \"a b\"", '--verify'),
983+
'#10889: …in either quote style, alongside other quoted arguments',
984+
);
985+
assert(
986+
!invokesScript('echo "scripts/check-shard-attestation.mjs" --verify', '--verify'),
987+
'#10889: a QUOTED program word is a string being passed to something else — here, to `echo`',
988+
);
989+
// The word-splitter itself, at the seam the readings above rest on.
990+
assert(
991+
commandWords('echo "a b c" d').map(({ word, quoted }) => `${quoted ? 'Q' : 'U'}:${word}`).join('|') === 'U:echo|Q:a b c|U:d',
992+
'commandWords: a quoted region is ONE word and is marked as quoted; unquoted words split on whitespace',
993+
);
994+
assert(
995+
invokes('pnpm check:shard-attestation', 'pnpm', 'check:shard-attestation') &&
996+
!invokes('echo "pnpm check:shard-attestation is wired below"', 'pnpm', 'check:shard-attestation'),
997+
'invokes: the same reading generalised over any program — this is the seam check-required-contexts imports (#10877/#10889)',
998+
);
999+
7951000
assert(
7961001
shellCommands('a \\\n b\n# c\nd; e | f')
7971002
.map((command) => command.split(/\s+/).join(' '))
@@ -945,7 +1150,7 @@ async function selfTest() {
9451150
process.exit(1);
9461151
}
9471152
console.log(
948-
`✓ check-shard-attestation --self-test: ${checked} assertions (dominance experiment + both #6082 counter-examples + the #4928 guard + the #6589 classifier pins).`,
1153+
`✓ check-shard-attestation --self-test: ${checked} assertions (dominance experiment + both #6082 counter-examples + the #4928 guard + the #6589 classifier pins + the #10889 quoting pins).`,
9491154
);
9501155
}
9511156

0 commit comments

Comments
 (0)