Repository navigation
Peer: give a request handler the request's id - #14
Merged
Merged
Conversation
JSONRPCPeer handed its request handler only the method and params, so a handler could not tell two requests with the same method and params apart, nor tie a request to what it belongs to by its id. setHandlers(identifiedRequest: notification:) installs an IdentifiedRequestHandler, which is given the id of the request it answers as well. setHandlers(request:notification:) is unchanged: its handler is wrapped into the new form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
JSONRPCPeerhands its request handler only the method and params (RequestHandler). A handler therefore can't tell two inbound requests apart when their method and params are the same. It also can't tie a request to what it belongs to by its JSON-RPC id, even though the wire log (setWireLog) sees that id as the request is read.This is Cocoanetics/SwiftACP#138. SwiftACP binds each request an ACP agent makes (a permission question, a file read or write, starting a command) to the prompt that was in flight when the request was read, as acpx does. The wire log sees the id, but the handler that serves the request has only method and params. So it claims "the first unclaimed request with this method and params", and two identical requests can swap owners when their handler tasks start out of order. The one belonging to an answered prompt then gets served, and the live one is answered
Request cancelled. acpx has no such ambiguity, because the ACP SDK hands each handler its own request.What
JSONRPCPeer.IdentifiedRequestHandler:(id, method, params) async -> Result<JSONValue, JSONRPCError>.setHandlers(identifiedRequest:notification:): installs one. The peer passes the id of the request the handler answers, the same id its reply goes out with.setHandlers(request:notification:)is unchanged: its handler is wrapped into the new form. The new method has its own label, sosetHandlers(request: nil, notification: …)call sites stay unambiguous.It's additive, so this can be a minor release.
Verification
givesAnIdentifiedRequestHandlerEachRequestsID) sends two requests with the same method and params, under an integer id and a string id. It expects each to be answered with what its own id gave. With the peer passing a fixed id instead, both expectations fail.swiftlint --strictis clean.🤖 Generated with Claude Code