Skip to content

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

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

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

Conversation

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

Description

Fixes #2103: SingleCubeView.search_memories no longer leaks full embedding vectors into INFO logs. Under the default APISearchRequest (dedup="mmr"), each memory previously carried its raw metadata.embedding into a logger.info(f"Search memories result: {memories_result}") call, producing megabytes of floats per search plus a paired misleading len(memories_result) line that counted top-level dict keys instead of memories.

The fix introduces _summarize_search_result_for_log in src/memos/multi_mem_cube/single_cube.py — a pure helper that produces a bounded dict: per-bucket counts (text_mem / pref_mem / tool_mem / skill_mem), an aggregate total, a capped 5-per-bucket sample exposing only {id, memory_type, relativity}, and a has_embedding boolean flag (never the raw vector). The two offending logger.info lines are replaced with a single sanitized summary line using %s deferred formatting (matches AGENTS.md logging style). Search API behaviour, response payload, and OpenAPI contract are unchanged; this is a logging-only fix.

Verification: PYTHONPATH=src pytest tests/multi_mem_cube/ -q → 11 passed (all new regression cases covering no-embedding-leak, per-bucket counts, sample bounding, both dedup branches, and malformed-input robustness). ruff check and ruff format --check both clean on changed files. tests/api/test_search_pipeline_hooks.py → 4 passed. Adjacent tests/test_add_stage_logging.py failures verified identical between clean tree and this branch → pre-existing and unrelated.

Reviewers: @MatthewZhuang, @CarltonXiang, @syzsunshine219, @World-controller.

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

SingleCubeView.search_memories was logging the full MOSSearchResult at
INFO on every call. Under the default APISearchRequest (dedup="mmr"),
each memory keeps its metadata.embedding vector, so the log line
contained megabytes of floats per search — an explosion of noise plus
a privacy/compliance risk. The paired `len(memories_result)` line was
also buggy: it counted top-level dict keys (~7 constant), not memories.

Introduce _summarize_search_result_for_log(result), a pure helper that
returns a bounded dict with per-bucket counts, an aggregate total, a
capped sample of {id, memory_type, relativity} entries per bucket, and
a has_embedding boolean flag — never the raw vector. Replace the two
offending logger.info lines with a single sanitized summary line using
%s deferred formatting.

Behaviour of the search API and its response payload is unchanged; this
is a logging-only fix.

Add regression tests under tests/multi_mem_cube/ covering the no-leak
contract, per-bucket counts, sample bounding, both dedup branches
(mmr/sim vs no), and malformed-input robustness. Tests import the
helper lazily to work around the existing circular import in
memos.api.handlers.__init__ (same workaround as
tests/test_add_stage_logging.py).

Fixes MemTensor#2103

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@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 14, 2026
@Memtensor-AI
Memtensor-AI requested a review from WeiminLee July 14, 2026 10:33
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2106
Task: c21ad1b92b74ac0c
Base: dev-v2.0.24
Head: feature/autodev-2103-20260714101225112
Head SHA: b057c12353634c26c9d21dbab85d6d34192acc89

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

⚠️ 1 warning(s) occurred during review.


1. src/memos/multi_mem_cube/single_cube.py (L93-L97)

counts[bucket] += len(memories) counts all items in the list — including non-dict entries like strings or None — before the per-item isinstance(mem, dict) guard below. This means counts over-reports when malformed items are present.

The docstring promises "total memories per bucket", which implies valid memory objects. If a caller accidentally passes a heterogeneous list, the count will be inflated silently. Consider counting only after validating each item:

for mem in memories:
    if not isinstance(mem, dict):
        continue
    counts[bucket] += 1
    ...

Alternatively, if counting all list entries (including malformed ones) is intentional, update the docstring to reflect that.

💡 Suggested Change

Before:

            counts[bucket] += len(memories)

            for mem in memories:
                if not isinstance(mem, dict):
                    continue

After:

            for mem in memories:
                if not isinstance(mem, dict):
                    continue

                counts[bucket] += 1

2. tests/multi_mem_cube/test_search_log_sanitization.py (L258-L260)

When embedding_value is [] (the dedup='no' branch), the for value in embedding_value loop body never executes, so zero assertions about embedding leaks are actually verified for that branch. Only the structural-key check runs. The test docstring and name imply symmetric coverage for both branches, but the empty-embedding case provides no leak-detection signal whatsoever.

Consider either: (a) adding an explicit assertion that 'embedding' or a sentinel string does not appear in rendered for the empty branch, or (b) splitting into two separate test cases with clearly scoped assertions so the intent is unambiguous.

💡 Suggested Change

Before:

        # These specific floats must not leak.
        for value in embedding_value:
            assert str(value) not in rendered

After:

        # These specific floats must not leak.
        for value in embedding_value:
            assert str(value) not in rendered

        # For the empty-embedding branch the loop above is a no-op; explicitly
        # verify that no raw 'embedding' key survives into the rendered output.
        assert '"embedding"' not in rendered

🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (duplicate: 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: c21ad1b92b74ac0c
  • Fix attempt: 1/2
  • Finding delta: 0 repeated / 2 new / 0 likely resolved

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

…rtion

Address two Open Code Review findings on PR MemTensor#2106:

1) src/memos/multi_mem_cube/single_cube.py — `_summarize_search_result_for_log`
   was doing `counts[bucket] += len(memories)` before the per-item
   `isinstance(mem, dict)` guard, so heterogeneous input (strings, None, etc.)
   silently inflated the reported bucket total and diverged from the
   docstring's "total memories per bucket" contract. Move the increment inside
   the loop, after validation, so only well-formed dict entries count.

2) tests/multi_mem_cube/test_search_log_sanitization.py — the parametrized
   `test_both_branches_produce_safe_output` case had zero leak assertions on
   the `embedding_value=[]` branch because the `for value in embedding_value`
   loop is a no-op. Add an explicit structural assertion that `'"embedding"'`
   never appears in the rendered summary so `dedup="no"` carries real
   leak-detection signal.

Also update the malformed-input regression case
(`test_non_dict_memory_items_do_not_raise`) to assert the new post-validation
count semantics (1 valid dict, not 3 raw list entries).

Related PR: MemTensor#2106
Refs MemTensor#2103

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2106
Task: c21ad1b92b74ac0c
Base: dev-v2.0.24
Head: feature/autodev-2103-20260714101225112
Head SHA: 33e6043fb10cb65a2ed53b61e0d651097aaf04fa

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

⚠️ 1 warning(s) occurred during review.


1. tests/multi_mem_cube/test_search_log_sanitization.py (L265)

The compensating assertion for the embedding_value=[] (dedup='no') branch is ineffective as a leak-detection guard.

The _summarize_search_result_for_log helper never writes an "embedding" key into the summary dict — it only emits "has_embedding" (a boolean). So '"embedding"' not in rendered is trivially always True regardless of the implementation, and would remain True even if a future change accidentally re-introduced the raw vector under a different key name (e.g. "embed", "vector", "raw_embedding").

The comment explicitly acknowledges that this branch needs a leak-detection signal, but the chosen assertion provides none.

Suggestion: add a meaningful assertion — for example, verify that has_embedding is False (the correct expected value when embedding is empty) and that the summary keys are exactly the expected safe set:

# Confirm empty embedding → has_embedding must be False
assert summary["has_embedding"] is False
# Pin exact top-level structure so surprise keys cannot hide leaked data
assert set(summary.keys()) == {"counts", "total", "samples", "has_embedding"}
💡 Suggested Change

Before:

        assert '"embedding"' not in rendered

After:

        assert summary["has_embedding"] is False
        assert set(summary.keys()) == {"counts", "total", "samples", "has_embedding"}

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

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 1 issue(s). I have resumed the development Agent to fix them.

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

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

Address the OCR round-2 finding on tests/multi_mem_cube/test_search_log_sanitization.py L265:

The previous compensating check `assert '"embedding"' not in rendered` was
trivially always True — `_summarize_search_result_for_log` never emits an
`"embedding"` key at all (only the boolean `"has_embedding"`), so the
assertion couldn't fail even if a future change re-introduced the raw vector
under a different name (`"embed"`, `"vector"`, `"raw_embedding"`, …).

Replace it with two assertions that carry real signal:

  * Parametrize `expected_has_embedding` alongside `embedding_value` and
    assert `summary["has_embedding"] is expected_has_embedding`. The empty
    branch now has a concrete non-trivial claim (must be False), and the
    populated branch pins the True case.
  * Pin the top-level keyset with `==` (not `>=`) so any surprise extra key
    — the most likely shape of an accidental future leak — fails the test.

No production-code change; this is a pure test-strengthening fix.

Related PR: MemTensor#2106
Refs MemTensor#2103

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

🤖 Open Code Review

Target: PR #2106
Task: c21ad1b92b74ac0c
Base: dev-v2.0.24
Head: feature/autodev-2103-20260714101225112
Head SHA: a2e12f8f6ff46c4e11bb8f00b9664857c4051a8c

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


1. tests/multi_mem_cube/test_search_log_sanitization.py (L132)

The big_vector contains 5 distinct float values — 0.0123, -0.0456, 0.7890, -0.1234, 0.5678 — but the assertion loop only checks 4 of them, silently omitting -0.1234. If the implementation ever selectively leaked only that value (e.g., due to a branch specific to the 4th component), this check would not catch it. To make the assertion complete and consistent with the vector definition, include -0.1234 in the checked set.

Suggested fix:

for value in (0.0123, -0.0456, 0.7890, -0.1234, 0.5678):
💡 Suggested Change

Before:

        for value in (0.0123, -0.0456, 0.7890, 0.5678):

After:

        for value in (0.0123, -0.0456, 0.7890, -0.1234, 0.5678):

2. tests/multi_mem_cube/test_search_log_sanitization.py (L137-L142)

The has_embedding flag semantics ("True iff at least one memory across all buckets has a non-empty embedding") are only exercised through text_mem. The tests never verify the case where embeddings are present only in pref_mem, tool_mem, or skill_mem — leaving those branches of the implementation's for bucket in _LOG_SUMMARY_BUCKETS loop untested. While the current implementation is correct, a future change that accidentally scopes the embedding scan to text_mem only would go undetected. Consider adding a test that populates only pref_mems with an embedding and asserts has_embedding is True.

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

⚠️ Open Code Review automatic OCR fix limit reached

Open Code Review still found 2 issue(s), but the automatic OCR fix loop has reached 2/2 attempts.

Finding delta: 0 repeated, 2 newly detected, 1 likely resolved since the previous repair request.

I stopped auto-fixing this PR to avoid an infinite loop. This PR now requires manual review by the CodeOwner/maintainer.

Please review the latest OCR comment and either fix the remaining findings or explicitly dismiss false positives.

PR: #2106

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:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants