🤖 Generated by the Agentic Engineer
Evidence
scripts/guard-tracked-exec-bit.sh decides whether a .sh path sits in a command position by extracting the text in front of it and walking that prefix as whitespace-separated words. Four review rounds on #3667 have each found one more spelling that defeats it:
| round |
found by |
spelling |
mechanism |
| 1 |
Codex |
echo ready && ./scripts/x.sh |
classifier broke on echo before reaching && |
| 1b |
testing round 1's fix |
./scripts/a.sh && bash ./scripts/b.sh |
greedy grep -o swallowed a.sh into one occurrence |
| 2 |
CodeRabbit |
echo ready; scripts/x.sh |
prefix word is ready;, separator arm matches a separate ; only |
| 3 |
CodeRabbit |
echo ready| scripts/x.sh, echo ready&& scripts/x.sh, echo ready;scripts/x.sh |
same glue on |/&&; the third has no whitespace before the path, so extraction produces nothing at all |
Every one is a fail-open: a directly-execed script tracked 100644 goes unchecked whenever another invocation satisfies the anti-vacuity check. Each individual fix was correct and each was verified by ablation — the problem is that the enumeration does not converge.
Why the obvious fix is not available
The natural response — normalise shell separators across the scan before extraction — is unsafe here, and measurably so. The scanned corpus is *.sh, *.yaml, *.yml under scripts/ and .github/, and 140 files contain a literal |. Among them is the guard's own
readonly SCRIPT_PATH_RE='(\.github|scripts)/[A-Za-z0-9_./-]+\.sh'
Splitting on | rewrites that to (\.github | scripts)/…, which both breaks real matches and manufactures new apparent command positions. The same hazard applies to any regex alternation, YAML block scalar, or quoted string in the corpus, and to redirections such as 2>&1 if & is normalised. This is why the current &&/|| split is restricted to whitespace-delimited occurrences — the restriction is deliberate, and it is exactly what rounds 2 and 3 exploit.
The actual defect
The design is a blacklist of non-command prefixes: extract permissively, then discard an occurrence when a prefix word is recognised as something that does not exec. Anything unrecognised is treated as "not a command position", so every unlisted spelling fails open.
Repeated rounds differing only in syntax are a finding about the check's direction, not about the individual spellings.
Expected behaviour
Invert it, so the unlisted case fails closed:
- Decide command position from a whitelist of shapes the guard can positively prove are not direct execs (an interpreter, a recognised argument position), and treat anything it cannot classify as a direct invocation requiring the bit — or as an explicit "cannot classify" error naming the line.
- Either way the residual becomes a false positive that surfaces loudly at the offending line, rather than a silent miss. That is the correct direction for a guard whose whole purpose is to stop a silent CI break, and it matches the paved-road rule that a check should fail with the fix.
The cost to weigh: a false positive fails every PR and merge-group run, so the inversion needs the same conservation check the incremental fixes used — the committed tree currently reports 22 directly-invoked scripts and must still report exactly those, with no additions, before the inversion can land.
Tokenising the corpus the way a shell would (rather than by textual separator splitting) is the other credible route, and is the only one that satisfies "preserve quoted text" properly. It is a larger change than a regex.
Acceptance criteria
- Fixtures for every spelling in the table above, each failing against the pre-inversion guard.
- A fixture for a quoted separator (
echo "a; b" && scripts/x.sh) and for a redirection (cmd 2>&1 scripts/x.sh), proving the normalisation does not manufacture a command position.
- The committed tree still reports exactly the same 22 directly-invoked scripts.
- An unclassifiable line produces a named error, not a silent pass.
Rough size
Medium. This is a redesign of the prefix decision, not a regex change, and it should land on its own with its fixtures rather than on top of #3667.
Evidence
scripts/guard-tracked-exec-bit.shdecides whether a.shpath sits in a command position by extracting the text in front of it and walking that prefix as whitespace-separated words. Four review rounds on #3667 have each found one more spelling that defeats it:echo ready && ./scripts/x.shechobefore reaching&&./scripts/a.sh && bash ./scripts/b.shgrep -oswalloweda.shinto one occurrenceecho ready; scripts/x.shready;, separator arm matches a separate;onlyecho ready| scripts/x.sh,echo ready&& scripts/x.sh,echo ready;scripts/x.sh|/&&; the third has no whitespace before the path, so extraction produces nothing at allEvery one is a fail-open: a directly-execed script tracked
100644goes unchecked whenever another invocation satisfies the anti-vacuity check. Each individual fix was correct and each was verified by ablation — the problem is that the enumeration does not converge.Why the obvious fix is not available
The natural response — normalise shell separators across the scan before extraction — is unsafe here, and measurably so. The scanned corpus is
*.sh,*.yaml,*.ymlunderscripts/and.github/, and 140 files contain a literal|. Among them is the guard's ownSplitting on
|rewrites that to(\.github | scripts)/…, which both breaks real matches and manufactures new apparent command positions. The same hazard applies to any regex alternation, YAML block scalar, or quoted string in the corpus, and to redirections such as2>&1if&is normalised. This is why the current&&/||split is restricted to whitespace-delimited occurrences — the restriction is deliberate, and it is exactly what rounds 2 and 3 exploit.The actual defect
The design is a blacklist of non-command prefixes: extract permissively, then discard an occurrence when a prefix word is recognised as something that does not exec. Anything unrecognised is treated as "not a command position", so every unlisted spelling fails open.
Repeated rounds differing only in syntax are a finding about the check's direction, not about the individual spellings.
Expected behaviour
Invert it, so the unlisted case fails closed:
The cost to weigh: a false positive fails every PR and merge-group run, so the inversion needs the same conservation check the incremental fixes used — the committed tree currently reports 22 directly-invoked scripts and must still report exactly those, with no additions, before the inversion can land.
Tokenising the corpus the way a shell would (rather than by textual separator splitting) is the other credible route, and is the only one that satisfies "preserve quoted text" properly. It is a larger change than a regex.
Acceptance criteria
echo "a; b" && scripts/x.sh) and for a redirection (cmd 2>&1 scripts/x.sh), proving the normalisation does not manufacture a command position.Rough size
Medium. This is a redesign of the prefix decision, not a regex change, and it should land on its own with its fixtures rather than on top of #3667.