fix: remove duplicate BEFORE_TOOL/AFTER_TOOL hook firing in legacy async path

[praisonai-triage-agent]

Fixes #3455

Summary

The legacy async tool loop in _achat_completion (chat_mixin.py) fired the BEFORE_TOOL and AFTER_TOOL hooks inline, then called execute_tool_async, which also fires the same two hooks (guarded). This double-fired hooks when registered and, when none were registered, built BeforeToolInput/AfterToolInput (two os.getcwd() syscalls + two str(time.time())) per tool call for no consumer.

Change

Removed the redundant inline BEFORE_TOOL/AFTER_TOOL blocks (chat_mixin.py:3752-3801) and rely on the single already-guarded dispatch inside execute_tool_async (execution_mixin.py:1254/:1279), which the loop already calls.

Capability preserved

• BEFORE_TOOL/AFTER_TOOL fire once via execute_tool_async with identical inputs.
• Tool blocking preserved (block sentinel returned and appended to results).
• modified_input argument mutation and AFTER_TOOL additional_context aggregation preserved.
• Non-fatal try/except isolation preserved (inside execute_tool_async).

Net: 1 file changed, 4 insertions(+), 47 deletions(-).

Validation

• tests/unit/agent/test_achat_unified_dispatch.py — 5 passed
• Hook + tool tests (test_hooks, test_before_tool_definitions, test_hook_bridge, etc.) — 29 passed

Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved consistency and safety when tools are executed during asynchronous chat interactions.
  • Centralized tool execution handling so applicable safeguards and validations are applied reliably.

[MervinPraison]

@coderabbitai review

[MervinPraison]

/review

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8dec5be5-071f-4100-9286-327f5277b953

📥 Commits

Reviewing files that changed from the base of the PR and between 30c44e3 and a900a5c.

📒 Files selected for processing (1)

• src/praisonai-agents/praisonaiagents/agent/chat_mixin.py

---

📝 Walkthrough

Walkthrough

The legacy async tool-call path in ChatMixin._achat_completion now delegates tool execution directly to execute_tool_async, removing duplicate inline BEFORE_TOOL and AFTER_TOOL hook handling.

Changes

Async tool hook execution

Layer / File(s)	Summary
Centralized async tool execution src/praisonai-agents/praisonaiagents/agent/chat_mixin.py	_achat_completion removes inline tool hook dispatch and invokes execute_tool_async with the current tools override, leaving hook enforcement to the centralized execution pipeline.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• MervinPraison/PraisonAI#3196: Adds BEFORE_TOOL/AFTER_TOOL enforcement inside execute_tool_async, the pipeline used by this change.
• MervinPraison/PraisonAI#3451: Adjusts async tool dispatch in the same ChatMixin._achat_completion path.
• MervinPraison/PraisonAI#3172: Updates hook gating in the related tool-definition path.

Suggested labels: pipeline/awaiting-merge-gate

Suggested reviewers: cursoragent, mervinpraison

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: removing duplicate BEFORE_TOOL/AFTER_TOOL hook firing in the legacy async path.
Linked Issues check	✅ Passed	The change matches #3455 by removing inline duplicate hook dispatch and relying on execute_tool_async for guarded hook execution.
Out of Scope Changes check	✅ Passed	The summary shows a focused edit to chat_mixin.py with no unrelated behavior changes beyond the hook-dispatch refactor.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/issue-3455-20260727-1716

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

Removes duplicate inline tool-hook dispatch from the legacy asynchronous chat loop.

• Delegates BEFORE_TOOL and AFTER_TOOL handling exclusively to execute_tool_async.
• Preserves guarded hook execution, tool blocking, input mutation, and post-tool context aggregation.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The legacy asynchronous loop now relies on the existing guarded tool executor, which preserves hook blocking, argument mutation, failure isolation, and after-hook context handling without firing hooks twice.

Important Files Changed

Filename	Overview
src/praisonai-agents/praisonaiagents/agent/chat_mixin.py	Removes redundant hook dispatch while retaining the established asynchronous tool-execution pipeline.

_{Reviews (1): Last reviewed commit: "fix: remove duplicate BEFORE_TOOL/AFTER_..." | Re-trigger Greptile}

[MervinPraison]

@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: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.

Phase 1: Review per AGENTS.md

1. Protocol-driven: check heavy implementations vs core SDK
2. Backward compatible: ensure zero feature regressions
3. Performance: no hot-path regressions
4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep
5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.
6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (praisonai.sandbox entry point) — request changes if wrongly added to praisonaiagents/

Phase 2: FIX Valid Issues
7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback
9. Push all code fixes directly to THIS branch (do NOT create a new PR)
10. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
11. If all issues are resolved, approve the PR / close the Issue
12. If blocking issues remain, request changes / leave clear action items

[praisonai-triage-agent]

Claude finished @MervinPraison's task —— View job

---

Final Architecture Review — PR #3456 ✅ Approved

• Read AGENTS.md guidelines
• Review changed section in chat_mixin.py
• Verify hook parity in execute_tool_async
• Confirm blocking / modified_input / additional_context preserved
• Run tests
• Final verdict

---

Phase 1 — Architecture review (per AGENTS.md)

Criterion	Verdict
Protocol-driven	✅ No new heavy impl; consolidates onto the existing guarded pipeline.
Backward compatible	✅ Hooks still fire once with identical inputs; zero feature regression.
Performance	✅ Improved hot path — removes 2 unconditional os.getcwd() syscalls + 2 str(time.time()) and needless BeforeToolInput/AfterToolInput construction per tool call when no hooks are registered. execute_tool_async is gated on has_hooks(...) so no work when unused.
SDK value	✅ Genuine simplification: -47/+4, removes duplicated dispatch, single source of truth. No scope creep, no new params/exports, Agent class untouched.
Repo routing	✅ Bug fix in praisonaiagents/ core — correct location.

Phase 2 — Capability parity verification

I traced the sole remaining dispatch (execution_mixin.py:1251-1300) against the deleted inline blocks:

• BEFORE_TOOL fires once — execution_mixin.py:1254-1267, same BeforeToolInput fields. ✅
• Blocking preserved — is_blocked returns a sentinel string that the loop appends to results (chat_mixin.py), so a blocked call still yields a result entry. ✅
  • Note: block message text changes from "[Tool blocked by hook: …]" to "Execution of {tool} was blocked by security policy." — this now matches the sync path (tool_execution.py), an intentional consistency improvement, not a regression.
• modified_input mutation preserved — execution_mixin.py:1271-1273 (was NOT applied in the old inline path — this fixes a latent gap). ✅
• AFTER_TOOL + additional_context aggregation preserved — execution_mixin.py:1279-1299. ✅
• Non-fatal isolation — retained inside the pipeline; the loop keeps its own try/except around execute_tool_async. ✅

The old inline BEFORE_TOOL also used hook_runner.execute(...) without target=function_name, whereas the pipeline passes target=function_name (:1266, :1292) — the retained path is strictly more correct for tool-scoped hook matching.

Phase 3 — Tests

tests/unit/agent/test_achat_unified_dispatch.py .......... 5 passed
tests/unit/hooks/test_hooks.py .......................... 42 passed
tests/unit/hooks/test_before_tool_definitions.py ......... 8 passed
tests/unit/plugins/test_hook_bridge.py .................. 27 passed

82 passed, 0 failed. (3 unrelated collection errors elsewhere in the suite are pre-existing — missing resolve_output export / install state — and untouched by this PR.)

Verdict

Approve. The change eliminates real double-firing of BEFORE_TOOL/AFTER_TOOL in the legacy async loop, removes a per-tool-call hot-path cost, and actually closes two latent gaps (missing modified_input application and missing target= scoping) by routing through the single guarded executor. No fixes required — no valid issues raised by CodeRabbit/Greptile, and none found independently.

Files modified by me: none — the PR is correct as-is.

Job run · Branch claude/issue-3455-20260727-1716