From 81284f49e26bc5b42c0969120c22557f8d3c324f Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 19 Aug 2026 11:56:43 +0200 Subject: [PATCH 1/3] ci: one shellcheck gate, and it is the one that does not apt-install (#753) shellcheck ran in three places in this repo. Two of them apt-installed it and hung for 35+ minutes on 2026-08-19; the third, the org reusable job `quality / shellcheck`, passed green in ~5 seconds throughout, because shellcheck is preinstalled on ubuntu-latest (0.9.0, measured in run 32232682350). The premise written above the install step -- "shellcheck itself is not on the runner image" -- was simply false. Measured the file sets before deleting anything, as the ticket asks. The enumerated list in installer-tests.yaml and the Makefile's SHELLCHECK_FILES are byte-identical to each other, expand to 34 files, and are a STRICT SUBSET of the 42 the org job derives from the tree. It had drifted past eight real scripts, among them chart-version-guard.sh, check-digest-drift.sh and index-invariants.sh. A parse error planted in index-invariants.sh is invisible to the enumerated set and caught by the derived one, both verified with the anchor restored afterwards. So the org job's coverage is a superset and both local copies go: * installer-tests.yaml `static` loses its ShellCheck step. Nothing else in that job moves -- the R8 manifest check, check-facts and check-style still run there, which matters because `Static analysis` is not a required context and R8 has no other home. Making it required is a separate ticket. * standard-checks.yml `Lint` loses the apt install and calls `make parse`. It stays a required context reporting unconditionally, as its header demands. The Makefile keeps shellcheck for the pre-push tier but stops enumerating: it derives the file set with the org job's own rule and flags. backend#1606 found this list drifted once before and fixed it by re-copying the list, which reset the clock rather than stopping it. Enumerating the set is the defect. Two fail-closed properties the old one-liner lacked, both mutation-proven: classifying zero files now FAILS rather than reporting green, and the classifier's exit status is not swallowed by a pipe (recipes run under dash, which has no pipefail -- the #630 hazard). Verified green under both bash and dash; `make check` is 9.9 s against a 60 s budget. Also: timeout-minutes on both standard-checks jobs, which had none and so drifted toward the 360-minute default; and a stale comment in helm-ci.yaml that cited the shellcheck apt install as its precedent. The bats install is deliberately left alone -- bats really does need installing, and bounding package installs is #753's own scope. Refs #753 --- .github/workflows/helm-ci.yaml | 14 ++- .github/workflows/installer-tests.yaml | 41 ++++---- .github/workflows/standard-checks.yml | 56 ++++++----- Makefile | 132 +++++++++++++++++++------ 4 files changed, 166 insertions(+), 77 deletions(-) diff --git a/.github/workflows/helm-ci.yaml b/.github/workflows/helm-ci.yaml index 73d66219..d15f1029 100644 --- a/.github/workflows/helm-ci.yaml +++ b/.github/workflows/helm-ci.yaml @@ -54,9 +54,17 @@ jobs: # helm is not on the runner image at a version we choose, so it is # installed here at the pin this repo standardises on. That is - # environment bootstrapping, not a duplicated command -- the same - # distinction that keeps the shellcheck apt install in - # standard-checks.yml. Both `helm-lint` and `helm-vocab` shell out to + # environment bootstrapping, not a duplicated command. + # + # This comment used to cite the shellcheck apt install in + # standard-checks.yml as the parallel case. That install is gone (#753): + # its premise was wrong -- shellcheck IS on the runner image -- so it was + # never the same distinction, and it hung CI for 35 minutes. helm is the + # genuine case: pinned, absent from the image, and installed by a + # sha-pinned action rather than a package manager, which is what a + # bootstrap step should look like. + # + # Both `helm-lint` and `helm-vocab` shell out to # `helm`, and chart-env-vocabulary.sh's helper-backstop cases branch on # the helm VERSION (they self-skip below 3.16), so an unpinned runner # helm would silently change which assertions run. drift-checks.yaml diff --git a/.github/workflows/installer-tests.yaml b/.github/workflows/installer-tests.yaml index f3904f71..6e13079d 100644 --- a/.github/workflows/installer-tests.yaml +++ b/.github/workflows/installer-tests.yaml @@ -2,7 +2,9 @@ name: Installer tests # Validates the curl/PowerShell installer (scripts/) across the breadth of # environments a customer might actually have: -# • static — shellcheck + bash -n + PSScriptAnalyzer +# • static — bash -n + PSScriptAnalyzer + the repo's drift guards +# (shellcheck is NOT here: it is the `quality / shellcheck` +# required check — see the `static` job for why) # • unit-bash — bats (mocked) for the bash installer # • unit-pester— Pester for the PowerShell installer, on Linux AND real Windows # • distro-prereqs — runs the REAL Linux prerequisite-install path (package @@ -66,22 +68,27 @@ jobs: | while IFS= read -r -d '' f; do bash -n "$f" || exit 1; done echo "all shell scripts parse" - - name: ShellCheck (libs + entrypoints) - run: | - sudo apt-get update -qq && sudo apt-get install -y -qq shellcheck - shellcheck --version | grep version - # Gate at error severity. The libs are sourced together as one program, - # so single-file shellcheck reports SC2034 "unused" false positives for - # shared vars (CURL_SECURE, ARCH_DL, colours…) that are defined in - # common.sh and consumed in other sourced files. Warnings are printed - # below for visibility but don't fail the gate. - shellcheck --severity=error --shell=bash \ - scripts/install.sh scripts/install-k8s.sh scripts/gen-manifest.sh scripts/check-facts.sh scripts/check-style.sh scripts/resolve-ingestor-digest.sh scripts/lib/*.sh \ - scripts/tests/check-drift.sh scripts/tests/distro-prereqs.sh scripts/tests/e2e-auto-upgrade.sh scripts/tests/e2e-seal-check.sh scripts/tests/e2e-full-seal.sh scripts/tests/e2e-cluster.sh scripts/tests/e2e-journey.sh scripts/tests/e2e-proxy.sh scripts/tests/lib/e2e-common.sh scripts/tests/path-persist.sh scripts/tests/chart-env-vocabulary.sh scripts/tests/env-vocabulary-agreement.sh - echo "── shellcheck warnings (advisory, non-blocking) ──" - shellcheck --severity=warning --shell=bash \ - scripts/install.sh scripts/install-k8s.sh scripts/gen-manifest.sh scripts/check-facts.sh scripts/check-style.sh scripts/resolve-ingestor-digest.sh scripts/lib/*.sh \ - scripts/tests/check-drift.sh scripts/tests/distro-prereqs.sh scripts/tests/e2e-auto-upgrade.sh scripts/tests/e2e-seal-check.sh scripts/tests/e2e-full-seal.sh scripts/tests/e2e-cluster.sh scripts/tests/e2e-journey.sh scripts/tests/e2e-proxy.sh scripts/tests/lib/e2e-common.sh scripts/tests/path-persist.sh scripts/tests/chart-env-vocabulary.sh scripts/tests/env-vocabulary-agreement.sh || true + # There is no shellcheck step here any more, and adding one back is a + # regression. shellcheck ran in THREE places in this repo (#753): here, + # `Standard checks / Lint`, and `quality / shellcheck` from + # tracebloc/.github's code-quality.yml. Only the third is worth keeping: + # + # • It is a REQUIRED check on develop; this job is not (so the copy that + # lived here could never block anything — it was advice, not a gate). + # • It DERIVES its file set from the tree (extension, else shebang) + # instead of enumerating it. Measured on develop 2026-08-19: the list + # that used to sit here expanded to 34 files where the derivation + # yields 42 — it had drifted past eight real scripts, including + # scripts/chart-version-guard.sh, scripts/check-digest-drift.sh and + # scripts/index-invariants.sh. A parse error planted in the last of + # those was invisible to this step and caught by the org job. + # • shellcheck is preinstalled on ubuntu-latest, so it needs no + # `apt-get install` — the very step that hung this job to its + # 10-minute timeout twice on 2026-08-19 and reported as `cancelled`. + # + # Same severity (error) and same SC1091 exclusion, so no verdict changes. + # For a whole-tree sweep rather than a PR diff, run the Code quality + # workflow via workflow_dispatch with all-files: true. - name: Installer manifest is current (supply-chain, R8) # The bootstrap verifies each sub-script against scripts/manifest.sha256 diff --git a/.github/workflows/standard-checks.yml b/.github/workflows/standard-checks.yml index 5e3caced..db8cbdc0 100644 --- a/.github/workflows/standard-checks.yml +++ b/.github/workflows/standard-checks.yml @@ -29,43 +29,47 @@ jobs: lint: name: Lint runs-on: ubuntu-latest + # #753: this job had no timeout at all, so when `Install shellcheck` stalled + # on 2026-08-19 it drifted toward GitHub's 360-minute default instead of + # failing fast. Every job in this file now has a bound. + timeout-minutes: 10 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - # shellcheck itself is not on the runner image, so it is installed here. - # That is environment bootstrapping, not a duplicated command. - - name: Install shellcheck - run: | - sudo apt-get update -qq && sudo apt-get install -y -qq shellcheck - shellcheck --version | grep version + # The `Install shellcheck` step that used to sit here is gone, and the + # premise it was written on ("shellcheck itself is not on the runner + # image") is simply false — it IS preinstalled on ubuntu-latest, which is + # how `quality / shellcheck` runs without installing anything. The apt + # call bought nothing and cost this job 35+ minutes on 2026-08-19 (#753). + # + # shellcheck now runs in exactly one place: `quality / shellcheck`, also + # a required check on develop, which derives its file set from the tree + # rather than enumerating it. See installer-tests.yaml's `static` job for + # the measurement. This job keeps the parse half, which needs no install. - - name: make lint - # `make lint` (backend#1606). This job used to restate the parse loop AND - # spell out the shellcheck file list inline, while the Makefile kept the - # same list in SHELLCHECK_FILES. The two had ALREADY drifted, and in the - # direction nobody notices: the Makefile carried 19 entries and this file - # 9, so ten scripts were shellchecked on a contributor's machine and NOT - # at the merge gate -- - # - # scripts/gen-manifest.sh scripts/check-facts.sh - # scripts/check-style.sh scripts/lib/*.sh - # scripts/tests/check-drift.sh scripts/tests/e2e-full-seal.sh - # scripts/tests/e2e-journey.sh scripts/tests/path-persist.sh - # scripts/tests/chart-env-vocabulary.sh - # scripts/tests/env-vocabulary-agreement.sh - # - # gen-manifest.sh is the installer's integrity manifest generator, so a - # shell defect there was invisible to this gate. Measured green across - # all 19 before this landed, so it arms a green check rather than - # importing a backlog. - run: make lint + - name: make parse + # Calls the Makefile rather than restating the loop (backend#1606). That + # ticket's concern was a hand-written FILE LIST drifting between the + # Makefile and this workflow; it fixed the drift by re-copying the list, + # which only reset the clock — measured 2026-08-19, the list had drifted + # again, past eight scripts. `make parse` enumerates nothing (it is a + # `find`), and shellcheck's file set is now derived by the org gate, so + # there is no longer a list here for anything to drift from. + run: make parse unit-tests: name: Unit tests runs-on: ubuntu-latest + # See the lint job: no job in this file had a timeout before #753. + # bats itself runs in ~2 min; the headroom is for the apt install. + timeout-minutes: 15 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + # NOT hardened here. bats has no preinstalled-or-reusable equivalent, so + # unlike shellcheck it genuinely has to be installed, and bounding and + # retrying package installs is #753's own scope rather than this change's. + # The timeout above at least turns a stall into a bounded failure. - name: Install bats run: sudo apt-get update -qq && sudo apt-get install -y -qq bats diff --git a/Makefile b/Makefile index 3116ffb3..3acf3e75 100644 --- a/Makefile +++ b/Makefile @@ -9,35 +9,57 @@ # make setup install what those targets need, and a git pre-push hook # that runs `make check` (skip once with --no-verify). # -# This file is a THIN WRAPPER. Every command below is copied from the +# This file is a THIN WRAPPER. Almost every command below is copied from the # workflow that already runs it — standard-checks.yml, installer-tests.yaml # and helm-ci.yaml. It introduces no new tool, no new config, and no new # rule. When a workflow changes, change the matching line here. +# +# ONE target is no longer a copy of a workflow step: `shellcheck`. Since #753 +# the only shellcheck in CI is the org reusable job `quality / shellcheck` +# (tracebloc/.github), which this repo cannot copy a line out of. That target +# reproduces its rule and flags instead, and says so in its own comment. .DEFAULT_GOAL := help -# The shellcheck file set from installer-tests.yaml's `static` job (a -# superset of standard-checks.yml's list), at the same `error` severity. -SHELLCHECK_FILES := \ - scripts/install.sh \ - scripts/install-k8s.sh \ - scripts/gen-manifest.sh \ - scripts/check-facts.sh \ - scripts/check-style.sh \ - scripts/resolve-ingestor-digest.sh \ - scripts/lib/*.sh \ - scripts/tests/check-drift.sh \ - scripts/tests/distro-prereqs.sh \ - scripts/tests/e2e-auto-upgrade.sh \ - scripts/tests/e2e-seal-check.sh \ - scripts/tests/e2e-full-seal.sh \ - scripts/tests/e2e-cluster.sh \ - scripts/tests/e2e-journey.sh \ - scripts/tests/e2e-proxy.sh \ - scripts/tests/lib/e2e-common.sh \ - scripts/tests/path-persist.sh \ - scripts/tests/chart-env-vocabulary.sh \ - scripts/tests/env-vocabulary-agreement.sh +# The shellcheck file set is DERIVED from the tree, never written down. +# +# It used to be a 19-entry SHELLCHECK_FILES list copied out of +# installer-tests.yaml. Measured on develop 2026-08-19: that list expanded to +# 34 files, while the same classification applied to the tree yields 42. It had +# drifted past eight real scripts -- +# +# docker/k3s-cuda/build.sh docker/k3s-cuda/k3d-entrypoint-tracebloc-cdi.sh +# docs/migration-tools/generate.sh docs/migration-tools/migrate-tenant.sh +# scripts/chart-version-guard.sh scripts/check-digest-drift.sh +# scripts/index-invariants.sh scripts/tests/test_helper.bash +# +# -- and a parse error planted in index-invariants.sh was invisible to the list +# and caught by the derivation. This is the SECOND drift of this list; +# backend#1606 found the first and fixed it by re-copying, which reset the +# clock rather than stopping it. Enumerating a file set is the defect. +# +# The rule below is the one `quality / shellcheck` applies (the required check, +# in tracebloc/.github's code-quality.yml): classify by extension, else by +# shebang; skip .bats/.ps1/.psm1/.zsh; error severity; SC1091 excluded because +# a library sourced through a variable path can never be followed. Same rule, +# same flags. +# +# NOT the same shellcheck, though, so this predicts the gate rather than +# reproducing it: the runner ships 0.9.0 (measured in run 32232682350) and a +# dev box is typically newer -- 0.11.0 via brew today. A finding either side +# sees alone is a version difference, not a drift in this file. And in CI the +# gate reads only the PR DIFF, where this target reads the whole tree, so +# locally it is the stricter of the two. That is the right direction for a +# pre-push check. +# +# Honest limit, because a comment claiming "cannot drift" would be the thing +# this change is deleting: this MIRRORS the org job's rule across a repo +# boundary, it does not read it. If that classification changes, this needs the +# same edit. What is gone is the enumeration -- the part that actually drifted. +# +# Note `--shell=bash` is deliberately NOT passed (the old inline copies did). +# The gate infers the dialect from each file's shebang; forcing bash would make +# a local run disagree with it on the repo's `#!/bin/sh` scripts. # The bats total, DERIVED — never written down. It moves on most PRs that add a # test, nothing enforces it, and the help text had drifted from its hardcoded @@ -54,12 +76,13 @@ BATS_TEST_COUNT = $(shell grep -h '^@test' scripts/tests/*.bats 2>/dev/null | wc help: @echo "tracebloc/client — make targets" @echo - @echo " check lint + fast checks (~4 s) — run this before every push" + @echo " check lint + fast checks (~10 s) — run this before every push" @echo " check-all everything CI runs locally, including the $(BATS_TEST_COUNT)-test bats suite" @echo " setup check for / point at the tools these targets need; installs the pre-push hook" @echo " install-hooks (re)install the git pre-push hook that runs 'make check'" @echo - @echo " individual: lint bats helm-lint helm-vocab helm-template helm-unittest drift" + @echo " individual: lint (= parse + shellcheck) bats helm-lint helm-vocab" + @echo " helm-template helm-unittest drift" @echo @echo " NOT here (CI-only, by name): the 9-distro prereq matrix, e2e-cluster" @echo " (k3d), e2e-proxy (squid), path-persist, Pester, windows-e2e," @@ -67,8 +90,13 @@ help: # ---- check: the pre-push tier ------------------------------------ # -# Measured at ~4 s (macOS): bash -n 0.1 s, shellcheck 3.2 s, the three -# drift guards 0.2 s, helm lint 0.7 s. +# Re-measured 2026-08-19 (macOS, shellcheck 0.11.0), ~10 s total: parse 0.1 s, +# shellcheck 5.2 s, drift 0.4 s, helm-lint 0.7 s, helm-vocab 3.1 s. The old +# note here said 4 s and listed only four of the five targets -- it predated +# helm-vocab and never counted it. shellcheck went 3.2 -> 5.2 s when #753 +# replaced the enumerated 34-file list with the 42 the derivation actually +# finds; that is the eight scripts nothing was checking, not a slowdown. +# Still six times inside the 60 s budget. # # The bats suite is deliberately NOT here. It is the repo's real unit # suite and it takes ~2 min serially on macOS — three times over the @@ -147,7 +175,9 @@ install-hooks: # ---- individual targets ------------------------------------------ -# lint: standard-checks.yml `Lint` + installer-tests.yaml `static`. +# lint: both halves, so `make check` and the pre-push hook are unchanged by +# #753 splitting them. In CI the halves now live in different places: `parse` +# is `Standard checks / Lint`, `shellcheck` is `quality / shellcheck`. # .bats files are bats DSL, not valid bash — they are exercised by # actually running them in the `bats` target. # @@ -157,12 +187,52 @@ install-hooks: # pipeline would still exit 0, so `make check` would cheerfully print # "all shell scripts parse" having parsed nothing. GitHub Actions uses # bash, so CI never showed it. xargs is POSIX, NUL-safe, and propagates -# a child failure as a non-zero exit. (Bugbot, #630.) +# a child failure as a non-zero exit. (Bugbot, #630.) The same hazard is why +# the `shellcheck` target materialises its file list instead of piping it. .PHONY: lint -lint: +lint: parse shellcheck + +# parse: the half CI still runs, as `Standard checks / Lint` -> `make parse`. +# Needs nothing but bash, which is why it survived the install purge in #753. +.PHONY: parse +parse: @find scripts -type f -name '*.sh' -print0 | xargs -0 -n1 bash -n @echo "all shell scripts parse" - shellcheck --severity=error --shell=bash $(SHELLCHECK_FILES) + +# shellcheck: LOCAL pre-push convenience only -- in CI this is the required +# `quality / shellcheck` job, which needs no install because shellcheck is +# preinstalled on ubuntu-latest. Kept here because the point of `make check` is +# to predict CI before you push, and a dev with shellcheck installed should get +# the gate's answer without opening a PR (backend#1850). +# +# Two fail-closed properties, both of which the old one-liner lacked: +# * classifying ZERO files is a FAILURE, not a green run. A silent no-op is +# exactly how a broken derivation would look (backend#1729 rule 3). +# * the classifier's exit code is not swallowed by the pipe. Recipes run +# under /bin/sh -- dash on Debian, no pipefail -- so the file list is +# materialised first and shellcheck's own status is what propagates. +# The `; :` inside the classifier is load-bearing: `grep -q ... && printf` +# exits 1 on every non-shell file, which would otherwise make xargs return 123. +.PHONY: shellcheck +shellcheck: + @files=$$(mktemp); \ + git ls-files -z \ + | xargs -0 -r -n1 sh -c 'case "$$1" in \ + *.sh|*.bash|*.ksh) printf "%s\n" "$$1" ;; \ + *.bats|*.ps1|*.psm1|*.zsh) ;; \ + *) head -n 1 "$$1" 2>/dev/null \ + | grep -Eq "^#![[:space:]]*[^[:space:]]*(/|[[:space:]])(ba|da|k)?sh([[:space:]]|$$)" \ + && printf "%s\n" "$$1" ;; \ + esac; :' sh > "$$files"; \ + n=$$(wc -l < "$$files" | tr -d " "); \ + if [ "$$n" -eq 0 ]; then \ + echo "shellcheck: classified ZERO shell files -- the derivation above is broken."; \ + echo " Refusing to report green on an empty file set."; \ + rm -f "$$files"; exit 1; \ + fi; \ + echo "shellcheck: $$n file(s), severity=error"; \ + tr "\n" "\0" < "$$files" | xargs -0 -r shellcheck --severity=error --exclude=SC1091; \ + rc=$$?; rm -f "$$files"; exit $$rc # drift: the repo's duplicated-declaration guards. The first three come from # installer-tests.yaml's `static` job; all three are pure local file From 80671e8b2d993dc58e7d79183ccbc7819fc6b968 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 19 Aug 2026 13:57:36 +0200 Subject: [PATCH 2/3] ci: fail `parse` closed, and keep a whole-tree shellcheck sweep (#754 review) Both findings from Arturo's review. Built on top of b4ab27b (a concurrent session's develop merge) rather than a second resolution of my own -- see the note at the end, which is the more useful half of this message. FIX 1 -- `make parse` failed open, and #753 had just promoted it to be the entire content of the required `Lint` check. `find scripts ... | xargs -0 -n1 bash -n` loses find's status across the pipe (recipes run under dash, no pipefail) and xargs with no input runs nothing and exits 0, so in a tree with no scripts/ dir it printed find: scripts: No such file or directory all shell scripts parse and exited 0, under both shells. It now materialises the list, refuses a zero count, and prints the derived count. Proven four ways: missing dir -> non-zero, zero .sh files -> non-zero, real parse error -> non-zero, clean tree -> "all 39 shell scripts parse". The first attempt at that third anchor was INERT -- `if [ -z "$x" ; then` is valid bash, the missing bracket being a runtime not a parse error -- so it was redone with an unterminated `if`, after checking that bash -n rejects it standalone. FIX 2 -- the superset claim was true set-wise and false scope-wise. The deleted jobs swept their 34 files on every run; `quality / shellcheck` reads only the PR diff, its caller passes all-files: false and declares no schedule. So #753 as written left NO CI job doing a whole-tree error-severity sweep -- demonstrable on #754 itself, where that job is green in 8s reading "Shell files to check: 0". `Lint` therefore runs `make lint` (parse + derived whole-tree shellcheck) rather than `make parse`. Costs no install, shellcheck being preinstalled is the whole finding, and it restores `make lint` == CI `Lint` parity (backend#1850). The two survivors are complementary: whole-tree here, diff-scoped in the org job, neither installing anything, neither holding a file list. Verified green on the 44-file derived set under the RUNNER's shellcheck 0.9.0 via koalaman/shellcheck:v0.9.0, not merely a local 0.11.0, before arming it on a required check. WHY THIS SITS ON b4ab27b. I had resolved the same develop conflict locally and resolved the Makefile with `git checkout --ours`, which takes the whole file and so silently DROPPED develop's non-conflicting addition of `scripts/tests/telemetry-vocabulary-agreement.sh` to the `drift` target. Nothing would have failed: the guard would simply have stopped running. b4ab27b kept it, so that resolution is the base and mine was discarded. This is the same shape as the #755 hazard flagged on the PR -- the damage from a merge is in what disappears without a marker, not in what conflicts -- and it caught me one hour after I wrote that warning. The conflict itself is the thesis: to add one script, #747 had to hand-edit the enumerated list in three places. The derivation picked it up with no edit, 42 -> 44 on its own. Refs #753 --- .github/workflows/installer-tests.yaml | 28 ++++++---- .github/workflows/standard-checks.yml | 43 +++++++++------ Makefile | 76 ++++++++++++++++++++------ 3 files changed, 100 insertions(+), 47 deletions(-) diff --git a/.github/workflows/installer-tests.yaml b/.github/workflows/installer-tests.yaml index 6e13079d..d7840640 100644 --- a/.github/workflows/installer-tests.yaml +++ b/.github/workflows/installer-tests.yaml @@ -71,24 +71,28 @@ jobs: # There is no shellcheck step here any more, and adding one back is a # regression. shellcheck ran in THREE places in this repo (#753): here, # `Standard checks / Lint`, and `quality / shellcheck` from - # tracebloc/.github's code-quality.yml. Only the third is worth keeping: + # tracebloc/.github's code-quality.yml. This copy is the one with no + # reason to exist: # - # • It is a REQUIRED check on develop; this job is not (so the copy that - # lived here could never block anything — it was advice, not a gate). - # • It DERIVES its file set from the tree (extension, else shebang) - # instead of enumerating it. Measured on develop 2026-08-19: the list - # that used to sit here expanded to 34 files where the derivation - # yields 42 — it had drifted past eight real scripts, including + # • It is not a REQUIRED check on develop (this whole job isn't), so it + # could never block anything — it was advice, not a gate. Making the + # R8 step below a gate is backend#2182 / #2183, a separate concern. + # • It ENUMERATED its file set. Measured on develop 2026-08-19: the list + # that used to sit here expanded to 34 files where deriving from the + # tree yields 44 — it had drifted past eight real scripts, including # scripts/chart-version-guard.sh, scripts/check-digest-drift.sh and - # scripts/index-invariants.sh. A parse error planted in the last of - # those was invisible to this step and caught by the org job. + # scripts/index-invariants.sh, and #747 then had to hand-edit the list + # in three places to add one new script. A parse error planted in + # index-invariants.sh was invisible to this step. # • shellcheck is preinstalled on ubuntu-latest, so it needs no # `apt-get install` — the very step that hung this job to its # 10-minute timeout twice on 2026-08-19 and reported as `cancelled`. # - # Same severity (error) and same SC1091 exclusion, so no verdict changes. - # For a whole-tree sweep rather than a PR diff, run the Code quality - # workflow via workflow_dispatch with all-files: true. + # The two survivors are complementary rather than duplicated, and both + # derive their file set: `Standard checks / Lint` runs `make lint` over + # the WHOLE TREE, and `quality / shellcheck` scans the PR DIFF. Same + # severity (error) and same SC1091 exclusion as the step deleted here, so + # no verdict changes. - name: Installer manifest is current (supply-chain, R8) # The bootstrap verifies each sub-script against scripts/manifest.sha256 diff --git a/.github/workflows/standard-checks.yml b/.github/workflows/standard-checks.yml index db8cbdc0..d93fc9c2 100644 --- a/.github/workflows/standard-checks.yml +++ b/.github/workflows/standard-checks.yml @@ -38,24 +38,33 @@ jobs: # The `Install shellcheck` step that used to sit here is gone, and the # premise it was written on ("shellcheck itself is not on the runner - # image") is simply false — it IS preinstalled on ubuntu-latest, which is - # how `quality / shellcheck` runs without installing anything. The apt - # call bought nothing and cost this job 35+ minutes on 2026-08-19 (#753). - # - # shellcheck now runs in exactly one place: `quality / shellcheck`, also - # a required check on develop, which derives its file set from the tree - # rather than enumerating it. See installer-tests.yaml's `static` job for - # the measurement. This job keeps the parse half, which needs no install. + # image") is simply false — it IS preinstalled on ubuntu-latest (0.9.0, + # measured in run 32232682350), which is how `quality / shellcheck` runs + # without installing anything. The apt call bought nothing and cost this + # job 35+ minutes on 2026-08-19 (#753). - - name: make parse - # Calls the Makefile rather than restating the loop (backend#1606). That - # ticket's concern was a hand-written FILE LIST drifting between the - # Makefile and this workflow; it fixed the drift by re-copying the list, - # which only reset the clock — measured 2026-08-19, the list had drifted - # again, past eight scripts. `make parse` enumerates nothing (it is a - # `find`), and shellcheck's file set is now derived by the org gate, so - # there is no longer a list here for anything to drift from. - run: make parse + - name: make lint + # Calls the Makefile rather than restating the commands (backend#1606). + # That ticket's concern was a hand-written FILE LIST drifting between + # the Makefile and this workflow; it fixed the drift by re-copying the + # list, which only reset the clock — measured 2026-08-19, the list had + # drifted again, past eight scripts, and #747 then had to hand-edit it + # in three places to add one new script. `make lint` enumerates nothing: + # `parse` is a find, `shellcheck` derives from `git ls-files`. + # + # WHOLE TREE, and that is the point of running it here (Arturo, #754 + # review). `quality / shellcheck` is required and derives the same way, + # but on a PR it scans only the DIFF and exits 0 on an empty one. If + # this job dropped shellcheck, no CI job would run a whole-tree + # error-severity sweep at all — the caller has no `schedule:` and passes + # `all-files: false` — so the superset claim behind #753 would have been + # true set-wise and false scope-wise. The two are complementary, not + # duplicates: diff-scoped gate + whole-tree gate, neither installing + # anything, neither holding a file list. + # + # Verified green on the 44-file derived set under the runner's 0.9.0 + # (not just a newer local build) before arming it. + run: make lint unit-tests: name: Unit tests diff --git a/Makefile b/Makefile index 8073215c..18c971af 100644 --- a/Makefile +++ b/Makefile @@ -14,10 +14,12 @@ # and helm-ci.yaml. It introduces no new tool, no new config, and no new # rule. When a workflow changes, change the matching line here. # -# ONE target is no longer a copy of a workflow step: `shellcheck`. Since #753 -# the only shellcheck in CI is the org reusable job `quality / shellcheck` -# (tracebloc/.github), which this repo cannot copy a line out of. That target -# reproduces its rule and flags instead, and says so in its own comment. +# `shellcheck` is the one target not copied from a workflow LINE, but it is +# still exactly what a workflow runs: since #753, `Standard checks / Lint` runs +# `make lint`, so this file IS the definition rather than a copy of one. The org +# reusable job `quality / shellcheck` (tracebloc/.github) applies the same rule +# to the PR diff; that one cannot be copied from, so this target reproduces its +# classification and flags, and says so in its own comment. .DEFAULT_GOAL := help @@ -44,13 +46,19 @@ # a library sourced through a variable path can never be followed. Same rule, # same flags. # -# NOT the same shellcheck, though, so this predicts the gate rather than -# reproducing it: the runner ships 0.9.0 (measured in run 32232682350) and a -# dev box is typically newer -- 0.11.0 via brew today. A finding either side -# sees alone is a version difference, not a drift in this file. And in CI the -# gate reads only the PR DIFF, where this target reads the whole tree, so -# locally it is the stricter of the two. That is the right direction for a -# pre-push check. +# SCOPE, precisely, because #753's first draft got this wrong (Arturo review): +# * this target, run by `Standard checks / Lint` and by `make check` -- the +# WHOLE TREE, on every PR and every push. +# * `quality / shellcheck` -- the PR DIFF only. Its caller passes +# `all-files: false` and declares no `schedule:`, so on a PR touching no +# shell file it legitimately reports "Shell files to check: 0" and exits 0. +# Both are required checks and both derive. Dropping the whole-tree half would +# leave no CI job sweeping the full tree at error severity, which is what the +# old enumerated jobs did on every run. +# +# Versions differ, and that is not drift: the runner ships 0.9.0 (measured in +# run 32232682350), a dev box is typically newer -- 0.11.0 via brew today. The +# 44-file set was verified green under BOTH before this was armed. # # Honest limit, because a comment claiming "cannot drift" would be the thing # this change is deleting: this MIRRORS the org job's rule across a repo @@ -194,16 +202,48 @@ lint: parse shellcheck # parse: the half CI still runs, as `Standard checks / Lint` -> `make parse`. # Needs nothing but bash, which is why it survived the install purge in #753. +# Materialises the list and counts it, for the same reason the `shellcheck` +# target does (Arturo, #754 review). The previous one-liner was +# +# @find scripts -type f -name '*.sh' -print0 | xargs -0 -n1 bash -n +# @echo "all shell scripts parse" +# +# and it FAILED OPEN in two ways at once. `find`'s exit status is lost across +# the pipe -- recipes run under dash, which has no `pipefail` -- and `xargs` +# with no input runs nothing and exits 0. So in a tree where `scripts/` is +# missing or renamed it printed +# +# find: scripts: No such file or directory +# all shell scripts parse +# +# and exited 0, under both dash and bash. That was survivable while an +# enumerated shellcheck line ran straight after it and would have failed on the +# same breakage; #753 made `parse` a load-bearing half of the required `Lint` +# check, which is exactly when a fail-open stops being tolerable. +# +# The count is derived and printed, so "parsed nothing" can no longer read +# identically to "parsed everything" -- the #630 hazard, one target over. .PHONY: parse parse: - @find scripts -type f -name '*.sh' -print0 | xargs -0 -n1 bash -n - @echo "all shell scripts parse" + @files=$$(mktemp); \ + if ! find scripts -type f -name '*.sh' -print > "$$files" 2>/dev/null; then \ + echo "parse: could not enumerate scripts/ -- refusing to report green"; \ + rm -f "$$files"; exit 1; \ + fi; \ + n=$$(wc -l < "$$files" | tr -d " "); \ + if [ "$$n" -eq 0 ]; then \ + echo "parse: found ZERO shell scripts under scripts/ -- refusing to report green"; \ + rm -f "$$files"; exit 1; \ + fi; \ + tr "\n" "\0" < "$$files" | xargs -0 -r -n1 bash -n; \ + rc=$$?; rm -f "$$files"; \ + if [ "$$rc" -eq 0 ]; then echo "all $$n shell scripts parse"; fi; \ + exit $$rc -# shellcheck: LOCAL pre-push convenience only -- in CI this is the required -# `quality / shellcheck` job, which needs no install because shellcheck is -# preinstalled on ubuntu-latest. Kept here because the point of `make check` is -# to predict CI before you push, and a dev with shellcheck installed should get -# the gate's answer without opening a PR (backend#1850). +# shellcheck: run BOTH by the pre-push tier and by CI -- `Standard checks / +# Lint` calls `make lint`, which is this plus `parse`. No install anywhere: +# shellcheck is preinstalled on ubuntu-latest. One definition, so `make check` +# genuinely predicts the gate instead of merely resembling it (backend#1850). # # Two fail-closed properties, both of which the old one-liner lacked: # * classifying ZERO files is a FAILURE, not a green run. A silent no-op is From 9d811b3939689d5d8c0dc9b8f05b18890cd6241f Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Wed, 19 Aug 2026 14:16:07 +0200 Subject: [PATCH 3/3] ci: bound the bats install, and fix four stale comments (Asad review) COMMENTS (Asad, #754) -- three he named plus one he did not, since the point of his finding is that a stale comment is this PR's whole subject: * `lint:` still said the halves "live in different places" and `parse:` still said `Lint` runs `make parse`. Both were true of the previous commit and false after it. Fixed. * `check:` said "the 42 the derivation actually finds" -- 44 on this head. * the header block also carried a bare "yields 42", which he did not flag. Rather than update the numbers, the counts are GONE from the prose. They move -- 42 when #753 was measured, 44 once #747 landed -- and both targets already print their live count when they run. That is the BATS_TEST_COUNT reasoning three declarations up, applied to the thing that just rotted twice in one day. The one surviving figure is pinned to a commit ("measured on develop at 8de5d64") so it reads as history rather than as current state. BATS INSTALL -- bounded and retried, in both workflows. This is the finding landing on its own author: the first draft left these two apt calls alone on the grounds that hardening installs was #753's scope, and then `unit-bash`'s `Install bats` stalled at 11:58 today, burned the whole 10-minute budget and reported `cancelled` with an empty log and `Run bats` skipped -- the exact signature #753 documents for shellcheck. A required check is the wrong place to leave a known unbounded external call. Sizing is measured, not guessed, and the first attempt at it was WRONG: 120 s bounds x 3 attempts plus backoff is 780 s worst case, which overruns the 600 s budget and would have reported `cancelled` again -- reintroducing the bug the bound exists to remove. Healthy numbers are install 9 s and bats 155 s (measured from run history), so: 60 s bounds, 3 attempts, backoff only BETWEEN attempts (10 + 20, no trailing sleep), worst case 390 s + ~155 s of bats, and unit-bash's budget raised 10 -> 15 to match its sibling. ~9 min inside 15. Two details that are load-bearing: * `sudo timeout`, not `timeout sudo` -- timeout must be root to signal apt-get directly instead of signalling sudo and trusting the TERM to be forwarded. * `if [ "$i" -lt 3 ]; then sleep; fi`, not `[ ... ] && sleep` -- the default Actions shell is `bash -e`, where a bare test returning false on the last iteration aborts the script BEFORE the ::error:: line prints, turning a diagnosed failure back into a silent one. * `-qq` dropped from the install and `bats --version` kept as the proof-of-install marker: its absence is what identified the stall. Verified by extracting the LIVE run block out of the YAML with a yaml parser and executing it under `bash -e` against a stubbed apt-get -- not a retyped copy. Always-fails -> 3 warnings, the ::error:: line, exit 1, 30 s. Fails-once -> warning then success. Healthy -> version marker, exit 0, no sleep. Refs #753 --- .github/workflows/installer-tests.yaml | 40 +++++++++++++++++++++-- .github/workflows/standard-checks.yml | 44 ++++++++++++++++++++++---- Makefile | 31 +++++++++++------- 3 files changed, 95 insertions(+), 20 deletions(-) diff --git a/.github/workflows/installer-tests.yaml b/.github/workflows/installer-tests.yaml index d7840640..c167b0bc 100644 --- a/.github/workflows/installer-tests.yaml +++ b/.github/workflows/installer-tests.yaml @@ -139,13 +139,47 @@ jobs: Write-Host "no PSScriptAnalyzer errors" unit-bash: - timeout-minutes: 10 + # 10 -> 15. At 10 the bounded install (390 s worst case) plus ~155 s of bats + # could still reach the cap and report `cancelled`, which is the #753 bug the + # bound exists to remove. Matches standard-checks.yml's `Unit tests`. + timeout-minutes: 15 name: bats (bash unit, mocked) runs-on: ubuntu-latest steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - name: Install bats - run: sudo apt-get update -qq && sudo apt-get install -y -qq bats + + # Bounded and retried, same shape as "Pull (bounded, retried)" + # below. This is not speculative hardening: this exact step stalled on + # 2026-08-19 at 11:58 and burned the whole `timeout-minutes: 10` budget, + # reporting `cancelled` with an empty log and `Run bats` skipped -- the + # #753 signature, on bats rather than shellcheck. shellcheck could be + # fixed by deleting the install (it is preinstalled); bats genuinely has + # to be installed, so it gets a bound instead. + # + # `sudo timeout`, not `timeout sudo`: timeout must run as root so it + # signals apt-get directly rather than signalling sudo and hoping the TERM + # is forwarded. `-qq` is dropped on the install so a slow mirror leaves + # something in the log, and `bats --version` is the proof-of-install + # marker whose absence identified the stall in the first place. + - name: Install bats (bounded, retried) + # Bounds sized against the job budget (see the twin step in + # standard-checks.yml for the arithmetic): worst case 3 x (60 + 60) + + # 30 s backoff = 390 s, plus ~155 s of bats, inside the 15 minutes this + # job now allows. `if`/`fi` not `[ ... ] &&` because the default shell is + # `bash -e` and a false test on the last iteration would abort before the + # ::error:: line printed. + run: | + for i in 1 2 3; do + if sudo timeout 60 apt-get update -qq \ + && sudo timeout 60 apt-get install -y bats; then + bats --version && exit 0 + fi + echo "::warning::bats install stalled or failed (attempt $i/3)" + if [ "$i" -lt 3 ]; then sleep $((i*10)); fi + done + echo "::error::could not install bats in 3 bounded attempts - runner-to-mirror connectivity, not this PR. Re-run this job." + exit 1 + - name: Run bats run: bats scripts/tests/*.bats diff --git a/.github/workflows/standard-checks.yml b/.github/workflows/standard-checks.yml index d93fc9c2..b3a16d63 100644 --- a/.github/workflows/standard-checks.yml +++ b/.github/workflows/standard-checks.yml @@ -75,12 +75,44 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - # NOT hardened here. bats has no preinstalled-or-reusable equivalent, so - # unlike shellcheck it genuinely has to be installed, and bounding and - # retrying package installs is #753's own scope rather than this change's. - # The timeout above at least turns a stall into a bounded failure. - - name: Install bats - run: sudo apt-get update -qq && sudo apt-get install -y -qq bats + # bats genuinely has to be installed -- unlike shellcheck there is no + # preinstalled or reusable equivalent -- so it gets a bound rather than a + # deletion. Bounded and retried in the same shape as installer-tests.yaml's + # "Pull (bounded, retried)". + # + # This was left as plain apt in the first draft of #753, on the grounds + # that hardening installs was that ticket's own scope. Then the identical + # step in installer-tests.yaml's `unit-bash` stalled at 11:58 on + # 2026-08-19 and burned its whole 10-minute budget, reporting `cancelled` + # with an empty log. A required check is the wrong place to leave a known + # unbounded external call. + # + # `sudo timeout`, not `timeout sudo`: timeout must run as root to signal + # apt-get directly instead of signalling sudo and hoping TERM is + # forwarded. `-qq` dropped on the install so a stall is not silent, and + # `bats --version` is the proof-of-install marker. + - name: Install bats (bounded, retried) + # Bounds SIZED against the job budget, the way the distro-pull step + # sizes its own. Measured healthy: install 9 s, bats run 155 s. Worst + # case here is 3 x (60 + 60) + 10 + 20 = 390 s, plus ~155 s of bats = + # ~9 min, inside this job's 15. At the 120 s bound this first had, worst + # case was 780 s -- which would have blown a 10-minute budget and + # reported `cancelled` again, i.e. reintroduced the very bug. + # + # `if`/`fi` rather than `[ ... ] && sleep`: the default shell is + # `bash -e`, so a bare test returning false on the last iteration would + # abort the script before the ::error:: line ever printed. + run: | + for i in 1 2 3; do + if sudo timeout 60 apt-get update -qq \ + && sudo timeout 60 apt-get install -y bats; then + bats --version && exit 0 + fi + echo "::warning::bats install stalled or failed (attempt $i/3)" + if [ "$i" -lt 3 ]; then sleep $((i*10)); fi + done + echo "::error::could not install bats in 3 bounded attempts - runner-to-mirror connectivity, not this PR. Re-run this job." + exit 1 - name: Run bats (bash unit suite, mocked) run: bats scripts/tests/*.bats diff --git a/Makefile b/Makefile index 18c971af..c9bf962d 100644 --- a/Makefile +++ b/Makefile @@ -26,9 +26,10 @@ # The shellcheck file set is DERIVED from the tree, never written down. # # It used to be a 19-entry SHELLCHECK_FILES list copied out of -# installer-tests.yaml. Measured on develop 2026-08-19: that list expanded to -# 34 files, while the same classification applied to the tree yields 42. It had -# drifted past eight real scripts -- +# installer-tests.yaml. Measured on develop at 8de5d64: that list expanded to 34 +# files where the same classification applied to the tree found 42 (the live +# number is higher now and is printed by the target -- see `check` above on why +# it is not restated here). It had drifted past eight real scripts -- # # docker/k3s-cuda/build.sh docker/k3s-cuda/k3d-entrypoint-tracebloc-cdi.sh # docs/migration-tools/generate.sh docs/migration-tools/migrate-tenant.sh @@ -101,11 +102,16 @@ help: # Re-measured 2026-08-19 (macOS, shellcheck 0.11.0), ~10 s total: parse 0.1 s, # shellcheck 5.2 s, drift 0.4 s, helm-lint 0.7 s, helm-vocab 3.1 s. The old # note here said 4 s and listed only four of the five targets -- it predated -# helm-vocab and never counted it. shellcheck went 3.2 -> 5.2 s when #753 -# replaced the enumerated 34-file list with the 42 the derivation actually -# finds; that is the eight scripts nothing was checking, not a slowdown. +# helm-vocab and never counted it. shellcheck got slower (3.2 -> 5.2 s) because +# #753 replaced the enumerated 34-file list with everything the derivation +# finds; that is the scripts nothing was checking, not a slowdown. # Still six times inside the 60 s budget. # +# No file count is written down here on purpose. It moves -- it was 42 when #753 +# was measured and 44 once #747 landed -- and a stale number in a comment is the +# defect this PR exists to remove. Both targets print their live count when they +# run, the same reasoning as BATS_TEST_COUNT above. +# # The bats suite is deliberately NOT here. It is the repo's real unit # suite and it takes ~2 min serially on macOS — three times over the # budget — and it does not parallelise without GNU parallel, which is @@ -183,9 +189,10 @@ install-hooks: # ---- individual targets ------------------------------------------ -# lint: both halves, so `make check` and the pre-push hook are unchanged by -# #753 splitting them. In CI the halves now live in different places: `parse` -# is `Standard checks / Lint`, `shellcheck` is `quality / shellcheck`. +# lint: both halves, and `Standard checks / Lint` runs exactly this target, so +# the pre-push tier and the merge gate cannot disagree about what linting means +# (backend#1850). Neither half installs anything: bash is bash, and shellcheck is +# preinstalled on ubuntu-latest. # .bats files are bats DSL, not valid bash — they are exercised by # actually running them in the `bats` target. # @@ -200,8 +207,10 @@ install-hooks: .PHONY: lint lint: parse shellcheck -# parse: the half CI still runs, as `Standard checks / Lint` -> `make parse`. -# Needs nothing but bash, which is why it survived the install purge in #753. +# parse: bash -n over every shell script under scripts/. Reached in CI through +# `make lint`, which is what `Standard checks / Lint` runs -- not as a target of +# its own. Needs nothing but bash, which is why it survived #753's install purge. +# # Materialises the list and counts it, for the same reason the `shellcheck` # target does (Arturo, #754 review). The previous one-liner was #