Attribute approve_merge_request SLI errors from the service result

The approve_merge_request UX SLI shows a roughly 53% error rate on GitLab.com, but most of that is repeat approvals and approvals of already-merged MRs being recorded as failures, because MergeRequests::ApprovalService#execute only ever returned nil or a bare success hash. This gives the service a ServiceResponse with a domain-specific reason:, updates all five call sites to branch on .success? instead of each inventing its own workaround, and has the REST approve endpoint complete handled refusals as non-errors with the reason recorded as a log field, following the ui_button_rebase precedent. HTTP responses are unchanged.

Detailed context for AI agents

Problem

• The approve_merge_request User Experience SLI (Labkit UX SLI) reports roughly a 53% error rate on GitLab.com. Every error carries the same message, "Merge request was not approved".
• Cause: MergeRequests::ApprovalService#execute collapsed at least five different outcomes into either a bare nil or a legacy success hash. The REST approve endpoint recorded any falsey return as a failed user experience.
• Most of that 53% is not a failure. A repeat approve by the same user, and an approve on an already-merged merge request, are idempotent no-ops that callers make freely, but they returned the same nil as a genuine refusal.
• The undifferentiated error message means nobody can break the 53% down in Kibana to see which branch dominates.
• Because the service told callers nothing, all five call sites had invented their own workaround: the MCP tool re-read merge_request.reset.approved_by? after the call (with a code comment saying the return value was untrustworthy), quick actions and the SAML approvals controller fell back to a generic failure message, the drafts controller discarded the result entirely, and the REST endpoint mapped truthiness to a 401.
• Related side effect of the old contract: return success unless save_approval(approval) reported success when the row was never created by this request.

What changed

• MergeRequests::ApprovalService#execute now returns a ServiceResponse with a domain-specific reason:. Reasons: :already_approved, :not_eligible, :merge_request_merged, and in EE :incorrect_approval_password and :saml_reauthentication_required.
• All five call sites now branch on .success?. The MCP tool's post-hoc merge_request.reset.approved_by? re-check is deleted.
• The REST approve endpoint (lib/api/merge_request_approvals.rb) completes every handled outcome without an SLI error and records it as approval_result (:success, the service reason, :refused for an error without a reason, :publish_review_failed, or :error when an exception aborted the request). Only a failed draft-note publish and unexpected exceptions call error!.
• HTTP responses are unchanged. A repeat approve still returns 401, a successful approve still returns 201.

Why handled outcomes complete without an SLI error

• Policy set by observability on labkit-ruby !310 (merged) and applied in !244312 (merged) (ui_button_rebase): a UX SLI error means the system failed, not that the user was refused. A handled refusal completes as a non-error and the business outcome goes into a custom log field, since metric labels are capped for cardinality.
• Labkit's Experience#complete has no third state: an experience is either an error or an apdex sample. So repeat approvals now feed apdex as fast successes. That is the accepted trade-off on the rebase SLI too; #628081 (closed) can filter them out in Kibana via approval_result when calibrating.
• An earlier revision of this MR excluded the no-ops entirely with Labkit::UserExperienceSli.observed. That was dropped in review: it diverged from the rebase precedent, lost the notes_published checkpoint, and needed a hand-rolled rescue to see exceptions.
• Grape's unauthorized! and render_api_error! exit with throw, which the block form cannot see, so the endpoint uses the explicit start / complete form with ensure, like RebaseWorker. experience is nil if the lookup raised before start, so a 404 records nothing.

Findings left out of scope (not addressed here)

• :saml_reauthentication_required looks unreachable via the group SAML path. EE::ProjectPolicy's needs_new_sso_session condition prevents :read_project when SSO access is restricted, so the eligibility check fails first and the caller gets :not_eligible instead. The service spec now asserts the reason by stubbing the SSO state out of time (a sessionless state counts as in time, which is why the pre-existing not-in-time example never reached this branch).
• app/controllers/projects/merge_requests/drafts_controller.rb still discards the approval result inside the submit_mr_review_ui SLI, so a failed approve there is still recorded as a successful review submission. Same bug class, different SLI, worth a separate change.
• The raced-save path still returns success, preserving today's 201. Making it consistent with the sequential repeat approve (which returns 401) is an API semantics question, not an observability one.
• Four other UX SLIs do no error attribution at all and still carry the same false-success behaviour: immediate_web_merge in app/controllers/projects/merge_requests_controller.rb, create_merge_request on three legs (app/controllers/projects/issues_controller.rb, lib/api/merge_requests.rb, app/graphql/mutations/merge_requests/create.rb), and load_comments.

Verification

• Green locally: spec/requests/api/merge_request_approvals_spec.rb (61 examples), ee/spec/services/ee/merge_requests/approval_service_spec.rb, spec/services/merge_requests/approval_service_spec.rb, ee/spec/requests/api/merge_request_approvals_spec.rb, plus the drafts controller, MCP tool and quick action specs. RuboCop clean.
• Manually on GDK against the REST endpoint: plain approve and approve with publish_review=true complete the experience; repeat approve returns 401; wrong sha (409) and unknown MR (404) record nothing.
• Query count on the happy path was measured against origin/master: approved_by? is called 3 times both before and after, so the new call adds nothing to the happy path.
• NOT run locally: ee/spec/features/merge_request/user_approves_with_saml_auth_spec.rb. CI covers it.

References

• Issue that prompted this: #628081 (closed)
• The merge request that introduced the SLI: !252318 (merged)
• Blocked SLO change the issue depends on: gitlab-com/gl-infra/observability/team#4467