Adding agents MCP tools for GitLab#673
Conversation
avasconcelos114
added
2: Dev Review
📝 WalkthroughWalkthroughAdds an MCP server and HTTP /mcp routing, extends the GitLab interface, implements GitLab MCP API methods and handlers, registers MCP tools and schemas, updates generated mocks, and adds unit tests; also updates module dependencies in go.mod. ChangesMCP Server and GitLab Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/mcp_handlers.go (2)
302-324: ⚡ Quick winConsider populating the WebURL field in AddMergeRequestCommentOutput.
Similar to the issue comment handler, the
AddMergeRequestCommentOutputstruct defines aWebURLfield (line 164-167 in mcp_tools.go), but line 323 doesn't populate it from the returned Note.Proposed enhancement
- return nil, AddMergeRequestCommentOutput{NoteID: note.ID, Body: note.Body}, nil + return nil, AddMergeRequestCommentOutput{ + NoteID: note.ID, + Body: note.Body, + WebURL: note.NoteableWebURL, + }, nilNote: Verify the exact field name in
gitlab0.Note(might beNoteableWebURL,WebURL, or similar).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mcp_handlers.go` around lines 302 - 324, The AddMergeRequestCommentOutput returned by handleAddMergeRequestComment doesn't set WebURL; update handleAddMergeRequestComment to populate AddMergeRequestCommentOutput.WebURL from the note returned by GitlabClient.AddMergeRequestNote (e.g., map to the correct field on the returned note such as NoteableWebURL, WebURL, or equivalent), ensuring the output includes NoteID, Body and WebURL before returning.
154-176: ⚡ Quick winConsider populating the WebURL field in AddIssueCommentOutput.
The output struct (line 90-94 in mcp_tools.go) includes a
WebURLfield, but the handler doesn't populate it. Thegitlab0.Notereturned fromAddIssueNotelikely contains URL information that should be included in the response for better user experience.Proposed enhancement
- return nil, AddIssueCommentOutput{NoteID: note.ID, Body: note.Body}, nil + return nil, AddIssueCommentOutput{ + NoteID: note.ID, + Body: note.Body, + WebURL: note.NoteableWebURL, + }, nilNote: Verify the exact field name in
gitlab0.Note(might beNoteableWebURL,WebURL, or similar).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mcp_handlers.go` around lines 154 - 176, The handler handleAddIssueComment currently returns AddIssueCommentOutput without setting the WebURL; update it to extract the appropriate URL from the gitlab0.Note returned by GitlabClient.AddIssueNote (e.g., note.WebURL or note.NoteableWebURL — verify the exact field name on gitlab0.Note) and assign that value to AddIssueCommentOutput.WebURL before returning, so the response includes the note's web link.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/gitlab/mcp_api.go`:
- Around line 47-54: The code dereferences opts without nil checks when building
updateOpts (internGitlab.UpdateIssueOptions) and in the merge request creation
path; add a nil guard before accessing any opts fields (e.g., check if opts ==
nil and return an error or use default empty values) in the functions that build
updateOpts and the merge request creation routine so you avoid panics; locate
the builder that constructs updateOpts and the merge request creation code (the
scopes that reference opts, Title, Description, StateEvent, AssigneeIDs, Labels,
MilestoneID and the MR-equivalent fields) and either early-return an error when
opts is nil or initialize a safe zero-value struct and then reference its
fields.
- Line 39: Multiple methods in server/gitlab/mcp_api.go dereference the OAuth
token pointer (e.g., the call site client, err := g.GitlabConnect(*token>) )
without nil checks which can panic; update each method that uses the token
pointer (the occurrences at the shown call sites and similar lines) to first
check if token == nil and return a controlled authentication error (or nil
client + descriptive error) instead of dereferencing, and only call
GitlabConnect(*token) after the nil check; reference the token parameter in
these functions and the GitlabConnect call sites to locate and fix each
occurrence.
---
Nitpick comments:
In `@server/mcp_handlers.go`:
- Around line 302-324: The AddMergeRequestCommentOutput returned by
handleAddMergeRequestComment doesn't set WebURL; update
handleAddMergeRequestComment to populate AddMergeRequestCommentOutput.WebURL
from the note returned by GitlabClient.AddMergeRequestNote (e.g., map to the
correct field on the returned note such as NoteableWebURL, WebURL, or
equivalent), ensuring the output includes NoteID, Body and WebURL before
returning.
- Around line 154-176: The handler handleAddIssueComment currently returns
AddIssueCommentOutput without setting the WebURL; update it to extract the
appropriate URL from the gitlab0.Note returned by GitlabClient.AddIssueNote
(e.g., note.WebURL or note.NoteableWebURL — verify the exact field name on
gitlab0.Note) and assign that value to AddIssueCommentOutput.WebURL before
returning, so the response includes the note's web link.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b9314b6b-9d42-4a40-8944-ef703d97aff9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modserver/api.goserver/gitlab/gitlab.goserver/gitlab/mcp_api.goserver/gitlab/mocks/mock_gitlab.goserver/mcp.goserver/mcp_handlers.goserver/mcp_test.goserver/mcp_tools.goserver/mocks/mock_gitlab.goserver/plugin.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
| p.registerTools(s) | ||
| p.mcpServer = s | ||
|
|
||
| if err := s.Register(); err != nil { |
There was a problem hiding this comment.
Register() returns nil immediately and retries in a goroutine and this branch cant fire. Either delete it or replace the body with a comment :)
| return nil, nil, fmt.Errorf("GitLab account not connected: %s", apiErr.Message) | ||
| } | ||
|
|
||
| token, err := p.getOrRefreshTokenWithMutex(info) |
There was a problem hiding this comment.
This DMs the user when the refresh fails and with an LLM in the loop the agent will retry a tool call that just failed and we'll DM the same user a few times. Worth considering skip the DM when called from the MCP path. Not blocking.
| @@ -0,0 +1,432 @@ | |||
| // Copyright (c) 2019-present Mattermost, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
22 tools is too many. The pluginmcp README says that 10 is the budget per plugin because every tool's schema goes into the system prompt of every LLM call the agent makes. I see a few obvious merges to get us under but worth considering stripping more
1.list_my_assigned_issues and search_issues can become one list_issues with assigned_to_me and search fields.
2. list_my_assigned_merge_requests, list_my_review_requests and search_merge_requests can become list_merge_requests.
3. list_project_labels, list_project_milestones and list_project_members can be one get_project_metadata with a kind enum.
4. get_gitlab_dashboard I'd just drop
|
|
||
| func (p *Plugin) registerTools(s *pluginmcp.Server) { | ||
| // Issues | ||
| pluginmcp.AddTool(s, &mcp.Tool{ |
There was a problem hiding this comment.
No annotations on any of these tools. Without ReadOnlyHint or DestructiveHint the agents can't tell that get_issue is safe to auto-run. Read tools want ReadOnlyHint: true. The add_*_comment and create_* calls need DestructiveHint: ptr(false). run_pipeline I'd actually mark destructive since it can kick off CI that costs money
| return nil, GetIssueOutput{Issue: issueToSummary(issue.Issue)}, nil | ||
| } | ||
|
|
||
| func (p *Plugin) handleListMyAssignedIssues(ctx context.Context, _ *mcp.CallToolRequest, _ struct{}) (*mcp.CallToolResult, ListMyAssignedIssuesOutput, error) { |
There was a problem hiding this comment.
for the list endpoints (this one, handleListMyAssignedMRs, handleListMyReviewRequests, handleListMyProjects, handleGetMyTodos) we ship no pagination but the agent has no way to ask for more than GitLab's default page. Proposing to embed PaginationInput like ListProjectPipelinesInput does or document in the description that these are capped.
| return nil, ListMyAssignedIssuesOutput{}, err | ||
| } | ||
|
|
||
| client, err := p.GitlabClient.GitlabConnect(*token) |
There was a problem hiding this comment.
Every other handler hands the token to the gitlab package and lets it construct the client internally. These four reach in directly. A little concerned its the only place we leak the raw client out of the gitlab package.
| Labels: internGitlab.LabelOptions(in.Labels), | ||
| } | ||
|
|
||
| owner, repo := splitProjectPathParts(in.ProjectPath) |
There was a problem hiding this comment.
nit: Use splitProjectPath here so the agent gets the actionable "must be in namespace format" message before we go to GitLab. Same in handleGetProject.
| if baseURL == "" || projectPath == "" { | ||
| return "" | ||
| } | ||
| return fmt.Sprintf("%s/%s/-/%s/%d#note_%d", baseURL, projectPath, kind, parentIID, noteID) |
There was a problem hiding this comment.
If GitlabURL ends with a slash you get https://gitlab.com//ns/proj/ it returns the canonical URL on the note itself for newer gitlab versions. I would check whether note.URL or note.WebURL is populated and use that, or minimum strings.TrimRight(baseURL, "/") here.
|
|
||
| // --- Handler validation tests (mocked GitlabClient) ------------------------ | ||
|
|
||
| func newPluginWithMockGitlab(t *testing.T) (*Plugin, *mockgitlab.MockGitlab) { |
There was a problem hiding this comment.
Can we add end to end test that exercises pluginmcp.Server.ServeHTTP with a real tools payload? Validation tests are useful but we need the gitlab__ name prefix actually appearing on the wire and the schema we generate from our struct tags being valid and annotations once we add them.
| allowPrivate bool, | ||
| ) (namespace string, project string, err error) | ||
|
|
||
| UpdateIssue(ctx context.Context, user *UserInfo, token *oauth2.Token, projectID string, issueIID int, opts *UpdateIssueOptions) (*internGitlab.Issue, error) |
There was a problem hiding this comment.
Nit: I'd split out a GitlabMCP interface for the new ones.
| // Issues | ||
| pluginmcp.AddTool(s, &mcp.Tool{ | ||
| Name: "get_issue", | ||
| Description: "Retrieve details of a single GitLab issue by project path and issue IID (the number shown in the GitLab UI). Returns title, state, description, labels, assignees, milestone, and web URL.", |
There was a problem hiding this comment.
The descriptions are doing two things they shouldn't and missing one thing they should. First, and I also struggle with where to draw this line too tbh :D took me a minute to look it up. They restate the input schema "by project path and issue IID", "Use state_event 'close' or 'reopen'" and the output struct fields "Returns title, state, description, labels, assignees, milestone, and web URL". The JSON Schema auto generated from your structs already carries all of that ans the LLM gets it from there.
What's missing is sibling disambiguation. With 22 tools the LLM's actual problem is "when do I pick get_issue vs search_issues vs list_my_assigned_issues". That's what the description should answer.
Sources that i read but still struggle to separate it for myself :D
https://modelcontextprotocol.io/specification/2025-06-18/server/tools - description is for the LLM's understanding.
https://github.com/ProfessionalWiki/MediaWiki-MCP-Server/blob/master/docs/tool-conventions.md is verb phrase, what it returns, sibling routing, no schema duplication.
https://github.com/googleapis/mcp-toolbox/blob/main/docs/en/reference/style-guide.md which explicitly says don't put imperatives in descriptions ("IMPORTANT: you MUST…") because they're prompt-injection-shaped.
so this line will be something like: Description: "Returns a single issue's details. For keyword lookup use search_issues; for the calling user's open issues use list_my_assigned_issues.",
nang2049
left a comment
nang2049
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114 mostly it looks great! As I worked with a few MCP's and still struggle to define best practices for registration, I added some comments that I have seen along the way for 'proper' use. Feel free to connect if you want to aling on the approach 🙏
|
@nang2049 First of all thank you for the tremendous review, I haven't worked much with MCP's so your review is pure gold, I'll get to addressing each of the points and might reply in specific threads if anything isn't clear to me, many thanks again! |
2 participants
Summary
This PR registers the GitLab plugin as an MCP server with the Mattermost Agents plugin, exposing 22 tools that let AI agents read and act on GitLab data on behalf of the calling user (issues, merge requests, projects, pipelines, todos, labels, milestones, members).
All tool calls execute against the user's own OAuth token and respect the plugin's
GitlabGroupnamespace restriction. The/mcpendpoint is gated bypluginmcpto inter-plugin RPC only.QA Notes
Please note that in order for this to be testable, you'll also need to have an agents plugin with MCP plugin registration supported, which hasn't yet been released (building from the master branch is recommended)
Once you have the agents plugin setup you should be able to see the MCP tools appearing in the list, after that you may prompt Matty to:
Ticket Link
Change Impact: 🟠 Medium
Reasoning: Adds an MCP server surface and 22 MCP tools that enable inter-plugin RPC using callers' OAuth tokens, extends the GitLab client interface, and updates mocks and tests—significant new capability that is mostly localized to the plugin but touches auth and inter-plugin surfaces.
Regression Risk: Moderate — touches authentication/authorization flows (caller resolution, token refresh), registers a new /mcp HTTP endpoint gated by pluginmcp permission, expands public interfaces and generated mocks, and introduces substantial new handler logic; changes are mostly additive and include unit tests, but the inter-plugin and token-scoped nature increases risk.
** QA Recommendation:** Manual QA recommended: build and install an Agents plugin supporting MCP registration, verify MCP tools appear and are permission-gated, test representative flows (list/create/update issues & MRs, add comments, list/trigger pipelines, fetch todos/dashboard), and confirm correct caller resolution and token usage. Skipping manual QA is not advised given the authentication and inter-plugin surface.
Generated by CodeRabbitAI