fix: gate BEFORE_TOOL_DEFINITIONS deepcopy behind has_hooks

[praisonai-triage-agent]

Fixes #3171

Summary

On the tool-calling hot path, _apply_before_tool_definitions_hook deep-copied the full formatted tool schema on every LLM round-trip whenever the agent had tools — even when no BEFORE_TOOL_DEFINITIONS hook was registered. The existing guard only checked _hook_runner, which is a property that is never None once accessed, so it never short-circuited when tools were present.

This gates the work behind the same cheap registry.has_hooks(...) check used by every other hook event (tool_execution.py, chat_mixin.py BEFORE_AGENT), so the deep copy only happens when a hook is actually registered.

Changes

• _apply_before_tool_definitions_hook (sync) and _aapply_before_tool_definitions_hook (async) in chat_mixin.py now short-circuit via has_hooks(HookEvent.BEFORE_TOOL_DEFINITIONS) before building input / deep-copying.

Capability preserved

• When a hook is registered, the full build → deepcopy → execute path runs exactly as before.
• Fail-closed blocking, in-place mutation adoption, and cache isolation via deepcopy are unchanged on the path where a hook runs.
• Only the no-hook case (already a no-op) is spared the deepcopy.

Testing

• tests/unit/hooks/test_before_tool_definitions.py and tests/unit/plugins/test_hook_bridge.py: 35 passed.

Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved tool-definition processing when no pre-processing hooks are configured.
  • Ensured both synchronous and asynchronous workflows skip unnecessary hook execution cleanly.

[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

Run ID: 66487073-6605-421b-ab7f-9b65e3966ab4

📥 Commits

Reviewing files that changed from the base of the PR and between 882143d and 26900f4.

📒 Files selected for processing (1)

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

---

📝 Walkthrough

Walkthrough

ChatMixin now checks whether BEFORE_TOOL_DEFINITIONS hooks are registered before building hook input or executing the hook, in both synchronous and asynchronous paths.

Changes

Tool Definition Hook Gating

Layer / File(s)	Summary
Sync and async hook gating src/praisonai-agents/praisonaiagents/agent/chat_mixin.py	Both hook paths use the runner registry to detect BEFORE_TOOL_DEFINITIONS hooks and return the original tool definitions when none are registered.

Estimated code review effort: 1 (Trivial) | ~5 minutes

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: gating BEFORE_TOOL_DEFINITIONS deep copies behind has_hooks.
Linked Issues check	✅ Passed	The sync and async hook paths now use has_hooks gating, matching issue #3171's hot-path optimization and preserving hook behavior.
Out of Scope Changes check	✅ Passed	The PR stays focused on the BEFORE_TOOL_DEFINITIONS hook fast-path and does not introduce unrelated changes.
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-3171-20260718-1713

---

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

This PR avoids unnecessary tool-schema copies when no definition hook is registered. The main changes are:

• Adds a registry check to the synchronous hook path.
• Adds the same check to the asynchronous hook path.
• Preserves hook execution when an enabled hook exists.

Confidence Score: 5/5

This looks safe to merge.

• The guard checks the same registry used by hook execution.
• Plugin hooks use the default registry used by normal agents.
• No blocking issues were found in the changed code.

Important Files Changed

Filename	Overview
src/praisonai-agents/praisonaiagents/agent/chat_mixin.py	Gates synchronous and asynchronous tool-definition hook preparation on the presence of an enabled hook.

_{Reviews (1): Last reviewed commit: "fix: gate BEFORE_TOOL_DEFINITIONS deepco..." | 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 #3172

• Read AGENTS.md guidelines
• Review changed code in chat_mixin.py
• Verify has_hooks gating correctness (sync + async)
• Check for regressions / edge cases
• Run relevant tests
• Post final verdict

---

Verdict: ✅ APPROVE — merge as-is

This is a clean, minimal, correct hot-path fix. No code changes needed.

Phase 1 — Architecture review (AGENTS.md)

1. Protocol-driven ✅ — Change lives in the core chat_mixin.py hot path; no heavy impl added, no new module/param/export. Zero surface-area growth.
2. Backward compatible ✅ — Behaviour is identical when a BEFORE_TOOL_DEFINITIONS hook is registered (build → deepcopy → execute → fail-closed → adopt mutations all unchanged). Only the already-no-op no-hook case is spared work.
3. Performance ✅ — This is the entire point and it's correct. The old guard not getattr(self, '_hook_runner', None) never short-circuited because _hook_runner is a lazy property that instantiates a runner on first access (agent.py:238) and is never falsy once accessed. So with tools present, _build_before_tool_definitions_input ran a copy.deepcopy of the full tool schema on every LLM round-trip (chat_mixin.py:718). The new guard gates on _runner.registry.has_hooks(...), killing the deepcopy on the common no-hook path.
4. SDK value ✅ — Genuine robustness/perf win, no scope creep.
5. No Agent bloat ✅ — No new params.
6. Repo routing ✅ — Core hook fast-path fix belongs in praisonaiagents/.

Correctness checks I independently verified

• _runner.registry resolves correctly — HookRunner.registry property returns self._registry (runner.py:80), and _hook_runner defaults to the global registry so plugins bridged via wire_into_hook_registry() still fire (agent.py:242-247). The guard reads the same registry the execution path uses → no false negatives for plugin hooks.
• The gating pattern matches every other event exactly: BEFORE_AGENT (chat_mixin.py:2487), BEFORE_TOOL/AFTER_TOOL/AFTER_AGENT (tool_execution.py:547,982,1139). Consistent and idiomatic.
• The from ..hooks import HookEvent was correctly hoisted above the guard in both variants (needed by the new has_hooks(HookEvent.…) check); the now-redundant import inside the try was removed. Sync + async paths kept symmetric.
• getattr(self, '_hook_runner', None) is None defensive check retained — safe for subclasses/mixin contexts lacking the property.

Phase 2 — Fixes: None required. No valid issues raised by CodeRabbit (no actionable comments), Greptile (5/5, safe to merge), or found independently.

Phase 3 — Testing
tests/unit/hooks/test_before_tool_definitions.py + tests/unit/plugins/test_hook_bridge.py → 35 passed.

Files modified this review: none.
• Branch: claude/issue-3171-20260718-1713