Skip to content

[finding] check-shard-attestation's invokesScript() reads an echo QUOTING the invocation as an invocation — adjacency without quote-awareness #10889

Description

@os-zhuang

Noticed while implementing #10877 (PR #10884), which needed a narrow "does this step actually
INVOKE the script?" recognizer and evaluated invokesScript() as the candidate to reuse.

What it is

scripts/check-shard-attestation.mjs's invokesScript(runText, flag) added adjacency to
fix #6589's false red (a bare --verify substring anywhere in a run: block). Adjacency is
the right idea, but the tokenizer under it splits on whitespace and then strips the quotes off
each piece:

const tokens = command.split(/\s+/).map((token) => token.replace(/^['"]|['"]$/g, ''));

Shell does not word-split inside quotes. So a quoted string being handed to echo is torn
into separate "tokens", and the script name plus its flag inside that string read exactly like
a program being run with an argument.

Measured, on this tree

Against the real exported invokesScript (worktree at c20069e17f):

true   | genuine invocation          node scripts/check-shard-attestation.mjs --verify --dir x
true   | an ECHO merely quoting it   echo "node scripts/check-shard-attestation.mjs --verify is how you would check"
false  | a ::error:: annotation      echo "::error::check-shard-attestation.mjs --verify exited 1"

The second line is the finding. The third is green only incidentally — the ::error:: prefix
happens to fuse onto the basename token so it matches neither === SCRIPT_BASENAME nor
endsWith('/' + SCRIPT_BASENAME). Write the same annotation with a space, or as
echo "run node scripts/check-shard-attestation.mjs --verify to reproduce", and it counts.

Why it matters, and its direction

invokesScript feeds presence questions — whether an attesting job ends with the
--emit pair, whether an aggregate gate runs --verify. Presence assertions fail toward a
silent green, so the symptom is the gate reporting that a leg attests when it does not.

⚠️ The trigger is the ordinary act of writing a diagnostic about the script, which is the
same #4890 shape the neighbouring ci.yml block already walked into for the sibling flag —
and the reason #10818/#10877 exist at all. This is the same family, one file over.

Not urgent and not currently mis-firing on the checked-in tree: no workflow presently
echoes the invocation with a spelling that matches. It is latent, exactly as #10818 was.

Prior art for the fix, already in the tree

#10884 solved the same problem for check-required-contexts.mjs without touching this file, by
splitting commands with a word-splitter that records which words came out of a quoted
region
and requiring the program word to be unquoted (arguments may be quoted freely —
refusing "--verify" would narrow a presence assertion into a false red). See commandWords()
and invokes() there.

Whether this file should adopt that, or the two should converge on one shared helper, is a
judgment call rather than a mechanical swap — and ⚠️ #10628 already had to undo one
mirrored helper in a neighbouring file, so "extract a shared module" is not automatically the
right answer here.

Filed unassigned, per scope discipline — #10884 deliberately did not touch this file.


Generated by Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions