fix: preserve nix-shell environment when creating terminals #7022
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Modified Terminal.getEnv() to spread process.env first, preserving nix-shell and other environment variables - Updated TerminalRegistry tests to use expect.objectContaining() for environment checks - Added comprehensive tests for environment preservation in Terminal.env-preservation.spec.ts Fixes #7021
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| public static getEnv(): Record<string, string> { | ||
| // Start with the existing process environment to preserve nix-shell and other environment variables | ||
| const env: Record<string, string> = { | ||
| ...process.env, |
Contributor
Author
There was a problem hiding this comment.
Is this intentional that we're spreading process.env without filtering undefined values? The type signature suggests Record<string, string> but process.env can contain string | undefined. While the tests show this works, would it be safer to explicitly filter out undefined values?
Suggested change
| ...process.env, | |
| // Start with the existing process environment to preserve nix-shell and other environment variables | |
| const env: Record<string, string> = { | |
| ...Object.fromEntries( | |
| Object.entries(process.env).filter(([_, v]) => v !== undefined) | |
| ) as Record<string, string>, |
| } | ||
|
|
||
| public static getEnv(): Record<string, string> { | ||
| // Start with the existing process environment to preserve nix-shell and other environment variables |
Contributor
Author
There was a problem hiding this comment.
Could we consider adding more detailed documentation here about the precedence order? It would help future maintainers understand which variables Roo Code specifically overrides and why.
For example:
Suggested change
| // Start with the existing process environment to preserve nix-shell and other environment variables | |
| // Start with the existing process environment to preserve nix-shell and other environment variables | |
| // Precedence order: process.env values are preserved, then Roo Code-specific overrides are applied. | |
| // Overridden variables: PAGER, VTE_VERSION, and conditionally: PROMPT_COMMAND, PROMPT_EOL_MARK, | |
| // ITERM_SHELL_INTEGRATION_INSTALLED, POWERLEVEL9K_TERM_SHELL_INTEGRATION, ZDOTDIR |
| // Should not throw and should not include undefined values | ||
| expect(env.SOME_UNDEFINED_VAR).toBeUndefined() | ||
| }) | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Great test coverage! Could we add an edge case test for when process.env might contain non-string values? Though this shouldn't happen in practice, it would make the test suite more robust.
hannesrudolph
added
the
Issue/PR - Triage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes an issue where Roo Code was creating new terminal instances that lost the nix-shell environment variables, breaking the nix-shell environment for users.
Problem
When a user is in a
nix-shellenvironment and asks Roo Code to run a command, it creates a new terminal with a limited set of environment variables, which cancels out all the nix-shell specific environment variables likeNIX_BUILD_CORES,NIX_STORE,IN_NIX_SHELL,PKG_CONFIG_PATH, etc.Solution
Modified
Terminal.getEnv()to preserve the existingprocess.envvariables by spreading them first before applying Roo Code-specific overrides. This ensures that nix-shell environment variables (and any other environment variables from the parent process) are preserved when creating new terminals.Changes
Terminal.getEnv()to spreadprocess.envfirst, preserving all existing environment variablesTerminalRegistrytests to useexpect.objectContaining()for environment checks since we now include all process environment variablesTerminal.env-preservation.spec.tsto verify environment preservation works correctlyTesting
Fixes #7021
Important
Fixes environment variable preservation in terminals by modifying
Terminal.getEnv()to retain all existingprocess.envvariables, ensuring nix-shell compatibility.Terminal.getEnv()now preserves all existingprocess.envvariables, ensuring nix-shell and other environment variables are retained.Terminal.env-preservation.spec.tsto test environment preservation, including nix-shell variables.TerminalRegistry.spec.tsto useexpect.objectContaining()for environment checks.Terminal.tsto spreadprocess.envingetEnv()function.This description was created by
for d1612f4. You can customize this summary. It will automatically update as commits are pushed.