fix(tests): replace pipefail-racy echo | grep -q with here-strings
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
This commit is contained in:
@@ -115,9 +115,9 @@ EOF
|
||||
run_fake "$DIR1"
|
||||
if [[ $FAKE_RC -ne 0 ]]; then
|
||||
fail "a healthy fixture failed: $FAKE_OUT"
|
||||
elif ! echo "$FAKE_OUT" | grep -q "^=== bats ===$"; then
|
||||
elif ! grep -q "^=== bats ===$" <<< "$FAKE_OUT"; then
|
||||
fail "a healthy run never announced the bats leg: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$"; then
|
||||
elif grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$" <<< "$FAKE_OUT"; then
|
||||
pass "a passing case script and a healthy bats runner report 1 passed, 0 failed"
|
||||
else
|
||||
fail "a healthy run produced the wrong summary: $FAKE_OUT"
|
||||
@@ -140,7 +140,7 @@ EOF
|
||||
run_fake "$DIR2"
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a non-executable run-bats.sh exited 0 — the whole bats suite can vanish silently"
|
||||
elif echo "$FAKE_OUT" | grep -q "bats runner not found or not executable"; then
|
||||
elif grep -q "bats runner not found or not executable" <<< "$FAKE_OUT"; then
|
||||
pass "a non-executable run-bats.sh fails the run and names what is missing"
|
||||
else
|
||||
fail "the run failed but not with the missing-runner message: $FAKE_OUT"
|
||||
@@ -160,7 +160,7 @@ EOF
|
||||
run_fake "$DIR3"
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a missing run-bats.sh exited 0 — a rename deletes the bats suite from the run with no diagnostic"
|
||||
elif echo "$FAKE_OUT" | grep -q "bats runner not found or not executable"; then
|
||||
elif grep -q "bats runner not found or not executable" <<< "$FAKE_OUT"; then
|
||||
pass "a missing run-bats.sh fails the run and names what is missing"
|
||||
else
|
||||
fail "the run failed but not with the missing-runner message: $FAKE_OUT"
|
||||
@@ -180,7 +180,7 @@ EOF
|
||||
run_fake "$DIR4" --bats-only
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "--bats-only with no runner exited 0 having printed nothing — a total no-op reported as a pass"
|
||||
elif echo "$FAKE_OUT" | grep -q "bats runner not found or not executable"; then
|
||||
elif grep -q "bats runner not found or not executable" <<< "$FAKE_OUT"; then
|
||||
pass "--bats-only fails when the runner is missing rather than doing nothing quietly"
|
||||
else
|
||||
fail "--bats-only failed but not with the missing-runner message: $FAKE_OUT"
|
||||
@@ -232,11 +232,11 @@ add_case "$DIR5B" test-ok.sh <<'EOF'
|
||||
echo "fine"
|
||||
EOF
|
||||
run_fake "$DIR5B"
|
||||
if echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$"; then
|
||||
if grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$" <<< "$FAKE_OUT"; then
|
||||
fail "an empty run-bats.sh produced a green summary — the bats suite vanished with no diagnostic"
|
||||
elif [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "an empty run-bats.sh exited 0: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "without reporting an 'N tests, M failures' summary"; then
|
||||
elif grep -q "without reporting an 'N tests, M failures' summary" <<< "$FAKE_OUT"; then
|
||||
pass "an empty run-bats.sh fails the run and says the bats suite was never verified"
|
||||
else
|
||||
fail "the run failed but not with the no-summary message: $FAKE_OUT"
|
||||
@@ -264,7 +264,7 @@ EOF
|
||||
run_fake "$DIR5C"
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a bats runner reporting 0 tests exited 0 — a suite that executed nothing read as green: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "reported 0 tests"; then
|
||||
elif grep -q "reported 0 tests" <<< "$FAKE_OUT"; then
|
||||
pass "a bats runner reporting 0 tests fails the run and says the suite executed nothing"
|
||||
else
|
||||
fail "the run failed but not with the zero-tests message: $FAKE_OUT"
|
||||
@@ -298,11 +298,11 @@ kill -9 $PPID
|
||||
sleep 5
|
||||
EOF
|
||||
run_fake "$DIR6"
|
||||
if echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$"; then
|
||||
if grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$" <<< "$FAKE_OUT"; then
|
||||
fail "an empty status file counted as a pass — a killed job reads as green"
|
||||
elif [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "an empty status file did not fail the run: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^=== Summary: 0 passed, 0 skipped, 1 failed ===$"; then
|
||||
elif grep -q "^=== Summary: 0 passed, 0 skipped, 1 failed ===$" <<< "$FAKE_OUT"; then
|
||||
pass "an empty status file is counted as FAILED"
|
||||
else
|
||||
fail "an empty status file failed the run with the wrong summary: $FAKE_OUT"
|
||||
@@ -326,7 +326,7 @@ EOF
|
||||
run_fake "$DIR7"
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a SIGKILLed job did not fail the run: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^=== Summary: 0 passed, 0 skipped, 1 failed ===$"; then
|
||||
elif grep -q "^=== Summary: 0 passed, 0 skipped, 1 failed ===$" <<< "$FAKE_OUT"; then
|
||||
pass "a job killed with no status file written is counted as FAILED"
|
||||
else
|
||||
fail "a SIGKILLed job failed the run with the wrong summary: $FAKE_OUT"
|
||||
@@ -348,7 +348,7 @@ EOF
|
||||
run_fake "$DIR8"
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a case script with a syntax error did not fail the run: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^=== Summary: 0 passed, 0 skipped, 1 failed ===$"; then
|
||||
elif grep -q "^=== Summary: 0 passed, 0 skipped, 1 failed ===$" <<< "$FAKE_OUT"; then
|
||||
pass "a case script that fails to parse is counted as FAILED"
|
||||
else
|
||||
fail "a syntax error failed the run with the wrong summary: $FAKE_OUT"
|
||||
@@ -380,11 +380,11 @@ EOF
|
||||
run_fake "$DIR9"
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a case script exiting 1 did not fail the run: $FAKE_OUT"
|
||||
elif ! echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 1 skipped, 1 failed ===$"; then
|
||||
elif ! grep -q "^=== Summary: 1 passed, 1 skipped, 1 failed ===$" <<< "$FAKE_OUT"; then
|
||||
fail "the pass/skip/fail split was miscounted: $FAKE_OUT"
|
||||
elif ! echo "$FAKE_OUT" | grep -q "^ test-b-skips.sh$"; then
|
||||
elif ! grep -q "^ test-b-skips.sh$" <<< "$FAKE_OUT"; then
|
||||
fail "the skipped script was not named in the skip list: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^ test-a-fails.sh$"; then
|
||||
elif grep -q "^ test-a-fails.sh$" <<< "$FAKE_OUT"; then
|
||||
pass "exit 1 is FAILED, exit 77 is SKIPPED, and both are named in their lists"
|
||||
else
|
||||
fail "the failed script was not named in the failure list: $FAKE_OUT"
|
||||
@@ -422,11 +422,11 @@ EOF
|
||||
run_fake "$DIR10" --strict
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "--strict passed with a skipped suite — the gate reports green having verified less than it ran: $FAKE_OUT"
|
||||
elif ! echo "$FAKE_OUT" | grep -q "a skip is a SETUP ERROR"; then
|
||||
elif ! grep -q "a skip is a SETUP ERROR" <<< "$FAKE_OUT"; then
|
||||
fail "--strict failed but never said a skip is a setup error: $FAKE_OUT"
|
||||
elif ! echo "$FAKE_OUT" | grep -q "test-needs-a-binary.sh"; then
|
||||
elif ! grep -q "test-needs-a-binary.sh" <<< "$FAKE_OUT"; then
|
||||
fail "--strict failed without naming the skipped suite: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^ SKIP: frobnicator is not installed"; then
|
||||
elif grep -q "^ SKIP: frobnicator is not installed" <<< "$FAKE_OUT"; then
|
||||
pass "--strict fails on a skip, names the suite, and carries through the reason it printed"
|
||||
else
|
||||
fail "--strict named the suite but swallowed its skip reason: $FAKE_OUT"
|
||||
@@ -445,7 +445,7 @@ STRICT_ENV_OUT="$(TMPDIR="$STRICT_ENV_PRIV" TEST_DIR="$DIR10/cases" RUN_TESTS_ST
|
||||
bash "$DIR10/tests/run-tests.sh" 2>&1)" || STRICT_ENV_RC=$?
|
||||
if [[ $STRICT_ENV_RC -eq 0 ]]; then
|
||||
fail "RUN_TESTS_STRICT=1 passed with a skipped suite: $STRICT_ENV_OUT"
|
||||
elif echo "$STRICT_ENV_OUT" | grep -q "a skip is a SETUP ERROR"; then
|
||||
elif grep -q "a skip is a SETUP ERROR" <<< "$STRICT_ENV_OUT"; then
|
||||
pass "RUN_TESTS_STRICT=1 is the same gate as --strict"
|
||||
else
|
||||
fail "RUN_TESTS_STRICT=1 failed for some other reason: $STRICT_ENV_OUT"
|
||||
@@ -460,9 +460,9 @@ echo "--- the same skipped suite passes, still SKIPPED, without strict ---"
|
||||
run_fake "$DIR10"
|
||||
if [[ $FAKE_RC -ne 0 ]]; then
|
||||
fail "a skipped suite failed a non-strict run — graceful skipping is gone: $FAKE_OUT"
|
||||
elif ! echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 1 skipped, 0 failed ===$"; then
|
||||
elif ! grep -q "^=== Summary: 1 passed, 1 skipped, 0 failed ===$" <<< "$FAKE_OUT"; then
|
||||
fail "a non-strict run miscounted the skip: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^ SKIP: frobnicator is not installed"; then
|
||||
elif grep -q "^ SKIP: frobnicator is not installed" <<< "$FAKE_OUT"; then
|
||||
pass "without strict the suite is SKIPPED, the run passes, and the reason is still reported"
|
||||
else
|
||||
fail "a non-strict run passed but dropped the skip reason: $FAKE_OUT"
|
||||
@@ -484,7 +484,7 @@ EOF
|
||||
run_fake "$DIR10D" --strict
|
||||
if [[ $FAKE_RC -ne 0 ]]; then
|
||||
fail "--strict failed a run with nothing skipped — it fails unconditionally: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$"; then
|
||||
elif grep -q "^=== Summary: 1 passed, 0 skipped, 0 failed ===$" <<< "$FAKE_OUT"; then
|
||||
pass "--strict leaves a run with no skips green"
|
||||
else
|
||||
fail "--strict passed with the wrong summary: $FAKE_OUT"
|
||||
@@ -506,7 +506,7 @@ EOF
|
||||
run_fake "$DIR10E" --strickt
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "a misspelled flag was ignored and the run passed — a typo silently disarms the gate: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "Usage: .*--bats-only.*--strict"; then
|
||||
elif grep -q "Usage: .*--bats-only.*--strict" <<< "$FAKE_OUT"; then
|
||||
pass "an unrecognised flag fails the run with usage"
|
||||
else
|
||||
fail "an unrecognised flag failed but not with usage: $FAKE_OUT"
|
||||
@@ -532,7 +532,7 @@ EOF
|
||||
run_fake "$DIR10F" --strict
|
||||
if [[ $FAKE_RC -eq 0 ]]; then
|
||||
fail "--strict passed on a suite that skipped via stderr: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^ widgetizer not installed -- skipping"; then
|
||||
elif grep -q "^ widgetizer not installed -- skipping" <<< "$FAKE_OUT"; then
|
||||
pass "a skip reason printed to stderr without a SKIP: prefix is still carried into the failure"
|
||||
else
|
||||
fail "--strict failed but lost the stderr skip reason: $FAKE_OUT"
|
||||
@@ -570,9 +570,9 @@ run_fake "$DIR10"
|
||||
unset RUN_TESTS_STRICT
|
||||
if [[ $FAKE_RC -ne 0 ]]; then
|
||||
fail "an inherited RUN_TESTS_STRICT=1 turned a non-strict fixture strict — this suite's own result depends on how it was launched: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "a skip is a SETUP ERROR"; then
|
||||
elif grep -q "a skip is a SETUP ERROR" <<< "$FAKE_OUT"; then
|
||||
fail "the fixture ran strict despite not asking for it: $FAKE_OUT"
|
||||
elif echo "$FAKE_OUT" | grep -q "^=== Summary: 1 passed, 1 skipped, 0 failed ===$"; then
|
||||
elif grep -q "^=== Summary: 1 passed, 1 skipped, 0 failed ===$" <<< "$FAKE_OUT"; then
|
||||
pass "an ambient RUN_TESTS_STRICT=1 is scrubbed from fixtures that did not ask for strict"
|
||||
else
|
||||
fail "the scrubbed run produced the wrong summary: $FAKE_OUT"
|
||||
|
||||
Reference in New Issue
Block a user