Add cross-channel install guard for cargo, winget, and Homebrew - #93
Add cross-channel install guard for cargo, winget, and Homebrew#93tonythethompson wants to merge 5 commits into
Conversation
Prompt to uninstall an existing numan from another package manager before installing via a different channel; declining cancels the install. Cargo install runs the guard automatically via build.rs; winget and Homebrew use wrapper scripts. Doctor warns when multiple channels are detected. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 40 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 (11)
📝 WalkthroughWalkthroughAdds shared installation-channel detection, interactive conflict guards for Cargo, Winget, and Homebrew, installer wrappers, Cargo build integration, multiple-channel doctor diagnostics, and updated installation documentation. ChangesInstallation channel management
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 install-channel guard across cargo, winget, and Homebrew
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
conflicting_installs_excludes_pathstest doesn’t exerciseconflicting_installsat all and instead reimplements its filtering logic inline; consider rewriting it to callconflicting_installswith a controlleddiscover_installations(e.g., via dependency injection or a small refactor) so regressions in the real function are caught. - The directory walks in
known_install_candidates(especially underWinGet/Packagesand Homebrew Cellar) now run both fornuman doctorand during install; if these trees are large they could add noticeable latency, so you may want to constrain the search more tightly (e.g., narrower depth/paths or a fast path for PATH hits) or cache results across a single process run.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `conflicting_installs_excludes_paths` test doesn’t exercise `conflicting_installs` at all and instead reimplements its filtering logic inline; consider rewriting it to call `conflicting_installs` with a controlled `discover_installations` (e.g., via dependency injection or a small refactor) so regressions in the real function are caught.
- The directory walks in `known_install_candidates` (especially under `WinGet/Packages` and Homebrew Cellar) now run both for `numan doctor` and during install; if these trees are large they could add noticeable latency, so you may want to constrain the search more tightly (e.g., narrower depth/paths or a fast path for PATH hits) or cache results across a single process run.
## Individual Comments
### Comment 1
<location path="src/cmd/doctor.rs" line_range="327-336" />
<code_context>
+fn check_install_channels(findings: &mut Vec<Finding>) {
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicate uninstall hints when multiple installs share the same channel.
`fix` currently aggregates `uninstall_hint()` for each conflicting install, so multiple installs from the same channel (e.g., two cargo builds) produce repeated commands. Please deduplicate uninstall commands per `InstallChannel` before joining, either by collecting hints into a `HashSet` or by deduping channels first and then mapping to hints.
Suggested implementation:
```rust
fn check_install_channels(findings: &mut Vec<Finding>) {
let installs = discover_installations();
if installs.len() <= 1 {
return;
}
// Collect all distinct install channels
let channels: std::collections::HashSet<InstallChannel> =
installs.iter().map(|install| install.channel).collect();
if channels.len() <= 1 {
return;
}
// Deduplicate uninstall hints per InstallChannel by:
// 1. Iterating over each unique channel
// 2. Picking one representative install for that channel
// 3. Collecting its uninstall_hint() into a HashSet<String>
//
// The resulting `_uninstall_hints` set can be used when constructing
// the Finding's fix to avoid duplicated uninstall commands.
let _uninstall_hints: std::collections::HashSet<String> = channels
.into_iter()
.filter_map(|channel| {
installs
.iter()
.find(|install| install.channel == channel)
.map(|install| install.uninstall_hint())
})
.collect();
```
To fully apply the deduplicated uninstall hints in the `fix`:
1. Replace any existing logic that aggregates uninstall commands like:
`let fixes = installs.iter().map(|i| i.uninstall_hint()).collect::<Vec<_>>().join("\n");`
with code that uses the `_uninstall_hints` set, e.g. by:
- Converting `_uninstall_hints` into a `Vec<String>`, sorting if desired for determinism, and then joining with `"\n"`.
2. Ensure that the `Finding` created inside `check_install_channels` (or any helper it calls) uses this joined string of deduplicated hints as the fix text.
3. If `InstallChannel` does not yet implement `Eq + Hash + Copy/Clone`, derive or implement these traits so it can be used as a key in `HashSet<InstallChannel>`.
</issue_to_address>
### Comment 2
<location path="docs/numan-doctor.md" line_range="132" />
<code_context>
+| `install.multiple_channels` | `warn` | More than one `numan` binary from different install channels (cargo, winget, homebrew, release archive) → fix: uninstall the other channel(s) before reinstalling |
</code_context>
<issue_to_address>
**nitpick (typo):** Capitalize "Homebrew" to match its usage elsewhere and maintain consistency in tool names.
Update "homebrew" here to "Homebrew" so it matches the capitalization used elsewhere in the docs.
```suggestion
| `install.multiple_channels` | `warn` | More than one `numan` binary from different install channels (cargo, winget, Homebrew, release archive) → fix: uninstall the other channel(s) before reinstalling |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fn check_install_channels(findings: &mut Vec<Finding>) { | ||
| let installs = discover_installations(); | ||
| if installs.len() <= 1 { | ||
| return; | ||
| } | ||
|
|
||
| let channels: std::collections::HashSet<InstallChannel> = | ||
| installs.iter().map(|install| install.channel).collect(); | ||
| if channels.len() <= 1 { | ||
| return; |
There was a problem hiding this comment.
suggestion: Avoid duplicate uninstall hints when multiple installs share the same channel.
fix currently aggregates uninstall_hint() for each conflicting install, so multiple installs from the same channel (e.g., two cargo builds) produce repeated commands. Please deduplicate uninstall commands per InstallChannel before joining, either by collecting hints into a HashSet or by deduping channels first and then mapping to hints.
Suggested implementation:
fn check_install_channels(findings: &mut Vec<Finding>) {
let installs = discover_installations();
if installs.len() <= 1 {
return;
}
// Collect all distinct install channels
let channels: std::collections::HashSet<InstallChannel> =
installs.iter().map(|install| install.channel).collect();
if channels.len() <= 1 {
return;
}
// Deduplicate uninstall hints per InstallChannel by:
// 1. Iterating over each unique channel
// 2. Picking one representative install for that channel
// 3. Collecting its uninstall_hint() into a HashSet<String>
//
// The resulting `_uninstall_hints` set can be used when constructing
// the Finding's fix to avoid duplicated uninstall commands.
let _uninstall_hints: std::collections::HashSet<String> = channels
.into_iter()
.filter_map(|channel| {
installs
.iter()
.find(|install| install.channel == channel)
.map(|install| install.uninstall_hint())
})
.collect();To fully apply the deduplicated uninstall hints in the fix:
- Replace any existing logic that aggregates uninstall commands like:
let fixes = installs.iter().map(|i| i.uninstall_hint()).collect::<Vec<_>>().join("\n");
with code that uses the_uninstall_hintsset, e.g. by:- Converting
_uninstall_hintsinto aVec<String>, sorting if desired for determinism, and then joining with"\n".
- Converting
- Ensure that the
Findingcreated insidecheck_install_channels(or any helper it calls) uses this joined string of deduplicated hints as the fix text. - If
InstallChanneldoes not yet implementEq + Hash + Copy/Clone, derive or implement these traits so it can be used as a key inHashSet<InstallChannel>.
| |----|----------|-----------| | ||
| | `nu.binary.missing_on_path` | `error` | Nu not on PATH and not under `$NUMAN_ROOT/tools/nushell/` → fix: `numan setup nu` | | ||
| | `nu.binary.found_off_path` | `warn` | Nu exists in a known install root (e.g. `~/.cargo/bin`, `%LOCALAPPDATA%\Programs\nushell`) but not on PATH → fix: `numan setup nu use <path>` | | ||
| | `install.multiple_channels` | `warn` | More than one `numan` binary from different install channels (cargo, winget, homebrew, release archive) → fix: uninstall the other channel(s) before reinstalling | |
There was a problem hiding this comment.
nitpick (typo): Capitalize "Homebrew" to match its usage elsewhere and maintain consistency in tool names.
Update "homebrew" here to "Homebrew" so it matches the capitalization used elsewhere in the docs.
| | `install.multiple_channels` | `warn` | More than one `numan` binary from different install channels (cargo, winget, homebrew, release archive) → fix: uninstall the other channel(s) before reinstalling | | |
| | `install.multiple_channels` | `warn` | More than one `numan` binary from different install channels (cargo, winget, Homebrew, release archive) → fix: uninstall the other channel(s) before reinstalling | |
Greptile SummaryThe PR adds cross-channel installation detection and uninstall prompting for Cargo, winget, and Homebrew, plus a doctor warning for mixed installations.
Confidence Score: 5/5The PR appears safe to merge based on established findings, although the prior ordinary-Cargo-install guard issue remains unresolved by the available evidence. No blocking failure remains established; the previous path-dependency publication issue is fixed, while the repository does not establish whether the revised OUT_DIR heuristic fixes the previously reported ordinary cargo-install path.
|
| Filename | Overview |
|---|---|
| Cargo.toml | Registers the install guard as a binary in the main publishable crate, removing the prior path-only dependency packaging failure. |
| build.rs | Adds a fallback Cargo-install heuristic, but the available repository evidence does not establish that ordinary documented installs satisfy it. |
| src/util/install_channel.rs | Implements channel classification, installation discovery, prompting, and package-manager-specific uninstall behavior. |
| src/cmd/doctor.rs | Adds a warning when PATH contains numan binaries classified under multiple installation channels. |
| scripts/install-homebrew.sh | Runs the install guard before invoking the Homebrew installation. |
| scripts/install-winget.ps1 | Runs the install guard before invoking the winget installation. |
Reviews (4): Last reviewed commit: "Address install guard review feedback" | 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: 4e0aa500c9
ℹ️ 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".
| [build-dependencies] | ||
| numan-install-guard = { path = "install-guard" } | ||
|
|
||
| [dependencies] | ||
| numan-install-guard = { path = "install-guard" } |
There was a problem hiding this comment.
When publishing or packaging the crates.io channel, these path-only numan-install-guard dependencies make cargo package fail with “all dependencies must have a version specified”; the guard crate is also marked publish = false, so cargo install numan-cli cannot resolve it from the registry. This breaks the documented crates.io/source distribution path before users can install the binary.
AGENTS.md reference: AGENTS.md:L197-L197
Useful? React with 👍 / 👎.
| if std::env::var("CARGO_INSTALL_ROOT").is_ok() | ||
| && numan_install_guard::run_cargo_install_guard() != std::process::ExitCode::SUCCESS |
There was a problem hiding this comment.
For normal cargo install numan-cli or cargo install --git ..., Cargo does not set CARGO_INSTALL_ROOT for build scripts unless the user explicitly exports it; I verified with a minimal cargo install --root ... -vv probe that the build script saw CARGO_INSTALL_ROOT=None. This condition therefore skips the advertised automatic guard on the default cargo install path, allowing cross-channel installs to proceed silently.
AGENTS.md reference: AGENTS.md:L37-L38
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| echo "Checking for conflicting numan installs..." | ||
| cargo run --quiet --manifest-path "${guard_manifest}" -- brew | ||
| echo "Running: brew install tonythethompson/numan/numan ${*}" | ||
| brew install tonythethompson/numan/numan "$@" |
There was a problem hiding this comment.
1. Homebrew install ignores numan_root 📘 Rule violation ≡ Correctness
The install-time scripts scripts/install-homebrew.sh, scripts/install-winget.ps1, and scripts/install-cargo.ps1 invoke brew install, winget install, and cargo install, respectively, which perform installations into tool/system-managed locations not constrained under $NUMAN_ROOT. Compliance requires install-time code to keep all filesystem writes confined to paths rooted under $NUMAN_ROOT.
Agent Prompt
## Issue description
`scripts/install-homebrew.sh`, `scripts/install-winget.ps1`, and `scripts/install-cargo.ps1` run `brew install`, `winget install`, and `cargo install`, respectively, which perform filesystem mutations outside `$NUMAN_ROOT`.
## Issue Context
Compliance requires install-time scripts/hooks to avoid Nu integration APIs and to keep all filesystem writes confined to paths rooted under `$NUMAN_ROOT`.
## Fix Focus Areas
- scripts/install-homebrew.sh[13-16]
- scripts/install-winget.ps1[36-38]
- scripts/install-cargo.ps1[31-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@build.rs`:
- Around line 2-3: Replace the CARGO_INSTALL_ROOT check in build.rs with a
reliable install-only signal or wrapper so run_cargo_install_guard executes only
for cargo install invocations and still covers standard cargo install numan-cli
without --root. Update AGENTS.md line 37, README.md line 148, docs/PACKAGING.md
line 49, and packaging/winget/README.md line 29 to remove or correct claims that
plain cargo install numan-cli automatically enforces the guard.
In `@Cargo.toml`:
- Around line 20-27: Make numan-install-guard publishable before numan-cli by
publishing the install-guard crate and replacing its path-only entries in both
[build-dependencies] and [dependencies] with the released registry version. Keep
the dependency available in both sections and ensure the version matches the
published crate.
In `@install-guard/src/lib.rs`:
- Around line 327-332: Update should_skip_guard to treat CI and GITHUB_ACTIONS
as enabled when their values are either "1" or case-insensitive "true", while
preserving the existing NUMAN_SKIP_INSTALL_GUARD behavior and guard bypass
logic.
- Around line 414-433: Replace the local filtering in
conflicting_installs_excludes_paths with a call to the production
conflicting_installs function through an injectable discovery or candidate-roots
seam. Extend the tests to cover excluded targets, same-channel installs,
unreadable directories, and cancellation in non-interactive mode, ensuring each
assertion exercises the real conflicting_installs behavior.
- Around line 1-2: Remove the narrative comments at the module header and line
10; retain comments only if they explain a non-obvious design reason rather than
describing what the code declares or does.
- Line 315: Restore rustfmt formatting for the conflicting_installs call in the
surrounding function so its indentation matches rustfmt output and cargo fmt
--check passes.
- Around line 354-380: Update the uninstall flow in run_install_guard and
uninstall_channel so package-manager mutations are serialized under the
installation-root mutation lock. Resolve an authoritative root and create the
pre-mutation snapshot before invoking run_command; if the standalone guard
cannot obtain either, do not uninstall and instead print the appropriate
uninstall command. Ensure the lock and snapshot cover the entire cargo, winget,
or brew uninstall operation.
- Around line 181-183: Update walk_for_numan and the related discovery flow in
discover_installations to propagate read_dir failures as contextual
anyhow::Result errors instead of returning an empty traversal. Treat an absent
directory as the only case that may skip the install guard; otherwise fail
closed, and ensure numan doctor reports incomplete detection rather than
interpreting missing results as no multi-install finding.
In `@install-guard/src/main.rs`:
- Around line 8-18: Replace the manual argument matching in main with a
clap-based CLI module: add install-guard/src/cli.rs defining Parser and
Subcommand-derived types for cargo, winget, and brew/homebrew commands, add the
clap 4 dependency to Cargo.toml, and have main parse and dispatch the CLI
values. Preserve the existing usage/error behavior where applicable, and do not
modify Cargo.lock.
In `@scripts/install-cargo.ps1`:
- Line 13: Update the install-cargo wrapper so the default Cargo --path is
derived from the script’s resolved repository root instead of the caller’s
current directory. In the install flow around $repoRoot and $CargoArgs, set or
rewrite the default path after $repoRoot is established so cargo install always
targets the checkout the script belongs to, while preserving any explicit path
overrides.
🪄 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: 95ff94aa-0a7e-4f26-be54-2d54a1ca1273
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
AGENTS.mdCargo.tomlREADME.mdbuild.rsdocs/PACKAGING.mddocs/numan-doctor.mdinstall-guard/Cargo.tomlinstall-guard/src/lib.rsinstall-guard/src/main.rspackaging/winget/README.mdscripts/install-cargo.ps1scripts/install-homebrew.shscripts/install-winget.ps1src/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. (4)
- GitHub Check: Greptile Review
- GitHub Check: Sourcery review
- GitHub Check: Test (windows-latest)
- 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:
build.rsscripts/install-cargo.ps1install-guard/src/main.rsinstall-guard/Cargo.tomlpackaging/winget/README.mdscripts/install-winget.ps1scripts/install-homebrew.shREADME.mdAGENTS.mddocs/PACKAGING.mdsrc/cmd/doctor.rsdocs/numan-doctor.mdCargo.tomlinstall-guard/src/lib.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:
build.rsinstall-guard/src/main.rssrc/cmd/doctor.rsinstall-guard/src/lib.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:
build.rsscripts/install-cargo.ps1install-guard/src/main.rsinstall-guard/Cargo.tomlpackaging/winget/README.mdscripts/install-winget.ps1scripts/install-homebrew.shREADME.mdAGENTS.mddocs/PACKAGING.mdsrc/cmd/doctor.rsdocs/numan-doctor.mdCargo.tomlinstall-guard/src/lib.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:
build.rsinstall-guard/src/main.rssrc/cmd/doctor.rsinstall-guard/src/lib.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
build.rsinstall-guard/src/main.rsinstall-guard/Cargo.tomlsrc/cmd/doctor.rsCargo.tomlinstall-guard/src/lib.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:
build.rsinstall-guard/src/main.rssrc/cmd/doctor.rsinstall-guard/src/lib.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:
build.rsinstall-guard/src/main.rspackaging/winget/README.mdREADME.mdAGENTS.mddocs/PACKAGING.mdsrc/cmd/doctor.rsdocs/numan-doctor.mdinstall-guard/src/lib.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
build.rsinstall-guard/src/main.rsinstall-guard/Cargo.tomlpackaging/winget/README.mdREADME.mdAGENTS.mddocs/PACKAGING.mdsrc/cmd/doctor.rsdocs/numan-doctor.mdCargo.tomlinstall-guard/src/lib.rs
**/*.md
📄 CodeRabbit inference engine (REVIEW.md)
Update documentation and
AGENTS.mdwhen project structure or conventions change.
Files:
packaging/winget/README.mdREADME.mdAGENTS.mddocs/PACKAGING.mddocs/numan-doctor.md
src/{state/migration_journal.rs,nu/migrate_legacy.rs,cmd/doctor.rs}
📄 CodeRabbit inference engine (AGENTS.md)
The legacy-Nu migration journal must use the documented
Prepared→Renamed→Activestages, self-heal at the top ofmigrate_legacy_install_with_detector, and leave unreadable or schema-mismatched journals for manual repair.
Files:
src/cmd/doctor.rs
🔍 Remote MCP Context7
Based on my search attempts, I've gathered relevant context about the technologies used in this PR. Let me provide a summary of the review context:
Additional Context Found for PR Review
Build System Integration (Rust Cargo)
The PR's use of build.rs as an entry point that checks CARGO_INSTALL_ROOT and calls numan_install_guard::run_cargo_install_guard() leverages Cargo's standard build script capabilities. According to Rust documentation, Cargo build scripts can access environment variables at runtime using std::env, and build scripts execute during the compilation phase before the binary is created. The implementation checks exit status and fails the build (exit status 1) if the guard doesn't return success—a correct pattern for guarding against conflicting installations during cargo install.
PowerShell Script Patterns
The PR introduces scripts/install-cargo.ps1 and scripts/install-winget.ps1 with configurable parameters:
- Both scripts accept
$CargoArgsand$WingetArgsparameters with default values - This follows PowerShell best practices for parameter validation and native executable invocation with argument arrays (
& $exePath $argsArray) as documented in the PowerShell best practices guide. - The pattern of validating manifest presence and forwarding arguments to native tools (cargo, winget) is consistent with cross-platform script design practices.
Shell Script Error Handling
The scripts/install-homebrew.sh uses strict Bash error handling as mentioned in the AI summary. The documented approach for production shell scripts includes mechanisms like set -e (exit on error) and set -u (error on undefined variables), though the specific implementation details would require viewing the actual script content.
Install Guard Architecture
The PR introduces a comprehensive install guard library (install-guard/) with:
- Public API:
InstallChannelenum (Cargo, Winget, Homebrew, ReleaseArchive, Unknown), discovery functions (discover_installations(),conflicting_installs()), and channel-specific guard functions - Integration points: Called during Cargo build, wrapped in WinGet/Homebrew scripts, and integrated into the
numan doctorcommand for diagnostic warnings - Behavior: Interactive installs prompt user uninstall confirmation; non-interactive installs fail; CI environments bypass checks via environment variable
Test Coverage
The AI summary indicates tests are included for install guard classification and conflict detection, with the code review effort rated as "High" for the 434-line install-guard/src/lib.rs implementation and "Medium" for the CLI dispatcher (install-guard/src/main.rs at 18 lines).
Note: The user requirements referenced a different project (Babel-Player), so this analysis focused on the technical context directly relevant to the numan install-guard PR.
🔇 Additional comments (10)
Cargo.toml (1)
80-83: LGTM!scripts/install-winget.ps1 (1)
1-38: LGTM!scripts/install-homebrew.sh (1)
1-16: LGTM!README.md (1)
130-140: LGTM!docs/PACKAGING.md (1)
40-41: LGTM!packaging/winget/README.md (1)
23-27: LGTM!build.rs (1)
3-3: 🎯 Functional CorrectnessNo change needed.
install-guard/src/lib.rs (1)
6-6: 📐 Maintainability & Code QualityVerify MSRV support for
IsTerminal.Rust 2021 does not define a minimum compiler version that guarantees
std::io::IsTerminal. Confirm that the declared toolchain supports this API, or use a compatible alternative.Source: Coding guidelines
src/cmd/doctor.rs (1)
327-378: 📐 Maintainability & Code QualityRun the required Rust validation before merge.
The PR summary states that Ubuntu, Windows, and macOS validation remains pending. Run
cargo test,cargo clippy -- -D warnings, andcargo fmt --checkon the final revision before release.As per coding guidelines, "
**/*.rs: All CI gates must pass:cargo test,cargo clippy -- -D warnings, andcargo fmt --check."Source: Coding guidelines
docs/numan-doctor.md (1)
132-132: LGTM!
| if std::env::var("CARGO_INSTALL_ROOT").is_ok() | ||
| && numan_install_guard::run_cargo_install_guard() != std::process::ExitCode::SUCCESS |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files relevant =="
git ls-files | rg '(^build\.rs$|AGENTS\.md$|README\.md$|docs/PACKAGING\.md$|packaging/winget/README\.md$|^Cargo\.toml$|install-guard|numan_install_guard)' || true
echo
echo "== build.rs =="
cat -n build.rs
echo
echo "== relevant docs snippets =="
for f in AGENTS.md README.md docs/PACKAGING.md packaging/winget/README.md Cargo.toml; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p'
fi
done
echo
echo "== references to CARGO_INSTALL_ROOT / cargo install guard =="
rg -n "CARGO_INSTALL_ROOT|cargo install|install-guard|guard|uninstall|winget|Homebrew" .Repository: tonythethompson/numan
Length of output: 50379
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
echo "== cargo reference for CARGO_INSTALL_ROOT via rustforge/cargo-source =="
curl -fsSL 'https://raw.githubusercontent.com/rust-lang/cargo/master/src/cargo/util/config/mod.rs' \
| rg -n "CARGO_INSTALL_ROOT|install\.root" -C 4 || true
echo
echo "== cargo docs pages (raw) for CARGO_INSTALL_ROOT =="
curl -fsSL 'https://raw.githubusercontent.com/rust-lang/cargo/master/src/doc/reference/environment-variables.adoc' \
| rg -n "CARGO_INSTALL_ROOT|install\.root" -C 6 || true
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== install-guard public API =="
cat -n install-guard/src/lib.rs | sed -n '1,260p'
echo
echo "== install-guard CLI behavior =="
cat -n install-guard/src/main.rs | sed -n '1,260p'
echo
echo "== cargo wrapper scripts =="
for f in scripts/install-cargo.ps1 scripts/install-homebrew.sh scripts/install-winget.ps1; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p'
fi
doneRepository: tonythethompson/numan
Length of output: 14109
🌐 Web query:
Cargo reference CARGO_INSTALL_ROOT install.root environment variable documentation
💡 Result:
The CARGO_INSTALL_ROOT environment variable is used to define the root directory where the cargo install command places installed binary crates [1][2]. Executables are specifically placed into a bin subdirectory under this root path [1][2]. Additionally, metadata files used to track installed packages, such as.crates.toml and.crates2.json, are stored within this root directory [1][2]. If not specified, the default installation root is Cargo's home directory, which typically defaults to.cargo in the user's home directory [1][2]. The precedence for determining the installation root, from highest to lowest, is as follows [3][4]: 1. The --root command-line option [3][1] 2. The CARGO_INSTALL_ROOT environment variable [3][1] 3. The install.root Cargo configuration value [3][1] 4. The CARGO_HOME environment variable [3][4] 5. The default location ($HOME/.cargo) [3][4] The install.root configuration value mentioned in Cargo's TOML configuration is equivalent to the CARGO_INSTALL_ROOT environment variable [1][2]. Cargo configuration keys formatted as foo.bar can generally be defined by setting an environment variable of the form CARGO_FOO_BAR [1][2]. In this case, the install.root key corresponds to CARGO_INSTALL_ROOT [1][2].
Citations:
- 1: https://rustwiki.org/en/cargo/reference/config.html
- 2: https://doc.rust-lang.org/stable/cargo/reference/config.html
- 3: https://doc.rust-lang.org/cargo/commands/cargo-install.html
- 4: https://github.com/rust-lang/cargo/blob/master/doc/man/generated_txt/cargo-install.txt
Use an install-only guard boundary.
CARGO_INSTALL_ROOT sets Cargo’s install root path and is resolved after resolving the default .cargo root if absent. It does not prove the current invocation is cargo install, so build.rs can also run the uninstall prompt for configured cargo build/cargo test invocations, while standard cargo install numan-cli without --root may skip it. Gate this with an install-only signal or wrapper instead of treating CARGO_INSTALL_ROOT as the installation boundary.
Update the docs that describe plain cargo install numan-cli as automatically enforcing this guard: AGENTS.md#37, README.md#148, docs/PACKAGING.md#49, and packaging/winget/README.md#29.
📍 Affects 5 files
build.rs#L2-L3(this comment)AGENTS.md#L37-L37README.md#L148-L148docs/PACKAGING.md#L49-L49packaging/winget/README.md#L29-L29
🤖 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 `@build.rs` around lines 2 - 3, Replace the CARGO_INSTALL_ROOT check in
build.rs with a reliable install-only signal or wrapper so
run_cargo_install_guard executes only for cargo install invocations and still
covers standard cargo install numan-cli without --root. Update AGENTS.md line
37, README.md line 148, docs/PACKAGING.md line 49, and
packaging/winget/README.md line 29 to remove or correct claims that plain cargo
install numan-cli automatically enforces the guard.
| "install-guard/**", | ||
| ] | ||
|
|
||
| [[bin]] | ||
| name = "numan" | ||
| path = "src/main.rs" | ||
| [build-dependencies] | ||
| numan-install-guard = { path = "install-guard" } | ||
|
|
||
| [dependencies] | ||
| numan-install-guard = { path = "install-guard" } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make numan-install-guard publishable before releasing numan-cli.
Cargo rejects non-dev path dependencies without a version during cargo package, and manifests for published crates remove path. Since this package excludes install-guard/**, the archive also cannot provide that source. Publish numan-install-guard and switch both [build-dependencies]/[dependencies] to a registry version, or move the guard code into this package.
🤖 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 `@Cargo.toml` around lines 20 - 27, Make numan-install-guard publishable before
numan-cli by publishing the install-guard crate and replacing its path-only
entries in both [build-dependencies] and [dependencies] with the released
registry version. Keep the dependency available in both sections and ensure the
version matches the published crate.
| //! Detect Numan binaries installed via different package managers and gate | ||
| //! cross-channel installs behind an interactive uninstall prompt. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove narrative code comments.
Lines 1-2 and Line 10 describe what the code declares. Remove them or document a non-obvious design reason instead.
As per coding guidelines, “Add comments only when the WHY is non-obvious; do not narrate what the code does.”
Also applies to: 10-10
🤖 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 `@install-guard/src/lib.rs` around lines 1 - 2, Remove the narrative comments
at the module header and line 10; retain comments only if they explain a
non-obvious design reason rather than describing what the code declares or does.
Source: Coding guidelines
| let entries = match std::fs::read_dir(dir) { | ||
| Ok(entries) => entries, | ||
| Err(_) => return, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching install-guard/src/lib.rs:"
if [ -f install-guard/src/lib.rs ]; then
wc -l install-guard/src/lib.rs
echo "--- outline ---"
ast-grep outline install-guard/src/lib.rs || true
echo "--- relevant lines 140-240 ---"
sed -n '140,240p' install-guard/src/lib.rs | nl -ba -v140
else
fd -a 'lib\.rs$' .
fi
echo "--- references to walk_for_numan/conflicting_installs/discover_installations/doctor ---"
rg -n "walk_for_numan|conflicting_installs|discover_installations|doctor|InstallGuard|install guard|Guard" install-guard . -g '*.rs' || trueRepository: tonythethompson/numan
Length of output: 1961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- install-guard/src/lib.rs line 165-222 ---"
sed -n '165,222p' install-guard/src/lib.rs | awk '{printf "%4d\t%s\n", NR+164, $0}'
echo "--- install-guard/src/lib.rs line 256-330 ---"
sed -n '256,330p' install-guard/src/lib.rs | awk '{printf "%4d\t%s\n", NR+256, $0}'
echo "--- cargo/doctor references in repo by file type ---"
rg -n "install-guard|discover_installations|conflicting_installs|doctor|Doctor|RunDoctor|run.*doctor|numan.*doctor|doctor.*numan" -g '*.rs' -g '*.toml' . || trueRepository: tonythethompson/numan
Length of output: 32559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- install-guard/src/lib.rs discovery symbols ---"
sed -n '50,180p' install-guard/src/lib.rs | awk '{printf "%4d\t%s\n", NR+49, $0}'
echo "--- install-guard/src/lib.rs guard return paths ---"
sed -n '202,326p' install-guard/src/lib.rs | awk '{printf "%4d\t%s\n", NR+201, $0}'
echo "--- src/cmd/doctor.rs off-path/install-findings usage ---"
sed -n '315,365p' src/cmd/doctor.rs | awk '{printf "%4d\t%s\n", NR+314, $0}'
echo "--- src/main.rs install guard launch paths ---"
sed -n '1,80p' src/main.rs | awk '{printf "%4d\t%s\n", NR, $0}'Repository: tonythethompson/numan
Length of output: 13580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- src/cmd/install.rs guard call around install entry points ---"
rg -n "numan_install_guard|run_.*_install_guard|install_guard|NUMAN_SKIP_INSTALL_GUARD|skip.*guard|guard" src/cmd/install.rs | head -n 80 || true
echo "--- install command outline ---"
ast-grep outline src/cmd/install.rs --view expanded || true
echo "--- src/cmd/install.rs relevant search regions ---"
for pat in "install_guard|NUMAN_SKIP|install"; do
echo "### $pat"
rg -n "$pat" src/cmd/install.rs -C 2 || true
doneRepository: tonythethompson/numan
Length of output: 1596
Fail closed when installation discovery cannot read a directory.
walk_for_numan turns std::fs::read_dir failures into an empty traversal result at install-guard/src/lib.rs:181-183. discover_installations() then returns an incomplete list, so the install guard and numan doctor can miss an installed Numan channel or off-path binary. Change discovery to return a contexted anyhow::Result; for the install guard, fail closed or skip the guard only when the directory is absent. For doctor, report incomplete detection instead of treating the missing results as no multi-install finding. Also applies to install-guard/src/lib.rs:218-222.
🤖 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 `@install-guard/src/lib.rs` around lines 181 - 183, Update walk_for_numan and
the related discovery flow in discover_installations to propagate read_dir
failures as contextual anyhow::Result errors instead of returning an empty
traversal. Treat an absent directory as the only case that may skip the install
guard; otherwise fail closed, and ensure numan doctor reports incomplete
detection rather than interpreting missing results as no multi-install finding.
Source: Coding guidelines
| fn should_skip_guard() -> bool { | ||
| std::env::var("NUMAN_SKIP_INSTALL_GUARD") | ||
| .map(|v| v == "1") | ||
| .unwrap_or(false) | ||
| || std::env::var("CI").ok().as_deref() == Some("true") | ||
| || std::env::var("GITHUB_ACTIONS").ok().as_deref() == Some("true") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept common truthy CI values.
CI=1 does not bypass the guard. If that job detects another install, Lines 269-275 cancel the non-interactive installation. Accept 1 and case-insensitive true for CI flags.
🤖 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 `@install-guard/src/lib.rs` around lines 327 - 332, Update should_skip_guard to
treat CI and GITHUB_ACTIONS as enabled when their values are either "1" or
case-insensitive "true", while preserving the existing NUMAN_SKIP_INSTALL_GUARD
behavior and guard bypass logic.
Source: MCP tools
| fn uninstall_channel(channel: InstallChannel) -> bool { | ||
| match channel { | ||
| InstallChannel::Cargo => run_command("cargo", &["uninstall", "numan-cli"]), | ||
| InstallChannel::Winget => run_command( | ||
| "winget", | ||
| &[ | ||
| "uninstall", | ||
| "--id", | ||
| "tonythethompson.numan", | ||
| "--exact", | ||
| "--accept-source-agreements", | ||
| ], | ||
| ), | ||
| InstallChannel::Homebrew => run_command("brew", &["uninstall", "numan"]), | ||
| InstallChannel::ReleaseArchive | InstallChannel::Unknown => false, | ||
| } | ||
| } | ||
|
|
||
| fn run_command(program: &str, args: &[&str]) -> bool { | ||
| Command::new(program) | ||
| .args(args) | ||
| .stdin(Stdio::inherit()) | ||
| .stdout(Stdio::inherit()) | ||
| .stderr(Stdio::inherit()) | ||
| .status() | ||
| .map(|status| status.success()) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file locations =="
git ls-files | rg '(^|/)install-guard/src/lib\.rs$|install-guard|Root|acquire_mutation_lock|lockfile|numan-cli|winget|homebrew' || true
echo
echo "== install-guard outline =="
ast-grep outline install-guard/src/lib.rs --view compact || true
echo
echo "== relevant install-guard section =="
sed -n '1,460p' install-guard/src/lib.rs
echo
echo "== search acquire_mutation_lock in install-guard =="
rg -n "acquire_mutation_lock|snapshot|lockfile|root" install-guard -S || true
echo
echo "== search mutation helpers repo-wide =="
rg -n "acquire_mutation_lock|write_json_atomic|snapshot" . -S | head -200 || trueRepository: tonythethompson/numan
Length of output: 40422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== install-guard Cargo.toml =="
cat -n install-guard/Cargo.toml
echo
echo "== main entrypoints =="
sed -n '1,240p' install-guard/src/main.rs
echo
echo "== mutation_lock implementation =="
rg -n -C 5 "pub fn acquire_mutation_lock|fn acquire_mutation_lock|struct MutationLock|impl MutationLock|create_snapshot" src -S
echo
echo "== fs_safety outline/locations =="
fd -a 'fs_safety\.rs|lockfile\.rs|snapshot' src | sed 's#^\./##'
rg -n "use crate::util::fs_safety|acquire_mutation_lock|Lockfile::snapshot|write_json_atomic" src/install src/util src/state src/nupm_compat docs -S
echo
echo "== precise call sites for Lockfile snapshot methods =="
rg -n -C 3 "Lockfile::snapshot\(|\.snapshot\(|create_snapshot\(" src -SRepository: tonythethompson/numan
Length of output: 50379
Serialize the guard’s uninstall path behind the root mutation lock.
numan-install-guard now runs cargo uninstall, winget uninstall, or brew uninstall inside run_install_guard. Concurrent guard runs can detect the same existing install and start these package-manager mutations concurrently. Acquire the installation-root mutation lock and create a pre-mutation snapshot before uninstalling; if the standalone guard cannot determine an authoritative root/snapshot, stop before mutation and print the appropriate uninstall command.
🤖 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 `@install-guard/src/lib.rs` around lines 354 - 380, Update the uninstall flow
in run_install_guard and uninstall_channel so package-manager mutations are
serialized under the installation-root mutation lock. Resolve an authoritative
root and create the pre-mutation snapshot before invoking run_command; if the
standalone guard cannot obtain either, do not uninstall and instead print the
appropriate uninstall command. Ensure the lock and snapshot cover the entire
cargo, winget, or brew uninstall operation.
Source: Coding guidelines
| fn conflicting_installs_excludes_paths() { | ||
| let excluded = PathBuf::from("/tmp/excluded/numan"); | ||
| let installs = vec![ | ||
| DiscoveredInstall { | ||
| path: excluded.clone(), | ||
| channel: InstallChannel::Winget, | ||
| }, | ||
| DiscoveredInstall { | ||
| path: PathBuf::from("/tmp/other/numan"), | ||
| channel: InstallChannel::Cargo, | ||
| }, | ||
| ]; | ||
| let filtered = installs | ||
| .into_iter() | ||
| .filter(|install| !std::slice::from_ref(&excluded).contains(&install.path)) | ||
| .filter(|install| install.channel != InstallChannel::Winget) | ||
| .collect::<Vec<_>>(); | ||
| assert_eq!(filtered.len(), 1); | ||
| assert_eq!(filtered[0].channel, InstallChannel::Cargo); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test conflicting_installs through an injectable discovery seam.
This test duplicates the filter predicates locally. It never calls conflicting_installs, so it cannot detect a regression in that function.
Inject installation discovery or candidate roots. Test target exclusion, same-channel filtering, unreadable directories, and non-interactive conflict cancellation.
As per coding guidelines, “Add or update tests for behavior changes, including relevant failure paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install-guard/src/lib.rs` around lines 414 - 433, Replace the local filtering
in conflicting_installs_excludes_paths with a call to the production
conflicting_installs function through an injectable discovery or candidate-roots
seam. Extend the tests to cover excluded targets, same-channel installs,
unreadable directories, and cancellation in non-interactive mode, ensuring each
assertion exercises the real conflicting_installs behavior.
Source: Coding guidelines
| fn main() -> ExitCode { | ||
| match env::args().nth(1).map(|s| s.to_ascii_lowercase()) { | ||
| Some(arg) if arg == "cargo" => run_cargo_install_guard(), | ||
| Some(arg) if arg == "winget" => run_winget_install_guard(), | ||
| Some(arg) if arg == "brew" || arg == "homebrew" => run_homebrew_install_guard(), | ||
| _ => { | ||
| eprintln!("usage: numan-install-guard <cargo|winget|brew>"); | ||
| ExitCode::from(2) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'Cargo.toml|main.rs' . | sed 's#^\./##'
echo
echo "== install-guard Cargo.toml =="
cat -n install-guard/Cargo.toml
echo
echo "== install-guard main.rs =="
cat -n install-guard/src/main.rs
echo
echo "== clap usages in repo =="
rg -n "use clap|clap::|Command|Subcommand" --glob '*.rs' .
echo
echo "== cargo dependencies containing clap =="
rg -n "clap\s*=|dependencies" Cargo.toml install-guard/Cargo.tomlRepository: tonythethompson/numan
Length of output: 15778
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
echo "== crates.io clap 4.6.5 derive feature metadata =="
curl -s https://crates.io/api/v1/crates/clap/4.6.5 --fail | jq '{name, version, features}'
echo
echo "== crates.io clap 4.6.5 Cargo.toml features =="
curl -sL https://crates.io/api/v1/crates/clap/4.6.5/download | tar -xOzf - clap-4.6.5/Cargo.toml | sed -n '/\[features\]/,/^$/p'
Length of output: 178
🌐 Web query:
clap Rust derive feature version 4 documentation
💡 Result:
To use the derive API in clap version 4, you must enable the derive feature flag [1][2]. You can enable this feature by adding clap to your Cargo.toml with the derive feature enabled [2]: cargo add clap --features derive Once enabled, the derive API allows you to define your command-line interface using structs, enums, and derive macros such as #[derive(Parser)], #[derive(Args)], #[derive(Subcommand)], and #[derive(ValueEnum)] [1][3]. The derive feature works by depending on the clap_derive crate [4]. You can refer to the official clap documentation, specifically the Derive Reference and the Derive Tutorial, for detailed usage examples and configuration options [1][2][5].
Citations:
- 1: https://docs.rs/clap/4.6.0/clap/_derive/
- 2: https://docs.rs/clap/latest/clap/_derive/_tutorial/index.html
- 3: https://docs.rs/clap/latest/clap/_derive/
- 4: https://crates.io/crates/clap
- 5: https://docs.rs/clap/latest/clap/
Use clap derives for the install guard CLI.
install-guard/src/main.rs manually matches env::args().nth(1), and install-guard/Cargo.toml has no CLI dependencies. Move this command into install-guard/src/cli.rs with #[derive(Parser)]/Subcommand, add clap = "4", and call it from main. No change should mutate install-guard/Cargo.lock.
🤖 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 `@install-guard/src/main.rs` around lines 8 - 18, Replace the manual argument
matching in main with a clap-based CLI module: add install-guard/src/cli.rs
defining Parser and Subcommand-derived types for cargo, winget, and
brew/homebrew commands, add the clap 4 dependency to Cargo.toml, and have main
parse and dispatch the CLI values. Preserve the existing usage/error behavior
where applicable, and do not modify Cargo.lock.
Source: Coding guidelines
| [CmdletBinding()] | ||
| param( | ||
| [Parameter(ValueFromRemainingArguments = $true)] | ||
| [string[]]$CargoArgs = @("install", "--path", ".") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve the default Cargo path from the script directory.
$repoRoot uses the script location, but the default --path . uses the caller’s current directory. If a user invokes this wrapper outside the checkout, Cargo installs the wrong directory or fails to find a manifest. Set the default path after $repoRoot is known.
Proposed fix
$repoRoot = Split-Path -Parent $PSScriptRoot
$guardManifest = Join-Path $repoRoot "install-guard\Cargo.toml"
+
+if (-not $PSBoundParameters.ContainsKey("CargoArgs")) {
+ $CargoArgs = @("install", "--path", $repoRoot)
+}Also applies to: 18-19, 31-32
🤖 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 `@scripts/install-cargo.ps1` at line 13, Update the install-cargo wrapper so
the default Cargo --path is derived from the script’s resolved repository root
instead of the caller’s current directory. In the install flow around $repoRoot
and $CargoArgs, set or rewrite the default path after $repoRoot is established
so cargo install always targets the checkout the script belongs to, while
preserving any explicit path overrides.
- Format install guard sources - Remove install guard bypass flag - Use PATH-only doctor scanning
Qodo Fixer✅ Committed (4) · ☑ Fixed (4) Commits pushed directly to this PR — no separate fix PR opened. Process — 4 fixed
|
Move install-channel logic to src/util/install_channel.rs shared by build.rs, doctor, and the numan-install-guard binary. Removes the path-dependency install-guard crate so cargo package and cargo deny succeed. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/cmd/doctor.rs (2)
346-354: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse channel-neutral wording in the diagnostic.
InstallChannelincludesReleaseArchiveandUnknown, but Line 347 says “different package managers.” The warning can therefore describe a release archive or unknown binary as a package manager. Replace that phrase with “different installation channels” to match theinstall.multiple_channelsfinding and the labels below.🤖 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/doctor.rs` around lines 346 - 354, Update the diagnostic text in the `install.multiple_channels` handling within `doctor.rs` so the first line no longer says “different package managers” and instead uses channel-neutral wording like “different installation channels.” Keep the existing loop over `installs` and `install.channel.label()` output unchanged so the message stays aligned with `InstallChannel` values such as `ReleaseArchive` and `Unknown`.
340-369: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not generate uninstall hints when the current channel is unknown.
current_exe().ok()maps lookup failures toInstallChannel::Unknown, andclassify_binary_pathcan also returnUnknown. Lines 365-369 then treat every known channel as an installation from another channel. If the running executable belongs to one of those channels, the manual fix can recommend uninstalling the active installation.Keep the warning, but leave
fixunset until the current executable is matched to a known channel. Add regression tests for lookup failure and unknown classification.As per coding guidelines, Rust error paths should return
anyhow::Resultwith context.🤖 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/doctor.rs` around lines 340 - 369, Update the warning assembly in doctor.rs so `fix` is only populated when `current_exe()` resolves to a known install channel via `classify_binary_path`; if `current_channel` is `InstallChannel::Unknown`, leave `fix` unset and keep the warning text. Adjust the `fix` filter logic around `installs`/`uninstall_hint` to skip uninstall recommendations until the running executable is positively matched to a known channel. If you touch the executable lookup path, return `anyhow::Result` with context instead of silently converting failures to `Unknown`. Add regression coverage for both `current_exe()` lookup failure and `classify_binary_path` returning `Unknown`.Source: Coding guidelines
install-guard/src/lib.rs (4)
381-432: 📐 Maintainability & Code Quality | 🔵 TrivialRun the required Rust gates before declaring this ready.
Run
cargo test,cargo clippy -- -D warnings, andcargo fmt --check. Validate the guarded installer flows on Ubuntu, Windows, and macOS before release.As per coding guidelines, all Rust CI gates must pass:
cargo test,cargo clippy -- -D warnings, andcargo fmt --check.🤖 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 `@install-guard/src/lib.rs` around lines 381 - 432, Before considering the installer changes complete, run the Rust validation gates: cargo test, cargo clippy -- -D warnings, and cargo fmt --check. Resolve any failures, then validate the guarded installer flows on Ubuntu, Windows, and macOS.Source: Coding guidelines
307-321: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreflight manual-only conflicts before mutating state.
ReleaseArchiveandUnknownhave no automatic uninstall hint, but the loop still attempts every conflict. If a supported channel appears before one of these channels, the guard removes the supported installation and then cancels on the unsupported one. The user loses an installation and the requested installation does not proceed.Reject manual-only conflicts before running any uninstall command. Print the path and manual action first.
🤖 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 `@install-guard/src/lib.rs` around lines 307 - 321, Before the uninstall loop, preflight all conflicts whose channels lack an automatic uninstall action or hint, such as ReleaseArchive and Unknown; print each affected path and the required manual action, then return ExitCode::from(1) without mutating state. Only proceed to the existing uninstall_channel loop when every conflict is automatically removable.
307-321: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize uninstall mutations and snapshot before writes.
After confirmation, Line 308 starts package-manager removal without
acquire_mutation_lock(root)or a pre-mutation lockfile snapshot. Two guard runs can act on the same stale discovery result and mutate installation state concurrently. The check at Line 321 does not repair that race.Resolve the authoritative root before mutation. Acquire the mutation lock and create the snapshot before the first uninstall. Hold the lock through the complete uninstall sequence. If the standalone guard cannot obtain these, stop before mutation and print the manual command.
As per coding guidelines, every mutating command must call
acquire_mutation_lock(root), and new mutating paths must snapshot the lockfile before writes.🤖 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 `@install-guard/src/lib.rs` around lines 307 - 321, Before the uninstall loop in the guard flow, resolve the authoritative root, acquire_mutation_lock(root), and create the required pre-mutation lockfile snapshot; abort before any uninstall and print the manual command if either setup step fails. Hold the acquired lock through the complete conflict uninstall sequence, including the calls to uninstall_channel, and retain the existing failure handling.Source: Coding guidelines
307-315: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUninstall each detected copy instead of running channel-level uninstall commands again.
At
src/util/install_channel.rs:319-339, the loop iterates byDiscoveredInstall, butuninstall_channel(conflict.channel)runscargo uninstall numan-cli,winget, orbrewwithout the discovered path. For Cargo withCARGO_INSTALL_ROOTset, this can uninstall a different home root; duplicate discoverednumanbinaries also re-run the same package-manager command. Pass the detected install identity into the uninstall routine and avoid re-running the same command for multiple paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install-guard/src/lib.rs` around lines 307 - 315, Update the conflict-removal loop and its uninstall helper so each DiscoveredInstall is removed using its detected path and install identity, rather than repeatedly invoking channel-level commands through uninstall_channel. Ensure duplicate discoveries for the same channel do not rerun the package-manager uninstall command, while preserving the existing failure message and uninstall hint behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 23-26: Update the release pipeline around the numan-cli
publish/package flow to ensure numan-install-guard version 0.1.0 is published to
the registry before any CLI packaging or publish step runs. Keep the local
path-plus-version dependency in Cargo.toml for development, but make the release
ordering in the pipeline explicitly publish the guard first so the packaged
numan-cli can resolve numan-install-guard = 0.1.0 from the registry.
In `@install-guard/src/lib.rs`:
- Around line 84-93: Update discover_path_installations and its callers to
return or propagate a contexted error when path discovery cannot read an
existing root, rather than treating it as an empty result. In the underlying
push_if_numan scan, skip only roots that are genuinely missing; propagate all
other directory-read failures so the install guard and src/cmd/doctor.rs fail
closed instead of omitting cross-channel or install.multiple_channels findings.
---
Outside diff comments:
In `@install-guard/src/lib.rs`:
- Around line 381-432: Before considering the installer changes complete, run
the Rust validation gates: cargo test, cargo clippy -- -D warnings, and cargo
fmt --check. Resolve any failures, then validate the guarded installer flows on
Ubuntu, Windows, and macOS.
- Around line 307-321: Before the uninstall loop, preflight all conflicts whose
channels lack an automatic uninstall action or hint, such as ReleaseArchive and
Unknown; print each affected path and the required manual action, then return
ExitCode::from(1) without mutating state. Only proceed to the existing
uninstall_channel loop when every conflict is automatically removable.
- Around line 307-321: Before the uninstall loop in the guard flow, resolve the
authoritative root, acquire_mutation_lock(root), and create the required
pre-mutation lockfile snapshot; abort before any uninstall and print the manual
command if either setup step fails. Hold the acquired lock through the complete
conflict uninstall sequence, including the calls to uninstall_channel, and
retain the existing failure handling.
- Around line 307-315: Update the conflict-removal loop and its uninstall helper
so each DiscoveredInstall is removed using its detected path and install
identity, rather than repeatedly invoking channel-level commands through
uninstall_channel. Ensure duplicate discoveries for the same channel do not
rerun the package-manager uninstall command, while preserving the existing
failure message and uninstall hint behavior.
In `@src/cmd/doctor.rs`:
- Around line 346-354: Update the diagnostic text in the
`install.multiple_channels` handling within `doctor.rs` so the first line no
longer says “different package managers” and instead uses channel-neutral
wording like “different installation channels.” Keep the existing loop over
`installs` and `install.channel.label()` output unchanged so the message stays
aligned with `InstallChannel` values such as `ReleaseArchive` and `Unknown`.
- Around line 340-369: Update the warning assembly in doctor.rs so `fix` is only
populated when `current_exe()` resolves to a known install channel via
`classify_binary_path`; if `current_channel` is `InstallChannel::Unknown`, leave
`fix` unset and keep the warning text. Adjust the `fix` filter logic around
`installs`/`uninstall_hint` to skip uninstall recommendations until the running
executable is positively matched to a known channel. If you touch the executable
lookup path, return `anyhow::Result` with context instead of silently converting
failures to `Unknown`. Add regression coverage for both `current_exe()` lookup
failure and `classify_binary_path` returning `Unknown`.
🪄 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: 674101b8-99ed-4445-bb31-3b9358601311
📒 Files selected for processing (4)
Cargo.tomlinstall-guard/Cargo.tomlinstall-guard/src/lib.rssrc/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: MSRV (1.88)
- 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:
install-guard/Cargo.tomlCargo.tomlsrc/cmd/doctor.rsinstall-guard/src/lib.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:
install-guard/Cargo.tomlCargo.tomlsrc/cmd/doctor.rsinstall-guard/src/lib.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run and keep
cargo fmt/rustfmtclean, and ensurecargo clippy -- -D warningspasses.
Files:
install-guard/Cargo.tomlCargo.tomlsrc/cmd/doctor.rsinstall-guard/src/lib.rs
**/*.{rs,md,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's established serialization and module conventions rather than introducing unrelated refactors.
Files:
install-guard/Cargo.tomlCargo.tomlsrc/cmd/doctor.rsinstall-guard/src/lib.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Maintain MSRV Rust 1.88 via the crate’s
rust-versionsetting.
Files:
Cargo.toml
**/*.{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.rsinstall-guard/src/lib.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/doctor.rsinstall-guard/src/lib.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/doctor.rsinstall-guard/src/lib.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/doctor.rsinstall-guard/src/lib.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use paths and names through environment variables (
NUMAN_PLUGIN_BINARY,NUMAN_PLUGIN_CONFIG,NUMAN_PLUGIN_NAME); the Nu program string must be a compile-time constant with no runtime interpolation.
Files:
src/cmd/doctor.rs
🔇 Additional comments (12)
src/cmd/doctor.rs (4)
36-36: LGTM!
199-199: LGTM!
327-338: LGTM!
356-363: LGTM!install-guard/src/lib.rs (5)
1-39: 📐 Maintainability & Code QualityVerify that module comments explain a non-obvious WHY.
The current range summary does not show the module header or the comment at Line 10. A previous review flagged those comments as narrative. Remove them unless they explain a non-obvious design reason.
As per coding guidelines, add comments only when the WHY is non-obvious.
Source: Coding guidelines
381-432: 🎯 Functional CorrectnessVerify that tests exercise production conflict filtering.
Confirm that
conflicting_installs_excludes_pathscallsconflicting_installs()through an injectable discovery or candidate-root seam. A local copy of the predicates can pass while production filtering regresses.Add cases for excluded targets, same-channel filtering, unreadable roots, unsupported manual-only conflicts, and non-interactive cancellation.
As per coding guidelines, tests must cover behavior changes and relevant failure paths.
Source: Coding guidelines
41-81: LGTM!
235-264: LGTM!
272-281: 🎯 Functional CorrectnessVerify the interactive Cargo path end to end.
build.rsinvokesrun_cargo_install_guard()duringcargo install, but Line 275 refuses when standard input is not a terminal. Confirm that Cargo preserves the user's TTY for the build script. If it does not, every conflict cancels the installation and the documented prompt is unreachable.install-guard/Cargo.toml (2)
7-11: LGTM!
5-5: 📐 Maintainability & Code QualityNo changes needed.
numan-install-guardis an[[bin]]in the root manifest, whoserust-version = "1.88"applies to both binary targets.Cargo.toml (1)
79-82: LGTM!
| numan-install-guard = { path = "install-guard", version = "0.1.0" } | ||
|
|
||
| [dependencies] | ||
| numan-install-guard = { path = "install-guard", version = "0.1.0" } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial
Publish the guard before packaging numan-cli.
The path-plus-version dependency is suitable for local development, but a packaged numan-cli artifact requires numan-install-guard = 0.1.0 from the registry. Verify that the release pipeline publishes that exact guard version before the CLI package and publish stages. Otherwise the release pipeline can fail before producing the CLI artifact.
🤖 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 `@Cargo.toml` around lines 23 - 26, Update the release pipeline around the
numan-cli publish/package flow to ensure numan-install-guard version 0.1.0 is
published to the registry before any CLI packaging or publish step runs. Keep
the local path-plus-version dependency in Cargo.toml for development, but make
the release ordering in the pipeline explicitly publish the guard first so the
packaged numan-cli can resolve numan-install-guard = 0.1.0 from the registry.
| pub fn discover_path_installations() -> Vec<DiscoveredInstall> { | ||
| let mut seen = HashSet::new(); | ||
| let mut out = Vec::new(); | ||
| for dir in path_directories() { | ||
| push_if_numan(&dir, &mut seen, &mut out); | ||
| } | ||
| out.sort_by(|a, b| a.path.cmp(&b.path)); | ||
| out | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat unreadable discovery as a clean result.
The scan converts directory-read failures into “no matching binary”. This makes discover_path_installations() and the full discovery path indistinguishable from a complete empty scan. The install guard can continue while a cross-channel installation is hidden, and src/cmd/doctor.rs can omit install.multiple_channels.
Return a contexted error or an explicit incomplete result. Skip only missing roots. Fail closed for unreadable roots.
As per coding guidelines, error paths must return contexted results instead of hiding failures.
Also applies to: 181-183, 218-222
🤖 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 `@install-guard/src/lib.rs` around lines 84 - 93, Update
discover_path_installations and its callers to return or propagate a contexted
error when path discovery cannot read an existing root, rather than treating it
as an empty result. In the underlying push_if_numan scan, skip only roots that
are genuinely missing; propagate all other directory-read failures so the
install guard and src/cmd/doctor.rs fail closed instead of omitting
cross-channel or install.multiple_channels findings.
Source: Coding guidelines
Detect cargo install via OUT_DIR cargo-install path when CARGO_INSTALL_ROOT is unset. Deduplicate doctor uninstall hints and fix docs capitalization. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
install-guardcrate to detect numan binaries installed via cargo, winget, Homebrew, or release archivescargo install(build.rs): prompt to uninstall other channels first; declining cancels the installscripts/install-winget.ps1) and Homebrew (scripts/install-homebrew.sh)numan doctorwarninginstall.multiple_channelswhen mixed install channels are detectedTest plan
cargo test -p numan-install-guardcargo test cmd::doctorcargo clippy -- -D warningsMade with Cursor