Improve shell PATH hydration and fallback detection#1799
Improve shell PATH hydration and fallback detection#1799juliusmarminge wants to merge 1 commit intomainfrom
Conversation
- Probe multiple login shells and merge inherited PATH entries - Fall back to launchctl PATH on macOS when shell lookup fails - Preserve extra Homebrew and XDG env vars while adding coverage
github-actions
bot
added
size:L
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const launchctlPath = | ||
| platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined; | ||
| const mergedPath = mergePathEntries(shellEnvironment.PATH ?? launchctlPath, env.PATH, platform); |
There was a problem hiding this comment.
🟡 Medium src/syncShellEnvironment.ts:52
On Darwin, readPathFromLaunchctl() is invoked unconditionally even when shellEnvironment.PATH was already retrieved from the shell. If readPathFromLaunchctl() throws, the outer catch logs a generic warning and none of the successfully-read shell values (PATH, SSH_AUTH_SOCK, HOMEBREW_*, XDG_*) are applied to env. Since launchctlPath is only used as a fallback (shellEnvironment.PATH ?? launchctlPath), the call should be conditional: only invoke it when !shellEnvironment.PATH.
- const launchctlPath =
- platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined;
+ const launchctlPath =
+ platform === "darwin" && !shellEnvironment.PATH ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/desktop/src/syncShellEnvironment.ts around lines 52-54:
On Darwin, `readPathFromLaunchctl()` is invoked unconditionally even when `shellEnvironment.PATH` was already retrieved from the shell. If `readPathFromLaunchctl()` throws, the outer catch logs a generic warning and none of the successfully-read shell values (`PATH`, `SSH_AUTH_SOCK`, `HOMEBREW_*`, `XDG_*`) are applied to `env`. Since `launchctlPath` is only used as a fallback (`shellEnvironment.PATH ?? launchctlPath`), the call should be conditional: only invoke it when `!shellEnvironment.PATH`.
Evidence trail:
apps/desktop/src/syncShellEnvironment.ts at REVIEWED_COMMIT:
- Lines 51-52: unconditional call to `readPathFromLaunchctl()` on Darwin
- Lines 53-68: code that applies shell environment values to `env` (executed after the launchctl call)
- Lines 69-71: outer catch block that would catch any throw from line 52
- Line 53: `shellEnvironment.PATH ?? launchctlPath` showing launchctlPath is only a fallback
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Unnecessary synchronous subprocess spawned on macOS every time
- Deferred the readPathFromLaunchctl() call by adding a
!shellEnvironment.PATH/!shellPathguard so the synchronous subprocess is only spawned when the login shell did not provide a PATH.
- Deferred the readPathFromLaunchctl() call by adding a
- ✅ Fixed:
resolveLoginShellhas no production callers after refactor- Removed the dead
resolveLoginShellfunction from shell.ts and its corresponding import and test from shell.test.ts.
- Removed the dead
- ✅ Fixed: Outer catch discards error details unlike server counterpart
- Changed the bare
catch {in syncShellEnvironment.ts tocatch (error)and passed the error tologWarningto match the os-jank.ts pattern.
- Changed the bare
Or push these changes by commenting:
@cursor push 526ff40ceb
Preview (526ff40ceb)
diff --git a/apps/desktop/src/syncShellEnvironment.ts b/apps/desktop/src/syncShellEnvironment.ts
--- a/apps/desktop/src/syncShellEnvironment.ts
+++ b/apps/desktop/src/syncShellEnvironment.ts
@@ -50,7 +50,9 @@
}
const launchctlPath =
- platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined;
+ platform === "darwin" && !shellEnvironment.PATH
+ ? (options.readLaunchctlPath ?? readPathFromLaunchctl)()
+ : undefined;
const mergedPath = mergePathEntries(shellEnvironment.PATH ?? launchctlPath, env.PATH, platform);
if (mergedPath) {
env.PATH = mergedPath;
@@ -71,7 +73,7 @@
env[name] = shellEnvironment[name];
}
}
- } catch {
- logWarning("Failed to synchronize the desktop shell environment.");
+ } catch (error) {
+ logWarning("Failed to synchronize the desktop shell environment.", error);
}
}
diff --git a/apps/server/src/os-jank.ts b/apps/server/src/os-jank.ts
--- a/apps/server/src/os-jank.ts
+++ b/apps/server/src/os-jank.ts
@@ -43,7 +43,9 @@
}
const launchctlPath =
- platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined;
+ platform === "darwin" && !shellPath
+ ? (options.readLaunchctlPath ?? readPathFromLaunchctl)()
+ : undefined;
const mergedPath = mergePathEntries(shellPath ?? launchctlPath, env.PATH, platform);
if (mergedPath) {
env.PATH = mergedPath;
diff --git a/packages/shared/src/shell.test.ts b/packages/shared/src/shell.test.ts
--- a/packages/shared/src/shell.test.ts
+++ b/packages/shared/src/shell.test.ts
@@ -7,7 +7,6 @@
readEnvironmentFromLoginShell,
readPathFromLaunchctl,
readPathFromLoginShell,
- resolveLoginShell,
} from "./shell";
describe("extractPathFromShellOutput", () => {
@@ -176,14 +175,6 @@
});
});
-describe("resolveLoginShell", () => {
- it("returns the first available login shell candidate", () => {
- expect(resolveLoginShell("darwin", undefined, "/opt/homebrew/bin/fish")).toBe(
- "/opt/homebrew/bin/fish",
- );
- });
-});
-
describe("mergePathEntries", () => {
it("prefers login-shell PATH entries and keeps inherited extras", () => {
expect(
diff --git a/packages/shared/src/shell.ts b/packages/shared/src/shell.ts
--- a/packages/shared/src/shell.ts
+++ b/packages/shared/src/shell.ts
@@ -45,14 +45,6 @@
return candidates;
}
-export function resolveLoginShell(
- platform: NodeJS.Platform,
- shell: string | undefined,
- userShell = readUserLoginShell(),
-): string | undefined {
- return listLoginShellCandidates(platform, shell, userShell)[0];
-}
-
export function extractPathFromShellOutput(output: string): string | null {
const startIndex = output.indexOf(PATH_CAPTURE_START);
if (startIndex === -1) return null;You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 0d649e0. Configure here.
| if (shellEnvironment.PATH) { | ||
| env.PATH = shellEnvironment.PATH; | ||
| const launchctlPath = | ||
| platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined; |
There was a problem hiding this comment.
Unnecessary synchronous subprocess spawned on macOS every time
Medium Severity
readPathFromLaunchctl() is called unconditionally on macOS, spawning a synchronous subprocess via execFileSync even when the login shell already provided a PATH. The result is only used as a fallback (shellEnvironment.PATH ?? launchctlPath), but the child process is always spawned. This blocks the main thread unnecessarily on every startup in the common happy-path case.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0d649e0. Configure here.
| shell: string | undefined, | ||
| userShell = readUserLoginShell(), | ||
| ): string | undefined { | ||
| return listLoginShellCandidates(platform, shell, userShell)[0]; |
There was a problem hiding this comment.
resolveLoginShell has no production callers after refactor
Low Severity
resolveLoginShell is exported but no longer imported or called in any production code. Both previous callers (syncShellEnvironment.ts and os-jank.ts) now use listLoginShellCandidates directly. The function is only referenced in shell.test.ts, making it dead code in production.
Reviewed by Cursor Bugbot for commit 0d649e0. Configure here.
| } | ||
| } catch { | ||
| // Keep inherited environment if shell lookup fails. | ||
| logWarning("Failed to synchronize the desktop shell environment."); |
There was a problem hiding this comment.
Outer catch discards error details unlike server counterpart
Low Severity
The outer catch in syncShellEnvironment.ts uses a bare catch { that discards the error object, so logWarning is called without the error details. The equivalent block in os-jank.ts correctly uses catch (error) and passes the error through. If mergePathEntries, listLoginShellCandidates, or the Homebrew/XDG variable loop throws unexpectedly, the root cause would be lost from the log output.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0d649e0. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR changes runtime behavior for shell environment hydration, introducing multi-shell fallback probing, launchctl PATH retrieval on macOS, and PATH merging logic. Additionally, there are unresolved medium-severity review comments identifying a potential bug (launchctl called unconditionally may prevent env vars from being applied) and unnecessary subprocess spawning on the happy path. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Closes #1787
Summary
PATHby merging shell-provided entries with inherited entries instead of replacing them outright.launchctlfallback when shell probing does not yield aPATHvalue.launchctl, and PATH merge behavior with tests in shared, desktop, and server layers.Testing
bun fmtbun lintbun typecheckbun run testNote
Medium Risk
Changes how
PATHand related environment variables are derived/merged on macOS and Linux for both desktop and server, which could affect process execution if merging or fallback selection behaves unexpectedly.Overview
Improves environment hydration on macOS/Linux by probing a prioritized list of login-shell candidates (configured
SHELL, detected user shell, then platform default) instead of relying on a single resolved shell.PATHhydration now merges login-shell (or macOSlaunchctl) entries with the inheritedPATHto retain extra user-provided paths, and adds a macOS fallback tolaunchctl getenv PATHwhen shell probing fails/returns noPATH.Desktop shell sync also opportunistically hydrates additional variables (
HOMEBREW_*,XDG_*) when missing, and both desktop/server paths add warning hooks plus expanded test coverage for candidate selection, merging, and fallback behavior.Reviewed by Cursor Bugbot for commit 0d649e0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Merge login-shell and launchctl PATH instead of replacing it in shell environment hydration
syncShellEnvironment(desktop) andfixPath(server) now merge the login-shell PATH with the inherited PATH using the newmergePathEntriesutility, so inherited entries are preserved rather than discarded.console.warnrather than silently swallowed.readPathFromLaunchctlis used as a PATH fallback when no shell candidate returns one.HOMEBREW_PREFIX,HOMEBREW_CELLAR,HOMEBREW_REPOSITORY,XDG_CONFIG_HOME, andXDG_DATA_HOMEare now hydrated into the environment when absent.📊 Macroscope summarized 0d649e0. 6 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues