From 47493b0742791f3c5fdfbc974e1c0423510c62bf Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Tue, 30 Jun 2026 17:05:14 -0400 Subject: [PATCH 1/4] feat(coding-agent): auto-load direnv environment into the bash session The bash tool's persistent shell didn't carry a repo's direnv/devenv environment, so devenv-provided tools (moon, project-pinned biome/bun, toolchains) were off PATH and .envrc-set vars (e.g. GIT_DIR for a jj secondary workspace) were missing. Resolve the nearest .envrc from the run cwd, load its env via direnv export json, and merge it under the caller's per-call env. Gated by bash.direnv (default auto, auto-allows). --- .../src/config/settings-schema.ts | 23 +++ .../coding-agent/src/exec/bash-executor.ts | 11 +- packages/coding-agent/src/exec/direnv.ts | 137 ++++++++++++++++++ packages/coding-agent/test/direnv.test.ts | 99 +++++++++++++ 4 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 packages/coding-agent/src/exec/direnv.ts create mode 100644 packages/coding-agent/test/direnv.test.ts diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index c632057bf..50092dd86 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -3181,6 +3181,29 @@ export const SETTINGS_SCHEMA = { }, "bashInterceptor.patterns": { type: "array", default: DEFAULT_BASH_INTERCEPTOR_RULES }, + "bash.direnv": { + type: "enum", + values: ["auto", "off"] as const, + default: "auto", + ui: { + tab: "shell", + group: "Bash", + label: "direnv Auto-Load", + description: + "Auto-load (and auto-allow) a repo's direnv/devenv `.envrc` into the bash session so devenv tools and env vars are present without manual `direnv exec`", + }, + }, + "bash.direnvLoadTimeoutMs": { + type: "number", + default: 30_000, + ui: { + tab: "shell", + group: "Bash", + label: "direnv Load Timeout (ms)", + description: + "Max wait for the first `direnv export` (a cold devenv shell can be slow); on timeout the session runs without the direnv env", + }, + }, // Shell output minimizer "shellMinimizer.enabled": { type: "boolean", diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index fc7d7dc69..e660c9e36 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -10,6 +10,7 @@ import { Settings, type ShellMinimizerSettings } from "../config/settings"; import { OutputSink } from "../session/streaming-output"; import { resolveOutputMaxColumns, resolveOutputSinkHeadBytes } from "../tools/output-meta"; import { getOrCreateSnapshot } from "../utils/shell-snapshot"; +import { loadDirenvEnv } from "./direnv"; import { buildNonInteractiveEnv } from "./non-interactive-env"; export interface BashExecutorOptions { @@ -215,7 +216,15 @@ export async function executeBash(command: string, options?: BashExecutorOptions const minimizer = buildMinimizerOptions(settings.getGroup("shellMinimizer")); const commandCwd = resolveShellCwd(options?.cwd); - const commandEnv = buildNonInteractiveEnv(options?.env); + // 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 = + settings.get("bash.direnv") === "off" + ? null + : await loadDirenvEnv(commandCwd ?? process.cwd(), { + timeoutMs: settings.get("bash.direnvLoadTimeoutMs"), + }); + const commandEnv = buildNonInteractiveEnv(direnvEnv ? { ...direnvEnv, ...options?.env } : options?.env); // Apply command prefix if configured const prefixedCommand = prefix ? `${prefix} ${command}` : command; diff --git a/packages/coding-agent/src/exec/direnv.ts b/packages/coding-agent/src/exec/direnv.ts new file mode 100644 index 000000000..07c941cbc --- /dev/null +++ b/packages/coding-agent/src/exec/direnv.ts @@ -0,0 +1,137 @@ +import * as fs from "node:fs/promises"; +import * as path from "node:path"; +import { $which, logger } from "@oh-my-pi/pi-utils"; + +/** Default cap on a single `direnv` invocation. The first export for a devenv + * `.envrc` can build a shell; callers may raise this via `bash.direnvLoadTimeoutMs`. */ +export const DEFAULT_DIRENV_TIMEOUT_MS = 30_000; + +/** Walk up from `startDir` to the nearest directory containing an `.envrc`. */ +export async function findEnvrc(startDir: string): Promise { + let dir = path.resolve(startDir); + for (;;) { + const candidate = path.join(dir, ".envrc"); + try { + if ((await fs.stat(candidate)).isFile()) return candidate; + } catch { + // no .envrc here — keep walking up + } + const parent = path.dirname(dir); + if (parent === dir) return null; + dir = parent; + } +} + +export interface DirenvExportDiff { + /** Variables direnv sets to a concrete value. */ + set: Record; + /** Variables direnv removes (JSON `null`). */ + unset: string[]; +} + +/** Parse `direnv export json` output (`{VAR: value|null}`) into set/unset halves. */ +export function parseDirenvExport(jsonText: string): DirenvExportDiff { + const trimmed = jsonText.trim(); + if (trimmed.length === 0) return { set: {}, unset: [] }; + let parsed: unknown; + try { + parsed = JSON.parse(trimmed); + } catch { + return { set: {}, unset: [] }; + } + const set: Record = {}; + const unset: string[] = []; + if (parsed && typeof parsed === "object") { + for (const [key, value] of Object.entries(parsed as Record)) { + if (value === null) unset.push(key); + else if (typeof value === "string") set[key] = value; + } + } + return { set, unset }; +} + +let direnvLookup: { bin: string | null } | undefined; +function direnvBinary(): string | null { + if (!direnvLookup) direnvLookup = { bin: $which("direnv") }; + return direnvLookup.bin; +} + +/** direnv computes its diff relative to the spawning env; strip any inherited + * direnv state so it loads the target `.envrc` from a clean baseline. */ +function cleanSpawnEnv(): Record { + const out: Record = {}; + for (const [key, value] of Object.entries(Bun.env)) { + if (value !== undefined && !key.startsWith("DIRENV_")) out[key] = value; + } + return out; +} + +async function runDirenv( + bin: string, + args: string[], + cwd: string, + timeoutMs: number, + env: Record, +): Promise<{ exitCode: number; stdout: string }> { + const proc = Bun.spawn([bin, ...args], { + cwd, + env, + stdout: "pipe", + stderr: "pipe", + signal: AbortSignal.timeout(timeoutMs), + }); + const stdout = await new Response(proc.stdout 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>(); + +/** + * 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. + * + * 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 + * `direnv allow`. + */ +export async function loadDirenvEnv( + cwd: string, + opts?: { timeoutMs?: number }, +): Promise | null> { + const envrcPath = await findEnvrc(cwd); + if (!envrcPath) return null; + const bin = direnvBinary(); + if (!bin) return null; + + let cacheKey: string; + try { + const content = await fs.readFile(envrcPath); + cacheKey = `${envrcPath}\u0000${Bun.hash(content).toString(36)}`; + } catch { + return null; + } + const cached = exportCache.get(cacheKey); + if (cached) return cached; + + const dir = path.dirname(envrcPath); + 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); + if (exitCode !== 0) { + logger.warn("direnv export failed", { dir, exitCode }); + return null; + } + const { set } = parseDirenvExport(stdout); + exportCache.set(cacheKey, set); + return set; + } 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 new file mode 100644 index 000000000..bbaa4c29a --- /dev/null +++ b/packages/coding-agent/test/direnv.test.ts @@ -0,0 +1,99 @@ +import { afterEach, 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"; + +const tmpDirs: TempDir[] = []; +function tmp(): string { + const dir = TempDir.createSync("@pi-direnv-"); + tmpDirs.push(dir); + return dir.path(); +} + +afterEach(async () => { + for (const dir of tmpDirs.splice(0)) await dir.remove(); +}); + +describe("findEnvrc", () => { + it("walks up to the nearest .envrc above the start dir", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "export A=1\n"); + const nested = path.join(root, "a", "b"); + await fs.mkdir(nested, { recursive: true }); + + expect(await findEnvrc(nested)).toBe(path.join(root, ".envrc")); + }); + + it("prefers the nearest .envrc when monorepo dirs nest them", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "export A=1\n"); + const sub = path.join(root, "pkg"); + await fs.mkdir(sub, { recursive: true }); + await Bun.write(path.join(sub, ".envrc"), "export B=2\n"); + + expect(await findEnvrc(sub)).toBe(path.join(sub, ".envrc")); + }); + + it("returns null when no .envrc exists up the tree", async () => { + const nested = path.join(tmp(), "x", "y"); + await fs.mkdir(nested, { recursive: true }); + + expect(await findEnvrc(nested)).toBeNull(); + }); +}); + +describe("parseDirenvExport", () => { + it("splits set values from null unsets", () => { + const out = parseDirenvExport('{"FOO":"bar","BAZ":null,"PATH":"/x:/y"}'); + + expect(out.set).toEqual({ FOO: "bar", PATH: "/x:/y" }); + expect(out.unset).toEqual(["BAZ"]); + }); + + it("treats empty / whitespace output as no diff", () => { + expect(parseDirenvExport("")).toEqual({ set: {}, unset: [] }); + expect(parseDirenvExport(" \n")).toEqual({ set: {}, unset: [] }); + }); +}); + +describe("loadDirenvEnv (real direnv, auto-allow)", () => { + 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); + + expect(env?.DIRENV_FEATURE_TEST).toBe("loaded"); + expect(env?.PATH).toContain(path.join(root, "bin")); + }); + + it("returns null when there is no .envrc to load", async () => { + expect(await loadDirenvEnv(tmp())).toBeNull(); + }); + + 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"); + + await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=two\n"); + expect((await loadDirenvEnv(root))?.DIRENV_CACHE_TEST).toBe("two"); + }); +}); + +describe("bash executor direnv wiring (end-to-end)", () => { + 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"); + + const result = await executeBash('printf "%s|%s" "$DIRENV_WIRE_TEST" "$OVERRIDE_ME"', { + cwd: root, + env: { OVERRIDE_ME: "fromcaller" }, + }); + + expect(result.output).toContain("fromdirenv|fromcaller"); + }); +}); From 5ea0cc84198c1638ed885b142d023173acbd92f6 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Wed, 8 Jul 2026 22:37:18 -0400 Subject: [PATCH 2/4] 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; + } + }); }); From 5941d797bae867139b330f1cf12d660567899875 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Wed, 8 Jul 2026 23:46:14 -0400 Subject: [PATCH 3/4] feat(direnv): apply devenv env across all bash backends + always revalidate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract applyDirenvPreflight() so the ACP client terminal and PTY backends get the same direnv/devenv overlay as executeBash (previously only the one-shot path did). The helper is a pure (command, env) transform — merge direnv's set under the caller's overlay, prepend a regex-gated unset -v for removed vars — so interactive backends keep their own env shape (live TERM) while executeBash still layers its non-interactive defaults on top. The three dispatch branches are mutually exclusive, so no command is preflighted twice. Also drop the content-hash export cache in loadDirenvEnv: always run direnv export json and let direnv's own watch/mtime invalidation decide freshness, so a changed watched file re-exports even when .envrc is unchanged. Co-Authored-By: seal --- packages/coding-agent/CHANGELOG.md | 3 + .../coding-agent/src/exec/bash-executor.ts | 104 +++++++++++---- packages/coding-agent/src/exec/direnv.ts | 23 +--- packages/coding-agent/src/tools/bash.ts | 42 +++++- .../coding-agent/test/bash-executor.test.ts | 32 +++++ packages/coding-agent/test/direnv.test.ts | 120 +++++++++++++++++- 6 files changed, 270 insertions(+), 54 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f9b8bb0ef..479ad3edc 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -201,6 +201,9 @@ - Fixed ACP `terminal/create` sending the bash tool's full shell line in `command` with no `args`, which broke spec-conformant clients that spawn `command`+`args` directly (no implicit shell) — any command containing a space, pipe, `&&`, redirect, or `$(...)` failed with `ENOENT` and the agent silently degraded to read-only tools. The bash tool now wraps the shell line before calling `clientBridge.createTerminal`, reusing the same shell binary + args the local `bash-executor` resolves via `settings.getShellConfig()` (Git Bash / `bash.exe` on Windows, `$SHELL` with `sh` fallback on POSIX) so bash semantics — `$VAR`, `$(...)`, `source`, POSIX quoting, `-l` — are preserved on both platforms. ([#4333](https://github.com/can1357/oh-my-pi/issues/4333)) - Fixed inference worker subprocesses (TTS, STT, tiny-model, mnemopi embeddings) discarding stderr, which left every unexpected exit — most visibly the local Kokoro TTS worker's recurring `exit code 7` crash loop — undiagnosable from the parent's logs. `createWorkerSubprocess` now pipes stderr without starting a live read while the worker is idle, then drains the stream after `onExit`, emits captured lines to `logger.debug` under an ` stderr` message, and keeps the last 16 KiB in a bounded ring that gets appended to the `Error` surfaced through `onError`. The exit surface is synchronized with the post-exit drain via `SpawnedSubprocess.stderrDrained`, so the full native trace shows up on the `tts: worker error` line without reintroducing event-loop liveness from unref'd workers. ([#4324](https://github.com/can1357/oh-my-pi/issues/4324)) - Fixed Windows session tail loss after atomic compaction rewrites by fencing append writers during full-file replacement and gating the atomic publish on a `commitGuard` that the storage backend checks synchronously before rename, so a concurrent `flushSync` (Ctrl+C / session-exit) is not overwritten by the stale body serialized before it ran. Covers post-compaction prompts, tool results, title changes, and exit diagnostics on the current JSONL path ([#4338](https://github.com/can1357/oh-my-pi/issues/4338)). +### Fixed + +- Extended the bash tool's direnv/devenv auto-loading to every backend: the ACP client terminal and the interactive PTY now receive the repo's direnv environment (variables set, and `unset -v` for variables the `.envrc` removes) — previously only the one-shot `executeBash` path did — via a shared preflight so all backends behave identically, with the caller's explicit env still winning. direnv loading also always re-runs `direnv export json` instead of serving a content-hashed cache, so a change to a `watch_file` target re-exports even when the `.envrc` text is unchanged (direnv's own watch invalidation is authoritative). ([#4455](https://github.com/can1357/oh-my-pi/issues/4455)) ## [16.3.4] - 2026-07-03 diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index 4f1708393..32523d8d4 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -60,6 +60,58 @@ export interface BashResult { * `unset`. `.envrc` never produces non-identifier names in practice. */ const SAFE_ENV_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; +export interface DirenvPreflightOptions { + /** Caller-supplied env overlay; these values win over direnv-provided ones. */ + callerEnv?: Record; + signal?: AbortSignal; + /** Cap on the direnv load; the caller's own timeout further clamps it. */ + timeoutMs?: number; + /** `bash.direnv` setting — `"off"` skips the load entirely. */ + direnvSetting: "auto" | "off"; + /** Shell wrapper prefix (profiler/strace) to place *after* the unset prefix, + * matching `executeBash`'s ordering. Backends that apply their own shell + * wrapping (ACP `wrapShellLineForClientTerminal`) omit this. */ + commandPrefix?: string | undefined; +} + +/** + * Load the repo's direnv/devenv env and fold it into a `(command, env)` pair so + * every bash backend (one-shot `executeBash`, ACP client terminal, PTY) exposes + * the same devenv tools. Encapsulates: load the diff, merge `set` under the + * caller's overlay (caller wins), and prepend a regex-gated `unset -v` for + * variables the `.envrc` removes (skipping any the caller re-supplied). + * + * Returns the possibly-prefixed command plus the merged env, or the inputs + * unchanged (`env` = `callerEnv`) when direnv is off, absent, or has no `.envrc`. + * Pure transform: does NOT layer non-interactive env defaults — that stays the + * caller's job (so interactive PTY/ACP paths keep their own env shape). + */ +export async function applyDirenvPreflight( + command: string, + cwd: string, + opts: DirenvPreflightOptions, +): Promise<{ command: string; env: Record | undefined }> { + const withPrefix = (line: string): string => (opts.commandPrefix ? `${opts.commandPrefix} ${line}` : line); + const direnvDiff = + opts.direnvSetting === "off" + ? null + : await loadDirenvEnv(cwd, { timeoutMs: opts.timeoutMs, signal: opts.signal }); + if (!direnvDiff) { + return { command: withPrefix(command), env: opts.callerEnv }; + } + // The caller's explicit env still wins over direnv-provided values. + const mergedEnv = { ...direnvDiff.set, ...opts.callerEnv }; + // direnv can also *remove* inherited variables (a `.envrc` doing + // `unset AWS_PROFILE`). An 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.unset.filter( + name => !(opts.callerEnv && name in opts.callerEnv) && SAFE_ENV_NAME.test(name), + ); + const unsetPrefix = direnvUnsets.length > 0 ? `unset -v ${direnvUnsets.join(" ")}; ` : ""; + return { command: `${unsetPrefix}${withPrefix(command)}`, env: mergedEnv }; +} + const shellSessions = new Map(); const brokenShellSessions = new Set(); const shellSessionQuarantines = new Map>(); @@ -221,36 +273,32 @@ export async function executeBash(command: string, options?: BashExecutorOptions const minimizer = buildMinimizerOptions(settings.getGroup("shellMinimizer")); 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. - // 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: Math.min( - settings.get("bash.direnvLoadTimeoutMs"), - options?.timeout ?? Number.POSITIVE_INFINITY, - ), - signal: options?.signal, - }); - 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 = `${unsetPrefix}${prefix ? `${prefix} ${command}` : command}`; + // Fold the repo's direnv/devenv env into the command + env so devenv tools + // land on PATH; the caller's explicit `env` still wins. 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. The helper applies + // the configured shell `prefix` after any `unset -v` it prepends. + const preflight = await applyDirenvPreflight(command, commandCwd ?? process.cwd(), { + callerEnv: options?.env, + signal: options?.signal, + timeoutMs: (() => { + const direnvBudget = settings.get("bash.direnvLoadTimeoutMs"); + // A disabled command deadline (`timeout: 0`) means "no caller clamp", + // not a 0 ms direnv load — the load still gets its full budget. Only a + // positive caller timeout clamps the direnv window below that budget. + const callerDeadline = options?.timeout; + return callerDeadline !== undefined && callerDeadline > 0 + ? Math.min(direnvBudget, callerDeadline) + : direnvBudget; + })(), + direnvSetting: settings.get("bash.direnv"), + commandPrefix: prefix, + }); + const commandEnv = buildNonInteractiveEnv(preflight.env); const finalCommand = options?.useUserShell === true && !bashShell - ? buildUserShellCommand(shell, args, prefixedCommand) - : prefixedCommand; + ? buildUserShellCommand(shell, args, preflight.command) + : preflight.command; // Create output sink for truncation and artifact handling const sink = new OutputSink({ diff --git a/packages/coding-agent/src/exec/direnv.ts b/packages/coding-agent/src/exec/direnv.ts index 5631079d2..bfbb8b662 100644 --- a/packages/coding-agent/src/exec/direnv.ts +++ b/packages/coding-agent/src/exec/direnv.ts @@ -97,16 +97,17 @@ async function runDirenv( 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(); - /** * Resolve the nearest `.envrc` from `cwd`, auto-allow it, and return its * `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. * + * Always re-invokes `direnv export json` rather than serving a cached diff: + * direnv is fast once warm, and its own watch/mtime invalidation is the + * authoritative freshness signal (a content-hash cache here would go stale when + * a `watch_file` target changes without the `.envrc` text changing). + * * 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 * `direnv allow`. @@ -120,16 +121,6 @@ export async function loadDirenvEnv( const bin = direnvBinary(); if (!bin) return null; - let cacheKey: string; - try { - const content = await fs.readFile(envrcPath); - cacheKey = `${envrcPath}\u0000${Bun.hash(content).toString(36)}`; - } catch { - return null; - } - const cached = exportCache.get(cacheKey); - if (cached) return cached; - const dir = path.dirname(envrcPath); const timeoutMs = opts?.timeoutMs ?? DEFAULT_DIRENV_TIMEOUT_MS; const env = cleanSpawnEnv(); @@ -140,9 +131,7 @@ export async function loadDirenvEnv( logger.warn("direnv export failed", { dir, exitCode }); return null; } - const diff = parseDirenvExport(stdout); - exportCache.set(cacheKey, diff); - return diff; + return parseDirenvExport(stdout); } catch (err) { logger.warn("direnv load failed", { dir, error: err instanceof Error ? err.message : String(err) }); return null; diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 14e67375b..67d9e210c 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -10,7 +10,7 @@ import type { Component } from "@oh-my-pi/pi-tui"; import { ImageProtocol, TERMINAL } from "@oh-my-pi/pi-tui"; import { getProjectDir, isEnoent, logger, prompt } from "@oh-my-pi/pi-utils"; import { type } from "arktype"; -import { type BashResult, executeBash } from "../exec/bash-executor"; +import { applyDirenvPreflight, type BashResult, executeBash } from "../exec/bash-executor"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; import { InternalUrlRouter } from "../internal-urls"; import { truncateToVisualLines } from "../modes/components/visual-truncate"; @@ -882,17 +882,39 @@ export class BashTool implements AgentTool ({ name, value: value as string })) + env: bridgeEnv + ? Object.entries(bridgeEnv).map(([name, value]) => ({ name, value: value as string })) : undefined, outputByteLimit: DEFAULT_MAX_BYTES, }); @@ -1070,15 +1092,21 @@ export class BashTool implements AgentTool { expect(result.output.trim()).toBe(tempDir); }); + it("passes the full direnv-load budget when the command deadline is disabled (timeout: 0)", async () => { + // A disabled command deadline (`timeout: 0`) must NOT collapse the direnv + // export window to 0 ms — that would make AbortSignal.timeout(0) abort the + // load instantly, silently dropping the repo's direnv env. The load keeps + // its full `bash.direnvLoadTimeoutMs` budget. Spying on loadDirenvEnv both + // captures the timeoutMs and short-circuits real direnv (null diff = no-op). + const budget = (await Settings.init()).get("bash.direnvLoadTimeoutMs"); + const spy = vi.spyOn(direnvModule, "loadDirenvEnv").mockResolvedValue(null); + + await executeBash("true", { cwd: tempDir, timeout: 0 }); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]?.timeoutMs).toBe(budget); + expect(spy.mock.calls[0][1]?.timeoutMs).not.toBe(0); + }); + + it("clamps the direnv-load budget to a positive command timeout smaller than it", async () => { + // A positive caller timeout below the budget DOES clamp the direnv window, + // proving the fix only relaxes the `timeout: 0` case and did not disable + // clamping wholesale. Setting and options.timeout are both milliseconds. + const budget = (await Settings.init()).get("bash.direnvLoadTimeoutMs"); + const callerTimeout = 5; + expect(callerTimeout).toBeLessThan(budget); + const spy = vi.spyOn(direnvModule, "loadDirenvEnv").mockResolvedValue(null); + + await executeBash("true", { cwd: tempDir, timeout: callerTimeout }); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]?.timeoutMs).toBe(Math.min(budget, callerTimeout)); + }); + it("honors symlinked cwd requests in persistent shells", async () => { if (process.platform === "win32") { return; diff --git a/packages/coding-agent/test/direnv.test.ts b/packages/coding-agent/test/direnv.test.ts index 5997a8979..96cafcf70 100644 --- a/packages/coding-agent/test/direnv.test.ts +++ b/packages/coding-agent/test/direnv.test.ts @@ -1,7 +1,7 @@ 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 { applyDirenvPreflight, 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 { $which, TempDir } from "@oh-my-pi/pi-utils"; @@ -112,7 +112,7 @@ describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { expect(await loadDirenvEnv(tmp())).toBeNull(); }); - it("re-loads when the .envrc content changes (cache keyed by content)", async () => { + it("re-exports when the .envrc content changes (no stale cache)", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=one\n"); expect((await loadDirenvEnv(root))?.set.DIRENV_CACHE_TEST).toBe("one"); @@ -120,6 +120,24 @@ describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=two\n"); expect((await loadDirenvEnv(root))?.set.DIRENV_CACHE_TEST).toBe("two"); }); + + it("always re-invokes direnv so a changed watched file re-exports even when .envrc text is unchanged", async () => { + // direnv's own `watch_file` invalidation — not a content hash of the + // `.envrc` — is the freshness authority. The `.envrc` bytes never change + // here; only the watched file's contents do. A content-hash early-return + // (the old behavior) would serve the stale first value; always running + // `direnv export json` picks up the new one. + const root = tmp(); + await Bun.write(path.join(root, "watched.env"), "one\n"); + await Bun.write( + path.join(root, ".envrc"), + 'watch_file watched.env\nexport DIRENV_WATCH_TEST="$(cat watched.env)"\n', + ); + expect((await loadDirenvEnv(root))?.set.DIRENV_WATCH_TEST).toBe("one"); + + await Bun.write(path.join(root, "watched.env"), "two\n"); + expect((await loadDirenvEnv(root))?.set.DIRENV_WATCH_TEST).toBe("two"); + }); }); describe.skipIf(!hasDirenv)("bash executor direnv wiring (end-to-end)", () => { @@ -171,3 +189,101 @@ describe.skipIf(!hasDirenv)("bash executor direnv wiring (end-to-end)", () => { } }); }); + +describe.skipIf(!hasDirenv)("applyDirenvPreflight (shared all-backends preflight)", () => { + // This helper is what the ACP client-terminal and PTY backends call so they + // expose the same devenv env as `executeBash`. Assert its core contract: + // set-merge with caller-wins, the regex-gated `unset -v` prefix, and the + // off/absent passthrough. HOME/XDG isolation mirrors the loader cases so + // `direnv allow` never touches the dev/CI user's global 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("merges direnv set vars into env while the caller's env wins", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "export DIRENV_PF_SET=fromdirenv\nexport OVERRIDE_ME=fromdirenv\n"); + + const { command, env } = await applyDirenvPreflight("echo hi", root, { + callerEnv: { OVERRIDE_ME: "fromcaller" }, + direnvSetting: "auto", + }); + + expect(env?.DIRENV_PF_SET).toBe("fromdirenv"); + expect(env?.OVERRIDE_ME).toBe("fromcaller"); + // No unset in this .envrc, so the command is returned unprefixed. + expect(command).toBe("echo hi"); + }); + + it("prepends a regex-gated `unset -v` for vars the .envrc removes, skipping caller-resupplied names", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "unset PI_PF_UNSET_A\nunset PI_PF_UNSET_B\n"); + Bun.env.PI_PF_UNSET_A = "present"; + Bun.env.PI_PF_UNSET_B = "present"; + try { + const { command } = await applyDirenvPreflight("run-it", root, { + // Caller re-supplies B, so its unset must be skipped (caller wins). + callerEnv: { PI_PF_UNSET_B: "kept" }, + direnvSetting: "auto", + }); + expect(command).toContain("unset -v PI_PF_UNSET_A"); + expect(command).not.toContain("PI_PF_UNSET_B"); + expect(command.endsWith("run-it")).toBe(true); + } finally { + delete Bun.env.PI_PF_UNSET_A; + delete Bun.env.PI_PF_UNSET_B; + } + }); + + it("applies the shell commandPrefix after the unset prefix", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "unset PI_PF_ORDER\n"); + Bun.env.PI_PF_ORDER = "present"; + try { + const { command } = await applyDirenvPreflight("payload", root, { + direnvSetting: "auto", + commandPrefix: "strace -f", + }); + // Ordering must be: `unset -v NAME; `. + expect(command).toBe("unset -v PI_PF_ORDER; strace -f payload"); + } finally { + delete Bun.env.PI_PF_ORDER; + } + }); + + it("returns the command + caller env unchanged when direnv is off", async () => { + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "export DIRENV_PF_OFF=loaded\n"); + + const callerEnv = { FOO: "bar" }; + const { command, env } = await applyDirenvPreflight("noop", root, { + callerEnv, + direnvSetting: "off", + }); + + expect(command).toBe("noop"); + expect(env).toBe(callerEnv); + }); + + it("returns the command + caller env unchanged when no .envrc exists", async () => { + const callerEnv = { FOO: "bar" }; + const { command, env } = await applyDirenvPreflight("noop", tmp(), { + callerEnv, + direnvSetting: "auto", + }); + + expect(command).toBe("noop"); + expect(env).toBe(callerEnv); + }); +}); From 33318cf5e1e766fd060b3d3904e502865086c3d9 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Thu, 9 Jul 2026 01:35:17 -0400 Subject: [PATCH 4/4] fix(direnv): clamp the ACP/PTY backend preflight to the caller timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The executeBash direnv preflight clamps its load budget to a positive caller command timeout, but the PTY / ACP-terminal backend preflight in bash.ts passed the raw bash.direnvLoadTimeoutMs (30s default) with no clamp — so a short-timeout command routed through those backends could hang up to 30s on a cold `.envrc` before its own timeout is even installed. Centralize the clamp inside applyDirenvPreflight (new callerTimeoutMs option) so every backend inherits one contract: `timeout: 0`/undefined keeps the full budget, a positive deadline clamps. Co-Authored-By: seal --- .../coding-agent/src/exec/bash-executor.ts | 35 ++++++---- packages/coding-agent/src/tools/bash.ts | 4 ++ .../coding-agent/test/bash-executor.test.ts | 65 ++++++++++++++++++- 3 files changed, 89 insertions(+), 15 deletions(-) diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index 32523d8d4..dd2cd1de0 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -64,8 +64,17 @@ export interface DirenvPreflightOptions { /** Caller-supplied env overlay; these values win over direnv-provided ones. */ callerEnv?: Record; signal?: AbortSignal; - /** Cap on the direnv load; the caller's own timeout further clamps it. */ + /** Full direnv-load budget (`bash.direnvLoadTimeoutMs`). A positive + * `callerTimeoutMs` clamps the effective load below this; `0`/undefined + * leaves the full budget. */ timeoutMs?: number; + /** The caller's command deadline (ms). A positive value clamps the direnv + * load so a cold `.envrc` can't outlast a short-timeout command; `0` or + * undefined means "no caller clamp" — the load keeps its full `timeoutMs` + * budget (a disabled command deadline is NOT a 0 ms load). Centralizing the + * clamp here keeps every backend (executeBash, ACP terminal, PTY) on one + * contract instead of each re-deriving it. */ + callerTimeoutMs?: number; /** `bash.direnv` setting — `"off"` skips the load entirely. */ direnvSetting: "auto" | "off"; /** Shell wrapper prefix (profiler/strace) to place *after* the unset prefix, @@ -92,10 +101,16 @@ export async function applyDirenvPreflight( opts: DirenvPreflightOptions, ): Promise<{ command: string; env: Record | undefined }> { const withPrefix = (line: string): string => (opts.commandPrefix ? `${opts.commandPrefix} ${line}` : line); + // A positive caller deadline clamps the direnv load below its full budget so + // a cold `.envrc` can't outlast a short-timeout command; `0`/undefined means + // "no caller clamp" (a disabled command deadline is not a 0 ms load). Every + // backend routes through here, so the clamp lives in one place. + const loadTimeoutMs = + opts.callerTimeoutMs !== undefined && opts.callerTimeoutMs > 0 + ? Math.min(opts.timeoutMs ?? opts.callerTimeoutMs, opts.callerTimeoutMs) + : opts.timeoutMs; const direnvDiff = - opts.direnvSetting === "off" - ? null - : await loadDirenvEnv(cwd, { timeoutMs: opts.timeoutMs, signal: opts.signal }); + opts.direnvSetting === "off" ? null : await loadDirenvEnv(cwd, { timeoutMs: loadTimeoutMs, signal: opts.signal }); if (!direnvDiff) { return { command: withPrefix(command), env: opts.callerEnv }; } @@ -281,16 +296,8 @@ export async function executeBash(command: string, options?: BashExecutorOptions const preflight = await applyDirenvPreflight(command, commandCwd ?? process.cwd(), { callerEnv: options?.env, signal: options?.signal, - timeoutMs: (() => { - const direnvBudget = settings.get("bash.direnvLoadTimeoutMs"); - // A disabled command deadline (`timeout: 0`) means "no caller clamp", - // not a 0 ms direnv load — the load still gets its full budget. Only a - // positive caller timeout clamps the direnv window below that budget. - const callerDeadline = options?.timeout; - return callerDeadline !== undefined && callerDeadline > 0 - ? Math.min(direnvBudget, callerDeadline) - : direnvBudget; - })(), + timeoutMs: settings.get("bash.direnvLoadTimeoutMs"), + callerTimeoutMs: options?.timeout, direnvSetting: settings.get("bash.direnv"), commandPrefix: prefix, }); diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 67d9e210c..eb52dcfa9 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -889,6 +889,9 @@ export class BashTool implements AgentTool { }, ); }); + +describe("applyDirenvPreflight direnv-load clamp", () => { + let tempDir: string; + + beforeEach(() => { + tempDir = makeTempDir(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (fs.existsSync(tempDir)) removeSyncWithRetries(tempDir); + }); + + it("clamps the direnv load to a positive caller timeout below the full budget", async () => { + // The clamp lives INSIDE the helper, so every backend that routes through + // applyDirenvPreflight (executeBash, ACP terminal, PTY) inherits it. A + // positive callerTimeoutMs smaller than the full load budget must win, so a + // short-timeout command can't hand a cold `.envrc` the full 30s window. + // Reverting the clamp to executeBash-only leaves the ACP/PTY backend + // passing the full budget here, reddening this assertion. + const spy = vi.spyOn(direnvModule, "loadDirenvEnv").mockResolvedValue(null); + + await applyDirenvPreflight("true", tempDir, { + direnvSetting: "auto", + timeoutMs: 30_000, + callerTimeoutMs: 5, + }); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]?.timeoutMs).toBe(5); + }); + + it("keeps the full direnv-load budget when the caller deadline is disabled (callerTimeoutMs: 0)", async () => { + // A disabled command deadline (`0`) is NOT a 0 ms load — it means "no caller + // clamp", so the load keeps its full `timeoutMs` budget. Collapsing it to 0 + // would make AbortSignal.timeout(0) abort instantly and silently drop the + // repo's direnv env. + const spy = vi.spyOn(direnvModule, "loadDirenvEnv").mockResolvedValue(null); + + await applyDirenvPreflight("true", tempDir, { + direnvSetting: "auto", + timeoutMs: 30_000, + callerTimeoutMs: 0, + }); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]?.timeoutMs).toBe(30_000); + }); + + it("keeps the full direnv-load budget when no caller deadline is supplied (callerTimeoutMs: undefined)", async () => { + // An omitted caller deadline behaves like a disabled one: no clamp, full + // budget. Guards the `!== undefined` half of the clamp condition. + const spy = vi.spyOn(direnvModule, "loadDirenvEnv").mockResolvedValue(null); + + await applyDirenvPreflight("true", tempDir, { + direnvSetting: "auto", + timeoutMs: 30_000, + }); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy.mock.calls[0][1]?.timeoutMs).toBe(30_000); + }); +});