feat(acp-nats): complete ACP 0.10.2 agent method coverage#48
Merged
Conversation
yordis
added
the
rust:coverage-baseline-reset
PR SummaryMedium Risk Overview Adds generic client extension routing: parses Extends NATS subject helpers to cover the new agent methods and adds extensive tests around routing, error handling, metrics, and Written by Cursor Bugbot for commit 580eaf0. This will update automatically on new commits. Configure here. |
WalkthroughThis PR expands the acp-nats crate with six new session-related agent request handlers (list, fork, resume, close, set model, set config option), introduces client extension method support via JSON-RPC-like handling over NATS, adds corresponding NATS subject builders, and extends NATS parsing to recognize and route extension methods while enabling additional Cargo feature flags for the underlying protocol library. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bridge
participant Handler as Session Handler
participant NATS
participant Metrics
Client->>Bridge: list_sessions(args)
Bridge->>Handler: handle(bridge, args)
Handler->>Handler: Start elapsed timer
Handler->>NATS: request_with_timeout(subject)
alt NATS Success
NATS-->>Handler: ListSessionsResponse
Handler->>Handler: Parse response
Handler->>Metrics: record_request("list_sessions", elapsed, true)
Handler-->>Bridge: Ok(response)
else NATS Failure
NATS-->>Handler: Error
Handler->>Handler: map_nats_error()
Handler->>Metrics: record_request("list_sessions", elapsed, false)
Handler-->>Bridge: Err(protocol_error)
end
Bridge-->>Client: Result
sequenceDiagram
participant Client
participant Bridge as Bridge<N,C>
participant Handler as Session Handler<br/>(fork/resume)
participant NATS
participant Scheduler
participant Metrics
Client->>Bridge: fork_session(args) or resume_session(args)
Bridge->>Handler: handle(bridge, args)
Handler->>Handler: Validate session_id
Handler->>NATS: request_with_timeout(subject)
alt Session ID Invalid
Handler->>Metrics: record_error(session_validate)
Handler-->>Bridge: Err(InvalidParams)
else NATS Success
NATS-->>Handler: ForkSessionResponse/<br/>ResumeSessionResponse
Handler->>Scheduler: schedule_session_ready(new_session_id)
Scheduler->>NATS: publish(ready_subject)
Handler->>Metrics: record_request(method, elapsed, true)
Handler-->>Bridge: Ok(response)
else NATS Failure
NATS-->>Handler: Error
Handler->>Metrics: record_request(method, elapsed, false)
Handler-->>Bridge: Err(protocol_error)
end
Bridge-->>Client: Result
sequenceDiagram
participant Client as NATS Client
participant Dispatcher as client/mod
participant ExtHandler as client/ext
participant AppClient as Application<br/>Client
participant NATS as NATS<br/>Broker
Client->>Dispatcher: dispatch(ClientMethod::Ext(name), payload, reply)
Dispatcher->>ExtHandler: handle(payload, app_client, reply, ...)
alt Has Reply (Request-Response)
ExtHandler->>ExtHandler: Parse ExtRequest (with request_id)
ExtHandler->>AppClient: ext_method(name, params)
alt Client Success
AppClient-->>ExtHandler: Response
ExtHandler->>ExtHandler: Serialize result
ExtHandler->>NATS: publish(reply, success_response)
else Client Error
AppClient-->>ExtHandler: Err
ExtHandler->>NATS: publish(reply, error_response)
end
else No Reply (Notification)
ExtHandler->>ExtHandler: Parse ExtNotification
ExtHandler->>AppClient: ext_notification(name, params)
AppClient-->>ExtHandler: (fire-and-forget)
end
ExtHandler->>NATS: flush()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
yordis
removed
the
rust:coverage-baseline-reset
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 580eaf0 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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rsworkspace/crates/acp-nats/src/agent/resume_session.rs (1)
232-319: Replace the fixed sleeps with deterministic task draining.The waits on Lines 244, 266, and 308 couple these tests to
SESSION_READY_DELAYand publish-retry timing, which makes them slower and eventually flaky.Bridge::drain_background_tasks()is already the right barrier for the publish/error-path cases, and the request-metric assertion on Lines 254-274 does not need to wait at all becauserecord_requestruns beforeresume_sessionreturns.⏱️ Suggested cleanup
- tokio::time::sleep(Duration::from_millis(300)).await; + bridge.drain_background_tasks().await; let published = mock.published_messages(); @@ - tokio::time::sleep(Duration::from_millis(150)).await; provider.force_flush().unwrap(); @@ - tokio::time::sleep(Duration::from_millis(600)).await; + bridge.drain_background_tasks().await; provider.force_flush().unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/resume_session.rs` around lines 232 - 319, Replace the fixed tokio::time::sleep waits with deterministic task draining: in resume_session_publishes_session_ready_to_correct_subject and resume_session_records_error_when_session_ready_publish_fails call bridge.drain_background_tasks() instead of sleeping to wait for publish/retry completion; in resume_session_records_metrics_on_success remove the sleep entirely (the request metric is recorded synchronously before resume_session returns) and proceed to provider.force_flush() and assertions; keep provider.force_flush()/shutdown where present and ensure you call bridge.drain_background_tasks() only where publish background work must complete.rsworkspace/crates/acp-nats/src/nats/subjects.rs (1)
53-75: Stop widening the raw&strsubject API.These new builders take primitive
prefix/session_idvalues even though the crate already has validated domain values likeAcpSessionId. Accepting the value objects here would make invalid subject parts unrepresentable and remove the need for every caller to remember pre-validation.As per coding guidelines, "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction—invalid instances should be unrepresentable."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/nats/subjects.rs` around lines 53 - 75, The subject builder functions (session_list, session_set_config_option, session_set_model, session_fork, session_resume, session_close) currently accept raw &str for prefix and session_id; change their signatures to accept the domain value objects (e.g., AcpPrefix for prefix and AcpSessionId for session_id) so invalid parts are unrepresentable, and build the subject string from those types (use their accessor method, e.g., as_str() or to_string()) when calling format!; update all usages to pass the domain types instead of raw &str.rsworkspace/crates/acp-nats/src/agent/fork_session.rs (1)
248-252: Preferdrain_background_tasks()over fixed sleeps in these tests.These waits are coupled to the current
session.readydelay and publish-retry timing. SinceBridgealready tracks the spawned background work, awaitingbridge.drain_background_tasks().awaithere will make the assertions deterministic and cut CI flakiness.♻️ Suggested change
- tokio::time::sleep(Duration::from_millis(300)).await; + bridge.drain_background_tasks().await; let published = mock.published_messages(); @@ - tokio::time::sleep(Duration::from_millis(150)).await; + bridge.drain_background_tasks().await; provider.force_flush().unwrap(); @@ - tokio::time::sleep(Duration::from_millis(600)).await; + bridge.drain_background_tasks().await; provider.force_flush().unwrap();Also applies to: 270-275, 312-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs` around lines 248 - 252, Replace the fixed tokio::time::sleep(Duration::from_millis(...)).await waits after calling bridge.fork_session(ForkSessionRequest::new(...)).await with an await on bridge.drain_background_tasks().await so the test waits for Bridge-tracked background work instead of timing assumptions; update the occurrences around the fork_session calls (including the similar blocks at the other noted locations) to call bridge.drain_background_tasks().await immediately after the fork_session await to make assertions deterministic.
🤖 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/src/agent/close_session.rs`:
- Around line 22-47: The session ID validation path in the
AcpSessionId::try_from call returns early on error and only records a
"session_validate" metric, so malformed IDs never get counted by
bridge.metrics.record_request for "close_session"; update the error handling in
the AcpSessionId::try_from map_err closure (the block around
AcpSessionId::try_from(&args.session_id)) to also call
bridge.metrics.record_request("close_session",
bridge.clock.elapsed(start).as_secs_f64(), false) before returning the Error so
that every request path (including validation failures) logs a failed
close_session request.
In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs`:
- Around line 43-47: The success branch currently treats response.session_id as
valid without converting it into the domain type; change the flow in the
Ok(result) branch to attempt constructing/validating an AcpSessionId (using its
try_from/parse/factory) from response.session_id first, and only upon successful
creation record the id via Span::current().record, call info!(new_session_id =
%session_id, ...) and call bridge.schedule_session_ready with the validated
AcpSessionId; if validation fails, emit an error log (including the raw
response.session_id and the validation error) and do not call
schedule_session_ready so we never publish against an invalid subject. Ensure
you update uses of response.session_id in this block to the validated variable
and keep scheduling limited to the typed AcpSessionId.
---
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs`:
- Around line 248-252: Replace the fixed
tokio::time::sleep(Duration::from_millis(...)).await waits after calling
bridge.fork_session(ForkSessionRequest::new(...)).await with an await on
bridge.drain_background_tasks().await so the test waits for Bridge-tracked
background work instead of timing assumptions; update the occurrences around the
fork_session calls (including the similar blocks at the other noted locations)
to call bridge.drain_background_tasks().await immediately after the fork_session
await to make assertions deterministic.
In `@rsworkspace/crates/acp-nats/src/agent/resume_session.rs`:
- Around line 232-319: Replace the fixed tokio::time::sleep waits with
deterministic task draining: in
resume_session_publishes_session_ready_to_correct_subject and
resume_session_records_error_when_session_ready_publish_fails call
bridge.drain_background_tasks() instead of sleeping to wait for publish/retry
completion; in resume_session_records_metrics_on_success remove the sleep
entirely (the request metric is recorded synchronously before resume_session
returns) and proceed to provider.force_flush() and assertions; keep
provider.force_flush()/shutdown where present and ensure you call
bridge.drain_background_tasks() only where publish background work must
complete.
In `@rsworkspace/crates/acp-nats/src/nats/subjects.rs`:
- Around line 53-75: The subject builder functions (session_list,
session_set_config_option, session_set_model, session_fork, session_resume,
session_close) currently accept raw &str for prefix and session_id; change their
signatures to accept the domain value objects (e.g., AcpPrefix for prefix and
AcpSessionId for session_id) so invalid parts are unrepresentable, and build the
subject string from those types (use their accessor method, e.g., as_str() or
to_string()) when calling format!; update all usages to pass the domain types
instead of raw &str.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7495593b-446e-465a-ad0b-46b552627512
📒 Files selected for processing (9)
rsworkspace/crates/acp-nats/Cargo.tomlrsworkspace/crates/acp-nats/src/agent/close_session.rsrsworkspace/crates/acp-nats/src/agent/fork_session.rsrsworkspace/crates/acp-nats/src/agent/list_sessions.rsrsworkspace/crates/acp-nats/src/agent/mod.rsrsworkspace/crates/acp-nats/src/agent/resume_session.rsrsworkspace/crates/acp-nats/src/agent/set_session_config_option.rsrsworkspace/crates/acp-nats/src/agent/set_session_model.rsrsworkspace/crates/acp-nats/src/nats/subjects.rs
yordis
force-pushed
the
feat/acp-complete-agent-coverage
branch
from
28bb5b8 to
1486981
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
feat/acp-complete-agent-coverage
branch
from
1486981 to
e125286
Compare
yordis
force-pushed
the
feat/acp-complete-agent-coverage
branch
from
e125286 to
cfd8df4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
rsworkspace/crates/acp-nats/src/agent/fork_session.rs (1)
43-47:⚠️ Potential issue | 🟠 MajorValidate the forked session ID before treating this as success.
response.session_idnever crossesAcpSessionId, so this branch recordsnew_session_idand schedulessession.readyfor whatever the agent returned. Because the publish is fire-and-forget, a malformed ID becomes a silent readiness miss unless you validate it here or makeschedule_session_readyaccept a validated type.As per coding guidelines, "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction—invalid instances should be unrepresentable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs` around lines 43 - 47, The code treats response.session_id as valid without converting it into the domain type, so a malformed ID can be silently accepted; update the branch handling Ok(response) to validate/construct an AcpSessionId (e.g., via AcpSessionId::try_from or a parse/try_new factory) from response.session_id before recording it with Span::current().record, logging it with info!, and calling bridge.schedule_session_ready; on validation failure, log an error (with the invalid raw value) and do not call bridge.schedule_session_ready so only valid AcpSessionId instances are scheduled.rsworkspace/crates/acp-nats/src/agent/close_session.rs (1)
22-30:⚠️ Potential issue | 🟠 MajorCount invalid session IDs as failed requests.
Line 30 returns before Lines 43-47, so malformed IDs only emit
session_validateand never increment failedclose_sessionrequests. That skewsacp.requests, and the same copied pattern is still present inset_session_config_option.rs,set_session_model.rs,fork_session.rs, andresume_session.rs.Also applies to: 43-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/close_session.rs` around lines 22 - 30, The invalid-session-id branch in close_session (where AcpSessionId::try_from(&args.session_id) maps Err to Error::new with ErrorCode::InvalidParams) currently only calls bridge.metrics.record_error("session_validate", "invalid_session_id") and returns early, so it never increments the failed "close_session" request metric; update that error handler to also increment the close_session failure metric (e.g., call bridge.metrics.record_error("close_session", "invalid_session_id") or the appropriate failed-request counter) before returning the Error, and apply the same change to the analogous handlers in set_session_config_option.rs, set_session_model.rs, fork_session.rs, and resume_session.rs to ensure malformed IDs count as failed requests.
🧹 Nitpick comments (2)
rsworkspace/crates/acp-nats/src/nats/parsing.rs (1)
36-40: Consider storingExtMethodNameinstead of rawString.The code validates via
ExtMethodName::new()but then discards the value object and stores only the raw string. Per coding guidelines, domain-specific value objects should be preferred over primitives. StoringExtMethodNamedirectly would preserve the validation guarantee downstream.That said, this is a minor point since validation does occur, and the string is immediately used for forwarding.
♻️ Optional refactor to preserve the value object
#[derive(Debug, Clone, PartialEq, Eq)] pub enum ClientMethod { // ... other variants - Ext(String), + Ext(ExtMethodName), } impl ClientMethod { pub fn from_subject_suffix(suffix: &str) -> Option<Self> { match suffix { // ... other cases other => { let ext_name = other.strip_prefix(EXT_METHOD_SUBJECT_PREFIX)?; - ExtMethodName::new(ext_name).ok()?; - Some(Self::Ext(ext_name.to_string())) + let validated = ExtMethodName::new(ext_name).ok()?; + Some(Self::Ext(validated)) } } } }As per coding guidelines: "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/nats/parsing.rs` around lines 36 - 40, The branch validates the subject by calling ExtMethodName::new(ext_name) but then throws away the value object and stores a raw String in Self::Ext(ext_name.to_string()); change this to construct and store the ExtMethodName instance instead (e.g., create the ExtMethodName via ExtMethodName::new(ext_name)? and return Some(Self::Ext(<the ExtMethodName>))) so downstream code receives the validated domain value object rather than a plain String; update the Ext variant/constructor to accept ExtMethodName if necessary and adjust usages that expect a String.rsworkspace/crates/acp-nats/src/agent/fork_session.rs (1)
252-252: Usedrain_background_tasks()instead of fixed sleeps in these tests.The 150/300/600 ms waits are coupled to
SESSION_READY_DELAYand scheduler latency.Bridge::drain_background_tasks()already gives these assertions a deterministic completion point, and the same sleep-based pattern is repeated inresume_session.rs.Also applies to: 274-274, 316-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs` at line 252, Replace the fixed sleep calls that are waiting for SESSION_READY_DELAY and scheduler latency with a deterministic drain of background tasks: call Bridge::drain_background_tasks() (e.g., bridge.drain_background_tasks().await) instead of tokio::time::sleep(Duration::from_millis(...)).await in the tests in fork_session.rs; do the same for the other identical sleep occurrences in this file and replicate the change in resume_session.rs so assertions wait on drain_background_tasks() rather than timing-based sleeps.
🤖 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/src/agent/resume_session.rs`:
- Around line 24-34: The code validates args.session_id into an AcpSessionId but
later publishes using the original args.session_id, which can diverge if
AcpSessionId canonicalizes; update the publish path to use the validated
AcpSessionId (use session_id.as_str() when building the subject via
agent::session_resume and when publishing the "session.ready" message on
bridge.nats()) so both the request subject and follow-up "session.ready" subject
use the canonical ID from AcpSessionId rather than the raw args.session_id.
---
Duplicate comments:
In `@rsworkspace/crates/acp-nats/src/agent/close_session.rs`:
- Around line 22-30: The invalid-session-id branch in close_session (where
AcpSessionId::try_from(&args.session_id) maps Err to Error::new with
ErrorCode::InvalidParams) currently only calls
bridge.metrics.record_error("session_validate", "invalid_session_id") and
returns early, so it never increments the failed "close_session" request metric;
update that error handler to also increment the close_session failure metric
(e.g., call bridge.metrics.record_error("close_session", "invalid_session_id")
or the appropriate failed-request counter) before returning the Error, and apply
the same change to the analogous handlers in set_session_config_option.rs,
set_session_model.rs, fork_session.rs, and resume_session.rs to ensure malformed
IDs count as failed requests.
In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs`:
- Around line 43-47: The code treats response.session_id as valid without
converting it into the domain type, so a malformed ID can be silently accepted;
update the branch handling Ok(response) to validate/construct an AcpSessionId
(e.g., via AcpSessionId::try_from or a parse/try_new factory) from
response.session_id before recording it with Span::current().record, logging it
with info!, and calling bridge.schedule_session_ready; on validation failure,
log an error (with the invalid raw value) and do not call
bridge.schedule_session_ready so only valid AcpSessionId instances are
scheduled.
---
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs`:
- Line 252: Replace the fixed sleep calls that are waiting for
SESSION_READY_DELAY and scheduler latency with a deterministic drain of
background tasks: call Bridge::drain_background_tasks() (e.g.,
bridge.drain_background_tasks().await) instead of
tokio::time::sleep(Duration::from_millis(...)).await in the tests in
fork_session.rs; do the same for the other identical sleep occurrences in this
file and replicate the change in resume_session.rs so assertions wait on
drain_background_tasks() rather than timing-based sleeps.
In `@rsworkspace/crates/acp-nats/src/nats/parsing.rs`:
- Around line 36-40: The branch validates the subject by calling
ExtMethodName::new(ext_name) but then throws away the value object and stores a
raw String in Self::Ext(ext_name.to_string()); change this to construct and
store the ExtMethodName instance instead (e.g., create the ExtMethodName via
ExtMethodName::new(ext_name)? and return Some(Self::Ext(<the ExtMethodName>)))
so downstream code receives the validated domain value object rather than a
plain String; update the Ext variant/constructor to accept ExtMethodName if
necessary and adjust usages that expect a String.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e005c95d-a870-4935-baa7-bc9369268c93
📒 Files selected for processing (13)
rsworkspace/crates/acp-nats/Cargo.tomlrsworkspace/crates/acp-nats/src/agent/close_session.rsrsworkspace/crates/acp-nats/src/agent/fork_session.rsrsworkspace/crates/acp-nats/src/agent/list_sessions.rsrsworkspace/crates/acp-nats/src/agent/mod.rsrsworkspace/crates/acp-nats/src/agent/resume_session.rsrsworkspace/crates/acp-nats/src/agent/set_session_config_option.rsrsworkspace/crates/acp-nats/src/agent/set_session_model.rsrsworkspace/crates/acp-nats/src/client/ext_method.rsrsworkspace/crates/acp-nats/src/client/ext_notification.rsrsworkspace/crates/acp-nats/src/client/mod.rsrsworkspace/crates/acp-nats/src/nats/parsing.rsrsworkspace/crates/acp-nats/src/nats/subjects.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/acp-nats/Cargo.toml
- rsworkspace/crates/acp-nats/src/nats/subjects.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rsworkspace/crates/acp-nats/src/agent/fork_session.rs (1)
43-47:⚠️ Potential issue | 🟠 MajorStill validate the returned
session_idbefore publishingsession.ready.
response.session_idcomes back from NATS and is used here without crossing anAcpSessionIdboundary. If the agent returns an invalid ID, this branch still logs success and schedulessession.readyon a bad subject; because that publish is async, the caller only notices later via a missing readiness event. Validate the response ID first, then record/log/schedule from the typed value.♻️ Minimal fix
-use tracing::{Span, info, instrument}; +use tracing::{Span, info, instrument, warn}; @@ - if let Ok(ref response) = result { - Span::current().record("new_session_id", response.session_id.to_string().as_str()); - info!(new_session_id = %response.session_id, "Session forked"); - - bridge.schedule_session_ready(response.session_id.clone()); - } + if let Ok(ref response) = result { + match AcpSessionId::try_from(&response.session_id) { + Ok(new_session_id) => { + Span::current().record("new_session_id", new_session_id.as_str()); + info!(new_session_id = new_session_id.as_str(), "Session forked"); + bridge.schedule_session_ready(agent_client_protocol::SessionId::from( + new_session_id.as_str(), + )); + } + Err(e) => { + warn!( + raw_session_id = %response.session_id, + error = %e, + "Ignoring invalid forked session ID from agent" + ); + } + } + }As per coding guidelines, "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction—invalid instances should be unrepresentable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs` around lines 43 - 47, Validate the returned response.session_id by attempting to construct the domain type (e.g., AcpSessionId::try_from or the crate's constructor) before any side effects: try to parse/construct an AcpSessionId from response.session_id.clone(), and if that fails log an error (with the invalid raw ID) and return/skip scheduling; only after successful construction use the typed AcpSessionId value for Span::current().record, the info! log, and bridge.schedule_session_ready so the published session.ready uses a guaranteed-valid subject.
🧹 Nitpick comments (1)
rsworkspace/crates/acp-nats/src/client/ext.rs (1)
59-65: Keep ext method names typed after parsing.
rsworkspace/crates/acp-nats/src/nats/parsing.rs:23-42already validates extension names withExtMethodName::new(...), but this handler drops back to a raw&str. ThreadingExtMethodNamethroughClientMethod::Ext(...)andext::handle(...)would keep invalid names unrepresentable past the parsing boundary.As per coding guidelines, "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction—invalid instances should be unrepresentable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/client/ext.rs` around lines 59 - 65, The handler currently accepts ext_method_name as a raw &str—make the extension name a typed domain value by changing the parameter to accept ExtMethodName (e.g., &ExtMethodName or ExtMethodName) in ext::handle and propagate that type through ClientMethod::Ext so parsed/validated names become unrepresentable when invalid; update all call sites that construct ClientMethod::Ext and invocations of ext::handle to pass the validated ExtMethodName (or a reference), and adjust any pattern matches or uses of the name inside handle to use the ExtMethodName API instead of string operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rsworkspace/crates/acp-nats/src/agent/fork_session.rs`:
- Around line 43-47: Validate the returned response.session_id by attempting to
construct the domain type (e.g., AcpSessionId::try_from or the crate's
constructor) before any side effects: try to parse/construct an AcpSessionId
from response.session_id.clone(), and if that fails log an error (with the
invalid raw ID) and return/skip scheduling; only after successful construction
use the typed AcpSessionId value for Span::current().record, the info! log, and
bridge.schedule_session_ready so the published session.ready uses a
guaranteed-valid subject.
---
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/client/ext.rs`:
- Around line 59-65: The handler currently accepts ext_method_name as a raw
&str—make the extension name a typed domain value by changing the parameter to
accept ExtMethodName (e.g., &ExtMethodName or ExtMethodName) in ext::handle and
propagate that type through ClientMethod::Ext so parsed/validated names become
unrepresentable when invalid; update all call sites that construct
ClientMethod::Ext and invocations of ext::handle to pass the validated
ExtMethodName (or a reference), and adjust any pattern matches or uses of the
name inside handle to use the ExtMethodName API instead of string operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cb4c416-f012-4e56-b541-3823843d1341
📒 Files selected for processing (12)
rsworkspace/crates/acp-nats/Cargo.tomlrsworkspace/crates/acp-nats/src/agent/close_session.rsrsworkspace/crates/acp-nats/src/agent/fork_session.rsrsworkspace/crates/acp-nats/src/agent/list_sessions.rsrsworkspace/crates/acp-nats/src/agent/mod.rsrsworkspace/crates/acp-nats/src/agent/resume_session.rsrsworkspace/crates/acp-nats/src/agent/set_session_config_option.rsrsworkspace/crates/acp-nats/src/agent/set_session_model.rsrsworkspace/crates/acp-nats/src/client/ext.rsrsworkspace/crates/acp-nats/src/client/mod.rsrsworkspace/crates/acp-nats/src/nats/parsing.rsrsworkspace/crates/acp-nats/src/nats/subjects.rs
✅ Files skipped from review due to trivial changes (2)
- rsworkspace/crates/acp-nats/Cargo.toml
- rsworkspace/crates/acp-nats/src/agent/set_session_model.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rsworkspace/crates/acp-nats/src/nats/subjects.rs
- rsworkspace/crates/acp-nats/src/nats/parsing.rs
- rsworkspace/crates/acp-nats/src/agent/close_session.rs
yordis
force-pushed
the
feat/acp-complete-agent-coverage
branch
from
cfd8df4 to
c6409a0
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
feat/acp-complete-agent-coverage
branch
from
c6409a0 to
580eaf0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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.
yordis
added
the
rust:coverage-baseline-reset
This was referenced Mar 25, 2026
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.
Summary
ext_methodandext_notificationhandlersFeature flags enabled
unstable_session_modelunstable_session_forkunstable_session_resumeunstable_session_usageunstable_session_closeunstable_message_idunstable_auth_methodsunstable_cancel_requestunstable_boolean_configNew agent handlers
list_sessionsinitializeset_session_config_optionset_session_modeset_session_modelset_session_modefork_sessionnew_sessionsession.ready(new session ID)resume_sessionload_sessionsession.ready(request session ID)close_sessionset_session_modeNew client handlers
ext_methodfs_read_text_file(request/response)_-prefixed ext RPC calls to clientext_notificationsession_update(fire-and-forget)_-prefixed ext notifications to clientACP uses
_prefix on JSON-RPC method names for extensions (e.g.,_my_method). These map to NATS subjects like{prefix}.{sid}.client._my_method. Previously these were silently dropped; now they're routed to theClienttrait'sext_method/ext_notificationbased on whether a reply subject is present.NATS subject mapping (agent)
session/list{prefix}.agent.session.listsession/setConfigOption{prefix}.{sid}.agent.session.set_config_optionsession/setModel{prefix}.{sid}.agent.session.set_modelsession/fork{prefix}.{sid}.agent.session.forksession/resume{prefix}.{sid}.agent.session.resumesession/close{prefix}.{sid}.agent.session.closeTest plan
cargo check --workspacecargo clippy --workspace --all-targetscargo test --workspace— 492 tests pass (75 new)cargo fmt --all -- --check