feat(memory): add Perseus Vault memory adapter (local-first, single-binary, no SDK dep)#2900
feat(memory): add Perseus Vault memory adapter (local-first, single-binary, no SDK dep)#2900tcconnally wants to merge 3 commits into
Conversation
Adds `perseus_vault` as a registered memory backend implementing `MemoryProtocol`, alongside the existing sqlite/in_memory/mem0/chroma/ mongodb/dakera adapters. Perseus Vault (https://github.com/Perseus-Computing-LLC/perseus-vault) is a single static-binary MCP memory server: SQLite + FTS5 + bundled ONNX embeddings, optional AES-256-GCM, fully local and offline. Unlike the other wrapper adapters it needs **no third-party Python SDK** — the adapter speaks JSON-RPC 2.0 over the binary's stdio `serve` transport directly, matching Perseus Vault's zero-dependency, local-first design. - Short-term and long-term memory map onto vault entity categories (`working` / `episodic` by default), so the tiers stay independently queryable and resettable. - Search uses hybrid retrieval (FTS5 + dense vector fusion) by default; configurable via `search_mode` (fts5 | dense | hybrid). - `get_context` returns the vault's pre-rendered markdown injection block. - `reset_short_term`/`reset_long_term` use category-scoped prune (purge_all), leaving the other tier untouched. Config keys (top level or nested under `config`, mirroring mem0/dakera): `binary`/`bin`, `db_path`/`db`, `encryption_key`, `short_term_category`, `long_term_category`, `search_mode`, `default_importance`. Each falls back to the matching PERSEUS_VAULT_* env var. Includes: - praisonaiagents/memory/adapters/perseus_vault_adapter.py (adapter + stdio client) - factory create_perseus_vault_memory_adapter + registration in adapters/__init__ - examples/python/memory/perseus_vault_memory.py - tests/unit/memory/test_perseus_vault_adapter.py (16 tests, fake stdio client, no binary required — mirrors test_dakera_adapter.py) Verified end-to-end against perseus-vault 2.20.0 (store/search/reset/context round-trips) and via the mock-based unit suite (16 passed); existing memory adapter + registry + protocol tests remain green (70 passed).
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? |
📝 WalkthroughWalkthroughAdds a Perseus Vault-backed memory adapter using a local JSON-RPC stdio process, registers it as a memory provider, supplies a workflow example, and adds unit tests for configuration, tiered operations, deletion, reset, context retrieval, and timeout cleanup. ChangesPerseus Vault memory integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentWorkflow
participant PerseusVaultMemoryAdapter
participant _VaultStdioClient
participant perseus_vault_process
AgentWorkflow->>PerseusVaultMemoryAdapter: store or retrieve memory
PerseusVaultMemoryAdapter->>_VaultStdioClient: invoke remember or recall
_VaultStdioClient->>perseus_vault_process: send JSON-RPC over stdio
perseus_vault_process-->>_VaultStdioClient: return tool result
_VaultStdioClient-->>PerseusVaultMemoryAdapter: decode response
PerseusVaultMemoryAdapter-->>AgentWorkflow: return memory data or context
Possibly related PRs
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 |
MervinPraison
added
pipeline/blocked:ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new memory adapter for Perseus Vault, enabling a local-first, zero-dependency SQLite-based memory backend for PraisonAI agents. The changes include the core adapter implementation, a custom JSON-RPC stdio client, integration into the adapter registry, unit tests, and an example script. The review feedback highlights critical concurrency issues in the stdio client, such as a potential deadlock due to non-reentrant locking and a race condition during subprocess initialization. Additionally, the feedback identifies protocol non-compliance in delete_memory regarding the memory_type parameter, and suggests propagating custom importance values when storing memories.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self._db_path = db_path | ||
| self._encryption_key = encryption_key | ||
| self._env = {**os.environ, **(env or {})} | ||
| self._lock = threading.Lock() |
There was a problem hiding this comment.
Using a standard threading.Lock will cause a deadlock here because _start() is called from within _request (which acquires the lock) and recursively calls _request again. Since a standard lock is not reentrant, the second acquisition attempt will block indefinitely. Changing this to threading.RLock() allows the same thread to acquire the lock multiple times safely.
| self._lock = threading.Lock() | |
| self._lock = threading.RLock() |
| def _request(self, method: str, params: Dict[str, Any]) -> Dict[str, Any]: | ||
| if self._proc is None or self._proc.poll() is not None: | ||
| self._start() | ||
| with self._lock: | ||
| rid = self._next_id() | ||
| msg = {"jsonrpc": "2.0", "id": rid, "method": method, "params": params} | ||
| assert self._proc and self._proc.stdin and self._proc.stdout | ||
| self._proc.stdin.write(json.dumps(msg) + "\n") | ||
| self._proc.stdin.flush() |
There was a problem hiding this comment.
Spawning the process outside of the lock creates a race condition where multiple threads calling _request concurrently can spawn multiple subprocesses and leak resources. Moving the process check and startup inside the lock ensures thread-safe initialization. This is safe to do now that self._lock is a reentrant RLock.
| def _request(self, method: str, params: Dict[str, Any]) -> Dict[str, Any]: | |
| if self._proc is None or self._proc.poll() is not None: | |
| self._start() | |
| with self._lock: | |
| rid = self._next_id() | |
| msg = {"jsonrpc": "2.0", "id": rid, "method": method, "params": params} | |
| assert self._proc and self._proc.stdin and self._proc.stdout | |
| self._proc.stdin.write(json.dumps(msg) + "\n") | |
| self._proc.stdin.flush() | |
| def _request(self, method: str, params: Dict[str, Any]) -> Dict[str, Any]: | |
| with self._lock: | |
| if self._proc is None or self._proc.poll() is not None: | |
| self._start() | |
| rid = self._next_id() | |
| msg = {"jsonrpc": "2.0", "id": rid, "method": method, "params": params} | |
| assert self._proc and self._proc.stdin and self._proc.stdout | |
| self._proc.stdin.write(json.dumps(msg) + "\n") | |
| self._proc.stdin.flush() |
| def delete_memory(self, memory_id: str, memory_type: Optional[str] = None) -> bool: | ||
| cat = self._st_cat if memory_type == "short" else self._lt_cat |
There was a problem hiding this comment.
The MemoryProtocol defines memory_type values as "short_term" or "long_term". Checking only for "short" will cause delete_memory to incorrectly default to the long-term category (self._lt_cat) when memory_type="short_term" is passed. Checking for both "short" and "short_term" ensures protocol compliance.
| def delete_memory(self, memory_id: str, memory_type: Optional[str] = None) -> bool: | |
| cat = self._st_cat if memory_type == "short" else self._lt_cat | |
| def delete_memory(self, memory_id: str, memory_type: Optional[str] = None) -> bool: | |
| cat = self._st_cat if memory_type in ("short", "short_term") else self._lt_cat |
| def _store(self, category: str, text: str, metadata: Optional[Dict[str, Any]]) -> str: | ||
| key = (metadata or {}).get("key") or f"{category}-{uuid.uuid4().hex[:12]}" | ||
| body = {"content": text} | ||
| if metadata: | ||
| body["metadata"] = metadata | ||
| self._client.call_tool("perseus_vault_remember", { | ||
| "category": category, | ||
| "key": key, | ||
| "body_json": json.dumps(body), | ||
| "importance": self._default_importance, | ||
| }) | ||
| return key |
There was a problem hiding this comment.
The _store method currently hardcodes self._default_importance and ignores any custom importance value passed via kwargs or metadata. Extracting importance from kwargs or metadata (similar to how other adapters like DakeraMemoryAdapter do it) allows storing memories with custom importance levels.
def _store(self, category: str, text: str, metadata: Optional[Dict[str, Any]], **kwargs) -> str:
key = (metadata or {}).get("key") or f"{category}-{uuid.uuid4().hex[:12]}"
body = {"content": text}
if metadata:
body["metadata"] = metadata
importance = kwargs.get("importance")
if importance is None and metadata:
importance = metadata.get("importance")
if importance is None:
importance = self._default_importance
else:
importance = float(importance)
self._client.call_tool("perseus_vault_remember", {
"category": category,
"key": key,
"body_json": json.dumps(body),
"importance": importance,
})
return key| def store_short_term(self, text: str, metadata: Optional[Dict[str, Any]] = None, **kwargs) -> str: | ||
| return self._store(self._st_cat, text, metadata) |
There was a problem hiding this comment.
Pass **kwargs to _store so that custom parameters like importance can be propagated and stored.
| def store_short_term(self, text: str, metadata: Optional[Dict[str, Any]] = None, **kwargs) -> str: | |
| return self._store(self._st_cat, text, metadata) | |
| def store_short_term(self, text: str, metadata: Optional[Dict[str, Any]] = None, **kwargs) -> str: | |
| return self._store(self._st_cat, text, metadata, **kwargs) |
| def store_long_term(self, text: str, metadata: Optional[Dict[str, Any]] = None, **kwargs) -> str: | ||
| return self._store(self._lt_cat, text, metadata) |
There was a problem hiding this comment.
Pass **kwargs to _store so that custom parameters like importance can be propagated and stored.
| def store_long_term(self, text: str, metadata: Optional[Dict[str, Any]] = None, **kwargs) -> str: | |
| return self._store(self._lt_cat, text, metadata) | |
| def store_long_term(self, text: str, metadata: Optional[Dict[str, Any]] = None, **kwargs) -> str: | |
| return self._store(self._lt_cat, text, metadata, **kwargs) |
Greptile SummaryThis PR adds Perseus Vault as a local memory backend. The main changes are:
Confidence Score: 4/5This is close, but the delete routing should be fixed before merging.
src/praisonai-agents/praisonaiagents/memory/adapters/perseus_vault_adapter.py Important Files Changed
Reviews (3): Last reviewed commit: "fix(memory): address post-review finding..." | 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/blocked:cooldown
|
Claude finished @MervinPraison's task —— View job Perseus Vault Adapter — Final Architecture Review ✅Verdict: Approve (after fixes). Architecture is sound and the fix commit is pushed to the fork branch. Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Fixes pushed (
|
![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 (2)
src/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py (2)
205-209: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for deleting a non-existent memory.
The adapter's
delete_memoryreturnsTruewhenevercall_tool("perseus_vault_forget", ...)doesn't raise, regardless of whether the entity actually existed. The fake client never raises for missing keys, sodelete_memoryalways returnsTrue. A test assertingdelete_memory("nonexistent-key")returnsFalse(or documenting the current always-Truebehavior) would clarify the intended contract.🧪 Suggested additional test
+ def test_delete_nonexistent_memory_returns_false(self): + adapter = _make_adapter() + # No memory stored with this key — should not report success. + assert adapter.delete_memory("does-not-exist") is FalseNote: This test will currently fail because the adapter sets
ok = Trueon any non-exception return fromcall_tool. If the intended contract is to returnFalsewhen the memory doesn't exist, the adapter should inspect the{"archived": N}response and setokonly whenN > 0.🤖 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/memory/test_perseus_vault_adapter.py` around lines 205 - 209, Add coverage for deleting a missing key in test_delete_memory by asserting delete_memory("nonexistent-key") returns False; update the adapter’s delete_memory implementation to inspect the perseus_vault_forget response and return True only when archived is greater than zero, otherwise False.
1-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding a real agentic test per coding guidelines.
The coding guidelines require that "every feature requires both smoke tests and a real agentic test that calls
agent.start()with a real prompt, invokes the LLM, and prints the full output." The current 16 tests are all mock-based unit tests. If a real agentic test is planned for a separate e2e file, consider noting that here.As per coding guidelines: "Every feature requires both smoke tests and a real agentic test that calls
agent.start()with a real prompt, invokes the LLM, and prints the full output."🤖 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/memory/test_perseus_vault_adapter.py` around lines 1 - 222, Add a separate real agentic/e2e test for the Perseus Vault feature that constructs the relevant agent, calls agent.start() with a meaningful prompt, invokes the configured LLM, and prints the complete output; if this test is intentionally maintained elsewhere, document its location or planned coverage alongside TestPerseusVaultBehavior.Source: Coding guidelines
🤖 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/examples/python/memory/perseus_vault_memory.py`:
- Around line 14-16: Add a concise expected-output comment immediately after the
workflow invocation in the example’s main execution flow, documenting the
successful output produced by workflow.start().
In
`@src/praisonai-agents/praisonaiagents/memory/adapters/perseus_vault_adapter.py`:
- Around line 223-230: Update get_all_memories to paginate through each category
via _search until no more results are returned, rather than limiting each
category to the default 100 entries. Treat kwargs["limit"] as an optional
overall cap across both categories, stop once it is reached, and preserve
returning all memories when no limit is provided.
- Around line 159-162: The configuration initialization in the Perseus vault
adapter currently accepts invalid values. In the constructor or setup logic
containing _search_mode and _default_importance, validate search_mode against
the supported modes and ensure default_importance is finite and within its
documented range; raise remediation-focused ValueErrors that state the invalid
value and how to correct it before starting the client.
- Around line 29-124: Replace the blocking subprocess and threading
implementation in _VaultStdioClient with asyncio subprocess streams and
asyncio.Lock, using asyncio.wait_for around every stream read/write and startup
handshake so timeouts remain effective. Add async methods for startup, requests,
notifications, tool calls, and close, then expose synchronous wrappers that run
the async operations without nesting an existing event loop; update adapter
methods in the referenced later section to provide both async and synchronous
entry points while using the async implementation internally.
- Around line 247-260: The delete_memory method should only return success when
perseus_vault_forget explicitly confirms deletion. Inspect each call_tool
response for the tool’s success/deletion indicator before setting ok, and
preserve or propagate exceptions and failure responses rather than silently
swallowing them and continuing. Update the loop in delete_memory accordingly
while retaining support for trying both categories when memory_type is
unspecified.
---
Nitpick comments:
In `@src/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py`:
- Around line 205-209: Add coverage for deleting a missing key in
test_delete_memory by asserting delete_memory("nonexistent-key") returns False;
update the adapter’s delete_memory implementation to inspect the
perseus_vault_forget response and return True only when archived is greater than
zero, otherwise False.
- Around line 1-222: Add a separate real agentic/e2e test for the Perseus Vault
feature that constructs the relevant agent, calls agent.start() with a
meaningful prompt, invokes the configured LLM, and prints the complete output;
if this test is intentionally maintained elsewhere, document its location or
planned coverage alongside TestPerseusVaultBehavior.
🪄 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: 6a9d3738-b0d4-41f6-b9fe-8f11478c71ec
📒 Files selected for processing (5)
src/praisonai-agents/examples/python/memory/perseus_vault_memory.pysrc/praisonai-agents/praisonaiagents/memory/adapters/__init__.pysrc/praisonai-agents/praisonaiagents/memory/adapters/factories.pysrc/praisonai-agents/praisonaiagents/memory/adapters/perseus_vault_adapter.pysrc/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py
| class _VaultStdioClient: | ||
| """Minimal JSON-RPC 2.0 client over the perseus-vault `serve` stdio transport. | ||
|
|
||
| Spawns `perseus-vault serve --db <path>` as a long-lived child and exchanges | ||
| newline-delimited JSON-RPC messages over its stdin/stdout. Thread-safe: a | ||
| single lock serializes request/response round-trips. | ||
| """ | ||
|
|
||
| def __init__(self, binary: str, db_path: str, env: Optional[Dict[str, str]] = None, | ||
| encryption_key: Optional[str] = None, startup_timeout: float = 10.0): | ||
| self._binary = binary | ||
| self._db_path = db_path | ||
| self._encryption_key = encryption_key | ||
| self._env = {**os.environ, **(env or {})} | ||
| self._lock = threading.Lock() | ||
| self._id = 0 | ||
| self._proc: Optional[subprocess.Popen] = None | ||
| self._startup_timeout = startup_timeout | ||
| self._start() | ||
|
|
||
| def _start(self) -> None: | ||
| cmd = [self._binary, "serve", "--db", self._db_path] | ||
| if self._encryption_key: | ||
| cmd += ["--encryption-key", self._encryption_key] | ||
| self._proc = subprocess.Popen( | ||
| cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, | ||
| stderr=subprocess.DEVNULL, text=True, bufsize=1, env=self._env, | ||
| ) | ||
| # Handshake: initialize + notifications/initialized. | ||
| self._request("initialize", { | ||
| "protocolVersion": "2024-11-05", | ||
| "capabilities": {}, | ||
| "clientInfo": {"name": "praisonai-perseus-vault", "version": "0.1.0"}, | ||
| }) | ||
| self._notify("notifications/initialized", {}) | ||
|
|
||
| def _next_id(self) -> int: | ||
| self._id += 1 | ||
| return self._id | ||
|
|
||
| def _request(self, method: str, params: Dict[str, Any]) -> Dict[str, Any]: | ||
| if self._proc is None or self._proc.poll() is not None: | ||
| self._start() | ||
| with self._lock: | ||
| rid = self._next_id() | ||
| msg = {"jsonrpc": "2.0", "id": rid, "method": method, "params": params} | ||
| assert self._proc and self._proc.stdin and self._proc.stdout | ||
| self._proc.stdin.write(json.dumps(msg) + "\n") | ||
| self._proc.stdin.flush() | ||
| # Read until we get the response with our id (skip notifications). | ||
| deadline = time.time() + self._startup_timeout | ||
| while time.time() < deadline: | ||
| line = self._proc.stdout.readline() | ||
| if not line: | ||
| raise RuntimeError("perseus-vault closed stdout unexpectedly") | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| resp = json.loads(line) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if resp.get("id") == rid: | ||
| if "error" in resp and resp["error"]: | ||
| raise RuntimeError(f"perseus-vault error: {resp['error']}") | ||
| return resp.get("result", {}) | ||
| raise TimeoutError(f"perseus-vault did not respond to {method} in time") | ||
|
|
||
| def _notify(self, method: str, params: Dict[str, Any]) -> None: | ||
| with self._lock: | ||
| assert self._proc and self._proc.stdin | ||
| msg = {"jsonrpc": "2.0", "method": method, "params": params} | ||
| self._proc.stdin.write(json.dumps(msg) + "\n") | ||
| self._proc.stdin.flush() | ||
|
|
||
| def call_tool(self, name: str, arguments: Dict[str, Any]) -> Any: | ||
| """Invoke an MCP tool; return the parsed JSON body of the first content item.""" | ||
| result = self._request("tools/call", {"name": name, "arguments": arguments}) | ||
| content = result.get("content", []) | ||
| if not content: | ||
| return result | ||
| text = content[0].get("text", "") | ||
| try: | ||
| return json.loads(text) | ||
| except (json.JSONDecodeError, TypeError): | ||
| return text | ||
|
|
||
| def close(self) -> None: | ||
| if self._proc and self._proc.poll() is None: | ||
| try: | ||
| if self._proc.stdin: | ||
| self._proc.stdin.close() | ||
| self._proc.terminate() | ||
| self._proc.wait(timeout=5) | ||
| except Exception: | ||
| self._proc.kill() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Replace blocking stdio with a genuinely timeout-bounded async transport.
Line 81’s blocking readline() can wait forever, so the deadline is ineffective while the child remains alive but silent. All adapter operations consequently block an async agent context. Use asyncio subprocess streams with asyncio.wait_for, then provide non-nesting synchronous entry points.
As per coding guidelines, “All I/O operations must have async variants, synchronous I/O must not block an async context, and coordination should use asyncio primitives rather than threading,” and “Provide both synchronous and asynchronous user-facing entry points, prefer async internally.”
Also applies to: 211-282
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 75-75: use jsonify instead of json.dumps for JSON output
Context: json.dumps(msg)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 100-100: use jsonify instead of json.dumps for JSON output
Context: json.dumps(msg)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 52-55: Command coming from incoming request
Context: subprocess.Popen(
cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL, text=True, bufsize=1, env=self._env,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 52-55: Use of unsanitized data to create processes
Context: subprocess.Popen(
cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL, text=True, bufsize=1, env=self._env,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
🪛 Ruff (0.15.20)
[error] 53-53: subprocess call: check for execution of untrusted input
(S603)
[warning] 92-92: Unnecessary key check before dictionary access
Replace with dict.get
(RUF019)
[warning] 123-123: 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/adapters/perseus_vault_adapter.py`
around lines 29 - 124, Replace the blocking subprocess and threading
implementation in _VaultStdioClient with asyncio subprocess streams and
asyncio.Lock, using asyncio.wait_for around every stream read/write and startup
handshake so timeouts remain effective. Add async methods for startup, requests,
notifications, tool calls, and close, then expose synchronous wrappers that run
the async operations without nesting an existing event loop; update adapter
methods in the referenced later section to provide both async and synchronous
entry points while using the async implementation internally.
Source: Coding guidelines
| def get_all_memories(self, **kwargs) -> List[Dict[str, Any]]: | ||
| # Enumerate both tiers. An empty query returns all entities in a | ||
| # category (ordered by the vault's recency/decay ranking). | ||
| results: List[Dict[str, Any]] = [] | ||
| limit = kwargs.get("limit", 100) | ||
| for cat in (self._st_cat, self._lt_cat): | ||
| results.extend(self._search(cat, "", limit)) | ||
| return results |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Paginate instead of truncating get_all_memories.
The default returns at most 100 entries per category, despite the method and comment promising all memories. Fetch successive pages until exhausted; treat a caller-provided limit as an explicit total cap.
🤖 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/adapters/perseus_vault_adapter.py`
around lines 223 - 230, Update get_all_memories to paginate through each
category via _search until no more results are returned, rather than limiting
each category to the default 100 entries. Treat kwargs["limit"] as an optional
overall cap across both categories, stop once it is reached, and preserve
returning all memories when no limit is provided.
- Use RLock so the handshake recursion (_request -> _start -> _request) does not deadlock, and move process spawn inside the lock to close a concurrent-startup race. - Make the stdio timeout actually fire: read stdout on a daemon thread so a stuck/slow binary raises TimeoutError instead of hanging every call. - delete_memory now honours the MemoryProtocol "short_term"/"long_term" hints (not just "short") and only reports success when the vault actually archived the entity. - Normalise search results so metadata is always a dict and score always numeric (never None), matching what callers expect. - Propagate caller-supplied importance (kwargs > metadata > default) through store_short_term/store_long_term. - Add unit tests for the above. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adapters/perseus_vault_adapter.py`:
- Around line 82-116: Fix timed-out reads in _readline_with_timeout and _request
so blocked reader threads cannot remain attached to a reused stdout stream. On
any response timeout, terminate and clean up the current child process, reset
its state, and ensure the next request starts a fresh process before reading;
alternatively replace the per-read daemon thread approach with a single
persistent reader or asynchronous subprocess transport.
🪄 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: d5cdd14b-ff34-4717-a1ce-ae6ed6ddaf20
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/memory/adapters/perseus_vault_adapter.pysrc/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py
Follow-up to the triage-agent hardening commit, resolving the remaining review findings from Gemini / CodeRabbit / Greptile: - Stdio timeout now tears down the child process (new _teardown) instead of leaving a blocked daemon reader attached to a reused stdout. Prevents a later call from racing a stale reader and stealing its response. close() reuses the same teardown under the lock. - delete_memory no longer treats a non-dict vault response as success — only an explicit archived>0 counts, so an ambiguous/unknown response reports False rather than a delete that may not have happened. - Validate config on init: reject unsupported search_mode and non-numeric / out-of-range default_importance with clear ValueError. - Example: add expected-output comment per the examples guideline. Tests: +5 (invalid search_mode, out-of-range/non-numeric importance, non-dict delete not success, real-subprocess timeout teardown). Full adapter suite 25 passed; memory/registry/protocol regression suite 70 passed, 1 skipped; verified end-to-end against perseus-vault 2.20.0.
|
Thanks @MervinPraison — the triage-agent hardening commit (RLock handshake, daemon-thread timeout, protocol-correct delete, result normalization, importance propagation) all looks right, and I appreciate the fast, thorough review pass. I pushed a follow-up (
Tests: +5 (incl. a real-subprocess test that proves a hung binary raises Happy to keep iterating on anything else you'd like adjusted. |
MervinPraison
added
the
pipeline/blocked:manual-review
| else: | ||
| # Type unspecified: try both tiers. | ||
| cats = [self._st_cat, self._lt_cat] |
There was a problem hiding this comment.
memory_type to both memory tiers. delete_memory(id, memory_type="entity") can return True if the same key exists in short- or long-term memory, even though no entity memory was deleted. Callers then receive a successful deletion result for the wrong memory namespace. Unsupported types should return False or be handled explicitly instead of falling back to both tiers.
| else: | |
| # Type unspecified: try both tiers. | |
| cats = [self._st_cat, self._lt_cat] | |
| elif memory_type is None: | |
| # Type unspecified: try both tiers. | |
| cats = [self._st_cat, self._lt_cat] | |
| else: | |
| return False |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py (1)
275-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic-analysis S603/CWE-78 flag is a false positive; test logic is sound.
The
subprocess.Popenat Line 323 is flagged by Ruff (S603) and ast-grep (CWE-78) for OS-command injection. In context, both arguments (sys.executableandstr(fake)) are fully test-controlled —fakeis a temp file written by the test itself undertmp_path. There is no untrusted or request-derived input, so the flag is not actionable.The test correctly validates the key contract: after a successful handshake, a hung subsequent request must raise
TimeoutError, terminate the child process, and reset_proctoNone.One fragility note: the test bypasses the constructor via
_VaultStdioClient.__new__and manually initializes private fields (Lines 310-319). If the internal field set or defaults change, this test will break withAttributeErrors rather than a clear failure. Consider extracting a smallfrom_configclassmethod or a test-only builder on_VaultStdioClientso the field initialization stays in one place. This is optional and can be deferred.🤖 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/memory/test_perseus_vault_adapter.py` around lines 275 - 338, Retain the subprocess test as-is; the S603/CWE-78 warning is a false positive because both command arguments are fully test-controlled. Optionally improve maintainability by adding a small test-only builder or _VaultStdioClient.from_config classmethod and updating test_timeout_tears_down_process to use it instead of __new__ with manual private-field initialization.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py`:
- Around line 275-338: Retain the subprocess test as-is; the S603/CWE-78 warning
is a false positive because both command arguments are fully test-controlled.
Optionally improve maintainability by adding a small test-only builder or
_VaultStdioClient.from_config classmethod and updating
test_timeout_tears_down_process to use it instead of __new__ with manual
private-field initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd11c8ea-856f-470c-bc43-6f4e739b3353
📒 Files selected for processing (3)
src/praisonai-agents/examples/python/memory/perseus_vault_memory.pysrc/praisonai-agents/praisonaiagents/memory/adapters/perseus_vault_adapter.pysrc/praisonai-agents/tests/unit/memory/test_perseus_vault_adapter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/praisonai-agents/examples/python/memory/perseus_vault_memory.py
…transport Adds `integrations/client/` (package `perseus-vault-client`) — a small, zero-runtime-dependency client that drives a local `perseus-vault serve` process over its MCP JSON-RPC 2.0 stdio transport. Motivation: the LangGraph / CrewAI / AutoGen adapters (and the new PraisonAI adapter, MervinPraison/PraisonAI#2900) each re-implement this transport and independently re-discover the same bugs the PraisonAI review surfaced. This centralizes and hardens it once: - Reentrant-lock handshake (initialize recurses through _request); spawn under the lock to avoid a concurrent-startup race that leaks children. - Deadline-bounded reads on a daemon thread; on timeout the child is torn down so a later call never races a still-blocked reader on a reused stdout. - Auto-respawn on dead child; normalized recall items ({id, text, metadata, score, raw}) with score never None. - Typed helpers: remember / recall / semantic_search / scan (paginated full-category enumeration) / context / forget / prune / get_entity / stats / health, plus call_tool + list_tools escape hatches. The client is transport-only and framework-agnostic; downstream adapters (and the planned pydantic-ai harness capability, #552) become thin mappings onto it instead of re-implementing stdio. Tests (15, all passing): helper/normalization logic via an in-memory fake; real-subprocess transport behaviors (round-trip, timeout-teardown, reentrant handshake, missing-binary error); and a real-binary store/recall/prune test gated on PERSEUS_VAULT_BIN. Package builds clean (wheel + sdist), zero deps. Refs #561. Follow-up: refactor existing adapters to consume this client.
2 participants
Summary
Adds
perseus_vaultas a registered memory backend implementingMemoryProtocol, alongside the existingsqlite/in_memory/mem0/chroma/mongodb/dakeraadapters.Perseus Vault is a single static-binary MCP memory server — SQLite + FTS5 + bundled ONNX embeddings, optional AES-256-GCM, fully local and offline. Unlike the other wrapper adapters it needs no third-party Python SDK: the adapter speaks JSON-RPC 2.0 over the binary's stdio
servetransport directly, matching Perseus Vault's zero-dependency, local-first design (and following the protocol-driven adapter pattern inmemory/adapters/).What's included
praisonaiagents/memory/adapters/perseus_vault_adapter.py— the adapter plus a minimal thread-safe JSON-RPC stdio client for the vault binary.create_perseus_vault_memory_adapterfactory infactories.py+ registration inadapters/__init__.py(register_memory_factory("perseus_vault", ...)).examples/python/memory/perseus_vault_memory.py.tests/unit/memory/test_perseus_vault_adapter.py— 16 tests using a fake stdio client, no binary required, mirroringtest_dakera_adapter.py.Design
working/episodicby default) so each tier stays independently queryable and resettable.search_mode=fts5|dense|hybrid.get_contextreturns the vault's pre-rendered markdown injection block.reset_short_term/reset_long_termuse a category-scoped prune (purge_all), leaving the other tier untouched.Config keys (top level or nested under
config, mirroring mem0/dakera):binary/bin,db_path/db,encryption_key,short_term_category,long_term_category,search_mode,default_importance— each falling back to the matchingPERSEUS_VAULT_*env var.Usage
Verification
perseus-vault 2.20.0: store/search (both tiers, correctly isolated),get_context,get_all_memories, and category-scoped reset all round-trip through the real MCP protocol.Happy to adjust naming, config keys, or packaging (e.g. move to an optional-extra) to fit your conventions.
Summary by CodeRabbit