Skip to content

Tolerate spec-violating list methods on backend init #5232

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.

Sign up for GitHub

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

Open
tgrunnagle wants to merge 1 commit into
base: main
Choose a base branch
from
+428 −25
Open

Tolerate spec-violating list methods on backend init #5232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 48 additions & 25 deletions pkg/vmcp/session/internal/backend/mcp_session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package backend

import (
"context"
"errors"
"fmt"
"io"
"log/slog"
Expand Down Expand Up @@ -385,40 +386,62 @@ func initAndQueryCapabilities(

if serverCaps.Resources != nil {
resResult, listErr := c.ListResources(ctx, mcp.ListResourcesRequest{})
if listErr != nil {
switch {
case errors.Is(listErr, mcp.ErrMethodNotFound):
Copy link
Copy Markdown
Contributor

@lorr1 lorr1 May 8, 2026

Choose a reason for hiding this comment

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

[LOW] errors.Is will not match HTTP-level method-missing variants.

Confirmed mcp-go@v0.49.0's JSONRPCErrorDetails.AsError() wraps mcp.ErrMethodNotFound for the -32601 JSON-RPC envelope, so this discriminator is correct for the Atlassian Rovo case (which returns a proper JSON-RPC error response).

Caveat worth noting: some real backends behind reverse proxies or API gateways surface "this method isn't here" as HTTP 404 / 405 / 501 rather than a JSON-RPC -32601 envelope. Those return non-sentinel errors from the transport layer, so they will fall through to the fatal case listErr != nil arm under this PR. The narrow scope is defensible (the issue specifically calls out -32601), but the next operator hitting an HTTP-404 backend will wonder why their case wasn't recovered.

No change required for this PR. Worth either (a) a follow-up issue to enumerate which transport-level signals should also be treated as method-missing, or (b) a half-line in the recovery comment noting that only JSON-RPC -32601 envelopes are recovered (HTTP-level method absence is intentionally fatal).

// Spec violation: backend advertised the resources capability in its
// initialize response but does not implement resources/list. Recover
// with an empty resource set so the backend's tools remain usable
// instead of failing init outright. See issue #5231.
Comment on lines +391 to +394
Copy link
Copy Markdown
Contributor

@lorr1 lorr1 May 8, 2026

Choose a reason for hiding this comment

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

[LOW] Comment phrasing. Three concerns converge on this comment block (mirrored in the prompts case at line 418):

  1. "Recover with an empty resource set" reads as if a slice is allocated. Actually caps.Resources simply remains at its zero value (nil) — range over nil and len are both zero, so the behavior is correct, but the wording mildly misleads about state.
  2. The MCP spec doesn't contain a normative MUST for "if you advertise capability X you must implement X/list." The label "spec violation" is rhetorically reasonable but not directly citable from the spec.
  3. The "why" (keep tools usable) is the third sentence in; the first two restate what the surrounding errors.Is(...) and slog.Warn already convey.

Suggest tightening to one rationale-focused sentence, e.g.:

Suggested change
// Spec violation: backend advertised the resources capability in its
// initialize response but does not implement resources/list. Recover
// with an empty resource set so the backend's tools remain usable
// instead of failing init outright. See issue #5231.
// Tolerate -32601 here so a backend that advertises the resources
// capability but does not implement resources/list (e.g. Atlassian Rovo,
// see #5231) still contributes its tools instead of being dropped.

Apply symmetrically to the prompts block at line 418.

slog.Warn("backend advertised resources capability but does not implement resources/list",
"backendID", target.WorkloadID,
"method", "resources/list",
)
Comment on lines +395 to +398
Copy link
Copy Markdown
Contributor

@lorr1 lorr1 May 8, 2026

Choose a reason for hiding this comment

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

[LOW] WARN log lacks workload name and base URL.

When this WARN fires in production it's the operator's primary breadcrumb that a backend silently produced an empty resource/prompt set. backendID (= target.WorkloadID) requires an extra ID-to-name lookup to correlate to a specific MCP server. Adding name and baseURL makes the log directly greppable. Same applies to the prompts WARN at line 422.

Suggested change
slog.Warn("backend advertised resources capability but does not implement resources/list",
"backendID", target.WorkloadID,
"method", "resources/list",
)
slog.Warn("backend advertised resources capability but does not implement resources/list",
"backendID", target.WorkloadID,
"name", target.WorkloadName,
"baseURL", target.BaseURL,
"method", "resources/list",
)

case listErr != nil:
return nil, fmt.Errorf("list resources failed: %w", listErr)
}
for _, r := range resResult.Resources {
caps.Resources = append(caps.Resources, vmcp.Resource{
URI: r.URI,
Name: r.Name,
Description: r.Description,
MimeType: r.MIMEType,
BackendID: target.WorkloadID,
})
default:
for _, r := range resResult.Resources {
caps.Resources = append(caps.Resources, vmcp.Resource{
URI: r.URI,
Name: r.Name,
Description: r.Description,
MimeType: r.MIMEType,
BackendID: target.WorkloadID,
})
}
}
}

if serverCaps.Prompts != nil {
promptsResult, listErr := c.ListPrompts(ctx, mcp.ListPromptsRequest{})
if listErr != nil {
switch {
case errors.Is(listErr, mcp.ErrMethodNotFound):
// Spec violation: backend advertised the prompts capability in its
// initialize response but does not implement prompts/list. Recover
// with an empty prompt set so the backend's tools remain usable
// instead of failing init outright. See issue #5231.
slog.Warn("backend advertised prompts capability but does not implement prompts/list",
"backendID", target.WorkloadID,
"method", "prompts/list",
)
case listErr != nil:
return nil, fmt.Errorf("list prompts failed: %w", listErr)
}
for _, p := range promptsResult.Prompts {
args := make([]vmcp.PromptArgument, len(p.Arguments))
for j, a := range p.Arguments {
args[j] = vmcp.PromptArgument{
Name: a.Name,
Description: a.Description,
Required: a.Required,
default:
for _, p := range promptsResult.Prompts {
args := make([]vmcp.PromptArgument, len(p.Arguments))
for j, a := range p.Arguments {
args[j] = vmcp.PromptArgument{
Name: a.Name,
Description: a.Description,
Required: a.Required,
}
}
caps.Prompts = append(caps.Prompts, vmcp.Prompt{
Name: p.Name,
Description: p.Description,
Arguments: args,
BackendID: target.WorkloadID,
})
}
caps.Prompts = append(caps.Prompts, vmcp.Prompt{
Name: p.Name,
Description: p.Description,
Arguments: args,
BackendID: target.WorkloadID,
})
}
}

Expand Down
Loading
Loading