fix: Unity CLI install repairs missing terminal command setup#1176
Merged
Conversation
Document the rebuild goal, scope, and verification gates before reimplementing the installer and Unity UI flow from a clean v3-beta branch.
Keep the POSIX shell installer informational while moving profile writes behind the Unity UI action. Use fresh login-shell command detection as the authority so a running Unity session does not trust stale PATH state. - Add shell-specific PATH setup plans and newline-safe profile writes for zsh, bash, and fish - Refresh the settings and setup wizard primary action so missing terminal visibility shows Fix PATH - Cover installer guidance, profile detection, duplicate prevention, and repair flow with focused tests
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds end-to-end CLI PATH visibility detection and repair: data contracts, shell-aware detection, per-shell plan resolution, safe profile writing, native-installer integration, orchestration service and facade, UI wiring (Setup Wizard/Settings), user prompts, shell-install guidance scripts, and expanded tests/fakes. ChangesCLI PATH Visibility & Repair Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 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: 2
🧹 Nitpick comments (1)
scripts/install.sh (1)
122-129: ⚡ Quick winEnsure the printed append command creates the profile directory first.
The suggested
printf ... >> profilecommand can fail when the profile parent directory does not exist (notably possible withZDOTDIR-based zsh setups).Proposed patch
print_append_command() { append_line=$1 profile_path=$2 + profile_dir=${profile_path%/*} quoted_line=$(quote_for_single_quoted_shell "$append_line") quoted_profile=$(quote_for_single_quoted_shell "$profile_path") + quoted_profile_dir=$(quote_for_single_quoted_shell "$profile_dir") - printf "%s\n" " printf '\n%s\n' '$quoted_line' >> '$quoted_profile'" + printf "%s\n" " mkdir -p '$quoted_profile_dir' && printf '\n%s\n' '$quoted_line' >> '$quoted_profile'" }🤖 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.sh` around lines 122 - 129, The printed append command in print_append_command must ensure the profile's parent directory exists before attempting to append; modify print_append_command to emit a command that first creates the profile directory (using mkdir -p on the dirname of profile_path) and only then runs the printf '... >> profile' append, combining them with a short-circuit operator so the append runs only after successful directory creation; make sure to reuse the already-quoted_profile value and preserve the single-quote-safe quoting produced by quote_for_single_quoted_shell when composing the emitted mkdir and printf sequence.
🤖 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 `@docs/architecture/cli-path-setup-rebuild-plan.md`:
- Line 11: Replace the user-specific absolute path
"/Users/masamichi/work/unity-cli-loop2" in the sentence that references the
example implementation with a portable cwd-based or placeholder path (e.g.,
"cwd/unity-cli-loop2" or "<your-repo-path>") and keep the branch name
"codex/fix-v3-cli-path-setup-ui" unchanged; update any verification commands or
examples in the same sentence to use "cwd" or a placeholder so the doc is
reusable and does not expose local environment details.
In `@Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs`:
- Around line 238-248: BuildShellCliDetectionCommandForPlan currently chooses
Fish vs POSIX syntax based on pathSetupPlan.ShellKind, but ShellKind can be
Unsupported; instead accept and use the actual shell value supplied by
BuildShellCliDetectionStartInfo to pick the correct syntax. Update
BuildShellCliDetectionCommandForPlan to take the real shell (or obtain it from
the StartInfo parameter passed in), then branch on that runtime shell to call
BuildFishShellCliDetectionCommand(executableName) or
BuildShellCliDetectionCommand(executableName) so fish never receives POSIX-style
checks when the running shell is fish.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 122-129: The printed append command in print_append_command must
ensure the profile's parent directory exists before attempting to append; modify
print_append_command to emit a command that first creates the profile directory
(using mkdir -p on the dirname of profile_path) and only then runs the printf
'... >> profile' append, combining them with a short-circuit operator so the
append runs only after successful directory creation; make sure to reuse the
already-quoted_profile value and preserve the single-quote-safe quoting produced
by quote_for_single_quoted_shell when composing the emitted mkdir and printf
sequence.
🪄 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: 4322f5a4-a96c-4128-a6b2-9ee15f2d8916
⛔ Files ignored due to path filters (7)
Assets/Tests/Editor/CliPathSetupFlowTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/CliPathSetupProfileResolverTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/CliPathSetupWriterTests.cs.metais excluded by none and included by nonePackages/src/Editor/Application/CliPathSetupPlan.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/CLI/CliPathSetupWriter.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/CliPathSetupPrompt.cs.metais excluded by none and included by none
📒 Files selected for processing (22)
Assets/Tests/Editor/CliPathSetupFlowTests.csAssets/Tests/Editor/CliPathSetupProfileResolverTests.csAssets/Tests/Editor/CliPathSetupWriterTests.csAssets/Tests/Editor/CliSetupApplicationServiceTests.csAssets/Tests/Editor/CliSetupSectionTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/SkillsTargetSelectionResolverTests.csAssets/Tests/Editor/UnityCliLoopSettingsWindowCliActionTests.csPackages/src/Editor/Application/CliPathSetupPlan.csPackages/src/Editor/Application/CliSetupApplicationService.csPackages/src/Editor/Infrastructure/CLI/CliInstallationDetector.csPackages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.csPackages/src/Editor/Infrastructure/CLI/CliPathSetupWriter.csPackages/src/Editor/Infrastructure/CLI/NativeCliInstaller.csPackages/src/Editor/Presentation/CliPathSetupPrompt.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindowViewData.csdocs/architecture/cli-path-setup-rebuild-plan.mdscripts/install.shscripts/test-install-release-filter.sh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 29 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Keep the clicked Settings action stable across async state refreshes so a stale install click cannot turn into an uninstall action. Tighten PATH profile handling so Unity only rewrites shell configuration when the terminal cannot resolve uloop, fish guidance moves ~/.local/bin to the front, and duplicate detection avoids both stale PATH entries and unrelated trailing lines.
hatayama
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Packages/src/Editor/Infrastructure/CLI/CliPathSetupWriter.cs (1)
96-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScan all PATH lines for an exact managed-line match before falling back to the last-line heuristic.
Right now an exact match only counts if it is also the last PATH-like line. A profile like:
export PATH="$HOME/.local/bin:$PATH" export PATH="$PATH:/opt/homebrew/bin"will be treated as not configured, so every repair run appends a duplicate managed line.
💡 Proposed fix
internal static bool ContainsExistingPathSetup(string content, CliPathSetupPlan plan) { if (string.IsNullOrEmpty(content)) { return false; } string[] references = BuildReferenceCandidates(plan); string[] lines = content.Replace("\r\n", "\n").Split('\n'); string lastPathSetupLine = null; foreach (string line in lines) { string trimmedLine = line.Trim(); if (string.IsNullOrEmpty(trimmedLine) || trimmedLine.StartsWith("#", StringComparison.Ordinal)) { continue; } + if (string.Equals(trimmedLine, plan.ConfigurationLine, StringComparison.Ordinal)) + { + return true; + } + if (!LooksLikePathSetupLine(trimmedLine, plan.ShellKind)) { continue; } lastPathSetupLine = trimmedLine; } return lastPathSetupLine != null && ContainsPrependingReference(lastPathSetupLine, references, plan.ShellKind); }🤖 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/CLI/CliPathSetupWriter.cs` around lines 96 - 118, The loop that currently records only the last PATH-like line should instead check every PATH-like line for an exact managed-line match or for a prepending reference; inside the foreach over lines (using LooksLikePathSetupLine) add an immediate check that if trimmedLine exactly equals plan.ConfigurationLine (StringComparison.Ordinal) or if ContainsPrependingReference(trimmedLine, references, plan.ShellKind) returns true then return true immediately, and only fall back to the existing lastPathSetupLine comparison if no earlier match was found; keep BuildReferenceCandidates(plan) and the existing shell-kind checks unchanged.Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs (1)
690-735:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the wizard's installed-state model with the new
Fix PATHbranch.These helpers now allow
needsCliPathSetup == trueeven whencliInstalled == false—the new tests cover that state. The rest of the wizard still derives installation state fromcliVersion, so the UI can renderNot installedwhile offeringFix PATH, and the repair flow can temporarily show a success state with an emptyvlabel when the cached version is null. Please normalize hidden-but-installed CLIs to a single state before driving the button text/enabled logic.🤖 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/Presentation/Setup/SetupWizardWindow.cs` around lines 690 - 735, Normalize a single "hidden-but-installed" state before deciding button text/enabled logic: in GetCliButtonTextForSetupWizard and IsCliButtonEnabledForSetupWizard derive an effective installed flag (e.g., effectiveCliInstalled = cliInstalled || needsCliPathSetup) and use that instead of raw cliInstalled (also treat cliVersion/cliVersionMatched consistently against that effective flag) so the Fix PATH branch and the Install/Installed/Update logic behave from the same normalized state.Packages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.cs (1)
155-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't default bash PATH repair to
~/.bash_profilewhen no existing profile is found.When none of the existing login profiles exist, this code falls back to
~/.bash_profile. However, interactive non-login bash shells (the typical default for terminal emulators) read only~/.bashrc, not~/.bash_profile. This means the repair can succeed on disk but still leave the PATH unavailable in a fresh terminal session.Consider falling back to
~/.bashrcinstead, or verify that this behavior aligns with the tool's expectations for shell configuration across all usage scenarios.🤖 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/CLI/CliPathSetupProfileResolver.cs` around lines 155 - 170, The SelectBashProfilePath function currently falls back to ~/.bash_profile when no profile exists; change the fallback to ~/.bashrc to match interactive non-login shells: after checking BashProfileFileName, BashLoginFileName and PosixProfileFileName, return Path.Combine(homeDirectory, BashRcFileName) (or the explicit "~/.bashrc" path) instead of bashProfilePath, and update any constant/reference from PosixProfileFileName fallback logic if needed so the final returned path is the bash rc file when no existing profile is found.
🤖 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.
Outside diff comments:
In `@Packages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.cs`:
- Around line 155-170: The SelectBashProfilePath function currently falls back
to ~/.bash_profile when no profile exists; change the fallback to ~/.bashrc to
match interactive non-login shells: after checking BashProfileFileName,
BashLoginFileName and PosixProfileFileName, return Path.Combine(homeDirectory,
BashRcFileName) (or the explicit "~/.bashrc" path) instead of bashProfilePath,
and update any constant/reference from PosixProfileFileName fallback logic if
needed so the final returned path is the bash rc file when no existing profile
is found.
In `@Packages/src/Editor/Infrastructure/CLI/CliPathSetupWriter.cs`:
- Around line 96-118: The loop that currently records only the last PATH-like
line should instead check every PATH-like line for an exact managed-line match
or for a prepending reference; inside the foreach over lines (using
LooksLikePathSetupLine) add an immediate check that if trimmedLine exactly
equals plan.ConfigurationLine (StringComparison.Ordinal) or if
ContainsPrependingReference(trimmedLine, references, plan.ShellKind) returns
true then return true immediately, and only fall back to the existing
lastPathSetupLine comparison if no earlier match was found; keep
BuildReferenceCandidates(plan) and the existing shell-kind checks unchanged.
In `@Packages/src/Editor/Presentation/Setup/SetupWizardWindow.cs`:
- Around line 690-735: Normalize a single "hidden-but-installed" state before
deciding button text/enabled logic: in GetCliButtonTextForSetupWizard and
IsCliButtonEnabledForSetupWizard derive an effective installed flag (e.g.,
effectiveCliInstalled = cliInstalled || needsCliPathSetup) and use that instead
of raw cliInstalled (also treat cliVersion/cliVersionMatched consistently
against that effective flag) so the Fix PATH branch and the
Install/Installed/Update logic behave from the same normalized state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34f22860-c031-4717-a748-e5db0a49c471
⛔ Files ignored due to path filters (7)
Assets/Tests/Editor/CliPathSetupFlowTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/CliPathSetupProfileResolverTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/CliPathSetupWriterTests.cs.metais excluded by none and included by nonePackages/src/Editor/Application/CliPathSetupPlan.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/CLI/CliPathSetupWriter.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/CliPathSetupPrompt.cs.metais excluded by none and included by none
📒 Files selected for processing (16)
Assets/Tests/Editor/CliPathSetupFlowTests.csAssets/Tests/Editor/CliPathSetupProfileResolverTests.csAssets/Tests/Editor/CliPathSetupWriterTests.csAssets/Tests/Editor/CliSetupSectionTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/UnityCliLoopSettingsWindowCliActionTests.csPackages/src/Editor/Application/CliPathSetupPlan.csPackages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.csPackages/src/Editor/Infrastructure/CLI/CliPathSetupWriter.csPackages/src/Editor/Presentation/CliPathSetupPrompt.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/UIToolkit/Components/CliSetupSection.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.csdocs/architecture/cli-path-setup-rebuild-plan.mdscripts/install.shscripts/test-install-release-filter.sh
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/cli-path-setup-rebuild-plan.md
There was a problem hiding this comment.
2 issues found across 23 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Resolve review feedback that could make terminal visibility checks use the wrong shell syntax, produce misleading manual PATH guidance when the install directory is unavailable, or leave the Setup Wizard CLI action stuck in a checking state. Remove the temporary rebuild plan document now that the implementation is represented by code and tests.
Prevent the Settings primary button from executing a stale destructive action after refreshing CLI and PATH state. Return failed PATH setup results for invalid or unsupported profile paths so UI repair flows can surface the error instead of throwing.
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
uloopcommand is still missing from a fresh terminal and surfaces a direct Fix PATH action in Setup and Settings.~/.local/binand prints shell-specific PATH guidance without editing shell profiles automatically.User Impact
uloopbecause~/.local/binwas not available in the current shell configuration.uloop, and keeps already working PATH setups untouched.Changes
~/.local/binto the front of PATH and tightened stale PATH entry detection for existing profiles.Verification
sh -n scripts/install.shscripts/test-install-release-filter.shPackages/src/Cli~/dist/darwin-arm64/uloop compile --project-path /Users/masamichi/work/unity-cli-loopPackages/src/Cli~/dist/darwin-arm64/uloop run-tests --project-path /Users/masamichi/work/unity-cli-loop --test-mode EditMode --filter-type regex --filter-value 'CliPathSetupWriterTests|CliPathSetupFlowTests|CliPathSetupProfileResolverTests|CliSetupSectionTests|SetupWizardWindowTests|UnityCliLoopSettingsWindowCliActionTests'codex-review v3-betawith the verification commands above