Skip to content

[Server][Capability] Element sources should return values, and RegistryInterface should be sliced by role #495

Description

@chr-hertel

Opening this as an ADR candidate rather than a PR, because it supersedes the approach in #494 and should be agreed before anything moves.

LoaderInterface, RegistryInterface and Server\Builder are the seam every framework integration binds to. Five independent consumers now drive it. Only the case it was designed for needs nothing added; the other four each build something the seam does not provide.

Consumer How elements arrive What it needs that the seam does not give
mcp/sdk file-system discovery setDiscovery()DiscoveryLoaderDiscoverer — (the case the seam was designed for)
API Platform (api-platform/mcp, Symfony + Laravel) ApiPlatform\Mcp\Capability\Registry\Loader over the resource metadata collection container-resolved handlers; per-name dispatch; a cacheable loader
drupal/mcp_server Drupal plugin managers, via Builder::add() and DiscoveryLoader a list-time visibility seam; per-element failure isolation; laziness that also covers imperative registration
symfony/mcp-bundle McpPass at container-compile time, multi-server a value representation of a pre-computed element set; a lazy custom registry
Shopware 6.8 Framework\Mcp\Loader\AbstractAppMcpLoader over the database a way to say "source unavailable" that is not an empty registry

What each consumer had to build around the seam

API Platform routes around the registry in three places.

  1. Its handler is the literal string 'api_platform.mcp.handler' — a service id, not a class. ReferenceHandler::handle() cannot resolve it (not a class with __invoke, not a function, not callable, not an array → InvalidArgumentException('Invalid handler type'), src/Capability/Registry/ReferenceHandler.php:78). The string is a sentinel that is never invoked.
  2. tools/call and resources/read therefore bypass the registry entirely: ApiPlatform\Mcp\Server\Handler is tagged mcp.request_handler, and Builder::resolve() merges caller handlers before the SDK's (src/Server/Builder.php:1086), so it wins. It resolves the operation by name through its own OperationMetadataFactory. Dispatch is a per-name resolver; the registry is used only as a catalog for */list.
  3. tools/list carries ApiPlatform\Mcp\Server\ListHandler, which holds private bool $loaded and calls $this->loader->load($this->registry) itself — a hand-rolled reimplementation of Registry::load(), forced because setRegistry() puts it on the eager path. Its docblock carries TODO: remove once php-sdk:^0.7 has […] pull/389.

drupal/mcp_server (Mateu Aguiló Bosch, mcp/sdk: ^0.6 || ^0.7) is the closest thing we have to a model consumer, and it is worth being precise about what it proves. It does not call setRegistry(), so it is on the lazy path; its tool plugins implement ToolHandlerInterface directly and are registered through Builder::add($tool, $plugin), so ReferenceHandler dispatches straight to the plugin with no handler-identity problem; it passes Drupal's own PSR-11 container to setContainer(); and it feeds DiscoveryLoader a CachedDiscoverer wrapped around a DrupalDiscoveryCache (PSR-16 over Drupal's cache.discovery bin). The explicit-element API and the discovery cache seam both do their job here. Two things still bite it:

  • It hand-rolls per-element failure isolation. Every register*() in McpServerFactory wraps a single element in try/catch and logs, and registerTools() additionally pre-runs NameValidator itself with the comment "The SDK rejects names that fail NameValidator at registration time. Skip them with a warning to avoid aborting the whole server build." That is the third consumer to build this defence by hand.
  • Its access checks have nowhere to live at list time. ToolPluginInterface declares checkAccess(AccountInterface) and checkToolAccess(string, AccountInterface), ToolPluginBase implements both — and nothing in the module ever calls them. Resources fare slightly better: ResourceProviderHandler and ResourceTemplateProviderHandler call $plugin->checkAccess($uri, $currentUser) inside read(), i.e. at read time. So resources/list is unfiltered while resources/read is checked, and the tool-side access API is stranded — the same split API Platform has, arrived at independently.

(The other Drupal module, project/mcp from the AI initiative, does not use this SDK at all — it reimplements the JSON-RPC methods over drupal/jsonrpc. Its plugin manager filters the tool list per user and resolves tools/call from a name prefix without instantiating anything else. It is a useful sketch of what a visibility seam and a resolver role would have to support, but it is not a consumer, and nothing below depends on it.)

symfony/mcp-bundle hoists every piece of the SDK's runtime work to container-compile time and has a regression test forbidding the SDK's own discovery path (McpBundleTest.php:468). Because LoaderInterface has no value representation, the pre-computed element set has to be expressed as N positional builder method calls, with Icon/Annotations/\stdClass schema markers hand-marshalled back into inline Definitions so the container dumper can hold them. It must call setRegistry() — for debug:mcp, the profiler, and its own event dispatcher — which pins it to the eager path and makes setLazyLoading() dead API for it.

Shopware wraps its query in catch (DBALException) { return; }, silently yielding an empty registry when the database is unreachable, because a loader that throws takes the server down.

The structural causes

A loader is a sink, not a source. LoaderInterface::load(RegistryInterface $registry): void has no return value, no error contract and no idempotency contract; everything it produces is a side effect on a mutable object it was handed. So it cannot be cached — contrast DiscovererInterface::discover(): DiscoveryState one layer down, which is cached by CachedDiscoverer; we put the cacheable shape on the thing nobody extends and the uncacheable shape on the thing everybody implements. It cannot be composed with a policy (ChainLoader is a bare foreach with no try, no dedup, no priority). It cannot be partial, so every registry read materializes everything. And it cannot be introspected, so detectCapabilities() must guess — any opaque source makes it advertise all kinds (Builder.php:1146-1158).

It also grants loaders write access to live state and read access that re-enters loading: DiscoveryLoader calls hasTool()/getTool() during its own load(), and those call Registry::load(). It terminates only because of Registry's $loading re-entrancy guard (Registry.php:87,105-114) — a precondition RegistryInterface does not express. And loaders are secretly stateful: DiscoveryLoader::$owned ties an instance to one registry and one process.

Deferral only covers loaders, not the elements a caller registers itself. setLazyLoading() defers ChainLoader::load(); it cannot defer Builder::add()/addTool(), because those take an already-constructed definition and handler. drupal/mcp_server registers mcp_server.server as shared: false, so McpServerFactory::create() runs per request and instantiates every tool plugin, loads every enabled prompt config entity and instantiates every resource provider before build() is reached. For a consumer that builds its element set imperatively — which is what Builder::add() is for — lazy loading currently buys nothing. A source that is registered and only asked for its elements on demand fixes this; a pre-built definition+handler pair cannot.

RegistryInterface is sliced by element kind instead of by role, and omits its own lifecycle. Of its 24 methods, exactly 7 are called at request time. Five singular has* methods plus getResourceTemplate() exist solely to serve DiscoveryLoader's identity check; four plural has* methods exist solely to serve detectCapabilities(), and only on the eager branch. Meanwhile load(), loadFrom() — and, in #494, deferLoadingFrom() and isEmpty() — are not on the interface, so Builder type-tests the concrete class ($registry instanceof Registry, Builder.php:1050). #494's own body states the constraint:

RegistryInterface is untouched. Both new methods live on Registry, which is what the builder already type-checks for loadFrom() — adding them to the interface would break third-party implementations.

That is the diagnosis stated as a constraint. A third-party RegistryInterface gets a strictly worse path forever, and Builder::resolve() now carries four behavioural combinations (builder-owned lazy / builder-owned eager / custom empty lazy / custom pre-populated eager) for what is one concept.

Why this is urgent now

#389 made loading lazy, which moved it into request handling. All-or-nothing loading then became a runtime liability: #476 / #477 report one placeholder-less resource template making tools/list and tools/call answer -32602 on every subsequent request, because ChainLoader has no try and ReflectedElementLoader rethrows as ConfigurationException. Shopware and API Platform each defend against this by hand.

Proposal

Element sources return immutable values; the registry interface is sliced by role, not by element kind; and the load lifecycle is part of the contract.

1. A source returns a collection. Replace the sink signature with a source returning an immutable, serializable ElementCollection (the existing DiscoveryState, generalized), plus a cheap declaration of which kinds it can contribute:

interface ElementSourceInterface
{
    public function provide(): ElementCollection;
    public function kinds(): ElementKind;   // answered without providing
}

Caching, filtering, prioritizing and failure isolation become decorators over this.

2. Split RegistryInterface by role, with Registry implementing all four:

interface ElementCatalogInterface    // getTools/getPrompts/getResources/getResourceTemplates
interface ElementResolverInterface   // getTool/getPrompt/getResource — per name/URI, no catalog
interface ElementWriterInterface     // register*/unregister*
interface DeferredRegistryInterface  // load(): void, isEmpty(): bool

Request handlers depend on the narrowest role they use: List*Handler on the catalog, CallToolHandler/GetPromptHandler/ReadResourceHandler on the resolver. Builder depends on DeferredRegistryInterface instead of instanceof Registry, making the lazy path available to any registry implementation.

3. Conflict resolution moves out of the loader. Precedence (manual beats discovered) becomes an explicit merge rule over ElementCollections applied once by the composition, replacing DiscoveryLoader::mayWrite*(), unregisterOwned() and the $owned state — and with them the re-entrant registry reads that only work because of Registry::$loading.

4. Failure is isolated per element and per source. A failing element is skipped and logged; a failing source is skipped and logged; the server stays servable. Generalizes #477 to the whole pipeline, and is non-negotiable while loading happens at request time.

5. Visibility is a seam, not a request-scoped registry.

interface ElementVisibilityInterface
{
    public function isVisible(ElementReference $ref, SessionInterface $session): bool;
}

Applied by the list and call handlers, so the registry stays a process-level, cacheable catalog while per-user filtering becomes expressible.

6. Handler resolution accepts container ids. ReferenceHandler consults the container with the raw handler string before requiring class_exists().

The five singular has* methods and getResourceTemplate() leave the public surface once (1) and (3) land.

BC posture

We're 0.x and both interfaces change. Known implementors — api-platform, symfony/mcp-bundle, shopware, craft-mcp, kirby-mcp, webman-mcp, itop-mcp — get a migration note and a thin SourceLoader implements LoaderInterface adapter so an existing loader can be wrapped rather than rewritten in one step. No deprecation layer carried past 0.9.

Consequences

  • API Platform drops ListHandler entirely (already true once [Server][Capability] Defer loading into a custom registry #494 lands), registers a real container-resolved handler instead of the sentinel, and can retire its custom Handler in favour of the resolver role — or keep it, now as a choice rather than a workaround. Its loader becomes cacheable.
  • symfony/mcp-bundle dumps one ElementCollection instead of N positional builder method calls, dropping dumpable() and the Definition re-marshalling; its custom registry gets the lazy path; per-app handler services become possible.
  • drupal/mcp_server stops hand-rolling failure isolation, gets a place to call the checkToolAccess() it already implements, and its per-request create() stops materializing every plugin up front. project/mcp becomes adoptable rather than a reimplementation: prefix-based dispatch maps to the resolver role, role checks to the visibility seam.
  • Shopware gains a typed way to report an unavailable source, and per-source failure isolation means a cold database no longer silently empties the server.
  • We give up a single flat RegistryInterface and a one-method loader — both easy to explain. We gain cacheable sources, cheap per-name dispatch, honest capability advertisement, isolated failures, and a visibility seam.
  • Not in scope: Builder::addTool() and friends, Builder::add() with the four handler interfaces, and setDiscovery() keep their public shape. Only the loader seam beneath them changes.

Sequencing

  1. Land [Server][Capability] Defer loading into a custom registry #494. It is a correct tactical fix, removes API Platform's ListHandler workaround and unblocks mcp-bundle today. It is not the structural fix, and its body says why it cannot be.
  2. Agree this, then write it up as adr/0002.
  3. Small independent fixes first: container-id handler resolution (6); CachedDiscoverer's cache key omits $namePatterns and its clearCache() wipes the whole PSR-16 pool rather than the mcp_discovery_ prefix; [Server] Handler type uses bare Closure, hard to decorate RegistryInterface under strict PHPStan #468's Handler phpstan alias.
  4. The interface split (2) — the smallest change that removes the instanceof Registry class of problem.
  5. The source rework (1, 3, 4).
  6. The visibility seam (5), which depends on 4.

#44 (extract discovery from core) becomes straightforward once discovery is one ElementSourceInterface among several.

Alternatives considered

  • Keep the sink and add a CachedLoader. Not possible: there is nothing to cache. A void method's only output is mutation of a collaborator supplied at call time.
  • Make the registry request-scoped so per-user visibility falls out. Forfeits process-level caching for everyone in order to serve the subset that needs filtering; a filter at the handler is strictly cheaper.
  • Add deferLoadingFrom()/isEmpty() to RegistryInterface rather than splitting it. This is the option [Server][Capability] Defer loading into a custom registry #494 explicitly declines, and rightly — it widens an interface that is already three roles wide, and 17 of whose 24 methods no request path calls.
  • Do nothing; document the workarounds. Each integration has already paid the cost, so the status quo is survivable — drupal/mcp_server shows the intended path does work when a consumer stays off setRegistry(). What it does not survive is the accumulation: three consumers have now independently hand-rolled per-element failure isolation, and two have independently ended up with an access-checked call path and an unfiltered list.

/cc @soyuka

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ADRDiscussing and documenting fundamental topics with Architecture Decision RecordsServerIssues & PRs related to the Server componentbreaking changeBreaking the Backwards Compatibility Promiseneeds designValid issue but needs maintainer alignment on design or approach

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions