fix: prune permission-denied tools from LLM-advertised set#2475
fix: prune permission-denied tools from LLM-advertised set#2475praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
|
@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? |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
Greptile SummaryThis PR makes the model-facing tool list respect agent permissions. The main changes are:
Confidence Score: 4/5This is close, but the cache behavior should be fixed before merging.
src/praisonai-agents/praisonaiagents/agent/chat_mixin.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/chat_mixin.py | Adds permission pruning to tool schema and prompt generation, but the formatted schema cache can still return stale permission-filtered results. |
| src/praisonai-agents/tests/unit/test_permissions.py | Adds tests for advertised tool pruning across denied, allowed, and no-rule cases. |
Reviews (3): Last reviewed commit: "fix: prune permission-denied tools from ..." | 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:
📝 WalkthroughWalkthrough
Permission-aware tool advertisement pruning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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 523-527: The pruning is only applied to the final tools payload,
but `_build_system_prompt()` and the tool-search metadata still use unfiltered
`self.tools` and `formatted_tools`, so denied tools can remain advertised. In
`chat_mixin.py`, update the `build_system_prompt`/tool-search assembly path to
call `_prune_denied_tools` before constructing any advertised surface, and reuse
that same filtered set for both the system prompt and metadata. Keep the final
`cleaned_tools` path consistent with the already-filtered names so all exposed
tool references match the execution-time permissions.
In `@src/praisonai-agents/tests/unit/test_permissions.py`:
- Around line 181-188: Add a real agentic end-to-end regression for the
permission pruning behavior in TestAdvertisedToolPruning, not just helper-level
coverage. Extend the test suite in test_permissions.py to drive an agent through
the LLM call path and assert that the denied tool is neither present in the
advertised tool list nor invoked, using the existing permission-tier flow and
any agent/tool symbols already exercised by the current deterministic tests.
Keep the new test deterministic by mocking the LLM/tool execution while still
verifying the full agentic path.
🪄 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: 4feaff82-4337-43f2-b667-5760bea1b23a
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/agent/chat_mixin.pysrc/praisonai-agents/tests/unit/test_permissions.py
| # Prune permission-denied tools from the advertised set so the model is | ||
| # only ever offered tools it can actually call. Mirrors the execution-time | ||
| # enforcement in tool_execution.py (_perm_deny / _perm_allow). No-op when | ||
| # no deny set and no allow set are configured (backward compatible). | ||
| cleaned_tools = self._prune_denied_tools(cleaned_tools) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Apply pruning before every advertised surface, not only the final tools payload.
_build_system_prompt() still enumerates raw self.tools, and tool-search metadata is assembled from unpruned formatted_tools before this line. Denied tools can therefore remain visible in the prompt or bridge metadata even though the final schema list is filtered. Prune before tool-search assembly and reuse the same filtered names when building the system prompt.
🤖 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 523 -
527, The pruning is only applied to the final tools payload, but
`_build_system_prompt()` and the tool-search metadata still use unfiltered
`self.tools` and `formatted_tools`, so denied tools can remain advertised. In
`chat_mixin.py`, update the `build_system_prompt`/tool-search assembly path to
call `_prune_denied_tools` before constructing any advertised surface, and reuse
that same filtered set for both the system prompt and metadata. Keep the final
`cleaned_tools` path consistent with the already-filtered names so all exposed
tool references match the execution-time permissions.
| class TestAdvertisedToolPruning: | ||
| """Permission-denied tools must be pruned from the LLM-advertised set. | ||
|
|
||
| Regression for: the agent advertised its entire tool set to the model | ||
| regardless of permission rules, only blocking at execution time. The | ||
| advertised surface should be shaped by the permission tier. | ||
| """ | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add an end-to-end agentic regression for this feature.
The new tests cover the helper/formatting path, but this permission feature should also drive an agent through an LLM call path and assert the denied tool is not advertised or called. As per coding guidelines, src/praisonai-agents/tests/**/*.py requires every feature to include a real agentic end-to-end LLM test and deterministic tests.
🤖 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/test_permissions.py` around lines 181 - 188,
Add a real agentic end-to-end regression for the permission pruning behavior in
TestAdvertisedToolPruning, not just helper-level coverage. Extend the test suite
in test_permissions.py to drive an agent through the LLM call path and assert
that the denied tool is neither present in the advertised tool list nor invoked,
using the existing permission-tier flow and any agent/tool symbols already
exercised by the current deterministic tests. Keep the new test deterministic by
mocking the LLM/tool execution while still verifying the full agentic path.
Source: Coding guidelines
|
@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 #2475 ✅
Phase 1 — Architecture validation (per AGENTS.md)
Phase 2 — Reviewer findings triage & fixes
Files modified
Skipped: Greptile cache concern (not a real bug); no Test results
Phase 3 — Verdict: ✅ ApproveThe valid security finding is fixed, change is minimal, focused, and backward-compatible. Pre-existing unrelated test failures should be addressed separately (env/default-preset assumption). Job run · branch |
_build_system_prompt() enumerated raw self.tools and named every tool in
the prompt ("You have access to the following tools: ..."), so a denied
tool stayed advertised in natural language even after being pruned from
the function schema. Filter tool_names through a shared _tool_name_allowed
predicate so the prompt and schema advertise the same permission-shaped
surface. Refactors _prune_denied_tools to reuse the predicate (DRY).
Addresses CodeRabbit major (security) finding on PR #2475.
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 #2475 ✅
Phase 1 — Architecture validation (per AGENTS.md)
Phase 2 — Reviewer findings triage
No new code changes required — every valid finding was already implemented in commit Test results
Phase 3 — Verdict: ✅ ApproveThe change is minimal, focused, backward-compatible, and adds real security value. The one valid security finding (system-prompt leak) is fixed; the cache concern is not a real bug given the immutable-after-init permission fields. No blocking issues remain. Note: I cannot formally approve via GitHub's review API (security restriction), but this is my approval recommendation. Job run · branch |
MervinPraison
added
claude-conflict-pending
and removed
pipeline/blocked:cooldown
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebase PR #2475 onto latest
|
Shape the advertised tool surface by the agent's effective permission tier so the model is only ever offered tools it can actually call. Previously the entire tool set was advertised and denial was enforced only at execution time, causing denied-call loops that burn tokens and turns. The new _prune_denied_tools helper mirrors the execution-time enforcement (_perm_deny / _perm_allow) and runs at the end of _format_tools_for_completion. It is a no-op when no deny set and no allow set are configured, preserving backward compatibility. ask/allow tools remain advertised (approval still happens at execution time as defence in depth). Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
_build_system_prompt() enumerated raw self.tools and named every tool in
the prompt ("You have access to the following tools: ..."), so a denied
tool stayed advertised in natural language even after being pruned from
the function schema. Filter tool_names through a shared _tool_name_allowed
predicate so the prompt and schema advertise the same permission-shaped
surface. Refactors _prune_denied_tools to reuse the predicate (DRY).
Addresses CodeRabbit major (security) finding on PR #2475.
Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-2472-20260630-0846
branch
from
18b2ec1 to
7087454
Compare
|
@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
the
pipeline/blocked:cooldown
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2475 ✅
Phase 1 — Architecture validation (per AGENTS.md)
Phase 2 — Reviewer findings triage
No new code changes required — every valid finding is already implemented and the post-rebase tree is correct. Working tree is clean; nothing to push. Test results
ScopeOnly Python package inspected ( Phase 3 — Verdict: ✅ Approve (recommendation)The change is minimal, focused, backward-compatible, and adds real security value. The one valid security finding (system-prompt leak) is fixed; the Greptile cache concern is not a real bug given immutable-after-init permission fields. No blocking issues remain. Note: I cannot formally approve via GitHub's review API (security restriction), but this is my approval recommendation. The 3 env-sensitive pre-existing test failures should be addressed in a separate PR. Job run · branch |
| # only ever offered tools it can actually call. Mirrors the execution-time | ||
| # enforcement in tool_execution.py (_perm_deny / _perm_allow). No-op when | ||
| # no deny set and no allow set are configured (backward compatible). | ||
| cleaned_tools = self._prune_denied_tools(cleaned_tools) |
There was a problem hiding this comment.
This pruning still only runs on a cache miss. The formatted-tools cache key is based on the tool list and tool-search flag, not the current _perm_deny or _perm_allow values, so a later permission update can reuse an older advertised payload without rechecking this line. When a tool is denied after the first formatting call, the model can still see it in the cached schema and then hit an execution-time permission denial when it tries to call it. Include the effective permission state in the cache key, invalidate the cache when permissions change, or filter cached results before returning them.
MervinPraison
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #2472
Summary
The agent advertised its entire tool set to the model on every request regardless of the active permission rules, enforcing denial only at execution time. A read-only agent (e.g.
safe/read_onlypresets, or anydenyrule) still presented denied tools to the model, which then attempted them, got blocked, retried, and burned tokens and turns.This PR shapes the advertised tool surface by the agent's effective permission tier so the model is only ever offered tools it can actually call.
Changes
agent/chat_mixin.py: new_prune_denied_toolshelper applied at the end of_format_tools_for_completion, before caching. It mirrors the execution-time enforcement intool_execution.py(_perm_deny/_perm_allow), droppingdenytools while keepingallow/askadvertised (approval still happens at execution time as defence in depth).tests/unit/test_permissions.py: regression tests asserting a denied tool is absent from the payload while an allowed tool remains, plus helper-level allow/deny/no-op coverage.Validation
TestAdvertisedToolPruningtests pass (5/5).test_permissions.pyare pre-existing and unrelated (they assumePRAISONAI_TOOL_SAFETYunset; confirmed identical failures on the base commit viagit stash).Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests