Conversation
cole-h
added
the
upload to s3
|
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:
📝 WalkthroughWalkthroughAdds per-user path constants and switches to PathBuf-based iteration for Nix tree ownership checks; renames and expands ownership-checking logic to consider UID and GID; adjusts filtering to prefix checks and explicitly includes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cole-h
force-pushed
the
fixup-single-user-curing
branch
4 times, most recently
from
7ba308b to
147718e
Compare
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/action/common/provision_nix.rs (1)
174-179:⚠️ Potential issue | 🟠 MajorOwnership enforcement is skipped when only UID is wrong.
The guard runs the walk only when the store’s GID mismatches. If/nix/store(or entries) haveuid != 0but correct GID, the function now returns early and leaves bad ownership despite the “ownership” semantics.✅ Fix: gate on UID or GID mismatch
- let previous_store_group_id = previous_store_metadata.gid(); - if previous_store_group_id != desired_nix_build_group_id { + let previous_store_group_id = previous_store_metadata.gid(); + let previous_store_user_id = previous_store_metadata.uid(); + if previous_store_group_id != desired_nix_build_group_id || previous_store_user_id != 0 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/provision_nix.rs` around lines 174 - 179, The code currently only triggers the WalkDir when previous_store_group_id != desired_nix_build_group_id, skipping fixes if UID differs; change the guard to check either UID or GID mismatch by reading previous_store_metadata.uid() and comparing it to the desired UID (root/0) as well as comparing gid to desired_nix_build_group_id, and only skip the walk when both UID and GID already match; then proceed with walkdir::WalkDir over NIX_STORE_LOCATION to enforce ownership when either value is wrong.src/action/common/create_nix_tree.rs (1)
159-169:⚠️ Potential issue | 🟡 MinorKeep the per-user directory itself eligible for ownership fixes.
filter_entryreturningfalseskips the directory entry itself, so/nix/var/nix/*/per-userwon’t be re-owned even though only its children are exempted by the doc comment. This can leave the parent directory user-owned and contradict the stated rule.🔧 Suggested fix
- .filter_entry(|entry| { - let path = entry.path(); - - if path.starts_with("/nix/var/nix/profiles/per-user") - || path.starts_with("/nix/var/nix/gcroots/per-user") - { - // False means do *not* descend into this directory - // ...which we don't want to do, because the per-user subdirectories are usually owned by that user. - return false; - } - - true - }) + .filter_entry(|entry| { + let path = entry.path(); + let profiles_per_user = std::path::Path::new("/nix/var/nix/profiles/per-user"); + let gcroots_per_user = std::path::Path::new("/nix/var/nix/gcroots/per-user"); + + if path == profiles_per_user || path == gcroots_per_user { + return true; + } + if path.starts_with(profiles_per_user) || path.starts_with(gcroots_per_user) { + // do not descend into per-user subdirectories + return false; + } + true + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/create_nix_tree.rs` around lines 159 - 169, The filter_entry closure currently returns false for paths that start_with the per-user prefixes, which excludes the per-user directory itself; change the logic so the exact per-user directory paths ("/nix/var/nix/profiles/per-user" and "/nix/var/nix/gcroots/per-user") are allowed (return true) but descendants are still pruned (return false when path.starts_with(...) and path != that exact directory). Edit the filter_entry closure in create_nix_tree.rs (the closure using let path = entry.path()) to check for equality with the two per-user directory paths first and return true for those, otherwise return false for their descendants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/action/common/create_nix_tree.rs`:
- Around line 159-169: The filter_entry closure currently returns false for
paths that start_with the per-user prefixes, which excludes the per-user
directory itself; change the logic so the exact per-user directory paths
("/nix/var/nix/profiles/per-user" and "/nix/var/nix/gcroots/per-user") are
allowed (return true) but descendants are still pruned (return false when
path.starts_with(...) and path != that exact directory). Edit the filter_entry
closure in create_nix_tree.rs (the closure using let path = entry.path()) to
check for equality with the two per-user directory paths first and return true
for those, otherwise return false for their descendants.
In `@src/action/common/provision_nix.rs`:
- Around line 174-179: The code currently only triggers the WalkDir when
previous_store_group_id != desired_nix_build_group_id, skipping fixes if UID
differs; change the guard to check either UID or GID mismatch by reading
previous_store_metadata.uid() and comparing it to the desired UID (root/0) as
well as comparing gid to desired_nix_build_group_id, and only skip the walk when
both UID and GID already match; then proceed with walkdir::WalkDir over
NIX_STORE_LOCATION to enforce ownership when either value is wrong.
cole-h
force-pushed
the
fixup-single-user-curing
branch
from
147718e to
5989b3a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/action/common/create_nix_tree.rs (1)
162-201:⚠️ Potential issue | 🔴 CriticalOperator precedence bug + tautological condition in
filter_entry.Two issues in the filter predicate at Lines 165–167:
Tautology:
path != PROFILES_PER_USER || path != GCROOTS_PER_USERis always true — a path cannot be equal to both distinct strings simultaneously. This should use&&:path != PROFILES_PER_USER && path != GCROOTS_PER_USEROperator precedence:
&&binds tighter than||, so the "not-equal" guard only applies to theGCROOTS_PER_USERbranch, leaving thePROFILES_PER_USERbranch unguarded. The whole expression needs outer parentheses around the||.Because of (1), the guard is dead code and the condition effectively becomes
path.starts_with(PROFILES_PER_USER) || path.starts_with(GCROOTS_PER_USER), which excludes the per-user directories themselves (not just their children) from the walk —filter_entryreturningfalseboth prevents descent and drops the entry. In a single-user → multi-user migration scenario (the exact case this PR targets), those directories may be user-owned and will silently remain un-chowned.If the intent is "don't descend into per-user children, but do chown the per-user dirs themselves,"
filter_entryalone can't express that. A straightforward fix: unconditionally exclude them from the walk and chain them like you already do for/nix:Proposed fix
.filter_entry(|entry| { let path = entry.path(); - if path.starts_with(PROFILES_PER_USER) - || path.starts_with(GCROOTS_PER_USER) - && (path != PROFILES_PER_USER || path != GCROOTS_PER_USER) - { + if path.starts_with(PROFILES_PER_USER) || path.starts_with(GCROOTS_PER_USER) { // False means do *not* descend into this directory // ...which we don't want to do, because the per-user subdirectories are usually owned by that user. return false; } true })…and at the chain site:
// Ensure /nix is also owned by 0:0 - .chain(std::iter::once(PathBuf::from("/nix"))); + .chain( + [PROFILES_PER_USER, GCROOTS_PER_USER, "/nix"] + .into_iter() + .map(PathBuf::from), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/create_nix_tree.rs` around lines 162 - 201, The filter_entry closure incorrectly uses a tautological and precedence-misordered expression; replace its predicate with a grouped expression so only per-user children are skipped (not the per-user dirs themselves). Specifically, in the filter_entry closure in create_nix_tree.rs, change the condition to: if (path.starts_with(PROFILES_PER_USER) || path.starts_with(GCROOTS_PER_USER)) && (path != PROFILES_PER_USER && path != GCROOTS_PER_USER) { return false; } so the starts_with checks are grouped and the not-equal checks use &&, ensuring you prevent descent into per-user subdirs but still keep the per-user directories for later chown handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/action/common/create_nix_tree.rs`:
- Around line 162-201: The filter_entry closure incorrectly uses a tautological
and precedence-misordered expression; replace its predicate with a grouped
expression so only per-user children are skipped (not the per-user dirs
themselves). Specifically, in the filter_entry closure in create_nix_tree.rs,
change the condition to: if (path.starts_with(PROFILES_PER_USER) ||
path.starts_with(GCROOTS_PER_USER)) && (path != PROFILES_PER_USER && path !=
GCROOTS_PER_USER) { return false; } so the starts_with checks are grouped and
the not-equal checks use &&, ensuring you prevent descent into per-user subdirs
but still keep the per-user directories for later chown handling.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rustfmt.tomlsrc/action/base/create_or_insert_into_file.rssrc/action/base/create_or_merge_nix_config.rssrc/action/common/create_nix_tree.rssrc/action/common/provision_nix.rs
💤 Files with no reviewable changes (1)
- rustfmt.toml
cole-h
force-pushed
the
fixup-single-user-curing
branch
from
5989b3a to
7581ecc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/action/common/create_nix_tree.rs`:
- Around line 165-167: The conditional in filter_entry wrongly relies on
operator precedence so PROFILES_PER_USER itself is excluded; update the guard
around the starts_with checks so the exclusion (path != PROFILES_PER_USER &&
path != GCROOTS_PER_USER) applies to both branches—i.e., add parentheses
grouping (path.starts_with(PROFILES_PER_USER) ||
path.starts_with(GCROOTS_PER_USER)) && (path != PROFILES_PER_USER && path !=
GCROOTS_PER_USER) in src/action/common/create_nix_tree.rs (referencing the
path.starts_with checks, PROFILES_PER_USER, GCROOTS_PER_USER and the
filter_entry conditional) to ensure the per-user directories themselves are not
skipped.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rustfmt.tomlsrc/action/base/create_or_insert_into_file.rssrc/action/base/create_or_merge_nix_config.rssrc/action/common/create_nix_tree.rssrc/action/common/provision_nix.rs
💤 Files with no reviewable changes (1)
- rustfmt.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/action/base/create_or_insert_into_file.rs
This way we handle the possibility of multiple child directories.
cole-h
force-pushed
the
fixup-single-user-curing
branch
from
7581ecc to
7fefbd6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/action/common/create_nix_tree.rs (1)
82-86:⚠️ Potential issue | 🟡 MinorStale user-facing description after
/nixwas added to the ownership pass.
execute_descriptionstill reads "Synchronize /nix/var ownership" / "Will update existing files in /nix/var", butensure_nix_ownershipnow also covers/nixitself.📝 Proposed fix
- ActionDescription::new( - "Synchronize /nix/var ownership".to_string(), - vec![format!( - "Will update existing files in /nix/var to be owned by User ID 0, Group ID 0" - )], - ), + ActionDescription::new( + "Synchronize /nix ownership".to_string(), + vec![format!( + "Will update /nix and existing files in /nix/var to be owned by User ID 0, Group ID 0" + )], + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/create_nix_tree.rs` around lines 82 - 86, Update the stale user-facing description strings to reflect that ownership is synchronized for both /nix and /nix/var (not just /nix/var): change the title "Synchronize /nix/var ownership" and the detail "Will update existing files in /nix/var to be owned by User ID 0, Group ID 0" to mention both /nix and /nix/var (e.g., "Synchronize /nix and /nix/var ownership" and "Will update existing files in /nix and /nix/var to be owned by User ID 0, Group ID 0") where these strings are defined in create_nix_tree.rs so they match the behavior of ensure_nix_ownership.
♻️ Duplicate comments (1)
src/action/common/create_nix_tree.rs (1)
165-166: Operator-precedence fix correctly applied —starts_withbranches are now properly grouped.Current form
(starts_with(PER_USER) || starts_with(GCROOTS)) && (path != PER_USER && path != GCROOTS)is logically equivalent to the fix proposed in the previous review. BothPROFILES_PER_USERandGCROOTS_PER_USERexact-match entries now correctly pass throughfilter_entryand receive ownership treatment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/create_nix_tree.rs` around lines 165 - 166, The conditional using path.starts_with(PROFILES_PER_USER) || path.starts_with(GCROOTS_PER_USER) grouped with && (path != PROFILES_PER_USER && path != GCROOTS_PER_USER) is correct and matches the intended operator-precedence fix; no code change required — keep the condition as written in create_nix_tree (the expression referencing PROFILES_PER_USER, GCROOTS_PER_USER, and path) so exact-match entries still pass through filter_entry and receive ownership treatment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/action/common/create_nix_tree.rs`:
- Around line 82-86: Update the stale user-facing description strings to reflect
that ownership is synchronized for both /nix and /nix/var (not just /nix/var):
change the title "Synchronize /nix/var ownership" and the detail "Will update
existing files in /nix/var to be owned by User ID 0, Group ID 0" to mention both
/nix and /nix/var (e.g., "Synchronize /nix and /nix/var ownership" and "Will
update existing files in /nix and /nix/var to be owned by User ID 0, Group ID
0") where these strings are defined in create_nix_tree.rs so they match the
behavior of ensure_nix_ownership.
---
Duplicate comments:
In `@src/action/common/create_nix_tree.rs`:
- Around line 165-166: The conditional using path.starts_with(PROFILES_PER_USER)
|| path.starts_with(GCROOTS_PER_USER) grouped with && (path != PROFILES_PER_USER
&& path != GCROOTS_PER_USER) is correct and matches the intended
operator-precedence fix; no code change required — keep the condition as written
in create_nix_tree (the expression referencing PROFILES_PER_USER,
GCROOTS_PER_USER, and path) so exact-match entries still pass through
filter_entry and receive ownership treatment.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rustfmt.tomlsrc/action/base/create_or_insert_into_file.rssrc/action/base/create_or_merge_nix_config.rssrc/action/common/create_nix_tree.rssrc/action/common/provision_nix.rs
💤 Files with no reviewable changes (1)
- rustfmt.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/action/base/create_or_insert_into_file.rs
This was breaking some formatting.
cole-h
force-pushed
the
fixup-single-user-curing
branch
from
7fefbd6 to
31856b5
Compare
This will house the final actions for every planner, allowing for easily extending it in the future.
This will be used in the next commit.
… with new default profile When we migrate from a single-user install to a multi-user, daemon-based install, we need to take special care to make sure that the user's profile doesn't include packages that conflict with the new default profile (most specifically, Nix itself). We don't want a user to be stuck with mismatched client-daemon versions!
The receipt format changed.
cole-h
force-pushed
the
fixup-single-user-curing
branch
from
49c8165 to
7d3d72c
Compare
cole-h
force-pushed
the
fixup-single-user-curing
branch
from
7d3d72c to
fd8343d
Compare
| impl NixCommandExt for tokio::process::Command { | ||
| fn set_nix_options(&mut self) -> Result<&mut tokio::process::Command, Error> { | ||
| Ok(self | ||
| .args(["--option", "substitute", "false"]) |
There was a problem hiding this comment.
Since we're dropping the nss cacert we might want to pass --offline or similar
There was a problem hiding this comment.
Unfortunately, this is used with both the new CLI (nix build) and the old CLI (nix-env).
nix build --offline would work just fine, but nix-env --install etc have no equivalent flag to disable networking:
vin@scadrial ~/w/d/harmonic (7c105d4d3d11 (empty)) 08:45:52 26 Feb 2026 1m 32.6s
;: nix-env --install --offline
error: unknown flag '--offline'
Try 'nix-env --help' for more information.
vin@scadrial ~/w/d/harmonic (7c105d4d3d11 (empty)) [1] 08:46:00 26 Feb 2026 0.4s
;: nix-env --install --no-net
error: unknown flag '--no-net'
Try 'nix-env --help' for more information.
vin@scadrial ~/w/d/harmonic (7c105d4d3d11 (empty)) [1] 08:46:02 26 Feb 2026 19ms
;: nix-env --install --no-networking
error: unknown flag '--no-networking'
Try 'nix-env --help' for more information.
2 participants
Description
Fixes #1594
Fixes #1773
Fixes #1215
Fixes #1765
Fixes #819
Fixes #680
Checklist
cargo fmtnix buildnix flake checkValidating with
install.determinate.systemsIf a maintainer has added the
upload to s3label to this PR, it will become available for installation viainstall.determinate.systems:Summary by CodeRabbit
Refactor
Style
Chores