feat(mnemosyne): implement forget functionality across subsystems#480
feat(mnemosyne): implement forget functionality across subsystems#480Theory903 wants to merge 1 commit into
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthrough
ChangesMemorySystem.forget() — input normalization and type-based delete counting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ippoc/mnemosyne/core.py (1)
323-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply strict type validation for
ids/skills/entitiesbefore deletion loops.Line 323, Line 338, and Line 353 still allow non-string iterables to flow into deletion paths; malformed criteria (like dict/set/mixed types) can be iterated and delete unintended records.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ippoc/mnemosyne/core.py` around lines 323 - 357, Add strict type validation after the string-to-list conversions for semantic_ids, skills, and entities to prevent malformed criteria from causing unintended deletions. After each isinstance(X, str) check and conversion (around lines where semantic_ids, skills, and entities are processed), validate that the resulting variable is a list containing only string elements, and either raise an exception or skip the deletion if the validation fails.infra/src/mnemosyne/core.py (1)
323-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate criteria collection types before iterating destructive deletes.
Line 323, Line 338, and Line 353 only special-case
str; other iterables (for exampledict,set, or mixed lists) are accepted and iterated, which can delete unintended IDs/skills/entities by accident.🔧 Suggested hardening
if "ids" in semantic_criteria: semantic_ids = semantic_criteria["ids"] if isinstance(semantic_ids, str): semantic_ids = [semantic_ids] + elif not isinstance(semantic_ids, list) or any(not isinstance(x, str) for x in semantic_ids): + raise ValueError("criteria['semantic']['ids'] must be str or list[str]") if semantic_ids: res = await self.semantic.delete_memories(semantic_ids) if res: total_deleted += len(semantic_ids) if type(res) is bool else (res if type(res) is int else 1) @@ if "skills" in proc_criteria: skills = proc_criteria["skills"] if isinstance(skills, str): skills = [skills] + elif not isinstance(skills, list) or any(not isinstance(x, str) for x in skills): + raise ValueError("criteria['procedural']['skills'] must be str or list[str]") for skill_name in skills: res = await self.procedural.delete_skill(skill_name) if res: total_deleted += 1 if type(res) is bool else (res if type(res) is int else 1) @@ if "entities" in graph_criteria: entities = graph_criteria["entities"] if isinstance(entities, str): entities = [entities] + elif not isinstance(entities, list) or any(not isinstance(x, str) for x in entities): + raise ValueError("criteria['graph']['entities'] must be str or list[str]") for entity in entities: count = await self.graph.delete_entity(entity) total_deleted += count if type(count) is int else (1 if count else 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/src/mnemosyne/core.py` around lines 323 - 357, The semantic_ids, skills, and entities parameters lack type validation before iteration, allowing unintended iterable types like dicts or sets to be processed unsafely. For each of the three deletion blocks (semantic, procedural, and graph deletion sections), replace the simple string type check with proper validation that accepts only strings (which get converted to single-element lists) or list/tuple types. Add explicit type checking after the string conversion to ensure the resulting value is a list or tuple before iterating, and either raise a clear validation error or skip processing if an unexpected iterable type is encountered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@infra/src/mnemosyne/core.py`:
- Around line 323-357: The semantic_ids, skills, and entities parameters lack
type validation before iteration, allowing unintended iterable types like dicts
or sets to be processed unsafely. For each of the three deletion blocks
(semantic, procedural, and graph deletion sections), replace the simple string
type check with proper validation that accepts only strings (which get converted
to single-element lists) or list/tuple types. Add explicit type checking after
the string conversion to ensure the resulting value is a list or tuple before
iterating, and either raise a clear validation error or skip processing if an
unexpected iterable type is encountered.
In `@src/ippoc/mnemosyne/core.py`:
- Around line 323-357: Add strict type validation after the string-to-list
conversions for semantic_ids, skills, and entities to prevent malformed criteria
from causing unintended deletions. After each isinstance(X, str) check and
conversion (around lines where semantic_ids, skills, and entities are
processed), validate that the resulting variable is a list containing only
string elements, and either raise an exception or skip the deletion if the
validation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92b56c06-d98d-4e34-a13a-fa66f78217e3
📒 Files selected for processing (2)
infra/src/mnemosyne/core.pysrc/ippoc/mnemosyne/core.py
1 participant
💡 What: Implemented the
forgetmethod to orchestrate deleting memories acrossepisodic,semantic,procedural, andgraphsubsystems.🎯 Why: The original
forgetmethod was raisingNotImplementedError, blocking memory deletion functionalities.📊 Impact: The
forgetcapability is now fully operational based on provided memory criteria.🔬 Measurement: Local tests
test_forget_functionality.pyvalidated using mocked systems, confirming the deletion sequence and proper string vs. list (duck-typing) edge cases are correctly evaluated.PR created automatically by Jules for task 10719882569377002438 started by @Theory903
Summary by CodeRabbit