## Why `Mcp::Tools` is currently organized on two competing axes at once, applied inconsistently: - **Domain** — `work_items/`, `labels/`, `merge_requests/` - **Implementation kind** — custom / REST (`api_*`) / graphql / aggregated This leads to inconsistent locations for related tools: - **Same-domain tools are split by kind.** `get_merge_request_conflicts_service.rb` (custom) sits flat at the top level, while `merge_requests/get_merge_request_notes_*` (graphql) lives in a domain folder. Same domain, two homes. - **Aggregated tools sit flat at the top level.** `SearchService` and `PipelineService` (`< AggregatedService`) live at the root next to base classes. - **Base/framework classes are interleaved with concrete tools.** `base_service`, `graphql_service`, `graphql_tool`, `api_tool`, `aggregated_service` sit beside `get_server_version_service` and `get_merge_request_conflicts_service`. - **The `Graphql` prefix on Service classes carries no signal.** There is no `graphql` folder, the paired Tool classes are unprefixed (`GetWorkItemNotesTool`), and each class already inherits from `GraphqlService`. The new `get_merge_request_notes` tool already ships unprefixed (`GetMergeRequestNotesService`); this converges the rest. ## Guiding principle - **Folder = domain** (what the tool is about) - **Base class = kind** (how it's implemented: `CustomService` / `GraphqlService` / `AggregatedService` / `ApiTool`) - **`manager.rb` registry = dispatch** (`CUSTOM_TOOLS` / `GRAPHQL_TOOLS` / discovered API / aggregated) Kind is already expressed by the base class **and** the registry bucket — it does not need a third expression as a folder. Domain is the axis currently expressed nowhere consistently; that is the gap to close. ## Proposed structure ``` tools/ . ← get_server_version_service (any non-domain specific service/tool) base/ ← BaseService, CustomService, GraphqlService, GraphqlTool, ApiService, ApiTool, AggregatedService, response, version_helper, icon_config concerns/ manager.rb ← the one place kind/dispatch lives merge_requests/ ← get_merge_request_notes_{tool,service}, get_merge_request_conflicts_service work_items/ ← the 5 existing graphql tools (+ paired services) labels/ ← search_tool + service search/ ← search_service (aggregated; domain = search) pipelines/ ← pipeline_service (aggregated; domain = pipelines) ``` Notes: - **API tools do not move** — they are discovered from `route_setting :mcp` in `lib/api/**` and wrapped by the generic `ApiTool`. There are no per-tool files for them here. - **Aggregated tools group by domain** (`search/`, `pipelines/`), not into an `aggregated/` bucket — the aggregated-ness is an implementation strategy, not a domain. - **Consistent foldering, including domains-of-one** (`search/`). The alternative (singletons flat) is the inconsistency this issue removes. - Non-domain tools (MCP server only, not related to GitLab data) remain in `tools/` folder ### How to identify a GraphQL tool after the reorg So the dropped prefix does not hurt discoverability: 1. **File pairing** — GraphQL tools are the only kind with both a `*_tool.rb` (`< GraphqlTool`) and a `*_service.rb` (`< GraphqlService`). A lone `*_service.rb` is custom or aggregated. 2. **Base class** — explicit `< GraphqlService` / `< GraphqlTool` in every file. 3. **`GRAPHQL_TOOLS`** in `manager.rb` is the canonical index. ## Scope 1. Introduce `tools/base/` and move the framework classes into it (update namespaces to match — Zeitwerk requires file path == module path). 2. Move every concrete tool into a domain folder per the tree above. 3. Drop the redundant `Graphql` prefix from the six Service classes (class + file + `manager.rb` registration value + EE overrides + spec files / `describe` blocks). **Keep the agent-facing `tool_name` keys unchanged** (contracts are append-only): - `Mcp::Tools::WorkItems::GraphqlCreateWorkItemNoteService` → `CreateWorkItemNoteService` - `Mcp::Tools::WorkItems::GraphqlGetSavedViewWorkItemsService` → `GetSavedViewWorkItemsService` - `Mcp::Tools::WorkItems::GraphqlGetWorkItemNotesService` → `GetWorkItemNotesService` - `Mcp::Tools::WorkItems::GraphqlGetWorkItemTypesService` → `GetWorkItemTypesService` - `Mcp::Tools::WorkItems::GraphqlLinkWorkItemsService` → `LinkWorkItemsService` - `Mcp::Tools::Labels::GraphqlSearchService` → `Mcp::Tools::Labels::SearchService` (parallels the existing `Labels::SearchTool`; distinct from the aggregated `Search::SearchService`) 4. Update the developer docs to match the new layout and naming: - `doc/development/duo_agent_platform/mcp/graphql_integration.md` — the hardcoded file paths (`app/services/mcp/tools/graphql_tool.rb`, `graphql_service.rb`) move under `base/`, and the worked example (`GraphqlCreateIssueService` / `app/services/mcp/tools/graphql_create_issue_service.rb`) must drop the prefix and reflect a domain folder. - `doc/development/duo_agent_platform/mcp/_index.md` — update any `manager.rb` / path references. ## Watch out for - **Zeitwerk** — every move must update the module nesting to match the new path, or autoloading breaks. - **EE overrides** under `ee/app/services/ee/mcp/tools/**` (`link_work_items`, aggregated `search`) — they mirror the FOSS paths and must move in lockstep. - **`manager.rb`** maps (FOSS + EE) reference every class by constant. - **Specs** referencing these class names, including the schema-locked specs from !240451. - **`Labels::SearchService` vs `Search::SearchService`** — distinct namespaces, no collision, but name them deliberately. ## Sequencing Do this **after !240451 merges** (it locks the service schemas in specs and references the current class names). ## Out of scope - Agent-facing `tool_name` casing (`get_workitem_notes` → `get_work_item_notes`, etc.) — tracked separately. - Collapsing the `*Tool` / `*Service` two-file pattern into one — a larger framework change; separate spike if desired.