Merge the mcp sweep: an answer with no result is not a success (#256)

Client.call treated a frame carrying our id and neither result nor error
as success, so CallTool returned an empty string and no error: the act
is logged as run and the tool never ran, and ListTools returned an empty
catalogue silently. httpTransport.Call already refused exactly this and
names it 'the one answer that lies'; stdioTransport.Call did not, so the
refusal depended on which door the server was behind. Refused centrally
now, so both transports are covered.

ReadResource collapsed 'not configured' and 'configured but down' into
ErrNoServer by discarding lookup's configured return. Manager.Call keeps
them apart on purpose, since a caller needs the distinction to avoid
proposing a capability that already exists.

internal/memeval was read end to end and is clean. No commit there.

(V-613)
This commit is contained in:
2026-08-06 04:31:42 +04:00
3 changed files with 74 additions and 3 deletions
+10 -1
View File
@@ -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 {
+8 -2
View File
@@ -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)
+56
View File
@@ -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)
}
}