Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 68 additions & 2 deletions lib/Service/Mcp/ToolRegistryFacade.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<int,string> $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<int,string> $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).
Expand All @@ -112,15 +132,61 @@ 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;
}
}

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<string,mixed> $function The LLPhant function descriptor.
* @param string $registryId Registry id of the owning tool.
* @param array<int,string> $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.
*
Expand Down
132 changes: 132 additions & 0 deletions tests/Unit/Service/Mcp/ToolRegistryFacadeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
Loading