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
72 changes: 72 additions & 0 deletions client/constructor_options_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package client

import (
"testing"

"github.com/mark3labs/mcp-go/client/transport"
)

// TestNewStdioClient_AppliesClientOptions exercises the new constructor's
// promise: every ClientOption passed in the variadic slot is applied to the
// returned client. WithSession is convenient because it sets a single bool
// observable from the test.
func TestNewStdioClient_AppliesClientOptions(t *testing.T) {
// Use a no-op command for the underlying subprocess; the test only
// cares that NewStdioClient applies the options before returning.
c, err := NewStdioClient("true", nil, nil, WithSession())
if err != nil {
t.Fatalf("NewStdioClient: %v", err)
}
t.Cleanup(func() { _ = c.Close() })
if !c.initialized {
t.Fatalf("WithSession not applied; initialized=false")
}
}

// TestNewSSEClient_AppliesClientOptions verifies that NewSSEClient routes
// the variadic ClientOption arguments through to the constructed client
// while keeping transportOpts separate.
func TestNewSSEClient_AppliesClientOptions(t *testing.T) {
c, err := NewSSEClient(
"http://example.invalid/sse",
[]transport.ClientOption{transport.WithHeaders(map[string]string{"x-test": "1"})},
WithSession(),
)
if err != nil {
t.Fatalf("NewSSEClient: %v", err)
}
if !c.initialized {
t.Fatalf("WithSession not applied; initialized=false")
}
}

// TestNewStreamableHTTPClient_AppliesClientOptions verifies the same routing
// for the streamable-http convenience constructor.
func TestNewStreamableHTTPClient_AppliesClientOptions(t *testing.T) {
c, err := NewStreamableHTTPClient(
"http://example.invalid/mcp",
nil,
WithSession(),
)
if err != nil {
t.Fatalf("NewStreamableHTTPClient: %v", err)
}
if !c.initialized {
t.Fatalf("WithSession not applied; initialized=false")
}
}

// TestNewStreamableHTTPClient_PreservesAutoSession asserts that the
// existing "transport reports an active session ID → set WithSession"
// behaviour from NewStreamableHttpClient is preserved on the new
// constructor. The fixture transport here has no session, so initialized
// reflects only the caller-supplied opts.
func TestNewStreamableHTTPClient_PreservesAutoSession(t *testing.T) {
c, err := NewStreamableHTTPClient("http://example.invalid/mcp", nil)
if err != nil {
t.Fatalf("NewStreamableHTTPClient: %v", err)
}
if c.initialized {
t.Fatalf("initialized=true without an active session; auto-session must not fire")
}
}
Comment on lines +13 to +72

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Align these tests with repository testing conventions.

These tests use t.Fatalf and duplicated per-case functions; the repo guideline requires testify/assert + testify/require and table-driven tests.

As per coding guidelines, "Use testify/assert and testify/require for testing; implement table-driven tests with tests := []struct{ name, ... }."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/constructor_options_test.go` around lines 13 - 72, Convert the four
separate tests into a single table-driven test that iterates over cases for
NewStdioClient, NewSSEClient, NewStreamableHTTPClient and the auto-session case
(referencing the constructors NewStdioClient, NewSSEClient,
NewStreamableHTTPClient), and replace all t.Fatalf checks with testify/require
and testify/assert calls (use require.NoError for constructor errors,
require.NotNil for returned client where appropriate, and
assert.True/assert.False for the initialized checks). Keep cleanup by calling
t.Cleanup(func(){ _ = c.Close() }) per case; for SSE and HTTP constructors
include the transport.ClientOption and transport header case as a test case
input. Ensure the table has fields for name, constructor args, and
expectedInitialized, and run each case with t.Run(name, func(t *testing.T){ ...
}) using require/assert.

26 changes: 26 additions & 0 deletions client/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,29 @@ func NewStreamableHttpClient(baseURL string, options ...transport.StreamableHTTP
}
return NewClient(trans, clientOptions...), nil
}

// NewStreamableHTTPClient creates a new streamable-http-based MCP client
// with the given base URL, applying the provided transport-level options
// when constructing the transport and the provided client-level options
// to the returned client.
//
// Pass transport options (e.g. transport.WithContinuousListening) as a
// slice in transportOpts, and client options (e.g. WithTracer,
// WithPropagator) as the variadic opts. When the transport reports an
// active session ID at construction time, WithSession is appended
// automatically so the returned client skips re-initialisation.
func NewStreamableHTTPClient(
baseURL string,
transportOpts []transport.StreamableHTTPCOption,
opts ...ClientOption,
) (*Client, error) {
trans, err := transport.NewStreamableHTTP(baseURL, transportOpts...)
if err != nil {
return nil, fmt.Errorf("failed to create streamable-http transport: %w", err)
}
clientOpts := opts
if trans.GetSessionId() != "" {
clientOpts = append(clientOpts, WithSession())
}
Comment on lines +43 to +46

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid mutating caller-provided option slices.

Line 43 aliases opts, and Line 45 may append into the same backing array. If the caller passed someOpts..., this can unexpectedly mutate shared state. Copy before appending.

Suggested fix
-	clientOpts := opts
+	clientOpts := append([]ClientOption(nil), opts...)
 	if trans.GetSessionId() != "" {
 		clientOpts = append(clientOpts, WithSession())
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
clientOpts := opts
if trans.GetSessionId() != "" {
clientOpts = append(clientOpts, WithSession())
}
clientOpts := append([]ClientOption(nil), opts...)
if trans.GetSessionId() != "" {
clientOpts = append(clientOpts, WithSession())
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/http.go` around lines 43 - 46, The code aliases the caller-provided
slice opts into clientOpts and then appends WithSession(), which can mutate the
caller's backing array; change clientOpts to be a copy of opts before appending
when trans.GetSessionId() != "" (e.g., allocate a new slice and copy opts into
it, then append WithSession()) so appending the session option does not modify
the original opts slice used by the caller; update the block around clientOpts,
opts, trans.GetSessionId() and WithSession() accordingly.

return NewClient(trans, clientOpts...), nil
}
25 changes: 25 additions & 0 deletions client/sse.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,31 @@ func NewSSEMCPClient(baseURL string, options ...transport.ClientOption) (*Client
return NewClient(sseTransport), nil
}

// NewSSEClient creates a new SSE-based MCP client with the given base URL,
// applying the provided transport-level options when constructing the
// transport and the provided client-level options to the returned client.
//
// Pass transport options (e.g. WithHeaders, WithHTTPClient) as a slice in
// transportOpts, and client options (e.g. WithTracer, WithPropagator) as
// the variadic opts:
//
// c, err := client.NewSSEClient(
// url,
// []transport.ClientOption{client.WithHeaders(h)},
// client.WithTracer(t),
// )
func NewSSEClient(
baseURL string,
transportOpts []transport.ClientOption,
opts ...ClientOption,
) (*Client, error) {
sseTransport, err := transport.NewSSE(baseURL, transportOpts...)
if err != nil {
return nil, fmt.Errorf("failed to create SSE transport: %w", err)
}
return NewClient(sseTransport, opts...), nil
}

// GetEndpoint returns the current endpoint URL for the SSE connection.
//
// Note: This method only works with SSE transport, or it will panic.
Expand Down
24 changes: 24 additions & 0 deletions client/stdio.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,30 @@ func NewStdioMCPClientWithOptions(
return NewClient(stdioTransport), nil
}

// NewStdioClient creates a new stdio-based MCP client that communicates with
// a subprocess. It launches the specified command with the given environment
// variables and arguments, starts the underlying transport, and applies the
// supplied client-level options (e.g. WithTracer, WithPropagator) to the
// returned client.
//
// Callers that need transport-level options (e.g. a custom command function)
// should fall back to NewStdioMCPClientWithOptions or construct the transport
// directly with transport.NewStdioWithOptions and call NewClient.
func NewStdioClient(
command string,
env []string,
args []string,
opts ...ClientOption,
) (*Client, error) {
stdioTransport := transport.NewStdioWithOptions(command, env, args)

if err := stdioTransport.Start(context.Background()); err != nil {
return nil, fmt.Errorf("failed to start stdio transport: %w", err)
}

return NewClient(stdioTransport, opts...), nil
}

// GetStderr returns a reader for the stderr output of the subprocess.
// This can be used to capture error messages or logs from the subprocess.
//
Expand Down
Loading