refactor: extract all inline home.activation scripts to bash files - #1443
Merged
Conversation
Move all inline shell code from home.activation blocks in .nix files
to external .sh scripts invoked via ${pkgs.bash}/bin/bash. This ensures
consistent bash detection and makes activation logic shellcheck-able.
- 21 new .sh files for activation scripts
- 3 shared scripts in home-manager/activation/ (ensure-directory,
deploy-agenix-secret, import-gpg-key) used by matic + kyber
- Boot detection moved from inline Nix conditionals to .sh scripts
via SYSTEMCTL_BIN env var (npm/cargo/uv-globals)
- Rustup setup moved to install-cargo-globals.sh via RUSTUP_BIN
- Updated check-nix-inline-scripts.sh to also lint activation blocks
- 10 new spec files with tests for all extracted scripts
- Updated coverage_spec.sh with all new scripts
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRefactors many inline Nix activation snippets into standalone Bash scripts and updates Nix modules to delegate activation to those scripts; adds shared activation helpers and extensive shell-spec tests covering creation, copying, permissioning, privilege escalation, and boot-time guards. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 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)
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;DRRefactored the What changed?Activation Logic Extraction
Configuration Management
Linting and Testing Enhancements
Miscellaneous
Description generated by Mesa. Update settings |
Mesa DescriptionTL;DRExtracted all inline What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="home-manager/activation/deploy-agenix-secret.sh">
<violation number="1" location="home-manager/activation/deploy-agenix-secret.sh:10">
P2: Re-add quotes around `DEST` in the file test to avoid test parse errors when the argument is empty.</violation>
<violation number="2" location="home-manager/activation/deploy-agenix-secret.sh:15">
P2: Quote `SECRET_FILE` in the file-existence check to keep behavior safe for empty input values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fi | ||
|
|
||
| echo "Deploying secret from agenix..." | ||
| if [[ ! -f $SECRET_FILE ]]; then |
Contributor
There was a problem hiding this comment.
P2: Quote SECRET_FILE in the file-existence check to keep behavior safe for empty input values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/activation/deploy-agenix-secret.sh, line 15:
<comment>Quote `SECRET_FILE` in the file-existence check to keep behavior safe for empty input values.</comment>
<file context>
@@ -7,12 +7,12 @@ SECRET_FILE="$2"
echo "Deploying secret from agenix..."
-if [[ ! -f "$SECRET_FILE" ]]; then
+if [[ ! -f $SECRET_FILE ]]; then
echo "Warning: Secret file not found at $SECRET_FILE" >&2
exit 0
</file context>
Suggested change
| if [[ ! -f $SECRET_FILE ]]; then | |
| if [[ ! -f "$SECRET_FILE" ]]; then |
| IDENTITY_KEY="$3" | ||
| RAGE_BIN="$4" | ||
|
|
||
| if [[ -f $DEST ]]; then |
Contributor
There was a problem hiding this comment.
P2: Re-add quotes around DEST in the file test to avoid test parse errors when the argument is empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/activation/deploy-agenix-secret.sh, line 10:
<comment>Re-add quotes around `DEST` in the file test to avoid test parse errors when the argument is empty.</comment>
<file context>
@@ -7,12 +7,12 @@ SECRET_FILE="$2"
RAGE_BIN="$4"
-if [[ -f "$DEST" ]]; then
+if [[ -f $DEST ]]; then
exit 0
fi
</file context>
Suggested change
| if [[ -f $DEST ]]; then | |
| if [[ -f "$DEST" ]]; then |
Contributor
There was a problem hiding this comment.
4 issues found across 62 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="home-manager/programs/fnm/default.nix">
<violation number="1" location="home-manager/programs/fnm/default.nix:28">
P2: Quote/escape activation script arguments to avoid argv splitting when `homeDir` contains shell-significant characters.</violation>
</file>
<file name="home-manager/modules/cargo-globals/install-cargo-globals.sh">
<violation number="1" location="home-manager/modules/cargo-globals/install-cargo-globals.sh:13">
P2: Avoid mutating the user’s global rustup default in activation. Use a script-local stable toolchain selection instead.</violation>
</file>
<file name="home-manager/activation/import-gpg-key.sh">
<violation number="1" location="home-manager/activation/import-gpg-key.sh:25">
P1: Ensure the decrypted temp key file is removed even when GPG import fails; with `set -e`, the current order can leave sensitive key material on disk.</violation>
</file>
<file name="scripts/check-nix-inline-scripts.sh">
<violation number="1" location="scripts/check-nix-inline-scripts.sh:76">
P2: The activation lint only reports violations when no `bin/bash` call exists, so blocks with a bash delegation plus extra inline shell commands incorrectly pass.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| echo "Importing GPG key from agenix..." | ||
| if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then | ||
| "$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null |
Contributor
There was a problem hiding this comment.
P1: Ensure the decrypted temp key file is removed even when GPG import fails; with set -e, the current order can leave sensitive key material on disk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/activation/import-gpg-key.sh, line 25:
<comment>Ensure the decrypted temp key file is removed even when GPG import fails; with `set -e`, the current order can leave sensitive key material on disk.</comment>
<file context>
@@ -0,0 +1,28 @@
+
+echo "Importing GPG key from agenix..."
+if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then
+ "$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null
+ rm -f "$GPG_TEMP_FILE"
+ echo "GPG key imported successfully"
</file context>
Comment on lines
+13
to
+14
| "$RUSTUP_BIN" toolchain install stable --no-self-update || true | ||
| "$RUSTUP_BIN" default stable || true |
Contributor
There was a problem hiding this comment.
P2: Avoid mutating the user’s global rustup default in activation. Use a script-local stable toolchain selection instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/modules/cargo-globals/install-cargo-globals.sh, line 13:
<comment>Avoid mutating the user’s global rustup default in activation. Use a script-local stable toolchain selection instead.</comment>
<file context>
@@ -2,6 +2,18 @@
+
+# Ensure stable toolchain is installed (RUSTUP_BIN set by nix activation)
+if [ -n "${RUSTUP_BIN:-}" ]; then
+ "$RUSTUP_BIN" toolchain install stable --no-self-update || true
+ "$RUSTUP_BIN" default stable || true
+fi
</file context>
Suggested change
| "$RUSTUP_BIN" toolchain install stable --no-self-update || true | |
| "$RUSTUP_BIN" default stable || true | |
| "$RUSTUP_BIN" toolchain install stable --no-self-update || true | |
| export RUSTUP_TOOLCHAIN=stable |
![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 refactors the Nix configuration by moving inline shell scripts from home.activation blocks into external bash scripts across numerous modules, including Tailscale, Neovim, and various AI tool configurations. It introduces shared activation helpers for directory management and secret deployment, while updating the repository linter to enforce this separation. Feedback highlights several critical improvements: implementing trap for secure GPG key cleanup, validating sudoers syntax with visudo before deployment, correcting a version-matching bug in the fnm script, and addressing platform-specific issues in the Neovim plugin build process. Additionally, it is recommended to refactor duplicated sudo detection logic and tighten the linter's validation of inline shell code.
Comment on lines
+23
to
+28
| echo "Importing GPG key from agenix..." | ||
| if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then | ||
| "$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null | ||
| rm -f "$GPG_TEMP_FILE" | ||
| echo "GPG key imported successfully" | ||
| fi |
Contributor
There was a problem hiding this comment.
The decrypted GPG private key is written to a temporary file on disk. If the script is interrupted or if the gpg --import command fails, this sensitive file may not be deleted, potentially exposing the private key. It is recommended to use a trap to ensure cleanup.
Suggested change
| echo "Importing GPG key from agenix..." | |
| if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then | |
| "$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null | |
| rm -f "$GPG_TEMP_FILE" | |
| echo "GPG key imported successfully" | |
| fi | |
| echo "Importing GPG key from agenix..." | |
| trap 'rm -f "$GPG_TEMP_FILE"' EXIT | |
| if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then | |
| "$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null | |
| echo "GPG key imported successfully" | |
| fi |
Comment on lines
+36
to
+38
| run_root_cmd ln -sf "$BASH_PATH" /bin/bash | ||
| run_root_cmd ln -sf "$FISH_PATH" /bin/fish | ||
| run_root_cmd ln -sf "$ZSH_PATH" /bin/zsh |
Contributor
There was a problem hiding this comment.
Overwriting /bin/bash with a symlink to the Nix store on a non-NixOS system is highly intrusive and risky. If the Nix store path becomes invalid or inaccessible, the system's primary shell will be broken, potentially preventing logins or breaking system scripts. It is generally safer to manage user shells via chsh or to provide the Nix-managed shells in the user's PATH rather than modifying system-wide paths in /bin.
Comment on lines
+53
to
+57
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | ||
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | ||
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | ||
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | ||
| fi |
Contributor
There was a problem hiding this comment.
The script overwrites the sudoers configuration without validating the syntax of the new file. A syntax error in /etc/sudoers.d/ can break sudo access for the entire system, requiring manual recovery. Use visudo -cf to validate the temporary file before copying it to the destination.
Suggested change
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | |
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | |
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | |
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | |
| fi | |
| if ! cmp -s "$TEMP_SUDOERS" "$SUDOERS_FILE" 2>/dev/null; then | |
| if ! visudo -cf "$TEMP_SUDOERS" >/dev/null 2>&1; then | |
| echo "Error: Invalid sudoers syntax generated" >&2 | |
| rm -f "$TEMP_SUDOERS" | |
| exit 1 | |
| fi | |
| run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" | |
| run_root_cmd chmod 0440 "$SUDOERS_FILE" | |
| echo "Sudo PATH configured. You can now use: sudo tailscale login" | |
| fi |
Comment on lines
+34
to
+51
| if [ -n "$fff_version" ]; then | ||
| _arch=$(uname -m) | ||
| _ldd=$(ldd --version 2>&1 || echo "") | ||
| if echo "$_ldd" | grep -q musl; then | ||
| _triple="${_arch}-unknown-linux-musl" | ||
| else | ||
| _triple="${_arch}-unknown-linux-gnu" | ||
| fi | ||
| mkdir -p "$fff_dir/target" | ||
| echo "Fetching https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/${_triple}.${LIB_EXT}" | ||
| curl --fail --location --silent --show-error \ | ||
| -o "$fff_binary" \ | ||
| "https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/${_triple}.${LIB_EXT}" && | ||
| echo "fff.nvim binary downloaded successfully" || | ||
| echo "fff.nvim binary download failed (will fall back to build on first use)" | ||
| else | ||
| echo "fff.nvim: could not determine version, skipping download" | ||
| fi |
Contributor
There was a problem hiding this comment.
The script assumes a Linux environment when constructing the download URL for the fff.nvim binary. On macOS, ldd is not available, and the resulting _triple (e.g., arm64-unknown-linux-gnu) will likely lead to a 404 error or an incompatible binary download. Additionally, consider adding a network connectivity check before attempting the download to avoid failures in offline environments.
if [ -n "$fff_version" ]; then
if ! timeout 3 bash -c 'exec 3<>/dev/tcp/1.1.1.1/53' 2>/dev/null; then
echo "fff.nvim: network unavailable, skipping download"
else
_arch=$(uname -m)
if [ "$(uname)" = "Darwin" ]; then
_triple="${_arch}-apple-darwin"
else
_ldd=$(ldd --version 2>&1 || echo "")
if echo "$_ldd" | grep -q musl; then
_triple="${_arch}-unknown-linux-musl"
else
_triple="${_arch}-unknown-linux-gnu"
fi
fi
mkdir -p "$fff_dir/target"
echo "Fetching https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/${_triple}.${LIB_EXT}"
curl --fail --location --silent --show-error \
-o "$fff_binary" \
"https://github.com/dmtrKovalenko/fff.nvim/releases/download/$fff_version/${_triple}.${LIB_EXT}" \
&& echo "fff.nvim binary downloaded successfully" \
|| echo "fff.nvim binary download failed (will fall back to build on first use)"
fi
else
echo "fff.nvim: could not determine version, skipping download"
fi
Comment on lines
+9
to
+16
| SUDO_CMD="" | ||
| if command -v sudo >/dev/null 2>&1; then | ||
| SUDO_CMD="sudo" | ||
| elif [ -x /run/wrappers/bin/sudo ]; then | ||
| SUDO_CMD="/run/wrappers/bin/sudo" | ||
| elif [ -x /usr/bin/sudo ]; then | ||
| SUDO_CMD="/usr/bin/sudo" | ||
| fi |
Contributor
There was a problem hiding this comment.
The sudo detection logic is duplicated across multiple activation scripts (k3s, bin-shells, tailscale, ip-forwarding). This logic should be refactored into a shared helper script in home-manager/activation/ to improve maintainability and ensure consistent behavior (e.g., consistent support for doas and error handling).
|
|
||
| # Install each version if not already installed | ||
| for version in "$DEFAULT_VERSION" "$@"; do | ||
| if ! ls -d "$FNM_DIR/node-versions/v${version}"* >/dev/null 2>&1; then |
Contributor
There was a problem hiding this comment.
The ls -d check for existing Node.js versions is too broad. For example, if version is "2", the pattern v2* will match an existing v22 installation, causing the script to incorrectly skip the installation of version 2. Use a more precise pattern or fnm list with a strict regex to verify the installed version.
Suggested change
| if ! ls -d "$FNM_DIR/node-versions/v${version}"* >/dev/null 2>&1; then | |
| if ! "$FNM_BIN" list 2>/dev/null | grep -qE "v${version}(\s|$|\.)"; then |
| # Anything else is a violation | ||
| has_violation = 1 | ||
| } | ||
| if (has_violation && !has_bash_call) { |
Contributor
There was a problem hiding this comment.
The linter logic if (has_violation && !has_bash_call) is too permissive. It allows inline shell code within a home.activation block as long as there is at least one call to bin/bash. This defeats the purpose of the check, which is to ensure that all logic is moved to external scripts. The check should flag any block where has_violation is true.
Suggested change
| if (has_violation && !has_bash_call) { | |
| if (has_violation) { |
Remove false-negative where $DRY_RUN_CMD lines were whitelisted unconditionally. Now any non-export, non-bash-delegation line is flagged, even with $DRY_RUN_CMD prefix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
scripts/check-nix-inline-scripts.sh-67-75 (1)
67-75:⚠️ Potential issue | 🟠 MajorAllowlist is too broad and can let inline commands pass undetected.
Lines 71–75 allow
${...}, quoted strings, and bare identifiers unconditionally. That permits inline command lines (for example,${pkgs.coreutils}/bin/mkdir ...) unless they fail some other check. Sincehas_bash_callis never enforced, blocks can bypass the delegation policy.Suggested tightening
- has_bash_call = 0 + has_bash_call = 0 + in_bash_continuation = 0 for (i in lines) { line = lines[i] gsub(/^[[:space:]]+/, "", line) if (line == "") continue if (line ~ /^export /) continue if (line ~ /^\$\{lib\.optionalString/) continue - if (line ~ /bin\/bash/) { has_bash_call = 1; continue } - if (line ~ /\\$/) continue - if (line ~ /^\$\{/) continue - if (line ~ /^".*"$/) continue - if (line ~ /^[A-Za-z0-9_-]+$/) continue + if (line ~ /^\$DRY_RUN_CMD[[:space:]]+\$\{pkgs\.bash\}\/bin\/bash[[:space:]]+\$\{\.\/[^}]+\}/) { + has_bash_call = 1 + in_bash_continuation = (line ~ /\\$/) + continue + } + if (in_bash_continuation && (line ~ /^\$\{/ || line ~ /^".*"$/ || line ~ /^[A-Za-z0-9_-]+$/)) { + in_bash_continuation = (line ~ /\\$/) + continue + } has_violation = 1 } + if (!has_bash_call) { + has_violation = 1 + } if (has_violation) { printf "%s: inline shell in home.activation block\n", FILENAME }Also applies to: 79-81
scripts/check-nix-inline-scripts.sh-49-49 (1)
49-49:⚠️ Potential issue | 🟠 MajorActivation block end matcher misses valid
''closers.Line 49 only accepts
'';, buthome.activationblocks wrapped in expressions (e.g.,mkIf (...)) commonly close with bare''and put;later. This can skip/merge blocks and miss violations.Suggested fix
- body_started && /^[[:space:]]*''';/ { + body_started && /^[[:space:]]*''([[:space:]]*;)?[[:space:]]*$/ {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-nix-inline-scripts.sh` at line 49, The activation-block end matcher in the body_started conditional only matches closing sequences that include a trailing semicolon (`'';`) which misses cases where activation blocks end with a bare `''` and the `;` appears later (e.g., inside `mkIf` wrappers); update the pattern used in the `body_started && /^[[:space:]]*'"''"';/` test to also accept a bare `''` closer (and optional following `;`) so the `body_started` branch correctly detects the end of `home.activation` blocks; locate the `body_started` conditional and its regex in scripts/check-nix-inline-scripts.sh and broaden the regex to match `''` with or without an immediate semicolon.home-manager/modules/cargo-globals/install-cargo-globals.sh-11-15 (1)
11-15:⚠️ Potential issue | 🟠 MajorMove rustup setup behind connectivity check and add timeouts.
Lines 11-15 run network-sensitive rustup operations before the offline guard (line 18) and without timeout, which can stall activation on flaky/offline systems.
💡 Suggested patch
-# Ensure stable toolchain is installed (RUSTUP_BIN set by nix activation) -if [ -n "${RUSTUP_BIN:-}" ]; then - "$RUSTUP_BIN" toolchain install stable --no-self-update || true - "$RUSTUP_BIN" default stable || true -fi - # Skip if offline if ! timeout 3 bash -c 'exec 3<>/dev/tcp/1.1.1.1/53' 2>/dev/null; then echo "Network unavailable, skipping cargo globals install" exit 0 fi + +# Ensure stable toolchain is installed (RUSTUP_BIN set by nix activation) +if [ -n "${RUSTUP_BIN:-}" ]; then + timeout 120 "$RUSTUP_BIN" toolchain install stable --no-self-update || true + timeout 30 "$RUSTUP_BIN" default stable || true +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/modules/cargo-globals/install-cargo-globals.sh` around lines 11 - 15, The rustup network operations (invocations of "$RUSTUP_BIN" toolchain install/default) must be moved to run after the existing offline guard check and protected with timeouts so activation won’t hang on flaky or offline systems; relocate the whole if [ -n "${RUSTUP_BIN:-}" ]; then ... fi block to after the offline connectivity check, and wrap the rustup calls with a timeout (e.g., timeout 10s) and preserve the || true fallback so failures don’t break activation.home-manager/modules/cargo-globals/default.nix-28-29 (1)
28-29:⚠️ Potential issue | 🟠 MajorPropagate the new script env vars to the user service too.
These exports only affect the Home Manager activation path. The Linux
systemd.user.services.install-cargo-globalsunit at Lines 42-51 runs the sameinstall-cargo-globals.sh, but withoutRUSTUP_BINthat path skipsrustup toolchain install stable/rustup default stable, so login-time behavior no longer matches activation.🔧 Proposed fix
Service = { Type = "simple"; Environment = [ "PATH=${pkgs.rustup}/bin:${pkgs.cargo}/bin:${pkgs.rustc}/bin:${pkgs.dasel}/bin:${pkgs.jq}/bin:${pkgs.gcc}/bin:${pkgs.pkg-config}/bin:${pkgs.perl}/bin:${pkgs.coreutils}/bin:${pkgs.bash}/bin" "CARGO_HOME=%h/.cargo" "HOME=%h" + "SYSTEMCTL_BIN=${pkgs.systemd}/bin/systemctl" + "RUSTUP_BIN=${pkgs.rustup}/bin/rustup" "PKG_CONFIG_PATH=${pkgs.openssl.dev}/lib/pkgconfig${libiconvPkgConfigPath}" "OPENSSL_DIR=${pkgs.openssl.dev}" "OPENSSL_LIB_DIR=${pkgs.openssl.out}/lib" "OPENSSL_INCLUDE_DIR=${pkgs.openssl.dev}/include" ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/modules/cargo-globals/default.nix` around lines 28 - 29, The activation exports (RUSTUP_BIN and conditional SYSTEMCTL_BIN) are not propagated into the systemd.user.services.install-cargo-globals unit, so the user service runs install-cargo-globals.sh without RUSTUP_BIN; update the service definition (install-cargo-globals) to export the same environment into the unit—e.g., add Environment=RUSTUP_BIN=${pkgs.rustup}/bin/rustup and, when !isDarwin, Environment=SYSTEMCTL_BIN=${pkgs.systemd}/bin/systemctl (or point EnvironmentFile to the same generated env file) so install-cargo-globals.sh sees the same variables during login as during activation.config/k3s/activate.sh-18-23 (1)
18-23:⚠️ Potential issue | 🟠 MajorHandle root-without-sudo and make copy non-interactive.
Current logic skips installation whenever
sudois missing, even if the script runs as root. Also, usecp -fto comply with non-interactive file-operation rules.Proposed fix
-if [ -n "$SUDO_CMD" ]; then - $SUDO_CMD mkdir -p /etc/rancher/k3s - $SUDO_CMD cp "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml +if [ -n "$SUDO_CMD" ]; then + "$SUDO_CMD" mkdir -p /etc/rancher/k3s + "$SUDO_CMD" cp -f "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml +elif [ "$(id -u)" -eq 0 ]; then + mkdir -p /etc/rancher/k3s + cp -f "$HOME/.config/k3s/config.yaml" /etc/rancher/k3s/config.yaml else echo "Warning: sudo not found, skipping k3s config installation" >&2 fiAs per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/k3s/activate.sh` around lines 18 - 23, The script currently only attempts installation when SUDO_CMD is set; change the logic to: if SUDO_CMD is available use it, else if the script is running as root (UID=0) run the commands without sudo, otherwise print the warning; also make the file copy non-interactive by using cp -f and ensure mkdir uses -p as before; update references to SUDO_CMD and the commands that create /etc/rancher/k3s and copy "$HOME/.config/k3s/config.yaml" so they run under sudo when present or directly when UID=0.named-hosts/matic/default.nix-520-523 (1)
520-523:⚠️ Potential issue | 🟠 MajorPotential secret path mismatch for GitHub SSH key deployment.
Line 522 uses
id_ed25519.age, but this host’s agenix mapping targets"keys/id_github.age"for~/.ssh/id_ed25519_github(Line 496-500), andnamed-hosts/kyber/default.nixusesid_github.agefor the same deploy flow. This looks like a likely wrong secret input.Likely fix
- ${builtins.toString ../galactica/keys/id_ed25519.age} \ + ${builtins.toString ../galactica/keys/id_github.age} \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/matic/default.nix` around lines 520 - 523, The deploy call to deploy-agenix-secret.sh is passing ../galactica/keys/id_ed25519.age but this host's agenix mapping and the kyber host expect keys named id_github.age for ~/.ssh/id_ed25519_github; update the secret argument passed to deploy-agenix-secret.sh (the third positional arg in the invocation using DRY_RUN_CMD and ${pkgs.bash}/bin/bash) to reference the consistent secret name (keys/id_github.age) so the agenix mapping for id_ed25519_github matches across hosts.home-manager/activation/deploy-agenix-secret.sh-20-25 (1)
20-25:⚠️ Potential issue | 🟠 MajorHarden secret file creation permissions during decrypt.
At Line 20,
ragewrites directly to"$DEST"before Line 21 applieschmod 0600. File mode is umask-dependent until chmod runs, which can briefly expose secret contents.Suggested hardening
set -euo pipefail +umask 077 DEST="$1" SECRET_FILE="$2" IDENTITY_KEY="$3" RAGE_BIN="$4"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/activation/deploy-agenix-secret.sh` around lines 20 - 25, The script currently lets rage write directly to "$DEST" (subject to the current umask) before chmod 0600 is applied, risking brief exposure; change the flow to decrypt into a securely-created temporary file in the same directory (create with mktemp or set umask 077 and create temp with restricted perms), ensure the temp is chmod 0600 before invoking RAGE_BIN -o "$TMP", on success atomically move the temp into "$DEST" (mv -f) and remove the temp on failure; reference the variables RAGE_BIN, IDENTITY_KEY, SECRET_FILE and DEST and ensure cleanup/exit codes are preserved.home-manager/activation/import-gpg-key.sh-11-27 (1)
11-27:⚠️ Potential issue | 🟠 MajorAvoid predictable temp key path and guarantee cleanup on all exits.
At Line 11/24-27, decrypted key material is written to a fixed path and only removed on success. If import fails, plaintext key can remain on disk.
Safer temp-file handling
-GPG_TEMP_FILE="$AGENIX_DIR/gpg.key" +GPG_TEMP_FILE="$(mktemp "$AGENIX_DIR/gpg.XXXXXX.key")" +trap 'rm -f "$GPG_TEMP_FILE"' EXIT @@ if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then "$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null - rm -f "$GPG_TEMP_FILE" echo "GPG key imported successfully" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/activation/import-gpg-key.sh` around lines 11 - 27, The script writes decrypted key material to a predictable path in GPG_TEMP_FILE and only removes it on successful import, risking leftover plaintext; change to create a secure temporary file (use mktemp or similar) with restrictive permissions for the decrypted output, write the RAGE_BIN -d -i output to that temp file, and add a cleanup trap (trap 'rm -f "$TMPFILE"' EXIT INT TERM) so the temp is removed on all exits/failures; update references from GPG_TEMP_FILE to the new TMPFILE and ensure any early-exit paths (including failed import) still trigger the cleanup before exiting.config/gemini/activate.sh-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorUse
cp -fto keep activation non-interactive.Line 9 uses plain
cp; this violates the repo’s non-interactive shell rule for file operations.Proposed fix
- cp "$SETTINGS_JSON" "$HOME/.gemini/settings.json" + cp -f "$SETTINGS_JSON" "$HOME/.gemini/settings.json"As per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/gemini/activate.sh` at line 9, Replace the interactive copy with a forced, non-interactive copy: update the cp invocation that copies "$SETTINGS_JSON" to "$HOME/.gemini/settings.json" by adding the -f flag (i.e., use cp -f) so the activation script's copy operation is non-interactive and won't prompt for confirmations.config/codex/activate.sh-8-12 (1)
8-12:⚠️ Potential issue | 🟠 MajorHooks destination path appears incorrect.
Line 8 creates
~/.codex/hooks, but Lines 11-12 write to~/.codex/hooks.jsoninstead of inside that directory. This mismatch is likely a path bug.Proposed fix
mkdir -p ~/.codex/hooks cp -f "$CONFIG_TOML" ~/.codex/config.toml chmod 600 ~/.codex/config.toml -cp -f "$HOOKS_JSON" ~/.codex/hooks.json -chmod 644 ~/.codex/hooks.json +cp -f "$HOOKS_JSON" ~/.codex/hooks/hooks.json +chmod 644 ~/.codex/hooks/hooks.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/codex/activate.sh` around lines 8 - 12, The script creates a hooks directory (~/.codex/hooks) but then copies the hooks file to ~/.codex/hooks.json, so update the destination to the directory: copy "$HOOKS_JSON" to ~/.codex/hooks/hooks.json and set its permissions (chmod 644 ~/.codex/hooks/hooks.json); keep the CONFIG_TOML handling as-is (CONFIG_TOML -> ~/.codex/config.toml) and ensure references to HOOKS_JSON and the mkdir command align with the new path.config/serena/activate.sh-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorUse forced non-interactive copy.
Line 11 should use
cp -fto comply with the repo’s non-interactive shell policy.Proposed fix
- cp "$SRC" "$DEST" + cp -f "$SRC" "$DEST"As per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/serena/activate.sh` at line 11, Replace the interactive copy command with a forced non-interactive copy: update the cp invocation that uses the variables SRC and DEST (cp "$SRC" "$DEST") to include the -f flag so it becomes a forced, non-interactive operation (use cp -f "$SRC" "$DEST"). Ensure the surrounding logic still handles errors/exit codes as before.hosts/linux/activate-backup-files.sh-8-8 (1)
8-8:⚠️ Potential issue | 🟠 MajorUse forced non-interactive moves and quote the codex path.
Line 8 and Line 14 should use
mv -fper repo policy. Also prefer"$HOME/.codex"on Line 17 for path safety.Proposed fix
- mv "$HOME/$file" "$HOME/$file.hm-backup" + mv -f "$HOME/$file" "$HOME/$file.hm-backup" @@ - mv "$HOME/.openclaw/openclaw.json" "$HOME/.openclaw/openclaw.json.hm-backup" + mv -f "$HOME/.openclaw/openclaw.json" "$HOME/.openclaw/openclaw.json.hm-backup" @@ -find ~/.codex -name "*.hm-backup*" -delete 2>/dev/null || true +find "$HOME/.codex" -name "*.hm-backup*" -delete 2>/dev/null || trueAs per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
Also applies to: 14-14, 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hosts/linux/activate-backup-files.sh` at line 8, Change the interactive move commands to forced non-interactive moves and quote the codex path: replace occurrences of mv "$HOME/$file" "$HOME/$file.hm-backup" (both spots) with a non-interactive mv -f variant, and update the codex directory reference to use the quoted form "$HOME/.codex" to ensure path safety and comply with the repo policy on file operations.config/omp/activate.sh-7-10 (1)
7-10:⚠️ Potential issue | 🟠 MajorQuote HOME-derived paths to avoid word-splitting bugs.
Line 7–Line 10 use unquoted
~paths. If HOME ever contains spaces, these commands can break.Proposed fix
-mkdir -p ~/.omp/agent ~/.omp/agent/extensions -cp -f "$CONFIG_YML" ~/.omp/agent/config.yml -chmod 644 ~/.omp/agent/config.yml -ln -sfn "$HOME/dotfiles/node_modules/@oh-my-pi/swarm-extension" ~/.omp/agent/extensions/swarm-extension +mkdir -p "$HOME/.omp/agent" "$HOME/.omp/agent/extensions" +cp -f "$CONFIG_YML" "$HOME/.omp/agent/config.yml" +chmod 644 "$HOME/.omp/agent/config.yml" +ln -sfn "$HOME/dotfiles/node_modules/@oh-my-pi/swarm-extension" "$HOME/.omp/agent/extensions/swarm-extension"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/omp/activate.sh` around lines 7 - 10, The commands mkdir -p, cp -f, chmod and ln are using unquoted HOME-derived paths (e.g. ~/.omp/agent, ~/.omp/agent/extensions and $HOME/dotfiles/...) which can break if HOME contains spaces; change to use explicit "$HOME" and quote all path arguments (e.g. mkdir -p "$HOME/.omp/agent" "$HOME/.omp/agent/extensions", cp -f "$CONFIG_YML" "$HOME/.omp/agent/config.yml", chmod 644 "$HOME/.omp/agent/config.yml", ln -sfn "$HOME/dotfiles/node_modules/@oh-my-pi/swarm-extension" "$HOME/.omp/agent/extensions/swarm-extension") so that CONFIG_YML and HOME-derived paths are safely handled in mkdir, cp, chmod and ln.spec/activate_fnm_spec.sh-35-42 (1)
35-42:⚠️ Potential issue | 🟠 MajorThese assertions can pass on comments instead of the command lines.
grep 'install'and especiallygrep 'fnm.*default'are broad enough to match the usage/comments inhome-manager/programs/fnm/activate.sh, so this spec can stay green even if the actual"$FNM_BIN" install ...or"$FNM_BIN" default ...commands are removed. Match the executable lines more precisely here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/activate_fnm_spec.sh` around lines 35 - 42, The current spec uses broad grep patterns that can match comments; tighten assertions to search for the actual executable lines by matching the literal command invocations such as "$FNM_BIN" install and "$FNM_BIN" default (or the exact line in activate.sh that invokes FNM) instead of generic 'install' or 'fnm.*default'; update the tests in spec/activate_fnm_spec.sh to grep for the quoted command pattern (including "$FNM_BIN" and the subcommand) or anchor the line start so the test only passes when the real install/default command is present.home-manager/programs/fnm/default.nix-24-30 (1)
24-30:⚠️ Potential issue | 🟠 MajorThe stable
nodesymlink is still effectively pinned to v22.This activation now passes
${defaultVersion}, buthome-manager/programs/fnm/activate.sh, Lines 21-28, still resolves~/.local/bin/nodefromv22*instead ofDEFAULT_VERSION. As soon asnodeVersionschanges,fnm defaultand the stable symlink diverge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/fnm/default.nix` around lines 24 - 30, The activation currently passes defaultVersion from home.activation.fnmSetup but activate.sh still resolves the stable node symlink by hardcoded glob v22*; update activate.sh to accept and use the passed DEFAULT_VERSION (third argument from the activation call) when locating/creating the ~/.local/bin/node stable symlink (i.e., replace the v22* glob resolution with logic that uses the DEFAULT_VERSION argument or its derived path), ensuring fnm default and the stable symlink remain in sync with the defaultVersion provided by home.activation.fnmSetup.spec/activate_hosts_spec.sh-43-72 (1)
43-72:⚠️ Potential issue | 🟠 MajorAdd a regression check for non-interactive
mv -f.
hosts/linux/activate-backup-files.sh, Lines 5-13, still use plainmv, and this suite would stay green. That leaves the extracted activation logic free to violate the repo shell rule and potentially block on confirmation prompts.Proposed spec hardening
Describe 'backup behavior' +It 'uses non-interactive moves' +When run bash -c "grep 'mv -f' '$SCRIPT'" +The output should include 'mv -f' +End + It 'backs up .bashrc' When run bash -c "grep '.bashrc' '$SCRIPT'" The output should include '.bashrc' EndAs per coding guidelines "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/activate_hosts_spec.sh` around lines 43 - 72, The activation script hosts/linux/activate-backup-files.sh is using plain mv (lines ~5-13) which can block on confirmation; update every mv invocation in that script to use mv -f (non-interactive) and similarly ensure any cp/rm calls there use their non-interactive flags (cp -f, rm -f or rm -rf) so file ops never prompt; verify the updated commands still preserve the existing behavior and adjust the spec if needed to assert the presence of '-f' in the generated activation script.spec/activate_config_spec.sh-80-110 (1)
80-110:⚠️ Potential issue | 🟠 MajorAdd assertions for repo-mandated
cp -fusage in these config suites.
config/gemini/activate.sh, Lines 7-10, andconfig/serena/activate.sh, Lines 9-13, currently use plaincp, and nothing in this spec would catch that. Since these tests were added as the regression net for the extracted scripts, they should also guard the required non-interactive copy flags.As per coding guidelines "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/activate_config_spec.sh` around lines 80 - 110, Add assertions to the two spec blocks that verify the scripts use non-interactive cp flags: for the gemini block (where SCRIPT="$PWD/config/gemini/activate.sh") add a test that greps for 'cp -f' (similar style to the existing grep tests), and for the serena block (where SCRIPT="$PWD/config/serena/activate.sh") add the same grep assertion for 'cp -f'; ensure the new It/When run lines mirror the existing pattern so the spec fails if plain `cp` is used instead of `cp -f`.named-hosts/kyber/activate-ip-forwarding.sh-34-37 (1)
34-37:⚠️ Potential issue | 🟠 MajorPersist the desired sysctl contents every run.
This only writes
99-tailscale.confwhen the file is missing. If the file already exists with stale or partial contents, activation never repairs it, so the system can drift permanently from the declared state.Proposed fix
-if [ ! -f /etc/sysctl.d/99-tailscale.conf ]; then - echo 'net.ipv4.ip_forward=1' | run_root_cmd tee /etc/sysctl.d/99-tailscale.conf - echo 'net.ipv6.conf.all.forwarding=1' | run_root_cmd tee -a /etc/sysctl.d/99-tailscale.conf -fi +{ + echo 'net.ipv4.ip_forward=1' + echo 'net.ipv6.conf.all.forwarding=1' +} | run_root_cmd tee /etc/sysctl.d/99-tailscale.conf >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/kyber/activate-ip-forwarding.sh` around lines 34 - 37, The script currently only writes /etc/sysctl.d/99-tailscale.conf if it does not exist; change it so activation enforces the two sysctl settings every run by ensuring /etc/sysctl.d/99-tailscale.conf contains both "net.ipv4.ip_forward=1" and "net.ipv6.conf.all.forwarding=1" (either by atomically overwriting the file with those two lines or by idempotently replacing/adding those keys), using the same helper run_root_cmd and tee invocation patterns so the activation always repairs stale or partial contents.named-hosts/kyber/activate-ip-forwarding.sh-29-33 (1)
29-33:⚠️ Potential issue | 🟠 MajorIPv6 forwarding is skipped whenever IPv4 is already enabled.
The guard only reads
/proc/sys/net/ipv4/ip_forward, but it gates bothsysctl -wcalls. A machine with IPv4 forwarding already set to1and IPv6 forwarding still at0stays half-configured after activation.Proposed fix
-if [ "$(cat /proc/sys/net/ipv4/ip_forward)" != "1" ]; then - echo "Enabling IP forwarding for Tailscale exit node..." - run_root_cmd sysctl -w net.ipv4.ip_forward=1 - run_root_cmd sysctl -w net.ipv6.conf.all.forwarding=1 -fi +if [ "$(cat /proc/sys/net/ipv4/ip_forward)" != "1" ]; then + echo "Enabling IPv4 forwarding for Tailscale exit node..." + run_root_cmd sysctl -w net.ipv4.ip_forward=1 +fi + +if [ "$(cat /proc/sys/net/ipv6/conf/all/forwarding)" != "1" ]; then + echo "Enabling IPv6 forwarding for Tailscale exit node..." + run_root_cmd sysctl -w net.ipv6.conf.all.forwarding=1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/kyber/activate-ip-forwarding.sh` around lines 29 - 33, The current if guard checks only /proc/sys/net/ipv4/ip_forward and gates both sysctl calls, leaving IPv6 unchanged if IPv4 is already 1; update the logic to test and enable IPv4 and IPv6 separately by reading /proc/sys/net/ipv4/ip_forward and /proc/sys/net/ipv6/conf/all/forwarding, and call run_root_cmd sysctl -w net.ipv4.ip_forward=1 only when the IPv4 value is not "1" and run_root_cmd sysctl -w net.ipv6.conf.all.forwarding=1 only when the IPv6 value is not "1" (preserve existing echo/logging behavior for each change).home-manager/modules/tailscale/activate-install-service.sh-38-38 (1)
38-38:⚠️ Potential issue | 🟠 MajorUse forced copy flags for root writes to keep activation non-interactive.
Line 38 and Line 54 use
cpwithout-f. This can become interactive in some environments and breaks the repo’s non-interactive contract for activation scripts.Suggested patch
- run_root_cmd cp "$NIX_SERVICE" "$SERVICE_FILE" + run_root_cmd cp -f "$NIX_SERVICE" "$SERVICE_FILE" @@ - run_root_cmd cp "$TEMP_SUDOERS" "$SUDOERS_FILE" + run_root_cmd cp -f "$TEMP_SUDOERS" "$SUDOERS_FILE"As per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/modules/tailscale/activate-install-service.sh` at line 38, The cp invocations performed via run_root_cmd (specifically the calls that copy "$NIX_SERVICE" to "$SERVICE_FILE" and the similar copy at the later occurrence) are missing the non-interactive forced flag and can block activation; update those calls to use cp -f (i.e., run_root_cmd cp -f "$NIX_SERVICE" "$SERVICE_FILE") so root writes are forced and non-interactive, ensuring both the first occurrence around the run_root_cmd copying NIX_SERVICE->SERVICE_FILE and the second occurrence (the copy at the later location) are changed consistently.home-manager/programs/fnm/activate.sh-23-29 (1)
23-29:⚠️ Potential issue | 🟠 MajorDon’t hardcode
v22whenDEFAULT_VERSIONis already an input.Line 25 ignores the passed default version, so the
nodesymlink can drift from the configured default when that value changes.Suggested patch
- latest_v22=$(ls -d "$FNM_DIR/node-versions/v22"* 2>/dev/null | head -1) - if [ -n "$latest_v22" ] && [ -d "$latest_v22/installation/bin" ]; then + latest_default=$(ls -d "$FNM_DIR/node-versions/v${DEFAULT_VERSION}"* 2>/dev/null | head -1) + if [ -n "$latest_default" ] && [ -d "$latest_default/installation/bin" ]; then mkdir -p "$HOME/.local/bin" - ln -sf "$latest_v22/installation/bin/node" "$HOME/.local/bin/node" + ln -sf "$latest_default/installation/bin/node" "$HOME/.local/bin/node" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/fnm/activate.sh` around lines 23 - 29, The code hardcodes "v22" when locating the default Node version; update the logic to use the DEFAULT_VERSION variable instead: replace the latest_v22 lookup with a variable that builds the pattern from "$DEFAULT_VERSION" (e.g. latest_v_default=$(ls -d "$FNM_DIR/node-versions/v${DEFAULT_VERSION}"* 2>/dev/null | head -1)), then proceed to test [ -n "$latest_v_default" ] and [ -d "$latest_v_default/installation/bin" ] and create the symlink from that path to "$HOME/.local/bin/node"; also rename references to latest_v22 accordingly (and ensure DEFAULT_VERSION is quoted when interpolated).named-hosts/kyber/default.nix-36-36 (1)
36-36:⚠️ Potential issue | 🟠 MajorInconsistent dry-run guarding in backup activation hook.
Line 36 invokes
activate-backup-files.shwithout the$DRY_RUN_CMDprefix, while other activation hooks (lines 62, 67, 73–77, 83–89, 122) consistently use it. The backup script containsmvcommands without internal dry-run protection, meaning a dry-run of home-manager will still mutate user files instead of being safely skipped.Apply the fix:
Suggested fix
- ${pkgs.bash}/bin/bash ${./activate-backup-files.sh} + $DRY_RUN_CMD ${pkgs.bash}/bin/bash ${./activate-backup-files.sh}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/kyber/default.nix` at line 36, The activate-backup-files hook calls ${pkgs.bash}/bin/bash ${./activate-backup-files.sh} without using the DRY_RUN_CMD prefix, so a home-manager dry-run still executes the script (which contains mv operations) and mutates files; change the invocation to include the DRY_RUN_CMD (i.e. prefix the command with ${DRY_RUN_CMD}) so that when DRY_RUN_CMD is set the script run is a no-op, ensuring activate-backup-files.sh is guarded consistently with the other activation hooks that use DRY_RUN_CMD.
🟡 Minor comments (3)
named-hosts/kyber/activate-backup-files.sh-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorUse
mv -ffor non-interactive activation behavior.Please force the move to avoid any prompt-driven hangs in automation contexts.
Proposed fix
- mv "$HOME/$file" "$HOME/$file.hm-backup" + mv -f "$HOME/$file" "$HOME/$file.hm-backup"As per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@named-hosts/kyber/activate-backup-files.sh` at line 8, The mv invocation in activate-backup-files.sh uses interactive defaults which can hang automation; update the move command that references "$HOME/$file" -> "$HOME/$file.hm-backup" to use the non-interactive forced flag (mv -f) so the rename proceeds without prompts in scripts and CI.hosts/linux/default.nix-39-39 (1)
39-39:⚠️ Potential issue | 🟡 MinorDelegated backup script uses interactive-prone
mvcalls.This activation now routes through
hosts/linux/activate-backup-files.sh, where the backup moves are currentlymv ...without-f. Please switch those tomv -fto keep activation fully non-interactive.Proposed patch (in
hosts/linux/activate-backup-files.sh)- mv "$HOME/$file" "$HOME/$file.hm-backup" + mv -f "$HOME/$file" "$HOME/$file.hm-backup" @@ - mv "$HOME/.openclaw/openclaw.json" "$HOME/.openclaw/openclaw.json.hm-backup" + mv -f "$HOME/.openclaw/openclaw.json" "$HOME/.openclaw/openclaw.json.hm-backup"As per coding guidelines, "Always use non-interactive flags with file operations (cp -f, mv -f, rm -f, rm -rf, cp -rf) to avoid hanging on confirmation prompts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hosts/linux/default.nix` at line 39, The delegated activation script hosts/linux/activate-backup-files.sh currently uses interactive-prone move commands; update every plain mv invocation in that script to use mv -f so the activation is non-interactive (replace occurrences of "mv " with "mv -f " for the backup/move operations), ensuring all backup/move paths handled by the script (including any within functions or loops) use the -f flag.home-manager/programs/neovim/default.nix-47-50 (1)
47-50:⚠️ Potential issue | 🟡 MinorQuote
${packDir}when invoking the extracted script.
packDirexpands$HOMEat runtime. Without quotes, a home path containing spaces is split into multiple argv entries andactivate-build-plugins.shreceives a broken pack directory.Proposed fix
- $DRY_RUN_CMD ${pkgs.bash}/bin/bash ${./activate-build-plugins.sh} ${packDir} ${libExt} + $DRY_RUN_CMD ${pkgs.bash}/bin/bash ${./activate-build-plugins.sh} "${packDir}" "${libExt}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/programs/neovim/default.nix` around lines 47 - 50, In the home.activation.buildNvimNativePlugins shell block, the unquoted ${packDir} can expand to a path with spaces and break the argv passed to ${./activate-build-plugins.sh}; update the invocation inside the heredoc so the script is called with "${packDir}" (and optionally quote ${libExt} too) to ensure the pack directory is passed as a single argument; locate the block using home.activation.buildNvimNativePlugins and the activate-build-plugins.sh invocation to apply the change.
🧹 Nitpick comments (3)
config/openclaw/hydrate.sh (1)
9-9: Remove redundantmkdir -p "$STATE_DIR"invocation.Now that Line 9 creates
STATE_DIR, the secondmkdir -p "$STATE_DIR"at Line 61 is duplicate and can be removed to keep flow cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/openclaw/hydrate.sh` at line 9, Remove the redundant directory creation: since STATE_DIR is already created earlier in hydrate.sh with mkdir -p "$STATE_DIR", delete the later duplicate mkdir -p "$STATE_DIR" invocation (the second occurrence) so the script only creates STATE_DIR once; ensure no other logic depends on that repeated call and preserve surrounding commands as-is.hosts/darwin/default.nix (1)
47-47: Consider honoring Home Manager dry-run mode here.Line 47 runs cleanup unconditionally; prefixing with
$DRY_RUN_CMDkeeps behavior consistent with other activation hooks during dry runs.Suggested adjustment
- ${pkgs.bash}/bin/bash ${./activate-remove-backups.sh} + $DRY_RUN_CMD ${pkgs.bash}/bin/bash ${./activate-remove-backups.sh}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hosts/darwin/default.nix` at line 47, The activation hook currently executes the cleanup script unconditionally; update the activation command that calls activate-remove-backups.sh to respect Home Manager dry-run mode by prefixing the invocation with the DRY_RUN_CMD variable (i.e., use $DRY_RUN_CMD before ${./activate-remove-backups.sh}) so the script runs only when not in dry-run, matching other activation hooks' behavior.home-manager/modules/tailscale/activate-install-service.sh (1)
50-59: Guard temp-file cleanup with a trap.
TEMP_SUDOERSis only removed at the end; any earlier failure leaves it behind. Add a trap right after creation.Suggested patch
TEMP_SUDOERS=$(mktemp) +cleanup() { + rm -f "$TEMP_SUDOERS" +} +trap cleanup EXIT echo "$SUDOERS_CONTENT" >"$TEMP_SUDOERS" @@ -rm -f "$TEMP_SUDOERS"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/modules/tailscale/activate-install-service.sh` around lines 50 - 59, TEMP_SUDOERS created by mktemp can be left behind on early exit; after creating TEMP_SUDOERS (and after ensuring mktemp succeeded) add a trap to remove it on EXIT/INT/TERM (e.g., trap 'rm -f "$TEMP_SUDOERS"' EXIT) so the temp file is always cleaned up, and keep the existing rm -f at the end or remove it if relying solely on the trap; reference TEMP_SUDOERS, mktemp, SUDOERS_CONTENT, SUDOERS_FILE and run_root_cmd when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Major comments:
In `@config/codex/activate.sh`:
- Around line 8-12: The script creates a hooks directory (~/.codex/hooks) but
then copies the hooks file to ~/.codex/hooks.json, so update the destination to
the directory: copy "$HOOKS_JSON" to ~/.codex/hooks/hooks.json and set its
permissions (chmod 644 ~/.codex/hooks/hooks.json); keep the CONFIG_TOML handling
as-is (CONFIG_TOML -> ~/.codex/config.toml) and ensure references to HOOKS_JSON
and the mkdir command align with the new path.
In `@config/gemini/activate.sh`:
- Line 9: Replace the interactive copy with a forced, non-interactive copy:
update the cp invocation that copies "$SETTINGS_JSON" to
"$HOME/.gemini/settings.json" by adding the -f flag (i.e., use cp -f) so the
activation script's copy operation is non-interactive and won't prompt for
confirmations.
In `@config/k3s/activate.sh`:
- Around line 18-23: The script currently only attempts installation when
SUDO_CMD is set; change the logic to: if SUDO_CMD is available use it, else if
the script is running as root (UID=0) run the commands without sudo, otherwise
print the warning; also make the file copy non-interactive by using cp -f and
ensure mkdir uses -p as before; update references to SUDO_CMD and the commands
that create /etc/rancher/k3s and copy "$HOME/.config/k3s/config.yaml" so they
run under sudo when present or directly when UID=0.
In `@config/omp/activate.sh`:
- Around line 7-10: The commands mkdir -p, cp -f, chmod and ln are using
unquoted HOME-derived paths (e.g. ~/.omp/agent, ~/.omp/agent/extensions and
$HOME/dotfiles/...) which can break if HOME contains spaces; change to use
explicit "$HOME" and quote all path arguments (e.g. mkdir -p "$HOME/.omp/agent"
"$HOME/.omp/agent/extensions", cp -f "$CONFIG_YML"
"$HOME/.omp/agent/config.yml", chmod 644 "$HOME/.omp/agent/config.yml", ln -sfn
"$HOME/dotfiles/node_modules/@oh-my-pi/swarm-extension"
"$HOME/.omp/agent/extensions/swarm-extension") so that CONFIG_YML and
HOME-derived paths are safely handled in mkdir, cp, chmod and ln.
In `@config/serena/activate.sh`:
- Line 11: Replace the interactive copy command with a forced non-interactive
copy: update the cp invocation that uses the variables SRC and DEST (cp "$SRC"
"$DEST") to include the -f flag so it becomes a forced, non-interactive
operation (use cp -f "$SRC" "$DEST"). Ensure the surrounding logic still handles
errors/exit codes as before.
In `@home-manager/activation/deploy-agenix-secret.sh`:
- Around line 20-25: The script currently lets rage write directly to "$DEST"
(subject to the current umask) before chmod 0600 is applied, risking brief
exposure; change the flow to decrypt into a securely-created temporary file in
the same directory (create with mktemp or set umask 077 and create temp with
restricted perms), ensure the temp is chmod 0600 before invoking RAGE_BIN -o
"$TMP", on success atomically move the temp into "$DEST" (mv -f) and remove the
temp on failure; reference the variables RAGE_BIN, IDENTITY_KEY, SECRET_FILE and
DEST and ensure cleanup/exit codes are preserved.
In `@home-manager/activation/import-gpg-key.sh`:
- Around line 11-27: The script writes decrypted key material to a predictable
path in GPG_TEMP_FILE and only removes it on successful import, risking leftover
plaintext; change to create a secure temporary file (use mktemp or similar) with
restrictive permissions for the decrypted output, write the RAGE_BIN -d -i
output to that temp file, and add a cleanup trap (trap 'rm -f "$TMPFILE"' EXIT
INT TERM) so the temp is removed on all exits/failures; update references from
GPG_TEMP_FILE to the new TMPFILE and ensure any early-exit paths (including
failed import) still trigger the cleanup before exiting.
In `@home-manager/modules/cargo-globals/default.nix`:
- Around line 28-29: The activation exports (RUSTUP_BIN and conditional
SYSTEMCTL_BIN) are not propagated into the
systemd.user.services.install-cargo-globals unit, so the user service runs
install-cargo-globals.sh without RUSTUP_BIN; update the service definition
(install-cargo-globals) to export the same environment into the unit—e.g., add
Environment=RUSTUP_BIN=${pkgs.rustup}/bin/rustup and, when !isDarwin,
Environment=SYSTEMCTL_BIN=${pkgs.systemd}/bin/systemctl (or point
EnvironmentFile to the same generated env file) so install-cargo-globals.sh sees
the same variables during login as during activation.
In `@home-manager/modules/cargo-globals/install-cargo-globals.sh`:
- Around line 11-15: The rustup network operations (invocations of "$RUSTUP_BIN"
toolchain install/default) must be moved to run after the existing offline guard
check and protected with timeouts so activation won’t hang on flaky or offline
systems; relocate the whole if [ -n "${RUSTUP_BIN:-}" ]; then ... fi block to
after the offline connectivity check, and wrap the rustup calls with a timeout
(e.g., timeout 10s) and preserve the || true fallback so failures don’t break
activation.
In `@home-manager/modules/tailscale/activate-install-service.sh`:
- Line 38: The cp invocations performed via run_root_cmd (specifically the calls
that copy "$NIX_SERVICE" to "$SERVICE_FILE" and the similar copy at the later
occurrence) are missing the non-interactive forced flag and can block
activation; update those calls to use cp -f (i.e., run_root_cmd cp -f
"$NIX_SERVICE" "$SERVICE_FILE") so root writes are forced and non-interactive,
ensuring both the first occurrence around the run_root_cmd copying
NIX_SERVICE->SERVICE_FILE and the second occurrence (the copy at the later
location) are changed consistently.
In `@home-manager/programs/fnm/activate.sh`:
- Around line 23-29: The code hardcodes "v22" when locating the default Node
version; update the logic to use the DEFAULT_VERSION variable instead: replace
the latest_v22 lookup with a variable that builds the pattern from
"$DEFAULT_VERSION" (e.g. latest_v_default=$(ls -d
"$FNM_DIR/node-versions/v${DEFAULT_VERSION}"* 2>/dev/null | head -1)), then
proceed to test [ -n "$latest_v_default" ] and [ -d
"$latest_v_default/installation/bin" ] and create the symlink from that path to
"$HOME/.local/bin/node"; also rename references to latest_v22 accordingly (and
ensure DEFAULT_VERSION is quoted when interpolated).
In `@home-manager/programs/fnm/default.nix`:
- Around line 24-30: The activation currently passes defaultVersion from
home.activation.fnmSetup but activate.sh still resolves the stable node symlink
by hardcoded glob v22*; update activate.sh to accept and use the passed
DEFAULT_VERSION (third argument from the activation call) when locating/creating
the ~/.local/bin/node stable symlink (i.e., replace the v22* glob resolution
with logic that uses the DEFAULT_VERSION argument or its derived path), ensuring
fnm default and the stable symlink remain in sync with the defaultVersion
provided by home.activation.fnmSetup.
In `@hosts/linux/activate-backup-files.sh`:
- Line 8: Change the interactive move commands to forced non-interactive moves
and quote the codex path: replace occurrences of mv "$HOME/$file"
"$HOME/$file.hm-backup" (both spots) with a non-interactive mv -f variant, and
update the codex directory reference to use the quoted form "$HOME/.codex" to
ensure path safety and comply with the repo policy on file operations.
In `@named-hosts/kyber/activate-ip-forwarding.sh`:
- Around line 34-37: The script currently only writes
/etc/sysctl.d/99-tailscale.conf if it does not exist; change it so activation
enforces the two sysctl settings every run by ensuring
/etc/sysctl.d/99-tailscale.conf contains both "net.ipv4.ip_forward=1" and
"net.ipv6.conf.all.forwarding=1" (either by atomically overwriting the file with
those two lines or by idempotently replacing/adding those keys), using the same
helper run_root_cmd and tee invocation patterns so the activation always repairs
stale or partial contents.
- Around line 29-33: The current if guard checks only
/proc/sys/net/ipv4/ip_forward and gates both sysctl calls, leaving IPv6
unchanged if IPv4 is already 1; update the logic to test and enable IPv4 and
IPv6 separately by reading /proc/sys/net/ipv4/ip_forward and
/proc/sys/net/ipv6/conf/all/forwarding, and call run_root_cmd sysctl -w
net.ipv4.ip_forward=1 only when the IPv4 value is not "1" and run_root_cmd
sysctl -w net.ipv6.conf.all.forwarding=1 only when the IPv6 value is not "1"
(preserve existing echo/logging behavior for each change).
In `@named-hosts/kyber/default.nix`:
- Line 36: The activate-backup-files hook calls ${pkgs.bash}/bin/bash
${./activate-backup-files.sh} without using the DRY_RUN_CMD prefix, so a
home-manager dry-run still executes the script (which contains mv operations)
and mutates files; change the invocation to include the DRY_RUN_CMD (i.e. prefix
the command with ${DRY_RUN_CMD}) so that when DRY_RUN_CMD is set the script run
is a no-op, ensuring activate-backup-files.sh is guarded consistently with the
other activation hooks that use DRY_RUN_CMD.
In `@named-hosts/matic/default.nix`:
- Around line 520-523: The deploy call to deploy-agenix-secret.sh is passing
../galactica/keys/id_ed25519.age but this host's agenix mapping and the kyber
host expect keys named id_github.age for ~/.ssh/id_ed25519_github; update the
secret argument passed to deploy-agenix-secret.sh (the third positional arg in
the invocation using DRY_RUN_CMD and ${pkgs.bash}/bin/bash) to reference the
consistent secret name (keys/id_github.age) so the agenix mapping for
id_ed25519_github matches across hosts.
In `@scripts/check-nix-inline-scripts.sh`:
- Line 49: The activation-block end matcher in the body_started conditional only
matches closing sequences that include a trailing semicolon (`'';`) which misses
cases where activation blocks end with a bare `''` and the `;` appears later
(e.g., inside `mkIf` wrappers); update the pattern used in the `body_started &&
/^[[:space:]]*'"''"';/` test to also accept a bare `''` closer (and optional
following `;`) so the `body_started` branch correctly detects the end of
`home.activation` blocks; locate the `body_started` conditional and its regex in
scripts/check-nix-inline-scripts.sh and broaden the regex to match `''` with or
without an immediate semicolon.
In `@spec/activate_config_spec.sh`:
- Around line 80-110: Add assertions to the two spec blocks that verify the
scripts use non-interactive cp flags: for the gemini block (where
SCRIPT="$PWD/config/gemini/activate.sh") add a test that greps for 'cp -f'
(similar style to the existing grep tests), and for the serena block (where
SCRIPT="$PWD/config/serena/activate.sh") add the same grep assertion for 'cp
-f'; ensure the new It/When run lines mirror the existing pattern so the spec
fails if plain `cp` is used instead of `cp -f`.
In `@spec/activate_fnm_spec.sh`:
- Around line 35-42: The current spec uses broad grep patterns that can match
comments; tighten assertions to search for the actual executable lines by
matching the literal command invocations such as "$FNM_BIN" install and
"$FNM_BIN" default (or the exact line in activate.sh that invokes FNM) instead
of generic 'install' or 'fnm.*default'; update the tests in
spec/activate_fnm_spec.sh to grep for the quoted command pattern (including
"$FNM_BIN" and the subcommand) or anchor the line start so the test only passes
when the real install/default command is present.
In `@spec/activate_hosts_spec.sh`:
- Around line 43-72: The activation script hosts/linux/activate-backup-files.sh
is using plain mv (lines ~5-13) which can block on confirmation; update every mv
invocation in that script to use mv -f (non-interactive) and similarly ensure
any cp/rm calls there use their non-interactive flags (cp -f, rm -f or rm -rf)
so file ops never prompt; verify the updated commands still preserve the
existing behavior and adjust the spec if needed to assert the presence of '-f'
in the generated activation script.
---
Minor comments:
In `@home-manager/programs/neovim/default.nix`:
- Around line 47-50: In the home.activation.buildNvimNativePlugins shell block,
the unquoted ${packDir} can expand to a path with spaces and break the argv
passed to ${./activate-build-plugins.sh}; update the invocation inside the
heredoc so the script is called with "${packDir}" (and optionally quote
${libExt} too) to ensure the pack directory is passed as a single argument;
locate the block using home.activation.buildNvimNativePlugins and the
activate-build-plugins.sh invocation to apply the change.
In `@hosts/linux/default.nix`:
- Line 39: The delegated activation script hosts/linux/activate-backup-files.sh
currently uses interactive-prone move commands; update every plain mv invocation
in that script to use mv -f so the activation is non-interactive (replace
occurrences of "mv " with "mv -f " for the backup/move operations), ensuring all
backup/move paths handled by the script (including any within functions or
loops) use the -f flag.
In `@named-hosts/kyber/activate-backup-files.sh`:
- Line 8: The mv invocation in activate-backup-files.sh uses interactive
defaults which can hang automation; update the move command that references
"$HOME/$file" -> "$HOME/$file.hm-backup" to use the non-interactive forced flag
(mv -f) so the rename proceeds without prompts in scripts and CI.
---
Nitpick comments:
In `@config/openclaw/hydrate.sh`:
- Line 9: Remove the redundant directory creation: since STATE_DIR is already
created earlier in hydrate.sh with mkdir -p "$STATE_DIR", delete the later
duplicate mkdir -p "$STATE_DIR" invocation (the second occurrence) so the script
only creates STATE_DIR once; ensure no other logic depends on that repeated call
and preserve surrounding commands as-is.
In `@home-manager/modules/tailscale/activate-install-service.sh`:
- Around line 50-59: TEMP_SUDOERS created by mktemp can be left behind on early
exit; after creating TEMP_SUDOERS (and after ensuring mktemp succeeded) add a
trap to remove it on EXIT/INT/TERM (e.g., trap 'rm -f "$TEMP_SUDOERS"' EXIT) so
the temp file is always cleaned up, and keep the existing rm -f at the end or
remove it if relying solely on the trap; reference TEMP_SUDOERS, mktemp,
SUDOERS_CONTENT, SUDOERS_FILE and run_root_cmd when making the change.
In `@hosts/darwin/default.nix`:
- Line 47: The activation hook currently executes the cleanup script
unconditionally; update the activation command that calls
activate-remove-backups.sh to respect Home Manager dry-run mode by prefixing the
invocation with the DRY_RUN_CMD variable (i.e., use $DRY_RUN_CMD before
${./activate-remove-backups.sh}) so the script runs only when not in dry-run,
matching other activation hooks' behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09a03d4b-7dcc-4149-9a6b-51200b35611b
📒 Files selected for processing (62)
config/claude/activate.shconfig/claude/default.nixconfig/codex/activate.shconfig/codex/default.nixconfig/cursor/activate.shconfig/cursor/default.nixconfig/gemini/activate.shconfig/gemini/default.nixconfig/k3s/activate.shconfig/k3s/default.nixconfig/omp/activate.shconfig/omp/default.nixconfig/openclaw/default.nixconfig/openclaw/hydrate.shconfig/serena/activate.shconfig/serena/default.nixhome-manager/activation/deploy-agenix-secret.shhome-manager/activation/ensure-directory.shhome-manager/activation/import-gpg-key.shhome-manager/modules/bin-shells/activate.shhome-manager/modules/bin-shells/default.nixhome-manager/modules/cargo-globals/default.nixhome-manager/modules/cargo-globals/install-cargo-globals.shhome-manager/modules/npm-globals/default.nixhome-manager/modules/npm-globals/install-npm-globals.shhome-manager/modules/openclaw/activate.shhome-manager/modules/openclaw/default.nixhome-manager/modules/paperclip/activate.shhome-manager/modules/paperclip/default.nixhome-manager/modules/tailscale/activate-create-dirs.shhome-manager/modules/tailscale/activate-install-service.shhome-manager/modules/tailscale/default.nixhome-manager/modules/uv-globals/default.nixhome-manager/modules/uv-globals/install-uv-globals.shhome-manager/programs/fnm/activate.shhome-manager/programs/fnm/default.nixhome-manager/programs/neovim/activate-build-plugins.shhome-manager/programs/neovim/activate-copy-pack-lock.shhome-manager/programs/neovim/default.nixhosts/darwin/activate-remove-backups.shhosts/darwin/default.nixhosts/linux/activate-backup-files.shhosts/linux/default.nixnamed-hosts/kyber/activate-backup-files.shnamed-hosts/kyber/activate-ip-forwarding.shnamed-hosts/kyber/default.nixnamed-hosts/matic/default.nixscripts/check-nix-inline-scripts.shspec/activate_bin_shells_spec.shspec/activate_config_spec.shspec/activate_fnm_spec.shspec/activate_hosts_spec.shspec/activate_k3s_spec.shspec/activate_kyber_spec.shspec/activate_neovim_spec.shspec/activate_paperclip_openclaw_spec.shspec/activate_tailscale_spec.shspec/activation_shared_spec.shspec/cargo_globals_spec.shspec/coverage_spec.shspec/npm_globals_spec.shspec/uv_globals_spec.sh
💤 Files with no reviewable changes (1)
- config/openclaw/default.nix
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-nix-inline-scripts.sh">
<violation number="1" location="scripts/check-nix-inline-scripts.sh:71">
P2: The new `^\$\{` allowlist is too broad and can let direct inline activation commands bypass linting if they start with Nix interpolation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Replace builtins.toJSON inline generation with tracked config.json files and activation scripts. git_path and home directory are injected at activation time via jq/sed.
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/obsidian/default.nix">
<violation number="1" location="config/obsidian/default.nix:17">
P2: Quote `config.home.homeDirectory` in the activation command to prevent shell argument splitting for paths with spaces.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 33 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-nix-inline-scripts.sh">
<violation number="1" location="scripts/check-nix-inline-scripts.sh:60">
P2: Iterating `split()` results with `for (i in lines)` makes validation order-unsafe; this can falsely flag valid multi-line bash delegations depending on awk key iteration order.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| saw_content = 0 | ||
| split(body, lines, "\n") | ||
|
|
||
| for (i in lines) { |
Contributor
There was a problem hiding this comment.
P2: Iterating split() results with for (i in lines) makes validation order-unsafe; this can falsely flag valid multi-line bash delegations depending on awk key iteration order.
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 60:
<comment>Iterating `split()` results with `for (i in lines)` makes validation order-unsafe; this can falsely flag valid multi-line bash delegations depending on awk key iteration order.</comment>
<file context>
@@ -29,66 +29,74 @@ python_violations=$(grep -rn \
+ saw_content = 0
+ split(body, lines, "\n")
+
+ for (i in lines) {
+ line = trim(lines[i])
+
</file context>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
config/git-ai/default.nix (1)
3-5: Minor DAG access inconsistency with other modules.This module uses
lib.hm.dag.entryAfterwhileconfig/obsidian/default.nixusesconfig.lib.dag.entryAfter. Both work, butconfig.lib.dagis the more canonical Home Manager pattern. Consider aligning for consistency across activation modules.♻️ Optional consistency fix
- home.activation.gitAiConfig = lib.hm.dag.entryAfter [ "writeBoundary" ] '' + home.activation.gitAiConfig = config.lib.dag.entryAfter [ "writeBoundary" ] ''Note: This would also require adding
configto the module arguments:{ config, pkgs, lib, ... }:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/git-ai/default.nix` around lines 3 - 5, The module uses lib.hm.dag.entryAfter for the activation DAG but other modules use the canonical config.lib.dag.entryAfter; change the invocation to config.lib.dag.entryAfter and update the module argument list to include config (e.g. add config to the parameter tuple) so the entryAfter call resolves from config.lib.dag instead of lib.hm.dag, keeping the rest of the activation string unchanged.home-manager/modules/tailscale/default.nix (1)
210-218: LGTM! Consider adding a platform guard (optional).The refactoring correctly delegates systemd service installation to the external script. The context snippets confirm the script properly handles sudo/doas detection and systemd operations.
One optional consideration:
installSystemServicedefaults totruebut the underlying script targets systemd, which doesn't exist on macOS/Darwin. Users on Darwin would need to explicitly setinstallSystemService = false. If Darwin users are expected, you could wrap the activation with a platform check or add early exit logic in the script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@home-manager/modules/tailscale/default.nix` around lines 210 - 218, Wrap the existing activation that runs activate-install-service.sh in a Darwin/platform guard so it doesn't run on macOS where systemd isn't present: modify the mkIf condition around home.activation.installTailscaleService (which currently uses cfg.installSystemService) to also check the platform (e.g., config.system.isLinux or !config.system.isDarwin) or alternatively add an early-return guard inside activate-install-service.sh that no-ops on Darwin; ensure references to tailscaledServiceFile and config.home.homeDirectory remain passed unchanged when the guard allows execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 421: The nix-build target currently depends on nix-trust which makes
ordinary builds mutate system state; remove nix-trust from the nix-build
prerequisite list so nix-build depends only on nix-connect (i.e., change the
target from "nix-build: nix-connect nix-trust" to "nix-build: nix-connect"),
leaving the standalone nix-trust target unchanged for explicit privileged
operations; update any comments or README references if they mention nix-build
invoking trust changes.
In `@named-hosts/matic/default.nix`:
- Around line 530-536: The import helper leaves the decrypted private key under
${AGENIX_DIR} when gpg --import fails; modify import-gpg-key.sh so the decrypted
file is created in a temporary path (or the existing AGENIX_DIR filename) and
ensure it is always removed on exit by installing a trap handler (trap 'rm -f
"$DECRYPTED_FILE"' EXIT) that runs regardless of success, and explicitly call
the cleanup before any early returns/errors after the gpg --import step; update
references in the script (the decryption step, the variable that holds the
decrypted filename, and the gpg --import invocation) to use the temp variable so
the trap can reliably remove the plaintext file.
In `@scripts/check-nix-inline-scripts.sh`:
- Around line 69-70: The current whitelist skips any line that starts with
${lib.optionalString...} without validating the interpolated payload, allowing
inline shell like ${lib.optionalString cond ''mkdir -p "$HOME/.cache"''}; update
the check so when a line matches /^\$\{lib\.optionalString/ you parse the
interpolated string argument (capture the quoted payload inside the
${lib.optionalString ...} wrapper) and only allow the line to continue if that
payload is empty or contains no shell metacharacters (for example: ';', '&',
'|', '`', '$(', '>', '<') — otherwise treat it as disallowed inline shell in
home.activation. Ensure the regex targets the quoted payload and applies the
metacharacter check before continuing.
- Around line 55-80: The loop in block_has_violation relies on processing lines
in original order but uses "for (i in lines)" which iterates in arbitrary order
in AWK; change to an index-based loop by capturing the split count (n =
split(body, lines, "\n")) and iterate for (i = 1; i <= n; i++) so the code
examining is_bash_delegate(...) and subsequent continuation checks
(is_single_arg(...)) see the bash delegation before its args; replace the
existing for (i in lines) construct accordingly while keeping the existing
body/trim logic and variables saw_bash_call and saw_content.
In `@spec/coverage_spec.sh`:
- Around line 416-417: The coverage spec is missing explicit assertions for the
two scripts listed in covered_scripts; add "has spec file for" assertions for
scripts/sync-codex-security.sh and scripts/sync-rtk-rewrite.sh in the same "has
spec file for …" section of spec/coverage_spec.sh so the test fails if either
corresponding spec file is removed (refer to the existing pattern used for other
scripts in that section to match wording and placement).
---
Nitpick comments:
In `@config/git-ai/default.nix`:
- Around line 3-5: The module uses lib.hm.dag.entryAfter for the activation DAG
but other modules use the canonical config.lib.dag.entryAfter; change the
invocation to config.lib.dag.entryAfter and update the module argument list to
include config (e.g. add config to the parameter tuple) so the entryAfter call
resolves from config.lib.dag instead of lib.hm.dag, keeping the rest of the
activation string unchanged.
In `@home-manager/modules/tailscale/default.nix`:
- Around line 210-218: Wrap the existing activation that runs
activate-install-service.sh in a Darwin/platform guard so it doesn't run on
macOS where systemd isn't present: modify the mkIf condition around
home.activation.installTailscaleService (which currently uses
cfg.installSystemService) to also check the platform (e.g.,
config.system.isLinux or !config.system.isDarwin) or alternatively add an
early-return guard inside activate-install-service.sh that no-ops on Darwin;
ensure references to tailscaledServiceFile and config.home.homeDirectory remain
passed unchanged when the guard allows execution.
🪄 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: 8adad49e-3a97-4bb1-99ae-f4a6639956d6
📒 Files selected for processing (38)
Makefileconfig/claude/default.nixconfig/codex/default.nixconfig/cursor/default.nixconfig/gemini/default.nixconfig/git-ai/activate.shconfig/git-ai/default.nixconfig/k3s/default.nixconfig/obsidian/activate.shconfig/obsidian/default.nixconfig/obsidian/obsidian.jsonconfig/omp/default.nixconfig/openclaw/default.nixconfig/serena/default.nixhome-manager/modules/bin-shells/default.nixhome-manager/modules/cargo-globals/default.nixhome-manager/modules/local-binaries/default.nixhome-manager/modules/npm-globals/default.nixhome-manager/modules/openclaw/default.nixhome-manager/modules/paperclip/default.nixhome-manager/modules/tailscale/default.nixhome-manager/modules/uv-globals/default.nixhome-manager/packages/default.nixhome-manager/programs/fnm/default.nixhome-manager/programs/neovim/default.nixhosts/darwin/default.nixhosts/linux/default.nixnamed-hosts/kyber/default.nixnamed-hosts/matic/default.nixscripts/check-nix-inline-scripts.shspec/activate_config_spec.shspec/activate_fnm_spec.shspec/activate_kyber_spec.shspec/activate_neovim_spec.shspec/activate_paperclip_openclaw_spec.shspec/activation_shared_spec.shspec/check_nix_inline_scripts_spec.shspec/coverage_spec.sh
✅ Files skipped from review due to trivial changes (6)
- home-manager/packages/default.nix
- home-manager/modules/local-binaries/default.nix
- config/obsidian/obsidian.json
- spec/activate_fnm_spec.sh
- spec/activate_neovim_spec.sh
- spec/activation_shared_spec.sh
🚧 Files skipped from review as they are similar to previous changes (19)
- config/openclaw/default.nix
- home-manager/modules/npm-globals/default.nix
- config/cursor/default.nix
- config/claude/default.nix
- config/serena/default.nix
- config/codex/default.nix
- home-manager/modules/paperclip/default.nix
- config/k3s/default.nix
- home-manager/modules/openclaw/default.nix
- hosts/linux/default.nix
- hosts/darwin/default.nix
- home-manager/modules/bin-shells/default.nix
- config/omp/default.nix
- home-manager/modules/cargo-globals/default.nix
- spec/activate_paperclip_openclaw_spec.sh
- home-manager/programs/fnm/default.nix
- spec/activate_config_spec.sh
- named-hosts/kyber/default.nix
- spec/activate_kyber_spec.sh
|
|
||
| .PHONY: nix-build | ||
| nix-build: nix-connect ## Build Nix configuration. | ||
| nix-build: nix-connect nix-trust ## Build Nix configuration. |
There was a problem hiding this comment.
Keep nix-build read-only.
Adding nix-trust here means a normal build can now rewrite /etc/nix/nix.conf and restart nix-daemon via the nix-trust target at Lines 377-385. That turns make build/make install into a privileged, system-mutating step and will fail for users who can still run nix build but cannot or should not modify daemon trust on every build.
Suggested change
-nix-build: nix-connect nix-trust ## Build Nix configuration.
+nix-build: nix-connect ## Build Nix configuration.📝 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
| nix-build: nix-connect nix-trust ## Build Nix configuration. | |
| nix-build: nix-connect ## Build Nix configuration. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 421, The nix-build target currently depends on nix-trust
which makes ordinary builds mutate system state; remove nix-trust from the
nix-build prerequisite list so nix-build depends only on nix-connect (i.e.,
change the target from "nix-build: nix-connect nix-trust" to "nix-build:
nix-connect"), leaving the standalone nix-trust target unchanged for explicit
privileged operations; update any comments or README references if they mention
nix-build invoking trust changes.
Comment on lines
+530
to
+536
| $DRY_RUN_CMD ${pkgs.bash}/bin/bash "${../../home-manager/activation/import-gpg-key.sh}" \ | ||
| "${config.home.homeDirectory}/dotfiles/named-hosts/galactica/keys/gpg.age" \ | ||
| "${config.home.homeDirectory}/.ssh/id_ed25519" \ | ||
| "${config.home.homeDirectory}/.config/agenix" \ | ||
| "${pkgs.rage}/bin/rage" \ | ||
| "${pkgs.gnupg}/bin/gpg" \ | ||
| "C2E97FCFF482925D" |
There was a problem hiding this comment.
Avoid leaving the decrypted GPG key on disk if import fails.
This activation now depends on home-manager/activation/import-gpg-key.sh, and that helper decrypts the private key to a fixed file under ${AGENIX_DIR} before running gpg --import. If the import errors, the plaintext key is never removed because cleanup only happens on the success path.
Suggested fix in home-manager/activation/import-gpg-key.sh
set -euo pipefail
GPG_SECRET_FILE="$1"
IDENTITY_KEY="$2"
AGENIX_DIR="$3"
RAGE_BIN="$4"
GPG_BIN="$5"
KEY_FINGERPRINT="$6"
-GPG_TEMP_FILE="$AGENIX_DIR/gpg.key"
mkdir -p "$AGENIX_DIR"
+GPG_TEMP_FILE="$(mktemp "$AGENIX_DIR/gpg.XXXXXX")"
+cleanup() {
+ rm -f "$GPG_TEMP_FILE"
+}
+trap cleanup EXIT
if [[ ! -f $GPG_SECRET_FILE ]]; then
exit 0
fi
if "$GPG_BIN" --list-secret-keys 2>/dev/null | grep -q "$KEY_FINGERPRINT"; then
exit 0
fi
echo "Importing GPG key from agenix..."
if "$RAGE_BIN" -d -i "$IDENTITY_KEY" -o "$GPG_TEMP_FILE" "$GPG_SECRET_FILE" 2>/dev/null; then
"$GPG_BIN" --batch --import "$GPG_TEMP_FILE" 2>/dev/null
- rm -f "$GPG_TEMP_FILE"
echo "GPG key imported successfully"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@named-hosts/matic/default.nix` around lines 530 - 536, The import helper
leaves the decrypted private key under ${AGENIX_DIR} when gpg --import fails;
modify import-gpg-key.sh so the decrypted file is created in a temporary path
(or the existing AGENIX_DIR filename) and ensure it is always removed on exit by
installing a trap handler (trap 'rm -f "$DECRYPTED_FILE"' EXIT) that runs
regardless of success, and explicitly call the cleanup before any early
returns/errors after the gpg --import step; update references in the script (the
decryption step, the variable that holds the decrypted filename, and the gpg
--import invocation) to use the temp variable so the trap can reliably remove
the plaintext file.
Comment on lines
+55
to
+80
| function block_has_violation( i, line, saw_bash_call, saw_content) { | ||
| saw_bash_call = 0 | ||
| saw_content = 0 | ||
| split(body, lines, "\n") | ||
|
|
||
| for (i in lines) { | ||
| line = trim(lines[i]) | ||
|
|
||
| if (line == "" || line ~ /^#/) continue | ||
| saw_content = 1 | ||
|
|
||
| if (line ~ /&&|;|`|[<>]/) return 1 | ||
| if (line ~ /\|/ && line !~ /[[:space:]]\|\|[[:space:]]+true[[:space:]]*\\?$/) return 1 | ||
|
|
||
| if (line ~ /^export /) continue | ||
| if (line ~ /^\$\{lib\.optionalString/) continue | ||
| if (is_bash_delegate(line)) { | ||
| saw_bash_call = 1 | ||
| continue | ||
| } | ||
| if (saw_bash_call && is_single_arg(line)) continue | ||
|
|
||
| return 1 | ||
| } | ||
|
|
||
| return saw_content && !saw_bash_call |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/check-nix-inline-scripts.sh | sed -n '30,110p'Repository: shunkakinoki/dotfiles
Length of output: 3002
Iterate activation lines in index order, not for (i in lines).
This logic depends on seeing the bash delegation before its continuation arguments, but awk does not guarantee iteration order for for (i in array). On a different awk implementation, the same activation block can be accepted or rejected inconsistently.
Suggested change
- split(body, lines, "\n")
-
- for (i in lines) {
+ line_count = split(body, lines, "\n")
+
+ for (i = 1; i <= line_count; i++) {
line = trim(lines[i])📝 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
| function block_has_violation( i, line, saw_bash_call, saw_content) { | |
| saw_bash_call = 0 | |
| saw_content = 0 | |
| split(body, lines, "\n") | |
| for (i in lines) { | |
| line = trim(lines[i]) | |
| if (line == "" || line ~ /^#/) continue | |
| saw_content = 1 | |
| if (line ~ /&&|;|`|[<>]/) return 1 | |
| if (line ~ /\|/ && line !~ /[[:space:]]\|\|[[:space:]]+true[[:space:]]*\\?$/) return 1 | |
| if (line ~ /^export /) continue | |
| if (line ~ /^\$\{lib\.optionalString/) continue | |
| if (is_bash_delegate(line)) { | |
| saw_bash_call = 1 | |
| continue | |
| } | |
| if (saw_bash_call && is_single_arg(line)) continue | |
| return 1 | |
| } | |
| return saw_content && !saw_bash_call | |
| function block_has_violation( i, line, saw_bash_call, saw_content) { | |
| saw_bash_call = 0 | |
| saw_content = 0 | |
| line_count = split(body, lines, "\n") | |
| for (i = 1; i <= line_count; i++) { | |
| line = trim(lines[i]) | |
| if (line == "" || line ~ /^#/) continue | |
| saw_content = 1 | |
| if (line ~ /&&|;|`|[<>]/) return 1 | |
| if (line ~ /\|/ && line !~ /[[:space:]]\|\|[[:space:]]+true[[:space:]]*\\?$/) return 1 | |
| if (line ~ /^export /) continue | |
| if (line ~ /^\$\{lib\.optionalString/) continue | |
| if (is_bash_delegate(line)) { | |
| saw_bash_call = 1 | |
| continue | |
| } | |
| if (saw_bash_call && is_single_arg(line)) continue | |
| return 1 | |
| } | |
| return saw_content && !saw_bash_call |
🤖 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 55 - 80, The loop in
block_has_violation relies on processing lines in original order but uses "for
(i in lines)" which iterates in arbitrary order in AWK; change to an index-based
loop by capturing the split count (n = split(body, lines, "\n")) and iterate for
(i = 1; i <= n; i++) so the code examining is_bash_delegate(...) and subsequent
continuation checks (is_single_arg(...)) see the bash delegation before its
args; replace the existing for (i in lines) construct accordingly while keeping
the existing body/trim logic and variables saw_bash_call and saw_content.
Comment on lines
+69
to
+70
| if (line ~ /^export /) continue | ||
| if (line ~ /^\$\{lib\.optionalString/) continue |
There was a problem hiding this comment.
Only allow lib.optionalString exports here.
if (line ~ /^\$\{lib\.optionalString/) continue is too broad: ${lib.optionalString cond ''mkdir -p "$HOME/.cache"''} would currently pass even though this checker is supposed to ban inline shell in home.activation blocks. The whitelist needs to inspect the interpolated payload, not just the wrapper.
Suggested change
+ function is_optional_export(line) {
+ return line ~ /^\$\{lib\.optionalString/ && line ~ /''export /
+ }
+
function block_has_violation( i, line, saw_bash_call, saw_content) {
@@
- if (line ~ /^\$\{lib\.optionalString/) continue
+ if (is_optional_export(line)) continue📝 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
| if (line ~ /^export /) continue | |
| if (line ~ /^\$\{lib\.optionalString/) continue | |
| function is_optional_export(line) { | |
| return line ~ /^\$\{lib\.optionalString/ && line ~ /''export / | |
| } | |
| function block_has_violation( i, line, saw_bash_call, saw_content) { | |
| if (line ~ /^export /) continue | |
| if (is_optional_export(line)) continue |
🤖 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 69 - 70, The current
whitelist skips any line that starts with ${lib.optionalString...} without
validating the interpolated payload, allowing inline shell like
${lib.optionalString cond ''mkdir -p "$HOME/.cache"''}; update the check so when
a line matches /^\$\{lib\.optionalString/ you parse the interpolated string
argument (capture the quoted payload inside the ${lib.optionalString ...}
wrapper) and only allow the line to continue if that payload is empty or
contains no shell metacharacters (for example: ';', '&', '|', '`', '$(', '>',
'<') — otherwise treat it as disallowed inline shell in home.activation. Ensure
the regex targets the quoted payload and applies the metacharacter check before
continuing.
Comment on lines
+416
to
+417
| scripts/sync-codex-security.sh | ||
| scripts/sync-rtk-rewrite.sh |
There was a problem hiding this comment.
Add the matching spec-file assertions for these two scripts.
scripts/sync-codex-security.sh and scripts/sync-rtk-rewrite.sh are back in covered_scripts, but the earlier “has spec file for …” section still has no explicit assertion for either one. If one of their spec files is deleted, this coverage spec will no longer catch it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/coverage_spec.sh` around lines 416 - 417, The coverage spec is missing
explicit assertions for the two scripts listed in covered_scripts; add "has spec
file for" assertions for scripts/sync-codex-security.sh and
scripts/sync-rtk-rewrite.sh in the same "has spec file for …" section of
spec/coverage_spec.sh so the test fails if either corresponding spec file is
removed (refer to the existing pattern used for other scripts in that section to
match wording and placement).
…hell-check command
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
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
1 participant
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
home.activationblocks in.nixfiles to external.shscripts invoked via${pkgs.bash}/bin/bash.shfiles + 3 shared scripts inhome-manager/activation/(ensure-directory, deploy-agenix-secret, import-gpg-key).shscripts viaSYSTEMCTL_BINenv varcheck-nix-inline-scripts.shto lint activation blocks (not justwriteScript)Test plan
make shell-test- 1195 examples, 0 failuresmake shell-inline-check- passesnix eval .#darwinConfigurations.aarch64-darwin.system --impure- evaluates successfullySummary by cubic
Moved all inline
home.activationshell to external.shfiles run via${pkgs.bash}/bin/bashfor clearer, testable activation and stronger ShellCheck coverage. Also tightened the inline-script checker, addedact, switched select configs to tracked JSON with activation-time injection, and improved the ShellCheck runner and ignores.Refactors
/binshell symlinks, k3s config sync, Neovim native builds + pack-lock copy,fnmNode setup, OpenClaw/Paperclip dirs, and app configs (claude,cursor,codex,gemini,serena,omp,git-ai,obsidian).git-aiinjectsgit_pathviajq/sed; Obsidian usesobsidian.jsonwith home path injected viased.ensure-directory.sh,deploy-agenix-secret.sh,import-gpg-key.sh; moved host-specific steps into scripts (Linux/Darwin backups, Kyber IP forwarding). Boot checks moved into scripts viaSYSTEMCTL_BIN;rustupsetup moved intoinstall-cargo-globals.shviaRUSTUP_BIN.nix-buildnow depends onnix-trust; addedactto packages..shfiles viagit ls-files; updated.gitignoreto ignoredotagentsanddotagents/private-skills.Bug Fixes
home.activationblocks unless delegating to${pkgs.bash}/bin/bash;$DRY_RUN_CMDno longer bypasses the check.Written for commit 860d305. Summary will update on new commits.