Skip to content

Conversation

@Vidushi2709
Copy link

What Changed

Resolved TODO at line 2641 by implementing intelligent context retrieval for auto-ingest mode.

issue -> #119

Implementation

  • Core Fix: get_auto_ingest_system_prompt() now uses user_input to search for relevant memories
  • Search: Calls db_manager.search_memories() with user query for intelligent retrieval
  • Fallback: Gracefully falls back to conscious context when search fails/empty
  • Error Handling: Comprehensive exception handling with debug logging
  • Linter Fix: Resolved litellm import warning

Tests

Added 7 comprehensive test cases - All passing (7/7):
✓ Happy path with search results
✓ Empty search triggers fallback
✓ Deduplication works correctly
✓ Exception handling
✓ Empty input handling
✓ 5-item limit enforced
✓ Query parameters validated

Impact

Before: Auto-ingest ignored user_input → generic recent memories
After: Auto-ingest performs intelligent search → relevant, context-aware memories

Backward Compatibility

✅ Fully backward compatible - fallback behavior preserved

@harshalmore31
Copy link
Collaborator

@Vidushi2709 Thank you for tackling this TODO! However, I need to point out that this functionality already exists in the
codebase.

The Issue

The intelligent context retrieval you've implemented here is already handled by _get_auto_ingest_context()
(line 1164 in memory.py), which:

  • Searches the database using user_input (line 1204)
  • Has fallback logic to recent memories (line 1276)
  • Includes error handling and recursion guards (line 1178)
  • Is already called during auto-ingest mode context injection (line 817-822)

Your PR modifies get_auto_ingest_system_prompt(), which is a formatting function. The actual retrieval
happens upstream in _get_auto_ingest_context(). if merged will cause double database queries !

You can update your PR :

  • The TODO at line 2641 is misleading and should simply be deleted - the feature is already implemented in
    _get_auto_ingest_context().
  • Your tests are well-written though! They could be adapted to test the existing _get_auto_ingest_context()
    function instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants