fix: resolve type signature incompatibility in LongTermMemory #4219
Conversation
- Update save() and asave() to match parent Memory class signature - Update search() and asearch() to match parent Memory class signature - Maintain backward compatibility with LongTermMemoryItem - Remove all type: ignore comments for proper type safety The methods now properly follow Liskov Substitution Principle by matching the parent class signatures while maintaining backward compatibility through isinstance checks for LongTermMemoryItem. Fixes crewAIInc#4213
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 28
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
- Accept both old (task, latest_n) and new (query, limit) parameter names - Support keyword arguments for backward compatibility - Fixes issue raised by Cursor bot review The previous implementation only renamed local variables, which didn't support keyword arguments using old parameter names. This would break existing code calling methods like: memory.search(task='find', latest_n=5) Now both old and new keyword arguments work properly.
| agent=self.agent.role if self.agent else None, | ||
| expected_output="", | ||
| datetime=time.strftime("%Y-%m-%d %H:%M:%S"), | ||
| ) |
There was a problem hiding this comment.
None passed to LongTermMemoryItem agent parameter expecting str
Medium Severity
When save() or asave() receives a non-LongTermMemoryItem value, the code constructs a new LongTermMemoryItem with agent=self.agent.role if self.agent else None. However, LongTermMemoryItem.__init__ expects agent to be of type str, not str | None. When self.agent is None, this passes None to a parameter that expects a string, violating the type contract. This could cause downstream issues where code assumes item.agent is always a string.
Additional Locations (1)
| if task is not None: | ||
| query = task | ||
| if latest_n is not None: | ||
| limit = latest_n |
There was a problem hiding this comment.
Deprecated parameter silently overrides new parameter when both provided
Low Severity
The backward compatibility logic in search() and asearch() unconditionally overwrites query with task when task is not None, even if query was already provided. If someone calls search(query="new", task="old"), the deprecated task parameter silently takes precedence over the new query parameter. The logic should only use task as a fallback when query is None, e.g., if query is None and task is not None. Same issue affects limit and latest_n.
Additional Locations (1)
1 participant
Closes #4213
What's wrong?
LongTermMemory wasn't playing nice with its parent class Memory. The method signatures didn't match, so there were a bunch of
# type: ignorecomments scattered around to silence mypy. Not great.The parent class expected:
save(self, value: Any, metadata: dict | None = None)
search(self, query: str, limit: int = 5, score_threshold: float = 0.6)But LongTermMemory was doing its own thing:
save(self, item: LongTermMemoryItem) # type: ignore
search(self, task: str, latest_n: int = 3) # type: ignore### The fix
Made LongTermMemory match the parent signatures while keeping everything backward compatible:
def save(self, value: Any | LongTermMemoryItem, metadata: dict | None = None):
# If it's the old LongTermMemoryItem, use it directly
if isinstance(value, LongTermMemoryItem):
item = value
else:
# Convert to LongTermMemoryItem
item = LongTermMemoryItem(...)Same approach for
save(),asave(),search(), andasearch().What changed
# type: ignorecommentsBackward compatible
Old code still works:
item = LongTermMemoryItem(...)
memory.save(item) # Still works!New way also works:
memory.save("task description", metadata={...})### No breaking changes
Type safety is back, inheritance is proper, and all existing code keeps working.
Note
Aligns
LongTermMemorywith baseMemorysignatures while preserving backward compatibility.save/asaveto acceptvalue: Any | LongTermMemoryItemplus optionalmetadata; auto-wraps non-item values intoLongTermMemoryItem.search/asearchto usequery/limit(with deprecatedtask/latest_naliases), adds input validation and defaultlimit=3.scoreviametadata.get("quality", 0.0)to avoid KeyError.Written by Cursor Bugbot for commit 23b36e3. This will update automatically on new commits. Configure here.