Fix doctor managed Nu reporting and improve registry packages listing - #106
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesThe PR adds persistent activation profiles per Nu minor and shared orchestration for cross-minor switching. It updates activation, deactivation, removal, Activation Profiles
Managed Nu reporting
Registry output
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Greptile SummaryThe PR improves managed Nu diagnostics and package-list presentation while making local activation state updates safer.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/cmd/doctor.rs | Resolves versioned managed Nu binaries and maps resolution errors to more appropriate diagnostic guidance. |
| src/cmd/registry.rs | Documents the existing terminal-width and display-width-aware description wrapping behavior; both previously reported failures are resolved. |
| src/cmd/use_cmd.rs | Refreshes cached Nu paths after repairing a dangling off-tree active binary marker. |
| src/cmd/remove.rs | Moves activation-profile cleanup ahead of destructive lockfile and payload changes. |
| src/cmd/activation_switch.rs | Exposes cached Nu-path refresh within the crate for reuse by the use command. |
Reviews (9): Last reviewed commit: "fix: address Codex review findings on PR..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cmd/deactivate.rs (1)
1557-1573: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTest the retain-on-failure invariant, not only the clear-on-success path.
This test covers the reclassified skip. The new selective-clear block at lines 231-249 has no test. Its whole purpose is to keep profile desire when a lane fails, and that branch is currently unverified.
deactivate_already_inactive_clears_profilein src/cmd/activation_switch.rs lines 1187-1222 covers only the opposite direction.Add a test using
PluginTestEnvwith a failing unregistrar: seed an active plugin, seed profile desire for its minor, runexecute_with_unregistrarwith a closure that bails, then assert the profile still contains the id.As per coding guidelines: "Tests must cover failure modes, not only successful execution."
🤖 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/deactivate.rs` around lines 1557 - 1573, Add a failure-path test near inactive_module_is_skipped_idempotently using PluginTestEnv: seed an active plugin and profile desire for its minor, run execute_with_unregistrar with an unregistrar closure that fails, then assert the profile still contains the plugin id. Verify the retain-on-failure behavior rather than only the successful selective-clear path.Source: Coding guidelines
src/cmd/activate.rs (1)
244-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFailed activations are still recorded as desired state.
Line 245 reloads the lockfile so profile sync sees post-activation state.
sync_user_activate_profilethen discards that intent for the explicit-package case: line 269 passesargs.packagesverbatim, andsync_profile_after_user_activate(src/cmd/activation_switch.rs:535-553) only readspackage_type, never activation state. Line 246 also runs before theany_failedbail at line 248.Result: a package that failed its lane is written into
activation-profile.json. Every laternuman usethen attempts to restore it and reports a restore failure.This is the opposite of the deactivate side. src/cmd/deactivate.rs lines 237-249 deliberately verifies each id is now inactive before clearing desire. Apply the same discipline here: record desire only for packages the reloaded lockfile confirms as active.
🐛 Proposed fix — verify post-activation state before recording desire
fn sync_user_activate_profile( root: &Path, nu_paths: &NuPaths, args: &ActivateArgs, lockfile: &Lockfile, ) -> Result<()> { - let ids = if args.packages.is_empty() { - let active = crate::cmd::activation_switch::collect_currently_active(lockfile, nu_paths); - let mut all = active.plugins; - all.extend(active.modules); - all - } else { - args.packages.clone() - }; + let active = crate::cmd::activation_switch::collect_currently_active(lockfile, nu_paths); + let mut ids: Vec<String> = active.plugins; + ids.extend(active.modules); + if !args.packages.is_empty() { + // Only record desire for requested packages that are actually active now. + ids.retain(|id| args.packages.contains(id)); + } crate::cmd::activation_switch::sync_profile_after_user_activate( root, &nu_paths.nu_version, &ids, lockfile, ) }🤖 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/activate.rs` around lines 244 - 277, Update the activation flow around sync_user_activate_profile and the any_failed check so failed activations are not persisted as desired state. Before calling sync_profile_after_user_activate, derive the post-activation active packages from the reloaded lockfile and retain only requested IDs confirmed active, including for explicit args.packages; preserve the existing all-packages behavior. Ensure the any_failed bail occurs before profile synchronization, or otherwise guarantee failed IDs cannot reach the sync call.
🤖 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/cmd/activate.rs`:
- Around line 601-638: The unlocked mutating primitives expose lock-safety only
through documentation; narrow activate_modules_unlocked in
src/cmd/activate.rs:601-638 and deactivate_modules_unlocked in
src/cmd/deactivate.rs:405-458 to pub(crate), or require a &MutationLock
parameter so callers must prove the root mutation lock is held. Apply the same
enforcement to both functions without changing their existing unlocked behavior.
In `@src/cmd/activation_switch.rs`:
- Around line 838-903: Update teardown_order_modules_then_plugins so the module
lane records its execution before delegating to FakeCandidateRunner: add a local
CandidateRunner wrapper around the runner that pushes "module" into order in
run, then invokes the inner runner. Pass this wrapper through SwitchHooks and
change the assertion to require ["module", "plugin"], preserving the existing
lockfile checks.
- Around line 256-269: Update try_load_paths to check whether
nu_state/paths.json exists before calling NuPaths::load, then perform exactly
one load when the file exists and propagate its error; return Ok(None) only when
the file is absent, avoiding the current initial load, discarded error, and
TOCTOU-prone second load.
- Around line 407-439: Update restore_desired and its per-plugin call to pass
the already-validated &NuPaths into restore_one_plugin. Change
restore_one_plugin to accept that reference and remove its NuPaths::load(root)
block, using the forwarded value for is_active_for so all restore decisions
share the validated snapshot.
- Around line 359-405: Refactor classify_restore_target to use one shared
registry lookup, version match, and resolver.is_compatible check for both
plugins and modules. Keep the existing resolver.has_compatible_version(&pkg)
fallback only when expected_type is "plugin" and the installed version is
absent; preserve the current module behavior and RestoreClass results.
In `@src/cmd/doctor.rs`:
- Around line 2343-2379: The test coverage around
doctor_reports_versioned_managed_nu only exercises a present versioned binary.
Add a failure-path test with an active-version marker pointing to an absent
binary, then assert the nu.managed.version finding has Severity::Warn and
RepairTier::Manual, and does not report “Managed Nu: not installed.”
In `@src/cmd/registry.rs`:
- Around line 246-283: Extract the terminal-column to description-width
calculation from the registry formatting flow into a named helper that accepts
the column count, then use it at the existing call site. Extend the tests around
wrap_words with a narrow-terminal boundary case (below 44 columns) and a
non-ASCII description whose display width differs from byte length, asserting
the calculated width and wrapping behavior without querying the real terminal.
- Around line 227-233: Update wrap_words to measure terminal display columns
rather than UTF-8 byte lengths: use the existing console/unicode-width
display-width helper for each word and the accumulated current line, including
the separator space, when comparing against width. Preserve the current wrapping
behavior for ASCII text and word ordering.
- Around line 210-218: Update package_description_width() so the 40-column
minimum is applied only when the available width after subtracting the
four-space prefix is at least 40; otherwise return the actual saturated
available width without forcing it to 40.
In `@src/cmd/remove.rs`:
- Around line 164-165: Handle errors from remove_from_all_minors in the removal
flow by emitting a warning and continuing instead of propagating them with ?.
Match the existing payload-delete warning pattern around the completed removal
path, while preserving successful cleanup behavior and allowing stale profile
IDs to be treated as non-fatal.
In `@src/cmd/use_cmd.rs`:
- Around line 32-74: Add an integration-style test in the tests for
execute_with_hooks_and_refresh that seeds an active plugin and lockfile, injects
counting registrar, unregistrar, and path_refresh hooks, and invokes the real
dispatch entry point. Assert the unregistrar executes before the active-version
marker changes, the registrar and path refresh are invoked as expected, and the
switch completes successfully. This should exercise hook assembly, lock
ordering, snapshot-before-switch, and dispatch rather than calling
switch_active_nu_version directly.
In `@src/state/activation_profile.rs`:
- Around line 226-305: Add failure-path tests alongside the existing
ActivationProfile tests for the load method: write a profile with an unsupported
schema_version and assert the returned error mentions schema_version, then write
malformed JSON and assert the formatted error contains “Malformed activation
profile.” Use ActivationProfile::profile_path and temporary directories,
preserving the existing success tests.
- Around line 188-201: Update the doc comments for ensure_contains_for_paths and
ensure_absent_for_paths to state that an unresolvable Nu version returns an
error, matching nu_minor_key_from_version and the propagated Result behavior; do
not describe these cases as no-ops.
---
Outside diff comments:
In `@src/cmd/activate.rs`:
- Around line 244-277: Update the activation flow around
sync_user_activate_profile and the any_failed check so failed activations are
not persisted as desired state. Before calling sync_profile_after_user_activate,
derive the post-activation active packages from the reloaded lockfile and retain
only requested IDs confirmed active, including for explicit args.packages;
preserve the existing all-packages behavior. Ensure the any_failed bail occurs
before profile synchronization, or otherwise guarantee failed IDs cannot reach
the sync call.
In `@src/cmd/deactivate.rs`:
- Around line 1557-1573: Add a failure-path test near
inactive_module_is_skipped_idempotently using PluginTestEnv: seed an active
plugin and profile desire for its minor, run execute_with_unregistrar with an
unregistrar closure that fails, then assert the profile still contains the
plugin id. Verify the retain-on-failure behavior rather than only the successful
selective-clear path.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3d260d3-32e6-4cf7-a046-9ba64c95fef3
📒 Files selected for processing (13)
AGENTS.mdCHANGELOG.mdREADME.mdsrc/cmd/activate.rssrc/cmd/activation_switch.rssrc/cmd/deactivate.rssrc/cmd/doctor.rssrc/cmd/mod.rssrc/cmd/registry.rssrc/cmd/remove.rssrc/cmd/use_cmd.rssrc/state/activation_profile.rssrc/state/mod.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
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: Real-Nu acceptance (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (13)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Use Serena's semantic, symbol-aware tools as the primary tools for reading and editing code; use built-in Read, Glob, Grep, and Edit only under the stated exceptions.
Before editing a code file, inspect its symbol overview, read the specific symbols being changed, and edit them with Serena's symbol-aware tools.
Understand the existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, unnecessary error handling, feature flags, and compatibility shims.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a 2–3 sentence recommendation with the main tradeoff and do not implement until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Address security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, force-pushing, modifying CI/CD, posting externally, or uploading content.
When blocked, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration before deleting them.
Only commit when explicitly asked; do not proactively update git configuration or push changes.
Do not skip Git hooks unless explicitly asked; if a pre-commit hook fails, fix the issue, re-stage, and create a new commit rather than amending.
Stage files by name rather than using git add -A or git add ., to avoid accidentally including secrets or large binaries.
Use a HEREDOC for commit messages, and add co-author attribution only when the user explicitly requests the exact trailer.
Do not force-pus...
Files:
README.mdCHANGELOG.mdsrc/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rsAGENTS.mdsrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
!**/.env,!**/credentials.json,!**/*.pem
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit files that appear to contain secrets, including .env, credentials.json, and PEM files; warn before doing so even if explicitly requested.
Files:
README.mdCHANGELOG.mdsrc/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rsAGENTS.mdsrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Match existing naming, module layout, and documentation level in the file being edited; update
AGENTS.md,docs/, or command help when structure, conventions, or user-visible behavior changes.Tests must cover failure modes, not only successful execution.
Files:
README.mdCHANGELOG.mdsrc/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rsAGENTS.mdsrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
README.mdCHANGELOG.mdsrc/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rsAGENTS.mdsrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
**/*.md
📄 CodeRabbit inference engine (REVIEW.md)
Update documentation and
AGENTS.mdwhen project structure or conventions change.
Files:
README.mdCHANGELOG.mdAGENTS.md
**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Add comments only when the WHY is non-obvious; do not narrate what the code does, reference the current task, or reference the PR in comments.
Files:
src/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rssrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rust 2021 edition.
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.
Library code must not panic; error paths should returnanyhow::Resultwith context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock viaacquire_mutation_lock(root)and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must usewrite_json_atomic.
numan installmust write only to$NUMAN_ROOT; it 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 must not be authoritative.
Install payloads under versioned, content-addressed paths and never overwrite them 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.
**/*.rs: All CI gates must pass:cargo test,cargo clippy -- -D warnings, andcargo fmt --check.
Every mutating command—includinginstall,remove,update,gc, and futurenupm import—must callacquire_mutation_lock(root).
Lockfiles, journals, and state files must usewrite_json_atomic; partial writes are not allowed.
Pending activation, autoload, and lifecycle journals must be stored under$NUMAN_ROOT/state/.
Module autoload identity must match all four fields: Nu executable hash, Nu version, vendor autoload directory, and managed file path; the lockfilemodule_activationvalue is authoritative.
Never overwrite foreign autoload files; respectOWNERSHIP_MARKER.
Pass paths to Nu only throu...
Files:
src/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rssrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rssrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
**/*.{rs,nu}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{rs,nu}: Real-Nu acceptance tests must be marked#[ignore]and should be run when changes affect activation or nupm import; unit tests must not spawn realnuand should use injectable seams such asFakeCandidateRunneror registrars.
The nupm integration must be read-only towardNUPM_HOME, must not executebuild.nu, and must not perform bidirectional synchronization.Unit tests must use
FakeCandidateRunneror injectable registrars and must not spawn a realnuprocess.
Files:
src/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rssrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
src/state/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/state/**/*.rs: Usewrite_json_atomicfor all JSON state files, including lockfiles, journals, and Nu state, to prevent partial-write corruption.
The lockfile is authoritative for module activation;autoload-state.jsonis only a derived fast-check projection.
Acquireacquire_mutation_lock(root)before serialized mutations; a second acquisition must fail immediately.
Files:
src/state/mod.rssrc/state/activation_profile.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Nu paths and names through environment variables (
NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG, andNUMAN_PLUGIN_NAME); the Nu program string must be a compile-time constant with no runtime interpolation.
Files:
src/state/mod.rssrc/cmd/mod.rssrc/cmd/remove.rssrc/cmd/doctor.rssrc/cmd/activate.rssrc/cmd/registry.rssrc/state/activation_profile.rssrc/cmd/use_cmd.rssrc/cmd/deactivate.rssrc/cmd/activation_switch.rs
src/cmd/{activate,deactivate,plugin_lifecycle}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Nu plugin registration and unregistration must be owned by the activate/deactivate lifecycle boundary; opted-in update orchestration may coordinate it but must not invoke callbacks directly.
Files:
src/cmd/activate.rssrc/cmd/deactivate.rs
src/cmd/{activate,deactivate}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Inject registrar, unregistrar, and candidate-runner dependencies through the
execute_with_*APIs instead of invoking real Nu binaries in unit tests.
Files:
src/cmd/activate.rssrc/cmd/deactivate.rs
🔍 Remote MCP GitHub Copilot
Relevant review context
- PR
#106currently contains 3 commits, 13 files, +2,022/-134 lines. The PR body still describes only doctor/registry changes, while the latest commit adds the large activation-profile/version-switching implementation. - Earlier PR
#67explicitly listed per-version activation sets and reconciliation as future work; PR#41established that activations remain Nu-specific. This PR implements that deferred behavior. - Lockfile activity is identity-bound: plugins require matching Nu hash/version/registry path; modules additionally require matching vendor-autoload and managed-file paths.
NuPaths::validate_drift()also verifies the Nu binary hash and registry parent before lifecycle operations. - Four unresolved review threads are already present:
- registry wrapping can exceed narrow terminal width because of
.max(40); String::len()counts UTF-8 bytes rather than terminal columns;- doctor presents every resolver error as an “active managed binary” problem and always suggests
numan use, including filesystem-scan errors.
- registry wrapping can exceed narrow terminal width because of
- At retrieval time, formatting, Clippy, Deny, packaging, Linux/macOS tests, MSRV, and Linux/macOS real-Nu acceptance had passed; Windows tests, macOS analysis, Rust analysis, and Greptile were still running. The PR was marked
mergeable_state: blocked.
🔀 Multi-repo context tonythethompson/QuickShell, tonythethompson/dependency-chain-substrate
Linked repositories findings
tonythethompson/QuickShell
- Nushell integration is generic terminal launching:
TerminalLauncher.cs:343-346detects Nushell profiles and builds anu -ccommand viaTerminalLauncherArgs.cs:62-70; it does not referencenuman, activation profiles, or version switching. [::tonythethompson/QuickShell::]
tonythethompson/dependency-chain-substrate
- No references to
numan, Nushell, activation profiles, or the changed CLI behavior were found. [::tonythethompson/dependency-chain-substrate::]
🔇 Additional comments (17)
src/cmd/doctor.rs (2)
520-526: Existing finding: classify resolver errors by source.An unresolved review thread already covers the generic active-binary message and unconditional
CMD_USEremediation in this branch.Source: MCP tools
547-552: 🎯 Functional CorrectnessNo change needed.
list_installed_versionsonly adds versioned directories that containnu_binary_name(), solatest_installed_versionreturns a version whose binary is already checked on disk.src/cmd/registry.rs (1)
183-208: LGTM!src/state/activation_profile.rs (1)
15-186: LGTM!src/state/mod.rs (1)
1-1: LGTM!AGENTS.md (1)
69-70: LGTM!Also applies to: 88-88, 152-152
src/cmd/activate.rs (1)
141-142: LGTM!Also applies to: 182-182
src/cmd/deactivate.rs (3)
128-128: LGTM!Also applies to: 166-166, 189-199, 266-281
231-255: Good boundary discipline here.This block verifies post-mutation lockfile state before clearing desire, and it runs before the
any_failedbail at line 257. A failed deactivation therefore keeps its profile entry, so a laternuman usewill retry the restore. That is the correct desired-state semantic. This is the discipline the activate side is missing; see the comment on src/cmd/activate.rs lines 244-277.
481-482: LGTM!Also applies to: 572-573
src/cmd/activation_switch.rs (2)
23-121: LGTM!Also applies to: 148-254, 441-532, 534-611
760-835: LGTM!Also applies to: 905-1284
src/cmd/mod.rs (1)
2-2: LGTM!src/cmd/use_cmd.rs (2)
7-19: LGTM!Also applies to: 94-95
122-144: Sequencing is correct here.The self-heal write at line 137 lands before
switch_active_nu_versionreads the marker, so the same-target branch sees a repaired on-tree selection. The mutation lock and the PreMutation snapshot are both already established bysetup_subcommand_lockat line 77 andcreate_snapshotat line 81 before this function runs.README.md (1)
357-358: LGTM!CHANGELOG.md (1)
14-14: LGTM!Also applies to: 19-19, 23-23
Fixes Applied SuccessfullyFixed 7 file(s) based on 9 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Doctor still looked for the legacy tools/nushell/nu path, so versioned setup nu installs were reported as missing. Co-authored-by: Cursor <cursoragent@cursor.com>
Style package ids, soft-wrap dim descriptions, and separate entries with blank lines. Co-authored-by: Cursor <cursoragent@cursor.com>
Remember per-Nu-minor activation desire so cross-minor switches leave and restore plugins/modules without shrinking leave profiles. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fixed 7 file(s) based on 9 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- Rebased onto latest master to resolve merge conflicts - Cleaned up activation_switch.rs per review (simplified logic) - Fixed use_cmd.rs per review comments - Cleaned up activation_profile.rs per review Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
3d764f0 to
34cd479
Compare
|
@codex review |
![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: af80c14c59
ℹ️ 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".
Filter activate profile sync to packages that are active, retain profile desire when deactivate fails, tighten use switch ordering assertions, and document ensure_absent_for_paths error behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Verified each review finding against current Fixed
Already fixed / skipped
|
Reserve the numan use hint for dangling active markers; filesystem and marker parse failures get a permissions/reinstall message instead. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed misleading
|
d5c8aef
…nu-versioned # Conflicts: # AGENTS.md # CHANGELOG.md # src/cmd/activate.rs # src/cmd/activation_switch.rs # src/cmd/deactivate.rs # src/cmd/doctor.rs # src/cmd/registry.rs # src/cmd/use_cmd.rs # src/state/activation_profile.rs
P1 (use_cmd.rs): refresh cached paths.json after the dangling off-tree binary_path repair in execute_latest. Without this, the same-target reconcile loads the stale cache and validate_drift fails after the marker has already been rewritten. Added regression test test_use_latest_self_heal_clears_stale_paths_cache. P2 (remove.rs): move activation-profile cleanup to a preflight before lockfile/payload removal so a profile-write failure aborts the remove while state is still intact and retryable. Removed the duplicate dead warning-only call. P1 (activation_profile snapshots/rollback): already addressed by #108; snapshot capture and rollback restore are in place. No code change. Also applies cargo fmt to doctor.rs (Format CI was failing).
06b1d7b
1 participant
Summary
numan doctor: detect versioned managed Nu installs (tools/nushell/<version>/nu) instead of only the legacytools/nushell/nupath (fixes false “Managed Nu: not installed” aftersetup nu)numan registry packages: blank line between entries, cyan/bold package id, dim version/type/description, soft-wrap descriptions on word boundariesTest plan
cargo test --lib cmd::doctor::tests::doctor_reports_versionedcargo test --lib cmd::registrycargo clippy -- -D warningsnuman setup nuon a versioned root:numan doctorshowsManaged Nu: x.y.z (.../tools/nushell/x.y.z/nu.exe)numan registry packagesis scannable (styled headers, wrapped descriptions, spacing)Made with Cursor