Skip to content

fix(acp): propagate request cancellation to peers - #158

Open
YoungSx wants to merge 6 commits into
developfrom
implement-cancel-request-protocol
Open

fix(acp): propagate request cancellation to peers#158
YoungSx wants to merge 6 commits into
developfrom
implement-cancel-request-protocol

Conversation

@YoungSx

@YoungSx YoungSx commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add ACP $/cancel_request modeling with lossless request-id forms
  • notify peers when outbound request cancellation races transport writes
  • classify peer -32800 responses as cancellation instead of persistent UI failure

Validation

  • SalmonEgg.Acp.Tests: 378 passed
  • SalmonEgg.Presentation.Core.Tests: 3291 passed
  • SalmonEgg.Infrastructure.Tests: 728 passed, 6 environment/platform skips
  • real stdio and WebSocket transports both exercised end to end
  • the Unix-only chmod in the new stdio test is guarded for CA1416, which the
    platform analyzer cannot infer from Assert.SkipWhen; reproduced and cleared
    locally with the same dotnet format --verify-no-changes the gate runs
  • post-rebase builds and diff checks passed

Target branch: develop. Branch is rebased onto the latest origin/develop with a
linear, single-parent history, so it rebase-merges cleanly.

YoungSx and others added 3 commits September 2, 2026 00:51
ACP lists -32800 (Request cancelled) alongside the JSON-RPC standard codes
and the ACP extension codes, but it belongs to neither: IsStandardErrorCode
covers -32700..-32603 and IsAcpErrorCode covers -32099..-32000, so a peer's
compliant cancellation response fell through GetErrorMessage's switch and
rendered as "Unknown error (code: -32800)" — indistinguishable from a real
failure.

Give it a dedicated constant, region and predicate rather than widening
either band, which would also reclassify every unassigned code in between.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…chema

ACP RequestId permits null, numbers, and strings. Add a public union that
preserves all three forms and keeps raw numeric token text so cancellation
notifications echo the exact request identity a peer can correlate.

Add CancelRequestParams for the protocol-level $/cancel_request notification,
register it in the source-generated serialization context, update the public
surface baseline, and lock null/number/string plus invalid-token behavior with
round-trip contract tests.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Once an ACP request has been dispatched, caller cancellation now sends the
protocol-level $/cancel_request notification with the original JSON-RPC id.
The cancelled local await returns promptly while its correlation entry stays
alive until the peer's required terminal response arrives, including -32800.

Delivery is best effort: peers may ignore $/ notifications and a disconnect
or failed notification cannot become a second user-facing failure. Tests
assert the actual wire frame and matching request id, not local state.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@vercel

vercel Bot commented Sep 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
salmonegg Ready Ready Preview Sep 2, 2026 2:45am UTC

YoungSx and others added 3 commits September 2, 2026 02:37
A compliant agent may settle an original request with JSON-RPC -32800 after
receiving $/cancel_request. Treat that as cancellation at the presentation
boundary: prompts enter the existing cancelled turn phase, activation and
hydration avoid fault callouts, and session operations/recovery skip their
persistent failure messages.

Also add a real stdio child-process test that captures ACP frames on the
actual pipe and proves $/cancel_request carries the original request id.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…tion

An in-flight transport send may have already placed bytes on the peer's pipe
or socket when its cancellation token trips. Treat entry into SendMessageAsync
as potentially dispatched and send $/cancel_request after the caller cancels,
while retaining the pre-send token check so a request cancelled before writing
still emits nothing.

The test holds a send in progress, cancels it, and proves the notification is
still emitted after releasing the write.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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.

1 participant