feat: extract facts with main agent LLM, bypass mem0 GPT-nano #15
Reference in New Issue
Block a user
Delete Branch "feat/mem0-extract-llm"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Cherry-picked from feat/mem0-haiku-oauth onto a clean nanobot-owned branch.
Uses the main agent's existing LLM provider to extract facts from conversations, then stores them with infer=False — bypassing mem0's default GPT-nano call.
No new files, no Dockerfile changes, no mem0 package patches.
77e54b6bc6to9e8c910ab1Code Review: Critical Issues Found ❌
Summary
This PR is BROKEN and cannot be merged. The code references undefined methods and will cause runtime failures.
Critical Issues
1. Missing Method Implementations 🔴
The code calls two methods that are never defined:
await self.extract_facts(mem0_messages, provider, model)(line ~61)self.store_facts(facts, user_id=user_id, session_id=session.key)(line ~62)These methods don't exist anywhere in the file. Runtime will fail immediately with
AttributeError.2. Incomplete Class Structure 🔴
The modified file is only 80 lines vs 431 lines on main. Critical deletions include:
search_memories()- semantic search (deleted, no replacement)get_memory_context()- system prompt integration (deleted, breaks agent loop)add_conversation()- message ingestion (deleted)update_memory(),delete_memory(),get_all_memories()- all CRUD operations deletedconsolidate()method - file shows orphaned code fragments without proper method definition3. Broken Code Structure 🔴
Lines 61-80 show code fragments not inside any method:
4. Incomplete Cherry-Pick 🟡
The commit message describes implementing
extract_facts()andstore_facts(), but these methods were never included in the cherry-picked commit. Likely the originalfeat/mem0-haiku-oauthbranch had these in a separate commit that wasn't picked.What the PR Should Do
According to the description (and it's a good architectural idea):
infer=Falseto bypass mem0's extractionThe concept is sound, but the implementation is incomplete.
Required Fixes
To make this PR viable:
Implement
extract_facts()method:Implement
store_facts()method:Restore complete
consolidate()method with proper structureHandle deleted methods:
search_memories(),get_memory_context(), etc., update all calling code inagent/loop.pyRecommendation
DO NOT MERGE. Options:
feat/mem0-extract-llmbranch with missing implementationsfeat/mem0-haiku-oauthfor missing commits and cherry-pick properlyImpact if merged: Immediate runtime failure. The nanobot agent will crash on first memory consolidation attempt.
Code Review
Implementation Overview
The PR successfully implements the described feature:
extract_facts(): Uses agent's LLM to extract facts from conversationstore_facts(): Stores withinfer=Falseto bypass mem0's GPT-nanoconsolidate(): Integrated to use new extraction flowCritical Issues
1. Dead Code - Custom Prompt in MemoryConfig 🔴
Lines 60-67: The code still sets
custom_fact_extraction_promptin MemoryConfig:Problem: This prompt is never used. The new flow calls
provider.chat()directly withself.custom_prompt, completely bypassing mem0's extraction system.Fix: Remove this dead code:
2. JSON Parsing Bug 🔴
Lines 184-188: Missing
.strip()will cause parse failures:When it breaks: LLM returns:
3. No Response Validation 🟡
Line 189:
data.get("facts", [])assumes the JSON is well-formed.What if:
{"facts": "not a list"}→ returns string, not list → runtime error later{"thoughts": "...", "facts": [...]}→ works, but wastes tokens[]Fix:
Quality Concerns
4. Prompt May Be Too Minimal 🟡
The old prompt was 100 lines with:
New prompt:
Risk: Without guardrails, the LLM may:
Recommendation: At minimum, add:
5. Message Content Handling 🟢
Lines 166-169:
extract_facts()only processes string content:This is actually fine because
consolidate()normalizes list content to strings before callingextract_facts()(lines 360-380). Messages with tool_use blocks are already flattened.Performance/Economics
6. API Call Impact 🟡
Old flow:
New flow:
extract_facts()(1 additional API call)Question: Does this actually save costs, or just shift them?
With
max_tokens=2000per extraction, frequent consolidations could burn quota faster than the old approach.Testing Requirements
Before merge:
.strip())isinstance(facts, list)){"facts": "string"}instead of listVerdict
HOLD - Implementation logic is sound, but has bugs that will cause runtime failures:
.strip())Once bugs are fixed, this is a reasonable approach if prompt quality holds up in practice.
✅ Approved
All critical issues have been fixed in commit
790bdd6:✅ Dead code removed (custom_fact_extraction_prompt in MemoryConfig)
✅ JSON parsing bug fixed (added
.strip())✅ Facts validation added (
isinstance(facts, list)check)The implementation is now solid and won't cause runtime failures.
Remaining items are monitoring tasks:
Ready to merge.