fix: Setup no longer reports optional npm cleanup failures#1452
Conversation
Keep the success message when the installer removes the old npm package, but avoid surfacing optional cleanup failures during native CLI setup. Update installer tests and docs to match the quieter behavior.
Keep legacy npm cleanup failures quiet while still checking the persisted Machine and User PATH that new Windows shells will use. This avoids reporting setup success when a remaining legacy shim shadows the native CLI.
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR removes manual npm-uninstall guidance messages from the Windows PowerShell and POSIX installer scripts, replacing them with silent early returns on failure. It reworks Windows PATH-shadowing detection to resolve uloop commands from Machine+User PATH, updates associated Go and shell tests, and bumps the dispatcher contract version. ChangesLegacy npm cleanup silencing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InstallScript
participant ReportPathShadowing
participant GetFirstUloopCommandFromPath
participant InvokeAllLegacyNpmPackageRemoval
InstallScript->>ReportPathShadowing: run at end of install
ReportPathShadowing->>GetFirstUloopCommandFromPath: resolve uloop from Machine+User PATH
GetFirstUloopCommandFromPath-->>ReportPathShadowing: resolved path
ReportPathShadowing->>InstallScript: print shadowing message if mismatched
InstallScript->>InvokeAllLegacyNpmPackageRemoval: invoke cleanup | Out-Null
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
97-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe manual uninstall example can miss the shadowing install.
The condition here is specifically “the old npm command still shadows the native CLI”. In that case the shadowing shim may belong to a different npm prefix, which is why the installer code now infers
--prefixfrom the resolvedulooppath. Telling readers to run plainnpm uninstall -g uloop-clican remove a different global install and leave the shadowing shim untouched. Please point readers to the exactulooppath fromwhich/Get-Command, or mention uninstalling from the matching prefix instead.🤖 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 `@README.md` around lines 97 - 102, The manual uninstall guidance for the native CLI can miss the actual shadowing shim, so update the README instructions tied to the native install/uninstall flow to tell users to uninstall from the same npm prefix as the resolved uloop command. Reference the existing “old npm command still shadows the native CLI” guidance and make the example point to the exact uloop path from which/Get-Command, or explicitly say to run npm uninstall with the matching --prefix instead of a plain global uninstall.README_ja.md (1)
96-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis manual uninstall example can target the wrong npm prefix.
Because this section is now about the case where the old npm command still shadows the native CLI, the stale shim may belong to a different global prefix. The installer code handles that by inferring the prefix from the resolved
ulooppath, butnpm uninstall -g uloop-clidoes not. Please adjust the Japanese guidance to tell users to remove the exactuloopthey resolved, or to uninstall from the matching prefix.🤖 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 `@README_ja.md` around lines 96 - 101, The Japanese uninstall guidance should not hardcode npm uninstall -g uloop-cli because it can point at the wrong global prefix when the old shim is shadowing the native CLI. Update the README_ja.md instructions to tell users to remove the exact resolved uloop command/path they found, or to uninstall from the matching npm prefix inferred from that resolved path, so the guidance aligns with the installer 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 `@cli/internal/install/command.go`:
- Around line 293-309: `Report-PathShadowing` is passing raw Machine/User PATH
values into `Get-FirstUloopCommandFromPath`, so quoted or slash-terminated
entries may fail path probing and hide the shadowing warning. Update the PATH
scanning logic to normalize each split segment with `$NormalizePath` before
checking for `uloop*`, and keep the existing comparison against
`$ExpectedUloopPath` and legacy-path warning flow intact.
---
Outside diff comments:
In `@README_ja.md`:
- Around line 96-101: The Japanese uninstall guidance should not hardcode npm
uninstall -g uloop-cli because it can point at the wrong global prefix when the
old shim is shadowing the native CLI. Update the README_ja.md instructions to
tell users to remove the exact resolved uloop command/path they found, or to
uninstall from the matching npm prefix inferred from that resolved path, so the
guidance aligns with the installer behavior.
In `@README.md`:
- Around line 97-102: The manual uninstall guidance for the native CLI can miss
the actual shadowing shim, so update the README instructions tied to the native
install/uninstall flow to tell users to uninstall from the same npm prefix as
the resolved uloop command. Reference the existing “old npm command still
shadows the native CLI” guidance and make the example point to the exact uloop
path from which/Get-Command, or explicitly say to run npm uninstall with the
matching --prefix instead of a plain global uninstall.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f4c74be-ca1b-4383-9233-8312721884fb
⛔ Files ignored due to path filters (1)
scripts/install.ps1is excluded by none and included by none
📒 Files selected for processing (7)
README.mdREADME_ja.mdcli/internal/install/command.gocli/internal/install/command_test.gocli/internal/install/posix_install.goscripts/install.shscripts/test-install-release-filter.sh
💤 Files with no reviewable changes (1)
- cli/internal/install/posix_install.go
Leave the README wording unchanged because the installer behavior change should stay scoped to setup output and tests.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Advance the dispatcher release version because this PR changes dispatcher release inputs in the installer scripts.
Normalize persisted Machine and User PATH entries before probing for uloop shims, and remove the unused aggregate cleanup result now that cleanup failures are intentionally silent.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Keep normalized drive-root PATH entries usable for uloop shadow detection by restoring the trailing slash before filesystem probing.
1 participant
Summary
User Impact
Changes
Verification
go test ./internal/install -run TestCommandForWindowsConfiguresUserPathAndLegacyCleanupscripts/test-install-release-filter.shgo test ./internal/installscripts/check-go-cli.shgo run ./cmd/check-dispatcher-version-bump --base origin/v3-beta --head HEADautoreview --mode branch --base origin/v3-beta