fix(gitprovider): skip GitLab approve when the caller's approval already stands - #564
Conversation
…ady 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.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f4bdd1f7e8c5
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| security:code-auditor | 0 |
| architecture:solid-reviewer-agnostic | 1 |
architecture:solid-reviewer-agnostic (1 finding)
Minor - internal/gitprovider/gitlab/rest_writes.go:118
currentUserApprovedis a read-only helper (two GETs, no writes) placed inrest_writes.go, and it rebuilds the approvals endpoint URL thatListReviewsalready builds verbatim atinternal/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 inrest_reads.go(line 147) and is called fromSubmitReview, and repeated endpoint construction is factored into a named method (c.notesURL,c.discussionsURLindiscussions.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) stringnext tonotesURL, use it from bothListReviewsand the new helper, and movecurrentUserApprovedintorest_reads.goalongsidegetMergeRequest(keeping the write-path decision — call it, and skip ontrue— inapprove). 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/userread is gated on approvals being non-empty, and all three branches (standing approval, others' approvals, genuine 401 on the read) have direct tests.
Reviewer Coverage
go:implementation-tests— complete (broad); skipped: none; constraints: nonesecurity:code-auditor— complete (broad); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (broad); inspected 2 assigned files (3 inspected across reviewers):internal/gitprovider/gitlab/rest_reads.go,internal/gitprovider/gitlab/rest_writes.go; skipped: none; constraints: Rango test ./internal/gitprovider/gitlab/: all SubmitReview tests pass. TestProductionImportsStayInAdapterLayer fails, but only because the workbench checkout path contains spaces and the test splitsgo listoutput on whitespace — environmental, not diff-caused. Scope limited to the two assigned files; internal/gitprovider/gitlab/rest_writes_test.go was read as evidence for U-T1 but not reviewed as an assigned file.
Inspected files (3)
internal/gitprovider/gitlab/rest_reads.gointernal/gitprovider/gitlab/rest_writes.gointernal/gitprovider/gitlab/rest_writes_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 4m 42s | ~$3.40 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.285
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, security:code-auditor, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 4m 42s wall · 7m 19s compute |
| Cost | ~$3.40 (est.) |
| Tokens | 90 in / 22.3k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 4
- Out: 2.8k
- Cache read: 31.2k
- Cache create: 74.1k
- Cost: ~$0.33 (est.)
- Duration: 44s
go:implementation-tests— claude-sonnet-5- In: 26
- Out: 5.3k
- Cache read: 898.7k
- Cache create: 91.0k
- Cost: ~$0.69 (est.)
- Duration: 1m 41s
security:code-auditor— claude-sonnet-5- In: 24
- Out: 2.8k
- Cache read: 747.6k
- Cache create: 82.9k
- Cost: ~$0.58 (est.)
- Duration: 1m 06s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 30
- Out: 10.8k
- Cache read: 1.0M
- Cache create: 98.9k
- Cost: ~$1.41 (est.)
- Duration: 3m 32s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 474
- Cache read: 104.5k
- Cache create: 94.2k
- Cost: ~$0.39 (est.)
- Duration: 15s
| // 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) { |
There was a problem hiding this comment.
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.
2 participants
Why
GitLab's
POST /projects/:id/merge_requests/:iid/approveresponds 401 Unauthorized when the caller already holds a standing approval on the merge request. The GitLab provider mapped that 401 through the approve-specificErrAuth -> ErrPermissionpath, so a rerun of an already-approved MR failed the whole run:Outcome: failed, exit 3,posted=0 failed_terminal=1— even though the review itself completed and the approval already stood. The message sends the operator down a credential-debugging path for a token that is fine.What
SubmitReview's approve path now checks whether the current user's approval already stands before writing, and treats a standing approval as already satisfied:GET .../merge_requests/:iid/approvals— the same endpointListReviewsalready consumes; ifapproved_byis empty the approve proceeds with no further reads.GET /userresolves the authenticated user; a match inapproved_byskips the approve write and the summary note posts as usual.A pre-check was chosen over tolerating 401 on the approve write because that status is genuinely ambiguous on this endpoint (GitLab also uses it for approval-permission restrictions), and blanket tolerance would mask real failures. Genuine credential 401s still surface as
ErrAuth: every read in the same submission (merge request, approvals,/user) fails loudly with a real bad token, and the approve write's existing 401 ->ErrPermissionmapping is unchanged for callers who are not allowed to approve.Tests
TestSubmitReviewApproveSkipsApproveWhenApprovalAlreadyStands— standing approval by the current user: no approve call (the fake would 401 it), summary note still posts, run succeeds.TestSubmitReviewApproveStillApprovesWhenOnlyOthersApproved— approvals by other users do not suppress the approve write.TestSubmitReviewSurfacesAuthFailureFromApprovalsRead— a genuine 401 on a read still surfaces asErrAuthwith no writes attempted.make checkgreen locally (lint 0 issues; the one failing test,llmadapters/TestPiRPCReviewerExtensionLoadsInInstalledPi, fails identically on cleanmain— it shells out to the locally installedpi, whose version predates--no-builtin-tools).Fixes #563