fix: knowledge scope enforcement, async persist offload, tool schema *args/**kwargs (fixes #3834) - #3835
Conversation
…rgs from tool schema (fixes #3834) Gap 1: add shared require_scope() in knowledge/protocols.py and enforce it in ChromaKnowledgeAdapter.delete_all and SQLiteKnowledgeAdapter.delete_all so an unscoped delete_all raises ScopeRequiredError instead of wiping the whole shared store, matching mem0's existing contract. Gap 2: add MemoryMixin._apersist_message async wrapper that offloads _persist_message to a worker thread, and await it at every _achat_impl call site so file-locked disk I/O no longer blocks the event loop each turn (sync _chat_impl and the sync _start_stream generator keep the sync call). Gap 3: skip VAR_POSITIONAL/VAR_KEYWORD in build_parameters_schema so *args and **kwargs are no longer emitted as required parameters, matching build_tool_definition and preventing invalid schemas / tool-call TypeErrors. 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? |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
📝 WalkthroughWalkthroughAsync chat persistence now uses worker-thread offloading. ChromaDB and SQLite deletion require tenant scope. Tool schemas exclude ChangesAsync message persistence
Scoped knowledge deletion
Tool parameter schema
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 aligns knowledge-adapter deletion scope enforcement, moves async assistant-message persistence off the event-loop thread, and makes shared tool-schema generation omit variadic parameters.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect identified. The new scope checks execute before broad adapter exception handling, async persistence remains awaited, and variadic omission is consistent with the existing tool-definition path.
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/chat_mixin.py | Replaces six synchronous assistant-message persistence calls in async chat branches with awaited async wrappers. |
| src/praisonai-agents/praisonaiagents/agent/memory_mixin.py | Adds an async wrapper that offloads synchronous message persistence through asyncio.to_thread. |
| src/praisonai-agents/praisonaiagents/knowledge/adapters/factories.py | Requires tenant scope before Chroma or SQLite bulk deletion while preserving their existing scoped deletion behavior. |
| src/praisonai-agents/praisonaiagents/knowledge/protocols.py | Adds the shared require_scope helper and raises the existing typed scope error for unscoped operations. |
| src/praisonai-agents/praisonaiagents/tools/schema.py | Omits positional and keyword variadic parameters from LLM-facing JSON schemas, matching the sibling schema builder. |
Reviews (1): Last reviewed commit: "fix: enforce knowledge scope, offload as..." | Re-trigger Greptile
|
@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 #3835 — ✅ APPROVE
Phase 1 — Architecture review
Phase 2 — Correctness verification (independent)
Behavioural + test verification
Phase 3 — VerdictNo changes required — approving. The PR is minimal, protocol-driven, backward-compatible, and fixes three real safety/correctness/performance gaps without adding surface area. Files modified by me: none. Job run • Branch: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/tools/schema.py (1)
241-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for variadic filtering.
Verify that a signature such as
(value, *args, **kwargs)exposes onlyvalueinpropertiesandrequired. Cover bothsrc/praisonai-agents/praisonaiagents/tools/base.pyandsrc/praisonai-agents/praisonaiagents/tools/decorator.py, because both callbuild_parameters_schema.🤖 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/tools/schema.py` around lines 241 - 251, Add regression tests for variadic parameter filtering in both the base and decorator tool builders that call build_parameters_schema. Use a function signature of (value, *args, **kwargs) and assert that only value appears in both the generated properties and required fields, with variadic arguments excluded.
🤖 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 3495-3498: Complete async persistence in the chat turn handlers:
after every successful async user-message append to chat_history, including the
unified and custom-provider branches, call _apersist_message for the user with
normalized_content. After each custom-provider assistant response append, call
_apersist_message for the assistant with response_text. Preserve the existing
sync/async APIs and use async persistence without changing message ordering.
- Around line 3495-3498: Update the async chat flow containing
`_build_messages()` and `_apersist_message()` to initialize persistence first:
run `_init_db_session()` and `_init_session_store()` via `asyncio.to_thread()`,
matching the sync path’s setup and ensuring `_session_store` and `_session_id`
are ready before history construction or the first write.
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py`:
- Around line 438-458: Serialize the complete persistence operation in
_apersist_message using an instance-owned asyncio.Lock initialized once for the
owning event loop, then acquire it before calling
asyncio.to_thread(self._persist_message, ...). Ensure concurrent achat() calls
sharing the agent execute _persist_message, including index and statistics
updates, in order while retaining thread offloading for blocking I/O.
In `@src/praisonai-agents/praisonaiagents/knowledge/adapters/factories.py`:
- Around line 371-376: Update ChromaKnowledgeAdapter.delete_all to perform
scoped deletion directly through collection.delete(where=...) using the
applicable user_id, agent_id, and run_id filter, rather than relying on get_all
and its limited peek results. Ensure retrieval or deletion failures propagate
instead of being converted into an empty successful deletion, while preserving
the True result after a successful delete.
In `@src/praisonai-agents/praisonaiagents/knowledge/protocols.py`:
- Around line 215-220: Update the custom message in the ScopeRequiredError
construction for this scope-validation operation to include the backend
identifier alongside the existing operation and required-scope details. Preserve
the current remediation hint and pass the same backend value already supplied to
the exception.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/tools/schema.py`:
- Around line 241-251: Add regression tests for variadic parameter filtering in
both the base and decorator tool builders that call build_parameters_schema. Use
a function signature of (value, *args, **kwargs) and assert that only value
appears in both the generated properties and required fields, with variadic
arguments excluded.
🪄 Autofix
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 Plus
Run ID: e0951210-d9d6-48ba-8e81-0d24ab673d25
📒 Files selected for processing (5)
src/praisonai-agents/praisonaiagents/agent/chat_mixin.pysrc/praisonai-agents/praisonaiagents/agent/memory_mixin.pysrc/praisonai-agents/praisonaiagents/knowledge/adapters/factories.pysrc/praisonai-agents/praisonaiagents/knowledge/protocols.pysrc/praisonai-agents/praisonaiagents/tools/schema.py
| # Persist assistant message to DB (offloaded to a | ||
| # worker thread so file-locked disk I/O doesn't | ||
| # block the event loop on this async turn). | ||
| await self._apersist_message("assistant", response_text) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Complete async persistence coverage.
These changes persist assistant responses in the unified branch, but the async user turn at Lines 3408-3413 is only appended to chat_history. The custom-provider branch at Lines 3287-3292 and 3369 also updates only in-memory history. The sync path persists the corresponding turns at Lines 2770-2774 and 2861-2863.
After initialization is fixed, async sessions can still omit user messages, and custom-provider sessions can omit both sides of the exchange. Add _apersist_message("user", normalized_content) after each successful user append. Add _apersist_message("assistant", response_text) after the custom-provider response append.
As per coding guidelines, preserve sync/async API behavior and use async persistence for all async I/O.
Also applies to: 3515-3517, 3573-3574, 3604-3605, 3623-3624, 3673-3674
🤖 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 3495 -
3498, Complete async persistence in the chat turn handlers: after every
successful async user-message append to chat_history, including the unified and
custom-provider branches, call _apersist_message for the user with
normalized_content. After each custom-provider assistant response append, call
_apersist_message for the assistant with response_text. Preserve the existing
sync/async APIs and use async persistence without changing message ordering.
Source: Coding guidelines
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Initialize persistence before the async chat path uses the wrapper.
The async path does not call _init_db_session() or _init_session_store(). The sync path calls both at Lines 2618-2623 before using history. For a session_id-backed JSON session, _session_store can remain unset, so _persist_message() returns without writing. DB-backed sessions can also lack the generated or restored _session_id.
Initialize the persistence backends before _build_messages() and the first message write. Run the synchronous initializers through asyncio.to_thread().
Suggested initialization
llm_prompt = self._build_multimodal_prompt(prompt, attachments) if attachments else prompt
+ import asyncio
+ await asyncio.to_thread(self._init_db_session)
+ await asyncio.to_thread(self._init_session_store)As per coding guidelines, async I/O must not block the event loop.
🤖 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 3495 -
3498, Update the async chat flow containing `_build_messages()` and
`_apersist_message()` to initialize persistence first: run `_init_db_session()`
and `_init_session_store()` via `asyncio.to_thread()`, matching the sync path’s
setup and ensuring `_session_store` and `_session_id` are ready before history
construction or the first write.
Source: Coding guidelines
| async def _apersist_message( | ||
| self, | ||
| role: str, | ||
| content: str, | ||
| tool_calls=None, | ||
| tool_call_id: Optional[str] = None, | ||
| ): | ||
| """Async wrapper for :meth:`_persist_message`. | ||
|
|
||
| The default JSON session store performs a file-locked, blocking | ||
| read-modify-write to disk on every turn. Calling ``_persist_message`` | ||
| directly from ``_achat_impl`` would stall the entire event loop (other | ||
| agents, streaming, background tasks) for the duration of that I/O on | ||
| every user/assistant/tool turn. Offloading to a worker thread — the | ||
| same treatment ``_persist_compaction_checkpoint`` already gets — keeps | ||
| ``achat()`` non-blocking as documented. | ||
| """ | ||
| import asyncio | ||
| await asyncio.to_thread( | ||
| self._persist_message, role, content, tool_calls, tool_call_id | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize per-agent persistence calls.
When two achat() calls share an agent or session, asyncio.to_thread can run _persist_message concurrently in different worker threads. _persist_message writes the message, updates _auto_save_last_index, and persists session statistics as separate operations. The file lock described in the docstring does not preserve logical message order across calls.
Serialize the full operation with an instance-owned asyncio.Lock or an ordered per-session queue. Initialize the coordination primitive once for the owning event loop.
As per coding guidelines, use asyncio primitives rather than threading for coordination.
🤖 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/memory_mixin.py` around lines 438
- 458, Serialize the complete persistence operation in _apersist_message using
an instance-owned asyncio.Lock initialized once for the owning event loop, then
acquire it before calling asyncio.to_thread(self._persist_message, ...). Ensure
concurrent achat() calls sharing the agent execute _persist_message, including
index and statistics updates, in order while retaining thread offloading for
blocking I/O.
Source: Coding guidelines
| # Get filtered items and delete them | ||
| all_items = self.get_all(user_id=user_id, agent_id=agent_id, run_id=run_id) | ||
| item_ids = [item.id for item in all_items.results] | ||
| if item_ids: | ||
| self.collection.delete(ids=item_ids) | ||
| return True |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '305,378p' src/praisonai-agents/praisonaiagents/knowledge/adapters/factories.py
rg -n -C 4 --glob '*.py' 'def delete_all\(|ChromaKnowledgeAdapter|collection\.(get|delete|peek)\(' src/praisonai-agentsRepository: MervinPraison/PraisonAI
Length of output: 18660
🌐 Web query:
ChromaDB collection delete method support where parameter ids where metadata
💡 Result:
The ChromaDB collection delete method supports both ids and metadata-based filtering, along with document content filtering. You can remove records by specifying their IDs, metadata, or document content [1][2]. The delete method supports the following parameters: - ids: An optional list of strings representing the IDs of the specific records you want to delete [2][3]. - where: An optional dictionary used as a metadata filter to delete records that match specific metadata criteria [2][3][4]. - where_document: An optional dictionary used to filter and delete records based on their document content (e.g., using operators like $contains) [1][2][3]. - limit: An optional integer that specifies the maximum number of records to delete. This parameter can only be used in conjunction with where or where_document filters [3][5]. Note that these filters (where, where_document) are applied to the entire collection to identify records for deletion [2][3]. If no IDs or filters are provided, the operation will raise a ValueError [2][3]. While the delete method removes records, it keeps the collection itself intact; to delete an entire collection, you must use the delete_collection method instead [1][6].
Citations:
- 1: https://cookbook.chromadb.dev/core/collections/
- 2: https://docs.trychroma.com/reference/python/collection
- 3: https://github.com/chroma-core/chroma/blob/main/chromadb/api/models/Collection.py
- 4: https://docs.trychroma.com/docs/collections/delete-data
- 5: https://github.com/chroma-core/chroma/blob/main/chromadb/api/fastapi.py
- 6: https://docs.trychroma.com/docs/collections/manage-collections
Use a ChromaDB-side delete for scoped deletion.
ChromaKnowledgeAdapter.delete_all builds the deletion key from get_all, which checks at most 100 unfiltered records with collection.peek(limit=100), and turns retrieval failures into empty results. A scope with records beyond the first page can return True while keeping those records. Use collection.delete(where=...) with the scope filter or page scoped IDs, and keep retrieval failures as failures.
🤖 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/knowledge/adapters/factories.py` around
lines 371 - 376, Update ChromaKnowledgeAdapter.delete_all to perform scoped
deletion directly through collection.delete(where=...) using the applicable
user_id, agent_id, and run_id filter, rather than relying on get_all and its
limited peek results. Ensure retrieval or deletion failures propagate instead of
being converted into an empty successful deletion, while preserving the True
result after a successful delete.
| raise ScopeRequiredError( | ||
| message=( | ||
| f"{operation} requires at least one of 'user_id', 'agent_id', " | ||
| f"or 'run_id' to scope the {operation}." | ||
| ), | ||
| backend=backend, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the backend in the exception message.
The custom message bypasses ScopeRequiredError's backend-aware default message. As a result, str(error) does not identify the failing backend. Include backend in this message for direct diagnostics.
Proposed fix
- f"or 'run_id' to scope the {operation}."
+ f"or 'run_id' to scope the {operation}"
+ f"{f' for the {backend} backend' if backend else ''}."As per coding guidelines, “Fail fast with clear exceptions, include remediation hints, propagate agent/tool/session context.”
📝 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.
| raise ScopeRequiredError( | |
| message=( | |
| f"{operation} requires at least one of 'user_id', 'agent_id', " | |
| f"or 'run_id' to scope the {operation}." | |
| ), | |
| backend=backend, | |
| raise ScopeRequiredError( | |
| message=( | |
| f"{operation} requires at least one of 'user_id', 'agent_id', " | |
| f"or 'run_id' to scope the {operation}" | |
| f"{f' for the {backend} backend' if backend else ''}." | |
| ), | |
| backend=backend, |
🤖 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/knowledge/protocols.py` around lines 215
- 220, Update the custom message in the ScopeRequiredError construction for this
scope-validation operation to include the backend identifier alongside the
existing operation and required-scope details. Preserve the current remediation
hint and pass the same backend value already supplied to the exception.
Source: Coding guidelines
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #3834
Summary
Minimal, focused fixes for the three core-SDK gaps in the audit.
Gap 1 — Knowledge adapters diverged on tenant-scope (data-loss/leak)
require_scope(...)helper inknowledge/protocols.py.ChromaKnowledgeAdapter.delete_allandSQLiteKnowledgeAdapter.delete_allnow call it, so an unscopeddelete_all()raisesScopeRequiredError(matching the mem0 backend) instead of wiping the entire shared collection/table. Scoped deletes are unchanged.Gap 2 — Async chat blocked the event loop on every turn
MemoryMixin._apersist_message, a thin async wrapper that offloads the file-locked_persist_messagedisk write viaasyncio.to_thread(the same treatment_persist_compaction_checkpointalready got)._persist_messagecall sites inside_achat_impltoawait self._apersist_message(...)._chat_impland the synchronous_start_streamgenerator correctly keep the sync call (the issue's line refs for the streaming sites actually fell in the sync generator).Gap 3 —
build_parameters_schemamis-marked*args/**kwargsas requiredVAR_POSITIONAL/VAR_KEYWORDparams inbuild_parameters_schema, matching the filterbuild_tool_definitionalready applies.*args/**kwargsare no longer emitted as properties or inrequired.Verification
@tool def f(query: str, **kwargs)→required == ['query'], nokwargsproperty.@tool def f(query: str, *args, flag=False)→required == ['query'].delete_all()on SQLite and Chroma adapters raisesScopeRequiredError; scoped delete still removes only the matching rows._apersist_messageis a coroutine offloading to a worker thread; all 6 async call sites confirmed inside_achat_impl.Note: the sandbox blocked direct
pytestinvocation, so verification was done via targeted import/behaviour scripts.Generated with Claude Code
Summary by CodeRabbit
Performance
Bug Fixes