-
Notifications
You must be signed in to change notification settings - Fork 601
Fix form mode elicitation schema check to apply for all form requests #1151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ut schema Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
| Elicitation = new ElicitationCapability | ||
| { | ||
| Url = new UrlElicitationCapability(), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Elicitation = new ElicitationCapability | |
| { | |
| Url = new UrlElicitationCapability(), | |
| } | |
| Elicitation = new(), |
@copilot I think it's only by chance that we check request.RequestedSchema is null before elicitationCapability.Form is null in McpServer.ThrowIfElicitationUnsupported, so we should make sure we configure the client with valid capabilities in case we ever reorder those checks which should be inconsequential.
I think this should work without explicitly declaring the Form capability, since we default to setting it if you don't set Url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested change Elicitation = new() causes the test to fail because McpClientImpl defaults to setting Form = new() when both Form and Url are null (lines 108-113 in McpClientImpl.cs). With Form capability enabled, the check in ThrowIfElicitationUnsupported at line 491 (elicitationCapability.Form is null) is false, so the block containing the ArgumentException check is never entered.
The current production code only throws the "Form mode elicitation requests require a requested schema" ArgumentException when the client doesn't support form mode (Form is null). This seems to be the intended behavior based on the condition structure. The test with URL-only capability correctly tests this specific case.
Should we instead change the production code to check for missing RequestedSchema for all form mode requests, regardless of Form capability? That would require moving the schema check outside the Form is null block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead change the production code to check for missing
RequestedSchemafor all form mode requests, regardless of Form capability? That would require moving the schema check outside theForm is nullblock.
Yes. But rather than moving the schema check out of the if block, I think if (string.Equals(request.Mode, "form", StringComparison.Ordinal) && elicitationCapability.Form is null) should probably just be changed to if (string.Equals(request.Mode, "form", StringComparison.Ordinal)). We check elicitationCapability.Form is null again before throwing an InvalidOperationException, so it was redundant.
halter73
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please make this change.
Should we instead change the production code to check for missing
RequestedSchemafor all form mode requests, regardless of Form capability? That would require moving the schema check outside theForm is nullblock.Yes. But rather than moving the schema check out of the if block, I think
if (string.Equals(request.Mode, "form", StringComparison.Ordinal) && elicitationCapability.Form is null)should probably just be changed toif (string.Equals(request.Mode, "form", StringComparison.Ordinal)). We checkelicitationCapability.Form is nullagain before throwing anInvalidOperationException, so it was redundant.
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Done in 9a2de43. Changed the condition to |
if (string.Equals(request.Mode, "form", StringComparison.Ordinal) && elicitationCapability.Form is null)toif (string.Equals(request.Mode, "form", StringComparison.Ordinal))FormElicitation_Requires_RequestedSchemato useElicitation = new()instead of explicit URL-only capabilityOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.