Skip to content

fix: Resolve #1528 - fix: enforce ALLOWED_UPDATE_FIELDS in update_memory (#770) #1529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
washim0988-art wants to merge 2 commits into from
Closed

fix: Resolve #1528 - fix: enforce ALLOWED_UPDATE_FIELDS in update_memory (#770) #1529

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d63cc3
fix: Resolve #1528
washim0988-art Jul 19, 2026
1c06a61
Enhance update_memory with logging and namespace support
washim0988-art Jul 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions fix_1528.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import logging
from memanto.app.constants import ALLOWED_UPDATE_FIELDS

logging.basicConfig(level=logging.INFO)

@coderabbitai coderabbitai Bot Jul 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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.


Comment on lines +1 to +5

@coderabbitai coderabbitai Bot Jul 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

def update_memory(memory_id, namespace, updates):
"""
Updates an existing memory record, restricting changes to ALLOWED_UPDATE_FIELDS.
"""
# 1. Align with get_memory contract and handle missing records
memory = get_memory(memory_id, namespace)
if memory is None:
raise ValueError(f"Memory record {memory_id} not found in namespace {namespace}")

# 2. Log fields removed by the whitelist (names only, not values)
dropped_fields = [k for k in updates if k not in ALLOWED_UPDATE_FIELDS]
if dropped_fields:
logging.warning(f"Update dropped protected fields: {', '.join(dropped_fields)}")

# 3. Filter updates
allowed_updates = {k: v for k, v in updates.items() if k in ALLOWED_UPDATE_FIELDS}

# 4. Reconstruct the complete MemoryRecord, not a partial record
# Normalize the existing record to a dictionary to preserve all fields (agent_id, created_at, etc.)
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()
Comment on lines +25 to +30

@coderabbitai coderabbitai Bot Jul 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.


# Overlay only allowed updates
existing_data.update(allowed_updates)

# Return the reconstructed record
return MemoryRecord(**existing_data)
Loading