fix(planners): allow BuiltInPlanner subclasses to override process_planning_response

[maru0804]

Summary

Fixes #4133

Problem

When users create a subclass of BuiltInPlanner and override process_planning_response(), the method was never called because the response processor used isinstance(planner, BuiltInPlanner) which returns True for all subclasses.

Solution

Changed the check to detect whether process_planning_response has been overridden:

# Before
if not planner or isinstance(planner, BuiltInPlanner):
    return

# After
if (
    not planner
    or type(planner).process_planning_response
    is BuiltInPlanner.process_planning_response
):
    return

This ensures:

• BuiltInPlanner itself is skipped (returns None)
• Subclasses without override are skipped (avoids side effects)
• Subclasses with override have their method called

Testing

Added 3 new tests:

1. test_overridden_subclass_process_planning_response_called - Regression test for Bug: AutomaticFunctionCallingConfig(disable=True) is ignored - Planner hook bypassed #4133
2. test_base_builtin_planner_process_planning_response_not_called - Verifies base class is skipped
3. test_non_overridden_subclass_process_planning_response_not_called - Verifies non-overriding subclasses are also skipped

[gemini-code-assist]

Summary of Changes

Hello @maru0804, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical bug that prevented BuiltInPlanner subclasses from effectively customizing their planning response processing. By refining the method invocation logic, the system now correctly dispatches to overridden process_planning_response implementations, significantly enhancing the extensibility and customizability of the planning framework for developers.

Highlights

• Fix for process_planning_response override: Corrected an issue where BuiltInPlanner subclasses overriding process_planning_response were not having their custom logic executed due to an incorrect isinstance check.
• Updated conditional logic: The check for invoking process_planning_response was changed from isinstance(planner, BuiltInPlanner) to type(planner).process_planning_response is BuiltInPlanner.process_planning_response.
• Ensured correct method invocation: This modification ensures that the base BuiltInPlanner and subclasses without an override are skipped, while subclasses that do override the method have their custom implementation called.
• Comprehensive test coverage: Three new unit tests were added to validate the fix: one for overridden subclasses being called, one for the base class being skipped, and one for non-overridden subclasses also being skipped.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a bug where overridden process_planning_response methods in BuiltInPlanner subclasses were not being called. The fix, which involves checking the method identity directly instead of using isinstance, is clever and correct. The accompanying tests are thorough, covering the overridden subclass, the base class, and a non-overridden subclass to ensure the new logic works as expected. I've suggested a minor refactoring in the tests to reduce code duplication by parameterizing two similar test cases, which will improve maintainability. Overall, this is a high-quality contribution that effectively resolves the issue.

[maru0804]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly fixes an issue where process_planning_response was not called for subclasses of BuiltInPlanner. The change from isinstance to a method identity check is a clever and effective solution. The accompanying tests are thorough, covering the regression case for overridden methods, as well as verifying that the base class and non-overriding subclasses are correctly skipped. I have one suggestion to simplify the test code slightly. Overall, this is a great contribution.

[maru0804]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively resolves an issue where overridden methods in BuiltInPlanner subclasses were not being called. The solution, which checks for method identity rather than using isinstance, is clever and correct. The accompanying tests are thorough, covering the fixed case, the base class case, and the non-overridden subclass case. I've made one suggestion to improve the robustness of one of the new tests by narrowing the scope of a patch, but overall this is a solid contribution.

[gemini-code-assist]

The current test patches BuiltInPlanner at the class level. While this works, it's a bit subtle as it relies on the patch interacting with the is check in the implementation. A clearer and more robust approach is to patch the method on the planner instance. This better isolates the test's scope and verifies the behavior without being tightly coupled to the implementation of the conditional check.

  with patch.object(planner, 'process_planning_response') as mock_method: