Uh oh!
There was an error while loading. Please reload this page.
fix(check-facts,e2e-windows): sed|head pipefail crash + false-PASS pre-clean (Bugbot #542) - #544
Conversation
…PASS pre-clean (Bugbot #542) check-facts.sh: _spec_get and _extract piped `sed -n … | head -1` under `set -o pipefail`. On a second match large enough to fill the pipe buffer, head closes after line 1, sed takes SIGPIPE, and the pipeline exits 141. In _extract the pipeline is the function's terminal command, so that 141 aborts the facts gate before any drift message prints — a crash / fail-open on duplicate input. Both helpers now capture the whole output and take the first line with `${all%%$'\n'*}` (the repo's gpu-nvidia.sh idiom), leaving no pipe to break. Empty (no-match) results still yield empty, so callers are unchanged. e2e-windows.ps1: the pre-clean `k3d cluster delete` ran via `Invoke-Bounded … | Out-Null`, discarding its exit code. On the persistent Windows runner a timed-out (124) or failed delete leaves the tbe2ewin cluster in place; New-K3dCluster then takes its REUSE path, which still logs "Creating k3d cluster", so the copy check + PASS succeed WITHOUT a real create — the exact false-green the pre-clean was added to prevent. The pre-clean now gates on the exit code (k3d delete is idempotent → 0 when absent) and Stop-E2e's the run on non-zero, so a stale cluster can never fall through. Tests: two bats cases in check-facts.bats feed duplicate matches past the pipe buffer and assert --check still passes (the _extract case reproduced exit 141 on the pre-fix code). e2e-windows.ps1 has no Pester suite (it's an integration driver dot-sourcing the real installer against live Docker/k3d), so it relies on the source-level guarantee; PSScriptAnalyzer lints it in CI and still passes. Both findings surfaced by Cursor Bugbot on the release-train PR client#542. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Reviewed and verified locally.
Finding 1 — check-facts.sh: Both _spec_get and _extract now capture the full sed output and take the first line with ${all%%$'\n'*} instead of sed | head -1. Behavior is identical for the single-match and no-match cases, and the pipefail SIGPIPE (exit 141) on duplicate input is gone. Reproduced the old crash locally: sed … | head -1 on a 20k-line duplicate-match file exits 141 under pipefail; the new form exits 0 with the same value. Both #542 regressions in check-facts.bats genuinely reproduce the pre-fix crash — full suite 13/13 green.
Finding 2 — e2e-windows.ps1: The pre-clean now captures Invoke-Bounded's exit code and fails the run via Stop-E2e on non-zero, matching the docker info bounded-check idiom directly above. Invoke-Bounded returns a clean integer (124 on timeout, else $p.ExitCode) and emits nothing else to the pipeline, so $preclean is exactly the exit code — a stale cluster can no longer be silently reused into a false PASS. Best-effort teardown correctly keeps | Out-Null.
All 40 CI checks green including Cursor Bugbot; no open review threads. LGTM.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Aug 3, 2026
/fr-pass |
Fixes two Cursor Bugbot findings raised on the release-train staging-hop PR client#542. Per process, the fix lands on
develop(not on the promotion branch #542) and rides the normal develop → staging → main promotion.Finding 1 —
scripts/check-facts.sh: pipefail broken bysed | head -1(Medium)_spec_getand_extractpipedsed -n … | head -1underset -o pipefail. When a file holds a second match large enough to fill the ~64KB pipe buffer,headcloses after line 1,sedtakes SIGPIPE, and the pipeline exits 141. In_extractthe pipeline is the function's terminal command, so that 141 propagates out ofgot="$(_extract …)"and aborts the facts gate before any drift message prints — a crash / fail-open on duplicate input. (In_spec_geta trailingprintfmasked the status on some shells, but the same fragile pipe was there.)Fix: both helpers now capture the whole
sedoutput and take the first line via parameter expansion —all="$(sed -n … )"then${all%%$'\n'*}— the repo's own house idiom (seescripts/lib/gpu-nvidia.sh). No pipe intohead. An empty (no-match) result still yields empty, so every caller behaves exactly as before. These were the only two| head -1occurrences incheck-facts.sh; the anti-pattern was not spread into unrelated sibling scripts.Tests: two cases added to
scripts/tests/check-facts.batsfeed duplicate matches past the pipe buffer and assert--checkstill passes. The_extractcase reproduced exit 141 against the pre-fix code; the fixed code is green.Finding 2 —
scripts/tests/e2e-windows.ps1: pre-clean failure allows false PASS (Medium)The pre-clean
k3d cluster deleteran viaInvoke-Bounded … | Out-Null, discarding its exit code. On the persistent Windows runner a timed-out (124) or failed delete leaves thetbe2ewincluster in place;New-K3dClusterthen takes its reuse path — which still logsCreating k3d cluster(line 2205, before the exists-check) — so the copy check + PASS succeed without a real create, the exact false-green this pre-clean was added to prevent.Fix: the pre-clean now captures the exit code and fails the run (
Stop-E2e) on non-zero, matching the existingdocker infobounded-check idiom right above it.k3d cluster deleteis idempotent — exit 0 when the cluster is absent — so the clean/first-run path is unaffected, while a timeout or real failure aborts instead of falling through to reuse.Tests:
e2e-windows.ps1has no Pester suite — it is an integration driver that dot-sources the real installer and runs against live Docker/k3d, so it cannot be unit-tested in isolation; it relies on the source-level guarantee. PSScriptAnalyzer lints it on every push (CIstaticjob) and still reports no errors.Verification (local)
bats scripts/tests/*.bats— 782 pass (13/13 in check-facts.bats, incl. the 2 new regressions)bash -n scripts/check-facts.sh+scripts/check-facts.sh --check— greenpwshparse ofe2e-windows.ps1— OK; PSScriptAnalyzer Error severity — clean (only pre-existing Write-Host/BOM warnings, unchanged)scripts/manifest.sha256, so no manifest regeneration neededNote
Low Risk
Changes harden test/CI gates and an e2e driver; no runtime installer behavior for end users beyond more reliable checks.
Overview
Addresses two Bugbot findings on CI/installer reliability.
check-facts.sh:_spec_getand_extractno longer usesed … | head -1underset -o pipefail. They capture fullsedoutput and take the first line with${all%%$'\n'*}(same pattern asgpu-nvidia.sh), so duplicate matches past the pipe buffer cannot SIGPIPE the script and abort the facts gate before drift is reported.check-facts.batsadds two regressions that append ~20k duplicate spec/consumer lines and assert--checkstill succeeds.e2e-windows.ps1: The boundedk3d cluster deletepre-clean now honors its exit code and callsStop-E2eon timeout (124) or failure instead of piping toOut-Null, so a stale cluster cannot be silently reused and produce a false PASS.Reviewed by Cursor Bugbot for commit 670a6be. Bugbot is set up for automated code reviews on this repo. Configure here.