Skip to content

Conversation

@roomote
Copy link

@roomote roomote bot commented Oct 9, 2025

Description

This PR attempts to address Issue #8578. The user reported that the getShellFromEnv() function was returning hardcoded fallback values instead of properly checking environment variables.

Changes

  • Modified getShellFromEnv() to return null instead of hardcoded fallback values when COMSPEC (Windows) or SHELL (Unix) environment variables are not set
  • This ensures the function accurately reflects whether the shell path was obtained from environment variables
  • Updated test comments to clarify the expected behavior with the fix

Technical Details

The fix ensures that getShellFromEnv() properly returns null when environment variables are not set, allowing the main getShell() function's fallback chain to work as intended:

  1. First checks VS Code config
  2. Then checks userInfo()
  3. Then checks environment variables (now returns null if not set)
  4. Finally falls back to getSafeFallbackShell()

Testing

  • All existing tests continue to pass
  • The fallback logic is properly handled by getSafeFallbackShell()
  • Tests have been updated with comments to document the new behavior

Fixes #8578

Feedback and guidance are welcome!


Important

getShellFromEnv() now returns null when environment variables are not set, ensuring proper fallback logic in shell detection.

  • Behavior:
    • getShellFromEnv() in shell.ts now returns null if COMSPEC (Windows) or SHELL (Unix) environment variables are not set.
    • Ensures getShell() function's fallback chain works as intended, checking VS Code config, userInfo(), environment variables, and finally getSafeFallbackShell().
  • Testing:
    • Updated test comments in shell.spec.ts to clarify expected behavior with the fix.
    • All existing tests continue to pass, ensuring fallback logic is properly handled by getSafeFallbackShell().

This description was created by Ellipsis for d63a431. You can customize this summary. It will automatically update as commits are pushed.

…not set

- Modified getShellFromEnv() to return null instead of hardcoded fallback values when COMSPEC (Windows) or SHELL (Unix) environment variables are not set
- This ensures the function accurately reflects whether the shell path was obtained from environment variables
- Updated test comments to clarify the expected behavior with the fix
- All existing tests continue to pass as the fallback logic is handled by getSafeFallbackShell()

Fixes #8578
@roomote roomote bot requested review from cte, jr and mrubens as code owners October 9, 2025 11:46
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Oct 9, 2025
Copy link
Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

I found some issues that need attention before merging.

// On Linux, SHELL is commonly the environment variable
return env.SHELL || "/bin/bash"
}
return null
Copy link
Author

Choose a reason for hiding this comment

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

P1: Potential regression for unknown Unix-like platforms

The current implementation will return null for platforms that are not win32, darwin, or linux (e.g., freebsd, openbsd, sunos, aix). This means these platforms won't check the SHELL environment variable at all.

Before this PR, these platforms would have gotten a hardcoded fallback (/bin/bash for Linux path). While returning null allows the fallback chain to work, it skips checking SHELL entirely for these platforms.

Consider this alternative that checks SHELL for all Unix-like platforms:

Suggested change
return null
// On Unix-like systems, SHELL is the environment variable
return env.SHELL || null
}

This way, any Unix-like platform (including FreeBSD, OpenBSD, etc.) will check SHELL, and only return null if it's not set, allowing the proper fallback chain to work.

@roomote roomote bot mentioned this pull request Oct 9, 2025
2 tasks
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 9, 2025
@daniel-lxs daniel-lxs closed this Oct 28, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Oct 28, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Oct 28, 2025
@daniel-lxs daniel-lxs deleted the fix/shell-path-env-detection branch October 28, 2025 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Prompt Add Shell Path

4 participants