feat(apply): add support for remote building/activation#116
feat(apply): add support for remote building/activation#116water-sucks merged 13 commits intonix-community:mainfrom
Conversation
WalkthroughAdds SSH-backed remote build/deploy support, filesystem abstractions, and system injection into activation helpers; exposes --build-host/--target-host/--remote-root and ActivateOpts.Verbose; refactors Nix option handling to typed NixOption with category filtering; updates deps, docs, tests, and command/runner APIs. Changes
Sequence Diagram(s)%% Remote build + deploy high-level flow
sequenceDiagram
participant User
participant CLI as Apply CLI
participant BuildSys as Build System (Local/SSH)
participant TargetSys as Target System (Local/SSH)
participant Copy as nix-copy-closure
User->>CLI: run apply --build-host <H> --target-host <T>
CLI->>BuildSys: NewSSHSystem/LocalSystem(build-host)
CLI->>TargetSys: NewSSHSystem/LocalSystem(target-host)
CLI->>BuildSys: build (nix-instantiate / nix build)
BuildSys-->>CLI: closure paths
CLI->>Copy: CopyClosures(BuildSys -> TargetSys, paths...)
Copy-->>TargetSys: transfer closures
CLI->>TargetSys: SwitchToConfiguration (activation)
TargetSys-->>CLI: activation result
CLI-->>User: success/failure
%% Activation helpers using injected system + FS
sequenceDiagram
participant Activation
participant Sys as system.System
participant FS as Filesystem
Activation->>Sys: FindDefaultSpecialisationFromConfig(s, gen)
Sys->>FS: ReadFile(generation config)
FS-->>Sys: contents
Activation->>Sys: VerifySpecialisationExists(s, gen, spec)
Sys->>FS: Stat(switch-to-configuration)
FS-->>Sys: exists?
Activation->>Sys: GetCurrentGenerationNumber(s, profile)
Sys->>FS: ReadLink(profile/current)
FS-->>Sys: link target
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
module.nix (1)
102-114: Make SSH_AUTH_SOCK preservation configurable for security-conscious environments.Preserving
SSH_AUTH_SOCKin sudo invocations allows root commands to access the user's SSH agent. While necessary for remote activation workflows, this degrades security posture by violating the principle of least privilege. Users enablingservices.nixos-cli.enableautomatically inherit this behavior without explicit opt-in.Consider adding a module option (e.g.,
preserveSSHAuthInSudo) that defaults to the behavior required for remote operations but allows security-conscious users to disable it.Would you like me to help design the module option structure for making this configurable?
doc/man/nixos-cli-apply.1.scd (2)
112-115: "--no-build" is not a supported flag; fix dry-build instructions.The CLI exposes --no-activate and --no-boot, not --no-build. Update docs accordingly.
Suggested edit:
- Dry builds of the configuration can be achieved by passing *--no-activate* - and *--no-build*, as well as *--output*. If any of these options are not - passed, then dry activation is assumed. + Dry builds can be achieved by passing *--no-activate* and *--no-boot*, + as well as *--output*. If any of these options are not passed, dry + activation is assumed.
131-133: Document interaction of --output with remote flags.Code disallows --output together with --build-host or --target-host. Add a note here to prevent surprises.
Proposed addition:
*-o*, *--output* <PATH> Symlink the result of the build to the given *PATH*. + Note: not available with *--build-host* or *--target-host*.cmd/apply/apply.go (2)
292-297: Fix stray formatted logging arg.Second Warnf has no formatting directive but passes an argument.
- log.Warnf("continuing with existing channels", err) + log.Warn("continuing with existing channels")
149-159: Manpage section reference mismatch (1 vs 5).Help text points to nixos-cli-apply(5) but the man page is section 1.
- Check the man page nixos-cli-apply(5) for more details on what options are available. + Check the man page nixos-cli-apply(1) for more details on what options are available.internal/cmd/nixopts/nixopts.go (1)
182-188: Use typed constants for consistency.These two still pass raw strings; switch to the typed NixOption constants for uniformity.
-func AddUpdateInputNixOption(cmd *cobra.Command, dest *[]string) { - addNixOptionStringArray(cmd, dest, "update-input", "", "Update a specific flake input") -} +func AddUpdateInputNixOption(cmd *cobra.Command, dest *[]string) { + addNixOptionStringArray(cmd, dest, NixOptionUpdateInput, "", "Update a specific flake input") +} @@ -func AddOverrideInputNixOption(cmd *cobra.Command, dest *map[string]string) { - addNixOptionStringMap(cmd, dest, "override-input", "", "Override a specific flake input (passed as 1 arg, requires = separator)") -} +func AddOverrideInputNixOption(cmd *cobra.Command, dest *map[string]string) { + addNixOptionStringMap(cmd, dest, NixOptionOverrideInput, "", "Override a specific flake input (passed as 1 arg, requires = separator)") +}internal/system/local.go (1)
95-110: Check Scanner errors and surface them.parseOSRelease never checks Scanner errors; corrupted/long lines would be silently ignored. Return s.Err() after the loop.
Apply this diff:
func parseOSRelease(r io.Reader) (map[string]string, error) { values := make(map[string]string) s := bufio.NewScanner(r) s.Split(bufio.ScanLines) for s.Scan() { key, value, found := strings.Cut(s.Text(), "=") if !found { continue } values[key] = value } - return values, nil + if err := s.Err(); err != nil { + return nil, err + } + return values, nil }internal/activation/activation.go (1)
58-63: Remove the unnecessaryos.ErrExistcheck; bothos.MkdirAlland SFTP'sMkdirAllreturnnilfor existing directories.Both
LocalFilesystemandSFTPFilesystemimplementations delegate to standard MkdirAll functions that returnnil(notos.ErrExist) when the directory already exists. The check at line 60 is dead code and should be removed:err := s.FS().MkdirAll(constants.NixSystemProfileDirectory, 0o755) if err != nil { return fmt.Errorf("failed to create nix system profile directory: %w", err) }
🧹 Nitpick comments (17)
cmd/apply/apply.go (4)
214-221: Defer root escalation to only when needed (avoid prompting on build-only/VM flows).Currently re-execs as root for any local target, including pure builds and VM builds. Escalate only when an operation actually requires root (activation/profile/boot, or channel upgrades for legacy).
Apply this change:
- if os.Geteuid() != 0 && !targetHost.IsRemote() { - err := utils.ExecAsRoot(cfg.RootCommand) - if err != nil { - log.Errorf("failed to re-exec command as root: %v", err) - return err - } - } + // Escalate only when local operations require root. + needsRoot := !targetHost.IsRemote() && + ( // activation/boot paths + buildType != configuration.SystemBuildTypeSystem || + // legacy channel upgrades act on root's channels + (!build.Flake() && (opts.UpgradeChannels || opts.UpgradeAllChannels))) + if os.Geteuid() != 0 && needsRoot { + if err := utils.ExecAsRoot(cfg.RootCommand); err != nil { + log.Errorf("failed to re-exec command as root: %v", err) + return err + } + }
283-287: Restore working directory after temporary chdir.Avoid leaking CWD changes to subsequent steps.
- err := os.Chdir(configDirname) - if err != nil { - configIsDirectory = false - } + if err := os.Chdir(configDirname); err != nil { + configIsDirectory = false + } else { + defer func() { _ = os.Chdir(originalCwd) }() + }
103-105: Flags and docs: align --output restrictions with remote flags.Code marks --output mutually exclusive with both --build-host and --target-host. Ensure the man page calls this out (see doc comment). If you intend to allow --output when target is local and only the builder is remote, relax the exclusivity with --build-host.
Option A (docs-only): keep exclusivity and document it.
Option B (behavior change): allow --output with --build-host if --target-host is empty; requires revisiting where the symlink is created.Also applies to: 143-146
404-409: Diff failure is non-fatal: OK, but consider surfacing a concise hint.You log the error and proceed; consider adding a one-line hint (e.g., "continuing without diff") to set expectations.
internal/cmd/opts/opts.go (1)
43-70: Avoid duplication: extract a shared NixOptions type.ApplyOpts and InstallOpts both carry near-identical Nix options; inline struct invites drift.
Refactor into a reusable type (e.g., type NixOptions struct { … }) with consistent
nixCategorytags; embed it in both opts to keep parsing behavior aligned.cmd/activate/run.go (1)
19-19: shlex implementation swap: verify parsing compatibility on edge casescarapace-shlex may differ from google/shlex on escapes/quotes. Please validate these inputs still produce identical argv:
"/nix/store/…/script --flag='a b' --x=\"c\\\"d\"""cmd multiple spaces --opt=value""cmd 'unterminated"(should error)Consider extracting a small helper:
buildAndRun(s system.CommandRunner, cmdStr string, extraArgs ...string) error
and use it from runPreSwitchCheck/installBootloader to de-duplicate logic.internal/cmd/nixopts/convert_test.go (1)
12-17: Keep test category metadata in sync with production flagsThese tags mirror production behavior. To prevent drift, consider deriving from shared constants or a small helper in test that references nixopts’ NixOption definitions instead of re-declaring categories here.
internal/settings/settings.go (1)
191-206: DRY parsing: factor common logic to a provider-based helperImplementation is fine. To reduce duplication, extract a shared
parseWithProvider(p koanf.Provider) (*Settings, error)used by bothParseSettingsandParseSettingsFromString.Example sketch:
+func parseWithProvider(p koanf.Provider) (*Settings, error) { + k := koanf.New(".") + if err := k.Load(p, toml.Parser()); err != nil { return nil, err } + cfg := NewSettings() + if err := k.Unmarshal("", cfg); err != nil { return nil, err } + return cfg, nil +}Then:
- func ParseSettings(location string) (*Settings, error) { - k := koanf.New(".") - if err := k.Load(file.Provider(location), toml.Parser()); err != nil { return nil, err } - cfg := NewSettings() - if err := k.Unmarshal("", cfg); err != nil { return nil, err } - return cfg, nil - } + func ParseSettings(location string) (*Settings, error) { + return parseWithProvider(file.Provider(location)) + }and
- func ParseSettingsFromString(input string) (*Settings, error) { - k := koanf.New(".") - if err := k.Load(rawbytes.Provider([]byte(input)), toml.Parser()); err != nil { return nil, err } - cfg := NewSettings() - if err := k.Unmarshal("", cfg); err != nil { return nil, err } - return cfg, nil - } + func ParseSettingsFromString(input string) (*Settings, error) { + return parseWithProvider(rawbytes.Provider([]byte(input))) + }internal/system/system.go (1)
56-67: Check nix-copy-closure availability before running; provide clearer failure.If nix-copy-closure isn’t installed on the selected runner, this fails late. Check first and return a clear error; optionally fall back to
nix copyif supported.Apply inside this function before Run:
log.CmdArray(argv) cmd := NewCommand(argv[0], argv[1:]...) -_, err := commandRunner.Run(cmd) +if hc, ok := any(commandRunner).(interface{ HasCommand(string) bool }); ok && !hc.HasCommand("nix-copy-closure") { + return fmt.Errorf("nix-copy-closure not found on selected runner; ensure it is installed or add a fallback to `nix copy`") +} +_, err := commandRunner.Run(cmd) return errNote: Requires
fmtimport (already added in previous suggestion).internal/system/fs.go (1)
5-10: Filesystem abstraction looks good. Consider symlink-aware needs.API is minimal and clear. If callers need to avoid following symlinks, consider adding Lstat in a future change for parity with os.*.
internal/system/local_fs.go (1)
7-23: Thin local FS wrapper is correct.Straight delegation to os.* is fine.
Add brief doc comments to methods for clarity when browsing implementations.
internal/cmd/nixopts/nixopts.go (1)
172-176: Dual flags map to the same dest; consider deprecation handling.Binding both --no-use-registries and --no-registries to the same bool is fine, but add a deprecation notice for the legacy alias via Cobra’s Deprecated field for discoverability.
Example:
f := cmd.Flags().Lookup(string(NixOptionNoRegistries)) if f != nil { f.Deprecated = "use --no-use-registries" }go.mod (1)
97-97: Document the go-systemd fork rationale in go.mod.The fork commit (2d9fdc7) adds the replace directive but provides no explanation of why the fork was needed. While go.sum properly pins the fork to a specific commit (v22.0.0-20251014195852-fe9dbfc225a6), addressing the reproducibility concern, the lack of documentation makes it unclear whether this is temporary or permanent, and what the fork fixes compared to upstream.
Add an inline comment in go.mod explaining:
- Why the fork is used (what issue/incompatibility does it resolve?)
- Whether this is a temporary workaround pending an upstream fix or a permanent fork
// Fork includes [brief explanation of what/why] replace github.com/coreos/go-systemd/v22 v22.6.0 => github.com/water-sucks/go-systemd/v22 v22.0.0-20251014195852-fe9dbfc225a6internal/system/local.go (1)
95-110: Normalize os-release parsing (trim/unquote) for robustness.IDs may be quoted and/or have stray spaces. Trim and unquote values to reduce regex special-casing.
Apply this diff (adds import “strconv”):
@@ -import ( +import ( "bufio" "io" "os" "os/exec" "regexp" "strings" + "strconv" @@ - key, value, found := strings.Cut(s.Text(), "=") + key, value, found := strings.Cut(s.Text(), "=") if !found { continue } - values[key] = value + key = strings.TrimSpace(key) + v := strings.TrimSpace(value) + if len(v) >= 2 && (v[0] == '"' || v[0] == '\'') { + if unq, err := strconv.Unquote(v); err == nil { + v = unq + } + } + values[key] = vinternal/configuration/flake.go (1)
146-157: Prefer TrimSpace for output.Use strings.TrimSpace to remove all trailing whitespace, not only spaces/newlines.
Apply this diff:
- return strings.Trim(stdout.String(), "\n "), err + return strings.TrimSpace(stdout.String()), errinternal/configuration/legacy.go (1)
144-166: Use the provided LocalSystem consistently.You log via s.Logger() but execute via l.Builder.Run(). Prefer s.Run(cmd) for consistency and easier substitution/mocking.
Apply this diff:
- _, err := l.Builder.Run(cmd) + _, err := s.Run(cmd)internal/system/ssh.go (1)
102-148: Consider adding timeout for SSH session execution.The SSH session has no timeout configured, which could cause commands to hang indefinitely if the remote system becomes unresponsive or if a command gets stuck.
Consider adding a configurable timeout. You could add a
Contextparameter to theCommandstruct or usesession.Wait()with a goroutine and timer:func (s *SSHSystem) Run(cmd *Command) (int, error) { // ... session setup ... // Start command asynchronously err = session.Start(fullCmd) if err != nil { return 0, err } // Wait with timeout done := make(chan error, 1) go func() { done <- session.Wait() }() timeout := 5 * time.Minute // Make this configurable select { case err := <-done: // Handle error as before case <-time.After(timeout): _ = session.Signal(ssh.SIGTERM) return 0, fmt.Errorf("command timed out after %v", timeout) } // ... rest of error handling ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
cmd/activate/activate.go(1 hunks)cmd/activate/run.go(3 hunks)cmd/apply/apply.go(13 hunks)cmd/generation/rollback/rollback.go(1 hunks)cmd/generation/switch/switch.go(1 hunks)cmd/info/info.go(1 hunks)doc/man/nixos-cli-apply.1.scd(2 hunks)go.mod(4 hunks)internal/activation/activation.go(5 hunks)internal/cmd/nixopts/convert.go(2 hunks)internal/cmd/nixopts/convert_test.go(2 hunks)internal/cmd/nixopts/nixopts.go(1 hunks)internal/cmd/opts/opts.go(2 hunks)internal/configuration/flake.go(3 hunks)internal/configuration/legacy.go(3 hunks)internal/settings/settings.go(2 hunks)internal/system/fs.go(1 hunks)internal/system/local.go(4 hunks)internal/system/local_fs.go(1 hunks)internal/system/runner.go(1 hunks)internal/system/sftp_fs.go(1 hunks)internal/system/ssh.go(1 hunks)internal/system/system.go(1 hunks)module.nix(2 hunks)package.nix(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (28)
package.nix (1)
16-16: vendorHash bump looks fine; please verify with a clean build.No functional changes here. Run a clean nix build to confirm the new vendorHash matches the updated module graph.
doc/man/nixos-cli-apply.1.scd (1)
163-179: Target host requirements: call out privilege and crossSystem explicitly.Text is good; consider adding a one-liner that activation runs as root on the target (sudo not auto-invoked) to set user expectations.
Please confirm whether activation escalates via sudo on the target or requires direct root login; reflect that here if applicable.
cmd/apply/apply.go (2)
306-316: nom availability check: good.Correctly checks builder-side availability and errors only on explicit --use-nom; otherwise warns and falls back.
411-430: Remote skip for confirmation: fine as a temporary workaround.Clear FIXME and safe default (skip prompt remotely). Consider adding a --yes default in remote mode in future to keep UX consistent.
internal/cmd/opts/opts.go (1)
24-42: Surface area looks good.Additions for remote build/target, VM modes, tags, and verbosity are coherent and map cleanly to apply.go usage.
cmd/info/info.go (1)
57-57: LGTM: system context correctly threaded into activation helpercmd/generation/rollback/rollback.go (3)
110-110: LGTM: default specialisation lookup now uses system context
118-118: LGTM: specialisation verification updated to system-aware helper
124-124: LGTM: capture current generation number via system-aware APICapturing the pre-change generation for rollback safety remains correct with the new signature.
internal/cmd/nixopts/convert_test.go (1)
102-147: Code is correct; map-based flags are sorted deterministicallyVerification confirms the implementation already addresses the ordering concern. At lines 145-147 in
internal/cmd/nixopts/convert.go, map keys are explicitly sorted lexicographically before iteration inexpandNixOptionArg, ensuring deterministic output across runs regardless of Go's random map iteration. No changes needed.cmd/activate/activate.go (1)
63-64: LGTM: verbose flag binding.Flag wiring matches opts.Verbose and aligns with existing logging flow.
cmd/generation/switch/switch.go (3)
162-168: LGTM: pass system context into default specialisation discovery.Matches the new activation API and enables remote-capable FS access.
170-174: LGTM: system-aware specialisation verification.Propagating
sensures consistent local/remote handling.
176-181: LGTM: system-aware current generation lookup.Consistent with the rest of the changes.
internal/system/system.go (1)
6-10: No breaking changes detected — both System implementors already satisfy the full interface.Verification confirms that LocalSystem and SSHSystem already implement all CommandRunner methods (Run, Logger, HasCommand) plus the System-specific methods (IsNixOS, IsRemote, FS). Embedding CommandRunner is safe and doesn't require changes to existing implementors or downstream code that imports CommandRunner directly.
go.mod (1)
3-3: The original review comment is incorrect. Go 1.25 was released on August 12, 2025, making it a valid, currently available toolchain version. The code ingo.modis correct as-is and requires no changes.Likely an incorrect or invalid review comment.
internal/configuration/flake.go (1)
195-207: The code already correctly includes evalArgs in the drvPath eval command.The verification confirms that line 196 already contains
evalDrvCmdArgv = append(evalDrvCmdArgv, evalArgs...), which applies the computed eval flags to the drvPath evaluation. The evalArgs variable, defined at line 162, is populated with relevant nix options from the configuration (lock file options, impure flag, etc.) and is correctly passed to the eval command.The review comment's diff is identical on both sides, indicating no actual change is needed. The requested behavior is already implemented in the current code.
Likely an incorrect or invalid review comment.
internal/activation/activation.go (5)
19-33: LGTM! Filesystem abstraction properly applied.The refactoring correctly uses the injected system's filesystem interface for reading the configuration file, enabling remote system support while maintaining the existing logic.
37-49: LGTM! Filesystem abstraction properly applied.The specialization verification correctly uses the injected filesystem interface.
68-85: LGTM! System abstraction properly applied.The function correctly uses the injected system for directory creation, logging, and command execution.
87-104: LGTM! System abstraction properly applied.The function correctly uses the injected system for directory creation and command execution.
106-128: LGTM! Filesystem abstraction properly applied.The function correctly uses the injected filesystem interface for reading the symbolic link.
internal/system/ssh.go (6)
20-27: LGTM! Clean struct definition.The
SSHSystemstruct properly encapsulates SSH client, SFTP client, connection details, and logger.
29-29: LGTM! Clear error message aligns with PR objectives.The error message appropriately guides users to start an SSH agent, which aligns with the PR's goal of using SSH agent-based authentication exclusively.
94-100: LGTM! Clean accessor methods.The
FS()andLogger()methods correctly expose the SFTP filesystem and logger.
150-173: LGTM! Robust NixOS detection with fallback.The method properly checks for NixOS using the canonical
/etc/NIXOSfile and falls back to parsing/etc/os-release, providing robust detection.
175-181: LGTM! Simple and correct.The
Address()andIsRemote()methods correctly return the connection details and remote status.
201-204: Close method looks good, but ensure agent connection cleanup.The method correctly closes the SFTP and SSH clients. However, ensure the SSH agent connection (identified in the earlier comment) is also cleaned up here once you add it to the struct.
89e166f to
e23b978
Compare
e23b978 to
a67360a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/configuration/legacy.go (1)
131-133: Filter local build flags to build-only.Passing all flags risks injecting unsupported options into nix-build. Use the “build” category.
- if opts.NixOpts != nil { - argv = append(argv, nixopts.NixOptionsToArgsList(opts.CmdFlags, opts.NixOpts)...) - } + if opts.NixOpts != nil { + argv = append(argv, nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "build")...) + }internal/activation/activation.go (1)
124-127: Return an error instead of panicking when the generation link doesn’t match.Panic here can crash callers; surface an actionable error.
- } else { - panic("current link format does not match 'profile-generation-link' format") - } + } + return 0, fmt.Errorf("current link %q does not match expected 'profile-generation-link' format", currentGenerationLink)
♻️ Duplicate comments (7)
doc/man/nixos-cli-apply.1.scd (2)
87-99: Clarify HOST syntax and document limitations.The HOST format description is incomplete. Based on the parser implementation in
internal/system/ssh.go, the format should indicate that username and port are optional, and note that IPv6 addresses are not currently supported due to parsing limitations.Suggested documentation update:
-*HOST* is a simple SSH address with the format "_user@address:port_". +*HOST* is an SSH address "[user@]host[:port]". Username defaults to the current user, and port defaults to 22 if omitted. IPv6 addresses are not currently supported.
163-179: Same HOST syntax issue as --build-host.Apply the same documentation clarification for the --target-host HOST format.
internal/configuration/flake.go (2)
165-185: Critical: Manual flag parsing strips "--" prefix and drops --impure.The manual loop has several issues:
- Line 166 strips "--" via
TrimPrefixbut never restores it when buildingbuildArgs(line 183)- The
nixopts.NixOptionImpurecase (line 181) has no body, silently dropping the flag- Values for
UpdateInputandOverrideInputare appended without bounds checkingThis causes
nix-storeto receive malformed arguments like"quiet"instead of"--quiet".Replace the manual parsing with category-based helpers:
-func (f *FlakeRef) buildRemoteSystem(s *system.SSHSystem, buildType SystemBuildType, opts *SystemBuildOptions) (string, error) { - nixArgs := nixopts.NixOptionsToArgsList(opts.CmdFlags, opts.NixOpts) - - var evalArgs []string - var buildArgs []string - - for i := 0; i < len(nixArgs); i++ { - opt := nixopts.NixOption(strings.TrimPrefix(nixArgs[i], "--")) - switch opt { - case nixopts.NixOptionRecreateLockFile, - nixopts.NixOptionNoUpdateLockFile, - nixopts.NixOptionNoWriteLockFile, - nixopts.NixOptionNoUseRegistries, - nixopts.NixOptionCommitLockFile: - evalArgs = append(evalArgs, string(opt)) - case nixopts.NixOptionUpdateInput: - evalArgs = append(evalArgs, string(opt), nixArgs[i+1]) - i++ - case nixopts.NixOptionOverrideInput: - evalArgs = append(evalArgs, string(opt), nixArgs[i+1], nixArgs[i+2]) - i += 2 - case nixopts.NixOptionImpure: - default: - buildArgs = append(buildArgs, string(opt)) - } - } +func (f *FlakeRef) buildRemoteSystem(s *system.SSHSystem, buildType SystemBuildType, opts *SystemBuildOptions) (string, error) { + evalArgs := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "lock") + buildArgs := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "build")Note: The "lock" category should contain the flake lock-file related options based on the nixCategory tags in opts.go.
227-238: Critical: nix-store receives malformed buildArgs due to upstream parsing issue.Once the parsing issue on lines 165-185 is fixed (replacing manual loop with
NixOptionsToArgsListByCategory), this code will work correctly. The current implementation will fail becausebuildArgscontains flag names without "--" prefixes.internal/system/system.go (1)
39-61: Fix remote→remote src address and remove panic-prone type asserts.
- Critical: Line 46 builds srcAddr from dest; should use src. This sends closures from the wrong host.
- Replace unsafe
.(*SSHSystem)with a small interface-based helper to avoid panics when other remote System types appear. This also addresses the earlier review note.Apply:
@@ - commandRunner = NewLocalSystem(log) - srcAddr := fmt.Sprintf("ssh://%s", dest.(*SSHSystem).Address()) - destAddr := dest.(*SSHSystem).Address() - argv = append(argv, "--store", srcAddr, "--to", destAddr) + commandRunner = NewLocalSystem(log) + srcRaw, err := systemAddress(src) + if err != nil { + return err + } + destRaw, err := systemAddress(dest) + if err != nil { + return err + } + srcAddr := fmt.Sprintf("ssh://%s", srcRaw) + destAddr := destRaw + argv = append(argv, "--store", srcAddr, "--to", destAddr) @@ - commandRunner = dest - srcAddr := src.(*SSHSystem).Address() - argv = append(argv, "--from", srcAddr) + commandRunner = dest + srcAddr, err := systemAddress(src) + if err != nil { + return err + } + argv = append(argv, "--from", srcAddr) @@ - commandRunner = src - destAddr := dest.(*SSHSystem).Address() - argv = append(argv, "--to", destAddr) + commandRunner = src + destAddr, err := systemAddress(dest) + if err != nil { + return err + } + argv = append(argv, "--to", destAddr)Add helper:
+// systemAddress returns the remote address if sys exposes it. +func systemAddress(sys System) (string, error) { + type addresser interface{ Address() string } + if rs, ok := sys.(addresser); ok { + return rs.Address(), nil + } + return "", fmt.Errorf("system marked remote but does not expose Address()") +}internal/configuration/legacy.go (2)
200-202: Pass copy-category flags to CopyClosures.Remote copy currently ignores user-specified copy options (e.g., substitute-on-destination). Forward the “copy” category.
- if err := system.CopyClosures(localSystem, s, []string{drvPath}); err != nil { + copyFlags := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "copy") + if err := system.CopyClosures(localSystem, s, []string{drvPath}, copyFlags...); err != nil { return "", fmt.Errorf("failed to copy drv to build host: %v", err) }
208-214: Limit realise flags to build-only instead of removing --impure post-hoc.Filter at the source by category “build”; no need to delete flags afterward and avoids leaking unsupported options.
-realiseNixOptions := nixopts.NixOptionsToArgsList(opts.CmdFlags, opts.NixOpts) -realiseNixOptions = slices.DeleteFunc(realiseNixOptions, func(s string) bool { - return s == "--impure" -}) +realiseNixOptions := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "build")
🧹 Nitpick comments (8)
cmd/apply/apply.go (1)
411-430: Document the confirmation skip limitation for remote targets.The FIXME comment indicates that interactive confirmation doesn't work properly for remote targets. While skipping confirmation is a reasonable workaround, this should be documented or tracked.
Consider whether:
- The --yes flag should be automatically implied for remote targets
- This limitation should be mentioned in the man page
- An issue should be opened to track fixing ConfirmationInput for remote scenarios
Do you want me to suggest documentation updates or help draft an issue?
internal/system/system.go (2)
67-76: Log with the actual runner’s logger and decide -v from it.Currently verbosity and CmdArray use src’s logger, which can differ from the host executing the command. After picking the runner, use its logger.
@@ -argv = append(argv, extraArgs...) -if log.GetLogLevel() == logger.LogLevelDebug { - argv = append(argv, "-v") -} +argv = append(argv, extraArgs...) +runLog := commandRunner.Logger() +if runLog.GetLogLevel() == logger.LogLevelDebug { + argv = append(argv, "-v") +} @@ -log.CmdArray(argv) +runLog.CmdArray(argv)
26-33: Optional: preflight check for nix-copy-closure availability.Avoid surprising failures by checking presence on the selected runner.
@@ argv := []string{"nix-copy-closure"} @@ cmd := NewCommand(argv[0], argv[1:]...) -_, err := commandRunner.Run(cmd) +if !commandRunner.HasCommand("nix-copy-closure") { + return fmt.Errorf("nix-copy-closure not found on runner host") +} +_, err := commandRunner.Run(cmd) return errAlso applies to: 74-78
internal/configuration/legacy.go (3)
175-181: Use eval-category flags for nix-instantiate.Instantiate is an evaluation step; prefer “eval” over “build” for correctness and future-proofing.
-extraBuildFlags := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "build") +extraEvalFlags := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "eval") @@ -instantiateArgv = append(instantiateArgv, extraBuildFlags...) +instantiateArgv = append(instituteArgv, extraEvalFlags...)Note: s/instituteArgv/instateArgv – keep the original variable name; only the flags variable changes.
196-197: Defensive check: ensure drvPath is non-empty.If instantiate produced no output but exited 0, fail early with a clear error.
drvPath := strings.TrimSpace(drvPathBuf.String()) +if drvPath == "" { + return "", fmt.Errorf("nix-instantiate returned empty drv path") +}
165-166: Consistency: prefer s.Run over l.Builder.Run.In the local path,
sandl.Builderare the same runner. Usings.Runmakes both local and remote helpers symmetrical.-_, err := l.Builder.Run(cmd) +_, err := s.Run(cmd)internal/cmd/nixopts/convert.go (1)
113-151: Support all integer kinds (int8..int64, uint).*Some options may be typed as non-default ints. Broaden handling to avoid panics/missing args.
- case reflect.Int: - return []string{optionArg, fmt.Sprintf("%d", field.Int())} + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return []string{optionArg, fmt.Sprintf("%d", field.Int())} + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: + return []string{optionArg, fmt.Sprintf("%d", field.Uint())}internal/activation/activation.go (1)
58-63: Simplify MkdirAll error handling; use errors.Is only if you must special-case.MkdirAll returns nil if the dir exists. The equality check against os.ErrExist isn’t needed and won’t match wrapped errors.
-err := s.FS().MkdirAll(constants.NixSystemProfileDirectory, 0o755) -if err != nil { - if err != os.ErrExist { - return fmt.Errorf("failed to create nix system profile directory: %w", err) - } -} +if err := s.FS().MkdirAll(constants.NixSystemProfileDirectory, 0o755); err != nil { + return fmt.Errorf("failed to create nix system profile directory: %w", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
cmd/activate/activate.go(1 hunks)cmd/activate/run.go(3 hunks)cmd/apply/apply.go(14 hunks)cmd/generation/rollback/rollback.go(1 hunks)cmd/generation/switch/switch.go(1 hunks)cmd/info/info.go(1 hunks)doc/man/nixos-cli-apply.1.scd(3 hunks)go.mod(4 hunks)internal/activation/activation.go(5 hunks)internal/cmd/nixopts/convert.go(2 hunks)internal/cmd/nixopts/convert_test.go(2 hunks)internal/cmd/nixopts/nixopts.go(1 hunks)internal/cmd/opts/opts.go(2 hunks)internal/configuration/flake.go(3 hunks)internal/configuration/legacy.go(4 hunks)internal/settings/settings.go(2 hunks)internal/system/fs.go(1 hunks)internal/system/local.go(4 hunks)internal/system/local_fs.go(1 hunks)internal/system/runner.go(1 hunks)internal/system/sftp_fs.go(1 hunks)internal/system/ssh.go(1 hunks)internal/system/system.go(1 hunks)internal/utils/utils.go(2 hunks)module.nix(2 hunks)package.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/system/fs.go
- cmd/activate/run.go
- package.nix
- internal/system/local_fs.go
- internal/system/runner.go
- cmd/info/info.go
- internal/system/local.go
- module.nix
- internal/utils/utils.go
- internal/system/ssh.go
- internal/cmd/nixopts/nixopts.go
- cmd/generation/rollback/rollback.go
- internal/system/sftp_fs.go
🧰 Additional context used
🧬 Code graph analysis (9)
cmd/generation/switch/switch.go (1)
internal/activation/activation.go (3)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(106-128)
internal/configuration/legacy.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (3)
SystemBuildType(71-71)SystemBuildOptions(13-26)SystemBuildTypeSystemActivation(75-75)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(25-33)internal/cmd/nixopts/convert.go (2)
NixOptionsToArgsListByCategory(78-111)NixOptionsToArgsList(49-76)internal/system/runner.go (1)
NewCommand(25-34)internal/system/system.go (1)
CopyClosures(18-79)
internal/cmd/opts/opts.go (1)
internal/configuration/flake.go (1)
FlakeRef(16-22)
internal/cmd/nixopts/convert_test.go (1)
internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(78-111)
internal/system/system.go (5)
internal/system/runner.go (2)
CommandRunner(10-14)NewCommand(25-34)internal/system/fs.go (1)
Filesystem(5-10)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/local.go (1)
NewLocalSystem(18-22)internal/system/ssh.go (1)
SSHSystem(25-33)
internal/configuration/flake.go (8)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (2)
SystemBuildType(71-71)SystemBuildOptions(13-26)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(25-33)internal/cmd/nixopts/convert.go (2)
NixOptionsToArgsList(49-76)NixOptionsToArgsListByCategory(78-111)internal/cmd/nixopts/nixopts.go (9)
NixOption(7-7)NixOptionRecreateLockFile(28-28)NixOptionNoUpdateLockFile(29-29)NixOptionNoWriteLockFile(30-30)NixOptionNoUseRegistries(31-31)NixOptionCommitLockFile(33-33)NixOptionUpdateInput(34-34)NixOptionOverrideInput(35-35)NixOptionImpure(19-19)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/runner.go (1)
NewCommand(25-34)
internal/activation/activation.go (4)
internal/system/system.go (1)
System(9-14)internal/constants/constants.go (2)
DefaultConfigLocation(6-6)NixSystemProfileDirectory(5-5)internal/settings/settings.go (1)
ParseSettingsFromString(191-206)internal/generation/generation.go (2)
GenerationLinkTemplateRegex(146-146)GetProfileDirectoryFromName(19-25)
internal/cmd/nixopts/convert.go (1)
internal/cmd/nixopts/nixopts.go (26)
NixOption(7-7)NixOptionQuiet(10-10)NixOptionPrintBuildLogs(11-11)NixOptionNoBuildOutput(12-12)NixOptionShowTrace(13-13)NixOptionKeepGoing(14-14)NixOptionKeepFailed(15-15)NixOptionFallback(16-16)NixOptionRefresh(17-17)NixOptionRepair(18-18)NixOptionImpure(19-19)NixOptionOffline(20-20)NixOptionNoNet(21-21)NixOptionMaxJobs(23-23)NixOptionCores(24-24)NixOptionLogFormat(26-26)NixOptionOption(27-27)NixOptionBuilders(25-25)NixOptionRecreateLockFile(28-28)NixOptionNoUpdateLockFile(29-29)NixOptionNoWriteLockFile(30-30)NixOptionNoUseRegistries(31-31)NixOptionCommitLockFile(33-33)NixOptionUpdateInput(34-34)NixOptionOverrideInput(35-35)NixOptionInclude(22-22)
cmd/apply/apply.go (9)
internal/system/local.go (2)
NewLocalSystem(18-22)LocalSystem(14-16)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/ssh.go (3)
NewSSHSystem(37-112)ErrAgentNotStarted(35-35)SSHSystem(25-33)internal/utils/utils.go (1)
ExecAsRoot(15-26)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(78-111)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/constants/constants.go (1)
CurrentSystem(7-7)internal/cmd/utils/confirmation.go (1)
ConfirmationInput(11-28)internal/activation/activation.go (7)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(106-128)AddNewNixProfile(68-85)SetNixProfileGeneration(87-104)SwitchToConfiguration(180-205)SwitchToConfigurationOptions(175-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (26)
go.mod (3)
25-27: Update to latest golang.org/x/crypto version v0.43.0 for consistency.v0.41.0 is stable and released, but v0.43.0 is the latest available. v0.41.0 is not affected by CVE-2025-22869 (the DoS in x/crypto/ssh), which was fixed in v0.35.0. Consider bumping to v0.43.0 to stay current with the latest patch; both v0.41.0 and v0.43.0 are secure.
67-67: No issues found; dependency state is correct.The verification confirms
google/shlex v0.0.0-20191202100458-e7afc7fbc510is correctly marked as an indirect dependency. Thego mod graphreveals it is transitively pulled in viacarapace-shlex(and also viawater-sucks/optnix, which is normal deduplication in Go's module system). There is only one version ofgoogle/shlexin the dependency tree, no stale or conflicting implementations exist (the separateanmitsu/go-shlexfromgo-gitis a distinct package), and the dependency state is as intended.
7-7: I have the results for two of the three new dependencies. Let me verify the remaining direct dependency and the security-sensitive crypto library update before generating the final response.All new dependencies verified as secure.
Based on security vulnerability checks:
- koanf/providers/rawbytes v1.0.0 is a stable v1 release, and koanf has no vulnerabilities reported, and its dependent libraries have no vulnerabilities reported.
- pkg/sftp v1.13.10 has no public CVEs; a known DoS issue affected only versions < 1.11.0.
- carapace-shlex v1.0.1 (Go version) has no published security advisories.
- golang.org/x/crypto was upgraded to address DoS vulnerabilities fixed in version 0.35.0 or above, and v0.41.0 exceeds this threshold.
The PR's dependency changes are appropriate for the remote build/activation feature and align with current security best practices.
cmd/activate/activate.go (1)
63-64: LGTM: Verbose flag addition is correct.The flag binding follows standard Cobra patterns and integrates cleanly with the existing command structure.
internal/cmd/opts/opts.go (3)
16-16: LGTM: Verbose field addition supports new logging capabilities.The field integrates cleanly with the activation flow.
24-42: LGTM: New fields support remote build/deploy workflow.The additions of BuildHost, TargetHost, and related fields enable the core remote functionality introduced in this PR.
43-70: LGTM: NixOptions restructuring with category tags enables flexible filtering.The nixCategory tags (build, copy, lock) provide clear categorization for selective option passing to different Nix commands. This aligns well with the NixOptionsToArgsListByCategory helper introduced elsewhere in the PR.
cmd/apply/apply.go (10)
103-104: LGTM: Remote host flags properly declared.Flag declarations follow Cobra conventions and provide clear usage strings.
144-146: LGTM: Mutual exclusivity constraints are appropriate.Preventing --output with remote host flags makes sense since output symlinks are local-only operations.
174-191: LGTM: Target host SSH initialization with proper error handling.The error handling properly distinguishes between SSH agent issues and other connection failures, providing actionable error messages.
193-203: LGTM: NixOS check with appropriate error messages.The error messages have been improved (based on past review feedback) to be explicit and consistent between local and remote cases.
224-241: LGTM: Build host initialization mirrors target host pattern.Consistent SSH connection handling and error reporting.
307-316: Consider: Should nom availability be checked on targetHost for activation?The code checks
buildHost.HasCommand("nom")when determining whether to use nom. However, since nom is used for building (which happens on buildHost), this appears correct. The activation phase doesn't use nom.
435-447: LGTM: Activation helpers properly receive targetHost context.Default specialisation lookup and verification correctly operate on the target system.
449-466: LGTM: Profile management operates on targetHost.Generation number retrieval and profile creation correctly use targetHost.
509-517: LGTM: Final activation uses targetHost.The switch-to-configuration action correctly runs on the target system.
376-383: The review comment is incorrect; the current design is appropriate.CopyClosures already handles the local→local case efficiently with an early return (line 64 of internal/system/system.go), and the overhead of the function call plus two IsRemote() checks is negligible. Adding a guard at the call site would only duplicate this equality logic across files and scatter responsibility. The centralized design in CopyClosures—which handles all four scenarios (remote→remote, remote→local, local→remote, local→local) in one place—is cleaner and more maintainable than fragmenting the logic.
Likely an incorrect or invalid review comment.
doc/man/nixos-cli-apply.1.scd (1)
104-104: LGTM: Dry run constraint updated correctly.The mutual exclusivity with --output is properly documented.
internal/settings/settings.go (1)
191-206: LGTM: String-based settings parser follows existing pattern.The new function correctly mirrors
ParseSettingsbut accepts a string input, using the rawbytes provider appropriately. This enables parsing configuration from file content read via the system abstraction's filesystem interface.internal/cmd/nixopts/convert_test.go (2)
12-17: LGTM: Category tags enable test coverage for filtering logic.The tags mirror the actual NixOptions structure and properly support the new test cases.
102-147: LGTM: Comprehensive test coverage for category-based filtering.The test cases thoroughly exercise the filtering behavior across different categories (build, eval, copy) and verify that:
- Only flags matching the specified category are included
- Flags in multiple categories appear when appropriate
- Non-matching categories return empty results
internal/configuration/flake.go (3)
107-157: LGTM: Local build handler preserves existing logic.The extracted
buildLocalSystemfunction correctly maintains the original build flow with proper logging, environment setup, and command execution.
187-222: LGTM: Remote build workflow correctly implements three-step process.The remote build flow properly:
- Evaluates drvPath locally with lock-file related flags
- Copies the derivation to the build host
- Uses category-based filtering for copyFlags (line 218)
The CopyClosures call correctly uses the category-based helper.
241-254: LGTM: BuildSystem dispatcher correctly routes by system type.The type switch properly delegates to the appropriate build handler based on the system type, with a clear error for unsupported types.
cmd/generation/switch/switch.go (2)
162-174: LGTM: Activation helpers correctly receive system context.The updated calls properly propagate the system instance to enable filesystem abstraction usage in the activation helpers.
176-180: LGTM: Generation number retrieval uses system context.The call correctly passes the system instance as the first parameter.
dccd666 to
9fb8629
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
cmd/activate/run.go (2)
142-146: Guard empty PRE_SWITCH_CHECK commands before using argv[0]When PRE_SWITCH_CHECK splits to zero tokens (e.g., whitespace), the subsequent appends make argv[0] equal the toplevel path, so we end up trying to exec the store directory instead of the intended command. Bail out before dereferencing.
argv := args.Strings() + if len(argv) == 0 { + return fmt.Errorf("invalid PRE_SWITCH_CHECK: no command parsed from %q", cmdStr) + } argv = append(argv, toplevel) argv = append(argv, action.String())
161-164: Guard empty INSTALL_BOOTLOADER commands before using argv[0]The same issue exists here: if INSTALL_BOOTLOADER parses to zero tokens, argv[0] becomes the toplevel path and we exec the wrong binary. Guard against an empty command before building system.NewCommand.
argv := args.Strings() + if len(argv) == 0 { + return fmt.Errorf("invalid INSTALL_BOOTLOADER: no command parsed from %q", cmdStr) + } argv = append(argv, toplevel) cmd := system.NewCommand(argv[0], argv[1:]...)internal/system/sftp_fs.go (1)
37-39: Honor thepermargument inMkdirAll.
Filesystem.MkdirAllpromises to respect the requested mode, but this implementation dropsperm, diverging from the local filesystem contract. Please apply the mode (best-effort viaChmod) after creating the directory.internal/system/system.go (1)
39-60: Replace unchecked*SSHSystemassertions with a safe Address accessor.These casts (
dest.(*SSHSystem),src.(*SSHSystem)) will panic the moment another remoteSystemimplementation appears (tests, future transports, mocks). Please gate them behind an interface check (e.g.type addresser interface { Address() string }) and return a descriptive error if it’s missing.internal/configuration/legacy.go (2)
198-202: Pass copy-category flags through toCopyClosures.Remote copies currently ignore user-specified copy options (e.g.
--substitute-on-destination) because we drop them here. Please forward thecopy-category args toCopyClosuresso the behavior matches the flake path.
206-214: Filternix-storeargs to its supported subset.
realiseNixOptions := nixopts.NixOptionsToArgsList(...)feeds all Nix flags—including lock/update flags andnix build-only switches like--print-build-logs—intonix-store -r.nix-storeerrors on those, so remote builds fail. Restrict this slice to the optionsnix-storeunderstands (max-jobs/cores/fallback/keep-going/keep-failed/--option, etc.) before appending.doc/man/nixos-cli-apply.1.scd (1)
181-181: Duplicate HOST format issue.The HOST format description at line 181 has the same issues flagged for line 92 in the previous review. Both locations should be updated consistently to clarify that username and port are optional, and to note that IPv6 addresses are not currently supported due to parsing limitations.
internal/cmd/nixopts/nixopts.go (1)
94-96: Description mismatch for --print-build-logs.The description still reads "Decrease logging verbosity level" but should describe printing build logs during builds.
🧹 Nitpick comments (4)
internal/activation/activation.go (4)
73-94: Add nil check for options parameter.The function accesses
opts.UseRootCommandandopts.RootCommandwithout verifying thatoptsis non-nil. If a caller passesnilfor options, this will cause a panic.Apply this diff to add a defensive nil check:
func AddNewNixProfile(s system.System, profile string, closure string, opts *AddNewNixProfileOptions) error { + if opts == nil { + opts = &AddNewNixProfileOptions{} + } + if profile != "system" { err := EnsureSystemProfileDirectoryExists(s)
101-122: Add nil check for options parameter.Similar to
AddNewNixProfile, this function accessesopts.UseRootCommandandopts.RootCommandwithout verifying thatoptsis non-nil.Apply this diff to add a defensive nil check:
func SetNixProfileGeneration(s system.System, profile string, genNumber uint64, opts *SetNixProfileGenerationOptions) error { + if opts == nil { + opts = &SetNixProfileGenerationOptions{} + } + if profile != "system" { err := EnsureSystemProfileDirectoryExists(s)
68-71: Consider consolidating duplicate option structs.
AddNewNixProfileOptionsandSetNixProfileGenerationOptionshave identical fields (RootCommandandUseRootCommand). Consider consolidating these into a single shared struct (e.g.,RootCommandOptionsorProfileOperationOptions) to reduce duplication and improve maintainability.Example consolidation:
type ProfileOperationOptions struct { RootCommand string UseRootCommand bool }Then update the function signatures:
func AddNewNixProfile(s system.System, profile string, closure string, opts *ProfileOperationOptions) error func SetNixProfileGeneration(s system.System, profile string, genNumber uint64, opts *ProfileOperationOptions) errorAlso applies to: 96-99
200-229: Add nil check for options parameter.The function accesses multiple fields from
opts(lines 202, 218, 222) without verifying thatoptsis non-nil. This pattern is repeated across the file and should be addressed consistently.Apply this diff to add a defensive nil check:
func SwitchToConfiguration(s system.CommandRunner, generationLocation string, action SwitchToConfigurationAction, opts *SwitchToConfigurationOptions) error { + if opts == nil { + opts = &SwitchToConfigurationOptions{} + } + var commandPath string if opts.Specialisation != "" {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
cmd/activate/activate.go(1 hunks)cmd/activate/run.go(3 hunks)cmd/apply/apply.go(15 hunks)cmd/generation/rollback/rollback.go(3 hunks)cmd/generation/switch/switch.go(3 hunks)cmd/info/info.go(1 hunks)doc/man/nixos-cli-apply.1.scd(6 hunks)go.mod(4 hunks)internal/activation/activation.go(7 hunks)internal/cmd/nixopts/convert.go(2 hunks)internal/cmd/nixopts/convert_test.go(2 hunks)internal/cmd/nixopts/nixopts.go(1 hunks)internal/cmd/opts/opts.go(2 hunks)internal/configuration/flake.go(4 hunks)internal/configuration/legacy.go(4 hunks)internal/settings/settings.go(2 hunks)internal/system/fs.go(1 hunks)internal/system/local.go(4 hunks)internal/system/local_fs.go(1 hunks)internal/system/runner.go(2 hunks)internal/system/sftp_fs.go(1 hunks)internal/system/ssh.go(1 hunks)internal/system/system.go(1 hunks)internal/utils/utils.go(2 hunks)module.nix(2 hunks)package.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/settings/settings.go
- cmd/info/info.go
- internal/utils/utils.go
- package.nix
- internal/system/local_fs.go
- module.nix
- cmd/generation/switch/switch.go
- internal/cmd/nixopts/convert_test.go
- internal/system/local.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T20:32:37.721Z
Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:752-761
Timestamp: 2025-10-20T20:32:37.721Z
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.
Applied to files:
internal/activation/activation.go
🧬 Code graph analysis (10)
internal/system/system.go (5)
internal/system/runner.go (2)
CommandRunner(10-14)NewCommand(25-34)internal/system/fs.go (1)
Filesystem(5-10)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/local.go (1)
NewLocalSystem(18-22)internal/system/ssh.go (1)
SSHSystem(28-36)
internal/configuration/legacy.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (3)
SystemBuildType(71-71)SystemBuildOptions(13-26)SystemBuildTypeSystemActivation(75-75)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(28-36)internal/cmd/nixopts/convert.go (2)
NixOptionsToArgsListByCategory(91-132)NixOptionsToArgsList(54-89)internal/system/runner.go (1)
NewCommand(25-34)internal/system/system.go (1)
CopyClosures(18-79)
cmd/apply/apply.go (9)
internal/cmd/nixopts/nixopts.go (1)
AddSubstituteOnDestinationNixOption(162-166)internal/system/local.go (2)
NewLocalSystem(18-22)LocalSystem(14-16)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/ssh.go (3)
NewSSHSystem(40-115)ErrAgentNotStarted(38-38)SSHSystem(28-36)internal/utils/utils.go (1)
ExecAsRoot(15-26)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/cmd/utils/confirmation.go (1)
ConfirmationInput(11-28)internal/activation/activation.go (9)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)AddNewNixProfile(73-94)AddNewNixProfileOptions(68-71)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)SwitchToConfiguration(200-229)SwitchToConfigurationOptions(193-198)
internal/cmd/nixopts/convert.go (1)
internal/cmd/nixopts/nixopts.go (25)
NixOptionQuiet(10-10)NixOptionPrintBuildLogs(11-11)NixOptionNoBuildOutput(12-12)NixOptionShowTrace(13-13)NixOptionKeepGoing(14-14)NixOptionKeepFailed(15-15)NixOptionFallback(16-16)NixOptionRefresh(17-17)NixOptionRepair(18-18)NixOptionImpure(19-19)NixOptionOffline(20-20)NixOptionNoNet(21-21)NixOptionMaxJobs(23-23)NixOptionCores(24-24)NixOptionLogFormat(26-26)NixOptionOption(27-27)NixOptionBuilders(25-25)NixOptionRecreateLockFile(30-30)NixOptionNoUpdateLockFile(31-31)NixOptionNoWriteLockFile(32-32)NixOptionNoUseRegistries(33-33)NixOptionCommitLockFile(35-35)NixOptionUpdateInput(36-36)NixOptionOverrideInput(37-37)NixOptionInclude(22-22)
internal/configuration/flake.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (2)
SystemBuildType(71-71)SystemBuildOptions(13-26)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(28-36)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/runner.go (1)
NewCommand(25-34)
internal/system/ssh.go (5)
internal/logger/logger.go (1)
Logger(12-27)internal/system/fs.go (1)
Filesystem(5-10)internal/system/sftp_fs.go (1)
SFTPFilesystem(10-12)internal/system/runner.go (1)
Command(16-23)internal/utils/utils.go (1)
Quote(50-60)
internal/activation/activation.go (4)
internal/system/system.go (1)
System(9-14)internal/constants/constants.go (2)
DefaultConfigLocation(6-6)NixSystemProfileDirectory(5-5)internal/settings/settings.go (1)
ParseSettingsFromString(191-206)internal/generation/generation.go (2)
GenerationLinkTemplateRegex(146-146)GetProfileDirectoryFromName(19-25)
internal/cmd/opts/opts.go (1)
internal/configuration/flake.go (1)
FlakeRef(17-23)
cmd/activate/run.go (1)
internal/system/runner.go (1)
NewCommand(25-34)
cmd/generation/rollback/rollback.go (1)
internal/activation/activation.go (5)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (7)
cmd/apply/apply.go (3)
172-193: LGTM: Proper SSH initialization and error handling.Target host initialization correctly handles SSH agent errors and defers connection cleanup.
382-389: Good: Conditional copy with category-based Nix options.The copy operation correctly skips on dry builds and uses category-filtered Nix options for the copy closure command.
417-436: Good: Remote-aware confirmation handling.The confirmation prompt is correctly skipped for remote targets with a helpful debug message, working around the current input limitation noted in the FIXME comment.
internal/system/ssh.go (3)
237-284: LGTM: Safe shell wrapper with proper escaping.The
buildSafeShellWrapperimplementation correctly:
- Validates environment variable names and prohibits NUL bytes
- Uses deterministic ordering for environment exports
- Applies proper shell quoting via
utils.Quoteat all levels- Constructs a safe
sh -cinvocation that exports env vars and executes the commandThis effectively addresses the environment variable propagation issue noted in past reviews.
319-333: LGTM: Command injection vulnerability fixed.Using
shlex.Join([]string{"command", "-v", name})properly escapes the command name, addressing the command injection vulnerability flagged in past reviews.
335-339: LGTM: Proper resource cleanup.The
Close()method now closes all three resources (SFTP client, SSH client, and agent connection), addressing the resource leak issue from past reviews.internal/activation/activation.go (1)
58-63: ****Both
os.MkdirAlland the SFTP client'sMkdirAllimplementation returnnilwhen the path already exists and is a directory—not a different error type. The code's nested error check is defensive but correct: whenMkdirAllsucceeds (whether or not the directory pre-existed),errisnil, so the inner condition is never reached. The abstracted filesystem implementations are compatible with this error handling approach.Likely an incorrect or invalid review comment.
9fb8629 to
9ae685d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
internal/system/system.go (1)
39-65: Unsafe type assertions remain unaddressed.The unchecked type assertions to
*SSHSystemat lines 46, 54, and 59 can panic if another remote System implementation is introduced. While the comment justifies this design choice, a safer pattern using a checked assertion or interface would prevent runtime panics.internal/system/sftp_fs.go (1)
37-39: MkdirAll ignores perm parameter.The
permparameter is currently unused, causing divergence from the interface contract and local filesystem behavior. The SFTP client should be followed by a chmod call to apply the requested permissions.internal/configuration/legacy.go (2)
200-202: CopyClosures omits copy-category flags.Remote copy currently ignores user-specified copy options (e.g.,
--substitute-on-destination). The call should pass copy-category flags similar to the flake path implementation.
206-219: Realise command receives unfiltered Nix options.Line 208 uses
NixOptionsToArgsListwhich passes all flags tonix-store -r, potentially including unsupported options. Should useNixOptionsToArgsListByCategorywith category "build" to filter appropriately.doc/man/nixos-cli-apply.1.scd (2)
92-92: HOST format documentation incomplete.The format should clarify that both username and port are optional:
[user@]host[:port]. Username defaults to current user, and port defaults to 22.
181-181: HOST format documentation incomplete.The format should clarify that both username and port are optional:
[user@]host[:port]. Username defaults to current user, and port defaults to 22.internal/cmd/nixopts/nixopts.go (1)
94-96: Correct the description for--print-build-logs.The description "Decrease logging verbosity level" is incorrect for this flag. It should describe that it prints build logs.
Apply this diff to fix the description:
func AddPrintBuildLogsNixOption(cmd *cobra.Command, dest *bool) { - addNixOptionBool(cmd, dest, NixOptionPrintBuildLogs, "L", "Decrease logging verbosity level") + addNixOptionBool(cmd, dest, NixOptionPrintBuildLogs, "L", "Print build logs during Nix builds") }
🧹 Nitpick comments (1)
internal/cmd/nixopts/convert.go (1)
13-44: Consider using typed NixOption constants directly.The
availableOptionsmap now castsNixOptionconstants to strings. This adds unnecessary conversion overhead. Since the map is used internally andgetNixFlagreturns a string anyway, consider either:
- Keeping the map as
map[string]stringwith direct string literals, or- Changing
getNixFlagto returnNixOptionand adjusting callers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
cmd/activate/activate.go(1 hunks)cmd/activate/run.go(3 hunks)cmd/apply/apply.go(15 hunks)cmd/generation/rollback/rollback.go(3 hunks)cmd/generation/switch/switch.go(3 hunks)cmd/info/info.go(1 hunks)doc/man/nixos-cli-apply.1.scd(6 hunks)go.mod(4 hunks)internal/activation/activation.go(7 hunks)internal/cmd/nixopts/convert.go(2 hunks)internal/cmd/nixopts/convert_test.go(2 hunks)internal/cmd/nixopts/nixopts.go(1 hunks)internal/cmd/opts/opts.go(2 hunks)internal/configuration/flake.go(4 hunks)internal/configuration/legacy.go(4 hunks)internal/settings/settings.go(2 hunks)internal/system/fs.go(1 hunks)internal/system/local.go(4 hunks)internal/system/local_fs.go(1 hunks)internal/system/runner.go(2 hunks)internal/system/sftp_fs.go(1 hunks)internal/system/ssh.go(1 hunks)internal/system/system.go(1 hunks)internal/utils/utils.go(2 hunks)module.nix(2 hunks)package.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/system/runner.go
- internal/settings/settings.go
- package.nix
- cmd/activate/run.go
- module.nix
- cmd/activate/activate.go
- internal/system/fs.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T20:32:37.721Z
Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:752-761
Timestamp: 2025-10-20T20:32:37.721Z
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.
Applied to files:
internal/activation/activation.go
🧬 Code graph analysis (13)
internal/configuration/legacy.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (3)
SystemBuildType(71-71)SystemBuildOptions(13-26)SystemBuildTypeSystemActivation(75-75)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(28-36)internal/cmd/nixopts/convert.go (2)
NixOptionsToArgsListByCategory(91-132)NixOptionsToArgsList(54-89)internal/system/runner.go (1)
NewCommand(25-34)internal/system/system.go (1)
CopyClosures(18-79)
cmd/info/info.go (1)
internal/activation/activation.go (1)
GetCurrentGenerationNumber(124-146)
cmd/generation/switch/switch.go (1)
internal/activation/activation.go (5)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)
internal/system/system.go (5)
internal/system/runner.go (2)
CommandRunner(10-14)NewCommand(25-34)internal/system/fs.go (1)
Filesystem(5-10)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/local.go (1)
NewLocalSystem(18-22)internal/system/ssh.go (1)
SSHSystem(28-36)
internal/cmd/nixopts/convert.go (1)
internal/cmd/nixopts/nixopts.go (25)
NixOptionQuiet(10-10)NixOptionPrintBuildLogs(11-11)NixOptionNoBuildOutput(12-12)NixOptionShowTrace(13-13)NixOptionKeepGoing(14-14)NixOptionKeepFailed(15-15)NixOptionFallback(16-16)NixOptionRefresh(17-17)NixOptionRepair(18-18)NixOptionImpure(19-19)NixOptionOffline(20-20)NixOptionNoNet(21-21)NixOptionMaxJobs(23-23)NixOptionCores(24-24)NixOptionLogFormat(26-26)NixOptionOption(27-27)NixOptionBuilders(25-25)NixOptionRecreateLockFile(30-30)NixOptionNoUpdateLockFile(31-31)NixOptionNoWriteLockFile(32-32)NixOptionNoUseRegistries(33-33)NixOptionCommitLockFile(35-35)NixOptionUpdateInput(36-36)NixOptionOverrideInput(37-37)NixOptionInclude(22-22)
internal/system/local.go (2)
internal/system/fs.go (1)
Filesystem(5-10)internal/system/local_fs.go (5)
LocalFilesystem(7-7)LocalFilesystem(9-11)LocalFilesystem(13-15)LocalFilesystem(17-19)LocalFilesystem(21-23)
cmd/generation/rollback/rollback.go (1)
internal/activation/activation.go (4)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)
internal/system/ssh.go (5)
internal/logger/logger.go (1)
Logger(12-27)internal/system/fs.go (1)
Filesystem(5-10)internal/system/sftp_fs.go (1)
SFTPFilesystem(10-12)internal/system/runner.go (1)
Command(16-23)internal/utils/utils.go (1)
Quote(50-60)
internal/configuration/flake.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (2)
SystemBuildType(71-71)SystemBuildOptions(13-26)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(28-36)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/runner.go (1)
NewCommand(25-34)
internal/cmd/opts/opts.go (1)
internal/configuration/flake.go (1)
FlakeRef(17-23)
cmd/apply/apply.go (9)
internal/cmd/nixopts/nixopts.go (1)
AddSubstituteOnDestinationNixOption(162-166)internal/system/local.go (2)
NewLocalSystem(18-22)LocalSystem(14-16)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/ssh.go (3)
NewSSHSystem(40-122)ErrAgentNotStarted(38-38)SSHSystem(28-36)internal/utils/utils.go (1)
ExecAsRoot(15-26)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/cmd/utils/confirmation.go (1)
ConfirmationInput(11-28)internal/activation/activation.go (9)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)AddNewNixProfile(73-94)AddNewNixProfileOptions(68-71)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)SwitchToConfiguration(200-229)SwitchToConfigurationOptions(193-198)
internal/activation/activation.go (4)
internal/system/system.go (1)
System(9-14)internal/constants/constants.go (2)
DefaultConfigLocation(6-6)NixSystemProfileDirectory(5-5)internal/settings/settings.go (1)
ParseSettingsFromString(191-206)internal/generation/generation.go (2)
GenerationLinkTemplateRegex(146-146)GetProfileDirectoryFromName(19-25)
internal/cmd/nixopts/convert_test.go (1)
internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (50)
go.mod (1)
1-97: Dependency updates align with remote build support.The addition of
pkg/sftpand related dependencies supports the new remote build/activation functionality. The version updates for golang.org/x packages are routine maintenance.internal/cmd/nixopts/convert.go (2)
91-132: Category-based filtering correctly implements nixCategory tag logic.The function properly splits comma-separated categories from struct tags and filters fields using
slices.Index. The verbatim flag handling for options starting with-is consistent withNixOptionsToArgsList.
134-172: Good: deterministic map key ordering ensures reproducible builds.The
expandNixOptionArghelper correctly sorts map keys (lines 157-159) before appending arguments, ensuring consistent command-line argument ordering across invocations.internal/cmd/nixopts/convert_test.go (1)
102-147: Comprehensive test coverage for category-based filtering.The new test function validates filtering across multiple categories (build, eval, copy) and includes an edge case for categories with no matching flags. The test structure mirrors the existing
TestNixOptionsToArgsListpattern.cmd/info/info.go (1)
57-57: Correct integration with system abstraction.Threading the system instance to
GetCurrentGenerationNumberaligns with the broader refactor enabling remote/local filesystem operations.internal/cmd/opts/opts.go (2)
13-17: New Verbose flag enables detailed activation logging.The addition of
VerbosetoActivateOptssupports user-controlled verbosity during activation.
23-73: Well-structured inline NixOptions with consistent category tags.The inline
NixOptionsstruct consolidates all Nix-related flags in one place. ThenixCategorytags (build, copy, lock) enable proper filtering when constructing command arguments for different Nix operations. This refactor improves maintainability compared to the previous separate type.internal/system/local.go (3)
24-26: Filesystem abstraction enables testability and remote/local separation.The
FS()method returns aLocalFilesysteminstance, supporting the broader system abstraction that allows activation code to work uniformly across local and remote systems.
63-70: Proper resource management in IsNixOS.The refactored code correctly opens the file, defers closing, and passes the handle to
parseOSRelease. This is cleaner than the previous approach and enables better testing.
95-110: parseOSRelease refactor improves testability.Accepting an
io.Readerparameter instead of directly opening the file enables unit testing with mock readers and aligns with Go best practices for I/O operations.internal/configuration/flake.go (3)
108-158: Clean extraction of local build logic.The
buildLocalSystemmethod consolidates the local build workflow clearly. Logging vias.Logger()and environment variable propagation (NIXOS_GENERATION_TAG) are correctly implemented.
167-170: Good: --impure correctly moved to eval phase.The code properly removes
--impurefrombuildArgsand adds it toevalArgssince environment variables are accessed during evaluation, not realisation.
226-239: BuildSystem dispatcher correctly routes to per-system builders.The type switch cleanly dispatches to
buildRemoteSystemfor SSH systems andbuildLocalSystemfor local systems, with appropriate error handling for unsupported types.internal/system/local_fs.go (1)
7-23: LGTM!The LocalFilesystem implementation is clean and correctly delegates to the standard library. The value receivers are appropriate for an empty struct, and error propagation is correct.
cmd/generation/switch/switch.go (1)
162-190: LGTM!The activation helper calls correctly propagate the system context, aligning with the updated API signatures. Error handling and control flow remain intact.
cmd/generation/rollback/rollback.go (1)
110-138: LGTM!The activation helper calls are correctly updated to include the system context parameter, maintaining consistency with the refactored API.
internal/system/ssh.go (2)
244-291: LGTM!The
buildSafeShellWrapperimplementation properly validates environment variable names, checks for NUL bytes, uses deterministic ordering, and correctly quotes values using theutils.Quotefunction. This prevents command injection while supporting environment variable propagation.
326-340: LGTM!The
HasCommandimplementation now usesshlex.Jointo properly escape the command name, addressing the command injection vulnerability mentioned in past reviews.cmd/apply/apply.go (20)
4-24: LGTM!The added imports (
errors, aliasedcmdOpts, andcmdUtils) support the new remote build/deploy features and are used appropriately throughout the file.
102-106: LGTM!The new flags (
--remote-root,--build-host,--target-host) are properly declared and their descriptions clearly convey their purpose.
119-119: LGTM!The
AddSubstituteOnDestinationNixOptionis correctly wired for remote copy scenarios.
176-193: LGTM!The SSH target-host construction correctly handles connection errors and provides helpful guidance when the SSH agent is not running. The deferred cleanup ensures proper resource management.
195-205: LGTM!The NixOS verification correctly differentiates error messages between local and remote targets, addressing previous feedback.
216-226: LGTM!The root re-exec logic correctly skips re-execution for remote targets, as the remote host will handle privilege escalation independently.
228-247: LGTM!The build-host construction follows the same pattern as target-host with consistent error handling and resource cleanup.
267-267: LGTM!Setting the builder on the configuration correctly routes build operations through the selected build host.
298-303: LGTM!Channel upgrades correctly run on the local system, as channels are local to the machine running the command.
313-322: LGTM!The
nomavailability check correctly queries the build host and appropriately distinguishes between explicit flag usage (error) and config setting (warning with fallback).
382-389: LGTM!The closure copy step correctly transfers the built system from the build host to the target host, with appropriate category-based option filtering and dry-run handling.
394-397: LGTM!The VM messaging updates correctly inform the user about the script location.
410-415: LGTM!The diff command correctly runs on the target host to compare against its current system state.
417-437: Acknowledge the interim limitation.The confirmation prompt is correctly skipped for remote targets due to stdin handling issues. The FIXME comment documents this limitation.
Please ensure this limitation is tracked as a follow-up task. Consider adding a GitHub issue to track implementing remote-friendly confirmation (e.g., via SSH tty allocation or a separate confirmation mechanism).
441-453: LGTM!The specialisation lookup and verification correctly query the target host's filesystem.
455-459: LGTM!The current generation number is correctly read from the target host.
468-479: LGTM!The profile creation correctly targets the remote host and applies root escalation only when
--remote-rootis specified for remote targets.
502-513: LGTM!The rollback logic consistently applies the same root escalation pattern as profile creation.
531-536: LGTM!The activation command correctly targets the remote host with consistent root escalation handling.
552-579: LGTM!The
upgradeChannelsfunction correctly accepts a system parameter, enabling it to run on the appropriate host.internal/cmd/nixopts/nixopts.go (5)
7-38: LGTM!The
NixOptiontype and constants provide strong typing for Nix options, improving maintainability and reducing the risk of typos.
40-88: LGTM!The internal option-adding functions correctly use the new
NixOptiontype, maintaining type safety throughout the flag registration process.
98-160: LGTM!The remaining option wrappers correctly use the new
NixOptionconstants with appropriate descriptions.
162-166: LGTM!The new
AddSubstituteOnDestinationNixOptioncorrectly wires both the new and deprecated options, providing a smooth migration path.
168-196: LGTM!The lock file and input option wrappers correctly use the new constants and properly handle deprecation for registry-related options.
internal/activation/activation.go (7)
19-33: LGTM!The function correctly uses the system's filesystem abstraction to read configuration, enabling it to work with both local and remote systems.
37-49: LGTM!The specialisation verification correctly uses the system's filesystem abstraction to check for the switch-to-configuration script.
51-66: LGTM!The directory creation correctly uses the system's filesystem abstraction, enabling it to work on remote hosts.
68-94: LGTM!The profile creation function correctly accepts the new options struct and conditionally applies root escalation, supporting remote deployment scenarios.
96-122: LGTM!The generation switching function follows the same pattern as profile creation, with consistent root escalation handling.
124-146: LGTM!The generation number retrieval correctly uses the system's filesystem abstraction to read the profile symlink.
193-229: LGTM!The configuration switch function correctly extends the options struct and conditionally applies root escalation, maintaining consistency with the other activation functions.
9ae685d to
42ae369
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/system/local.go (1)
95-110: Handle scanner errors from/etc/os-release.After switching
parseOSReleaseto use anio.Reader, we no longer surfacebufio.Scannererrors. If the reader hits an I/O failure or a line longer than 64 KiB, the scanner silently stops andIsNixOS()will fall back tofalse, misidentifying the local system. Please propagates.Err()so callers can distinguish “not NixOS” from “failed to read/etc/os-release”.func parseOSRelease(r io.Reader) (map[string]string, error) { values := make(map[string]string) s := bufio.NewScanner(r) s.Split(bufio.ScanLines) for s.Scan() { key, value, found := strings.Cut(s.Text(), "=") if !found { continue } values[key] = value } - - return values, nil + if err := s.Err(); err != nil { + return nil, err + } + + return values, nil }
♻️ Duplicate comments (4)
internal/configuration/flake.go (1)
212-213: Filter buildArgs to nix-store-compatible flags before realising.
buildArgsderived from the "build" category includes flags thatnix-store -rdoes not accept (e.g.,--print-build-logs,--show-trace,--offline,--builders,--log-format). Appending these directly causes remote realisation to fail.internal/system/system.go (1)
46-60: Unsafe type assertions can panic if system types expand.Lines 46, 47, 54, and 59 perform unchecked type assertions to
*SSHSystem. While the current code comment states no other system types will exist, this is fragile and can panic if the codebase evolves.internal/configuration/legacy.go (1)
199-201: Pass copy-category flags to CopyClosures.Remote copy ignores user-specified copy options (e.g.,
--substitute-on-destination). Add the copy-category flags similar to the flake path.Apply this diff:
+ copyFlags := nixopts.NixOptionsToArgsListByCategory(opts.CmdFlags, opts.NixOpts, "copy") - if err := system.CopyClosures(localSystem, s, []string{drvPath}); err != nil { + if err := system.CopyClosures(localSystem, s, []string{drvPath}, copyFlags...); err != nil { return "", fmt.Errorf("failed to copy drv to build host: %v", err) }internal/system/ssh.go (1)
91-94: Validate HOME environment variable before use.Line 91 uses
os.Getenv("HOME")directly without checking if it's empty. If HOME is unset,knownhosts.New()will attempt to read from an invalid path like/.ssh/known_hosts, potentially failing silently or producing confusing errors.Apply this diff to validate HOME before use:
- knownHostsKeyCallback, err := knownhosts.New(filepath.Join(os.Getenv("HOME"), ".ssh", "known_hosts")) + homeDir := os.Getenv("HOME") + if homeDir == "" { + current, err := user.Current() + if err != nil || current.HomeDir == "" { + return nil, fmt.Errorf("cannot determine home directory: HOME not set and user lookup failed") + } + homeDir = current.HomeDir + } + knownHostsKeyCallback, err := knownhosts.New(filepath.Join(homeDir, ".ssh", "known_hosts")) if err != nil { return nil, fmt.Errorf("failed to create known hosts callback: %v", err) }
🧹 Nitpick comments (1)
internal/configuration/legacy.go (1)
164-164: Consider using s.Run(cmd) for consistency.Line 164 calls
l.Builder.Run(cmd)while the flake.go equivalent (line 155) usess.Run(cmd). Sincesis derived froml.Buildervia the type switch inBuildSystem, usings.Run(cmd)would be more consistent with the system-aware pattern.Apply this diff:
- _, err := l.Builder.Run(cmd) + _, err := s.Run(cmd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
cmd/activate/activate.go(1 hunks)cmd/apply/apply.go(15 hunks)cmd/generation/rollback/rollback.go(3 hunks)cmd/generation/switch/switch.go(3 hunks)cmd/info/info.go(1 hunks)doc/man/nixos-cli-apply.1.scd(6 hunks)go.mod(4 hunks)internal/activation/activation.go(7 hunks)internal/cmd/opts/opts.go(2 hunks)internal/configuration/flake.go(4 hunks)internal/configuration/legacy.go(3 hunks)internal/settings/settings.go(2 hunks)internal/system/fs.go(1 hunks)internal/system/local.go(4 hunks)internal/system/local_fs.go(1 hunks)internal/system/runner.go(2 hunks)internal/system/sftp_fs.go(1 hunks)internal/system/ssh.go(1 hunks)internal/system/system.go(1 hunks)internal/utils/utils.go(2 hunks)package.nix(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/system/sftp_fs.go
🚧 Files skipped from review as they are similar to previous changes (4)
- package.nix
- doc/man/nixos-cli-apply.1.scd
- internal/system/runner.go
- internal/utils/utils.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T20:32:37.721Z
Learnt from: water-sucks
PR: nix-community/nixos-cli#94
File: cmd/activate/run.go:752-761
Timestamp: 2025-10-20T20:32:37.721Z
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.
Applied to files:
internal/activation/activation.go
🧬 Code graph analysis (11)
cmd/generation/switch/switch.go (1)
internal/activation/activation.go (5)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)
internal/system/system.go (5)
internal/system/runner.go (2)
CommandRunner(10-14)NewCommand(25-34)internal/system/fs.go (1)
Filesystem(5-10)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/local.go (1)
NewLocalSystem(18-22)internal/system/ssh.go (1)
SSHSystem(28-36)
internal/configuration/flake.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (2)
SystemBuildType(71-71)SystemBuildOptions(13-26)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(28-36)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/runner.go (1)
NewCommand(25-34)
internal/system/ssh.go (5)
internal/logger/logger.go (1)
Logger(12-27)internal/system/fs.go (1)
Filesystem(5-10)internal/system/sftp_fs.go (1)
SFTPFilesystem(10-12)internal/system/runner.go (1)
Command(16-23)internal/utils/utils.go (1)
Quote(50-60)
cmd/generation/rollback/rollback.go (1)
internal/activation/activation.go (5)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)
cmd/info/info.go (1)
internal/activation/activation.go (1)
GetCurrentGenerationNumber(124-146)
internal/configuration/legacy.go (7)
internal/system/local.go (2)
LocalSystem(14-16)NewLocalSystem(18-22)internal/configuration/configuration.go (3)
SystemBuildType(71-71)SystemBuildOptions(13-26)SystemBuildTypeSystemActivation(75-75)internal/logger/logger.go (2)
Logger(12-27)LogLevelDebug(6-6)internal/system/ssh.go (1)
SSHSystem(28-36)internal/cmd/nixopts/convert.go (2)
NixOptionsToArgsListByCategory(91-132)NixOptionsToArgsList(54-89)internal/system/runner.go (1)
NewCommand(25-34)internal/system/system.go (1)
CopyClosures(18-79)
cmd/apply/apply.go (9)
internal/cmd/nixopts/nixopts.go (1)
AddSubstituteOnDestinationNixOption(162-166)internal/system/local.go (2)
NewLocalSystem(18-22)LocalSystem(14-16)internal/system/system.go (2)
System(9-14)CopyClosures(18-79)internal/system/ssh.go (3)
NewSSHSystem(40-122)ErrAgentNotStarted(38-38)SSHSystem(28-36)internal/utils/utils.go (1)
ExecAsRoot(15-26)internal/cmd/nixopts/convert.go (1)
NixOptionsToArgsListByCategory(91-132)internal/generation/diff.go (2)
RunDiffCommand(13-38)DiffCommandOptions(9-11)internal/cmd/utils/confirmation.go (1)
ConfirmationInput(11-28)internal/activation/activation.go (9)
FindDefaultSpecialisationFromConfig(19-33)VerifySpecialisationExists(37-49)GetCurrentGenerationNumber(124-146)AddNewNixProfile(73-94)AddNewNixProfileOptions(68-71)SetNixProfileGeneration(101-122)SetNixProfileGenerationOptions(96-99)SwitchToConfiguration(200-229)SwitchToConfigurationOptions(193-198)
internal/system/local.go (2)
internal/system/fs.go (1)
Filesystem(5-10)internal/system/local_fs.go (5)
LocalFilesystem(7-7)LocalFilesystem(9-11)LocalFilesystem(13-15)LocalFilesystem(17-19)LocalFilesystem(21-23)
internal/activation/activation.go (4)
internal/system/system.go (1)
System(9-14)internal/constants/constants.go (2)
DefaultConfigLocation(6-6)NixSystemProfileDirectory(5-5)internal/settings/settings.go (1)
ParseSettingsFromString(191-206)internal/generation/generation.go (2)
GenerationLinkTemplateRegex(146-146)GetProfileDirectoryFromName(19-25)
internal/cmd/opts/opts.go (1)
internal/configuration/flake.go (1)
FlakeRef(17-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build/Test
🔇 Additional comments (20)
internal/configuration/flake.go (2)
108-158: LGTM! Clean local build refactor.The local build path correctly routes logging and command execution through the system abstraction, maintaining the original logic while enabling multi-system support.
226-239: LGTM! Clean system-aware build dispatcher.The type-switched dispatch correctly routes to the appropriate build handler based on the system type, with a clear error path for unsupported systems.
internal/system/local_fs.go (1)
1-23: LGTM! Clean filesystem abstraction.The
LocalFilesystemimplementation correctly delegates to the standard library and complements the system abstraction layer.cmd/generation/switch/switch.go (1)
162-190: LGTM! Activation API updated correctly.The system context parameter is properly threaded through all activation helper calls, aligning with the broader system-aware refactor.
cmd/generation/rollback/rollback.go (1)
110-168: LGTM! System context threaded correctly.All activation helper calls properly include the system context parameter, maintaining consistency with the broader system-aware refactor.
internal/cmd/opts/opts.go (1)
13-73: LGTM! Clean options refactor with category-based tagging.The inline
NixOptionsstruct withnixCategorytags enables proper category-based filtering throughout the build and activation flows. The new remote-related fields (BuildHost,TargetHost,RemoteRoot) align with the PR objectives.cmd/apply/apply.go (5)
176-247: LGTM! Clean host selection with proper resource management.SSH connections are properly established, error-handled (including SSH agent checks), and closed via defer. The fallback to local system is clear.
216-226: LGTM! Re-exec condition correctly excludes remote targets.The early re-exec for root access correctly checks
!targetHost.IsRemote(), avoiding unnecessary privilege escalation when deploying to a remote machine.
382-389: LGTM! Copy flags properly propagated.The closure copy correctly uses category-based
copyFlagsand passes them toCopyClosures, respecting user-specified options like--substitute-on-destination.
417-437: LGTM! Reasonable workaround for remote confirmation issues.The FIXME comment clearly documents the limitation, and the debug message at line 435 provides visibility. Skipping interactive confirmation for remote targets is a reasonable temporary solution.
439-541: LGTM! Activation flow correctly uses targetHost throughout.All activation helpers properly receive
targetHost, and theUseRootCommandlogic correctly applies privilege escalation only when--remote-rootis specified and the target is remote.internal/system/ssh.go (6)
54-67: LGTM! Robust username extraction with proper error handling.The username extraction correctly uses
u.Username()to avoid password leakage, and properly handlesuser.Current()failure with a fallback to theUSERenvironment variable. Clear error message when both methods fail.
78-87: LGTM! SSH agent connection properly managed.The agent connection is stored in the
connfield (line 29) and properly closed in theClose()method (line 345), preventing the file descriptor leak noted in earlier reviews.
132-234: LGTM! Well-structured remote command execution with proper resource management.The
Run()method correctly:
- Handles SSH session lifecycle with proper cleanup (lines 140-144)
- Uses
buildSafeShellWrapper()for safe environment variable propagation- Allocates PTY only for interactive root commands (lines 165-197)
- Implements signal forwarding with proper goroutine cleanup (lines 203-221)
- Extracts exit codes from SSH exit errors (lines 229-231)
The PTY handling complexity is justified for supporting interactive
sudo,doas, andrun0commands.
244-291: LGTM! Secure shell wrapper implementation for environment variable propagation.
buildSafeShellWrapper()correctly:
- Validates environment variable names against POSIX regex (line 250-252)
- Rejects NUL bytes in both env values and args (lines 253-261)
- Uses deterministic ordering for env exports (lines 263-278)
- Properly escapes all values via
utils.Quote()(lines 272, 283, 290)- Builds a safe
sh -cinvocation withexec "$@"to preserve exit codesThis approach elegantly sidesteps
sshd_configAcceptEnvrequirements while maintaining security.
326-340: LGTM! Command injection vulnerability resolved.
HasCommand()now usesshlex.Join()(line 336) to safely construct the command, preventing the injection vulnerability noted in earlier reviews. The command name is properly escaped before execution.
124-130: LGTM! Supporting methods are clean and correctly implemented.The supporting methods are well-structured:
FS()andLogger()provide clean interface accessIsNixOS()has proper error handling with fallback to/etc/os-releaseparsingClose()properly releases all resources in the correct order (SFTP, SSH client, agent connection)- Helper functions
isTerminal()andisRootCommand()are straightforward and correctAlso applies to: 293-346, 348-365
internal/activation/activation.go (3)
19-49: LGTM! Clean filesystem abstraction in config/verification functions.
FindDefaultSpecialisationFromConfig()andVerifySpecialisationExists()correctly adopt thesystem.Systemparameter and replace direct filesystem calls withs.FS().ReadFile()ands.FS().Stat()respectively. The original logic and error handling are preserved while enabling remote system support.
51-122: LGTM! Consistent refactoring of profile management functions.The profile management functions correctly:
- Accept
system.Systemas the first parameter (lines 51, 73, 101, 124)- Use abstracted filesystem operations via
s.FS()(lines 58, 131)- Introduce option structs (
AddNewNixProfileOptions,SetNixProfileGenerationOptions) with consistentUseRootCommand/RootCommandfields (lines 68-71, 96-99)- Apply root command escalation when needed (lines 88-90, 116-118)
- Preserve original logic while enabling remote system support
The pattern is consistent with
SwitchToConfigurationOptionsand follows the broader system abstraction migration.
193-229: LGTM! SwitchToConfiguration options extended consistently.The
SwitchToConfigurationOptionsstruct is correctly extended withUseRootCommandandRootCommandfields (lines 196-197), and the function applies root escalation when configured (lines 218-220). This follows the same pattern as the other activation functions and maintains backward compatibility.
Description
This adds support for remote activation of NixOS configurations through the
--build-hostand--target-hostparameters. These were originally flags innixos-rebuild.sh, so I looked to mimic that behavior as closely as possible.Closes #114.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores