Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes executor hook functionality for plugins by generalizing hook execution to work across different services and moving command encoding to occur after plugin hooks are called.
- Refactored specific builder plugin hook check to support any plugin hooks through a generic
_call_ability_plugin_hooksmethod - Added hook execution to the operations API manager to ensure plugins can modify abilities outside the planning service
- Moved command encoding to occur after plugin hooks to ensure hooks can modify abilities before encoding
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/service/planning_svc.py | Replaced specific builder plugin hook check with generic hook execution method |
| app/api/v2/managers/operation_api_manager.py | Added plugin hook execution and moved command encoding after hooks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/service/planning_svc.py
Outdated
| for _hook, fcall in executor.HOOKS.items(): | ||
| await fcall(ability, executor) |
There was a problem hiding this comment.
This code will fail with an AttributeError if executor.HOOKS is None or undefined. Add a null check before iterating: if executor.HOOKS: before the for loop.
| for _hook, fcall in executor.HOOKS.items(): | |
| await fcall(ability, executor) | |
| if executor.HOOKS: | |
| for _hook, fcall in executor.HOOKS.items(): | |
| await fcall(ability, executor) |
| for _hook, fcall in executor.HOOKS.items(): | ||
| await fcall(ability, executor) |
There was a problem hiding this comment.
This code will fail with an AttributeError if executor.HOOKS is None or undefined. Add a null check before iterating: if executor.HOOKS: before the for loop.
| for _hook, fcall in executor.HOOKS.items(): | |
| await fcall(ability, executor) | |
| if executor.HOOKS: | |
| for _hook, fcall in executor.HOOKS.items(): | |
| await fcall(ability, executor) |
| async def _call_ability_plugin_hooks(self, ability, executor): | ||
| """Calls any plugin hooks (at runtime) that exist for the ability and executor.""" | ||
| for _hook, fcall in executor.HOOKS.items(): | ||
| await fcall(ability, executor) | ||
|
|
There was a problem hiding this comment.
The duplicated hook execution logic in both files should be extracted to a shared utility or base class to avoid code duplication and ensure consistent behavior.
| async def _call_ability_plugin_hooks(self, ability, executor): | |
| """Calls any plugin hooks (at runtime) that exist for the ability and executor.""" | |
| for _hook, fcall in executor.HOOKS.items(): | |
| await fcall(ability, executor) | |
| # _call_ability_plugin_hooks is now implemented in BasePlanningService |
|
Can address the suggestions above and update the PR for approval |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Description
This pull request addresses issues #3078 and #3079.
Currently there is a bug identified in #3078 that prevents the existing builder plugin from hooking the executor outside of the planning service. This is addressed by adding a similar check in the operations api manager.
Looking at the bigger picture, the executor hook check was specialized just for the builder plugin. The change below will parse through any hook that is added to the executor hook data structure allowing for other plugins to easily leverage this capability.
Type of change
How Has This Been Tested?
I have tested these changes on Caldera v5 with a plugin that leverages this hooking method to edit ability files before they are staged for an agent.
Checklist: