-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -151,14 +151,22 @@ export class Terminal extends BaseTerminal { | |||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
public static getEnv(): Record<string, string> { | ||||||||||||||||||||
const env: Record<string, string> = { | ||||||||||||||||||||
PAGER: process.platform === "win32" ? "" : "cat", | ||||||||||||||||||||
|
||||||||||||||||||||
// VTE must be disabled because it prevents the prompt command from executing | ||||||||||||||||||||
// 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 | ||||||||||||||||||||
const env: Record<string, string> = {} | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Suggested change
Though the current approach is clear and works well. |
||||||||||||||||||||
for (const [key, value] of Object.entries(process.env)) { | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||||
if (value !== undefined) { | ||||||||||||||||||||
env[key] = value | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// Apply Roo-specific overrides | ||||||||||||||||||||
env.PAGER = process.platform === "win32" ? "" : "cat" | ||||||||||||||||||||
|
||||||||||||||||||||
// VTE must be disabled because it prevents the prompt command from executing | ||||||||||||||||||||
// See https://wiki.gnome.org/Apps/Terminal/VTE | ||||||||||||||||||||
env.VTE_VERSION = "0" | ||||||||||||||||||||
|
||||||||||||||||||||
// Set Oh My Zsh shell integration if enabled | ||||||||||||||||||||
if (Terminal.getTerminalZshOhMy()) { | ||||||||||||||||||||
env.ITERM_SHELL_INTEGRATION_INSTALLED = "Yes" | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,167 @@ | ||
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest" | ||
import * as vscode from "vscode" | ||
import { Terminal } from "../Terminal" | ||
|
||
// Mock vscode module | ||
vi.mock("vscode", () => ({ | ||
window: { | ||
createTerminal: vi.fn(), | ||
}, | ||
ThemeIcon: vi.fn(), | ||
workspace: { | ||
fs: { | ||
createDirectory: vi.fn().mockResolvedValue(undefined), | ||
writeFile: vi.fn().mockResolvedValue(undefined), | ||
}, | ||
}, | ||
Uri: { | ||
file: vi.fn((path: string) => ({ fsPath: path })), | ||
}, | ||
env: { | ||
appRoot: "/mock/app/root", | ||
}, | ||
})) | ||
|
||
describe("Terminal Environment Variables", () => { | ||
let originalEnv: NodeJS.ProcessEnv | ||
|
||
beforeEach(() => { | ||
// Save original environment | ||
originalEnv = { ...process.env } | ||
|
||
// Set up test environment variables | ||
process.env.PATH = "/custom/bin:/usr/local/bin:/usr/bin:/bin" | ||
process.env.CUSTOM_VAR = "test_value" | ||
process.env.NODE_ENV = "test" | ||
process.env.HOME = "/home/testuser" | ||
|
||
// Reset Terminal static properties | ||
Terminal.setShellIntegrationTimeout(5000) | ||
Terminal.setCommandDelay(0) | ||
Terminal.setTerminalZshClearEolMark(true) | ||
Terminal.setTerminalZshOhMy(false) | ||
Terminal.setTerminalZshP10k(false) | ||
Terminal.setTerminalZdotdir(false) | ||
}) | ||
|
||
afterEach(() => { | ||
// Restore original environment | ||
process.env = originalEnv | ||
vi.clearAllMocks() | ||
}) | ||
|
||
describe("getEnv", () => { | ||
it("should inherit process environment variables", () => { | ||
const env = Terminal.getEnv() | ||
|
||
// Check that custom environment variables are inherited | ||
expect(env.PATH).toBe("/custom/bin:/usr/local/bin:/usr/bin:/bin") | ||
expect(env.CUSTOM_VAR).toBe("test_value") | ||
expect(env.NODE_ENV).toBe("test") | ||
expect(env.HOME).toBe("/home/testuser") | ||
}) | ||
|
||
it("should apply Roo-specific overrides", () => { | ||
const env = Terminal.getEnv() | ||
|
||
// Check Roo-specific overrides | ||
expect(env.PAGER).toBe(process.platform === "win32" ? "" : "cat") | ||
expect(env.VTE_VERSION).toBe("0") | ||
}) | ||
|
||
it("should preserve user PATH while applying overrides", () => { | ||
// Set a complex PATH with many directories | ||
process.env.PATH = | ||
"/home/user/.local/bin:/home/user/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" | ||
|
||
const env = Terminal.getEnv() | ||
|
||
// PATH should be preserved | ||
expect(env.PATH).toBe( | ||
"/home/user/.local/bin:/home/user/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", | ||
) | ||
// But Roo overrides should still be applied | ||
expect(env.VTE_VERSION).toBe("0") | ||
}) | ||
|
||
it("should handle Oh My Zsh integration when enabled", () => { | ||
Terminal.setTerminalZshOhMy(true) | ||
const env = Terminal.getEnv() | ||
|
||
expect(env.ITERM_SHELL_INTEGRATION_INSTALLED).toBe("Yes") | ||
}) | ||
|
||
it("should handle Powerlevel10k integration when enabled", () => { | ||
Terminal.setTerminalZshP10k(true) | ||
const env = Terminal.getEnv() | ||
|
||
expect(env.POWERLEVEL9K_TERM_SHELL_INTEGRATION).toBe("true") | ||
}) | ||
|
||
it("should set PROMPT_COMMAND when command delay is configured", () => { | ||
Terminal.setCommandDelay(100) | ||
const env = Terminal.getEnv() | ||
|
||
expect(env.PROMPT_COMMAND).toBe("sleep 0.1") | ||
}) | ||
|
||
it("should clear ZSH EOL mark when configured", () => { | ||
Terminal.setTerminalZshClearEolMark(true) | ||
const env = Terminal.getEnv() | ||
|
||
expect(env.PROMPT_EOL_MARK).toBe("") | ||
}) | ||
|
||
it("should not override PROMPT_EOL_MARK when disabled", () => { | ||
process.env.PROMPT_EOL_MARK = "%" | ||
Terminal.setTerminalZshClearEolMark(false) | ||
const env = Terminal.getEnv() | ||
|
||
expect(env.PROMPT_EOL_MARK).toBe("%") | ||
}) | ||
|
||
it("should handle undefined environment variables gracefully", () => { | ||
// Remove some environment variables | ||
delete process.env.PATH | ||
delete process.env.HOME | ||
|
||
const env = Terminal.getEnv() | ||
|
||
// Should not throw and should still have Roo overrides | ||
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 commentThe 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. |
||
expect(env.HOME).toBeUndefined() | ||
}) | ||
|
||
it("should not mutate the original process.env", () => { | ||
const originalPath = process.env.PATH | ||
const originalVTE = process.env.VTE_VERSION | ||
|
||
Terminal.getEnv() | ||
|
||
// Original process.env should remain unchanged | ||
expect(process.env.PATH).toBe(originalPath) | ||
expect(process.env.VTE_VERSION).toBe(originalVTE) | ||
}) | ||
}) | ||
|
||
describe("Terminal creation with environment", () => { | ||
it("should pass environment to createTerminal", () => { | ||
const mockCreateTerminal = vi.mocked(vscode.window.createTerminal) | ||
|
||
new Terminal(1, undefined, "/test/dir") | ||
|
||
expect(mockCreateTerminal).toHaveBeenCalledWith( | ||
expect.objectContaining({ | ||
env: expect.objectContaining({ | ||
PATH: "/custom/bin:/usr/local/bin:/usr/bin:/bin", | ||
CUSTOM_VAR: "test_value", | ||
VTE_VERSION: "0", | ||
}), | ||
}), | ||
) | ||
}) | ||
}) | ||
}) |
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"