Conversation
PR SummaryMedium Risk Overview Expands Adds the Written by Cursor Bugbot for commit ca5db16. This will update automatically on new commits. 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:
WalkthroughAdds a full prompt request/response flow: new prompt handler, per-session waiter registry, prompt concurrency controls, session prompt NATS subject helper, prompt-related config, integration into Bridge, and related metric/test adjustments including cancel handling tweaks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Bridge as Agent Bridge
participant SlotMgr as Slot Manager
participant NATS
participant Waiters as Waiter Registry
participant Backend
Client->>Bridge: prompt(request)
activate Bridge
Bridge->>SlotMgr: try_acquire_prompt_slot()
SlotMgr-->>Bridge: slot acquired
Bridge->>Bridge: validate session_id / check cancelled
Bridge->>NATS: publish(<prefix>.<sid>.agent.session.prompt, request)
NATS-->>Bridge: publish result
Bridge->>Waiters: register_waiter(session_id)
Note over Bridge: await response with timeout
deactivate Bridge
Backend->>NATS: receive prompt subject, produce response
NATS->>Bridge: deliver message -> deliver_prompt_response(session_id, response)
activate Bridge
Bridge->>Waiters: resolve_waiter(session_id, response)
Waiters-->>Bridge: oneshot notified
Bridge-->>Client: PromptResponse / error (with StopReason)
Bridge->>SlotMgr: release_prompt_slot()
deactivate Bridge
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 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rsworkspace/crates/acp-nats/src/agent/prompt.rs (1)
101-109: Add an explicit error metric on prompt publish failure.This branch logs and returns
InternalError, but it currently does not emitacp.errors.totallike other prompt failure paths. Emitting one here will keep error observability consistent.Proposed fix
if let Err(e) = nats::publish(nats, &subject, &args, publish_options).await { bridge .pending_session_prompt_responses .remove_waiter(&args.session_id); warn!(session_id = %args.session_id, error = %e, "Failed to publish prompt request"); + bridge.metrics.record_error("prompt", "prompt_publish_failed"); return Err(Error::new( ErrorCode::InternalError.into(), format!("Failed to publish prompt request: {}", e), )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/prompt.rs` around lines 101 - 109, The publish failure branch that calls nats::publish in prompt.rs removes the waiter and logs a warning but does not increment the error metric; add an increment of the acp.errors.total metric (same metric used on other prompt failure paths) immediately after bridge.pending_session_prompt_responses.remove_waiter(&args.session_id) and before returning Err so failures are counted; modify the branch around nats::publish to call the metric increment helper (or metrics::increment_counter! / equivalent used elsewhere) and keep the existing warn! and Err return intact.rsworkspace/crates/acp-nats/src/nats/subjects.rs (1)
18-20: Use domain value objects forsession_promptinputs.This helper exposes raw
&strfor both prefix and session ID, so invalid subject tokens can still be constructed at this boundary if a caller bypasses upstream checks. Prefer typed inputs (AcpPrefix,AcpSessionId) for this new subject helper as well.
As per coding guidelines:Prefer domain-specific value objects over primitives (e.g., AcpPrefix not String). Each type's factory must guarantee 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 18 - 20, Replace the primitive &str parameters on session_prompt with the domain value objects so invalid tokens cannot be constructed: change pub fn session_prompt(prefix: &str, session_id: &str) -> String to take AcpPrefix and AcpSessionId (e.g., pub fn session_prompt(prefix: &AcpPrefix, session_id: &AcpSessionId) -> String), use their canonical string accessor (e.g., prefix.as_str() or prefix.to_string()/into_inner() depending on the type API) when formatting the subject, update callers to pass AcpPrefix/AcpSessionId instances, and add the necessary use/import for AcpPrefix and AcpSessionId so the module compiles.
🤖 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/cancel.rs`:
- Around line 1-7: The file fails Rustfmt; run rustfmt across the workspace
(e.g., cargo fmt --all) or format
rsworkspace/crates/acp-nats/src/agent/cancel.rs and related modules so imports
and use statements (e.g., Bridge, CancelNotification, PromptResponse,
StopReason, agent_client_protocol types) conform to rustfmt style; commit the
formatted changes so the CI formatting check passes.
In `@rsworkspace/crates/acp-nats/src/agent/mod.rs`:
- Around line 204-206: has_pending_session_ready_tasks can return true due to
stale finished join handles left in session_ready_publish_tasks; before checking
emptiness prune completed handles (those where JoinHandle::is_finished() is
true). Update has_pending_session_ready_tasks (or extract a small helper used by
both) to borrow_mut() the session_ready_publish_tasks and retain only handles
where !handle.is_finished(), then return !is_empty(); reference the
functions/fields: has_pending_session_ready_tasks, register_session_ready_task,
and session_ready_publish_tasks.
In `@rsworkspace/crates/acp-nats/src/agent/prompt.rs`:
- Around line 46-52: The code calls bridge.metrics.record_request twice for
invalid session IDs: once inside the AcpSessionId::try_from(...).map_err(...)
closure and again at the function end; remove the duplicated record_request from
the map_err closure so map_err only records the error
(bridge.metrics.record_error) and returns the mapped error, letting the existing
final bridge.metrics.record_request call (with success=false) handle request
counting and timing. Apply the same change for the similar map_err block at the
other occurrence (lines around the second AcpSessionId::try_from use).
---
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/agent/prompt.rs`:
- Around line 101-109: The publish failure branch that calls nats::publish in
prompt.rs removes the waiter and logs a warning but does not increment the error
metric; add an increment of the acp.errors.total metric (same metric used on
other prompt failure paths) immediately after
bridge.pending_session_prompt_responses.remove_waiter(&args.session_id) and
before returning Err so failures are counted; modify the branch around
nats::publish to call the metric increment helper (or
metrics::increment_counter! / equivalent used elsewhere) and keep the existing
warn! and Err return intact.
In `@rsworkspace/crates/acp-nats/src/nats/subjects.rs`:
- Around line 18-20: Replace the primitive &str parameters on session_prompt
with the domain value objects so invalid tokens cannot be constructed: change
pub fn session_prompt(prefix: &str, session_id: &str) -> String to take
AcpPrefix and AcpSessionId (e.g., pub fn session_prompt(prefix: &AcpPrefix,
session_id: &AcpSessionId) -> String), use their canonical string accessor
(e.g., prefix.as_str() or prefix.to_string()/into_inner() depending on the type
API) when formatting the subject, update callers to pass AcpPrefix/AcpSessionId
instances, and add the necessary use/import for AcpPrefix and AcpSessionId so
the module compiles.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rsworkspace/crates/acp-nats/src/agent/cancel.rsrsworkspace/crates/acp-nats/src/agent/mod.rsrsworkspace/crates/acp-nats/src/agent/prompt.rsrsworkspace/crates/acp-nats/src/config.rsrsworkspace/crates/acp-nats/src/nats/subjects.rs
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
92ef9e2 to
753ac4a
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
753ac4a to
0952857
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
0952857 to
24679d9
Compare
Code Coverage SummaryDetailsDiff against mainResults for commit: ca5db16 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
24679d9 to
92fc3ba
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
92fc3ba to
428aa3a
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
428aa3a to
1e9de54
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
1e9de54 to
8d4c507
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
8d4c507 to
dbab690
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rsworkspace/crates/acp-nats/src/agent/prompt.rs (1)
49-55:⚠️ Potential issue | 🟠 Major
acp.request.countis double-recorded for invalid session IDsLine 50-54 records a failed request inside
map_err, and Line 182-186 records the request again for the same path. This inflates failed prompt counts.💡 Suggested fix
let session_id = AcpSessionId::try_from(&args.session_id).map_err(|e| { - bridge.metrics.record_request( - "prompt", - bridge.clock.elapsed(start).as_secs_f64(), - false, - ); bridge.metrics.record_error("prompt", "invalid_session_id"); Error::new( ErrorCode::InvalidParams.into(), format!("Invalid session ID: {}", e), ) })?;Also applies to: 182-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/prompt.rs` around lines 49 - 55, The code double-records failed requests when an invalid session ID occurs because AcpSessionId::try_from(&args.session_id).map_err(...) records a failed request via bridge.metrics.record_request/record_error inside the map_err closure and the request is also recorded later (same path around the second block that calls bridge.metrics.record_request for the prompt); remove the duplicate recording by moving metric/error recording out of the map_err closure and instead return the mapped error there, then ensure the single bridge.metrics.record_request and bridge.metrics.record_error call path (the existing later block that handles errors for the prompt) is the sole place that records metrics for this error, or alternatively make the map_err closure only construct/return the error without invoking bridge.metrics.* so the later error handler records it once.
🧹 Nitpick comments (2)
rsworkspace/crates/AGENTS.md (1)
3-3: Consider clarifying the export pattern for infallible or generic-error value objects.The prescribed export pattern
pub use {module}::{Type, TypeError}assumes every value object has an associatedTypeError. However, some value objects might be infallible (e.g., validated at compile time via newtypes) or use generic error types likeanyhow::Error.Consider adding guidance on when to export just
Typeor alternative patterns for different error-handling strategies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/AGENTS.md` at line 3, The guidance currently assumes every value object exports both Type and TypeError (pub use {module}::{Type, TypeError}), but some value objects are infallible (newtypes) or use generic error types (e.g., anyhow::Error); update AGENTS.md to document export variants: 1) for infallible/newtype value objects export only Type (pub use {module}::Type), 2) for objects with a module-local error type export both Type and TypeError as before, and 3) for objects that use generic/shared error types document exporting Type and optionally re-exporting the shared error alias (e.g., pub use {module}::Type and pub use crate::errors::SharedError as TypeError) or note not to re-export concrete anyhow::Error; reference the file layout rule and lib.rs export examples (Type, TypeError) and add a short decision rule for which pattern to use.rsworkspace/crates/acp-nats/src/nats/subjects.rs (1)
18-20: Use value-object inputs forsession_prompt
session_promptaccepts raw&strfor both prefix and session id, which weakens the type guarantees already enforced elsewhere (AcpPrefix, session-id value object types).As per coding guidelines,
rsworkspace/crates/**/*.rs: Prefer domain-specific value objects over primitives (e.g.AcpPrefixnotString).🤖 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 18 - 20, Change session_prompt to take the domain value objects instead of raw &str: replace the signature pub fn session_prompt(prefix: &str, session_id: &str) -> String with one that accepts the domain types (e.g. &AcpPrefix and &SessionId or other project-specific session-id value object). Inside session_prompt, use the value-object accessors or Display/AsRef<str> impls (e.g. prefix.as_ref() or format!("{}", prefix) and session_id.as_ref() or format!("{}", session_id)) to compose the subject string ("{}.{}.agent.session.prompt"). Update any call sites to pass the value objects instead of raw strings.
🤖 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/prompt.rs`:
- Around line 49-55: The code double-records failed requests when an invalid
session ID occurs because AcpSessionId::try_from(&args.session_id).map_err(...)
records a failed request via bridge.metrics.record_request/record_error inside
the map_err closure and the request is also recorded later (same path around the
second block that calls bridge.metrics.record_request for the prompt); remove
the duplicate recording by moving metric/error recording out of the map_err
closure and instead return the mapped error there, then ensure the single
bridge.metrics.record_request and bridge.metrics.record_error call path (the
existing later block that handles errors for the prompt) is the sole place that
records metrics for this error, or alternatively make the map_err closure only
construct/return the error without invoking bridge.metrics.* so the later error
handler records it once.
---
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/nats/subjects.rs`:
- Around line 18-20: Change session_prompt to take the domain value objects
instead of raw &str: replace the signature pub fn session_prompt(prefix: &str,
session_id: &str) -> String with one that accepts the domain types (e.g.
&AcpPrefix and &SessionId or other project-specific session-id value object).
Inside session_prompt, use the value-object accessors or Display/AsRef<str>
impls (e.g. prefix.as_ref() or format!("{}", prefix) and session_id.as_ref() or
format!("{}", session_id)) to compose the subject string
("{}.{}.agent.session.prompt"). Update any call sites to pass the value objects
instead of raw strings.
In `@rsworkspace/crates/AGENTS.md`:
- Line 3: The guidance currently assumes every value object exports both Type
and TypeError (pub use {module}::{Type, TypeError}), but some value objects are
infallible (newtypes) or use generic error types (e.g., anyhow::Error); update
AGENTS.md to document export variants: 1) for infallible/newtype value objects
export only Type (pub use {module}::Type), 2) for objects with a module-local
error type export both Type and TypeError as before, and 3) for objects that use
generic/shared error types document exporting Type and optionally re-exporting
the shared error alias (e.g., pub use {module}::Type and pub use
crate::errors::SharedError as TypeError) or note not to re-export concrete
anyhow::Error; reference the file layout rule and lib.rs export examples (Type,
TypeError) and add a short decision rule for which pattern to use.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
rsworkspace/crates/AGENTS.mdrsworkspace/crates/acp-nats/src/agent/cancel.rsrsworkspace/crates/acp-nats/src/agent/cancelled_sessions.rsrsworkspace/crates/acp-nats/src/agent/mod.rsrsworkspace/crates/acp-nats/src/agent/pending_prompt_waiters.rsrsworkspace/crates/acp-nats/src/agent/prompt.rsrsworkspace/crates/acp-nats/src/config.rsrsworkspace/crates/acp-nats/src/nats/subjects.rs
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
dbab690 to
3d26705
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
3d26705 to
ce67f76
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
ce67f76 to
b742439
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
198b8bd to
1b74ca5
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
1b74ca5 to
d3165d0
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
d3165d0 to
4624b7c
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
4624b7c to
9ab1f21
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
9ab1f21 to
e387fa8
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
e387fa8 to
29e63da
Compare
- Add prompt handler with session validation, cancel pre-flight, backpressure - Expand Bridge with CancelledSessions, PendingSessionPromptResponseWaiters - Update cancel to mark sessions cancelled and resolve pending prompt waiters - Add prompt_timeout and max_concurrent_client_tasks to config - Add session_prompt NATS subject Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
29e63da to
df2643c
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
df2643c to
93ae914
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.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
93ae914 to
50e3211
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
50e3211 to
ca5db16
Compare
Summary
Replaces the prompt stub with the real prompt handler from the add-acp migration. Prompt publishes session/prompt requests to NATS and awaits responses via the pending waiter mechanism. The bridge acts as a pass-through; session and cancel state are owned by the backend.
Changes
Tests
Not in this slice