Reviewed and merged GitHub PR #787

Summary

Closed, Completed

Understand
Reviewed PR description, comments, commits, and linked issues.
Understand
Understand
Analyzed relevant source files and code diffs.
Understand
Edit
Fixed issues and updated the branch.
Edit
Verify
Confirmed fixes and branch cleanliness.
Verify
Cleanup
Merged PR into main and cleaned up branch.
Cleanup

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

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 source files to understand the full context:

Readagent-session.ts200 lines

Readbranch-summarization.ts

Readtypes.ts150 lines

ReadCHANGELOG.md50 lines

Let me read more of the types file and check how appendLabelChange works:

Readtypes.ts80 lines

Readsession-manager.ts40 lines

Readinteractive-mode.ts50 lines

Readrunner.ts30 lines

I see there are more places using navigateTree:

types.ts line 240 - ExtensionContext.navigateTree - NOT updated
types.ts line 920 - ExtensionCommandContextActions.navigateTree - updated
runner.ts line 57 - NavigateTreeHandler type - updated
runner.ts line 349 - passing through to handler
print-mode.ts - NOT updated
rpc-mode.ts - NOT updated

The PR updates:

runner.ts line 57 (NavigateTreeHandler type)
types.ts line 920 (ExtensionCommandContextActions.navigateTree)
interactive-mode.ts line 719 (the actual handler)

But it misses:

types.ts line 240 (ExtensionContext.navigateTree)
print-mode.ts line 97-98
rpc-mode.ts line 313-314

Let me check these files to see if this is actually a problem:

Let me check the other places that use navigateTree to verify consistency:

Readtypes.ts25 lines

Readprint-mode.ts30 lines

Readrpc-mode.ts30 lines

I see the issue. The PR updates:

NavigateTreeHandler type in runner.ts
ExtensionCommandContextActions.navigateTree in types.ts (line 920 area)
interactive-mode.ts's handler implementation

But it does NOT update:

ExtensionContext.navigateTree in types.ts (line 240) - this is the interface that extensions use directly
print-mode.ts handler (line 97-98) - doesn't pass through the new options
rpc-mode.ts handler (line 313-314) - doesn't pass through the new options

This is a bug. The type signature was updated for ExtensionCommandContextActions and NavigateTreeHandler but:

The ExtensionContext interface still has the old signature
print-mode and rpc-mode don't pass the new options through

Let me check for documentation changes needed and look for tests:

Let me check for any tests and documentation that might need updates:

Readextensions.md25 lines

Readsdk.md25 lines

Readtree.md80 lines

Now I have a comprehensive understanding. Let me summarize the issues:

Missing type updates:

ExtensionContext.navigateTree in types.ts (line 240) - still has old signature
print-mode.ts (line 97-98) - doesn't pass new options through
rpc-mode.ts (line 313-314) - doesn't pass new options through

Documentation needs updating:

docs/extensions.md - line 729+ shows only summarize option
docs/sdk.md - line 113 has old signature
docs/tree.md - lines 108+ have outdated interface definitions for:
- navigateTree options
- TreePreparation interface
- SessionBeforeTreeResult interface

No changelog entry exists.

No tests for new functionality:

No test for replaceInstructions option
No test for label option
No test for extension override of these options

Let me verify the current state of branch-summarization.ts more closely to make sure my understanding is correct:

Now let me run the type check to see if there are any type errors with the current PR:

Readtypes.ts50 lines

I found it. There's ExtensionCommandContext which extends ExtensionContext and has its own navigateTree at line 240. This signature was NOT updated in the PR.

So there are actually two separate navigateTree signatures:1. ExtensionCommandContext.navigateTree (line 240) - NOT UPDATED - this is what extensions would directly call 2. ExtensionCommandContextActions.navigateTree (line 920 area) - UPDATED - this is the callback passed to the RunnerThe PR updates ExtensionCommandContextActions but not ExtensionCommandContext. This is a bug! The ExtensionCommandContext is what extension commands actually use when they call ctx.navigateTree(...).

Let me double check how runner.ts creates the context to see if there's some magic that makes this work:

Readrunner.ts30 lines

I see. In createCommandContext(), the navigateTree function is created by calling this.navigateTreeHandler(targetId, options). The NavigateTreeHandler type was updated to include the new options, so the runtime code at line 349 passes through options correctly.

However, the type definition for ExtensionCommandContext.navigateTree at line 240 still has the old signature:

typescript

navigateTree(targetId: string, options?: { summarize?: boolean }): Promise<{ cancelled: boolean }>;

navigateTree(targetId: string, options?: { summarize?: boolean }): Promise<{ cancelled: boolean }>;

This is a type mismatch. The runtime code will work (it passes through options), but TypeScript won't allow extensions to use the new options because the interface type doesn't include them.

So TypeScript will compile fine (the implementation is compatible with the narrow interface type), but extensions that try to use ctx.navigateTree(targetId, { customInstructions: "...", replaceInstructions: true, label: "foo" }) will get a type error.

This is a bug. The PR author needs to also update ExtensionCommandContext.navigateTree in types.ts around line 240.

Summary of issues:

Missing type update: ExtensionCommandContext.navigateTree (types.ts line 240) not updated
Missing handler updates: print-mode.ts and rpc-mode.ts don't pass the new options
Missing changelog entry
Missing documentation updates: docs/extensions.md, docs/sdk.md, docs/tree.md need updates
Missing tests for the new options

Let me go back to main:

Now I have all the information needed to provide a complete review. Let me summarize:

PR Summary: The PR adds the ability to:

Attach labels to branch summary entries (or target entries when not summarizing)
Override the summarization prompt entirely via replaceInstructions option
Allow extensions to override customInstructions, replaceInstructions, and label via SessionBeforeTreeResult

Good:

Clean implementation of the new options in agent-session.ts
Good JSDoc comments
Extension can now override these values via the session_before_tree event result
Label is applied to summary entry if summarizing, otherwise to target entry

Bad/Missing:

ExtensionCommandContext.navigateTree (types.ts line 240) still has old signature - extensions can't use new options with type safety
print-mode.ts and rpc-mode.ts don't pass the new options through their handlers
No changelog entry
Documentation not updated (extensions.md, sdk.md, tree.md)
No tests for new options

Ugly:

The inconsistency between ExtensionCommandContext (not updated) and ExtensionCommandContextActions (updated) is confusing and will cause type errors for extension authors

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

Changelog:

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

Good:

Clean implementation in agent-session.ts for passing replaceInstructions and label options through the navigation flow
Extensions can now override customInstructions, replaceInstructions, and label via SessionBeforeTreeResult return value from session_before_tree handlers
Label is correctly applied to summary entry when summarizing, or to target entry when not summarizing (useful for "review on empty branch" use case)
Good JSDoc documentation on the new interface fields
The TreePreparation event data now includes the options so extensions have full visibility

Bad:

Missing type update: ExtensionCommandContext.navigateTree (types.ts:240) still has old signature { summarize?: boolean }. Extensions calling ctx.navigateTree() won't have type-safe access to the new options. Only ExtensionCommandContextActions was updated.
Missing handler updates: print-mode.ts:97-98 and rpc-mode.ts:313-314 don't pass the new options through to session.navigateTree(). They only pass { summarize: options?.summarize }, dropping any customInstructions, replaceInstructions, or label options.
No tests: No tests for the new replaceInstructions, label, or extension override behavior.
Documentation not updated:
- docs/extensions.md:729+ - shows only summarize option
- docs/sdk.md:113 - old signature
- docs/tree.md:108+ - old interface definitions for navigateTree, TreePreparation, and SessionBeforeTreeResult

Ugly:

The type inconsistency between ExtensionCommandContext.navigateTree (not updated) and ExtensionCommandContextActions.navigateTree (updated) means extensions will get TypeScript errors if they try to use the new options, but the runtime code will actually work (runner.ts:349 passes options through). This is a footgun.

Questions or Assumptions:

None. The PR description explains the use case (review on empty branch feature).

Change summary:

agent-session.ts: navigateTree() now accepts replaceInstructions and label options. Label is attached via sessionManager.appendLabelChange() to either the summary entry or target entry.
branch-summarization.ts: generateBranchSummary() now accepts replaceInstructions option. When true, customInstructions completely replaces the default BRANCH_SUMMARY_PROMPT instead of being appended as "Additional focus".
types.ts: TreePreparation and SessionBeforeTreeResult interfaces extended with new fields. ExtensionCommandContextActions.navigateTree updated.
runner.ts: NavigateTreeHandler type updated with new options.
interactive-mode.ts: Handler implementation now passes all new options through.

Tests:

Existing agent-session-tree-navigation.test.ts tests navigation and summarization but doesn't cover the new options.
No new tests added for replaceInstructions, label, or extension override scenarios.

ok, check out into a local branch, fix up the things you identified.

Readtypes.ts15 lines

Editedtypes.ts

Readprint-mode.ts15 lines

Readrpc-mode.ts15 lines

Editedprint-mode.ts

Editedrpc-mode.ts

Now let me update the documentation:

Readextensions.md20 lines

Editedextensions.md

Readsdk.md15 lines

Editedsdk.md

Readtree.md80 lines

Editedtree.md

Now let me add the changelog entry:

ReadCHANGELOG.md30 lines

EditedCHANGELOG.md

Now let me run the check to verify everything compiles:

All checks pass. Here's a summary of the fixes:

Fixed:

ExtensionCommandContext.navigateTree type (types.ts:240) - Now includes customInstructions, replaceInstructions, and label options so extensions get type-safe access
print-mode.ts handler - Now passes all new options through to session.navigateTree()
rpc-mode.ts handler - Now passes all new options through to session.navigateTree()
Documentation updates:
- docs/extensions.md - Added example with new options and option descriptions
- docs/sdk.md - Updated navigateTree signature
- docs/tree.md - Updated navigateTree signature, TreePreparation interface, and SessionBeforeTreeResult interface. Also fixed stale model field in SessionBeforeTreeEvent (removed, as the actual code doesn't pass model).
Changelog entry - Added under ### Added with PR link and author attribution

rebase the branch on main

ReadCHANGELOG.md40 lines

EditedCHANGELOG.md

Done. Branch pr-787 is now rebased on main with my fixes on top:

107554ee Fix navigateTree API: add missing type updates, handler passthrough, and docs6b6707f3 Improve navigateTree API  (original PR commit)ffdc8d68 docs: add OpenCode Zen to provider lists in coding-agent README  (main)

107554ee Fix navigateTree API: add missing type updates, handler passthrough, and docs6b6707f3 Improve navigateTree API  (original PR commit)ffdc8d68 docs: add OpenCode Zen to provider lists in coding-agent README  (main)

All checks pass. The branch is ready for review/merge.

holy shit that added coding-agent/binaries ... unfuck that

Fixed. Branch is clean now:

572ec64d Fix navigateTree API: add missing type updates, handler passthrough, and docs6b6707f3 Improve navigateTree APIffdc8d68 docs: add OpenCode Zen to provider lists...  (main)

572ec64d Fix navigateTree API: add missing type updates, handler passthrough, and docs6b6707f3 Improve navigateTree APIffdc8d68 docs: add OpenCode Zen to provider lists...  (main)

11 files changed total (PR's 5 original files + my 6 additional fixes).

ok, looks good, merge into main