fix(memory): validate merged configuration 🤖🤖🤖 - #279
Conversation
Signed-off-by: Sampoorn Nagpal <nagpalsampoorn@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthrough
ChangesMemory configuration validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Configuration merges now validate and coerce override data while preserving replacement semantics and original configuration immutability. The covered behavior is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Sampoorn Nagpal <nagpalsampoorn@gmail.com>
What does this PR do?
MemoryConfig.merge_with()bypasses Pydantic validation:enabled="false"remains truthy, invalid owners are accepted, and nested embedding dictionaries remain plain dictionaries that fail when used by the manager.Validate the merged data through the configuration class. Preserve the documented top-level replacement behavior and the original frozen configuration.
Validation
All four new cases fail before the fix.
uv run pytest -q packages/nooa-memory/tests/memory/test_memory_config.py packages/nooa-memory/tests/memory/test_memory_skill.py packages/nooa-memory/tests/memory/test_memory_owner_roles.py— 19 passed, including the MemorySkill call site ofmerge_with().Windows/Python 3.12 with external import-only helpers for the existing
fcntl/SIGUSR2blockers (#84/#85). No live model calls; helpers are not included and do not validate locking or signals.Related issues
No matching open issue found.
Checklist
Summary by CodeRabbit
Bug Fixes
Tests