fix(ci): notify success on releases that skip CLI tests#1484
fix(ci): notify success on releases that skip CLI tests#1484mishushakov wants to merge 1 commit into
Conversation
report-success used `if: needs.publish.result == 'success'`, which lacks a status-check function. When cli-tests is skipped (a release that doesn't bump the CLI), GitHub Actions skip propagation cascades through the dependency graph and skips report-success too — so no Slack success notification fires, even though publish succeeded. Guard the condition with `(!cancelled())` (the same pattern the publish job already uses) so the `if` is actually evaluated regardless of upstream skips, while still only notifying when publish succeeded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit beb6a10. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from 872fbd2. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.31.1-mishushakov-notify-success-not-triggered.0.tgzCLI ( npm install ./e2b-cli-2.13.1-mishushakov-notify-success-not-triggered.0.tgzPython SDK ( pip install ./e2b-2.30.0+mishushakov.notify.success.not.triggered-py3-none-any.whl |
There was a problem hiding this comment.
LGTM — one-line CI fix that mirrors the existing (!cancelled()) guard already used by the publish job.
Extended reasoning...
Overview
This PR modifies a single if condition in .github/workflows/release.yml for the report-success Slack notification job, adding (!cancelled()) && before needs.publish.result == 'success'. The change addresses a known GitHub Actions skip-propagation behavior where a job's if is not meaningfully evaluated when an upstream needed job (here, cli-tests) is skipped — even if other needs succeeded.
Security risks
None. This is a CI workflow change that only affects when a Slack notification job fires. It does not touch authentication, credentials, publish logic, or any package code. The Slack webhook secret usage is unchanged.
Level of scrutiny
Low. This is a mechanical one-line fix to a notification job, with strong precedent in the same file: the publish job already uses the same (!cancelled()) && ... pattern (line ~128) for the same reason. The PR description includes empirical evidence (run IDs) showing the exact correlation between skipped cli-tests and skipped report-success, and the fix preserves the original intent (needs.publish.result == 'success').
Other factors
The changeset-bot warning about no changeset is expected and correct — CI workflow changes don't ship to users and don't need a version bump. No prior reviewer comments are outstanding. The PR is self-contained and reversible.
What
report-success(the Release Succeeded Slack notification) silently skips on releases that don't bump the CLI — even when the release publishes successfully.Why
The job used:
That
ifcontains no status-check function (always(),!cancelled(),failure(),success()). Whencli-testsis skipped — which happens whenever the changeset releases the SDKs but not the CLI (cli-testshasif: needs.preflight.outputs.cli == 'true') — GitHub Actions skip propagation cascades through the dependency graph and skipsreport-successtoo, before its condition is meaningfully evaluated. So no success notification fires.The
publishjob avoids this exact trap because itsifalready starts with(!cancelled()), which is whypublishruns (and succeeds) regardless.report-successjust lacked the same guard.Evidence
report-successskipped iffcli-testsskipped, across recent releases:cli-testsreport-successFix
(!cancelled())disables skip propagation so the condition is always evaluated, whileneeds.publish.result == 'success'preserves the original intent: notify only when the publish actually succeeded.report-failure(if: failure()) andreport-startare unaffected — both already evaluate correctly.🤖 Generated with Claude Code