fix(build-jupyter-cache): alert when the run aborts before the builds (#123) - #127
Conversation
…#123) `Setup environment` has no `continue-on-error`, so a failure there aborts the composite before `verify-builds` runs. Its `all-passed` output is then `''`, not `'false'` — and all three downstream guards tested `== 'false'`. The alert step, the postcondition assert added in #122, and the fail step therefore all skipped: an unattended weekly build could go red with nothing reaching the tracker. That is the same silence #83 was about, on the one path #122 did not cover. It also affects the input-validation steps above `Setup environment`, which have the identical hole. Resolve the overall status in a single `always()` step that maps "verify-builds never ran" to `false`, and have every guard test `!= 'true'` rather than `== 'false'`, so the failure path fails safe — it alerts on any abort, not only on anticipated ones. `build-success`/`cache-saved` consequently report `false` where they used to report `''`, a value no caller could distinguish from success. The `continue-on-error: true` on `Setup environment` sketched in the issue was not taken: it would let all three builds run against a broken environment, burning the build timeout to file an issue whose table blames the lectures for what was an environment failure. Two follow-on defects on the same path go with it. The issue body listed a `build-cache-<run-id>` artifact whenever `upload-artifact` was on, but that upload is correctly skipped when nothing was built — it now keys off the upload step's `artifact-id`, which is also empty when `if-no-files-found: warn` uploads nothing. Its "no artifacts" fallback blamed both upload inputs being off when the real reason was that no build ran. The issue body and the job summary now name the failing phase and distinguish a builder that was considered and skipped from one that was never run. New `bjc-abort-guard` harness job covers both abort shapes. Whether an issue is actually filed stays canary-only — that needs `issues: write`, which would open real issues in this repo on every PR run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches the convention the #122 entries use, where the parenthesised number is the PR that landed the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the build-jupyter-cache composite action’s failure alerting by ensuring an aborted run (e.g., invalid builders input or setup-environment failure) still resolves a definitive overall failure status, so downstream guards reliably file/verify alerts and fail the job.
Changes:
- Add an
always()“Resolve overall status” step that maps “verify-builds never ran” toall-passed=false, and invert downstream guards to test!= 'true'. - Improve failure reporting UX by distinguishing “skipped” vs “not run” in the failure issue body and step summary, and by gating artifact mentions on whether an artifact was actually created.
- Add a new harness job (
bjc-abort-guard) to cover abort-before-build paths and assertbuild-success == 'false'.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
PLAN.md |
Updates project plan/status notes to reflect the abort-path alerting fix and related historical corrections. |
CHANGELOG.md |
Documents the abort-path guard fix and associated alert/reporting improvements. |
build-jupyter-cache/README.md |
Clarifies output contracts for build-success/builder statuses when the run aborts before builds. |
build-jupyter-cache/action.yml |
Implements the new overall-status resolution step, inverts guards, and improves issue/summary rendering for abort cases. |
.github/workflows/test-actions.yml |
Adds bjc-abort-guard harness coverage for pre-build/setup abort scenarios. |
setup-environment validates the environment file's existence before touching conda, so the abort lands in seconds and no conda cache is restored or saved. An unsolvable environment.yml would be a closer analogue of the real trigger in #28, but it would sit in the solver for minutes and leave a junk cache entry on every run. Record that so it does not get "improved" later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note for whoever picks this up: GitHub is in an Actions major outage (incident opened 2026-08-06 15:22 UTC), and The harness was therefore run manually instead, via |
All three land on the same slip: the comments were written when the artifact list gated on the upload step's outcome, then the implementation moved to its artifact-id output and the prose stayed behind. - The comment explaining the #123 mechanism said verify-builds "carries a bare `if:`". It has no `if:` key at all — it is skipped because a composite step without always() is skipped once an earlier step has failed. The mechanism was right, the step description was not, in the one comment whose whole job is explaining the bug. - The upload-artifact comment and the CHANGELOG entry both said the issue body keys off the step's outcome. It keys off artifact-id, and the difference is load-bearing: `if-no-files-found: warn` lets the step succeed having uploaded nothing, which is exactly the case outcome would have missed. Comment-only; no behaviour change. Raised by Copilot on #127. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Minor, not patch: `upload-failure-reports` is a new input on build-jupyter-cache, so this is more than bug fixes even though the headline changes are two alerting fixes. Also merges the two separate `### Fixed` blocks the Unreleased section had accumulated — the #123 work was written above the #122 entries rather than into them — and orders the merged block oldest-first, so the #123 entry reads after the #122 entries it refers back to. Contents: the container-mode alerting fix (#83, #122), the abort-before-builds fix (#123, #127), the harness relevance gate (#116), and container smoke tests that can actually fail (#108). CONTRIBUTING step 5 (drop @v0 workarounds this release makes obsolete): none outstanding — the `@v0` mentions in test-actions.yml are comments describing the structural limitation, not workarounds. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #123.
The bug
Setup environmenthas nocontinue-on-error, so a failure there aborts the composite beforeverify-buildsruns.verify-buildscarries a bareif:, which in a composite means an implicitsuccess(), so it never executes and itsall-passedoutput is''— not'false'. Every downstream guard tested== 'false', so on that path all three skipped: the alert step, the "Verify the failure alert was filed" assert added in #122, andFail if builds failed. An unattended weekly build could go red with nothing reaching the tracker, which is the same silence #83 was about, on the one path #122 did not cover.Verified against the code rather than from an incident — the canary's silent weeks in Feb–Apr were build failures, so this variant has not been observed live. The likeliest real trigger is an
environment-updatethat no longer solves, given the conda pinning churn tracked in #28.What changed, and why not the fix the issue sketched
The issue proposed
continue-on-error: trueonSetup environmentplus a guard step. That was rejected: with setup no longer failing the job, all three builds would run against a broken environment, burning the build timeout to produce an issue whose table readshtml: ❌ failure— pointing the maintainer at a lecture bug when the cause was conda. It also only fixes the one step.Instead the guard polarity is inverted. A new
Resolve overall statusstep runs underalways()and maps "verify-builds never ran" tofalse, and every guard now tests!= 'true'rather than== 'false'. The failure path fails safe: it alerts on any abort, not only on the ones someone anticipated. That matters because the gap is wider thansetup-environment—Generate cache key,Validate builders inputandParse builders inputsit above it with the identical hole, and a badbuildersvalue on an unattended run was equally silent. The file already used the safe idiom in two places (Save build cacheis== 'true', the artifact upload is!= 'true'); the alerting guards were the outliers.This also gives
build-successandcache-savedan honest contract. They previously returned''on a setup failure, which no consumer could distinguish from success; they now return'false'.Two follow-on defects on the same path
The issue body's artifact list was gated on
inputs.upload-artifact, but the upload step is correctly skipped when nothing was built — so the issue named abuild-cache-<run-id>artifact that does not exist and sent the maintainer looking for a download that was never uploaded. It now keys off the upload step'sartifact-idoutput, which is documented as empty unless an artifact was really created; that also covers the case whereif-no-files-found: warnsucceeds having uploaded nothing.Its "no artifacts" fallback asserted that
upload-artifactandupload-failure-reportswere both false, when on this path the real reason is that no build ran.Both the issue body and the job summary now name the failing phase, and distinguish a builder that was considered and skipped (⏭️) from one that was never run (⚪) — previously an aborted run rendered three blank cells in the summary table and three misleading "skipped" rows in the issue.
Testing
New
bjc-abort-guardharness job, covering both abort shapes: an invalidbuildersinput (aborts in this repo's own code, no conda, seconds) and a nonexistent environment file (aborts insidesetup-environment@v0, the shape the issue reported). Its central assertion isbuild-success == 'false'— the value all three guards read, and the exact thing that was''before this change, so the job fails onmaintoday.Scope stated honestly, matching the existing note on
bjc-fail-guard: this cannot assert that an issue was actually filed. That needsissues: writeand would open real issues in this repo on every PR run, so it stays with the canary. What it does assert is the decision every guard depends on.Also exercised locally before pushing: the resolve step across all four paths (
build-pass,build-fail,setup,pre-build), and the rendered issue body and job summary for both the build-failure and setup-failure cases. The build-failure body is byte-identical to today's apart from the new**Failed during:**line.Residual, deliberately out of scope
If the job itself never starts, or is cancelled, timed out, or loses its runner, no step runs at all —
always()included — so nothing can be filed from inside the action. Covering that needs a workflow-levelif: failure()notify job in the consumer repo, or a scheduled sweeper. Recorded in PLAN.md so it is not rediscovered later as a fourth variant of #83.PLAN.md
Updated for this fix, and three unrelated facts that had gone stale were corrected while there: the latest release is
v0.10.0, notv0.8.0; the container science stack moved to the anaconda 2026.06 baseline in #95, not 2025.12; and the canary is on a floating@v0rather than a stale, dormant@v0.6.0. The #122 closure note claimed alerting "cannot silently no-op" — #123 disproved that, so it is corrected rather than left standing.🤖 Generated with Claude Code