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

fix(tools): cap per-tool default timeout with tools.maxTimeout

Browse Source 
clampTimeout resolved the per-tool default (bash 300s) whenever the agent
omitted `timeout` and only enforced the tool's own min/max, so the
tools.maxTimeout global ceiling — applied solely in sdk.ts on explicitly
numeric args — was bypassed on the common default-fallback path.

Thread maxTimeout into clampTimeout so the resolved effective timeout,
including the default path, is capped before the per-tool floor/ceiling
apply. Explicit values below the cap still win; maxTimeout <= 0 stays
no-cap. Applied at every call site (bash, eval, browser, debug, lsp,
fetch, and the session-level bash executor), and the bash clamp notice
now names the global ceiling when it is the binding limit.

Fixes #6294
This commit is contained in:
roboomp
2026-07-22 15:18:02 +00:00
parent 7b141199d5
commit 1b6588e520
11 changed files with 99 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed `tools.maxTimeout` only clamping explicitly-passed tool timeouts: the per-tool default (e.g. `bash` 300s), used whenever the agent omits `timeout`, bypassed the ceiling entirely. `clampTimeout` now caps the resolved effective timeout — including the default-fallback path — against `tools.maxTimeout` at every call site (`bash`, `eval`, `browser`, `debug`, `lsp`, `fetch`, and the session-level bash executor) ([#6294](https://github.com/can1357/oh-my-pi/issues/6294)).
## [17.0.7] - 2026-07-21
### Fixed
packages/coding-agent/src/lsp/index.ts
+1 -1
View File
@@ -1565,7 +1565,7 @@ export class LspTool implements AgentTool<typeof lspSchema, LspToolDetails, Them
_context?: AgentToolContext,
): Promise<AgentToolResult<LspToolDetails>> {
const { action, file, line, symbol, query, new_name, apply, timeout } = params;
const timeoutSec = clampTimeout("lsp", timeout);
const timeoutSec = clampTimeout("lsp", timeout, this.session.settings.get("tools.maxTimeout"));
const timeoutSignal = AbortSignal.timeout(timeoutSec * 1000);
const callerSignal = signal;
signal = callerSignal ? AbortSignal.any([callerSignal, timeoutSignal]) : timeoutSignal;
packages/coding-agent/src/session/agent-session.ts
+1 -1
View File
@@ -15941,7 +15941,7 @@ export class AgentSession {
signal: abortController.signal,
sessionKey: target.sessionId,
cwd,
timeout: clampTimeout("bash") * 1000,
timeout: clampTimeout("bash", undefined, this.settings.get("tools.maxTimeout")) * 1000,
onMinimizedSave: originalText => this.#saveBashOriginalArtifact(target, originalText),
useUserShell: options?.useUserShell,
});
packages/coding-agent/src/tools/bash.ts
+14 -6
View File
@@ -312,10 +312,17 @@ function extractPartialBashEnv(partialJson: string | undefined): Record<string,
return Object.keys(env).length > 0 ? env : undefined;
}
function formatTimeoutClampNotice(requestedTimeoutSec: number, effectiveTimeoutSec: number): string | undefined {
return requestedTimeoutSec !== effectiveTimeoutSec
? `Timeout clamped to ${effectiveTimeoutSec}s (requested ${requestedTimeoutSec}s; allowed range ${TOOL_TIMEOUTS.bash.min}-${TOOL_TIMEOUTS.bash.max}s).`
: undefined;
function formatTimeoutClampNotice(
requestedTimeoutSec: number,
effectiveTimeoutSec: number,
maxTimeout: number,
): string | undefined {
if (requestedTimeoutSec === effectiveTimeoutSec) return undefined;
const cappedByGlobal = maxTimeout > 0 && effectiveTimeoutSec === maxTimeout && maxTimeout < TOOL_TIMEOUTS.bash.max;
const limit = cappedByGlobal
? `global tools.maxTimeout ceiling ${maxTimeout}s`
: `allowed range ${TOOL_TIMEOUTS.bash.min}-${TOOL_TIMEOUTS.bash.max}s`;
return `Timeout clamped to ${effectiveTimeoutSec}s (requested ${requestedTimeoutSec}s; ${limit}).`;
}
function formatWallTimeSeconds(wallTimeMs: number): string {
@@ -831,11 +838,12 @@ export class BashTool implements AgentTool<typeof bashSchemaBase | typeof bashSc
// must still cancel the call or job, but OMP does not impose a deadline.
const requestedTimeoutSec = rawTimeout;
const timeoutDisabled = requestedTimeoutSec === 0;
const timeoutSec = timeoutDisabled ? undefined : clampTimeout("bash", requestedTimeoutSec);
const maxTimeout = this.session.settings.get("tools.maxTimeout");
const timeoutSec = timeoutDisabled ? undefined : clampTimeout("bash", requestedTimeoutSec, maxTimeout);
const timeoutMs = timeoutSec === undefined ? undefined : timeoutSec * 1000;
const pendingNotices: string[] = [];
if (timeoutSec !== undefined) {
const timeoutClampNotice = formatTimeoutClampNotice(requestedTimeoutSec, timeoutSec);
const timeoutClampNotice = formatTimeoutClampNotice(requestedTimeoutSec, timeoutSec, maxTimeout);
if (timeoutClampNotice) pendingNotices.push(timeoutClampNotice);
}
packages/coding-agent/src/tools/browser.ts
+1 -1
View File
@@ -190,7 +190,7 @@ export class BrowserTool implements AgentTool<typeof browserSchema, BrowserToolD
): Promise<AgentToolResult<BrowserToolDetails>> {
try {
throwIfAborted(signal);
const timeoutSeconds = clampTimeout("browser", params.timeout);
const timeoutSeconds = clampTimeout("browser", params.timeout, this.session.settings.get("tools.maxTimeout"));
const timeoutMs = timeoutSeconds * 1000;
const name = params.name ?? DEFAULT_TAB_NAME;
const details: BrowserToolDetails = { action: params.action, name };
packages/coding-agent/src/tools/debug.ts
+1 -1
View File
@@ -729,7 +729,7 @@ export class DebugTool implements AgentTool<typeof debugSchema, DebugToolDetails
_onUpdate?: AgentToolUpdateCallback<DebugToolDetails>,
_context?: AgentToolContext,
): Promise<AgentToolResult<DebugToolDetails>> {
const timeoutSec = clampTimeout("debug", params.timeout);
const timeoutSec = clampTimeout("debug", params.timeout, this.session.settings.get("tools.maxTimeout"));
const timeoutSignal = AbortSignal.timeout(timeoutSec * 1000);
const combinedSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal;
const details: DebugToolDetails = { action: params.action, success: true };
packages/coding-agent/src/tools/eval.ts
+4 -5
View File
@@ -217,10 +217,6 @@ function detailsNotice(cells: ResolvedEvalCell[]): string | undefined {
return notices.length > 0 ? notices.join(" ") : undefined;
}
function timeoutSecondsFromMs(timeoutMs: number): number {
return clampTimeout("eval", timeoutMs / 1000);
}
async function resolveBackend(session: ToolSession, language: EvalLanguage): Promise<ResolvedBackend> {
const backends = resolveEvalBackends(session);
const allowPy = backends.python;
@@ -538,7 +534,10 @@ export class EvalTool implements AgentTool<typeof evalSchema> {
// ordinary tool calls all count against the budget. The watchdog drives
// `combinedSignal`; we pass no wall-clock deadline downstream so the
// backends never arm a competing fixed timer.
const idleTimeoutMs = cell.timeoutMs === 0 ? undefined : timeoutSecondsFromMs(cell.timeoutMs) * 1000;
const idleTimeoutMs =
cell.timeoutMs === 0
? undefined
: clampTimeout("eval", cell.timeoutMs / 1000, session.settings.get("tools.maxTimeout")) * 1000;
const idle = idleTimeoutMs === undefined ? undefined : new IdleTimeout(idleTimeoutMs);
const combinedSignal =
signal && idle
packages/coding-agent/src/tools/fetch.ts
+1 -1
View File
@@ -1622,7 +1622,7 @@ export async function fetchReadUrl(
): Promise<ReadUrlEntry> {
const { path: url, raw = false } = params;
const effectiveTimeout = clampTimeout("fetch", 30);
const effectiveTimeout = clampTimeout("fetch", 30, session.settings.get("tools.maxTimeout"));
if (signal?.aborted) {
throw new ToolAbortError();
packages/coding-agent/src/tools/tool-timeouts.ts
+10 -3
View File
@@ -21,10 +21,17 @@ export type ToolWithTimeout = keyof typeof TOOL_TIMEOUTS;
/**
* Clamp a raw timeout to the allowed range for a tool.
* If rawTimeout is undefined, returns the tool's default.
*
* When `rawTimeout` is undefined the tool's `default` is used. A positive
* `maxTimeout` (the `tools.maxTimeout` global ceiling) caps the *resolved*
* value — including the default-fallback path — before the per-tool `min`/`max`
* floor and ceiling apply, so a configured global cap governs calls where the
* agent omits `timeout`, not only explicitly-passed values. `maxTimeout <= 0`
* means no global cap.
*/
export function clampTimeout(tool: ToolWithTimeout, rawTimeout?: number): number {
export function clampTimeout(tool: ToolWithTimeout, rawTimeout?: number, maxTimeout?: number): number {
const config = TOOL_TIMEOUTS[tool];
const timeout = rawTimeout ?? config.default;
return Math.max(config.min, Math.min(config.max, timeout));
const capped = maxTimeout !== undefined && maxTimeout > 0 ? Math.min(timeout, maxTimeout) : timeout;
return Math.max(config.min, Math.min(config.max, capped));
}
packages/coding-agent/test/tools/lsp-regressions.test.ts
+22 -16
View File
@@ -4,6 +4,7 @@ import * as os from "node:os";
import * as path from "node:path";
import type { AgentToolResult, RenderResultOptions } from "@oh-my-pi/pi-agent-core";
import { arkToWireSchema } from "@oh-my-pi/pi-ai/utils/schema";
import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { preloadPluginRoots } from "@oh-my-pi/pi-coding-agent/discovery/helpers";
import { LspTool } from "@oh-my-pi/pi-coding-agent/lsp";
import * as lspClient from "@oh-my-pi/pi-coding-agent/lsp/client";
@@ -50,6 +51,11 @@ import type { Subprocess } from "bun";
import DEFAULTS from "../../src/lsp/defaults.json" with { type: "json" };
import { getLanguageFromPath } from "../../src/utils/lang-from-path";
/** Minimal LSP tool session: production always supplies `settings`; these tests only need cwd + a default settings stub. */
function makeLspSession(cwd: string): ToolSession {
return { cwd, settings: Settings.isolated() } as ToolSession;
}
interface RpcMessage {
jsonrpc?: string;
id?: number | string;
@@ -850,7 +856,7 @@ describe("lsp regressions", () => {
});
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["rust-analyzer", server]]);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("rust-wait-test", {
action: "definition",
file: sourcePath,
@@ -919,7 +925,7 @@ describe("lsp regressions", () => {
});
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["rust-analyzer", server]]);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("rust-standalone-test", {
action: "definition",
file: sourcePath,
@@ -1269,7 +1275,7 @@ describe("lsp regressions", () => {
});
vi.spyOn(lspConfig, "getServersForFile").mockReturnValue([["fake-lsp", serverConfig]]);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute(`pull-diagnostics-${dynamicRegistration}`, {
action: "diagnostics",
file: targetFile,
@@ -1361,7 +1367,7 @@ describe("lsp regressions", () => {
client.diagnosticsVersion += 1;
}, 80);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("diag-stale", {
action: "diagnostics",
file: targetFile,
@@ -1395,7 +1401,7 @@ describe("lsp regressions", () => {
await Bun.write(path.join(tempDir.path(), "go.work"), ["go 1.22", "", "use ./service", ""].join("\n"));
await Bun.write(path.join(serviceDir, "go.mod"), "module example.com/service\n\ngo 1.22\n");
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("go-work-only-diagnostics", {
action: "diagnostics",
file: "*",
@@ -1442,7 +1448,7 @@ describe("lsp regressions", () => {
["go 1.22", "", "use (", "\t.", "\t./service", "\t./tools/helper", ")", ""].join("\n"),
);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("go-work-module-patterns", {
action: "diagnostics",
file: "*",
@@ -1754,7 +1760,7 @@ describe("lsp regressions", () => {
notifications.push({ method, params });
});
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("rename-file-test", {
action: "rename_file",
file: sourceFile,
@@ -1834,7 +1840,7 @@ describe("lsp regressions", () => {
});
const notifySpy = vi.spyOn(lspClient, "sendNotification").mockResolvedValue();
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
await tool.execute("rename-file-preview", {
action: "rename_file",
file: sourceFile,
@@ -1898,7 +1904,7 @@ describe("lsp regressions", () => {
});
vi.spyOn(lspClient, "sendNotification").mockResolvedValue();
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
await tool.execute("rename-dir-test", {
action: "rename_file",
file: srcDir,
@@ -1970,7 +1976,7 @@ describe("lsp regressions", () => {
return { expansion: "macro_rules!" };
});
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("request-test", {
action: "request",
file: filePath,
@@ -2037,7 +2043,7 @@ describe("lsp regressions", () => {
return null;
});
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
await tool.execute("request-payload", {
action: "request",
query: "workspace/executeCommand",
@@ -2095,7 +2101,7 @@ describe("lsp regressions", () => {
});
vi.spyOn(lspClient, "getOrCreateClient").mockResolvedValue(client);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("caps-test", {
action: "capabilities",
timeout: 5,
@@ -2514,7 +2520,7 @@ describe("lsp regressions", () => {
const notifySpy = vi.spyOn(lspClient, "sendNotification");
const getClientSpy = vi.spyOn(lspClient, "getOrCreateClient");
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const result = await tool.execute("rename-md", {
action: "rename_file",
file: sourceFile,
@@ -2558,7 +2564,7 @@ describe("lsp regressions", () => {
vi.spyOn(lspClient, "getOrCreateClient").mockResolvedValue(client);
vi.spyOn(lspClient, "sendRequest").mockResolvedValue(null);
const tool = new LspTool({ cwd: tempDir.path() } as ToolSession);
const tool = new LspTool(makeLspSession(tempDir.path()));
const initial = await tool.execute("reload-redetect-status", { action: "status" });
const initialOutput = initial.content
.filter(block => block.type === "text")
@@ -2608,7 +2614,7 @@ describe("lsp regressions", () => {
{ name: "typescript-language-server", status: "ready", fileTypes: [".ts"] },
]);
const tool = new LspTool({ cwd: process.cwd() } as ToolSession);
const tool = new LspTool(makeLspSession(process.cwd()));
const result = await tool.execute("status-test", { action: "status" });
const output = result.content
.filter(block => block.type === "text")
@@ -2649,7 +2655,7 @@ describe("lsp regressions", () => {
vi.spyOn(lspClient, "getOrCreateClient").mockRejectedValue(new Error("spawn suppressed in test"));
vi.spyOn(lspClient, "getActiveClients").mockReturnValue([]);
const tool = new LspTool({ cwd } as ToolSession);
const tool = new LspTool(makeLspSession(cwd));
const status1 = await tool.execute("cache-1", { action: "status" });
const text1 = status1.content
packages/coding-agent/test/tools/tool-timeouts.test.ts
+40
View File
@@ -0,0 +1,40 @@
import { describe, expect, it } from "bun:test";
import { clampTimeout, TOOL_TIMEOUTS } from "@oh-my-pi/pi-coding-agent/tools/tool-timeouts";
describe("clampTimeout", () => {
it("returns the per-tool default when no raw timeout is given", () => {
expect(clampTimeout("bash")).toBe(TOOL_TIMEOUTS.bash.default);
expect(clampTimeout("eval")).toBe(TOOL_TIMEOUTS.eval.default);
});
it("clamps explicit values to the per-tool min/max", () => {
expect(clampTimeout("bash", 0.1)).toBe(TOOL_TIMEOUTS.bash.min);
expect(clampTimeout("bash", 999_999)).toBe(TOOL_TIMEOUTS.bash.max);
expect(clampTimeout("lsp", 1)).toBe(TOOL_TIMEOUTS.lsp.min);
});
it("caps the default-fallback path with a positive maxTimeout", () => {
// Regression for #6294: omitting the raw timeout used the 300s bash
// default and bypassed tools.maxTimeout entirely.
expect(clampTimeout("bash", undefined, 30)).toBe(30);
});
it("caps an explicit value above maxTimeout", () => {
expect(clampTimeout("bash", 600, 30)).toBe(30);
});
it("lets an explicit value below maxTimeout win", () => {
expect(clampTimeout("bash", 10, 30)).toBe(10);
});
it("treats maxTimeout <= 0 as no global cap", () => {
expect(clampTimeout("bash", undefined, 0)).toBe(TOOL_TIMEOUTS.bash.default);
expect(clampTimeout("bash", undefined, -1)).toBe(TOOL_TIMEOUTS.bash.default);
});
it("still enforces the per-tool min when maxTimeout is below it", () => {
// maxTimeout under the floor cannot drive the effective timeout below
// the tool's own minimum (bash min = 1s).
expect(clampTimeout("bash", undefined, 0.1)).toBe(TOOL_TIMEOUTS.bash.min);
});
});