feat(mcp)!: migrate to ModelContextProtocol 2.2.0 — requires SDK 2.x, changes IMcpFeedback.SendMessageAsync, and makes the 2026-07-28 tool list invariant - #71
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cb6d8d09b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Adversarial review pass applied:
|
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
I validated the current head locally: strict Release build is clean (0 warnings / 0 errors), and the full suite passes with 1,314 total / 1,313 passed / 1 known Inspector skip. The SDK migration itself is small, but the 2.0 request model exposes one blocking concurrency regression plus three contract/documentation gaps.
The blocker is the shared mutable MCP service overlay: with the default 2026-07-28 protocol, every request has its own destination-bound request.Server, but AttachServer overwrites _roots, _sampling, _elicitation, and _feedback shared by concurrent invocations. I reproduced this deterministically: request A advertised Sampling and paused; request B without Sampling ran; when A resumed it observed not-supported.
The existing Codex thread is also still partially unresolved: CommandAnnotations.LongRunning, CommandAnnotationsBuilder.LongRunning(), and CommandBuilder.LongRunning() continue to promise task-based programmatic execution, although this PR deliberately emits no Tasks/Execution signal. Please describe it as Repl-local metadata until #72 is implemented.
Non-blocking release note: a stable package with ModelContextProtocol 2.0.0-preview.3 fails pack with NU5104. The PR caveat already acknowledges the required re-pin before stable 0.12; keeping main on 0.12.0-dev.* is fine, but this should remain an explicit release gate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c235f9a90e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cc415ae43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Re-reviewed current head 3cc415ae. The original request-binding blocker is fixed: the new AsyncLocal regression passes, the pinned 2025-11-25 legacy handshake/list/call regression passes, the strict MCP test build is clean (0 warnings / 0 errors), and the documentation/.LongRunning() contract corrections are present. CI is green, including the required Build, Test, Pack check.
Two cross-session lifecycle defects remain and are independently reproduced in the existing threads:
- Roots cache/snapshot affinity: after session A loads
root-a, session B configured withroot-breceives"root-a:file:///root-a". This leaks one client workspace into another and can contaminate root-dependent tool discovery. - Routing subscription lifetime: after session A closes, routing invalidation no longer produces
tools/list_changedfor still-active session B; the deterministic probe times out after 2 seconds.
Please key Roots and root-dependent snapshots by destination/session, and track active session notification/subscription lifetimes so one session cleanup cannot disable another. The migration is materially better, but these remain blocking before merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b094d05506
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Re-reviewed current head b094d055. The two previous blockers are genuinely fixed: the new hard-roots isolation regression and surviving-session routing-notification regression both pass locally; strict MCP test build is clean (0 warnings / 0 errors); CI is green, including the required Build, Test, Pack check.
Three remaining handler-global states still violate the new multi-session model, and all three are independently reproduced in the existing threads:
- Soft roots leak: after session A sets
file:///root-a, session B reads the same URI through its own command. - Generated snapshot affinity: with A advertising Roots and B not advertising Roots, A first
tools/listreturns theroots_unsupportedmodule, showing B session state influenced A generation; the single_snapshotalso caches the cross-session result. - Compatibility bootstrap affinity: in
DiscoverAndCallShimmode, A receivesdiscover_tools/call_tool, while B first list receives onlyechobecause A consumed the handler-global bootstrap flag.
Please move all session-sensitive state — hard and soft roots, generated snapshots/versioning, and compatibility intro state — behind one coherent per-session context. The latest commit is another real improvement, but these remain blocking cross-session correctness/data-isolation issues.
Architecture follow-up: relation to #70There is a strong architectural relationship between this PR's multi-session findings and #70, but they cover different lifetime boundaries:
A hosted/MCP session can process multiple requests concurrently, so a DI scope per session does not replace request-level binding to Recommended splitRequired in this PRIntroduce one coherent internal MCP session container, for example: internal sealed class McpSessionContext
{
public McpClientRootsService Roots { get; }
public McpSnapshotCache Snapshot { get; }
public McpCompatibilityState Compatibility { get; }
public IServiceProvider Services { get; }
}The exact shape is flexible, but it should own all state that currently assumes one handler equals one session:
For Externally hosted Remains in #70#70 should still implement the general Repl lifetime contract:
Once #70 exists, Sequencing recommendationThe current cross-session leaks should not be merged with the expectation that #70 will repair them later: they are observable correctness/data-isolation bugs introduced by sharing one MCP handler across sessions. The pragmatic sequence is:
Implementing #70 first and rebasing this PR on it would also be architecturally valid, but #70 has a much broader surface and would significantly delay the SDK migration. The key requirement either way is that this PR must not merge while known MCP session state remains handler-global. |
|
Agreed on the layering table and the sequencing — this matches the plan on #70. Concretely:
You are right that the snapshot cache and |
|
McpSessionContext landed in 1f8950f, per the recommended split:
Both remaining handler-global leaks were RED-observed before the refactor with dedicated regressions: the roots-less session saw the roots-gated tool of the other session (snapshot cache), and only the first session received the Linked as Related to #70 — PR #74 implements the general session-scope contract, and this context will construct from the session-scoped provider once both merge. |
b362736 to
ddce4d4
Compare
carldebilly
changed the title
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Re-reviewed the exact current head ddce4d408466630cacc59d5c67b53cb92f6d8653 against base d4f039841568c27dbb820ddfece9ec488106cf48.
The previous request-binding, RunAsync hard/soft-roots isolation, snapshot contamination, compatibility-intro ownership, first-session-close lifecycle, legacy handshake, Tasks wording, and deprecation-guidance findings are fixed. I independently rebuilt with warnings as errors, ran the full solution (1,467 passed / 0 failed / 1 external-toolchain smoke skipped), packed Repl.Mcp, ran the concurrency tests repeatedly, and verified the exact-head CI provenance.
I still cannot approve the MCP 2026-07-28 migration. Four blockers remain:
- the documented reusable
BuildMcpServerOptions()path captures one context and demonstrably leaks soft roots across servers while losing request capabilities; - modern list results still vary by connection/first-call state;
- list-change notifications bypass
subscriptions/listen; - logging notifications ignore the modern per-request
logLevelopt-in.
Two additional medium findings cover a teardown false-green and modern soft-roots guidance. Each inline comment includes a deterministic reproduction and acceptance-test shape.
Primary references: MCP 2026-07-28 key changes and the exact ModelContextProtocol SDK 2.2.0 implementation.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Follow-up to review #4933578205 on the unchanged head. A late independent concurrency pass identified one additional cache-publication regression; the inline comment records it separately without duplicating the existing blockers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f561022a12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Bump ModelContextProtocol 1.4.1 -> 2.0.0-preview.3. - Remove the Tool.Execution mapping for .LongRunning() commands: SDK 2.0 dropped the experimental MCP Tasks tool augmentation (Tasks SEP deferred out of the 2.0 protocol release). The annotation stays in Repl's model; protocol-level task support returns with the SDK Tasks runtime. - Keep supporting Roots, Sampling, and Logging: deprecated by spec 2026-07-28 (SEP-2577, MCP9005) with no replacement, still relied on by current hosts. Scoped, documented pragmas at the feature touchpoints. - Document the SDK/protocol version posture in docs/mcp-reference.md. Full suite green against the new SDK (1312 passed, 1 known skip), including all MCP capability, tool-call, roots, sampling, and logging regressions. Note: re-pin to the stable 2.0.0 release before cutting stable 0.12. Refs #51
…e shape (review) - Correct the migration rationale: MCP Tasks was EXTRACTED to ModelContextProtocol.Extensions.Tasks (store, task results, client polling), not removed; the per-tool Tool.Execution augmentation is gone from the protocol surface. Comments and docs now say so, and Repl still deliberately does not advertise task support without the runtime. - Name the designated successor (SEP-2322 multi-round-trip requests) in the deprecation pragmas instead of claiming 'no replacement API'. - Narrow MCP9005 pragmas to their touchpoints in McpServerHandler and Given_McpIntegration (file-scoped kept only where usage is dense). - Lock the SDK-2.0 tools/list wire shape: a .LongRunning() tool serializes its annotations and emits no task/execution augmentation. - Align remaining .LongRunning() doc mentions (overview, coding-agents guide, package README) with the current no-advertisement posture.
…ed server field SDK 2.0's 2026-07-28 protocol path hands each request a destination-bound McpServer, and one handler can serve several sessions. The four capability services (roots, sampling, elicitation, feedback) stored the last-attached server in a shared mutable field, so a concurrent request from another session could cross-wire capabilities mid-call (IsSupported flipping while a handler was awaiting). - McpRequestServerAccessor: AsyncLocal request binding flowing with the invocation, session-level server as fallback for code outside a request (routing notifications, roots list-changed handler). - Services resolve the effective server through the accessor with a single read per operation (no torn check-then-use). - McpServerHandler splits session-level attach (RunAsync, once) from request-level binding (every handler); externally hosted servers adopt the first observed server for session concerns. - Deterministic regression: two sessions on one handler, sampling-capable client pauses mid-call while a sampling-less client is served — the paused call must keep observing ITS client's capabilities (RED observed: 'True|False' on the pre-fix code, exactly the reported repro).
… Tasks wording (review) - Regression pinning the last initialize-era protocol revision (2025-11-25): asserts the negotiated version and a tool list + call — the default client negotiates 2026-07-28 and never exercised the fallback path. - Roots/Sampling/Logging documented as legacy-compatibility only: deprecation notices in mcp-agent-capabilities.md and mcp-advanced.md steer new applications toward IReplInteractionChannel / soft roots; mcp-reference.md no longer reads as an endorsement. - Tasks wording corrected everywhere: the SDK has shipped the Tasks extension (ModelContextProtocol.Extensions.Tasks); what is pending is Repl's integration (issue #72) — including the CommandAnnotations.LongRunning XML doc that still promised task-based execution.
…le (review) - Hard roots are now SESSION state: entries keyed by destination server in a ConditionalWeakTable (weak keys die with the session), with a global version stamp for roots-list-changed invalidation. One session can no longer receive another session's cached workspace roots, and the root-dependent snapshot builds from the right workspace (RED observed: client B received client A's roots). - Session attachment is reference-counted: the handler tracks every active session, discovery notifications fan out to ALL of them, the accessor fallback moves to a surviving session on close, and the routing subscription is dropped only when the LAST session ends. A first-session close no longer silences the survivors (RED observed: surviving session timed out waiting for tools/list_changed). - Roots list-changed handler registered once per session (per-server registration replaces the single global flag).
One handler serves several sessions; everything that varied per client was still handler-global after the earlier point fixes. McpSessionContext now owns it all, per the architecture review: - hard AND soft roots: McpClientRootsService is one instance per session (plain fields again — the ConditionalWeakTable keying is gone); a session's 'workspace init' no longer sets another session's workspace. - generated snapshot + version + gate: the tool graph can be gated on session capabilities, so each session caches its own build against the handler-global routing version (RED observed: the roots-less session saw the roots-gated tool of the other session). - compatibility-shim intro: per-session flag, reset for every active session on routing invalidation (RED observed: only the first session received the discover_tools/call_tool intro). - per-session service overlay handed to McpServer.Create; request handlers recover their session through request.Server.Services instead of using a destination-bound per-request server as a surrogate session key. - externally hosted servers (BuildDynamicServerOptions) share one explicit lazy fallback context instead of racing a last-attached field. Request-bound OUTBOUND capabilities (sampling/elicitation/feedback) keep flowing through the per-request AsyncLocal accessor — finer than the session, unchanged. Related to #70 (per-session DI scopes generalize the lifetime contract; this context will construct from the session-scoped provider once both merge).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e30f0de788
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ce read A failed read has no body to carry what the handler reported, so the surfaced error is the only place left for it. Carrying it meant wrapping the failure in an McpException whose message began with the handler's own — and that wrapping is precisely what discloses it: the SDK flattens any non-McpException to "An error occurred." and passes an McpException's message through verbatim, so the bare rethrow was sanitized and the wrapped one was not. The client chose between them. A modern request that declares a log level gets its feedback as notifications, leaves the buffer empty and takes the rethrow; omitting that one _meta field fills the buffer and takes the wrapping path. So an untrusted client could turn sanitizing off by leaving a field out, and read back an IOException's path or the parameter and CLR type named by a binding failure. Only the app-authored feedback travels now. The same rule the handler already applies in ThrowSanitizedIfAClientAlreadyHasASchema.
…licating it ResolveAllRegisteredRoutes ran every module through ResolveActiveRoutes, which keeps one route per template and lets the last registration win. So it was not reporting registration, as its summary claimed, but one particular resolution of it — the resolution in which every module is present, which is the one a shadowing module always wins. An App whose template is claimed by a later registration was invisible to the probe, while every connection whose gate excludes that later module is served the App underneath it and told no extension exists. Replaced by AnyRegisteredRoute, which walks the registrations and answers a predicate. That drops the dedup and the array allocation, and returns a verdict rather than routes, so nothing downstream can project command names through a member whose whole point is to ignore the gates that hide them. The doc block this probe's last change replaced was left stacked above its replacement, still claiming both eras are probed; removed. The two shared-options guards kept rationales describing that two-era probe, which no longer exists — restated around what they actually pin.
…tand down Both scopes coalesced their concurrent first callers onto one roots/list, and each had its own copy of how: retract a settled-but-failed attempt at acquisition, start one when there is none, speak for its fault, wait on the caller's own token. Every part of that is load-bearing and easy to fix in one place only, which is what kept happening. JoinOrStartAsync now holds it, called under whichever lock the caller already owns, so neither scope gains a lock or changes behaviour. The eager prime also stands down for 30s after a failure. It runs at every execution boundary and asks on nobody's behalf, so pairing it with a retraction rule designed to make the next caller retry meant a client that declares the roots capability and never answers cost the full 10s budget on every single tools/call and resource read, for the life of the connection, with the failure swallowed each time. A handler that asks for roots itself is untouched: it still gets a fresh attempt and a real error. A roots/list_changed clears the stand-down, since whatever the client is reporting may be what failed. Also corrects what the version check claims: ordering a roots/list_changed against an unanswered fetch is awkward, not impossible, and the next commit does it.
…force the race 26b2045 removed the invalidation guard saying the ordering was not expressible here. It is. The client holds its roots/list answer and sends roots/list_changed; the server emits tools/list_changed from the same handler that moves the version, so receiving that echo proves the version moved while the fetch is still unanswered. Releasing the answer then exercises the branch that refuses to cache it. The technique was already in this file, guarding the neighbouring case. Verified red by widening the version comparison: the late answer is cached, the next caller reads a retired workspace and sends no roots/list at all. The sharing guard did not force the sharing. It started two calls together and counted round-trips, but a second call that arrives after the first has filled the cache also pays one — so it passed against code with no coalescing whatever. It now waits until the first roots/list is certainly outstanding and unanswered before issuing the second, and asserts while both are still held: the count moves when an unshared fetch is received, not when it is answered. Verified red by leaving nothing to join. Also drops the intra-branch narration from the retraction guard's description, which described the order two copies of the code were written in rather than what the test pins.
IMcpFeedback.SendMessageAsync and the reference both promised an undeliverable message is appended to the result so no host loses it. A resource read that succeeds is the exception: its result is a typed body whose MIME type has already been advertised, so buffered feedback is dropped rather than appended, and only a read that fails carries it in the surfaced error. Consumers were told to treat IsLoggingSupported as a hint on the strength of a guarantee that has a hole in it. Both halves now have a guard: the success path asserts the body is exactly what the handler returned, and the failure path asserts the feedback rides in the error while the handler's own message does not.
… the budget The stand-down exists to stop paying the 10s roots budget on every execution when a client declares the capability and never answers. A client that answers promptly with something unusable costs nothing to ask again, so backing off there bought nothing and held Current empty for half a minute after a fault that may already have cleared — trading one availability problem for a smaller one. It now triggers only when the failed attempt consumed at least half the budget, which is the shape of the failure it was written for. A fast failure retries at the very next execution, as before.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3de5db0492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Current answered from the native cache whenever the client declared the roots capability, so a connection whose roots/list was never asked or could not be reached reported an empty set — indistinguishable from a client that declared no workspace boundary at all. The eager prime absorbs that failure by design, so commands which never read roots still run, which left Current speaking for a resolution that never happened. An empty answer is the reading a handler is most likely to act on and the one it cannot check. Soft roots now stand in until something native is resolved. A client that really answers with zero roots is still told apart, because that answer is recorded as resolved. Handlers that need the failure itself call GetAsync, which surfaces it rather than absorbing it — now said on the member, in the transport guide and in the reference. Scoped to connection state. Request scope has no prime and therefore no swallowed failure: its empty answer means "this request has not resolved roots yet", which is the contract it already documents.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4bb1db1d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ed error The catch filter skipped the feedback-carrying path for any OperationCanceledException, which is the right rule only when the caller withdrew. A handler running its own budget — an internal timeout, a linked token — reports a failure like any other while the request token stays live, and its buffered diagnostic was dropped: the read produced nothing and nothing explained why. Cancellation is now told apart by who asked for it, matching McpClientRootsService.PrimeFromServicesAsync. Only the caller abandoning the request takes the bare path, where there is nobody left to read the answer. Also asserts that a command whose roots were invalidated mid-prime still runs against resolved roots rather than an empty set.
The feedback section stated flatly that a resource read has nowhere to put buffered messages and drops them. That is true only of a read that succeeds, whose body must match the advertised MIME type; a read that fails carries them in the surfaced error, on all three paths — the command-backed resource through McpToolAdapter, ReplMcpServerUiResource, and McpAppResource. The same page already said so where the protocol split is described, so the unqualified sentence contradicted it and told readers their failure diagnostics were gone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a31c83dfc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return _hardRoots; | ||
| } |
There was a problem hiding this comment.
On 2026-07-28, two requests on the same mcp serve transport may carry different destination-bound capabilities and identities, but RunAsync creates a connection-scoped roots service and this fast path returns the first request's _hardRoots without querying the current request.Server. Thus, after request A primes roots, request B with its own Roots capability can receive A's workspace URI, creating a cross-caller disclosure. Fresh evidence beyond the resolved reusable-options finding is that the dynamic path remains McpRootsScope.Connection; use request-scoped roots for sessionless requests while retaining connection caching for initialize-era sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I could not reproduce this, but one half of your reasoning is right and my own prior model was wrong — worth recording precisely.
Where you are right. I had believed sessionless requests never populate the connection cache, on the strength of DestinationBoundMcpServer.ClientCapabilities being null in HttpServerSessionMode.Stateless. That is an HTTP-path fact and does not carry to RunAsync. A probe on the mcp serve path, negotiating 2026-07-28:
era=2026-07-28 supported=True current=[file:///ga] get=[file:///ga] roundTrips=1
So the connection-scoped cache does fill on modern requests here, and a later request on that connection does read it without consulting its own request.Server. The mechanism is exactly as you describe.
Why it is not a disclosure. The premise that fails is "two requests on the same mcp serve transport may carry different identities". RunAsync builds one transport and serves one peer: docs/mcp-transports.md Scenario A is "stdio-over-anything" — WebSocket bridge, named pipe, SSH tunnel — each a single client channel. Every shape that carries more than one caller already goes through the reused-BuildMcpServerOptions() path, which is McpRootsScope.Request: the "Multi-session custom transports" section tells hosts to build options once and create a server per connection, and Scenario B (HTTP) does the same. That path resolves roots per request and is guarded by Given_McpSharedServerOptions.
So the containment is topological, not capability-based — one caller per connection-scoped service — which is a different argument from the one I would have given you yesterday.
Why I am not applying the suggested fix. Making sessionless requests request-scoped under mcp serve would cost one roots/list per request for the single peer that path serves, to defend a two-caller topology that no supported shape produces. That is a real regression against a hypothetical.
If you have a configuration where a single RunAsync fans two distinct callers over one transport, that is the missing piece and I will take it — it would make this a genuine P1 and the scope split the right fix.
There was a problem hiding this comment.
Follow-up: I audited the SDK surface after replying, and the argument is stronger than the one I gave you. My previous answer rested on documentation and host topology; the real reason is structural.
One ITransport is one peer by construction. It exposes a single ChannelReader<JsonRpcMessage> created with SingleReader = true, one read loop, and — decisively — messages carry no source discriminator: JsonRpcMessageContext has no peer member at all (RelatedTransport, ExecutionContext, User, Items, RoutingName, ProtocolVersion, ClientInfo, ClientCapabilities, LogLevel is the whole class). A host merging two clients onto one pipe would collide their JSON-RPC request IDs long before roots leaked. There is no multiplexing primitive anywhere below the HTTP layer; it exists only in ModelContextProtocol.AspNetCore, where each StreamableHttpSession owns its own transport and its own McpServer.
And there is no per-caller key to validate against, checked exhaustively: SessionId is null on stdio and StreamServerTransport in every revision — 2026-07-28 removed session IDs outright (SEP-2567), so it makes this less available, not more; MessageContext.User is populated only by the ASP.NET Core transports and is unconditionally null here; ClientInfo/ClientCapabilities are client-asserted capability data, not identity; Services is per-connection; request.Server is a fresh DestinationBoundMcpServer per message; _meta has no protocol-reserved identity key. The only latent seam is TransportBase.SessionId, whose setter is protected.
So the finding is closed as unreachable-by-construction rather than unreachable-by-convention — but I want the record to say plainly that your mechanism was correct: the connection cache does fill on modern requests under mcp serve, and a later request does read it without consulting its own request.Server. Only the two-callers-on-one-transport premise fails.
What your finding did expose is that nothing in this repository states the single-peer assumption, which is why it looked live to you and why I got the reason wrong on my first pass. I am filing a follow-up issue to say it at the TransportFactory sample, in the isolation table, and at the McpRootsScope.Connection construction site, plus a guard pinning the one-round-trip-per-connection cost model so a later switch to request scope has to be chosen rather than drifted into. I will link it here.
Leaving this thread open for you rather than resolving it, since I argued rather than changed anything.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a31c83dfc824e8fb4714585a6c29743a77e6621d. The previous blocking Apps capability mismatch is fixed: capability detection is now conservative over registrations, including presence-gated and shadowed App routes, and the exact legacy Roots-data counterexample now passes. The documentation corrections from the previous review are also present.
I revalidated the remaining high-risk paths rather than relying on CI alone:
- the five discriminating Apps/Roots regressions pass (5/5);
- a reviewer-only real-session probe covering prime timeout, stand-down, explicit
GetAsync, and an orderedroots/list_changedrace passes and never exposes the retired answer; - the full Release solution passes: 1,735 total, 1,733 succeeded, 0 failed, 2 expected skips;
Repl.Mcp.0.12.0-dev.134.ga31c83dfc8.nupkgbuilds and its packaged README contains the Apps and feedback contracts;- exact-head CI is green on Ubuntu, Windows, macOS, pack, analyzers, documentation, real-shell smoke, process-signal stress, and CodeQL;
git diff --checkis clean and the review worktree is clean.
The resource-failure change preserves app-authored buffered feedback while withholding the handler exception text, so it does not undo the SDK's sanitization boundary. I found no remaining correctness, isolation, protocol, or security issue that should block merge.
Two non-blocking cleanup nits remain: McpAppResource duplicates one cancellation comment block, and IMcpClientRoots.Current XML remarks could distinguish the connection-scoped soft-root fallback from the request-scoped reusable-options behavior more explicitly. Neither changes runtime behavior or makes the current public guide materially misleading.
Approved.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Correction to my approval on this same exact head. A late concurrency-panel result exposed a coverage gap, and I independently reproduced the product defect before changing the verdict.
Blocking: a direct Roots waiter receives an answer retired while in flight
GetAndMaybeCacheRootsAsync correctly refuses to cache a response after roots/list_changed advances _hardRootsVersion, but its mismatch branch still returns that response to the existing waiter. The deterministic real-session probe primes workspace-v0, starts and holds a direct GetAsync() for workspace-v1, invalidates while it is unanswered, then releases it. The handler returns file:///workspace-v1; it never reaches the current workspace-v2 answer.
Evidence and acceptance-test shape: #71 (comment)
The current committed race test does not cover this observable: its cold command path can perform a later prime/refetch before the handler reads Current, masking the stale value returned to the original waiter. Please make a version-mismatched fetch retry or fail rather than return mappedRoots, and add a regression asserting the direct GetAsync() result.
All previously reported Apps and documentation fixes remain validated, and the full suite/CI remain green; this newly reproduced stale-workspace result is nevertheless a correctness boundary and blocks merge. I am explicitly superseding review #5244065644.
The cancellation rationale above the App resource catch filter was pasted twice — a partial revert during verification left the first copy in place when the patch was replayed. IMcpClientRoots.Current described only the connection-scoped behaviour, where soft roots stand in until something native resolves. On a reused BuildMcpServerOptions() result the state belongs to the request instead: a roots-capable client reads empty until GetAsync has been called within that request, and soft roots answer only when the client supports no native roots at all. Both are now stated, since a reader on the wrong path would draw the opposite conclusion about what empty means.
| var errorText = result.Content?.OfType<TextContentBlock>().FirstOrDefault()?.Text | ||
| ?? "UI resource read failed."; | ||
| throw new McpException(errorText); | ||
| throw new McpException(McpToolAdapter.BuildErrorMessage(blocks, "UI resource read failed.")); |
There was a problem hiding this comment.
[required][security] Command-backed App failures re-expose handler exception text
The raw UiResource path deliberately withholds the handler exception message before wrapping buffered feedback, because converting it to McpException would bypass the SDK's generic-error sanitization. The command-backed .AsMcpAppResource(...) path does the opposite here: it turns the pipeline's exception-bearing output into the McpException message.
Deterministic wire probe: the handler emits render-notice and throws InvalidOperationException("secret-internal-detail"). The client receives:
Request failed (remote): {
"kind": "validation",
"message": "secret-internal-detail"
}
[info] render-notice
The feedback is app-authored and should survive; the handler's own exception text should not. Please align this path with McpAppResource.ThrowWithBufferedFeedback and add a regression asserting that the feedback is present while the internal detail is absent.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Current-head correction after the late panel.
I reviewed 30d639768b0b88a6d40e65de6614636103993d50. Its delta from a31c83d only removes the duplicated cancellation comment and clarifies request-vs-connection Roots documentation; both nits are resolved. Two required findings remain:
1. Direct Roots waiters still receive answers retired while in flight
The implementation at McpClientRootsService.cs:382-396 is unchanged. Its version-mismatch branch declines to cache an invalidated answer but still returns mappedRoots to the waiter. The deterministic real-session reproduction primes workspace-v0, parks a direct GetAsync() response as workspace-v1, invalidates while it is unanswered, then releases it. Actual handler result: file:///workspace-v1; expected: discard/retry to the current workspace-v2. Full evidence and acceptance-test shape: #71 (comment)
2. Command-backed App failures expose handler exception text
The changed command-backed resource path throws McpException(BuildErrorMessage(...)) from the pipeline's exception-bearing output, whereas the raw UiResource path explicitly withholds the handler exception text to preserve SDK sanitization. A deterministic wire probe emitted render-notice and threw InvalidOperationException("secret-internal-detail"); the client received both [info] render-notice and "message": "secret-internal-detail". Preserve the app-authored feedback, but sanitize the handler's own exception detail consistently with McpAppResource.ThrowWithBufferedFeedback, and add an absence assertion.
The late panel also found duplicate raw UI-resource URIs behave inconsistently between dynamic and static hosting. I reproduced it, but ReplMcpServerOptions.cs is not modified in this PR, so I am recording that as nonblocking/out-of-diff rather than adding a third requirement. Documentation and packaging had no additional findings.
…ched The version check stopped a retired answer from being cached, but GetAndMaybeCacheRootsAsync still returned it, so a direct GetAsync waiter received roots the client had already withdrawn — with nothing in the value to say so. The command boundary hid this: a later prime refetches before the handler reads Current, so only the value GetAsync itself returns shows the stale workspace. GetAsync now reads its answer back from the cache rather than from the task, because whether the answer was cached is exactly what says it is still current. An uncached one means the version moved while the fetch was unanswered, and the call asks again instead of returning it, bounded at three attempts — the client decides how often it invalidates, so the loop cannot be open-ended. Exhausting them throws rather than yielding a known-retracted answer; the eager prime swallows that, so only a caller that asked for roots sees it. Reported on #71 after a concurrency panel reproduced it against a held roots/list. The regression drives the same sequence — prime, invalidate, hold the handler's own fetch, invalidate again, release — and asserts on GetAsync's result rather than on Current, which is the observable the previous guard was missing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91eed5c625
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…urface A failed run surfaced whatever the pipeline rendered. For a handler that returned a failure result that is right — the app wrote that text for whoever called it. For one that threw, the text is the framework's rendering of an exception, which routinely names a filesystem path, a parameter and its full CLR type, or a connection string. At a console the reader is the operator; over MCP it is a remote client, and that is the whole difference. A wire probe received both the app's own `[warning] render-notice` and `"message": "secret-internal-detail"`. The two cannot be told apart from an exit code, so the sub-invocation now reports how the run ended alongside it, and only ReplExecutionOutcomeKind.HandlerException is replaced. Applied at both surfaces — tool results and command-backed resource reads — because a tool result carries its text as content rather than as an exception, so the SDK never sanitizes it and nothing else would. Same rule the App resource path already applies. Guards cover all three directions: the thrown message is withheld on a resource read and on a tool call, and a handler's own Results.Error still reaches the client word for word — without that last one this change would quietly turn every actionable error into a bare exit code. Also stops the roots prime re-arming its stand-down across an invalidation. The notification clears it deliberately, since whatever made the attempt fail may be what the client is reporting; an attempt that began earlier and failed slowly afterwards put it back, so no execution primed for half a minute despite the client having asked. The attempt now installs the stand-down only if the version it started under is still current. Reported on #71.
There was a problem hiding this comment.
💡 Codex Review
repl/src/Repl.Mcp/McpServerHandler.cs
Lines 204 to 206 in 5ca08b2
Stamp cache hints on prebuilt list responses
When a host uses the documented reusable BuildMcpServerOptions() path on 2026-07-28, these collection-only options bypass ListToolsAsync, ListResourcesAsync, ListResourceTemplatesAsync, and ListPromptsAsync, which are the only list paths that call McpCacheHints.MarkPrivateToThisClient. The SDK-generated list envelopes therefore retain the public/no-expiry defaults, so a client or shared gateway that opens no subscription can cache a list indefinitely even if the host later mutates one of these returned collections. Add static list handlers that apply the same private, zero-TTL hints before returning the collection contents.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| _ = SendNotificationSafeAsync(NotificationMethods.ToolListChangedNotification); | ||
| return new ListToolsResult | ||
| SignalToolListChanged(); |
There was a problem hiding this comment.
When one handler serves multiple initialize-era sessions with DiscoverAndCallShim, this call clears the handler-wide _toolListChanged collection, whose SDK subscribers broadcast the notification to every attached legacy session. A list request from session A can therefore make session B refresh and consume B's one-time compatibility intro before B's own caller requests it; it also sends spurious notifications to unrelated modern subscribers. The intro transition is connection-local, so its tools/list_changed notification must target only request.Server rather than the shared routing-change signal.
Useful? React with 👍 / 👎.
The 2026-07-28 invariance rule has two halves, and only one was closed. Discovery answered the four capability services with constants, which covers per-connection variance. The other half — the advertised set must not change as a side effect of another request on the connection — stayed open, because a presence predicate receives whatever it declares and everything else resolved live. IReplSessionState is the case that matters: a mutable singleton, an implicit handler parameter, and the same instance behind discovery and execution. A tools/call that wrote it and invalidated routing decided what the next tools/list advertised. That needs no second connection to observe, which is why the conformance page calls it the half that matters even on plain stdio — while claiming it was already closed. Session metadata is the per-connection half of the same gap: the live implementation is a façade over the ambient session. Both now resolve to constants on modern requests, in the same shape as the capability services and for the same structural reason: holding no inner service is what makes the invariant a property of the type rather than of each member. Consequence, stated rather than buried: a module gated on session state is now advertised to nobody on 2026-07-28, exactly as one gated on the roots data already was. The sign-in-then-reveal pattern no longer moves the modern catalog. The legacy guard is the other half of the pair — that revision has sessions and its dynamic-tools story depends on the graph moving, so the freeze must not reach it. Both verified red first: the modern one only after wiring the handler to the app's own container, since the default test helper passes an empty provider and the command could not bind IReplSessionState at all.
Discovery decides what is advertised from constants; execution decided whether the command exists all over again, from the live services. On 2026-07-28 those two views disagree by design, so a module gated on roots.IsSupported was offered to every client and then not found when called — the caller got "Unknown command", which names nothing it can act on. Three places in this repository promise that such a call "fails with an actionable tool error" instead; none of them was true, and no test had ever invoked an advertised gated tool. Presence and binding now come from different providers on that revision: presence from the same frozen answers discovery used, binding from the live services. So the command runs, and the handler — which does see the real client — can report what is missing. The framework guarantees reachability; the message is the author's. The routing graph is keyed and resolved on the presence provider, which is what decides it. The parameter is optional and defaults to today's behaviour everywhere else, so no non-MCP host changes: ResolveActiveRoutingGraph serves every host, and the Given_ModulePresence suite is the guard that it still does. The initialize era is untouched — it resolves per session and is allowed to vary. Both directions guarded: an advertised gated tool reaches its handler, and one the catalog never offered stays unreachable, so this does not quietly open commands the graph hides.
The conformance page described the freeze entirely in terms of capability services, which is what the code used to do. It now names the whole set — the four capability services plus IReplSessionState and IReplSessionInfo, every session-scoped input a presence predicate can receive by injection — and states the residual honestly: a predicate injecting an application service of its own is outside it by construction, because Repl cannot know which of your singletons a command mutates. It also promised that an advertised-but-ungated command "must fail with an actionable tool error", which read as a guarantee and was in fact impossible: the route was not even found. Reworded to the division that now holds — Repl guarantees the command is reachable and that the handler sees the real client; writing the error is the author's job. The module-presence page documents the sign-in-then-reveal flow as the motivating example. It works everywhere except the one revision that forbids it, so the flow now says so where it is taught rather than only in a note at the top.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660147a8d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…talog's own view Two gaps in yesterday's fixes, both reported on #71. **A raised McpException said something, and the wrapper threw it away.** The withholding rule is about text the framework rendered from an exception nobody meant to surface. Raising an McpException is the opposite — a deliberate act whose message was written for this client, which is why the SDK passes it through. The wrapper replaced it whenever feedback had been buffered, so the same failure explained itself without feedback and stopped explaining itself with it, which no caller could account for. Only a non-McpException is replaced now. **Presence followed the request's era; it must follow the catalog's.** A reusable BuildMcpServerOptions() result is frozen once, with the modern view, and then serves clients of either era. Keying execution on the request meant an initialize-era client was offered that catalog and could not call what it offered — the same advertised-but-unreachable defect the dynamic path had, left open on the static one by the shape of the fix. The adapter now carries how the catalog it serves was built. Both verified red first: the raised message survives alongside its feedback, and a legacy client can call the gated tool the static catalog advertised to it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d158550ceb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (invocation.Kind is ReplExecutionOutcomeKind.HandlerException) | ||
| { | ||
| return withheld; |
There was a problem hiding this comment.
When a tool using the default PrefillThenFail mode reaches an unresolved prompt—for example, the caller omits an answer.* value—McpInteractionChannel deliberately throws McpInteractionException with instructions identifying the missing answer. The pipeline classifies that as HandlerException, so this branch replaces the actionable message with only Command failed with exit code 1. for both tool calls and command-backed resource reads. Preserve this known client-facing exception while continuing to sanitize arbitrary handler exceptions.
Useful? React with 👍 / 👎.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Re-reviewed exact head d158550ceb6604b4f61a01b93ebc792a8d67a2fc. The strict Release build is clean, the focused MCP suite passed (295 passed / 1 external-toolchain skip), the full solution passed (1,743 passed / 2 documented skips), CI is green, and the worktree is clean.
I found one additional required security defect, distinct from Codex’s open McpInteractionException finding: exceptions raised while DI-backed arguments are being bound remain BindingError, so the MCP adapter returns the framework-rendered exception message verbatim. A deterministic wire probe with a [FromServices] dependency whose factory throws returned IsError=True and exposed the marker in the JSON message. See the inline comment for the exact path and acceptance test.
The current PR correctly added sanitization for uncaught handler exceptions, but the same policy is incomplete for dependency activation/binding failures. Please carry enough failure provenance/surface policy through the sub-invocation result to sanitize unexpected binding exceptions without suppressing intentional client-facing validation.
| private static string DescribeFailure(in McpPipelineInvocation invocation) | ||
| { | ||
| var withheld = $"Command failed with exit code {invocation.ExitCode}."; | ||
| if (invocation.Kind is ReplExecutionOutcomeKind.HandlerException) |
There was a problem hiding this comment.
[required][security] Binding-time DI failures still expose internal exception text
DescribeFailure masks only HandlerException. HandlerArgumentBinder.Bind runs before bound = true, so an exception from a [FromServices] factory is classified as BindingError; the framework renders ex.Message, and this branch then returns that rendered payload verbatim.
I reproduced this over the real in-process MCP transport on the exact head with a remotely callable probe tool whose dependency factory throws IOException("review-marker-detail"). The client received IsError=True and:
{
"kind": "error",
"code": "execution_error",
"message": "review-marker-detail",
"details": null
}This can disclose filesystem paths, connection strings, or provider internals whenever dependency activation fails. It is also the binding-failure case named in this method’s own security rationale, but ReplExecutionOutcomeKind alone cannot distinguish an unexpected activation exception from an intentional client-facing binding diagnostic. Please carry enough exception provenance/surface policy through SubInvocationOutcome to sanitize the former while retaining the latter, and add regressions with a throwing [FromServices] factory for both tools/call and command-backed resource reads.
2 participants
Summary
Updates Repl.Mcp to the stable official
ModelContextProtocolSDK 2.2.0 and adapts the MCP server to its per-request, multi-session model.SDK migration
ModelContextProtocolfrom 1.4.1 to 2.2.0.initializecompatibility for older hosts..LongRunning()remains a Repl annotation in this PR. Modern MCP Tasks integration is handled separately in Implement MCP Tasks runtime for .LongRunning() commands (ModelContextProtocol.Extensions.Tasks) #72.Session isolation and concurrency
One handler can serve multiple MCP sessions. This PR makes MCP-local state session-bound:
This prevents capability cross-wiring, workspace roots leaking between clients, stale session-specific tool graphs, and notifications stopping when one of several sessions closes.
Broader application DI scoping remains out of scope and is tracked in #70.
Breaking changes
Beyond the SDK 2.x requirement and the
IMcpFeedback.SendMessageAsyncsignature:Requires
ModelContextProtocol2.x. 1.4.x is not supported.On revision
2026-07-28, the advertised tool set no longer moves. That revision forbids it fromvarying per connection or changing as a side effect of another request, so discovery answers every
session-scoped input with a constant: capability checks read as supported, soft roots as absent, the
root list as empty, and the session state as empty.
The practical consequence: a module gated on session state is advertised to no MCP client on that
revision. The sign-in-then-reveal pattern — write session state, call
InvalidateRouting(), watchcommands appear — still works in the console, over the earlier MCP revisions, and everywhere else,
but it no longer changes what a modern MCP client is offered. Gate on something that does not change,
or map the command unconditionally and refuse inside it.
A capability-gated command that is advertised is now reachable. Execution decides presence from
the same answers discovery used, so a command offered to a client can be called by it, and the
handler runs with the real client rather than the catalog's view of it. Returning a clear error when
the capability is missing is the command author's job — previously such a call could not reach the
handler at all.
See Conformance for the full per-revision table.
Validation
Refs #51