fix: rename deduplication tool to memory_deduplicate to avoid collision#441
fix: rename deduplication tool to memory_deduplicate to avoid collision#441Displayer226 wants to merge 1 commit intoCortexReach:masterfrom
Conversation
Review: REQUEST-CHANGESThe rename approach is the right call — both tools serve different purposes (dedup vs progressive compaction), so they should coexist under distinct names. Closing #433 in favor of this direction. Must fix:
Worth considering:
|
eb1fb62 to
2fdec0e
Compare
|
Hello ! Thanks for the review! I've updated the PR to address all the 'Must fix' items and rebased on the latest master:
The full test suite is now passing locally (npm test). |
- Rename metadata-based deduplication tool to memory_deduplicate to resolve conflict with progressive summarization - Standardize all memory management tools to use camelCase parameters (dryRun, minAgeDays, similarityThreshold) - Set dryRun: true as the safe default for all compaction/deduplication tools - Fix broken memory-governance, plugin-manifest, and session-summary tests - Add explicit verification for standardized tool registration in manifest regression tests - Document breaking changes in CHANGELOG.md
2fdec0e to
2bfccf3
Compare
AliceLJY
left a comment
There was a problem hiding this comment.
LGTM — clean rename + parameter standardization, addresses all prior review feedback.
Verified:
memory_compact(progressive summarization) andmemory_deduplicate(metadata dedup) now coexist with distinct names- Parameters consistently use camelCase across both tools
dryRundefaults totrue(safe preview mode) for both tools- Governance test updated to target
memory_deduplicate - Plugin manifest regression test extended with assertions for both tools' parameter schemas
- CHANGELOG clearly documents the breaking changes
selfImprovement: { enabled: false }additions fix unrelated test regressions from upstream
Minor note (non-blocking): In plugin-manifest-regression.mjs, the updated registerTool mock silently drops calls where toolOrFactory is a function but meta is undefined (neither if branch matches). Original code would have thrown on meta.name. Since this is test-only, low risk, but worth noting in case a future tool registration path hits that gap.
Assigning to @rwmjhb for final merge decision.
Summary
This PR resolves the naming conflict for the
memory_compacttool by renaming the metadata-based deduplication version tomemory_deduplicate.Motivation
Fixes #431.
This is offered as an alternative to PR #433.
While #433 proposes removing one of the implementations, this PR argues that both are essential for a complete memory lifecycle:
memory_deduplicate): A lightweight, metadata-based tool insrc/tools.tsused for structural hygiene (archiving identical or redundant facts).memory_compact): A heavyweight, LLM-based tool inindex.tsused for increasing knowledge density by merging related memories into refined entries.By renaming instead of removing, we preserve both functionalities for the user.
Key Changes
registerMemoryCompactTooltoregisterMemoryDeduplicateToolinsrc/tools.ts.memory_compacttomemory_deduplicate.registerAllMemoryToolsto reflect the new name.Testing
memory_deduplicateconfirmed it correctly identifies and archives duplicate metadata entries.