From bb4c9cae1e9b707543a910ff73f45f2d20129c15 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 26 May 2026 14:46:06 +0200 Subject: [PATCH] feat(coding-agent): added configurable IRC timeout with AbortSignal cancellation - Added configurable IRC message timeout setting with 120-second default to prevent indefinite hangs. - Implemented timeout enforcement for IRC send operations using AbortSignal-based cancellation. - Modified Python tool bridge to route concurrent evaluations using per-run identifiers alongside session IDs. - Enhanced test coverage for IRC timeout behavior, tool validation, and ephemeral cache key separation. --- packages/coding-agent/CHANGELOG.md | 4 +- .../coding-agent/src/session/agent-session.ts | 2 - packages/coding-agent/src/tools/irc.ts | 64 +++++++++++++++++-- packages/coding-agent/src/tools/search.ts | 14 ++-- .../agent-session-message-pipeline.test.ts | 63 +++++++++++++++++- .../test/core/python-tool-bridge.test.ts | 18 ++++-- packages/coding-agent/test/tools/irc.test.ts | 55 +++++++++++++--- .../test/tools/search-path-lists.test.ts | 25 ++++++++ 8 files changed, 213 insertions(+), 32 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 5d384c86d..7c8249277 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,13 +1,14 @@ # Changelog ## [Unreleased] - ### Breaking Changes - The `vim` edit mode option is no longer available; configurations using `edit.mode: vim` will be automatically mapped to `hashline` mode ### Added +- Added `irc.timeoutMs` setting to configure IRC message timeout duration with a default of 120 seconds +- Added timeout enforcement for IRC send operations to prevent indefinite hangs when recipients are unresponsive - Added evaluator state inheritance for `task`-spawned subagents so JavaScript and Python variables are visible between a parent agent and its child sessions - Added `hashline-per` edit mode to restore the legacy per-line hashline dialect alongside the default file-hash dialect - Added file-hash computation and validation for hashline sections to detect stale edits @@ -21,6 +22,7 @@ ### Changed +- Changed Python tool bridge to use per-run identifiers alongside session IDs for correct routing of tool responses and output in concurrent evaluations - Changed JavaScript and Python `eval` execution to allow overlapping asynchronous cells on the same session ID to run concurrently instead of being strictly queued - Updated the edit mode option set to support `replace`, `patch`, `hashline`, and `apply_patch` variants - Bare `A:` / `A-B:` (no payload, no inline body) now replaces the line/range with a single blank line, symmetric with bare `A↑` / `A↓` inserting a blank line; previously rejected as ambiguous diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index f6320f140..8563185f4 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -140,7 +140,6 @@ import type { Skill, SkillWarning } from "../extensibility/skills"; import { expandSlashCommand, type FileSlashCommand } from "../extensibility/slash-commands"; import { GoalRuntime } from "../goals/runtime"; import type { Goal, GoalModeState } from "../goals/state"; -import { getHashlineSyntax } from "../hashline/hash"; import type { HindsightSessionState } from "../hindsight/state"; import { type LocalProtocolOptions, resolveLocalUrlToPath } from "../internal-urls"; import { @@ -4137,7 +4136,6 @@ export class AgentSession { const fileMentionMessages = await generateFileMentionMessages(fileMentions, this.sessionManager.getCwd(), { autoResizeImages: this.settings.get("images.autoResize"), useHashLines: resolveFileDisplayMode(this).hashLines, - syntax: getHashlineSyntax(this.#resolveActiveEditMode()), }); messages.push(...fileMentionMessages); } diff --git a/packages/coding-agent/src/tools/irc.ts b/packages/coding-agent/src/tools/irc.ts index b1f169ba6..42382425a 100644 --- a/packages/coding-agent/src/tools/irc.ts +++ b/packages/coding-agent/src/tools/irc.ts @@ -25,6 +25,7 @@ import ircDescription from "../prompts/tools/irc.md" with { type: "text" }; import type { AgentRef, AgentRegistry } from "../registry/agent-registry"; import type { ToolSession } from "."; +const DEFAULT_IRC_TIMEOUT_MS = 120_000; const ircSchema = z.object({ op: z.enum(["send", "list"]).describe("irc operation"), to: z.string().optional().describe('recipient agent id or "all"'), @@ -159,6 +160,7 @@ export class IrcTool implements AgentTool { const awaitReply = params.awaitReply ?? !isBroadcast; + const timeoutMs = normalizeIrcTimeoutMs(this.session.settings.get("irc.timeoutMs")); const delivered: string[] = []; const replies: IrcReply[] = []; const failed: Array<{ id: string; error: string }> = []; @@ -174,12 +176,18 @@ export class IrcTool implements AgentTool { return; } try { - const result = await targetSession.respondAsBackground({ - from: senderId, - message, - awaitReply, + const result = await runIrcDispatchWithTimeout( + timeoutMs, signal, - }); + timeoutSignal => + targetSession.respondAsBackground({ + from: senderId, + message, + awaitReply, + signal: timeoutSignal, + }), + target.id, + ); delivered.push(target.id); if (awaitReply && result.replyText) { replies.push({ from: target.id, text: result.replyText }); @@ -237,3 +245,49 @@ function errorResult(text: string, details: IrcDetails): AgentToolResult( + timeoutMs: number, + parentSignal: AbortSignal | undefined, + run: (signal?: AbortSignal) => Promise, + targetId: string, +): Promise { + if (timeoutMs <= 0) { + return await run(parentSignal); + } + + const controller = new AbortController(); + const timeoutError = new Error(`IRC timed out waiting for ${targetId} after ${timeoutMs} ms`); + let timeout: NodeJS.Timeout | undefined; + let parentAbortListener: (() => void) | undefined; + + const timeoutDeferred = Promise.withResolvers(); + if (parentSignal) { + if (parentSignal.aborted) { + throw parentSignal.reason instanceof Error ? parentSignal.reason : new Error("IRC aborted"); + } + parentAbortListener = () => { + controller.abort(parentSignal.reason); + timeoutDeferred.reject(parentSignal.reason instanceof Error ? parentSignal.reason : new Error("IRC aborted")); + }; + parentSignal.addEventListener("abort", parentAbortListener, { once: true }); + } + + timeout = setTimeout(() => { + controller.abort(timeoutError); + timeoutDeferred.reject(timeoutError); + }, timeoutMs); + timeout.unref?.(); + + try { + return await Promise.race([run(controller.signal), timeoutDeferred.promise]); + } finally { + if (timeout) clearTimeout(timeout); + if (parentSignal && parentAbortListener) parentSignal.removeEventListener("abort", parentAbortListener); + } +} diff --git a/packages/coding-agent/src/tools/search.ts b/packages/coding-agent/src/tools/search.ts index 6a8310b6e..9ca63b1ab 100644 --- a/packages/coding-agent/src/tools/search.ts +++ b/packages/coding-agent/src/tools/search.ts @@ -9,12 +9,11 @@ import { prompt, untilAborted } from "@oh-my-pi/pi-utils"; import * as z from "zod/v4"; import { getFileReadCache } from "../edit/file-read-cache"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; -import { formatHashlineHeader, getHashlineSyntax } from "../hashline/hash"; +import { formatHashlineHeader } from "../hashline/hash"; import type { Theme } from "../modes/theme/theme"; import searchDescription from "../prompts/tools/search.md" with { type: "text" }; import { DEFAULT_MAX_COLUMN, type TruncationResult, truncateHead } from "../session/streaming-output"; import { Ellipsis, fileHyperlink, renderStatusLine, renderTreeList, truncateToWidth } from "../tui"; -import { resolveEditMode } from "../utils/edit-mode"; import { resolveFileDisplayMode } from "../utils/file-display-mode"; import type { ToolSession } from "."; import { @@ -488,7 +487,6 @@ export class SearchTool implements AgentTool(); - const hashlineSyntax = getHashlineSyntax(resolveEditMode(this.session)); if (baseDisplayMode.hashLines) { for (const relativePath of fileList) { if (archiveDisplaySet.has(relativePath)) continue; @@ -496,7 +494,7 @@ export class SearchTool implements AgentTool { afterEach(async () => { vi.restoreAllMocks(); + clearCustomApis(); for (const session of sessions.splice(0)) { await session.dispose(); } @@ -90,6 +99,58 @@ describe("AgentSession message pipeline", () => { expect(requestOnPayload).toHaveBeenCalledWith({ original: true, session: true }, undefined); expect(result).toEqual({ original: true, session: true }); }); + it("keeps ephemeral side-channel cache key separate from provider routing", async () => { + const api = "test-ephemeral-side-channel"; + let capturedOptions: SimpleStreamOptions | undefined; + registerCustomApi(api, (_model, _context, options) => { + capturedOptions = options; + const stream = new AssistantMessageEventStream(); + queueMicrotask(() => { + const message = createAssistantMessage("Answer"); + stream.push({ type: "text_delta", contentIndex: 0, delta: "Answer", partial: message }); + stream.push({ type: "done", reason: "stop", message }); + }); + return stream; + }); + + const model = { + id: "side-model", + name: "Side Model", + api, + provider: "test-provider", + baseUrl: "", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 4096, + maxTokens: 1024, + } satisfies Model; + const session = new AgentSession({ + agent: new Agent({ + initialState: { + model, + systemPrompt: ["system prompt"], + messages: [], + tools: [], + }, + }), + sessionManager: SessionManager.inMemory(), + settings: Settings.isolated({ "compaction.enabled": false }), + modelRegistry: { + getApiKey: vi.fn(async () => "key"), + } as never, + }); + sessions.push(session); + const cacheSessionId = session.sessionId; + + const result = await session.runEphemeralTurn({ promptText: "Question?" }); + + expect(result.replyText).toBe("Answer"); + expect(capturedOptions?.promptCacheKey).toBe(cacheSessionId); + expect(capturedOptions?.sessionId).toStartWith(`${cacheSessionId}:side:`); + expect(capturedOptions?.sessionId).not.toBe(cacheSessionId); + expect(capturedOptions?.preferWebsockets).toBe(false); + }); it("records raw SSE diagnostics into the session buffer before request hooks", async () => { const requestOnSseEvent = vi.fn(); diff --git a/packages/coding-agent/test/core/python-tool-bridge.test.ts b/packages/coding-agent/test/core/python-tool-bridge.test.ts index c905fe922..67d158356 100644 --- a/packages/coding-agent/test/core/python-tool-bridge.test.ts +++ b/packages/coding-agent/test/core/python-tool-bridge.test.ts @@ -58,10 +58,11 @@ describe("Python tool bridge HTTP server", () => { }); const session = makeSession(new Map([["read", readTool]])); const info = await ensurePyToolBridge(); - const unregister = registerPyToolBridge("test-session-1", { toolSession: session }); + const unregister = registerPyToolBridge("test-session-1", "run-1", { toolSession: session }); try { const res = await call(info, { session: "test-session-1", + run: "run-1", name: "read", args: { path: "foo.ts", _i: "py prelude" }, }); @@ -78,7 +79,7 @@ describe("Python tool bridge HTTP server", () => { it("returns ok=false when no session is registered for the given id", async () => { const info = await ensurePyToolBridge(); - const res = await call(info, { session: "missing", name: "read", args: {} }); + const res = await call(info, { session: "missing", run: "run-missing", name: "read", args: {} }); expect(res.status).toBe(200); const body = (await res.json()) as { ok: boolean; error?: string }; expect(body.ok).toBe(false); @@ -99,9 +100,9 @@ describe("Python tool bridge HTTP server", () => { }) as unknown as AgentTool, } as unknown as ToolSession; const info = await ensurePyToolBridge(); - const unregister = registerPyToolBridge("err-session", { toolSession: session }); + const unregister = registerPyToolBridge("err-session", "run-err", { toolSession: session }); try { - const res = await call(info, { session: "err-session", name: "boom", args: {} }); + const res = await call(info, { session: "err-session", run: "run-err", name: "boom", args: {} }); expect(res.status).toBe(200); const body = await res.json(); expect(body).toEqual({ ok: false, error: "kapow" }); @@ -112,7 +113,11 @@ describe("Python tool bridge HTTP server", () => { it("rejects requests with a bad bearer token", async () => { const info = await ensurePyToolBridge(); - const res = await call(info, { session: "anything", name: "read", args: {} }, { token: "wrong" }); + const res = await call( + info, + { session: "anything", run: "run-anything", name: "read", args: {} }, + { token: "wrong" }, + ); expect(res.status).toBe(403); }); @@ -130,13 +135,14 @@ describe("Python tool bridge HTTP server", () => { const session = makeSession(new Map([["read", readTool]])); const info = await ensurePyToolBridge(); const statusEvents: Array<{ op: string }> = []; - const unregister = registerPyToolBridge("status-session", { + const unregister = registerPyToolBridge("status-session", "run-status", { toolSession: session, emitStatus: event => statusEvents.push(event), }); try { const res = await call(info, { session: "status-session", + run: "run-status", name: "read", args: { path: "foo.ts" }, }); diff --git a/packages/coding-agent/test/tools/irc.test.ts b/packages/coding-agent/test/tools/irc.test.ts index f489f9e99..7bcc77ce9 100644 --- a/packages/coding-agent/test/tools/irc.test.ts +++ b/packages/coding-agent/test/tools/irc.test.ts @@ -14,15 +14,22 @@ interface FakeSession { setError: (error: Error) => void; /** Resolve the next respondAsBackground call only when allowed. */ gateNextCall: () => { release: () => void }; + /** Keep the next respondAsBackground call pending until aborted. */ + hangNextCall: () => void; } - function makeFakeSession(): FakeSession { let nextReply = "auto-reply"; let nextError: Error | null = null; let gate: { promise: Promise; release: () => void } | null = null; + let hangNext = false; const calls: Array<{ from: string; message: string; awaitReply: boolean }> = []; const session = { - respondAsBackground: async (args: { from: string; message: string; awaitReply?: boolean }) => { + respondAsBackground: async (args: { + from: string; + message: string; + awaitReply?: boolean; + signal?: AbortSignal; + }) => { const awaitReply = args.awaitReply !== false; calls.push({ from: args.from, message: args.message, awaitReply }); if (gate) { @@ -30,6 +37,21 @@ function makeFakeSession(): FakeSession { gate = null; await g.promise; } + if (hangNext) { + hangNext = false; + const deferred = Promise.withResolvers(); + if (args.signal?.aborted) { + deferred.reject(args.signal.reason instanceof Error ? args.signal.reason : new Error("aborted")); + } else { + args.signal?.addEventListener( + "abort", + () => + deferred.reject(args.signal?.reason instanceof Error ? args.signal.reason : new Error("aborted")), + { once: true }, + ); + } + return await deferred.promise; + } if (nextError) { const err = nextError; nextError = null; @@ -48,12 +70,12 @@ function makeFakeSession(): FakeSession { nextError = error; }, gateNextCall: () => { - let release!: () => void; - const promise = new Promise(resolve => { - release = resolve; - }); - gate = { promise, release }; - return { release }; + const { promise, resolve } = Promise.withResolvers(); + gate = { promise, release: resolve }; + return { release: resolve }; + }, + hangNextCall: () => { + hangNext = true; }, }; } @@ -202,6 +224,23 @@ describe("IrcTool", () => { expect(result.details?.notFound).toEqual(["0-Ghost"]); }); + it("op=send fails a hung recipient after the configured timeout", async () => { + const main = makeFakeSession(); + const sub = makeFakeSession(); + sub.hangNextCall(); + registry.register({ id: "0-Main", displayName: "main", kind: "main", session: main.session }); + registry.register({ id: "0-Hung", displayName: "task", kind: "sub", parentId: "0-Main", session: sub.session }); + + const toolSession = makeToolSession(registry, "0-Main"); + toolSession.settings.set("irc.timeoutMs", 5); + const tool = new IrcTool(toolSession); + const result = await tool.execute("call-timeout", { op: "send", to: "0-Hung", message: "ping" }); + + expect(result.details?.delivered ?? []).toEqual([]); + expect(result.details?.failed).toEqual([{ id: "0-Hung", error: "IRC timed out waiting for 0-Hung after 5 ms" }]); + expect(sub.calls).toEqual([{ from: "0-Main", message: "ping", awaitReply: true }]); + }); + it("op=send surfaces recipient errors as failed", async () => { const main = makeFakeSession(); const sub = makeFakeSession(); diff --git a/packages/coding-agent/test/tools/search-path-lists.test.ts b/packages/coding-agent/test/tools/search-path-lists.test.ts index 5bae608a6..077e56ecf 100644 --- a/packages/coding-agent/test/tools/search-path-lists.test.ts +++ b/packages/coding-agent/test/tools/search-path-lists.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import { validateToolArguments } from "@oh-my-pi/pi-ai/utils/validation"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { ToolChoiceQueue } from "@oh-my-pi/pi-coding-agent/session/tool-choice-queue"; import { createTools, type ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; @@ -90,6 +91,30 @@ describe("tool path arrays", () => { expect(details?.scopePath).toBe("apps/, packages/, phases/"); }); + it("search accepts a single string path through tool validation", async () => { + const tools = await createTools(createTestSession(tempDir)); + const tool = tools.find(entry => entry.name === "search"); + expect(tool).toBeDefined(); + if (!tool) throw new Error("Missing search tool"); + + const args = validateToolArguments(tool, { + type: "toolCall", + id: "search-single-string-path", + name: tool.name, + arguments: { + pattern: "space-needle", + paths: "folder with spaces/", + }, + }); + const result = await tool.execute("search-single-string-path", args); + const text = getText(result); + const details = result.details as { fileCount?: number; scopePath?: string } | undefined; + + expect(text).toContain("note.txt"); + expect(details?.fileCount).toBe(1); + expect(details?.scopePath).toBe("folder with spaces"); + }); + it("search keeps a single path that contains spaces", async () => { const tools = await createTools(createTestSession(tempDir)); const tool = tools.find(entry => entry.name === "search");