From b1b28d72a3fe94462002fa3618ad7f547c8fd79d Mon Sep 17 00:00:00 2001 From: Henry Date: Thu, 10 Sep 2026 19:09:30 +0800 Subject: [PATCH] fix(mcp): stop reporting a dropped backend response as success (#3562) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../internal/mcp/server/backend_client.go | 21 ++- .../mcp/server/backend_client_test.go | 150 ++++++++++++++++++ .../mcp/server/dispatcher_backend_client.go | 15 +- .../server/dispatcher_backend_client_test.go | 116 ++++++++++++++ .../server/task_plan_empty_payload_test.go | 22 +++ .../requirements/mcp-bridge-reliability.md | 36 +++++ .../system-design/mcp-bridge-reliability.md | 26 +++ 7 files changed, 378 insertions(+), 8 deletions(-) create mode 100644 apps/backend/internal/mcp/server/task_plan_empty_payload_test.go diff --git a/apps/backend/internal/mcp/server/backend_client.go b/apps/backend/internal/mcp/server/backend_client.go index d3ce808b3a..5008d6f6ec 100644 --- a/apps/backend/internal/mcp/server/backend_client.go +++ b/apps/backend/internal/mcp/server/backend_client.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "sync" "time" @@ -13,6 +14,9 @@ import ( "go.uber.org/zap" ) +// ErrEmptyBackendPayload identifies a response with no payload bytes when a result sink was provided. +var ErrEmptyBackendPayload = errors.New("mcp backend response payload was empty") + // MCPRequest represents an MCP request to be sent to the backend. type MCPRequest struct { ID string `json:"id"` @@ -236,10 +240,19 @@ func (c *ChannelBackendClient) RequestPayload(ctx context.Context, action string } return fmt.Errorf("backend error: %s", string(resp.Payload)) } - if result != nil && len(resp.Payload) > 0 { - if err := json.Unmarshal(resp.Payload, result); err != nil { - return fmt.Errorf("failed to unmarshal response: %w", err) - } + if result == nil { + return nil + } + if len(resp.Payload) == 0 { + c.logger.Warn(ErrEmptyBackendPayload.Error(), + zap.String("request_id", id), + zap.String("action", action), + zap.String("session_id", response.sessionID), + zap.Duration("duration", time.Since(start))) + return fmt.Errorf("empty response payload for action %q: %w", action, ErrEmptyBackendPayload) + } + if err := json.Unmarshal(resp.Payload, result); err != nil { + return fmt.Errorf("failed to unmarshal response: %w", err) } return nil case <-ctx.Done(): diff --git a/apps/backend/internal/mcp/server/backend_client_test.go b/apps/backend/internal/mcp/server/backend_client_test.go index 627462cc47..8e62130618 100644 --- a/apps/backend/internal/mcp/server/backend_client_test.go +++ b/apps/backend/internal/mcp/server/backend_client_test.go @@ -2,6 +2,7 @@ package mcp import ( "context" + "encoding/json" "errors" "testing" "time" @@ -10,6 +11,7 @@ import ( ws "github.com/kandev/kandev/pkg/websocket" "github.com/stretchr/testify/require" "go.uber.org/zap" + "go.uber.org/zap/zapcore" "go.uber.org/zap/zaptest/observer" ) @@ -126,6 +128,154 @@ func TestChannelBackendClientTerminalFailureLogIncludesConfiguredSession(t *test require.Equal(t, "session-log", entries[0].ContextMap()["session_id"]) } +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.1 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.2 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.9 +func TestChannelBackendClientEmptyPayloadWithResultSinkErrors(t *testing.T) { + core, observed := observer.New(zap.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + client := NewChannelBackendClient(log) + t.Cleanup(client.Close) + + var result map[string]interface{} + errCh := make(chan error, 1) + go func() { + errCh <- client.RequestPayload(context.Background(), "test.action", map[string]any{"secret": "canary-leak-9f3a"}, &result) + }() + request := <-client.GetRequestChannel() + + response, err := ws.NewResponse(request.ID, request.Action, nil) + require.NoError(t, err) + response.Payload = nil + client.HandleResponse(response) + + respErr := <-errCh + require.Error(t, respErr) + require.ErrorIs(t, respErr, ErrEmptyBackendPayload) + require.Contains(t, respErr.Error(), "test.action") + require.NotContains(t, respErr.Error(), "secret") + + entries := observed.FilterMessage(ErrEmptyBackendPayload.Error()).All() + require.Len(t, entries, 1) + require.Equal(t, zapcore.WarnLevel, entries[0].Level) + fields := entries[0].ContextMap() + require.Equal(t, request.ID, fields["request_id"]) + require.Equal(t, "test.action", fields["action"]) + require.Contains(t, fields, "session_id") + require.Contains(t, fields, "duration") + + logJSON, err := json.Marshal(fields) + require.NoError(t, err) + require.NotContains(t, string(logJSON), "secret") + require.NotContains(t, string(logJSON), "canary-leak-9f3a") +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.3 +func TestChannelBackendClientEmptyPayloadWithNilResultSucceeds(t *testing.T) { + client := NewChannelBackendClient(nil) + t.Cleanup(client.Close) + + errCh := make(chan error, 1) + go func() { + errCh <- client.RequestPayload(context.Background(), "test.action", nil, nil) + }() + request := <-client.GetRequestChannel() + + response, err := ws.NewResponse(request.ID, request.Action, nil) + require.NoError(t, err) + response.Payload = nil + client.HandleResponse(response) + + require.NoError(t, <-errCh) +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.4 +func TestChannelBackendClientErrorTypeWithEmptyPayloadKeepsBackendErrorBehavior(t *testing.T) { + client := NewChannelBackendClient(nil) + t.Cleanup(client.Close) + + var result map[string]interface{} + errCh := make(chan error, 1) + go func() { + errCh <- client.RequestPayload(context.Background(), "test.action", nil, &result) + }() + request := <-client.GetRequestChannel() + + response := &ws.Message{ID: request.ID, Action: request.Action, Type: ws.MessageTypeError, Payload: nil} + client.HandleResponse(response) + + respErr := <-errCh + require.Error(t, respErr) + require.NotErrorIs(t, respErr, ErrEmptyBackendPayload) + require.Contains(t, respErr.Error(), "backend error") +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.5 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.8 +func TestChannelBackendClientEmptyObjectPayloadDecodesToNonNilEmptyMap(t *testing.T) { + client := NewChannelBackendClient(nil) + t.Cleanup(client.Close) + + var result map[string]interface{} + errCh := make(chan error, 1) + go func() { + errCh <- client.RequestPayload(context.Background(), "test.action", nil, &result) + }() + request := <-client.GetRequestChannel() + + response, err := ws.NewResponse(request.ID, request.Action, map[string]interface{}{}) + require.NoError(t, err) + client.HandleResponse(response) + + require.NoError(t, <-errCh) + require.NotNil(t, result) + require.Empty(t, result) +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.6 +func TestChannelBackendClientNullPayloadStillDecodesWithoutError(t *testing.T) { + client := NewChannelBackendClient(nil) + t.Cleanup(client.Close) + + result := map[string]interface{}{"stale": "value"} + errCh := make(chan error, 1) + go func() { + errCh <- client.RequestPayload(context.Background(), "test.action", nil, &result) + }() + request := <-client.GetRequestChannel() + + response, err := ws.NewResponse(request.ID, request.Action, nil) + require.NoError(t, err) + client.HandleResponse(response) + + require.NoError(t, <-errCh) + require.Nil(t, result) +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.7 +func TestChannelBackendClientInvalidJSONPayloadKeepsUnmarshalError(t *testing.T) { + client := NewChannelBackendClient(nil) + t.Cleanup(client.Close) + + var result map[string]interface{} + errCh := make(chan error, 1) + go func() { + errCh <- client.RequestPayload(context.Background(), "test.action", nil, &result) + }() + request := <-client.GetRequestChannel() + + response, err := ws.NewResponse(request.ID, request.Action, nil) + require.NoError(t, err) + response.Payload = []byte("not json") + client.HandleResponse(response) + + respErr := <-errCh + require.Error(t, respErr) + require.Contains(t, respErr.Error(), "failed to unmarshal") + require.NotErrorIs(t, respErr, ErrEmptyBackendPayload) +} + func TestChannelBackendClientRedactsPluginInvocationPayload(t *testing.T) { core, observed := observer.New(zap.DebugLevel) log, err := logger.NewFromZap(zap.New(core)) diff --git a/apps/backend/internal/mcp/server/dispatcher_backend_client.go b/apps/backend/internal/mcp/server/dispatcher_backend_client.go index 75ccb5b7f4..8760f145d3 100644 --- a/apps/backend/internal/mcp/server/dispatcher_backend_client.go +++ b/apps/backend/internal/mcp/server/dispatcher_backend_client.go @@ -75,10 +75,17 @@ func (c *DispatcherBackendClient) RequestPayload(ctx context.Context, action str } return fmt.Errorf("backend error: %s", string(resp.Payload)) } - if result != nil && len(resp.Payload) > 0 { - if err := json.Unmarshal(resp.Payload, result); err != nil { - return fmt.Errorf("failed to unmarshal response for %s: %w", action, err) - } + if result == nil { + return nil + } + if len(resp.Payload) == 0 { + c.logger.Warn(ErrEmptyBackendPayload.Error(), + zap.String("request_id", id), + zap.String("action", action)) + return fmt.Errorf("empty response payload for action %q: %w", action, ErrEmptyBackendPayload) + } + if err := json.Unmarshal(resp.Payload, result); err != nil { + return fmt.Errorf("failed to unmarshal response for %s: %w", action, err) } return nil } diff --git a/apps/backend/internal/mcp/server/dispatcher_backend_client_test.go b/apps/backend/internal/mcp/server/dispatcher_backend_client_test.go index 90558ee0f1..a9fa860fd7 100644 --- a/apps/backend/internal/mcp/server/dispatcher_backend_client_test.go +++ b/apps/backend/internal/mcp/server/dispatcher_backend_client_test.go @@ -6,10 +6,14 @@ import ( "errors" "testing" + "github.com/kandev/kandev/internal/common/logger" mcporigin "github.com/kandev/kandev/internal/mcp/origin" ws "github.com/kandev/kandev/pkg/websocket" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" ) // fakeDispatcher is a minimal Dispatcher used to drive DispatcherBackendClient tests. @@ -114,3 +118,115 @@ func TestDispatcherBackendClient_NilResultIsAllowed(t *testing.T) { err = client.RequestPayload(context.Background(), "test.action", nil, nil) require.NoError(t, err) } + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.1 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.2 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.9 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.10 +func TestDispatcherBackendClient_EmptyPayloadWithResultSinkErrors(t *testing.T) { + core, observed := observer.New(zap.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + require.NoError(t, err) + + d := &fakeDispatcher{resp: &ws.Message{Type: ws.MessageTypeResponse, Payload: nil}} + client := NewDispatcherBackendClient(d, log) + + var result map[string]interface{} + respErr := client.RequestPayload(context.Background(), "test.action", map[string]any{"secret": "canary-leak-9f3a"}, &result) + require.Error(t, respErr) + require.ErrorIs(t, respErr, ErrEmptyBackendPayload) + require.Contains(t, respErr.Error(), "test.action") + require.NotContains(t, respErr.Error(), "secret") + + require.Len(t, d.calls, 1) + outboundID := d.calls[0].ID + require.NotEmpty(t, outboundID) + + entries := observed.FilterMessage(ErrEmptyBackendPayload.Error()).All() + require.Len(t, entries, 1) + require.Equal(t, zapcore.WarnLevel, entries[0].Level) + fields := entries[0].ContextMap() + require.Equal(t, outboundID, fields["request_id"]) + require.Equal(t, "test.action", fields["action"]) + _, hasSessionID := fields["session_id"] + require.False(t, hasSessionID) + _, hasDuration := fields["duration"] + require.False(t, hasDuration) + + logJSON, err := json.Marshal(fields) + require.NoError(t, err) + require.NotContains(t, string(logJSON), "secret") + require.NotContains(t, string(logJSON), "canary-leak-9f3a") +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.3 +func TestDispatcherBackendClient_EmptyPayloadWithNilResultSucceeds(t *testing.T) { + log := newTestLogger(t) + + d := &fakeDispatcher{resp: &ws.Message{Type: ws.MessageTypeResponse, Payload: nil}} + client := NewDispatcherBackendClient(d, log) + + err := client.RequestPayload(context.Background(), "test.action", nil, nil) + require.NoError(t, err) +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.4 +func TestDispatcherBackendClient_ErrorTypeWithEmptyPayloadKeepsBackendErrorBehavior(t *testing.T) { + log := newTestLogger(t) + + d := &fakeDispatcher{resp: &ws.Message{Type: ws.MessageTypeError, Payload: nil}} + client := NewDispatcherBackendClient(d, log) + + var result map[string]interface{} + err := client.RequestPayload(context.Background(), "test.action", nil, &result) + require.Error(t, err) + require.NotErrorIs(t, err, ErrEmptyBackendPayload) + require.Contains(t, err.Error(), "backend error") +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.5 +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.8 +func TestDispatcherBackendClient_EmptyObjectPayloadDecodesToNonNilEmptyMap(t *testing.T) { + log := newTestLogger(t) + + respMsg, err := ws.NewResponse("ignored", "test.action", map[string]interface{}{}) + require.NoError(t, err) + d := &fakeDispatcher{resp: respMsg} + client := NewDispatcherBackendClient(d, log) + + var result map[string]interface{} + err = client.RequestPayload(context.Background(), "test.action", nil, &result) + require.NoError(t, err) + require.NotNil(t, result) + require.Empty(t, result) +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.6 +func TestDispatcherBackendClient_NullPayloadStillDecodesWithoutError(t *testing.T) { + log := newTestLogger(t) + + respMsg, err := ws.NewResponse("ignored", "test.action", nil) + require.NoError(t, err) + d := &fakeDispatcher{resp: respMsg} + client := NewDispatcherBackendClient(d, log) + + result := map[string]interface{}{"stale": "value"} + err = client.RequestPayload(context.Background(), "test.action", nil, &result) + require.NoError(t, err) + require.Nil(t, result) +} + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.7 +func TestDispatcherBackendClient_InvalidJSONPayloadKeepsUnmarshalError(t *testing.T) { + log := newTestLogger(t) + + respMsg := &ws.Message{Type: ws.MessageTypeResponse, Payload: json.RawMessage("not json")} + d := &fakeDispatcher{resp: respMsg} + client := NewDispatcherBackendClient(d, log) + + var result map[string]interface{} + err := client.RequestPayload(context.Background(), "test.action", nil, &result) + require.Error(t, err) + require.Contains(t, err.Error(), "failed to unmarshal") + require.NotErrorIs(t, err, ErrEmptyBackendPayload) +} diff --git a/apps/backend/internal/mcp/server/task_plan_empty_payload_test.go b/apps/backend/internal/mcp/server/task_plan_empty_payload_test.go new file mode 100644 index 0000000000..598c66db8a --- /dev/null +++ b/apps/backend/internal/mcp/server/task_plan_empty_payload_test.go @@ -0,0 +1,22 @@ +package mcp + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// @covers AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.5 +// TestGetTaskPlan_EmptyObjectPayloadStillRendersNoPlan guards the transport +// change against the deliberately unchanged "no plan" convention: a `{}` +// backend response must keep decoding successfully so this rendering is +// unaffected by the new empty-payload rule. +func TestGetTaskPlan_EmptyObjectPayloadStillRendersNoPlan(t *testing.T) { + backend := &testBackend{response: map[string]interface{}{}} + s := newTaskModeServer(t, backend, "task-A") + + result := callTool(t, s, "get_task_plan_kandev", map[string]interface{}{}) + + require.False(t, result.IsError) + require.Equal(t, "No plan exists for this task yet.", resultText(t, result)) +} diff --git a/docs/specs/agents/requirements/mcp-bridge-reliability.md b/docs/specs/agents/requirements/mcp-bridge-reliability.md index ffec64e639..adb58bdcce 100644 --- a/docs/specs/agents/requirements/mcp-bridge-reliability.md +++ b/docs/specs/agents/requirements/mcp-bridge-reliability.md @@ -56,8 +56,44 @@ client timeout without diagnostic evidence. action, request, and session for accepted requests and terminal bridge errors. These logs shall not contain tool arguments. +### REQ-AGENTS-MCP-BRIDGE-RELIABILITY-002: Empty response payload reporting + +**Intent:** A backend client must report an empty response payload as a bridge +error when a caller expects a result. It must not turn an empty payload into a +successful result with an unset value. + +#### Acceptance criteria + +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.1:** When a successful response has + zero payload bytes and the caller supplied a result sink, the backend client + shall return an error. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.2:** The error shall identify the + empty-payload condition and include the action name. It shall not include the + request payload or any tool argument. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.3:** When the caller supplied no + result sink, a zero-byte payload shall keep the existing successful behavior. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.4:** An error response shall keep the + existing backend-error behavior, even when its payload has zero bytes. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.5:** A valid empty object shall keep + decoding successfully. The task-plan tool shall keep its current no-plan + text for that object. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.6:** A valid `null` payload shall keep + its current decoding behavior and shall not become an empty-payload error. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.7:** A non-empty invalid or + unassignable payload shall keep returning its decoding error. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.8:** The channel and dispatcher + backend clients shall use the same response classification rules. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.9:** Each empty-payload error shall + create one warning with the request ID and action. The channel client shall + also include the session ID and duration. Warnings shall not include tool + arguments. +- **AC-AGENTS-MCP-BRIDGE-RELIABILITY-002.10:** The error shall reach the caller + on its first occurrence. This behavior shall add no retry or backoff. + ## Out of scope - Deadlines for task, integration, plugin, or agent-launch business operations. - A new user-interface alert for a stalled MCP request. - Traffic between an agent and a third-party MCP server. +- Treating valid `null` payloads as transport errors. +- Reproducing or assigning a root cause to a historical empty-result incident. diff --git a/docs/specs/agents/system-design/mcp-bridge-reliability.md b/docs/specs/agents/system-design/mcp-bridge-reliability.md index a6e8c241f6..de19c717fe 100644 --- a/docs/specs/agents/system-design/mcp-bridge-reliability.md +++ b/docs/specs/agents/system-design/mcp-bridge-reliability.md @@ -3,6 +3,7 @@ status: current system: agents requirements: - REQ-AGENTS-MCP-BRIDGE-RELIABILITY-001 + - REQ-AGENTS-MCP-BRIDGE-RELIABILITY-002 created: 2026-09-04 owners: - Kandev @@ -25,6 +26,7 @@ operation keeps its own context and deadline. | Requirement | Design sections | | --- | --- | | `REQ-AGENTS-MCP-BRIDGE-RELIABILITY-001` | [Current dispatcher binding](#current-dispatcher-binding), [Request ownership](#request-ownership), [Failure behavior](#failure-behavior), [Observability](#observability) | +| `REQ-AGENTS-MCP-BRIDGE-RELIABILITY-002` | [Empty response classification](#empty-response-classification), [Failure behavior](#failure-behavior), [Observability](#observability), [Testing strategy](#testing-strategy) | ## Confirmed fault @@ -127,6 +129,27 @@ This design does not add a common response deadline. Such a deadline can stop valid clarification waits and long launch operations. Transport ownership and caller contexts provide the required termination signals. +## Empty response classification + +`ChannelBackendClient` and `DispatcherBackendClient` classify a response after +the transport has returned a non-nil message without an error. + +Both clients apply this order: + +1. An error response returns the existing backend error. +2. A nil result sink returns success without payload decoding. +3. A zero-byte payload returns the shared `ErrEmptyBackendPayload` error. +4. A non-empty payload passes to JSON decoding, which returns any existing error. + +The clients use an action-specific wrapped error for the empty-payload case. +The channel client logs the configured session ID and elapsed duration. The +dispatcher client logs only fields that exist at its in-process boundary. + +The rule detects an empty response envelope. It does not identify the cause of +the missing payload, validate a response schema, or classify valid `null` data. +Valid `{}`, `[]`, `null`, and other non-empty JSON values keep their current +behavior. + ## Observability The backend records one request-received event at `Info`. The event includes @@ -153,6 +176,9 @@ Unit tests cover these cases: - A stream disconnect releases only requests owned by that stream. - Successful responses, context cancellation, reset, close, and clarification waits keep their current behavior. +- Both clients return an error for a zero-byte success payload when a result + sink exists, and they preserve the nil-sink, error-response, valid JSON, and + decode-error cases described in [Empty response classification](#empty-response-classification). Race-enabled tests cover concurrent dispatcher updates, request completion, stream replacement, reset, and close.