refactor(acp-nats): route the byte-stream boundary through the SDK dispatch enums#478
Merged
Conversation
…tream boundary The SDK's ClientRequest/ClientNotification enums already carry the method table, so the per-method shim functions and registrations only added a third place to forget when the spec grows. ADR 0020 amended. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Three private naming dialects made telemetry and conformance checks speak a language the spec does not, and blocked SDK enum decoding. The labels are never serialized onto the NATS wire, so peers upgrade independently. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview ADR 0020 is amended to describe this dispatch shape; acp-conformance’s upgrade ritual now says new methods add a boundary match arm (not a separate registration). ADR 0022 records the rejection of renaming NATS subject tokens to spec-canonical method names—the NATS leg keeps subject-token identity; canonical names stay on JSON-RPC byte-stream boundaries only. Reviewed by Cursor Bugbot for commit d318e9a. Bugbot is set up for automated code reviews on this repo. Configure here. |
WalkthroughSimplifies ACP-over-NATS by consolidating per-method inbound boundary registrations into a single ChangesACP-over-NATS wire method and dispatch consolidation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SDK
participant OnReceiveDispatch
participant handle_client_dispatch
participant AgentHandler
SDK->>OnReceiveDispatch: Dispatch<ClientRequest, ClientNotification>
OnReceiveDispatch->>handle_client_dispatch: forward dispatch
handle_client_dispatch->>AgentHandler: matched request variant via respond_in_task
AgentHandler-->>handle_client_dispatch: serialized result
handle_client_dispatch-->>SDK: response or method_not_found
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: d318e9a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/acp-nats/src/agent/ext_method.rs (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
GlobalAgentMethod::Extinstead of duplicating the wire-method format.All other handler files in this PR derive their wire method from
GlobalAgentMethod::wire_method(), but this file manually reconstructs the sameformat!("_{}", ...)pattern thatGlobalAgentMethod::Ext(name)already encapsulates inparsing.rs. If the extension wire-method format ever changes, this becomes a second update site.♻️ Proposed refactor for DRY consistency
+use crate::nats::{GlobalAgentMethod, RequestClient, global};let subject = global::ExtSubject::new(bridge.config.acp_prefix_ref(), &method_name); - let wire_method = format!("_{}", method_name.as_str()); + let wire_method = GlobalAgentMethod::Ext(method_name.clone()).wire_method();🤖 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-nats/src/agent/ext_method.rs` at line 34, The wire-method string is being rebuilt manually in ext_method.rs instead of using the shared GlobalAgentMethod::wire_method() path. Update the ext method handler to construct or reuse GlobalAgentMethod::Ext and call wire_method() so the formatting logic stays centralized in parsing.rs and matches the other handler files.Source: Coding guidelines
rsworkspace/crates/acp-nats/src/client/ext_session_prompt_response.rs (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sourcing the wire-method string from
ClientMethod::ExtSessionPromptResponse.wire_method().The hardcoded
"_session/prompt_response"duplicates the value already defined byClientMethod::ExtSessionPromptResponseinparsing.rs. The agent-side handlers in this PR were updated to useGlobalAgentMethod::wire_method()for the same reason. This is a lower priority since it's a single static literal rather than a format pattern, but it would keep a single source of truth for the wire-method name.♻️ Optional refactor
- match decode_notification_params::<PromptResponse>("_session/prompt_response", headers, payload) { + match decode_notification_params::<PromptResponse>( + &ClientMethod::ExtSessionPromptResponse.wire_method(), + headers, + payload, + ) {🤖 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-nats/src/client/ext_session_prompt_response.rs` at line 45, The `decode_notification_params` call in `ext_session_prompt_response.rs` hardcodes the wire-method string, duplicating the value already owned by `ClientMethod::ExtSessionPromptResponse`. Update this path to source the method name from `ClientMethod::ExtSessionPromptResponse.wire_method()` instead of the literal, mirroring the `GlobalAgentMethod::wire_method()` refactor used elsewhere, so the `decode_notification_params` usage stays aligned with the single source of truth in `parsing.rs`.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/agent/ext_method.rs`:
- Line 34: The wire-method string is being rebuilt manually in ext_method.rs
instead of using the shared GlobalAgentMethod::wire_method() path. Update the
ext method handler to construct or reuse GlobalAgentMethod::Ext and call
wire_method() so the formatting logic stays centralized in parsing.rs and
matches the other handler files.
In `@rsworkspace/crates/acp-nats/src/client/ext_session_prompt_response.rs`:
- Line 45: The `decode_notification_params` call in
`ext_session_prompt_response.rs` hardcodes the wire-method string, duplicating
the value already owned by `ClientMethod::ExtSessionPromptResponse`. Update this
path to source the method name from
`ClientMethod::ExtSessionPromptResponse.wire_method()` instead of the literal,
mirroring the `GlobalAgentMethod::wire_method()` refactor used elsewhere, so the
`decode_notification_params` usage stays aligned with the single source of truth
in `parsing.rs`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18fcd487-b101-4ab2-879b-cb78a883f988
📒 Files selected for processing (18)
PLAN.mddocs/adr/0020-acp-sdk-1x-boundary-and-bridge-traits.mddocs/adr/0022-canonical-acp-wire-methods-on-nats.mddocs/architecture/acp-conformance.mdrsworkspace/crates/acp-nats-agent/src/connection/tests.rsrsworkspace/crates/acp-nats/src/agent/ext_method.rsrsworkspace/crates/acp-nats/src/agent/list_sessions.rsrsworkspace/crates/acp-nats/src/agent/new_session.rsrsworkspace/crates/acp-nats/src/agent/providers_disable.rsrsworkspace/crates/acp-nats/src/agent/providers_list.rsrsworkspace/crates/acp-nats/src/agent/providers_set.rsrsworkspace/crates/acp-nats/src/boundary/connect_agent_boundary.rsrsworkspace/crates/acp-nats/src/client/ext.rsrsworkspace/crates/acp-nats/src/client/ext/tests.rsrsworkspace/crates/acp-nats/src/client/ext_session_prompt_response.rsrsworkspace/crates/acp-nats/src/client/ext_session_prompt_response/tests.rsrsworkspace/crates/acp-nats/src/nats/parsing.rsrsworkspace/crates/acp-nats/src/nats/parsing/tests.rs
…0022 Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The rename's justifications did not survive scrutiny: the method is never serialized on the NATS wire, enum decode was skipped, and the token-to-name mapping exists either way. Splitting wire_method() from the subject suffix traded a NATS-native invariant (one identity for routing, decode, telemetry, ACLs) for vocabulary aesthetics. ADR 0022 is recorded as rejected. 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>
yordis
changed the title
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.
ClientRequest/ClientNotificationenums; re-enumerating it per method at the byte-stream boundary meant every new spec method had to be wired in three places instead of two, and a forgotten registration was silently unreachable rather than answeringmethod_not_found.