Validate the default shell #7680
Conversation
dosubot
bot
added
the
size:L
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for your contribution! I've reviewed the changes and found some issues that need attention. The implementation adds valuable shell path validation, but there are a few concerns around race conditions, error handling, and test coverage that should be addressed.
| const shellPathCache = new Map<string, { path: string; timestamp: number }>() | ||
| const CACHE_TTL = 5 * 60 * 1000 | ||
|
|
||
| async function validateShellPath(shellPath: string): Promise<string | null> { |
There was a problem hiding this comment.
Is this intentional? There's a potential race condition here. If multiple calls to validateShellPath happen simultaneously for the same path, between checking the cache (line 29-31) and setting it (line 37-40), another call could start validation, leading to duplicate which executions.
Consider using a Map of Promises to handle concurrent requests:
| async function validateShellPath(shellPath: string): Promise<string | null> { | |
| const shellPathCache = new Map<string, { path: string; timestamp: number }>() | |
| const pendingValidations = new Map<string, Promise<string | null>>() | |
| const CACHE_TTL = 5 * 60 * 1000 | |
| async function validateShellPath(shellPath: string): Promise<string | null> { | |
| try { | |
| const cached = shellPathCache.get(shellPath) | |
| if (cached && Date.now() - cached.timestamp < CACHE_TTL) { | |
| return cached.path | |
| } | |
| // Check if validation is already in progress | |
| const pending = pendingValidations.get(shellPath) | |
| if (pending) { | |
| return pending | |
| } | |
| // Start new validation | |
| const validationPromise = which(shellPath, { nothrow: true }).then(resolvedPath => { | |
| if (resolvedPath) { | |
| shellPathCache.set(shellPath, { | |
| path: resolvedPath, | |
| timestamp: Date.now(), | |
| }) | |
| } | |
| pendingValidations.delete(shellPath) | |
| return resolvedPath | |
| }) | |
| pendingValidations.set(shellPath, validationPromise) | |
| return validationPromise | |
| } catch { | |
| return null | |
| } | |
| } |
| } | ||
|
|
||
| return null | ||
| } catch { |
There was a problem hiding this comment.
Could we improve error visibility here? The catch block silently swallows errors without any logging. This could make debugging difficult if which throws unexpected errors.
Consider adding error logging:
| } catch { | |
| } catch (error) { | |
| // Log the error for debugging purposes | |
| console.warn('Failed to validate shell path:', shellPath, error) | |
| return null |
|
|
||
| // 4. Finally, fall back to a default | ||
| // 4. Finally, fall back to platform-specific defaults | ||
| if (process.platform === "win32") { |
There was a problem hiding this comment.
Is it intentional that these fallback paths are returned without validation? On some systems, even these default paths might not exist. Should we validate these as well, or at least document why we're confident they'll always exist?
Consider:
| if (process.platform === "win32") { | |
| // 4. Finally, fall back to platform-specific defaults | |
| // Note: These defaults are returned without validation as they are | |
| // considered system essentials that should always exist | |
| if (process.platform === "win32") { | |
| return SHELL_PATHS.CMD | |
| } else { | |
| return SHELL_PATHS.FALLBACK | |
| } |
| /** | ||
| * Clears the shell path cache (useful for testing) | ||
| */ | ||
| export function clearShellPathCache(): void { |
There was a problem hiding this comment.
Is this function intended for future use? It's exported but never called in production code, only in tests. If it's for testing only, should it be made internal or documented as a test utility?
| vi.mocked(userInfo).mockReturnValue({ shell: null } as any) | ||
|
|
||
| // Mock which to always resolve paths successfully for tests | ||
| vi.mocked(which).mockImplementation(async (cmd: string) => { |
There was a problem hiding this comment.
Would it be valuable to add test cases where which returns null to ensure the fallback logic works correctly? Currently, the mock always returns the command successfully.
Example test case:
| vi.mocked(which).mockImplementation(async (cmd: string) => { | |
| it("falls back through all options when validation fails", async () => { | |
| // Mock which to return null for all paths | |
| vi.mocked(which).mockResolvedValue(null) | |
| mockVsCodeConfig("windows", "PowerShell", { | |
| PowerShell: { path: "C:\Invalid\Path\pwsh.exe" }, | |
| }) | |
| vi.mocked(userInfo).mockReturnValue({ shell: "C:\Invalid\Shell.exe" } as any) | |
| process.env.COMSPEC = "C:\Invalid\cmd.exe" | |
| // Should fall back to the default CMD path | |
| expect(await getShell()).toBe("C:\Windows\System32\cmd.exe") | |
| }) |
Important
Enhance shell path validation and caching in
getShell()with async handling and update related tests and dependencies.getShell()inshell.tsnow validates shell paths usingwhichand caches results for 5 minutes.getSystemInfoSection()insystem-info.tsupdated to use asyncgetShell().generatePrompt()insystem.tsupdated to awaitgetSystemInfoSection().whichand@types/whichtopackage.json.shell.spec.tsto mockwhichand test asyncgetShell()behavior across platforms.This description was created by
for 0423aa4. You can customize this summary. It will automatically update as commits are pushed.