fix(scripts): retire the remaining early-exit pipe consumers (client#686)

[LukasWodka]

Closes#686.

The sites #680 and #683 did not reach. Same transform every time: capture the producer, match the captured value, so the producer always runs to completion — case where the needle is a fixed substring, which additionally drops the A && B set -e subtlety two of these carried.

I re-derived reachability per site rather than taking the issue's list at face value, and three of its entries do not hold. Those are left alone, with evidence, so the next sweep doesn't re-open them.

The mechanism, measured

Four contexts, measured on this branch rather than reasoned about — they decide which sites are real:

Shape	Under set -euo pipefail	Consequence
v="$(producer | head -1)"	aborts, 141	assignment position propagates
cmd "$(producer | head -1)"	survives	argument position never trips errexit
if producer | grep -q X	misbranches when the match LEADS	141 reads as "no match"
same, match TRAILS	correct	grep must read to EOF — the vacuous-test trap
anything after set +e	survives	errexit is off
function called as if ! f	survives	a condition suppresses errexit for the whole body

Two more that shaped the fixes:

• SIGPIPE needs the producer to write again after the consumer closes. A single-write producer under the 64 KiB pipe buffer is safe; a producer that streams in stdio chunks can lose the race at ~4 KiB. That is why lspci, helm upgrade --help and kubectl get nodes are the genuinely exposed producers and id -nG is not.
• errexit propagates out of a command substitution only on bash ≥ 4.4. The macOS system bash is 3.2, so preflight.sh:100 aborts on Linux (the only place findmnt exists) and not locally — which also made my first test for it vacuous.

Sites changed

Misbranch — inside if/&&, so pipefail's 141 is read as "no match"

Site	Wrong branch does what
lib/detect-gpu.sh:28,34	lspci | grep -qi → GPU_VENDOR stays none on a GPU host → CPU-mode cluster. One capture now serves both probes and the AMD label.
lib/install-client-helm.sh:449	repo believed absent → re-runs helm repo add, which is unguarded on the next line and fails when the name exists with a different URL — the misbranch escalates into an aborted install
lib/install-client-helm.sh:594	loses --reset-then-reuse-values → reconcile silently stops picking up new chart defaults
lib/install-client-helm.sh:835	sticky 8.4 lost → resolves 5.7 against an 8.4 datadir, which MySQL 5.7 will not open
lib/setup-linux.sh:281	docker-group membership misread → redundant usermod/warn
lib/setup-linux.sh:348	membership misread → skips the sg-docker re-exec → dead-ends at "log out and back in" (the #427 loop). Nested, so the two mode guards still short-circuit ahead of id, exactly as the old && did
lib/setup-linux.sh:898	nvidia runtime not detected → CPU-only cluster on a Tier-0 GPU host that already has the toolkit
lib/setup-linux.sh:1151	the capture was already there (Asad #458); this drops the leftover printf | grep -q re-pipe of it

Abort

Site	Wrong branch does what
lib/preflight.sh:100	findmnt | head -1 in an assignment → 141 aborts the installer inside preflight with no message. The sibling mount pipeline two lines down was fixed in #680; this one was missed. Linux-only, per the bash-version note above.

Hardening — shape retired, but it cannot abort today

Called out as such rather than dressed up as field fixes:

Site	Why it is not a live bug
install.sh:538	the sole caller is if ! ensure_cosign, and a condition context suppresses errexit for the whole function, so the 141 is swallowed and want is already correct. Retired anyway: a function sitting in the signature-verification path should not depend on how its caller happens to be written, and one bare call would make it an abort of the bootstrap.
lib/common.sh:262	argument position — a 141 never trips errexit there

Sites deliberately left alone

The issue lists these as bugs; they are not.

Site	Evidence
lib/diagnose.sh:61,96	run_diagnose executes set +e as its first statement (line 42), before every site in the function, and it is only ever entered via install-k8s.sh:117. No site in that function can abort. The support bundle was never at risk — the issue's "would break diagnostics exactly when they are needed" does not hold.
lib/gpu-plugins.sh:112	the trailing || echo "" already guards the abort, and the value survives: head -5 has emitted its five lines into the capture before the SIGPIPE propagates. Measured — the pre-fix pipeline returns 141 while RAW still holds the correct five keys. Changing it would have been churn on a signed-manifest file for no behaviour change.
lib/detect-gpu.sh:22,23,36	argument position inside success/log; proven not to trip errexit, and head -1 captures the right value regardless
lib/preflight.sh:727	producer is the printf builtin emitting a sub-1 KB issuer string — one write, far under the pipe buffer, so SIGPIPE is impossible
lib/common.sh:393	already || true
common.sh:591, preflight.sh:84,106, install.sh:320,322, install-client-helm.sh:78,245,576, diagnose.sh:135	awk without exit, or a tail consumer — all read the stream to completion
common.sh:847 (Darwin ARCH), cluster.sh, assess.sh, setup-macos.sh	owned by the open #683 — untouched to avoid conflicts

Mutation evidence

Every test was run against the pre-fix code. Where a function has fallback probes the whole function was reverted, per the #683 lesson.

Revert	Result
detect-gpu.sh lspci block (both probes + label)	3 of 6 fail — the two detection tests and the AMD-label test. The no-GPU control and the two short-circuit tests correctly still pass.
preflight.sh:100	fails — [ "$status" -eq 0 ], i.e. the abort reproduced
setup-linux.sh:898	2 of 3 fail; the "no nvidia runtime" control correctly still passes
install-client-helm.sh:449 + :835 (both reverted together)	2 of 4 fail — the two bug tests; both "did not invert the verdict" controls pass

Two of my tests were vacuous on the first cut and are worth flagging, because they are the traps this class sets:

1. The preflight test used the production t="$(_pf_fstype …)" shape and passed against unfixed code — errexit does not propagate out of a command substitution on bash 3.2, which is what the suite runs on locally. It now calls the function bare, which asserts the same contract on every bash.
2. The _resolve_chart_ref mock ended in return 0, which masked seq's SIGPIPE; and the _resolve_mysql_engine test ran on arm64, where the fallthrough also yields 8.4 so the misbranch was invisible. Fixed by dropping the mock's return 0 and pinning ARCH=x86_64.

Both are now noted in the test comments so the next person doesn't repeat them. Filler comes from an external command (seq) throughout — a producer built only from bash builtins does not reproduce a real command's SIGPIPE death.

Test plan

• bash scripts/gen-manifest.sh — regenerated; on my own commit exactly 5 hashes changed, matching the 5 edited libs (gpu-plugins.sh unchanged, confirming that revert was clean). install.sh is the bootstrap and is not listed in its own manifest.
• make check — green (style guard, shellcheck, drift, helm lint on 4 value sets + ingestor), before and after the merge below.
• make bats — 937 tests, 0 failures on my commits alone, up from 923 on the develop I branched from (+14: 6 detect-gpu.bats new file, 4 install-client-helm.bats, 3 setup-linux.bats, 1 preflight.bats). 960, 0 failures after merging current develop.
• bash -n clean on all 6 edited scripts; shellcheck reports nothing new (the remaining common.sh warnings are pre-existing and in untouched lines).

#683 merged mid-flight

#683 landed on develop while this was in progress, touching common.sh, setup-linux.sh and manifest.sha256 — all files this PR also edits. develop is merged in here: the libs auto-merged (its _record_err/install_cleanup work and my probe rewrites are disjoint), and the only conflict was manifest.sha256, resolved by regenerating it from the merged file contents rather than hand-picking either side. make check and the full suite were re-run on the merged tree.

scripts/tests/detect-gpu.bats is new — detect-gpu.sh had no suite at all.

One existing test updated

setup-linux.bats' id -nG assertion grepped the source for the literal id -nG "$_grant_user" … | grep -qw docker, which this change removes. It still pins what it was written to pin — that both probes key off $_grant_user and never bare $USER — plus that the captured value is word-matched rather than substring-matched.

🤖 Generated with Claude Code

[LukasWodka]

CI triage — Pester (ubuntu-latest)

The first run after merging develop came back 42 pass / 4 skipped / 1 fail, the failure being Pester (ubuntu-latest). It is not from this diff:

[-] minted mode (#388): no prompts; values carry the minted credential; slug namespace used
SocketException: Broken pipe
MethodInvocationException: Exception calling "Write" with "1" argument(s): "Broken pipe"
at Invoke-BoundedProcess, scripts/install-k8s.ps1:1789
at Invoke-DockerCli, scripts/install-k8s.ps1:1807
at Initialize-ReleaseDataDirs, scripts/install-k8s.ps1:3265
at Install-ClientHelm, scripts/install-k8s.ps1:4726
at <ScriptBlock>, scripts/tests/install-k8s.Tests.ps1:1041

• This branch changes no PowerShell at all.git diff --name-only origin/develop...HEAD is 6 shell scripts, 4 bats files and manifest.sha256 — zero .ps1.
• The manifest.sha256 diff against develop is exactly the five libs this PR edits; install-k8s.ps1's hash is untouched.
• Pester (windows-latest) passed on the same commit, and develop at ef5c159 (fix(installer): say where a step died, and stop calling a stopped runtime a fresh machine (client#681, client#682) #683) passed this job.
• 662 of 663 Pester assertions passed; the one failure is a write to a mocked process whose peer had already exited — a race, and one that only shows up under runner load.

Re-ran the failed job rather than pushing a no-op commit, so the rest of the run is not re-rolled.

Re-run result: green — Pester (ubuntu-latest) passed on re-run with no code change, confirming the flake. Full run is now 43 pass / 4 skipped / 0 fail.

[saadqbal]

Clean, careful sweep. Traced all 10 converted sites: each preserves exact semantics (first-line / match-boolean / first-field via case), every new capture var is local, and answer-correctness holds — a genuine producer failure still lands on empty-then-return 1 or the safe default, same as before, never a silent wrong answer. The re-exec guard staying nested so the two mode checks short-circuit ahead of id is the right call. Manifest matches the five changed libs (recomputed). New bats are non-vacuous — match leads, filler from an external seq, explicit no-inversion cases — and green locally.

One non-blocking follow-up: detect-gpu.sh:22-23 still has nvidia-smi … | head -1 in argument position — exactly the shape you retired in assert_tool_runs here, two branches above the lspci block you did convert. A multi-GPU host emits one line per card so head SIGPIPEs nvidia-smi; it's abort-safe (arg position) and answer-correct, so not a bug, just the one spot in a touched file the fleet-wide sweep skipped. Fine to leave for a follow-up.