diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 945a4ed1c..ffc6a9836 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 diff --git a/packages/coding-agent/src/lsp/index.ts b/packages/coding-agent/src/lsp/index.ts index 2e411fa7d..a33733475 100644 --- a/packages/coding-agent/src/lsp/index.ts +++ b/packages/coding-agent/src/lsp/index.ts @@ -1565,7 +1565,7 @@ export class LspTool implements AgentTool> { 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; diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 28f4ee31d..565e41111 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -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, }); diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index e12fd489e..588867a60 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -312,10 +312,17 @@ function extractPartialBashEnv(partialJson: string | undefined): Record 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> { 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 }; diff --git a/packages/coding-agent/src/tools/debug.ts b/packages/coding-agent/src/tools/debug.ts index 2fcd8e5ac..ced6ffc6e 100644 --- a/packages/coding-agent/src/tools/debug.ts +++ b/packages/coding-agent/src/tools/debug.ts @@ -729,7 +729,7 @@ export class DebugTool implements AgentTool, _context?: AgentToolContext, ): Promise> { - 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 }; diff --git a/packages/coding-agent/src/tools/eval.ts b/packages/coding-agent/src/tools/eval.ts index 8b40fff40..b3990b842 100644 --- a/packages/coding-agent/src/tools/eval.ts +++ b/packages/coding-agent/src/tools/eval.ts @@ -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 { const backends = resolveEvalBackends(session); const allowPy = backends.python; @@ -538,7 +534,10 @@ export class EvalTool implements AgentTool { // 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 diff --git a/packages/coding-agent/src/tools/fetch.ts b/packages/coding-agent/src/tools/fetch.ts index c41d5dc93..d83c6d849 100644 --- a/packages/coding-agent/src/tools/fetch.ts +++ b/packages/coding-agent/src/tools/fetch.ts @@ -1622,7 +1622,7 @@ export async function fetchReadUrl( ): Promise { 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(); diff --git a/packages/coding-agent/src/tools/tool-timeouts.ts b/packages/coding-agent/src/tools/tool-timeouts.ts index 102c640ec..26d4395c8 100644 --- a/packages/coding-agent/src/tools/tool-timeouts.ts +++ b/packages/coding-agent/src/tools/tool-timeouts.ts @@ -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)); } diff --git a/packages/coding-agent/test/tools/lsp-regressions.test.ts b/packages/coding-agent/test/tools/lsp-regressions.test.ts index 8a215fcc0..c2da5ed90 100644 --- a/packages/coding-agent/test/tools/lsp-regressions.test.ts +++ b/packages/coding-agent/test/tools/lsp-regressions.test.ts @@ -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 diff --git a/packages/coding-agent/test/tools/tool-timeouts.test.ts b/packages/coding-agent/test/tools/tool-timeouts.test.ts new file mode 100644 index 000000000..2a1ae0711 --- /dev/null +++ b/packages/coding-agent/test/tools/tool-timeouts.test.ts @@ -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); + }); +});