diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 169e6e2e0..f17b1b970 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Breaking Changes + +- `user_bash` now fails closed: errors or invalid defined results abort the command without invoking later handlers or executing locally. Return `undefined` to continue propagation; otherwise return `{ operations }` or `{ result }` ([#9068](https://github.com/earendil-works/pi/issues/9068)). + ### Added - Added `ctx.modelRegistry.stream()` and `streamSimple()` for extension model calls through configured providers with resolved authentication ([#8964](https://github.com/earendil-works/pi/issues/8964)). diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index 2d95d1b46..9b666e649 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -909,6 +909,8 @@ pi.on("user_bash", (event, ctx) => { }); ``` +Returning `undefined` continues to the next handler, then local execution if none handles the event. A valid result stops propagation: `operations` executes the command through the supplied backend, while `result` records the completed command without executing it. + ### Input Events #### input diff --git a/packages/coding-agent/src/core/extensions/runner.ts b/packages/coding-agent/src/core/extensions/runner.ts index c0ef16b2c..68d008694 100644 --- a/packages/coding-agent/src/core/extensions/runner.ts +++ b/packages/coding-agent/src/core/extensions/runner.ts @@ -118,6 +118,32 @@ const buildBuiltinKeybindings = (resolvedKeybindings: KeybindingsConfig): BuiltI return builtinKeybindings; }; +function isUserBashEventResult(value: unknown): value is UserBashEventResult { + if (typeof value !== "object" || value === null) return false; + const candidate = value as Record; + const hasOperations = candidate.operations !== undefined; + const hasResult = candidate.result !== undefined; + if (hasOperations === hasResult) return false; + + if (hasOperations) { + const operations = candidate.operations; + if (typeof operations !== "object" || operations === null) return false; + return typeof (operations as Record).exec === "function"; + } + + const result = candidate.result; + if (typeof result !== "object" || result === null) return false; + const resultRecord = result as Record; + return ( + typeof resultRecord.output === "string" && + "exitCode" in resultRecord && + (resultRecord.exitCode === undefined || typeof resultRecord.exitCode === "number") && + typeof resultRecord.cancelled === "boolean" && + typeof resultRecord.truncated === "boolean" && + (resultRecord.fullOutputPath === undefined || typeof resultRecord.fullOutputPath === "string") + ); +} + /** Combined result from all before_agent_start handlers. */ interface BeforeAgentStartCombinedResult { messages: NonNullable[]; @@ -1019,9 +1045,13 @@ export class ExtensionRunner { for (const handler of handlers) { try { const handlerResult = await handler(event, ctx); - if (handlerResult) { - return handlerResult as UserBashEventResult; + if (handlerResult === undefined) continue; + if (!isUserBashEventResult(handlerResult)) { + throw new Error( + "Invalid user_bash handler result: return undefined for local execution or exactly one valid { operations } or { result } object", + ); } + return handlerResult; } catch (err) { const message = err instanceof Error ? err.message : String(err); const stack = err instanceof Error ? err.stack : undefined; @@ -1031,6 +1061,7 @@ export class ExtensionRunner { error: message, stack, }); + throw err; } } } diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 2416de981..afbd82b74 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/packages/coding-agent/src/core/extensions/types.ts @@ -1134,12 +1134,17 @@ export interface ToolCallEventResult { } /** Result from user_bash event handler */ -export interface UserBashEventResult { - /** Custom operations to use for execution */ - operations?: BashOperations; - /** Full replacement: extension handled execution, use this result */ - result?: BashResult; -} +export type UserBashEventResult = + | { + /** Custom operations to use for execution */ + operations: BashOperations; + result?: never; + } + | { + operations?: never; + /** Full replacement: extension handled execution, use this result */ + result: BashResult; + }; export interface ToolResultEventResult { content?: (TextContent | ImageContent)[]; diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 64264288d..81d6e0dea 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -76,6 +76,7 @@ import type { ExtensionWidgetOptions, MarkdownTransformer, ProjectTrustContext, + UserBashEventResult, WorkingIndicatorOptions, } from "../../core/extensions/index.ts"; import { FooterDataProvider, type ReadonlyFooterDataProvider } from "../../core/footer-data-provider.ts"; @@ -6507,12 +6508,18 @@ export class InteractiveMode { const extensionRunner = this.session.extensionRunner; // Emit user_bash event to let extensions intercept - const eventResult = await extensionRunner.emitUserBash({ - type: "user_bash", - command, - excludeFromContext, - cwd: this.sessionManager.getCwd(), - }); + let eventResult: UserBashEventResult | undefined; + try { + eventResult = await extensionRunner.emitUserBash({ + type: "user_bash", + command, + excludeFromContext, + cwd: this.sessionManager.getCwd(), + }); + } catch { + // The extension runner already reported the error. Do not fall back to local execution. + return; + } // If extension returned a full result, use it directly if (eventResult?.result) { diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index c1fa4df13..88088685d 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -617,6 +617,84 @@ describe("ExtensionRunner", () => { expect(errors[0].error).toContain("Handler error!"); expect(errors[0].event).toBe("context"); }); + + // Regression test for #9068. + it("fails closed when a user_bash handler throws", async () => { + const extCode = ` + export default function(pi) { + pi.on("user_bash", async () => { + throw new Error("Routing failed"); + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "throws.ts"), extCode); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const errors: Array<{ event: string; error: string }> = []; + runner.onError((error) => errors.push(error)); + + await expect( + runner.emitUserBash({ type: "user_bash", command: "pwd", excludeFromContext: false, cwd: tempDir }), + ).rejects.toThrow("Routing failed"); + expect(errors).toMatchObject([{ event: "user_bash", error: "Routing failed" }]); + }); + + // Regression test for #9068. + it.each([ + ["an empty object", "{}"], + ["null operations", "{ operations: null }"], + ["operations without exec", "{ operations: {} }"], + ["a null result", "{ result: null }"], + ["an incomplete result", '{ result: { output: "handled" } }'], + [ + "operations and a result", + '{ operations: { exec: async () => ({ exitCode: 0 }) }, result: { output: "handled", exitCode: 0, cancelled: false, truncated: false } }', + ], + ])("fails closed when a user_bash handler returns %s", async (_description, handlerResult) => { + const extCode = ` + export default function(pi) { + pi.on("user_bash", async () => (${handlerResult})); + } + `; + fs.writeFileSync(path.join(extensionsDir, "invalid-result.ts"), extCode); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const errors: Array<{ event: string; error: string }> = []; + runner.onError((error) => errors.push(error)); + + await expect( + runner.emitUserBash({ type: "user_bash", command: "pwd", excludeFromContext: false, cwd: tempDir }), + ).rejects.toThrow("Invalid user_bash handler result"); + expect(errors).toMatchObject([ + { event: "user_bash", error: expect.stringContaining("Invalid user_bash handler result") }, + ]); + }); + + it("accepts valid user_bash operations and result overrides", async () => { + const extCode = ` + export default function(pi) { + pi.on("user_bash", async (event) => { + if (event.command === "operations") { + return { operations: { exec: async () => ({ exitCode: 0 }) } }; + } + return { result: { output: "handled", exitCode: 0, cancelled: false, truncated: false } }; + }); + } + `; + fs.writeFileSync(path.join(extensionsDir, "valid-results.ts"), extCode); + + const result = await discoverAndLoadExtensions([], tempDir, tempDir); + const runner = new ExtensionRunner(result.extensions, result.runtime, tempDir, sessionManager, modelRegistry); + const event = { type: "user_bash" as const, excludeFromContext: false, cwd: tempDir }; + + const operations = await runner.emitUserBash({ ...event, command: "operations" }); + expect(operations).toEqual({ operations: { exec: expect.any(Function) } }); + await expect(runner.emitUserBash({ ...event, command: "result" })).resolves.toEqual({ + result: { output: "handled", exitCode: 0, cancelled: false, truncated: false }, + }); + }); }); describe("message and entry renderers", () => { diff --git a/packages/coding-agent/test/suite/regressions/9068-user-bash-fail-closed.test.ts b/packages/coding-agent/test/suite/regressions/9068-user-bash-fail-closed.test.ts new file mode 100644 index 000000000..58c0ec18b --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/9068-user-bash-fail-closed.test.ts @@ -0,0 +1,272 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import type { AgentSessionRuntime } from "../../../src/core/agent-session-runtime.ts"; +import type { ExtensionAPI, UserBashEvent, UserBashEventResult } from "../../../src/core/extensions/types.ts"; +import { InteractiveMode } from "../../../src/modes/interactive/interactive-mode.ts"; +import { runRpcMode } from "../../../src/modes/rpc/rpc-mode.ts"; +import { createHarness, type Harness } from "../harness.ts"; + +// Regression for https://github.com/earendil-works/pi/issues/9068 + +const rpcIo = vi.hoisted(() => ({ + outputLines: [] as string[], + lineHandler: undefined as ((line: string) => void) | undefined, +})); + +vi.mock("../../../src/core/output-guard.js", () => ({ + flushRawStdout: vi.fn(async () => {}), + takeOverStdout: vi.fn(), + waitForRawStdoutBackpressure: vi.fn(async () => {}), + writeRawStdout: (line: string) => { + rpcIo.outputLines.push(line); + }, +})); + +vi.mock("../../../src/modes/interactive/theme/theme.js", () => ({ theme: {} })); + +vi.mock("../../../src/modes/interactive/components/bash-execution.js", () => ({ + BashExecutionComponent: class { + appendOutput(): void {} + setComplete(): void {} + }, +})); + +vi.mock("../../../src/modes/rpc/jsonl.js", () => ({ + attachJsonlLineReader: vi.fn((_stream: NodeJS.ReadableStream, onLine: (line: string) => void) => { + rpcIo.lineHandler = onLine; + return () => { + rpcIo.lineHandler = undefined; + }; + }), + serializeJsonLine: (value: unknown) => `${JSON.stringify(value)}\n`, +})); + +type NodeListener = Parameters[1]; + +type ListenerSnapshot = { + stdinEnd: NodeListener[]; + signals: Map; +}; + +function takeListenerSnapshot(): ListenerSnapshot { + const signals: NodeJS.Signals[] = process.platform === "win32" ? ["SIGTERM"] : ["SIGTERM", "SIGHUP"]; + return { + stdinEnd: process.stdin.listeners("end") as NodeListener[], + signals: new Map(signals.map((signal) => [signal, process.listeners(signal) as NodeListener[]])), + }; +} + +function restoreListeners(snapshot: ListenerSnapshot): void { + for (const listener of process.stdin.listeners("end") as NodeListener[]) { + if (!snapshot.stdinEnd.includes(listener)) process.stdin.off("end", listener); + } + for (const [signal, previousListeners] of snapshot.signals) { + for (const listener of process.listeners(signal) as NodeListener[]) { + if (!previousListeners.includes(listener)) process.off(signal, listener); + } + } +} + +function parseOutputLines(): Array> { + return rpcIo.outputLines + .flatMap((line) => line.split("\n")) + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line) as Record); +} + +function createRuntimeHost(harness: Harness): AgentSessionRuntime { + return { + session: harness.session, + newSession: vi.fn(async () => ({ cancelled: true })), + switchSession: vi.fn(async () => ({ cancelled: true })), + fork: vi.fn(async () => ({ cancelled: true, selectedText: "" })), + dispose: vi.fn(async () => {}), + setRebindSession: vi.fn(), + } as unknown as AgentSessionRuntime; +} + +async function startRpcHarness(extension: (pi: ExtensionAPI) => void): Promise<{ + harness: Harness; + send(command: Record): void; + cleanup(): void; +}> { + const listenerSnapshot = takeListenerSnapshot(); + const harness = await createHarness({ extensionFactories: [extension] }); + const cleanup = () => { + harness.cleanup(); + restoreListeners(listenerSnapshot); + }; + + try { + void runRpcMode(createRuntimeHost(harness)); + await vi.waitFor(() => expect(rpcIo.lineHandler).toBeDefined()); + } catch (error) { + cleanup(); + throw error; + } + + return { + harness, + send(command) { + if (!rpcIo.lineHandler) throw new Error("RPC line handler is not attached"); + rpcIo.lineHandler(JSON.stringify(command)); + }, + cleanup, + }; +} + +type InteractiveBashContext = { + defaultEditor: { onSubmit?: (text: string) => Promise | void }; + editor: { addToHistory?: (text: string) => void }; + session: Harness["session"]; + sessionManager: Harness["sessionManager"]; + ui: { requestRender(): void }; + chatContainer: { addChild(component: unknown): void }; + pendingMessagesContainer: { addChild(component: unknown): void }; + pendingBashComponents: unknown[]; + isBashMode: boolean; + handleBashCommand(command: string, excludeFromContext?: boolean): Promise; + showError(message: string): void; + updateEditorBorderColor(): void; +}; + +const interactiveModePrototype = InteractiveMode.prototype as unknown as { + setupEditorSubmitHandler(this: InteractiveBashContext): void; + handleBashCommand(this: InteractiveBashContext, command: string, excludeFromContext?: boolean): Promise; +}; + +const localResult = { + output: "local output", + exitCode: 0, + cancelled: false, + truncated: false, +}; + +const rpcCases: Array<{ + name: string; + extension: (pi: ExtensionAPI) => void; + error?: string; + executeCount: number; +}> = [ + { + name: "fails the request without executing bash when a handler throws", + extension: (pi) => { + pi.on("user_bash", async () => { + throw new Error("Routing failed"); + }); + }, + error: "Routing failed", + executeCount: 0, + }, + { + name: "fails the request without executing bash when a handler returns an empty result", + extension: (pi) => { + pi.on("user_bash", async () => ({}) as unknown as UserBashEventResult); + }, + error: "Invalid user_bash handler result", + executeCount: 0, + }, + { + name: "executes bash normally when a handler returns undefined", + extension: (pi) => { + pi.on("user_bash", async () => undefined); + }, + executeCount: 1, + }, +]; + +afterEach(() => { + rpcIo.outputLines = []; + rpcIo.lineHandler = undefined; +}); + +describe("RPC user_bash failure handling (#9068)", () => { + test.each(rpcCases)("$name", async ({ extension, error, executeCount }) => { + const rpc = await startRpcHarness(extension); + const executeBash = vi.spyOn(rpc.harness.session, "executeBash").mockResolvedValue(localResult); + + try { + rpc.send({ id: "bash-request", type: "bash", command: "pwd" }); + + await vi.waitFor(() => { + const response = parseOutputLines().find( + (output) => output.type === "response" && output.id === "bash-request", + ); + expect(response).toMatchObject({ + id: "bash-request", + type: "response", + command: "bash", + success: error === undefined, + ...(error ? { error: expect.stringContaining(error) } : { data: localResult }), + }); + }); + + if (error) { + expect(parseOutputLines()).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + type: "extension_error", + event: "user_bash", + error: expect.stringContaining(error), + }), + ]), + ); + } + expect(executeBash).toHaveBeenCalledTimes(executeCount); + } finally { + executeBash.mockRestore(); + rpc.cleanup(); + } + }); +}); + +describe("Interactive user_bash failure handling (#9068)", () => { + test.each([ + ["!pwd", false], + ["!!pwd", true], + ])("fails closed for %s when a handler returns an empty result", async (input, excludeFromContext) => { + const events: UserBashEvent[] = []; + const harness = await createHarness({ + extensionFactories: [ + (pi) => { + pi.on("user_bash", async (event) => { + events.push(event); + return {} as unknown as UserBashEventResult; + }); + }, + ], + }); + const executeBash = vi.spyOn(harness.session, "executeBash").mockResolvedValue(localResult); + const context: InteractiveBashContext = { + defaultEditor: {}, + editor: { addToHistory: vi.fn() }, + session: harness.session, + sessionManager: harness.sessionManager, + ui: { requestRender: vi.fn() }, + chatContainer: { addChild: vi.fn() }, + pendingMessagesContainer: { addChild: vi.fn() }, + pendingBashComponents: [], + isBashMode: true, + handleBashCommand: interactiveModePrototype.handleBashCommand, + showError: vi.fn(), + updateEditorBorderColor: vi.fn(), + }; + interactiveModePrototype.setupEditorSubmitHandler.call(context); + + try { + await context.defaultEditor.onSubmit?.(input); + + expect(events).toEqual([ + { + type: "user_bash", + command: "pwd", + excludeFromContext, + cwd: harness.sessionManager.getCwd(), + }, + ]); + expect(executeBash).not.toHaveBeenCalled(); + } finally { + executeBash.mockRestore(); + harness.cleanup(); + } + }); +});