fix(noctalia): lock before lid suspend - #1688
Conversation
|
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a lock-before-sleep script and a systemd user oneshot service that runs it before ChangesScreen Lock Before Sleep
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Mesa DescriptionTL;DR
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 introduces a new systemd user service, noctalia-lock-before-sleep, designed to lock the screen via noctalia-shell before the system enters a sleep state. Feedback highlights that sleep.target is a system-level target and may not correctly trigger a user-level service without a proxy. Additionally, the service appears to duplicate existing configuration settings, and the use of a manual sleep command to prevent race conditions is noted as a fragile workaround that should be replaced with a synchronous IPC call.
| systemd.user.services.noctalia-lock-before-sleep = { | ||
| Unit = { | ||
| Description = "Lock Noctalia before system sleep"; | ||
| Before = [ "sleep.target" ]; |
There was a problem hiding this comment.
sleep.target is a system-level target. By default, systemd --user instances do not have a sleep.target, so this service will not be triggered upon suspension unless a proxy like systemd-lock-handler is used. For a more robust implementation, a system-level service or a dedicated locker manager is typically required.
| Install.WantedBy = [ "graphical-session.target" ]; | ||
| }; | ||
|
|
||
| systemd.user.services.noctalia-lock-before-sleep = { |
There was a problem hiding this comment.
This service duplicates the functionality of the lockOnSuspend = true; setting at line 102. If the built-in setting is non-functional, consider removing it or adding a comment explaining why this manual service is necessary to avoid maintainability issues.
| Type = "oneshot"; | ||
| ExecStart = pkgs.writeShellScript "noctalia-lock-before-sleep" '' | ||
| ${inputs.noctalia-shell.packages.${pkgs.system}.default}/bin/noctalia-shell ipc call lockScreen lock | ||
| ${pkgs.coreutils}/bin/sleep 1 |
There was a problem hiding this comment.
The sleep 1 command is a fragile workaround for the race condition between the IPC call and system suspension. It is better to use a synchronous IPC call if noctalia-shell supports one, ensuring the lock is fully active before the script exits and the system suspends.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/noctalia/default.nix`:
- Around line 34-37: The current ExecStart script (created via
pkgs.writeShellScript "noctalia-lock-before-sleep") masks failures because
`${inputs.noctalia-shell…}/bin/noctalia-shell ipc call lockScreen lock` is
followed by `${pkgs.coreutils}/bin/sleep 1`; ensure the IPC command's exit
status is propagated by changing the script to either enable strict error
handling (e.g., set -e) at the top of the script or capture the exit code of
`noctalia-shell ipc call lockScreen lock` and `exit` with that code before
running `sleep 1`, so the service will fail (non-zero) when the IPC call fails
rather than always succeeding due to the trailing `sleep`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11474d69-13f0-49c6-b674-66b34972533c
📒 Files selected for processing (1)
config/noctalia/default.nix
Add a user sleep-target hook that calls the same Noctalia lock IPC path as the manual Hyprland lock binding before system sleep continues. Co-authored-by: Codex <noreply@openai.com>
fd88f09 to
2360d61
Compare
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| Type = "oneshot"; | ||
| ExecStart = pkgs.writeShellScript "noctalia-lock-before-sleep" '' | ||
| ${inputs.noctalia-shell.packages.${pkgs.system}.default}/bin/noctalia-shell ipc call lockScreen lock | ||
| ${pkgs.coreutils}/bin/sleep 1 |
There was a problem hiding this comment.
Lock UI render race: this sleep 1 is a heuristic — if the noctalia lock surface needs more than ~1s to render (cold start, slow GPU init, system under load), the kernel suspends before the lock is on screen, so the desktop is briefly visible on resume. There's no deterministic synchronization here; the script returns 0 either way and the system happily proceeds to suspend. Consider either (a) polling for lock-ready via a noctalia IPC status call with a bounded timeout, or (b) bumping the sleep with a comment explaining the trade-off so it isn't trimmed back later.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/noctalia/default.nix">
<violation number="1" location="config/noctalia/default.nix:30">
P1: Don't hook a user service into sleep.target; user managers don't get that target, so the pre-suspend lock won't fire.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| systemd.user.services.noctalia-lock-before-sleep = { | ||
| Unit = { | ||
| Description = "Lock Noctalia before system sleep"; | ||
| Before = [ "sleep.target" ]; |
There was a problem hiding this comment.
Missing graphical-session ordering: the sibling ac-idle-inhibit unit (lines 13-25) uses After = [ "graphical-session.target" ] and PartOf = [ "graphical-session.target" ], but this new unit has neither. In edge sequences where sleep.target activates while graphical-session.target is inactive, the noctalia-shell IPC call will fail silently (and the script still exits 0). Adding After = [ "graphical-session.target" ] keeps the file consistent and makes the dependency explicit; avoid PartOf here since you don't want the unit stopped mid-suspend.
| Install.WantedBy = [ "graphical-session.target" ]; | ||
| }; | ||
|
|
||
| systemd.user.services.noctalia-lock-before-sleep = { |
There was a problem hiding this comment.
Document the overlap with general.lockOnSuspend: noctalia is already configured with general.lockOnSuspend = true on line 102, so it isn't obvious from the diff alone that this systemd unit is intentionally a second mechanism rather than a duplicate. The commit message explains it (lid-suspend bypasses the in-app hook), but that context is lost once merged. A one-line comment above the unit pointing to services.logind.settings.Login.HandleLidSwitch in named-hosts/matic/default.nix will prevent a future cleanup pass from deleting one of the two and silently regressing lid-close locking.
Mesa DescriptionTL;DRLock Noctalia before suspend to prevent the system sleeping with an unlocked session. Mirrors the Hyprland manual lock command for consistent behavior on lid close. What changed?
Description generated by Mesa. Update settings |
| ${pkgs.coreutils}/bin/sleep 1 | ||
| ''; | ||
| }; | ||
| Install.WantedBy = [ "sleep.target" ]; |
There was a problem hiding this comment.
This unit will likely never run on system suspend. systemd's user-instance sleep.target is not automatically activated when the system enters sleep — there is no built-in bridge from system to per-user systemd manager. This is a long-standing systemd limitation (open RFE: systemd#40387, prior RFE #15477 closed without implementation), and the NixOS Power Management wiki only documents system-level wantedBy = [ "post-resume.target" ] for this reason. I grepped the rest of the dotfiles and there's no user-sleep@.service proxy or systemd-lock-handler configured to bridge the gap. The fix is one of:
- Move this to a system-level
systemd.services.*(innamed-hosts/matic/default.nix) withwantedBy = [ "sleep.target" ]; before = [ "sleep.target" ];and run as your user withXDG_RUNTIME_DIRset so the IPC reaches noctalia. - Add a system-level proxy unit
user-sleep@.servicethat doessystemctl --user start sleep.target— then this existing user unit fires. - Have a system-level Before=sleep.target unit just call
loginctl lock-session, and let noctalia react via the login1LockD-Bus signal (this is essentially whatgeneral.lockOnSuspend = truealready does, which would make the whole new unit unnecessary if the lid path can be made to lock-session correctly).
Please verify with journalctl --user -u noctalia-lock-before-sleep after a real suspend/resume cycle before merging — I expect the unit to never have a recorded activation.
(Apologies for missing this in my initial review — I assumed modern systemd propagates user sleep.target, but the sources above show it does not.)
Add a graphical-session user service that watches the ACPI lid state and calls Noctalia's lock IPC as soon as the lid transitions to closed. Keep the pre-sleep lock as a fallback for actual suspend paths. Co-authored-by: Codex <noreply@openai.com>
Avoid racing GitHub's pull request ref propagation by checking that refs/pull/<number>/head is fetchable before invoking install.sh in E2E. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/noctalia/lid-lock.sh">
<violation number="1" location="config/noctalia/lid-lock.sh:47">
P2: The first observed `closed` state is treated as a lid-close transition, causing an unnecessary lock when the script starts with the lid already closed.</violation>
</file>
<file name="config/noctalia/default.nix">
<violation number="1" location="config/noctalia/default.nix:52">
P3: Gate this lid-lock service with a unit condition so it does not run a permanent polling loop on systems without a lid state file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Hyprland sees the physical Lid Switch directly, while the previous ACPI polling service could miss the close event before logind handled suspend. Bind the switch-close event to the same Noctalia lock IPC as the working manual lock shortcut and remove the polling user service. Co-authored-by: Codex <noreply@openai.com>
Add the DarkMode widget immediately after Brightness on the right bar. Keep Noctalia lock-screen fingerprint auth loaded without pam_fprintd default try or timeout limits, and let Hyprland handle AC lid-close locking while battery lid close still suspends. Co-authored-by: Codex <noreply@openai.com>
| services.logind.settings.Login.HandlePowerKey = "lock"; | ||
| # Suspend on lid close | ||
| # Suspend on battery lid close; on AC, Hyprland locks on the lid switch event. | ||
| services.logind.settings.Login.HandleLidSwitch = "suspend"; |
There was a problem hiding this comment.
Battery lid-close still has a lock-vs-suspend race. With HandleLidSwitch = "suspend", systemd-logind starts the suspend chain immediately on lid close. The Hyprland bindl binding (added in this PR) async-forks noctalia-shell ipc call lockScreen lock in parallel — there’s no synchronization, so the kernel can win the race and suspend before noctalia has processed the IPC, leaving the desktop briefly visible on resume. The fallback noctalia-lock-before-sleep user unit doesn’t help here either (user-instance sleep.target isn’t bridged from system suspend).
The AC path you just added (HandleLidSwitchExternalPower = "ignore") sidesteps this because no suspend occurs. To close the gap on battery, either:
- Set
HandleLidSwitch = "ignore"for battery too, and let a Hyprland binding sequence lock-then-suspend (exec, noctalia-shell ipc call lockScreen lock && sleep 0.5 && systemctl suspend), or - Add a system-level
systemd.services.lock-before-sleeporderedBefore=sleep.targetthat callsloginctl lock-session(logind blocks suspend on the inhibitor until the unit completes; noctalia’s own login1Locksubscription viageneral.lockOnSuspend = truethen locks synchronously).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/coverage_spec.sh (1)
7-311:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
Itentry forconfig/noctalia/lock-before-sleep.shin the spec-existence block.
config/noctalia/lock-before-sleep.shis correctly added tocovered_scripts(line 373), but theDescribe 'all required scripts have spec files'block doesn't get a corresponding path-existence check. Ifspec/noctalia_lock_before_sleep_spec.shis deleted, nothing here will catch it.➕ Proposed addition (insert after line 209, following the same pattern)
+It 'has spec file for config/noctalia/lock-before-sleep.sh' +The path "spec/noctalia_lock_before_sleep_spec.sh" should be exist +End +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/coverage_spec.sh` around lines 7 - 311, The Describe 'all required scripts have spec files' block is missing an It test for config/noctalia/lock-before-sleep.sh; add an It that asserts the path "spec/noctalia_lock_before_sleep_spec.sh" should be exist (matching the pattern used for other entries) and insert it near the other entries (after the block handling similar home-manager/config entries, e.g., following the test around config/paperclip/hydrate.sh) so the presence of spec/noctalia_lock_before_sleep_spec.sh is validated alongside covered_scripts.
🧹 Nitpick comments (1)
spec/noctalia_bar_spec.sh (1)
8-9: ⚡ Quick win
awkpattern is fragile against minor Nix formatting variations.The
gsub(/.*id = \"|\";.*/, "")pattern assumes every widget ID line is formatted as exactlyid = "Foo";with a trailing semicolon and no surrounding content after the semicolon. It will silently produce an empty string (and the test will fail without clear indication why) ifalejandra/nixfmtreformats the line — e.g., dropping the trailing semicolon inside{ }, adding a comment, or usingid="Foo"without spaces.Additionally, the test only asserts
include 'Brightness DarkMode ControlCenter'rather than exact adjacency; if extra widgets are inserted betweenBrightnessandDarkModethe substring check would still fail correctly, but it would also fail if the three widgets are non-contiguous even when ordering is otherwise correct. This is the intended behaviour, so it's fine — just worth noting.Consider a slightly more resilient extraction:
♻️ More resilient awk alternative
- When run bash -c "awk '/widgets.right = \\[/{in_right=1} in_right && /id =/ { gsub(/.*id = \"|\";.*/, \"\"); print } in_right && /\\];/{exit}' '$CONFIG' | paste -sd ' ' -" + When run bash -c "awk '/widgets\\.right *= *\\[/{in_right=1} in_right && /id *=/ { match(\$0,/\"[^\"]+\"/); print substr(\$0,RSTART+1,RLENGTH-2) } in_right && /\\];/{exit}' '$CONFIG' | paste -sd ' ' -"Using
match+substrto extract the quoted value is immune to content before/after the quotes and handles bothid = "Foo"andid="Foo".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/noctalia_bar_spec.sh` around lines 8 - 9, The current awk gsub(/.*id = \"|\";.*/, "") is fragile to spacing, optional semicolons, or trailing comments; update the awk command that extracts widget IDs to use a robust quoted-value extraction (e.g. use match() and substr() to find the first quoted string after an id token or a regex that captures id\s*=\s*"([^"]*)") so it correctly handles variants like id="Foo", id = "Foo"; and trailing comments; keep the surrounding logic (the in_right flag and the right-section exit) intact so the output still lists the widget ids in order for the test assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yml:
- Around line 69-70: The guard loop is checking the wrong ref (refs/pull/${{
github.event.pull_request.number }}/head) while the download uses ${{ github.sha
}}, which on pull_request events refers to the merge commit; update the check to
wait for the exact SHA you later use—either test availability of ${{ github.sha
}} directly or switch the download to use ${{ github.event.pull_request.head.sha
}} so both the guard and fetch use the same ref; ensure the loop command that
currently references refs/pull/... is replaced to verify the chosen SHA/ref
(github.sha or github.event.pull_request.head.sha) before proceeding to
raw.githubusercontent.com.
In `@config/noctalia/lock-before-sleep.sh`:
- Around line 8-10: The script currently exits silently when NOCTALIA_SHELL ipc
call lockScreen lock fails; update the failure branch in lock-before-sleep.sh to
call the injected logger (the `@logger`@ variable provided via pkgs.replaceVars)
with a clear message including the command and its exit status before calling
exit 0 so the journal records the lock failure (refer to NOCTALIA_SHELL and the
ipc call "lockScreen lock" to find the branch to modify).
In `@spec/noctalia_bar_spec.sh`:
- Line 5: CONFIG is currently set using $PWD which can be wrong depending on how
ShellSpec is invoked; change the assignment to use ShellSpec's %SPECROOT (e.g.
CONFIG="%SPECROOT/config/noctalia/default.nix") or compute a path anchored to
the spec file (using dirname on $BASH_SOURCE or $SPEC) so it always resolves
relative to the spec, and add a guard immediately after (e.g. test -r "$CONFIG"
|| shellspec_fail "config not found: $CONFIG") so the spec fails clearly instead
of letting the downstream awk pipeline produce no output.
---
Outside diff comments:
In `@spec/coverage_spec.sh`:
- Around line 7-311: The Describe 'all required scripts have spec files' block
is missing an It test for config/noctalia/lock-before-sleep.sh; add an It that
asserts the path "spec/noctalia_lock_before_sleep_spec.sh" should be exist
(matching the pattern used for other entries) and insert it near the other
entries (after the block handling similar home-manager/config entries, e.g.,
following the test around config/paperclip/hydrate.sh) so the presence of
spec/noctalia_lock_before_sleep_spec.sh is validated alongside covered_scripts.
---
Nitpick comments:
In `@spec/noctalia_bar_spec.sh`:
- Around line 8-9: The current awk gsub(/.*id = \"|\";.*/, "") is fragile to
spacing, optional semicolons, or trailing comments; update the awk command that
extracts widget IDs to use a robust quoted-value extraction (e.g. use match()
and substr() to find the first quoted string after an id token or a regex that
captures id\s*=\s*"([^"]*)") so it correctly handles variants like id="Foo", id
= "Foo"; and trailing comments; keep the surrounding logic (the in_right flag
and the right-section exit) intact so the output still lists the widget ids in
order for the test assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bcac05b-32f0-4157-9a5b-f6133f74f3bc
📒 Files selected for processing (11)
.github/workflows/e2e.ymlconfig/hyprland/hyprland.confconfig/noctalia/default.nixconfig/noctalia/lock-before-sleep.shnamed-hosts/matic/default.nixspec/coverage_spec.shspec/hyprland_lid_lock_spec.shspec/matic_lid_policy_spec.shspec/matic_pam_fingerprint_spec.shspec/noctalia_bar_spec.shspec/noctalia_lock_before_sleep_spec.sh
✅ Files skipped from review due to trivial changes (1)
- spec/noctalia_lock_before_sleep_spec.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- config/noctalia/default.nix
| for attempt in 1 2 3 4 5 6; do | ||
| if git ls-remote --exit-code origin "refs/pull/${{ github.event.pull_request.number }}/head"; then |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Actions pull_request events, what does github.sha point to (head SHA vs merge SHA), and which refs are guaranteed to exist for raw.githubusercontent.com downloads?
💡 Result:
For GitHub Actions workflows triggered by pull_request events (excluding pull_request_target), github.sha points to the SHA of the merge commit (a temporary test merge of the PR head into the base branch). This is the last merge commit on the GITHUB_REF branch, which is refs/pull/<PR number>/merge [1][2][3]. The head SHA is available via github.event.pull_request.head.sha, and base SHA via github.event.pull_request.base.sha [4][5]. actions/checkout by default checks out this merge commit [6]. For raw.githubusercontent.com downloads, the following refs are guaranteed to exist for any repository (public or private, as long as the file/blob is accessible via the token/permissions): - refs/heads/<branch> for any branch (e.g., https://raw.githubusercontent.com/user/repo/refs/heads/main/file.txt) - refs/tags/<tag> for any tag - <commit-sha> directly (e.g., https://raw.githubusercontent.com/user/repo/abc123/file.txt) - refs/pull/<PR number>/head pointing to the head commit of the PR (even for fork PRs) [7][8][9] - refs/pull/<PR number>/merge pointing to the test merge commit (only if the PR is mergeable) [4][1][10] These refs allow serving raw files from branches, tags, commits, and PRs via raw.githubusercontent.com [11].
Citations:
- 1: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 2: https://docs.github.com/en/actions/reference/contexts-reference
- 3: https://raw.githubusercontent.com/github/docs/refs/heads/main/content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md
- 4: https://www.kenmuse.com/blog/the-many-shas-of-a-github-pull-request/
- 5: https://stackoverflow.com/questions/70104600/complete-list-of-github-actions-contexts
- 6: https://stackoverflow.com/questions/63568834/github-actions-which-branch-used-on-pr
- 7: https://stackoverflow.com/questions/17644164/refs-pull-head-origin-pr-appearing-on-a-git-pull
- 8: Replace
gh pr checkoutwithgit fetch refs/pullto avoid GH_HOST issues github/gh-aw#26136 - 9: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
- 10: https://docs.github.com/rest/git/refs
- 11: Add support for GitHub /files/ and raw.githubusercontent.com URL formats in workflow specs github/gh-aw#1407
🏁 Script executed:
cat -n .github/workflows/e2e.yml | sed -n '65,85p'Repository: shunkakinoki/dotfiles
Length of output: 1071
🏁 Script executed:
# Check if there are any recent changes or comments about this workflow
git log --oneline -20 .github/workflows/e2e.yml 2>/dev/null | head -10Repository: shunkakinoki/dotfiles
Length of output: 123
🏁 Script executed:
# Check if there are other uses of github.sha or github.event.pull_request.head.sha in the workflow
rg 'github\.(sha|event\.pull_request)' .github/workflows/e2e.ymlRepository: shunkakinoki/dotfiles
Length of output: 542
This step waits for the wrong ref—ensure the guarded SHA is actually available.
Line 70 checks for refs/pull/<num>/head, but line 82 downloads using ${{ github.sha }}, which on pull_request events points to the merge commit (refs/pull/<num>/merge). These are different refs. The merge ref exists only if the PR is mergeable, so the guard can pass while the SHA used by raw.githubusercontent.com is still unavailable.
Either wait for ${{ github.sha }} availability directly, or fetch using ${{ github.event.pull_request.head.sha }} consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 69 - 70, The guard loop is checking
the wrong ref (refs/pull/${{ github.event.pull_request.number }}/head) while the
download uses ${{ github.sha }}, which on pull_request events refers to the
merge commit; update the check to wait for the exact SHA you later use—either
test availability of ${{ github.sha }} directly or switch the download to use
${{ github.event.pull_request.head.sha }} so both the guard and fetch use the
same ref; ensure the loop command that currently references refs/pull/... is
replaced to verify the chosen SHA/ref (github.sha or
github.event.pull_request.head.sha) before proceeding to
raw.githubusercontent.com.
| if ! "$NOCTALIA_SHELL" ipc call lockScreen lock; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Silent lock failure allows suspend with no log; add a logger call before exit 0.
When lockScreen lock returns non-zero (Noctalia IPC down, daemon crashed mid-session, etc.) the script immediately exits with success. systemd proceeds to suspend an unprotected session, with zero visibility into the failure.
The dual Hyprland bindl reduces the risk but doesn't cover the case where the IPC is broken inside an active session (the bindl would have fired before the hook, and if that failed too, no one will know).
At minimum, journal the failure so it's auditable:
🛡️ Proposed fix: log the failed lock before gracefully exiting
+LOGGER="@logger@"
+
if ! "$NOCTALIA_SHELL" ipc call lockScreen lock; then
+ "$LOGGER" -t noctalia-lock-before-sleep \
+ "WARNING: lockScreen IPC call failed; suspending without a lock screen"
exit 0
fiWire @logger@ in the pkgs.replaceVars call in config/noctalia/default.nix:
+logger = "${pkgs.util-linux}/bin/logger";
noctalia_shell = ...;
sleep = ...;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/noctalia/lock-before-sleep.sh` around lines 8 - 10, The script
currently exits silently when NOCTALIA_SHELL ipc call lockScreen lock fails;
update the failure branch in lock-before-sleep.sh to call the injected logger
(the `@logger`@ variable provided via pkgs.replaceVars) with a clear message
including the command and its exit status before calling exit 0 so the journal
records the lock failure (refer to NOCTALIA_SHELL and the ipc call "lockScreen
lock" to find the branch to modify).
| # shellcheck disable=SC2329 | ||
|
|
||
| Describe 'config/noctalia/default.nix bar widgets' | ||
| CONFIG="$PWD/config/noctalia/default.nix" |
There was a problem hiding this comment.
$PWD-based path may resolve incorrectly depending on invocation context.
CONFIG is assigned at Describe scope using $PWD, which is evaluated when ShellSpec parses/sources the spec file. If ShellSpec is invoked from a directory other than the repository root (e.g., shellspec spec/ from a subdirectory, or via a CI runner with a different $CWD), the path $PWD/config/noctalia/default.nix will be wrong and the awk pipeline will silently produce no output, causing a misleading test failure.
Consider using ShellSpec's %SPECROOT or a relative path anchored to the spec's own location, or add a guard:
🛡️ Proposed guard
+ CONFIG="$PWD/config/noctalia/default.nix"
It 'places DarkMode immediately after Brightness on the right bar'
+ Skip if "config file not found" [ ! -f "$CONFIG" ]
When run bash -c "awk ..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/noctalia_bar_spec.sh` at line 5, CONFIG is currently set using $PWD
which can be wrong depending on how ShellSpec is invoked; change the assignment
to use ShellSpec's %SPECROOT (e.g.
CONFIG="%SPECROOT/config/noctalia/default.nix") or compute a path anchored to
the spec file (using dirname on $BASH_SOURCE or $SPEC) so it always resolves
relative to the spec, and add a guard immediately after (e.g. test -r "$CONFIG"
|| shellspec_fail "config not found: $CONFIG") so the spec fails clearly instead
of letting the downstream awk pipeline produce no output.
1 participant
Summary
sleep.targethook that locks Noctalia before suspendnoctalia-lid-lockservice that watches the ACPI lid state and locks as soon as the lid transitions to closedpkgs.replaceVars, matching the repo's no-inline-script conventioninstall.shValidation
make shell-inline-checkmake nix-format-checkmake shell-checkshellspec spec/noctalia_lid_lock_spec.sh spec/noctalia_lock_before_sleep_spec.sh spec/coverage_spec.shnix run nixpkgs#actionlint -- .github/workflows/e2e.ymlmake build && make switchnoctalia-lid-lock.serviceandnoctalia-lock-before-sleep.serviceare enablednoctalia-lid-lock.serviceis active after switch