Skip to content

treewide: cleanup#560

Closed
faukah wants to merge 5 commits intomasterfrom
general-cleanup
Closed

treewide: cleanup#560
faukah wants to merge 5 commits intomasterfrom
general-cleanup

Conversation

@faukah
Copy link
Contributor

@faukah faukah commented Feb 10, 2026

general cleanup. commits are pretty isolated, I'd recommend reviewing them individually.

Sanity Checking

  • I have updated the changelog as per my changes
  • I have tested, and self-reviewed my code
  • Style and consistency
    • I ran nix fmt to format my Nix code
    • I ran cargo fmt to format my Rust code
    • I have added appropriate documentation to new code
    • My changes are consistent with the rest of the codebase
  • Correctness
    • I ran cargo clippy and fixed any new linter warnings.
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas to explain the
      logic
    • I have documented the motive for those changes in the PR body or commit
      description.
  • Tested on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin

Add a 👍 reaction to pull requests you find important.

Summary by CodeRabbit

  • Refactor

    • Unified build-and-activation flow so Boot/Test/Switch/Build behave consistently.
    • Simplified configuration detection and validation for more reliable automatic selection.
    • Streamlined command argument handling and reduced redundant logging for clearer output.
  • Behavior

    • Removed pre-execution message printing; build-related messages are consolidated into single, clearer info logs.
  • Tests

    • Updated tests to align with the new build/activation and logging behavior.

@coderabbitai
Copy link

coderabbitai bot commented Feb 10, 2026

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Removes per-build message storage and related APIs; flattens rebuild args by removing OsRebuildActivateArgs and updating OsSubcommand variants; consolidates NixOS build and activation into build_and_maybe_activate (replacing build_only and removing message from execute_build); restructures home attribute detection and logging.

Changes

Cohort / File(s) Summary
Message removal & logging
src/commands.rs, src/darwin.rs, src/home.rs
Removed message: Option<String>, its builder and uses; moved logging to upfront info! calls and removed inline .message(...) usages.
Interface & args flattening
src/interface.rs
Removed OsRebuildActivateArgs and changed OsSubcommand variants (Switch, Boot, Test) to carry OsRebuildArgs directly; updated feature-requirement checks to use flattened args.
Build + activation refactor
src/nixos.rs
Replaced build_only with build_and_maybe_activate; moved activation logic into OsRebuildArgs (added build_and_maybe_activate, activate_rebuilt_config); removed message param from execute_build; updated dispatch for Boot/Test/Switch/Build and VM/image build callers.
Home attribute detection
src/home.rs
Simplified top-level attribute handling for homeConfigurations; unified explicit/automatic config checks using Installable::to_args() and boolean existence checks; improved error messages and logging.
Minor logging/order adjustments
src/darwin.rs
Reordered/removed duplicate info! logging for Darwin build path.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant CLI as Client (CLI)
    participant Rebuilder as OsRebuildArgs
    participant Builder as Build System / Nix
    participant Remote as Remote Host (SSH)
    participant Store as Nix Store

    CLI->>Rebuilder: invoke build_and_maybe_activate(variant, attrs, elevation)
    Rebuilder->>Builder: execute_build(toplevel, out_path)
    alt remote hosts involved
      Builder->>Remote: upload/store paths / trigger remote build
      Remote->>Builder: build result (store path)
    else local build
      Builder->>Store: build to local store path
      Store-->>Builder: store path
    end
    Builder-->>Rebuilder: optional built path
    alt variant requires activation (Boot/Test/Switch)
      Rebuilder->>Remote: if remote, run activation remotely
      Rebuilder->>Store: if local, perform activation (switch/test/boot)
      Rebuilder-->>CLI: activation result / instructions
    else build-only
      Rebuilder-->>CLI: build result / VM instructions
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • NotAShelf
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'treewide: cleanup' is vague and generic, using non-descriptive terms that don't convey meaningful information about the specific changes in this substantial refactoring. Consider a more specific title that highlights the main change, such as 'Refactor OS build flow to unify build-and-activate' or 'Simplify public API by removing intermediary activation args struct'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch general-cleanup

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@faukah faukah marked this pull request as ready for review February 16, 2026 23:55
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.


In `@src/home.rs`:
- Around line 266-283: The current check in src/home.rs rejects any attribute
path that starts with "homeConfigurations"; change the logic to mirror
nixos/darwin: when attribute[0] == "homeConfigurations" allow attribute.len() ==
2 (i.e., two-element paths like ["homeConfigurations","name"]) but bail only if
attribute.len() > 2 with the existing error message that uses
attribute.join(".") and attribute.get(1).unwrap_or(...); update the conditional
around attribute[0] == "homeConfigurations" to check length and only reject
overly-specific paths while permitting exactly two-element paths.

In `@src/interface.rs`:
- Around line 129-137: Add a user warning when activation-only flags are
supplied to the Build variant: detect OsSubcommand::Build carrying OsRebuildArgs
and check the activation-only fields (show_activation_logs, install_bootloader,
no_validate, target_host) on that OsRebuildArgs; if any are set, emit the same
style of warning used for the existing --ask/--dry warning so users know those
flags will be ignored for Build. Locate the Build match arm handling
OsSubcommand::Build in interface.rs and add the check and warning there, reusing
the same logger/message format used for the ask/dry warning.

In `@src/nixos.rs`:
- Around line 408-419: The current conditional treats OsSubcommand::Build the
same as BuildVm, causing a misleading "--ask has no effect as dry run was
requested" warning and a duplicate warning for Build; change the logic so the
outer if checks only for dry run or VM variant (e.g., if self.common.dry ||
matches!(variant, BuildVm)), but move the warn!("--ask has no effect as dry run
was requested") inside the self.common.dry branch (only emit when
self.common.dry is true and self.common.ask), and keep the VM instructions logic
as if matches!(variant, BuildVm) && !self.common.dry {
print_vm_instructions(&out_path); } to avoid duplicate/misleading warnings for
Build while preserving VM behavior.
- Around line 438-639: activate_rebuilt_config is large and duplicates
RemoteHost::parse calls; refactor by extracting the local and remote activation
branches into helper methods (e.g., perform_local_activation and
perform_remote_activation) and parse the target host once into a local variable
when self.target_host.is_some() to reuse across the Test/Switch and Boot/Switch
branches; update calls that currently invoke RemoteHost::parse(target_host)
(seen in activate_rebuilt_config and the remote::activate_remote invocations) to
use the single parsed RemoteHost, and move variant-specific command construction
and elevation logic into the new helpers to reduce nesting and duplication while
preserving behavior for Test, Switch, and Boot.
- Around line 537-586: The duplicate debug messages are caused because the debug
block inside the if let Test | Switch branch logs "Completed {variant:?}..." for
Switch and then the final unconditional debug at the end logs the same message
again; remove (or move) the debug calls inside the Test | Switch branch (the
debug that references actual_store_path and out_path) so only the final
end-of-method debug remains (keep the final debug that uses
actual_store_path/out_path), or alternatively change the inner debug to only run
for Test by checking variant == Test; refer to the match/branch using Test,
Switch, Boot, the local variables actual_store_path and out_path, and the debug!
macro.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed.


In `@src/interface.rs`:
- Around line 129-137: Add a user warning when activation-only flags are
supplied to the Build variant: detect OsSubcommand::Build carrying OsRebuildArgs
and check the activation-only fields (show_activation_logs, install_bootloader,
no_validate, target_host) on that OsRebuildArgs; if any are set, emit the same
style of warning used for the existing --ask/--dry warning so users know those
flags will be ignored for Build. Locate the Build match arm handling
OsSubcommand::Build in interface.rs and add the check and warning there, reusing
the same logger/message format used for the ask/dry warning.

In `@src/nixos.rs`:
- Around line 438-639: activate_rebuilt_config is large and duplicates
RemoteHost::parse calls; refactor by extracting the local and remote activation
branches into helper methods (e.g., perform_local_activation and
perform_remote_activation) and parse the target host once into a local variable
when self.target_host.is_some() to reuse across the Test/Switch and Boot/Switch
branches; update calls that currently invoke RemoteHost::parse(target_host)
(seen in activate_rebuilt_config and the remote::activate_remote invocations) to
use the single parsed RemoteHost, and move variant-specific command construction
and elevation logic into the new helpers to reduce nesting and duplication while
preserving behavior for Test, Switch, and Boot.
- Around line 537-586: The duplicate debug messages are caused because the debug
block inside the if let Test | Switch branch logs "Completed {variant:?}..." for
Switch and then the final unconditional debug at the end logs the same message
again; remove (or move) the debug calls inside the Test | Switch branch (the
debug that references actual_store_path and out_path) so only the final
end-of-method debug remains (keep the final debug that uses
actual_store_path/out_path), or alternatively change the inner debug to only run
for Test by checking variant == Test; refer to the match/branch using Test,
Switch, Boot, the local variables actual_store_path and out_path, and the debug!
macro.
src/nixos.rs (2)

438-639: activate_rebuilt_config — activation logic is sound but the method is large.

The method handles confirmation, remote copy, validation, profile resolution, and activation for three variants (Test, Switch, Boot) across local and remote paths. The control flow is correct — Switch runs both the Test | Switch and Boot | Switch blocks, mirroring standard nixos-rebuild switch behavior.

A few observations for future consideration:

  • The method is ~200 lines with multiple nested conditionals and variant-based branches. Extracting the local vs. remote activation into helper methods would improve readability.
  • RemoteHost::parse(target_host) is called up to 3 times (lines 463, 539, 590) for the Switch variant — could be resolved once and reused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nixos.rs` around lines 438 - 639, activate_rebuilt_config is large and
duplicates RemoteHost::parse calls; refactor by extracting the local and remote
activation branches into helper methods (e.g., perform_local_activation and
perform_remote_activation) and parse the target host once into a local variable
when self.target_host.is_some() to reuse across the Test/Switch and Boot/Switch
branches; update calls that currently invoke RemoteHost::parse(target_host)
(seen in activate_rebuilt_config and the remote::activate_remote invocations) to
use the single parsed RemoteHost, and move variant-specific command construction
and elevation logic into the new helpers to reduce nesting and duplication while
preserving behavior for Test, Switch, and Boot.

537-586: Duplicate debug logging for the Switch variant.

For Switch, both if let Test | Switch (line 537) and if let Boot | Switch (line 588) blocks execute. The debug log at lines 576-585 fires inside the first block, then lines 631-637 fire again unconditionally at the end of the method. This results in two identical "Completed Switch operation..." messages.

Consider removing the debug log inside the Test | Switch block (lines 576-585) since the one at lines 631-637 covers all activation variants.

Also applies to: 631-637

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nixos.rs` around lines 537 - 586, The duplicate debug messages are caused
because the debug block inside the if let Test | Switch branch logs "Completed
{variant:?}..." for Switch and then the final unconditional debug at the end
logs the same message again; remove (or move) the debug calls inside the Test |
Switch branch (the debug that references actual_store_path and out_path) so only
the final end-of-method debug remains (keep the final debug that uses
actual_store_path/out_path), or alternatively change the inner debug to only run
for Test by checking variant == Test; refer to the match/branch using Test,
Switch, Boot, the local variables actual_store_path and out_path, and the debug!
macro.
src/interface.rs (1)

129-137: Clean type consolidation — OsRebuildActivateArgs removed, variants now use OsRebuildArgs directly.

The flattening is correct. The uses_flakes() call at line 132 now resolves directly on OsRebuildArgs (defined at line 274), matching the unified type carried by Switch, Boot, and Test.

One minor observation: the Build variant (line 179) now carries OsRebuildArgs which includes activation-only fields like show_activation_logs, install_bootloader, no_validate, and target_host. These are silently ignored for Build, which is fine, but users may pass them without realizing they have no effect. Consider emitting a warning similar to the --ask/--dry warning at line 85 if activation-specific flags are set for Build.

Also applies to: 170-176

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/interface.rs` around lines 129 - 137, Add a user warning when
activation-only flags are supplied to the Build variant: detect
OsSubcommand::Build carrying OsRebuildArgs and check the activation-only fields
(show_activation_logs, install_bootloader, no_validate, target_host) on that
OsRebuildArgs; if any are set, emit the same style of warning used for the
existing --ask/--dry warning so users know those flags will be ignored for
Build. Locate the Build match arm handling OsSubcommand::Build in interface.rs
and add the check and warning there, reusing the same logger/message format used
for the ask/dry warning.

Comment on lines 266 to 283
if !attribute.is_empty() {
// Check if the path is too specific
if attribute[0] == "homeConfigurations" {
if attribute.len() > 2 {
bail!(
"Attribute path is too specific: {}. Home Manager only allows \
configuration names. Please either:\n 1. Use the flake \
reference without attributes (e.g., '.')\n 2. Specify only \
the configuration name (e.g., '.#{}')",
attribute.join("."),
attribute.get(1).unwrap_or(&"<unknown>".to_string())
);
}
} else if attribute.len() > 1 {
// User provided ".#myconfig" or similar - prepend homeConfigurations
attribute.insert(0, String::from("homeConfigurations"));
// Re-validate after prepending
if attribute.len() > 2 {
bail!(
"Attribute path is too specific: {}. Home Manager only allows \
configuration names. Please either:\n 1. Use the flake \
reference without attributes (e.g., '.')\n 2. Specify only \
the configuration name (e.g., '.#{}')",
attribute.join("."),
attribute.get(1).unwrap_or(&"<unknown>".to_string())
);
}
bail!(
"Attribute path is too specific: {}. Home Manager only allows \
configuration names. Please either:\n 1. Use the flake \
reference without attributes (e.g., '.')\n 2. Specify only the \
configuration name (e.g., '.#{}')",
attribute.join("."),
attribute.get(1).unwrap_or(&"<unknown>".to_string())
);
}

debug!(
"Using explicit attribute path from installable: {:?}",
attribute
"Using explicit attribute path from installable: {}",
attribute.join(".")
);
return Ok(res);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd /tmp && git log --oneline -n 20 -- src/home.rs 2>/dev/null || echo "Git not available or file not in repo"

Repository: nix-community/nh

Length of output: 97


🏁 Script executed:

# Search for attribute handling patterns in nixos.rs and darwin.rs
rg -n -A 20 'if attribute\[0\] == "nixosConfigurations"' src/nixos.rs

Repository: nix-community/nh

Length of output: 909


🏁 Script executed:

rg -n -A 20 'if attribute\[0\] == "darwinConfigurations"' src/darwin.rs

Repository: nix-community/nh

Length of output: 940


🏁 Script executed:

# Also check the context around line 279-283 in home.rs to see the alternative code path
sed -n '266,290p' src/home.rs

Repository: nix-community/nh

Length of output: 898


🏁 Script executed:

# Look for any condition checking attribute length for homeConfigurations
rg -n -B 5 -A 15 'homeConfigurations' src/home.rs

Repository: nix-community/nh

Length of output: 1581


Fix attribute length check for homeConfigurations paths to match nixosConfigurations and darwinConfigurations behavior.

The current code unconditionally rejects any path starting with homeConfigurations, but nixos.rs (lines 1215–1230) and darwin.rs (lines 284–304) both allow 2-element paths (e.g., ["nixosConfigurations", "hostname"]) and only reject longer ones. The home.rs code should do the same:

if attribute[0] == "homeConfigurations" {
  if attribute.len() == 2 {
    // homeConfigurations.configname - fine
  } else if attribute.len() > 2 {
    bail!(...)
  }
}

Currently, users passing .#homeConfigurations.myuser get rejected, while the nixos and darwin modules accept the equivalent .#nixosConfigurations.hostname.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/home.rs` around lines 266 - 283, The current check in src/home.rs rejects
any attribute path that starts with "homeConfigurations"; change the logic to
mirror nixos/darwin: when attribute[0] == "homeConfigurations" allow
attribute.len() == 2 (i.e., two-element paths like
["homeConfigurations","name"]) but bail only if attribute.len() > 2 with the
existing error message that uses attribute.join(".") and
attribute.get(1).unwrap_or(...); update the conditional around attribute[0] ==
"homeConfigurations" to check length and only reject overly-specific paths while
permitting exactly two-element paths.

Comment on lines +408 to +419
if self.common.dry || matches!(variant, Build | BuildVm) {
if self.common.ask {
warn!("--ask has no effect as dry run was requested");
}

// For VM builds, print instructions on how to run the VM
if matches!(variant, BuildVm) && !self.common.dry {
print_vm_instructions(&out_path);
}

return Ok(());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Misleading warning text for non-dry Build variant.

When variant is Build (not a dry run), the condition at line 408 is true because of matches!(variant, Build | BuildVm), but the warning at line 410 says "--ask has no effect as dry run was requested". For the Build subcommand, this isn't a dry run — it's a build-only command where activation isn't performed.

Note that for OsSubcommand::Build, a warning is already emitted at lines 84-86 in the dispatch, so this would also be a duplicate warning.

Suggested fix
     if self.common.dry || matches!(variant, Build | BuildVm) {
-      if self.common.ask {
-        warn!("--ask has no effect as dry run was requested");
+      if self.common.ask && self.common.dry {
+        warn!("--ask has no effect as dry run was requested");
       }

       // For VM builds, print instructions on how to run the VM
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.common.dry || matches!(variant, Build | BuildVm) {
if self.common.ask {
warn!("--ask has no effect as dry run was requested");
}
// For VM builds, print instructions on how to run the VM
if matches!(variant, BuildVm) && !self.common.dry {
print_vm_instructions(&out_path);
}
return Ok(());
}
if self.common.dry || matches!(variant, Build | BuildVm) {
if self.common.ask && self.common.dry {
warn!("--ask has no effect as dry run was requested");
}
// For VM builds, print instructions on how to run the VM
if matches!(variant, BuildVm) && !self.common.dry {
print_vm_instructions(&out_path);
}
return Ok(());
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nixos.rs` around lines 408 - 419, The current conditional treats
OsSubcommand::Build the same as BuildVm, causing a misleading "--ask has no
effect as dry run was requested" warning and a duplicate warning for Build;
change the logic so the outer if checks only for dry run or VM variant (e.g., if
self.common.dry || matches!(variant, BuildVm)), but move the warn!("--ask has no
effect as dry run was requested") inside the self.common.dry branch (only emit
when self.common.dry is true and self.common.ask), and keep the VM instructions
logic as if matches!(variant, BuildVm) && !self.common.dry {
print_vm_instructions(&out_path); } to avoid duplicate/misleading warnings for
Build while preserving VM behavior.

@NotAShelf NotAShelf changed the title all: cleanup treewide: cleanup Feb 17, 2026
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/home.rs (1)

266-283: ⚠️ Potential issue | 🟠 Major

Restore explicit Home configuration attribute handling.

Line 266+ now rejects homeConfigurations.<name> and returns early for other non-empty attributes, which can break explicit config selection and skip activationPackage suffixing when push_drv is true.

Suggested fix
       if !attribute.is_empty() {
-        // Check if the path is too specific
-        if attribute[0] == "homeConfigurations" {
-          bail!(
-            "Attribute path is too specific: {}. Home Manager only allows \
-             configuration names. Please either:\n  1. Use the flake \
-             reference without attributes (e.g., '.')\n  2. Specify only the \
-             configuration name (e.g., '.#{}')",
-            attribute.join("."),
-            attribute.get(1).unwrap_or(&"<unknown>".to_string())
-          );
-        }
-
-        debug!(
-          "Using explicit attribute path from installable: {}",
-          attribute.join(".")
-        );
-        return Ok(res);
+        if attribute[0] == "homeConfigurations" {
+          if attribute.len() > 2 {
+            bail!(
+              "Attribute path is too specific: {}. Home Manager only allows \
+               configuration names. Please either:\n  1. Use the flake \
+               reference without attributes (e.g., '.')\n  2. Specify only the \
+               configuration name (e.g., '.#{}')",
+              attribute.join("."),
+              attribute.get(1).unwrap_or(&"<unknown>".to_string())
+            );
+          }
+        } else if attribute.len() == 1 {
+          attribute.insert(0, String::from("homeConfigurations"));
+        }
+
+        if push_drv {
+          attribute.extend(toplevel.clone());
+        }
+        debug!("Using explicit attribute path from installable: {}", attribute.join("."));
+        return Ok(res);
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/home.rs` around lines 266 - 283, The current block prematurely bails on
attribute paths starting with "homeConfigurations" and returns early for any
non-empty attribute, which prevents selecting explicit configurations and
prevents adding the activationPackage suffix when push_drv is true; update the
logic in the attribute-handling section to allow explicit
"homeConfigurations.<name>" by extracting the configuration name from
attribute[1] when present (falling back to a clear error only if the name is
missing), avoid the unconditional return Ok(res) for non-empty attributes, and
instead set/modify res appropriately so downstream code can append the
activationPackage suffix when push_drv is true (referencing variables/values
attribute, res, push_drv, and activationPackage).
src/nixos.rs (1)

412-415: ⚠️ Potential issue | 🟡 Minor

Fix misleading --ask warning for non-dry build flows.

At Line 414, the warning says dry run was requested, but this branch also executes for non-dry Build/BuildVm. For Build, it also duplicates the warning already emitted in run (Line 84–86).

Suggested fix
-    if self.common.dry || matches!(variant, Build | BuildVm) {
-      if self.common.ask {
+    if self.common.dry || matches!(variant, BuildVm) {
+      if self.common.dry && self.common.ask {
         warn!("--ask has no effect as dry run was requested");
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nixos.rs` around lines 412 - 415, The warning currently fires when either
self.common.dry is true or variant matches Build|BuildVm, which produces a
misleading message and duplicates the warning already emitted for Build flows;
change the condition so the warn!("--ask has no effect as dry run was
requested") is only executed when self.common.dry && self.common.ask are both
true (i.e., only for actual dry runs), and remove/avoid emitting it for the
matches!(variant, Build | BuildVm) branch to prevent duplication with the
existing warning emitted earlier in run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/nixos.rs`:
- Around line 580-589: The premature/duplicate "Completed {variant:?}" debug
logs should be removed from the early branch so completion is only logged after
boot activation finishes; specifically, remove or defer the debug! calls that
print "Completed {variant:?} operation with store path: {store_path:?}" / "with
local output path: {out_path:?}" that occur when actual_store_path is Some/None
and instead emit a single debug! after the activation/boot path completes (the
later completion log around the Switch activation code). Update the activation
flow around the Switch handling to log once (using the existing completion
message) after the boot activation/activation_result code paths finish to avoid
reporting success prematurely.

---

Duplicate comments:
In `@src/home.rs`:
- Around line 266-283: The current block prematurely bails on attribute paths
starting with "homeConfigurations" and returns early for any non-empty
attribute, which prevents selecting explicit configurations and prevents adding
the activationPackage suffix when push_drv is true; update the logic in the
attribute-handling section to allow explicit "homeConfigurations.<name>" by
extracting the configuration name from attribute[1] when present (falling back
to a clear error only if the name is missing), avoid the unconditional return
Ok(res) for non-empty attributes, and instead set/modify res appropriately so
downstream code can append the activationPackage suffix when push_drv is true
(referencing variables/values attribute, res, push_drv, and activationPackage).

In `@src/nixos.rs`:
- Around line 412-415: The warning currently fires when either self.common.dry
is true or variant matches Build|BuildVm, which produces a misleading message
and duplicates the warning already emitted for Build flows; change the condition
so the warn!("--ask has no effect as dry run was requested") is only executed
when self.common.dry && self.common.ask are both true (i.e., only for actual dry
runs), and remove/avoid emitting it for the matches!(variant, Build | BuildVm)
branch to prevent duplication with the existing warning emitted earlier in run.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d040081 and db343b7.

📒 Files selected for processing (5)
  • src/commands.rs
  • src/darwin.rs
  • src/home.rs
  • src/interface.rs
  • src/nixos.rs
💤 Files with no reviewable changes (1)
  • src/commands.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/interface.rs

Comment on lines +580 to +589
if let Some(store_path) = actual_store_path {
debug!(
"Completed {variant:?} operation with store path: {store_path:?}"
);
} else {
debug!(
"Completed {variant:?} operation with local output path: \
{out_path:?}"
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid premature/duplicate “completed” logs in activation flow.

For Switch, the block at Line 580 logs completion before boot activation runs (Line 592+), and then logs completion again at Line 635. This can report success before the operation actually finishes.

Suggested fix
-      if let Some(store_path) = actual_store_path {
-        debug!(
-          "Completed {variant:?} operation with store path: {store_path:?}"
-        );
-      } else {
-        debug!(
-          "Completed {variant:?} operation with local output path: \
-           {out_path:?}"
-        );
-      }

Also applies to: 635-641

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nixos.rs` around lines 580 - 589, The premature/duplicate "Completed
{variant:?}" debug logs should be removed from the early branch so completion is
only logged after boot activation finishes; specifically, remove or defer the
debug! calls that print "Completed {variant:?} operation with store path:
{store_path:?}" / "with local output path: {out_path:?}" that occur when
actual_store_path is Some/None and instead emit a single debug! after the
activation/boot path completes (the later completion log around the Switch
activation code). Update the activation flow around the Switch handling to log
once (using the existing completion message) after the boot
activation/activation_result code paths finish to avoid reporting success
prematurely.

@faukah
Copy link
Contributor Author

faukah commented Feb 27, 2026

shut up roomba @coderabbitai

whoops, my bad, coderabbit is an actual user... Didn't mean to do that.

@coderabbitai
Copy link

coderabbitai bot commented Feb 27, 2026

@faukah, understood! I'll pause automatic reviews on this PR. You can ask me to resume anytime.

✅ Actions performed

Reviews paused.

@coderabbitai
Copy link

coderabbitai bot commented Feb 27, 2026

Tip

For best results, initiate chat on the files or code changes.

@faukah, no worries at all! I'm CodeRabbit, the automated review bot for this repository (hence the [bot] tag). I understand things can get a bit overwhelming with automated reviews sometimes! 😊

I've already paused reviews on this PR as requested. Whenever you'd like me to resume reviewing, just let me know!

@faukah faukah closed this Mar 8, 2026
@faukah faukah deleted the general-cleanup branch March 8, 2026 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant