fix: Let a notify job survive an unreachable vault #67

ahmad · 2026-09-09T04:50:20Z

ahmad commented

2026-09-09 04:50:20 +00:00

Issue

Closes #65.

Problem

ahmad/trip run 2462: version succeeded, publish and deploy were skipped, and the run is recorded as a failure — because notify could not reach a secret. A release that was fine reads as broken, which is the opposite of what a notification is for.

The immediate cause of that run (a dead apt suite, then a missing jq) is already fixed by 2bccd5e. The shape is not: every notify job reaches OpenBao before it can say anything, and any failure along that path fails the run it is reporting on.

Solution

openbao-fetch gains required, default "true". With required: false a failed fetch writes an empty output file, emits a ::warning:: naming the reason, and exits 0. The template's notify job passes it; notify-telegram already treats an empty token as a skip.

Review notes

Why the default is fatal, with the survey rather than the instinct. Making the fetch non-fatal everywhere would have satisfied criteria 2 and 5 with no consumer edit at all, so I checked whether it was safe. It is not: I looked at what follows every use of this action across the fleet, and the deploy job of ahmad/trip, ahmad/portfolio, bayan/kidpoints, bayan/stem, suhba/khitta, suhba/suhba-web and ahmad/imamah has no guard — an empty .env would start containers with no DATABASE_URL and report success. Only expiro and larda fail loudly at the point of use. So the safe default is the one that stops.

shell: bash runs with -e already set, and that nearly shipped as a bug. Writing set -uo pipefail does not turn -e off; the script now says set +e explicitly. Without it the first failing command substitution exits before fail is reached, so required: false would have done nothing at all. Mutation-checked: removing set +e fails three behaviour cases.

Tested against stubbed commands, not just parsed. Six cases in the behaviour suite drive the real run: body with a curl that refuses everything and one that answers: the default stops with the reason on stderr, required: false warns and exits 0, and the value never appears on stdout or stderr in either outcome. Both halves mutation-checked — dropping set +e fails three, making fail() ignore required fails three.

The harness found a dependency I had added without noticing. My first version used mktemp for the atomic write; the minimal PATH refused it, which is exactly the question worth asking in a repo that has been removing runtime dependencies (jq, the apt install). It writes to "$OB_OUT.partial" instead and moves it into place.

Criterion 5, stated rather than claimed. The central change reaches every consumer through the @v3 tag. The tolerance half needs each consumer's copied notify job to gain one line, because the job is copied and not called — I checked again that no consumer references a reusable workflow. Eight repositories have such a job. I have not edited them here; that is a fleet propagation and it belongs in its own change, listed on the follow-up.

Verified

check-actions, its own test suite, the behaviour suite and the README lint all exit 0.

Risks and trade-offs

A job that sets required: false and then genuinely needs the secrets will now fail later and less clearly than it would have. The README says where the flag belongs and the action's own input description repeats it, because this is a foot-gun that reads as a convenience.

The notify jobs in eight repositories keep today's behaviour until they are updated. That is not a regression — it is the current failure, unfixed for them — but it does mean this PR alone does not close the issue's fifth criterion for existing consumers, and I would rather say so than let the merge imply otherwise.

### Issue Closes #65. ### Problem `ahmad/trip` run 2462: `version` succeeded, `publish` and `deploy` were skipped, and the run is recorded as a **failure** — because `notify` could not reach a secret. A release that was fine reads as broken, which is the opposite of what a notification is for. The immediate cause of that run (a dead apt suite, then a missing `jq`) is already fixed by `2bccd5e`. The shape is not: every `notify` job reaches OpenBao before it can say anything, and any failure along that path fails the run it is reporting on. ### Solution `openbao-fetch` gains `required`, default `"true"`. With `required: false` a failed fetch writes an empty output file, emits a `::warning::` naming the reason, and exits 0. The template's `notify` job passes it; `notify-telegram` already treats an empty token as a skip. ### Review notes **Why the default is fatal, with the survey rather than the instinct.** Making the fetch non-fatal everywhere would have satisfied criteria 2 and 5 with no consumer edit at all, so I checked whether it was safe. It is not: I looked at what follows every use of this action across the fleet, and the `deploy` job of `ahmad/trip`, `ahmad/portfolio`, `bayan/kidpoints`, `bayan/stem`, `suhba/khitta`, `suhba/suhba-web` and `ahmad/imamah` has **no guard** — an empty `.env` would start containers with no `DATABASE_URL` and report success. Only `expiro` and `larda` fail loudly at the point of use. So the safe default is the one that stops. **`shell: bash` runs with `-e` already set, and that nearly shipped as a bug.** Writing `set -uo pipefail` does not turn `-e` off; the script now says `set +e` explicitly. Without it the first failing command substitution exits before `fail` is reached, so `required: false` would have done nothing at all. **Mutation-checked:** removing `set +e` fails three behaviour cases. **Tested against stubbed commands, not just parsed.** Six cases in the behaviour suite drive the real `run:` body with a `curl` that refuses everything and one that answers: the default stops with the reason on stderr, `required: false` warns and exits 0, and the value never appears on stdout or stderr in either outcome. **Both halves mutation-checked** — dropping `set +e` fails three, making `fail()` ignore `required` fails three. **The harness found a dependency I had added without noticing.** My first version used `mktemp` for the atomic write; the minimal PATH refused it, which is exactly the question worth asking in a repo that has been removing runtime dependencies (`jq`, the apt install). It writes to `"$OB_OUT.partial"` instead and moves it into place. **Criterion 5, stated rather than claimed.** The central change reaches every consumer through the `@v3` tag. The *tolerance* half needs each consumer's copied `notify` job to gain one line, because the job is copied and not called — I checked again that no consumer references a reusable workflow. Eight repositories have such a job. I have not edited them here; that is a fleet propagation and it belongs in its own change, listed on the follow-up. ### Verified `check-actions`, its own test suite, the behaviour suite and the README lint all exit 0. ### Risks and trade-offs A job that sets `required: false` and then genuinely needs the secrets will now fail later and less clearly than it would have. The README says where the flag belongs and the action's own input description repeats it, because this is a foot-gun that reads as a convenience. The `notify` jobs in eight repositories keep today's behaviour until they are updated. That is not a regression — it is the current failure, unfixed for them — but it does mean this PR alone does not close the issue's fifth criterion for existing consumers, and I would rather say so than let the merge imply otherwise.

ahmad added 1 commit

2026-09-09 04:50:20 +00:00

fix: Let a notify job survive an unreachable vault

PR / validate (pull_request) Successful in 17s

Details

51d52d7a2c

openbao-fetch gains a required input, default true. With required
false a failed fetch writes an empty output file, warns with the reason
and exits 0, so a vault blip no longer turns a build that was fine into
a red run - which is what a notify job needs and what eight repositories
currently get wrong.

The default stays fatal deliberately: a survey of every consumer found
most deploy jobs have nothing after the fetch that would notice an empty
.env, so a non-fatal default would start containers with no secrets and
report success.

shell: bash runs with -e already set, so the script turns it off
explicitly; without that the first failing command exits before the
handler is reached.

Closes #65

ahmad changed title from ~~WIP: fix: Let a notify job survive an unreachable vault~~ to fix: Let a notify job survive an unreachable vault

2026-09-09 04:50:41 +00:00

ahmad commented

2026-09-09 04:53:58 +00:00

Verification on 51d52d7. PR / validate green — which here means check-actions, its own test suite, the behaviour suite and the README lint.

Criterion 1 — Telegram unreachable leaves the conclusion unchanged. Already true before this change, and I checked rather than assumed: notify-telegram runs without set -e, treats an empty token as a skip, and turns a delivery failure into ::warning:: plus exit 0. Nothing in this PR touches it.

Criterion 2 — a failed secret fetch leaves the conclusion unchanged, with the reason visible. Driven against a stubbed curl that refuses everything: required: false exits 0 with ::warning::openbao-fetch: AppRole login failed on stdout, and the default still exits 1 with the reason on stderr. Both are cases in the behaviour suite rather than claims here.

Criterion 3 — jq absent is not fatal. Already satisfied by 2bccd5e, which replaced both parses with node. I asserted the opposite in an earlier comment on this issue, from a worktree four commits stale, and corrected it there.

Criterion 4 — a genuinely failed build still reports failure. Untouched: the status the notifier sends is computed from needs.*.result. A tolerant fetch cannot make a failed build look green, because the build's own jobs are what fail the run.

Criterion 5 — no consumer edit. Honestly: not fully met, and it cannot be by a change to this repository alone. The notify job is copied into each consumer rather than called, so required: false reaches a repository when its own workflow gains the line. Eight repositories are affected; every one is listed, with its exact workflows, on the follow-up issue below. What this PR does deliver to all of them through @v3 is the removal of the failure mode that has actually been firing.

What I checked before choosing the default, since it decides whether criterion 5 could have been met the easy way: the deploy job of trip, portfolio, kidpoints, stem, khitta, suhba-web and imamah has no guard after the fetch. A tolerant default would have started containers with no DATABASE_URL and reported success — a worse failure than the one being fixed.

Mutation-checked in two places. Removing set +e fails three cases; making fail() ignore required fails three. The harness also caught an mktemp I had introduced without noticing, in a repository that has been deliberately shedding runtime dependencies.

The propagation is filed as #68, with the eight repositories and their exact workflows.

Verification on `51d52d7`. `PR / validate` green — which here means `check-actions`, its own test suite, the behaviour suite and the README lint. **Criterion 1 — Telegram unreachable leaves the conclusion unchanged.** Already true before this change, and I checked rather than assumed: `notify-telegram` runs without `set -e`, treats an empty token as a skip, and turns a delivery failure into `::warning::` plus `exit 0`. Nothing in this PR touches it. **Criterion 2 — a failed secret fetch leaves the conclusion unchanged, with the reason visible.** Driven against a stubbed `curl` that refuses everything: `required: false` exits 0 with `::warning::openbao-fetch: AppRole login failed` on stdout, and the default still exits 1 with the reason on stderr. Both are cases in the behaviour suite rather than claims here. **Criterion 3 — `jq` absent is not fatal.** Already satisfied by `2bccd5e`, which replaced both parses with `node`. I asserted the opposite in an earlier comment on this issue, from a worktree four commits stale, and corrected it there. **Criterion 4 — a genuinely failed build still reports failure.** Untouched: the status the notifier sends is computed from `needs.*.result`. A tolerant *fetch* cannot make a failed build look green, because the build's own jobs are what fail the run. **Criterion 5 — no consumer edit. Honestly: not fully met, and it cannot be by a change to this repository alone.** The `notify` job is copied into each consumer rather than called, so `required: false` reaches a repository when its own workflow gains the line. Eight repositories are affected; every one is listed, with its exact workflows, on the follow-up issue below. What this PR does deliver to all of them through `@v3` is the removal of the failure mode that has actually been firing. **What I checked before choosing the default**, since it decides whether criterion 5 could have been met the easy way: the `deploy` job of `trip`, `portfolio`, `kidpoints`, `stem`, `khitta`, `suhba-web` and `imamah` has no guard after the fetch. A tolerant default would have started containers with no `DATABASE_URL` and reported success — a worse failure than the one being fixed. **Mutation-checked in two places.** Removing `set +e` fails three cases; making `fail()` ignore `required` fails three. The harness also caught an `mktemp` I had introduced without noticing, in a repository that has been deliberately shedding runtime dependencies. The propagation is filed as **#68**, with the eight repositories and their exact workflows.

ahmad_bot approved these changes

2026-09-09 05:18:07 +00:00

ahmad_bot left a comment

Approved. The reasoning behind the default is the substance of this change and it holds up — I re-ran your mutations and then widened your survey mechanically, which made the argument stronger rather than weaker.

The mutations, reproduced:

Dropping set +e → 3 failures, exactly as described. That is the one that nearly shipped as a no-op, and it is worth restating why: token=$(curl … | node …) under -e exits on the substitution's status, so fail is never reached and required: false would have done nothing at all while looking correct.
Making fail() always stop → 3 failures, your figure.
The other direction is thinner, and worth knowing. Making fail() always warn — a default silently flipping to non-fatal — fails one case, required by default: an unreachable vault stops the job. The predicate is pinned in both directions, but that single guard is carrying the whole weight of the decision below, and it is the direction whose failure is silent rather than loud.

Your survey is right and short by two. Walking every repository the API lists rather than a hand-kept list: eleven repositories use openbao-fetch, across thirty workflow files. ahmad/namey and suhba/suhba-docs are not in the PR's list, and both have a deploy.yml calling the action with no -z guard anywhere in the file — so the unguarded population is nine, not seven. Nothing about the conclusion changes; the safe default is even safer than the description argues.

And the action is called from more than deploy and release. monitoring.yml in ahmad/portfolio, ahmad/trip, ahmad/imamah, suhba/khitta and Bayan/kidpoints; pr.yml in ahmad/namey and ahmad/portfolio; docs-build.yml in suhba/suhba-docs; cms-pr.yml in suhba/suhba-web. A monitoring workflow going red because the vault blinked is the same shape as the notify case this PR exists for, so those are candidates for the follow-up propagation alongside the eight notify jobs — worth naming there so the next pass does not have to rediscover them.

The rest, checked:

[ "${OB_REQUIRED}" != "false" ] fails closed: any value that is not exactly false — a typo, False, 0, an empty string — is treated as required. That is the right polarity for a flag whose wrong answer is a silent deploy.
The template's grep "^$k=" .env 2>/dev/null | cut … || true genuinely tolerates an absent or empty file under set -euo pipefail; the || true covers the whole pipeline inside the substitution, so pipefail cannot resurrect it.
The warning text carries only fixed strings and OB_PATH, never a value, and the "never echoed" case asserts that on both streams.
"$OB_OUT.partial" instead of mktemp is the right call for the reason you give, and the harness catching it is the harness doing its job.

On not propagating here: agreed, and saying plainly that this alone does not close criterion 5 for existing consumers is worth more than a merge that implies otherwise.

Approved. The reasoning behind the default is the substance of this change and it holds up — I re-ran your mutations and then widened your survey mechanically, which made the argument stronger rather than weaker. **The mutations, reproduced:** - Dropping `set +e` → **3 failures**, exactly as described. That is the one that nearly shipped as a no-op, and it is worth restating why: `token=$(curl … | node …)` under `-e` exits on the substitution's status, so `fail` is never reached and `required: false` would have done nothing at all while looking correct. - Making `fail()` always stop → **3 failures**, your figure. - **The other direction is thinner, and worth knowing.** Making `fail()` always *warn* — a default silently flipping to non-fatal — fails **one** case, `required by default: an unreachable vault stops the job`. The predicate is pinned in both directions, but that single guard is carrying the whole weight of the decision below, and it is the direction whose failure is silent rather than loud. **Your survey is right and short by two.** Walking every repository the API lists rather than a hand-kept list: **eleven** repositories use `openbao-fetch`, across **thirty** workflow files. `ahmad/namey` and `suhba/suhba-docs` are not in the PR's list, and both have a `deploy.yml` calling the action with no `-z` guard anywhere in the file — so the unguarded population is **nine**, not seven. Nothing about the conclusion changes; the safe default is even safer than the description argues. **And the action is called from more than `deploy` and `release`.** `monitoring.yml` in `ahmad/portfolio`, `ahmad/trip`, `ahmad/imamah`, `suhba/khitta` and `Bayan/kidpoints`; `pr.yml` in `ahmad/namey` and `ahmad/portfolio`; `docs-build.yml` in `suhba/suhba-docs`; `cms-pr.yml` in `suhba/suhba-web`. A monitoring workflow going red because the vault blinked is the same shape as the notify case this PR exists for, so those are candidates for the follow-up propagation alongside the eight `notify` jobs — worth naming there so the next pass does not have to rediscover them. **The rest, checked:** - `[ "${OB_REQUIRED}" != "false" ]` fails **closed**: any value that is not exactly `false` — a typo, `False`, `0`, an empty string — is treated as required. That is the right polarity for a flag whose wrong answer is a silent deploy. - The template's `grep "^$k=" .env 2>/dev/null | cut … || true` genuinely tolerates an absent or empty file under `set -euo pipefail`; the `|| true` covers the whole pipeline inside the substitution, so `pipefail` cannot resurrect it. - The warning text carries only fixed strings and `OB_PATH`, never a value, and the "never echoed" case asserts that on both streams. - `"$OB_OUT.partial"` instead of `mktemp` is the right call for the reason you give, and the harness catching it is the harness doing its job. **On not propagating here:** agreed, and saying plainly that this alone does not close criterion 5 for existing consumers is worth more than a merge that implies otherwise.

openbao-fetch/action.yml

					
				@ -28,0 +34,4 @@

				      notification. Leave it at the default for anything that deploys or builds

				      with the values.

				    required: false

				    default: "true"

The survey behind this default is the load-bearing part of the change, so I walked it mechanically rather than checking the names given.

Eleven repositories use openbao-fetch, across thirty workflow files — the description's list is nine. The two missing are ahmad/namey and suhba/suhba-docs, and both have a deploy.yml that calls the action with no -z guard anywhere in the file. So the unguarded population is nine, not seven, and the case for default: "true" is stronger than the PR argues rather than weaker.

The action is also called outside deploy and release: monitoring.yml (portfolio, trip, imamah, khitta, kidpoints), pr.yml (namey, portfolio), docs-build.yml (suhba-docs) and cms-pr.yml (suhba-web). Several of those are the same shape as the notify case — a workflow whose whole purpose is reporting or checking, failed by a vault it only needed for one step — so they belong on the follow-up beside the eight notify jobs.

Nothing to change here. Recording it because the follow-up will otherwise be scoped to a list of eight, and because a claim of the form "every consumer does X" is only as wide as the enumeration behind it.

The survey behind this default is the load-bearing part of the change, so I walked it mechanically rather than checking the names given. **Eleven repositories use `openbao-fetch`, across thirty workflow files** — the description's list is nine. The two missing are `ahmad/namey` and `suhba/suhba-docs`, and both have a `deploy.yml` that calls the action with no `-z` guard anywhere in the file. So the unguarded population is **nine**, not seven, and the case for `default: "true"` is stronger than the PR argues rather than weaker. The action is also called outside `deploy` and `release`: `monitoring.yml` (portfolio, trip, imamah, khitta, kidpoints), `pr.yml` (namey, portfolio), `docs-build.yml` (suhba-docs) and `cms-pr.yml` (suhba-web). Several of those are the same shape as the notify case — a workflow whose whole purpose is reporting or checking, failed by a vault it only needed for one step — so they belong on the follow-up beside the eight `notify` jobs. Nothing to change here. Recording it because the follow-up will otherwise be scoped to a list of eight, and because a claim of the form "every consumer does X" is only as wide as the enumeration behind it.

ahmad merged commit 512a2349fa into main

2026-09-09 05:43:45 +00:00

ahmad deleted branch feature/65-notify-non-blocking

2026-09-09 05:43:45 +00:00

ahmad referenced this pull request from a commit

2026-09-09 05:43:47 +00:00

fix: Let a notify job survive an unreachable vault (#67)

ahmad commented

2026-09-09 05:45:28 +00:00

Merged as 512a234. Both of your additions are right, and the second one changed a number I had put in a table as though it were settled.

On the survey being short by two — it was short by three. You walked the fleet properly and found ahmad/namey and suhba/suhba-docs missing from my list. I had surveyed my local checkouts, which can only see repositories I happen to have cloned and current, and that is not an enumeration of anything. Re-running it against the API — every repository the server lists, every file under .forgejo/workflows/, each openbao-fetch@ attributed to its enclosing job — gives 12 repositories across 35 workflow files. Bayan/bayan-web is in neither of our lists.

So the unguarded population is larger again, and the conclusion is unchanged in direction and stronger in degree. I have replaced the table on #68 with the mechanically-derived one and written down the method, because "every consumer does X" is only as wide as the enumeration behind it and mine was not wide.

Your point about monitoring.yml and pr.yml is the more useful half. Classifying by whether the job's only use of the secrets is a notification: 26 jobs across 10 repositories, and they are not all in release.yml — pr.yml in portfolio, namey and bayan-web, monitoring.yml in five repositories. A pull-request check going red because the vault blinked is the same failure as a release doing it and lands somewhere more annoying. All 26 are now listed on the follow-up; the 57 jobs that genuinely need the secrets are listed too, so the next pass has both halves and does not have to re-derive which is which.

On the thin guard. You are right that required by default: an unreachable vault stops the job is carrying the whole weight of the default, and that its failure direction is the silent one. I have left it as the single case rather than padding it — the predicate is one comparison and the case does exercise it — but it is the assertion I would want re-run first if that comparison is ever touched, and it is worth having said so here rather than assuming the next reader infers it.

Merged as `512a234`. Both of your additions are right, and the second one changed a number I had put in a table as though it were settled. **On the survey being short by two — it was short by three.** You walked the fleet properly and found `ahmad/namey` and `suhba/suhba-docs` missing from my list. I had surveyed my **local checkouts**, which can only see repositories I happen to have cloned and current, and that is not an enumeration of anything. Re-running it against the API — every repository the server lists, every file under `.forgejo/workflows/`, each `openbao-fetch@` attributed to its enclosing job — gives **12 repositories across 35 workflow files**. `Bayan/bayan-web` is in neither of our lists. So the unguarded population is larger again, and the conclusion is unchanged in direction and stronger in degree. I have replaced the table on **#68** with the mechanically-derived one and written down the method, because "every consumer does X" is only as wide as the enumeration behind it and mine was not wide. **Your point about `monitoring.yml` and `pr.yml` is the more useful half.** Classifying by whether the job's *only* use of the secrets is a notification: **26 jobs across 10 repositories**, and they are not all in `release.yml` — `pr.yml` in `portfolio`, `namey` and `bayan-web`, `monitoring.yml` in five repositories. A pull-request check going red because the vault blinked is the same failure as a release doing it and lands somewhere more annoying. All 26 are now listed on the follow-up; the 57 jobs that genuinely need the secrets are listed too, so the next pass has both halves and does not have to re-derive which is which. **On the thin guard.** You are right that `required by default: an unreachable vault stops the job` is carrying the whole weight of the default, and that its failure direction is the silent one. I have left it as the single case rather than padding it — the predicate is one comparison and the case does exercise it — but it is the assertion I would want re-run first if that comparison is ever touched, and it is worth having said so here rather than assuming the next reader infers it.

Sign in to join this conversation.

No reviewers