feat(service): add linger option for service install and restart commands#71
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds an update-checking module and integrates update notices into agent/daemon/status; extends service CLI with per-install Linux linger modes and a Restart command; implements Linux linger handling in service install/status and restart; improves daemon logging for gateway port conflicts; fixes HTTP response header display and README docs. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runtime as Agent/Daemon Runtime
participant Update as Update Module
participant Cache as Local Cache
participant GH as GitHub API
CLI->>Runtime: run command (agent / daemon / status)
Runtime->>Update: maybe_print_update_notice(config)
alt CORVUS_DISABLE_UPDATE_CHECK set
Update-->>Runtime: no-op
else
Update->>Cache: load_state()
Cache-->>Update: state (fresh or stale)
alt state fresh
Update-->>Runtime: notice_from_state / no notice
else
Update->>GH: fetch primary release endpoint
GH-->>Update: version or error
alt primary failed
Update->>GH: fetch fallback endpoint
GH-->>Update: version or error
end
Update->>Update: normalize & compare versions
Update->>Cache: save_state(updated)
Update-->>Runtime: print notice if update_available
end
end
Runtime-->>CLI: continue execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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-02-23 to 2026-02-23 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/agent-runtime/src/tools/http_request.rs (1)
305-313:⚠️ Potential issue | 🟡 MinorPre-existing: response header values are never rendered — key is printed twice.
The iterator binds
(k, _), silently discarding theHeaderValue. The non-sensitive branch then formats the key as both the name and the "value", producing output likecontent-type: "content-type"instead ofcontent-type: "application/json".🐛 Proposed fix
-let headers_text = response_headers - .map(|(k, _)| { - let is_sensitive = k.as_str().to_lowercase().contains("set-cookie"); - if is_sensitive { - format!("{}: ***REDACTED***", k.as_str()) - } else { - format!("{}: {:?}", k.as_str(), k.as_str()) - } - }) +let headers_text = response_headers + .map(|(k, v)| { + let is_sensitive = k.as_str().to_lowercase().contains("set-cookie"); + if is_sensitive { + format!("{}: ***REDACTED***", k.as_str()) + } else { + format!("{}: {}", k.as_str(), v.to_str().unwrap_or("***BINARY***")) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/tools/http_request.rs` around lines 305 - 313, The map closure currently drops the header value by binding `(k, _)`, causing the key to be printed twice; change the closure to bind `(k, v)` so you can render the actual header value, keep the existing is_sensitive check, and format the non-sensitive branch with the header value (e.g., use v.to_str().unwrap_or_else(|_| format!("{:?}", v)) or format!("{:?}", v)) instead of k.as_str(); update the closure where the iterator is mapped (the `(k, _)` binding and is_sensitive logic) so headers print as `"name: value"` with sensitive values redacted.
♻️ Duplicate comments (1)
clients/agent-runtime/src/main.rs (1)
92-117:ServiceLingerModeandServiceCommandsare duplicated fromlib.rs.Already noted in the
lib.rsreview — consider re-exporting from the library crate (likeHardwareCommandson line 79) to keep a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/main.rs` around lines 92 - 117, ServiceLingerMode and ServiceCommands are duplicated here; remove these local definitions and re-use the single source from the library crate by importing/re-exporting the types instead (mirror how HardwareCommands is referenced). Replace the local enum declarations with a use or pub use of the library's ServiceLingerMode and ServiceCommands so all code relies on the definitions in lib.rs, ensuring any arg/value_enum derives remain provided by the shared types.
🧹 Nitpick comments (6)
clients/agent-runtime/src/lib.rs (1)
74-79:ServiceLingerModeis duplicated betweenlib.rsandmain.rs.This enum (and the updated
ServiceCommands) is defined identically in both crate roots. While this is a necessary pattern when the same module (service/mod.rs) is compiled under both the library and binary crate, the definitions must be kept in sync manually. Consider having the binary re-export from the library (likeHardwareCommandsandPeripheralCommandsinmain.rsline 79) to eliminate the duplication and reduce the risk of divergence.♻️ Re-export from the library instead of duplicating
In
main.rs, replace the local definitions with re-exports:-#[derive(Debug, Clone, Copy, ValueEnum, Serialize, Deserialize, PartialEq, Eq)] -enum ServiceLingerMode { - Keep, - On, - Off, -} - -#[derive(Subcommand, Debug)] -enum ServiceCommands { - /// Install daemon service unit for auto-start and restart - Install { - /// Linux only: keep user service active without an interactive session - #[arg(long, value_enum, default_value_t = ServiceLingerMode::Keep)] - linger: ServiceLingerMode, - }, - /// Start daemon service - Start, - /// Restart daemon service - Restart, - /// Stop daemon service - Stop, - /// Check daemon service status - Status, - /// Uninstall daemon service unit - Uninstall, -} +pub use corvus::{ServiceCommands, ServiceLingerMode};This mirrors the existing pattern at line 79 for
HardwareCommandsandPeripheralCommands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/lib.rs` around lines 74 - 79, Remove the duplicated ServiceLingerMode (and the identically duplicated ServiceCommands) from main.rs and re-export the definitions from the library crate instead (the versions defined in lib.rs), following the existing pattern used for HardwareCommands and PeripheralCommands; update main.rs to use pub use (or equivalent) to reference the ServiceLingerMode and ServiceCommands from the library so there is a single authoritative definition in lib.rs and no duplicated enum/command definitions in main.rs.clients/agent-runtime/src/service/mod.rs (1)
169-173:current_username()relies onUSER/LOGNAMEenvironment variables.These env vars are typically available in interactive sessions but may be absent in non-interactive contexts (e.g., cron, some CI environments). Since
service installandservice statusare interactive CLI commands, this should be fine in practice. If you ever need to support non-interactive invocations, consider falling back towhoamicommand output or thenixcrate'sgetuid/getpwuid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 169 - 173, current_username() currently only reads USER/LOGNAME env vars which can be missing in non-interactive contexts; update it to fall back when those vars are absent by attempting to run the system "whoami" command (or use the nix crate's getuid/getpwuid to resolve the login name) and return that result, ensuring any command execution errors are converted into the same Result::Err with context; reference the current_username function to implement the fallback path and preserve the existing context message on final failure.clients/agent-runtime/src/main.rs (1)
608-613: Update check adds up to ~2s latency on cold cache to theagentstartup path.When the version check cache is stale or missing,
maybe_print_update_noticeperforms an HTTP request with a 2s timeout before the agent starts. This is tolerable given the 24h cache TTL (most runs will be instant), but it does add observable latency on first use or after cache expiry.If this becomes a user experience concern, consider spawning the update check concurrently (e.g.,
tokio::spawn) and printing the notice after the agent completes, or running it in the background entirely.Based on learnings: "Keep startup path lean and avoid heavy initialization in command parsing flow."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/main.rs` around lines 608 - 613, The startup path blocks on update::maybe_print_update_notice(&config).await causing up to ~2s latency before agent::run starts when the cache is stale; change it to run the update check concurrently so agent::run(config, ...) starts immediately—spawn the check via tokio::spawn (or similar) calling update::maybe_print_update_notice cloned with needed args and handle/log its result asynchronously (optionally printing the notice after agent::run completes), ensuring no await on maybe_print_update_notice in the main startup path.clients/agent-runtime/src/update/mod.rs (2)
136-161: Synchronous file I/O (std::fs) called from async context.
load_stateandsave_stateuse blockingstd::fs::read_to_string/std::fs::write/std::fs::create_dir_all. These are called transitively from the asynccheck_for_update. For a tiny JSON file on a startup-only code path, the practical impact is negligible, but it technically blocks the Tokio runtime thread.If you'd like to align with the async-first pattern used elsewhere in the codebase (e.g.,
tokio::fsindaemon/mod.rsline 120/134), consider switching totokio::fs— though this is low priority.As per coding guidelines, "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/update/mod.rs` around lines 136 - 161, The synchronous file I/O in load_state and save_state blocks the Tokio runtime; make them async and switch from std::fs to tokio::fs (e.g., tokio::fs::read_to_string, tokio::fs::create_dir_all, tokio::fs::write) so they return Result<Option<VersionCheckState>> and Result<()> as async fns, keeping the existing anyhow::Context usage for error messages; update the call site (check_for_update) to .await these new async functions and adjust signatures/uses accordingly (preserve VersionCheckState, path handling, and the same error messages).
219-236: Pre-release suffixes are silently stripped during comparison.
parse_semverishstrips everything after-or+, so1.0.0-beta.1is treated as equal to1.0.0. This means a pre-release tagged as GitHub's "latest" could produce a false "update available" notice (or miss a real update). This is likely acceptable for your use case since GitHub "latest" typically points to stable releases, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/update/mod.rs` around lines 219 - 236, parse_semverish currently strips any pre-release/build suffix (splitting on '-' or '+') so versions like 1.0.0-beta.1 are treated identical to 1.0.0; update parse_semverish to preserve the pre-release part instead of discarding it (for example return a tuple including an Option<String> pre-release field, e.g., (u64,u64,u64, Option<String>)), and then adjust the version-comparison logic that consumes parse_semverish to treat pre-release versions with lower precedence than the same core version without a pre-release (or explicitly reject/ignore pre-release versions depending on desired behavior) so comparisons correctly distinguish 1.0.0 from 1.0.0-beta.1.clients/agent-runtime/src/daemon/mod.rs (1)
162-169: Helpful diagnostic for port conflicts.The guidance to use
corvus service restartis useful for users encountering this after upgrades. Two minor notes:
e.to_string()is called on both line 163 and line 165. You could bind it once and reuse it to avoid the duplicate allocation in the error path.- The string match on
"Address already in use"is best-effort since OS error messages can vary by locale/platform, but this is acceptable for a diagnostic hint.♻️ Reuse the error string
Err(e) => { - crate::health::mark_component_error(name, e.to_string()); - tracing::error!("Daemon component '{name}' failed: {e}"); - if name == "gateway" && e.to_string().contains("Address already in use") { + let err_msg = e.to_string(); + crate::health::mark_component_error(name, &err_msg); + tracing::error!("Daemon component '{name}' failed: {err_msg}"); + if name == "gateway" && err_msg.contains("Address already in use") { tracing::warn!( "Gateway port is already in use. This usually means another daemon/gateway instance is already running. If this happened after an upgrade, run `corvus service restart` instead of starting a second daemon process." ); }As per coding guidelines, "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency".
🤖 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` around lines 162 - 169, The error path repeatedly calls e.to_string()—bind a single local (e.g., let err_str = e.to_string()) before using it, then pass err_str to crate::health::mark_component_error(name, err_str.clone() or &err_str as appropriate) and use err_str in tracing::error! and the contains check for "Address already in use" to avoid duplicate allocations; update the block around the Err(e) match arm (referencing name and e) to reuse that single string variable.
ℹ️ 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/README.mdclients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/gateway/mod.rsclients/agent-runtime/src/lib.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/service/mod.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/update/mod.rs
🤖 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/update/mod.rs`:
- Around line 59-92: The outer tokio::time::timeout wrapping
fetch_latest_release_version() in check_for_update causes the loop inside
fetch_latest_release_version (which uses a reqwest::Client per-request timeout)
to not reach fallback endpoints when the first endpoint is slow; remove the
outer tokio::time::timeout and call fetch_latest_release_version().await
directly (or alternatively increase the timeout to VERSION_CHECK_TIMEOUT_SECS *
2 + 1 if you prefer keeping it), ensuring existing error handling around the
returned Result remains unchanged; refer to check_for_update,
fetch_latest_release_version, VERSION_CHECK_TIMEOUT_SECS, tokio::time::timeout,
reqwest::Client, and RELEASE_ENDPOINTS to locate and update the code.
---
Outside diff comments:
In `@clients/agent-runtime/src/tools/http_request.rs`:
- Around line 305-313: The map closure currently drops the header value by
binding `(k, _)`, causing the key to be printed twice; change the closure to
bind `(k, v)` so you can render the actual header value, keep the existing
is_sensitive check, and format the non-sensitive branch with the header value
(e.g., use v.to_str().unwrap_or_else(|_| format!("{:?}", v)) or format!("{:?}",
v)) instead of k.as_str(); update the closure where the iterator is mapped (the
`(k, _)` binding and is_sensitive logic) so headers print as `"name: value"`
with sensitive values redacted.
---
Duplicate comments:
In `@clients/agent-runtime/src/main.rs`:
- Around line 92-117: ServiceLingerMode and ServiceCommands are duplicated here;
remove these local definitions and re-use the single source from the library
crate by importing/re-exporting the types instead (mirror how HardwareCommands
is referenced). Replace the local enum declarations with a use or pub use of the
library's ServiceLingerMode and ServiceCommands so all code relies on the
definitions in lib.rs, ensuring any arg/value_enum derives remain provided by
the shared types.
---
Nitpick comments:
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Around line 162-169: The error path repeatedly calls e.to_string()—bind a
single local (e.g., let err_str = e.to_string()) before using it, then pass
err_str to crate::health::mark_component_error(name, err_str.clone() or &err_str
as appropriate) and use err_str in tracing::error! and the contains check for
"Address already in use" to avoid duplicate allocations; update the block around
the Err(e) match arm (referencing name and e) to reuse that single string
variable.
In `@clients/agent-runtime/src/lib.rs`:
- Around line 74-79: Remove the duplicated ServiceLingerMode (and the
identically duplicated ServiceCommands) from main.rs and re-export the
definitions from the library crate instead (the versions defined in lib.rs),
following the existing pattern used for HardwareCommands and PeripheralCommands;
update main.rs to use pub use (or equivalent) to reference the ServiceLingerMode
and ServiceCommands from the library so there is a single authoritative
definition in lib.rs and no duplicated enum/command definitions in main.rs.
In `@clients/agent-runtime/src/main.rs`:
- Around line 608-613: The startup path blocks on
update::maybe_print_update_notice(&config).await causing up to ~2s latency
before agent::run starts when the cache is stale; change it to run the update
check concurrently so agent::run(config, ...) starts immediately—spawn the check
via tokio::spawn (or similar) calling update::maybe_print_update_notice cloned
with needed args and handle/log its result asynchronously (optionally printing
the notice after agent::run completes), ensuring no await on
maybe_print_update_notice in the main startup path.
In `@clients/agent-runtime/src/service/mod.rs`:
- Around line 169-173: current_username() currently only reads USER/LOGNAME env
vars which can be missing in non-interactive contexts; update it to fall back
when those vars are absent by attempting to run the system "whoami" command (or
use the nix crate's getuid/getpwuid to resolve the login name) and return that
result, ensuring any command execution errors are converted into the same
Result::Err with context; reference the current_username function to implement
the fallback path and preserve the existing context message on final failure.
In `@clients/agent-runtime/src/update/mod.rs`:
- Around line 136-161: The synchronous file I/O in load_state and save_state
blocks the Tokio runtime; make them async and switch from std::fs to tokio::fs
(e.g., tokio::fs::read_to_string, tokio::fs::create_dir_all, tokio::fs::write)
so they return Result<Option<VersionCheckState>> and Result<()> as async fns,
keeping the existing anyhow::Context usage for error messages; update the call
site (check_for_update) to .await these new async functions and adjust
signatures/uses accordingly (preserve VersionCheckState, path handling, and the
same error messages).
- Around line 219-236: parse_semverish currently strips any pre-release/build
suffix (splitting on '-' or '+') so versions like 1.0.0-beta.1 are treated
identical to 1.0.0; update parse_semverish to preserve the pre-release part
instead of discarding it (for example return a tuple including an Option<String>
pre-release field, e.g., (u64,u64,u64, Option<String>)), and then adjust the
version-comparison logic that consumes parse_semverish to treat pre-release
versions with lower precedence than the same core version without a pre-release
(or explicitly reject/ignore pre-release versions depending on desired behavior)
so comparisons correctly distinguish 1.0.0 from 1.0.0-beta.1.
Deploying corvus-plugins with
|
| Latest commit: |
1fedaff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://786740db.corvus-plugins.pages.dev |
| Branch Preview URL: | https://feature-update-linger-comman.corvus-plugins.pages.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
clients/agent-runtime/src/daemon/mod.rs (1)
163-164: Unnecessaryclone()— pass&err_strinstead.
mark_component_erroracceptsimpl ToString, which&Stringsatisfies. Cloning allocates a secondStringthat is immediately dropped after the call.♻️ Proposed fix
- let err_str = e.to_string(); - crate::health::mark_component_error(name, err_str.clone()); - tracing::error!("Daemon component '{name}' failed: {err_str}"); + let err_str = e.to_string(); + crate::health::mark_component_error(name, &err_str); + tracing::error!("Daemon component '{name}' failed: {err_str}");As per coding guidelines: "Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency."
🤖 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` around lines 163 - 164, The code creates err_str and unnecessarily clones it when calling crate::health::mark_component_error(name, err_str.clone()); instead pass a reference to avoid allocating a second String: call mark_component_error with &err_str (or borrow err_str directly) since mark_component_error accepts impl ToString; update the call site in mod.rs where err_str is defined and used to remove .clone() and pass the borrowed &err_str.clients/agent-runtime/src/service/mod.rs (1)
169-189: No unit tests forcurrent_username,apply_linux_linger_mode, orlinux_linger_state.
current_usernameis fully testable with manipulated environment variables; the others require a Linux + loginctl environment but could at least have compile-time coverage on CI.Example for
current_username:#[test] fn current_username_reads_user_env() { // Safety: only safe in single-threaded tests; use serial_test crate if needed std::env::set_var("USER", "testuser"); assert_eq!(current_username().unwrap(), "testuser"); std::env::remove_var("USER"); } #[test] fn current_username_falls_back_to_logname() { std::env::remove_var("USER"); std::env::set_var("LOGNAME", "loguser"); assert_eq!(current_username().unwrap(), "loguser"); std::env::remove_var("LOGNAME"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 169 - 189, Add unit tests for current_username and compile-time/CI coverage for apply_linux_linger_mode and linux_linger_state: write tests that set and remove environment variables using std::env::set_var/std::env::remove_var and assert current_username() returns the expected string (e.g., set USER and assert "testuser", remove USER set LOGNAME and assert "loguser"), ensure tests run single-threaded or use the serial_test crate to avoid env races, and add conditional #[cfg(target_os = "linux")] tests for apply_linux_linger_mode and linux_linger_state (and #[cfg(not(target_os = "linux"))] stub tests) so CI compiles those symbols even if loginctl is unavailable. Ensure tests clean up/restore environment state after running and place them under the same module so they reference current_username, apply_linux_linger_mode, and linux_linger_state directly.
ℹ️ 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 (5)
clients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/service/mod.rsclients/agent-runtime/src/tools/http_request.rsclients/agent-runtime/src/update/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/agent-runtime/src/update/mod.rs
- clients/agent-runtime/src/tools/http_request.rs
🤖 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/main.rs`:
- Around line 583-591: The detached tokio::spawn for
update::maybe_print_update_notice causes the notice to be dropped in
single-message (--message) mode and to interleave with agent I/O in interactive
mode; instead, call and await update::maybe_print_update_notice(&update_config)
before entering agent::run (or the interactive loop) so the notice completes
synchronously, and if you want to bound startup latency use
tokio::time::timeout(Duration::from_millis(500),
update::maybe_print_update_notice(&update_config)).await and handle the timeout
result gracefully; update the main() branch where update_config is cloned (and
ensure similar behavior for Commands::Daemon and Commands::Status) to keep
output coordinated.
- Line 605: The call to update::maybe_print_update_notice(&config).await blocks
daemon startup; change it so the update check is non-blocking for the daemon
path by either spawning it as a background task or skipping it for daemon runs:
locate the call (update::maybe_print_update_notice) invoked before daemon::run
and replace the await with a fire-and-forget tokio::spawn (or equivalent
runtime::spawn) of an async closure that calls
update::maybe_print_update_notice(config.clone()) so the HTTP round-trip does
not stall startup, or add a config/flag check (e.g., detecting the Daemon
command) to not call it at all for daemon runs.
In `@clients/agent-runtime/src/service/mod.rs`:
- Around line 25-38: The code mutates system linger state before installation
and can leave it changed if install_linux(config) fails; change the call order
so install_linux(config) is invoked first and only after it returns Ok
apply_linux_linger_mode(linger)? is called, or alternatively ensure
apply_linux_linger_mode(linger)? is reverted on error by wrapping it in a
transactional pattern; update the install function (referencing install,
install_linux, and apply_linux_linger_mode and the linger parameter) to perform
the linger change only after a successful install or to roll back the linger
change if install_linux fails.
- Around line 208-227: The linux_linger_state() helper mis-parses legacy systemd
output and treats non-zero loginctl exits as success; update
linux_linger_state() to (1) call loginctl in a way that checks the child process
exit status (or change run_capture to return Err when status is non-zero) so
failures propagate instead of returning Ok(""), and (2) normalize the output by
handling both bare "yes"/"no" and "Key=Value" forms (e.g., if output contains
'=', split on '=' and use the right-hand side, then trim and compare
case-insensitively to "yes"). Ensure these changes are reflected where
linux_linger_state() is used (e.g., status()) so unavailable/error cases are
propagated instead of being reported as false.
---
Nitpick comments:
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Around line 163-164: The code creates err_str and unnecessarily clones it when
calling crate::health::mark_component_error(name, err_str.clone()); instead pass
a reference to avoid allocating a second String: call mark_component_error with
&err_str (or borrow err_str directly) since mark_component_error accepts impl
ToString; update the call site in mod.rs where err_str is defined and used to
remove .clone() and pass the borrowed &err_str.
In `@clients/agent-runtime/src/service/mod.rs`:
- Around line 169-189: Add unit tests for current_username and compile-time/CI
coverage for apply_linux_linger_mode and linux_linger_state: write tests that
set and remove environment variables using
std::env::set_var/std::env::remove_var and assert current_username() returns the
expected string (e.g., set USER and assert "testuser", remove USER set LOGNAME
and assert "loguser"), ensure tests run single-threaded or use the serial_test
crate to avoid env races, and add conditional #[cfg(target_os = "linux")] tests
for apply_linux_linger_mode and linux_linger_state (and #[cfg(not(target_os =
"linux"))] stub tests) so CI compiles those symbols even if loginctl is
unavailable. Ensure tests clean up/restore environment state after running and
place them under the same module so they reference current_username,
apply_linux_linger_mode, and linux_linger_state directly.
Deploying corvus with
|
| Latest commit: |
1fedaff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c208d7a8.corvus-42x.pages.dev |
| Branch Preview URL: | https://feature-update-linger-comman.corvus-42x.pages.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
clients/agent-runtime/src/daemon/mod.rs (2)
166-170: Missing test for the gateway port-conflict warning path.The new
if name == "gateway" && …branch has no test coverage. At a minimum, a test that spawns the supervisor withname = "gateway"and an error carrying the port-conflict string should assert that the health state and restart count are set correctly.🧪 Example test skeleton
#[tokio::test] async fn supervisor_sets_error_on_gateway_addr_in_use() { let handle = spawn_component_supervisor("gateway", 1, 1, || async { Err(anyhow::anyhow!(std::io::Error::from( std::io::ErrorKind::AddrInUse ))) }); tokio::time::sleep(Duration::from_millis(50)).await; handle.abort(); let _ = handle.await; let snapshot = crate::health::snapshot_json(); let component = &snapshot["components"]["gateway"]; assert_eq!(component["status"], "error"); assert!(component["restart_count"].as_u64().unwrap_or(0) >= 1); }🤖 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` around lines 166 - 170, Add a new async test that exercises the gateway port-conflict branch by using the existing spawn_component_supervisor helper to spawn a supervisor with name "gateway" and a task that returns an error containing "Address already in use" (e.g., an AddrInUse io::Error); after a short sleep abort the handle, call crate::health::snapshot_json(), and assert that snapshot["components"]["gateway"]["status"] == "error" and that snapshot["components"]["gateway"]["restart_count"] is >= 1; this ensures the if name == "gateway" && err_str.contains("Address already in use") branch in daemon::mod.rs is covered.
166-170: Missing test coverage for the new gateway port-conflict branch.The new
if name == "gateway" && err_str.contains("Address already in use")path has no corresponding test. At a minimum, a test that spawns the supervisor withname = "gateway"and anErrwhose message contains"Address already in use"would confirm the health state and restart count behave correctly (and, once tracing is captured, that the warning fires).🧪 Example skeleton
#[tokio::test] async fn supervisor_logs_hint_on_gateway_addr_in_use() { let handle = spawn_component_supervisor("gateway", 1, 1, || async { Err(anyhow::anyhow!("Address already in use")) }); tokio::time::sleep(Duration::from_millis(50)).await; handle.abort(); let _ = handle.await; let snapshot = crate::health::snapshot_json(); let component = &snapshot["components"]["gateway"]; assert_eq!(component["status"], "error"); assert!(component["last_error"] .as_str() .unwrap_or("") .contains("Address already in use")); }🤖 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` around lines 166 - 170, Add a tokio::test that exercises the new gateway branch by calling spawn_component_supervisor("gateway", ...) with a closure that returns Err(anyhow::anyhow!("Address already in use")), sleep briefly, abort the handle, then call crate::health::snapshot_json() and assert the "gateway" component has status "error" and that its "last_error" contains "Address already in use"; also assert the restart count/health fields update as expected and (optionally) capture tracing to assert the specific warning message was emitted. Ensure the test name is descriptive (e.g., supervisor_logs_hint_on_gateway_addr_in_use) and uses the same helper functions (spawn_component_supervisor, crate::health::snapshot_json) referenced in the diff.clients/agent-runtime/src/service/mod.rs (1)
191-206:current_username()called unnecessarily forServiceLingerMode::Keep.
current_username()runs aUSER/LOGNAMEenv lookup (and potentially spawnswhoami) even whenlinger == Keep, where the result is immediately discarded. Move the call into theOn/Offarms.♻️ Proposed refactor
fn apply_linux_linger_mode(linger: crate::ServiceLingerMode) -> Result<()> { - let user = current_username()?; match linger { crate::ServiceLingerMode::Keep => Ok(()), crate::ServiceLingerMode::On => { + let user = current_username()?; run_checked(Command::new("loginctl").args(["enable-linger", &user]))?; println!("✅ Enabled linger for user '{user}'"); Ok(()) } crate::ServiceLingerMode::Off => { + let user = current_username()?; run_checked(Command::new("loginctl").args(["disable-linger", &user]))?; println!("✅ Disabled linger for user '{user}'"); Ok(()) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 191 - 206, The call to current_username() is done unconditionally in apply_linux_linger_mode even when ServiceLingerMode::Keep is returned immediately; move the current_username() invocation into the On and Off match arms so it's only executed when needed (i.e., call current_username() at the start of the On arm and reuse its Result or ? to propagate errors, and likewise in the Off arm), leaving the Keep branch to return Ok(()) without calling current_username().
🤖 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/daemon/mod.rs`:
- Around line 166-170: The warning currently matches the OS-specific string
"Address already in use" and is emitted every retry; change it to detect
AddrInUse by walking the anyhow error chain and downcasting to std::io::Error to
check io::ErrorKind::AddrInUse (do this check before calling e.to_string() or
otherwise ensure the original error is available), and add a deduplication guard
(e.g., a local bool or a static AtomicBool like GATEWAY_PORT_IN_USE_WARNED) so
the warning inside the branch handling name == "gateway" is only logged once per
process.
- Around line 166-170: Replace the fragile OS-specific substring check with an
error-kind match against std::io::ErrorKind::AddrInUse when handling gateway
startup failures (i.e., where the code currently checks name == "gateway" &&
err_str.contains("Address already in use")), and add a boolean flag inside the
supervisor closure (e.g., gateway_port_conflict_logged) that is set the first
time you log the warning so subsequent retry/backoff cycles do not re-emit the
same message; ensure the branch still targets the "gateway" name and includes
the original descriptive message when the error kind matches AddrInUse.
---
Nitpick comments:
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Around line 166-170: Add a new async test that exercises the gateway
port-conflict branch by using the existing spawn_component_supervisor helper to
spawn a supervisor with name "gateway" and a task that returns an error
containing "Address already in use" (e.g., an AddrInUse io::Error); after a
short sleep abort the handle, call crate::health::snapshot_json(), and assert
that snapshot["components"]["gateway"]["status"] == "error" and that
snapshot["components"]["gateway"]["restart_count"] is >= 1; this ensures the if
name == "gateway" && err_str.contains("Address already in use") branch in
daemon::mod.rs is covered.
- Around line 166-170: Add a tokio::test that exercises the new gateway branch
by calling spawn_component_supervisor("gateway", ...) with a closure that
returns Err(anyhow::anyhow!("Address already in use")), sleep briefly, abort the
handle, then call crate::health::snapshot_json() and assert the "gateway"
component has status "error" and that its "last_error" contains "Address already
in use"; also assert the restart count/health fields update as expected and
(optionally) capture tracing to assert the specific warning message was emitted.
Ensure the test name is descriptive (e.g.,
supervisor_logs_hint_on_gateway_addr_in_use) and uses the same helper functions
(spawn_component_supervisor, crate::health::snapshot_json) referenced in the
diff.
In `@clients/agent-runtime/src/service/mod.rs`:
- Around line 191-206: The call to current_username() is done unconditionally in
apply_linux_linger_mode even when ServiceLingerMode::Keep is returned
immediately; move the current_username() invocation into the On and Off match
arms so it's only executed when needed (i.e., call current_username() at the
start of the On arm and reuse its Result or ? to propagate errors, and likewise
in the Off arm), leaving the Keep branch to return Ok(()) without calling
current_username().
ℹ️ 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 (3)
clients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/main.rsclients/agent-runtime/src/service/mod.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
clients/agent-runtime/src/service/mod.rs (3)
509-557: Environment save/restore in tests is not panic-safe.If an assertion (or any code between
set_envand the restore block) panics, the original environment values are never restored. Subsequent tests holdingENV_LOCKwill see corrupted state. A small RAII guard would fix this:💡 Sketch of an EnvGuard helper
struct EnvGuard { key: &'static str, original: Option<String>, } impl EnvGuard { fn set(key: &'static str, value: &str) -> Self { let original = std::env::var(key).ok(); unsafe { std::env::set_var(key, value) }; Self { key, original } } fn remove(key: &'static str) -> Self { let original = std::env::var(key).ok(); unsafe { std::env::remove_var(key) }; Self { key, original } } } impl Drop for EnvGuard { fn drop(&mut self) { match &self.original { Some(v) => unsafe { std::env::set_var(self.key, v) }, None => unsafe { std::env::remove_var(self.key) }, } } }This would simplify each test to:
let _guard = ENV_LOCK.lock().unwrap(); let _u = EnvGuard::set("USER", "testuser"); let _l = EnvGuard::set("LOGNAME", "loguser"); assert_eq!(current_username().unwrap(), "testuser"); // restore happens automatically on drop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 509 - 557, The tests manipulate environment variables with set_env/remove_env around assertions but are not panic-safe, so if a test panics the original env is not restored and ENV_LOCK-protected state is corrupted; create an RAII helper (e.g., EnvGuard) and replace the manual save/restore in the tests (current_username_prefers_user_env and current_username_uses_logname_when_user_missing) with EnvGuard::set / EnvGuard::remove instances so the Drop impl always restores the original value; keep using ENV_LOCK for synchronization and reference current_username, set_env, and remove_env usages when locating where to swap in the guard.
161-167:maybe_warn_unsupported_linger_modeprints to stdout — considereprintln!for warnings.Warnings are conventionally written to stderr so they don't interfere with machine-parseable stdout output. Using
eprintln!(ortracing::warn!) would align with the convention used elsewhere in the daemon.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 161 - 167, The warning in maybe_warn_unsupported_linger_mode currently prints to stdout with println!; change it to emit the warning to stderr (use eprintln!) or use the project's logging/tracing facility (e.g., tracing::warn!) so warnings don't pollute stdout; update the function to call eprintln! (or tracing::warn!) with the same message and ensure any tests or callers expecting stdout are adjusted accordingly.
40-43: Simple stop-then-start restart — works but differs from native restart semantics on Linux.
systemctl --user restart corvus.servicewould be atomic and avoid the brief window between stop and start. The current approach is fine for a CLI tool (and is more portable across the OS branches), but worth noting if you want to tighten the restart window on Linux.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 40 - 43, The restart function currently does stop(config)? then start(config) which leaves a small downtime window; change restart(config: &Config) to, on Linux when systemd is available, invoke an atomic systemctl --user restart corvus.service (e.g. spawn a Command calling "systemctl" with args ["--user","restart","corvus.service"] and propagate errors) and fall back to the existing stop(config)?; start(config) behavior on other platforms or if systemctl fails; update restart to reference the restart function itself and use Config only for the fallback path as needed.clients/agent-runtime/src/daemon/mod.rs (1)
304-325: Test uses the real component name"gateway", which shares state with the global health registry.The health registry is a process-wide singleton. Using
"gateway"here means this test writes to the same slot that a real daemon (or future test) would use. Today no other test collides, but parallel test runs or a new test targeting"gateway"will produce flaky failures. Consider a unique name like"gateway-addr-in-use-test"and adjusting the assertions accordingly — thename == "gateway"branch in the supervisor is the only part that actually requires the literal"gateway"string, but the health-state assertions don't.Actually, the supervisor itself checks
name == "gateway"to decide whether to inspect forAddrInUse, so using"gateway"is intentional and necessary to exercise that code path. Given that constraint, the current approach is acceptable but worth noting.🤖 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` around lines 304 - 325, The test supervisor_logs_hint_on_gateway_addr_in_use currently writes to the global health registry under the real component name "gateway", risking cross-test collisions; keep calling spawn_component_supervisor("gateway", ...) to exercise the name == "gateway" AddrInUse branch but avoid global-state leakage by isolating or cleaning the health registry after the test — e.g., call a teardown helper to remove or reset the component entry (use crate::health::clear_component("gateway") or crate::health::reset()/crate::health::remove(...) if available) or alternatively change the health assertions to target a unique component key (e.g., "gateway-addr-in-use-test") if you modify the supervisor registration, and ensure snapshot checks (crate::health::snapshot_json and the component variable) reference that unique key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Around line 304-325: The test supervisor_logs_hint_on_gateway_addr_in_use
currently writes to the global health registry under the real component name
"gateway", risking cross-test collisions; keep calling
spawn_component_supervisor("gateway", ...) to exercise the name == "gateway"
AddrInUse branch but avoid global-state leakage by isolating or cleaning the
health registry after the test — e.g., call a teardown helper to remove or reset
the component entry (use crate::health::clear_component("gateway") or
crate::health::reset()/crate::health::remove(...) if available) or alternatively
change the health assertions to target a unique component key (e.g.,
"gateway-addr-in-use-test") if you modify the supervisor registration, and
ensure snapshot checks (crate::health::snapshot_json and the component variable)
reference that unique key.
In `@clients/agent-runtime/src/service/mod.rs`:
- Around line 509-557: The tests manipulate environment variables with
set_env/remove_env around assertions but are not panic-safe, so if a test panics
the original env is not restored and ENV_LOCK-protected state is corrupted;
create an RAII helper (e.g., EnvGuard) and replace the manual save/restore in
the tests (current_username_prefers_user_env and
current_username_uses_logname_when_user_missing) with EnvGuard::set /
EnvGuard::remove instances so the Drop impl always restores the original value;
keep using ENV_LOCK for synchronization and reference current_username, set_env,
and remove_env usages when locating where to swap in the guard.
- Around line 161-167: The warning in maybe_warn_unsupported_linger_mode
currently prints to stdout with println!; change it to emit the warning to
stderr (use eprintln!) or use the project's logging/tracing facility (e.g.,
tracing::warn!) so warnings don't pollute stdout; update the function to call
eprintln! (or tracing::warn!) with the same message and ensure any tests or
callers expecting stdout are adjusted accordingly.
- Around line 40-43: The restart function currently does stop(config)? then
start(config) which leaves a small downtime window; change restart(config:
&Config) to, on Linux when systemd is available, invoke an atomic systemctl
--user restart corvus.service (e.g. spawn a Command calling "systemctl" with
args ["--user","restart","corvus.service"] and propagate errors) and fall back
to the existing stop(config)?; start(config) behavior on other platforms or if
systemctl fails; update restart to reference the restart function itself and use
Config only for the fallback path as needed.
ℹ️ 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 (2)
clients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/service/mod.rs
…ed resource management
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
clients/agent-runtime/src/daemon/mod.rs (1)
321-343: Test name doesn't match what is actually asserted.
supervisor_logs_hint_on_gateway_addr_in_useasserts health state (error status, restart count, last_error string) but does not verify thetracing::warn!emission. The name implies the warning is the subject under test. Renaming to something likesupervisor_sets_error_state_on_gateway_addr_in_usewould better reflect the actual assertions and avoid confusion when the test is read in isolation.🤖 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` around lines 321 - 343, The test named supervisor_logs_hint_on_gateway_addr_in_use currently asserts health state (component status, restart_count, last_error) but does not check for any tracing::warn! emission; rename the test function to a name that reflects its actual assertions (for example supervisor_sets_error_state_on_gateway_addr_in_use) and update any references to the old test name; ensure the new name appears on the async test function declaration (supervisor_logs_hint_on_gateway_addr_in_use -> supervisor_sets_error_state_on_gateway_addr_in_use) and keep the body (HealthComponentGuard, spawn_component_supervisor call, and the snapshot assertions) unchanged.clients/agent-runtime/src/service/mod.rs (2)
461-502:EnvGuard::droprelies on an external lock invariant the type doesn't enforce; lock poisoning can cascade test failures.Two related concerns with the test helpers:
EnvGuarddoesn't holdENV_LOCKitself. ItsDropimpl callsset_env/remove_envwhich carry the SAFETY requirement thatENV_LOCKis held by the caller. Any future test that constructs anEnvGuardwithout first locking will silently violate this contract. Consider makingEnvGuardhold theMutexGuardinternally, or at minimum makeEnvGuard::set/removeaccept&MutexGuard<'_, ()>as proof the lock is held.
ENV_LOCK.lock().unwrap()poisons on panic. If a test panics while holding the lock, every subsequent test calling.unwrap()also panics, obscuring the original failure. Use.unwrap_or_else(|e| e.into_inner())to recover from a poisoned lock.♻️ Suggested fix for poisoning resilience
- let _guard = ENV_LOCK.lock().unwrap(); + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());(apply to both
current_username_prefers_user_envandcurrent_username_uses_logname_when_user_missing)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 461 - 502, EnvGuard currently relies on the external invariant that ENV_LOCK is held, which is unsafe and brittle; change EnvGuard to capture the lock by storing the MutexGuard from ENV_LOCK (or alternately make EnvGuard::set and EnvGuard::remove accept a &MutexGuard<'_, ()> parameter) so Drop can call set_env/remove_env safely, and update call sites to pass the guard or construct EnvGuard after locking. Also make all ENV_LOCK.lock() usages resilient to poisoning by replacing .unwrap() with .unwrap_or_else(|e| e.into_inner()) so a panicked test doesn't permanently poison the lock (apply this to tests like current_username_prefers_user_env and current_username_uses_logname_when_user_missing).
40-52: Silent fallback path on Linux could benefit from a diagnostic log.When
systemctl --user restartfails, the code silently falls back tostop + startwithout any indication that the primary path failed. While the user eventually sees "✅ Service stopped / started", the reason for the two-step path is invisible.♻️ Suggested improvement
fn restart(config: &Config) -> Result<()> { if cfg!(target_os = "linux") { if run_checked(Command::new("systemctl").args(["--user", "restart", "corvus.service"])) .is_ok() { println!("✅ Service restarted"); return Ok(()); + } else { + tracing::debug!("systemctl restart failed; falling back to stop + start"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/service/mod.rs` around lines 40 - 52, The restart function currently calls run_checked(Command::new("systemctl").args(["--user", "restart", "corvus.service"])) and silently falls back to stop() + start() on failure; change it to capture the Result from run_checked, and when it is Err include a diagnostic log (e.g., log::warn! or eprintln!) that prints a clear message plus the error/output from the failed systemctl invocation (referencing restart, run_checked, and the Command::new("systemctl").args(...) call), then proceed with the existing stop(config)?; start(config) fallback so the fallback remains but is no longer silent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clients/agent-runtime/src/daemon/mod.rs`:
- Around line 321-343: The test named
supervisor_logs_hint_on_gateway_addr_in_use currently asserts health state
(component status, restart_count, last_error) but does not check for any
tracing::warn! emission; rename the test function to a name that reflects its
actual assertions (for example
supervisor_sets_error_state_on_gateway_addr_in_use) and update any references to
the old test name; ensure the new name appears on the async test function
declaration (supervisor_logs_hint_on_gateway_addr_in_use ->
supervisor_sets_error_state_on_gateway_addr_in_use) and keep the body
(HealthComponentGuard, spawn_component_supervisor call, and the snapshot
assertions) unchanged.
In `@clients/agent-runtime/src/service/mod.rs`:
- Around line 461-502: EnvGuard currently relies on the external invariant that
ENV_LOCK is held, which is unsafe and brittle; change EnvGuard to capture the
lock by storing the MutexGuard from ENV_LOCK (or alternately make EnvGuard::set
and EnvGuard::remove accept a &MutexGuard<'_, ()> parameter) so Drop can call
set_env/remove_env safely, and update call sites to pass the guard or construct
EnvGuard after locking. Also make all ENV_LOCK.lock() usages resilient to
poisoning by replacing .unwrap() with .unwrap_or_else(|e| e.into_inner()) so a
panicked test doesn't permanently poison the lock (apply this to tests like
current_username_prefers_user_env and
current_username_uses_logname_when_user_missing).
- Around line 40-52: The restart function currently calls
run_checked(Command::new("systemctl").args(["--user", "restart",
"corvus.service"])) and silently falls back to stop() + start() on failure;
change it to capture the Result from run_checked, and when it is Err include a
diagnostic log (e.g., log::warn! or eprintln!) that prints a clear message plus
the error/output from the failed systemctl invocation (referencing restart,
run_checked, and the Command::new("systemctl").args(...) call), then proceed
with the existing stop(config)?; start(config) fallback so the fallback remains
but is no longer silent.
ℹ️ 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 (3)
clients/agent-runtime/src/daemon/mod.rsclients/agent-runtime/src/health/mod.rsclients/agent-runtime/src/service/mod.rs
Merged
This was referenced Apr 9, 2026
Merged
Merged
Merged
This was referenced Apr 19, 2026
This was referenced Apr 29, 2026
8 tasks
8 tasks
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 several improvements and new features to the Corvus agent runtime, with a focus on service management, update notifications, and enhanced documentation. The most significant changes are the addition of a service restart command, support for Linux linger modes, and automatic update checks. The changes also improve error handling and clarify documentation for users.
Service management enhancements:
Restartcommand to theServiceCommandsenum, allowing users to restart the background service without manually stopping and starting it. [1] [2] [3]--lingeroption for theservice installcommand, enabling Linux users to keep the service running after logout or reboot. This includes support for enabling/disabling linger and status reporting, with clear warnings for unsupported OSes. [1] [2] [3]Update notification improvements:
agent,daemon, andstatuscommands, with an option to disable viaCORVUS_DISABLE_UPDATE_CHECK. [1] [2] [3] [4] [5]Documentation and usability:
README.mdto reflect new commands (restart,--linger), clarify service lifecycle management, and explain update notifications. [1] [2]These changes collectively make Corvus easier to manage, more robust, and clearer for end users.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation