Fix hash-based deduplication FT.AGGREGATE query execution #41
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in hash-based deduplication where the FT.AGGREGATE query was incorrectly executed as a single string, bypassing the FILTER clause and leading to confusing logs about finding N groups but merging 0 memories.
- Changed FT.AGGREGATE execution from string to individual arguments to ensure proper filter clause processing
- Improved logging messages for clarity and accuracy
- Added logging for the case when no duplicates are found
Fixes issue #31 where hash-based deduplication reported finding N groups but merged 0 memories, causing confusing logs. **Root Cause:** - FT.AGGREGATE query was executed as a single string via execute_command() - This bypassed the FILTER "@count>1" clause, returning all groups including unique memories (count=1) - Processing logic correctly skipped count<=1 groups, resulting in 0 merges **Solution:** - Change FT.AGGREGATE execution from single string to individual arguments - Use execute_command(*agg_query) instead of execute_command(agg_query) - This ensures the FILTER clause properly excludes unique memories - Added clearer logging messages **Testing:** - All existing tests pass - Verified fix with integration test showing: - 5 memories (2 duplicates + 3 unique) → 4 memories (1 duplicate removed) - Proper logging: "Found 1 groups with hash-based duplicates to process" - Correctly removes duplicate while preserving unique memories 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
abrookins
force-pushed
the
fix/hash-deduplication-issue-31
branch
from
c7ed894 to
ac6d88b
Compare
…tion work The test was expecting summarization to trigger but the messages were under the 1000 token limit. Increased the message length to ensure summarization happens and made the token count assertion more flexible since it varies based on actual content.
- Replace window_size parameter with model_name in examples and test files - Rename effective_window_size to effective_token_limit in API for clarity - Update all references to use the current token-based API instead of the old message-count based window_size approach
abrookins
commented
Jul 31, 2025
1. Fix string consistency in FT.AGGREGATE query parameters - now all numeric parameters are explicitly converted to strings using str() 2. Handle tool messages and other unknown roles in memory_prompt API - treat non-user/assistant roles as assistant messages for MCP compatibility since MCP base only supports UserMessage and AssistantMessage types
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue #31 where hash-based deduplication reported finding N groups but merged 0 memories, causing confusing logs.
Also removes deprecated
window_sizeparameter. Summarization is now based on token count, so you need to specify a model name (from which the server derives a max token count) or manually specify a max token count in order for us to auto-summarize messages in working memory.Root Cause:
Solution:
Testing:
🤖 Generated with Claude Code