Fixes #20018: prevent duplicate Accept headers in MCP Streamable HTTP transport#20553
Fixes #20018: prevent duplicate Accept headers in MCP Streamable HTTP transport#20553ravencore06 wants to merge 4 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the MCP Streamable HTTP transport that prevented connections to strict MCP servers. The problem stemmed from the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of duplicate Accept headers in the MCP Streamable HTTP transport, which was causing connection failures with strict MCP servers. The core change involves consistently lowercasing all header keys before merging, ensuring proper header management. The updated unit tests accurately reflect this new behavior. The changes are well-implemented and directly address the problem described.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/tools/mcp-client.ts (786-815)
The new logic to lowercase all header keys before merging is a robust solution to prevent duplicate headers with different casings. This directly addresses the issue of strict MCP servers failing due to concatenated Accept headers. The subsequent check for an existing accept header and the conditional assignment of a default value based on mcpServerConfig.type ensures that the correct Accept header is always present without overwriting user-defined values.
packages/core/src/tools/mcp-client.test.ts (1573)
The test confirms that the accept header is correctly added for Streamable HTTP transport when not explicitly provided.
packages/core/src/tools/mcp-client.test.ts (1512)
This change correctly updates the test expectation to reflect the lowercased authorization header and the automatically added accept header, aligning with the intended behavior.
packages/core/src/tools/mcp-client.test.ts (1534)
The test correctly verifies that the accept header is automatically added when not explicitly provided, which is crucial for the fix.
packages/core/src/tools/mcp-client.test.ts (1554)
This test ensures that the accept header is correctly set for SSE transport types, which is important for the createTransportRequestInit logic.
packages/core/src/tools/mcp-client.test.ts (1466)
The change from Authorization to authorization in the test expectation correctly reflects the new header processing logic, ensuring consistency with the lowercasing of header keys.
packages/core/src/tools/mcp-client.test.ts (1595)
This test correctly asserts the lowercasing of the authorization header and the inclusion of the default accept header.
packages/core/src/tools/mcp-client.test.ts (1618)
The test correctly verifies that custom headers like x-api-key are lowercased and the default accept header is added.
packages/core/src/tools/mcp-client.test.ts (1639)
This test correctly asserts the default accept header for HTTP transport.
packages/core/src/tools/mcp-client.test.ts (1802)
The change to x-goog-user-project ensures that the header key is consistently lowercased in the test expectation, matching the new behavior.
packages/core/src/tools/mcp-client.test.ts (1644-1661)
These new tests are crucial for verifying that user-provided Accept headers (regardless of casing) are preserved and not overwritten by the default logic. This ensures flexibility for users while fixing the duplicate header issue.
packages/core/src/tools/mcp-client.test.ts (2070)
The change to authorization in the test expectation is correct, reflecting the lowercasing of header keys.
packages/core/src/tools/mcp-client.test.ts (1864)
The test now correctly expects the x-goog-user-project header to be lowercased, which is consistent with the overall header normalization strategy.
packages/core/src/tools/mcp-client.test.ts (1833)
This update correctly reflects the lowercasing of the X-Goog-User-Project header in the test assertion.
packages/core/src/tools/mcp-client.test.ts (2117)
This update correctly asserts the lowercased authorization header in the test.
packages/core/src/tools/mcp-client.test.ts (1490)
The test now correctly asserts the presence of the accept header with its default value, which is a good validation of the new logic in createTransportRequestInit.
packages/core/src/tools/mcp-client.ts (1722)
Changing Accept to accept here is consistent with the new header normalization strategy implemented in createTransportRequestInit. This ensures that all outgoing headers are consistently lowercased, preventing potential issues with servers that are sensitive to header casing.
run-tests-json.cjs (1-16)
New source code files (.cjs, .mjs) should include the Apache-2.0 license header with the current year, as specified in the Repository Style Guide (lines 76-79).
References
- For all new source code files (
.ts,.tsx,.js), include the Apache-2.0 license header with the current year. (e.g.,Copyright 2026 Google LLC). This is enforced by ESLint. (link)
test-fetch-headers.mjs (1-20)
New source code files (.cjs, .mjs) should include the Apache-2.0 license header with the current year, as specified in the Repository Style Guide (lines 76-79).
References
- For all new source code files (
.ts,.tsx,.js), include the Apache-2.0 license header with the current year. (e.g.,Copyright 2026 Google LLC). This is enforced by ESLint. (link)
test-fetch-identical-headers.mjs (1-20)
New source code files (.cjs, .mjs) should include the Apache-2.0 license header with the current year, as specified in the Repository Style Guide (lines 76-79).
References
- For all new source code files (
.ts,.tsx,.js), include the Apache-2.0 license header with the current year. (e.g.,Copyright 2026 Google LLC). This is enforced by ESLint. (link)
test-mcp-headers.mjs (1-47)
New source code files (.cjs, .mjs) should include the Apache-2.0 license header with the current year, as specified in the Repository Style Guide (lines 76-79).
References
- For all new source code files (
.ts,.tsx,.js), include the Apache-2.0 license header with the current year. (e.g.,Copyright 2026 Google LLC). This is enforced by ESLint. (link)
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
Fixes an issue where the
httpUrltransport failed to connect to strict MCP servers (like Firecrawl) due to a duplicatedAcceptheader. The Gemini CLI and the@modelcontextprotocol/sdkwere both injecting this header with different casings (Acceptvsaccept), which caused fetch to concatenate them into an invalid duplicate string.Changes Made:
- Modified [createTransportRequestInit](cci:1://file:///c:/Users/srini/OneDrive/Documents/GSoC%2026/gemini- cli/packages/core/src/tools/mcp-client.ts:775:0-823:1) to lowercase all CLI-provided and configured headers before merging, enabling the SDK's headers to safely override without creating duplicates.
- Updated unit tests to assert against the new lowercased header keys.