ci(release): make the registry promotion a re-runnable job - #3456

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hack/promote-publish-chart.sh`:
- Around line 93-126: Update the skopeo inspect probe before the existing
published-chart comparison so its output and exit status are captured. Proceed
to the “not published; pushing” branch only when the registry response clearly
indicates the stable chart manifest is missing; for authentication, network,
timeout, or other probe failures, print the captured diagnostic and abort
instead of running helm push. Preserve the current diff and FORCE_CHART behavior
for charts confirmed to exist.

NOT LGTM. The branch conflicts with main in both files it shares with it, so the diff that would actually merge does not exist yet. Two smaller things below are worth folding into the same rebase pass.

Business context: the registry side effects of a promotion run after the stable tag and the GitHub release are already irreversible. v1.6.0 died partway through them, and re-running was no help, because the job re-entered tag creation and re-executed the same broken script.

The design itself is right, and I want to say that before the findings. Splitting the registry work into its own job, moving the logic into scripts, and taking those scripts from main at run time is the combination that makes a failed promotion both replayable and fixable. I checked the recovery path rather than assuming it: GitHub reuses the outputs of jobs that already succeeded when you re-run only the failed ones, so a replayed registry job still reads the tag and the latest decision finalize computed on the first attempt instead of falling through to an empty tag.

Blockers

B1: conflicts with main in .github/workflows/pull-requests-release.yaml and docs/release.md

The structural change survives, so this is rebase work. Splitting Promote Registry Artifacts out of Finalize Release merges cleanly, nothing on main re-implemented it, and the premise still holds, since main still runs the retag and the chart publish as inline steps of finalize.

Three hunks, two kinds. The workflow hunk is comment-only and convergent. Main landed the same persist-credentials: false fix independently, so the key merges and only the two comment blocks collide. That also makes the last commit on this branch redundant with what main already carries, so drop it on rebase.

The two docs/release.md hunks are a real semantic collision. Main rewrote the workflow inventory from five entries to seven, and rewrote step 4 for the changelog-as-release-body change. This branch edits item 4 of the five-item list, which no longer exists in that shape, and inserts the recovery section next to a step 5 whose surrounding text has moved.

So resolving those two hunks mechanically gives you docs that contradict themselves. Main's new item 5 still says finalize is what "retags the rc images to stable by digest and publishes the stable chart". After this change that is a separate job, so the rebase has to update main's item 5 as well, not only re-apply this branch's edit to the old item 4.

B2: the new invocation split for promote-retag.sh is asserted in a comment and pinned by no test

The diff changes how that script runs in production: from ./hack/promote-retag.sh with the working directory being the script's own tree, to ./.promote-scripts/hack/promote-retag.sh with the working directory being a different tree. Correctness now rests on two path resolutions staying independent, . "$(dirname "$0")/lib/image-refs.sh" landing in .promote-scripts/hack/lib/, and collect_image_refs packages globbing relative to the working directory. The workflow states that requirement in prose, "keep that property if either moves", which is a comment sitting where a test belongs.

hack/promote-retag_test.bats already exists and invokes hack/promote-retag.sh relatively from the repo root in all five cases, so script directory and scanned root are the same place and the split is never exercised. Someone tidying that source line to . hack/lib/image-refs.sh keeps the suite green and breaks the next release, after the tag and the release are irreversible.

The remedy is small and this PR already contains the pattern: promote-publish-chart_test.bats invokes "$ROOT/hack/promote-publish-chart.sh" after cd "$FIX", which is exactly the split. One case in promote-retag_test.bats doing the same, copying a couple of packages/*/*/values.yaml into a temp dir and asserting the dry run still selects owned refs, covers it.

B3: the manual-recovery fallback in docs/release.md names only half the promotion

The new recovery section correctly frames step 5 as both the retag and the chart publish, and then the fallback for a failure outside the 30-day re-run window says to run cd <tag-tree> && /path/to/main/hack/promote-retag.sh vX.Y.Z by hand.

A maintainer following that literally retags the images and never publishes the chart, so the documented helm upgrade --install --version X.Y.Z path still returns manifest unknown. That is precisely the half-finished state this PR exists to make recoverable, reached by following the PR's own recovery instructions. The snippet also omits the registry login both scripts require, and REGISTRY when promoting a fork.

Non-blocking follow-ups

1. hack/promote-publish-chart.sh stamps packages/core/installer/values.yaml in place and never restores it. Harmless on an ephemeral runner, but the manual-recovery path the new docs endorse runs it in an operator's own checkout, and there it silently leaves a modified tracked file, which is the leak the comment above that line warns about. The EXIT trap a few lines up already exists, so restoring that one file costs two lines.

2. The registry job runs hack/ scripts fetched from main at run time while holding packages: write against the release registry. The workflow comment and the PR body both name the tradeoff, but only as a correctness cost, "a broken promotion script on main breaks the next release". It is also a trust widening, since the code that can write release tags is no longer confined to the reviewed release tree. Worth stating in that comment, because it is the block the next person weighing this design will read.

3. The published-chart probe greps an empirical set of registry wordings. Failing closed is the right default, but GHCR can answer for a genuinely nonexistent repository with denied or unauthorized rather than a name-unknown code, so the first-ever push of a brand new chart repo may abort and need a human. A line in the script header would stop the next reader filing that as a bug.

4. Pre-existing and outside this diff, so not yours to fix here, but it wants a home. The troubleshooting table in docs/release.md blames Draft release for vX.Y.Z not found on Publish draft release having lost its github-token, and points at pull-requests-release.yaml:163. At the merge base that step is at line 218 and does carry github-token: ${{ steps.app-token.outputs.token }}, so both halves are already wrong, and the release checklist a few lines above repeats the claim. It sends a release-day maintainer chasing a regression that does not exist. Separately, the tag-immutability table's row for this workflow lists the image retag but never the chart publish.

What else I checked

promote-retag.sh inspects each destination tag, skips one already at the expected digest, refuses to move one pointing elsewhere, and re-verifies after copying, so a half-finished promotion genuinely replays. The chart script's content comparison is the other half of that and is correct, including DIFF_STATUS separating diff's "files differ" from a real comparison error, which I ran under sh, dash and bash rather than assuming.

make_latest is set unconditionally as a string before any branch, so needs.finalize.outputs.make_latest == 'true' reads the same decision the release got. Both jobs pin to github.sha, so the chart is still packaged from the tagged tree. The registry job cannot outrun its needs gate, since its if: carries no status function.

The bats suite pierces every guard instead of asserting one exists. The prerelease and malformed-tag tests check that no packaging and no registry traffic happened, the drift test forges a published chart whose content differs, and the probe test injects three real skopeo failure strings and checks nothing was pushed. Seven declared, seven run, seven green, no trap in the file to swallow a failure. The shared image-ref enumeration is not in play, since the chart script publishes one named chart and enumerates nothing.

E2E is the one red check. This diff touches a workflow that only fires when a release PR merges, two files only that workflow invokes, and a maintainer doc, so it cannot reach the E2E path. I am treating it as unrelated.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/release.md`:
- Line 276: Update the manual recovery guidance in the release procedure to run
both promote-retag.sh and promote-publish-chart.sh, using the same
MOVE_LATEST/latest decision for each command. Keep the existing release-tag
checkout context and clarify that both artifacts are restored when reruns are
unavailable.

---

Outside diff comments:
In `@docs/release.md`:
- Line 231: Update the inline Markdown example in the changelog validation
description to remove the trailing whitespace from the code span, using `##` and
prose describing it as a level-2 section; preserve the surrounding explanation
and validation behavior.

NOT LGTM. The conflict is resolved and that finding is closed. The two other blockers from my previous review are both still live, and the rebase did not touch either.

On what the rebase changed, since it bears on how much I re-read: the merge against main is clean now, the credentials commit dropped out as redundant with what main already carried, and the probe-fix commit is byte-identical to before. The only content change is the docs conflict resolution, and it is the right one. The workflow inventory entry now sits on main's seven-item list and keeps the changelog-as-release-body wording while adding the separate-job clause, instead of re-applying the old edit to a list that no longer exists in that shape. Both new files are unchanged, so I did not repeat the full read of them.

Blockers

B2 (still open): the invocation split for promote-retag.sh is asserted in a comment and pinned by no test

The production call is ./.promote-scripts/hack/promote-retag.sh with the working directory pointing at the release tree. That works only because the script sources its helper as . "$(dirname "$0")/lib/image-refs.sh" while scanning via collect_image_refs packages relative to the working directory. Two path resolutions that have to stay independent of each other, stated in a workflow comment and held by nothing executable.

hack/promote-retag_test.bats is not touched by this PR, and none of its eleven cases runs the script from a working directory other than the repo root, so script directory and scanned root remain the same place in every one of them. Main has grown that suite by roughly two hundred lines since this branch forked, adding digest-verification and transient-failure cases, so the file is actively maintained and still does not cover the one property this change newly depends on. Someone tidying that source line to . hack/lib/image-refs.sh keeps all eleven green and breaks the next release, after the tag and the release are already irreversible.

The remedy has a template in this PR: promote-publish-chart_test.bats invokes "$ROOT/hack/promote-publish-chart.sh" after cd "$FIX", which is the split. One case in promote-retag_test.bats copying a couple of packages/*/*/values.yaml into a temp dir, changing into it, invoking by absolute path and asserting the dry run still selects owned refs would pin it.

B3 (still open): the manual-recovery fallback names only half the promotion

The paragraph is unchanged on this head: cd <tag-tree> && /path/to/main/hack/promote-retag.sh vX.Y.Z. That is the retag alone. A maintainer following it outside the 30-day re-run window retags the images and leaves the chart unpublished, which is the half-finished state this PR exists to make recoverable, reached by following this PR's own recovery instructions. The paragraph directly above correctly describes both scripts, which is what makes the omission easy to read past. Adding the promote-publish-chart.sh invocation and the registry login both scripts require would close it.

Non-blocking

The four follow-ups from my earlier review are unchanged and still non-blocking: restoring packages/core/installer/values.yaml from the existing EXIT trap, naming the trust widening as well as the correctness cost in the comment about running scripts from main, a line in the script header about registry wordings for a nonexistent repository, and the two stale claims in the troubleshooting and tag-immutability tables, which this PR does not touch and which are now tracked separately.

Verification on this head

The chart suite is seven declared, seven run, seven green, with no trap in the file. The merge against main is clean.