ENG-856 - Agent tool derivation namespaces operation ids itself

backend/druks/mcp/app.py

@@ -77,25 +77,77 @@ def _validate_agent_tools(api: FastAPI) -> None:
     # The provider logs component-fn errors instead of raising, so derived tools
     # cannot refuse boot; validate the routes before derivation. Inclusion is
     # deferred (api.routes holds unresolved routers), so the contexts iterator
-    # is the one view with every route's merged tags — and the loader tags each
-    # extension route with its extension's name, so the tag names the owner.
-    extension_names = {extension.name for extension in iter_extensions()}
-
+    # is the one view with every route's merged tags. Validation owns only the
+    # two demands the author owns — an explicit operation_id and a non-empty
+    # docstring; the extension prefix is the framework's to derive, not the
+    # author's to repeat (see _namespace_agent_operations).
     for route in iter_route_contexts(api.routes):
         if not isinstance(route.original_route, APIRoute) or "agent" not in route.tags:
             continue
 
         where = f"{'/'.join(sorted(route.methods or ()))} {route.path}"
-        operation_id = route.operation_id
-        if not operation_id:
+        if not route.operation_id:
             raise InvalidAgentToolError(where, "an explicit operation_id is required")
         if not inspect.getdoc(route.endpoint):
             raise InvalidAgentToolError(where, "a non-empty endpoint docstring is required")
-        extension = next((tag for tag in route.tags if tag in extension_names), None)
-        if extension and not operation_id.startswith(f"{extension}_"):
-            raise InvalidAgentToolError(
-                where, f"operation_id {operation_id!r} must start with {extension + '_'!r}"
-            )
+
+
+def _namespace_agent_operations(spec: dict, extension_names: set[str]) -> None:
+    # Derive each extension-owned agent operation's id to f"{extension}_{operation_id}",
+    # so the tool name the provider reads off the spec is namespaced without the
+    # author repeating the prefix. The loader tags every extension route with its
+    # extension's name, so among an agent operation's tags the one naming an
+    # installed extension is the owner; platform agent operations carry no such
+    # tag and keep their declared ids. An already-prefixed id passes through, so
+    # stable names like ship_start never double — and the namespace is what makes
+    # the merged document's operation ids globally unique. A derived id that would
+    # collide with another route's explicit id is rejected: before this derivation
+    # the clash was visible in the author's code, so the framework must surface it
+    # now that it owns the naming.
+    existing_ids = {
+        op.get("operationId")
+        for ops in spec.get("paths", {}).values()
+        for op in ops.values()
+        if isinstance(op, dict) and op.get("operationId")
+    }
+    for path, operations in spec.get("paths", {}).items():
+        for operation in operations.values():
+            if not isinstance(operation, dict) or "agent" not in operation.get("tags", []):
+                continue
+            operation_id = operation.get("operationId")
+            if not operation_id:
+                continue
+            extension = next((tag for tag in operation["tags"] if tag in extension_names), None)
+            if extension and not operation_id.startswith(f"{extension}_"):
+                derived = f"{extension}_{operation_id}"
+                if derived in existing_ids:
+                    raise InvalidAgentToolError(
+                        path,
+                        f"derived operation id {derived!r} collides with existing "
+                        "operation id; rename the conflicting route",
+                    )
+                operation["operationId"] = derived
+
+
+def _install_agent_namespacing(api: FastAPI) -> None:
+    # The tool name comes from the spec's operation id, so the namespace must
+    # land on the document api.openapi() builds — not on FastAPI's cached, merged
+    # route contexts, which later generation silently discards. Wrap the app's
+    # own generator so the provider here and every later /openapi.json share one
+    # namespaced document: each fresh build (including after the openapi_schema
+    # reset below) re-derives the ids. generate() is the app's own api.openapi,
+    # which already returns the cached schema when it is warm, so namespaced()
+    # need not repeat that guard — and the derivation is idempotent, so running
+    # it on a warm cache leaves the already-prefixed ids untouched.
+    extension_names = {extension.name for extension in iter_extensions()}
+    generate = api.openapi
+
+    def namespaced() -> dict:
+        spec = generate()
+        _namespace_agent_operations(spec, extension_names)
+        return spec
+
+    api.openapi = namespaced
 
 
 def _annotate(route: HTTPRoute, component: object) -> None:
@@ -110,6 +162,7 @@ def _annotate(route: HTTPRoute, component: object) -> None:
 
 def create_mcp_app(api: FastAPI) -> StarletteWithLifespan:
     _validate_agent_tools(api)
+    _install_agent_namespacing(api)
     # Built directly rather than via from_fastapi, which owns the transport:
     # raise_app_exceptions=False makes an app crash reach the tool as the
     # app's sanitized 500, so no masking is needed and the taxonomy travels.

backend/tests/test_mcp_endpoint.py

@@ -19,6 +19,7 @@
 from fastapi.testclient import TestClient
 from fastmcp.client import Client
 from fastmcp.client.transports import StreamableHttpTransport
+from starlette.routing import Route
 
 _IN_APP_ASK = {
     "presentation": "in_app",
@@ -216,7 +217,6 @@ async def test_tools_list_pins_platform_and_extension_tools(app, pat_token):
     ("operation_id", "docstring", "message"),
     [
         (None, "Start a review.", "explicit operation_id"),
-        ("start_review", "Start a review.", "must start with 'review_'"),
         ("review_start", None, "docstring"),
     ],
 )
@@ -241,6 +241,90 @@ async def endpoint():
         create_mcp_app(api)
 
 
+def test_derived_operation_id_collision_stops_boot():
+    # The 'review' extension's unprefixed 'scan' derives to 'review_scan', which
+    # another route already claims explicitly — the framework must reject the
+    # clash rather than silently mint two operations sharing an id.
+    api = FastAPI()
+    router = APIRouter()
+
+    async def scan():
+        """Scan the review target."""
+
+    async def review_scan():
+        """Re-run the review scan."""
+
+    router.add_api_route("/scans", scan, methods=["POST"], operation_id="scan", tags=["agent"])
+    router.add_api_route(
+        "/rescans", review_scan, methods=["POST"], operation_id="review_scan", tags=["agent"]
+    )
+    api.include_router(router, prefix="/api/review", tags=["review"])
+
+    with pytest.raises(InvalidAgentToolError, match="collides with existing operation id"):
+        create_mcp_app(api)
+
+
+def _agent_route_app(operation_id: str) -> FastAPI:
+    # A synthetic agent route owned by the installed 'review' extension — the
+    # loader-stamped 'review' tag names the owner, exactly as a real router does.
+    # The endpoint mounts the same /mcp Route and mcp lifespan as the real app so
+    # tools/list resolves in-process.
+    held: dict[str, object] = {}
+
+    @asynccontextmanager
+    async def lifespan(scope_app):
+        async with held["mcp"].lifespan(scope_app):
+            yield
+
+    api = FastAPI(lifespan=lifespan)
+    router = APIRouter()
+
+    async def endpoint():
+        """Scan the review target."""
+
+    router.add_api_route(
+        "/scans", endpoint, methods=["POST"], operation_id=operation_id, tags=["agent"]
+    )
+    api.include_router(router, prefix="/api/review", tags=["review"])
+
+    mcp = create_mcp_app(api)
+    held["mcp"] = mcp
+    api.router.routes.append(
+        Route("/mcp", mcp, methods=["POST", "DELETE"], include_in_schema=False)
+    )
+    return api
+
+
+def _served_operation_id(schema: dict, path: str) -> str:
+    return schema["paths"][path]["post"]["operationId"]
+
+
+@pytest.mark.parametrize(
+    ("operation_id", "expected"),
+    [
+        ("scan", "review_scan"),  # an unprefixed id gains its owner's prefix
+        ("review_scan", "review_scan"),  # an already-prefixed id passes through, never doubled
+    ],
+)
+async def test_extension_agent_route_derives_the_namespaced_tool(
+    druks_db, pat_token, operation_id, expected
+):
+    api = _agent_route_app(operation_id)
+
+    # The document the provider consumed and every later regeneration carry the
+    # derived id, not the bare one the author declared.
+    assert _served_operation_id(api.openapi(), "/api/review/scans") == expected
+    api.openapi_schema = None
+    assert _served_operation_id(api.openapi(), "/api/review/scans") == expected
+
+    async with live(api), _client(api, pat_token) as client:
+        tools = {tool.name for tool in await client.list_tools()}
+
+    assert expected in tools
+    if operation_id != expected:
+        assert operation_id not in tools
+
+
 async def test_claims_resolve_the_calling_account(app, druks_db):
     # get_usage must answer as the token's account — the forwarded bearer.
     mine = Account.get_or_create("op@example.com")

docs/writing-an-extension.md

@@ -651,12 +651,14 @@ the prefix its own resource is called:
 router = APIRouter(prefix="/reviews")
 ```
 
-Tagging a route `agent` also derives it into an MCP tool: its explicit `operation_id`,
-prefixed with the extension name, is the tool name and its docstring is the description.
-`GET` derives read-only; a write declares `x-destructive: false` or `x-idempotent: true`
-in `openapi_extra` only when that statement is genuinely true, otherwise the safe defaults
-are destructive and non-idempotent. Boot refuses a missing or unprefixed tool name
-and a missing docstring.
+Tagging a route `agent` also derives it into an MCP tool: you give it an explicit
+`operation_id`, and Druks derives the tool name by prefixing it with your extension
+name — write `operation_id="add_peer"` in `peer_tracker` and the tool is
+`peer_tracker_add_peer`. The docstring is the description. `GET` derives read-only;
+a write declares `x-destructive: false` or `x-idempotent: true` in `openapi_extra`
+only when that statement is genuinely true, otherwise the safe defaults are
+destructive and non-idempotent. Boot refuses a missing `operation_id` or a missing
+docstring.
 
 Two spellings run through druks, and which one a segment wears says who owns it: