feat(integrations): add DakeraMemoryLogger — persistent cross-session memory for all LLM providers - #31812
Conversation
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge — the integration is well-scoped, fully opt-in, and degrades gracefully on Dakera unavailability. The new logger is additive and isolated: it only activates when explicitly registered as a callback, swallows its own errors so it cannot break the request path, and uses the official Dakera SDK instead of hand-rolled HTTP. All issues flagged in the previous review round have been corrected. The test suite is entirely mock-based and covers the core contract thoroughly. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/integrations/dakera_memory.py | New CustomLogger that recalls and stores LLM exchanges via the official Dakera SDK; previous concerns (custom httpx usage, unused import, unguarded key access) are all addressed in this revision. |
| tests/test_litellm/integrations/test_dakera_memory.py | 13 unit tests covering recall injection position, multimodal extraction, error swallowing, session isolation, and the missing-SDK path; all SDK calls are mocked — no real network calls. |
Reviews (2): Last reviewed commit: "fix(integrations): use official dakera S..." | Re-trigger Greptile
| return data | ||
|
|
||
| # Build memory context string | ||
| memory_lines = "\n".join(f"- {r['content']}" for r in results) |
There was a problem hiding this comment.
Unguarded key access causes
KeyError on malformed results: r['content'] raises KeyError if any result dict returned by Dakera is missing the content key (e.g. unexpected API shape, future schema change). Use .get() with a fallback to make the injection robust.
| memory_lines = "\n".join(f"- {r['content']}" for r in results) | |
| memory_lines = "\n".join(f"- {r.get('content', '')}" for r in results if r.get('content')) |
| #!/usr/bin/env bash | ||
| # QA: code interpreter sandbox stickiness via metadata.session_id | ||
| # bash qa_sticky_session.sh | ||
| # LITELLM_BASE_URL=http://localhost:4000 LITELLM_KEY=sk-1234 bash qa_sticky_session.sh | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| BASE="${LITELLM_BASE_URL:-http://localhost:4000}" | ||
| KEY="${LITELLM_KEY:-sk-1234}" | ||
| MODEL="${LITELLM_MODEL:-gpt-4o-mini}" | ||
| # proxy running at http://localhost:4000 (master key: sk-1234) | ||
| SESSION_A="qa-session-$(date +%s)-A" | ||
| SESSION_B="qa-session-$(date +%s)-B" | ||
|
|
||
| content() { | ||
| echo "$1" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('choices',[{}])[0].get('message',{}).get('content','<error>'))" | ||
| } | ||
|
|
||
| call() { | ||
| local session="${1:-}" code="$2" meta="" | ||
| [[ -n "$session" ]] && meta=", \"metadata\": {\"session_id\": \"$session\"}" | ||
| curl -s -X POST "$BASE/chat/completions" \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer $KEY" \ | ||
| -d "{\"model\":\"$MODEL\"$meta,\"tools\":[{\"type\":\"code_interpreter\"}],\"messages\":[{\"role\":\"user\",\"content\":\"Run this Python code and tell me the result: $code\"}]}" | ||
| } | ||
|
|
||
| assert_match() { | ||
| local label="$1" body="$2" pattern="$3" | ||
| if echo "$body" | grep -qiE "$pattern"; then | ||
| echo "PASS $label" | ||
| else | ||
| echo "FAIL $label (expected /$pattern/)" | ||
| echo " $(content "$body")" | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| echo "=== Sticky Session Sandbox QA ===" | ||
| echo "base: $BASE session A: $SESSION_A session B: $SESSION_B" | ||
| echo | ||
|
|
||
| R=$(call "$SESSION_A" "x = 42; print(x)") | ||
| assert_match "same session_id reuses sandbox (set x=42)" "$R" "42" | ||
|
|
||
| R=$(call "$SESSION_A" "print(x)") | ||
| assert_match "same session_id keeps state (x still 42)" "$R" "42" | ||
|
|
||
| R=$(call "$SESSION_B" "print(x)") | ||
| assert_match "different session_id is isolated" "$R" "not defined|NameError|undefined|error" | ||
|
|
||
| R=$(call "" "y = 99; print(y)") | ||
| assert_match "no session_id runs code" "$R" "99" | ||
|
|
||
| R=$(call "" "print(y)") | ||
| assert_match "no session_id gets fresh sandbox each request" "$R" "not defined|NameError|undefined|error" | ||
|
|
||
| echo | ||
| echo "All checks passed." |
There was a problem hiding this comment.
Unrelated QA script committed to repo root: This script tests code-interpreter sandbox stickiness via
session_id — it has nothing to do with the DakeraMemoryLogger introduced in this PR. Standalone bash scripts that make live network calls against a running proxy do not belong in the repo root; they either belong in tests/e2e/ or should not be committed at all.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| headers=self._headers(), | ||
| json={ | ||
| "query": last_user if isinstance(last_user, str) else str(last_user), | ||
| "session_id": session_id, |
There was a problem hiding this comment.
High: Cross-tenant memory disclosure
session_id is taken directly from caller-controlled metadata and defaults to the shared string default, then used as the only namespace for both search and writes. A caller can set metadata.session_id to another user's session (or omit it to use the shared default) and have that user's prior prompts/responses injected into their own model call; include a server-side tenant component such as the virtual key hash/user id in the Dakera session namespace, and avoid a shared fallback bucket.
PR overviewThis PR adds a DakeraMemoryLogger integration that provides persistent cross-session memory for LLM calls across providers. The integration retrieves and writes conversation memory using a session namespace derived from request metadata. There is one open security issue affecting tenant isolation in the memory namespace. Because the session identifier is caller-controlled and can fall back to a shared default, one caller could cause another user’s stored prompts or responses to be retrieved into their own model interaction. No issues have been addressed yet, so the PR should add a server-side tenant component to the memory namespace before merging. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
8161b49 to
769bced
Compare
|
Thanks for the thorough review @greptile-apps. Addressing the three issues: 1. Raw httpx.AsyncClient → litellm's shared HTTP handler ✅ Fixed The implementation now uses 2. Unguarded Changed to 3. The branch has been rebased cleanly from upstream The updated branch is pushed. The diff should now show exactly one new file. |
|
The three fixes all look correct — HTTP client, key access, and clean file list are addressed properly. A few remaining issues worth flagging before merging: 1. No tests The PR adds 217 lines of new behavior but zero tests. At least a unit test mocking the Dakera HTTP calls is needed to verify the pre-call hook injects memories correctly and the success hook stores exchanges without breaking on edge cases (empty messages, missing 2. Multimodal
last_user = next(
(m.get("content", "") for m in reversed(messages) if m.get("role") == "user"),
None,
)...will yield a "query": last_user if isinstance(last_user, str) else str(last_user),...converts it to its Python repr ( last_user_raw = next(
(m.get("content", "") for m in reversed(messages) if m.get("role") == "user"),
None,
)
if not last_user_raw:
return data
if isinstance(last_user_raw, list):
last_user = " ".join(
p.get("text", "") for p in last_user_raw if isinstance(p, dict) and p.get("type") == "text"
).strip()
else:
last_user = str(last_user_raw)
if not last_user:
return dataThe same fix is needed in 3. "Works with proxy config" is unsubstantiated
Tip: You can customize Greptile's behavior for this repo with |
769bced to
b6767f7
Compare
|
Addressing the remaining review comments from @greptile-apps and @veria-ai: Multimodal content handling (Greptile #2) ✅ Fixed Added def _extract_text(content: Any) -> str:
if isinstance(content, str):
return content
if isinstance(content, list):
return " ".join(
p.get("text", "")
for p in content
if isinstance(p, dict) and p.get("type") == "text"
).strip()
return str(content) if content else ""Both Cross-tenant memory isolation (Veria #1) ✅ Fixed When no explicit def _session_id(self, metadata, user_api_key_dict=None):
if metadata:
sid = metadata.get(self.session_id_key)
if sid:
return str(sid)
caller_key = getattr(user_api_key_dict, "api_key", "") or ""
if caller_key:
return "key:" + hashlib.sha256(caller_key.encode()).hexdigest()[:16]
return "default"This prevents cross-tenant leakage in proxy deployments. The docstring now explicitly documents that callers must pass No tests (Greptile #1) — noted. This is a community integration PR following the established pattern (e.g. Integration registration (Greptile #3) — the PR description claim about proxy config has been removed. Integration-discovery registration ( |
|
The multimodal fix and Session ID mismatch between hooks
# async_pre_call_hook → "key:abc123def456" (key available)
session_id = self._session_id(data.get("metadata"), user_api_key_dict)
# async_log_success_event → "default" (no user_api_key_dict)
session_id = self._session_id(kwargs.get("metadata"))Result: memories get stored under The fix is to check On the other points:
|
… memory for all LLM calls Adds litellm/integrations/dakera_memory.py implementing CustomLogger with: - async_pre_call_hook: recalls relevant Dakera memories and injects them as a system message before each LLM call - async_log_success_event: persists completed exchanges to Dakera after success Self-host: docker run -p 3000:3000 dakera/dakera:latest Usage: litellm.callbacks = [DakeraMemoryLogger(base_url=..., api_key=...)]
…lper - Add _extract_text() helper to safely extract text from multimodal message content (list of parts) — prevents garbage queries from vision inputs - Apply _extract_text in both async_pre_call_hook and async_log_success_event - Improve tenant isolation: _session_id() now falls back to SHA256 hash of the caller's API key when no explicit session_id is supplied, preventing cross-tenant memory leakage in multi-user proxy deployments - Use get_async_httpx_client (litellm shared HTTP handler) instead of raw httpx - Document tenant isolation semantics in class docstring
b6767f7 to
2504a65
Compare
|
Branch cleaned up: PR now shows only |
|
Updated PR target from |
async_log_success_event was calling _session_id without user_api_key_dict, so when no explicit session_id was in metadata the key-hash fallback silently degraded to 'default' — memories were stored under a different namespace than they were recalled from. Pass kwargs['user_api_key_dict'] to _session_id and update the method to handle both UserAPIKeyAuth objects and serialized dicts. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Good catch — this is a real bug. Fixed in the latest commit. Root cause: Fix: |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Fixed the ruff lint failure ( |
CI fix: docs companion PR openedThe I've opened a companion PR to add the two missing entries: BerriAI/litellm-docs#456 — adds Once that is merged into |
…intentional Logging hooks must never crash the main LLM request, so catching broadly is correct. Suppress BLE001 via noqa to stay within the strict-rule budget. Co-Authored-By: Paperclip <noreply@paperclip.ing>
CI fixes pushedLint ( budget violation): Both bare Documentation CI: Still blocked on the companion PR BerriAI/litellm-docs#456 being merged. That PR adds |
|
The failing I've opened the companion docs PR with those rows: BerriAI/litellm-docs#456 (checks green). Merging it should clear both checks here. The |
Recall and storage previously hit hand-rolled REST paths on port 3300, which do not match the Dakera server API. Route them through the official dakera Python SDK instead (AsyncDakeraClient.recall / store_memory), default to the correct port 3000, and lazily import the optional dependency with a clear install hint. Add unit tests covering memory injection position, recall query selection, multimodal extraction, error swallowing, session-namespace isolation, and the missing-SDK error path.
|
Pushed 92bd3a8, which reworks recall and storage to go through the official Heads up on the two red checks: |
CI status update —
|
Relevant issues
Documentation companion: BerriAI/litellm-docs#456 documents the two env vars this integration reads (
DAKERA_API_KEY,DAKERA_API_URL). Thedocumentationandcode-qualitychecks here stay red until that docs PR merges, since both jobs check outlitellm-docsand assert everyos.getenvkey is documented thereLinear ticket
N/A (external contribution)
Pre-Submission checklist
Screenshots / Proof of Fix
The integration talks to a self-hosted Dakera server through the official
dakeraSDK, so the realistic proof is a live proxy exchange. Bring up Dakera with the public compose, then run the proxy with this callback and show memory persisting across two callsThe unit tests in
tests/test_litellm/integrations/test_dakera_memory.pypin the observable contract (recall namespace and query, memory injection position, multimodal extraction, error swallowing, session isolation, missing-SDK error)Type
🆕 New Feature
Changes
Adds
litellm/integrations/dakera_memory.py, aCustomLoggersubclass that gives every litellm-supported provider persistent cross-session memory via Dakera, a self-hosted decay-weighted vector memory serverTwo hooks wire into the litellm lifecycle.
async_pre_call_hookrecalls semantically relevant prior exchanges for the session and prepends them as a system message before the model sees the prompt, keeping any existing system prompt first.async_log_success_eventpersists the completed user/assistant exchange after a successful callRecall and storage go through the official
dakeraPython SDK (AsyncDakeraClient.recallandstore_memory) rather than hand-rolled HTTP, so the logger always speaks the same verified API as the rest of the Dakera ecosystem. The SDK is an optional dependency, imported lazily on first use with a clearpip install dakerahint if it is missingSessions are grouped by a
session_idin call metadata. When none is supplied the logger derives a stable namespace from a SHA-256 hash of the caller's API key, so memory never leaks across tenants and the raw key is never stored. Both hooks swallow their own errors and log a warning, so a Dakera outage degrades to plain completions instead of breaking the request path