test(bats): make every assertion enforce — 1240 were advisory - #527
Merged
Conversation
Under Bats (verified 1.13.0) only the LAST command in a test body decides the result, so a failing assertion anywhere earlier is silently ignored: @test "middle failure ignored" { [[ "abc" == *"zzz"* ]] # FALSE [[ "abc" == *"abc"* ]] # TRUE (last) } # -> ok This suite is written multi-assertion throughout, so most assertions could not fail their test. Appending `|| return 1` makes them enforce. Scope is about twice what it first looked. It is not only `[[ ]]`: single-bracket `[ ... ]` has identical semantics and there are MORE of them (609 vs 574), plus 61 negated bare commands. 1240 assertions across 15 files — setup-linux.bats 296, cluster.bats 153, install-client-helm.bats 146, common.bats 118, preflight.bats 102, and the rest smaller. Only whole-line assertions INSIDE an @test body are touched. Helpers and setup/teardown are excluded (a bare `return` there means something different), as are the 9 control-flow `if/while` conditions and 18 lines already chained with && / ||. All files still parse (bats --count), no control-flow line was modified, and nothing was double-appended. TRIAGE RESULT: zero new failures. All 1240 were already true — the suite was accidentally correct, so there was no hidden-bug vs stale-assertion split to report. No assertion was deleted or weakened to reach green. That result only means something if the hardening has teeth, so it was proven rather than assumed. cluster.bats's "_augment_no_proxy: empty host NO_PROXY" asserts 7 substrings and only enforced the last. Deleting `localhost` from TB_NO_PROXY_DEFAULTS — the entry that keeps a corporate proxy from intercepting loopback — is a real regression, and: mutated source + ORIGINAL tests -> ok (invisible) mutated source + HARDENED tests -> not ok (caught) Guard, so the pattern cannot come back: scripts/tests/bats-hygiene.bats plus a shared scanner, scripts/tests/unenforced-assertions.awk (one implementation, used by the guard and by its own self-tests). Three tests: the suite is clean; the scanner flags an un-hardened assertion and spares a hardened one; and it ignores control flow, chained lines, helpers and HEREDOC BODIES. That last exclusion is not cosmetic — the first version flagged its own fixture, which would have made any future test embedding example bats source a false positive. The guard was mutation-tested against the real suite too: un-hardening one line in cluster.bats makes it fail, naming the exact file:line. Gates: bats scripts/tests/*.bats -> plan 693, ok 693, not ok 0 (complete TAP run, plan line checked — a truncated read can look green while half the suite never reports); shellcheck --severity=error over the CI file set -> rc=0; check-style clean; check-drift no drift; gen-manifest.sh --check current (only tests changed, and tests are not part of the hashed set). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ns (Bugbot #527) Two Bugbot findings on the hygiene guard this PR introduces. Both real: the guard could report "suite is clean" while assertions stayed advisory — the exact failure mode the PR exists to close. Measured semantics first (bats 1.13.0, bash 3.2 system bash), because the old header's "only the LAST command decides" was too broad. Bats does run bodies under errexit; exactly two classes escape it: [[ ... ]] bash 3.2 (macOS system bash) does not fire errexit for a failing conditional expression — a middle one is ignored ! cmd POSIX: a status inverted with '!' is never propagated, so this escapes on EVERY bash, CI included grep -q ... a plain bare command DOES fail the test — correctly not reported 1) Scanner skips internal-OR assertions — REAL. `[[ a || b ]]` is ONE assertion whose ||/&& is internal; it exits non-zero on failure like any other and needs `|| return 1` too. The scanner skipped every line merely CONTAINING ||/&&, and only matched `[`/`[[` that closed on the same line, so it missed both single-line internal-OR and multi-line forms. Rewritten to build a logical line (trailing backslash, or a newline inside the brackets) and to locate the closer that matches the opener, so only a TOP-level chain earns the exemption: `[[ a ]] || fail` still skipped, `[[ a || b ]]` flagged, and `||` appearing only inside a quoted grep pattern no longer hides an assertion. Multi-line offenders are reported joined, at their first line. Six offenders it now catches (Bugbot named two; four are the same class): install-bootstrap.bats:144 and :154 — mid-body, so genuinely advisory — common.bats:179, install-client-helm.bats:887, preflight.bats:542, summary.bats:73. All six now end in `|| return 1`. 2) Guard omits negated bare commands — REAL. The PR hardened 61 `! cmd` assertions but the guard did not cover the class, so a later unhardened one would pass unnoticed — and this is the class that is advisory on every bash, not just 3.2. The scanner now flags standalone `! cmd ...`, while sparing `! cmd || return 1`, `if ! cmd`, bare `cmd`, and `run ! cmd`. Zero live offenders: the 61 are all hardened. Failing-test-first evidence, both directions verified by flipping the change: - two new bats-hygiene tests (internal-OR incl. both continuation styles and a pattern-only `||`; negated bare commands) fail on the old scanner, pass on the new one, and assert the spared cases so the scanner cannot over-report - with the new scanner and the un-hardened files, the "suite is clean" test fails and names all six offenders - end to end on real code: blanking install.sh's "not an immutable release tag" message left install-bootstrap.bats's two path-traversal tests GREEN before the fix and fails both after — an R8 regression the suite had been ignoring Also made the scanner portable (\b and `close` are not safe in every awk) and corrected the guard's header to the measured semantics. Local: bats scripts/tests/*.bats 695/695, shellcheck --severity=error clean, bash -n clean, gen-manifest.sh --check up to date, check-style.sh clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI went red on the merge commit, not on this branch. Diagnosis first, because the answer changes the fix: #530 ("chart-guard: cover every published chart") merged scripts/tests/ chart-version-guard.bats into develop at 2026-07-31T15:59Z — 40 minutes AFTER this branch's last green Installer-tests run (30642417378, head 91a5fdd, 15:19Z). The file was written before this convention existed, so all 46 of its standalone assertions are bare. GitHub tests refs/pull/527/merge, so the guard correctly reported them. NOT caused by the scanner rewrite. The OLD scanner, exactly as shipped in 91a5fdd, flags the same 46 lines on that file — byte-identical output: awk -f <91a5fdd's scanner> chart-version-guard.bats | wc -l -> 46 awk -f <new scanner> chart-version-guard.bats | wc -l -> 46 diff of the two -> identical They are all plain single-line `[ ... ]` / `[[ ... ]]`, none of the classes this PR's rewrite added. So the branch head would have gone red on the same merge commit with or without my commit — this is develop drift meeting a guard that only just started existing, which is the guard doing its job on the first file that arrived after it. Fix: merge develop and append `|| return 1` to the 46. No assertion reworded, deleted or weakened; the guard is not relaxed to accommodate the new file. Local, post-merge: bats scripts/tests/*.bats 718/718, scanner reports 0, shellcheck --severity=error clean (incl. the new chart-version-guard.sh), gen-manifest.sh --check up to date, check-style.sh clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ugbot #527) REAL, and the most serious of the three findings — it made the guard lie. The heredoc detector matched `<<TAG` anywhere on a line, including inside a quoted string. Once tripped, it looked for a BARE terminator line that never comes, so the scanner silently ignored every remaining line in that file while the "suite is clean" test still reported clean. Proven on the real file, not just in theory. Appending an unhardened assertion to the end of bats-hygiene.bats — after its own `printf " cat > f <<'EOF'\n"`: awk -f unenforced-assertions.awk bats-hygiene.bats -> NO OUTPUT (invisible) bats bats-hygiene.bats -> ok 1 ... assertion ... ends in '|| return 1' The guard reporting clean while a bare assertion sits in the file it is scanning is the worst failure this PR could ship, since every other claim in the PR rests on that scan. A SECOND live instance Bugbot did not name: `<<<` herestrings. The regex matched from the second `<` of `run guard_leftover_data <<< "r"`, taking tag `r`, so leftover-guard.bats was swallowed from line 131 onward — the same canary appended there was equally invisible. Bugbot's Additional Locations listed only bats-hygiene.bats#L135-138. Fix, three parts: - `quoted_at()` walks shell quoting state, so a `<<TAG` inside '...' or "..." is text, not a redirection - `<<` immediately preceded by `<` is a herestring, not an opener - safety valve: an @test at column 0 ends heredoc-skip mode, so no future mis-detection can ever hide more than one test's worth of lines Real heredocs still skip their bodies: 12 genuine openers across the suite are still detected, and the pre-existing "ignores ... heredoc bodies" test fails if the tracking is deleted rather than fixed — so "stop tracking heredocs" cannot pass as a fix. Verified by disabling it: that test flips to not ok. Worth recording that the suite-clean scan CANNOT catch that regression (no real heredoc body in the suite contains a bare bracket line), so the fixture test is the only guard on it. Two new tests, flipped in both directions: old scanner -> not ok 5 (expected line 3 to be flagged) not ok 6 (expected line 6 to be flagged) new scanner -> ok 5, ok 6 Each fixture carries several distinguishable entries and asserts the exact offender count plus the spared lines, so neither can pass by over-reporting or by a fixture too small to tell an anchored rule from a loosened one. Local: bats scripts/tests/*.bats 720/720, scanner reports 0, shellcheck --severity=error clean, gen-manifest.sh --check up to date, check-style.sh clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # scripts/tests/preflight.bats
…hygiene) Merging develop brought in test files/tests added after this branch forked (check-facts.bats, index-invariants.bats, setup-macos-lifecycle.bats, and new preflight.bats cases) whose standalone assertions were written in the bare, advisory form. bats-hygiene.bats — the enforcing-assertion guard this PR adds — correctly flagged 134 of them. Append `|| return 1` to each so every assertion can fail its test, exactly as this PR does across the rest of the suite. Mechanical: `|| return 1` inserted before any trailing inline comment; negated bare commands (`! grep …`) get the same enforcing form. Verified by re-running the scanner to zero offenders and the full bats suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Brought this up to date with 1. Conflict — 2. Hygiene follow-up — develop had since added test files/tests this branch never saw ( One local-only note: 🤖 pushed by Claude Code on request |
… (Bugbot) The enforcing check was a line-wide substring match for `|| return 1`, so an assertion that merely MENTIONED the marker was treated as hardened though it does not enforce: `[[ "$output" == *"|| return 1"* ]]` (marker inside a quoted pattern) or `[[ "$x" == y ]] # ... || return 1` (marker only in a trailing comment) slipped through the guard (Cursor Bugbot, Medium). Add `strip_comment` (drop an unquoted trailing comment) + `is_enforcing` (require a `|| return 1` that is outside quotes and outside the comment), reusing the existing quote walker. New bats-hygiene self-test proves both fooling shapes are flagged and a real top-level `|| return 1` is still spared. Whole-suite sweep still reports 0 offenders, so the 134 conversions in the prior commit remain correctly recognized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
bugbot run |
…it revealed (Bugbot) The unenforced-assertions scanner classified a line as a bracket assertion only when it OPENED with `[[`/`[`, so a mid-line assertion — `run x; [[ ... ]]`, the last command of a compound line — was invisible. On bash 3.2 that `[[` still cannot fail the test, so 107 such assertions across preflight/check-drift/setup-* were advisory: the exact failure mode this PR closes. - Scanner: check the last `;`-segment of a compound line for a standalone bracket assertion or negated bare command (`last_segment` + `classify`). Quote-aware; `bracket_tail` distinguishes an internal `||` from a real top-level chain. - Harden the 107 revealed assertions (append `|| return 1`, before any trailing comment). No test logic changed — 304 suite tests still pass, 0 failures. - bats-hygiene.bats: regression test for the compound-line case. The other Bugbot findings on this PR (internal-OR, negated-bare, substring, false-heredoc) were already handled by earlier commits; this closes the last one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Addressed in
Scanner is clean across the suite, the hygiene guard passes, and I re-verified the remote hadn't moved before pushing. |
Contributor
Author
|
bugbot run |
# Conflicts: # scripts/tests/install-bootstrap.bats
… semicolons (Bugbot)
The hygiene scanner reported "clean" while missing real unhardened
assertions in three shapes Bugbot flagged:
- a nested `name() { ... }` stub's column-0 `}` ended the @test scan
early (check-drift.bats had an unhardened `[ "$_drift" -ge 1 ]`
after a helm() mock) -> track brace DEPTH, not the first `}`.
- one-line `@test "x" { run ...; [ ... ]; }` bodies were consumed as a
bare opener and never scanned (common.bats had two) -> scan the
inline body after the opening `{`.
- an assertion that is not the LAST statement of a compound/one-line
body -> classify each `;`-separated statement (subsumes the earlier
last_segment hack, more correctly); paren-aware so a `;` inside a
`( )`/`$( )` does not split a hardened `! ( a; b ) || return 1`, and
comment-aware so a `;` inside a trailing comment is not split either.
Hardens the 26 assertions the improved scanner then surfaced:
cluster.bats / assess.bats and the new #542/#547 check-facts tests (all
pulled in by the develop merge), plus check-drift.bats and the two
common.bats one-liners. Adds 3 regression tests (nested braces, one-line
bodies, subshell `;`).
Full suite 804/804; hygiene 12/12; scanner clean.
Contributor
Author
|
Both new Bugbot findings were real (the guard reported "clean" while missing real offenders) — fixed at the root rather than patched per-pattern, in Scanner (
Suite
Verify: full suite 804/804, hygiene 12/12, scanner clean on |
Contributor
Author
|
bugbot run |
…ne multiline brackets (Bugbot)
Two more scanner gaps Bugbot flagged on the rewrite, both real:
- the `||`/`&&` "already chained" exemption matched the operator
anywhere in the statement, including inside a quoted pattern
(`! grep -q "a||b" f`) or a `( )` subshell -> an unhardened negated
command was silently treated as chained. Now a quote- and paren-aware
top-level scan (`has_toplevel_chain`).
- `bracket_open` only saw a continued `[[`/`[` at the START of the
logical line, so a bracket opening mid-line (`run x; [[ a ||`
continued onto the next line) was never joined -> a multi-line
compound bracket stayed invisible. Now also checks the last
`;`-segment.
Adds 2 regression tests. Full suite 806/806; hygiene 14/14; scanner clean.
Contributor
Author
|
Both new findings were real — fixed in
Added regression tests 13 (quotes/subshell chain) and 14 (mid-line multiline bracket). Full suite 806/806, hygiene 14/14, scanner clean. The other three threads in the last pass (internal-OR, negated bare command, mid-line compound) are already covered by hygiene tests 3/5/6 and the scanner handles them — re-running Bugbot to reconcile. |
Contributor
Author
|
bugbot run |
The hygiene gate went red on its own merge commit: develop gained gpu-nvidia.bats (2 advisory assertions) and the #582 network-profile block in preflight.bats (22 more) after this branch's sweep. Same mechanical treatment -- append `|| return 1`, comments preserved in place. Scanner reports 0 offenders; gpu-nvidia, preflight and bats-hygiene suites pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Aug 4, 2026
The 20 assertions this PR adds to cluster.bats and install-bootstrap.bats were written multi-assertion without `|| return 1`, so under bats only the last command decided each test -- a regression in the cosign/helm/git CA wiring would have passed silently. Same hardening #527 applies suite-wide; appending it here keeps this PR green under #527's bats-hygiene gate whichever lands first. Both suites pass with enforcement on. The Pester additions need nothing: Should throws, so every assertion already enforces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
is_enforcing scanned for an unquoted `|| return 1` anywhere in the statement, so `! ( cmd || return 1 )` was spared — but that return only exits the subshell while the `!` still escapes errexit, leaving the statement advisory. Rewritten on the same quote+paren walker as has_toplevel_chain: only a top-level `|| return 1` counts. Fixture pins both subshell shapes (`( )` and `$( )`) flagged and both top-level shapes spared; the full-suite scan stays clean, so no real assertion was relying on the loophole. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Fixed the Bugbot finding: bugbot run |
heredoc_tag_of was quote-aware but not comment-aware, so a trailing comment DOCUMENTING heredocs opened skip mode with no terminator coming and the rest of the @test body was silently swallowed — live in this very suite, where bats-hygiene.bats comments mention <<TAG. Scan the comment-stripped line; strip_comment returns a prefix, so positions stay aligned for the quote and herestring look-arounds. Fixture pins: comment-mention doesn't skip, a real heredoc still does, and scanning resumes after its terminator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Fixed: bugbot run |
LukasWodka
added a commit
that referenced
this pull request
Aug 4, 2026
… curl (#583) (#592) * feat(installer): wire the corporate CA into cosign/helm/git, not just curl (#583) Child 2/4 of #578 — the single biggest lever for TLS-inspecting corporate networks. A break-and-inspect proxy re-signs HTTPS with a corporate root CA; tools that don't trust that root fail x509. curl already honored CURL_CA_BUNDLE and the k3d NODES got the CA at cluster-create (#424), but cosign, helm and git got nothing — the class behind both field failures (the k3d-checksum and the cosign/sigstore x509 failures). Extend the SAME resolved CA to every host tool that doesn't inherit the system store: - Bootstrap (install.sh / install.ps1): export SSL_CERT_FILE from TRACEBLOC_CA_BUNDLE / CURL_CA_BUNDLE before cosign runs, so keyless verification's HTTPS calls trust the corporate CA (cosign's Go client reads SSL_CERT_FILE). Not manifested (trust root). - Main installer: wire_ca_trust (bash) / Set-ToolTrust (PS) export SSL_CERT_FILE + GIT_SSL_CAINFO (+ CURL_CA_BUNDLE) from the resolved bundle, run BEFORE preflight's probes and any download, so helm, git and curl all trust it. Plain-language line: "Trusting your company's certificate for cosign, helm, git and downloads." No-op when unconfigured; fails fast on a set-but-unreadable bundle. When no CA is provided, tools fall back to the system store (which enterprise IT usually populates) — no user knowledge of CAs required in that common case. Tests: bats (wire_ca_trust exports/no-op/hard-fail; bootstrap cosign sees SSL_CERT_FILE via a recording mock) + Pester (Set-ToolTrust exports/no-op; bootstrap sets SSL_CERT_FILE). cluster.sh / install-k8s.* are manifested; manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): don't re-export CURL_CA_BUNDLE when wiring the corporate CA (#583) Bugbot (Medium): _bootstrap_wire_ca / wire_ca_trust / Set-ToolTrust exported CURL_CA_BUNDLE derived from the resolved bundle. But CURL_CA_BUNDLE is replace-not- augment, and TRACEBLOC_CA_BUNDLE is typically a corp-root-ONLY PEM (its documented k3d-node use), so re-exporting it could REPLACE curl's working trust with a bundle missing the public roots — breaking the manifest/sig fetches that were succeeding via the system store. The PowerShell bootstrap already set only SSL_CERT_FILE. curl already honors the user's own CURL_CA_BUNDLE natively, so we never re-export it. We only wire the tools that had NO corporate trust before: cosign/helm/Go (SSL_CERT_FILE) and git (GIT_SSL_CAINFO). This also makes the bash and PS bootstraps symmetric (both set only SSL_CERT_FILE). Adds a regression test asserting wire_ca_trust leaves a pre-set CURL_CA_BUNDLE intact while still exporting SSL_CERT_FILE for the other tools. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): SSL_CERT_FILE is Linux-only for Go; be honest on Win/mac + fail fast (#583) Two Bugbot follow-ups: - [High] SSL_CERT_FILE is inert for cosign/helm on Windows AND macOS. cosign/helm are Go; Go reads SSL_CERT_FILE only on Linux — on Windows it uses the certificate store and on macOS the Keychain, ignoring the env var. My comment wrongly claimed modern Go honors it on Windows. Corrected: * Linux: keep SSL_CERT_FILE (effective) + GIT_SSL_CAINFO; announce cosign/helm/git. * macOS (wire_ca_trust): still set the vars but announce only git + downloads, and hint that cosign/helm read the Keychain (add the CA there, or use the offline path). * Windows (Set-ToolTrust): set GIT_SSL_CAINFO (Git-for-Windows is OpenSSL-backed); do NOT set SSL_CERT_FILE (inert/misleading); hint cosign/helm read the cert store. * Windows bootstrap (install.ps1): drop the inert SSL_CERT_FILE set entirely. The robust cross-platform cosign fix for a PEM-only CA is the offline bundle (#584). - [Med] A set-but-unreadable CA bundle now fails fast with a clear "can't be read" message in both bootstraps, instead of silently no-opping and surfacing later as a generic cosign authenticity error. Tests updated for the platform-accurate behavior: Linux vs macOS announce, Windows sets only GIT_SSL_CAINFO (not SSL_CERT_FILE) + store hint, and the bootstrap fail-fast on a bad CA path (bash + PS). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): don't clobber a pre-set SSL_CERT_FILE / GIT_SSL_CAINFO either (#583) Bugbot (Medium): wire_ca_trust / Set-ToolTrust set GIT_SSL_CAINFO unconditionally from the resolved (corp-root-only) bundle. GIT_SSL_CAINFO is replace-not-augment (same OpenSSL contract as CURL_CA_BUNDLE), so a fuller pre-set git CA bundle got overwritten and host git HTTPS could x509-fail on non-intercepted endpoints. The same applies to SSL_CERT_FILE. Apply the consistent rule everywhere we wire trust: only set a trust var the user hasn't already set — never override their existing bundle. Covers SSL_CERT_FILE (bash bootstrap + wire_ca_trust) and GIT_SSL_CAINFO (wire_ca_trust + Set-ToolTrust); curl's CURL_CA_BUNDLE was already left untouched. Adds regression tests (bash + Pester) that a pre-set SSL_CERT_FILE / GIT_SSL_CAINFO survives while an unset one is still wired. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): don't over-claim download trust; check CA readability on Windows (#583) Two Bugbot follow-ups: - [Med] Set-ToolTrust printed "…and downloads" trust the corporate CA, but on Windows downloads use the certificate store (Invoke-WebRequest/Schannel) which this path never configures — only GIT_SSL_CAINFO. Green message, still-failing fetch. The announce now names only what's actually wired (git), and the store hint covers cosign, helm AND the installer's downloads. Same over-claim dropped on Linux/macOS: curl "downloads" trust the user's own CURL_CA_BUNDLE (which we deliberately don't touch), so Linux announces "cosign, helm and git" and macOS "git" only. - [Med] The Windows bootstrap CA fail-fast only tested existence (Test-Path -PathType Leaf); a present-but-unreadable file slipped through to a generic cosign error. It now also opens the file (mirrors bash -r and Resolve-CaBundle) and fails fast with a clear "can't be read" message. Tests updated: Linux/macOS announce wording (no "downloads"), and the Windows success line names only git while the store hint covers downloads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#583): make the new CA-wiring assertions enforce The 20 assertions this PR adds to cluster.bats and install-bootstrap.bats were written multi-assertion without `|| return 1`, so under bats only the last command decided each test -- a regression in the cosign/helm/git CA wiring would have passed silently. Same hardening #527 applies suite-wide; appending it here keeps this PR green under #527's bats-hygiene gate whichever lands first. Both suites pass with enforcement on. The Pester additions need nothing: Should throws, so every assertion already enforces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#583): on macOS, wire nothing — Keychain guidance instead (Bugbot ×2) Two Darwin holes, same root: exporting trust vars the platform ignores. - SSL_CERT_FILE: Go reads the Keychain on macOS, so the export helped neither cosign nor helm — while OpenSSL-backed curl DOES honor it, replace-not- augment, so a corp-root-only bundle shrank download trust for zero gain. Dropped from wire_ca_trust and platform-gated in _bootstrap_wire_ca (readability fail-fast still runs everywhere). - GIT_SSL_CAINFO: Apple's system git (SecureTransport) ignores it, and the clone that matters most — Homebrew's own bootstrap — runs system git. The "Trusting your company's certificate for git" claim was false on Darwin. Darwin now exports neither var and prints one honest hint: add the CA to the login Keychain so git, cosign and helm trust it. Same decision, same reason as Windows (store-based trust; no inert claims). Tests: Darwin announce updated, Darwin exports-nothing pinned at both layers, and the Linux bootstrap test now stubs uname so it doesn't flip on a macOS dev machine. Manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#583): say only what was actually wired (Bugbot) wire_ca_trust and Set-ToolTrust printed the green "Trusting your company's certificate…" even when every only-if-unset guard skipped its export -- claiming wiring that did not happen, and masking a pre-set bundle that may still lack the corporate CA. Both now track wired vs kept per variable: the success names only what was actually exported, and anything kept gets an explicit "make sure that bundle includes your company's CA" hint instead. Pinned on both layers: both-kept claims nothing, partial pre-set claims only the wired half (bats), skipped export claims nothing (Pester). Manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Lukas Wuttke <lukas@tracebloc.io>
Contributor
Author
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ 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 a2e6ff4. Configure here.
shujaatTracebloc
approved these changes
Aug 5, 2026
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the full advisory→enforcing conversion via a 1:1 removed-vs-added reconciliation across all 20 modified .bats files: every removed assertion core reappears verbatim as an added core — the change is purely … || return 1 appended, with no logic inversions (no ==↔!= flips, nothing made trivially true) and no dropped coverage. The new unenforced-assertions.awk hygiene gate genuinely fails on offenders and ships 15 self-tests proving it's non-vacuous. Faithful, mechanically-correct hardening. CI green, Bugbot clean. LGTM.
shujaatTracebloc
pushed a commit
that referenced
this pull request
Aug 5, 2026
…eqs hang (#593) * fix(ci): bound the two unbounded network waits behind the ubuntu Prereqs hang Three times on 2026-08-04 (#525, #592) the "Prereqs — ubuntu:*" matrix jobs died at the 20-minute job timeout with nothing in the log but "Installing Docker…", and once more failed in 20 seconds with a registry-1.docker.io timeout (exit 125). Two unbounded waits, one per layer: - Workflow: `docker run` pulls the distro image implicitly with no timeout, so Hub connectivity trouble either failed fast (exit 125) or stalled the whole job. Both container-matrix jobs (distro-prereqs, path-persist) now pre-pull with three bounded attempts (timeout 300 + backoff) and an honest "runner-to-registry connectivity, not this PR" error. - setup-linux.sh: the get.docker.com convenience script's internal apt/download.docker.com fetches carry no timeout, so a stalled connection hung silently behind the spinner. The run is now bounded at 10 minutes (healthy installs take 1-3) and fails with a clear stalled-download message telling the operator to re-run; the fetch of the script itself already had retry + curl_secure timeouts. Same shape as the existing dpkg-lock and kubectl-fetch bounds. New bats test pins the timeout bound on the get.docker.com branch (hardened with || return 1 for the incoming #527 hygiene gate). Manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(ci): distinguish a stall from a real failure; keep the pull budget small (Bugbot ×2) - setup-linux.sh: `if ! spin_cmd …; then error "stalled 10 minutes"` fired on ANY failure, mislabelling a fast real apt/script error as a stall — and it bypassed the existing spin_cmd_bounded helper, which returns 124 only on the deadline and tails the log on every failure. Switched to it: rc 124 gets the stalled-download message, any other rc gets an honest install-failed message pointing at the log tail. Harness gains a default spin_cmd_bounded mock; the bats test now pins the helper + its 600s bound. - installer-tests.yaml: three timeout-300 attempts + backoff could eat ~16 of the job's 20 minutes, so a late-succeeding pull just moved the death from the pull to the install. Bounds resized (3 × timeout 90, 10/20s backoff, ~5.5 min worst case) so the job keeps most of its budget; a healthy pull takes seconds. Manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): prepare-host gets prepare-host re-run advice (Bugbot) The new get.docker.com stall/failure errors always said "re-run the installer" — but with TB_PREPARE_HOST_MODE set that points an admin at a full provision as themselves, the exact outcome prepare-host exists to prevent. Pick the re-run verb by mode, matching the daemon-check errors later in the same function. Manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Aug 5, 2026
…ine resolver (Bugbot) The auto branch treated a failed ls -A as an empty datadir — on arm64, --reuse-data commonly leaves a uid-999 mysql dir the host user cannot list, so the resolver opted the reuse into 8.4 and the format guard then (correctly) refused the 5.7 datadir: the reuse path never came up. An unlistable dir now counts as content (mirrors _leftover_data_dirs' fail- closed stance for the same ownership case), with a chmod-000 regression test. Rebased over #593/#527/#525 (manifest regenerated; my bats negations now carry the #527 '|| return 1' enforcement idiom). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shujaatTracebloc
added a commit
that referenced
this pull request
Aug 5, 2026
The bats-hygiene gate (#527) flagged the four bare `[[ … ]]` assertions in the two #585 preflight tests as advisory — under bats a non-final bare test can't fail its @test. Append `|| return 1` so they actually enforce. Fixes the "Unit tests" + "bats (bash unit, mocked)" CI failures on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Aug 5, 2026
…hygiene guard bats-hygiene's scanner requires every standalone bracket assertion in an @test body to end in '|| return 1'; the resolver tests added on this branch predated rebasing onto that guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shujaatTracebloc
added a commit
that referenced
this pull request
Aug 5, 2026
…r/offline (#585) (#601) * feat(installer): detect a blocked container registry + guide to mirror/offline (#585) Child 4/4 of #578, first slice: detection + clean guidance (the acceptance's core; the mirror/air-gap mechanisms + the offline bundle are follow-on slices). Some sites hard-block Docker Hub / GHCR outright — the images aren't reachable directly at all (distinct from a proxy or TLS-inspection). The preflight connectivity check already probes the registry hosts; now, when the blocked hosts are specifically the CONTAINER REGISTRIES, the installer surfaces the mirror / offline options in plain language instead of leaving only the generic egress hint — and it stays a clean preflight stop, never a raw pull failure (builds on #576/#577/#582). - preflight.sh: after the connectivity hints, if any failed critical is a registry host (registry-1.docker.io / auth.docker.io / ghcr.io), print mirror/offline guidance pointing at docs/INSTALL.md. - install-k8s.ps1: same, via a $regBlocked flag in Test-Preflight. - docs/INSTALL.md: new "Blocked container registry (mirror / air-gapped)" section — point the install at a reachable mirror via TRACEBLOC_VALUES_FILE overriding images.*.registry (+ dockerRegistry creds), or an air-gapped bundle for fully offline sites, with the honest limit stated. Tests: bats — the registry-block guidance fires when a registry is blocked and does NOT fire when only a non-registry host fails; Pester — Test-Preflight carries the detection + guidance + docs pointer. preflight.sh + install-k8s.ps1 are manifested; manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#585): enforce the new registry-guidance assertions (|| return 1) The bats-hygiene gate (#527) flagged the four bare `[[ … ]]` assertions in the two #585 preflight tests as advisory — under bats a non-final bare test can't fail its @test. Append `|| return 1` so they actually enforce. Fixes the "Unit tests" + "bats (bash unit, mocked)" CI failures on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Aug 5, 2026
…ine resolver (Bugbot) The auto branch treated a failed ls -A as an empty datadir — on arm64, --reuse-data commonly leaves a uid-999 mysql dir the host user cannot list, so the resolver opted the reuse into 8.4 and the format guard then (correctly) refused the 5.7 datadir: the reuse path never came up. An unlistable dir now counts as content (mirrors _leftover_data_dirs' fail- closed stance for the same ownership case), with a chmod-000 regression test. Rebased over #593/#527/#525 (manifest regenerated; my bats negations now carry the #527 '|| return 1' enforcement idiom). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Aug 5, 2026
…hygiene guard bats-hygiene's scanner requires every standalone bracket assertion in an @test body to end in '|| return 1'; the resolver tests added on this branch predated rebasing onto that guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Aug 6, 2026
…r format guard (backend#723 PR-2) (#597) * feat(mysql): A2 engine split — 8.4 opt-in for fresh installs + datadir format guard (backend#723 PR-2) Chart: mysql-format-guard init container fails fast (with an actionable message) when the engine major and datadir format disagree — 8.4 over a 5.7-format datadir and 5.7 over an 8.x one are both refused before mysqld CrashLoops; the 8.0 transit hop and custom digest pins stand down. tracebloc.mysqlEngineMajor derives the expected engine (digest-wins, mirroring tracebloc.image); the 5.7 digest literal is CI-pinned to the values default. Default render changes by exactly the guard. Installer (A2, decision 2026-08-05): _resolve_mysql_engine picks the engine for the generated values — explicit TB_MYSQL_ENGINE wins; a previous 8.4 opt-in is sticky; any existing release or real datadir content pins 5.7; only a fresh arm64 install auto-selects 8.4 (native multi-arch instead of amd64 emulation). amd64 fresh installs stay 5.7 for now (soak first). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(installer): fail CLOSED on an unlistable mysql datadir in the engine resolver (Bugbot) The auto branch treated a failed ls -A as an empty datadir — on arm64, --reuse-data commonly leaves a uid-999 mysql dir the host user cannot list, so the resolver opted the reuse into 8.4 and the format guard then (correctly) refused the 5.7 datadir: the reuse path never came up. An unlistable dir now counts as content (mirrors _leftover_data_dirs' fail- closed stance for the same ownership case), with a chmod-000 regression test. Rebased over #593/#527/#525 (manifest regenerated; my bats negations now carry the #527 '|| return 1' enforcement idiom). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(bats): harden the 11 new engine-resolver assertions per the #527 hygiene guard bats-hygiene's scanner requires every standalone bracket assertion in an @test body to end in '|| return 1'; the resolver tests added on this branch predated rebasing onto that guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tests): restore the brace the #604 merge seam ate (last mirror test) git hoisted the shared closing brace out of the conflict region during the rebase onto #604; the file then died at parse (1 of 88 tests ran). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(chart): the format guard honors global.imageRegistry (Bugbot) Semantic rebase conflict with #604: every other image include gained the mirror dig while the guard (written pre-#604, merged clean textually) kept a hardcoded docker.io — on mirrored/air-gapped edges the always-on guard alone would ImagePullBackOff and block mysql on exactly the fleets #604 serves. Same dig expression now + a mirror re-home pin test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(chart): bump 1.9.20 -> 1.9.21, as this PR changes chart content My earlier merge resolution took develop's 1.9.20 verbatim, reasoning from the release train's rule: v1.9.20 is untagged, and the train's version_preflight only refuses when the version is ALREADY released, so one bump covers a whole release cycle. That reasoning is correct for the train and wrong for this repo. client/scripts/chart-version-guard.sh enforces a stricter rule for a repo-specific reason: chart content reaches installs only via a NEW chart version, because a Helm repo publishes on version change. An unbumped template/values edit therefore either reaches nobody or overwrites an already-published version. Both have happened here - the perIngestionTables block shipped dark in PR #472, and ingestor-0.2.0.tgz was overwritten 5x between 2026-05-20 and 2026-07-29. This PR changes client/templates/** and client/values.yaml, so it needs its own version rather than riding develop's. v1.9.21 is untagged. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The mechanism
Under Bats (verified on 1.13.0) only the last command in a test body decides the result, so a failing assertion anywhere earlier is silently ignored:
This suite is written multi-assertion throughout, so most assertions could not fail their test. Appending
|| return 1makes them enforce.Scope — about 2× what it first looked
It isn't only
[[ ]]. Single-bracket[ ... ]has identical semantics and there are more of them, plus negated bare commands:[ ... ][[ ... ]]! cmd)Heaviest:
setup-linux.bats296,cluster.bats153,install-client-helm.bats146,common.bats118,preflight.bats102.Only whole-line assertions inside an
@testbody are touched. Excluded: helpers andsetup/teardown(a barereturnthere means something different), the 9 control-flowif/whileconditions, and 18 lines already chained with&&/||. Verified all files still parse (bats --count), no control-flow line was modified, nothing double-appended.Triage result: zero new failures
All 1240 were already true — the suite was accidentally correct. So there's no hidden-bug vs stale-assertion split to report, and no assertion was deleted or weakened to reach green.
…which only means something if the hardening has teeth
So that was proven, not assumed.
cluster.bats's_augment_no_proxy: empty host NO_PROXYasserts 7 substrings and enforced only the last. DeletinglocalhostfromTB_NO_PROXY_DEFAULTS— the entry that keeps a corporate proxy from intercepting loopback — is a real regression:localhostdroppedlocalhostdroppedGuard, so it can't come back
scripts/tests/bats-hygiene.batsplus a shared scannerscripts/tests/unenforced-assertions.awk— one implementation, used by the guard and by its own self-tests. Three tests:That last exclusion isn't cosmetic — my first version flagged its own fixture, which would have made any future test embedding example bats source a false positive.
The guard is also mutation-tested against the real suite: un-hardening one line in
cluster.batsmakes it fail, naming the exactfile:line.Gates
bats scripts/tests/*.batsshellcheck --severity=error(CI file set)bash scripts/check-style.shbash scripts/tests/check-drift.shbash scripts/gen-manifest.sh --checkI checked the TAP plan line deliberately rather than a bare pass count: a truncated read can look green while half the suite never reported.
Note for whoever merges
This touches
preflight.bats, which #445 also changes, so whichever lands second needs a trivial re-resolve. #445's own new tests are already hardened, so there's no double work.🤖 Generated with Claude Code
Note
Low Risk
Test-only hardening and a new Bats hygiene guard; no production paths or runtime behavior change.
Overview
Bats can ignore failing assertions that are not the last command in a test (
[[ … ]]on bash 3.2, and! cmdon all bash versions). This PR makes those checks actually fail tests by appending|| return 1to about 1240 standalone assertions across 15*.batsfiles (assess, cluster, common, chart-version-guard, install-bootstrap, and others).It also adds
bats-hygiene.batsandunenforced-assertions.awk, which scan@testbodies and fail CI if any assertion is still advisory. The awk scanner is covered by many fixture tests (internal||, quoted|| return 1, heredocs, nested functions, one-line tests).No installer or library code changes—tests and guard only.
Reviewed by Cursor Bugbot for commit a2e6ff4. Bugbot is set up for automated code reviews on this repo. Configure here.