Long term memory with letta #547
Conversation
WalkthroughThis PR introduces a comprehensive memory system that leverages Letta to manage user and project-level memories. It adds database support for vector embeddings via pgvector, integrates memory extraction into the agent task pipeline, exposes memory operations through new API endpoints, equips all system agents with a search_user_memories tool, and updates infrastructure to run Letta alongside the main application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Agent as Agent Execution
participant Conv as Conversation Service
participant Memory as Memory Service
participant Letta as Letta Server
participant LLM
User->>Agent: Request
activate Agent
Agent->>Agent: Process & Generate Response
Agent->>Conv: Store Response
deactivate Agent
rect rgb(220, 240, 255)
Note over Agent,LLM: Memory Extraction (Async)
Agent->>Agent: Queue extract_user_preferences task
activate Agent
Agent->>Memory: create via Factory
Memory->>Memory: _extract_facts_with_llm(recent_messages)
Memory->>LLM: Extract facts
LLM-->>Memory: Facts
Memory->>Memory: _categorize_memory(user/project, episodic/semantic)
Memory->>Letta: Add passages to agent
Letta-->>Memory: Stored
Memory-->>Agent: Completion
deactivate Agent
end
User->>Agent: New Request with Memory Context
activate Agent
Agent->>Agent: Plan with search_user_memories tool
Agent->>Memory: search(query, user_id, project_id)
activate Memory
Memory->>Letta: Search across user/project agents
Letta-->>Memory: Results with metadata
Memory-->>Agent: MemorySearchResponse
deactivate Memory
Agent->>Agent: Use retrieved memories for context
Agent->>LLM: Generate with memory context
LLM-->>Agent: Response
Agent-->>User: Personalized Response
deactivate Agent
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (15)
app/modules/code_provider/code_provider_service.py (1)
47-54: Consider adding a public method to the provider interface instead of accessing_get_repo.Accessing the private
_get_repomethod couplesMockRepoto provider implementation details that may change without notice. Consider adding aget_repo_object(repo_name)method to theICodeProviderinterface that each provider implements, abstracting the name normalization logic (e.g., GitBucket's actual repo name lookup).This would make the interface contract explicit and avoid future breakage if providers refactor their internals.
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (2)
21-21: Duplicate import ofOptional.
Optionalis already imported fromtypingat line 4. This import is redundant.🔎 Proposed fix
-from typing import OptionalMove it to line 4 if needed, or simply remove since it's already there:
from typing import List, AsyncGenerator, Sequence, Optional
70-70: Consider using a Protocol or TYPE_CHECKING formemory_managertype.Using
Optional[object]loses all type information. Consider usingTYPE_CHECKINGto provide a proper type hint without causing circular imports at runtime.🔎 Proposed improvement
from typing import TYPE_CHECKING if TYPE_CHECKING: from app.modules.intelligence.memory.memory_manager import MemoryManager # Then in __init__: memory_manager: Optional["MemoryManager"] = NoneAlso applies to: 85-86
app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py (1)
115-132: Consistent memory_manager propagation - same verification applies.The
memory_managerpropagation follows the same pattern as other agents. The inline comments (# NEW: Pass memory_manager) are helpful during review but consider removing them before merging to keep the codebase clean.🔎 Optional: Remove inline comments post-merge
return PydanticMultiAgent( self.llm_provider, agent_config, tools, None, delegate_agents, - memory_manager=getattr(self, 'memory_manager', None) # NEW: Pass memory_manager + memory_manager=getattr(self, 'memory_manager', None) ) else: logger.info("❌ Multi-agent disabled by config, using PydanticRagAgent") return PydanticRagAgent( self.llm_provider, agent_config, tools, - memory_manager=getattr(self, 'memory_manager', None) # NEW: Pass memory_manager + memory_manager=getattr(self, 'memory_manager', None) ) else: logger.error( f"❌ Model '{self.llm_provider.chat_config.model}' does not support Pydantic - using fallback PydanticRagAgent" ) return PydanticRagAgent( self.llm_provider, agent_config, tools, - memory_manager=getattr(self, 'memory_manager', None) # NEW: Pass memory_manager + memory_manager=getattr(self, 'memory_manager', None) )pyproject.toml (1)
23-23: Clarify the relationship between chromadb and pgvector.The comment states chromadb was "replaced by pgvector," but chromadb is a vector database while pgvector is a PostgreSQL extension. If you're using pgvector for embeddings storage via Letta, consider clarifying that the system now uses PostgreSQL with pgvector instead of a standalone chromadb instance.
app/celery/celery_app.py (1)
146-146: Consider removing the blanketnoqadirective.The static analysis tool indicates the
# noqacomment may be unnecessary. If this import doesn't trigger any linting warnings, remove the directive to keep the codebase clean.🔎 Proposed fix
-import app.celery.tasks.memory_tasks # noqa # Ensure memory tasks are registered +import app.celery.tasks.memory_tasks # Ensure memory tasks are registeredBased on static analysis hints.
GETTING_STARTED.md (1)
86-86: Format the bare URL as a Markdown link.Per Markdown best practices, the bare URL should be wrapped in a proper link format.
🔎 Proposed fix
-The Letta web UI is available at http://localhost:8283 for viewing agents and memories. +The Letta web UI is available at [http://localhost:8283](http://localhost:8283) for viewing agents and memories.app/celery/tasks/agent_tasks.py (1)
178-238: Good defensive implementation of post-completion memory extraction.The fire-and-forget pattern with
.delay()and the try/except wrapper ensure that memory extraction failures don't impact the main agent task. The lazy imports are appropriate here.One consideration: the
isinstance(msg, HumanMessage)check on line 201 only coversHumanMessage. For messages that aren'tHumanMessage, they're assumed to be "assistant" - this works but you might want to explicitly import and check forAIMessageas well for clarity.🔎 Optional: More explicit message type handling
from app.celery.tasks.memory_tasks import extract_user_preferences from app.modules.intelligence.memory.chat_history_service import ChatHistoryService -from langchain_core.messages import HumanMessage +from langchain_core.messages import HumanMessage, AIMessage # ... recent_messages = [ { - "role": "user" if isinstance(msg, HumanMessage) else "assistant", + "role": "user" if isinstance(msg, HumanMessage) else ("assistant" if isinstance(msg, AIMessage) else "system"), "content": msg.content } for msg in recent_history[-2:] if hasattr(msg, 'content') ]app/modules/intelligence/memory/memory_service_factory.py (1)
32-41: Unused variable inget_default_llm_config.The
openai_api_keyvariable is fetched but not included in the returned config dict. This appears intentional (the key is used by litellm/Letta directly via environment), but consider removing the variable assignment and just checking the environment directly in the warning condition for clarity.🔎 Proposed simplification
@staticmethod def get_default_llm_config() -> dict: - openai_api_key = os.getenv("OPENAI_API_KEY") - if not openai_api_key: + if not os.getenv("OPENAI_API_KEY"): logger.warning("OPENAI_API_KEY not found in environment - Letta may not work") return { "model": os.getenv("LETTA_MODEL", "openai/gpt-4o-mini"), "embedding": os.getenv("LETTA_EMBEDDING", "openai/text-embedding-ada-002"), }app/modules/intelligence/memory/memory_router.py (2)
49-54: Use exception chaining for better debugging.When re-raising exceptions, chain them with
from eto preserve the original traceback.🔎 Proposed fix
except Exception as e: logger.error(f"Error retrieving memories: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Failed to retrieve memories: {str(e)}" - ) + detail=f"Failed to retrieve memories: {e!s}" + ) from e
107-112: Use exception chaining here as well.🔎 Proposed fix
except Exception as e: logger.error(f"Error deleting memories: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Failed to delete memories: {str(e)}" - ) + detail=f"Failed to delete memories: {e!s}" + ) from eapp/celery/tasks/memory_tasks.py (1)
48-52: Consider using a proper timestamp forextracted_at.Currently,
extracted_atis set to the Celery task'srequest_id, which is a UUID, not a timestamp. This is semantically confusing. Consider using an actual timestamp.🔎 Proposed fix
+from datetime import datetime, timezone + # ... inside run_extraction: result = await memory_service.add( messages=messages, user_id=user_id, project_id=project_id, metadata={ **(metadata or {}), "conversation_id": conversation_id, - "extracted_at": str(request_id), + "extracted_at": datetime.now(timezone.utc).isoformat(), + "task_id": str(request_id), }, )app/modules/intelligence/memory/letta_service.py (3)
106-112: Useasyncio.get_running_loop()instead of deprecatedasyncio.get_event_loop().
asyncio.get_event_loop()is deprecated in Python 3.10+ when there's no running loop. Since this is called within an async context, useasyncio.get_running_loop()instead.🔎 Proposed fix
- import asyncio - loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop()Apply this pattern to all occurrences in the file (lines 106-107, 184-185, 248-249, 382-383, 436-437, 490-491).
243-243: Remove extraneous f-string prefix.The f-string has no placeholders.
🔎 Proposed fix
- logger.info(f"[letta add] No facts extracted from message") + logger.info("[letta add] No facts extracted from message")
145-180: Keyword-based memory categorization may be fragile.The
_categorize_memorymethod uses keyword matching which could misclassify memories. For example, "I use PostgreSQL at home" would be classified as project-scoped due to "postgresql" keyword, even though it's a personal preference.Consider this a known limitation for now, but you might want to leverage the LLM for more accurate categorization in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
GETTING_STARTED.mdapp/alembic/versions/20251222_enable_pgvector_extension.pyapp/celery/celery_app.pyapp/celery/tasks/agent_tasks.pyapp/celery/tasks/memory_tasks.pyapp/celery/worker.pyapp/main.pyapp/modules/code_provider/code_provider_service.pyapp/modules/conversations/conversation/conversation_service.pyapp/modules/intelligence/agents/agents_service.pyapp/modules/intelligence/agents/chat_agent.pyapp/modules/intelligence/agents/chat_agents/pydantic_agent.pyapp/modules/intelligence/agents/chat_agents/pydantic_multi_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/debug_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/qna_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.pyapp/modules/intelligence/agents/chat_agents/tool_helpers.pyapp/modules/intelligence/memory/letta_service.pyapp/modules/intelligence/memory/memory_interface.pyapp/modules/intelligence/memory/memory_router.pyapp/modules/intelligence/memory/memory_service_factory.pyapp/modules/intelligence/tools/memory_search_tool.pyapp/modules/intelligence/tools/tool_service.pydocker-compose.yamlpyproject.toml
🧰 Additional context used
🧬 Code graph analysis (17)
app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/celery/tasks/memory_tasks.py (5)
app/celery/tasks/base_task.py (2)
BaseTask(10-103)run_async(72-77)app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-41)create(14-30)app/modules/utils/logger.py (2)
log_context(302-314)setup_logger(317-331)app/modules/intelligence/memory/letta_service.py (2)
add(217-301)close(544-546)app/modules/intelligence/memory/memory_interface.py (2)
add(36-44)close(67-69)
app/modules/intelligence/agents/chat_agents/system_agents/debug_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/modules/intelligence/memory/memory_service_factory.py (2)
app/modules/intelligence/memory/memory_interface.py (1)
MemoryInterface(19-69)app/modules/intelligence/memory/letta_service.py (1)
LettaService(17-546)
app/modules/intelligence/tools/tool_service.py (2)
app/modules/intelligence/tools/memory_search_tool.py (1)
search_user_memories_tool(205-247)app/celery/tasks/base_task.py (1)
db(15-18)
app/modules/intelligence/memory/memory_interface.py (1)
app/modules/intelligence/memory/letta_service.py (5)
search(303-371)add(217-301)get_all_for_user(423-479)delete(481-542)close(544-546)
app/modules/code_provider/code_provider_service.py (4)
app/modules/code_provider/gitbucket/gitbucket_provider.py (1)
_get_repo(99-117)app/modules/code_provider/local_repo/local_provider.py (1)
_get_repo(49-63)app/modules/code_provider/github/github_service.py (1)
get_repo(624-678)app/modules/code_provider/local_repo/local_repo_service.py (1)
get_repo(46-51)
app/modules/intelligence/memory/letta_service.py (2)
app/modules/intelligence/memory/memory_interface.py (8)
MemoryInterface(19-69)MemorySearchResponse(13-16)MemorySearchResult(6-10)add(36-44)search(23-33)get_all_for_user(47-54)delete(57-64)close(67-69)app/modules/intelligence/memory/memory_service_factory.py (1)
create(14-30)
app/modules/intelligence/tools/memory_search_tool.py (3)
app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-41)create(14-30)app/modules/intelligence/memory/letta_service.py (1)
search(303-371)app/modules/intelligence/memory/memory_interface.py (1)
search(23-33)
app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1010)
app/celery/worker.py (1)
app/celery/tasks/memory_tasks.py (1)
extract_user_preferences(16-92)
app/modules/intelligence/agents/agents_service.py (7)
app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py (1)
QnAAgent(21-160)app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py (1)
UnitTestAgent(14-77)app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py (1)
IntegrationTestAgent(21-138)app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py (1)
BlastRadiusAgent(14-67)app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py (1)
CodeGenAgent(23-162)app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (1)
GeneralPurposeAgent(23-137)app/modules/intelligence/agents/chat_agent.py (1)
AgentWithInfo(106-111)
🪛 markdownlint-cli2 (0.18.1)
GETTING_STARTED.md
86-86: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.10)
app/celery/celery_app.py
146-146: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
app/celery/tasks/agent_tasks.py
233-233: Do not catch blind exception: Exception
(BLE001)
app/modules/intelligence/memory/memory_router.py
51-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
53-53: Use explicit conversion flag
Replace with conversion flag
(RUF010)
61-61: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
62-62: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
77-80: Abstract raise to an inner function
(TRY301)
109-112: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
111-111: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/code_provider/code_provider_service.py
59-59: Do not catch blind exception: Exception
(BLE001)
app/modules/intelligence/memory/letta_service.py
139-139: Consider moving this statement to an else block
(TRY300)
209-209: Do not catch blind exception: Exception
(BLE001)
243-243: f-string without any placeholders
Remove extraneous f prefix
(F541)
295-297: Consider moving this statement to an else block
(TRY300)
417-417: Consider moving this statement to an else block
(TRY300)
507-507: Do not catch blind exception: Exception
(BLE001)
534-534: Do not catch blind exception: Exception
(BLE001)
app/modules/intelligence/tools/memory_search_tool.py
159-159: Use explicit conversion flag
Replace with conversion flag
(RUF010)
184-184: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (32)
app/modules/code_provider/code_provider_service.py (2)
59-66: Exception handling is appropriate for graceful degradation.The static analysis flags the broad
Exceptioncatch, but this is acceptable here.get_languages()provides supplementary data, and failing gracefully with an empty dict is preferable to propagating unexpected errors that could break callers. The warning-level logging with full context (repo name, provider class, error) provides sufficient observability.
56-58: LGTM!The
or {}guards againstNonereturns, and lowercase normalization ensures consistent language key comparisons across different provider implementations. Thestr(lang)cast is a good defensive measure.app/modules/intelligence/agents/chat_agents/tool_helpers.py (3)
131-132: LGTM!The tool run message is consistent with the existing pattern and provides clear user feedback.
247-248: LGTM!The tool response message follows the established pattern and provides appropriate completion feedback.
704-708: LGTM!The result content handling is consistent with similar tools (todo management, code changes) and appropriately passes through string content.
app/modules/intelligence/agents/chat_agent.py (1)
54-54: LGTM! Integration with memory operations verified.The addition of the optional
user_idfield is backward compatible and properly typed. Both ChatContext instantiation sites inconversation_service.py(lines 699 and 731) populateuser_idfrom the request context, ensuring the field is available for memory operations.The Letta memory integration properly consumes
user_idacross all memory operations (add(),search(),get_all(),delete()), using it to scope memories at both user and project levels. Memory service calls inmemory_tasks.pyandmemory_search_tool.pycorrectly pass the user ID for memory operations.No privacy or compliance concerns detected—user IDs are appropriately scoped to memory operations and not over-logged.
app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py (1)
125-142: Memory manager propagation looks correct.The
memory_manageris consistently passed to bothPydanticMultiAgentandPydanticRagAgentacross all code paths usinggetattr(self, 'memory_manager', None). This pattern allows for optional memory integration when amemory_managerattribute is set on theCodeGenAgentinstance externally, without breaking existing functionality.app/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py (2)
289-300: Memory manager integration looks good.The
memory_managerparameter follows the same pattern asPydanticRagAgent, and the_preferences_blockcache is properly initialized for supervisor prompts.
818-819: Preferences block correctly injected into supervisor instructions.The
_preferences_blockis properly included in the supervisor agent's instructions, allowing memory-derived preferences to influence agent behavior when populated.app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py (1)
110-127: Memory manager propagation is consistent.The
memory_manageris correctly propagated to all agent construction paths following the established pattern.app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py (1)
55-58: Memory manager integration is correct.The
memory_managerpropagation follows the established pattern for optional memory integration.app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py (1)
63-66: Memory manager integration is correct.Consistent with the pattern used across other system agents.
app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py (1)
54-57: Memory manager integration is correct.The implementation is consistent with the established pattern across other system agents.
app/modules/intelligence/agents/agents_service.py (2)
72-80: Good refactoring to pre-instantiate agents.Pre-creating agent instances enables efficient memory manager injection and avoids redundant object creation. This pattern works well with the memory management integration since the same agent instance can be configured with a
memory_manageronce and reused.
82-131: Agent reuse and state isolation are properly implemented.The pre-instantiated agents are correctly referenced in the
AgentWithInfomappings. State isolation between calls is properly handled:_current_contextis initialized at the start of eachrun()call (line 1235) and eachrun_stream()call (line 1353), with additional per-request state managers (todo_manager, code_changes_manager) explicitly reset to ensure isolation between requests.app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py (1)
52-86: Consider addingsearch_user_memoriestool for consistency.
GeneralPurposeAgentincludes thesearch_user_memoriestool, butQnAAgentdoes not. If long-term memory retrieval would benefit Q&A responses (e.g., recalling user preferences or past context), consider adding it here as well.Is this intentional, or should
QnAAgentalso have access to user memories?app/modules/intelligence/agents/chat_agents/system_agents/debug_agent.py (1)
112-129: Memory manager propagation looks correct.The
memory_manageris consistently wired through all three code paths (multi-agent, single-agent, and fallback). Thegetattr(self, 'memory_manager', None)pattern provides a safe default when the attribute isn't set.app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (2)
78-78: Thesearch_user_memoriestool is already properly registered inToolServiceand will be available at runtime with no issues.
111-128: Verify thatmemory_manageris initialized onGeneralPurposeAgentinstances.The code uses
getattr(self, 'memory_manager', None)to safely accessmemory_manager, butGeneralPurposeAgent.__init__does not initialize this attribute, and no external code sets it after instantiation. This means the pattern will always returnNone, making thememory_managerparameter passed toPydanticMultiAgentandPydanticRagAgentalwaysNone. Either initializememory_managerin__init__, accept it as a constructor parameter, or document why it is intentionally omitted.app/modules/intelligence/memory/memory_interface.py (1)
1-70: LGTM! Well-designed memory interface.The memory interface is clean, with proper type hints and a clear separation of concerns. The abstract methods cover the core memory operations (search, add, get_all, delete, close), and the Pydantic models provide good structure for responses.
app/main.py (1)
19-19: LGTM! Memory router integration follows existing patterns.The memory router is properly imported and included with the standard
/api/v1prefix and appropriate tagging, consistent with other API routers in the application.Also applies to: 150-150
app/celery/celery_app.py (1)
100-102: LGTM! Memory task routing is properly configured.The
extract_user_preferencestask is correctly routed to theagent_tasksqueue, which makes sense given that the worker is already listening to this queue.app/celery/worker.py (1)
14-16: LGTM! Memory task registration follows established patterns.The
extract_user_preferencestask is properly imported and registered with the Celery app, consistent with the existing task registration approach.Also applies to: 33-36
app/modules/conversations/conversation/conversation_service.py (1)
706-706: LGTM! Adding user_id to ChatContext enables memory integration.The
user_idparameter is now properly passed toChatContextfor both custom agents and standard agents, enabling the memory system to associate memories with the correct user.Also applies to: 738-738
app/modules/intelligence/tools/tool_service.py (1)
97-97: LGTM! Memory search tool is properly integrated.The
search_user_memoriestool is correctly imported and registered in the tool dictionary, following the established pattern for tool initialization with database and user_id context.Also applies to: 195-195
app/alembic/versions/20251222_enable_pgvector_extension.py (1)
23-26: CASCADE is safe and appropriate here.The search shows no columns in the codebase actually use pgvector's
vectortype. Embeddings are stored asARRAY(Float)in the inference cache table and Neo4j handles its own vector indexes separately. Since no schema objects depend on the pgvector extension, theCASCADEclause will not drop any dependent objects, and the downgrade should succeed without issues or manual cleanup.docker-compose.yaml (1)
3-3: Good choice using pgvector for vector search capabilities.The switch to
pgvector/pgvector:pg16aligns well with the memory system's semantic search requirements.GETTING_STARTED.md (1)
68-95: Well-documented Letta Memory System section.The documentation clearly explains the memory system capabilities, verification steps, and configuration options. This will help users understand and troubleshoot the integration.
app/modules/intelligence/memory/memory_service_factory.py (1)
10-30: Factory implementation is clean and follows best practices.The static factory method with optional config and sensible defaults is well-designed. The logging provides good observability.
app/celery/tasks/memory_tasks.py (1)
11-92: Overall task implementation is solid.The task properly uses
log_contextfor structured logging, handles async operations viaBaseTask.run_async, and ensures cleanup infinallyblocks. Good defensive programming.app/modules/intelligence/memory/letta_service.py (1)
17-31: LettaService implementation looks solid overall.Good use of agent caching, proper async/sync bridging with executors, and comprehensive error handling. The interface implementation is complete and follows the contract defined in
MemoryInterface.app/modules/intelligence/tools/memory_search_tool.py (1)
115-119: Potential runtime error:asyncio.run()cannot be called from within an existing event loop.
asyncio.run()will raiseRuntimeError: This event loop is already runningif called from within an async context (e.g., when invoked viaarun→asyncio.to_thread→run→asyncio.run).Consider restructuring to avoid nested event loop issues:
🔎 Proposed fix: Use nest_asyncio or restructure
Option 1: Make
runfully synchronous using a new event loop in a thread:def run( self, query: str, project_id: Optional[str] = None, scope: Optional[str] = None, memory_type: Optional[str] = None, limit: int = 5 ) -> str: try: logger.info(...) - # Run async search in sync context - search_response = asyncio.run(self._async_search( - query, project_id, scope, memory_type, limit - )) + # Run async search in a new event loop in a separate thread + import concurrent.futures + with concurrent.futures.ThreadPoolExecutor() as pool: + future = pool.submit( + asyncio.run, + self._async_search(query, project_id, scope, memory_type, limit) + ) + search_response = future.result()Option 2: Simplify by making
arunthe primary method and removingrun's async call.Likely an incorrect or invalid review comment.
app/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/modules/intelligence/memory/memory_router.py (1)
12-36: Authentication properly implemented.The GET endpoint now correctly requires authentication and validates that the requested
user_idmatches the authenticated user. This addresses the security concern from the previous review.app/modules/intelligence/tools/memory_search_tool.py (1)
71-103: Resource management properly implemented.The class now correctly implements context manager protocol with
__enter__/__exit__, an explicitclose()method, and a__del__fallback for cleanup. This addresses the resource leak concern from the previous review.app/modules/intelligence/memory/letta_service.py (1)
182-216: Fact extraction properly uses configured model.The LLM call now correctly uses
self.llm_config.get("model", "gpt-4o-mini")(line 188) instead of a hardcoded model string. This addresses the concern from the previous review.
🧹 Nitpick comments (4)
docs/memory.md (2)
43-43: Format bare URL as markdown link.The bare URL should be formatted as a markdown link for better presentation and to satisfy markdown linting standards.
🔎 Proposed fix
-The Letta web UI is available at **http://localhost:8283** for viewing agents and memories. +The Letta web UI is available at [http://localhost:8283](http://localhost:8283) for viewing agents and memories.
78-96: Add language identifier to fenced code block.The architecture diagram code fence should specify a language identifier (or use
textfor plain ASCII art) to satisfy markdown linting standards.🔎 Proposed fix
-``` +```text ┌─────────────────┐ │ Conversation │ │ Service │ └────────┬────────┘ │ ▼ ┌─────────────────┐ │ LettaService │ │ (Memory Layer) │ └────────┬────────┘ │ ▼ ┌─────────────────┐ │ Letta Server │ │ (Docker) │ │ Port: 8283 │ └─────────────────┘ -``` +```app/modules/intelligence/memory/memory_router.py (1)
14-14: Consider removing redundant user_id parameter.Since the
user_idquery parameter must match the authenticated user (enforced at lines 30-36), it's redundant. You could simplify by removing the parameter and using the authenticated user's ID directly.🔎 Proposed refactor
@router.get("/memories") async def get_memories( - user_id: str = Query(..., description="User ID to fetch memories for"), project_id: Optional[str] = Query(None, description="Optional project ID to scope memories"), limit: Optional[int] = Query(None, ge=1, le=1000, description="Maximum number of memories to return"), user=Depends(AuthService.check_auth), ): """ Get all stored memories/preferences for a specific user. Returns all stored user preferences and memories that have been extracted from conversation history for the given user. Uses Letta's API to fetch memories. - - **user_id**: User ID (must match authenticated user) - **project_id**: Optional project ID to scope memories - **limit**: Maximum number of memories to return """ try: - # Verify user_id matches authenticated user - authenticated_user_id = user["user_id"] - if user_id != authenticated_user_id: - raise HTTPException( - status_code=403, - detail="Cannot fetch memories for another user" - ) + user_id = user["user_id"] memory_service = MemoryServiceFactory.create()Also applies to: 30-36
app/modules/intelligence/memory/letta_service.py (1)
244-244: Remove unnecessary f-string prefix.Line 244 has an f-string without any placeholders. Remove the
fprefix for clarity.🔎 Proposed fix
- logger.info(f"[letta add] No facts extracted from message") + logger.info("[letta add] No facts extracted from message")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
app/alembic/versions/20251222_enable_pgvector_extension.pyapp/modules/intelligence/memory/letta_service.pyapp/modules/intelligence/memory/memory_router.pyapp/modules/intelligence/tools/memory_search_tool.pydocker-compose.yamldocs/memory.mdpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
app/modules/intelligence/memory/memory_router.py (5)
app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-41)create(14-30)app/modules/auth/auth_service.py (1)
check_auth(48-97)app/modules/intelligence/memory/letta_service.py (3)
get_all_for_user(424-480)close(545-547)delete(482-543)app/modules/intelligence/memory/memory_interface.py (3)
get_all_for_user(47-54)close(67-69)delete(57-64)app/modules/intelligence/tools/memory_search_tool.py (1)
close(86-94)
🪛 markdownlint-cli2 (0.18.1)
docs/memory.md
43-43: Bare URL used
(MD034, no-bare-urls)
78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
app/modules/intelligence/memory/letta_service.py
139-139: Consider moving this statement to an else block
(TRY300)
210-210: Do not catch blind exception: Exception
(BLE001)
244-244: f-string without any placeholders
Remove extraneous f prefix
(F541)
296-298: Consider moving this statement to an else block
(TRY300)
418-418: Consider moving this statement to an else block
(TRY300)
508-508: Do not catch blind exception: Exception
(BLE001)
535-535: Do not catch blind exception: Exception
(BLE001)
app/modules/intelligence/memory/memory_router.py
17-17: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-36: Abstract raise to an inner function
(TRY301)
66-69: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
68-68: Use explicit conversion flag
Replace with conversion flag
(RUF010)
76-76: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
77-77: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
92-95: Abstract raise to an inner function
(TRY301)
124-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
126-126: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/intelligence/tools/memory_search_tool.py
91-91: Do not catch blind exception: Exception
(BLE001)
101-103: try-except-pass detected, consider logging the exception
(S110)
101-101: Do not catch blind exception: Exception
(BLE001)
188-188: Use explicit conversion flag
Replace with conversion flag
(RUF010)
213-213: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
app/alembic/versions/20251222_enable_pgvector_extension.py (3)
11-13: Revision ID format fixed.The revision ID now follows the date-prefixed format as suggested in the previous review. This maintains consistency with the migration naming convention.
23-26: LGTM!The
CASCADEclause is necessary to handle tables or columns using thevectortype, and the comment appropriately warns about potential cascading effects. This is standard practice for extension downgrades.
18-20: The migration is safe as-is. The docker-compose setup already usespgvector/pgvector:pg16, which includes the pgvector extension pre-installed. The migration's use ofCREATE EXTENSION IF NOT EXISTSis idempotent and handles scenarios where the extension may already be available. For deployments using external PostgreSQL instances, ensure pgvector is installed via thepostgresql-<version>-pgvectorpackage on the system before running migrations.docker-compose.yaml (1)
3-3: LGTM!The change to
pgvector/pgvector:pg16is appropriate for supporting vector operations required by Letta's embedding-based memory storage and search.app/modules/intelligence/memory/memory_router.py (1)
72-127: DELETE endpoint properly implemented.The DELETE endpoint correctly implements authentication, user validation, and resource cleanup. The same optional refactor suggestion for removing the redundant
user_idparameter applies here as well.app/modules/intelligence/tools/memory_search_tool.py (1)
190-231: Search logic properly implemented.The scope-based search logic correctly handles user-level, project-level, and combined searches with appropriate parameter validation. The error at line 213 for missing
project_idwhen searching project-level memories is appropriate and user-friendly.app/modules/intelligence/memory/letta_service.py (5)
19-37: Initialization and configuration look good.The service properly initializes the Letta client with configurable base URL and LLM config, with sensible defaults from environment variables. The agent cache is a good optimization for reducing API calls.
93-143: Agent management efficiently implemented.The agent creation and caching logic is well-designed: it checks the cache first, then searches for existing agents by name, and only creates a new agent if needed. This minimizes API calls to the Letta server.
218-302: Memory storage with smart routing looks excellent.The
addmethod intelligently routes memories to the appropriate agent based on their scope (lines 254-267): user-level memories go to the user agent (no project_id), while project-level memories go to the project-specific agent. This design ensures proper memory isolation and scoping.
304-372: Search implementation with deduplication is well-designed.The search method correctly queries both project-level and user-level agents when appropriate, deduplicates results (lines 343-351), and applies optional memory_type filtering. The logic properly handles different search scenarios based on
project_idandinclude_user_preferencesparameters.
482-543: Delete operation handles both specific and bulk deletion well.The delete method properly handles two scenarios: deleting specific passages by ID (lines 494-512) and deleting all passages for an agent (lines 513-539). Per-passage error handling ensures that failures on individual deletions don't prevent other deletions from succeeding.
ASCE-D
force-pushed
the
feat/letta-memory
branch
from
c915124 to
007969c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py (1)
289-302: Incomplete integration:_preferences_blockis never populated.The
_preferences_blockfield is initialized (line 302), reset inrun_stream(line 1357), and injected into the supervisor prompt (line 820), but it's never populated with actual memory data from thememory_manager.This means the preferences block will always be empty, and the memory system won't provide context to the agent.
Consider adding logic to populate
_preferences_blockat the start ofrun()andrun_stream()methods, such as:# Fetch user preferences from memory_manager if self.memory_manager: preferences = await self.memory_manager.get_preferences( user_id=ctx.user_id, project_id=ctx.project_id ) if preferences: self._preferences_block = f""" **User Preferences & Memory:** {preferences} """Would you like me to help implement the preference population logic?
♻️ Duplicate comments (3)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
546-551: Missing _preferences_block reset in run_stream.The
_preferences_blockis reset at the start ofrun()(line 458), but the same reset is missing inrun_stream(). This could lead to stale preferences persisting across streaming calls.🔎 Proposed fix
async def run_stream( self, ctx: ChatContext ) -> AsyncGenerator[ChatAgentResponse, None]: + self._preferences_block = "" + logger.info( f"Running pydantic-ai agent stream {'with multimodal support' if ctx.has_images() else ''}" )docker-compose.yaml (1)
48-64: Fix Letta service configuration issues.The Letta service configuration still has the issues identified in the previous review:
- Line 56: Incorrect volume mount to PostgreSQL data directory path
- Missing
LETTA_PG_URIenvironment variable to connect Letta to the postgres service- Missing
depends_onwith healthcheck to ensure PostgreSQL is readyPlease address the configuration issues as outlined in the previous review comment.
app/modules/intelligence/tools/memory_search_tool.py (1)
105-116: Nested event loop issue remains unresolved.The
arunmethod callsself.runviaasyncio.to_thread(lines 114-116), which then callsasyncio.run()at line 146. This creates a nested event loop situation that can cause runtime errors.Since
arunis already async, it should directlyawait self._async_search()instead of going through the syncrunmethod.🔎 Proposed fix
async def arun( self, query: str, project_id: Optional[str] = None, scope: Optional[str] = None, memory_type: Optional[str] = None, limit: int = 5 ) -> str: """Async version of memory search""" - return await asyncio.to_thread( - self.run, query, project_id, scope, memory_type, limit - ) + try: + logger.info( + f"[memory search tool] Searching: query='{query}', " + f"user_id={self.user_id}, project_id={project_id}, scope={scope}, memory_type={memory_type}, limit={limit}" + ) + + search_response = await self._async_search(query, project_id, scope, memory_type, limit) + + # ... (copy result formatting logic from run() method) + + except Exception as e: + logger.error(f"[memory search tool] Error: {e}", exc_info=True) + return f"Error searching memories: {str(e)}"
🧹 Nitpick comments (2)
app/modules/code_provider/code_provider_service.py (1)
42-66: Consider more specific exception handling.The broad
Exceptioncatch provides a safe fallback but could mask unexpected errors. While the logging helps with visibility, consider catching more specific exceptions related to API calls, network issues, or provider-specific errors.However, given that language data is supplementary metadata and the method logs failures, returning
{}as a fallback is reasonable for resilience.💡 Optional: More specific exception handling
- except Exception as e: + except (AttributeError, KeyError, RuntimeError, HTTPException) as e: logger.warning( "ProviderWrapper: Failed to fetch languages for %s via provider %s: %s", self.full_name, provider.__class__.__name__, e, ) return {} + except Exception as e: + logger.error( + "ProviderWrapper: Unexpected error fetching languages for %s: %s", + self.full_name, + e, + ) + raiseThis distinguishes between expected failures (log as warning, return
{}) and unexpected errors (log as error, re-raise).app/modules/intelligence/tools/memory_search_tool.py (1)
213-215: Consider extracting error message to exception class.The ValueError message is quite long and specific. Per static analysis hint TRY003, consider defining a custom exception class with this message, or simplify the inline message.
🔎 Proposed refactor
+class ProjectScopeError(ValueError): + """Raised when project_id is required but not provided""" + def __init__(self): + super().__init__("Cannot search project-level memories: project_id is required") + async def _async_search(...): ... elif scope == "project": if not project_id: - raise ValueError( - "Cannot search project-level memories: project_id is required" - ) + raise ProjectScopeError()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
GETTING_STARTED.mdapp/alembic/versions/20251222_enable_pgvector_extension.pyapp/celery/celery_app.pyapp/celery/tasks/agent_tasks.pyapp/celery/tasks/memory_tasks.pyapp/celery/worker.pyapp/main.pyapp/modules/code_provider/code_provider_service.pyapp/modules/conversations/conversation/conversation_service.pyapp/modules/intelligence/agents/agents_service.pyapp/modules/intelligence/agents/chat_agent.pyapp/modules/intelligence/agents/chat_agents/pydantic_agent.pyapp/modules/intelligence/agents/chat_agents/pydantic_multi_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/debug_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/qna_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.pyapp/modules/intelligence/agents/chat_agents/tool_helpers.pyapp/modules/intelligence/memory/letta_service.pyapp/modules/intelligence/memory/memory_interface.pyapp/modules/intelligence/memory/memory_router.pyapp/modules/intelligence/memory/memory_service_factory.pyapp/modules/intelligence/tools/memory_search_tool.pyapp/modules/intelligence/tools/tool_service.pydocker-compose.yamldocs/memory.mdpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- app/alembic/versions/20251222_enable_pgvector_extension.py
- pyproject.toml
- app/modules/conversations/conversation/conversation_service.py
- app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py
- app/modules/intelligence/tools/tool_service.py
- app/modules/intelligence/agents/chat_agents/system_agents/debug_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py
- app/modules/intelligence/agents/chat_agents/tool_helpers.py
🧰 Additional context used
🧬 Code graph analysis (11)
app/celery/tasks/agent_tasks.py (4)
app/celery/tasks/memory_tasks.py (1)
extract_user_preferences(16-93)app/modules/intelligence/memory/chat_history_service.py (2)
ChatHistoryService(23-159)get_session_history(28-66)app/celery/tasks/base_task.py (1)
db(15-18)app/modules/conversations/conversation/conversation_model.py (1)
Conversation(23-59)
app/modules/intelligence/memory/memory_router.py (4)
app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-43)create(14-30)app/modules/auth/auth_service.py (2)
AuthService(14-100)check_auth(48-100)app/modules/intelligence/memory/letta_service.py (2)
get_all_for_user(485-538)delete(540-607)app/modules/intelligence/memory/memory_interface.py (2)
get_all_for_user(49-56)delete(59-66)
app/celery/tasks/memory_tasks.py (4)
app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-43)create(14-30)app/modules/intelligence/memory/letta_service.py (2)
add(273-364)close(609-611)app/modules/intelligence/memory/memory_interface.py (2)
add(38-46)close(69-71)app/modules/intelligence/tools/memory_search_tool.py (1)
close(86-94)
app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1011)
app/celery/worker.py (1)
app/celery/tasks/memory_tasks.py (1)
extract_user_preferences(16-93)
app/modules/intelligence/tools/memory_search_tool.py (3)
app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-43)create(14-30)app/modules/intelligence/memory/letta_service.py (2)
close(609-611)search(366-434)app/modules/intelligence/memory/memory_interface.py (2)
close(69-71)search(25-35)
app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1011)
app/modules/intelligence/memory/memory_service_factory.py (2)
app/modules/intelligence/memory/memory_interface.py (1)
MemoryInterface(21-71)app/modules/intelligence/memory/letta_service.py (1)
LettaService(16-611)
app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1011)
app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py (1)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
PydanticRagAgent(63-1011)
app/modules/code_provider/code_provider_service.py (3)
app/modules/code_provider/local_repo/local_provider.py (1)
_get_repo(49-63)app/modules/code_provider/gitbucket/gitbucket_provider.py (1)
_get_repo(99-117)app/modules/code_provider/local_repo/local_repo_service.py (1)
get_repo(46-51)
🪛 markdownlint-cli2 (0.18.1)
GETTING_STARTED.md
86-86: Bare URL used
(MD034, no-bare-urls)
docs/memory.md
86-86: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.10)
app/celery/tasks/agent_tasks.py
250-250: Do not catch blind exception: Exception
(BLE001)
app/modules/intelligence/memory/memory_router.py
21-21: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-39: Abstract raise to an inner function
(TRY301)
67-69: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
68-68: Use explicit conversion flag
Replace with conversion flag
(RUF010)
78-81: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
82-82: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
97-99: Abstract raise to an inner function
(TRY301)
123-125: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
124-124: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/intelligence/tools/memory_search_tool.py
91-91: Do not catch blind exception: Exception
(BLE001)
101-103: try-except-pass detected, consider logging the exception
(S110)
101-101: Do not catch blind exception: Exception
(BLE001)
188-188: Use explicit conversion flag
Replace with conversion flag
(RUF010)
213-215: Avoid specifying long messages outside the exception class
(TRY003)
app/modules/intelligence/memory/letta_service.py
141-141: Consider moving this statement to an else block
(TRY300)
263-263: Do not catch blind exception: Exception
(BLE001)
360-360: Consider moving this statement to an else block
(TRY300)
479-479: Consider moving this statement to an else block
(TRY300)
566-566: Do not catch blind exception: Exception
(BLE001)
595-595: Do not catch blind exception: Exception
(BLE001)
app/celery/celery_app.py
146-146: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
app/modules/code_provider/code_provider_service.py
59-59: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (25)
app/modules/intelligence/agents/chat_agent.py (1)
54-54: LGTM! Clean addition of user_id field for memory operations.The optional
user_idfield is properly typed and documented, maintaining backward compatibility with existing code paths.GETTING_STARTED.md (1)
21-26: LGTM! Clear documentation of Letta environment variables.The environment variables are well-documented with sensible defaults, and the note about automatic startup helps users understand the deployment model.
app/modules/intelligence/memory/memory_interface.py (1)
1-71: LGTM! Well-designed memory interface.The abstract interface provides a clean contract for memory operations with:
- Proper type hints using Pydantic models
- Comprehensive CRUD operations (search, add, get_all, delete)
- Resource cleanup via synchronous
close()method- Clear separation of concerns between user-level and project-level memories
app/celery/worker.py (2)
14-16: LGTM! Proper import of memory task.The import follows the established pattern for other task modules.
34-35: LGTM! Task registration is correct.The memory task is properly registered alongside other Celery tasks, maintaining consistency with the existing registration pattern.
app/main.py (2)
19-19: LGTM! Memory router import is correct.The import follows the established pattern for other router modules.
156-156: LGTM! Memory router is properly registered.The router is correctly included with the standard
/api/v1prefix and appropriate tag, consistent with other router registrations.app/celery/celery_app.py (1)
100-102: LGTM! Memory task routing is properly configured.The
extract_user_preferencestask is correctly routed to theagent_tasksqueue, which is appropriate since the worker is already listening to that queue and the task is part of the agent workflow.app/modules/intelligence/agents/agents_service.py (2)
72-92: LGTM! Clean refactoring to pre-instantiate system agents.Pre-creating agent instances improves code clarity and prepares the structure for potential reuse across multiple calls without the overhead of repeated instantiation.
94-143: LGTM! Agent mappings correctly reference pre-instantiated instances.The dictionary now cleanly references the pre-created agents, maintaining the same functionality while improving maintainability.
app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py (1)
54-59: LGTM! Memory manager integration follows the correct pattern.The
memory_managerparameter is properly propagated usinggetattr(self, "memory_manager", None), which maintains backward compatibility while enabling memory capabilities when the attribute is present.app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py (1)
125-150: LGTM! Clean memory_manager propagation pattern.The memory_manager is correctly propagated through all agent construction paths (multi-agent, standard Pydantic, and fallback) using
getattr(self, "memory_manager", None). This defensive pattern gracefully handles cases where memory_manager may not be present on the instance.docs/memory.md (1)
1-127: Excellent documentation for the Memory System!The documentation is comprehensive, well-structured, and covers all essential aspects:
- Clear prerequisites and setup instructions
- Configuration options with defaults
- Architecture overview
- Troubleshooting guidance
This will help users understand and configure the Letta-based memory system effectively.
app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py (1)
110-135: LGTM! Consistent memory_manager integration.The memory_manager propagation follows the same defensive pattern as other system agents, ensuring consistent behavior across multi-agent and single-agent paths.
app/celery/tasks/agent_tasks.py (1)
179-256: Good defensive error handling for memory extraction.The memory extraction is correctly implemented as a fire-and-forget task with proper exception handling. The broad Exception catch on line 250 is appropriate here since memory extraction failures should not impact the main agent task completion.
app/modules/intelligence/agents/chat_agents/pydantic_agent.py (1)
70-88: LGTM! Memory manager integration is clean.The optional memory_manager parameter is properly added to the constructor and stored for future use. The _preferences_block cache is initialized appropriately.
app/modules/intelligence/memory/memory_service_factory.py (1)
10-43: LGTM! Clean factory pattern with good defaults.The factory provides a clean abstraction for creating memory service instances. The default configuration reading from environment variables is well-implemented, and the warning for missing
OPENAI_API_KEYhelps with troubleshooting.app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (2)
78-78: Good addition of memory search tool.The
search_user_memoriestool integration enables the GeneralPurposeAgent to leverage the new Letta-based memory system for retrieving user preferences and project-specific context.
111-136: LGTM! Consistent memory_manager propagation.The memory_manager is correctly threaded through all agent construction paths, maintaining consistency with other system agents in the codebase.
app/celery/tasks/memory_tasks.py (1)
11-93: LGTM! Well-structured Celery task with proper resource management.The task implementation follows best practices:
- Proper async handling via
self.run_async- Comprehensive logging with structured context
- Memory service cleanup in finally block (line 81)
- Robust error handling and propagation
The result processing logic (lines 56-72) correctly extracts memory contents from various response formats.
app/modules/intelligence/memory/memory_router.py (1)
12-69: LGTM! Endpoints are properly secured with authentication.Both GET and DELETE endpoints correctly:
- Require authentication via
AuthService.check_auth(lines 21, 82)- Validate that
user_idmatches the authenticated user (lines 35-39, 95-99)- Return 403 for unauthorized access attempts
- Clean up memory service resources in finally blocks
This addresses the security concern from the previous review.
app/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py (1)
820-821: Verify injection point for empty preferences block.The
_preferences_blockis injected into the supervisor instructions here, but since it's never populated (see comment on lines 289-302), this will always inject an empty string. While not harmful, this suggests the feature is incomplete.app/modules/intelligence/memory/letta_service.py (3)
16-145: LGTM! Well-structured Letta service initialization and agent management.The implementation correctly:
- Uses configurable LLM settings (line 240 addresses previous hardcoded model issue)
- Implements agent caching to avoid redundant lookups (lines 29, 100-101)
- Handles async operations properly with
run_in_executor- Provides comprehensive logging for debugging
Agent naming convention (lines 88-91) creates clear separation between user-level and project-level agents.
147-231: Solid memory categorization logic.The
_categorize_memorymethod uses practical heuristics to determine:
- Memory type: episodic vs semantic (lines 152-175)
- Scope: user vs project (lines 177-230)
The indicator-based approach is straightforward and maintainable. The fallback logic (lines 224-229) handles ambiguous cases reasonably.
273-364: Robust memory addition with intelligent routing.The
addmethod implementation is excellent:
- Extracts facts from the latest user message (lines 286-296)
- Categorizes each fact and routes to the appropriate agent (lines 310-325)
- Stores memories with rich metadata including scope and type (lines 335-349)
- Provides detailed logging for observability
The separation of user-scoped vs project-scoped memories (lines 316-325) enables proper memory isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py (1)
139-196: Consider documenting the "search_user_memories" tool in the HOW TO GUIDE.The prompt's "HOW TO GUIDE" section documents available tools and their usage patterns, but doesn't mention the newly added memory search capability. Adding guidance on when and how to use user memories could help the agent leverage this feature more effectively.
💡 Suggested addition to the HOW TO GUIDE
Consider adding a step after line 153 to document the memory search tool:
9. Use above mentioned tools to fetch imported code, referenced code, helper functions, classes etc to understand the control flow + 10. Use search_user_memories to retrieve relevant user preferences, patterns, or past context that might inform test generation Your mission is to create comprehensive test plans and corresponding integration tests based on the user's query and provided code.app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py (1)
38-50: Tool is properly registered—consider enhancing the prompt for better utilization.The
search_user_memoriestool is correctly registered in ToolService and fully integrated with proper handlers. This addition follows a systematic pattern being applied across all system agents (general_purpose_agent, code_gen_agent, debug_agent, qna_agent, and others), indicating a deliberate PR-wide effort to enable personalized context in agent reasoning.For blast_radius_agent specifically, memory search is conceptually valuable—it can surface past similar changes, known sensitivities, or user preferences that inform impact analysis. However, the task prompt (lines 68-120) doesn't currently guide when or how to leverage this capability. Consider adding a step in the "HOW TO GUIDE" section to suggest the agent search for related past changes or known issues, helping ensure the tool is effectively utilized.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/debug_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/qna_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.pyapp/modules/intelligence/agents/custom_agents/custom_agents_service.py
🚧 Files skipped from review as they are similar to previous changes (6)
- app/modules/intelligence/agents/chat_agents/system_agents/debug_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py
🔇 Additional comments (4)
app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py (1)
46-58: No concerns found. The "search_user_memories" tool is properly implemented inmemory_search_tool.pyand registered inToolServiceat line 195. The change aligns with the PR's memory integration objectives and follows the pattern established across other system agents.app/modules/intelligence/agents/custom_agents/custom_agents_service.py (3)
1011-1016: Clear and actionable guidance forsearch_user_memories.The tool-specific guidance effectively describes:
- What the tool queries (user preferences, past interactions, personalized context)
- When to use it (understanding coding style, naming conventions, requirements)
- Why it's valuable (personalized and contextually relevant responses)
- How to format queries (search for relevant preferences)
This level of detail will help the LLM understand when and how to leverage the memory system effectively.
1082-1082: Consistent guidance across both templates.The
search_user_memoriesguidance is consistently replicated in bothCREATE_AGENT_FROM_PROMPTandTASK_ENHANCEMENT_PROMPTtemplates. This ensures uniform behavior whether agents are created from prompts or tasks are enhanced.However, the same validation concern applies here: verify that
search_user_memoriesis available in the tools list before mandating its inclusion with "CRITICAL" and "ALWAYS" language.Also applies to: 1145-1150
926-930: The review comment aboutsearch_user_memoriestool availability is not applicable. The tool is unconditionally initialized inToolService._initialize_tools()and is always included in the available tools list. Since it's guaranteed to be present, the template's "CRITICAL" and "ALWAYS" language for including this tool is appropriate and will not cause runtime failures from missing tools.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/modules/intelligence/tools/memory_search_tool.py (1)
264-266: Consider extracting error message to a constant.The ValueError message is clear, but per Python best practices (PEP 8), consider extracting longer exception messages to module-level constants to improve maintainability.
🔎 Optional refactor
At module level (after imports):
_PROJECT_ID_REQUIRED_ERROR = ( "Cannot search project-level memories: project_id is required" )Then use:
if not project_id: - raise ValueError( - "Cannot search project-level memories: project_id is required" - ) + raise ValueError(_PROJECT_ID_REQUIRED_ERROR)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/modules/intelligence/tools/memory_search_tool.py
🧰 Additional context used
🪛 Ruff (0.14.10)
app/modules/intelligence/tools/memory_search_tool.py
91-91: Do not catch blind exception: Exception
(BLE001)
101-103: try-except-pass detected, consider logging the exception
(S110)
101-101: Do not catch blind exception: Exception
(BLE001)
199-199: Use explicit conversion flag
Replace with conversion flag
(RUF010)
239-239: Use explicit conversion flag
Replace with conversion flag
(RUF010)
264-266: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
app/modules/intelligence/tools/memory_search_tool.py (2)
71-104: LGTM! Lifecycle management properly implemented.The context manager pattern, close() method, and del fallback correctly address the previous resource leak concern. The _closed flag prevents double-close issues.
162-200: LGTM! Previous nested event loop issue resolved.The async method now directly calls
_async_searchwithout going through the syncrunmethod, avoiding the nested event loop problem flagged in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
app/modules/intelligence/agents/custom_agents/custom_agents_service.py (1)
1011-1020: Consider extracting duplicated memory guidance.The detailed
search_user_memoriesguidance is duplicated between theCREATE_AGENT_FROM_PROMPT(lines 1011-1020) andTASK_ENHANCEMENT_PROMPT(lines 1149-1158) templates. While the duplication ensures each template is self-contained, it creates maintenance overhead.Consider extracting this guidance into a shared constant or method that can be interpolated into both templates, ensuring consistency and reducing the risk of divergence.
💡 Example refactoring approach
# Extract common guidance SEARCH_USER_MEMORIES_GUIDANCE = """ **search_user_memories (ALWAYS RECOMMENDED):** - **This tool provides access to user's past interactions, preferences, and decisions** - Agents should use this tool actively throughout their work whenever they need context - Query user preferences, past interactions, and personalized context - Use to understand user's coding style preferences, naming conventions, or specific requirements - Essential for providing personalized and contextually relevant responses - **Instruct agents:** "You have access to past memories via search_user_memories. Use this tool whenever you need context about user preferences or past interactions." - Format queries to search for relevant user preferences related to the current task - Use `scope="project"` for project-specific memories, `scope="user"` for cross-project preferences """ # Then use in templates: CREATE_AGENT_FROM_PROMPT = f""" ... {SEARCH_USER_MEMORIES_GUIDANCE} ... """ TASK_ENHANCEMENT_PROMPT = f""" ... {SEARCH_USER_MEMORIES_GUIDANCE} ... """Also applies to: 1149-1158
app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (1)
133-142: Consider consolidating memory search introduction.The prompt has two separate sections introducing memory search (lines 134-136 and 137-142). While both provide value, consolidating them into a single cohesive section might improve clarity and reduce redundancy.
🔎 Proposed consolidation
- 🧠 **YOU HAVE ACCESS TO PAST MEMORIES:** - You have the `search_user_memories` tool that provides access to user's past interactions, preferences, and decisions. Use this tool whenever you need context to provide better, personalized responses. - - 🧠 **USE MEMORY SEARCH ACTIVELY:** + 🧠 **YOU HAVE ACCESS TO PAST MEMORIES - USE THEM ACTIVELY:** + You have the `search_user_memories` tool that provides access to user's past interactions, preferences, and decisions. + - **Search memories throughout your work** using `search_user_memories` - Query for: user preferences, past interactions, relevant project decisions, any contextual information - Personalize your responses based on discovered memories - Example queries: "user preferences", "coding style", "[topic] preferences", "past decisions about [topic]" - Use this tool whenever you're making recommendations or providing guidanceapp/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py (1)
782-858: Consider resilience for mandatory memory checks.The supervisor instructions make memory checking MANDATORY in the planning phase (line 805: "Check Memories: Use
search_user_memories..."), but don't address what the agent should do if memory searches fail or are unavailable.Since this is marked as the first mandatory step in the planning phase, a failure here could block the entire workflow.
Consider adding guidance for when memory searches fail, such as:
**🚀 MANDATORY PLANNING PHASE (DO THIS FIRST):** 1. **Check Memories:** Use `search_user_memories` to find relevant user preferences, past decisions, and project-specific context + (If memory search is unavailable, proceed with documented assumptions) 2. **Analyze:** Understand the request, identify objectives, dependencies, and constraintsNote on verbosity: The memory instructions are thorough but quite verbose (lines 784-802, 852-858). While comprehensive, consider that LLMs have context limits and shorter, more focused instructions might be more effective. The current repetition of "search memories before making decisions" appears at lines 796-800, 805, 853, and 855-856.
app/modules/auth/sso_providers/google_provider.py (2)
97-102: Fragile name parsing logic.Splitting
nameby space to derivegiven_nameandfamily_nameis unreliable for names with multiple spaces (e.g., "Mary Jane Watson"), single-word names (e.g., "Madonna" → family_name becomes ""), or cultural naming conventions where family name comes first.Consider using the name as-is for
display_nameand leavinggiven_name/family_nameasNonewhen only a composite name is available, or check if Firebase provides separate fields.🔎 Proposed fix
user_info = SSOUserInfo( email=decoded_token.get("email", ""), email_verified=decoded_token.get("email_verified", False), provider_uid=decoded_token["uid"], # Firebase UID - consistent! display_name=decoded_token.get("name"), - given_name=decoded_token.get("name", "").split(" ")[0] - if decoded_token.get("name") - else None, - family_name=" ".join(decoded_token.get("name", "").split(" ")[1:]) - if decoded_token.get("name") - else None, + given_name=None, # Firebase only provides composite 'name' + family_name=None, picture=decoded_token.get("picture"), raw_data=decoded_token, )
122-126: Broad exception catch is acceptable here but consider narrowing.While catching bare
Exceptionis flagged by static analysis (BLE001), it's intentional as a fallback to Google OAuth. However, this could mask unexpected errors (e.g., network issues, SDK bugs). Consider logging atwarninglevel instead ofdebugfor non-specific exceptions to improve observability.🔎 Proposed fix
except Exception as e: # Other Firebase errors, try Google OAuth - logger.debug( + logger.warning( "Firebase verification failed, trying Google OAuth: %s", str(e) )app/modules/auth/unified_auth_service.py (2)
445-506: Consider consolidating exception handling and using more specific exception types.The nested exception handling with multiple blind
Exceptioncatches makes error tracing difficult. The repeated pattern checking for"does not exist"or"initialize_app"in exception strings is fragile.🔎 Suggested refactor to consolidate exception handling
+def _is_firebase_init_error(error: Exception) -> bool: + """Check if error indicates Firebase is not initialized.""" + error_str = str(error) + return "does not exist" in error_str or "initialize_app" in error_str firebase_user_exists = False try: try: firebase_admin.get_app() firebase_initialized = True except ValueError: firebase_initialized = False logger.warning( - f"Firebase not initialized. Skipping Firebase verification for user {existing_user.uid}. " - f"This is normal in development mode. Error: {str(init_error)}" + "Firebase not initialized for user %s. Normal in dev mode.", + existing_user.uid, ) firebase_user_exists = True if firebase_initialized: try: auth.get_user(existing_user.uid) firebase_user_exists = True except NotFoundError: firebase_user_exists = False - except Exception as e: - if "does not exist" in str(e) or "initialize_app" in str(e): - # ... - else: - # ... + except Exception as e: + if _is_firebase_init_error(e): + logger.warning("Firebase not initialized, assuming user exists") + firebase_user_exists = True + else: + logger.exception("Error verifying Firebase user %s", existing_user.uid) + firebase_user_exists = True except Exception as e: - # Similar handling... + if _is_firebase_init_error(e): + logger.warning("Firebase not initialized, assuming user exists") + else: + logger.exception("Unexpected error verifying Firebase user %s", existing_user.uid) + firebase_user_exists = True
683-759: Code duplication in race condition handling.The GitHub linking check logic (lines 699-721) duplicates the pattern from lines 573-609. While functional, consider extracting to a helper method if this pattern is used elsewhere.
The current implementation is correct; this is just a maintainability suggestion for future refactoring.
app/modules/auth/auth_router.py (1)
233-237: Redundantdb.commit()afteradd_provider.The
add_providermethod inUnifiedAuthServicealready commits the transaction (at line 293 of unified_auth_service.py). Thedb.commit()at line 237 is redundant.🔎 Proposed fix
try: unified_auth.add_provider( user_id=user.uid, provider_create=provider_create ) - db.commit() except IntegrityError as e:app/modules/users/user_schema.py (1)
41-53: Consider adding validation constraints and documenting naming convention.The models are functional but could benefit from:
- Field validation (e.g., email format, max lengths)
- A comment explaining the camelCase naming (likely for frontend compatibility)
🔎 Example with validation
from pydantic import BaseModel, EmailStr, Field class OnboardingDataRequest(BaseModel): """Request model for onboarding data. Uses camelCase for frontend compatibility.""" uid: str = Field(..., min_length=1) email: EmailStr name: str = Field(..., min_length=1, max_length=255) source: str = Field(..., max_length=100) industry: str = Field(..., max_length=100) jobTitle: str = Field(..., max_length=100) # camelCase for frontend companyName: str = Field(..., max_length=255) # camelCase for frontend
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/modules/auth/auth_router.pyapp/modules/auth/auth_schema.pyapp/modules/auth/sso_providers/google_provider.pyapp/modules/auth/unified_auth_service.pyapp/modules/code_provider/github/github_service.pyapp/modules/intelligence/agents/chat_agents/pydantic_multi_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/debug_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/qna_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.pyapp/modules/intelligence/agents/custom_agents/custom_agents_service.pyapp/modules/intelligence/agents/custom_agents/runtime_agent.pyapp/modules/users/user_router.pyapp/modules/users/user_schema.py
🚧 Files skipped from review as they are similar to previous changes (5)
- app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/debug_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T10:22:24.733Z
Learnt from: dhirenmathur
Repo: potpie-ai/potpie PR: 506
File: app/modules/auth/auth_provider_model.py:59-63
Timestamp: 2025-12-17T10:22:24.733Z
Learning: Ensure OAuth tokens (access_token, refresh_token) stored in the database are encrypted at rest using the TokenEncryption utility (Fernet-based, located at app/modules/integrations/token_encryption.py) before persistence and decrypted upon retrieval. Apply this guidance to all authentication-related data persistence code paths in app/modules/auth. Update models, repositories, and service layers to invoke encryption/decryption consistently, avoid logging plaintext tokens, and verify encryption is applied at every token write/read path for SOC 2 compliance.
Applied to files:
app/modules/auth/sso_providers/google_provider.pyapp/modules/auth/auth_router.pyapp/modules/auth/unified_auth_service.pyapp/modules/auth/auth_schema.py
🧬 Code graph analysis (3)
app/modules/auth/sso_providers/google_provider.py (2)
app/modules/auth/sso_providers/base_provider.py (1)
SSOUserInfo(13-23)tests/conftest.py (1)
get(142-150)
app/modules/auth/auth_router.py (6)
app/modules/auth/auth_service.py (1)
check_auth(48-100)app/modules/auth/auth_provider_model.py (1)
UserAuthProvider(25-83)app/modules/auth/auth_schema.py (1)
AuthProviderCreate(26-37)app/modules/users/user_service.py (1)
update_last_login(24-56)app/modules/auth/unified_auth_service.py (3)
add_provider(228-311)authenticate_or_create(387-805)check_github_linked(130-174)app/modules/utils/posthog_helper.py (2)
PostHogClient(9-45)send_event(21-45)
app/modules/code_provider/github/github_service.py (1)
app/modules/auth/auth_provider_model.py (1)
UserAuthProvider(25-83)
🪛 Ruff (0.14.10)
app/modules/auth/sso_providers/google_provider.py
112-112: Consider moving this statement to an else block
(TRY300)
122-122: Do not catch blind exception: Exception
(BLE001)
app/modules/users/user_router.py
32-32: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-33: Unused method argument: db
(ARG002)
33-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
50-53: Abstract raise to an inner function
(TRY301)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
88-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
89-89: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/auth/auth_router.py
247-249: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
254-254: Use explicit conversion flag
Replace with conversion flag
(RUF010)
361-361: Use explicit conversion flag
Replace with conversion flag
(RUF010)
606-606: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/code_provider/github/github_service.py
297-300: Abstract raise to an inner function
(TRY301)
329-329: Do not catch blind exception: Exception
(BLE001)
330-330: Use explicit conversion flag
Replace with conversion flag
(RUF010)
334-337: Abstract raise to an inner function
(TRY301)
app/modules/auth/unified_auth_service.py
451-451: Do not catch blind exception: Exception
(BLE001)
456-456: Use explicit conversion flag
Replace with conversion flag
(RUF010)
479-479: Do not catch blind exception: Exception
(BLE001)
488-490: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
489-489: Use explicit conversion flag
Replace with conversion flag
(RUF010)
494-494: Do not catch blind exception: Exception
(BLE001)
502-504: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
503-503: Use explicit conversion flag
Replace with conversion flag
(RUF010)
538-538: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1115-1115: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1122-1123: try-except-pass detected, consider logging the exception
(S110)
1122-1122: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (19)
app/modules/intelligence/agents/custom_agents/runtime_agent.py (1)
148-161: LGTM! Clear and actionable memory guidance.The memory-related prompts are well-structured and provide clear instructions for agents to proactively use the
search_user_memoriestool. The guidance appropriately emphasizes:
- When to search memories (before making decisions)
- What to query for (coding style, preferences, past decisions)
- Why it's valuable (personalized, context-aware responses)
This aligns well with the PR's goal of adding long-term memory capabilities.
app/modules/intelligence/agents/custom_agents/custom_agents_service.py (3)
926-926: LGTM! Appropriate critical emphasis.The CRITICAL marker appropriately highlights the importance of including the
search_user_memoriestool for memory-driven agent behavior.
930-930: LGTM! Consistent guidance.The instruction to ALWAYS include
search_user_memoriesis consistent with the critical emphasis earlier and reinforces the memory-first approach.
1058-1058: LGTM! Effective reinforcement of critical requirements.These CRITICAL reminders appropriately reinforce the importance of including
search_user_memoriesin both prompt templates, ensuring the requirement is not overlooked during agent planning.Also applies to: 1086-1086
app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py (2)
85-85: LGTM: Memory search tool added to QnA agent.The addition of
search_user_memoriesto the tools list appropriately extends the QnA agent's capabilities for context-aware responses.
159-169: Well-structured memory search integration.The prompt updates are well-organized with a clear STEP 0 that precedes the existing traversal steps. The memory search queries are contextually appropriate for Q&A tasks (coding preferences, project architecture, past decisions), and the integration aligns well with the agent's purpose.
app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py (2)
80-80: LGTM: Memory search tool added to design agent.The addition of
search_user_memoriesextends the LowLevelDesignAgent's capabilities to access past architectural decisions and design patterns, which is appropriate for its role.
155-164: Excellent design-specific memory integration.The prompt updates are well-tailored to the design agent's purpose. The memory search queries ("architecture preferences", "design patterns", "framework choices", "past architectural decisions") are contextually appropriate and will help ensure design consistency with established patterns.
app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (1)
78-78: No action needed. Thesearch_user_memoriestool is already properly registered in ToolService (line 195 oftool_service.py) and is correctly available to agents via theget_tools()method.Likely an incorrect or invalid review comment.
app/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py (2)
435-453: Good memory integration in delegation prompts.The memory access instructions are well-integrated into the delegation prompt, emphasizing proactive memory usage before decision-making. The instruction to search memories "BEFORE making style/formatting/architecture decisions" (line 446) is particularly valuable for maintaining consistency with user preferences.
The instructions are appropriately repeated here even though similar guidance exists in
DELEGATE_AGENT_INSTRUCTIONS, as this delegation prompt serves a different context and agents need clear reminders.
311-330: Recommendation is not applicable — thesearch_user_memoriestool already has comprehensive error handling built in. The tool gracefully handles failures by catching exceptions, logging errors, and returning formatted messages. Agents don't need explicit error-handling guidance in instructions because the tool transparently manages all edge cases (unavailable service, empty results, timeouts) and always returns a readable response string.Likely an incorrect or invalid review comment.
app/modules/auth/sso_providers/google_provider.py (1)
64-78: Clear documentation of dual verification strategy.The docstring accurately describes the two token types and the fallback behavior. This is helpful for maintainability.
app/modules/auth/auth_schema.py (1)
86-88: LGTM!The new
needs_github_linkingfield is well-designed with an appropriate type (Optional[bool]), sensible default (False), and clear description. This cleanly extends the SSO login response to support the GitHub linking flow.app/modules/auth/unified_auth_service.py (2)
130-174: LGTM with minor note on logging.The method logic is clear and well-documented. Consider using lazy string formatting for logging (e.g.,
logger.info("User %s not found", user_id)) instead of f-strings for slight performance improvement when the log level is disabled, but this is a minor stylistic preference.
959-1124: CASCADE delete properly configured for user_auth_providers and pending_provider_links — no explicit deletion needed.The orphaned user deletion method comprehensively handles all related records. Both
user_auth_providersandpending_provider_linksare correctly configured withondelete="CASCADE"on theiruser_idforeign keys, so they are automatically deleted when the user is removed. All other tables without CASCADE (prompts,user_preferences,custom_agents,organization_sso_config) are explicitly deleted in the correct dependency order before user deletion. The defensive bare except at lines 1122–1123 appropriately handles already-expunged objects after rollback.app/modules/code_provider/github/github_service.py (1)
281-337: LGTM - Well-designed multi-path GitHub username resolution.The implementation provides a robust fallback chain:
- Unified auth provider data
- Legacy
provider_usernamefield- GitHub API lookup as last resort
This ensures backward compatibility while supporting the new provider system. The error messages are clear and guide users to link their GitHub account.
app/modules/auth/auth_router.py (2)
12-29: LGTM!The new imports are appropriate for supporting the multi-provider authentication flows and IntegrityError handling for database constraint violations.
604-617: LGTM!Using
Depends(AuthService.check_auth)is the correct FastAPI pattern for dependency injection. The dual key check (uidoruser_id) provides good compatibility with different token formats. The static analysis warning aboutDependsin argument defaults is a false positive for FastAPI.app/modules/users/user_router.py (1)
55-76: LGTM - Firestore write implementation is correct.The use of
merge=Trueis appropriate for onboarding updates, allowing partial document updates. The Admin SDK provides the necessary permissions for server-side writes.
ASCE-D
force-pushed
the
feat/letta-memory
branch
from
f0a40cd to
b3726b4
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
GETTING_STARTED.md (1)
68-95: Format bare URL for better rendering.The documentation is clear and comprehensive. However, line 86 contains a bare URL that should be formatted.
🔎 Proposed fix
-The Letta web UI is available at http://localhost:8283 for viewing agents and memories. +The Letta web UI is available at [http://localhost:8283](http://localhost:8283) for viewing agents and memories.app/celery/celery_app.py (1)
146-146: Remove unused noqa directive.The blanket
noqadirective is flagged as unused by Ruff. If there's no specific linting issue to suppress, remove the directive.🔎 Proposed fix
-import app.celery.tasks.memory_tasks # noqa # Ensure memory tasks are registered +import app.celery.tasks.memory_tasks # Ensure memory tasks are registeredapp/celery/tasks/agent_tasks.py (1)
198-212: Fix inconsistent comment about message count.Line 198 states "last 10 messages for context" but the code on lines 208-210 only extracts the last 2 messages (
recent_history[-2:]). Update the comment to accurately reflect the actual behavior.app/modules/intelligence/agents/chat_agents/tool_helpers.py (1)
495-513: Add type safety for project_id slicing.Line 504 assumes
project_idis a string when slicing with[:8]. Ifproject_idis a different type (e.g., UUID object, int), this will raise aTypeError.🔎 Proposed fix to ensure type safety
filters = [] if project_id: - filters.append(f"project: {project_id[:8]}...") + filters.append(f"project: {str(project_id)[:8]}...") if scope: filters.append(f"scope: {scope}")
🧹 Nitpick comments (2)
app/alembic/versions/20251222_enable_pgvector_extension.py (1)
23-26: Clarify the comment to accurately reflect CASCADE behavior.The comment states that dropping the extension "may fail" and "manual cleanup may be required," but the
CASCADEclause ensures the operation will succeed by automatically dropping all dependent objects (tables, columns, indexes using the vector type). This could be misleading about the actual behavior and potential data loss implications.🔎 Suggested comment clarification
def downgrade() -> None: - # Note: Dropping extension may fail if tables use vector type - # This is a safety measure - manual cleanup may be required + # Note: CASCADE will automatically drop all dependent objects + # (tables, columns, indexes using vector type). This may result in data loss. + # Consider backing up data before running this downgrade. op.execute('DROP EXTENSION IF EXISTS vector CASCADE')app/modules/intelligence/memory/letta_service.py (1)
485-538: LGTM! Retrieval method with minor efficiency consideration.The method correctly retrieves all memories for a user/project. One minor note: the limit is applied after fetching all passages (line 529-530), which could be inefficient for users with many memories. This is acceptable for MVP but consider using pagination in the Letta API if performance becomes an issue.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
GETTING_STARTED.mdapp/alembic/versions/20251222_enable_pgvector_extension.pyapp/celery/celery_app.pyapp/celery/tasks/agent_tasks.pyapp/celery/tasks/memory_tasks.pyapp/celery/worker.pyapp/main.pyapp/modules/code_provider/code_provider_service.pyapp/modules/conversations/conversation/conversation_service.pyapp/modules/intelligence/agents/agents_service.pyapp/modules/intelligence/agents/chat_agent.pyapp/modules/intelligence/agents/chat_agents/pydantic_multi_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/debug_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/qna_agent.pyapp/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.pyapp/modules/intelligence/agents/chat_agents/tool_helpers.pyapp/modules/intelligence/agents/custom_agents/custom_agents_service.pyapp/modules/intelligence/agents/custom_agents/runtime_agent.pyapp/modules/intelligence/memory/letta_service.pyapp/modules/intelligence/memory/memory_interface.pyapp/modules/intelligence/memory/memory_router.pyapp/modules/intelligence/memory/memory_service_factory.pyapp/modules/intelligence/tools/memory_search_tool.pyapp/modules/intelligence/tools/tool_service.pydocker-compose.yamldocs/memory.mdpyproject.toml
✅ Files skipped from review due to trivial changes (1)
- app/modules/intelligence/agents/chat_agents/system_agents/low_level_design_agent.py
🚧 Files skipped from review as they are similar to previous changes (14)
- app/modules/intelligence/agents/chat_agents/system_agents/integration_test_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/unit_test_agent.py
- pyproject.toml
- app/modules/intelligence/tools/tool_service.py
- app/modules/intelligence/agents/custom_agents/custom_agents_service.py
- docker-compose.yaml
- app/celery/tasks/memory_tasks.py
- app/modules/intelligence/agents/chat_agents/system_agents/blast_radius_agent.py
- app/modules/intelligence/agents/chat_agents/system_agents/qna_agent.py
- app/modules/conversations/conversation/conversation_service.py
- app/modules/intelligence/agents/agents_service.py
- app/modules/intelligence/memory/memory_interface.py
- app/main.py
- app/modules/intelligence/agents/chat_agents/system_agents/debug_agent.py
🧰 Additional context used
🧬 Code graph analysis (7)
app/modules/intelligence/memory/memory_service_factory.py (2)
app/modules/intelligence/memory/memory_interface.py (1)
MemoryInterface(21-71)app/modules/intelligence/memory/letta_service.py (1)
LettaService(16-611)
app/modules/intelligence/agents/chat_agents/tool_helpers.py (1)
tests/conftest.py (1)
get(142-150)
app/modules/intelligence/tools/memory_search_tool.py (3)
app/modules/intelligence/memory/memory_service_factory.py (2)
MemoryServiceFactory(10-43)create(14-30)app/modules/intelligence/memory/letta_service.py (2)
close(609-611)search(366-434)app/modules/intelligence/memory/memory_interface.py (2)
close(69-71)search(25-35)
app/modules/code_provider/code_provider_service.py (4)
app/modules/code_provider/local_repo/local_provider.py (1)
_get_repo(49-63)app/modules/code_provider/gitbucket/gitbucket_provider.py (1)
_get_repo(99-117)app/modules/code_provider/github/github_service.py (1)
get_repo(706-760)app/modules/code_provider/local_repo/local_repo_service.py (1)
get_repo(46-51)
app/celery/worker.py (1)
app/celery/tasks/memory_tasks.py (1)
extract_user_preferences(16-93)
app/modules/intelligence/memory/letta_service.py (3)
app/modules/intelligence/memory/memory_interface.py (8)
MemoryInterface(21-71)MemorySearchResponse(14-18)MemorySearchResult(6-11)add(38-46)search(25-35)get_all_for_user(49-56)delete(59-66)close(69-71)app/modules/intelligence/memory/memory_service_factory.py (1)
create(14-30)tests/conftest.py (1)
get(142-150)
app/celery/tasks/agent_tasks.py (3)
app/celery/tasks/memory_tasks.py (1)
extract_user_preferences(16-93)app/modules/intelligence/memory/chat_history_service.py (2)
ChatHistoryService(23-159)get_session_history(28-66)app/modules/conversations/conversation/conversation_model.py (1)
Conversation(23-59)
🪛 markdownlint-cli2 (0.18.1)
GETTING_STARTED.md
86-86: Bare URL used
(MD034, no-bare-urls)
docs/memory.md
86-86: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.10)
app/modules/intelligence/tools/memory_search_tool.py
91-91: Do not catch blind exception: Exception
(BLE001)
101-103: try-except-pass detected, consider logging the exception
(S110)
101-101: Do not catch blind exception: Exception
(BLE001)
199-199: Use explicit conversion flag
Replace with conversion flag
(RUF010)
239-239: Use explicit conversion flag
Replace with conversion flag
(RUF010)
264-266: Avoid specifying long messages outside the exception class
(TRY003)
app/modules/intelligence/memory/memory_router.py
21-21: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-39: Abstract raise to an inner function
(TRY301)
67-69: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
68-68: Use explicit conversion flag
Replace with conversion flag
(RUF010)
78-81: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
82-82: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
97-99: Abstract raise to an inner function
(TRY301)
123-125: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
124-124: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/modules/code_provider/code_provider_service.py
59-59: Do not catch blind exception: Exception
(BLE001)
app/modules/intelligence/memory/letta_service.py
141-141: Consider moving this statement to an else block
(TRY300)
263-263: Do not catch blind exception: Exception
(BLE001)
360-360: Consider moving this statement to an else block
(TRY300)
479-479: Consider moving this statement to an else block
(TRY300)
566-566: Do not catch blind exception: Exception
(BLE001)
595-595: Do not catch blind exception: Exception
(BLE001)
app/celery/celery_app.py
146-146: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
app/celery/tasks/agent_tasks.py
250-250: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (37)
app/alembic/versions/20251222_enable_pgvector_extension.py (2)
11-15: LGTM!The revision metadata is correctly formatted and follows the established naming convention. The revision ID issue from the previous review has been properly addressed.
18-20: LGTM!The upgrade implementation correctly enables the pgvector extension with proper idempotency using
IF NOT EXISTS.app/modules/intelligence/agents/chat_agent.py (1)
54-54: No actionable changes needed. Theuser_idfield addition is well-integrated throughout the system. ChatContext instances are consistently populated with user_id where memory features are invoked, memory operations properly handle user identification through the authenticated request context, and user_id logging follows appropriate operational patterns without PII concerns.app/modules/code_provider/code_provider_service.py (3)
43-66: Change appears unrelated to PR objectives.The PR objectives describe implementing "Long term memory with letta" (memory system, vector embeddings, Letta integration), but this change implements language retrieval for repositories. This appears to be an incidental fix that should either be mentioned in the summary or moved to a separate PR for clarity.
59-66: Exception handling pattern is appropriate here.While the static analysis tool flags the bare
Exceptioncatch, this pattern is reasonable in this context because:
- Multiple provider types can raise different exception types (ValueError, FileNotFoundError, GitHub API exceptions, etc.)
- The method gracefully degrades by returning
{}as a safe default- Detailed logging captures the failure context (repository, provider class, error message)
The broad exception handling ensures robustness across different provider implementations.
43-66: Implementation correctly handles provider-driven language retrieval with safe lowercase normalization.The implementation:
- Handles provider absence gracefully
- Checks for both
_get_repoandclient.get_repopatterns appropriately- Normalizes language keys to lowercase for consistent downstream processing
- Logs failures with sufficient context
Downstream consumers (parsing_service.py and parsing_helper.py) are compatible with lowercase normalization—parsing_service already applies
.lower()to its result, and parsing_helper only sums values.app/modules/intelligence/agents/custom_agents/runtime_agent.py (1)
148-161: LGTM! Memory guidance enhances agent capabilities.The prompt additions provide clear instructions for memory tool usage, including query patterns and application guidance. This aligns well with the new
search_user_memoriestool introduced in the PR.GETTING_STARTED.md (1)
21-24: LGTM! Environment variables properly documented.The Letta-related environment variables are well-documented with sensible defaults that align with the docker-compose setup.
app/modules/intelligence/memory/letta_service.py (10)
17-37: LGTM! Clean initialization with sensible defaults.The initialization logic properly handles optional configuration and defaults to environment-driven values. The in-memory agent cache is appropriate for performance optimization.
39-86: LGTM! Comprehensive extraction prompt with clear examples.The fact extraction prompt is well-designed with clear guidelines, good examples, and proper date handling for time-based events.
88-145: LGTM! Robust agent management with proper caching.The agent creation and caching logic is well-implemented with proper async handling of blocking Letta client calls. The naming convention is clear and the error handling is appropriate.
Note: The static analysis hint (TRY300) about moving the return to an else block is a style preference and doesn't affect correctness.
147-231: LGTM! Heuristic-based categorization provides good baseline.The memory categorization logic uses reasonable heuristics to determine scope (user vs project) and type (episodic vs semantic). While keyword-based classification may not be perfect, it provides a solid foundation that can be refined over time based on real-world usage.
233-271: LGTM! Robust LLM-based fact extraction.The fact extraction implementation properly uses the configured model and handles errors gracefully. The use of
json_repairfor parsing LLM responses is a good practice given LLM output variability.Note: The static analysis warnings about catching broad exceptions are false positives here - the error handling is appropriate for this use case.
273-364: LGTM! Well-structured memory addition with proper routing.The memory addition logic correctly extracts facts, categorizes them, and routes them to the appropriate agent (user-level or project-level) based on scope. The metadata tracking is comprehensive and the error handling is appropriate.
366-434: LGTM! Comprehensive search with deduplication and filtering.The search implementation correctly queries both user-level and project-level agents as needed, deduplicates results, and supports optional filtering by memory type. The error handling provides a safe fallback.
436-483: LGTM! Helper method provides clean search abstraction.The helper method properly encapsulates agent-specific search logic and ensures consistent metadata through re-categorization. The error handling is appropriate.
540-607: LGTM! Robust deletion with partial success handling.The deletion logic properly handles both specific passage deletion and bulk deletion, with resilient error handling that allows partial success. The lambda captures are correct to avoid closure issues in the executor.
Note: The static analysis warnings about catching broad exceptions are false positives - the error handling allows the loop to continue despite individual failures, which is the correct behavior.
609-611: LGTM! Simple and effective resource cleanup.The close method appropriately clears the agent cache. The Letta client doesn't appear to require explicit cleanup based on its usage pattern.
app/celery/worker.py (1)
14-16: LGTM! Task registration follows established pattern.The memory task import and registration follow the same pattern as existing tasks, ensuring consistent integration with the Celery worker.
Also applies to: 34-35
app/celery/celery_app.py (1)
100-102: LGTM! Task routing consistent with agent tasks.The memory task is correctly routed to the agent_tasks queue, which is appropriate since the worker is already listening to this queue and memory extraction is conceptually related to agent operations.
app/modules/intelligence/agents/chat_agents/system_agents/code_gen_agent.py (2)
94-94: LGTM! Memory tool integration enhances code generation.Adding the
search_user_memoriestool enables the code generation agent to access user preferences and past decisions, improving code consistency with user style.
163-177: LGTM! Comprehensive memory usage guidance for code generation.The prompt additions provide clear, actionable guidance for memory-aware code generation. The STEP 0 approach ensures the agent searches for relevant preferences before writing code, which will help maintain consistency with user coding style and project patterns.
Also applies to: 195-195
app/modules/intelligence/agents/chat_agents/system_agents/general_purpose_agent.py (2)
78-78: LGTM! Memory tool enables personalized responses.Adding the
search_user_memoriestool to the general purpose agent enables it to provide more personalized and context-aware responses based on user history.
134-142: LGTM! Clear memory usage guidance for general queries.The prompt additions provide concise guidance for memory-aware interactions, encouraging the agent to personalize responses based on discovered user preferences and past interactions.
app/celery/tasks/agent_tasks.py (1)
179-256: LGTM! Memory extraction trigger is well-implemented.The post-completion memory extraction is properly isolated from the main agent task with:
- Fire-and-forget pattern via
.delay()ensures main task isn't blocked- Defensive error handling prevents extraction failures from affecting agent completion
- Dynamic imports scope dependencies appropriately
- Safe metadata extraction with fallbacks for missing conversation data
app/modules/intelligence/memory/memory_service_factory.py (1)
10-43: LGTM! Clean factory implementation.The factory pattern is well-implemented with:
- Clear separation of concerns between factory and service
- Appropriate logging for default config usage
- Sensible defaults for LETTA_MODEL and LETTA_EMBEDDING
- Warning when OPENAI_API_KEY is missing
app/modules/intelligence/agents/chat_agents/tool_helpers.py (1)
131-132: LGTM! Consistent integration of search_user_memories tool.The new memory search tool is properly integrated across all tool helper functions with:
- Clear user-facing messages for call and response events
- Detailed filter information in call details
- Direct content pass-through for result display
One minor type safety issue noted separately for line 504.
Also applies to: 247-248, 495-513, 706-710
app/modules/intelligence/agents/chat_agents/pydantic_multi_agent.py (3)
311-340: LGTM! Clear memory access guidance for delegate agents.The updated instructions effectively communicate:
- Proactive memory usage throughout execution
- Specific use cases for searching memories
- When and how to query memories for context
- Query examples that align with the tool's capabilities
435-453: LGTM! Delegation prompts now emphasize memory context.The delegation prompt properly:
- Reminds subagents to check memories before style/architecture decisions
- Integrates memory guidance into the critical guidelines
- Maintains focus on task execution while adding memory awareness
782-868: LGTM! Comprehensive memory integration in supervisor prompts.The supervisor instructions effectively integrate memory awareness by:
- Emphasizing proactive memory searches throughout workflow
- Including memory checks in the mandatory planning phase (line 805)
- Providing clear guidance on what can be found in memories
- Encouraging memory queries before making decisions
- Giving specific query examples for different scenarios
The integration feels natural and reinforces the value of the memory system.
app/modules/intelligence/memory/memory_router.py (2)
12-69: LGTM! GET endpoint is properly secured.The endpoint correctly:
- Requires authentication via
Depends(AuthService.check_auth)- Validates that
user_idmatches the authenticated user (lines 34-39)- Returns 403 for unauthorized access attempts
- Ensures memory service cleanup in finally block
72-125: LGTM! DELETE endpoint follows the same security pattern.Consistent implementation with GET endpoint:
- Same authentication and authorization checks
- Proper resource cleanup
- Clear error handling and logging
app/modules/intelligence/tools/memory_search_tool.py (5)
71-104: LGTM! Proper resource lifecycle management.The implementation includes multiple cleanup mechanisms:
- Context manager support (
__enter__/__exit__)- Explicit
close()method- Fallback
__del__cleanup for garbage collection_closedflag prevents double-cleanupThe
__del__method appropriately suppresses exceptions during cleanup (line 101-103), which is correct for garbage collection scenarios.
162-199: LGTM! Async implementation avoids nested event loops.The
arun()method correctly calls_async_search()directly without creating nested event loops, addressing the concern from previous reviews.
201-239: LGTM! Proper sync-to-async bridge.The
run()method correctly usesasyncio.run()to execute async code from a synchronous context. This is the standard pattern for sync-async bridging.
241-284: LGTM! Well-structured search strategy.The
_async_search()method properly handles different scope scenarios:
- User-only:
project_id=None,include_user_preferences=True- Project-only: requires
project_id,include_user_preferences=False- Both: uses
project_id,include_user_preferences=TrueClear error for missing
project_idwhen required (lines 264-266).
287-329: Factory pattern relies on garbage collection for cleanup.The factory creates a
MemorySearchToolinstance and passes its methods toStructuredTool.from_function(), but doesn't explicitly manage the instance lifecycle. While the class has proper cleanup mechanisms (__del__, context manager), relying on garbage collection is less deterministic.However, this pattern is acceptable because:
- The tool has multiple cleanup mechanisms as fallbacks
- StructuredTool.from_function() is the standard LangChain pattern
- The resource (memory_service) will eventually be cleaned up
Consider documenting this lifecycle dependency if it becomes problematic in production.
| # Memory System Setup | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| **⚠️ Important**: The memory system requires the **Letta server** to be running, which needs an **OpenAI API key** in your environment. | ||
|
|
||
| Before using the memory features, ensure: | ||
| 1. `OPENAI_API_KEY` is set in your environment | ||
| 2. Letta server is running (starts automatically with `docker-compose up`) | ||
|
|
||
| ## Quick Start | ||
|
|
||
| ### 1. Set Environment Variables | ||
|
|
||
| ```bash | ||
| export OPENAI_API_KEY=your_openai_api_key_here | ||
| ``` | ||
|
|
||
| ### 2. Start Letta Server | ||
|
|
||
| The Letta server runs as a Docker container. Start it with: | ||
|
|
||
| ```bash | ||
| docker-compose up -d letta | ||
| ``` | ||
|
|
||
| Or start all services: | ||
|
|
||
| ```bash | ||
| docker-compose up -d | ||
| ``` | ||
|
|
||
| ### 3. Verify Letta is Running | ||
|
|
||
| ```bash | ||
| # Check if the server is responding | ||
| curl http://localhost:8283/ | ||
|
|
||
| # Or check container logs | ||
| docker logs potpie-letta | ||
| ``` | ||
|
|
||
| The Letta web UI is available at **http://localhost:8283** for viewing agents and memories. | ||
|
|
||
| ## Configuration | ||
|
|
||
| Memory system configuration via environment variables: | ||
|
|
||
| | Variable | Description | Default | | ||
| |----------|-------------|---------| | ||
| | `OPENAI_API_KEY` | **Required** - OpenAI API key for LLM and embeddings | - | | ||
| | `LETTA_SERVER_URL` | Letta server URL | `http://localhost:8283` | | ||
| | `LETTA_MODEL` | LLM model in `provider/model` format | `openai/gpt-4o-mini` | | ||
| | `LETTA_EMBEDDING` | Embedding model | `openai/text-embedding-ada-002` | | ||
|
|
||
| ## How It Works | ||
|
|
||
| Potpie uses **[Letta](https://www.letta.com/)** for stateful agent memory management. The system: | ||
|
|
||
| 1. **Extracts facts** from conversations using LLM-based fact extraction | ||
| 2. **Categorizes memories** into: | ||
| - **User-level**: Personal preferences that persist across all projects | ||
| - **Project-level**: Project-specific information scoped to individual codebases | ||
| - **Memory types**: Semantic (general knowledge) or Episodic (events/actions) | ||
| 3. **Stores passages** in Letta for semantic search | ||
| 4. **Retrieves relevant memories** when needed using embedding-based search | ||
|
|
||
| ## Key Features | ||
|
|
||
| - ✅ **User-level memories**: Personal preferences, coding styles, tool preferences | ||
| - ✅ **Project-level memories**: Tech stack, architecture decisions, project-specific context | ||
| - ✅ **Semantic search**: Intelligent memory retrieval using embeddings | ||
| - ✅ **Automatic fact extraction**: LLM extracts meaningful facts from conversations | ||
| - ✅ **Memory categorization**: Automatically determines scope (user vs project) and type | ||
|
|
||
| ## Architecture | ||
|
|
||
| ``` | ||
| ┌─────────────────┐ | ||
| │ Conversation │ | ||
| │ Service │ | ||
| └────────┬────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────┐ | ||
| │ LettaService │ | ||
| │ (Memory Layer) │ | ||
| └────────┬────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────┐ | ||
| │ Letta Server │ | ||
| │ (Docker) │ | ||
| │ Port: 8283 │ | ||
| └─────────────────┘ | ||
| ``` | ||
|
|
||
| ## API Usage | ||
|
|
||
| The memory system is accessed through: | ||
| - **Memory Router**: `/api/memories` endpoints | ||
| - **Memory Service Factory**: Creates `LettaService` instances | ||
| - **Memory Search Tool**: Integrated into agent toolkits | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| **Letta server not responding?** | ||
| - Check if container is running: `docker ps | grep letta` | ||
| - Check logs: `docker logs potpie-letta` | ||
| - Verify `OPENAI_API_KEY` is set in the container environment | ||
|
|
||
| **Memory not being stored?** | ||
| - Ensure Letta server is healthy (check healthcheck) | ||
| - Verify OpenAI API key is valid | ||
| - Check application logs for memory service errors | ||
|
|
||
| **Can't connect to Letta?** | ||
| - Verify `LETTA_SERVER_URL` matches the running container | ||
| - Check network connectivity between services | ||
| - Ensure port 8283 is accessible | ||
|
|
||
| ## Related Files | ||
|
|
||
| - **Service Implementation**: `app/modules/intelligence/memory/letta_service.py` | ||
| - **Service Factory**: `app/modules/intelligence/memory/memory_service_factory.py` | ||
| - **API Router**: `app/modules/intelligence/memory/memory_router.py` | ||
| - **Docker Config**: `docker-compose.yaml` (letta service) |
There was a problem hiding this comment.
Excellent documentation with minor formatting improvement.
The memory system documentation is comprehensive, well-structured, and provides clear guidance for setup, configuration, and troubleshooting.
One minor improvement: Line 43 contains a bare URL that could be formatted as a markdown link for consistency with best practices.
🔎 Proposed fix for line 43
-The Letta web UI is available at **http://localhost:8283** for viewing agents and memories.
+The Letta web UI is available at **[http://localhost:8283](http://localhost:8283)** for viewing agents and memories.🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
86-86: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In docs/memory.md around line 43, the bare URL "https://www.letta.com/" should
be formatted as a Markdown link for consistency; replace the plain URL with a
markdown-style link (e.g., [Letta](https://www.letta.com/)) in the "Potpie uses
Letta" sentence or wherever the URL appears so it matches the surrounding docs
formatting.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.