Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/integrations/terminal/Terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,9 @@ export class Terminal extends BaseTerminal {
}

public static getEnv(): Record<string, string> {
// Start with the existing process environment to preserve nix-shell and other environment variables
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const env: Record<string, string> = {
...process.env,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>,

PAGER: process.platform === "win32" ? "" : "cat",

// VTE must be disabled because it prevents the prompt command from executing
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// npx vitest run src/integrations/terminal/__tests__/Terminal.env-preservation.spec.ts

import { Terminal } from "../Terminal"

describe("Terminal.getEnv() - Environment Preservation", () => {
let originalEnv: NodeJS.ProcessEnv

beforeEach(() => {
// Save original environment
originalEnv = { ...process.env }
})

afterEach(() => {
// Restore original environment
process.env = originalEnv
})

it("should preserve existing environment variables", () => {
// Set up test environment variables (simulating nix-shell)
process.env.NIX_BUILD_CORES = "4"
process.env.NIX_STORE = "/nix/store"
process.env.IN_NIX_SHELL = "impure"
process.env.CUSTOM_VAR = "custom_value"

const env = Terminal.getEnv()

// Check that nix-shell variables are preserved
expect(env.NIX_BUILD_CORES).toBe("4")
expect(env.NIX_STORE).toBe("/nix/store")
expect(env.IN_NIX_SHELL).toBe("impure")
expect(env.CUSTOM_VAR).toBe("custom_value")

// Check that Roo Code specific variables are still set
expect(env.PAGER).toBe(process.platform === "win32" ? "" : "cat")
expect(env.VTE_VERSION).toBe("0")
})

it("should override specific environment variables when needed", () => {
// Set conflicting environment variables
process.env.PAGER = "less"
process.env.VTE_VERSION = "6003"

const env = Terminal.getEnv()

// Check that Roo Code overrides take precedence
expect(env.PAGER).toBe(process.platform === "win32" ? "" : "cat")
expect(env.VTE_VERSION).toBe("0")
})

it("should preserve PATH and other critical environment variables", () => {
// Set critical environment variables
process.env.PATH = "/usr/local/bin:/usr/bin:/bin"
process.env.HOME = "/home/user"
process.env.USER = "testuser"
process.env.SHELL = "/bin/zsh"

const env = Terminal.getEnv()

// Check that critical variables are preserved
expect(env.PATH).toBe("/usr/local/bin:/usr/bin:/bin")
expect(env.HOME).toBe("/home/user")
expect(env.USER).toBe("testuser")
expect(env.SHELL).toBe("/bin/zsh")
})

it("should handle undefined environment variables gracefully", () => {
// Clear some environment variables
delete process.env.SOME_UNDEFINED_VAR

const env = Terminal.getEnv()

// Should not throw and should not include undefined values
expect(env.SOME_UNDEFINED_VAR).toBeUndefined()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

it("should preserve nix-shell specific environment modifications", () => {
// Simulate a comprehensive nix-shell environment
process.env.NIX_BUILD_CORES = "8"
process.env.NIX_STORE = "/nix/store"
process.env.IN_NIX_SHELL = "impure"
process.env.NIX_ENFORCE_NO_NATIVE = "1"
process.env.PKG_CONFIG_PATH = "/nix/store/xxx/lib/pkgconfig"
process.env.NODE_PATH = "/nix/store/yyy/lib/node_modules"
process.env.buildInputs = "/nix/store/aaa /nix/store/bbb"
process.env.nativeBuildInputs = "/nix/store/ccc /nix/store/ddd"

const env = Terminal.getEnv()

// All nix-shell variables should be preserved
expect(env.NIX_BUILD_CORES).toBe("8")
expect(env.NIX_STORE).toBe("/nix/store")
expect(env.IN_NIX_SHELL).toBe("impure")
expect(env.NIX_ENFORCE_NO_NATIVE).toBe("1")
expect(env.PKG_CONFIG_PATH).toBe("/nix/store/xxx/lib/pkgconfig")
expect(env.NODE_PATH).toBe("/nix/store/yyy/lib/node_modules")
expect(env.buildInputs).toBe("/nix/store/aaa /nix/store/bbb")
expect(env.nativeBuildInputs).toBe("/nix/store/ccc /nix/store/ddd")
})

it("should add PROMPT_EOL_MARK when Terminal.getTerminalZshClearEolMark() is true", () => {
// This is the default behavior
const originalValue = Terminal.getTerminalZshClearEolMark()
Terminal.setTerminalZshClearEolMark(true)

try {
const env = Terminal.getEnv()
expect(env.PROMPT_EOL_MARK).toBe("")
} finally {
Terminal.setTerminalZshClearEolMark(originalValue)
}
})

it("should not override PROMPT_EOL_MARK when Terminal.getTerminalZshClearEolMark() is false", () => {
const originalValue = Terminal.getTerminalZshClearEolMark()
Terminal.setTerminalZshClearEolMark(false)
process.env.PROMPT_EOL_MARK = "%"

try {
const env = Terminal.getEnv()
expect(env.PROMPT_EOL_MARK).toBe("%")
} finally {
Terminal.setTerminalZshClearEolMark(originalValue)
}
})
})
16 changes: 8 additions & 8 deletions src/integrations/terminal/__tests__/TerminalRegistry.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ describe("TerminalRegistry", () => {
cwd: "/test/path",
name: "Roo Code",
iconPath: expect.any(Object),
env: {
env: expect.objectContaining({
PAGER,
VTE_VERSION: "0",
PROMPT_EOL_MARK: "",
},
}),
})
})

Expand All @@ -64,12 +64,12 @@ describe("TerminalRegistry", () => {
cwd: "/test/path",
name: "Roo Code",
iconPath: expect.any(Object),
env: {
env: expect.objectContaining({
PAGER,
PROMPT_COMMAND: "sleep 0.05",
VTE_VERSION: "0",
PROMPT_EOL_MARK: "",
},
}),
})
} finally {
// Restore original delay
Expand All @@ -86,12 +86,12 @@ describe("TerminalRegistry", () => {
cwd: "/test/path",
name: "Roo Code",
iconPath: expect.any(Object),
env: {
env: expect.objectContaining({
PAGER,
VTE_VERSION: "0",
PROMPT_EOL_MARK: "",
ITERM_SHELL_INTEGRATION_INSTALLED: "Yes",
},
}),
})
} finally {
Terminal.setTerminalZshOhMy(false)
Expand All @@ -107,12 +107,12 @@ describe("TerminalRegistry", () => {
cwd: "/test/path",
name: "Roo Code",
iconPath: expect.any(Object),
env: {
env: expect.objectContaining({
PAGER,
VTE_VERSION: "0",
PROMPT_EOL_MARK: "",
POWERLEVEL9K_TERM_SHELL_INTEGRATION: "true",
},
}),
})
} finally {
Terminal.setTerminalZshP10k(false)
Expand Down
Loading