Add warning for PATH Nu version mismatch and improve tests - #119
Conversation
Adds a Warn-severity finding 'nu.version_mismatch' when both PATH Nu and managed Nu are detected but report different versions. Explains that plugins are ABI-locked and packages installed under one won't load in the other. 30 doctor unit tests pass.
![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
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe doctor command retains successfully probed PATH and managed Nu versions. When both versions exist and differ, it emits a ChangesNu environment diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Greptile SummaryThe PR adds report-only detection of incompatible PATH and managed Nushell major/minor versions.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported contract-script permission issue is fixed at current HEAD.
|
| Filename | Overview |
|---|---|
| src/cmd/doctor.rs | Adds major/minor compatibility comparison, a report-only mismatch warning, report ordering, and focused tests; no eligible follow-up issue remains. |
| docs/numan-doctor.md | Documents the mismatch finding, patch-version compatibility, repair policy, and manual remediation consistently with the implementation. |
Reviews (6): Last reviewed commit: "Potential fix for pull request finding" | Re-trigger Greptile
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. |
![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
https://github.com/tonythethompson/numan/blob/a7a6c3bcb337fd7a599365ca599e27cb49c74f45/scripts/bump-contract.sh#L1
On Unix, this mode-only change makes the documented direct command scripts/bump-contract.sh ... fail with permission denied (exit 126). The same regression affects push-homebrew-tap.sh and sign-sha256sums.py; restore mode 100755 on all three scripts.
ℹ️ 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".
a7a6c3b to
8fea7a2
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/doctor.rs`:
- Around line 467-481: Run cargo fmt to format the changes in the doctor command
code, then verify cargo fmt --check passes without modifying the intended
behavior around find_nu_on_path and probe_nu_version.
- Around line 535-546: Add doctor tests covering Nu version mismatch detection:
verify distinct incompatible major/minor PATH and managed versions produce
exactly one Warn-severity finding with code nu.version_mismatch, and verify
versions differing only by patch do not produce that warning. Reuse the existing
doctor test setup and finding assertions.
- Around line 535-546: Update the finding-ID allowlist in print_report to
include nu.version_mismatch, ensuring normal numan doctor output displays this
warning while preserving the existing JSON and human-output behavior for other
findings.
- Around line 535-546: Document the new nu.version_mismatch warning and its
remediation guidance in the repository’s existing user-facing documentation or
command-help location, including the PATH-versus-managed Nu version mismatch and
the numan setup nu/numan activate remedies. Update AGENTS.md or the relevant
docs/help source rather than changing the diagnostic implementation in
src/cmd/doctor.rs.
- Around line 532-545: Update the version comparison in the PATH-versus-managed
Nu check to use the documented major/minor compatibility identity or existing
compatibility helper, so patch-only differences do not trigger
nu.version_mismatch. Remove or revise the nearby comment so it describes only
the non-obvious compatibility rationale, and update the mismatch guidance to
explain compatible versions while offering the documented managed-Nu pinning
flow.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94bc8b52-5953-46ce-8395-f48f8537c81d
📒 Files selected for processing (1)
src/cmd/doctor.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. (5)
- GitHub Check: Greptile Review
- GitHub Check: Real-Nu acceptance (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Format
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 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:
src/cmd/doctor.rs
**/*.{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/cmd/doctor.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:
src/cmd/doctor.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: Use&Pathrather than&PathBufin Rust function parameters.
Useanyhow::Resultfor application code,thiserrorfor library errors, and add context with.context(...)or?.
Never panic in library code; return errors instead.
Test-first development is expected: write a failing test, implement the change, then verify it passes.
Format and lint Rust code withcargo fmt --checkandcargo clippy -- -D warnings; no warnings are permitted.
**/*.rs: All CI gates must pass:cargo test,cargo clippy -- -D warnings,cargo fmt --check, MSRVcargo +1.88 check --locked --all-targets,cargo package --locked,cargo deny, and ignored real-Nu acceptance tests.
numan installmust not invoke...
Files:
src/cmd/doctor.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/cmd/doctor.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.
Files:
src/cmd/doctor.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.
Files:
src/cmd/doctor.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
src/cmd/doctor.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use compile-time
#[cfg(target_env)]platform detection rather thanstd::env::consts.
Files:
src/cmd/doctor.rs
src/cmd/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Create a snapshot before mutations performed by install, update, remove, activate, deactivate, nupm import, and
init --refresh.
Files:
src/cmd/doctor.rs
🔍 Remote MCP GitHub Copilot
Review-relevant context
- The current PR head changes only
src/cmd/doctor.rs(+41/−14); the PR description’s claims about CI, tests, and other files do not match the actual diff., nu.version_mismatchis added to findings but omitted fromprint_report’s human-output allowlist. It will appear in JSON, but not normalnuman doctoroutput.- The warning compares complete version strings, while the repository documents plugin ABI compatibility as Nu-major/minor scoped and supports exact-minor constraints. Patch-only differences may therefore trigger a misleading warning.,
- No test currently exercises a mismatch: the test probe returns the same version for PATH and managed Nu, and code search found no
nu.version_mismatchtest., - Current checks show
Formatfailed; Linux/macOS tests, Clippy, Deny, and Ubuntu acceptance passed, while several checks remain in progress.
Fixes Applied SuccessfullyFixed 1 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Dismissing stale CHANGES_REQUESTED review: every finding here was confirmed addressed in-thread by CodeRabbit itself and by the PR author (commit 7d542b4). All review threads on this PR are now resolved.
There was a problem hiding this comment.
Pull request overview
This PR enhances numan doctor to warn when the Nushell binary found on PATH is ABI-incompatible (major/minor mismatch) with the managed Nushell version under the Numan root, and updates the doctor spec documentation to describe the new finding and remediation guidance.
Changes:
- Add a new
nu.version_mismatchWarn finding when both PATH Nu and managed Nu are present but major/minor versions differ. - Introduce a
versions_compatiblehelper (major/minor comparison; patch differences treated as compatible). - Document the new finding and recommended user actions in
docs/numan-doctor.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/cmd/doctor.rs | Adds PATH-vs-managed Nu mismatch detection and reporting, including a major/minor compatibility helper and unit tests. |
| docs/numan-doctor.md | Documents the new nu.version_mismatch finding in both the repair policy table and check catalog. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
PR Summary by Qodo
Warn on PATH vs managed Nu version mismatch; expand unit tests; add CI coverage job
✨ Enhancement🧪 Tests⚙️ Configuration changes🕐 40+ MinutesAI Description
Diagram
graph TD U(("User")) --> D["doctor command"] --> PV["Probe PATH Nu"] --> MV["Probe managed Nu"] --> C{"Versions differ?"} C -->|"yes"| W[/"Warn: nu.version_mismatch"/] C -->|"no"| I[/"Info findings only"/] subgraph Legend direction LR _actor(("Actor")) ~~~ _proc["Command/step"] ~~~ _dec{"Decision"} ~~~ _out[/"Finding"/] endHigh-Level Assessment
1. Compare only Nu major/minor (semver-aware)
2. Report mismatch as Info with actionable remediation
Files changed (11)
+488 / -14· 3 not countedEnhancement (1)
+41 / -14doctor.rs
Warn when PATH Nu version differs from managed Nu+41/-14Tests (5)
+420 / -0list.rs
Add unit tests for list command output paths+93/-0nu_pin_offer.rs
Add unit tests for Nu pin offer interaction flow+68/-0registry.rs
Expand registry command test coverage+133/-0snapshot.rs
Add unit tests for snapshot listing/inspection helpers+96/-0download.rs
Add unit tests for local and file:// download paths+30/-0Other (5)
+27 / -0ci.yml
Add informational coverage job using cargo-llvm-cov+25/-0.gitignore
Ignore llvm-cov profraw artifacts+2/-0bump-contract.sh
Normalize script file permissionsnot countedpush-homebrew-tap.sh
Normalize script file permissionsnot countedsign-sha256sums.py
Normalize script file permissionsnot counted