[phase-1] cleanup: journaled legacy migration - #69
Conversation
…nfirm utility Breaking change: --remove, --use-path, --use-existing are replaced by subcommands (remove, path, use <path>). Hidden backward-compat flags emit deprecation warnings (remove in v0.3.0). - Add src/util/confirm.rs: confirm_or_auto / confirm_or_bail (non-TTY auto-confirms) - Replace 16 inline TTY-check sites across 8 files - Normalize all --yes help text to 'Skip confirmation prompts' - Redesign NuSetupArgs with Option<NuAction> subcommand + positional VERSION - Add NuSetupArgs constructors for internal callers (doctor, nu_pin_offer) - Add 8 CLI-parse tests + update all affected test assertions - Update README, CHANGELOG, docs/numan-doctor.md, AGENTS.md
- Prevent PATH subcommand from deleting active managed Nu - Guard loader overwrites with ownership verification - Reject incompatible legacy Nu setup flags
Fix remaining --version references in resolve.rs, tighten doctor_test assertion, add skip_path guard to legacy compat path, add negative tests for version+subcommand and legacy use_existing+skip_path.
Change 'your PATH Nu is not touched' to 'your existing Nu is not replaced' since setup nu does modify PATH by default.
Introduce a new `numan use <version>` command path (`cli`, `main`, and `cmd::use_cmd`) as a post-1.0 placeholder that currently fails with a clear guidance message to use `numan setup nu <version>`. Update roadmap/docs to reflect the consolidated plan filename, add post-1.0 side-by-side Nu management notes, and register the new command module in AGENTS metadata.
Fixed 4 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- Add src/nu/version_manager.rs for active version tracking - Active version stored in <root>/nu_state/active-version.json - Installed versions discovered from <root>/tools/nushell/<version>/ - Helpers: read/write active version, list installed, check if installed - Migration logic for legacy single-binary installs - Update bootstrap.rs to install to versioned subdirectories - install_from_archive() now writes to <root>/tools/nushell/<version>/ - managed_nu_binary() delegates to version_manager for active version - Automatically sets newly installed version as active - Implement numan use command - numan use <version> — switch to specific installed version - numan use latest — switch to newest installed version - numan use list — show all installed versions with active marker - Validates version is installed before switching - Provides helpful hints when version not found - Auto-migrates legacy single-binary installs on first run - Remove Commands::Use from root init exclusion (now needs root) All 425 tests pass, clippy clean, fmt applied.
- Validate and normalize Nu versions - Propagate legacy migration errors - Handle invalid active markers gracefully - Avoid parent path panic - Update Nu setup guidance
…ompilation errors
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…master for setup.rs, snapshot.rs, cli.rs, bootstrap.rs; keep full use_cmd.rs implementation
wip: puts numan use + confirm-gate UX + migration journal + reconcile into a single baseline commit so the pr-migrate-legacy-installs split can branch off a known starting point. This commit does not represent either final PR; the next step surgically extracts migration into its own branch and reverts the migration hunks on feature/numan-use.
- Pull 251 lines of migration fs (Legacy*Detector type alias, LegacyPostCreateHook type alias, detect_legacy_version, migrate_legacy_install, migrate_legacy_install_with_detector, parse_nu_version_from_output) plus 8 migrate_legacy_* tests + create_legacy_binary helper + production_detector_prefers_version_metadata_file + test_parse_nu_version_from_output into a new crate::nu::migrate_legacy module. - Add 'pub(crate)' to write_active_marker (now consumed by crate::nu::migrate_legacy). - Retarget use_cmd.rs to call crate::nu::migrate_legacy::migrate_legacy_install. - Re-register the module in src/nu/mod.rs. This is the pr-migrate-legacy-installs half of the split: phase 1 cleanup + journaled transaction for legacy single-binary -> versioned installation with self-healing and numan doctor --fix reconciliation. Co-authored-by: Codebuff <noreply@codebuff.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughThe PR adds journaled migration from legacy single-binary Nu installations to versioned directories. It adds recovery, typed version-state errors, version switching, mutation locking, setup safeguards, and doctor reporting and repair. ChangesNu version management
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a journaled, crash-recoverable migration for legacy single-binary Nushell installs and expands the Nu version-management surface (numan use, active-version marker, marker-aware Nu discovery) while wiring migration reconciliation into doctor --fix.
Changes:
- Adds a new migration journal (
state/migration-journal.json) withPrepared → Renamed → Activestages and areconcile(root)self-heal path. - Adds legacy-install migration logic (
nu/migrate_legacy.rs) and integrates it intonuman useplusnuman doctor --fix. - Introduces/updates active Nu version tracking (
nu_state/active-version.json) and marker-aware Nu resolution (nu/paths.rs).
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/hints.rs | Adds CMD_USE hint text describing numan use and its migration reconciliation role. |
| src/util/confirm.rs | Adds require_tty_or_yes() to fail-closed destructive setup flows in non-TTY sessions without --yes. |
| src/state/mod.rs | Exposes the new migration_journal module. |
| src/state/migration_journal.rs | New journal format + reconcile(root) recovery logic and unit tests. |
| src/nu/version_manager.rs | Active-version marker read/write + installed-version discovery + active binary resolution logic. |
| src/nu/paths.rs | Consults the active-version marker as a hint table during Nu executable discovery (after legacy-managed check). |
| src/nu/mod.rs | Registers new migrate_legacy and version_manager modules. |
| src/nu/migrate_legacy.rs | New journaled migration from legacy tools/nushell/nu to versioned layout with DI seams and tests. |
| src/nu/bootstrap.rs | Adds snapshotting + non-TTY consent guard + (pinned-only) active-marker persistence; adjusts PATH prompt hoisting. |
| src/cmd/use_cmd.rs | Implements `numan use |
| src/cmd/setup.rs | Adds binary validation and consolidated destructive confirmation for setup nu use/path flows via DI seams/tests. |
| src/cmd/doctor.rs | Adds journal.migration_pending finding and an Auto-tier repair that runs migration reconcile. |
| docs/plans/consolidated-multi-repo-roadmap.md | Updates roadmap semantics: numan use never auto-downloads; documents marker shape and ownership. |
| AGENTS.md | Documents new migration journal + active-version marker invariants. |
| .gitignore | Adds /.freebuff ignore entry. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0e467e842
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
PR Summary by QodoJournal legacy Nu migration and finalize
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Qodo Fixer✅ Committed (4) · ☑ Fixed (4) Commits pushed directly to this PR — no separate fix PR opened. Process — 4 fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
AGENTS.md (2)
93-96: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the new
util/modules to the structure block.The
util/subtree listsatomic.rs,fs_safety.rs, andhints.rs. This PR's stack also relies onsrc/util/stdio_redirect.rs(imported bysrc/cmd/doctor.rsLine 41 to keep--jsonstdout clean) andsrc/util/test_paths.rs(thePathRestoreGuardused to isolate PATH-sensitive tests). Neither appears in the map. The same omission was raised for thenu/subtree in an earlier review and fixed at Lines 91-92; apply it here.📝 Proposed documentation addition
util/ atomic.rs — write_json_atomic helper (tempfile+persist) fs_safety.rs — OWNERSHIP_MARKER, acquire_mutation_lock (advisory fd_lock mutex), assert_managed_file_owned (Phase 4) hints.rs — Canonical `fix` hint strings aligned with docs/numan-doctor.md (Phase 7.3) + stdio_redirect.rs — StdoutToStderr guard so nested repair output cannot corrupt `--json` stdout + test_paths.rs — PathRestoreGuard: serializes and restores `PATH` for PATH-sensitive testsAs per coding guidelines: "Update documentation and
AGENTS.mdwhen 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 93 - 96, Update the util/ structure block in AGENTS.md to include src/util/stdio_redirect.rs and src/util/test_paths.rs, with concise descriptions matching their roles in doctor JSON output handling and PATH-sensitive test isolation. Preserve the existing entries for atomic.rs, fs_safety.rs, and hints.rs.Source: Coding guidelines
68-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winState that mutating
numan usearms now run legacy migration.This line describes the snapshot and the mutation lock but not the migration step added in
src/cmd/use_cmd.rsLine 74. Legacy migration renames the Nu binary and writes the active-version marker, so it is a user-visible mutation performed bynuman use. The line also does not record thatlistis exempt from all three (lock, snapshot, migration), which is the contract the command's own comment at Lines 26-29 establishes.📝 Proposed documentation change
- use_cmd.rs — `numan use <version>|latest|list`: activates a previously installed managed Nu version (no auto-download); writes the active-version marker after a PreMutation snapshot under the root mutation lock + use_cmd.rs — `numan use <version>|latest|list`: activates a previously installed managed Nu version (no auto-download); mutating arms take the root mutation lock, snapshot (PreMutation), run journaled legacy migration, then write the active-version marker; `list` is read-only (no lock, snapshot, or migration)As per coding guidelines: "update
AGENTS.md,docs/, or command help when structure, conventions, or user-visible behavior changes."🤖 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` at line 68, Update the `use_cmd.rs` entry in `AGENTS.md` to state that mutating `numan use` variants acquire the root mutation lock, run the PreMutation snapshot, perform legacy migration, and write the active-version marker; explicitly note that `numan use list` is exempt from all four operations.Source: Coding guidelines
docs/numan-doctor.md (1)
141-150: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the two migration-journal findings to the check catalog.
Section 3 lists eight journal findings. This PR adds two more and both reach users:
journal.migration_pending—warn, Auto repair, hintnuman use(src/cmd/doctor.rsLines 665-674).journal.migration_invalid—error, Manual, no fix hint (src/cmd/doctor.rsLines 676-687).Both are registered in the human report's Journals section at
src/cmd/doctor.rsLines 1546-1547, andjournal.migration_pendingappears in the repair-policy table at Line 88's neighbourhood only implicitly. This document is the authority for the check catalog, so a reader auditing doctor's output against the spec finds two undocumented ids.📝 Proposed documentation addition
| `journal.lifecycle_pending` | `warn` | `state/pending-lifecycle.json` exists | **manual:** re-run or clear per op | | `journal.lifecycle_stale` | `error` | Stale lifecycle journal | **manual** | +| `journal.migration_pending` | `warn` | `state/migration-journal.json` exists and parses (stage `Prepared` \| `Renamed` \| `Active`) | **auto:** `migration_journal::reconcile` under the mutation lock, after a PreMutation snapshot | +| `journal.migration_invalid` | `error` | `state/migration-journal.json` is present but unreadable, unparseable, or carries an unsupported `schema_version` | **manual:** delete the journal file; `numan use` cannot reconcile an unreadable journal |Add a matching row to the repair-policy table at Lines 86-99 for the
journal.migration_pendingauto tier.As per coding guidelines: "update
AGENTS.md,docs/, or command help when structure, conventions, or user-visible behavior changes."🤖 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 `@docs/numan-doctor.md` around lines 141 - 150, Add both migration findings to the journal check catalog in docs/numan-doctor.md: document journal.migration_pending as warn with auto repair and the numan use hint, and journal.migration_invalid as error with manual repair and no fix hint. Also add journal.migration_pending to the repair-policy table with the auto tier, matching the corresponding doctor.rs behavior.Source: Coding guidelines
src/cmd/doctor.rs (2)
2418-2450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the failing-reconcile counterpart to this test.
doctor_fix_reconciles_migration_journalcovers the success path.doctor_fix_continues_after_absent_migration_journalcovers the absent path. Nothing covers the path wherereconcilereturnsErr— the branch at Lines 1464-1468 that recordsjournal.migration_repairedasFailed.That branch matters because it must retain the journal and must not stop later repairs.
MigrationJournalError::PreparedOrphanRemoveFailedis trivial to stage: aPreparedjournal plus a version directory containing one stray file, the same fixture already used atsrc/state/migration_journal.rsLines 568-604.💚 Proposed test
#[test] fn doctor_fix_records_failed_migration_repair_and_continues() { let dir = TempDir::new().unwrap(); let root = dir.path(); // `Prepared` journal over a version dir that `remove_dir` cannot clear. let tools = root.join("tools").join("nushell"); let version_dir = tools.join("0.113.1"); std::fs::create_dir_all(&version_dir).unwrap(); std::fs::write(version_dir.join("stray.dat"), b"foreign").unwrap(); PendingMigration { schema_version: crate::state::migration_journal::SCHEMA_VERSION, version: "0.113.1".to_string(), stage: crate::state::migration_journal::MigrationStage::Prepared, } .save(root) .unwrap(); // A second Auto repair that must still run after the migration fails. let marker = root.join("nu_state").join("active-version.json"); std::fs::create_dir_all(marker.parent().unwrap()).unwrap(); std::fs::write(&marker, b"{ not valid json").unwrap(); let args = DoctorArgs { scan: false, json: false, nupm_home: None, }; let report = run_checks_with_options( &DoctorArgs { scan: true, json: false, nupm_home: None, }, root, &test_doctor_options(), ) .unwrap(); let repairs = apply_repairs(&args, root, &report.findings, &test_doctor_options()).unwrap(); assert!( repairs.iter().any(|r| { r.id == "journal.migration_repaired" && r.status == RepairStatus::Failed }), "a failed reconcile must be recorded, not swallowed: {repairs:?}" ); assert!( PendingMigration::load(root).unwrap().is_some(), "the journal must survive a failed repair so the user can retry" ); assert!( repairs.iter().any(|r| { r.id == "nu.active_version.repaired" && r.status == RepairStatus::Applied }), "later repairs must still run after a failed migration repair: {repairs:?}" ); }As per coding guidelines: "Tests must cover failure modes, not only successful execution."
🤖 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 2418 - 2450, Add a test alongside doctor_fix_reconciles_migration_journal that creates a Prepared PendingMigration for a version directory containing a stray file, causing reconcile to fail with the journal retained. Run the repair flow with a second invalid active-version marker, then assert journal.migration_repaired is Failed, PendingMigration remains present, and nu.active_version.repaired is Applied to verify later repairs continue.Source: Coding guidelines
1476-1494: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThis repair deletes state without a snapshot and without checking
snapshot_ok.Every other snapshot-dependent repair in this function is gated: Lines 1174, 1225, 1252, 1292, 1331, and 1371 all skip with
snapshot_unavailablewhen the PreMutation snapshot failed. The migration repair at Lines 1445-1456 takes its own snapshot and fails the record when that snapshot fails. This block does neither. It acquires the lock at Line 1481 and callsclear_active_version, which removesnu_state/active-version.jsonoutright.When
snapshot_okisfalse— a malformed lockfile, a missing payload revision — this is the only repair in the pass that mutates state with no baseline anywhere.
docs/numan-doctor.mdLine 88 documents the behavior as intentional: "Independent of PreMutation success". The reasoning holds for a torn marker, since the content is already unusable. Two things do not follow from it:
- The repository rule has no carve-out. It requires a snapshot before doctor-repair state mutations.
- The marker can hold a valid off-tree
binary_paththat a partial write corrupted.write_active_version_with_binaryrecords the user's external Nu at an absolute path (src/nu/version_manager.rsLines 154-185). Deleting it loses the only record of that selection, and no other file holds it.Pick one and make it explicit in the code, not only in the spec:
🔧 Option A — gate on the existing snapshot like every neighbour
if findings .iter() .any(|f| f.id == "nu.active_version.invalid" && f.repair == RepairTier::Auto) { let id = "nu.active_version.repaired".to_string(); + if !snapshot_ok { + records.push(RepairRecord { + id, + status: RepairStatus::Skipped, + reason: Some("snapshot_unavailable".to_string()), + }); + } else { let _lock = acquire_mutation_lock(root)?; match version_manager::clear_active_version(root) {🔧 Option B — keep it unconditional and record why
Preserve the bytes before deleting, so the selection is recoverable:
// The marker is unreadable, so no PreMutation baseline can capture anything // useful from it. Copy the raw bytes aside before clearing: a torn marker may // still contain a recoverable off-tree `binary_path`. let marker = version_manager::active_version_path(root); let _ = std::fs::rename(&marker, marker.with_extension("json.corrupt"));Option A matches the surrounding code. Option B matches the documented intent. Either is fine; the current state matches the docs but not the rule, and the code carries no record of the decision.
As per coding guidelines: "Create a snapshot before mutating install, update, remove, activate, deactivate,
init --refresh, nupm import, or doctor-repair state."🤖 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 1476 - 1494, Update the active-version repair block around version_manager::clear_active_version to explicitly satisfy the snapshot-before-mutation rule: either skip and record snapshot_unavailable when snapshot_ok is false, matching neighboring repairs, or preserve the raw active-version marker before unconditional clearing so a recoverable binary_path is retained. Make the chosen behavior explicit in the code and keep the existing RepairRecord outcomes for successful or failed clearing.Source: Coding guidelines
src/state/migration_journal.rs (1)
521-561: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a test for the
Prepared-with-binary-already-moved recovery path.The reconcile tests cover the
Prepared-orphan branch three ways, but not the branch at Lines 331-350. That branch is the whole point of the filesystem-truth rule: a crash betweenrenameand theRenamedjournal advance. Without a test, a future edit can delete theversioned_binary_presentcheck and every test in this module still passes, while the active marker is silently never written.Two smaller gaps in the same module:
MigrationJournalError::UnsafeVersionReconcile(Line 321) andMigrationJournalError::UnsafeVersionWrite(Line 258) are both untested.💚 Proposed tests
#[test] fn reconcile_prepared_completes_when_binary_already_moved() { let tmp = TempDir::new().unwrap(); let root = tmp.path(); // Crash window: rename landed, the journal never advanced to Renamed. let version_dir = version_install_dir(root, "0.113.1"); std::fs::create_dir_all(&version_dir).unwrap(); std::fs::write(version_dir.join(bin_name()), b"binary").unwrap(); write_journal(root, "0.113.1", MigrationStage::Prepared); let recovered = reconcile(root).unwrap().unwrap(); assert_eq!(recovered.stage, MigrationStage::Prepared); assert!( version_dir.join(bin_name()).is_file(), "the moved binary must survive Prepared recovery" ); let active = read_active_version(root).unwrap().unwrap(); assert_eq!( active.version, "0.113.1", "Prepared recovery must complete the active-marker write" ); assert!(PendingMigration::load(root).unwrap().is_none()); } #[test] fn reconcile_refuses_traversal_version() { let tmp = TempDir::new().unwrap(); let root = tmp.path(); // Bypass `save`, which rejects unsafe versions, to simulate tampering. let path = PendingMigration::journal_path(root); std::fs::create_dir_all(path.parent().unwrap()).unwrap(); std::fs::write( &path, format!( r#"{{"schema_version":{SCHEMA_VERSION},"version":"../etc","stage":"prepared"}}"# ), ) .unwrap(); let err = reconcile(root).unwrap_err(); assert!(matches!( err, MigrationJournalError::UnsafeVersionReconcile { .. } )); assert!( PendingMigration::load(root).unwrap().is_some(), "a tampered journal must be retained for `numan doctor --fix`" ); } #[test] fn save_refuses_unsafe_version_component() { let tmp = TempDir::new().unwrap(); let err = PendingMigration { schema_version: SCHEMA_VERSION, version: "../escape".to_string(), stage: MigrationStage::Prepared, } .save(tmp.path()) .unwrap_err(); assert!(matches!( err, MigrationJournalError::UnsafeVersionWrite { .. } )); assert!(!PendingMigration::journal_path(tmp.path()).exists()); }As per coding guidelines: "Tests must cover failure modes, not only successful execution."
🤖 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 521 - 561, Extend the migration journal tests around reconcile and PendingMigration::save to cover Prepared recovery when the versioned binary already exists, asserting the binary survives, the active marker is written, and the journal is cleared. Add failure-mode tests for traversal versions that assert reconcile returns UnsafeVersionReconcile while retaining the journal, and for unsafe versions passed to save that assert UnsafeVersionWrite without creating the journal.Source: Coding guidelines
src/nu/migrate_legacy.rs (1)
252-256: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProvide a recovery path for
RenamedBinaryMissing.
numan use <version>andnuman use latestpropagatereconcilebefore switching versions, so the missing binary blocks both commands.numan use listis unaffected.doctor --fixcalls the same failing reconciliation and leaves the journal, whilejournal.migration_pendingstill recommendsnuman use. Classify this state as manual recovery and provide an explicit journal path and discard or reinstall command.🤖 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/migrate_legacy.rs` around lines 252 - 256, Update the migration recovery handling around migration_journal::reconcile to explicitly classify RenamedBinaryMissing as manual recovery instead of propagating a blocking error. Provide the journal path in the diagnostic and tell the user how to discard the journal or reinstall the missing version, while preserving existing recovery behavior for other journal states.src/cmd/use_cmd.rs (2)
107-118: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExtract the on-tree/off-tree marker write, and add coverage for the off-tree branch.
Lines 109-118 and Lines 163-170 are the same logic: compare the resolved binary against
version_binary(root, &version), then callwrite_active_versionorwrite_active_version_with_binary. Two copies of the rule that preserves a user's off-tree Nu selection. If one copy loses theelsearm,numan usesilently rewrites an off-tree marker into an on-tree one and Nu resolution breaks for that user.The tests compound this.
create_fake_versionat Lines 180-185 only builds on-tree installs, so every test in this module takes theifbranch.write_active_version_with_binary— including itsBinaryPathTraversalguard — is never exercised fromnuman use.♻️ Proposed refactor
+/// Persist the active-version marker, preserving an off-tree binary path when +/// the resolved binary lives outside `tools/nushell/<version>/`. +fn select_version(root: &Path, version: &str, installed_binary: &Path) -> Result<()> { + if installed_binary == version_manager::version_binary(root, version) { + version_manager::write_active_version(root, version) + } else { + version_manager::write_active_version_with_binary(root, version, installed_binary) + } + .with_context(|| format!("Failed to switch to Nu {}", version)) +}Then both call sites collapse to one line:
- let on_tree = version_manager::version_binary(root, &version); - if installed_binary == on_tree { - version_manager::write_active_version(root, &version)?; - } else { - version_manager::write_active_version_with_binary( - root, - &version, - &installed_binary, - )?; - } + select_version(root, &version, &installed_binary)?;💚 Proposed off-tree test
#[test] fn test_use_switch_preserves_off_tree_binary_path() { let tmp = TempDir::new().unwrap(); let root = tmp.path(); // Off-tree Nu recorded by `numan setup nu use <path>`. let external = tmp.path().join("external-nu"); std::fs::write(&external, "fake").unwrap(); version_manager::write_active_version_with_binary(root, "0.113.1", &external).unwrap(); execute( &UseArgs { version: "0.113.1".to_string(), }, root, ) .unwrap(); let active = version_manager::read_active_version(root).unwrap().unwrap(); assert_eq!(active.version, "0.113.1"); assert_eq!( active.binary_path.as_deref(), Some(external.to_string_lossy().as_ref()), "`numan use` must not downgrade an off-tree selection to an on-tree path" ); }As per coding guidelines: "Add or update tests for behavior changes, including relevant failure paths."
Also applies to: 162-170
🤖 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/use_cmd.rs` around lines 107 - 118, The active-version marker logic is duplicated and the off-tree path is untested. Extract the comparison and write behavior into a shared helper, then replace both call sites in the use flow with that helper while preserving on-tree and off-tree writes; add coverage for switching to a version whose recorded binary is outside the managed tree, including the existing binary-path validation path where appropriate.Source: Coding guidelines
272-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin that
listruns no migration, not only that it takes no snapshot.
test_use_list_takes_no_snapshotchecks for the absence ofstate/snapshots. The read-only contract at Lines 26-29 makes three claims — no lock, no snapshot, no migration — and only the snapshot claim is tested. Migration is the claim that mutates the most: it renames the Nu binary and writes the active-version marker.The assertion is also weak on its own terms. The test never runs a mutating arm, so
state/snapshotsis absent regardless of whatlistdoes.💚 Proposed test
#[test] fn test_use_list_runs_no_migration() { // Stage a legacy single-binary layout. A read-only `list` must leave it // exactly as-is; only the mutating arms may migrate. let tmp = TempDir::new().unwrap(); let root = tmp.path(); let legacy = version_manager::versioned_nu_dir(root) .join(if cfg!(windows) { "nu.exe" } else { "nu" }); std::fs::create_dir_all(legacy.parent().unwrap()).unwrap(); std::fs::write(&legacy, "fake legacy nu").unwrap(); execute( &UseArgs { version: "list".to_string(), }, root, ) .unwrap(); assert!( legacy.is_file(), "`numan use list` must not migrate the legacy binary" ); assert!( version_manager::read_active_version(root).unwrap().is_none(), "`numan use list` must not write the active-version marker" ); }🤖 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/use_cmd.rs` around lines 272 - 290, Extend the use-list test around `execute` to stage a legacy single-binary layout, then verify that `execute` with `UseArgs { version: "list" }` leaves the legacy binary in place and `version_manager::read_active_version(root)` remains unset. Replace or supplement the snapshot-only assertion so the test directly covers the read-only no-migration contract.tests/doctor_test.rs (2)
293-302: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose stdin on this subprocess; the two sibling subprocess tests already do.
doctor_json_default_stdout_is_valid_jsonsets.stdin(std::process::Stdio::null())at Line 825, anddoctor_json_scan_omits_repairs_fieldsets it at Line 865. This invocation does not. It inherits the test harness stdin.The risk is concrete for this specific test. It runs
doctorin default repair mode against a root with a malformed lockfile. Doctor's repair path reachessetup nu use, which is fail-closed on TTY perdocs/numan-doctor.mdLine 93. If any repair in that path prompts, the child blocks on a read that never returns and CI hangs until the job timeout rather than failing with a diff.🔧 Proposed fix
.env("NUMAN_ALLOW_UNSIGNED", "1") + .stdin(std::process::Stdio::null()) .output() .expect("run numan doctor --json with malformed lockfile");🤖 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 `@tests/doctor_test.rs` around lines 293 - 302, Add `.stdin(std::process::Stdio::null())` to the `Command` chain in the malformed-lockfile doctor subprocess test, before `.output()`, matching the sibling tests so the child cannot inherit the test harness stdin or block during repair.
33-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis fixture does two unrelated jobs, and it makes the test that uses it misleading.
nu_setup_repair_testis injected as doctor'snu_setup_repair. It asserts the argument contract doctor must honour —NuAction::Use, no deprecated flag, no--yes. That is its job as a test double, and those assertions are correct.Lines 55-68 then do something else entirely. The fixture seeds a managed install, calls the production
setup::execute_nuwith a deliberately missing binary path, asserts the managed tree survived, and returns that error. That is a test ofsetup's wipe-protection, embedded inside doctor's test double.The consequence surfaces in
doctor_fix_registers_off_path_nu_without_networkat Lines 521-557. The name says the repair registers the off-path Nu. The fixture guarantees it always fails, which is why Line 556 assertscode == 1. A reader cannot tell from the test name or body that the repair is expected to fail by construction, or why.Split the two concerns. Keep the doctor double minimal and put the wipe-protection assertion in its own test next to
execute_nu_use_existing_refuses_without_consent_and_keeps_managedat Lines 89-110, which already covers that ground.♻️ Proposed change
fn nu_setup_repair_test( args: &numan_cli::cmd::setup::NuSetupArgs, - root: &Path, + _root: &Path, ) -> anyhow::Result<()> { let expected = TEST_OFF_PATH.lock().unwrap().clone(); let Some(NuAction::Use { path }) = &args.action else { panic!("expected NuAction::Use, got {:?}", args.action); }; assert_eq!(Some(path.as_path()), expected.as_ref().map(|p| p.as_path())); assert!( args.use_existing.is_none(), "doctor must not use the deprecated flag" ); assert!( !args.yes, "doctor found_off_path repair must not pass --yes" ); *TEST_NU_SETUP_CALLED.lock().unwrap() = true; - - // Seed a managed install just before the production use path. ... - let managed = managed_nu_binary(root); - ... - Err(err) + Ok(()) }Then
doctor_fix_registers_off_path_nu_without_networkassertscode == 0and the name matches the behavior. Move the managed-survives assertion into a dedicated test that callssetup::execute_nudirectly.🤖 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 `@tests/doctor_test.rs` around lines 33 - 69, Split nu_setup_repair_test so it only validates the doctor-to-NuSetupArgs contract and returns success without invoking setup::execute_nu. Update doctor_fix_registers_off_path_nu_without_network to expect successful registration (code 0). Add a separate test beside execute_nu_use_existing_refuses_without_consent_and_keeps_managed that directly calls setup::execute_nu with a missing path, seeds the managed binary, and verifies it remains intact.src/util/hints.rs (1)
35-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
shell_quoteproduces POSIX-only syntax for a cross-platform hint.
shell_quotealways uses POSIX single-quote escaping. Windows paths contain backslashes, soshell_quotewraps them in single quotes (test at line 236-238 pins this). Single-quoted strings are not valid the same way incmd.exe, and PowerShell escapes embedded quotes with'', not'\''.
setup_nu_use_existingprints this hint on every platform, including Windows. A Windows user who copies the printednuman setup nu use '<path>'hint intocmd.exegets a broken command.Branch the quoting style on
cfg!(windows), or use double quotes for Windows paths.Also applies to: 234-239
🤖 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/util/hints.rs` around lines 35 - 83, Update shell_quote to produce Windows-compatible quoting when cfg!(windows) is enabled, using a style valid for the shell targeted by setup_nu_use_existing, including appropriate embedded-quote escaping; retain the existing POSIX single-quote behavior on non-Windows platforms and preserve the unquoted result for safe values.
♻️ Duplicate comments (1)
src/state/migration_journal.rs (1)
331-350: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the shared completion path; these 20 lines are a verbatim copy of the
Renamedbranch.Lines 331-350 and Lines 376-406 perform the same three steps: write the active version when no selection exists, remove a stray legacy binary, and surface the same two error variants. Two copies of a recovery path that must stay byte-identical will drift. A later fix applied to one stage will silently miss the other.
♻️ Proposed refactor
match journal.stage { MigrationStage::Prepared => { // The rename can complete before the journal advances to Renamed. // Trust the filesystem in that crash window and finish recovery. if versioned_binary_present(root, &journal.version) { - if read_active_version(root)?.is_none() { - write_active_version(root, &journal.version).map_err(|source| { - MigrationJournalError::RecoveryWriteActive { - version: journal.version.clone(), - source, - } - })?; - } - let bin_name = if cfg!(windows) { "nu.exe" } else { "nu" }; - let legacy_binary = versioned_nu_dir(root).join(bin_name); - if legacy_binary.is_file() { - if let Err(source) = std::fs::remove_file(&legacy_binary) { - return Err(MigrationJournalError::LegacyBinaryRemoveFailed { - path: PendingMigration::journal_path(root), - legacy_binary, - source, - }); - } - } + complete_migration(root, &journal.version)?; } else {Then reuse the same helper in the
Renamedarm:/// Finish a migration whose versioned binary is already in place: adopt the /// version as active when the user has made no other selection, then clear a /// stray legacy binary that would otherwise re-trigger migration. fn complete_migration(root: &Path, version: &str) -> Result<(), MigrationJournalError> { if read_active_version(root)?.is_none() { write_active_version(root, version).map_err(|source| { MigrationJournalError::RecoveryWriteActive { version: version.to_string(), source, } })?; } let legacy_binary = versioned_nu_dir(root).join(nu_binary_name()); if legacy_binary.is_file() { if let Err(source) = std::fs::remove_file(&legacy_binary) { return Err(MigrationJournalError::LegacyBinaryRemoveFailed { path: PendingMigration::journal_path(root), legacy_binary, source, }); } } Ok(()) }🤖 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 331 - 350, Extract the duplicated completion logic from the migration recovery flow into a shared helper, such as complete_migration, covering active-version adoption, legacy-binary removal, and the existing RecoveryWriteActive and LegacyBinaryRemoveFailed errors. Replace both the current branch in the versioned-binary path and the corresponding Renamed arm with calls to this helper, preserving the existing binary-name selection and 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 1440-1444: Update the repair pass around PendingMigration::load
and acquire_mutation_lock so errors are recorded as failed repairs and skipped
without returning early, allowing later repairs to continue. Follow the existing
PendingPluginDeactivate::load handling pattern, preserve the lock guard through
reconciliation, and add a test mirroring
doctor_fix_continues_after_absent_migration_journal that covers an unreadable
migration journal returning Err.
- Around line 352-357: Expose active_version_path with pub(crate) visibility so
it can be reused outside version_manager, then update the doctor error message
near read_active_version to call version_manager::active_version_path(root)
instead of rebuilding the nu_state/active-version.json path inline.
In `@src/cmd/setup.rs`:
- Around line 364-369: In execute_use_path and execute_use_existing, determine
managed_dir_was_present before any TTY/yes validation, then replace both
require_tty_or_yes calls with one opts.confirm.is_none() check using the
branch-appropriate message: the destructive managed Nushell wipe + PATH update
message when present, otherwise the generic PATH Nu registration message. Remove
the later duplicate safety checks while preserving the existing mutation
behavior.
In `@src/cmd/use_cmd.rs`:
- Around line 74-78: Update the migration flow in the caller around
migrate(root) to retain its returned bool and, when migration occurred, print a
concise user-facing message describing the completed legacy Nu layout migration
before continuing to op(root). Do not change the existing error propagation; if
silence is intentionally preserved instead, bind the result to _migrated and
document that intent briefly.
In `@src/nu/bootstrap.rs`:
- Around line 753-797: The already-installed Nushell path in the setup flow
passes a hardcoded true to confirm_or_bail, bypassing consent. In the existing
confirmation block around confirm_or_bail, pass options.yes instead, and add an
interactive test covering options.yes == false to verify the prompt is required
before PATH mutations.
In `@src/nu/migrate_legacy.rs`:
- Around line 308-311: Update the detect error conversion in the migration flow
around detect_legacy_version so DetectFailed wraps the original typed detector
error in a boxed source field rather than storing only e.to_string(). Preserve
the legacy binary path on DetectFailed, and apply the same typed-source wrapping
to the post_create hook error conversion.
In `@src/state/migration_journal.rs`:
- Around line 95-110: Remove the `{source}` interpolation from the
`#[error(...)]` display strings for both affected migration-journal variants,
including `PreparedOrphanRemoveFailed` and the corresponding variant around the
other referenced block. Preserve each `#[source]` field so callers can still
traverse the underlying I/O error through the error chain.
- Around line 240-244: Remove review-task and tool provenance identifiers from
the rationale comments while preserving their technical explanations: in
src/state/migration_journal.rs at lines 240-244, 352, and 563 remove
“copilot”/PR tags; in src/nu/migrate_legacy.rs at lines 234-237 and 416-417
remove the “cubic”/PR tags; in src/cmd/doctor.rs at lines 659-663 remove “PR69
WCk” and at lines 2556-2559 remove “Greptile:”. Keep only durable WHY-focused
rationale, including the unknown schema hard-failure, symlink refusal,
build-hash parsing, surfaced findings, and stale-finding explanation.
In `@src/util/test_paths.rs`:
- Around line 9-16: Update all tests that mutate PATH to use the shared
crate::util::test_paths::PathRestoreGuard, including the tests in
src/cmd/doctor.rs and the five PATH-mutating tests in src/nu/paths.rs. Remove or
replace the separate doctor-specific guard/mutex, and ensure each mutation is
protected for the full test scope through restoration.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 93-96: Update the util/ structure block in AGENTS.md to include
src/util/stdio_redirect.rs and src/util/test_paths.rs, with concise descriptions
matching their roles in doctor JSON output handling and PATH-sensitive test
isolation. Preserve the existing entries for atomic.rs, fs_safety.rs, and
hints.rs.
- Line 68: Update the `use_cmd.rs` entry in `AGENTS.md` to state that mutating
`numan use` variants acquire the root mutation lock, run the PreMutation
snapshot, perform legacy migration, and write the active-version marker;
explicitly note that `numan use list` is exempt from all four operations.
In `@docs/numan-doctor.md`:
- Around line 141-150: Add both migration findings to the journal check catalog
in docs/numan-doctor.md: document journal.migration_pending as warn with auto
repair and the numan use hint, and journal.migration_invalid as error with
manual repair and no fix hint. Also add journal.migration_pending to the
repair-policy table with the auto tier, matching the corresponding doctor.rs
behavior.
In `@src/cmd/doctor.rs`:
- Around line 2418-2450: Add a test alongside
doctor_fix_reconciles_migration_journal that creates a Prepared PendingMigration
for a version directory containing a stray file, causing reconcile to fail with
the journal retained. Run the repair flow with a second invalid active-version
marker, then assert journal.migration_repaired is Failed, PendingMigration
remains present, and nu.active_version.repaired is Applied to verify later
repairs continue.
- Around line 1476-1494: Update the active-version repair block around
version_manager::clear_active_version to explicitly satisfy the
snapshot-before-mutation rule: either skip and record snapshot_unavailable when
snapshot_ok is false, matching neighboring repairs, or preserve the raw
active-version marker before unconditional clearing so a recoverable binary_path
is retained. Make the chosen behavior explicit in the code and keep the existing
RepairRecord outcomes for successful or failed clearing.
In `@src/cmd/use_cmd.rs`:
- Around line 107-118: The active-version marker logic is duplicated and the
off-tree path is untested. Extract the comparison and write behavior into a
shared helper, then replace both call sites in the use flow with that helper
while preserving on-tree and off-tree writes; add coverage for switching to a
version whose recorded binary is outside the managed tree, including the
existing binary-path validation path where appropriate.
- Around line 272-290: Extend the use-list test around `execute` to stage a
legacy single-binary layout, then verify that `execute` with `UseArgs { version:
"list" }` leaves the legacy binary in place and
`version_manager::read_active_version(root)` remains unset. Replace or
supplement the snapshot-only assertion so the test directly covers the read-only
no-migration contract.
In `@src/nu/migrate_legacy.rs`:
- Around line 252-256: Update the migration recovery handling around
migration_journal::reconcile to explicitly classify RenamedBinaryMissing as
manual recovery instead of propagating a blocking error. Provide the journal
path in the diagnostic and tell the user how to discard the journal or reinstall
the missing version, while preserving existing recovery behavior for other
journal states.
In `@src/state/migration_journal.rs`:
- Around line 521-561: Extend the migration journal tests around reconcile and
PendingMigration::save to cover Prepared recovery when the versioned binary
already exists, asserting the binary survives, the active marker is written, and
the journal is cleared. Add failure-mode tests for traversal versions that
assert reconcile returns UnsafeVersionReconcile while retaining the journal, and
for unsafe versions passed to save that assert UnsafeVersionWrite without
creating the journal.
In `@src/util/hints.rs`:
- Around line 35-83: Update shell_quote to produce Windows-compatible quoting
when cfg!(windows) is enabled, using a style valid for the shell targeted by
setup_nu_use_existing, including appropriate embedded-quote escaping; retain the
existing POSIX single-quote behavior on non-Windows platforms and preserve the
unquoted result for safe values.
In `@tests/doctor_test.rs`:
- Around line 293-302: Add `.stdin(std::process::Stdio::null())` to the
`Command` chain in the malformed-lockfile doctor subprocess test, before
`.output()`, matching the sibling tests so the child cannot inherit the test
harness stdin or block during repair.
- Around line 33-69: Split nu_setup_repair_test so it only validates the
doctor-to-NuSetupArgs contract and returns success without invoking
setup::execute_nu. Update doctor_fix_registers_off_path_nu_without_network to
expect successful registration (code 0). Add a separate test beside
execute_nu_use_existing_refuses_without_consent_and_keeps_managed that directly
calls setup::execute_nu with a missing path, seeds the managed binary, and
verifies it remains intact.
---
Duplicate comments:
In `@src/state/migration_journal.rs`:
- Around line 331-350: Extract the duplicated completion logic from the
migration recovery flow into a shared helper, such as complete_migration,
covering active-version adoption, legacy-binary removal, and the existing
RecoveryWriteActive and LegacyBinaryRemoveFailed errors. Replace both the
current branch in the versioned-binary path and the corresponding Renamed arm
with calls to this helper, preserving the existing binary-name selection and
behavior.
🪄 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: 752fe759-b03c-4853-b1e8-0291d0aa2672
📒 Files selected for processing (14)
AGENTS.mddocs/numan-doctor.mdsrc/cmd/doctor.rssrc/cmd/setup.rssrc/cmd/use_cmd.rssrc/nu/bootstrap.rssrc/nu/migrate_legacy.rssrc/nu/paths.rssrc/nu/version_manager.rssrc/state/migration_journal.rssrc/util/hints.rssrc/util/mod.rssrc/util/test_paths.rstests/doctor_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)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/mod.rsdocs/numan-doctor.mdsrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rsAGENTS.mdsrc/cmd/setup.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/mod.rssrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rssrc/cmd/setup.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/mod.rsdocs/numan-doctor.mdsrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rsAGENTS.mdsrc/cmd/setup.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rust 2021 edition.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Library code must not panic; error paths should returnanyhow::Resultwith context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock viaacquire_mutation_lock(root)and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must usewrite_json_atomic.
numan installmust write only to$NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay 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; respectOWNERSHIP_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, andcargo fmt --check.
Every mutating command—includinginstall,remove,update,gc, and futurenupm import—must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must usewrite_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 lockfilemodule_activationvalue is authoritative.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass paths to Nu only throu...
Files:
src/util/mod.rssrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rssrc/cmd/setup.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/util/mod.rssrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rssrc/cmd/setup.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 realnuand should use injectable seams such asFakeCandidateRunneror registrars.
The nupm integration must be read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Unit tests must use
FakeCandidateRunneror injectable registrars and must not spawn a realnuprocess.
Files:
src/util/mod.rssrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rssrc/cmd/setup.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/mod.rsdocs/numan-doctor.mdsrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rsAGENTS.mdsrc/cmd/setup.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/mod.rsdocs/numan-doctor.mdsrc/nu/paths.rstests/doctor_test.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rsAGENTS.mdsrc/cmd/setup.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Usewrite_json_atomicfor all JSON state files to prevent partial-write corruption.
Use&Pathrather than&PathBufin function parameters.
Useanyhow::Resultfor application errors,thiserrorfor matchable library errors, add context with.context(...)or?, and never panic in library code.
Use the binary build target's platform information via#[cfg(target_env)]rather than runtime constants fromstd::env::consts.
Files:
src/util/mod.rssrc/nu/paths.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/cmd/doctor.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/nu/migrate_legacy.rssrc/cmd/setup.rs
**/*.md
📄 CodeRabbit inference engine (REVIEW.md)
Update documentation and
AGENTS.mdwhen project structure or conventions change.
Files:
docs/numan-doctor.mdAGENTS.md
src/nu/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Invoke Nu using paths and names supplied through environment variables; the Nu program string must be a compile-time constant with no runtime interpolation.
Files:
src/nu/paths.rssrc/nu/version_manager.rssrc/nu/bootstrap.rssrc/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 behavior with mock platforms.
Files:
tests/doctor_test.rs
src/cmd/{install,update,remove,activate,deactivate,init,nupm.rs,doctor,gc}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Create a snapshot before mutating install, update, remove, activate, deactivate,
init --refresh, nupm import, or doctor-repair state; garbage collection must treat snapshot-referenced payloads as live.
Files:
src/cmd/doctor.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Lockfiles must pin immutable artifact paths, and cached artifacts referenced by the lockfile must not be deleted.
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 written atomically and reconciled after interruption according to their documented stages.
Files:
src/state/migration_journal.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-03T02:41:41.522Z
Learning: Install operations must remain inert: they must not invoke Nu integration and may only write under `$NUMAN_ROOT`.
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-03T02:41:41.522Z
Learning: Run and maintain the CI gates: `cargo test`, `cargo clippy -- -D warnings`, `cargo fmt --check`, and ignored real-Nu acceptance tests where applicable.
🪛 markdownlint-cli2 (0.23.1)
docs/numan-doctor.md
[warning] 181-181: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 292-292: Files should end with a single newline character
(MD047, single-trailing-newline)
🔍 Remote MCP DeepWiki, GitHub Copilot
Review-relevant context
-
Migration stages are
Prepared → Renamed → Active; reconciliation trusts filesystem state, protects against symlinked managed directories, and retains journals when cleanup fails. -
The retrieved PR diff makes
numan use listacquire the mutation lock and run legacy migration before listing. This conflicts with the stated “read-only list” behavior; current tests only verify no snapshot, not absence of filesystem mutation. -
Switching commands snapshot before migration, while
listmigrates without a snapshot, creating inconsistent mutation semantics withinnuman use. -
Public version-management APIs now propagate typed
VersionManagerErrorvalues for malformed markers, invalid versions, directory-read failures, dangling selections, and traversal paths.
Take #70 injectable is_tty / require_tty_or_yes_with_tty for download and already-installed setup paths. Keep PR 69 refuse-in-place --force behavior and both sides' non-TTY regression tests. Co-authored-by: Anthony Thompson <github@trackdub.com>
Record migration-journal load errors as Failed without aborting later doctor repairs; document migration findings and AGENTS util/use_cmd; gate already-installed confirm on options.yes; extract migration complete_migration and use select_version; add coverage for Prepared recovery, unsafe versions, failed migration continuing, off-tree use, and list no-migration; close stdin on malformed-lockfile doctor subprocess; share PathRestoreGuard; strip review provenance comments. Co-authored-by: Anthony Thompson <github@trackdub.com>
Co-authored-by: Anthony Thompson <github@trackdub.com>
Preserve corrupt active-version bytes before doctor clear, classify RenamedBinaryMissing as manual migration recovery, and split the doctor off-PATH repair contract from wipe-protection coverage. Co-authored-by: Anthony Thompson <github@trackdub.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (3)
src/nu/paths.rs (1)
310-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrupt-marker handling now fails loud; add failure-path tests to lock it in.
The rewritten match in
find_nu_executable_with_root(Lines 320-350) no longer treats everyErrfromread_active_versionthe same way. It tolerates onlyReadActiveMarkerwithio::ErrorKind::NotFound(the TOCTOU case) and escalates every otherReadActiveMarkerand everyMalformedActiveMarkerwith an actionablenuman doctor --fixmessage. This closes the "wrong Nu" gap the earlier review flagged.None of the three tests you show for this function (Lines 848-933) exercise the escalation branches. Add a test that writes an unreadable or malformed
active-version.jsonand assertsfind_nu_executable_with_rootreturns anErrmentioningdoctor --fix, instead of silently falling through.🧪 Suggested test skeleton
#[test] fn find_nu_executable_with_root_escalates_malformed_marker() { let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("numan-root"); std::fs::create_dir_all(&root).unwrap(); let marker_path = crate::nu::version_manager::active_version_path(&root); std::fs::create_dir_all(marker_path.parent().unwrap()).unwrap(); std::fs::write(&marker_path, b"not json").unwrap(); let err = find_nu_executable_with_root(&root).unwrap_err(); assert!(err.to_string().contains("doctor --fix")); }As per coding guidelines: "Add or update tests for behavior changes, including relevant failure paths."
🤖 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/paths.rs` around lines 310 - 399, Add a failure-path test alongside the existing find_nu_executable_with_root tests that creates an active-version.json containing malformed data, invokes find_nu_executable_with_root, and asserts it returns an error whose message includes “doctor --fix”. Ensure the test sets up the marker location under a temporary root and does not allow fallback to PATH Nu.Source: Coding guidelines
src/nu/bootstrap.rs (2)
1191-1224: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore
PATHafterexecute_nu_setup_already_installed_non_tty_requires_yes.The
options_yesbranch of this test runsexecute_nu_setup_with_installerwithyes: trueagainst an already-installed binary. That path callsprepend_process_path(parent)?unconditionally (Lines 807-809), independent ofskip_path. The test never saves or restoresPATH, so the temp directory'sbinparent stays prepended to the processPATHfor the rest of the test binary run, even after theTempDiris dropped.This PR adds a shared
PATH_ENV_LOCK-guarded restoration type insrc/util/test_paths.rsfor exactly this purpose. Use it here so this test does not leak a dangling path entry intoPATHfor every test that runs afterward in the same process.🤖 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 1191 - 1224, Update the test execute_nu_setup_already_installed_non_tty_requires_yes to acquire the shared PATH_ENV_LOCK-guarded restoration type from test_paths before invoking either setup branch, ensuring the original PATH is restored when the test exits. Keep the existing assertions and installer behavior unchanged.
1252-1306: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIsolate the real Nu probe from this unit test.
The
PATHscan andregister_existing_nuboth invokevalidate_nushell_binary, which runsstd::process::Command::new(nu_exe). Mark this test#[ignore]or inject a validation seam.🤖 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 1252 - 1306, Isolate register_existing_nu_refuses_non_tty_without_yes_before_path_mutation from executing a real Nu binary: either mark the test #[ignore] or introduce and use a validation seam so PATH discovery and register_existing_nu do not invoke validate_nushell_binary through std::process::Command during this unit test.Source: Coding guidelines
🤖 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 `@docs/numan-doctor.md`:
- Line 89: Update the migration repair handling around acquire_mutation_lock in
the Doctor repair pass so a lock-acquisition failure is treated like other
migration repair failures: record or skip the failure without propagating it via
?, allowing subsequent repairs to continue. Preserve the existing behavior for
successful lock acquisition and reconciliation.
In `@src/cmd/doctor.rs`:
- Around line 1496-1497: Expose version_manager’s active_version_path helper as
pub(crate), then update the doctor code around marker and backup construction to
call it instead of rebuilding the nu_state/active-version.json path. Derive the
corrupt-backup path from the shared active-version path while preserving the
existing backup filename behavior.
- Around line 1448-1451: Update both apply_repairs lock-acquisition sites in
src/cmd/doctor.rs (1448-1451 and 1490-1495) to handle acquire_mutation_lock
errors without returning early: push the corresponding
journal.migration_repaired record with RepairStatus::Failed, continue or return
collected records, and bind the guard only on success. Add a test that holds the
mutation lock during apply_repairs and verifies both repairs are recorded as
Failed and the call returns Ok. No change is needed to docs/numan-doctor.md:89;
its wording should remain unchanged.
In `@src/nu/migrate_legacy.rs`:
- Around line 272-278: Add a test covering the auto-healed Renamed journal path
through migrate_legacy_install_with_detector: create the version install and
Renamed PendingMigration state, use a detector that must not run, assert the
call succeeds with false, and verify the active version marker is set and the
pending journal is cleared.
- Around line 762-765: Replace the OR assertion in the migration diagnostic test
with a direct check for the full “Discard the journal file to unblock” guidance,
so it fails if that recovery instruction is removed. Apply the same tightening
to the corresponding assertion in migration_journal.rs, preserving the existing
diagnostic formatting.
In `@src/state/migration_journal.rs`:
- Around line 305-306: Update the legacy binary path construction in the
migration journal to use the shared Nu binary-name helper, such as
nu_binary_file_name, instead of the inline cfg!(windows) selection. Keep
versioned_nu_dir(root) unchanged and join it with the helper’s result so legacy
and versioned paths use the same naming source.
---
Outside diff comments:
In `@src/nu/bootstrap.rs`:
- Around line 1191-1224: Update the test
execute_nu_setup_already_installed_non_tty_requires_yes to acquire the shared
PATH_ENV_LOCK-guarded restoration type from test_paths before invoking either
setup branch, ensuring the original PATH is restored when the test exits. Keep
the existing assertions and installer behavior unchanged.
- Around line 1252-1306: Isolate
register_existing_nu_refuses_non_tty_without_yes_before_path_mutation from
executing a real Nu binary: either mark the test #[ignore] or introduce and use
a validation seam so PATH discovery and register_existing_nu do not invoke
validate_nushell_binary through std::process::Command during this unit test.
In `@src/nu/paths.rs`:
- Around line 310-399: Add a failure-path test alongside the existing
find_nu_executable_with_root tests that creates an active-version.json
containing malformed data, invokes find_nu_executable_with_root, and asserts it
returns an error whose message includes “doctor --fix”. Ensure the test sets up
the marker location under a temporary root and does not allow fallback to PATH
Nu.
🪄 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: 69cb802c-3097-4793-abd3-a8e85c05e673
📒 Files selected for processing (10)
AGENTS.mddocs/numan-doctor.mdsrc/cmd/doctor.rssrc/cmd/setup.rssrc/cmd/use_cmd.rssrc/nu/bootstrap.rssrc/nu/migrate_legacy.rssrc/nu/paths.rssrc/state/migration_journal.rstests/doctor_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)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:
AGENTS.mddocs/numan-doctor.mdsrc/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/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:
AGENTS.mddocs/numan-doctor.mdsrc/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/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:
AGENTS.mddocs/numan-doctor.mdsrc/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/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:
AGENTS.mddocs/numan-doctor.mdsrc/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/cmd/doctor.rs
**/*.md
📄 CodeRabbit inference engine (REVIEW.md)
Update documentation and
AGENTS.mdwhen project structure or conventions change.
Files:
AGENTS.mddocs/numan-doctor.md
**/*.{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/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/cmd/doctor.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rust 2021 edition.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Library code must not panic; error paths should returnanyhow::Resultwith context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock viaacquire_mutation_lock(root)and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must usewrite_json_atomic.
numan installmust write only to$NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay 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; respectOWNERSHIP_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, andcargo fmt --check.
Every mutating command—includinginstall,remove,update,gc, and futurenupm import—must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must usewrite_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 lockfilemodule_activationvalue is authoritative.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass paths to Nu only throu...
Files:
src/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/cmd/doctor.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/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 realnuand should use injectable seams such asFakeCandidateRunneror registrars.
The nupm integration must be read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Unit tests must use
FakeCandidateRunneror injectable registrars and must not spawn a realnuprocess.
Files:
src/nu/paths.rssrc/cmd/use_cmd.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/cmd/doctor.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Derive platform detection from compile-time#[cfg(target_env)]values, notstd::env::consts;LIBCmust be a compile-time constant.
Use Rust 2021-compatible code and maintain compatibility with the declared MSRV of Rust 1.88.
Files:
src/nu/paths.rssrc/cmd/use_cmd.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/migrate_legacy.rssrc/cmd/doctor.rs
src/nu/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
FakeCandidateRunneras the test seam for module candidate validation rather than invoking a real Nu process in unit tests.
Files:
src/nu/paths.rssrc/nu/bootstrap.rssrc/nu/migrate_legacy.rs
src/cmd/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Create an activation snapshot before mutations performed by install, update, remove, activate, deactivate,
init --refresh, nupm import, or doctor repair.
Files:
src/cmd/use_cmd.rssrc/cmd/setup.rssrc/cmd/doctor.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Maintain unit and integration coverage, test platform-specific behavior with mock platforms, and run real-Nu acceptance tests only in the ignored acceptance suite.
Files:
tests/doctor_test.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/state/**/*.rs: Lockfiles must pin immutable artifact paths, and cached artifacts must be retained while referenced.
Write JSON state files atomically usingwrite_json_atomicwith a same-directory temporary file and persist operation.
Files:
src/state/migration_journal.rs
src/state/{journal,plugin_deactivate_journal,migration_journal,autoload_journal,lifecycle_journal}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Journal multi-step mutations and advance stages atomically so interrupted operations can be reconciled safely.
Files:
src/state/migration_journal.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-03T03:44:24.622Z
Learning: Do not force-push to `master`; use feature branches and squash-merge feature work.
🔍 Remote MCP GitHub Copilot
Additional review context
-
Related PR
#82shows the migration design usesPrepared → Renamed → Active, with atomic journal persistence and filesystem-truth-based reconciliation.Renamedrecovery writes the active marker only when no active selection already exists, preserving a user’s existing selection. -
PR
#82’snuman use listimplementation acquired the mutation lock and ran legacy migration before listing, whereas the current PR diff changeslistto bypass both locking and migration. This is a behavioral change that should be explicitly verified against the intended “read-only list” contract. -
The migration journal’s
Preparedreconciliation removes only the associated version directory and retains the journal if removal fails;Renamedreconciliation requires the versioned binary to exist and preserves an already-selected active version. These are important failure-path invariants for doctor repair. -
PR
#82reports the scope already spans 23 files and 48 commits, combining CLI confirmation changes, typed version-manager errors, setup locking, migration journaling, and path discovery. This breadth increases the risk of cross-feature regressions and makes focused validation of lock ordering and marker semantics especially important.
🔇 Additional comments (21)
src/state/migration_journal.rs (4)
242-246: Historical narration about a previous revision remains in the comment, but the durable rationale on lines 242-243 is correct and sufficient. Not worth a change request.
113-116: LGTM!
355-398: LGTM!
756-823: LGTM!src/nu/migrate_legacy.rs (2)
43-53: LGTM!
245-245: LGTM!Also applies to: 438-438
AGENTS.md (1)
68-68: LGTM!Also applies to: 83-83, 91-92, 97-99
src/cmd/use_cmd.rs (2)
102-120: LGTM!Also applies to: 165-165
287-346: LGTM!src/cmd/doctor.rs (3)
659-688: LGTM!
1666-1666: LGTM!
2444-2661: LGTM!docs/numan-doctor.md (1)
79-79: LGTM!Also applies to: 99-99, 134-134, 152-153
tests/doctor_test.rs (3)
35-35: LGTM!Also applies to: 54-54, 552-562
306-306: LGTM!
98-115: 🩺 Stability & AvailabilityKeep
execute_nu_use_existing_missing_path_keeps_managedungated. It runs independently of the Unix-only helper and neighboring test, including on Windows.> Likely an incorrect or invalid review comment.src/cmd/setup.rs (3)
302-302: LGTM!Also applies to: 439-439, 530-530
429-440: 🎯 Functional CorrectnessNo change required. The no-managed
execute_use_existingtest passesyes: true, so the nested TTY guard succeeds without anis_ttyoverride. Production callers do not injectconfirm.> Likely an incorrect or invalid review comment.
188-197: 🎯 Functional CorrectnessNo lock-ownership issue exists in the setup repair path.
execute_nu_implacquiressetup_subcommand_lockonce and delegates toexecute_nu_impl_locked;execute_nu_repaircan call it directly without a missing lock or double-lock deadlock.> Likely an incorrect or invalid review comment.src/nu/bootstrap.rs (2)
468-470: LGTM!Also applies to: 505-524, 713-717, 776-793, 824-834
1226-1250: LGTM!
Treat mutation-lock failures during migration and active-marker repairs as Failed records so later Auto repairs still run. Share active_version_path, cover Renamed auto-heal and malformed-marker lookup, isolate bootstrap PATH tests, and use nu_binary_name in the migration journal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Anthony Thompson <github@trackdub.com>
Consolidate setup nu use consent gates, surface legacy migration success, preserve typed detect/hook errors, share PathRestoreGuard across PATH tests, and keep doctor repairs moving after an unreadable migration journal. Co-authored-by: Anthony Thompson <github@trackdub.com>
Real-Nu CI runs cargo test with --ignored, which compiles rustdoc ignore fences. Use a text example so the illustrative snippet is not treated as a doctest. Co-authored-by: Anthony Thompson <github@trackdub.com>
170f1ed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/nu/bootstrap.rs (1)
541-562: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite the active-version marker before the persistent PATH change.
persist_path_dirat Line 543 modifies the shell profile on Unix and the user PATH on Windows.write_active_version_with_binaryat Line 561 runs after it. If the marker write fails, the user keeps a permanent PATH entry and has no active selection. Nu resolution then falls back to PATH with no record of the registration.The comment at Lines 505-507 already treats "failing after PATH mutation" as the hazard to avoid. The semver fallback removes one cause; an IO failure on the marker write remains.
Move the marker write ahead of the persistent PATH write. Keep
prepend_process_pathwhere it is, because it affects only this process.♻️ Proposed reorder
prepend_process_path(&parent)?; + version_manager::write_active_version_with_binary(root, &version, &resolved) + .with_context(|| format!("Failed to persist active Nu version '{}'", version))?; + if !options.skip_path { persist_path_dir(&parent)?;- version_manager::write_active_version_with_binary(root, &version, &resolved) - .with_context(|| format!("Failed to persist active Nu version '{}'", version))?; - println!();Add a test that forces the marker write to fail and asserts the shell profile is unchanged.
🤖 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 541 - 562, Move version_manager::write_active_version_with_binary ahead of persist_path_dir while keeping prepend_process_path unchanged, so marker persistence completes before any permanent PATH mutation. Add a test covering marker-write failure that verifies the shell profile remains unchanged.src/cmd/doctor.rs (1)
1080-1122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSoft-failing the lock lets later repairs mutate state without the mutation lock.
The soft-fail keeps
apply_repairsrunning, which is the stated goal. It also removes the lock from the repairs that never reacquire it:
- Line 1103:
create_snapshotcopies the lockfile while another process can be mutating it.- Line 1130:
create_dir_all(root.join(dir))for every layout repair.- Line 1154:
ensure_official_registry_configwritesconfig.toml.The comment at Lines 1145-1147 states the contract for that config write: "Config write does not reacquire the mutation lock; keep it under doctor's lock." After this change, that lock can be absent. A concurrent
numan installornuman usethen interleaves with a doctor snapshot and a config write.Keep the soft-fail, but gate the lock-owning section on
lock.is_some()and recordSkippedfor those repairs. The migration and active-version repairs reacquire the lock themselves, so they still reach theirFailedrecords.As per coding guidelines: "New mutating paths must acquire the mutation lock and snapshot the lockfile before making changes."
🔒 Proposed fix
let mut records = Vec::new(); // Snapshot failure must not block independent layout/config repairs. // Nested mutations that rely on a PreMutation baseline are skipped instead. let mut snapshot_ok = true; - if needs_lock { + if needs_lock && lock.is_some() { if let Err(e) = create_snapshot(- for dir in LAYOUT_DIRS { + for dir in LAYOUT_DIRS { let id = format!("layout.{dir}"); if findings .iter() .any(|f| f.id == id && f.severity == Severity::Warn) { + if lock.is_none() { + records.push(RepairRecord { + id, + status: RepairStatus::Skipped, + reason: Some("mutation_lock_unavailable".to_string()), + }); + continue; + } match std::fs::create_dir_all(root.join(dir)) {Apply the same
lock.is_none()guard to theregistry.nonerepair at Line 1148.Add a test that holds the lock, supplies a
layout.*finding and aregistry.nonefinding, and asserts both are recorded asSkippedand no directory or config file is written.🤖 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 1080 - 1122, Keep the soft-fail behavior in apply_repairs, but gate lock-owning mutations on lock.is_some(): skip and record RepairStatus::Skipped for the PreMutation create_snapshot flow, layout.* directory repairs, ensure_official_registry_config, and registry.none when lock.is_none(). Leave migration and active-version repairs reachable because they reacquire the lock themselves. Add a test that holds the lock with layout.* and registry.none findings, asserting both are Skipped and no directory or config file is created.Source: Coding guidelines
src/nu/migrate_legacy.rs (1)
309-332: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPre-journal empty-subdir cleanup still swallows removal failures.
Line 330 discards the
remove_dirresult withlet _ = .... If the directory is not actually empty,found_installedstaysfalseand migration proceeds as if the sibling directory is harmless. This repeats every invocation without surfacing the problem.This same failure mode was fixed for the journaled
Preparedstage insrc/state/migration_journal.rs, where removal failures are now surfaced instead of silently discarded. This loop is the pre-journal-era counterpart and still uses the old, discarded-error pattern.Treat an unremovable "empty" subdir like a populated install so a foreign directory blocks migration until the user resolves it, instead of retrying the same masked failure indefinitely.
🔧 Proposed fix
if entry.path().join(bin_name).exists() { found_installed = true; } else { - // Empty subdir — likely from an aborted previous migration. - // Remove it so the user is not permanently stuck with an - // empty <version>/ blocking every future attempt. - let _ = std::fs::remove_dir(entry.path()); + // Empty subdir — likely from an aborted previous migration. + // Remove it so the user is not permanently stuck with an + // empty <version>/ blocking every future attempt. If removal + // fails, the directory holds unexpected content; treat it + // like a populated install instead of silently retrying. + if std::fs::remove_dir(entry.path()).is_err() { + found_installed = true; + } }🤖 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/migrate_legacy.rs` around lines 309 - 332, Update the empty-subdirectory cleanup in the legacy migration loop to handle remove_dir failures instead of discarding them: when removal fails, set found_installed to true so the foreign or non-empty directory blocks migration; retain the current removal attempt and successful cleanup 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/nu/paths.rs`:
- Around line 912-935: Gate the test-only `test_paths` module in
`src/util/mod.rs` with `#[cfg(test)]`, ensuring `PathRestoreGuard`, its `new`
and `Default` implementations, and `PATH_ENV_LOCK` are not compiled or publicly
exported in release builds. Preserve access for unit tests such as
`find_nu_executable_with_root` without adding non-test exports.
---
Outside diff comments:
In `@src/cmd/doctor.rs`:
- Around line 1080-1122: Keep the soft-fail behavior in apply_repairs, but gate
lock-owning mutations on lock.is_some(): skip and record RepairStatus::Skipped
for the PreMutation create_snapshot flow, layout.* directory repairs,
ensure_official_registry_config, and registry.none when lock.is_none(). Leave
migration and active-version repairs reachable because they reacquire the lock
themselves. Add a test that holds the lock with layout.* and registry.none
findings, asserting both are Skipped and no directory or config file is created.
In `@src/nu/bootstrap.rs`:
- Around line 541-562: Move version_manager::write_active_version_with_binary
ahead of persist_path_dir while keeping prepend_process_path unchanged, so
marker persistence completes before any permanent PATH mutation. Add a test
covering marker-write failure that verifies the shell profile remains unchanged.
In `@src/nu/migrate_legacy.rs`:
- Around line 309-332: Update the empty-subdirectory cleanup in the legacy
migration loop to handle remove_dir failures instead of discarding them: when
removal fails, set found_installed to true so the foreign or non-empty directory
blocks migration; retain the current removal attempt and successful cleanup
behavior.
🪄 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: bf49094a-12dc-4271-93fa-db87b711c79e
📒 Files selected for processing (12)
src/cmd/doctor.rssrc/cmd/setup.rssrc/cmd/use_cmd.rssrc/nu/bootstrap.rssrc/nu/migrate_legacy.rssrc/nu/paths.rssrc/nu/version_manager.rssrc/state/migration_journal.rssrc/util/hints.rssrc/util/mod.rssrc/util/test_paths.rstests/doctor_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/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/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:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/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:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/nu/migrate_legacy.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rust 2021 edition.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Library code must not panic; error paths should returnanyhow::Resultwith context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock viaacquire_mutation_lock(root)and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must usewrite_json_atomic.
numan installmust write only to$NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay 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; respectOWNERSHIP_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, andcargo fmt --check.
Every mutating command—includinginstall,remove,update,gc, and futurenupm import—must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must usewrite_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 lockfilemodule_activationvalue is authoritative.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass paths to Nu only throu...
Files:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/nu/migrate_legacy.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/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 realnuand should use injectable seams such asFakeCandidateRunneror registrars.
The nupm integration must be read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Unit tests must use
FakeCandidateRunneror injectable registrars and must not spawn a realnuprocess.
Files:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/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:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/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:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rstests/doctor_test.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/nu/migrate_legacy.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Pass Nu paths and names only through
NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG, andNUMAN_PLUGIN_NAME; keep the Nu program string a compile-time constant with no runtime interpolation.
Files:
src/util/mod.rssrc/util/test_paths.rssrc/cmd/use_cmd.rssrc/util/hints.rssrc/nu/paths.rssrc/nu/bootstrap.rssrc/state/migration_journal.rssrc/cmd/setup.rssrc/nu/version_manager.rssrc/cmd/doctor.rssrc/nu/migrate_legacy.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use unit tests inline with source modules and integration tests under
tests/; test platform-specific code with mock platforms.
Files:
tests/doctor_test.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/state/**/*.rs: The lockfile is authoritative for module activation;autoload-state.jsonis only a derived fast-check projection.
Write JSON state files atomically usingwrite_json_atomicwith a temporary file in the same directory and persistence.
Files:
src/state/migration_journal.rs
src/state/migration_journal.rs
📄 CodeRabbit inference engine (AGENTS.md)
Journal legacy Nushell migration through
Prepared→Renamed→Active; reconcile filesystem truth and self-heal before migration, with doctor repair support.
Files:
src/state/migration_journal.rs
src/nu/version_manager.rs
📄 CodeRabbit inference engine (AGENTS.md)
Treat
nu_state/active-version.jsonas the sole authority for the selected managed Nushell version; preserve optional off-treebinary_pathdata.
Files:
src/nu/version_manager.rs
src/cmd/{install,update,remove,activate,deactivate,init,nupm,doctor}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Create a state snapshot before install, update, remove, activate, deactivate,
init --refresh, nupm import, and doctor repair mutations.
Files:
src/cmd/doctor.rs
🔍 Remote MCP GitHub Copilot
Relevant review context
- The reviewed PR is
#69, “journaled legacy migration”, targetingmaster; it addsMigrationStage::{Prepared, Renamed, Active}and typed migration/journal errors. Preparedreconciliation removes an orphan version directory and retains the journal if removal fails;Renamedrecovery requires the versioned binary, preserves an existing active selection, and retains the journal on manual-recovery errors.- Potential inconsistency:
migrate_legacy_install_with_detectorstill ignores failures from cleanup of pre-journal empty/version directories (let _ = std::fs::remove_dir(...)), whereas journaledPreparedreconciliation explicitly surfaces removal failures and retains recovery state. This may allow migration to continue despite an unresolved filesystem artifact. - The migration scan treats any existing
nupath as a populated install via.exists(), while journal reconciliation requires.is_file(). A directory or symlink namednucould therefore suppress migration in one path but not qualify as a valid binary in recovery. - The current
detect_legacy_versionimplementation uses blockingCommand::output()without a timeout, despite the stated concern that version probing occurs while migration holds the mutation lock. A hung legacy binary could block migration and serialized mutations indefinitely. Cargo.tomlpins Rust 1.88 andthiserror2; the new typed public error APIs should therefore be checked for compatibility with that declared toolchain.
🔇 Additional comments (15)
src/nu/version_manager.rs (1)
88-90: LGTM!Also applies to: 514-531, 565-575
src/nu/paths.rs (1)
604-618: LGTM!Also applies to: 841-841, 867-867, 890-890
src/cmd/doctor.rs (1)
1444-1506: LGTM!Also applies to: 1512-1564, 2652-2764
src/util/hints.rs (1)
35-39: LGTM!Also applies to: 75-80, 235-238
src/cmd/setup.rs (2)
365-379: LGTM!Also applies to: 458-470
436-439: 🗄️ Data Integrity & IntegrationNo change needed.
remove_managed_nu_if_presentonly removes the managed Nu directory and does not callclear_active_version, so the active marker already persisted byregister_existing_nuremains intact.src/nu/bootstrap.rs (2)
488-516: LGTM!
1076-1139: LGTM!Also applies to: 1185-1230, 1258-1298
src/state/migration_journal.rs (1)
37-38: LGTM!Also applies to: 95-110, 128-139, 288-317, 418-420, 677-679
src/nu/migrate_legacy.rs (2)
12-13: LGTM!Also applies to: 742-777, 807-810
60-65: 🗄️ Data Integrity & IntegrationNo external
DetectFailed/PostCreateHookdestructure found.The only Rust references for these variants are within
src/nu/migrate_legacy.rs, so no caller currently depends on the removedmessagefield.src/cmd/use_cmd.rs (1)
74-79: LGTM!src/util/test_paths.rs (1)
1-5: LGTM!Also applies to: 24-26
src/util/mod.rs (1)
6-7: LGTM!tests/doctor_test.rs (1)
18-18: LGTM!Also applies to: 431-432, 479-480, 517-518, 696-697
Keep PathRestoreGuard out of non-test library exports, and duplicate a local guard in doctor_test so integration tests still isolate PATH. Co-authored-by: Cursor <cursoragent@cursor.com>
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)
tests/doctor_test.rs (2)
130-146: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCompare the managed binary bytes before the failure.
managed.exists()is too weak: a failed off-path resolve must not delete or overwrite the managed installation, so readmanagedbeforeexecute_nu(...).expect_err(...)and assert the bytes are unchanged afterward.🤖 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 `@tests/doctor_test.rs` around lines 130 - 146, Update execute_nu_use_existing_missing_path_keeps_managed to read and store the managed binary’s bytes before calling setup::execute_nu. After the expected resolution error, read the file again and assert its bytes exactly match the pre-call contents, replacing the managed.exists() check while preserving the failure assertion.
86-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a failing setup-repair path for
doctor --fix.
nu_setup_repair_test()now records the call and returns success for all inputs. Keep that success case separate and cover a setup-repair failure withTestNushellBinaryvalidation or an injected error, assertingRepairStatus::Failedin JSON. Also cover the pre-condition path wherenu.binary.found_off_pathis not a warning so setup repair cannot run.🤖 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 `@tests/doctor_test.rs` at line 86, Add failure-path coverage to nu_setup_repair_test while preserving its existing success case: use TestNushellBinary validation or an injected error to make setup repair fail, then assert the doctor --fix JSON reports RepairStatus::Failed. Also add coverage for the precondition where nu.binary.found_off_path is not a warning and verify setup repair is not run.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/doctor_test.rs`:
- Around line 130-146: Update execute_nu_use_existing_missing_path_keeps_managed
to read and store the managed binary’s bytes before calling setup::execute_nu.
After the expected resolution error, read the file again and assert its bytes
exactly match the pre-call contents, replacing the managed.exists() check while
preserving the failure assertion.
- Line 86: Add failure-path coverage to nu_setup_repair_test while preserving
its existing success case: use TestNushellBinary validation or an injected error
to make setup repair fail, then assert the doctor --fix JSON reports
RepairStatus::Failed. Also add coverage for the precondition where
nu.binary.found_off_path is not a warning and verify setup repair is not run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10eac69a-9199-479f-ac50-86d09653950a
📒 Files selected for processing (3)
src/util/mod.rssrc/util/test_paths.rstests/doctor_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. (6)
- GitHub Check: Greptile Review
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Real-Nu acceptance (windows-latest)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 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/mod.rssrc/util/test_paths.rstests/doctor_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/util/mod.rssrc/util/test_paths.rstests/doctor_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/util/mod.rssrc/util/test_paths.rstests/doctor_test.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rust 2021 edition.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Library code must not panic; error paths should returnanyhow::Resultwith context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock viaacquire_mutation_lock(root)and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must usewrite_json_atomic.
numan installmust write only to$NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay 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; respectOWNERSHIP_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, andcargo fmt --check.
Every mutating command—includinginstall,remove,update,gc, and futurenupm import—must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must usewrite_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 lockfilemodule_activationvalue is authoritative.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass paths to Nu only throu...
Files:
src/util/mod.rssrc/util/test_paths.rstests/doctor_test.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/util/mod.rssrc/util/test_paths.rstests/doctor_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 realnuand should use injectable seams such asFakeCandidateRunneror registrars.
The nupm integration must be read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Unit tests must use
FakeCandidateRunneror injectable registrars and must not spawn a realnuprocess.
Files:
src/util/mod.rssrc/util/test_paths.rstests/doctor_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/util/mod.rssrc/util/test_paths.rstests/doctor_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/util/mod.rssrc/util/test_paths.rstests/doctor_test.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Acquireacquire_mutation_lock(root)before mutating shared state; the lock is non-blocking and a second acquisition must fail immediately.
Pass Nu paths and names through environment variables (NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG, andNUMAN_PLUGIN_NAME); keep the Nu program string a compile-time constant without runtime interpolation.
UsePluginActivationidentity(nu_executable_sha256, nu_version, plugin_registry_path)rather than a bare active boolean.
Usecargo fmtformatting and keep clippy clean undercargo clippy -- -D warnings.
Files:
src/util/mod.rssrc/util/test_paths.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use inline unit tests and integration tests under
tests/; test platform-specific behavior with mock platforms and follow test-first development.
Files:
tests/doctor_test.rs
🔍 Remote MCP GitHub Copilot
Relevant review context
- PR
#69introduces journal stagesPrepared → Renamed → Active; reconciliation uses filesystem state and retains journals when cleanup fails. - The current code still performs legacy pre-journal cleanup with
let _ = std::fs::remove_dir(...), so an unremovable populated directory can be ignored while migration continues. install_from_archivestill checks/installs against the managed destination while version discovery expectstools/nushell/<version>/nu; prior review context identifies this as a potential layout mismatch requiring verification.write_active_version_with_binaryrejects..components but still accepts relative paths, making off-tree resolution dependent on the process working directory.numan useupdates the active marker but does not refresh cachedNuPaths/activation identity state used by later lifecycle operations.- The latest reported CI state has several jobs still in progress; earlier PR-head checks reported compilation failures and unused imports, so final green CI should be confirmed before approval.
🔇 Additional comments (7)
src/util/mod.rs (1)
6-8: LGTM!src/util/test_paths.rs (1)
1-16: LGTM!Also applies to: 18-35, 37-49, 51-58, 60-64
tests/doctor_test.rs (5)
18-67: LGTM!
245-246: LGTM!
283-283: LGTM!
317-317: LGTM!
463-464: LGTM!Also applies to: 511-512, 549-550, 728-729
Resolve conflicts between landed #69/#71 on master and PR83 review hardening. Prefer PR83 safety gates (layout containment, exact latest dest, off-tree marker preserve, list read-only) while adopting master's DoctorArgs --scan API, confirm alias, and snapshot-before-setup wiring. Co-authored-by: Anthony Thompson <github@trackdub.com>
Summary
Promotes the legacy single-binary Nu install → versioned-layout transition to a journaled transaction (
Prepared→Renamed→Active) tracked atstate/migration-journal.json. Reconciliation lives at two layers: a self-healingreconcile(root)?at the top of everymigrate_legacy_install_with_detectorcall, plus an explicitnuman doctor --fix(Auto-tier) catch-up for users who never callednuman use.This branch is the second half of the split that landed alongside #67 — the numan-use feature PR stays small, the journaled migration lands as its own phase-1 cleanup PR.
Changes
New:
src/nu/migrate_legacy.rsExtracted from
nu::version_manager:pub type LegacyVersionDetector— injectable version-detection seam for tests.pub type LegacyPostCreateHook— fired aftercreate_dir_all(<version>/)and before the rename, simulating the original cross-device-rename bug.pub fn detect_legacy_version(binary)— production detector (VERSION metadata file preferred, fall back tonu --version).pub fn migrate_legacy_install(root)andpub fn migrate_legacy_install_with_detector(root, detect, post_create)— journaled migration with self-healing reconcile at entry.fn parse_nu_version_from_output(output)(private helper).Created-only journal reconcile, version-metadata-file detection, etc.New:
src/state/migration_journal.rsThe journal file format, lifecycle stages, and the
reconcile(root)self-heal implementation.SCHEMA_VERSION,PendingMigration { schema_version, version, stage },MigrationStage::{ Prepared, Renamed, Active }.PendingMigration::save/delete/loadagainststate/migration-journal.json(write-after-mkdir-stub viaatomic::write_json_atomic).reconcile(root)— for eachPrepared-only entry: remove the orphan<root>/tools/nushell/<version>/subdir the failedcreate_dir_allleft behind, thendeletethe journal. File-system truth takes precedence over journal stage.Updated:
src/cmd/doctor.rsjournal.migration_pending(Severity::Warn, RepairTier::Auto, fix hintnuman use).apply_repairs: when the finding is present ANDPendingMigration::load(...).is_some(), callsmigration_journal::reconcile(root)and records the result asjournal.migration_repaired.doctor_reports_migration_journal_findinganddoctor_fix_reconciles_migration_journal.Updated:
src/util/hints.rspub const CMD_USE: &str = "numan use";— fix-hint for the migration journal finding.Updated:
src/state/mod.rspub mod migration_journal;Updated:
src/nu/version_manager.rswrite_active_markerispub(crate)(consumed bymigrate_legacy::migrate_legacy_install_with_detector). All other migration-related code and tests moved out.Updated:
src/cmd/use_cmd.rsnuman useshould callcrate::nu::migrate_legacy::migrate_legacy_install(root)(andwrite_active_markerstays as the public hook).Testing
cargo test --lib— 468 passed (26 added: 24 migrate_legacy_* + 2 doctor migration-finding).cargo clippy -- -D warningsclean.cargo fmt --checkclean.Architecture Notes
The migration is a journaled transaction with two explicit stages and three transitions:
Preparedwritten beforecreate_dir_all(<version>/).Renamedwritten after the legacy binary has been moved into<version>/<bin>.Activereached afterwrite_active_version(root, &version)succeeds.Active, the journal is deleted.numan useinvocation ornuman doctor --fixreconciles by removing the orphan empty<version>/directory the failedcreate_dir_allleft behind, then clearing the journal — never auto-deleting a populated install.This is consistent with Numan's wider journal pattern (
journal.rs,autoload_journal.rs,lifecycle_journal.rs,plugin_deactivate_journal.rs): on-disk state is the source of truth, and journals exist to detect interrupted transitions, not to bypass the filesystem.Future Work
reconcileto pick up aRenamed-only entry by completing thewrite_active_versionstep when the on-tree binary exists (i.e., a fully renamed but unactivated install).Prepared-only,Renamed-only, andActive-reached state.write_active_markermigration call into a follow-up PR that re-adds the migrate line incmd/use_cmd.rs(post-merge gating by feature flag, then unconditionally after one release cycle).