an mcp answer with no result is not a success (V-613)
Two defects in internal/mcp, both about a call that reports done when nothing happened. Client.call accepted a frame carrying our id and neither result nor error. The HTTP transport already refuses one; the stdio transport does not, so the refusal depended on which door the server was behind. Down that path tools/call returns an empty string and a nil error, and the act is recorded as run. Manager.ReadResource reported ErrNoServer for a server that is configured but down. Call keeps those two apart on purpose — one says the tool can never exist, the other says not right now.
This commit is contained in:
+10
-1
@@ -257,7 +257,16 @@ func (c *Client) call(ctx context.Context, method string, params any, out any) e
|
||||
if resp.Error != nil {
|
||||
return fmt.Errorf("mcp: %s: %s: %w", c.name, method, resp.Error)
|
||||
}
|
||||
if out == nil || len(resp.Result) == 0 {
|
||||
// A frame carrying our id and neither result nor error is not an answer.
|
||||
// The HTTP transport already refuses one; the stdio transport does not, and
|
||||
// without this check the refusal depended on which door the server was
|
||||
// behind. Letting it through is the one failure that lies: tools/call
|
||||
// returns an empty string and a nil error, so the act is recorded as done
|
||||
// and the tool never ran.
|
||||
if len(resp.Result) == 0 {
|
||||
return fmt.Errorf("mcp: %s: %s: response carries neither result nor error", c.name, method)
|
||||
}
|
||||
if out == nil {
|
||||
return nil
|
||||
}
|
||||
if err := json.Unmarshal(resp.Result, out); err != nil {
|
||||
|
||||
@@ -525,10 +525,16 @@ func (m *Manager) Resources(ctx context.Context) []Resource {
|
||||
|
||||
// ReadResource reads one resource from one server.
|
||||
func (m *Manager) ReadResource(ctx context.Context, server, uri string) (string, error) {
|
||||
cl, cfg, _, _, _ := m.lookup(server, "")
|
||||
if cl == nil {
|
||||
cl, cfg, _, configured, _ := m.lookup(server, "")
|
||||
if !configured {
|
||||
return "", fmt.Errorf("%w: %s", ErrNoServer, server)
|
||||
}
|
||||
// A configured server that is merely down is not an unknown server. Call
|
||||
// already keeps the two apart; reporting ErrNoServer here tells a caller
|
||||
// the resource can never exist, when the truth is "not right now".
|
||||
if cl == nil {
|
||||
return "", fmt.Errorf("%w: %s", ErrNotConnected, server)
|
||||
}
|
||||
cctx, cancel := context.WithTimeout(ctx, cfg.Timeout)
|
||||
defer cancel()
|
||||
return cl.ReadResource(cctx, uri)
|
||||
|
||||
@@ -641,3 +641,59 @@ func TestReconnectBackoffGrows(t *testing.T) {
|
||||
t.Fatalf("first retry = %v, want %v", c.backoff(), DefaultReconnectEvery)
|
||||
}
|
||||
}
|
||||
|
||||
// emptyFrameTransport answers the handshake normally and then replies to every
|
||||
// later call with a well-formed frame carrying our id and nothing else — no
|
||||
// result, no error. That is the answer a partially-implemented server gives,
|
||||
// and it is the one that lies: without a check it reads as success.
|
||||
type emptyFrameTransport struct{ handshaken bool }
|
||||
|
||||
func (t *emptyFrameTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse, error) {
|
||||
id := req.ID
|
||||
if !t.handshaken {
|
||||
t.handshaken = true
|
||||
raw, _ := json.Marshal(map[string]any{
|
||||
"protocolVersion": ProtocolVersion,
|
||||
"serverInfo": map[string]any{"name": "empty", "version": "0"},
|
||||
})
|
||||
return &rpcResponse{JSONRPC: "2.0", ID: &id, Result: raw}, nil
|
||||
}
|
||||
return &rpcResponse{JSONRPC: "2.0", ID: &id}, nil
|
||||
}
|
||||
|
||||
func (t *emptyFrameTransport) Notify(context.Context, string, any) error { return nil }
|
||||
func (t *emptyFrameTransport) Close() error { return nil }
|
||||
|
||||
func TestResultlessResponseIsNotSuccess(t *testing.T) {
|
||||
c := newClient("empty", &emptyFrameTransport{})
|
||||
if err := c.Initialize(context.Background()); err != nil {
|
||||
t.Fatalf("initialize: %v", err)
|
||||
}
|
||||
out, err := c.CallTool(context.Background(), "break_thing", map[string]any{"q": "x"})
|
||||
if err == nil {
|
||||
t.Fatalf("a frame with neither result nor error must not read as success (got %q)", out)
|
||||
}
|
||||
if out != "" {
|
||||
t.Fatalf("out = %q", out)
|
||||
}
|
||||
if _, err := c.ListTools(context.Background()); err == nil {
|
||||
t.Fatal("tools/list with no result must be an error, not an empty catalogue")
|
||||
}
|
||||
}
|
||||
|
||||
func TestReadResourceOnDownServerIsNotConnected(t *testing.T) {
|
||||
// A url server with no poster factory: configured, validated, never dialed.
|
||||
m, err := NewManager(nil, []ServerConfig{{
|
||||
Name: "down", URL: "http://example.test/mcp", Enabled: true,
|
||||
}})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
m.Connect(context.Background())
|
||||
if _, err := m.ReadResource(context.Background(), "down", "note://one"); !errors.Is(err, ErrNotConnected) {
|
||||
t.Fatalf("err = %v, want ErrNotConnected", err)
|
||||
}
|
||||
if _, err := m.ReadResource(context.Background(), "nosuch", "note://one"); !errors.Is(err, ErrNoServer) {
|
||||
t.Fatalf("err = %v, want ErrNoServer", err)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user