ci: one shellcheck gate, and it is the one that does not apt-install (#753) - #754
Conversation
…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
|
Companion ticket for the sibling finding is now filed: tracebloc/backend#2182 — the R8 manifest check, This PR deliberately leaves that job's steps intact so the gap is not widened. Confirmed while writing it that the required |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 81284f4. Configure here.
|
Note for review: this branch is 3 commits behind
Re-verifying rather than assuming was the point: #741 and #752 both edited |
|
Findings from a review pass. No verdict attached. The main one is about this PR's interaction with #755. #754 × #755 — the dangerous breakage merges without a conflict markerVerified by running a real 3-way
The problem is what merges cleanly. #754 deletes I built the merged Makefile, resolved only the visible conflict (a comment about seconds), and ran it: The wrong order (#755 first) is worse still: #754's diff replaces the whole They're also contradictory in principle, not merely overlapping. #754's thesis is "shellcheck runs in exactly one place, Suggestion: land #754 first, then rebase #755 with Two findings on #754 itself1.
|
The telemetry-vocab shellcheck additions on develop (client#747) land inside the enumerated set this PR deletes; the derived rule already covers scripts/tests/telemetry-vocabulary-agreement.sh from the tree. develop's `drift:` target addition is kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…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
|
Both findings on #754 are correct and both are fixed in 80671e8. I reproduced each before touching anything. 1.
|
|
bugbot run |
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
Measured the coverage delta myself instead of taking the body's word for it. Exported the head tree, ran both sets: the enumerated list (36 files now that #747 landed) is a strict subset of the derived 44, comm -23 empty, and both are green — nothing silenced, nothing imported. All 17 shell entries in scripts/manifest.sha256 are in the derived set, so every file the installer fetches is linted. Dropping --shell=bash is a no-op here: everything in the set is #!/usr/bin/env bash except docker/k3s-cuda/k3d-entrypoint-tracebloc-cdi.sh, which the old list never covered. No new # shellcheck disable= anywhere. Ran make lint under /bin/sh and /bin/dash in throwaway repos — 39 parse / 44 shellcheck green, zero-file guards on both halves exit 1, missing scripts/ exits 1, a planted parse error propagates. And the Lint job log on this head settles the load-bearing claim empirically: all 39 shell scripts parse / shellcheck: 44 file(s), 16 s, no install step. Keeping the whole-tree half in Lint after the scope review is the right call — approving.
Two comments in the Makefile went stale in the fixup; inline. And two things outside the diff: the title still says "one shellcheck gate" when the design is now deliberately two (whole-tree in Lint, diff in quality / shellcheck) — worth fixing since the body is what people will read this by. .pre-commit-config.yaml also still claims "Same gate as CI: error severity, bash dialect, installer scripts only" and scopes itself ^scripts/.*\.sh$ with --shell=bash; after this PR that's wrong on two of three counts, and the regex reaches 37 of the 44 — missing docker/k3s-cuda/*.sh, docs/migration-tools/*.sh and test_helper.bash. It is the same enumerated-scope defect the new Makefile comment argues against, so it belongs in this PR.
nit for the record: the body still says "same --exclude=SC1091 on all three" — neither removed invocation passed it. No behaviour change (SC1091 is info-level, both ran at --severity=error), but it isn't accurate.
One pre-merge check I can't do: branch protection isn't readable with my token, so someone with admin should confirm quality / shellcheck is genuinely a required context on develop before this merges.
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 80671e8. Configure here.
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
|
Pushed 9d811b3. Both of Asad's threads addressed and resolved, plus the CI failure this PR was sitting on. The
|
|
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 9d811b3. Configure here.
|
Green at 9d811b3 — 43 pass, 0 fail, 0 unresolved. Re-requesting review from you both: the approval at Verified the changed jobs specifically rather than trusting the rollup:
What changed since the approval:
Still open and deliberately not decided here: the #755 sequencing (note on that PR — its |
aptracebloc
left a comment
aptracebloc
left a comment
There was a problem hiding this comment.
Re-reviewed at 9d811b39. Both of my earlier findings are fixed, and fixed with reproductions rather than prose. Approving.
make parse no longer fails open. It now materialises the list, counts it, refuses zero, and captures rc after the pipe. You reproduced my repro and posted four mutation proofs — including flagging that your first anchor was inert, which is the part most people leave out. CI on this head: all 39 shell scripts parse.
The scope-wise gap is closed by removing the regression, not documenting it. Lint runs make lint = parse + a derived whole-tree shellcheck; CI logs shellcheck: 44 file(s), severity=error. I independently re-confirmed the org job reads git diff --name-only --diff-filter=ACMR "$BASE_SHA...HEAD" unless all-files: true and exits 0 on an empty set, so the two gates are genuinely complementary now rather than one silently replacing the other with less.
The bats hardening is a real self-caught defect with measured sizing. I tested the retry loop under bash -e with stubs: the if/fi and bats --version && exit 0 shapes are safe, and the ::error:: line does print. 390 s worst case against the raised 15-minute budget is right.
All 8 required contexts green. The only reason this is blocked is that the two fix commits auto-dismissed the earlier approval.
Two nits, neither blocking
installer-tests.yaml:94 — "same SC1091 exclusion as the step deleted here" isn't true of that step. The deleted step ran shellcheck --severity=error --shell=bash <files> with no --exclude (verified on c564a15d). The claim holds for the org job, which does use --exclude=SC1091 — just not for the one being compared. A reader who trusts "so no verdict changes" won't notice the new whole-tree gate is strictly more permissive on SC1091 than the sweep it replaced.
The shellcheck: target omits the org job's [ -f "$f" ] || continue. Tested: with scripts/check-facts.sh deleted from disk but still in the index, it stays in the 44-operand argv. Mid-refactor rm scripts/foo.sh then fails the pre-push hook with a shellcheck "does not exist" error instead of something useful — and it's a divergence from the rule the comment claims to mirror exactly ("Same rule, same flags").
Also informational: the E2E auth-proxy (squid) failure at 80671e8 was a flake, not this diff — the #119 assertion failed once at 11:59, passes on develop's last three runs, passed on #755's identical develop base, and is green here. This PR touches no installer script.
On #755 — two things to know before either merges
The PSScriptAnalyzer hazard I flagged earlier is gone, and this PR is what killed it. Changing run: make parse → run: make lint made the make lint step byte-identical on both sides, so it's now context rather than a change. The standard-checks.yml conflict shrank to the Install shellcheck region only, and #755's advisory-shellcheck and PSScriptAnalyzer steps both merge in cleanly below it. I verified the merged file contains both intact.
But the $(SHELLCHECK_FILES) no-op is still live. Merged 9d811b39 × 74eeb059, resolved only the one visible Makefile conflict (a timing comment), and ran it with an argv-logging stub:
$ make lint-warnings
STUB shellcheck argv: --severity=warning --shell=bash
No files specified.
EXIT=0
SHELLCHECK_FILES has no definition in the merged Makefile — only a mention inside a comment — while #755's lint-warnings still references it. Zero operands, || true swallows the usage error, exit 0. For contrast make lint on the same merged tree is healthy: 39 parsed, 44 shellcheck operands.
One resolution hazard worth naming: in the merged installer-tests.yaml the conflict ends at line 153 and unit-bash: starts at 155, with your new bounded bats install at 178. Resolving that file with git checkout --theirs would silently drop it — the same whole-file hazard that bit the Makefile an hour after you wrote the warning about it. Resolve region 48–153 only, hunk by hunk.
Also correcting myself from my earlier comment: I characterised #754 and #755 as opposite remedies for the same gap. That was wrong, and your correction is right — backend#2182/#2183 are about making the R8 step a gate, which this PR explicitly leaves alone. #755 is a different answer on where shellcheck lives, not the opposite of this one.
— drafted with Claude Code
…ings #754 landed on develop, so the sequencing Arturo recommended is satisfied: it removed shellcheck's third copy and explicitly left the R8-as-a-gate question to this PR. Three files conflicted; resolved hunk-by-hunk, taking develop's derived classifier and this PR's deletion of the `static` job. N3 (blocking, Asad + Arturo) — `make drift` could report green having run zero guards, in the very check this PR makes required: $ make drift DRIFT_GUARDS= -> green, exit 0 $ make drift "DRIFT_GUARDS=bash -c 'exit 1'|scripts/..." -> green, exit 0 `guards='$(DRIFT_GUARDS)'` expanded inside single quotes, so the first guard containing a `'` terminated the assignment and the list collapsed. Fixed at the root by exporting the list and reading it from the environment — no quote in a guard can collapse an env var — plus an empty-list refusal and an iteration count asserted against the number of declared entries. Both shapes now exit non-zero; the quoted case runs both guards and reports the failing one. The comment that claimed "this is NOT the fail-open shape" is now three machine checks instead of a claim (backend#1729 rule 7). The `$(SHELLCHECK_FILES)` no-op (Arturo) — #754 deleted that variable while this PR's new `lint-warnings` still referenced it, so the advisory sweep ran with no operands and `|| true` swallowed the usage error: permanently green. Confirmed on the merged tree. Both sweeps now read ONE derivation, scripts/sh-files.sh, which exits non-zero on a zero-file classification — so a broken derivation is loud in both instead of silent in either. Mutation-proved via SH_FILES= stub: both targets go red. Also from the reviews: - drift-checks.yaml: deleted the five-guard enumeration. It was already wrong (six since telemetry-vocabulary-agreement.sh arrived) eight lines under a header claiming the job "holds no list of its own". `make drift` prints the live set and count. - drift-checks.yaml: `timeout-minutes: 20`. The "no timeout, deliberately" argument didn't survive this PR — `skipped` guards were harmless only while the job was required nowhere; now a kill is `cancelled`, which blocks. The hang risk is azure/setup-helm, not the guards. - standard-checks.yml: dropped `Ensure shellcheck` (preinstalled — #754's basis). - standard-checks.yml: PSScriptAnalyzer installs only if absent. 1.25.0 ships on the image and was already loaded while the step spent 38s of 59s re-fetching it inside a required check. - standard-checks.yml: `if: always()` -> `if: !cancelled()`, so a lost job stops spending its budget. - docs/SUPPLY_CHAIN.md: the runbook said to read `branches/{b}/protection`, which is admin-only and 404s indistinguishably from "not protected" — a non-admin following it concluded the opposite of the truth. Now names `branches/develop` -> `.protection.required_status_checks.contexts`. - Makefile header: names drift-checks.yaml, and `drift` as the second target this file defines rather than copies. Answering Asad's pre-merge ask: `Static analysis` is in NO required-check list — develop, staging and main all read, plus both rulesets. Deleting the job leaves no PR waiting on a context that will never report. Verified: make check green; shellcheck 45 files (the new script included, and mutation-proved covered — a planted parse error reddens the gating sweep); lint-warnings reports 45 and real warnings instead of "No files specified"; drift 6/6 green. bats runs past the local 10-minute budget, as Asad also found; CI is the authority. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/fr-pass Best-effort functional review passed (triage: merged clean, non-interactive; behavioral evidence limited while e2e journey is red — backend#2206). Advancing to Ready for prod. |
3 participants
Closes the shellcheck half of #753.
The measurement
shellcheck runs in three places in this repo. On 2026-08-19 two of them hung 35+ minutes on
apt-get install shellcheck; the third passed green in ~5 seconds.quality / shellcheck(orgcode-quality.yml)Lint(standard-checks.yml→make lint)apt-gettimeout-minutesat allStatic analysis(installer-tests.yaml)apt-gettimeout-minutes: 10, twice, reportedcancelledThe comment above #2's install said "shellcheck itself is not on the runner image, so it is installed here." That is false. Run
32232682350printsversion: 0.9.03.9 s into the job with no install step.The file sets, measured before deleting anything
The ticket asks whether #2 and #3 add coverage over #1. They do not — they subtract it.
SHELLCHECK_FILESand Add label type=system #3's inline list are byte-identical to each other: 19 entries, expanding to 34 files.commreports zero files it catches that the derivation misses.The eight it had drifted past:
Mutation-proven, anchor confirmed applied:
Same severity (
error) and same--exclude=SC1091on all three, so no verdict changes for the files they share.Scope, stated plainly (corrected after review)
The two claims below are separate, and the first draft of this PR conflated them:
quality / shellcheckreads only the PR diff — its caller passesall-files: falseand declares noschedule:. Demonstrated on this very PR: that job is green in 8s withShell files to check: 0.As first written, this PR would therefore have left no CI job doing a whole-tree error-severity sweep. Fixed in 80671e8 rather than merely documented:
Lintrunsmake lint(parse + derived whole-tree shellcheck), which needs no install because shellcheck is preinstalled. The result is two complementary required gates — whole-tree inLint, diff-scoped inquality / shellcheck— neither installing anything and neither holding a file list. Verified green on the 44-file set under the runner's own shellcheck 0.9.0, not just a newer local build.The change
installer-tests.yaml—staticloses its ShellCheck step only. R8 manifest, check-facts and check-style are untouched. This matters:Static analysisis not a required context and R8 has no other home, so this change must not be read as thinning that job. Making it required is the companion ticket, filed separately.standard-checks.yml—Lintloses the apt install, callsmake parse. Still a required context reporting unconditionally, as its own header demands.timeout-minutesadded to both jobs.Makefile— keeps shellcheck for the pre-push tier but stops enumerating, deriving the set with the org job's rule and flags instead. backend#1606 found this list drifted once already and fixed it by re-copying it, which reset the clock rather than stopping it.helm-ci.yaml— a comment citing the shellcheck apt install as its precedent; that precedent no longer exists and was never sound.Fail-closed properties, both mutation-proven
The old one-liner had neither:
/bin/sh= dash on Debian, which has nopipefail— the exact chore(devex): Makefile with a uniform 'make check' (backend#1606) #630 hazard the Makefile already warns about. The list is materialised first.Verification
make checkgreen under bash and dash, 9.9 s against a 60 s budget.make parse,make shellcheckgreen under both shells; derived set byte-identical to an independently computed one (42 files).actionlintclean on all three touched files. (It reports pre-existing info-level findings inrelease-helm-chart.yaml/windows-e2e.yaml, untouched here — that is backend#1776.)scripts/file changed, soscripts/manifest.sha256needs no regeneration;gen-manifest.sh --checkpasses as part ofmake check.Deliberately not in scope
timeout-minutes: 15at least turns a stall into a bounded failure instead of a 6-hour one.quality / gitleaks, tracked separately.🤖 Generated with Claude Code
Note
Low Risk
Changes are limited to CI workflows and Makefile lint orchestration; behavior shifts toward broader shellcheck coverage with no application runtime impact.
Overview
Consolidates shellcheck so CI stops
apt-get install shellcheck(preinstalled on runners; those installs hung jobs for 35+ minutes). The duplicate, non-required shellcheck step is removed from installer Static analysis; required Lint runsmake lintonly.make lintnow means parse + derived whole-tree shellcheck: the old enumeratedSHELLCHECK_FILESlist is gone in favor of the same classification rule as orgquality / shellcheck(extensions/shebangs, error severity, SC1091 excluded). That pairs with the diff-scoped org check so every push still gets a full-tree sweep without maintaining three file lists.Fail-closed lint:
parseandshellcheckmaterialize file lists, refuse zero files, and propagate real exit codes under dash (fixes fail-openfind | xargsbehavior).CI resilience: job
timeout-minuteson standard checks; bats install is bounded, retried, and usessudo timeoutin standard-checks and installer unit-bash (15-minute job budget). helm-ci comment updated to drop the invalid shellcheck-install precedent.Reviewed by Cursor Bugbot for commit 9d811b3. Bugbot is set up for automated code reviews on this repo. Configure here.