Honest install-only UX and Nu-agnostic try fallbacks - #95
Conversation
Label scripts/completions as install-only in search/info, and let numan try install wttr/nufetch when no activatable starter fits without calling activate. Co-authored-by: Cursor <cursoragent@cursor.com>
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: 31 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 (3)
📝 WalkthroughWalkthroughChangesPackage activation behavior
Estimated code review effort: 3 (Moderate) | ~25 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 QodoMark scripts/completions as install-only; make
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Greptile SummaryThe PR makes script and completion UX explicitly install-only and updates
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior wrong-version hint issue is fixed by deriving the entry and payload path from the completed installation’s lockfile record.
|
| Filename | Overview |
|---|---|
| src/cmd/info.rs | Adds explicit install-only activation messaging for script and completion package details. |
| src/cmd/search.rs | Gives scripts and completions install-only status labels even when Nu detection is unavailable. |
| src/cmd/try_cmd.rs | Adds script fallbacks, skips activation for install-only types, and fixes usage hints to read the resolved entry and payload path from the lockfile. |
Reviews (3): Last reviewed commit: "Harden install-only try hints and search..." | Re-trigger Greptile
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/cmd/info.rs`:
- Around line 169-174: Add a separate test case alongside
format_info_marks_script_install_only that sets package_type to
PackageType::Completion, calls format_info with the same inputs, and asserts the
output contains “Activation: install-only”.
In `@src/cmd/try_cmd.rs`:
- Around line 334-343: Update the overlay hint logic in print_install_only_hint
to use the installed lockfile record’s resolved artifact entry instead of
pkg.versions.last(). Ensure the payload path and entry name come from the same
installed version, and add a multi-version test with distinct artifact.entry
values verifying the printed overlay path is correct.
🪄 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: 1d22b12b-2d3d-4203-ad93-1b6d6c8eae6b
📒 Files selected for processing (3)
src/cmd/info.rssrc/cmd/search.rssrc/cmd/try_cmd.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 (9)
**/*
📄 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/search.rssrc/cmd/info.rssrc/cmd/try_cmd.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/search.rssrc/cmd/info.rssrc/cmd/try_cmd.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/search.rssrc/cmd/info.rssrc/cmd/try_cmd.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/cmd/search.rssrc/cmd/info.rssrc/cmd/try_cmd.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/cmd/search.rssrc/cmd/info.rssrc/cmd/try_cmd.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/cmd/search.rssrc/cmd/info.rssrc/cmd/try_cmd.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:
src/cmd/search.rssrc/cmd/info.rssrc/cmd/try_cmd.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/search.rssrc/cmd/info.rssrc/cmd/try_cmd.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Never panic in library code; return errors instead.
Useclapderive macros for CLI definitions and preserve lowercasenumanfor the executable while usingNumanin prose.
Useserde/serde_jsonfor JSON serialization andtomlfor configuration serialization.
Files:
src/cmd/search.rssrc/cmd/info.rssrc/cmd/try_cmd.rs
🔍 Remote MCP GitHub Copilot
Additional review context
- PR
#95changes onlyinfo.rs,search.rs, andtry_cmd.rs(+171/−38). CI had Clippy, formatting, Deny, MSRV, package, and roadmap checks passing; platform tests were still running when queried. - Existing project support defines plugins/modules as activatable, while scripts/completions are install-only; the install transaction preserves
artifact.entryin the lockfile. - The proposed fallback packages are present in the registry:
SuaveIV/nu_script_wttrandSanceilaks/nufetchare scripts withnu_version: "*", single archive entries (weather.nuandnufetch.nu), and no activation metadata. try_cmdresolves the selected version throughResolver, but its install-only hint readspkg.versions.last().artifact.entryrather than the resolved entry stored in the lockfile. Current fallback specs each have one version, but multi-version script packages should be covered before relying on this path.- Historical PR
#39documents the intended activation boundary: only plugins and modules are activated; scripts and completion packages remain inert.
🔇 Additional comments (1)
src/cmd/try_cmd.rs (1)
513-543: 📐 Maintainability & Code QualityVerify all required Rust gates before merge.
The supplied CI context confirms formatting and Clippy checks, but platform tests were still running. Confirm that
cargo test,cargo clippy -- -D warnings, andcargo fmt --checkall pass for this change.As per coding guidelines, “All CI gates must pass:
cargo test,cargo clippy -- -D warnings, andcargo fmt --check.”Sources: Coding guidelines, MCP tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfae650055
ℹ️ 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.
|
- Use installed lockfile entry metadata - Surface lockfile loading errors
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cmd/try_cmd.rs`:
- Around line 336-340: Update the lockfile lookup in the package installation
flow around transaction::install_package so a missing package record returns an
error with contextual information instead of falling back to (None, None).
Preserve entry == None only when the package record exists, and add Rust tests
covering both a missing record and a malformed lockfile.
🪄 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: 25d3c338-7b12-41ac-a3a4-25a758a837bc
📒 Files selected for processing (1)
src/cmd/try_cmd.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 (9)
**/*
📄 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/try_cmd.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/try_cmd.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/try_cmd.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/cmd/try_cmd.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
src/cmd/try_cmd.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/cmd/try_cmd.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:
src/cmd/try_cmd.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/try_cmd.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Never panic in library code; return errors instead.
Useclapderive macros for CLI definitions and preserve lowercasenumanfor the executable while usingNumanin prose.
Useserde/serde_jsonfor JSON serialization andtomlfor configuration serialization.
Files:
src/cmd/try_cmd.rs
🔍 Remote MCP DeepWiki, GitHub Copilot
Additional review context
- Package types are
Plugin,Module,Script, andCompletion; only plugins/modules are activatable. Scripts and completions intentionally remain deferred. - Installation persists the resolved artifact’s
entryand payload path in the lockfile. The latest PR commit changestry_cmdto use this installed metadata and surfaces lockfile-load errors instead of suppressing them. - The earlier review concern was valid: using
versions.last().artifact.entrycould produce an incorrectoverlay usepath when registry versions are unsorted or compatibility selects another version. This was addressed in commit2fdd2d3. - Current tests cover fallback ordering, but no visible test exercises a multi-version script package or lockfile-load failure path; these remain worthwhile edge cases to verify.
- At query time, Clippy, formatting, MSRV, package, Deny, and macOS acceptance checks passed. Linux/Windows tests, CodeQL analyses, and CodeRabbit review were still pending or in progress.
🔇 Additional comments (1)
src/cmd/try_cmd.rs (1)
170-170: LGTM!Also applies to: 332-335, 354-354
Address PR review: require lockfile records, quote Nu overlay paths, label scripts when Nu is unknown, and cover completion info. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
search/info: scripts and completions report install-only (activation deferred) instead of module-style ABI labelsnuman try: prefer activatable curated starters; fall back toSuaveIV/nu_script_wttrthenSanceilaks/nufetch; skip activate for install-only types and printoverlay useguidanceTest plan
cargo test --lib select_starter/ install-only search+info testscargo clippy -- -D warningsnuman search cargoetc.;numan trywith no plugin starter installs a script without activation error