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".
f561022 to
3aeb83f
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3aeb83fc6a
ℹ️ 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".
carldebilly
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7135ede252
ℹ️ 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".
…boundaries The shared last-known-good catalog introduced two commits ago shared the wrong thing. A snapshot is not an immutable catalog: its tools, resources, prompts and their adapter capture the session's own services, including its connection-scoped roots service. Letting one connection fall back to another's snapshot therefore executes with that connection's roots — one client's workspace reaching another, which is the defect this whole migration exists to remove. Reported on #71 as a P1 against the commit that introduced it. There is no cheap repair: the fallback exists precisely because this connection's own projection failed, so there is nothing to rebuild its primitives from. Sharing only the catalog decision would still leave the executable half unusable. So the per-session fallback is restored, and the invariance gap it leaves — two modern connections can serve different sets while a transient failure lasts — stands open rather than being closed with something worse. The guard that pinned the shared behaviour is withdrawn with it; keeping a green test for a design that is being taken back would misrepresent what is covered. Two smaller faults from the same commit go with it. The eligibility check ran twice, once in the exception filter and once in the body, with a null-forgiving operator between them: a retraction landing in that window turned the original projection failure into a NullReferenceException. It now resolves once, re-checks before serving, and rethrows the original failure when the candidate has gone.
…he cause Marking application callbacks put the cause behind one wrapper, and the local renderer unwrapped one layer. But a property setter, an options-group constructor and a keyed-service factory all reach the binder through reflection, which adds a layer of its own — so those three produced "Exception has been thrown by the target of an invocation", which is true and useless. Only the direct service factory, the case I had measured, came out right. Unwrapping now continues until the application's own failure is what remains. The conformance page also listed explicitly registered prompts as unable to inject the MCP capability services. They can, as of the previous commit, and the tests that prove it ship with it — leaving the row there would tell a reader to avoid a feature that works, or to follow an issue that is closed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac84f51c8c
ℹ️ 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-review of exact head ac84f51c8c820c6f0527121a845bc38fdb700f2f.
The latest two commits correctly remove the cross-session executable snapshot (my A/B roots probe no longer leaks A into B), unwrap reflection wrappers in local diagnostics, and remove the stale prompt-injection limitation from the conformance page. The direct cancellation, explicit-prompt feedback/roots, and local diagnostic fixes from the previous round remain sound.
One blocker remains: after withdrawing the shared fallback, modern projection failures again use each connection own stale snapshot. A discriminating two-session probe on this head returns {always} to one 2026-07-28 connection and {always, added} to another after the same invalidation and projection failure. That violates the revision MUST NOT vary per-connection rule and the invariant documented by this PR. Exact evidence and the proposed safe split are in the existing thread:
#71 (comment)
Requested convergence: keep the historical per-session availability fallback for initialize-era clients, but fail closed for sessionless/2026-07-28 projection failures, with a restored two-session regression guard proving that the modern path never serves divergent stale sets. Do not reintroduce a shared executable snapshot.
I am not blocking this PR on the roots internal-budget timeout or the single-slot dual-era cache: both are acknowledged availability losses that fail closed and can be tracked. I am also not blocking on the static-catalog/application-service predicate drift because comparison with main confirms that behavior predates this migration.
Verification on this head: Release build with warnings as errors: 0 warnings, 0 errors. Full solution: 1,755 total, 1,753 passed, 2 documented skips. All current GitHub checks are green.
2026-07-28 requires the advertised set not to vary per connection, and the availability fallback varied it: a connection that had not read the catalog since the last routing change kept serving its older set while another already served the newer one, and a projection failure held that difference in place for as long as it lasted. Restrict the fallback to the initialize era, where the catalog is session state and a set that differs per connection is the point. A modern request now fails instead, and retries on the next one without needing another invalidation. Sharing one last-known-good catalog across modern connections was the other way to converge, and it is the worse one: the snapshot carries the executable primitives, which captured the services of whichever connection built it. The two stale-route guards become initialize-era guarantees, and a discriminating two-session guard pins that no modern connection is answered from its own cache.
Discovery evaluates presence against an empty interaction channel — nothing prefilled, and nobody to elicit or sample from — so a module gated on a confirmation that defaults to true is advertised to every modern connection. Execution overlaid the frozen capability answers but left the live channel in place, so the same predicate read the call's own answer.gate and re-decided the module absent: the advertised tool came back as an unknown command, and a tool argument decided whether the tool it was passed to existed. The channel joins the frozen set, and discovery and execution now build it from one factory so the two views cannot drift apart again.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ae3d78c43
ℹ️ 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".
| /// passed to exists. | ||
| /// </para> | ||
| /// </remarks> | ||
| public static Dictionary<Type, object> CreateSessionScopedOverrides(InteractivityMode interactivityMode) => new() |
There was a problem hiding this comment.
When the server is launched with a custom global option such as --env prod mcp serve and a module predicate reads IGlobalOptionsAccessor, discovery sees prod and advertises that module. A subsequent MCP tool sub-invocation contains no custom global options, so ExecuteParsedCoreAsync replaces the shared snapshot with an empty set before routing; because this override list omits IGlobalOptionsAccessor, execution then reevaluates the predicate against that live empty snapshot and rejects the advertised tool as unknown. Include a discovery-stable global-options view here so execution uses the same presence inputs as catalog construction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a defect and fixed in 58dd46c — but the mechanism is not the one described, and that difference decides where the fix belongs.
Update does not replace the shared snapshot with an empty set. It merges the invocation's parsed values over _sessionBaseline, so a launch-time --env prod survives every sub-invocation. Measured on 3ae3d78, with a predicate reading GetValue<string>("env") == "prod": advertised, and the call returns roots-only. No divergence on that member.
What a sub-invocation did clobber is _explicitKeys, which is replaced rather than merged — and HasValue is the only member that reads it. Same probe, predicate reading globals.HasValue("env"):
advertised=True isError=True
{ "kind": "error", "code": "unknown_command", "message": "Unknown command 'gated'." }
So the finding is real and your repro shape is right; the inconsistency is internal to GlobalOptionsSnapshot — values merge, explicitness did not — and it reaches every host that runs sub-invocations, not only MCP.
That is also why the fix is not a frozen view in the MCP overlay. Global options are application-global, not per-connection: they are exactly the "set that changes over time for everyone" the revision still allows, so freezing them to constants would change what is advertised rather than make it consistent. Update now carries the session's explicit keys through a sub-invocation, so HasValue and GetValue agree, and discovery and execution read the same answer by construction. A top-level run keeps the previous behaviour, since it is about to become the baseline itself and carrying the old keys into SetSessionBaseline is the leak that method exists to prevent.
Two guards, both verified red on 3ae3d78:
When_SubInvocationAfterRun_Then_BaselineGlobalOptionsAreStillExplicit(Repl.IntegrationTests), beside the sibling that pins the value;When_AModernClientCallsAToolGatedOnALaunchGlobal_Then_TheCommandRuns(Repl.McpTests), for the advertise-then-reject shape.
Update merges parsed values over the session baseline, but it replaced the explicit-key set with the sub-invocation's own keys — so HasValue denied an option whose value GetValue still returned. A module presence predicate reading HasValue therefore decided differently depending on whether a top-level run or a sub-invocation went last. Under `--env prod mcp serve` that is an advertised tool its own call rejects as unknown: the catalog is built outside any sub-invocation, the call is one, and it reset the set before routing resolved. A top-level run keeps the previous behaviour. It is about to become the baseline itself, and carrying the previous one's keys into SetSessionBaseline is the leak that method exists to prevent.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58dd46c245
ℹ️ 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 new GetPromptResult { Messages = messages, Description = result.Description }; |
There was a problem hiding this comment.
When an explicit options.Prompt(...) handler returns a GetPromptResult carrying result-level metadata and also emits feedback that must be buffered, this reconstruction copies only Messages and Description, silently dropping the original result metadata and any other protocol fields. Preserve those fields while replacing or extending the message collection so adding feedback does not change the handler's response contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Independently reproduced on exact head 58dd46c24523350c95fa8eba698f3b58dc689988.
The explicit handler returned a GetPromptResult containing all three public result fields:
Description = "kept-description"Meta = {"sentinel":"kept"}- one payload message
It also emitted prompt-notice, forcing McpExplicitPrompt through the reconstruction at line 88. The description and appended feedback survive, but the client observes result.Meta == null; the discriminating test fails at the sentinel assertion. Without feedback, the wrapper returns the original result and the metadata survives.
This is therefore an observable response-contract regression rather than a theoretical future-field concern. Please preserve Meta while extending Messages and add the same combined metadata-plus-feedback guard; removing the Meta assignment should make that guard red.
There was a problem hiding this comment.
Fixed in 920aa1c.
The rebuild named Messages and Description; it now carries Meta and ResultType as well, through a WithMessages helper. Rebuilt rather than mutated on purpose: a handler is free to return an instance it reuses across calls, so extending that one would make the notice permanent, and cumulative.
Two guards, both verified red against the previous head:
When_AnExplicitPromptReportsBesideMetadata_Then_TheWholeResultSurvives— a handler that setsDescription,_metaand one message, then emitsprompt-notice; it asserts the payload, the notice, the description and the sentinel.When_TheSdkPromptResultCarriesAField_Then_TheWrapperCopiesIt— readsGetPromptResult's own writable surface and pins it to the four fields the wrapper copies. The root cause is a hand-written field list going stale, so this turns the next SDK field into a red build rather than another silent drop.
One refinement on the observable, since it matters for the next probe. On the handler path the SDK stamps its identity into every modern result, so _meta is not null before the fix — it is the handler's entries that are gone:
_meta = {"io.modelcontextprotocol/serverInfo":{"name":"Repl.McpTests","version":"1.0.0"}}
A NotBeNull assertion therefore passes against the unfixed code; the sentinel is what discriminates. The defect is exactly as you described it.
docs/mcp-reference.md now states the contract: appending feedback is additive — a prompt returning its own GetPromptResult keeps the description and _meta it set.
Release build with warnings as errors: 0 warnings. Suites: 793 / 625 / 309 / 6 / 17, 0 failed.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Re-review of exact head 58dd46c24523350c95fa8eba698f3b58dc689988.
The previous blocker is closed correctly: modern/sessionless projection failures now fail closed, the two-connection regression guard is restored, recovery converges both clients on the current set, and the initialize-era stale fallback remains explicitly covered. The interaction-channel and session-global explicitness follow-up fixes also preserve discovery/execution agreement in their measured cases.
One new blocking response-contract regression remains in McpExplicitPrompt: when feedback is buffered, it reconstructs GetPromptResult with Messages and Description only. An independent exact-head probe returned Meta = {"sentinel":"kept"} plus feedback; the feedback and description survived, but the client observed result.Meta == null. Evidence:
#71 (comment)
Please preserve the original result metadata while extending its messages and add a combined metadata-plus-feedback regression guard. With SDK 2.2.0, GetPromptResult exposes Description, Messages, and inherited Result.Meta; all three must survive the wrapper.
Verification on this head: Release build with warnings as errors: 0 warnings, 0 errors. Full solution: 1,759 total, 1,757 passed, 2 documented skips. All current GitHub checks are green.
An explicitly registered prompt handler returns its own GetPromptResult. When a modern request declared no log level, the notices it emitted have nowhere to go but the result, so the wrapper rebuilds it around an extended message list — and the rebuild named Messages and Description only. Every other field the handler set was dropped on the way out, _meta included, for any prompt that reported anything. Without feedback the original result is returned untouched, so the loss appeared only on the path that carries the notice. Rebuilt rather than mutated: a handler is free to hand back an instance it reuses across calls, and appending to that one would make the notice permanent and cumulative. Copying every field is what the rebuild costs, and a second guard reads GetPromptResult's own surface so the next SDK release cannot add a field the wrapper silently drops. The discriminating assertion is the sentinel, not the presence of _meta: the SDK stamps its own serverInfo entry on results, so _meta is never null here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920aa1cd96
ℹ️ 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".
| public bool IsService(Type serviceType) => | ||
| overrides.ContainsKey(serviceType) | ||
| || (inner.GetService(typeof(IServiceProviderIsService)) as IServiceProviderIsService)?.IsService(serviceType) == true; |
There was a problem hiding this comment.
When BuildMcpServerOptions(..., services:) receives a custom IServiceProvider that resolves application dependencies but does not also implement IServiceProviderIsService, this returns false for every inner service even though GetService above can resolve it. The SDK uses this probe to classify explicit-prompt parameters, so those dependencies become client-supplied prompt arguments and invocation fails instead of injecting them. Preserve the inner provider's resolvability when it lacks the optional discovery interface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Measured rather than reasoned, because the premise is what decides this one. A prompt taking IThing, classified by McpServerPrompt.Create under four providers:
| Provider | Overlay | Classified as |
|---|---|---|
custom, no IServiceProviderIsService |
none | argument:thing |
custom, no IServiceProviderIsService |
this overlay | argument:thing |
ServiceCollection.BuildServiceProvider() |
none | injected |
ServiceCollection.BuildServiceProvider() |
this overlay | injected |
The overlay changes nothing in the scenario you describe. The SDK's own classification is options?.Services?.GetService<IServiceProviderIsService>() is { } ispis && ispis.IsService(type) — a provider that does not offer the interface yields the same "client-supplied argument" outcome with or without us, so this is the SDK's contract for provider capability discovery rather than something the overlay introduced. Against that baseline the overlay is a strict superset: it answers true for the MCP services it adds, which is why it exists.
Declining the suggested fallback on purpose. Answering IsService by calling GetService would construct every candidate service to decide a question about it — discarding scoped and disposable instances — and would misclassify a genuine client-supplied argument as injectable for any permissive container. That is a behaviour change for consumers who currently get argument semantics, traded for a case the SDK itself does not serve.
Happy to file a follow-up for first-class support of containers without IServiceProviderIsService if that is wanted; it is a feature, and it belongs with the other DI work rather than in this PR.
There was a problem hiding this comment.
One correction to my own wording, since it overstated the case.
"Strict superset" is true of the outcome — what a prompt parameter is classified as, with the overlay versus without it — which is what the measurement showed. It is not true of the overlay's own two methods: when inner exposes no IServiceProviderIsService, GetService still resolves from inner while IsService answers only from overrides. Those two do disagree, exactly as you described.
It does not change the conclusion, since the SDK reaches the same classification either way, and the probing alternative would instantiate candidates to answer a question about them. Recorded so the next reader does not take the stronger claim at face value.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved on exact head 920aa1cd96748dd3cc5090df963013202f825425.
This review has converged. The previously demonstrated blockers are closed with discriminating guards: application cancellation provenance, local diagnostic cause retention, session-safe discovery/execution agreement, modern two-connection list invariance with fail-closed projection, and complete explicit-prompt result preservation when feedback is appended.
For the final prompt-result fix I independently removed only the Meta copy: When_AnExplicitPromptReportsBesideMetadata_Then_TheWholeResultSurvives turned red on the missing sentinel, then passed again on the restored head. The SDK-surface guard also pins every currently writable GetPromptResult field.
The remaining custom-provider observation is not a regression of the overlay: a provider without IServiceProviderIsService gets the same argument classification from the SDK without the overlay, while probing GetService would instantiate candidates and alter existing semantics. That can be considered separately if first-class support for such containers is wanted.
Final verification: Release build with warnings as errors: 0 warnings, 0 errors. Full solution: 1,761 total, 1,759 passed, 2 documented skips. All GitHub checks are green, git diff --check is clean, and the worktree is clean.
…gain ReplBindingCallbackException derives from InvalidOperationException, which this pipeline renders as a validation result. So marking what escapes application code during binding — the fix that let an MCP host withhold the cause — moved four shapes of failure into the bucket that tells a caller their input was wrong: an options-group constructor, a property setter, a keyed-service factory, and any DI factory throwing something other than an InvalidOperationException. All four rendered as execution_error before, and nothing declared the change. A dedicated arm ahead of the InvalidOperationException one restores them, and takes the fifth with it: a factory throwing an InvalidOperationException was already classified as validation, by accident of its type. Binding callbacks now answer alike whatever the application threw, which is the same rule this code already applies to cancellation — decide by who failed, not by the type. Validation keeps its meaning. The binder's own diagnostics about the caller's input never travel through the marker, so a missing required parameter is still a validation result. The two existing guards assert the rendered message and both throw an InvalidOperationException, the one type that masks this — they stayed green throughout. The new ones assert the status and the code.
An initialize-era projection awaits the connection's roots fetch, which runs on its own ten-second budget rather than on the caller's token — deliberately, since the result is shared and no single caller may bound it. So the budget expiring surfaces as an OperationCanceledException while the request's own token is still live, and the unfiltered cancellation arm sat above the availability fallback: it rethrew past a catalog this connection had been serving a moment earlier. Reachable whenever roots/list_changed bumps the routing version while the client has stopped answering roots/list. Every other cancellation catch in this package already asks who withdrew rather than reading the exception's type; this one did not. The fallback itself is untouched — still initialize-era, still this connection's own previous catalog, still re-reading the retraction watermark before serving. GetSnapshotAsync sat exactly at the sixty-line cap, so the three failure arms move into their own method rather than the reasoning that separates them being cut to fit. Pure extraction: same order, same filters, same gate around it.
Every MCP invocation opens a session so the command sees isolated I/O, under an identifier minted for that one call. ReplSessionIO unregisters on dispose only when the caller supplied no identifier, because supplying one is read as owning the lifetime — right for a transport host, which unregisters its own at shutdown, wrong for a throwaway. So every tool call, resource read and prompt get left an entry in a process-wide dictionary that nothing would ever remove, on the one path built to run as a long-lived server. Fixed where the wrong answer came from: ownership is now something a caller can state rather than something inferred from whether it passed an identifier. Inference stays the default, so no existing caller changes. ExecuteThroughPipelineAsync sat at the sixty-line cap, so the overlay's single-entry map loses five lines of braces to make room for the argument.
…ts reach The upgrade list counted seven changes and described seven, but the one a consumer is most likely to meet at runtime was not among them: an uncaught handler exception, or an application callback failing while it supplies a parameter, now reaches an MCP client as a generic sentence. The phrase appeared nowhere in docs/ or in the packed README. It is the eighth, with the note a host reading outcomes directly needs — the marker type now travels on the outcome, the application's own exception inside it. The packed README said five things change and then pointed at the reference for the full list, which cannot both be true. It now says what it is: the ones met first, with a pointer to all eight. Two API remarks described a behaviour narrower than the one they have. IsLoggingSupported documents what a request reads and never said a presence predicate reads true regardless — so a module gated on it hides from nobody. SetSoftRoots says "the current session", which on a reused BuildMcpServerOptions result is every connection built from it; the transports page has carried that limitation all along, the API it applies to did not. And GlobalOptionsSnapshot.Update names its callers to explain a choice, but there are three, not two. The interactive resolver takes the same default for a different reason, which is worth stating where the other two are.
Nothing here changes behaviour; each was raised by a review lens and each is small enough that landing them separately would cost more than it explains. The sessionless discovery overlay built an interaction channel and then let the frozen set overwrite that same key, so a channel and a dictionary were allocated and dropped on every discovery build, and which one won depended on merge order. The frozen set already carries the channel, so on that path it is the overlay. It is handed out as IReadOnlyDictionary now: no caller mutates it. DescribeFailure never returns whitespace, so the non-zero arm of the guard below its call could not run, and it kept a second copy of the withheld sentence in step with the first for no reason. IMcpFeedback was resolved twice per invocation, the second resolve assuming a registration the first had already made. ThrowWithBufferedFeedback returns when nothing is buffered, which is why every call site needs a bare throw after it — it is ThrowIfFeedbackBuffered now, the shape the sibling in McpServerHandler already uses. ISubInvocableReplApp declared a sub-invocation member the outcome overload superseded; no caller reached it through the interface. Its param tags were also ordered differently from the signature. And two pieces of prose: a stray semicolon on its own line, and a clause in McpExplicitPrompt narrating how _meta stopped reaching the client — a state that only ever existed between two commits on this branch, so nothing a reader can reach explains it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c87d61a4c
ℹ️ 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".
| private static bool IsApplicationFailure(Exception exception, CancellationToken cancellationToken) => | ||
| exception is not OperationCanceledException || !cancellationToken.IsCancellationRequested; |
There was a problem hiding this comment.
When the request token is canceled while a keyed-service factory, options-group constructor, or property setter throws OperationCanceledException, the reflection call surfaces it as a TargetInvocationException. This predicate examines only the outer exception, so it classifies the cancellation as an application failure and wraps it in ReplBindingCallbackException; the execution pipeline then returns an ordinary execution error instead of propagating cancellation. Unwrap reflection exceptions before this test so caller-canceled CLI and MCP requests still terminate as cancellations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Measured on a817963, and the answer is in two parts.
You are right about the predicate. Reflection does wrap: a probe calling PropertyInfo.SetValue on a setter that throws OperationCanceledException returns TargetInvocationException, so IsApplicationFailure never sees the cancellation underneath and marks it.
The consequence does not follow. Both directions, measured through the pipeline:
| scenario | outcome |
|---|---|
| caller withdraws mid-binding, setter observes it and throws | Kind = Cancelled, no error result rendered |
| setter gives up on a token of its own, caller's token live | Kind = BindingError, error / execution_error |
So a caller-cancelled request does terminate as a cancellation, and a self-cancelling callback is still an application failure. The rule you are defending holds — it is enforced at the pipeline boundary, where RunUnderCancellationPolicyAsync converts on the caller's own token, rather than at the marker.
What is fair in your finding is that the right answer is reached one level out from where the test is written, so the two could drift. Both directions are now guards rather than readings — When_TheCallerWithdrawsDuringAnOptionsGroupSetter_Then_TheRunIsCancelled and When_AnOptionsGroupSetterCancelsItself_Then_TheOutcomeIsAnExecutionError — so a later change to either the marker or the boundary turns red instead of quietly relabelling a withdrawal.
Unwrapping inside the predicate is not the fix it looks like: the filter returning false leaves reflection's wrapper to propagate, and the pipeline's cancellation arm does not match a TargetInvocationException either — it would land in the general arm and report execution_error, which is the outcome you are warning about. Making it work would mean rethrowing the unwrapped cancellation from the binder. That is exception plumbing worth doing deliberately rather than in a closing pass, so it is noted in #103 alongside the other robustness items.
…e binder A review raised that the marker's who-cancelled test reads the exception reflection handed it, and reflection wraps what a setter, a constructor or a keyed factory threw. That is true — measured, the setter surfaces a TargetInvocationException — but the consequence it predicted does not follow, and neither direction was covered. Measured on both sides. A caller that withdraws mid-binding ends the run Cancelled with no error result: the answer is reached at the pipeline boundary, on the caller's own token, rather than at the marker. A callback that gives up on a budget of its own, while the caller's token is live, is marked and rendered execution_error, which is the application failing like any other. Both are now guards rather than readings, so a later change to either the marker or the boundary cannot quietly turn a withdrawal into a failure report.
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.
An uncaught exception no longer reaches the client as text. A command that throws, or an
application callback that fails while supplying a parameter — a service factory, an options-group
constructor, a property setter — is surfaced to an MCP client as
Command failed with exit code N.The framework renders that message for an operator at a console, and it routinely carries a path,
a parameter and its CLR type, or a connection string; over MCP the reader is a remote client.
Feedback the application reported itself still travels, so return an error from the command when
the client needs the reason. Nothing changes locally — the console still names the cause. For a
host reading outcomes directly, such as an
ExitCodes.Resolver: a binding-callback failure nowcarries
ReplBindingCallbackExceptiononReplExecutionOutcome.Exception, with the application'sown exception in
InnerException, and is classifiedexecution_erroras it was before themarker existed.
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