Conversation
…ndaloneSSE to HTTP transport Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the MCP inspector client to reduce duplicated root-inference code and avoid maintaining an unnecessary SSE stream for HTTP-based inspection.
Changes:
- Extracted shared root URI inference into
extractRootsFromResources()and used it from both stdio and HTTP connect paths. - Disabled standalone SSE (
DisableStandaloneSSE: true) for the HTTP MCP inspector transport to reduce resource usage. - Reformatted portions of
docs/public/editor/autocomplete-data.jsonby inlining manyenumarrays.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cli/mcp_inspect_mcp.go | Deduplicates root extraction logic and disables standalone SSE for HTTP transport. |
| docs/public/editor/autocomplete-data.json | Formatting-only changes to enum array layout (inlined). |
Comments suppressed due to low confidence (1)
pkg/cli/mcp_inspect_mcp.go:318
- The new
extractRootsFromResourceshelper has non-trivial behavior (scheme parsing + de-duping). Since this repo already has unit tests for the MCP inspect CLI, it would be good to add a focused unit test for this helper to lock in ordering and de-duplication behavior and ensure it stays in sync with expectations.
// extractRootsFromResources infers root URIs from a list of resources by extracting
// the scheme portion (e.g. "file://") of each resource URI.
func extractRootsFromResources(resources []*mcp.Resource) []*mcp.Root {
var roots []*mcp.Root
for _, resource := range resources {
if strings.Contains(resource.URI, "://") {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func extractRootsFromResources(resources []*mcp.Resource) []*mcp.Root { | ||
| var roots []*mcp.Root | ||
| for _, resource := range resources { | ||
| if strings.Contains(resource.URI, "://") { | ||
| parts := strings.SplitN(resource.URI, "://", 2) | ||
| if len(parts) == 2 { | ||
| rootURI := parts[0] + "://" | ||
| // Check if we already have this root | ||
| found := false | ||
| for _, root := range info.Roots { | ||
| for _, root := range roots { | ||
| if root.URI == rootURI { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| info.Roots = append(info.Roots, &mcp.Root{ | ||
| roots = append(roots, &mcp.Root{ | ||
| URI: rootURI, | ||
| Name: parts[0], | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return info, nil | ||
| return roots | ||
| } |
There was a problem hiding this comment.
extractRootsFromResources returns a nil slice when no roots are inferred (because roots is never initialized). Previously info.Roots stayed as a non-nil empty slice, so this subtly changes behavior (e.g., JSON marshaling could emit null instead of []). Consider initializing roots to an empty slice (or returning []*mcp.Root{} when none found) to preserve prior semantics.
This issue also appears on line 313 of the same file.
| @@ -238,192 +236,116 @@ | |||
| "reaction": { | |||
| "type": "string|integer", | |||
| "desc": "AI reaction to add/remove on triggering item (one of: +1, -1, laugh, confused, heart, hooray, rocket, eyes, none).", | |||
| "enum": [ | |||
| "+1", | |||
| "-1", | |||
| "laugh", | |||
| "confused", | |||
| "heart", | |||
| "hooray", | |||
| "rocket", | |||
| "eyes", | |||
| "none" | |||
| ], | |||
| "enum": ["+1", "-1", "laugh", "confused", "heart", "hooray", "rocket", "eyes", "none"], | |||
| "leaf": true | |||
| }, | |||
| "status-comment": { | |||
| "type": "boolean", | |||
| "desc": "Whether to post status comments (started/completed) on the triggering item.", | |||
| "enum": [ | |||
| true, | |||
| false | |||
| ], | |||
| "enum": [true, false], | |||
| "leaf": true | |||
There was a problem hiding this comment.
This file has a large set of formatting-only changes (inlining many enum arrays) that aren't mentioned in the PR description and appear unrelated to the MCP inspector refactor. Consider reverting this change or explaining why it's needed here (or moving it to a dedicated formatting PR) to keep the diff focused.
connectStdioMCPServerandconnectHTTPMCPServercontained identical ~20-line root URI extraction logic, and the HTTP transport was unnecessarily maintaining a persistent SSE connection despite only needing request-response communication.Changes
extractRootsFromResources()— deduplicates the root URI inference loop (split on://, dedup by scheme) shared by both connect functionsDisableStandaloneSSE: trueonStreamableClientTransportinconnectHTTPMCPServer()— the inspector only queries capabilities; no server-initiated messages are needed, so the persistent GET/SSE stream is wastefulOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.