From 42a8505c676792b25346345ca66e4a08ce3f13c6 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 22 Oct 2026 07:12:26 +0300 Subject: [PATCH 1/4] Preserve MCP tool results in CLI JSON output --- cmd/mcpproxy/call_cmd.go | 67 ++++++++++-- cmd/mcpproxy/call_result_json_test.go | 102 +++++++++++++++++++ docs/cli-output-formatting.md | 13 +++ internal/cliclient/call_tool_test.go | 38 +++++++ internal/cliclient/client.go | 40 ++++++-- internal/httpapi/server.go | 3 + internal/httpapi/tool_call_envelope_test.go | 48 +++++++++ internal/reqcontext/toolresult.go | 14 +++ internal/server/mcp.go | 18 +++- internal/server/mcp_call_tool_direct_test.go | 43 ++++++++ internal/server/server.go | 8 +- 11 files changed, 373 insertions(+), 21 deletions(-) create mode 100644 cmd/mcpproxy/call_result_json_test.go create mode 100644 internal/cliclient/call_tool_test.go create mode 100644 internal/httpapi/tool_call_envelope_test.go create mode 100644 internal/reqcontext/toolresult.go diff --git a/cmd/mcpproxy/call_cmd.go b/cmd/mcpproxy/call_cmd.go index 70ba9b93e..338893cb2 100644 --- a/cmd/mcpproxy/call_cmd.go +++ b/cmd/mcpproxy/call_cmd.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "os" "strings" @@ -317,22 +318,33 @@ func createLogger(level string) (*zap.Logger, error) { } // runCallToolRead handles the tool-read command (Spec 018) -func runCallToolRead(_ *cobra.Command, _ []string) error { - return runCallToolVariant("call_tool_read", "read") +func runCallToolRead(cmd *cobra.Command, _ []string) error { + return runCallToolVariant(cmd, "call_tool_read", "read") } // runCallToolWrite handles the tool-write command (Spec 018) -func runCallToolWrite(_ *cobra.Command, _ []string) error { - return runCallToolVariant("call_tool_write", "write") +func runCallToolWrite(cmd *cobra.Command, _ []string) error { + return runCallToolVariant(cmd, "call_tool_write", "write") } // runCallToolDestructive handles the tool-destructive command (Spec 018) -func runCallToolDestructive(_ *cobra.Command, _ []string) error { - return runCallToolVariant("call_tool_destructive", "destructive") +func runCallToolDestructive(cmd *cobra.Command, _ []string) error { + return runCallToolVariant(cmd, "call_tool_destructive", "destructive") } // runCallToolVariant is the common implementation for intent-based tool calls -func runCallToolVariant(toolVariant, operationType string) error { +func runCallToolVariant(cmd *cobra.Command, toolVariant, operationType string) (callErr error) { + // Cobra validates required flags before RunE. Runtime failures in JSON mode + // get one machine-readable document on stdout and retain a nonzero exit. + if callOutputFormat == outputFormatJSON { + cmd.SilenceUsage = true + defer func() { + var resultErr callResultError + if callErr != nil && !errors.As(callErr, &resultErr) { + writeCallJSONError(callErr) + } + }() + } ctx, cancel := context.WithTimeout(context.Background(), callTimeout) defer cancel() @@ -391,6 +403,29 @@ func runCallToolVariant(toolVariant, operationType string) error { return runCallToolVariantStandalone(ctx, toolVariant, variantArgs, globalConfig) } +// callResultError retains the nonzero process status after a complete MCP +// isError result has already been written to stdout. +type callResultError struct{} + +func (callResultError) Error() string { return "tool returned an error result" } + +type callDaemonError struct{ cause error } + +func (e callDaemonError) Error() string { + return "failed to call tool via daemon: " + formatErrorWithRequestID(e.cause) +} + +func (e callDaemonError) Unwrap() error { return e.cause } + +func writeCallJSONError(err error) { + structured := output.NewStructuredError(output.ErrCodeOperationFailed, err.Error()) + var apiErr *cliclient.APIError + if errors.As(err, &apiErr) && apiErr.RequestID != "" { + structured = structured.WithRequestID(apiErr.RequestID) + } + _ = outputCallResultAsJSON(structured) +} + // runCallToolVariantClientMode calls tool variant via the daemon HTTP API func runCallToolVariantClientMode(client *cliclient.Client, toolVariant string, args map[string]interface{}, logger *zap.Logger) error { // Ping daemon to verify connectivity @@ -417,13 +452,19 @@ func runCallToolVariantClientMode(client *cliclient.Client, toolVariant string, result, err := client.CallTool(callCtx, toolVariant, args) if err != nil { // T028: Use cliError to include request_id in error output - return cliError("failed to call tool via daemon", err) + return callDaemonError{cause: err} } // Output results based on format switch callOutputFormat { case outputFormatJSON: - return outputCallResultAsJSON(result) + if err := outputCallResultAsJSON(result); err != nil { + return err + } + if result.IsError { + return callResultError{} + } + return nil case outputFormatPretty, "": fallthrough default: @@ -499,7 +540,13 @@ func runCallToolVariantStandalone(ctx context.Context, toolVariant string, args // Output results based on format switch callOutputFormat { case outputFormatJSON: - return outputCallResultAsJSON(result) + if err := outputCallResultAsJSON(result); err != nil { + return err + } + if result.IsError { + return callResultError{} + } + return nil case outputFormatPretty, "": fallthrough default: diff --git a/cmd/mcpproxy/call_result_json_test.go b/cmd/mcpproxy/call_result_json_test.go new file mode 100644 index 000000000..0deae7ec9 --- /dev/null +++ b/cmd/mcpproxy/call_result_json_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +func TestCallToolJSONFailureIsParseableAndNonzero(t *testing.T) { + oldFormat, oldArgs := callOutputFormat, callJSONArgs + t.Cleanup(func() { callOutputFormat, callJSONArgs = oldFormat, oldArgs }) + callOutputFormat, callJSONArgs = "json", "{invalid" + reader, writer, err := os.Pipe() + require.NoError(t, err) + oldStdout := os.Stdout + os.Stdout = writer + t.Cleanup(func() { os.Stdout = oldStdout }) + + cmd := &cobra.Command{} + callErr := runCallToolVariant(cmd, "call_tool_read", "read") + require.Error(t, callErr) + require.True(t, cmd.SilenceUsage) + require.NoError(t, writer.Close()) + bytes, err := io.ReadAll(reader) + require.NoError(t, err) + var got output.StructuredError + require.NoError(t, json.Unmarshal(bytes, &got)) + require.Equal(t, output.ErrCodeOperationFailed, got.Code) + require.Contains(t, got.Message, "invalid JSON arguments") +} + +func TestCallToolDaemonJSONRetainsResultAndErrorStatus(t *testing.T) { + oldFormat := callOutputFormat + callOutputFormat = "json" + t.Cleanup(func() { callOutputFormat = oldFormat }) + for _, tc := range []struct { + name, data string + wantError bool + }{ + {"success", `{"content":[{"type":"text","text":"{\"ok\":true}"},{"type":"image","data":"abc","mimeType":"image/png"}],"structuredContent":{"ok":true},"_meta":{"trace":"test"}}`, false}, + {"tool error", `{"content":[{"type":"text","text":"denied"}],"isError":true}`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/status" { + _, _ = w.Write([]byte(`{"status":"ok"}`)) + return + } + require.Equal(t, "/api/v1/tools/call", r.URL.Path) + _, _ = w.Write([]byte(`{"success":true,"data":` + tc.data + `}`)) + })) + defer srv.Close() + reader, writer, err := os.Pipe() + require.NoError(t, err) + oldStdout := os.Stdout + os.Stdout = writer + defer func() { os.Stdout = oldStdout }() + err = runCallToolVariantClientMode(cliclient.NewClient(srv.URL, nil), "call_tool_read", map[string]interface{}{"name": "fixture:read"}, zap.NewNop()) + if tc.wantError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.NoError(t, writer.Close()) + actual, err := io.ReadAll(reader) + require.NoError(t, err) + var got map[string]interface{} + require.NoError(t, json.Unmarshal(actual, &got)) + var want map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(tc.data), &want)) + require.Equal(t, want["content"], got["content"]) + require.Equal(t, want["structuredContent"], got["structuredContent"]) + require.Equal(t, want["_meta"], got["_meta"]) + require.Equal(t, tc.wantError, got["isError"]) + }) + } +} + +func TestCallToolJSONErrorRetainsRequestID(t *testing.T) { + reader, writer, err := os.Pipe() + require.NoError(t, err) + oldStdout := os.Stdout + os.Stdout = writer + defer func() { os.Stdout = oldStdout }() + writeCallJSONError(callDaemonError{cause: &cliclient.APIError{Message: "missing server", RequestID: "req-123"}}) + require.NoError(t, writer.Close()) + bytes, err := io.ReadAll(reader) + require.NoError(t, err) + var got output.StructuredError + require.NoError(t, json.Unmarshal(bytes, &got)) + require.Equal(t, "req-123", got.RequestID) + require.Equal(t, output.ErrCodeOperationFailed, got.Code) +} diff --git a/docs/cli-output-formatting.md b/docs/cli-output-formatting.md index 570b058b2..15f7f8ae2 100644 --- a/docs/cli-output-formatting.md +++ b/docs/cli-output-formatting.md @@ -144,6 +144,19 @@ This enables AI agents to discover available commands and their options programm ## Structured Error Output +For `mcpproxy call tool-read`, `tool-write`, and `tool-destructive -o json`, +stdout contains the MCP result envelope. The CLI opts into this REST response +shape while existing REST callers retain their content-array response. `content` retains its ordered text, +image, audio, or resource blocks; `structuredContent` and `_meta` are included +when supplied by the daemon. Older daemons that return a content array are +normalized to `{ "content": [...], "isError": false }`. An `isError` result +keeps that envelope on stdout and exits nonzero. + +Runtime tool-call failures emit one structured error object on stdout with +`code: "OPERATION_FAILED"`, a `message`, and `request_id` when available, and +exit nonzero. Diagnostics may also appear on stderr. Required-flag validation +still follows Cobra's usual usage output. + When using JSON output format, errors include structured information: ```json diff --git a/internal/cliclient/call_tool_test.go b/internal/cliclient/call_tool_test.go new file mode 100644 index 000000000..08fd1e795 --- /dev/null +++ b/internal/cliclient/call_tool_test.go @@ -0,0 +1,38 @@ +package cliclient_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" + "github.com/stretchr/testify/require" +) + +func TestCallToolPreservesResultShapes(t *testing.T) { + for _, tc := range []struct { + name string + data string + want string + }{ + {"legacy content array", `[{"type":"text","text":"{\"ok\":true}"},{"type":"image","data":"abc","mimeType":"image/png"}]`, `{"content":[{"type":"text","text":"{\"ok\":true}"},{"type":"image","data":"abc","mimeType":"image/png"}],"isError":false}`}, + {"full result", `{"content":[{"type":"text","text":"hello"}],"structuredContent":{"count":2},"isError":true,"_meta":{"source":"test"}}`, `{"content":[{"type":"text","text":"hello"}],"structuredContent":{"count":2},"isError":true,"_meta":{"source":"test"}}`}, + {"structured only", `{"structuredContent":{"count":2}}`, `{"content":[],"structuredContent":{"count":2},"isError":false}`}, + } { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/tools/call", r.URL.Path) + require.Equal(t, "1", r.Header.Get("X-MCPProxy-Result-Envelope")) + _, _ = w.Write([]byte(`{"success":true,"data":` + tc.data + `}`)) + })) + defer srv.Close() + got, err := cliclient.NewClient(srv.URL, nil).CallTool(context.Background(), "call_tool_read", nil) + require.NoError(t, err) + actual, err := json.Marshal(got) + require.NoError(t, err) + require.JSONEq(t, tc.want, string(actual)) + }) + } +} diff --git a/internal/cliclient/client.go b/internal/cliclient/client.go index 3f4b3f24e..4479941ea 100644 --- a/internal/cliclient/client.go +++ b/internal/cliclient/client.go @@ -359,9 +359,10 @@ func (c *Client) GetCodeScripts(ctx context.Context) (dir string, entries []code // CallToolResult represents tool call result. type CallToolResult struct { - Content []interface{} `json:"content"` - IsError bool `json:"isError"` - Metadata map[string]interface{} `json:"_meta,omitempty"` + Content []interface{} `json:"content"` + IsError bool `json:"isError"` + StructuredContent interface{} `json:"structuredContent,omitempty"` + Metadata map[string]interface{} `json:"_meta,omitempty"` } // CallTool calls a tool on an upstream server via daemon API. @@ -388,6 +389,7 @@ func (c *Client) CallTool( return nil, fmt.Errorf("failed to create request: %w", err) } req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-MCPProxy-Result-Envelope", "1") // Send request resp, err := c.httpClient.Do(req) @@ -429,16 +431,25 @@ func (c *Client) CallTool( // Convert data to CallToolResult format result := &CallToolResult{} - // Try to extract as map with content field - if dataMap, ok := apiResp.Data.(map[string]interface{}); ok { + // Older daemons return the MCP content array directly. + if content, ok := apiResp.Data.([]interface{}); ok { + result.Content = content + } else if dataMap, ok := apiResp.Data.(map[string]interface{}); ok { if content, hasContent := dataMap["content"].([]interface{}); hasContent { result.Content = content + } else if _, hasStructured := dataMap["structuredContent"]; hasStructured { + // Structured-only MCP results can omit content. + result.Content = []interface{}{} } else { - // Wrap data in content array if not already in that format + // Legacy non-MCP object payloads remain available as JSON text. + dataJSON, err := json.Marshal(apiResp.Data) + if err != nil { + return nil, fmt.Errorf("failed to marshal tool result: %w", err) + } result.Content = []interface{}{ map[string]interface{}{ "type": "text", - "text": fmt.Sprintf("%v", apiResp.Data), + "text": string(dataJSON), }, } } @@ -446,16 +457,27 @@ func (c *Client) CallTool( if isError, ok := dataMap["isError"].(bool); ok { result.IsError = isError } + if structured, ok := dataMap["structuredContent"]; ok { + result.StructuredContent = structured + } if meta, ok := dataMap["_meta"].(map[string]interface{}); ok { result.Metadata = meta } } else { - // Fallback: wrap data in content array + // Fallback for legacy scalar payloads. + dataJSON, err := json.Marshal(apiResp.Data) + if err != nil { + return nil, fmt.Errorf("failed to marshal tool result: %w", err) + } + text := string(dataJSON) + if raw, ok := apiResp.Data.(string); ok { + text = raw + } result.Content = []interface{}{ map[string]interface{}{ "type": "text", - "text": fmt.Sprintf("%v", apiResp.Data), + "text": text, }, } } diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 0efd0c9b8..f0774f1e6 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -6293,6 +6293,9 @@ func (s *Server) handleCallTool(w http.ResponseWriter, r *http.Request) { // logged every Web-UI tool call — and every shed of one — as if it came from // the CLI. The surface header the clients already send is the discriminator. ctx := reqcontext.WithRequestSource(r.Context(), toolCallRequestSource(r)) + if r.Header.Get("X-MCPProxy-Result-Envelope") == "1" { + ctx = reqcontext.WithToolResultEnvelope(ctx) + } // Call tool via controller result, err := s.controller.CallTool(ctx, request.ToolName, request.Arguments) diff --git a/internal/httpapi/tool_call_envelope_test.go b/internal/httpapi/tool_call_envelope_test.go new file mode 100644 index 000000000..3609f0057 --- /dev/null +++ b/internal/httpapi/tool_call_envelope_test.go @@ -0,0 +1,48 @@ +package httpapi + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" + "github.com/stretchr/testify/require" +) + +type toolCallEnvelopeProbe struct { + *baseController + wantsEnvelope bool +} + +func (p *toolCallEnvelopeProbe) GetCurrentConfig() *config.Config { return nil } + +func (p *toolCallEnvelopeProbe) CallTool(ctx context.Context, _ string, _ map[string]interface{}) (interface{}, error) { + p.wantsEnvelope = reqcontext.ToolResultEnvelope(ctx) + return []interface{}{map[string]interface{}{"type": "text", "text": "ok"}}, nil +} + +func TestToolCallResultEnvelopeHeaderIsOptIn(t *testing.T) { + for _, tc := range []struct { + name, header string + want bool + }{ + {"ordinary REST", "", false}, + {"CLI opt-in", "1", true}, + } { + t.Run(tc.name, func(t *testing.T) { + probe := &toolCallEnvelopeProbe{baseController: &baseController{}} + srv := &Server{controller: probe} + req := httptest.NewRequest(http.MethodPost, "/api/v1/tools/call", strings.NewReader(`{"tool_name":"call_tool_read","arguments":{}}`)) + if tc.header != "" { + req.Header.Set("X-MCPProxy-Result-Envelope", tc.header) + } + w := httptest.NewRecorder() + srv.handleCallTool(w, req) + require.Equal(t, http.StatusOK, w.Code) + require.Equal(t, tc.want, probe.wantsEnvelope) + }) + } +} diff --git a/internal/reqcontext/toolresult.go b/internal/reqcontext/toolresult.go new file mode 100644 index 000000000..0df0beee5 --- /dev/null +++ b/internal/reqcontext/toolresult.go @@ -0,0 +1,14 @@ +package reqcontext + +import "context" + +type toolResultEnvelopeKey struct{} + +// WithToolResultEnvelope opts a REST tool call into the full MCP result shape. +func WithToolResultEnvelope(ctx context.Context) context.Context { + return context.WithValue(ctx, toolResultEnvelopeKey{}, true) +} + +func ToolResultEnvelope(ctx context.Context) bool { + return ctx != nil && ctx.Value(toolResultEnvelopeKey{}) == true +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 71e7ec97a..6f20e9012 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -7264,6 +7264,16 @@ func (p *MCPProxyServer) monitorConnectionStatusAt(ctx context.Context, serverNa // CallToolDirect calls a tool directly without going through the MCP server's request handling // This is used for REST API calls that bypass the MCP protocol layer func (p *MCPProxyServer) CallToolDirect(ctx context.Context, request mcp.CallToolRequest) (interface{}, error) { + return p.callToolDirect(ctx, request, false) +} + +// CallToolDirectResult keeps the MCP result envelope for the REST tool-call +// endpoint. Other direct callers retain the historical content-only return. +func (p *MCPProxyServer) CallToolDirectResult(ctx context.Context, request mcp.CallToolRequest) (interface{}, error) { + return p.callToolDirect(ctx, request, true) +} + +func (p *MCPProxyServer) callToolDirect(ctx context.Context, request mcp.CallToolRequest, preserveResult bool) (interface{}, error) { toolName := request.Params.Name // Spec 093 FR-011: the REST surface must answer a concurrency-limiter shed @@ -7349,13 +7359,19 @@ func (p *MCPProxyServer) CallToolDirect(ctx context.Context, request mcp.CallToo if refusal := codeExecRefusal.take(); refusal != nil { return nil, &codeExecDispatchError{err: refusal, message: textContent.Text} } + if preserveResult { + return result, nil + } return nil, fmt.Errorf("%s", textContent.Text) } } return nil, fmt.Errorf("tool call failed") } - // Return the content as the result + if preserveResult { + return result, nil + } + // Return the content as the result for existing direct callers. return result.Content, nil } diff --git a/internal/server/mcp_call_tool_direct_test.go b/internal/server/mcp_call_tool_direct_test.go index 3696e338d..80c6a8dec 100644 --- a/internal/server/mcp_call_tool_direct_test.go +++ b/internal/server/mcp_call_tool_direct_test.go @@ -11,6 +11,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/cache" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/reqcontext" ) // Every truncation banner mcpproxy emits points the caller at read_cache. That @@ -70,6 +71,48 @@ func TestCallToolDirect_ReadCachePagesStoredRecords(t *testing.T) { assert.Equal(t, "github:get_repo", page.Records[0]["name"]) } +func TestCallToolDirectResultPreservesEnvelopeForREST(t *testing.T) { + proxy := createTestMCPProxyServer(t) + const key = "cache-key-result-envelope" + require.NoError(t, proxy.cacheManager.StoreAs(key, "retrieve_tools", map[string]interface{}{"query": "manage"}, `{"tools":[{"name":"echo"}]}`, "tools", 1, + cache.Authorization{CallerKind: cache.CallerKindAnonymous})) + request := mcp.CallToolRequest{} + request.Params.Name = "read_cache" + request.Params.Arguments = map[string]interface{}{"key": key, "offset": float64(0), "limit": float64(50)} + + got, err := proxy.CallToolDirectResult(context.Background(), request) + require.NoError(t, err) + result, ok := got.(*mcp.CallToolResult) + require.True(t, ok, "REST path must retain the MCP result envelope") + require.NotEmpty(t, result.Content) + _, ok = result.Content[0].(mcp.TextContent) + require.True(t, ok) + controller := &Server{mcpProxy: proxy} + legacy, err := controller.CallTool(context.Background(), "read_cache", request.Params.Arguments.(map[string]interface{})) + require.NoError(t, err) + _, ok = legacy.([]mcp.Content) + require.True(t, ok, "ordinary REST callers keep the content-array response") + envelope, err := controller.CallTool(reqcontext.WithToolResultEnvelope(context.Background()), "read_cache", request.Params.Arguments.(map[string]interface{})) + require.NoError(t, err) + _, ok = envelope.(*mcp.CallToolResult) + require.True(t, ok, "opted-in callers receive the full result") +} + +func TestCallToolDirectResultPreservesToolError(t *testing.T) { + proxy := createTestMCPProxyServer(t) + request := mcp.CallToolRequest{} + request.Params.Name = "read_cache" + request.Params.Arguments = map[string]interface{}{"key": "no-such-key", "offset": float64(0), "limit": float64(50)} + got, err := proxy.CallToolDirectResult(context.Background(), request) + require.NoError(t, err) + result, ok := got.(*mcp.CallToolResult) + require.True(t, ok) + require.True(t, result.IsError) + require.NotEmpty(t, result.Content) + _, legacyErr := proxy.CallToolDirect(context.Background(), request) + require.Error(t, legacyErr) +} + // Spec 105 FR-001/FR-010 (gap FR001-G5, task T025): the REST direct call path // (`POST /api/v1/tools/call` → CallToolDirect) is a read_cache redemption // door too, and an agent token's refusal there must be as non-disclosing as diff --git a/internal/server/server.go b/internal/server/server.go index 091acd3e1..3e688f089 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -4424,7 +4424,13 @@ func (s *Server) CallTool(ctx context.Context, toolName string, arguments map[st } // Call the tool via MCP proxy - result, err := s.mcpProxy.CallToolDirect(ctx, request) + var result interface{} + var err error + if reqcontext.ToolResultEnvelope(ctx) { + result, err = s.mcpProxy.CallToolDirectResult(ctx, request) + } else { + result, err = s.mcpProxy.CallToolDirect(ctx, request) + } if err != nil { return nil, fmt.Errorf("tool call failed: %w", err) } From 7c47118598ef29b4c318a1b417940635a050c017 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 22 Oct 2026 07:17:54 +0300 Subject: [PATCH 2/4] Preserve non-text MCP tool errors in CLI results --- internal/server/mcp.go | 42 +++++++++++--------- internal/server/mcp_call_tool_direct_test.go | 19 +++++++++ 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 6f20e9012..6eeda1658 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -7343,29 +7343,26 @@ func (p *MCPProxyServer) callToolDirect(ctx context.Context, request mcp.CallToo if limitErr := shed.take(); limitErr != nil { return nil, &shedDispatchError{limitErr: limitErr, message: shedMessage(limitErr)} } + message := "tool call failed" if len(result.Content) > 0 { if textContent, ok := result.Content[0].(mcp.TextContent); ok { - if refusal := profileToolRefusal.take(); refusal != nil { - return nil, refusal - } - // Quarantine / pending-approval / changed-tool blocks keep their - // typed identity so REST answers 403 with the payload, not a 500. - if refusal := quarantineRefusal.take(); refusal != nil { - return nil, refusal - } - // A code_execution refusal keeps its typed identity so the HTTP - // layer can answer 403/404/400 (Spec 097). The message stays the - // agent-readable one either way. - if refusal := codeExecRefusal.take(); refusal != nil { - return nil, &codeExecDispatchError{err: refusal, message: textContent.Text} - } - if preserveResult { - return result, nil - } - return nil, fmt.Errorf("%s", textContent.Text) + message = textContent.Text } } - return nil, fmt.Errorf("tool call failed") + if refusal := profileToolRefusal.take(); refusal != nil { + return nil, refusal + } + // Quarantine / pending-approval / changed-tool blocks keep their + // typed identity so REST answers 403 with the payload, not a 500. + if refusal := quarantineRefusal.take(); refusal != nil { + return nil, refusal + } + // A code_execution refusal keeps its typed identity so the HTTP + // layer can answer 403/404/400 (Spec 097). + if refusal := codeExecRefusal.take(); refusal != nil { + return nil, &codeExecDispatchError{err: refusal, message: message} + } + return directToolErrorPayload(result, preserveResult, message) } if preserveResult { @@ -7375,6 +7372,13 @@ func (p *MCPProxyServer) callToolDirect(ctx context.Context, request mcp.CallToo return result.Content, nil } +func directToolErrorPayload(result *mcp.CallToolResult, preserveResult bool, message string) (interface{}, error) { + if preserveResult { + return result, nil + } + return nil, fmt.Errorf("%s", message) +} + // ============================================================================ // Intent Declaration Support (Spec 018) // ============================================================================ diff --git a/internal/server/mcp_call_tool_direct_test.go b/internal/server/mcp_call_tool_direct_test.go index 80c6a8dec..376397f81 100644 --- a/internal/server/mcp_call_tool_direct_test.go +++ b/internal/server/mcp_call_tool_direct_test.go @@ -113,6 +113,25 @@ func TestCallToolDirectResultPreservesToolError(t *testing.T) { require.Error(t, legacyErr) } +func TestDirectToolErrorPayloadPreservesArbitraryContent(t *testing.T) { + for _, tc := range []struct { + name string + content []mcp.Content + }{ + {"empty", []mcp.Content{}}, + {"image first", []mcp.Content{mcp.NewImageContent("YWJj", "image/png")}}, + } { + t.Run(tc.name, func(t *testing.T) { + want := &mcp.CallToolResult{Content: tc.content, IsError: true, StructuredContent: map[string]any{"reason": tc.name}} + got, err := directToolErrorPayload(want, true, "tool call failed") + require.NoError(t, err) + require.Same(t, want, got) + _, err = directToolErrorPayload(want, false, "tool call failed") + require.EqualError(t, err, "tool call failed") + }) + } +} + // Spec 105 FR-001/FR-010 (gap FR001-G5, task T025): the REST direct call path // (`POST /api/v1/tools/call` → CallToolDirect) is a read_cache redemption // door too, and an agent token's refusal there must be as non-disclosing as From 71d7a93d09530ed9b3985aa7d8f819c1647f0b29 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 22 Oct 2026 07:23:55 +0300 Subject: [PATCH 3/4] Return nonzero for pretty tool errors --- cmd/mcpproxy/call_cmd.go | 17 ++++++++--- cmd/mcpproxy/call_result_json_test.go | 41 +++++++++++++++++++++++++++ docs/cli-output-formatting.md | 5 ++-- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/cmd/mcpproxy/call_cmd.go b/cmd/mcpproxy/call_cmd.go index 338893cb2..288d6fa8b 100644 --- a/cmd/mcpproxy/call_cmd.go +++ b/cmd/mcpproxy/call_cmd.go @@ -289,6 +289,17 @@ func outputCallResultPretty(result interface{}) { fmt.Printf("━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━\n\n") } +func outputPrettyCallResult(result interface{}, isError bool) error { + if isError { + fmt.Fprintln(os.Stderr, "⚠️ Tool call returned an error.") + outputCallResultPretty(result) + return callResultError{} + } + fmt.Fprintf(os.Stderr, "✅ Tool call completed successfully!\n\n") + outputCallResultPretty(result) + return nil +} + // createLogger creates a zap logger with the specified level func createLogger(level string) (*zap.Logger, error) { var zapLevel zap.AtomicLevel @@ -468,8 +479,7 @@ func runCallToolVariantClientMode(client *cliclient.Client, toolVariant string, case outputFormatPretty, "": fallthrough default: - fmt.Fprintf(os.Stderr, "✅ Tool call completed successfully!\n\n") - outputCallResultPretty(result) + return outputPrettyCallResult(result, result.IsError) } return nil @@ -550,8 +560,7 @@ func runCallToolVariantStandalone(ctx context.Context, toolVariant string, args case outputFormatPretty, "": fallthrough default: - fmt.Fprintf(os.Stderr, "✅ Tool call completed successfully!\n\n") - outputCallResultPretty(result) + return outputPrettyCallResult(result, result.IsError) } return nil diff --git a/cmd/mcpproxy/call_result_json_test.go b/cmd/mcpproxy/call_result_json_test.go index 0deae7ec9..6cfc2766d 100644 --- a/cmd/mcpproxy/call_result_json_test.go +++ b/cmd/mcpproxy/call_result_json_test.go @@ -6,8 +6,10 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" + "github.com/mark3labs/mcp-go/mcp" "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" "github.com/spf13/cobra" @@ -100,3 +102,42 @@ func TestCallToolJSONErrorRetainsRequestID(t *testing.T) { require.Equal(t, "req-123", got.RequestID) require.Equal(t, output.ErrCodeOperationFailed, got.Code) } + +func TestCallToolDaemonPrettyErrorIsNonzeroWithoutSuccessMessage(t *testing.T) { + oldFormat := callOutputFormat + callOutputFormat = "pretty" + defer func() { callOutputFormat = oldFormat }() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/status" { + _, _ = w.Write([]byte(`{"status":"ok"}`)) + return + } + _, _ = w.Write([]byte(`{"success":true,"data":{"content":[{"type":"text","text":"fixture denied"}],"isError":true}}`)) + })) + defer srv.Close() + stdoutReader, stdoutWriter, err := os.Pipe() + require.NoError(t, err) + stderrReader, stderrWriter, err := os.Pipe() + require.NoError(t, err) + oldStdout, oldStderr := os.Stdout, os.Stderr + os.Stdout, os.Stderr = stdoutWriter, stderrWriter + defer func() { os.Stdout, os.Stderr = oldStdout, oldStderr }() + + err = runCallToolVariantClientMode(cliclient.NewClient(srv.URL, nil), "call_tool_read", map[string]interface{}{"name": "fixture:fail"}, zap.NewNop()) + require.ErrorAs(t, err, new(callResultError)) + require.NoError(t, stdoutWriter.Close()) + require.NoError(t, stderrWriter.Close()) + stdout, err := io.ReadAll(stdoutReader) + require.NoError(t, err) + stderr, err := io.ReadAll(stderrReader) + require.NoError(t, err) + require.True(t, strings.Contains(string(stdout), "fixture denied")) + require.NotContains(t, string(stderr), "completed successfully") + require.Contains(t, string(stderr), "returned an error") +} + +func TestStandalonePrettyResultErrorIsNonzero(t *testing.T) { + result := &mcp.CallToolResult{Content: []mcp.Content{mcp.NewTextContent("standalone denied")}, IsError: true} + err := outputPrettyCallResult(result, result.IsError) + require.ErrorAs(t, err, new(callResultError)) +} diff --git a/docs/cli-output-formatting.md b/docs/cli-output-formatting.md index 15f7f8ae2..33b6356c8 100644 --- a/docs/cli-output-formatting.md +++ b/docs/cli-output-formatting.md @@ -145,8 +145,9 @@ This enables AI agents to discover available commands and their options programm ## Structured Error Output For `mcpproxy call tool-read`, `tool-write`, and `tool-destructive -o json`, -stdout contains the MCP result envelope. The CLI opts into this REST response -shape while existing REST callers retain their content-array response. `content` retains its ordered text, +stdout contains the MCP result envelope. The CLI sends +`X-MCPProxy-Result-Envelope: 1` to opt into this REST response shape; calls +without that header retain their content-array response. `content` retains its ordered text, image, audio, or resource blocks; `structuredContent` and `_meta` are included when supplied by the daemon. Older daemons that return a content array are normalized to `{ "content": [...], "isError": false }`. An `isError` result From 00355254f1dd19376e23d3e45bccf5baeacd2120 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 22 Oct 2026 07:28:01 +0300 Subject: [PATCH 4/4] Remove unreachable CLI result returns --- cmd/mcpproxy/call_cmd.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cmd/mcpproxy/call_cmd.go b/cmd/mcpproxy/call_cmd.go index 288d6fa8b..8b63ec222 100644 --- a/cmd/mcpproxy/call_cmd.go +++ b/cmd/mcpproxy/call_cmd.go @@ -462,7 +462,7 @@ func runCallToolVariantClientMode(client *cliclient.Client, toolVariant string, // For tool variants, we call the variant tool directly result, err := client.CallTool(callCtx, toolVariant, args) if err != nil { - // T028: Use cliError to include request_id in error output + // Keep the API error wrapped so JSON output can retain request_id. return callDaemonError{cause: err} } @@ -481,8 +481,6 @@ func runCallToolVariantClientMode(client *cliclient.Client, toolVariant string, default: return outputPrettyCallResult(result, result.IsError) } - - return nil } // runCallToolVariantStandalone calls tool variant directly without daemon @@ -562,6 +560,4 @@ func runCallToolVariantStandalone(ctx context.Context, toolVariant string, args default: return outputPrettyCallResult(result, result.IsError) } - - return nil }