Refactor setup commands and enhance version management features

[tonythethompson]

This pull request introduces robust support for migration journal handling during the legacy Nushell (Nu) single-binary to versioned layout transition. It ensures that migration journals are properly surfaced, reconciled, and repaired by numan doctor --fix, addressing both pending and malformed journal states. Additionally, the tests and documentation are updated to reflect and verify these behaviors.

Migration journal handling and recovery:

• src/cmd/doctor.rs: Surfaces unreadable or malformed migration-journal.json files as journal.migration_invalid findings with error severity and a manual repair tier, ensuring users are notified and guided to delete stale journals. Pending migration journals are now reported as journal.migration_pending with auto repair via numan use or doctor --fix. The repair logic reacquires the root mutation lock and reconciles the migration state, cleaning up both the journal and any empty subdirectories. [1] [2]

• src/cmd/doctor.rs: Adds comprehensive tests for migration journal states, including detection of pending journals, successful reconciliation with doctor --fix, and error reporting for malformed journals.

Documentation updates:

• src/AGENTS.md, docs/plans/consolidated-multi-repo-roadmap.md, tests/AGENTS.md: Documents the migration journal’s role, its reconciliation process, and the associated repair hints and error handling. Updates clarify that numan use now triggers legacy migration when needed and that backfill waves will be driven by this mechanism. [1] [2] [3] [4]

Test and codebase improvements:

• src/cmd/doctor.rs: Refactors test utilities for restoring the PATH environment variable, replacing a custom mutex and guard with a shared utility. [1] [2] [3]

Minor fixes and consistency:

• src/cmd/doctor.rs: Updates function signatures and repair function references for clarity and correctness, such as using setup::execute_nu_repair as the default for repair operations. [1] [2]
• src/cmd/remove.rs: Adds a missing import (std::io::IsTerminal).

These changes significantly improve the reliability and user experience of migration state handling and recovery during Nu version transitions.

[cursor]

Addressed remaining still-valid review findings on this branch (pushed through 7f58734).

Fixed: bootstrap selective active_nu_binary errors + latest-resolve gate + normalized VERSION; setup single lock / off-tree preserve / PathRestoreGuard; doctor Applied/Skipped/Failed + schema_version invalid coverage; use lock tests + skip redundant rewrite; snapshot --yes downstream asserts; migrate pipe-drain + foreign-content deferral + docs/tests; version_manager/paths/migration_journal/hints/confirm/AGENTS/test updates.

Skipped:

• migration_journal doubled backslash: already fixed on branch.
• force acceptance skip_path: true: rejected by product (setup nu use cannot combine with --skip-path); tests are #[ignore] and restore PATH; unit path uses PathRestoreGuard.

Validation: cargo fmt, cargo clippy -- -D warnings, full cargo test green.

[cursor]

Verified the Codex/Qodo/Cubic/Greptile findings against current HEAD and fixed only what was still valid (3de0849).

Already fixed (skipped):

• Qodo unsafe journal auto-repair: PendingMigration::load already rejects unsafe versions → journal.migration_invalid
• Qodo dangling off-tree in execute_latest: already gated on is_file(); added self-heal regression test
• Qodo/Cubic real-Nu tests: already #[ignore]
• Cubic interactive remove confirm: already has confirm_or_bail
• Cubic bare setup nu --yes older install: already resolves latest before gate
• Greptile remove_managed_nu off-tree wipe: already preserves live binary_path

Fixed now:

• Cubic: symlinked tools ancestor escape → assert_managed_nushell_layout (leaf + containment)
• Cubic: symlinked version dir before rename → refuse + tests
• Cubic: doctor --fix ordering → migration reconcile before off-PATH Nu repair

Validation: cargo fmt, cargo clippy -- -D warnings, full cargo test green.

[cursor]

Merged origin/master into this branch (1f9281f) and resolved conflicts.

Simple (auto-combined): remove.rs, snapshot.rs, paths.rs, version_manager.rs, migration_journal.rs, confirm.rs, fs_safety.rs, hints.rs, test_paths.rs, doctor_test.rs, acceptance/model.rs, AGENTS.md.

Complicated (resolved with explicit choices):

• doctor.rs: master's --scan DoctorArgs + PR83 migration-before-off-path repair ordering; off-path use_existing stays fail-closed (yes=false)
• setup.rs: master's snapshot-before-mutation + PR83 off-tree should_clear / single lock / PathRestoreGuard
• use_cmd.rs: list stays read-only (no lock/migrate); latest preserves live off-tree and self-heals dangling
• bootstrap.rs: PR83 exact latest-dest gate (not any-version heuristic) + selective dangling fallback
• migrate_legacy.rs: master's preserve-existing-active-marker + PR83 layout/symlink hardening
• setup_nu_test.rs: aligned with those bootstrap/setup choices; keep #[ignore] real-Nu tests

Validation: cargo fmt, cargo clippy -- -D warnings, full cargo test green.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

src/nu/bootstrap.rs (2)

758-792: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Pinned short-circuit can persist one version while pointing PATH at another.

Inside this branch dest.is_file() is already true, so the requested version exists on disk. The effective chain still prefers the active marker, then latest_installed_version, and only falls back to dest. When the requested version differs from the active or latest version, the two writes disagree:

• Line 797 and Line 799 prepend and persist effective, which is the other version's binary.
• Line 805 writes normalized_version, which is the requested version.

Example: numan setup nu 0.113.0 --yes with 0.113.0 installed and 0.114.0 active leaves the marker at 0.113.0 while ~/.local/bin/nu resolves to 0.114.0.

Since execute_nu_setup now converts the unpinned flow into a pinned one, dest is always the intended target here. Use dest directly for PATH handling when normalized_version is Some, and keep the active/latest fallback only for the None (legacy placeholder) case. Add a test covering the mismatch case.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/nu/bootstrap.rs` around lines 758 - 792, The already-installed
short-circuit in the setup flow uses the active/latest binary for pinned
installs, which can disagree with the requested version recorded by
normalized_version. Update the effective-path selection to use dest directly
whenever normalized_version is Some, retaining the active/latest fallback only
when it is None; ensure PATH prepend/persistence and the installed marker all
target the requested binary. Add a test covering an installed requested version
that differs from the active version.

---

1271-1325: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

This inline unit test spawns a real nu process and reads PATH without the guard.

Two problems:

1. validate_nushell_binary(p) at Line 1277 and inside register_existing_nu execute the copied host Nushell binary. Inline unit tests must not spawn a real nu. Move this case to tests/ with the #[ignore = "requires real Nu binary on $PATH — run in platform acceptance job"] marker used in tests/setup_nu_test.rs, or inject a validation seam.
2. The test snapshots PATH at Line 1297 and compares at Line 1314 without holding PathRestoreGuard. src/util/test_paths.rs serializes PATH access through PATH_MUTEX. A sibling test that mutates PATH in parallel makes this assertion flaky.

As per coding guidelines: "Unit tests must use FakeCandidateRunner or injectable registrars and must not spawn a real nu process." and "Real-Nu acceptance tests must be marked #[ignore]".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/nu/bootstrap.rs` around lines 1271 - 1325, Move
register_existing_nu_refuses_non_tty_without_yes_before_path_mutation out of the
inline unit tests into the real-Nu acceptance tests, marking it #[ignore =
"requires real Nu binary on $PATH — run in platform acceptance job"] like
tests/setup_nu_test.rs; alternatively inject a validation seam using
FakeCandidateRunner so no real nu process runs. If the test remains inline,
guard all PATH snapshot and comparison access with PathRestoreGuard/PATH_MUTEX
to prevent concurrent PATH mutations.

Source: Coding guidelines

src/cmd/setup.rs (2)

1019-1035: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Tests that drive execute_use_existing edit the developer shell profile. The shared root cause is execute_use_existing at Line 513 of src/cmd/setup.rs, which hardcodes skip_path: false. register_existing_nu then calls persist_path_dir, and PathRestoreGuard restores only the process PATH, not the profile edit.

• src/cmd/setup.rs#L1019-L1035: run these inline unit tests through a seam that sets skip_path: true, and apply the same change to the cases at Line 1062 and Line 1083.
• tests/setup_nu_test.rs#L141-L195: confirm this test carries the #[ignore = "requires real Nu binary on $PATH — run in platform acceptance job"] marker used at Line 368 and Line 430, so the profile write happens only in the acceptance job.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cmd/setup.rs` around lines 1019 - 1035, Update the inline tests invoking
execute_use_existing in src/cmd/setup.rs ranges 1019-1035, 1062, and 1083 to use
a seam that sets skip_path: true, preventing profile edits while preserving test
behavior. In tests/setup_nu_test.rs range 141-195, ensure the test has the same
ignore marker used by the acceptance-only cases at lines 368 and 430, so profile
writes occur only in the platform acceptance job.

Source: Coding guidelines

---

563-566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicate non-TTY guard.

Line 532 already calls require_tty_or_yes(yes, "managed Nushell removal") at function entry. Line 566 repeats the same call with the same label. The second call is unreachable as a distinct gate and it emits a second audit line for one operation.

♻️ Proposed cleanup

-    // Fail closed in non-interactive sessions without `--yes`. Removal wipes
-    // the managed tree; `confirm_or_bail`'s non-TTY auto-confirm must not
-    // silently proceed.
-    require_tty_or_yes(yes, "managed Nushell removal")?;
     confirm_or_bail(

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cmd/setup.rs` around lines 563 - 566, Remove the duplicate
require_tty_or_yes call near the managed Nushell removal logic, keeping the
existing guard at the function entry. Preserve the single initial validation and
avoid emitting a second audit record for the same operation.

AGENTS.md (1)

152-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the new destructive-operation confirmation convention.

remove.rs and snapshot.rs delete/rollback now require interactive confirmation or --yes before destructive work. "Key Conventions" and "Architecture Rules" do not mention this. Add a bullet describing when confirm_or_bail / require_tty_or_yes_with_seam gates apply, alongside the existing snapshot-before-mutation rule.

As per coding guidelines: "Update documentation and AGENTS.md when project structure or conventions change."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@AGENTS.md` around lines 152 - 160, Update AGENTS.md’s Key Conventions and
Architecture Rules documentation to describe the destructive-operation
confirmation convention: remove and snapshot delete/rollback flows must gate
destructive work with confirm_or_bail or require_tty_or_yes_with_seam, accepting
interactive confirmation or --yes, alongside the existing
snapshot-before-mutation rule.

Source: Coding guidelines

src/cmd/doctor.rs (1)

1474-1507: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Delete the leftover duplicate migration-repair block.

The new migration-repair block at lines 1188-1216 already reconciles the pending migration journal, with correct handling of Ok(Some(_)), Ok(None), and Err(e). The old block at lines 1474-1507 checks the exact same journal.migration_pending condition again, later in the same function. This block is unchanged from before this PR and was never removed.

This causes two problems:

1. When the first block's reconcile() call fails but leaves the journal file intact (for example, a symlink guard or an orphan-directory removal failure), this second block runs migration_journal::reconcile(root) a second time and pushes a second RepairRecord with the same id "journal.migration_repaired". The JSON repairs array then contains two records for the same repair, which misleads any automation parsing that report.
2. At Line 1484, PendingMigration::load(root)? uses ? instead of pushing a Failed record. Every other branch in apply_repairs accumulates a record on error. If load itself errors here, this line discards every RepairRecord already collected in records (layout repairs, active-version-marker repair, registry repair) and aborts the whole function.

A prior review already flagged this exact early-return/Ok(_) pattern and it was marked addressed. The fix, however, appears to have been implemented as a new block placed earlier in the function, and this old block was left in place. Delete it; block 1188-1216 already covers this repair correctly.

🐛 Proposed fix: remove the dead block

-    // The migration journal path is self-healing in normal use (top of
-    // `migrate_legacy_install_with_detector`); the doctor repair is the
-    // catch-up for users who ran `numan doctor --fix` without ever calling
-    // `numan use`. Gating on `PendingMigration::load(...).is_some()` keeps
-    // the Applied record honest — re-runs of `doctor --fix` produce no
-    // second repair.
-    if findings
-        .iter()
-        .any(|f| f.id == "journal.migration_pending" && f.severity == Severity::Warn)
-    {
-        if PendingMigration::load(root)?.is_none() {
-            return Ok(records);
-        }
-        let id = "journal.migration_repaired".to_string();
-        // chatgpt PR69 S1A: reacquire the root mutation lock before the
-        // self-healing reconcile so concurrent `numan use` cannot race the
-        // journal stage advance + directory rename the same way AGENTS.md
-        // requires install/remove/activate/deactivate/numan-use to.
-        let _migration_repair_lock = acquire_mutation_lock(root)?;
-        match migration_journal::reconcile(root) {
-            Ok(_) => records.push(RepairRecord {
-                id,
-                status: RepairStatus::Applied,
-                reason: None,
-            }),
-            Err(e) => records.push(RepairRecord {
-                id,
-                status: RepairStatus::Failed,
-                reason: Some(e.to_string()),
-            }),
-        }
-    }
-
     Ok(records)
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cmd/doctor.rs` around lines 1474 - 1507, Delete the duplicate
migration-repair block guarded by the `journal.migration_pending` warning check
near the end of `apply_repairs`. Keep the earlier migration-repair logic that
handles `reconcile` outcomes and records failures, so this function performs the
repair only once and preserves previously accumulated `RepairRecord` values.

♻️ Duplicate comments (1)

src/state/migration_journal.rs (1)

194-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Finish the nu_binary_name() consolidation and use one import style.

Two of the four binary-name sites now call the shared helper, but the consolidation stopped short:

• Line 195 uses a function-local use.
• Line 260 uses the fully qualified path.
• Line 323 (Renamed branch) still hand-rolls if cfg!(windows) { "nu.exe" } else { "nu" }.
• The test helper bin_name() at lines 355-361 hand-rolls it again.

Add nu_binary_name to the existing crate::nu::version_manager import list and call it at all four sites. A future change to the binary name then has one edit site.

♻️ Proposed consolidation

 fn versioned_binary_present(root: &Path, version: &str) -> bool {
-    use crate::nu::version_manager::nu_binary_name;
     version_install_dir(root, version)
         .join(nu_binary_name())
         .is_file()
 }

Extend the existing import (near line 39):

use crate::nu::version_manager::{
    nu_binary_name, read_active_version, version_install_dir, versioned_nu_dir,
    write_active_version,
};

Then replace line 260 with let bin_name = nu_binary_name();, replace the conditional at line 323 with let bin_name = nu_binary_name();, and delete the test helper bin_name() in favor of nu_binary_name().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/state/migration_journal.rs` around lines 194 - 199, Consolidate all
binary-name lookups in migration_journal.rs by adding nu_binary_name to the
existing crate::nu::version_manager import list, removing the function-local and
fully qualified imports, and calling the shared helper in
versioned_binary_present, the line-260 usage, the Renamed branch, and the tests.
Delete the hand-rolled test helper bin_name() and use nu_binary_name() directly
at its call sites.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/cmd/setup.rs`:
- Around line 536-555: Update the active-version cleanup flow around
read_active_version so malformed or unreadable markers are reported and returned
as an anyhow error instead of being treated as clearable. Preserve the existing
handling for absent markers and invalid managed-tree selections, but when
clearing is requested, propagate clear_active_version(root) failures with
context rather than discarding them; ensure the enclosing setup operation
returns anyhow::Result.

In `@src/nu/bootstrap.rs`:
- Around line 717-729: Update the release-install flow around
`fetch_latest_release`, `execute_nu_setup_with_installer`, and `install_version`
to pass the already-fetched `GitHubRelease` through to installation, avoiding a
second `fetch_release_by_tag` request. Add descriptive anyhow context to both
the `Client::builder().build()` and `fetch_latest_release(&client)` error paths
while preserving the existing pinned version behavior.
- Around line 712-729: Defer latest-release resolution in execute_nu_setup until
after the consent and already-installed gates, so non-interactive callers
without --yes and already-installed unpinned setups do not access the network.
Keep the unpinned destination gate behavior intact, then resolve the tag and
pass it into the installer closure used by execute_nu_setup_with_installer. Add
coverage matching
execute_nu_setup_refuses_non_tty_without_yes_and_skips_installer for an unpinned
setup that verifies no network request occurs.

---

Outside diff comments:
In `@AGENTS.md`:
- Around line 152-160: Update AGENTS.md’s Key Conventions and Architecture Rules
documentation to describe the destructive-operation confirmation convention:
remove and snapshot delete/rollback flows must gate destructive work with
confirm_or_bail or require_tty_or_yes_with_seam, accepting interactive
confirmation or --yes, alongside the existing snapshot-before-mutation rule.

In `@src/cmd/doctor.rs`:
- Around line 1474-1507: Delete the duplicate migration-repair block guarded by
the `journal.migration_pending` warning check near the end of `apply_repairs`.
Keep the earlier migration-repair logic that handles `reconcile` outcomes and
records failures, so this function performs the repair only once and preserves
previously accumulated `RepairRecord` values.

In `@src/cmd/setup.rs`:
- Around line 1019-1035: Update the inline tests invoking execute_use_existing
in src/cmd/setup.rs ranges 1019-1035, 1062, and 1083 to use a seam that sets
skip_path: true, preventing profile edits while preserving test behavior. In
tests/setup_nu_test.rs range 141-195, ensure the test has the same ignore marker
used by the acceptance-only cases at lines 368 and 430, so profile writes occur
only in the platform acceptance job.
- Around line 563-566: Remove the duplicate require_tty_or_yes call near the
managed Nushell removal logic, keeping the existing guard at the function entry.
Preserve the single initial validation and avoid emitting a second audit record
for the same operation.

In `@src/nu/bootstrap.rs`:
- Around line 758-792: The already-installed short-circuit in the setup flow
uses the active/latest binary for pinned installs, which can disagree with the
requested version recorded by normalized_version. Update the effective-path
selection to use dest directly whenever normalized_version is Some, retaining
the active/latest fallback only when it is None; ensure PATH prepend/persistence
and the installed marker all target the requested binary. Add a test covering an
installed requested version that differs from the active version.
- Around line 1271-1325: Move
register_existing_nu_refuses_non_tty_without_yes_before_path_mutation out of the
inline unit tests into the real-Nu acceptance tests, marking it #[ignore =
"requires real Nu binary on $PATH — run in platform acceptance job"] like
tests/setup_nu_test.rs; alternatively inject a validation seam using
FakeCandidateRunner so no real nu process runs. If the test remains inline,
guard all PATH snapshot and comparison access with PathRestoreGuard/PATH_MUTEX
to prevent concurrent PATH mutations.

---

Duplicate comments:
In `@src/state/migration_journal.rs`:
- Around line 194-199: Consolidate all binary-name lookups in
migration_journal.rs by adding nu_binary_name to the existing
crate::nu::version_manager import list, removing the function-local and fully
qualified imports, and calling the shared helper in versioned_binary_present,
the line-260 usage, the Renamed branch, and the tests. Delete the hand-rolled
test helper bin_name() and use nu_binary_name() directly at its call sites.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e9a36d98-907c-451e-83ea-bdbdd3f38863

📥 Commits

Reviewing files that changed from the base of the PR and between 9d80322 and 1f9281f.

📒 Files selected for processing (13)

• AGENTS.md
• src/cmd/doctor.rs
• src/cmd/remove.rs
• src/cmd/setup.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/util/fs_safety.rs
• tests/doctor_test.rs
• tests/setup_nu_test.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• tonythethompson/QuickShell (manual)
• tonythethompson/numan (manual)
• tonythethompson/dependency-chain-substrate (manual)

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: Greptile Review
• GitHub Check: Real-Nu acceptance (windows-latest)
• GitHub Check: Analyze (rust)

🧰 Additional context used

📓 Path-based instructions (18)

**/*

📄 CodeRabbit inference engine (CLAUDE.md)

**/*: Use Serena's semantic, symbol-aware tools as the primary tools for reading and editing code; use built-in Read, Glob, Grep, and Edit only under the stated exceptions.
Before editing a code file, inspect its symbol overview, read the specific symbols being changed, and edit them with Serena's symbol-aware tools.
Understand the existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, unnecessary error handling, feature flags, and compatibility shims.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a 2–3 sentence recommendation with the main tradeoff and do not implement until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Address security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, force-pushing, modifying CI/CD, posting externally, or uploading content.
When blocked, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration before deleting them.
Only commit when explicitly asked; do not proactively update git configuration or push changes.
Do not skip Git hooks unless explicitly asked; if a pre-commit hook fails, fix the issue, re-stage, and create a new commit rather than amending.
Stage files by name rather than using git add -A or git add ., to avoid accidentally including secrets or large binaries.
Use a HEREDOC for commit messages, and add co-author attribution only when the user explicitly requests the exact trailer.
Do not force-pus...

Files:

• tests/doctor_test.rs
• AGENTS.md
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}

📄 CodeRabbit inference engine (CLAUDE.md)

Add comments only when the WHY is non-obvious; do not narrate what the code does, reference the current task, or reference the PR in comments.

Files:

• tests/doctor_test.rs
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

!**/.env,!**/credentials.json,!**/*.pem

📄 CodeRabbit inference engine (CLAUDE.md)

Do not commit files that appear to contain secrets, including .env, credentials.json, and PEM files; warn before doing so even if explicitly requested.

Files:

• tests/doctor_test.rs
• AGENTS.md
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use the Rust 2021 edition.
Use anyhow::Result with .context(...) in application code; use thiserror for library error types that callers match on.
Use clap derive macros for CLI definitions.
Use serde with serde_json or toml for serialization.
Function parameters must use &Path, not &PathBuf.
Library code must not panic; error paths should return anyhow::Result with context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock via acquire_mutation_lock(root) and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must use write_json_atomic.
numan install must write only to $NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Only activate and deactivate may modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state must not be authoritative.
Install payloads under versioned, content-addressed paths and never overwrite them in place.
Never overwrite foreign autoload files; respect OWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.

**/*.rs: All CI gates must pass: cargo test, cargo clippy -- -D warnings, and cargo fmt --check.
Every mutating command—including install, remove, update, gc, and future nupm import—must call acquire_mutation_lock(root).
Lockfiles, journals, and state files must use write_json_atomic; partial writes are not allowed.
Pending activation, autoload, and lifecycle journals must be stored under $NUMAN_ROOT/state/.
Module autoload identity must match all four fields: Nu executable hash, Nu version, vendor autoload directory, and managed file path; the lockfile module_activation value is authoritative.
Never overwrite foreign autoload files; respect OWNERSHIP_MARKER.
Pass paths to Nu only throu...

Files:

• tests/doctor_test.rs
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

**/*.{rs,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Run and keep cargo fmt/rustfmt clean, and ensure cargo clippy -- -D warnings passes.

Use serde/serde_json for JSON serialization and toml for configuration serialization.

Files:

• tests/doctor_test.rs
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

**/*.{rs,nu}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{rs,nu}: Real-Nu acceptance tests must be marked #[ignore] and should be run when changes affect activation or nupm import; unit tests must not spawn real nu and should use injectable seams such as FakeCandidateRunner or registrars.
The nupm integration must be read-only toward NUPM_HOME, must not execute build.nu, and must not perform bidirectional synchronization.

Unit tests must use FakeCandidateRunner or injectable registrars and must not spawn a real nu process.

Files:

• tests/doctor_test.rs
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Match existing naming, module layout, and documentation level in the file being edited; update AGENTS.md, docs/, or command help when structure, conventions, or user-visible behavior changes.

Tests must cover failure modes, not only successful execution.

Files:

• tests/doctor_test.rs
• AGENTS.md
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

**/*.{rs,md,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Use the repository's established serialization and module conventions rather than introducing unrelated refactors.

Files:

• tests/doctor_test.rs
• AGENTS.md
• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Keep integration tests under tests/, place unit tests inline with source modules, and test platform-specific code with mock platforms.

Files:

• tests/doctor_test.rs
• tests/setup_nu_test.rs

**/*.md

📄 CodeRabbit inference engine (REVIEW.md)

Update documentation and AGENTS.md when project structure or conventions change.

Files:

• AGENTS.md

src/util/fs_safety.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/util/fs_safety.rs: Managed files must contain OWNERSHIP_MARKER; use assert_managed_file_owned to prevent overwriting foreign files.
Serialize mutations with acquire_mutation_lock(root); the second acquisition for the same root must fail immediately.

Files:

• src/util/fs_safety.rs

src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use cargo fmt formatting and keep Clippy clean under cargo clippy -- -D warnings.

Files:

• src/util/fs_safety.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/state/migration_journal.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/nu/migrate_legacy.rs

src/cmd/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use clap derive-based CLI definitions and preserve the lowercase numan binary name.

Files:

• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• src/cmd/remove.rs
• src/cmd/doctor.rs
• src/cmd/setup.rs

src/cmd/{activate,deactivate,update,remove,init,doctor,nupm}.rs

📄 CodeRabbit inference engine (AGENTS.md)

Create a state snapshot before install, update, remove, activate, deactivate, init --refresh, nupm import, or doctor repair mutations.

Files:

• src/cmd/remove.rs
• src/cmd/doctor.rs

src/nu/version_manager.rs

📄 CodeRabbit inference engine (AGENTS.md)

nu_state/active-version.json is the sole authority for the selected managed Nu version; writes occur through managed setup/use operations.

Files:

• src/nu/version_manager.rs

src/state/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/state/**/*.rs: Write JSON state files atomically using write_json_atomic (a same-directory tempfile followed by persist) to prevent partial-write corruption.
The lockfile is authoritative for module activation; autoload-state.json is only a derived fast-check projection.

Files:

• src/state/migration_journal.rs

src/state/{journal,plugin_deactivate_journal,migration_journal,lifecycle_journal,autoload_journal,autoload_recovery}.rs

📄 CodeRabbit inference engine (AGENTS.md)

Journal state transitions must be written atomically and reconciled after interruption according to their defined recovery stages.

Files:

• src/state/migration_journal.rs

src/nu/migrate_legacy.rs

📄 CodeRabbit inference engine (AGENTS.md)

Migration reconciliation must refuse to act when tools/nushell is a symlink or reparse point, retain unreconciled journals on failure, and let filesystem truth take precedence over journal stage.

Files:

• src/nu/migrate_legacy.rs

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: tonythethompson/numan

Timestamp: 2026-08-03T18:49:02.290Z
Learning: Source builds require explicit user consent before cloning/building, with separate consent scopes.

Learnt from: CR
Repo: tonythethompson/numan

Timestamp: 2026-08-03T18:49:02.290Z
Learning: Lockfiles must pin immutable artifact paths, and cached artifacts must remain while referenced.

Learnt from: CR
Repo: tonythethompson/numan

Timestamp: 2026-08-03T18:49:02.290Z
Learning: Use imperative commit subjects shorter than 72 characters, branches named `feature/description` or `fix/description`, never force-push `master`, and squash feature merges.

Learnt from: CR
Repo: tonythethompson/numan

Timestamp: 2026-08-03T18:49:02.290Z
Learning: Write tests first, run the full test suite, and update `AGENTS.md` when project structure or conventions change.

🔍 Remote MCP GitHub Copilot

Additional review context

• PR #82 is open and unmerged with mergeable_state: blocked; its current head is 7f487d1.
• All reported CI checks currently pass, including Clippy, Format, MSRV, platform tests, Real-Nu acceptance, CodeQL, and package checks.
• Review history identified important invariants around:
  • numan use locking and snapshot behavior.
  • Bare setup nu fetching and selecting the latest release.
  • Atomicity of managed-tree deletion and active-marker cleanup.
  • Migration probe timeout/deadlock avoidance.
  • Actionable doctor repair hints.
  • Preservation and validation of off-tree binary markers.
• Several earlier findings were marked resolved/outdated, but the review history still documents these as areas requiring verification against the final diff—especially latest-install marker persistence, mutation locking, migration reconciliation failure handling, and interactive removal confirmation.
• The final diff adds a regression test ensuring doctor skips off-PATH repair when a managed tree exists, moves snapshot confirmation before lock acquisition, and adds legacy-version discovery tests.

🔇 Additional comments (43)

src/nu/version_manager.rs (5)

48-81: LGTM!

---

350-363: LGTM!

---

404-410: LGTM!

---

467-483: LGTM!

---

776-867: LGTM!

src/state/migration_journal.rs (5)

40-40: LGTM!

Also applies to: 150-157

---

234-245: LGTM!

---

279-279: LGTM!

---

660-664: LGTM!

---

691-807: LGTM!

src/util/fs_safety.rs (2)

248-269: LGTM!

---

464-487: LGTM!

src/nu/migrate_legacy.rs (7)

79-99: LGTM!

---

122-138: LGTM!

---

147-155: LGTM!

Also applies to: 168-186

---

208-212: LGTM!

Also applies to: 236-241, 254-260

---

288-306: LGTM!

---

343-354: LGTM!

---

494-527: LGTM!

Also applies to: 595-604, 626-725

src/nu/bootstrap.rs (6)

24-35: LGTM!

---

231-267: LGTM!

---

468-508: LGTM!

---

680-698: LGTM!

---

826-862: LGTM!

---

1109-1269: LGTM!

src/cmd/setup.rs (6)

12-23: LGTM!

---

198-236: LGTM!

---

418-435: LGTM!

Also applies to: 508-525

---

576-586: LGTM!

Also applies to: 620-620

---

942-962: LGTM!

---

1061-1118: Good coverage of the off-tree marker contract.

The assertions pin binary_path, active_nu_binary resolution, and list_installed_versions inclusion. That matches the three-way contract in src/nu/version_manager.rs. The skip_path concern is tracked at Line 1019.

tests/setup_nu_test.rs (4)

13-13: LGTM!

Also applies to: 56-95

---

101-116: LGTM!

---

235-241: LGTM!

Also applies to: 368-372, 430-434

---

462-500: LGTM!

src/cmd/doctor.rs (2)

143-152: LGTM!

Also applies to: 325-354, 1128-1150, 1188-1216, 1257-1266, 2418-2447, 2458-2502

---

346-353: 🗄️ Data Integrity & Integration

No stale --fix hint remains.

src/cmd/use_cmd.rs (1)

79-115: LGTM!

Also applies to: 118-154, 163-194, 296-400, 475-523

tests/doctor_test.rs (2)

33-117: LGTM!

---

2458-2502: 🎯 Functional Correctness

No change needed. MigrationStage uses #[serde(rename_all = "snake_case")], so "stage": "prepared" deserializes correctly and can reach the schema-version mismatch check.

src/cmd/remove.rs (1)

38-45: LGTM!

src/cmd/snapshot.rs (1)

287-334: LGTM!

AGENTS.md (1)

67-68: LGTM!

Also applies to: 147-147

[linear-code]

NUM-52