fix(mcp): roll the dependency contracts, so the tool classes can be built - #21
Merged
Conversation
…uilt
AgentTool does `use ValidatesDependencies` and `implements HasDependencies`,
and neither symbol has ever existed in this repository — they live in
dappcore/mcp, which this package deliberately does not depend on. Every class
extending AgentTool was therefore fatal on load:
php -r 'new \Core\Mod\Agentic\Mcp\Tools\Agent\Brain\BrainRemember();'
FATAL: Trait "Core\Mcp\Tools\Concerns\ValidatesDependencies" not found
All forty registered tool classes, unconstructible in every environment. Not
"unregistered" — unbuildable. The dead $listens event registration that meant
nothing ever tried to instantiate one was anaesthetic, not a second bug: make
registration live without this and the suite goes from 156 failures to 1321.
The design is a local copy of Core\Mcp, not a dependency on it, so the fix is
to finish the copy. Three contracts roll in unchanged from dappcore/mcp, which
now holds the canonical versions: DependencyType, ToolDependency and
HasDependencies. Each gains the SPDX header this repo requires.
Two could NOT be rolled verbatim, because upstream's and this repo's
ToolDependencyService share a name and not an API.
MissingDependencyException upstream takes
(string $toolName, array $missingDependencies, array $suggestedOrder) and
composes its own message. This service raises it as `new $exceptionClass($message)`
— one argument. Copying the upstream signature would have replaced a
class-not-found with an ArgumentCountError the first time a dependency went
unmet. It is written message-first, with the detail as optional arguments.
ValidatesDependencies upstream calls checkDependencies() and
getMissingDependencies(), which do not exist here — the equivalents are
canExecute() and missing() — and passes named arguments to a method declared
`validateDependencies(mixed ...$arguments)`, where they bind to nothing. It is
written against the API this repo actually has, positionally, and returns the
service's own {tool, type, key, message} rows rather than pretending to hand
back ToolDependency objects.
Two supporting fixes fall out. AgentToolRegistry imported
Core\Mcp\Services\ToolDependencyService, which resolves to this repo's
php/Mcp/Services/ToolDependencyService.php — a file declaring
Core\Mod\Agentic\Mcp\Services\ToolDependencyService — so autoloading it raised
"Cannot redeclare". It now imports the class that is actually there. And
normaliseDependency() met ToolDependency objects with get_object_vars(), which
hands the array branch a DependencyType enum where it casts to string, and
loses the text because the object calls it description where the service reads
message; it now goes through toArray() and maps the field across.
Sixteen tools declare real dependencies via ToolDependency::contextExists(),
so this path is live, not hypothetical.
Receipts: all 40 tool classes construct, verified by instantiating every one.
Suite 131 failed / 1190 passed, from 156 / 1165 — exactly the 25-test
ValidatesDependencies cluster fixed, zero new failures, confirmed by diffing
failing test names either side.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced Aug 8, 2026
Snider
added a commit
that referenced
this pull request
Aug 8, 2026
* wip: consolidate tool registries (blocked on Core\Mcp contracts) 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. * fix(mcp): one tool registry, filled at boot, read by the server 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>
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.
All 40 MCP tool classes have been unconstructible in every environment. Not unregistered — unbuildable.
AgentTooldoesuse ValidatesDependenciesandimplements HasDependencies, and neither symbol has ever existed in this repository. They live indappcore/mcp, which this package deliberately does not depend on.The dead
$listensevent registration — which meant nothing ever tried to instantiate a tool — was anaesthetic, not a second bug. Make registration live without this fix and the suite goes from 156 failures to 1321.The fix
The design is a local copy of
Core\Mcp, not a dependency on it, so the fix is to finish the copy. Three contracts roll in unchanged fromdappcore/mcp(now canonical after mcp#20):DependencyType,ToolDependency,HasDependencies, each gaining the SPDX header this repo requires.Two could not be rolled verbatim
Upstream's
ToolDependencyServiceand this repo's share a name and not an API.MissingDependencyException— upstream takes(string $toolName, array $missingDependencies, array $suggestedOrder)and composes its own message. This service raises it asnew $exceptionClass($message), one argument. Copying the upstream signature would have replaced a class-not-found with anArgumentCountErrorthe first time a dependency went unmet. Written message-first, detail optional.ValidatesDependencies— upstream calls methods that don't exist here, and passes named arguments to a variadic:checkDependencies(...)canExecute($toolId, $context, $args, $session)getMissingDependencies(...)missing($toolId, $context, $args, $session)validateDependencies(sessionId:, toolName:, args:)validateDependencies(mixed ...$arguments)— named args bind to nothingWritten against the API this repo actually has, positionally, returning the service's own
{tool, type, key, message}rows rather than pretending to hand backToolDependencyobjects.Two supporting fixes fell out
AgentToolRegistryimportedCore\Mcp\Services\ToolDependencyService, which resolves to this repo'sphp/Mcp/Services/ToolDependencyService.php— a file declaringCore\Mod\Agentic\Mcp\Services\ToolDependencyService— so autoloading it raised "Cannot redeclare". It now imports the class that is actually there.normaliseDependency()metToolDependencyobjects withget_object_vars(), handing the array branch aDependencyTypeenum where it casts to string, and losing the text because the object calls itdescriptionwhile the service readsmessage. It now goes throughtoArray()and maps the field across.Sixteen tools declare real dependencies via
ToolDependency::contextExists(), so this path is live, not hypothetical.Receipts
ValidatesDependenciesclusterGate re-verified after linting, not before.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io