Skip to content

fix(gitprovider): skip GitLab approve when the caller's approval already stands #564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
piekstra merged 1 commit into from
Aug 10, 2026
Merged

fix(gitprovider): skip GitLab approve when the caller's approval already stands #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions internal/gitprovider/gitlab/rest_reads.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
43 changes: 42 additions & 1 deletion internal/gitprovider/gitlab/rest_writes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {

@monit-reviewer monit-reviewer Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentUserApproved is a read-only helper (two GETs, no writes) placed in rest_writes.go, and it rebuilds the approvals endpoint URL that ListReviews already builds verbatim at internal/gitprovider/gitlab/rest_reads.go:342. Both cut against the package's own file/helper split (U-S1, U-G1): the read helper the write path already depends on, getMergeRequest, lives in rest_reads.go (line 147) and is called from SubmitReview, and repeated endpoint construction is factored into a named method (c.notesURL, c.discussionsURL in discussions.go:169) rather than inlined at each call site.

Impact is small but it is the kind of drift that compounds: the approvals path/segment ordering now exists in two files and will need editing in both if the endpoint or project-segment handling changes, and reads/writes are no longer cleanly separated by file.

Suggested fix: add func (c *Client) approvalsURL(ref gitprovider.PRRef) string next to notesURL, use it from both ListReviews and the new helper, and move currentUserApproved into rest_reads.go alongside getMergeRequest (keeping the write-path decision — call it, and skip on true — in approve). No behavior change; the existing tests cover it.

The rest of the change reads well: the pre-check keeps approve's error mapping intact, the 401-vs-standing-approval ambiguity is documented at the call site, the extra /user read is gated on approvals being non-empty, and all three branches (standing approval, others' approvals, genuine 401 on the read) have direct tests.

Reply inline to this comment.

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)
Expand Down
98 changes: 90 additions & 8 deletions internal/gitprovider/gitlab/rest_writes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down
Loading