From 37e4034c665df3dbd7fad6d9327b705ed7e57b46 Mon Sep 17 00:00:00 2001 From: Will Roden Date: Mon, 1 Jun 2026 12:08:57 -0500 Subject: [PATCH 1/2] Push release target before publishing to avoid tag-name reservation on Immutable Releases (#195) When a repository has Immutable Releases enabled, publishing a release acquires a row in release_immutable_tags that permanently reserves the tag name on the repo. Previously, createRelease published the release *before* pushTarget, so a rejected push to a protected branch caused the deferred DeleteRelease cleanup to orphan that row, making the tag name unrecoverable without GitHub Support. Reorder createRelease so pushTarget happens between uploadAssets and PublishRelease. This matches GitHub's documented Immutable Releases workflow (create draft -> upload assets -> publish) and keeps the release as a draft (no immutable row) through the only step that plausibly fails for non-retriable reasons. Make addErrCleanup return a cancel function and use it to disable both the DeleteRelease and tag-delete cleanups immediately before PublishRelease. After publish has been attempted, neither cleanup is safe: the server may have processed the publish even when the client saw an error, and deleting the release or tag in that ambiguous state permanently reserves the tag name. Behavior changes: - pushTarget failure now cleans up cleanly (draft release + tag deleted). - PublishRelease failure now leaves the release, tag, and target push in place for manual recovery. Previously the cleanup would delete the release and tag; that auto-cleanup is exactly what causes the immutable-row orphan, so it had to go. Fixes #195. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 19 +++++++- release.go | 51 ++++++++++++++++---- release_test.go | 124 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index c79054b5..92cef146 100644 --- a/README.md +++ b/README.md @@ -116,8 +116,23 @@ release. Some options such as `--check-pr` will modify this behavior. yet. 6. **Upload release assets**. Any files written to `$ASSETS_DIR` will be uploaded as release assets. -7. **Publish the release**. -8. **Emit output** including release version, tag, change level, etc. +7. **Push the release target** (e.g. a bake commit produced by the pre-tag + hook) to its branch on the remote. Pushing the target before publishing + ensures that on a repository with + [Immutable Releases](https://docs.github.com/en/code-security/concepts/supply-chain-security/immutable-releases) + enabled, a rejected push to a protected branch does not permanently reserve + the tag name. +8. **Publish the release**. +9. **Emit output** including release version, tag, change level, etc. + +If the push in step 7 fails, the draft release and the pushed tag are cleaned +up so the run can be retried. If step 8 (publish) fails, the draft release, +the tag, and the pushed target are left in place — once publish has been +attempted there is no way to tell whether the server processed it, and +deleting the release or tag at that point would permanently reserve the tag +name on Immutable Releases repositories. Recover by publishing the draft +release manually, or by deleting the draft release and tag and reverting the +target branch by hand. ## Recipes diff --git a/release.go b/release.go index e5b2c662..e0ff54d3 100644 --- a/release.go +++ b/release.go @@ -65,8 +65,23 @@ func (o *Runner) cleanupAfterErr() error { return err } -func (o *Runner) addErrCleanup(fn func() error) { - o.errCleanups = append(o.errCleanups, fn) +// addErrCleanup registers fn to run during cleanupAfterErr. The returned +// cancel function deactivates the cleanup so that it becomes a no-op. Callers +// must invoke cancel once the action that fn would undo has reached a point +// where rolling it back is no longer safe or correct — for example, after +// PublishRelease has been attempted, because the server may have processed +// the publish even if the client saw an error, and on a repository with +// Immutable Releases enabled, deleting either the release or its tag in that +// state permanently reserves the tag name. +func (o *Runner) addErrCleanup(fn func() error) (cancel func()) { + active := true + o.errCleanups = append(o.errCleanups, func() error { + if !active { + return nil + } + return fn() + }) + return func() { active = false } } type Result struct { @@ -327,7 +342,7 @@ func (o *Runner) run(ctx context.Context) (_ *Result, errOut error) { if err != nil { return nil, err } - o.addErrCleanup(func() error { + cancelTagDelete := o.addErrCleanup(func() error { _, e := o.runCmd(ctx, nil, "git", "push", o.PushRemote, "--delete", result.ReleaseTag) return e }) @@ -335,7 +350,7 @@ func (o *Runner) run(ctx context.Context) (_ *Result, errOut error) { result.CreatedTag = true if o.CreateRelease { - err = o.createRelease(ctx, result) + err = o.createRelease(ctx, result, cancelTagDelete) if err != nil { return nil, err } @@ -356,7 +371,17 @@ func (o *Runner) rejectShallowCheckout(ctx context.Context) error { } // createRelease handles the creation of a GitHub release, including notes, assets, and publishing. -func (o *Runner) createRelease(ctx context.Context, result *Result) error { +// +// To remain compatible with repositories that have Immutable Releases enabled +// (https://docs.github.com/en/code-security/concepts/supply-chain-security/immutable-releases), +// the release is created as a draft, all assets are uploaded, the release +// target is pushed, and only then is the release published. This matches the +// workflow GitHub recommends for immutable releases: create draft → upload +// assets → publish. Once PublishRelease has been attempted both destructive +// cleanups (DeleteRelease and the tag delete) are cancelled, because the +// server may have processed the publish even if the client saw an error, and +// deleting either side in that state permanently reserves the tag name. +func (o *Runner) createRelease(ctx context.Context, result *Result, cancelTagDelete func()) error { releaseNotes, err := o.getReleaseNotes(ctx, result) if err != nil { return err @@ -368,7 +393,7 @@ func (o *Runner) createRelease(ctx context.Context, result *Result) error { return err } - o.addErrCleanup(func() error { + cancelDeleteRelease := o.addErrCleanup(func() error { return o.GithubClient.DeleteRelease(ctx, o.repoOwner(), o.repoName(), rel.ID) }) @@ -382,13 +407,21 @@ func (o *Runner) createRelease(ctx context.Context, result *Result) error { return nil } - err = o.GithubClient.PublishRelease(ctx, o.repoOwner(), o.repoName(), o.MakeLatest, rel.ID) + // Push the target before publishing so the release stays a draft (and no + // immutable-tag row exists) until the most failure-prone step — the push + // to a possibly-protected branch — has succeeded. + err = o.pushTarget(ctx) if err != nil { return err } - // push target last because it cannot be easily rolled back - return o.pushTarget(ctx) + // Publish boundary: from here on, neither the release nor the tag may be + // deleted by cleanup. A PublishRelease error does not guarantee the + // server did not process the publish. + cancelDeleteRelease() + cancelTagDelete() + + return o.GithubClient.PublishRelease(ctx, o.repoOwner(), o.repoName(), o.MakeLatest, rel.ID) } func (o *Runner) uploadAssets(ctx context.Context, uploadURL string) error { diff --git a/release_test.go b/release_test.go index 6061ad51..3f268cc2 100644 --- a/release_test.go +++ b/release_test.go @@ -764,4 +764,128 @@ echo bar > "$ASSETS_DIR/bar.txt" ChangeLevel: changeLevelMinor, }, got) }) + + t.Run("pushTarget failure cleans up draft release and tag", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + repos := setupGit(t) + githubClient := mocks.NewMockGithubClient(gomock.NewController(t)) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", "v2.0.0", repos.taggedCommits["head"], -1).Return( + &github.CommitComparison{ + AheadBy: 1, + Commits: []string{repos.taggedCommits["head"]}, + }, nil, + ) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", mergeSha, repos.taggedCommits["head"], 0).Return( + &github.CommitComparison{AheadBy: 0}, nil, + ) + githubClient.EXPECT().ListMergedPullsForCommit(gomock.Any(), "orgName", "repoName", repos.taggedCommits["head"]).Return( + []github.BasePull{{Number: 2, MergeCommitSha: mergeSha, Labels: []string{labelBreaking}}}, nil, + ) + githubClient.EXPECT().GenerateReleaseNotes(gomock.Any(), "orgName", "repoName", "v3.0.0", "v2.0.0").Return( + "release notes", nil, + ) + githubClient.EXPECT().CreateRelease(gomock.Any(), "orgName", "repoName", "v3.0.0", "release notes", false).Return( + &github.RepoRelease{ + ID: 1, + UploadURL: "localhost", + }, nil, + ) + // The release is still a draft when pushTarget fails, so DeleteRelease + // cleanup runs safely. No PublishRelease expectation: the publish must + // not be attempted when pushTarget fails. + githubClient.EXPECT().DeleteRelease(gomock.Any(), "orgName", "repoName", int64(1)).Return(nil) + preHook := ` +#!/bin/sh +set -e +git config user.name 'tester' +git config user.email 'tester' +current_branch=$(git rev-parse --abbrev-ref HEAD) +echo "bake" > bake.txt +git add bake.txt > /dev/null +git commit -m "bake commit" > /dev/null +echo "$current_branch" > "$RELEASE_TARGET" +` + runner := &Runner{ + CheckoutDir: repos.clone, + Ref: repos.taggedCommits["head"], + TagPrefix: "v", + Repo: "orgName/repoName", + PushRemote: "origin", + GithubClient: githubClient, + CreateRelease: true, + PreTagHook: preHook, + TempDir: t.TempDir(), + } + _, err := runner.run(ctx) + // Push to a checked-out branch of a non-bare origin is rejected by + // the default receive.denyCurrentBranch=refuse setting. + require.ErrorContains(t, err, "remote rejected") + ok, err := localTagExists(ctx, repos.origin, "v3.0.0") + require.NoError(t, err) + require.False(t, ok, "tag must be cleaned up after pushTarget failure") + }) + + t.Run("PublishRelease failure does not delete release or tag", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + repos := setupGit(t) + // Allow pushes to origin's currently checked-out branch so pushTarget + // succeeds and we reach PublishRelease. + mustRunCmd(t, repos.origin, "git", "config", "receive.denyCurrentBranch", "ignore") + githubClient := mocks.NewMockGithubClient(gomock.NewController(t)) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", "v2.0.0", repos.taggedCommits["head"], -1).Return( + &github.CommitComparison{ + AheadBy: 1, + Commits: []string{repos.taggedCommits["head"]}, + }, nil, + ) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", mergeSha, repos.taggedCommits["head"], 0).Return( + &github.CommitComparison{AheadBy: 0}, nil, + ) + githubClient.EXPECT().ListMergedPullsForCommit(gomock.Any(), "orgName", "repoName", repos.taggedCommits["head"]).Return( + []github.BasePull{{Number: 2, MergeCommitSha: mergeSha, Labels: []string{labelBreaking}}}, nil, + ) + githubClient.EXPECT().GenerateReleaseNotes(gomock.Any(), "orgName", "repoName", "v3.0.0", "v2.0.0").Return( + "release notes", nil, + ) + githubClient.EXPECT().CreateRelease(gomock.Any(), "orgName", "repoName", "v3.0.0", "release notes", false).Return( + &github.RepoRelease{ + ID: 1, + UploadURL: "localhost", + }, nil, + ) + githubClient.EXPECT().PublishRelease(gomock.Any(), "orgName", "repoName", "", int64(1)).Return(errors.New("publish failed")) + // No DeleteRelease expectation: once PublishRelease has been attempted + // the cleanup must not delete the release. The mock controller fails + // the test if DeleteRelease is invoked. + preHook := ` +#!/bin/sh +set -e +git config user.name 'tester' +git config user.email 'tester' +current_branch=$(git rev-parse --abbrev-ref HEAD) +echo "bake" > bake.txt +git add bake.txt > /dev/null +git commit -m "bake commit" > /dev/null +echo "$current_branch" > "$RELEASE_TARGET" +` + runner := &Runner{ + CheckoutDir: repos.clone, + Ref: repos.taggedCommits["head"], + TagPrefix: "v", + Repo: "orgName/repoName", + PushRemote: "origin", + GithubClient: githubClient, + CreateRelease: true, + PreTagHook: preHook, + TempDir: t.TempDir(), + } + _, err := runner.run(ctx) + require.ErrorContains(t, err, "publish failed") + ok, err := localTagExists(ctx, repos.origin, "v3.0.0") + require.NoError(t, err) + require.True(t, ok, "tag must not be deleted once PublishRelease has been attempted") + }) + } From c011a5c2caf2b48063f6af52a9b224543d750e49 Mon Sep 17 00:00:00 2001 From: Will Roden Date: Mon, 1 Jun 2026 12:37:05 -0500 Subject: [PATCH 2/2] Fix gofumpt formatting in release_test.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- release_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/release_test.go b/release_test.go index 3f268cc2..7ede9ae1 100644 --- a/release_test.go +++ b/release_test.go @@ -887,5 +887,4 @@ echo "$current_branch" > "$RELEASE_TARGET" require.NoError(t, err) require.True(t, ok, "tag must not be deleted once PublishRelease has been attempted") }) - }