[WIP] Migrate to Tasks extension SDK - #2308
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Connection lifecycle, degraded-mode tool calls, legacy timeouts, and bounded task-input handling have unresolved regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Migrates Inspector task handling to the extracted ext-tasks SDK while retaining generation-neutral task APIs.
Changes:
- Adds and externalizes the new Tasks SDK dependency.
- Replaces custom requester/receiver task orchestration with SDK sessions.
- Updates task types and refactors related test coverage.
File summaries
| File | Description |
|---|---|
package.json |
Adds the local Tasks SDK dependency. |
package-lock.json |
Locks the local package. |
core/mcp/types.ts |
Defines neutral Inspector task types. |
core/mcp/modernTaskSchemas.ts |
Re-exports SDK schemas and identifiers. |
core/mcp/messageTrackingTransport.ts |
Simplifies raw-response consumption. |
core/mcp/inspectorClientProtocol.ts |
Updates task API types and capabilities. |
core/mcp/inspectorClientEventTarget.ts |
Migrates task event payload types. |
core/mcp/inspectorClient.ts |
Integrates requester and receiver task sessions. |
clients/web/tsup.runner.config.ts |
Externalizes the Tasks SDK. |
clients/web/src/test/integration/mcp/inspectorClient.test.ts |
Removes superseded task tests. |
clients/web/src/test/integration/mcp/inspectorClient-tasks-era.test.ts |
Removes the task input-round limit test. |
clients/web/src/test/integration/mcp/inspectorClient-modern-era.test.ts |
Accepts the SDK’s revised error wording. |
clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts |
Removes tests for deleted internals. |
clients/web/src/test/core/mcp/modernTaskSchemas.test.ts |
Removes tests for replaced schemas. |
clients/web/src/test/core/mcp/inspectorClient-raw-wire.test.ts |
Adds SDK-boundary and raw-dispatch tests. |
clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts |
Updates session lifecycle tests. |
clients/tui/tsup.config.ts |
Externalizes the Tasks SDK. |
clients/cli/tsup.config.ts |
Externalizes the Tasks SDK. |
Review details
- Files reviewed: 16/18 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Modern task calls currently regress progress delivery, timeout behavior, and protocol-error preservation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/18 changed files
- Comments generated: 3
- Review effort level: Balanced
| "@modelcontextprotocol/client": "2.0.0", | ||
| "@modelcontextprotocol/core": "2.0.0", | ||
| "@modelcontextprotocol/ext-apps": "^1.7.4", | ||
| "@modelcontextprotocol/ext-tasks": "file:../mcp-ext-tasks/packages/ext-tasks", |
There was a problem hiding this comment.
🔵 Needs a closer look
Core protocol lifecycle changes still contain duplicate failure events and a disconnect race, while depending on an unpublished draft SDK.
Review details
- Files reviewed: 17/19 changed files
- Comments generated: 1
- Review effort level: Balanced
| }, | ||
| ); | ||
| await this.closeTaskSession(); | ||
| this.taskSession = createTaskSessionFromClient(client, { |
6a69c33 to
790b421
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new dependency breaks the SDK watch invariant, and concurrent task calls can misattribute progress events.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
package.json:94
- Adding this scoped package makes the nightly SDK watch fail immediately:
scripts/sdk-watch.mjs:250-259rejects every root@modelcontextprotocol/*dependency absent fromSDK_GROUPS, which currently contains only the TypeScript SDK packages andext-apps. Addext-tasksas its own upstream group and update the sdk-watch fixtures/tests so releases of this new dependency are tracked instead of breaking the workflow.
"@modelcontextprotocol/ext-tasks": "file:../mcp-ext-tasks/packages/ext-tasks",
- Files reviewed: 18/20 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Auth recovery can duplicate remote tasks, SDK-watch registration is missing, and the dependency remains an unpublished local path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
package.json:94
- Adding another
@modelcontextprotocol/*dependency without registering it inSDK_GROUPSmakes the nightly SDK watch fail inassertEveryPackageWatched(scripts/sdk-watch.mjs:250-258) before it can check any releases. Add an independent ext-tasks group (it has its own repository/release cadence) and update the watcher tests as part of this dependency addition.
"@modelcontextprotocol/ext-tasks": "file:../mcp-ext-tasks/packages/ext-tasks",
- Files reviewed: 18/20 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Hi @LucaButBoring, could you create an issue for this PR? We manage the milestones via Issues on the project board, so every PR needs an issue. |
|
@cliffhall Opened an issue: #2316 |
There was a problem hiding this comment.
🔵 Needs a closer look
Core task lifecycle paths have unresolved cancellation, timeout, and duplicate-event issues, and the dependency remains unpublished.
Review details
- Files reviewed: 20/22 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
🟡 Changes recommended
It currently has a compilation error, duplicate failure events, a legacy timeout regression, and an unpublished local dependency.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
| @@ -24,6 +61,7 @@ import type { | |||
| ResourceSubscriptionStreamState, | |||
| ExcludedTool, | |||
| RequestMetadata, | |||
| InspectorTask, | |||
There was a problem hiding this comment.
🟡 Changes recommended
Raw modern requests no longer propagate cancellation to stdio or SSE servers.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 21/23 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
| if (signal) { | ||
| onAbort = () => { | ||
| const pending = this.pendingRawWireRequests.get(id); | ||
| if (!pending) return; | ||
| pending.cleanup(); | ||
| reject(abortError(signal)); | ||
| }; | ||
| signal.addEventListener("abort", onAbort, { once: true }); |
Following discussion on Discord, this uses the extracted Tasks implementation (see modelcontextprotocol/ext-tasks#21) to replace the implementation here.
Publishing as a draft for reference and comments; this is unusable until the actual extension SDK is merged and published to NPM (after which point I'll swap the new dependency to not be a local path). I'm keeping this PR in sync with the Tasks extension one, and once the Tasks extension one is merged, this PR can either be merged directly or used as the basis for a separate implementation.