fix(tests): install-journey guard() asserted nothing + CI-config gaps (#310)#311
Merged
Merged
Conversation
… label trigger (#310) The install-journey harness merged in #214 asserted nothing: guard() captured `local rc=$?` inside the `then` branch of `if ! timeout ...`, so rc read the NEGATED status (always 0). Every assertion runs under guard, so set -e never tripped and the job printed E2E JOURNEY PASS even when steps failed or hung. Reproduced: a step exiting 3 and a 124 hang both returned 0. - guard(): run the command, capture status via `|| rc=$?`, then act on it — a failed step now propagates, a 124 hang is treated as a hard failure. - Add a negative-control self-test at startup: guard() must return non-zero for a command that exits non-zero, or the script refuses to run. Locks the vacuous-pass class so it can't regress silently (bash -n and shellcheck both passed on the buggy version). - installer-tests.yaml: add timeout-minutes to path-persist (20) and e2e-journey (30) — every other job already had one; and add `labeled` to the pull_request trigger types so adding the `e2e` label starts a run immediately instead of on the next push. Remaining from the #214 review (path-persist has no red/green signal) is tracked separately on #310 — it's a genuine rewrite that needs CI validation across the distro matrix. Refs #310. Follow-up to #214 (review by @aptracebloc). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aptracebloc
approved these changes
Jul 7, 2026
This was referenced Jul 7, 2026
divyasinghds
pushed a commit
that referenced
this pull request
Jul 7, 2026
…316) The guard() watchdog added in #310/#311 wraps steps in `timeout`, which execs an external command in a fresh process and cannot see shell functions. create_cluster is a function sourced from cluster.sh, so `guard 600 "create_cluster" -- create_cluster` failed with "timeout: failed to run command 'create_cluster': No such file or directory" (exit 127) before the cluster was ever created — failing the E2E last-mile journey job. Run create_cluster inside a real `bash` (which timeout CAN exec) that re-sources the libs. Exported CLUSTER_NAME / TRACEBLOC_NO_AUTOSTART / USER carry through the env, and the CLI binaries install_* dropped into /usr/local/bin are on the inherited PATH. Refs #310 Co-authored-by: Claude Opus 4.8 <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
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.
Follow-up to #214 (post-merge review by @aptracebloc). Fixes the findings that are solidly correct and locally reasoned; the path-persist signal rework is scoped separately on #310 (needs CI validation across the distro matrix).
🔴 Critical —
guard()swallowed every failure (Leg 2 asserted nothing)scripts/tests/e2e-journey.shcapturedlocal rc=$?inside thethenbranch ofif ! timeout .... In bash,$?there is the negated status — always0. Since every assertion (cluster info,dataset push --dry-run) runs underguard,set -enever tripped and the job printedE2E JOURNEY PASSeven when steps failed or hung.Reproduced before the fix — a step exiting 3 and a 124 hang both made
guardreturn0.rc=0; timeout ... || rc=$?; return $rc— a failed step now propagates and a 124 hang is a hard failure.guardmust return non-zero for a command that exits non-zero, else the script refuses to run.bash -nand shellcheck both passed on the buggy version, so this locks the class.🟡 Medium — CI config
timeout-minutestopath-persist(20) ande2e-journey(30). Every other job in the file had one; these two rode to the 6h ceiling on a hang.labeledto thepull_requesttrigger types so adding thee2elabel starts ane2e-journeyrun immediately instead of only on the next push.Deferred to #310 — 🟠 path-persist has no red/green signal
Not in this PR: it's a real rewrite, not a one-liner. The cli
install.shpersists PATH to one rc file keyed off$SHELL(Linux bash→.bashrc, zsh→.zshrc, fish→config.fish, else.profile), and only when the prefix is under$HOME/off-PATH. As root the test installs to/usr/local/bin(already on PATH → persistence never exercised), and thelogin × nonloginmatrix doesn't match the installer's model (bash -creads no rc files at all). Making it actually distinguish a pre-fix vs fixed installer means forcingINSTALL_PREFIX=$HOME/.local/bin, installing once per$SHELL, and asserting an interactive shell of that kind — which can only be validated in CI. Tracked on #310.Test plan
guard()bug and verified the fix: self-test detects a non-zero exit; a failing guarded step aborts with its true exit code (SHOULD-NOT-PRINT line absent).bash -n scripts/tests/e2e-journey.sh— parses.shellcheck --severity=error(the CI gate) and--severity=warning(advisory) on the script + libs — both clean.actionlint .github/workflows/installer-tests.yaml— clean.Refs #310.
🤖 Generated with Claude Code
Note
Medium Risk
The guard fix restores real failure semantics for the labeled e2e journey job; CI-only changes with no production runtime impact, but a previously green e2e job may now correctly fail when steps break.
Overview
Fixes a critical bug in
e2e-journey.shwhereguard()always returned success: capturing$?insideif ! timeout ...read the negated status, so every guarded step (cluster info,dataset push --dry-run, etc.) could fail or hang while the job still printed E2E JOURNEY PASS. The watchdog now records the real exit viatimeout ... || rc=$?and returns it; exit 124 still fails hard on timeout.Adds a negative-control self-test at startup that refuses to run if
guarddoes not propagate a non-zero exit from a deliberate failure.In installer-tests.yaml:
timeout-minutesforpath-persist(20) ande2e-journey(30);pull_requesttrigger includeslabeledso adding thee2elabel starts the journey job without waiting for another push.Reviewed by Cursor Bugbot for commit 1652770. Bugbot is set up for automated code reviews on this repo. Configure here.