diff --git a/docs/numan-doctor.md b/docs/numan-doctor.md index 5f4d82e7..1d53ddcc 100644 --- a/docs/numan-doctor.md +++ b/docs/numan-doctor.md @@ -149,8 +149,8 @@ Checks run in order below. Implementation should call existing validators (`NuPa | `journal.autoload_stale` | `error` | Journal identity mismatch | **confirm:** `init --refresh` then `activate` | | `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; hint `numan use` | -| `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 | +| `journal.migration_pending` | `warn` | `state/migration-journal.json` exists, parses, and [`validate_reconcile`](../src/state/migration_journal.rs) accepts it (normalized version, non-symlink managed tree, Renamed binary present or Prepared orphan empty/absent) | **auto:** `migration_journal::reconcile` under the mutation lock, after a PreMutation snapshot; hint `numan use ` when the versioned binary is present, else `numan doctor --fix` (or `numan setup nu ` to install) | +| `journal.migration_invalid` | `error` | journal unreadable/unparseable/unsupported `schema_version`, or `validate_reconcile` refuses (unsafe/non-normalizable version, symlink managed tree, Renamed missing binary, non-empty Prepared orphan) | **manual:** `numan setup nu ` when the binary is missing, or delete the stale journal; auto-reconcile refuses these states | ### 4. Lockfile and activation identity diff --git a/src/cmd/doctor.rs b/src/cmd/doctor.rs index 747e3eec..9ae34b7c 100644 --- a/src/cmd/doctor.rs +++ b/src/cmd/doctor.rs @@ -654,16 +654,68 @@ fn check_journals(root: &Path, nu_paths: Option<&NuPaths>, findings: &mut Vec findings.push(finding( - "journal.migration_pending", - Severity::Warn, - format!( - "Pending legacy-Nu migration journal (stage: {}, version: {})", - j.stage, j.version - ), - Some(CMD_USE), - RepairTier::Auto, - )), + Ok(Some(j)) => { + let journal_path = PendingMigration::journal_path(root); + // Only Auto when validate_reconcile says repair can succeed + // (normalization, symlink safety, Renamed binary presence, and + // Prepared orphan emptiness). Otherwise Manual so doctor --fix + // does not exit 0 after a Failed reconcile. + match migration_journal::validate_reconcile(root, &j) { + Err(e) => { + // Hint from journal stage + binary probe, not error-string wording. + let binary_present = match version_manager::normalize_version(&j.version) { + Ok(normalized) => { + version_manager::version_binary(root, &normalized).is_file() + } + Err(_) => false, + }; + let fix = if matches!(j.stage, migration_journal::MigrationStage::Renamed) + && !binary_present + { + format!( + "Run `{CMD_SETUP_NU} {}` to repair, or delete the stale journal at '{}'", + j.version, + journal_path.display() + ) + } else { + format!("Delete the stale journal at '{}'", journal_path.display()) + }; + findings.push(finding( + "journal.migration_invalid", + Severity::Error, + e.to_string(), + Some(&fix), + RepairTier::Manual, + )); + } + Ok(normalized) => { + // Hint `numan use ` when the versioned binary is present + // (switch can succeed after reconcile). Otherwise prefer + // doctor --fix / setup nu — Prepared without a binary only + // clears the journal. + let binary_present = + version_manager::version_binary(root, &normalized).is_file(); + let fix = if binary_present { + format!("{CMD_USE} {}", j.version) + } else { + format!( + "{CMD_DOCTOR_FIX} (or `{CMD_SETUP_NU} {}` to install)", + j.version + ) + }; + findings.push(finding( + "journal.migration_pending", + Severity::Warn, + format!( + "Pending legacy-Nu migration journal (stage: {}, version: {})", + j.stage, j.version + ), + Some(fix.as_str()), + RepairTier::Auto, + )); + } + } + } Ok(None) => {} Err(e) => { let journal_path = PendingMigration::journal_path(root); @@ -1226,20 +1278,31 @@ fn apply_repairs( reason: Some("snapshot_unavailable".to_string()), }); } else if let Some(off_path) = resolve_off_path(options) { - let setup_fn = options.nu_setup_repair.unwrap_or(setup::execute_nu_repair); - // Never pass `--yes` here: `setup nu use` may wipe a managed install - // and that path is fail-closed without explicit consent / TTY. - match setup_fn(&NuSetupArgs::use_existing(off_path, false, false), root) { - Ok(()) => records.push(RepairRecord { - id, - status: RepairStatus::Applied, - reason: None, - }), - Err(e) => records.push(RepairRecord { + // Doctor never auto-passes `--force`: wiping a managed install needs + // an explicit `numan setup nu use --force`. Skip with a clear reason + // instead of recording a Failed repair when the managed tree exists. + if crate::nu::bootstrap::managed_nu_dir(root).is_dir() { + records.push(RepairRecord { id, - status: RepairStatus::Failed, - reason: Some(e.to_string()), - }), + status: RepairStatus::Skipped, + reason: Some("managed_tree_present_requires_force".to_string()), + }); + } else { + let setup_fn = options.nu_setup_repair.unwrap_or(setup::execute_nu_repair); + // Never pass `--yes` here: `setup nu use` may wipe a managed install + // and that path is fail-closed without explicit consent / TTY. + match setup_fn(&NuSetupArgs::use_existing(off_path, false, false), root) { + Ok(()) => records.push(RepairRecord { + id, + status: RepairStatus::Applied, + reason: None, + }), + Err(e) => records.push(RepairRecord { + id, + status: RepairStatus::Failed, + reason: Some(e.to_string()), + }), + } } } else { records.push(RepairRecord { @@ -2396,7 +2459,15 @@ mod tests { f.message ); assert!(f.message.contains("0.113.1")); - assert_eq!(f.fix.as_deref(), Some(crate::util::hints::CMD_USE)); + assert!( + f.fix.as_deref().is_some_and(|s| { + s.contains(crate::util::hints::CMD_DOCTOR_FIX) + && s.contains(crate::util::hints::CMD_SETUP_NU) + && s.contains("0.113.1") + }), + "Prepared-without-binary hint must prefer doctor --fix / setup nu, got {:?}", + f.fix + ); } #[test] @@ -2455,6 +2526,68 @@ mod tests { ); } + #[test] + fn doctor_skips_off_path_repair_when_managed_tree_present() { + use std::sync::Mutex; + static OFF: Mutex> = Mutex::new(None); + static CALLED: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); + + fn discover() -> Option { + OFF.lock().ok()?.clone() + } + fn setup_must_not_run(_: &NuSetupArgs, _: &Path) -> Result<()> { + CALLED.store(true, std::sync::atomic::Ordering::SeqCst); + Ok(()) + } + + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(crate::nu::bootstrap::managed_nu_dir(root)).unwrap(); + let off_path = root.join("off-path-nu"); + std::fs::write(&off_path, b"fake").unwrap(); + *OFF.lock().unwrap() = Some(off_path); + CALLED.store(false, std::sync::atomic::Ordering::SeqCst); + + let findings = vec![Finding { + id: "nu.binary.found_off_path".to_string(), + severity: Severity::Warn, + message: "off path".to_string(), + fix: None, + repair: RepairTier::Confirm, + }]; + let args = DoctorArgs { + scan: false, + json: false, + nupm_home: None, + }; + let repairs = apply_repairs( + &args, + root, + &findings, + &DoctorOptions { + skip_network: true, + discover_off_path: Some(discover), + nu_setup_repair: Some(setup_must_not_run), + ..test_doctor_options() + }, + ) + .unwrap(); + + assert!( + !CALLED.load(std::sync::atomic::Ordering::SeqCst), + "setup repair must not run when managed tree exists" + ); + let record = repairs + .iter() + .find(|r| r.id == "nu.binary.found_off_path") + .expect("off-path repair record"); + assert_eq!(record.status, RepairStatus::Skipped); + assert_eq!( + record.reason.as_deref(), + Some("managed_tree_present_requires_force") + ); + } + /// A well-formed journal with an unknown `schema_version` must surface a /// `journal.migration_invalid` finding (Error severity, Manual repair tier) /// and must NOT produce a `journal.migration_pending` finding. @@ -2550,4 +2683,349 @@ mod tests { "invalid journal must NOT also produce a Pending finding" ); } + + /// Renamed journal + missing versioned binary: reconcile refuses this + /// state, so doctor must report Error (not Warn/Auto) so `doctor --fix` + /// cannot exit 0 while leaving corrupt migration state masked. + #[test] + fn doctor_reports_renamed_missing_binary_as_migration_invalid() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + // Bypass `save`'s unsafe-version guard is not needed; use a valid + // version string but leave the binary absent so Renamed is inconsistent. + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"0.113.1","stage":"renamed"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_invalid") + .expect("journal.migration_invalid for Renamed+missing binary"); + assert_eq!(f.severity, Severity::Error); + assert_eq!(f.repair, RepairTier::Manual); + assert!( + f.message.contains("Renamed") && f.message.contains("missing"), + "finding must name Renamed+missing: {}", + f.message + ); + assert!( + report + .findings + .iter() + .all(|f| f.id != "journal.migration_pending"), + "inconsistent Renamed journal must not also be Warn/Auto pending" + ); + // exit_code must be non-zero so safe-batch / CI do not treat this as healthy + assert_eq!(report.exit_code(), 1); + } + + /// Hand-edited journals may keep a safe `v`-prefix. Doctor must probe the + /// normalized layout path so Renamed + present `0.113.1/nu` is Warn/Auto, + /// not a false-positive Error/Manual "missing binary". + #[test] + fn doctor_renamed_probe_normalizes_v_prefix() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + let version_dir = version_manager::version_install_dir(root, "0.113.1"); + std::fs::create_dir_all(&version_dir).unwrap(); + let bin = if cfg!(windows) { "nu.exe" } else { "nu" }; + std::fs::write(version_dir.join(bin), b"binary").unwrap(); + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"v0.113.1","stage":"renamed"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + assert!( + report + .findings + .iter() + .all(|f| f.id != "journal.migration_invalid"), + "v-prefixed Renamed with normalized binary present must not be invalid" + ); + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_pending") + .expect("journal.migration_pending for recoverable Renamed"); + assert_eq!(f.severity, Severity::Warn); + assert_eq!(f.repair, RepairTier::Auto); + } + + #[test] + fn doctor_pending_renamed_with_binary_hints_numan_use() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + let version_dir = version_manager::version_install_dir(root, "0.113.1"); + std::fs::create_dir_all(&version_dir).unwrap(); + let bin = if cfg!(windows) { "nu.exe" } else { "nu" }; + std::fs::write(version_dir.join(bin), b"binary").unwrap(); + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"0.113.1","stage":"renamed"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_pending") + .expect("journal.migration_pending when Renamed binary present"); + assert!( + f.fix.as_deref().is_some_and(|s| { + s.starts_with(crate::util::hints::CMD_USE) && s.contains("0.113.1") + }), + "recoverable Renamed with binary should hint numan use, got {:?}", + f.fix + ); + } + + #[test] + fn doctor_reports_unsafe_migration_version_as_invalid() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + // Tampered journal: valid schema/stage, unsafe version component. + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"../etc","stage":"prepared"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_invalid") + .expect("journal.migration_invalid for unsafe version"); + assert_eq!(f.severity, Severity::Error); + assert_eq!(f.repair, RepairTier::Manual); + assert!( + f.message.contains("unsafe"), + "finding must mention unsafe component: {}", + f.message + ); + assert_eq!(report.exit_code(), 1); + } + + fn assert_migration_invalid_manual(report: &DoctorReport) { + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_invalid") + .expect("journal.migration_invalid"); + assert_eq!(f.severity, Severity::Error); + assert_eq!(f.repair, RepairTier::Manual); + assert!( + report + .findings + .iter() + .all(|x| x.id != "journal.migration_pending"), + "invalid journal must not also produce pending" + ); + assert_eq!(report.exit_code(), 1); + } + + /// Path-safe but non-semver journal versions fail validate_reconcile. + #[test] + fn doctor_reports_non_normalizable_migration_version_as_invalid() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"not-a-semver","stage":"prepared"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + assert_migration_invalid_manual(&report); + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_invalid") + .unwrap(); + assert!( + f.message.contains("non-normalizable") || f.message.contains("not-a-semver"), + "finding must name non-normalizable version: {}", + f.message + ); + } + + /// Symlinked managed tree is refused by validate_reconcile. + #[test] + fn doctor_reports_symlinked_managed_dir_migration_as_invalid() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + + let real_managed = dir.path().join("real-nushell"); + let version_dir = real_managed.join("0.113.1"); + std::fs::create_dir_all(&version_dir).unwrap(); + let bin = if cfg!(windows) { "nu.exe" } else { "nu" }; + std::fs::write(version_dir.join(bin), b"binary").unwrap(); + + let tools = root.join("tools"); + std::fs::create_dir_all(&tools).unwrap(); + let managed_link = tools.join("nushell"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real_managed, &managed_link).unwrap(); + #[cfg(windows)] + { + // Symlink creation needs Developer Mode or elevation on Windows. + // Skip rather than unwrap-fail when privileges are missing; Unix + // plus mock-platform coverage elsewhere still exercise reparse logic. + if std::os::windows::fs::symlink_dir(&real_managed, &managed_link).is_err() { + return; + } + } + + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"0.113.1","stage":"renamed"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + assert_migration_invalid_manual(&report); + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_invalid") + .unwrap(); + assert!( + f.message.contains("symlink") || f.message.contains("reparse"), + "finding must name symlink/reparse guard: {}", + f.message + ); + } + + /// Non-empty Prepared orphan cannot be remove_dir'd by reconcile. + #[test] + fn doctor_reports_nonempty_prepared_orphan_as_invalid() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + let version_dir = version_manager::version_install_dir(root, "0.113.1"); + std::fs::create_dir_all(&version_dir).unwrap(); + std::fs::write(version_dir.join("stray.dat"), b"foreign").unwrap(); + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{},"version":"0.113.1","stage":"prepared"}}"#, + crate::state::migration_journal::SCHEMA_VERSION + ), + ) + .unwrap(); + + let report = run_checks_with_options( + &DoctorArgs { + scan: true, + json: false, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + + assert_migration_invalid_manual(&report); + let f = report + .findings + .iter() + .find(|f| f.id == "journal.migration_invalid") + .unwrap(); + assert!( + f.message.contains("Prepared-but-orphan") || f.message.contains("not empty"), + "finding must name non-empty Prepared orphan: {}", + f.message + ); + } } diff --git a/src/cmd/remove.rs b/src/cmd/remove.rs index 6860ce55..7bd9ac23 100644 --- a/src/cmd/remove.rs +++ b/src/cmd/remove.rs @@ -16,7 +16,7 @@ pub struct RemoveArgs { /// Package to remove (owner/name) package: String, - /// Skip confirmation prompts (required in non-interactive sessions) + /// Skip interactive confirmation (required in non-interactive sessions) #[arg(long)] yes: bool, @@ -33,7 +33,7 @@ pub fn execute(args: &RemoveArgs, root: &Path) -> Result<()> { fn execute_with_tty(args: &RemoveArgs, root: &Path, is_tty: bool) -> Result<()> { // Destructive: permanently deletes the package payload and lockfile entry. // Refuse unattended (non-TTY) sessions without explicit --yes so safe-batch - // automation has to opt in; interactive sessions keep the existing flow. + // automation has to opt in; interactive TTY sessions still confirm below. crate::util::confirm::require_tty_or_yes_with_seam(args.yes, "package removal", is_tty)?; crate::util::confirm::confirm_or_bail( &format!( @@ -44,9 +44,10 @@ fn execute_with_tty(args: &RemoveArgs, root: &Path, is_tty: bool) -> Result<()> "Cancelled.", )?; - let _lock = acquire_mutation_lock(root)?; - - let mut lockfile = Lockfile::load(root)?; + // Validate before taking the mutation lock so a typo'd package id fails + // fast, and so an idle interactive prompt does not block other destructive + // ops on the same root (mirrors snapshot delete/rollback ordering). + let lockfile = Lockfile::load(root)?; let entry = match lockfile.packages.get(&args.package) { Some(e) => e.clone(), @@ -63,6 +64,40 @@ fn execute_with_tty(args: &RemoveArgs, root: &Path, is_tty: bool) -> Result<()> ); } + // Interactive confirmation after validation so a typo'd package id fails + // fast, and so `--yes` truly means "skip confirmation" rather than only + // the non-TTY gate. + crate::util::confirm::confirm_or_bail( + &format!( + "Remove package '{}' (payload will be deleted permanently)?", + args.package + ), + args.yes, + "Package removal cancelled.", + )?; + + let _lock = acquire_mutation_lock(root)?; + + // Reload under the lock so the confirm-time view cannot race a concurrent + // install/activate that landed while the user was at the prompt. + let mut lockfile = Lockfile::load(root)?; + let entry = match lockfile.packages.get(&args.package) { + Some(e) => e.clone(), + None => bail!( + "Package '{}' is no longer installed (removed while confirmation was pending).", + args.package + ), + }; + ensure_plugin_not_active(&entry, &args.package)?; + if !args.force && entry.module_activation.is_some() { + bail!( + "Package '{}' is currently active as a module. \ + Run `numan deactivate {}` first or use --force.", + args.package, + args.package + ); + } + let payload_path = entry.payload_path().to_string(); let payload_dir = root.join(&payload_path); diff --git a/src/cmd/setup.rs b/src/cmd/setup.rs index 6aedd9b5..e27ae5f3 100644 --- a/src/cmd/setup.rs +++ b/src/cmd/setup.rs @@ -272,19 +272,24 @@ fn execute_nu_impl_locked(args: &NuSetupArgs, root: &Path) -> Result<()> { return remove_managed_nu(root, args.yes); } if args.use_path { - eprintln!("warning: --use-path is deprecated, use 'numan setup nu path' instead"); - return execute_use_path(args.yes, root, args.force, ExecuteUseOpts::default()); + // Do not pass install-scoped `args.force` into the managed-tree + // destructive gate. Legacy flags always refuse when a managed tree + // exists; use the explicit subcommand with `--force` instead. + eprintln!( + "warning: --use-path is deprecated, use 'numan setup nu path' instead \ + (and 'numan setup nu path --force' if a managed tree must be replaced; \ + install --force does not authorize managed-tree deletion on this flag)" + ); + return execute_use_path(args.yes, root, false, ExecuteUseOpts::default()); } if let Some(existing) = &args.use_existing { - eprintln!("warning: --use-existing is deprecated, use 'numan setup nu use ' instead"); - reject_skip_path_for_off_path_registration(args.skip_path)?; - return execute_use_existing( - existing, - args.yes, - root, - args.force, - ExecuteUseOpts::default(), + eprintln!( + "warning: --use-existing is deprecated, use 'numan setup nu use ' instead \ + (and 'numan setup nu use --force' if a managed tree must be replaced; \ + install --force does not authorize managed-tree deletion on this flag)" ); + reject_skip_path_for_off_path_registration(args.skip_path)?; + return execute_use_existing(existing, args.yes, root, false, ExecuteUseOpts::default()); } match &args.action { @@ -436,7 +441,10 @@ fn execute_use_path(yes: bool, root: &Path, force: bool, opts: ExecuteUseOpts<'_ } } + // Snapshot true pre-operation state before preflight creates nu_state / + // probe files or any destructive managed-tree removal. snapshot_before_setup_mutation(root, SnapshotTrigger::Update)?; + preflight_active_marker_writable(root)?; remove_managed_nu_if_present(root)?; let options = NuSetupOptions { yes, @@ -450,8 +458,8 @@ fn execute_use_path(yes: bool, root: &Path, force: bool, opts: ExecuteUseOpts<'_ is_tty: None, }; let registered = bootstrap::register_existing_nu(Path::new(&path_nu), &options)?; - // chatgpt PR69 S08: persist the registered binary as the active version - // marker so `numan use list` reports it as the selection. + // Persist the registered binary as the active version marker so + // `numan use list` reports it as the selection. version_manager::write_active_version_with_binary(root, &normalized_version, ®istered)?; Ok(()) } @@ -527,7 +535,10 @@ fn execute_use_existing( version_manager::normalize_version(&detected.version)? }; + // Snapshot true pre-operation state before preflight creates nu_state / + // probe files or any destructive managed-tree removal. snapshot_before_setup_mutation(root, SnapshotTrigger::Update)?; + preflight_active_marker_writable(root)?; remove_managed_nu_if_present(root)?; let options = NuSetupOptions { yes, @@ -541,12 +552,36 @@ fn execute_use_existing( is_tty: None, }; let registered = bootstrap::register_existing_nu(path, &options)?; - // chatgpt PR69 S08: persist the registered binary as the active version - // marker so `numan use list` reports it as the selection. + // Persist the registered binary as the active version marker so + // `numan use list` reports it as the selection. version_manager::write_active_version_with_binary(root, &normalized_version, ®istered)?; Ok(()) } +/// Ensure `nu_state/` is creatable/writable before destructive PATH/off-path +/// registration. A later active-marker write failure after managed-tree +/// deletion + PATH mutation leaves Nu selection incomplete; refuse early. +fn preflight_active_marker_writable(root: &Path) -> Result<()> { + let nu_state = root.join("nu_state"); + std::fs::create_dir_all(&nu_state).with_context(|| { + format!( + "Failed to create nu_state directory '{}' before PATH/off-path Nu registration; \ + refusing destructive switch while the active-version marker cannot be written", + nu_state.display() + ) + })?; + let probe = nu_state.join(".numan-active-marker-write-probe"); + std::fs::write(&probe, b"ok").with_context(|| { + format!( + "Failed to write probe file in '{}' before PATH/off-path Nu registration; \ + refusing destructive switch while the active-version marker cannot be written", + nu_state.display() + ) + })?; + let _ = std::fs::remove_file(&probe); + Ok(()) +} + /// Remove the managed Nushell install, prompting unless `--yes`. fn remove_managed_nu(root: &Path, yes: bool) -> Result<()> { // PR #69 WCt: refuse the operation on a non-TTY session without @@ -614,18 +649,18 @@ fn remove_managed_nu(root: &Path, yes: bool) -> Result<()> { snapshot_before_setup_mutation(root, SnapshotTrigger::Remove)?; - // Clear after confirmation so decline leaves selection intact. Fail loud if - // the marker cannot be cleared; do not proceed to delete with a stale marker. - version_manager::clear_active_version(root).with_context(|| { + // Symlink refusal and delete must succeed before clearing the marker so a + // rejected managed tree leaves the active selection intact. + assert_not_symlink(&managed_dir, "managed Nushell directory")?; + std::fs::remove_dir_all(&managed_dir).with_context(|| { format!( - "Failed to clear active-version marker before removing managed Nu at '{}'", + "Failed to remove managed Nushell directory '{}'", managed_dir.display() ) })?; - - std::fs::remove_dir_all(&managed_dir).with_context(|| { + version_manager::clear_active_version(root).with_context(|| { format!( - "Failed to remove managed Nushell directory '{}'", + "Failed to clear active-version marker after removing managed Nu at '{}'", managed_dir.display() ) })?; @@ -636,7 +671,8 @@ fn remove_managed_nu(root: &Path, yes: bool) -> Result<()> { Ok(()) } -/// Silently remove the managed Nu directory if it exists (used by --use-existing). +/// Silently remove the managed Nu directory if it exists (used by +/// `setup nu path` / `setup nu use ` when replacing a managed tree). fn remove_managed_nu_if_present(root: &Path) -> Result<()> { let managed_dir = bootstrap::managed_nu_dir(root); // Clear the marker immediately before deleting the managed tree (confirm was @@ -644,30 +680,45 @@ fn remove_managed_nu_if_present(root: &Path) -> Result<()> { // no-op path cannot wipe a still-valid off-tree selection. Propagate clear // failures so deletion does not proceed with a stale marker. if managed_dir.is_dir() { - version_manager::clear_active_version(root).with_context(|| { + // Symlink refusal and delete must succeed before clearing the marker so a + // rejected managed tree leaves the active selection intact. + assert_not_symlink(&managed_dir, "managed Nushell directory")?; + std::fs::remove_dir_all(&managed_dir).with_context(|| { format!( - "Failed to clear active-version marker before removing managed Nu at '{}'", + "Failed to remove managed Nushell directory '{}'", managed_dir.display() ) })?; - std::fs::remove_dir_all(&managed_dir).with_context(|| { + version_manager::clear_active_version(root).with_context(|| { format!( - "Failed to remove managed Nushell directory '{}'", + "Failed to clear active-version marker after removing managed Nu at '{}'", managed_dir.display() ) })?; - println!("Removed managed Nushell at '{}'.", managed_dir.display()); + println!( + "Removed managed Nushell at '{}' (replaced by registered off-path Nu).", + managed_dir.display() + ); } Ok(()) } pub fn execute_loader(args: &LoaderArgs, root: &Path) -> Result<()> { - execute_loader_with_probe(args, || { - let nu_exe = find_nu_executable_with_root(root)?; - probe_nu_config_path(&nu_exe) + // Public entry holds the root mutation lock (same boundary as setup nu / + // numan use). The probe helper below stays unlocked so unit tests can + // inject a fake config path without contending on the advisory lock. + setup_subcommand_lock(root, "nushell-loader install", || { + execute_loader_with_probe(args, || { + let nu_exe = find_nu_executable_with_root(root)?; + probe_nu_config_path(&nu_exe) + }) }) } +/// Install loader.nu using an injected config-path probe. +/// +/// Unlocked test seam — production callers must go through [`execute_loader`], +/// which acquires [`setup_subcommand_lock`]. pub fn execute_loader_with_probe(args: &LoaderArgs, probe: F) -> Result<()> where F: FnOnce() -> Result, @@ -1044,6 +1095,69 @@ mod tests { assert!(!managed_dir.exists()); } + #[test] + fn remove_managed_nu_if_present_preserves_off_tree_marker_when_absent() { + let dir = TempDir::new().unwrap(); + let root = dir.path().join("numan-root"); + let off_tree = dir.path().join("external-nu"); + std::fs::write(&off_tree, b"fake").unwrap(); + version_manager::write_active_version_with_binary(&root, "0.113.1", &off_tree).unwrap(); + + remove_managed_nu_if_present(&root).unwrap(); + + let active = version_manager::read_active_version(&root) + .unwrap() + .expect("off-tree selection must survive a no-op managed removal"); + assert_eq!(active.version, "0.113.1"); + assert_eq!( + active.binary_path.as_deref(), + Some(off_tree.to_string_lossy().as_ref()) + ); + } + + /// Symlink refusal must not clear the active marker (marker is cleared only + /// after successful delete). + #[test] + fn remove_managed_nu_symlink_refusal_preserves_active_marker() { + let dir = TempDir::new().unwrap(); + let root = dir.path().join("numan-root"); + let real_managed = dir.path().join("real-nushell"); + std::fs::create_dir_all(&real_managed).unwrap(); + let bin = if cfg!(windows) { "nu.exe" } else { "nu" }; + std::fs::write(real_managed.join(bin), b"fake").unwrap(); + + let tools = root.join("tools"); + std::fs::create_dir_all(&tools).unwrap(); + let managed_link = tools.join("nushell"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real_managed, &managed_link).unwrap(); + #[cfg(windows)] + { + if std::os::windows::fs::symlink_dir(&real_managed, &managed_link).is_err() { + return; + } + } + + version_manager::write_active_version(&root, "0.113.1").unwrap(); + + let err = remove_managed_nu_if_present(&root).expect_err("symlink must refuse"); + let msg = err.to_string(); + assert!( + msg.contains("symlink") || msg.contains("reparse"), + "expected symlink/reparse refusal, got: {msg}" + ); + assert!( + version_manager::read_active_version(&root) + .unwrap() + .is_some(), + "active marker must survive symlink refusal" + ); + assert!( + managed_link.exists(), + "symlinked managed tree must remain after refusal" + ); + } + #[test] fn execute_use_existing_invalid_binary_preserves_managed_installation() { let dir = TempDir::new().unwrap(); @@ -1066,6 +1180,60 @@ mod tests { ); } + #[test] + fn preflight_active_marker_writable_creates_nu_state() { + let dir = TempDir::new().unwrap(); + let root = dir.path().join("numan-root"); + preflight_active_marker_writable(&root).unwrap(); + assert!( + root.join("nu_state").is_dir(), + "preflight must create nu_state so marker write can succeed" + ); + assert!( + !root + .join("nu_state") + .join(".numan-active-marker-write-probe") + .exists(), + "probe file must be cleaned up" + ); + } + + /// Production PATH/off-path flows call snapshot before preflight so a + /// preflight failure still leaves a PreMutation snapshot of the true + /// pre-operation root (without the nu_state/probe side effects). + #[test] + fn snapshot_occurs_before_preflight_nu_state_creation() { + use crate::state::snapshot::list_snapshots; + + let dir = TempDir::new().unwrap(); + let root = dir.path().join("numan-root"); + std::fs::create_dir_all(&root).unwrap(); + // Block preflight's create_dir_all(nu_state) by planting a file. + std::fs::write(root.join("nu_state"), b"blocked").unwrap(); + + snapshot_before_setup_mutation(&root, SnapshotTrigger::Update).unwrap(); + assert!( + !list_snapshots(&root).unwrap().is_empty(), + "snapshot must be recorded before preflight runs" + ); + + let err = preflight_active_marker_writable(&root) + .expect_err("preflight must fail when nu_state is a blocking file"); + let msg = err.to_string(); + assert!( + msg.contains("nu_state") || msg.contains("Failed"), + "preflight error should mention nu_state, got: {msg}" + ); + assert!( + root.join("nu_state").is_file(), + "blocked nu_state file must remain (preflight must not replace it)" + ); + assert!( + !list_snapshots(&root).unwrap().is_empty(), + "snapshot from before preflight must remain after preflight failure" + ); + } + #[test] fn remove_managed_nu_if_present_noop_when_absent() { let dir = TempDir::new().unwrap(); diff --git a/src/cmd/snapshot.rs b/src/cmd/snapshot.rs index bb513e62..d0a62fe7 100644 --- a/src/cmd/snapshot.rs +++ b/src/cmd/snapshot.rs @@ -231,12 +231,12 @@ fn delete_with_tty(root: &Path, id: &str, yes: bool, is_tty: bool) -> Result<()> // Refuse unattended (non-TTY) sessions without explicit --yes; interactive // sessions keep the confirmation prompt below. crate::util::confirm::require_tty_or_yes_with_seam(yes, "snapshot deletion", is_tty)?; - let _lock = acquire_mutation_lock(root)?; crate::util::confirm::confirm_or_bail( &format!("Delete snapshot '{id}'? This cannot be undone."), yes, "Cancelled.", )?; + let _lock = acquire_mutation_lock(root)?; delete_snapshot(root, id)?; println!("{} Deleted snapshot {}", console::style("✓").green(), id); Ok(()) @@ -251,7 +251,6 @@ fn rollback_with_tty(root: &Path, id: &str, yes: bool, is_tty: bool) -> Result<( // unattended sessions without explicit --yes; interactive sessions keep // the confirmation prompt (a pre-rollback snapshot is still taken first). crate::util::confirm::require_tty_or_yes_with_seam(yes, "snapshot rollback", is_tty)?; - let _lock = acquire_mutation_lock(root)?; crate::util::confirm::confirm_or_bail( &format!( "Roll back Numan-managed state to snapshot '{id}'? \ @@ -260,6 +259,7 @@ fn rollback_with_tty(root: &Path, id: &str, yes: bool, is_tty: bool) -> Result<( yes, "Cancelled.", )?; + let _lock = acquire_mutation_lock(root)?; let nu_paths = NuPaths::load(root)?; let runner = NuCandidateRunner::new(&nu_paths.nu_executable); diff --git a/src/cmd/use_cmd.rs b/src/cmd/use_cmd.rs index b877832c..bcd432f5 100644 --- a/src/cmd/use_cmd.rs +++ b/src/cmd/use_cmd.rs @@ -13,7 +13,7 @@ use std::path::Path; use crate::nu::paths::NuPaths; use crate::nu::version_manager; use crate::state::snapshot::{create_snapshot, SnapshotReason, SnapshotTrigger}; -use crate::util::fs_safety::acquire_mutation_lock; +use crate::util::fs_safety::setup_subcommand_lock; use crate::util::hints::CMD_INIT_REFRESH; #[derive(Args, Debug)] @@ -24,34 +24,35 @@ pub struct UseArgs { } pub fn execute(args: &UseArgs, root: &Path) -> Result<()> { - // Listing is read-only: do not lock, snapshot, or migrate the install. + // Listing is read-only: no lock, snapshot, or migrate. A flat legacy + // install is still surfaced via the VERSION marker without on-disk migrate. if args.version == "list" { return execute_list(root); } // Hold the mutation lock for the entire operation to prevent races // between concurrent `numan setup nu` and `numan use` invocations. - let _lock = acquire_mutation_lock(root)?; - - // Snapshot established state before any mutation. This covers both the - // legacy-migration step (rename + active-version write) and the version - // switch below. - create_snapshot( - root, - SnapshotReason::PreMutation, - SnapshotTrigger::Update, - None, - None, - ) - .with_context(|| "Failed to create pre-mutation snapshot for `numan use`")?; - - crate::nu::migrate_legacy::migrate_legacy_install(root) - .with_context(|| "Failed to migrate legacy Nu installation")?; - - match args.version.as_str() { - "latest" => execute_latest(root), - version => execute_switch(root, version), - } + setup_subcommand_lock(root, "Nu version switch", || { + // Snapshot established state before any mutation. This covers both the + // legacy-migration step (rename + active-version write) and the version + // switch below. + create_snapshot( + root, + SnapshotReason::PreMutation, + SnapshotTrigger::Update, + None, + None, + ) + .with_context(|| "Failed to create pre-mutation snapshot for `numan use`")?; + + crate::nu::migrate_legacy::migrate_legacy_install(root) + .with_context(|| "Failed to migrate legacy Nu installation")?; + + match args.version.as_str() { + "latest" => execute_latest(root), + version => execute_switch(root, version), + } + }) } /// List all installed Nu versions, marking the active one. @@ -197,6 +198,7 @@ fn refresh_cached_nu_paths_after_switch(root: &Path) -> Result<()> { mod tests { use super::*; use crate::state::snapshot::{list_snapshots, SnapshotReason, SnapshotTrigger}; + use crate::util::fs_safety::acquire_mutation_lock; use tempfile::TempDir; fn create_fake_version(root: &Path, version: &str) { @@ -259,13 +261,52 @@ mod tests { assert!( list_snapshots(root).unwrap().is_empty(), - "`numan use list` is read-only and must not create a snapshot" + "`numan use list` must not create a PreMutation snapshot" ); - // Active selection unchanged (no mutation / no rollback side effects). + // Active selection unchanged. let active = version_manager::read_active_version(root).unwrap().unwrap(); assert_eq!(active.version, "0.113.1"); } + #[test] + fn test_use_list_discovers_legacy_via_version_marker_without_mutation() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + let tools = version_manager::versioned_nu_dir(root); + std::fs::create_dir_all(&tools).unwrap(); + let bin = if cfg!(windows) { "nu.exe" } else { "nu" }; + std::fs::write(tools.join(bin), b"legacy").unwrap(); + std::fs::write(tools.join("VERSION"), "0.113.1\n").unwrap(); + + execute( + &UseArgs { + version: "list".to_string(), + }, + root, + ) + .unwrap(); + + assert!( + list_snapshots(root).unwrap().is_empty(), + "list must not create a PreMutation snapshot" + ); + assert!( + tools.join(bin).is_file(), + "list must leave the flat legacy binary in place" + ); + assert!( + !version_manager::version_binary(root, "0.113.1").is_file(), + "list must not migrate into tools/nushell//" + ); + assert!( + version_manager::list_installed_versions(root) + .unwrap() + .iter() + .any(|v| v == "0.113.1"), + "VERSION marker must still surface the legacy install to list" + ); + } + #[test] fn test_use_latest_no_versions() { let tmp = TempDir::new().unwrap(); diff --git a/src/nu/bootstrap.rs b/src/nu/bootstrap.rs index 0292cc28..f8c6e94b 100644 --- a/src/nu/bootstrap.rs +++ b/src/nu/bootstrap.rs @@ -809,6 +809,12 @@ where .unwrap_or_else(|| dest.clone()) }; if options.yes && effective.is_file() { + // PATH/marker mutation on the short-circuit path still needs a + // PreMutation snapshot (same AGENTS.md boundary as install). + snapshot_before_nu_setup( + root, + "Failed to create pre-mutation snapshot for existing `numan setup nu`", + )?; let tools_dir = match effective.parent() { Some(parent) => parent.to_path_buf(), None => managed_nu_dir(root), diff --git a/src/nu/paths.rs b/src/nu/paths.rs index c9242f17..091d5ae1 100644 --- a/src/nu/paths.rs +++ b/src/nu/paths.rs @@ -334,18 +334,13 @@ pub fn find_nu_executable_with_root(root: &Path) -> Result { { None } - VersionManagerError::ReadMarker { source, .. } => { - return Err(anyhow::anyhow!( - "Failed to read active-version marker (io: {source}); \ - a torn marker must not silently fall back to PATH Nu. \ - Run `numan doctor --fix` to reconcile." - )); - } + // Preserve VersionManagerError as the cause so `{:#}` / `{:?}` + // still surface the underlying io::Error or serde_json::Error. other => { - return Err(anyhow::anyhow!( - "Failed to parse active-version marker: {other}. \ - A malformed marker must not silently fall back to PATH Nu. \ - Run `numan doctor --fix` to reconcile." + return Err(anyhow::Error::new(other).context( + "Active-version marker is unreadable or malformed; \ + a torn marker must not silently fall back to PATH Nu. \ + Run `numan doctor --fix` to reconcile.", )); } } diff --git a/src/nu/version_manager.rs b/src/nu/version_manager.rs index 6c2558af..597fda03 100644 --- a/src/nu/version_manager.rs +++ b/src/nu/version_manager.rs @@ -478,7 +478,9 @@ pub(crate) fn nu_binary_name() -> &'static str { /// Legacy single-binary path that older installs wrote: /// `/tools/nushell/${bin}`. pub(crate) fn legacy_managed_binary_with_bin(root: &Path, bin: &str) -> PathBuf { - root.join("tools").join("nushell").join(bin) + // Own layout via versioned_nu_dir so migration cannot drift from the + // versioned managed-tree helper if tools/nushell ever moves. + versioned_nu_dir(root).join(bin) } #[cfg(test)] @@ -533,6 +535,56 @@ mod tests { assert_eq!(versions, vec!["0.114.0", "0.113.1", "0.112.0"]); } + #[test] + fn list_installed_versions_includes_legacy_binary_with_version_file() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + let tools = versioned_nu_dir(root); + std::fs::create_dir_all(&tools).unwrap(); + std::fs::write(tools.join(nu_binary_name()), b"legacy").unwrap(); + std::fs::write(tools.join("VERSION"), "0.113.1\n").unwrap(); + + let versions = list_installed_versions(root).unwrap(); + assert_eq!(versions, vec!["0.113.1"]); + } + + #[test] + fn list_installed_versions_omits_legacy_when_version_file_missing() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + let tools = versioned_nu_dir(root); + std::fs::create_dir_all(&tools).unwrap(); + std::fs::write(tools.join(nu_binary_name()), b"legacy").unwrap(); + + let versions = list_installed_versions(root).unwrap(); + assert!( + versions.is_empty(), + "legacy binary without VERSION must not invent a version: {versions:?}" + ); + } + + #[test] + fn list_installed_versions_omits_legacy_when_versioned_installs_exist() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + let binary_name = nu_binary_name(); + let tools = versioned_nu_dir(root); + std::fs::create_dir_all(&tools).unwrap(); + std::fs::write(tools.join(binary_name), b"legacy").unwrap(); + std::fs::write(tools.join("VERSION"), "0.112.0\n").unwrap(); + + let versioned = version_install_dir(root, "0.113.1"); + std::fs::create_dir_all(&versioned).unwrap(); + std::fs::write(versioned.join(binary_name), b"versioned").unwrap(); + + let versions = list_installed_versions(root).unwrap(); + assert_eq!( + versions, + vec!["0.113.1"], + "flat VERSION must be suppressed once any versioned install exists" + ); + } + #[test] fn test_is_version_installed() { let tmp = TempDir::new().unwrap(); diff --git a/src/state/migration_journal.rs b/src/state/migration_journal.rs index ec19d9d0..d2897852 100644 --- a/src/state/migration_journal.rs +++ b/src/state/migration_journal.rs @@ -34,7 +34,8 @@ use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use crate::nu::version_manager::{ - read_active_version, version_install_dir, versioned_nu_dir, write_active_version, + normalize_version, read_active_version, version_install_dir, versioned_nu_dir, + write_active_version, }; use crate::util::atomic::write_json_atomic; use crate::util::fs_safety::{assert_managed_nushell_layout, assert_not_symlink}; @@ -141,7 +142,7 @@ impl PendingMigration { if journal.schema_version != SCHEMA_VERSION { anyhow::bail!( "Migration journal at '{}' uses schema_version {} but this build expects {}. \ - Upgrade Numan or remove the stale journal.", +Upgrade Numan or remove the stale journal.", path.display(), journal.schema_version, SCHEMA_VERSION, @@ -191,13 +192,94 @@ impl PendingMigration { /// /// Takes precedence over the journal stage when there is disagreement — /// recovery actions are gated by what's actually on disk. +/// +/// Normalizes first so a safe-but-prefixed journal value like `v0.113.1` +/// probes `tools/nushell/0.113.1/` (the path migrate_legacy writes), +/// not `tools/nushell/v0.113.1/`. fn versioned_binary_present(root: &Path, version: &str) -> bool { - use crate::nu::version_manager::nu_binary_name; - version_install_dir(root, version) - .join(nu_binary_name()) + let Ok(normalized) = normalize_version(version) else { + return false; + }; + version_install_dir(root, &normalized) + .join(crate::nu::version_manager::nu_binary_name()) .is_file() } +/// Non-mutating preflight for [`reconcile`]. +/// +/// Returns `Ok(normalized_version)` only when Auto-tier repair can proceed +/// without failing mid-flight. Doctor uses this to classify journals as +/// `journal.migration_pending` (Auto) vs `journal.migration_invalid` (Manual) +/// without mutating the root, and reuses the normalized version for binary +/// presence probes. +pub fn validate_reconcile(root: &Path, journal: &PendingMigration) -> Result { + if !is_safe_version_component(&journal.version) { + bail!( + "Migration journal at '{}' has unsafe version component '{}'. \ + Refusing to reconcile to avoid escaping the managed tree. \ + Delete the stale journal to recover.", + PendingMigration::journal_path(root).display(), + journal.version + ); + } + + let version = normalize_version(&journal.version).map_err(|_| { + anyhow::anyhow!( + "Migration journal at '{}' has non-normalizable version '{}'. \ + Refusing to reconcile. Delete the stale journal to recover.", + PendingMigration::journal_path(root).display(), + journal.version + ) + })?; + + // Layout + symlink guards before any stage-specific recovery. + assert_managed_nushell_layout(root)?; + let managed_dir = versioned_nu_dir(root); + assert_not_symlink(&managed_dir, "managed Nushell directory")?; + + match journal.stage { + MigrationStage::Prepared => { + if versioned_binary_present(root, &version) { + return Ok(version); + } + // Orphan `/` must be empty so `remove_dir` can succeed. + let version_dir = version_install_dir(root, &version); + if version_dir.is_dir() && !dir_is_empty(&version_dir)? { + bail!( + "Migration journal at '{}' has '{}' as Prepared-but-orphan, \ + but the version directory '{}' is not empty and cannot be \ + removed by reconcile. Resolve the directory contents or \ + delete the stale journal.", + PendingMigration::journal_path(root).display(), + version, + version_dir.display(), + ); + } + Ok(version) + } + MigrationStage::Renamed => { + if !versioned_binary_present(root, &version) { + bail!( + "Migration journal at '{}' is staged 'Renamed' but the versioned binary \ + for '{}' is missing. Run `numan setup nu {}` to repair, or delete the \ + stale journal.", + PendingMigration::journal_path(root).display(), + version, + version, + ); + } + Ok(version) + } + MigrationStage::Active => Ok(version), + } +} + +fn dir_is_empty(dir: &Path) -> Result { + let mut entries = std::fs::read_dir(dir) + .with_context(|| format!("Failed to read directory '{}'", dir.display()))?; + Ok(entries.next().is_none()) +} + /// Reconcile any in-flight migration journal. /// /// Recovery actions: @@ -223,37 +305,19 @@ pub fn reconcile(root: &Path) -> Result> { return Ok(None); }; - // Refuse to act on a tampered or corrupted journal whose version - // contains path-traversal segments. `version_install_dir(, v)` - // appends v as a directory name; if v is `../etc` we would otherwise - // scrub a directory outside `/tools/nushell`. - if !is_safe_version_component(&journal.version) { - bail!( - "Migration journal at '{}' has unsafe version component '{}'. \ - Refusing to reconcile to avoid escaping the managed tree. \ - Delete the journal manually to recover.", - PendingMigration::journal_path(root).display(), - journal.version - ); - } - - // Validate once before any stage-specific recovery. Active-version writes, - // orphan scrubbing, and journal deletion must not run when the managed - // tree has been replaced by a symlink/reparse point or when a symlinked - // ancestor would redirect mutations outside `$NUMAN_ROOT`. + let version = validate_reconcile(root, &journal)?; let managed_dir = versioned_nu_dir(root); - assert_managed_nushell_layout(root)?; 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 versioned_binary_present(root, &version) { if read_active_version(root)?.is_none() { - write_active_version(root, &journal.version).with_context(|| { + write_active_version(root, &version).with_context(|| { format!( "Migration recovery: failed to write active version '{}'", - journal.version + version ) })?; } @@ -275,7 +339,7 @@ pub fn reconcile(root: &Path) -> Result> { // once the user resolves the underlying issue. Discarding // both the orphan dir AND the journal would silently lose // the recoverable crash window. - let version_dir = version_install_dir(root, &journal.version); + let version_dir = version_install_dir(root, &version); assert_not_symlink(&version_dir, "migration version install directory")?; if version_dir.is_dir() { if let Err(e) = std::fs::remove_dir(&version_dir) { @@ -286,7 +350,7 @@ pub fn reconcile(root: &Path) -> Result> { `numan doctor --fix`) can recover once permissions or \ the directory contents are resolved.", PendingMigration::journal_path(root).display(), - journal.version, + version, version_dir.display(), e ); @@ -297,23 +361,13 @@ pub fn reconcile(root: &Path) -> Result> { Ok(Some(journal)) } MigrationStage::Renamed => { - // File-system truth: the versioned binary should exist. - if !versioned_binary_present(root, &journal.version) { - anyhow::bail!( - "Migration journal at '{}' is staged 'Renamed' but the versioned binary is missing.\n\ - Run `numan setup nu {}` to repair.", - PendingMigration::journal_path(root).display(), - journal.version, - ); - } - // Complete the transaction — write active-version if no selection - // exists. (If a user has already chosen a different active - // version, that user-controlled choice takes precedence.) + // Presence already validated; complete the active-version write + // unless a user-controlled selection already exists. if read_active_version(root)?.is_none() { - write_active_version(root, &journal.version).with_context(|| { + write_active_version(root, &version).with_context(|| { format!( "Migration recovery: failed to write active version '{}'", - journal.version + version ) })?; } @@ -396,6 +450,80 @@ mod tests { assert!(PendingMigration::load(tmp.path()).unwrap().is_none()); } + /// Contract: unknown schema_version hard-fails (doctor journal.migration_invalid). + #[test] + fn load_rejects_unknown_schema_version() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + std::fs::write( + PendingMigration::journal_path(root), + br#"{"schema_version":999,"version":"0.113.1","stage":"prepared"}"#, + ) + .unwrap(); + + let err = PendingMigration::load(root).unwrap_err().to_string(); + assert!( + err.contains("schema_version"), + "err must name schema_version: {err}" + ); + assert!( + err.contains("999"), + "err must surface the actual value: {err}" + ); + assert!( + err.contains(&SCHEMA_VERSION.to_string()), + "err must name expected schema: {err}" + ); + } + + /// Path-traversal guard on save: refuse unsafe version components before write. + #[test] + fn save_refuses_unsafe_version_component() { + let tmp = TempDir::new().unwrap(); + let j = PendingMigration { + schema_version: SCHEMA_VERSION, + version: "../etc".to_string(), + stage: MigrationStage::Prepared, + }; + let err = j.save(tmp.path()).unwrap_err().to_string(); + assert!( + err.contains("unsafe version component"), + "err must name the guard: {err}" + ); + assert!( + !PendingMigration::journal_path(tmp.path()).exists(), + "unsafe save must not write a journal file" + ); + } + + /// Path-traversal guard on reconcile: tampered on-disk journal is retained + /// for manual repair (not deleted on refusal). + #[test] + fn reconcile_refuses_tampered_version_component() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + std::fs::create_dir_all(root.join("state")).unwrap(); + // Bypass `save`'s guard to simulate a tampered on-disk journal. + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{SCHEMA_VERSION},"version":"../etc","stage":"prepared"}}"# + ), + ) + .unwrap(); + + let err = reconcile(root).unwrap_err().to_string(); + assert!( + err.contains("unsafe version component"), + "err must name the guard: {err}" + ); + assert!( + PendingMigration::journal_path(root).exists(), + "tampered journal must be retained for manual repair" + ); + } + #[test] fn delete_removes_file() { let tmp = TempDir::new().unwrap(); @@ -531,6 +659,36 @@ mod tests { assert!(PendingMigration::load(root).unwrap().is_none()); } + /// Hand-edited `v0.113.1` journals must reconcile against the normalized + /// `tools/nushell/0.113.1/` path that migrate_legacy writes. + #[test] + fn reconcile_renamed_normalizes_v_prefix_before_path_probe() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path(); + + 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(); + + // Bypass save() so the on-disk journal keeps the v-prefix. + std::fs::create_dir_all(root.join("state")).unwrap(); + std::fs::write( + PendingMigration::journal_path(root), + format!( + r#"{{"schema_version":{SCHEMA_VERSION},"version":"v0.113.1","stage":"renamed"}}"# + ), + ) + .unwrap(); + + let recovered = reconcile(root).unwrap().unwrap(); + assert_eq!(recovered.version, "v0.113.1"); + assert_eq!(recovered.stage, MigrationStage::Renamed); + + let active = read_active_version(root).unwrap().unwrap(); + assert_eq!(active.version, "0.113.1"); + assert!(PendingMigration::load(root).unwrap().is_none()); + } + #[test] fn reconcile_renamed_skips_active_write_when_already_set() { let tmp = TempDir::new().unwrap(); @@ -691,29 +849,6 @@ mod tests { // ── failure-path tests ────────────────────────────────────────────────── - /// Loading a journal with an unknown `schema_version` must fail, not - /// silently coerce the record to the current schema. - #[test] - fn load_rejects_unknown_schema_version() { - let tmp = TempDir::new().unwrap(); - let root = tmp.path(); - let path = PendingMigration::journal_path(root); - std::fs::create_dir_all(path.parent().unwrap()).unwrap(); - let content = serde_json::json!({ - "schema_version": 9999, - "version": "0.113.1", - "stage": "prepared" - }); - std::fs::write(&path, serde_json::to_vec(&content).unwrap()).unwrap(); - - let err = PendingMigration::load(root).unwrap_err(); - let msg = err.to_string(); - assert!( - msg.contains("schema_version") || msg.contains("9999"), - "error must mention schema_version or unknown value, got: {msg}" - ); - } - /// Saving a journal with a version containing path-traversal components /// must fail without creating the file on disk. #[test] diff --git a/src/util/fs_safety.rs b/src/util/fs_safety.rs index 22c3e7de..0536b0d3 100644 --- a/src/util/fs_safety.rs +++ b/src/util/fs_safety.rs @@ -100,12 +100,12 @@ pub fn acquire_mutation_lock(root: &Path) -> Result { /// release the lock on return. /// /// Every destructive setup entry point (install, off-path registration, -/// PATH-Nu registration, managed removal, derive/active/upgrade via -/// `numan use`) flows through this helper so the lock boundary has exactly -/// one source of truth — closing PR #69's WCr (`setup_family_mutation_lock`) -/// and ensuring that a concurrent `numan use`, `numan install`, or -/// `numan doctor --fix` cannot interleave filesystem mutations on the same -/// root. +/// PATH-Nu registration, managed removal, `numan setup loader`, and +/// derive/active/upgrade via `numan use`) flows through this helper so the +/// lock boundary has exactly one source of truth — closing PR #69's WCr +/// (`setup_family_mutation_lock`) and ensuring that a concurrent `numan use`, +/// `numan install`, or `numan doctor --fix` cannot interleave filesystem +/// mutations on the same root. /// /// `what` is a short human-readable label (e.g. `"Nushell install"`, /// `"off-path Nu registration"`, `"managed Nushell removal"`); it lands diff --git a/tests/doctor_test.rs b/tests/doctor_test.rs index dbbb9f2c..f9beb3d3 100644 --- a/tests/doctor_test.rs +++ b/tests/doctor_test.rs @@ -37,11 +37,15 @@ fn nu_setup_repair_test( let expected = TEST_OFF_PATH.lock().unwrap().clone(); // The doctor passes the off-path binary via NuSetupArgs::use_existing(), // which sets action = Some(NuAction::Use { path, force }) and leaves use_existing unset. - let Some(numan_cli::cmd::setup::NuAction::Use { path, force }) = &args.action else { + let Some(NuAction::Use { path, force }) = &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!(!force, "doctor must not set force=true on NuAction::Use"); + assert!( + !*force, + "doctor found_off_path repair must not pass --force" + ); assert!( args.use_existing.is_none(), "doctor must not use the deprecated flag" diff --git a/tests/setup_nu_test.rs b/tests/setup_nu_test.rs index 2b495fa3..e63ca0a0 100644 --- a/tests/setup_nu_test.rs +++ b/tests/setup_nu_test.rs @@ -95,14 +95,14 @@ fn setup_nu_uses_injected_installer_without_network() { } #[test] -fn execute_nu_command_wraps_installer() { +fn execute_nu_command_short_circuits_pinned_install_without_network() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); let _path_guard = PathRestoreGuard::new(); - // Pre-install at the versioned path so execute_nu short-circuits without - // network. The versioned layout is now the only gate checked; the legacy - // flat path no longer triggers the short-circuit. + // Pin-only short-circuit at the versioned path (legacy flat path is not a + // gate). When the exact requested version binary exists, setup must not + // hit the network installer and must still persist the active-version marker. let version = "0.113.1"; let bin_dir = version_manager::version_install_dir(root, version); std::fs::create_dir_all(&bin_dir).unwrap(); @@ -114,6 +114,17 @@ fn execute_nu_command_wraps_installer() { root, ) .unwrap(); + + let active = version_manager::read_active_version(root).unwrap().unwrap(); + assert_eq!( + active.version, version, + "pinned short-circuit must still write the active-version marker" + ); + assert_eq!( + std::fs::read(&binary).unwrap(), + b"fake nu", + "existing pinned binary must not be replaced or reinstalled" + ); } /// Return the first runnable Nushell binary on `$PATH` (or `/usr/local/bin/nu` on Unix).