feat(acp): catch up to the current ACP spec and make drift observable - #477
Merged
Conversation
yordis
commented
Jul 8, 2026
yordis
commented
Member
- The pinned agent-client-protocol fell one full SDK redesign behind upstream (0.10.4 vs 1.2.0, schema 0.11.4 vs 1.4.0) with no signal: the exact pin made it invisible to dependabot, and the bridge's typed re-encode turned every unrepresentable spec feature into silent data loss against current-spec peers (additionalDirectories stripped, session/delete unroutable, new session/update variants dropped on decode).
- Tracking now exists so this cannot recur silently: a conformance matrix with an upgrade ritual (docs/architecture/acp-conformance.md), a weekly freshness workflow that files a checklist-bearing drift issue, and ADRs 0017/0018 recording why the bridge owns its handler traits and keeps typed decode. Closes ACP SDK migration: agent-client-protocol 0.10.4 to 1.2.x #474.
- Unstable spec features are adopted ahead of stabilization by policy; the only opt-out is providers at the byte-stream boundary, blocked because upstream omits the provider variants from its request enums (rationale in the matrix).
- session/set_model is removed because upstream deleted the API; external runner repos that publish to that subject must migrate to model_config session config options before upgrading against this bridge.
- The migration surfaced two latent correctness defects worth reviewer attention: the 1.x SDK treats peer EOF as graceful and would keep bridge connections alive forever (fixed with an EOF-signaling reader), and boundary handlers originally ran on the SDK dispatch loop, serializing all requests on a connection and making cancellation unreachable (fixed by spawning handler work onto the connection task actor).
The pinned agent-client-protocol fell one major redesign behind upstream without any signal, and the bridge's typed re-encode turns spec lag into silent data loss. Record the spec position, make drift observable, and make feature mapping a mandatory part of every future bump. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…bility payloads The bridge re-encodes typed structs, so any field the pinned SDK does not compile in is silently stripped. additionalDirectories is stable in the current ACP spec and peers already send it. Elicitation and NES follow the opt-in-ahead-of-stable policy. Unknown session/update variants now emit an error metric instead of vanishing. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ed handler traits The 0.10.4 SDK is one full redesign behind upstream, and every spec feature it cannot express is silently stripped by the bridge's typed re-encode. The 1.x SDK removed the Agent/Client traits our routing was built on, so per ADR 0017 the bridge now owns its handler contract. session/set_model is gone because upstream deleted the API. The direct schema dependency exists to enable schema-level unstable flags the SDK facade does not forward, per the opt-in-ahead-of-stable policy. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ed AgentHandler trait The SDK 1.x removed the Agent trait this crate dispatched against, and upstream deleted session/set_model from the protocol. Runners now implement acp_nats::AgentHandler per ADR 0017. Unhandled ext methods surface MethodNotFound instead of silently succeeding, matching every other defaulted method. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The 1.x SDK removed AgentSideConnection, so the server and stdio boundaries now compose the shared acp_nats::boundary adapters per ADR 0017. The SDK treats incoming EOF as a graceful stream end and keeps connections alive forever; an EOF-signaling reader restores the old close-on-disconnect semantics. ADR 0018 records why the bridge keeps typed decode instead of adopting passthrough forwarding. session/delete groundwork lands in semconv; the set_model span is gone with the protocol method. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… end session/delete has been stable in the spec since schema 0.13.6 and was unroutable here. Cancellation exposed a deeper defect: boundary handlers ran inline on the SDK dispatch loop, so one in-flight request blocked every other message on the connection, including the cancellation that would have freed it. Handler work now runs on the connection task actor. Round-trip tests pin every newly representable session/update variant so the next schema bump cannot silently regress them. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…loads The opt-in-ahead-of-stable policy requires every unstable spec feature to traverse the bridge before peers depend on it. Elicitation routes end to end including enum option descriptions. Providers are fully routed on the NATS leg; the SDK 1.2.0 byte-stream boundary cannot dispatch them because upstream omits the provider variants from its request enums, which the conformance matrix records as the blocking rationale. MCP-over-ACP payloads round-trip so acp-transport MCP servers survive session setup. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview Wire surface changes: adds NATS/agent dispatch for Drift is now explicit: Reviewed by Cursor Bugbot for commit 270f941. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughMigrates the ACP bridge to SDK/schema 1.x, replacing removed SDK traits with bridge-owned handler traits and boundary wiring. It adds session delete, provider management, and elicitation routing, updates transport/server/stdio integration, and adds ACP conformance docs, freshness automation, and a new dylint. ChangesACP SDK 1.x Migration
Function-local macro_rules Dylint
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The conformance matrix, upgrade ritual, and ADRs carry the durable decisions; the plan was session scaffolding. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The working plan file is gone; the tracking issue and freshness workflow are the long-lived anchors. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The registration chain was fifteen anonymous closure blocks; reviewers could not see the routed surface at a glance. Each handler is now a named function and registration reads as a routing table. A local macro does the Arc binding because return-position AsyncFnMut cannot promise Send futures on stable Rust yet. 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 01f70c0. Configure here.
Inline workflow bash cannot be run or tested locally; as a mise task the same script serves developers and CI, and the workflow shrinks to checkout, run, file issue. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Main claimed ADR numbers 0017 through 0019, so the ACP boundary and typed-decode ADRs move to 0020 and 0021 with references updated. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Workflow YAML cannot be exercised locally; with the whole flow in the task, the workflow is checkout plus one invocation, and issue filing stays opt-in so local runs are read-only. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…offender Function-local macros lean on definition-site hygiene to silently capture surrounding locals, hiding the macro's real inputs. The boundary routing macros now live at module scope with the agent passed explicitly, and the lint keeps the pattern from coming back. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 270f941 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
The shutdown test asserted error display text, which the error_string_comparison lint exists to forbid. The coverage gate flagged the handler defaults, routing enums, and boundary adapters as newly uncovered; exercising every routed method end to end also caught a real defect: a spawned notification handler returning an error killed the entire connection, so one unknown ext notification from a peer could take the bridge down. Notification handlers now log and swallow. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
CI's coverage gate compares per-file missed counts against main, so newly created files must arrive essentially fully covered. The last uncovered boundary paths were the ext success response, the failing notification handlers, and the non-ext enum fallthroughs, which needed an agent stub that succeeds and fails in the opposite places from the existing one. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
rsworkspace/crates/acp-nats/src/agent/prompt/tests.rs (1)
169-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the duplicated notification construction into a test helper.
Both
prompt_js_notification_forwarding(lines 169–178) andprompt_js_notification_receiver_dropped(lines 451–460) build an identicalSessionNotificationwith the sameAgentThoughtChunk→ContentChunk→TextContent::new("thinking...")structure. Extracting a small helper (e.g.,fn make_thought_notification(session_id: &str) -> SessionNotification) would eliminate the duplication and keep future schema migrations to a single site.Also applies to: 451-460
🤖 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/prompt/tests.rs` around lines 169 - 178, The test setup duplicates the same SessionNotification construction in prompt_js_notification_forwarding and prompt_js_notification_receiver_dropped; extract that repeated AgentThoughtChunk/ContentChunk/TextContent::new("thinking...") build into a small helper such as make_thought_notification(session_id: &str) and update both tests to use it. Keep the helper near the existing test utilities in tests.rs so future schema changes only need one update.rsworkspace/crates/acp-nats/src/boundary/connection_client.rs (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ext_methodandext_notificationcontain conversion logic, violating the zero-cost passthrough guideline.The doc comment claims "Every method is a zero-cost passthrough," but
ext_method(lines 66-75) andext_notification(lines 77-81) perform wire-method construction (format!),UntypedMessagecreation, error wrapping (map_err(Error::into_internal_error)), and return-type conversion (to_raw_value→ExtResponse::new). The coding guideline requires production trait implementations to be pure passthroughs with conversion logic in the caller.Consider either updating the doc to acknowledge the exception for ext methods, or adding raw untyped methods to
ClientHandler(e.g.,send_untyped_request/send_untyped_notification) so the conversion can live inclient/ext.rswhileConnectionClientremains a thin wrapper.As per coding guidelines: "Production implementations of infrastructure traits must be zero-cost passthroughs to the underlying SDK: no error wrapping, no return-type conversion, no
map_err… any conversion logic belongs in the caller."♻️ Proposed doc fix (minimal)
/// See ADR 0020: this is the SDK-connection-aware boundary shared by /// `acp-nats-server` and `acp-nats-stdio`. Every method is a zero-cost -/// passthrough to the connection's outbound request/notification calls. +/// passthrough to the connection's outbound request/notification calls, +/// except `ext_method`/`ext_notification` which adapt typed ext payloads +/// to the SDK's `UntypedMessage` wire format.Also applies to: 66-81
🤖 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/boundary/connection_client.rs` around lines 14 - 16, The `ConnectionClient` doc and its `ext_method`/`ext_notification` implementations conflict with the zero-cost passthrough guideline because they still do request formatting, `UntypedMessage` construction, error mapping, and response conversion. Update `ConnectionClient` so it only forwards raw untyped SDK calls by introducing or using raw methods on `ClientHandler` (such as `send_untyped_request`/`send_untyped_notification`) and move all conversion logic into `client/ext.rs`, or else revise the doc comment to explicitly call out the exception for ext methods. Keep `ext_method` and `ext_notification` as thin wrappers around the underlying handler.Source: Coding guidelines
rsworkspace/crates/acp-nats/src/client_proxy/tests.rs (1)
167-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider verifying response content in the
elicitation_createtest.The test asserts
result.is_ok()but doesn't verify the deserialized response content, unlike neighboring tests (e.g.,read_text_file_publishes_to_correct_subjectchecksresult.unwrap().content). Adding a content assertion would catch deserialization regressions in theCreateElicitationResponsepath.🧪 Suggested assertion
let result = p .elicitation_create(CreateElicitationRequest::new(mode, "please respond")) .await; - assert!(result.is_ok()); + assert!(result.is_ok()); + let response = result.unwrap(); + assert!(response.action.is_accept());🤖 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_proxy/tests.rs` around lines 167 - 180, The `elicitation_create_publishes_to_correct_subject` test only checks that `elicitation_create` succeeds, so it misses deserialization regressions in `CreateElicitationResponse`. Update this test to also inspect the returned value from `p.elicitation_create(...)` and assert the expected response content, following the pattern used by neighboring tests like `read_text_file_publishes_to_correct_subject`. Use the existing `elicitation_create`, `CreateElicitationResponse`, and `proxy` symbols to locate the test and add the content assertion.rsworkspace/crates/acp-nats/src/client/elicitation_complete.rs (1)
8-34: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider recording metrics on client forwarding failure too.
Metrics are recorded only on decode failure (line 30); the
client.elicitation_completeerror path (line 25-27) only logs a warning without a corresponding metric. This asymmetry may leave client-side forwarding failures unobserved.🤖 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/elicitation_complete.rs` around lines 8 - 34, The handle function in elicitation_complete.rs records metrics for decode failures but not for client forwarding failures. Update the client.elicitation_complete error branch inside handle so it also calls metrics.record_error with an appropriate category/reason before logging the warning, keeping the telemetry consistent with the decode_failure path. Use the existing handle, client.elicitation_complete, and metrics.record_error symbols to place the fix..github/workflows/acp-freshness.yml (2)
1-23: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider adding concurrency control to prevent duplicate drift issues.
Without a
concurrencyblock, a scheduled run and a manualworkflow_dispatchcould overlap. Both would find no existing drift issue (TOCTOU race) and each would create a separate issue.♻️ Proposed addition
on: schedule: # Weekly, Monday 09:00 UTC - cron: '0 9 * * 1' workflow_dispatch: +concurrency: + group: acp-freshness + cancel-in-progress: true + permissions:🤖 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 @.github/workflows/acp-freshness.yml around lines 1 - 23, Add workflow concurrency to the ACP Freshness GitHub Actions workflow so scheduled and manual runs cannot overlap and create duplicate drift issues. Update the acp-freshness workflow to include a concurrency block for the check job or workflow, using a stable group name and cancel-in-progress behavior, so the existing check step and drift issue creation run only once at a time.
17-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd
persist-credentials: falseto the checkout action.The workflow uses
GH_TOKENfrom the environment forghcommands, so persisting the GitHub token in git config (the default behavior) is unnecessary and expands the credential exposure surface. This matches the zizmorartipackedwarning.🔒 Proposed fix
- - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false🤖 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 @.github/workflows/acp-freshness.yml at line 17, Add persist-credentials: false to the actions/checkout step in the ACP freshness workflow. Update the checkout configuration in the workflow job that uses actions/checkout so it does not write the default GitHub token into git config, since GH_TOKEN is already used for gh commands; keep the change scoped to the checkout action and preserve the rest of the workflow behavior.Source: Linters/SAST tools
🤖 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 @.config/mise/tasks/acp-freshness:
- Around line 52-69: The heredoc in the drift issue body generation has
unescaped backticks that will be treated as command substitution, so the message
loses paths and filenames. Update the `cat >"$body_file" <<EOF` block to escape
every literal backtick consistently, matching the existing escaped entries, so
strings like `docs/architecture/acp-conformance.md`,
`acp-nats/src/nats/parsing.rs`, and `.github/workflows/acp-freshness.yml` are
preserved in the final body.
In `@docs/architecture/acp-conformance.md`:
- Around line 82-87: The ACP conformance table uses status labels like
“capabilities implemented” and “watch-only” that are not defined in the legend,
so update these rows to a documented legend value or extend the legend to
explicitly define the new statuses. Make the change in the architecture document
table entries for the relevant capability rows and ensure the legend and row
labels stay aligned with the existing status vocabulary used throughout the
document.
- Around line 40-42: Reclassify the providers rows in the conformance table so
the status reflects an SDK boundary limitation rather than missing routing:
`AgentHandler::list_providers`, `AgentHandler::set_provider`,
`AgentHandler::disable_provider`, and the NATS subjects (`providers.list`,
`providers.set`, `providers.disable`) are already implemented, but
`agent-client-protocol` 1.2.0 cannot represent these
`ClientRequest`/`JsonRpcRequest` variants. Update the `providers/list`,
`providers/set`, `providers/disable`, and payload-level Providers entries to use
`unrepresentable` (or split the upstream SDK note into a separate rationale) so
the label matches the actual blocker.
In `@rsworkspace/crates/acp-nats-server/src/connection.rs`:
- Around line 66-101: The cleanup in the `connect_agent_boundary` callback does
not run when EOF wins, so the spawned `client::run` task and notification
forwarder can survive past the WebSocket lifetime. Update the `connection.rs`
handler around `connect_agent_boundary`, `spawn_notification_forwarder`, and the
`tokio::task::spawn_local(client::run(...))` call to ensure both tasks are tied
to the callback’s lifetime, using an abort-on-drop guard or equivalent
cancellation wiring. Make sure the spawned client task is always aborted and
awaited when the boundary callback exits for any reason, not only in the current
shutdown branch.
In `@rsworkspace/crates/acp-nats/src/boundary/connect_agent_boundary.rs`:
- Around line 249-269: The connect_agent_boundary builder is missing the
AgentHandler provider RPC wiring, so list_providers, set_provider, and
disable_provider never get routed. Update connect_agent_boundary’s request
registration sequence alongside the existing route_request! calls for methods
like list_sessions and fork_session, and add route_request! entries for
AgentHandler’s list_providers, set_provider, and disable_provider so the bridge
exposes those provider operations.
In `@rsworkspace/crates/acp-nats/src/boundary/connect_agent_boundary/tests.rs`:
- Around line 188-300: Extend every_agent_method_is_routed_through_the_boundary
to include provider-related requests so the routing coverage matches the
boundary behavior. Add cases for providers/list, providers/set, and
providers/disable to the requests table using the corresponding request types
already used elsewhere in the test file, then update the expected results map
and assertions so these new ids are validated like the existing session and auth
methods. Use the request construction patterns in connect_agent_boundary::tests
and the method routing handled by ConnectAgentBoundary as the reference points.
In `@rsworkspace/crates/acp-nats/src/boundary/eof_signal_reader.rs`:
- Around line 29-37: The EOF signaling in eof_signal_reader::poll_read is
treating every Ok(0) as end-of-stream, which can wrongly fire on zero-length
reads. Update the poll_read logic to only send the signal when the buffer is
non-empty and the inner reader returns Ok(0) or an error, keeping the existing
signal.take() and send flow in place. Use the poll_read method and the signal
handling in EOFSignalReader as the target locations for this guard.
In `@rsworkspace/crates/acp-nats/src/client/elicitation_create.rs`:
- Around line 83-95: The request parsing in elicitation_create should preserve
the typed wire error and keep session validation separate. Update the
decode_request_params handling in ElicitationCreateRequest so InvalidRequest
carries the WireError directly instead of converting it through
serde_json::Error::custom, and introduce/use a dedicated error variant for the
scope.sessionId vs expected_session_id mismatch in the ElicitationCreateError
flow rather than fabricating a serde error for that validation failure.
In `@rsworkspace/crates/acp-nats/src/wire/tests.rs`:
- Around line 240-252: The test in
set_session_config_option_request_model_config_category_option_survives_round_trip
is asserting the local SessionConfigOption instead of anything produced by
encode_request/decode_request_params, so it does not verify a wire round-trip.
Update the test to assert only fields that actually exist on
SetSessionConfigOptionRequest after decoding (such as config_id and value), or
remove the category assertion entirely since SetSessionConfigOptionRequest has
no category field and that behavior is already covered by ConfigOptionUpdate.
- Around line 188-205: The round-trip test is using the wrong access pattern for
SessionInfoUpdate::title, since it is not an Option and cannot be taken. Update
session_update_session_info_update_survives_round_trip to inspect the decoded
SessionUpdate::SessionInfoUpdate value through the title accessor on
SessionInfoUpdate, and compare it directly against the expected "renamed
session" string. Keep the rest of the
encode_notification/decode_notification_params flow and the match on
SessionUpdate unchanged.
---
Nitpick comments:
In @.github/workflows/acp-freshness.yml:
- Around line 1-23: Add workflow concurrency to the ACP Freshness GitHub Actions
workflow so scheduled and manual runs cannot overlap and create duplicate drift
issues. Update the acp-freshness workflow to include a concurrency block for the
check job or workflow, using a stable group name and cancel-in-progress
behavior, so the existing check step and drift issue creation run only once at a
time.
- Line 17: Add persist-credentials: false to the actions/checkout step in the
ACP freshness workflow. Update the checkout configuration in the workflow job
that uses actions/checkout so it does not write the default GitHub token into
git config, since GH_TOKEN is already used for gh commands; keep the change
scoped to the checkout action and preserve the rest of the workflow behavior.
In `@rsworkspace/crates/acp-nats/src/agent/prompt/tests.rs`:
- Around line 169-178: The test setup duplicates the same SessionNotification
construction in prompt_js_notification_forwarding and
prompt_js_notification_receiver_dropped; extract that repeated
AgentThoughtChunk/ContentChunk/TextContent::new("thinking...") build into a
small helper such as make_thought_notification(session_id: &str) and update both
tests to use it. Keep the helper near the existing test utilities in tests.rs so
future schema changes only need one update.
In `@rsworkspace/crates/acp-nats/src/boundary/connection_client.rs`:
- Around line 14-16: The `ConnectionClient` doc and its
`ext_method`/`ext_notification` implementations conflict with the zero-cost
passthrough guideline because they still do request formatting, `UntypedMessage`
construction, error mapping, and response conversion. Update `ConnectionClient`
so it only forwards raw untyped SDK calls by introducing or using raw methods on
`ClientHandler` (such as `send_untyped_request`/`send_untyped_notification`) and
move all conversion logic into `client/ext.rs`, or else revise the doc comment
to explicitly call out the exception for ext methods. Keep `ext_method` and
`ext_notification` as thin wrappers around the underlying handler.
In `@rsworkspace/crates/acp-nats/src/client_proxy/tests.rs`:
- Around line 167-180: The `elicitation_create_publishes_to_correct_subject`
test only checks that `elicitation_create` succeeds, so it misses
deserialization regressions in `CreateElicitationResponse`. Update this test to
also inspect the returned value from `p.elicitation_create(...)` and assert the
expected response content, following the pattern used by neighboring tests like
`read_text_file_publishes_to_correct_subject`. Use the existing
`elicitation_create`, `CreateElicitationResponse`, and `proxy` symbols to locate
the test and add the content assertion.
In `@rsworkspace/crates/acp-nats/src/client/elicitation_complete.rs`:
- Around line 8-34: The handle function in elicitation_complete.rs records
metrics for decode failures but not for client forwarding failures. Update the
client.elicitation_complete error branch inside handle so it also calls
metrics.record_error with an appropriate category/reason before logging the
warning, keeping the telemetry consistent with the decode_failure path. Use the
existing handle, client.elicitation_complete, and metrics.record_error symbols
to place the fix.
🪄 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: 771cae0a-6898-4468-bd8f-3f004f129a72
⛔ Files ignored due to path filters (3)
rsworkspace/Cargo.lockis excluded by!**/*.lockrsworkspace/crates/trogon-semconv/src/gen/attribute.rsis excluded by!**/gen/**rsworkspace/crates/trogon-semconv/src/gen/span.rsis excluded by!**/gen/**
📒 Files selected for processing (129)
.config/mise/tasks/acp-freshness.github/workflows/acp-freshness.ymldocs/.vitepress/config.mtsdocs/adr/0020-acp-sdk-1x-boundary-and-bridge-traits.mddocs/adr/0021-typed-decode-over-passthrough-forwarding.mddocs/adr/index.mddocs/architecture/acp-conformance.mdotel/semconv/registry/acp.yamlrsworkspace/Cargo.tomlrsworkspace/crates/AGENTS.mdrsworkspace/crates/acp-nats-agent/Cargo.tomlrsworkspace/crates/acp-nats-agent/src/connection.rsrsworkspace/crates/acp-nats-agent/src/connection/tests.rsrsworkspace/crates/acp-nats-server/src/connection.rsrsworkspace/crates/acp-nats-server/src/connection/tests.rsrsworkspace/crates/acp-nats-server/src/tests.rsrsworkspace/crates/acp-nats-server/src/transport.rsrsworkspace/crates/acp-nats-stdio/src/main.rsrsworkspace/crates/acp-nats/Cargo.tomlrsworkspace/crates/acp-nats/src/agent/authenticate.rsrsworkspace/crates/acp-nats/src/agent/authenticate/tests.rsrsworkspace/crates/acp-nats/src/agent/bridge.rsrsworkspace/crates/acp-nats/src/agent/cancel.rsrsworkspace/crates/acp-nats/src/agent/cancel/tests.rsrsworkspace/crates/acp-nats/src/agent/close_session.rsrsworkspace/crates/acp-nats/src/agent/close_session/tests.rsrsworkspace/crates/acp-nats/src/agent/delete_session.rsrsworkspace/crates/acp-nats/src/agent/delete_session/tests.rsrsworkspace/crates/acp-nats/src/agent/ext_method.rsrsworkspace/crates/acp-nats/src/agent/ext_method/tests.rsrsworkspace/crates/acp-nats/src/agent/ext_notification.rsrsworkspace/crates/acp-nats/src/agent/ext_notification/tests.rsrsworkspace/crates/acp-nats/src/agent/fork_session.rsrsworkspace/crates/acp-nats/src/agent/fork_session/tests.rsrsworkspace/crates/acp-nats/src/agent/initialize.rsrsworkspace/crates/acp-nats/src/agent/initialize/tests.rsrsworkspace/crates/acp-nats/src/agent/js_request/tests.rsrsworkspace/crates/acp-nats/src/agent/list_sessions.rsrsworkspace/crates/acp-nats/src/agent/list_sessions/tests.rsrsworkspace/crates/acp-nats/src/agent/load_session.rsrsworkspace/crates/acp-nats/src/agent/load_session/tests.rsrsworkspace/crates/acp-nats/src/agent/logout.rsrsworkspace/crates/acp-nats/src/agent/logout/tests.rsrsworkspace/crates/acp-nats/src/agent/mod.rsrsworkspace/crates/acp-nats/src/agent/new_session.rsrsworkspace/crates/acp-nats/src/agent/new_session/tests.rsrsworkspace/crates/acp-nats/src/agent/prompt.rsrsworkspace/crates/acp-nats/src/agent/prompt/tests.rsrsworkspace/crates/acp-nats/src/agent/providers_disable.rsrsworkspace/crates/acp-nats/src/agent/providers_disable/tests.rsrsworkspace/crates/acp-nats/src/agent/providers_list.rsrsworkspace/crates/acp-nats/src/agent/providers_list/tests.rsrsworkspace/crates/acp-nats/src/agent/providers_set.rsrsworkspace/crates/acp-nats/src/agent/providers_set/tests.rsrsworkspace/crates/acp-nats/src/agent/resume_session.rsrsworkspace/crates/acp-nats/src/agent/resume_session/tests.rsrsworkspace/crates/acp-nats/src/agent/set_session_config_option.rsrsworkspace/crates/acp-nats/src/agent/set_session_config_option/tests.rsrsworkspace/crates/acp-nats/src/agent/set_session_mode.rsrsworkspace/crates/acp-nats/src/agent/set_session_mode/tests.rsrsworkspace/crates/acp-nats/src/agent/set_session_model/tests.rsrsworkspace/crates/acp-nats/src/agent/tests.rsrsworkspace/crates/acp-nats/src/agent_handler.rsrsworkspace/crates/acp-nats/src/agent_handler/tests.rsrsworkspace/crates/acp-nats/src/boundary/boundary_exit.rsrsworkspace/crates/acp-nats/src/boundary/connect_agent_boundary.rsrsworkspace/crates/acp-nats/src/boundary/connect_agent_boundary/tests.rsrsworkspace/crates/acp-nats/src/boundary/connection_client.rsrsworkspace/crates/acp-nats/src/boundary/eof_signal_reader.rsrsworkspace/crates/acp-nats/src/boundary/eof_signal_reader/tests.rsrsworkspace/crates/acp-nats/src/boundary/mod.rsrsworkspace/crates/acp-nats/src/client/elicitation_complete.rsrsworkspace/crates/acp-nats/src/client/elicitation_complete/tests.rsrsworkspace/crates/acp-nats/src/client/elicitation_create.rsrsworkspace/crates/acp-nats/src/client/elicitation_create/tests.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/client/fs_read_text_file.rsrsworkspace/crates/acp-nats/src/client/fs_read_text_file/tests.rsrsworkspace/crates/acp-nats/src/client/fs_write_text_file.rsrsworkspace/crates/acp-nats/src/client/fs_write_text_file/tests.rsrsworkspace/crates/acp-nats/src/client/mod.rsrsworkspace/crates/acp-nats/src/client/request_permission.rsrsworkspace/crates/acp-nats/src/client/request_permission/tests.rsrsworkspace/crates/acp-nats/src/client/session_update.rsrsworkspace/crates/acp-nats/src/client/session_update/tests.rsrsworkspace/crates/acp-nats/src/client/terminal_create.rsrsworkspace/crates/acp-nats/src/client/terminal_create/tests.rsrsworkspace/crates/acp-nats/src/client/terminal_kill.rsrsworkspace/crates/acp-nats/src/client/terminal_kill/tests.rsrsworkspace/crates/acp-nats/src/client/terminal_output.rsrsworkspace/crates/acp-nats/src/client/terminal_output/tests.rsrsworkspace/crates/acp-nats/src/client/terminal_release.rsrsworkspace/crates/acp-nats/src/client/terminal_release/tests.rsrsworkspace/crates/acp-nats/src/client/terminal_wait_for_exit.rsrsworkspace/crates/acp-nats/src/client/terminal_wait_for_exit/tests.rsrsworkspace/crates/acp-nats/src/client/tests.rsrsworkspace/crates/acp-nats/src/client_handler.rsrsworkspace/crates/acp-nats/src/client_handler/tests.rsrsworkspace/crates/acp-nats/src/client_proxy.rsrsworkspace/crates/acp-nats/src/client_proxy/tests.rsrsworkspace/crates/acp-nats/src/lib.rsrsworkspace/crates/acp-nats/src/nats/extensions.rsrsworkspace/crates/acp-nats/src/nats/parsing.rsrsworkspace/crates/acp-nats/src/nats/parsing/tests.rsrsworkspace/crates/acp-nats/src/nats/subjects/client_ops/elicitation_complete.rsrsworkspace/crates/acp-nats/src/nats/subjects/client_ops/elicitation_create.rsrsworkspace/crates/acp-nats/src/nats/subjects/client_ops/mod.rsrsworkspace/crates/acp-nats/src/nats/subjects/commands/delete.rsrsworkspace/crates/acp-nats/src/nats/subjects/commands/mod.rsrsworkspace/crates/acp-nats/src/nats/subjects/global/mod.rsrsworkspace/crates/acp-nats/src/nats/subjects/global/providers_disable.rsrsworkspace/crates/acp-nats/src/nats/subjects/global/providers_list.rsrsworkspace/crates/acp-nats/src/nats/subjects/global/providers_set.rsrsworkspace/crates/acp-nats/src/nats/subjects/stream.rsrsworkspace/crates/acp-nats/src/nats/subjects/tests.rsrsworkspace/crates/acp-nats/src/pending_prompt_waiters.rsrsworkspace/crates/acp-nats/src/pending_prompt_waiters/tests.rsrsworkspace/crates/acp-nats/src/session_id.rsrsworkspace/crates/acp-nats/src/session_id/tests.rsrsworkspace/crates/acp-nats/src/tests.rsrsworkspace/crates/acp-nats/src/wire.rsrsworkspace/crates/acp-nats/src/wire/tests.rsrsworkspace/dylints/trogon_lints/src/function_local_macro_rules.rsrsworkspace/dylints/trogon_lints/src/lib.rsrsworkspace/dylints/trogon_lints/ui/function_local_macro_rules.rsrsworkspace/dylints/trogon_lints/ui/function_local_macro_rules.stderr
💤 Files with no reviewable changes (1)
- rsworkspace/crates/acp-nats/src/agent/set_session_model/tests.rs
The function_local_use policy lint applies to test targets, and the coverage tests declared imports inside test bodies. Local verification had skipped --all-targets, which is why this reached CI. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… config test The elicitation handler laundered a typed WireError through a fabricated serde error and did the same for session-scope validation, against the typed-errors rule. The model_config request round-trip test asserted a local variable rather than decoded data and duplicated the boolean coverage; the real category round-trip lives in the ConfigOptionUpdate test. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
A zero-length poll_read conventionally returns Ok(0) without meaning end of stream, which would have torn the connection down as if the peer disconnected. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
When EOF wins the boundary select, main_fn is dropped before its cleanup block, so the client proxy loop and notification forwarder outlived their WebSocket on the shared LocalSet. Both are now tied to main_fn's lifetime with an abort-on-drop guard. The conformance matrix also gains legend entries for its two informal statuses, and providers move from unwired to unrepresentable since the blocker is the SDK's missing request impls, not missing routing. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…gh paths The lockfile refresh grew OutboundFrame past clippy's large-error threshold in a crate this branch never touched. The boundary coverage test sent mcp frames with wrong field names, so enum decoding failed upstream and the catch-all fallthrough lines were never executed even though the observable response looked identical. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The mcp/connect frame is now built by serializing the schema type so enum decoding cannot silently diverge, and both notification warn paths funnel through one helper that a synchronous unit test covers, removing the timing dependence that made per-run coverage flap. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
mcp/connect flows agent to client, so the client-to-agent request enum never matched it and the role default answered with the same error the assertion accepted. mcp/message is the one non-ext variant that can reach the catch-all; verified per-line with llvm-cov that the branch now executes. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
This was referenced Jul 27, 2026
yordis
added a commit
that referenced
this pull request
Jul 28, 2026
…#477) 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.