feat(ci): publish GitHub Release on every merge to main#16
feat(ci): publish GitHub Release on every merge to main#16akshaymagapu wants to merge 11 commits into
Conversation
Removes the manual "draft a Release" step. On push to main, computes the next semver tag from the highest existing vN.N.N (major/minor/patch bump driven by conventional-commit subject), creates a GitHub Release on that commit, then release.yaml fires automatically and moves the floating vN tag — consumers using @vn pick up the change on their next CI run. Bump rules: - feat!: / fix!: / *!: / BREAKING CHANGE in body -> major - feat: / feat(scope): -> minor - anything else -> patch Skip a release for a specific merge by adding [skip release] to the commit message. Doc-only / .md / LICENSE changes are also skipped via paths-ignore.
Matches the release pattern used by every other spacecat repo
(spacecat-api-service, spacecat-audit-worker, spacecat-shared,
spacecat-import-worker, spacecat-jobs-dispatcher): conventional-commit-
driven semver via semantic-release, run on every push to main.
End-to-end after this lands:
PR merge to main
-> semantic-release.yaml fires
-> semantic-release computes next version from conventional commits
-> publishes GitHub Release + updates CHANGELOG.md
-> release.yaml fires on release:published, moves vN tag
-> consumers pinning @vn pick up on next CI run
No more manual 'draft a Release' step.
Skip a release by adding [skip ci] to the merge commit message.
mysticat-ci is yaml-only, no Node code anywhere — so wiring up
semantic-release (and its 300+ npm dev-deps) would be machinery for
machinery's sake. Instead: a small workflow that on push to main
computes the next vN.N.N from existing tags + conventional-commit
subject, then publishes a GitHub Release via gh CLI. The existing
release.yaml workflow handles the floating vN tag move as before.
End-to-end:
PR merge to main
-> auto-release.yaml fires
-> computes next tag from latest vN.N.N + commit prefix
-> gh release create publishes the Release
-> release.yaml fires on release:published, moves vN
-> consumers' next CI run picks up @vn
Skip a release with [skip release] in the commit message; docs/.md/
LICENSE changes are also skipped via paths-ignore.
solaris007
left a comment
There was a problem hiding this comment.
Hey @akshaymagapu,
Thanks for tackling this - the merge-to-consumer gap is real. I confirmed the live state: floating v2 is behind main (the most recent merged fix has not propagated to @v2), and v2.1.4/v2.1.5 exist as tags with no matching Release, so the manual process has already drifted. Worth solving. That said, the mechanism needs rework before it lands, and there is a security issue to fix first.
Strengths
persist-credentials: falseon checkout (line 36) and least-privilegepermissions: contents: write(lines 22-23) match the repo-wide convention.set -euo pipefail(line 43) plus the explicit empty-tag guard (lines 45-48) fail loudly instead of minting a garbage version.- The tag glob with
--sort=-v:refname(line 44) correctly excludes the floatingvNtags from the "latest" selection - a subtle trap avoided. concurrencywithcancel-in-progress: false(lines 25-27) is the right call for a publish job.- The
Create releasestep passes the token viaenv: GH_TOKEN(lines 69-70) - the safe pattern, and exactly what the compute step should do too (see Critical 1). - Staying with pure
gh/bash instead of semantic-release is defensible for a yaml-only repo.
Issues
Critical (Must Fix)
-
Script injection via
github.event.head_commit.message(line 53).MSG="${{ github.event.head_commit.message }}"interpolates the commit message into the shell script before bash parses it. A message containing"; ... ; echo "or a$(...)/backtick payload executes arbitrary commands on the runner, which holdscontents: writeandGH_TOKEN. This is the same class this repo already fixed once ("fix(ci): prevent script injection via branch name") - the safe pattern is live inservice-ci.yaml(branch-deploy passesREF_NAMEviaenv:). Because this workflow mints releases andrelease.yamlthen moves the floatingvNthat every consumer pins, code execution here is a supply-chain foothold across the whole consumer fleet. Fix: route the message throughenv:and reference it as a quoted variable (see inline comment). -
The bump logic never produces minor or major under this repo's merge strategy (lines 53-62). PRs land here as merge commits (recent
maintips read "Merge pull request ... from adobe/fix/..."), sohead_commit.messageis the merge-commit subject, which matches neither thefeat:nor the!:/BREAKING CHANGEpatterns. Every merge falls through to patch, so the documentedfeat -> minor/! -> majorbehavior (lines 7-10) is unreachable in practice. It is also merge-strategy-dependent: a squash-merged PR (the repo allows squash) would classify correctly, so identical changes get different bumps based on which merge button is used. Fix: derive the bump from a reliable source - read the merged PR title/labels viagh pr view, or scan the push range (git log <before>..<after>) for the strongest conventional type - and decide, document, and enforce the merge strategy this depends on.
Important (Should Fix)
-
Asymmetric grep: major matches the full message, minor matches only the subject (lines 56, 58). Line 56 greps
$MSG(the whole multi-line message); line 58 greps$SUBJECT(first line only). Becausegrep -E '^...'anchors per line, the literal textBREAKING CHANGE, or asomething!:token, anywhere in a commit body triggers a major bump - including prose like "this is not a BREAKING CHANGE". A false major mintsvN+1.0.0and (via release.yaml's force-move) stands up a brand-newvN+1floating tag that no current consumer references, whilevNstops advancing. Conversely, afeat:only in the body is missed. Fix: read both checks from the same source; match!in the subject type/scope, and if you support the breaking footer, anchor it (^BREAKING CHANGE:) rather than substring-matching the whole blob. -
"Release on every merge" is likely the wrong granularity - settle this before merge (trigger design, lines 14-16, plus the release.yaml chain). What consumers consume is the floating
vNtag (README pins@v1); the real gap is that nothing movesvNautomatically. Minting an immutablevN.N.NRelease on every merge is heavier than that gap needs - consumers never referencevN.N.N, so the Release mostly exists to triggerrelease.yamlto movevN, a two-workflow baton-pass you could collapse into one step. Separately, auto-movingvNon every merge propagates to all consumers on their next CI run with no soak, canary, or human gate; a badservice-ci.yamlchange breaks every consumer at once and rollback is a manual force-move ofvN. Options, roughly in order: (a) movevNdeliberately but add automation/notification so drift is visible; (b) movevNon every merge directly, drop the per-merge Release, and collapse to one workflow; (c) keep minting Releases but gate thevNmove behind a GitHub Environment with required reviewers (the mechanism service-ci already uses for deploys). Pick consciously - the current design couples release cadence to merge cadence with no gate. -
gh release createis not idempotent, and a failed run is silent (lines 68-74).gh release create vXerrors if the tag/release already exists, so a re-run after a partial failure - or two merges that compute the sameNEXTbefore the first tag is visible - fails the job hard. And a failed auto-release reproduces exactly the gap this PR exists to close (merge landed, consumers get nothing) with no notification; the only signal is a red run on an infra repo service teams do not watch. Fix: re-fetch tags before computing (git fetch --force --tags origin), guard the create (gh release view "$NEXT" >/dev/null 2>&1and skip, or treat "already exists" as success), and add anif: failure()notification so a stuck release is noticed.
Minor (Nice to Have)
[skip release]only inspects the head commit (line 31). With merge commits, a marker placed on a branch commit is ignored. Document where it must live, or check the push range.- The header comment (lines 7-10) documents bump tiers the code cannot deliver today (see Critical 2). Update it once the bump source is fixed.
- The PATCH parse (line 51) and glob (line 44) silently strip suffixes, so a future
v2.1.5-rc1would collapse into the release lineage. Latent today; anchor to^v[0-9]+\.[0-9]+\.[0-9]+$if a prerelease scheme ever appears.
Recommendations
- Extract the bump computation into a script (e.g.
.github/scripts/compute-next-tag.shtaking the latest tag plus message and printing tag/bump) with a table-driven smoke test. The logic decides published version numbers for downstream consumers but can currently only be exercised by pushing tomain; a smoke test would have caught Critical 2 and Important 3 before merge. - Add a short "Releasing" section to the README: bump rules, the
[skip release]hatch, and the rollback command for the floating tag. actions/checkout@v6(line 35) is pinned to a major tag, which matches repo-wide convention, so no change is needed here. If you want to raise the supply-chain bar, do it repo-wide (SHA-pin via Dependabot/Renovate), not just on this file.
Out of scope, worth tracking
- Reconcile the existing drift (
v2.0.0,v2.1.4,v2.1.5are tags without Releases;v2lagsmain) before turning automation on, so the first auto-run starts clean. This workflow will not backfill those.
Assessment
Ready to merge? No, with fixes. The problem is real and the skeleton (least-privilege, concurrency, fail-on-missing-seed-tag) is sound, but the commit-message interpolation is an exploitable injection on a supply-chain-critical workflow, and the bump detection does not work under the repo's merge-commit reality (every release becomes a patch, with false majors possible from body text). Fix the injection, settle the release-trigger model, make the bump source reliable, and make the create idempotent and observable before this lands.
Next Steps
- Fix the script injection (Critical 1) by routing the commit message through
env:. - Decide the release-trigger model (Important 4) - the "do we release on every merge" call - and fix the bump source (Critical 2) accordingly.
- Address the false-major grep (Important 3); make
gh release createidempotent and add a failure notification (Important 5). - Minor items and the README/test recommendations are optional but cheap.
| MAJOR=$(echo "$LATEST" | sed -E 's/^v([0-9]+)\..*/\1/') | ||
| MINOR=$(echo "$LATEST" | sed -E 's/^v[0-9]+\.([0-9]+)\..*/\1/') | ||
| PATCH=$(echo "$LATEST" | sed -E 's/^v[0-9]+\.[0-9]+\.([0-9]+).*/\1/') | ||
|
|
There was a problem hiding this comment.
Critical: script injection. MSG="${{ github.event.head_commit.message }}" interpolates an attacker-influenceable commit message into the shell before bash parses it; a $(...), backtick, or "; ... ; echo " payload runs arbitrary commands on the runner, which holds contents: write and GH_TOKEN. This is the same class this repo already fixed for branch names ("fix(ci): prevent script injection via branch name"), and the safe pattern is live in service-ci.yaml (branch-deploy passes REF_NAME via env:). Because this workflow mints releases and release.yaml then moves the floating vN that every consumer pins, code execution here is a fleet-wide supply-chain foothold.
Fix - pass the message through env: and reference it as a quoted variable:
- name: Compute next tag
id: t
env:
HEAD_COMMIT_MSG: ${{ github.event.head_commit.message }}
run: |
set -euo pipefail
...
MSG="$HEAD_COMMIT_MSG"There was a problem hiding this comment.
Fixed in 81acd7a. HEAD_COMMIT_MSG now flows through the step's env: block, exactly the pattern you cited from the branch-name fix.
The env declaration:
env:
HEAD_COMMIT_MSG: ${{ github.event.head_commit.message }}
GH_TOKEN: ${{ github.token }}
REPO: ${{ github.repository }}The script reads it as a plain shell variable — no inline expansion:
set -euo pipefail
SUBJECT=$(printf '%s\n' "$HEAD_COMMIT_MSG" | head -1)No ${{ }} expansion anywhere inside any run: body. Same hardening applied to the publish step — NEXT_TAG, REPO, TARGET_SHA all routed through env:.
|
|
||
| MSG="${{ github.event.head_commit.message }}" | ||
| SUBJECT=$(printf '%s\n' "$MSG" | head -1) | ||
|
|
There was a problem hiding this comment.
Critical (bump logic is unreachable) + Important (false major) - both on this block.
Critical: PRs land here as merge commits (recent main tips read "Merge pull request ... from adobe/fix/..."), so head_commit.message is the merge-commit subject, which matches neither this regex nor the feat: check on line 58. Every merge falls through to patch, so the documented feat -> minor / ! -> major behavior (lines 7-10) is unreachable in practice. It is also merge-strategy-dependent: a squash-merged PR would classify correctly, so identical changes get different bumps based on which merge button is used.
Important: this check greps the full multi-line $MSG while line 58 greps only $SUBJECT. Since grep -E '^...' anchors per line, the text BREAKING CHANGE or a x!: token anywhere in a commit body forces a false major - which mints vN+1.0.0 and (via release.yaml's force-move) a brand-new vN+1 floating tag no consumer references, while vN stops advancing.
Fix: derive the bump from a reliable source (merged PR title/labels via gh pr view, or scan the push range git log <before>..<after>); read both checks from the same source; anchor the breaking footer as ^BREAKING CHANGE: rather than substring-matching the whole blob. Decide, document, and enforce the merge strategy this depends on.
There was a problem hiding this comment.
Both fixed in 81acd7a.
Bump source is now the merged PR's title, fetched via gh pr view:
SUBJECT=$(printf '%s\n' "$HEAD_COMMIT_MSG" | head -1)
PR_NUM=$(printf '%s' "$SUBJECT" \
| grep -oE 'Merge pull request #[0-9]+' \
| grep -oE '[0-9]+' || true)
if [ -n "$PR_NUM" ]; then
BUMP_SUBJECT=$(gh pr view "$PR_NUM" --repo "$REPO" --json title --jq .title)
else
BUMP_SUBJECT="$SUBJECT" # squash / rebase / direct push fallback
fiSo the bump decision is now deterministic across merge strategies — same PR title produces the same bump regardless of whether the PR was merge-committed, squashed, or rebase-merged. The "Merge pull request #N from …" case (this repo's default) now resolves to the actual PR title, which is what the conventional-commit prefixes were always meant to live on.
Grep symmetry is fixed — both checks read from BUMP_SUBJECT (a single resolved line), not the full multi-line message:
if printf '%s' "$BUMP_SUBJECT" | grep -qE '^[a-zA-Z]+(\([^)]+\))?!:'; then
...
elif printf '%s' "$BUMP_SUBJECT" | grep -qE '^feat(\([^)]+\))?:'; thenNo more false majors from prose in commit bodies. I dropped scanning for BREAKING CHANGE in the body entirely — the ! marker covers the common case and there's no clean way to substring-match the body without re-introducing the false-major risk. Happy to add ^BREAKING CHANGE: as a footer-only check (parsing trailers properly) if you want full conventional-commit footer support, but I'd treat that as a follow-up.
Merge strategy enforcement: the logic handles all three (merge, squash, rebase), so we don't strictly need to enforce one. I can add a note to a README "Releasing" section if you want it documented; left it out of this revision to keep the diff focused.
| - name: Create release | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | |
There was a problem hiding this comment.
Important: not idempotent, and failures are silent. gh release create vX errors if the tag/release already exists, so a re-run after a partial failure - or two merges that compute the same NEXT before the first tag is visible - fails the job hard. And a failed auto-release reproduces exactly the gap this PR exists to close (merge landed, consumers get nothing) with no notification; the only signal is a red run on an infra repo service teams do not watch.
Fix:
- Re-fetch tags before computing:
git fetch --force --tags originas the first line of the compute step (removes the dependency on checkout timing). - Guard the create:
if gh release view "$NEXT" >/dev/null 2>&1; then echo "exists, skipping"; exit 0; fi, or treat "already exists" as success. - Add an
if: failure()step that notifies (Slack/issue) so a stuck release is noticed.
There was a problem hiding this comment.
Re-fetch and idempotency guard fixed in 81acd7a:
# Compute next tag step:
git fetch --force --tags origin >/dev/null 2>&1
# Create release step:
if gh release view "$NEXT_TAG" --repo "$REPO" >/dev/null 2>&1; then
echo "Release $NEXT_TAG already exists — skipping."
exit 0
fi
gh release create "$NEXT_TAG" --repo "$REPO" --target "$TARGET_SHA" \
--title "$NEXT_TAG" --generate-notesSo:
- Racing runs see each other's tags before computing → no duplicate
NEXT. - A re-run after partial failure (tag exists, Release doesn't or vice versa) exits cleanly instead of erroring.
- Concurrent runs that did both compute the same
NEXT— the slower one sees the Release and exits 0.
Failure notification is the one piece I didn't ship in this revision. The repo doesn't currently wire any Slack/webhook notifications from CI workflows, and I didn't want to introduce that pattern here without knowing the right convention (which Slack channel, what secret holds the webhook). Two options:
- Add an
if: failure()step in a follow-up that posts to a Slack webhook — point me at the channel + secret and I'll wire it up. - Keep the signal as the workflow's red badge for now; revisit if we ever see a stuck release in practice.
Happy with either, slight preference for (2) until the rest of the design proves out in practice — but if you'd rather start with notification on day one, (1) is cheap.
- script injection: route head_commit.message through env (no inline expansion) - bump source: read merged PR title via gh pr view (merge commits never matched feat:/!: prefixes; would default to patch always). Falls back to head commit subject for squash/rebase/direct pushes. - grep symmetry: both major and minor checks now anchor to the same resolved subject (no more false majors from prose in commit bodies) - idempotency: re-fetch tags before computing; gh release view guard before create - prerelease safety: tag glob anchored ^vN.N.N$ (excludes future -rc suffixes)
|
Thanks @solaris007 — all four blocking items addressed in 81acd7a. Replies inline: Critical 1 — script injection ✅Both Critical 2 — bump logic under merge commits ✅The job now extracts the PR number from the merge subject and reads the PR's title via This also makes the rules deterministic across merge strategies — same PR, same bump. Important 3 — asymmetric grep ✅Both bump checks now match against the same resolved subject ( Important 5 — idempotency + failure visibility (partial) ✅ /
|
solaris007
left a comment
There was a problem hiding this comment.
Hey @akshaymagapu,
Strong revision - thanks for the thorough turnaround in 81acd7a. I re-verified all five round-1 findings and they are genuinely addressed (Strengths below), and I accept your two judgment calls (release-on-every-merge, and deferring the failure notification) with the reasoning below. The re-review did surface one new blocker: as written, the release chain will not actually move the floating vN tag, because the Release is created with the default GITHUB_TOKEN.
Strengths - previously flagged, now fixed
- Script injection (was Critical): fully closed.
head_commit.messageflows throughenv: HEAD_COMMIT_MSG(line 58) and is only ever handled as quoted data; no inline${{ }}remains in anyrun:body, and the new PR-title path (BUMP_SUBJECTviagh pr view,PR_NUM,SUBJECT) reaches no shell sink. Matches theservice-ci.yamlbranch-name precedent you cited. - Bump source unreachable under merge commits (was Critical): fixed. Extracting the PR number from the merge subject and reading the real PR title via
gh pr view(lines 85-91) makes the bump correct and deterministic across merge/squash/rebase. Traced this PR's own merge: titlefeat(ci): ...-> minor -> v2.1.5 to v2.2.0. - Asymmetric grep / false major (was Important): fixed. Both checks now read the single resolved line
BUMP_SUBJECT(lines 99, 102); the bodyBREAKING CHANGEscan is gone. - Idempotency, tag-collision race (was Important):
git fetch --force --tags(line 66) + thegh release view ... && exit 0guard (lines 126-129) + the concurrency group close the two-merges-racing case cleanly. - Minors: prerelease glob anchored (line 71), PATCH sed anchored (line 79), header comment rewritten to match behavior,
pull-requests: readcorrectly scoped.
On the two items you pushed back on
- Release-on-every-merge (no human gate): accepted. Your cost-benefit holds for a CI-workflow repo - a gate is a per-merge tax forever and reintroduces the "someone must remember" failure mode this PR kills, while the reversal (a one-line
environment:with required reviewers on release.yaml's tag-move job) is cheap to add on the first bad fan-out incident. One nuance worth recording: a gate is not purely a delay - it would prevent fleet exposure for the narrow "do not roll this to everyone right now" case (release freeze, mid-incident, canary first) - but that class is rare and recoverable. Please capture the decision durably (see Recommendations). - Deferring the failure notification: accepted as a follow-up. The repo has no existing Slack/webhook convention and inventing it here would be guessing, and the most common failure (tag collision) now exits 0 cleanly. One contingency: make the remaining failures legible (see the Minor on the
git fetchredirect) and file the notification follow-up as a tracked issue with the channel + secret to use.
Issues
Critical (Must Fix)
- The release chain is broken: a Release created with
GITHUB_TOKENwill not triggerrelease.yaml, so the floatingvNtag never moves (lines 116, 131). The intended chain is: auto-release publishes a Release -> release.yaml fires onrelease: published-> movesvN-> consumers on@vNpick it up. But GitHub has an intentional anti-recursion rule: events triggered using the repositoryGITHUB_TOKENdo not start new workflow runs. The create step authenticates withGH_TOKEN: ${{ github.token }}(line 116), sorelease: publishedwill not fire and release.yaml will not run. I verified why it works manually today: all eight existing releases were published by humans (danieljchuser, alinarublea, solaris007), whose events do fire release.yaml. Automating the publish with the default token removes the human identity that was making the chain work. The PR will look successful (Release created, job green) whilevNsilently stops advancing - a worse failure than the manual process it replaces, because it looks automated. Fix, pick one: (a) create the Release with a PAT or GitHub App token that hascontents: write(the repo already referencesADOBE_BOT_GITHUB_TOKENin service-ci.yaml) sorelease: publishedfires; or (b) collapse the two workflows - have auto-release.yaml move thevNtag itself aftergh release create, removing the cross-workflow event dependency. Option (b) is cleaner now that releases are automated: the split only existed because releases used to be manual.
Important (Should Fix)
-
gh pr viewfailure aborts the job and silently drops the release (line 91).BUMP_SUBJECT=$(gh pr view "$PR_NUM" ...)is a bare command substitution underset -euo pipefailwith no fallback. A transient API error, rate limit, or not-found PR exits the step non-zero -> no Release for that merge -> consumers get nothing, signalled only by a red run. This trades round-1's "visible wrong bump" for "invisible no-release," the worse direction for release automation. Fix: degrade to the merge subject (a safe patch bump) on failure, with a warning. Fold in a| head -1on the PR_NUM pipeline (line 86) so a branch name containing a second "Merge pull request #N" cannot produce a multi-line PR_NUM. -
A manual re-run (or a create-then-error) cuts a duplicate release, because idempotency is keyed off the computed tag, not the merge SHA (lines 70, 126). The
gh release view "$NEXT" && exit 0guard only blocks re-creating the identical tag. On a re-run,git fetchsees the tag the first run created,LATESTadvances, and afeat:recomputes a differentNEXT(e.g. v2.2.0 to v2.3.0) -> a second Release for the same commit, and release.yaml movesvNagain. Lower-probability than the race you fixed, but the same non-idempotency class. Fix: before computing, check whether the currentgithub.shahas already been released and exit 0 if so, rather than keying off the computed tag.
Minor (Nice to Have)
git fetch --force --tags origin >/dev/null 2>&1(line 66) swallows stderr, so a fetch failure fails the job with no diagnostic in the log. Drop the2>&1(keep>/dev/null) so the failure is legible - this is the contingency that makes deferring the Slack notification acceptable.- Case asymmetry between the bump regexes (lines 99, 102): the major check is case-insensitive on the type token (
FEAT!:-> major) while the minor check is lowercase-only (Feat:-> patch). Harmless for well-formed lowercase titles; align them if you want consistency.
Recommendations
- Once the token/trigger issue is fixed, add a sentence to the header comment describing how the Release event reaches release.yaml (the bot identity, or "this workflow moves the tag directly"). That mechanism is the load-bearing part of the design and is currently undocumented.
- Capture the release-on-every-merge risk-acceptance durably (a README "Releasing" section or a comment block): every merge auto-releases with immediate fan-out and no gate, this is intentional, rollback is a manual
vNforce-move, and the trigger to add a required-reviewer Environment is "first bad fan-out incident." Fold in the bump rules and the[skip release]note. - File the failure-notification follow-up as a tracked issue now (channel + secret to use), so it does not evaporate after merge.
- The two-workflow baton-pass (minting a Release purely to trigger release.yaml to move
vN) is acceptable - the immutablevN.N.NRelease earns its keep as an audit trail and rollback anchor - but note the coupling: thevNmove is only reachable through a successful Release publish. If you take fix (b) for the Critical, this resolves itself. - Reconcile the existing drift (v2.0.0/v2.1.4/v2.1.5 are tags without Releases;
vNlags main) before this lands, so the first auto-run starts clean. - Extracting the bump logic to a script with table-driven smoke tests remains a good follow-up; fine to defer to keep this diff focused.
Assessment
Ready to merge? No, with fixes - but the bar is one real blocker, not a redo. The revision resolved all five round-1 findings cleanly and your two judgment calls are sound. The blocker is new and structural: as written, the Release is created by GITHUB_TOKEN, which by GitHub's anti-recursion design will not trigger release.yaml, so the floating vN tag - the entire deliverable - never moves (Critical 1). Fix the token/trigger coupling, add the gh pr view fallback (Important 2) and the SHA-based idempotency guard (Important 3), and this is in good shape. Note: CI is green.
Next Steps
- Fix the release-trigger coupling (Critical 1) - PAT/App token for the create step, or collapse the tag-move into this workflow. This is the one that makes the PR actually work.
- Add the
gh pr viewfallback +head -1on PR_NUM (Important 2), and the SHA-keyed idempotency guard (Important 3). - Drop the
2>&1on the git fetch (Minor) so failures are legible; align the regex case if you want. - Capture the release-model decision and file the notification follow-up (Recommendations).
|
|
||
| - name: Create release | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
Critical: this Release is created with the default GITHUB_TOKEN, which will not trigger release.yaml. The intended chain is: publish a Release here -> release.yaml fires on release: published -> moves the floating vN. But GitHub's anti-recursion rule means events triggered using the repository GITHUB_TOKEN do not start new workflow runs, so release: published will not fire and vN never moves.
Verified: all 8 existing releases were human-published (danieljchuser / alinarublea / solaris007), which is why the chain works manually today. Automating the publish with the default token removes exactly the human identity that was making it work. As written the job goes green while vN silently stops advancing - worse than the manual process, because it looks automated.
Fix, pick one:
- (a) create the Release with a PAT or GitHub App token that has
contents: write(the repo already referencesADOBE_BOT_GITHUB_TOKENin service-ci.yaml) sorelease: publishedfires; or - (b) move the
vNtag directly in this workflow aftergh release createand drop the cross-workflow event dependency. Cleaner now that releases are automated - the two-workflow split only existed because releases used to be manual.
There was a problem hiding this comment.
Fixed in 9a1b7d4. Took option (b) — collapse the cross-workflow event handoff and move vN inline. After gh release create, the same step now also moves the major tag directly via gh api:
MAJOR_TAG=$(echo "$NEXT_TAG" | grep -oE '^v[0-9]+')
if gh api "/repos/$REPO/git/ref/tags/$MAJOR_TAG" >/dev/null 2>&1; then
gh api -X PATCH "/repos/$REPO/git/refs/tags/$MAJOR_TAG" \
-f sha="$TARGET_SHA" -F force=true
else
gh api -X POST "/repos/$REPO/git/refs" \
-f ref="refs/tags/$MAJOR_TAG" -f sha="$TARGET_SHA"
fiSame gh api shape as release.yaml so the two paths stay aligned.
Picked (b) over (a) for two reasons:
- No PAT/secret rotation problem. The
ADOBE_BOT_GITHUB_TOKENpath works, but it introduces a credential dependency for what's now a self-contained workflow. The only reason the cross-workflow handoff existed was that releases used to be manual. - Faster + simpler. No "Release published → wait for
release.yamlrun → tag finally moves" two-hop. The tag moves in the same step as the Release create.
release.yaml stays in place as the path for manual Releases (created via the GitHub UI by a real user) — workflow_run does fire for those, and we shouldn't break the existing escape hatch. Both paths now end at the same place (vN pointing at the right commit) via two different mechanisms appropriate to their trigger.
The header comment on auto-release.yaml also documents this design choice explicitly so future readers don't have to reverse-engineer why the tag move lives in this workflow instead of release.yaml.
| | grep -oE '[0-9]+' \ | ||
| || true) | ||
| if [ -n "$PR_NUM" ]; then | ||
| BUMP_SUBJECT=$(gh pr view "$PR_NUM" --repo "$REPO" --json title --jq .title) |
There was a problem hiding this comment.
Important: a gh pr view failure here silently drops the release. This bare command substitution runs under set -euo pipefail with no fallback, so a transient API error, rate limit, or not-found PR aborts the step -> no Release for this merge -> consumers get nothing, signalled only by a red run. That trades round-1's "visible wrong bump" for "invisible no-release," the worse direction for release automation.
Degrade to the merge subject (a safe patch bump) instead of aborting:
BUMP_SUBJECT=$(gh pr view "$PR_NUM" --repo "$REPO" --json title --jq .title 2>/dev/null || true)
if [ -z "$BUMP_SUBJECT" ]; then
echo "::warning::gh pr view $PR_NUM failed; falling back to merge subject"
BUMP_SUBJECT="$SUBJECT"
fiAlso add | head -1 to the PR_NUM pipeline (line 86) so a branch name containing a second "Merge pull request #N" cannot produce a multi-line PR_NUM (which would itself fail this call).
There was a problem hiding this comment.
Fixed in 9a1b7d4:
if [ -n "$PR_NUM" ]; then
if PR_TITLE=$(gh pr view "$PR_NUM" --repo "$REPO" --json title --jq .title 2>/dev/null); then
BUMP_SUBJECT="$PR_TITLE"
echo "Bump source: PR #$PR_NUM title -> $BUMP_SUBJECT"
else
# Degrade gracefully — better a safe patch bump than dropping
# the release entirely.
BUMP_SUBJECT="$SUBJECT"
echo "::warning::gh pr view #$PR_NUM failed; falling back to merge subject (patch bump)"
fi
else
...
figh pr view is now in if-then-else, so a non-zero exit doesn't blow up the step under set -e. On failure we fall back to the merge subject — which never matches feat: or *!:, so it yields a patch bump. Worst case: a feat: PR ships as a patch instead of a minor on the rare API failure, which is recoverable on the next release; best case: the bump is correct. Either way, the Release ships.
The ::warning:: annotation gives a yellow badge on the run so degradation is visible without a notification path.
Also added | head -1 on the PR_NUM pipeline as you suggested:
PR_NUM=$(printf '%s' "$SUBJECT" \
| grep -oE 'Merge pull request #[0-9]+' \
| grep -oE '[0-9]+' \
| head -1 \
|| true)— so a branch name like Merge pull request #99 from foo appearing inside the subject can't produce a multi-line PR_NUM.
| # Idempotency guard: if a Release for this tag already exists | ||
| # (e.g. created by a racing run that finished a moment earlier), | ||
| # skip cleanly instead of failing the job. | ||
| if gh release view "$NEXT_TAG" --repo "$REPO" >/dev/null 2>&1; then |
There was a problem hiding this comment.
Important: idempotency is keyed off the computed tag, not the merge SHA, so a re-run cuts a duplicate release. This guard blocks re-creating the identical tag, but on a manual re-run (or a create-then-error), git fetch sees the tag the first run created, LATEST advances, and a feat: recomputes a different NEXT (e.g. v2.2.0 -> v2.3.0) -> a second Release for the same commit, and release.yaml moves vN again.
Lower-probability than the race you fixed (it needs a re-run of a successful job, or a create that errors after creating), but the same non-idempotency class. Gate on whether the current github.sha has already been released (check existing releases' target commit) and exit 0 if so, rather than keying off the computed tag - that makes re-runs and double-pushes no-ops regardless of tag arithmetic.
There was a problem hiding this comment.
Fixed in 9a1b7d4 — SHA-keyed guard added before computing:
# SHA-keyed idempotency: if a Release already targets this exact
# commit (re-run, retried merge, etc.), exit cleanly. Tag-keyed
# checks alone don't catch this — a re-run sees the previous tag
# in `git tag`, recomputes a NEW NEXT, and would mint a second
# Release on the same commit.
EXISTING_TAG=$(gh release list --repo "$REPO" --json tagName,targetCommitish --limit 100 \
| jq -r --arg sha "$TARGET_SHA" '.[] | select(.targetCommitish == $sha) | .tagName' \
| head -1)
if [ -n "$EXISTING_TAG" ]; then
echo "Commit $TARGET_SHA already released as $EXISTING_TAG — skipping."
echo "skip=true" >> "$GITHUB_OUTPUT"
exit 0
fisteps.t.outputs.skip then gates the publish step (if: steps.t.outputs.skip != 'true'), so a re-run on an already-released commit completes the workflow green without minting a duplicate.
The tag-keyed gh release view "$NEXT_TAG" guard inside the publish step is kept as a second line of defence — it handles the narrower "two concurrent runs raced past the SHA check and both computed the same NEXT" case.
Critical: move the floating vN tag inline (don't rely on release.yaml). GitHub suppresses workflow_run events triggered by the default GITHUB_TOKEN, so a Release created by this workflow would never fire release.yaml. Workflow now moves the major tag directly, mirroring release.yaml's gh-api pattern. release.yaml stays for manual Releases (where the actor is a real user and the trigger fires correctly). Important: gh pr view failure no longer drops the release. Wrapped in if/else with fallback to the merge subject (safe patch bump) and a workflow ::warning:: so the degradation is visible. Also added head -1 on the PR_NUM pipeline to guard pathological branch names containing 'Merge pull request #N'. Important: SHA-keyed idempotency. Earlier guard only checked the computed tag. A re-run would see the previous tag in git, recompute a different NEXT, and mint a second Release for the same commit. Now checks whether any existing Release targets the current commit and exits cleanly if so. Minor: - git fetch keeps stderr (drop 2>&1) so failures are legible. - Bump regexes both lowercase-only — consistent and matches conventional-commits convention. Header comment rewritten to describe the new mechanism explicitly (why vN is moved inline, fallback behaviour, etc.).
|
Round 2 fixes pushed in 9a1b7d4. Inline replies on each comment. Summary:
Recommendations status
What changes when this mergesFirst auto-run will see PR #15's |
solaris007
left a comment
There was a problem hiding this comment.
Hey @akshaymagapu,
Strong round-3 turnaround on what round 2 flagged - the injection fix holds, the inline vN move is the right call, and the gh pr view fallback is clean. I verified each against the live repo. But the idempotency guard added in this commit does not work as written: it calls a gh field that does not exist, which aborts the job on every run. Because this workflow executes only on push to main (never in PR CI), the green checks here do not exercise it, so this surfaces only after merge. There is also a related gap where a re-run after a partial failure leaves the floating tag silently stale.
Strengths
- The round-2 Critical (vN never moves) is genuinely resolved, and option (b) is the right architecture. Moving vN inline via
gh api(lines 169-181) while keeping release.yaml for human-published Releases correctly works around GitHub's GITHUB_TOKEN anti-recursion rule. The two trigger paths are disjoint (auto on push under GITHUB_TOKEN, manual on a human Release), so they cannot race to double-move vN. The move mirrors release.yaml exactly, including the subtle singular/git/ref/tags/...existence check vs plural/git/refs/...for PATCH/POST, and-F force=true. - The round-1 script-injection fix holds after the round-3 edits. Tracing the untrusted path (
HEAD_COMMIT_MSGtoSUBJECTtoPR_NUM/BUMP_SUBJECT), every consumer is a quotedprintf '%s' | grepor a quoted test; nothing reaches eval, a command position, or$GITHUB_OUTPUT. The new values (TARGET_SHA,NEXT_TAG,MAJOR_TAG) are trusted contexts or numeric-clean computed tags, so no new injection or argument-injection surface was added.gh pr view "$PR_NUM"is safe because PR_NUM is strictly numeric. - Round-2 Important 2 (gh pr view) is fixed cleanly: wrapping the call in
if PR_TITLE=$(... 2>/dev/null); then ... else fallback + ::warning:: fiis the set-e-safe way to degrade to a patch bump instead of aborting, andhead -1plus|| truecorrectly handle the no-match case. - Round-2 Minors fixed:
git fetchkeeps stderr visible (failures abort red and are legible), and both bump regexes are lowercase-only and consistent. - Token scope is least-privilege and correct (
contents: write+pull-requests: read),persist-credentials: false, and theon: pushtrigger checks out post-merge code so there is no pwn-request exposure.concurrencywithcancel-in-progress: falseserializes runs so vN settles on the latest merge. - The header comment documents the inline-move rationale, which is the load-bearing design context a future maintainer needs.
Issues
Critical (Must Fix)
- The idempotency guard calls a non-existent gh field and hard-fails the job on every run (line 81, "Compute next tag"). Also posted inline.
gh release list --jsondoes not supporttargetCommitish. Verified live (gh 2.92.0):
$ gh release list --repo adobe/mysticat-ci --json tagName,targetCommitish --limit 100
Unknown JSON field: "targetCommitish"
Available fields: createdAt, isDraft, isImmutable, isLatest, isPrerelease, name, publishedAt, tagName
targetCommitish exists only on gh release view --json, not on release list. Because this is the first command in a set -euo pipefail block and its output is captured (EXISTING_TAG=$(...)), the non-zero exit aborts the step before any tag is computed, any Release is created, or vN is moved. I reproduced the exact pipeline end to end: it exits 1 and never reaches the next statement. Net effect: the first merge to main fails "Compute next tag" and no Release ever publishes - the feature is dead on arrival. It passed two rounds and green CI because the workflow triggers only on push to main, so the PR checks never execute these gh calls; the break surfaces only post-merge.
This also answers the round-2 question "does targetCommitish equal the full SHA": where the field does exist (gh release view / the REST API), all current releases in this repo return target_commitish: "main", not a SHA, so SHA-equality would not match them anyway. Prefer a SHA-native check that does not touch target_commitish. Right after the existing git fetch --force --tags:
EXISTING_TAG=$(git tag --points-at "$TARGET_SHA" | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | head -1 || true)This returns the semver tag at an already-released commit, returns empty with exit 0 when none (safe under set -e), and sidesteps the --limit 100 boundary entirely.
- A re-run after a partial failure leaves vN silently stale with a green job (line 87
exit 0, plus the publish gateif: steps.t.outputs.skip != 'true'at line 148). Also posted inline.
The publish step does two non-atomic things: create the Release, then move vN. If the create succeeds but the vNgh apiPATCH/POST then fails (transient API error, or - plausible in an Adobe org - av*tag-protection rule that GITHUB_TOKEN cannot force-update), the job goes red with vN stale (correct so far). The operator does the routine thing and re-runs the job. On the samegithub.sha, the (fixed) guard finds the Release already targets this commit, setsskip=true, andexit 0short-circuits the entire publish step, so the vN move is never retried. The job goes green, but vN still points at the old commit. This reproduces the exact round-2 Critical (green job, fleet-wide stale vN), now triggered by the standard recovery action, and it does not self-heal until a later successful release leapfrogs it. If the cause is a persistent tag-protection rule, every merge red-then-greens into a permanently stale vN with no signal.
The vN move is already idempotent (force PATCH to a SHA, POST if missing), so it is always safe on a re-run. Fix together with finding 1: when a release already exists for this SHA, do notexit 0. Instead settag=$EXISTING_TAGandskip=falseand let the publish step run - its existinggh release viewguard skips the duplicate create, and the vN move reconciles. That makes re-runs go green only when vN is actually correct. Reusing$EXISTING_TAGalso avoidsgit tagseeing the new version as LATEST and minting a spurious next version.
Minor (Nice to Have)
- A successful-but-wrong PR lookup chooses the wrong bump with no signal (lines 114-122). PR_NUM is parsed from the merge subject; a direct push to main whose subject happens to contain the literal
Merge pull request #Nfor an unrelated N will resolve this repo's PR N and use its title for the bump. The::warning::only fires whengh pr viewfails, not when it succeeds on the wrong PR. Low probability under the normal merge flow; a log line recording the resolved title and chosen bump would make it auditable. - No post-move read-back (lines 169-181). The PATCH/POST prints the ref object so success is legible, but an explicit assert that
refs/tags/$MAJOR_TAGresolves to$TARGET_SHAwould surface a silently-wrong move and complements the structural fix in finding 2.
Recommendations
- The invalid-field Critical is the strongest argument for the already-agreed follow-up to extract the bump/idempotency logic into a testable script. A table-driven unit test on a runner would have caught a guard bug that only executes post-merge. Worth prioritizing.
- The four-line force-move idiom is now byte-identical in auto-release.yaml and release.yaml, including the singular/plural endpoint subtlety that is easy to get wrong. Consider extracting a shared composite action (e.g.
.github/actions/move-major-tag) both call; at minimum add a cross-reference comment in each pointing at its twin. (release.yaml has no checkout today, so this touches that file, reasonable to schedule rather than block here.) - Add observability to the bump decision: log the resolved bump and its source, and emit a
::warning::on near-miss titles (a capitalized type likeFeat:or a leading space) that silently collapse to patch. Today every route to patch except the gh pr view failure is silent, and with release-on-every-merge a wrong bump is a published, hard-to-retract Release. - Give
MAJOR_TAG=$(echo "$NEXT_TAG" | grep -oE '^v[0-9]+')an explicit empty-check for parity with release.yaml'sif [ -z "$MAJOR" ], so a miss fails with a clear message rather than relying on set -e. - Document the merge-strategy assumption next to the bump rules: PR_NUM extraction assumes the
Merge pull request #Nmerge-commit subject. Squash degrades safely to the subject; rebase carries mis-bump risk. - From a security standpoint, bump magnitude is influenceable by the PR title (a
feat!:title forces a major). Impact is limited - the tag always points at the human-mergedgithub.sha, so an attacker cannot redirect a tag onto their own commit, and a forced major creates a new vN+1 line while leaving the pinned vN untouched. If you ever want to close it, derive the bump from a maintainer-applied label or verified git trailers. Not a blocker. - Once finding 1 is fixed, the deferred
if: failure()notification becomes more valuable, because the dangerous case is precisely a run that should stay red being flipped green by a re-run. Keep it on the tracked follow-up. - A short rollback note would close the "bad vN shipped at 2am" gap: recovery is a revert/fix commit to main (mints a new release and advances vN), or an emergency manual
gh api -X PATCH .../git/refs/tags/vN -f sha=<good> -F force=true.
Out of scope, worth tracking
- This repo already has tag/Release drift: v2.0.0, v2.1.4, and v2.1.5 exist as tags with no matching GitHub Release (Releases stop at v2.1.3). The workflow computes LATEST from tags but checks existence against Releases, so the two views can diverge. Already noted as a deferred reconcile item; worth doing before the first auto-run so it starts clean.
Assessment
Ready to merge? No.
Reasoning: The round-2 findings are genuinely addressed and the design is sound, but the new idempotency guard calls an invalid gh field that hard-fails the job under set -euo pipefail, so no Release would ever publish, and PR CI cannot catch it because the workflow runs only post-merge. The related vN-stale-on-rerun gap is a silent-wrong-outcome to fix in the same pass. Note: CI checks here are green, but they do not execute this workflow.
Next Steps
- Fix the idempotency guard (finding 1) - replace
gh release list --json targetCommitishwith a SHA-native check such asgit tag --points-at "$TARGET_SHA". - In the same pass, close the re-run gap (finding 2) - when a release already exists for this SHA, set
skip=falseandtag=$EXISTING_TAGinstead ofexit 0, so the idempotent vN move always reconciles. - The two Minors and the Recommendations are optional, though the testable-script extraction would have caught finding 1 pre-merge.
| # checks alone don't catch this — a re-run sees the previous tag | ||
| # in `git tag`, recomputes a NEW NEXT, and would mint a second | ||
| # Release on the same commit. | ||
| EXISTING_TAG=$(gh release list --repo "$REPO" --json tagName,targetCommitish --limit 100 \ |
There was a problem hiding this comment.
Critical: gh release list --json does not support targetCommitish - that field exists only on gh release view --json. Verified live (gh 2.92.0):
$ gh release list --repo adobe/mysticat-ci --json tagName,targetCommitish --limit 100
Unknown JSON field: "targetCommitish"
Available fields: createdAt, isDraft, isImmutable, isLatest, isPrerelease, name, publishedAt, tagName
As the first command in this set -euo pipefail block, captured in EXISTING_TAG=$(...), the non-zero exit aborts the step on every run, before any tag is computed, any Release is created, or vN is moved. I reproduced the exact pipeline end to end: exit 1, never reaching the next statement. The feature never publishes a Release. PR CI does not catch this because the workflow runs only on push to main, not in PR checks.
Note: where the field does exist, all current releases in this repo report target_commitish: "main", not a SHA, so a SHA-equality match would not fire against them anyway. Prefer a SHA-native check that avoids target_commitish entirely, right after git fetch --force --tags:
EXISTING_TAG=$(git tag --points-at "$TARGET_SHA" | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | head -1 || true)This returns the semver tag at an already-released commit, returns empty with exit 0 when none (safe under set -e), and sidesteps the --limit 100 boundary.
There was a problem hiding this comment.
Fixed in 1cf7b16. Replaced the gh release list --json targetCommitish lookup with a SHA-native git check exactly as you suggested:
EXISTING_TAG=$(git tag --points-at "$TARGET_SHA" \
| grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' \
| head -1 \
|| true)Three properties that make this the right shape:
- SHA-native. No dependency on
target_commitish, which in this repo holds the branch name"main"rather than a SHA — so even with the correct field name a SHA-equality check would never have matched. - Safe under
set -e.git tag --points-atreturns exit 0 with empty stdout when no tag matches; the|| truedefends against the rare exit-1 case (no tags repo-wide). - Bounded by local tags, not API pagination. The previous
--limit 100could have silently missed older tags once the repo grew past 100 Releases.
Good catch — the fact that PR CI never executes this workflow is exactly why a runtime-only bug like this needs a testable script with table-driven smoke tests. Adding to the deferred follow-ups.
| if [ -n "$EXISTING_TAG" ]; then | ||
| echo "Commit $TARGET_SHA already released as $EXISTING_TAG — skipping." | ||
| echo "skip=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 |
There was a problem hiding this comment.
Critical (fix together with the guard above): this exit 0 short-circuits the entire publish step via the if: steps.t.outputs.skip != 'true' gate (line 148), including the idempotent vN move - not just the Release create.
Failure sequence: gh release create succeeds, then the vN gh api PATCH/POST fails (transient error, or a v* tag-protection rule that GITHUB_TOKEN cannot force-update) -> job red, vN stale (correct so far). Operator re-runs the job. On the same github.sha, the (fixed) guard finds the Release already targets this commit, sets skip=true, and this exit 0 skips the publish step -> the vN move is never retried -> job goes green with vN still pointing at the old commit. That reproduces the round-2 Critical (green job, fleet-wide stale vN) under the routine recovery action, and it does not self-heal until a later release leapfrogs it.
The vN move is already idempotent, so it is always safe on a re-run. Instead of exit 0 here, set tag=$EXISTING_TAG and skip=false and let the publish step run - its existing gh release view guard skips the duplicate create while the vN move reconciles. Re-runs then go green only when vN is actually correct. Reusing $EXISTING_TAG also avoids git tag seeing the new version as LATEST and minting a spurious next version.
…observability Critical 1 — gh release list --json targetCommitish doesn't exist. Replaced the SHA-keyed guard with git tag --points-at, which is SHA-native, safe under set -e (returns empty + exit 0 when no match), and sidesteps the gh release list --limit 100 boundary. The previous guard would have hard-failed the job on every push to main; PR CI didn't catch it because the workflow runs only post-merge. Critical 2 — re-run after partial failure left vN silently stale. Previously: existing release for this SHA -> skip=true -> entire publish step short-circuits -> green job, vN still stale. Now: reuse the existing tag ($EXISTING_TAG), skip=false, let the publish step run. The existing 'gh release view' guard skips the duplicate create but the vN move (idempotent: force PATCH or POST) executes and reconciles whatever was stale. Observability: - ::notice:: logging the chosen tag, bump, source, and resolved title so post-merge release decisions are auditable. - ::warning:: on near-miss conventional-commit prefixes (Feat:, leading whitespace, missing colon) that silently collapse to patch. Defensive: - Empty-check on MAJOR_TAG with a clear error (parity with release.yaml). - Read-back assert on the vN move (verify the tag actually resolves to TARGET_SHA after PATCH/POST). Header comment updated: - Documents merge-strategy assumption (merge-commit reads PR title, squash carries it in subject directly, rebase/direct carry mis-bump risk). - Documents idempotency rationale and why reconciliation is non- short-circuiting.
Strip out narrative comments that re-stated what the code obviously
does. Keep only load-bearing notes: the vN-move-inline rationale,
the singular vs plural /git/ref(s) gotcha, the reuse-on-rerun guard
intent, and the bump-rules summary. Collapse single-use single-line
guards ("[ -n "$X" ] || { echo ERROR; exit 1; }"). Drop verbose
header sections; the bump rules and trigger model are still
documented but each in one or two lines.
No behaviour change — diff is comments and one-line shell collapsing
only. File now 131 lines vs 215.
Critical:
- C1: sanitize untrusted strings before workflow-command echoes. PR title
flowing into ::notice:: / ::warning:: was an injection vector — runner
parses workflow commands from any step's stdout, so a PR titled
'feat: foo\n::add-mask::secret' would be honored. Strip CR/LF and
neutralize :: sequences before echo. Truncate to 200 chars.
- C2: resolve PR by SHA via /commits/{sha}/pulls, not by parsing the
merge commit subject. The old path was spoofable by anyone with
direct-push to main (forge a 'Merge pull request #999' line referencing
an unrelated feat!: PR to force a major).
- C3: retry gh pr view 3x with backoff, fail loudly if all attempts
fail. The previous silent-fallback to merge subject downgraded
feat!: PRs to patch during transient API errors.
- C5: check [skip release] in BOTH the merge commit message AND the
PR title (the marker is unreachable in merge commits today because
the auto-generated subject doesn't contain it). Emit ::notice:: when
the skip fires so it's auditable.
- C6: capture and log the previous vN SHA before the PATCH/POST, and
emit a one-line rollback command in the run log. Closes the 3am-
rollback story.
Important:
- I1: ::warning:: on major bumps — consumers pinned to @vn-1 are NOT
updated automatically and must manually bump 'uses:' refs.
- I2: scan PR body for BREAKING CHANGE: footer (per conventional-commits
spec). The ! marker alone misses PRs that declare breaking via the
footer convention.
- I4: SHA-pin actions/checkout@de0fac2 (v6.0.2) instead of @v6 floating.
Floating major is mutable; on a contents:write workflow that ships
to ~10 services, that's an unforced supply-chain risk.
- I5: drop paths-ignore. A feat!: PR touching only LICENSE or docs/**
skipped silently — the trigger filter and bump source are decoupled.
vN move is idempotent, so no-op patch releases for pure-docs PRs are
fine.
- I6: verify the underlying tag's SHA matches TARGET_SHA before skipping
create when a Release already exists. Catches drift between Release
object and tag.
- I7: --prune-tags on git fetch so stale local tags (after a force-
delete on origin during recovery) don't send the workflow down the
reuse path.
- I8: job timeout-minutes: 10. Default 6-hour timeout would tie up the
concurrency slot on a hung gh api call.
- I10: 'sort -V | tail -1' on git tag --points-at for deterministic
ordering when multiple semver tags point at one commit.
Other:
- Use ::error:: annotation for tag-not-found and bad-NEXT_TAG cases.
- fetch-depth: 1 (was 0). Workflow only reads tags + head commit.
- NEXT_TAG shape validation before any side effects.
- Step summary write: one-screen post-mortem artifact with target SHA,
previous vN SHA, and explicit rollback command.
Deferred to follow-ups (will file as issues before this merges):
- Extract bump logic into testable script (.github/scripts/compute-next-tag.sh)
- README 'Releasing' section + RUNBOOK.md
- CODEOWNERS for .github/workflows/**
- dependabot.yml for actions ecosystem
- release.yaml deletion or rewrite (C4)
- Failure notification (Slack webhook)
- Existing tag/Release drift reconcile (v2.0.0, v2.1.4, v2.1.5)
Diff size: +138/-29 (file 131 -> 240 lines).
QA Criticals:
- [skip release] now anchored to word boundaries (whitespace or edge on
both sides) AND case-insensitive AND accepts space-or-other whitespace
between 'skip' and 'release'. Prose mentions like 'Reverts the [skip
release] mechanism' inside a PR title no longer match. Helper
matches_skip() shared across both checks.
- BREAKING CHANGE: footer detection now extracts the LAST paragraph of
the PR body (per conventional-commits spec) using awk RS='' paragraph
mode before grepping. Quoted spec mentions in earlier paragraphs no
longer trigger a false major.
Important items folded in:
- I-A sanitize residual ::: scrubs ALL colons via tr ':' '_' (not just
::), eliminating odd-colon-count edge cases.
- I-B /commits/{sha}/pulls: now filters by merge_commit_sha == $sha
AND base.ref == 'main' (jq --arg, cleaner than nested quoting), so a
commit cherry-picked into multiple branches doesn't let the wrong
PR's title drive the bump.
- I-C case glob -> real regex via 'grep -qE'.
- I-D retry helper: small bash function used for the side-effect-free
reads and the PATCH/POST writes. Existence checks (gh release view,
initial PREV_VN_SHA capture) deliberately NOT retried since 'not
found' is a normal outcome there and retry would add 15s per release.
- I-E jq null handling: 'jq -r .title // ""' on both fields.
- I-F error-path echoes now route untrusted values through sanitize().
- I-G release.yaml kept as the manual-Release path; the two triggers
are disjoint (push vs release:published from a human), so no double-
write. Documented the split in the header.
- I-H if: failure() notify step writes an explicit STEP_SUMMARY block
with run URL + computed tag + common-cause checklist. No Slack since
no webhook convention exists yet; the summary is the durable artifact.
Other:
- success() guard on publish step so a compute-step failure doesn't
trigger the publish step via the custom 'if:' bypassing the implicit
'all prior steps succeeded' default.
- BUMP_SOURCE and BUMP_SUBJECT (already sanitized) now passed through
step outputs for inclusion in the step summary.
- Direct push to main now emits ::warning:: when falling back to head
commit subject for the bump.
- PR_NUM resolution uses retry helper.
- timeout-minutes bumped to 15 for headroom.
- Order-locked sed-then-cut in sanitize() with a code comment so a
future refactor can't reorder them.
Yaml parses cleanly. File grew 240 -> 344 lines; the growth is mostly
helpers (sanitize, retry, matches_skip) plus the new notify step and
the failure-summary block. Logic remains a single linear flow.
solaris007
left a comment
There was a problem hiding this comment.
Hey @akshaymagapu,
Round 4. I re-reviewed the full file at HEAD (8f05ec8), focusing on the logic added since round 3 (the sanitize() helper, PR-resolution-by-SHA, the retry-then-fail loop, the BREAKING-CHANGE footer scan, and the reconcile/drift/read-back paths), since none of that is exercised by PR CI. The three round-3 blockers are genuinely resolved, the architecture is sound, and the security posture is strong. What remains is a set of should-fix correctness gaps, all small in-file changes, concentrated in two areas: the bump-source resolution and the reconcile-on-re-run path.
Strengths
Previously flagged, now resolved:
- R3 Critical (invalid
gh release list --json targetCommitishfield that hard-failed every run): replaced with the SHA-nativegit tag --points-at "$TARGET_SHA"(lines 71-72). - R3 Critical (re-run left
vNsilently stale becauseexit 0short-circuited the move): the reconcile path now setsskip=falseand reuses$EXISTING_TAG(lines 71-77), so the publish step always re-drives the idempotent move and the read-back gate (lines 221-224) means the job can only go green whenvNactually resolves toTARGET_SHA. - R2 Critical (a
GITHUB_TOKEN-published Release does not firerelease.yaml): movingvNinline is the correct architecture; the two trigger paths are cleanly disjoint and cannot double-move. - R1 Criticals (run-block script injection; spoofable merge-subject bump source): closed. Untrusted webhook data flows only through
env:(lines 46-50), the privileged publish step receives only the constrainedtag/bump/skipoutputs, and the PR is resolved by SHA rather than by parsing the commit subject.
New this round:
- The
actions/checkoutSHA-pin (line 38) resolves to the authentic v6.0.2 commit;sanitize()correctly neutralizes workflow-command injection at its current call sites (GitHub workflow commands require the literal::, and thesedruns beforecutso truncation cannot reintroduce one); idempotent create + drift detection (lines 181-193) + post-move read-back (lines 221-224) form a solid recovery story; and the retry-then-fail-loud reversal ongh pr view(lines 100-112) is the right call.
Issues
Important (Should Fix)
-
The PR-number resolution silently falls back to the merge subject on a transient API failure, reintroducing the unrecoverable mis-bump you hardened the next call against (lines 90-92). The
gh pr viewcall below got the round-4 retry-then-fail-loud treatment, with the comment that a silentfeat!:->patch downgrade is unrecoverable. But the resolution call that feeds it kept the original behavior:2>/dev/null ... || truecollapses two distinct outcomes - (a) the API succeeded and the commit genuinely has no PR (a real direct push, where falling back to the commit subject is correct), and (b) the API failed transiently, which also yields an emptyPR_NUMand also falls back to the merge subject. On a blip, a merge-commit-mergedfeat!:PR resolves to "Merge pull request ..." (no conventional prefix) and ships as a patch; a squash without the PR body misses a body-onlyBREAKING CHANGE:footer. The repo enables merge, squash, and rebase, none of which guarantee the head subject equals the PR title, so this is reachable on the normal path. Fix: capture the API exit status and only fall back on a genuine empty-success; fail loud (or reuse the same 3x loop) on error. The|| trueis load-bearing for the head-closes-pipe-on-success case, so restructure rather than delete it (see inline comment). -
Re-running a superseded run moves the floating
vNbackward, and goes green (reconcile path, lines 71-77 plus the move at 209-216). GitHub re-runs replay the originalgithub.sha. If run A (SHA_A) published v1.5.0 and moved v1 to SHA_A, and a later run B moved v1 to SHA_B, then re-running A (a natural operator action, especially if A had gone red on a transient read-back lag) takes the reconcile path:EXISTING_TAG=v1.5.0, skip create, then the move runs unconditionally and forces v1 back to SHA_A, the read-back passes (v1 == SHA_A), and the job goes green. The floating tag has silently regressed to the older release for every consumer pinning @v1. The round-3 reconcile fix assumed re-runs are always for the latest release; the move is never bounded to forward-only. Fix: before movingMAJOR_TAG, confirmNEXT_TAG/EXISTING_TAGis the highest vN.N.N tag on that line; if a higher release exists, ensure the release object but skip thevNmove and log a notice (only ever movevNforward). -
The rollback breadcrumb can be a no-op-to-self on a reconcile re-run (lines 203-207, 233-238).
PREV_VN_SHA(line 203) is read after a prior run may have already movedvN. On a reconcile re-run of the latest run (for example after a false-red read-back),PREV_VN_SHAreads the already-moved value (==TARGET_SHA), so both the::notice::hint (line 206) and the step-summary command block (lines 233-238) becomegh api -X PATCH .../tags/vN -f sha=<TARGET_SHA> -F force=true, a roll-to-self. The job is green andvNis correct, but the rollback command in the run an operator is most likely to open does nothing; with the failure notification deferred, this run-log artifact is the recovery story, so its correctness matters. Fix: whenPREV_VN_SHA==TARGET_SHA, suppress the no-op command and point to the prior run's log, or carry the true previous SHA forward.
Minor (Nice to Have)
- Empty-tag guard is dead code under
set -e+pipefail(lines 79-81).LATEST=$(... | grep ... | head -1)has no|| true, unlike lines 72/92. With no semver tag,grepexits 1,pipefailpropagates it, the assignment tripsset -e, and the script exits before line 81, so the::error::no vN.N.N tag in repomessage never prints (a bootstrap or post-recovery repo gets a bare exit). One-token fix: append|| true, matching your pattern elsewhere. - Read-back has no retry (lines 221-224). The comment says it catches eventually-consistent races, but a single GET cannot distinguish a transient race from a true reject, so a read lag false-reds a successful move. Safe direction (never false-green), but it is the upstream trigger for the two reconcile-re-run items above, and every other remote read here already retries. Wrap it in the same 3x backoff.
sanitize()leaves a surviving::on 3+ consecutive colons (line 64).:::xbecomes:_::x. Not exploitable in current use (trstrips newlines and every call site places the value in the data portion after a literal::notice::/::warning::), but since this is a security helper, tighten it to a fixpoint (sed ':a;s/::/:_:/g;ta') so it stays safe if reused where the value can start a line.- Drift check assumes lightweight tags (lines 182-184).
--jq .object.shareturns the tag-object SHA, not the commit, for an annotated tag, so an externally hand-cut annotated vN.N.N would false-positive as drift and fail the re-run. All current tags are lightweight and the workflow creates them lightweight, so this is a narrow blind spot; deref on.object.type == "tag"if you want belt-and-suspenders. - Burst merges can understate the published bump (concurrency lines 26-28 plus the per-PR bump source). The concurrency group keeps one running plus one pending run, so in a burst of 3+ merges the middle runs are cancelled. Each run computes its bump from only its own PR, so if a cancelled middle merge was the major/minor and the survivor is a patch, the published vN.N.N is labelled lower than what shipped (the floating
vNstill lands on HEAD and--generate-notesstill covers the commits, so this is a wrong version number, not lost code). Acceptable to document and accept for a low-traffic repo, consistent with the accepted no-gate stance; the alternative is computing the bump from the full range since the last release tag.
Recommendations
- Testability - the calculus has shifted. You have deferred extracting the bump and
sanitize()logic into a table-tested script every round, and I accepted that in rounds 1-3. At round 4 it is worth revisiting: those are pure string-to-string functions that need no API and no push-to-main to test, they are the highest-churn region of the file, and three of the four review rounds found a defect that was invisible until post-merge in exactly that region. A minimal table-driven harness over(subject, body, LATEST) -> (bump, next_tag)plussanitize()cases is cheap and would have caught the near-miss and guard edges. I would not hard-gate the merge on test extraction alone, but I would bring at least those tests into this PR or the immediate next. Note that Important 1, 2, and 3 are about API timing and re-run semantics and are not unit-testable, so the defensive fixes are required regardless. [skip release]is a substring match (lines 126-127), so a legitimately-titled PR such asfix: prevent [skip release] bypasswould suppress its own release. Not a security issue (author-set, visible to the merger, only freezesvN), but anchor the marker (a commit trailer or standalone token) to remove the footgun.- If this gets standardized across the other spacecat repos that share this release trap, do not copy the file. Publish it once as a reusable workflow (
on: workflow_call) or composite action consumed@v1, so you maintain one tested implementation of the most volatile logic instead of many drifting copies (and it dogfoods the exactvNmechanism this repo provides). Decide the vehicle before the second repo copies this. - Add a one-line comment at the
LATESTcomputation noting thepipefailinteraction so the next maintainer does not "tidy up" the|| trueon the other substitutions.
Out of scope, worth tracking
All already on your deferred list or in other files; tracking only, not blockers here: the force-move idiom now exists in two copies (auto-release.yaml and release.yaml) and has already drifted (release.yaml lacks the read-back and rollback hint and still uses the inline-${{ }} pattern this file was hardened against); release.yaml is now effectively dead for the auto path; the pre-existing v2.0.0/v2.1.4/v2.1.5 tags-without-Releases drift should be reconciled before the first auto-run so it starts clean; the deferred failure notification is what turns the false-red and stale-rollback items into an actual page, so it is worth sequencing soon since those interact; and if the repo is ever made private, the raw git fetch --tags origin at line 57 loses auth.
Assessment
Ready to merge? With fixes. The three round-3 blockers are genuinely resolved, all perspectives agree the architecture and security posture are sound, and the remaining items are small in-file changes, not a redo. The one I would not merge without is the PR-number fallback (Important 1): it is on the normal merge path and reintroduces the unrecoverable mis-bump this PR spent a round eliminating. The two reconcile-re-run items (Important 2 and 3) are narrower (they need a manual re-run) but go green while producing a wrong outcome, which is the failure mode hardest to notice. Note: CI is green, but it does not execute this workflow, so it does not validate any of the above.
Next Steps
- Harden the PR-number resolution (Important 1) so a transient API failure fails loud instead of silently patch-bumping.
- Bound the
vNmove to forward-only (Important 2) and fix the rollback breadcrumb on reconcile re-runs (Important 3); the read-back retry (Minor) removes the false-red that triggers both. - The remaining Minors are quick (the
|| trueone-token fix restores a lost error message); the testability and[skip release]items are the highest-value recommendations. - Capture the burst-merge and no-gate decisions in the planned README "Releasing" section.
|
|
||
| # Anchored [skip release] match — requires whitespace or edge on | ||
| # both sides so prose mentions in the middle of a sentence don't |
There was a problem hiding this comment.
Important: this resolution call still silently falls back to the merge subject on a transient API failure, reintroducing the unrecoverable mis-bump the gh pr view retry below was specifically added to prevent. 2>/dev/null ... || true collapses two distinct outcomes: (a) the API succeeded and the commit genuinely has no PR (a real direct push, where falling back to the commit subject is correct), and (b) the API failed transiently, which also yields an empty PR_NUM and also falls back to the merge subject. On a blip, a merge-commit-merged feat!: PR resolves to "Merge pull request ..." (no conventional prefix) and ships as a patch; a squash without the PR body misses a body-only BREAKING CHANGE: footer. The repo enables merge, squash, and rebase, none of which guarantee the head subject equals the PR title, so this is reachable on the normal path - the hardening is one call too late.
Fix: capture the API exit status and fail loud on error, only falling back on a genuine empty-success (the || true is load-bearing for the head-closes-pipe-on-success case, so restructure rather than delete it):
if PR_PULLS=$(gh api "/repos/$REPO/commits/$TARGET_SHA/pulls" \
--jq '.[] | select(.merged_at != null) | .number'); then
PR_NUM=$(printf '%s\n' "$PR_PULLS" | head -1)
else
echo "::error::failed to resolve PR for $TARGET_SHA"; exit 1
fi| if [ "$BUMP" = "major" ]; then | ||
| echo "::warning::MAJOR bump $LATEST -> $NEXT: consumers pinned to v$MAJOR are NOT updated and must bump their 'uses:' refs manually." | ||
| fi | ||
|
|
There was a problem hiding this comment.
Important: on a reconcile re-run this rollback breadcrumb becomes a no-op-to-self. PREV_VN_SHA is read after a prior run may have already moved vN; on a re-run of the latest run (for example after a false-red read-back), it reads the already-moved value (== TARGET_SHA), so the emitted command here (and in the step summary) is gh api -X PATCH .../tags/vN -f sha=<TARGET_SHA> -F force=true - rolling vN to where it already is. The job is green and vN is correct, but the rollback command in the run an operator is most likely to open does nothing; at 3am they copy-paste it, get a 200, and believe a rollback happened. With the failure notification deferred, this run-log artifact is the recovery story, so its correctness matters.
Fix: when PREV_VN_SHA == TARGET_SHA, suppress the no-op command and point to the prior run's log, or carry the true previous SHA forward.
| } >> "$GITHUB_OUTPUT" | ||
| echo "::notice::$LATEST -> $NEXT ($BUMP) from $BUMP_SOURCE: \"$SAFE_SUBJECT\"" | ||
|
|
||
| - name: Publish release and move floating tag | ||
| # success() short-circuits if a prior step failed — without it, a | ||
| # custom `if:` re-enables the step even when the compute step errored. | ||
| if: success() && steps.t.outputs.skip != 'true' | ||
| env: |
There was a problem hiding this comment.
Important: the vN move is unconditional and can move vN BACKWARD on a re-run of a superseded run. GitHub re-runs replay the original github.sha. If run A (SHA_A) published v1.5.0 and moved v1 to SHA_A, and a later run B moved v1 to SHA_B, then re-running A (a natural operator action, especially if A had gone red on a transient read-back lag) takes the reconcile path: EXISTING_TAG=v1.5.0, skip the create, then this block forces v1 back to SHA_A; the read-back passes (v1 == SHA_A) and the job goes green, having silently regressed the floating tag to the older release for every consumer pinning @v1. The round-3 reconcile fix assumed re-runs are always for the latest release; the move is never bounded to forward-only.
Fix: before moving MAJOR_TAG, confirm NEXT_TAG/EXISTING_TAG is the highest vN.N.N tag on that line; if a higher release exists, ensure the release object but skip the vN move and log a notice (only ever move vN forward).
…il PR lookup, no-op rollback suppression solaris007's round-4 review against 8f05ec8 surfaced three Important items unaffected by my round-5 (f90bc3c) fixes: Important 1 — PR-number resolution silently fell back to merge subject on transient API failure. The line below (gh pr view) already fail-loud per round-4; this call inherited the original silent-fallback pattern. A feat!: PR during a 30s API blip would ship as patch. Split into fetch-then-parse so we distinguish API-failed (loud-fail) from API-succeeded-no-PR (legitimate direct push, safe fallback). Important 2 — Re-running a superseded run regressed vN. Walk: run A publishes v1.5.0 at SHA_A, moves v1->SHA_A. Run B publishes v1.5.1 at SHA_B, moves v1->SHA_B. Operator re-runs A (e.g. after a transient read-back lag). Reconcile path: EXISTING_TAG=v1.5.0, skip create, force PATCH v1 -> SHA_A. Read-back passes. Job green. v1 has SILENTLY regressed. New forward-only guard: before moving vN, check if a higher semver tag exists on the same major line; if so, log notice, skip the move, exit 0 (release object is preserved either way). Important 3 — Rollback breadcrumb showed roll-to-self on reconcile re-runs. PREV_VN_SHA capture happened after the move may have already landed, so a re-run sees PREV_VN_SHA == TARGET_SHA and emits a no-op 'gh api -X PATCH ... -f sha=<TARGET_SHA>' command. New NOOP_MOVE flag suppresses both the ::notice:: and the step-summary rollback block in that case. Minor — LATEST empty-guard was dead code. 'grep ... | head -1' on an empty repo causes pipefail+set-e to abort before the error message prints. Added '|| true' so the guard runs and surfaces the seeding hint. Matches the pattern already in use on lines 72/92. Also: tightened [skip release] to TRAILER-only anchoring. Previous regex (whitespace boundaries on both sides) still false-positived on prose like 'docs: explain how [skip release] works'. Now requires the marker to be on its own line (a git trailer) or the entire trimmed PR title. Matches semantic-release convention. Header comment corrected to reflect actual behavior. Verified yaml parses. File: 344 -> 405 lines. The growth is the forward-only guard (24 lines), the no-op detection (8 lines), and the fetch-then-parse split (5 lines). Logic remains a single linear flow.
| echo "::error::Tag $NEXT_TAG exists at $(sanitize "$TAG_SHA"), expected $TARGET_SHA. Drift detected." | ||
| exit 1 | ||
| fi | ||
| else |
There was a problem hiding this comment.
Important: When TAG_SHA is empty (transient API failure on the retry), this comparison fails and emits Drift detected - a misleading diagnosis that sends operators investigating phantom consistency problems instead of simply re-running.
Fix: check for empty before comparing:
if [ -z "$TAG_SHA" ]; then
echo "::error::Failed to read tag SHA for $NEXT_TAG after 3 attempts (transient API failure). Re-run should resolve."
exit 1
fi|
Hey @akshaymagapu, Strengths
Previously flagged issues now addressed: script injection (R1), bump-source unreachable under merge commits (R1), GITHUB_TOKEN anti-recursion (R2), invalid gh field (R3), re-run leaving vN stale (R3), fail-loud on API failure (R4), forward-only guard (R4), rollback no-op detection (R4). IssuesImportant (Should Fix)
When Why it matters: this is the re-run path - an operator re-running after a partial failure would hit this on a transient GitHub API blip and waste time investigating a non-existent consistency problem. Fix: check for empty if [ -z "$TAG_SHA" ]; then
echo "::error::Failed to read tag SHA for $NEXT_TAG after 3 attempts (transient API failure). Re-run should resolve."
exit 1
fi
if [ "$TAG_SHA" = "$TARGET_SHA" ]; thenMinor (Nice to Have)
The header comment uses the word "TRAILER" implying git-trailer semantics (last paragraph), but the grep matches
A PR titled Recommendations
AssessmentReady to merge? With fixes. Next Steps
|
When the release exists but the tag-SHA read fails transiently, TAG_SHA becomes empty and the equality check emitted a phantom "Drift detected" error, sending operators to investigate a non-existent consistency problem. Check for empty TAG_SHA first and fail as a transient error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Pushed if [ -z "$TAG_SHA" ]; then
echo "::error::Failed to read tag SHA for $NEXT_TAG after 3 attempts (transient API failure). Re-run should resolve."
exit 1
fi
if [ "$TAG_SHA" = "$TARGET_SHA" ]; thenThe minor nits (docs "TRAILER" wording, CRLF in last-paragraph extraction, capitalized near-miss types) and the recommendations (shared helper script, dry-run mode, |
There was a problem hiding this comment.
Hey @akshaymagapu,
Strengths
- The TAG_SHA empty-check fix (lines 280-283) is correct and well-structured. It separates transient-failure from drift-detection with a clear error message that tells the operator exactly what happened and what to do ("Re-run should resolve"). Good 3am ergonomics.
- All Critical/Important findings from rounds 1-5 are genuinely resolved. The progression from script injection to SHA-based PR resolution to inline vN move to forward-only guard to this transient-failure handling shows disciplined iteration.
- Security posture is sound: no inline
${{ }}in anyrun:block, SHA-pinned checkout withpersist-credentials: false, least-privilege permissions,sanitize()before all workflow command output, and PR resolution by merge_commit_sha preventing bump spoofing. - Forward-only monotonicity via
sort -Vguard (lines 297-321) protects the consumer fleet invariant that@vNnever moves backward. - The trigger disjunction between auto-release (push:main under GITHUB_TOKEN) and release.yaml (release:published from humans) is clean and eliminates double-write races without coordination locks.
- SHA-keyed idempotency (git tag --points-at) combined with read-back verification forms a solid recovery model for re-runs.
- NOOP detection prevents misleading rollback commands during reconcile re-runs.
- Step summary with rollback command, failure diagnostics, and
::warning::annotations provide good operational observability.
Previously flagged issues now addressed: script injection (R1), bump-source unreachable (R1), GITHUB_TOKEN anti-recursion (R2), invalid gh field (R3), re-run leaving vN stale (R3), API failure fallback (R4), forward-only guard (R4), rollback no-op detection (R4), TAG_SHA empty-check (R5).
Issues
Minor (Nice to Have)
2>/dev/nullon API calls suppresses diagnostic stderr, reducing operator visibility on failures (.github/workflows/auto-release.yaml:147, 273)
Multiple API calls (gh api "/repos/$REPO/commits/$TARGET_SHA/pulls" 2>/dev/null at line 147, TAG_SHA retry at line 273) discard stderr. When retries exhaust, the operator sees the ::error:: annotation but not the underlying cause (rate limit, auth expiry, network timeout, 5xx). Since these paths now explicitly handle the failure case with dedicated error messages, the 2>/dev/null no longer serves a purpose - the || echo "" or if ! already prevents set -e from aborting. Dropping 2>/dev/null would let the last attempt's diagnostic reach the log without changing control flow.
- Two divergent vN-move implementations will drift over time (.github/workflows/auto-release.yaml:340-354 vs release.yaml)
release.yaml still uses raw ${{ }} interpolation, lacks the forward-only guard, lacks the read-back, and lacks retry. The auto-release path has all of these. Not a merge blocker (release.yaml is untouched by this PR and the manual path is low-frequency), but a shared composite action (.github/actions/move-major-tag/action.yml) called by both would consolidate the logic. Worth tracking as a follow-up.
sanitize()relies ontrbeing character-level, not pattern-level (.github/workflows/auto-release.yaml:73)
Currently safe (tr replaces every individual colon), but a future refactor from tr to sed 's/:://g' would reintroduce workflow-command injection surface. A one-line comment noting that tr (character-level replacement) is intentional would prevent this maintenance-time regression.
Recommendations
- Add
--notes-start-tag "$LATEST"togh release create(line 296) so auto-generated release notes are bounded to commits since the previous release rather than relying on GitHub's heuristic. - The failure notification follow-up (Slack/webhook) remains the single highest-value remaining item from an operational standpoint - the step summary is only visible if someone opens the failed run.
- When standardizing this across other spacecat repos, publish as a reusable workflow (
on: workflow_call) or composite action consumed via@v2rather than copying the file. - Document the trust model for PR title-driven bump magnitude (a contributor with write access to main can force a major bump via
feat!:title - limited impact since the tag points at a human-merged SHA, but worth noting explicitly).
Assessment
Ready to merge? Yes.
Reasoning: The architecture is sound, the security posture is strong, all 8 prior Critical/Important findings across 5 rounds are genuinely addressed, and the latest fix (TAG_SHA empty-check) is correct and complete. The remaining items are minor hardening suggestions. After 5 rounds of substantive review and 11 commits, this workflow is in solid shape.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 2s | Cost: $3.26 | Commit: 0a3351d401cf5df3fc91cd0243aa7b572fdc585f
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Problem
There's no automation between "merge to main" and "consumers see the change":
Every meaningful change requires a maintainer to remember to publish a Release. PR #15 illustrates this — merged 2026-05-26, but
v2still points atv2.1.5because no Release was created.Solution
Add
.github/workflows/auto-release.yaml. On push to main, it computes the nextvN.N.Nfrom the latest existing tag plus the merge commit's conventional-commit subject, then publishes a GitHub Release withgh release create. The existingrelease.yamlhandles thevNfloating-tag move on therelease: publishedevent — unchanged.How it fixes
Version bump rules (from the merge commit's conventional-commit subject):
*!:/BREAKING CHANGEin bodyfeat:/feat(scope):Escape hatches:
[skip release]in the commit message → no release**/*.md,docs/**,LICENSE) → skipped viapaths-ignoreconcurrency: auto-release-main→ prevents two releases racing if PRs merge close togetherWhy pure bash /
gh(no Node, no semantic-release)mysticat-ciis yaml-only — no source code that would warrant apackage.json. Other spacecat repos use semantic-release because they're Node projects where the same tool also handles npm version bumps, CHANGELOG generation, and Lambda deploys. None of that applies here. A small bash workflow +ghCLI (already on every Actions runner) does the one thing that's actually needed — publish a Release — with zero new dependencies.Scope
.github/workflows/auto-release.yaml.release.yamluntouched.service-ci.yamluntouched.🤖 Generated with Claude Code