internal/cmd/utils/utils.go (1)

48-68: Consider using strings.Builder for better performance.

The current implementation concatenates strings in a loop (line 64), which creates multiple string allocations. While this is acceptable for small option maps (help text), using strings.Builder would be more efficient.

Apply this diff to improve performance:

 func AlignedOptions(options map[string]string) string {
 	maxLen := 0
 	for cmd := range options {
 		if len(cmd) > maxLen {
 			maxLen = len(cmd)
 		}
 	}
 
-	result := ""
+	var result strings.Builder
 	format := fmt.Sprintf("  %%-%ds  %%s\n", maxLen)
 
 	keys := slices.Collect(maps.Keys(options))
 	sort.Strings(keys)
 
 	for _, cmd := range keys {
 		desc := options[cmd]
-		result += fmt.Sprintf(format, cmd, desc)
+		result.WriteString(fmt.Sprintf(format, cmd, desc))
 	}
 
-	return result
+	return result.String()
 }

cmd/activate/user.go (1)

15-39: Consider attaching stdin to the child process.

Lines 35-36 attach Stdout and Stderr, but Stdin is not attached. If the re-executed process needs to read from stdin (e.g., for confirmation prompts), it will fail.

Apply this diff to attach stdin:

 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
+	cmd.Stdin = os.Stdin
 
 	return cmd.Run()

cmd/activate/fs.go (1)

29-74: Consider logging malformed fstab entries.

The parseFstab function silently skips malformed lines (lines with fewer than 3 fields). While this is lenient and prevents activation failures due to minor fstab issues, it could also hide configuration errors that users should be aware of.

Consider adding optional debug logging for skipped lines:

 		fields := strings.Fields(line)
 
 		if len(fields) < 3 {
+			// Optionally log: "skipping malformed fstab line: %s", line
 			continue
 		}

This would help users diagnose why expected filesystems or swap devices aren't being processed, especially during troubleshooting.

cmd/activate/activate.go (2)

28-45: Tighten arg validation (reject extra positional args).

Currently only checks for presence of ACTION. Enforce exactly one positional arg when not under parent exe to avoid accepting unintended args.

Apply:

-        Args: func(cmd *cobra.Command, args []string) error {
+        Args: func(cmd *cobra.Command, args []string) error {
           if os.Getenv(NIXOS_STC_PARENT_EXE) != "" {
             return nil
           }
 
-          if len(args) == 0 {
+          if len(args) == 0 {
             return fmt.Errorf("missing required argument <ACTION>")
           }
+          if len(args) > 1 {
+            return fmt.Errorf("too many arguments; expected exactly one <ACTION>")
+          }
 
           a, err := activation.ParseSwitchToConfigurationAction(args[0])
           if err != nil {
             return err
           }
 
           opts.Action = a
 
           return nil
         },

---

16-22: Deterministic help ordering for actions.

Map iteration is random; help/completion order will vary. Sort keys before assembling results and AlignedOptions.

-   results := make([]string, 0, len(commands))
-   for command, desc := range commands {
-     results = append(results, fmt.Sprintf("%v\t%v", command, desc))
-   }
+   keys := make([]string, 0, len(commands))
+   for k := range commands { keys = append(keys, k) }
+   sort.Strings(keys)
+   results := make([]string, 0, len(keys))
+   for _, command := range keys {
+     results = append(results, fmt.Sprintf("%v\t%v", command, commands[command]))
+   }

- cmd.SetHelpTemplate(cmd.HelpTemplate() + "\nActions:\n" + cmdUtils.AlignedOptions(commands))
+ cmd.SetHelpTemplate(cmd.HelpTemplate() + "\nActions:\n" + cmdUtils.AlignedOptions(cmdUtils.SortOptions(commands)))

(Assumes a SortOptions helper; otherwise sort locally before formatting.)

Also applies to: 47-53, 63-65

internal/activation/activation.go (1)

159-174: Prefer returning "unknown" over panicking in String().

Panics in String() make diagnostics harder. Consider returning "unknown".

- default:
-   panic("unknown switch to configuration action type")
+ default:
+   return "unknown"

module.nix (2)

48-66: Polish option docs and line breaks.

Minor grammar/formatting fixes; avoid breaking a sentence across lines “switchable. system.”.

-        own, but it will serve solely as a shim to run the switch script on a switchable.
-        system.
+        own, but it will serve solely as a shim to run the switch script on a
+        switchable system.

---

144-154: Ensure wrapper works with paths containing spaces and missing vars.

Even if unlikely in Nix store, quote $out and guard unset variables (like toplevelVar). Also consider exporting NIXOS_ACTION in wrapper for parity.

-          mkdir $out/bin
+          mkdir -p "$out/bin"
-          ln -sf ${lib.getExe cfg.package} $out/bin/switch-to-configuration
+          ln -sf ${lib.getExe cfg.package} "$out/bin/switch-to-configuration"
-          wrapProgram $out/bin/switch-to-configuration \
+          wrapProgram "$out/bin/switch-to-configuration" \
             --add-flags activate \
             --set NIXOS_CLI_ATTEMPTING_ACTIVATION 1 \
-            --set OUT $out \
-            --set TOPLEVEL ''${!toplevelVar} \
+            --set OUT "$out" \
+            --set TOPLEVEL ''${!toplevelVar:-} \

cmd/activate/run.go (2)

171-186: Handle missing init-interface-version files gracefully.

Older generations may not have this file. Treat missing files as mismatch (exit 100) without printing a read error, or warn and continue per intended compatibility.

- currentInitInterfaceVersion, err := os.ReadFile(currentInitInterfaceVersionFile)
- if err != nil {
-   return fmt.Errorf("failed to read %s: %v", currentInitInterfaceVersionFile, err)
- }
+ currentInitInterfaceVersion, err := os.ReadFile(currentInitInterfaceVersionFile)
+ if err != nil {
+   if errors.Is(err, os.ErrNotExist) {
+     return ErrMismatchedInterfaceVersion
+   }
+   return fmt.Errorf("failed to read %s: %w", currentInitInterfaceVersionFile, err)
+ }
 
- newInitInterfaceVersion, err := os.ReadFile(newInitInterfaceVersionFile)
- if err != nil {
-   return fmt.Errorf("failed to read %s: %v", newInitInterfaceVersionFile, err)
- }
+ newInitInterfaceVersion, err := os.ReadFile(newInitInterfaceVersionFile)
+ if err != nil {
+   if errors.Is(err, os.ErrNotExist) {
+     return ErrMismatchedInterfaceVersion
+   }
+   return fmt.Errorf("failed to read %s: %w", newInitInterfaceVersionFile, err)
+ }

And keep the os.Exit(100) branch as-is for a clear user-facing outcome.

Also applies to: 431-437

---

331-335: Nit: wording.

“This command must be ran as root” → “run”.

- err := fmt.Errorf("this command must be ran as root")
+ err := fmt.Errorf("this command must be run as root")

internal/systemd/unit.go (3)

125-133: Make ParseBool robust to common forms.

Support 0/1 and mixed/upper case to match systemd’s lenient booleans.

-func ParseBool(value string) bool {
-  switch value {
-  case "1", "yes", "true", "on":
-    return true
-  default:
-    return false
-  }
-}
+func ParseBool(value string) bool {
+  v := strings.TrimSpace(strings.ToLower(value))
+  switch v {
+  case "1", "yes", "true", "on":
+    return true
+  default:
+    return false
+  }
+}

(Remember to import "strings".)

---

20-58: Consider parsing override unit file itself (not only .d).

When unitFilePath differs from baseUnitFilePath, you parse only its .d/*.conf and not the unitFilePath file. If that path can be a complete override file, include it.

 if unitFilePath != baseUnitFilePath {
-  if _, err := os.Stat(unitFilePath); err == nil {
+  if _, err := os.Stat(unitFilePath); err == nil {
+    if err := parseAndMerge(unitFilePath); err != nil { return nil, err }
     matches, _ := filepath.Glob(fmt.Sprintf("%s.d/*.conf", unitFilePath))
     for _, path := range matches {
       _ = parseAndMerge(path)
     }
   }
 }

---

60-82: Doc nit: complete the comment.

Finish the “Consider using …” sentence or remove it to avoid confusion.

cmd/activate/unit.go (4)

192-200: Sorted() mutates case of unit names in output.

Lowercasing before returning changes values. Sort case-insensitively but return originals.

Apply:

- keys := make([]string, 0, len(l))
- for unit := range l {
-   keys = append(keys, strings.ToLower(unit))
- }
- sort.Strings(keys)
- return keys
+ keys := make([]string, 0, len(l))
+ for unit := range l {
+   keys = append(keys, unit)
+ }
+ sort.Slice(keys, func(i, j int) bool {
+   a, b := strings.ToLower(keys[i]), strings.ToLower(keys[j])
+   if a == b {
+     return keys[i] < keys[j]
+   }
+   return a < b
+ })
+ return keys

---

27-29: Template regex may reject valid instances with dots; prefer last-dot split for extension.

Instances can contain dots; current [^\.]* disallows them. Use a pattern that captures up to the last dot as the extension while allowing dots in the instance.

Apply:

- templateUnitPattern = regexp.MustCompile(`^(.*)@([^\.]*)\.(.*)$`)
+ // <name>@<instance>.<ext> — allow dots in <instance>, ext has no dots
+ templateUnitPattern = regexp.MustCompile(`^([^@]+)@(.+)\.([^.]+)$`)

---

493-505: resolveUnit only falls back to base when both instance files are missing.

If either current or new instance file exists but the other doesn’t, we still want base resolution for templated units. Otherwise CurrentBaseFile may point to a non-existent instance file and upstream checks will skip active templated units.

Apply:

- if matches := templateUnitPattern.FindStringSubmatch(unit); len(matches) > 3 {
-   templateName, _, unitType := matches[1], matches[2], matches[3]
-
-   if _, err := os.Stat(currentUnitFile); os.IsNotExist(err) {
-     if _, err := os.Stat(newUnitFile); os.IsNotExist(err) {
-       baseUnit = fmt.Sprintf("%s@.%s", templateName, unitType)
-       currentBaseFile = filepath.Join("/etc/systemd/system", baseUnit)
-       newBaseFile = filepath.Join(toplevel, "etc/systemd/system", baseUnit)
-     }
-   }
- }
+ if matches := templateUnitPattern.FindStringSubmatch(unit); len(matches) > 3 {
+   templateName, _, unitType := matches[1], matches[2], matches[3]
+   // Prefer base unit for templates unless both instance files are present.
+   if _, err := os.Stat(currentUnitFile); os.IsNotExist(err) {
+     baseUnit = fmt.Sprintf("%s@.%s", templateName, unitType)
+   } else if _, err := os.Stat(newUnitFile); os.IsNotExist(err) {
+     baseUnit = fmt.Sprintf("%s@.%s", templateName, unitType)
+   }
+   if baseUnit != unit {
+     currentBaseFile = filepath.Join("/etc/systemd/system", baseUnit)
+     newBaseFile = filepath.Join(toplevel, "etc/systemd/system", baseUnit)
+   }
+ }

---

554-569: Make list-file writes idempotent to avoid duplicate entries.

recordUnit can append the same unit multiple times; unrecordUnit then rewrites the whole file. Consider de-duping on write for stability and smaller files.

Example approach:

• Keep an in-memory recorded set per file to skip duplicates; or
• Read once and append only if not present; or
• Write list files from the in-memory UnitList at the end of classification.

Also applies to: 595-596

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/systemd/unit.go (1)

48-55: Critical: ParseUnit still ignores the instance file (unitFilePath).

Only the base file and .d drop-ins are merged; the main unitFilePath is never parsed, so instance-level unit content is lost and comparisons can be wrong.

The code parses baseUnitFilePath (line 39) and its drop-ins (lines 43-46), then parses drop-ins for unitFilePath (lines 50-52), but never parses unitFilePath itself. This causes instance-specific unit settings to be silently ignored, leading to incorrect unit comparisons and potentially failed activations.

Apply this diff to parse the instance file:

 if unitFilePath != baseUnitFilePath {
   if _, err := os.Stat(unitFilePath); err == nil {
+    // Parse the instance file itself before its drop-ins
+    if err := parseAndMerge(unitFilePath); err != nil {
+      return nil, err
+    }
     matches, _ := filepath.Glob(fmt.Sprintf("%s.d/*.conf", unitFilePath))
     for _, path := range matches {
       _ = parseAndMerge(path)
     }
   }
 }

cmd/activate/activate.go (1)

28-31: Undefined identifier on non-Linux builds (use literal or move const).

activate.go references NIXOS_STC_PARENT_EXE but it’s defined only in run.go with a linux build tag, causing compile errors on other platforms. Use the literal here or move the const to a non-OS‑specific file.

-            if os.Getenv(NIXOS_STC_PARENT_EXE) != "" {
+            if os.Getenv("__NIXOS_SWITCH_TO_CONFIGURATION_PARENT_EXE") != "" {
                 return nil
             }

module.nix (1)

102-112: Scope sudo env_keep to the specific command; avoid global leakage.

Defaults env_keep applies to all sudo sessions, expanding attack surface. Restrict to the nixos-cli/switch wrapper path via a Cmnd_Alias and Defaults!cmd form.

Example:

security.sudo.extraConfig = ''
  Cmnd_Alias NIXOS_SWITCH = ${lib.getExe cfg.package} *, ${lib.getExe cfg.package} activate *
  Defaults!${lib.getExe cfg.package} env_keep += "NIXOS_CONFIG NIXOS_GENERATION_TAG NIXOS_CLI_CONFIG NIXOS_CLI_DISABLE_STEPS NIXOS_CLI_DEBUG_MODE NIXOS_CLI_SUPPRESS_NO_SETTINGS_WARNING"
'';

Would you like a PR-ready Nix snippet tailored to your exact wrapper path?

cmd/activate/run.go (1)

117-121: Fix specialisation existence check and error.

Formats wrong value and returns a possibly stale err. Return a new error that includes the requested specialisation.

-        if !slices.Contains(specialisations, specialisation) {
-            err = fmt.Errorf("specialisation '%v' does not exist", specialisations)
-            log.Error(err)
-            return err
-        }
+        if !slices.Contains(specialisations, specialisation) {
+            return fmt.Errorf("specialisation defined in $NIXOS_SPECIALISATION %q does not exist", specialisation)
+        }

Also guard the check behind err == nil to avoid false negatives when listing fails:

-    if specialisation != "" {
-        specialisations, err := generation.CollectSpecialisations(constants.CurrentSystem)
-        if err != nil {
-            log.Warnf("unable to access specialisations: %v", err)
-        }
-        if !slices.Contains(specialisations, specialisation) {
+    if specialisation != "" {
+        specialisations, err := generation.CollectSpecialisations(constants.CurrentSystem)
+        if err == nil && !slices.Contains(specialisations, specialisation) {
             ...
         }
     }

internal/systemd/unit.go (1)

126-133: Consider documenting boolean parsing semantics.

The ParseBool function implements systemd's boolean parsing logic but doesn't document which values map to true vs false. This is important for maintainability since systemd has specific semantics.

Add a function comment:

 // Parse a systemd boolean value string into a Go boolean.
+// Returns true for "1", "yes", "true", "on" (case-sensitive).
+// All other values, including "0", "no", "false", "off", return false.
 func ParseBool(value string) bool {

cmd/activate/user.go (2)

15-39: Consider validating UID/GID parameters.

The execUserSwitchProcess function accepts uid and gid parameters but doesn't validate they are non-zero or represent valid system users. While the caller may be trusted, defensive validation can prevent subtle bugs.

Consider adding basic validation:

 func execUserSwitchProcess(uid uint32, gid uint32, runtimePath string) error {
+  if uid == 0 || gid == 0 {
+    return fmt.Errorf("uid (%d) and gid (%d) must be non-zero for user switch", uid, gid)
+  }
+
   exe, err := filepath.EvalSymlinks("/proc/self/exe")
   if err != nil {
     return err
   }

---

64-68: Document why ReexecuteContext errors are ignored.

The comment explains that the systemd user session doesn't send a Reloaded signal and that dbus session bus errors are expected, but it would be helpful to clarify under what conditions errors are expected and whether any errors should be propagated.

Consider enhancing the comment:

 // The systemd user session seems to not send a Reloaded signal,
 // so we don't have anything to wait on here.
 //
 // Also, ignore errors, since the dbus session bus will probably
-// return an error here due to it running in the user's context.
+// return an error here due to it running in the user's context
+// and not having sufficient privileges for certain operations.
+// These errors are harmless and can be safely ignored.
 _ = systemd.ReexecuteContext(ctx)

module.nix (1)

144-154: Minor polish: wrapProgram arg quoting and comment accuracy.

• Quote/escape SYSTEMD like others for consistency (paths are safe but consistency helps).
• The comment mentions “nixos apply” but this module wires “nixos activate”.

-            --set SYSTEMD ${config.systemd.package}
+            --set SYSTEMD ${lib.escapeShellArg config.systemd.package}

-  # Preserve NIXOS_CONFIG and NIXOS_CLI_CONFIG in sudo invocations of
-  # `nixos apply`. This is required in order to keep ownership across
+  # Preserve NIXOS_CONFIG and NIXOS_CLI_CONFIG in sudo invocations of
+  # `nixos activate`. This is required in order to keep ownership across

Also applies to: 151-154

cmd/activate/run.go (1)

168-186: Be robust to newline/whitespace in init-interface-version files.

Trim file contents before comparing to avoid false mismatches.

-    if string(currentInitInterfaceVersion) != string(newInitInterfaceVersion) {
+    if strings.TrimSpace(string(currentInitInterfaceVersion)) != strings.TrimSpace(string(newInitInterfaceVersion)) {
         return ErrMismatchedInterfaceVersion
     }

cmd/activate/unit.go (2)

192-200: Keep original unit names while sorting case-insensitively.

Current Sorted() lowercases outputs. Sort by Lower but return original strings.

 func (l UnitList) Sorted() []string {
-    keys := make([]string, 0, len(l))
-    for unit := range l {
-        keys = append(keys, strings.ToLower(unit))
-    }
-    sort.Strings(keys)
-    return keys
+    keys := make([]string, 0, len(l))
+    for unit := range l {
+        keys = append(keys, unit)
+    }
+    sort.Slice(keys, func(i, j int) bool {
+        return strings.ToLower(keys[i]) < strings.ToLower(keys[j])
+    })
+    return keys
 }

---

450-459: Avoid panic on Variant type assertion.

Be defensive when extracting ActiveState.

-    state := prop.Value.Value().(string)
-    return state == "active" || state == "activating", nil
+    v, ok := prop.Value.Value().(string)
+    if !ok {
+        return false, fmt.Errorf("ActiveState is not a string")
+    }
+    return v == "active" || v == "activating", nil

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:245-281
Timestamp: 2025-10-20T02:59:26.104Z
Learning: Go 1.25 introduced the sync.WaitGroup.Go(f func()) method, which starts f in a new goroutine and automatically handles Add(1) and defer Done(). This is valid Go 1.25+ syntax and should not be flagged as an error.

module.nix (1)

102-112: Scope down sudo env_keep to nixos-cli only; avoid global leakage.

Keep env across sudo only for the nixos-cli/switch command, not globally. Also consider dropping DEBUG_MODE from env_keep.

Example (command-scoped Defaults):

-      security.sudo.extraConfig = ''
-        Defaults env_keep += "NIXOS_CONFIG"
-        Defaults env_keep += "NIXOS_GENERATION_TAG"
-        Defaults env_keep += "NIXOS_CLI_CONFIG"
-        Defaults env_keep += "NIXOS_CLI_DISABLE_STEPS"
-        Defaults env_keep += "NIXOS_CLI_DEBUG_MODE"
-        Defaults env_keep += "NIXOS_CLI_SUPPRESS_NO_SETTINGS_WARNING"
-      '';
+      security.sudo.extraConfig = ''
+        # Restrict preserved env to nixos-cli binary
+        Defaults!${lib.getExe cfg.package} env_keep += "NIXOS_CONFIG NIXOS_GENERATION_TAG NIXOS_CLI_CONFIG NIXOS_CLI_DISABLE_STEPS NIXOS_CLI_SUPPRESS_NO_SETTINGS_WARNING"
+      '';

cmd/activate/run.go (2)

332-336: Grammar: “must be run as root.”

-		err := fmt.Errorf("this command must be ran as root")
+		err := fmt.Errorf("this command must be run as root")

---

117-121: Fix specialisation error: wrong arg and unclear message.

Use the requested specialisation in the error; no need to log first if caller logs.

-		if !slices.Contains(specialisations, specialisation) {
-			err = fmt.Errorf("specialisation '%v' does not exist", specialisations)
-			log.Error(err)
-			return err
-		}
+		if !slices.Contains(specialisations, specialisation) {
+			return fmt.Errorf("specialisation defined in $NIXOS_SPECIALISATION %q does not exist", specialisation)
+		}

internal/system/local.go (3)

50-50: Consider a more precise regex pattern.

The current pattern ^\"?nixos\"?$ will match edge cases like "nixos (quote only at start) or nixos" (quote only at end). While unlikely in practice for the os-release ID field, a more precise pattern would be:

var nixosDistroIDRegex = regexp.MustCompile(`^(?:"nixos"|nixos)$`)

This explicitly matches either quoted or unquoted variants without allowing mismatched quotes.

---

82-82: Simplify the defer close pattern.

The anonymous function wrapper is unnecessarily verbose. The idiomatic Go pattern for deferring a close is simply:

defer osRelease.Close()

For read-only files, the close error is typically not critical, and the standard pattern doesn't require explicit error suppression.

Apply this diff to simplify:

-	defer func() { _ = osRelease.Close() }()
+	defer osRelease.Close()

---

87-95: Check scanner error after loop.

After scanning completes, the scanner's Err() method should be checked to detect any errors that occurred during scanning (e.g., I/O errors).

Apply this diff to add error checking:

 	for s.Scan() {
 		key, value, found := strings.Cut(s.Text(), "=")
 		if !found {
 			continue
 		}
 		values[key] = value
 	}

+	if err := s.Err(); err != nil {
+		return nil, err
+	}
+
 	return values, nil
 }

doc/man/nixos-cli-env.5.scd (3)

83-92: Align env var semantics with code (presence vs “1”).

Docs say “set to 1,” but run.go currently treats any non-empty value as “set.” Either:

• Change code to check for value == "1" (recommended; see my run.go diff), or
• Update these lines to say “if set (non-empty)”.

Please pick one for consistency.

---

90-92: Clarify accepted values.

State explicitly “Set to 1 to show all units; unset or other values hide non-essential units.” Matches current usage in code.

---

103-106: Grammar nit.

“Only useful for during development.” → “Only useful during development.”

-	Show log messages for when developing TUIs. Only useful for during
-	development.
+	Show log messages when developing TUIs. Only useful during development.

cmd/activate/run.go (1)

749-753: Document checkpoint/resume intent for list files.

Consider adding a brief comment where you record/unrecord units and where you RemoveAll to explain the resilience pattern (persist before action; clean on success). Reduces future confusion.

Also applies to: 757-758, 772-785

cmd/activate/unit.go (2)

192-200: Preserve original unit names while sorting case-insensitively.

Current implementation lowercases returned names. Keep original strings for readability.

-func (l UnitList) Sorted() []string {
-	keys := make([]string, 0, len(l))
-	for unit := range l {
-		keys = append(keys, strings.ToLower(unit))
-	}
-	sort.Strings(keys)
-	return keys
-}
+func (l UnitList) Sorted() []string {
+	keys := make([]string, 0, len(l))
+	for unit := range l {
+		keys = append(keys, unit)
+	}
+	sort.Slice(keys, func(i, j int) bool {
+		return strings.ToLower(keys[i]) < strings.ToLower(keys[j])
+	})
+	return keys
+}

---

533-542: Handle scanner error (optional).

Capture scanner.Err() to avoid silently ignoring read errors.

 s := bufio.NewScanner(file)
 for s.Scan() {
   // ...
 }
-
-return units
+if err := s.Err(); err != nil {
+  // best-effort: ignore content on error
+  return units
+}
+return units

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:245-281
Timestamp: 2025-10-20T02:59:26.104Z
Learning: Go 1.25 introduced the sync.WaitGroup.Go(f func()) method, which starts f in a new goroutine and automatically handles Add(1) and defer Done(). This is valid Go 1.25+ syntax and should not be flagged as an error.

Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:752-761
Timestamp: 2025-10-20T20:32:37.692Z
Learning: In cmd/activate/run.go, the global list files (START_LIST_FILE, RELOAD_LIST_FILE, RESTART_LIST_FILE) implement a checkpoint/recovery pattern for activation resilience: they are read at start (unit.go), written during processing before actions are taken (for persistence across interruptions), and removed after successful completion (for cleanup). If the activation process is interrupted, the next run loads these files to complete unfinished unit actions.

cmd/activate/run.go (3)

387-395: Enforce explicit "1" value for NIXOS_NO_CHECK.

The current logic skips checks when NIXOS_NO_CHECK is empty, but the environment variable semantics should require the literal value "1" to disable checks (any other value, including empty, should run checks).

Apply this diff:

-	if skipCheck := os.Getenv("NIXOS_NO_CHECK"); skipCheck == "" {
+	if os.Getenv("NIXOS_NO_CHECK") != "1" {
 		log.Info("running pre-switch checks")
 
 		err = runPreSwitchCheck(s, vars.PreSwitchCheckCmd, vars.Toplevel, opts.Action)

---

410-426: Enforce explicit "1" value for NIXOS_NO_SYNC.

The current logic syncs when NIXOS_NO_SYNC is empty, but the environment variable semantics should require the literal value "1" to skip syncing (any other value, including empty, should perform the sync).

Apply this diff:

-	if skipSync := os.Getenv("NIXOS_NO_SYNC"); skipSync == "" {
+	if os.Getenv("NIXOS_NO_SYNC") != "1" {
 		log.Info("syncing /nix/store to disk")
 
 		dir, err := os.Open("/nix/store")

---

332-336: Fix grammar in error message.

Apply this diff:

 	if os.Geteuid() != 0 {
-		err := fmt.Errorf("this command must be ran as root")
+		err := fmt.Errorf("this command must be run as root")
 		log.Errorf("%s", err)
 		return err
 	}

module.nix (1)

102-112: Security concern: Global sudo env_keep scope remains unaddressed.

The previous review raised a security concern about preserving NIXOS_* variables globally across all sudo invocations. This issue has not been addressed in the current changes. These variables can leak control to arbitrary sudo sessions beyond nixos activate or nixos apply.

As suggested in the previous review, restrict the env_keep settings to a specific command alias:

1. Define a Cmnd_Alias for the nixos-cli commands
2. Apply env_keep only to that alias via a Runas_Spec

Example approach:

security.sudo.extraConfig = ''
  # Define command alias for nixos-cli activation
  Cmnd_Alias NIXOS_CLI = ${lib.getExe cfg.package}
  
  # Preserve NIXOS_* variables only for nixos-cli commands
  Defaults!NIXOS_CLI env_keep += "NIXOS_CONFIG NIXOS_GENERATION_TAG NIXOS_CLI_CONFIG NIXOS_CLI_DISABLE_STEPS NIXOS_CLI_DEBUG_MODE NIXOS_CLI_SUPPRESS_NO_SETTINGS_WARNING"
'';

This minimizes the blast radius by ensuring these variables are preserved only for the specific nixos-cli executable.

go.mod (1)

13-13: Consider migrating to carapace-shlex to avoid relying on an archived library.

The google/shlex repository is archived and hasn't received updates since 2019, meaning it won't receive security patches or bug fixes. The codebase already has carapace-shlex v1.0.1 as an indirect dependency, which is an actively maintained fork with the same Split() API. Since google/shlex is only used twice in cmd/activate/run.go (lines 137 and 155), migration is straightforward.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:245-281
Timestamp: 2025-10-20T02:59:26.104Z
Learning: Go 1.25 introduced the sync.WaitGroup.Go(f func()) method, which starts f in a new goroutine and automatically handles Add(1) and defer Done(). This is valid Go 1.25+ syntax and should not be flagged as an error.

Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:752-761
Timestamp: 2025-10-20T20:32:37.692Z
Learning: In cmd/activate/run.go, the global list files (START_LIST_FILE, RELOAD_LIST_FILE, RESTART_LIST_FILE) implement a checkpoint/recovery pattern for activation resilience: they are read at start (unit.go), written during processing before actions are taken (for persistence across interruptions), and removed after successful completion (for cleanup). If the activation process is interrupted, the next run loads these files to complete unfinished unit actions.