fix: Resolve #1528 - fix: enforce ALLOWED_UPDATE_FIELDS in update_memory (#770)#1529
fix: Resolve #1528 - fix: enforce ALLOWED_UPDATE_FIELDS in update_memory (#770)#1529washim0988-art wants to merge 2 commits into
Conversation
Signed-off-by: washim0988-art <islowashin@gmail.com>
📝 WalkthroughWalkthroughAdds ChangesMemory update enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@fix_1528.py`:
- Around line 6-18: Update the MemoryRecord reconstruction to preserve the
complete existing record, including required agent_id and fields such as
created_at, expires_at, and ttl_seconds. In the update flow around
updated_memory, normalize memory first and overlay only values from
allowed_updates, while retaining all other existing fields unchanged.
- Line 1: Update the constants import in fix_1528.py to reference the
package-defined ALLOWED_UPDATE_FIELDS from memanto.app.constants using the
project’s established package-qualified or relative import style, rather than
the top-level constants module.
- Line 4: Update the filtering logic around allowed_updates to identify fields
present in updates but absent from ALLOWED_UPDATE_FIELDS, and emit a warning
listing only those dropped field names before proceeding with the filtered
update. Preserve the existing whitelist behavior and never include removed field
values in the log.
- Around line 3-5: Update update_memory to use the MemoryReadService.get_memory
contract by supplying the required namespace, using the appropriate service
access, and handling a None result before dereferencing memory. Preserve the
allowed_updates filtering and return the established not-found outcome when the
memory record is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| def update_memory(memory_id, updates): | ||
| allowed_updates = {key: value for key, value in updates.items() if key in ALLOWED_UPDATE_FIELDS} | ||
| memory = get_memory(memory_id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Align with the get_memory contract and handle missing records.
MemoryReadService.get_memory requires (memory_id, namespace) and returns a dictionary or None. This helper has no namespace, calls an undefined one-argument get_memory, and dereferences the result without a not-found check.
🤖 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 `@fix_1528.py` around lines 3 - 5, Update update_memory to use the
MemoryReadService.get_memory contract by supplying the required namespace, using
the appropriate service access, and handling a None result before dereferencing
memory. Preserve the allowed_updates filtering and return the established
not-found outcome when the memory record is absent.
Refactor update_memory function to include namespace handling, logging for dropped fields, and reconstruct the MemoryRecord from existing data.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
fix_1528.py (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a module-level logger.
Since it is recommended to remove the global
logging.basicConfig()at the module level, consider using a module-specific logger here instead of the root logger.♻️ Proposed refactor
if dropped_fields: - logging.warning(f"Update dropped protected fields: {', '.join(dropped_fields)}") + logging.getLogger(__name__).warning(f"Update dropped protected fields: {', '.join(dropped_fields)}")🤖 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 `@fix_1528.py` around lines 17 - 18, Replace the root logger call in the dropped-fields warning block with a module-level logger, defining or reusing the module-specific logger consistently in fix_1528.py. Keep the existing warning message and conditional behavior unchanged.
🤖 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.
Inline comments:
In `@fix_1528.py`:
- Around line 1-5: Add the missing MemoryRecord import and resolve get_memory
through its properly scoped MemoryReadService API before update_memory uses
them. Update update_memory or its callers to provide the service instance as
needed, while preserving the existing memory update behavior and avoiding an
undefined global get_memory reference.
- Around line 25-30: Update the memory conversion block to handle the dictionary
returned by get_memory directly, copying its contents into existing_data before
the Pydantic and standard-object fallbacks. Avoid calling vars(memory) when
memory is a dictionary, while preserving the existing model_dump and dict
compatibility paths.
- Line 4: Remove the module-level logging.basicConfig() call from fix_1528.py,
leaving logging configuration to the application's entry point while preserving
the module's other logging behavior.
---
Nitpick comments:
In `@fix_1528.py`:
- Around line 17-18: Replace the root logger call in the dropped-fields warning
block with a module-level logger, defining or reusing the module-specific logger
consistently in fix_1528.py. Keep the existing warning message and conditional
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| import logging | ||
| from memanto.app.constants import ALLOWED_UPDATE_FIELDS | ||
|
|
||
| logging.basicConfig(level=logging.INFO) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add missing imports for MemoryRecord and get_memory.
MemoryRecord and get_memory are used in the update_memory function but are not imported in this file. This will cause a NameError at runtime. Note that get_memory appears to be a method of MemoryReadService according to the provided context, so you may need to either instantiate the service, pass the service as an argument, or import a properly scoped wrapper function.
🤖 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 `@fix_1528.py` around lines 1 - 5, Add the missing MemoryRecord import and
resolve get_memory through its properly scoped MemoryReadService API before
update_memory uses them. Update update_memory or its callers to provide the
service instance as needed, while preserving the existing memory update behavior
and avoiding an undefined global get_memory reference.
| import logging | ||
| from memanto.app.constants import ALLOWED_UPDATE_FIELDS | ||
|
|
||
| logging.basicConfig(level=logging.INFO) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove module-level logging.basicConfig().
Calling logging.basicConfig() at the module level is an anti-pattern as it forces a global logging configuration on the entire application when this module is imported. The application's entry point should be responsible for configuring logging.
♻️ Proposed fix
-logging.basicConfig(level=logging.INFO)
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logging.basicConfig(level=logging.INFO) |
🤖 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 `@fix_1528.py` at line 4, Remove the module-level logging.basicConfig() call
from fix_1528.py, leaving logging configuration to the application's entry point
while preserving the module's other logging behavior.
| if hasattr(memory, 'model_dump'): # Pydantic v2 | ||
| existing_data = memory.model_dump() | ||
| elif hasattr(memory, 'dict'): # Pydantic v1 | ||
| existing_data = memory.dict() | ||
| else: # Standard object | ||
| existing_data = vars(memory).copy() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Handle dict return type from get_memory directly to avoid TypeError.
According to the get_memory contract, the returned memory object is a dictionary, not a Pydantic model or an object with a __dict__ attribute.
Calling vars(memory) on a dictionary will raise a TypeError: vars() argument must have __dict__ attribute.
Since memory is already a dictionary, you can safely simplify this block by handling dictionaries directly.
🐛 Proposed fix
- if hasattr(memory, 'model_dump'): # Pydantic v2
- existing_data = memory.model_dump()
- elif hasattr(memory, 'dict'): # Pydantic v1
- existing_data = memory.dict()
- else: # Standard object
- existing_data = vars(memory).copy()
+ if isinstance(memory, dict):
+ existing_data = memory.copy()
+ elif hasattr(memory, 'model_dump'): # Pydantic v2
+ existing_data = memory.model_dump()
+ elif hasattr(memory, 'dict'): # Pydantic v1
+ existing_data = memory.dict()
+ else: # Standard object
+ existing_data = vars(memory).copy()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hasattr(memory, 'model_dump'): # Pydantic v2 | |
| existing_data = memory.model_dump() | |
| elif hasattr(memory, 'dict'): # Pydantic v1 | |
| existing_data = memory.dict() | |
| else: # Standard object | |
| existing_data = vars(memory).copy() | |
| if isinstance(memory, dict): | |
| existing_data = memory.copy() | |
| elif hasattr(memory, 'model_dump'): # Pydantic v2 | |
| existing_data = memory.model_dump() | |
| elif hasattr(memory, 'dict'): # Pydantic v1 | |
| existing_data = memory.dict() | |
| else: # Standard object | |
| existing_data = vars(memory).copy() |
🤖 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 `@fix_1528.py` around lines 25 - 30, Update the memory conversion block to
handle the dictionary returned by get_memory directly, copying its contents into
existing_data before the Pydantic and standard-object fallbacks. Avoid calling
vars(memory) when memory is a dictionary, while preserving the existing
model_dump and dict compatibility paths.
Signed-off-by: washim0988-art <islowashin@gmail.com>
|
Thanks for your contributions! We're closing this PR as this is a duplicate. |
2 participants
Resolves #1528
Built autonomously by Cloud Agent.
Bounty payout address: Gq46qirFLJY3qptAWkAmAeDfGVAE4MYYGTcRmpKjsyR
Summary by CodeRabbit