Add journaled legacy Nu migration and setup nu subcommands

[tonythethompson]

PR Summary by Qodo

Nu setup subcommands, journaled legacy migration, and hardened confirm/version-manager APIs

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Replace numan setup nu action flags (--remove, --use-path, --use-existing) with
 subcommands (remove, path, use ); legacy flags kept as deprecated hidden compat paths.
• Add --force gate to path/use subcommands so they refuse to silently delete an active managed
 Nu install; add shared confirm_or_auto/require_tty_or_yes utilities and apply non-TTY --yes
 guards uniformly across setup, remove, and snapshot commands.
• Introduce a journaled legacy-Nu → versioned-layout migration (src/nu/migrate_legacy.rs +
 src/state/migration_journal.rs) with self-healing reconcile, wired into numan use and `numan
 doctor --fix`.
• Convert nu::version_manager public API from anyhow::Result to a thiserror-based
 VersionManagerError, add path-traversal guards on the active-version marker, and fix several
 silent-error-swallowing bugs (resolve_installed_version, find_nu_executable_with_root).
• Centralize the root mutation lock acquisition behind setup_subcommand_lock and extend doctor
 findings/repairs to cover migration-journal corruption and pending states.
• Update docs (AGENTS.md, roadmap, CHANGELOG references) and add/adjust extensive unit and CLI-parse
 tests.

Diagram

graph TD
  CLI["numan setup nu CLI"] --> ARGS["NuSetupArgs / NuAction"]
  ARGS --> LOCK["setup_subcommand_lock()"]
  LOCK --> EXEC["execute_nu_impl_locked()"]
  EXEC --> VM["version_manager\n(VersionManagerError)"]
  EXEC --> BOOT["bootstrap::register_existing_nu"]
  USECMD["numan use"] --> MIG["migrate_legacy::migrate_legacy_install"]
  MIG --> JOURNAL[("migration_journal.rs\nstate/migration-journal.json")]
  MIG --> VM
  DOCTOR["numan doctor --fix"] --> JOURNAL
  DOCTOR --> EXEC
  CONFIRM["util::confirm\n(require_tty_or_yes)"] --> EXEC
  CONFIRM --> USECMD

  subgraph Legend
    direction LR
    _svc(["Service/Command"]) ~~~ _db[("Journal/State file")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Split into separate PRs (CLI redesign / thiserror migration / migration journal)

• ➕ Smaller, independently reviewable diffs
• ➕ Easier to bisect regressions
• ➕ Faster incremental merges
• ➖ Slower overall delivery due to sequencing/rebase overhead
• ➖ Interdependent changes (e.g. version_manager errors used by migrate_legacy) would require careful ordering

2. Use anyhow::Error downcasting instead of a new thiserror enum

• ➕ Less boilerplate, no new enum to maintain
• ➖ Callers can't match on error kind (needed by doctor/paths.rs to distinguish NotFound from corruption)
• ➖ Weaker API contract for a library-facing module

Recommendation: The PR's approach—journaled staged migration mirroring existing journal modules, plus a typed error enum for the library surface—is consistent with the codebase's established patterns (autoload_journal, lifecycle_journal) and is the right call given the crash-recovery requirements. The main alternative worth flagging is the scope: bundling the CLI subcommand redesign, the thiserror migration, and the new migration-journal subsystem into a single PR increases review risk given the amount of interacting invariants (locking order, marker clearing sequencing, force-gate placement). Splitting these into 2-3 PRs would have let reviewers verify each safety property in isolation.

Files changed (23) +2711 / -435

Enhancement (7) +1862 / -201

setup.rsReplace setup nu flags with subcommands, add --force gates and locking +184/-108

Replace setup nu flags with subcommands, add --force gates and locking

• Redesigns NuAction::Path/Use to carry a 'force' flag guarding destructive managed-tree replacement; centralizes root mutation lock acquisition via setup_subcommand_lock with audit labeling; moves TTY/--yes checks earlier in execute_use_path/execute_use_existing before any destructive step; persists active-version marker after PATH/off-path registration; reorders active-version clearing to occur after successful deletion.

src/cmd/setup.rs

migrate_legacy.rsNew module: journaled legacy single-binary to versioned Nu migration +709/-0

New module: journaled legacy single-binary to versioned Nu migration

• New file implementing migrate_legacy_install/migrate_legacy_install_with_detector with injectable version-detection and post-create-hook seams for testing crash-recovery of the legacy-to-versioned Nu layout transition.

src/nu/migrate_legacy.rs

migration_journal.rsNew module: crash-recovery journal for legacy Nu migration +616/-0

New module: crash-recovery journal for legacy Nu migration

• New file defining PendingMigration/MigrationStage (Prepared/Renamed/Active) with atomic save/load/delete and a reconcile() self-healer that repairs half-applied migrations, guarding against unsafe version components.

src/state/migration_journal.rs

doctor.rsSurface and repair migration-journal findings +204/-3

Surface and repair migration-journal findings

• Adds journal.migration_pending/journal.migration_invalid findings backed by PendingMigration::load, and an apply_repairs branch that reacquires the mutation lock and calls migration_journal::reconcile as an Auto-tier repair.

src/cmd/doctor.rs

use_cmd.rsRun legacy migration before switching Nu versions; preserve off-tree marker +56/-90

Run legacy migration before switching Nu versions; preserve off-tree marker

• Inlines the mutation-guard helper into execute(), invokes migrate_legacy_install before dispatching to latest/switch, preserves off-tree binary_path when re-selecting the same version via 'latest', and switches to Result-returning version_manager calls.

src/cmd/use_cmd.rs

fs_safety.rsAdd setup_subcommand_lock helper for audited destructive-setup locking +87/-0

Add setup_subcommand_lock helper for audited destructive-setup locking

• New setup_subcommand_lock() wraps acquire_mutation_lock with audit logging and a closure-based API used by every destructive setup entry point; adds tests for lock acquisition, error propagation, and concurrent-call rejection.

src/util/fs_safety.rs

hints.rsAdd CMD_USE constant for hint messages +6/-0

Add CMD_USE constant for hint messages

• Adds a CMD_USE constant referencing 'numan use' for doctor finding hints.

src/util/hints.rs

Bug fix (4) +317 / -128

bootstrap.rsInstall into versioned layout; rework already-installed detection +111/-101

Install into versioned layout; rework already-installed detection

• install_from_archive now always writes into the versioned layout instead of the legacy flat path; execute_nu_setup_with_installer reworks the already-installed short-circuit to probe the versioned layout and persists the active version on pinned reinstalls; register_existing_nu no longer writes the active marker itself (caller now does).

src/nu/bootstrap.rs

paths.rsPropagate marker read errors instead of silently falling back to PATH +49/-15

Propagate marker read errors instead of silently falling back to PATH

• find_nu_executable_with_root now surfaces corrupt/unreadable active-version marker errors instead of swallowing them and falling back to PATH Nu; validate_nushell_binary signature simplified to return unit.

src/nu/paths.rs

remove.rsAdd non-TTY --yes guard to package removal +66/-0

Add non-TTY --yes guard to package removal

• Adds a --yes flag and require_tty_or_yes_with_seam gate before the destructive removal proceeds, plus injectable TTY seam and new tests.

src/cmd/remove.rs

snapshot.rsAdd non-TTY --yes guards to snapshot delete/rollback +91/-12

Add non-TTY --yes guards to snapshot delete/rollback

• Introduces delete_with_tty/rollback_with_tty with require_tty_or_yes_with_seam gating before destructive snapshot operations, replacing the local confirm() helper with confirm_or_bail directly, plus new tests.

src/cmd/snapshot.rs

Refactor (2) +287 / -93

version_manager.rsMigrate version_manager errors to thiserror::VersionManagerError +244/-75

Migrate version_manager errors to thiserror::VersionManagerError

• Replaces anyhow::Result with a dedicated VersionManagerError enum across all public APIs, adds path-traversal rejection on write_active_version_with_binary, fixes silent error-swallowing in resolve_installed_version/list_installed_versions, and adds legacy-install/version-name helper functions used by migrate_legacy.

src/nu/version_manager.rs

confirm.rsRename TTY seam and expand branch test coverage +43/-18

Rename TTY seam and expand branch test coverage

• Renames require_tty_or_yes_with_tty's implementation to require_tty_or_yes_with_seam (kept as a thin wrapper) and adds tests covering all three confirm branches.

src/util/confirm.rs

Tests (4) +220 / -10

test_paths.rsNew PathRestoreGuard test helper +47/-0

New PathRestoreGuard test helper

• New cfg(test) module providing an RAII guard that snapshots and restores the process PATH env var around PATH-mutating tests.

src/util/test_paths.rs

doctor_test.rsUpdate NuAction::Use pattern match for new force field +2/-2

Update NuAction::Use pattern match for new force field

• Adjusts the destructured NuAction::Use pattern to account for the added force field.

tests/doctor_test.rs

setup_nu_test.rsAdd force-gate, active-marker, and CLI-parse tests for setup nu +166/-7

Add force-gate, active-marker, and CLI-parse tests for setup nu

• Adds tests covering the new --force refusal/acceptance behavior for path/use subcommands, active-version marker assertions after install, updated CLI-parse assertions for NuAction::Path/Use with force, and legacy-flag negative tests.

tests/setup_nu_test.rs

model.rsPass --yes to acceptance-test remove step +5/-1

Pass --yes to acceptance-test remove step

• Adds the --yes argument to the Remove acceptance-test step now that remove requires explicit confirmation in non-interactive sessions.

tests/support/acceptance/model.rs

Documentation (3) +18 / -3

lib.rsDocument anyhow vs thiserror error-handling convention +6/-0

Document anyhow vs thiserror error-handling convention

• Adds crate-level doc comment explaining that application handlers use anyhow::Result while library modules like nu::version_manager return concrete thiserror types.

src/lib.rs

AGENTS.mdDocument migration_journal, version_manager and setup nu subcommand syntax +5/-1

Document migration_journal, version_manager and setup nu subcommand syntax

• Updates module map entries to describe migration_journal.rs, migrate_legacy.rs, version_manager.rs, and the new 'setup nu [VERSION]|remove|path|use <path>' CLI shape; updates the migration-journal state doc bullet.

AGENTS.md

consolidated-multi-repo-roadmap.mdUpdate roadmap notes on active-marker ownership and backfill data +7/-2

Update roadmap notes on active-marker ownership and backfill data

• Clarifies that 'numan use' now also performs journaled legacy migration before writing the active marker, and reflows backfill-data wording.

docs/plans/consolidated-multi-repo-roadmap.md

Other (3) +7 / -0

mod.rsRegister cfg(test) test_paths module +3/-0

Register cfg(test) test_paths module

• Adds '#[cfg(test)] pub mod test_paths;' declaration.

src/util/mod.rs

mod.rsRegister migrate_legacy module and re-export version types +3/-0

Register migrate_legacy module and re-export version types

• Adds 'pub mod migrate_legacy;' and re-exports ActiveVersion/VersionManagerError from version_manager.

src/nu/mod.rs

mod.rsRegister migration_journal module +1/-0

Register migration_journal module

• Adds 'pub mod migration_journal;' declaration.

src/state/mod.rs

[coderabbitai]

Actionable comments posted: 6

Caution

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

⚠️ Outside diff range comments (1)

src/cmd/remove.rs (1)

39-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not hold the mutation lock during confirmation.

Line 39 acquires the root lock before Line 61 opens an interactive prompt. An unanswered prompt can block all mutations for this root.

Perform the read-only package validation first. Request confirmation next. Then acquire the lock, reload the lockfile, and repeat the activation checks before creating the snapshot or writing state. This post-lock validation prevents a TOCTOU error.

🤖 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/remove.rs` around lines 39 - 68, Move acquire_mutation_lock out of
the initial validation phase: load the lockfile, resolve the package, and run
the pre-confirmation checks before confirm_or_bail. After confirmation, acquire
the lock, reload the lockfile, re-resolve the package, and repeat
ensure_plugin_not_active plus the module_activation check before proceeding to
snapshot or state writes, preserving the existing force behavior.

🤖 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/doctor.rs`:
- Around line 665-725: The journal.migration_pending finding must only be marked
Auto when reconciliation can actually proceed. In src/cmd/doctor.rs:665-725,
call a shared non-mutating reconciliation validator before assigning
RepairTier::Auto; in src/state/migration_journal.rs:240-255, expose
normalization and managed-directory safety checks through it, and in
src/state/migration_journal.rs:288-305 include Prepared orphan removability. In
src/cmd/doctor.rs:2597-2738, add failure tests for safe invalid semantic
versions, symlinked managed directories, and non-empty Prepared orphans,
asserting Error, Manual, no pending finding, and exit code 1. In
docs/numan-doctor.md:152-153, define pending journals by validator acceptance
and document the runnable numan use <version> hint.

In `@src/cmd/setup.rs`:
- Around line 434-438: In src/cmd/setup.rs lines 434-438, reorder
snapshot_before_setup_mutation before preflight_active_marker_writable so the
snapshot captures the true pre-operation state; apply the same ordering in lines
528-532 for off-path registration. In lines 1047-1064, add a regression test
verifying the snapshot occurs before nu_state and probe creation.

In `@src/cmd/use_cmd.rs`:
- Around line 33-38: Make the args.version == "list" branch in the use command
read-only by removing migrate_legacy_install and setup_subcommand_lock,
returning execute_list(root) directly. Update the related test to verify legacy
version discovery through the VERSION marker while asserting that no filesystem
state changes.

In `@src/nu/bootstrap.rs`:
- Around line 724-727: Remove the review and PR metadata from the comments at
src/nu/bootstrap.rs lines 724-727 and 1164-1165, and src/cmd/remove.rs lines
58-60. Preserve only durable explanations of the required behavior, deleting
references to “review P1,” “PR `#82`,” and “cubic” without changing the
surrounding implementation.

In `@tests/setup_nu_test.rs`:
- Around line 109-126: The short-circuit test around execute_nu must verify the
pinned binary is neither replaced nor reinstalled. Read and assert the binary
still contains b"fake nu", then add a focused lower-level test using an injected
failing installer to confirm the installer is not invoked while preserving the
active-version marker behavior.
- Around line 105-108: Remove the “(PR `#82`)” reference from the comment near the
pin-only short-circuit test, preserving the behavior rationale and all other
comment text.

---

Outside diff comments:
In `@src/cmd/remove.rs`:
- Around line 39-68: Move acquire_mutation_lock out of the initial validation
phase: load the lockfile, resolve the package, and run the pre-confirmation
checks before confirm_or_bail. After confirmation, acquire the lock, reload the
lockfile, re-resolve the package, and repeat ensure_plugin_not_active plus the
module_activation check before proceeding to snapshot or state writes,
preserving the existing force behavior.

🪄 Autofix

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: da6d3ce6-64cb-4639-9b28-0857779ae9fd

📥 Commits

Reviewing files that changed from the base of the PR and between 847938d and 3f71579.

📒 Files selected for processing (14)

• docs/numan-doctor.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/paths.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. (4)

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

🧰 Additional context used

📓 Path-based instructions (15)

**/*

📄 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:

• src/util/fs_safety.rs
• docs/numan-doctor.md
• tests/doctor_test.rs
• src/nu/paths.rs
• src/nu/migrate_legacy.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/state/migration_journal.rs
• src/nu/bootstrap.rs
• src/cmd/setup.rs
• src/cmd/doctor.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:

• src/util/fs_safety.rs
• tests/doctor_test.rs
• src/nu/paths.rs
• src/nu/migrate_legacy.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/state/migration_journal.rs
• src/nu/bootstrap.rs
• src/cmd/setup.rs
• src/cmd/doctor.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:

• src/util/fs_safety.rs
• docs/numan-doctor.md
• tests/doctor_test.rs
• src/nu/paths.rs
• src/nu/migrate_legacy.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/state/migration_journal.rs
• src/nu/bootstrap.rs
• src/cmd/setup.rs
• src/cmd/doctor.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:

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

**/*.{rs,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

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

Files:

• src/util/fs_safety.rs
• tests/doctor_test.rs
• src/nu/paths.rs
• src/nu/migrate_legacy.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/state/migration_journal.rs
• src/nu/bootstrap.rs
• src/cmd/setup.rs
• src/cmd/doctor.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:

• src/util/fs_safety.rs
• tests/doctor_test.rs
• src/nu/paths.rs
• src/nu/migrate_legacy.rs
• src/cmd/remove.rs
• src/nu/version_manager.rs
• src/cmd/snapshot.rs
• src/cmd/use_cmd.rs
• tests/setup_nu_test.rs
• src/state/migration_journal.rs
• src/nu/bootstrap.rs
• src/cmd/setup.rs
• src/cmd/doctor.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:

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

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

📄 CodeRabbit inference engine (CONTRIBUTING.md)

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

Files:

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

src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.rs: Use anyhow::Result for application code, thiserror for library errors, and add failure context with .context(...) or ?.
Never panic in library code; return errors instead.

Files:

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

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) and retain its RAII MutationLock; a second acquisition must fail immediately.

Files:

• src/util/fs_safety.rs

**/*.md

📄 CodeRabbit inference engine (REVIEW.md)

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

Files:

• docs/numan-doctor.md

tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use mock platforms for platform-specific tests and injected fake Nu runners for unit tests; real-Nu behavior belongs in ignored acceptance tests.

Files:

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

src/cmd/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

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

Files:

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

src/state/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use atomic writes for all JSON state, including lockfiles, journals, and nu_state/paths.json, to prevent partial-write corruption.

Files:

• src/state/migration_journal.rs

src/state/{journal,plugin_deactivate_journal,migration_journal}.rs

📄 CodeRabbit inference engine (AGENTS.md)

Journal state transitions must be persisted atomically and reconciled after interruption; migration journals use Prepared → Renamed → Active, with filesystem truth taking precedence.

Files:

• src/state/migration_journal.rs

🔍 Remote MCP GitHub Copilot

Additional review context

• The latest PR checks currently show Clippy and Format failing, while MSRV, packaging, Ubuntu tests, and Ubuntu Real-Nu acceptance pass. macOS/Windows tests and Rust analysis remain in progress.
• Prior CI output reported a setup test failure caused by Text file busy when executing a temporary fake Nushell binary.
• Outstanding review concerns include:
  • migrate_legacy still using .exists() instead of .is_file(), allowing a directory to be migrated as a binary.
  • numan setup nu still aborting on dangling active markers instead of falling back to an installed version.
  • MigrationJournalError and LegacyMigrationError remaining transparent wrappers around anyhow::Error, providing no matchable typed variants.
  • The migration version probe’s timeout path lacking focused failure-mode coverage and nonzero-exit diagnostics omitting stderr.
  • Repeated platform-specific Nushell binary-name logic in migration_journal.rs.
• Existing review feedback confirms that the new symlink regression test and active-marker preservation test are useful and that the migration timeout implementation now bounds child execution.

🔇 Additional comments (14)

src/nu/migrate_legacy.rs (1)

134-135: LGTM!

Also applies to: 787-822

tests/setup_nu_test.rs (1)

381-381: LGTM!

Also applies to: 441-441

src/state/migration_journal.rs (1)

195-198: Reuse nu_binary_name() for the platform binary name.

Line 195 reintroduces the platform branch that nu_binary_name() centralizes. Use the helper so binary probes and cleanup paths cannot diverge.

tests/doctor_test.rs (1)

40-47: LGTM!

src/cmd/setup.rs (3)

266-283: LGTM!

---

660-675: 🗄️ Data Integrity & Integration

No change needed. Direct production callers of execute_loader_with_probe do not exist outside tests; production writes go through execute_loader, which holds the required lock.

---

552-573: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Path Traversal (CWE-59)

Reachability path

● Entry
  src/nu/migrate_legacy.rs:789
  migrate_legacy_refuses_symlinked_managed_dir: Real directory holding a legacy binary, reached only via a symlink.
│
▼
● Hop
  src/cmd/doctor.rs
│
▼
● Sink
  src/cmd/setup.rs

Create the marker probe without following a symlink.

If another principal can modify $NUMAN_ROOT/nu_state, it can pre-create .numan-active-marker-write-probe as a symlink to a file writable by the Numan process. std::fs::write follows that link and overwrites the target with ok. setup_subcommand_lock does not protect against external filesystem writers. The later remove_file runs after the overwrite and ignores cleanup failure.

Use exclusive no-follow creation in a verified non-symlinked directory, or use a securely created temporary file. Add a Unix regression test that pre-creates the probe symlink and verifies that its target remains unchanged.

As per coding guidelines: “Address security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.”

src/nu/bootstrap.rs (1)

259-267: LGTM!

Also applies to: 728-879, 1040-1163, 1166-1268

src/nu/paths.rs (1)

337-343: LGTM!

src/nu/version_manager.rs (1)

500-549: LGTM!

Also applies to: 810-812

src/cmd/use_cmd.rs (1)

16-16: LGTM!

Also applies to: 41-63

src/cmd/snapshot.rs (1)

213-218: LGTM!

Also applies to: 233-241

src/cmd/remove.rs (1)

19-19: LGTM!

Also applies to: 36-36

src/util/fs_safety.rs (1)

103-108: LGTM!

[coderabbitai]

Actionable comments posted: 4

🤖 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/doctor.rs`:
- Around line 663-681: The repair hint selection is fragile because it greps
e.to_string() for "Renamed" and "missing", creating hidden coupling to exact
error message wording in validate_reconcile. Instead, replace the string
matching check with j.stage == MigrationStage::Renamed combined with the
binary-presence probe that already exists in the lines 688-693 section to
determine whether to offer the setup repair hint or the generic delete hint.
This decouples the hint logic from error message wording and keeps the hint
decision tied to observable journal state.
- Around line 2845-2848: Update the Windows branch around symlink creation in
the affected test to avoid unwrapping symlink_dir, skipping the test when
creation fails due to missing privileges; keep the Unix symlink behavior
unchanged and preserve coverage of the reparse-case logic through the existing
mock-platform testing approach where applicable.

In `@src/cmd/setup.rs`:
- Around line 616-617: The managed-directory removal flows must preserve the
active marker until deletion succeeds. In src/cmd/setup.rs lines 616-617 and
645-646, reorder the logic around clear_active_version so symlink validation and
remove_dir_all complete successfully before clearing the marker, while symlink
refusal leaves it intact; add a regression test confirming a rejected symlinked
managed directory preserves the marker.

In `@src/state/migration_journal.rs`:
- Around line 298-306: Change validate_reconcile to return Result<String>,
returning the computed normalized version from every successful arm, including
Active. Update reconcile to capture that returned value and remove its
normalize_version call and unreachable internal error; update the doctor caller
to match Ok(normalized) and use it for the version_binary probe instead of
re-normalizing.

🪄 Autofix

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: cc066d99-70ea-4a44-8a5a-35825cd6ef94

📥 Commits

Reviewing files that changed from the base of the PR and between 3f71579 and 4198847.

📒 Files selected for processing (8)

• docs/numan-doctor.md
• src/cmd/doctor.rs
• src/cmd/remove.rs
• src/cmd/setup.rs
• src/cmd/use_cmd.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.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. (4)

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

🧰 Additional context used

📓 Path-based instructions (14)

**/*

📄 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:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• docs/numan-doctor.md
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.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:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.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:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• docs/numan-doctor.md
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.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:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.rs

**/*.{rs,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

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

Files:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.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:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.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:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• docs/numan-doctor.md
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.rs

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

📄 CodeRabbit inference engine (CONTRIBUTING.md)

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

Files:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• docs/numan-doctor.md
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs
• tests/setup_nu_test.rs

src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.rs: Use anyhow::Result for application code, thiserror for library errors, and add failure context with .context(...) or ?.
Never panic in library code; return errors instead.

Files:

• src/cmd/setup.rs
• src/nu/bootstrap.rs
• src/state/migration_journal.rs
• src/cmd/remove.rs
• src/cmd/use_cmd.rs
• src/cmd/doctor.rs

src/cmd/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

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

Files:

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

src/state/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use atomic writes for all JSON state, including lockfiles, journals, and nu_state/paths.json, to prevent partial-write corruption.

Files:

• src/state/migration_journal.rs

src/state/{journal,plugin_deactivate_journal,migration_journal}.rs

📄 CodeRabbit inference engine (AGENTS.md)

Journal state transitions must be persisted atomically and reconciled after interruption; migration journals use Prepared → Renamed → Active, with filesystem truth taking precedence.

Files:

• src/state/migration_journal.rs

**/*.md

📄 CodeRabbit inference engine (REVIEW.md)

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

Files:

• docs/numan-doctor.md

tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use mock platforms for platform-specific tests and injected fake Nu runners for unit tests; real-Nu behavior belongs in ignored acceptance tests.

Files:

• tests/setup_nu_test.rs

🔍 Remote MCP GitHub Copilot

Additional review context

• Current CI has Clippy, Format, Ubuntu tests, MSRV, packaging, and macOS acceptance passing. Windows tests, Rust analysis, and Greptile review remain in progress.
• Review history identifies remaining concerns:
  • LegacyMigrationError and MigrationJournalError still wrap anyhow::Error transparently, so callers lack matchable error variants.
  • use latest may preserve a stale off-tree binary_path without checking that the path still exists.
  • The migration version probe’s timeout/nonzero-exit behavior has limited focused coverage.
• The PR now includes tests for symlink refusal, normalized versions, invalid journals, non-empty migration orphans, read-only listing, marker preflight ordering, and pinned-install binary preservation.
• Previously reported issues involving interactive removal confirmation, latest-install short-circuiting, active-marker persistence, lock routing, and migration-journal validation are addressed in the current changes.

🔇 Additional comments (11)

src/state/migration_journal.rs (2)

201-271: LGTM!

---

414-423: LGTM!

src/cmd/doctor.rs (2)

2474-2534: LGTM!

---

2429-2437: LGTM!

Also applies to: 2586-2686, 2688-2729, 2771-2828, 2883-2923

docs/numan-doctor.md (1)

152-153: LGTM!

tests/setup_nu_test.rs (2)

440-446: 🩺 Stability & Availability

Verify persistent PATH isolation for this acceptance test.

execute_use_existing constructs NuSetupOptions { skip_path: false } in src/cmd/setup.rs:532-546. Confirm that PathRestoreGuard also redirects or restores persistent user PATH state. If it does not, use a registration seam or a temporary platform backend.

#!/usr/bin/env bash
set -euo pipefail

ast-grep outline tests/setup_nu_test.rs --items all
ast-grep outline src/cmd/setup.rs --items all
ast-grep outline src/nu/bootstrap.rs --items all

rg -n -C 8 \
  'struct PathRestoreGuard|impl Drop for PathRestoreGuard|setup_nu_use_existing_force_drops_managed_tree|execute_use_existing|skip_path:\s*false|register_existing_nu|persist_user_path|persist_path_dir' \
  tests/setup_nu_test.rs src/cmd/setup.rs src/nu/bootstrap.rs

---

56-128: LGTM!

src/nu/bootstrap.rs (1)

724-813: LGTM!

Also applies to: 856-878, 1040-1077, 1157-1275

src/cmd/use_cmd.rs (1)

26-55: LGTM!

Also applies to: 250-310

src/cmd/setup.rs (1)

434-453: LGTM!

Also applies to: 527-546, 974-1100

src/cmd/remove.rs (1)

39-92: LGTM!

[linear-code]

NUM-54