fix(mcp): let a prompt reach the capability services through its IServiceProvider - #110
Open
carldebilly wants to merge 2 commits into
Open
carldebilly wants to merge 2 commits into
carldebilly wants to merge 2 commits into
Conversation
…viceProvider McpServiceProviderOverlay layers IMcpSampling, IMcpClientRoots, IMcpElicitation and IMcpFeedback in front of the provider it wraps, but answered a request for IServiceProvider with that wrapped provider. A handler that declares IServiceProvider and resolves a capability from it therefore got a container that knew none of them: an explicitly registered prompt asking for IMcpSampling that way got null, on both mcp serve and the reusable BuildMcpServerOptions() path. The overlay now answers IServiceProvider with itself, as it already does for IServiceProviderIsService. The defect turned out narrower than #96's table. Declaring a capability service directly as a parameter already works: McpExplicitPrompt replaces request.Services with the session's overlay before the SDK binds the handler, which is also where the request is bound, the issue's third point. That case is pinned now too, since #96 reported it failing and it had no test. Making the overlay an IServiceScopeFactory, as the issue floated, would change nothing observable: McpExplicitPrompt discards the SDK's per-request scope either way. TDD: both IServiceProvider tests red first with "sp-null"; the direct-parameter test green on main from the start, measured before any change. Refs #96
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. |
![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: b70bec8851
ℹ️ 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".
The overlay now answers IServiceProvider with itself, but IsService still asked the wrapped provider about it. The IServiceProviderIsService contract is that IsService reports what GetService can supply, so IsService now counts IServiceProvider and IServiceProviderIsService as well. Raised in review on #110, which predicted that a prompt declaring IServiceProvider would be classified as a client-supplied argument on ICoreReplApp.BuildMcpServerOptions() without an app provider, where the wrapped provider is empty. That did not reproduce: the new test for that path passed before this change, because the SDK binds an IServiceProvider parameter itself without asking IsService. The test stays as a guard for the path, and says so; it is not a reproduction. Refs #96
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #96.
Before any change, I measured each row of #96's table against current
main:(IMcpSampling sampling) => …(declared directly)(IServiceProvider sp) => sp.GetService(typeof(IMcpSampling))sp-nullThe real defect is narrower than the issue said.
McpServiceProviderOverlayputs the four capability services in front of the provider it wraps. When asked forIServiceProvider, though, it returned that wrapped provider, and that container knows none of the four. It now returns itself, as it already does forIServiceProviderIsService. This applies tomcp serveand to the reusableBuildMcpServerOptions()path; both are tested.Declaring a capability directly already worked because
McpExplicitPromptswaps in the session's overlay forrequest.Servicesbefore the SDK binds the handler. It also binds the flowing request there, which covers the issue's third point. I added a test for that case, since the issue reported it as failing and nothing covered it.I did not make the overlay an
IServiceScopeFactory, which the issue floated.McpExplicitPromptthrows away the SDK's per-request scope either way, so that change would have no observable effect.Test plan
IServiceProvidertests (mcp serveand reusable options) were red first, withsp-null.mainbefore any change, measured rather than assumed.IsServicenow also reportsIServiceProviderandIServiceProviderIsService, in step withGetService.When_APromptDeclaresIServiceProviderWithoutAnAppProvider_Then_ItIsBoundAsADependencycoversICoreReplApp.BuildMcpServerOptions()without an app provider. It passed before that change, because the SDK binds anIServiceProviderparameter itself, so it guards the path rather than reproducing a failure.