chore(acp): track companion crates and correct the conformance record - #512
Merged
Conversation
Watching only the core SDK crate let an official HTTP/WebSocket transport go unnoticed while the same surface was hand-rolled here, and left the matrix asserting that a half-landed extension works in production. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview Governance docs add a Companion crates table in Conformance honesty: new Reviewed by Cursor Bugbot for commit df03096. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds an SDK HTTP/WebSocket bridge for NATS-backed ACP agents, makes client dispatch Send-safe, adds companion-crate freshness tracking, expands conformance records, and updates the AgentHandler example and crate documentation. ChangesACP conformance and freshness
SDK transport bridge and concurrency
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AcpHttpServer
participant NatsAgentComponent
participant connect_agent_boundary_with
participant client_run
participant NATS
AcpHttpServer->>NatsAgentComponent: connect transport
NatsAgentComponent->>connect_agent_boundary_with: create bridge boundary
connect_agent_boundary_with->>client_run: dispatch ACP messages
client_run->>NATS: forward agent request
NATS-->>client_run: return agent response
client_run-->>AcpHttpServer: return HTTP response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: df03096 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
The boundary only ever accepted a byte-stream pair, which is why evaluating the SDK's own HTTP/WebSocket transport required touching bridge internals rather than swapping a transport. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The LocalSet requirement was incidental, not a design constraint: every ClientHandler implementation was already Send + Sync, so only an Rc, a Cell and two spawn_local calls stood between the bridge and any SDK-owned transport. Forcing callers into spawn_local also pinned all client-side work for every connection to a single thread. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Proves the SDK's own Streamable HTTP and WebSocket transport can serve the byte-stream boundary that ADR#0020 always identified as upstream's territory, so the remaining question is the cutover's cost rather than whether the swap is possible at all. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/architecture/acp-conformance.md (1)
74-74: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not mark the aggregate batch row
implementedwhile HTTP remains non-conformant.This line explicitly documents that
POST /acpis untested and can return a bare object for a one-element batch, violating JSON-RPC batch response semantics. That conflicts with theimplementeddefinition in Lines 47-49. Fixrsworkspace/crates/acp/acp-nats-server/src/transport.rsand add HTTP regression coverage, or split the row by boundary and report the HTTP path separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/acp-conformance.md` at line 74, Update the JSON-RPC batch handling around IncomingHttpMessage::parse_all and http_post so POST /acp preserves batch semantics, including returning a one-element response array rather than a bare object; add HTTP regression coverage for this case and adjust the architecture table’s aggregate status unless all documented boundaries are conformant and tested.
🧹 Nitpick comments (1)
rsworkspace/crates/acp/acp-nats-server/src/component.rs (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated notification-channel capacity constant.
NOTIFICATION_CHANNEL_CAPACITY = 64is a new named constant here, but the comment itself acknowledges it must stay in sync with the literal64used for the same channel inconnection.rs,transport.rs, andacp-nats-stdio/src/main.rs. Consider promoting this to a sharedpub constinacp_natsso all transports reference one source of truth instead of relying on a comment to keep them aligned.Also applies to: 63-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/acp/acp-nats-server/src/component.rs` around lines 20 - 21, The notification channel capacity is duplicated across transports and can drift. Move the capacity value into a shared public constant in the acp_nats module, then update NOTIFICATION_CHANNEL_CAPACITY and the channel definitions in connection.rs, transport.rs, and acp-nats-stdio/src/main.rs to reference that shared constant instead of literal 64 values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/acp-conformance.md`:
- Line 30: Qualify the surrounding lockstep-release statement so it applies only
to companions that actually version and release in lockstep with core 2.0.0.
Preserve the table’s independent version entries, including
agent-client-protocol-schema 1.6.0 and agent-client-protocol-tokio 0.11.1, and
make the freshness-tracking wording unambiguous.
In `@rsworkspace/crates/acp/acp-nats-server/src/component.rs`:
- Around line 95-103: Guard the client-task cleanup in the component startup
flow: after the tokio::select! involving client_task.handle_mut() and
shutdown_rx, call abort_and_wait() only when !client_task.is_finished().
Preserve the existing shutdown logging and return behavior, matching the
established pattern in the related connection and transport flows.
In `@rsworkspace/crates/acp/acp-nats-server/src/main.rs`:
- Around line 4-7: The production server still uses the legacy transport routes
instead of the official bridge. In
rsworkspace/crates/acp/acp-nats-server/src/main.rs lines 4-7, update main’s
router setup to serve the AcpHttpServer/NatsAgentComponent path; retain the
LocalSet rationale at lines 126-132 until the legacy connection actors are
removed from the active server path.
In `@rsworkspace/crates/acp/acp-nats-stdio/src/main.rs`:
- Around line 102-106: Replace spawn_local with tokio::spawn for client::run,
remove the LocalSet and its local.run_until wrapper from main, and update the
run_bridge test wrappers in src/tests.rs to await the futures directly without
LocalSet.
---
Outside diff comments:
In `@docs/architecture/acp-conformance.md`:
- Line 74: Update the JSON-RPC batch handling around
IncomingHttpMessage::parse_all and http_post so POST /acp preserves batch
semantics, including returning a one-element response array rather than a bare
object; add HTTP regression coverage for this case and adjust the architecture
table’s aggregate status unless all documented boundaries are conformant and
tested.
---
Nitpick comments:
In `@rsworkspace/crates/acp/acp-nats-server/src/component.rs`:
- Around line 20-21: The notification channel capacity is duplicated across
transports and can drift. Move the capacity value into a shared public constant
in the acp_nats module, then update NOTIFICATION_CHANNEL_CAPACITY and the
channel definitions in connection.rs, transport.rs, and
acp-nats-stdio/src/main.rs to reference that shared constant instead of literal
64 values.
🪄 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 Plus
Run ID: 81e85dac-55de-4bb1-b40d-c6cdee4f9879
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docs/architecture/acp-conformance.mdrsworkspace/Cargo.tomlrsworkspace/crates/acp/acp-nats-server/Cargo.tomlrsworkspace/crates/acp/acp-nats-server/src/component.rsrsworkspace/crates/acp/acp-nats-server/src/connection.rsrsworkspace/crates/acp/acp-nats-server/src/main.rsrsworkspace/crates/acp/acp-nats-server/src/tests.rsrsworkspace/crates/acp/acp-nats-server/src/transport.rsrsworkspace/crates/acp/acp-nats-stdio/src/main.rsrsworkspace/crates/acp/acp-nats/src/boundary/connect_agent_boundary.rsrsworkspace/crates/acp/acp-nats/src/boundary/mod.rsrsworkspace/crates/acp/acp-nats/src/client/mod.rsrsworkspace/crates/acp/acp-nats/src/client/tests.rsrsworkspace/crates/acp/acp-nats/src/in_flight_slot_guard.rsrsworkspace/crates/acp/acp-nats/src/in_flight_slot_guard/tests.rsrsworkspace/crates/acp/acp-nats/src/lib.rs
Review found a panic on the client task's normal-exit path: racing the join handle in a select and then cleaning up unconditionally awaits it twice. Guarding inside the helper rather than at each call site keeps the next caller from rediscovering it. Also: a companion crate lookup that failed to reach crates.io was reported as a release, so a transient blip could file a drift issue; and retiring the last spawn_local in acp-nats-stdio lets its LocalSet go. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
A panicked client proxy was reported as a clean close, so the transport could not tell a crashed connection from a disconnected peer. The WebSocket and stdio paths already inspect this result; the new component now agrees with them. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
CI caught two things local clippy does not: the repo's own dylint rejects inline test module blocks, and the coverage gate treats a new file's uncovered lines as a regression. Both were mine. The teardown and error paths were unreachable from the one HTTP round-trip test, so they are now driven directly: the boundary's generic entry point gets its own tests instead of borrowing coverage from the server crate, and the two outcome decisions became functions that can be asserted rather than only observed. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The factory runs once per connection, so looking up the instrumentation scope inside it rebuilt the bridge's three metric instruments for every client. The scope belongs to the service, so the caller owns it now. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The HTTP and WebSocket boundary was never the part that had to be ours. Handing it to upstream leaves one framing implementation across HTTP, WebSocket, and stdio instead of three, and retires a JSON-RPC batch divergence the hand-rolled path carried. The NATS leg stays hand-rolled for the reason ADR#0020 gives: its wire format carries no JSON-RPC envelope, so no byte-stream transport can express it. Behavior visible to clients changes in several ways, none a spec regression and two of them stricter than before. They are enumerated under "Remote transport behavior changes" in the conformance doc; the two costs accepted rather than solved are UUIDv4 connection ids and refusing browser WebSocket upgrades. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rsworkspace/crates/acp/acp-nats-server/src/compat/tests.rs (1)
84-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntested branch: unspecified bind host (
0.0.0.0/::).
origin_rejectionhas a third branch forbind_host.is_unspecified()that allows loopback origins or an origin matching theHostheader, which is the configuration a containerized deployment actually runs with. Only the loopback and specific-address branches are covered here; a case bound to0.0.0.0(loopback origin allowed, foreign origin with mismatchedHostrejected) would lock that behavior down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/acp/acp-nats-server/src/compat/tests.rs` around lines 84 - 103, Add coverage for the unspecified bind-host branch in origin_rejection using a 0.0.0.0 or :: bind configuration. In the origin_status tests, assert a loopback origin is allowed and a foreign origin with a mismatched Host header is rejected, preserving the existing loopback and specific-address cases.rsworkspace/crates/acp/acp-nats-server/src/tests.rs (1)
992-1027: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name no longer matches what it asserts.
streamable_http_get_broadcasts_connection_stream_updates_to_all_active_listenersnow proves two things: connection-stream fan-out for thesession/newreply and session-scoped fan-out forsession/updateon separate session streams. The second half is the interesting new behavior and is invisible from the name; splitting it into a dedicated..._broadcasts_session_stream_updates_to_all_session_listenerstest (or renaming) would make a future regression easier to place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/acp/acp-nats-server/src/tests.rs` around lines 992 - 1027, The existing test name no longer describes its session-scoped fan-out assertions. Rename or split the test around the session_stream setup and SessionNotification flow so the session/update behavior is covered by a dedicated test named to indicate broadcasting session-stream updates to all session listeners, while preserving the existing connection-stream coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/acp/acp-nats-server/src/compat.rs`:
- Around line 41-75: Bound the storage used by NegotiatedVersions so repeated
initialize calls cannot grow the map indefinitely when forget is never invoked.
Update NegotiatedVersions and its record path to enforce a bounded capacity or
expiration policy, while preserving get and forget behavior and the existing
negotiated-version lookup contract.
- Around line 230-235: Update the response inspection flow around
axum::body::to_bytes so oversized responses are passed through unchanged: check
the response CONTENT_LENGTH before consuming the body and return the original
response when it exceeds MAX_INSPECTED_BODY. For bodies without a usable length,
preserve the existing inspection path, but handle a genuinely broken to_bytes
result with an explicit 502 response rather than returning an empty body with
stale response parts; revise the comment to match these outcomes.
- Around line 142-155: Normalize the IPv6 representation before comparing the
origin host with the configured bind host in the allowed-host logic. Update the
global-bind comparison near is_loopback_host and reuse the existing
bracketed/unbracketed handling so Uri::host() values such as “[2001:db8::1]”
match bind_host.to_string() without relying on matches_request_host().
---
Nitpick comments:
In `@rsworkspace/crates/acp/acp-nats-server/src/compat/tests.rs`:
- Around line 84-103: Add coverage for the unspecified bind-host branch in
origin_rejection using a 0.0.0.0 or :: bind configuration. In the origin_status
tests, assert a loopback origin is allowed and a foreign origin with a
mismatched Host header is rejected, preserving the existing loopback and
specific-address cases.
In `@rsworkspace/crates/acp/acp-nats-server/src/tests.rs`:
- Around line 992-1027: The existing test name no longer describes its
session-scoped fan-out assertions. Rename or split the test around the
session_stream setup and SessionNotification flow so the session/update behavior
is covered by a dedicated test named to indicate broadcasting session-stream
updates to all session listeners, while preserving the existing
connection-stream coverage.
🪄 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 Plus
Run ID: 76d877b1-4f42-4eda-aa75-1a6b6ff78f10
📒 Files selected for processing (14)
docs/adr/0020-acp-sdk-1x-boundary-and-bridge-traits.mddocs/architecture/acp-conformance.mdrsworkspace/crates/acp/acp-nats-server/README.mdrsworkspace/crates/acp/acp-nats-server/src/acp_connection_id.rsrsworkspace/crates/acp/acp-nats-server/src/acp_connection_id/tests.rsrsworkspace/crates/acp/acp-nats-server/src/compat.rsrsworkspace/crates/acp/acp-nats-server/src/compat/tests.rsrsworkspace/crates/acp/acp-nats-server/src/connection.rsrsworkspace/crates/acp/acp-nats-server/src/connection/tests.rsrsworkspace/crates/acp/acp-nats-server/src/constants.rsrsworkspace/crates/acp/acp-nats-server/src/main.rsrsworkspace/crates/acp/acp-nats-server/src/tests.rsrsworkspace/crates/acp/acp-nats-server/src/transport.rsrsworkspace/crates/acp/acp-nats-server/src/transport/tests.rs
💤 Files with no reviewable changes (6)
- rsworkspace/crates/acp/acp-nats-server/src/acp_connection_id/tests.rs
- rsworkspace/crates/acp/acp-nats-server/src/connection/tests.rs
- rsworkspace/crates/acp/acp-nats-server/src/acp_connection_id.rs
- rsworkspace/crates/acp/acp-nats-server/src/transport/tests.rs
- rsworkspace/crates/acp/acp-nats-server/src/connection.rs
- rsworkspace/crates/acp/acp-nats-server/src/transport.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture/acp-conformance.md
Two reviewers independently found the version map growing without bound. Only DELETE reports a close, and upstream keeps connection lifetime private, so a peer drop or crash left an entry behind for the life of the process. It is now bounded by eviction, which degrades to skipping a SHOULD-level check rather than rejecting a request. Reading a body to discover it exceeded the inspection cap consumed it, and the response then went out with its original Content-Length and no payload: a truncated reply that still looked successful. The declared length is checked before the body is touched, and a body that genuinely cannot be read now fails loudly. Also normalizes IPv6 brackets, which `Uri::host()` keeps and `IpAddr` does not, so a global IPv6 bind address could never match its own origin. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Parsing the header straight to an Option conflated absent with unparseable, so a garbage value was served as though the client had sent nothing. The hand-rolled transport answered 400 there, and restoring that check is why this layer exists. Also drops three dependencies the transport deletion orphaned. Two were reported; serde was found by auditing the rest, since nothing had flagged it. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The coverage gate was right and the local gate was not strict enough to see it: two branches had no test. One is the bound-to-all-interfaces origin policy, which is the shape production runs in and had never been exercised, so this closes a real gap rather than a reporting one. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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 9b5bcb5. Configure here.
An unreadable companion table only reached the issue when nothing else drifted, so a core bump would hide the fact that companion versions were not being compared at all. That is the silent gap this task exists to catch, produced by the task itself. 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
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.
agent-client-protocol-http, released in lockstep with the 2.0.0 core we just adopted) went unnoticed while the same surface was hand-rolled here. Companion versions now live in the conformance doc and the freshness task reads them, so a companion release reaches us the same way a core release does.ext/session/prompt_responsewas recorded as implemented while its completion half cannot run in a release build. The waiter registry it resolves against has no production insertion site, so every such notification is logged and dropped. Recording that honestly matters more than the code being tidy, and the machinery is deliberately kept because the problem it solves is real.implementedwas carrying two different meanings: routed and working, versus routed and fully unit-tested but unreachable end to end. The newhalf-wiredvalue separates them so a green test suite can no longer imply a working surface.acp-nats-agentusage example could not compile: it implemented an SDK trait removed in 2.0.0 and omitted two required methods. It is now a compiled doctest, so it fails CI instead of misleading readers.