Why Two suites failed intermittently — tests/test-vale-wrap.sh case 21 and tests/test-check-release-needed.sh cases 4 and 15 — on correct output, and never when run alone. The cause is the `echo "$OUT" | grep -q P` idiom under `set -o pipefail`: grep -q exits as soon as it has an answer, bash's echo can hand a multi-line value to the pipe one line at a time, and a write after the reader is gone kills echo with SIGPIPE. pipefail then reports the writer's death, so output that DID match reads as "no match". Every observed failure had lines after its match; case 15's match is on line 1 of 6, the widest window in that file. Forced with a pause before the writer's last line, the pipe form failed 50 of 50 runs; a here-string, a match on the last line, and the same pipe without pipefail each passed 50 of 50. Unforced the rate is about 1 per 670 suite runs, which is why it read as a flaky gate rather than a bug. The failures at review time are consistent with this, but were not proven to be it: the suite was running while agents edited live config files in place, and a brief change to .vale.ini or .pre-commit-hooks.yaml would produce the same two failures. The race is real and fixed either way. Implementation Notes `grep -q P <<< "$VAR"` has no separate writer process, so there is nothing to race. It is not a retry or a sleep. 121 sites converted across 9 files, three of them scripts rather than tests: new-agent.sh, new-skill.sh and check-executables-allow-sync.sh. None ships via .pre-commit-hooks.yaml, so no external consumer pins them, and all three are single-pipeline checks whose verdict cannot change. Left alone deliberately: 14 sites whose writer is a command, not a shell builtin — they either absorb the writer's status with `|| true` or are python3 and awk, which write once at exit — and one file with no pipefail. `printf '%s'` sites differ from a here-string only by a trailing newline, which no -q verdict on a non-empty pattern depends on. tests/test-no-pipefail-early-exit-grep.sh is a static guard against new occurrences, discovered automatically by run-tests.sh. It only scans files that set pipefail, joins continuation lines, skips comments, and flags only echo/printf writers. Its first case proves the scanner can fail before its second trusts a clean verdict on the tree. A guard covers exactly the spellings its regex models, so the miss surface was measured rather than assumed. Four were found and closed: pipefail declared as `set -o errexit -o pipefail` (where the old pattern required pipefail to follow the FIRST -o, and a file-level miss skips every site in that file); a writer separated from grep by an intermediate stage; a pipeline wrapped on a trailing `|` rather than a backslash; and readers spelled egrep, fgrep, /bin/grep, `command grep` or with an env-var prefix. Segment characters exclude a bare `&` so `echo ok && other | grep -q x`, whose writer is `other`, does not false-fire. Widening surfaced 5 live sites invisible to the original scanner, all in tests/test-apm-current-hook.sh, all `echo "$out" | json_field ... | grep -q`; they are safe today only because json_field is python3, which reads to EOF and writes once. Fixtures go 4 to 12 vulnerable spellings plus near-miss negatives. Two `grep ... | head -1` sites (test-vale-wrap.sh) are the same race with a different early-exiting reader, and are fixed by absorbing the writer. The scanner deliberately does not model `head`, `sed -n 1p` or a bare `read`: most legitimate uses in this tree are already absorbed with `|| true` and the scanner cannot see absorption from pipeline text, so a high false-positive rate would be how this guard gets weakened. Heredoc bodies are scanned as code; none in the tree trips it today. Impact The bug predates the factory-audit merge: every converted site in check-release-needed and case 21 dates to4d018afandaa8cc22(2026-08-09). Test suites go 19 to 20. `run-tests.sh --strict` passes 20/20 with 0 skipped, four consecutive runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YR2CjVumUbEGWcMikcoXBD
222 lines
9.4 KiB
Bash
Executable File
222 lines
9.4 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
|
|
# Static guard: no `echo|printf ... | grep -q` pipeline in a script that runs
|
|
# under `set -o pipefail`.
|
|
#
|
|
# The defect. grep -q (and -m, -l, -L, --quiet, --silent) exits as soon as it
|
|
# has an answer. If the writer on the left of the pipe still has output to
|
|
# write, that next write hits a closed pipe and the writer dies of SIGPIPE.
|
|
# Under pipefail the pipeline then reports the WRITER's failure, so output that
|
|
# matched reads as "no match". Whether it happens depends on scheduling: bash's
|
|
# echo can hand a multi-line value to the pipe a line at a time, and a reader
|
|
# that matched on an early line exits before the later lines arrive. Forced with
|
|
# a pause before the last line, the pipe form failed 50 of 50 runs; each of a
|
|
# here-string, a match on the last line, and the same pipe without pipefail
|
|
# passed 50 of 50. Unforced it surfaced about once per 670 suite runs, which is
|
|
# why it read as a flaky gate rather than as a bug.
|
|
#
|
|
# The fix is `grep -q PATTERN <<< "$VAR"`: a here-string has no separate writer
|
|
# process, so there is nothing to race. `printf '%s\n' "$X" | grep` and
|
|
# `<<< "$X"` feed grep the same bytes; `printf '%s'` and `echo` differ only by a
|
|
# trailing newline, which no -q verdict on a non-empty pattern depends on.
|
|
#
|
|
# Scope. Only echo/printf writers are flagged: they are the shell builtins that
|
|
# can split a write, and they are always replaceable by a here-string. A
|
|
# command writer (`run_wrap ... | grep -q`) is not flagged; those sites either
|
|
# absorb the writer's status (`|| true`) or write once at exit. Files without
|
|
# pipefail are not flagged, because without it the pipeline's status is grep's.
|
|
# Comment-only lines are skipped so the pattern can be named in prose.
|
|
#
|
|
# Remit. This scanner models early-exiting GREP readers only — grep, egrep and
|
|
# fgrep, however they are spelled (a path prefix, a `command` prefix, env-var
|
|
# assignments), anywhere in the pipeline. They are not the only readers that
|
|
# exit early: `head`, `sed -n 1p` and a bare `read` do too, and an echo/printf
|
|
# writer feeding any of them is the same race. Those sites are guarded by
|
|
# convention instead — absorb the writer's status with `|| true` (or take the
|
|
# verdict from a here-string) — and deliberately not by this test, because most
|
|
# legitimate uses of them in this tree are already absorbed and the scanner
|
|
# cannot see the absorption from the pipeline text alone. Flagging them would be
|
|
# noise; the grep readers are flagged because a here-string is always available.
|
|
#
|
|
# Known limitation: heredoc bodies are scanned as code, so a `cat <<'EOF'` body
|
|
# containing a vulnerable-looking line reads as a real site. There are none in
|
|
# the tree today.
|
|
#
|
|
# Case 1 proves the scanner can fail before case 2 trusts its clean verdict on
|
|
# the live tree.
|
|
|
|
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
|
SELF="$REPO_ROOT/tests/$(basename "${BASH_SOURCE[0]}")"
|
|
PASS=0
|
|
FAIL=0
|
|
|
|
pass() { echo " PASS: $1"; PASS=$((PASS + 1)); }
|
|
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
|
|
|
|
RUN_TMP="$(mktemp -d)"
|
|
trap 'rm -rf "$RUN_TMP"' EXIT
|
|
|
|
# A pipeline-safe run of characters: anything but `|`, `;` or `&`, except that a
|
|
# `&` immediately followed by a digit is kept so `2>&1` does not end a segment.
|
|
# Excluding a bare `&` is what stops `echo ok && other | grep -q x` — a pipeline
|
|
# whose writer is `other`, not the echo — from being read as one site.
|
|
SAFE='([^|;&]|&[[:digit:]])'
|
|
|
|
# Writer, then any number of intermediate stages, then an early-exiting grep.
|
|
# The reader may be spelled `grep`, `egrep` or `fgrep`, behind a path prefix
|
|
# (`/bin/grep`), a `command` prefix, or env-var assignments (`LC_ALL=C grep`).
|
|
SITE_RE="(^|[^[:alnum:]_])(echo|printf)[[:space:]]${SAFE}*[|][[:space:]]*"
|
|
SITE_RE+="([^|;&[:space:]]${SAFE}*[|][[:space:]]*)*"
|
|
SITE_RE+="((command|[A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*)[[:space:]]+)*"
|
|
SITE_RE+="([^[:space:];&|]*/)?(grep|egrep|fgrep)[[:space:]]"
|
|
SITE_RE+="(${SAFE}*[[:space:]])?(-[[:alpha:]]*[qmlL][[:alnum:]]*|--quiet|--silent|--max-count|--files-with)"
|
|
|
|
# `pipefail` need not follow the first `-o`: `set -o errexit -o pipefail` and
|
|
# `set -o posix -o pipefail` are the same setting. A file-level miss here skips
|
|
# every site in the file, so this is deliberately loose — the cost of scanning a
|
|
# file that does not set pipefail is nil, the cost of skipping one is total.
|
|
PIPEFAIL_RE='(^|[^[:alnum:]_-])set[[:space:]]+-[^;#]*pipefail'
|
|
|
|
# Prints `path:line: text` for every vulnerable pipeline in the given files.
|
|
# Continued lines are joined first — both a trailing backslash and a trailing
|
|
# `|`, which is equally legal as a line continuation in a pipeline — so a reader
|
|
# on the next physical line is still seen; the reported line is where the
|
|
# command starts.
|
|
scan() {
|
|
local f
|
|
for f in "$@"; do
|
|
grep -Eq "$PIPEFAIL_RE" "$f" || continue
|
|
awk -v file="$f" -v re="$SITE_RE" '
|
|
function flush( b) {
|
|
if (buf == "") return
|
|
if (buf !~ /^[[:space:]]*#/ && buf ~ re) print file ":" start ": " buf
|
|
buf = ""
|
|
}
|
|
{
|
|
if (buf == "") start = NR
|
|
line = $0
|
|
if (line ~ /\\$/) { buf = buf substr(line, 1, length(line) - 1) " "; next }
|
|
if (line ~ /[|][[:space:]]*$/) { buf = buf line " "; next }
|
|
buf = buf line
|
|
flush()
|
|
}
|
|
END { flush() }
|
|
' "$f"
|
|
done
|
|
}
|
|
|
|
# ---------------------------------------------------------------------------
|
|
echo "--- the scanner flags each vulnerable spelling and nothing else ---"
|
|
# ---------------------------------------------------------------------------
|
|
BAD="$RUN_TMP/bad.sh"
|
|
cat > "$BAD" <<'EOF_BAD'
|
|
#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
if echo "$OUT" | grep -q "needle"; then :; fi
|
|
! printf '%s\n' "$OUT" | grep -qF "needle" && :
|
|
if printf '%s' "$OUT" | grep -E -q "a|b"; then :; fi
|
|
if printf '%s\n' "$OUT" \
|
|
| grep -m1 "needle"; then :; fi
|
|
if echo "$OUT" | tr -d ' ' | grep -q "needle"; then :; fi
|
|
echo "$OUT" |
|
|
grep -q "needle" && :
|
|
if echo "$OUT" | egrep -q "needle"; then :; fi
|
|
if printf '%s\n' "$OUT" | fgrep -q "needle"; then :; fi
|
|
if echo "$OUT" | /bin/grep -q "needle"; then :; fi
|
|
if echo "$OUT" | command grep -q "needle"; then :; fi
|
|
if echo "$OUT" | LC_ALL=C grep -q "needle"; then :; fi
|
|
if echo "$OUT" | sed -n '1,$p' | command /usr/bin/grep --quiet "needle"; then :; fi
|
|
EOF_BAD
|
|
ALTSET="$RUN_TMP/alt-pipefail.sh"
|
|
cat > "$ALTSET" <<'EOF_ALT'
|
|
#!/usr/bin/env bash
|
|
set -o errexit -o pipefail
|
|
if echo "$OUT" | grep -q "needle"; then :; fi
|
|
EOF_ALT
|
|
GOOD="$RUN_TMP/good.sh"
|
|
cat > "$GOOD" <<'EOF_GOOD'
|
|
#!/usr/bin/env bash
|
|
set -euo pipefail
|
|
# A comment naming the bad form: echo "$OUT" | grep -q needle
|
|
if grep -q "needle" <<< "$OUT"; then :; fi
|
|
COUNT="$(echo "$OUT" | grep -c "needle" || true)"
|
|
echo "$OUT" || grep -q "needle" <<< "$OUT"
|
|
run_wrap "$DIR" file.md | grep -q "needle" || :
|
|
printf '%s\n' "$OUT" | grep -c "needle"
|
|
echo "$OUT" | tail -n 1
|
|
echo "$OUT" | tr -d ' ' | tail -n 1
|
|
echo "$OUT" | mygrep -q "needle"
|
|
echo "$OUT" | command tail -n 1
|
|
echo "$OUT" | LC_ALL=C sort
|
|
echo "$OUT" |
|
|
tail -n 1
|
|
echo ok && run_wrap "$DIR" | grep -q "needle"
|
|
EOF_GOOD
|
|
NOPIPEFAIL="$RUN_TMP/no-pipefail.sh"
|
|
cat > "$NOPIPEFAIL" <<'EOF_NOPF'
|
|
#!/usr/bin/env bash
|
|
set -eu
|
|
if echo "$OUT" | grep -q "needle"; then :; fi
|
|
EOF_NOPF
|
|
|
|
BAD_HITS="$(scan "$BAD")"
|
|
BAD_COUNT="$(grep -c . <<< "$BAD_HITS" || true)"
|
|
if [[ "$BAD_COUNT" -eq 12 ]]; then
|
|
pass "all 12 vulnerable spellings are flagged (echo, negated printf, split options, both continuations, an intermediate stage, egrep/fgrep, and path/command/env-assignment reader prefixes)"
|
|
else
|
|
fail "expected 12 hits in the bad fixture, got $BAD_COUNT: $BAD_HITS"
|
|
fi
|
|
if grep -q "^$BAD:6: " <<< "$BAD_HITS"; then
|
|
pass "a backslash-continued pipe is reported at the line the command starts"
|
|
else
|
|
fail "the backslash-continued site was not reported at line 6: $BAD_HITS"
|
|
fi
|
|
if grep -q "^$BAD:9: " <<< "$BAD_HITS"; then
|
|
pass "a pipeline wrapped after a trailing '|' is reported at the line the command starts"
|
|
else
|
|
fail "the trailing-pipe continuation site was not reported at line 9: $BAD_HITS"
|
|
fi
|
|
ALT_HITS="$(scan "$ALTSET")"
|
|
if [[ -n "$ALT_HITS" ]]; then
|
|
pass "pipefail set as 'set -o errexit -o pipefail' is recognised, so its sites are scanned"
|
|
else
|
|
fail "a file setting pipefail after another -o option was skipped entirely"
|
|
fi
|
|
GOOD_HITS="$(scan "$GOOD")"
|
|
if [[ -z "$GOOD_HITS" ]]; then
|
|
pass "here-strings, comments, grep -c, '||', '&&', command writers and non-grep readers are left alone"
|
|
else
|
|
fail "the clean fixture was flagged: $GOOD_HITS"
|
|
fi
|
|
if [[ -z "$(scan "$NOPIPEFAIL")" ]]; then
|
|
pass "a file without pipefail is not flagged — grep's own status is the pipeline's"
|
|
else
|
|
fail "a file without pipefail was flagged"
|
|
fi
|
|
|
|
# ---------------------------------------------------------------------------
|
|
echo "--- no tracked shell script carries the race ---"
|
|
# ---------------------------------------------------------------------------
|
|
FILES=()
|
|
while IFS= read -r rel; do
|
|
[[ "$REPO_ROOT/$rel" == "$SELF" ]] && continue
|
|
[[ -f "$REPO_ROOT/$rel" ]] && FILES+=("$REPO_ROOT/$rel")
|
|
done < <(git -C "$REPO_ROOT" ls-files -- '*.sh' '*.bats' '*.bash')
|
|
|
|
if [[ "${#FILES[@]}" -lt 20 ]]; then
|
|
fail "only ${#FILES[@]} tracked shell files found — the scan is looking in the wrong place"
|
|
else
|
|
LIVE_HITS="$(scan ${FILES[@]+"${FILES[@]}"})"
|
|
if [[ -z "$LIVE_HITS" ]]; then
|
|
pass "none of ${#FILES[@]} tracked shell files pipes echo/printf into an early-exit grep under pipefail"
|
|
else
|
|
fail "use \`grep -q PATTERN <<< \"\$VAR\"\` instead at:"
|
|
echo "${LIVE_HITS//"$REPO_ROOT/"/ }"
|
|
fi
|
|
fi
|
|
|
|
echo ""
|
|
echo "Results: $PASS passed, $FAIL failed"
|
|
[[ "$FAIL" -eq 0 ]]
|