From af45b1a0d598edc497daab09ecf16a4d1a825d16 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 6 Aug 2026 21:57:53 +0530 Subject: [PATCH 1/2] fix(mcp): name the non-text blocks a tool result drops TextContent keeps only blocks whose type is "text", so a server that returns an image, audio, or an embedded resource produced "(empty MCP tool result)". A screenshot or browser server would complete a call successfully, hand back a perfectly good image, and the model would be told nothing came back. It then usually retries, which spends another call to reach the same empty answer, and the user never learns an image existed at all. Carrying the payload is the larger half of #823 and is not this change. Dropping silently is the part that causes the retry loop, and it can be fixed on its own. Decode mimeType on Content, and add DroppedContentSummary, which describes what TextContent discarded ("1 image/png block", "2 resource blocks, 1 audio/wav block"), grouping by mime type and ordering by first appearance so the same result always reads the same way. Servers that omit mimeType fall back to the block type rather than printing an empty label. registryTool.Run appends that as a note, and only when something was actually dropped, so a text-only result is byte-for-byte unchanged. The note also says retrying will return the same thing, since telling the model what happened without telling it not to retry only fixes half the loop. Refs #823. --- internal/mcp/client.go | 53 ++++++++++ internal/mcp/non_text_content_test.go | 146 ++++++++++++++++++++++++++ internal/mcp/registry.go | 12 +++ 3 files changed, 211 insertions(+) create mode 100644 internal/mcp/non_text_content_test.go diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 665e0dfa2..064e7f213 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -26,6 +26,10 @@ type RemoteTool struct { type Content struct { Type string `json:"type"` Text string `json:"text,omitempty"` + // MimeType names what a non-text block holds. Decoded but not yet forwarded: + // it is what lets a dropped block be described to the model instead of + // vanishing (#823). Servers that omit it still decode fine. + MimeType string `json:"mimeType,omitempty"` } type CallToolResult struct { @@ -495,3 +499,52 @@ func TextContent(content []Content) string { } return strings.TrimSpace(strings.Join(parts, "\n")) } + +// DroppedContentSummary describes the blocks TextContent discards, e.g. +// "1 image/png block" or "2 resource blocks, 1 audio/wav block". It returns "" +// when a result is entirely text, so a caller adds nothing to the ordinary case. +// +// This exists because dropping silently is the worst available behaviour. A +// screenshot server returns a valid image, TextContent keeps nothing, and the +// call is reported as "(empty MCP tool result)" — so the model concludes the +// tool produced nothing and usually retries, burning another call on the same +// empty answer. Naming what came back costs nothing and ends that loop even +// though the payload still cannot be forwarded. +// +// Counts are grouped by mime type and ordered by first appearance, so the same +// result always produces the same sentence. +func DroppedContentSummary(content []Content) string { + labels := make([]string, 0, len(content)) + counts := make(map[string]int, len(content)) + for _, item := range content { + if item.Type == "text" { + continue + } + // Prefer the mime type: "image/png" tells the reader more than "image". + // A server may omit it, so fall back to the block type rather than + // printing an empty label. + label := strings.TrimSpace(item.MimeType) + if label == "" { + label = strings.TrimSpace(item.Type) + } + if label == "" { + label = "unknown" + } + if _, seen := counts[label]; !seen { + labels = append(labels, label) + } + counts[label]++ + } + if len(labels) == 0 { + return "" + } + parts := make([]string, 0, len(labels)) + for _, label := range labels { + part := fmt.Sprintf("%d %s block", counts[label], label) + if counts[label] != 1 { + part += "s" + } + parts = append(parts, part) + } + return strings.Join(parts, ", ") +} diff --git a/internal/mcp/non_text_content_test.go b/internal/mcp/non_text_content_test.go new file mode 100644 index 000000000..46517107e --- /dev/null +++ b/internal/mcp/non_text_content_test.go @@ -0,0 +1,146 @@ +package mcp + +import ( + "context" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" +) + +// A server that returns only an image currently reports "(empty MCP tool +// result)": TextContent keeps text blocks and drops the rest, so a successful +// call looks like it produced nothing. The model then usually retries, which is +// the worst outcome, and the user is never told an image existed (#823). +// +// Carrying the payload is a separate change. Naming what was dropped is what +// stops the retry loop, and it has to be true of the DELIVERED result, so this +// drives registryTool.Run rather than the helper alone. +func TestAnImageOnlyResultSaysWhatItReturned(t *testing.T) { + tool := registryTool{ + client: &nonTextClient{content: []Content{ + {Type: "image", MimeType: "image/png"}, + }}, + server: Server{Name: "shots"}, + remote: RemoteTool{Name: "screenshot"}, + } + + result := tool.Run(context.Background(), map[string]any{}) + + if strings.Contains(result.Output, "(empty MCP tool result)") { + t.Fatalf("an image-only result still reports empty, so the model will retry:\n%s", result.Output) + } + if !strings.Contains(result.Output, "image/png") { + t.Errorf("the output does not name what the server returned:\n%s", result.Output) + } + if result.Status != tools.StatusOK { + t.Errorf("status = %v, want OK: the call succeeded, we just cannot forward the payload", result.Status) + } +} + +// The quieter half of the same bug: when a result carries text AND an image, +// the text arrives and the image vanishes with no mention at all. +func TestTextAlongsideAnImageStillReportsTheImage(t *testing.T) { + tool := registryTool{ + client: &nonTextClient{content: []Content{ + {Type: "text", Text: "captured the page"}, + {Type: "image", MimeType: "image/png"}, + }}, + server: Server{Name: "shots"}, + remote: RemoteTool{Name: "screenshot"}, + } + + result := tool.Run(context.Background(), map[string]any{}) + + if !strings.Contains(result.Output, "captured the page") { + t.Errorf("the text block was lost:\n%s", result.Output) + } + if !strings.Contains(result.Output, "image/png") { + t.Errorf("the dropped image was not mentioned:\n%s", result.Output) + } +} + +// A text-only result must be byte-for-byte what it was before: this change adds +// a line only when something was actually dropped. +func TestATextOnlyResultIsUnchanged(t *testing.T) { + tool := registryTool{ + client: &nonTextClient{content: []Content{{Type: "text", Text: "plain answer"}}}, + server: Server{Name: "shots"}, + remote: RemoteTool{Name: "lookup"}, + } + + if got := tool.Run(context.Background(), map[string]any{}).Output; got != "plain answer" { + t.Fatalf("output = %q, want exactly %q", got, "plain answer") + } +} + +func TestDroppedContentSummaryNamesTheBlocks(t *testing.T) { + tests := []struct { + name string + content []Content + want string + }{ + { + name: "nothing dropped", + content: []Content{{Type: "text", Text: "hi"}}, + want: "", + }, + { + name: "no content at all", + content: nil, + want: "", + }, + { + name: "one image with a mime type", + content: []Content{{Type: "image", MimeType: "image/png"}}, + want: "1 image/png block", + }, + { + // A server may omit mimeType. Fall back to the block type rather than + // inventing one or printing an empty pair of slashes. + name: "one image without a mime type", + content: []Content{{Type: "image"}}, + want: "1 image block", + }, + { + name: "several of the same kind are counted, not repeated", + content: []Content{ + {Type: "image", MimeType: "image/png"}, + {Type: "image", MimeType: "image/png"}, + }, + want: "2 image/png blocks", + }, + { + // Order follows first appearance so the message is stable to read and + // to assert on. + name: "mixed kinds", + content: []Content{ + {Type: "text", Text: "ignored here"}, + {Type: "resource"}, + {Type: "audio", MimeType: "audio/wav"}, + {Type: "resource"}, + }, + want: "2 resource blocks, 1 audio/wav block", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := DroppedContentSummary(test.content); got != test.want { + t.Fatalf("DroppedContentSummary() = %q, want %q", got, test.want) + } + }) + } +} + +type nonTextClient struct { + content []Content +} + +func (client *nonTextClient) ListTools(context.Context) ([]RemoteTool, error) { return nil, nil } + +func (client *nonTextClient) CallTool(context.Context, string, map[string]any) (CallToolResult, error) { + return CallToolResult{Content: client.content}, nil +} + +func (client *nonTextClient) Close() error { return nil } diff --git a/internal/mcp/registry.go b/internal/mcp/registry.go index 8c9dac7bd..d5ba3d712 100644 --- a/internal/mcp/registry.go +++ b/internal/mcp/registry.go @@ -325,6 +325,18 @@ func (tool registryTool) Run(ctx context.Context, args map[string]any) tools.Res status = tools.StatusError } output := TextContent(result.Content) + // Say what was thrown away. Without this an image-only result reads as + // "(empty MCP tool result)", the model concludes the call produced nothing + // and retries, and the user never learns an image came back (#823). The note + // is appended only when something was actually dropped, so a text-only + // result is byte-for-byte what it was before. + if dropped := DroppedContentSummary(result.Content); dropped != "" { + note := "[zero] this server also returned " + dropped + ", which Zero cannot forward yet. Retrying will return the same thing." + if output == "" { + note = "[zero] this server returned " + dropped + ", which Zero cannot forward yet. Retrying will return the same thing." + } + output = strings.TrimSpace(output + "\n\n" + note) + } if output == "" { output = "(empty MCP tool result)" } From 55d7cc37ef7cf47eab7920b9b7f4b83e78d90435 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 7 Aug 2026 21:03:51 +0530 Subject: [PATCH 2/2] fix(mcp): do not promise a retry returns the same content The dropped-block note said "Retrying will return the same thing". registryTool.Run makes a fresh remote call on every retry, so the server may well answer differently; that is a promise this code is in no position to make. What cannot change is that Zero still has nowhere to put a non-text block, so the note now says retrying cannot RECOVER the payload. Same guidance, a claim that is actually true. The test pins both directions: the new wording must be present, and the absolute claim must not come back. Raised by CodeRabbit on #874. --- internal/mcp/non_text_content_test.go | 12 ++++++++++++ internal/mcp/registry.go | 10 ++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/mcp/non_text_content_test.go b/internal/mcp/non_text_content_test.go index 46517107e..a1082962f 100644 --- a/internal/mcp/non_text_content_test.go +++ b/internal/mcp/non_text_content_test.go @@ -33,6 +33,18 @@ func TestAnImageOnlyResultSaysWhatItReturned(t *testing.T) { if !strings.Contains(result.Output, "image/png") { t.Errorf("the output does not name what the server returned:\n%s", result.Output) } + // Naming the block is only half of it. Without the guidance the model still + // retries, which is the expensive symptom, so the wording is pinned too. + // + // "cannot recover this payload" and not "will return the same thing": every + // retry is a fresh call and the server may answer differently. What cannot + // change is that Zero has nowhere to put a non-text block. + if !strings.Contains(result.Output, "Retrying cannot recover this payload.") { + t.Errorf("the output does not tell the model retrying is pointless:\n%s", result.Output) + } + if strings.Contains(result.Output, "will return the same thing") { + t.Errorf("the output promises an identical response, which a fresh call cannot guarantee:\n%s", result.Output) + } if result.Status != tools.StatusOK { t.Errorf("status = %v, want OK: the call succeeded, we just cannot forward the payload", result.Status) } diff --git a/internal/mcp/registry.go b/internal/mcp/registry.go index d5ba3d712..bb44900b7 100644 --- a/internal/mcp/registry.go +++ b/internal/mcp/registry.go @@ -330,10 +330,16 @@ func (tool registryTool) Run(ctx context.Context, args map[string]any) tools.Res // and retries, and the user never learns an image came back (#823). The note // is appended only when something was actually dropped, so a text-only // result is byte-for-byte what it was before. + // + // It says retrying cannot RECOVER the payload rather than that a retry + // returns the same thing. Each retry is a fresh call, so the server may well + // answer differently; what cannot change is that Zero still has nowhere to + // put a non-text block. Claiming the response would be identical would be a + // promise this code is in no position to make. if dropped := DroppedContentSummary(result.Content); dropped != "" { - note := "[zero] this server also returned " + dropped + ", which Zero cannot forward yet. Retrying will return the same thing." + note := "[zero] this server also returned " + dropped + ", which Zero cannot forward yet. Retrying cannot recover this payload." if output == "" { - note = "[zero] this server returned " + dropped + ", which Zero cannot forward yet. Retrying will return the same thing." + note = "[zero] this server returned " + dropped + ", which Zero cannot forward yet. Retrying cannot recover this payload." } output = strings.TrimSpace(output + "\n\n" + note) }