fix(release): refuse to bump on top of an unpublished release

[adnaan]

Same gap as livetemplate#500, fixed there in livetemplate#501. Raised while reviewing that one — this repo shares the structure.

If a release got as far as commit_and_tag and then failed to publish — a push error, a goreleaser failure — the tree held a release commit and tags the remote had never seen, and nothing said so. The next run read the already-bumped VERSION as current and offered to bump on top of it, skipping a version in the published sequence.

main() now detects that state and refuses:

✗ v0.2.1 is committed locally but has not reached origin/main

  git push origin main
  git push origin components/v0.2.1
  git push origin v0.2.1

Then check whether the GitHub release and binaries exist:

  gh release view v0.2.1

Two lvt-specific differences from the core fix

Nested module tags. This repo tags components/vX.Y.Z alongside the main tag, and publish_github pushes both. Finishing an interrupted release therefore means pushing both, so the guidance enumerates whatever tags the version actually created (git tag -l "v$pending" "*/v$pending") rather than assuming a fixed set — it adapts if more nested modules appear.

goreleaser, not gh release create. The GitHub release and binaries come from goreleaser, which isn't a one-liner to hand-run, so the guidance points at gh release view to check and then at publish_github rather than pretending there's a simple command.

Design notes (same reasoning as livetemplate#501)

Detect before the pull — git pull --rebase would move the release commit out from under its tags, leaving them on an orphaned commit.

Detect-and-refuse, not resume — publish_github isn't idempotent, so an automatic resume would break at one of the likeliest ways to reach this state.

Fails toward proceeding — this runs on every release while the state is rare, so a false positive (blocking a healthy release) is worse than a false negative. A failed fetch returns "not detected" rather than testing ancestry against a stale ref: right after a successful release, the HEAD subject and VERSION are identical to the unpublished case, and that ancestry test is the only discriminator.

Verification

Against a bare origin so nothing escaped:

State	Expected	Result
Healthy tree	proceeds to version prompt	✅
Unpublished release	refuses, VERSION unchanged	✅
— lists both tags	v0.2.1 + components/v0.2.1	✅
Following the printed commands	refusal clears	✅
Broken remote	no false positive	✅
--dry-run on unpublished	refuses	✅

The fourth row is the one I most wanted to confirm — the guidance isn't just plausible, following it verbatim actually resolves the state.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG

[claude]

Review

Solid, well-scoped port of the core-repo fix (#501) to this repo's release script. The design rationale in the PR description (detect-before-pull, detect-and-refuse rather than auto-resume, fail-toward-proceeding) is sound and the implementation matches it faithfully. I read through check_unpublished_release and the main() integration in scripts/release.sh; here's the detail.

Correctness

• The ancestry check is the right approach and each failure mode correctly falls back to "not detected" (missing VERSION path aside, which was pre-existing behavior): git log -1 mismatch, failed git fetch, is-ancestor true, and missing remote ref all return 1 cleanly.
• set -e interaction is handled correctly - every fallible command inside check_unpublished_release is either the condition of an if or paired with || return 1, so nothing here will abort the script unexpectedly.
• The commit-message signature (chore(release): v$version) matches exactly what commit_and_tag writes as the first line (git log --pretty=%s only reads the subject), so the detection path is correctly wired to the actual commit format.
• The tag-enumeration pattern git tag -l "v$pending" "*/v$pending" correctly matches both the main tag and nested components/vX.Y.Z tags created in commit_and_tag/pushed in publish_github - verified against the actual tag-creation code, the naming schemes line up.
• Placement before the git pull --rebase call is correct - pulling first really would risk rebasing the release commit off its tags, as the comment says.

Minor nit

• The pre-existing Get current version one-line comment (scripts/release.sh:47) now sits directly above the newly-inserted check_unpublished_release function instead of get_current_version, which it no longer describes. Worth moving that comment down to keep it attached to the function it labels, since get_current_version now has no header comment of its own.

Edge case worth a thought (not blocking)

• If tag creation somehow didn't happen for a given release (e.g., commit_and_tag committed but the tag step didn't persist for some out-of-band reason), the printed guidance would only include git push origin $branch with no tag-push lines, since the for loop over git tag -l would be empty. Given commit_and_tag creates the commit and tags in the same function with no gap for partial failure under set -e, this is unlikely in practice - just flagging as the one place the guidance could theoretically go silent about a missing step.

Test coverage

• No automated tests exist for release.sh (before or after this change), consistent with the rest of the file. The PR description documents thorough manual verification against a bare origin (healthy tree, unpublished release, tag listing, following the printed remediation verbatim, broken remote, --dry-run), which is a reasonable substitute given the script's nature (it shells out to git/gh/goreleaser against real remotes). If this repo ever grows a bats/shell-test harness, this function is a good, self-contained candidate to unit-test - it takes a branch name and only touches git state.

Security

• No concerns. No new external input handling, no injection surface - all interpolated values (branch, pending) come from git symbolic-ref/the local VERSION file, not attacker-controlled input, and are used in contexts (git subcommand args, echoed strings) where that's safe.

Overall: a careful, well-reasoned change that mirrors an already-vetted fix. Nothing here blocks merging; the comment-placement nit is cosmetic.

[adnaan]

Both applied (4c2e7b9) — and the second was more than the edge case it looked like.

Applied: comment placement. You're right, # Get current version ended up heading check_unpublished_release. Moved back. (The core PR doesn't have this because I happened to include the comment in that edit's replacement text — same mistake, avoided by luck rather than care.)

Applied: empty tag list. You flagged this as theoretical — "no gap for partial failure under set -e" — but there is a gap, and I put it there:

242:    release_committed=true
244:    log_step "Creating git tag v$new_version"
245:    git tag -a "v$new_version" ...

The flag is set immediately after the commit and before tagging, deliberately (livetemplate#499): so that a tag failure doesn't cause the abort trap to discard the release commit. That same ordering makes "commit exists, tags don't" reachable — a duplicate tag, a bad -m, a signing failure.

In that state the old guidance printed only git push origin main and said nothing about the tag, which is the failure mode I'd just written into the core PR description: guidance that reads as complete while omitting the step that actually failed is worse than no guidance, because nobody double-checks it.

Now:

  # No v0.2.1 tag exists locally — the tag step never completed.
  git tag -a v0.2.1 -m "Release v0.2.1"
  # ...plus one per nested Go module, e.g. components/v0.2.1

Verified both branches — tags present lists v0.2.1 and components/v0.2.1; tags deleted prints the recreate guidance.

Acknowledged: no automated tests. Agreed, and your point that check_unpublished_release is a good bats candidate is well taken — it takes a branch name and only touches git state, so it's the one genuinely unit-testable piece here. Not standing up a harness inside a fix PR, but it's the natural first test if one appears.

[claude]

Review: refuse to bump on top of an unpublished release

Overview
This mirrors the fix landed in livetemplate/livetemplate#501: main() now detects the case where a previous run committed and tagged a release but died before publish_github finished pushing it, and refuses to continue (rather than silently bumping on top of the unpublished version). The detection (check_unpublished_release) is gated on the HEAD commit subject matching the release-commit message and, only then, checks ancestry of HEAD against a freshly-fetched origin/$branch. The design choices are sound: checking before the git pull --rebase, refusing instead of auto-resuming (since publish_github isn't idempotent), and failing toward not blocking when the check itself can't be completed (fetch failure).

Code quality

• The comments explaining why (not what) are excellent and match the existing style in this file (e.g. restore_release_files) — they call out the non-obvious invariant (HEAD subject + VERSION look identical right after both a successful and an unpublished release, so ancestry is the only discriminator) rather than restating the code.
• Logic is well-contained in a single new function with an early-return style that's easy to follow.
• Nice touch enumerating actual tags via git tag -l instead of hardcoding v$pending + components/v$pending, so it keeps working if another nested Go module is added later.

Potential bug: incomplete guidance when tags are missing/partial
This is the part worth fixing before merge, since giving correct recovery instructions is the entire point of this PR.

1. In the "no tags found" branch (release commit exists, but the tag step never ran), the printed guidance only shows how to create the tags (git tag -a v$pending ... plus a comment about nested modules). It never prints the corresponding git push origin v$pending (and nested-tag) commands. A user following this block literally will recreate the tags locally and stop — still leaving the release unpublished. Compare with the "tags found" branch, which does print git push origin $t for each tag it finds.

2. More importantly, the branch is chosen purely on [ -n "$tags" ] — i.e. "zero tags" vs "at least one tag". commit_and_tag creates the main tag first and then loops over nested go.mods creating one tag per module, so a failure partway through that loop (plausible for exactly the multi-nested-module future this PR says it wants to "adapt" to) leaves a partial tag set: main tag present, some/all nested tags missing. In that case tags is non-empty, so the code takes the "push what's there" branch and never tells the user a nested tag is missing at all — the recreate-and-warn path only triggers when zero tags exist. Silently omitting a tag here reproduces exactly the "skips a version in the published sequence" failure mode this PR is trying to eliminate, just one level down (a missing nested-module tag rather than a missing top-level release).

Suggestion: compute the expected tag set the same way commit_and_tag/publish_github do (via the find . -name go.mod ... loop), diff it against what git tag -l actually finds, and for each expected tag print either a push line or a create+push line depending on whether it exists. That would also let this collapse the now-duplicated go.mod-discovery snippet (already duplicated between commit_and_tag and publish_github) down to one shared helper.

Performance
Negligible impact. The one added network call (git fetch --quiet origin "$branch") only runs when HEAD's commit subject already matches the release-commit pattern, which in normal usage is only true immediately after a prior release — not on every invocation.

Security
No concerns. $branch comes from git symbolic-ref --short HEAD (a trusted local value), no user-controlled input is interpolated into shell commands, and the added fetch is read-only.

Test coverage
No automated tests are added, but there's no existing test harness for scripts/release.sh in this repo either, so that's consistent with current convention. The PR description's manual verification table (tested against a bare origin) is a reasonable substitute given the script's nature — though it looks like it covered the "tags found" recovery path and not the "tag step never completed" / partial-tag path flagged above, which would be worth adding a row for.

Summary
Solid, well-reasoned fix for a real gap, with thoughtful design tradeoffs (fail-open on fetch failure, checking before pull, refuse-not-resume). The main thing worth addressing is the incomplete/misleading recovery guidance when the tag-creation step partially or fully failed — right now it can leave a user believing they've fully recovered when a tag was never created or pushed.

[adnaan]

Both real, both fixed (0203fc4). The second one is the better catch and I'd underweighted the same risk in my own comment.

1. The no-tags branch never printed the pushes. Correct — following it literally recreated tags locally and stopped, leaving the release exactly as unpublished as before. Straightforwardly wrong.

2. Partial tag sets took the wrong branch. This is the one worth dwelling on. My condition was [ -n "$tags" ] — zero-vs-some — but commit_and_tag creates the main tag then loops over nested modules, so a mid-loop failure leaves main present and a nested one missing. Non-empty, so it took the push-what's-there path and never mentioned the gap.

That's the exact failure this check exists to prevent, one level down: silently incomplete output that reads as authoritative. I'd written that sentence into the PR description and then shipped an instance of it.

Fixed by inverting the question — instead of listing what exists, derive what the release should have (main tag plus one per nested go.mod, mirroring commit_and_tag) and report each independently:

### PARTIAL — nested tag deleted ###
  git push origin main
  git push origin v0.2.1
  git tag -a components/v0.2.1 -m "Release components/v0.2.1"   # missing — tag step did not complete
  git push origin components/v0.2.1

Verified all three states plus end-to-end:

Tag state	Output
all present	pushes only
nested missing	main pushed, nested recreated + pushed
all missing	both recreated + pushed

And following the all-missing output verbatim publishes the release and clears the refusal — which is the property I actually care about, since guidance that's merely plausible is what produced both of these bugs.

Core (#501) has neither issue: it has no nested modules, and its missing-tag branch already printed the push. Left unchanged.

[claude]

Review

Solid fix, and the commit history shows the design already went through a couple of real rounds of self-correction (the partial-tag-set bug and the missing-push-command bug are exactly the kind of thing that's easy to miss on a first pass). The comments throughout do a good job explaining why rather than what, which fits the surrounding file's style.

Code quality

• Duplicated find . -name "go.mod" ... pipeline — this exact nested-module-discovery pipeline now appears three times: commit_and_tag (line 256), publish_github (line 358), and the new check_unpublished_release/main recovery block (line 469). Given that this PR's own revision history was driven by exactly this kind of derived-state mismatch (the expected-tag-set bug), it'd be worth extracting a small list_nested_module_tags() helper that returns the subdir/vX.Y.Z list, so all three call sites can never drift from each other again.
• Minor nit: local t is declared twice in the main() recovery block — once in local pending t and again a few lines later alongside local expected=(...) subdir gomod ... local t. Harmless, but the first declaration of t is dead until the second one shadows it.

Bugs

• Didn't find a correctness bug in the new logic itself. The ancestry check (git merge-base --is-ancestor HEAD refs/remotes/origin/$branch), the "fetch failed → not detected" fallback, and the missing-ref fallback all look right, and the "fails toward proceeding" philosophy is consistently applied.
• One thing worth double-checking in practice (not blocking): check_unpublished_release calls get_current_version, which on a missing VERSION file does log_error ...; exit 1 — but since that call happens inside $(...), the exit 1 only ends the subshell, and log_error's echo -e output would actually be captured as the "version" string. This is a pre-existing quirk in get_current_version (also present at the existing call site in main), not something this PR introduces, and it degrades safely here (the commit-message comparison just won't match, so the function correctly falls through to "not detected"). Flagging only because the new call site inherits it.

Performance

• check_unpublished_release does its own git fetch --quiet origin "$branch", and then (when the check passes) main does a second fetch implicitly via git pull --rebase origin "$branch". That's one extra network round-trip per release run. Given release.sh is an infrequent, interactive script, this is a totally reasonable trade for correctness — just noting it as the cost of the safety check.

Security

No concerns. All the new logic operates on internally-derived values (git plumbing output, the VERSION file, go.mod paths under the repo) rather than untrusted input, and there's no new eval/interpolated shell execution.

Test coverage

There's no automated test harness for scripts/release.sh (consistent with the rest of the file — it wasn't tested before this PR either). The PR description documents thorough manual verification against a bare origin across all the relevant states (healthy tree, unpublished release, partial tags, broken remote, --dry-run), which is a reasonable bar for a script like this, but it's not repeatable in CI. If this script gets touched again, it might be worth a lightweight bats/shell test suite that fakes a bare remote — the manual test matrix in the PR description would translate almost directly into test cases.

Overall: nice defensive addition, ports the core fix well, and the two follow-up commits show good responsiveness to review feedback already. The only actionable suggestion is deduplicating the nested-module find pipeline.

[adnaan]

Converged — this round found no correctness bug, and independently re-verified the ancestry logic and both fallbacks. Stopping pushes here per the project's review-loop guidance; answering the remaining points by reply.

Deferred, tracked as #341: the triplicated find . -name "go.mod" pipeline. Your argument is the strongest version of this suggestion I could make myself — this PR's own history is the evidence, since both bugs came from derived state disagreeing with real state, and the third copy exists specifically to mirror commit_and_tag. "Mirrors" is exactly the coupling that breaks quietly.

Deferring on blast radius, not disagreement. The recovery block is read-only — it prints guidance. commit_and_tag and publish_github are the code that actually creates and pushes tags during a real release. Getting the extraction wrong there could make a release skip a nested-module tag, which is a worse outcome than the guidance drift it prevents. That asymmetry says land this green and do the refactor deliberately, re-testing all three call sites against a bare origin.

Deferred to the same issue: the dead local t. You're right, local pending t declares it and local t re-declares it a few lines down. I made the one-word fix, then reverted it — both PRs are green, and restarting a full CI cycle plus another review round for a harmless dead declaration is exactly the loop the guidance warns about. It folds into #341.

Acknowledged, no change: get_current_version inside $(...). Good spot, and correctly identified as pre-existing — its exit 1 only kills the subshell and log_error's output would be captured as the version string. Worth noting it degrades safely at this call site specifically: a garbage version can't match chore(release): v$version, so the function falls through to "not detected", which is the direction this check is designed to fail in. Not fixing it here since it predates the PR and the new call site is the safe one.

Acknowledged, no change: the extra fetch. Correct that it's one additional round-trip per run. For an interactive script invoked a few times a month, that's a fair price for the check being right — and it can't be avoided without reusing a possibly-stale ref, which is the failure mode the fetch exists to prevent.