diff --git a/AGENTS.md b/AGENTS.md index 2bb04895..4b8e018d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,7 +54,7 @@ src/ registry.rs — Registry management subcommands activate.rs — Plugin + module activation (Phase 3 & 4); public entry: execute_with_candidate_runner init.rs — `numan init [--refresh]`: Nu probe, paths cache, auto-configures official registry - doctor.rs — `numan doctor [--scan]`: health checks + safe repairs by default (or report-only with `--scan`) (Phase 7.2; spec: docs/numan-doctor.md) + doctor.rs — `numan doctor [--scan] [--json]`: repairs by default; `--scan` report-only (Phase 7.2; spec: docs/numan-doctor.md) snapshot.rs — `numan snapshot list|inspect|delete|rollback` (Phase 5.3) deactivate.rs — Plugin + module deactivation: journaled plugin unregister (`execute_with_unregistrar`); module full/partial (Phase 4 / Issue #22 PR2) plugin_lifecycle.rs — Activate/deactivate-owned lifecycle boundary exposed to opt-in update orchestration (Issue #22 PR3) @@ -109,7 +109,7 @@ tests/ fixtures/nupm/ — supported/rejected fixture corpus for parser/classifier tests init_test.rs — `numan init` / `init --refresh` (vendor drift, managed-file revalidation) completions_test.rs — shell completion script generation (Phase 7.3) - doctor_test.rs — `numan doctor` report-only (`--scan`), default auto/confirm tiers, journal checks (Phase 7.2) + doctor_test.rs — `numan doctor` default repairs, `--scan` report-only, journal checks (Phase 7.2) nupm_compat_test.rs — Phase 6 integration tests (T13–T25, import/drift/manifest/activation/platform) nupm_real_nu_test.rs — Phase 6.4 real-Nu #[ignore] acceptance tests (run with `cargo test -- --ignored`) plugin_lifecycle_real_nu.rs — Issue #22 smoke marker (points at Stage 1 + active-update suite) diff --git a/Cargo.toml b/Cargo.toml index f984de06..4993d2a6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -83,5 +83,4 @@ libc = "0.2" [dev-dependencies] tempfile = "3" rand_core = { version = "0.6", features = ["getrandom"] } -libc = "0.2" wait-timeout = "0.2" diff --git a/README.md b/README.md index d621efe1..3f6d309a 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ Numan is **early-stage**. Core install, activate, update, remove, gc, registry, - **Module autoloads** — managed vendor autoload files with ownership markers and candidate validation - **Lifecycle management** — `update`, `remove`, and `gc` with pending-lifecycle journal recovery - **nupm interoperability** — `numan nupm status|inspect|import|diff` for migration from [nupm](https://github.com/nushell/nupm) -- **Health checks** — `numan doctor [--fix]` diagnoses root state and applies safe repairs +- **Health checks** — `numan doctor` diagnoses root health and applies safe repairs by default; `--scan` reports only - **Shell completions** — bash, fish, zsh, PowerShell, and Nushell via `numan completions` --- @@ -187,7 +187,7 @@ Or pick a package yourself (`numan search` hides incompatible hits by default; u numan search nutest numan info vyadh/nutest numan install vyadh/nutest -numan activate vyadh/nutest --yes +numan activate vyadh/nutest ``` Install is **inert** — nothing is registered with Nu until you run `numan activate` (or `numan try`, which activates after install). If a package needs a different Nu minor, Numan explains the mismatch and can offer `numan setup nu ` (activations are per-Nu; re-activate after switching). When no compatible starter exists, `numan try` suggests installing a matching managed Nu version or searching for another package with `numan search`. @@ -352,7 +352,7 @@ Global flag: `--root ` — override the Numan root directory (all commands | `nupm status` | `--nupm-home ` | | `nupm inspect` | `--all` scan home; `--nupm-home `; `--exit-on-ineligible` fail on ineligible | | `nupm import` | `--as owner/name` (single import); `--manifest ` (batch); `--nupm-home `; `--yes` skip consent | -| `doctor` | `--scan` report-only; `--json` machine output; `--nupm-home ` | +| `doctor` | `--scan` report only; `--json` machine output; `--nupm-home ` (repairs by default) | | `setup nu` | `--force` re-download; `--skip-path` don't update PATH; `--yes` skip prompt | Run `numan --help` for full flag documentation. @@ -432,7 +432,7 @@ For the cross-repository plan snapshot (as of 2026-07-29) across `numan`, | **5** | `update` / `remove` / `gc`, lockfile v2, [snapshots + rollback](docs/snapshots-and-rollback.md) | ✅ (source builds deferred) | | **6** | [nupm](docs/nupm-compatibility.md) status, inspect, import, drift | ✅ | | **7** | Doctor, completions, onboarding, CI hardening, [winget packaging](docs/PACKAGING.md) | ✅ — [plan](docs/plans/Phase7Plan.md) | -| **Post-7.6** | Production [official registry](https://tonythethompson.github.io/numan-registry/) cutover; `numan init` and `numan doctor --fix` auto-configure `official` | ✅ (v0.1.4) | +| **Post-7.6** | Production [official registry](https://tonythethompson.github.io/numan-registry/) cutover; `numan init` and `numan doctor` auto-configure `official` | ✅ (v0.1.4) | ### Next (toward 1.0) diff --git a/docs/numan-doctor.md b/docs/numan-doctor.md index e0ca87fb..017b1b41 100644 --- a/docs/numan-doctor.md +++ b/docs/numan-doctor.md @@ -7,9 +7,9 @@ `numan doctor` diagnoses the health of a Numan root and applies **safe automated repairs** by default — the same pattern as `brew doctor`, `npm doctor`, and similar tooling. -Use `--scan` for **report-only** mode (safe for CI and scripting). Repair mode delegates to existing commands (`init`, `activate`, `registry sync`) rather than inventing new mutation paths. +Default mode repairs. Use `--scan` for report-only output (safe for CI and scripting). Repairs delegate to existing commands (`init`, `activate`, `registry sync`) rather than inventing new mutation paths. -It answers: *“Is this Numan root consistent, safe to mutate, and aligned with the current Nu environment?”* and optionally *“Fix what you can.”* +It answers: *“Is this Numan root consistent, safe to mutate, and aligned with the current Nu environment?”* and *“Fix what you can.”* ## Non-goals @@ -34,9 +34,10 @@ numan doctor [--scan] [--json] [--nupm-home PATH] Global `--root` applies as for all commands. **Default (no flags):** diagnose and apply available repairs, then print findings. -Confirm-tier repairs run only when stdin is a TTY (allow-gate, not a prompt); -non-interactive sessions skip them as `not_confirmed`. Nested `setup nu` -repairs may still prompt when allowed. +Confirm-tier repairs are applied automatically in default mode (no TTY gate, +no `not_confirmed` outcome). Nested `setup nu use` for +`nu.binary.found_off_path` still requires explicit consent (`--yes` / TTY) +and is never auto-approved by doctor. **`--scan`:** diagnose and print findings without mutating state. ## Exit codes @@ -66,25 +67,34 @@ Each finding has: ## Repair policy -By default, doctor acquires `acquire_mutation_lock(root)` for its own +In default (repair) mode, doctor acquires `acquire_mutation_lock(root)` for its own filesystem repairs (layout dirs, `registry.none` config writes), then **releases** that guard before nested `init` / `setup` / `registry sync` / `activate` / `deactivate` commands. Those nested commands acquire the mutation lock themselves. Doctor does **not** hold one lock for the entire repair pass end-to-end. +A PreMutation snapshot (`SnapshotTrigger::Doctor`) is taken after the lock is +acquired and before layout/config writes. If snapshot creation fails (for +example a malformed lockfile or missing payload revision), doctor records +`snapshot.pre_mutation` as failed, continues with independent `layout.*`, +`nu.active_version.malformed` cleanup, and `registry.none` repairs, and skips +nested mutations that need a PreMutation baseline (`nu_paths.missing` → `init`, +plus `setup` / `registry sync` / `activate` / `deactivate`) with reason +`snapshot_unavailable`. Repair steps run in this **order** (each step re-validates only what it changed): | Tier | Prompt? | Finding IDs | Action | |------|---------|-------------|--------| -| **auto** | Never | `layout.*` (missing dirs), `nu_paths.missing` | `create_dir_all` for layout; `numan init` | -| **auto** | Never | `registry.index_missing` | `numan registry sync` | -| **auto** | Never | `registry.none` (production trust root only) | Add official registry via same path as `numan init` | -| **confirm** | TTY required (non-TTY → `not_confirmed`) | `nu.binary.missing_on_path` | `numan setup nu` (downloads managed Nushell) | -| **confirm** | TTY required (non-TTY → `not_confirmed`) | `nu.binary.found_off_path` | `numan setup nu use ` (adds existing install to PATH) | -| **confirm** | TTY required (non-TTY → `not_confirmed`) | `nu_paths.drift`, `nu_paths.vendor_drift` | `numan init --refresh` | -| **confirm** | TTY required (non-TTY → `not_confirmed`) | `journal.plugin_pending`, `journal.autoload_pending`, `journal.plugin_stale`, `journal.autoload_stale`, `activation.plugin_stale`, `activation.module_stale`, `autoload.projection`, `autoload.managed_missing` | `numan activate` (empty package list — reconciles journals and re-activates stale entries; same entry point as normal activate recovery) | -| **confirm** | TTY required (non-TTY → `not_confirmed`) | `journal.plugin_deactivate_pending` | `numan deactivate ` (reconciles pending-plugin-deactivate journal only; not a full-root deactivate) | -| **confirm** | TTY required (non-TTY → `not_confirmed`) | `journal.plugin_deactivate_stale` | `numan init --refresh` then `numan deactivate` | +| **auto** | Never | `layout.*` (missing dirs), `nu.active_version.malformed` | Independent of PreMutation success: `create_dir_all` for layout; clear invalid `nu_state/active-version.json` via `clear_active_version` | +| **auto** | Never | `nu_paths.missing` | `numan init` (skipped with `snapshot_unavailable` when PreMutation fails) | +| **auto** | Never | `registry.index_missing` | `numan registry sync` (skipped with `snapshot_unavailable` when PreMutation fails) | +| **auto** | Never | `registry.none` (production trust root only) | Add official registry via same path as `numan init` (continues even when PreMutation fails) | +| **manual** | Never auto | `nu.binary.missing_on_path` | Print fix hint (`numan setup nu`); doctor never downloads managed Nu without explicit user opt-in | +| **confirm** | Explicit consent when managed Nu exists | `nu.binary.found_off_path` | `numan setup nu use ` (adds existing install to PATH; doctor never passes `--yes`, so a managed wipe stays fail-closed / interactive) | +| **confirm** | Never (applied in default mode) | `nu_paths.drift`, `nu_paths.vendor_drift` | `numan init --refresh` | +| **confirm** | Never (applied in default mode) | `journal.plugin_pending`, `journal.autoload_pending`, `journal.plugin_stale`, `journal.autoload_stale`, `activation.plugin_stale`, `activation.module_stale`, `autoload.projection`, `autoload.managed_missing` | `numan activate` (empty package list — reconciles journals and re-activates stale entries; same entry point as normal activate recovery) | +| **confirm** | Never (applied in default mode) | `journal.plugin_deactivate_pending` | `numan deactivate ` (reconciles pending-plugin-deactivate journal only; not a full-root deactivate) | +| **confirm** | Never (applied in default mode) | `journal.plugin_deactivate_stale` | `numan init --refresh` then `numan deactivate` | | **manual** | Never auto | `autoload.managed_foreign`, `payload.missing`, `journal.lifecycle_pending`, `journal.lifecycle_stale`, `registry.none` (placeholder trust root), `nu_paths.vendor_missing`, `nupm.*` | Print fix hint only | | **none** | Never | `activation.plugin_mutation_gated` (`info`) | Informational only; see [docs/active-plugin-gate.md](active-plugin-gate.md) | @@ -98,7 +108,7 @@ Repair steps run in this **order** (each step re-validates only what it changed) 6. Mutation lock ownership is **staged**: doctor's lock covers only its direct edits; nested mutators reacquire after doctor drops the guard (see above). -**Journal note:** Repair mode reconciles plugin/autoload journals via `activate` recovery, and plugin-deactivate journals via `deactivate` recovery scoped to journal package IDs — not by editing journal files directly. +**Journal note:** `--scan` only *reports* journals without acting. Default repair mode may reconcile plugin/autoload journals via `activate` recovery, and plugin-deactivate journals via `deactivate` recovery scoped to journal package IDs — not by editing journal files directly. ## Check catalog @@ -118,8 +128,9 @@ Checks run in order below. Implementation should call existing validators (`NuPa |----|----------|-----------| | `nu.binary.missing_on_path` | `error` | Nu not on PATH and not under `$NUMAN_ROOT/tools/nushell/` → fix: `numan setup nu` | | `nu.binary.found_off_path` | `warn` | Nu exists in a known install root (e.g. `~/.cargo/bin`, `%LOCALAPPDATA%\Programs\nushell`) but not on PATH → fix: `numan setup nu use ` | -| `nu.path.version` | `info` | PATH-only Nu version (`PATH Nu: 0.114.1`), `PATH Nu: not found`, or `PATH Nu: found at '' but version probe failed ()` when the binary exists but `--version` fails. Does not treat managed Nu as PATH. Report-only (no repair). | -| `nu.managed.version` | `info` | Managed binary under `$NUMAN_ROOT/tools/nushell/` with version, `Managed Nu: not installed`, or `Managed Nu: present at '' but version probe failed ()` when the binary exists but `--version` fails. Report-only (no repair). | +| `nu.path.version` | `info` | PATH-only Nu version (`PATH Nu: 0.114.1`), `PATH Nu: not found`, or `PATH Nu: found at '' but version probe failed ()` when the binary exists but `--version` fails. Does not treat managed Nu as PATH. Report-only (no automatic repair). | +| `nu.managed.version` | `info` | Managed binary under `$NUMAN_ROOT/tools/nushell/` with version, `Managed Nu: not installed`, or `Managed Nu: present at '' but version probe failed ()` when the binary exists but `--version` fails. Report-only (no automatic repair). | +| `nu.active_version.malformed` | `error` | `nu_state/active-version.json` is present but unreadable/invalid JSON. Lookup would otherwise soft-miss the marker and fall back to PATH. **auto:** clear the marker via `clear_active_version` so resolution recovers cleanly. | | `nu_paths.missing` | `error` | `paths.json` absent → fix: `numan init` | | `nu_paths.drift` | `error` | `NuPaths::validate_drift()` fails → fix: `numan init --refresh` | | `nu_paths.vendor_drift` | `error` | `validate_vendor_drift()` fails when `data_dir` cached → fix: `numan init --refresh` | @@ -165,10 +176,9 @@ No re-hash or revision recompute in v1 (too expensive for doctor). | ID | Severity | Condition | |----|----------|-----------| -| `registry.none` | `warn` | `config.toml` has no registries → fix: `numan init` before first init; `numan doctor` after init (production trust root auto-repair); `numan registry add …` for custom/placeholder builds | -| `registry.index_missing` | `info` | Enabled registry has no cached index under `registries/` → fix: `numan registry sync` | -| `registry.trust_root` | `info` | Enabled `official` registry: reports built-in key id (e.g. `official-2026-07-01`). Placeholder builds note that the key is not production. Report-only (no repair). | - +| `registry.none` | `warn` | `config.toml` has no registries → fix: `numan init` before first init; `numan doctor` after init (production trust root); `numan registry add …` for custom/placeholder builds | +| `registry.index_missing` | `info` | Enabled registry has no cached index under `registry/` → fix: `numan registry sync` | +| `registry.trust_root` | `info` | Enabled `official` registry: reports built-in key id (e.g. `official-2026-07-01`). Placeholder builds note that the key is not production. Report-only (no automatic repair). | ### 7. nupm coexistence (optional section) Controlled by `config.toml` → `[nupm_compat] scan_on_doctor` (default `true`). When `false`, skip section entirely. @@ -205,8 +215,7 @@ nupm coexistence Summary: 1 error, 1 warning -Repairs: 2 applied, 1 skipped -Confirm-tier repairs skipped: stdin is not a TTY. Re-run `numan doctor` interactively, or use `--scan` for report-only. +Repairs: 3 applied, 0 skipped ``` Use `console` styling consistent with `activate --check`. @@ -229,9 +238,13 @@ Use `console` styling consistent with `activate --check`. ], "repairs": [ { "id": "registry.index_missing", "status": "applied" }, - { "id": "nu_paths.drift", "status": "skipped", "reason": "not_confirmed" } + { "id": "nu_paths.drift", "status": "applied" }, + { "id": "nu.binary.missing_on_path", "status": "skipped", "reason": "requires_explicit_setup_nu" } ] } +``` + +`repairs` is present in default repair mode and omitted when `--scan` is set. ## Architecture @@ -253,9 +266,9 @@ pub fn execute_with_options(args: &DoctorArgs, root: &Path, options: DoctorOptio | Command | Role | |---------|------| | `numan init` / `init --refresh` | **Repair** Nu path drift (default doctor delegates here) | -| `numan setup nu` | **Repair** missing Nushell (`nu.binary.missing_on_path`; confirm-tier downloads managed binary) | -| `numan setup nu use ` | **Repair** off-PATH Nushell (`nu.binary.found_off_path`; confirm-tier adds parent dir to user PATH) | -| `numan activate` | **Repair** activation + journal reconciliation (default doctor delegates here) | +| `numan setup nu` | **Manual fix** for missing Nushell (`nu.binary.missing_on_path`; doctor prints the hint and does not download) | +| `numan setup nu use ` | **Repair** off-PATH Nushell (`nu.binary.found_off_path`; adds parent dir to user PATH; consented wipe of managed Nu requires `--yes` / TTY; doctor does not auto-approve) | +| `numan activate` | **Repair** activation + journal reconciliation | | `numan registry sync` | **Repair** missing index cache (auto tier) | | `numan activate --check` | Deep **module** check only; no repair | | `numan nupm status` | nupm-only summary; doctor embeds optional subset | @@ -266,15 +279,14 @@ pub fn execute_with_options(args: &DoctorArgs, root: &Path, options: DoctorOptio - [x] `numan doctor`, `numan doctor --scan`, and `numan doctor --json` implemented per check catalog - [x] `scan_on_doctor` respected - [x] `--scan` mode: no state mutation (test: hashes unchanged) -- [x] Default mode: applies repair tiers per policy; uses mutation lock; delegates to init/activate/sync +- [x] Default repair mode: only repair tiers in policy; uses mutation lock; PreMutation Doctor snapshot; delegates to init/activate/sync - [x] Documented in README command table and `AGENTS.md` -- [x] Integration tests: `--scan` report-only, default auto tier, default confirm tier with TTY, manual tier untouched +- [x] Integration tests: `--scan` report-only, default auto repairs, confirm-tier applied by default, manual tier untouched ## Changelog | Date | Change | |------|--------| | 2026-06-30 | Initial spec (Phase 7.2) | -| 2026-06-30 | Add repair policy (auto / confirm / manual tiers) | -| 2026-08-02 | Update to reflect `--scan` flag (repairs by default; `--scan` for report-only) | -| 2026-08-03 | Document confirm-tier TTY allow-gate and plain-text `not_confirmed` remediation | +| 2026-06-30 | Add `--fix` / `--yes` repair policy (auto / confirm / manual tiers) | +| 2026-08-02 | Invert defaults: repair by default; `--scan` for report-only; remove `--fix` / `--yes` | \ No newline at end of file diff --git a/src/cmd/doctor.rs b/src/cmd/doctor.rs index 667c9a45..13f2eb09 100644 --- a/src/cmd/doctor.rs +++ b/src/cmd/doctor.rs @@ -2,7 +2,7 @@ use anyhow::{Context, Result}; use clap::Args; use console::style; use serde::Serialize; -use std::io::{IsTerminal, Write}; +use std::io::Write; use std::path::{Path, PathBuf}; use crate::cmd::activate::{execute as activate_execute, ActivateArgs}; @@ -18,6 +18,7 @@ use crate::nu::bootstrap::managed_nu_binary; use crate::nu::paths::{ discover_nu_off_path, find_nu_executable_with_root, find_nu_on_path, NuPaths, }; +use crate::nu::version_manager; use crate::nupm_compat::NupmCompatibility; use crate::nupm_compat::{ count_drifted_imports, resolve_nupm_home, scan_nupm_home, NupmHomeResolution, @@ -33,8 +34,8 @@ use crate::state::snapshot::{create_snapshot, SnapshotReason, SnapshotTrigger}; use crate::util::fs_safety::{acquire_mutation_lock, assert_managed_file_owned}; use crate::util::hints::{ self, active_plugin_mutation_gated_doctor_message, registry_none_fix, setup_nu_use_existing, - ACTIVE_PLUGIN_MUTATION_GATED_FIX, CMD_ACTIVATE, CMD_DEACTIVATE, CMD_INIT, CMD_INIT_REFRESH, - CMD_REGISTRY_SYNC, CMD_SETUP_NU, + ACTIVE_PLUGIN_MUTATION_GATED_FIX, CMD_ACTIVATE, CMD_DEACTIVATE, CMD_DOCTOR_FIX, CMD_INIT, + CMD_INIT_REFRESH, CMD_REGISTRY_SYNC, CMD_SETUP_NU, }; use crate::util::stdio_redirect::StdoutToStderr; @@ -132,10 +133,6 @@ pub struct DoctorOptions { pub discover_off_path: Option Option>, /// Override Nu `--version` probing (tests inject a fixed version string). pub nu_version_probe: Option Result>, - /// Override confirm-tier allow-gate (tests inject `|_| true` to exercise - /// repairs under non-TTY CI; production uses [`confirm_repairs`], which is - /// TTY detection only and does not prompt). - pub confirm_repairs: Option bool>, } pub fn execute(args: &DoctorArgs, root: &Path) -> Result { @@ -197,6 +194,7 @@ pub fn run_checks_with_options( let mut findings = Vec::new(); check_root_layout(root, &mut findings); + check_active_version_marker(root, &mut findings); let nu_paths = check_nu_paths(root, options, &mut findings); check_nu_environments(root, options, &mut findings); check_journals(root, nu_paths.as_ref(), &mut findings); @@ -323,6 +321,37 @@ fn nu_is_available(root: &Path) -> bool { false } +/// Detect a present-but-unreadable `nu_state/active-version.json`. +/// +/// `find_nu_executable_with_root` treats marker read errors as soft misses and +/// falls through to PATH. Doctor surfaces the broken marker so repair can clear +/// it instead of leaving resolution silently degraded. +fn check_active_version_marker(root: &Path, findings: &mut Vec) { + match version_manager::read_active_version(root) { + Ok(None) => findings.push(finding( + "nu.active_version.malformed", + Severity::Ok, + "No active-version marker", + None, + RepairTier::None, + )), + Ok(Some(active)) => findings.push(finding( + "nu.active_version.malformed", + Severity::Ok, + format!("Active Nu version marker: {}", active.version), + None, + RepairTier::None, + )), + Err(e) => findings.push(finding( + "nu.active_version.malformed", + Severity::Error, + e.to_string(), + Some(CMD_DOCTOR_FIX), + RepairTier::Auto, + )), + } +} + fn check_nu_paths( root: &Path, options: &DoctorOptions, @@ -352,7 +381,7 @@ fn check_nu_paths( Severity::Error, "Nu not found on PATH or in the Numan tools directory.", Some(CMD_SETUP_NU), - RepairTier::Confirm, + RepairTier::Manual, )); } } else { @@ -987,13 +1016,6 @@ fn count_nupm_name_overlap( Ok(count) } -fn confirm_repairs(_args: &DoctorArgs) -> bool { - // Confirm-tier allow-gate: TTY detection only (no prompt here). - // Nested setup repairs may still prompt when allowed. Non-TTY sessions - // skip confirm-tier repairs as `not_confirmed`. - std::io::stdin().is_terminal() -} - fn apply_repairs( args: &DoctorArgs, root: &Path, @@ -1021,23 +1043,32 @@ fn apply_repairs( None }; + 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 { - create_snapshot( + if let Err(e) = create_snapshot( root, SnapshotReason::PreMutation, SnapshotTrigger::Doctor, None, None, ) - .context("Failed to create doctor pre-mutation snapshot")?; + .context("Failed to create doctor pre-mutation snapshot") + { + snapshot_ok = false; + records.push(RepairRecord { + id: "snapshot.pre_mutation".to_string(), + status: RepairStatus::Failed, + reason: Some(format!("{e:#}")), + }); + eprintln!( + "warning: doctor PreMutation snapshot failed; applying independent layout/config repairs only: {e:#}" + ); + } } - let mut records = Vec::new(); - let confirm = match options.confirm_repairs { - Some(gate) => gate(args), - None => confirm_repairs(args), - }; - for dir in LAYOUT_DIRS { let id = format!("layout.{dir}"); if findings @@ -1059,6 +1090,30 @@ fn apply_repairs( } } + if findings + .iter() + .any(|f| f.id == "nu.active_version.malformed" && f.severity == Severity::Error) + { + let id = "nu.active_version.malformed".to_string(); + match version_manager::clear_active_version(root) { + Ok(true) => records.push(RepairRecord { + id, + status: RepairStatus::Applied, + reason: None, + }), + Ok(false) => records.push(RepairRecord { + id, + status: RepairStatus::Skipped, + reason: Some("marker_already_absent".to_string()), + }), + Err(e) => records.push(RepairRecord { + id, + status: RepairStatus::Failed, + reason: Some(e.to_string()), + }), + } + } + // Config write does not reacquire the mutation lock; keep it under doctor's // lock. Nested mutators below (setup / init / registry sync / refresh / // activate / deactivate) do acquire again, so release first. @@ -1100,16 +1155,16 @@ fn apply_repairs( .any(|f| f.id == "nu.binary.found_off_path" && f.severity == Severity::Warn) { let id = "nu.binary.found_off_path".to_string(); - if !confirm { + if !snapshot_ok { records.push(RepairRecord { id, status: RepairStatus::Skipped, - reason: Some("not_confirmed".to_string()), + 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); - // Pass yes: false to let setup code handle TTY checking and confirmation. - // If there's a managed install, setup will prompt the user before removal. + // 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), root) { Ok(()) => records.push(RepairRecord { id, @@ -1135,23 +1190,31 @@ fn apply_repairs( .iter() .any(|f| f.id == "nu.binary.missing_on_path" && f.severity == Severity::Error) { + // Never auto-download managed Nu from doctor. Print the existing fix + // hint; the user opts in explicitly via `numan setup nu`. let id = "nu.binary.missing_on_path".to_string(); - if !confirm { - records.push(RepairRecord { - id, - status: RepairStatus::Skipped, - reason: Some("not_confirmed".to_string()), - }); - } else if options.skip_network { + eprintln!(" → Fix: {CMD_SETUP_NU}"); + records.push(RepairRecord { + id, + status: RepairStatus::Skipped, + reason: Some("requires_explicit_setup_nu".to_string()), + }); + } + + if findings + .iter() + .any(|f| f.id == "nu_paths.missing" && f.severity == Severity::Error) + { + let id = "nu_paths.missing".to_string(); + if !snapshot_ok { records.push(RepairRecord { id, status: RepairStatus::Skipped, - reason: Some("skip_network".to_string()), + reason: Some("snapshot_unavailable".to_string()), }); } else { - let setup_fn = options.nu_setup_repair.unwrap_or(setup::execute_nu_repair); - // Pass yes: false to let setup code handle TTY checking and confirmation. - match setup_fn(&NuSetupArgs::install(None, false, false, false), root) { + let init_fn = options.init_repair.unwrap_or(init_execute); + match init_fn(&InitArgs { refresh: false }, root) { Ok(()) => records.push(RepairRecord { id, status: RepairStatus::Applied, @@ -1166,41 +1229,29 @@ fn apply_repairs( } } - if findings - .iter() - .any(|f| f.id == "nu_paths.missing" && f.severity == Severity::Error) - { - let id = "nu_paths.missing".to_string(); - let init_fn = options.init_repair.unwrap_or(init_execute); - match init_fn(&InitArgs { refresh: 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()), - }), - } - } - if findings.iter().any(|f| { f.id == "registry.index_missing" && f.severity == Severity::Info && !options.skip_network }) { let id = "registry.index_missing".to_string(); - match registry::execute(RegistryCommands::Sync, root) { - Ok(()) => records.push(RepairRecord { - id, - status: RepairStatus::Applied, - reason: None, - }), - Err(e) => records.push(RepairRecord { + if !snapshot_ok { + records.push(RepairRecord { id, - status: RepairStatus::Failed, - reason: Some(e.to_string()), - }), + status: RepairStatus::Skipped, + reason: Some("snapshot_unavailable".to_string()), + }); + } else { + match registry::execute(RegistryCommands::Sync, 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 if findings.iter().any(|f| f.id == "registry.index_missing") && options.skip_network { records.push(RepairRecord { @@ -1222,11 +1273,11 @@ fn apply_repairs( if needs_refresh { let id = "nu_paths.refresh".to_string(); - if !confirm { + if !snapshot_ok { records.push(RepairRecord { id, status: RepairStatus::Skipped, - reason: Some("not_confirmed".to_string()), + reason: Some("snapshot_unavailable".to_string()), }); } else { let init_fn = options.init_repair.unwrap_or(init_execute); @@ -1261,11 +1312,11 @@ fn apply_repairs( if needs_activate { let id = "activation.reconcile".to_string(); - if !confirm { + if !snapshot_ok { records.push(RepairRecord { id, status: RepairStatus::Skipped, - reason: Some("not_confirmed".to_string()), + reason: Some("snapshot_unavailable".to_string()), }); } else { let activate_args = ActivateArgs { @@ -1301,46 +1352,56 @@ fn apply_repairs( if needs_deactivate { let id = "plugin_deactivate.reconcile".to_string(); - if !confirm { + if !snapshot_ok { records.push(RepairRecord { id, status: RepairStatus::Skipped, - reason: Some("not_confirmed".to_string()), + reason: Some("snapshot_unavailable".to_string()), }); } else { - let journal_packages = PendingPluginDeactivate::load(root)? - .map(|journal| { - journal - .entries - .iter() - .map(|entry| entry.package_id.clone()) - .collect::>() - }) - .unwrap_or_default(); - if journal_packages.is_empty() { - records.push(RepairRecord { - id, - status: RepairStatus::Skipped, - reason: Some("no_pending_plugin_deactivate_journal".to_string()), - }); - } else { - let deactivate_args = DeactivateArgs { - packages: journal_packages, - - verbose: false, - }; - let deactivate_fn = options.deactivate_repair.unwrap_or(deactivate_execute); - match deactivate_fn(&deactivate_args, root) { - Ok(()) => records.push(RepairRecord { - id, - status: RepairStatus::Applied, - reason: None, - }), - Err(e) => records.push(RepairRecord { + match PendingPluginDeactivate::load(root) { + Err(e) => { + records.push(RepairRecord { id, status: RepairStatus::Failed, reason: Some(e.to_string()), - }), + }); + } + Ok(journal) => { + let journal_packages = journal + .map(|journal| { + journal + .entries + .iter() + .map(|entry| entry.package_id.clone()) + .collect::>() + }) + .unwrap_or_default(); + if journal_packages.is_empty() { + records.push(RepairRecord { + id, + status: RepairStatus::Skipped, + reason: Some("no_pending_plugin_deactivate_journal".to_string()), + }); + } else { + let deactivate_args = DeactivateArgs { + packages: journal_packages, + verbose: false, + }; + let deactivate_fn = options.deactivate_repair.unwrap_or(deactivate_execute); + match deactivate_fn(&deactivate_args, 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()), + }), + } + } } } } @@ -1378,6 +1439,7 @@ fn print_report(args: &DoctorArgs, root: &Path, report: &DoctorReport) -> Result "nu.binary.found_off_path", "nu.path.version", "nu.managed.version", + "nu.active_version.malformed", "nu_paths.missing", "nu_paths.drift", "nu_paths.vendor_drift", @@ -1466,15 +1528,6 @@ fn print_report(args: &DoctorArgs, root: &Path, report: &DoctorReport) -> Result if !repairs.is_empty() { writeln!(out)?; writeln!(out, "Repairs: {applied} applied, {skipped} skipped")?; - let not_confirmed = repairs.iter().any(|r| { - r.status == RepairStatus::Skipped && r.reason.as_deref() == Some("not_confirmed") - }); - if not_confirmed { - writeln!( - out, - "Confirm-tier repairs skipped: stdin is not a TTY. Re-run `numan doctor` interactively, or use `--scan` for report-only." - )?; - } } } @@ -1509,13 +1562,6 @@ mod tests { /// Serializes tests that mutate the process-wide `PATH` env var. static TEST_PATH_GUARD: Mutex<()> = Mutex::new(()); - /// Off-PATH discovery seam for unit tests (`discover_off_path` is an fn pointer). - static TEST_OFF_PATH: Mutex> = Mutex::new(None); - - fn discover_off_path_test() -> Option { - TEST_OFF_PATH.lock().ok()?.clone() - } - /// RAII guard that restores the original PATH on drop, ensuring restoration /// during both normal return and panic unwinding. struct PathRestoreGuard { @@ -1631,7 +1677,6 @@ mod tests { nu_setup_repair: None, discover_off_path: None, nu_version_probe: Some(probe_fixed_version), - confirm_repairs: Some(confirm_repairs_always), }, ) .unwrap(); @@ -1675,7 +1720,7 @@ mod tests { assert_eq!(none.repair, RepairTier::Manual); return; } - assert_eq!(none.fix.as_deref(), Some(hints::CMD_DOCTOR)); + assert_eq!(none.fix.as_deref(), Some(hints::CMD_DOCTOR_FIX)); assert_eq!(none.repair, RepairTier::Auto); execute_with_options( @@ -1689,7 +1734,6 @@ mod tests { nu_setup_repair: None, discover_off_path: None, nu_version_probe: Some(probe_fixed_version), - confirm_repairs: Some(confirm_repairs_always), }, ) .unwrap(); @@ -1979,21 +2023,11 @@ mod tests { anyhow::bail!("simulated version probe failure") } - fn confirm_repairs_always(_args: &DoctorArgs) -> bool { - true - } - - fn confirm_repairs_never(_args: &DoctorArgs) -> bool { - false - } - /// Skip network and never exec a real `nu` during doctor unit tests. - /// Force confirm-tier repairs so non-TTY unit tests can exercise them. fn test_doctor_options() -> DoctorOptions { DoctorOptions { skip_network: true, nu_version_probe: Some(probe_fixed_version), - confirm_repairs: Some(confirm_repairs_always), ..DoctorOptions::default() } } @@ -2248,104 +2282,4 @@ mod tests { assert!(json.contains("nu.managed.version")); assert!(json.contains("registry.trust_root")); } - - use std::sync::atomic::{AtomicUsize, Ordering}; - - static SETUP_REPAIR_CALLS: AtomicUsize = AtomicUsize::new(0); - - fn counting_setup_repair(_args: &NuSetupArgs, _root: &Path) -> Result<()> { - SETUP_REPAIR_CALLS.fetch_add(1, Ordering::SeqCst); - Ok(()) - } - - #[test] - fn doctor_found_off_path_repair_preserves_managed_install_when_not_confirmed() { - // Regression: when confirm-tier is closed, found_off_path must be Skipped - // without dispatching setup (which could wipe a managed Nu tree). - let _path_guard = TEST_PATH_GUARD.lock().unwrap(); - let dir = TempDir::new().unwrap(); - let root = dir.path(); - - // Clear PATH so managed/off-PATH discovery controls availability. - let _path_restore = PathRestoreGuard::new(); - std::env::set_var("PATH", ""); - - let off_path_dir = root.join("external_nu"); - std::fs::create_dir_all(&off_path_dir).unwrap(); - let off_path_binary = off_path_dir.join("nu"); - std::fs::write(&off_path_binary, b"external nu").unwrap(); - *TEST_OFF_PATH.lock().unwrap() = Some(off_path_binary); - - let args = DoctorArgs { - scan: false, - json: false, - nupm_home: None, - }; - let options = DoctorOptions { - skip_network: true, - nu_version_probe: Some(probe_fixed_version), - discover_off_path: Some(discover_off_path_test), - nu_setup_repair: Some(counting_setup_repair), - confirm_repairs: Some(confirm_repairs_never), - // Avoid init creating a managed Nu mid-pass before we seed one. - init_repair: Some(|_, _| Ok(())), - ..DoctorOptions::default() - }; - - SETUP_REPAIR_CALLS.store(0, Ordering::SeqCst); - // No managed binary yet: otherwise nu_is_available suppresses found_off_path. - let report = run_checks_with_options(&args, root, &options).expect("doctor checks"); - assert!( - report - .findings - .iter() - .any(|f| f.id == "nu.binary.found_off_path" && f.severity == Severity::Warn), - "expected found_off_path finding before repair" - ); - - // Seed managed Nu after findings so a mistaken setup dispatch would wipe it. - let managed_binary = ensure_fake_managed_nu(root); - assert!(managed_binary.exists(), "managed binary should exist"); - - let repairs = - apply_repairs(&args, root, &report.findings, &options).expect("apply_repairs"); - let off_path_repair = repairs - .iter() - .find(|r| r.id == "nu.binary.found_off_path") - .expect("nu.binary.found_off_path repair record"); - assert_eq!(off_path_repair.status, RepairStatus::Skipped); - assert_eq!( - off_path_repair.reason.as_deref(), - Some("not_confirmed"), - "confirm gate closed must record not_confirmed" - ); - assert_eq!( - SETUP_REPAIR_CALLS.load(Ordering::SeqCst), - 0, - "setup repair must not be dispatched when confirm-tier is skipped" - ); - - let options = DoctorOptions { - skip_network: true, - nu_version_probe: Some(probe_fixed_version), - discover_off_path: Some(discover_off_path_test), - nu_setup_repair: Some(counting_setup_repair), - confirm_repairs: Some(confirm_repairs_never), - init_repair: Some(|_, _| Ok(())), - ..DoctorOptions::default() - }; - execute_with_options(&args, root, options) - .expect("execute_with_options must succeed when confirm-tier is skipped"); - assert_eq!( - SETUP_REPAIR_CALLS.load(Ordering::SeqCst), - 0, - "execute_with_options must not dispatch setup when confirm-tier is skipped" - ); - - *TEST_OFF_PATH.lock().unwrap() = None; - assert!( - managed_binary.exists(), - "managed installation should be preserved when confirm-tier repair is skipped" - ); - } } diff --git a/src/cmd/init.rs b/src/cmd/init.rs index 56bfeb57..7150fd13 100644 --- a/src/cmd/init.rs +++ b/src/cmd/init.rs @@ -165,15 +165,10 @@ fn execute_refresh( where F: Fn() -> Result, { - let old_paths = NuPaths::load(root)?; + // Probe live Nu outside the lock; reload mutable Numan state only after + // the mutation lock is held (same reload-after-lock pattern as activate). let new_paths = detect()?; - let lockfile = Lockfile::load(root)?; - let has_active_modules = lockfile - .packages - .values() - .any(|entry| entry.module_activation.is_some()); - // Always serialize refresh writes (lockfile, autoload-state, paths), even // when no packages are active. Skipping the lock left a race window against // concurrent mutators. @@ -187,6 +182,13 @@ where ) .context("Failed to create pre-mutation snapshot for `numan init --refresh`")?; + let old_paths = NuPaths::load(root)?; + let mut lockfile = Lockfile::load(root)?; + let has_active_modules = lockfile + .packages + .values() + .any(|entry| entry.module_activation.is_some()); + if has_active_modules { validate_refresh_for_active_modules( root, @@ -196,8 +198,6 @@ where runner_factory, )?; } - - let mut lockfile = lockfile; refresh_activation_records(&mut lockfile, &new_paths)?; lockfile.nu_version = new_paths.nu_version.clone(); lockfile.platform = new_paths.platform.clone(); @@ -484,7 +484,8 @@ mod tests { #[test] fn refresh_without_active_packages_still_locks_and_snapshots() { - use crate::state::snapshot::list_snapshots; + use crate::state::snapshot::{list_snapshots, SnapshotReason}; + use crate::util::fs_safety::acquire_mutation_lock; let dir = TempDir::new().unwrap(); let root = dir.path(); @@ -510,8 +511,66 @@ mod tests { assert_eq!(loaded.nu_executable_hash, paths_v2.nu_executable_hash); let snapshots = list_snapshots(root).unwrap(); assert!( - snapshots.iter().any(|s| s.trigger == SnapshotTrigger::Init), + snapshots.iter().any(|s| { + s.trigger == SnapshotTrigger::Init && s.reason == SnapshotReason::PreMutation + }), "init --refresh must create a PreMutation Init snapshot even with no active packages" ); + + // Holding the mutation lock must fail-closed a concurrent refresh. + let _held = acquire_mutation_lock(root).unwrap(); + let err = execute_with_runner( + &InitArgs { refresh: true }, + root, + make_detect(paths_v2), + fake_runner_factory, + ) + .unwrap_err(); + assert!( + err.to_string().contains("mutation is already in progress"), + "expected lock contention error, got: {err:#}" + ); + } + + #[test] + fn refresh_snapshot_failure_does_not_write_paths() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + let nu_v1 = root.join("nu_v1"); + let nu_v2 = root.join("nu_v2"); + std::fs::write(&nu_v1, b"v1").unwrap(); + std::fs::write(&nu_v2, b"v2").unwrap(); + + let paths_v1 = fake_paths(root, &nu_v1, None); + paths_v1.save(root).unwrap(); + + // Lockfile references a missing payload so create_snapshot fails. + let mut lockfile = Lockfile::empty(); + lockfile.packages.insert( + "owner/pkg".to_string(), + plugin_entry("packages/plugins/owner/pkg/1.0.0-deadbeef", None), + ); + lockfile.save(root).unwrap(); + + let paths_v2 = fake_paths(root, &nu_v2, None); + let err = execute_with_runner( + &InitArgs { refresh: true }, + root, + make_detect(paths_v2.clone()), + fake_runner_factory, + ) + .unwrap_err(); + assert!( + err.to_string() + .contains("Failed to create pre-mutation snapshot"), + "expected snapshot failure, got: {err:#}" + ); + + let loaded = NuPaths::load(root).unwrap(); + assert_eq!( + loaded.nu_executable_hash, paths_v1.nu_executable_hash, + "paths.json must stay unchanged when the PreMutation snapshot fails" + ); + assert_ne!(loaded.nu_executable_hash, paths_v2.nu_executable_hash); } } diff --git a/src/cmd/nu_pin_offer.rs b/src/cmd/nu_pin_offer.rs index 91cceda7..536c529b 100644 --- a/src/cmd/nu_pin_offer.rs +++ b/src/cmd/nu_pin_offer.rs @@ -19,16 +19,35 @@ pub fn offer_managed_nu_pin( current_nu: &str, diagnosis: &PackageIncompatibility, ) -> Result { - offer_managed_nu_pin_with_tty(root, current_nu, diagnosis, None) + offer_managed_nu_pin_with_interaction( + root, + current_nu, + diagnosis, + std::io::stdin().is_terminal(), + || { + let mut input = String::new(); + std::io::stdin() + .read_line(&mut input) + .context("Failed to read Nu pin confirmation from stdin")?; + Ok(input) + }, + ) } -/// Same as [`offer_managed_nu_pin`] but allows injecting TTY state for testing. -pub fn offer_managed_nu_pin_with_tty( +/// Testable offer path with explicit terminal/interaction state. +/// +/// When `interactive` is false, prints setup hints and returns `Ok(false)` without +/// installing managed Nu (never auto-downloads). +pub fn offer_managed_nu_pin_with_interaction( root: &Path, current_nu: &str, diagnosis: &PackageIncompatibility, - is_tty: Option, -) -> Result { + interactive: bool, + read_line: F, +) -> Result +where + F: FnOnce() -> Result, +{ let Some(pin) = diagnosis.suggested_pin.as_deref() else { return Ok(false); }; @@ -45,8 +64,7 @@ pub fn offer_managed_nu_pin_with_tty( let setup_cmd = hints::setup_nu_version(pin); - let is_terminal = is_tty.unwrap_or_else(|| std::io::stdin().is_terminal()); - if !is_terminal { + if !interactive { println!("To switch Nu, run:"); println!(" {setup_cmd} --yes --force"); println!(" {CMD_INIT_REFRESH}"); @@ -56,8 +74,7 @@ pub fn offer_managed_nu_pin_with_tty( print!("Install managed Nu {pin} via `{setup_cmd}`? [y/N] "); std::io::stdout().flush()?; - let mut input = String::new(); - std::io::stdin().read_line(&mut input)?; + let input = read_line().context("Failed to read Nu pin confirmation input")?; if !input.trim().eq_ignore_ascii_case("y") { println!("Skipped Nu switch."); return Ok(false); diff --git a/src/cmd/setup.rs b/src/cmd/setup.rs index fe0a98aa..c6318535 100644 --- a/src/cmd/setup.rs +++ b/src/cmd/setup.rs @@ -8,11 +8,19 @@ use crate::nu::bootstrap::{self, NuSetupOptions}; use crate::nu::paths::{ find_nu_executable_with_root, find_nu_on_path, probe_nu_config_path, validate_nushell_binary, }; +use crate::state::snapshot::{create_snapshot, SnapshotReason, SnapshotTrigger}; use crate::util::atomic::write_bytes_atomic; use crate::util::fs_safety::{ acquire_mutation_lock, assert_managed_file_owned, assert_not_symlink, }; +/// Snapshot established Numan state before a `setup nu` mutation. +fn snapshot_before_setup_mutation(root: &Path, trigger: SnapshotTrigger) -> Result<()> { + create_snapshot(root, SnapshotReason::PreMutation, trigger, None, None) + .context("Failed to create pre-mutation snapshot for `numan setup nu`")?; + Ok(()) +} + const VENDOR_LOADER: &str = include_str!("../../assets/nushell-loader/loader.nu"); const CONFIG_SOURCE_LINE: &str = "source ($nu.config-path | path dirname | path join 'loader.nu')"; @@ -355,6 +363,7 @@ fn execute_use_path(yes: bool, root: &Path, opts: ExecuteUseOpts<'_>) -> Result< } } + snapshot_before_setup_mutation(root, SnapshotTrigger::Update)?; remove_managed_nu_if_present(root)?; let options = NuSetupOptions { yes, @@ -416,6 +425,7 @@ fn execute_use_existing( } } + snapshot_before_setup_mutation(root, SnapshotTrigger::Update)?; remove_managed_nu_if_present(root)?; let options = NuSetupOptions { yes, @@ -451,6 +461,8 @@ fn remove_managed_nu(root: &Path, yes: bool) -> Result<()> { "Managed Nushell removal cancelled.", )?; + snapshot_before_setup_mutation(root, SnapshotTrigger::Remove)?; + // Clear the active-version marker before removing the versioned tree so // it cannot dangle at a now-missing binary (see `clear_active_version`). crate::nu::version_manager::clear_active_version(root)?; @@ -832,6 +844,28 @@ mod tests { assert!(!managed_dir.exists()); } + #[test] + fn execute_use_existing_invalid_binary_preserves_managed_installation() { + let dir = TempDir::new().unwrap(); + let root = dir.path().join("numan-root"); + let managed_dir = bootstrap::managed_nu_dir(&root); + std::fs::create_dir_all(&managed_dir).unwrap(); + let marker = managed_dir.join("keep-me"); + std::fs::write(&marker, b"managed").unwrap(); + + let missing = dir.path().join("no-such-nu"); + let err = + execute_use_existing(&missing, true, &root, ExecuteUseOpts::default()).unwrap_err(); + assert!( + err.to_string().contains("Failed to resolve") || err.to_string().contains("no-such-nu"), + "expected resolve failure, got: {err}" + ); + assert!( + marker.exists(), + "managed Nu must remain intact when off-PATH binary fails validation" + ); + } + #[test] fn remove_managed_nu_if_present_noop_when_absent() { let dir = TempDir::new().unwrap(); diff --git a/src/cmd/try_cmd.rs b/src/cmd/try_cmd.rs index d0984113..2d943ea5 100644 --- a/src/cmd/try_cmd.rs +++ b/src/cmd/try_cmd.rs @@ -290,10 +290,25 @@ fn detect_nu(root: &Path) -> Result { #[cfg(test)] mod tests { use super::*; + use crate::cli::{Cli, Commands}; use crate::core::package::*; use crate::core::resolve::{Incompatibility, PackageIncompatibility}; + use clap::Parser; use std::collections::{BTreeMap, HashMap}; + #[test] + fn try_cli_rejects_obsolete_yes_flag() { + assert!( + Cli::try_parse_from(["numan", "try", "--yes"]).is_err(), + "numan try --yes must be rejected" + ); + let cli = Cli::try_parse_from(["numan", "try", "--no-activate"]).unwrap(); + match cli.command { + Commands::Try(args) => assert!(args.no_activate), + _ => panic!("expected Try command"), + } + } + fn pkg(id: &str, constraint: &str, plugin: bool) -> Package { let (owner, name) = id.split_once('/').unwrap(); let mut targets = HashMap::new(); @@ -442,12 +457,12 @@ mod tests { available_versions: vec!["1.0.0".to_string()], }; let root = tempfile::tempdir().unwrap(); - // Explicitly pass is_tty: false to avoid depending on process-global stdin state. - let accepted = nu_pin_offer::offer_managed_nu_pin_with_tty( + let accepted = nu_pin_offer::offer_managed_nu_pin_with_interaction( root.path(), "0.114.1", &diagnosis, - Some(false), + false, + || panic!("non-interactive path must not read stdin"), ) .unwrap(); assert!( diff --git a/src/nu/bootstrap.rs b/src/nu/bootstrap.rs index 377f4ec3..b35260b8 100644 --- a/src/nu/bootstrap.rs +++ b/src/nu/bootstrap.rs @@ -21,6 +21,19 @@ const RELEASES_LATEST: &str = "https://api.github.com/repos/nushell/nushell/rele const RELEASES_TAGS_BASE: &str = "https://api.github.com/repos/nushell/nushell/releases/tags/"; const USER_AGENT: &str = "numan-cli (https://github.com/tonythethompson/numan)"; +/// Snapshot established Numan state before a `setup nu` install/PATH mutation. +fn snapshot_before_nu_setup(root: &Path, context: &'static str) -> Result<()> { + create_snapshot( + root, + SnapshotReason::PreMutation, + SnapshotTrigger::Install, + None, + None, + ) + .context(context)?; + Ok(()) +} + #[derive(Debug, Deserialize)] struct GitHubRelease { tag_name: String, @@ -717,6 +730,11 @@ where if let Some(dest) = already_installed.as_ref() { if !options.force { if options.yes { + // PATH persistence mutates user shell state; snapshot first. + snapshot_before_nu_setup( + root, + "Failed to create pre-mutation snapshot for existing `numan setup nu`", + )?; if let Some(parent) = dest.parent() { prepend_process_path(parent)?; } @@ -756,14 +774,10 @@ where // Snapshot established state right before the download/install mutates the // filesystem. Same lifecycle boundary used by install/update/remove/activate // per AGENTS.md. - create_snapshot( + snapshot_before_nu_setup( root, - SnapshotReason::PreMutation, - SnapshotTrigger::Install, - None, - None, - ) - .with_context(|| "Failed to create pre-install snapshot for `numan setup nu`")?; + "Failed to create pre-install snapshot for `numan setup nu`", + )?; let installed = install(root, platform)?; diff --git a/src/util/hints.rs b/src/util/hints.rs index 1f4c2371..00220dd1 100644 --- a/src/util/hints.rs +++ b/src/util/hints.rs @@ -15,11 +15,8 @@ pub const CMD_ACTIVATE_CHECK: &str = "numan activate --check"; /// `numan registry sync` pub const CMD_REGISTRY_SYNC: &str = "numan registry sync"; -/// `numan doctor` (default mode applies safe repairs; use `--scan` for report-only) -pub const CMD_DOCTOR: &str = "numan doctor"; - -/// Historical alias for [`CMD_DOCTOR`] (named when doctor repairs required `--fix`). -pub const CMD_DOCTOR_FIX: &str = CMD_DOCTOR; +/// `numan doctor` +pub const CMD_DOCTOR_FIX: &str = "numan doctor"; /// `numan setup nu` pub const CMD_SETUP_NU: &str = "numan setup nu"; @@ -50,6 +47,7 @@ pub fn shell_quote(s: &str) -> String { '\'' | '"' | '`' | '$' + | '\\' | '&' | '|' | ';' @@ -124,7 +122,7 @@ pub fn registry_none_fix(root: &std::path::Path) -> &'static str { if OFFICIAL_REGISTRY.is_placeholder_key() { CMD_REGISTRY_ADD } else if root.join("nu_state/paths.json").exists() { - CMD_DOCTOR + CMD_DOCTOR_FIX } else { CMD_INIT } @@ -227,6 +225,11 @@ mod tests { assert_eq!(shell_quote("it's"), "'it'\\''s'"); } + #[test] + fn shell_quote_wraps_paths_with_backslashes() { + assert_eq!(shell_quote(r"/tmp/nu\bin"), r"'/tmp/nu\bin'"); + } + #[test] fn active_plugin_mutation_gated_mentions_package_and_issue() { let hint = active_plugin_mutation_gated("owner/plugin"); @@ -302,6 +305,6 @@ mod tests { let dir = tempfile::tempdir().unwrap(); std::fs::create_dir_all(dir.path().join("nu_state")).unwrap(); std::fs::write(dir.path().join("nu_state/paths.json"), b"{}").unwrap(); - assert_eq!(registry_none_fix(dir.path()), CMD_DOCTOR); + assert_eq!(registry_none_fix(dir.path()), CMD_DOCTOR_FIX); } } diff --git a/tests/doctor_test.rs b/tests/doctor_test.rs index 67806f8b..dfe0d011 100644 --- a/tests/doctor_test.rs +++ b/tests/doctor_test.rs @@ -5,6 +5,7 @@ use numan_cli::cmd::doctor::{ execute_with_options, run_checks_with_options, DoctorArgs, DoctorOptions, Severity, }; use numan_cli::cmd::init::{execute_with_runner, InitArgs}; +use numan_cli::cmd::setup::{self, NuAction}; use numan_cli::core::integrity; use numan_cli::nu::autoload::FakeCandidateRunner; use numan_cli::nu::bootstrap::managed_nu_binary; @@ -13,6 +14,7 @@ use numan_cli::state::journal::{PendingActivation, PendingActivationEntry, Pendi use numan_cli::state::plugin_deactivate_journal::{ PendingPluginDeactivate, PendingPluginDeactivateEntry, PluginDeactivateStatus, }; +use numan_cli::state::snapshot::{list_snapshots, SnapshotTrigger}; use std::path::{Path, PathBuf}; use std::sync::Mutex; use tempfile::TempDir; @@ -30,12 +32,12 @@ fn discover_off_path_test() -> Option { fn nu_setup_repair_test( args: &numan_cli::cmd::setup::NuSetupArgs, - _root: &Path, + root: &Path, ) -> anyhow::Result<()> { - let expected = TEST_OFF_PATH.lock().unwrap(); + 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 }) and leaves use_existing unset. - let Some(numan_cli::cmd::setup::NuAction::Use { path }) = &args.action else { + 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())); @@ -49,7 +51,62 @@ fn nu_setup_repair_test( "doctor found_off_path repair must not pass --yes" ); *TEST_NU_SETUP_CALLED.lock().unwrap() = true; - Ok(()) + + // Seed a managed install just before the production use path. Seeding earlier + // would make Nu "available" and suppress `nu.binary.found_off_path`. + let managed = managed_nu_binary(root); + std::fs::create_dir_all(managed.parent().unwrap())?; + std::fs::write(&managed, b"managed-nu")?; + // Use a missing binary so execute_nu fails before PATH persistence or wipe. + let missing = root.join("missing-off-path-nu"); + let err = setup::execute_nu(&setup::NuSetupArgs::use_existing(missing, args.yes), root) + .expect_err("expected resolve failure for missing off-PATH binary"); + assert!( + managed.exists(), + "doctor off-PATH repair must not delete managed tools/nushell without consent: {err}" + ); + Err(err) +} + +/// Valid fake Nu for consent-gate tests (Unix). Must look runnable to +/// `validate_nushell_binary` so the failure is the wipe/PATH consent gate. +#[cfg(unix)] +fn write_valid_off_path_nu(tmp: &Path) -> PathBuf { + use std::os::unix::fs::PermissionsExt; + let bin = tmp.join("valid-off-path-nu"); + let script: &[u8] = b"#!/bin/sh\n\ +case $1 in\n\ + -c|--version) printf '{ \"version\":\"0.113.1\", \"plugin_path\":\"/tmp\", \"data_dir\":\"/tmp\", \"vendor_autoload_dirs\":[\"/tmp/vendor/autoload\"] }\n' ;;\n\ +esac\n"; + std::fs::write(&bin, script).expect("write valid off-path nu"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)) + .expect("chmod valid off-path nu"); + bin +} + +#[cfg(unix)] +#[test] +fn execute_nu_use_existing_refuses_without_consent_and_keeps_managed() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root).unwrap(); + + let managed = managed_nu_binary(root); + std::fs::create_dir_all(managed.parent().unwrap()).unwrap(); + std::fs::write(&managed, b"managed-nu").unwrap(); + + let off_path = write_valid_off_path_nu(dir.path()); + let err = setup::execute_nu(&setup::NuSetupArgs::use_existing(off_path, false), root) + .expect_err("expected consent-gate refusal with yes=false"); + let msg = format!("{err:#}"); + assert!( + msg.contains("non-interactive") || msg.contains("--yes"), + "expected require_tty_or_yes consent refusal, got: {msg}" + ); + assert!( + managed.exists(), + "managed install must remain when consent gate refuses off-PATH registration" + ); } struct ClearedPath { @@ -137,6 +194,174 @@ fn doctor_fix_auto_creates_layout_without_network() { assert_eq!(code, 0); assert!(root.join("state").is_dir()); assert!(root.join("nu_state/paths.json").is_file()); + let snapshots = list_snapshots(root).unwrap(); + assert!( + snapshots + .iter() + .any(|s| s.trigger == SnapshotTrigger::Doctor), + "default doctor repairs must create a PreMutation Doctor snapshot" + ); +} + +#[test] +fn doctor_repairs_malformed_active_version_marker() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("nu_state")).unwrap(); + std::fs::write(root.join("nu_state/active-version.json"), b"{not-json").unwrap(); + let nu_exe = managed_nu_binary(root); + std::fs::create_dir_all(nu_exe.parent().unwrap()).unwrap(); + std::fs::write(&nu_exe, b"nu").unwrap(); + + let scan = run_checks_with_options( + &DoctorArgs { + scan: true, + json: true, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + let finding = scan + .findings + .iter() + .find(|f| f.id == "nu.active_version.malformed") + .expect("expected nu.active_version.malformed"); + assert_eq!(finding.severity, Severity::Error); + + let code = execute_with_options( + &DoctorArgs { + scan: false, + json: true, + nupm_home: None, + }, + root, + DoctorOptions { + init_repair: Some(fake_init), + ..test_doctor_options() + }, + ) + .unwrap(); + assert_eq!( + code, 0, + "clearing the marker should leave no remaining errors" + ); + assert!( + !root.join("nu_state/active-version.json").exists(), + "doctor must clear the malformed active-version marker" + ); + let after = run_checks_with_options( + &DoctorArgs { + scan: true, + json: true, + nupm_home: None, + }, + root, + &test_doctor_options(), + ) + .unwrap(); + let repaired = after + .findings + .iter() + .find(|f| f.id == "nu.active_version.malformed") + .expect("finding remains as ok after repair"); + assert_eq!(repaired.severity, Severity::Ok); +} + +#[test] +fn doctor_repairs_layout_when_lockfile_malformed() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root).unwrap(); + std::fs::write(root.join("lockfile"), b"{not-valid-json").unwrap(); + let nu_exe = managed_nu_binary(root); + std::fs::create_dir_all(nu_exe.parent().unwrap()).unwrap(); + std::fs::write(&nu_exe, b"nu").unwrap(); + // Seed a cached index so default repair mode does not attempt registry sync. + let index_path = root.join("registry/official/index.json"); + std::fs::create_dir_all(index_path.parent().unwrap()).unwrap(); + std::fs::write( + &index_path, + r#"{"schema_version":1,"packages":[],"generated_at":"test"}"#, + ) + .unwrap(); + + // Capture the JSON repair report via subprocess stdout (same pattern as + // doctor_json_default_stdout_is_valid_json). execute_with_options returns + // only an exit code, so the repairs contract must be asserted from --json. + let output = std::process::Command::new(env!("CARGO_BIN_EXE_numan")) + .args([ + "--root", + root.to_str().expect("temp root is utf-8"), + "doctor", + "--json", + ]) + .env("NUMAN_ALLOW_UNSIGNED", "1") + .output() + .expect("run numan doctor --json with malformed lockfile"); + + assert_eq!( + output.status.code(), + Some(1), + "malformed lockfile leaves error-severity findings; expected exit 1\nstdout={}\nstderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let stdout = String::from_utf8_lossy(&output.stdout); + let value: serde_json::Value = serde_json::from_str(&stdout).unwrap_or_else(|e| { + panic!( + "doctor --json stdout must be valid JSON after malformed lockfile: {e}\nstdout={stdout}\nstderr={}", + String::from_utf8_lossy(&output.stderr) + ) + }); + let repairs = value + .get("repairs") + .and_then(|r| r.as_array()) + .unwrap_or_else(|| panic!("doctor --json must include repairs array: {value}")); + + let snapshot = repairs + .iter() + .find(|r| r.get("id").and_then(|id| id.as_str()) == Some("snapshot.pre_mutation")) + .unwrap_or_else(|| panic!("expected snapshot.pre_mutation repair record: {repairs:?}")); + assert_eq!( + snapshot.get("status").and_then(|s| s.as_str()), + Some("failed"), + "snapshot.pre_mutation must be failed: {snapshot}" + ); + + let skipped_snapshot_deps: Vec<_> = repairs + .iter() + .filter(|r| { + r.get("status").and_then(|s| s.as_str()) == Some("skipped") + && r.get("reason").and_then(|s| s.as_str()) == Some("snapshot_unavailable") + }) + .collect(); + assert!( + !skipped_snapshot_deps.is_empty(), + "snapshot-dependent repairs must be skipped with snapshot_unavailable: {repairs:?}" + ); + + assert!( + root.join("state").is_dir(), + "layout.state must still be created when PreMutation snapshot fails" + ); + assert!( + root.join("packages").is_dir(), + "layout.packages must still be created when PreMutation snapshot fails" + ); + assert!( + root.join("registries").is_dir(), + "layout.registries must still be created when PreMutation snapshot fails" + ); + assert!( + list_snapshots(root) + .unwrap() + .iter() + .all(|s| s.trigger != SnapshotTrigger::Doctor), + "malformed lockfile must not publish a Doctor PreMutation snapshot" + ); } #[test] @@ -251,6 +476,47 @@ fn doctor_reports_off_path_nu_without_download() { assert_eq!(missing.severity, Severity::Ok); } +fn nu_setup_must_not_be_called( + _args: &numan_cli::cmd::setup::NuSetupArgs, + _root: &Path, +) -> anyhow::Result<()> { + *TEST_NU_SETUP_CALLED.lock().unwrap() = true; + panic!("doctor must not invoke setup::execute_nu_repair for missing Nu"); +} + +#[test] +fn doctor_default_does_not_auto_install_managed_nu() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root).unwrap(); + + *TEST_NU_SETUP_CALLED.lock().unwrap() = false; + let _path_guard = TEST_PATH_GUARD.lock().unwrap(); + let _cleared_path = ClearedPath::new(); + let args = DoctorArgs { + scan: false, + json: false, + nupm_home: None, + }; + let _code = execute_with_options( + &args, + root, + DoctorOptions { + // Network allowed: previously this would have downloaded managed Nu. + skip_network: false, + nu_setup_repair: Some(nu_setup_must_not_be_called), + discover_off_path: Some(|| None), + ..test_doctor_options() + }, + ) + .unwrap(); + + assert!( + !*TEST_NU_SETUP_CALLED.lock().unwrap(), + "default doctor must not call setup Nu install without explicit consent" + ); +} + #[test] fn doctor_fix_registers_off_path_nu_without_network() { let dir = tempfile::tempdir().unwrap(); @@ -283,6 +549,10 @@ fn doctor_fix_registers_off_path_nu_without_network() { .unwrap(); assert!(*TEST_NU_SETUP_CALLED.lock().unwrap()); + assert!( + managed_nu_binary(root).exists(), + "off-PATH registration must not delete managed tools/nushell" + ); assert_eq!(code, 1); } @@ -427,18 +697,11 @@ fn probe_fixed_version(_path: &Path) -> anyhow::Result { Ok("0.99.9".to_string()) } -fn confirm_repairs_always(_args: &DoctorArgs) -> bool { - true -} - /// Skip network and never exec a real `nu` during doctor integration tests. -/// Force confirm-tier repairs on so non-TTY CI can exercise repair paths that -/// production gates behind an interactive session. fn test_doctor_options() -> DoctorOptions { DoctorOptions { skip_network: true, nu_version_probe: Some(probe_fixed_version), - confirm_repairs: Some(confirm_repairs_always), ..DoctorOptions::default() } } @@ -536,20 +799,21 @@ fn doctor_reports_managed_and_trust_root_findings() { assert!(json.contains("registry.trust_root")); } -/// Default `doctor --json` runs repairs; nested helpers may `println!`. -/// Stdout must remain a single JSON object (repair chatter goes to stderr). #[test] fn doctor_json_default_stdout_is_valid_json() { let dir = TempDir::new().unwrap(); let root = dir.path(); std::fs::create_dir_all(root).unwrap(); + fake_init(&InitArgs { refresh: false }, root).unwrap(); + // Seed a cached index so default repair mode does not attempt registry sync. + let index_path = root.join("registry/official/index.json"); + std::fs::create_dir_all(index_path.parent().unwrap()).unwrap(); + std::fs::write( + &index_path, + r#"{"schema_version":1,"packages":[],"generated_at":"test"}"#, + ) + .unwrap(); - // Uninitialized root so default repairs call `init`, which prints human - // text that must not land on stdout when `--json` is set. - // Null stdin makes the confirm-tier allow-gate deterministic (never inherit - // a developer TTY that would download managed Nu). Empty-root initial - // findings do not include `registry.index_missing`, so no network sync runs - // in this first repair pass. let output = std::process::Command::new(env!("CARGO_BIN_EXE_numan")) .args([ "--root", @@ -562,16 +826,18 @@ fn doctor_json_default_stdout_is_valid_json() { .output() .expect("run numan doctor --json"); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); assert!( - matches!(output.status.code(), Some(0) | Some(1) | Some(2)), - "default doctor --json must exit with a doctor status code: status={}\nstdout={stdout}\nstderr={stderr}", - output.status + output.status.success(), + "doctor --json must exit 0\nstdout={}\nstderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) ); + + let stdout = String::from_utf8_lossy(&output.stdout); let value: serde_json::Value = serde_json::from_str(&stdout).unwrap_or_else(|e| { panic!( - "default doctor --json stdout must be valid JSON: {e}\nstdout={stdout}\nstderr={stderr}" + "default doctor --json stdout must be valid JSON: {e}\nstdout={stdout}\nstderr={}", + String::from_utf8_lossy(&output.stderr) ) }); assert!( diff --git a/tests/module_autoload_test.rs b/tests/module_autoload_test.rs index 479f946e..6d8fe6fb 100644 --- a/tests/module_autoload_test.rs +++ b/tests/module_autoload_test.rs @@ -1543,16 +1543,9 @@ fn real_nu_import_mode_all_exports_to_global_scope() { } #[test] +#[cfg(target_os = "windows")] #[ignore = "requires real Nu binary on $PATH — run in platform acceptance job"] fn real_nu_windows_path_with_spaces_validates() { - // On Windows, verify that a path containing spaces is correctly escaped - // and that Nu can parse the generated use statement. - #[cfg(not(windows))] - { - eprintln!("Skipping: Windows-specific test"); - return; - } - let nu = match find_nu_binary() { Some(p) => p, None => {