feat(code-quality): arm the pipefail early-close gate fleet-wide (backend#2264)

[LukasWodka]

Closes the build half of tracebloc/backend#2264.

client has carried the only copy of this rule for a month. This moves it into the shared workflow so every repo is covered by one implementation — six copies of a scanner is exactly the drift the rule exists to prevent.

No per-repo caller changes

Every repo already calls code-quality.yml, so the gate arrives as a new job inside it. That sidesteps caller-drift.py's sequencing constraint completely: there is no 16-repo wave to land in a particular order, and nothing goes red on main while the last repo catches up.

Armed green — measured, not assumed

Across 18 repos at their default branches:

shell files in scope	134
running under errexit + pipefail	68
offenders	0

The file counts are here because "0 offenders" and "scanned nothing" print the same thing. Five repos have no shell at all and exit 0 on "nothing in scope" — honest, rather than vacuous. This satisfies backend#1729 rule 4: never land a red gate.

Whole-tree, and here that is correctness rather than policy

action-pins scans whole-tree as a policy choice. This job has a harder reason: whether a line is hazardous depends on whether its file runs under both options, and a library that sets neither inherits them from whatever sources it. A diff-scoped scan resolves inheritance against a partial tree — edit lib/foo.sh without touching its sourcer and the gate calls it safe. One awk pass over the tracked shell files costs nothing worth saving.

rc=2 (cannot tell) is fatal regardless of soft-fail. A gate that could not read the tree has not reported a clean tree; letting soft-fail swallow that is how a gate becomes decoration.

Proof the shared copy behaves like the local one

Not asserted — run. Pointed at clientat the commit before its #1778 cleanup, the shared gate finds exactly the instances that cleanup converted:

scripts/lib/detect-gpu.sh:22 / :23
scripts/lib/gpu-plugins.sh:47
scripts/lib/preflight.sh:909
scripts/tests/e2e-auto-upgrade.sh:74 / :78
scripts/tests/e2e-proxy.sh:131 / :301

Three of those are inside scripts/lib/*.sh, which set neither option — they are only reachable through the inheritance fixpoint. That is the half a plain awk cannot do, and it works here.

The three reviewer-found fixes from the client PRs are carried over intact: || is not a pipe, the stand-in must also be a boundary, and |&is a pipe.

Tests, in this repo's conventions

bats isn't available here, so the 42 bats cases are ported to the plain-shell record style used by house-rules-selftest.sh:

• pipefail-early-close-selftest.sh — 40 cases, both directions: the hazard fires, the house idioms are spared, options are positional (set +e stands the rule down), long and short spellings, one-line function bodies, the terminator class, inheritance (direct and transitive), the derived file list, and fail-closed
• pipefail-early-close-mutations.py — 17 mutations across both files, 0 uncaught. Two targets because the rule genuinely lives in two: the awk decides which lines offend, the wrapper decides which files run under both options
• wired into SELFTEST_TARGETS / MUTATION_TARGETS, so selftests-cover sees them (13 selftests, 2 mutation runners), and the cheap --dry anchor check joins make lint

The mutation harness earned its keep on the first run."the inheritance fixpoint is skipped" came back UNCAUGHT — my fixture was one level deep, and one loop iteration resolves that, so the fixpoint itself was unpinned. Real installers are deeper (install.sh → common.sh → log.sh). I added a transitive two-level case and the mutation is caught now.

That is the same trap client#781 is about — a fixture that only exercises the shape the author had in mind — caught here by the harness instead of by a reviewer.

Verification

make check green (lint + 13 selftests). make lint includes the new dry-anchor check. YAML parses, actionlint clean.

Follow-up, deliberately not in this PR

Retire client's local copy once this is on main. Doing it here would leave client ungated in between. Note the job only goes live for callers when develop → main promotes, since callers pin @main.

🤖 Generated with Claude Code

---

Note

Medium Risk
New default-on job in the org reusable quality workflow can annotate or fail every caller once soft-fail is off. Scanner false negatives would hide real abort bugs; false positives would train teams to disable the job.

Overview
Lifts the client-local SIGPIPE rule into shared code-quality.yml as a new early-close job (on by default, whole-tree). It flags producer | head / grep -q / grep -m N in files that actually run under errexit and pipefail, including libraries that inherit those options via source.

The scanner is a positional state machine (pipefail-early-close.awk) plus a wrapper that closes inheritance to a fixpoint and uses the same shell-file classifier as shellcheck. Opt-out is # pipefail-guard: allow. Findings follow soft-fail; non-verdict exits (rc ≠ 0/1) always fail.

Also stops mutation CI from naming a single runner: make mutations / mutations-dry drive the full MUTATION_TARGETS list, and selftests-cover asserts the workflow runs both tiers. New selftest + mutation harness pin the hazard, house idioms, and inheritance.

^{Reviewed by Cursor Bugbot for commit b7c67c7. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

The gate flagged its own test file — fixed, and the failure is worth recording

selftests went red and my local run could not reproduce it. That gap is the interesting part.

git ls-files enumerates tracked files. When I ran make check, the selftest was still untracked, so the gate never scanned it and reported clean. The commit made it tracked; CI scanned it and found ~20 findings in the test suite itself.

The findings were correct, by the rule's own documented limitation. Written as multi-line quoted strings, the fixtures put set -euo pipefail at column 0 of the suite, and the scanner cannot distinguish a quoted string from code — the limitation documented and pinned in client#777. So it read its own test file as a script enabling errexit and flagged every fixture pipe.

Two things I'd rather not gloss over:

1. I wrote that limitation down yesterday and then walked straight into it in the next file I authored. The doc was correct and did not help, which is a fair argument that the limitation deserves a real fix eventually rather than a paragraph.
2. A local check that passes because the file isn't tracked yet is not a check. Anything using git ls-files measures a different tree before and after the first commit.

Fix

Fixtures are now one-line printfformats, so no fixture line sits at column 0. scan_raw takes a format rather than a literal, with the reason written at the helper so nobody tidies them back into heredocs.

Verified in the state CI actually runs

file tracked (confirmed via git ls-files)	✅ so the run below really scanned it
gate on this repo	0 findings
selftest	40 passed, 0 failed
mutations	17, 0 stale, 0 uncaught
make check	green

One more thing this exposed: the suite's final case — "tracebloc/.github is itself clean under the rule" — was vacuous for this file until now, for the same tracked-vs-untracked reason. It is live now: mutating the comment-skip rule reddens it.

[LukasWodka]

bugbot run

[saadqbal]

Read the implementation rather than the write-up. This is a well-built gate and I'd approve it — holding only because Cypress, Vitest, ESLint and Bugbot are all still pending. Four findings below, and I want to be clear up front that none of them is live against the current fleet; they're all latent, and I verified that rather than assuming it.

What I checked and found genuinely right

The fail-closed paths do what the header claims, and — unusually — the file distinguishes the two cases that matter:

cand=$(git ls-files)|| { … "refusing to report clean";exit 2; }
[ -n"$cand" ] || { … "refusing to report clean";exit 2; }
…
[ "${#files[@]}"-eq 0 ] && { echo"no shell files in scope.";exit 0; }

"Nothing in scope" exits 0 with a message; "cannot tell" exits 2. That's the distinction most guards collapse, and getting it right is why the "5 repos have no shell at all" line in the description is evidence rather than hand-waving.

The armed-green evidence is non-vacuous by construction — 134 files in scope / 68 under both options / 0 offenders, published because "0 offenders" and "scanned nothing" print the same thing. That's the same discipline applied to the evidence rather than only to the tests.

The replay against client before its #1778 cleanup is the proof that counts. Finding exactly the eight converted instances, three of them in scripts/lib/*.sh which set neither option and are only reachable through the inheritance fixpoint, validates the hardest part end-to-end rather than by assertion.

All three reviewer-found rules are carried intact, and I checked the awk rather than the claim: || neutralised before the hazard test (:136), |& admitted as a pipe via \|&? in all three hazard patterns (:179–181), and the stand-in treated as a boundary with the reasoning recorded — "doing half of it moves the bug rather than fixing" (:174).

The classifier is derived, not restated — deliberately the same extension-else-shebang rule as the shellcheck job, so the two jobs cannot disagree about what a shell file is.

And the disclosed uncaught mutation — the fixpoint being unpinned because the fixture was one level deep — is the kind of thing most descriptions omit.

1. The source extraction misses two common idioms (latent)

I tested the regex rather than reading it, and two forms produce nothing:

source "$(dirname "$0")/lib.sh" → missed (space inside $( … ) ends the char class)
source "${SCRIPT_DIR}"/file.sh → missed (closing quote mid-path ends it)

Both are ordinary bash. The consequence runs against the file's own contract: an unextracted source means the library never enters haz, so a | head -n1 inside it is not reported — fail-open, in a script whose header says "cannot tell is a finding, never a pass". The header's claim that basename matching is the fail-closed direction is true of matching but not of extraction.

Not live: I grepped client, client-runtime, backend, e2e-test-agent and tracebloc-engine at their default branches — zero occurrences of either form. So the gate is correct against today's fleet. But source "$(dirname "$0")/lib.sh" is common enough that I'd expect it to appear, and when it does the coverage loss is silent. This is the one I'd like fixed here or tracked as a follow-up rather than left implicit.

2. haz is a space-separated string, iterated unquoted (latent)

haz="$haz$f"forfin$haz;do … # word-splitscase"$haz"in*"$cand_f"*) …

A tracked shell path containing a space splits into bogus entries, [ -f "$f" ] skips them, and the real file never joins haz — fail-open again. Not live: zero shell paths contain a space across the five repos I checked. An array (haz+=("$f"), "${haz[@]}") closes it and also removes the substring-membership test.

3. The third seed grep is unreachable — proved, not argued

grep -qE '…-[a-zA-Z]*o?[[:space:]]+pipefail|…-[a-zA-Z]*o[[:space:]]*$' \
|| grep -qE '…-[a-zA-Z]*o[[:space:]]+pipefail' \
||continue

I ran both against set -o pipefail, set -euo pipefail, set +o pipefail, set -eo pipefail, set -euo, set -o, set -e: no input matches the third that the first does not already match. It's dead.

Worth mentioning only because of the standard this PR sets for itself: deleting that clause is a mutation the 17-case suite would report as uncaught, so it's a small blind spot in an otherwise complete harness rather than a bug.

4. The seed also matches two lines that don't enable pipefail (latent, safe direction)

The -[a-zA-Z]*o[[:space:]]*$ alternative matches set -euo and bare set -o. Neither enables pipefail in bash — -o with no argument prints the option list — so those files get scanned as if pipefail were on. That's the fail-closed direction and I'd keep it, but the comment above it says it "mirrors the sign check" and doesn't mention the widening. One clause in that comment would stop the next reader treating it as a bug.

---

The sign check itself is right, which is the part that mattered: set +o pipefail is rejected by both greps, so a file explicitly turning pipefail off is not seeded as hazardous.

When the four checks come back green I'll approve. Item 1 is the only one I'd want an answer on — fix here or a ticket, your call.

[LukasWodka]

bugbot run

[saadqbal]

Three fail-open holes closed since my last read, and Bugbot found two that I read straight past. Worth being explicit about that before the remaining items, because one of them is the more serious defect on this PR and I had the line in front of me.

The one I should have caught

set -eu -o pipefail. I tested the seed empirically rather than by eye — seven spellings: set -o pipefail, set -euo pipefail, set +o pipefail, set -eo pipefail, set -euo, set -o, set -e — and reported the sign check sound. The split form wasn't in my input set, and it's an ordinary spelling that house-rules.sh already treats as first-class. I've spent several reviews this week criticising fixtures that only exercise the shapes their author had in mind; my seven-case table did exactly that.

Verified the fix, including that widening .* didn't cost the sign check:

set -eu -o pipefail SEEDED ← the hole, now closed
set -o pipefail / set -euo pipefail SEEDED
set +o pipefail skipped
set -eu +o pipefail skipped ← sign check survives the split form too
# set -o pipefail skipped ← still anchored

And your note on it is the right standard: "no repo used the split form, so nothing was being under-reported in practice — but 'no instance today' is not a property." That's the same test I applied to my own latent findings, applied back.

The one that actually mattered, and I quoted the code approvingly

out=$(awk -v hazardous="$haz" -f "$AWK_PROG""${files[@]}")if [ -n"$out" ];then …

This file runs set -uo pipefailwithout errexit — deliberately, so it can classify rather than die — so a crashing awk left $out empty and the script exited 0. A crashed scanner read as a clean tree.

I listed the fail-closed paths in my review (git ls-files failing → 2, no tracked files → 2) and called the distinction between "nothing in scope" and "cannot tell" the thing most guards get wrong — and missed that the scanner's own status was the one unchecked path. Reproducing it by corrupting the awk program (rc 0 before, 2 after) is how that should be proven.

Bugbot's open Makefile Medium is right, and the file already argues its case

check-all at :129 is still check credential-scan mutation-house-rules, so make check-all can go green without running mutation-pipefail-early-close.

What makes it more than a one-word slip is that this PR documents the exact failure mode two hundred lines further down — and then guards only half of it:

# CI runs this rather than any individual target: `selftests.yml` used to say# `make mutation-house-rules`, so adding a second runner wired it into# `selftests-cover` ... while CI never executed it -- covered on paper, unrun# in fact (Bugbot, .github#300).mutations: $(MUTATION_TARGETS)

selftests-cover now enforces that for the workflow — it greps selftests.yml for make mutations. But it reads only .github/workflows/selftests.yml, so the identical mistake in the Makefile's own full-check target is outside its scope. Same bug, one layer over, exactly like the sibling-guard flaw in client#787.

check-all: check credential-scan mutations is the fix. The derived version, if you want the class rather than the instance: have selftests-cover also refuse any Makefile recipe that names a MUTATION_TARGETS member directly, allowing only the mutations aggregate and the member definitions. That's the shape that caught two extra files in unlisted_namers() on .github#295.

New, small, and in the safe direction

Testing the widened seed turned up one more: .* now spans a trailing comment.

set -e # then -o pipefail elsewhere SEEDED

That's fail-closed — the file gets scanned when it needn't be — so I wouldn't hold anything on it. But it's the mirror image of the hole this same rule family closed in e2e-test-agent#184, where a trailing comment could disarm the flag check. There the awk strips trailing comments before deciding; here the seed doesn't. Worth a sed 's/#.*//' or a note, mostly so the asymmetry is deliberate rather than incidental.

Still open from my last round

Neither was among the three, and both remain latent rather than live:

1. Source extraction (:135, unchanged) still misses source "$(dirname "$0")/lib.sh" and source "${DIR}"/file.sh — the fail-open direction, in the half this wrapper exists for. Zero occurrences fleet-wide when I checked.
2. haz is still a space-separated string iterated unquoted at :125. Zero spaced shell paths fleet-wide.

Given how the split form went — no instance today, real hole tomorrow — item 1 is the one I'd still like closed here or ticketed.

Holding: Cursor Bugbot is re-running on a91e6ffb and its Makefile thread is open. Everything else on the PR is green.

[LukasWodka]

bugbot run

[LukasWodka]

bugbot run

[saadqbal]

Two more fixed, three threads open — and Bugbot has now independently found both of the findings I left open last round, which is a reasonable signal they're worth closing rather than deferring.

Fixed

The Makefile member-naming, in both places.check-allandlint — the commit says "Bugbot, 2nd time", which is the argument for the derived version I suggested last round: a guard that refuses any recipe naming a MUTATION_TARGETS member would have caught both in one pass instead of one per round. Worth doing even now, because there'll be a third target eventually.

source "worker.sh" — the s|^"|| closes the leading-double-quote case.

Still open, and I tested rather than assumed

:125 — haz is still a space-joined string (:126, :133, :140), while files is correctly an array. Same finding I raised; Bugbot's framing of the consequence is right — inheritance never walks that sourcer, so libs under it read as safe. Fail-open. haz+=(…) / "${haz[@]}" closes it and drops the substring-membership test with it.

:136 — the extraction is fixed for double quotes only. I ran the current grep+sed against the forms that matter:

source 'single.sh' → 'single.sh ← leading apostrophe survives
source "$(dirname "$0")/lib.sh" → <none> ← space inside $( ) ends the char class
source "${SCRIPT_DIR}"/file.sh → <none> ← closing quote mid-path ends it
source "worker.sh" → worker.sh ✓ (this round's fix)

Three of four still miss. I'd stop accreting a sed clause per quoting style — that's how the fourth one arrives — and extract the basename directly, which is what the file's own header says is the only statically-known part anyway. Tested against all eight forms in play:

# line-level filter, then the basename-shaped token
grep -nE '(^|[[:space:]])(source|\.)[[:space:]]'"$f"| … grep -oE '[A-Za-z0-9._-]+\.(sh|bash)'

$(dirname "$0")/lib.sh → lib.sh "${SCRIPT_DIR}"/quoted-outside.sh → quoted-outside.sh
"worker.sh" → worker.sh 'single.sh' → single.sh
"${LIB_DIR}/common.sh" → common.sh . "${LIB_DIR}/log.sh" → log.sh
. ./relative.sh → relative.sh lib/plain.sh → plain.sh
echo "not a source: helper.sh" → correctly skipped

Two caveats I'd rather state than have you discover: it will over-match prose like echo "source the setup.sh first" — the fail-closed direction, an extra file in haz, which someone can silence with the marker — and you'd want to decide whether to take every basename-shaped token on the line or just the last, where taking all is the safer choice.

The selftest finding is worse than Low, and in the opposite direction

Bugbot says a 141 would "record a false failure". For most of the fourteen, yes. But :166 has a negated half:

ifprintf'%s'"$OUT"| grep -q 'a-bad.sh'&&!printf'%s'"$OUT"| grep -q 'b-good.sh';then

grep -q closes the pipe when it matches, so printf takes SIGPIPE, pipefail makes the pipeline 141, and ! flips that to true. So the assertion "b-good.sh is absent from the output" passes precisely when b-good.shis present and the output is large enough. That's a false pass, not a false failure — and it's the one assertion proving the gate spares clean files. If the gate ever regressed into flagging b-good.sh, this case could stay green.

And your own gate cannot catch it. The selftest is set -uo pipefail at :26 with no -e, so the seed's errexit requirement isn't met and the file is never scanned. That's correct by the rule's definition — without errexit a 141 doesn't abort anything — but the consequence is that the fourteen instances sit in the one file nobody will be warned about, and for a test suite the 141 doesn't need to abort to do damage; flipping an assertion is enough.

Not reachable today: $OUT is a handful of lines over a fixture tree. But it's a test whose correctness depends on its own output staying under the pipe buffer, in the file whose entire purpose is proving that dependency is a defect.

The fix is the idiom this file already teaches — its own fixtures at :142/:146 use grep -q needle <<<"$out" as the spare form:

if grep -q 'a-bad.sh'<<<"$OUT"&&! grep -q 'b-good.sh'<<<"$OUT";then

Holding — Cursor Bugbot is re-running and three threads are open. Everything else is green, and none of the above is a new regression; it's the same fail-open surface being closed one facet at a time.

[LukasWodka]

@saadqbal all three of your remaining items are closed, plus one your review could not have seen.

1. Source extraction (fail-open) — fixed

source "$(dirname "$0")/lib.sh" and source "${DIR}"/file.sh both missed, confirmed before fixing. An embedded quote ended the match early.

Quotes are now stripped before extraction rather than tolerated inside the pattern, and (/) are excluded from the token so a $(dirname …) prefix ends it. Six spellings asserted through the real gate:

"${LIB_DIR}/worker.sh" · "worker.sh" · bare path · . form · "$(dirname "$0")/lib/worker.sh" · "${LIB_DIR}"/worker.sh

You were right to push on this rather than accept the ticket — same reasoning as the split form.

2. haz as a string — fixed

Now an array. Zero spaced paths fleet-wide, but after the split form I'm not treating "no instance today" as a reason.

3. Trailing comment in the seed — fixed

Both halves strip comments now, so the asymmetry with the awk is gone. Agreed it was fail-closed and not urgent; the point was that it should be deliberate.

And one CI would have passed

Converting haz to an array introduced a bash 3.2 regression:

scripts/pipefail-early-close.sh: line 168: haz[@]: unbound variable

3.2 is still /bin/bash on every macOS, and it treats an empty array's [@] as unbound under set -u. The runners are bash 5. So this would have been green in CI and broken for every developer running the suite locally — and specifically on the trees where nothing is hazardous, i.e. the healthy case. Now ${haz[@]+"${haz[@]}"}.

Only caught because the suite gets run on macOS before pushing. Worth noting since the whole tier is meant to run locally too.

On your check-all item

Already fixed two commits before your review — and in the derived form you suggested: selftests-cover now refuses any Makefile where check-all or lint names a MUTATION_TARGETS member instead of the mutations/mutations-dry aggregate. Both arms mutation-proved. You were reading a stale head.

The mutation harness caught itself twice

The seed rewrite left two anchors stale and made one mutation inert — quote-tolerance can't redden anything once the sed strips quotes first. Anchors are now generated from the file rather than retyped, the inert one replaced by removing the strip itself, duplicates gone.

State

selftest	52 passed, 0 failed
mutation tier	7 + 20, 0 stale, 0 uncaught
fleet, re-measured after both widenings	18 repos, 0 offenders, no rc>1
make check / make lint	green

Every fix in this PR that widens what the gate can see gets the fleet sweep re-run — that's four re-runs now. Ready for another look.

[LukasWodka]

bugbot run

[saadqbal]

All three of my items are closed, each with an end-to-end test rather than an assertion — which is the part worth noting, because two of them were latent and easy to fix without proving anything.

• Extraction. I ran sourced_basenames() against all eight forms in play and it returns the right basename for every one, including the two that were missing ($(dirname "$0")/lib.sh, "${DIR}"/file.sh) and the single-quoted case. Stripping quotes and comments before extraction is cleaner than the token class I suggested — it makes (/) the only thing the pattern has to reason about. Both new spellings are in the fixture list too.
• haz as an array, and the selftest actually builds a repo under my scripts/ and drives the gate through it. The ${haz[@]+"${haz[@]}"} expansion for bash 3.2 is a real trap avoided — "${haz[@]}" on an empty array under set -u is an error there, which is exactly the environment a macOS pre-push hook runs in.
• The trailing-comment seed, with a test proving a comment naming the flag doesn't seed. And the reason given is the right one: "fixed so the two halves of one rule agree, since the awk already strips comments." That was a fail-closed nit and it got treated as a consistency bug, which is the correct weight.

"No repo has one today; that is not a property" appearing in the test comment is the standard I'd want applied to the next one of these.

The one still open — and only one of the seven matters

Bugbot's :166 threads are correct, and the scope is narrower than the count suggests. Seven printf '%s' "$OUT" | grep -q assertions in the harness; exactly one is negated:

ifprintf'%s'"$OUT"| grep -q 'a-bad.sh'&&!printf'%s'"$OUT"| grep -q 'b-good.sh';then

The other six would turn a SIGPIPE into a spurious failure — loud, someone looks. This one turns it into a spurious pass, because grep -q closes the pipe when it matches, pipefail returns 141, and ! flips that to true. So "b-good.sh is absent" would hold precisely when it is present — in the assertion that proves the gate spares clean files.

Genuinely Low in practice: $OUT is a few lines over a fixture tree, nowhere near the pipe buffer, and the file runs without errexit so nothing aborts. I would not block on it. But it's the one place where the fix is already written down in this very file and not applied — :142 and :146 teach grep -q needle <<<"$out" as the spare form, inside fixture strings, while the harness's own seven assertions use the hazardous shape. Two of those seven arrived in this commit, so the pattern is spreading rather than shrinking.

if grep -q 'a-bad.sh'<<<"$OUT"&&! grep -q 'b-good.sh'<<<"$OUT";then

Still holding: Cursor Bugbot is re-running on 7e360a93 and its two threads are open. Nothing outstanding from me.

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 142f854. Configure here.}

[saadqbal]

The comment-disarm High is properly closed — verified end to end rather than by reading. sub(/[[:space:]]*#.*$/, "", line) as the first statement in apply_set, and running the real awk on set -euo pipefail # note: +e would be bad now flags the hazard below it. The test is two-sided, which is the part that makes it worth having: :143 asserts a +e in a comment does not disarm, and :146 asserts a real set +e still does. Strip-the-comments alone would have passed the first and silently broken the second.

Bugbot's new Medium is real, and the discriminator is a single space

I ran the four variants through the real awk:

rm -f x || true; producer | head -1 FLAGGED ✓
rm -f x || true ; producer | head -1 MISSED ← fail-open
rm -f x || true && producer | head -1 MISSED ← fail-open
rm -f x || : ; producer | head -1 MISSED ← fail-open

Two lines that do exactly the same thing get opposite verdicts depending on whether there's a space before the ;. That's what makes this worth fixing rather than noting — the version people actually write (|| true ; with a space, or || true &&) is the one that fails open, and the version that works is the one that looks like a typo.

The cause is that ([[:space:]]|$|\)) is a half-implemented boundary. It correctly refuses true; — a ; is not in the class — but accepts true followed by anything, and a space is not a boundary between commands. Which is the same shape as the stand-in finding from three rounds ago: "neutralising the pipe is only half the job; the boundary has to read as a boundary." Same lesson, other end of the same line.

A tested fix, and its cost stated

Anchoring the spare to the end of the line (allowing a closing paren) — tested:

\|\|[[:space:]]*(true|:)[[:space:]]*\)?[[:space:]]*$

rm -f x || true ; producer | head -1 now scanned ✓
rm -f x || true && producer | head -1 now scanned ✓
producer | head -1 || true still spared ✓
( producer | head -1 || true ) still spared ✓
producer | head -1 || true; echo done now scanned ← the cost

That last row is a false positive and I'd rather name it than have you find it: a pipeline whose status genuinely is discarded, followed by another command on the same line, would now be reported. It's the fail-closed direction and it's loud, and # pipefail-guard: allow already exists for it — but it is a precision loss on a real idiom, so it's your call whether that's the right trade.

The zero-false-positive version is per-segment evaluation: split on ; and && and apply both the spare and the hazard test to each segment independently. That's more work, and this file already has the \001 stand-in machinery for exactly this kind of boundary rewriting, so it may be less work than it sounds. I'd take the anchored regex now and the segment split only if the false positive actually bites someone.

Holding: Cursor Bugbot is re-running on 142f854a and this thread is open. Nothing else outstanding from me.

[LukasWodka]

bugbot run

[LukasWodka]

@saadqbal your table is now pinned as tests, and the answer to your open question is that the trade you offered was not needed — segment-wise evaluation had already shipped in 142f854a, and it is the version you called "more work… only if the false positive actually bites someone."

All seven rows, measured against the current awk:

rm -f x || true; producer | head -1 FLAGGED
rm -f x || true ; producer | head -1 FLAGGED
rm -f x || true && producer | head -1 FLAGGED
rm -f x || : ; producer | head -1 FLAGGED
producer | head -1 || true spared
( producer | head -1 || true ) spared
producer | head -1 || true; echo done spared ← the cost you named, avoided

So no precision loss, and no # pipefail-guard: allow needed for a real idiom.

Your diagnosis is the part I want on the record.([[:space:]]|$|\)) was a half-implemented boundary: it refused true; because ; is not in the class, but accepted true followed by anything — and a space is not a boundary between commands. So whitespace before the ; decided the verdict, and the spelling people actually write was the one that failed open. You also named it as the same shape as the \001 stand-in finding three rounds earlier, at the other end of the same line. That connection is what turns two bugs into one lesson.

Four cases added: the two whitespace spellings, plus the two spare-forms that would have regressed under the cheaper fix — those recorded explicitly as the reason this approach was chosen, so nobody simplifies it back to the regex later.

67 selftest cases / 0 failed; mutation tier 7 + 21 / 0 stale / 0 uncaught; make check and make lint green.

Nothing outstanding from Bugbot. This is the tenth review round — worth noting that the last four findings were all caught by the mutation harness or by you rather than by CI, which is the argument for the tier existing.

[LukasWodka]

bugbot run

[LukasWodka]

bugbot run

[saadqbal]

You declined my fix and took the harder correct one, and it's strictly better. I ran my own table against the new awk — all eight rows:

rm -f x || true ; producer | head -1 FLAGGED ← was fail-open
rm -f x || true && producer | head -1 FLAGGED ← was fail-open
rm -f x || : ; producer | head -1 FLAGGED ← was fail-open
rm -f x || true; producer | head -1 FLAGGED
producer | head -1 || true spared
( producer | head -1 || true ) spared
producer | head -1 || true; echo done spared ← the false positive my anchor WOULD have caused
x="$(ls /tmp | head -1 || true)" spared ← and this one I hadn't even tested

Segment-scoped with continue instead of a whole-line next is the version I described as "more work"; doing it means the fail-open closes with no precision loss, where my anchored regex would have traded one for the other. The trailing class [[:space:])\"']* picking up the command-substitution form is a case I didn't think to check and would have broken.

And 44aabe9a turning that table into selftest cases — including the negative row proving the false positive doesn't occur — is the right move: my ad-hoc verification was a one-off, and now it's a check. That's the second time on this PR that the harder path was the correct one over what I proposed.

Why I'm still not approving, and it isn't the code

Every check on 44aabe9a is success or skipped — gate, selftests, actionlint, all five quality jobs — except Cursor Bugbot, which is neutral.

I've told this team seven times over the last few hours that neutral is an absence rather than a pass, and that an absence never approves. That applies to me too. Normally I'd resolve it by reading the review threads directly, but GitHub's GraphQL API is rate-limited out for this account right now (0/5000, resets 14:41:13Z) and thread resolution state is GraphQL-only — REST exposes the comments but not whether they're resolved. So the one gate signal I can't read is the one that has been finding real defects on this PR all afternoon.

To be explicit about what that does and doesn't mean: the awk logic is verified by me, by running it, on eight cases including two I'd have got wrong. What I can't tell you is whether the thread at awk:146 is closed or whether Bugbot has opened another on this head. If it's clean, this is an approve on the next pass with nothing further from me.

Worth noting for the record that this is the second GraphQL exhaustion today, and the desk's own org-wide scan is the main consumer — so a pass landing in a depleted window loses exactly the signal that matters most on a PR like this one.

[saadqbal]

Both open findings reproduce. I ran them rather than reading them, and one needs its severity raised.

The High at :146 — confirmed

set -euo pipefail; x="$(ls /tmp | head -1)" spared ← fail-open
set -euo pipefail producer | head -1 FLAGGED ← control

The unconditional next after apply_set means the options get enabled and the rest of the physical line is never judged. Real, and it's the fail-open direction.

The fix is to stop nexting and fall through to the hazard check — and the segmentation you added two rounds ago is what makes that safe: the set -euo pipefail part becomes its own segment, carries no pipe, and can't produce a false positive. set +e; producer | head -1 still correctly spares, because e_on is 0 by the time the second segment is judged.

The Medium at :220 should be a High — there's a third direction

Bugbot named two, and both are fail-closed noise:

producer | head -1 || true # keep going FLAGGED ← spare defeated by the comment
echo hi # producer | head -1 FLAGGED ← comment prose read as a pipe

Annoying, loud, someone investigates. But the same root cause runs the other way too, and this one is silent:

producer | head -1 # || true spared ← REAL hazard, silenced by a comment

A comment that merely mentions|| true end-anchors the spare and the live hazard disappears. That is exactly the class this rule family has been closing all week — a guard a comment can satisfy — except it's now the spare rather than the flag check. e2e-test-agent#184 fixed it for the flag check, apply_set fixed it for the option state, and this is the third face of it.

The obvious fix has its own fail-open — tested

Bugbot's note points at it: "apply_set already strips comments before option parsing; this path does not." Mirroring that strip here is not safe, because a general shell line can carry a # inside quotes where a set line cannot:

echo '# not a comment' | head -1 currently FLAGGED ✓
after a naive sub(/[[:space:]]*#.*$/, "", line):
echo ' the pipe is GONE ← new fail-open

I verified the current awk flags that line correctly today, so a naive strip would trade one silent miss for another. Worth knowing before the one-liner goes in — that's why apply_set's version is sound and a copy of it here wouldn't be: I checked, and the only legitimate # on a set line is inside a positional argument the parser already ignores.

The quote-aware version already exists in the fleet.e2e-test-agent/tests/test_kubectl_bounded.py::_strip_trailing_comments walks the line tracking single/double quote state and breaks on the first # outside quotes — same problem, solved once, in the sibling guard for the same rule. Porting that shape (or masking the comment region with the \001 stand-in this file already uses for ||) closes all three of :220's directions with one change.

---

Requesting changes rather than commenting: :146 is a confirmed fail-open on the gate's core dispatch, and :220's third direction is another. Both are the same shape as findings this PR has already closed twice, which is the argument for the quote-aware boundary rather than a third targeted patch.

For the record on the gate state — Cursor Bugbot reports skipping on 5feabbeb while two of its own threads are open, one of them High. That's now the ninth time this session the check state hasn't reflected its findings, and last pass I couldn't read the threads at all because GraphQL was rate-limited out. Anyone triaging this PR from the checks list alone would see green.

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 5494149. Configure here.}

[saadqbal]

The comment fix is complete and it's the right one. I ran all five cases against 54941493:

producer | head -1 # || true FLAGGED ← was spared (the fail-open I found)
producer | head -1 || true # keep going spared ← was a false positive
echo hi # producer | head -1 spared ← was a false positive
echo '# not a comment' | head -1 FLAGGED ← quote-awareness holds
grep "#" f | head -1 FLAGGED ← quote-awareness holds

Those last two are the ones that matter: a naive sub(/[[:space:]]*#.*$/, "", line) would have reduced the first to echo ' and lost the pipe, so the fix genuinely tracks quote state rather than cutting at the first #. And "in ONE place" in the commit title is the right call — one boundary function serving the spare, the hazard test and the option parser is what stops a fourth face of this appearing.

The High at :173 is still open, and still reproduces

set -euo pipefail; x="$(ls /tmp | head -1)" spared

The fix, and the part of it that a one-liner won't reach

Dropping the unconditional next after apply_set and falling through closes the common shape. Two things I checked so the change lands with its limits known:

No regression in the other direction.producer | head -1; set +e is already flagged correctly today — it doesn't start with set, so it never hits the dispatch and gets judged under the prevailing options. Falling through doesn't disturb that.

But it does not close everything.apply_set applies every option change on the line before any segment is judged, so:

set -euo pipefail; producer | head -1; set +e spared — and still spared after a naive fall-through

The trailing set +e clears e_on before the middle segment is looked at, even though the pipeline runs while errexit is live. Fully correct handling means applying set changes per segment, in order, and judging each segment under the options in effect at that point — which is a bigger change than removing a next.

I'd take the fall-through now: it turns the realistic shape (set …; hazard) from silent to caught, and leaves only set …; hazard; set +e, which nobody writes. Worth a comment saying so rather than leaving the next reader to rediscover it.

Changes requested still stands on that one thread. Everything else on this PR is green and, for the record, Cursor Bugbot reports skipping again while its own High sits open.

[LukasWodka]

@saadqbal changes requested → addressed. Your :146 was real and it hid a bigger one behind it.

:146 — fixed, and the first fix was not enough

Removing the next did not make your case fire. Running it rather than assuming:

set -euo pipefail; producer | head -1 still missed

apply_set splits on whitespace only, so set -euo pipefail; cd /tmp tokenises as pipefail; — which never equals pipefail. The -o handler missed it and p_on stayed 0 for the whole file. Every hazard in such a file was skipped, not just the one sharing the line.

So the visible bug was one line; the one behind it was file-scoped. Both fixed — fall through, and split on [[:space:];]+.

Four cases: the same-line hazard, the whole-file version, and the two discriminations (set +e; cmd and set +o pipefail; cmd must still disarm, or falling through just trades a fail-open for a fail-closed). Mutation-proved: restoring the next and reverting the split each redden their own case and nothing else.

:220 — already closed before your review

All three directions, including the silent one you added, plus the fail-open you warned a naive strip would cause:

producer | head -1 || true # keep going spared
echo hi # producer | head -1 spared
producer | head -1 # || true FLAGGED ← your silent case
echo '# not a comment' | head -1 FLAGGED ← the regex trap you flagged

You were right that the quote-aware shape was the answer rather than a third targeted patch, and right that copying apply_set's regex would have been unsafe. It walks quote state and cuts only at a # outside quotes and preceded by whitespace — which is also why x=a#b; producer | head -1 still fires.

On the Bugbot check state

Noted, and I agree it is the more serious process issue: skipping while its own High thread is open means the checks list lies to anyone triaging. That is not something I can fix in this PR — worth its own ticket against the Bugbot integration, and I'll file one if you want it tracked.

State: 80 selftest cases / 0 failed; mutation tier 7 + 25 / 0 stale / 0 uncaught; make check and make lint green; fleet re-measured after both widenings at 18 repos / 0 offenders.

Ready for re-review.

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit b7c67c7. Configure here.}

[saadqbal]

Zero open threads, Cursor Bugbot reports pass for the first time on this PR, everything else green. Verified the last fix with a battery rather than a read:

set -euo pipefail; x="$(ls /tmp | head -1)" FLAGGED ← the High, was spared
set -eu -o pipefail; producer | grep -q x FLAGGED ← split form, same line
set +e; producer | head -1 spared ← disable still disables
producer | head -1 (own line) FLAGGED ← no regression
producer | head -1 || true spared ← spare intact
echo '# not a comment' | head -1 FLAGGED ← quote-aware strip intact

And you found a second fail-open while fixing the first. "pipefail; never registered" — whitespace-splitting leaves the semicolon attached, so a[i+1] == "pipefail" never matched on set -euo pipefail; … and pipefail was never even enabled. My reproduction case was therefore spared for two independent reasons, and patching only the next would have left it silently spared. That's the difference between fixing the symptom you were handed and reading the function.

One thing I nearly got wrong, and checked instead.set -o pipefail; producer | head -1 came back FLAGGED in my harness and I had it pencilled as a false positive — until I read :39–40: the hazardous list seeds files that inherit both options, so a declared-hazardous file starts with errexit already on and set -o pipefail doesn't clear it. Flagging is correct; my expectation was the thing that was wrong. Worth saying because that seeding is the whole reason the wrapper exists, and it isn't obvious from the awk alone.

The KNOWN LIMITATION is the right way to leave something unfixed

# Counting quotes to find such regions was tried and REJECTED: apostrophes in
# prose … desynchronise the count …, and a desynchronised count HIDES real
# offenders -- strictly worse than reporting a false one.

Four things right about that note: the direction is stated (a false positive, so fail-closed), the obvious fix was tried and rejected with the reason, the escape hatch is named, and the behaviour is pinned by a test so changing it has to be deliberate. A limitation documented like that is worth more than one quietly fixed the wrong way.

---

Eleven rounds, and the shape of it is worth recording: every finding was a boundary in the same rule — the || stand-in, comment tokens crossing into the option state, a space read as a command separator, a comment end-anchoring the spare, and finally the dispatch skipping its own line. Each one closed with an end-to-end test, and by the end the fixes were declining my suggestions in favour of harder correct ones twice (segment-scoping over my end-of-line anchor; the quote-aware boundary over a naive strip). Bugbot found more of the live ones than I did, and two of my sign-offs were wrong along the way — including quoting a code comment's claim about another file without checking it.

The gate itself is good work and I'd own it. Approving.

[LukasWodka]

/fr-pass