chore: sort nix package lists alphabetically - #1198
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR reorders package and plugin lists across multiple Nix configuration files without adding or removing entries. Changes affect home.packages in PHP config, tmux plugins, nix-ld libraries, and build inputs across home-manager and named-hosts configurations. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
📝 Coding Plan
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 significantly refactors how shell scripts are managed within Nix configurations by moving inline scripts to external files, enhancing maintainability and enabling dedicated testing. It also introduces new testing and linting capabilities for both Nix and Python code, alongside general alphabetical sorting of various package lists to improve configuration consistency. Highlights
Ignored Files
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;DRRefactored inline shell/Python scripts in Nix modules to external files for improved maintainability and testability, introduced new testing infrastructure (CI, linters, unit tests), and alphabetized several Nix package lists for consistency. 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 is a significant and valuable refactoring that extracts numerous inline shell scripts from Nix files into separate, lintable, and testable script files. This greatly improves the maintainability and quality of the codebase. The addition of a new nix-inline-check to enforce this convention, along with comprehensive tests for the new scripts, is excellent.
The pull request title and description, which only mention sorting package lists, are misleading and do not capture the scope and importance of these changes. It would be beneficial to update them to accurately reflect the refactoring work done.
I've found one area for improvement to make a script more robust.
| # shellcheck source=/dev/null | ||
| . /etc/falcon-sensor.env |
There was a problem hiding this comment.
For improved robustness, it's a good practice to verify that FALCON_CID is set and not empty after sourcing /etc/falcon-sensor.env. This prevents falconctl from being called with an empty CID, which could lead to unexpected behavior. Since set -u is active, the script will fail if FALCON_CID is not set, but it won't catch the case where it's set to an empty string.
| # shellcheck source=/dev/null | |
| . /etc/falcon-sensor.env | |
| # shellcheck source=/dev/null | |
| . /etc/falcon-sensor.env | |
| # Exit if FALCON_CID is not set or is empty | |
| if [ -z "$FALCON_CID" ]; then | |
| echo "ERROR: FALCON_CID is not set or is empty in /etc/falcon-sensor.env" >&2 | |
| exit 1 | |
| fi |
References
- This rule emphasizes maintaining consistency with established patterns for writing scripts extracted from Nix expressions, even if it involves managing linter warnings like ShellCheck SC2024. The suggested
# shellcheck source=/dev/nulldirective aligns with this principle by explicitly handling ShellCheck's concerns when sourcing files, contributing to consistent script patterns.
There was a problem hiding this comment.
Pull request overview
This PR goes beyond alphabetical reordering: it refactors multiple Nix-defined inline scripts into external script files, adds new script/test infrastructure (ShellSpec + Python), and introduces CI/Make targets to enforce and run those checks.
Changes:
- Refactor inline
write*Script*/writeTextbodies into external*.sh/*.py/ unit files loaded viabuiltins.readFile+pkgs.replaceVars. - Add new validation/test tooling:
nix-inline-check, Python pytest + ruff config, and a dedicated GitHub Actions Python workflow. - Add/expand ShellSpec coverage for multiple scripts and add Python unit tests for the gnome-keyring unlock protocol.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_unlock_gnome_keyring.py | Adds Python unit tests for the gnome-keyring socket protocol implementation. |
| spec/yek_shim_spec.sh | ShellSpec assertions for yek shim wrapper behavior. |
| spec/unlock_gnome_keyring_spec.sh | ShellSpec sanity checks for the unlock Python script. |
| spec/start_postgres_wrapper_spec.sh | ShellSpec checks for docker-postgres wrapper script behavior. |
| spec/pam_gnome_keyring_tpm_unlock_spec.sh | ShellSpec checks for PAM TPM-unlock script structure/logic. |
| spec/install_yek_shim_spec.sh | ShellSpec assertions for install-yek shim wrapper behavior. |
| spec/fishtape_wrapper_spec.sh | ShellSpec checks for the fishtape wrapper script. |
| spec/falcon_init_spec.sh | ShellSpec checks for the Falcon init script. |
| spec/docker_setup_wrapper_spec.sh | ShellSpec checks for docker-setup wrapper script. |
| spec/docker_setup_spec.sh | ShellSpec checks for docker setup script behavior/structure. |
| spec/coverage_spec.sh | Expands the coverage list and required spec files for new scripts. |
| spec/cliproxyapi_docker_start_spec.sh | ShellSpec checks for cliproxyapi docker wrapper. |
| spec/check_nix_inline_scripts_spec.sh | ShellSpec checks for the nix inline-script detector script. |
| scripts/fishtape-wrapper.sh | Adds a bash wrapper to run the fish fishtape plugin as a command. |
| scripts/check-nix-inline-scripts.sh | Adds a repo-wide check to reject inline write*Script* strings in Nix files. |
| pyproject.toml | Adds pytest/ruff config and moves tool deps into a dependency group. |
| named-hosts/matic/unlock-gnome-keyring.py | Adds a Python implementation to unlock gnome-keyring via the control socket protocol. |
| named-hosts/matic/pam-gnome-keyring-tpm-unlock.sh | Adds a PAM exec helper to decrypt a TPM credential and unlock gnome-keyring asynchronously. |
| named-hosts/matic/falcon/default.nix | Sorts buildInputs (minor reorder). |
| named-hosts/matic/falcon.nix | Refactors Falcon init script into an external falcon-init.sh with replaceVars. |
| named-hosts/matic/falcon-init.sh | New external Falcon init script used by the NixOS module. |
| named-hosts/matic/default.nix | Refactors PAM/keyring scripts into external files; sorts nix-ld.libraries. |
| home-manager/services/docker/setup-docker.sh | New external docker setup script used by HM module. |
| home-manager/services/docker/docker.service | New external docker systemd unit template used by HM module. |
| home-manager/services/docker/docker-setup.sh | New wrapper exposing setup-docker as a command. |
| home-manager/services/docker/default.nix | Refactors inline unit/script to external files with replaceVars. |
| home-manager/services/docker-postgres/start-postgres-wrapper.sh | New external wrapper script (used by HM docker-postgres). |
| home-manager/services/docker-postgres/default.nix | Refactors wrapper from inline to external file with replaceVars. |
| home-manager/services/cliproxyapi/scripts/docker-start.sh | New external docker wrapper script for cliproxyapi. |
| home-manager/services/cliproxyapi/default.nix | Refactors docker wrapper from inline to external file with replaceVars. |
| home-manager/programs/tmux/default.nix | Sorts tmux plugins. |
| home-manager/programs/php/default.nix | Sorts home.packages. |
| home-manager/modules/yek/yek-shim.sh | New shim delegating to the Nix-store yek wrapper. |
| home-manager/modules/yek/install-yek-shim.sh | New shim delegating to the Nix-store install-yek script. |
| home-manager/modules/yek/default.nix | Refactors inline shim scripts into external files with replaceVars. |
| devenv.nix | Adds statix and refactors fishtape wrapper to be loaded from an external file. |
| Makefile | Adds python and nix-inline-check targets and wires them into make test. |
| .github/workflows/python.yml | Adds CI for Python tests + ruff lint/format checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| DOCKER_SERVICE_FILE=@docker_service_file@ | ||
|
|
||
| # Check if docker group exists and user is in it | ||
| if ! "$GROUPS_CMD" | "$GREP" -q docker; then |
| test: neovim-test nix-test shell-test python-test nix-inline-check ## Run all tests (neovim + nix + shell + python + nix-inline-check). | ||
|
|
| packages = [ | ||
| pkgs.nodejs | ||
| pkgs.bun | ||
| pkgs.neovim | ||
| pkgs.shellcheck | ||
| pkgs.shellspec | ||
| pkgs.gnumake | ||
| pkgs.gcc | ||
| pkgs.fish | ||
| (pkgs.writeShellScriptBin "fishtape" '' | ||
| exec ${pkgs.fish}/bin/fish \ | ||
| -C "source ${pkgs.fishPlugins.fishtape_3.src}/functions/fishtape.fish" \ | ||
| -c 'fishtape $argv' \ | ||
| -- "$@" | ||
| '') | ||
| pkgs.statix | ||
| (pkgs.writeShellScriptBin "fishtape" ( | ||
| builtins.readFile ( | ||
| pkgs.replaceVars ./scripts/fishtape-wrapper.sh { | ||
| fish = pkgs.fish; | ||
| fishtape_3_src = pkgs.fishPlugins.fishtape_3.src; | ||
| } | ||
| ) | ||
| )) | ||
| ]; |
| pw = sys.stdin.read().rstrip("\n") | ||
| result = unlock(pw) | ||
| codes = {0: "OK", 1: "DENIED", 2: "FAILED", 3: "NO_DAEMON"} | ||
| print(f"gnome-keyring unlock: {codes.get(result, result)}", flush=True) | ||
| sys.exit(0 if result == 0 else 1) |
|
|
||
| violations=$(grep -rn \ | ||
| --include='*.nix' \ | ||
| -E 'write(Shell)?(Script|ScriptBin)[[:space:]]+"[^"]+"+[[:space:]]+'"''" \ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 38 files
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="spec/check_nix_inline_scripts_spec.sh">
<violation number="1" location="spec/check_nix_inline_scripts_spec.sh:21">
P2: This assertion is too broad to validate the detector: it also matches the script's comment, so the test can pass even if the real `write*Script*` grep pattern breaks.</violation>
</file>
<file name="spec/yek_shim_spec.sh">
<violation number="1" location="spec/yek_shim_spec.sh:26">
P2: This assertion is too broad: it matches the placeholder in the shim's comment, so the test can pass even if the `exec` line no longer uses `@bash@`/`@yek_wrapper_script@`. Match the actual `exec` command instead of any occurrence in the file.</violation>
</file>
<file name="named-hosts/matic/falcon.nix">
<violation number="1" location="named-hosts/matic/falcon.nix:16">
P2: Avoid `builtins.readFile` on `pkgs.replaceVars` here; it turns this script extraction into import-from-derivation during evaluation, which adds avoidable eval-time realisation and can fail where IFD is disabled.</violation>
</file>
<file name="spec/start_postgres_wrapper_spec.sh">
<violation number="1" location="spec/start_postgres_wrapper_spec.sh:38">
P2: This test only greps for implementation strings, so it won't fail when the wrapper's docker-access fallback logic is broken. Exercise the script with mocked `docker`/`sg` commands instead of asserting on source text.</violation>
</file>
<file name="spec/pam_gnome_keyring_tpm_unlock_spec.sh">
<violation number="1" location="spec/pam_gnome_keyring_tpm_unlock_spec.sh:74">
P2: Use an exact count assertion here; `include '1'` also passes for counts like `10` and `11`.</violation>
</file>
<file name="spec/docker_setup_spec.sh">
<violation number="1" location="spec/docker_setup_spec.sh:53">
P2: These grep-based checks are too weak to validate the Docker setup behavior; they only prove that certain words exist in the script, so regressions can slip through while the tests still pass.</violation>
</file>
<file name="scripts/check-nix-inline-scripts.sh">
<violation number="1" location="scripts/check-nix-inline-scripts.sh:14">
P2: The grep only detects inline scripts when the name argument is a quoted literal, so inline `write*Script*` calls with interpolated or variable names will slip through.</violation>
</file>
<file name="home-manager/services/docker-postgres/start-postgres-wrapper.sh">
<violation number="1" location="home-manager/services/docker-postgres/start-postgres-wrapper.sh:9">
P2: `docker info` is treating "daemon still starting" as "permission denied", so the wrapper can exit early instead of letting `start-postgres.sh` wait for Docker to come up.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| Describe 'detection pattern' | ||
| It 'searches for writeScript patterns' | ||
| When run bash -c "grep 'write.*Script' '$SCRIPT'" |
There was a problem hiding this comment.
P2: This assertion is too broad to validate the detector: it also matches the script's comment, so the test can pass even if the real write*Script* grep pattern breaks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/check_nix_inline_scripts_spec.sh, line 21:
<comment>This assertion is too broad to validate the detector: it also matches the script's comment, so the test can pass even if the real `write*Script*` grep pattern breaks.</comment>
<file context>
@@ -0,0 +1,36 @@
+
+Describe 'detection pattern'
+It 'searches for writeScript patterns'
+When run bash -c "grep 'write.*Script' '$SCRIPT'"
+The output should include 'write'
+End
</file context>
| End | ||
|
|
||
| It 'references @bash@' | ||
| When run bash -c "grep '@bash@' '$SCRIPT'" |
There was a problem hiding this comment.
P2: This assertion is too broad: it matches the placeholder in the shim's comment, so the test can pass even if the exec line no longer uses @bash@/@yek_wrapper_script@. Match the actual exec command instead of any occurrence in the file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/yek_shim_spec.sh, line 26:
<comment>This assertion is too broad: it matches the placeholder in the shim's comment, so the test can pass even if the `exec` line no longer uses `@bash@`/`@yek_wrapper_script@`. Match the actual `exec` command instead of any occurrence in the file.</comment>
<file context>
@@ -0,0 +1,41 @@
+End
+
+It 'references @bash@'
+When run bash -c "grep '@bash@' '$SCRIPT'"
+The output should include '@bash@'
+End
</file context>
| # sanity print | ||
| ${falcon}/bin/fs-bash -c "/opt/CrowdStrike/falconctl -g --cid" | ||
| ''; | ||
| initScript = pkgs.writeScript "init-falcon" ( |
There was a problem hiding this comment.
P2: Avoid builtins.readFile on pkgs.replaceVars here; it turns this script extraction into import-from-derivation during evaluation, which adds avoidable eval-time realisation and can fail where IFD is disabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At named-hosts/matic/falcon.nix, line 16:
<comment>Avoid `builtins.readFile` on `pkgs.replaceVars` here; it turns this script extraction into import-from-derivation during evaluation, which adds avoidable eval-time realisation and can fail where IFD is disabled.</comment>
<file context>
@@ -13,37 +13,15 @@
- # sanity print
- ${falcon}/bin/fs-bash -c "/opt/CrowdStrike/falconctl -g --cid"
- '';
+ initScript = pkgs.writeScript "init-falcon" (
+ builtins.readFile (
+ pkgs.replaceVars ./falcon-init.sh {
</file context>
|
|
||
| Describe 'docker access strategy' | ||
| It 'tries docker directly first' | ||
| When run bash -c "grep 'docker info' '$SCRIPT'" |
There was a problem hiding this comment.
P2: This test only greps for implementation strings, so it won't fail when the wrapper's docker-access fallback logic is broken. Exercise the script with mocked docker/sg commands instead of asserting on source text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/start_postgres_wrapper_spec.sh, line 38:
<comment>This test only greps for implementation strings, so it won't fail when the wrapper's docker-access fallback logic is broken. Exercise the script with mocked `docker`/`sg` commands instead of asserting on source text.</comment>
<file context>
@@ -0,0 +1,58 @@
+
+Describe 'docker access strategy'
+It 'tries docker directly first'
+When run bash -c "grep 'docker info' '$SCRIPT'"
+The output should include 'docker info'
+End
</file context>
|
|
||
| It 'runs unlock in background subshell' | ||
| When run bash -c "grep -c ') &' '$SCRIPT'" | ||
| The output should include '1' |
There was a problem hiding this comment.
P2: Use an exact count assertion here; include '1' also passes for counts like 10 and 11.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/pam_gnome_keyring_tpm_unlock_spec.sh, line 74:
<comment>Use an exact count assertion here; `include '1'` also passes for counts like `10` and `11`.</comment>
<file context>
@@ -0,0 +1,83 @@
+
+It 'runs unlock in background subshell'
+When run bash -c "grep -c ') &' '$SCRIPT'"
+The output should include '1'
+End
+
</file context>
| @@ -0,0 +1,80 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
P2: These grep-based checks are too weak to validate the Docker setup behavior; they only prove that certain words exist in the script, so regressions can slip through while the tests still pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/docker_setup_spec.sh, line 53:
<comment>These grep-based checks are too weak to validate the Docker setup behavior; they only prove that certain words exist in the script, so regressions can slip through while the tests still pass.</comment>
<file context>
@@ -0,0 +1,80 @@
+
+Describe 'docker group management'
+It 'checks group membership'
+When run bash -c "grep 'docker group' '$SCRIPT'"
+The output should include 'docker group'
+End
</file context>
|
|
||
| violations=$(grep -rn \ | ||
| --include='*.nix' \ | ||
| -E 'write(Shell)?(Script|ScriptBin)[[:space:]]+"[^"]+"+[[:space:]]+'"''" \ |
There was a problem hiding this comment.
P2: The grep only detects inline scripts when the name argument is a quoted literal, so inline write*Script* calls with interpolated or variable names will slip through.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-nix-inline-scripts.sh, line 14:
<comment>The grep only detects inline scripts when the name argument is a quoted literal, so inline `write*Script*` calls with interpolated or variable names will slip through.</comment>
<file context>
@@ -0,0 +1,30 @@
+
+violations=$(grep -rn \
+ --include='*.nix' \
+ -E 'write(Shell)?(Script|ScriptBin)[[:space:]]+"[^"]+"+[[:space:]]+'"''" \
+ "$ROOT" \
+ --exclude-dir='.git' \
</file context>
| SCRIPT="@bash@/bin/bash @start_script@" | ||
|
|
||
| # Try docker directly first (works on NixOS or when user has docker group) | ||
| if @docker@/bin/docker info >/dev/null 2>&1; then |
There was a problem hiding this comment.
P2: docker info is treating "daemon still starting" as "permission denied", so the wrapper can exit early instead of letting start-postgres.sh wait for Docker to come up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/docker-postgres/start-postgres-wrapper.sh, line 9:
<comment>`docker info` is treating "daemon still starting" as "permission denied", so the wrapper can exit early instead of letting `start-postgres.sh` wait for Docker to come up.</comment>
<file context>
@@ -0,0 +1,21 @@
+SCRIPT="@bash@/bin/bash @start_script@"
+
+# Try docker directly first (works on NixOS or when user has docker group)
+if @docker@/bin/docker info >/dev/null 2>&1; then
+ exec $SCRIPT
+fi
</file context>
![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
🧹 Nitpick comments (3)
home-manager/services/docker-postgres/start-postgres-wrapper.sh (1)
1-21: Consider extracting the shared Docker fallback logic.This script is nearly identical to
home-manager/services/cliproxyapi/scripts/docker-start.sh. Both implement the same Docker access fallback pattern (direct → NixOS sg → system sg → error).Consider extracting this common logic into a shared wrapper template that both services can use, parameterized only by
@start_script@. This would reduce maintenance burden and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/services/docker-postgres/start-postgres-wrapper.sh` around lines 1 - 21, The Docker-access fallback logic is duplicated (see SCRIPT, the `@start_script`@ substitution and the direct → NixOS sg → system sg → error flow) and should be extracted into a shared wrapper template used by both start-postgres-wrapper.sh and docker-start.sh; create a single reusable script/template that accepts the start script path (parameterize `@start_script`@) and performs the same checks (try `@docker@/bin/docker info`, then `/run/wrappers/bin/sg docker -c`, then `/usr/bin/sg docker -c`, else error) and update both callers to invoke that shared wrapper instead of duplicating the logic.scripts/check-nix-inline-scripts.sh (1)
12-20: Regex may miss multi-line patterns.The detection regex requires
''to be on the same line as the function call. Patterns like:writeShellScript "name" '' content ''would not be detected. This is likely acceptable as a best-effort check, but consider documenting this limitation or using a multi-line grep approach with
-zflag if stricter enforcement is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-nix-inline-scripts.sh` around lines 12 - 20, The current grep regex in check-nix-inline-scripts.sh (searching for write(Shell)?(Script|ScriptBin)...) only matches when the Nix literal delimiter '' appears on the same line and will miss multi-line usages; either document this limitation in the script header/README or switch to a multi-line search using grep -z and a PCRE pattern (e.g., use grep -z -P to allow matching across newlines and a pattern like write(Shell)?(Script|ScriptBin)\s*"[^"]+"\s*''.*?''), updating the violations variable construction and tests accordingly so multi-line writeShellScript/writeScript/writeScriptBin invocations are detected.spec/pam_gnome_keyring_tpm_unlock_spec.sh (1)
13-16: Minor inconsistency in placeholder replacement.This spec uses
trueas the placeholder replacement value (Line 14), while other specs likespec/docker_setup_spec.shuse/usr. Both work for syntax validation, but using/usrconsistently across all specs would improve maintainability.Not a blocking issue since
trueis a valid command/path for syntax checking purposes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/pam_gnome_keyring_tpm_unlock_spec.sh` around lines 13 - 16, Update the placeholder replacement in the spec that tests syntax after stripping placeholders: in the example under "It 'passes bash syntax check after stripping placeholders'" change the sed replacement value from "true" to "/usr" so the command using bash -c "sed 's|@[a-z_]*@|true|g' '$SCRIPT' | bash -n" becomes consistent with other specs (use '/usr' as the replacement for @[a-z_]*@ applied to $SCRIPT).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@home-manager/programs/tmux/default.nix`:
- Around line 11-21: The tmux plugin load order is incorrect: move
tmux-resurrect to be listed before tmux-continuum and place tmux-continuum at or
near the end of the plugins array so resurrect is available when continuum
initializes and continuum's status-bar modifications are not overwritten; update
the plugins list (the array assigned to plugins using pkgs.tmuxPlugins that
currently contains continuum, resurrect, etc.) to ensure resurrect appears
before continuum and continuum is last (or near-last).
In `@home-manager/services/docker/setup-docker.sh`:
- Around line 16-20: The group membership check can match substrings (e.g.,
"dockerroot"); update the conditional that uses GROUPS_CMD and GREP so it
matches the exact group name "docker" (for example, pipe GROUPS_CMD output
through tr ' ' '\n' and use GREP -x -q docker, or use GREP -w/-x with an
anchored pattern) so the block that calls USERMOD -aG docker on USER only runs
when the exact "docker" group is absent; update the line that currently reads:
if ! "$GROUPS_CMD" | "$GREP" -q docker; then accordingly.
- Around line 25-31: The current check only tests for the presence of
/etc/systemd/system/docker.service and may overwrite or shadow a vendor-supplied
unit; update setup-docker.sh to query systemd for the actual installed unit
(e.g., use systemctl show -p FragmentPath docker.service) and only write
/etc/systemd/system/docker.service (using TEE and DOCKER_SERVICE_FILE, then
SYSTEMCTL daemon-reload/enable) when there is no existing fragment path or when
FragmentPath is empty/indicates no installed unit; if FragmentPath points to a
vendor/path, skip creating the override so you don't shadow the packaged unit.
---
Nitpick comments:
In `@home-manager/services/docker-postgres/start-postgres-wrapper.sh`:
- Around line 1-21: The Docker-access fallback logic is duplicated (see SCRIPT,
the `@start_script`@ substitution and the direct → NixOS sg → system sg → error
flow) and should be extracted into a shared wrapper template used by both
start-postgres-wrapper.sh and docker-start.sh; create a single reusable
script/template that accepts the start script path (parameterize `@start_script`@)
and performs the same checks (try `@docker@/bin/docker info`, then
`/run/wrappers/bin/sg docker -c`, then `/usr/bin/sg docker -c`, else error) and
update both callers to invoke that shared wrapper instead of duplicating the
logic.
In `@scripts/check-nix-inline-scripts.sh`:
- Around line 12-20: The current grep regex in check-nix-inline-scripts.sh
(searching for write(Shell)?(Script|ScriptBin)...) only matches when the Nix
literal delimiter '' appears on the same line and will miss multi-line usages;
either document this limitation in the script header/README or switch to a
multi-line search using grep -z and a PCRE pattern (e.g., use grep -z -P to
allow matching across newlines and a pattern like
write(Shell)?(Script|ScriptBin)\s*"[^"]+"\s*''.*?''), updating the violations
variable construction and tests accordingly so multi-line
writeShellScript/writeScript/writeScriptBin invocations are detected.
In `@spec/pam_gnome_keyring_tpm_unlock_spec.sh`:
- Around line 13-16: Update the placeholder replacement in the spec that tests
syntax after stripping placeholders: in the example under "It 'passes bash
syntax check after stripping placeholders'" change the sed replacement value
from "true" to "/usr" so the command using bash -c "sed 's|@[a-z_]*@|true|g'
'$SCRIPT' | bash -n" becomes consistent with other specs (use '/usr' as the
replacement for @[a-z_]*@ applied to $SCRIPT).
🪄 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: f244a4a7-cbe4-4efc-a355-f8647eb8924e
📒 Files selected for processing (38)
.github/workflows/python.ymlMakefiledevenv.nixhome-manager/modules/yek/default.nixhome-manager/modules/yek/install-yek-shim.shhome-manager/modules/yek/yek-shim.shhome-manager/programs/php/default.nixhome-manager/programs/tmux/default.nixhome-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/docker-start.shhome-manager/services/docker-postgres/default.nixhome-manager/services/docker-postgres/start-postgres-wrapper.shhome-manager/services/docker/default.nixhome-manager/services/docker/docker-setup.shhome-manager/services/docker/docker.servicehome-manager/services/docker/setup-docker.shnamed-hosts/matic/default.nixnamed-hosts/matic/falcon-init.shnamed-hosts/matic/falcon.nixnamed-hosts/matic/falcon/default.nixnamed-hosts/matic/pam-gnome-keyring-tpm-unlock.shnamed-hosts/matic/unlock-gnome-keyring.pypyproject.tomlscripts/check-nix-inline-scripts.shscripts/fishtape-wrapper.shspec/check_nix_inline_scripts_spec.shspec/cliproxyapi_docker_start_spec.shspec/coverage_spec.shspec/docker_setup_spec.shspec/docker_setup_wrapper_spec.shspec/falcon_init_spec.shspec/fishtape_wrapper_spec.shspec/install_yek_shim_spec.shspec/pam_gnome_keyring_tpm_unlock_spec.shspec/start_postgres_wrapper_spec.shspec/unlock_gnome_keyring_spec.shspec/yek_shim_spec.shtests/test_unlock_gnome_keyring.py
| plugins = with pkgs.tmuxPlugins; [ | ||
| continuum | ||
| extrakto | ||
| open | ||
| resurrect | ||
| sensible | ||
| yank | ||
| tmux-fzf | ||
| resurrect | ||
| continuum | ||
| tmux-sessionx | ||
| tmux-thumbs | ||
| open | ||
| extrakto | ||
| yank | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check tmux plugin dependencies and home-manager implementation
# Check home-manager's tmux module implementation for plugin ordering behavior
rg -n -C5 'tmux.*plugins' --type=nix --glob='**/modules/programs/tmux*'
# Check if continuum has explicit resurrect dependency
fd -e nix -e txt -e md 'continuum|resurrect' --exec rg -l 'continuum.*resurrect|resurrect.*continuum' {}
# Look for any plugin ordering documentation or comments
rg -n 'plugin.*order|load.*order|before.*after' --type=nix --glob='**/tmux*'Repository: shunkakinoki/dotfiles
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Check repository structure and locate the tmux configuration file
echo "=== Checking for tmux-related files ==="
find . -name "*tmux*" -type f | head -20
echo -e "\n=== Checking if home-manager directory exists ==="
ls -la home-manager/ 2>/dev/null || echo "home-manager not at root"
echo -e "\n=== Finding tmux configuration files ==="
fd -e nix . | rg -i tmuxRepository: shunkakinoki/dotfiles
Length of output: 762
🏁 Script executed:
#!/bin/bash
# Read the actual tmux configuration file mentioned in the review
cat -n home-manager/programs/tmux/default.nix 2>/dev/null || echo "File not found at specified path"Repository: shunkakinoki/dotfiles
Length of output: 632
🌐 Web query:
home-manager tmux plugin loading order dependencies
💡 Result:
In Home Manager's programs.tmux module, the generated ~/.config/tmux/tmux.conf has a specific structure for plugin loading and dependencies. Structure (order in tmux.conf): 1. Module defaults (e.g., set -g escape-time 500, default-terminal "screen", etc.) using lib.mkBefore. 2. Plugins section: For each plugin in programs.tmux.plugins (in list order): - Optional plugin.extraConfig (must be before run-shell for plugin vars to be available). - run-shell ${plugin.rtp} (loads the plugin script). 3. Global extraConfig using lib.mkAfter (after plugins). Plugin loading order is the order in the plugins list. Order matters for dependencies: plugins that modify shared elements like status-right must come before dependent plugins (e.g., theme before resurrect/continuum). Use plugin.extraConfig for per-plugin config before its run-shell. To place config before all plugins, users can override with xdg.configFile."tmux/tmux.conf".text = lib.mkOrder 600 "..."; (between defaults priority 500 and plugins 1000). extraConfig after plugins can reference plugin variables but cannot affect pre-plugin-run settings. No built-in extraConfigBeforePlugins (proposed PRs #3801, #4670, #6486 open/stale as of 2025). Workarounds for dependencies: - Put theme/config in plugin.extraConfig. - Order plugins carefully (e.g., sensible on top via sensibleOnTop=true). - Custom tmux.conf override for full control. Examples: plugins = with pkgs.tmuxPlugins; [ { plugin = catppuccin; extraConfig = "set -g @catppuccin_flavour 'frappe'"; } resurrect # depends on prior status-right setup ];
Citations:
- 1: bug: order of
pluginsandextraConfigsections in generatedtmux.confnix-community/home-manager#3555 - 2: bug: tmux power theme cannot be applied in tmux nix-community/home-manager#4894
- 3: https://haseebmajid.dev/posts/2023-07-10-setting-up-tmux-with-nix-home-manager
- 4: https://github.com/p3t33/nixos_flake/blob/master/modules/home-manager/tmux.nix
- 5: https://github.com/nix-community/home-manager/blob/master/modules/programs/tmux.nix
- 6: tmux: Add
extraConfigBeforePluginstotmux.confnix-community/home-manager#4670 - 7: draft: tmux: add configBeforePlugin option nix-community/home-manager#3801
🏁 Script executed:
#!/bin/bash
# Check NixPkgs definition of tmux plugins to understand dependencies
echo "=== Searching for continuum and resurrect plugin definitions ==="
rg -A10 -B2 "continuum|resurrect" --type nix | head -100
echo -e "\n=== Check for any plugin dependency information ==="
rg "depends|require" --type nix | rg -i "continuum|resurrect"Repository: shunkakinoki/dotfiles
Length of output: 967
🌐 Web query:
tmux-continuum depends on tmux-resurrect plugin loading order
💡 Result:
Yes, tmux-continuum depends on tmux-resurrect and requires specific attention to plugin loading order when using Tmux Plugin Manager (TPM). tmux-continuum is built on top of tmux-resurrect: it automates saving and restoring tmux environments by calling resurrect's functionality. The official tmux-continuum README explicitly states "Please make sure you have tmux-resurrect installed" before adding continuum, and all installation examples list resurrect first: set -g @plugin 'tmux-plugins/tmux-resurrect' set -g @plugin 'tmux-plugins/tmux-continuum' While TPM loads plugins in the order listed in .tmux.conf, continuum modifies the status-right (or status-left) to insert its periodic save hook (continuum_save.sh). If another plugin loaded after continuum overwrites status-right (common with themes), continuum's autosave stops working. The official docs recommend: "To fix this issue, place the plugin last in the TPM plugins list." Resurrect has no such status modifications, so the standard order (resurrect before continuum, continuum last or near-last) ensures: - Continuum can call resurrect functions (loaded earlier). - No later plugins break continuum's status hook. User reports confirm issues when continuum is not positioned correctly relative to status-altering plugins, but no explicit "continuum requires resurrect before it" beyond dependency and examples. Continuum restore calls also assume resurrect is available at runtime. Example working .tmux.conf snippet: set -g @plugin 'tmux-plugins/tpm' set -g @plugin 'tmux-plugins/tmux-sensible' # or other non-status plugins set -g @plugin 'tmux-plugins/tmux-resurrect' set -g @plugin 'tmux-plugins/tmux-continuum' # last, before run tpm set -g @continuum-restore 'on' run '~/.tmux/plugins/tpm/tpm' Press prefix+I to install.
Citations:
- 1: https://github.com/tmux-plugins/tmux-continuum
- 2: https://github.com/tmux-plugins/tmux-resurrect
- 3: https://www.reddit.com/r/NixOS/comments/1f6s0li/anyone_else_have_issues_with_tmuxcontinuum_not/
- 4: tmuxPlugins.continuum NixOS/nixpkgs#352290
- 5: Continuum Does Not Save Session After Interval Time tmux-plugins/tmux-continuum#97
🌐 Web query:
tmux continuum plugin how it works resurrect dependency
💡 Result:
The tmux-continuum plugin provides continuous saving of the tmux environment every 15 minutes (configurable) in the background, automatic restoration of the last saved environment upon tmux server start (if enabled with set -g @continuum-restore 'on'), and optional automatic tmux startup after computer boot (if enabled with set -g @continuum-boot 'on'). It requires the tmux-resurrect plugin as a dependency, which handles the actual saving and restoring of tmux sessions, windows, panes, and running programs (manual save/restore via prefix + Ctrl-s / Ctrl-r). Continuum automates resurrect's functionality for seamless persistence across restarts. Installation via Tmux Plugin Manager: add set -g @plugin 'tmux-plugins/tmux-resurrect' and set -g @plugin 'tmux-plugins/tmux-continuum' to .tmux.conf, then prefix + I. Requires tmux 1.9+ and bash. Last updated 2024.
Citations:
- 1: https://github.com/tmux-plugins/tmux-continuum
- 2: https://github.com/tmux-plugins/tmux-resurrect
- 3: https://arcolinux.com/everything-you-need-to-know-about-tmux-reconstructing-tmux-sessions-after-restarts/
- 4: https://cms.typecraft.dev/community/tmux-continuum/
Plugin load order breaks tmux-continuum functionality—resurrect must come before continuum, and continuum should be positioned near the end of the list.
The alphabetical reordering introduces a critical issue. The tmux-continuum plugin depends on tmux-resurrect and requires it to be loaded first. According to the official tmux-continuum documentation, continuum automates resurrect's save/restore functionality and will not work correctly if resurrect is not available at runtime. Additionally, continuum modifies the tmux status bar to insert its autosave hook; if status-altering plugins (like themes) load after continuum, they can overwrite this hook, breaking the automatic save feature.
The current order places continuum (line 12) before resurrect (line 15), and continuum is not positioned near the end of the list. This will break tmux session persistence. The correct ordering should be: resurrect before continuum, and continuum positioned last (or near-last) to prevent other plugins from overwriting its status modifications.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/programs/tmux/default.nix` around lines 11 - 21, The tmux plugin
load order is incorrect: move tmux-resurrect to be listed before tmux-continuum
and place tmux-continuum at or near the end of the plugins array so resurrect is
available when continuum initializes and continuum's status-bar modifications
are not overwritten; update the plugins list (the array assigned to plugins
using pkgs.tmuxPlugins that currently contains continuum, resurrect, etc.) to
ensure resurrect appears before continuum and continuum is last (or near-last).
| if ! "$GROUPS_CMD" | "$GREP" -q docker; then | ||
| echo "Adding user to docker group..." | ||
| sudo "$USERMOD" -aG docker "$USER" | ||
| echo "Added to docker group. Please log out and back in, or run: newgrp docker" | ||
| fi |
There was a problem hiding this comment.
Use exact group-name matching for membership checks.
Line 16 uses grep -q docker, which can match partial names (e.g., dockerroot). This can skip required group enrollment.
Proposed fix
-if ! "$GROUPS_CMD" | "$GREP" -q docker; then
+if ! "$GROUPS_CMD" | "$GREP" -qw 'docker'; then📝 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.
| if ! "$GROUPS_CMD" | "$GREP" -q docker; then | |
| echo "Adding user to docker group..." | |
| sudo "$USERMOD" -aG docker "$USER" | |
| echo "Added to docker group. Please log out and back in, or run: newgrp docker" | |
| fi | |
| if ! "$GROUPS_CMD" | "$GREP" -qw 'docker'; then | |
| echo "Adding user to docker group..." | |
| sudo "$USERMOD" -aG docker "$USER" | |
| echo "Added to docker group. Please log out and back in, or run: newgrp docker" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/services/docker/setup-docker.sh` around lines 16 - 20, The group
membership check can match substrings (e.g., "dockerroot"); update the
conditional that uses GROUPS_CMD and GREP so it matches the exact group name
"docker" (for example, pipe GROUPS_CMD output through tr ' ' '\n' and use GREP
-x -q docker, or use GREP -w/-x with an anchored pattern) so the block that
calls USERMOD -aG docker on USER only runs when the exact "docker" group is
absent; update the line that currently reads: if ! "$GROUPS_CMD" | "$GREP" -q
docker; then accordingly.
| if [ ! -f /etc/systemd/system/docker.service ]; then | ||
| echo "Installing Docker systemd service..." | ||
| # shellcheck disable=SC2024 | ||
| sudo "$TEE" /etc/systemd/system/docker.service >/dev/null <"$DOCKER_SERVICE_FILE" | ||
| sudo "$SYSTEMCTL" daemon-reload | ||
| sudo "$SYSTEMCTL" enable docker | ||
| fi |
There was a problem hiding this comment.
Service-unit detection is too narrow and can incorrectly install an override.
Lines 25-31 only check /etc/systemd/system/docker.service. If Docker is already provided in another systemd unit path (e.g., vendor unit), this still installs a new /etc unit and may unintentionally shadow the packaged one.
Proposed fix
- if [ ! -f /etc/systemd/system/docker.service ]; then
+ if ! "$SYSTEMCTL" cat docker >/dev/null 2>&1; then
echo "Installing Docker systemd service..."
# shellcheck disable=SC2024
sudo "$TEE" /etc/systemd/system/docker.service >/dev/null <"$DOCKER_SERVICE_FILE"
sudo "$SYSTEMCTL" daemon-reload
sudo "$SYSTEMCTL" enable docker
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/services/docker/setup-docker.sh` around lines 25 - 31, The
current check only tests for the presence of /etc/systemd/system/docker.service
and may overwrite or shadow a vendor-supplied unit; update setup-docker.sh to
query systemd for the actual installed unit (e.g., use systemctl show -p
FragmentPath docker.service) and only write /etc/systemd/system/docker.service
(using TEE and DOCKER_SERVICE_FILE, then SYSTEMCTL daemon-reload/enable) when
there is no existing fragment path or when FragmentPath is empty/indicates no
installed unit; if FragmentPath points to a vendor/path, skip creating the
override so you don't shadow the packaged unit.
Sort package/plugin lists in tmux plugins, nix-ld libraries, falcon buildInputs, and php packages for consistency.
0976ab1 to
2c3d646
Compare
Summary
home-manager/programs/tmux/default.nixnix-ld.librariesalphabetically innamed-hosts/matic/default.nixbuildInputsalphabetically innamed-hosts/matic/falcon/default.nixhome.packagesalphabetically inhome-manager/programs/php/default.nixTest plan
nix flake checkpasses🤖 Generated with Claude Code
Summary by cubic
Sorted package and plugin lists alphabetically across Nix modules for consistency and easier diffs. No functional changes.
Sorted
tmuxplugins,nix-ld.libraries, FalconbuildInputs, and PHPhome.packages.Written for commit 2c3d646. Summary will update on new commits.