mcpx: give tasks honest cancellation and error endings (#2499) by dezren39 · Pull Request #3699 · developing-today/code

dezren39 · 2026-11-01T13:21:12Z

Summary

tasks/cancel
  mark cancellation in-memory; invoke existing hooks and context cancel
  if work finishes in 5s, record its own completed/failed status
  otherwise record cancelled with the caller's reason; discard late results

isError results now carry the first text block as statusMessage (512-rune limit), or the specified fallback. Adds protoTasks.cancelGrace (default 5s), store/settings wiring, docs, tests, and retires both conformance gaps. Closes #2499.

Evidence

Before: CancelWithReason persisted cancelled before invoking cancellation hooks; finish discarded any later result. The isError branch set failed without a message.
After: go test ./internal/tasks -count=1 passes; changed task tests pass with -race -shuffle=on -count=5; focused setting/docs tests pass; internal/mcpserver, internal/daemon, and internal/cli compile.
internal/conformance test package currently does not compile on this base because existing test backends implement the older Complete signature; CI should confirm whether that baseline has since been repaired.

Merge Danger

Door: two-way

Blast Radius: tasks

Only task-store terminal-state behavior and its daemon setting change; the existing wire status enum and OnCancel contract remain unchanged.

^{Need help on this PR? Tag @codesmith-bot with what you need.}

coderabbitai · 2026-11-01T13:21:20Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 43 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 0254d48a-2802-4afd-b1ac-2e3eb9f1e210

Commits

Reviewing files that changed from the base of the PR and between f2006aa and f847036.

Files selected for processing: 14

pkgs/mcpx/docs/changes/prototasks-cancel-grace.md
pkgs/mcpx/docs/compendium.manifest.json
pkgs/mcpx/docs/configuration.md
pkgs/mcpx/internal/cli/serve.go
pkgs/mcpx/internal/conformance/coverage_stateful.go
pkgs/mcpx/internal/conformance/harness_test.go
pkgs/mcpx/internal/daemon/ops.go
pkgs/mcpx/internal/mcpserver/tasks.go
pkgs/mcpx/internal/mcpserver/timing.go
pkgs/mcpx/internal/settings/effect.go
pkgs/mcpx/internal/settings/prototasks.go
pkgs/mcpx/internal/settings/prototasks_test.go
pkgs/mcpx/internal/tasks/tasks.go
pkgs/mcpx/internal/tasks/tasks_test.go

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

chatgpt-codex-connector · 2026-11-01T13:21:28Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T04:36:04.934346Z	`f847036`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0c4363614

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-01T13:24:18Z

+		t.cancelTimer = time.AfterFunc(grace, func() {
+			s.expireCancellation(t, reason)
+		})


Preserve immediate cancellation for legacy MCP clients

For clients negotiated to 2025-11-25, mcpserver/tasks.go still handles tasks/cancel by calling this shared store method and returning its snapshot. Replacing the immediate terminal transition with a grace timer now returns working or input_required, and cooperative work can subsequently become completed or failed. That violates the legacy requirements recorded in internal/conformance/requirements.json (tasks-cancel-transition-before-response and tasks-cancelled-sticky): cancellation must transition to cancelled before replying and remain cancelled. Keep the cooperative behavior for the modern extension while preserving the legacy cancellation semantics.

Useful? React with 👍 / 👎.

Fixed in f75de20: the legacy 2025-11-25 tasks/cancel handler now calls the new tasks.Store.CancelImmediate, which records Cancelled under the store lock before the reply (grace timer stopped, StatusMessage set) and returns the terminal snapshot; the work is still notified but its later finish cannot change the ending (finish() drops results for terminal tasks). The cooperative grace window remains only on CancelWithReason (modern extension + daemon paths). Regression test TestCancelImmediateIsTerminalAndSticky covers terminal-and-sticky with a grace window configured; internal/tasks, mcpserver Task|Cancel, and conformance Cancel|Stateful tests pass on the laptop.

dezren39 · 2026-11-02T02:32:31Z

🤖 Agent note · parked (task 5601, lane E resume)

The earlier "Drew-authored hard stop" park premise was wrong: this PR's dezren39 authorship is the fleet's GitHub account, and neither this PR nor issue mcpx: Phase 2 of #2497 — honest task endings: cooperative cancel grace window + isError statusMessage #2499 carries a hard-stop label. This PR is the canonical implementation for task 5601 and the fix-forward path applies.
Codex P1 (finding 4435807111) — disposition: FIX before merge. Legacy clients negotiated to 2025-11-25 must keep immediate, sticky cancelled per internal/conformance/requirements.json (tasks-cancel-transition-before-response, tasks-cancelled-sticky); the cooperative grace window applies only to the modern extension path. Not merging past this.
Both red checks share one root cause (CI run 41312917040): the conformance test stubs (failingReads features_test.go:308/389, relayBackend utilities_test.go:177, *backend harness_test.go:358, transport_test.go:510) implement Complete returning ([]string, error) while mcpserver.Backend wants (mcpserver.Completion, error) — vet fails in lint and the darwin compile gate fails in stress. Resume first verifies whether this is pre-existing on current main.
PR is also CONFLICTING with current main; resume merges main in, applies the two fixes, re-runs targeted tests + CI, then merges.

…ete stub (#2499)

…ment-honest-task-endings # Conflicts: # pkgs/mcpx/internal/settings/effect.go

dezren39 · 2026-11-02T04:22:54Z

Fix-forward for the Codex P1 (finding 4435807111), in f75de20 + merge 7f37ee0:

Legacy cancellation is immediate and sticky again. New tasks.Store.CancelImmediate: records cancelled under the store lock before replying, stops any grace timer, still notifies the work (OnCancel / store hook / context) but the work's later finish cannot change the ending. The legacy 2025-11-25 tasks/cancel handler in mcpserver/tasks.go now calls it; the cooperative grace window (CancelWithReason) remains for the modern extension path and the daemon. Regression test: TestCancelImmediateIsTerminalAndSticky.
Conformance stub root cause fixed. *backend in internal/conformance/harness_test.go implemented Complete returning ([]string, error) while mcpserver.Backend wants (mcpserver.Completion, error) — verified byte-identical on current main (pre-existing); failingReads/relayBackend/transport stubs all embed that backend, so the one signature fix covers them. go vet on conformance/tasks/mcpserver/settings is clean now.
Merged current main (one conflict, settings/effect.go: kept both sides' exemptions; pin 191 = 188 base + 1 cancelGrace + 2 completion).

Verification on the laptop: go test ./internal/tasks -count=1 ok; go test ./internal/mcpserver -run Task|Cancel ok; go test ./internal/conformance -run Cancel|Stateful ok. go test ./internal/settings fails in 5 tests — verified identical failures on clean current main (pre-existing main breakage: undeclared model./proto./script.* settings, env-doc drift, compendium manifest drift); protoTasks.cancelGrace is not among the violations. That main-side breakage is noted for a separate issue, not fixed here.

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f37ee033b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-02T04:25:30Z

+sections: prototasks
+kind: Added
+summary: protoTasks.cancelGrace setting (5s) — cooperative task work may finish inside the cancel grace window instead of being recorded cancelled (#2499)


Register the new change fragment in the compendium manifest

This newly added Markdown file has no entry in docs/compendium.manifest.json. TestEveryDocumentHasAChapter in internal/settings/compendium_test.go scans all Markdown files, including docs/changes, and fails for each unregistered file. Although main already has unrelated manifest drift, this fragment introduces an additional failure that will remain after those baseline issues are fixed. Add a manifest entry, using the existing change fragment's not-compendium role if appropriate.

Useful? React with 👍 / 👎.

Fixed: docs/changes/prototasks-cancel-grace.md is now registered in docs/compendium.manifest.json as a not-compendium docs-history fragment (same shape as the artifact-copy-semantics entry). Re-running TestEveryDocumentHasAChapter, our file no longer appears; the 3 remaining unregistered files (TESTING.md, docs/changes/docs-history-tool.md, docs/changes/research-and-findings-chapter.md) are pre-existing on clean main and tracked by #2846.

…anifest (#2499)

dezren39 · 2026-11-02T04:27:08Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d81a702983

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-02T04:31:48Z


-	if moved {
+	if transitioned {
+		s.changed(id)


Cancel the worker before notifying terminal-state watchers

When a legacy client cancels an ownerless modern task with an active task subscription, this synchronous notification can deadlock before reaching cancel(). The production watcher in mcpserver/listen.go calls detailedNow, which calls Store.Result for terminal tasks and waits for t.done. If the worker is waiting on ctx.Done(), neither side can proceed: the watcher waits for the worker, while cancellation waits for the watcher. Previously, the worker context was cancelled before notifying watchers. Move this notification after the cancellation hooks and context cancellation so the cancellation request and subscribed stream can finish.

Useful? React with 👍 / 👎.

Fixed in f847036: CancelImmediate now invokes the OnCancel hook, the store hook, and the worker context cancellation before s.changed(id) notifies watchers, so a watcher waiting on Store.Result/done cannot deadlock against the pending cancellation. internal/tasks tests pass.

…#2499)

dezren39 · 2026-11-02T04:33:58Z

@codex review

chatgpt-codex-connector · 2026-11-02T04:36:03Z

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: f8470369c2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

dezren39 added 2 commits November 1, 2026 07:20

mcpx: record honest task endings (#2499)

5d5943f

Merge origin/main into queue task 5601

e0c4363

dezren39 requested a review from a team as a code owner November 1, 2026 13:21

dezren39 mentioned this pull request Nov 1, 2026

mcpx: Phase 2 of #2497 — honest task endings: cooperative cancel grace window + isError statusMessage #2499

Closed

10 tasks

chatgpt-codex-connector Bot reviewed Nov 1, 2026

View reviewed changes

docs(mcpx): docs-history fragment for protoTasks.cancelGrace (#2499)

a7c6024

dezren39 mentioned this pull request Nov 1, 2026

mcpx: conformance test package does not compile — test backend Complete returns []string, mcpserver.Backend wants Completion #3710

Closed

github-actions Bot assigned dezren39 Nov 1, 2026

dezren39 mentioned this pull request Nov 1, 2026

mcpx: Phase 3 of #2497 — task audit logging, unbound-model documentation, and gap truth-up #2500

Closed

9 tasks

dezren39 added 2 commits November 1, 2026 22:20

fix(mcpx): legacy tasks/cancel stays immediate; fix conformance Compl…

f75de20

…ete stub (#2499)

Merge remote-tracking branch 'origin/main' into queue/5601-2499-imple…

7f37ee0

…ment-honest-task-endings # Conflicts: # pkgs/mcpx/internal/settings/effect.go

chatgpt-codex-connector Bot reviewed Nov 2, 2026

View reviewed changes

docs(mcpx): register prototasks-cancel-grace fragment in compendium m…

d81a702

…anifest (#2499)

chatgpt-codex-connector Bot reviewed Nov 2, 2026

View reviewed changes

fix(mcpx): CancelImmediate cancels the work before notifying watchers (…

f847036

…#2499)

dezren39 merged commit 687ac07 into main Nov 2, 2026
2 checks passed

dezren39 deleted the queue/5601-2499-implement-honest-task-endings branch November 2, 2026 04:36

Conversation

dezren39 commented Nov 1, 2026 • edited by blacksmith-sh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Evidence

Merge Danger

Uh oh!

coderabbitai Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

chatgpt-codex-connector Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

dezren39 Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

dezren39 commented Nov 2, 2026

Uh oh!

dezren39 commented Nov 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

dezren39 Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

dezren39 commented Nov 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

dezren39 Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

dezren39 commented Nov 2, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 2, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dezren39 commented Nov 1, 2026 •

edited by blacksmith-sh Bot

Loading

coderabbitai Bot commented Nov 1, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Nov 1, 2026 •

edited

Loading