fix(release): survive an aborted release instead of leaving a half-bumped tree

[adnaan]

Same ordering bug as livetemplate#499 — found while releasing livetemplate v0.19.1, then confirmed present here.

The problem

The step order was update_versions → generate_changelog → build_and_test, so a test failure aborted with VERSION and CHANGELOG.md already rewritten:

 M CHANGELOG.md
 M VERSION

main() refuses to start on a dirty tree, so the retry is blocked until someone works out what to revert — a confusing trap at exactly the wrong moment, and the failure that triggers it is most likely a flake.

The fix

build_and_test runs first. Worth a note for this repo specifically, because unlike core, lvt does read the VERSION file from Go — getCurrentVersion() in commands/install_agent.go. It's safe regardless: no test exercises it, and it resolves to "unknown" under go test anyway since it reads a relative path from the package directory.

A trap covers the remaining window between writing the files and committing. It restores from HEAD, not the index:

commit_and_tag stages VERSION (and CHANGELOG.md) before committing, so on a failed commit they are already staged. A plain git checkout -- restores from the index — copying the modified versions back over themselves.

That was a real bug in the first draft of the core fix, caught by the abort test rather than by reading the diff.

Verification

In a throwaway clone with a forced post-bump failure:

• VERSION restored to 0.2.0, 0 dirty files, retry unblocked
• reorder visible in the log — Running Go tests… precedes Updating VERSION file

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG

[claude]

Review

Reordering build_and_test to run before the file mutations, plus a trap-based safety net for the remaining write→commit window, is a solid, well-scoped fix. The explanation in the PR body (and mirrored in code comments) about why each piece is needed is genuinely helpful — especially the note about commit_and_tag staging the files, which makes a plain git checkout -- restore from the index instead of HEAD. Verified that claim against commands/install_agent.go:391 (getCurrentVersion()) — it does fall back to "unknown" on a relative-path read failure, so reordering tests first is safe as described.

Correctness

• Edge case in restore_release_files (scripts/release.sh:100): git checkout HEAD -- VERSION CHANGELOG.md restores both paths in a single invocation. If either pathspec doesn't resolve in HEAD — e.g., CHANGELOG.md were ever untracked/never committed — git's checkout_paths reports the pathspec error and returns before writing anything to the worktree, so neither file gets restored, not just the missing one. In this repo CHANGELOG.md is already tracked (ls shows it committed on main), so this isn't reachable today, but it's a latent trap for a differently-bootstrapped repo or if CHANGELOG.md is ever removed from tracking. The existing log_warn fallback at least surfaces that restoration failed, so it's not silent — but the message doesn't explain why, which could send someone down the wrong debugging path. Might be worth restoring the two files with separate git checkout calls (or -- per-path with || true each) so one missing path doesn't block the other from being restored.
• The state machine (release_files_written / release_committed) correctly covers the cases I traced through: pre-write failure (no-op), write→commit-failure (restore), post-commit failure incl. push/goreleaser failure (guard stood down, commit preserved). Good attention to the "don't discard the release commit on a later network failure" case.
• Trap is registered at top-level script scope (line 104) rather than inside main, so it's live for the whole process including signal-based exits (Ctrl+C) — that's actually a plus, not an issue, since bash's EXIT trap fires on those too as long as nothing overrides it.

Style / minor nits

• The comment block for the trap machinery (scripts/release.sh:86-91) is sandwiched under the old # Update version files header, which now reads oddly — the header describes the section below it, but the paragraph immediately after describes the trap instead. Consider moving the trap setup above that header or giving it its own comment.
• 2>/dev/null on the restore's git checkout (line 100) swallows the actual git error before falling back to the generic "could not restore, do it by hand" warning. For an already-rare failure path, surfacing the real stderr would save whoever hits it a round of guessing.

Test coverage

No automated coverage (no existing bats/shell test harness for scripts/release.sh in the repo, consistent with prior state). The PR description documents manual verification in a throwaway clone with a forced post-bump failure, which is reasonable given this is a manually-invoked release script rather than CI-exercised code. Not a blocker, but if this script grows more failure-handling logic it'd be worth standing up a minimal bats suite at some point.

Security / performance

No concerns — this only touches local git plumbing in an interactive release script; no new external inputs or elevated privileges introduced.

Overall: correct fix for the bug it targets, with thoughtful comments explaining non-obvious reasoning. The one edge case above is worth a look but doesn't block given the current repo state.

[adnaan]

All three applied — the pathspec one was a real bug, thanks.

Applied: single-call git checkout HEAD -- a b restores neither file (b5ece6f). Verified rather than taken on faith, in a throwaway repo:

$ echo 9.9.9 > VERSION; echo x > CHANGELOG.md   # CHANGELOG untracked
$ git checkout HEAD -- VERSION CHANGELOG.md
error: pathspec 'CHANGELOG.md' did not match any file(s) known to git
$ cat VERSION
9.9.9      # ← VERSION not restored either

You were also right that it's reachable here specifically: commit_and_tag guards with [ -f CHANGELOG.md ] && git add, so this script already allows for a repo without one. Now restores one path per invocation; a path absent from HEAD gets a message saying this run created it and to delete it. Re-tested both scenarios — with CHANGELOG.md untracked, VERSION returns to 0.2.0 and only the created file remains.

Applied: 2>/dev/null swallowing git's error. Agreed, and it's the same shape as the bug above — a restore that reports failure without saying why. stderr is now visible.

Applied: comment block under the wrong header. Moved the trap machinery above # Update version files.

Acknowledged, no change: no automated coverage for release.sh. Correct, and I'd rather not stand up a bats suite inside a fix PR. Worth noting the manual verification did earn its keep — the pathspec bug you caught was present in my first draft and survived reading the diff; it only fell out of actually running the abort. If this script grows more failure handling, that's the argument for the suite.

[claude]

Reviewed the current diff (both commits). This is a solid, well-reasoned fix — the reasoning trail in the commit messages made it easy to verify.

Correctness

• Reordering build_and_test before update_versions/generate_changelog is the right primary fix: a test failure now happens before anything touches the tree, so release_files_written never gets set to true and the trap is a no-op — no dirty tree, no retry block.
• The restore_release_files trap correctly restores from HEAD rather than the index. This matters because commit_and_tag runs git add before git commit, so a failed commit leaves both files staged — a plain git checkout -- <path> would restore from the index and just copy the staged (modified) content back onto itself, silently no-oping. Good catch, and good that it was caught by testing rather than left latent.
• Restoring VERSION and CHANGELOG.md as two separate git checkout HEAD -- <path> calls (rather than one call with both paths) is correct: git checkout HEAD -- a b resolves the whole pathspec before touching the worktree and aborts entirely if any single path is missing from HEAD. Since commit_and_tag explicitly guards CHANGELOG.md with [ -f ] (i.e., a repo without a prior CHANGELOG.md is a supported case), a combined call could fail closed and restore neither file. This isn't reachable in this repo today (CHANGELOG.md is tracked at HEAD), but it's correct defensive handling for the general case the script already allows for.
• The release_committed guard correctly stands down the trap once the commit lands — it won't try to "restore" files that are now legitimately part of history, and it won't interfere with later failures in publish_github (push/goreleaser failures leave the local commit as-is, which is out of scope for this fix and pre-existing behavior).
• Letting git's stderr through (instead of swallowing it) on a failed checkout is a good call for debuggability at exactly the moment things have already gone wrong.

Minor/non-blocking observations

• When CHANGELOG.md doesn't exist at HEAD (freshly created by the aborted run), the trap logs "delete it by hand before retrying" rather than removing it itself. Since the flag state at that point (release_files_written=true, file absent from HEAD) unambiguously means "this run created it," an rm -f there would fully restore a clean tree automatically instead of leaving one manual step. Reasonable to leave as a conservative choice (auto-deleting a file is a slightly bigger hammer than restoring a tracked one), but worth a second look if the "retry unblocked" guarantee in the PR description is meant to hold unconditionally rather than just for the common case.
• No automated test accompanies this (understandable given the script is interactive and touches git push/goreleaser/GitHub release creation — hard to unit test meaningfully). The PR description documents manual verification in a throwaway clone for both the happy-path reorder and the untracked-CHANGELOG.md edge case, which is a reasonable substitute here.
• The comments are dense but earn their keep — each one explains a non-obvious "why" (e.g., index-vs-HEAD, single-vs-per-path checkout) rather than restating the code, consistent with keeping only comments that carry real information.

Security/performance: no concerns — this only touches local git plumbing in a release script gated behind interactive confirmation and existing set -euo pipefail/prerequisite checks.

Overall: the fix addresses a real, previously-hit pain point (per the linked livetemplate#499 precedent), and the trap logic holds up under the edge cases I traced through by hand.

[adnaan]

Applied the rm -f observation (56e3a59) — on reflection it wasn't a style preference, it was a gap between what the PR claimed and what the code did.

The description promised "retry unblocked," but git status --porcelain lists untracked and staged-new files alike, and main() refuses to start on any non-empty output. So leaving the created CHANGELOG.md behind meant the guarantee held for the common case and silently not for the edge one.

On safety — removing it is provable here, not a judgement call:

1. main() refuses to start on a dirty tree.
2. git status --porcelain reports untracked files (?? path).
3. Therefore a CHANGELOG.md absent from HEAD at trap time cannot have pre-existed as an untracked file — the script would never have started. This run created it.

So removing it restores the exact pre-run state rather than destroying anything the operator had.

One wrinkle your suggestion didn't mention and my scenario-2 test surfaced: the file may already be staged, since commit_and_tag runs git add before the commit that failed. A bare rm -f leaves an A entry, which still counts as dirty — so the guarantee would still not hold. It now does git rm -f --ignore-unmatch followed by rm -f, covering staged and unstaged alike.

Re-verified both branches of the condition in throwaway clones:

CHANGELOG.md at HEAD	Result after aborted run
untracked	removed, 0 dirty files
tracked	restored (not deleted), 0 dirty files

Same change applied to livetemplate#499, which shares this function.

[claude]

Review

Well-scoped fix with unusually thorough comments explaining the why. I read through scripts/release.sh in full (not just the diff) to check the trap logic against every call site. Summary below.

What it does

• Reorders main() so build_and_test runs before update_versions/generate_changelog/commit_and_tag, so a test/build failure no longer leaves VERSION/CHANGELOG.md half-bumped.
• Adds an EXIT trap (restore_release_files) that restores VERSION/CHANGELOG.md from HEAD if the script aborts after those files are written but before the release commit lands, guarded by release_files_written/release_committed flags.

Correctness

• Traced every path through restore_release_files against the state machine (pre-write, post-write/pre-commit, post-commit) and it holds up:
  • No-op before update_versions runs and after commit_and_tag's git commit succeeds (flag flips right after the commit, before git tag, so a later tag failure correctly leaves the commit alone rather than trying to "restore" over it).
  • Restoring from HEAD instead of the index correctly sidesteps the staged-file trap the PR description calls out.
  • The one-path-per-git checkout loop is justified — git checkout HEAD -- a b does bail entirely if either path is missing from HEAD, so batching would silently skip both files in the "repo has no CHANGELOG.md yet" case.
  • The "absent from HEAD but present on disk ⇒ this run created it" inference is sound given main()'s upfront clean-tree check — worth knowing that if that check is ever loosened, this inference silently breaks.
  • Every command inside the trap that could fail is guarded (||, || true, or used as an if/elif condition), so nothing inside the trap can trip set -e and abandon the loop partway through the two files.
• Verified the core safety claim independently rather than taking the PR body at its word: getCurrentVersion() in commands/install_agent.go:391-394 does os.ReadFile("VERSION") (relative path), no test in commands/ references it, and .goreleaser.yml sources {{.Version}} from the git tag, not the VERSION file — so reordering build_and_test ahead of the bump doesn't change what gets built or released. Reasoning checks out.

Test coverage

• This PR itself adds no automated test — verification was a manual throwaway-clone run described in the PR body. Given the trap logic already caught one real bug in its own first draft (the staged-index issue mentioned in the description), and shell trap/state-machine logic is exactly the kind of thing that regresses silently on a future edit, a small script-level test (even a simple one under Dockerfile.test/CI that forces a post-bump failure and asserts git status --porcelain is empty afterward) would be worth having here rather than relying on the next release attempt to notice a regression.

Style / minor nit

• The new globals (release_files_written, release_committed) and the trap registration sit between bump_version and update_versions, i.e. mid-way through the function definitions rather than grouped with the other top-of-file state or immediately before main. Not wrong, just a bit of an odd read on first pass — a brief top-of-file comment pointing at this block, or relocating it next to main, would make the control flow easier to follow for the next person skimming the file top-to-bottom.

Security

No concerns — no new external input handling, no injection surface introduced (all variables used in git/rm calls are script-internal, quoted).

Overall: solid, well-reasoned fix for a real operational trap. The main gap is the missing regression test given how easy this class of bug is to reintroduce silently.

[adnaan]

Thanks — and noting this as the convergence point: this round raised no new functional issue, it independently re-derived the reasoning (including checking .goreleaser.yml sources {{.Version}} from the git tag rather than the VERSION file, which is the part I'd most want a second pair of eyes on). Per the project's review-loop guidance I'll stop pushing here and answer the remaining point by reply instead.

On the coupling you flagged — "if main()'s clean-tree check is ever loosened, this inference silently breaks" — that's the sharpest observation in the round, and you're right that it's a hidden dependency rather than a local property.

The code comment currently states the premise ("main() refuses to start on a dirty tree and git status --porcelain lists untracked files, so it cannot have pre-existed") but doesn't warn that the premise is load-bearing elsewhere. Someone relaxing the clean-tree gate would have no reason to look in the trap.

Leaving it as-is deliberately rather than pushing a fourth revision for a comment, since a push costs another full CI + review cycle. Worth folding in whenever this function is next touched — and if you'd rather have it now, say so and I'll add it.

For the record, the three rounds each caught something real, all of which survived reading the diff and only fell out of running the abort:

1. restore-from-index instead of HEAD — trap logged success, restored nothing
2. combined pathspec — one missing file meant neither got restored
3. created CHANGELOG.md left staged — "retry unblocked" held for the common case only

That's a decent argument for the bats suite you mentioned, if this grows further.