fix: close $IFS permission bypass, litellm.callbacks overwrite, cross-agent memory reset#3150
fix: close $IFS permission bypass, litellm.callbacks overwrite, cross-agent memory reset#3150praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…-agent memory reset (fixes #3149) - permissions: escalate shell commands containing unresolvable parameter expansion ($IFS / ${...}) to ASK instead of letting a broad allow rule short-circuit a specific deny. Command substitution ($()/backticks) is left to the existing per-op decomposition. - llm: _setup_event_tracking now no-ops on empty events and merges into the process-global litellm.callbacks (tracking per-instance ownership) instead of overwriting it, so unrelated agents' callbacks are preserved. - memory: reset_long_term() deletes only this instance's own Chroma collection rather than resetting the entire shared client; collection name is now an opt-in config override (default unchanged). Co-authored-by: Mervin Praison <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
✅ Action performedReview finished.
|
|
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:
📝 WalkthroughWalkthroughThe changes update LiteLLM callback merging, make Chroma collection names configurable and instance-scoped, limit long-term resets to one collection, and escalate shell commands containing unresolved parameter expansions. ChangesLLM callback registration
Memory collection scoping
Shell expansion permission handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 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 |
Greptile SummaryThis PR closes three gaps in shell permissions, callback registration, and shared memory resets. The main changes are:
Confidence Score: 4/5The shell permission gap should be fixed before merging.
src/praisonai-agents/praisonaiagents/permissions/command_parser.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/permissions/command_parser.py | Adds unresolved parameter-expansion detection, but omits Bash arithmetic expansion. |
| src/praisonai-agents/praisonaiagents/permissions/manager.py | Escalates recognized shell expansions to approval while preserving explicit denies. |
| src/praisonai-agents/praisonaiagents/llm/llm.py | Merges instance callbacks into LiteLLM's global callback list. |
| src/praisonai-agents/praisonaiagents/memory/memory.py | Scopes Chroma resets to one collection and propagates deletion failures. |
| src/praisonai-agents/praisonaiagents/memory/adapters/factories.py | Stores the configured Chroma collection name on the adapter. |
Reviews (2): Last reviewed commit: "fix: escalate bare $VAR expansion to ASK..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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 #3150
Phase 1 — AGENTS.md compliance ✅
Phase 2 — Fixes implementedBoth Greptile P1 findings were valid and are now fixed: 1. _UNSAFE_EXPANSION_RE = re.compile(r"\$(?:\{|[A-Za-z_])")
2. Files modified this round
Skipped (intentionally)
Phase 3 — Verdict✅ Approve. Both blocking P1 findings resolved; changes are minimal, backward-compatible, and correctly scoped. Recommend merge once CI passes. (I can't submit a formal GitHub approval for security reasons — this is my review sign-off.) |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-agents/praisonaiagents/llm/llm.py (1)
5259-5292: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix cross-instance clobbering, state leaks, and TOCTOU races.
The callback tracking logic has three severe flaws:
- Cross-instance clobbering: Removing callbacks from
success_callbackand_async_success_callbackbased ontype(event)wipes out callbacks registered by other concurrent agents of the same type (e.g., if multiple agents use the string"langfuse", all string callbacks are wiped globally).- TOCTOU Data Race: The
if cb in list: list.remove(cb)pattern is not thread-safe. Concurrent modifications by multi-agent workflows can raiseValueError: list.remove(x): x not in list.- State leak on clear: The early return
if not events: returnprevents the cleanup of previously registered callbacks if the instance is updated with an empty list.As per coding guidelines, avoid shared mutable global state between agents; isolating agent context requires strictly instance-scoped management without clobbering other instances. Replace the type-based removal with exact-instance removal, eliminate the TOCTOU race with
try...except, and ensure the early return correctly checks for existing registrations.🔒️ Proposed fix to isolate callbacks safely
- if not events: + if not events and not getattr(self, "_registered_callbacks", []): return try: import litellm except ImportError: raise ImportError( "LiteLLM is required but not installed. " "Please install it with: pip install 'praisonaiagents[llm]'" ) - event_types = [type(event) for event in events] - - # Remove old events of same type - for event in litellm.success_callback[:]: - if type(event) in event_types: - litellm.success_callback.remove(event) - - for event in litellm._async_success_callback[:]: - if type(event) in event_types: - litellm._async_success_callback.remove(event) - - # Merge into the global list rather than replacing it. Only remove the - # callbacks this instance registered on a prior call, then append the - # current ones, preserving other instances' callbacks. - if litellm.callbacks is None: + # Merge into the global lists rather than replacing them. Only remove the + # exact callbacks this instance registered on a prior call, avoiding + # cross-instance clobbering and TOCTOU races. + if getattr(litellm, "callbacks", None) is None: litellm.callbacks = [] + for cb in getattr(self, "_registered_callbacks", []): - if cb in litellm.callbacks: - litellm.callbacks.remove(cb) + try: + litellm.callbacks.remove(cb) + except ValueError: + pass + try: + litellm.success_callback.remove(cb) + except ValueError: + pass + try: + litellm._async_success_callback.remove(cb) + except ValueError: + pass + for event in events: if event not in litellm.callbacks: litellm.callbacks.append(event) + self._registered_callbacks = list(events)🤖 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/llm/llm.py` around lines 5259 - 5292, Update the callback registration method around the events handling to track and remove only callbacks previously registered by the current instance, rather than removing global callbacks by type; preserve other agents’ callbacks. Before returning for an empty events list, clean up any callbacks stored in _registered_callbacks, and make each removal resilient to concurrent changes by catching a missing-entry ValueError. Apply the same instance-scoped cleanup to success_callback, _async_success_callback, and callbacks, then refresh _registered_callbacks.Source: Coding guidelines
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/permissions/manager.py (1)
358-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify comment regarding command substitution.
The comment implies that command substitutions like
$(...)and backticks cannot be statically verified and will trigger the escalation toASK. However, as documented incommand_parser.py,has_unresolvable_expansionexplicitly ignores them becauseparse_commandsuccessfully decomposes them for per-operation verification.Consider updating the comment to focus solely on parameter expansion, avoiding confusion about the behavior of command substitution.
♻️ Proposed tweak
- # Shell parameter/command expansion (``${IFS}``, ``$(...)``, backticks) - # is resolved by bash at runtime and is invisible to the static + # Shell parameter expansion (e.g., ``${IFS}``, ``${VAR}``) + # is resolved by bash at runtime and is invisible to the static # tokenizer, so a payload like ``rm${IFS}-rf${IFS}/`` could slip past a # specific deny rule while a broad ``bash:*`` allow matches. When such # expansion is present we cannot statically verify the command: an # explicit deny still wins, but otherwise we escalate to ASK rather than # letting the flat matcher optimistically ALLOW it.🤖 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/permissions/manager.py` around lines 358 - 364, Update the comment near the expansion handling in the permission manager to describe only unresolved shell parameter expansion, such as ${IFS}, as bypassing static verification; remove references to command substitution ($(...) and backticks), since command_parser.py handles those through parse_command for per-operation verification.
🤖 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/memory/memory.py`:
- Around line 1156-1168: After the delete and _init_chroma() call, update the
active memory_adapter so its collection reference points to the newly
initialized self.chroma_col. Ensure subsequent store_short_term calls use the
recreated collection instead of the deleted one, without changing the existing
scoped deletion or error handling.
In `@src/praisonai-agents/praisonaiagents/permissions/manager.py`:
- Around line 353-356: In the import block within the permission-manager method,
replace the broad Exception handler around importing parse_command and
has_unresolvable_expansion with an ImportError handler only. Preserve the
existing return None fallback for genuine import failures while allowing
internal module errors such as SyntaxError or NameError to propagate.
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/llm/llm.py`:
- Around line 5259-5292: Update the callback registration method around the
events handling to track and remove only callbacks previously registered by the
current instance, rather than removing global callbacks by type; preserve other
agents’ callbacks. Before returning for an empty events list, clean up any
callbacks stored in _registered_callbacks, and make each removal resilient to
concurrent changes by catching a missing-entry ValueError. Apply the same
instance-scoped cleanup to success_callback, _async_success_callback, and
callbacks, then refresh _registered_callbacks.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/permissions/manager.py`:
- Around line 358-364: Update the comment near the expansion handling in the
permission manager to describe only unresolved shell parameter expansion, such
as ${IFS}, as bypassing static verification; remove references to command
substitution ($(...) and backticks), since command_parser.py handles those
through parse_command for per-operation verification.
🪄 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: 99dee3ad-b112-43e0-a7de-4d192dab359c
📒 Files selected for processing (5)
src/praisonai-agents/praisonaiagents/llm/llm.pysrc/praisonai-agents/praisonaiagents/memory/adapters/factories.pysrc/praisonai-agents/praisonaiagents/memory/memory.pysrc/praisonai-agents/praisonaiagents/permissions/command_parser.pysrc/praisonai-agents/praisonaiagents/permissions/manager.py
| # Scope the reset to this instance's own collection. A full | ||
| # ``chroma_client.reset()`` wipes *every* collection in the shared | ||
| # persistent store, destroying other agents' long-term memory that | ||
| # happens to live in the same directory. | ||
| collection_name = getattr(self, "_collection_name", "memory_store") | ||
| try: | ||
| self.chroma_client.delete_collection(name=collection_name) | ||
| except Exception as e: | ||
| self._log_verbose( | ||
| f"Error deleting ChromaDB collection '{collection_name}': {e}", | ||
| logging.ERROR, | ||
| ) | ||
| self._init_chroma() # recreate only this collection |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Sync reinitialized Chroma client and collection to the active adapter.
While _init_chroma() correctly recreates the collection and binds it to self.chroma_col, it leaves self.memory_adapter holding a stale reference to the deleted collection. Because store_short_term blindly delegates to the active adapter (e.g., self.memory_adapter.store_short_term), subsequent memory writes will encounter an exception when accessing the deleted collection and silently fall back to SQLite, which breaks RAG memory context for the remainder of the instance's lifecycle.
Update the adapter's references immediately after re-initialization to maintain state consistency.
🐛 Proposed fix
except Exception as e:
self._log_verbose(
f"Error deleting ChromaDB collection '{collection_name}': {e}",
logging.ERROR,
)
self._init_chroma() # recreate only this collection
+
+ # Sync the recreated client and collection to the adapter to prevent writes to the deleted collection
+ if getattr(self, "memory_adapter", None) and hasattr(self.memory_adapter, "collection"):
+ self.memory_adapter.client = self.chroma_client
+ self.memory_adapter.collection = self.chroma_col📝 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.
| # Scope the reset to this instance's own collection. A full | |
| # ``chroma_client.reset()`` wipes *every* collection in the shared | |
| # persistent store, destroying other agents' long-term memory that | |
| # happens to live in the same directory. | |
| collection_name = getattr(self, "_collection_name", "memory_store") | |
| try: | |
| self.chroma_client.delete_collection(name=collection_name) | |
| except Exception as e: | |
| self._log_verbose( | |
| f"Error deleting ChromaDB collection '{collection_name}': {e}", | |
| logging.ERROR, | |
| ) | |
| self._init_chroma() # recreate only this collection | |
| # Scope the reset to this instance's own collection. A full | |
| # ``chroma_client.reset()`` wipes *every* collection in the shared | |
| # persistent store, destroying other agents' long-term memory that | |
| # happens to live in the same directory. | |
| collection_name = getattr(self, "_collection_name", "memory_store") | |
| try: | |
| self.chroma_client.delete_collection(name=collection_name) | |
| except Exception as e: | |
| self._log_verbose( | |
| f"Error deleting ChromaDB collection '{collection_name}': {e}", | |
| logging.ERROR, | |
| ) | |
| self._init_chroma() # recreate only this collection | |
| # Sync the recreated client and collection to the adapter to prevent writes to the deleted collection | |
| if getattr(self, "memory_adapter", None) and hasattr(self.memory_adapter, "collection"): | |
| self.memory_adapter.client = self.chroma_client | |
| self.memory_adapter.collection = self.chroma_col |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 1163-1163: Do not catch blind exception: Exception
(BLE001)
🤖 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/memory/memory.py` around lines 1156 -
1168, After the delete and _init_chroma() call, update the active memory_adapter
so its collection reference points to the newly initialized self.chroma_col.
Ensure subsequent store_short_term calls use the recreated collection instead of
the deleted one, without changing the existing scoped deletion or error
handling.
| try: | ||
| from .command_parser import parse_command | ||
| from .command_parser import parse_command, has_unresolvable_expansion | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Catch ImportError instead of a blind Exception to fail fast.
Catching a broad Exception on the import statement masks unexpected programming errors (such as a SyntaxError or NameError within command_parser.py). If the module fails to load due to an internal error, this broad catch will silently downgrade the permission manager to legacy flat matching, which reduces security by bypassing shell decomposition and workspace boundary checks.
As per coding guidelines, "Fail fast with clear exceptions". Catch ImportError instead so that internal codebase errors are explicitly surfaced.
♻️ Proposed fix
try:
from .command_parser import parse_command, has_unresolvable_expansion
- except Exception:
+ except ImportError:
return None📝 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.
| try: | |
| from .command_parser import parse_command | |
| from .command_parser import parse_command, has_unresolvable_expansion | |
| except Exception: | |
| return None | |
| try: | |
| from .command_parser import parse_command, has_unresolvable_expansion | |
| except ImportError: | |
| return None |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 355-355: Do not catch blind exception: Exception
(BLE001)
🤖 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/permissions/manager.py` around lines 353
- 356, In the import block within the permission-manager method, replace the
broad Exception handler around importing parse_command and
has_unresolvable_expansion with an ImportError handler only. Preserve the
existing return None fallback for genuine import failures while allowing
internal module errors such as SyntaxError or NameError to propagate.
Sources: Coding guidelines, Linters/SAST tools
Address two greptile P1 findings on PR #3150: - command_parser: broaden unsafe-expansion detector to match bare $VAR (e.g. `rm -rf $HOME`) in addition to $IFS/${...}; $(...) and backticks remain excluded so command substitution stays on the deny-checked decomposition path. - memory.reset_long_term: re-raise on Chroma delete_collection failure so a failed reset is not silently reported as success while long-term memories remain available. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
| # pattern only accepts ``${`` or ``$`` followed by an identifier char, so | ||
| # ``$(`` (command substitution) never matches. Only parameter expansion, | ||
| # which cannot be statically resolved, is treated as unverifiable. | ||
| _UNSAFE_EXPANSION_RE = re.compile(r"\$(?:\{|[A-Za-z_])") |
There was a problem hiding this comment.
The pattern excludes $((...)) because the character after $ is (. Bash evaluates this expression at runtime, so a broad bash:* allow can still override a deny for the expanded command. For example, a deny for bash:rm -rf /tmp/1 does not match rm -rf /tmp/$((1)), which Bash later executes against the denied path. Detect arithmetic expansion while continuing to exclude $(...) command substitution.
| _UNSAFE_EXPANSION_RE = re.compile(r"\$(?:\{|[A-Za-z_])") | |
| _UNSAFE_EXPANSION_RE = re.compile(r"\$(?:\{|\(\(|[A-Za-z_])") |
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3149
Summary
Addresses three validated high-impact gaps in
praisonaiagentscore, each with a minimal, backward-compatible fix (no new public knobs, defaults unchanged).Gap 1 —
$IFSshell-permission bypass (Security)permissions/command_parser.py+manager.py: the static tokenizer cannot see bash runtime parameter expansion, sorm${IFS}-rf${IFS}/slipped past abash:rm -rf *deny while a broadbash:*allow matched. Now_check_shell_commandescalates any command containing unresolvable parameter expansion ($IFS/${...}) to ASK (an explicit deny still wins). Command substitution ($(...)/backticks) is intentionally excluded — it is already decomposed and deny-checked per-op by #2202, and those tests still pass.Gap 2 —
litellm.callbacksglobal overwrite (Correctness, multi-agent)llm/llm.py:_setup_event_trackingunconditionally didlitellm.callbacks = events, wiping other agents' callbacks process-wide (even from an unrelatedAgentbuilt with the defaultevents=[]). It now no-ops on empty events and merges into the global list, removing only the callbacks this instance previously registered (tracked onself._registered_callbacks).Gap 3 — cross-agent Memory reset wipes shared DB (Data isolation)
memory/memory.py(+adapters/factories.py):reset_long_term()calledchroma_client.reset(), destroying every collection in the shared persistent store. It now scopes deletion to this instance's own collection viadelete_collection(name=...). Collection name is also an opt-incollection_nameconfig override; the default (memory_store) and the documented deterministic-persistence behaviour are unchanged.Scope / critical review
Per AGENTS.md, kept lightweight: no new modules or Agent params. I deliberately did not implement the issue's suggested "namespace every collection by instance" default, since that would break the intended deterministic/persistent collection (
knowledge.py:95) — only the safe collection-scoped reset + opt-in override.Test plan
${IFS}/$IFSpayloads → ASK;$(rm -rf x)→ DENY; normal commands → ALLOWreset()never calledtests/unit/permissions/test_command_aware.py,test_permissions.py— 72 passedtests/test_memory_delete.py— 18 passedGenerated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes