From f4bdd1f7e8c501ab7605de9c8698c403e076245d Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 10 Aug 2026 16:32:13 -0400 Subject: [PATCH] fix(gitprovider): skip GitLab approve when the caller's approval already stands GitLab's approve endpoint responds 401 when the authenticated user already holds a standing approval on the merge request, which cr mapped to a permission/authentication failure and failed the whole run even though the review completed. SubmitReview now reads the merge request approvals first and treats a standing approval by the current user as already satisfied, so reruns post their summary note and succeed. A genuine credential 401 still surfaces as an authentication failure via the reads that precede the approve write. --- internal/gitprovider/gitlab/rest_reads.go | 8 +- internal/gitprovider/gitlab/rest_writes.go | 43 +++++++- .../gitprovider/gitlab/rest_writes_test.go | 98 +++++++++++++++++-- 3 files changed, 137 insertions(+), 12 deletions(-) diff --git a/internal/gitprovider/gitlab/rest_reads.go b/internal/gitprovider/gitlab/rest_reads.go index b8a2a046..ab1f17aa 100644 --- a/internal/gitprovider/gitlab/rest_reads.go +++ b/internal/gitprovider/gitlab/rest_reads.go @@ -53,10 +53,12 @@ type diffFileResponse struct { Diff string `json:"diff"` } +type approvalEntryResponse struct { + User userResponse `json:"user"` +} + type approvalsResponse struct { - ApprovedBy []struct { - User userResponse `json:"user"` - } `json:"approved_by"` + ApprovedBy []approvalEntryResponse `json:"approved_by"` } type noteResponse struct { diff --git a/internal/gitprovider/gitlab/rest_writes.go b/internal/gitprovider/gitlab/rest_writes.go index c43c00ad..ee018c6d 100644 --- a/internal/gitprovider/gitlab/rest_writes.go +++ b/internal/gitprovider/gitlab/rest_writes.go @@ -79,10 +79,21 @@ func (c *Client) SubmitReview(ctx context.Context, ref gitprovider.PRRef, reques return gitprovider.ReviewID(stringIDFromInt(response.ID)), nil } +// approve applies the caller's approval to the merge request. GitLab responds +// 401 when the caller already holds a standing approval, indistinguishable by +// status from a credential failure, so a standing approval is detected first +// and treated as already satisfied. func (c *Client) approve(ctx context.Context, ref gitprovider.PRRef, commitSHA string) error { op := gitprovider.OperationSubmitReview + approved, err := c.currentUserApproved(ctx, op, ref) + if err != nil { + return err + } + if approved { + return nil + } endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "approve") - err := c.doRESTJSON(ctx, op, http.MethodPost, endpoint, approveRequest{SHA: commitSHA}, nil) + err = c.doRESTJSON(ctx, op, http.MethodPost, endpoint, approveRequest{SHA: commitSHA}, nil) switch { case err == nil: return nil @@ -100,6 +111,36 @@ func (c *Client) approve(ctx context.Context, ref gitprovider.PRRef, commitSHA s } } +// currentUserApproved reports whether the authenticated user already has a +// standing approval on the merge request. The current-user lookup only happens +// when the merge request has approvals at all, so the common fresh-approve +// path costs one extra read. +func (c *Client) currentUserApproved(ctx context.Context, op gitprovider.Operation, ref gitprovider.PRRef) (bool, error) { + var approvals approvalsResponse + endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "approvals") + if _, _, err := c.doREST(ctx, op, http.MethodGet, endpoint, acceptJSON, &approvals); err != nil { + return false, err + } + if len(approvals.ApprovedBy) == 0 { + return false, nil + } + var user userResponse + if _, _, err := c.doREST(ctx, op, http.MethodGet, restURL(c.baseURL, "user"), acceptJSON, &user); err != nil { + return false, err + } + if user.ID <= 0 { + // Without a usable current-user ID the standing approval cannot be + // attributed; fall through to the approve attempt. + return false, nil + } + for _, approval := range approvals.ApprovedBy { + if approval.User.ID == user.ID { + return true, nil + } + } + return false, nil +} + func (c *Client) unapprove(ctx context.Context, ref gitprovider.PRRef) error { endpoint := restURL(c.baseURL, "projects", projectSegment(ref), "merge_requests", fmt.Sprint(ref.Number), "unapprove") err := c.doRESTJSON(ctx, gitprovider.OperationSubmitReview, http.MethodPost, endpoint, nil, nil) diff --git a/internal/gitprovider/gitlab/rest_writes_test.go b/internal/gitprovider/gitlab/rest_writes_test.go index 59d74953..8d86ebc0 100644 --- a/internal/gitprovider/gitlab/rest_writes_test.go +++ b/internal/gitprovider/gitlab/rest_writes_test.go @@ -35,16 +35,26 @@ func TestPostIssueCommentPostsNote(t *testing.T) { } } -func submitReviewServer(t *testing.T, headSHA string, approveStatus, unapproveStatus int, calls *[]string) *httptest.Server { +func submitReviewServer(t *testing.T, headSHA string, approveStatus, unapproveStatus int, approverIDs []int64, calls *[]string) *httptest.Server { t.Helper() return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/user": + *calls = append(*calls, "user") + writeJSON(t, w, userResponse{ID: 7, Username: "review-bot"}) case r.Method == http.MethodGet && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42": *calls = append(*calls, "get") writeJSON(t, w, mergeRequestResponse{ State: "opened", SourceBranch: "feature", TargetBranch: "main", DiffRefs: diffRefsResponse{BaseSHA: "basesha", StartSHA: "startsha", HeadSHA: headSHA}, }) + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/approvals": + *calls = append(*calls, "approvals") + var payload approvalsResponse + for _, id := range approverIDs { + payload.ApprovedBy = append(payload.ApprovedBy, approvalEntryResponse{User: userResponse{ID: id}}) + } + writeJSON(t, w, payload) case r.Method == http.MethodPost && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/approve": var request approveRequest decodeJSON(t, r.Body, &request) @@ -74,7 +84,7 @@ func submitReviewServer(t *testing.T, headSHA string, approveStatus, unapproveSt func TestSubmitReviewApproveAppliesApprovalThenPostsSummaryNote(t *testing.T) { var calls []string - server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, &calls) + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, nil, &calls) defer server.Close() client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) id, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ @@ -88,14 +98,54 @@ func TestSubmitReviewApproveAppliesApprovalThenPostsSummaryNote(t *testing.T) { if id != gitprovider.ReviewID("88") { t.Fatalf("id = %q, want 88", id) } - if !reflect.DeepEqual(calls, []string{"get", "approve:headsha", "note:review summary"}) { + if !reflect.DeepEqual(calls, []string{"get", "approvals", "approve:headsha", "note:review summary"}) { t.Fatalf("calls = %#v, want approval before summary note", calls) } } +func TestSubmitReviewApproveSkipsApproveWhenApprovalAlreadyStands(t *testing.T) { + var calls []string + // User 7 is the authenticated user served by /user; GitLab would respond + // 401 to a second approve, so the approve call must not happen at all. + server := submitReviewServer(t, "headsha", http.StatusUnauthorized, http.StatusCreated, []int64{7}, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + id, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventApprove, + Body: "review summary", + }) + if err != nil { + t.Fatalf("SubmitReview: %v", err) + } + if id != gitprovider.ReviewID("88") { + t.Fatalf("id = %q, want 88", id) + } + if !reflect.DeepEqual(calls, []string{"get", "approvals", "user", "note:review summary"}) { + t.Fatalf("calls = %#v, want standing approval detected and approve skipped", calls) + } +} + +func TestSubmitReviewApproveStillApprovesWhenOnlyOthersApproved(t *testing.T) { + var calls []string + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, []int64{12}, &calls) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + if _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventApprove, + Body: "review summary", + }); err != nil { + t.Fatalf("SubmitReview: %v", err) + } + if !reflect.DeepEqual(calls, []string{"get", "approvals", "user", "approve:headsha", "note:review summary"}) { + t.Fatalf("calls = %#v, want approve despite other users' approvals", calls) + } +} + func TestSubmitReviewRequestChangesRevokesApprovalAndIgnoresMissingApproval(t *testing.T) { var calls []string - server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusNotFound, &calls) + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusNotFound, nil, &calls) defer server.Close() client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) if _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ @@ -112,7 +162,7 @@ func TestSubmitReviewRequestChangesRevokesApprovalAndIgnoresMissingApproval(t *t func TestSubmitReviewCommentOnlyPostsNote(t *testing.T) { var calls []string - server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, &calls) + server := submitReviewServer(t, "headsha", http.StatusCreated, http.StatusCreated, nil, &calls) defer server.Close() client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) if _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ @@ -129,7 +179,7 @@ func TestSubmitReviewCommentOnlyPostsNote(t *testing.T) { func TestSubmitReviewRejectsStaleCommitBeforeWriting(t *testing.T) { var calls []string - server := submitReviewServer(t, "newer-head", http.StatusCreated, http.StatusCreated, &calls) + server := submitReviewServer(t, "newer-head", http.StatusCreated, http.StatusCreated, nil, &calls) defer server.Close() client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ @@ -147,7 +197,7 @@ func TestSubmitReviewRejectsStaleCommitBeforeWriting(t *testing.T) { func TestSubmitReviewMapsApproveConflictToStaleSHA(t *testing.T) { var calls []string - server := submitReviewServer(t, "headsha", http.StatusConflict, http.StatusCreated, &calls) + server := submitReviewServer(t, "headsha", http.StatusConflict, http.StatusCreated, nil, &calls) defer server.Close() client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ @@ -162,7 +212,7 @@ func TestSubmitReviewMapsApproveConflictToStaleSHA(t *testing.T) { func TestSubmitReviewMapsApproveUnauthorizedToPermission(t *testing.T) { var calls []string - server := submitReviewServer(t, "headsha", http.StatusUnauthorized, http.StatusCreated, &calls) + server := submitReviewServer(t, "headsha", http.StatusUnauthorized, http.StatusCreated, nil, &calls) defer server.Close() client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ @@ -175,6 +225,38 @@ func TestSubmitReviewMapsApproveUnauthorizedToPermission(t *testing.T) { } } +func TestSubmitReviewSurfacesAuthFailureFromApprovalsRead(t *testing.T) { + var calls []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42": + calls = append(calls, "get") + writeJSON(t, w, mergeRequestResponse{ + State: "opened", SourceBranch: "feature", TargetBranch: "main", + DiffRefs: diffRefsResponse{BaseSHA: "basesha", StartSHA: "startsha", HeadSHA: "headsha"}, + }) + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/projects/"+testProjectPath()+"/merge_requests/42/approvals": + calls = append(calls, "approvals") + w.WriteHeader(http.StatusUnauthorized) + default: + t.Fatalf("unexpected request %s %q", r.Method, r.URL.EscapedPath()) + } + })) + defer server.Close() + client := mustClient(t, Options{Host: "gitlab.example.com", BaseURL: server.URL}) + _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{ + CommitSHA: "headsha", + Event: review.ReviewEventApprove, + Body: "review summary", + }) + if !errors.Is(err, gitprovider.ErrAuth) { + t.Fatalf("SubmitReview error = %v, want ErrAuth", err) + } + if !reflect.DeepEqual(calls, []string{"get", "approvals"}) { + t.Fatalf("calls = %#v, want no writes after credential failure", calls) + } +} + func TestSubmitReviewRejectsBundledComments(t *testing.T) { client := mustClient(t, Options{Host: "gitlab.example.com"}) _, err := client.SubmitReview(context.Background(), testPRRef(), gitprovider.ReviewRequest{