fix(tests): preflight PyYAML in every yaml-parsing guard (backend#2686)

[aptracebloc]

What

The chart guards in scripts/tests/ check command -v python3 and then import yaml inside a python heredoc. command -v python3 proves the interpreter exists, not the PyYAML module: on a runner with python3 but no PyYAML the import dies as a bare ModuleNotFoundError traceback (in a .sh guard) or degrades to a silent skip (in a .bats one — indistinguishable from a pass in a required gate), where the sibling helm-unittest-error-assertions.sh already gives [ERROR] PyYAML required.

Cursor Bugbot flagged one instance — the CronJob guard (cronjob-failures-are-readable.sh), deferred as backend#2686 from the refused client#869 staging hop.

Fix the class, not the instance

Every scripts/tests guard that imports the yaml module now preflights it:

• 10 .sh guards get the interpreter-then-module try/except ImportError → sys.exit("[ERROR] PyYAML required …") the siblings already carry (preserving each heredoc's other imports).
• chart-pull-secret.bats gets a require_pymodule helper mirroring its existing require_tool (local skip / CI hard-fail via ::error::).
• image-refresh-skip-streak.bats and image-refresh-stale-pin.bats get the same named refusal in their inline python.

Every python import yaml in the repo lives under scripts/tests/, so this covers the whole class (14 guards; 4 siblings were already correct).

Regression guard + test (missing-PyYAML case)

Adds scripts/tests/pyyaml-preflight.bats (auto-discovered by the required Unit tests job):

1. Structural, DERIVED, fails-closed — enumerates every yaml-importing guard from the tree (no hardcoded names), asserts each preflights, and fails if zero are found. Proven non-inert: reverting one preflight makes it fail and name the offender.
2. Behavioural (missing case) — runs the real cited sibling under a shadowed PyYAML and asserts a named refusal, no Traceback, no ModuleNotFoundError.
3. Behavioural (present case) — the guard is transparent when PyYAML is installed.

Records the recurring finding (client#827/#830 → backend#2686) in .cursor/BUGBOT.md per CLAUDE.md.

Verification

• make drift: 10 edited .sh guards render + parse green (happy path unchanged). The one local failure is pre-existing/environmental — local helm v4 defaults kubeVersion to 1.20.0 vs the chart's >=1.24.0-0; CI pins helm v3.15.4 and renders fine (the pristine develop guard fails identically locally).
• All 11 .sh guards under a shadowed PyYAML: named [ERROR] PyYAML required, no traceback, no ModuleNotFoundError.
• The 3 edited .bats + new .bats: 47/47 green on the happy path; missing-PyYAML gives a CI hard-fail (chart-pull-secret) / named refusal (image-refresh), local skip where appropriate.
• bash -n clean on all .sh; all .bats parse.

Closes tracebloc/backend#2686

---

Note

Low Risk
Changes are limited to test scripts, bats helpers, and Bugbot guidance; production chart or installer behavior is unchanged, and CI gates become stricter rather than looser.

Overview
Chart test guards under scripts/tests/ only proved python3 exists, not that PyYAML is importable. On runners with the interpreter but no module, embedded import yaml failed as a ModuleNotFoundError traceback in shell guards or as a silent bats skip that looks like a green required gate.

This PR applies the interpreter-then-module pattern everywhere that class appeared: ten .sh guards wrap import yaml in try/except ImportError with a named [ERROR] PyYAML required (pip install pyyaml) exit; chart-pull-secret.bats adds require_pymodule (CI hard-fail / local skip, mirroring require_tool); image-refresh-skip-streak.bats and image-refresh-stale-pin.bats get the same refusal in inline Python.

scripts/tests/pyyaml-preflight.bats is new regression coverage: it derives the list of yaml-importing guards from the tree (fails closed if zero found), statically checks shell gates or guarded imports, and behaviorally asserts a real sibling (helm-unittest-error-assertions.sh) refuses cleanly under a shadowed yaml module. .cursor/BUGBOT.md records the recurring finding so new guards are flagged if they skip preflight.

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

[aptracebloc]

bugbot run

[saqlainsyed007]

Reviewed at adde48b (high effort). The shipped fix is correct and I'd own it — the notes are all non-blocking robustness suggestions on the new enforcer, not the fix. Holding only on CI (still in progress; nothing failing).

What I verified on the fix: the PyYAML preflight is applied UNIFORMLY across all 14 yaml-importing guards — the .sh/heredoc guards try: import yaml / except ImportError: sys.exit("[ERROR] PyYAML required …") (with sys imported first and other imports preserved), the two image-refresh-*.bats do it in setup/setup_file, and chart-pull-secret.bats uses a require_pymodule helper mirroring the existing require_tool pattern. telemetry-token-agreement.sh's two heredocs both got it. Fail-closed is correct everywhere it matters: CI paths sys.exit/::error::+return 1; require_pymodule only skips on a local run (matching require_tool). The new pyyaml-preflight.bats class-enforcer greps the tree so a future unguarded guard reddens CI — exactly the class-fix #2686 wants.

Non-blocking notes on the enforcer (inline): (1) the enumerator regex \bimport\b[^\n]*\byaml\b misses the from yaml import … idiom, so such a future guard wouldn't be enumerated or checked (no current guard uses it); (2) preflight detection is a whole-file substring match, so the literal string PyYAML required in a comment/heredoc alone would satisfy it; (3) the enumerator counts pyyaml-preflight.bats itself (its own error text matches the import pattern and carries the signal), inflating the [OK] all N denominator by one; (4) the "PyYAML present" case re-runs the fullhelm-unittest-error-assertions.sh, coupling this suite to that sibling's unrelated hygiene. I withdrew a fifth note (the broad except ImportError) — your enforcer's own shadow-simulation test depends on that breadth, so it's correct as-is; resolved that thread.

None block. Get CI green and I'll reconcile the four enforcer notes (address or wave off — they're all optional hardening) and approve.

[saqlainsyed007]

Re-reviewed at 05cbe77. Good hardening — and a real one, not cosmetic: bats runs @test bodies with set -e OFF, so the bare mid-test [[ … ]] assertions here were partially vacuous (an intermediate failure — a leaked Traceback/ModuleNotFoundError, or the wrong status — wouldn't fail the test, only the last command's status counted). Adding || return 1 to each assertion in all three tests makes every one of them able to redden. That closes a mutation-proofing gap in the enforcer's own tests.

This is independent of my four earlier notes on the enforcer's structure (the \bimport\b…\byaml\b regex missing from yaml import …; substring-based preflight detection; the enumerator counting pyyaml-preflight.bats itself in the denominator; the "PyYAML present" test re-running the full sibling). Those are all still open and all still optional — none block, and the shipped preflight across the 14 guards remains correct and uniform. Take or leave them.

Holding the approve only on CI (PENDING). Once it's green I'll reconcile the four notes (address or wave off) and approve.

[aptracebloc]

bugbot run

[saqlainsyed007]

Approving at a732ac4 — CI green, and you closed every one of my notes; thanks for the thorough rework.

The enforcer is now structural and complete, exactly as hoped: import_yaml matches the from yaml import … idiom as well as import … yaml (finding 1); the python-level preflight is verified STRUCTURALLY via guarded_by_try — the import must sit inside a try: whose except names ImportError/ModuleNotFoundError, so a stray "PyYAML required" in a comment no longer satisfies the rule (finding 2); and the enforcer skips itself via self_name so it no longer inflates the [OK] all N denominator (finding 3). all(guarded_by_try(...) for i in hits) means EVERY yaml import in a guard must be wrapped, and the || return 1 on the assertions (from the prior commit) keeps the enforcer's own tests non-vacuous. The shipped 14-guard preflight was already correct and uniform. Full rollup green, mergeable, all my threads resolved. LGTM.

[aptracebloc]

bugbot run

[saqlainsyed007]

My earlier approval was dismissed by the post-approval push, so I re-reviewed the delta a732ac48..0012ebec. It's a single commit touching only scripts/tests/pyyaml-preflight.bats, and it's a strict hardening of exactly the enforcer I approved — no regression.

It replaces the windowed-regex heuristic (guarded_by_try scanning ±5 lines) with an AST-based check: it extracts the embedded Python (heredocs + python3 -c '…') and confirms every yaml import — both import yaml and from yaml import — sits lexically inside a try whose handler names ImportError/ModuleNotFoundError, with shell-gate detection now matching on comment-stripped lines. The two behavioural tests are untouched from what I approved and every assertion still carries || return 1.

I re-verified empirically at head: green on the real tree (17 guards — 15 AST-guarded + 2 shell-gated), and mutation-proof — stripping a try/except, wrong except type, or a broken require_pymodule name each reddens the suite and names the offender; comment-only markers, a try: closed before the import, an unguarded from yaml import, and a heredoc-in-a-comment trap are all caught; zero guards and unparseable source both fail closed.

This strengthens the mutation-proofness the approval rested on. I'll re-approve as soon as CI is green — mutation-shard/Bugbot are still running on 0012ebec. Nothing outstanding on my side.

[aptracebloc]

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 f4141ae. Configure here.}

[saqlainsyed007]

Approving at f4141ae5. CI is fully green, mergeable, Bugbot clear, and no threads are open.

The one commit since my last review (0012ebec..f4141ae5) is a strict tightening of the pyyaml-preflight.bats enumerator — it closes two more Bugbot mediums without weakening anything: the shell-gate probe now accepts both quote styles and require_yaml_tooling requires an actual call (a copied definition no longer counts as a gate via a (?![ \t]*\() lookahead); the python3 -c extractor now captures and closes on the matching quote so a double-quoted one-liner is enumerated instead of silently dropped; and the heredoc opener matches quoted/bare spellings.

I re-verified empirically at head: [OK] all 17 guards; removing a guard's preflight, using the wrong except type, or breaking a gate name each redden the suite and name the offender; zero yaml imports fails closed; and the two newly-covered holes (an unguarded double-quoted -c import, a define-but-never-call helper) are now flagged while their guarded/called forms pass. The enforcer I approved on remains AST-based and mutation-proof — this only makes it stricter. LGTM.