Energy & performance checks for doctor #102
Conversation
📝 WalkthroughWalkthroughAdds three dependencies and introduces new energy and performance doctor checks (battery charge, battery health, and power-mode management), registers them, and adds a POWERPROFILESDAEMON dependency entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Doctor as PerformanceTest
participant Factory as PowerHandleFactory
participant Handle as PowerHandle (Gnome or Legacy)
participant System as External (powerprofilesctl / sysfs)
Doctor->>Factory: build_power_handle()
Factory-->>Doctor: Box<dyn PowerHandle>
Doctor->>Handle: query_performance_mode()
Handle->>System: query backend (powerprofilesctl or sysfs)
System-->>Handle: current mode
Handle-->>Doctor: PowerMode
alt user invokes fix
Doctor->>Handle: change_performance_mode(Performance)
Handle->>System: set performance mode (tool or sysfs write)
System-->>Handle: result
Handle-->>Doctor: success/failure
end
sequenceDiagram
participant Doctor as PowerCheck / BatteryHealthCheck
participant Manager as battery::Manager
participant Batteries as System Batteries
Doctor->>Manager: Manager::new() & get_batteries()
Manager->>Batteries: enumerate
Batteries-->>Manager: battery list
Manager-->>Doctor: Vec<Battery>
Doctor->>Doctor: check_parameter(batteries) -> Pass/Warning/Fail/Skipped
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/doctor/checks/performance.rs:
- Line 138: The variable name legacy_idenitfier is misspelled; rename it to
legacy_identifier and update all usages accordingly (e.g., where it's referenced
later around the match result and at the usage on Line 172) so the binding and
subsequent references match; ensure you update the let binding
(legacy_idenitfier -> legacy_identifier) and any occurrences in the surrounding
function/checks in performance.rs.
- Around line 131-180: PerformanceTest's fix_privilege_level is missing so the
check wrongly reports the fixer can run as any user; override the
DoctorCheck::fix_privilege_level method on the PerformanceTest implementation to
return PrivilegeLevel::Root (i.e., add a fn fix_privilege_level(&self) ->
PrivilegeLevel { PrivilegeLevel::Root } in the PerformanceTest impl) so the
framework knows the fix requires root to write
/sys/devices/system/cpu/*/cpufreq/scaling_governor.
- Around line 72-89: The error returned from change_performance_mode is
mode-agnostic; update the Err path to include which mode failed by using the
chosen gnome_identifier or the PowerMode variant in the message (e.g., "Failed
to set {gnome_identifier} mode" or including mode via format!("{:?}", mode)) so
users see the specific mode that failed; locate change_performance_mode and
replace the hardcoded "Failed to set power-saver mode" with a formatted error
string referencing gnome_identifier or mode.
🧹 Nitpick comments (4)
src/doctor/checks/energy.rs (2)
15-18: Preferfilter_mapoverfilter+map+unwrap.The current pattern filters for
Okresults and then unwraps, which is safe but not idiomatic Rust.♻️ Proposed refactor
- let batteries = maybe_batteries - .filter(|maybe_battery| maybe_battery.is_ok()) - .map(|battery| battery.unwrap()) - .collect::<Vec<_>>(); + let batteries = maybe_batteries + .filter_map(|maybe_battery| maybe_battery.ok()) + .collect::<Vec<_>>();
32-38: Consider makingformat_batterya standalone function.Since
format_batteryhas identical behavior for all implementations ofBatteryCheck, it could be a free function or an associated function (non-trait method) rather than a trait method. This would make the trait simpler and the code more maintainable.src/doctor/checks/performance.rs (2)
201-224: Use thewhichcrate instead of shell command.Lines 203-208 spawn a shell command to check for
powerprofilesctl. The project already depends on thewhichcrate (version 8.0.0 in Cargo.toml), which provides a more reliable and efficient way to check for executables without spawning a subprocess.♻️ Proposed refactor
async fn build_power_handle(&self) -> Option<Box<dyn PowerHandle>> { // We check if powerprofilesctl is available - let profiled_power = TokioCommand::new("which") - .arg("powerprofilesctl") - .output() - .await - .map(|output| output.status.success()) - .unwrap_or(false); + let profiled_power = which::which("powerprofilesctl").is_ok(); + if profiled_power { return Some(Box::new(GnomePowerHandle::default())); }Note: Since
which::whichis synchronous, you may want to wrap it intokio::task::spawn_blockingif this method must remain async, though in this case the sync call should be fast enough.
276-302: Improve test assertions and handling.The current tests only print output without making assertions, and they use
unwrap()which will cause panics on systems without power management support. Consider either:
- Adding
#[ignore]attribute so these tests only run manually on supported systems- Converting to proper tests with assertions and graceful handling of unavailable features
♻️ Example improved test
#[tokio::test] + #[ignore] // Only run on systems with power management async fn test_factory() { let factory = PowerHandleFactory::default(); - let power_handle = factory.build_power_handle().await.unwrap(); - println!( - "Current mode: {:?}", - power_handle.query_performance_mode().await.unwrap() - ); - println!("Power modes:"); - for power_mode in power_handle.available_modes().await { - println!("{:?}", power_mode) - } + let power_handle = factory.build_power_handle().await; + assert!(power_handle.is_some(), "Power handle should be available on test system"); + + let handle = power_handle.unwrap(); + let current_mode = handle.query_performance_mode().await; + assert!(current_mode.is_some(), "Should be able to query current mode"); + + let modes = handle.available_modes().await; + assert!(!modes.is_empty(), "Should have at least one available mode"); }Based on coding guidelines: "Respect single-thread test configuration and avoid spawning unnecessary threads in tests."
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/doctor/checks.rssrc/doctor/checks/energy.rssrc/doctor/checks/performance.rssrc/doctor/registry.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Avoid test flakiness by using preparedINS_BINhelpers intests/helpers.sh
Rust style: use edition 2024, preferanyhow::Resultor typed errors viathiserror; use?operator overunwrap/expect
Group imports as: std/prelude, third-party, crate; avoid unusedpubexposures; keepuse crate::...paths concise
Use descriptive snake_case for functions and variables, CamelCase for types, SCREAMING_SNAKE_CASE for constants and environment keys
Bubble errors with context usinganyhow::Context; log user-facing messages and avoid silent failures
Respect single-thread test configuration and avoid spawning unnecessary threads in testsAlways compile the code after making changes to verify correctness with the compiler. Use
cargo checkfor quick syntax checks orcargo buildfor full compilation.Always compile the code after making changes to verify correctness with the compiler using
cargo checkfor quick syntax checks orcargo buildfor full compilation
Files:
src/doctor/registry.rssrc/doctor/checks/energy.rssrc/doctor/checks/performance.rssrc/doctor/checks.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Support multi-line confirmation dialogs with custom yes/no text for critical operations like game removal
Files:
src/doctor/registry.rssrc/doctor/checks/energy.rssrc/doctor/checks/performance.rssrc/doctor/checks.rs
🧬 Code graph analysis (2)
src/doctor/checks/energy.rs (1)
src/doctor/registry.rs (1)
new(12-38)
src/doctor/checks/performance.rs (1)
src/doctor/registry.rs (1)
new(12-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and build on Arch Linux
🔇 Additional comments (8)
src/doctor/registry.rs (1)
2-2: LGTM!The new check registrations are properly integrated with the existing registry pattern, and the import aligns with the new
energymodule structure.Also applies to: 32-34
src/doctor/checks.rs (1)
17-17: LGTM!The module structure appropriately exposes the new check types while keeping the
performancemodule's internals private. This design choice limits the public API surface while still allowing external access toPerformanceTest.Also applies to: 21-21, 30-30, 34-34
src/doctor/checks/energy.rs (2)
60-94: LGTM!The battery power level check logic is sound. The thresholds (critical below 25%, warning below 50%) are reasonable for user notifications. The
unwrap()on Line 69 is safe because theexecute()method ensures at least one battery is present before callingcheck_parameter().
116-134: LGTM!The battery health check implementation follows the same safe pattern as
PowerCheck. The 90% health threshold provides an early warning for battery degradation, which is appropriate for system health monitoring.src/doctor/checks/performance.rs (3)
22-34: LGTM!The
PowerHandletrait provides a clean abstraction for different power management backends. The async design is appropriate for I/O operations.
36-47: LGTM!The
PowerModeenum appropriately handles both "power-saver" (used by powerprofilesctl) and "powersave" (used by legacy sysfs governors) through multiple serialization attributes, ensuring compatibility across different power management systems.
227-274: LGTM with privilege consideration.The
PerformanceTestimplementation correctly uses warnings rather than failures for non-performance modes, which is appropriate since this is a recommendation rather than a critical issue. Thefix()method properly propagates errors usinganyhow::Result.Note: Ensure users are aware that fixing this check may require elevated privileges when using the legacy sysfs backend (as flagged in a separate comment).
Cargo.toml (1)
63-65: All specified versions are valid and current. Verification confirms:
battery0.7.8 is the latest available versionstrum0.27.2 is the latest available versionstrum_macros0.27.2 is the latest available version- No security advisories found for any of these versions in the RustSec advisory database
The dependencies are in good standing and suitable for use.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/doctor/checks/performance.rs:
- Around line 273-285: The test currently panics by calling unwrap() on
PowerHandleFactory::default().build_power_handle(), which will fail in CI
without power management; change the test to handle the Err case gracefully by
attempting let res = PowerHandleFactory::default().build_power_handle().await
and if it returns Err(e) log or eprintln!("power management unavailable: {}", e)
and return (skip) the test instead of unwrap; if Ok(power_handle) continue to
call power_handle.query_performance_mode().await and
power_handle.available_modes().await as before so the test only asserts behavior
when a power handle is actually available.
- Around line 130-133: The legacy mapping in src/doctor/checks/performance.rs
incorrectly maps PowerMode::Balanced to "performance" and also has a typo in the
variable name legacy_idenitfier; update the match in the code that builds
legacy_idenitfier (or rename to legacy_identifier) so that
PowerMode::Performance maps to "performance", PowerMode::Balanced maps to the
appropriate legacy governor (e.g., "ondemand" or "conservative" per project
convention), and PowerMode::PowerSaver maps to "powersave"; ensure the variable
name is corrected to legacy_identifier and update any usages accordingly.
- Around line 66-83: The error message in change_performance_mode always says
"Failed to set power-saver mode"; update change_performance_mode (and its use of
gnome_identifier/PP_CTL/TokioCommand) to return a contextual error that includes
the actual mode being set (use gnome_identifier or PowerMode::{:?}) and include
command output or stderr for diagnostics; replace the hardcoded
Err(anyhow!("Failed to set power-saver mode")) with an anyhow::Error that
formats the attempted mode and the command's output/status.
🧹 Nitpick comments (2)
src/doctor/checks/performance.rs (2)
140-171: Simplify loop pattern withwhile let.The manual loop with explicit
matchonnext_entry()can be simplified using idiomaticwhile letsyntax, improving readability.♻️ Proposed refactor
- loop { - let entry_opt = match cpu_dirs.next_entry().await { - Ok(entry) => entry, - Err(_) => { - return Err(anyhow!( - "Failed to read directory entry from /sys/devices/system/cpu" - )); - } - }; - - if let Some(entry) = entry_opt { + while let Some(entry) = cpu_dirs + .next_entry() + .await + .context("Failed to read directory entry from /sys/devices/system/cpu")? + { let name = entry.file_name(); let name_str = name.to_string_lossy(); if !name_str.starts_with("cpu") { continue; } if let Ok(core) = name_str[3..].parse::<u32>() { let governor_path = format!( "/sys/devices/system/cpu/cpu{}/cpufreq/scaling_governor", core ); tokio::fs::write(&governor_path, legacy_identifier) .await .with_context(|| format!("writing to {}", governor_path))?; } - } else { - return Ok(()); - } } + Ok(())Note: This also uses
.context()per the anyhow guidelines instead of manual error construction.
193-203: Consider more robust command availability check.Using
whichto check forpowerprofilesctlassumeswhichitself is available, which may not be true on minimal installations. A more robust approach is to attempt running the command directly and handle the error.♻️ Alternative approach
- // We check if powerprofilesctl is available - let profiled_power = TokioCommand::new("which") - .arg("powerprofilesctl") + // Try to use powerprofilesctl if available + let profiled_power = TokioCommand::new(PP_CTL) + .arg("--version") .output() .await .map(|output| output.status.success()) .unwrap_or(false); if profiled_power { return Ok(Box::new(GnomePowerHandle::default())); }This directly checks if
powerprofilesctlcan be executed, which is more reliable across different environments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/doctor/checks/performance.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Avoid test flakiness by using preparedINS_BINhelpers intests/helpers.sh
Rust style: use edition 2024, preferanyhow::Resultor typed errors viathiserror; use?operator overunwrap/expect
Group imports as: std/prelude, third-party, crate; avoid unusedpubexposures; keepuse crate::...paths concise
Use descriptive snake_case for functions and variables, CamelCase for types, SCREAMING_SNAKE_CASE for constants and environment keys
Bubble errors with context usinganyhow::Context; log user-facing messages and avoid silent failures
Respect single-thread test configuration and avoid spawning unnecessary threads in testsAlways compile the code after making changes to verify correctness with the compiler. Use
cargo checkfor quick syntax checks orcargo buildfor full compilation.Always compile the code after making changes to verify correctness with the compiler using
cargo checkfor quick syntax checks orcargo buildfor full compilation
Files:
src/doctor/checks/performance.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Support multi-line confirmation dialogs with custom yes/no text for critical operations like game removal
Files:
src/doctor/checks/performance.rs
🧬 Code graph analysis (1)
src/doctor/checks/performance.rs (1)
src/doctor/registry.rs (1)
new(12-38)
🔇 Additional comments (4)
src/doctor/checks/performance.rs (4)
1-8: LGTM: Imports follow Rust guidelines.The imports are well-organized and align with the coding guidelines (edition 2024, anyhow::Result, async patterns with tokio).
22-34: LGTM: Well-designed trait for power management abstraction.The async trait with Send + Sync bounds is appropriate for concurrent usage. The
available_modesmethod signature (returningVecinstead ofResult) means implementations will return an empty vector on error, which is a reasonable design choice for optional feature detection.
36-47: LGTM: Clean enum design with multiple serialization aliases.The
PowerModeenum properly usesstrummacros for string conversion, and the dual serialization forPowerSaver("power-saver" and "powersave") provides good compatibility across different backends.
219-267: LGTM: Clean implementation of the DoctorCheck trait.The
PerformanceTestimplementation correctly:
- Separates error handling with
try_execute()and returnsSkippedon unavailability- Provides clear messaging and fixability status
- Requires
Rootprivilege for fixes, which is appropriate for system power mode changes- Uses the factory pattern to select the appropriate backend
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/doctor/checks/performance.rs:
- Around line 117-123: query_performance_mode currently uses
PowerMode::from_str(content.trim())? which turns unrecognized governors (e.g.,
"schedutil") into an error and causes the test to be skipped; change the
function to handle unknown governors gracefully by matching the string (or
matching the Result from PowerMode::from_str) and returning a non-error
PowerMode variant for non-performance governors (e.g., PowerMode::Other or
PowerMode::NotPerformance) instead of propagating an error; apply the same fix
to the other occurrence (the similar logic at lines ~175-186) so unknown/legacy
governor values yield a concrete PowerMode and only I/O errors are returned as
Err.
- Around line 193-212: The factory currently trusts
deps::POWERPROFILESDAEMON.is_installed() and returns a GnomePowerHandle without
validating runtime usability; update PowerHandleFactory::build_power_handle to
probe the chosen handle (call a lightweight runtime check on GnomePowerHandle,
e.g., invoking the equivalent of `powerprofilesctl get` or a provided
is_usable()/probe method) and if that probe fails, fall back to creating and
returning LegacyPowerHandle (or try sysfs probe) instead of immediately erroring
or skipping; ensure you reference PowerHandleFactory::build_power_handle,
deps::POWERPROFILESDAEMON, GnomePowerHandle, and LegacyPowerHandle when making
the change.
- Around line 265-282: The test test_factory is environment-dependent and should
not run in CI: add the #[ignore] attribute to the async test function, remove
the println! side-effects, and replace the fragile unwrap() calls on
PowerHandleFactory::build_power_handle(), power_handle.query_performance_mode(),
and power_handle.available_modes() with graceful error handling that returns
early (or skips) on Err to avoid panics in CI while keeping the test intact for
local/manual runs.
🧹 Nitpick comments (1)
src/doctor/checks/performance.rs (1)
243-249: Don’t drop the underlying error context when skipping.
Right now any failure becomesSkipped("Could not query performance mode")(Line 247), which makes field debugging hard.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.ymlsrc/common/deps.rssrc/doctor/checks/performance.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Avoid test flakiness by using preparedINS_BINhelpers intests/helpers.sh
Rust style: use edition 2024, preferanyhow::Resultor typed errors viathiserror; use?operator overunwrap/expect
Group imports as: std/prelude, third-party, crate; avoid unusedpubexposures; keepuse crate::...paths concise
Use descriptive snake_case for functions and variables, CamelCase for types, SCREAMING_SNAKE_CASE for constants and environment keys
Bubble errors with context usinganyhow::Context; log user-facing messages and avoid silent failures
Respect single-thread test configuration and avoid spawning unnecessary threads in testsAlways compile the code after making changes to verify correctness with the compiler. Use
cargo checkfor quick syntax checks orcargo buildfor full compilation.Always compile the code after making changes to verify correctness with the compiler using
cargo checkfor quick syntax checks orcargo buildfor full compilation
Files:
src/common/deps.rssrc/doctor/checks/performance.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Support multi-line confirmation dialogs with custom yes/no text for critical operations like game removal
Files:
src/common/deps.rssrc/doctor/checks/performance.rs
🧬 Code graph analysis (1)
src/doctor/checks/performance.rs (1)
src/doctor/registry.rs (1)
new(12-38)
🔇 Additional comments (1)
src/common/deps.rs (1)
112-120: Dependency definition is correct.power-profiles-daemonon both Apt and Pacman provides thepowerprofilesctlbinary, so the install test guard is appropriate and will work reliably across both distros.
| async fn query_performance_mode(&self) -> anyhow::Result<PowerMode> { | ||
| const GOVERNOR_PATH: &str = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor"; | ||
|
|
||
| let content = tokio::fs::read_to_string(GOVERNOR_PATH).await?; | ||
| Ok(PowerMode::from_str(content.trim())?) | ||
| } | ||
|
|
There was a problem hiding this comment.
Legacy path likely skips on common governors (e.g., schedutil) instead of warning.
PowerMode::from_str(content.trim())? (Line 121) will error for non-performance/powersave, causing PerformanceTest to become Skipped rather than “not performance”.
Suggested direction
async fn query_performance_mode(&self) -> anyhow::Result<PowerMode> {
@@
- let content = tokio::fs::read_to_string(GOVERNOR_PATH).await?;
- Ok(PowerMode::from_str(content.trim())?)
+ let governor = tokio::fs::read_to_string(GOVERNOR_PATH).await?;
+ let g = governor.trim();
+ Ok(match g {
+ "performance" => PowerMode::Performance,
+ "powersave" => PowerMode::PowerSaver,
+ // Treat everything else as “not performance” so the doctor check can warn.
+ _ => PowerMode::Balanced,
+ })
}Also applies to: 175-186
🤖 Prompt for AI Agents
In @src/doctor/checks/performance.rs around lines 117 - 123,
query_performance_mode currently uses PowerMode::from_str(content.trim())? which
turns unrecognized governors (e.g., "schedutil") into an error and causes the
test to be skipped; change the function to handle unknown governors gracefully
by matching the string (or matching the Result from PowerMode::from_str) and
returning a non-error PowerMode variant for non-performance governors (e.g.,
PowerMode::Other or PowerMode::NotPerformance) instead of propagating an error;
apply the same fix to the other occurrence (the similar logic at lines ~175-186)
so unknown/legacy governor values yield a concrete PowerMode and only I/O errors
are returned as Err.
|
I tested this feature on Ubuntu. I do not know why powerprofilessctl returns non-zero value but it is probably because the CI is running on Arch. Maybe Arch uses some other kind of power management... |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/doctor/checks/performance.rs:
- Around line 67-84: Replace the manual mapping in change_performance_mode by
using PowerMode's Display implementation (e.g., let gnome_identifier =
mode.to_string() or format!("{}", mode)) and use that identifier when calling
TokioCommand; also fix the error branch to include the actual mode in the
message (e.g., Err(anyhow!("Failed to set power mode to {}", gnome_identifier)))
so the command and error reflect the requested PowerMode (symbols:
change_performance_mode, PowerMode, gnome_identifier, PP_CTL, TokioCommand).
- Around line 131-134: There's a typo in the local variable name: rename
legacy_idenitfier to legacy_identifier wherever it's declared and used (the
match creating "performance"/"powersave" for PowerMode in the code block that
defines legacy_idenitfier) so the identifier spelling is correct and consistent
across the file.
- Around line 194-212: In build_power_handle, avoid blocking the async runtime:
run the synchronous which check inside tokio::task::spawn_blocking by moving
deps::POWERPROFILESDAEMON.is_installed() into a spawn_blocking call and await
its boolean result (preserving the early-return behavior that returns
GnomePowerHandle when true); and replace the inefficient
TokioFile::open(...).await.is_ok() existence check with
tokio::fs::try_exists("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors").await
to decide whether to return LegacyPowerHandle or the error.
- Around line 86-109: The available_modes function currently checks for lines
that end with ':' which only matches the header and yields no profiles; update
the parsing to detect profile lines that begin with a marker ('*' or '-') after
trimming leading whitespace, e.g. for each line do let line = line.trim(); if
line.starts_with('*') || line.starts_with('-') { let name =
line.trim_start_matches(&['*','-'][..]).trim(); name.parse::<PowerMode>().ok() }
else { None }, so the code extracts "balanced", "performance", etc., and returns
parsed PowerMode values; keep the surrounding TokioCommand/PP_CTL call and error
handling unchanged.
🧹 Nitpick comments (1)
src/doctor/checks/performance.rs (1)
136-173: Simplify the directory iteration loop.The manual loop pattern with
next_entry().await(lines 141-172) can be simplified using awhile letpattern for better readability.♻️ Proposed simplification
let mut cpu_dirs = match tokio::fs::read_dir("/sys/devices/system/cpu").await { Ok(dir) => dir, Err(_) => return Err(anyhow!("Failed to read dir /sys/devices/system/cpu")), }; - loop { - let entry_opt = match cpu_dirs.next_entry().await { - Ok(entry) => entry, - Err(_) => { - return Err(anyhow!( - "Failed to read directory entry from /sys/devices/system/cpu" - )); - } - }; - - if let Some(entry) = entry_opt { + while let Some(entry) = cpu_dirs + .next_entry() + .await + .context("Failed to read directory entry from /sys/devices/system/cpu")? + { let name = entry.file_name(); let name_str = name.to_string_lossy(); if !name_str.starts_with("cpu") { continue; } if let Ok(core) = name_str[3..].parse::<u32>() { let governor_path = format!( "/sys/devices/system/cpu/cpu{}/cpufreq/scaling_governor", core ); tokio::fs::write(&governor_path, legacy_identifier) .await .with_context(|| format!("writing to {}", governor_path))?; } - } else { - return Ok(()); - } } + Ok(())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/doctor/checks/performance.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Avoid test flakiness by using preparedINS_BINhelpers intests/helpers.sh
Rust style: use edition 2024, preferanyhow::Resultor typed errors viathiserror; use?operator overunwrap/expect
Group imports as: std/prelude, third-party, crate; avoid unusedpubexposures; keepuse crate::...paths concise
Use descriptive snake_case for functions and variables, CamelCase for types, SCREAMING_SNAKE_CASE for constants and environment keys
Bubble errors with context usinganyhow::Context; log user-facing messages and avoid silent failures
Respect single-thread test configuration and avoid spawning unnecessary threads in testsAlways compile the code after making changes to verify correctness with the compiler. Use
cargo checkfor quick syntax checks orcargo buildfor full compilation.Always compile the code after making changes to verify correctness with the compiler using
cargo checkfor quick syntax checks orcargo buildfor full compilation
Files:
src/doctor/checks/performance.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Support multi-line confirmation dialogs with custom yes/no text for critical operations like game removal
Files:
src/doctor/checks/performance.rs
🧠 Learnings (1)
📚 Learning: 2025-12-06T13:52:52.247Z
Learnt from: CR
Repo: instantOS/instantCLI PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T13:52:52.247Z
Learning: Tests default to 1 thread (configured in `.cargo/config.toml`); run `cargo test` for unit/integration tests
Applied to files:
src/doctor/checks/performance.rs
🧬 Code graph analysis (1)
src/doctor/checks/performance.rs (1)
src/doctor/registry.rs (1)
new(12-38)
🔇 Additional comments (6)
src/doctor/checks/performance.rs (6)
1-21: LGTM!The imports and module documentation are clear and appropriate for the async power management functionality.
23-48: LGTM!The trait and enum design provide a clean abstraction for power mode management with proper async support and string conversions.
55-65: LGTM!Proper async command execution with appropriate error handling.
115-122: LGTM!Clean sysfs-based query implementation with proper error handling.
175-187: LGTM!Clean implementation for querying available governors from sysfs.
215-263: Consider adding tests for the new functionality.The
PerformanceTestimplementation is well-structured with proper error handling and privilege level requirements. However, no tests are included in this file.Consider adding unit tests for:
- PowerMode enum string parsing
- Mock implementations of PowerHandle for testing PerformanceTest logic
- Factory selection logic
As per coding guidelines and learnings, tests can be run with
cargo test(defaults to 1 thread per.cargo/config.toml).Based on learnings, tests default to 1 thread.
| async fn available_modes(&self) -> Vec<PowerMode> { | ||
| let output = TokioCommand::new(PP_CTL).arg("list").output().await; | ||
|
|
||
| match output { | ||
| Ok(output) if output.status.success() => { | ||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| stdout | ||
| .lines() | ||
| .filter_map(|line| { | ||
| let line = line.trim(); | ||
| if line.ends_with(':') { | ||
| let profile_name = | ||
| line.trim_end_matches(':').trim_start_matches('*').trim(); | ||
| profile_name.parse().ok() | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect() | ||
| } | ||
| _ => vec![], | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if powerprofilesctl is available and get its output
if command -v powerprofilesctl &>/dev/null; then
echo "=== powerprofilesctl list output ==="
powerprofilesctl list
echo ""
echo "=== powerprofilesctl version ==="
powerprofilesctl --version || echo "No version flag available"
else
echo "powerprofilesctl not found on this system"
fiRepository: instantOS/instantCLI
Length of output: 105
🏁 Script executed:
# Find the PowerMode enum definition and related code
rg "enum PowerMode" -A 20 src/Repository: instantOS/instantCLI
Length of output: 1344
🏁 Script executed:
# Search for any tests related to available_modes
rg "available_modes" tests/ --type rs -B 2 -A 10Repository: instantOS/instantCLI
Length of output: 90
🏁 Script executed:
# Look for any comments or documentation about powerprofilesctl format
rg "powerprofilesctl\|PP_CTL" src/ -B 2 -A 5Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Find the complete PowerMode enum definition with derive macros
rg "enum PowerMode" -B 5 -A 15 src/doctor/checks/performance.rsRepository: instantOS/instantCLI
Length of output: 539
🏁 Script executed:
# Search for test files related to performance checks
fd "test" -e rs src/doctor/checks/ -o fd "*performance*" tests/Repository: instantOS/instantCLI
Length of output: 187
🏁 Script executed:
# Look for any mocked outputs or test data
rg "performance\|powerprofilesctl" tests/ -A 5Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Check if there's any documentation in comments about the expected format
rg "list.*output\|output.*format" src/doctor/checks/performance.rs -B 3 -A 3Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Search online documentation or comments about powerprofilesctl format in the repo
rg "^\s*\*\s|//.*powerprofilesctl\|/\*.*powerprofilesctl" src/ -B 1 -A 3Repository: instantOS/instantCLI
Length of output: 251
🌐 Web query:
powerprofilesctl list output format documentation
💡 Result:
The output format is documented in the powerprofilesctl manual pages (manpage). The list command prints the available profile names, with the currently active profile marked (typically with an asterisk *). See the manpage for details and examples: [1], [2]. Example output form:
Available power profiles:
- balanced
- performance
- power-saver
References:
- powerprofilesctl manpage (Arch/Upstream). [1]
- powerprofilesctl manpage (Ubuntu). [2]
🏁 Script executed:
# Search for any comments or context around this parsing code
rg "lines\(\)" -B 10 -A 10 src/doctor/checks/performance.rsRepository: instantOS/instantCLI
Length of output: 857
🏁 Script executed:
# Look for integration tests or examples showing expected output
git log --oneline -n 20 -- src/doctor/checks/performance.rsRepository: instantOS/instantCLI
Length of output: 119
🏁 Script executed:
# Check if there are any GitHub issues or discussions mentioning the format
find . -name "*.md" -type f | xargs grep -l "powerprofilesctl\|performance" 2>/dev/null | head -5Repository: instantOS/instantCLI
Length of output: 86
🏁 Script executed:
# Search for any tests or mocked outputs that show the expected format
rg "available_modes\|powerprofilesctl" -B 5 -A 10 tests/Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Check the git history to see if this was changed recently
git log -p --follow -S "ends_with" -- src/doctor/checks/performance.rs | head -100Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Look for any test fixtures or example outputs in the repo
find . -name "*.txt" -o -name "*.out" | xargs grep -l "balanced\|performance" 2>/dev/null | head -5Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Let me verify the exact line ending pattern by checking if there are any test cases or comments
rg "balanced|performance|power-saver" src/doctor/checks/performance.rs -B 2 -A 2Repository: instantOS/instantCLI
Length of output: 3479
🏁 Script executed:
# Check the actual structure of the code more carefully
cat -n src/doctor/checks/performance.rs | sed -n '86,109p'Repository: instantOS/instantCLI
Length of output: 1096
The parsing logic is broken and will return an empty list with actual powerprofilesctl list output.
The actual powerprofilesctl list output format is:
Available power profiles:
* balanced
- performance
- power-saver
The profile names (lines 92-104) don't end with :, but the code filters for line.ends_with(':'). This means all actual profile lines are skipped. Only the header line "Available power profiles:" ends with :, and it won't parse to a valid PowerMode, so the function returns an empty vector regardless of available modes.
Fix the parsing to match the actual format: check for lines starting with * or -, strip those prefixes, and parse the profile name.
🤖 Prompt for AI Agents
In @src/doctor/checks/performance.rs around lines 86 - 109, The available_modes
function currently checks for lines that end with ':' which only matches the
header and yields no profiles; update the parsing to detect profile lines that
begin with a marker ('*' or '-') after trimming leading whitespace, e.g. for
each line do let line = line.trim(); if line.starts_with('*') ||
line.starts_with('-') { let name =
line.trim_start_matches(&['*','-'][..]).trim(); name.parse::<PowerMode>().ok() }
else { None }, so the code extracts "balanced", "performance", etc., and returns
parsed PowerMode values; keep the surrounding TokioCommand/PP_CTL call and error
handling unchanged.
There was a problem hiding this comment.
Weird, on my machine output of powerprofilesctl list looks like this:
performance:
CpuDriver: intel_pstate
Degraded: yes (high-operating-temperature)
balanced:
CpuDriver: intel_pstate
PlatformDriver: placeholder
* power-saver:
CpuDriver: intel_pstate
PlatformDriver: placeholder
Parsing logic works for this output.
Please confirm on Arch @paperbenni !
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| let legacy_idenitfier = match mode { | ||
| PowerMode::Performance | PowerMode::Balanced => "performance", | ||
| PowerMode::PowerSaver => "powersave", | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find the relevant file
find . -type f -name "performance.rs" | head -20Repository: instantOS/instantCLI
Length of output: 98
🏁 Script executed:
# Read the performance.rs file to see the full context around lines 131-134
if [ -f "src/doctor/checks/performance.rs" ]; then
echo "=== Full function context (lines 120-175) ==="
sed -n '120,175p' src/doctor/checks/performance.rs
fiRepository: instantOS/instantCLI
Length of output: 2077
🏁 Script executed:
# Search for PowerMode enum definition to understand the mode types
rg "enum PowerMode" --type rs -A 10Repository: instantOS/instantCLI
Length of output: 90
🏁 Script executed:
# Check for any compilation errors related to this file
if [ -f "Cargo.toml" ]; then
echo "=== Checking if Rust project can be compiled ==="
cargo check 2>&1 | head -100
fiRepository: instantOS/instantCLI
Length of output: 222
🏁 Script executed:
# Search for PowerMode enum definition with correct syntax
rg "enum PowerMode" -A 10Repository: instantOS/instantCLI
Length of output: 641
🏁 Script executed:
# Check if the typo causes any compilation issues by looking at all uses of legacy_idenitfier
rg "legacy_idenitfier" src/Repository: instantOS/instantCLI
Length of output: 246
🏁 Script executed:
# Let's also check the broader context to understand PowerMode better
rg "PowerMode::" src/doctor/checks/performance.rs -B 2 -A 2Repository: instantOS/instantCLI
Length of output: 1435
Fix the typo in the variable name.
Line 131 has a typo: legacy_idenitfier should be legacy_identifier.
The mapping of both PowerMode::Performance and PowerMode::Balanced to the "performance" governor string is intentional: the legacy sysfs CPU frequency scaling interface (/sys/devices/system/cpu/cpu{N}/cpufreq/scaling_governor) only supports "performance" and "powersave" governors, not a "balanced" mode. Unlike the GNOME interface which supports three distinct modes, the legacy sysfs interface lacks a balanced governor, so the implementation approximates balanced mode using performance.
🐛 Proposed fix for the typo
- let legacy_idenitfier = match mode {
+ let legacy_identifier = match mode {
PowerMode::Performance | PowerMode::Balanced => "performance",
PowerMode::PowerSaver => "powersave",
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let legacy_idenitfier = match mode { | |
| PowerMode::Performance | PowerMode::Balanced => "performance", | |
| PowerMode::PowerSaver => "powersave", | |
| }; | |
| let legacy_identifier = match mode { | |
| PowerMode::Performance | PowerMode::Balanced => "performance", | |
| PowerMode::PowerSaver => "powersave", | |
| }; |
🤖 Prompt for AI Agents
In @src/doctor/checks/performance.rs around lines 131 - 134, There's a typo in
the local variable name: rename legacy_idenitfier to legacy_identifier wherever
it's declared and used (the match creating "performance"/"powersave" for
PowerMode in the code block that defines legacy_idenitfier) so the identifier
spelling is correct and consistent across the file.
| async fn build_power_handle(&self) -> anyhow::Result<Box<dyn PowerHandle>> { | ||
| // We check if powerprofilesctl is available | ||
| let profiled_power = deps::POWERPROFILESDAEMON.is_installed(); | ||
| if profiled_power { | ||
| return Ok(Box::new(GnomePowerHandle::default())); | ||
| } | ||
|
|
||
| // If not, we check if we have access to the sysfiles | ||
| let sys_available = | ||
| TokioFile::open("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors") | ||
| .await | ||
| .is_ok(); | ||
| if sys_available { | ||
| Ok(Box::new(LegacyPowerHandle::default())) | ||
| } else { | ||
| // Unfortunately we have no way to control power management | ||
| Err(anyhow!("Power management is not available")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(performance\.rs|deps|doc)" | head -20Repository: instantOS/instantCLI
Length of output: 577
🏁 Script executed:
cat -n src/doctor/checks/performance.rs | sed -n '189,220p'Repository: instantOS/instantCLI
Length of output: 1400
🏁 Script executed:
# Find the deps module and is_installed implementation
fd -t f "deps" src/ --type fRepository: instantOS/instantCLI
Length of output: 122
🏁 Script executed:
# Search for is_installed in the codebase
rg "fn is_installed" -A 5Repository: instantOS/instantCLI
Length of output: 843
🏁 Script executed:
# Find POWERPROFILESDAEMON definition
rg "POWERPROFILESDAEMON" -B 2 -A 2Repository: instantOS/instantCLI
Length of output: 795
🏁 Script executed:
# Get full Dependency implementation
cat -n src/common/package/dependency.rs | head -100Repository: instantOS/instantCLI
Length of output: 4514
🏁 Script executed:
# Check Cargo.toml for tokio version
cat Cargo.toml | grep -A 2 tokioRepository: instantOS/instantCLI
Length of output: 221
🏁 Script executed:
# Verify test.run() implementation
rg "fn run\(" src/common/package/ -A 8Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Find InstallTest definition and run() implementation
rg "impl InstallTest" -A 20Repository: instantOS/instantCLI
Length of output: 644
🏁 Script executed:
# Also search for enum InstallTest
rg "enum InstallTest" -A 30Repository: instantOS/instantCLI
Length of output: 1306
🏁 Script executed:
# Check if try_exists is available in tokio 1.47.1 by searching usage in codebase
rg "try_exists" src/Repository: instantOS/instantCLI
Length of output: 46
🏁 Script executed:
# Check what tests are used for POWERPROFILESDAEMON
cat -n src/common/deps.rs | grep -A 15 "POWERPROFILESDAEMON"Repository: instantOS/instantCLI
Length of output: 544
Optimize file existence check and address synchronous blocking call in async context.
Two actionable issues:
-
Line 196:
is_installed()callswhich::which(), a synchronous I/O operation that can block the async runtime. Consider spawning this check on a blocking thread pool usingtokio::task::spawn_blocking()to avoid blocking the async executor. -
Lines 202-205: Using
TokioFile::open()to check file existence is inefficient. Replace withtokio::fs::try_exists(), which is the proper API for async file existence checks.
♻️ Use try_exists for file check
// If not, we check if we have access to the sysfiles
- let sys_available =
- TokioFile::open("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors")
- .await
- .is_ok();
+ let sys_available = tokio::fs::try_exists(
+ "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"
+ )
+ .await
+ .unwrap_or(false);
if sys_available {
Ok(Box::new(LegacyPowerHandle::default()))🤖 Prompt for AI Agents
In @src/doctor/checks/performance.rs around lines 194 - 212, In
build_power_handle, avoid blocking the async runtime: run the synchronous which
check inside tokio::task::spawn_blocking by moving
deps::POWERPROFILESDAEMON.is_installed() into a spawn_blocking call and await
its boolean result (preserving the early-return behavior that returns
GnomePowerHandle when true); and replace the inefficient
TokioFile::open(...).await.is_ok() existence check with
tokio::fs::try_exists("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors").await
to decide whether to return LegacyPowerHandle or the error.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/doctor/checks/performance.rs:
- Around line 38-48: Update parsing and fallback for legacy sysfs governors:
make PowerMode parsing case-insensitive by adding the strum attribute (e.g.,
#[strum(ascii_case_insensitive)] or equivalent) to the PowerMode enum, and
change LegacyPowerHandle::query_performance_mode so that when
PowerMode::from_str(...) returns Err, you map common governors like "schedutil",
"ondemand", "conservative", etc. to PowerMode::Balanced before treating it as an
unsupported/Skipped case; only treat truly unknown values as skipped and
preserve existing behavior for explicit "performance" and "power-saver" strings.
🧹 Nitpick comments (2)
src/doctor/checks/performance.rs (2)
25-35: Trait docs / signature mismatch + consider not exporting this API publicly.
- Doc says “Returns true on success” (Line 30-31) but
change_performance_modereturnsanyhow::Result<()>.- If
PowerHandleisn’t meant as a public API, considerpub(crate)to avoid locking the crate into this interface.
211-217: Don’t drop the root cause when returningSkipped.
executecurrently turns all failures intoSkipped("Could not query performance mode")(Line 211-216). Including the error (or at least logging it) will make field debugging much faster.Proposed fix
async fn execute(&self) -> CheckStatus { - if let Ok(status) = self.try_execute().await { - status - } else { - CheckStatus::Skipped("Could not query performance mode".into()) - } + match self.try_execute().await { + Ok(status) => status, + Err(e) => CheckStatus::Skipped(format!("Could not query performance mode: {e:#}")), + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/doctor/checks/performance.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Avoid test flakiness by using preparedINS_BINhelpers intests/helpers.sh
Rust style: use edition 2024, preferanyhow::Resultor typed errors viathiserror; use?operator overunwrap/expect
Group imports as: std/prelude, third-party, crate; avoid unusedpubexposures; keepuse crate::...paths concise
Use descriptive snake_case for functions and variables, CamelCase for types, SCREAMING_SNAKE_CASE for constants and environment keys
Bubble errors with context usinganyhow::Context; log user-facing messages and avoid silent failures
Respect single-thread test configuration and avoid spawning unnecessary threads in testsAlways compile the code after making changes to verify correctness with the compiler. Use
cargo checkfor quick syntax checks orcargo buildfor full compilation.Always compile the code after making changes to verify correctness with the compiler using
cargo checkfor quick syntax checks orcargo buildfor full compilation
Files:
src/doctor/checks/performance.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Support multi-line confirmation dialogs with custom yes/no text for critical operations like game removal
Files:
src/doctor/checks/performance.rs
🧠 Learnings (1)
📚 Learning: 2025-12-06T13:52:52.247Z
Learnt from: CR
Repo: instantOS/instantCLI PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T13:52:52.247Z
Learning: Tests default to 1 thread (configured in `.cargo/config.toml`); run `cargo test` for unit/integration tests
Applied to files:
src/doctor/checks/performance.rs
🧬 Code graph analysis (1)
src/doctor/checks/performance.rs (1)
src/doctor/registry.rs (1)
new(12-38)
🔇 Additional comments (2)
src/doctor/checks/performance.rs (2)
161-179: TestGnomePowerHandlefunctionality before committing to it; fallback to sysfs on probe failure.Currently,
build_power_handleselectsGnomePowerHandlebased solely ondeps::POWERPROFILESDAEMON.is_installed(). On systems wherepowerprofilesctlexists but the daemon is unavailable or the platform is unsupported, this returns a non-functional handle instead of attempting the sysfs fallback.Consider probing the handle's actual functionality (e.g., via
query_performance_mode()) and falling back toLegacyPowerHandleif it fails:Proposed probe-and-fallback approach
impl PowerHandleFactory { async fn build_power_handle(&self) -> anyhow::Result<Box<dyn PowerHandle>> { // We check if powerprofilesctl is available let profiled_power = deps::POWERPROFILESDAEMON.is_installed(); if profiled_power { - return Ok(Box::new(GnomePowerHandle::default())); + let handle = GnomePowerHandle::default(); + if handle.query_performance_mode().await.is_ok() { + return Ok(Box::new(handle)); + } } // If not, we check if we have access to the sysfiles let sys_available = TokioFile::open("/sys/devices/system/cpu/cpufreq/policy0/scaling_available_governors") .await .is_ok();
223-230: Privilege level may be overly strict ifpowerprofilesctlhandles polkit negotiation—verify UX intent.
fix_privilege_levelalways returnsRoot(lines 228–230). By default,powerprofilesctluses polkit (net.hadess.PowerProfiles.*) which allows active local session users to switch profiles without root authentication. However, if the implementation has an sysfs fallback, that path genuinely requires root. If multiple handles with different privilege requirements exist, hardcodingRootcould be a UX regression for the polkit path or insufficient for the sysfs path. Consider whetherfix_privilege_levelshould vary based on which handle is actually used.
| #[derive(PartialEq, EnumString, Debug, Display)] | ||
| pub enum PowerMode { | ||
| #[strum(serialize = "performance")] | ||
| Performance, | ||
|
|
||
| #[strum(serialize = "balanced")] | ||
| Balanced, | ||
|
|
||
| #[strum(serialize = "power-saver", serialize = "powersave")] | ||
| PowerSaver, | ||
| } |
There was a problem hiding this comment.
Legacy sysfs governor parsing will likely skip on common defaults (e.g., schedutil).
LegacyPowerHandle::query_performance_mode parses /sys/.../scaling_governor into PowerMode (Line 120-122). Many systems report governors like schedutil (or others), which will fail from_str and cause the check to become Skipped (Line 211-216), defeating the feature on those machines.
Proposed fix (map common governors to “Balanced”, and make parsing case-insensitive)
use crate::common::deps;
use crate::doctor::{CheckStatus, DoctorCheck, PrivilegeLevel};
use anyhow::anyhow;
use async_trait::async_trait;
use std::str::FromStr;
use strum_macros::{Display, EnumString};
use tokio::fs::File as TokioFile;
use tokio::process::Command as TokioCommand;
/// Performance modes
-#[derive(PartialEq, EnumString, Debug, Display)]
+#[derive(PartialEq, EnumString, Debug, Display)]
+#[strum(ascii_case_insensitive)]
pub enum PowerMode {
#[strum(serialize = "performance")]
Performance,
- #[strum(serialize = "balanced")]
+ // sysfs governors frequently map closest to a “balanced” experience
+ #[strum(serialize = "balanced", serialize = "schedutil")]
Balanced,
#[strum(serialize = "power-saver", serialize = "powersave")]
PowerSaver,
}Also applies to: 117-122
🤖 Prompt for AI Agents
In @src/doctor/checks/performance.rs around lines 38 - 48, Update parsing and
fallback for legacy sysfs governors: make PowerMode parsing case-insensitive by
adding the strum attribute (e.g., #[strum(ascii_case_insensitive)] or
equivalent) to the PowerMode enum, and change
LegacyPowerHandle::query_performance_mode so that when PowerMode::from_str(...)
returns Err, you map common governors like "schedutil", "ondemand",
"conservative", etc. to PowerMode::Balanced before treating it as an
unsupported/Skipped case; only treat truly unknown values as skipped and
preserve existing behavior for explicit "performance" and "power-saver" strings.
| async fn query_performance_mode(&self) -> anyhow::Result<PowerMode> { | ||
| let output = TokioCommand::new(PP_CTL).arg("get").output().await?; | ||
| if output.status.success() { | ||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
| Ok(PowerMode::from_str(stdout.as_ref().trim())?) | ||
| } else { | ||
| Err(anyhow!("{} get returned non-zero value", PP_CTL)) | ||
| } | ||
| } | ||
|
|
||
| async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> { | ||
| let gnome_identifier = match mode { | ||
| PowerMode::Performance => "performance", | ||
| PowerMode::Balanced => "balanced", | ||
| PowerMode::PowerSaver => "power-saver", | ||
| }; | ||
|
|
||
| let success = TokioCommand::new(PP_CTL) | ||
| .args(["set", gnome_identifier]) | ||
| .output() | ||
| .await | ||
| .map(|output| output.status.success())?; | ||
| if !success { | ||
| Err(anyhow!("Failed to set power-saver mode")) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/doctor/checks/performance.rs | head -100Repository: instantOS/instantCLI
Length of output: 184
🏁 Script executed:
rg -A 5 "async fn query_performance_mode|async fn change_performance_mode" src/doctor/checks/performance.rsRepository: instantOS/instantCLI
Length of output: 184
🌐 Web query:
powerprofilesctl output format JSON machine-readable
💡 Result:
Short answer: powerprofilesctl does not provide a built‑in JSON / machine‑readable output mode. If you need machine‑readable data, query the power‑profiles‑daemon D‑Bus interface (org.freedesktop.PowerProfiles) directly (eg. with gdbus, dbus-send or python‑dbus) or parse powerprofilesctl text output in a script.
Do you want example gdbus/dbus-send commands or a short Python script to read the profile info from D‑Bus?
Add timeout + include stderr/context for powerprofilesctl failures; fix hardcoded error message.
The code has three issues:
-
Missing timeouts (lines 58, 74–78):
powerprofilesctlcalls can hang indefinitely. Wrap withtokio::time::timeout(Duration::from_secs(2), ...)to prevent blocking. -
Hardcoded misleading error message (line 80): "Failed to set power-saver mode" is always shown regardless of whether setting performance, balanced, or power-saver mode. Use the actual mode value in the error message.
-
Missing stderr in errors: Both functions drop stderr, making debugging harder for CI and users. Capture and include
String::from_utf8_lossy(&output.stderr)in error messages.
Proposed fix
use crate::common::deps;
use crate::doctor::{CheckStatus, DoctorCheck, PrivilegeLevel};
-use anyhow::anyhow;
+use anyhow::{anyhow, Context};
use async_trait::async_trait;
use std::str::FromStr;
use strum_macros::{Display, EnumString};
use tokio::fs::File as TokioFile;
use tokio::process::Command as TokioCommand;
+use tokio::time::{timeout, Duration};
#[async_trait]
impl PowerHandle for GnomePowerHandle {
async fn query_performance_mode(&self) -> anyhow::Result<PowerMode> {
- let output = TokioCommand::new(PP_CTL).arg("get").output().await?;
- if output.status.success() {
+ let output = timeout(
+ Duration::from_secs(2),
+ TokioCommand::new(PP_CTL).arg("get").output(),
+ )
+ .await
+ .context("powerprofilesctl get timed out")??;
+
+ if output.status.success() {
let stdout = String::from_utf8_lossy(&output.stdout);
Ok(PowerMode::from_str(stdout.as_ref().trim())?)
} else {
- Err(anyhow!("{} get returned non-zero value", PP_CTL))
+ let stderr = String::from_utf8_lossy(&output.stderr);
+ Err(anyhow!(
+ "{} get returned non-zero value: {}",
+ PP_CTL,
+ stderr.trim()
+ ))
}
}
async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> {
let gnome_identifier = match mode {
PowerMode::Performance => "performance",
PowerMode::Balanced => "balanced",
PowerMode::PowerSaver => "power-saver",
};
- let success = TokioCommand::new(PP_CTL)
- .args(["set", gnome_identifier])
- .output()
- .await
- .map(|output| output.status.success())?;
- if !success {
- Err(anyhow!("Failed to set power-saver mode"))
- } else {
- Ok(())
- }
+ let output = timeout(
+ Duration::from_secs(2),
+ TokioCommand::new(PP_CTL).args(["set", gnome_identifier]).output(),
+ )
+ .await
+ .context("powerprofilesctl set timed out")??;
+
+ if !output.status.success() {
+ let stderr = String::from_utf8_lossy(&output.stderr);
+ Err(anyhow!(
+ "Failed to set power mode to {}: {}",
+ gnome_identifier,
+ stderr.trim()
+ ))
+ } else {
+ Ok(())
+ }
}
}Verify the fix compiles with cargo check and test with timeout scenarios.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn query_performance_mode(&self) -> anyhow::Result<PowerMode> { | |
| let output = TokioCommand::new(PP_CTL).arg("get").output().await?; | |
| if output.status.success() { | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| Ok(PowerMode::from_str(stdout.as_ref().trim())?) | |
| } else { | |
| Err(anyhow!("{} get returned non-zero value", PP_CTL)) | |
| } | |
| } | |
| async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> { | |
| let gnome_identifier = match mode { | |
| PowerMode::Performance => "performance", | |
| PowerMode::Balanced => "balanced", | |
| PowerMode::PowerSaver => "power-saver", | |
| }; | |
| let success = TokioCommand::new(PP_CTL) | |
| .args(["set", gnome_identifier]) | |
| .output() | |
| .await | |
| .map(|output| output.status.success())?; | |
| if !success { | |
| Err(anyhow!("Failed to set power-saver mode")) | |
| } else { | |
| Ok(()) | |
| } | |
| } | |
| async fn query_performance_mode(&self) -> anyhow::Result<PowerMode> { | |
| let output = timeout( | |
| Duration::from_secs(2), | |
| TokioCommand::new(PP_CTL).arg("get").output(), | |
| ) | |
| .await | |
| .context("powerprofilesctl get timed out")??; | |
| if output.status.success() { | |
| let stdout = String::from_utf8_lossy(&output.stdout); | |
| Ok(PowerMode::from_str(stdout.as_ref().trim())?) | |
| } else { | |
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| Err(anyhow!( | |
| "{} get returned non-zero value: {}", | |
| PP_CTL, | |
| stderr.trim() | |
| )) | |
| } | |
| } | |
| async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> { | |
| let gnome_identifier = match mode { | |
| PowerMode::Performance => "performance", | |
| PowerMode::Balanced => "balanced", | |
| PowerMode::PowerSaver => "power-saver", | |
| }; | |
| let output = timeout( | |
| Duration::from_secs(2), | |
| TokioCommand::new(PP_CTL).args(["set", gnome_identifier]).output(), | |
| ) | |
| .await | |
| .context("powerprofilesctl set timed out")??; | |
| if !output.status.success() { | |
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| Err(anyhow!( | |
| "Failed to set power mode to {}: {}", | |
| gnome_identifier, | |
| stderr.trim() | |
| )) | |
| } else { | |
| Ok(()) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/doctor/checks/performance.rs:
- Around line 67-84: The error message in change_performance_mode always says
"Failed to set power-saver mode" even when other modes are requested; update the
Err returned when !success to include the requested mode (use PowerMode or
gnome_identifier) so the log/error reports which mode failed to apply (reference
function change_performance_mode, enum PowerMode, local variable
gnome_identifier and PP_CTL).
🧹 Nitpick comments (2)
src/doctor/checks/performance.rs (2)
131-134: Typo in variable name.
legacy_idenitfiershould belegacy_identifier.Proposed fix
- let legacy_idenitfier = match mode { + let legacy_identifier = match mode { PowerMode::Performance | PowerMode::Balanced => "performance", PowerMode::PowerSaver => "powersave", };Also update the usage on line 138:
- legacy_idenitfier, + legacy_identifier,
214-220: Consider preserving error context in skipped status.When
try_execute()fails, the actual error is discarded, making debugging harder. Consider including the error details.Proposed fix
async fn execute(&self) -> CheckStatus { - if let Ok(status) = self.try_execute().await { - status - } else { - CheckStatus::Skipped("Could not query performance mode".into()) + match self.try_execute().await { + Ok(status) => status, + Err(e) => CheckStatus::Skipped(format!("Could not query performance mode: {}", e)), } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/doctor/checks/performance.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Avoid test flakiness by using preparedINS_BINhelpers intests/helpers.sh
Rust style: use edition 2024, preferanyhow::Resultor typed errors viathiserror; use?operator overunwrap/expect
Group imports as: std/prelude, third-party, crate; avoid unusedpubexposures; keepuse crate::...paths concise
Use descriptive snake_case for functions and variables, CamelCase for types, SCREAMING_SNAKE_CASE for constants and environment keys
Bubble errors with context usinganyhow::Context; log user-facing messages and avoid silent failures
Respect single-thread test configuration and avoid spawning unnecessary threads in testsAlways compile the code after making changes to verify correctness with the compiler. Use
cargo checkfor quick syntax checks orcargo buildfor full compilation.Always compile the code after making changes to verify correctness with the compiler using
cargo checkfor quick syntax checks orcargo buildfor full compilation
Files:
src/doctor/checks/performance.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Support multi-line confirmation dialogs with custom yes/no text for critical operations like game removal
Files:
src/doctor/checks/performance.rs
🧠 Learnings (1)
📚 Learning: 2025-12-06T13:52:52.247Z
Learnt from: CR
Repo: instantOS/instantCLI PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T13:52:52.247Z
Learning: Tests default to 1 thread (configured in `.cargo/config.toml`); run `cargo test` for unit/integration tests
Applied to files:
src/doctor/checks/performance.rs
🧬 Code graph analysis (1)
src/doctor/checks/performance.rs (1)
src/doctor/registry.rs (1)
new(12-38)
🔇 Additional comments (4)
src/doctor/checks/performance.rs (4)
1-21: LGTM!Imports are well-organized and the module documentation clearly explains the dual-approach power management strategy (powerprofilesctl vs. sysfs fallback).
22-34: LGTM!The trait design is clean with appropriate
Send + Syncbounds for async usage. The decision to returnVec<PowerMode>directly fromavailable_modes(rather thanResult) aligns with the implementation pattern of gracefully degrading to an empty list on errors.
36-48: LGTM!Good use of
strumfor bidirectional string conversion. Theascii_case_insensitiveattribute and dual serialization forPowerSaver("power-saver" / "powersave") properly handle the different naming conventions betweenpowerprofilesctland sysfs.
157-184: LGTM!The factory properly implements the fallback strategy. The runtime validation via
query_performance_mode()after checkingis_installed()correctly addresses the CI issue mentioned in PR comments wherepowerprofilesctlmay be present but non-functional.
| async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> { | ||
| let gnome_identifier = match mode { | ||
| PowerMode::Performance => "performance", | ||
| PowerMode::Balanced => "balanced", | ||
| PowerMode::PowerSaver => "power-saver", | ||
| }; | ||
|
|
||
| let success = TokioCommand::new(PP_CTL) | ||
| .args(["set", gnome_identifier]) | ||
| .output() | ||
| .await | ||
| .map(|output| output.status.success())?; | ||
| if !success { | ||
| Err(anyhow!("Failed to set power-saver mode")) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading error message.
The error message on line 80 always says "Failed to set power-saver mode" regardless of which mode was requested. Include the actual mode in the error for clarity.
Proposed fix
- if !success {
- Err(anyhow!("Failed to set power-saver mode"))
+ if !success {
+ Err(anyhow!("Failed to set power mode to {}", gnome_identifier))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> { | |
| let gnome_identifier = match mode { | |
| PowerMode::Performance => "performance", | |
| PowerMode::Balanced => "balanced", | |
| PowerMode::PowerSaver => "power-saver", | |
| }; | |
| let success = TokioCommand::new(PP_CTL) | |
| .args(["set", gnome_identifier]) | |
| .output() | |
| .await | |
| .map(|output| output.status.success())?; | |
| if !success { | |
| Err(anyhow!("Failed to set power-saver mode")) | |
| } else { | |
| Ok(()) | |
| } | |
| } | |
| async fn change_performance_mode(&self, mode: PowerMode) -> anyhow::Result<()> { | |
| let gnome_identifier = match mode { | |
| PowerMode::Performance => "performance", | |
| PowerMode::Balanced => "balanced", | |
| PowerMode::PowerSaver => "power-saver", | |
| }; | |
| let success = TokioCommand::new(PP_CTL) | |
| .args(["set", gnome_identifier]) | |
| .output() | |
| .await | |
| .map(|output| output.status.success())?; | |
| if !success { | |
| Err(anyhow!("Failed to set power mode to {}", gnome_identifier)) | |
| } else { | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/doctor/checks/performance.rs around lines 67 - 84, The error message in
change_performance_mode always says "Failed to set power-saver mode" even when
other modes are requested; update the Err returned when !success to include the
requested mode (use PowerMode or gnome_identifier) so the log/error reports
which mode failed to apply (reference function change_performance_mode, enum
PowerMode, local variable gnome_identifier and PP_CTL).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.