refactor: extract inline Nix scripts to external files - #1175
Merged
Conversation
…nd linting Move all inline shell/Python scripts from write*Script* calls in .nix files to sibling .sh/.py files referenced via builtins.readFile + pkgs.replaceVars. This makes scripts testable with ShellSpec/pytest, lintable with ShellCheck, and enforced by a new nix-inline-check linter. - Extract 11 inline scripts across 5 Nix modules to external files - Add 10 ShellSpec test files (82 examples) for extracted shell scripts - Add 8 pytest unit tests for the gnome-keyring unlock Python script - Add scripts/check-nix-inline-scripts.sh linter (fails if inline scripts found) - Add .github/workflows/python.yml CI (pytest + ruff via uv) - Add Makefile targets: python-test, python-lint, nix-inline-check - Add statix to devenv.nix for Nix linting - Update pyproject.toml with pytest/ruff config and restructure tool tracking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughWalkthroughRefactors many inlined shell/Python scripts in Nix configs into external template files with pkgs.replaceVars/builtins.readFile, adds Python tests and Ruff config, introduces CI workflows and Makefile targets for Python, and adds numerous ShellSpec tests plus an inline-script checker. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant PAM as PAM
participant PAM_Unlock as pam-gnome-keyring-tpm-unlock.sh
participant SystemdCreds as systemd-creds
participant Runuser as runuser
participant Keyring as Keyring Socket
participant UnlockPy as unlock-gnome-keyring.py
User->>PAM: Login
PAM->>PAM_Unlock: execute unlock script
PAM_Unlock->>SystemdCreds: decrypt TPM credential
SystemdCreds-->>PAM_Unlock: password
PAM_Unlock->>PAM: exit 0 (non-blocking)
PAM_Unlock->>PAM_Unlock: fork background retry loop
loop up to 8 attempts
PAM_Unlock->>Keyring: check /run/user/$UID/keyring/control
alt socket exists
PAM_Unlock->>Runuser: exec unlock as user
Runuser->>UnlockPy: invoke with password
UnlockPy->>Keyring: connect AF_UNIX and send unlock packet
Keyring-->>UnlockPy: return result code
UnlockPy-->>Runuser: exit status
else socket missing
PAM_Unlock->>PAM_Unlock: sleep 3s
end
end
sequenceDiagram
participant CLI as Caller
participant DockerStart as docker-start.sh
participant Docker as Docker daemon
participant SgNix as /run/wrappers/bin/sg
participant SgSys as /usr/bin/sg
CLI->>DockerStart: invoke start script
DockerStart->>Docker: attempt docker access (docker info)
alt docker accessible
Docker-->>DockerStart: success
DockerStart-->>CLI: run start script
else docker inaccessible
DockerStart->>SgNix: try NixOS sg wrapper
alt SgNix exists
SgNix->>Docker: run via group switch
else
DockerStart->>SgSys: try system sg
alt SgSys exists
SgSys->>Docker: run via group switch
else
DockerStart->>CLI: print error and exit 1
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
Mesa DescriptionTL;DRExtracted all 11 inline shell/Python scripts from What changed?
Description generated by Mesa. Update settings |
Contributor
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 the codebase by externalizing inline shell and Python scripts previously embedded within Nix expressions. This change enhances the modularity, readability, and testability of the system's configuration and helper scripts. It also introduces new linting and testing mechanisms for both Nix and Python code, ensuring higher code quality and preventing the reintroduction of inline scripts. The overall impact is a more robust and maintainable development environment. 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
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 34 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=".github/workflows/python.yml">
<violation number="1" location=".github/workflows/python.yml:48">
P2: The Ruff formatting check is configured to ignore failures, so CI can pass even when formatting violations are detected.</violation>
</file>
<file name="tests/test_unlock_gnome_keyring.py">
<violation number="1" location="tests/test_unlock_gnome_keyring.py:111">
P2: `assertRaises(..., msg=...)` does not check exception text. Use `assertRaisesRegex` (or inspect `cm.exception`) so these tests actually verify the expected error message.</violation>
</file>
<file name="named-hosts/matic/unlock-gnome-keyring.py">
<violation number="1" location="named-hosts/matic/unlock-gnome-keyring.py:29">
P1: Set a socket timeout before connect/recv to avoid indefinite hangs when the keyring daemon is unresponsive.</violation>
</file>
<file name="home-manager/services/docker/setup-docker.sh">
<violation number="1" location="home-manager/services/docker/setup-docker.sh:16">
P2: Use an exact word match when checking group membership; substring matching can incorrectly skip adding the user to the `docker` group.</violation>
<violation number="2" location="home-manager/services/docker/setup-docker.sh:25">
P1: Checking only `/etc/systemd/system/docker.service` is not a reliable way to detect whether the Docker unit exists in systemd.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:900">
P2: `ruff format --diff` alone does not fail when files need reformatting, so this lint target can pass with formatting violations.</violation>
</file>
<file name="scripts/check-nix-inline-scripts.sh">
<violation number="1" location="scripts/check-nix-inline-scripts.sh:14">
P2: The inline-script detector is too narrow and can miss violations (multi-line calls or non-literal first arguments), leading to false negatives in CI.</violation>
</file>
<file name="named-hosts/matic/falcon-init.sh">
<violation number="1" location="named-hosts/matic/falcon-init.sh:26">
P1: Sourcing `/etc/falcon-sensor.env` directly executes arbitrary shell as root; validate file ownership/permissions before sourcing (or parse the variable without `.`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Check if system docker service exists and is running | ||
| if ! "$SYSTEMCTL" is-active --quiet docker 2>/dev/null; then | ||
| echo "Starting Docker daemon..." | ||
| if [ ! -f /etc/systemd/system/docker.service ]; then |
Contributor
There was a problem hiding this comment.
P1: Checking only /etc/systemd/system/docker.service is not a reliable way to detect whether the Docker unit exists in systemd.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/docker/setup-docker.sh, line 25:
<comment>Checking only `/etc/systemd/system/docker.service` is not a reliable way to detect whether the Docker unit exists in systemd.</comment>
<file context>
@@ -0,0 +1,36 @@
+# Check if system docker service exists and is running
+if ! "$SYSTEMCTL" is-active --quiet docker 2>/dev/null; then
+ echo "Starting Docker daemon..."
+ if [ ! -f /etc/systemd/system/docker.service ]; then
+ echo "Installing Docker systemd service..."
+ # shellcheck disable=SC2024
</file context>
Suggested change
| if [ ! -f /etc/systemd/system/docker.service ]; then | |
| if ! "$SYSTEMCTL" cat docker >/dev/null 2>&1; then |
Owner
Author
There was a problem hiding this comment.
Not applicable — this is pre-existing code that was simply extracted from inline Nix. The docker service detection logic was not changed in this PR.
Contributor
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
|
|
||
| # load CID from /etc/falcon-sensor.env (root-only) | ||
| # shellcheck source=/dev/null | ||
| . /etc/falcon-sensor.env |
Contributor
There was a problem hiding this comment.
P1: Sourcing /etc/falcon-sensor.env directly executes arbitrary shell as root; validate file ownership/permissions before sourcing (or parse the variable without .).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At named-hosts/matic/falcon-init.sh, line 26:
<comment>Sourcing `/etc/falcon-sensor.env` directly executes arbitrary shell as root; validate file ownership/permissions before sourcing (or parse the variable without `.`).</comment>
<file context>
@@ -0,0 +1,32 @@
+
+# load CID from /etc/falcon-sensor.env (root-only)
+# shellcheck source=/dev/null
+. /etc/falcon-sensor.env
+
+# set CID via falconctl inside FHS env
</file context>
Owner
Author
There was a problem hiding this comment.
Not applicable — pre-existing behavior extracted from inline Nix. The file at /etc/falcon-sensor.env is already root-owned and the script runs as root via systemd. No change in security posture.
Contributor
There was a problem hiding this comment.
Thanks for the feedback! I've updated an existing learning with this new information.
| DOCKER_SERVICE_FILE=@docker_service_file@ | ||
|
|
||
| # Check if docker group exists and user is in it | ||
| if ! "$GROUPS_CMD" | "$GREP" -q docker; then |
Contributor
There was a problem hiding this comment.
P2: Use an exact word match when checking group membership; substring matching can incorrectly skip adding the user to the docker group.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/docker/setup-docker.sh, line 16:
<comment>Use an exact word match when checking group membership; substring matching can incorrectly skip adding the user to the `docker` group.</comment>
<file context>
@@ -0,0 +1,36 @@
+DOCKER_SERVICE_FILE=@docker_service_file@
+
+# Check if docker group exists and user is in it
+if ! "$GROUPS_CMD" | "$GREP" -q docker; then
+ echo "Adding user to docker group..."
+ sudo "$USERMOD" -aG docker "$USER"
</file context>
Suggested change
| if ! "$GROUPS_CMD" | "$GREP" -q docker; then | |
| if ! "$GROUPS_CMD" | "$GREP" -qw docker; then |
Owner
Author
There was a problem hiding this comment.
Not applicable — pre-existing code extracted from inline Nix. The groups command outputs space-separated group names so grep -q docker works correctly for the docker group name.
Contributor
There was a problem hiding this comment.
Got it—I’ll avoid flagging issues in pre-existing code that’s only been extracted without behavior changes (e.g., from inline Nix). Thanks for the clarification.
- Sort imports in unlock-gnome-keyring.py (I001) - Remove unused imports in test file (F401: importlib.util, socket, sys) - Break long line in test method signature (E501) - Apply ruff format to Python files - Apply treefmt/alejandra to all Nix files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the repo’s “externalize scripts” direction by moving inline write*Script* bodies out of Nix files into standalone .sh/.py files, and adds CI/linting to prevent regressions (plus Python testing/linting support).
Changes:
- Extracts multiple inline Nix
write*Script*bodies into external scripts referenced viabuiltins.readFile+pkgs.replaceVars. - Adds ShellSpec coverage for the new scripts and expands the coverage list.
- Adds Python test/lint infrastructure (pytest + ruff via uv) and a Nix inline-script linter + Makefile targets.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_unlock_gnome_keyring.py |
New unit tests for the gnome-keyring unlock script protocol logic. |
spec/yek_shim_spec.sh |
ShellSpec coverage for the yek shim wrapper script. |
spec/unlock_gnome_keyring_spec.sh |
ShellSpec checks for unlock script invariants (placeholders, protocol strings). |
spec/start_postgres_wrapper_spec.sh |
ShellSpec coverage for Postgres docker wrapper logic/strings. |
spec/pam_gnome_keyring_tpm_unlock_spec.sh |
ShellSpec coverage for PAM TPM unlock wrapper behavior/strings. |
spec/install_yek_shim_spec.sh |
ShellSpec coverage for the install-yek shim wrapper. |
spec/fishtape_wrapper_spec.sh |
ShellSpec coverage for the fishtape wrapper script. |
spec/falcon_init_spec.sh |
ShellSpec coverage for Falcon init script invariants/strings. |
spec/docker_setup_wrapper_spec.sh |
ShellSpec coverage for docker-setup wrapper. |
spec/docker_setup_spec.sh |
ShellSpec coverage for the docker setup script behavior/strings. |
spec/coverage_spec.sh |
Extends coverage assertions and the “coverage list” to include new scripts. |
spec/cliproxyapi_docker_start_spec.sh |
ShellSpec coverage for cliproxyapi docker-start wrapper. |
scripts/fishtape-wrapper.sh |
Extracted fishtape wrapper used by devenv.nix. |
scripts/check-nix-inline-scripts.sh |
New linter to reject inline write*Script* '' ... '' usage in .nix files. |
pyproject.toml |
Adds pytest + ruff configuration and a dependency-groups.tools section. |
named-hosts/matic/unlock-gnome-keyring.py |
New externalized Python implementation for gnome-keyring unlock protocol. |
named-hosts/matic/pam-gnome-keyring-tpm-unlock.sh |
New externalized PAM script wrapping TPM decrypt + background retry unlock. |
named-hosts/matic/falcon.nix |
Switches Falcon init script to readFile + replaceVars external script. |
named-hosts/matic/falcon-init.sh |
New externalized Falcon init shell script. |
named-hosts/matic/default.nix |
Switches inline PAM + Python unlock scripts to external files via readFile + replaceVars. |
home-manager/services/docker/setup-docker.sh |
New externalized docker setup script (group membership + daemon management). |
home-manager/services/docker/docker.service |
New externalized systemd unit template with placeholders. |
home-manager/services/docker/docker-setup.sh |
New wrapper script exposing setup-docker as a command. |
home-manager/services/docker/default.nix |
Refactors inline docker scripts/service text to external files via readFile + replaceVars. |
home-manager/services/docker-postgres/start-postgres-wrapper.sh |
New externalized docker postgres wrapper script. |
home-manager/services/docker-postgres/default.nix |
Refactors wrapper to external script via readFile + replaceVars. |
home-manager/services/cliproxyapi/scripts/docker-start.sh |
New externalized cliproxyapi docker-start wrapper script. |
home-manager/services/cliproxyapi/default.nix |
Refactors docker-start wrapper to external file via readFile + replaceVars. |
home-manager/modules/yek/yek-shim.sh |
New externalized yek shim script. |
home-manager/modules/yek/install-yek-shim.sh |
New externalized install-yek shim script. |
home-manager/modules/yek/default.nix |
Refactors inline shims to external shim files via readFile + replaceVars. |
devenv.nix |
Adds statix and refactors fishtape wrapper to external script. |
Makefile |
Adds python and nix-inline-check targets; wires them into make test. |
.github/workflows/python.yml |
Adds Python CI workflow (pytest + ruff). |
Comments suppressed due to low confidence (1)
named-hosts/matic/unlock-gnome-keyring.py:49
- The script executes its stdin-reading logic at import time (
pw = sys.stdin.read() ... sys.exit(...)). This forces tests to do brittle source-slicing/exec. Consider moving the CLI behavior into amain()and guarding it withif __name__ == "__main__":so the module can be imported and unit-tested normally.
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)
💡 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.
Comment on lines
+22
to
+26
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version-file: "pyproject.toml" | ||
| - name: Run tests |
Owner
Author
There was a problem hiding this comment.
Using python-version-file: pyproject.toml with actions/setup-python@v6 as intended. This is supported — v6 reads requires-python from pyproject.toml.
Comment on lines
+45
to
+47
| run: ruff check --output-format=github --target-version=py39 | ||
| - name: Check code formatting with Ruff | ||
| run: ruff format --diff --target-version=py39 |
Comment on lines
+47
to
+50
| run: ruff format --diff --target-version=py39 | ||
| continue-on-error: true | ||
| python-check: | ||
| if: always() |
Comment on lines
+899
to
+900
| @uv run ruff check --target-version=py313 | ||
| @uv run ruff format --diff --target-version=py313 |
|
|
||
| violations=$(grep -rn \ | ||
| --include='*.nix' \ | ||
| -P "write(Shell)?(Script|ScriptBin)\s+\"[^\"]+\"\s+''" \ |
Comment on lines
+60
to
+66
| sock.__enter__ = lambda s: s | ||
| sock.__exit__ = MagicMock(return_value=False) | ||
| sock.recv.return_value = resp | ||
| return sock | ||
|
|
||
| @patch("socket.socket") | ||
| @patch("os.lstat") |
Owner
Author
There was a problem hiding this comment.
Not applicable — lambda s: s is correct. Python calls __enter__(self) on the instance, so the mock receives itself as s and returns it. Tests pass.
Comment on lines
+8
to
+14
| import os | ||
| import stat | ||
| import struct | ||
| import types | ||
| import unittest | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request successfully refactors all inline shell and Python scripts from Nix files into external files, using builtins.readFile and pkgs.replaceVars. This significantly improves maintainability and readability by separating script logic from Nix configuration. The addition of a linter (scripts/check-nix-inline-scripts.sh) to prevent future inline scripts is a great proactive measure. Python testing and linting with pytest and ruff have also been integrated into the Makefile and CI, enhancing code quality for Python components. New ShellSpec and Python unit tests provide good coverage for the extracted scripts and new functionality.
| 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" |
Contributor
There was a problem hiding this comment.
While sudo "$TEE" is functional and acknowledged by the shellcheck disable=SC2024 comment, it's generally safer and more robust to pipe the content to sudo tee rather than relying on shell expansion with sudo. This helps prevent potential issues if the TEE variable were ever to be manipulated in a more complex environment, though less likely in Nix-controlled paths.
Consider changing it to:
cat "$DOCKER_SERVICE_FILE" | sudo "$TEE" /etc/systemd/system/docker.service > /dev/null
Suggested change
| sudo "$TEE" /etc/systemd/system/docker.service >/dev/null <"$DOCKER_SERVICE_FILE" | |
| sudo sh -c "cat \"$DOCKER_SERVICE_FILE\" > /etc/systemd/system/docker.service" |
Owner
Author
There was a problem hiding this comment.
Not applicable — pre-existing pattern extracted from inline Nix. ShellCheck SC2024 is already suppressed.
| # PAM exec script: runs as root, decrypts TPM credential, then | ||
| # uses runuser to run the Python unlock as the target user. | ||
| # @logger@, @systemd_creds@, @id@, @sleep@, @env@, @runuser@, @unlock_py@ | ||
| # are substituted by pkgs.replaceVars. |
Contributor
There was a problem hiding this comment.
It's good practice to include set -euo pipefail at the beginning of shell scripts to ensure strict error handling. This makes the script more robust by exiting immediately on errors, undefined variables, or failed pipeline commands.
Suggested change
| # are substituted by pkgs.replaceVars. | |
| set -euo pipefail |
Owner
Author
There was a problem hiding this comment.
Intentionally omitted — this PAM exec script must not use set -e because it needs to handle failures gracefully (credential decrypt, UID lookup) without blocking login. The background retry loop also requires careful error handling that set -e would break.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
named-hosts/matic/pam-gnome-keyring-tpm-unlock.sh (1)
16-21: Consider zeroing the password variable after the background fork inherits it.The decrypted password in
$PWpersists in memory for both the parent and the forked background subshell. While the parent exits immediately (line 56), the background loop retains$PWuntil all attempts complete or succeed. In the background subshell, consider unsetting$PWafter it's been used:🛡️ Optional: Clear password after use in background loop
if [ "$STATUS" -eq 0 ]; then UNLOCKED=1 + PW="" break fi done + PW="" [ "$UNLOCKED" -eq 0 ] && log "all attempts exhausted — keyring was NOT unlocked for $PAM_USER"Note: Bash doesn't guarantee memory is zeroed, but this reduces the exposure window.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/matic/pam-gnome-keyring-tpm-unlock.sh` around lines 16 - 21, The decrypted password stored in the PW variable is retained by both the parent and the background subshell; modify the background loop in pam-gnome-keyring-tpm-unlock.sh so that after the background subshell uses PW (e.g., after the decrypt result is consumed/passed to the unlock attempt), it explicitly unsets and clears PW (unset PW and set PW='' or similar) inside the subshell to minimize the exposure window; ensure the parent still exits as intended and that any subsequent logic does not rely on PW after it is cleared.scripts/check-nix-inline-scripts.sh (1)
12-20: Regex may miss edge cases with complex whitespace or multi-line patterns.The single-line grep pattern works for common inline script violations but won't catch cases where there's a newline between the function name and the
''delimiter, or where the script name uses string interpolation. For the PR's scope (enforcing the new convention going forward), this is acceptable.💡 Optional: Consider a multi-line aware pattern for completeness
If edge cases become a problem, you could use
pcregrep -Mfor multi-line matching:-violations=$(grep -rn \ - --include='*.nix' \ - -P "write(Shell)?(Script|ScriptBin)\s+\"[^\"]+\"\s+''" \ +violations=$(pcregrep -rn -M \ + --include='*.nix' \ + "write(Shell)?(Script|ScriptBin)\s+\"[^\"]+\"\s*\n?\s*''" \However, this adds a dependency and may not be necessary if the current pattern catches all existing violations.
🤖 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 pattern using write(Shell)?(Script|ScriptBin) ... '' only matches single-line cases and can miss when there's a newline between the function call and the '' delimiter or when the script name uses interpolation; to fix, switch to a multi-line aware matcher (e.g. use pcregrep -M) or enable PCRE DOTALL mode so the pattern spans newlines, update the pattern that references write(Shell)?(Script|ScriptBin) to allow arbitrary whitespace/newlines between tokens and to accept interpolated names (e.g. accept characters like ${...} inside the quoted name), and fall back to the original single-line grep only if pcregrep is unavailable. Ensure you update the command that creates the violations variable to use the new multi-line-aware invocation and the revised pattern.spec/cliproxyapi_docker_start_spec.sh (1)
36-55: These checks don’t actually validate the fallback behavior.This block only greps for strings in the template, so a broken condition, swapped branch order, or quoting regression in
home-manager/services/cliproxyapi/scripts/docker-start.shwould still go green. Consider substituting the placeholders into a temp copy and stubbingdocker/sgso each branch is executed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/cliproxyapi_docker_start_spec.sh` around lines 36 - 55, The tests in spec/cliproxyapi_docker_start_spec.sh only grep for strings in the script template and don't exercise branch logic in home-manager/services/cliproxyapi/scripts/docker-start.sh; update the spec to render the template into a temporary file, substitute any placeholders, then run controlled stubs for the external commands to force each branch: create a fake executable "docker" that returns success for the direct-docker branch, create fake "sg" under /run/wrappers/bin/sg to trigger the NixOS wrapper branch, and another fake /usr/bin/sg to trigger the system sg branch, and also stub all as absent to test the error path (assert output contains the appropriate messages like "docker info" or "Cannot access Docker"); ensure the test uses PATH manipulation and cleanup so the correct script branches (in docker-start.sh) are actually executed rather than just grepping the template.home-manager/services/docker-postgres/start-postgres-wrapper.sh (1)
6-10: Consider quoting the variable expansion for defensive coding.While Nix store paths don't contain spaces, quoting
$SCRIPTis shell best practice to guard against edge cases.♻️ Suggested improvement
-SCRIPT="@bash@/bin/bash `@start_script`@" +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 + exec $SCRIPT # $SCRIPT intentionally unquoted for word-splitting (bash + script path) fiActually, on second look, the variable is meant to be word-split into
bashandscript-path. Add a comment to clarify this is intentional:+# Note: SCRIPT intentionally unquoted to allow word splitting (bash binary + script path) SCRIPT="@bash@/bin/bash `@start_script`@"🤖 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 6 - 10, The exec invocation currently uses unquoted variable expansion exec $SCRIPT in start-postgres-wrapper.sh where SCRIPT is set to "@bash@/bin/bash `@start_script`@"; clarify intent by either quoting the expansion or, if the space is intentional to split into the interpreter and script, add an explicit comment beside the SCRIPT variable and the exec line stating that word-splitting is deliberate (and keep exec $SCRIPT unchanged), otherwise change to exec "$SCRIPT" to be defensive. Ensure references to the SCRIPT variable and the exec call remain consistent when editing.named-hosts/matic/falcon-init.sh (1)
1-4: Comment references@bash@but it's not used in the script.The comment on line 3 lists
@bash@as a substituted placeholder, andnamed-hosts/matic/falcon.nixpassesbash = pkgs.bashtoreplaceVars, but@bash@is never actually referenced in this script. Consider removing the unused binding fromfalcon.nixand updating this comment.♻️ Suggested fix
#!/usr/bin/env bash # CrowdStrike Falcon sensor init script. -# `@bash`@, `@e2fsprogs`@, `@rsync`@, `@falcon`@ are substituted by pkgs.replaceVars. +# `@e2fsprogs`@, `@rsync`@, `@falcon`@ are substituted by pkgs.replaceVars. set -euo pipefailAnd in
named-hosts/matic/falcon.nix:initScript = pkgs.writeScript "init-falcon" (builtins.readFile (pkgs.replaceVars ./falcon-init.sh { - bash = pkgs.bash; e2fsprogs = pkgs.e2fsprogs; rsync = pkgs.rsync; falcon = falcon; }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/matic/falcon-init.sh` around lines 1 - 4, The comment in falcon-init.sh mentions the substituted placeholder `@bash`@ which is never used; update the script comment to remove `@bash`@ from the list and then remove the unused binding passed into replaceVars from named-hosts/matic/falcon.nix (drop the bash = pkgs.bash argument or stop passing it to replaceVars) so the nix expression and the script comment remain consistent; locate the replaceVars call in falcon.nix and the header comment in falcon-init.sh to make these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/python.yml:
- Around line 44-47: The CI is forcing Ruff to use --target-version=py39 which
conflicts with the project's Python 3.13 target; edit the GitHub Actions steps
named "Lint code with Ruff" and "Check code formatting with Ruff" to remove the
--target-version=py39 flags from the ruff check and ruff format commands so Ruff
will read target-version from pyproject.toml (and thus align with the project's
py313 configuration).
In `@home-manager/services/docker/setup-docker.sh`:
- Around line 8-19: The script assumes the docker group exists and uses $USER
which can be wrong under sudo; change the logic to determine the target account
as TARGET_USER="${SUDO_USER:-$USER}", check for the docker group explicitly
(e.g. with getent group docker or similar) and create it if missing (use
groupadd -f docker via the appropriate groupadd binary), then run "$USERMOD" -aG
docker "$TARGET_USER" instead of modifying $USER so the correct non-root caller
is updated and group creation race/failure is avoided.
In `@Makefile`:
- Around line 896-900: Update the CI workflow to match the project's Ruff target
version: replace occurrences of the flag value "--target-version=py39" in the
".github/workflows/python.yml" workflow steps that run ruff with
"--target-version=py313" so the CI's ruff check/format invocations use the same
Python target as the Makefile (and pyproject.toml); ensure both ruff check and
ruff format steps are changed.
In `@named-hosts/matic/unlock-gnome-keyring.py`:
- Around line 12-16: The import block at the top of the module is unsorted and
failing Ruff; reorder the standard-library imports alphabetically (e.g., ensure
the imports read in alphabetical order: os, socket, stat, struct, sys) so the
import section is sorted per linter expectations; update the import block in the
module containing these names and re-run the linter/CI to verify the fix.
In `@spec/pam_gnome_keyring_tpm_unlock_spec.sh`:
- Around line 19-39: The test suite is missing assertions for three
placeholders—@id@, `@sleep`@, and `@env`@—so add three parallel examples to the
existing placeholder tests: create specs that run bash -c "grep '@id@'
'$SCRIPT'", bash -c "grep '@sleep@' '$SCRIPT'", and bash -c "grep '@env@'
'$SCRIPT'" and assert the output includes the respective placeholder; mirror the
pattern used by the existing tests for `@logger`@, `@systemd_creds`@, `@runuser`@, and
`@unlock_py`@ so all seven placeholders are covered.
In `@tests/test_unlock_gnome_keyring.py`:
- Around line 162-174: The test method test_daemon_closes_connection_raises has
a line that exceeds the 88-character limit; split or shorten the long
statement(s) where you assign socket context methods to the MagicMock
(specifically the sock.__enter__ and sock.__exit__ assignments) so each
assignment is on its own shorter line (e.g., set sock.__enter__ = lambda s: s on
one line and set sock.__exit__ = MagicMock(return_value=False) on the next, or
replace the lambda with a small named function) to bring the line length under
the limit.
- Around line 8-14: Remove the unused top-level imports importlib.util, socket,
and sys from the test file (they are declared but never referenced); update the
import list to only keep used modules (os, stat, struct, types) and ensure there
are no remaining references to importlib.util, socket, or sys elsewhere in the
file (e.g., in functions or fixtures such as any test helper) before running the
tests/linters.
---
Nitpick comments:
In `@home-manager/services/docker-postgres/start-postgres-wrapper.sh`:
- Around line 6-10: The exec invocation currently uses unquoted variable
expansion exec $SCRIPT in start-postgres-wrapper.sh where SCRIPT is set to
"@bash@/bin/bash `@start_script`@"; clarify intent by either quoting the expansion
or, if the space is intentional to split into the interpreter and script, add an
explicit comment beside the SCRIPT variable and the exec line stating that
word-splitting is deliberate (and keep exec $SCRIPT unchanged), otherwise change
to exec "$SCRIPT" to be defensive. Ensure references to the SCRIPT variable and
the exec call remain consistent when editing.
In `@named-hosts/matic/falcon-init.sh`:
- Around line 1-4: The comment in falcon-init.sh mentions the substituted
placeholder `@bash`@ which is never used; update the script comment to remove
`@bash`@ from the list and then remove the unused binding passed into replaceVars
from named-hosts/matic/falcon.nix (drop the bash = pkgs.bash argument or stop
passing it to replaceVars) so the nix expression and the script comment remain
consistent; locate the replaceVars call in falcon.nix and the header comment in
falcon-init.sh to make these edits.
In `@named-hosts/matic/pam-gnome-keyring-tpm-unlock.sh`:
- Around line 16-21: The decrypted password stored in the PW variable is
retained by both the parent and the background subshell; modify the background
loop in pam-gnome-keyring-tpm-unlock.sh so that after the background subshell
uses PW (e.g., after the decrypt result is consumed/passed to the unlock
attempt), it explicitly unsets and clears PW (unset PW and set PW='' or similar)
inside the subshell to minimize the exposure window; ensure the parent still
exits as intended and that any subsequent logic does not rely on PW after it is
cleared.
In `@scripts/check-nix-inline-scripts.sh`:
- Around line 12-20: The current grep pattern using
write(Shell)?(Script|ScriptBin) ... '' only matches single-line cases and can
miss when there's a newline between the function call and the '' delimiter or
when the script name uses interpolation; to fix, switch to a multi-line aware
matcher (e.g. use pcregrep -M) or enable PCRE DOTALL mode so the pattern spans
newlines, update the pattern that references write(Shell)?(Script|ScriptBin) to
allow arbitrary whitespace/newlines between tokens and to accept interpolated
names (e.g. accept characters like ${...} inside the quoted name), and fall back
to the original single-line grep only if pcregrep is unavailable. Ensure you
update the command that creates the violations variable to use the new
multi-line-aware invocation and the revised pattern.
In `@spec/cliproxyapi_docker_start_spec.sh`:
- Around line 36-55: The tests in spec/cliproxyapi_docker_start_spec.sh only
grep for strings in the script template and don't exercise branch logic in
home-manager/services/cliproxyapi/scripts/docker-start.sh; update the spec to
render the template into a temporary file, substitute any placeholders, then run
controlled stubs for the external commands to force each branch: create a fake
executable "docker" that returns success for the direct-docker branch, create
fake "sg" under /run/wrappers/bin/sg to trigger the NixOS wrapper branch, and
another fake /usr/bin/sg to trigger the system sg branch, and also stub all as
absent to test the error path (assert output contains the appropriate messages
like "docker info" or "Cannot access Docker"); ensure the test uses PATH
manipulation and cleanup so the correct script branches (in docker-start.sh) are
actually executed rather than just grepping the template.
🪄 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: a7c2a6e7-73be-4879-953a-760741f41fe6
📒 Files selected for processing (34)
.github/workflows/python.ymlMakefiledevenv.nixhome-manager/modules/yek/default.nixhome-manager/modules/yek/install-yek-shim.shhome-manager/modules/yek/yek-shim.shhome-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/pam-gnome-keyring-tpm-unlock.shnamed-hosts/matic/unlock-gnome-keyring.pypyproject.tomlscripts/check-nix-inline-scripts.shscripts/fishtape-wrapper.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
Comment on lines
+8
to
+19
| GROUPS_CMD=@shadow@/bin/groups | ||
| GREP=@gnugrep@/bin/grep | ||
| USERMOD=@shadow@/bin/usermod | ||
| SYSTEMCTL=@systemd@/bin/systemctl | ||
| TEE=@coreutils@/bin/tee | ||
| DOCKER_SERVICE_FILE=@docker_service_file@ | ||
|
|
||
| # Check if docker group exists and user is in it | ||
| 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" |
There was a problem hiding this comment.
Handle the target account and docker group explicitly.
Line 18 assumes the docker group already exists and that $USER is the account to modify. On a first run with the Nix-provided daemon/unit, usermod -aG docker ... can fail because the group is missing; if the wrapper itself is run under sudo, it updates root instead of the original caller.
Suggested fix
# Define paths
GROUPS_CMD=@shadow@/bin/groups
GREP=@gnugrep@/bin/grep
+GROUPADD=@shadow@/bin/groupadd
USERMOD=@shadow@/bin/usermod
SYSTEMCTL=@systemd@/bin/systemctl
TEE=@coreutils@/bin/tee
DOCKER_SERVICE_FILE=@docker_service_file@
+TARGET_USER="${SUDO_USER:-$USER}"
# Check if docker group exists and user is in it
-if ! "$GROUPS_CMD" | "$GREP" -q docker; then
+if ! "$GROUPS_CMD" "$TARGET_USER" | "$GREP" -qw docker; then
echo "Adding user to docker group..."
- sudo "$USERMOD" -aG docker "$USER"
+ sudo "$GROUPADD" -f docker
+ sudo "$USERMOD" -aG docker "$TARGET_USER"
echo "Added to docker group. Please log out and back in, or run: newgrp docker"
fi📝 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.
Suggested change
| GROUPS_CMD=@shadow@/bin/groups | |
| GREP=@gnugrep@/bin/grep | |
| USERMOD=@shadow@/bin/usermod | |
| SYSTEMCTL=@systemd@/bin/systemctl | |
| TEE=@coreutils@/bin/tee | |
| DOCKER_SERVICE_FILE=@docker_service_file@ | |
| # Check if docker group exists and user is in it | |
| 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" | |
| GROUPS_CMD=@shadow@/bin/groups | |
| GREP=@gnugrep@/bin/grep | |
| GROUPADD=@shadow@/bin/groupadd | |
| USERMOD=@shadow@/bin/usermod | |
| SYSTEMCTL=@systemd@/bin/systemctl | |
| TEE=@coreutils@/bin/tee | |
| DOCKER_SERVICE_FILE=@docker_service_file@ | |
| TARGET_USER="${SUDO_USER:-$USER}" | |
| # Check if docker group exists and user is in it | |
| if ! "$GROUPS_CMD" "$TARGET_USER" | "$GREP" -qw docker; then | |
| echo "Adding user to docker group..." | |
| sudo "$GROUPADD" -f docker | |
| sudo "$USERMOD" -aG docker "$TARGET_USER" | |
| echo "Added to docker group. Please log out and back in, or run: newgrp docker" |
🤖 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 8 - 19, The script
assumes the docker group exists and uses $USER which can be wrong under sudo;
change the logic to determine the target account as
TARGET_USER="${SUDO_USER:-$USER}", check for the docker group explicitly (e.g.
with getent group docker or similar) and create it if missing (use groupadd -f
docker via the appropriate groupadd binary), then run "$USERMOD" -aG docker
"$TARGET_USER" instead of modifying $USER so the correct non-root caller is
updated and group creation race/failure is avoided.
Owner
Author
There was a problem hiding this comment.
Not applicable — pre-existing code extracted from inline Nix. Docker group management logic was not changed in this PR.
CI fixes: - Add scripts/check-nix-inline-scripts.sh to coverage_spec.sh + create spec - Fix python-version in workflow: pin to "3.13" instead of pyproject.toml - Remove --target-version=py39 from CI ruff (reads from pyproject.toml) - Remove continue-on-error on ruff format; use --check to fail on violations - Fix Makefile python-lint to use --with ruff --no-project Review comment fixes: - Add 10s socket timeout in unlock-gnome-keyring.py (cubic P1) - Remove unused bash binding from falcon.nix replaceVars (coderabbit) - Use assertRaisesRegex instead of assertRaises(msg=) (cubic P2) - Add missing @id@, @sleep@, @env@ placeholder specs (coderabbit) - Replace grep -P with grep -E for macOS portability (copilot) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
home-manager/services/docker-postgres/default.nix (1)
9-17: Add behavioral tests for wrapper branch execution (not just content checks).After extraction, correctness now depends on runtime branching in
start-postgres-wrapper.sh, but current specs (seespec/start_postgres_wrapper_spec.sh, Lines 36-56) only grep for strings. Please add execution-focused tests that stub docker/sg paths and assert which branch is actually executed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/services/docker-postgres/default.nix` around lines 9 - 17, The test currently only greps the generated script content; instead add execution-based tests that run the produced wrapper and assert which runtime branch executes by stubbing executables: create temporary fake executables for docker and sg (or adjust PATH) that echo a distinct marker and are executable, then invoke the generated start-postgres-wrapper.sh (from startPostgresWrapper) with the test start_script and environment, capturing stdout/stderr to assert the marker for the expected branch; update spec/start_postgres_wrapper_spec.sh to set PATH to the temp bin and run the wrapper rather than only grepping its contents so runtime branching is 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 `@home-manager/services/docker-postgres/default.nix`:
- Around line 9-17: The test currently only greps the generated script content;
instead add execution-based tests that run the produced wrapper and assert which
runtime branch executes by stubbing executables: create temporary fake
executables for docker and sg (or adjust PATH) that echo a distinct marker and
are executable, then invoke the generated start-postgres-wrapper.sh (from
startPostgresWrapper) with the test start_script and environment, capturing
stdout/stderr to assert the marker for the expected branch; update
spec/start_postgres_wrapper_spec.sh to set PATH to the temp bin and run the
wrapper rather than only grepping its contents so runtime branching is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b310dd1-90e3-4bb7-ac9d-2be9daac91f4
📒 Files selected for processing (12)
devenv.nixhome-manager/modules/yek/default.nixhome-manager/services/cliproxyapi/default.nixhome-manager/services/docker-postgres/default.nixhome-manager/services/docker/default.nixhome-manager/services/docker/setup-docker.shnamed-hosts/matic/default.nixnamed-hosts/matic/falcon.nixnamed-hosts/matic/pam-gnome-keyring-tpm-unlock.shnamed-hosts/matic/unlock-gnome-keyring.pyscripts/check-nix-inline-scripts.shtests/test_unlock_gnome_keyring.py
✅ Files skipped from review due to trivial changes (1)
- scripts/check-nix-inline-scripts.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- home-manager/modules/yek/default.nix
- home-manager/services/docker/default.nix
- named-hosts/matic/unlock-gnome-keyring.py
- home-manager/services/docker/setup-docker.sh
- named-hosts/matic/pam-gnome-keyring-tpm-unlock.sh
- home-manager/services/cliproxyapi/default.nix
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
2 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
write*Script*calls in.nixfiles to external.sh/.pyfiles, wired back viabuiltins.readFile+pkgs.replaceVarsscripts/check-nix-inline-scripts.shlinter that fails CI if any inlinewrite*Script*patterns are found in.nixfiles.github/workflows/python.ymlwith pytest + ruff CI viauvpython-test,python-lint,python-test-dev,nix-inline-checkstatixtodevenv.nixfor Nix lintingTest plan
shellcheckpasses on all 10 new.shfilesshellspecpasses 82 examples across 10 new spec files (0 failures)pytestpasses 8 unit tests forunlock-gnome-keyring.pyscripts/check-nix-inline-scripts.shexits 0 (no violations)nix flake check— Nix evaluation succeeds withbuiltins.readFilereferencesmake build— home-manager/NixOS builds succeed🤖 Generated with Claude Code
Summary by cubic
Extracted all inline shell/Python in
.nixto external.sh/.pyfiles read viabuiltins.readFileandpkgs.replaceVars, and added CI/tests to lint and cover them. Follow‑ups harden CI, add a 10s timeout to the keyring unlock script, and fix portability issues.Refactors
yek, Docker setup, cliproxy API start, Docker Postgres start, Falcon init, and GNOME keyring unlock; templateddocker.service.ruffand treefmt/Alejandra; added a 10s socket timeout tounlock-gnome-keyring.pyand removed an unused Falcon binding.New Features
scripts/check-nix-inline-scripts.shandmake nix-inline-checkto block inlinewrite*Script*content (now usesgrep -Efor macOS); added ShellSpec coverage for new shells andpytesttests forunlock-gnome-keyring.py; addedstatix..github/workflows/python.ymlrunningpytestandruffviauv; usesactions/setup-python@v6withpython-version-filepointing topyproject.toml. Make targetspython-test,python-lint,python-test-dev;ruffreads frompyproject.tomlandruff formatuses--checkto fail on violations.Written for commit 4bdd234. Summary will update on new commits.