Adding agents MCP tools for GitLab - #673
Conversation
avasconcelos114
added
2: Dev Review
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MCP routing and lifecycle, extends GitLab contracts and MCP methods, defines MCP schemas and handlers, updates mocks and tests, and refreshes module dependencies. 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)
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! |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/plugin_test.go (1)
409-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease add coverage for
notifyOnRevoke=false.This update only fixes the call site for the new parameter; it does not exercise the new no-DM path. A small test that proves revoke/disconnect still happens while notification is suppressed would lock in the spam-prevention behavior.
🤖 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/plugin_test.go` around lines 409 - 435, The current test coverage exercises token refresh failure but does not verify the new notifyOnRevoke=false behavior. Add a focused test around the revoke/disconnect flow in Plugin.refreshToken or the revoke helper that passes notifyOnRevoke=false, asserts the revoke/disconnect still occurs, and verifies no direct message/notification is sent. Use the existing TestRefreshTokenReturnsErrorWhenOAuthConfigFails pattern and the relevant Plugin methods to keep the new no-DM path covered.
🤖 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/mcp_handlers.go`:
- Around line 231-239: The selector handling in mcp_handlers.go silently ignores
conflicting merge-request filters in the request flow around the mrs switch.
Update the logic in the handler that calls GitlabClient.SearchMergeRequests,
ListReviewRequests, and ListAssignedMergeRequests to validate combinations of
search, assigned_to_me, and review_requested up front, and return a validation
error when more than one selector is set unless you explicitly document and
enforce a single precedence rule. Make the behavior deterministic so callers
cannot get the wrong result set from mixed selectors.
In `@server/mcp_test.go`:
- Around line 211-212: The MCP tool-count assertion in the test is using a stale
budget cap that no longer matches the contract exposed by this PR. Update the
check in the test around the `res.Tools` assertion to validate the intended
22-tool registration, or assert the expected tool names via the MCP response
instead of `assert.LessOrEqual`. Use the existing test flow that inspects
`res.Tools` so the expectation matches the full set of registered tools.
In `@server/mcp_tools.go`:
- Around line 175-176: The MCP hint text on ProjectMemberSummary and the related
merge-request tool descriptions still mention stale merge-request-write concepts
such as reviewer_ids and create_merge_request, which are no longer available in
the current tool set. Update the jsonschema hint strings in these MCP types and
any related descriptions around ProjectMemberSummary so they only reference
supported workflows and parameters that exist in the current 10-tool surface,
keeping the guidance focused on valid assignee/review selection behavior.
- Around line 207-208: The update_issue tool is incorrectly marked as
non-destructive via the ToolAnnotations setup, which can make it eligible for
unsafe auto-run behavior. Update the annotation used for update_issue in
mcp_tools.go so it has its own destructive or unknown classification instead of
reusing DestructiveHint: false, and keep the change localized to the tool
registration/annotation logic around the ToolAnnotations value.
---
Nitpick comments:
In `@server/plugin_test.go`:
- Around line 409-435: The current test coverage exercises token refresh failure
but does not verify the new notifyOnRevoke=false behavior. Add a focused test
around the revoke/disconnect flow in Plugin.refreshToken or the revoke helper
that passes notifyOnRevoke=false, asserts the revoke/disconnect still occurs,
and verifies no direct message/notification is sent. Use the existing
TestRefreshTokenReturnsErrorWhenOAuthConfigFails pattern and the relevant Plugin
methods to keep the new no-DM path covered.
🪄 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: 07221ffb-773d-48be-a2b7-a8e5ba0a50f1
📒 Files selected for processing (11)
go.modserver/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.goserver/plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- server/mcp.go
# Conflicts: # go.mod # go.sum
ogi-m
added
4: Reviews Complete
# Conflicts: # go.mod # go.sum
3 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: This adds an MCP server integration plus a new inter-plugin
/mcpHTTP surface with ~10–22 GitLab-backed tools, including caller resolution and OAuth token handling. While the changes are largely additive and covered by an expanded unit/E2E test suite, they touch authentication/authorization and inter-plugin request routing.Regression Risk: Moderate — new MCP lifecycle/endpoint registration, permission gating (
pluginmcp), and token refresh/revocation notification behavior can affect critical flows, though most logic is tool-scoped and tests cover initialization, authorization/caller resolution, and handler validation.QA Recommendation: Manual QA recommended: install a compatible Agents build, verify MCP tools appear and are correctly permission-gated, and exercise representative flows (list/create/update issues & merge requests, add comments, fetch projects/metadata, and at least one pipeline/todo-related read/write if implemented) to confirm correct caller identity and OAuth-scoped GitLab access.
Generated by CodeRabbitAI