Repository navigation
fix: await async def plugin hooks instead of silently dropping them - #5965
Conversation
…ixes #5964) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
/review |
|
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
[Medium impact] The change reaches plugin callbacks throughout the core agent hook workflow. The PR still needs to prevent explicitly enabled plugins from running before setup finishes. Findings
SummaryThe PR makes plugin dispatch await coroutine results and adds tests for async hooks.
Reviews (4) · Last reviewed commit: "fix: keep an explicit disable() made dur..." · Reviewed by Greptile |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. MANDATORY READ (before reviewing):
Phase 1: Review per AGENTS.md
MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment: Phase 1 — AGENTS.md review
For TypeScript PRs (src/praisonai-ts/), also add: Phase 2: FIX Valid Issues Phase 3: Final Verdict |
…/on_config/on_shutdown Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Final architecture review — PR #5965Phase 1 — AGENTS.md review
Phase 2 — Fixes pushed (commit 13b33e5)Both Greptile P1 findings were valid and are fixed:
Files modified
Tests
Skipped
Phase 3 — Verdict✅ Approve. Every Phase 1 row passes and both blocking findings are fixed. |
|
Merge gate scan — eligible for assessment. Claude merge gate will assess and may auto-merge on an APPROVE verdict. |
|
MERGE_GATE_VERDICT: BLOCK Rationale:
Next step: run the async lifecycle callbacks ( Note: this gate could not read AGENTS.md or the full |
|
MERGE_GATE_VERDICT: BLOCK Rationale:
Next step: keep a new plugin out of dispatch until Note: tool permissions blocked reading AGENTS.md and the inline review threads. This verdict is based on the PR diff, all PR comments and the review summaries. |
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fix pushed (HEAD
|
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. MANDATORY READ (before reviewing):
Phase 1: Review per AGENTS.md
MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment: Phase 1 — AGENTS.md review
For TypeScript PRs (src/praisonai-ts/), also add: Phase 2: FIX Valid Issues Phase 3: Final Verdict |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Final architecture review (HEAD
|
| Check | Result |
|---|---|
| Protocol-driven / no heavy impl in core | ✅ The fix stays in the core plugin dispatcher (plugins/manager.py). It is a small private helper (_ResolvingPlugin) that reuses the existing run_coroutine_from_any_context bridge. |
| Backward compatible | ✅ Sync hooks behave the same as before. async def hooks used to be silently dropped and are now awaited. A plugin that calls enable() or disable() inside on_init keeps that choice; this commit restores the disable() case. |
| Performance (hot path) | ✅ Each dispatch adds one small wrapper and an iscoroutine check. The event loop or bridge is used only when a hook actually returns a coroutine. |
| SDK value | ✅ It fixes a real silent failure: async def plugin hooks (guardrails, before_tool, lifecycle) returned unawaited coroutines and never ran. The plugin stays out of dispatch until on_init finishes, so dispatch can't reach a plugin that is still setting up. |
| No Agent param bloat | ✅ agent.py and the Agent params are untouched. No new exports or dependencies. |
| Repo routing | ✅ This is a bug fix to the core plugin framework, not a vendor tool or plugin. |
Phase 2: fix pushed
Earlier reviewer findings are all fixed on this branch: dropped coroutines, unbounded async hooks, async on_init/on_config/on_shutdown, the lock deadlock, rollback when init fails, and dispatch reaching a plugin before on_init finishes.
Found in this review: a plugin that disables itself in on_init got re-enabled. register() published the plugin with default_enabled or self._enabled[...]. A plugin calling mgr.disable(name) from on_init (for example, when credentials are missing) was turned back on. Before this PR, that disable was respected, so this was a regression.
Files modified
src/praisonai-agents/praisonaiagents/plugins/manager.py(+12/−2): a private_initializingset.register()applies the default enabled state only ifenable()ordisable()was not called duringon_init.enable(),disable(),unregister()and the rollback path all clear the entry.src/praisonai-agents/tests/unit/plugins/test_async_plugin_hooks.py: addedtest_disable_during_on_init_is_kept. It fails onb3c55dba4and passes now.
Tests
tests/unit/plugins: 208 passed.tests/unit -k "plugin or hook": 568 passed, 3 skipped.- Full
tests/unit -xwas not completed. It stopped at an unrelated test intest_agent_clone.pythat needs litellm, which is not installed in this runner.
Skipped
- The
main (3.11)failure intest_yaml_headless_5826was not changed. It comes from a litellm/mcp.typesversion mismatch and fails onmaintoo. - No new public state or API;
_initializingis private.
Phase 3: verdict
Every Phase 1 row is ✅ and all known P1 findings are fixed. Approve for merge once CI passes, apart from the test_yaml_headless_5826 failure that also fails on main.
|
Merge gate scan — eligible for assessment. Claude merge gate will assess and may auto-merge on an APPROVE verdict. |
|
MERGE_GATE_VERDICT: APPROVE Rationale (HEAD
Note: tool permissions blocked reading AGENTS.md directly. This verdict is based on the PR diff, all PR comments and the review summaries. |
|
Merged by Claude PR merge gate ( |
Fixes #5964
Problem
Plugin lifecycle hooks written as
async defreturned a coroutine that was never awaited. Observer hooks (session_start,message_sent,gateway_start, …) silently did nothing, and transforming hooks (before_tool,before_message, …) passed a coroutine object downstream where a value was expected.Fix (core, minimal, no new public API)
plugins/manager.py: added a private_ResolvingPluginview plus a_resolve_awaitablehelper. The HookRegistry bridge adapters (_adapt_plugin_hooks, which is the gateway/agent hot path) andPluginManager.execute_hook/async_execute_hooknow call plugin methods through this view. Any returned coroutine is run to completion with the existingrun_coroutine_from_any_contextbridge. Adapters already run on the HookRunner worker thread, so the coroutine gets a private loop there and never blocks the caller's loop.GuardrailBlockedandPluginDecisionfrom async hooks behave the same as from sync hooks.plugins/plugin.py: the docstring now says hooks may bedeforasync def.No new params, exports or dependencies.
agent.pyis untouched.Tradeoff to note: async hooks run on a private loop in the hook worker thread, not on the caller's (gateway) loop. That fixes the silent no-op without duplicating every adapter as an async variant. An async plugin that holds a client bound to the gateway loop (e.g. an aiohttp session created there) still won't work. If that case comes up, it could be handled later by registering async adapters with
is_async=True.Tests
tests/unit/plugins/test_async_plugin_hooks.py: an async transforming hook is applied (no "never awaited" warning), an asyncGuardrailBlockeddenies, an async observer runs from inside a running loop, andexecute_hook/async_execute_hookboth return the awaited value. All 4 fail onmainand pass with this change.tests/unit/plugins/plus all hook/plugin-related tests: the same set fails before and after (the existing failures come from optional deps such aspraisonai/litellmthat aren't installed in this environment).🤖 Generated with Claude Code