fix(shell): bound PATH repair fallbacks#831
fix(shell): bound PATH repair fallbacks#831Chrono-byte wants to merge 3 commits intopingdotgg:mainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request refactors shell-based PATH resolution across desktop and server modules. Instead of platform-gating solely on Darwin with manual shell selection, the system now uses new utility functions: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
the
vouch:unvouched
There was a problem hiding this comment.
Pull request overview
This PR refactors PATH “repair” during startup on macOS and Linux by centralizing login-shell PATH resolution behind a shared resolver with a bounded set of shell candidates and invocation modes, to avoid excessive synchronous probing.
Changes:
- Add bounded shell candidate selection (
defaultShellCandidates) and a multi-shell resolver (resolvePathFromLoginShells) inpackages/shared. - Update desktop + server startup PATH repair to use the shared resolver and enable Linux support.
- Expand unit tests for the new fallback behavior and candidate selection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/shared/src/shell.ts | Adds login-shell PATH resolution fallbacks and shared candidate/resolution helpers. |
| packages/shared/src/shell.test.ts | Adds tests for the new fallback behavior and default candidate sets. |
| apps/server/src/os-jank.ts | Switches server PATH repair to the shared bounded resolver; enables Linux. |
| apps/desktop/src/fixPath.ts | Switches desktop PATH repair to the shared bounded resolver; enables Linux. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/shared/src/shell.ts
Outdated
| 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. | ||
| } | ||
| } |
There was a problem hiding this comment.
resolvePathFromLoginShells's onError hook (and the T3CODE_DEBUG_SHELL_PATH logging) is effectively unreachable for normal shell failures because readPathFromLoginShell swallows all execFile errors internally. If the intent is to report per-shell failures, either let errors bubble up from readPathFromLoginShell (after trying its arg fallbacks) or invoke the reporter from within readPathFromLoginShell when each mode fails.
| 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); | ||
| if (resolvedPath) { | ||
| process.env.PATH = resolvedPath; | ||
| } |
There was a problem hiding this comment.
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.
apps/server/src/os-jank.ts
Outdated
|
|
||
| const resolvedPath = resolvePathFromLoginShells(shells); |
There was a problem hiding this comment.
defaultShellCandidates() includes process.env.SHELL, which means on Linux this server startup path repair may execute whatever binary is pointed to by SHELL. If this code can run in environments where SHELL is not trusted/controlled (e.g., service managers, container entrypoints), consider ignoring process.env.SHELL here or validating/allowlisting shell paths before executing them.
| 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); |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
packages/shared/src/shell.test.ts
Outdated
| 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); | ||
| }); |
There was a problem hiding this comment.
The resolvePathFromLoginShells test undercounts how many execFile calls occur. readPathFromLoginShell now tries both -ilc and -lc per shell candidate, so a shell that always throws (like /bin/zsh in this mock) will be invoked twice before moving on. Update the expectation (or the mock) so the call count matches the two-mode fallback behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/shell.ts`:
- Around line 38-64: readPathFromLoginShell currently ignores the overall
deadline and always uses LOGIN_SHELL_TIMEOUT_MS for each exec, so a shell
started late can exceed the remaining time budget; modify readPathFromLoginShell
(and the similar block at 105-123) to accept a deadline (or remainingMs)
parameter and before each execFile call compute remaining = deadline -
Date.now(), return/stop if remaining <= 0, and set the execFile timeout to
Math.min(LOGIN_SHELL_TIMEOUT_MS, remaining) when calling execFile(shell, args, {
encoding: "utf8", timeout: effectiveTimeout }). Ensure
resolvePathFromLoginShells passes its deadline through to readPathFromLoginShell
so the global 2s budget is respected across attempts.
- Around line 155-159: The Linux branch currently returns
!entries.has(`${homePath}/.local/bin`) && !entries.has("/usr/local/bin") which
lets presence of /usr/local/bin short-circuit and skip repair even when the
user-local ~/.local/bin is missing; change the Linux return to only check the
user-local path (return !entries.has(`${homePath}/.local/bin`)) so the repair
condition is driven solely by the missing ~/.local/bin. Update the expression in
the branch that uses platform, entries and homePath accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3480afc0-f1d5-49c5-9eec-6b71c7c3a3da
📒 Files selected for processing (4)
apps/desktop/src/fixPath.tsapps/server/src/os-jank.tspackages/shared/src/shell.test.tspackages/shared/src/shell.ts
| export function readPathFromLoginShell( | ||
| shell: string, | ||
| execFile: ExecFileSyncLike = execFileSync, | ||
| onError?: LoginShellErrorReporter, | ||
| ): string | undefined { | ||
| const output = execFile(shell, ["-ilc", PATH_CAPTURE_COMMAND], { | ||
| encoding: "utf8", | ||
| timeout: 5000, | ||
| }); | ||
| return extractPathFromShellOutput(output) ?? undefined; | ||
| let lastError: unknown; | ||
| for (const args of LOGIN_SHELL_ARG_SETS) { | ||
| try { | ||
| const output = execFile(shell, args, { | ||
| encoding: "utf8", | ||
| timeout: LOGIN_SHELL_TIMEOUT_MS, | ||
| }); | ||
| const resolvedPath = extractPathFromShellOutput(output) ?? undefined; | ||
| if (resolvedPath) { | ||
| return resolvedPath; | ||
| } | ||
| } catch (error) { | ||
| lastError = error; | ||
| onError?.(shell, args, error); | ||
| } | ||
| } | ||
|
|
||
| if (lastError) { | ||
| throw lastError; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Carry the 2s budget into each shell attempt.
resolvePathFromLoginShells() only checks deadline before starting a shell. Once inside readPathFromLoginShell(), that shell can still spend both 750 ms fallbacks, so a probe that starts just before the cutoff can add another ~1.5 s. Because fixPath() is called during startup in apps/desktop/src/main.ts, Line 47, and apps/server/src/main.ts, Line 80, this still shows up as launch latency.
⏱️ 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
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/shell.ts` around lines 38 - 64, readPathFromLoginShell
currently ignores the overall deadline and always uses LOGIN_SHELL_TIMEOUT_MS
for each exec, so a shell started late can exceed the remaining time budget;
modify readPathFromLoginShell (and the similar block at 105-123) to accept a
deadline (or remainingMs) parameter and before each execFile call compute
remaining = deadline - Date.now(), return/stop if remaining <= 0, and set the
execFile timeout to Math.min(LOGIN_SHELL_TIMEOUT_MS, remaining) when calling
execFile(shell, args, { encoding: "utf8", timeout: effectiveTimeout }). Ensure
resolvePathFromLoginShells passes its deadline through to readPathFromLoginShell
so the global 2s budget is respected across attempts.
| if (platform === "darwin") { | ||
| return !entries.has("/opt/homebrew/bin") && !entries.has("/usr/local/bin"); | ||
| } | ||
|
|
||
| return !entries.has(`${homePath}/.local/bin`) && !entries.has("/usr/local/bin"); |
There was a problem hiding this comment.
Don't let /usr/local/bin short-circuit Linux repair.
The Linux expectations in packages/shared/src/shell.test.ts, Lines 158-165, key off ~/.local/bin availability, but this branch also skips when only /usr/local/bin exists. That means repair can be bypassed while the user-local PATH entry is still missing.
🩹 Minimal fix
- return !entries.has(`${homePath}/.local/bin`) && !entries.has("/usr/local/bin");
+ return !entries.has(`${homePath}/.local/bin`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/shell.ts` around lines 155 - 159, The Linux branch
currently returns !entries.has(`${homePath}/.local/bin`) &&
!entries.has("/usr/local/bin") which lets presence of /usr/local/bin
short-circuit and skip repair even when the user-local ~/.local/bin is missing;
change the Linux return to only check the user-local path (return
!entries.has(`${homePath}/.local/bin`)) so the repair condition is driven solely
by the missing ~/.local/bin. Update the expression in the branch that uses
platform, entries and homePath accordingly.
What Changed
Tightened desktop/server PATH repair on macOS and Linux by moving recovery through a shared resolver with bounded fallbacks.
Why
Packaged launches can start with an incomplete PATH, which breaks downstream process resolution. We need to recover a login-shell PATH, but doing that by probing too many shells synchronously can stall startup. This change keeps the recovery behavior while capping worst-case startup cost.
Note
Bound PATH repair fallbacks with multi-shell candidates and a platform gate
fixPathandos-jank.tswith a shared, cross-platform flow from@t3tools/shared/shell.shouldRepairPathto gate repair by platform (darwin/linux only) and PATH contents (e.g. missing/opt/homebrew/binon macOS).resolvePathFromLoginShellswhich iteratesdefaultShellCandidates()within a 2,000ms global deadline, falling back from-ilcto-lcper shell at 750ms each.Macroscope summarized e488a7f.
Summary by CodeRabbit
Bug Fixes
Tests