-
Notifications
You must be signed in to change notification settings - Fork 235
Feat: shared plugin context and error handling. #816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
d14876f
to
fa1d735
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Overall this is a solid step toward predictable cross-plugin state sharing and clearer failure modes.
This PR introduces:
- a shared
GlobalContext
that flows across plugins within a request, alongside a per-pluginPluginContext
; and - a rework of error handling semantics, adding
enforce_ignore_error
and wiring behavior tofail_on_plugin_error
.
The server now returns a structured result object {result, context?, error?}. New unit tests and fixtures were added to exercise context propagation and error modes.
I like the design! Please consider the following:
- Fix the Pydantic mutable defaults
- Consider the small change to add the
plugin_name
to the server result, consider the additional check on the global context metadata - Add a CHANGELOG note for the new mode + server response shape
- Document the new
enforce_ignore_error
mode
try: | ||
if plugin: | ||
_payload = payload_model.model_validate(payload) | ||
_context = PluginContext.model_validate(context) | ||
result = await asyncio.wait_for(hook_function(plugin, _payload, _context), plugin_timeout) | ||
return result.model_dump() | ||
result_payload[RESULT] = result.model_dump() | ||
if _context.state or _context.metadata or _context.global_context.state: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe consider wrapping this expression into an instance method is_empty(self) -> bool
.
e.g., if _context.is_empty(): ...
Also, do we need to include _context.global_context.metadata
in the expression?
@@ -662,17 +668,32 @@ class GlobalContext(BaseModel): | |||
user: Optional[str] = None | |||
tenant_id: Optional[str] = None | |||
server_id: Optional[str] = None | |||
state: dict[str, Any] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having mutable defaults ({}
) assign this way can be an issue; the dict is created once at class definition time and may be shared across instances. Use Field(default_factory=dict)
instead. This could cause subtle state bleed across requests/plugins (didn't verify it).
state: dict[str, Any] = {}
metadata: dict[str, Any] = {}
Replace = {}
with Field(default_factory=dict)
for all dict fields (state, metadata), and consider the same for any lists/sets.
True | ||
>>> ctx.state["somekey"] = "some value" | ||
>>> ctx.state["somekey"] | ||
'some value' | ||
""" | ||
|
||
state: dict[str, Any] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue here (state, metadata) with mutable dict assignments. Change to default_factory.
False | ||
""" | ||
global_plugin_manager = PluginManager() | ||
plugin_timeout = global_plugin_manager.config.plugin_settings.plugin_timeout if global_plugin_manager.config else DEFAULT_PLUGIN_TIMEOUT | ||
plugin = global_plugin_manager.get_plugin(plugin_name) | ||
result_payload: dict[str, Any] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we do this?
- result_payload: dict[str, Any] = {}
+ result_payload: dict[str, Any] = {"plugin": plugin_name}
This would always return the plugin_name in the result, which could be handy for logging and other things without extra lookups.
β¨ Feature / Enhancement PR
π Epic / Issue
Link to the epic or parent issue:
Closes #
π Summary
This PR does two things:
π§ͺ Checks
make lint
passesmake test
passesπ Notes
On Errors:
Currently, plugin errors are being handled as follows:
enforcement
mode, it blocks the continued processing. If the plugin is in permissive mode, the error is logged, but the system is allowed to go on.I'm do not think PluginViolation is the best way to bubble up an error. It's also not clear how this part works with the global plugin settings:
plugin_settings:
parallel_execution_within_band: true
plugin_timeout: 30
fail_on_plugin_error: false <---------------------------------- this..
enable_plugin_api: true
plugin_health_check_interval: 60
I reimplemented it a bit and would like feedback:
enforce
both violations and errors are bubbled up as exceptions and the execution is blocked.enforce_ignore_error
violations are bubbled up as exceptions and execution is blocked, but errors are logged and execution continues.permissive
execution is allowed to proceed whether there are errors or violations. Both are logged.On Context Sharing
Each plugin hook takes a
PluginContext
. ThePluginContext
now looks as follows:The
GlobalContext
is designed to store global information about the hookpoint. Items such as:Plugins can pass state information to OTHER plugins by setting the values in the
GlobalContext->state
object. State information is maintained across pre/post hooks (for a tool for example) but is deleted after a pre/post hook pair is complete.The global context is accessible through the
PluginContext
using theglobal_context
field. This field represents a copy of the GlobalContext and is updated during each plugin pre and post hook invocation.Plugins can pass state information across PRE/POST hook pairs within the SAME plugin using the
PluginContext->state
object. Data is deleted from the state after pre/post invocations.PluginContext->metadata
is designed to hold PluginContext statistical information. This could be removed in a later release if not deemed useful.