Skip to content

Fix #2103: fix: Search_memories在默认dedup=mmr下调试用 info 日志打印完整 embedding 向量#2110

Closed
Memtensor-AI wants to merge 3 commits into
MemTensor:dev-v2.0.24from
Memtensor-AI:feature/autodev-2103-20260715081424787
Closed

Fix #2103: fix: Search_memories在默认dedup=mmr下调试用 info 日志打印完整 embedding 向量#2110
Memtensor-AI wants to merge 3 commits into
MemTensor:dev-v2.0.24from
Memtensor-AI:feature/autodev-2103-20260715081424787

Conversation

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

Description

Fixes issue #2103: SingleCubeView.search_memories no longer leaks full embedding vectors into INFO logs.

Root cause: logger.info(f"Search memories result: {memories_result}") in src/memos/multi_mem_cube/single_cube.py interpolated the entire result dict; under the default APISearchRequest.dedup="mmr" (also dedup="sim"), format_memory_item(..., include_embedding=True) keeps metadata.embedding populated, so every search dumped MBs of floats into logs and buried the useful memory_id / text / relativity fields — plus a potential privacy / compliance concern.

Fix: added a small pure helper _redact_embeddings_for_log() at module level that returns a shallow copy of memories_result with each text_mem / pref_mem / tool_mem / skill_mem memory's metadata.embedding list replaced by a <embedding len=N> placeholder. The caller's live dict is not mutated — downstream consumers (dedup, API response) still see the real vector. Only the log line changes; no API / schema / behavior change.

Tests: new tests/multi_mem_cube/test_search_log_redaction.py — 6 pure-helper unit tests (covering multi-partition, empty vector, missing metadata, non-mutation, partial dict) + 3 integration tests through the actual SingleCubeView.search_memories flow (default dedup=mmr, embedding never appears in the INFO log, useful fields preserved, count log preserved). 9/9 passing. Ruff check + format clean. Independent code-reviewer sub-agent returned APPROVE.

Note: 4 pre-existing failures in tests/api/test_mcp_serve.py are unrelated baseline failures on this branch (confirmed via git stash comparison against the pristine tree) — not caused by this change.

Related Issue (Required): Fixes #2103

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (does not change functionality, e.g. code style improvements, linting)
  • Documentation update

How Has This Been Tested?

Not run; documentation-only change.

  • Unit Test
  • Test Script Or Test Steps (please provide)
  • Pipeline Automated API Test (please provide)

Checklist

  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have created related documentation issue/PR in MemOS-Docs (if applicable)
  • I have linked the issue to this PR (if applicable)
  • I have mentioned the person who will review this PR

@MatthewZhuang, @CarltonXiang, @syzsunshine219, @World-controller please review this PR.

Reviewer Checklist

… log

`SingleCubeView.search_memories` used to `logger.info(f"Search memories
result: {memories_result}")`, which — under the default `dedup=mmr`
(and also `dedup=sim`) — leaked full high-dimensional embedding vectors
into INFO logs on every request. `format_memory_item(...,
include_embedding=True)` keeps `metadata.embedding` on each memory when
`dedup in ("mmr","sim")`, and `APISearchRequest.dedup` defaults to
`"mmr"`, so the default search path dumped MBs of floats into logs and
buried useful fields (memory_id / text / relativity).

Fix: introduce a small pure helper `_redact_embeddings_for_log()` that
returns a shallow copy of `memories_result` with each
`text_mem / pref_mem / tool_mem / skill_mem` memory's
`metadata.embedding` replaced by a `<embedding len=N>` placeholder. The
caller's live dict is not mutated — downstream consumers (dedup, API
response) still see the real embedding.

Behavior boundary: only the log line changes. Public API, schema, and
returned `memories_result` are unchanged.

Tests: `tests/multi_mem_cube/test_search_log_redaction.py` — 6
pure-helper unit tests + 3 integration tests via `SingleCubeView.
search_memories` (9/9 passing). The 4 pre-existing failures in
`tests/api/test_mcp_serve.py` are unrelated baseline failures on the
current branch (confirmed by git-stash comparison against the pristine
tree).

Refs MemTensor#2103
@Memtensor-AI Memtensor-AI added ai:generated Generated or modified by AI | 由 AI 生成或修改 area:memcube GeneralMemCube / cube 生命周期 / cube 配置 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Jul 15, 2026
@Memtensor-AI
Memtensor-AI requested a review from WeiminLee July 15, 2026 08:54
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2110
Task: 9c482c4b3ae26fe4
Base: dev-v2.0.24
Head: feature/autodev-2103-20260715081424787
Head SHA: 3ecf32626060116363ed967b29c1a6f9589bf0e6

🔍 OpenCodeReview found 3 issue(s) in this PR.

⚠️ 1 warning(s) occurred during review.


1. src/memos/multi_mem_cube/single_cube.py (L78)

The or [] guard is redundant here. group.get("memories", []) already returns [] when the key is absent, so [] or [] is just []. The or [] pattern is only useful when the value can be None (i.e., the key exists but its value is None). If None is a legitimate value for memories, consider using group.get("memories") or [] (without the default) to make the intent explicit.

💡 Suggested Change

Before:

            for mem in group.get("memories", []) or []:

After:

            for mem in group.get("memories") or []:

2. src/memos/multi_mem_cube/single_cube.py (L191)

Passing an eagerly-evaluated f-string to logger.info calls _redact_embeddings_for_log — a non-trivial multi-level dict traversal — unconditionally, even when the INFO level is disabled. This negates Python's lazy logging optimization. Consider passing a callable wrapper or using %s-style formatting with a helper object's __str__ to defer computation until the level check passes.

💡 Suggested Change

Before:

        self.logger.info(f"Search memories result: {_redact_embeddings_for_log(memories_result)}")

After:

        self.logger.info("Search memories result: %s", _redact_embeddings_for_log(memories_result))

3. tests/multi_mem_cube/test_search_log_redaction.py (L315)

next(...) without a default raises StopIteration if no log record contains "Search memories result", producing an opaque traceback instead of a clear assertion failure. This makes it harder to diagnose whether the log line was never emitted or the caplog scope was misconfigured. Use a None sentinel default and assert on it, or mirror the list-comprehension + assert len == 1 pattern already used in test_default_dedup_mmr_does_not_leak_embedding.

💡 Suggested Change

Before:

        summary = next(r.message for r in caplog.records if "Search memories result" in r.message)

After:

        matched = [r.message for r in caplog.records if "Search memories result" in r.message]
        assert len(matched) == 1, f"expected exactly one summary log line, got: {matched}"
        summary = matched[0]

🧹 Filtered 9 low-confidence OCR finding(s) before posting/fix-loop (duplicate: 9).

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🔧 Open Code Review requested Agent fix

Open Code Review found 3 issue(s). I have resumed the development Agent to fix them.

  • Task: 9c482c4b3ae26fe4
  • Fix attempt: 1/2
  • Finding delta: 0 repeated / 3 new / 0 likely resolved

The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed.

Apply three review-suggested fixes on top of the search_memories log
redaction change:

1. src/memos/multi_mem_cube/single_cube.py L78 — drop the redundant
   `or []` after `group.get("memories", [])`. `dict.get(key, default)`
   already returns `[]` when the key is missing; the extra `or []` is
   only meaningful when the value can be `None`. Switch to
   `group.get("memories") or []` so the intent (None-tolerant) is
   explicit and the default is not doubled.

2. src/memos/multi_mem_cube/single_cube.py L191 — restore Python's
   lazy logging by passing the redacted dict as a `%s` argument
   instead of interpolating it into an f-string. When INFO is
   disabled `_redact_embeddings_for_log` no longer runs a full
   multi-level dict copy for a message that will be dropped.

3. tests/multi_mem_cube/test_search_log_redaction.py L315 — replace
   `next(...)` (which raises an opaque StopIteration if the record is
   missing) with the list-comprehension + `assert len == 1` pattern
   already used by `test_default_dedup_mmr_does_not_leak_embedding`,
   giving a clear diagnostic when the log line isn't emitted.

Verified: `pytest tests/multi_mem_cube/test_search_log_redaction.py`
(9 passed) and `ruff check` on both files.
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2110
Task: 9c482c4b3ae26fe4
Base: dev-v2.0.24
Head: feature/autodev-2103-20260715081424787
Head SHA: 5ed7c26ca60a3d3316c2f209d1ea820e34c20bf5

🔍 OpenCodeReview found 2 issue(s) in this PR.

⚠️ 1 warning(s) occurred during review.


1. src/memos/multi_mem_cube/single_cube.py (L78)

Finding #1 — still unresolved. The or [] guard is functionally correct (it handles None values), but the original finding noted it is misleading/redundant when group.get("memories", []) already returns [] for absent keys. The real reason to keep or [] is to handle the case where the key exists but its value is None (e.g. {"memories": None}). The cleaner, more explicit way to express this intent is:

for mem in group.get("memories") or []:

vs.

for mem in (group.get("memories") or []):

Both are equivalent, but the original finding requested switching to group.get("memories", []) which would silently skip a None value — making the behavior subtly different. If None is a possible value and should be treated as empty, keep or [] but document the intent. If None cannot occur, switch to group.get("memories", []). The finding is not a correctness bug — it is a clarity issue that remains unaddressed.

💡 Suggested Change

Before:

            for mem in group.get("memories") or []:

After:

            for mem in (group.get("memories") or []):

2. src/memos/multi_mem_cube/single_cube.py (L191)

Finding #2 — still unresolved. Switching from an f-string to %s lazy formatting avoids the cost of str() conversion when INFO is disabled, but _redact_embeddings_for_log(memories_result) — a multi-level dict traversal — is still evaluated eagerly as a Python argument before logger.info is even called. Python evaluates all arguments before the function call, so the traversal overhead is unconditional regardless of log level. To truly guard the traversal, wrap it in an isEnabledFor check:

if self.logger.isEnabledFor(logging.INFO):
    self.logger.info("Search memories result: %s", _redact_embeddings_for_log(memories_result))
💡 Suggested Change

Before:

        self.logger.info("Search memories result: %s", _redact_embeddings_for_log(memories_result))

After:

        if self.logger.isEnabledFor(logging.INFO):
            self.logger.info("Search memories result: %s", _redact_embeddings_for_log(memories_result))

🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (existing_code_mismatch: 1).

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🔧 Open Code Review requested Agent fix

Open Code Review found 2 issue(s). I have resumed the development Agent to fix them.

  • Task: 9c482c4b3ae26fe4
  • Fix attempt: 2/2
  • Finding delta: 2 repeated / 0 new / 1 likely resolved

Repeated findings are prioritized because the previous repair did not converge.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed.

Two OCR findings from round 1 were flagged as "still unresolved" because
the round-1 fixes were only surface-level. This round addresses the
underlying intent:

1. src/memos/multi_mem_cube/single_cube.py L78 — the OCR concern is
   clarity, not correctness. `or []` is genuinely needed to tolerate
   an explicit `{"memories": None}` payload (which `dict.get(key)`
   returns as `None`, not `[]`). Round 1 removed the redundant
   default arg but left the intent implicit. Add a code comment
   spelling out that `or []` is a None-guard. (Note: OCR round 2
   suggested wrapping the expression in parentheses, but the project
   uses ruff format which strips such redundant parens on the next
   run — a comment survives the formatter and delivers the same
   clarity gain.)

2. src/memos/multi_mem_cube/single_cube.py L191 — round 1 switched
   from f-string to `%s`-style logging, which defers `str()`
   interpolation but NOT the argument evaluation. Python evaluates
   `_redact_embeddings_for_log(memories_result)` before the
   `logger.info` call regardless of level, so the multi-level dict
   traversal runs even when INFO is disabled. Wrap the log statement
   in `self.logger.isEnabledFor(logging.INFO)` so the traversal is
   truly skipped when INFO is off. Add `import logging` at the top.

Verified: `pytest tests/multi_mem_cube/test_search_log_redaction.py`
(9 passed), `ruff check`, `ruff format --check` on the changed file.
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2110
Task: 9c482c4b3ae26fe4
Base: dev-v2.0.24
Head: feature/autodev-2103-20260715081424787
Head SHA: e4b2f76684a414080867d89fd06395779a608c7a

🔍 OpenCodeReview found 1 issue(s) in this PR.

⚠️ 1 warning(s) occurred during review.


1. src/memos/multi_mem_cube/single_cube.py (L83)

Finding #1 — still unresolved. The or [] pattern remains unchanged. group.get("memories") or [] does correctly handle the {"memories": None} explicit-null case (which group.get("memories", []) would not — it would return None), so it is functionally justified. However, the or [] form also silently coerces any other falsy value (e.g. 0, False, "") to [], which is broader than intended. The idiomatic way to handle both the absent-key and explicit-None cases without that extra coercion is:

for mem in (group.get("memories") or []):

...which is what exists, or more explicitly:

memories = group.get("memories")
if memories is None:
    memories = []
for mem in memories:

If only None needs guarding (not other falsy values), prefer the explicit is None check. The original finding was flagged as still unresolved from the previous review cycle, and the code is unchanged here.

💡 Suggested Change

Before:

            for mem in group.get("memories") or []:

After:

            memories = group.get("memories")
            if memories is None:
                memories = []
            for mem in memories:

🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (duplicate: 1).

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai:generated Generated or modified by AI | 由 AI 生成或修改 area:memcube GeneralMemCube / cube 生命周期 / cube 配置 status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants