diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a94969a4d..02217349e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 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 cbd232a0e..d4fc5d96b 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -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, }); 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); + }); +});