Merge 2.0.24 into main

[bittergreen]

Description

Merge 2.0.24 into main

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Unit Test

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) | 我已在 MemOS-Docs 中创建了相关的文档 issue/PR（如果适用）
• I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR（如果适用）
• I have mentioned the person who will review this PR | 我已提及将审查此 PR 的人

Reviewer Checklist

• closes #xxxx (Replace xxxx with the GitHub issue number)
• Made sure Checks passed
• Tests have been provided

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2119
Task: e0f9489efbd5c5ac
Base: main
Head: dev-v2.0.24

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

⚠️ 1 warning(s) occurred during review.

---

1. src/memos/api/lifecycle.py (L15-L17)

The early return when mem_scheduler is None will silently skip any other components that may be present in the components mapping (and any future entries added later). If the intent is to process only mem_scheduler, use continue (or just guard the block below) instead of return, so the loop can still handle additional keys without a logic change.

Suggested fix: replace the guard with a conditional block rather than an early return.

💡 Suggested Change

Before:

    mem_scheduler = components.get("mem_scheduler")
    if mem_scheduler is None:
        return

After:

    mem_scheduler = components.get("mem_scheduler")
    if mem_scheduler is not None:
        for method_name in ("stop", "rabbitmq_close"):
            method = getattr(mem_scheduler, method_name, None)
            if not callable(method):
                continue
            try:
                method()
            except Exception:
                logger.exception("Failed to run mem_scheduler.%s during API shutdown", method_name)

---

2. src/memos/api/client.py (L423)

File handles opened here inside a list comprehension are not managed by a with statement. If the list comprehension processes multiple files and a later open() call raises (e.g., permission error), previously opened file handles in the partially built list are silently leaked because there is no cleanup path for them.

Consider opening and closing each file inside the finally block, or accumulate them individually with explicit error handling:

def build_file_form_params() -> list:
    file_params = []
    for file_path in files:
        param = build_file_form_param(file_path)
        if param is not None:
            file_params.append(param)
    if not file_params:
        raise ValueError("files must contain at least one valid file path")
    return file_params

This does not fully solve the problem either — the real fix is to wrap each open() with a with statement and buffer the data, or close already-opened handles in an except block within the comprehension.

---

3. src/memos/api/client.py (L610-L614)

When user_ids has more than one element AND memory_ids is also provided, the compat shim silently skips setting user_id (because not memory_ids is False). In that case user_id stays None, memory_ids is truthy, and the delete proceeds by memory_id only — the extra user_ids entries are silently ignored rather than raising an error.

If the intent is that user_ids with memory_ids means "delete these memory_ids (user_ids is ignored)", that should be documented. If user_ids with multiple entries should always be rejected, the guard should be:

if user_id is None and user_ids:
    if len(user_ids) != 1:
        raise ValueError("current API supports a single user_id, not multiple user_ids")
    user_id = user_ids[0]

---

4. src/memos/api/client.py (L106)

get_message now requires both user_id and conversation_id at runtime (via _validate_required_params), but the signature declares conversation_id: str | None = None, making it appear optional. Any caller that previously passed only user_id will now receive a ValueError without any indication from the type signature.

Either make conversation_id a required positional parameter in the signature, or keep the validation as user_id-only (consistent with the original behaviour) and let the server enforce conversation_id if needed.

---

5. src/memos/api/client.py (L661-L662)

Falsy check incorrectly rejects legitimate empty-string values. If a caller passes content="" (e.g., to clear a memory's content) or status="", this guard raises ValueError even though the caller explicitly supplied a value.

Use None-checks instead:

if content is None and title is None and status is None:
    raise ValueError("content, title or status is required")

---

6. src/memos/api/client.py (L460)

This f-string logger call was not updated alongside the nearby logger.warning fix. Using an f-string defeats lazy log formatting (the string is always constructed even when the ERROR level is disabled) and hardcodes 3 instead of MAX_RETRY_COUNT. The same pattern also remains in get_message (line 126), add_message (line 176), add_feedback (line 595), delete_memory (line 648), and chat (line 844).

Suggested fix:

logger.error(
    "Failed to add knowledgebase-file form (retry %s/%s): %s",
    retry + 1,
    MAX_RETRY_COUNT,
    e,
)

---

7. src/memos/api/client.py (L425-L433)

File handles opened inside the list comprehension in build_file_form_params are not managed by a with statement. If a later open() call raises mid-iteration (e.g., permission error), previously opened handles in the partially built list leak — there is no cleanup path for them.

Suggested fix: accumulate file handles one-by-one with explicit error tracking so they can be closed on failure:

def build_file_form_params() -> list:
    file_params = []
    try:
        for file_path in files:
            param = build_file_form_param(file_path)
            if param is not None:
                file_params.append(param)
    except Exception:
        for p in file_params:
            p[1][1].close()
        raise
    if not file_params:
        raise ValueError("files must contain at least one valid file path")
    return file_params

---

8. apps/memos-local-plugin/core/llm/client.ts (L268)

The early-exit check matches any occurrence of the word "json" in the user message, including when it appears in the input data context (e.g., "Parse this JSON object: {...}"). In those cases the output-format instruction is never appended, which defeats the purpose of this function.

Consider testing only the final portion of the message (e.g., the last 200 characters), or matching more specific instruction phrases such as /respond.*json|return.*json|output.*json|json.*only/i.

💡 Suggested Change

Before:

    if (/\bjson\b/i.test(msg.content)) return messages;

After:

    if (/respond.*json|return.*json|output.*json|json.*only/i.test(msg.content)) return messages;

---

9. apps/memos-local-plugin/core/llm/client.ts (L265)

When no user message exists in the array (e.g., the caller passed only system/assistant messages), a bare "Return valid json only." user message is appended. This drops all original user intent from the conversation and replaces the user turn with a single output-format directive, which is likely not the intended behaviour for callers who craft explicit multi-turn prompts without a user role.

Consider throwing an error or logging a warning here, since a missing user message in a JSON-mode call is probably a caller bug rather than a case that should be silently patched.

---

10. src/memos/log.py (L243-L245)

The PID is recorded after dictConfig returns. Because an RLock allows re-entrant acquisition by the same thread, if dictConfig triggers any logging call on the same thread (e.g. a handler __init__ calling get_logger), configure_logging will be entered again while _LOGGING_CONFIGURED_PID is still None, so dictConfig will be called recursively — exactly the race this guard was designed to prevent.

Record the PID before invoking dictConfig to close the re-entrant window:

💡 Suggested Change

Before:

        if force or current_pid != _LOGGING_CONFIGURED_PID:
            dictConfig(LOGGING_CONFIG)
            _LOGGING_CONFIGURED_PID = current_pid

After:

        if force or current_pid != _LOGGING_CONFIGURED_PID:
            _LOGGING_CONFIGURED_PID = current_pid  # mark configured before calling dictConfig
            dictConfig(LOGGING_CONFIG)

---

11. src/memos/log.py (L229-L230)

_get_current_pid is a single-line wrapper whose only purpose is to make _get_current_pid monkeypatchable in tests. The standard Python approach is to monkeypatch os.getpid directly (or patch the name log.os.getpid), which avoids the extra indirection and the need for a dedicated internal helper. Consider removing the wrapper and calling os.getpid() inline inside configure_logging.

---

12. src/memos/api/product_models.py (L1244-L1249)

Breaking change in data field type: MemOSGetTaskStatusResponse.data was previously list[GetTaskStatusMessageData] and is now GetTaskStatusData (a single object). Any caller that accesses .data directly as a list (e.g., response.data[0].status, for item in response.data, len(response.data)) will fail at runtime with a TypeError or ValidationError. The backward-compat .messages property wraps the object in [self.data], but only callers who exclusively use .messages (not .data) are protected.

If callers exist outside this repository that iterate .data, those will silently break. If this shape change is intentional and matches the actual server response, consider also deprecating the old .messages property or at minimum documenting that .data is no longer iterable.

---

13. src/memos/api/product_models.py (L1151-L1158)

The GetTaskStatusMessageData class (which only had status: str) is now superseded by GetTaskStatusData but is still defined in the file. Since MemOSGetTaskStatusResponse.data no longer references it, and GetTaskStatusMessageData is not used anywhere else in the diff, it becomes dead code. If it is still exported and used by external consumers, removing it would be a breaking change — but if it is unused, it should be removed to avoid confusion.

---

14. src/memos/api/product_models.py (L1093-L1095)

Inconsistent pagination field naming across the two new pagination blocks. GetKnowledgebaseFileData uses page (current page number) and page_size, while GetMemoryData uses current (current page number) and size (page size). API consumers building generic pagination handling will have to special-case each model. If these names reflect different backend response contracts, that is acceptable, but it should be explicitly documented. If they reflect the same contract, align the names (e.g., both use page/page_size or both use current/size).

---

15. src/memos/api/server_api_ext.py (L65-L68)

Blocking I/O inside an async lifespan handler stalls the event loop.

shutdown_components is a synchronous function that ultimately calls rabbitmq_close(), which performs a blocking thread.join(timeout=5) (see rabbitmq_service.py:479). Calling it directly from an async def lifespan coroutine blocks the event loop for up to 5 seconds during shutdown, preventing other coroutines and cleanup tasks from running.

Wrap the call with asyncio.to_thread so it executes in a thread-pool worker without blocking the event loop:

import asyncio

@asynccontextmanager
async def lifespan(_app: FastAPI) -> AsyncIterator[None]:
    yield
    await asyncio.to_thread(shutdown_components, server_router_module.components)

💡 Suggested Change

Before:

@asynccontextmanager
async def lifespan(_app: FastAPI) -> AsyncIterator[None]:
    yield
    shutdown_components(server_router_module.components)

After:

@asynccontextmanager
async def lifespan(_app: FastAPI) -> AsyncIterator[None]:
    yield
    await asyncio.to_thread(shutdown_components, server_router_module.components)

---

16. src/memos/api/server_api_ext.py (L67-L68)

Blocking I/O inside an async def lifespan stalls the event loop.

shutdown_components is synchronous and ultimately calls rabbitmq_close(), which performs a blocking _io_loop_thread.join(timeout=5) (see rabbitmq_service.py line 479). Calling it directly from the async def lifespan coroutine will block the event loop for up to 5 seconds during shutdown, preventing other coroutines and pending cleanup tasks from running.

Wrap the call with asyncio.to_thread so the blocking work runs in a thread-pool worker:

import asyncio

@asynccontextmanager
async def lifespan(_app: FastAPI) -> AsyncIterator[None]:
    yield
    await asyncio.to_thread(shutdown_components, server_router_module.components)

💡 Suggested Change

Before:

    yield
    shutdown_components(server_router_module.components)

After:

    yield
    await asyncio.to_thread(shutdown_components, server_router_module.components)

---

17. tests/api/test_lifecycle.py (L34-L38)

This test has no assertions, so it only verifies that the function does not raise — not that it behaves correctly when methods are absent. Consider adding a verifiable observable side effect, for example by giving MinimalScheduler a flag or a calls list and asserting it remains empty, or by asserting the return value is None. Without an assertion the test will pass even if shutdown_components is completely broken for this path.

💡 Suggested Change

Before:

def test_shutdown_components_skips_missing_methods():
    class MinimalScheduler:
        pass

    shutdown_components({"mem_scheduler": MinimalScheduler()})

After:

def test_shutdown_components_skips_missing_methods():
    class MinimalScheduler:
        calls = []

    scheduler = MinimalScheduler()
    shutdown_components({"mem_scheduler": scheduler})

    assert scheduler.calls == []  # no method was invoked

---

18. tests/api/test_product_models.py (L25-L27)

These two tests access schema["example"] with a hard dict key lookup rather than .get(). If the "example" key is absent (e.g., the json_schema_extra changes or is removed), both tests will raise an uncaught KeyError instead of producing a clean assertion failure with a meaningful message.

Because each test function is executed independently (pytest does not guarantee ordering or share state between tests), test_add_request_example_messages_is_structured_list and test_add_request_example_covers_core_fields each need their own guard.

Suggestion: use .get() with an explicit fallback and assert on it, or at minimum add a per-test guard:

example = APIADDRequest.model_json_schema().get("example")
assert example is not None, "APIADDRequest should define a model-level example"

This produces a clear assertion failure message instead of a confusing KeyError traceback.

💡 Suggested Change

Before:

    example = APIADDRequest.model_json_schema()["example"]

    messages = example.get("messages")

After:

    schema = APIADDRequest.model_json_schema()
    example = schema.get("example")
    assert example is not None, "APIADDRequest should define a model-level example"

    messages = example.get("messages")

---

19. tests/api/test_product_models.py (L38-L40)

Same issue: direct ["example"] key access will raise KeyError instead of an assertion failure if the key is absent. Add a per-test guard:

example = APIADDRequest.model_json_schema().get("example")
assert example is not None, "APIADDRequest should define a model-level example"

💡 Suggested Change

Before:

    example = APIADDRequest.model_json_schema()["example"]

    assert "user_id" in example

After:

    schema = APIADDRequest.model_json_schema()
    example = schema.get("example")
    assert example is not None, "APIADDRequest should define a model-level example"

    assert "user_id" in example

---

20. tests/api/test_client.py (L14-L24)

These stub entries are injected into sys.modules but never removed. Because sys.modules is a process-global singleton that persists for the entire pytest session, once any test that uses the client_module fixture runs, the empty memos and memos.api stub objects remain cached.

tests/api/test_lifecycle.py and tests/api/test_product_models.py both perform real top-level imports (from memos.api.lifecycle import ... and from memos.api.product_models import ...). If test_client.py runs first, those files will find the empty stub memos.api module in the cache and raise an ImportError (or silently import the wrong object) — making test success dependent on execution order.

Suggestion: use a pytest autouse fixture with monkeypatch to install and automatically clean up the stub entries, or scope a conftest.py fixture with yield + restore logic:

@pytest.fixture(autouse=True)
def _memos_stub(monkeypatch):
    _install_memos_package_stub()
    yield
    # monkeypatch restores sys.modules automatically

Alternatively, do a proper package install (e.g. pip install -e .) so no manual stub is needed at all.

---

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

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (66/66 executed). memos_github_open_source/smoke: 1/1, memos_local_plugin/changed-repo-python: 20/20, memos_python_core/changed-repo-python: 45/45. Duration: 8s [advisory, non-gating] AI-generated tests on branch test/auto-gen-e0f9489efbd5c5ac-20260717153929: 89/134 passed, 45 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: dev-v2.0.24