Reviewed and proposed improvements for bash execution PR

Summary

In Progress

Understand
Reviewed PR details, linked issues, and code changes.
Understand
Understand
Analyzed error handling and use cases.
Understand
Understand
Identified inconsistencies and design flaws.
Understand
Plan
Proposed a simplified, composable hook design.
Plan

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/903

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me now check for linked issues and read the current state of relevant files:

Now let me read the relevant source files to understand the existing implementation:

Let me also read the bash-executor and agent-session files:

Now let me read the tools/bash.ts file to see the current implementation:

Now let me check the test file referenced in the PR:

Now let me check the index exports:

Now I have a good understanding of the PR. Let me review the README and docs to see if documentation changes are needed:

Now I have a comprehensive understanding of the PR. Let me provide the review:

PR: https://github.com/badlogic/pi-mono/pull/903

Changelog:

Missing changelog entry in packages/coding-agent/CHANGELOG.md. A changelog entry is required before merge. If the user decides to merge, I will add it.

Good:

Clean separation of concerns: before_bash_exec event gives extensions a unified hook for both tool-based and user (!/!!) bash execution
The uv.ts example demonstrates a practical use case (Python/uv interception) and serves as good documentation
BashExecBlockResult allows extensions to block bash commands with custom reasons
Environment variable manipulation via env override with undefined to remove keys is intuitive
The originalCommand field preserves the original command for logging/debugging while allowing command modification
The isError flag in tool_result allows extensions to convert successful tool results to errors (useful for policy enforcement)
Type exports are comprehensive: BashExecEvent, BashExecOverrides, BashExecSource, BeforeBashExecEvent, BeforeBashExecEventResult
Documentation in extensions.md and README.md is clear with good examples
Test update correctly mocks resolveShellExecutionOptions instead of getShellConfig to match the new implementation

Bad:

The PR extends BashOperations.exec interface with new optional parameters (env, shell, args) but doesn't update all callers. The executeBashWithOperations function passes these new parameters to operations.exec, but existing implementations (like in ssh.ts example) don't expect them. This could break extensions using custom BashOperations.
The BashExecutionMessage.executedCommand field is added but never read/used anywhere in the codebase. It's stored in session history but not surfaced in UI or used for replay.
The tool_result error handling in wrapper.ts has complex branching logic for forcedError and isError. The flow is:
1. If handler returns isError: true on success, throw error with content as message
2. If handler returns content on existing error without isError: true, the original error is rethrown (content ignored)
3. If handler returns content with isError: true on existing error, content becomes new error message
This is confusing and the !resultResult.isError check on line 167 seems inverted (it throws the original error when NOT an error, but that's inside the catch block for errors).

Ugly:

The applyBeforeBashExecOverrides function in wrapper.ts is defined inside wrapToolWithExtensions, creating a new function definition on every call. This should be hoisted outside.
The PR description admits the approach of "abusing content to carry error messages" feels "somewhat consistent to how it works but a bit ugly." The semantic mismatch between "content" (for display) and "error message" (for Error constructor) creates a leaky abstraction.
resolveShellExecutionOptions in shell.ts has redundant undefined-deletion loop. If options?.env is passed, we create a new object from it, then iterate to delete undefined keys. But the delete-undefined loop also runs on getShellEnv() fallback which shouldn't have undefined values.

Questions or Assumptions:

Should the executedCommand field be displayed in the bash execution UI component when it differs from command? Currently it's stored but invisible.
Is the intention that before_bash_exec handlers are chained? The implementation chains them (currentEvent = applyBashExecOverrides(currentEvent, overrideResult)), but this isn't documented.
Should blocking return a BashResult with an error message instead of throwing? The current behavior throws which may not provide the best UX in all cases.
The timeout override in before_bash_exec could allow extensions to extend or override timeouts. Is this intentional?

Change summary:

Adds before_bash_exec extension event fired before bash execution (both tool and user !/!!)
Extensions can rewrite command, cwd, env, shell, args, timeout, or block execution entirely
Adds executedCommand to session history when command is rewritten
Extends tool_result event to allow modifying error messages via isError: true + content
Adds resolveShellExecutionOptions helper for unified shell config resolution
Adds uv.ts example extension demonstrating Python/uv interception
Updates documentation in extensions.md and README.md

Tests:

Single test update: mocks resolveShellExecutionOptions instead of getShellConfig for spawn error test
No new tests for the before_bash_exec event handler
No tests for the tool_result error manipulation flow
No tests for chaining multiple before_bash_exec handlers
No tests for blocking execution with { block: true }

what is executeCommand supposed to do?

Looking at the PR diff, executedCommand (not executeCommand) is a new optional field added to BashExecutionMessage:

typescript

export interface BashExecutionMessage {	role: "bashExecution";	command: string;	executedCommand?: string;  // NEW	output: string;	exitCode: number | undefined;	// ...}

export interface BashExecutionMessage {	role: "bashExecution";	command: string;	executedCommand?: string;  // NEW	output: string;	exitCode: number | undefined;	// ...}

It's set in recordBashResult() when the command was rewritten by before_bash_exec handlers:

typescript

this.recordBashResult(command, result, {    excludeFromContext: options?.excludeFromContext,    executedCommand: execCommand === command ? undefined : execCommand,});

this.recordBashResult(command, result, {    excludeFromContext: options?.excludeFromContext,    executedCommand: execCommand === command ? undefined : execCommand,});

The purpose is to preserve what command was actually executed when it differs from the original. For example, with the uv.ts extension:

User runs: python script.py
before_bash_exec rewrites to: uv run python script.py
Session stores: command: "python script.py", executedCommand: "uv run python script.py"

This allows:

The UI to show the original command the user/model requested
Session history to record what actually ran (for debugging, audit, replay)

However, as I noted in the review, the field is stored but never actually used anywhere. The BashExecutionComponent doesn't render it, and there's no logic that reads it back. It's essentially dead data in the current implementation.

this seems like a shitty PR overall

Yeah, it's rough. The core problems:

Incomplete implementation - executedCommand is stored but never displayed or used. Dead code.
Breaking change to BashOperations - Adds env, shell, args params to the interface without updating existing implementations like ssh.ts. Extensions using custom BashOperations will silently ignore these overrides.
No tests - Zero tests for the new before_bash_exec event, blocking, chaining, or the tool_result error manipulation.
Confusing error handling - The tool_result isError logic in wrapper.ts is convoluted. The PR author admits the approach is "ugly" in the description.
Function defined inside function - applyBeforeBashExecOverrides recreated on every tool call.
Scope creep - Mixes two unrelated features: bash interception AND tool_result error manipulation. Should be separate PRs.

The uv.ts example is nice, but it's lipstick on a pig. The feature isn't production-ready without tests and the BashOperations interface change needs a migration path or at least documentation about breaking existing extensions.

I'd request significant revisions or close it and ask for a cleaner implementation.

can we have a cleaner, more concise implementation? explain the error handling in detail to me first

The error handling in wrapper.ts has two paths:

Success path (tool executed without throwing):

typescript

const result = await tool.execute(...);
if (runner.hasHandlers("tool_result")) {    const resultResult = await runner.emit({..., isError: false});        if (resultResult) {        if (resultResult.isError) {            // Extension wants to FORCE this successful result into an error            forcedError = true;            throw new Error(contentToString(resultResult.content));        }        // Otherwise just modify content/details        return { content: resultResult.content, details: resultResult.details };    }}return result;

const result = await tool.execute(...);
if (runner.hasHandlers("tool_result")) {    const resultResult = await runner.emit({..., isError: false});        if (resultResult) {        if (resultResult.isError) {            // Extension wants to FORCE this successful result into an error            forcedError = true;            throw new Error(contentToString(resultResult.content));        }        // Otherwise just modify content/details        return { content: resultResult.content, details: resultResult.details };    }}return result;

Error path (tool threw):

typescript

catch (err) {    if (forcedError) throw err;  // Already handled above, just rethrow        if (runner.hasHandlers("tool_result")) {        const resultResult = await runner.emit({..., isError: true});                if (resultResult) {            if (!resultResult.isError) {                // Extension returned content but didn't set isError: true                // So we ignore the content and rethrow original                throw err;            }            // Extension wants to REPLACE the error message            throw new Error(contentToString(resultResult.content));        }    }    throw err;}

catch (err) {    if (forcedError) throw err;  // Already handled above, just rethrow        if (runner.hasHandlers("tool_result")) {        const resultResult = await runner.emit({..., isError: true});                if (resultResult) {            if (!resultResult.isError) {                // Extension returned content but didn't set isError: true                // So we ignore the content and rethrow original                throw err;            }            // Extension wants to REPLACE the error message            throw new Error(contentToString(resultResult.content));        }    }    throw err;}

The confusion:

On success, isError: true in the result converts success to error
On error, you MUST return isError: true to replace the error message, otherwise your content is ignored
There's no way to convert an error into a success (swallow the error)

The !resultResult.isError check on line 167 says: "if the extension responded to an error event but didn't explicitly say isError: true, ignore whatever they returned and throw the original error."

This is defensive but confusing. Why emit the event if you're going to ignore the response?

what in the fuck, i don't understand what this is trying to do, what's the use case? how'd that look?

From the PR description and the uv.ts example, here's the use case:

Scenario: Python script fails with ModuleNotFoundError. The extension wants to append a hint to the error message telling the model to use uv run --with <module>.

typescript

// uv.ts example from the PRpi.on("tool_result", (event) => {    if (!isBashToolResult(event)) return;        // Check if output has Python import error    if (!TRACEBACK_PATTERN.test(text) || !IMPORT_ERROR_PATTERN.test(text)) {        return;    }        // Append hint to the error    const hint = "\n\nHint: Use uv run --with <module> ...";    return {        content: [...event.content, { type: "text", text: hint }],        isError: true,  // <-- Must set this or hint is ignored    };});

// uv.ts example from the PRpi.on("tool_result", (event) => {    if (!isBashToolResult(event)) return;        // Check if output has Python import error    if (!TRACEBACK_PATTERN.test(text) || !IMPORT_ERROR_PATTERN.test(text)) {        return;    }        // Append hint to the error    const hint = "\n\nHint: Use uv run --with <module> ...";    return {        content: [...event.content, { type: "text", text: hint }],        isError: true,  // <-- Must set this or hint is ignored    };});

The problem: Bash tool throws on non-zero exit. The extension catches tool_result with isError: true, wants to modify the error message. But the current API requires you to return isError: true again or your modifications are silently discarded.

Simpler design:

Just let extensions modify content regardless. If the tool errored, the modified content becomes the new error message. If it succeeded, the modified content is the new result. No isError flag gymnastics.

typescript

pi.on("tool_result", (event) => {    if (event.isError && isPythonImportError(event.content)) {        return { content: [...event.content, { type: "text", text: hint }] };    }});

pi.on("tool_result", (event) => {    if (event.isError && isPythonImportError(event.content)) {        return { content: [...event.content, { type: "text", text: hint }] };    }});

The whole isError return flag is overengineered. The only legitimate use case for changing error status would be converting success to error (policy blocking), which could be a separate { block: true, reason: "..." } return like tool_call already has.

return { content: [...event.content, { type: "text", text: hint }], isError: true, // <-- Must set this or hint is ignored };

why in the fuck do we need isError: true here? tool_result event handler can modify the returned result without having to rewrite the error state? the model will receive the additional content just fine, no?

Exactly. The isError flag is completely unnecessary for this use case.

When bash exits non-zero, the tool throws. The error message becomes the tool result content sent to the model. If an extension appends a hint to that content, the model receives the modified content. Done.

The model doesn't care whether it was an "error" or not. It just sees the text content in the tool result. Whether that text says "Command failed" or "Command failed\n\nHint: try X" makes no difference to the protocol.

The only place isError matters is internally: do we throw or return? But the extension shouldn't control that. The tool already decided by throwing or not.

So the entire isError machinery in this PR is pointless complexity. Extensions should just be able to:

typescript

pi.on("tool_result", (event) => {    if (shouldAddHint(event)) {        return { content: [...event.content, { type: "text", text: hint }] };    }});

pi.on("tool_result", (event) => {    if (shouldAddHint(event)) {        return { content: [...event.content, { type: "text", text: hint }] };    }});

And that content modification should work regardless of whether the tool succeeded or failed. The PR overengineered this by conflating "modify the content" with "change the error state."

what other inconsistencies are there?

Looking at the PR again:

1. before_bash_exec vs user_bash overlap

There's already a user_bash event that fires for !/!! commands. Now before_bash_exec also fires for user bash with source: "user_bash". Two events for the same thing, different capabilities:

user_bash can return custom BashOperations or a full BashResult
before_bash_exec can modify command/env/cwd but not provide custom operations

Why not just extend user_bash?

2. Tool bash gets before_bash_exec, but not custom operations

For user bash, you can provide custom BashOperations via user_bash event. For tool bash, you can only modify parameters via before_bash_exec. No way to redirect tool bash to SSH without replacing the entire tool.

3. BashOperations interface extended but not consistently

The PR adds env, shell, args to the exec call in defaultBashOperations, but BashOperations interface signature in the types still shows:

Extensions implementing custom BashOperations won't receive the new params.

4. executedCommand stored but invisible

As discussed. Dead field.

5. Timeout handling inconsistent

before_bash_exec can override timeout, but for user bash (agent-session.ts), the timeout isn't even a concept. User bash has no timeout parameter to override.

6. No way to observe what actually executed

Extensions can modify the command, but there's no after_bash_exec event to see the final result with the modified command context.

what would be a better implementation? more concise, more consistent?

while still supporting what armin does in his uv.ts stuff. note that before_bash_exec is composable as oposed to user_bash or overwriting the bash tool, which i believe is the point of armin's exercise

The core insight is right: a composable hook that intercepts bash before execution, works for both tool and user bash, and lets multiple extensions chain modifications.

Simplified design:

Single event: before_bash_exec - fires for all bash (tool + user), composable
Drop the isError return gymnastics - tool_result just modifies content, period
Remove executedCommand storage - if you need it, add an after_bash_exec event later
Don't extend BashOperations interface - that's a different feature (remote execution)

The event:

typescript

interface BeforeBashExecEvent {    type: "before_bash_exec";    source: "tool" | "user";    command: string;    cwd: string;    env: Record<string, string>;    timeout?: number;}
type BeforeBashExecResult =     | { block: true; reason?: string }    | { command?: string; cwd?: string; env?: Record<string, string>; timeout?: number };

interface BeforeBashExecEvent {    type: "before_bash_exec";    source: "tool" | "user";    command: string;    cwd: string;    env: Record<string, string>;    timeout?: number;}
type BeforeBashExecResult =     | { block: true; reason?: string }    | { command?: string; cwd?: string; env?: Record<string, string>; timeout?: number };

No shell, no args (internal details). No originalCommand (handler already has it in the event). Handlers chain by mutating fields they care about.

Implementation:

One function emitBeforeBashExec() in runner. Call it from:

wrapToolWithExtensions for bash tool (before execute)
agent-session.executeBash for user bash (before execute)

For tool_result:

Just remove the isError return field entirely. Extensions return { content } to modify, that's it. The tool already threw or didn't. The model receives whatever content you return.

This cuts the PR roughly in half and removes all the confusing parts while keeping the composable interception that makes uv.ts work.