From a69c816fd70396dd2162026d5369af7554615117 Mon Sep 17 00:00:00 2001 From: Michael Lin Date: Sat, 25 Jul 2026 02:16:24 -0400 Subject: [PATCH] fix(core): preserve activated venv/conda env in spawned shells --- packages/core/src/shell.ts | 16 +- packages/core/test/shell.test.ts | 232 +++++++++++++++++++- packages/desktop/src/main/server.ts | 10 +- packages/desktop/src/main/shell-env.test.ts | 52 ++++- packages/desktop/src/main/shell-env.ts | 26 ++- 5 files changed, 330 insertions(+), 6 deletions(-) diff --git a/packages/core/src/shell.ts b/packages/core/src/shell.ts index 29089106d904..4b2569fd0c37 100644 --- a/packages/core/src/shell.ts +++ b/packages/core/src/shell.ts @@ -165,7 +165,13 @@ function info(file: string): Item { export function args(file: string, command: string, cwd: string) { const n = name(file) - if (n === "nu" || n === "fish") return ["-c", command] + if (n === "nu") return ["-c", command] + if (n === "fish") { + return [ + "-c", + `set -q CONDA_PREFIX; and test "$PATH[1]" != "$CONDA_PREFIX/bin"; and set -gx PATH $CONDA_PREFIX/bin $PATH; set -q VIRTUAL_ENV; and test "$PATH[1]" != "$VIRTUAL_ENV/bin"; and set -gx PATH $VIRTUAL_ENV/bin $PATH; ${command}`, + ] + } if (n === "zsh") { return [ "-l", @@ -173,6 +179,10 @@ export function args(file: string, command: string, cwd: string) { ` [[ -f ~/.zshenv ]] && source ~/.zshenv >/dev/null 2>&1 || true [[ -f "\${ZDOTDIR:-$HOME}/.zshrc" ]] && source "\${ZDOTDIR:-$HOME}/.zshrc" >/dev/null 2>&1 || true + # Restore env bin dirs only if rc files cost them first position; a + # containment check would wrongly skip the restore when merely demoted. + [[ -n "\${CONDA_PREFIX:-}" && ":$PATH:" != ":$CONDA_PREFIX/bin:"* ]] && export PATH="$CONDA_PREFIX/bin:$PATH" + [[ -n "\${VIRTUAL_ENV:-}" && ":$PATH:" != ":$VIRTUAL_ENV/bin:"* ]] && export PATH="$VIRTUAL_ENV/bin:$PATH" cd -- "$1" eval ${JSON.stringify(command)} `, @@ -187,6 +197,10 @@ export function args(file: string, command: string, cwd: string) { ` shopt -s expand_aliases [[ -f ~/.bashrc ]] && source ~/.bashrc >/dev/null 2>&1 || true + # Restore env bin dirs only if rc files cost them first position; a + # containment check would wrongly skip the restore when merely demoted. + [[ -n "\${CONDA_PREFIX:-}" && ":$PATH:" != ":$CONDA_PREFIX/bin:"* ]] && export PATH="$CONDA_PREFIX/bin:$PATH" + [[ -n "\${VIRTUAL_ENV:-}" && ":$PATH:" != ":$VIRTUAL_ENV/bin:"* ]] && export PATH="$VIRTUAL_ENV/bin:$PATH" cd -- "$1" eval ${JSON.stringify(command)} `, diff --git a/packages/core/test/shell.test.ts b/packages/core/test/shell.test.ts index 1cc47a79f6cd..02d5d5d2647f 100644 --- a/packages/core/test/shell.test.ts +++ b/packages/core/test/shell.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from "bun:test" import path from "path" +import { spawnSync } from "child_process" +import { mkdtempSync, mkdirSync, writeFileSync, chmodSync, rmSync } from "fs" import { Shell } from "@opencode-ai/core/shell" import { FSUtil } from "@opencode-ai/core/fs-util" import { which } from "@opencode-ai/core/util/which" @@ -29,6 +31,230 @@ describe("shell", () => { } }) + test("preserves inherited venv after bash -l with rc reordering", () => { + if (process.platform === "win32") return + const bash = which("bash") + if (!bash) return + + const tmp = mkdtempSync("/tmp/opencode-test-venv-") + const home = path.join(tmp, "home") + const venv = path.join(tmp, "venv") + const system = path.join(tmp, "system-bin") + try { + mkdirSync(path.join(venv, "bin"), { recursive: true }) + mkdirSync(home, { recursive: true }) + mkdirSync(system, { recursive: true }) + + writeFileSync( + path.join(venv, "bin", "python"), + "#!/usr/bin/env bash\necho 'venv-python'\nexit 0\n", + ) + chmodSync(path.join(venv, "bin", "python"), 0o755) + writeFileSync( + path.join(system, "python"), + "#!/usr/bin/env bash\necho 'system-python'\nexit 1\n", + ) + chmodSync(path.join(system, "python"), 0o755) + writeFileSync(path.join(home, ".bashrc"), `export PATH="${system}:\$PATH"`, "utf8") + + const args = Shell.args(bash, "command -v python", "/tmp") + const result = spawnSync(bash, args, { + env: { + HOME: home, + VIRTUAL_ENV: venv, + PATH: `${venv}/bin:${system}:/usr/bin:/bin`, + LC_ALL: "C.UTF-8", + }, + cwd: "/tmp", + stdio: ["ignore", "pipe", "ignore"], + timeout: 10_000, + }) + + expect(result.status).toBe(0) + expect(result.stdout.toString("utf8").trim()).toBe(path.join(venv, "bin", "python")) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + test("preserves inherited conda after bash -l with rc reordering", () => { + if (process.platform === "win32") return + const bash = which("bash") + if (!bash) return + + const tmp = mkdtempSync("/tmp/opencode-test-conda-") + const home = path.join(tmp, "home") + const conda = path.join(tmp, "conda") + const system = path.join(tmp, "system-bin") + try { + mkdirSync(path.join(conda, "bin"), { recursive: true }) + mkdirSync(home, { recursive: true }) + mkdirSync(system, { recursive: true }) + + writeFileSync( + path.join(conda, "bin", "python"), + "#!/usr/bin/env bash\necho 'conda-python'\nexit 0\n", + ) + chmodSync(path.join(conda, "bin", "python"), 0o755) + writeFileSync( + path.join(system, "python"), + "#!/usr/bin/env bash\necho 'system-python'\nexit 1\n", + ) + chmodSync(path.join(system, "python"), 0o755) + writeFileSync(path.join(home, ".bashrc"), `export PATH="${system}:\$PATH"`, "utf8") + + const args = Shell.args(bash, "command -v python", "/tmp") + const result = spawnSync(bash, args, { + env: { + HOME: home, + CONDA_PREFIX: conda, + PATH: `${conda}/bin:${system}:/usr/bin:/bin`, + LC_ALL: "C.UTF-8", + }, + cwd: "/tmp", + stdio: ["ignore", "pipe", "ignore"], + timeout: 10_000, + }) + + expect(result.status).toBe(0) + expect(result.stdout.toString("utf8").trim()).toBe(path.join(conda, "bin", "python")) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + test("preserves inherited venv after zsh -l with rc reordering", () => { + if (process.platform === "win32") return + const zsh = which("zsh") + if (!zsh) return + + const tmp = mkdtempSync("/tmp/opencode-test-zsh-venv-") + const home = path.join(tmp, "home") + const venv = path.join(tmp, "venv") + const system = path.join(tmp, "system-bin") + try { + mkdirSync(path.join(venv, "bin"), { recursive: true }) + mkdirSync(home, { recursive: true }) + mkdirSync(system, { recursive: true }) + + writeFileSync( + path.join(venv, "bin", "python"), + "#!/usr/bin/env zsh\necho 'zsh-venv-python'\nexit 0\n", + ) + chmodSync(path.join(venv, "bin", "python"), 0o755) + writeFileSync( + path.join(system, "python"), + "#!/usr/bin/env zsh\necho 'system-python'\nexit 1\n", + ) + chmodSync(path.join(system, "python"), 0o755) + writeFileSync(path.join(home, ".zshrc"), `export PATH="${system}:\$PATH"`, "utf8") + + const args = Shell.args(zsh, "command -v python", "/tmp") + const result = spawnSync(zsh, args, { + env: { + HOME: home, + VIRTUAL_ENV: venv, + PATH: `${venv}/bin:${system}:/usr/bin:/bin`, + LC_ALL: "C.UTF-8", + }, + cwd: "/tmp", + stdio: ["ignore", "pipe", "ignore"], + timeout: 10_000, + }) + + expect(result.status).toBe(0) + expect(result.stdout.toString("utf8").trim()).toBe(path.join(venv, "bin", "python")) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + test("does not duplicate venv bin already first in PATH after bash -l", () => { + if (process.platform === "win32") return + const bash = which("bash") + if (!bash) return + + const tmp = mkdtempSync("/tmp/opencode-test-venv-dup-") + const home = path.join(tmp, "home") + const venv = path.join(tmp, "venv") + try { + mkdirSync(path.join(venv, "bin"), { recursive: true }) + mkdirSync(home, { recursive: true }) + writeFileSync(path.join(home, ".bashrc"), "# rc leaves PATH untouched\n", "utf8") + + const args = Shell.args(bash, "echo $PATH", "/tmp") + const result = spawnSync(bash, args, { + env: { + HOME: home, + VIRTUAL_ENV: venv, + PATH: `${venv}/bin:/usr/bin:/bin`, + LC_ALL: "C.UTF-8", + }, + cwd: "/tmp", + stdio: ["ignore", "pipe", "ignore"], + timeout: 10_000, + }) + + expect(result.status).toBe(0) + const entries = result.stdout.toString("utf8").trim().split(":") + expect(entries[0]).toBe(path.join(venv, "bin")) + // macOS /etc/profile (path_helper) demotes the venv bin even when rc files + // leave PATH alone, so the guard re-prepends and an inert second copy + // remains mid-PATH. Elsewhere the untouched PATH keeps it first and the + // guard skips the prepend entirely. + if (process.platform !== "darwin") { + expect(entries.filter((entry) => entry === path.join(venv, "bin"))).toHaveLength(1) + } + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + test("does not duplicate venv bin already first in PATH in fish -c", () => { + if (process.platform === "win32") return + const fish = which("fish") + if (!fish) return + + const tmp = mkdtempSync("/tmp/opencode-test-fish-dup-") + const home = path.join(tmp, "home") + const venv = path.join(tmp, "venv") + try { + mkdirSync(path.join(venv, "bin"), { recursive: true }) + mkdirSync(home, { recursive: true }) + + const args = Shell.args(fish, "string join : $PATH", "/tmp") + const result = spawnSync(fish, args, { + env: { + HOME: home, + VIRTUAL_ENV: venv, + PATH: `${venv}/bin:/usr/bin:/bin`, + LC_ALL: "C.UTF-8", + }, + cwd: "/tmp", + stdio: ["ignore", "pipe", "ignore"], + timeout: 10_000, + }) + + expect(result.status).toBe(0) + const entries = result.stdout.toString("utf8").trim().split(":") + expect(entries[0]).toBe(path.join(venv, "bin")) + expect(entries.filter((entry) => entry === path.join(venv, "bin"))).toHaveLength(1) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + test("fish args includes venv prelude", () => { + if (process.platform === "win32") return + const fish = which("fish") + if (!fish) return + + const args = Shell.args(fish, "command -v python", "/tmp") + expect(args[0]).toBe("-c") + expect(args[1]).toContain("set -q VIRTUAL_ENV") + expect(args[1]).toContain("set -q CONDA_PREFIX") + }) + test("detects login shells", () => { expect(Shell.login("/bin/bash")).toBe(true) expect(Shell.login("C:/tools/pwsh.exe")).toBe(false) @@ -56,7 +282,11 @@ describe("shell", () => { test("builds command args per shell family", () => { expect(Shell.args("/bin/sh", "echo hi", "/tmp")).toEqual(["-c", "echo hi"]) - expect(Shell.args("/usr/bin/fish", "echo hi", "/tmp")).toEqual(["-c", "echo hi"]) + expect(Shell.args("/bin/nu", "echo hi", "/tmp")).toEqual(["-c", "echo hi"]) + const fish = Shell.args("/usr/bin/fish", "echo hi", "/tmp") + expect(fish[0]).toBe("-c") + expect(fish[1]).toContain("set -q VIRTUAL_ENV") + expect(fish[1]).toContain("set -q CONDA_PREFIX") const zsh = Shell.args("/bin/zsh", "echo hi", "/tmp") expect(zsh[0]).toBe("-l") expect(zsh[1]).toBe("-c") diff --git a/packages/desktop/src/main/server.ts b/packages/desktop/src/main/server.ts index 0f2d9d6ad120..4986b506f178 100644 --- a/packages/desktop/src/main/server.ts +++ b/packages/desktop/src/main/server.ts @@ -3,7 +3,7 @@ import { fileURLToPath } from "node:url" import { app, utilityProcess } from "electron" import type { Details } from "electron" import { getLogger } from "./logging" -import { getUserShell, loadShellEnv } from "./shell-env" +import { getUserShell, loadShellEnv, restoreVirtualEnv } from "./shell-env" import { getStore } from "./store" import { DEFAULT_SERVER_URL_KEY } from "./store-keys" @@ -43,13 +43,19 @@ export function setDefaultServerUrl(url: string | null) { export function preferAppEnv(userDataPath: string) { const shell = process.platform === "win32" ? null : getUserShell() + const venv = process.env.VIRTUAL_ENV + const conda = process.env.CONDA_PREFIX + const shellEnv = shell ? loadShellEnv(shell, getLogger()) : null Object.assign(process.env, { - ...(shell ? loadShellEnv(shell, getLogger()) : null), + ...shellEnv, OPENCODE_EXPERIMENTAL_ICON_DISCOVERY: "true", OPENCODE_EXPERIMENTAL_FILEWATCHER: "true", OPENCODE_CLIENT: "desktop", XDG_STATE_HOME: process.env.XDG_STATE_HOME ?? userDataPath, }) + if (shellEnv && (venv || conda)) { + restoreVirtualEnv(process.env, { VIRTUAL_ENV: venv, CONDA_PREFIX: conda }) + } } export async function spawnLocalServer( diff --git a/packages/desktop/src/main/shell-env.test.ts b/packages/desktop/src/main/shell-env.test.ts index e71708ad0491..dd3857db6d18 100644 --- a/packages/desktop/src/main/shell-env.test.ts +++ b/packages/desktop/src/main/shell-env.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test" -import { isNushell, mergeShellEnv, parseShellEnv, resolveUserShell } from "./shell-env" +import { isNushell, mergeShellEnv, parseShellEnv, resolveUserShell, restoreVirtualEnv } from "./shell-env" describe("shell env", () => { test("parseShellEnv supports null-delimited pairs", () => { @@ -47,4 +47,54 @@ describe("shell env", () => { expect(isNushell("C:\\Program Files\\nu.exe")).toBe(true) expect(isNushell("/bin/zsh")).toBe(false) }) + + test("restoreVirtualEnv prepends venv bin to PATH", () => { + const env: Record = { PATH: "/usr/bin:/bin" } + restoreVirtualEnv(env, { VIRTUAL_ENV: "/tmp/venv", CONDA_PREFIX: undefined }) + expect(env.VIRTUAL_ENV).toBe("/tmp/venv") + expect(env.PATH).toBe("/tmp/venv/bin:/usr/bin:/bin") + }) + + test("restoreVirtualEnv prepends conda bin when conda prefix set", () => { + const env: Record = { PATH: "/usr/bin:/bin" } + restoreVirtualEnv(env, { VIRTUAL_ENV: undefined, CONDA_PREFIX: "/tmp/conda" }) + expect(env.CONDA_PREFIX).toBe("/tmp/conda") + expect(env.PATH).toBe("/tmp/conda/bin:/usr/bin:/bin") + }) + + test("restoreVirtualEnv orders conda before venv in PATH", () => { + const env: Record = { PATH: "/usr/bin:/bin" } + restoreVirtualEnv(env, { VIRTUAL_ENV: "/tmp/venv", CONDA_PREFIX: "/tmp/conda" }) + expect(env.CONDA_PREFIX).toBe("/tmp/conda") + expect(env.VIRTUAL_ENV).toBe("/tmp/venv") + // conda prepended first, venv prepended second → venv wins at front + expect(env.PATH).toBe("/tmp/venv/bin:/tmp/conda/bin:/usr/bin:/bin") + }) + + test("restoreVirtualEnv no-op when neither venv nor conda set", () => { + const env: Record = { PATH: "/usr/bin:/bin" } + restoreVirtualEnv(env, { VIRTUAL_ENV: undefined, CONDA_PREFIX: undefined }) + expect(env.PATH).toBe("/usr/bin:/bin") + expect(env.VIRTUAL_ENV).toBeUndefined() + expect(env.CONDA_PREFIX).toBeUndefined() + }) + + test("restoreVirtualEnv sets PATH when no PATH existed", () => { + const env: Record = {} + restoreVirtualEnv(env, { VIRTUAL_ENV: "/tmp/venv", CONDA_PREFIX: undefined }) + expect(env.VIRTUAL_ENV).toBe("/tmp/venv") + expect(env.PATH).toBe("/tmp/venv/bin") + }) + + test("restoreVirtualEnv does not duplicate bin already first in PATH", () => { + const env: Record = { PATH: "/tmp/venv/bin:/usr/bin:/bin" } + restoreVirtualEnv(env, { VIRTUAL_ENV: "/tmp/venv", CONDA_PREFIX: undefined }) + expect(env.PATH).toBe("/tmp/venv/bin:/usr/bin:/bin") + }) + + test("restoreVirtualEnv re-prepends bin demoted from first in PATH", () => { + const env: Record = { PATH: "/usr/bin:/tmp/venv/bin:/bin" } + restoreVirtualEnv(env, { VIRTUAL_ENV: "/tmp/venv", CONDA_PREFIX: undefined }) + expect(env.PATH).toBe("/tmp/venv/bin:/usr/bin:/tmp/venv/bin:/bin") + }) }) diff --git a/packages/desktop/src/main/shell-env.ts b/packages/desktop/src/main/shell-env.ts index 082ed5e930db..fc374a87942c 100644 --- a/packages/desktop/src/main/shell-env.ts +++ b/packages/desktop/src/main/shell-env.ts @@ -1,6 +1,6 @@ import { spawnSync } from "node:child_process" import { userInfo } from "node:os" -import { basename } from "node:path" +import { basename, delimiter, join } from "node:path" const TIMEOUT = 5_000 @@ -99,3 +99,27 @@ export function mergeShellEnv(shell: Record | null, env: Record< ...env, } } + +export function restoreVirtualEnv( + env: Record, + parent: { VIRTUAL_ENV?: string; CONDA_PREFIX?: string }, +) { + const conda = parent.CONDA_PREFIX + if (conda) { + env.CONDA_PREFIX = conda + env.PATH = prependFirst(join(conda, "bin"), env.PATH) + } + const venv = parent.VIRTUAL_ENV + if (venv) { + env.VIRTUAL_ENV = venv + env.PATH = prependFirst(join(venv, "bin"), env.PATH) + } +} + +// Prepend only when the dir is not already first: a containment check would +// wrongly skip the restore when the probed env demoted the dir from the front. +function prependFirst(dir: string, path: string | undefined) { + if (!path) return dir + if (path === dir || path.startsWith(`${dir}${delimiter}`)) return path + return `${dir}${delimiter}${path}` +}