Skip to content

[azsdk-cli] Change boolean? to boolean for convert swagger/tsp tool#12050

Merged
benbp merged 1 commit into
Azure:mainfrom
benbp:benbp/azsdk-nullable
Sep 11, 2025
Merged

[azsdk-cli] Change boolean? to boolean for convert swagger/tsp tool#12050
benbp merged 1 commit into
Azure:mainfrom
benbp:benbp/azsdk-nullable

Conversation

@benbp
Copy link
Copy Markdown
Member

@benbp benbp commented Sep 11, 2025

Using the boolean? nullable type results in a tools/list object that has an array type for the type property of the parameter (see below) instead of string. This breaks the IntelliJ copilot chat plugin when loading the MCP server tools (found by adding com.github.copilot:trace to the intellij debug log configuration):

2025-09-11 15:37:46,006 [ 439679] WARN - #com.github.copilot.mcp.agent.lsp.LspGetToolsNotificationListener - failed to handle MCP tools notification
com.google.gson.JsonSyntaxException: java.lang.IllegalStateException: Expected STRING but was BEGIN_ARRAY at path $.servers[0].tools[31].inputSchema.properties..type

The MCP schema appears to support any object type for the whole parameter property (https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.json#L2373-L2377), so my guess is this is an implementation issue with the plugin. Other mcp clients like vscode load the tools list just fine.

Below is an example of tool parameter properties returned from the tools/list call that reproduces the issue. Using "type": "boolean" works but "type": ["boolean","null"] will crash the plugin.

"fullyCompatible": {
  "description": "Indicates whether the generated TypeSpec project should be fully compatible with the swagger. It is recommended to set this to `false` so that the generated project leverages TypeSpec built-in libraries with standard patterns and templates.",
  "type": [
    "boolean",
    "null"
  ]
}

As a follow-up I am going to add an analyzer to prevent nullable parameter types for MCP tools, unless we can confirm this is an issue with the copilot plugin or the MCP C# sdk and get it fixed upstream.

@benbp benbp added the Central-EngSys This issue is owned by the Engineering System team. label Sep 11, 2025
Copilot AI review requested due to automatic review settings September 11, 2025 20:20
@benbp benbp self-assigned this Sep 11, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR simplifies the type signature of the ConvertSwaggerAsync method by changing two parameters from nullable booleans (bool?) to non-nullable booleans (bool). This removes the need for null-coalescing operations when passing these parameters to downstream methods.

  • Changes isAzureResourceManagement parameter from bool? to bool
  • Changes fullyCompatible parameter from bool? to bool
  • Removes null-coalescing operators (?? false) when passing parameters to RunTspClientAsync

@benbp benbp force-pushed the benbp/azsdk-nullable branch from 4218c88 to 7d1b018 Compare September 11, 2025 20:24
@benbp benbp moved this from 🤔 Triage to 🔬 Dev in PR in Azure SDK EngSys 🚀🌒🧑‍🚀 Sep 11, 2025
@benbp benbp added the azsdk-cli Issues related to Azure/azure-sdk-tools::tools/azsdk-cli label Sep 11, 2025
Copy link
Copy Markdown
Member

@samvaity samvaity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, with a minor note that the bool? specification isn't the main concern. For now, this serves as a good workaround until the copilot-intellij plugin addresses the actual issue.

@benbp
Copy link
Copy Markdown
Member Author

benbp commented Sep 11, 2025

Approved, with a minor note that the bool? specification isn't the main concern. For now, this serves as a good workaround until the copilot-intellij plugin addresses the actual issue.

@samvaity agreed, the bug report against the plugin is linked ^^

Though regardless I don't think we should really be using nullable parameter types anyways, so I'm happy to remove it permanently.

@benbp benbp merged commit f95508c into Azure:main Sep 11, 2025
14 checks passed
@benbp benbp deleted the benbp/azsdk-nullable branch September 11, 2025 21:32
@kurtzeborn kurtzeborn moved this from 🔬 Dev in PR to 🎊 Closed in Azure SDK EngSys 🚀🌒🧑‍🚀 Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

azsdk-cli Issues related to Azure/azure-sdk-tools::tools/azsdk-cli Central-EngSys This issue is owned by the Engineering System team.

Projects

Development

Successfully merging this pull request may close these issues.

4 participants