fix(build-jupyter-cache): make container-mode failure alerting actually work (#83) - #122
Merged
Merged
Conversation
…ly work Closes the alerting half of #83. Three independent bugs sat on the same 14-line path, and because that path only runs once something else is already broken, none of them were ever caught. 1. The step reached its script through `${{ github.action_path }}`, which expands to the RUNNER's path (/home/runner/work/_actions/...). In a job `container:` the action is mounted at /__w/_actions/..., so bash got a path that does not exist: "No such file or directory", exit 127, before the script ran (actions/runner#2185). Note $GITHUB_ACTION_PATH does NOT have this problem — env values go through StepHost.TranslateToContainerPath — which is why preview-netlify/preview-cloudflare are correct as written. 2. Neither container image installs `gh`, so fixing (1) alone would have died one line later on `gh issue list`. 3. `gh issue create --label` validates labels client-side and fails when they do not exist. Neither consumer has `build-failure` or `automated`, so even a hosted runner with `gh` would have failed. Replacing the script with actions/github-script (REST, already the house pattern in both preview actions) kills all three: no CLI, no action_path, and missing labels are created implicitly by the API. Also fixes a second silent-failure class in the same file. Composite steps carry an implicit success() in a plain `if:`, so once any earlier step failed — including the alerting step itself — `Fail if builds failed` skipped, and the job's only error was whatever incidental step happened to break. In lecture-dp run 26381283100 the visible red was the exit-127 from "Create failure issue" while the real build failure never surfaced at all. Both steps are now `always() && ...`. Alerting can no longer no-op: a new step asserts an issue was really filed and fails loudly otherwise, exposing the URL as `failure-issue-url`. Consumers need `issues: write` — templates/cache.yml already has it, and the README example now shows it, because a missing permission is now a loud, self-describing failure instead of silence. Adds `upload-failure-reports` (default true) passed to the inner build-lectures calls. #83 said no report was uploaded; more precisely it was reachable only buried inside the full `_build` artifact, and not at all with upload-artifact off. The issue body now names the artifacts actually produced and gives per-builder reproduce commands matching build-lectures' real flags. Adds bjc-fail-guard to the harness: a failing cache build must fail the step, report html-status=failure, and save no cache over the last good one. Issue filing itself stays out of the PR harness — it needs `issues: write` and would file real issues in this repo on every run — so that assertion belongs to the canary, which runs container-mode against @v0. The github-script logic was exercised against mocked octokit for all three paths: create, dedupe-comment, and a 403 producing an actionable error before rethrowing. Refs #83 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens build-jupyter-cache’s failure path so container-mode failures reliably surface (via REST-based issue filing) and can’t silently no-op, while also improving debuggability by surfacing per-builder execution reports.
Changes:
- Replaced the failure-issue bash/
ghpath with anactions/github-script(Octokit/REST) implementation and added a postcondition check that fails loudly if alerting didn’t actually file an issue. - Ensured failure-path steps run under
always()so the real build failure is not masked/skipped after earlier step failures. - Added
upload-failure-reportsinput (defaulttrue) and a CI harness job to assert failure-path contract (fails the step, correct status outputs, no cache overwrite).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the failure-alerting fixes, new input, and new CI harness coverage. |
build-jupyter-cache/scripts/create-failure-issue.sh |
Removes the old gh-based issue creation script. |
build-jupyter-cache/README.md |
Updates inputs/outputs docs and clarifies required issues: write permission and artifact/debug guidance. |
build-jupyter-cache/action.yml |
Implements REST-based issue filing, adds verification guard, propagates upload-failure-reports, and makes failure steps always()-safe. |
.github/workflows/test-actions.yml |
Adds bjc-fail-guard harness job asserting the failure-path contract. |
…escription Copilot review nit. Apostrophes are literal inside a `>-` folded block, so no escaping is needed and the description still parses. Refs #83 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Aug 5, 2026
Copilot review. Closing the backlog row left two other places still asserting the opposite, which is the exact failure mode this repo keeps hitting — a document that contradicts itself one section apart. "Current state" said container-mode alerting was broken and pointed at "the P0 item below", which the same PR had just marked empty. Replaced rather than deleted: there IS still a gap in that area — a setup-environment failure alerts nobody, because all-passed ends up unset rather than 'false' and every guard skips (#123). The #83 disposition row was wrong twice over. It claimed the script-not-found 127 was already fixed; it was not — that was the PRIMARY bug, and #122 fixed it. And it listed the gh gap and missing reports as remaining, both of which #122 closed. Corrected to record what the three bugs actually were, so a reader does not inherit the old model. "Last updated" bumped to August 2026. The "Backlog (July 2026 review)" heading stays: it names when the review happened, which is accurate provenance rather than staleness, and individual items carry their own closure notes. Refs #83, #122, #123 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Aug 5, 2026
Found by sweeping the file rather than by review. Backlog item 14 proposes hardening create-failure-issue.sh, but #122 deleted that script outright; every concern it listed is structurally gone under actions/github-script. My own corrected #83 row also pointed at item 14 as remaining work, which was wrong. Also fixes a reference to "the P0 item above" that dangled once P0 emptied. Refs #83, #122 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Aug 5, 2026
* docs(plan): close the P0 alerting item, and correct its diagnosis #122 fixed container-mode failure alerting, so PLAN's P0 section — "broken safety net" — is now empty. Leaving it as-is overstates the repo's risk. Worth more than a strikethrough: the item's stated diagnosis was wrong. Missing `gh` was real but never reached. The step invoked its script through `${{ github.action_path }}`, which expands to the runner's path, and in a container job the action is mounted at /__w/_actions/... — so bash got a nonexistent path and exited 127 before the script ran. A third bug nobody had noticed broke alerting on hosted runners too: `gh issue create --label` validates labels client-side and no consumer has `build-failure` or `automated`. Each remedy the item proposed would have fixed only one of the three, and the suggested container test asserting `gh` is on PATH is now moot. Recording that so the next person reading a closed item does not inherit the wrong model. P1 item 2 is closed the same way, with the correction that the execution reports were not absent before — only undiscoverable, buried in the full _build artifact. Refs #83, #122 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(plan): reconcile the rest of PLAN with the closed P0 Copilot review. Closing the backlog row left two other places still asserting the opposite, which is the exact failure mode this repo keeps hitting — a document that contradicts itself one section apart. "Current state" said container-mode alerting was broken and pointed at "the P0 item below", which the same PR had just marked empty. Replaced rather than deleted: there IS still a gap in that area — a setup-environment failure alerts nobody, because all-passed ends up unset rather than 'false' and every guard skips (#123). The #83 disposition row was wrong twice over. It claimed the script-not-found 127 was already fixed; it was not — that was the PRIMARY bug, and #122 fixed it. And it listed the gh gap and missing reports as remaining, both of which #122 closed. Corrected to record what the three bugs actually were, so a reader does not inherit the old model. "Last updated" bumped to August 2026. The "Backlog (July 2026 review)" heading stays: it names when the review happened, which is accurate provenance rather than staleness, and individual items carry their own closure notes. Refs #83, #122, #123 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(plan): item 14 is moot — the script it hardens was deleted Found by sweeping the file rather than by review. Backlog item 14 proposes hardening create-failure-issue.sh, but #122 deleted that script outright; every concern it listed is structurally gone under actions/github-script. My own corrected #83 row also pointed at item 14 as remaining work, which was wrong. Also fixes a reference to "the P0 item above" that dangled once P0 emptied. Refs #83, #122 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 2026
mmcky
added a commit
that referenced
this pull request
Aug 6, 2026
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>
mmcky
added a commit
that referenced
this pull request
Aug 6, 2026
…#123) (#127) * fix(build-jupyter-cache): alert when the run aborts before the builds (#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> * docs: reference PR #127 alongside issue #123 in PLAN and CHANGELOG 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> * test(harness): explain why bjc-abort-guard uses a missing env file 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> * docs: correct three stale statements about the new guard mechanism 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
mmcky
added a commit
that referenced
this pull request
Aug 7, 2026
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>
This was referenced Aug 11, 2026
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.
Closes the alerting half of #83. Part 1 of that issue (sub-actions pinned
@main) was already fixed; parts 2 and 3 are here.Three bugs, one 14-line path
The path only runs once something else has already broken, which is why three independent defects accumulated on it unnoticed.
${{ github.action_path }}expands to the runner's path (/home/runner/work/_actions/...). Inside a jobcontainer:the action is mounted at/__w/_actions/..., so bash got a nonexistent path —No such file or directory, exit 127, before the script ranghgh issue create --labelvalidates labels client-side and fails when they don't exist. No consumer hasbuild-failureorautomatedghpresentBug 3 is new — it isn't in #83, and it means the alerting was broken on every runner type, not just in containers.
actions/github-script(REST, already the house pattern in both preview actions) kills all three: no CLI, noaction_path, and the API creates missing labels implicitly.A correction to the folk fix, so nobody "fixes" the wrong thing later:
$GITHUB_ACTION_PATHdoes not have bug 1. Env values are rewritten byStepHost.TranslateToContainerPathbeforedocker exec, so the env-var form arrives correctly as/__w/....preview-netlifyandpreview-cloudflareuse exactly that form and are correct as written — verified live, canary run 30971787039 executedbash "$GITHUB_ACTION_PATH/../scripts/detect-changed-lectures.sh"insidecontainer: quantecon-buildand it worked.The second silent-failure class
Composite steps carry an implicit
success()in a plainif:. So once any earlier step failed — including the alerting step itself —Fail if builds failedsilently skipped, and the job's only error was whatever incidental step happened to break.In lecture-dp run 26381283100 the visible red was the exit-127 from "Create failure issue", while the actual build failure never surfaced at all. Both steps are now
always() && ….Alerting can no longer no-op
A new step asserts an issue was genuinely filed and fails the job with an explicit error otherwise, exposing the URL as a new
failure-issue-urloutput. The defect class here was silence, so the postcondition is asserted rather than trusted — this also survives someone later addingcontinue-on-errorto the alert step.issues: writewhilecreate-issue-on-failureistrue.templates/cache.ymland the canary already declare it. A missing permission is now a loud failure whose message names the fix, instead of silence — that is the intended behaviour change, and the README example now shows the permissions block (it previously had none at all).upload-failure-reportsNew input, default
true, passed to the three innerbuild-lecturescalls. #83 says no report was uploaded; more precisely it was reachable only buried inside the full_buildartifact — hundreds of MB including.jupyter_cache— and not at all whenupload-artifactwas off, while the issue body said "download the build artifact for detailed execution reports" and named nothing.The body now names the artifacts actually produced and gives per-builder reproduce commands matching
build-lectures' real flags (the old text printedjb build <dir> --builder jupyter, which is simply wrong — that builder is--builder=custom --custom-builder=jupyter -n).The default deliberately differs from
build-lectures' ownfalse: that action is driven by a human watching a PR; this one runs unattended, typically weekly.Evidence this was real, not theoretical
The canary repo's
cache.yml(container mode,create-issue-on-failure: true) failed eight consecutive weeks, 2026-02-22 → 2026-04-12, filing zero issues — whilelinkcheck.yml, which uses a REST-API action rather thangh, filed a report every one of those weeks. Same repo, same period, same token.Verification
build-lectures' actualcasestatement.bjc-fail-guard: a failing cache build must fail the step, reporthtml-status=failure, and save no cache over the last good one. That failure path had no coverage at all.issues: writeand would file real issues in this repo on every run. That assertion belongs to the canary, which runs container-mode against@v0.Still open on #83
Setup environmenthas nocontinue-on-error, so if it fails the composite aborts beforeverify-buildsruns,all-passedis empty, and the alert guard's== 'false'is never true. Container-pull failures and image-tag rot therefore still alert nobody. Worth its own issue rather than widening this one.Refs #83
🤖 Generated with Claude Code