From bbefa61279cbe2baf65221a5be1aea620ae95387 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 8 Apr 2026 20:51:25 -0400 Subject: [PATCH 1/9] Fix CallToolResult handling across all SDKs When a tool handler returns an MCP CallToolResult object ({ content: [...], isError?: bool }), all four SDKs were JSON-serializing it instead of converting it to ToolResultObject. This caused the LLM to see raw JSON instead of actual tool output. Add detection and conversion of CallToolResult in Node.js, Python, Go, and .NET. The .NET SDK additionally handles Microsoft.Extensions.AI content types (TextContent, DataContent, and unknown subtypes via AIJsonUtilities serialization). Fixes #937 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/src/Client.cs | 28 +++- dotnet/src/Session.cs | 28 +++- dotnet/src/Types.cs | 171 ++++++++++++++++++++ go/definetool.go | 105 ++++++++++++- go/definetool_test.go | 184 ++++++++++++++++++++++ nodejs/src/client.ts | 5 + nodejs/src/index.ts | 3 +- nodejs/src/session.ts | 3 + nodejs/src/types.ts | 113 ++++++++++++++ nodejs/test/call-tool-result.test.ts | 223 +++++++++++++++++++++++++++ python/copilot/tools.py | 52 +++++++ python/e2e/test_tools_unit.py | 90 ++++++++++- 12 files changed, 989 insertions(+), 16 deletions(-) create mode 100644 nodejs/test/call-tool-result.test.ts diff --git a/dotnet/src/Client.cs b/dotnet/src/Client.cs index aad44e4eb..539cd57af 100644 --- a/dotnet/src/Client.cs +++ b/dotnet/src/Client.cs @@ -1549,13 +1549,29 @@ public async Task OnToolCallV2(string sessionId, var result = await tool.InvokeAsync(aiFunctionArgs); - var toolResultObject = result is ToolResultAIContent trac ? trac.Result : new ToolResultObject + ToolResultObject toolResultObject; + if (result is ToolResultAIContent trac) { - ResultType = "success", - TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } je - ? je.GetString()! - : JsonSerializer.Serialize(result, tool.JsonSerializerOptions.GetTypeInfo(typeof(object))), - }; + toolResultObject = trac.Result; + } + else if (ToolResultObject.TryConvertFromAIContent(result) is { } aiConverted) + { + toolResultObject = aiConverted; + } + else if (ToolResultObject.TryConvertFromCallToolResult(result) is { } converted) + { + toolResultObject = converted; + } + else + { + toolResultObject = new ToolResultObject + { + ResultType = "success", + TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } je + ? je.GetString()! + : JsonSerializer.Serialize(result, tool.JsonSerializerOptions.GetTypeInfo(typeof(object))), + }; + } return new ToolCallResponseV2(toolResultObject); } catch (Exception ex) diff --git a/dotnet/src/Session.cs b/dotnet/src/Session.cs index 4e5142cb8..a39c560b0 100644 --- a/dotnet/src/Session.cs +++ b/dotnet/src/Session.cs @@ -568,13 +568,29 @@ private async Task ExecuteToolAndRespondAsync(string requestId, string toolName, var result = await tool.InvokeAsync(aiFunctionArgs); - var toolResultObject = result is ToolResultAIContent trac ? trac.Result : new ToolResultObject + ToolResultObject toolResultObject; + if (result is ToolResultAIContent trac) { - ResultType = "success", - TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } je - ? je.GetString()! - : JsonSerializer.Serialize(result, tool.JsonSerializerOptions.GetTypeInfo(typeof(object))), - }; + toolResultObject = trac.Result; + } + else if (ToolResultObject.TryConvertFromAIContent(result) is { } aiConverted) + { + toolResultObject = aiConverted; + } + else if (ToolResultObject.TryConvertFromCallToolResult(result) is { } converted) + { + toolResultObject = converted; + } + else + { + toolResultObject = new ToolResultObject + { + ResultType = "success", + TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } strJe + ? strJe.GetString()! + : JsonSerializer.Serialize(result, tool.JsonSerializerOptions.GetTypeInfo(typeof(object))), + }; + } await Rpc.Tools.HandlePendingToolCallAsync(requestId, toolResultObject, error: null); } diff --git a/dotnet/src/Types.cs b/dotnet/src/Types.cs index d8262e140..9ac0ddaf6 100644 --- a/dotnet/src/Types.cs +++ b/dotnet/src/Types.cs @@ -324,6 +324,177 @@ public class ToolResultObject /// [JsonPropertyName("toolTelemetry")] public Dictionary? ToolTelemetry { get; set; } + + /// + /// Attempts to interpret the result as an MCP CallToolResult + /// (shape: { content: [...], isError?: bool }) and converts it to a + /// . Returns if the value does + /// not match the expected shape. + /// + internal static ToolResultObject? TryConvertFromCallToolResult(object? result) + { + if (result is not JsonElement element) + { + return null; + } + + if (element.ValueKind != JsonValueKind.Object) + { + return null; + } + + if (!element.TryGetProperty("content", out var contentProp) || contentProp.ValueKind != JsonValueKind.Array) + { + return null; + } + + // Validate every element has a string "type" field + foreach (var item in contentProp.EnumerateArray()) + { + if (item.ValueKind != JsonValueKind.Object || + !item.TryGetProperty("type", out var typeProp) || + typeProp.ValueKind != JsonValueKind.String) + { + return null; + } + } + + List? textParts = null; + List? binaryResults = null; + + foreach (var block in contentProp.EnumerateArray()) + { + var blockType = block.GetProperty("type").GetString(); + + switch (blockType) + { + case "text": + if (block.TryGetProperty("text", out var textProp) && textProp.ValueKind == JsonValueKind.String) + { + (textParts ??= []).Add(textProp.GetString()!); + } + break; + + case "image": + (binaryResults ??= []).Add(new ToolBinaryResult + { + Data = block.TryGetProperty("data", out var imgData) && imgData.ValueKind == JsonValueKind.String ? imgData.GetString() ?? "" : "", + MimeType = block.TryGetProperty("mimeType", out var imgMime) && imgMime.ValueKind == JsonValueKind.String ? imgMime.GetString() ?? "" : "", + Type = "image", + }); + break; + + case "resource": + if (block.TryGetProperty("resource", out var resProp) && resProp.ValueKind == JsonValueKind.Object) + { + if (resProp.TryGetProperty("text", out var resText) && resText.ValueKind == JsonValueKind.String) + { + var text = resText.GetString(); + if (!string.IsNullOrEmpty(text)) + { + (textParts ??= []).Add(text!); + } + } + + if (resProp.TryGetProperty("blob", out var resBlob) && resBlob.ValueKind == JsonValueKind.String) + { + var blob = resBlob.GetString(); + if (!string.IsNullOrEmpty(blob)) + { + var mimeType = resProp.TryGetProperty("mimeType", out var resMime) && resMime.ValueKind == JsonValueKind.String + ? resMime.GetString() ?? "application/octet-stream" + : "application/octet-stream"; + var uri = resProp.TryGetProperty("uri", out var resUri) && resUri.ValueKind == JsonValueKind.String + ? resUri.GetString() + : null; + + (binaryResults ??= []).Add(new ToolBinaryResult + { + Data = blob!, + MimeType = mimeType, + Type = "resource", + Description = uri, + }); + } + } + } + break; + } + } + + var isError = element.TryGetProperty("isError", out var isErrorProp) && + isErrorProp.ValueKind == JsonValueKind.True; + + return new ToolResultObject + { + TextResultForLlm = textParts is not null ? string.Join("\n", textParts) : "", + ResultType = isError ? "failure" : "success", + BinaryResultsForLlm = binaryResults, + }; + } + + /// + /// Attempts to convert a result from an invocation into a + /// . Handles , + /// , and collections of . + /// Returns if the value is not a recognized type. + /// + internal static ToolResultObject? TryConvertFromAIContent(object? result) + { + if (result is AIContent singleContent) + { + return ConvertAIContents([singleContent]); + } + + if (result is IEnumerable contentList) + { + return ConvertAIContents(contentList); + } + + return null; + } + + private static ToolResultObject ConvertAIContents(IEnumerable contents) + { + List? textParts = null; + List? binaryResults = null; + + foreach (var content in contents) + { + switch (content) + { + case TextContent textContent: + if (textContent.Text is { } text) + { + (textParts ??= []).Add(text); + } + break; + + case DataContent dataContent: + (binaryResults ??= []).Add(new ToolBinaryResult + { + Data = dataContent.Base64Data.ToString(), + MimeType = dataContent.MediaType ?? "application/octet-stream", + Type = dataContent.HasTopLevelMediaType("image") ? "image" : "resource", + }); + break; + + default: + (textParts ??= []).Add(SerializeAIContent(content)); + break; + } + } + + return new ToolResultObject + { + TextResultForLlm = textParts is not null ? string.Join("\n", textParts) : "", + ResultType = "success", + BinaryResultsForLlm = binaryResults, + }; + } + + private static string SerializeAIContent(AIContent content) => + JsonSerializer.Serialize(content, AIJsonUtilities.DefaultOptions.GetTypeInfo(typeof(AIContent))); } /// diff --git a/go/definetool.go b/go/definetool.go index 406a8c0b8..d661e72fe 100644 --- a/go/definetool.go +++ b/go/definetool.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "reflect" + "strings" "github.com/google/jsonschema-go/jsonschema" ) @@ -65,7 +66,8 @@ func createTypedHandler[T any, U any](handler func(T, ToolInvocation) (U, error) } // normalizeResult converts any value to a ToolResult. -// Strings pass through directly, ToolResult passes through, other types are JSON-serialized. +// Strings pass through directly, ToolResult passes through, CallToolResult is +// converted, and other types are JSON-serialized. func normalizeResult(result any) (ToolResult, error) { if result == nil { return ToolResult{ @@ -87,6 +89,11 @@ func normalizeResult(result any) (ToolResult, error) { }, nil } + // MCP CallToolResult shape: { content: [...], isError?: bool } + if tr, ok := convertCallToolResult(result); ok { + return tr, nil + } + // Everything else gets JSON-serialized jsonBytes, err := json.Marshal(result) if err != nil { @@ -99,7 +106,101 @@ func normalizeResult(result any) (ToolResult, error) { }, nil } -// generateSchemaForType generates a JSON schema map from a Go type using reflection. +// convertCallToolResult attempts to interpret value as an MCP CallToolResult +// (map with "content" array and optional "isError" bool). Returns the converted +// ToolResult and true if it matched, or a zero ToolResult and false otherwise. +func convertCallToolResult(value any) (ToolResult, bool) { + m, ok := value.(map[string]any) + if !ok { + jsonBytes, err := json.Marshal(value) + if err != nil { + return ToolResult{}, false + } + + if err := json.Unmarshal(jsonBytes, &m); err != nil { + return ToolResult{}, false + } + } + + contentRaw, exists := m["content"] + if !exists { + return ToolResult{}, false + } + + contentSlice, ok := contentRaw.([]any) + if !ok { + return ToolResult{}, false + } + + // Verify every element has a string "type" field + for _, item := range contentSlice { + block, ok := item.(map[string]any) + if !ok { + return ToolResult{}, false + } + if _, ok := block["type"].(string); !ok { + return ToolResult{}, false + } + } + + var textParts []string + var binaryResults []ToolBinaryResult + + for _, item := range contentSlice { + block := item.(map[string]any) + blockType := block["type"].(string) + + switch blockType { + case "text": + if text, ok := block["text"].(string); ok { + textParts = append(textParts, text) + } + case "image": + data, _ := block["data"].(string) + mimeType, _ := block["mimeType"].(string) + binaryResults = append(binaryResults, ToolBinaryResult{ + Data: data, + MimeType: mimeType, + Type: "image", + }) + case "resource": + if resRaw, ok := block["resource"].(map[string]any); ok { + if text, ok := resRaw["text"].(string); ok && text != "" { + textParts = append(textParts, text) + } + if blob, ok := resRaw["blob"].(string); ok && blob != "" { + mimeType, _ := resRaw["mimeType"].(string) + if mimeType == "" { + mimeType = "application/octet-stream" + } + uri, _ := resRaw["uri"].(string) + binaryResults = append(binaryResults, ToolBinaryResult{ + Data: blob, + MimeType: mimeType, + Type: "resource", + Description: uri, + }) + } + } + } + } + + resultType := "success" + if isErr, ok := m["isError"].(bool); ok && isErr { + resultType = "failure" + } + + tr := ToolResult{ + TextResultForLLM: strings.Join(textParts, "\n"), + ResultType: resultType, + } + if len(binaryResults) > 0 { + tr.BinaryResultsForLLM = binaryResults + } + return tr, true +} + +// generateSchemaForTypegenerates a JSON schema map from a Go type using reflection. // Panics if schema generation fails, as this indicates a programming error. func generateSchemaForType(t reflect.Type) map[string]any { if t == nil { diff --git a/go/definetool_test.go b/go/definetool_test.go index af620b180..9181cebbd 100644 --- a/go/definetool_test.go +++ b/go/definetool_test.go @@ -253,6 +253,190 @@ func TestNormalizeResult(t *testing.T) { }) } +func TestConvertCallToolResult(t *testing.T) { + t.Run("typed CallToolResult struct is converted", func(t *testing.T) { + type Resource struct { + URI string `json:"uri"` + Text string `json:"text"` + } + type ContentBlock struct { + Type string `json:"type"` + Resource *Resource `json:"resource,omitempty"` + } + type CallToolResult struct { + Content []ContentBlock `json:"content"` + } + + input := CallToolResult{ + Content: []ContentBlock{ + { + Type: "resource", + Resource: &Resource{URI: "file:///report.txt", Text: "details"}, + }, + }, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result.TextResultForLLM != "details" { + t.Errorf("Expected 'details', got %q", result.TextResultForLLM) + } + if result.ResultType != "success" { + t.Errorf("Expected 'success', got %q", result.ResultType) + } + }) + + t.Run("text-only CallToolResult is converted", func(t *testing.T) { + input := map[string]any{ + "content": []any{ + map[string]any{"type": "text", "text": "hello"}, + }, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result.TextResultForLLM != "hello" { + t.Errorf("Expected 'hello', got %q", result.TextResultForLLM) + } + if result.ResultType != "success" { + t.Errorf("Expected 'success', got %q", result.ResultType) + } + }) + + t.Run("multiple text blocks are joined with newline", func(t *testing.T) { + input := map[string]any{ + "content": []any{ + map[string]any{"type": "text", "text": "line 1"}, + map[string]any{"type": "text", "text": "line 2"}, + }, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result.TextResultForLLM != "line 1\nline 2" { + t.Errorf("Expected 'line 1\\nline 2', got %q", result.TextResultForLLM) + } + }) + + t.Run("isError maps to failure resultType", func(t *testing.T) { + input := map[string]any{ + "content": []any{ + map[string]any{"type": "text", "text": "oops"}, + }, + "isError": true, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result.ResultType != "failure" { + t.Errorf("Expected 'failure', got %q", result.ResultType) + } + }) + + t.Run("image content becomes binaryResultsForLLM", func(t *testing.T) { + input := map[string]any{ + "content": []any{ + map[string]any{"type": "image", "data": "base64data", "mimeType": "image/png"}, + }, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if len(result.BinaryResultsForLLM) != 1 { + t.Fatalf("Expected 1 binary result, got %d", len(result.BinaryResultsForLLM)) + } + if result.BinaryResultsForLLM[0].Data != "base64data" { + t.Errorf("Expected data 'base64data', got %q", result.BinaryResultsForLLM[0].Data) + } + if result.BinaryResultsForLLM[0].MimeType != "image/png" { + t.Errorf("Expected mimeType 'image/png', got %q", result.BinaryResultsForLLM[0].MimeType) + } + }) + + t.Run("resource text goes to textResultForLLM", func(t *testing.T) { + input := map[string]any{ + "content": []any{ + map[string]any{ + "type": "resource", + "resource": map[string]any{"uri": "file:///tmp/data.txt", "text": "file contents"}, + }, + }, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result.TextResultForLLM != "file contents" { + t.Errorf("Expected 'file contents', got %q", result.TextResultForLLM) + } + }) + + t.Run("resource blob goes to binaryResultsForLLM", func(t *testing.T) { + input := map[string]any{ + "content": []any{ + map[string]any{ + "type": "resource", + "resource": map[string]any{"uri": "file:///img.png", "blob": "blobdata", "mimeType": "image/png"}, + }, + }, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if len(result.BinaryResultsForLLM) != 1 { + t.Fatalf("Expected 1 binary result, got %d", len(result.BinaryResultsForLLM)) + } + if result.BinaryResultsForLLM[0].Description != "file:///img.png" { + t.Errorf("Expected description 'file:///img.png', got %q", result.BinaryResultsForLLM[0].Description) + } + }) + + t.Run("non-CallToolResult map is JSON serialized", func(t *testing.T) { + input := map[string]any{ + "key": "value", + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + expected := `{"key":"value"}` + if result.TextResultForLLM != expected { + t.Errorf("Expected %q, got %q", expected, result.TextResultForLLM) + } + }) + + t.Run("empty content array is converted", func(t *testing.T) { + input := map[string]any{ + "content": []any{}, + } + + result, err := normalizeResult(input) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if result.TextResultForLLM != "" { + t.Errorf("Expected empty text, got %q", result.TextResultForLLM) + } + if result.ResultType != "success" { + t.Errorf("Expected 'success', got %q", result.ResultType) + } + }) +} + func TestGenerateSchemaForType(t *testing.T) { t.Run("generates schema for simple struct", func(t *testing.T) { type Simple struct { diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 6941598b8..b5eb1eb64 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -55,6 +55,7 @@ import type { TraceContextProvider, TypedSessionLifecycleHandler, } from "./types.js"; +import { isCallToolResult, convertCallToolResult } from "./types.js"; /** * Minimum protocol version this SDK can communicate with. @@ -1929,6 +1930,10 @@ export class CopilotClient { return result; } + if (isCallToolResult(result)) { + return convertCallToolResult(result); + } + const textResult = typeof result === "string" ? result : JSON.stringify(result); return { textResultForLlm: textResult, diff --git a/nodejs/src/index.ts b/nodejs/src/index.ts index 3fab122db..9281725dd 100644 --- a/nodejs/src/index.ts +++ b/nodejs/src/index.ts @@ -10,7 +10,7 @@ export { CopilotClient } from "./client.js"; export { CopilotSession, type AssistantMessageEvent } from "./session.js"; -export { defineTool, approveAll, SYSTEM_PROMPT_SECTIONS } from "./types.js"; +export { defineTool, approveAll, isCallToolResult, convertCallToolResult, SYSTEM_PROMPT_SECTIONS } from "./types.js"; export type { CommandContext, CommandDefinition, @@ -73,6 +73,7 @@ export type { ToolHandler, ToolInvocation, ToolResultObject, + CallToolResult, TypedSessionEventHandler, TypedSessionLifecycleHandler, ZodSchema, diff --git a/nodejs/src/session.ts b/nodejs/src/session.ts index ffb2c045a..fc08aa031 100644 --- a/nodejs/src/session.ts +++ b/nodejs/src/session.ts @@ -43,6 +43,7 @@ import type { UserInputRequest, UserInputResponse, } from "./types.js"; +import { isCallToolResult, convertCallToolResult } from "./types.js"; export const NO_RESULT_PERMISSION_V2_ERROR = "Permission handlers cannot return 'no-result' when connected to a protocol v2 server."; @@ -478,6 +479,8 @@ export class CopilotSession { result = rawResult; } else if (isToolResultObject(rawResult)) { result = rawResult; + } else if (isCallToolResult(rawResult)) { + result = convertCallToolResult(rawResult); } else { result = JSON.stringify(rawResult); } diff --git a/nodejs/src/types.ts b/nodejs/src/types.ts index c2d095234..386b59de3 100644 --- a/nodejs/src/types.ts +++ b/nodejs/src/types.ts @@ -207,6 +207,119 @@ export type ToolResultObject = { export type ToolResult = string | ToolResultObject; +// ============================================================================ +// MCP CallToolResult support +// ============================================================================ + +/** + * Content block types within an MCP CallToolResult. + */ +type CallToolResultTextContent = { + type: "text"; + text: string; +}; + +type CallToolResultImageContent = { + type: "image"; + data: string; + mimeType: string; +}; + +type CallToolResultResourceContent = { + type: "resource"; + resource: { + uri: string; + mimeType?: string; + text?: string; + blob?: string; + }; +}; + +type CallToolResultContent = + | CallToolResultTextContent + | CallToolResultImageContent + | CallToolResultResourceContent; + +/** + * MCP-compatible CallToolResult type. When a tool handler returns this shape, + * the SDK automatically converts it to a {@link ToolResultObject} before + * sending it over RPC. + */ +export type CallToolResult = { + content: CallToolResultContent[]; + isError?: boolean; +}; + +/** + * Type guard that checks whether a value is shaped like an MCP CallToolResult. + */ +export function isCallToolResult(value: unknown): value is CallToolResult { + if (typeof value !== "object" || value === null) { + return false; + } + + const obj = value as Record; + if (!Array.isArray(obj.content)) { + return false; + } + + // Verify every element in content has a valid "type" field + return obj.content.every( + (item: unknown) => + typeof item === "object" && + item !== null && + "type" in item && + typeof (item as Record).type === "string" + ); +} + +/** + * Converts an MCP CallToolResult into the SDK's ToolResultObject format. + */ +export function convertCallToolResult(callResult: CallToolResult): ToolResultObject { + const textParts: string[] = []; + const binaryResults: ToolBinaryResult[] = []; + + for (const block of callResult.content) { + switch (block.type) { + case "text": + // Guard against malformed input where text field is missing at runtime + if (typeof block.text === "string") { + textParts.push(block.text); + } + break; + case "image": + binaryResults.push({ + data: block.data, + mimeType: block.mimeType, + type: "image", + }); + break; + case "resource": { + // Use optional chaining: resource field may be absent in malformed input + if (block.resource?.text) { + textParts.push(block.resource.text); + } + if (block.resource?.blob) { + binaryResults.push({ + data: block.resource.blob, + mimeType: block.resource.mimeType ?? "application/octet-stream", + type: "resource", + description: block.resource.uri, + }); + } + break; + } + } + } + + return { + textResultForLlm: textParts.join("\n"), + resultType: callResult.isError ? "failure" : "success", + ...(binaryResults.length > 0 ? { binaryResultsForLlm: binaryResults } : {}), + }; +} + export interface ToolInvocation { sessionId: string; toolCallId: string; diff --git a/nodejs/test/call-tool-result.test.ts b/nodejs/test/call-tool-result.test.ts new file mode 100644 index 000000000..0256da112 --- /dev/null +++ b/nodejs/test/call-tool-result.test.ts @@ -0,0 +1,223 @@ +import { describe, expect, it } from "vitest"; +import { isCallToolResult, convertCallToolResult } from "../src/types.js"; +import type { CallToolResult, ToolResultObject } from "../src/types.js"; + +describe("isCallToolResult", () => { + it("returns true for a text-only CallToolResult", () => { + expect( + isCallToolResult({ + content: [{ type: "text", text: "hello" }], + }) + ).toBe(true); + }); + + it("returns true for CallToolResult with isError", () => { + expect( + isCallToolResult({ + content: [{ type: "text", text: "oops" }], + isError: true, + }) + ).toBe(true); + }); + + it("returns true for CallToolResult with image content", () => { + expect( + isCallToolResult({ + content: [{ type: "image", data: "abc123", mimeType: "image/png" }], + }) + ).toBe(true); + }); + + it("returns true for CallToolResult with resource content", () => { + expect( + isCallToolResult({ + content: [ + { type: "resource", resource: { uri: "file:///tmp/out.txt", text: "data" } }, + ], + }) + ).toBe(true); + }); + + it("returns true for empty content array", () => { + expect(isCallToolResult({ content: [] })).toBe(true); + }); + + it("returns false for null", () => { + expect(isCallToolResult(null)).toBe(false); + }); + + it("returns false for a string", () => { + expect(isCallToolResult("hello")).toBe(false); + }); + + it("returns false for a ToolResultObject", () => { + expect( + isCallToolResult({ textResultForLlm: "hi", resultType: "success" }) + ).toBe(false); + }); + + it("returns false when content is not an array", () => { + expect(isCallToolResult({ content: "text" })).toBe(false); + }); + + it("returns false when content items lack type field", () => { + expect(isCallToolResult({ content: [{ text: "no type" }] })).toBe(false); + }); +}); + +describe("convertCallToolResult", () => { + it("extracts text from text content blocks", () => { + const input: CallToolResult = { + content: [ + { type: "text", text: "line 1" }, + { type: "text", text: "line 2" }, + ], + }; + + const result = convertCallToolResult(input); + + expect(result.textResultForLlm).toBe("line 1\nline 2"); + expect(result.resultType).toBe("success"); + expect(result.binaryResultsForLlm).toBeUndefined(); + }); + + it("maps isError to failure resultType", () => { + const input: CallToolResult = { + content: [{ type: "text", text: "error occurred" }], + isError: true, + }; + + const result = convertCallToolResult(input); + + expect(result.textResultForLlm).toBe("error occurred"); + expect(result.resultType).toBe("failure"); + }); + + it("maps isError: false to success", () => { + const input: CallToolResult = { + content: [{ type: "text", text: "ok" }], + isError: false, + }; + + expect(convertCallToolResult(input).resultType).toBe("success"); + }); + + it("converts image content to binaryResultsForLlm", () => { + const input: CallToolResult = { + content: [{ type: "image", data: "base64data", mimeType: "image/png" }], + }; + + const result = convertCallToolResult(input); + + expect(result.textResultForLlm).toBe(""); + expect(result.binaryResultsForLlm).toHaveLength(1); + expect(result.binaryResultsForLlm![0]).toEqual({ + data: "base64data", + mimeType: "image/png", + type: "image", + }); + }); + + it("converts resource with text to textResultForLlm", () => { + const input: CallToolResult = { + content: [ + { + type: "resource", + resource: { uri: "file:///tmp/data.txt", text: "file contents" }, + }, + ], + }; + + const result = convertCallToolResult(input); + + expect(result.textResultForLlm).toBe("file contents"); + }); + + it("converts resource with blob to binaryResultsForLlm", () => { + const input: CallToolResult = { + content: [ + { + type: "resource", + resource: { + uri: "file:///tmp/image.png", + mimeType: "image/png", + blob: "blobdata", + }, + }, + ], + }; + + const result = convertCallToolResult(input); + + expect(result.binaryResultsForLlm).toHaveLength(1); + expect(result.binaryResultsForLlm![0]).toEqual({ + data: "blobdata", + mimeType: "image/png", + type: "resource", + description: "file:///tmp/image.png", + }); + }); + + it("handles mixed content types", () => { + const input: CallToolResult = { + content: [ + { type: "text", text: "Analysis complete" }, + { type: "image", data: "chartdata", mimeType: "image/svg+xml" }, + { + type: "resource", + resource: { uri: "file:///report.txt", text: "Report details" }, + }, + ], + }; + + const result = convertCallToolResult(input); + + expect(result.textResultForLlm).toBe("Analysis complete\nReport details"); + expect(result.binaryResultsForLlm).toHaveLength(1); + expect(result.binaryResultsForLlm![0]!.mimeType).toBe("image/svg+xml"); + }); + + it("handles empty content array", () => { + const result = convertCallToolResult({ content: [] }); + + expect(result.textResultForLlm).toBe(""); + expect(result.resultType).toBe("success"); + expect(result.binaryResultsForLlm).toBeUndefined(); + }); + + it("defaults resource blob mimeType to application/octet-stream", () => { + const input: CallToolResult = { + content: [ + { + type: "resource", + resource: { uri: "file:///data.bin", blob: "binarydata" }, + }, + ], + }; + + const result = convertCallToolResult(input); + + expect(result.binaryResultsForLlm![0]!.mimeType).toBe("application/octet-stream"); + }); + + it("handles text block with missing text field without corrupting output", () => { + // isCallToolResult only checks that type is a string, not that type-specific + // fields are present. convertCallToolResult must be defensive at runtime. + const input = { content: [{ type: "text" }] } as unknown as CallToolResult; + + const result = convertCallToolResult(input); + + expect(result.textResultForLlm).toBe(""); + expect(result.textResultForLlm).not.toBe("undefined"); + }); + + it("handles resource block with missing resource field without crashing", () => { + // A resource content item missing the resource field would crash with an + // unguarded block.resource.text access. Optional chaining must be used. + const input = { content: [{ type: "resource" }] } as unknown as CallToolResult; + + expect(() => convertCallToolResult(input)).not.toThrow(); + const result = convertCallToolResult(input); + expect(result.textResultForLlm).toBe(""); + }); +}); diff --git a/python/copilot/tools.py b/python/copilot/tools.py index 66c660536..3aeb9dab8 100644 --- a/python/copilot/tools.py +++ b/python/copilot/tools.py @@ -240,6 +240,7 @@ def _normalize_result(result: Any) -> ToolResult: - None returns empty success - Strings pass through directly - ToolResult passes through + - MCP CallToolResult dicts are converted automatically - Everything else gets JSON-serialized (with Pydantic support) """ if result is None: @@ -259,6 +260,10 @@ def _normalize_result(result: Any) -> ToolResult: result_type="success", ) + # MCP CallToolResult shape: { content: [...], isError?: bool } + if _is_call_tool_result(result): + return _convert_call_tool_result(result) + # Everything else gets JSON-serialized (with Pydantic model support) def default(obj: Any) -> Any: if isinstance(obj, BaseModel): @@ -274,3 +279,50 @@ def default(obj: Any) -> Any: text_result_for_llm=json_str, result_type="success", ) + + +def _is_call_tool_result(value: Any) -> bool: + """Check whether a value is shaped like an MCP CallToolResult.""" + if not isinstance(value, dict): + return False + content = value.get("content") + if not isinstance(content, list): + return False + return all( + isinstance(item, dict) and isinstance(item.get("type"), str) + for item in content + ) + + +def _convert_call_tool_result(call_result: dict[str, Any]) -> ToolResult: + """Convert an MCP CallToolResult dict into a ToolResult.""" + text_parts: list[str] = [] + binary_results: list[ToolBinaryResult] = [] + + for block in call_result["content"]: + block_type = block.get("type") + if block_type == "text": + text_parts.append(block.get("text", "")) + elif block_type == "image": + binary_results.append(ToolBinaryResult( + data=block.get("data", ""), + mime_type=block.get("mimeType", ""), + type="image", + )) + elif block_type == "resource": + resource = block.get("resource", {}) + if resource.get("text"): + text_parts.append(resource["text"]) + if resource.get("blob"): + binary_results.append(ToolBinaryResult( + data=resource["blob"], + mime_type=resource.get("mimeType", "application/octet-stream"), + type="resource", + description=resource.get("uri", ""), + )) + + return ToolResult( + text_result_for_llm="\n".join(text_parts), + result_type="failure" if call_result.get("isError") else "success", + binary_results_for_llm=binary_results if binary_results else None, + ) diff --git a/python/e2e/test_tools_unit.py b/python/e2e/test_tools_unit.py index c9c996f0e..637d1f8eb 100644 --- a/python/e2e/test_tools_unit.py +++ b/python/e2e/test_tools_unit.py @@ -6,7 +6,7 @@ from pydantic import BaseModel, Field from copilot import define_tool -from copilot.tools import ToolInvocation, ToolResult, _normalize_result +from copilot.tools import ToolInvocation, ToolResult, _normalize_result, _is_call_tool_result, _convert_call_tool_result class TestDefineTool: @@ -284,3 +284,91 @@ def test_raises_for_unserializable_value(self): # Functions cannot be JSON serialized with pytest.raises(TypeError, match="Failed to serialize"): _normalize_result(lambda x: x) + + +class TestCallToolResult: + def test_text_only_call_tool_result(self): + result = _normalize_result({ + "content": [{"type": "text", "text": "hello"}], + }) + assert result.text_result_for_llm == "hello" + assert result.result_type == "success" + + def test_multiple_text_blocks(self): + result = _normalize_result({ + "content": [ + {"type": "text", "text": "line 1"}, + {"type": "text", "text": "line 2"}, + ], + }) + assert result.text_result_for_llm == "line 1\nline 2" + + def test_is_error_maps_to_failure(self): + result = _normalize_result({ + "content": [{"type": "text", "text": "oops"}], + "isError": True, + }) + assert result.result_type == "failure" + + def test_is_error_false_maps_to_success(self): + result = _normalize_result({ + "content": [{"type": "text", "text": "ok"}], + "isError": False, + }) + assert result.result_type == "success" + + def test_image_content_to_binary(self): + result = _normalize_result({ + "content": [{"type": "image", "data": "base64data", "mimeType": "image/png"}], + }) + assert result.binary_results_for_llm is not None + assert len(result.binary_results_for_llm) == 1 + assert result.binary_results_for_llm[0].data == "base64data" + assert result.binary_results_for_llm[0].mime_type == "image/png" + assert result.binary_results_for_llm[0].type == "image" + + def test_resource_text_to_text_result(self): + result = _normalize_result({ + "content": [ + {"type": "resource", "resource": {"uri": "file:///data.txt", "text": "file contents"}}, + ], + }) + assert result.text_result_for_llm == "file contents" + + def test_resource_blob_to_binary(self): + result = _normalize_result({ + "content": [ + { + "type": "resource", + "resource": {"uri": "file:///img.png", "blob": "blobdata", "mimeType": "image/png"}, + }, + ], + }) + assert result.binary_results_for_llm is not None + assert len(result.binary_results_for_llm) == 1 + assert result.binary_results_for_llm[0].data == "blobdata" + assert result.binary_results_for_llm[0].description == "file:///img.png" + + def test_empty_content_array(self): + result = _normalize_result({"content": []}) + assert result.text_result_for_llm == "" + assert result.result_type == "success" + + def test_non_call_tool_result_dict_is_json_serialized(self): + result = _normalize_result({"key": "value"}) + parsed = json.loads(result.text_result_for_llm) + assert parsed == {"key": "value"} + + def test_is_call_tool_result_false_for_non_dict(self): + assert _is_call_tool_result("hello") is False + assert _is_call_tool_result(None) is False + assert _is_call_tool_result(42) is False + + def test_is_call_tool_result_false_without_content(self): + assert _is_call_tool_result({"key": "value"}) is False + + def test_is_call_tool_result_false_when_content_not_list(self): + assert _is_call_tool_result({"content": "text"}) is False + + def test_is_call_tool_result_false_when_items_lack_type(self): + assert _is_call_tool_result({"content": [{"text": "no type"}]}) is False From 5a326f329681eff78340d3e4e6428b469f7bab41 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 8 Apr 2026 20:54:26 -0400 Subject: [PATCH 2/9] Fix formatting and lint issues Run prettier on Node.js files, ruff format on Python files, and remove unused ToolResultObject import from test file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nodejs/src/index.ts | 8 ++- nodejs/test/call-tool-result.test.ts | 6 +- python/copilot/tools.py | 31 ++++----- python/e2e/test_tools_unit.py | 95 ++++++++++++++++++---------- 4 files changed, 86 insertions(+), 54 deletions(-) diff --git a/nodejs/src/index.ts b/nodejs/src/index.ts index 9281725dd..9b331ec6d 100644 --- a/nodejs/src/index.ts +++ b/nodejs/src/index.ts @@ -10,7 +10,13 @@ export { CopilotClient } from "./client.js"; export { CopilotSession, type AssistantMessageEvent } from "./session.js"; -export { defineTool, approveAll, isCallToolResult, convertCallToolResult, SYSTEM_PROMPT_SECTIONS } from "./types.js"; +export { + defineTool, + approveAll, + isCallToolResult, + convertCallToolResult, + SYSTEM_PROMPT_SECTIONS, +} from "./types.js"; export type { CommandContext, CommandDefinition, diff --git a/nodejs/test/call-tool-result.test.ts b/nodejs/test/call-tool-result.test.ts index 0256da112..3cfe2bbcf 100644 --- a/nodejs/test/call-tool-result.test.ts +++ b/nodejs/test/call-tool-result.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { isCallToolResult, convertCallToolResult } from "../src/types.js"; -import type { CallToolResult, ToolResultObject } from "../src/types.js"; +import type { CallToolResult } from "../src/types.js"; describe("isCallToolResult", () => { it("returns true for a text-only CallToolResult", () => { @@ -51,9 +51,7 @@ describe("isCallToolResult", () => { }); it("returns false for a ToolResultObject", () => { - expect( - isCallToolResult({ textResultForLlm: "hi", resultType: "success" }) - ).toBe(false); + expect(isCallToolResult({ textResultForLlm: "hi", resultType: "success" })).toBe(false); }); it("returns false when content is not an array", () => { diff --git a/python/copilot/tools.py b/python/copilot/tools.py index 3aeb9dab8..4f65b1f58 100644 --- a/python/copilot/tools.py +++ b/python/copilot/tools.py @@ -288,10 +288,7 @@ def _is_call_tool_result(value: Any) -> bool: content = value.get("content") if not isinstance(content, list): return False - return all( - isinstance(item, dict) and isinstance(item.get("type"), str) - for item in content - ) + return all(isinstance(item, dict) and isinstance(item.get("type"), str) for item in content) def _convert_call_tool_result(call_result: dict[str, Any]) -> ToolResult: @@ -304,22 +301,26 @@ def _convert_call_tool_result(call_result: dict[str, Any]) -> ToolResult: if block_type == "text": text_parts.append(block.get("text", "")) elif block_type == "image": - binary_results.append(ToolBinaryResult( - data=block.get("data", ""), - mime_type=block.get("mimeType", ""), - type="image", - )) + binary_results.append( + ToolBinaryResult( + data=block.get("data", ""), + mime_type=block.get("mimeType", ""), + type="image", + ) + ) elif block_type == "resource": resource = block.get("resource", {}) if resource.get("text"): text_parts.append(resource["text"]) if resource.get("blob"): - binary_results.append(ToolBinaryResult( - data=resource["blob"], - mime_type=resource.get("mimeType", "application/octet-stream"), - type="resource", - description=resource.get("uri", ""), - )) + binary_results.append( + ToolBinaryResult( + data=resource["blob"], + mime_type=resource.get("mimeType", "application/octet-stream"), + type="resource", + description=resource.get("uri", ""), + ) + ) return ToolResult( text_result_for_llm="\n".join(text_parts), diff --git a/python/e2e/test_tools_unit.py b/python/e2e/test_tools_unit.py index 637d1f8eb..ca43a5f51 100644 --- a/python/e2e/test_tools_unit.py +++ b/python/e2e/test_tools_unit.py @@ -6,7 +6,13 @@ from pydantic import BaseModel, Field from copilot import define_tool -from copilot.tools import ToolInvocation, ToolResult, _normalize_result, _is_call_tool_result, _convert_call_tool_result +from copilot.tools import ( + ToolInvocation, + ToolResult, + _normalize_result, + _is_call_tool_result, + _convert_call_tool_result, +) class TestDefineTool: @@ -288,39 +294,49 @@ def test_raises_for_unserializable_value(self): class TestCallToolResult: def test_text_only_call_tool_result(self): - result = _normalize_result({ - "content": [{"type": "text", "text": "hello"}], - }) + result = _normalize_result( + { + "content": [{"type": "text", "text": "hello"}], + } + ) assert result.text_result_for_llm == "hello" assert result.result_type == "success" def test_multiple_text_blocks(self): - result = _normalize_result({ - "content": [ - {"type": "text", "text": "line 1"}, - {"type": "text", "text": "line 2"}, - ], - }) + result = _normalize_result( + { + "content": [ + {"type": "text", "text": "line 1"}, + {"type": "text", "text": "line 2"}, + ], + } + ) assert result.text_result_for_llm == "line 1\nline 2" def test_is_error_maps_to_failure(self): - result = _normalize_result({ - "content": [{"type": "text", "text": "oops"}], - "isError": True, - }) + result = _normalize_result( + { + "content": [{"type": "text", "text": "oops"}], + "isError": True, + } + ) assert result.result_type == "failure" def test_is_error_false_maps_to_success(self): - result = _normalize_result({ - "content": [{"type": "text", "text": "ok"}], - "isError": False, - }) + result = _normalize_result( + { + "content": [{"type": "text", "text": "ok"}], + "isError": False, + } + ) assert result.result_type == "success" def test_image_content_to_binary(self): - result = _normalize_result({ - "content": [{"type": "image", "data": "base64data", "mimeType": "image/png"}], - }) + result = _normalize_result( + { + "content": [{"type": "image", "data": "base64data", "mimeType": "image/png"}], + } + ) assert result.binary_results_for_llm is not None assert len(result.binary_results_for_llm) == 1 assert result.binary_results_for_llm[0].data == "base64data" @@ -328,22 +344,33 @@ def test_image_content_to_binary(self): assert result.binary_results_for_llm[0].type == "image" def test_resource_text_to_text_result(self): - result = _normalize_result({ - "content": [ - {"type": "resource", "resource": {"uri": "file:///data.txt", "text": "file contents"}}, - ], - }) + result = _normalize_result( + { + "content": [ + { + "type": "resource", + "resource": {"uri": "file:///data.txt", "text": "file contents"}, + }, + ], + } + ) assert result.text_result_for_llm == "file contents" def test_resource_blob_to_binary(self): - result = _normalize_result({ - "content": [ - { - "type": "resource", - "resource": {"uri": "file:///img.png", "blob": "blobdata", "mimeType": "image/png"}, - }, - ], - }) + result = _normalize_result( + { + "content": [ + { + "type": "resource", + "resource": { + "uri": "file:///img.png", + "blob": "blobdata", + "mimeType": "image/png", + }, + }, + ], + } + ) assert result.binary_results_for_llm is not None assert len(result.binary_results_for_llm) == 1 assert result.binary_results_for_llm[0].data == "blobdata" From 4f18d31b70f5c23aa4a23701ca4cfd85c7eeae25 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 8 Apr 2026 20:55:16 -0400 Subject: [PATCH 3/9] Remove unused _convert_call_tool_result import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/e2e/test_tools_unit.py | 1 - 1 file changed, 1 deletion(-) diff --git a/python/e2e/test_tools_unit.py b/python/e2e/test_tools_unit.py index ca43a5f51..3f6ec07bb 100644 --- a/python/e2e/test_tools_unit.py +++ b/python/e2e/test_tools_unit.py @@ -11,7 +11,6 @@ ToolResult, _normalize_result, _is_call_tool_result, - _convert_call_tool_result, ) From 2101205c76defc33253586f804be33ec0e10fde1 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 8 Apr 2026 20:59:41 -0400 Subject: [PATCH 4/9] Address review feedback: add type guards in Python, fix Go comment typo Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- go/definetool.go | 2 +- python/copilot/tools.py | 41 +++++++++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/go/definetool.go b/go/definetool.go index d661e72fe..70a5f3a75 100644 --- a/go/definetool.go +++ b/go/definetool.go @@ -200,7 +200,7 @@ func convertCallToolResult(value any) (ToolResult, bool) { return tr, true } -// generateSchemaForTypegenerates a JSON schema map from a Go type using reflection. +// generateSchemaForType generates a JSON schema map from a Go type using reflection. // Panics if schema generation fails, as this indicates a programming error. func generateSchemaForType(t reflect.Type) map[string]any { if t == nil { diff --git a/python/copilot/tools.py b/python/copilot/tools.py index 4f65b1f58..c8ca8e438 100644 --- a/python/copilot/tools.py +++ b/python/copilot/tools.py @@ -299,31 +299,44 @@ def _convert_call_tool_result(call_result: dict[str, Any]) -> ToolResult: for block in call_result["content"]: block_type = block.get("type") if block_type == "text": - text_parts.append(block.get("text", "")) + text = block.get("text", "") + if isinstance(text, str): + text_parts.append(text) elif block_type == "image": - binary_results.append( - ToolBinaryResult( - data=block.get("data", ""), - mime_type=block.get("mimeType", ""), - type="image", + data = block.get("data", "") + mime_type = block.get("mimeType", "") + if isinstance(data, str) and isinstance(mime_type, str): + binary_results.append( + ToolBinaryResult( + data=data, + mime_type=mime_type, + type="image", + ) ) - ) elif block_type == "resource": resource = block.get("resource", {}) - if resource.get("text"): - text_parts.append(resource["text"]) - if resource.get("blob"): + if not isinstance(resource, dict): + continue + text = resource.get("text") + if isinstance(text, str) and text: + text_parts.append(text) + blob = resource.get("blob") + if isinstance(blob, str) and blob: + mime_type = resource.get("mimeType", "application/octet-stream") + uri = resource.get("uri", "") binary_results.append( ToolBinaryResult( - data=resource["blob"], - mime_type=resource.get("mimeType", "application/octet-stream"), + data=blob, + mime_type=mime_type + if isinstance(mime_type, str) + else "application/octet-stream", type="resource", - description=resource.get("uri", ""), + description=uri if isinstance(uri, str) else "", ) ) return ToolResult( text_result_for_llm="\n".join(text_parts), - result_type="failure" if call_result.get("isError") else "success", + result_type="failure" if call_result.get("isError") is True else "success", binary_results_for_llm=binary_results if binary_results else None, ) From e6729bcaa01b22639eef3325bdb75d842b609836 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 8 Apr 2026 21:17:06 -0400 Subject: [PATCH 5/9] Fix Python import sorting in test_tools_unit.py Sort imports in copilot.tools import block to satisfy ruff I001 rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/e2e/test_tools_unit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/e2e/test_tools_unit.py b/python/e2e/test_tools_unit.py index 3f6ec07bb..6da639962 100644 --- a/python/e2e/test_tools_unit.py +++ b/python/e2e/test_tools_unit.py @@ -9,8 +9,8 @@ from copilot.tools import ( ToolInvocation, ToolResult, - _normalize_result, _is_call_tool_result, + _normalize_result, ) From 80bd4263518943382ca0baff9a92d78a3af9f92b Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 8 Apr 2026 22:35:02 -0400 Subject: [PATCH 6/9] Remove isCallToolResult and convertCallToolResult from public exports These are internal implementation details used by session.ts and client.ts. Go and Python already keep them private (lowercase/underscore-prefixed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nodejs/src/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/nodejs/src/index.ts b/nodejs/src/index.ts index 9b331ec6d..4fcd43b87 100644 --- a/nodejs/src/index.ts +++ b/nodejs/src/index.ts @@ -13,8 +13,6 @@ export { CopilotSession, type AssistantMessageEvent } from "./session.js"; export { defineTool, approveAll, - isCallToolResult, - convertCallToolResult, SYSTEM_PROMPT_SECTIONS, } from "./types.js"; export type { From d644fc255a06ae0f0e10543353e078a5aac929b7 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 9 Apr 2026 10:35:43 +0100 Subject: [PATCH 7/9] TypeScript formatting Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com> --- nodejs/src/index.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/nodejs/src/index.ts b/nodejs/src/index.ts index 4fcd43b87..5a3209897 100644 --- a/nodejs/src/index.ts +++ b/nodejs/src/index.ts @@ -10,11 +10,7 @@ export { CopilotClient } from "./client.js"; export { CopilotSession, type AssistantMessageEvent } from "./session.js"; -export { - defineTool, - approveAll, - SYSTEM_PROMPT_SECTIONS, -} from "./types.js"; +export { defineTool, approveAll, SYSTEM_PROMPT_SECTIONS } from "./types.js"; export type { CommandContext, CommandDefinition, From f125b7d549b1b73de884b9c200a3edf26c705c3e Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 9 Apr 2026 13:52:31 -0400 Subject: [PATCH 8/9] Address review feedback: explicit MCP conversion, shared .NET helper, consistent guards - Remove implicit duck-typing of MCP CallToolResult from all SDKs - Add explicit public conversion: convertMcpCallToolResult (TS), ConvertMCPCallToolResult (Go), convert_mcp_call_tool_result (Python) - Extract shared ConvertFromInvocationResult helper in .NET - Remove isCallToolResult type guard (TS) and _is_call_tool_result (Python) - Rename types/functions to include 'Mcp' prefix across all languages - Make McpCallToolResult type non-exported in TS (structural typing) - Skip image blocks with empty data consistently across TS/Go/Python - Update all tests to use explicit conversion functions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/src/Client.cs | 24 +----- dotnet/src/Session.cs | 24 +----- dotnet/src/Types.cs | 106 +++---------------------- go/definetool.go | 21 +++-- go/definetool_test.go | 62 +++++++-------- nodejs/src/client.ts | 5 -- nodejs/src/index.ts | 8 +- nodejs/src/session.ts | 3 - nodejs/src/types.ts | 60 +++++--------- nodejs/test/call-tool-result.test.ts | 114 +++++++-------------------- python/copilot/__init__.py | 3 +- python/copilot/tools.py | 19 +---- python/e2e/test_tools_unit.py | 41 ++++------ 13 files changed, 123 insertions(+), 367 deletions(-) diff --git a/dotnet/src/Client.cs b/dotnet/src/Client.cs index 539cd57af..cbceeede2 100644 --- a/dotnet/src/Client.cs +++ b/dotnet/src/Client.cs @@ -1549,29 +1549,7 @@ public async Task OnToolCallV2(string sessionId, var result = await tool.InvokeAsync(aiFunctionArgs); - ToolResultObject toolResultObject; - if (result is ToolResultAIContent trac) - { - toolResultObject = trac.Result; - } - else if (ToolResultObject.TryConvertFromAIContent(result) is { } aiConverted) - { - toolResultObject = aiConverted; - } - else if (ToolResultObject.TryConvertFromCallToolResult(result) is { } converted) - { - toolResultObject = converted; - } - else - { - toolResultObject = new ToolResultObject - { - ResultType = "success", - TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } je - ? je.GetString()! - : JsonSerializer.Serialize(result, tool.JsonSerializerOptions.GetTypeInfo(typeof(object))), - }; - } + var toolResultObject = ToolResultObject.ConvertFromInvocationResult(result, tool.JsonSerializerOptions); return new ToolCallResponseV2(toolResultObject); } catch (Exception ex) diff --git a/dotnet/src/Session.cs b/dotnet/src/Session.cs index a39c560b0..189cdfaff 100644 --- a/dotnet/src/Session.cs +++ b/dotnet/src/Session.cs @@ -568,29 +568,7 @@ private async Task ExecuteToolAndRespondAsync(string requestId, string toolName, var result = await tool.InvokeAsync(aiFunctionArgs); - ToolResultObject toolResultObject; - if (result is ToolResultAIContent trac) - { - toolResultObject = trac.Result; - } - else if (ToolResultObject.TryConvertFromAIContent(result) is { } aiConverted) - { - toolResultObject = aiConverted; - } - else if (ToolResultObject.TryConvertFromCallToolResult(result) is { } converted) - { - toolResultObject = converted; - } - else - { - toolResultObject = new ToolResultObject - { - ResultType = "success", - TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } strJe - ? strJe.GetString()! - : JsonSerializer.Serialize(result, tool.JsonSerializerOptions.GetTypeInfo(typeof(object))), - }; - } + var toolResultObject = ToolResultObject.ConvertFromInvocationResult(result, tool.JsonSerializerOptions); await Rpc.Tools.HandlePendingToolCallAsync(requestId, toolResultObject, error: null); } diff --git a/dotnet/src/Types.cs b/dotnet/src/Types.cs index 9ac0ddaf6..1645b1eb0 100644 --- a/dotnet/src/Types.cs +++ b/dotnet/src/Types.cs @@ -326,110 +326,28 @@ public class ToolResultObject public Dictionary? ToolTelemetry { get; set; } /// - /// Attempts to interpret the result as an MCP CallToolResult - /// (shape: { content: [...], isError?: bool }) and converts it to a - /// . Returns if the value does - /// not match the expected shape. + /// Converts the result of an invocation into a + /// . Handles , + /// , and falls back to JSON serialization. /// - internal static ToolResultObject? TryConvertFromCallToolResult(object? result) + internal static ToolResultObject ConvertFromInvocationResult(object? result, JsonSerializerOptions jsonOptions) { - if (result is not JsonElement element) + if (result is ToolResultAIContent trac) { - return null; + return trac.Result; } - if (element.ValueKind != JsonValueKind.Object) + if (TryConvertFromAIContent(result) is { } aiConverted) { - return null; + return aiConverted; } - if (!element.TryGetProperty("content", out var contentProp) || contentProp.ValueKind != JsonValueKind.Array) - { - return null; - } - - // Validate every element has a string "type" field - foreach (var item in contentProp.EnumerateArray()) - { - if (item.ValueKind != JsonValueKind.Object || - !item.TryGetProperty("type", out var typeProp) || - typeProp.ValueKind != JsonValueKind.String) - { - return null; - } - } - - List? textParts = null; - List? binaryResults = null; - - foreach (var block in contentProp.EnumerateArray()) - { - var blockType = block.GetProperty("type").GetString(); - - switch (blockType) - { - case "text": - if (block.TryGetProperty("text", out var textProp) && textProp.ValueKind == JsonValueKind.String) - { - (textParts ??= []).Add(textProp.GetString()!); - } - break; - - case "image": - (binaryResults ??= []).Add(new ToolBinaryResult - { - Data = block.TryGetProperty("data", out var imgData) && imgData.ValueKind == JsonValueKind.String ? imgData.GetString() ?? "" : "", - MimeType = block.TryGetProperty("mimeType", out var imgMime) && imgMime.ValueKind == JsonValueKind.String ? imgMime.GetString() ?? "" : "", - Type = "image", - }); - break; - - case "resource": - if (block.TryGetProperty("resource", out var resProp) && resProp.ValueKind == JsonValueKind.Object) - { - if (resProp.TryGetProperty("text", out var resText) && resText.ValueKind == JsonValueKind.String) - { - var text = resText.GetString(); - if (!string.IsNullOrEmpty(text)) - { - (textParts ??= []).Add(text!); - } - } - - if (resProp.TryGetProperty("blob", out var resBlob) && resBlob.ValueKind == JsonValueKind.String) - { - var blob = resBlob.GetString(); - if (!string.IsNullOrEmpty(blob)) - { - var mimeType = resProp.TryGetProperty("mimeType", out var resMime) && resMime.ValueKind == JsonValueKind.String - ? resMime.GetString() ?? "application/octet-stream" - : "application/octet-stream"; - var uri = resProp.TryGetProperty("uri", out var resUri) && resUri.ValueKind == JsonValueKind.String - ? resUri.GetString() - : null; - - (binaryResults ??= []).Add(new ToolBinaryResult - { - Data = blob!, - MimeType = mimeType, - Type = "resource", - Description = uri, - }); - } - } - } - break; - } - } - - var isError = element.TryGetProperty("isError", out var isErrorProp) && - isErrorProp.ValueKind == JsonValueKind.True; - return new ToolResultObject { - TextResultForLlm = textParts is not null ? string.Join("\n", textParts) : "", - ResultType = isError ? "failure" : "success", - BinaryResultsForLlm = binaryResults, + ResultType = "success", + TextResultForLlm = result is JsonElement { ValueKind: JsonValueKind.String } je + ? je.GetString()! + : JsonSerializer.Serialize(result, jsonOptions.GetTypeInfo(typeof(object))), }; } diff --git a/go/definetool.go b/go/definetool.go index 70a5f3a75..ccaa69a58 100644 --- a/go/definetool.go +++ b/go/definetool.go @@ -66,8 +66,8 @@ func createTypedHandler[T any, U any](handler func(T, ToolInvocation) (U, error) } // normalizeResult converts any value to a ToolResult. -// Strings pass through directly, ToolResult passes through, CallToolResult is -// converted, and other types are JSON-serialized. +// Strings pass through directly, ToolResult passes through, and other types +// are JSON-serialized. func normalizeResult(result any) (ToolResult, error) { if result == nil { return ToolResult{ @@ -89,11 +89,6 @@ func normalizeResult(result any) (ToolResult, error) { }, nil } - // MCP CallToolResult shape: { content: [...], isError?: bool } - if tr, ok := convertCallToolResult(result); ok { - return tr, nil - } - // Everything else gets JSON-serialized jsonBytes, err := json.Marshal(result) if err != nil { @@ -106,10 +101,11 @@ func normalizeResult(result any) (ToolResult, error) { }, nil } -// convertCallToolResult attempts to interpret value as an MCP CallToolResult -// (map with "content" array and optional "isError" bool). Returns the converted -// ToolResult and true if it matched, or a zero ToolResult and false otherwise. -func convertCallToolResult(value any) (ToolResult, bool) { +// ConvertMCPCallToolResult converts an MCP CallToolResult value (a map or struct +// with a "content" array and optional "isError" bool) into a ToolResult. +// Returns the converted ToolResult and true if the value matched the expected +// shape, or a zero ToolResult and false otherwise. +func ConvertMCPCallToolResult(value any) (ToolResult, bool) { m, ok := value.(map[string]any) if !ok { jsonBytes, err := json.Marshal(value) @@ -158,6 +154,9 @@ func convertCallToolResult(value any) (ToolResult, bool) { case "image": data, _ := block["data"].(string) mimeType, _ := block["mimeType"].(string) + if data == "" { + continue + } binaryResults = append(binaryResults, ToolBinaryResult{ Data: data, MimeType: mimeType, diff --git a/go/definetool_test.go b/go/definetool_test.go index 9181cebbd..cc9fecb2c 100644 --- a/go/definetool_test.go +++ b/go/definetool_test.go @@ -253,7 +253,7 @@ func TestNormalizeResult(t *testing.T) { }) } -func TestConvertCallToolResult(t *testing.T) { +func TestConvertMCPCallToolResult(t *testing.T) { t.Run("typed CallToolResult struct is converted", func(t *testing.T) { type Resource struct { URI string `json:"uri"` @@ -276,9 +276,9 @@ func TestConvertCallToolResult(t *testing.T) { }, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if result.TextResultForLLM != "details" { t.Errorf("Expected 'details', got %q", result.TextResultForLLM) @@ -295,9 +295,9 @@ func TestConvertCallToolResult(t *testing.T) { }, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if result.TextResultForLLM != "hello" { t.Errorf("Expected 'hello', got %q", result.TextResultForLLM) @@ -315,9 +315,9 @@ func TestConvertCallToolResult(t *testing.T) { }, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if result.TextResultForLLM != "line 1\nline 2" { t.Errorf("Expected 'line 1\\nline 2', got %q", result.TextResultForLLM) @@ -332,9 +332,9 @@ func TestConvertCallToolResult(t *testing.T) { "isError": true, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if result.ResultType != "failure" { t.Errorf("Expected 'failure', got %q", result.ResultType) @@ -348,9 +348,9 @@ func TestConvertCallToolResult(t *testing.T) { }, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if len(result.BinaryResultsForLLM) != 1 { t.Fatalf("Expected 1 binary result, got %d", len(result.BinaryResultsForLLM)) @@ -373,9 +373,9 @@ func TestConvertCallToolResult(t *testing.T) { }, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if result.TextResultForLLM != "file contents" { t.Errorf("Expected 'file contents', got %q", result.TextResultForLLM) @@ -392,9 +392,9 @@ func TestConvertCallToolResult(t *testing.T) { }, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if len(result.BinaryResultsForLLM) != 1 { t.Fatalf("Expected 1 binary result, got %d", len(result.BinaryResultsForLLM)) @@ -404,18 +404,14 @@ func TestConvertCallToolResult(t *testing.T) { } }) - t.Run("non-CallToolResult map is JSON serialized", func(t *testing.T) { + t.Run("non-CallToolResult map returns false", func(t *testing.T) { input := map[string]any{ "key": "value", } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - expected := `{"key":"value"}` - if result.TextResultForLLM != expected { - t.Errorf("Expected %q, got %q", expected, result.TextResultForLLM) + _, ok := ConvertMCPCallToolResult(input) + if ok { + t.Error("Expected ConvertMCPCallToolResult to return false for non-CallToolResult map") } }) @@ -424,9 +420,9 @@ func TestConvertCallToolResult(t *testing.T) { "content": []any{}, } - result, err := normalizeResult(input) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + result, ok := ConvertMCPCallToolResult(input) + if !ok { + t.Fatal("Expected ConvertMCPCallToolResult to succeed") } if result.TextResultForLLM != "" { t.Errorf("Expected empty text, got %q", result.TextResultForLLM) diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index b5eb1eb64..6941598b8 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -55,7 +55,6 @@ import type { TraceContextProvider, TypedSessionLifecycleHandler, } from "./types.js"; -import { isCallToolResult, convertCallToolResult } from "./types.js"; /** * Minimum protocol version this SDK can communicate with. @@ -1930,10 +1929,6 @@ export class CopilotClient { return result; } - if (isCallToolResult(result)) { - return convertCallToolResult(result); - } - const textResult = typeof result === "string" ? result : JSON.stringify(result); return { textResultForLlm: textResult, diff --git a/nodejs/src/index.ts b/nodejs/src/index.ts index 5a3209897..13e0670fb 100644 --- a/nodejs/src/index.ts +++ b/nodejs/src/index.ts @@ -10,7 +10,12 @@ export { CopilotClient } from "./client.js"; export { CopilotSession, type AssistantMessageEvent } from "./session.js"; -export { defineTool, approveAll, SYSTEM_PROMPT_SECTIONS } from "./types.js"; +export { + defineTool, + approveAll, + convertMcpCallToolResult, + SYSTEM_PROMPT_SECTIONS, +} from "./types.js"; export type { CommandContext, CommandDefinition, @@ -73,7 +78,6 @@ export type { ToolHandler, ToolInvocation, ToolResultObject, - CallToolResult, TypedSessionEventHandler, TypedSessionLifecycleHandler, ZodSchema, diff --git a/nodejs/src/session.ts b/nodejs/src/session.ts index fc08aa031..ffb2c045a 100644 --- a/nodejs/src/session.ts +++ b/nodejs/src/session.ts @@ -43,7 +43,6 @@ import type { UserInputRequest, UserInputResponse, } from "./types.js"; -import { isCallToolResult, convertCallToolResult } from "./types.js"; export const NO_RESULT_PERMISSION_V2_ERROR = "Permission handlers cannot return 'no-result' when connected to a protocol v2 server."; @@ -479,8 +478,6 @@ export class CopilotSession { result = rawResult; } else if (isToolResultObject(rawResult)) { result = rawResult; - } else if (isCallToolResult(rawResult)) { - result = convertCallToolResult(rawResult); } else { result = JSON.stringify(rawResult); } diff --git a/nodejs/src/types.ts b/nodejs/src/types.ts index 386b59de3..7e15f4d1b 100644 --- a/nodejs/src/types.ts +++ b/nodejs/src/types.ts @@ -214,18 +214,18 @@ export type ToolResult = string | ToolResultObject; /** * Content block types within an MCP CallToolResult. */ -type CallToolResultTextContent = { +type McpCallToolResultTextContent = { type: "text"; text: string; }; -type CallToolResultImageContent = { +type McpCallToolResultImageContent = { type: "image"; data: string; mimeType: string; }; -type CallToolResultResourceContent = { +type McpCallToolResultResourceContent = { type: "resource"; resource: { uri: string; @@ -235,48 +235,24 @@ type CallToolResultResourceContent = { }; }; -type CallToolResultContent = - | CallToolResultTextContent - | CallToolResultImageContent - | CallToolResultResourceContent; +type McpCallToolResultContent = + | McpCallToolResultTextContent + | McpCallToolResultImageContent + | McpCallToolResultResourceContent; /** - * MCP-compatible CallToolResult type. When a tool handler returns this shape, - * the SDK automatically converts it to a {@link ToolResultObject} before - * sending it over RPC. + * MCP-compatible CallToolResult type. Can be passed to + * {@link convertMcpCallToolResult} to produce a {@link ToolResultObject}. */ -export type CallToolResult = { - content: CallToolResultContent[]; +type McpCallToolResult = { + content: McpCallToolResultContent[]; isError?: boolean; }; -/** - * Type guard that checks whether a value is shaped like an MCP CallToolResult. - */ -export function isCallToolResult(value: unknown): value is CallToolResult { - if (typeof value !== "object" || value === null) { - return false; - } - - const obj = value as Record; - if (!Array.isArray(obj.content)) { - return false; - } - - // Verify every element in content has a valid "type" field - return obj.content.every( - (item: unknown) => - typeof item === "object" && - item !== null && - "type" in item && - typeof (item as Record).type === "string" - ); -} - /** * Converts an MCP CallToolResult into the SDK's ToolResultObject format. */ -export function convertCallToolResult(callResult: CallToolResult): ToolResultObject { +export function convertMcpCallToolResult(callResult: McpCallToolResult): ToolResultObject { const textParts: string[] = []; const binaryResults: ToolBinaryResult[] = []; @@ -289,11 +265,13 @@ export function convertCallToolResult(callResult: CallToolResult): ToolResultObj } break; case "image": - binaryResults.push({ - data: block.data, - mimeType: block.mimeType, - type: "image", - }); + if (typeof block.data === "string" && block.data && typeof block.mimeType === "string") { + binaryResults.push({ + data: block.data, + mimeType: block.mimeType, + type: "image", + }); + } break; case "resource": { // Use optional chaining: resource field may be absent in malformed input diff --git a/nodejs/test/call-tool-result.test.ts b/nodejs/test/call-tool-result.test.ts index 3cfe2bbcf..132e482bd 100644 --- a/nodejs/test/call-tool-result.test.ts +++ b/nodejs/test/call-tool-result.test.ts @@ -1,78 +1,18 @@ import { describe, expect, it } from "vitest"; -import { isCallToolResult, convertCallToolResult } from "../src/types.js"; -import type { CallToolResult } from "../src/types.js"; - -describe("isCallToolResult", () => { - it("returns true for a text-only CallToolResult", () => { - expect( - isCallToolResult({ - content: [{ type: "text", text: "hello" }], - }) - ).toBe(true); - }); - - it("returns true for CallToolResult with isError", () => { - expect( - isCallToolResult({ - content: [{ type: "text", text: "oops" }], - isError: true, - }) - ).toBe(true); - }); - - it("returns true for CallToolResult with image content", () => { - expect( - isCallToolResult({ - content: [{ type: "image", data: "abc123", mimeType: "image/png" }], - }) - ).toBe(true); - }); +import { convertMcpCallToolResult } from "../src/types.js"; - it("returns true for CallToolResult with resource content", () => { - expect( - isCallToolResult({ - content: [ - { type: "resource", resource: { uri: "file:///tmp/out.txt", text: "data" } }, - ], - }) - ).toBe(true); - }); - - it("returns true for empty content array", () => { - expect(isCallToolResult({ content: [] })).toBe(true); - }); - - it("returns false for null", () => { - expect(isCallToolResult(null)).toBe(false); - }); - - it("returns false for a string", () => { - expect(isCallToolResult("hello")).toBe(false); - }); - - it("returns false for a ToolResultObject", () => { - expect(isCallToolResult({ textResultForLlm: "hi", resultType: "success" })).toBe(false); - }); - - it("returns false when content is not an array", () => { - expect(isCallToolResult({ content: "text" })).toBe(false); - }); - - it("returns false when content items lack type field", () => { - expect(isCallToolResult({ content: [{ text: "no type" }] })).toBe(false); - }); -}); +type McpCallToolResult = Parameters[0]; -describe("convertCallToolResult", () => { +describe("convertMcpCallToolResult", () => { it("extracts text from text content blocks", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [ { type: "text", text: "line 1" }, { type: "text", text: "line 2" }, ], }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe("line 1\nline 2"); expect(result.resultType).toBe("success"); @@ -80,32 +20,32 @@ describe("convertCallToolResult", () => { }); it("maps isError to failure resultType", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [{ type: "text", text: "error occurred" }], isError: true, }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe("error occurred"); expect(result.resultType).toBe("failure"); }); it("maps isError: false to success", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [{ type: "text", text: "ok" }], isError: false, }; - expect(convertCallToolResult(input).resultType).toBe("success"); + expect(convertMcpCallToolResult(input).resultType).toBe("success"); }); it("converts image content to binaryResultsForLlm", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [{ type: "image", data: "base64data", mimeType: "image/png" }], }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe(""); expect(result.binaryResultsForLlm).toHaveLength(1); @@ -117,7 +57,7 @@ describe("convertCallToolResult", () => { }); it("converts resource with text to textResultForLlm", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [ { type: "resource", @@ -126,13 +66,13 @@ describe("convertCallToolResult", () => { ], }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe("file contents"); }); it("converts resource with blob to binaryResultsForLlm", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [ { type: "resource", @@ -145,7 +85,7 @@ describe("convertCallToolResult", () => { ], }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.binaryResultsForLlm).toHaveLength(1); expect(result.binaryResultsForLlm![0]).toEqual({ @@ -157,7 +97,7 @@ describe("convertCallToolResult", () => { }); it("handles mixed content types", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [ { type: "text", text: "Analysis complete" }, { type: "image", data: "chartdata", mimeType: "image/svg+xml" }, @@ -168,7 +108,7 @@ describe("convertCallToolResult", () => { ], }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe("Analysis complete\nReport details"); expect(result.binaryResultsForLlm).toHaveLength(1); @@ -176,7 +116,7 @@ describe("convertCallToolResult", () => { }); it("handles empty content array", () => { - const result = convertCallToolResult({ content: [] }); + const result = convertMcpCallToolResult({ content: [] }); expect(result.textResultForLlm).toBe(""); expect(result.resultType).toBe("success"); @@ -184,7 +124,7 @@ describe("convertCallToolResult", () => { }); it("defaults resource blob mimeType to application/octet-stream", () => { - const input: CallToolResult = { + const input: McpCallToolResult = { content: [ { type: "resource", @@ -193,17 +133,17 @@ describe("convertCallToolResult", () => { ], }; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.binaryResultsForLlm![0]!.mimeType).toBe("application/octet-stream"); }); it("handles text block with missing text field without corrupting output", () => { - // isCallToolResult only checks that type is a string, not that type-specific - // fields are present. convertCallToolResult must be defensive at runtime. - const input = { content: [{ type: "text" }] } as unknown as CallToolResult; + // The input type uses structural typing, so type-specific fields might be absent + // at runtime. convertMcpCallToolResult must be defensive. + const input = { content: [{ type: "text" }] } as unknown as McpCallToolResult; - const result = convertCallToolResult(input); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe(""); expect(result.textResultForLlm).not.toBe("undefined"); @@ -212,10 +152,10 @@ describe("convertCallToolResult", () => { it("handles resource block with missing resource field without crashing", () => { // A resource content item missing the resource field would crash with an // unguarded block.resource.text access. Optional chaining must be used. - const input = { content: [{ type: "resource" }] } as unknown as CallToolResult; + const input = { content: [{ type: "resource" }] } as unknown as McpCallToolResult; - expect(() => convertCallToolResult(input)).not.toThrow(); - const result = convertCallToolResult(input); + expect(() => convertMcpCallToolResult(input)).not.toThrow(); + const result = convertMcpCallToolResult(input); expect(result.textResultForLlm).toBe(""); }); }); diff --git a/python/copilot/__init__.py b/python/copilot/__init__.py index 702d35035..6333aea51 100644 --- a/python/copilot/__init__.py +++ b/python/copilot/__init__.py @@ -29,7 +29,7 @@ SessionUiApi, SessionUiCapabilities, ) -from .tools import define_tool +from .tools import convert_mcp_call_tool_result, define_tool __version__ = "0.1.0" @@ -55,5 +55,6 @@ "SessionUiApi", "SessionUiCapabilities", "SubprocessConfig", + "convert_mcp_call_tool_result", "define_tool", ] diff --git a/python/copilot/tools.py b/python/copilot/tools.py index c8ca8e438..c94c396e9 100644 --- a/python/copilot/tools.py +++ b/python/copilot/tools.py @@ -240,7 +240,6 @@ def _normalize_result(result: Any) -> ToolResult: - None returns empty success - Strings pass through directly - ToolResult passes through - - MCP CallToolResult dicts are converted automatically - Everything else gets JSON-serialized (with Pydantic support) """ if result is None: @@ -260,10 +259,6 @@ def _normalize_result(result: Any) -> ToolResult: result_type="success", ) - # MCP CallToolResult shape: { content: [...], isError?: bool } - if _is_call_tool_result(result): - return _convert_call_tool_result(result) - # Everything else gets JSON-serialized (with Pydantic model support) def default(obj: Any) -> Any: if isinstance(obj, BaseModel): @@ -281,17 +276,7 @@ def default(obj: Any) -> Any: ) -def _is_call_tool_result(value: Any) -> bool: - """Check whether a value is shaped like an MCP CallToolResult.""" - if not isinstance(value, dict): - return False - content = value.get("content") - if not isinstance(content, list): - return False - return all(isinstance(item, dict) and isinstance(item.get("type"), str) for item in content) - - -def _convert_call_tool_result(call_result: dict[str, Any]) -> ToolResult: +def convert_mcp_call_tool_result(call_result: dict[str, Any]) -> ToolResult: """Convert an MCP CallToolResult dict into a ToolResult.""" text_parts: list[str] = [] binary_results: list[ToolBinaryResult] = [] @@ -305,7 +290,7 @@ def _convert_call_tool_result(call_result: dict[str, Any]) -> ToolResult: elif block_type == "image": data = block.get("data", "") mime_type = block.get("mimeType", "") - if isinstance(data, str) and isinstance(mime_type, str): + if isinstance(data, str) and data and isinstance(mime_type, str): binary_results.append( ToolBinaryResult( data=data, diff --git a/python/e2e/test_tools_unit.py b/python/e2e/test_tools_unit.py index 6da639962..61c0b3950 100644 --- a/python/e2e/test_tools_unit.py +++ b/python/e2e/test_tools_unit.py @@ -9,7 +9,7 @@ from copilot.tools import ( ToolInvocation, ToolResult, - _is_call_tool_result, + convert_mcp_call_tool_result, _normalize_result, ) @@ -291,9 +291,9 @@ def test_raises_for_unserializable_value(self): _normalize_result(lambda x: x) -class TestCallToolResult: +class TestConvertMcpCallToolResult: def test_text_only_call_tool_result(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [{"type": "text", "text": "hello"}], } @@ -302,7 +302,7 @@ def test_text_only_call_tool_result(self): assert result.result_type == "success" def test_multiple_text_blocks(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [ {"type": "text", "text": "line 1"}, @@ -313,7 +313,7 @@ def test_multiple_text_blocks(self): assert result.text_result_for_llm == "line 1\nline 2" def test_is_error_maps_to_failure(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [{"type": "text", "text": "oops"}], "isError": True, @@ -322,7 +322,7 @@ def test_is_error_maps_to_failure(self): assert result.result_type == "failure" def test_is_error_false_maps_to_success(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [{"type": "text", "text": "ok"}], "isError": False, @@ -331,7 +331,7 @@ def test_is_error_false_maps_to_success(self): assert result.result_type == "success" def test_image_content_to_binary(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [{"type": "image", "data": "base64data", "mimeType": "image/png"}], } @@ -343,7 +343,7 @@ def test_image_content_to_binary(self): assert result.binary_results_for_llm[0].type == "image" def test_resource_text_to_text_result(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [ { @@ -356,7 +356,7 @@ def test_resource_text_to_text_result(self): assert result.text_result_for_llm == "file contents" def test_resource_blob_to_binary(self): - result = _normalize_result( + result = convert_mcp_call_tool_result( { "content": [ { @@ -376,25 +376,12 @@ def test_resource_blob_to_binary(self): assert result.binary_results_for_llm[0].description == "file:///img.png" def test_empty_content_array(self): - result = _normalize_result({"content": []}) + result = convert_mcp_call_tool_result({"content": []}) assert result.text_result_for_llm == "" assert result.result_type == "success" - def test_non_call_tool_result_dict_is_json_serialized(self): - result = _normalize_result({"key": "value"}) + def test_call_tool_result_dict_is_json_serialized_by_normalize(self): + """_normalize_result does NOT auto-detect MCP results; it JSON-serializes them.""" + result = _normalize_result({"content": [{"type": "text", "text": "hello"}]}) parsed = json.loads(result.text_result_for_llm) - assert parsed == {"key": "value"} - - def test_is_call_tool_result_false_for_non_dict(self): - assert _is_call_tool_result("hello") is False - assert _is_call_tool_result(None) is False - assert _is_call_tool_result(42) is False - - def test_is_call_tool_result_false_without_content(self): - assert _is_call_tool_result({"key": "value"}) is False - - def test_is_call_tool_result_false_when_content_not_list(self): - assert _is_call_tool_result({"content": "text"}) is False - - def test_is_call_tool_result_false_when_items_lack_type(self): - assert _is_call_tool_result({"content": [{"text": "no type"}]}) is False + assert parsed == {"content": [{"type": "text", "text": "hello"}]} From a7caf51059d0b23af021aedaf515c47550438ea5 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 9 Apr 2026 14:04:43 -0400 Subject: [PATCH 9/9] Fix CI: prettier formatting and ruff import sorting - Fix prettier line-length violation in nodejs/src/types.ts (long if condition) - Fix ruff I001 import sorting in python/e2e/test_tools_unit.py (_normalize_result before convert_mcp_call_tool_result) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nodejs/src/types.ts | 6 +++++- python/e2e/test_tools_unit.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/nodejs/src/types.ts b/nodejs/src/types.ts index 7e15f4d1b..c8a27009d 100644 --- a/nodejs/src/types.ts +++ b/nodejs/src/types.ts @@ -265,7 +265,11 @@ export function convertMcpCallToolResult(callResult: McpCallToolResult): ToolRes } break; case "image": - if (typeof block.data === "string" && block.data && typeof block.mimeType === "string") { + if ( + typeof block.data === "string" && + block.data && + typeof block.mimeType === "string" + ) { binaryResults.push({ data: block.data, mimeType: block.mimeType, diff --git a/python/e2e/test_tools_unit.py b/python/e2e/test_tools_unit.py index 61c0b3950..bbbe2190f 100644 --- a/python/e2e/test_tools_unit.py +++ b/python/e2e/test_tools_unit.py @@ -9,8 +9,8 @@ from copilot.tools import ( ToolInvocation, ToolResult, - convert_mcp_call_tool_result, _normalize_result, + convert_mcp_call_tool_result, )