smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(direnv): clamp the ACP/PTY backend preflight to the caller timeout

Browse Source 
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 <noreply@sealedsecurity.com>
This commit is contained in:
Matt Wilkinson
2026-07-09 01:35:17 -04:00
parent 5941d797ba
commit 33318cf5e1
3 changed files with 89 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/exec/bash-executor.ts
+21 -14
View File
@@ -64,8 +64,17 @@ export interface DirenvPreflightOptions {
/** Caller-supplied env overlay; these values win over direnv-provided ones. */
callerEnv?: Record<string, string>;
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<string, string> | 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,
});
packages/coding-agent/src/tools/bash.ts
+4
View File
@@ -889,6 +889,9 @@ export class BashTool implements AgentTool<typeof bashSchemaBase | typeof bashSc
// would double-apply the unset prefix and re-merge the env. No
// `commandPrefix` here: ACP applies the shell prefix via
// `wrapShellLineForClientTerminal`, and the PTY path never wrapped one.
// `callerTimeoutMs` clamps the direnv load to a positive command timeout
// (the backend's own timeout is installed only after this await), matching
// the executeBash branch so a cold `.envrc` can't outlast a short call.
const backendPreflight =
(clientBridge?.capabilities.terminal && clientBridge.createTerminal && !pty) ||
canUseInteractiveBashPty(pty, ctx)
@@ -896,6 +899,7 @@ export class BashTool implements AgentTool<typeof bashSchemaBase | typeof bashSc
callerEnv: resolvedEnv,
signal,
timeoutMs: this.session.settings.get("bash.direnvLoadTimeoutMs"),
callerTimeoutMs: timeoutMs,
direnvSetting: this.session.settings.get("bash.direnv"),
})
: undefined;
packages/coding-agent/test/bash-executor.test.ts
+64 -1
View File
@@ -3,7 +3,7 @@ import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import { resetSettingsForTest, Settings, type ShellMinimizerSettings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { buildMinimizerOptions, executeBash } from "@oh-my-pi/pi-coding-agent/exec/bash-executor";
import { applyDirenvPreflight, buildMinimizerOptions, executeBash } from "@oh-my-pi/pi-coding-agent/exec/bash-executor";
import * as direnvModule from "@oh-my-pi/pi-coding-agent/exec/direnv";
import { DEFAULT_MAX_BYTES } from "@oh-my-pi/pi-coding-agent/session/streaming-output";
import * as shellSnapshot from "@oh-my-pi/pi-coding-agent/utils/shell-snapshot";
@@ -1120,3 +1120,66 @@ describe("executeBash :async: background retention", () => {
},
);
});
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);
});
});