feat: add BEFORE_TOOL_DEFINITIONS hook to transform advertised tool defs#2477
Conversation
…efs (fixes #2473) Adds a first-class, additive extension point that receives the assembled tool definitions for a request and may modify or filter them before they reach the LLM, mirroring how BEFORE_LLM lets a hook mutate messages. - New HookEvent.BEFORE_TOOL_DEFINITIONS + BeforeToolDefinitionsInput dataclass - Fired in chat_mixin.py (sync + async paths) right after formatted_tools is assembled; mutations are adopted, no-op when nothing registered - New before_tool_definitions plugin protocol/base method for plugin consumers - Unit tests for event, runner mutation, and agent integration Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
Greptile SummaryThis PR adds a hook point for changing tool definitions before they are sent to the model. The main changes are:
Confidence Score: 4/5This is close, but the plugin-facing path should be fixed before merging.
src/praisonai-agents/praisonaiagents/plugins/plugin.py and the plugin dispatch wiring Important Files Changed
Reviews (3): Last reviewed commit: "fix: address reviewer findings on BEFORE..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesBEFORE_TOOL_DEFINITIONS hook
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/praisonai-agents/tests/unit/hooks/test_before_tool_definitions.py (1)
105-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a regression test for cache reuse.
The integration tests call
_apply_before_tool_definitions_hookonce. Adding a test that invokes the agent's tool-formatting + hook path twice and asserts the second call still sees the full tool set would have caught the cache-mutation issue flagged inchat_mixin.py. Optional, but valuable as a guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/tests/unit/hooks/test_before_tool_definitions.py` around lines 105 - 127, Add a regression test around Agent’s tool-formatting and BEFORE_TOOL_DEFINITIONS hook path to catch cache reuse mutations. In the TestAgentIntegration suite, use Agent with HookRegistry and a hook like drop_delete, then invoke the same formatting/hook flow twice and assert the second call still starts from the full tool list rather than the mutated result from the first call. Reuse the existing _apply_before_tool_definitions_hook and _sample_tools helpers to keep the test focused on the cache-mutation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py`:
- Around line 543-554: The BEFORE_TOOL_DEFINITIONS hook in `ChatMixin` is
receiving the cached `formatted_tools` list by reference, so in-place hook
mutations are leaking back into `_format_tools_for_completion`’s cache and
affecting later calls. Update the `BeforeToolDefinitionsInput` construction to
pass a defensive copy of `formatted_tools` (and its nested tool dicts if needed)
before calling `self._hook_runner.execute_sync`, while keeping the returned
`_inp.tool_definitions` as the adopted per-request value.
---
Nitpick comments:
In `@src/praisonai-agents/tests/unit/hooks/test_before_tool_definitions.py`:
- Around line 105-127: Add a regression test around Agent’s tool-formatting and
BEFORE_TOOL_DEFINITIONS hook path to catch cache reuse mutations. In the
TestAgentIntegration suite, use Agent with HookRegistry and a hook like
drop_delete, then invoke the same formatting/hook flow twice and assert the
second call still starts from the full tool list rather than the mutated result
from the first call. Reuse the existing _apply_before_tool_definitions_hook and
_sample_tools helpers to keep the test focused on the cache-mutation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd31fb63-ee1d-4e99-87a0-08c7e0520732
📒 Files selected for processing (7)
src/praisonai-agents/praisonaiagents/agent/chat_mixin.pysrc/praisonai-agents/praisonaiagents/hooks/__init__.pysrc/praisonai-agents/praisonaiagents/hooks/events.pysrc/praisonai-agents/praisonaiagents/hooks/types.pysrc/praisonai-agents/praisonaiagents/plugins/plugin.pysrc/praisonai-agents/praisonaiagents/plugins/protocols.pysrc/praisonai-agents/tests/unit/hooks/test_before_tool_definitions.py
| _inp = BeforeToolDefinitionsInput( | ||
| session_id=getattr(self, '_session_id', 'default'), | ||
| cwd=os.getcwd(), | ||
| event_name=HookEvent.BEFORE_TOOL_DEFINITIONS, | ||
| timestamp=str(time.time()), | ||
| agent_name=self.name, | ||
| model=self.llm if isinstance(self.llm, str) else str(self.llm), | ||
| tool_definitions=formatted_tools, | ||
| ) | ||
| self._hook_runner.execute_sync(HookEvent.BEFORE_TOOL_DEFINITIONS, _inp) | ||
| # Adopt mutations, mirroring how BEFORE_LLM adopts messages. | ||
| return _inp.tool_definitions |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Hook mutations poison the formatted-tools cache.
_format_tools_for_completion caches and returns the same cleaned_tools list object it stores (Lines 524-529). Here you pass that exact object as tool_definitions, and registered hooks mutate it in place (e.g. data.tool_definitions[:] = [...] or t["function"]["description"] += ... as in the tests). Because the hook mutates the cached list/dicts, the changes persist into the cache and bleed into every subsequent request sharing the cache key — dropped tools stay dropped and appended description text accumulates each call. This breaks the intended per-request semantics.
Pass a copy into the hook so the cache is never mutated:
🛡️ Proposed fix
if not formatted_tools or not getattr(self, '_hook_runner', None):
return formatted_tools
try:
- from ..hooks import HookEvent, BeforeToolDefinitionsInput
+ import copy
+ from ..hooks import HookEvent, BeforeToolDefinitionsInput
_inp = BeforeToolDefinitionsInput(
session_id=getattr(self, '_session_id', 'default'),
cwd=os.getcwd(),
event_name=HookEvent.BEFORE_TOOL_DEFINITIONS,
timestamp=str(time.time()),
agent_name=self.name,
model=self.llm if isinstance(self.llm, str) else str(self.llm),
- tool_definitions=formatted_tools,
+ tool_definitions=copy.deepcopy(formatted_tools),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _inp = BeforeToolDefinitionsInput( | |
| session_id=getattr(self, '_session_id', 'default'), | |
| cwd=os.getcwd(), | |
| event_name=HookEvent.BEFORE_TOOL_DEFINITIONS, | |
| timestamp=str(time.time()), | |
| agent_name=self.name, | |
| model=self.llm if isinstance(self.llm, str) else str(self.llm), | |
| tool_definitions=formatted_tools, | |
| ) | |
| self._hook_runner.execute_sync(HookEvent.BEFORE_TOOL_DEFINITIONS, _inp) | |
| # Adopt mutations, mirroring how BEFORE_LLM adopts messages. | |
| return _inp.tool_definitions | |
| import copy | |
| from ..hooks import HookEvent, BeforeToolDefinitionsInput | |
| _inp = BeforeToolDefinitionsInput( | |
| session_id=getattr(self, '_session_id', 'default'), | |
| cwd=os.getcwd(), | |
| event_name=HookEvent.BEFORE_TOOL_DEFINITIONS, | |
| timestamp=str(time.time()), | |
| agent_name=self.name, | |
| model=self.llm if isinstance(self.llm, str) else str(self.llm), | |
| tool_definitions=copy.deepcopy(formatted_tools), | |
| ) | |
| self._hook_runner.execute_sync(HookEvent.BEFORE_TOOL_DEFINITIONS, _inp) | |
| # Adopt mutations, mirroring how BEFORE_LLM adopts messages. | |
| return _inp.tool_definitions |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/agent/chat_mixin.py` around lines 543 -
554, The BEFORE_TOOL_DEFINITIONS hook in `ChatMixin` is receiving the cached
`formatted_tools` list by reference, so in-place hook mutations are leaking back
into `_format_tools_for_completion`’s cache and affecting later calls. Update
the `BeforeToolDefinitionsInput` construction to pass a defensive copy of
`formatted_tools` (and its nested tool dicts if needed) before calling
`self._hook_runner.execute_sync`, while keeping the returned
`_inp.tool_definitions` as the adopted per-request value.
|
@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
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2477 (BEFORE_TOOL_DEFINITIONS hook)Verdict: ✅ Approved — fixes pushed to Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Valid issues fixed
New regression tests added:
Skipped (with reasoning)
Files modified
Out of scope honored: no changes to --- · branch |
- deep-copy tool defs before hook to stop cache poisoning (CodeRabbit) - add async helper using await execute so async chat runs the hook (Greptile P1) - correct events.py docstring to only promise in-place mutation (Greptile P2) - revert protocol method addition that broke HookPluginProtocol isinstance (backward-compat) - add cache-reuse and async regression tests Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@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
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2477 (BEFORE_TOOL_DEFINITIONS hook)Verdict: ✅ Approved — no further code changes required. The fixes from commit
Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Reviewer issues (all verified resolved)
Skipped (with reasoning)
Test results (this run)All hook + agent-integration tests pass, including both the cache-poisoning guard and the async-loop regression test (confirmed executing, not skipped). ConclusionMinimal, focused, backward-compatible, and well-tested. No bloat, no hot-path regression, no Agent params added. Approved — ready to merge. |
| def before_tool_definitions( | ||
| self, tool_definitions: List[Dict[str, Any]] | ||
| ) -> List[Dict[str, Any]]: | ||
| """Called after advertised tool definitions are assembled and before | ||
| they reach the LLM. Can inspect, filter, or rewrite them.""" | ||
| return tool_definitions |
There was a problem hiding this comment.
This method is still disconnected from the runtime path that sends tool definitions to the model. The chat flow fires BEFORE_TOOL_DEFINITIONS through the hook runner and hook registry, while plugin subclasses are invoked through the plugin manager. A plugin can implement before_tool_definitions() and declare the hook, but its edits are never called before the LLM receives the tools. Plugin-based schema filtering or redaction will be silently ignored unless this event is bridged into the plugin dispatch path.
MervinPraison
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #2473
Summary
Adds a first-class, additive extension point that lets a hook/plugin inspect or rewrite the tool definitions (descriptions and parameter schemas) an agent advertises to the model for a given request, before they reach the LLM. This mirrors how
BEFORE_LLMalready lets a hook mutatemessagesin place, and is fully backward compatible (no hook registered → identical behaviour).Changes
praisonaiagents/hooks/):HookEvent.BEFORE_TOOL_DEFINITIONSenum value (types.py)BeforeToolDefinitionsInputdataclass carryingtool_definitions,model, plus read context (agent name, session id) (events.py)__init__.py(__all__+ lazy group)agent/chat_mixin.py):_apply_before_tool_definitions_hook()helper that fires the event afterformatted_toolsis assembled and adopts mutationsplugins/):before_tool_definitions()method onHookPluginProtocoland basePlugintests/unit/hooks/test_before_tool_definitions.py):Layer placement
Primary layer is core (the tool payload is assembled in the agent runtime and only core can expose it at the right point). Plugins are the consumer via the new protocol method.
Test plan
test_agent_execute_decorated_toolre: tool retrymax_attempts, present onmain)Generated with Claude Code
Summary by CodeRabbit
New Features
Tests