refactor(mcp): stop importing the consumer, and drop what agent already owns - #19
Merged
Conversation
…dy owns
This package imported Core\Mod\Agentic\Models\* in four files while
dappcore/agent is meant to depend on it — a library importing the thing that
imports it. AX-8: the arrow points one way.
Three files go because agent already owns them, verified rather than assumed:
AgentToolRegistry agent's has all 13 shared methods plus 6 more, none
only here. Unreferenced in this package — even
McpApiController reaches for agent's FQCN, not this.
AgentSessionService agent's has all 20 shared plus replay and
getReplayContext, none only here. Zero references.
McpAgentServerCommand 2064 lines. Its 29 tool methods each map to an
extracted class in agent's Mcp\Tools\Agent, and
agent's Boot::onMcpTools registers every one — checked
for registration, not just for the files existing.
Its five resource handlers were the one thing agent did NOT have: agent's
server answered resources/list with a hardcoded empty array and had no
resources/read. Deleting this first would have silently dropped
plans://all, plans://{slug}, plans://{slug}/phases/{order},
plans://{slug}/state/{key} and sessions://{id}/context. They were ported to
agent first, in dAppCore/agent#20, and only then is this removed.
McpApiController carried a third copy of the same rendering, and that is what
made the cycle a compile-time one. The plan and session renderers are replaced
by AgentResourceProvider, an interface this package owns and the module that
owns the data implements, resolved from the container and absent without
complaint when agent is not installed. Both model imports go with them.
Interim worth stating plainly: nothing is bound to AgentResourceProvider yet,
so this package's HTTP plans:// and sessions:// endpoints answer "not found"
until agent binds its registry to it. Agent implementing the interface needs
agent to depend on dappcore/mcp, which is still blocked on the Core\Mcp\ PSR-4
root being claimed by both packages. The data itself is not lost — agent's own
MCP server serves all five URIs as of #20; it is the HTTP mirror here that is
degraded until that lands.
Two soft couplings remain by design, both container lookups by string with no
import and a guard: AgentToolRegistry in executeTool and AgentApiKeyService in
McpAuthenticate. Neither is a compile-time dependency and neither creates a
composer cycle; both would be better behind interfaces, and are left for when
the namespace collision is resolved.
Suite unchanged: 21 failed, 299 passed. 2786 deletions, 102 insertions.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 40 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 (6)
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 |
This was referenced Aug 8, 2026
Snider
added a commit
that referenced
this pull request
Aug 8, 2026
…ally (#21) #19 moved the plans:// and sessions:// rendering behind AgentResourceProvider and I described the dead endpoints as an interim, waiting on dappcore/agent to depend on this package and implement the interface. That day does not come: agent maintains its own copy of Core\Mcp rather than depending on this package, so it can never name the interface in an implements clause. An interim with no end is a decision that has not been made, so it is made here. The controller now accepts any object bound under the interface name that provides read(). A provider able to name the interface still implements it and is accepted nominally; one that cannot binds and satisfies the method. Both work, neither package imports the other, and two endpoints that would otherwise have stayed dead permanently come back on as soon as agent binds. Half a contract is rejected rather than accepted: an object with neither read() nor the interface returns null, so the controller answers a clean not-found instead of fatalling mid-request. The contract also loses entries(). Nothing in this package ever called it — GET servers/{id}/resources lists a server's own configured resources, which is a different concept — so it was a method an implementer had to satisfy for no consumer. Providers are still free to offer listing for their own transports; agent's registry does, for its stdio server. This package asks only for what it uses. Four tests cover the resolution: a duck-typed binding with no implements clause, a nominal implementation, an object missing read(), and nothing bound at all. Suite: 21 failed, 303 passed, from 21 failed, 299 passed — the four new tests, no change to the existing failures.
Snider
added a commit
that referenced
this pull request
Aug 8, 2026
…lasses (#20) Step 1 of letting dappcore/agent consume this package instead of forking it. Eleven classes exist in both repos under the same Core\Mcp namespace, because agent maps Core\Mcp\ onto its own php/Mcp/ as well. The public API is identical on all eleven — same methods, same constants — and every difference is in protected helpers: this package prefixes them with get (getStateKey, getCacheKey, getRegisteredServers) where agent does not, and agent extracts a few more. Provenance settles which way to merge. These files date from this package's 2026-03-09 initial commit; agent's landed 2026-04-25 as "implement extended RFC services + transport" and were then hardened on 2026-04-27 by "address CodeRabbit + SonarCloud findings". Agent's are the downstream, newer, reviewed copies, so they come upstream rather than being overwritten by ours — SPOR with best-version-wins, not first-writer-wins. Chased the one candidate for the other direction: OpenApiGenerator:: buildSchemas exists only here. It is not lost — agent emits the identical schemas block inline inside buildComponents rather than through an extracted helper. No behaviour only this side had. AX-8 verified before copying, not assumed: none of the eleven imports Core\Mod\Agentic. Their only non-framework imports are Core\Tenant, Core\Mod\Content and Laravel\Mcp, all of which this package already uses. php-content is now declared for the first time — ContentResource has always imported Core\Mod\Content\Models\ContentItem without it being in composer.json. CircuitBreaker arrives with its namespace normalised from the braced `namespace X { ... }` form, which was the only file in either repo using it. One correction to #19, which I got wrong. That PR was titled "stop importing the consumer" and it introduced one: running pint over the changed files let fully_qualified_strict_types rewrite the inline \Core\Mod\Agentic\Services\AgentToolRegistry::class in executeTool into a top-of-file use statement. The coupling never changed — it is still a guarded, late-bound app()->bound() lookup that autoloads nothing — but the source did, and the claim in that PR body was wrong as written. It is now a string literal, which expresses the late binding honestly and which no formatter will promote into an import. SPDX headers come across with the files: 13 of 132 files now carry one, from 2. Suite unchanged: 21 failed, 299 passed.
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.
PR 2 of 2 in the dependency-cycle break. dAppCore/agent#20 landed the capability first; this removes the copy. 2786 deletions, 102 insertions.
This package imported
Core\Mod\Agentic\Models\*in four files whiledappcore/agentis meant to depend on it — a library importing the thing that imports it.Three files go, because agent already owns them
Verified rather than assumed:
AgentToolRegistryMcpApiControllerreaches for agent's FQCN, not this one.AgentSessionServicereplayandgetReplayContext, none only here. Zero references.McpAgentServerCommandtool*methods each map to an extracted class in agent'sMcp\Tools\Agent, and agent'sBoot::onMcpToolsregisters every one — I checked registration, not just that the files exist.Why the capability moved first
The command's five
resource*handlers were the one thing agent did not have — its server answeredresources/listwith a hardcoded empty array and had noresources/readat all. Deleting this first would have silently dropped:They were ported to agent in #20, and only then is this removed.
The interface split
McpApiControllercarried a third copy of the same rendering, and that is what made the cycle compile-time. The plan and session renderers are replaced byAgentResourceProvider— an interface this package owns, implemented by the module that owns the data, resolved from the container and absent without complaint when agent isn't installed. Both model imports go with them.Interim, stated plainly
Nothing is bound to
AgentResourceProvideryet, so this package's HTTPplans://andsessions://endpoints answer "not found" until agent binds its registry to it. Agent implementing the interface requires agent to depend ondappcore/mcp, which is still blocked on theCore\Mcp\PSR-4 root being claimed by both packages.The data is not lost: agent's own MCP server serves all five URIs as of #20. It is the HTTP mirror here that is degraded until the namespace work lands.
Left by design
Two soft couplings remain — container lookups by string, no import, both guarded:
AgentToolRegistryinexecuteToolandAgentApiKeyServiceinMcpAuthenticate. Neither is a compile-time dependency and neither creates a composer cycle. Both would be better behind interfaces, and are left for when the namespace collision is resolved.Verification
Suite unchanged: 21 failed, 299 passed — identical to
main.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io