Skip to content

Conversation

@cx215133873
Copy link

@cx215133873 cx215133873 commented Oct 31, 2025

Description

Fixes #627 (if applicable)

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
    • Improved header handling in server-sent events transport to ensure incoming request headers are properly propagated to outgoing requests when not already configured.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

The change adds header propagation to the SSE transport layer by merging incoming request headers into outgoing HTTP requests. Headers are copied from the request if not already present in the transport configuration, occurring before OAuth handling.

Changes

Cohort / File(s) Summary
Header propagation in SSE transport
client/transport/sse.go
Added logic to merge incoming request headers into outgoing HTTP request headers before OAuth handling, copying headers that are not already set by transport configuration

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Area of focus: Verify the header merging logic correctly copies headers from request without overwriting existing transport-configured headers
  • Consideration: Confirm the placement before OAuth handling is the correct execution order for the intended behavior

Possibly related PRs

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 (4 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix(toocall): properly set custom header to ensure correct request handling" clearly describes the main change in the pull request. The title uses the "fix" prefix to indicate a bug fix and explicitly mentions "custom header" and "request handling," which directly aligns with the objective of fixing Client.CallTool's header handling as described in issue #627. The title is concise, readable, and accurately represents the modifications to the transport layer's SSE implementation.
Linked Issues Check ✅ Passed The PR directly addresses the objective from issue #627 by implementing header handling in the SSE transport layer. The code changes add a header merge operation that copies headers from the incoming request into outgoing HTTP request headers, which fulfills the stated requirement to "ensure Client.CallTool correctly handles and sets request headers so requests are processed as intended." The implementation focuses on the transport layer where Client.CallTool requests are processed, making it a proper solution to the linked issue.
Out of Scope Changes Check ✅ Passed All changes in this PR are within scope and directly address the objective from issue #627. The modification to client/transport/sse.go implements header merging logic to handle request headers, which is the specific problem described in the linked issue. The raw summary confirms that changes are limited to header merging with no unrelated modifications to error handling or control flow, ensuring the PR remains focused on fixing the header handling issue.
Description Check ✅ Passed The PR description follows the template structure with the critical sections properly completed: the issue reference (#627) is provided, the bug fix type of change is checked, and code style compliance is marked as completed. However, the Description section itself lacks a concise explanation of what was changed, and several checklist items (self-review, tests, documentation) are left unchecked. Despite these gaps, the description contains the essential elements needed for tracking and categorization, making it mostly complete according to template requirements.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd61ef9 and b777e87.

📒 Files selected for processing (1)
  • client/transport/sse.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:

  • client/transport/sse.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). (2)
  • GitHub Check: test
  • GitHub Check: coverage
🔇 Additional comments (1)
client/transport/sse.go (1)

366-370: ****

The code is correct and type-safe. Verification confirms:

  1. Type compatibility: request.Header is http.Header (map[string][]string), so the direct assignment is type-correct.
  2. No inconsistency with SendNotification: mcp.JSONRPCNotification does not include a Header field, so header merging is not applicable or needed there. The difference is intentional and correct.

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.

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.

bug: Client.CallTool does not handle header.

1 participant