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

Merge PR #6296: fix(tools): cap per-tool default timeout with tools.maxTimeout (@roboomp)

Browse Source
This commit is contained in:
can1357
2026-07-22 21:13:20 +02:00
parent d4219e6d0b 1b6588e520
commit 13e7f29bc4
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
@@ -19,6 +19,10 @@
- Fixed `/agents` showing prewalk as off for the bundled `task` agent when `task.prewalk` enables its runtime default ([#6306](https://github.com/can1357/oh-my-pi/issues/6306)).
- Fixed `hub start`/`for:"ready"` waits blocking for the full readiness timeout when the launched process became ready and exited within one poll interval (or exited before ever becoming ready). The wait now wakes on the sticky `readyAt` marker or any terminal state instead of sampling the transient live state, and `start` reports `Process exited before readiness was observed.` for a pre-ready exit ([#6303](https://github.com/can1357/oh-my-pi/issues/6303)).
### 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
@@ -16044,7 +16044,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);
});
});