macOS: Auto-install CLI to ~/.local/bin - #2937
Conversation
Replace the sudo-based /usr/local/bin symlink approach with a user-writable ~/.local/bin directory. Auto-install CLI on app startup and manage shell profile PATH if needed.
Add cliAutoInstalled flag to app config so auto-install only runs on first launch. If the user disables the CLI toggle, it won't be re-enabled on the next app start.
Use path.join() for test constants and assertions so paths use the correct separator on Windows CI (backslashes) vs macOS (forward slashes).
Use describe.skipIf for the MacOSCliInstallationManager tests since they use macOS path conventions. Matches the existing codebase pattern of using hardcoded forward-slash paths in test mocks.
The old /usr/local/bin/studio symlink almost always requires sudo to remove, making cleanup a no-op for most users. The old symlink is harmless since ~/.local/bin takes priority in PATH.
Prevents dev mode from creating a symlink pointing to dev resources and setting the cliAutoInstalled flag, which would interfere with production installs.
|
|
||
| const ERROR_FILE_ALREADY_EXISTS = 'Studio CLI symlink path already occupied by non-symlink'; | ||
| // Defined in @vscode/sudo-prompt | ||
| const ERROR_PERMISSION = 'User did not grant permission.'; |
There was a problem hiding this comment.
Since we don't run sudo, the error message is no longer needed.
There was a problem hiding this comment.
Pull request overview
Refactors the macOS Studio CLI installation flow to avoid sudo by installing the studio symlink into ~/.local/bin, adds first-run auto-install behavior, and updates user config/types to track whether auto-install has occurred.
Changes:
- Switch macOS CLI symlink target from
/usr/local/bin/studioto~/.local/bin/studioand remove the legacy sudo-based shell scripts. - Add
autoInstallMacOSCliIfNeeded()on app boot (production + darwin only) and persistcliAutoInstalledin app config. - Add PATH management by appending
export PATH="$HOME/.local/bin:$PATH"to a detected shell profile when needed, plus new unit tests.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/studio/src/storage/user-data.ts | Allows cliAutoInstalled to be persisted via updateAppdata() safe keys. |
| apps/studio/src/storage/storage-types.ts | Adds cliAutoInstalled?: boolean to the UserData type. |
| apps/studio/src/modules/cli/lib/macos-installation-manager.ts | Implements ~/.local/bin symlink install, profile PATH updates, and auto-install entrypoint. |
| apps/studio/src/index.ts | Calls autoInstallMacOSCliIfNeeded() during app boot. |
| apps/studio/src/modules/cli/lib/tests/macos-installation-manager.test.ts | Adds unit tests for the refactored macOS installation manager and auto-install behavior. |
| apps/studio/bin/install-studio-cli.sh | Removed legacy sudo install script. |
| apps/studio/bin/uninstall-studio-cli.sh | Removed legacy sudo uninstall script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Return early in uninstallCli() when the symlink doesn't exist instead of proceeding to unlink (which would throw ENOENT). Also narrow the test skip from all non-darwin platforms to Windows only, since the POSIX paths work fine on Linux.
📊 Performance Test ResultsComparing fb6f9dc vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
I left a couple of comments on the implementation. We also need to consider users updating from older Studio versions. We have two options there, as I see it:
- Write a data migration that respects the old CLI installation setting, and only runs the auto-installer logic if the CLI was already installed. The downside is that this is probably not a conscious decision for a lot of users.
- Run the auto-installer logic for all users: upgraders and new installers alike. The downside is that this will modify the environment for users who previously consciously uninstalled the CLI.
I'm leaning towards the second option.
As for the old CLI path, it's unfortunately not so easy to clean up, since we need elevated privileges to modify it. We could either leave it lying around, or add a function to the CLI un/-install flow that checks the old path and cleans it up if needed.
| if ( existingContent.includes( '$HOME/.local/bin' ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
It makes sense not to append the same content repeatedly to the file, but at the same time, we also know that it doesn't have the intended effect at this point. Is there a fallback strategy we can use in this case..? 🤔
There was a problem hiding this comment.
Do you think a case like this one where the path would be overwritten?
export PATH="$HOME/.local/bin:$PATH"
# ... bunch of other stuff ...
export PATH="/usr/bin:/bin"
I think it would be quite an edge case, no? But maybe you mean some different case? 🤔
|
Thank you for your review and feedback, Fredrik!
Yes, I see it the same way and so we can keep the logic as-is. 🙂
Yes, I was actually trying to clean it up before, but then decided to remove the whole logic in 20e9dae - as it would run for a very few users only + keeping the old path there should not cause any issues. I will check out your other review comments as well. |
installCli() already checks isCliInstalled() internally, so the check in autoInstallIfNeeded() was redundant.
isCliInstalled now also verifies that ~/.local/bin is in PATH, not just that the symlink exists. PATH entries are normalized by expanding ~ and $HOME and resolving to absolute paths before comparison.
Instead of checking which profile files exist on disk, map the user's SHELL environment variable to the corresponding profile file. Defaults to .zshrc when SHELL is empty or unrecognized.
I have applied the changes and will retest everything tomorrow with fresh. 🧠 🙂 |
Dock-launched Electron apps on macOS don't inherit shell PATH, so checking process.env.PATH always returned false. Instead, read the shell profile file to verify the export line is present. Refactor shared helpers to avoid duplicate file reads and logic.
|
I ended up checking the shell profile file content instead of There's also a this package that helps with that, but I think checking the file contents itself should be enough. |
Use mockResolvedValue instead of chaining mockResolvedValueOnce twice with the same content. Removes confusing comments about call order.
|
Note: Failed E2E tests are unrelated to this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
apps/studio/src/modules/cli/lib/macos-installation-manager.ts:170
installCli()treats any existing symlink at~/.local/bin/studioas safe to replace: it only errors when the path is a non-symlink, then later unlinks and recreates the symlink ifisCliInstalled()is false. This can silently overwrite a user’s pre-existingstudiosymlink that points elsewhere. Consider validating the current symlink destination withreadlink()and (a) no-op if it already points tocliPackagedPath, or (b) surface a clear “path is occupied” error (or ask for confirmation) if it points somewhere else, rather than overwriting.
try {
const stats = await lstat( cliSymlinkPath );
if ( ! stats.isSymbolicLink() ) {
throw new Error( ERROR_FILE_ALREADY_EXISTS );
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Prevents silently overwriting the user's shell profile if readFile fails for a reason other than the file not existing (e.g. permissions or I/O errors).
|
Hey @fredrikekelund 👋🏼🙂 The PR is now ready for another review round. I have tested the updated logic on the built macOS app and did not observe any issues. I think we will probably won't get to merge today so considering my coming limited availability due to WordCamp I have also added the |
Check isCliInstalled() after installCli() before setting the flag. Consistent with the same change on the Windows PR.
Unlike Windows, macOS installCli() throws on errors rather than swallowing them, so the post-install verification is unnecessary and adds test complexity.
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
I made a bunch of additional changes here, informed by my own review and local AI agent reviews.
- Removed the tests.
MacOSCliInstallationManageris not well-suited for testing, IMO. We simply have to mock too much for the tests to really give us any real assurances. - I brought back the old uninstallation script and added an
uninstallLegacyCliIfNeededmethod. Without this,/usr/local/bin/studiowould be left intact, meaning thestudiocommand would not truly be uninstalled. - Started using
os.userInfo()to look up the user's shell. This is a more reliable indicator thanprocess.env.SHELL. We still fall back to the environment variable, though. - Started looking up shells by basenames rather than complete paths. This helps if the user has installed their current shell with Homebrew, for example.
- Removed the "CLI Installed" modal. This is subjective on my part, but I don't think this really makes sense when we're no longer prompting the user for their password. We should only interrupt them with a modal if there was an error.
|
Thank you for the updates to the PR and pushing it to the finish line while I was AFK, Fredrik! |
Related issues
How AI was used in this PR
Claude Code was used to implement the changes, write tests, and create this PR. The solution was developed iteratively through discussion — we explored the existing codebase patterns (macOS and Windows installation managers), identified edge cases (e.g., respecting user preference when CLI is explicitly disabled, Dock-launched apps not inheriting shell PATH), and refined the approach based on the Linear issue description and comments.
Proposed Changes
/usr/local/bin/studiosymlink (requires sudo) with~/.local/bin/studio(user-writable, no sudo needed)autoInstallMacOSCliIfNeeded(), with acliAutoInstalledflag in app config to avoid re-enabling if the user explicitly disabled itexport PATH="$HOME/.local/bin:$PATH"to the user's shell profile if the export line isn't already present. The profile file is determined byprocess.env.SHELL(.zshrcfor zsh,.bash_profilefor bash, defaults to.zshrc).~/.local/binis prepended so the bundled CLI takes priority over any standalone npm-installed versionisCliInstalled) checks the profile file content rather than runtimeprocess.env.PATH, since Dock-launched Electron apps on macOS don't inherit shell PATHinstall-studio-cli.sh,uninstall-studio-cli.sh) that are no longer needednpm start) to avoid creating symlinks pointing to dev resourcesTesting Instructions
Setup
rm -f ~/.local/bin/studio~/.studio/app.jsonand delete the"cliAutoInstalled": trueentrynpm run make, then install the resulting DMG to/Applications/Auto-install on first launch
ls -la ~/.local/bin/studio— should point to/Applications/Studio.app/Contents/Resources/bin/studio-cli.shgrep cliAutoInstalled ~/.studio/app.json— should show"cliAutoInstalled": truestudio --version— should print the current versiontail -3 ~/.zshrc— should containexport PATH="$HOME/.local/bin:$PATH"(if it wasn't already there)Shell profile PATH management
$HOME/.local/binfrom your shell profile (e.g., edit~/.zshrcand delete theexport PATH="$HOME/.local/bin:$PATH"line), then remove the CLI symlink and reset the flag:rm -f ~/.local/bin/studio~/.studio/app.jsonand delete the"cliAutoInstalled": trueentrygrep '$HOME/.local/bin' ~/.zshrc— should showexport PATH="$HOME/.local/bin:$PATH"grep -c '$HOME/.local/bin' ~/.zshrc— should show1User preference is respected
ls ~/.local/bin/studio— should not existls ~/.local/bin/studio— should still not existgrep cliAutoInstalled ~/.studio/app.json— should still showtrueRe-enabling works without sudo
ls -la ~/.local/bin/studioNo-op on subsequent launches
Pre-merge Checklist