<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Work on this issue](https://contributors.gitlab.com/manage-issue?action=work&projectId=278964&issueIid=606934) - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=606934) </details> <!--IssueSummary end--> ## Problem `list_merge_requests` is project-only, so an agent has to already know which project to look in. The questions that motivated the tool aren't project-scoped — from gitlab-org/gitlab#605879: > Agents and users need to find merge requests ... (e.g. **"my open MRs"**, **"MRs awaiting my review"**) ## What's needed Add `group_id` beside `url` and `project_id`, keeping the "exactly one of" rule. Three things make it more than a parameter: 1. **Subgroups must always be included, and must not be a parameter.** The resolver defaults them off, so a group query silently returns nothing. 2. **Group results need a project path.** Without one they can't be chained into `get_merge_request`. 3. **The query has to branch on project-vs-group**, which makes `operation_name` dynamic. This was built and verified in gitlab-org/gitlab!246413, then cut — that MR's issue is explicitly project-only. Everything needed to redo it is below. ## Non-goals - Changing project-scoped behaviour. - Exposing `include_subgroups` (see below). - `include_archived`, also defaulted `false` by `GroupIssuableResolver`. Probably the right default for an agent, but unexamined. <details><summary>1. Why subgroups must be unconditional</summary> `Resolvers::GroupMergeRequestsResolver` includes `GroupIssuableResolver`: ```ruby argument :include_subgroups, GraphQL::Types::Boolean, required: false, default_value: false, description: "Include #{issuable_collection_name} belonging to subgroups" ``` Without passing it, a group query covers only projects **directly** in the group. For `gitlab-org` that is close to nothing. Don't expose it as a tool parameter: - **The product answer is always "yes".** `app/controllers/concerns/issuable_collections.rb:68-69` hardcodes it for the group merge request list UI: ```ruby options[:group_id] = @group.id options[:include_subgroups] = true ``` The GraphQL `false` default is an API artifact, not what "merge requests in a group" means to anyone using GitLab. - **The wrong branch fails silently** — empty list, no error. An agent will retry, fabricate, or report "you have no merge requests". !246413 exposed it in an earlier revision and reverted for these reasons. Measured over real `POST /api/v4/mcp` calls against GDK, on seeded group `top-level-public` whose only merge request lives in a sub-subgroup: ``` direct MRs: 0 subgroup MRs: 1 include_subgroups=omitted (default) -> 0 node(s) include_subgroups=false -> 0 node(s) include_subgroups=true -> 1 node(s) ``` Finding equivalent fixtures: ```ruby Group.find_each do |g| next if g.projects.empty? || g.descendants.empty? nested = Project.where(namespace_id: g.descendants.pluck(:id)).pluck(:id) next if nested.empty? nested_mrs = MergeRequest.where(target_project_id: nested).count next unless nested_mrs.positive? puts "#{g.full_path} direct=#{MergeRequest.where(target_project_id: g.projects.pluck(:id)).count} nested=#{nested_mrs}" end ``` </details> <details><summary>2. Project path on group results — options and measured costs</summary> Nodes return `iid` but no project. Fine for project scope; a group query returns `iid`s from many projects, and `get_merge_request` needs `id` (project) **and** `merge_request_iid`. Parsing `webUrl` is unreliable under a relative URL root (self-managed at `https://host/gitlab/...`) — there's no way to tell where the instance prefix ends and the project path begins. `Mcp::Tools::Base::ApiTool#execute` merges args into `GRAPE_ROUTING_ARGS` rather than interpolating a URL path: ```ruby args = params[:arguments]&.slice(*settings[:params]) || {} request.env[Grape::Env::GRAPE_ROUTING_ARGS].merge!(args) ``` So `get_merge_request`'s `id` takes a plain full path with literal slashes — no URL-encoding. `project.fullPath` is directly copyable. Measured on a real 13-node response (358 chars/node): | Option | Added | Per node | Agent has to… | |---|---|---|---| | **A.** `project { id fullPath }` always | +1,066 (+22.9%) | +82 | copy `fullPath` → `id` | | **B.** Same, `@skip(if: $isProject)` | group only, 0 for project | +82 / 0 | copy `fullPath` → `id` | | **C.** `reference(full: true)` | +563 (+12.1%) | +43 | split on `!` | | **D.** Document `webUrl` | 0 | 0 | parse a URL (unreliable) | ~30 of A/B's 82 chars is a forced `gid://gitlab/Project/N` — `@graphql-eslint/require-selections` mandates `id` on nested object types. Lint tax, not payload. **B** was the leading candidate: mirrors the `$isProject` branch the query already needs, free on project-scoped calls. Downside is a response shape that varies by scope. </details> <details><summary>3. Working GraphQL query (passed all_queries_spec)</summary> Every object type selects `id` — required by `@graphql-eslint/require-selections`, which applies to `**/*.graphql` and is **not** caught by rspec, only the eslint CI job. ```graphql # @feature_category: mcp_server query listMergeRequests( $fullPath: ID! $isProject: Boolean = false $authorUsername: String $assigneeUsername: String $reviewerUsername: String $state: MergeRequestState $milestoneTitle: String $labelName: [String] $search: String $first: Int $after: String ) { project(fullPath: $fullPath) @include(if: $isProject) { id mergeRequests( authorUsername: $authorUsername assigneeUsername: $assigneeUsername reviewerUsername: $reviewerUsername state: $state milestoneTitle: $milestoneTitle labelName: $labelName search: $search first: $first after: $after ) { ...mergeRequestListFields } } group(fullPath: $fullPath) @skip(if: $isProject) { id mergeRequests( includeSubgroups: true authorUsername: $authorUsername assigneeUsername: $assigneeUsername reviewerUsername: $reviewerUsername state: $state milestoneTitle: $milestoneTitle labelName: $labelName search: $search first: $first after: $after ) { ...mergeRequestListFields } } } fragment mergeRequestListFields on MergeRequestConnection { pageInfo { hasNextPage endCursor } nodes { id iid title state webUrl sourceBranch targetBranch author { id username } } } ``` </details> <details><summary>Tool changes: operation_name, parent resolution, schema</summary> `operation_name` must become dynamic — `process_result` digs `result.dig('data', operation_name)`. Drop the static `operation_name: 'project'` from `register_version`: ```ruby def operation_name resolved_parent[:type] == :project ? 'project' : 'group' end ``` Parent resolution reusing the `UrlParser` / `ResourceFinder` concerns — `resolve_parent_from_url` already does parse → find → `authorize_parent_access!` and returns `{type:, full_path:, record:}`: ```ruby PARENT_PARAMS = %i[url project_id group_id].freeze def resolved_parent @resolved_parent ||= resolve_parent end def resolve_parent provided = PARENT_PARAMS.select { |key| params[key].present? } raise ArgumentError, 'Provide exactly one of: url, project_id, or group_id' unless provided.one? case provided.first when :url then resolve_parent_from_url(params[:url]) when :project_id then resolve_parent_by_id(:project, params[:project_id]) when :group_id then resolve_parent_by_id(:group, params[:group_id]) end end def resolve_parent_by_id(type, identifier) parent = find_parent_by_id_or_path!(type, identifier) { type: type, full_path: parent.full_path, record: parent } end ``` `build_variables` gains `isProject: resolved_parent[:type] == :project`. Schema — don't repeat "Provide exactly one of…", it lives once in the tool description: ```ruby group_id: { type: 'string', description: 'ID or full path of the group. Covers every project in the group, ' \ 'including its subgroups.' }, ``` ```ruby def resource_not_found_error resource_type = resolved_parent[:type].to_s.capitalize ::Mcp::Tools::Base::Response.error( "#{resource_type} not found: it does not exist or you do not have access to it." ) end ``` </details> <details><summary>Specs, and two traps that cost time</summary> ```ruby describe 'group scope' do let(:params) { { group_id: group.id.to_s } } it 'covers projects in the group and in its subgroups', :aggregate_failures do urls = result_urls(tool.execute) expect(urls).to include(mr_url(mr_by_user)) expect(urls).to include(mr_url(subgroup_mr)) end end context 'when a group holds a project the caller cannot read' do let_it_be(:mixed_group) { create(:group, :public) } let_it_be(:readable_project) { create(:project, :public, group: mixed_group) } let_it_be(:unreadable_project) { create(:project, :private, group: mixed_group) } let(:tool) { described_class.new(current_user: non_member, params: params) } it 'omits the unreadable merge request', :aggregate_failures do urls = result_urls(tool.execute) expect(urls).to include(mr_url(readable_mr)) expect(urls).not_to include(mr_url(unreadable_mr)) end end ``` **`iid` is not unique across projects.** It restarts at 1 per project, so asserting on `iid` in a group query gives false passes. Compare web URLs: ```ruby def result_urls(result) result[:structuredContent]['nodes'].map { |node| node['webUrl'] } end def mr_url(merge_request) Gitlab::UrlBuilder.build(merge_request) # MergeRequest has no #web_url end ``` **A public project cannot live in a private group.** `create(:project, :public, group: private_group)` fails with a misleading `ActiveRecord::RecordNotSaved: You cannot call create unless the parent is saved` from the factory's `create_ci_project_mirror!` callback — the project itself is invalid. Use a **public group with a private project** for the "caller cannot read one project" case. </details> <details><summary>Manual MCP verification recipe</summary> Don't rely on specs alone. Confirm over real calls that a group scope returns subgroup merge requests, and that a non-member gets nothing from a private project in a public group. 1. `gdk restart rails-web` so tool changes register. 2. PAT with a known value: ```ruby tok = User.find_by_username('root').personal_access_tokens.create!( name: 'mcp-verify', scopes: [:api, :mcp], expires_at: 7.days.from_now) tok.set_token('someknownvalue'); tok.save! # tok.revoke! when done ``` 3. Drive HTTP from `bundle exec rails runner` — gives DB access for fixtures in the same script. Puma binds a **UNIX socket** (`config/puma.rb`), so not `127.0.0.1:3000`; go through nginx: ```ruby BASE = URI('https://gdk.test:3443/api/v4/mcp') opts = { use_ssl: true, verify_mode: OpenSSL::SSL::VERIFY_NONE, read_timeout: 120 } res = Net::HTTP.start(BASE.hostname, BASE.port, **opts) { |h| h.request(req) } ``` </details> <details><summary>Why it was cut from !246413</summary> gitlab-org/gitlab#605879 is project-only: schema has `project_id`/`url` only, the example description says "in a **project**", the implementation plan wraps `GET /api/v4/projects/{id}/merge_requests`, and it states "Follow-ups: none". The tool being replaced, `gitlab_merge_request_search`, is also project-only — its input derives from `ProjectResourceInput`. An MCP input schema is versioned (`doc/development/duo_agent_platform/mcp/_index.md:358`), so adding a parameter later is additive and safe, while removing one is a breaking change needing `register_version '0.2.0'` plus multi-version support. Shipping `group_id` speculatively was the expensive direction. </details>