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); + }); +});