Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRestructures the repository into a multi-crate Rust workspace under Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "crates/nh (binary)"
participant Core as "nh_core"
participant Remote as "nh_remote"
participant Host as "RemoteHost"
CLI->>Core: parse CLI args (interface -> core arg types)
Core->>Core: validate feature requirements & build command
alt remote build requested
Core->>Remote: init_ssh_control / build_remote(RemoteBuildConfig)
Remote->>Host: establish SSH and run remote build
Host-->>Remote: return build result
Remote-->>Core: forward remote result
else local build
Core->>Core: construct command::Build and execute locally
end
Core-->>CLI: return exit/result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
faukah
force-pushed
the
faukah/push-utlvolmwlukw
branch
from
bcce188 to
1db219e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
crates/nh-core/Cargo.toml (1)
18-27: Move test-only crates to[dev-dependencies]to slim normal builds.
proptest,serial_test, andtempfileare used only in tests. Keeping them in[dependencies]unnecessarily inflates the build surface for non-test builds. Create a[dev-dependencies]section and move these three crates there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-core/Cargo.toml` around lines 18 - 27, The Cargo.toml currently lists proptest, serial_test, and tempfile alongside normal dependencies; move these test-only crates into a new [dev-dependencies] section to avoid inflating standard builds. Edit Cargo.toml to remove the proptest, serial_test, and tempfile entries from the regular dependency list and add them under a new [dev-dependencies] header (keeping their existing configuration keys such as proptest.workspace, serial_test.workspace, tempfile.workspace as appropriate). Leave all other crates (e.g., regex, secrecy, semver, serde_json, thiserror, etc.) in the normal dependencies block unchanged.crates/nh/src/lib.rs (1)
8-10: Duplicate version constants across crates.
NH_VERSIONandNH_REVare defined here and also incrates/nh-core/src/lib.rs. Note thatverify_variables()innh_core::checkslogs the version usingsuper::NH_VERSION(the nh-core version). If these crates are versioned independently, users may see different version strings depending on which code path logs the version.Consider either:
- Using only
nh_core::NH_VERSIONthroughout for consistency, or- Documenting the intentional distinction between binary vs library versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh/src/lib.rs` around lines 8 - 10, This file defines duplicate constants NH_VERSION and NH_REV that also exist in crates/nh-core; to fix, remove or stop exporting the duplicate here and make the binary crate reference nh_core::NH_VERSION and nh_core::NH_REV instead (or, if you intend different values, add clear doc comments and renames to avoid confusion). Update any code that logs or checks versions—specifically verify_variables in nh_core::checks which currently reads super::NH_VERSION—to consistently use nh_core::NH_VERSION (or the new documented symbol) so runtime logs always reflect the single authoritative version constant.crates/nh-nixos/src/args.rs (1)
16-30: Remove commented-out code.This block contains unused commented-out imports that appear to be leftover from development. They should be removed to keep the codebase clean.
🧹 Suggested cleanup
use crate::{ - // Result, - // checks::{ - // DarwinReplFeatures, - // FeatureRequirements, - // FlakeFeatures, - // HomeReplFeatures, - // LegacyFeatures, - // NoFeatures, - // OsReplFeatures, - // }, - // commands::ElevationStrategy, generations::Field, - // remote::RemoteHost, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-nixos/src/args.rs` around lines 16 - 30, The use block in args.rs contains leftover commented-out imports (e.g., Result, checks::DarwinReplFeatures, commands::ElevationStrategy, remote::RemoteHost, and others) that should be removed; edit the use crate::{ ... } statement to delete all commented lines and leave only the actual import(s) you need (generations::Field), then reformat the use declaration so it is clean and compact (e.g., use crate::generations::Field; or a single-brace use with just Field).crates/nh-home/src/home.rs (1)
217-220: Add a safety comment for theunsafeblock.The
env::set_varbecameunsafein Rust 1.80+ due to potential data races when called concurrently with environment reads. Consider adding a// SAFETY:comment explaining why this usage is safe (e.g., single-threaded context, no concurrent environment access).📝 Suggested documentation
if let Some(ext) = &self.backup_extension { info!("Using {} as the backup extension", ext); + // SAFETY: This is called in a single-threaded context before spawning + // the activation process. No concurrent environment access occurs. unsafe { env::set_var("HOME_MANAGER_BACKUP_EXT", ext); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-home/src/home.rs` around lines 217 - 220, The unsafe block around env::set_var("HOME_MANAGER_BACKUP_EXT", ext) needs a // SAFETY: comment explaining why calling the now-unsafe env::set_var here is safe — state the invariant (e.g., this code runs on a single-threaded initialization path, there is no concurrent access to env vars, or any required synchronization is held) and reference the symbol HOME_MANAGER_BACKUP_EXT and the unsafe block so future readers know the concurrency assumption and why no data race can occur; update the comment if the invariant changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/nh-core/src/args.rs`:
- Around line 135-145: The CLI flags commit_lock_file and use_substitutes are
defined on NixBuildPassthroughArgs but never emitted by
generate_passthrough_args(), so the flags are ignored; update
generate_passthrough_args() to append the corresponding "--commit-lock-file" and
"--use-substitutes" switches when NixBuildPassthroughArgs.commit_lock_file or
.use_substitutes are true (respectively), following the same pattern used for
other boolean flags (e.g., no_build_output) and ensure the option names and
NixBuildPassthroughArgs field names are used exactly to locate the correct logic
to modify.
In `@crates/nh-nixos/src/args.rs`:
- Around line 300-311: OsReplArgs::uses_flakes currently treats NH_OS_FLAKE as
present even when it's an empty string by using env::var("NH_OS_FLAKE").is_ok();
change that check to mirror OsRebuildArgs::uses_flakes by using
env::var("NH_OS_FLAKE").is_ok_and(|v| !v.is_empty()) so empty values are
rejected, keeping the existing matches!(self.installable, Installable::Flake {
.. }) fallback unchanged.
In `@crates/nh-remote/Cargo.toml`:
- Around line 16-27: Move the test-only crates out of the regular dependencies:
remove the proptest.workspace and serial_test.workspace entries from the
[dependencies] table and add them under a [dev-dependencies] table instead
(keeping the workspace = true setting). This ensures proptest and serial_test
are only built for tests; update the Cargo.toml by creating or appending to
[dev-dependencies] with proptest.workspace = true and serial_test.workspace =
true.
In `@crates/nh-search/src/args.rs`:
- Around line 27-28: The query Vec<String> field currently allows zero terms;
add a Clap argument constraint to require at least one term by annotating the
query field (pub query: Vec<String>) with a num_args/min_values attribute (e.g.,
num_args = "1.." or min_values = 1 depending on your clap version) so clap will
reject empty input before it reaches the search builder used in search.rs (where
the joined query is passed at the search builder call).
---
Nitpick comments:
In `@crates/nh-core/Cargo.toml`:
- Around line 18-27: The Cargo.toml currently lists proptest, serial_test, and
tempfile alongside normal dependencies; move these test-only crates into a new
[dev-dependencies] section to avoid inflating standard builds. Edit Cargo.toml
to remove the proptest, serial_test, and tempfile entries from the regular
dependency list and add them under a new [dev-dependencies] header (keeping
their existing configuration keys such as proptest.workspace,
serial_test.workspace, tempfile.workspace as appropriate). Leave all other
crates (e.g., regex, secrecy, semver, serde_json, thiserror, etc.) in the normal
dependencies block unchanged.
In `@crates/nh-home/src/home.rs`:
- Around line 217-220: The unsafe block around
env::set_var("HOME_MANAGER_BACKUP_EXT", ext) needs a // SAFETY: comment
explaining why calling the now-unsafe env::set_var here is safe — state the
invariant (e.g., this code runs on a single-threaded initialization path, there
is no concurrent access to env vars, or any required synchronization is held)
and reference the symbol HOME_MANAGER_BACKUP_EXT and the unsafe block so future
readers know the concurrency assumption and why no data race can occur; update
the comment if the invariant changes.
In `@crates/nh-nixos/src/args.rs`:
- Around line 16-30: The use block in args.rs contains leftover commented-out
imports (e.g., Result, checks::DarwinReplFeatures, commands::ElevationStrategy,
remote::RemoteHost, and others) that should be removed; edit the use crate::{
... } statement to delete all commented lines and leave only the actual
import(s) you need (generations::Field), then reformat the use declaration so it
is clean and compact (e.g., use crate::generations::Field; or a single-brace use
with just Field).
In `@crates/nh/src/lib.rs`:
- Around line 8-10: This file defines duplicate constants NH_VERSION and NH_REV
that also exist in crates/nh-core; to fix, remove or stop exporting the
duplicate here and make the binary crate reference nh_core::NH_VERSION and
nh_core::NH_REV instead (or, if you intend different values, add clear doc
comments and renames to avoid confusion). Update any code that logs or checks
versions—specifically verify_variables in nh_core::checks which currently reads
super::NH_VERSION—to consistently use nh_core::NH_VERSION (or the new documented
symbol) so runtime logs always reflect the single authoritative version
constant.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
Cargo.tomlcrates/nh-clean/Cargo.tomlcrates/nh-clean/src/args.rscrates/nh-clean/src/clean.rscrates/nh-core/Cargo.tomlcrates/nh-core/src/args.rscrates/nh-core/src/checks.rscrates/nh-core/src/command.rscrates/nh-core/src/installable.rscrates/nh-core/src/json.rscrates/nh-core/src/lib.rscrates/nh-core/src/update.rscrates/nh-core/src/util.rscrates/nh-darwin/Cargo.tomlcrates/nh-darwin/src/args.rscrates/nh-darwin/src/darwin.rscrates/nh-home/Cargo.tomlcrates/nh-home/src/args.rscrates/nh-home/src/home.rscrates/nh-nixos/Cargo.tomlcrates/nh-nixos/src/args.rscrates/nh-nixos/src/generations.rscrates/nh-nixos/src/lib.rscrates/nh-nixos/src/nixos.rscrates/nh-remote/Cargo.tomlcrates/nh-remote/src/remote.rscrates/nh-search/Cargo.tomlcrates/nh-search/src/args.rscrates/nh-search/src/search.rscrates/nh/Cargo.tomlcrates/nh/src/interface.rscrates/nh/src/lib.rscrates/nh/src/logging.rscrates/nh/src/main.rspackage.nixsrc/interface.rssrc/lib.rsxtask/Cargo.toml
💤 Files with no reviewable changes (2)
- src/lib.rs
- src/interface.rs
Member
|
this PR pushed my grandma off the stairs and shot my dog like that CIA |
Contributor
Author
what |
Member
|
what |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/nh-home/src/home.rs (1)
309-323: Extract duplicatednix evalexistence-check logic into one helper.Line 309 and Line 365 repeat the same command-building/check pattern. A shared helper would reduce drift risk and keep error handling/messages consistent.
Also applies to: 365-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-home/src/home.rs` around lines 309 - 323, Duplicate nix eval command-building and capture logic around Command::new("nix")/with_required_env()/arg("eval")/arg("--apply")/run_capture()/wrap_err is repeated; extract it into a single helper (e.g., fn run_nix_eval_apply(func: &str, extra_args: &[&str], installable: &Installable) -> Result<Output, Error>) and replace both occurrences to call that helper, reusing Installable::Flake.to_args() where needed; ensure the helper preserves the same error context text returned by wrap_err so callers get identical messages and update callers that used check_res to use the helper result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/nh-clean/src/clean.rs`:
- Line 1: Move the `pub mod args;` declaration out of clean.rs and place it in
the crate root (e.g., lib.rs or main.rs) so Rust's module resolution finds
crates/nh-clean/src/args.rs; alternatively, if you must keep the declaration in
clean.rs, add an explicit path attribute (#[path = "../args.rs"]) above the `pub
mod args;` in clean.rs to point to the correct file location; update any uses of
symbols from args (e.g., functions/types referenced in clean.rs) if their
visibility changes after moving the module.
In `@crates/nh-search/src/search.rs`:
- Line 21: The local constant NH_VERSION is defined but the code incorrectly
references crate::NH_VERSION (which doesn't exist); update any references to
crate::NH_VERSION in this module (e.g., the usage at the search implementation
around the version formatting) to use the local NH_VERSION constant instead so
the defined constant is actually used and the crate-root lookup is removed.
---
Nitpick comments:
In `@crates/nh-home/src/home.rs`:
- Around line 309-323: Duplicate nix eval command-building and capture logic
around
Command::new("nix")/with_required_env()/arg("eval")/arg("--apply")/run_capture()/wrap_err
is repeated; extract it into a single helper (e.g., fn run_nix_eval_apply(func:
&str, extra_args: &[&str], installable: &Installable) -> Result<Output, Error>)
and replace both occurrences to call that helper, reusing
Installable::Flake.to_args() where needed; ensure the helper preserves the same
error context text returned by wrap_err so callers get identical messages and
update callers that used check_res to use the helper result.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/nh-clean/src/clean.rscrates/nh-darwin/src/darwin.rscrates/nh-home/src/home.rscrates/nh-search/src/search.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/nh-nixos/src/args.rs (1)
16-30: Remove commented-out dead code.These commented imports appear to be remnants from before the crate split. They should be cleaned up to improve readability.
🧹 Proposed cleanup
use crate::{ - // Result, - // checks::{ - // DarwinReplFeatures, - // FeatureRequirements, - // FlakeFeatures, - // HomeReplFeatures, - // LegacyFeatures, - // NoFeatures, - // OsReplFeatures, - // }, - // commands::ElevationStrategy, generations::Field, - // remote::RemoteHost, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-nixos/src/args.rs` around lines 16 - 30, Remove the commented-out imports in the use crate::{ ... } block (the large commented section including Result, checks::DarwinReplFeatures/FeatureRequirements/FlakeFeatures/HomeReplFeatures/LegacyFeatures/NoFeatures/OsReplFeatures, commands::ElevationStrategy, remote::RemoteHost) so only the actually used symbol generations::Field (and any other live imports) remain; edit the use statement in this module (args.rs) to delete the dead commented lines and tidy spacing to improve readability.crates/nh/src/interface.rs (2)
8-8: Redundant crate import.
use nh_nixos;is unnecessary since Rust 2018+ allows direct use of external crate paths likenh_nixos::args::OsArgs. This is also inconsistent with hownh_home,nh_darwin,nh_search, andnh_cleanare referenced (inline without top-level imports).🧹 Proposed cleanup
use nh_core::{ checks::{FeatureRequirements, NoFeatures}, command::ElevationStrategy, }; -use nh_nixos; use crate::Result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh/src/interface.rs` at line 8, Remove the redundant top-level import "use nh_nixos;" and instead reference the crate inline like the other crates (e.g., nh_nixos::args::OsArgs) to match existing style (nh_home, nh_darwin, nh_search, nh_clean); locate the use statement named "use nh_nixos;" and delete it so all external crates are referenced directly via their paths.
84-96: Add a comment explaining the elevation parameter pattern.The
HomeandSearchvariants intentionally omit theelevationparameter because home-manager operations are per-user (no root needed) and search is a read-only operation. However, this pattern isn't self-documenting. Consider adding a brief inline comment on line 89-95 to clarify why onlyOs,Clean, andDarwinuse elevation whileHomeandSearchdo not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh/src/interface.rs` around lines 84 - 96, Add a brief inline comment in the run(&self, elevation: ElevationStrategy) matching block explaining the elevation parameter pattern: note that Os, Clean, and Darwin variants receive/need elevation (system-wide or root actions) while Home and Search intentionally omit elevation because HomeManager operations are per-user (no root) and Search is read-only; place the comment near the match or above the variant arms referencing the run function and the ElevationStrategy enum so future readers understand why elevation is not passed to Home::run and Search::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 `@crates/nh/src/interface.rs`:
- Around line 99-108: Remove the duplicate UpdateArgs struct from this file (the
UpdateArgs type defined here is dead code); delete the entire UpdateArgs
definition block and ensure callers import & use the canonical type
(nh_core::update::UpdateArgs) instead—verify there are no local references to
the removed symbol in this crate and, if needed, add or adjust a use statement
to reference nh_core::update::UpdateArgs so nh-nixos/nh-home/nh-darwin keep
using the single canonical definition.
---
Nitpick comments:
In `@crates/nh-nixos/src/args.rs`:
- Around line 16-30: Remove the commented-out imports in the use crate::{ ... }
block (the large commented section including Result,
checks::DarwinReplFeatures/FeatureRequirements/FlakeFeatures/HomeReplFeatures/LegacyFeatures/NoFeatures/OsReplFeatures,
commands::ElevationStrategy, remote::RemoteHost) so only the actually used
symbol generations::Field (and any other live imports) remain; edit the use
statement in this module (args.rs) to delete the dead commented lines and tidy
spacing to improve readability.
In `@crates/nh/src/interface.rs`:
- Line 8: Remove the redundant top-level import "use nh_nixos;" and instead
reference the crate inline like the other crates (e.g., nh_nixos::args::OsArgs)
to match existing style (nh_home, nh_darwin, nh_search, nh_clean); locate the
use statement named "use nh_nixos;" and delete it so all external crates are
referenced directly via their paths.
- Around line 84-96: Add a brief inline comment in the run(&self, elevation:
ElevationStrategy) matching block explaining the elevation parameter pattern:
note that Os, Clean, and Darwin variants receive/need elevation (system-wide or
root actions) while Home and Search intentionally omit elevation because
HomeManager operations are per-user (no root) and Search is read-only; place the
comment near the match or above the variant arms referencing the run function
and the ElevationStrategy enum so future readers understand why elevation is not
passed to Home::run and Search::run.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/nh-nixos/src/args.rscrates/nh-nixos/src/nixos.rscrates/nh-search/src/search.rscrates/nh/src/interface.rs
Contributor
Author
|
hey @troutbot, does this look good? |
Closed
2 tasks
13 tasks
NotAShelf
approved these changes
Mar 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ready to review, works great.
Sanity Checking
nix fmtto format my Nix codecargo fmtto format my Rust codecargo clippyand fixed any new linter warnings.logic
description.
x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwinAdd a 👍 reaction to pull requests you find important.
Summary by CodeRabbit
New Features
Refactor