Skip to content

Pr/protocol overwriting transport hooks #477

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2025
Merged

Pr/protocol overwriting transport hooks #477

merged 1 commit into from
Jun 26, 2025
+25 −1

Conversation

nichtsam
Copy link
Contributor

@nichtsam nichtsam commented May 9, 2025 •
edited
Loading

Motivation and Context

Protocol class currently hooks onto transports by completely overwriting them, which causes the originally hooked functions to be lost and not executed.

For example this code snippet in the README.md wouldn't be executed.
https://github.com/modelcontextprotocol/typescript-sdk/blob/main/README.md?plain=1#L258-L262

How Has This Been Tested?

Breaking Changes

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

@nichtsam nichtsam force-pushed the pr/protocol-overwriting-transport-hooks branch from 2117e7b to 8e8e447 Compare May 13, 2025 21:07
@nichtsam nichtsam mentioned this pull request May 14, 2025
Open
@nichtsam
Copy link
Contributor Author

Just wanted to gently bring this back up. It’s a small fix.
Let me know if anything needs to be changed or discussed!

@nichtsam nichtsam force-pushed the pr/protocol-overwriting-transport-hooks branch 3 times, most recently from 59359b1 to e8931ac Compare May 27, 2025 12:30
@nichtsam nichtsam force-pushed the pr/protocol-overwriting-transport-hooks branch from e8931ac to 8b1a78c Compare June 3, 2025 02:32
@nichtsam nichtsam mentioned this pull request Jun 3, 2025
Open
@nichtsam nichtsam force-pushed the pr/protocol-overwriting-transport-hooks branch from 8b1a78c to 9edb619 Compare June 20, 2025 17:59
@nichtsam
Copy link
Contributor Author

Hi @ihrpr, just bringing this PR back to attention in case it got buried.
Tagging you since I think you might have merge access—please let me know if it should go to someone else!

@ihrpr ihrpr added this to the HPR milestone Jun 20, 2025
ihrpr
ihrpr approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ihrpr ihrpr merged commit 71d309d into modelcontextprotocol:main Jun 26, 2025
2 checks passed
@nichtsam nichtsam deleted the pr/protocol-overwriting-transport-hooks branch June 26, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihrpr ihrpr ihrpr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
HPR
Development

Successfully merging this pull request may close these issues.
2 participants
@nichtsam @ihrpr