-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: inherit process environment variables in terminal shells #7095
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?
Conversation
- Modified Terminal.getEnv() to start with process.env and apply Roo-specific overrides - This ensures user PATH customizations and other environment variables are available in Roo terminals - Added comprehensive tests for environment variable inheritance - Updated existing tests to use expect.objectContaining() for environment checks Fixes #7094
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
// Start with the current process environment to inherit user's PATH and other customizations | ||
// Filter out undefined values to satisfy the return type | ||
const env: Record<string, string> = {} | ||
for (const [key, value] of Object.entries(process.env)) { |
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.
Security consideration: We're now inheriting ALL environment variables from process.env. While this solves the PATH issue perfectly, should we consider adding a whitelist/blacklist mechanism to filter out potentially sensitive variables like API keys or tokens? Some users might have secrets in their environment that shouldn't be exposed to terminal processes.
VTE_VERSION: "0", | ||
// Start with the current process environment to inherit user's PATH and other customizations | ||
// Filter out undefined values to satisfy the return type | ||
const env: Record<string, string> = {} |
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.
Performance thought: Would using Object.assign() with spread operator be more performant here? Something like:
const env: Record<string, string> = {} | |
// Start with the current process environment to inherit user's PATH and other customizations | |
// Filter out undefined values to satisfy the return type | |
const env: Record<string, string> = { ...process.env } | |
for (const key in env) { | |
if (env[key] === undefined) { | |
delete env[key] | |
} | |
} |
Though the current approach is clear and works well.
expect(env.VTE_VERSION).toBe("0") | ||
expect(env.PAGER).toBeDefined() | ||
// PATH and HOME will be undefined but that's okay | ||
expect(env.PATH).toBeUndefined() |
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.
Nice edge case testing! Though I wonder if we should set sensible defaults for essential variables like PATH when they're undefined? Some shell operations might fail without these critical variables.
// See https://wiki.gnome.org/Apps/Terminal/VTE | ||
VTE_VERSION: "0", | ||
// Start with the current process environment to inherit user's PATH and other customizations | ||
// Filter out undefined values to satisfy the return type |
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.
Could we make this comment more explicit about WHY we filter undefined values? Something like: "Filter out undefined values to satisfy TypeScript's Record<string, string> return type"
Fixes #7094
Problem
When Roo Code creates terminals (especially in remote SSH environments), it was not inheriting the user's PATH customizations and other environment variables from their shell configuration files. This meant that tools and commands available in the user's normal shell were not accessible in Roo's integrated terminal.
Solution
Modified
Terminal.getEnv()
to start withprocess.env
and then apply Roo-specific overrides. This ensures that:Changes
Terminal.getEnv()
to inherit fromprocess.env
while filtering out undefined valuesexpect.objectContaining()
for more flexible environment checksTesting
Terminal.env.spec.ts
with 11 tests covering environment inheritanceThis change ensures that users working in remote SSH environments or with custom shell configurations will have their full environment available when Roo executes commands.
Important
Terminal.getEnv()
now inherits user environment variables, preserving PATH and custom settings while applying Roo-specific overrides, with comprehensive tests added.Terminal.getEnv()
now starts withprocess.env
to inherit user environment variables, preserving PATH and custom variables.VTE_VERSION
andPAGER
are applied after inheriting user environment.Terminal.env.spec.ts
with 11 tests for environment variable inheritance.TerminalRegistry.spec.ts
to useexpect.objectContaining()
for environment checks.This description was created by
for 6b50a26. You can customize this summary. It will automatically update as commits are pushed.