Area: ci — fail-open gating bug · found while running make ci on an unrelated SDK branch
scripts/classify-paths.sh can report code=false for a change set that is code, which makes CI and the pre-push hook skip the Go and SDK test suites.
Cause
Both classification branches pipe into grep -q under set -euo pipefail (line 42, and the same shape at line 49):
if printf '%s\n' "$files" | grep -qvE '^(docs/|.*\.md$|…)'; then
echo "code=true"
else
echo "code=false"
fi
grep -q exits as soon as its condition is decided. printf is then writing to a closed pipe, takes SIGPIPE (141), and pipefail makes that the pipeline's exit status — so the if takes the else branch even though grep succeeded.
It only bites when grep can exit early, i.e. when the first path already decides the answer. That's why dep-bump-go (go.mod go.sum — first line is non-matching, so grep -qv exits after line 1) is the single failing case, while mixed-docs-go passes: its first line docs/x.md matches, so grep reads on and printf finishes first.
Reproduction
Deterministic once the input is big enough that printf can't win the race:
$ bash -c 'set -euo pipefail; files=$(printf "go.mod\n%.0s" {1..200000});
if printf "%s\n" "$files" | grep -qvE "^(docs/|.*\.md$)"; then echo code=true; else echo "code=false <-- WRONG"; fi'
code=false <-- WRONGAt the 2-line size the unit test uses it is load-dependent: scripts/classify-paths.test.sh passes standalone (5/5 for me) and passed in a pre-commit make verify, then failed minutes later on the same tree under make ci's 14-way parallel verify:
FAIL dep-bump-go want code=true docs=false, got code=false docs=false
Why it matters
This is fail-open, and it is worse in CI than locally. scripts/ci/classify-changes.sh pipes the GitHub API file list through this script, and real PRs carry many more paths than the test's two — more input means printf is more likely to still be writing when grep exits, not less. A misclassification silently drops the Go + SDK suites for that run, and the aggregator still reports green. The local pre-push hook consults the same classifier to decide between the full make ci marker and the lighter make verify marker.
Line 49 (docs=) has the identical shape and the same latent race; it just needs a first-path match to trigger.
Suggested fix
Drop the pipe so there is no SIGPIPE to propagate — a here-string keeps the script dependency-free and pure:
if grep -qvE '^(docs/|…)' <<<"$files"; then
Worth a test that pins the failure mode rather than only the happy path, since the current case only catches it probabilistically — e.g. assert against an input large enough to force the race deterministically.
Introduced in #312. Not urgent-looking day to day precisely because it is intermittent, which is the argument for fixing it rather than re-running.
Area: ci — fail-open gating bug · found while running
make cion an unrelated SDK branchscripts/classify-paths.shcan reportcode=falsefor a change set that is code, which makes CI and the pre-push hook skip the Go and SDK test suites.Cause
Both classification branches pipe into
grep -qunderset -euo pipefail(line 42, and the same shape at line 49):grep -qexits as soon as its condition is decided.printfis then writing to a closed pipe, takesSIGPIPE(141), andpipefailmakes that the pipeline's exit status — so theiftakes the else branch even though grep succeeded.It only bites when grep can exit early, i.e. when the first path already decides the answer. That's why
dep-bump-go(go.mod go.sum— first line is non-matching, sogrep -qvexits after line 1) is the single failing case, whilemixed-docs-gopasses: its first linedocs/x.mdmatches, so grep reads on and printf finishes first.Reproduction
Deterministic once the input is big enough that
printfcan't win the race:At the 2-line size the unit test uses it is load-dependent:
scripts/classify-paths.test.shpasses standalone (5/5 for me) and passed in a pre-commitmake verify, then failed minutes later on the same tree undermake ci's 14-way parallelverify:Why it matters
This is fail-open, and it is worse in CI than locally.
scripts/ci/classify-changes.shpipes the GitHub API file list through this script, and real PRs carry many more paths than the test's two — more input meansprintfis more likely to still be writing when grep exits, not less. A misclassification silently drops the Go + SDK suites for that run, and the aggregator still reports green. The local pre-push hook consults the same classifier to decide between the fullmake cimarker and the lightermake verifymarker.Line 49 (
docs=) has the identical shape and the same latent race; it just needs a first-path match to trigger.Suggested fix
Drop the pipe so there is no SIGPIPE to propagate — a here-string keeps the script dependency-free and pure:
Worth a test that pins the failure mode rather than only the happy path, since the current case only catches it probabilistically — e.g. assert against an input large enough to force the race deterministically.
Introduced in #312. Not urgent-looking day to day precisely because it is intermittent, which is the argument for fixing it rather than re-running.