discuss: how to surface ClientOptions on convenience constructors - #891
Open
QuentinBisson wants to merge 1 commit into
Open
discuss: how to surface ClientOptions on convenience constructors#891QuentinBisson wants to merge 1 commit into
QuentinBisson wants to merge 1 commit into
Conversation
|
Connected to Huly®: MCP_G-456 |
Contributor
WalkthroughThis PR adds three new convenience constructors ( ChangesClient Convenience Constructors
🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The existing convenience constructors (NewStdioMCPClient, NewSSEMCPClient, NewStreamableHttpClient) reserve their variadic slot for transport-level options. Client-level options (WithTracer, WithPropagator, WithSession, WithSamplingHandler, …) have no entry point on those constructors and can only be reached by dropping to NewClient + manual transport construction. Add new constructors that take ClientOption as the final variadic parameter; transport options remain available via a leading slice where applicable. Existing constructors are kept verbatim, so this is strictly additive. - NewStdioClient(cmd, env, args, ...ClientOption) - NewSSEClient(url, []transport.ClientOption, ...ClientOption) - NewStreamableHTTPClient(url, []transport.StreamableHTTPCOption, ...ClientOption) NewStreamableHTTPClient preserves the auto-WithSession behaviour from NewStreamableHttpClient when the transport reports an active session ID, so callers swapping over keep the same semantics.
QuentinBisson
force-pushed
the
feat/client-constructor-options
branch
from
May 18, 2026 11:35
34ecaff to
5841ef2
Compare
QuentinBisson
changed the title
QuentinBisson
changed the title
QuentinBisson
marked this pull request as ready for review
May 18, 2026 13:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@client/constructor_options_test.go`:
- Around line 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.
In `@client/http.go`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00120425-29b8-4730-99e2-200bd2f788f8
📒 Files selected for processing (4)
client/constructor_options_test.goclient/http.goclient/sse.goclient/stdio.go
Comment on lines
+13
to
+72
| 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") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+43
to
+46
| clientOpts := opts | ||
| if trans.GetSessionId() != "" { | ||
| clientOpts = append(clientOpts, WithSession()) | ||
| } |
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The convenience client constructors reserve their variadic slot for transport-level options:
Client-level options —
WithTracer,WithPropagator,WithSession,WithSamplingHandler,WithRootsHandler,WithElicitationHandler,WithClientCapabilities— have no entry point on these constructors. Today users have to drop to the lower-levelNewClient(transport, opts...)and reproduce the transport-construction code (notably thestdio.Start(ctx)step the convenience helpers paper over).This PR is open for design discussion before code lands. Below are the alternatives I see; the current commits show Option C as a worked example but I'd rather the maintainer pick.
Option A — modify the existing signatures (breaking)
args ...stringbecomesargs []stringso callers wrap their args in a slice literal. For SSE / streamable-http, transport options move to a leading slice parameter.Option B — method on
*Clientfor post-construction optionsCall sites:
errreturn.Option C — add new constructors alongside the existing ones
The current commits on this branch implement Option C. Tests pass;
NewStreamableHTTPClientpreserves the auto-WithSessionbehaviour from the existing constructor when the transport reports an active session ID.Recommendation
I'd lean Option A — pre-1.0 is the right time, the migration is mechanical, and Option C's duplicate surface bothers me. But I want your call before pushing code in one direction.
Happy to reshape the PR into whichever option you prefer.
Summary by CodeRabbit
New Features
Tests