[Refactor] Govern MCP work item and note tools through their own container lookup

Raounak Sharmarequested to merge
633015-mcp-governance-tool-container-lookup into master

What does this MR do and why?

MCP governance resolves the containers of a tools/call with the GovernanceContainerResolver. The resolver parses the declared container arguments itself. Each tool finds its container with its own code. !258209 (merged) made the two agree for every tool, but only by keeping two pieces of code in sync.

This is the first of three stacked MRs that remove that duplication. Each governable tool exposes governed_containers, built on the same resolve methods its execute path uses. This MR adds the plumbing and converts the work item and note tools:

  • GraphqlService builds its GraphqlTool and asks it for governed_containers. A tool that doesn't have the method yet keeps the declared lookup, so tools that haven't converted keep master behaviour.
  • The work item tools return their parent through resolve_parent or resolve_work_item_parent. save_work_item and get_saved_view_work_items build their tool through build_graphql_tool, because governance now builds a tool for every GraphQL service. save_work_item checks work_item_iid with present?, so a malformed value reaches schema validation instead of raising while governance builds the tool.
  • save_note returns the parent of the noteable. A resolve_discussion call names no noteable, so the tool returns no container and a new record: lookup on discussion_id governs the call by the project or group that holds the discussion.
  • Record arguments (parent_id, saved views, work item GIDs, discussion_id) use the declared record: lookups.
  • When a tool's own lookup raises (not found, forbidden, access denied, or bad arguments), governance treats the call as having no container. The tool then reports its own error at execute time. The tool path runs the mcp_container_lookup_cap check first and lets TooManyContainersError through.
  • BaseService#tool_governed_containers drops omitted arguments (null, "", zero-filled IDs) before the tool's lookup, the same way execute does. Without this, a call with url: "" or project_id: 0 made the lookup raise, so no rule was checked, and the tool then ran on group_id.
  • Telemetry keeps the declaration-based path as declared_containers. The start event runs before set_cred, so it must not do an access-checked lookup. This has to land before any tool's own lookup does one.
  • UrlParser#resolve_work_item_from_url now returns the work item instead of its Global ID, and save_note calls it to find the work item a URL names.

The stack:

  1. This MR: plumbing, work items and notes.
  2. !268283: merge requests, commits and repositories.
  3. !268284: the rest of CE and EE, then GraphqlTool#governed_containers raises NotImplementedError by default and the fallback goes.

!261051 stacks on the third.

How to reproduce the issue

There is no user-facing defect. On master, governance for a work item tool goes through the resolver's parsing of the declared arguments and never reaches the tool's resolve methods.

  1. On master, build Mcp::Tools::WorkItems::GetWorkItemService and call governed_containers with a work item URL.
  2. You can see that the result comes from containers_from_identifiers, and GetWorkItemTool#resolve_work_item_parent is never called.

How to test the fix

  1. On this branch, repeat the call. The result is the parent that GetWorkItemTool#resolve_work_item_parent returns, which is the parent build_variables uses.
  2. Call governed_containers for a tool this MR doesn't convert, such as get_merge_request. It still returns the declared lookup's result.

Run the specs:

bin/rspec spec/services/mcp ee/spec/services/mcp ee/spec/services/ee/mcp spec/lib/api/mcp/handlers ee/spec/lib/ee/api/mcp/handlers ee/spec/lib/ai/tool_rules spec/requests/api/mcp ee/spec/requests/api/mcp

References

Edited by Raounak Sharma

Merge request reports