-
Notifications
You must be signed in to change notification settings - Fork 380
Fixed Translate streamablehttp #729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
PR Testing Summary:
Testing Steps Performed:
|
madhav165
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added sse and mcp servers from translate and both worked from gateway.
- Connect to gateway
- Tools work
Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com>
Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com>
b59c10c to
adc9575
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ PR #729 Review Complete
The PR successfully fixes the Streamable HTTP support in the translate feature.
Code Review
- The changes correctly implement request/response forwarding between Streamable HTTP and stdio servers
- Proper use of shared pubsub for both SSE and Streamable HTTP protocols
- Clean implementation with good error handling and timeout management
Testing Results
✅ Rebased against main branch✅ Linting - All checks pass (flake8, pylint)✅ Doctests - 469 passed✅ Unit tests - 1646 passed, translate-specific tests working
translate.py)✅ Manual testing - Both single and multi-protocol servers work correctly:
- Streamable HTTP /mcp endpoint handles initialize, tools/list, and tools/call
- Multi-protocol server successfully exposes both SSE and Streamable HTTP endpoints
- Proper JSON-RPC request/response correlation
The implementation properly addresses issue #728 and enables full MCP compliance for both SSE and Streamable HTTP modes. The PR is ready for merge.
Manual testing
pip install .
python3 -m mcpgateway.translate --stdio "npx -y kubernetes-mcp-server@latest" --port 8105 --expose-streamable-http
Works in gateway
- Replace generic Exception with specific json.JSONDecodeError and ValueError - Addresses bandit security scan warning about try/except/continue pattern Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
- Update name-tests-test hook to exclude files in helpers/ directories - Prevents false positives for helper modules like trace_generator.py - Keeps the test naming convention check for actual test files Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
* Fixed streamable http support for translating stdio servers Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> * minor changes Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> * Fix bandit B112 warning: Use specific exception types - Replace generic Exception with specific json.JSONDecodeError and ValueError - Addresses bandit security scan warning about try/except/continue pattern Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * Fix pre-commit hook to exclude test helper files - Update name-tests-test hook to exclude files in helpers/ directories - Prevents false positives for helper modules like trace_generator.py - Keeps the test naming convention check for actual test files Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
* Fixed streamable http support for translating stdio servers Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> * minor changes Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> * Fix bandit B112 warning: Use specific exception types - Replace generic Exception with specific json.JSONDecodeError and ValueError - Addresses bandit security scan warning about try/except/continue pattern Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * Fix pre-commit hook to exclude test helper files - Update name-tests-test hook to exclude files in helpers/ directories - Prevents false positives for helper modules like trace_generator.py - Keeps the test naming convention check for actual test files Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
* Fixed streamable http support for translating stdio servers Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> * minor changes Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> * Fix bandit B112 warning: Use specific exception types - Replace generic Exception with specific json.JSONDecodeError and ValueError - Addresses bandit security scan warning about try/except/continue pattern Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * Fix pre-commit hook to exclude test helper files - Update name-tests-test hook to exclude files in helpers/ directories - Prevents false positives for helper modules like trace_generator.py - Keeps the test naming convention check for actual test files Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
🐛 Bug-fix PR
📌 Summary
Closes #728
In Translate feature (stdio server to streamable-http server):
Implemented a fix that forwards requests/responses from the Streamable HTTP server to the stdio server using pub/sub, enabling full support for the connection.
🐞 Root Cause
The root cause was that responses from the Streamable HTTP server were not being forwarded to the stdio server, resulting in incomplete communication. Without this forwarding, the connection was established via StreamableHTTPSessionManager but subsequent interactions such as listing tools and handling tool calls failed.
💡 Fix Description
Previously, the multi-protocol server only implemented SSE (/message) and lacked proper handling for the Streamable HTTP protocol’s /mcp POST endpoint. As a result, HTTP clients could not send JSON-RPC requests/notifications or receive responses per the MCP spec.
This fix:
This ensures full compliance with the MCP specification for both SSE and Streamable HTTP modes.
Testing 🛠️
🧪 Verification
make lintmake testmake coverage📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)