Skip to content
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

Added preservation of request handler params #248

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Kevandrew
Copy link

@Kevandrew Kevandrew commented Apr 1, 2025

fix(protocol): Ensure request params reach handlers and fix progress notifications

Motivation and Context

This PR fixes two critical issues within the base Protocol class (src/shared/protocol.ts):

  1. Loss of Request Parameters: The params object sent by clients in JSONRPCRequest messages was being lost before reaching handlers registered via setRequestHandler. This forced developers(me) to implement workarounds. The root cause was identified as incorrect internal schema handling within the setRequestHandler wrapper logic.
  2. Broken Progress Notifications: An earlier refactoring inadvertently broke the handling of notifications/progress messages, causing onprogress callbacks provided in client.request options to fail.

This fix ensures handlers receive expected params reliably and progress updates function correctly, improving SDK usability and correctness.

How Has This Been Tested?

  • Verified all existing test suites pass locally after the fix (npm test).
  • Confirmed previously failing tests for progress notification handling in src/shared/protocol.test.ts now pass.
  • Added a new test suite (request handler params preservation) to src/shared/protocol.test.ts specifically verifying that params objects are correctly preserved and passed to handlers registered via setRequestHandler. This test failed before the fix and passes with the fix applied.

Breaking Changes

  • No breaking changes. Corrects internal behavior to match expectations.

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 (re: expected protocol behavior)
  • My code follows the repository's style guidelines
  • New and existing tests pass locally (including the new test for param preservation)
  • I have added appropriate error handling (within the fix for parsing errors)
  • I have added tests that prove my fix is effective or that my feature works (the new request handler params preservation test)
  • I have added or updated documentation as needed

Additional context

  • The fix for params loss refines internal Zod schema usage in setRequestHandler/setNotificationHandler to correctly incorporate user-defined params schemas.
  • The fix for progress notifications uses a dedicated parsing path for that specific internal notification type.

First contribution.

Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

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

Sorry, I don't understand the underlying issue that this PR is intended to fix. Can you please provide more details, repro, etc.?

When I run this test without the corresponding protocol.ts changes, I see:

 FAIL  src/shared/protocol.test.ts (6.641 s)
  ● protocol tests › request handler params preservation › should preserve request params when passed to handler via setRequestHandler

    expect(received).toEqual(expected) // deep equality

    - Expected  - 3
    + Received  + 1

    @@ -1,8 +1,6 @@
    - ObjectContaining {
    -   "id": 99,
    -   "jsonrpc": "2.0",
    + Object {
        "method": "test/paramsMethod",
        "params": Object {
          "key1": "value1",
          "key2": 123,
        },

However, the "received" object is what I would expect for a request handler.

@Kevandrew
Copy link
Author

You're right, the simple base test passes. My PR test assertion was also wrong (expecting the full envelope) and I'll fix it.

The issue surfaced downstream in my governance SDK . The pipeline re-validates the request received from the base SDK using the user's original Zod schema.

The original base protocol.ts's implicit parsing (requestSchema.parse(request)) was fragile. Even if it sometimes passed the correct { method, params } subset, slight inconsistencies or schema complexities caused it to sometimes pass params as undefined or unvalidated to our pipeline. Our pipeline's stricter re-validation then failed, blocking the request.

The fix in protocol.ts makes the base SDK's internal parsing robust by explicitly matching the full JSON-RPC structure while embedding the user's params schema. This ensures the data passed downstream (to our pipeline or any handler) is reliably parsed and validated first, preventing the errors we saw.

So, the fix improves base SDK robustness, which directly impacts downstream reliability. I'll correct the PR test assertion to match the expected { method, params } handler input.

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.

2 participants