diff --git a/src/Capability/Registry.php b/src/Capability/Registry.php index 97840431..92c4a4f6 100644 --- a/src/Capability/Registry.php +++ b/src/Capability/Registry.php @@ -11,6 +11,7 @@ namespace Mcp\Capability; +use Mcp\Capability\Registry\Loader\ChainLoader; use Mcp\Capability\Registry\Loader\LoaderInterface; use Mcp\Capability\Registry\PromptReference; use Mcp\Capability\Registry\ResourceReference; @@ -69,7 +70,7 @@ public function __construct( private readonly ?EventDispatcherInterface $eventDispatcher = null, private readonly LoggerInterface $logger = new NullLogger(), private readonly NameValidator $nameValidator = new NameValidator(), - private readonly ?LoaderInterface $loader = null, + private ?LoaderInterface $loader = null, ) { } @@ -114,6 +115,28 @@ public function loadFrom(LoaderInterface $loader): void } } + /** + * Adopts $loader for the deferred load, so a registry the caller constructed can still load at + * first read instead of at build time. Chains behind a loader the constructor already took, + * but only when that loader is still owed its run: once it has already run, chaining would run + * it a second time — discovery would rescan — so $loader replaces it instead. Resetting $loaded + * is what makes the adopted loader actually run on the next read. + */ + public function deferLoadingFrom(LoaderInterface $loader): void + { + $this->loader = null === $this->loader || $this->loaded ? $loader : new ChainLoader([$this->loader, $loader]); + $this->loaded = false; + } + + /** + * True when nothing is registered yet. Reads the backing arrays directly, so unlike has*() it + * never triggers the loader. + */ + public function isEmpty(): bool + { + return [] === $this->tools && [] === $this->resources && [] === $this->resourceTemplates && [] === $this->prompts; + } + public function registerTool(Tool $tool, callable|array|string $handler): ToolReference { if (!$this->nameValidator->isValid($tool->name)) { diff --git a/src/Server/Builder.php b/src/Server/Builder.php index 1a5dd73b..0f42c8c5 100644 --- a/src/Server/Builder.php +++ b/src/Server/Builder.php @@ -522,8 +522,9 @@ public function setRegistry(RegistryInterface $registry): self * * Lazy (the default) defers loading to the first registry read so a persistent runtime does not * freeze the registry to a source not yet ready at build time. Disable to load eagerly at build. - * A registry supplied via setRegistry() is always loaded eagerly; its own constructor loader, - * if it has one, still runs on the first read. + * A registry supplied via setRegistry() is deferred the same way, whatever it already holds is + * still advertised by capability detection. Either way its own constructor loader, if it has + * one, still runs on the first read. */ public function setLazyLoading(bool $lazyLoading = true): self { @@ -1043,17 +1044,25 @@ private function resolve(): array } $chainLoader = new ChainLoader($loaders); + $hasPreloadedElements = false; if ($this->hasCustomRegistry) { - // Builder can't inject the loader into an already-constructed instance, so load it eagerly. - // Via loadFrom(), which suppresses the change events the load would otherwise dispatch. $registry = $this->registry; - if ($registry instanceof Registry) { - $registry->loadFrom($chainLoader); + + if ($this->lazyLoading && $registry instanceof Registry) { + $hasPreloadedElements = !$registry->isEmpty(); + $registry->deferLoadingFrom($chainLoader); + $eagerlyLoaded = false; } else { - $chainLoader->load($registry); + // A foreign RegistryInterface cannot be deferred into, so it is loaded eagerly here. + // loadFrom() suppresses the change events the load would otherwise dispatch. + if ($registry instanceof Registry) { + $registry->loadFrom($chainLoader); + } else { + $chainLoader->load($registry); + } + $eagerlyLoaded = true; } - $eagerlyLoaded = true; } else { $registry = new Registry($eventDispatcher, $logger, loader: $chainLoader); if (!$this->lazyLoading) { @@ -1064,7 +1073,7 @@ private function resolve(): array $messageFactory = MessageFactory::make(additional: $this->extensionMessages); - $capabilities = $this->serverCapabilities ?? $this->detectCapabilities($registry, $eagerlyLoaded, $eventDispatcher); + $capabilities = $this->serverCapabilities ?? $this->detectCapabilities($registry, $eagerlyLoaded, $eventDispatcher, $hasPreloadedElements); // Extensions enabled via enableExtension() are folded into caller-supplied // capabilities too, so setCapabilities() does not silently drop them. @@ -1118,9 +1127,11 @@ private function resolve(): array /** * When loaded, capabilities are read from the registry. When deferred, reading it would force * the load, so they are advertised from the configured sources instead — opaque sources (custom - * loaders, discovery) advertise all kinds, and over-advertising is harmless per MCP semantics. + * loaders, discovery) advertise all kinds, and over-advertising is harmless per MCP semantics. A + * custom registry deferred while already holding elements ($hasPreloadedElements) counts as an + * opaque source too, for the same reason: reading it would force the load it is deferred to avoid. */ - private function detectCapabilities(RegistryInterface $registry, bool $eagerlyLoaded, ?EventDispatcherInterface $eventDispatcher): ServerCapabilities + private function detectCapabilities(RegistryInterface $registry, bool $eagerlyLoaded, ?EventDispatcherInterface $eventDispatcher, bool $hasPreloadedElements): ServerCapabilities { // Without a dispatcher the registry announces nothing, so there is no // list-changed notification to advertise. @@ -1143,7 +1154,7 @@ private function detectCapabilities(RegistryInterface $registry, bool $eagerlyLo ); } - $hasOpaqueSources = [] !== $this->loaders || null !== $this->discoveryBasePath; + $hasOpaqueSources = [] !== $this->loaders || null !== $this->discoveryBasePath || $hasPreloadedElements; $hasResources = [] !== $this->resources || [] !== $this->explicitResources || [] !== $this->resourceTemplates || [] !== $this->explicitResourceTemplates || $hasOpaqueSources; return new ServerCapabilities( diff --git a/tests/Unit/Capability/RegistryTest.php b/tests/Unit/Capability/RegistryTest.php index 92782ab9..4a35cbb8 100644 --- a/tests/Unit/Capability/RegistryTest.php +++ b/tests/Unit/Capability/RegistryTest.php @@ -853,6 +853,104 @@ public function testLoadIsANoopWithoutAConfiguredLoader(): void $this->assertFalse($registry->hasTools()); } + public function testDeferLoadingFromDoesNotRunUntilFirstRead(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->never())->method('load'); + + $registry = new Registry(null, $this->logger); + $registry->deferLoadingFrom($loader); + } + + public function testDeferLoadingFromRunsOnFirstReadAndPopulatesTheRegistry(): void + { + $registry = new Registry(null, $this->logger); + $registry->deferLoadingFrom($this->toolLoader($this->createValidTool('deferred'))); + + $this->assertTrue($registry->hasTools()); + $this->assertArrayHasKey('deferred', $registry->getTools()->references); + } + + public function testDeferLoadingFromRunsTheLoaderExactlyOnceAcrossManyReads(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->once())->method('load'); + + $registry = new Registry(null, $this->logger); + $registry->deferLoadingFrom($loader); + + $registry->hasTools(); + $registry->getTools(); + $registry->hasResources(); + } + + public function testDeferLoadingFromChainsBehindTheConstructorLoader(): void + { + // Both register 'shared'; last-write-wins proves the run order, since + // ChainLoader lets the later loader overwrite the earlier one's registration. + $constructorLoader = $this->toolLoader($this->createValidTool('shared', null, 'from constructor')); + $deferredLoader = $this->toolLoader($this->createValidTool('shared', null, 'from deferred')); + + $registry = new Registry(null, $this->logger, loader: $constructorLoader); + $registry->deferLoadingFrom($deferredLoader); + + $tools = $registry->getTools()->references; + + $this->assertArrayHasKey('shared', $tools); + $this->assertSame('from deferred', $tools['shared']->description); + } + + public function testDeferLoadingFromRunsTheAdoptedLoaderAfterTheConstructorLoaderAlreadyRan(): void + { + // A read before deferLoadingFrom() runs the constructor loader and sets $loaded, the bug + // this covers: the adopted loader was then stored but never run because load() returned on + // $loaded before consulting it. + $constructorLoader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + $adoptedLoader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + + $registry = new Registry(null, $this->logger, loader: $constructorLoader); + $registry->hasTools(); + + $registry->deferLoadingFrom($adoptedLoader); + $registry->hasTools(); + $registry->hasResources(); + + $this->assertSame(1, $constructorLoader->calls); + $this->assertSame(1, $adoptedLoader->calls); + } + + public function testIsEmptyIsTrueForAFreshRegistryAndDoesNotTriggerTheLoader(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->never())->method('load'); + + $registry = new Registry(null, $this->logger, loader: $loader); + + $this->assertTrue($registry->isEmpty()); + } + + public function testIsEmptyIsFalseAfterRegisterTool(): void + { + $registry = new Registry(null, $this->logger); + $registry->registerTool($this->createValidTool('registered'), 'handler'); + + $this->assertFalse($registry->isEmpty()); + } + private function toolLoader(Tool $tool): LoaderInterface { return new class($tool) implements LoaderInterface { @@ -907,7 +1005,7 @@ public function jsonSerialize(): float $this->assertNull($toolRef->extractStructuredContent($result, ProtocolVersion::V2025_11_25)); } - private function createValidTool(string $name, ?array $outputSchema = null): Tool + private function createValidTool(string $name, ?array $outputSchema = null, ?string $description = null): Tool { return new Tool( name: $name, @@ -919,7 +1017,7 @@ private function createValidTool(string $name, ?array $outputSchema = null): Too ], 'required' => null, ], - description: "Test tool: {$name}", + description: $description ?? "Test tool: {$name}", annotations: null, icons: null, meta: null, diff --git a/tests/Unit/Server/BuilderTest.php b/tests/Unit/Server/BuilderTest.php index 790af5f5..8bbe197e 100644 --- a/tests/Unit/Server/BuilderTest.php +++ b/tests/Unit/Server/BuilderTest.php @@ -443,6 +443,92 @@ public function testAThirdPartyRegistryIsStillLoadedThroughThePlainLoader(): voi ->addTool(static fn (): string => 'ok', 'alpha') ->build(); } + + #[TestDox('An empty custom registry with lazy loading defers its configured loader past build(), running it on the first registry read')] + public function testEmptyCustomRegistryDefersLoaderPastBuild(): void + { + $loader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + + $registry = new Registry(); + + Server::builder() + ->setRegistry($registry) + ->addLoader($loader) + ->build(); + + $this->assertSame(0, $loader->calls); + + $registry->hasTools(); + + $this->assertSame(1, $loader->calls); + } + + #[TestDox('An empty custom registry with lazy loading advertises tools from the configured loader without forcing a load')] + public function testEmptyCustomRegistryAdvertisesToolsFromConfiguredLoaderWithoutLoading(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->never())->method('load'); + + $registry = new Registry(); + + $server = Server::builder() + ->setServerInfo('test', '1.0.0') + ->setRegistry($registry) + ->addLoader($loader) + ->build(); + + $capabilities = $this->extractServerCapabilities($server); + + $this->assertTrue($capabilities->tools); + } + + #[TestDox('setLazyLoading(false) with an empty custom registry loads it eagerly during build()')] + public function testSetLazyLoadingFalseWithEmptyCustomRegistryLoadsEagerly(): void + { + $loader = $this->createMock(LoaderInterface::class); + $loader->expects($this->once())->method('load'); + + $registry = new Registry(); + + Server::builder() + ->setRegistry($registry) + ->setLazyLoading(false) + ->addLoader($loader) + ->build(); + } + + #[TestDox('A pre-populated custom registry with lazy loading also defers its configured loader past build(), instead of the old isEmpty() gate loading it eagerly')] + public function testPreloadedCustomRegistryDefersLoaderPastBuild(): void + { + $loader = new class implements LoaderInterface { + public int $calls = 0; + + public function load(RegistryInterface $registry): void + { + ++$this->calls; + } + }; + + $registry = new Registry(); + $registry->registerTool( + new Tool(name: 'preloaded_tool', title: null, inputSchema: ['type' => 'object', 'properties' => [], 'required' => null], description: 'A preloaded tool', annotations: null), + static fn (): string => 'result', + ); + + Server::builder() + ->setRegistry($registry) + ->addLoader($loader) + ->build(); + + $this->assertSame(0, $loader->calls); + } } /**