fix: address PR 68 review findings (doctor UX) - #79
Conversation
📝 WalkthroughWalkthroughChangesDoctor repair and managed Nu setup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@Cargo.toml`:
- Around line 79-80: Move the libc dependency from the general dependencies
section into [target.'cfg(unix)'.dependencies] in Cargo.toml, keeping its
existing version constraint. Ensure Unix-only code and test helpers retain
access while non-Unix targets no longer resolve libc.
In `@docs/numan-doctor.md`:
- Line 244: Update the JSON example in the default repair mode section of
numan-doctor.md so the repair status reflects automatic application of
confirm-tier repairs, replacing the outdated not_confirmed value with a status
valid under the new policy.
- Around line 66-72: The Doctor repair flow must continue creating missing
layout directories when the existing lockfile contains malformed JSON. Defer or
degrade only the snapshot operation that fails, preserve the mutation-lock and
safe layout-repair behavior, and add a regression test in tests/doctor_test.rs
covering a malformed lockfile with missing layout directories; update the
documented behavior in docs/numan-doctor.md accordingly.
In `@src/cmd/doctor.rs`:
- Around line 998-1003: The fallible operations in apply_repairs need contextual
error messages: at src/cmd/doctor.rs lines 998-1003, wrap
StdoutToStderr::redirect() with the specified stdout-redirection context; at
src/cmd/doctor.rs lines 1014-1022, wrap create_snapshot(...) with the specified
pre-mutation snapshot context while preserving the separately required
availability fix.
- Around line 986-990: Remove the unused confirm_repairs function and the five
not_confirmed branches in apply_repairs, allowing repair results to follow the
remaining executable paths directly. Update docs/numan-doctor.md to match the
resulting JSON contract and remove any documentation for the eliminated
not-confirmed outcome.
- Around line 1014-1022: Update the apply_repairs snapshot flow around
create_snapshot so snapshot failures do not return from apply_repairs or prevent
unrelated repairs from being recorded and applied. Add context at this call
site, mark or skip only the repairs requiring the failed snapshot, and continue
allowing independent repairs such as layout.* and registry.none while keeping
protected mutations blocked.
In `@src/cmd/nu_pin_offer.rs`:
- Around line 27-30: Update the Nu pin confirmation input closures to return
anyhow::Result and add contextual error handling to both the stdin read in the
shown closure and the injected callback path at the referenced location. Use
context identifying the failed Nu pin confirmation read, while preserving the
existing successful input behavior.
In `@src/cmd/setup.rs`:
- Around line 623-642: Rename
execute_use_existing_preserves_managed_installation to describe preservation
during early resolution failure, since the missing-path setup never reaches PATH
or shell persistence. Add separate successful-registration coverage using
injectable PATH and configuration seams rather than host state; update
execute_use_existing or its dependencies so the test can avoid real
PATH/configuration writes while verifying registration behavior.
In `@src/util/stdio_redirect.rs`:
- Around line 1-11: Add Unix-focused tests for StdoutToStderr covering
pipe-based redirection, writes while the guard is active, restoration after
Drop, and relevant failure paths; add mock-platform coverage for
Windows-specific behavior where feasible. Update the StdoutToStderr
documentation to explicitly state that dup2/SetStdHandle redirection is
process-global and affects stdout writes from all threads during the guard’s
lifetime.
- Around line 36-97: Centralize the Windows FFI declarations, handle constants,
and related values in one private Windows-only module. Update the Windows paths
in redirect and Drop to reference that module’s shared symbols, preserving the
existing stdio redirection and handle cleanup behavior.
In `@tests/doctor_test.rs`:
- Line 56: Update the test flow around setup::execute_nu to stop discarding its
repair result: propagate failures as anyhow::Result with descriptive context,
unless this test explicitly expects an error and asserts that outcome. Keep the
subsequent managed-file assertion dependent on successful repair execution.
- Around line 559-576: Update the default doctor JSON subprocess test around the
Command invocation to assert output.status.success(), parse stdout only after
that check, and require the JSON object to contain the repairs field. Make the
fixture network-free by seeding a valid registry index compatible with
fake_init, or otherwise isolating registry synchronization while preserving the
default-mode CLI path.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: 680d994d-5d58-4e11-910d-e15239e7ff8f
📒 Files selected for processing (13)
AGENTS.mdCargo.tomlREADME.mddocs/numan-doctor.mdsrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/setup.rssrc/cmd/try_cmd.rssrc/state/snapshot.rssrc/util/mod.rssrc/util/stdio_redirect.rstests/doctor_test.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
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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:
Cargo.tomltests/support/acceptance/model.rssrc/util/stdio_redirect.rssrc/util/mod.rssrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rstests/doctor_test.rssrc/state/snapshot.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
Cargo.tomltests/support/acceptance/model.rssrc/util/stdio_redirect.rsREADME.mdsrc/util/mod.rssrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rstests/doctor_test.rsAGENTS.mddocs/numan-doctor.mdsrc/state/snapshot.rs
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Preserve MSRV 1.88 as specified by the crate's
rust-version.
Files:
Cargo.toml
**/*.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:
tests/support/acceptance/model.rssrc/util/stdio_redirect.rssrc/util/mod.rssrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rstests/doctor_test.rssrc/state/snapshot.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:
tests/support/acceptance/model.rssrc/util/stdio_redirect.rsREADME.mdsrc/util/mod.rssrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rstests/doctor_test.rsAGENTS.mddocs/numan-doctor.mdsrc/state/snapshot.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use a test-first workflow, include platform-specific behavior under mock platforms, and use fake Nu runners rather than real Nu binaries in unit tests.
Files:
tests/support/acceptance/model.rstests/doctor_test.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
anyhow::Resultfor application code,thiserrorfor library errors, add context with.context(...)or?, and never panic in library code.
Files:
src/util/stdio_redirect.rssrc/util/mod.rssrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rssrc/state/snapshot.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.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rssrc/state/snapshot.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.rssrc/cmd/nu_pin_offer.rssrc/cmd/try_cmd.rssrc/cmd/setup.rs
src/state/snapshot.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Activation snapshots are immutable; snapshot operations must preserve immutable snapshot semantics.
Files:
src/state/snapshot.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/state/**/*.rs: Create a snapshot before install, update, remove, activate, deactivate, or nupm-import mutations; garbage collection must treat snapshot-referenced payloads as live.
Write all JSON state files atomically usingwrite_json_atomicwith a temporary file in the same directory followed by persist.
Reconcile pending activation, deactivation, autoload, and lifecycle journals after interrupted operations according to their staged state transitions.
Files:
src/state/snapshot.rs
🔍 Remote MCP Context7, GitHub Copilot
Additional review context
-
Fresh-root regression risk:
main.rsintentionally does not create the root fordoctor. In default repair mode,doctordetectsnu_paths.missing, acquires the mutation lock, then callscreate_snapshot; snapshot creation canonicalizes the root before repairs create any directories. Therefore,numan doctoragainst a nonexistent root can fail before applying its advertised repairs. -
Unrelated corruption can block repairs: snapshots compute revisions for every lockfile payload and fail when any referenced payload is missing. Since
apply_repairssnapshots whenever any auto/confirm finding exists, an unrelated missing payload or malformed lockfile can prevent otherwise independent doctor repairs. -
Validation status: PR
#79reports passing tests, Clippy, and formatting, but GitHub currently reports zero check runs; the PR is draft and hasmergeable_state: dirty. -
JSON implementation: the new guard redirects process stdout using Unix file-descriptor duplication and Windows standard-handle replacement.
libcis now correctly a runtime dependency for the Unix implementation; Context7 identifies it as the raw Unix FFI binding crate.
🔇 Additional comments (12)
AGENTS.md (1)
67-68: LGTM!Also applies to: 112-112
README.md (1)
190-190: LGTM!Also applies to: 311-311, 343-351, 432-432
docs/numan-doctor.md (1)
31-31: LGTM!Also applies to: 55-55, 81-86, 100-100, 120-121, 167-169, 207-216
tests/support/acceptance/model.rs (1)
94-98: LGTM!src/cmd/nu_pin_offer.rs (1)
22-26: LGTM!Also applies to: 35-48, 65-70
src/cmd/try_cmd.rs (1)
445-452: LGTM!tests/doctor_test.rs (1)
8-8: LGTM!Also applies to: 17-17, 35-55, 57-60, 302-305
src/util/stdio_redirect.rs (1)
15-34: LGTM!src/util/mod.rs (1)
5-5: LGTM!src/state/snapshot.rs (1)
76-76: LGTM!src/cmd/doctor.rs (1)
32-39: LGTM!src/cmd/setup.rs (1)
242-253: LGTM!
![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
PR Summary by QodoFix doctor UX: preserve managed Nu, pre-repair snapshot, parseable --json
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
There was a problem hiding this comment.
🟡 Not ready to approve
The new Windows FFI declarations in src/util/stdio_redirect.rs likely trigger non_snake_case warnings (failing -D warnings CI), and a doctor integration test helper risks persisting PATH changes by calling setup::execute_nu in-process.
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
Follow-up fixes for the doctor UX refactor (#68), tightening behavior and docs around “repair by default / --scan report-only”, improving JSON-mode robustness, and adding regression tests for managed Nu preservation and snapshots.
Changes:
- Update doctor to create a
SnapshotTrigger::DoctorPreMutation snapshot on repair passes and keep--jsonstdout parseable by redirecting nested chatter to stderr during repairs. - Prevent
setup nu use <path>(off-PATH registration) from deleting an existing managed$NUMAN_ROOT/tools/nushellinstall; expand regression coverage. - Align docs/README/acceptance harness/tests with the new doctor defaults (
--scan, removed--fix/--yes) and makenu_pin_offertests non-interactive.
File summaries
| File | Description |
|---|---|
| tests/support/acceptance/model.rs | Stage1 acceptance doctor step now runs doctor --scan --json to keep harness non-mutating and machine-parseable. |
| tests/doctor_test.rs | Adds regression checks for Doctor snapshots, JSON stdout validity, and managed-Nu preservation on off-PATH repair paths. |
| src/util/stdio_redirect.rs | Introduces RAII stdout→stderr redirection used to keep JSON stdout parseable during repair passes. |
| src/util/mod.rs | Exposes the new stdio_redirect util module. |
| src/state/snapshot.rs | Adds SnapshotTrigger::Doctor to snapshot metadata. |
| src/cmd/try_cmd.rs | Updates nu_pin_offer test to use an explicit non-interactive helper. |
| src/cmd/setup.rs | setup nu use no longer deletes managed tools; adds a focused unit test for preservation. |
| src/cmd/nu_pin_offer.rs | Adds offer_managed_nu_pin_with_interaction for testability (explicit interactive/read_line injection). |
| src/cmd/doctor.rs | Creates PreMutation Doctor snapshots for repair mode and redirects nested stdout to stderr during --json repair passes. |
| README.md | Removes stale --yes/--fix references and documents doctor’s new flag set and defaults. |
| docs/numan-doctor.md | Updates spec to match inverted defaults (repair-by-default; --scan report-only) + snapshot semantics. |
| Cargo.toml | Promotes libc to a normal dependency to support stdio redirection on Unix. |
| AGENTS.md | Updates project map and command summaries for doctor/try/use behavior. |
Review details
Suppressed comments (1)
src/util/stdio_redirect.rs:123
- Same
non_snake_caselint issue as above for the Win32 FFI declarations inDrop; without an allow/wrapper this will warn on Windows and fail-D warningsbuilds.
#[link(name = "kernel32")]
unsafe extern "system" {
fn SetStdHandle(n_std_handle: u32, h_handle: *mut core::ffi::c_void) -> i32;
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo Fixer✅ Committed (1) · ☑ Fixed (1) Commits pushed directly to this PR — no separate fix PR opened. Process — 1 fixed
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
![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: 4a665ef821
ℹ️ 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".
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
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 (4)
src/cmd/setup.rs (3)
454-456: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not clear the active marker before deletion can succeed.
clear_active_version(root)?succeeds first, butremove_dir_allcan then fail. The managed tree remains while the authoritative active-version marker is gone. This leaves inconsistent state and can change later version resolution.Delete the tree first, then clear the marker, or restore the marker when deletion fails. Add a failure-path test.
Safer minimum ordering
- crate::nu::version_manager::clear_active_version(root)?; - std::fs::remove_dir_all(&managed_dir).with_context(|| { format!( "Failed to remove managed Nushell directory '{}'", managed_dir.display() ) })?; + crate::nu::version_manager::clear_active_version(root)?;Also applies to: 475-480
🤖 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/setup.rs` around lines 454 - 456, Update the setup removal flow around clear_active_version and remove_dir_all so the active-version marker is cleared only after the versioned tree has been deleted successfully; preserve the marker when deletion fails. Apply the same ordering to the additionally referenced removal path, and add a failure-path test verifying the marker remains when tree deletion errors.
386-390: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject paths inside the managed tree before deletion.
execute_use_pathalready rejects a resolved path undermanaged_dir, butexecute_use_existingdoes not. If the caller passes$NUMAN_ROOT/tools/nushell/nu, validation succeeds,remove_managed_nu_if_presentdeletes the target, andregister_existing_nuthen fails because the path no longer exists. This loses the managed installation and active-version state.Resolve the managed directory and reject
resolved_path.starts_with(&resolved_managed_dir)before prompting or deleting.Proposed guard
if managed_dir_was_present { let resolved_path = std::fs::canonicalize(path) .with_context(|| format!("Failed to resolve Nushell binary '{}'", path.display()))?; + let resolved_managed_dir = managed_dir.canonicalize().with_context(|| { + format!( + "Failed to resolve managed Nushell directory '{}'", + managed_dir.display() + ) + })?; + if resolved_path.starts_with(&resolved_managed_dir) { + bail!("The selected Nushell binary is inside Numan's managed install."); + }Also applies to: 419-430
🤖 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/setup.rs` around lines 386 - 390, Update execute_use_existing to canonicalize managed_dir and reject resolved_path when it starts with the resolved managed directory, before any prompt or deletion occurs. Reuse the existing path-resolution flow and preserve execute_use_path’s rejection behavior, ensuring remove_managed_nu_if_present is never called for paths inside the managed tree.
911-928: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSerialize tests that mutate process-global PATH.
Rust runs tests in parallel.
run_with_path_snapshotsaves and restores the process-globalPATHwithout a shared lock. Two tests can capture and restore each other’s temporary values, causing flaky results.Hold a shared mutex for the entire snapshot/body operation, and use the same guard for every test that changes
PATH.Proposed test guard
+static PATH_TEST_LOCK: std::sync::OnceLock<std::sync::Mutex<()>> = + std::sync::OnceLock::new(); + fn run_with_path_snapshot<F, R>(body: F) -> R where F: FnOnce() -> R + std::panic::UnwindSafe, { + let _guard = PATH_TEST_LOCK + .get_or_init(|| std::sync::Mutex::new(())) + .lock() + .unwrap(); let saved = std::env::var("PATH").ok();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/setup.rs` around lines 911 - 928, Update run_with_path_snapshot to acquire a shared process-wide mutex before reading PATH and hold the guard through the body execution and restoration, including panic handling. Ensure every test or helper that mutates PATH uses this same mutex/guard rather than changing PATH outside the serialized path.src/cmd/try_cmd.rs (1)
22-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the public
try --yesoption with the documented CLI.
#[arg(long)]exposesnuman try --yes, but README.md and AGENTS.md now omit that option. Choose one contract: removeTryArgs::yesand its call sites, or document the option and its limited consent semantics in both files.As per coding guidelines, user-visible behavior changes require corresponding documentation.
🤖 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/try_cmd.rs` around lines 22 - 27, Align TryArgs::yes with the documented CLI contract: either remove the yes field and all call sites, or retain it and document numan try --yes, including its limited consent semantics, in README.md and AGENTS.md. Ensure the chosen behavior and documentation remain consistent.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/setup.rs`:
- Around line 454-456: Update the setup removal flow around clear_active_version
and remove_dir_all so the active-version marker is cleared only after the
versioned tree has been deleted successfully; preserve the marker when deletion
fails. Apply the same ordering to the additionally referenced removal path, and
add a failure-path test verifying the marker remains when tree deletion errors.
- Around line 386-390: Update execute_use_existing to canonicalize managed_dir
and reject resolved_path when it starts with the resolved managed directory,
before any prompt or deletion occurs. Reuse the existing path-resolution flow
and preserve execute_use_path’s rejection behavior, ensuring
remove_managed_nu_if_present is never called for paths inside the managed tree.
- Around line 911-928: Update run_with_path_snapshot to acquire a shared
process-wide mutex before reading PATH and hold the guard through the body
execution and restoration, including panic handling. Ensure every test or helper
that mutates PATH uses this same mutex/guard rather than changing PATH outside
the serialized path.
In `@src/cmd/try_cmd.rs`:
- Around line 22-27: Align TryArgs::yes with the documented CLI contract: either
remove the yes field and all call sites, or retain it and document numan try
--yes, including its limited consent semantics, in README.md and AGENTS.md.
Ensure the chosen behavior and documentation remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99a94c31-64c6-4944-9f21-bb76292f366c
📒 Files selected for processing (7)
AGENTS.mdREADME.mddocs/numan-doctor.mdsrc/cmd/doctor.rssrc/cmd/setup.rssrc/cmd/try_cmd.rstests/doctor_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 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 and follow th...
Files:
src/cmd/try_cmd.rssrc/cmd/setup.rssrc/cmd/doctor.rstests/doctor_test.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/try_cmd.rssrc/cmd/setup.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/setup.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/cmd/try_cmd.rssrc/cmd/setup.rssrc/cmd/doctor.rstests/doctor_test.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/try_cmd.rsAGENTS.mdsrc/cmd/setup.rsREADME.mdsrc/cmd/doctor.rsdocs/numan-doctor.mdtests/doctor_test.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
src/cmd/try_cmd.rsAGENTS.mdsrc/cmd/setup.rsREADME.mdsrc/cmd/doctor.rsdocs/numan-doctor.mdtests/doctor_test.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use injected fakes and test seams for Nu-dependent behavior; real-Nu acceptance tests belong in ignored integration tests.
Files:
tests/doctor_test.rs
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
main.rsdeliberately does not create the root directory fordoctor; thereforecreate_snapshot()must canonicalize an already-existing root before repairs create layout directories. A nonexistent root can fail during snapshot creation.create_snapshot()also loads the lockfile and computes revisions for every referenced payload; one missing/inaccessible payload causes the entire snapshot—and thus doctor repairs—to fail.- The PR’s
doctor --jsonimplementation redirects process-global stdout to stderr while nested repairs run, then restores it through an RAII guard. This is the mechanism preserving parseable JSON stdout. - Current CI status is not fully green: one Greptile check is still
in_progress; Sourcery was skipped. - DeepWiki’s indexed description still refers to the prior
--fixflow and says doctor does not create snapshots, so it is stale for this PR; the PR diff and current source explicitly add default repairs andSnapshotTrigger::Doctor.
🔇 Additional comments (8)
src/cmd/doctor.rs (4)
986-990: Remove the unreachable confirmation branches.
confirm_repairsalways returnstrue, and--scandoes not callapply_repairs. Therefore thenot_confirmedbranches cannot execute. Remove the dead predicate and branches, or restore a real confirmation input. Keep the JSON contract aligned with executable statuses.
1000-1001: Add context to the stdout-redirection error.
StdoutToStderr::redirect()?returns a bare error. Add context that identifies the failed Doctor JSON redirection.As per coding guidelines, application error paths must return
anyhow::Resultwith.context("what failed").Source: Coding guidelines
1015-1022: Do not abort independent repairs when snapshot creation fails.
create_snapshotcomputes revisions for every lockfile-referenced payload. A missing or inaccessible payload makes this?return before Doctor records or applies layout and configuration repairs. Scope snapshot failure to repairs that require the snapshot, or create a degraded snapshot and record the affected repairs as failed. Add context to the propagated error.Sources: Coding guidelines, MCP tools
1097-1100: LGTM!Also applies to: 1139-1140
src/cmd/setup.rs (1)
944-946: Keep successful registration tests away from the host shell profile.These tests call
execute_use_existingwith persistent PATH updates enabled.register_existing_nureachespersist_path_dir, so the tests can append the temporary binary directory to the developer or CI shell profile.run_with_path_snapshotrestores only processPATH; it cannot undo persistent profile changes.Inject the persistence seam, or expose a test-only
skip_pathoption and assert the registration without modifying host state.Also applies to: 971-973
tests/doctor_test.rs (1)
556-581: Verify the complete default-mode JSON contract.The test must assert
output.status.success()and require the parsed JSON to containrepairs. A default Doctor run can emit valid JSON and still exit nonzero after a repair failure. Keep the fixture network-free by seeding a valid registry index or isolating registry synchronization.#!/bin/bash set -euo pipefail sed -n '556,581p' tests/doctor_test.rs rg -n -C6 \ 'doctor_json|output\.status|repairs|registry\.index_missing|skip_network|registry sync' \ tests/doctor_test.rs src/cmd/doctor.rsAGENTS.md (1)
144-144: LGTM!README.md (1)
190-190: LGTM!Also applies to: 346-355, 435-435
Greptile SummaryThis PR inverts the doctor UX: repair mode is now the default and
Confidence Score: 5/5Safe to merge; the behavior changes are intentional and well-covered by new integration tests. The removal of the TTY confirm gate is intentional and correct: confirm-tier repairs now run unconditionally, nu.binary.missing_on_path is demoted to manual so doctor never auto-downloads Nu, and nu.binary.found_off_path delegates the managed-wipe consent gate to setup nu use rather than bypassing it. The soft-fail snapshot path is clearly documented and tested end-to-end in doctor_repairs_layout_when_lockfile_malformed. The remove_managed_nu_if_present silent helper (no consent, no snapshot) is correctly distinct from the standalone remove_managed_nu (has its own consent plus snapshot), so there is no double-snapshot issue. No correctness or safety regressions were found. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/cmd/doctor.rs | Removes confirm_repairs TTY gate; replaces with snapshot_ok guard; adds check_active_version_marker and independent repair; nu.binary.missing_on_path demoted to manual tier; PendingPluginDeactivate load errors now recorded as Failed instead of propagated. Logic is correct and well-tested. |
| src/cmd/setup.rs | Adds snapshot_before_setup_mutation helper; snapshots taken before execute_use_path, execute_use_existing, and standalone remove_managed_nu. remove_managed_nu_if_present is the silent variant (no snapshot, no consent gate) — no double-snapshot issue. |
| src/cmd/init.rs | Applies reload-after-lock pattern: NuPaths and Lockfile loaded after the mutation lock is held; adds two new tests for lock-contention fail-close and snapshot-failure path isolation. |
| src/cmd/nu_pin_offer.rs | Refactors TTY seam from Option to explicit interactive: bool + read_line: F closure, enabling tests to assert the non-interactive path never reads stdin. |
| src/nu/bootstrap.rs | Extracts snapshot_before_nu_setup helper; adds snapshot for the already_installed+--yes PATH-only path that was previously unguarded. |
| src/util/hints.rs | Collapses CMD_DOCTOR and CMD_DOCTOR_FIX into one constant; adds backslash to shell_quote special-character set with a covering test. |
| tests/doctor_test.rs | Adds tests for malformed active-version marker repair, soft-fail snapshot path, default no-install-Nu behavior, consent-gate preservation, and snapshot presence after default repairs. confirm_repairs helpers cleanly removed. |
| tests/module_autoload_test.rs | Adds #[cfg(target_os = "windows")] to the Windows-specific real_nu_windows_path_with_spaces_validates test, replacing the runtime early-return with a compile-time gate. |
| docs/numan-doctor.md | Repair-policy table rewritten for new tier semantics (confirm-tier now applied by default, nu.binary.missing_on_path demoted to manual). Snapshot soft-fail and snapshot_unavailable behavior documented. |
| src/cmd/try_cmd.rs | Adds a CLI-rejection test for the obsolete --yes flag on numan try; updates nu_pin_offer call site to use the new interaction seam. |
Reviews (9): Last reviewed commit: "fix: address still-valid PR review findi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/activate_test.rs`:
- Line 608: Update test_activate_succeeds_without_prompt so it installs a prompt
callback that fails if invoked, while preserving the existing no-input setup and
successful activation assertion. Ensure the test explicitly verifies the prompt
path is never used rather than relying only on result.is_ok().
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: ab60485d-d858-43e8-a5eb-a7c7f00c4f75
📒 Files selected for processing (7)
docs/numan-doctor.mdsrc/cmd/doctor.rssrc/cmd/try_cmd.rssrc/util/hints.rssrc/util/stdio_redirect.rstests/activate_test.rstests/doctor_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Greptile Review
- GitHub Check: Test (windows-latest)
- GitHub Check: Real-Nu acceptance (windows-latest)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (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: Useanyhow::Resultfor applicatio...
Files:
src/util/stdio_redirect.rssrc/util/hints.rstests/activate_test.rssrc/cmd/try_cmd.rstests/doctor_test.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/stdio_redirect.rssrc/util/hints.rstests/activate_test.rssrc/cmd/try_cmd.rstests/doctor_test.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/stdio_redirect.rssrc/util/hints.rstests/activate_test.rssrc/cmd/try_cmd.rstests/doctor_test.rssrc/cmd/doctor.rsdocs/numan-doctor.md
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
src/util/stdio_redirect.rssrc/util/hints.rstests/activate_test.rssrc/cmd/try_cmd.rstests/doctor_test.rssrc/cmd/doctor.rsdocs/numan-doctor.md
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Platform detection must use compile-time#[cfg(target_env)]information from the binary build target, notstd::env::consts.
All JSON state files, including lockfiles, journals, andnu_state/paths.json, must be written withwrite_json_atomicusing a same-directory temporary file and persist operation.
Acquireacquire_mutation_lock(root)before root mutations; the lock is an RAII guard and a second acquisition must fail immediately rather than block.
Nu executable paths and plugin names/configuration must be passed through environment variables (NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG,NUMAN_PLUGIN_NAME); the Nu program string must be a compile-time constant with no runtime interpolation.
Files:
src/util/stdio_redirect.rssrc/util/hints.rssrc/cmd/try_cmd.rssrc/cmd/doctor.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use mock platforms for platform-specific tests and injected fake Nu runners for unit tests; reserve real-Nu execution for ignored acceptance tests.
Files:
tests/activate_test.rstests/doctor_test.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/try_cmd.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/doctor.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep review conventions in
REVIEW.md; Copilot review instructions in.github/instructions/review.instructions.mdmust remain aligned rather than duplicating review rules elsewhere.
Files:
docs/numan-doctor.md
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
-
Open review findings: PR
#79has unresolved comments about:create_snapshot(...)aborting all repairs when a payload is missing or the lockfile is malformed, preventing the diagnosis report from being emitted.- Windows stdout restoration potentially closing the handle installed as
STD_OUTPUT_HANDLE. StdoutToStderrlacking tests and being process-global; on Unix it isSend + Syncdespite mutating global file-descriptor state.libcbeing unconditional despite production usage being Unix-only.- The JSON documentation example still using obsolete
not_confirmedstatus. - The subprocess JSON test not asserting successful exit or presence of
repairs, and potentially triggering registry-network behavior.
-
create_snapshotcanonicalizes the root, loads the lockfile, and computes revisions for every referenced payload; any missing or unhashable payload returns an error before the snapshot is published.SnapshotTrigger::Doctoris present, but snapshot creation remains fail-fast. -
doctorcurrently creates the stdout redirect before determining whether repairs are needed, and propagates both redirection and snapshot errors directly with?. Default repair mode therefore can terminate beforeprint_report. -
Current checks show CodeQL, formatting, Clippy, MSRV, Linux/macOS tests, and real-Nu acceptance passing; Windows tests, Windows real-Nu acceptance, Rust analysis, and Greptile review were still in progress at retrieval time.
-
DeepWiki’s indexed documentation is stale: it still describes the former
--fixflow and old confirmation behavior. The current PR source is the authoritative reference for the new--scan/repair-by-default semantics.
🔇 Additional comments (8)
docs/numan-doctor.md (1)
81-81: LGTM!Also applies to: 266-266
src/cmd/try_cmd.rs (1)
293-310: LGTM!Also applies to: 460-467
src/cmd/doctor.rs (1)
351-351: LGTM!Also applies to: 1125-1133
src/util/stdio_redirect.rs (1)
127-127: LGTM!tests/doctor_test.rs (1)
275-315: LGTM!src/util/hints.rs (2)
50-50: LGTM!
228-231: LGTM!tests/activate_test.rs (1)
25-25: LGTM!
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/doctor_test.rs (1)
60-68: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd coverage for the managed-install consent gate.
execute_use_existingvalidates the binary before it reachesif !yesorremove_managed_nu_if_present. The missing binary therefore tests only the resolve-failure path. Add a second case with a valid off-PATH Nushell binary, an existing managed install, andyes == false. Assert that the repair is refused and the managed install remains. Keep the resolve-failure case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/doctor_test.rs` around lines 60 - 68, Add a second test case alongside the existing resolve-failure case using a valid off-PATH Nushell binary, an existing managed installation, and yes set to false. Exercise execute_use_existing through setup::execute_nu, assert that the repair is refused at the consent gate, and verify the managed installation remains; preserve the current missing-binary case unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/util/stdio_redirect.rs`:
- Around line 159-180: Update
redirect_sends_stdout_writes_to_stderr_pipe_then_restores to acquire and retain
both Rust stdout and stderr locks on the forking thread before calling
libc::fork(), keeping the guards alive through the fork and child redirect
assertions. Preserve the existing FD-isolation safety comment and parent wait
behavior.
In `@tests/doctor_test.rs`:
- Around line 190-213: Expand the malformed-lockfile test around
execute_with_options to capture the JSON subprocess output, following
doctor_json_default_stdout_is_valid_json, and assert snapshot.pre_mutation has
status failed plus snapshot-dependent repairs have status skipped with reason
snapshot_unavailable. Replace the discarded code value with an assertion for the
expected error exit code, and update the misleading report-capture comment to
reflect the actual subprocess capture.
- Around line 690-693: Update the default doctor JSON assertion around the
repairs field to verify that value.get("repairs") is a JSON array rather than
merely present, so null does not satisfy the contract. Preserve the existing
failure message and test scope.
---
Outside diff comments:
In `@tests/doctor_test.rs`:
- Around line 60-68: Add a second test case alongside the existing
resolve-failure case using a valid off-PATH Nushell binary, an existing managed
installation, and yes set to false. Exercise execute_use_existing through
setup::execute_nu, assert that the repair is refused at the consent gate, and
verify the managed installation remains; preserve the current missing-binary
case unchanged.
🪄 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: 2edb8fd8-d46c-4b59-8cfe-0e8fdbe64103
📒 Files selected for processing (7)
Cargo.tomlREADME.mddocs/numan-doctor.mdsrc/cmd/doctor.rssrc/cmd/nu_pin_offer.rssrc/util/stdio_redirect.rstests/doctor_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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:
Cargo.tomlsrc/cmd/nu_pin_offer.rstests/doctor_test.rssrc/util/stdio_redirect.rssrc/cmd/doctor.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
Cargo.tomlsrc/cmd/nu_pin_offer.rsREADME.mdtests/doctor_test.rssrc/util/stdio_redirect.rssrc/cmd/doctor.rsdocs/numan-doctor.md
**/*.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/cmd/nu_pin_offer.rstests/doctor_test.rssrc/util/stdio_redirect.rssrc/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/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/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/cmd/nu_pin_offer.rsREADME.mdtests/doctor_test.rssrc/util/stdio_redirect.rssrc/cmd/doctor.rsdocs/numan-doctor.md
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use
anyhow::Resultfor application code andthiserrorfor library error types; add context with.context(...)or?, and never panic in library code.
Files:
src/cmd/nu_pin_offer.rssrc/util/stdio_redirect.rssrc/cmd/doctor.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use a test-first workflow, keep unit tests inline with source modules, place integration tests under
tests/, and test platform-specific behavior with mock platforms.
Files:
tests/doctor_test.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-02T16:23:27.798Z
Learning: Do not silently switch the user's Nushell version; offer managed Nu pinning or onboarding instead.
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-02T16:23:27.798Z
Learning: Do not force-push to `master`; use imperative commit messages under 72 characters, feature/fix branches, and squash merges.
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- The current PR diff includes fixes for previously reported issues: Unix-only
libc, contextual errors, snapshot-failure degradation, JSON test assertions, Windows handle lifetime, and!Send/!Syncprotection forStdoutToStderr. - The remaining unresolved, non-outdated review concern is that
test_activate_succeeds_without_promptonly checksresult.is_ok(); it does not prove that no prompt path was invoked. - CI status at retrieval: CodeQL, Python analysis, and Actions analysis passed; Rust analysis and Greptile review were still in progress.
- DeepWiki could not provide repository context because the requested Babel-Player repository was not found. It is not relevant to this Rust
numanPR.
🔇 Additional comments (14)
README.md (1)
47-47: LGTM!docs/numan-doctor.md (1)
72-77: LGTM!Also applies to: 86-91, 244-250
src/cmd/doctor.rs (5)
992-1037: LGTM!
1101-1111: LGTM!
1136-1145: LGTM!
1152-1197: LGTM!
1219-1223: LGTM!Also applies to: 1258-1262, 1298-1302
src/util/stdio_redirect.rs (3)
4-47: LGTM!
69-126: LGTM!Also applies to: 142-142
182-238: LGTM!Cargo.toml (1)
79-80: LGTM!src/cmd/nu_pin_offer.rs (2)
22-35: LGTM!
41-50: 📐 Maintainability & Code QualityNo change required.
offer_managed_nu_pin_yes_refuses_silent_switchpassesinteractive = falsewith a callback that panics if invoked, proving the prompt path is skipped.> Likely an incorrect or invalid review comment.tests/doctor_test.rs (1)
656-663: LGTM!Also applies to: 672-681
The merge-base changed after approval.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent ran but didn't make any changes. The issues may already be fixed or require manual intervention. |
3e026f5 to
ac75819
Compare
Document that layout/active-version/registry.none continue when PreMutation fails while nu_paths.missing init is skipped. Record Failed repair records when PendingPluginDeactivate::load errors instead of aborting apply_repairs. Co-authored-by: Anthony Thompson <github@trackdub.com>
51a0238
…-fc68 Resolve conflicts by keeping PR 79 doctor UX (no confirm_repairs TTY gate, PreMutation snapshot model, manual missing-Nu path) while taking compatible base updates such as the --json --scan regression test. Co-authored-by: Anthony Thompson <github@trackdub.com>
62045ee
Always acquire the mutation lock and take a PreMutation Init snapshot on init --refresh (even with no active packages). Repair the doctor spec table merge corruption and note snapshot_unavailable for registry.index_missing. PendingPluginDeactivate load failures were already recorded as Failed repair records. Co-authored-by: Anthony Thompson <github@trackdub.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The PR introduces broken Markdown tables/formatting in docs (docs/numan-doctor.md) and a concatenated entry in AGENTS.md that should be fixed before approval.
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.
Review details
Suppressed comments (3)
docs/numan-doctor.md:270
- In the "Relationship to existing commands" table, the
numan activateandnuman registry syncentries are merged into a single row (contains||mid-row). This breaks markdown rendering and makes the command/role mapping unclear. Split them into two rows.
| `numan init` / `init --refresh` | **Repair** Nu path drift (default doctor delegates here) |
| `numan setup nu` | **Manual fix** for missing Nushell (`nu.binary.missing_on_path`; doctor prints the hint and does not download) |
| `numan setup nu use <path>` | **Repair** off-PATH Nushell (`nu.binary.found_off_path`; adds parent dir to user PATH; consented wipe of managed Nu requires `--yes` / TTY; doctor does not auto-approve) |
| `numan activate` | **Repair** activation + journal reconciliation || `numan registry sync` | **Repair** missing index cache (auto tier) |
| `numan activate --check` | Deep **module** check only; no repair |
docs/numan-doctor.md:133
- The markdown table row for the Nu checks is malformed: the
nu.active_version.malformedrow and thenu_paths.missingrow are accidentally concatenated into one line (contains an extra||), which breaks rendering and makes the catalog ambiguous. Split into two separate table rows.
This issue also appears on line 266 of the same file.
| `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 <path>` |
| `nu.path.version` | `info` | PATH-only Nu version (`PATH Nu: 0.114.1`), `PATH Nu: not found`, or `PATH Nu: found at '<path>' but version probe failed (<error>)` when the binary exists but `--version` fails. Does not treat managed Nu as PATH. Report-only (no automatic repair). |
| `nu.managed.version` | `info` | Managed binary under `$NUMAN_ROOT/tools/nushell/` with version, `Managed Nu: not installed`, or `Managed Nu: present at '<path>' but version probe failed (<error>)` when the binary exists but `--version` fails. Report-only (no automatic repair). |
| `nu.active_version.malformed` | `error` | `nu_state/active-version.json` is present but unreadable/invalid JSON. Lookup would otherwise soft-miss the marker and fall back to PATH. **auto:** clear the marker via `clear_active_version` so resolution recovers cleanly. || `nu_paths.missing` | `error` | `paths.json` absent → fix: `numan init` |
| `nu_paths.drift` | `error` | `NuPaths::validate_drift()` fails → fix: `numan init --refresh` |
AGENTS.md:113
- The
tests/section formatting is broken: thedoctor_test.rsandnupm_compat_test.rsentries are concatenated onto a single line, which makes the file list hard to read and breaks the intended structure. Put them on separate lines like the surrounding entries.
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` default repairs, `--scan` report-only, journal checks (Phase 7.2) nupm_compat_test.rs — Phase 6 integration tests (T13–T25, import/drift/manifest/activation/platform)
nupm_real_nu_test.rs — Phase 6.4 real-Nu #[ignore] acceptance tests (run with `cargo test -- --ignored`)
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…-fc68 Resolve conflicts favoring PR 79 doctor UX (soft-fail PreMutation snapshot, no confirm_repairs). Keep Init trigger and unify docs wording. Co-authored-by: Anthony Thompson <github@trackdub.com>
Fix malformed tables in numan-doctor.md and the concatenated doctor_test/nupm_compat_test line in AGENTS.md from the merge. Co-authored-by: Anthony Thompson <github@trackdub.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nu/bootstrap.rs (1)
717-775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
create_snapshotcall into a local helper.Lines 720-728 and 768-775 both call
create_snapshot(root, SnapshotReason::PreMutation, SnapshotTrigger::Install, None, None)inside the same function,execute_nu_setup_with_installer, differing only in the context message. Extract a small local helper, similar tosetup.rs'ssnapshot_before_setup_mutation, to avoid the two call sites drifting out of sync if the trigger or reason changes later.♻️ Proposed refactor
+fn snapshot_before_install_mutation(root: &Path, context_msg: &str) -> Result<()> { + create_snapshot(root, SnapshotReason::PreMutation, SnapshotTrigger::Install, None, None) + .with_context(|| context_msg.to_string()) +} + ... if options.yes { // PATH persistence mutates user shell state; snapshot first. - create_snapshot( - root, - SnapshotReason::PreMutation, - SnapshotTrigger::Install, - None, - None, - ) - .context("Failed to create pre-mutation snapshot for existing `numan setup nu`")?; + snapshot_before_install_mutation( + root, + "Failed to create pre-mutation snapshot for existing `numan setup nu`", + )?; ... - create_snapshot( - root, - SnapshotReason::PreMutation, - SnapshotTrigger::Install, - None, - None, - ) - .with_context(|| "Failed to create pre-install snapshot for `numan setup nu`")?; + snapshot_before_install_mutation( + root, + "Failed to create pre-install snapshot for `numan setup nu`", + )?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nu/bootstrap.rs` around lines 717 - 775, In execute_nu_setup_with_installer, extract the shared create_snapshot invocation using SnapshotReason::PreMutation and SnapshotTrigger::Install into a local helper, following the pattern of snapshot_before_setup_mutation. Have the helper accept or apply the differing context message, then replace both existing call sites while preserving their current error context and behavior.docs/numan-doctor.md (1)
36-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale text on confirm-tier behavior contradicts this same file's repair policy.
Lines 36-39 state that confirm-tier repairs run only when stdin is a TTY, and non-interactive sessions skip them as
not_confirmed. This is not true anymore. The Repair Policy table (lines 85-96) states that confirm-tier findings, such asnu_paths.driftandjournal.plugin_pending, apply "Never (applied in default mode)" with no TTY gate.src/cmd/doctor.rsapply_repairsdoes not checkis_terminal()for these repairs, and thenot_confirmedstatus was removed from the code (per an earlier fixed review comment). Onlynu.binary.found_off_pathfunnels through a nestedsetup nu usecall, which is a separate consent mechanism.Update this paragraph to state that confirm-tier repairs apply automatically in default mode, matching the policy table below it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/numan-doctor.md` around lines 36 - 40, Update the default-mode behavior paragraph in numan-doctor.md to state that confirm-tier repairs are applied automatically in default mode without a TTY gate or not_confirmed outcome. Keep the separate nested setup nu consent behavior for nu.binary.found_off_path, and leave the --scan description unchanged.
🤖 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 130-133: Split the merged Markdown table entry after the
nu.active_version.malformed row into two distinct rows: keep its existing three
columns intact, then place nu_paths.missing and its description on a new row.
Ensure the table has exactly three columns per row and preserves the existing
nu_paths.drift row.
- Around line 266-270: Split the merged Markdown table entry after the `numan
activate` row so `numan registry sync` starts on its own row. Preserve both
commands and their existing descriptions while ensuring each row has exactly two
table columns.
In `@src/cmd/doctor.rs`:
- Around line 329-353: Update the Err branch in check_active_version_marker to
use the canonical hints::CMD_DOCTOR_FIX constant instead of the hardcoded "numan
doctor" string, adding the necessary import or qualified reference while
preserving the existing finding behavior.
In `@src/cmd/init.rs`:
- Around line 177-180: Move acquire_mutation_lock before loading NuPaths and
Lockfile in the init flow, including old_paths and Lockfile::load. Compute
has_active_modules and perform validation using the state loaded after the lock
is held, following activate.rs’s reload-after-lock pattern, so refresh cannot
save stale mutable state.
- Around line 484-516: The refresh safety tests around
refresh_without_active_packages_still_locks_and_snapshots are incomplete. Extend
coverage to verify the mutation lock blocks or serializes a concurrent writer,
assert the created Init snapshot has reason PreMutation, and add a failure-path
test proving a snapshot error prevents path writes. Use the existing runner,
lock, snapshot, and injectable seams rather than changing unrelated behavior.
- Around line 181-188: Extend the Init snapshot flow around create_snapshot to
capture NuPaths as a snapshot sidecar whenever numan init --refresh rewrites
nu_state/paths.json. Update snapshot restore logic to restore the saved NuPaths
alongside the lockfile, including when no package is active, and add a rollback
test covering that no-active-package case.
---
Outside diff comments:
In `@docs/numan-doctor.md`:
- Around line 36-40: Update the default-mode behavior paragraph in
numan-doctor.md to state that confirm-tier repairs are applied automatically in
default mode without a TTY gate or not_confirmed outcome. Keep the separate
nested setup nu consent behavior for nu.binary.found_off_path, and leave the
--scan description unchanged.
In `@src/nu/bootstrap.rs`:
- Around line 717-775: In execute_nu_setup_with_installer, extract the shared
create_snapshot invocation using SnapshotReason::PreMutation and
SnapshotTrigger::Install into a local helper, following the pattern of
snapshot_before_setup_mutation. Have the helper accept or apply the differing
context message, then replace both existing call sites while preserving their
current error context and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 186d44c2-631f-4e57-9eda-2c2773f3711a
📒 Files selected for processing (15)
AGENTS.mdCargo.tomlREADME.mddocs/numan-doctor.mddocs/snapshots-and-rollback.mdsrc/cmd/doctor.rssrc/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/cmd/setup.rssrc/cmd/try_cmd.rssrc/nu/bootstrap.rssrc/state/snapshot.rssrc/util/hints.rstests/doctor_test.rstests/module_autoload_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{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:
docs/snapshots-and-rollback.mdtests/module_autoload_test.rsREADME.mdAGENTS.mdsrc/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/util/hints.rstests/doctor_test.rssrc/cmd/setup.rsdocs/numan-doctor.mdsrc/state/snapshot.rssrc/nu/bootstrap.rssrc/cmd/try_cmd.rssrc/cmd/doctor.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Redact secrets when reporting relevant logs or lockfile excerpts in issues.
Files:
docs/snapshots-and-rollback.mdtests/module_autoload_test.rsREADME.mdAGENTS.mdsrc/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/util/hints.rsCargo.tomltests/doctor_test.rssrc/cmd/setup.rsdocs/numan-doctor.mdsrc/state/snapshot.rssrc/nu/bootstrap.rssrc/cmd/try_cmd.rssrc/cmd/doctor.rs
**/*.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:
tests/module_autoload_test.rssrc/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/util/hints.rstests/doctor_test.rssrc/cmd/setup.rssrc/state/snapshot.rssrc/nu/bootstrap.rssrc/cmd/try_cmd.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:
tests/module_autoload_test.rssrc/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/util/hints.rsCargo.tomltests/doctor_test.rssrc/cmd/setup.rssrc/state/snapshot.rssrc/nu/bootstrap.rssrc/cmd/try_cmd.rssrc/cmd/doctor.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use unit tests inline with source modules and integration tests under
tests/; test platform-specific behavior with mock platforms and inject fake Nu runners instead of real binaries in unit tests.
Files:
tests/module_autoload_test.rstests/doctor_test.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/init.rssrc/cmd/nu_pin_offer.rssrc/cmd/setup.rssrc/state/snapshot.rssrc/cmd/try_cmd.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/init.rssrc/cmd/nu_pin_offer.rssrc/cmd/setup.rssrc/cmd/try_cmd.rssrc/cmd/doctor.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Useanyhow::Resultfor application code,thiserrorfor library errors, add error context with.context(...)or?, and never panic in library code.
Use#[cfg(target_env)]and compile-time platform constants for platform detection; do not usestd::env::constsfor the target platform triple.
Files:
src/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/util/hints.rssrc/cmd/setup.rssrc/state/snapshot.rssrc/nu/bootstrap.rssrc/cmd/try_cmd.rssrc/cmd/doctor.rs
src/cmd/{install,update,remove,activate,deactivate,init,doctor}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mutation commands must create a snapshot before changing managed state.
Files:
src/cmd/init.rssrc/cmd/doctor.rs
src/cmd/{setup,use_cmd}.rs
📄 CodeRabbit inference engine (AGENTS.md)
The active-version marker is the sole authority for the selected managed Nu version; write it after a pre-mutation snapshot and support the optional resolved
binary_pathfor off-tree selections.
Files:
src/cmd/setup.rs
src/state/snapshot.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Activation snapshots are immutable; snapshot operations must preserve immutable snapshot semantics.
Create a state snapshot before mutations to install, update, remove, activate, deactivate,
init --refresh, nupm import, and doctor repair.
Files:
src/state/snapshot.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write JSON state files atomically with
write_json_atomicusing a temporary file in the same directory followed by persist.
Files:
src/state/snapshot.rs
src/nu/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Managed Nushell files must contain
OWNERSHIP_MARKER; useassert_managed_file_ownedto prevent overwriting foreign files.
Files:
src/nu/bootstrap.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-03T01:55:38.489Z
Learning: Create a feature branch, implement with tests, run the full test suite, update AGENTS.md when conventions or structure change, and open a pull request with a description.
Learnt from: CR
Repo: tonythethompson/numan
Timestamp: 2026-08-03T01:55:38.489Z
Learning: Use imperative commit messages shorter than 72 characters, use `feature/description` or `fix/description` branch names, never force-push `master`, and squash-merge features.
🪛 markdownlint-cli2 (0.23.1)
docs/numan-doctor.md
[warning] 132-132: Table column count
Expected: 3; Actual: 7; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 179-179: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 269-269: Table column count
Expected: 2; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 289-289: Files should end with a single newline character
(MD047, single-trailing-newline)
🔍 Remote MCP GitHub Copilot
Additional review context
- The latest PR head still contains
StdoutToStderrwith process-global stdout redirection, explicit Unix/Windows implementations, Unix fork-isolated testing, and Windows restoration without closing the restored handle. - An unresolved review comment remains on
stdio_redirect.rs: it questions whetherPhantomData<*const ()>reliably enforces!Send + !Sync; it recommends a definitively non-thread-safe marker such asRc. - Another unresolved comment targets
tests/activate_test.rs:test_activate_succeeds_without_promptonly assertsresult.is_ok()and does not prove that a prompt callback was not invoked. The current test has no prompt-invocation seam/assertion. - Current CI results are broadly successful, including Windows/macOS/Linux tests, real-Nu acceptance, MSRV, Clippy, formatting, CodeQL, packaging, and static analysis. Rust analysis and Greptile review were still in progress when queried.
- The latest commits specifically document snapshot-gated repairs and record plugin-deactivation journal-load failures rather than aborting the repair pass.
🔇 Additional comments (13)
src/state/snapshot.rs (1)
67-78: LGTM!src/cmd/init.rs (1)
11-11: LGTM!Also applies to: 450-457
docs/snapshots-and-rollback.md (1)
48-49: LGTM!AGENTS.md (1)
57-57: LGTM!Also applies to: 112-112, 154-154
Cargo.toml (1)
79-86: LGTM!README.md (1)
47-47: LGTM!Also applies to: 190-190, 355-355, 435-435
src/cmd/try_cmd.rs (1)
293-311: LGTM!Also applies to: 450-472
src/util/hints.rs (1)
41-83: LGTM!Also applies to: 228-232, 297-309
src/cmd/doctor.rs (1)
1019-1411: LGTM!The scoped-snapshot handling (independent layout/marker/registry repairs proceed on
snapshot_ok == false, while nested-mutation repairs skip withsnapshot_unavailable) and thePendingPluginDeactivate::loaderror handling match the documented repair policy and the PR objective of not aborting the repair pass on journal-load failures.src/cmd/nu_pin_offer.rs (1)
17-85: LGTM!src/cmd/setup.rs (1)
17-22: LGTM!Also applies to: 366-366, 428-428, 464-465
tests/doctor_test.rs (1)
33-110: LGTM!Coverage now closes every previously-flagged gap (scoped snapshot failure handling, malformed-marker repair, off-PATH consent, no auto-install, JSON repairs contract).
Also applies to: 197-365, 479-557, 802-895
tests/module_autoload_test.rs (1)
1546-1548: 🩺 Stability & AvailabilityNo change required.
real_nu_windows_path_with_spaces_validatesalready includes#[ignore]and runs only on Windows.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nu/bootstrap.rs (1)
717-775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
create_snapshotcall into a local helper.Lines 720-728 and 768-775 both call
create_snapshot(root, SnapshotReason::PreMutation, SnapshotTrigger::Install, None, None)inside the same function,execute_nu_setup_with_installer, differing only in the context message. Extract a small local helper, similar tosetup.rs'ssnapshot_before_setup_mutation, to avoid the two call sites drifting out of sync if the trigger or reason changes later.♻️ Proposed refactor
+fn snapshot_before_install_mutation(root: &Path, context_msg: &str) -> Result<()> { + create_snapshot(root, SnapshotReason::PreMutation, SnapshotTrigger::Install, None, None) + .with_context(|| context_msg.to_string()) +} + ... if options.yes { // PATH persistence mutates user shell state; snapshot first. - create_snapshot( - root, - SnapshotReason::PreMutation, - SnapshotTrigger::Install, - None, - None, - ) - .context("Failed to create pre-mutation snapshot for existing `numan setup nu`")?; + snapshot_before_install_mutation( + root, + "Failed to create pre-mutation snapshot for existing `numan setup nu`", + )?; ... - create_snapshot( - root, - SnapshotReason::PreMutation, - SnapshotTrigger::Install, - None, - None, - ) - .with_context(|| "Failed to create pre-install snapshot for `numan setup nu`")?; + snapshot_before_install_mutation( + root, + "Failed to create pre-install snapshot for `numan setup nu`", + )?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nu/bootstrap.rs` around lines 717 - 775, In execute_nu_setup_with_installer, extract the shared create_snapshot invocation using SnapshotReason::PreMutation and SnapshotTrigger::Install into a local helper, following the pattern of snapshot_before_setup_mutation. Have the helper accept or apply the differing context message, then replace both existing call sites while preserving their current error context and behavior.docs/numan-doctor.md (1)
36-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale text on confirm-tier behavior contradicts this same file's repair policy.
Lines 36-39 state that confirm-tier repairs run only when stdin is a TTY, and non-interactive sessions skip them as
not_confirmed. This is not true anymore. The Repair Policy table (lines 85-96) states that confirm-tier findings, such asnu_paths.driftandjournal.plugin_pending, apply "Never (applied in default mode)" with no TTY gate.src/cmd/doctor.rsapply_repairsdoes not checkis_terminal()for these repairs, and thenot_confirmedstatus was removed from the code (per an earlier fixed review comment). Onlynu.binary.found_off_pathfunnels through a nestedsetup nu usecall, which is a separate consent mechanism.Update this paragraph to state that confirm-tier repairs apply automatically in default mode, matching the policy table below it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/numan-doctor.md` around lines 36 - 40, Update the default-mode behavior paragraph in numan-doctor.md to state that confirm-tier repairs are applied automatically in default mode without a TTY gate or not_confirmed outcome. Keep the separate nested setup nu consent behavior for nu.binary.found_off_path, and leave the --scan description unchanged.
🤖 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 130-133: Split the merged Markdown table entry after the
nu.active_version.malformed row into two distinct rows: keep its existing three
columns intact, then place nu_paths.missing and its description on a new row.
Ensure the table has exactly three columns per row and preserves the existing
nu_paths.drift row.
- Around line 266-270: Split the merged Markdown table entry after the `numan
activate` row so `numan registry sync` starts on its own row. Preserve both
commands and their existing descriptions while ensuring each row has exactly two
table columns.
In `@src/cmd/doctor.rs`:
- Around line 329-353: Update the Err branch in check_active_version_marker to
use the canonical hints::CMD_DOCTOR_FIX constant instead of the hardcoded "numan
doctor" string, adding the necessary import or qualified reference while
preserving the existing finding behavior.
In `@src/cmd/init.rs`:
- Around line 177-180: Move acquire_mutation_lock before loading NuPaths and
Lockfile in the init flow, including old_paths and Lockfile::load. Compute
has_active_modules and perform validation using the state loaded after the lock
is held, following activate.rs’s reload-after-lock pattern, so refresh cannot
save stale mutable state.
- Around line 484-516: The refresh safety tests around
refresh_without_active_packages_still_locks_and_snapshots are incomplete. Extend
coverage to verify the mutation lock blocks or serializes a concurrent writer,
assert the created Init snapshot has reason PreMutation, and add a failure-path
test proving a snapshot error prevents path writes. Use the existing runner,
lock, snapshot, and injectable seams rather than changing unrelated behavior.
- Around line 181-188: Extend the Init snapshot flow around create_snapshot to
capture NuPaths as a snapshot sidecar whenever numan init --refresh rewrites
nu_state/paths.json. Update snapshot restore logic to restore the saved NuPaths
alongside the lockfile, including when no package is active, and add a rollback
test covering that no-active-package case.
---
Outside diff comments:
In `@docs/numan-doctor.md`:
- Around line 36-40: Update the default-mode behavior paragraph in
numan-doctor.md to state that confirm-tier repairs are applied automatically in
default mode without a TTY gate or not_confirmed outcome. Keep the separate
nested setup nu consent behavior for nu.binary.found_off_path, and leave the
--scan description unchanged.
In `@src/nu/bootstrap.rs`:
- Around line 717-775: In execute_nu_setup_with_installer, extract the shared
create_snapshot invocation using SnapshotReason::PreMutation and
SnapshotTrigger::Install into a local helper, following the pattern of
snapshot_before_setup_mutation. Have the helper accept or apply the differing
context message, then replace both existing call sites while preserving their
current error context and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 186d44c2-631f-4e57-9eda-2c2773f3711a
📒 Files selected for processing (15)
AGENTS.mdCargo.tomlREADME.mddocs/numan-doctor.mddocs/snapshots-and-rollback.mdsrc/cmd/doctor.rssrc/cmd/init.rssrc/cmd/nu_pin_offer.rssrc/cmd/setup.rssrc/cmd/try_cmd.rssrc/nu/bootstrap.rssrc/state/snapshot.rssrc/util/hints.rstests/doctor_test.rstests/module_autoload_test.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Trackdubllc/Trackdub(manual)tonythethompson/QuickShell(manual)tonythethompson/numan(manual)tonythethompson/dependency-chain-substrate(manual)
📜 Review details
🔇 Additional comments (13)
src/state/snapshot.rs (1)
67-78: LGTM!src/cmd/init.rs (1)
11-11: LGTM!Also applies to: 450-457
docs/snapshots-and-rollback.md (1)
48-49: LGTM!AGENTS.md (1)
57-57: LGTM!Also applies to: 112-112, 154-154
Cargo.toml (1)
79-86: LGTM!README.md (1)
47-47: LGTM!Also applies to: 190-190, 355-355, 435-435
src/cmd/try_cmd.rs (1)
293-311: LGTM!Also applies to: 450-472
src/util/hints.rs (1)
41-83: LGTM!Also applies to: 228-232, 297-309
src/cmd/doctor.rs (1)
1019-1411: LGTM!The scoped-snapshot handling (independent layout/marker/registry repairs proceed on
snapshot_ok == false, while nested-mutation repairs skip withsnapshot_unavailable) and thePendingPluginDeactivate::loaderror handling match the documented repair policy and the PR objective of not aborting the repair pass on journal-load failures.src/cmd/nu_pin_offer.rs (1)
17-85: LGTM!src/cmd/setup.rs (1)
17-22: LGTM!Also applies to: 366-366, 428-428, 464-465
tests/doctor_test.rs (1)
33-110: LGTM!Coverage now closes every previously-flagged gap (scoped snapshot failure handling, malformed-marker repair, off-PATH consent, no auto-install, JSON repairs contract).
Also applies to: 197-365, 479-557, 802-895
tests/module_autoload_test.rs (1)
1546-1548: 🩺 Stability & AvailabilityNo change required.
real_nu_windows_path_with_spaces_validatesalready includes#[ignore]and runs only on Windows.> Likely an incorrect or invalid review comment.
🛑 Comments failed to post (3)
src/cmd/init.rs (3)
177-180: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Acquire the mutation lock before loading mutable state.
Lockfile::loadruns beforeacquire_mutation_lockat Line [180]. A concurrent mutating command can write a newer lockfile while refresh waits. Refresh then saves its stale copy at Line [205] and can erase that command’s changes.old_pathsat Line [168] has the same race.Acquire the lock before loading
NuPathsandLockfile, or reload both after the lock. Computehas_active_modulesand run validation from the locked state.src/cmd/activate.rsalready reloads its lockfile after lock acquisition.🤖 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/init.rs` around lines 177 - 180, Move acquire_mutation_lock before loading NuPaths and Lockfile in the init flow, including old_paths and Lockfile::load. Compute has_active_modules and perform validation using the state loaded after the lock is held, following activate.rs’s reload-after-lock pattern, so refresh cannot save stale mutable state.
181-188: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 12 \ 'SnapshotTrigger::Init|create_snapshot|paths\.json|NuPaths|rollback' \ src/state src/cmd tests docsRepository: tonythethompson/numan
Length of output: 50378
🏁 Script executed:
set -euo pipefail printf '%s\n' '--- candidate files ---' fd -t f '(snapshot|init|rollback|paths)' src tests docs | sort printf '%s\n' '--- snapshot symbols ---' rg -n 'enum SnapshotTrigger|SnapshotTrigger::Init|struct Snapshot|create_snapshot|NuPaths|paths\.json|restore' src/state/snapshot.rs src/state/rollback.rs src/cmd/init.rs docs/snapshots-and-rollback.md tests 2>/dev/null | head -n 240 printf '%s\n' '--- outlines ---' ast-grep outline src/state/snapshot.rs ast-grep outline src/state/rollback.rs ast-grep outline src/cmd/init.rsRepository: tonythethompson/numan
Length of output: 15366
🏁 Script executed:
set -euo pipefail printf '%s\n' '--- snapshot model and creation ---' sed -n '30,250p' src/state/snapshot.rs printf '%s\n' '--- snapshot capture helpers ---' sed -n '470,590p' src/state/snapshot.rs printf '%s\n' '--- rollback flow ---' sed -n '1,270p' src/state/rollback.rs printf '%s\n' '--- init refresh flow ---' sed -n '150,325p' src/cmd/init.rs printf '%s\n' '--- relevant tests ---' sed -n '330,540p' src/cmd/init.rs sed -n '520,790p' src/state/rollback.rs printf '%s\n' '--- documentation scope ---' sed -n '1,165p' docs/snapshots-and-rollback.mdRepository: tonythethompson/numan
Length of output: 50379
🏁 Script executed:
set -euo pipefail python3 - <<'PY' from pathlib import Path import re snapshot = Path("src/state/snapshot.rs").read_text() rollback = Path("src/state/rollback.rs").read_text() init = Path("src/cmd/init.rs").read_text() init_tests = Path("src/cmd/init.rs").read_text() docs = Path("docs/snapshots-and-rollback.md").read_text() snapshot_struct = re.search( r"pub struct Snapshot \{(?P<body>.*?)\n\}", snapshot, re.S ).group("body") create_body = re.search( r"pub fn create_snapshot\(.*?\n\}\n\n/// Load a snapshot", snapshot, re.S ).group(0) rollback_body = rollback.split("pub fn rollback_to_snapshot", 1)[1].split( "/// Commit the managed autoload file", 1 )[0] print("Snapshot fields:", re.findall(r"pub (\w+):", snapshot_struct)) print("Snapshot creation writes paths sidecar:", bool(re.search(r"paths\.json|NuPaths", create_body))) print("Rollback reads paths.json:", "paths.json" in rollback_body) print("Rollback saves NuPaths:", bool(re.search(r"\b(?:new_paths|nu_paths|paths)\.save\(root\)", rollback_body))) print("Rollback restored files:", re.findall(r'root\.join\("([^"]+)"\)', rollback_body)) print("Refresh writes paths.json via NuPaths::save:", bool(re.search(r"new_paths\.save\(root\)", init))) print("Refresh test asserts old paths are restored:", bool(re.search( r"rollback|old_paths|paths_v1\.nu_executable_hash", init_tests, re.I ))) print("Docs list paths.json as captured:", "paths.json" in docs) PYRepository: tonythethompson/numan
Length of output: 526
Capture and restore
NuPathsinInitsnapshots.numan init --refreshrewritesnu_state/paths.json, but snapshots store noNuPathssidecar and rollback never restores it. Rollback can therefore leavepaths.jsoninconsistent with the restored lockfile. Extend the snapshot schema and add a no-active-package rollback test.🤖 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/init.rs` around lines 181 - 188, Extend the Init snapshot flow around create_snapshot to capture NuPaths as a snapshot sidecar whenever numan init --refresh rewrites nu_state/paths.json. Update snapshot restore logic to restore the saved NuPaths alongside the lockfile, including when no package is active, and add a rollback test covering that no-active-package case.
484-516: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Extend the refresh test to cover the full safety contract.
refresh_without_active_packages_still_locks_and_snapshotschecks only the updated paths and the presence of anInitsnapshot. It does not prove that the mutation lock serializes a concurrent writer, that the snapshot reason isPreMutation, or that a snapshot failure prevents writes.Add a lock-contention test or injectable lock seam. Assert the snapshot metadata. Add the snapshot-failure test.
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/init.rs` around lines 484 - 516, The refresh safety tests around refresh_without_active_packages_still_locks_and_snapshots are incomplete. Extend coverage to verify the mutation lock blocks or serializes a concurrent writer, assert the created Init snapshot has reason PreMutation, and add a failure-path test proving a snapshot error prevents path writes. Use the existing runner, lock, snapshot, and injectable seams rather than changing unrelated behavior.Source: Coding guidelines
Use CMD_DOCTOR_FIX for the active-version marker hint, reload NuPaths and lockfile after the init --refresh mutation lock, harden refresh snapshot tests, extract bootstrap PreMutation snapshot helper, and align doctor default-mode docs with confirm-tier auto-apply. Co-authored-by: Anthony Thompson <github@trackdub.com>
Summary
Doctor UX follow-up on top of
refactor/ux-improvements: repair by default,--scanreport-only, no doctor--fix/--yes, soft-fail PreMutation Doctor snapshots, off-PATH viasetup nu usewithout--yes.Review finding triage (
ceb00f1)Fixed:
doctor.rs:check_active_version_markerErr hint useshints::CMD_DOCTOR_FIXinit.rs: mutation lock before NuPaths/Lockfile reload; validate from post-lock stateinit.rstests: PreMutation reason assert, concurrent lock contention, snapshot failure does not write pathsbootstrap.rs: sharedsnapshot_before_nu_setuphelper for both PreMutation call sitesdocs/numan-doctor.md: default mode applies confirm-tier without TTY/not_confirmedgate (nestedsetup nu useconsent unchanged;--scanunchanged)Skipped (already fixed):
docs/numan-doctor.md(active_version / activate+registry sync)Skipped (out of minimal scope):
paths.jsonas a snapshot sidecar on Init/rollback: would expand snapshot schema + rollback commits beyond documented snapshot scope (docs/snapshots-and-rollback.md); Nu identity hash/version is already recorded when availableTest plan
cargo fmt/cargo clippy -- -D warningscargo test --lib 'cmd::init::'cargo test --lib 'cmd::doctor::tests::doctor'cargo test --test doctor_test --test init_testcargo test --lib 'nu::bootstrap'