Skip to content

Conversation

@button-chen
Copy link
Contributor

@button-chen button-chen commented Dec 30, 2025

Description

For example, the MCP client has the following code

toolsRequest := mcp.ListToolsRequest{Header: make(http.Header)}
toolsRequest.Header.Set("X-Tenant-ID", "10000")
toolsResult, err := mcpClient.ListTools(ctx, toolsRequest)

The set X-Tenant-ID for the header was not transmitted to the server.

The problem lies in the code shown in the screenshot:

image

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • MCP spec compatibility implementation
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring (no functional changes)
  • Performance improvement
  • Tests only (no functional changes)
  • Other (please describe):

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly

MCP Spec Compliance

  • This PR implements a feature defined in the MCP specification
  • Link to relevant spec section: Link text
  • Implementation follows the specification exactly

Additional Information

Summary by CodeRabbit

  • Bug Fixes

    • Request-level HTTP headers are now propagated across all paginated listing calls, ensuring headers sent with the original request are included on each page fetch and preserved across pagination.
  • Tests

    • Added a test to verify header propagation for listing operations, preventing regressions and ensuring headers are sent on every paginated request.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

Walkthrough

listByPage now accepts an http.Header and forwards it to client.sendRequest; per-page listing helpers pass the originating request's Header. A new test TestListToolsWithHeader verifies header propagation for ListTools.

Changes

Cohort / File(s) Summary
Client pagination header propagation
client/client.go
Added header http.Header parameter to listByPage and used it in client.sendRequest(...). Updated per-page listing helpers (ListResourcesByPage, ListResourceTemplatesByPage, ListPromptsByPage, ListToolsByPage) to pass request.Header into listByPage.
Header propagation test
mcptest/mcptest_test.go
Added TestListToolsWithHeader to assert that ListToolsRequest carries expected headers (e.g., X-Test-Header) through the client to server hooks; added imports (net/http, client package).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

status: needs submitter response

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing the loss of HTTP header information set by the client during pagination operations.
Description check ✅ Passed The description follows the template with a clear bug description, correct type of change selection (bug fix), and completed checklist items, though the author noted that tests were added but the review indicated they don't fully exercise the listByPage path.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c16aef7 and 75605de.

📒 Files selected for processing (1)
  • mcptest/mcptest_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • mcptest/mcptest_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@button-chen button-chen changed the title Add header parameter to listByPage function Fix: fix the issue of the header information set by the client being lost. Dec 31, 2025
@button-chen button-chen changed the title Fix: fix the issue of the header information set by the client being lost. Fix: the header information set by the client being lost. Dec 31, 2025
@button-chen
Copy link
Contributor Author

Please check this PR. Thank you. @ezynda3

@ezynda3
Copy link
Contributor

ezynda3 commented Jan 5, 2026

Hey @button-chen could you add a quick test that does the following?

  1. Creates a request with custom headers
  2. Calls a *ByPage method
  3. Verifies the headers reach the server

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Fix all issues with AI Agents 🤖
In @mcptest/mcptest_test.go:
- Around line 403-405: The call to client.ListTools currently ignores its
returned error; update the test to capture the result (err :=
client.ListTools(context.Background(), req)) and assert no error using
testify/require (e.g., require.NoError(t, err)) so the test fails when ListTools
returns an error; locate the call that constructs req (mcp.ListToolsRequest) and
the client.ListTools invocation to make this change.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
mcptest/mcptest_test.go (1)

1-12: Add missing imports.

The test references http.Header and client.NewStreamableHttpClient but the required packages are not imported, causing compilation failures.

🔎 Proposed fix to add missing imports
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"strings"
 	"testing"
 
+	"github.com/mark3labs/mcp-go/client"
 	"github.com/mark3labs/mcp-go/mcp"
 	"github.com/mark3labs/mcp-go/mcptest"
 	"github.com/mark3labs/mcp-go/server"
 )
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b5b41a and 5dc8471.

📒 Files selected for processing (1)
  • mcptest/mcptest_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing

Files:

  • mcptest/mcptest_test.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go

Files:

  • mcptest/mcptest_test.go
🧠 Learnings (2)
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.

Applied to files:

  • mcptest/mcptest_test.go
🧬 Code graph analysis (1)
mcptest/mcptest_test.go (6)
server/hooks.go (1)
  • Hooks (106-141)
mcp/tools.go (2)
  • ListToolsRequest (18-21)
  • ListToolsResult (25-28)
server/server.go (3)
  • NewMCPServer (379-408)
  • WithToolCapabilities (329-336)
  • WithHooks (312-316)
server/streamable_http.go (1)
  • NewTestStreamableHTTPServer (1409-1413)
mcp/types.go (4)
  • InitializeRequest (467-471)
  • Params (211-211)
  • InitializeParams (473-479)
  • Implementation (580-586)
client/http.go (1)
  • NewStreamableHttpClient (11-22)
🪛 GitHub Actions: golangci-lint
mcptest/mcptest_test.go

[error] 386-386: undefined: client

🪛 GitHub Check: lint
mcptest/mcptest_test.go

[failure] 403-403:
missing type in composite literal (typecheck)


[failure] 403-403:
undefined: http


[failure] 386-386:
undefined: client

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (1)
mcptest/mcptest_test.go (1)

356-402: Test structure looks good.

The test correctly sets up hooks, creates an MCP server with tool capabilities, initializes a streamable HTTP client, and prepares to validate header propagation through the ListTools flow.

Comment on lines 403 to 405
req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {"test-header-value"}}}
client.ListTools(context.Background(), req)
}
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Check the error returned by ListTools.

The call to client.ListTools ignores the returned error, which could cause the test to pass silently if the request fails before the hook can validate the header.

🔎 Proposed fix to add error handling

Per coding guidelines, consider using testify/require:

 	req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {"test-header-value"}}}
-	client.ListTools(context.Background(), req)
+	_, err = client.ListTools(ctx, req)
+	if err != nil {
+		t.Fatalf("Failed to list tools: %v", err)
+	}
 }

Or using testify/require as specified in the coding guidelines:

+	"github.com/stretchr/testify/require"
+
 	req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {"test-header-value"}}}
-	client.ListTools(context.Background(), req)
+	_, err = client.ListTools(ctx, req)
+	require.NoError(t, err, "Failed to list tools")
 }

As per coding guidelines, tests should use testify/require for error assertions.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: lint

[failure] 403-403:
missing type in composite literal (typecheck)


[failure] 403-403:
undefined: http

🤖 Prompt for AI Agents
In @mcptest/mcptest_test.go around lines 403-405, The call to client.ListTools
currently ignores its returned error; update the test to capture the result (err
:= client.ListTools(context.Background(), req)) and assert no error using
testify/require (e.g., require.NoError(t, err)) so the test fails when ListTools
returns an error; locate the call that constructs req (mcp.ListToolsRequest) and
the client.ListTools invocation to make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@button-chen please address

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
mcptest/mcptest_test.go (1)

406-406: Check the error returned by ListTools.

The call to client.ListTools ignores the returned error, which could cause the test to pass silently if the request fails before the hook can validate the header. This was also flagged by the linter and pipeline.

🔎 Proposed fix using testify/require

Per coding guidelines, use testify/require for error assertions:

+import (
+	"github.com/stretchr/testify/require"
+)
+
 	req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {"test-header-value"}}}
-	client.ListTools(context.Background(), req)
+	_, err = client.ListTools(ctx, req)
+	require.NoError(t, err, "Failed to list tools")
 }

As per coding guidelines, tests should use testify/require for error assertions.

🧹 Nitpick comments (2)
mcptest/mcptest_test.go (2)

388-392: Remove unnecessary return statements after t.Fatalf.

The t.Fatalf function already terminates the test, making the subsequent return statements on lines 391 and 396 redundant.

🔎 Proposed fix
 	client, err := client.NewStreamableHttpClient(testServer.URL)
 	if err != nil {
 		t.Fatalf("Create client failed %v", err)
-		return
 	}
 	ctx := context.Background()
 	if err := client.Start(ctx); err != nil {
 		t.Fatalf("Failed to start client: %v", err)
-		return
 	}

Also applies to: 393-397


358-407: Consider using testify/require throughout the test.

While the test correctly validates header propagation, the coding guidelines specify using testify/require for error assertions. The test currently uses t.Fatalf directly, which works but doesn't align with project standards.

Example conversion
+import (
+	"github.com/stretchr/testify/require"
+)
+
 func TestListToolsWithHeader(t *testing.T) {
 	// ... hook setup ...
 	
 	client, err := client.NewStreamableHttpClient(testServer.URL)
-	if err != nil {
-		t.Fatalf("Create client failed %v", err)
-	}
+	require.NoError(t, err, "Create client failed")
+	
 	ctx := context.Background()
-	if err := client.Start(ctx); err != nil {
-		t.Fatalf("Failed to start client: %v", err)
-	}
+	err = client.Start(ctx)
+	require.NoError(t, err, "Failed to start client")
 	
 	_, err = client.Initialize(ctx, initRequest)
-	if err != nil {
-		t.Fatalf("Failed to initialize: %v\n", err)
-	}
+	require.NoError(t, err, "Failed to initialize")

As per coding guidelines, this improves consistency across the test suite.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5dc8471 and aa46634.

📒 Files selected for processing (1)
  • mcptest/mcptest_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing

Files:

  • mcptest/mcptest_test.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go

Files:

  • mcptest/mcptest_test.go
🧠 Learnings (4)
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.

Applied to files:

  • mcptest/mcptest_test.go
🧬 Code graph analysis (1)
mcptest/mcptest_test.go (6)
server/hooks.go (1)
  • Hooks (106-141)
mcp/tools.go (2)
  • ListToolsRequest (18-21)
  • ListToolsResult (25-28)
server/server.go (2)
  • WithToolCapabilities (329-336)
  • WithHooks (312-316)
server/streamable_http.go (1)
  • NewTestStreamableHTTPServer (1409-1413)
mcp/types.go (4)
  • InitializeRequest (467-471)
  • Params (211-211)
  • InitializeParams (473-479)
  • Implementation (580-586)
client/http.go (1)
  • NewStreamableHttpClient (11-22)
🪛 GitHub Actions: golangci-lint
mcptest/mcptest_test.go

[error] 406-406: Error return value of client.ListTools is not checked (errcheck)

🪛 GitHub Check: lint
mcptest/mcptest_test.go

[failure] 406-406:
Error return value of client.ListTools is not checked (errcheck)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (1)
mcptest/mcptest_test.go (1)

6-6: LGTM! Imports are appropriate.

The added imports for net/http and the client package are necessary for the new header propagation test.

Also applies to: 10-10

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Fix all issues with AI Agents 🤖
In @mcptest/mcptest_test.go:
- Around line 388-412: Add the testify/require import
("github.com/stretchr/testify/require") and replace all t.Fatalf error
assertions in this test with require helpers: use require.NoError(t, err, "...")
for client.NewStreamableHttpClient, client.Start, client.Initialize and
client.ListTools error checks, and replace the header comparison t.Fatalf with
require.Equal(t, expectedHeaderValue, gotHeaderValue). Remove any trailing
returns after the former t.Fatalf calls since require will fail the test for
you.
🧹 Nitpick comments (2)
mcptest/mcptest_test.go (2)

406-406: Use the existing ctx variable for consistency.

Line 406 creates a new context with context.Background(), but ctx was already defined on line 393 and used for previous operations. Using the same context throughout the test improves consistency.

🔎 Proposed fix
 	req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {expectedHeaderValue}}}
-	_, err = client.ListTools(context.Background(), req)
+	_, err = client.ListTools(ctx, req)
 	if err != nil {
 		t.Fatalf("Failed to ListTools: %v\n", err)
 	}

391-391: Remove unnecessary return statements after t.Fatalf.

The t.Fatalf function already terminates test execution, making the subsequent return statements unreachable and redundant.

🔎 Proposed fix
 	client, err := client.NewStreamableHttpClient(testServer.URL)
 	if err != nil {
 		t.Fatalf("Create client failed %v", err)
-		return
 	}
 	ctx := context.Background()
 	if err := client.Start(ctx); err != nil {
 		t.Fatalf("Failed to start client: %v", err)
-		return
 	}

Also applies to: 396-396

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa46634 and c16aef7.

📒 Files selected for processing (1)
  • mcptest/mcptest_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing

Files:

  • mcptest/mcptest_test.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go

Files:

  • mcptest/mcptest_test.go
🧠 Learnings (4)
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.

Applied to files:

  • mcptest/mcptest_test.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.

Applied to files:

  • mcptest/mcptest_test.go
🧬 Code graph analysis (1)
mcptest/mcptest_test.go (4)
mcp/tools.go (2)
  • ListToolsRequest (18-21)
  • ListToolsResult (25-28)
server/streamable_http.go (1)
  • NewTestStreamableHTTPServer (1409-1413)
mcp/types.go (3)
  • InitializeRequest (467-471)
  • Params (211-211)
  • InitializeParams (473-479)
client/http.go (1)
  • NewStreamableHttpClient (11-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (2)
mcptest/mcptest_test.go (2)

6-6: LGTM! Necessary imports for header testing.

The net/http and client package imports are correctly added to support HTTP header handling and client initialization in the new test.

Also applies to: 10-10


358-365: Well-structured header verification setup.

The test correctly uses hooks to capture the header value server-side, enabling verification that headers are propagated through the ListTools request path.

Comment on lines 388 to 412
client, err := client.NewStreamableHttpClient(testServer.URL)
if err != nil {
t.Fatalf("Create client failed %v", err)
return
}
ctx := context.Background()
if err := client.Start(ctx); err != nil {
t.Fatalf("Failed to start client: %v", err)
return
}

// Initialize
_, err = client.Initialize(ctx, initRequest)
if err != nil {
t.Fatalf("Failed to initialize: %v\n", err)
}

req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {expectedHeaderValue}}}
_, err = client.ListTools(context.Background(), req)
if err != nil {
t.Fatalf("Failed to ListTools: %v\n", err)
}
if expectedHeaderValue != gotHeaderValue {
t.Fatalf("Expected value is %s, got %s", expectedHeaderValue, gotHeaderValue)
}
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 5, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Replace t.Fatalf with testify/require per coding guidelines.

The test uses t.Fatalf for error assertions, but the coding guidelines explicitly require using testify/require for test files. Additionally, the past review comment and retrieved learnings confirm this requirement.

🔎 Proposed refactor to use testify/require

Add the import:

 import (
 	"context"
 	"fmt"
 	"net/http"
 	"strings"
 	"testing"
 
 	"github.com/mark3labs/mcp-go/client"
 	"github.com/mark3labs/mcp-go/mcp"
 	"github.com/mark3labs/mcp-go/mcptest"
 	"github.com/mark3labs/mcp-go/server"
+	"github.com/stretchr/testify/require"
 )

Then update the error handling:

 	client, err := client.NewStreamableHttpClient(testServer.URL)
-	if err != nil {
-		t.Fatalf("Create client failed %v", err)
-		return
-	}
+	require.NoError(t, err, "Create client failed")
+
 	ctx := context.Background()
-	if err := client.Start(ctx); err != nil {
-		t.Fatalf("Failed to start client: %v", err)
-		return
-	}
+	err = client.Start(ctx)
+	require.NoError(t, err, "Failed to start client")
 
 	// Initialize
 	_, err = client.Initialize(ctx, initRequest)
-	if err != nil {
-		t.Fatalf("Failed to initialize: %v\n", err)
-	}
+	require.NoError(t, err, "Failed to initialize")
 
 	req := mcp.ListToolsRequest{Header: http.Header{"X-Test-Header": {expectedHeaderValue}}}
 	_, err = client.ListTools(context.Background(), req)
-	if err != nil {
-		t.Fatalf("Failed to ListTools: %v\n", err)
-	}
-	if expectedHeaderValue != gotHeaderValue {
-		t.Fatalf("Expected value is %s, got %s", expectedHeaderValue, gotHeaderValue)
-	}
+	require.NoError(t, err, "Failed to ListTools")
+	require.Equal(t, expectedHeaderValue, gotHeaderValue, "Header value mismatch")
 }

As per coding guidelines, test files should use testify/require.

🤖 Prompt for AI Agents
In @mcptest/mcptest_test.go around lines 388-412, Add the testify/require import
("github.com/stretchr/testify/require") and replace all t.Fatalf error
assertions in this test with require helpers: use require.NoError(t, err, "...")
for client.NewStreamableHttpClient, client.Start, client.Initialize and
client.ListTools error checks, and replace the header comparison t.Fatalf with
require.Equal(t, expectedHeaderValue, gotHeaderValue). Remove any trailing
returns after the former t.Fatalf calls since require will fail the test for
you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@button-chen please address

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@button-chen
Copy link
Contributor Author

Hey @button-chen could you add a quick test that does the following?

  1. Creates a request with custom headers
  2. Calls a *ByPage method
  3. Verifies the headers reach the server

Bro, I've already added the unit tests @ezynda3

@ezynda3
Copy link
Contributor

ezynda3 commented Jan 6, 2026

Hey @button-chen could you add a quick test that does the following?

  1. Creates a request with custom headers
  2. Calls a *ByPage method
  3. Verifies the headers reach the server

Bro, I've already added the unit tests @ezynda3

The test doesn't test listByPage at all that I can see. @coderabbitai, please double check.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the test file to examine what method is being called
cat mcptest/mcptest_test.go

Length of output: 11089


@ezynda3, you're correct. The test TestListToolsWithHeader calls client.ListTools(), which is not a *ByPage method. It does not test the listByPage function that was fixed in this PR.

The test should call one of these methods instead:

  • client.ListToolsByPage()
  • client.ListResourcesByPage()
  • client.ListResourceTemplatesByPage()
  • client.ListPromptsByPage()

These *ByPage methods are the ones that use the listByPage helper function where the header propagation fix was applied. The current test verifies headers work with the regular ListTools method, but that uses a different code path that wasn't modified by this PR.

To properly test the fix, the test needs to:

  1. ✅ Create a request with custom headers (already done)
  2. ❌ Call a *ByPage method (currently calls ListTools instead)
  3. ✅ Verify headers reach the server (hook mechanism is good)

@button-chen
Copy link
Contributor Author

Hey @button-chen could you add a quick test that does the following?

  1. Creates a request with custom headers
  2. Calls a *ByPage method
  3. Verifies the headers reach the server

Bro, I've already added the unit tests @ezynda3

The test doesn't test listByPage at all that I can see. @coderabbitai, please double check.

Hey @button-chen could you add a quick test that does the following?

  1. Creates a request with custom headers
  2. Calls a *ByPage method
  3. Verifies the headers reach the server

Bro, I've already added the unit tests @ezynda3

The test doesn't test listByPage at all that I can see. @coderabbitai, please double check.

Yes, I just made the correction.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the current state of the test file to verify the correction
cat mcptest/mcptest_test.go

Length of output: 11093


@button-chen, confirmed! ✅ The test now correctly calls client.ListToolsByPage() instead of client.ListTools(). This properly exercises the listByPage code path where your header propagation fix was applied.

The test now satisfies all three requirements:

  1. ✅ Creates a request with custom headers (X-Test-Header)
  2. ✅ Calls a *ByPage method (ListToolsByPage)
  3. ✅ Verifies headers reach the server (via the AddAfterListTools hook)

The fix looks good and is now properly tested. Thank you for making the correction!

 /\_/\
( ^.^ ) _
 > ^ <  /
/|   |\

(| |)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants