diff --git a/AGENTS.md b/AGENTS.md index 4308f1ce..2bb04895 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 [--fix] [--yes]`: health checks + safe repairs (Phase 7.2; spec: docs/numan-doctor.md) + doctor.rs — `numan doctor [--scan]`: health checks + safe repairs by default (or report-only with `--scan`) (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) @@ -64,7 +64,7 @@ src/ nupm.rs — `numan nupm status|inspect|import|diff`: nupm discovery + import + drift (Phase 6.1–6.3) completions.rs — `numan completions `: bash/fish/zsh/powershell/nushell scripts (Phase 7.3) setup.rs — `numan setup nu [VERSION]|remove|path|use ` + `setup loader`: Nushell bootstrap + nushell-loader install - try_cmd.rs — `numan try [--yes] [--no-activate]`: curated starter install + activate for current Nu + try_cmd.rs — `numan try [--no-activate]`: curated starter install + activate for current Nu use_cmd.rs — `numan use |latest|list`: activates a previously installed managed Nu version (no auto-download); writes the active-version marker after a PreMutation snapshot under the root mutation lock nu_pin_offer.rs — Shared TTY offer to `setup nu ` + `init --refresh` on Nu mismatch install/ @@ -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, --fix auto tier, journal checks (Phase 7.2) + doctor_test.rs — `numan doctor` report-only (`--scan`), default auto/confirm tiers, 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) @@ -152,7 +152,7 @@ tests/ 4. **Lockfile pins immutable paths** — cached artifacts retained while referenced 5. **Registry trust** — Ed25519 signatures over exact `index.json` bytes; bypass requires `NUMAN_ALLOW_UNSIGNED=1` (dev only) 6. **Artifact SHA256 is mandatory for plugins** — the install transaction bails if `sha256` is missing from a binary artifact -7. **State snapshots before mutation** — `create_snapshot()` before `install`/`update`/`remove`/`activate`/`deactivate`/nupm-import mutations; `numan gc` treats every snapshot's referenced payloads as live roots +7. **State snapshots before mutation** — `create_snapshot()` before `install`/`update`/`remove`/`activate`/`deactivate`/`init --refresh`/nupm-import/doctor repair mutations; `numan gc` treats every snapshot's referenced payloads as live roots 8. **Platform triple** — comes from `#[cfg(target_env)]` at compile time, not `std::env::consts` (see `core/platform.rs`; `LIBC` is a compile-time const) ## Development Workflow diff --git a/Cargo.toml b/Cargo.toml index 7271e062..f984de06 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,10 @@ uuid = { version = "1", features = ["v7", "v4"] } # Git (for source builds and registry sync) git2 = "0.21" +# stdio redirect for doctor --json repair passes (Unix only) +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [dev-dependencies] tempfile = "3" rand_core = { version = "0.6", features = ["getrandom"] } diff --git a/README.md b/README.md index 73605997..d621efe1 100644 --- a/README.md +++ b/README.md @@ -269,8 +269,8 @@ See [docs/snapshots-and-rollback.md](docs/snapshots-and-rollback.md) for scope, #### 6. Verify health ```bash -numan doctor # report-only diagnosis -numan doctor --fix --yes # apply safe automated repairs +numan doctor # diagnose and apply safe automated repairs +numan doctor --scan # report-only diagnosis ``` --- @@ -308,7 +308,7 @@ Global flag: `--root ` — override the Numan root directory (all commands | Command | Description | |---------|-------------| | `numan init [--refresh]` | Probe Nu and cache paths for activation | -| `numan try [--yes] [--no-activate]` | Install and activate a curated starter package for your Nu + platform (prefers Nu 0.114 starters; suggests managed Nu pin or search if no compatible starter) | +| `numan try [--no-activate]` | Install and activate a curated starter package for your Nu + platform (prefers Nu 0.114 starters; suggests managed Nu pin or search if no compatible starter) | | `numan search ` | Search registry by name, description, or tags | | `numan info ` | Show package metadata and available versions | | `numan install ` | Download, verify, extract, and lock | @@ -336,15 +336,15 @@ Global flag: `--root ` — override the Numan root directory (all commands | `numan nupm import --manifest file.toml` | Batch import from manifest | | `numan nupm diff ` | Compare imported payload vs nupm source | | `numan completions ` | Generate bash, fish, zsh, powershell, or nushell completions | -| `numan doctor [--fix] [--yes] [--json]` | Diagnose root health; optional safe repairs | +| `numan doctor [--scan] [--json]` | Diagnose root health and repair (use `--scan` for report-only) | ### Common flags (by command) | Command | Flags | |---------|-------| | `install` | `--force` reinstall; `-v` / `--verbose` | -| `activate` | `--yes` skip prompt; `--verbose`; `--list` status only; `--check` integrity only | -| `deactivate` | `--yes` skip prompt; `--verbose` | +| `activate` | `--verbose`; `--list` status only; `--check` integrity only | +| `deactivate` | `--verbose` | | `update` | `--check` report only; `-v` / `--verbose` | | `remove` | `--force` remove despite active activation | | `gc` | `--dry-run` preview only | @@ -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` | `--fix` apply safe repairs; `--yes` skip confirm tier; `--json` machine output; `--nupm-home ` | +| `doctor` | `--scan` report-only; `--json` machine output; `--nupm-home ` | | `setup nu` | `--force` re-download; `--skip-path` don't update PATH; `--yes` skip prompt | Run `numan --help` for full flag documentation. diff --git a/docs/numan-doctor.md b/docs/numan-doctor.md index 730fb5f8..e0ca87fb 100644 --- a/docs/numan-doctor.md +++ b/docs/numan-doctor.md @@ -5,9 +5,9 @@ ## Purpose -`numan doctor` diagnoses the health of a Numan root and, with `--fix`, applies **safe automated repairs** — the same pattern as `brew doctor`, `npm doctor`, and similar tooling. +`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. -Default mode is **report-only** (safe for CI and scripting). Repair mode delegates to existing commands (`init`, `activate`, `registry sync`) rather than inventing new mutation paths. +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. It answers: *“Is this Numan root consistent, safe to mutate, and aligned with the current Nu environment?”* and optionally *“Fix what you can.”* @@ -22,20 +22,22 @@ It answers: *“Is this Numan root consistent, safe to mutate, and aligned with ## Invocation ```text -numan doctor [--fix] [--yes] [--json] [--nupm-home PATH] +numan doctor [--scan] [--json] [--nupm-home PATH] ``` | Flag | Behavior | |------|----------| -| `--fix` | After reporting, apply automated repairs (see [Repair policy](#repair-policy)) | -| `--yes` | Skip confirmation prompts for **confirm**-tier repairs (non-TTY implies `--yes` for confirm tier only) | -| `--json` | Emit a single JSON object (schema versioned); no ANSI styling. With `--fix`, include `repairs` attempted/applied | +| `--scan` | Report findings without applying repairs (dry-run mode) | +| `--json` | Emit a single JSON object (schema versioned); no ANSI styling. Includes a `repairs` array when repairs ran; omitted under `--scan` | | `--nupm-home PATH` | Override nupm home for the optional coexistence section (same resolution order as `numan nupm status`) | Global `--root` applies as for all commands. -**Default (no flags):** diagnose and print findings + manual fix hints. -**`--fix`:** diagnose, then repair what is allowed without user-supplied data. +**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. +**`--scan`:** diagnose and print findings without mutating state. ## Exit codes @@ -53,7 +55,7 @@ Each finding has: - `severity` — `ok` \| `info` \| `warn` \| `error` - `message` — human-readable summary - `fix` — optional suggested command for manual issues (e.g. `numan init` or `numan registry add …`) -- `repair` — `none` \| `auto` \| `confirm` \| `manual` (whether `--fix` can act; see below) +- `repair` — `none` \| `auto` \| `confirm` \| `manual` (whether default repair mode can act; see below) **Rules:** @@ -64,7 +66,7 @@ Each finding has: ## Repair policy -When `--fix` is set, doctor acquires `acquire_mutation_lock(root)` for its own +By default, 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. @@ -77,12 +79,12 @@ Repair steps run in this **order** (each step re-validates only what it changed) | **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** | Unless `--yes` / non-TTY | `nu.binary.missing_on_path` | `numan setup nu` (downloads managed Nushell) | -| **confirm** | Unless `--yes` / non-TTY | `nu.binary.found_off_path` | `numan setup nu use ` (adds existing install to PATH) | -| **confirm** | Unless `--yes` / non-TTY | `nu_paths.drift`, `nu_paths.vendor_drift` | `numan init --refresh` | -| **confirm** | Unless `--yes` / non-TTY | `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** | Unless `--yes` / non-TTY | `journal.plugin_deactivate_pending` | `numan deactivate ` (reconciles pending-plugin-deactivate journal only; not a full-root deactivate) | -| **confirm** | Unless `--yes` / non-TTY | `journal.plugin_deactivate_stale` | `numan init --refresh` then `numan deactivate` | +| **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` | | **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) | @@ -96,7 +98,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:** Default mode still *reports* journals without acting. `--fix` 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. +**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. ## Check catalog @@ -116,8 +118,8 @@ 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 `--fix`). | -| `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 `--fix`). | +| `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_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` | @@ -163,9 +165,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 --fix` after init (production trust root); `numan registry add …` for custom/placeholder builds | +| `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 `--fix`). | +| `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). | ### 7. nupm coexistence (optional section) @@ -203,16 +205,8 @@ nupm coexistence Summary: 1 error, 1 warning -Repairs (--fix only): - ✓ Created missing state/ directory - ✓ Ran registry sync - → numan init --refresh required (skipped; re-run with --yes) -``` - -With `--fix` and repairs applied: - -```text -Repairs: 2 applied, 1 skipped (use --yes to apply confirm-tier fixes) +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. ``` Use `console` styling consistent with `activate --check`. @@ -238,9 +232,6 @@ Use `console` styling consistent with `activate --check`. { "id": "nu_paths.drift", "status": "skipped", "reason": "not_confirmed" } ] } -``` - -`repairs` is present only when `--fix` was passed. ## Architecture @@ -261,27 +252,29 @@ pub fn execute_with_options(args: &DoctorArgs, root: &Path, options: DoctorOptio | Command | Role | |---------|------| -| `numan init` / `init --refresh` | **Repair** Nu path drift (`--fix` delegates here) | -| `numan setup nu` | **Repair** missing Nushell (`nu.binary.missing_on_path`; `--fix` downloads managed binary) | -| `numan setup nu use ` | **Repair** off-PATH Nushell (`nu.binary.found_off_path`; `--fix` adds parent dir to user PATH) | -| `numan activate` | **Repair** activation + journal reconciliation (`--fix` delegates here) | -| `numan registry sync` | **Repair** missing index cache (`--fix` auto tier) | +| `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 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 | | `numan update` / `remove` / `gc` | Block on stale lifecycle journal; doctor reports, does not fix lifecycle | ## Definition of done -- [x] `numan doctor`, `numan doctor --fix`, and `numan doctor --json` implemented per check catalog +- [x] `numan doctor`, `numan doctor --scan`, and `numan doctor --json` implemented per check catalog - [x] `scan_on_doctor` respected -- [x] Default mode: no state mutation (test: hashes unchanged) -- [x] `--fix` mode: only repair tiers in policy; uses mutation lock; delegates to init/activate/sync +- [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] Documented in README command table and `AGENTS.md` -- [x] Integration tests: report-only, `--fix` auto tier, `--fix` confirm tier with `--yes`, manual tier untouched +- [x] Integration tests: `--scan` report-only, default auto tier, default confirm tier with TTY, manual tier untouched ## Changelog | Date | Change | |------|--------| | 2026-06-30 | Initial spec (Phase 7.2) | -| 2026-06-30 | Add `--fix` / `--yes` repair policy (auto / confirm / manual tiers) | +| 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 | diff --git a/docs/snapshots-and-rollback.md b/docs/snapshots-and-rollback.md index 841be994..2d0c3891 100644 --- a/docs/snapshots-and-rollback.md +++ b/docs/snapshots-and-rollback.md @@ -45,7 +45,8 @@ Snapshots are cheap: they store JSON metadata and small sidecar files (lockfile, autoload content, imports), not payload copies. Storage under `/state/snapshots//` grows roughly linearly with the number of snapshot-triggering operations (`install`, `update`, `remove`, `activate`, -`deactivate`, nupm imports, and rollback itself), not with payload size. +`deactivate`, `init --refresh`, doctor repairs, nupm imports, and rollback +itself), not with payload size. A snapshot does, however, keep any payload directory it references alive: `numan gc` treats every committed snapshot's lockfile as a live root, so an diff --git a/src/cli.rs b/src/cli.rs index 5482d245..37e20002 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -36,9 +36,9 @@ pub enum Commands { Remove(cmd::remove::RemoveArgs), /// Garbage-collect orphaned package directories Gc(cmd::gc::GcArgs), - /// Activate installed plugins with Nu + /// Activate installed plugins and modules with Nu Activate(cmd::activate::ActivateArgs), - /// Deactivate active modules + /// Deactivate active plugins and modules Deactivate(cmd::deactivate::DeactivateArgs), /// List all installed packages List, @@ -54,7 +54,7 @@ pub enum Commands { Nupm(cmd::nupm::NupmArgs), /// Generate shell completion scripts Completions(cmd::completions::CompletionsArgs), - /// Diagnose Numan root health and optionally apply safe repairs + /// Diagnose Numan root health and apply safe repairs (use `--scan` for report-only) Doctor(cmd::doctor::DoctorArgs), /// Install optional Nushell integration helpers #[command(subcommand)] diff --git a/src/cmd/activate.rs b/src/cmd/activate.rs index 6b12b049..cebe89f4 100644 --- a/src/cmd/activate.rs +++ b/src/cmd/activate.rs @@ -25,16 +25,12 @@ pub struct ActivateArgs { /// Package IDs (owner/name) to activate. Omit to activate all installed inactive packages. pub packages: Vec, - /// Skip confirmation prompts - #[arg(long)] - pub yes: bool, - /// Show detailed output #[arg(long)] pub verbose: bool, /// List all installed packages and their activation status (read-only) - #[arg(long, conflicts_with_all = ["yes", "packages"])] + #[arg(long, conflicts_with = "packages")] pub list: bool, /// Check activation integrity for packages (read-only, no mutation) @@ -154,7 +150,7 @@ fn execute_with_registrar_and_runner( }; drop(planning_lock); - // 8. Consent table + confirmation + // 8. Consent table (informational only; no prompt) print_grouped_consent_table( &plugin_targets, &module_targets, @@ -162,8 +158,6 @@ fn execute_with_registrar_and_runner( &nu_paths.plugin_registry_path, ); - crate::util::confirm::confirm_or_bail("Proceed?", args.yes, "Activation cancelled.")?; - // 9. Reacquire the root mutation lock after consent. let _lock = acquire_mutation_lock(root)?; @@ -1421,7 +1415,7 @@ mod tests { let args = ActivateArgs { packages: vec!["owner/myscript".to_string()], - yes: true, + verbose: false, list: false, check: false, @@ -1501,7 +1495,7 @@ mod tests { let args = ActivateArgs { packages: vec!["owner/mycomp".to_string()], - yes: true, + verbose: false, list: false, check: false, @@ -1587,7 +1581,7 @@ mod tests { let args = ActivateArgs { packages: vec!["owner/mymod".to_string()], - yes: true, + verbose: false, list: false, check: false, @@ -1669,7 +1663,7 @@ mod tests { let args = ActivateArgs { packages: vec!["owner/depmod".to_string()], - yes: true, + verbose: false, list: false, check: false, diff --git a/src/cmd/deactivate.rs b/src/cmd/deactivate.rs index 046d9d70..295e0bfe 100644 --- a/src/cmd/deactivate.rs +++ b/src/cmd/deactivate.rs @@ -39,10 +39,6 @@ pub struct DeactivateArgs { /// Package IDs (owner/name) to deactivate. Omit to deactivate all active plugins and modules. pub packages: Vec, - /// Skip confirmation prompts - #[arg(long)] - pub yes: bool, - /// Show detailed output #[arg(long)] pub verbose: bool, @@ -146,11 +142,9 @@ pub fn execute_with_candidate_runner_and_unregistrar( } drop(planning_lock); - // 5. Show consent table and confirm + // 5. Show consent table (informational only; no prompt) print_consent_table(&targets_requested, &nu_paths.plugin_registry_path); - crate::util::confirm::confirm_or_bail("Proceed?", args.yes, "Deactivation cancelled.")?; - // 6. Reacquire the root mutation lock after consent. let _lock = acquire_mutation_lock(root)?; @@ -166,7 +160,7 @@ pub fn execute_with_candidate_runner_and_unregistrar( // on the current authoritative state. let mut lockfile = Lockfile::load(root)?; let targets_requested = - reclassify_confirmed_targets(args, &lockfile, root, &nu_paths, &targets_requested)?; + reclassify_targets(args, &lockfile, root, &nu_paths, &targets_requested)?; if targets_requested.is_empty() { println!("Nothing to deactivate."); return Ok(()); @@ -505,17 +499,17 @@ fn active_plugin_from_entry( }) } -fn reclassify_confirmed_targets( +fn reclassify_targets( args: &DeactivateArgs, lockfile: &Lockfile, root: &Path, nu_paths: &NuPaths, - confirmed_targets: &ClassifiedTargets, + planned_targets: &ClassifiedTargets, ) -> Result { let current_targets = classify_and_validate_packages(args, lockfile, root, nu_paths)?; - if current_targets != *confirmed_targets { + if current_targets != *planned_targets { bail!( - "Activation state changed after confirmation. No packages were deactivated; retry the command to review the current targets." + "Activation state changed since planning; no packages were deactivated. Retry to review current targets." ); } Ok(current_targets) @@ -1339,7 +1333,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/myplugin".to_string()], - yes: true, + verbose: false, }; @@ -1360,7 +1354,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/myplugin".to_string()], - yes: true, + verbose: false, }; @@ -1381,7 +1375,7 @@ mod tests { let args = DeactivateArgs { packages: vec![], - yes: true, + verbose: false, }; @@ -1397,7 +1391,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/myplugin".to_string()], - yes: true, + verbose: false, }; @@ -1414,7 +1408,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/myscript".to_string()], - yes: true, + verbose: false, }; @@ -1433,7 +1427,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/mycomp".to_string()], - yes: true, + verbose: false, }; @@ -1453,7 +1447,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/mymod".to_string()], - yes: true, + verbose: false, }; @@ -1472,7 +1466,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/nosuchpkg".to_string()], - yes: true, + verbose: false, }; @@ -1492,7 +1486,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/mymod".to_string()], - yes: true, + verbose: false, }; @@ -1552,7 +1546,7 @@ mod tests { let args = DeactivateArgs { packages: vec![], - yes: true, + verbose: false, }; @@ -1564,11 +1558,11 @@ mod tests { } #[test] - fn reclassification_rejects_expanded_implicit_targets_after_confirmation() { + fn reclassification_rejects_expanded_implicit_targets_after_planning() { let dir = TempDir::new().unwrap(); let args = DeactivateArgs { packages: vec![], - yes: true, + verbose: false, }; let confirmed_lockfile = make_lockfile_with_modules(vec![("owner/alpha", "module", true)]); @@ -1584,7 +1578,7 @@ mod tests { ("owner/beta", "module", true), ]); - let error = reclassify_confirmed_targets( + let error = reclassify_targets( &args, &changed_lockfile, dir.path(), @@ -1592,7 +1586,7 @@ mod tests { &confirmed_targets, ) .unwrap_err(); - assert!(error.to_string().contains("changed after confirmation")); + assert!(error.to_string().contains("changed since planning")); } #[test] @@ -1613,7 +1607,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/highlight".to_string()], - yes: true, + verbose: false, }; execute_with_unregistrar(&args, env.root(), &|_nu, identity, _cfg| { @@ -1670,7 +1664,7 @@ mod tests { let called = AtomicBool::new(false); let args = DeactivateArgs { packages: vec![], - yes: true, + verbose: false, }; let err = execute_with_unregistrar(&args, env.root(), &|_nu, _identity, _cfg| { @@ -1730,7 +1724,7 @@ mod tests { let called = AtomicBool::new(false); let args = DeactivateArgs { packages: vec!["owner/highlight".to_string()], - yes: true, + verbose: false, }; execute_with_unregistrar(&args, env.root(), &|_nu, _identity, _cfg| { @@ -1761,7 +1755,7 @@ mod tests { let args = DeactivateArgs { packages: vec!["owner/highlight".to_string()], - yes: true, + verbose: false, }; let err = execute_with_unregistrar(&args, env.root(), &|_nu, _name, _cfg| { diff --git a/src/cmd/doctor.rs b/src/cmd/doctor.rs index a6564625..667c9a45 100644 --- a/src/cmd/doctor.rs +++ b/src/cmd/doctor.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Context, Result}; use clap::Args; use console::style; use serde::Serialize; @@ -29,25 +29,23 @@ use crate::state::lifecycle_journal::PendingLifecycle; use crate::state::lockfile::Lockfile; use crate::state::nupm_import::NupmImportsFile; use crate::state::plugin_deactivate_journal::PendingPluginDeactivate; +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, }; +use crate::util::stdio_redirect::StdoutToStderr; const SCHEMA_VERSION: u32 = 1; const LAYOUT_DIRS: &[&str] = &["nu_state", "state", "packages", "registries"]; #[derive(Debug, Args)] pub struct DoctorArgs { - /// Apply safe automated repairs after reporting + /// Scan only — report issues without applying fixes #[arg(long)] - pub fix: bool, - - /// Skip confirmation prompts for confirm-tier repairs - #[arg(long)] - pub yes: bool, + pub scan: bool, /// Emit JSON report (no ANSI styling) #[arg(long)] @@ -134,6 +132,10 @@ 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 { @@ -142,7 +144,7 @@ pub fn execute(args: &DoctorArgs, root: &Path) -> Result { pub fn execute_with_options(args: &DoctorArgs, root: &Path, options: DoctorOptions) -> Result { let mut report = run_checks_with_options(args, root, &options)?; - if args.fix { + if !args.scan { let repairs = apply_repairs(args, root, &report.findings, &options)?; report = run_checks_with_options(args, root, &options)?; report.repairs = Some(repairs); @@ -985,8 +987,11 @@ fn count_nupm_name_overlap( Ok(count) } -fn confirm_repairs(args: &DoctorArgs) -> bool { - args.yes || !std::io::stdin().is_terminal() +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( @@ -998,14 +1003,40 @@ fn apply_repairs( let needs_lock = findings.iter().any(|f| { matches!(f.repair, RepairTier::Auto | RepairTier::Confirm) && f.severity != Severity::Ok }); + + // Nested repair handlers may println!; redirect only when those handlers + // are about to run so healthy --json scans avoid mutating process stdio. + let _stdout_guard = if args.json && needs_lock { + Some( + StdoutToStderr::redirect() + .context("Failed to redirect stdout while emitting doctor JSON")?, + ) + } else { + None + }; + let mut lock = if needs_lock { Some(acquire_mutation_lock(root)?) } else { None }; + if needs_lock { + create_snapshot( + root, + SnapshotReason::PreMutation, + SnapshotTrigger::Doctor, + None, + None, + ) + .context("Failed to create doctor pre-mutation snapshot")?; + } + let mut records = Vec::new(); - let confirm = confirm_repairs(args); + let confirm = match options.confirm_repairs { + Some(gate) => gate(args), + None => confirm_repairs(args), + }; for dir in LAYOUT_DIRS { let id = format!("layout.{dir}"); @@ -1077,7 +1108,9 @@ fn apply_repairs( }); } else if let Some(off_path) = resolve_off_path(options) { let setup_fn = options.nu_setup_repair.unwrap_or(setup::execute_nu_repair); - match setup_fn(&NuSetupArgs::use_existing(off_path, true), root) { + // 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. + match setup_fn(&NuSetupArgs::use_existing(off_path, false), root) { Ok(()) => records.push(RepairRecord { id, status: RepairStatus::Applied, @@ -1117,7 +1150,8 @@ fn apply_repairs( }); } else { let setup_fn = options.nu_setup_repair.unwrap_or(setup::execute_nu_repair); - match setup_fn(&NuSetupArgs::install(None, false, false, true), root) { + // Pass yes: false to let setup code handle TTY checking and confirmation. + match setup_fn(&NuSetupArgs::install(None, false, false, false), root) { Ok(()) => records.push(RepairRecord { id, status: RepairStatus::Applied, @@ -1236,7 +1270,6 @@ fn apply_repairs( } else { let activate_args = ActivateArgs { packages: Vec::new(), - yes: true, verbose: false, list: false, check: false, @@ -1293,7 +1326,7 @@ fn apply_repairs( } else { let deactivate_args = DeactivateArgs { packages: journal_packages, - yes: true, + verbose: false, }; let deactivate_fn = options.deactivate_repair.unwrap_or(deactivate_execute); @@ -1433,8 +1466,14 @@ fn print_report(args: &DoctorArgs, root: &Path, report: &DoctorReport) -> Result if !repairs.is_empty() { writeln!(out)?; writeln!(out, "Repairs: {applied} applied, {skipped} skipped")?; - if skipped > 0 && !args.yes { - writeln!(out, "(use --yes to apply confirm-tier fixes)")?; + 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." + )?; } } } @@ -1470,6 +1509,13 @@ 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 { @@ -1513,8 +1559,7 @@ mod tests { let root = dir.path(); std::fs::create_dir_all(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -1531,8 +1576,7 @@ mod tests { let root = dir.path(); std::fs::create_dir_all(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -1572,8 +1616,7 @@ mod tests { ensure_fake_managed_nu(root); let args = DoctorArgs { - fix: true, - yes: true, + scan: false, // Apply fixes (default behavior) json: false, nupm_home: None, }; @@ -1588,6 +1631,7 @@ mod tests { nu_setup_repair: None, discover_off_path: None, nu_version_probe: Some(probe_fixed_version), + confirm_repairs: Some(confirm_repairs_always), }, ) .unwrap(); @@ -1607,15 +1651,13 @@ mod tests { crate::config::Config::default().save(root).unwrap(); let args = DoctorArgs { - fix: true, - yes: true, + scan: false, // Apply fixes (default behavior) json: false, nupm_home: None, }; let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, // First check: report only to see findings json: false, nupm_home: None, }, @@ -1633,7 +1675,7 @@ mod tests { assert_eq!(none.repair, RepairTier::Manual); return; } - assert_eq!(none.fix.as_deref(), Some(hints::CMD_DOCTOR_FIX)); + assert_eq!(none.fix.as_deref(), Some(hints::CMD_DOCTOR)); assert_eq!(none.repair, RepairTier::Auto); execute_with_options( @@ -1647,6 +1689,7 @@ mod tests { nu_setup_repair: None, discover_off_path: None, nu_version_probe: Some(probe_fixed_version), + confirm_repairs: Some(confirm_repairs_always), }, ) .unwrap(); @@ -1664,8 +1707,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }, @@ -1694,8 +1736,7 @@ mod tests { fake_paths(root, &nu_exe).save(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }; @@ -1759,8 +1800,7 @@ mod tests { std::fs::create_dir_all(root.join("packages/plugins/owner/plugin/1.0.0-abc")).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -1829,8 +1869,7 @@ mod tests { std::fs::create_dir_all(root.join("packages/plugins/owner/plugin/1.0.0-abc")).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -1915,8 +1954,7 @@ mod tests { lockfile.save(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -1941,11 +1979,21 @@ 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() } } @@ -1958,8 +2006,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -2017,8 +2064,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -2060,8 +2106,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -2102,8 +2147,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -2142,8 +2186,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -2186,8 +2229,7 @@ mod tests { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -2206,4 +2248,104 @@ 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 19c24d09..56bfeb57 100644 --- a/src/cmd/init.rs +++ b/src/cmd/init.rs @@ -8,6 +8,7 @@ use crate::nu::autoload::{validate_candidate, CandidateRunner, NuCandidateRunner use crate::nu::paths::NuPaths; use crate::state::autoload_state::AutoloadState; use crate::state::lockfile::Lockfile; +use crate::state::snapshot::{create_snapshot, SnapshotReason, SnapshotTrigger}; use crate::util::format_timestamp; use crate::util::fs_safety::acquire_mutation_lock; use crate::util::fs_safety::assert_managed_file_owned; @@ -152,7 +153,7 @@ fn print_onboarding_next_steps(official_configured: bool) { } println!(); println!( - "Run 'numan doctor' to verify setup (use 'numan doctor --fix --yes' for safe repairs)." + "Run 'numan doctor' to verify setup and apply safe repairs (use 'numan doctor --scan' for report-only output)." ); } @@ -168,20 +169,23 @@ where let new_paths = detect()?; let lockfile = Lockfile::load(root)?; - let has_active_plugins = lockfile - .packages - .values() - .any(|entry| entry.activation.is_some()); let has_active_modules = lockfile .packages .values() .any(|entry| entry.module_activation.is_some()); - let _lock = if has_active_plugins || has_active_modules { - Some(acquire_mutation_lock(root)?) - } else { - None - }; + // Always serialize refresh writes (lockfile, autoload-state, paths), even + // when no packages are active. Skipping the lock left a race window against + // concurrent mutators. + let _lock = acquire_mutation_lock(root)?; + create_snapshot( + root, + SnapshotReason::PreMutation, + SnapshotTrigger::Init, + None, + None, + ) + .context("Failed to create pre-mutation snapshot for `numan init --refresh`")?; if has_active_modules { validate_refresh_for_active_modules( @@ -443,10 +447,14 @@ mod tests { let hash_v1 = paths_v1.nu_executable_hash.clone(); let mut lockfile = Lockfile::empty(); + let payload_rel = "packages/plugins/owner/plugin/1.0.0-abc"; + let payload_dir = root.join(payload_rel); + std::fs::create_dir_all(&payload_dir).unwrap(); + std::fs::write(payload_dir.join("nu_plugin_test"), b"bin").unwrap(); lockfile.packages.insert( "owner/plugin".to_string(), plugin_entry( - "packages/plugins/owner/plugin/1.0.0-abc", + payload_rel, Some(PluginActivation { plugin_registry_path: paths_v1.plugin_registry_path.clone(), nu_executable_sha256: hash_v1, @@ -473,4 +481,37 @@ mod tests { integrity::compute_sha256(b"v2") ); } + + #[test] + fn refresh_without_active_packages_still_locks_and_snapshots() { + use crate::state::snapshot::list_snapshots; + + 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::empty().save(root).unwrap(); + + let paths_v2 = fake_paths(root, &nu_v2, None); + execute_with_runner( + &InitArgs { refresh: true }, + root, + make_detect(paths_v2.clone()), + fake_runner_factory, + ) + .unwrap(); + + let loaded = NuPaths::load(root).unwrap(); + 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), + "init --refresh must create a PreMutation Init snapshot even with no active packages" + ); + } } diff --git a/src/cmd/install.rs b/src/cmd/install.rs index 5acf7299..9caeb2ae 100644 --- a/src/cmd/install.rs +++ b/src/cmd/install.rs @@ -24,10 +24,6 @@ pub struct InstallArgs { /// Verbose output #[arg(short, long)] verbose: bool, - - /// Skip confirmation prompts (does not auto-download a different Nu) - #[arg(long)] - yes: bool, } pub fn execute(args: &InstallArgs, root: &Path) -> Result<()> { @@ -46,12 +42,8 @@ pub fn execute(args: &InstallArgs, root: &Path) -> Result<()> { } eprintln!("Error: {first_err:#}"); - let accepted = nu_pin_offer::offer_managed_nu_pin( - root, - &nu_version.version, - &diagnosis, - args.yes, - )?; + let accepted = + nu_pin_offer::offer_managed_nu_pin(root, &nu_version.version, &diagnosis)?; if !accepted { return Err(first_err); } diff --git a/src/cmd/nu_pin_offer.rs b/src/cmd/nu_pin_offer.rs index 6ff90e9c..91cceda7 100644 --- a/src/cmd/nu_pin_offer.rs +++ b/src/cmd/nu_pin_offer.rs @@ -12,13 +12,22 @@ use crate::util::hints::{self, CMD_INIT_REFRESH}; /// Print blast-radius warning and optionally install managed Nu + refresh paths. /// /// Returns `Ok(true)` when a pin was installed and `init --refresh` succeeded. -/// Returns `Ok(false)` when the user declined or the session is non-interactive / -/// `--yes` (hints printed only; never auto-downloads Nu from `--yes` alone). +/// Returns `Ok(false)` when the user declined or the session is non-interactive +/// (hints printed only; never auto-downloads Nu without explicit confirmation). pub fn offer_managed_nu_pin( root: &Path, current_nu: &str, diagnosis: &PackageIncompatibility, - auto_yes: bool, +) -> Result { + offer_managed_nu_pin_with_tty(root, current_nu, diagnosis, None) +} + +/// Same as [`offer_managed_nu_pin`] but allows injecting TTY state for testing. +pub fn offer_managed_nu_pin_with_tty( + root: &Path, + current_nu: &str, + diagnosis: &PackageIncompatibility, + is_tty: Option, ) -> Result { let Some(pin) = diagnosis.suggested_pin.as_deref() else { return Ok(false); @@ -36,7 +45,8 @@ pub fn offer_managed_nu_pin( let setup_cmd = hints::setup_nu_version(pin); - if auto_yes || !std::io::stdin().is_terminal() { + let is_terminal = is_tty.unwrap_or_else(|| std::io::stdin().is_terminal()); + if !is_terminal { println!("To switch Nu, run:"); println!(" {setup_cmd} --yes --force"); println!(" {CMD_INIT_REFRESH}"); diff --git a/src/cmd/try_cmd.rs b/src/cmd/try_cmd.rs index 5bb61070..d0984113 100644 --- a/src/cmd/try_cmd.rs +++ b/src/cmd/try_cmd.rs @@ -20,12 +20,6 @@ use crate::util::hints::{self, CMD_REGISTRY_SYNC}; /// a matching managed Nu version or searching for another package with `numan search`. #[derive(Parser, Debug)] pub struct TryArgs { - /// Skip confirmation prompts. Does NOT consent to a managed-Nu install or - /// version switch via `nu_pin_offer`; that path is hard-gated against - /// silent switching even with `--yes`. - #[arg(long)] - pub yes: bool, - /// Install only; do not activate #[arg(long)] pub no_activate: bool, @@ -95,8 +89,7 @@ pub fn execute(args: &TryArgs, root: &Path) -> Result<()> { StarterSelection::Compatible(id) => id, StarterSelection::NeedsPin { id, diagnosis } => { println!("Starter '{id}' needs a different Nu than {}.", nu.version); - let accepted = - nu_pin_offer::offer_managed_nu_pin(root, &nu.version, &diagnosis, args.yes)?; + let accepted = nu_pin_offer::offer_managed_nu_pin(root, &nu.version, &diagnosis)?; if !accepted { bail!( "{}", @@ -152,16 +145,14 @@ pub fn execute(args: &TryArgs, root: &Path) -> Result<()> { } if args.no_activate { - println!( - "Installed '{package_id}' (not activated). Run `numan activate {package_id} --yes`." - ); + println!("Installed '{package_id}' (not activated). Run `numan activate {package_id}`."); return Ok(()); } activate::execute( &ActivateArgs { packages: vec![package_id.clone()], - yes: true, + verbose: false, list: false, check: false, @@ -442,7 +433,7 @@ mod tests { } #[test] - fn offer_managed_nu_pin_yes_refuses_silent_switch() { + fn offer_managed_nu_pin_non_interactive_refuses_silent_switch() { let diagnosis = PackageIncompatibility { suggested_pin: Some("0.113.1".to_string()), issue: Incompatibility::NuTooNew { @@ -451,11 +442,17 @@ mod tests { available_versions: vec!["1.0.0".to_string()], }; let root = tempfile::tempdir().unwrap(); - let accepted = - nu_pin_offer::offer_managed_nu_pin(root.path(), "0.114.1", &diagnosis, true).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( + root.path(), + "0.114.1", + &diagnosis, + Some(false), + ) + .unwrap(); assert!( !accepted, - "--yes must not silent-switch / auto-install managed Nu" + "non-interactive mode must not auto-install managed Nu" ); } diff --git a/src/cmd/update.rs b/src/cmd/update.rs index 443d0b5d..5bda4785 100644 --- a/src/cmd/update.rs +++ b/src/cmd/update.rs @@ -312,7 +312,7 @@ pub fn execute_with_hooks( update.package_id, e ); eprintln!( - "Package was upgraded but is inactive. Run `numan activate {} --yes`.", + "Package was upgraded but is inactive. Run `numan activate {}`.", update.package_id ); // Leave LockfileUpdated journal + activate journal for recovery. @@ -345,7 +345,7 @@ pub fn execute_with_hooks( update.package_id, restore_err ); eprintln!( - "Package may be inactive. Run `numan activate {} --yes` or `numan gc`.", + "Package may be inactive. Run `numan activate {}` or `numan gc`.", update.package_id ); // Leave journals for recovery. @@ -391,7 +391,7 @@ fn warn_stale_lifecycle_journal(root: &Path) -> Result<()> { journal.package_id ); eprintln!( - "Run `numan update` to resume reactivation, or `numan activate {} --yes`.", + "Run `numan update` to resume reactivation, or `numan activate {}`.", journal.package_id ); return Ok(()); @@ -454,7 +454,7 @@ fn resume_interrupted_reactivate(root: &Path, hooks: &UpdateHooks<'_>) -> Result format!( "Failed to resume reactivation of '{}'. {}", journal.package_id, - hints::run(&format!("numan activate {} --yes", journal.package_id)) + hints::run(&format!("numan activate {}", journal.package_id)) ) })?; PendingLifecycle::clear(root)?; diff --git a/src/state/snapshot.rs b/src/state/snapshot.rs index e68e1235..93edcf5b 100644 --- a/src/state/snapshot.rs +++ b/src/state/snapshot.rs @@ -73,6 +73,10 @@ pub enum SnapshotTrigger { NupmImport, NupmImportManifest, Rollback, + /// `numan doctor` default repair pass. + Doctor, + /// `numan init --refresh` (paths / activation identity rewrite). + Init, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] diff --git a/src/util/hints.rs b/src/util/hints.rs index e36ee074..1f4c2371 100644 --- a/src/util/hints.rs +++ b/src/util/hints.rs @@ -15,8 +15,11 @@ pub const CMD_ACTIVATE_CHECK: &str = "numan activate --check"; /// `numan registry sync` pub const CMD_REGISTRY_SYNC: &str = "numan registry sync"; -/// `numan doctor --fix` -pub const CMD_DOCTOR_FIX: &str = "numan doctor --fix"; +/// `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 setup nu` pub const CMD_SETUP_NU: &str = "numan setup nu"; @@ -121,7 +124,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_FIX + CMD_DOCTOR } else { CMD_INIT } @@ -299,6 +302,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_FIX); + assert_eq!(registry_none_fix(dir.path()), CMD_DOCTOR); } } diff --git a/src/util/mod.rs b/src/util/mod.rs index d0eb8fc7..256ff78f 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -2,6 +2,7 @@ pub mod atomic; pub mod confirm; pub mod fs_safety; pub mod hints; +pub mod stdio_redirect; pub fn format_timestamp() -> String { format!( diff --git a/src/util/stdio_redirect.rs b/src/util/stdio_redirect.rs new file mode 100644 index 00000000..3defa93e --- /dev/null +++ b/src/util/stdio_redirect.rs @@ -0,0 +1,223 @@ +//! Temporarily redirect stdout to stderr so JSON emitters stay parseable. + +use std::io::{self, Write}; +use std::marker::PhantomData; + +/// RAII guard that redirects process stdout to stderr for nested CLI chatter. +/// +/// This type is intentionally `!Send + !Sync`: it captures and restores +/// process-global stdio state via `dup`/`dup2` (Unix) or `SetStdHandle` +/// (Windows), which is not safe to use from multiple threads concurrently. +/// +/// The redirection is process-global: while the guard is alive, stdout writes +/// from every thread in the process follow the redirected descriptor/handle. +pub struct StdoutToStderr { + #[cfg(unix)] + saved_fd: i32, + #[cfg(windows)] + saved_handle: *mut core::ffi::c_void, + /// Makes the type `!Send + !Sync` on every platform. `*const ()` does not + /// implement those auto-traits; needed on Unix where the saved fd alone + /// would otherwise be `Send + Sync`. + _not_send_sync: PhantomData<*const ()>, +} + +#[cfg(windows)] +mod win_stdio { + pub const STD_OUTPUT_HANDLE: u32 = 0xFFFFFFF5; + pub const STD_ERROR_HANDLE: u32 = 0xFFFFFFF4; + pub const INVALID_HANDLE_VALUE: isize = -1; + + #[allow(non_snake_case)] + #[link(name = "kernel32")] + unsafe extern "system" { + pub fn GetStdHandle(n_std_handle: u32) -> *mut core::ffi::c_void; + pub fn SetStdHandle(n_std_handle: u32, h_handle: *mut core::ffi::c_void) -> i32; + } +} + +impl StdoutToStderr { + /// Redirect process stdout to stderr until this guard is dropped. + pub fn redirect() -> io::Result { + let _ = io::stdout().flush(); + let _ = io::stderr().flush(); + + #[cfg(unix)] + { + // SAFETY: dup/dup2 operate on the standard stdio descriptors. + unsafe { + let saved_fd = libc::dup(libc::STDOUT_FILENO); + if saved_fd < 0 { + return Err(io::Error::last_os_error()); + } + if libc::dup2(libc::STDERR_FILENO, libc::STDOUT_FILENO) < 0 { + let err = io::Error::last_os_error(); + let _ = libc::close(saved_fd); + return Err(err); + } + Ok(Self { + saved_fd, + _not_send_sync: PhantomData, + }) + } + } + + #[cfg(windows)] + { + use win_stdio::*; + + // SAFETY: Win32 stdio handle swap for the current process only. + // Save the existing GetStdHandle value and point STD_OUTPUT_HANDLE at + // stderr. SetStdHandle does not close the previous handle, so the + // original remains valid for restore without DuplicateHandle. + unsafe { + let stdout = GetStdHandle(STD_OUTPUT_HANDLE); + let stderr = GetStdHandle(STD_ERROR_HANDLE); + if stdout as isize == INVALID_HANDLE_VALUE + || stdout.is_null() + || stderr as isize == INVALID_HANDLE_VALUE + || stderr.is_null() + { + return Err(io::Error::last_os_error()); + } + + if SetStdHandle(STD_OUTPUT_HANDLE, stderr) == 0 { + return Err(io::Error::last_os_error()); + } + + Ok(Self { + saved_handle: stdout, + _not_send_sync: PhantomData, + }) + } + } + + #[cfg(not(any(unix, windows)))] + { + Ok(Self { + _not_send_sync: PhantomData, + }) + } + } +} + +impl Drop for StdoutToStderr { + fn drop(&mut self) { + let _ = io::stdout().flush(); + + #[cfg(unix)] + // SAFETY: restore the saved stdout descriptor captured in `redirect`. + unsafe { + let _ = libc::dup2(self.saved_fd, libc::STDOUT_FILENO); + let _ = libc::close(self.saved_fd); + } + + #[cfg(windows)] + { + use win_stdio::{SetStdHandle, STD_OUTPUT_HANDLE}; + + // SAFETY: restore the original stdout handle captured in `redirect`. + // Do not CloseHandle it: it is the process's prior standard handle. + unsafe { + let _ = SetStdHandle(STD_OUTPUT_HANDLE, self.saved_handle); + } + } + } +} + +#[cfg(all(test, unix))] +mod tests { + use super::StdoutToStderr; + + /// Exercise process-global FD remounts in a forked child so parallel lib + /// tests cannot observe or race on the parent's stdout/stderr descriptors. + #[test] + fn redirect_sends_stdout_writes_to_stderr_pipe_then_restores() { + use std::io::Write; + + // Own both stdio locks on the forking thread. A fork copies only this + // thread, so an inherited lock held by another thread would deadlock + // the child inside `redirect()` (which flushes stdout/stderr). + let mut out_lock = std::io::stdout().lock(); + let mut err_lock = std::io::stderr().lock(); + let _ = out_lock.flush(); + let _ = err_lock.flush(); + + // SAFETY: fork isolates FD mutations; child calls `_exit` and never returns. + unsafe { + let pid = libc::fork(); + assert!(pid >= 0, "fork failed: {}", std::io::Error::last_os_error()); + if pid == 0 { + let code = match run_redirect_pipe_assertions() { + Ok(()) => 0, + Err(_) => 1, + }; + libc::_exit(code); + } + + let mut status = 0; + assert_eq!(libc::waitpid(pid, &mut status, 0), pid); + assert!( + libc::WIFEXITED(status) && libc::WEXITSTATUS(status) == 0, + "child redirect assertions failed (status={status})" + ); + } + } + + fn run_redirect_pipe_assertions() -> Result<(), ()> { + // SAFETY: runs only in the forked child from the test above. + unsafe { + let mut stdout_pipe = [0; 2]; + let mut stderr_pipe = [0; 2]; + if libc::pipe(stdout_pipe.as_mut_ptr()) != 0 + || libc::pipe(stderr_pipe.as_mut_ptr()) != 0 + { + return Err(()); + } + + if libc::dup2(stdout_pipe[1], libc::STDOUT_FILENO) < 0 + || libc::dup2(stderr_pipe[1], libc::STDERR_FILENO) < 0 + { + return Err(()); + } + let _ = libc::close(stdout_pipe[1]); + let _ = libc::close(stderr_pipe[1]); + + { + let guard = StdoutToStderr::redirect().map_err(|_| ())?; + let msg = b"during-redirect\n"; + if libc::write(libc::STDOUT_FILENO, msg.as_ptr().cast(), msg.len()) + != msg.len() as isize + { + return Err(()); + } + drop(guard); + } + + let restore_msg = b"after-restore\n"; + if libc::write( + libc::STDOUT_FILENO, + restore_msg.as_ptr().cast(), + restore_msg.len(), + ) != restore_msg.len() as isize + { + return Err(()); + } + + let mut err_buf = [0u8; 64]; + let err_n = libc::read(stderr_pipe[0], err_buf.as_mut_ptr().cast(), err_buf.len()); + let mut out_buf = [0u8; 64]; + let out_n = libc::read(stdout_pipe[0], out_buf.as_mut_ptr().cast(), out_buf.len()); + let _ = libc::close(stdout_pipe[0]); + let _ = libc::close(stderr_pipe[0]); + + if err_n <= 0 || &err_buf[..err_n as usize] != b"during-redirect\n" { + return Err(()); + } + if out_n <= 0 || &out_buf[..out_n as usize] != b"after-restore\n" { + return Err(()); + } + Ok(()) + } + } +} diff --git a/tests/activate_test.rs b/tests/activate_test.rs index 5368e504..595c9407 100644 --- a/tests/activate_test.rs +++ b/tests/activate_test.rs @@ -22,7 +22,6 @@ use numan_cli::core::integrity; use numan_cli::nu::paths::NuPaths; use numan_cli::state::journal::{PendingActivation, PendingActivationEntry, PendingStatus}; use numan_cli::state::lockfile::{Lockfile, LockfileEntry, PluginActivation}; -use std::io::IsTerminal; use std::path::PathBuf; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; @@ -137,7 +136,6 @@ impl TestEnv { fn no_args(&self) -> ActivateArgs { ActivateArgs { packages: vec![], - yes: true, verbose: false, list: false, check: false, @@ -147,7 +145,6 @@ impl TestEnv { fn args_for(&self, packages: &[&str]) -> ActivateArgs { ActivateArgs { packages: packages.iter().map(|s| s.to_string()).collect(), - yes: true, verbose: false, list: false, check: false, @@ -352,7 +349,6 @@ fn test_activate_path_with_spaces() { let args = ActivateArgs { packages: vec![], - yes: true, verbose: false, list: false, check: false, @@ -609,12 +605,9 @@ fn test_activate_stale_journal_requires_refresh() { } #[test] -fn test_activate_auto_confirms_on_no_tty() { - // With the shared confirm utility, non-TTY sessions auto-confirm instead of - // bailing. Without --yes and no TTY, activate should proceed. The TTY - // branches of `confirm_or_auto` are covered by the unconditional unit tests - // in `src/util/confirm.rs`; this end-to-end path now runs everywhere. - +fn test_activate_succeeds_without_prompt() { + // Activate prints an informational consent table and proceeds without a + // prompt (no `--yes` flag). This covers the no-input activation path. let env = TestEnv::new(); env.write_nu_paths(); env.create_plugin_binary("owner", "guarded", "1.0.0"); @@ -628,7 +621,6 @@ fn test_activate_auto_confirms_on_no_tty() { let args = ActivateArgs { packages: vec![], - yes: false, verbose: false, list: false, check: false, @@ -636,7 +628,7 @@ fn test_activate_auto_confirms_on_no_tty() { let result = execute_with_registrar(&args, &env.root(), &ok_registrar); assert!( result.is_ok(), - "non-TTY should auto-confirm, got error: {:?}", + "activation should succeed without a prompt, got error: {:?}", result.err() ); } diff --git a/tests/doctor_test.rs b/tests/doctor_test.rs index 1f1b960e..67806f8b 100644 --- a/tests/doctor_test.rs +++ b/tests/doctor_test.rs @@ -43,7 +43,11 @@ fn nu_setup_repair_test( args.use_existing.is_none(), "doctor must not use the deprecated flag" ); - assert!(args.yes); + // Doctor must not auto-approve consented wipe of a managed install. + assert!( + !args.yes, + "doctor found_off_path repair must not pass --yes" + ); *TEST_NU_SETUP_CALLED.lock().unwrap() = true; Ok(()) } @@ -98,8 +102,7 @@ fn doctor_report_only_leaves_root_unchanged() { std::fs::create_dir_all(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -118,8 +121,7 @@ fn doctor_fix_auto_creates_layout_without_network() { std::fs::write(&nu_exe, b"nu").unwrap(); let args = DoctorArgs { - fix: true, - yes: true, + scan: false, json: false, nupm_home: None, }; @@ -164,8 +166,7 @@ fn doctor_reports_pending_plugin_journal() { journal.save(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -195,8 +196,7 @@ fn doctor_detects_nu_path_drift() { paths.save(root).unwrap(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -221,8 +221,7 @@ fn doctor_reports_off_path_nu_without_download() { let _path_guard = TEST_PATH_GUARD.lock().unwrap(); let _cleared_path = ClearedPath::new(); let args = DoctorArgs { - fix: false, - yes: false, + scan: true, json: false, nupm_home: None, }; @@ -267,8 +266,7 @@ fn doctor_fix_registers_off_path_nu_without_network() { let _path_guard = TEST_PATH_GUARD.lock().unwrap(); let _cleared_path = ClearedPath::new(); let args = DoctorArgs { - fix: true, - yes: true, + scan: false, json: false, nupm_home: None, }; @@ -289,7 +287,7 @@ fn doctor_fix_registers_off_path_nu_without_network() { } fn fake_deactivate_repair(args: &DeactivateArgs, root: &Path) -> anyhow::Result<()> { - assert!(args.yes); + assert!(!args.verbose); assert_eq!(args.packages, vec!["owner/plugin".to_string()]); *TEST_DEACTIVATE_REPAIR_CALLED.lock().unwrap() = true; if *TEST_DEACTIVATE_REPAIR_SHOULD_FAIL.lock().unwrap() { @@ -333,8 +331,7 @@ fn doctor_fix_reconciles_pending_plugin_deactivate_journal() { *TEST_DEACTIVATE_REPAIR_SHOULD_FAIL.lock().unwrap() = false; let args = DoctorArgs { - fix: true, - yes: true, + scan: false, json: false, nupm_home: None, }; @@ -370,8 +367,7 @@ fn doctor_fix_stale_plugin_deactivate_runs_refresh_then_deactivate() { *TEST_DEACTIVATE_REPAIR_SHOULD_FAIL.lock().unwrap() = false; let args = DoctorArgs { - fix: true, - yes: true, + scan: false, json: false, nupm_home: None, }; @@ -405,8 +401,7 @@ fn doctor_fix_reports_deactivate_repair_failure() { *TEST_DEACTIVATE_REPAIR_SHOULD_FAIL.lock().unwrap() = true; let args = DoctorArgs { - fix: true, - yes: true, + scan: false, json: true, nupm_home: None, }; @@ -432,11 +427,18 @@ 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() } } @@ -452,8 +454,7 @@ fn doctor_reports_path_nu_not_found_when_path_cleared() { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -496,8 +497,7 @@ fn doctor_reports_managed_and_trust_root_findings() { let report = run_checks_with_options( &DoctorArgs { - fix: false, - yes: false, + scan: true, json: true, nupm_home: None, }, @@ -535,3 +535,95 @@ fn doctor_reports_managed_and_trust_root_findings() { assert!(json.contains("nu.managed.version")); 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(); + + // 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", + root.to_str().expect("temp root is utf-8"), + "doctor", + "--json", + ]) + .env("NUMAN_ALLOW_UNSIGNED", "1") + .stdin(std::process::Stdio::null()) + .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 + ); + 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}" + ) + }); + assert!( + value.get("repairs").is_some_and(|r| r.is_array()), + "default doctor --json must include repairs: {value}" + ); +} + +/// `doctor --json --scan` must omit the `repairs` field (single JSON object). +#[test] +fn doctor_json_scan_omits_repairs_field() { + let dir = TempDir::new().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root).unwrap(); + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_numan")) + .args([ + "--root", + root.to_str().expect("temp root is utf-8"), + "doctor", + "--json", + "--scan", + ]) + .env("NUMAN_ALLOW_UNSIGNED", "1") + .stdin(std::process::Stdio::null()) + .output() + .expect("run numan doctor --json --scan"); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success() + || output.status.code() == Some(1) + || output.status.code() == Some(2), + "doctor --json --scan unexpected status: {}\nstdout={stdout}\nstderr={stderr}", + output.status + ); + let value: serde_json::Value = serde_json::from_str(&stdout).unwrap_or_else(|e| { + panic!( + "doctor --json --scan stdout must be valid JSON: {e}\nstdout={stdout}\nstderr={stderr}" + ) + }); + assert!( + value.is_object(), + "doctor --json --scan must emit a single JSON object: {value}" + ); + assert!( + value.get("repairs").is_none(), + "doctor --json --scan must omit repairs: {value}" + ); + assert!( + value.get("findings").is_some_and(|f| f.is_array()), + "doctor --json --scan must include findings: {value}" + ); +} diff --git a/tests/module_autoload_test.rs b/tests/module_autoload_test.rs index 4d442314..479f946e 100644 --- a/tests/module_autoload_test.rs +++ b/tests/module_autoload_test.rs @@ -167,22 +167,20 @@ impl ModuleTestEnv { lockfile.save(self.root()).unwrap(); } - /// Build ActivateArgs for the given package IDs with --yes. + /// Build ActivateArgs for the given package IDs. fn activate_args(&self, packages: &[&str]) -> ActivateArgs { ActivateArgs { packages: packages.iter().map(|s| s.to_string()).collect(), - yes: true, verbose: false, list: false, check: false, } } - /// Build DeactivateArgs for the given package IDs with --yes. + /// Build DeactivateArgs for the given package IDs. fn deactivate_args(&self, packages: &[&str]) -> DeactivateArgs { DeactivateArgs { packages: packages.iter().map(|s| s.to_string()).collect(), - yes: true, verbose: false, } } diff --git a/tests/nupm_compat_test.rs b/tests/nupm_compat_test.rs index 2d8417c3..a8449bf8 100644 --- a/tests/nupm_compat_test.rs +++ b/tests/nupm_compat_test.rs @@ -541,7 +541,6 @@ fn t24_import_then_activate_nupm_module() { let args = ActivateArgs { packages: vec!["test/minimal".to_string()], - yes: true, verbose: false, list: false, check: false, diff --git a/tests/plugin_active_update_real_nu.rs b/tests/plugin_active_update_real_nu.rs index b804a118..f6a380b3 100644 --- a/tests/plugin_active_update_real_nu.rs +++ b/tests/plugin_active_update_real_nu.rs @@ -175,10 +175,7 @@ fn real_nu_active_update_resume_lockfile_updated_reactivates() -> Result<()> { run.prepare_active_v1()?; // Deactivate + install v2 without reactivate, then seed needs_reactivate journal. - run.require_ok( - &["deactivate", &run.package_id, "--yes"], - Duration::from_secs(120), - )?; + run.require_ok(&["deactivate", &run.package_id], Duration::from_secs(120))?; let install_spec = format!("{}@{TO_VERSION}", run.package_id); run.require_ok(&["install", &install_spec], Duration::from_secs(300))?; diff --git a/tests/support/acceptance/model.rs b/tests/support/acceptance/model.rs index 2a5254b6..990eceae 100644 --- a/tests/support/acceptance/model.rs +++ b/tests/support/acceptance/model.rs @@ -90,18 +90,14 @@ impl StepName { Self::Search => vec!["search".to_string(), config.query.clone()], Self::Info => vec!["info".to_string(), config.package_id.clone()], Self::Install => vec!["install".to_string(), config.package_id.clone()], - Self::Activate => vec![ - "activate".to_string(), - config.package_id.clone(), - "--yes".to_string(), + Self::Activate => vec!["activate".to_string(), config.package_id.clone()], + Self::Doctor => vec![ + "doctor".to_string(), + "--scan".to_string(), + "--json".to_string(), ], - Self::Doctor => vec!["doctor".to_string(), "--json".to_string()], Self::List => vec!["list".to_string()], - Self::Deactivate => vec![ - "deactivate".to_string(), - config.package_id.clone(), - "--yes".to_string(), - ], + Self::Deactivate => vec!["deactivate".to_string(), config.package_id.clone()], Self::Remove => vec!["remove".to_string(), config.package_id.clone()], Self::Gc => vec!["gc".to_string()], Self::Preflight => panic!("preflight has no numan command arguments"), diff --git a/tests/support/active_update/mod.rs b/tests/support/active_update/mod.rs index fb869d19..1eed98d9 100644 --- a/tests/support/active_update/mod.rs +++ b/tests/support/active_update/mod.rs @@ -243,10 +243,7 @@ impl ActiveUpdateRun { self.plant_fixture_registry()?; let install_spec = format!("{}@{FROM_VERSION}", self.package_id); self.require_ok(&["install", &install_spec], Duration::from_secs(300))?; - self.require_ok( - &["activate", &self.package_id, "--yes"], - Duration::from_secs(120), - )?; + self.require_ok(&["activate", &self.package_id], Duration::from_secs(120))?; let lockfile = Lockfile::load(&self.root)?; let entry = lockfile .packages