-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Expose get_session_id callback #2486
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/client/test_streamable_http.pyis excluded by none and included by none
📒 Files selected for processing (1)
src/fastmcp/client/transports.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/fastmcp/client/transports.py (2)
src/fastmcp/server/context.py (1)
session(383-393)src/fastmcp/client/client.py (1)
session(291-298)
⏰ 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). (3)
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
🔇 Additional comments (1)
src/fastmcp/client/transports.py (1)
285-292: Tuple unpack and callback capture look correctUnpacking the third element from
streamablehttp_clientand storing it on the instance gives callers a straightforward way to reach the session-id callback and matches the intended API surface for this transport.
Test Failure AnalysisSummary: The test failure in the Windows CI run is not related to the PR changes. This is a known Windows-specific flaky test issue with pytest-xdist parallel execution. Root Cause: The failing test The error shows a pytest-xdist worker crash: This is accompanied by Windows-specific resource warnings about closed pipes and event loops ( Verification: I tested both the failing test and the new feature:
Suggested Solution: This is a flaky test infrastructure issue, not a code problem. The CI should be re-run. If the Windows test continues to flake, consider one of these approaches:
The PR code itself is solid and ready for merge once the CI passes. Detailed AnalysisWhat the PR Changes
What Failed
Log EvidenceThese are classic Windows asyncio cleanup issues when tests run in parallel with pytest-xdist. Related FilesModified by PR:
Unrelated failing test:
|
jlowin
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.
Thanks!
get_session_id_cbshould be a private variable (_get_session_id_cb) since it is only used to persist state for the duration of a connection- relatedly,
_get_session_id_cbneeds to be reset when the connection is closed. I'm not sure what would happen if the user tried to callget_session_id()after closing a session but I imagine it would raise a low-level error. Better to return None in that case - the private variable should get typing in init of
_get_session_id_cb: Callable[[], str | None] | NoneI believe
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/fastmcp/client/transports.py (2)
299-305: Avoid catching broadExceptioninget_session_idCatching all
Exceptionand returningNonewill silently hide programming or transport errors and conflicts with the guideline to avoid blind exception handling (also flagged by Ruff BLE001). Unless the underlying callback is documented to raise a specific “no session” error, it’s safer to either:
- Rely on the callback returning
str | None, or- Catch only the specific, expected exception type(s) once known.
A simple, clearer version that still guards against “not connected yet” would be:
- def get_session_id(self) -> str | None: - if self._get_session_id_cb: - try: - return self._get_session_id_cb() - except Exception: - return None - return None + def get_session_id(self) -> str | None: + cb = self._get_session_id_cb + if cb is None: + return None + return cb()If the callback can raise a specific “no session available” exception, narrow the
exceptto just that type instead ofException.
307-309:close()behavior is fine; consider adding a return annotationResetting
_get_session_id_cbinclose()avoids stale callbacks after the client is torn down and integrates cleanly withClient.close(). For consistency with the “full type annotations” guideline, you could optionally annotate the return type:- async def close(self): + async def close(self) -> None: # Reset the session id callback self._get_session_id_cb = NoneNot critical, but it would bring this in line with stronger typing elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/client/transports.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Python source code must use Python ≥3.10 with full type annotations
Never use bareexcept- be specific with exception types
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code organization and style
Files:
src/fastmcp/client/transports.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 0
File: :0-0
Timestamp: 2025-12-01T15:48:05.095Z
Learning: PR #2505 in fastmcp adds NEW functionality to get_access_token(): it now first checks request.scope["user"] for the token (which never existed before), then falls back to _sdk_get_access_token() (the only thing the original code did). This is not a reversal of order but entirely new functionality to fix stale token issues.
🧬 Code graph analysis (1)
src/fastmcp/client/transports.py (2)
src/fastmcp/server/context.py (1)
session(383-393)src/fastmcp/client/client.py (2)
session(291-298)close(490-492)
🪛 Ruff (0.14.7)
src/fastmcp/client/transports.py
303-303: Do not catch blind exception: Exception
(BLE001)
⏰ 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: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (3)
src/fastmcp/client/transports.py (3)
9-9: ImportingCallablefromcollections.abcis appropriateUsing
collections.abc.Callablefor type annotations is consistent with modern Python (3.10+) style and fits the existing import pattern in this file.
257-257: Good: initialize_get_session_id_cbin__init__to avoid attribute errorsStoring the session-id callback as
self._get_session_id_cb: Callable[[], str | None] | None = Noneensuresget_session_id()can be safely called before any connection is established and keeps typing explicit.
287-295: Callback wiring fromstreamablehttp_clientlooks correctUnpacking
read_stream, write_stream, get_session_id = transportand assigningself._get_session_id_cb = get_session_idcleanly exposes the underlying session-id callback at the transport level, matching the PR’s goal without altering existing session setup.
|
@jlowin thanks for the feedback! I've updated the code accordingly, let me know if there is anything else you'd like me to change 👍 |
Description
This PR exposes the session id callback from MCP's
streamablehttp_clientas a method inStreamableHTTPTransport. It let's users have easier access tomcp-session-id.Contributors Checklist
Review Checklist