diff --git a/internal/agent/tools/mcp/channel.go b/internal/agent/tools/mcp/channel.go index ebd29d92fd..0becddd3c3 100644 --- a/internal/agent/tools/mcp/channel.go +++ b/internal/agent/tools/mcp/channel.go @@ -285,6 +285,9 @@ type channelTransport struct { } // Connect implements mcp.Transport. +// unwrapTransport implements [transportWrapper]. +func (t *channelTransport) unwrapTransport() mcp.Transport { return t.inner } + func (t *channelTransport) Connect(ctx context.Context) (mcp.Connection, error) { conn, err := t.inner.Connect(ctx) if err != nil { diff --git a/internal/agent/tools/mcp/init.go b/internal/agent/tools/mcp/init.go index c3cf935b47..1873a03f5b 100644 --- a/internal/agent/tools/mcp/init.go +++ b/internal/agent/tools/mcp/init.go @@ -967,6 +967,25 @@ func createSession(ctx context.Context, cfg *config.ConfigStore, name string, m }, nil } +// transportWrapper is implemented by every transport decorator crush layers +// around a base transport, so diagnostics that need the innermost transport +// can reach it without knowing which decorators are in play. +type transportWrapper interface { + unwrapTransport() mcp.Transport +} + +// unwrapTransport peels every decorator off a transport and returns the +// innermost one. +func unwrapTransport(transport mcp.Transport) mcp.Transport { + for { + w, ok := transport.(transportWrapper) + if !ok { + return transport + } + transport = w.unwrapTransport() + } +} + // maybeStdioErr if a stdio mcp prints an error in non-json format, it'll fail // to parse, and the cli will then close it, causing the EOF error. // so, if we got an EOF err, and the transport is STDIO, we try to exec it @@ -978,6 +997,13 @@ func maybeStdioErr(err error, transport mcp.Transport) error { if !errors.Is(err, io.EOF) { return err } + // The transport is wrapped in one or more decorators before Connect (the + // channel gate today); the stdio transport we're probing for is the + // innermost one. Unwrap all of them — without this the assertion below + // never matches and stdio startup failures report a bare EOF instead of + // the child's actual output. Every wrapper must implement + // unwrapTransport or it will hide this diagnostic again. + transport = unwrapTransport(transport) ct, ok := transport.(*mcp.CommandTransport) if !ok { return err @@ -1277,7 +1303,14 @@ func clearMCPData(name string) { func stdioCheck(old *exec.Cmd) error { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) defer cancel() - cmd := exec.CommandContext(ctx, old.Path, old.Args...) + // old.Args includes argv0 as the first element; exec.CommandContext + // prepends old.Path as argv0, so we must skip it to avoid duplication + // (e.g. "npx npx -y pkg" instead of "npx -y pkg"). + args := old.Args + if len(args) > 0 { + args = args[1:] + } + cmd := exec.CommandContext(ctx, old.Path, args...) cmd.Env = old.Env out, err := cmd.CombinedOutput() if err == nil || errors.Is(ctx.Err(), context.DeadlineExceeded) { diff --git a/internal/agent/tools/mcp/lifecycle_test.go b/internal/agent/tools/mcp/lifecycle_test.go index 1379b8eb93..0e2a011ea2 100644 --- a/internal/agent/tools/mcp/lifecycle_test.go +++ b/internal/agent/tools/mcp/lifecycle_test.go @@ -3,6 +3,8 @@ package mcp import ( "context" "errors" + "io" + "os/exec" "sync" "sync/atomic" "testing" @@ -393,3 +395,63 @@ func TestGetOrRenewClient_RestoresPromptsAndResources(t *testing.T) { require.Equal(t, Counts{Tools: 1, Prompts: 1, Resources: 1}, info.Counts, "reported counts must match the restored registries") } + +// testTransportWrapper is a second, test-local decorator. maybeStdioErr must +// see through an arbitrary stack of them, not just the one wrapper that +// happens to exist in createSession today. +type testTransportWrapper struct { + mcp.Transport + inner mcp.Transport +} + +func (t *testTransportWrapper) unwrapTransport() mcp.Transport { return t.inner } + +// TestMaybeStdioErr_UnwrapsChannelTransport pins that maybeStdioErr sees +// through the channelTransport wrapper to the inner CommandTransport. +// +// Every transport is wrapped in a channelTransport before Connect, so the +// *mcp.CommandTransport assertion never matched and a failed stdio server (a +// missing npx, node not on PATH) reported a bare EOF with the child's stderr +// thrown away — the exact diagnostic stdioCheck exists to provide. We assert +// both that the unwrap reaches the command (the error is no longer bare EOF) +// and that the re-executed child's output surfaces in the joined error. +func TestMaybeStdioErr_UnwrapsChannelTransport(t *testing.T) { + cmd := exec.CommandContext(t.Context(), "sh", "-c", "echo 'startup failed: bad config'; exit 3") + inner := &mcp.CommandTransport{Command: cmd} + wrapped := &channelTransport{inner: inner, name: "t", gate: newChannelGate()} + + got := maybeStdioErr(io.EOF, wrapped) + require.Error(t, got) + require.NotEqual(t, io.EOF, got, "the unwrap must reach the command transport") + require.ErrorContains(t, got, "startup failed: bad config", + "the re-executed child's output must surface in the error") +} + +// TestMaybeStdioErr_UnwrapsEveryWrapper pins the unwrap against future +// decorators: it must peel the whole stack, not a fixed number of layers. +func TestMaybeStdioErr_UnwrapsEveryWrapper(t *testing.T) { + cmd := exec.CommandContext(t.Context(), "sh", "-c", "echo boom-diagnostic >&2; exit 3") + var transport mcp.Transport = &mcp.CommandTransport{Command: cmd} + transport = &channelTransport{inner: transport, name: "t", gate: newChannelGate()} + transport = &testTransportWrapper{inner: transport} + + got := maybeStdioErr(io.EOF, transport) + require.ErrorContains(t, got, "boom-diagnostic", + "stdio diagnostics must survive every transport decorator") +} + +// TestStdioCheck_DoesNotDuplicateArgv0 pins the argv0 handling in the +// diagnostic re-run. exec.Cmd.Args carries argv0 as its first element and +// exec.CommandContext prepends Path as argv0 itself, so passing Args through +// whole re-ran "sh sh -c ..." — and the error reported that malformed +// command's failure instead of the child's real startup output. +func TestStdioCheck_DoesNotDuplicateArgv0(t *testing.T) { + cmd := exec.CommandContext(t.Context(), "sh", "-c", "echo 'real startup error'; exit 3") + + err := stdioCheck(cmd) + require.Error(t, err) + require.ErrorContains(t, err, "real startup error", + "the re-run must execute the original command, not a duplicated argv0") + require.NotContains(t, err.Error(), "cannot execute binary file", + "a duplicated argv0 makes the shell try to exec itself as a script") +}