discord: retry the webhook instead of dropping the message - #7
Merged
Merged
Conversation
A reusable workflow is job-level, so it cannot be a step: that is why gaia had to ship the same retry logic as a script and check it out to run it. A composite action can be a step and the runner fetches it, so callers need no checkout, no sparse-checkout and no continue-on-error trap around one. The retry engine is gaia's, unchanged in behaviour: honour retry_after and X-RateLimit-Reset-After, bounded 5 attempts / 30s per wait / 60s total, --max-time so a hung connection cannot stall a job, no retry on a non-429 4xx, and a non-zero exit when the message never lands. Two things are new, both because an action has an interface where a script had argv. `content` takes plain text and does the JSON encoding, so no caller writes jq to get a title with a quote in it into valid JSON; `payload` still takes a whole object for embeds. And fail-on-undeliverable makes gaia's exit-code split declarative instead of a trailing `|| true` — a notifier that is the deliverable may fail, one reporting someone else's failure may not, and the ::error:: annotation fires either way. Inputs cross into the script through env only. A PR title is attacker-controlled text and must never be interpolated into a shell body. Replayed against a stubbed curl and sleep: 45 assertions covering gaia's original 11 cases plus content encoding, payload validation, the undeliverable switch and the delivered output. shellcheck 0.11.0 clean. No request reached Discord; the harness webhook is example.invalid.
Both workflows ended in a bare `curl`, so a rate-limited notification printed Discord's refusal into the log and the step went green having sent nothing. This is the same defect as gaia#2409, in the repo gaia calls for it. The `workflow_call` interface does not move — same inputs, same `discord_webhook` secret — so pinned callers are unaffected. What changes is that an undeliverable message now reddens the run. Sending it is these workflows' whole purpose; nothing else would record that it vanished. The action is referenced by full path and SHA rather than a local `./discord-notify`. A relative action path inside a reusable workflow resolves against the CALLER's workspace, which never has this repo checked out, so the self-reference has to go through the registry. That is also why the pin is a SHA: a caller who pins this workflow would otherwise get a floating action underneath it. Message building keeps its own step and hands the text over through GITHUB_OUTPUT with a random delimiter, so a title cannot close the heredoc and inject further outputs. `github.event.action` and `.pull_request.merged` move into env with the rest — they were interpolated straight into the shell body. Since `content` now does the JSON encoding, both call sites lose their jq invocation.
The runner evaluates input descriptions, so the sentence explaining that a composite action cannot use the secrets context named it in expression syntax and failed the manifest before the action ran: action.yml (Line: 8, Col: 18): Unrecognized named-value: 'secrets' The advice was right; writing it as a live expression was not. It now names secrets.DISCORD_WEBHOOK_URL as plain text and says why the wrapper is missing, so the next person does not restore it. No local check could have caught this. The harness exercises the script through a stubbed curl, and the manifest was only checked for YAML validity — it parses fine and is still rejected by the expression evaluator. CI was the first thing to actually evaluate it.
Both notifiers pinned fafee57, the commit whose action.yml the runner rejects, so fixing the manifest alone left them fetching the broken one. They now pin the fix. A pin inside this repo has to be repointed by a follow-up commit rather than an amend: the SHA has to exist before anything can reference it, and the earlier commits are already pushed.
bjornars
marked this pull request as ready for review
August 28, 2026 12:01
Contributor
Author
|
I think we'll just have to try this live |
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
1 participant
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.
Fixes #6
Both notifiers ended in a bare
curl, which exits 0 whatever Discord answered, so a 429 printed the refusal into the log and the step went green with the message thrown away. The retry now lives in a composite action,discord-notify, and both workflows route through it.A composite action rather than a script because it can be a step and the runner fetches it — no checkout. That is what lets novem-code/gaia#3905 delete its own
.github/scripts/discord-notify.shand both of its sparse-checkouts and call this instead. The retry engine is that PR's, behaviour unchanged:retry_after/X-RateLimit-Reset-After/Retry-Afterin descending authority, 5 attempts, 30s per wait, 60s total,--max-time 15, non-429 4xx not retried.The interface
contenttakes plain text and the action does the JSON encoding, which deletes thejq -n --argblock from every call site and with it the class of bug where a title containing a quote produced invalid JSON.payloadremains for embeds. Both, neither, or invalid JSON fails before any request is made.fail-on-undeliverableexists instead of the caller writing|| true, and deliberately not instead ofcontinue-on-error.continue-on-error: truestill renders the step as failed, which reintroduces the exact problem being fixed — a notifier reporting someone else's failure going red reads as "the build broke at the notify step". The switch exits 0 instead. The::error::annotation fires either way, so the loss is on the run summary regardless of which you choose.Where to spend attention
This changes observable behaviour for every repo calling these two workflows. An undeliverable notification now reddens a run that would be green today. I think that is right — these workflows exist only to notify, so nothing else records that the message vanished — but it is the one thing to agree with rather than discover. Note
discord.ymlstill carries adummyjob named "Make sure the action is always successful", which suggests someone once wanted the opposite; left alone.Merge, do not squash. The workflows pin the action by full SHA at
fafee578, and that commit has to stay reachable frommainor the pin inside the shipped workflows breaks — along with gaia's pin to the same SHA. That is also why this is two commits rather than one: the pin needs a SHA that already exists. If this branch is rebased the SHA moves and commit 2's pin goes stale, so say so rather than hand-patching it.The self-reference goes through
novem-code/github-actions/discord-notify@<sha>rather than a relative./discord-notify, because a relative action path inside a reusable workflow resolves against the caller's workspace, which never has this repo checked out.The same retry policy also exists in Python, in gaia's
ops-announce/discordsender (novem-code/gaia#4006) — one retry, a 5s wait capped at 30s, and 5xx not retried; the numbers differ deliberately, because that one blocks a rabbit consumer callback where this one only delays a CI step.Verification
45 assertions, 0 failures, against a stubbed
curlandsleep: all 11 cases from novem-code/gaia#3905 replayed unchanged, plus content encoding round-trip (quotes, newline, backslash), payload passthrough, both-inputs, neither-input, invalid JSON, missing webhook, and the undeliverable switch swallowing a 429 and a 400 while still annotating. shellcheck 0.11.0 exit 0,bash -nclean. No request reached Discord — the harness webhook isexample.invalid. The harness is deliberately not committed: it stubscurlonPATH, which is a sharp tool to leave in a repo.The
workflow_callinterface of both workflows is byte-identical tomain, so existing pinned callers are unaffected.What the harness could not check: it exercises
discord-notify.shthrough a stubbedcurl, andaction.ymlwas only validated as YAML. A manifest can parse cleanly and still be rejected by the runner's expression evaluator — which is exactly what happened, on the${{ }}inside an input description — so the manifest is CI-verified, not harness-verified. Read the first greennotify_prrun as the real evidence for it.Review the
fail-on-undeliverablesemantics and the reddening behaviour first; the retry engine itself arrived already tested.