Add approve and unapprove methods to save_merge_request_review MCP tool

Tian Gaorequested to merge
617967-add-approve-unapprove-methods into master

What does this MR do and why?

Adds approve and unapprove values to the method enum of the save_merge_request_review MCP tool. This closes the Red Hat gap-analysis blocker where agents could not approve merge requests, and satisfies the Anthropic integration's hard requirement for unapprove support.

Both are Ruby-side dispatch branches following the existing post_duo_review precedent: approve calls MergeRequests::ApprovalService, unapprove calls MergeRequests::RemoveApprovalService — the same services backing the REST POST .../approve and POST .../unapprove endpoints.

This is a purely additive schema change: two enum values plus one optional sha parameter. Existing calls to save_merge_request_review are untouched.

Closes #617967 (closed)

Design decisions
  • Idempotent statuses instead of REST's errors: approving an already-approved MR returns success with status already_approved; unapproving without a prior approval returns success with status not_approved. REST returns 401 for both cases, indistinguishable from a permission failure. For unattended agent retries, explicit-state success is safer. This is a deliberate divergence from REST behavior.
  • sha guard: an optional sha parameter on approve mirrors the REST staleness guard (compares against diff_head_sha, with the identical error message "SHA does not match HEAD of source branch: "). Silently approving stale content is a worse failure for an agent than for a human. The tool description recommends passing the diff_head_sha obtained from get_merge_request.
  • Outcome verified from state, not the service return: ApprovalService returns nil on every failure indistinguishably, and can even report success when the row save races. The tool re-checks approved_by? after the call and derives a typed error (merged / not permitted / instance approval settings) locally rather than trusting the service's return value.
  • unapprove permission gate: RemoveApprovalService performs no permission check itself; the REST endpoint authorizes :approve_merge_request before calling it, and the tool replicates that same gate.
  • EE approval settings not supported: instance settings that require a password or a SAML re-authentication within 5 seconds to approve cannot be satisfied by a PAT-driven MCP call. Those rejections surface as a typed error explaining the limitation. No approval_password parameter is exposed.

How to validate locally

  1. gdk restart rails-web on this branch (run rails db:migrate first if pending).
  2. Call the MCP endpoint with a PAT (mcp scope): tools/list shows the 8-value method enum and the sha parameter.
  3. tools/call save_merge_request_review with method: approve / unapprove against a merge request you can approve; verify the approval appears/disappears on the MR page.
GDK JSON-RPC round trip 
1a. enum: ["create_note", "reply_discussion", "create_diff_note", "resolve_discussion", "submit_review", "post_duo_review", "approve", "unapprove"]
1b. sha param: Head SHA guard (approve). When given and it no longer matches the merg
MR: jashkenas/Underscore iid=2 author=sherril head=826ba14c
2. approve:           isError=false {"method"=>"approve", "status"=>"approved", "merge_request_url"=>"https://gdk.test:3443/jashkenas/Underscore/-/merge_requests/2"}
   db approved_by?:   true
3. approve again:     isError=false {"method"=>"approve", "status"=>"already_approved", ...}
4. unapprove:         isError=false {"method"=>"unapprove", "status"=>"unapproved", ...}
   db approved_by?:   false
5. unapprove again:   isError=false {"method"=>"unapprove", "status"=>"not_approved", ...}
6. approve good sha:  isError=false {"method"=>"approve", "status"=>"approved", ...}
7. approve bad sha:   isError=true "Validation error: SHA does not match HEAD of source branch: 826ba14cfc12f8a31e0d4f3b3e98737bc922cce8"
cleanup done; final approved_by?: false

The non-member permission rejection is covered by unit specs; it cannot be driven end-to-end on GDK because the MCP endpoint itself returns 404 for users without Duo availability (a pre-existing endpoint gate, unrelated to this change).

References

  • Issue: #617967 (closed)
  • Tool's original MR: !249708 (merged)
  • REST endpoints: lib/api/merge_request_approvals.rb
  • Per the issue's implementation plan, the MCP Tool Proposal for these methods runs in parallel with this MR.

Merge request reports