Skip to content

Tasks: io.modelcontextprotocol/tasks extension on modern servers (era fork)#1731

Merged
cliffhall merged 10 commits into
v2/mainfrom
v2/tasks-extension-modern-1631
Jul 21, 2026
Merged

Tasks: io.modelcontextprotocol/tasks extension on modern servers (era fork)#1731
cliffhall merged 10 commits into
v2/mainfrom
v2/tasks-extension-modern-1631

Conversation

@cliffhall

@cliffhall cliffhall commented Jul 21, 2026

Copy link
Copy Markdown
Member

Closes #1631

Forks the Tasks tab by protocol era (SDK V2 + New Spec, card 8/10). The legacy 2025-11-25 core-tasks flow is preserved; a new io.modelcontextprotocol/tasks (SEP-2663) implementation is added for modern (2026-07-28) connections and gated on the extension being negotiated.

The SDK reality this works around

SDK v2 didn't just remove the tasks helpers — it actively blocks the extension on a modern connection in three places:

  1. Outbound: tasks/get / tasks/update / tasks/cancel are spec-method names absent from the 2026-07-28 era, so client.request(...) throws MethodNotSupportedByProtocolVersion before anything hits the wire.
  2. Inbound (result): the modern codec only accepts resultType: "complete" | "input_required" — a resultType: "task" CreateTaskResult is rejected outright, and the error carries no payload.
  3. Server side: the SDK's modern leg (createMcpHandler) era-gates inbound tasks/* too, so a conformant SDK-v2 server answers them -32601 — the extension can't even be served through the SDK.

So the Inspector implements the extension itself (as the issue anticipated), without patching the SDK:

  • Task creation rides tools/call (a real modern method). The resultType: "task" response is rewritten at the transport into a benign CallToolResult carrying the real DetailedTask in _metaafter trackResponse logs the true frame, so the Protocol/Network tabs still show resultType: "task". pollTaskToolCall reads the handle from _meta and drives the poll.
  • tasks/get / tasks/update / tasks/cancel go over a raw-wire request channel that bypasses the era gate: it mints a string JSON-RPC id, stamps the full modern envelope (protocolVersion / clientInfo / clientCapabilities), sends straight through the transport (still logged), and is consumed by the transport before the SDK Client sees the response.
  • input_required tasks reuse the existing pending-request modal via fulfilInputRequests, answered with tasks/update. No tasks/list, no blocking tasks/result — the completed task inlines its result.

Changes

  • Core: modernTaskSchemas.ts (explicit SEP-2663 wire schemas + normalize/handle helpers); era-forked getRequestorTask / cancelRequestorTask, new updateRequestorTask, era-forked pollTaskToolCall; the transport-rewrite + raw-wire channel (messageTrackingTransport.ts gains rewrite/consume hooks); advertise the tasks extension in client capabilities so every modern request declares it (enables server-directed creation).
  • State: ManagedRequestorTasksState.refresh() is era-aware — legacy paginates tasks/list; modern re-polls the handles the client already holds (no server list).
  • Tab gating: Tasks tab shows on capabilities.tasks (legacy) OR the negotiated extension (modern).
  • Tools screen: on modern, "Run as task" is offered for any tool (task creation is server-directed), threaded via a modernTasks prop.
  • Test server: modern-tasks.ts — a shared poll-driven runtime + modern_task / modern_input_task tools, served via an Express interceptor ahead of the SDK handler (the SDK can't). New configs tasks-modern-http.json / tasks-legacy-http.json.

Verification

  • New integration suite (inspectorClient-tasks-era.test.ts) drives both eras against real servers: create → poll → input_required → tasks/update → inline result, unsolicited-handle passthrough, unknown-id error, modern store re-poll, and the legacy tasks/list flow. Unit coverage for modernTaskSchemas and the raw-wire channel (transport-null, send-failure, timeout, error response, teardown).
  • Driven end-to-end in the web UI (remote proxy) on both eras — see pr-screenshots/ for the modern create/poll/inline-result, the modern input_required elicitation → tasks/update, and the legacy tasks/list/tasks/result flow.
  • npm run ci green.

Follow-ups (addressed in this PR)

  • Accurate input_required note. A modern task round now carries a distinct task-input-required pending-request origin, so the modal note reads "your answer is submitted via a tasks/update request (SEP-2663), not a retry" — instead of the MRTR "retry the original request" copy.
  • Ordinary call path auto-polls unsolicited handles. On a modern connection, a task handle returned from a non-run-as-task tools/call is now detected (in the rewritten result's _meta) and polled to completion by callTool, so the call resolves to the task's final result and the Tasks tab tracks it — matching the run-as-task path. The two poll loops share extracted helpers.

Bug fixes (post-review, 7a8d791) — found in manual testing

  • Uncancelable input_required loop. A modern task paused at input_required blocked its poll loop on the pending-request modal, so cancelRequestorTask sent tasks/cancel but the modal kept re-prompting (reproducible with the new never-completing modern_loop_task). Each modern poll loop now registers a per-task AbortController (registerTaskInputAbort); cancelRequestorTask aborts it to reject the pending elicitation and unblock the poll, and the embedded input requests are tagged with their owning task id (RELATED_TASK_META_KEY) so the modal's Cancel cancels the task rather than sending a "cancel" answer the server would re-prompt on.
  • Cancel wasn't sticky. On the legacy server, cancelling briefly showed CANCELLED then flipped to COMPLETED when the in-flight tools/call resolved. ManagedRequestorTasksState now remembers cancelled task ids and ignores any later non-cancelled status update, so a deliberate cancel wins.
  • Also: compact task id moved to its own line in the embedded monitoring sidebar so the status badge / Cancel control don't wrap; duplicated poll-interval logic DRY'd into a shared taskPollInterval helper.

Note on SDK legacy

Confirmed there is no SDK tasks runtime to reuse: the only -legacy package is @modelcontextprotocol/server-legacy (frozen SSE transport + OAuth AS helpers, zero task exports), and no TaskStore / callToolStream / task-handler registration exists anywhere in the SDK — only the deprecated wire schemas (kept "for interoperability only"), which we reuse for the legacy path. The tasks runtime was deleted, not relocated, and there's no ext-tasks package installed.

Smoke test (web UI, remote proxy — proof of functionality)

Driven end-to-end against the real modern + legacy test servers, screenshots freshly recaptured after the review fixes.

Modern — connected (MCP 2026-07-28), Tasks tab gated on the negotiated extension:

Modern connected

Modern — modern_task run as task: CreateTaskResulttasks/get poll → inline result (no tasks/list / tasks/result):

Modern task completed

Modern — modern_input_task at input_required: embedded elicitation surfaced with the accurate "submitted via a tasks/update request (SEP-2663), not a retry" note:

Modern input_required

Modern — after answering: tasks/update submitted, task completes with the input echoed:

Modern input completed

Modern — bug fix (7a8d791): modern_input_task never-completing loop (modern_loop_task) is now cancelable — clicking the pending-request modal's Cancel closes the modal and settles the task cancelled (previously it re-prompted forever):

Modern loop cancelled

Legacy (2025-11-25, unchanged) — capabilities.tasks + tasks/list/tasks/result. (The Logs tab also shows here because this server advertises logging; that tab is gated on capabilities.logging and is era-agnostic as of #1629 — modern servers supporting logging get it too, with the per-request opt-in control.)

Legacy run-as-task

Legacy — bug fix (7a8d791): cancelling a task via the Cancel Task link now stays cancelled — a late completion of the in-flight call no longer flips it back to completed (shown after a 6s wait):

Legacy cancel sticky

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

cliffhall and others added 5 commits July 20, 2026 20:21
Core + test-server implementation of the SEP-2663 tasks extension for modern
(2026-07-28) connections, forked from the legacy 2025-11-25 tasks path.

SDK v2 removed all tasks support AND era-gates the tasks/* spec methods out of
the modern era on both the client (outbound) and server (inbound) sides, and its
codec rejects a resultType:"task" result outright. So the Inspector drives the
extension itself: task creation rides tools/call (a modern method) with the
resultType:"task" frame rewritten at the transport to a CallToolResult carrying
the handle in _meta; tasks/get/update/cancel go over a raw-wire request channel
that bypasses the era gate (full modern envelope, string ids, consumed by the
transport). input_required tasks reuse the existing pending-request modal via
fulfilInputRequests, answered with tasks/update. No tasks/list, no tasks/result.

Tab + run-as-task gate on the negotiated extension; the managed task store's
refresh re-polls known handles on modern (no server list). Test server serves
the extension via an Express interceptor (SDK can't) + a shared poll-driven
runtime. Full integration coverage of both eras.

Refs #1631

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
Adds unit coverage for modernTaskSchemas and the raw-wire request channel
(transport-null guard, send failure, timeout, error response, teardown), plus
end-to-end era integration tests (create/poll/input-required/update/cancel,
synchronous passthrough, unknown-id error, modern store re-poll). Fixes the App
and constructor-capabilities tests for the always-on tasks extension.

Refs #1631

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
Document the tasks era-fork test configs in README and the new core/test-server
files in AGENTS.md. Share one modern + one legacy test server across the era
integration block (started lazily, stopped in afterAll) — starting a fresh
Express server per test made connection negotiation flaky under the heavy
concurrent coverage run.

Refs #1631

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
The logging/subscriptions PR screenshots + README were staged-deleted in the
working tree before this branch; restore them from v2/main so this PR doesn't
remove other PRs' artifacts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
On modern (SEP-2663) task creation is server-directed — the client declares the
extension once and any tool may return a task — so the Tools screen now offers
'Run as task' for every tool on a modern connection (threaded via a modernTasks
prop through InspectorView → ToolsScreen → ToolDetailPanel), rather than gating
on the legacy per-tool taskSupport. Verified end-to-end in the web UI (remote
proxy): create → poll tasks/get → input_required elicitation → tasks/update →
inline result, plus the unchanged legacy tasks/list/result flow. Adds proof
screenshots.

Refs #1631

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 21, 2026
…path auto-poll)

1. The pending-request note now distinguishes a modern task round from an MRTR
   round: a new 'task-input-required' PendingRequestOrigin threads through
   fulfilInputRequests → the pending-request modal → MrtrOriginNote, which shows
   'submitted via a tasks/update request (SEP-2663), not a retry' instead of the
   MRTR retry copy.

2. The ordinary (non-run-as-task) modern callTool path now auto-polls an
   unsolicited task handle: attemptToolCall detects the rewritten handle in the
   result _meta and drives it to termination (get → input_required/update →
   inline result), dispatching task events so the Tasks tab + toasts track it and
   the tool result resolves to the task's final result. The poll loop is shared
   with the streaming path via extracted helpers (submitModernTaskInput /
   modernTaskTerminalOutcome / modernTaskPollInterval).

Refs #1631

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 9s —— View job


Review: Tasks extension on modern servers (era fork)

Read the core (inspectorClient.ts, messageTrackingTransport.ts, modernTaskSchemas.ts), the state store, the web-UI gating, and the test server + tests.

Overall this is excellent work — the transport-rewrite + raw-wire approach is a clean way to route an extension the SDK actively blocks, the "log the true frame, hand the SDK a benign one" ordering is exactly right for keeping the Protocol/Network tabs honest, and the shared poll helpers (submitModernTaskInput / modernTaskTerminalOutcome / modernTaskPollInterval) keep the two poll loops from drifting. The comments carry the why well, the v8 ignore annotations are justified, and the raw-wire unit tests hit all the defensive branches (null transport, send-failure, timeout, error response, teardown). No blocking issues found. A few observations below, all non-blocking.

Observations / suggestions

  1. Envelope protocolVersion is hardcoded rather than the negotiated versioncore/mcp/inspectorClient.ts:1799 (withModernTaskEnvelope) stamps PROTOCOL_VERSION_META_KEY: MODERN_PROTOCOL_VERSION ("2026-07-28"), but the transport stamps the MCP-Protocol-Version header from the negotiated version (this.protocolVersion, set from getNegotiatedProtocolVersion()). isModernEra() gates on this.protocolEra === "modern", which is the selected era — not a guarantee the negotiated version string equals the constant. If a future modern-family revision negotiates a different string, the header and envelope protocolVersion would disagree. Using this.protocolVersion ?? MODERN_PROTOCOL_VERSION keeps them in lockstep. Today they match, so this is latent. Fix this →

  2. Modern poll loops don't honor an abort of the tool call. Both pollModernTaskToTermination (inspectorClient.ts:~2990) and the streaming loop in pollTaskToolCall loop on !isTerminalTaskStatus(task.status) without checking requestOptions.signal?.aborted. Disconnect is handled (a pending rawWireRequest is rejected by rejectPendingRawWireRequests, which throws out of the loop), but aborting just the in-flight ordinary tool call won't stop the poll — it keeps polling tasks/get until the task terminates on its own. Worth confirming this matches the legacy behavior / is acceptable; a signal.aborted guard in the while condition (and ideally a tasks/cancel) would tighten it.

  3. A stuck input_required task can re-prompt indefinitely. In the poll loop, submitModernTaskInput runs whenever task.status === "input_required". If a non-conformant server keeps returning input_required with the same inputRequests after tasks/update, the user is re-prompted every poll with no bound (the MRTR path has MRTR_MAX_ROUNDS). Conformant servers advance, so this is edge-case hardening, not a correctness bug — but a round cap would make the debugging tool more forgiving of a misbehaving server.

  4. On modern, taskSupport: "forbidden" is overridden. ToolDetailPanel.tsx:~197: showRunAsTask = serverSupportsTaskToolCalls && (modernTasks || taskSupport !== "forbidden"), and effectiveRunAsTask similarly lets modernTasks && runAsTask win for any tool. So a tool that explicitly declares taskSupport: "forbidden" still shows and can route through the task pipeline on a modern connection. If taskSupport is simply not meaningful in SEP-2663 (task creation is purely server-directed) this is fine and intentional — but it's worth a one-line comment stating that forbidden is deliberately ignored on modern, since the code currently reads as an accidental override.

Things I verified are not problems

  • Runtime sharing on the modern leg: createMcpServer(this.configWithCallback) and the interceptor both ??= configWithCallback.modernTaskRuntime on the same object, so a task created via tools/call is visible to a later tasks/get. ✅
  • String vs. numeric id collision: raw-wire ids are inspector-ext-N; the SDK only mints numeric ids, and consumeRawWireResponse returns false for any id it doesn't own, so normal SDK traffic is untouched. ✅
  • Rewrite/consume ordering: consume-response runs before the codec rewrite, and trackResponse logs the true frame before either — Protocol/Network tabs see the real wire. ✅
  • Legacy path preserved: every era branch falls back to the SDK client.request + deprecated schema. ✅

Todo

  • Read core changes (inspectorClient.ts, messageTrackingTransport.ts, modernTaskSchemas.ts)
  • Read state + web UI changes
  • Read test-server changes
  • Assess tests & coverage
  • Post review
    · branch v2/tasks-extension-modern-1631

…round cap, clarifying comment) + fresh smoke screenshots

Review of #1731:
1. withModernTaskEnvelope now stamps the NEGOTIATED protocol version
   (getProtocolVersion() ?? MODERN_PROTOCOL_VERSION) so the envelope matches the
   MCP-Protocol-Version header instead of a hardcoded constant.
2. A never-completing input_required task is now bounded by MRTR_MAX_ROUNDS in the
   shared submitModernTaskInput (threaded through both poll loops), so a
   non-conformant server can't re-prompt the user forever. New modern_loop_task
   test-server tool + integration test cover the cap.
3. ToolDetailPanel: comment clarifying that per-tool taskSupport:"forbidden" is
   deliberately ignored on modern (task creation is server-directed under SEP-2663).

(The abort-during-poll observation is addressed in the PR reply: the cancel path
uses tasks/cancel and disconnect rejects the in-flight raw request, so the poll
stops; matches legacy.)

Fresh web-UI smoke screenshots (modern connected, task completed, input_required
with the corrected tasks/update note, input completed).

Refs #1631

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the thorough review! Addressed all four observations — three with code, one with reasoning.

1. Envelope protocolVersion hardcoded → fixed. withModernTaskEnvelope now stamps this.getProtocolVersion() ?? MODERN_PROTOCOL_VERSION, so the envelope value tracks the negotiated version in lockstep with the MCP-Protocol-Version header (the constant is now only a defensive fallback, v8 ignore-annotated since the raw channel only runs on a connected modern session). Good catch — that was exactly the latent mismatch you described.

2. Poll loops don't honor a signal abort → confirmed acceptable, left as-is (matches legacy). Walking the real cancel/disconnect paths:

  • User cancel (Cancel button on the tool panel, or "Cancel Task" on the Tasks screen) routes through cancelRequestorTasktasks/cancel (via the toolCallTaskUpdated task-id, for both the streaming and ordinary paths), not a signal abort. The server marks the task cancelled, and the next tasks/get returns cancelled → terminal → the loop exits within one poll interval. So cancel does stop the poll.
  • Disconnect aborts the signal, but it also rejects the in-flight rawWireRequest via rejectPendingRawWireRequests, which throws straight out of the loop.

So the signal-aborted-but-still-polling case doesn't arise in the actual flows, and this matches the legacy pollTaskToolCall loop (which also doesn't check the signal). I did try adding !signal?.aborted to the while condition, but it made disconnect surface a misleading "Task working" terminal error (the guard breaks before the rawWireRequest "Disconnected" rejection lands) — a small regression — so I left the loop mechanics alone. Happy to revisit if you'd prefer an explicit tasks/cancel-on-abort.

3. Stuck input_required can re-prompt indefinitely → fixed. Added a round cap (reusing MRTR_MAX_ROUNDS) in the now-shared submitModernTaskInput, threaded through both poll loops: a task that keeps returning input_required without completing throws Modern task "…" exceeded N input_required rounds without completing instead of looping. Covered by a new modern_loop_task test-server tool (never advances) + an integration test asserting the cap trips.

4. taskSupport: "forbidden" overridden on modern → clarified. Added a comment at ToolDetailPanel.tsx stating the override is deliberate: under SEP-2663 task creation is server-directed (decided per request), so the legacy per-tool taskSupport doesn't gate the affordance — the toggle just declares intent to poll a returned handle.

Also re-verified end-to-end in the web UI after the changes (fresh screenshots in the PR description), including that the input_required note now correctly reads "submitted via a tasks/update request (SEP-2663), not a retry."

npm run ci green.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review — addressed all four observations from the previous review (envelope now uses the negotiated version, input-round cap added with a covering test, forbidden-on-modern comment; abort-during-poll explained). Please take another look.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 1s —— View job


Re-review: Tasks extension — review follow-ups

Re-read the follow-up commit (ada0d32b) against the four observations from the first pass. All four are properly resolved — three in code, one with sound reasoning. No new issues, nothing blocking. Nice work.

Verification

1. Envelope now tracks the negotiated version ✅withModernTaskEnvelope (core/mcp/inspectorClient.ts:1809) stamps this.getProtocolVersion() ?? MODERN_PROTOCOL_VERSION. getProtocolVersion() (:2406) returns this.protocolVersion, which is set from this.client.getNegotiatedProtocolVersion() (:3952) — the same source the transport stamps the MCP-Protocol-Version header from. So header and envelope are now in lockstep, and the constant is a defensive-only fallback (justified v8 ignore). Exactly the latent mismatch closed.

2. Abort-during-poll ✅ (reasoning accepted) — walked the same paths in your reply: user cancel routes through tasks/cancel (terminal within one poll), disconnect rejects the in-flight rawWireRequest and throws out of the loop, and the signal-aborted-but-still-polling case doesn't arise in the real flows. Matches the legacy pollTaskToolCall. Leaving the loop mechanics alone (rather than a signal.aborted guard that regressed the disconnect error copy) is the right call.

3. Input-round cap ✅ — the shared submitModernTaskInput (:2961) now takes/returns a round count and throws once rounds > MRTR_MAX_ROUNDS, threaded through both poll loops (pollModernTaskToTermination :3056, and the streaming loop in pollTaskToolCall :3184). The semantics match the MRTR driver (:2231 — round starts at 1, throws when > 10), so the two paths agree. A conformant task prompts once and exits on the next completed poll; only a non-advancing server hits the cap. Well covered by the new modern_loop_task test-server tool (stays input_required every poll, ignores tasks/update) and the integration test asserting callToolStream rejects with /exceeded \d+ input_required rounds/.

4. forbidden-on-modern ✅ — the comment at ToolDetailPanel.tsx:197 now states plainly that per-tool taskSupport: "forbidden" is deliberately ignored on modern because SEP-2663 task creation is server-directed per request, not declared per tool. That removes the "accidental override" reading.

One minor, non-blocking FYI (pre-existing, not introduced here)

The poll loops call submitModernTaskInput on every iteration where status === "input_required", so a conformant server that stays input_required for a poll or two after tasks/update lands (the "eventually consistent" window the :3189 comment describes) would re-prompt the user and consume a round each time. The test server advances immediately after update, so this never fires in tests, and the new cap bounds the pathological case — but if a real server is briefly slow to advance, the user could see a duplicate prompt. If that ever shows up in practice, de-duping by already-answered inputRequests id (rather than by status alone) would be the fix. Purely a hardening note; not something to change in this PR.

Verdict

The era-fork design remains clean, the follow-ups tightened the two real edges (version lockstep + unbounded re-prompt) without touching the working loop mechanics, and coverage grew with the changes. Looks good to merge. ✅

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the re-review and the merge 👍

On the FYI: agreed — the re-prompt-on-input_required window is real for an eventually-consistent server, and de-duping by already-answered inputRequests id (rather than by status alone) is the right fix, matching the SEP-2663 client SHOULD to dedupe keys across polls. Per your guidance I'll leave it out of this PR (the round cap bounds the pathological case and the flows here advance immediately) and track it as a follow-up hardening item.

No further changes on this pass.

Two bug fixes reported during manual testing of the tasks era-fork (#1631):

1. A modern task paused at `input_required` (e.g. `modern_loop_task`, which
   never advances) could not be cancelled: the poll loop blocked on the
   pending-request modal, so `cancelRequestorTask` sent `tasks/cancel` but the
   modal kept re-prompting. Now each modern poll loop registers a per-task
   AbortController (`registerTaskInputAbort`), `cancelRequestorTask` aborts it
   to reject the pending elicitation and unblock the poll, and the embedded
   input requests are tagged with their owning task id
   (`RELATED_TASK_META_KEY`) so the pending-request UI's Cancel action cancels
   the TASK rather than sending a "cancel" answer the server would re-prompt on.

2. On the legacy server, cancelling a task briefly showed CANCELLED then
   flipped to COMPLETED when the in-flight tool call resolved. Cancellation is
   a deliberate terminal decision, so `ManagedRequestorTasksState` now remembers
   cancelled task ids and ignores any later non-cancelled status update.

Also: move the compact task id onto its own line in the embedded monitoring
sidebar so the status badge / Cancel Task control don't wrap; DRY the duplicated
task poll-interval logic into a shared `taskPollInterval` helper.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

New commit 7a8d7911 since the last approvals — two manual-testing bug fixes:

  1. A modern task paused at input_required (e.g. modern_loop_task) is now cancelable: each poll loop registers a per-task AbortController that cancelRequestorTask aborts to reject the pending elicitation and unblock the poll; embedded input requests are tagged with their owning task id so the modal's Cancel cancels the task.
  2. Sticky cancel: ManagedRequestorTasksState remembers cancelled task ids and ignores later non-cancelled updates so a cancelled task no longer flips to COMPLETED.

Plus the embedded-sidebar TaskCard header fix and a taskPollInterval DRY cleanup. npm run ci is green.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 3m 46s —— View job


Re-review: commit 7a8d7911 — cancelable input_required + sticky cancel

Read the full commit diff and traced the new abort lifecycle, the modal→task cancel routing, the sticky-cancel state changes, and the surrounding wiring (ElicitationCreateMessage.taskId, refresh()/refreshModern, the poll loops). CI is green (run 29841025840).

This is a clean, well-reasoned fix. Both bugs are addressed at the right layer, the abort lifecycle is tidy, and the comments carry the why. Two non-blocking observations below.

What I verified is correct ✅

  • AbortController lifecycle — registered before each poll loop (both pollModernTaskToTermination and the pollTaskToolCall stream), released in finally, and release() only deletes its own entry (so a later poll can't drop it). Disconnect both aborts every controller and clears the map. AbortController.abort() is idempotent, so the cancel-then-disconnect double-abort is safe.
  • Modal Cancel closes immediatelycancelRequestorTask aborts the per-task controller synchronously (before the async tasks/cancel raw-wire request), so wirePendingAbort rejects the pending elicitation and removePendingElicitation fires right away. The integration test confirms the call rejects, tasks/cancel is sent, and getPendingElicitations() is empty — no dangling modal.
  • taskId taggingtagInputRequestsWithTask stamps _meta[RELATED_TASK_META_KEY], which ElicitationCreateMessage already reads into .taskId, so App.onElicitationRespond can route action === "cancel" on a task-input-required origin to cancelRequestorTask instead of sending a "cancel" answer the server would re-prompt on. Correct distinction; Submit/Decline still answer normally.
  • Sticky cancelshouldIgnoreUpdate guards both the taskStatusChange and requestorTaskUpdated handlers, correctly still lets a later cancelled update through (so statusMessage can refine), and clears on disconnect/destroy. Well covered by the two new state tests.
  • taskPollInterval DRY — the three loops now share one helper with the same > 0 guard and justified v8 ignore; behavior is unchanged.

Observations (non-blocking)

  1. Sticky cancel is bypassed by the legacy refresh() (tasks/list) path. The event handlers now honor cancelledTaskIds, but the legacy refresh() rebuilds this.tasks straight from listRequestorTasks() filtering only dismissedTaskIds — not cancelledTaskIds (core/mcp/state/managedRequestorTasksState.ts:~120). Since cancellation is cooperative, a legacy server that completes the task anyway and still lists it as completed would flip a user-cancelled task back to COMPLETED on a manual Refresh — exactly the resurrection the fix prevents on the event path. refreshModern is safe (it routes getRequestorTask through the guarded event handler). Adding && !this.cancelledTaskIds.has(t.taskId) to the legacy page filter would make the two paths consistent. Edge-case (cooperative-cancel server + manual Refresh), so non-blocking. Fix this →

  2. The web crux of fix Progress notifications #1 looks untested at the App level. The new onElicitationRespond branch (a cancel on a task-input-required elicitation routing to cancelRequestorTask rather than respond) is the user-facing half of the fix, but I couldn't find an App.test.tsx case that dispatches a pending elicitation with origin: "task-input-required" + taskId and asserts the modal's Cancel calls cancelRequestorTask (not respond). The integration test covers the client-side abort/unblock, and CI's ≥90 gate is green — but a focused App test would lock in the routing so a future refactor of that if can't silently regress it back to sending a "cancel" answer. Add a test →

Verdict

Both manual-testing bugs are fixed correctly and at the right layer, the abort mechanics are sound, and coverage grew with the state changes. The two notes above are hardening/consistency, not correctness blockers. Looks good. ✅

  • Read the new commit diff
  • Review the per-task AbortController / cancel-unblock logic
  • Review sticky-cancel in ManagedRequestorTasksState
  • Review the RELATED_TASK_META_KEY tagging + modal Cancel wiring
  • Review TaskCard header fix + taskPollInterval DRY
  • Post verdict
    · branch v2/tasks-extension-modern-1631

cliffhall and others added 2 commits July 21, 2026 12:30
Recapture the five tasks flow screenshots against the current build and add
two demonstrating the post-review fixes: modern_loop_task cancelled from the
pending-request modal (uncancelable-loop fix) and a legacy task staying
CANCELLED after the in-flight call resolves (sticky-cancel fix).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
…ng test

Re-review of the cancelable-loop / sticky-cancel fix raised two non-blocking
hardening items:

1. The legacy tasks/list refresh() rebuilt the list from the server, honoring
   only dismissedTaskIds — so a cooperative-cancel server that re-lists a
   user-cancelled task as completed could un-stick the cancel on a manual
   Refresh. The refresh path now pins a cancelledTaskIds task to "cancelled"
   (keeping it visible-as-cancelled, matching the live-merge handlers) rather
   than dropping it. Covered by a new state test.

2. Added an App.test.tsx case locking in the user-facing half of the fix: Cancel
   on a task-input-required elicitation calls cancelRequestorTask (not respond).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the re-review — both observations were fair; addressed in 6145bcdc (npm run ci green).

1. Sticky cancel bypassed by the legacy refresh() (tasks/list) path — good catch, real gap. Fixed in managedRequestorTasksState.ts, with one deliberate deviation from the suggested && !this.cancelledTaskIds.has(t.taskId) filter: rather than dropping the task from the refreshed list, I pin its status to cancelled:

const page = result.tasks
  .filter((t) => !this.dismissedTaskIds.has(t.taskId))
  .map((t) =>
    this.cancelledTaskIds.has(t.taskId)
      ? { ...t, status: "cancelled" as const }
      : t,
  );

Dropping it would make a user-cancelled task vanish on Refresh, whereas the live-merge path (onTaskCancelled + shouldIgnoreUpdate) keeps it in the list as cancelled. Pinning keeps the two paths consistent — the task stays visible and cancelled, and a server that re-lists it completed can't un-stick it. Covered by a new test ("keeps a cancelled task cancelled across a legacy refresh that re-lists it completed").

2. App-level cancel-routing untested — agreed, that's the user-facing half. Added an App.test.tsx case: dispatch a pendingElicitationsChange with a task-input-required elicitation carrying a taskId, click the modal's Cancel, and assert inspectorClient.cancelRequestorTask is called with that id and respond is not — so a future refactor of that if can't silently regress it back to sending a "cancel" answer.

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

Addressed both non-blocking observations from the last re-review in 6145bcdc:

  1. Legacy refresh() (tasks/list) now pins a cancelledTaskIds task to cancelled (kept visible-as-cancelled, matching the live-merge path) instead of letting the server's re-listed completed un-stick it. New covering test.
  2. New App.test.tsx case asserting Cancel on a task-input-required elicitation routes to cancelRequestorTask, not respond.

npm run ci green.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 57s —— View job


Re-review: commit 6145bcdc — sticky cancel on legacy refresh + App cancel-routing test

Read the full 6145bcdc diff and traced both fixes against the observations from the previous re-review. Both are properly resolved, and the deviation you chose on #1 is the better call. No new issues, nothing blocking.

Verification

1. Legacy refresh() sticky-cancel ✅ — and pinning is the right shape. refresh() (core/mcp/state/managedRequestorTasksState.ts:170) now maps a cancelledTaskIds task to { ...t, status: "cancelled" } after the dismissedTaskIds filter, rather than dropping it. This is a deliberate improvement over the && !cancelledTaskIds.has(...) filter I suggested: dropping would make a user-cancelled task vanish on Refresh, whereas the live-merge path (onTaskCancelled + shouldIgnoreUpdate, :75/:94) keeps it visible as cancelled. Pinning makes the two paths agree — the task stays in the list, and a server that re-lists it completed can't un-stick the cancel. cancelledTaskIds is populated by the taskCancelled handler (:100) and cleared on disconnect/destroy (:68/:244), so the pin has the same lifecycle as the guard. refreshModern was already safe (it routes through the guarded getRequestorTask handler), so this closes the last inconsistent path. The new state test drives exactly the resurrection scenario (cancel → server re-lists completed → refresh → still cancelled).

(Minor, non-blocking, pre-existing: the pin keeps the server's statusMessage on the refreshed task rather than the one the onTaskCancelled handler set — so a manual Refresh could replace a "cancelled by user" message with whatever the server returns. Status stays cancelled either way, which is the property under test; the message text is cosmetic. Not worth changing here.)

2. App-level cancel-routing test ✅. The new App.test.tsx case (:1118) dispatches a pendingElicitationsChange with origin: "task-input-required" + taskId: "task-77", waits for the modal, clicks Cancel, and asserts cancelRequestorTask("task-77") fired while respond did not. That locks in the exact branch at App.tsx:4017 — the action === "cancel" && origin === "task-input-required" && taskId guard routing to cancelRequestorTask instead of pending.respond(result) — so a future refactor of that if can't silently regress it back to sending a { action: "cancel" } answer. The test also correctly leaves the Submit/Decline path (which still calls respond) exercised elsewhere.

Verdict

Both non-blocking observations are resolved — one with a design refinement (pin-to-cancelled over drop) that's better than what I suggested, one with the focused App test I asked for. The era-fork design is sound, the cancel semantics are now consistent across the event/legacy-refresh/modern-refresh paths, and coverage grew with each change. Looks good to merge. ✅

  • Read the 6145bcdc diff
  • Verify legacy refresh() sticky-cancel pin
  • Verify the new App.test.tsx cancel-routing test
  • Post verdict
    · branch v2/tasks-extension-modern-1631

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — agreed on the cosmetic statusMessage note: on a manual legacy Refresh the pin keeps the server's message rather than the "cancelled by user" one, but status stays cancelled (the property that matters), so leaving it as-is per your call. Appreciate the thorough re-reviews.

@cliffhall
cliffhall merged commit 2259b1a into v2/main Jul 21, 2026
3 checks passed
@cliffhall
cliffhall deleted the v2/tasks-extension-modern-1631 branch July 21, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tasks: io.modelcontextprotocol/tasks extension on modern servers (era fork)

1 participant