Completed detailed PR review for pi-mono #903

Summary

Closed, Completed

Understand
Reviewed PR description, comments, commits, and changed files.
Understand
Understand
Identified and read linked issues.
Understand
Understand
Analyzed PR diff and relevant code files.
Understand
Understand
Checked documentation and implementation details.
Understand
Verify
Provided structured review with feedback and changelog note.
Verify

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 read the relevant files to fully understand the context and validate the implementation:

Readagent-session.ts

Readtypes.ts

Readrunner.ts

Readwrapper.ts

Readshell.ts

ReadCHANGELOG.md

Readbash-executor.ts

Readindex.ts

Readextensions.md

Let me check the README.md and understand what documentation changes might be needed:

The PR is targeting a specific base commit that's not on my current HEAD. Let me understand the diff better and analyze it:

From the PR diff, I can see:

New types added in types.ts:
- BashExecSource - "tool" | "user_bash"
- BashExecOverrides - interface for command/cwd/env/shell/args overrides
- BashExecEvent - base event with command, originalCommand, cwd, env, shell, args
- BeforeBashExecEvent - extends BashExecEvent with type: "before_bash_exec"
- BeforeBashExecEventResult - alias for BashExecOverrides
New method in runner.ts:
- emitBeforeBashExec(event) - emits the new event to extensions
Agent session changes (agent-session.ts):
- Now calls the new before_bash_exec event for user bash commands (! prefix)
Wrapper changes (wrapper.ts):
- Now intercepts the bash tool and emits before_bash_exec event
New example extension (uv.ts):
- Demonstrates intercepting Python commands and redirecting through uv
Exports in index.ts:
- Exports the new types

Now let me evaluate the PR:

Issues I see:

Incomplete implementation for bash tool: The wrapper.ts changes intercept the bash tool, but the execEvent values like cwd, env, shell, args are not being passed to the actual bash executor. The executeBash and executeBashWithOperations functions in bash-executor.ts don't accept these parameters. The only thing being passed is execEvent.command. So the extensions can modify cwd, env, shell, args but they're ignored.

The user bash path in agent-session.ts has the same problem—it doesn't apply the overrides to actual execution. Additionally, some core types like BashExecSource, BashExecOverrides, and BashExecEvent aren't exported from index.ts, only the event and result types are. The documentation in docs/extensions.md also doesn't cover the new before_bash_exec event, only mentioning user_bash.

There's no changelog entry for this feature, and the models.generated.ts changes appear to be unrelated model updates that should probably be in a separate commit. I need to check how the bash-executor is actually being invoked to understand the full picture.

Let me verify a key implementation detail - whether the bash-executor can accept the overridden values:

Readbash.ts

Now I understand better. The bash tool has a BashOperations interface that allows custom execution. But looking at the PR:

In wrapper.ts, the PR intercepts the bash tool and emits before_bash_exec, but only uses execEvent.command to update the params. The cwd, env, shell, args overrides are computed but NOT passed to the actual execution.
In agent-session.ts, for user bash (!), the event is emitted but only the command is used. The cwd, env, shell, args are not passed.

This is a significant gap in the implementation. The PR adds the ability for extensions to return these overrides, but they're not being applied.

Let me also check what the example extension uv.ts expects:

Looking at uv.ts:

typescript

pi.on("before_bash_exec", (event) => {    // ...    return {        command: uvCommand,  // Only returns command    };});

pi.on("before_bash_exec", (event) => {    // ...    return {        command: uvCommand,  // Only returns command    };});

The example only modifies command, so it would work. But the types advertise that extensions can also modify cwd, env, shell, args, which would not work.

Summary of Issues:

Incomplete implementation: The cwd, env, shell, args overrides are not applied to actual bash execution
Missing exports: BashExecSource, BashExecOverrides, BashExecEvent are not exported
Missing documentation: No documentation update for the new before_bash_exec event
Missing changelog: No changelog entry
Unrelated changes: models.generated.ts changes should be a separate commit

Let me write up the review.

Now I have a complete picture. Let me provide the structured review.

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

Changelog:

Missing. A changelog entry is required before merge. I will add it if you decide to merge.

Good:

Clean event design: BeforeBashExecEvent is well-structured with source ("tool" | "user_bash"), originalCommand, and full execution context (cwd, env, shell, args)
Consistent chaining pattern in emitBeforeBashExec() using applyBashExecOverrides follows established patterns in the codebase
The uv.ts example is practical and demonstrates a real use case (redirecting Python through uv)
Event fires for both tool-invoked bash and user bash (! prefix), providing unified interception

Bad:

Incomplete implementation of overrides: Extensions can return cwd, env, shell, args overrides, but these are NOT applied to actual execution:
- In wrapper.ts (lines 81-95): only execEvent.command is used to update effectiveParams, the other overrides are ignored
- In agent-session.ts (lines 1776-1792): only execEvent.command is passed to executeBash/executeBashWithOperations, the cwd, env, shell, args overrides are computed but discarded
- The underlying executeBash() and executeBashWithOperations() functions don't even accept these parameters
Missing exports: BashExecSource, BashExecOverrides, BashExecEvent types are defined but not exported in index.ts. Only BeforeBashExecEvent and BeforeBashExecEventResult are exported.
Unrelated changes in PR: The models.generated.ts diff contains model pricing updates (cacheWrite values, removal of qwen/qwen2.5-vl-72b-instruct, cacheRead change for a model) that are unrelated to bash interception. These should be a separate commit.
No documentation: The docs/extensions.md documents user_bash but the new before_bash_exec event is not documented.

Ugly:

API promises more than it delivers: The BeforeBashExecEventResult type (aliased to BashExecOverrides) advertises that extensions can override cwd, env, shell, args, but these are silently ignored. This is a footgun for extension authors who will expect these to work based on the types.

Questions or Assumptions:

Was the intent to only support command rewriting initially, with cwd/env/shell/args planned for a follow-up? If so, the types should be narrowed or documented as "not yet implemented".
Should before_bash_exec be emitted for user bash (! prefix) at all? The existing user_bash event already handles that case. Having both events fire could be confusing.

Change summary:

Adds before_bash_exec extension event for intercepting bash commands before execution
Fires for both bash tool calls and user ! commands
Extensions can return a modified command (other overrides are defined but not implemented)
Includes example extension uv.ts demonstrating Python-to-uv redirection
Contains unrelated model pricing updates in models.generated.ts

Tests:

No tests added for the new functionality