Change type of Tool.InputSchema to be JsonElement#4
Conversation
| /// </summary> | ||
| [JsonPropertyName("inputSchema")] | ||
| public JsonSchema? InputSchema { get; set; } | ||
| public JsonElement InputSchema { get; set; } |
There was a problem hiding this comment.
Not all JsonElement are valid JSON schemas, and not all JSON schemas are valid root-level schemas per the MCP specification. We should try to insert baseline validation (e.g. using a wrapper type or validation on the setter) to make sure we conform.
There was a problem hiding this comment.
Roughly, this should be checking that the JsonElement is an object and also contains a "type" keyword that is set to "object". Everything else including the "properties" keyword is optional IIRC.
|
This seems to be causing hangs in the Ubuntu debug leg. |
There appear to be general reliability issues with our integration testing, although this is the first time I see a hang happening. |
044a291 to
65a9f2e
Compare
65a9f2e to
36333a0
Compare
|
The Ubuntu hand is happening on #9 as well, so whatever the issue is, it's already in main it seems. |
|
Adding validation to the property setter revealed a few bugs in the process :-) Merging since CI is borked right now. |
| ["description"] = tool.Description ?? string.Empty, | ||
| ["properties"] = tool.InputSchema?.Properties ?? [], | ||
| ["required"] = tool.InputSchema?.Required ?? [] | ||
| ["properties"] = new Dictionary<string, object?>(), |
There was a problem hiding this comment.
I just noticed this. Your changes are leaving the properties and required keywords empty. Was this an intentional change?
There was a problem hiding this comment.
I've updated the code so that it always just forwards the JsonElement in the Tool. It is now always guaranteed to be populated.
Three related fixes to the task-store wrapper in McpServerImpl, all in the same catch chain around L920-970: modelcontextprotocol#2 - Failed payload shape + JsonDocument lifetime (SEP-2663 186) - Register JsonRpcErrorDetail in McpJsonUtilities source-gen context. - Replace JsonDocument.Parse(...).RootElement (leaks pooled buffer) with typed JsonRpcErrorDetail + SerializeToElement. - Emit {code, message} per spec; McpProtocolException preserves its ErrorCode + Message (documented safe to propagate); all other exceptions redact to InternalError + generic message. - Test: McpProtocolException_FromTool_StoresAsFailedWithJsonRpcErrorShape. modelcontextprotocol#3 - Orphan-on-IsTask=true - When TaskStore is configured AND CallToolWithTaskHandler returns IsTask=true, the store's pre-created task was left in Working forever. Now fails the store's task with a clear InternalError identifying the misconfiguration (use only one mechanism). - New test class: TaskStoreOrphanedTaskTests. modelcontextprotocol#4 - Dedicated InputRequiredException catch - When [McpServerTool] throws InputRequiredException under the task wrapper, it fell through to the generic catch and surfaced as a misleading Failed task. The taskId was already returned to the client synchronously, so InputRequiredResult cannot be surfaced retroactively. Now fails the task with an actionable InvalidRequest message pointing the user to CallToolWithTaskHandler. - Test: InputRequiredException_FromTool_FailsTaskWithActionableMessage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
No description provided.