Skip to content

Conversation

AishwaryaKalloli
Copy link
Contributor

Handles message type Exception in lowlevel/server.py _handle_message function.
Right now I am sending a error log if parameter raise_exception=False.
Initital commit, need feedback.

Motivation and Context

Addressing a TODO mentioned in the code, handling one of the unhandled types of message in server.

How Has This Been Tested?

Initial commit, testing pending, need feedback to move ahead.

Breaking Changes

Does not break existing flow. Since the method raises exception it will break the code in case of exception, however it is expected behaviour.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@AishwaryaKalloli
Copy link
Contributor Author

AishwaryaKalloli commented May 22, 2025

@Kludex I came across TODO mentioned in src/mcp/lowlevel/server.py by you while reading the code.

Would appreciate feedback on how to handle the exception. Since there is no request attached to the message I have sent a log notification response to the client with the error message.
Please let me know if it looks good.

@felixweinberger felixweinberger self-assigned this Jul 15, 2025
@felixweinberger felixweinberger self-requested a review July 15, 2025 12:14
felixweinberger added a commit to AishwaryaKalloli/python-sdk that referenced this pull request Jul 15, 2025
- Handle exceptions in match statement as suggested by TODO
- Use proper dict structure for error logging instead of ErrorData
- Add comprehensive tests with parameterization
- Include exception type, module, and args in log data

Reported-by: AishwaryaKalloli
Github-Issue: modelcontextprotocol#786
felixweinberger added a commit that referenced this pull request Jul 15, 2025
- Handle exceptions in match statement as suggested by TODO
- Use proper dict structure for error logging instead of ErrorData
- Add comprehensive tests with parameterization
- Include exception type, module, and args in log data

Reported-by: AishwaryaKalloli
Github-Issue: #786
@felixweinberger
Copy link
Contributor

felixweinberger commented Jul 15, 2025

@AishwaryaKalloli thanks for your contribution!

Made some tweaks on top of your branch + rebased + added some test cases for exception handling. I don't think we should be using JSONRPC types for log messages, so changed this to a literal dict.

@felixweinberger felixweinberger requested review from Kludex and ihrpr and removed request for felixweinberger July 15, 2025 13:48
gspencergoog pushed a commit to gspencergoog/mcp-python-sdk that referenced this pull request Jul 29, 2025
…-h5-and-h6-styles-globally

Apply `h5` and `h6` styles to all content
@felixweinberger felixweinberger requested a review from a team August 21, 2025 14:44
AishwaryaKalloli and others added 4 commits August 21, 2025 15:46
- Handle exceptions in match statement as suggested by TODO
- Use proper dict structure for error logging instead of ErrorData
- Add comprehensive tests with parameterization
- Include exception type, module, and args in log data

Reported-by: AishwaryaKalloli
Github-Issue: modelcontextprotocol#786
Changed exception data format to match Python's __exit__ format:
- exception_type: The exception class name
- exception_value: The exception message/value
- exception_traceback: The traceback (set to None for now)

Updated tests to verify the new data format.
Added proper type hints for parametrized test function parameters to resolve pyright errors.
Use modern type[Exception] annotation instead of deprecated typing.Type.
@felixweinberger
Copy link
Contributor

@Kludex ready for another look

@felixweinberger felixweinberger added the needs sync Needs sync with latest main branch to ensure CI passes label Sep 8, 2025
@felixweinberger felixweinberger removed the needs sync Needs sync with latest main branch to ensure CI passes label Sep 8, 2025
Send generic "Internal Server Error" message to clients instead of
exposing exception types, values, and tracebacks. This prevents
leaking sensitive implementation details while still logging full
exception information server-side for debugging.
@felixweinberger felixweinberger added the needs more eyes Needs alignment among maintainers whether this is something we want to add label Sep 16, 2025
@felixweinberger felixweinberger added needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention enhancement New feature or request and removed needs more eyes Needs alignment among maintainers whether this is something we want to add labels Sep 30, 2025
@felixweinberger felixweinberger removed the needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention label Sep 30, 2025
@felixweinberger felixweinberger requested review from Kludex and removed request for Kludex and ihrpr September 30, 2025 13:10
@felixweinberger felixweinberger added the needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention label Sep 30, 2025
@felixweinberger felixweinberger added needs more eyes Needs alignment among maintainers whether this is something we want to add and removed needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention labels Sep 30, 2025
@felixweinberger
Copy link
Contributor

@Kludex quick stamp?

@felixweinberger felixweinberger requested review from Kludex and removed request for Kludex October 13, 2025 13:25
@felixweinberger felixweinberger merged commit b4e50aa into modelcontextprotocol:main Oct 13, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request needs more eyes Needs alignment among maintainers whether this is something we want to add

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants