fix(github): pick allowed merge method so squash-only repos stop 405ing (#999)

* fix(github): pick allowed merge method so squash-only repos stop 405ing The merge button sent an empty merge_method to GitHub, which defaults to "merge" regardless of repo settings — squash-only / rebase-only repos returned 405 "Merge commits are not allowed on this repository". The backend now resolves an empty method to the first allowed one via a new GetRepoMergeMethods endpoint (5-min cache), and the frontend renders a GitHub-style split button so users can pick among the allowed methods. * fix(github): address PR #999 review feedback - pr-merge-button: extract repo-methods fetch to useRepoMergeMethods hook (Claude bot); hide button while methods are loading so the primary label doesn't flicker (Claude bot); drop e.preventDefault on dropdown items so Radix closes the menu on selection (Greptile P1). - gh_client / pat_client: treat missing allow_* fields as false instead of true so a permission-gated response can't pick a disallowed merge method (cubic P2). - service.MergePR: only overwrite mergeMethod when pickDefaultMergeMethod returns non-empty, so a degenerate all-false config falls through to GitHub's default with the real error (Claude bot). * fix(github): don't hide merge button on lookup failure Round-2 PR feedback (Claude, Greptile, CodeRabbit, cubic): the `if (!methods) return null` guard added in the previous round to avoid label flicker turned a transient repo-methods lookup failure into a permanent UI lockout — the button would stay hidden until unmount/remount. - pr-merge-button: revert the null-guard; treat null as "use backend resolver", render the plain primary button without a dropdown. - use-repo-merge-methods: add a 5-min frontend TTL to match the backend cache so admin merge-strategy flips are visible without a page reload. Catch block now triggers a rerender, and co-waiting consumers attach both fulfilled/rejected handlers to avoid silent rejection swallowing. * test(github): cover useRepoMergeMethods + silence rejection warning Round-3 PR feedback (Claude): - Swallow rejections in the originating fetch chain instead of rethrowing, so the first consumer's promise never escapes the hook as an unhandled rejection. Coalesced consumers already re-render to null, which is the same state a fresh "not loaded" call would show. - Add use-repo-merge-methods.test.ts covering: null-on-missing-params, loading then resolution, cache hit on remount, concurrent-fetch coalescing, retry-on-failure (no cache poisoning, no unhandled rejection), and TTL-expiry refetch via fake timers. * test(github): cover httpGetRepoMergeMethods error branches Round-4 PR feedback (Claude): the new endpoint had only happy-path coverage. Add tests matching httpMergePR's existing shape: - nil client → 503 (ErrNoClient) - GitHubAPIError{404} → 404 - generic upstream error → 500 --------- Co-authored-by: Kandev Agent <agent@kandev.dev>
2026-05-21 23:22:31 +01:00
parent f8654880f3
commit b8a09bc5ea
16 changed files with 778 additions and 36 deletions
@@ -17,8 +17,9 @@ import (

 // stubClient implements Client with no-op defaults; override fields as needed.
 type stubClient struct {
-	getPRFunc func(ctx context.Context, owner, repo string, number int) (*PR, error)
-	mergePRFn func(ctx context.Context, owner, repo string, number int, mergeMethod string) error
+	getPRFunc             func(ctx context.Context, owner, repo string, number int) (*PR, error)
+	mergePRFn             func(ctx context.Context, owner, repo string, number int, mergeMethod string) error
+	getRepoMergeMethodsFn func() (RepoMergeMethods, error)
 }

 func (s *stubClient) IsAuthenticated(context.Context) (bool, error) { return true, nil }
@@ -80,6 +81,12 @@ func (s *stubClient) MergePR(ctx context.Context, owner, repo string, number int
 func (s *stubClient) ListRepoBranches(context.Context, string, string) ([]RepoBranch, error) {
 	return nil, nil
 }
+func (s *stubClient) GetRepoMergeMethods(context.Context, string, string) (RepoMergeMethods, error) {
+	if s.getRepoMergeMethodsFn != nil {
+		return s.getRepoMergeMethodsFn()
+	}
+	return RepoMergeMethods{Merge: true, Squash: true, Rebase: true}, nil
+}
 func (s *stubClient) ListIssues(context.Context, string, string) ([]*Issue, error) {
 	return nil, nil
 }
@@ -217,13 +224,82 @@ func TestHttpMergePR_InvalidMethod(t *testing.T) {
 	}
 }

-func TestHttpMergePR_EmptyBody_UsesDefaultMethod(t *testing.T) {
+func TestHttpMergePR_EmptyBody_ResolvesToAllowedMethod(t *testing.T) {
+	// Empty merge_method must NOT propagate to GitHub — that would let
+	// GitHub default to "merge" and 405 on squash-only repos. The service
+	// should resolve to the first allowed method instead.
 	var gotMethod string
 	sc := &stubClient{
 		mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error {
 			gotMethod = mergeMethod
 			return nil
 		},
+		getRepoMergeMethodsFn: func() (RepoMergeMethods, error) {
+			// Squash-only repo (the case that surfaced this bug).
+			return RepoMergeMethods{Squash: true}, nil
+		},
+	}
+	router, _ := setupControllerTest(sc)
+
+	req := httptest.NewRequest(http.MethodPut, "/api/v1/github/prs/acme/widget/42/merge", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if gotMethod != "squash" {
+		t.Errorf("expected merge_method=squash, got %q", gotMethod)
+	}
+}
+
+func TestHttpMergePR_ExplicitMethod_Passthrough(t *testing.T) {
+	// When the user picks a method from the dropdown, the service must
+	// NOT second-guess it via the repo lookup.
+	var gotMethod string
+	var lookupCalls int
+	sc := &stubClient{
+		mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error {
+			gotMethod = mergeMethod
+			return nil
+		},
+		getRepoMergeMethodsFn: func() (RepoMergeMethods, error) {
+			lookupCalls++
+			return RepoMergeMethods{Merge: true}, nil
+		},
+	}
+	router, _ := setupControllerTest(sc)
+
+	body := bytes.NewBufferString(`{"merge_method":"rebase"}`)
+	req := httptest.NewRequest(http.MethodPut, "/api/v1/github/prs/acme/widget/42/merge", body)
+	req.Header.Set("Content-Type", "application/json")
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if gotMethod != "rebase" {
+		t.Errorf("expected merge_method=rebase, got %q", gotMethod)
+	}
+	if lookupCalls != 0 {
+		t.Errorf("expected no merge-methods lookup for explicit pin, got %d", lookupCalls)
+	}
+}
+
+func TestHttpMergePR_EmptyBody_LookupFails_FallsBackToGitHubDefault(t *testing.T) {
+	// If the repo lookup itself errors, we still attempt the merge with an
+	// empty method and let GitHub surface whatever error it surfaces — better
+	// than refusing to merge because of an unrelated lookup failure.
+	var gotMethod string
+	sc := &stubClient{
+		mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error {
+			gotMethod = mergeMethod
+			return nil
+		},
+		getRepoMergeMethodsFn: func() (RepoMergeMethods, error) {
+			return RepoMergeMethods{}, fmt.Errorf("rate limited")
+		},
 	}
 	router, _ := setupControllerTest(sc)

@@ -235,7 +311,84 @@ func TestHttpMergePR_EmptyBody_UsesDefaultMethod(t *testing.T) {
 		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
 	}
 	if gotMethod != "" {
-		t.Errorf("expected empty merge_method, got %q", gotMethod)
+		t.Errorf("expected empty merge_method (fall back to GitHub default), got %q", gotMethod)
+	}
+}
+
+func TestHttpGetRepoMergeMethods_OK(t *testing.T) {
+	sc := &stubClient{
+		getRepoMergeMethodsFn: func() (RepoMergeMethods, error) {
+			return RepoMergeMethods{Squash: true, Rebase: true}, nil
+		},
+	}
+	router, _ := setupControllerTest(sc)
+
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	var got RepoMergeMethods
+	if err := json.NewDecoder(w.Body).Decode(&got); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	want := RepoMergeMethods{Squash: true, Rebase: true}
+	if got != want {
+		t.Errorf("got %+v, want %+v", got, want)
+	}
+}
+
+func TestHttpGetRepoMergeMethods_NoClient_Returns503(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	log := newControllerTestLogger()
+	// nil client triggers ErrNoClient via Service.GetRepoMergeMethods.
+	svc := NewService(nil, "none", nil, nil, nil, log)
+	ctrl := NewController(svc, log)
+	router := gin.New()
+	ctrl.RegisterHTTPRoutes(router)
+
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusServiceUnavailable {
+		t.Fatalf("expected 503, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestHttpGetRepoMergeMethods_NotFound_Returns404(t *testing.T) {
+	sc := &stubClient{
+		getRepoMergeMethodsFn: func() (RepoMergeMethods, error) {
+			return RepoMergeMethods{}, &GitHubAPIError{StatusCode: http.StatusNotFound, Endpoint: "/repos/acme/widget"}
+		},
+	}
+	router, _ := setupControllerTest(sc)
+
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusNotFound {
+		t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestHttpGetRepoMergeMethods_OtherError_Returns500(t *testing.T) {
+	sc := &stubClient{
+		getRepoMergeMethodsFn: func() (RepoMergeMethods, error) {
+			return RepoMergeMethods{}, fmt.Errorf("unexpected upstream failure")
+		},
+	}
+	router, _ := setupControllerTest(sc)
+
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String())
 	}
 }