From 1a44cd2e36e3b350383c8c94d83031aa160bc1fb Mon Sep 17 00:00:00 2001 From: Ogrodev Date: Mon, 11 May 2026 21:48:13 -0300 Subject: [PATCH] Fix ACP review follow-ups --- .../coding-agent/src/session/agent-session.ts | 12 +- packages/coding-agent/src/tools/bash.ts | 188 +++++++++--------- .../test/agent-session-acp-permission.test.ts | 15 ++ .../test/bash-acp-terminal.test.ts | 48 +++++ 4 files changed, 167 insertions(+), 96 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 7b4b65669..5412ab1a2 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -515,6 +515,8 @@ const PERMISSION_OPTIONS: ClientBridgePermissionOption[] = [ { optionId: "reject_always", name: "Always reject", kind: "reject_always" }, ]; +const PERMISSION_OPTIONS_BY_ID = new Map(PERMISSION_OPTIONS.map(option => [option.optionId, option])); + function derivePermissionTitle(toolName: string, args: unknown): string { const a = args && typeof args === "object" ? (args as Record) : {}; if (toolName === "bash") { @@ -2691,12 +2693,16 @@ export class AgentSession { if (outcome.outcome === "cancelled") { throw new ToolAbortError("Permission request cancelled"); } - if (outcome.optionId === "allow_always") { + const selectedOption = PERMISSION_OPTIONS_BY_ID.get(outcome.optionId); + if (!selectedOption) { + throw new ToolError(`Tool permission response used unknown option ID: ${outcome.optionId}`); + } + if (selectedOption.kind === "allow_always") { this.#acpPermissionDecisions.set(target.name, "allow_always"); - } else if (outcome.optionId === "reject_always") { + } else if (selectedOption.kind === "reject_always") { this.#acpPermissionDecisions.set(target.name, "reject_always"); } - if (outcome.optionId.startsWith("reject")) { + if (selectedOption.kind === "reject_once" || selectedOption.kind === "reject_always") { throw new ToolError(`Tool call rejected by user (${target.name})`); } return await target.execute(toolCallId, args as never, signal, onUpdate, ctx); diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index aca1a3c04..8148c879b 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -2,7 +2,7 @@ import * as fs from "node:fs"; import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import type { Component } from "@oh-my-pi/pi-tui"; import { ImageProtocol, TERMINAL, Text } from "@oh-my-pi/pi-tui"; -import { $env, getProjectDir, isEnoent, prompt } from "@oh-my-pi/pi-utils"; +import { $env, getProjectDir, isEnoent, logger, prompt } from "@oh-my-pi/pi-utils"; import { Type } from "@sinclair/typebox"; import { AsyncJobManager } from "../async"; import { type BashResult, executeBash } from "../exec/bash-executor"; @@ -654,108 +654,110 @@ export class BashTool implements AgentTool { signal?.addEventListener("abort", onAbortSignal, { once: true }); try { - if (signal?.aborted) { - await handle.kill(); - await handle.release(); - throw new ToolAbortError("Command aborted"); - } - - const timeoutPromise = Bun.sleep(timeoutMs).then(() => ({ kind: "timeout" as const })); - // Poll until the process exits, times out, or the caller aborts. - for (;;) { - const racers: Array> = [ - exitPromise.then(s => ({ kind: "exit" as const, status: s })), - timeoutPromise, - Bun.sleep(250).then(() => ({ kind: "poll" as const })), - ]; - if (signal) { - racers.push(abortedP.then(() => ({ kind: "aborted" as const }))); - } - const raced = await Promise.race(racers); - - if (raced.kind === "aborted" || signal?.aborted) { + try { + if (signal?.aborted) { await handle.kill(); - await handle.release(); throw new ToolAbortError("Command aborted"); } - if (raced.kind === "timeout") { - let current = { output: "", truncated: false }; - try { - current = await handle.currentOutput(); - } finally { - try { - await handle.kill(); - } finally { - await handle.release(); - } + const timeoutPromise = Bun.sleep(timeoutMs).then(() => ({ kind: "timeout" as const })); + // Poll until the process exits, times out, or the caller aborts. + for (;;) { + const racers: Array> = [ + exitPromise.then(s => ({ kind: "exit" as const, status: s })), + timeoutPromise, + Bun.sleep(250).then(() => ({ kind: "poll" as const })), + ]; + if (signal) { + racers.push(abortedP.then(() => ({ kind: "aborted" as const }))); } - const timedOutResult: BashInteractiveResult = { - output: current.output, - exitCode: undefined, - cancelled: false, - timedOut: true, - truncated: current.truncated, - totalLines: current.output.length > 0 ? current.output.split("\n").length : 0, - totalBytes: current.output.length, - outputLines: current.output.length > 0 ? current.output.split("\n").length : 0, - outputBytes: current.output.length, - }; - return this.#buildCompletedResult(timedOutResult, timeoutSec, { - requestedTimeoutSec, - notices: [timeoutClampNotice].filter((notice): notice is string => Boolean(notice)), - terminalId: handle.terminalId, + const raced = await Promise.race(racers); + + if (raced.kind === "aborted" || signal?.aborted) { + await handle.kill(); + throw new ToolAbortError("Command aborted"); + } + + if (raced.kind === "timeout") { + let current = { output: "", truncated: false }; + try { + current = await handle.currentOutput(); + } finally { + await handle.kill(); + } + const timedOutResult: BashInteractiveResult = { + output: current.output, + exitCode: undefined, + cancelled: false, + timedOut: true, + truncated: current.truncated, + totalLines: current.output.length > 0 ? current.output.split("\n").length : 0, + totalBytes: current.output.length, + outputLines: current.output.length > 0 ? current.output.split("\n").length : 0, + outputBytes: current.output.length, + }; + return this.#buildCompletedResult(timedOutResult, timeoutSec, { + requestedTimeoutSec, + notices: [timeoutClampNotice].filter((notice): notice is string => Boolean(notice)), + terminalId: handle.terminalId, + }); + } + + if (raced.kind === "exit") { + exitStatus = raced.status; + break; + } + + // Poll tick: push current output so agent-loop transcript stays consistent. + const current = await handle.currentOutput(); + onUpdate?.({ + content: [{ type: "text", text: current.output }], + details: { terminalId: handle.terminalId }, }); } - - if (raced.kind === "exit") { - exitStatus = raced.status; - break; - } - - // Poll tick: push current output so agent-loop transcript stays consistent. - const current = await handle.currentOutput(); - onUpdate?.({ - content: [{ type: "text", text: current.output }], - details: { terminalId: handle.terminalId }, - }); + } finally { + signal?.removeEventListener("abort", onAbortSignal); } + + // Fetch final output; the terminal is released in the outer finally. + const finalOutput = await handle.currentOutput(); + + // Map exit status: null exitCode with a signal → treat as signal kill (137). + const rawExitCode = exitStatus.exitCode; + const exitCode: number | undefined = + rawExitCode != null ? rawExitCode : exitStatus.signal ? 137 : undefined; + + const outputText = finalOutput.output; + const outputByteLen = outputText.length; + const outputLineCount = outputText.length > 0 ? outputText.split("\n").length : 0; + + const bridgeResult: BashResult = { + output: outputText, + exitCode, + cancelled: false, + truncated: finalOutput.truncated, + totalLines: outputLineCount, + totalBytes: outputByteLen, + outputLines: outputLineCount, + outputBytes: outputByteLen, + }; + + const bridgeNotices: string[] = []; + if (finalOutput.truncated) bridgeNotices.push("(output truncated)"); + if (timeoutClampNotice) bridgeNotices.push(timeoutClampNotice); + + return this.#buildCompletedResult(bridgeResult, timeoutSec, { + requestedTimeoutSec, + notices: bridgeNotices, + terminalId: handle.terminalId, + }); } finally { - signal?.removeEventListener("abort", onAbortSignal); + try { + await handle.release(); + } catch (error) { + logger.warn("ACP terminal release failed", { terminalId: handle.terminalId, error }); + } } - - // Fetch final output, then release the terminal. - const finalOutput = await handle.currentOutput(); - await handle.release(); - - // Map exit status: null exitCode with a signal → treat as signal kill (137). - const rawExitCode = exitStatus.exitCode; - const exitCode: number | undefined = rawExitCode != null ? rawExitCode : exitStatus.signal ? 137 : undefined; - - const outputText = finalOutput.output; - const outputByteLen = outputText.length; - const outputLineCount = outputText.length > 0 ? outputText.split("\n").length : 0; - - const bridgeResult: BashResult = { - output: outputText, - exitCode, - cancelled: false, - truncated: finalOutput.truncated, - totalLines: outputLineCount, - totalBytes: outputByteLen, - outputLines: outputLineCount, - outputBytes: outputByteLen, - }; - - const bridgeNotices: string[] = []; - if (finalOutput.truncated) bridgeNotices.push("(output truncated)"); - if (timeoutClampNotice) bridgeNotices.push(timeoutClampNotice); - - return this.#buildCompletedResult(bridgeResult, timeoutSec, { - requestedTimeoutSec, - notices: bridgeNotices, - terminalId: handle.terminalId, - }); } // Track output for streaming updates (tail only) diff --git a/packages/coding-agent/test/agent-session-acp-permission.test.ts b/packages/coding-agent/test/agent-session-acp-permission.test.ts index 5a742bc75..40874ac19 100644 --- a/packages/coding-agent/test/agent-session-acp-permission.test.ts +++ b/packages/coding-agent/test/agent-session-acp-permission.test.ts @@ -237,6 +237,21 @@ it("reject_once: throws ToolError and never calls underlying execute", async () expect(editTool.executeCalls).toBe(0); }); +it("unknown selected permission option ID fails closed without executing", async () => { + const editTool = makeFakeTool("edit"); + const bridge = makeBridge({ outcome: "selected", optionId: "allow_typo" }); + session = await createSession([editTool], bridge); + + await session.setActiveToolsByName(["edit"]); + const wrappedEdit = session.agent.state.tools.find(t => t.name === "edit"); + expect(wrappedEdit).toBeDefined(); + + await expect( + wrappedEdit!.execute("call-unknown", { path: "/tmp/foo.ts" }, undefined, undefined as never, undefined as never), + ).rejects.toThrow(/unknown option ID/); + expect(editTool.executeCalls).toBe(0); +}); + // --------------------------------------------------------------------------- // 3. Always allow caches: bridge called exactly once across two executions // --------------------------------------------------------------------------- diff --git a/packages/coding-agent/test/bash-acp-terminal.test.ts b/packages/coding-agent/test/bash-acp-terminal.test.ts index 25e91655e..d97115e70 100644 --- a/packages/coding-agent/test/bash-acp-terminal.test.ts +++ b/packages/coding-agent/test/bash-acp-terminal.test.ts @@ -76,6 +76,54 @@ describe("BashTool ACP terminal routing", () => { expect(releaseSpy).toHaveBeenCalledTimes(1); }); + it("releases the client terminal when final output retrieval fails", async () => { + const handle: ClientBridgeTerminalHandle = { + terminalId: "term-output-failure", + waitForExit: async () => ({ exitCode: 0, signal: null }), + currentOutput: async () => { + throw new Error("client output unavailable"); + }, + kill: async () => {}, + release: async () => {}, + }; + const bridge: ClientBridge = { + capabilities: { terminal: true }, + createTerminal: async () => handle, + }; + const releaseSpy = spyOn(handle, "release"); + + const tool = new BashTool(makeSession(bridge)); + + await expect(tool.execute("call-output-failure", { command: "echo hi" })).rejects.toThrow( + /client output unavailable/, + ); + expect(releaseSpy).toHaveBeenCalledTimes(1); + }); + + it("releases the client terminal when waiting for exit fails", async () => { + const handle: ClientBridgeTerminalHandle = { + terminalId: "term-exit-failure", + waitForExit: async () => { + throw new Error("client wait unavailable"); + }, + currentOutput: async () => ({ output: "", truncated: false }), + kill: async () => {}, + release: async () => {}, + }; + const bridge: ClientBridge = { + capabilities: { terminal: true }, + createTerminal: async () => handle, + }; + const releaseSpy = spyOn(handle, "release"); + + const tool = new BashTool(makeSession(bridge)); + + await expect(tool.execute("call-exit-failure", { command: "echo hi" })).rejects.toThrow( + /client wait unavailable/, + ); + expect(releaseSpy).toHaveBeenCalledTimes(1); + }); + it("kills and releases the client terminal when the command times out", async () => { const pendingExit = Promise.withResolvers<{ exitCode: number | null; signal: string | null }>(); const handle: ClientBridgeTerminalHandle = {