feat(mcp): take ownership of McpContext's scope support - #22
Merged
Conversation
Core\Front\Mcp\McpContext was declared in two packages at once — here and in dappcore/agent's php/Mcp/Transport/McpContext.php. Which one a consumer loads is decided by autoload order, and this copy wins, so agent's scope support was unreachable exactly where it mattered: a caller reaching hasScope() on the loaded class would have hit an undefined method. getScopes() and hasScope() come across with the helpers they need — scopeCandidates, resolveScopes, extractObjectValue, getterNames, currentRequest, normaliseScopes, canonicalScope — and the constructor gains an optional $scopeSource as its last parameter, so no existing call site changes. The scope source is resolved from whichever of these is available: an explicit source, the current plan, or the authenticated request's mcp_workspace_context attribute, which is how the HTTP transport carries it. One bug is fixed on the way rather than carried across. extractObjectValue() asked property_exists() before reading a property, and that answers true for private and protected properties as well — so reading one from out here raised "Cannot access private property" for any scope source holding its data privately, which is the commonest shape there is. get_object_vars(), which from outside the class returns only what is reachable, is now asked first. This is not a regression I introduced in the port: agent's own McpContextTest fails the same way today, four of its eight tests, and those failures are part of that repo's open count. The tests come with the methods. Coverage follows ownership — agent testing a class it does not own proves nothing about the copy a consumer actually loads — so six cases land here covering session scopes, an empty session, no source at all, a present scope, a missing scope, and scopes read off the authenticated request. Agent's file is deleted separately, in dAppCore/agent, so the methods exist here before they stop existing there. Suite: 21 failed, 309 passed, from 21 failed, 299 passed — six new tests plus four that were failing on the old code path, no change to the existing failures. Co-Authored-By: Virgil <virgil@lethean.io>
The previous message said "21 failed, 309 passed, from 21 failed, 299 passed — six new tests plus four that were failing on the old code path". Both halves were wrong, and I only caught it by re-measuring against main instead of trusting the baseline file I had been comparing with. 299 was stale, captured several merged PRs ago. Main is 21 failed / 303 passed, so this branch's 21 / 309 is exactly the six tests it adds and nothing else. There were no "four that were failing on the old code path" — not in this package. The four failures the visibility fix addresses are in dAppCore/agent's McpContextTest, against agent's copy of this class. This package had no test exercising that path at all, which is precisely why the bug survived: the copy that had the tests was the copy nobody loads. The fix and the tests stand as committed; only the arithmetic was wrong. A follow-up rather than an amend, because the branch was already pushed. Co-Authored-By: Virgil <virgil@lethean.io>
The scope tests came over with getScopes()/hasScope(). These are the rest, and leaving them behind would have deleted coverage rather than moved it: agent held the only tests for McpContext's session, plan and transport-callback behaviour and for the McpToolHandler contract, while this package owns the copy a consumer actually loads. Tested on one side, shipped from the other. Six cases: session and plan getters, callbacks being optional, callbacks forwarding notifications and session logs, the handler schema shape, a handler receiving the transport-agnostic context, and the interface exposing exactly its two contract methods. Twelve tests now cover this class here, where before there were none. Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 4 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 (4)
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 |
Snider
added a commit
to dAppCore/agent
that referenced
this pull request
Aug 8, 2026
php/Mcp/Transport/McpContext.php and its McpToolHandler contract declared
Core\Front\Mcp\McpContext and Core\Front\Mcp\Contracts\McpToolHandler — a
namespace this package maps nowhere. composer.json declares Core\Mcp\,
Core\Mod\Agentic\ and Core\Service\Agentic\, and nothing else, so
class_exists('Core\Front\Mcp\McpContext') is false here and always has been.
Their tests only ran because they hand-loaded the files:
mcpRequire('Mcp/Transport/McpContext.php');
which is the third manual-require-around-broken-autoload found in this repo
today, after loadAgenticPhpClass() and LivewireTestCase::livewireComponent().
The pattern is reliable evidence that a file cannot be reached the normal way.
dappcore/mcp declares the same two FQCNs and, unlike this package, actually
maps them — so in any consumer application mcp's copies are what load. Two
packages owning one class name is settled by autoload order, silently, and this
side lost twice over: shadowed where both were installed, unloadable where only
this one was.
The reference check found nothing else touching them. The fifty-odd other
toMcpContext hits in this repo are a model method of the same name on
AgentPlan, BrainMemory and friends — unrelated, and worth stating because a
careless grep would have read them as callers.
Agent's copy carried getScopes() and hasScope() that mcp's lacked. Those, and
every test in both files, moved to dAppCore/mcp#22 first, so the behaviour
exists in one place before it stops existing here. That PR also fixes a
property_exists() visibility bug in the scope helpers, which is why four of the
eight McpContextTest cases removed here were failing rather than passing.
Suite 127 failed / 1190 passed, from 131 / 1197 on a freshly measured main —
eleven tests removed, of which four were failing, and no other change.
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.
Pairs with dAppCore/agent#24, which deletes agent's copy. Methods and tests land here first, so the behaviour exists in one place before it stops existing in the other.
One class name, two owners
Core\Front\Mcp\McpContextwas declared in both packages — here and in agent'sphp/Mcp/Transport/McpContext.php. Which one a consumer loads is decided by autoload order, and this copy wins. So agent's scope support was unreachable exactly where it mattered: a caller reachinghasScope()on the loaded class would have hit an undefined method.getScopes()andhasScope()come across with their helpers, and the constructor gains an optional$scopeSourceas its last parameter, so no existing call site changes. Scopes resolve from an explicit source, the current plan, or the authenticated request'smcp_workspace_contextattribute — how the HTTP transport carries it.A bug fixed rather than carried across
extractObjectValue()calledproperty_exists()before reading a property. That answers true for private and protected properties too, so reading one from outside raised "Cannot access private property" for any scope source holding its data privately — the commonest shape there is.get_object_vars(), which from outside the class returns only what is reachable, is now asked first.Not a regression introduced by the port: agent's own
McpContextTestfails the same way today, four of its eight tests, and those failures are part of that repo's open count.Coverage follows ownership
Agent held the only tests for this class while this package shipped the copy consumers load — tested on one side, shipped from the other. All of it moves:
McpToolHandlerexposing exactly its two methodsTwelve tests now cover this class here, where before there were none.
Verification
One commit in this branch is a correction: I first reported the delta against a stale baseline captured several merged PRs earlier, and claimed four existing tests started passing. Re-measuring against main showed 303, not 299, and no such four — those four failures are agent's. The arithmetic was wrong; the fix and tests were not. Corrected in a follow-up rather than an amend, since the branch was pushed.
🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io