nh-core/installable: fix nushell completions#585
Merged
Conversation
nushell completions generated by clap treat the `installable` argument as a string and threfore does not expand ~ (the home directory) to its full path. This commit adds a value_parser of type PathBufValueParser to fix this. Fixes completions and shell expansion for nushell completions.
WalkthroughThis pull request restructures the project from a monolithic single-crate layout into a multi-crate workspace. CLI argument definitions previously consolidated in Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Main as crates/nh (main)
participant Interface as crates/nh/interface
participant CoreCmd as nh_core/command
participant SubCmd as Subcommand<br/>(Darwin/Home/NixOS)
participant FeatureCheck as Feature<br/>Requirements
participant Logic as Command<br/>Logic
User->>Main: nh os switch ~/Config
Main->>Interface: Parse CLI (Main struct)
Interface->>Interface: Extract NHCommand
Interface->>FeatureCheck: get_feature_requirements()
FeatureCheck-->>Interface: Feature requirements
Interface->>SubCmd: run(elevation)
SubCmd->>FeatureCheck: Validate features
FeatureCheck-->>SubCmd: Approved
SubCmd->>CoreCmd: Load pathbuf-parsed installable
CoreCmd-->>SubCmd: Expanded ~/Config to /home/user/Config
SubCmd->>Logic: Execute with resolved path
Logic-->>User: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/nh-core/Cargo.toml (1)
18-24: Moveproptestandserial_testto[dev-dependencies].Both dependencies are used exclusively within
#[cfg(test)]test modules and should not be listed in the main dependencies of a shared core crate.Proposed manifest change
[dependencies] clap.workspace = true color-eyre.workspace = true dix.workspace = true inquire.workspace = true nix.workspace = true -proptest.workspace = true regex.workspace = true secrecy.workspace = true semver.workspace = true serde_json.workspace = true -serial_test.workspace = true shlex.workspace = true subprocess.workspace = true tempfile.workspace = true thiserror.workspace = true tracing.workspace = true which.workspace = true yansi.workspace = true +[dev-dependencies] +proptest.workspace = true +serial_test.workspace = true🤖 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 - 24, Move the test-only crates `proptest` and `serial_test` out of the shared crate's main dependency section and into a `[dev-dependencies]` table in Cargo.toml: delete the `proptest.workspace = true` and `serial_test.workspace = true` entries from the current list and add them under a new or existing `[dev-dependencies]` section (e.g. `proptest = { workspace = true }` and `serial_test = { workspace = true }`) so they are only enabled for cfg(test) builds; keep other workspace entries (regex, secrecy, semver, serde_json, shlex) unchanged.
🤖 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 39-40: The help text for the DiffType::Auto variant contains a
typo ("if the of the") in the doc comment that will appear in CLI docs; update
the docstring associated with the DiffType::Auto enum variant in args.rs to a
grammatically correct sentence (e.g., "Display package diff only if the current
and deployed configuration match" or similar) so the CLI help shows the
corrected wording and tense.
In `@crates/nh-nixos/src/args.rs`:
- Around line 292-293: The current flake-detection check treats NH_OS_FLAKE=""
as enabled (env::var(...).is_ok()) but OsRebuildArgs::uses_flakes() expects a
non-empty value; make the semantics consistent by changing the detection to
require a non-empty env var value (e.g. mirror OsRebuildArgs::uses_flakes()
behavior) — locate the unconditional env::var("NH_OS_FLAKE").is_ok() check and
replace it with a check that the variable exists and is not empty (use env::var
or env::var_os and ensure the returned string/OsString is non-empty) so both
code paths use the same non-empty semantics.
- Around line 203-213: The pattern match on Installable in
OsRebuildArgs::uses_flakes moves the non-Copy value out of &self; change the
match to borrow the field (use matches!(&self.common.installable,
Installable::Flake { .. })) and apply the same fix to the other occurrence that
matches Installable (the other matches! call referencing
self.common.installable) so both use a borrowed reference
(&self.common.installable) instead of attempting to move the value.
In `@crates/nh-remote/Cargo.toml`:
- Around line 20-23: Move the test-only crates out of the main dependency list
into dev-dependencies: remove or relocate the proptest and serial_test entries
from [dependencies] in Cargo.toml and add them under [dev-dependencies] so they
are only compiled for tests; ensure the versions/feature flags remain identical
and run cargo test to verify usage in the #[cfg(test)] module in
crates::nh-remote::src::remote.rs (references: proptest, serial_test) still
resolves.
In `@crates/nh-search/src/search.rs`:
- Line 21: NH_VERSION is set from this crate’s own CARGO_PKG_VERSION so the HTTP
User-Agent string can be wrong after the crate split; change NH_VERSION to
prefer an externally-provided workspace/CLI version (e.g. an env var set by the
top-level build or a build.rs that exports the CLI release version) and fall
back to env!("CARGO_PKG_VERSION") if unset. Update the constant NH_VERSION and
the code that composes/sends the User-Agent (the place that formats
"nh/<version>") to use that new source so the reported version matches the CLI
release instead of the nh-search crate version.
---
Nitpick comments:
In `@crates/nh-core/Cargo.toml`:
- Around line 18-24: Move the test-only crates `proptest` and `serial_test` out
of the shared crate's main dependency section and into a `[dev-dependencies]`
table in Cargo.toml: delete the `proptest.workspace = true` and
`serial_test.workspace = true` entries from the current list and add them under
a new or existing `[dev-dependencies]` section (e.g. `proptest = { workspace =
true }` and `serial_test = { workspace = true }`) so they are only enabled for
cfg(test) builds; keep other workspace entries (regex, secrecy, semver,
serde_json, shlex) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02b001da-54b5-408d-ae32-6e0d7a006604
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
CHANGELOG.mdCargo.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
Comment on lines
+39
to
+40
| /// Display package diff only if the of the | ||
| /// current and the deployed configuration matches |
There was a problem hiding this comment.
Fix typo in DiffType::Auto help text.
Line 39 says “if the of the”, which will show up in CLI docs/help text.
✏️ Suggested patch
- /// Display package diff only if the of the
+ /// Display package diff only if the diff of the📝 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
| /// Display package diff only if the of the | |
| /// current and the deployed configuration matches | |
| /// Display package diff only if the diff of the | |
| /// current and the deployed configuration matches |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/nh-core/src/args.rs` around lines 39 - 40, The help text for the
DiffType::Auto variant contains a typo ("if the of the") in the doc comment that
will appear in CLI docs; update the docstring associated with the DiffType::Auto
enum variant in args.rs to a grammatically correct sentence (e.g., "Display
package diff only if the current and deployed configuration match" or similar)
so the CLI help shows the corrected wording and tense.
Comment on lines
+203
to
+213
| impl OsRebuildArgs { | ||
| #[must_use] | ||
| pub fn uses_flakes(&self) -> bool { | ||
| // Check environment variables first | ||
| if env::var("NH_OS_FLAKE").is_ok_and(|v| !v.is_empty()) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check installable type | ||
| matches!(self.common.installable, Installable::Flake { .. }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file at the specified lines
head -n 300 crates/nh-nixos/src/args.rs | tail -n 100 | cat -nRepository: nix-community/nh
Length of output: 3110
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '203,213p' crates/nh-nixos/src/args.rsRepository: nix-community/nh
Length of output: 360
🏁 Script executed:
# Read the second location mentioned
sed -n '288,298p' crates/nh-nixos/src/args.rsRepository: nix-community/nh
Length of output: 329
🏁 Script executed:
# Find Installable enum definition
rg -n "enum Installable" crates/nh-nixos/src/args.rsRepository: nix-community/nh
Length of output: 42
🏁 Script executed:
# Search for Installable enum definition in the file
rg -n "Installable" crates/nh-nixos/src/args.rs | head -20Repository: nix-community/nh
Length of output: 293
🏁 Script executed:
# Find where Installable is defined
rg -n "enum Installable\|struct Installable" crates/nh-nixos/src/Repository: nix-community/nh
Length of output: 42
🏁 Script executed:
# Get more context around Installable definition
rg -B 5 -A 10 "enum Installable" crates/nh-nixos/src/Repository: nix-community/nh
Length of output: 42
🏁 Script executed:
# Find the installable module
find crates/nh-nixos/src -name "*installable*"Repository: nix-community/nh
Length of output: 42
🏁 Script executed:
# Search for Installable definition across the codebase
rg -n "enum Installable" crates/nh-nixos/Repository: nix-community/nh
Length of output: 42
🏁 Script executed:
# Try searching in lib.rs or mod.rs files
rg -B 5 -A 15 "enum Installable" crates/Repository: nix-community/nh
Length of output: 1080
Add borrowed reference to Installable pattern matches.
Lines 212 and 297 pattern-match on Installable without borrowing. Since Installable is not Copy, this would move the value out of &self, which is invalid. Both matches need to borrow the field:
Suggested fix
- matches!(self.common.installable, Installable::Flake { .. })
+ matches!(&self.common.installable, Installable::Flake { .. })- matches!(self.installable, Installable::Flake { .. })
+ matches!(&self.installable, Installable::Flake { .. })📝 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
| impl OsRebuildArgs { | |
| #[must_use] | |
| pub fn uses_flakes(&self) -> bool { | |
| // Check environment variables first | |
| if env::var("NH_OS_FLAKE").is_ok_and(|v| !v.is_empty()) { | |
| return true; | |
| } | |
| // Check installable type | |
| matches!(self.common.installable, Installable::Flake { .. }) | |
| } | |
| impl OsRebuildArgs { | |
| #[must_use] | |
| pub fn uses_flakes(&self) -> bool { | |
| // Check environment variables first | |
| if env::var("NH_OS_FLAKE").is_ok_and(|v| !v.is_empty()) { | |
| return true; | |
| } | |
| // Check installable type | |
| matches!(&self.common.installable, Installable::Flake { .. }) | |
| } |
🤖 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 203 - 213, The pattern match on
Installable in OsRebuildArgs::uses_flakes moves the non-Copy value out of &self;
change the match to borrow the field (use matches!(&self.common.installable,
Installable::Flake { .. })) and apply the same fix to the other occurrence that
matches Installable (the other matches! call referencing
self.common.installable) so both use a borrowed reference
(&self.common.installable) instead of attempting to move the value.
Comment on lines
+292
to
+293
| if env::var("NH_OS_FLAKE").is_ok() { | ||
| return true; |
There was a problem hiding this comment.
Use consistent env-var semantics for flake detection.
Line 292 treats NH_OS_FLAKE="" as enabled, while OsRebuildArgs::uses_flakes() requires a non-empty value. This inconsistency can trigger unexpected feature checks in repl mode.
🔧 Suggested patch
- if env::var("NH_OS_FLAKE").is_ok() {
+ if env::var("NH_OS_FLAKE").is_ok_and(|v| !v.is_empty()) {
return true;
}🤖 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 292 - 293, The current
flake-detection check treats NH_OS_FLAKE="" as enabled (env::var(...).is_ok())
but OsRebuildArgs::uses_flakes() expects a non-empty value; make the semantics
consistent by changing the detection to require a non-empty env var value (e.g.
mirror OsRebuildArgs::uses_flakes() behavior) — locate the unconditional
env::var("NH_OS_FLAKE").is_ok() check and replace it with a check that the
variable exists and is not empty (use env::var or env::var_os and ensure the
returned string/OsString is non-empty) so both code paths use the same non-empty
semantics.
Comment on lines
+20
to
+23
| proptest.workspace = true | ||
| secrecy.workspace = true | ||
| serial_test.workspace = true | ||
| shlex.workspace = true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the current state of Cargo.toml
cat crates/nh-remote/Cargo.toml | head -40Repository: nix-community/nh
Length of output: 652
🏁 Script executed:
# Run the verification script provided in the review
rg -nP --type=rust -C3 '\b(proptest|serial_test)\b' crates/nh-remote/src/remote.rs
rg -n --type=rust -C2 '#\[cfg\(test\)\]' crates/nh-remote/src/remote.rsRepository: nix-community/nh
Length of output: 668
🏁 Script executed:
# Also search for any other files in the crate that might use these dependencies
fd --type f -name "*.rs" crates/nh-remote/src/ | while read file; do
if rg -q '\b(proptest|serial_test)\b' "$file"; then
echo "=== $file ==="
rg -nP -C2 '\b(proptest|serial_test)\b' "$file" | head -30
fi
doneRepository: nix-community/nh
Length of output: 290
🏁 Script executed:
# Check if there's any non-test usage (outside #[cfg(test)])
# Search for proptest/serial_test patterns and verify they're within test blocks
rg -nP --type=rust '\b(proptest|serial_test)\b' crates/nh-remote/src/ -B 20 | head -100Repository: nix-community/nh
Length of output: 2929
Move proptest and serial_test to dev-dependencies.
These test-only dependencies are currently in the [dependencies] section but are only used within the #[cfg(test)] module in crates/nh-remote/src/remote.rs (lines 1860–1861, 1865, and 2391). Moving them to [dev-dependencies] reduces the production dependency surface.
♻️ Proposed manifest change
[dependencies]
color-eyre.workspace = true
inquire.workspace = true
nh-core.workspace = true
-proptest.workspace = true
secrecy.workspace = true
-serial_test.workspace = true
shlex.workspace = true
signal-hook.workspace = true
subprocess.workspace = true
tracing.workspace = true
which.workspace = true
+
+[dev-dependencies]
+proptest.workspace = true
+serial_test.workspace = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/nh-remote/Cargo.toml` around lines 20 - 23, Move the test-only crates
out of the main dependency list into dev-dependencies: remove or relocate the
proptest and serial_test entries from [dependencies] in Cargo.toml and add them
under [dev-dependencies] so they are only compiled for tests; ensure the
versions/feature flags remain identical and run cargo test to verify usage in
the #[cfg(test)] module in crates::nh-remote::src::remote.rs (references:
proptest, serial_test) still resolves.
| use yansi::{Color, Paint}; | ||
|
|
||
| use crate::{Result, interface}; | ||
| const NH_VERSION: &str = env!("CARGO_PKG_VERSION"); |
There was a problem hiding this comment.
User-Agent version source is likely misleading after crate split.
Line 21 uses this crate’s CARGO_PKG_VERSION, and Line 177 sends it as nh/<version>. With crates/nh-search/Cargo.toml at 0.1.0, this can report nh/0.1.0 instead of the CLI release version.
💡 Suggested fix direction
-const NH_VERSION: &str = env!("CARGO_PKG_VERSION");
+// Prefer a shared NH version source (workspace-level constant or value passed
+// from the top-level CLI crate) to keep User-Agent aligned with actual NH release.Also applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/nh-search/src/search.rs` at line 21, NH_VERSION is set from this
crate’s own CARGO_PKG_VERSION so the HTTP User-Agent string can be wrong after
the crate split; change NH_VERSION to prefer an externally-provided
workspace/CLI version (e.g. an env var set by the top-level build or a build.rs
that exports the CLI release version) and fall back to env!("CARGO_PKG_VERSION")
if unset. Update the constant NH_VERSION and the code that composes/sends the
User-Agent (the place that formats "nh/<version>") to use that new source so the
reported version matches the CLI release instead of the nh-search crate version.
NotAShelf
approved these changes
Mar 13, 2026
Member
|
I don't really know how nushell works, so I'll trust your judgement. |
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.
depends on #580, closes #584.
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
Release Notes
New Features
Bug Fixes
Chores