fix(acp-nats-server): preserve ACP HTTP compatibility#135
Merged
Conversation
Member
yordis
commented
Apr 23, 2026
yordis
commented
- Align the ACP HTTP transport with the current upstream protocol semantics so remote clients do not rely on superseded request and streaming behavior.
- Preserve interoperability between this server and ACP clients that now expect JSON initialize responses, immediate follow-up POST acknowledgements, and a connection-scoped event stream.
- Keep the crate documentation and coverage pointed at the protocol shape the repo is intended to support.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Refactors SSE fanout from session-scoped listeners to a single per-connection listener set, removes the priming/empty SSE frame behavior, relaxes POST Reviewed by Cursor Bugbot for commit cbcd881. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR refactors the ACP NATS server's HTTP transport architecture from SSE-based response streaming to JSON-only POST responses combined with asynchronous delivery via a separate, connection-scoped GET stream. POST Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server as HTTP Server
participant NATS as NATS Backend
rect rgba(100, 150, 200, 0.5)
Note over Client,NATS: New Flow: JSON POST + Async GET Stream
end
Client->>Server: POST /acp (initialize request)<br/>with Acp-Connection-Id
Server->>NATS: Forward initialize request
NATS-->>Server: Initialize response
Server->>Server: Inject connectionId into result
Server-->>Client: 200 OK + JSON response
Client->>Server: GET /acp (opens stream)<br/>with Acp-Connection-Id
activate Server
Client->>Server: POST /acp (session/new request)
Server->>NATS: Forward session request
NATS-->>Server: Session response
Server->>Server: Inject sessionId into result
Server-->>Client: 202 Accepted (empty body)
Server-->>Client: JSON-RPC result on GET stream
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: cbcd881 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8375101. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rsworkspace/crates/acp-nats-server/README.md (1)
50-60: Document the required ordering: openGET /acpbefore sending follow-upPOSTs.With the new async delivery model, any response to a follow-up
POSTis fanned out to the currently-attached GET listeners on the connection (dispatch_to_get_listenersintransport.rs). If the client hasn't yet opened the long-livedGET /acpstream when the backend produces the response, the frame is dropped and the client will wait indefinitely for a reply it can never receive. The README should make this sequencing explicit so integrators don't get bitten by a race betweenPOSTand the subsequentGET.📝 Proposed wording tweak
-Then open the long-lived SSE stream for the connection: +After `initialize`, open the long-lived SSE stream for the connection **before** sending any follow-up `POST` requests — responses to those requests are delivered exclusively on this stream and are dropped if no listener is attached when they arrive:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats-server/README.md` around lines 50 - 60, Update the README to explicitly require opening the long-lived GET /acp SSE stream before sending any follow-up POSTs: explain that responses to follow-up POSTs are delivered asynchronously and fanned out to currently-attached GET listeners (see dispatch_to_get_listeners in transport.rs), so if the client hasn't attached a GET listener yet the response may be dropped and the client will hang; add a short example/one-line note near the curl examples that instructs users to open the GET /acp stream first and only then send subsequent POST requests, and mention that initialize (POST /acp) is the only POST that returns immediate JSON while others return 202 and deliver JSON on the GET stream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/acp-nats-server/src/transport.rs`:
- Around line 292-297: PendingResponseContext currently has inject_session_id
always true; change the construction site that sets inject_session_id (the code
around where PendingResponseContext is instantiated) so inject_session_id is
only true when the request method satisfies attaches_session() (i.e., only for
backend methods that may legitimately omit sessionId like session/load or
session/resume), and leave inject_session_id false otherwise; update any
comments and keep the single field usage (reading inject_session_id) untouched
so the injection behavior is gated to attaches_session() only.
- Around line 1241-1248: The code currently discards the Result from
dispatch_to_get_listeners; change it to inspect the returned enum
(Missing/Dropped/Delivered) and, when the parsed OutgoingHttpMessage (from
OutgoingHttpMessage::parse(&outbound)) represents a response (i.e., has a
correlating id and is not a pure notification—use
OutgoingHttpMessage::result_session_id or the message id accessor to detect
this), emit a warn! (or increment a dropped-response counter) if the outcome is
Missing or Dropped so operators can detect lost responses; keep notifications
best-effort and only log for messages identified as responses. Ensure you
reference SseFrame::json, parsed/outbound, dispatch_to_get_listeners, and
get_listeners in the change so the new logic replaces the discarded let _ = ...
line.
---
Nitpick comments:
In `@rsworkspace/crates/acp-nats-server/README.md`:
- Around line 50-60: Update the README to explicitly require opening the
long-lived GET /acp SSE stream before sending any follow-up POSTs: explain that
responses to follow-up POSTs are delivered asynchronously and fanned out to
currently-attached GET listeners (see dispatch_to_get_listeners in
transport.rs), so if the client hasn't attached a GET listener yet the response
may be dropped and the client will hang; add a short example/one-line note near
the curl examples that instructs users to open the GET /acp stream first and
only then send subsequent POST requests, and mention that initialize (POST /acp)
is the only POST that returns immediate JSON while others return 202 and deliver
JSON on the GET stream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f306e74c-0998-4293-ad9b-618ffb70a41d
📒 Files selected for processing (3)
rsworkspace/crates/acp-nats-server/README.mdrsworkspace/crates/acp-nats-server/src/main.rsrsworkspace/crates/acp-nats-server/src/transport.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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.