Skip to content

fix(ci): look the sibling up by workflow FILE name, not display name - #153

Merged
shockalotti merged 1 commit into
mainfrom
fix/release-verify-workflow-lookup
Oct 16, 2026
Merged

shockalotti merged 1 commit into
mainfrom
fix/release-verify-workflow-lookup

Conversation

@shockalotti

Copy link
Copy Markdown
Contributor

You were right on both counts. #152 is closed.

You were right: the manifest endpoint tolerates case

Here is the 4-way table I should have produced before writing that PR body:

repo arg passed built URL result
calnode/ns8-calnode /v2/calnode/ns8-calnode/... 200
Calnode/ns8-calnode /v2/Calnode/ns8-calnode/... 200
ghcr.io/calnode/ns8-calnode /v2/ghcr.io/calnode/... 200
ghcr.io/Calnode/ns8-calnode /v2/ghcr.io/Calnode/... 404

Row 2 disproves my claim directly. I asserted a mechanism I had not isolated, and I should have run this table before writing "the manifest endpoint is stricter". d5cc8ee was a complete fix for case, and I misread it as half a fix.

One loose end I am not going to quietly drop: row 4 is the exact string ns8-module.yml passes, and it 404s reproducibly. So something there is still real, it is not a case problem, and I have not isolated the cause. It is out of scope for this PR and recorded in the commit message as an open question rather than left as a claim.

You were right about the real bug, and it is the bigger one

The gate step sent the display name where the REST API wants the file name:

.../actions/workflows/NS8 module publish/runs  -> 404
.../actions/workflows/Docker publish/runs      -> 404
.../actions/workflows/ns8-module.yml/runs      -> 200, 109
.../actions/workflows/docker-publish.yml/runs  -> 200, 340

Under set -euo pipefail that killed the step, so every run failed before reaching a single line of the decision tree. #151 could never have succeeded. Main is red from this.

The fix

Display name to file name via a case, confined to the API call. Mapping rather than numeric id lookup, as you suggested, because it is simpler and keeps the trigger and the lookup visibly consistent.

The trigger is untouched. workflows: ['NS8 module publish', 'Docker publish'] is correct, since workflow_run matches on display name and github.event.workflow_run.name returns one. Only the API path needs the file name.

An unmapped name exits 1 with an annotation naming the file to edit. This is the part I would argue hardest for: a silent mapping would have made a rename look like a healthy waiting-for-sibling. Loud is the only version of this that cannot hide again.

Also collapsed two identical gh api calls into one fetch, reading status and conclusion from the same response.

The proof you asked for: real API, not a stub

The shipped block, real workflow names, real commit SHAs, no stubs:

5b4acef Docker publish      -> 340 runs, completed/success -> ready=true  outcome=verify
5b4acef NS8 module publish  -> 109 runs, completed/failure -> ready=false outcome=blocked
no run for that SHA         -> absent/absent              -> ready=false outcome=waiting-for-sibling

And the decision matrix with only gh stubbed at the API seam, so the mapping, the decision tree and the loud failure all execute for real:

case rc outcome
both succeeded 0 ready=true outcome=verify
sibling still running 0 waiting-for-sibling
sibling still queued 0 waiting-for-sibling
sibling concluded failure 0 blocked
no sibling run at all 0 waiting-for-sibling
API 404 1 loud
unmapped display name 1 loud

Why it slipped through, and it is your finding not mine

Your account is correct and I want it in the record rather than softened: my test executed the real block but stubbed status and conclusion, so the gh api call was never issued. Both halves were green and the workflow was dead. The decision tree was right; the I/O in its first step was unproven. That is #148's failure mode exactly, and you are right that you merged on the strength of a logic test.

The block no longer accepts status and conclusion as inputs at all. It derives them from a real response, so the only seam left to stub is gh itself, and the API I/O becomes a separate real assertion rather than an assumed one.

The structural answer, which is the part that matters

A new Probe both publish workflows step runs unconditionally, before the gate, and calls the real API for both file names, echoing the run count and the run for this commit. It fails the job if either lookup 404s. So the next version of this bug cannot hide in a stubbed test: it shows up as a red probe step in a live log.

Your point 3 is the right instinct and I have tried to make it structural rather than a one-time verification.

Verification of the rest

Unchanged from #151, re-checked: module-only exit 0, --pair --expect-app-tag 0.11.1 exit 0, dead --app-repo still rejected with exit 2, missing app image still fails bounded in 2.5s, mismatched pair still exit 1. assert-multiarc.py is not touched in this PR, so none of that is a claim about new code.

Also confirmed: :latest on the module passes module-only mode, so v0.11.1's artifacts are good, and I agree that staying red as history beats cutting an empty release.

No tag pushed, nothing merged from me. Worktree is /tmp/ns8-case-fix; /tmp/nm-registry is gone, and ~/Projects/calnode is untouched on main at 5b4acef.

release-verify.yml has never once succeeded, and could never have. The gate step
called:

    gh api "repos/${{ github.repository }}/actions/workflows/$SIBLING/runs"

where SIBLING is the display name, either 'NS8 module publish' or 'Docker
publish'. The REST path wants the workflow FILE name or the numeric id, and
answers 404 for a display name. Measured against the real repo:

    .../actions/workflows/NS8 module publish/runs  -> 404
    .../actions/workflows/Docker publish/runs      -> 404
    .../actions/workflows/ns8-module.yml/runs      -> 200, total_count 109
    .../actions/workflows/docker-publish.yml/runs  -> 200, total_count 340

Under `set -euo pipefail` that 404 killed the step, so every run failed before
reaching a single line of the decision tree. Main has been red here since #151
merged.

The trigger is deliberately untouched. `workflows: ['NS8 module publish',
'Docker publish']` is correct, because workflow_run matches on display NAME, and
github.event.workflow_run.name also returns the display name. Only the API path
needs the file name, so the mapping is confined to the one call that does. A
case statement maps display name to file name, which is simpler than resolving a
numeric id and keeps the trigger and the lookup visibly consistent.

An unmapped display name now exits 1 with an error annotation naming the file to
edit. That is the point: renaming a publish workflow must be loud, because a
silent no-op is precisely how this bug survived review. A silent mapping would
have let a rename look like a healthy waiting-for-sibling.

Two lookups became one, so the sibling's run list is fetched once and both
status and conclusion are read from it.

WHY THIS WAS MISSED, and it is the same failure as #148: the gate's decision
tree was tested and its integration with the thing it calls was not. The test
executed the real shipped block but stubbed status and conclusion, so the gh api
call was never issued. Both halves were green and the workflow was dead. The
block no longer accepts status and conclusion as inputs at all, it derives them
from a real response, so the seam that can be stubbed is now gh itself and the
API I/O is a separate, real assertion.

Proven against the REAL API, with the real shipped block, real workflow names
and real commit SHAs:

    5b4acef Docker publish      -> 340 runs, completed/success -> verify
    5b4acef NS8 module publish  -> 109 runs, completed/failure -> blocked
    no run for that SHA         -> absent/absent              -> waiting-for-sibling

And the decision matrix, with only gh stubbed at the API seam, so the mapping,
the decision tree and the loud failure all run for real:

    both succeeded            rc=0  ready=true  outcome=verify
    sibling still running     rc=0  ready=false outcome=waiting-for-sibling
    sibling still queued      rc=0  ready=false outcome=waiting-for-sibling
    sibling concluded failure rc=0  ready=false outcome=blocked
    no sibling run at all     rc=0  ready=false outcome=waiting-for-sibling
    api 404                   rc=1
    unmapped display name     rc=1

A new "Probe both publish workflows" step runs unconditionally, before the gate,
and calls the real API for both file names, echoing the run count and the run
for this commit. It fails the job if either lookup 404s. So a broken lookup is
visible in a live run log rather than inferred from a unit test, which is the
gap that let this ship.

Unrelated to this commit, and recorded because it is a real open question: the
module repository is passed to assert-multiarc.py as
`ghcr.io/${{ github.repository_owner }}/ns8-calnode`, and that exact string
404s, while every other spelling of the same image resolves. Left alone here on
purpose, since it is a separate change with a separate cause still to isolate.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ The core fix is correct and I verified the mechanics; one gap in the "loud, not silent" defence is worth knowing about.

Reviewed changes

  • Gate step now maps display name → workflow file name via a case before the API call, and exits 1 on an unmapped name instead of proceeding (release-verify.yml:106).
  • Sibling lookup collapsed to a single gh api call; status and conclusion are now derived from the same JSON response via one jq expression instead of two separate 404-prone calls (release-verify.yml:120).
  • New unconditional Probe both publish workflows step runs before the gate, hits the real API for both file names, and fails the job on a 404 (release-verify.yml:50).
  • The workflow_run trigger is deliberately untouched — workflows: ['NS8 module publish', 'Docker publish'] still matches on display name, which is correct.

What I checked and agree with: the REST endpoint takes a file name or numeric id, not a display name, so the 404 under set -e genuinely killed every run — the diagnosis and the minimal fix are right. I confirmed both display names still resolve (ns8-module.yml:1, docker-publish.yml:1), that workflows: [...] matches by display name, that this is the only place in .github/workflows/ using the actions/workflows/<x>/runs path (so no copy of the bug elsewhere), and that actions: read in the workflow permissions covers both new calls. I also traced the shell semantics: the read -r status conclusion < <(...) idiom yields absent absent cleanly on a miss, and the probe's ${match:-none} fallback is cosmetic — jq already prints none none ?, so that branch never fires.

ℹ️ The *) arm cannot catch the rename it is guarding against

Technical details
# The loud-failure arm does not cover the most likely rename

## Affected sites
- `.github/workflows/release-verify.yml:109` (`*)` arm) — reached only when `SIBLING` is neither trigger name
- `.github/workflows/release-verify.yml:26` (`workflows:` filter, unchanged) — the actual drift surface
- `.github/workflows/release-verify.yml:83` (`SIBLING` derivation, unchanged) — hardcodes both display names a second time

## Why
`SIBLING` is derived from `github.event.workflow_run.name`, and `workflow_run` only ever
delivers the names listed in `workflows:`. So the `*)` arm fires only on a hand-edited
half-update — never on the rename the comment above it describes. Rename
`Docker publish` → `Docker publish (multi-arch)` and the `workflows:` filter stops matching
anything: GitHub raises no annotation, and **this workflow never runs at all**, so neither
the probe step nor the `case` gets a chance to shout. A gate that has gone dark is
indistinguishable from a gate that simply has not fired yet.

The probe step does cover the other direction — a workflow *file* renamed while its display
name stays put still 404s and turns the job red. So the coverage is: file rename = loud,
display rename = silent.

## Open questions for the human
Not asking for a change in this PR — the fix does not live in the diff. Options if you want
the display-name case covered too:
  - reduce the coupling rather than detect it: a single source of truth (the `workflows:`
    list, or a small checked-in matrix) that both the `SIBLING` expression and the `case`
    read, so the names cannot drift between the three sites;
  - a liveness check from a workflow that is *not* gated on the pair (e.g. `test-module.yml`),
    which is the only place a dark gate is observable.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using space-bunny-free (free) | 𝕏

# Loud, not silent. A renamed workflow must not look like a healthy
# no-op, which is exactly how the previous version of this bug hid.
echo "::error::no file name is mapped for sibling workflow '$SIBLING'."
echo "::error::Add it to the case statement above. The display names live in .github/workflows/ns8-module.yml and docker-publish.yml."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This arm is sound, but it cannot fire for the rename it names in the comment above. SIBLING comes from github.event.workflow_run.name, which only ever holds a name from the workflows: filter — so if Docker publish is renamed, that filter stops matching, this workflow never runs, and neither this arm nor the probe step gets a chance to fail loudly. Renaming the workflow file is covered (the probe 404s); renaming its display name is still silent.

@shockalotti
shockalotti merged commit 5957084 into main Oct 16, 2026
7 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant