refactor: replace hardcoded Node.js paths with dynamic shell PATH res…#2
refactor: replace hardcoded Node.js paths with dynamic shell PATH res…#2ilizhu wants to merge 1 commit into
Conversation
…olution and version manager scanning
There was a problem hiding this comment.
Pull request overview
Refactors Node.js binary discovery in the macOS launcher to avoid hardcoded installation paths by resolving the user’s PATH via a login shell and scanning common version-manager install directories.
Changes:
- Replaced hardcoded
/usr/local/bin/node,/usr/bin/node,/opt/homebrew/bin/nodechecks with login-shell PATH resolution. - Added fallback scanning for Node binaries installed by popular version managers (nvm, volta, fnm, mise, asdf, nodenv, n).
- Added helper to extract Node major version for selecting a “best” candidate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let p = Process() | ||
| p.executableURL = URL(fileURLWithPath: "/usr/bin/which") | ||
| p.arguments = ["node"] | ||
| p.executableURL = URL(fileURLWithPath: shell) | ||
| // Use login + interactive-like mode so .zprofile/.bash_profile etc. are sourced | ||
| p.arguments = ["-l", "-c", "echo $PATH"] | ||
| let pipe = Pipe() | ||
| p.standardOutput = pipe | ||
| try? p.run() | ||
| p.waitUntilExit() | ||
| if let data = try? pipe.fileHandleForReading.readToEnd(), | ||
| let path = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !path.isEmpty, FileManager.default.isExecutableFile(atPath: path), | ||
| isNodeVersionCompatible(at: path) { | ||
| return path | ||
| p.standardError = Pipe() | ||
|
|
||
| do { | ||
| try p.run() | ||
| p.waitUntilExit() | ||
| } catch { | ||
| appendLog("Failed to resolve PATH from shell: \(error.localizedDescription)\n") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
findNodeFromShellPATH() runs a login shell and then calls waitUntilExit() synchronously. Since start() is invoked from UI code (e.g., AppDelegate/SwiftUI), a slow or hanging shell startup script can freeze the app indefinitely. Consider adding a timeout and/or resolving PATH on a background queue, falling back if the shell process doesn’t exit quickly.
| let shell = ProcessInfo.processInfo.environment["SHELL"] ?? "/bin/zsh" | ||
|
|
||
| let p = Process() | ||
| p.executableURL = URL(fileURLWithPath: "/usr/bin/which") | ||
| p.arguments = ["node"] | ||
| p.executableURL = URL(fileURLWithPath: shell) | ||
| // Use login + interactive-like mode so .zprofile/.bash_profile etc. are sourced | ||
| p.arguments = ["-l", "-c", "echo $PATH"] |
There was a problem hiding this comment.
This executes whatever binary is in the SHELL environment variable. A caller can launch the app with a crafted SHELL value and cause unintended code execution. Safer options: ignore SHELL and use a fixed allowlist (e.g., /bin/zsh, /bin/bash, /bin/sh), and/or verify the path exists and is one of the expected shells before executing it.
| // Use login + interactive-like mode so .zprofile/.bash_profile etc. are sourced | ||
| p.arguments = ["-l", "-c", "echo $PATH"] | ||
| let pipe = Pipe() | ||
| p.standardOutput = pipe | ||
| try? p.run() | ||
| p.waitUntilExit() | ||
| if let data = try? pipe.fileHandleForReading.readToEnd(), | ||
| let path = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !path.isEmpty, FileManager.default.isExecutableFile(atPath: path), | ||
| isNodeVersionCompatible(at: path) { | ||
| return path | ||
| p.standardError = Pipe() | ||
|
|
||
| do { | ||
| try p.run() | ||
| p.waitUntilExit() | ||
| } catch { | ||
| appendLog("Failed to resolve PATH from shell: \(error.localizedDescription)\n") | ||
| return nil | ||
| } | ||
|
|
||
| guard p.terminationStatus == 0, | ||
| let data = try? pipe.fileHandleForReading.readToEnd(), | ||
| let pathString = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !pathString.isEmpty else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The command echo $PATH assumes stdout contains only the PATH string. Login shell startup files can legally print to stdout/stderr, which would corrupt pathString and make node resolution unreliable. Consider emitting a unique sentinel or using a command that returns only the node path (e.g., command -v node) to avoid parsing PATH from potentially noisy shell output.
| var bestPath: String? | ||
| var bestMajor = 0 | ||
|
|
||
| for (dir, globPattern) in candidates { | ||
| guard fm.fileExists(atPath: dir) else { continue } | ||
|
|
||
| let parts = globPattern.split(separator: "/", maxSplits: 1) | ||
| if parts.first == "*" { | ||
| // Enumerate subdirectories, pick the highest compatible version | ||
| guard let subdirs = try? fm.contentsOfDirectory(atPath: dir) else { continue } | ||
| let suffix = parts.count > 1 ? "/\(parts[1])" : "" | ||
| for sub in subdirs.sorted().reversed() { | ||
| let candidate = "\(dir)/\(sub)\(suffix)" | ||
| if fm.isExecutableFile(atPath: candidate), | ||
| let major = nodeMainVersion(at: candidate), major >= 18, major > bestMajor { | ||
| bestPath = candidate | ||
| bestMajor = major | ||
| } | ||
| } |
There was a problem hiding this comment.
The version-manager scan claims it selects the “highest compatible version”, but the selection only compares major and ignores minor/patch (and the directory sort is lexicographic). With multiple installs in the same major (e.g., 20.9.0 vs 20.10.0), this can pick an older patch. Consider parsing full semver (major/minor/patch) and comparing tuples, or preferring the manager’s “current/default” symlink when present.
…olution and version manager scanning