fix: bound release pull request bodies

Author: pedronauck

internal/orchestrator/constants.go

@@ -72,12 +72,13 @@ const (
 	// ReleaseBodyOutputFile is the generated markdown consumed by GitHub Releases.
 	ReleaseBodyOutputFile = "RELEASE_BODY.md"
 	// ReleaseNotesGitKeepPath is the placeholder file that keeps `.release-notes/` in git.
-	ReleaseNotesGitKeepPath = ".release-notes/.gitkeep"
-	changelogFilePath       = "CHANGELOG.md"
-	packageManifestPath     = "package.json"
-	packageLockFilePath     = "package-lock.json"
-	releasePRLabelPending   = "release-pending"
-	releasePRLabelAutomated = "automated"
-	sagaStepSkipKey         = "skip"
-	releaseArtifactBun      = "bun"
+	ReleaseNotesGitKeepPath   = ".release-notes/.gitkeep"
+	changelogFilePath         = "CHANGELOG.md"
+	packageManifestPath       = "package.json"
+	packageLockFilePath       = "package-lock.json"
+	releasePRLabelPending     = "release-pending"
+	releasePRLabelAutomated   = "automated"
+	createPullRequestStepName = "Create Pull Request"
+	sagaStepSkipKey           = "skip"
+	releaseArtifactBun        = "bun"
 )

internal/orchestrator/pr_release.go

@@ -13,7 +13,6 @@ import (
 	"github.com/compozy/releasepr/internal/repository"
 	"github.com/compozy/releasepr/internal/service"
 	"github.com/compozy/releasepr/internal/usecase"
-	"github.com/sethvargo/go-retry"
 	"github.com/spf13/afero"
 	"go.uber.org/zap"
 	"golang.org/x/sync/errgroup"
@@ -459,14 +458,9 @@ func (o *PRReleaseOrchestrator) createPullRequest(
 	}
 	title := releasePullRequestTitle(version)
 	labels := []string{releasePRLabelPending, releasePRLabelAutomated}
-	// Create/Update PR with retry for network failures
-	return retry.Do(
-		ctx,
-		retry.WithMaxRetries(DefaultRetryCount, retry.NewExponential(DefaultRetryDelay)),
-		func(ctx context.Context) error {
-			return o.githubRepo.CreateOrUpdatePR(ctx, branchName, "main", title, body, labels)
-		},
-	)
+	return retryOperation(ctx, func(retryCtx context.Context) error {
+		return o.githubRepo.CreateOrUpdatePR(retryCtx, branchName, "main", title, body, labels)
+	})
 }
 
 func releasePullRequestTitle(version string) string {
@@ -940,7 +934,7 @@ func (o *PRReleaseOrchestrator) addCreatePRStep(
 	wctx *workflowContext,
 ) {
 	saga.AddStep(SagaStep{
-		Name: "Create Pull Request",
+		Name: createPullRequestStepName,
 		Type: domain.OperationTypeCreatePR,
 		Execute: func(ctx context.Context) (map[string]any, error) {
 			if wctx.version == "" || cfg.SkipPR || cfg.DryRun {
@@ -972,13 +966,7 @@ func (o *PRReleaseOrchestrator) addCreatePRStep(
 				zap.String("title", title),
 				zap.Strings("labels", labels),
 			)
-			err = retry.Do(
-				ctx,
-				retry.WithMaxRetries(DefaultRetryCount, retry.NewExponential(DefaultRetryDelay)),
-				func(ctx context.Context) error {
-					return o.githubRepo.CreateOrUpdatePR(ctx, wctx.branchName, "main", title, body, labels)
-				},
-			)
+			err = o.githubRepo.CreateOrUpdatePR(ctx, wctx.branchName, "main", title, body, labels)
 			if err != nil {
 				o.logger(ctx).Error("Failed to create or update PR", zap.Error(err))
 				return nil, fmt.Errorf("failed to create or update PR from %s to main: %w", wctx.branchName, err)

internal/orchestrator/pr_release_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/compozy/releasepr/internal/config"
 	"github.com/compozy/releasepr/internal/domain"
 	"github.com/compozy/releasepr/internal/logger"
+	"github.com/compozy/releasepr/internal/usecase"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -208,6 +209,96 @@ func TestPRReleaseOrchestrator_releaseArtifactCommands(t *testing.T) {
 		require.NoError(t, existsErr)
 		assert.False(t, exists)
 	})
+	t.Run("Should restore archived release notes from newly produced rollback data", func(t *testing.T) {
+		ctx := testReleaseContext(t)
+		fsRepo := afero.NewMemMapFs()
+		activePath := ".release-notes/a.md"
+		archivedPath := ".release-notes/archive/v1.2.3/a.md"
+		require.NoError(t, fsRepo.MkdirAll(".release-notes/archive/v1.2.3", 0755))
+		require.NoError(t, afero.WriteFile(fsRepo, archivedPath, []byte("note"), 0644))
+		require.NoError(t, afero.WriteFile(fsRepo, ReleaseNotesGitKeepPath, nil, 0644))
+		gitRepo := new(mockGitExtendedRepository)
+		gitRepo.On("MoveFile", mock.Anything, archivedPath, activePath).
+			Run(func(mock.Arguments) {
+				require.NoError(t, fsRepo.Rename(archivedPath, activePath))
+			}).
+			Return(nil).
+			Once()
+		compensator := NewCompensatingActions(gitRepo, new(mockGithubExtendedRepository), fsRepo)
+		rollbackData := usecase.ArchiveReleaseNotesResult{
+			Moves: []usecase.ArchivedReleaseNoteMove{
+				{
+					From: activePath,
+					To:   archivedPath,
+				},
+			},
+			GitKeepCreated: true,
+		}.ToRollbackData()
+
+		err := compensator.RestoreArchivedReleaseNotes(ctx, rollbackData)
+
+		require.NoError(t, err)
+		activeExists, activeErr := afero.Exists(fsRepo, activePath)
+		require.NoError(t, activeErr)
+		assert.True(t, activeExists)
+		archivedExists, archivedErr := afero.Exists(fsRepo, archivedPath)
+		require.NoError(t, archivedErr)
+		assert.False(t, archivedExists)
+		gitKeepExists, gitKeepErr := afero.Exists(fsRepo, ReleaseNotesGitKeepPath)
+		require.NoError(t, gitKeepErr)
+		assert.False(t, gitKeepExists)
+		gitRepo.AssertExpectations(t)
+	})
+}
+
+func TestPRReleaseOrchestrator_createPullRequest(t *testing.T) {
+	t.Run("Should stop after one permanent GitHub client error", func(t *testing.T) {
+		t.Parallel()
+		githubRepo := new(mockGithubExtendedRepository)
+		githubRepo.On(
+			"CreateOrUpdatePR",
+			mock.Anything,
+			"release/v1.0.0",
+			"main",
+			"build: release v1.0.0",
+			mock.Anything,
+			mock.Anything,
+		).Return(githubStatusError(422)).Once()
+		orchestrator := &PRReleaseOrchestrator{githubRepo: githubRepo}
+
+		err := orchestrator.createPullRequest(t.Context(), "v1.0.0", "changelog", "", "release/v1.0.0")
+
+		require.Error(t, err)
+		githubRepo.AssertExpectations(t)
+	})
+	t.Run("Should retry a transient GitHub server error", func(t *testing.T) {
+		t.Parallel()
+		githubRepo := new(mockGithubExtendedRepository)
+		githubRepo.On(
+			"CreateOrUpdatePR",
+			mock.Anything,
+			"release/v1.0.0",
+			"main",
+			"build: release v1.0.0",
+			mock.Anything,
+			mock.Anything,
+		).Return(githubStatusError(502)).Once()
+		githubRepo.On(
+			"CreateOrUpdatePR",
+			mock.Anything,
+			"release/v1.0.0",
+			"main",
+			"build: release v1.0.0",
+			mock.Anything,
+			mock.Anything,
+		).Return(nil).Once()
+		orchestrator := &PRReleaseOrchestrator{githubRepo: githubRepo}
+
+		err := orchestrator.createPullRequest(t.Context(), "v1.0.0", "changelog", "", "release/v1.0.0")
+
+		require.NoError(t, err)
+		githubRepo.AssertExpectations(t)
+	})
 }
 
 func TestPRReleaseOrchestrator_ExecuteReleaseArtifacts(t *testing.T) {
@@ -671,10 +762,8 @@ func TestPRReleaseOrchestrator_Execute(t *testing.T) {
 		gitRepo.On("Commit", mock.Anything, mock.Anything).Return(nil).Once()
 		gitRepo.On("PushBranch", mock.Anything, branchName).Return(nil).Once()
 
-		// Fail on PR creation (use mock.Anything for context)
-		// Note: The retry might not be happening for non-retryable errors
 		githubRepo.On("CreateOrUpdatePR", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-			Return(errors.New("GitHub API error")).
+			Return(githubStatusError(422)).
 			Once()
 
 		orch := NewPRReleaseOrchestrator(gitRepo, githubRepo, fsRepo, cliffSvc, npmSvc)
@@ -1086,8 +1175,10 @@ func TestPRReleaseOrchestrator_RollbackOnFailure(t *testing.T) {
 			// For file status checks during rollback
 		gitRepo.On("ListLocalBranches", mock.Anything).
 			Return([]string{"main", branchName}, nil).
+			Once()
+		gitRepo.On("ListLocalBranches", mock.Anything).
+			Return([]string{"main"}, nil).
 			Maybe()
-			// Check if branch exists
 		gitRepo.On("RemoteBranchExists", mock.Anything, branchName).
 			Return(true, nil).
 			Maybe()

internal/orchestrator/retry_policy.go

@@ -0,0 +1,49 @@
+package orchestrator
+
+import (
+	"context"
+	"errors"
+	"net/http"
+
+	"github.com/google/go-github/v74/github"
+	"github.com/sethvargo/go-retry"
+)
+
+func retryOperation(ctx context.Context, operation func(context.Context) error) error {
+	strategy := retry.WithMaxRetries(DefaultRetryCount, retry.NewExponential(DefaultRetryDelay))
+	return retry.Do(ctx, strategy, func(retryCtx context.Context) error {
+		err := operation(retryCtx)
+		if err == nil {
+			return nil
+		}
+		return classifyRetryError(err)
+	})
+}
+
+func classifyRetryError(err error) error {
+	if isPermanentRetryError(err) {
+		return err
+	}
+	return retry.RetryableError(err)
+}
+
+func isPermanentRetryError(err error) bool {
+	if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+		return true
+	}
+	var rateLimitError *github.RateLimitError
+	if errors.As(err, &rateLimitError) {
+		return false
+	}
+	var abuseRateLimitError *github.AbuseRateLimitError
+	if errors.As(err, &abuseRateLimitError) {
+		return false
+	}
+	var responseError *github.ErrorResponse
+	if !errors.As(err, &responseError) || responseError.Response == nil {
+		return false
+	}
+	statusCode := responseError.Response.StatusCode
+	isClientError := statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError
+	return isClientError && statusCode != http.StatusRequestTimeout && statusCode != http.StatusTooManyRequests
+}

internal/orchestrator/saga_executor.go

@@ -8,7 +8,6 @@ import (
 	"github.com/compozy/releasepr/internal/logger"
 	"github.com/compozy/releasepr/internal/repository"
 	"github.com/google/uuid"
-	"github.com/sethvargo/go-retry"
 	"go.uber.org/zap"
 )
 
@@ -115,8 +114,7 @@ func (s *SagaExecutor) executeStep(ctx context.Context, step SagaStep) error {
 		}
 	}
 	var rollbackData map[string]any
-	retryStrategy := retry.WithMaxRetries(DefaultRetryCount, retry.NewExponential(DefaultRetryDelay))
-	err := retry.Do(ctx, retryStrategy, func(retryCtx context.Context) error {
+	err := retryOperation(ctx, func(retryCtx context.Context) error {
 		// Check if context is canceled before executing
 		select {
 		case <-retryCtx.Done():
@@ -125,7 +123,7 @@ func (s *SagaExecutor) executeStep(ctx context.Context, step SagaStep) error {
 		}
 		data, execErr := step.Execute(retryCtx)
 		if execErr != nil {
-			return retry.RetryableError(execErr)
+			return execErr
 		}
 		rollbackData = data
 		return nil
@@ -190,18 +188,14 @@ func (s *SagaExecutor) rollback(ctx context.Context) error {
 
 // executeCompensation executes a compensating action with retry
 func (s *SagaExecutor) executeCompensation(ctx context.Context, step *SagaStep, rollbackData map[string]any) error {
-	retryStrategy := retry.WithMaxRetries(DefaultRetryCount, retry.NewExponential(DefaultRetryDelay))
-	return retry.Do(ctx, retryStrategy, func(retryCtx context.Context) error {
+	return retryOperation(ctx, func(retryCtx context.Context) error {
 		// Check if context is canceled
 		select {
 		case <-retryCtx.Done():
 			return retryCtx.Err()
 		default:
 		}
-		if err := step.Compensate(retryCtx, rollbackData); err != nil {
-			return retry.RetryableError(err)
-		}
-		return nil
+		return step.Compensate(retryCtx, rollbackData)
 	})
 }