-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(shell): bound PATH repair fallbacks #831
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,10 @@ | ||
| import { readPathFromLoginShell } from "@t3tools/shared/shell"; | ||
| import { defaultShellCandidates, resolvePathFromLoginShells } from "@t3tools/shared/shell"; | ||
|
|
||
| export function fixPath(): void { | ||
| if (process.platform !== "darwin") return; | ||
| if (process.platform !== "darwin" && process.platform !== "linux") return; | ||
|
|
||
| try { | ||
| const shell = process.env.SHELL ?? "/bin/zsh"; | ||
| const result = readPathFromLoginShell(shell); | ||
| if (result) { | ||
| process.env.PATH = result; | ||
| } | ||
| } catch { | ||
| // Keep inherited PATH if shell lookup fails. | ||
| const result = resolvePathFromLoginShells(defaultShellCandidates()); | ||
| if (result) { | ||
| process.env.PATH = result; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,15 @@ | ||||||||||||||||||||||||||||||||||||||||
| import * as OS from "node:os"; | ||||||||||||||||||||||||||||||||||||||||
| import { Effect, Path } from "effect"; | ||||||||||||||||||||||||||||||||||||||||
| import { readPathFromLoginShell } from "@t3tools/shared/shell"; | ||||||||||||||||||||||||||||||||||||||||
| import { defaultShellCandidates, resolvePathFromLoginShells } from "@t3tools/shared/shell"; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| export function fixPath(): void { | ||||||||||||||||||||||||||||||||||||||||
| if (process.platform !== "darwin") return; | ||||||||||||||||||||||||||||||||||||||||
| if (process.platform !== "darwin" && process.platform !== "linux") return; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||
| const shell = process.env.SHELL ?? "/bin/zsh"; | ||||||||||||||||||||||||||||||||||||||||
| const result = readPathFromLoginShell(shell); | ||||||||||||||||||||||||||||||||||||||||
| if (result) { | ||||||||||||||||||||||||||||||||||||||||
| process.env.PATH = result; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||
| // Silently ignore — keep default PATH | ||||||||||||||||||||||||||||||||||||||||
| const shells = defaultShellCandidates(); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const resolvedPath = resolvePathFromLoginShells(shells); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| const resolvedPath = resolvePathFromLoginShells(shells); | |
| const allowedShells = new Set<string>([ | |
| "/bin/bash", | |
| "/usr/bin/bash", | |
| "/bin/zsh", | |
| "/usr/bin/zsh", | |
| "/bin/sh", | |
| "/usr/bin/sh", | |
| "/bin/dash", | |
| "/usr/bin/dash", | |
| "/bin/fish", | |
| "/usr/bin/fish", | |
| ]); | |
| const filteredShells = shells.filter((shell) => allowedShells.has(shell)); | |
| if (filteredShells.length === 0) return; | |
| const resolvedPath = resolvePathFromLoginShells(filteredShells); |
Copilot
AI
Mar 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On darwin/linux this runs a synchronous PATH repair that can block startup for up to timeout * argModes * shellCandidates (currently 5000ms * 2 * up to 3 = 30s on macOS; 20s on Linux). If the PR goal is to cap worst-case startup cost, consider lowering the per-invocation timeout and/or adding an overall deadline / gating the repair to cases where PATH looks incomplete.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,11 @@ | ||
| import { describe, expect, it, vi } from "vitest"; | ||
|
|
||
| import { extractPathFromShellOutput, readPathFromLoginShell } from "./shell"; | ||
| import { | ||
| defaultShellCandidates, | ||
| extractPathFromShellOutput, | ||
| readPathFromLoginShell, | ||
| resolvePathFromLoginShells, | ||
| } from "./shell"; | ||
|
|
||
| describe("extractPathFromShellOutput", () => { | ||
| it("extracts the path between capture markers", () => { | ||
|
|
@@ -54,4 +59,111 @@ describe("readPathFromLoginShell", () => { | |
| expect(args?.[1]).toContain("__T3CODE_PATH_END__"); | ||
| expect(options).toEqual({ encoding: "utf8", timeout: 5000 }); | ||
| }); | ||
|
|
||
| it("falls back to non-interactive login mode when interactive login fails", () => { | ||
| const execFile = vi.fn< | ||
| ( | ||
| file: string, | ||
| args: ReadonlyArray<string>, | ||
| options: { encoding: "utf8"; timeout: number }, | ||
| ) => string | ||
| >((_, args) => { | ||
| if (args[0] === "-ilc") { | ||
| throw new Error("interactive login unsupported"); | ||
| } | ||
| return "__T3CODE_PATH_START__\n/a:/b\n__T3CODE_PATH_END__\n"; | ||
| }); | ||
|
|
||
| expect(readPathFromLoginShell("/bin/sh", execFile)).toBe("/a:/b"); | ||
| expect(execFile).toHaveBeenCalledTimes(2); | ||
| expect(execFile.mock.calls[0]?.[1]?.[0]).toBe("-ilc"); | ||
| expect(execFile.mock.calls[1]?.[1]?.[0]).toBe("-lc"); | ||
| }); | ||
|
|
||
| describe("resolvePathFromLoginShells", () => { | ||
| it("returns the first resolved PATH from the provided shells", () => { | ||
| const execFile = vi.fn< | ||
| ( | ||
| file: string, | ||
| args: ReadonlyArray<string>, | ||
| options: { encoding: "utf8"; timeout: number }, | ||
| ) => string | ||
| >((file) => { | ||
| if (file === "/bin/zsh") { | ||
| throw new Error("zsh unavailable"); | ||
| } | ||
| return "__T3CODE_PATH_START__\n/a:/b\n__T3CODE_PATH_END__\n"; | ||
| }); | ||
|
|
||
| const result = resolvePathFromLoginShells(["/bin/zsh", "/bin/bash"], execFile); | ||
| expect(result).toBe("/a:/b"); | ||
| expect(execFile).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
||
|
|
||
| it("returns undefined when all shells fail to resolve PATH", () => { | ||
| const execFile = vi.fn< | ||
| ( | ||
| file: string, | ||
| args: ReadonlyArray<string>, | ||
| options: { encoding: "utf8"; timeout: number }, | ||
| ) => string | ||
| >(() => { | ||
| throw new Error("no shells available"); | ||
| }); | ||
|
|
||
| const result = resolvePathFromLoginShells(["/bin/zsh", "/bin/bash"], execFile); | ||
| expect(result).toBeUndefined(); | ||
| expect(execFile).toHaveBeenCalledTimes(2); | ||
Chrono-byte marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("defaultShellCandidates", () => { | ||
| it("limits Linux candidates to the configured shell and POSIX fallback", () => { | ||
| const originalShell = process.env.SHELL; | ||
| process.env.SHELL = "/bin/bash"; | ||
|
|
||
| try { | ||
| expect(defaultShellCandidates("linux")).toEqual(["/bin/bash", "/bin/sh"]); | ||
| } finally { | ||
| process.env.SHELL = originalShell; | ||
| } | ||
| }); | ||
|
|
||
| it("dedupes repeated Linux shell candidates", () => { | ||
| const originalShell = process.env.SHELL; | ||
| process.env.SHELL = "/bin/sh"; | ||
|
|
||
| try { | ||
| expect(defaultShellCandidates("linux")).toEqual(["/bin/sh"]); | ||
| } finally { | ||
| process.env.SHELL = originalShell; | ||
| } | ||
| }); | ||
|
|
||
| it("limits macOS candidates to a small bounded fallback set", () => { | ||
| const originalShell = process.env.SHELL; | ||
| process.env.SHELL = "/opt/homebrew/bin/fish"; | ||
|
|
||
| try { | ||
| expect(defaultShellCandidates("darwin")).toEqual([ | ||
| "/opt/homebrew/bin/fish", | ||
| "/bin/zsh", | ||
| "/bin/bash", | ||
| ]); | ||
| } finally { | ||
| process.env.SHELL = originalShell; | ||
| } | ||
| }); | ||
|
|
||
| it("dedupes repeated macOS shell candidates", () => { | ||
| const originalShell = process.env.SHELL; | ||
| process.env.SHELL = "/bin/zsh"; | ||
|
|
||
| try { | ||
| expect(defaultShellCandidates("darwin")).toEqual(["/bin/zsh", "/bin/bash"]); | ||
| } finally { | ||
| process.env.SHELL = originalShell; | ||
| } | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,11 @@ type ExecFileSyncLike = ( | |
| options: { encoding: "utf8"; timeout: number }, | ||
| ) => string; | ||
|
|
||
| const LOGIN_SHELL_ARG_SETS = [ | ||
| ["-ilc", PATH_CAPTURE_COMMAND], | ||
| ["-lc", PATH_CAPTURE_COMMAND], | ||
| ] as const; | ||
|
|
||
| export function extractPathFromShellOutput(output: string): string | null { | ||
| const startIndex = output.indexOf(PATH_CAPTURE_START); | ||
| if (startIndex === -1) return null; | ||
|
|
@@ -30,9 +35,80 @@ export function readPathFromLoginShell( | |
| shell: string, | ||
| execFile: ExecFileSyncLike = execFileSync, | ||
| ): string | undefined { | ||
| const output = execFile(shell, ["-ilc", PATH_CAPTURE_COMMAND], { | ||
| encoding: "utf8", | ||
| timeout: 5000, | ||
| }); | ||
| return extractPathFromShellOutput(output) ?? undefined; | ||
| for (const args of LOGIN_SHELL_ARG_SETS) { | ||
| try { | ||
| const output = execFile(shell, args, { | ||
| encoding: "utf8", | ||
| timeout: 5000, | ||
| }); | ||
| const resolvedPath = extractPathFromShellOutput(output) ?? undefined; | ||
| if (resolvedPath) { | ||
| return resolvedPath; | ||
| } | ||
| } catch { | ||
| // Try the next shell invocation mode. | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
Comment on lines
38
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Carry the 2s budget into each shell attempt.
⏱️ One compatible fix export function readPathFromLoginShell(
shell: string,
execFile: ExecFileSyncLike = execFileSync,
onError?: LoginShellErrorReporter,
+ deadline = Number.POSITIVE_INFINITY,
): string | undefined {
let lastError: unknown;
for (const args of LOGIN_SHELL_ARG_SETS) {
+ const remainingMs = deadline - Date.now();
+ if (remainingMs <= 0) break;
+
try {
const output = execFile(shell, args, {
encoding: "utf8",
- timeout: LOGIN_SHELL_TIMEOUT_MS,
+ timeout: Math.min(LOGIN_SHELL_TIMEOUT_MS, remainingMs),
});
const resolvedPath = extractPathFromShellOutput(output) ?? undefined;
if (resolvedPath) {
return resolvedPath;
}
@@
try {
- const result = readPathFromLoginShell(shell, execFile, (_failedShell, _args, error) => {
- onError?.(shell, error);
- });
+ const result = readPathFromLoginShell(
+ shell,
+ execFile,
+ (_failedShell, _args, error) => {
+ onError?.(shell, error);
+ },
+ deadline,
+ );
if (result) {
return result;
}Also applies to: 105-123 🤖 Prompt for AI Agents |
||
|
|
||
| function uniqueShellCandidates(candidates: ReadonlyArray<string | undefined>): string[] { | ||
| const unique = new Set<string>(); | ||
|
|
||
| for (const candidate of candidates) { | ||
| if (typeof candidate !== "string") continue; | ||
| const normalized = candidate.trim(); | ||
| if (normalized.length === 0 || unique.has(normalized)) continue; | ||
| unique.add(normalized); | ||
| } | ||
|
|
||
| return [...unique]; | ||
| } | ||
|
|
||
| export function defaultShellCandidates(platform = process.platform): string[] { | ||
| if (platform === "linux") { | ||
| return uniqueShellCandidates([process.env.SHELL, "/bin/sh"]); | ||
| } | ||
|
|
||
| if (platform === "darwin") { | ||
| return uniqueShellCandidates([process.env.SHELL, "/bin/zsh", "/bin/bash"]); | ||
| } | ||
|
|
||
| return uniqueShellCandidates([ | ||
| process.env.SHELL, | ||
| "/bin/zsh", | ||
| "/usr/bin/zsh", | ||
| "/bin/bash", | ||
| "/usr/bin/bash", | ||
| ]); | ||
| } | ||
|
|
||
| type ShellPathResolveErrorReporter = (shell: string, error: unknown) => void; | ||
|
|
||
| const defaultShellPathErrorReporter: ShellPathResolveErrorReporter | undefined = | ||
| process.env.T3CODE_DEBUG_SHELL_PATH === "1" | ||
| ? (shell, error) => { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.warn(`[shell] PATH resolution failed for ${shell}: ${message}`); | ||
| } | ||
| : undefined; | ||
|
|
||
| export function resolvePathFromLoginShells( | ||
| shells: ReadonlyArray<string>, | ||
| execFile: ExecFileSyncLike = execFileSync, | ||
| onError: ShellPathResolveErrorReporter | undefined = defaultShellPathErrorReporter, | ||
| ): string | undefined { | ||
| for (const shell of shells) { | ||
| try { | ||
| const result = readPathFromLoginShell(shell, execFile); | ||
| if (result) { | ||
| return result; | ||
| } | ||
| } catch (error) { | ||
| onError?.(shell, error); | ||
| // Try next shell candidate. | ||
| } | ||
| } | ||
|
||
| return undefined; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs a synchronous PATH repair on Linux as well as macOS; with the current 5s timeout and two invocation modes per shell, Electron main startup can block for up to ~30s in the worst case. Consider reducing the timeout and/or short-circuiting when PATH is already populated enough to resolve required binaries.