Skip to content

Improve shell PATH hydration and fallback detection #1799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
juliusmarminge wants to merge 1 commit into
base: main
Choose a base branch
from
+337 −37
Open

Improve shell PATH hydration and fallback detection #1799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 70 additions & 4 deletions apps/desktop/src/syncShellEnvironment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,31 @@ describe("syncShellEnvironment", () => {
it("hydrates PATH and missing SSH_AUTH_SOCK from the login shell on macOS", () => {
const env: NodeJS.ProcessEnv = {
SHELL: "/bin/zsh",
PATH: "/usr/bin",
PATH: "/Users/test/.local/bin:/usr/bin",
};
const readEnvironment = vi.fn(() => ({
PATH: "/opt/homebrew/bin:/usr/bin",
SSH_AUTH_SOCK: "/tmp/secretive.sock",
HOMEBREW_PREFIX: "/opt/homebrew",
}));

syncShellEnvironment(env, {
platform: "darwin",
readEnvironment,
});

expect(readEnvironment).toHaveBeenCalledWith("/bin/zsh", ["PATH", "SSH_AUTH_SOCK"]);
expect(env.PATH).toBe("/opt/homebrew/bin:/usr/bin");
expect(readEnvironment).toHaveBeenCalledWith("/bin/zsh", [
"PATH",
"SSH_AUTH_SOCK",
"HOMEBREW_PREFIX",
"HOMEBREW_CELLAR",
"HOMEBREW_REPOSITORY",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
]);
expect(env.PATH).toBe("/opt/homebrew/bin:/usr/bin:/Users/test/.local/bin");
expect(env.SSH_AUTH_SOCK).toBe("/tmp/secretive.sock");
expect(env.HOMEBREW_PREFIX).toBe("/opt/homebrew");
});

it("preserves an inherited SSH_AUTH_SOCK value", () => {
Expand Down Expand Up @@ -77,11 +87,67 @@ describe("syncShellEnvironment", () => {
readEnvironment,
});

expect(readEnvironment).toHaveBeenCalledWith("/bin/zsh", ["PATH", "SSH_AUTH_SOCK"]);
expect(readEnvironment).toHaveBeenCalledWith("/bin/zsh", [
"PATH",
"SSH_AUTH_SOCK",
"HOMEBREW_PREFIX",
"HOMEBREW_CELLAR",
"HOMEBREW_REPOSITORY",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
]);
expect(env.PATH).toBe("/home/linuxbrew/.linuxbrew/bin:/usr/bin");
expect(env.SSH_AUTH_SOCK).toBe("/tmp/secretive.sock");
});

it("falls back to launchctl PATH on macOS when shell probing does not return one", () => {
const env: NodeJS.ProcessEnv = {
SHELL: "/opt/homebrew/bin/nu",
PATH: "/usr/bin",
};
const readEnvironment = vi
.fn()
.mockImplementationOnce(() => {
throw new Error("unknown flag");
})
.mockImplementationOnce(() => ({}));
const readLaunchctlPath = vi.fn(() => "/opt/homebrew/bin:/usr/bin");
const logWarning = vi.fn();

syncShellEnvironment(env, {
platform: "darwin",
readEnvironment,
readLaunchctlPath,
userShell: "/bin/zsh",
logWarning,
});

expect(readEnvironment).toHaveBeenNthCalledWith(1, "/opt/homebrew/bin/nu", [
"PATH",
"SSH_AUTH_SOCK",
"HOMEBREW_PREFIX",
"HOMEBREW_CELLAR",
"HOMEBREW_REPOSITORY",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
]);
expect(readEnvironment).toHaveBeenNthCalledWith(2, "/bin/zsh", [
"PATH",
"SSH_AUTH_SOCK",
"HOMEBREW_PREFIX",
"HOMEBREW_CELLAR",
"HOMEBREW_REPOSITORY",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
]);
expect(readLaunchctlPath).toHaveBeenCalledTimes(1);
expect(logWarning).toHaveBeenCalledWith(
"Failed to read login shell environment from /opt/homebrew/bin/nu.",
expect.any(Error),
);
expect(env.PATH).toBe("/opt/homebrew/bin:/usr/bin");
});

it("does nothing outside macOS and linux", () => {
const env: NodeJS.ProcessEnv = {
SHELL: "C:/Program Files/Git/bin/bash.exe",
Expand Down
63 changes: 52 additions & 11 deletions apps/desktop/src/syncShellEnvironment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,77 @@
import {
listLoginShellCandidates,
mergePathEntries,
readPathFromLaunchctl,
readEnvironmentFromLoginShell,
resolveLoginShell,
ShellEnvironmentReader,
} from "@t3tools/shared/shell";

const LOGIN_SHELL_ENV_NAMES = [
"PATH",
"SSH_AUTH_SOCK",
"HOMEBREW_PREFIX",
"HOMEBREW_CELLAR",
"HOMEBREW_REPOSITORY",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
] as const;

function logShellEnvironmentWarning(message: string, error?: unknown): void {
console.warn(`[desktop] ${message}`, error instanceof Error ? error.message : (error ?? ""));
}

export function syncShellEnvironment(
env: NodeJS.ProcessEnv = process.env,
options: {
platform?: NodeJS.Platform;
readEnvironment?: ShellEnvironmentReader;
readLaunchctlPath?: typeof readPathFromLaunchctl;
userShell?: string;
logWarning?: (message: string, error?: unknown) => void;
} = {},
): void {
const platform = options.platform ?? process.platform;
if (platform !== "darwin" && platform !== "linux") return;

try {
const shell = resolveLoginShell(platform, env.SHELL);
if (!shell) return;
const logWarning = options.logWarning ?? logShellEnvironmentWarning;
const readEnvironment = options.readEnvironment ?? readEnvironmentFromLoginShell;
const shellEnvironment: Partial<Record<string, string>> = {};

const shellEnvironment = (options.readEnvironment ?? readEnvironmentFromLoginShell)(shell, [
"PATH",
"SSH_AUTH_SOCK",
]);
try {
for (const shell of listLoginShellCandidates(platform, env.SHELL, options.userShell)) {
try {
Object.assign(shellEnvironment, readEnvironment(shell, LOGIN_SHELL_ENV_NAMES));
if (shellEnvironment.PATH) {
break;
}
} catch (error) {
logWarning(`Failed to read login shell environment from ${shell}.`, error);
}
}

if (shellEnvironment.PATH) {
env.PATH = shellEnvironment.PATH;
const launchctlPath =
platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined;
Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 6, 2026

Choose a reason for hiding this comment

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

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0d649e0. Configure here.

const mergedPath = mergePathEntries(shellEnvironment.PATH ?? launchctlPath, env.PATH, platform);
Comment on lines +52 to +54
Copy link
Copy Markdown
Contributor

@macroscopeapp macroscopeapp bot Apr 6, 2026

Choose a reason for hiding this comment

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

🟡 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

if (mergedPath) {
env.PATH = mergedPath;
}

if (!env.SSH_AUTH_SOCK && shellEnvironment.SSH_AUTH_SOCK) {
env.SSH_AUTH_SOCK = shellEnvironment.SSH_AUTH_SOCK;
}

for (const name of [
"HOMEBREW_PREFIX",
"HOMEBREW_CELLAR",
"HOMEBREW_REPOSITORY",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
] as const) {
if (!env[name] && shellEnvironment[name]) {
env[name] = shellEnvironment[name];
}
}
} catch {
// Keep inherited environment if shell lookup fails.
logWarning("Failed to synchronize the desktop shell environment.");
Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 6, 2026

Choose a reason for hiding this comment

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

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0d649e0. Configure here.

}
}
35 changes: 34 additions & 1 deletion apps/server/src/os-jank.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe("fixPath", () => {
it("hydrates PATH on linux using the resolved login shell", () => {
const env: NodeJS.ProcessEnv = {
SHELL: "/bin/zsh",
PATH: "/usr/bin",
PATH: "/Users/test/.local/bin:/usr/bin",
};
const readPath = vi.fn(() => "/opt/homebrew/bin:/usr/bin");

Expand All @@ -17,6 +17,39 @@ describe("fixPath", () => {
});

expect(readPath).toHaveBeenCalledWith("/bin/zsh");
expect(env.PATH).toBe("/opt/homebrew/bin:/usr/bin:/Users/test/.local/bin");
});

it("falls back to launchctl PATH on macOS when shell probing fails", () => {
const env: NodeJS.ProcessEnv = {
SHELL: "/opt/homebrew/bin/nu",
PATH: "/usr/bin",
};
const readPath = vi
.fn()
.mockImplementationOnce(() => {
throw new Error("unknown flag");
})
.mockImplementationOnce(() => undefined);
const readLaunchctlPath = vi.fn(() => "/opt/homebrew/bin:/usr/bin");
const logWarning = vi.fn();

fixPath({
env,
platform: "darwin",
readPath,
readLaunchctlPath,
userShell: "/bin/zsh",
logWarning,
});

expect(readPath).toHaveBeenNthCalledWith(1, "/opt/homebrew/bin/nu");
expect(readPath).toHaveBeenNthCalledWith(2, "/bin/zsh");
expect(readLaunchctlPath).toHaveBeenCalledTimes(1);
expect(logWarning).toHaveBeenCalledWith(
"Failed to read PATH from login shell /opt/homebrew/bin/nu.",
expect.any(Error),
);
expect(env.PATH).toBe("/opt/homebrew/bin:/usr/bin");
});

Expand Down
43 changes: 35 additions & 8 deletions apps/server/src/os-jank.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,55 @@
import * as OS from "node:os";
import { Effect, Path } from "effect";
import { readPathFromLoginShell, resolveLoginShell } from "@t3tools/shared/shell";
import {
listLoginShellCandidates,
mergePathEntries,
readPathFromLaunchctl,
readPathFromLoginShell,
} from "@t3tools/shared/shell";

function logPathHydrationWarning(message: string, error?: unknown): void {
console.warn(`[server] ${message}`, error instanceof Error ? error.message : (error ?? ""));
}

export function fixPath(
options: {
env?: NodeJS.ProcessEnv;
platform?: NodeJS.Platform;
readPath?: typeof readPathFromLoginShell;
readLaunchctlPath?: typeof readPathFromLaunchctl;
userShell?: string;
logWarning?: (message: string, error?: unknown) => void;
} = {},
): void {
const platform = options.platform ?? process.platform;
if (platform !== "darwin" && platform !== "linux") return;

const env = options.env ?? process.env;
const logWarning = options.logWarning ?? logPathHydrationWarning;
const readPath = options.readPath ?? readPathFromLoginShell;

try {
const shell = resolveLoginShell(platform, env.SHELL);
if (!shell) return;
const result = (options.readPath ?? readPathFromLoginShell)(shell);
if (result) {
env.PATH = result;
let shellPath: string | undefined;
for (const shell of listLoginShellCandidates(platform, env.SHELL, options.userShell)) {
try {
shellPath = readPath(shell);
} catch (error) {
logWarning(`Failed to read PATH from login shell ${shell}.`, error);
}

if (shellPath) {
break;
}
}

const launchctlPath =
platform === "darwin" ? (options.readLaunchctlPath ?? readPathFromLaunchctl)() : undefined;
const mergedPath = mergePathEntries(shellPath ?? launchctlPath, env.PATH, platform);
if (mergedPath) {
env.PATH = mergedPath;
}
} catch {
// Silently ignore — keep default PATH
} catch (error) {
logWarning("Failed to hydrate PATH from the user environment.", error);
}
}

Expand Down
71 changes: 71 additions & 0 deletions packages/shared/src/shell.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ import { describe, expect, it, vi } from "vitest";

import {
extractPathFromShellOutput,
listLoginShellCandidates,
mergePathEntries,
readEnvironmentFromLoginShell,
readPathFromLaunchctl,
readPathFromLoginShell,
resolveLoginShell,
} from "./shell";

describe("extractPathFromShellOutput", () => {
Expand Down Expand Up @@ -60,6 +64,38 @@ describe("readPathFromLoginShell", () => {
});
});

describe("readPathFromLaunchctl", () => {
it("returns a trimmed PATH value from launchctl", () => {
const execFile = vi.fn<
(
file: string,
args: ReadonlyArray<string>,
options: { encoding: "utf8"; timeout: number },
) => string
>(() => " /opt/homebrew/bin:/usr/bin \n");

expect(readPathFromLaunchctl(execFile)).toBe("/opt/homebrew/bin:/usr/bin");
expect(execFile).toHaveBeenCalledWith("/bin/launchctl", ["getenv", "PATH"], {
encoding: "utf8",
timeout: 2000,
});
});

it("returns undefined when launchctl is unavailable", () => {
const execFile = vi.fn<
(
file: string,
args: ReadonlyArray<string>,
options: { encoding: "utf8"; timeout: number },
) => string
>(() => {
throw new Error("spawn /bin/launchctl ENOENT");
});

expect(readPathFromLaunchctl(execFile)).toBeUndefined();
});
});

describe("readEnvironmentFromLoginShell", () => {
it("extracts multiple environment variables from a login shell command", () => {
const execFile = vi.fn<
Expand Down Expand Up @@ -126,3 +162,38 @@ describe("readEnvironmentFromLoginShell", () => {
});
});
});

describe("listLoginShellCandidates", () => {
it("returns env shell, user shell, then the platform fallback without duplicates", () => {
expect(listLoginShellCandidates("darwin", " /opt/homebrew/bin/nu ", "/bin/zsh")).toEqual([
"/opt/homebrew/bin/nu",
"/bin/zsh",
]);
});

it("falls back to the platform default when no shells are available", () => {
expect(listLoginShellCandidates("linux", undefined, "")).toEqual(["/bin/bash"]);
});
});

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(
mergePathEntries("/opt/homebrew/bin:/usr/bin", "/Users/test/.local/bin:/usr/bin", "darwin"),
).toBe("/opt/homebrew/bin:/usr/bin:/Users/test/.local/bin");
});

it("uses the platform-specific delimiter", () => {
expect(mergePathEntries("C:\\Tools;C:\\Windows", "C:\\Windows;C:\\Git", "win32")).toBe(
"C:\\Tools;C:\\Windows;C:\\Git",
);
});
});
Loading
Loading