From 5ea0cc84198c1638ed885b142d023173acbd92f6 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Wed, 8 Jul 2026 22:37:18 -0400 Subject: [PATCH] fix(direnv): drain stderr, honor cancellation, and apply unsets in bash direnv preflight Drain stdout and stderr concurrently so a cold .envrc/devenv load can't fill the stderr pipe and block until the timeout cap. Thread the caller's abort signal and per-call timeout into the direnv preflight (AbortSignal.any + min timeout) so an aborted or short-timeout bash call returns promptly. Return the full direnv export diff and honor variable *removals*: the per-command env overlay can only add/override, so prepend a shell-level 'unset -v' for direnv's unset list, gated by a POSIX-identifier regex and skipped when the caller re-supplied the var. Tests: skip the real-direnv cases when direnv is absent, and isolate HOME/XDG so 'direnv allow' never writes into the developer's global store; add unset coverage. --- .../coding-agent/src/exec/bash-executor.ts | 27 +++++- packages/coding-agent/src/exec/direnv.ts | 37 +++++--- packages/coding-agent/test/direnv.test.ts | 92 +++++++++++++++++-- 3 files changed, 131 insertions(+), 25 deletions(-) diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index e660c9e36..4f1708393 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -55,6 +55,11 @@ export interface BashResult { workingDir?: string; } +/** POSIX-safe variable name — gates which direnv unsets we inject into the + * command line, so a hostile `.envrc` can't smuggle shell syntax through + * `unset`. `.envrc` never produces non-identifier names in practice. */ +const SAFE_ENV_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; + const shellSessions = new Map(); const brokenShellSessions = new Set(); const shellSessionQuarantines = new Map>(); @@ -218,16 +223,30 @@ export async function executeBash(command: string, options?: BashExecutorOptions const commandCwd = resolveShellCwd(options?.cwd); // Load the repo's direnv/devenv env (cached per .envrc) so devenv tools land // on PATH; the caller's explicit `env` still wins over direnv-provided values. - const direnvEnv = + // Thread the caller's signal + timeout so an aborted / short-timeout call + // can't hang on a cold `.envrc` load before the abort listener is installed. + const direnvDiff = settings.get("bash.direnv") === "off" ? null : await loadDirenvEnv(commandCwd ?? process.cwd(), { - timeoutMs: settings.get("bash.direnvLoadTimeoutMs"), + timeoutMs: Math.min( + settings.get("bash.direnvLoadTimeoutMs"), + options?.timeout ?? Number.POSITIVE_INFINITY, + ), + signal: options?.signal, }); - const commandEnv = buildNonInteractiveEnv(direnvEnv ? { ...direnvEnv, ...options?.env } : options?.env); + const commandEnv = buildNonInteractiveEnv(direnvDiff ? { ...direnvDiff.set, ...options?.env } : options?.env); + // direnv can also *remove* inherited variables (a `.envrc` doing + // `unset AWS_PROFILE`). The per-command env overlay can only add/override, so + // prepend a real `unset` for those — unless the caller re-supplied the same + // var explicitly, in which case the caller wins. + const direnvUnsets = direnvDiff + ? direnvDiff.unset.filter(name => !(options?.env && name in options.env) && SAFE_ENV_NAME.test(name)) + : []; + const unsetPrefix = direnvUnsets.length > 0 ? `unset -v ${direnvUnsets.join(" ")}; ` : ""; // Apply command prefix if configured - const prefixedCommand = prefix ? `${prefix} ${command}` : command; + const prefixedCommand = `${unsetPrefix}${prefix ? `${prefix} ${command}` : command}`; const finalCommand = options?.useUserShell === true && !bashShell ? buildUserShellCommand(shell, args, prefixedCommand) diff --git a/packages/coding-agent/src/exec/direnv.ts b/packages/coding-agent/src/exec/direnv.ts index 07c941cbc..5631079d2 100644 --- a/packages/coding-agent/src/exec/direnv.ts +++ b/packages/coding-agent/src/exec/direnv.ts @@ -72,27 +72,40 @@ async function runDirenv( cwd: string, timeoutMs: number, env: Record, + signal?: AbortSignal, ): Promise<{ exitCode: number; stdout: string }> { + // Bail on the caller's cancellation as well as the per-invocation cap so a + // cold `.envrc` load can't outlive an aborted / short-timeout bash call. + const abortSignal = signal + ? AbortSignal.any([signal, AbortSignal.timeout(timeoutMs)]) + : AbortSignal.timeout(timeoutMs); const proc = Bun.spawn([bin, ...args], { cwd, env, stdout: "pipe", stderr: "pipe", - signal: AbortSignal.timeout(timeoutMs), + signal: abortSignal, }); - const stdout = await new Response(proc.stdout as ReadableStream).text(); + // Drain stdout AND stderr concurrently: a cold `use devenv`/Nix load emits + // enough diagnostics to fill the stderr pipe and block the child forever if + // only stdout is read (it would then wait out `timeoutMs`). + const [stdout] = await Promise.all([ + new Response(proc.stdout as ReadableStream).text(), + new Response(proc.stderr as ReadableStream).text(), + ]); const exitCode = await proc.exited; return { exitCode, stdout }; } /** Cache the parsed env per resolved `.envrc` + content hash, so the (possibly * slow) first export is paid once and a changed `.envrc` re-loads. */ -const exportCache = new Map>(); +const exportCache = new Map(); /** * Resolve the nearest `.envrc` from `cwd`, auto-allow it, and return its - * `direnv export` environment (set values only). Returns `null` when there is - * no `.envrc`, `direnv` is not installed, or the export fails/times out. + * `direnv export` diff (variables to set, and variables direnv removes). + * Returns `null` when there is no `.envrc`, `direnv` is not installed, or the + * export fails/times out. * * Auto-allow is deliberate: OMP already runs the repository's own code, so its * `.envrc` is trusted under the same model rather than forcing a manual @@ -100,8 +113,8 @@ const exportCache = new Map>(); */ export async function loadDirenvEnv( cwd: string, - opts?: { timeoutMs?: number }, -): Promise | null> { + opts?: { timeoutMs?: number; signal?: AbortSignal }, +): Promise { const envrcPath = await findEnvrc(cwd); if (!envrcPath) return null; const bin = direnvBinary(); @@ -121,15 +134,15 @@ export async function loadDirenvEnv( const timeoutMs = opts?.timeoutMs ?? DEFAULT_DIRENV_TIMEOUT_MS; const env = cleanSpawnEnv(); try { - await runDirenv(bin, ["allow"], dir, timeoutMs, env); - const { exitCode, stdout } = await runDirenv(bin, ["export", "json"], dir, timeoutMs, env); + await runDirenv(bin, ["allow"], dir, timeoutMs, env, opts?.signal); + const { exitCode, stdout } = await runDirenv(bin, ["export", "json"], dir, timeoutMs, env, opts?.signal); if (exitCode !== 0) { logger.warn("direnv export failed", { dir, exitCode }); return null; } - const { set } = parseDirenvExport(stdout); - exportCache.set(cacheKey, set); - return set; + const diff = parseDirenvExport(stdout); + exportCache.set(cacheKey, diff); + return diff; } catch (err) { logger.warn("direnv load failed", { dir, error: err instanceof Error ? err.message : String(err) }); return null; diff --git a/packages/coding-agent/test/direnv.test.ts b/packages/coding-agent/test/direnv.test.ts index bbaa4c29a..5997a8979 100644 --- a/packages/coding-agent/test/direnv.test.ts +++ b/packages/coding-agent/test/direnv.test.ts @@ -1,9 +1,13 @@ -import { afterEach, describe, expect, it } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs/promises"; import * as path from "node:path"; import { executeBash } from "@oh-my-pi/pi-coding-agent/exec/bash-executor"; import { findEnvrc, loadDirenvEnv, parseDirenvExport } from "@oh-my-pi/pi-coding-agent/exec/direnv"; -import { TempDir } from "@oh-my-pi/pi-utils"; +import { $which, TempDir } from "@oh-my-pi/pi-utils"; + +/** Real-direnv cases need the binary on PATH; skip cleanly when it's absent so + * the graceful-degradation code path (returns `null`) isn't asserted against. */ +const hasDirenv = $which("direnv") !== null; const tmpDirs: TempDir[] = []; function tmp(): string { @@ -58,16 +62,50 @@ describe("parseDirenvExport", () => { }); }); -describe("loadDirenvEnv (real direnv, auto-allow)", () => { +describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { + // `direnv allow` writes a trust entry into its data dir. Redirect HOME + the + // XDG dirs to a throwaway tmp so real-direnv cases never leak allow state + // into the dev/CI user's global direnv store. + const savedEnv: Record = {}; + beforeEach(() => { + const home = tmp(); + for (const key of ["HOME", "XDG_DATA_HOME", "XDG_CONFIG_HOME", "XDG_CACHE_HOME"]) { + savedEnv[key] = Bun.env[key]; + Bun.env[key] = path.join(home, key.toLowerCase()); + } + }); + afterEach(() => { + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete Bun.env[key]; + else Bun.env[key] = value; + } + }); + it("auto-allows an untrusted .envrc and returns its exported vars + PATH additions", async () => { const root = tmp(); await fs.mkdir(path.join(root, "bin"), { recursive: true }); await Bun.write(path.join(root, ".envrc"), "export DIRENV_FEATURE_TEST=loaded\nPATH_add bin\n"); - const env = await loadDirenvEnv(root); + const diff = await loadDirenvEnv(root); - expect(env?.DIRENV_FEATURE_TEST).toBe("loaded"); - expect(env?.PATH).toContain(path.join(root, "bin")); + expect(diff?.set.DIRENV_FEATURE_TEST).toBe("loaded"); + expect(diff?.set.PATH).toContain(path.join(root, "bin")); + }); + + it("reports variables a .envrc unsets", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "unset PI_DIRENV_UNSET_TEST\n"); + // direnv emits a JSON null for a var only when it was present in the + // spawn env and the `.envrc` removes it, so seed it in the parent env. + // (Avoid a `DIRENV_`-prefixed name — the loader strips those before spawn.) + Bun.env.PI_DIRENV_UNSET_TEST = "present"; + try { + const diff = await loadDirenvEnv(root); + expect(diff?.set.PI_DIRENV_UNSET_TEST).toBeUndefined(); + expect(diff?.unset).toContain("PI_DIRENV_UNSET_TEST"); + } finally { + delete Bun.env.PI_DIRENV_UNSET_TEST; + } }); it("returns null when there is no .envrc to load", async () => { @@ -77,14 +115,29 @@ describe("loadDirenvEnv (real direnv, auto-allow)", () => { it("re-loads when the .envrc content changes (cache keyed by content)", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=one\n"); - expect((await loadDirenvEnv(root))?.DIRENV_CACHE_TEST).toBe("one"); + expect((await loadDirenvEnv(root))?.set.DIRENV_CACHE_TEST).toBe("one"); await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=two\n"); - expect((await loadDirenvEnv(root))?.DIRENV_CACHE_TEST).toBe("two"); + expect((await loadDirenvEnv(root))?.set.DIRENV_CACHE_TEST).toBe("two"); }); }); -describe("bash executor direnv wiring (end-to-end)", () => { +describe.skipIf(!hasDirenv)("bash executor direnv wiring (end-to-end)", () => { + const savedEnv: Record = {}; + beforeEach(() => { + const home = tmp(); + for (const key of ["HOME", "XDG_DATA_HOME", "XDG_CONFIG_HOME", "XDG_CACHE_HOME"]) { + savedEnv[key] = Bun.env[key]; + Bun.env[key] = path.join(home, key.toLowerCase()); + } + }); + afterEach(() => { + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete Bun.env[key]; + else Bun.env[key] = value; + } + }); + it("exposes direnv-loaded vars to the command while per-call env still wins", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "export DIRENV_WIRE_TEST=fromdirenv\nexport OVERRIDE_ME=fromdirenv\n"); @@ -96,4 +149,25 @@ describe("bash executor direnv wiring (end-to-end)", () => { expect(result.output).toContain("fromdirenv|fromcaller"); }); + + it("removes variables the .envrc unsets from the command environment", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "unset PI_DIRENV_UNSET_E2E\n"); + // Inherited from the process env (as an OMP-provided var would be); the + // caller does NOT re-supply it, so direnv's unset must strip it. `printenv` + // exits non-zero and prints nothing when the name is genuinely absent. A + // unique sessionKey forces a fresh shell that captures the var we just set. + // (Avoid a `DIRENV_`-prefixed name — the loader strips those before spawn.) + Bun.env.PI_DIRENV_UNSET_E2E = "leaked"; + try { + const result = await executeBash('printenv PI_DIRENV_UNSET_E2E; printf "rc=%s" "$?"', { + cwd: root, + sessionKey: `direnv-unset-${Date.now()}`, + }); + expect(result.output).toContain("rc=1"); + expect(result.output).not.toContain("leaked"); + } finally { + delete Bun.env.PI_DIRENV_UNSET_E2E; + } + }); });