Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
yordis
force-pushed
the
add-acp
branch
27 times, most recently
from
ad93c4c to
5f2ba1e
Compare
PR SummaryMedium Risk Overview Adds a new Updates Written by Cursor Bugbot for commit 51f0ccf. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
add-acp
branch
3 times, most recently
from
f3e3b9e to
d8838ae
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Replace Arc with Rc for non-Send types used exclusively on spawn_local, collapse nested if-let chains, remove ~900 lines of unused test builders and handlers, and apply cargo fmt.
SIGINT and SIGTERM are expected operational events for graceful shutdown, not conditions requiring attention.
Malformed payloads from external NATS sources are not internal server errors — they indicate protocol mismatches, not issues requiring on-call intervention.
If the configurable prefix contains "client" as a dot-separated token (e.g. "org.client.app"), position() matched the wrong token and the parser silently dropped valid messages. Using rposition() finds the structural marker instead.
Re-check the cancel flag after registering the prompt response waiter. Without this, a cancel arriving between the initial check and waiter registration would be missed, causing the prompt to proceed and wait until timeout.
| match serde_json::from_slice::<SessionNotification>(payload) { | ||
| Ok(notification) => { | ||
| if let Err(e) = client.session_notification(notification).await { | ||
| error!(error = %e, "Failed to send session notification"); |
There was a problem hiding this comment.
Session notification failure logged at error level
Low Severity
error! is used for "Failed to send session notification," but this can occur during normal client disconnects, which are non-actionable. Per logging rules, error logs are treated as production issues that must be fixed. A warn level is more appropriate here since the failure may or may not require action.
Triggered by team rule: Logging Level and Error Reporting Usage
- Extract duplicated session.ready publish logic into Bridge::spawn_session_ready - Fix double remove_waiter in prompt timeout path - Record metric on unexpected prompt channel close - Fix HandlerType copy-paste bug (SessionNew was Authenticate) - Reduce string allocations in metrics hot path - Extract telemetry file layer init into try_open_log_file - Document test builders as arbitrary fixtures - Add Rc safety comment explaining !Send constraint
- Cancel handler now resolves pending prompt waiter with StopReason::Cancelled - Replace raw -32603 literals with JSONRPC_INTERNAL_ERROR/JSONRPC_SERVER_ERROR constants - Prompt timeout and channel-closed now return JSON-RPC errors instead of fake EndTurn - Add catch_unwind guards on spawn_local tasks - Warn on duplicate waiter replacement - Validate empty session ID in parse_client_subject - Remove dead code (unused wildcards module, #[allow(dead_code)]) - Add Metrics::with_meter() constructor, simplify test setup - Add 3 new waiter tests (cancel, resolve, duplicate) - Remove unnecessary Rc wrapping on RefCell fields
| } | ||
|
|
||
| pub fn from_env_with_provider<E: ReadEnv>(env_provider: &E) -> Config { | ||
| let args = Args::parse(); |
There was a problem hiding this comment.
Args::parse() in tests reads real process arguments
Medium Severity
from_env_with_provider hardcodes Args::parse(), which reads from std::env::args(). In test binaries, those args include cargo-test flags (e.g., --nocapture, test name filters). Clap will reject unrecognized args and call process::exit(2), killing the entire test suite. The tests only pass when cargo test is invoked with zero extra arguments — running cargo test test_default_config or cargo test -- --nocapture will abort the process before any test runs.
Additional Locations (1)
31 new tests covering Agent trait handler flows (initialize, authenticate, new_session, load_session, set_session_mode, cancel, prompt, ext_method, ext_notification) and Client-side handlers (ext_session_prompt_response, session_update, all 8 RPC handlers with JSON round-trip).
1 participant
Signed-off-by: Yordis Prieto yordis.prieto@gmail.com