Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/agent/tools/mcp/channel.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
35 changes: 34 additions & 1 deletion internal/agent/tools/mcp/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
62 changes: 62 additions & 0 deletions internal/agent/tools/mcp/lifecycle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package mcp
import (
"context"
"errors"
"io"
"os/exec"
"sync"
"sync/atomic"
"testing"
Expand Down Expand Up @@ -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")
}
Loading