test(shell): encode the pipefail early-close rule, and fix the 5 it found (backend#1778)

[LukasWodka]

Fixes tracebloc/backend#1778

The class

Under set -o pipefail + set -e, a producer | head -n N diagnostic aborts its own caller once the producer outgrows the ~64KB pipe buffer: head closes the pipe, the producer takes SIGPIPE, the pipeline returns 141, errexit kills the script. It is size-dependent, which is why instances survive review.

Reproduced, matching the ticket's measurements:

input lines	| grep -q under set -euo pipefail	here-string form
50	exit 0	exit 0
20,000	exit 141	exit 0
200,000	exit 141	exit 0

Every instance the ticket named is already fixed

Verified against the current tree rather than trusted (#1729 rule 8 — a fix for an unreachable path costs more than filing nothing):

Ticket candidate	Actual state
scripts/install.sh:538	already fixed — pure-bash slicing, with a note
scripts/lib/common.sh:237	already fixed — slicing, note cites this ticket
docs/migration-tools/migrate-tenant.sh:242	already fixed — head -25 <<<"$policy_ctx", note cites this ticket
scripts/tests/check-drift.sh:77,123	not the hazard — set -uo pipefail, no errexit, so nothing acts on the 141

So the sweep was done. What was not done is the ticket's last ask: encode the rule so new ones cannot land.

The scanner

scripts/tests/pipefail-early-close.awk flags a pipe into an early-closing reader — head, grep -q, grep -m N — in files enabling both errexit and pipefail. Both are required: pipefail alone returns 141 with nothing acting on it; errexit alone never sees non-zero, because the reader itself succeeded.

Spares || true, comments, and a # pipefail-guard: allow marker (mirroring check-style.sh's opt-out). Reads options set inside a function, not just at the top — that's check-drift.sh's shape, and a top-of-file-only check would misread it.

One implementation, shared by the gate and its own self-tests — never an inline copy, which drifts from the real scanner and then proves that a regex nobody runs would have caught the bug (#1729 rule 9).

Prose was not enough here: the repo already documented this idiom in seven places and still grew five new instances.

It found five the manual sweep missed — none of them head

Site	Shape
e2e-auto-upgrade.sh:74	netpol_has_external_443 — | grep -q
e2e-auto-upgrade.sh:78	jm_deploy — | grep -m1
e2e-proxy.sh:131	auth.docker.io CONNECT — | grep -q
e2e-proxy.sh:301	section A tunnelled — | grep -qiE
e2e-proxy.sh:313	section B did not tunnel — | grep -qiE

These are worse than the abort the ticket describes. All five sit in condition context, where errexit is suppressed — so they don't crash, they answer wrong. Under pipefail a matchedgrep -q still makes the pipeline 141, so:

• e2e-proxy.sh:131 — if ! … | grep -q "$PROXY_USER" fires its error because the CONNECT was present. On a busy proxy log, a passing system reports "the node's image pull did not traverse the proxy."
• e2e-proxy.sh:313 — the 141 reads as "no proxy used", so the assertion is skipped entirely: a false pass, the quieter half of the same bug.
• e2e-auto-upgrade.sh:74 — reports "no external 443" for a policy that has it.

All five converted to here-strings, each with a note naming the consequence.

Mutation proof

10 mutations, control 14/14.

Mutation	Result
head / grep -q / grep -m detection removed	RED (×3)
|| true exemption removed	RED
comment exemption removed	RED
errexit requirement dropped	RED
pipefail requirement dropped	RED
allow marker ignored	RED
per-file flush removed (state leaks across files)	RED
a real production conversion reverted	RED

The last row is the one that matters: the tree gate reddens when the actual fixed code regresses, so the gate guards the code and not merely itself.

Verification

• bats scripts/tests/pipefail-early-close.bats → 14/14; scanner over the tree → 0 offenders
• bats scripts/tests/bats-hygiene.bats → 0 offenders (new suite meets the || return 1 convention)
• bash -n + shellcheck -S error on both changed e2e scripts → clean
• check-style.sh, gen-manifest --check, check-facts --check, check-drift.sh → clean

Follow-up left to the fleet

The same rule as a code-quality house-rule in tracebloc/.github would cover all 16 repos instead of this one. Out of scope here — one self-contained change per PR — and worth filing separately.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes shell control flow in installer libs (GPU detection, diagnose bundle, TLS issuer check, GPU verify) and E2E gates—wrong patterns caused size-dependent aborts or false pass/fail, but logic intent is preserved via safer I/O idioms.

Overview
Fixes backend#1778 by retiring producer | head, | grep -q, and | grep -mN under inherited set -euo pipefail, where an early-closing reader can SIGPIPE the producer (exit 141) and either abort install/diagnostics or invert conditionals in E2E assertions.

Runtime fixes use capture-then-slice or here-strings in detect-gpu.sh, diagnose.sh, gpu-plugins.sh, preflight.sh (_pf_issuer_is_public), plus e2e-auto-upgrade.sh and e2e-proxy.sh helpers that had silent wrong answers on large output.

New CI gate:pipefail-early-close.sh resolves hazardous files via sh-files.sh and sources inherited errexit+pipefail from install.sh into scripts/lib/*; pipefail-early-close.awk flags offending lines; 14 bats tests cover scope, exemptions (|| true, # pipefail-guard: allow), and grep -m. manifest.sha256 updated for touched scripts.

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

[LukasWodka]

bugbot run

[LukasWodka]

bugbot run

[aptracebloc]

Reviewed the whole change — the approach is right and the execution is strong. The 5 conversions to capture-then-match are semantically faithful, and the test scaffolding meets the bar I'd want on a guard: paired FLAG/spare cases so no "spares X" test is vacuous, and a "revert a real production conversion → RED" mutation test so the tree gate guards the code, not merely itself.

Not blocking this, but the two open Bugbot findings are both valid and worth addressing before merge, because coverage is the deliverable here:

1. Sourced installer libraries slip the gate (High). The scanner treats a file as hazardous only if it sets both set -e and pipefail itself. scripts/lib/*.sh don't — they inherit both from install-k8s.sh — so a reverted helm repo list | grep -q / lspci | grep -qi in a lib stays green while it still misbranches under the inherited pipefail. Worth teaching the scanner about the sourced-from-a-hazardous-caller case (or scanning libs under the caller's options).

2. Tree scan misses docker/ and .bash (Medium). The gate hardcodes find scripts docs -name '*.sh', but docker/k3s-cuda/build.sh already enables set -euo pipefail and converted a grep | head for this very ticket — restoring that pipe wouldn't redden the gate. Deriving the file list from the repo's own sh-files.sh (and including *.bash) would close both gaps.

Happy to re-review once those land.

— drafted with Claude Code

[saadqbal]

Good PR. I tried hard to break the central claim — "the scanner is what found these five" — and it holds.

The guard is genuinely derived. I reverted all four lib files to their merge-base content and ran the entry point once:

scripts/lib/detect-gpu.sh:22: success "... $(nvidia-smi --query-gpu=name ... | head -1)"
scripts/lib/detect-gpu.sh:23: log "Driver: $(nvidia-smi --query-gpu=driver_version ... | head -1)"
scripts/lib/diagnose.sh:96: df -h 2>/dev/null | head -20
scripts/lib/gpu-plugins.sh:47: | sed 's/"//g; s/\s*:\s*/=/g' | head -5 \
scripts/lib/preflight.sh:909: printf '%s' "${1:-}" | grep -qiE "DigiCert|Sectigo|..."

Exactly five, exactly the five fixed. Restored, and the scanner is clean on the tree. 17 bats cases pass, and they cover both arms plus the two single-option cases, comments, the allow marker, and cross-file state leakage — which is the coverage that makes "0 findings" mean something.

The inheritance closure is the part that makes this work, and it's the non-obvious bit. Seeding from files that set both options and then closing transitively over source/. is what pulls in scripts/lib/*.sh — and all five hazards live in those inherited-option files. A guard that only asked each file's own set lines would have reported clean on every one of its own motivating cases. Worth saying out loud because I nearly filed that as a gap: I ran the awk directly on a pre-fix lib file, got zero hits, and thought the guard missed its own bugs. The awk header at :41-42 already says why (hazardous is resolved by the entry point and passed in), so that's on me for not reading far enough — but it does mean the awk alone is a misleading thing to reach for while debugging.

Deriving the file list from scripts/sh-files.sh rather than restating a find, and failing closed when it classifies nothing, are both right.

On the five fixes, gpu-plugins.sh is the one I'd highlight. The others turn an abort into a working diagnostic; that one turned a wrong answer into a right one. | head -5 outrunning the buffer meant sed took SIGPIPE and the || echo "" that suppressed the abort also converted the truncation into an empty result — "no GPU" on a GPU cluster, silently. That's a better bug than the ticket was looking for.

The preflight.sh one is the subtlest: grep -q closing on the first match means the pipeline could return 141 on a match, inverting the predicate. Easy to read past.

I also like that the || _x="" neutralisations in detect-gpu.sh are justified rather than assumed — moving argument-position substitutions into assignments genuinely does move them somewhere errexit can see, so the guard is needed and isn't cargo cult.

Holding the approval only on the one pending check. Everything else is clean — 41 passing, 4 skipped, all three Bugbot threads resolved, and the conflict that was there earlier has been merged out. Nothing outstanding from me; I'll approve next pass.

[saadqbal]

Correcting my previous comment — I said "nothing outstanding from me" and then a Bugbot Medium landed on diagnose.sh:101. It's right, and chasing it turned up a root cause worth fixing rather than a one-line revert.

The diagnose.sh change is a regression, and its comment is wrong

run_diagnose() does set +e on its own second line (diagnose.sh:42), commented "every step is best-effort — never abort the bundle mid-collection". The df site is inside that function. So the comment the PR adds above it —

head closes after 20 lines, df takes SIGPIPE, the pipeline is 141 and the whole diagnostic report dies here — on exactly the broken machine this tool exists for

— is not what happens. The 141 was harmless there; errexit is off by design. And the conversion makes things worse in the direction that matters: head -20 <<<"$(df -h)" must fully evaluate $(df -h) before head sees a byte, so a df wedged on a stale NFS or overlay mount now blocks the entire bundle. Previously head closed after 20 lines and the SIGPIPE actively ended the hang. On the broken machine this tool exists for, the old code was the resilient one.

The # pipefail-guard: allow marker you already built is the right home for this line, or just revert it — either way the comment needs to stop claiming an abort that cannot happen.

The root cause: the guard models enabling, never disabling

That flag was a false positive, and it will keep happening, because neither the awk nor the entry point knows about set +e / set +o pipefail:

$ printf 'set -euo pipefail\nf() {\n set +e\n df -h | head -20\n}\n' > t.sh
$ awk -f scripts/tests/pipefail-early-close.awk t.sh
t.sh:4: df -h | head -20 <- inside set +e, not a hazard

bats case 15 pins "it sees options set inside a function, not just at the top", which is the enabling half; the disabling half is unmodelled. Best-effort regions are a normal idiom here — run_diagnose is one in-tree instance and check-drift.sh is called out in the description as another shape of the same thing — so every one of them needs a hand-placed marker. That's the opposite of encoding the rule.

And a smaller one: the pipefail seed doesn't check the sign

The entry point's two seed greps aren't symmetric — errexit checks for -, pipefail matches anything containing the word:

 file with `set +o pipefail`: errexit-seed=yes pipefail-seed=yes <- counted as ON
file with `set -o pipefail`: errexit-seed=yes pipefail-seed=yes

^[[:space:]]*set[[:space:]].*pipefail (:61) matches set +o pipefail, so a file that explicitly turns pipefail off is seeded into the hazardous set. Cheap fix, and it should mirror the sign check the errexit line already does at :60.

What still stands

Everything I verified earlier holds, and I don't want the above to bury it: reverting all four lib files and running the entry point flags exactly the five hazards that were fixed, the tree is otherwise clean, the 17 bats cases cover both arms plus both single-option cases, and deriving the file list from sh-files.sh with a fail-closed empty check is right. The gpu-plugins.sh and preflight.sh findings are genuinely good — a truncation silently becoming "no GPU" on a GPU cluster, and grep -q returning 141 on a match and inverting a predicate, are both worth the whole exercise.

So: three of the five fixes are clean wins, detect-gpu.sh I'd also keep, and it's specifically diagnose.sh plus the two guard gaps above. Requesting changes on that basis.

[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 2637f45. Configure here.}

[saadqbal]

2637f450 landed 75 seconds after my change-request, which is a good turnaround. One of my three asks is closed; two are untouched. Scanner is clean on the tree (exit 0).

Closed — the diagnose.sh revert. The df -h | head -20 pipe is back with the # pipefail-guard: allow marker, and the replacement comment records why rather than just marking it:

head -20 streams and lets collection continue, whereas waiting for every mount blocks the whole bundle when df stalls on an unresponsive NFS/overlay mount

That's the right resolution and better documented than a bare marker. Also good that 9666dac6 caught the detect-gpu regression before I did — I'd endorsed those || _x="" guards last pass without spotting that the conversion was what made a failed --query-gpu fatal in the first place. Argument-position substitutions can't trip errexit; assignments can. Measured unguarded -> exit 1 / guarded -> exit 0 is the right way to settle it.

Still open 1 — the pipefail seed doesn't check the sign. Two characters, no downside:

$ printf 'set -e\nset +o pipefail\ndf -h | head -20\n' > t.sh
$ grep -qE '^[[:space:]]*set[[:space:]].*pipefail' t.sh && echo "seeds as pipefail-ON"
seeds as pipefail-ON

:61 matches anything containing the word, while :60 right above it carefully checks for - on errexit. A file that explicitly turns pipefail off is seeded into the hazardous set. Please just mirror the errexit line.

Still open 2 — set +e regions are still invisible, and I want to make the case better than I did, because "it's only a false positive" is a fair reading and I don't think it survives:

$ printf 'set -euo pipefail\nf() {\n set +e\n df -h | head -20\n}\n' > t.sh
$ awk -f scripts/tests/pipefail-early-close.awk t.sh
t.sh:4: df -h | head -20

The failure direction is a false positive, which normally means "noisy but safe". This PR is the counter-example. The diagnose.sh flag was exactly this false positive, and what happened next was not someone shrugging at noise — it was correct code being converted into a real regression that blocked the whole diagnostic bundle on a hung df, shipped with a comment confidently asserting an abort that could not occur. A guard that flags correct code gets the correct code "fixed". That's the cost, and it's already been paid once here.

Two ways I'd be happy:

• Model it — track set +e / set +o pipefail the way set -e is tracked, so best-effort regions stop being flagged. bats case 15 already pins the enabling half inside functions, so the machinery is there.
• Or make the marker the sanctioned answer, explicitly. If hand-marking is the intended workflow, say so in the awk header next to the inheritance note: "a set +e region is not a hazard and the scanner cannot see it — mark the line, do not convert it." Then the next person reaches for the marker instead of the here-string. Right now nothing tells them which.

Either closes it for me. Everything else from my earlier passes still stands — the inheritance fixpoint is the load-bearing idea, reverting all four lib files flags exactly the five hazards that were fixed, and gpu-plugins.sh (truncation silently becoming "no GPU" on a GPU cluster) remains the best find in the set.

CI has four checks in flight, so this isn't approvable yet regardless.

[LukasWodka]

@saadqbal — you were right that the root cause was worth fixing rather than a one-line revert. Both guard gaps are closed.

The guard modelled enabling, never disabling. The awk is now a positional state machine: set -e / set +e / set -o pipefail / set +o pipefail / combined set -euo pipefail all move the state, and a line is an offender only where both are live. Your repro is the new test:

f() { set +e; df -h | head -20; } -> not flagged
ls | head -1 (after the function) -> flagged

Function boundaries restore the prior state. That is an approximation on purpose — bash does not scope options to functions, a set +e leaks to the caller — but restoring is the conservative direction for a linter: it keeps asking about later code instead of going quiet after the first best-effort helper. Said so in the header rather than leaving it implicit.

The seed sign. Fixed and mirrored on the errexit check. Worth noting the test I first wrote for it was vacuous, and mutation caught it: handing the pipefail-off file straight to the scanner proves nothing now, because the awk's own positional model spares it either way and the seed's verdict is masked. It is only observable in what the seed PROPAGATES to a sourced lib, so that is what the test drives.

Consequence for diagnose.sh: no marker needed. The guard reads set +e and does not flag the region, so I removed the # pipefail-guard: allow from the previous commit and left the line with its reasoning. The marker stays for genuine cases.

Tests 17 → 21, all three fixes mutation-proved. End-to-end: reverting all the lib fixes re-flags 9 hazards, restored 0. diagnose 10/10, detect-gpu 6/6, gpu-nvidia 27/27, preflight 144/144, make lint clean.

And thanks for separating what still stands — the gpu-plugins truncation-reads-as-no-GPU and the preflight grep -q inverting a predicate are the two I would have most regretted missing.

[LukasWodka]

bugbot run

[saqlainsyed007]

Strong PR — the diagnosis is exactly right (early-close reader + pipefail + errexit → size-dependent 141), the line-by-line option tracking with set +e regions is the correct model, sharing one implementation between the gate and its self-tests is the right call, and I verified a sample of the five fixes (e.g. e2e-proxy.sh:131 capture-then-grep -q <<<, preflight.sh here-string) — they're careful and each names the consequence. | tail is correctly left unflagged (tail reads to EOF, no SIGPIPE).

Requesting changes on the scanner's coverage, though — this is a completeness tool ("encode the rule so new ones cannot land"), so a gap that lets the exact hazard shape through green is the one defect class that actually matters here. I verified all three against pipefail-early-close.awk:

1. One-line function bodies are never scanned (Bugbot :82, confirmed). The function-def rule matches name() { and unconditionally nexts the whole line, so first() { cmd | head -1; } — a form the PR body itself says this repo uses — is skipped entirely. The gate stays green while that exact early-close lands inside a one-liner. Fix: only next when the line is a bare name() { opener; when body follows the { on the same line, fall through to the hazard check (or detect the single-line name() { … } shape and scan the part after {).

2. | head inside a command substitution is missed (Bugbot :101, confirmed)./\|[[:space:]]*head([[:space:]]|$)/ only fires when head is followed by a space or EOL, so x="$(cmd | head)" (head followed by ) / ") never flags — yet x=$(big | head) is precisely the abort-under-errexit case, since the assignment takes the substitution's 141. Add the closing delimiters to the terminator class, e.g. head([[:space:]]|$|[)"'''])`.

3. Same class, not yet flagged: set -o errexit long form isn't recognized.apply_set handles set -o pipefail (the o$ + next-token branch) and short -e/-eu/-euo, but the errexit long form falls through — -o then errexit sets neither p_on nor e_on. So a file doing set -o pipefail; set -o errexit has p_on=1, e_on=0 and every hazard in it is skipped. Asymmetric with the pipefail handling right beside it; worth closing in the same pass so the matcher isn't spelling-sensitive. A self-test line for each of the three would lock them in.

None of these are in the five you fixed — they're holes in the guard that's meant to stop the sixth. Close them and I'll approve; the rest of the PR is ready.

[LukasWodka]

bugbot run

[LukasWodka]

@saqlainsyed007 — all three closed.

1 (one-line function bodies) and 2 (head before a closing delimiter) were fixed in the commit just before your review landed; you and Bugbot found the same two independently. Worth adding: the test I first wrote for the one-liner was vacuous — the fixture indented the function, and the opener rule is anchored at column 0, so it never took that path and was flagged either way. Mutation caught it; it is now at column 0 where the real case lives.

3 (long-form set -o errexit) was genuinely open and is fixed now. You are right that it was asymmetric with the pipefail handling right beside it: -o carries no e for the short-flag branch, and only pipefail was matched in the long-form branch, so

set -o pipefail
set -o errexit
ls /tmp | head -1 <- skipped

had p_on=1, e_on=0 and every hazard in the file was skipped. The long-form branch now dispatches on the option NAME, so both spellings behave the same and +o disables both.

Per your ask for a self-test line each, tests 23 → 25: a spelling matrix (long/long, long/short, combined short) plus the set +o errexit case. Mutation-proved — removing the long-form errexit arm reddens the matrix.

Tree gate still 0 offenders, 25/25, make lint and hygiene clean. Thanks for framing it as coverage of the guard rather than of the five fixes — that is the right bar for a completeness tool, and points 1 and 3 were both shapes that would have let the sixth one through.

[LukasWodka]

bugbot run

[saadqbal]

Both remaining asks are closed, and the disabling model is implemented more carefully than I asked for. Verified each rather than reading the commits.

Ask 3 — the seed now checks the sign:

 set +o pipefail -> seeded=no (was yes)
set -o pipefail -> seeded=yes

And bats case 21 pins it: "the seed checks the SIGN: a pipefail-OFF sourcer does not make its libs hazardous". Suite is 23 cases now, up from 17.

Ask 2 — set +e regions are no longer flagged:

$ printf 'set -euo pipefail\nf() {\n set +e\n df -h | head -20\n}\n' | awk -f pipefail-early-close.awk
(no output)

The part I actually wanted to check is whether modelling disabling introduced a false negative, because that's the direction that would make this worse than the noise it fixes. It doesn't — both probes behave correctly:

Re-enable after disable — the hazard inside set +e is spared, the one after set -e is caught:

 set +e ; df -h | head -20 -> not flagged
set -e ; lspci | grep -qi nvidia -> /tmp/reenable.sh:6 ✓

Scope — set +e in one function does not leak into the next:

 a() { set +e ; df -h | head -5 } -> not flagged
b() { helm list | grep -q foo } -> /tmp/scope.sh:7 ✓

That second one is the one I'd have expected to get wrong, since tracking per-function option state in awk is fiddlier than the enabling case. Nice.

Scanner still exit 0 on the tree, so nothing regressed into a false negative fleet-wide either.

Nothing outstanding from me. Six checks went back into flight on the latest push, so I can't lift the block yet — I'll approve next pass once they land. Flag me if any go red for a reason that isn't obvious.

[saqlainsyed007]

All three findings addressed, and I've verified each against the new pipefail-early-close.awk:

• One-line functions (2f9f401a) — the function-def rule now only saves state and nexts when the opener is not self-closing (if (line !~ /\}[[:space:]]*$/)), so first() { cmd | head -1; } falls through to the hazard check and is scanned. Correct.
• head before a closing delimiter (2f9f401a) — the terminator class is now ([[:space:]]|$|[)"''';|&]), so x="$(cmd | head)"` matches. Correct.
• Spelling-sensitive matcher (2d241b18) — apply_set now takes the errexit long form (else if (a[i+1] == "errexit") e_on = …) symmetrically with pipefail, so set -o pipefail; set -o errexit is no longer a blind spot. Correct.

Asad's option-disabling case is covered too, and every Bugbot thread is resolved. The fixes all sit in the scanner rather than the five sites — exactly where the gap was.

Only CI is outstanding (Bugbot re-run + prereqs + unit + bats still pending). I'll approve on the next pass as soon as it's green — nothing else left on my side.

[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 2d241b1. Configure here.}

[saadqbal]

Approving. Green (42 passing, 4 skipped, nothing failing or pending), no open threads, suite up to 25 cases, scanner clean on the tree.

The head had moved past what I'd verified, so I re-checked rather than approving on the promise — and both commits since are false-negative fixes, which is the direction that matters for a guard:

• 2d241b1 (Saqlain) — the option matcher was spelling-sensitive.set -o errexit fell through entirely: only pipefail was matched in the -o branch, and -o carries no e for the short-flag branch. So a file doing set -o pipefail; set -o errexit had p_on=1, e_on=0 and every hazard in it was skipped. That's the asymmetry sitting right beside the pipefail handling, and it's a good catch.
• 2f9f401 (Bugbot) — one-line function bodies were skipped, on an unconditional next at the function-open line, in a repo that writes helpers that way. Plus head followed by a closing delimiter.

Verified all three, and re-ran every probe from my earlier passes to check nothing regressed:

 long-form: set -o pipefail; set -o errexit -> FLAGGED (was skipped)
one-line fn body -> FLAGGED (was skipped)
head before a closing delimiter -> FLAGGED (was skipped)
set +e region -> clean ✓
re-enabled after set +e -> FLAGGED ✓
set +e does not leak to the next function -> FLAGGED ✓
pipefail-only, no errexit -> clean ✓

That's the whole matrix behaving. Worth noting the shape of this PR's history, because it's the argument for the guard existing: the guard itself went through four rounds of false negatives — inherited options, disabled options, long-form spellings, one-line bodies — each of which would have let real hazards through silently. A checker for a size-dependent bug is exactly the kind of thing that looks right and isn't, and the only reason those were found is that people kept probing it rather than reading it.

Everything from my earlier passes stands: reverting all four lib files makes the scanner flag exactly the five hazards that were fixed, the inheritance fixpoint is the load-bearing idea, and gpu-plugins.sh — truncation silently becoming "no GPU" on a GPU cluster — is still the best find in the set.

[saqlainsyed007]

Approving. CI is green now, which was the only thing outstanding. All three coverage gaps I requested are fixed and verified against pipefail-early-close.awk (head 2d241b18): one-line function bodies now fall through to the hazard check, the head terminator class covers closing delimiters, and apply_set takes the errexit long form symmetrically with pipefail. Asad's option-disabling case is handled, every Bugbot thread is resolved, and all the fixes landed in the scanner rather than the five call sites — exactly where the gap was. Nice work on a genuinely enforceable guard.

[saqlainsyed007]

Heads up — my approval got dismissed by the develop merge (adc253bf) moving the head, not by any new concern. I diffed the deliverable across the merge and pipefail-early-close.awk, its .sh, and its .bats are byte-identical to what I approved at 2d241b18; the merge brought nothing into them. Just waiting on the re-triggered CI (bats + unit + Bugbot pending) — I'll re-approve as soon as it's green. Nothing for you to do.

[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 9ec4c9f. Configure here.}

[saqlainsyed007]

Re-approving. New commit 9ec4c9ff addresses the trailing-comment edge Bugbot found in the one-liner fix, and I verified it: the self-closing test now runs against a comment-stripped copy (bare, via sub(/[[:space:]]*#.*$/, "", bare)), so first() { producer | head -1; } # why is correctly seen as a one-liner and its body scanned rather than nexted. The new bats case is mutation-proof — reverting the strip empties $output and reddens it. CI green, all Bugbot threads resolved, no open threads, no conflict.

Non-blocking note for later (not asking you to hold the PR): the comment-strip heuristic will over-strip a # that is preceded by whitespace inside a quoted string before the } — e.g. f() { printf '%s' "$x" | grep -q ' #tag'; } strips from #tag'; }, leaving bare not ending in }, so that one-liner is read as a multi-line opener and its grep -q is missed. It's a narrow false-negative (one-line function + hazard + whitespace-#-in-quote), and shell-comment stripping without a quote-state parse is a normal linter approximation — fine to leave, but if it ever bites, the same self-test file is where to pin it. Approving as-is.

[aptracebloc]

Approving after re-review.

The scanner is now genuinely robust and self-guarding, and I verified the current source at 9ec4c9ff against each concern the review surfaced:

• Sourced libraries are in scope (my earlier comment): inherits() seeds files that pick up errexit+pipefail from a sourcer via the hazardous list, with a paired negative test so inheritance isn't "everything."
• docker/ and .bash are covered: the tree gate runs through the repo's own sh-files.sh derivation rather than a hand-rolled find, and the test exercises the gate's scope with a real git fixture — after they caught and fixed their own vacuous first version of that test by mutation.
• Options are tracked line-by-line (Asad): set +e best-effort regions no longer flag every line, and set -o errexit is matched symmetrically with set -o pipefail (Saqlain).
• One-line function bodies are checked, including when a trailing comment follows the closing brace, and | head before a closing delimiter (), ", ;, …) is caught (Bugbot).

CI green, no open threads, no conflict. The five production conversions remain faithful.

— drafted with Claude Code