fix: Setup recognizes existing npm CLI installs#1131
Merged
Conversation
Use the macOS directory service shell when GUI-launched Unity does not inherit SHELL, so npm-installed legacy uloop commands remain visible as update candidates instead of looking absent. Add focused coverage for shell selection and directory service output parsing.
Execute command discovery and `uloop -v` in the same login shell so npm shims resolve node exactly as they do from the user's terminal. Keep path data as optional UI context, but treat version output as the installed-state signal.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds login-shell CLI detection on non-Windows using marker-delimited stdout parsing and short "-v" version flag; improves user-shell selection with macOS dscl lookup and safety checks; centralizes setup-wizard skills-target visibility logic; and implements package-removal settings reset with tests. ChangesShell-Based CLI Detection and Resolution
Setup Wizard Skills UI
Package-Removal Settings Reset
Sequence Diagram(s)sequenceDiagram
participant Detector as CliInstallationDetector
participant Shell as Login Shell
participant Process as Subprocess
participant Parser as Output Parser
Detector->>Detector: BuildShellCliDetectionCommand() (uses SHORT_VERSION_FLAG)
Detector->>Process: ExecuteShellCommandBlocking(shell, command, timeout)
Process->>Shell: launch login shell -c "command -v <cmd> && <cmd> -v"
Shell->>Process: Emit path and version between markers
Process-->>Detector: stdout (trimmed)
Detector->>Parser: ParseShellCliInstallationOutput(stdout)
Parser-->>Detector: CliInstallationDetection{ExecutablePath?, Version?}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Packages/src/Editor/Infrastructure/Utils/NodeEnvironmentResolver.cs (2)
184-184: 💤 Low valueDuplicate
/bin/shliteral across modules.
POSIX_FALLBACK_SHELL_PATHhere duplicatesCliConstants.POSIX_SHELL_EXECUTABLE_PATHin the Domain layer. Consider consolidating to a single source of truth so future changes to the fallback shell only need to land in one place. Skip if you want to keep Infrastructure free of Domain references.🤖 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 `@Packages/src/Editor/Infrastructure/Utils/NodeEnvironmentResolver.cs` at line 184, The constant POSIX_FALLBACK_SHELL_PATH in NodeEnvironmentResolver duplicates CliConstants.POSIX_SHELL_EXECUTABLE_PATH from the Domain layer; replace the literal with a single shared symbol by referencing CliConstants.POSIX_SHELL_EXECUTABLE_PATH (or, if you must avoid a Domain dependency, add a single shared constant in a common utilities library and use that from NodeEnvironmentResolver and the Domain class) so the fallback shell path is defined in one place only.
287-308: 💤 Low valueConsider handling multi-line
dscloutput format inExtractDirectoryServiceUserShell.The
dsclcommand switches to multi-line output format when attribute values contain spaces, emitting the attribute key on one line followed by the value indented on the next. The current parser only matches the single-lineUserShell: <value>format, so multi-line responses would returnnulland fall back to/bin/sh. While UserShell paths are typically simple and single-line (e.g.,/bin/zsh), supporting both formats is straightforward—check whether the trimmed line equals the prefix alone, then look ahead to the next line for the value.🤖 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 `@Packages/src/Editor/Infrastructure/Utils/NodeEnvironmentResolver.cs` around lines 287 - 308, ExtractDirectoryServiceUserShell currently only handles single-line "UserShell: <value>" outputs and misses the multi-line dscl format where a line may equal DIRECTORY_SERVICE_USER_SHELL_PREFIX alone and the next indented line contains the value; update ExtractDirectoryServiceUserShell to, when a trimmed line exactly equals DIRECTORY_SERVICE_USER_SHELL_PREFIX, peek at the next raw line (if any), trim it and treat that as the shell value (falling back to existing empty-check behavior), otherwise keep the existing substring logic for same-line matches; reference DIRECTORY_SERVICE_USER_SHELL_PREFIX and the ExtractDirectoryServiceUserShell method when making the change.
🤖 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 `@Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs`:
- Around line 197-236: The ExecuteAndGetOutput method has two unguarded
Process.Kill() calls that can throw if the process already exited; update the
cancellation registration callback and the timeout Kill to mirror
DetectCliInstallationAtExecutablePath by wrapping process.Kill() in a try/catch
that swallows System.InvalidOperationException (or the specific exception thrown
on already-exited processes) and leaves other exceptions unchanged, i.e., change
the ct.Register(() => process.Kill()) callback to a try { process.Kill(); }
catch (InvalidOperationException) { } and similarly wrap the bare process.Kill()
after WaitForExit(PROCESS_TIMEOUT_MS) in the same try/catch to prevent the
TOCTOU race from crashing ExecuteAndGetOutput.
---
Nitpick comments:
In `@Packages/src/Editor/Infrastructure/Utils/NodeEnvironmentResolver.cs`:
- Line 184: The constant POSIX_FALLBACK_SHELL_PATH in NodeEnvironmentResolver
duplicates CliConstants.POSIX_SHELL_EXECUTABLE_PATH from the Domain layer;
replace the literal with a single shared symbol by referencing
CliConstants.POSIX_SHELL_EXECUTABLE_PATH (or, if you must avoid a Domain
dependency, add a single shared constant in a common utilities library and use
that from NodeEnvironmentResolver and the Domain class) so the fallback shell
path is defined in one place only.
- Around line 287-308: ExtractDirectoryServiceUserShell currently only handles
single-line "UserShell: <value>" outputs and misses the multi-line dscl format
where a line may equal DIRECTORY_SERVICE_USER_SHELL_PREFIX alone and the next
indented line contains the value; update ExtractDirectoryServiceUserShell to,
when a trimmed line exactly equals DIRECTORY_SERVICE_USER_SHELL_PREFIX, peek at
the next raw line (if any), trim it and treat that as the shell value (falling
back to existing empty-check behavior), otherwise keep the existing substring
logic for same-line matches; reference DIRECTORY_SERVICE_USER_SHELL_PREFIX and
the ExtractDirectoryServiceUserShell method when making the change.
🪄 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: 777c6346-7dbc-4234-9d2b-c2c86edf8d35
📒 Files selected for processing (5)
Assets/Tests/Editor/CliInstallationDetectorTests.csAssets/Tests/Editor/NodeEnvironmentResolverTests.csPackages/src/Editor/Domain/CliConstants.csPackages/src/Editor/Infrastructure/CLI/CliInstallationDetector.csPackages/src/Editor/Infrastructure/Utils/NodeEnvironmentResolver.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs">
<violation number="1" location="Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs:229">
P2: Wrap timeout `process.Kill()` with `InvalidOperationException` handling to avoid race-condition crashes when the process exits just before kill.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The setup wizard should let first-time users choose a skill target before the CLI is installed. Split the target-row visibility from CLI skill management so the dropdown remains visible while status rows still wait for CLI inspection.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Assets/Tests/Editor/SetupWizardWindowTests.cs (1)
338-358: ⚡ Quick winConsider adding test cases for
shouldUseFirstInstallSkillsUi = true.The test coverage for
ShouldShowSkillsTargetListForSetupWizardis incomplete. The current tests verify behavior whenshouldUseFirstInstallSkillsUi = false, but miss the cases when it'strue. Given the implementation logic (canManageSkills && !shouldUseFirstInstallSkillsUi), both missing combinations should returnfalse:
canManageSkills = false, shouldUseFirstInstallSkillsUi = true→falsecanManageSkills = true, shouldUseFirstInstallSkillsUi = true→falseAdding these cases ensures complete coverage of the helper's truth table.
📋 Suggested test cases
+ [Test] + public void ShouldShowSkillsTargetList_WhenFirstInstallAndCliMissing_ReturnsFalse() + { + // Verifies that first-time setup hides the multi-target list even when CLI is missing. + bool shouldShow = SetupWizardWindow.ShouldShowSkillsTargetListForSetupWizard( + canManageSkills: false, + shouldUseFirstInstallSkillsUi: true); + + Assert.That(shouldShow, Is.False); + } + + [Test] + public void ShouldShowSkillsTargetList_WhenFirstInstallAndCliInstalled_ReturnsFalse() + { + // Verifies that first-time setup uses the compact target row instead of the multi-target list. + bool shouldShow = SetupWizardWindow.ShouldShowSkillsTargetListForSetupWizard( + canManageSkills: true, + shouldUseFirstInstallSkillsUi: true); + + Assert.That(shouldShow, Is.False); + }🤖 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 `@Assets/Tests/Editor/SetupWizardWindowTests.cs` around lines 338 - 358, Add two unit tests for SetupWizardWindow.ShouldShowSkillsTargetListForSetupWizard covering the missing cases where shouldUseFirstInstallSkillsUi is true: one with canManageSkills = false and one with canManageSkills = true; both should assert the result is false. Place them alongside the existing tests in SetupWizardWindowTests (e.g., names like ShouldHideSkillsTargetList_WhenFirstInstallAndCliMissing_ReturnsFalse and ShouldHideSkillsTargetList_WhenFirstInstallAndCliInstalled_ReturnsFalse) to complete the truth-table coverage for that helper.
🤖 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.
Nitpick comments:
In `@Assets/Tests/Editor/SetupWizardWindowTests.cs`:
- Around line 338-358: Add two unit tests for
SetupWizardWindow.ShouldShowSkillsTargetListForSetupWizard covering the missing
cases where shouldUseFirstInstallSkillsUi is true: one with canManageSkills =
false and one with canManageSkills = true; both should assert the result is
false. Place them alongside the existing tests in SetupWizardWindowTests (e.g.,
names like ShouldHideSkillsTargetList_WhenFirstInstallAndCliMissing_ReturnsFalse
and ShouldHideSkillsTargetList_WhenFirstInstallAndCliInstalled_ReturnsFalse) to
complete the truth-table coverage for that helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc20aa34-c0b1-445d-80cb-b6c583226fca
📒 Files selected for processing (2)
Assets/Tests/Editor/SetupWizardWindowTests.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.cs
When users remove and reinstall the Unity package, the persisted setup wizard version could prevent first-install setup from appearing again. Subscribe to package removal events and clear only wizard auto-show state while keeping other editor preferences intact.
The shell CLI detection path could kill a process after it had already exited during cancellation or timeout handling. Reuse a guarded process kill helper so only the already-exited race is ignored while preserving the detection flow.
Contributor
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 `@Assets/Tests/Editor/CliInstallationDetectorTests.cs`:
- Around line 160-164: The test in CliInstallationDetectorTests constructs a
ProcessStartInfo that hardcodes "/bin/sh", which breaks on Windows; update the
test to select the command by platform (use
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) to detect Windows) and set
ProcessStartInfo.FileName to "cmd.exe" with Arguments "/c exit 0" on Windows,
otherwise use "/bin/sh" with Arguments "-c \"exit 0\""; keep UseShellExecute =
false and other existing settings so the race test behavior is unchanged.
🪄 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: e3e978cb-8f16-43fc-b1ca-51aca8229dd8
⛔ Files ignored due to path filters (2)
Assets/Tests/Editor/UnityCliLoopPackageRemovalSettingsResetterTests.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/Settings/UnityCliLoopPackageRemovalSettingsResetter.cs.metais excluded by none and included by none
📒 Files selected for processing (6)
Assets/Tests/Editor/CliInstallationDetectorTests.csAssets/Tests/Editor/UnityCliLoopPackageRemovalSettingsResetterTests.csPackages/src/Editor/Infrastructure/CLI/CliInstallationDetector.csPackages/src/Editor/Infrastructure/InfrastructureEditorStartup.csPackages/src/Editor/Infrastructure/Settings/UnityCliLoopPackageRemovalSettingsResetter.csPackages/src/Editor/ToolContracts/UnityCliLoopConstants.cs
Record the shell-resolved uloop -v exit status before parsing stdout so a broken PATH command cannot be treated as an installed version. Also make the process cleanup race test choose a platform-specific immediate-exit command for Windows Editor runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User Impact
Changes
Verification
Summary
uloop -vwithin the same login shell environment.User impact
uloop-clibeing available in the user’s terminal (e.g., npm global shims and terminal node).Key changes
-vfor shell commands.Tests
-vusage, parsing shell output (path+version and version-only), and guarded process-kill race test.Verification
go run/test runs demonstrating scenario where SHELL=null returns Installed=True, Version=2.1.1, Path=/Users/a12115/.npm-global/bin/uloop (shell-based detection succeeds even when Unity's environment lacked SHELL).Files touched (high level)
Review notes / risk
-vflag is appropriate across supported CLI versions and platforms.