feat(plugins): wire on_state_change_callback into plugin framework#4395
feat(plugins): wire on_state_change_callback into plugin framework#4395mportdata wants to merge 2 commits intogoogle:mainfrom
Conversation
Add plumbing so that plugins are notified when an event carries session state changes (non-empty state_delta). This closes a gap where BasePlugin had no default method, PluginManager had no dispatcher, and the runner never triggered the callback. Fixes google#4393
Summary of ChangesHello @mportdata, I'm Gemini Code Assist1! 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 significantly enhances the plugin framework by introducing a new callback mechanism, Highlights
🧠 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. Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively introduces the on_state_change_callback into the plugin framework, providing a much-needed mechanism for plugins to react to state changes. The implementation is well-structured across the BasePlugin, PluginManager, and Runner layers, and is accompanied by a comprehensive set of unit tests. My review includes a couple of suggestions to improve docstring accuracy and type hint consistency, which will enhance clarity for future developers using this new callback.
| This callback is invoked after an event with a non-empty | ||
| ``state_delta`` is yielded from the runner. It is observational: | ||
| returning a value has no effect on execution flow. |
There was a problem hiding this comment.
The docstring states that "returning a value has no effect on execution flow." This is not entirely accurate. The PluginManager will short-circuit and stop calling this callback on subsequent plugins if any plugin returns a non-None value. While the runner ignores the returned value, this short-circuiting is an effect on the execution flow.
To avoid confusion for developers implementing this callback, I suggest clarifying this behavior.
| This callback is invoked after an event with a non-empty | |
| ``state_delta`` is yielded from the runner. It is observational: | |
| returning a value has no effect on execution flow. | |
| This callback is invoked after an event with a non-empty | |
| ``state_delta`` is yielded from the runner. It is observational, but | |
| returning a non-`None` value will short-circuit subsequent plugins. |
There was a problem hiding this comment.
Fixed in 1375419. Updated the docstring to clarify the short-circuiting behaviour:
This callback is invoked after an event with a non-empty
``state_delta`` is yielded from the runner. It is observational, but
returning a non-`None` value will short-circuit subsequent plugins.
| *, | ||
| callback_context: CallbackContext, | ||
| state_delta: dict[str, Any], | ||
| ) -> Optional[None]: |
There was a problem hiding this comment.
The return type hint is Optional[None], but this method returns the result of _run_callbacks, which can return any value if a plugin decides to return something. This creates a type inconsistency, as _run_callbacks is typed to return Optional[Any]. To align with the actual behavior and the return type of the underlying _run_callbacks method, this should be Optional[Any].
| ) -> Optional[None]: | |
| ) -> Optional[Any]: |
There was a problem hiding this comment.
Fixed in 1375419. Changed the return type from Optional[None] to Optional[Any] to align with the return type of _run_callbacks.
- Clarify docstring: non-None return short-circuits subsequent plugins - Fix return type: Optional[None] -> Optional[Any] to match _run_callbacks
Description
Fixes #4393
Problem
The plugin framework provides hooks for various lifecycle events but has no mechanism to notify plugins when session state changes occur via
event.actions.state_delta. TheBigQueryAgentAnalyticsPluginalready implementson_state_change_callback(), demonstrating real demand, but the framework never invokes it because plumbing is missing at three layers.Solution
Wire
on_state_change_callbackthrough the full plugin stack:BasePlugin— Add default no-opon_state_change_callbackmethod (consistent with all other callbacks)PluginManager— Add"on_state_change_callback"toPluginCallbackNameand addrun_on_state_change_callbackdispatcherRunner._exec_with_plugin— After yielding the final event, detect non-emptystate_deltaand invoke the callbackDesign decisions:
dict()copy: prevents plugins from mutating the event'sstate_deltaevent.actionsis never None: guaranteed bydefault_factory=EventActionsinevent.pyTest plan
on_state_change_callbackoverride toFullOverridePluginintest_base_plugin.pytest_base_plugin_default_callbacks_return_noneverifying default returnsNonetest_base_plugin_all_callbacks_can_be_overriddenverifying override workson_state_change_callbackhandler toTestPluginintest_plugin_manager.pytest_all_callbacks_are_supportedto include new callbacktest_run_on_state_change_callback— basic invocation returns None, callback loggedtest_run_on_state_change_callback_calls_all_plugins— both plugins' call_logs contain the callbacktest_run_on_state_change_callback_wraps_exceptions— exception wrapped in RuntimeError with chained causeChecklist
autoformat.shfor formatting