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.

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<string, Shell>();
 const brokenShellSessions = new Set<string>();
 const shellSessionQuarantines = new Map<string, Promise<unknown>>();
@@ -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)

packages/coding-agent/src/exec/direnv.ts

@@ -72,27 +72,40 @@ async function runDirenv(
 	cwd: string,
 	timeoutMs: number,
 	env: Record<string, string>,
+	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<Uint8Array>).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<Uint8Array>).text(),
+		new Response(proc.stderr as ReadableStream<Uint8Array>).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<string, Record<string, string>>();
+const exportCache = new Map<string, DirenvExportDiff>();
 
 /**
  * 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<string, Record<string, string>>();
  */
 export async function loadDirenvEnv(
 	cwd: string,
-	opts?: { timeoutMs?: number },
-): Promise<Record<string, string> | null> {
+	opts?: { timeoutMs?: number; signal?: AbortSignal },
+): Promise<DirenvExportDiff | null> {
 	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;

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<string, string | undefined> = {};
+	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<string, string | undefined> = {};
+	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;
+		}
+	});
 });