feat(update): implement update notification and confirmation handling#125
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive update notification and confirmation system. It adds an UpdateConfig configuration struct, integrates a daemon-based update watcher that polls for new versions, implements channel-based notifications with nonce confirmations, and adds opportunistic update notices within message processing workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Daemon as Update Daemon
participant Config as Config Store
participant Check as Check for Updates
participant Notify as Notification System
participant Channel as Channel Backend
participant State as Persistent State
Daemon->>Check: poll_and_notify_update()
Check->>Config: read current_version
Check->>Check: detect new version available
Check->>State: load pending_confirmations
Check->>Notify: collect_notification_targets()
Notify->>Config: read notify_destinations
Notify->>Notify: infer authorized senders
Notify->>Channel: build_channel(destination)
Notify->>Notify: generate nonce & hash
Channel->>Channel: send_update_notification()
Notify->>State: persist PendingConfirmation
rect rgb(200, 150, 255, 0.5)
Note over Daemon,State: User provides confirmation via channel
end
Channel->>Daemon: try_handle_channel_update_confirmation()
Daemon->>State: lookup PendingConfirmation by nonce
Daemon->>Daemon: validate nonce & TTL
Daemon->>Daemon: execute_minimal_update_strategy()
Daemon->>State: mark confirmation used
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 |
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-03 to 2026-03-03 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
clients/agent-runtime/src/daemon/mod.rs (1)
108-108: Consider rendering enabled components dynamically.Line 108 always prints
updatereven when updater startup is skipped, which can be confusing during ops/debugging. A dynamic list based on actual enabled components would make status output more trustworthy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/daemon/mod.rs` at line 108, The current println! in mod.rs always lists "updater" even when the updater startup is skipped; update the status printing to build the component list dynamically: create a components Vec (or similar) where you push "gateway", "channels", "heartbeat", and "scheduler" as before and only push "updater" when the same condition/flag used to decide updater startup (the variable or function that indicates updater is skipped) is false; then join the Vec into a string and pass that to the existing println! call so the printed Components line accurately reflects enabled components.clients/agent-runtime/src/config/schema.rs (1)
1936-1941: Validate non-zero update timing values at the schema boundary.
check_interval_minutesandconfirmation_ttl_minutescan deserialize as0. That can cause tight polling or instant nonce expiry depending on runtime consumers.🔧 Proposed schema hardening
pub struct UpdateConfig { @@ - #[serde(default = "default_update_check_interval_minutes")] + #[serde( + default = "default_update_check_interval_minutes", + deserialize_with = "deserialize_nonzero_u64" + )] pub check_interval_minutes: u64, @@ - #[serde(default = "default_update_confirmation_ttl_minutes")] + #[serde( + default = "default_update_confirmation_ttl_minutes", + deserialize_with = "deserialize_nonzero_u64" + )] pub confirmation_ttl_minutes: u64, @@ }fn deserialize_nonzero_u64<'de, D>(deserializer: D) -> Result<u64, D::Error> where D: serde::Deserializer<'de>, { let value = u64::deserialize(deserializer)?; if value == 0 { return Err(serde::de::Error::custom("value must be >= 1")); } Ok(value) }As per coding guidelines,
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs: Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/config/schema.rs` around lines 1936 - 1941, The schema currently allows check_interval_minutes and confirmation_ttl_minutes to deserialize as 0; add a custom deserializer (e.g., deserialize_nonzero_u64) that returns a serde::de::Error when value == 0 and apply it via #[serde(deserialize_with = "deserialize_nonzero_u64")] to the struct fields check_interval_minutes and confirmation_ttl_minutes so they cannot be zero after deserialization; ensure the error message is clear (e.g., "value must be >= 1") and keep the existing default_* functions for missing values.clients/agent-runtime/src/channels/mod.rs (1)
1991-2005: Add explicit tests for the new update pre-processing branches.These edits only inject
configinto test contexts. Please add direct assertions for the new behavior in Line 444+ and Line 454+ paths (confirmation short-circuit, auth/allowlist gating, and no downstream LLM execution when confirmation is handled).As per coding guidelines,
clients/agent-runtime/src/channels/**/*.rs: ImplementChanneltrait insrc/channels/with consistentsend,listen, andhealth_checksemantics and cover auth/allowlist/health behavior with tests.Also applies to: 2037-2051, 2135-2151, 2199-2215, 2576-2590, 2903-2919, 2950-2966
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/channels/mod.rs` around lines 1991 - 2005, Add explicit unit tests for the new update pre-processing branches in the channels tests: create tests that exercise the Channel trait implementations (send, listen, health_check) to cover the "confirmation short-circuit" branch and the "auth/allowlist gating" branch — for the confirmation short-circuit test assert that the Channel returns the confirmation result immediately and that no downstream LLM/tool execution is invoked; for the auth/allowlist gating test assert that the Channel rejects the request (proper error/deny response) and that no LLM/tool dispatch occurs. Use the existing test context setup (the struct initialized with config: Arc::new(Config::default()), tools_registry: Arc::new(vec![Box::new(MockPriceTool)]), provider: Arc::new(ToolCallingProvider), etc.) to inject any required config and mocks, and assert on interactions with MockPriceTool/provider to ensure no downstream execution when pre-processing short-circuits or denies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 444-452: Move the confirmation pre-check so
crate::update::try_handle_channel_update_confirmation(ctx.config.as_ref(), &msg,
target_channel.as_ref()).await is executed before any logging or autosave
operations that inspect or persist msg/target_channel; specifically, call
try_handle_channel_update_confirmation immediately after receiving/parsing the
message and return early on true, before the code paths that write logs or call
autosave functions, ensuring no logging/persistence of one-time tokens or
secrets from msg/target_channel occurs.
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Around line 90-100: The supervisor is restarted when run_daemon_update_watcher
returns Ok(()) for intentionally-disabled update checks; before calling
spawn_component_supervisor("updater", ...) add the same predicate used inside
update::is_update_check_disabled (make it pub(crate) or add an equivalent
predicate in clients::agent_runtime::update) and skip spawning the supervisor if
updates are disabled by policy. Specifically, export or add a public predicate
(is_update_check_disabled) in update::mod and, where you currently check
config.updates.enabled in mod.rs, also call
update::is_update_check_disabled(&config) and only spawn_component_supervisor
for the updater when both checks indicate active updates, leaving
run_daemon_update_watcher unchanged.
In `@clients/agent-runtime/src/update/mod.rs`:
- Around line 169-177: The matcher currently checks nonce_hash, channel, expiry
and sender but misses binding to the original recipient, allowing confirmations
from a different reply target; update the iterator/filter where
state.pending_confirmations.iter_mut().find(...) (the closure that references
pending, nonce_hash, msg.channel, msg.sender) to also require that
pending.recipient matches the incoming msg.recipient (use the same
case-insensitive comparison style as channel, e.g. eq_ignore_ascii_case or
equivalent) so confirmations are only accepted for the original
recipient/conversation.
- Around line 817-823: The tracing::warn! call currently logs recipient and the
raw error (variable error), which may expose sensitive data; change the warn to
omit recipient and raw stderr and instead log only sanitized metadata:
channel.name(), manager identifier if available, process exit code (from error)
and stderr length (e.g., stderr.as_ref().map(|s| s.len())). Update both
occurrences that use tracing::warn! (the one referencing result/error and the
other at the later occurrence) to construct the message like "update
notification failed: channel={} manager={} exit_code={:?} stderr_len={:?}" and
pass channel.name(), manager_id, exit_code, stderr_len; do not include recipient
or raw stderr content. Ensure you extract exit code and stderr length from the
error type before logging and leave return false behavior unchanged.
- Around line 323-324: The code currently ignores the result of
save_state(&state_path, &state).await which can orphan the nonce after sending;
change this to handle and propagate the error instead of discarding it: replace
the let _ = save_state(...) pattern with proper Result handling (use
`save_state(...).await?` or match and return an anyhow/thiserror error) so
failures are returned from the enclosing async function (or logged and trigger a
rollback) and ensure the enclosing function signature returns Result so callers
can react; key symbols: save_state, state_path, state, and the surrounding
send/nonce flow—ensure persistence failures abort or retry rather than being
ignored.
- Around line 133-208: This code performs an unprotected read-modify-write on
version_check.json (load_state -> mutate pending -> save_state) which can race
with other tasks; protect all RMW blocks by introducing a process-scoped async
lock (e.g., a tokio::sync::Mutex or RwLock) that is acquired before calling
version_check_path/load_state and held until after save_state completes; update
the handler here (the block using load_state, prune_pending_confirmations,
pending.used = true, save_state, and execute_minimal_update_strategy) to obtain
the lock, perform load->mutate->save while holding it, and ensure the same lock
is used in the other mentioned locations (the other handlers around the ranges
noted) so all accesses to load_state/save_state/modify of pending_confirmations
are serialized.
- Around line 842-843: The call
Command::new(bin).args(&filtered_args).output().await can hang; wrap the process
execution in a timeout (use tokio::time::timeout) and, instead of
.output().await directly, spawn the child (Command::new(...).spawn() -> Child)
and await child.wait_with_output() inside the timeout, so on Err(timeout) you
log/handle the timeout, kill the child process to avoid leaked processes, and
continue to the next candidate; keep use of bin, filtered_args, and the
surrounding logic intact and avoid extra clones or allocations when constructing
the command or handling results.
---
Nitpick comments:
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 1991-2005: Add explicit unit tests for the new update
pre-processing branches in the channels tests: create tests that exercise the
Channel trait implementations (send, listen, health_check) to cover the
"confirmation short-circuit" branch and the "auth/allowlist gating" branch — for
the confirmation short-circuit test assert that the Channel returns the
confirmation result immediately and that no downstream LLM/tool execution is
invoked; for the auth/allowlist gating test assert that the Channel rejects the
request (proper error/deny response) and that no LLM/tool dispatch occurs. Use
the existing test context setup (the struct initialized with config:
Arc::new(Config::default()), tools_registry:
Arc::new(vec![Box::new(MockPriceTool)]), provider:
Arc::new(ToolCallingProvider), etc.) to inject any required config and mocks,
and assert on interactions with MockPriceTool/provider to ensure no downstream
execution when pre-processing short-circuits or denies.
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 1936-1941: The schema currently allows check_interval_minutes and
confirmation_ttl_minutes to deserialize as 0; add a custom deserializer (e.g.,
deserialize_nonzero_u64) that returns a serde::de::Error when value == 0 and
apply it via #[serde(deserialize_with = "deserialize_nonzero_u64")] to the
struct fields check_interval_minutes and confirmation_ttl_minutes so they cannot
be zero after deserialization; ensure the error message is clear (e.g., "value
must be >= 1") and keep the existing default_* functions for missing values.
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Line 108: The current println! in mod.rs always lists "updater" even when the
updater startup is skipped; update the status printing to build the component
list dynamically: create a components Vec (or similar) where you push "gateway",
"channels", "heartbeat", and "scheduler" as before and only push "updater" when
the same condition/flag used to decide updater startup (the variable or function
that indicates updater is skipped) is false; then join the Vec into a string and
pass that to the existing println! call so the printed Components line
accurately reflects enabled components.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
clients/agent-runtime/src/agent/tests.rsclients/agent-runtime/src/channels/mod.rsclients/agent-runtime/src/config/mod.rsclients/agent-runtime/src/config/schema.rsclients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/onboard/wizard.rsclients/agent-runtime/src/update/mod.rs
Deploying corvus with
|
| Latest commit: |
209a1f4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d48fda47.corvus-42x.pages.dev |
| Branch Preview URL: | https://update-command.corvus-42x.pages.dev |
…nstructions and auto-labeling
|
This was referenced Mar 5, 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.
This pull request introduces support for update notifications and confirmation workflows in the agent runtime, along with associated configuration options. The changes include a new
UpdateConfigstruct in the configuration schema, wiring for update-related logic in the channel runtime, and the addition of an update watcher component in daemon mode. Tests and onboarding flows are updated to handle the new configuration.Update notification and confirmation support:
UpdateConfigstruct to the mainConfig, with options for enabling updates, check intervals, confirmation TTL, and per-channel notification destinations. Defaults are safe and enabled by default. (clients/agent-runtime/src/config/schema.rs,clients/agent-runtime/src/config/mod.rs) [1] [2] [3] [4]clients/agent-runtime/src/channels/mod.rs) [1] [2] [3]updatemodule to the agent runtime and started the update watcher as a supervised component in daemon mode if enabled in config. (clients/agent-runtime/src/lib.rs,clients/agent-runtime/src/daemon/mod.rs) [1] [2]Testing and configuration improvements:
updatesconfig field, ensuring correct initialization in all contexts. (clients/agent-runtime/src/channels/mod.rs,clients/agent-runtime/src/onboard/wizard.rs) [1] [2] [3] [4] [5] [6] [7] [8] [9]UpdateConfigto verify safety and correctness. (clients/agent-runtime/src/config/schema.rs)Other minor improvements:
clients/agent-runtime/src/agent/tests.rs)Summary by CodeRabbit
New Features
Tests