diff --git a/lib/Service/Mcp/ToolRegistryFacade.php b/lib/Service/Mcp/ToolRegistryFacade.php index e32c4705c..cd6749a79 100644 --- a/lib/Service/Mcp/ToolRegistryFacade.php +++ b/lib/Service/Mcp/ToolRegistryFacade.php @@ -99,7 +99,27 @@ public function __construct( * LLM provider. Mirrors what ToolManagementHandler does internally for * the in-app chat path, as a public stable read. * - * @param array $toolWhitelist Optional whitelist of registry ids + * The whitelist is matched PER FUNCTION, the same dual-form way invokeTool() + * resolves a call: an entry may name a function's LLPhant-safe `name` + * (`list_schemas`), its dotted `mcpId` (`openbuild.upsertSchema`), or the + * registry id of the owning tool (`openregister.schema`, which admits all of + * that tool's functions). + * + * Matching per FUNCTION rather than per TOOL is deliberate and load-bearing. + * A registry id such as `openregister.schema` is ONE tool exposing five + * functions (list/get/create/update/delete_schema), and most built-in + * descriptors carry no `mcpId` at all — so intersecting the whitelist against + * registry ids made the two id spaces disagree in BOTH directions: an entry + * naming a real function (`list_schemas`) matched nothing and silently + * resolved to zero tools, while an entry naming a tool handed over every + * function it owns — `delete_schema` included — defeating a caller's + * read-only intent. Consumers store function-level ids (an ADR-035 + * toolWhitelist, Hermiq's per-agent grants) and invokeTool() has always + * resolved them function-level; this is listTools() keeping the contract the + * rest of the class already documents. + * + * @param array $toolWhitelist Optional whitelist of function names, + * dotted mcpIds and/or registry ids * ({appId}.{toolName}). Empty = all * discovered tools allowed (ADR-035 * Decision 4 default semantics). @@ -112,8 +132,18 @@ public function listTools(array $toolWhitelist=[]): array { $descriptors = []; - foreach ($this->resolveRegisteredTools(toolWhitelist: $toolWhitelist) as $tool) { + foreach ($this->resolveRegisteredTools(toolWhitelist: []) as $registryId => $tool) { foreach ($tool->getFunctions() as $function) { + $allowed = $this->functionIsWhitelisted( + function: $function, + registryId: (string) $registryId, + toolWhitelist: $toolWhitelist + ); + + if ($allowed === false) { + continue; + } + $descriptors[] = $function; } } @@ -121,6 +151,42 @@ public function listTools(array $toolWhitelist=[]): array return $descriptors; }//end listTools() + /** + * Whether one function descriptor is admitted by a whitelist. + * + * An empty whitelist admits everything (ADR-035 Decision 4). Otherwise the + * entry must name the function's `name`, its dotted `mcpId`, or the registry + * id of the tool that owns it. + * + * @param array $function The LLPhant function descriptor. + * @param string $registryId Registry id of the owning tool. + * @param array $toolWhitelist The whitelist to apply. + * + * @return bool True when the descriptor should be listed. + * + * @spec openspec/specs/ai-mcp/spec.md + */ + private function functionIsWhitelisted(array $function, string $registryId, array $toolWhitelist): bool + { + if ($toolWhitelist === []) { + return true; + } + + $candidates = [$registryId]; + + $name = ($function['name'] ?? null); + if (is_string($name) === true && $name !== '') { + $candidates[] = $name; + } + + $mcpId = ($function['mcpId'] ?? null); + if (is_string($mcpId) === true && $mcpId !== '') { + $candidates[] = $mcpId; + } + + return (array_intersect($candidates, $toolWhitelist) !== []); + }//end functionIsWhitelisted() + /** * Invoke a tool function by its descriptor name or dotted mcpId. * diff --git a/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php b/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php index 1d02b6d33..b5f992738 100644 --- a/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php +++ b/tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php @@ -193,6 +193,138 @@ public function testListToolsNarrowsByWhitelist(): void $this->assertSame('decidesk_listMeetings', $result[0]['name']); }//end testListToolsNarrowsByWhitelist() + /** + * A whitelist naming a FUNCTION resolves it — the id space consumers + * actually store. + * + * Regression: the whitelist used to be intersected against registry ids, so + * an entry naming a real function of a multi-function tool (`list_schemas`, + * exactly what the agent tool-catalog offers) matched nothing and silently + * resolved to ZERO tools — indistinguishable from a tool-less agent. + * + * @return void + */ + public function testListToolsResolvesAWhitelistedFunctionName(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock( + [ + ['name' => 'list_schemas', 'description' => 'List', 'parameters' => []], + ['name' => 'delete_schema', 'description' => 'Delete', 'parameters' => []], + ] + ), + $this->metadata('openregister') + ); + + $result = $this->facade->listTools(['list_schemas']); + + $this->assertCount(1, $result); + $this->assertSame('list_schemas', $result[0]['name']); + }//end testListToolsResolvesAWhitelistedFunctionName() + + /** + * A whitelist naming one function does NOT drag in its tool's other + * functions. + * + * Regression (the other direction of the same defect): matching per TOOL + * meant a read-only intent silently handed over every function the tool + * owns, `delete_schema` included. + * + * @return void + */ + public function testListToolsDoesNotAdmitSiblingFunctionsOfAWhitelistedFunction(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock( + [ + ['name' => 'list_schemas', 'description' => 'List', 'parameters' => []], + ['name' => 'delete_schema', 'description' => 'Delete', 'parameters' => []], + ] + ), + $this->metadata('openregister') + ); + + $names = array_column($this->facade->listTools(['list_schemas']), 'name'); + + $this->assertNotContains('delete_schema', $names); + }//end testListToolsDoesNotAdmitSiblingFunctionsOfAWhitelistedFunction() + + /** + * A whitelist naming a tool's REGISTRY id still admits all of its functions + * (the pre-existing coarse-grained grant stays valid). + * + * @return void + */ + public function testListToolsRegistryIdStillAdmitsEveryFunctionOfThatTool(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock( + [ + ['name' => 'list_schemas', 'description' => 'List', 'parameters' => []], + ['name' => 'delete_schema', 'description' => 'Delete', 'parameters' => []], + ] + ), + $this->metadata('openregister') + ); + + $names = array_column($this->facade->listTools(['openregister.schema']), 'name'); + + $this->assertSame(['list_schemas', 'delete_schema'], $names); + }//end testListToolsRegistryIdStillAdmitsEveryFunctionOfThatTool() + + /** + * A whitelist naming a function's dotted `mcpId` resolves it — the third + * accepted form, and the one an ADR-035 toolWhitelist stores for bridged + * tools. + * + * @return void + */ + public function testListToolsResolvesAWhitelistedMcpId(): void + { + $this->registry->registerTool( + 'openbuild.upsertSchema', + $this->createToolMock( + [ + [ + 'name' => 'openbuild_upsertSchema', + 'mcpId' => 'openbuild.upsertSchema', + 'description' => 'Upsert', + 'parameters' => [], + ], + ] + ), + $this->metadata('openbuild') + ); + + $result = $this->facade->listTools(['openbuild.upsertSchema']); + + $this->assertCount(1, $result); + $this->assertSame('openbuild_upsertSchema', $result[0]['name']); + }//end testListToolsResolvesAWhitelistedMcpId() + + /** + * An unknown whitelist entry resolves to nothing — it must NOT fuzzy-match. + * + * `openregister.schemas` (plural) is not a real id; the tool is + * `openregister.schema`. Resolving to zero is correct here; making that zero + * VISIBLE is the consumer's job (Hermiq raises on it). + * + * @return void + */ + public function testListToolsUnknownWhitelistEntryResolvesToNothing(): void + { + $this->registry->registerTool( + 'openregister.schema', + $this->createToolMock([['name' => 'list_schemas', 'description' => 'List', 'parameters' => []]]), + $this->metadata('openregister') + ); + + $this->assertSame([], $this->facade->listTools(['openregister.schemas'])); + }//end testListToolsUnknownWhitelistEntryResolvesToNothing() + /** * An explicit empty whitelist means "all discovered tools allowed" * (hydra ADR-035 Decision 4 default semantics).