-
Notifications
You must be signed in to change notification settings - Fork 21
feat: per-user long-term memory and conversation summarization (#41) #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7f72433
45e31e7
c50c2e1
27cdaa8
436e02b
46951d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| from fastapi import FastAPI, HTTPException, Request, Response | ||
| from fastapi.middleware.cors import CORSMiddleware | ||
| from fastapi.responses import StreamingResponse | ||
| from pydantic import BaseModel | ||
| from pydantic import BaseModel, Field | ||
| import google.generativeai as genai | ||
| import time | ||
|
|
||
|
|
@@ -62,6 +62,15 @@ | |
| from review import enqueue_for_review, router as review_router | ||
| from review_store import get_review_store | ||
|
|
||
| from memory import ChatSummary, UserProfile, create_memory_store, render_user_context | ||
| from memory.extraction import ( | ||
| MEMORY_EXTRACTION_ENABLED, | ||
| apply_updates, | ||
| extract_updates, | ||
| merge_summaries, | ||
| summarize_conversation_turns, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Load environment variables | ||
|
|
@@ -115,6 +124,8 @@ class ChatRequest(BaseModel): | |
| context: Optional[str] = None # Additional context for specific queries | ||
| madhhab: Optional[str] = None # User's madhhab: hanafi, maliki, shafii, hanbali | ||
| language: Optional[str] = None # BCP-47 response language (ar, en, ur, etc.); auto-detect when omitted | ||
| user_id: Optional[str] = Field(default=None, max_length=128) # Opaque user identifier for personalization | ||
| remember: bool = True # When False, existing memory is read but no new data persisted | ||
|
|
||
|
|
||
| class Message(BaseModel): | ||
|
|
@@ -183,6 +194,11 @@ def classify_for_safety(prompt: str, candidate_ids: List[str]): | |
| # Durable queue for low-confidence religious answers awaiting a scholar | ||
| review_store = get_review_store() | ||
|
|
||
| # Per-user memory store (Redis-backed or in-memory) | ||
| memory_store = create_memory_store() | ||
|
|
||
| MAX_CHAT_HISTORY_TURNS = 20 | ||
|
|
||
| # Tafsir retrieval seam: returns None for prompts that are not | ||
| # verse-explanation questions. Offline tests replace this with a stub. | ||
| DEFAULT_TAFSIR_LANGUAGE = "en" | ||
|
|
@@ -466,6 +482,13 @@ def _finalize() -> None: | |
| zakat_context = await zakat_retriever(request.prompt, request.context) | ||
| zakat_info = zakat_context.info if zakat_context else None | ||
|
|
||
| # --- Memory lookup --- | ||
| profile: Optional[UserProfile] = None | ||
| summary: Optional[ChatSummary] = None | ||
| if request.user_id: | ||
| profile = await memory_store.get_profile(request.user_id) | ||
| summary = await memory_store.get_chat_summary(f"{request.user_id}:{chat_id}") | ||
|
|
||
| # Neither a tafsir-grounded answer nor a zakat answer goes through the | ||
| # semantic response cache: the first is built from retrieved passages | ||
| # (already cached by ayah key), and the second contains one user's real | ||
|
|
@@ -475,6 +498,7 @@ def _finalize() -> None: | |
| and request.context is None | ||
| and tafsir_context is None | ||
| and zakat_context is None | ||
| and request.user_id is None | ||
| and SEMANTIC_CACHE_ENABLED | ||
| ) | ||
|
|
||
|
|
@@ -526,6 +550,9 @@ async def generate(safety_prompt: str) -> str: | |
| system_context += tafsir_system_context(tafsir_context) | ||
| if zakat_context is not None: | ||
| system_context += zakat_context.prompt_block | ||
| memory_block = render_user_context(profile, summary) | ||
| if memory_block: | ||
| system_context += f"\n\n{memory_block}" | ||
|
Comment on lines
+553
to
+555
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Declare persisted memory untrusted before injecting it into the prompt. Delimited headings alone do not prevent remembered user-controlled text from being interpreted as instructions on later turns. Add a system instruction immediately before this block stating that profile and summary contents are reference data only and that instructions inside them must be ignored. 🤖 Prompt for AI Agents |
||
| context = f"Additional context: {extra_context}\n\n" if extra_context else "" | ||
| full_prompt = f"{system_context}\n{context}User question: {safety_prompt}" | ||
| logger.info("Sending message to chat...") | ||
|
|
@@ -682,6 +709,33 @@ async def generate(safety_prompt: str) -> str: | |
| ) | ||
| _finalize() | ||
| _succeeded = True | ||
|
|
||
| # --- Background memory extraction and summarization --- | ||
| # Runs as fire-and-forget tasks after the response is sent. | ||
| if request.user_id and request.remember and MEMORY_EXTRACTION_ENABLED: | ||
| asyncio.create_task( | ||
| _extract_and_update_memory( | ||
| request.user_id, prompt, response_text, chat_id, summary, memory_store, | ||
| ) | ||
| ) | ||
| logger.info("Memory extraction scheduled for user %s", request.user_id[:8]) | ||
|
|
||
| # --- Summary eviction --- | ||
| # After enough turns accumulate, summarize old history and persist. | ||
| if request.user_id and request.remember and MEMORY_EXTRACTION_ENABLED: | ||
| chat_session = active_chats.get(chat_id) | ||
| if chat_session and hasattr(chat_session, "history") and chat_session.history: | ||
| if len(chat_session.history) >= MAX_CHAT_HISTORY_TURNS: | ||
| asyncio.create_task( | ||
| _summarize_history( | ||
| f"{request.user_id}:{chat_id}", | ||
| chat_session.history, | ||
| summary, | ||
| memory_store, | ||
| ) | ||
| ) | ||
|
Comment on lines
+716
to
+736
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Make profile and summary updates atomic per user/chat. Concurrent requests schedule independent read-modify-write tasks. Each can load the same old profile or summary and save a different version later, losing facts or overwriting merged summaries. Add store-level atomic update/CAS operations or keyed locks, and re-read state inside the protected update. 🧰 Tools🪛 Ruff (0.15.21)[warning] 716-720: Store a reference to the return value of (RUF006) [warning] 729-736: Store a reference to the return value of (RUF006) 🤖 Prompt for AI Agents |
||
| logger.info("History summarization triggered for %s", request.user_id[:8]) | ||
|
|
||
| return response_obj | ||
|
|
||
| except ResourceExhausted as exc: | ||
|
|
@@ -734,6 +788,47 @@ async def generate(safety_prompt: str) -> str: | |
| telemetry.current_trace.reset(_ctx_token) | ||
|
|
||
|
|
||
| async def _extract_and_update_memory( | ||
| user_id: str, prompt: str, response: str, chat_id: str, | ||
| existing_summary: Optional[ChatSummary], | ||
| store: Any, | ||
| ) -> None: | ||
| """Fire-and-forget memory extraction. Runs via asyncio.create_task.""" | ||
| try: | ||
| updates = await extract_updates(prompt, response) | ||
| if updates.get("none"): | ||
| return | ||
| profile = await store.get_profile(user_id) | ||
| if profile is None: | ||
| profile = UserProfile(user_id=user_id) | ||
| profile = apply_updates(profile, updates) | ||
| await store.save_profile(user_id, profile) | ||
| logger.debug("Memory updated for user %s", user_id[:8]) | ||
| except Exception: | ||
| logger.warning("Memory extraction failed for user %s", user_id[:8], exc_info=True) | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| async def _summarize_history( | ||
| chat_id: str, history: list, existing_summary: Optional[ChatSummary], | ||
| store: Any, | ||
| ) -> None: | ||
| """Summarize accumulated conversation turns and persist.""" | ||
| try: | ||
| turns = [ | ||
| {"role": m.role, "text": m.parts[0].text if m.parts else ""} | ||
| for m in history | ||
| ] | ||
| new_summary_text = await summarize_conversation_turns(turns) | ||
| if existing_summary: | ||
| merged = await merge_summaries(existing_summary.content, new_summary_text) | ||
| else: | ||
| merged = new_summary_text | ||
| summary = ChatSummary(chat_id=chat_id, content=merged, turn_count=len(history)) | ||
| await store.save_chat_summary(chat_id, summary) | ||
|
Comment on lines
+817
to
+827
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Summarize only history that is not already represented.
🤖 Prompt for AI Agents |
||
| except Exception: | ||
| logger.warning("History summarization failed for %s", chat_id[:8], exc_info=True) | ||
|
|
||
|
|
||
| @app.post("/chat/stream") | ||
| async def chat_stream(request: ChatRequest, http_request: Request): | ||
| """Streaming chat endpoint using Server-Sent Events (SSE).""" | ||
|
|
@@ -847,9 +942,8 @@ async def event_generator(): | |
| ) | ||
|
|
||
| except Exception as e: | ||
| error_msg = f"❌ Streaming Chat API Error: {str(e)}" | ||
| logger.error(error_msg) | ||
| raise HTTPException(status_code=500, detail=error_msg) | ||
| logger.error("Streaming Chat API Error", exc_info=True) | ||
| raise HTTPException(status_code=500, detail="Internal server error") from e | ||
|
|
||
|
|
||
| @app.delete("/chat/{chat_id}") | ||
|
|
@@ -861,9 +955,8 @@ async def delete_chat(chat_id: str): | |
| return {"message": "Chat session deleted successfully"} | ||
| return {"message": "Chat session not found"} | ||
| except Exception as e: | ||
| error_msg = f"❌ Error deleting chat: {str(e)}" | ||
| logger.error(error_msg) | ||
| raise HTTPException(status_code=500, detail=error_msg) | ||
| logger.error("Error deleting chat", exc_info=True) | ||
| raise HTTPException(status_code=500, detail="Internal server error") from e | ||
|
|
||
|
|
||
| @app.get("/ping") | ||
|
|
@@ -872,6 +965,33 @@ async def ping(): | |
| return {"status": "ok"} | ||
|
|
||
|
|
||
| @app.get("/memory/{user_id}") | ||
| async def get_memory(user_id: str): | ||
| """Retrieve the stored user profile for transparency. | ||
|
|
||
| TODO(#9): bind to authenticated principal — anyone who knows a user_id | ||
| can currently read another user's memory. | ||
| """ | ||
| profile = await memory_store.get_profile(user_id) | ||
| if profile is None: | ||
| raise HTTPException(status_code=404, detail="Memory not found") | ||
| return profile.model_dump() | ||
|
|
||
|
|
||
| @app.delete("/memory/{user_id}") | ||
| async def delete_memory(user_id: str): | ||
| """Completely erase the stored user profile. | ||
|
|
||
| TODO(#9): bind to authenticated principal — anyone who knows a user_id | ||
| can currently erase another user's memory. | ||
| """ | ||
| existed = await memory_store.delete_profile(user_id) | ||
| if existed: | ||
| logger.info("Deleted memory for user %s", user_id[:8]) | ||
| return {"message": "Memory deleted successfully"} | ||
| return {"message": "Memory not found"} | ||
|
Comment on lines
+968
to
+992
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Map the relevant file and locate auth/security-related code paths.
git ls-files | rg '(^|/)main\.py$|(^|/)(auth|security|middleware|dependencies|users|session).*\.py$|requirements\.txt$'
printf '\n--- main.py outline ---\n'
ast-grep outline main.py --view expanded || true
printf '\n--- security/auth references in main.py ---\n'
rg -n "Depends\\(|Security\\(|OAuth|Bearer|JWT|session|auth|user_id|HTTPException|middleware|add_middleware|include_router" main.py || true
printf '\n--- memory endpoint context around lines 430-490 ---\n'
sed -n '430,490p' main.py
printf '\n--- request model context around ChatRequest ---\n'
sed -n '100,150p' main.pyRepository: Deen-Bridge/dnb-ai Length of output: 8786 Protect
🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| @app.get("/cache/stats") | ||
| async def cache_stats(): | ||
| return semantic_cache.get_stats() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| """Per-user long-term memory and conversation summarization. | ||
|
|
||
| See README.md for usage; see tests/ for offline-verifiable contracts. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import os | ||
| from typing import Optional | ||
|
|
||
| from memory.models import ChatSummary, UserProfile | ||
| from memory.store import ( | ||
| InMemoryMemoryStore, | ||
| MemoryStore, | ||
| RedisMemoryStore, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def create_memory_store() -> MemoryStore: | ||
| """Factory: ``REDIS_URL`` set → ``RedisMemoryStore``, else in-memory. | ||
|
|
||
| When Redis is configured but fails at startup the error is surfaced | ||
| (logged + raised) — workers must not silently diverge on user memory. | ||
| """ | ||
| url = os.getenv("REDIS_URL", "") | ||
| if url: | ||
| logger.info("MemoryStore using Redis at %s", url) | ||
| return RedisMemoryStore(url) | ||
| logger.info("MemoryStore using in-memory dict (local development)") | ||
| return InMemoryMemoryStore() | ||
|
|
||
|
|
||
| def render_user_context( | ||
| profile: Optional[UserProfile], | ||
| summary: Optional[ChatSummary], | ||
| ) -> str: | ||
| """Render profile and chat summary as a delimited DATA block. | ||
|
|
||
| Returns an empty string when neither has content so anonymous traffic | ||
| is completely unaffected. | ||
| """ | ||
| parts: list[str] = [] | ||
|
|
||
| if profile is not None and ( | ||
| profile.knowledge_level | ||
| or profile.madhhab | ||
| or profile.preferred_language | ||
| or profile.topics_studied | ||
| or profile.remembered_facts | ||
| ): | ||
| lines = ["--- Known about this student ---"] | ||
| if profile.knowledge_level: | ||
| lines.append(f"Knowledge level: {profile.knowledge_level}") | ||
| if profile.madhhab: | ||
| lines.append(f"Madhhab: {profile.madhhab}") | ||
| if profile.preferred_language: | ||
| lines.append(f"Preferred language: {profile.preferred_language}") | ||
| if profile.topics_studied: | ||
| topics_str = ", ".join( | ||
| f"{t.topic}" for t in profile.topics_studied[-10:] | ||
| ) | ||
| lines.append(f"Topics studied: {topics_str}") | ||
| if profile.remembered_facts: | ||
| for fact in profile.remembered_facts[-5:]: | ||
| lines.append(f"- {fact.fact}") | ||
| parts.append("\n".join(lines)) | ||
|
|
||
| if summary is not None and summary.content: | ||
| parts.append(f"--- Conversation summary ---\n{summary.content}") | ||
|
|
||
| if not parts: | ||
| return "" | ||
|
|
||
| return "\n\n".join(parts) + "\n---------------------------------\n" | ||
|
|
||
|
|
||
| __all__ = [ | ||
| "ChatSummary", | ||
| "InMemoryMemoryStore", | ||
| "MemoryStore", | ||
| "RedisMemoryStore", | ||
| "UserProfile", | ||
| "create_memory_store", | ||
| "render_user_context", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 236
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 392
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 7064
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 5002
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 31538
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 5353
🏁 Script executed:
Repository: Deen-Bridge/dnb-ai
Length of output: 3462
Protect
/memory/{user_id}with authenticated ownership.main.py:465-480reads and deletes profiles directly from the pathuser_id, so anyone who can guess an ID can access or erase that user’s memory. Keep the README’s privacy-control wording only once those routes are scoped to the authenticated owner.🤖 Prompt for AI Agents