Merge pull request #17 from ethpandaops/fix/stdio-mcp-args-toml-array

fix: serialize stdio MCP Args as TOML inline array

Author: Savid

examples/mcp_stdio_args/main.go

@@ -0,0 +1,55 @@
+// Package main demonstrates configuring a stdio MCP server with command-line
+// arguments (Args).
+//
+// The placeholder command `/bin/sh -c "cat"` is used so the example can run
+// without depending on a real MCP binary being installed — it is enough for
+// codex to load the config and complete the app-server handshake. Replace the
+// Command and Args with a real stdio MCP launcher (for example
+// `npx -y @playwright/mcp@latest`) in practice.
+//
+// Historical context: before Args were serialized as a TOML inline array,
+// running this example failed with "transport closed while waiting for
+// response" because codex rejected the config with
+// `invalid type: string ..., expected a sequence`.
+package main
+
+import (
+	"context"
+	"fmt"
+	"log/slog"
+	"os"
+	"time"
+
+	codexsdk "github.com/ethpandaops/codex-agent-sdk-go"
+)
+
+func main() {
+	logger := slog.New(slog.NewTextHandler(os.Stderr, nil))
+
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	client := codexsdk.NewClient()
+
+	defer func() {
+		if err := client.Close(); err != nil {
+			fmt.Fprintf(os.Stderr, "failed to close client: %v\n", err)
+		}
+	}()
+
+	err := client.Start(ctx,
+		codexsdk.WithLogger(logger),
+		codexsdk.WithMCPServers(map[string]codexsdk.MCPServerConfig{
+			"stdio-args": &codexsdk.MCPStdioServerConfig{
+				Command: "/bin/sh",
+				Args:    []string{"-c", "cat"},
+			},
+		}),
+	)
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "client.Start failed: %v\n", err)
+		os.Exit(1)
+	}
+
+	fmt.Println("client started: stdio MCP server with Args configured successfully")
+}

integration/mcp_tools_test.go

@@ -361,6 +361,40 @@ func TestSDKTools_ReturnValue(t *testing.T) {
 	require.True(t, mentionedNumber, "Agent should mention the returned number (42)")
 }
 
+// TestMCPStdio_ConfigLoadsWithArgs is a regression test for the bug where
+// MCPStdioServerConfig.Args was serialized as repeated scalar -c overrides,
+// each replacing the prior value at the same dotted path. That produced a
+// single string for `args` instead of a TOML array, and codex exited during
+// config validation before the app-server handshake could complete.
+//
+// The surfaced error was "transport closed while waiting for response" —
+// which pointed at the transport layer, not at config serialization.
+//
+// This test uses `/bin/sh -c 'cat'` as a placeholder stdio "server": it never
+// speaks MCP, but codex only needs the config to load cleanly for client.Start
+// to succeed. Inner MCP handshake failures surface via GetMCPStatus, not Start.
+func TestMCPStdio_ConfigLoadsWithArgs(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	client := codexsdk.NewClient()
+	defer client.Close()
+
+	err := client.Start(ctx,
+		codexsdk.WithPermissionMode("bypassPermissions"),
+		codexsdk.WithMCPServers(map[string]codexsdk.MCPServerConfig{
+			"stdio-args": &codexsdk.MCPStdioServerConfig{
+				Command: "/bin/sh",
+				Args:    []string{"-c", "cat"},
+			},
+		}),
+	)
+	if err != nil {
+		skipIfCLINotInstalled(t, err)
+		t.Fatalf("client.Start failed with stdio MCP args (likely Args serialization regression): %v", err)
+	}
+}
+
 func TestMCPStatus_IncludesSDKServerMetadata(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second)
 	defer cancel()

internal/cli/cli_test.go

@@ -878,6 +878,13 @@ func TestBuildExecArgs_WithMCPServers_SSE(t *testing.T) {
 }
 
 // TestBuildExecArgs_WithMCPServers_Stdio tests stdio MCP server serialization.
+//
+// Args must be emitted as a single TOML inline array under one -c override.
+// Emitting one -c mcp_servers.NAME.args=VAL per element does NOT accumulate
+// into an array — each -c override replaces the previous value at the same
+// dotted path. Codex's config schema requires Vec<String>, so repeated scalar
+// overrides cause the subprocess to exit before handshake with
+// `invalid type: string ..., expected a sequence`.
 func TestBuildExecArgs_WithMCPServers_Stdio(t *testing.T) {
 	options := &config.Options{
 		MCPServers: map[string]mcp.ServerConfig{
@@ -893,12 +900,90 @@ func TestBuildExecArgs_WithMCPServers_Stdio(t *testing.T) {
 
 	require.Contains(t, args, "mcp_servers.local-tool.type=stdio")
 	require.Contains(t, args, "mcp_servers.local-tool.command=/usr/bin/my-tool")
-	require.Contains(t, args, "mcp_servers.local-tool.args=--port")
-	require.Contains(t, args, "mcp_servers.local-tool.args=8080")
+	require.Contains(t, args, `mcp_servers.local-tool.args=["--port","8080"]`,
+		"Args must be serialized as a single TOML inline array override")
+	require.NotContains(t, args, "mcp_servers.local-tool.args=--port",
+		"Args must NOT be emitted as repeated scalar -c overrides")
+	require.NotContains(t, args, "mcp_servers.local-tool.args=8080",
+		"Args must NOT be emitted as repeated scalar -c overrides")
 	require.Contains(t, args, "mcp_servers.local-tool.env.DEBUG=true")
 	require.Contains(t, args, "mcp_servers.local-tool.env.HOME=/tmp")
 }
 
+// TestBuildExecArgs_WithMCPServers_StdioArgsNpx covers the real-world npx case
+// from the issue report: `npx -y @playwright/mcp@latest`.
+func TestBuildExecArgs_WithMCPServers_StdioArgsNpx(t *testing.T) {
+	options := &config.Options{
+		MCPServers: map[string]mcp.ServerConfig{
+			"playwright": &mcp.StdioServerConfig{
+				Command: "npx",
+				Args:    []string{"-y", "@playwright/mcp@latest"},
+			},
+		},
+	}
+
+	args := BuildExecArgs("test", options)
+
+	require.Contains(t, args, `mcp_servers.playwright.args=["-y","@playwright/mcp@latest"]`)
+}
+
+// TestBuildExecArgs_WithMCPServers_StdioArgsSingleton ensures even a single arg
+// is wrapped in array form — codex config declares args as Vec<String>, so a
+// single scalar value fails type-checking even with one element.
+func TestBuildExecArgs_WithMCPServers_StdioArgsSingleton(t *testing.T) {
+	options := &config.Options{
+		MCPServers: map[string]mcp.ServerConfig{
+			"solo": &mcp.StdioServerConfig{
+				Command: "tool",
+				Args:    []string{"only-arg"},
+			},
+		},
+	}
+
+	args := BuildExecArgs("test", options)
+
+	require.Contains(t, args, `mcp_servers.solo.args=["only-arg"]`)
+	require.NotContains(t, args, "mcp_servers.solo.args=only-arg")
+}
+
+// TestBuildExecArgs_WithMCPServers_StdioArgsEscaping verifies that args with
+// quotes, backslashes, or other special characters are safely escaped so the
+// resulting TOML is valid.
+func TestBuildExecArgs_WithMCPServers_StdioArgsEscaping(t *testing.T) {
+	options := &config.Options{
+		MCPServers: map[string]mcp.ServerConfig{
+			"edge": &mcp.StdioServerConfig{
+				Command: "echo",
+				Args:    []string{`has "quote"`, `c:\path\to`},
+			},
+		},
+	}
+
+	args := BuildExecArgs("test", options)
+
+	require.Contains(t, args, `mcp_servers.edge.args=["has \"quote\"","c:\\path\\to"]`)
+}
+
+// TestBuildExecArgs_WithMCPServers_StdioArgsEmpty ensures no args= override is
+// emitted at all when Args is empty — emitting args="" would make codex see a
+// scalar empty string instead of an empty Vec<String>.
+func TestBuildExecArgs_WithMCPServers_StdioArgsEmpty(t *testing.T) {
+	options := &config.Options{
+		MCPServers: map[string]mcp.ServerConfig{
+			"no-args": &mcp.StdioServerConfig{
+				Command: "my-cmd",
+			},
+		},
+	}
+
+	args := BuildExecArgs("test", options)
+
+	for _, a := range args {
+		require.NotContains(t, a, "mcp_servers.no-args.args",
+			"Empty Args must not produce any args= override")
+	}
+}
+
 // TestBuildExecArgs_WithMCPServers_SDKSkipped tests that SDK servers are skipped.
 func TestBuildExecArgs_WithMCPServers_SDKSkipped(t *testing.T) {
 	options := &config.Options{

internal/cli/command.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"sort"
 	"strconv"
+	"strings"
 
 	"github.com/ethpandaops/codex-agent-sdk-go/internal/config"
 	"github.com/ethpandaops/codex-agent-sdk-go/internal/mcp"
@@ -65,8 +66,18 @@ func serializeMCPServerConfigArgs(servers map[string]mcp.ServerConfig) []string
 			args = append(args, "-c", fmt.Sprintf("%s.type=stdio", prefix))
 			args = append(args, "-c", fmt.Sprintf("%s.command=%s", prefix, c.Command))
 
-			for _, arg := range c.Args {
-				args = append(args, "-c", fmt.Sprintf("%s.args=%s", prefix, arg))
+			// Args must be emitted as a single TOML inline array override.
+			// Repeated scalar `-c key=val` overrides replace (not append) at
+			// the same dotted path, which would leave `args` as a string and
+			// fail codex config validation (`expected a sequence`).
+			if len(c.Args) > 0 {
+				quoted := make([]string, len(c.Args))
+				for i, a := range c.Args {
+					quoted[i] = strconv.Quote(a)
+				}
+
+				args = append(args, "-c",
+					fmt.Sprintf("%s.args=[%s]", prefix, strings.Join(quoted, ",")))
 			}
 
 			envKeys := make([]string, 0, len(c.Env))