Adjust feedback update review flow & unify LLM provider environment config

[bittergreen]

Description

refactor: unify LLM provider environment config
fix: Adjust feedback update review flow & using independent model config
fix: fix log for embedding models
fix: preserve text when downgrading feedback updates
fix: fix tests

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Refactor (does not change functionality, e.g. code style improvements, linting)

How Has This Been Tested?

• Unit Test
• tests/api/test_llm_provider_config.py
• tests/mem_feedback/test_feedback.py
• tests/mem_reader/test_preference_extractor_llm_config.py
• tests/mem_scheduler/test_config.py
• tests/mem_scheduler/test_openai_env_unification.py
• tests/test_hello_world.py

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 #2135
Task: b299eebd624853c1
Base: dev-v2.0.25
Head: wq-dev-v2.0.25

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

---

1. src/memos/api/config.py (L290)

Typo in docstring: MEMRADER_MODEL should be MEMREADER_MODEL. This is the same misspelling used in the actual env var name MEMRADER_MODEL (line 410), but the docstring clearly intends to refer to the MemReader model env var. Consistent spelling helps developers locate the right variable.

---

2. src/memos/api/config.py (L437-L443)

The QWEN_REMOVE_THINK_PREFIX environment variable is silently dropped. The old get_qwen_llm_config respected os.getenv('QWEN_REMOVE_THINK_PREFIX', 'true').lower() == 'true', allowing operators to opt out of think-prefix stripping. The new call hardcodes remove_think_prefix=True (the _build_provider_llm_config default). Users who had QWEN_REMOVE_THINK_PREFIX=false in their deployment will silently get stripping enabled with no warning. If removing this override is intentional, the method's docstring should explicitly state it, and a migration note should be added.

---

3. src/memos/hello_world.py (L75)

Using or means an empty string OPENAI_API_BASE="" silently falls back to the hardcoded default, which may surprise callers who set the variable explicitly to empty. If the intent is only to default when the variable is absent (not when it is empty), prefer:

"api_base": os.environ.get("OPENAI_API_BASE", "https://api.openai.com/v1"),

dict.get's second argument is returned only when the key is missing, so an explicit empty string would be preserved as-is.

---

4. src/memos/mem_reader/multi_modal_struct.py (L381-L392)

The comment claims to propagate related_id from constituent items, but the loop body only propagates manager_user_id and project_id. There is no code that reads or forwards related_id. Either the comment is incorrect (should not mention related_id), or the propagation logic for related_id was accidentally omitted. If related_id propagation is intended, a block similar to the two existing ones should be added:

if not extra_kwargs.get("related_id"):
    rid = getattr(metadata, "related_id", None)
    if rid:
        extra_kwargs["related_id"] = rid

💡 Suggested Change

Before:

        # Propagate manager_user_id, project_id, and related_id from constituent items
        for item in items:
            metadata = getattr(item, "metadata", None)
            if metadata is not None:
                if not extra_kwargs.get("manager_user_id"):
                    mid = getattr(metadata, "manager_user_id", None)
                    if mid:
                        extra_kwargs["manager_user_id"] = mid
                if not extra_kwargs.get("project_id"):
                    pid = getattr(metadata, "project_id", None)
                    if pid:
                        extra_kwargs["project_id"] = pid

After:

        # Propagate manager_user_id, project_id, and related_id from constituent items
        for item in items:
            metadata = getattr(item, "metadata", None)
            if metadata is not None:
                if not extra_kwargs.get("manager_user_id"):
                    mid = getattr(metadata, "manager_user_id", None)
                    if mid:
                        extra_kwargs["manager_user_id"] = mid
                if not extra_kwargs.get("project_id"):
                    pid = getattr(metadata, "project_id", None)
                    if pid:
                        extra_kwargs["project_id"] = pid
                if not extra_kwargs.get("related_id"):
                    rid = getattr(metadata, "related_id", None)
                    if rid:
                        extra_kwargs["related_id"] = rid

---

5. src/memos/mem_feedback/feedback.py (L905-L908)

When a downgraded ADD (_downgraded_from_update=True) shares the same text as a regular ADD that was already appended to llm_operations, the downgraded item is silently dropped via the add_text in add_texts check. The surviving item in llm_operations is the regular ADD (without _downgraded_from_update), so it will later be placed into filtered_items and discarded when has_update is true — meaning the memory update is lost entirely.

Conversely, if the downgraded ADD appears first and is appended, its marker is preserved and it ends up in update_items correctly, but the order of iteration is not guaranteed to be consistent.

Suggestion: During deduplication, prefer the downgraded item over the regular ADD when both share the same text. For example:

for item in dehalluded_operations:
    if item["operation"].lower() == "add":
        add_text = item.get("text")
        if not add_text:
            continue
        if add_text in add_texts:
            # If the incoming item is downgraded, upgrade the existing entry
            if item.get("_downgraded_from_update"):
                for existing in llm_operations:
                    if existing.get("text") == add_text:
                        existing["_downgraded_from_update"] = True
                        break
            continue
        llm_operations.append(item)
        add_texts.append(add_text)
    elif item["operation"].lower() == "update":
        llm_operations.append(item)

This ensures the _downgraded_from_update marker survives deduplication regardless of iteration order.

---

6. tests/api/test_llm_provider_config.py (L39-L43)

These five env vars (QWEN_TEMPERATURE, QWEN_MAX_TOKENS, QWEN_TOP_P, QWEN_TOP_K, QWEN_REMOVE_THINK_PREFIX) are set but the new get_qwen_llm_config() implementation no longer reads them — _build_provider_llm_config is called with hardcoded defaults (temperature=0.8, max_tokens=8000, etc.), so these env-var assignments have zero effect on the config.

This means any user who previously relied on QWEN_TEMPERATURE (or the other knobs) to tune the Qwen LLM will silently have their configuration ignored after this change. The test correctly captures the new behavior, but the behavioral regression itself should be an intentional, documented decision.

If the intent is to drop support for these per-provider tuning env vars, consider:

1. Adding a deprecation warning when these env vars are detected but ignored.
2. Or preserving the behavior by forwarding them as optional overrides in _build_provider_llm_config.

---

7. tests/api/test_llm_provider_config.py (L67-L69)

The test for get_feedback_llm_config() doesn't assert config['config']['model_name_or_path']. Given that the feedback config is constructed from FEEDBACK_MODEL, omitting this assertion leaves a gap: if the model name is accidentally dropped or aliased in the implementation, the test won't catch it. Consider adding:

assert config["config"]["model_name_or_path"] == "qwen3.6-flash"

---

8. tests/mem_feedback/test_feedback.py (L121-L131)

This test conflates two distinct behaviors in a single assertion:

1. Skipping ADD operations with missing or empty text fields.
2. Deduplicating identical ADD entries (the two '用户喜欢简洁回答。' entries collapse to one).

If the deduplication logic is removed or changed independently of the empty-text filtering, this test will fail or give a misleading result without clearly indicating which behavior broke. Consider splitting into two focused tests — one that asserts skipping of empty/missing-text ADDs (without duplicates), and another that specifically tests deduplication of identical ADDs.

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

❌ Automated Test Results: FAILED

Auto-fix retry 1/2 triggered.

Failed tests:

• test_memos_chen_tang_hello_world

Error details

The test test_memos_chen_tang_hello_world failed during MemoryFactory.from_config(config) initialization, indicating a regression in memory class instantiation triggered by the diff's config/component changes. [advisory, non-gating] AI-generated tests on branch test/auto-gen-dde64408b0856185-20260721180831: 135/136 passed, 1 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: wq-dev-v2.0.25

[Memtensor-AI]

❌ Automated Test Results: FAILED

Auto-fix retry 1/2 triggered.

Failed tests:

• test_memos_chen_tang_hello_world

Error details

The test_memos_chen_tang_hello_world test fails during MemoryFactory.from_config() when constructing a memory instance, indicating the diff's changes to config/component initialization broke the hello_world flow. [advisory, non-gating] AI-generated tests on branch test/auto-gen-72b55fa7700dc5bd-20260722112012: 163/171 passed, 8 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: wq-dev-v2.0.25

[Memtensor-AI]

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Executor error: Command failed: git clone --depth 1 --branch wq-dev-v2.0.25 git@github.com:bittergreen/MemOS.git /data/test-workspaces/7aab68b6fcc2599c/repo
Cloning into '/data/test-workspaces/7aab68b6fcc2599c/repo'...
error: unable to write file apps/memos-local-openclaw/src/viewer/html.ts
error: unable to write file apps/memos-local-openclaw/src/viewer/server.ts
fatal: cannot create directory at 'apps/memos-local-openclaw/tests': No space left on device
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry with 'git restore --source=HEAD :/'
Branch: wq-dev-v2.0.25

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (54/54 executed, 1 skipped). memos_github_open_source/smoke: 1/1, memos_python_core/changed-repo-python: 53 passed, 1 skipped. Duration: 6s [advisory, non-gating] AI-generated tests on branch test/auto-gen-b299eebd624853c1-20260722154700: 30/30 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: wq-dev-v2.0.25