fix(coding-agent): fail closed on user bash hook errors (#9662)

* fix(coding-agent): fail closed on user bash hook errors

Prevent failed execution-routing hooks from falling back to the local shell.

Fixes #9068

* fix(coding-agent): validate user bash hook results

* docs(coding-agent): clarify user bash result semantics

* docs(coding-agent): streamline user bash semantics

* fix(coding-agent): strengthen user bash regressions

* docs(coding-agent): clarify user bash breaking change

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)).

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

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<string, unknown>;
+	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<string, unknown>).exec === "function";
+	}
+
+	const result = candidate.result;
+	if (typeof result !== "object" || result === null) return false;
+	const resultRecord = result as Record<string, unknown>;
+	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<BeforeAgentStartEventResult["message"]>[];
@@ -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;
 				}
 			}
 		}

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)[];

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) {

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", () => {

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<typeof process.on>[1];
+
+type ListenerSnapshot = {
+	stdinEnd: NodeListener[];
+	signals: Map<NodeJS.Signals, NodeListener[]>;
+};
+
+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<Record<string, unknown>> {
+	return rpcIo.outputLines
+		.flatMap((line) => line.split("\n"))
+		.filter((line) => line.trim().length > 0)
+		.map((line) => JSON.parse(line) as Record<string, unknown>);
+}
+
+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<string, unknown>): 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> | 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<void>;
+	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<void>;
+};
+
+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();
+		}
+	});
+});