From e459141384e692901ad62e55b3a191879e2f3f80 Mon Sep 17 00:00:00 2001 From: Thomas Vilte Date: Fri, 31 Jul 2026 15:29:23 -0300 Subject: [PATCH 1/2] fix(git): provide specific error for GitHub ruleset rejections --- internal/errors/errors.go | 3 + internal/git/git_service.go | 51 ++++++++++++- internal/git/git_service_test.go | 120 +++++++++++++++++++++++++++++++ internal/ui/ui.go | 6 ++ internal/ui/ui_test.go | 40 +++++++++++ 5 files changed, 218 insertions(+), 2 deletions(-) diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 68aeaf9..834bed9 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -132,6 +132,9 @@ var ( ErrPush = NewAppError(TypeGit, "Failed to push to remote", nil). WithSuggestion("Verify remote is configured: git remote -v") + ErrPushRejectedByRuleset = NewAppError(TypeGit, "Push rejected by a GitHub branch/tag ruleset", nil). + WithSuggestion("This ref is protected — push your changes through a pull request instead, or ask a repo admin to adjust the ruleset in Settings > Rules") + ErrFetchTags = NewAppError(TypeGit, "Failed to fetch tags from remote", nil). WithSuggestion("Check your network connection and remote access") diff --git a/internal/git/git_service.go b/internal/git/git_service.go index 18689f1..9162699 100644 --- a/internal/git/git_service.go +++ b/internal/git/git_service.go @@ -617,22 +617,69 @@ func (s *GitService) CreateTag(ctx context.Context, version, message string) err } func (s *GitService) PushTag(ctx context.Context, version string) error { + log := logger.FromContext(ctx) + cmd := exec.CommandContext(ctx, "git", "push", "origin", version) + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { - return errors.ErrPushTag.WithError(err).WithContext("version", version) + stderrStr := strings.TrimSpace(stderr.String()) + log.Error("git push tag failed", "error", err, "version", version, "stderr", stderrStr) + + if isRulesetRejection(stderrStr) { + return errors.ErrPushRejectedByRuleset.WithError(err).WithContext("version", version).WithContext("stderr", stderrStr) + } + return errors.ErrPushTag.WithError(err).WithContext("version", version).WithContext("stderr", stderrStr) } return nil } // Push pushes commits to the remote repository func (s *GitService) Push(ctx context.Context) error { + log := logger.FromContext(ctx) + cmd := exec.CommandContext(ctx, "git", "push") + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { - return errors.ErrPush.WithError(err) + stderrStr := strings.TrimSpace(stderr.String()) + log.Error("git push failed", "error", err, "stderr", stderrStr) + + if isRulesetRejection(stderrStr) { + return errors.ErrPushRejectedByRuleset.WithError(err).WithContext("stderr", stderrStr) + } + return errors.ErrPush.WithError(err).WithContext("stderr", stderrStr) } return nil } +// isRulesetRejection reports whether git's push failure output indicates the +// push was rejected by a GitHub ruleset or legacy branch/tag protection rule +// (as opposed to a network/auth/diverged-history failure). GH013 is GitHub's +// error code for the newer Rulesets feature; GH006 is the legacy branch +// protection equivalent — both include a human-readable reason afterward, +// which we surface via the raw stderr already attached to the AppError. +func isRulesetRejection(stderr string) bool { + markers := []string{ + "GH013", + "GH006", + "protected branch", + "protected tag", + "protected ref", + "repository rule violations", + "push declined due to repository rule violations", + } + lower := strings.ToLower(stderr) + for _, m := range markers { + if strings.Contains(lower, strings.ToLower(m)) { + return true + } + } + return false +} + func (s *GitService) GetCommitCount(ctx context.Context) (int, error) { cmd := exec.CommandContext(ctx, "git", "rev-list", "--count", "HEAD") output, err := cmd.Output() diff --git a/internal/git/git_service_test.go b/internal/git/git_service_test.go index a059ccd..1cc7a72 100644 --- a/internal/git/git_service_test.go +++ b/internal/git/git_service_test.go @@ -1182,6 +1182,102 @@ func TestGitService_Push(t *testing.T) { assert.Equal(t, domainErrors.TypeGit, appErr.Type) assert.Contains(t, err.Error(), "Failed to push to remote") }) + + t.Run("Push rejected by a GitHub ruleset returns a ruleset-specific error", func(t *testing.T) { + tempDir := setupTestRepo(t) + defer cleanupTestRepo(t, tempDir) + + installFakeGitRejectingPush(t, rulesetsRejectionStderr) + + service := NewGitService() + err := service.Push(context.Background()) + assert.Error(t, err) + + var appErr *domainErrors.AppError + assert.True(t, errors.As(err, &appErr)) + assert.Equal(t, "Push rejected by a GitHub branch/tag ruleset", appErr.Message, + "a ruleset rejection must surface as a distinct, actionable error — not the generic push failure") + assert.Contains(t, appErr.Suggestion, "pull request") + assert.Contains(t, err.Error(), "GH013") + }) + + t.Run("Push rejected by legacy branch protection also returns the ruleset-specific error", func(t *testing.T) { + tempDir := setupTestRepo(t) + defer cleanupTestRepo(t, tempDir) + + installFakeGitRejectingPush(t, legacyProtectedBranchStderr) + + service := NewGitService() + err := service.Push(context.Background()) + assert.Error(t, err) + + var appErr *domainErrors.AppError + assert.True(t, errors.As(err, &appErr)) + assert.Equal(t, "Push rejected by a GitHub branch/tag ruleset", appErr.Message) + }) +} + +func TestGitService_PushTag_RulesetRejection(t *testing.T) { + tempDir := setupTestRepo(t) + defer cleanupTestRepo(t, tempDir) + + installFakeGitRejectingPush(t, rulesetsRejectionStderr) + + service := NewGitService() + err := service.PushTag(context.Background(), "v1.2.3") + assert.Error(t, err) + + var appErr *domainErrors.AppError + assert.True(t, errors.As(err, &appErr)) + assert.Equal(t, "Push rejected by a GitHub branch/tag ruleset", appErr.Message) + assert.Equal(t, "v1.2.3", appErr.Context["version"]) +} + +const rulesetsRejectionStderr = `remote: error: GH013: Repository rule violations found for refs/heads/main. +remote: +remote: - Changes must be made through a pull request. +remote: +To github.com:owner/repo.git + ! [remote rejected] main -> main (push declined due to repository rule violations) +error: failed to push some refs to 'github.com:owner/repo.git' +` + +const legacyProtectedBranchStderr = `remote: error: GH006: Protected branch update failed for refs/heads/main. +remote: error: At least 1 approving review is required by reviewers with write access. +To github.com:owner/repo.git + ! [remote rejected] main -> main (protected branch hook declined) +error: failed to push some refs to 'github.com:owner/repo.git' +` + +// installFakeGitRejectingPush puts a fake "git" executable first on PATH +// that transparently delegates every subcommand to the real git, except +// "push", which fails immediately with the given stderr — simulating a +// GitHub ruleset/branch-protection rejection without a real remote. +func installFakeGitRejectingPush(t *testing.T, stderr string) { + t.Helper() + + realGit, err := exec.LookPath("git") + if err != nil { + t.Fatalf("could not locate real git binary: %v", err) + } + + dir := t.TempDir() + script := fmt.Sprintf(`#!/bin/sh +if [ "$1" = "push" ]; then + cat <<'EOF' >&2 +%s +EOF + exit 1 +fi +exec %q "$@" +`, stderr, realGit) + + fakeGit := filepath.Join(dir, "git") + if err := os.WriteFile(fakeGit, []byte(script), 0755); err != nil { + t.Fatalf("failed to write fake git script: %v", err) + } + + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) } func TestGitService_GetCommitsBetweenTags(t *testing.T) { @@ -1279,3 +1375,27 @@ func TestGitService_Fallback(t *testing.T) { } }) } + +func TestIsRulesetRejection(t *testing.T) { + cases := []struct { + name string + stderr string + want bool + }{ + {"GH013 ruleset rejection", rulesetsRejectionStderr, true}, + {"GH006 legacy branch protection", legacyProtectedBranchStderr, true}, + {"lowercase 'protected branch' phrase", "remote: this branch is a protected branch", true}, + {"protected tag phrase", "remote: error: cannot push to a protected tag", true}, + {"repository rule violations phrase without a code", "push declined due to repository rule violations", true}, + {"empty stderr", "", false}, + {"network failure", "ssh: connect to host github.com port 22: Connection refused", false}, + {"auth failure", "remote: Support for password authentication was removed", false}, + {"diverged history (non-fast-forward)", "! [rejected] main -> main (fetch first)\nerror: failed to push some refs", false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isRulesetRejection(tc.stderr)) + }) + } +} diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 47f4e6c..2083da7 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -194,6 +194,12 @@ func HandleAppError(err error, translations ...*i18n.Translations) error { _, _ = dimColor.Printf(" Details: %v\n", appErr.Err) } + if stderr, ok := appErr.Context["stderr"].(string); ok && stderr != "" { + for _, line := range strings.Split(stderr, "\n") { + _, _ = dimColor.Printf(" %s\n", line) + } + } + if appErr.Suggestion != "" { fmt.Println() tryPrefix := "💡 Try: " diff --git a/internal/ui/ui_test.go b/internal/ui/ui_test.go index c356053..6c6e777 100644 --- a/internal/ui/ui_test.go +++ b/internal/ui/ui_test.go @@ -3,12 +3,39 @@ package ui import ( "bytes" "errors" + "io" + "os" "testing" + "github.com/fatih/color" "github.com/stretchr/testify/assert" domainErrors "github.com/thomas-vilte/matecommit/internal/errors" ) +// captureStdout redirects os.Stdout (and fatih/color's cached Output, +// which is bound at package init and otherwise ignores test-time +// redirection) for the duration of fn, returning everything written. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + + oldStdout := os.Stdout + oldColorOutput := color.Output + r, w, err := os.Pipe() + assert.NoError(t, err) + os.Stdout = w + color.Output = w + + fn() + + assert.NoError(t, w.Close()) + os.Stdout = oldStdout + color.Output = oldColorOutput + + out, err := io.ReadAll(r) + assert.NoError(t, err) + return string(out) +} + func TestShownAndIsShown(t *testing.T) { t.Run("nil error stays nil", func(t *testing.T) { assert.Nil(t, Shown(nil)) @@ -57,6 +84,19 @@ func TestHandleAppError_ReturnsShownError(t *testing.T) { assert.Equal(t, "something git-related broke", appErr.Message) }) + t.Run("stderr context is printed to the user, not just captured", func(t *testing.T) { + original := domainErrors.NewAppError(domainErrors.TypeGit, "Push rejected by a GitHub branch/tag ruleset", nil). + WithContext("stderr", "remote: - Changes must be made through a pull request."). + WithSuggestion("Push your changes through a pull request instead") + + output := captureStdout(t, func() { + _ = HandleAppError(original) + }) + + assert.Contains(t, output, "Changes must be made through a pull request", + "the captured git/GitHub rejection reason must reach the user, not just live in Context") + }) + t.Run("plain (non-AppError) error is marked shown too", func(t *testing.T) { result := HandleAppError(errors.New("generic failure")) From e69d18d7bab77a93737bf0cdb94436b03dfe9611 Mon Sep 17 00:00:00 2001 From: Thomas Vilte Date: Fri, 31 Jul 2026 15:58:46 -0300 Subject: [PATCH 2/2] feat(release): open PR when direct push is rejected by ruleset --- internal/commands/release/create.go | 14 +++- internal/commands/release/create_test.go | 2 +- internal/commands/release/mocks_test.go | 4 +- internal/commands/release/release.go | 5 +- internal/errors/errors.go | 27 ++++++ internal/errors/errors_test.go | 22 +++++ internal/git/git_service.go | 58 +++++++++++++ internal/models/pr.go | 6 ++ internal/services/release_changelog.go | 82 ++++++++++++++++++- internal/services/release_service.go | 3 + .../release_service_improvements_test.go | 5 ++ internal/services/release_service_test.go | 77 ++++++++++++++++- internal/testutil/mockgit.go | 15 ++++ internal/testutil/mockvcs.go | 8 ++ internal/vcs/github/client.go | 1 + internal/vcs/github/client_pr.go | 30 +++++++ internal/vcs/github/mocks_test.go | 5 ++ internal/vcs/interfaces.go | 2 + 18 files changed, 356 insertions(+), 10 deletions(-) diff --git a/internal/commands/release/create.go b/internal/commands/release/create.go index e72886b..890587d 100644 --- a/internal/commands/release/create.go +++ b/internal/commands/release/create.go @@ -3,6 +3,7 @@ package release import ( "bufio" "context" + "errors" "fmt" "os" "strings" @@ -10,6 +11,7 @@ import ( "github.com/thomas-vilte/matecommit/internal/commands/completion_helper" cfg "github.com/thomas-vilte/matecommit/internal/config" + domainErrors "github.com/thomas-vilte/matecommit/internal/errors" "github.com/thomas-vilte/matecommit/internal/i18n" "github.com/thomas-vilte/matecommit/internal/logger" "github.com/thomas-vilte/matecommit/internal/models" @@ -141,7 +143,17 @@ func createReleaseAction(releaseSvc releaseService, trans *i18n.Translations, re sPush := ui.NewSmartSpinner(trans.GetMessage("release.pushing_changes", 0, nil)) sPush.Start() - if err := releaseSvc.PushChanges(ctx); err != nil { + if err := releaseSvc.PushChanges(ctx, release.Version); err != nil { + sPush.Stop() + if errors.Is(err, domainErrors.ErrReleasePROpened) { + // Not a failure: PushChanges couldn't push directly (the + // branch is ruleset-protected) but opened a PR instead. + // The release isn't finished — it needs a merge and a + // re-run — so this still surfaces as a non-zero exit, + // but ui.HandleAppError presents it as "action needed" + // (with the PR URL) rather than a generic push failure. + return ui.HandleAppError(err, trans) + } sPush.Error(trans.GetMessage("release.error_pushing_changes", 0, struct{ Error string }{err.Error()})) return fmt.Errorf("error pushing changes: %w", err) } diff --git a/internal/commands/release/create_test.go b/internal/commands/release/create_test.go index c97da90..536faed 100644 --- a/internal/commands/release/create_test.go +++ b/internal/commands/release/create_test.go @@ -269,7 +269,7 @@ func TestCreateCommand_WithChangelog(t *testing.T) { mockService.On("UpdateLocalChangelog", mock.Anything, release, notes).Return(nil) mockService.On("UpdateAppVersion", mock.Anything, "v1.0.0").Return(nil) mockService.On("CommitChangelog", mock.Anything, "v1.0.0").Return(nil) - mockService.On("PushChanges", mock.Anything).Return(nil) + mockService.On("PushChanges", mock.Anything, mock.Anything).Return(nil) mockService.On("CreateTag", mock.Anything, "v1.0.0", mock.Anything).Return(nil) diff --git a/internal/commands/release/mocks_test.go b/internal/commands/release/mocks_test.go index 4096c45..83ebcd5 100644 --- a/internal/commands/release/mocks_test.go +++ b/internal/commands/release/mocks_test.go @@ -75,8 +75,8 @@ func (m *MockReleaseService) CommitChangelog(ctx context.Context, version string return args.Error(0) } -func (m *MockReleaseService) PushChanges(ctx context.Context) error { - args := m.Called(ctx) +func (m *MockReleaseService) PushChanges(ctx context.Context, version string) error { + args := m.Called(ctx, version) return args.Error(0) } diff --git a/internal/commands/release/release.go b/internal/commands/release/release.go index f70bbf4..6eff0ac 100644 --- a/internal/commands/release/release.go +++ b/internal/commands/release/release.go @@ -30,7 +30,7 @@ type releaseService interface { EnrichReleaseContext(ctx context.Context, release *models.Release) error UpdateLocalChangelog(ctx context.Context, release *models.Release, notes *models.ReleaseNotes) error CommitChangelog(ctx context.Context, version string) error - PushChanges(ctx context.Context) error + PushChanges(ctx context.Context, version string) error UpdateAppVersion(ctx context.Context, version string) error ValidateMainBranch(ctx context.Context) error BuildChangelogPreview(ctx context.Context, release *models.Release, notes *models.ReleaseNotes) string @@ -57,6 +57,9 @@ type gitService interface { ValidateGitConfig(ctx context.Context) error ValidateTagExists(ctx context.Context, tag string) error GetRepoRoot(ctx context.Context) (string, error) + CreateAndSwitchBranch(ctx context.Context, branchName string) error + SwitchBranch(ctx context.Context, branchName string) error + PushBranch(ctx context.Context, branchName string) error } type ReleaseCommandFactory struct { diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 834bed9..05445f2 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -44,6 +44,18 @@ func (e *AppError) Unwrap() error { return e.Err } +// Is lets errors.Is match against the package-level sentinel AppErrors +// (e.g. ErrPushRejectedByRuleset) by Type+Message, even though +// WithError/WithContext/WithSuggestion return a new *AppError instance +// each time rather than the same pointer. +func (e *AppError) Is(target error) bool { + t, ok := target.(*AppError) + if !ok { + return false + } + return e.Type == t.Type && e.Message == t.Message +} + // WithError creates a new AppError with an underlying error func (e *AppError) WithError(err error) *AppError { return &AppError{ @@ -135,6 +147,21 @@ var ( ErrPushRejectedByRuleset = NewAppError(TypeGit, "Push rejected by a GitHub branch/tag ruleset", nil). WithSuggestion("This ref is protected — push your changes through a pull request instead, or ask a repo admin to adjust the ruleset in Settings > Rules") + // ErrReleasePROpened is not really a failure — it signals that PushChanges + // couldn't push directly (ruleset) but successfully opened a pull request + // as a fallback. Callers should check for it with errors.Is and present it + // as "action needed", not as a generic error. + ErrReleasePROpened = NewAppError(TypeGit, "Opened a pull request instead of pushing directly", nil) + + ErrCreateBranch = NewAppError(TypeGit, "Failed to create branch", nil). + WithSuggestion("Make sure the branch name is valid and doesn't already exist locally") + + ErrSwitchBranch = NewAppError(TypeGit, "Failed to switch branch", nil). + WithSuggestion("Make sure the branch exists: git branch -a") + + ErrPushBranch = NewAppError(TypeGit, "Failed to push branch", nil). + WithSuggestion("Check your remote connection: git remote -v") + ErrFetchTags = NewAppError(TypeGit, "Failed to fetch tags from remote", nil). WithSuggestion("Check your network connection and remote access") diff --git a/internal/errors/errors_test.go b/internal/errors/errors_test.go index 16e3622..fa8ac9b 100644 --- a/internal/errors/errors_test.go +++ b/internal/errors/errors_test.go @@ -126,6 +126,28 @@ func TestAppError_ChainedContext(t *testing.T) { } } +func TestAppError_Is(t *testing.T) { + t.Run("errors.Is matches through WithError/WithContext despite returning new instances", func(t *testing.T) { + wrapped := ErrPushRejectedByRuleset.WithError(errors.New("exit status 1")).WithContext("stderr", "GH013") + + if !errors.Is(wrapped, ErrPushRejectedByRuleset) { + t.Error("expected errors.Is to match the sentinel by Type+Message despite pointer inequality") + } + }) + + t.Run("different sentinel AppErrors of the same Type do not match", func(t *testing.T) { + if errors.Is(ErrPush, ErrPushRejectedByRuleset) { + t.Error("ErrPush and ErrPushRejectedByRuleset share TypeGit but have different Messages — must not match") + } + }) + + t.Run("a plain non-AppError never matches", func(t *testing.T) { + if errors.Is(errors.New("boom"), ErrPushRejectedByRuleset) { + t.Error("a plain error must never match an AppError sentinel") + } + }) +} + func contains(s, substr string) bool { return len(s) >= len(substr) && (s == substr || len(substr) == 0 || (len(s) > 0 && (s[:len(substr)] == substr || contains(s[1:], substr)))) diff --git a/internal/git/git_service.go b/internal/git/git_service.go index 9162699..eaf53aa 100644 --- a/internal/git/git_service.go +++ b/internal/git/git_service.go @@ -655,6 +655,64 @@ func (s *GitService) Push(ctx context.Context) error { return nil } +// CreateAndSwitchBranch creates a new local branch at the current HEAD and +// switches to it. Uses -B (not -b) so it's safe to call again with the same +// name (e.g. a retried release) — it just resets the branch to the current +// HEAD instead of failing because it already exists. +func (s *GitService) CreateAndSwitchBranch(ctx context.Context, branchName string) error { + log := logger.FromContext(ctx) + + cmd := exec.CommandContext(ctx, "git", "checkout", "-B", branchName) + var stderr strings.Builder + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + stderrStr := strings.TrimSpace(stderr.String()) + log.Error("git checkout -B failed", "error", err, "branch", branchName, "stderr", stderrStr) + return errors.ErrCreateBranch.WithError(err).WithContext("branch", branchName).WithContext("stderr", stderrStr) + } + return nil +} + +// SwitchBranch checks out an existing local branch. +func (s *GitService) SwitchBranch(ctx context.Context, branchName string) error { + log := logger.FromContext(ctx) + + cmd := exec.CommandContext(ctx, "git", "checkout", branchName) + var stderr strings.Builder + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + stderrStr := strings.TrimSpace(stderr.String()) + log.Error("git checkout failed", "error", err, "branch", branchName, "stderr", stderrStr) + return errors.ErrSwitchBranch.WithError(err).WithContext("branch", branchName).WithContext("stderr", stderrStr) + } + return nil +} + +// PushBranch pushes a branch to origin, setting up tracking. Like +// Push/PushTag, it detects and flags a GitHub ruleset rejection so callers +// can tell "the branch itself is also protected" apart from any other +// push failure. +func (s *GitService) PushBranch(ctx context.Context, branchName string) error { + log := logger.FromContext(ctx) + + cmd := exec.CommandContext(ctx, "git", "push", "-u", "origin", branchName) + var stderr strings.Builder + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + stderrStr := strings.TrimSpace(stderr.String()) + log.Error("git push branch failed", "error", err, "branch", branchName, "stderr", stderrStr) + + if isRulesetRejection(stderrStr) { + return errors.ErrPushRejectedByRuleset.WithError(err).WithContext("branch", branchName).WithContext("stderr", stderrStr) + } + return errors.ErrPushBranch.WithError(err).WithContext("branch", branchName).WithContext("stderr", stderrStr) + } + return nil +} + // isRulesetRejection reports whether git's push failure output indicates the // push was rejected by a GitHub ruleset or legacy branch/tag protection rule // (as opposed to a network/auth/diverged-history failure). GH013 is GitHub's diff --git a/internal/models/pr.go b/internal/models/pr.go index 9b88d81..fcc46d7 100644 --- a/internal/models/pr.go +++ b/internal/models/pr.go @@ -56,4 +56,10 @@ type ( Labels []string Usage *TokenUsage } + + // CreatedPR identifies a Pull Request that was just opened. + CreatedPR struct { + Number int + URL string + } ) diff --git a/internal/services/release_changelog.go b/internal/services/release_changelog.go index 196f6ad..db2ecb8 100644 --- a/internal/services/release_changelog.go +++ b/internal/services/release_changelog.go @@ -2,6 +2,7 @@ package services import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -773,7 +774,82 @@ func (s *ReleaseService) CommitChangelog(ctx context.Context, version string) er } -// PushChanges pushes committed changes to the remote repository -func (s *ReleaseService) PushChanges(ctx context.Context) error { - return s.git.Push(ctx) +// PushChanges pushes committed changes to the remote repository. If the +// push is rejected by a GitHub ruleset/branch protection rule, it falls +// back to pushing a new branch and opening a pull request instead — but +// only when that fallback actually works (the branch itself isn't also +// blocked by a broader ruleset). On success via the fallback, the returned +// error wraps domainErrors.ErrReleasePROpened rather than signaling a +// genuine failure; callers should check for it with errors.Is. +func (s *ReleaseService) PushChanges(ctx context.Context, version string) error { + log := logger.FromContext(ctx) + + err := s.git.Push(ctx) + if err == nil { + return nil + } + if !errors.Is(err, domainErrors.ErrPushRejectedByRuleset) { + return err + } + + log.Warn("push rejected by a ruleset, attempting to open a pull request instead", "error", err) + return s.openReleasePR(ctx, version) +} + +// openReleasePR is the fallback used by PushChanges when a direct push is +// blocked by a ruleset: it moves the already-committed changes onto a new +// branch, pushes that branch, and opens a PR into the original branch. +func (s *ReleaseService) openReleasePR(ctx context.Context, version string) error { + log := logger.FromContext(ctx) + + baseBranch, err := s.git.GetCurrentBranch(ctx) + if err != nil { + return domainErrors.NewAppError(domainErrors.TypeGit, "failed to determine current branch", err) + } + + branchName := fmt.Sprintf("matecommit/release-%s", version) + + if err := s.git.CreateAndSwitchBranch(ctx, branchName); err != nil { + return err + } + + pushErr := s.git.PushBranch(ctx, branchName) + + // Switch back to the original branch regardless of outcome so the user + // isn't left stranded on the temporary release branch. This only moves + // HEAD — the changelog commit stays safely on both branches, nothing + // destructive happens to the user's local state. + if switchErr := s.git.SwitchBranch(ctx, baseBranch); switchErr != nil { + log.Error("failed to switch back to original branch after opening release PR", + "branch", baseBranch, "error", switchErr) + } + + if pushErr != nil { + log.Error("pushing the fallback release branch also failed — a broader ruleset is likely in play", + "branch", branchName, "error", pushErr) + return pushErr + } + + if s.vcsClient == nil { + return domainErrors.NewAppError(domainErrors.TypeConfiguration, + fmt.Sprintf("branch %s was pushed, but no VCS provider is configured to open the pull request automatically", branchName), nil). + WithSuggestion(fmt.Sprintf("Open a pull request manually from %s into %s", branchName, baseBranch)) + } + + title := fmt.Sprintf("chore: release %s", version) + body := fmt.Sprintf( + "Opened automatically by matecommit: `%s` is protected by a ruleset that blocks direct pushes.\n\nMerge this PR, then re-run the release command to finish tagging and publishing %s.", + baseBranch, version) + + pr, err := s.vcsClient.CreatePR(ctx, title, body, branchName, baseBranch) + if err != nil { + return domainErrors.NewAppError(domainErrors.TypeVCS, "branch pushed, but failed to open the pull request", err). + WithSuggestion(fmt.Sprintf("Open a pull request manually from %s into %s", branchName, baseBranch)) + } + + log.Info("opened pull request for protected branch", "url", pr.URL, "number", pr.Number) + + return domainErrors.ErrReleasePROpened. + WithContext("pr_url", pr.URL). + WithSuggestion(fmt.Sprintf("Merge %s, then re-run this command to finish the release", pr.URL)) } diff --git a/internal/services/release_service.go b/internal/services/release_service.go index a8077ec..9648d33 100644 --- a/internal/services/release_service.go +++ b/internal/services/release_service.go @@ -30,6 +30,9 @@ type releaseGitService interface { FetchTags(ctx context.Context) error ValidateTagExists(ctx context.Context, tag string) error GetRepoRoot(ctx context.Context) (string, error) + CreateAndSwitchBranch(ctx context.Context, branchName string) error + SwitchBranch(ctx context.Context, branchName string) error + PushBranch(ctx context.Context, branchName string) error } type ReleaseService struct { git releaseGitService diff --git a/internal/services/release_service_improvements_test.go b/internal/services/release_service_improvements_test.go index 1f6788e..9299d4c 100644 --- a/internal/services/release_service_improvements_test.go +++ b/internal/services/release_service_improvements_test.go @@ -507,3 +507,8 @@ func (m *mockGitService) GetCurrentBranch(ctx context.Context) (string, error) func (m *mockGitService) FetchTags(ctx context.Context) error { return nil } func (m *mockGitService) ValidateTagExists(ctx context.Context, tag string) error { return nil } func (m *mockGitService) GetRepoRoot(ctx context.Context) (string, error) { return ".", nil } +func (m *mockGitService) CreateAndSwitchBranch(ctx context.Context, branchName string) error { + return nil +} +func (m *mockGitService) SwitchBranch(ctx context.Context, branchName string) error { return nil } +func (m *mockGitService) PushBranch(ctx context.Context, branchName string) error { return nil } diff --git a/internal/services/release_service_test.go b/internal/services/release_service_test.go index 20decae..ba2622c 100644 --- a/internal/services/release_service_test.go +++ b/internal/services/release_service_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/thomas-vilte/matecommit/internal/config" + domainErrors "github.com/thomas-vilte/matecommit/internal/errors" "github.com/thomas-vilte/matecommit/internal/models" "github.com/thomas-vilte/matecommit/internal/testutil" ) @@ -1435,7 +1436,7 @@ func TestReleaseService_PushChanges_RealScenarios(t *testing.T) { mockGit.On("Push", mock.Anything).Return(nil) - err := service.PushChanges(context.Background()) + err := service.PushChanges(context.Background(), "v1.0.0") assert.NoError(t, err) mockGit.AssertExpectations(t) @@ -1447,12 +1448,84 @@ func TestReleaseService_PushChanges_RealScenarios(t *testing.T) { mockGit.On("Push", mock.Anything).Return(errors.New("failed to push: remote rejected")) - err := service.PushChanges(context.Background()) + err := service.PushChanges(context.Background(), "v1.0.0") assert.Error(t, err) assert.Contains(t, err.Error(), "remote rejected") mockGit.AssertExpectations(t) }) + + t.Run("Ruleset rejection falls back to opening a pull request", func(t *testing.T) { + mockGit := new(testutil.MockGitService) + mockVCS := new(testutil.MockVCSClient) + service := NewReleaseService(mockGit, WithReleaseVCSClient(mockVCS)) + + rulesetErr := domainErrors.ErrPushRejectedByRuleset.WithError(errors.New("exit status 1")).WithContext("stderr", "GH013") + mockGit.On("Push", mock.Anything).Return(rulesetErr) + mockGit.On("GetCurrentBranch", mock.Anything).Return("master", nil) + mockGit.On("CreateAndSwitchBranch", mock.Anything, "matecommit/release-v1.0.0").Return(nil) + mockGit.On("PushBranch", mock.Anything, "matecommit/release-v1.0.0").Return(nil) + mockGit.On("SwitchBranch", mock.Anything, "master").Return(nil) + mockVCS.On("CreatePR", mock.Anything, mock.Anything, mock.Anything, "matecommit/release-v1.0.0", "master"). + Return(&models.CreatedPR{Number: 42, URL: "https://github.com/owner/repo/pull/42"}, nil) + + err := service.PushChanges(context.Background(), "v1.0.0") + + assert.Error(t, err, "still returns a non-nil error — the release isn't finished, it needs a merge + re-run") + assert.True(t, errors.Is(err, domainErrors.ErrReleasePROpened), + "callers must be able to distinguish this from a genuine failure via errors.Is") + + var appErr *domainErrors.AppError + assert.True(t, errors.As(err, &appErr)) + assert.Equal(t, "https://github.com/owner/repo/pull/42", appErr.Context["pr_url"]) + + mockGit.AssertExpectations(t) + mockVCS.AssertExpectations(t) + }) + + t.Run("Ruleset also blocking the fallback branch surfaces the original rejection, not a fake success", func(t *testing.T) { + mockGit := new(testutil.MockGitService) + mockVCS := new(testutil.MockVCSClient) + service := NewReleaseService(mockGit, WithReleaseVCSClient(mockVCS)) + + rulesetErr := domainErrors.ErrPushRejectedByRuleset.WithError(errors.New("exit status 1")) + branchRulesetErr := domainErrors.ErrPushRejectedByRuleset.WithError(errors.New("exit status 1")).WithContext("stderr", "GH013: broader ruleset") + mockGit.On("Push", mock.Anything).Return(rulesetErr) + mockGit.On("GetCurrentBranch", mock.Anything).Return("master", nil) + mockGit.On("CreateAndSwitchBranch", mock.Anything, "matecommit/release-v1.0.0").Return(nil) + mockGit.On("PushBranch", mock.Anything, "matecommit/release-v1.0.0").Return(branchRulesetErr) + mockGit.On("SwitchBranch", mock.Anything, "master").Return(nil) + + err := service.PushChanges(context.Background(), "v1.0.0") + + assert.Error(t, err) + assert.False(t, errors.Is(err, domainErrors.ErrReleasePROpened), + "must not claim a PR was opened when the fallback branch push also failed") + assert.True(t, errors.Is(err, domainErrors.ErrPushRejectedByRuleset)) + + mockGit.AssertExpectations(t) + mockVCS.AssertNotCalled(t, "CreatePR", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("No VCS provider configured: branch is pushed but PR creation is skipped with a manual-follow-up message", func(t *testing.T) { + mockGit := new(testutil.MockGitService) + service := NewReleaseService(mockGit) + + rulesetErr := domainErrors.ErrPushRejectedByRuleset.WithError(errors.New("exit status 1")) + mockGit.On("Push", mock.Anything).Return(rulesetErr) + mockGit.On("GetCurrentBranch", mock.Anything).Return("master", nil) + mockGit.On("CreateAndSwitchBranch", mock.Anything, "matecommit/release-v1.0.0").Return(nil) + mockGit.On("PushBranch", mock.Anything, "matecommit/release-v1.0.0").Return(nil) + mockGit.On("SwitchBranch", mock.Anything, "master").Return(nil) + + err := service.PushChanges(context.Background(), "v1.0.0") + + assert.Error(t, err) + assert.False(t, errors.Is(err, domainErrors.ErrReleasePROpened)) + assert.Contains(t, err.Error(), "no VCS provider is configured") + + mockGit.AssertExpectations(t) + }) } func TestReleaseService_UpdateLocalChangelog_RealScenarios(t *testing.T) { diff --git a/internal/testutil/mockgit.go b/internal/testutil/mockgit.go index dcba9d3..61bf2d8 100644 --- a/internal/testutil/mockgit.go +++ b/internal/testutil/mockgit.go @@ -124,6 +124,21 @@ func (m *MockGitService) Push(ctx context.Context) error { return args.Error(0) } +func (m *MockGitService) CreateAndSwitchBranch(ctx context.Context, branchName string) error { + args := m.Called(ctx, branchName) + return args.Error(0) +} + +func (m *MockGitService) SwitchBranch(ctx context.Context, branchName string) error { + args := m.Called(ctx, branchName) + return args.Error(0) +} + +func (m *MockGitService) PushBranch(ctx context.Context, branchName string) error { + args := m.Called(ctx, branchName) + return args.Error(0) +} + func (m *MockGitService) FetchTags(ctx context.Context) error { args := m.Called(ctx) return args.Error(0) diff --git a/internal/testutil/mockvcs.go b/internal/testutil/mockvcs.go index f2202eb..76e2414 100644 --- a/internal/testutil/mockvcs.go +++ b/internal/testutil/mockvcs.go @@ -18,6 +18,14 @@ func (m *MockVCSClient) UpdatePR(ctx context.Context, prNumber int, summary mode return args.Error(0) } +func (m *MockVCSClient) CreatePR(ctx context.Context, title, body, headBranch, baseBranch string) (*models.CreatedPR, error) { + args := m.Called(ctx, title, body, headBranch, baseBranch) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*models.CreatedPR), args.Error(1) +} + func (m *MockVCSClient) GetPR(ctx context.Context, prNumber int) (models.PRData, error) { args := m.Called(ctx, prNumber) return args.Get(0).(models.PRData), args.Error(1) diff --git a/internal/vcs/github/client.go b/internal/vcs/github/client.go index 8fb94f1..b06fc14 100644 --- a/internal/vcs/github/client.go +++ b/internal/vcs/github/client.go @@ -18,6 +18,7 @@ import ( var _ vcs.VCSClient = (*GitHubClient)(nil) type PullRequestsService interface { + Create(ctx context.Context, owner, repo string, pull *github.NewPullRequest) (*github.PullRequest, *github.Response, error) Edit(ctx context.Context, owner, repo string, number int, pr *github.PullRequest) (*github.PullRequest, *github.Response, error) List(ctx context.Context, owner, repo string, opts *github.PullRequestListOptions) ([]*github.PullRequest, *github.Response, error) Get(ctx context.Context, owner, repo string, number int) (*github.PullRequest, *github.Response, error) diff --git a/internal/vcs/github/client_pr.go b/internal/vcs/github/client_pr.go index fb20f8c..2daabba 100644 --- a/internal/vcs/github/client_pr.go +++ b/internal/vcs/github/client_pr.go @@ -53,6 +53,36 @@ func (ghc *GitHubClient) UpdatePR(ctx context.Context, prNumber int, summary mod return nil } +// CreatePR opens a new Pull Request from headBranch into baseBranch. +func (ghc *GitHubClient) CreatePR(ctx context.Context, title, body, headBranch, baseBranch string) (*models.CreatedPR, error) { + pr, resp, err := ghc.prService.Create(ctx, ghc.owner, ghc.repo, &github.NewPullRequest{ + Title: github.Ptr(title), + Body: github.Ptr(body), + Head: github.Ptr(headBranch), + Base: github.Ptr(baseBranch), + }) + if err != nil { + if resp != nil { + if resp.StatusCode == http.StatusTooManyRequests { + return nil, domainErrors.ErrGitHubRateLimit. + WithContext("retry_after", resp.Header.Get("Retry-After")). + WithContext("operation", "create PR") + } + if resp.StatusCode == http.StatusForbidden { + return nil, domainErrors.ErrGitHubInsufficientPerms. + WithContext("operation", "create PR"). + WithContext("repo", fmt.Sprintf("%s/%s", ghc.owner, ghc.repo)) + } + } + return nil, fmt.Errorf("failed to create PR from %s to %s: %w", headBranch, baseBranch, err) + } + + return &models.CreatedPR{ + Number: pr.GetNumber(), + URL: pr.GetHTMLURL(), + }, nil +} + func (ghc *GitHubClient) GetPR(ctx context.Context, prNumber int) (models.PRData, error) { log := logger.FromContext(ctx) diff --git a/internal/vcs/github/mocks_test.go b/internal/vcs/github/mocks_test.go index 4ee101b..5bb404a 100644 --- a/internal/vcs/github/mocks_test.go +++ b/internal/vcs/github/mocks_test.go @@ -14,6 +14,11 @@ type MockPRService struct { mock.Mock } +func (m *MockPRService) Create(ctx context.Context, owner, repo string, pull *github.NewPullRequest) (*github.PullRequest, *github.Response, error) { + args := m.Called(ctx, owner, repo, pull) + return args.Get(0).(*github.PullRequest), args.Get(1).(*github.Response), args.Error(2) +} + func (m *MockPRService) Edit(ctx context.Context, owner, repo string, number int, pr *github.PullRequest) (*github.PullRequest, *github.Response, error) { args := m.Called(ctx, owner, repo, number, pr) return args.Get(0).(*github.PullRequest), args.Get(1).(*github.Response), args.Error(2) diff --git a/internal/vcs/interfaces.go b/internal/vcs/interfaces.go index 0d671ad..0bc1c4b 100644 --- a/internal/vcs/interfaces.go +++ b/internal/vcs/interfaces.go @@ -10,6 +10,8 @@ import ( type VCSClient interface { // UpdatePR updates a Pull Request (title, body, and labels) in the provider. UpdatePR(ctx context.Context, prNumber int, summary models.PRSummary) error + // CreatePR opens a new Pull Request from headBranch into baseBranch. + CreatePR(ctx context.Context, title, body, headBranch, baseBranch string) (*models.CreatedPR, error) // GetPR gets the PR data (for example, to extract commits, diff, etc.). GetPR(ctx context.Context, prNumber int) (models.PRData, error) // GetRepoLabels gets all available labels in the repository