fix(nix-darwin): add battery sleep policy - #1258
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a services module loader to nix-darwin and a new pmset battery-policy service that installs an activation script and a launchd daemon to adjust macOS sleep and powernap settings based on battery percentage. Changes
Sequence Diagram(s)sequenceDiagram
participant System as System Startup
participant Activation as Activation Scripts
participant Launchd as launchd Daemon
participant Script as power-policy.sh
participant Pmset as pmset Utility
System->>Activation: Trigger activation scripts
Activation->>Script: Execute pmsetBatteryPolicy script
System->>Launchd: Load daemon (com.shunkakinoki...)
Launchd->>Launchd: Schedule daily (StartInterval: 86400)
rect rgba(200,150,255,0.5)
Launchd->>Script: Execute on schedule / RunAtLoad
Script->>Pmset: Query battery percentage (-g batt)
Pmset-->>Script: Return battery data
Script->>Script: Parse %, decide high/low policy
Script->>Pmset: Apply -b sleep <minutes>
Script->>Pmset: Apply -b powernap <0|1>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Nix-Darwin configuration module designed to intelligently manage macOS sleep settings based on the device's battery level. It aims to optimize power consumption and user experience by dynamically adjusting sleep timers and Power Nap behavior, ensuring the system sleeps for longer periods when sufficient battery is available and more aggressively when the battery is low. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Mesa DescriptionTL;DRAdded a dedicated nix-darwin battery policy service that keeps AC sleep disabled while making battery sleep depend on the current battery percentage. The What changed?
What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new power management configuration for macOS, which adjusts sleep settings based on the battery level. The implementation uses a shell script managed by a launchd daemon. My review has identified a significant issue in how the battery percentage is parsed within this script, which could lead to incorrect behavior. I've provided a more robust solution to ensure the power policy functions correctly on various systems.
| set -eu | ||
|
|
||
| # Idle sleep timers do not override the hardware lid-close sleep path. | ||
| batteryPercentage="$(${pmsetBin} -g batt | ${awkBin} -F';' 'NR == 2 { gsub(/[^0-9]/, "", $1); print $1; exit }')" |
There was a problem hiding this comment.
The current awk command to parse the battery percentage is not robust. The use of gsub(/[^0-9]/, "", $1) can lead to incorrect values by concatenating all digits present in the line, such as battery IDs, instead of just the percentage. For example, on some systems, an output like -InternalBattery-0 (id=5046371) 100% would incorrectly result in 05046371100.
A more reliable approach is to specifically extract the number that is followed by a % symbol.
batteryPercentage="$(${pmsetBin} -g batt | ${awkBin} -F';' 'NR == 2 { sub(/.*[ \t]/, "", $1); sub(/%/, "", $1); print $1; exit }')"
587dff1 to
ba63d47
Compare
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated nix-darwin power-management module to enforce a battery-percentage-based pmset sleep/powernap policy, applied both at activation time and periodically via a launchd daemon.
Changes:
- Import a new
powermodule fromnix-darwin/config/power.nixin the main nix-darwin module list. - Introduce an activation script and a daily
launchddaemon to applypmsetsettings for AC and battery, with battery behavior conditional on charge level.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| nix-darwin/default.nix | Wires the new power module into the nix-darwin imports list. |
| nix-darwin/config/power.nix | Implements the pmset battery/AC policy and schedules periodic refresh via launchd. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set -eu | ||
|
|
||
| # Idle sleep timers do not override the hardware lid-close sleep path. | ||
| batteryPercentage="$(${pmsetBin} -g batt | ${awkBin} -F';' 'NR == 2 { gsub(/[^0-9]/, "", $1); print $1; exit }')" |
There was a problem hiding this comment.
batteryPercentage parsing is incorrect: stripping all non-digits from the entire first field will also capture digits from strings like InternalBattery-0 and (id=1234567), producing values like 0123456795 instead of 95. This will make the -ge comparison almost always true and apply the high-battery policy even when the battery is low. Please extract just the number immediately preceding % (e.g., via an awk regex match on /([0-9]+)%/).
| batteryPercentage="$(${pmsetBin} -g batt | ${awkBin} -F';' 'NR == 2 { gsub(/[^0-9]/, "", $1); print $1; exit }')" | |
| batteryPercentage="$(${pmsetBin} -g batt | ${awkBin} -F';' 'NR == 2 { if (match($0, /[0-9]+%/)) { print substr($0, RSTART, RLENGTH-1); } exit }')" |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
6c9636c to
a02b2ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nix-darwin/services/pmset-battery-policy/default.nix (1)
7-9: Consider documenting the threshold and sleep values.The magic numbers would benefit from inline comments explaining the rationale:
highBatteryThreshold = 30- Why 30%?highBatterySleepMinutes = 300- 5 hours idle sleep when battery is healthylowBatterySleepMinutes = 30- 30 minutes when battery is lowThis aids future maintainability.
📝 Suggested documentation
let + # Battery percentage at or above which we use relaxed sleep settings highBatteryThreshold = 30; + # Idle sleep after 5 hours when battery is >= threshold highBatterySleepMinutes = 300; + # Aggressive 30-minute idle sleep when battery is < threshold lowBatterySleepMinutes = 30;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nix-darwin/services/pmset-battery-policy/default.nix` around lines 7 - 9, The three magic-number settings (highBatteryThreshold, highBatterySleepMinutes, lowBatterySleepMinutes) lack explanation; add brief inline comments next to each declaration describing the rationale and units (e.g., "30 = 30% battery threshold for 'healthy' state", "300 = 300 minutes (5 hours) idle sleep when battery >= threshold", "30 = 30 minutes idle sleep when battery < threshold") so future readers understand why these specific values were chosen and what units they represent.nix-darwin/services/pmset-battery-policy/power-policy.sh (1)
11-11: Consider adding resilience for varyingpmsetoutput formats.The awk pattern assumes a specific
pmset -g battoutput format. macOS versions may vary in output structure. If the percentage extraction fails (empty string), the script silently defaults to low-battery settings, which may be unexpected behavior on AC power or when the battery format changes.Consider adding a fallback or logging when
batteryPercentageis empty:💡 Suggested improvement
batteryPercentage="$($pmset_bin -g batt | $awk_bin -F';' 'NR == 2 { gsub(/[^0-9]/, "", $1); print $1; exit }')" + +if [ -z "$batteryPercentage" ]; then + echo "Warning: Could not determine battery percentage, using conservative defaults" >&2 +fi + batterySleepMinutes=$low_battery_sleep_minutes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nix-darwin/services/pmset-battery-policy/power-policy.sh` at line 11, The batteryPercentage extraction using batteryPercentage="$($pmset_bin -g batt | $awk_bin -F';' 'NR == 2 { gsub(/[^0-9]/, "", $1); print $1; exit }')" is brittle for varying pmset output; update the script to detect and handle an empty or missing batteryPercentage: run a more robust parse (e.g., search for the first numeric percentage token from $pmset_bin -g batt or fall back to checking "Now drawing from" / "AC Power" text), and if batteryPercentage is empty set a safe default or skip applying low-battery policy while logging a warning via the existing logger/echo so the script doesn’t silently apply low-power settings on AC power; reference the batteryPercentage variable and the $pmset_bin/$awk_bin parsing pipeline when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nix-darwin/services/pmset-battery-policy/default.nix`:
- Around line 7-9: The three magic-number settings (highBatteryThreshold,
highBatterySleepMinutes, lowBatterySleepMinutes) lack explanation; add brief
inline comments next to each declaration describing the rationale and units
(e.g., "30 = 30% battery threshold for 'healthy' state", "300 = 300 minutes (5
hours) idle sleep when battery >= threshold", "30 = 30 minutes idle sleep when
battery < threshold") so future readers understand why these specific values
were chosen and what units they represent.
In `@nix-darwin/services/pmset-battery-policy/power-policy.sh`:
- Line 11: The batteryPercentage extraction using
batteryPercentage="$($pmset_bin -g batt | $awk_bin -F';' 'NR == 2 {
gsub(/[^0-9]/, "", $1); print $1; exit }')" is brittle for varying pmset output;
update the script to detect and handle an empty or missing batteryPercentage:
run a more robust parse (e.g., search for the first numeric percentage token
from $pmset_bin -g batt or fall back to checking "Now drawing from" / "AC Power"
text), and if batteryPercentage is empty set a safe default or skip applying
low-battery policy while logging a warning via the existing logger/echo so the
script doesn’t silently apply low-power settings on AC power; reference the
batteryPercentage variable and the $pmset_bin/$awk_bin parsing pipeline when
making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 477a70a1-814f-4fcb-a953-2d017c36e81c
📒 Files selected for processing (4)
nix-darwin/default.nixnix-darwin/services/default.nixnix-darwin/services/pmset-battery-policy/default.nixnix-darwin/services/pmset-battery-policy/power-policy.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/pmset_battery_policy_spec.sh (1)
54-68: Add an exact-threshold test case (BATTERY_PERCENTAGE=30).Line 54 and Line 63 validate above/below, but not the
>=boundary itself. A dedicated equality case will lock in intended behavior and prevent off-by-one regressions.Suggested test addition
Describe 'percentage-based policy' Before 'setup_policy_script' After 'cleanup_policy_script' +It 'uses high-battery policy exactly at the threshold' +When run bash -c ': >"'"$PMSET_LOG"'"; BATTERY_PERCENTAGE=30 PMSET_LOG="'"$PMSET_LOG"'" "'"$PREPROCESSED_SCRIPT"'" >/dev/null && cat "'"$PMSET_LOG"'"' +The status should be success +The output should include '-b sleep 300' +The output should include '-b powernap 1' +End + It 'extends battery sleep at or above the threshold' When run bash -c ': >"'"$PMSET_LOG"'"; BATTERY_PERCENTAGE=35 PMSET_LOG="'"$PMSET_LOG"'" "'"$PREPROCESSED_SCRIPT"'" >/dev/null && cat "'"$PMSET_LOG"'"' The status should be success🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/pmset_battery_policy_spec.sh` around lines 54 - 68, Add a new spec case in spec/pmset_battery_policy_spec.sh that sets BATTERY_PERCENTAGE=30 (using the same bash invocation pattern with PMSET_LOG and PREPROCESSED_SCRIPT) to exercise the exact-threshold branch; assert the status is success and that the output includes the same pmset flags as the "extends battery sleep at or above the threshold" case (check for '-c sleep 0', '-c powernap 1', '-b sleep 300', '-b powernap 1') so the behavior at BATTERY_PERCENTAGE=30 is explicitly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/pmset_battery_policy_spec.sh`:
- Around line 54-68: Add a new spec case in spec/pmset_battery_policy_spec.sh
that sets BATTERY_PERCENTAGE=30 (using the same bash invocation pattern with
PMSET_LOG and PREPROCESSED_SCRIPT) to exercise the exact-threshold branch;
assert the status is success and that the output includes the same pmset flags
as the "extends battery sleep at or above the threshold" case (check for '-c
sleep 0', '-c powernap 1', '-b sleep 300', '-b powernap 1') so the behavior at
BATTERY_PERCENTAGE=30 is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03a11469-2d8d-417a-83ba-7c5b0dfaba14
📒 Files selected for processing (6)
nix-darwin/default.nixnix-darwin/services/default.nixnix-darwin/services/pmset-battery-policy/default.nixnix-darwin/services/pmset-battery-policy/power-policy.shspec/coverage_spec.shspec/pmset_battery_policy_spec.sh
✅ Files skipped from review due to trivial changes (2)
- nix-darwin/services/default.nix
- nix-darwin/services/pmset-battery-policy/power-policy.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- nix-darwin/default.nix
- nix-darwin/services/pmset-battery-policy/default.nix
Entire-Checkpoint: 60127ce9eb7b
a02b2ac to
2ddf78c
Compare
Add a dedicated nix-darwin battery policy service that keeps AC sleep disabled while making battery sleep depend on the current battery percentage.
The implementation now lives under
nix-darwin/services/pmset-battery-policy/, with the pmset logic extracted into a standalone shell script and injected with concrete paths and thresholds viapkgs.replaceVars. This matches the repo pattern of keeping substantive shell logic in standalone scripts instead of embedding it inline in Nix modules.The default macOS lid-close path is hardware-driven, so this change configures the truthful pmset knobs instead of pretending the idle sleep timer can keep a closed laptop awake. On battery, the service defaults to a conservative 30-minute sleep window and only extends it to 300 minutes when the battery is at least 30%.
Added ShellSpec coverage for the standalone battery policy script and updated the shell coverage inventory. The new tests also caught and fixed a parsing bug where the script could accidentally treat the battery line digits from
id=...as part of the battery percentage.Verified with
make shell-test,nix build .#checks.aarch64-darwin.eval-darwin-default .#checks.aarch64-darwin.eval-darwin-runner, andnix eval --raw .#darwinConfigurations.aarch64-darwin.config.system.activationScripts.pmsetBatteryPolicy.text | bash -n.