fix(mcp): one tool registry, filled at boot, read by the server - #23
Merged
Conversation
Absorbs listTools/resolve/buildDependencyGraph/call into AgentToolRegistry, rewires McpAgentServerCommand and ToolDependencyService onto it, and deletes Mcp\Services\ToolRegistry. Deliberately NOT pushed. Moving the fill off $listens — the other half of the fix — makes register() construct the tool classes, which fatals on the missing Core\Mcp\Tools\Concerns\ValidatesDependencies trait and takes the suite from 156 failed to 1321. The registries can only usefully merge once agent consumes dappcore/mcp and the tools become constructible. 19 tests still fail here: their fixture is a duck-typed anonymous class and the surviving registry requires a real AgentToolInterface. Migrating them belongs with the change that turns the server on.
# Conflicts: # php/Mcp/Services/ToolDependencyService.php # php/Services/AgentToolRegistry.php
The agent MCP server advertised no tools. Three faults, each of which alone was enough, and a green suite that noticed none of them. Boot filled Core\Mod\Agentic\Services\AgentToolRegistry. McpAgentServerCommand read Core\Mod\Agentic\Mcp\Services\ToolRegistry — a different class, never bound, so Laravel handed the command a fresh empty instance on every resolution. tools/list returned []; tools/call found nothing. Two registries meant two answers to "what tools exist", and the server asked the one nobody filled. Boot filled its registry from the McpToolsRegistering event via $listens, which ModuleScanner populates by scanning app/Core|Mod|Website. Under vendor/ that is dead, so the event never fired and the registry it did fill was empty anyway. And every tool class was fatal on load until #21, so even a correct registration would have thrown on the first `new`. That one masked the other two: nothing ever tried to construct a tool, so nothing ever failed loudly. ToolRegistry is deleted and its capability absorbed: listTools(), resolve() and buildDependencyGraph() return ToolMetadata built from the registered tools, call() invokes one without the permission and dependency checks execute() applies — kept separate because the stdio transport has no API key to check scopes against and runs its own quota and audit passes around it. The duplicate-name guard comes across too: two tools claiming one name is a wiring mistake, and silently keeping the last one means the surface serves whichever file loaded second. The fill moves from the event into register(), the same lifecycle-independent path used for resources, and is idempotent so a host that still delivers the event cannot double-register. Nineteen tests registered duck-typed anonymous classes into the loose registry. They now implement AgentToolInterface — which they arguably always should have, since it is the contract the tools they stand in for satisfy. One test goes rather than being migrated: it asserted that a payload without a callable handler is rejected, and register() is now typed, so no array can reach that validation. There is no code path left that produces the behaviour it asserted. Guarded against recurrence by the test that was missing all along: on a plain booted application, registering nothing of its own, a tool constructs, the registry is non-empty, listTools() contains plan_create, session_start and brain_remember, and the binding is one shared instance. McpAgentServerCommandTest passed throughout the outage because its beforeEach supplied a tool — it tested the plumbing with a registry the test had filled, which is precisely the blind spot. Receipts: registry holds 40 tools after a real boot, listTools() returns the same 40, plan_create among them. Suite 131 failed / 1193 passed, from 131 / 1190 — four guards added, one obsolete test removed, zero regressions confirmed by diffing failing test names. Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughThe change removes ChangesAgent tool registry migration
Sequence Diagram(s)sequenceDiagram
participant ApplicationBoot
participant AgentToolRegistry
participant McpAgentServerCommand
ApplicationBoot->>AgentToolRegistry: Register agent tools
McpAgentServerCommand->>AgentToolRegistry: Request tool metadata
AgentToolRegistry-->>McpAgentServerCommand: Return ToolMetadata list
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@php/Boot.php`:
- Around line 235-240: Update the registration logic in Boot.php so it checks
each built-in tool name individually instead of returning whenever
Registry::all() is non-empty. Register any missing built-in tools even when
custom tools already exist, while preserving the duplicate-name failure when an
existing tool with a built-in name is a different implementation.
In `@php/Services/AgentToolRegistry.php`:
- Around line 430-452: Update AgentToolRegistry::call() to validate dependencies
through ToolDependencyService::validateDependencies(...) before invoking the
tool, then record the call with recordToolCall(...) only after handle()
succeeds. Preserve the existing unknown-tool exception and direct invocation
behavior while ensuring the stdio tools/call path cannot bypass dependency
checks or recording.
In `@php/tests/Feature/Mcp/Services/AgentToolRegistryBootTest.php`:
- Around line 52-55: Type the callback parameter in the array_map call within
AgentToolRegistryBootTest using the ToolMetadata class returned by listTools(),
and add the corresponding import. Preserve the existing string return type and
mapping behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 25b6d1fc-2bd5-4b0e-9fb7-3e2ff2bf8ce5
📒 Files selected for processing (11)
php/Boot.phpphp/Mcp/Console/McpAgentServerCommand.phpphp/Mcp/Services/ToolDependencyService.phpphp/Mcp/Services/ToolRegistry.phpphp/Services/AgentToolRegistry.phpphp/tests/Feature/Mcp/Console/McpAgentServerCommandTest.phpphp/tests/Feature/Mcp/Middleware/McpAuthenticateTest.phpphp/tests/Feature/Mcp/Middleware/ValidateToolDependenciesTest.phpphp/tests/Feature/Mcp/Services/AgentToolRegistryBootTest.phpphp/tests/Feature/Mcp/Services/ToolDependencyServiceTest.phpphp/tests/Feature/Mcp/Services/ToolRegistryTest.php
💤 Files with no reviewable changes (1)
- php/Mcp/Services/ToolRegistry.php
Comment on lines
+235
to
+240
| // Idempotent: register() runs once per application, but a host that | ||
| // still delivers the event must not double-register and trip the | ||
| // duplicate-name guard. | ||
| if ($registry->all()->isNotEmpty()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Register missing built-in tools when the registry has custom tools.
At Line 238, one unrelated pre-registered tool makes this method return. If a host registers a custom tool before this provider, the built-in MCP tools are never registered.
Check registration per built-in tool. Keep the duplicate-name failure when a different implementation claims a built-in name.
Proposed fix
- if ($registry->all()->isNotEmpty()) {
- return;
- }
-
$toolClasses = [
// ...
];
- $registry->registerMany(array_map(
- static fn (string $toolClass) => new $toolClass,
- $toolClasses,
- ));
+ foreach ($toolClasses as $toolClass) {
+ $tool = new $toolClass;
+ $existing = $registry->get($tool->name());
+
+ if ($existing === null) {
+ $registry->register($tool);
+ continue;
+ }
+
+ if ($existing::class !== $tool::class) {
+ throw new \InvalidArgumentException(sprintf(
+ 'Tool [%s] is already registered by [%s].',
+ $tool->name(),
+ $existing::class,
+ ));
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@php/Boot.php` around lines 235 - 240, Update the registration logic in
Boot.php so it checks each built-in tool name individually instead of returning
whenever Registry::all() is non-empty. Register any missing built-in tools even
when custom tools already exist, while preserving the duplicate-name failure
when an existing tool with a built-in name is a different implementation.
Comment on lines
+430
to
+452
| /** | ||
| * Invoke a tool directly, without the permission and dependency checks | ||
| * execute() applies. | ||
| * | ||
| * Kept distinct from execute() rather than merged into it: the stdio | ||
| * transport has no API key to check scopes against and runs its own quota | ||
| * and audit passes around this call, whereas execute() is the governed | ||
| * path used where an ApiKey is present. | ||
| * | ||
| * @throws \InvalidArgumentException If the tool is not registered | ||
| * | ||
| * @example | ||
| * $registry->call('plan_list', [], ['workspace_id' => 'ws-1']); | ||
| */ | ||
| public function call(string $name, array $arguments = [], array $context = []): mixed | ||
| { | ||
| $tool = $this->get($name); | ||
|
|
||
| if (! $tool) { | ||
| throw new \InvalidArgumentException(sprintf('Unknown tool [%s].', $name)); | ||
| } | ||
|
|
||
| return $tool->handle($arguments, $context); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline php/Mcp/Console/McpAgentServerCommand.php --match handleToolCall --view expanded
rg -n -C 6 \
'function handleToolCall|validateDependencies|recordToolCall|\$toolRegistry->call' \
php/Mcp/Console/McpAgentServerCommand.php \
php/Mcp/Services/ToolDependencyService.phpRepository: dAppCore/agent
Length of output: 5562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '232,325p' php/Mcp/Console/McpAgentServerCommand.php
printf '\n--- Tool dependency service full missing methods ---\n'
sed -n '1,135p' php/Mcp/Services/ToolDependencyService.php
printf '\n--- AgentToolRegistry outline and relevant sections ---\n'
ast-grep outline php/Services/AgentToolRegistry.php --view expanded
sed -n '1,140p' php/Services/AgentToolRegistry.php
sed -n '390,470p' php/Services/AgentToolRegistry.phpRepository: dAppCore/agent
Length of output: 14941
Add dependency validation and call recording to handleToolCall().
The stdio tools/call path calls $toolRegistry->call() directly, while call() intentionally skips dependency checks. Add ToolDependencyService::validateDependencies(...) before the invocation and recordToolCall(...) after a successful call so MCP clients cannot bypass dependency ordering and tool-call recording.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@php/Services/AgentToolRegistry.php` around lines 430 - 452, Update
AgentToolRegistry::call() to validate dependencies through
ToolDependencyService::validateDependencies(...) before invoking the tool, then
record the call with recordToolCall(...) only after handle() succeeds. Preserve
the existing unknown-tool exception and direct invocation behavior while
ensuring the stdio tools/call path cannot bypass dependency checks or recording.
Comment on lines
+52
to
+55
| $names = array_map( | ||
| static fn ($tool): string => $tool->name, | ||
| $this->app->make(AgentToolRegistry::class)->listTools(), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Type the callback parameter.
$tool has no type hint. listTools() returns ToolMetadata instances, so type the parameter and import ToolMetadata.
Proposed fix
+use Core\Mod\Agentic\Mcp\Data\ToolMetadata;
use Core\Mod\Agentic\Mcp\Tools\Agent\Brain\BrainRemember;
- static fn ($tool): string => $tool->name,
+ static fn (ToolMetadata $tool): string => $tool->name,As per coding guidelines, php/**/*.php requires type hints for all function parameters and return types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $names = array_map( | |
| static fn ($tool): string => $tool->name, | |
| $this->app->make(AgentToolRegistry::class)->listTools(), | |
| ); | |
| $names = array_map( | |
| static fn (ToolMetadata $tool): string => $tool->name, | |
| $this->app->make(AgentToolRegistry::class)->listTools(), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@php/tests/Feature/Mcp/Services/AgentToolRegistryBootTest.php` around lines 52
- 55, Type the callback parameter in the array_map call within
AgentToolRegistryBootTest using the ToolMetadata class returned by listTools(),
and add the corresponding import. Preserve the existing string return type and
mapping behavior.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The agent MCP server advertised no tools. Three faults, each alone sufficient, and a green suite that noticed none of them.
The three
1. Two registries, and the server read the empty one. Boot filled
Services\AgentToolRegistry.McpAgentServerCommandreadMcp\Services\ToolRegistry— a different class, never bound, so Laravel handed the command a fresh empty instance on every resolution.tools/listreturned[];tools/callfound nothing.2. The fill hung off a dead event.
$listensis populated by ModuleScanner scanningapp/Core|Mod|Website, so undervendor/it never fires — and the registry it would have filled was the wrong one anyway.3. Every tool class was fatal on load until #21. That one masked the other two: nothing ever tried to construct a tool, so nothing ever failed loudly.
What lands
ToolRegistryis deleted, its capability absorbed into the survivor:listTools(),resolve(),buildDependencyGraph()—ToolMetadatabuilt from registered toolscall()— invokes without the permission/dependency checksexecute()applies, kept separate because the stdio transport has no API key to check scopes against and runs its own quota and audit passes around itThe fill moves from the event into
register()— the same lifecycle-independent path used for resources — and is idempotent, so a host that still delivers the event cannot double-register.Tests
Nineteen registered duck-typed anonymous classes into the loose registry; they now implement
AgentToolInterface, which they arguably always should have.One test is removed rather than migrated, with the reason: it asserted a payload without a callable handler is rejected.
register()is now typed, so no array can reach that validation — there is no code path left that produces the behaviour it asserted.The guard that was missing all along
On a plain booted application, registering nothing of its own: a tool constructs, the registry is non-empty,
listTools()containsplan_create/session_start/brain_remember, and the binding is one shared instance.McpAgentServerCommandTestpassed throughout the entire outage because itsbeforeEachsupplied a tool — it tested the plumbing with a registry the test had filled. That is precisely the blind spot.Receipts
listTools()(server read path)plan_createamong themFour guards added, one obsolete test removed. Gate re-verified after linting.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit