From 5941d797bae867139b330f1cf12d660567899875 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Wed, 8 Jul 2026 23:46:14 -0400 Subject: [PATCH] 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); + }); +});