Skip to content

chore: Simplify native CLI install path handling - #1620

Merged
hatayama merged 2 commits into
v3-betafrom
refactor/hatayama/extract-native-cli-path-resolver
Jul 8, 2026
Merged

chore: Simplify native CLI install path handling#1620
hatayama merged 2 commits into
v3-betafrom
refactor/hatayama/extract-native-cli-path-resolver

Conversation

@hatayama

@hatayama hatayama commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • Preserve native CLI setup behavior while separating install path resolution from process execution and orchestration.
  • Keep existing public installer and setup APIs unchanged.

User Impact

  • No user-visible behavior change.
  • Install, uninstall, PATH setup, and package-owned CLI detection continue to use the same paths, comparisons, and commands.

Changes

  • Move install path calculation, comparison, ownership checks, and environment reads into NativeCliInstallPathResolver.
  • Keep current-process PATH writes in NativeCliInstaller, maintaining a clear read/write boundary.
  • Move uninstall command construction into NativeCliCommandBuilder after path resolution becomes an independent dependency.
  • Update production callers and existing tests to reference the extracted responsibilities directly without compatibility wrappers.
  • Restore QuoteProcessArgument from internal to private visibility after all command construction becomes builder-internal; this is the only non-move production change.

Verification

  • Unity compile: 0 errors, 0 warnings
  • NativeCliInstallerTests: 32 passed
  • Related detector, PATH setup, application service, and static guard fixtures: 51 passed
  • No resolver-to-installer reverse references
  • No IPC or protocol changes

Review Follow-up

  • CodeRabbit and cubic identified a pre-existing raw PATH comparison that can leave a trailing-separator duplicate in the current-process PATH.
  • The finding is recorded as R2-29 [Medium] for a separate Red-Green TDD fix so this PR retains its move-only review guarantee; CLI resolution and the persisted User PATH are unaffected.

hatayama added 2 commits July 8, 2026 23:05
Move install path calculation, comparison, ownership checks, and environment reads into a focused resolver. Keep current-process PATH mutation in the installer so value resolution remains separate from state changes.
Place uninstall command assembly beside install command construction now that path resolution has a dedicated dependency. Restore process argument quoting to private visibility because it is again builder-internal.
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR extracts CLI install-path computation logic from NativeCliInstaller into a new NativeCliInstallPathResolver class and adds a BuildUninstallCommand helper to NativeCliCommandBuilder. Callers in NativeCliInstaller, CliInstallationDetector, CliPathSetupProfileResolver, and tests are updated to use these new entry points instead of prior NativeCliInstaller static methods.

Changes

Path resolver extraction and consumer migration

Layer / File(s) Summary
New NativeCliInstallPathResolver implementation
Packages/src/Editor/Infrastructure/CLI/NativeCliInstallPathResolver.cs
New static class computes global CLI install paths, builds/removes install directory from PATH, derives default install directories, checks package ownership, discovers current-user installs, and normalizes/compares paths per platform.
BuildUninstallCommand added to NativeCliCommandBuilder
Packages/src/Editor/Infrastructure/CLI/NativeCliCommandBuilder.cs
Adds BuildUninstallCommand using the resolver's global install path; changes QuoteProcessArgument visibility to private.
NativeCliInstaller refactor to delegate to resolver/builder
Packages/src/Editor/Infrastructure/CLI/NativeCliInstaller.cs
Install/uninstall flows, PATH mutation helpers, and ownership checks now delegate to NativeCliInstallPathResolver/NativeCliCommandBuilder; removes duplicated in-file path helpers and constants.
CliInstallationDetector and CliPathSetupProfileResolver migration
Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs, Packages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.cs
Switches calls for package ownership checks, install path lookup, and PATH building from NativeCliInstaller to NativeCliInstallPathResolver.
Test updates to target new resolver/builder entry points
Assets/Tests/Editor/NativeCliInstallerTests.cs
Updates test call sites for uninstall command construction and PATH/install-directory assertions to use NativeCliCommandBuilder/NativeCliInstallPathResolver; expected outputs unchanged.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NativeCliInstaller
  participant NativeCliInstallPathResolver
  participant NativeCliCommandBuilder
  NativeCliInstaller->>NativeCliInstallPathResolver: GetInstallDirectoryForCurrentUser(platform)
  NativeCliInstaller->>NativeCliCommandBuilder: BuildUninstallCommand(installDirectory, platform)
  NativeCliCommandBuilder->>NativeCliInstallPathResolver: GetGlobalCliInstallPath(installDirectory, platform)
  NativeCliInstaller->>NativeCliInstallPathResolver: DoesUserPathContainInstallDirectory(...)
Loading

Possibly related PRs

  • hatayama/unity-cli-loop#1154: Modifies the same uninstall flow logic in NativeCliInstaller and its tests that this PR refactors into resolver/builder helpers.
  • hatayama/unity-cli-loop#1169: Touches the same NativeCliInstallerTests.cs uninstall/PATH removal assertions that this PR migrates to the new resolver entry points.
  • hatayama/unity-cli-loop#1135: Introduced the uninstall command construction path that this PR relocates into NativeCliCommandBuilder/NativeCliInstallPathResolver.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the refactor of native CLI install path handling.
Description check ✅ Passed The description matches the refactor and behavior-preserving goals of the changeset.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/hatayama/extract-native-cli-path-resolver

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@Packages/src/Editor/Infrastructure/CLI/NativeCliInstallPathResolver.cs`:
- Around line 28-59: `BuildPathWithInstallDirectory` is comparing raw PATH
entries against `installDirectory`, so entries with trailing separators or other
path formatting differences can slip through as duplicates. Update the
comparison logic in `NativeCliInstallPathResolver.BuildPathWithInstallDirectory`
to normalize both sides for equality the same way
`BuildPathWithoutInstallDirectory` does (via `NormalizePathForComparison` or
equivalent), while still appending the original entry text to preserve output
formatting. Add or adjust a test covering a PATH entry with a trailing separator
so the duplicate install directory is not prepended.
🪄 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: 67f8e950-ae7b-44eb-b909-262281db4888

📥 Commits

Reviewing files that changed from the base of the PR and between 8f4b913 and 64db3a3.

⛔ Files ignored due to path filters (1)
  • Packages/src/Editor/Infrastructure/CLI/NativeCliInstallPathResolver.cs.meta is excluded by none and included by none
📒 Files selected for processing (6)
  • Assets/Tests/Editor/NativeCliInstallerTests.cs
  • Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs
  • Packages/src/Editor/Infrastructure/CLI/CliPathSetupProfileResolver.cs
  • Packages/src/Editor/Infrastructure/CLI/NativeCliCommandBuilder.cs
  • Packages/src/Editor/Infrastructure/CLI/NativeCliInstallPathResolver.cs
  • Packages/src/Editor/Infrastructure/CLI/NativeCliInstaller.cs

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@hatayama
hatayama merged commit cbd1674 into v3-beta Jul 8, 2026
10 checks passed
@hatayama
hatayama deleted the refactor/hatayama/extract-native-cli-path-resolver branch July 8, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant