From 8c1e45715013d6c77b2302b5424647982de1ae76 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 04:18:53 +0400 Subject: [PATCH] an mcp answer with no result is not a success (V-613) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/mcp/client.go | 11 +++++++- internal/mcp/manager.go | 10 +++++-- internal/mcp/mcp_test.go | 56 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 826c3f1..c7033d4 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -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 { diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index 9c8c433..4250a63 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -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) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 9c8c966..a1f2fbc 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -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) + } +}