refactor: UX improvements — reduce prompts and simplify doctor - #68
Conversation
- doctor: removed --fix and --yes flags, added --scan for report-only mode - Default behavior now applies fixes automatically - --scan flag shows issues without applying fixes - Kept --json for structured output - install: removed --yes flag (no prompt for routine install) - activate: removed --yes flag (no prompt for reversible operation) - deactivate: removed --yes flag (no prompt for reversible operation) - try: removed --yes flag (no prompt for starter install) - nu_pin_offer: removed auto_yes parameter from offer_managed_nu_pin() - Non-interactive mode prints hints and returns false - Interactive mode still prompts for significant Nu version switch All 414 tests pass, clippy clean, fmt applied. Note: setup nu and setup loader still have --yes flags (not addressed in this PR due to complexity of overwrite scenarios). Future work could simplify these too.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe CLI removes ChangesConfirmation removal across command flows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
PR Summary by QodoUX: simplify doctor mode and remove unnecessary confirmation flags
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
There was a problem hiding this comment.
🟡 Not ready to approve
It still produces/validates doctor fix hints that reference the removed --fix flag and the non-interactive Nu switch hint omits --yes, which can mislead or break non-interactive workflows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR streamlines CLI UX by removing unnecessary confirmation flags/prompts and simplifying doctor into two modes: default “apply fixes” vs --scan report-only, aligning with Numan’s goal of reducing friction for routine/reversible operations.
Changes:
- Simplifies
numan doctorby removing--fix/--yes, adding--scan, and making repairs the default behavior. - Removes
--yesfrominstall,try,activate, anddeactivate, and drops the “Proceed?” prompt while keeping informational consent tables. - Updates Nu pin offer flow to never auto-install in non-interactive sessions and to require explicit interactive confirmation.
File summaries
| File | Description |
|---|---|
| tests/doctor_test.rs | Updates DoctorArgs usage to the new scan mode. |
| src/cmd/try_cmd.rs | Removes --yes path and updates Nu pin offer + activation call sites accordingly. |
| src/cmd/nu_pin_offer.rs | Removes auto_yes and makes non-interactive sessions hint-only (no auto-download). |
| src/cmd/install.rs | Removes --yes and always uses interactive Nu pin offer behavior when applicable. |
| src/cmd/doctor.rs | Implements --scan and default-repair behavior; removes --yes plumbing. |
| src/cmd/deactivate.rs | Removes --yes and proceeds without prompting after showing consent table. |
| src/cmd/activate.rs | Removes --yes and proceeds without prompting after showing consent table; updates clap conflicts. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Greptile SummaryThis PR inverts the default doctor mode (repairs run without flags;
Confidence Score: 5/5Safe to merge. All previously reported blocking issues are addressed, the confirm-tier logic inversion is intentional and correct, and the new StdoutToStderr guard is well-implemented with a fork-isolated test. Every changed code path has tests, including the new confirm-gate seam, the off-path repair regression guard, and the JSON stdout-cleanliness integration tests. The only finding is a cosmetic no-op fallback in a niche platform branch of the stdio redirect utility. Files Needing Attention: src/util/stdio_redirect.rs — the non-Unix/non-Windows fallback silently skips the redirect; acceptable for now but worth a comment.
|
| Filename | Overview |
|---|---|
| src/cmd/doctor.rs | Major refactor: --fix/--yes flags replaced by --scan (report-only); confirm-tier now gates on TTY presence instead of --yes; confirm_repairs injectable seam added to DoctorOptions; pre-mutation snapshot created before any repair; stdout redirected to stderr during JSON repair passes. Previously flagged dead-function/dead-branch issue resolved. |
| src/util/stdio_redirect.rs | New file: RAII StdoutToStderr guard using dup/dup2 on Unix and SetStdHandle on Windows. Intentionally !Send + !Sync. Has a no-op fallback for non-Unix/non-Windows platforms (silently leaves JSON output unguarded on those targets). Fork-based unit test avoids corrupting parent process stdio. |
| src/cmd/nu_pin_offer.rs | Removed auto_yes parameter; split into offer_managed_nu_pin (public) and offer_managed_nu_pin_with_tty (testing seam). Non-interactive path now always prints hints and returns false, consistent with updated semantics. |
| src/cmd/activate.rs | Removed yes field from ActivateArgs and removed confirm_or_bail call; consent table is now informational only. conflicts_with_all on list simplified to conflicts_with after yes removal. |
| src/cmd/deactivate.rs | Removed yes field from DeactivateArgs and removed confirm_or_bail call; reclassify_confirmed_targets renamed to reclassify_targets. Tests and error message updated consistently. |
| src/cmd/init.rs | init --refresh now always acquires the mutation lock (fixes a race window when no packages were active) and creates a PreMutation/Init snapshot. New test refresh_without_active_packages_still_locks_and_snapshots verifies this behaviour. |
| tests/doctor_test.rs | All fix/yes fields updated to scan: bool; stale assert!(args.yes) assertions corrected; test_doctor_options() now injects confirm_repairs_always so confirm-tier exercises run in CI; two new process-spawn integration tests verify JSON stdout cleanliness. |
| src/state/snapshot.rs | Two new SnapshotTrigger variants: Doctor and Init, used by the new pre-mutation snapshots in doctor repairs and init --refresh. |
| src/cmd/try_cmd.rs | Removed --yes flag from TryArgs; stale --yes hint in the --no-activate message replaced with the correct numan activate {package_id} (fixes the previously-flagged CLI error). |
| src/cmd/update.rs | Four stale numan activate {} --yes hints replaced with numan activate {}, fixing the previously-flagged CLI errors users would hit when following error-recovery instructions. |
Reviews (13): Last reviewed commit: "fix: harden doctor off-path skip test an..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52ad910c58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- Remove stale DeactivateArgs yes fields - Remove stale ActivateArgs yes fields
- Remove redundant prompting comments - Clarify concurrent deactivation wording
Qodo Fixer✅ Committed (6) · ☑ Fixed (6) Commits pushed directly to this PR — no separate fix PR opened. Process — 6 fixed
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…n-interactive hint
|
@coderabbitai review |
…n-interactive hint
Fixed 7 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Replace a capturing closure with a Mutex-backed fn pointer for discover_off_path, add a DoctorOptions confirm_repairs test seam so non-TTY CI can exercise confirm-tier repairs, and align the off-PATH setup helper with yes=false. Co-authored-by: Anthony Thompson <github@trackdub.com>
Clarify doctor --json repairs docs and --help for --scan, force the confirm_repairs seam in the off-PATH skip unit test, drop dead try --yes, remove deactivate --yes from ignored active-update acceptance, and update the activate no-prompt test comments. Co-authored-by: Anthony Thompson <github@trackdub.com>
Redirect nested repair println! chatter to stderr during --json repair passes via StdoutToStderr. Rename CMD_DOCTOR_FIX to CMD_DOCTOR (keep alias) and add a subprocess regression test for parseable JSON stdout. Co-authored-by: Anthony Thompson <github@trackdub.com>
b7045c5 to
6d231f2
Compare
…thompson/numan into refactor/ux-improvements
708b24e
Bring std::io::IsTerminal into scope for doctor confirm_repairs so stdin().is_terminal() compiles, and drop the blank line in TryArgs. Co-authored-by: Anthony Thompson <github@trackdub.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/numan-doctor.md`:
- Around line 265-270: Fix the compilation error in src/cmd/doctor.rs by
bringing the std::io::IsTerminal trait into scope before calling
stdin().is_terminal(). Then rerun the required Format, Clippy, MSRV, Package,
and Ubuntu checks, and leave the definition-of-done checklist incomplete until
they pass.
- Line 31: Add a regression test in tests/doctor_test.rs covering the doctor
--json --scan invocation, parse the output as JSON, and assert that the repairs
field is omitted while preserving the documented single-object JSON contract.
In `@src/cmd/doctor.rs`:
- Around line 1023-1026: Replace the `confirm_repairs` TTY-only gate with real
confirmation for confirm-tier repairs, preserving `options.confirm_repairs` as
the override hook and ensuring direct mutations such as `nu_paths.refresh`,
`activation.reconcile`, and `plugin_deactivate.reconcile` do not run without
approval. Alternatively, if retaining TTY detection, rename the gate, report the
`not_confirmed` reason and remediation in the plain-text output near the
existing counts, and update `docs/numan-doctor.md` plus command help to document
that repairs are enabled by default only in interactive sessions.
- Around line 988-993: Add the std::io::IsTerminal trait import alongside the
existing std::io::Write import so confirm_repairs can resolve
stdin().is_terminal() and the crate compiles.
In `@src/cmd/try_cmd.rs`:
- Around line 446-456: Rename the test containing the shown
offer_managed_nu_pin_with_tty call to
offer_managed_nu_pin_non_interactive_refuses_silent_switch, replacing the
outdated name that includes “yes”; leave the test behavior unchanged.
In `@tests/doctor_test.rs`:
- Around line 549-570: Harden the doctor JSON test around the spawned `numan`
command: provide null stdin to make the confirmation path deterministic, disable
network-dependent repairs using the available CLI or environment controls (or
mark the test ignored for the acceptance suite if none exist), and assert that
`output.status` is successful before parsing stdout. Keep the existing JSON and
repairs assertions for successful, offline execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d09f4785-6f7f-4782-8382-4184559238b8
📒 Files selected for processing (15)
AGENTS.mdCargo.tomlREADME.mddocs/numan-doctor.mdsrc/cli.rssrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/util/hints.rssrc/util/mod.rssrc/util/stdio_redirect.rstests/activate_test.rstests/doctor_test.rstests/plugin_active_update_real_nu.rstests/support/acceptance/model.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (rust)
⚠️ CI failures not shown inline (4)
GitHub Actions: CI / Test (windows-latest): refactor: UX improvements — reduce prompts and simplify doctor
Conclusion: failure
##[group]Run rustup toolchain install stable --profile minimal --no-self-update
�[36;1mrustup toolchain install stable --profile minimal --no-self-update�[0m
shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}
env:
CARGO_TERM_COLOR: always
CARGO_HOME: C:\Users\runneradmin\.cargo
RUSTUP_PERMIT_COPY_RENAME: 1
##[endgroup]
info: syncing channel updates for stable-x86_64-pc-windows-msvc
info: latest update on 2026-07-16 for version 1.97.1 (8bab26f4f 2026-07-14)
info: removing previous version of component clippy
info: removing previous version of component rustfmt
info: removing previous version of component rust-std for target x86_64-pc-windows-gnu
info: removing previous version of component rust-std for target i686-pc-windows-msvc
info: removing previous version of component cargo
info: removing previous version of component rust-std
info: removing previous version of component rustc
info: downloading 7 components
stable-x86_64-pc-windows-msvc updated - rustc 1.97.1 (8bab26f4f 2026-07-14) (from rustc 1.97.0 (2d8144b78 2026-07-07))
##[error]The operation was canceled.
GitHub Actions: CI / Real-Nu acceptance (windows-latest): refactor: UX improvements — reduce prompts and simplify doctor
Conclusion: failure
##[group]Cache Configuration
Cache Provider:
github
Workspaces:
D:\a\numan\numan
Cache Paths:
C:\Users\runneradmin\.cargo\bin
C:\Users\runneradmin\.cargo\.crates.toml
C:\Users\runneradmin\.cargo\.crates2.json
C:\Users\runneradmin\.cargo\registry
C:\Users\runneradmin\.cargo\git
D:\a\numan\numan\target
Restore Key:
v0-rust-real-nu-acceptance-Windows_NT-x64-8af1e26a
Cache Key:
v0-rust-real-nu-acceptance-Windows_NT-x64-8af1e26a-ee16a705
.. Prefix:
- v0-rust-real-nu-acceptance-Windows_NT-x64
.. Environment considered:
- Rust Versions:
- 1.97.1 x86_64-pc-windows-msvc 8bab26f4f68e0e26f0bb7960be334d5b520ea452
- 1.97.1 x86_64-pc-windows-msvc 8bab26f4f68e0e26f0bb7960be334d5b520ea452
- CARGO_HOME
- CARGO_INCREMENTAL
- CARGO_TERM_COLOR
.. Lockfiles considered:
- D:\a\numan\numan\Cargo.lock
- D:\a\numan\numan\Cargo.toml
##[endgroup]
... Restoring cache ...
Cache hit for restore-key: v0-rust-real-nu-acceptance-Windows_NT-x64-8af1e26a-6c16b325
Received 192937984 of 231791840 (83.2%), 181.5 MBs/sec
Received 231791840 of 231791840 (100.0%), 178.3 MBs/sec
Cache Size: ~221 MB (231791840 B)
[command]"C:\Program Files\Git\usr\bin\tar.exe" -xf D:/a/_temp/d61898f6-1e7a-462d-b7f5-2b77df284772/cache.tzst -P -C D:/a/numan/numan --force-local --use-compress-program "zstd -d"
##[error]The operation was canceled.
GitHub Actions: CI / 3_Test (windows-latest).txt: refactor: UX improvements — reduce prompts and simplify doctor
Conclusion: failure
##[group]Run rustup toolchain install stable --profile minimal --no-self-update
�[36;1mrustup toolchain install stable --profile minimal --no-self-update�[0m
shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}
env:
CARGO_TERM_COLOR: always
CARGO_HOME: C:\Users\runneradmin\.cargo
RUSTUP_PERMIT_COPY_RENAME: 1
##[endgroup]
info: syncing channel updates for stable-x86_64-pc-windows-msvc
info: latest update on 2026-07-16 for version 1.97.1 (8bab26f4f 2026-07-14)
info: removing previous version of component clippy
info: removing previous version of component rustfmt
info: removing previous version of component rust-std for target x86_64-pc-windows-gnu
info: removing previous version of component rust-std for target i686-pc-windows-msvc
info: removing previous version of component cargo
info: removing previous version of component rust-std
info: removing previous version of component rustc
info: downloading 7 components
stable-x86_64-pc-windows-msvc updated - rustc 1.97.1 (8bab26f4f 2026-07-14) (from rustc 1.97.0 (2d8144b78 2026-07-07))
##[error]The operation was canceled.
GitHub Actions: CI / 1_Real-Nu acceptance (windows-latest).txt: refactor: UX improvements — reduce prompts and simplify doctor
Conclusion: failure
##[group]Cache Configuration
Cache Provider:
github
Workspaces:
D:\a\numan\numan
Cache Paths:
C:\Users\runneradmin\.cargo\bin
C:\Users\runneradmin\.cargo\.crates.toml
C:\Users\runneradmin\.cargo\.crates2.json
C:\Users\runneradmin\.cargo\registry
C:\Users\runneradmin\.cargo\git
D:\a\numan\numan\target
Restore Key:
v0-rust-real-nu-acceptance-Windows_NT-x64-8af1e26a
Cache Key:
v0-rust-real-nu-acceptance-Windows_NT-x64-8af1e26a-ee16a705
.. Prefix:
- v0-rust-real-nu-acceptance-Windows_NT-x64
.. Environment considered:
- Rust Versions:
- 1.97.1 x86_64-pc-windows-msvc 8bab26f4f68e0e26f0bb7960be334d5b520ea452
- 1.97.1 x86_64-pc-windows-msvc 8bab26f4f68e0e26f0bb7960be334d5b520ea452
- CARGO_HOME
- CARGO_INCREMENTAL
- CARGO_TERM_COLOR
.. Lockfiles considered:
- D:\a\numan\numan\Cargo.lock
- D:\a\numan\numan\Cargo.toml
##[endgroup]
... Restoring cache ...
Cache hit for restore-key: v0-rust-real-nu-acceptance-Windows_NT-x64-8af1e26a-6c16b325
Received 192937984 of 231791840 (83.2%), 181.5 MBs/sec
Received 231791840 of 231791840 (100.0%), 178.3 MBs/sec
Cache Size: ~221 MB (231791840 B)
[command]"C:\Program Files\Git\usr\bin\tar.exe" -xf D:/a/_temp/d61898f6-1e7a-462d-b7f5-2b77df284772/cache.tzst -P -C D:/a/numan/numan --force-local --use-compress-program "zstd -d"
##[error]The operation was canceled.
🧰 Additional context used
📓 Path-based instructions (9)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use
anyhow::Resultwith.context("what failed")for application code,thiserrorfor library types callers match on, and avoid panics in library code by returningResult.
**/*.rs: Use Rust 2021 edition conventions and match existing naming, module layout, and documentation level in edited files.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Error paths must returnanyhow::Resultwith context, and library code must not panic.
Unit tests must use injectable seams such asFakeCandidateRunnerand registrars; do not spawn realnuin unit tests.
Real-Nu acceptance tests should be marked#[ignore]and run separately when activation or nupm-import behavior changes.
Every new mutating code path must acquire the mutation lock and snapshot the lockfile before writes.
Mutating commands, includinginstall,remove,update,gc, andnupm import, must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must be written atomically usingwrite_json_atomic.
numan installmust write only to$NUMAN_ROOTand must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state are not authoritative.
Install payloads must use versioned, content-addressed paths and must never be overwritten in place.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.
The nupm boundary must remain read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.
**/*.rs: Use Rust 2021 edition conventions a...
Files:
src/util/mod.rstests/plugin_active_update_real_nu.rstests/support/acceptance/model.rstests/activate_test.rssrc/util/hints.rssrc/cli.rssrc/cmd/try_cmd.rstests/doctor_test.rssrc/util/stdio_redirect.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,toml}: All changes must pass formatting and linting:cargo fmtandcargo clippy -- -D warnings.
Behavior changes require corresponding tests, including relevant failure paths.
Files:
src/util/mod.rstests/plugin_active_update_real_nu.rsCargo.tomltests/support/acceptance/model.rstests/activate_test.rssrc/util/hints.rssrc/cli.rssrc/cmd/try_cmd.rstests/doctor_test.rssrc/util/stdio_redirect.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when changing structure, conventions, or user-visible behavior, using
AGENTS.md,docs/, or command help as appropriate.
Files:
src/util/mod.rstests/plugin_active_update_real_nu.rsREADME.mdAGENTS.mdtests/support/acceptance/model.rstests/activate_test.rssrc/util/hints.rssrc/cli.rssrc/cmd/try_cmd.rstests/doctor_test.rsdocs/numan-doctor.mdsrc/util/stdio_redirect.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
src/util/mod.rstests/plugin_active_update_real_nu.rsCargo.tomlREADME.mdAGENTS.mdtests/support/acceptance/model.rstests/activate_test.rssrc/util/hints.rssrc/cli.rssrc/cmd/try_cmd.rstests/doctor_test.rsdocs/numan-doctor.mdsrc/util/stdio_redirect.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Pass Nu executable paths and names only through
NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG, andNUMAN_PLUGIN_NAME; the Nu program must be a compile-time constant with no runtime interpolation.
Files:
src/util/mod.rssrc/util/hints.rssrc/cli.rssrc/cmd/try_cmd.rssrc/util/stdio_redirect.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep integration tests under
tests/, unit tests inline with source modules, and use mock platforms for platform-specific behavior.
Files:
tests/plugin_active_update_real_nu.rstests/support/acceptance/model.rstests/activate_test.rstests/doctor_test.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Declare and preserve the project MSRV as Rust 1.88 via
rust-version.
Files:
Cargo.toml
src/{cmd,state,install}/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Call
create_snapshot()before mutations performed by install, update, remove, activate, deactivate, or nupm-import; garbage collection must treat payloads referenced by every snapshot as live roots.
Files:
src/cmd/try_cmd.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
src/cmd/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep clap subcommand handlers thin; delegate domain logic to
core/or installation logic toinstall/.
Files:
src/cmd/try_cmd.rssrc/cmd/nu_pin_offer.rssrc/cmd/doctor.rs
🪛 GitHub Actions: CI / 0_Test (ubuntu-latest).txt
src/cmd/doctor.rs
[error] 992-992: cargo test --verbose failed to compile numan-cli: method is_terminal is not found for std::io::Stdin because the required std::io::IsTerminal or is_terminal_polyfill::IsTerminal trait is not imported. Rust error E0599.
🪛 GitHub Actions: CI / 10_Package.txt
src/cmd/doctor.rs
[error] 992-992: cargo package --locked failed to verify the package: no method named is_terminal found for std::io::Stdin because the required IsTerminal trait is not in scope. Import std::io::IsTerminal or is_terminal_polyfill::IsTerminal.
🪛 GitHub Actions: CI / 11_Format.txt
src/cmd/try_cmd.rs
[error] 20-20: cargo fmt --all -- --check failed because the file is not formatted. Remove the extra blank line before the Install only; do not activate documentation comment.
🪛 GitHub Actions: CI / 4_MSRV (1.88).txt
src/cmd/doctor.rs
[error] 992-992: cargo +1.88 check --locked --all-targets failed: method is_terminal is unavailable for std::io::Stdin because the std::io::IsTerminal trait is not in scope. Import std::io::IsTerminal.
🪛 GitHub Actions: CI / 6_Real-Nu acceptance (macos-latest).txt
src/cmd/doctor.rs
[error] 992-992: cargo test compilation failed: no method named is_terminal found for std::io::Stdin. Import std::io::IsTerminal (or is_terminal_polyfill::IsTerminal) before calling std::io::stdin().is_terminal().
🪛 GitHub Actions: CI / 7_Clippy.txt
src/cmd/doctor.rs
[error] 992-992: cargo clippy -- -D warnings failed to compile: no method named is_terminal found for std::io::Stdin. Import the std::io::IsTerminal trait (or is_terminal_polyfill::IsTerminal) before calling std::io::stdin().is_terminal().
🪛 GitHub Actions: CI / 8_Real-Nu acceptance (ubuntu-latest).txt
src/cmd/doctor.rs
[error] 992-992: Rust compilation failed: no method named is_terminal found for std::io::Stdin because the required std::io::IsTerminal or is_terminal_polyfill::IsTerminal trait is not in scope. The cargo test --verbose -- --ignored --skip acceptance_process_helper --skip stage1_official_registry --skip real_nu_active_update_ command failed with exit code 101.
🪛 GitHub Actions: CI / 9_Test (macos-latest).txt
src/cmd/doctor.rs
[error] 992-992: cargo test --verbose failed to compile numan-cli: no method named is_terminal found for std::io::Stdin (E0599). Import std::io::IsTerminal or is_terminal_polyfill::IsTerminal.
🪛 GitHub Actions: CI / Clippy
src/cmd/doctor.rs
[error] 992-992: cargo clippy -- -D warnings failed: no method named is_terminal found for std::io::Stdin because the required std::io::IsTerminal trait is not in scope. Import std::io::IsTerminal.
🪛 GitHub Actions: CI / Format
src/cmd/try_cmd.rs
[error] 20-20: cargo fmt formatting check failed. Remove the extra blank line before the 'Install only; do not activate' documentation comment, then rerun 'cargo fmt --all -- --check'.
🪛 GitHub Actions: CI / MSRV (1.88)
src/cmd/doctor.rs
[error] 992-992: cargo +1.88 check --locked --all-targets failed: no method named is_terminal found for std::io::Stdin (Rust E0599). Import the std::io::IsTerminal trait.
🪛 GitHub Actions: CI / Package
src/cmd/doctor.rs
[error] 992-992: cargo package --locked failed to verify the package: Rust error E0599, no method named 'is_terminal' found for 'Stdin'. Import the 'std::io::IsTerminal' trait (or the suggested polyfill) before calling std::io::stdin().is_terminal().
🪛 GitHub Actions: CI / Real-Nu acceptance (macos-latest)
src/cmd/doctor.rs
[error] 992-992: cargo test --verbose failed to compile: std::io::stdin().is_terminal() requires the std::io::IsTerminal trait to be imported. Add use std::io::IsTerminal;.
🪛 GitHub Actions: CI / Real-Nu acceptance (ubuntu-latest)
src/cmd/doctor.rs
[error] 992-992: cargo test compilation failed: no method named is_terminal found for std::io::Stdin because the std::io::IsTerminal trait is not in scope. Import std::io::IsTerminal (or is_terminal_polyfill::IsTerminal) to fix this error.
🪛 GitHub Actions: CI / Test (macos-latest)
src/cmd/doctor.rs
[error] 992-992: cargo test --verbose failed to compile numan-cli: no method named 'is_terminal' found for 'std::io::Stdin' because the required trait is not in scope. Import 'std::io::IsTerminal' or 'is_terminal_polyfill::IsTerminal'.
🪛 GitHub Actions: CI / Test (ubuntu-latest)
src/cmd/doctor.rs
[error] 992-992: cargo test --verbose failed to compile numan-cli: no method named is_terminal found for std::io::Stdin (Rust error E0599). Import std::io::IsTerminal or is_terminal_polyfill::IsTerminal.
🔍 Remote MCP GitHub Copilot
Additional review context
- Build is currently broken:
src/cmd/doctor.rscallsstd::io::stdin().is_terminal()but imports onlystd::io::Write; theIsTerminaltrait is not in scope. This likely explains the failed Format, Clippy, MSRV, Package, and Ubuntu test checks. - CI is not green: Ubuntu tests and real-Nu acceptance failed; MSRV, Package, Clippy, and Format failed. Windows/macOS jobs were cancelled, and Rust analysis remained in progress at retrieval time.
doctor --jsonredirects process-global stdout usingdup2on Unix andSetStdHandleon Windows. The implementation explicitly notes this is process-global and unsafe with concurrent use, so this utility should remain confined to the synchronous repair section as currently intended.
🔇 Additional comments (22)
src/cmd/nu_pin_offer.rs (2)
48-49: LGTM!
21-23: 🎯 Functional CorrectnessNo caller still passes
auto_yes. All callers use the updated API.> Likely an incorrect or invalid review comment.src/cmd/try_cmd.rs (1)
22-23: LGTM!Also applies to: 93-93, 149-156
src/cmd/doctor.rs (4)
134-136: LGTM!
1004-1020: LGTM!
1490-1496: LGTM!Also applies to: 1596-1612, 1960-1977, 2229-2284
1096-1100: 🩺 Stability & AvailabilityKeep
yes: falsefor this repair path. TTY confirmation occurs before managed-install removal. Non-TTY execution fails with a readable error, and doctor records that error asFailed.tests/doctor_test.rs (1)
46-50: LGTM!Also applies to: 430-444
Cargo.toml (1)
79-82: LGTM!src/util/mod.rs (1)
5-5: LGTM!src/util/stdio_redirect.rs (2)
39-126: LGTM!Also applies to: 128-223
31-37: 📐 Maintainability & Code QualityNo change required.
unsafe extern "system"is compatible with edition 2021 and the declared Rust 1.88 MSRV.src/cli.rs (1)
39-41: LGTM!Also applies to: 57-57
tests/activate_test.rs (1)
25-25: LGTM!Also applies to: 139-139, 148-148, 352-352, 608-610, 624-631
tests/support/acceptance/model.rs (1)
94-98: LGTM!tests/plugin_active_update_real_nu.rs (1)
178-178: 📐 Maintainability & Code QualityConfirm the dedicated acceptance job runs this ignored test separately.
real_nu_active_update_resume_lockfile_updated_reactivatesis marked#[ignore], but the acceptance-job configuration remains unknown.AGENTS.md (2)
57-57: Update the doctor test entry inAGENTS.md.Line 112 still describes
tests/doctor_test.rsas covering the--fixauto tier. The current contract is default repairs with--scanfor report-only mode. Update Line 112 to match Line 57.As per coding guidelines, update documentation when changing user-visible behavior.
Source: Coding guidelines
67-67: LGTM!docs/numan-doctor.md (2)
25-30: Remove the remaining--fixreferences from the doctor specification.The invocation table is correct, but this document still mentions
--fixat Lines 55, 118, 119, and 165. Replace those references with default repair or--scanwording. Otherwise, the specification defines two CLI contracts.As per coding guidelines, update documentation when changing user-visible behavior.
Source: Coding guidelines
8-10: LGTM!Also applies to: 34-37, 66-84, 98-98, 205-208, 272-278
README.md (1)
272-273: LGTM!Also applies to: 311-311, 339-355
src/util/hints.rs (1)
18-22: LGTM!Also applies to: 127-127, 305-305
Document confirm-tier as a TTY allow-gate, print not_confirmed remediation in plain-text output, harden doctor --json spawn tests, add --json --scan repairs-omitted coverage, and rename the stale try_cmd pin-offer unit test. Co-authored-by: Anthony Thompson <github@trackdub.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cmd/doctor.rs (2)
2229-2284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the off-PATH regression test observe the repair.
test_noop_setup_repaircannot detect a setup invocation because it does nothing. The test also discards theexecute_with_optionsresult. It can pass when setup runs or when execution fails before repair dispatch. Use a call-counting seam, require successful execution, assert thatnu.binary.found_off_pathisSkipped, and then assert thatmanaged_binarystill exists.As per coding guidelines: “Behavior changes require corresponding tests, including relevant failure paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/doctor.rs` around lines 2229 - 2284, Update doctor_found_off_path_repair_preserves_managed_install_when_not_confirmed to use a call-counting setup seam instead of test_noop_setup_repair, require execute_with_options to succeed, and assert the nu.binary.found_off_path result is Skipped before verifying managed_binary still exists. Ensure the test fails if repair dispatch occurs or execution errors before repair handling.Source: Coding guidelines
145-145: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a snapshot before doctor repairs.
apply_repairsacquires the mutation lock but never creates a snapshot.init::execute_refreshalso skips the lock when no active packages exist and never creates a snapshot before writing the lockfile, autoload state, and Nu paths. Add an unconditional lock and pre-mutation snapshot before these writes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/doctor.rs` at line 145, Update the repair and refresh flows around apply_repairs and init::execute_refresh to unconditionally acquire the mutation lock and create a pre-mutation snapshot before writing any repair changes, lockfiles, autoload state, or Nu paths; do not skip this protection when no active packages exist.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cmd/doctor.rs`:
- Around line 2229-2284: Update
doctor_found_off_path_repair_preserves_managed_install_when_not_confirmed to use
a call-counting setup seam instead of test_noop_setup_repair, require
execute_with_options to succeed, and assert the nu.binary.found_off_path result
is Skipped before verifying managed_binary still exists. Ensure the test fails
if repair dispatch occurs or execution errors before repair handling.
- Line 145: Update the repair and refresh flows around apply_repairs and
init::execute_refresh to unconditionally acquire the mutation lock and create a
pre-mutation snapshot before writing any repair changes, lockfiles, autoload
state, or Nu paths; do not skip this protection when no active packages exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8856128d-6691-477f-b6df-a9cd061fd196
📒 Files selected for processing (2)
src/cmd/doctor.rssrc/cmd/try_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
💤 Files with no reviewable changes (1)
- src/cmd/try_cmd.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Greptile Review
- GitHub Check: Real-Nu acceptance (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use
anyhow::Resultwith.context("what failed")for application code,thiserrorfor library types callers match on, and avoid panics in library code by returningResult.
**/*.rs: Use Rust 2021 edition conventions and match existing naming, module layout, and documentation level in edited files.
Useanyhow::Resultwith.context(...)in application code; usethiserrorfor library error types that callers match on.
Useclapderive macros for CLI definitions.
Useserdewithserde_jsonortomlfor serialization.
Function parameters must use&Path, not&PathBuf.
Error paths must returnanyhow::Resultwith context, and library code must not panic.
Unit tests must use injectable seams such asFakeCandidateRunnerand registrars; do not spawn realnuin unit tests.
Real-Nu acceptance tests should be marked#[ignore]and run separately when activation or nupm-import behavior changes.
Every new mutating code path must acquire the mutation lock and snapshot the lockfile before writes.
Mutating commands, includinginstall,remove,update,gc, andnupm import, must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must be written atomically usingwrite_json_atomic.
numan installmust write only to$NUMAN_ROOTand must not invoke Nu or register plugins/autoloads.
Onlyactivateanddeactivatemay modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state are not authoritative.
Install payloads must use versioned, content-addressed paths and must never be overwritten in place.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.
The nupm boundary must remain read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.
**/*.rs: Use Rust 2021 edition conventions; ...
Files:
src/cmd/doctor.rs
src/{cmd,state,install}/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Call
create_snapshot()before mutations performed by install, update, remove, activate, deactivate, or nupm-import; garbage collection must treat payloads referenced by every snapshot as live roots.
Files:
src/cmd/doctor.rs
src/cmd/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Keep clap subcommand handlers thin; delegate domain logic to
core/or installation logic toinstall/.
Files:
src/cmd/doctor.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,toml}: All changes must pass formatting and linting:cargo fmtandcargo clippy -- -D warnings.
Behavior changes require corresponding tests, including relevant failure paths.
Files:
src/cmd/doctor.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update documentation when changing structure, conventions, or user-visible behavior, using
AGENTS.md,docs/, or command help as appropriate.
Files:
src/cmd/doctor.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
src/cmd/doctor.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
#[cfg(target_env)]-based platform detection from the binary build target rather than runtime platform constants.
Files:
src/cmd/doctor.rs
🔍 Remote MCP GitHub Copilot
Additional review context
- The PR is
#68, currently open and markedmergeable_state: blocked; its CodeRabbit status is pending, while most CI checks currently pass. Windows tests and real-Nu acceptance remain in progress. - Issues
#76and#77independently concluded thatactivateanddeactivateare reversible, journaled, ownership-guarded operations and should retain their confirmation prompts and--yesflags. This conflicts directly with removing those flags in this PR. - The PR’s current
src/cmd/doctor.rsincludesstd::io::IsTerminal, so the previously reported missing-trait compile issue is not present on PR#68’s current head. - Follow-up PR
#79describes this work as a doctor-UX follow-up and records additional decisions around snapshots, off-PATH repairs, and non-interactive behavior; it may represent the intended resolution path for findings from#68.
🔇 Additional comments (3)
src/cmd/doctor.rs (3)
988-992: Duplicate: non-interactive repair gating and reporting remain unresolved.The TTY-only approval, removed nested
yesoverrides, and count-only skipped-repair output are covered by the previous review comment for Lines 1023-1026. Keep this finding linked to that comment.Also applies to: 1004-1026, 1259-1260, 1316-1317, 1455-1456
5-5: LGTM!Also applies to: 47-47, 136-136, 1490-1496, 1540-1540, 1557-1557, 1597-1612, 1632-1638, 1656-1656, 1670-1670, 1688-1688, 1717-1717, 1781-1781, 1850-1850, 1935-1935, 1960-1974, 1987-1987, 2045-2045, 2087-2087, 2128-2128, 2167-2167, 2210-2210
1098-1100: 🎯 Functional CorrectnessNo change required.
confirm_repairsonly allows confirm-tier repairs in a TTY; it does not collect user consent. The nested setup call correctly performs the actual prompt, and non-interactive doctor runs skip the repair before setup executes.> Likely an incorrect or invalid review comment.
Require a call-counting setup seam and not_confirmed Skipped record for found_off_path when confirm-tier is closed. Always lock and PreMutation- snapshot init --refresh (even with no active packages) and doctor repair passes before writes. Co-authored-by: Anthony Thompson <github@trackdub.com>
Summary
UX improvements to reduce unnecessary prompts and simplify the doctor command.
Changes
Doctor Command Simplification
--fixand--yesflags--scanflag for report-only modenuman doctorapplies fixes by defaultnuman doctor --scanreports without fixing--jsonfor structured output (works with both modes)Removed --yes Flags
Removed
--yesfrom commands where prompts were unnecessary:Updated
nu_pin_offerauto_yesparameter fromoffer_managed_nu_pin()Testing
-D warnings)cargo fmtappliedFuture Work
setup nuandsetup loaderstill have--yesflags (not addressed due to complexity of overwrite scenarios)