ENG-856 - Agent tool derivation namespaces operation ids itself

[druks-operator]

Linear ticket: ENG-856

Plan

Implementation plan

1. Update backend/druks/mcp/app.py with separate validation and naming responsibilities. Keep _validate_agent_tools validation-only: it continues requiring an explicit operation_id and a non-empty endpoint docstring, but removes the extension-prefix rejection.
2. Add an OpenAPI-document transformation in the MCP assembly path that identifies extension-owned agent operations from the loader-supplied extension tag and installed extension names. Set the effective operation ID to f"{extension}_{operation_id}" only when it is not already prefixed; leave platform operations without an extension tag untouched.
3. Install that transformation on the app's OpenAPI generation path before constructing OpenAPIProvider, rather than mutating FastAPI route contexts. The provider and the app must consume the same namespaced document, and the transformation must run again after the existing api.openapi_schema = None reset so later /openapi.json regeneration preserves the identifiers. Keep comments and names as end-state descriptions; do not broaden _validate_agent_tools into a naming pass.
4. Update focused coverage in backend/tests/test_mcp_endpoint.py: use a synthetic route owned by a real installed extension such as review to prove an unprefixed ID becomes the namespaced OpenAPI operation and MCP tool; cover the already-prefixed pass-through; retain missing-ID and missing-docstring boot failures; and preserve platform tool-name assertions.
5. Treat backend/tests/test_agent_routes.py as required regression coverage for the served document. Its direct assertions for platform IDs, review_request, and ship_start must continue proving cache regeneration does not alter established names.
6. Update the agent-route paragraph in docs/writing-an-extension.md to describe framework-derived namespacing and retire both live claims that authors must prefix the ID and that boot rejects an unprefixed tool name.

Scope is limited to MCP/OpenAPI derivation, focused backend tests, and the extension-author guide. No frontend, changelog, generated-client, or extension route declaration migration is needed: all existing extension agent IDs are already prefixed, so currently generated client and tool names remain stable. Verification is limited to the repository's backend Ruff, format-check, and pytest gates.

Acceptance Criteria

• AC1: For an extension-owned agent route whose explicit operation ID lacks the owner prefix, the OpenAPI document consumed by OpenAPIProvider, every later regenerated app OpenAPI document, and the derived MCP tool all use <owner>_<operation_id>.
  • Verification: Focused coverage in backend/tests/test_mcp_endpoint.py uses an installed extension owner such as review and asserts both the regenerated OpenAPI operation ID and the tool exposed through tools/list.
• AC2: An extension operation ID already beginning with its owner prefix is not doubled, and platform-owned agent operations without an extension tag retain their declared operation IDs and tool names.
  • Verification: Tests preserve the existing platform, review_request, and ship_start assertions in backend/tests/test_mcp_endpoint.py and backend/tests/test_agent_routes.py, including assertions against the served OpenAPI document after its cache is regenerated.
• AC3: Agent-tool boot validation rejects routes without an explicit operation ID or without a non-empty endpoint docstring, while an otherwise valid unprefixed extension operation ID boots successfully and is namespaced.
  • Verification: The validation matrix in backend/tests/test_mcp_endpoint.py retains the missing-ID and missing-docstring failure branches and replaces the prefix-error branch with successful derivation coverage.
• AC4: The extension author guide states that authors provide an explicit operation ID and Druks derives the extension-prefixed MCP tool name; it no longer instructs authors to include the prefix or claims boot rejects unprefixed IDs.
  • Verification: The evaluator reads the agent-route section of docs/writing-an-extension.md.

[druks-reviewer]

Verification (revision 1, round 1)

Verdict: pass. The diff is scoped exactly to the three files the plan specifies (backend/druks/mcp/app.py, backend/tests/test_mcp_endpoint.py, docs/writing-an-extension.md). Namespace derivation is implemented by wrapping api.openapi before OpenAPIProvider construction — not by mutating route contexts, not via Extension.load, not by prefixing unconditionally — and correctly re-derives after the api.openapi_schema = None reset. The validator was narrowed to only the two author-owned demands (explicit operation_id, non-empty docstring).

AC1 — pass. backend/druks/mcp/app.py:95-134: _namespace_agent_operations derives f"{extension}_{operation_id}" for extension-tagged agent operations; _install_agent_namespacing wraps api.openapi so the provider and every later regeneration share the namespaced document. backend/tests/test_mcp_endpoint.py:145-168 (test_extension_agent_route_derives_the_namespaced_tool) asserts both the served operationId and tools/list membership using the real installed review extension.

AC2 — pass. Pass-through case covered in the same test (review_scan → review_scan, no doubling). Platform routes unchanged: test_tools_list_pins_platform_and_extension_tools and backend/tests/test_agent_routes.py:91-101 (test_openapi_pins_platform_and_extension_agent_routes) still assert platform IDs plus review_request/ship_start, reading the served app.openapi() document directly.

AC3 — pass. _validate_agent_tools (app.py:76-92) retains the missing-operation_id and missing-docstring checks and drops prefix-rejection. test_invalid_extension_agent_route_stops_boot retains both failure branches; the derivation test proves an unprefixed operation_id="scan" boots and is namespaced.

AC4 — pass. docs/writing-an-extension.md:651-658 now describes framework-derived namespacing and drops both retired claims (author must prefix; boot rejects unprefixed IDs).

Verification commands: uv run ruff check backend — pass. uv run ruff format --check backend — pass. uv run pytest backend/ — pass, 1243 passed locally (focused run of the two touched test files: 37 passed). GitHub CI "On Pull Request Backend / checks" — SUCCESS for 837f0c8. Frontend lint/build/test — not_run: no frontend changes in this diff and no frontend CI check registered for this SHA, consistent with the stated backend/docs-only scope.

Blocking findings: none.

Open finding (non-blocking, carried as a review comment): backend/druks/mcp/app.py:127-129 — the wrapper's own if api.openapi_schema: return api.openapi_schema guard bypasses FastAPI's native routes-version cache invalidation once the schema is cached. Not reachable today (all routers are included before create_mcp_app runs, and it performs exactly one deliberate reset immediately after), so it doesn't affect any AC or shipped behavior — flagged as a latent caching-contract note for a possible follow-up, not a blocker.

Code review

Clean-room pass (repo + base/head SHAs only, no plan/AC/ticket context). Read the full diff and all three changed files end to end, plus .druks/review/checklist.md. Traced the namespacing logic for correctness (idempotency across repeated api.openapi() calls, tag-based extension ownership, cache-reset ordering) and found no bug. The new test asserts observable behavior (served operation id, tool-list membership) rather than internal calls, including an explicit idempotency case. No out-of-scope edits — all three touched files are part of the same feature.

No high or medium findings — no follow-up sub-issue filed. Two low, non-blocking notes (also left as inline comments):

• backend/druks/mcp/app.py:127-129 — redundant cache guard ahead of FastAPI's own cache check (same underlying issue as the verification lens's open finding above).
• backend/druks/mcp/app.py:95-134 — _namespace_agent_operations has a single caller; defensible as a policy/mechanics split, but worth a second look against the repo's stated preference against single-caller helpers.

[druks-reviewer]

Low, non-blocking (verification lens open finding): namespaced() short-circuits with if api.openapi_schema: return api.openapi_schema before ever calling the wrapped generate(). Real FastAPI's openapi() invalidates its cache based on a routes-version check, not just openapi_schema truthiness — so this wrapper's own truthy-check bypasses that native invalidation path once openapi_schema is set. Not currently reachable (all routers are included before create_mcp_app runs, and it performs exactly one deliberate reset right after provider construction), so it doesn't affect any acceptance criterion or shipped behavior today. Worth a short comment noting the assumption, or removing the redundant guard, as a follow-up — not blocking this PR. Happy to hear if you see it differently.

[druks-reviewer]

Low, non-blocking (code-review lens): _namespace_agent_operations currently has exactly one caller (_install_agent_namespacing). It's defensible here since it separates spec-mutation policy from app-wrapping mechanics, but worth a second look if the repo generally avoids single-caller helpers.

[czpython]

Two changes before merge:

1. _namespace_agent_operations can derive a duplicate id and nothing raises. If one extension declares scan and another route declares review_scan, both come out as review_scan — two operations, one tool name, silent. Before derivation the collision was visible in the author's code; now only the framework can see it. Raise on a duplicate derived id inside the pass.

2. Minor: namespaced() re-implements the api.openapi_schema cache check that the wrapped generator already performs, and the transform is idempotent anyway. The wrapper can be spec = generate(); _namespace_agent_operations(spec, extension_names); return spec.

The mechanism itself is right — route mutation doesn't survive FastAPI 0.139's version-cached route contexts, and the provider and /openapi.json must share one document.

[druks-reviewer]

@czpython — both points are valid and will be implemented.

1. Collision detection in _namespace_agent_operations — agreed, the derivation pass can silently produce duplicate operation IDs (e.g., review + scan → review_scan collides with an explicit review_scan on another route). The fix: collect all existing operationIds from the spec into a set before the transformation loop, then for each derived = f"{extension}_{operation_id}" raise InvalidAgentToolError(path, f"derived operation id {derived!r} collides with existing operation id; rename the conflicting route") before assigning. Iterating spec["paths"].items() rather than .values() gives the path string for the error message. A new test case in backend/tests/test_mcp_endpoint.py will cover this failure branch.

2. Remove the redundant cache guard in namespaced() — agreed. The if api.openapi_schema: return api.openapi_schema early-return is unnecessary: generate() (the original api.openapi) already does the same check internally, and _namespace_agent_operations is idempotent so calling it on an already-namespaced spec is safe. The wrapper simplifies to spec = generate(); _namespace_agent_operations(spec, extension_names); return spec exactly as you proposed.

Implementation running now.

Addressed in 8dc9775: added collision guard in _namespace_agent_operations and removed the redundant cache guard in namespaced().

[druks-reviewer]

Verdict: pass (revision 2, round 3). This round implements exactly the two fixes czpython requested in the CHANGES_REQUESTED review, and both are correct.

Fix 1 — collision detection (backend/druks/mcp/app.py:107-129): existing_ids is snapshotted as a set from the full spec before any mutation, iteration switched to .items() for path access, and a derived id colliding with an existing explicit id now raises InvalidAgentToolError(path, "derived operation id ... collides with existing operation id; rename the conflicting route") before assignment. Verified the existing_ids precomputation makes this collision check order-independent — no reliance on dict/tag iteration order.

Fix 2 — redundant cache guard removed: namespaced() in _install_agent_namespacing no longer short-circuits with if api.openapi_schema: return api.openapi_schema; generate() (the original api.openapi) already performs that check internally, and _namespace_agent_operations is idempotent, so re-deriving on a warm cache is safe. This also resolves the round-1 open finding about this exact guard bypassing FastAPI's native cache-invalidation contract.

New test test_derived_operation_id_collision_stops_boot (backend/tests/test_mcp_endpoint.py:244-264) registers an unprefixed review-owned route (operation_id="scan") alongside an explicit operation_id="review_scan" route and asserts create_mcp_app raises InvalidAgentToolError matching the collision message — exactly the scenario requested.

backend/tests/test_agent_routes.py has a genuinely empty diff for this round (confirmed via git diff), per czpython's explicit instruction to leave it untouched.

Acceptance criteria (full PR range) — all pass:

• AC1: test_extension_agent_route_derives_the_namespaced_tool asserts the namespaced id survives cache reset and appears via tools/list, using the real review extension.
• AC2: pass-through case (review_scan → review_scan) and platform-route assertions (review_request, ship_start) still hold in both test files.
• AC3: missing-id/missing-docstring failure branches retained; collision branch added; successful unprefixed-derivation boot path covered.
• AC4: docs/writing-an-extension.md describes framework-derived namespacing with a concrete example, no stale prefix-authoring instruction.

Verification commands: uv run ruff check backend and uv run ruff format --check backend — pass (re-ran locally, clean). uv run pytest backend/ — pass via the green GitHub check "On Pull Request Backend / checks" for head_sha 8dc9775 (could not run locally in this sandbox: no Postgres available, an environmental gap unrelated to this diff). Frontend commands — not_run, no frontend changes and no frontend CI check registered for this SHA.

Open findings

• The round-1 code-review lens's note that _namespace_agent_operations has a single caller remains open (not addressed this round, and this round's code-review lens independently judged it consistent with the file's existing one-caller-helper idiom rather than a fresh smell — see Code review section below). Non-blocking.

No other blocking or open findings.

Code review

Clean-room lens (repo + SHAs only, no plan/AC/ticket) read the full diff and every changed file end to end, plus the real /mcp route wiring in druks/api/app.py for context. No findings, high/medium/low.

Highlights: the collision check's existing_ids snapshot is taken before mutation and covers the full spec (not just agent routes), correctly rejecting a derived id that collides with any pre-existing explicit id; isinstance(operation, dict) guards correctly protect against non-operation keys (parameters, servers) in OpenAPI path items; the cache/idempotency reasoning in _install_agent_namespacing was traced and matches its comments; the new tests assert observable behavior (tools/list names, served operationId) rather than implementation details; docs give a concrete before/after example consistent with the new behavior. The lens considered flagging the two new one-caller helpers against a no-single-caller-abstractions concern but found the file already uses that pattern for _validate_agent_tools/_annotate pre-diff, so it's consistent with local idiom, not a new deviation.

No medium or high findings, so no follow-up sub-issue was filed this round.