Add cmd:: unit tests and CI coverage job - #120
Conversation
…er; add CI coverage job Adds in-process #[cfg(test)] coverage for previously-0%/low-coverage cmd entry points: cmd::list (0% -> 99%), cmd::nu_pin_offer (0% -> 71%), cmd::registry (34% -> 82%, add/remove/list, no network), cmd::snapshot (26% -> 73%, list/inspect/short_hash), and install::download's local file-path copy branch (19% -> 42%). Adds a coverage job to ci.yml using cargo-llvm-cov (informational only, no fail-under-lines threshold), reporting to the job summary.
![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: 3 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 (4)
📝 WalkthroughWalkthroughThe pull request adds Ubuntu workspace coverage reporting, injectable output handling for several commands, validation fixes for Nu versions and local downloads, test environment restoration, and expanded tests for command behavior and persisted state. ChangesCommand output and state flows
Validation and test isolation
Coverage pipeline
Estimated code review effort: 4 (Complex) | ~60 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 QodoAdd unit tests for cmd entrypoints and a CI coverage summary job
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Greptile SummaryAdds unit-test coverage across command, configuration, version-resolution, download, and snapshot paths, plus an informational CI coverage job.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| .github/workflows/ci.yml | Adds a non-gating cargo-llvm-cov job that publishes its summary to the GitHub Actions job summary. |
| src/cmd/list.rs | Routes list output through an injectable writer and adds empty, installed, and activated package tests. |
| src/cmd/registry.rs | Makes registry listing output injectable and adds coverage for listing, adding, removing, trust keys, and cached indexes. |
| src/cmd/search.rs | Makes search output injectable and adds fixture-backed end-to-end coverage for empty, matching, and incompatible results. |
| src/cmd/snapshot.rs | Routes snapshot list and inspect output through writers and adds snapshot creation, listing, inspection, and payload verification tests. |
| src/core/nu_version.rs | Makes malformed comparator bounds fail closed and correctly distinguishes exact zero-major versions from minor wildcards. |
| src/install/download.rs | Parses file URLs with the url crate and tests both plain local paths and platform-appropriate file URLs. |
| src/util/test_paths.rs | Adds an RAII guard and mutex for restoring and serializing tests that mutate NUMAN_ROOT. |
Reviews (7): Last reviewed commit: "Address remaining bot review findings" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 189e20c555
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo FixerNo findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page. |
- config.rs: load/save round trip, missing-file default, malformed TOML, serde defaults, NUMAN_ROOT env override - core/nu_version.rs: parse() error branches, matches_constraint's >, <, and =exact-version operators, from_paths_or_detect cached-version path - cmd::remove: full execute_with_tty happy path (lockfile entry removed, payload deleted) on top of the existing guard-path tests util::stdio_redirect.rs's remaining gap is the Windows-only code path, not exercisable on this platform — left as-is.
- cmd::search: 70% -> 91% (execute() end-to-end via a fixture registry index, covering the no-match/found/--all paths) - cmd::nupm: 61% -> 80% (diff/import/inspect argument-validation bails, status/inspect --all happy paths via the nupm-home-layout fixture)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.github/workflows/ci.yml:
- Line 64: Update the actions/checkout step in the coverage job to set
persist-credentials to false, while preserving the existing pinned action
reference and version comment.
- Line 75: Update the coverage-gating comment in the CI workflow to clarify that
although no line-coverage threshold is enforced, a nonzero exit from cargo
llvm-cov --workspace --summary-only still fails the Bash step and gates the
build.
- Line 81: Update the cargo llvm-cov coverage command in the CI workflow to
include the --locked flag, ensuring it uses the committed Cargo.lock dependency
graph without modifying it.
In `@src/cmd/list.rs`:
- Around line 86-135: The list command tests in src/cmd/list.rs lines 86-135
only check successful execution; capture stdout in execute_empty_lockfile,
execute_one_package, and execute_multiple_packages_with_one_active, then assert
the empty message, package IDs, and correct activated or installed status. In
src/cmd/snapshot.rs lines 337-387, capture stdout in the affected snapshot tests
and assert the empty-list message, snapshot metadata, payload provenance, and
hash formatting as applicable.
In `@src/cmd/nu_pin_offer.rs`:
- Around line 141-163: Strengthen the tests around
offer_managed_nu_pin_with_interaction by tracking whether input is read and
whether installation/refresh occurs. Add or use an injectable install/refresh
seam, or assert the temporary root remains unchanged, so
decline_returns_false_without_installing verifies no installation and
invalid_input_is_treated_as_decline verifies invalid input follows the same
rejection path.
- Around line 125-138: Strengthen
accept_proceeds_to_install_and_fails_hermetically_on_bad_pin by asserting the
error chain includes “Failed to normalize requested version 'not-a-version'”.
Preserve the existing outer install-failure assertion; alternatively, use the
injectable installer seam to verify installation is never invoked.
In `@src/cmd/nupm.rs`:
- Around line 483-498: Update inspect_all_without_nupm_home_bails to assert that
execute returns the expected failure result instead of discarding it. Remove the
narrative comment after adding the assertion, while preserving the existing
nonexistent nupm_home setup.
In `@src/cmd/registry.rs`:
- Around line 270-290: Update the command tests to capture user-visible output
at the command boundary or through an injectable writer, then assert the
expected results: in src/cmd/registry.rs lines 270-290, verify the
empty-registry message and configured registry name, URL, and enabled state; in
src/cmd/registry.rs lines 341-391, verify package ID, version, type, and
description; in src/cmd/search.rs lines 445-476, verify the no-match message,
matching package row, and incompatible --all result; and in src/cmd/nupm.rs
lines 610-637, verify the configured scan summary and inspection candidates.
Keep the existing command return checks while adding assertions for relevant
failure and behavior paths.
In `@src/config.rs`:
- Around line 220-227: Update the resolve_root test around
resolve_root_prefers_numan_root_env_var to acquire the shared test-only
environment lock before changing NUMAN_ROOT, and use the established RAII
restore guard to reinstate its prior value on every exit path. Ensure all tests
that mutate NUMAN_ROOT use the same lock and guard so environment changes cannot
overlap.
In `@src/core/nu_version.rs`:
- Around line 237-256: Update NuVersion::matches_constraint to parse inclusive
upper bounds by removing the <= prefix directly, and return false whenever a
constraint bound cannot be parsed instead of silently ignoring it. Revise
matches_constraint_ignores_unparseable_bound to assert failure and add coverage
that <=0.113.0 does not match 0.113.1.
In `@src/install/download.rs`:
- Around line 98-109: Update download_file to convert file URIs using
platform-aware URI-to-path handling rather than manually stripping the file://
prefix, ensuring standard Windows URIs such as file:///C:/... resolve correctly.
Extend download_file_copies_file_url with a Windows-specific test covering the
file:///C:/... form while preserving existing file-copy behavior.
🪄 Autofix
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: 4818a503-279f-457a-86db-51f5c8d6689f
📒 Files selected for processing (12)
.github/workflows/ci.yml.gitignoresrc/cmd/list.rssrc/cmd/nu_pin_offer.rssrc/cmd/nupm.rssrc/cmd/registry.rssrc/cmd/remove.rssrc/cmd/search.rssrc/cmd/snapshot.rssrc/config.rssrc/core/nu_version.rssrc/install/download.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. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (12)
!**/.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:
.gitignoresrc/cmd/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rs.github/workflows/ci.ymlsrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rs
**/*
📄 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/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.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/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.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/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/cmd/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.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/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.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/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.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/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use compile-time
#[cfg(target_env)]platform detection rather thanstd::env::consts.
Files:
src/cmd/remove.rssrc/config.rssrc/cmd/search.rssrc/cmd/registry.rssrc/core/nu_version.rssrc/cmd/nu_pin_offer.rssrc/install/download.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.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/remove.rssrc/cmd/search.rssrc/cmd/registry.rssrc/cmd/nu_pin_offer.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rs
src/cmd/{activate,deactivate,update,remove,nupm,init}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Acquire the root mutation lock before mutating managed state; the lock must serialize concurrent mutations and fail immediately on a second acquisition.
Files:
src/cmd/remove.rssrc/cmd/nupm.rs
src/install/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/install/**/*.rs: Install operations must remain inert: they may write only to$NUMAN_ROOTand must not perform Nu integration.
Plugin binary artifacts must include a SHA256 value; the install transaction must bail when it is missing.
Never overwrite installed payloads; use the immutable path shape<root>/packages/<type>/<owner>/<name>/<version>-<sha8prefix>/.
Files:
src/install/download.rs
🪛 zizmor (1.29.0)
.github/workflows/ci.yml
[warning] 64-64: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 69-69: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
[info] 65-65: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🔍 Remote MCP GitHub Copilot
Additional review context
- PR
#120adds only tests, CI coverage reporting, and.gitignorechanges; it does not alter production behavior. The PR reports +859 lines across 12 files. - CI coverage passed, along with Ubuntu/macOS tests, Clippy, formatting, MSRV, packaging, and acceptance checks. Windows tests and some analysis checks were still in progress; CodeRabbit status remained pending.
- The coverage command uses
cargo llvm-cov --workspace --summary-onlywithout--locked, despiteCargo.lockbeing committed and other reproducibility-sensitive jobs using--locked. Automated review flagged this. - The job is described as informational, but a failing
cargo llvm-covcommand can still fail the job; omitting--fail-under-linesonly disables the coverage threshold. download_filehandlesfile://by stripping the prefix into a rawPathBuf; the added test covers the Unix-style construction but not standard Windowsfile:///C:/...URLs.- Several new command tests call functions and assert only success/non-panicking behavior without checking stdout contents, notably list, registry, search, and snapshot tests.
- The new configuration test mutates the process-global
NUMAN_ROOT; existing repository tests also modify that variable, so parallel-test isolation is worth checking.
🔇 Additional comments (9)
src/config.rs (1)
164-218: LGTM!src/core/nu_version.rs (2)
217-235: LGTM!
258-281: LGTM!src/install/download.rs (1)
86-96: LGTM!src/cmd/list.rs (1)
45-84: LGTM!src/cmd/remove.rs (1)
372-398: LGTM!src/cmd/snapshot.rs (1)
290-335: LGTM!src/cmd/nu_pin_offer.rs (1)
110-123: LGTM!Also applies to: 165-175
.gitignore (1)
47-49: LGTM!
resolve_root_prefers_numan_root_env_var (new in this PR) and doctor.rs's two tests both mutate the process-global NUMAN_ROOT env var without synchronization, in the same parallel cargo-test binary. Since Config::resolve_root reads NUMAN_ROOT, a concurrently-running doctor.rs test could overwrite it mid-test and make the new assertion read the wrong tempdir — an intermittent, hard-to-reproduce CI failure. Adds NumanRootRestoreGuard to util/test_paths.rs, mirroring the existing PathRestoreGuard/HomeRestoreGuard pattern (a shared Mutex serializes snapshot/restore across threads), and uses it at all three call sites. As a side effect, doctor.rs's two tests now restore NUMAN_ROOT on drop instead of leaking it into the rest of the test binary's process state.
…ssertions
Real bug fix:
- NuVersion::matches_constraint's "<=" branch double-stripped the "="
prefix (strip_prefix("<=") then strip_prefix('=') again), so any real
"<=X.Y.Z" constraint silently matched everything — pre-existing on
master, predates this PR. Also changed all operators to fail closed
(return false) on an unparseable bound instead of silently ignoring
it, matching the safety expectation for compatibility gating.
CI hardening:
- coverage job: add persist-credentials: false to checkout (matches
the one other job in this file that handles a token), add --locked
to cargo llvm-cov so it uses the committed Cargo.lock, and clarify
the "informational only" comment (a nonzero cargo llvm-cov exit still
fails the step; only the coverage threshold is not enforced).
download_file: replace manual file:// prefix stripping (which produced
an invalid path like "/C:/..." for Windows file:///C:/... URLs) with
url::Url::to_file_path(), which is platform-aware. url was already a
transitive dependency (via reqwest) — added directly, zero resolver
changes. Added a #[cfg(windows)] test for the file:///C:/... form.
Strengthened weak assertions (mostly "call it, assert it doesn't
panic" -> real content checks). Where the target function only wrote
to real stdout, added an internal `execute_to`/`out: &mut dyn Write`
seam mirroring the pattern nupm.rs already used, so tests can capture
output without touching the public API:
- cmd::list, cmd::snapshot (list/inspect), cmd::registry
(list_registries/list_packages), cmd::search: capture stdout, assert
package/snapshot/registry content instead of just Result::is_ok.
- cmd::nu_pin_offer: decline/invalid-input tests now assert nothing
was written under root (proving install was never reached); the
accept-with-bad-pin test now asserts the underlying normalization
error is in the chain, not just the outer wrapper message.
- cmd::nupm: inspect_all_without_nupm_home_bails now asserts the
actual validation error instead of discarding the result; the
status/inspect-all fixture tests assert the real scan summary and
candidate listing instead of non-empty/not-contains checks.
Skipped as stale (already fixed in a prior review pass on this PR):
config.rs's resolve_root_prefers_numan_root_env_var already uses
NumanRootRestoreGuard.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/nu_version.rs`:
- Around line 108-118: Update the exact-version handling in
NuVersion::matches_constraint so the legacy minor-only branch runs only when the
value has the “.x” suffix; route exact 0.x.y values such as “=0.113.1” through
parse_version and version_eq. Add assertions covering matches_constraint for
“=0.113.1” and rejecting “=0.113.2”.
In `@src/util/test_paths.rs`:
- Around line 134-146: Update the test around the NUMAN_ROOT mutation in
doctor_test to hold a NumanRootRestoreGuard for the entire test scope, ensuring
the environment snapshot, lock, and restoration cover all related operations;
alternatively remove the mutation if the test does not require it.
🪄 Autofix
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: 0bcc61a4-7ebd-4875-a6e6-19e9274cfe98
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/ci.ymlCargo.tomlsrc/cmd/doctor.rssrc/cmd/list.rssrc/cmd/nu_pin_offer.rssrc/cmd/nupm.rssrc/cmd/registry.rssrc/cmd/search.rssrc/cmd/snapshot.rssrc/config.rssrc/core/nu_version.rssrc/install/download.rssrc/util/test_paths.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: Coverage
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (12)
**/*
📄 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:
Cargo.tomlsrc/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.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:
Cargo.tomlsrc/util/test_paths.rs.github/workflows/ci.ymlsrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
Cargo.tomlsrc/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
Cargo.tomlsrc/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.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/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.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/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.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/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.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/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use compile-time
#[cfg(target_env)]platform detection rather thanstd::env::consts.
Files:
src/util/test_paths.rssrc/cmd/doctor.rssrc/config.rssrc/cmd/nu_pin_offer.rssrc/core/nu_version.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rssrc/install/download.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.rssrc/cmd/nu_pin_offer.rssrc/cmd/nupm.rssrc/cmd/snapshot.rssrc/cmd/list.rssrc/cmd/search.rssrc/cmd/registry.rs
src/cmd/{activate,deactivate,update,remove,nupm,init}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Acquire the root mutation lock before mutating managed state; the lock must serialize concurrent mutations and fail immediately on a second acquisition.
Files:
src/cmd/nupm.rs
src/install/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/install/**/*.rs: Install operations must remain inert: they may write only to$NUMAN_ROOTand must not perform Nu integration.
Plugin binary artifacts must include a SHA256 value; the install transaction must bail when it is missing.
Never overwrite installed payloads; use the immutable path shape<root>/packages/<type>/<owner>/<name>/<version>-<sha8prefix>/.
Files:
src/install/download.rs
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
-
The PR’s
coveragejob usescargo llvm-cov --workspace --locked --summary-only; the committed lockfile also includes the newurldependency. The job can still fail if tests or coverage execution fail, but it does not enforce a coverage threshold. -
Command output refactors preserve normal CLI behavior by passing
stdoutfrom each publicexecutefunction into private writer-based helpers. Tests generally validate captured output, though many assertions usecontainsrather than exact output comparisons. -
Config::resolve_rootis an early, shared entry point for determining Numan’s managed state directory, and the newNUMAN_ROOTguard serializes only tests that explicitly construct it. Any other tests mutatingNUMAN_ROOTwithout the guard could still race. -
The
NuVersion::matches_constraintchange now fails closed for malformed parsed bounds, but the=0.*branch still treats every version beginning with0.as the special minor-version form. Consequently, a constraint such as=0.113.1attempts to parse113.1as a minor number and will not perform full-version equality. The added equality test covers only=1.2.3, so this case remains untested. -
Download handling now uses
url::Url::to_file_path(), while the Windows-specific test covers a drive-form URL only under#[cfg(windows)]; no test covers invalid file URLs or file URLs with a host component. -
Numan’s install transaction downloads artifacts into partial files, verifies SHA-256, and atomically renames them; the PR only adds direct
download_filetests and does not exercise that broader transaction path. -
Current checks show Format, Clippy, MSRV, Ubuntu tests, macOS acceptance, Ubuntu acceptance, packaging, and roadmap checks succeeded. Coverage, Windows tests, macOS tests, Rust analysis, and Windows acceptance were still in progress when queried.
🔇 Additional comments (14)
src/cmd/list.rs (1)
5-22: LGTM!Also applies to: 42-46, 57-153
src/cmd/registry.rs (1)
7-7: LGTM!Also applies to: 35-59, 175-213, 267-415
src/cmd/search.rs (1)
22-31: LGTM!Also applies to: 43-54, 117-150, 253-253, 433-518
src/cmd/snapshot.rs (1)
3-3: LGTM!Also applies to: 43-253, 321-435
src/install/download.rs (1)
98-109: Generate the test URL from the source path.Line 104 creates
file://C:\...on Windows. This is not a portable file URL. Useurl::Url::from_file_path(&src)so this cross-platform test validates a URL thatto_file_path()can round trip. Keep the explicit Windows drive-form test.Url::to_file_path()has platform-specific file-host handling. (docs.rs)Proposed fix
- let url = format!("file://{}", src.display()); + let url = url::Url::from_file_path(&src).unwrap().to_string();src/config.rs (1)
222-227: LGTM!Cargo.toml (1)
41-41: LGTM!src/cmd/nu_pin_offer.rs (2)
150-180: Prove that the interactive input path runs.The new filesystem assertions prove that no root artifact remains, but both tests still pass if
offer_managed_nu_pin_with_interactionskipsread_lineand returnsfalse. Track callback invocation in both closures, then assert that the callback was called. Keep the no-artifact assertion for the installation side effect.
139-147: LGTM!Also applies to: 182-192
src/cmd/nupm.rs (2)
483-498: Assert the explicit-path failure.Line 497 discards
execute's result, so the test passes ifinspect --allsucceeds. The test also suppliesSome("/nonexistent/nupm-home");resolve_nupm_homevalidates an explicit path instead of using theNotConfiguredbranch (src/nupm_compat/discovery.rs:18-29). Captureunwrap_err()and assert the path-validation error, or setnupm_hometoNoneand controlNUPM_HOMEif the test is intended to cover the not-configured branch. Remove the control-flow comment after adding the assertion.As per coding guidelines, “Add comments only when the WHY is non-obvious; do not narrate what the code does.”
Source: Coding guidelines
456-480: LGTM!Also applies to: 501-604, 606-608, 611-648
.github/workflows/ci.yml (1)
65-66: LGTM!Also applies to: 77-85
src/util/test_paths.rs (1)
148-174: LGTM!src/cmd/doctor.rs (1)
1860-1860: LGTM!Also applies to: 1893-1893
- ci.yml: add continue-on-error: true to the coverage job's "Run
coverage" step. Without it, cargo llvm-cov exiting nonzero (e.g. a
test failing during the instrumented run) would still fail the
build despite the "informational only" framing — the test job
already gates on real test failures, this job only publishes the
summary.
- nu_version.rs: matches_constraint's "=0.x.y" handling used
ver.strip_prefix("0.") to detect the legacy "=0.113.x" minor-only
format, which also matches full exact versions like "=0.113.1" —
"113.1" then fails to parse as a minor number and (after the earlier
fail-closed fix) always returns false, so an exact 0.x.y match was
reported incompatible. Only take the legacy path when the value ends
in ".x".
- nupm.rs: inspect_all_without_nupm_home_bails still discarded the
execute() result (`let _ = ...`) — an earlier commit's message
claimed this was fixed but the edit was never actually applied.
Fixed for real this time and verified by running the test.
- tests/doctor_test.rs: doctor_fix_auto_creates_layout_without_network
mutates the process-global NUMAN_ROOT env var without the
NumanRootRestoreGuard added earlier in this PR, so it could race a
passing test in the future if more of this integration binary's
tests start reading NUMAN_ROOT concurrently. Applied the same guard.
1 participant
Summary
#[cfg(test)]unit tests for previously-weakcmd::entry points:cmd::list(0% -> 99%),cmd::nu_pin_offer(0% -> 71%),cmd::registry(34% -> 82%, add/remove/list, no network),cmd::snapshot(26% -> 73%, list/inspect/short_hash),install::download's local file-path copy branch (19% -> 42%)config.rs(62% -> 90%),core/nu_version.rs(73% -> 89%),cmd::remove(66% -> 93%, including a full happy-path removal),cmd::search(70% -> 91%,execute()end-to-end via a fixture registry),cmd::nupm(61% -> 80%, argument-validation bails plus status/inspect --all happy paths)util::stdio_redirect.rs's remaining gap is the Windows-only code path, not exercisable on Linux CI — left as-isinstall/transaction.rs,cmd/setup.rs,cmd/update.rs,nu/bootstrap.rsremain below-average (see below) — their gaps sit inside large, monolithic functions that need the same heavy fixture machinerytests/install_test.rsalready builds forinstall_package, not a quick unit-test addition. Flagged as a follow-up rather than attempted here.coveragejob toci.ymlusingcargo-llvm-cov— informational only, reports to the job summary, no--fail-under-linesgateWorkspace line coverage: 81.3% -> 83.9%
Test plan
cargo test --lib— 757 tests, all greencargo fmt --all -- --check— cleancargo clippy -- -D warnings— clean