Merge pull request #329 from naturallaw777/copilot/fix-nix-build-regression

Fix `sovran-hosts-update` ShellCheck build regression from `writeShellApplication`

app/tests/test_local_domain_loopback_nix.py

@@ -0,0 +1,193 @@
+"""Structural regression tests for modules/core/local-domain-loopback.nix.
+
+Verifies that the sovran-hosts-update helper:
+  - is built as a pkgs.writeShellApplication with explicit runtimeInputs
+    (gawk, gnugrep, coreutils);
+  - uses ``lib.getExe hostsUpdateScript`` for both the systemd ExecStart and
+    the activation script so that both contexts share the same Nix-store
+    executable;
+  - does NOT point ExecStart at the raw /etc path;
+  - includes element-calling in the supported domain list;
+  - uses ``awk -v`` for safe marker-variable passing rather than interpolating
+    marker text directly into the awk program;
+  - retains the domain validation regex (idempotency / injection prevention);
+  - exposes /etc/sovran-hosts-update.sh as a symlink via ``source =`` (not a
+    second raw ``text =`` body);
+  - does NOT rely on environment.systemPackages for the helper's dependencies.
+"""
+
+import re
+import shutil
+import subprocess
+import unittest
+from pathlib import Path
+
+NIX_STRING_INDENT = 6
+REPO_ROOT = Path(__file__).resolve().parents[2]
+FLAKE_SOURCE = (REPO_ROOT / "flake.nix").read_text()
+PRIMARY_NIXOS_CONFIGURATION_MATCH = re.search(
+    r"nixosConfigurations\.([A-Za-z0-9_-]+)\s*=",
+    FLAKE_SOURCE,
+)
+if PRIMARY_NIXOS_CONFIGURATION_MATCH is None:
+    raise RuntimeError("Could not determine the primary nixosConfigurations entry from flake.nix")
+PRIMARY_NIXOS_CONFIGURATION = PRIMARY_NIXOS_CONFIGURATION_MATCH.group(1)
+HELPER_BUILD_ATTR = (
+    f'.#nixosConfigurations.{PRIMARY_NIXOS_CONFIGURATION}.config.environment.etc.'
+    '"sovran-hosts-update.sh".source'
+)
+NIX_FILE = (
+    REPO_ROOT
+    / "modules"
+    / "core"
+    / "local-domain-loopback.nix"
+)
+
+
+class LocalDomainLoopbackNixStructureTests(unittest.TestCase):
+    def setUp(self):
+        self.source = NIX_FILE.read_text()
+
+    def _helper_script(self) -> str:
+        start = self.source.index("text = ''") + len("text = ''")
+        end = self.source.index("    '';", start)
+        return "\n".join(
+            line[NIX_STRING_INDENT:]
+            if line.startswith(" " * NIX_STRING_INDENT)
+            else line
+            for line in self.source[start:end].splitlines()
+        ).lstrip("\n")
+
+    # ── writeShellApplication and explicit runtimeInputs ────────────────────
+
+    def test_uses_write_shell_application(self):
+        self.assertIn("pkgs.writeShellApplication", self.source)
+
+    def test_runtime_inputs_includes_coreutils(self):
+        self.assertIn("pkgs.coreutils", self.source)
+
+    def test_runtime_inputs_includes_gawk(self):
+        self.assertIn("pkgs.gawk", self.source)
+
+    def test_runtime_inputs_includes_gnugrep(self):
+        self.assertIn("pkgs.gnugrep", self.source)
+
+    def test_runtime_inputs_block_present(self):
+        self.assertIn("runtimeInputs", self.source)
+
+    # ── Both execution paths use lib.getExe ─────────────────────────────────
+
+    def test_exec_start_uses_lib_get_exe(self):
+        """systemd ExecStart must reference the Nix-store executable."""
+        self.assertIn("ExecStart = lib.getExe hostsUpdateScript", self.source)
+
+    def test_activation_script_uses_lib_get_exe(self):
+        """Activation text must call the same Nix-store executable."""
+        self.assertIn("${lib.getExe hostsUpdateScript}", self.source)
+
+    def test_exec_start_does_not_point_to_etc_path(self):
+        """ExecStart must NOT use the raw /etc path (which lacks a deterministic PATH)."""
+        self.assertNotIn('ExecStart = "/etc/sovran-hosts-update.sh"', self.source)
+
+    # ── /etc symlink uses source =, not a second text = body ────────────────
+
+    def test_etc_entry_uses_source_not_text(self):
+        """The /etc/sovran-hosts-update.sh entry must be a symlink (source =),
+        not a second raw script body (text =)."""
+        self.assertIn(
+            'environment.etc."sovran-hosts-update.sh".source', self.source
+        )
+
+    def test_etc_source_points_to_get_exe(self):
+        self.assertIn(
+            'environment.etc."sovran-hosts-update.sh".source = lib.getExe hostsUpdateScript',
+            self.source,
+        )
+
+    # ── element-calling domain is supported ─────────────────────────────────
+
+    def test_element_calling_domain_key_present(self):
+        self.assertIn("element-calling", self.source)
+
+    # ── Robust awk -v variable passing ──────────────────────────────────────
+
+    def test_awk_uses_dash_v_for_begin_marker(self):
+        """awk must receive the begin marker via -v, not by shell interpolation."""
+        self.assertIn('awk -v begin=', self.source)
+
+    def test_awk_uses_dash_v_for_end_marker(self):
+        self.assertIn('-v end=', self.source)
+
+    def test_awk_does_not_interpolate_marker_into_program(self):
+        """The old pattern interpolated $BEGIN_MARKER directly into the awk source."""
+        self.assertNotIn('/$BEGIN_MARKER', self.source)
+        self.assertNotIn('/$END_MARKER', self.source)
+
+    # ── Domain validation ────────────────────────────────────────────────────
+
+    def test_domain_validation_regex_present(self):
+        """The hostname validation regex must still be present for injection prevention."""
+        self.assertIn("grep -qE", self.source)
+        self.assertIn("[a-zA-Z0-9]", self.source)
+
+    def test_invalid_domain_warning_present(self):
+        self.assertIn("skipping invalid domain value", self.source)
+
+    # ── No environment.systemPackages reliance ───────────────────────────────
+
+    def test_no_environment_system_packages_for_helper(self):
+        """The helper's tools are declared via runtimeInputs; the module must
+        not add them to environment.systemPackages."""
+        self.assertNotIn("environment.systemPackages", self.source)
+
+    # ── Idempotency: existing Sovran block is removed before rewriting ───────
+
+    def test_existing_block_removal_logic_present(self):
+        """awk strip of the managed block must be present for idempotency."""
+        self.assertIn("skip=1", self.source)
+        self.assertIn("skip=0", self.source)
+
+    def test_managed_block_uses_grouped_append_redirect(self):
+        self.assertIn('} >> "$TMP"', self.source)
+        self.assertEqual(self.source.count('>> "$TMP"'), 1)
+
+    def test_helper_script_passes_shellcheck(self):
+        shellcheck = shutil.which("shellcheck")
+        if shellcheck is None:
+            self.skipTest("shellcheck is not installed")
+        proc = subprocess.run(
+            [shellcheck, "-s", "bash", "-"],
+            input=self._helper_script(),
+            text=True,
+            capture_output=True,
+            check=False,
+        )
+        output = proc.stdout + proc.stderr
+        self.assertEqual(proc.returncode, 0, output)
+
+    def test_helper_derivation_builds_when_nix_available(self):
+        nix = shutil.which("nix")
+        if nix is None:
+            self.skipTest("nix is not installed")
+        proc = subprocess.run(
+            [
+                nix,
+                "build",
+                HELPER_BUILD_ATTR,
+                "--no-link",
+            ],
+            cwd=REPO_ROOT,
+            text=True,
+            capture_output=True,
+            check=False,
+        )
+        self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr)
+
+    # ── Activation script warns on failure rather than silently swallowing ───
+
+    def test_activation_script_emits_warning_on_failure(self):
+        self.assertIn("warning: sovran-hosts-update", self.source)
+
+
+if __name__ == "__main__":
+    unittest.main()

modules/core/local-domain-loopback.nix

@@ -32,22 +32,31 @@
 # regenerated by the system activation script.  The ``system.activationScripts``
 # hook below converts it to a writable file each time the system is activated
 # (i.e. after every ``nixos-rebuild switch``) and then injects the Sovran block.
-# The same script is also run by the ``sovran-hosts-update.service`` unit so
-# that the Hub can trigger it immediately after saving a domain without
-# requiring a full rebuild.
+# The same wrapped Nix-store executable is reused by both the activation hook
+# and the ``sovran-hosts-update.service`` unit, ensuring a deterministic runtime
+# PATH in every execution context.
 
-{
-  # ── Helper script (stored in the Nix store, never reads /var/lib at eval) ──
+let
+  # ── Wrapped Nix-store executable ──────────────────────────────────────────
+  # Built with pkgs.writeShellApplication so that all required runtime tools
+  # (awk, grep, coreutils) are declared explicitly and injected into PATH by
+  # Nix.  Both the systemd service and the activation hook reference this same
+  # store-path executable — there is no second raw script body.
+  hostsUpdateScript = pkgs.writeShellApplication {
+    name = "sovran-hosts-update";
 
-  environment.systemPackages = [ pkgs.coreutils ];
+    # Declare every external command the script calls.  These packages are
+    # added to the script's runtime PATH by writeShellApplication; nothing from
+    # the system PATH is relied upon.
+    runtimeInputs = [
+      pkgs.coreutils   # readlink, cp, mv, chmod, mktemp, rm, tr, head
+      pkgs.gawk        # awk
+      pkgs.gnugrep     # grep
+    ];
 
-  environment.etc."sovran-hosts-update.sh" = {
-    mode = "0755";
     text = ''
-      #!/bin/sh
       # Regenerate the Sovran-managed loopback block in /etc/hosts.
       # Safe to run multiple times — idempotent.
-      set -eu
 
       DOMAINS_DIR="/var/lib/domains"
       HOSTS_FILE="/etc/hosts"
@@ -66,13 +75,14 @@
 
       # ── Step 2: remove any existing Sovran block ──────────────────────────
       # Use a temp file so the operation is atomic.
+      # awk -v passes marker strings safely without shell interpolation.
       TMP=$(mktemp "$HOSTS_FILE.XXXXXX")
       trap 'rm -f "$TMP"' EXIT
-      awk "
-        /^$BEGIN_MARKER\$/ { skip=1; next }
-        /^$END_MARKER\$/   { skip=0; next }
+      awk -v begin="$BEGIN_MARKER" -v end="$END_MARKER" '
+        $0 == begin { skip=1; next }
+        $0 == end   { skip=0; next }
         !skip
-      " "$HOSTS_FILE" > "$TMP"
+      ' "$HOSTS_FILE" > "$TMP"
 
       # ── Step 3: collect valid configured service domains ──────────────────
       # NOTE: The hostname validation regex below must stay in sync with
@@ -97,11 +107,13 @@
 
       # ── Step 4: append the Sovran block if there are any entries ──────────
       if [ -n "$ENTRIES" ]; then
-        printf '\n%s\n' "$BEGIN_MARKER" >> "$TMP"
-        printf '%s\n' "# These entries route configured service domains to local Caddy." >> "$TMP"
-        printf '%s\n' "# They are managed automatically — do not edit this block." >> "$TMP"
-        printf '%s\n' "$ENTRIES" >> "$TMP"
-        printf '%s\n' "$END_MARKER" >> "$TMP"
+        {
+          printf '\n%s\n' "$BEGIN_MARKER"
+          printf '%s\n' "# These entries route configured service domains to local Caddy."
+          printf '%s\n' "# They are managed automatically — do not edit this block."
+          printf '%s\n' "$ENTRIES"
+          printf '%s\n' "$END_MARKER"
+        } >> "$TMP"
       fi
 
       # ── Step 5: atomically replace /etc/hosts ─────────────────────────────
@@ -110,6 +122,14 @@
     '';
   };
 
+in
+{
+  # ── /etc/sovran-hosts-update.sh — operator discoverability symlink ─────────
+  # Retain the familiar /etc path so administrators can inspect or manually
+  # invoke the helper.  The target is the wrapped Nix-store executable, so
+  # there is no second raw script body to keep in sync.
+  environment.etc."sovran-hosts-update.sh".source = lib.getExe hostsUpdateScript;
+
   # ── Systemd service ────────────────────────────────────────────────────────
 
   systemd.services.sovran-hosts-update = {
@@ -126,18 +146,24 @@
     serviceConfig = {
       Type = "oneshot";
       RemainAfterExit = true;
-      ExecStart = "/etc/sovran-hosts-update.sh";
+      # Point directly at the wrapped Nix-store executable, not the /etc path.
+      ExecStart = lib.getExe hostsUpdateScript;
     };
   };
 
   # ── Activation script (runs after every nixos-rebuild switch) ─────────────
   # This ensures the loopback block survives rebuilds that restore the /etc/hosts
   # symlink.  The "users" and "etc" scripts must complete first.
+  # The same wrapped Nix-store executable used by the systemd service is
+  # referenced here, guaranteeing identical runtime dependencies in both
+  # execution contexts.
 
   system.activationScripts.sovranDomainLoopback = {
     text = ''
-      if [ -x /etc/sovran-hosts-update.sh ] && [ -d /var/lib/domains ]; then
-        /etc/sovran-hosts-update.sh || true
+      if [ -d /var/lib/domains ]; then
+        if ! ${lib.getExe hostsUpdateScript}; then
+          echo "warning: sovran-hosts-update: failed to update /etc/hosts loopback entries" >&2
+        fi
       fi
     '';
     deps = [ "etc" "users" ];