Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Nov 18, 2025

Fix SSE transport invalid parameter issue (#3938)

Summary

Fixes #3938 by removing the invalid terminate_on_close parameter from the SSE transport's sse_client() call. This parameter is only valid for streamable HTTP transport (per the MCP SDK documentation), not for SSE transport, and was causing crashes when agents tried to connect to MCP servers using SSE transport.

Changes:

  • Removed terminate_on_close=True from sse_client() call in SSETransport.connect()
  • Added comprehensive unit tests for SSETransport that mock the MCP SDK and verify correct parameters are passed
  • Added error-path tests to ensure Agent._get_native_mcp_tools() raises proper RuntimeError instead of UnboundLocalError when connection fails
  • Added storage isolation to new test modules to prevent interference with other tests

Verification:

  • Confirmed terminate_on_close is only used in HTTP transport (where it's valid) and MCPToolWrapper (also valid)
  • All new tests pass locally (5 SSE transport tests + 2 agent error handling tests)
  • Lint and type checks pass

Review & Testing Checklist for Human

  • Test with actual MCP server using SSE transport - The fix was validated with mocked components. Please verify it works end-to-end with a real SSE MCP server using the reproduction steps from issue [BUG] SSE transport connection passes an invalid argument to MCP's client class #3938 (the provided server.py and agent.py scripts)
  • Verify no regression in SSE transport functionality - Confirm that removing terminate_on_close doesn't break any existing SSE transport use cases beyond fixing the crash
  • Review test mocking approach - The tests mock the MCP SDK's sse_client function. Verify this accurately represents the SDK's behavior and that the test fixtures properly clean up sys.modules

Notes

  • CI Status: One test failed on Python 3.11 (tests/llms/openai/test_openai.py::test_multiple_openai_calls_in_crew with a directory cleanup error). This appears unrelated to the SSE transport changes as it's in the OpenAI test suite and involves a different storage directory. All other CI checks passed (lint, type-checker across all Python versions, CodeQL).
  • Storage Isolation: Added autouse fixtures to isolate CREWAI_STORAGE_DIR in the new test modules as a defensive measure to prevent any potential test interference.
  • MCP SDK Version: The fix assumes MCP SDK >=1.16.0 (the version specified in pyproject.toml) doesn't support terminate_on_close for SSE transport, which aligns with the issue report and SDK documentation.

Link to Devin run: https://app.devin.ai/sessions/d3870992d8df48bb82c99e260c70f9d3
Requested by: João (joao@crewai.com)

Remove terminate_on_close parameter from sse_client call in SSETransport.
This parameter is only valid for streamable HTTP transport, not for SSE transport.

Fixes #3938

Changes:
- Remove terminate_on_close=True from sse_client() call in SSETransport.connect()
- Add comprehensive unit tests for SSETransport that mock sse_client and verify correct parameters
- Add error-path tests to ensure Agent._get_native_mcp_tools raises proper RuntimeError instead of UnboundLocalError when connection fails

Co-Authored-By: João <joao@crewai.com>
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Add autouse fixture to isolate CREWAI_STORAGE_DIR in MCP test modules.
This prevents any potential interference with other tests that use the same storage directory.

Co-Authored-By: João <joao@crewai.com>
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] SSE transport connection passes an invalid argument to MCP's client class

2 participants