Skip to content

[v2] Fix registerToolTask's getTask and getTaskResult handlers not being invoked#1332

Closed
LucaButBoring wants to merge 11 commits intomodelcontextprotocol:mainfrom
LucaButBoring:fix/registerToolTask
Closed

[v2] Fix registerToolTask's getTask and getTaskResult handlers not being invoked#1332
LucaButBoring wants to merge 11 commits intomodelcontextprotocol:mainfrom
LucaButBoring:fix/registerToolTask

Conversation

@LucaButBoring
Copy link
Contributor

@LucaButBoring LucaButBoring commented Dec 22, 2025

This PR fixes a bug where custom getTask and getTaskResult handlers registered via registerToolTask were never invoked. The Protocol class's task handlers bypassed them entirely and used TaskStore directly. This was a refactoring oversight that was missed due to (1) the existing tests not explicitly checking if those handlers were called, and (2) setTimeout being used in createTask in many tests inadvertently masking the issue.

This also removes the argument-forwarding to getTask and getTaskResult, as that was originally built before the current TaskStore design was finalized, which broke the assumption that the original request would reliably be stored by the implementor. The current TaskStore design allows the Request to be saved, but does not require that, and also exposes no way to directly retrieve it in getTask or getTaskResult (it was possible but no longer intended at the time of the rewrite).

getTask and getTaskResult now only have the extra argument.

Motivation and Context

When using registerToolTask, developers could provide custom getTask and getTaskResult handlers:

mcpServer.experimental.tasks.registerToolTask('test-tool', options, {
    createTask: async (args, extra) => { /* ... */ },
    getTask: async (args, extra) => { /* not called */ },
    getTaskResult: async (args, extra) => { /* not called */ }
});

These handlers were never invoked because:

  1. The Protocol class's tasks/get and tasks/result handlers directly called TaskStore instead of forwarding to the custom handlers.
  2. McpServer's backwards-compat polling wrapper also bypassed the custom handlers
  3. Tests used setTimeout to complete tasks and did not explicitly assert on the handlers being called, inadvertently masking the issue since tasks completed regardless of whether handlers were invoked

How Has This Been Tested?

Updated unit tests with stricter/more robust assertions.

Breaking Changes

Yes, due to args no longer being passed to getTask or getTaskResult.

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

Previously, the code only called the underlying task store, and the tests were not complex enough to validate that the handlers were being called, so they missed this.
They weren't being populated correctly, and can't be without changing the TaskStore interface to require restoring the original request when retrieving a Task.
This removes the setTimeout logic we had in tests, which was masking an issue where the getTask handlers weren't being called.
The appropriate logic has been moved into the getTask handlers themselves.
@changeset-bot
Copy link

changeset-bot bot commented Dec 22, 2025

⚠️ No Changeset found

Latest commit: 6f877f7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 22, 2025

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@1332

@modelcontextprotocol/server

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@1332

@modelcontextprotocol/express

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@1332

@modelcontextprotocol/hono

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@1332

@modelcontextprotocol/node

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@1332

commit: 6f877f7

@LucaButBoring LucaButBoring marked this pull request as ready for review December 22, 2025 21:20
@LucaButBoring LucaButBoring requested a review from a team as a code owner December 22, 2025 21:20
@LucaButBoring LucaButBoring changed the title Fix registerToolTask handlers not being invoked Fix registerToolTask's getTask and getTaskResult handlers not being invoked Dec 22, 2025
@LucaButBoring
Copy link
Contributor Author

Either this or #1315 will need to be rebased after the other is merged.

@KKonstantinov
Copy link
Contributor

hi, is this something to be backported to v1 as well?

@LucaButBoring
Copy link
Contributor Author

@KKonstantinov Yes, I'll open a separate PR for that shortly.

@LucaButBoring LucaButBoring changed the title Fix registerToolTask's getTask and getTaskResult handlers not being invoked [v2] Fix registerToolTask's getTask and getTaskResult handlers not being invoked Dec 23, 2025
@LucaButBoring
Copy link
Contributor Author

Created #1335 against the v1.x branch.

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Hey @LucaButBoring apologies for the delay here. main has moved a lot as we're working on v2 so there are a lot of conflicts now. Do you still have time or want to pursue this? I do think the issue still exists on main even after #1673

I could also give it a shot based on what you described here to see if I can create a fresh solution (with you as co-author) - lmk how you want to proceed.

felixweinberger added a commit that referenced this pull request Mar 25, 2026
These handlers were defined on the ToolTaskHandler interface but never
invoked by the SDK. Three code paths bypassed them and called TaskStore
directly:

- TaskManager.handleGetTask
- TaskManager.handleGetTaskPayload
- McpServer.handleAutomaticTaskPolling

Every test and example implementation was pure boilerplate delegation
to ctx.task.store. Rather than wire them up (which would require
in-memory taskId→tool mapping that doesn't survive restarts or
multi-instance deployments), this removes them.

The TaskStore interface remains the pluggable extension point for
custom task retrieval.

Also adds an isToolTaskHandler type guard and uses it in place of
inline 'createTask' in checks.

BREAKING CHANGE: ToolTaskHandler.getTask and ToolTaskHandler.getTaskResult
have been removed. Delete these methods from your registerToolTask
handlers. If you need custom task retrieval, implement a custom
TaskStore.

Closes #1332

Co-authored-by: Luca Chang <lucalc@amazon.com>
felixweinberger added a commit that referenced this pull request Mar 25, 2026
…actually invoke them

These handlers were defined on the interface but never invoked — three
code paths bypassed them and called TaskStore directly:

- TaskManager.handleGetTask
- TaskManager.handleGetTaskPayload
- McpServer.handleAutomaticTaskPolling

The handlers exist to support proxying external job systems (AWS Step
Functions, CI/CD pipelines) where the external system is the source of
truth for task state. But every test/example implementation was pure
boilerplate delegation to the store.

This change makes the handlers optional: when omitted, TaskStore handles
requests (zero boilerplate, previous de-facto behavior). When provided,
they're invoked for tasks/get, tasks/result, and automatic polling.

Dispatch logic is isolated in ExperimentalMcpServerTasks. Core gains a
single setTaskOverrides() method on TaskManager — no changes to public
TaskManagerOptions type. McpServer itself gains ~5 lines.

Also drops the Args parameter from TaskRequestHandler since tool input
arguments aren't available at tasks/get/tasks/result time.

BREAKING CHANGE: TaskRequestHandler signature changed from (args, ctx)
to (ctx). ToolTaskHandler.getTask and getTaskResult are now optional.

Closes #1332

Co-authored-by: Luca Chang <lucalc@amazon.com>
@felixweinberger
Copy link
Contributor

Here's an attempt: #1764

@LucaButBoring
Copy link
Contributor Author

If you're up for it, we can move this work to your PRs and close this — I don't have too much time until this weekend when I come back from vacation, I can squeeze something in if needed but it might take a bit for me to find the time 😅

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.

3 participants