release: merge dev-v2.0.25 into main

[syzsunshine219]

Description

Merge dev-v2.0.25 into main to prepare the MemOS v2.0.25 release.

This release aggregates the fixes, features, performance improvements, and documentation updates currently integrated into the release branch. Major changes include:

• OpenMem v1 SDK and API alignment
• LLM provider configuration improvements and OpenRouter routing support
• Search, embedding, reranking, and PolarDB retrieval performance optimizations
• Feedback processing and document parsing fixes
• Local plugin reliability improvements covering logging, provider dispatch, namespace-aware viewer data, model availability handling, and session shutdown
• Release workflow and documentation updates

The release is implemented by promoting the validated dev-v2.0.25 branch into main. The package version has been confirmed as 2.0.25 in both pyproject.toml and src/memos/__init__.py.

No release-specific dependencies are introduced by this merge PR. Any dependency changes are contained in the individual changes already integrated into the release branch.

Related Issue (Required): N/A — this is a release aggregation PR; related issues are linked from the included pull requests.

Type of change

• 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?

The individual changes included in dev-v2.0.25 contain corresponding unit and regression tests where applicable.

Before merging, this release PR will be validated by the repository's required GitHub Actions test matrix across the supported Python versions and operating systems.

No separate manual API test was performed specifically for this release aggregation PR.

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

Checklist

-[x] 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 的人

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2156
Task: 47c422590cb9b57f
Base: main
Head: dev-v2.0.25

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

⚠️ 1 warning(s) occurred during review.

---

1. .github/scripts/draft-local-plugin-release-notes.mjs (L418-L424)

The heredoc delimiter __DOC_AGENT_EOF__ is hardcoded. If any value (e.g. release_notes_markdown, coverage JSON) contains the literal string __DOC_AGENT_EOF__ on its own line, the heredoc will be prematurely terminated, corrupting all subsequent GITHUB_OUTPUT variable assignments. This is a well-known GITHUB_OUTPUT injection vector.

Use a randomized delimiter that cannot appear in output values, e.g. a UUID or a crypto-random hex string generated once at startup.

💡 Suggested Change

Before:

function appendOutput(name, value) {
  const outputPath = process.env.GITHUB_OUTPUT;
  if (!outputPath) return;
  writeFileSync(outputPath, `${name}<<__DOC_AGENT_EOF__\n${value}\n__DOC_AGENT_EOF__\n`, {
    flag: "a",
  });
}

After:

const GITHUB_OUTPUT_DELIMITER = `DOC_AGENT_EOF_${Math.random().toString(36).slice(2).toUpperCase()}`;

function appendOutput(name, value) {
  const outputPath = process.env.GITHUB_OUTPUT;
  if (!outputPath) return;
  writeFileSync(outputPath, `${name}<<${GITHUB_OUTPUT_DELIMITER}\n${value}\n${GITHUB_OUTPUT_DELIMITER}\n`, {
    flag: "a",
  });
}

---

2. .github/scripts/draft-local-plugin-release-notes.mjs (L1210-L1213)

cleanError() only redacts Bearer-prefixed and sk-*-prefixed tokens. If the token is a UUID, opaque string, or any other non-matching format, it will appear in plain text in GitHub Actions warning annotations (via warn()) and in console.error() output at the top-level catch. The token is also passed verbatim in the Authorization header inside reportFailure(); any network-level exception there could surface the token in the thrown Error message before cleanError is applied.

Add a general token-scrubbing pattern to cleanError(), or store the token value separately and never pass it through error formatting paths.

💡 Suggested Change

Before:

  const token = process.env.DOC_AGENT_RELEASE_NOTES_DRAFT_TOKEN || "";
  if (!token.trim()) {
    fail("DOC_AGENT_RELEASE_NOTES_DRAFT_TOKEN secret is required when release_notes input is empty.");
  }

After:

function cleanError(value) {
  return String(value || "")
    .replace(/Bearer\s+\S+/gi, "Bearer ***")
    .replace(/sk-[A-Za-z0-9_-]+/g, "sk-***")
    .replace(/https?:\/\/[^\s"'<>]+/gi, "https://***")
    .replace(/\b\d{1,3}(?:\.\d{1,3}){3}(?::\d+)?\b/g, "***")
    // Scrub any token-like opaque strings (UUIDs, base64 tokens, etc.)
    .replace(/[A-Za-z0-9_-]{20,}/g, "***")
    .replace(/\s+/g, " ")
    .slice(0, 600);
}

---

3. .github/scripts/draft-local-plugin-release-notes.mjs (L1185-L1191)

RELEASE_FAILURE_ATTEMPT_DIR is used without any path validation. A misconfigured or attacker-controlled env var (e.g. set to /etc or /home/runner) would cause the function to read arbitrary files from the runner filesystem and POST their contents — after cleanError truncation — to the configured failure endpoint. The path should be validated to stay within an expected base directory (e.g. the runner's temp dir) before use.

💡 Suggested Change

Before:

  const attemptDir = String(process.env.RELEASE_FAILURE_ATTEMPT_DIR || "").trim();
  if (!phase || !attemptDir) fail("RELEASE_FAILURE_PHASE and RELEASE_FAILURE_ATTEMPT_DIR are required.");
  const attempts = [1, 2, 3].map((attempt) => {
    let message = "attempt log is unavailable";
    try { message = readFileSync(join(attemptDir, `${attempt}.log`), "utf8"); } catch {}
    return { error_code: phase.toUpperCase().replace(/[^A-Z0-9]+/g, "_"), message: cleanError(message), retryable: true };
  });

After:

  const attemptDir = String(process.env.RELEASE_FAILURE_ATTEMPT_DIR || "").trim();
  if (!phase || !attemptDir) fail("RELEASE_FAILURE_PHASE and RELEASE_FAILURE_ATTEMPT_DIR are required.");
  // Validate attemptDir is within the runner temp space to prevent path traversal
  const resolvedAttemptDir = join(tmpdir(), basename(attemptDir));
  if (!resolvedAttemptDir.startsWith(tmpdir())) {
    fail("RELEASE_FAILURE_ATTEMPT_DIR must be a path within the system temp directory.");
  }
  const attempts = [1, 2, 3].map((attempt) => {
    let message = "attempt log is unavailable";
    try { message = readFileSync(join(resolvedAttemptDir, `${attempt}.log`), "utf8"); } catch {}
    return { error_code: phase.toUpperCase().replace(/[^A-Z0-9]+/g, "_"), message: cleanError(message), retryable: true };
  });

---

4. .github/scripts/draft-local-plugin-release-notes.mjs (L334-L340)

The 24 KB diff slice is sent verbatim to an external HTTP endpoint. If any committed file in PRODUCT_PATH contains secrets (API keys, tokens in config files), they will be exfiltrated to the draft service without any scrubbing. Consider either omitting important_diff from the outbound payload and keeping it only for local inspection artifacts, or applying cleanError-style scrubbing before transmission.

💡 Suggested Change

Before:

  const importantDiff = sh([
    "diff",
    "--unified=2",
    diffRange,
    "--",
    PRODUCT_PATH,
  ]).slice(0, 24000);

After:

  // important_diff is intentionally kept out of the outbound evidence payload
  // to avoid exfiltrating potential secrets; it is written only to local artifacts.
  const importantDiff = sh([
    "diff",
    "--unified=2",
    diffRange,
    "--",
    PRODUCT_PATH,
  ]).slice(0, 24000);

---

5. .github/scripts/draft-local-plugin-release-notes.mjs (L1205-L1208)

There is no per-request timeout on the fetch calls. With up to 3 retry attempts in requestDraft() plus up to MAX_DRAFT_REPAIR_ATTEMPTS (2) repair cycles in requestValidatedDraft(), a slow or hanging remote service can block the GitHub Actions job indefinitely (runner default: 6 hours). Add an AbortController signal with a reasonable timeout (e.g. 60–120 s) to each fetchImpl call.

💡 Suggested Change

Before:

export async function requestDraft(
  evidence,
  { fetchImpl = fetch, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)) } = {},
) {

After:

export async function requestDraft(
  evidence,
  { fetchImpl = fetch, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), requestTimeoutMs = 120_000 } = {},
) {
  // ...
  // Inside the attempt loop:
  const controller = new AbortController();
  const timeoutId = setTimeout(() => controller.abort(), requestTimeoutMs);
  try {
    const response = await fetchImpl(url, {
      method: "POST",
      headers: { "content-type": "application/json", authorization: `Bearer ${token}` },
      body: JSON.stringify({ ... }),
      signal: controller.signal,
    });
  } finally {
    clearTimeout(timeoutId);
  }

---

6. .github/scripts/draft-local-plugin-release-notes.mjs (L566-L574)

rewriteKnownReleaseItem() silently replaces AI-generated bullet text with hardcoded version-specific Chinese/English strings whenever a commit subject blob matches keywords like hermes, vector scan, or lightweightmemory. In future releases these patterns may match unrelated commits and produce stale, incorrect release notes. The overwrite is not tracked in the postprocess metadata (reclassified_items only counts category changes, not text rewrites), making it invisible to reviewers.

At minimum, add a rewritten_items counter to postprocess and emit a warning when a rewrite occurs. Longer-term, move these overrides to an external data file that can be versioned and audited separately.

💡 Suggested Change

Before:

  if (item.category === "Improved") {
    if (/cpu|full-table vector scan|vector scan|向量/.test(blob)) {
      return {
        ...item,
        text_cn: "**向量扫描性能优化**：优化同步全表向量扫描路径，降低大数据量场景下的网关 CPU 压力。",
        text_en:
          "**Vector Scan Performance**: Optimized synchronous full-table vector scanning to reduce gateway CPU pressure at larger data sizes.",
      };
    }

After:

  // In postprocessDraftFromEvidence, track rewrites:
  let rewrittenItems = 0;
  items = inputItems.map((item) => {
    const rewritten = rewriteKnownReleaseItem({ ...item, category }, index);
    if (rewritten.text_cn !== item.text_cn || rewritten.text_en !== item.text_en) {
      rewrittenItems += 1;
      warn(`Release note item rewritten by known-pattern override: category=${item.category}, refs=${item.source_refs.join(",")}`);
    }
    return rewritten;
  });
  // And include in postprocess:
  const postprocess = {
    applied: true,
    removed_duplicate_source_refs: deduped.removedDuplicateRefs,
    dropped_empty_source_items: deduped.droppedItems,
    reclassified_items: reclassifiedItems,
    rewritten_items: rewrittenItems,
    final_item_count: items.length,
  };

---

7. .github/scripts/draft-local-plugin-release-notes.mjs (L109-L123)

Both gitShowJson and readJsonFile silently swallow all errors and return {}. When packageChanges() compares before and after objects, a missing or malformed package.json at either ref will silently produce spurious field-change entries (e.g. { field: 'version', before: undefined, after: '1.2.3' }) in the evidence payload, potentially misleading the draft service. At minimum, emit a warn() when the file cannot be read or parsed so the operator is aware.

💡 Suggested Change

Before:

function gitShowJson(ref, path) {
  try {
    return JSON.parse(sh(["show", `${ref}:${path}`]));
  } catch {
    return {};
  }
}

function readJsonFile(path) {
  try {
    return JSON.parse(readFileSync(path, "utf8"));
  } catch {
    return {};
  }
}

After:

function gitShowJson(ref, path) {
  try {
    return JSON.parse(sh(["show", `${ref}:${path}`]));
  } catch (e) {
    warn(`Could not read or parse ${path} at ref ${ref}: ${cleanError(e?.message || e)}`);
    return {};
  }
}

function readJsonFile(path) {
  try {
    return JSON.parse(readFileSync(path, "utf8"));
  } catch (e) {
    warn(`Could not read or parse file ${path}: ${cleanError(e?.message || e)}`);
    return {};
  }
}

---

8. .github/scripts/draft-local-plugin-release-notes.mjs (L46)

The CJK regex covers Hiragana/Katakana (\u3040-\u30ff), CJK Extension A + core block (\u3400-\u9fff), and CJK Compatibility Ideographs (\uf900-\ufaff), but misses CJK Extension B and beyond (\u{20000}+). More importantly, a text_cn field written entirely in ASCII pinyin (e.g. "Xinzeng L3 peizhizhi chi") would pass the Chinese-text check undetected, as pinyin contains no CJK code points. Consider adding a prose-level check or broadening the range to include the supplementary CJK blocks.

💡 Suggested Change

Before:

const CJK_RE = /[\u3040-\u30ff\u3400-\u9fff\uf900-\ufaff]/;

After:

// Broader CJK range including supplementary ideographs (requires unicode flag)
const CJK_RE = /[\u3040-\u30ff\u3400-\u9fff\uf900-\ufaff]|[\u{20000}-\u{2A6DF}]/u;

---

9. .github/scripts/draft-local-plugin-release-notes.mjs (L437-L438)

Any bare numeric string of 2+ digits (e.g. a version number component "42", a line count, or a year like "2024") is auto-prefixed with # and treated as a valid PR reference. This can produce false coverage hits where an unrelated number is matched to a real PR. Consider requiring at least 3–4 digits, or only accepting numbers in a realistic PR range for the repository.

💡 Suggested Change

Before:

  const text = String(value || "").trim().replace(/^[`[(\s]+|[`)\],.;\s]+$/g, "");
  if (/^\d{2,}$/.test(text)) return `#${text}`;

After:

  // Require at least 3 digits to reduce false PR ref matches
  if (/^\d{3,}$/.test(text)) return `#${text}`;

---

10. .github/scripts/draft-local-plugin-release-notes.test.mjs (L646-L649)

The sanitization assertion only checks the serialized request body. The reportFailure production function sends authorization: Bearer ${token} as an HTTP header (visible in the fetchImpl call's options.headers). If the token were ever accidentally included in the body too, the test would catch it — but a header-level leak would pass silently here.

Suggestion: also assert that the Authorization header in the failure-report fetch call does not contain the literal token value, e.g.:

const failureCall = calls.find((call) => call.url.includes("/failure"));
assert.ok(failureCall);
// Capture headers too: calls.push({ url, body: ..., headers: options.headers })
assert.doesNotMatch(failureCall.headers?.authorization ?? "", /test-token/);

This requires storing options.headers in the calls array alongside body.

---

11. .github/scripts/draft-local-plugin-release-notes.test.mjs (L249-L252)

Optional chaining makes this assertion silently incorrect when the expected item is absent. Array.prototype.find returns undefined when no match is found, so undefined?.text_cn.includes("XML") evaluates to undefined, and assert.equal(undefined, false) will throw — but not with a message that clearly indicates why the item was missing. More importantly, if the production postprocessing text ever changes the Chinese string (e.g., 记忆恢复 is replaced by a different term), this assertion becomes a false negative: undefined !== false causes a test failure, but the failure message won't explain that the item simply wasn't found.

Suggestion: separate the two concerns so failures are actionable:

const recoveryItem = processed.release_items.find((item) => item.text_cn.includes("记忆恢复"));
assert.ok(recoveryItem, "expected a release item containing '记忆恢复'");
assert.equal(recoveryItem.text_cn.includes("XML"), false);

---

12. .github/scripts/draft-local-plugin-release-notes.test.mjs (L531-L534)

The writeFileSync calls that create the test log files are placed before the try block. If any write fails (e.g., disk full), the temp directory is already created but the env vars are never set, so reportExternalFailureFromEnv is never called. The finally block will not execute (since we never entered the try), meaning both the temp directory and any partial env mutations from Object.assign will leak.

Suggestion: move the writeFileSync calls inside the try block so the finally cleanup always runs:

const directory = mkdtempSync(join(tmpdir(), "local-plugin-release-failure-"));
const previous = { ...process.env };
try {
  for (const attempt of [1, 2, 3]) writeFileSync(join(directory, `${attempt}.log`), `npm failure ${attempt}`);
  Object.assign(process.env, { ... });
  ...
} finally {
  process.env = previous;
  rmSync(directory, { recursive: true, force: true });
}

---

13. .github/scripts/draft-local-plugin-release-notes.test.mjs (L514)

Multiple tests restore the environment with process.env = previous (a reassignment) while others use Object.assign(process.env, {...}) to mutate it. These are not equivalent.

process.env = previous replaces the local binding but does NOT restore the OS-level environment. Any code that already holds a reference to the original process.env object (e.g., modules that cached process.env at import time, or Node internals) will still see the mutated values after the reassignment. This also means keys deleted mid-test (e.g., delete process.env.DOC_AGENT_RELEASE_NOTES_DRAFT_URL in the 'requires configured draft URL' test) are not actually removed from the original object.

The safe and consistent pattern is to restore via mutation:

// save
const saved = { url: process.env.DOC_AGENT_RELEASE_NOTES_DRAFT_URL };
// restore in finally
if (saved.url === undefined) delete process.env.DOC_AGENT_RELEASE_NOTES_DRAFT_URL;
else process.env.DOC_AGENT_RELEASE_NOTES_DRAFT_URL = saved.url;

Or use a helper that saves/deletes/restores the specific keys touched by each test.

---

14. .github/scripts/retry.sh (L26-L30)

Unrecognized flags that start with - silently fall through to the * catch-all and become part of the command instead of triggering an error. For example, retry.sh --attempts 3 --typo foo cmd would execute --typo foo cmd as the command. In a CI context this can silently swallow misconfiguration. Consider adding an explicit check for unknown --prefixed arguments:

    -*)
      echo "::error::Unknown option: $1" >&2
      exit 2
      ;;

---

15. .github/scripts/retry.sh (L42-L45)

The delay regex ^[0-9]+$ allows zero, meaning all retries fire with no pause between them. This can overwhelm downstream services and masks misconfigured env vars. There is also no upper-bound guard on attempts or delay, so values like --attempts 9999 could hang a CI job indefinitely. Consider rejecting zero delay and capping both values:

if ! [[ "${delay}" =~ ^[1-9][0-9]*$ ]] && [ "${delay}" -ne 0 ] || [ "${delay}" -gt 300 ]; then

Or at minimum reject zero: ^[1-9][0-9]*$ (mirroring the attempts pattern), and document recommended maximums.

---

16. .github/scripts/retry.sh (L64-L66)

The backoff formula delay * attempt is linear, not exponential. The variable name and warning message give no indication of this. If callers expect exponential backoff (common for rate-limited external services like npm or PyPI registries), retries may still be too aggressive. Either rename to reflect the linear intent, or use true exponential backoff:

sleep_seconds=$(( delay * (1 << (attempt - 1)) ))  # exponential: delay, 2*delay, 4*delay, ...

At minimum, add a comment clarifying that the backoff is linear.

---

17. .github/scripts/retry.sh (L49-L52)

set -e is re-enabled after capturing the exit status, but set -e inside a for loop body has well-known surprising interactions in bash: if any subcommand between set -e and the next set +e returns non-zero (e.g., the echo or sleep calls), the script exits without a clear error. A safer pattern avoids toggling set -e altogether by using an explicit conditional:

if "$@"; then
  status=0
else
  status=$?
fi

This captures the exit status reliably without re-enabling set -e mid-loop.

---

18. .github/scripts/retry.sh (L47)

seq is not POSIX and requires a subprocess fork. For the small iteration counts typical here it is harmless, but the idiomatic bash alternative avoids the external process:

attempt=1
while [ "${attempt}" -le "${attempts}" ]; do
  ...
  attempt=$(( attempt + 1 ))
done

This is more portable and consistent if the shebang (#!/usr/bin/env bash) is ever changed to #!/bin/sh.

---

19. .github/workflows/memos-local-plugin-post-merge-dry-run.yml (L14-L16)

These permissions are declared at the workflow level, so they apply to all jobs including this dry run. contents: write is almost certainly not needed for a dry run (no actual publish or git tag should occur). Scope permissions to the minimum required, and consider moving them into the job block instead of the workflow level to limit blast radius.

💡 Suggested Change

Before:

permissions:
  contents: write
  pull-requests: write

After:

permissions: {}

jobs:
  dry-run:
    permissions:
      pull-requests: write  # only if the called workflow needs to post a PR comment

---

20. .github/workflows/memos-local-plugin-post-merge-dry-run.yml (L19-L21)

The dry-run job has no timeout-minutes. If the called reusable workflow hangs (e.g., waiting for an npm registry or a long-running script), this job will consume runner resources indefinitely. Add a timeout appropriate for the expected publish duration.

💡 Suggested Change

Before:

  dry-run:
    if: ${{ github.repository == 'MemTensor/MemOS' }}
    uses: ./.github/workflows/memos-local-plugin-publish.yml

After:

  dry-run:
    if: ${{ github.repository == 'MemTensor/MemOS' }}
    timeout-minutes: 30
    uses: ./.github/workflows/memos-local-plugin-publish.yml

---

21. .github/workflows/memos-local-plugin-post-merge-dry-run.yml (L3-L7)

There is no concurrency group defined. Rapid successive pushes to main (e.g., a merge followed quickly by another) will queue multiple dry-run executions in parallel. Add a concurrency group with cancel-in-progress: true to avoid redundant runs.

💡 Suggested Change

Before:

on:
  push:
    branches:
      - main
    paths:

After:

on:
  push:
    branches:
      - main
    paths:

concurrency:
  group: dry-run-${{ github.ref }}
  cancel-in-progress: true

---

22. .github/workflows/memos-local-plugin-post-merge-dry-run.yml (L23)

The version is hardcoded to "2.0.10". This value will become stale as the project advances and someone will need to remember to update it manually. Consider deriving the version dynamically (e.g., from a package.json field or a dedicated version file) so the dry run always tests against the current release candidate.

---

23. apps/memos-local-openclaw/src/ingest/providers/anthropic.ts (L390-L391)

The raw Anthropic API response body is embedded verbatim in the thrown error message. Anthropic error responses can echo back metadata about the model, quota details, or partial request context. If this error propagates to application logs, client-facing error surfaces, or telemetry without sanitization, it becomes an unintended information-disclosure channel.

Consider logging the body at debug level and throwing only a sanitized message (status code + errorLabel), or truncating the body to a safe length.

---

24. apps/memos-local-openclaw/src/ingest/providers/anthropic.ts (L424-L433)

max_tokens: 10 is an extremely tight budget for the arbitration call. If the model produces any preamble before the keyword (e.g. "Answer: NEW" or "The answer is NEW"), the output may be cut off mid-preamble and answer.startsWith("NEW") will silently fall through to "SAME" — the wrong answer — with no warning. There is no log entry indicating that the model's finish reason was max_tokens, so this failure mode is invisible.

Recommendations:

1. Increase max_tokens to at least 16–32 to accommodate minimal preamble.
2. Check finish_reason from the response; if it is "max_tokens", log a warning before returning the default.
3. Verify that TOPIC_ARBITRATION_PROMPT strongly instructs the model to reply with a bare NEW or SAME token only.

---

25. apps/memos-local-openclaw/src/ingest/providers/anthropic.ts (L405)

taskState and newMessage are interpolated directly into the user message with no length cap or sanitization. Two risks:

1. Prompt injection: a crafted newMessage value containing \n\nHuman: or similar role-delimiter sequences could attempt to override the system prompt on certain model versions.
2. Token cost / API rejection: if taskState is a runaway blob (e.g. a large serialised state object), the payload could be extremely large, causing high latency, unexpected billing, or a 400 Request Too Large error with no graceful recovery.

Consider adding a max-length guard (e.g. truncate inputs to a configurable character limit) and logging a warning when truncation occurs.

---

26. apps/memos-local-openclaw/src/ingest/providers/anthropic.ts (L423-L430)

Same prompt-injection and unbounded-input concern as classifyTopicAnthropic above. Apply the same length cap and truncation warning here for consistency.

---

27. apps/memos-local-openclaw/src/ingest/providers/anthropic.ts (L355-L358)

The comment's description of the header-merge order is misleading in two ways:

1. It says cfg.headers is spread first to prevent credential overrides — but the actual spread order is Content-Type → ...cfg.headers → x-api-key → anthropic-version, meaning credentials are set last (preventing cfg.headers from overriding them), not first.
2. The phrase "user-supplied headers cannot override the credential" understates the implication: any x-api-key supplied through cfg.headers is silently discarded rather than merged, which is non-obvious to a future maintainer.

Update the comment to accurately describe the merge semantics and the intentional behaviour.

---

28. apps/memos-local-openclaw/src/ingest/providers/bedrock.ts (L399-L402)

The raw HTTP response body from an external Bedrock endpoint (or any proxy in front of it) is interpolated unsanitised into the error message. If the gateway echoes back request data or returns structured error text containing newlines/control characters, this creates a log-injection risk and leaks potentially sensitive upstream content to callers or log sinks. Truncate and strip control characters before including the body in the error string.

💡 Suggested Change

Before:

  if (!resp.ok) {
    const body = await resp.text();
    throw new Error(`Bedrock ${errorLabel} failed (${resp.status}): ${body}`);
  }

After:

  if (!resp.ok) {
    const rawBody = await resp.text();
    const safeBody = rawBody.replace(/[\r\n\t]/g, " ").slice(0, 200);
    throw new Error(`Bedrock ${errorLabel} failed (${resp.status}): ${safeBody}`);
  }

---

29. apps/memos-local-openclaw/src/ingest/providers/bedrock.ts (L383-L386)

cfg.headers is spread without any validation or allowlist. If SummarizerConfig.headers can be populated from user-supplied or environment-sourced data, a caller could inject sensitive headers (e.g., Authorization overrides, X-Forwarded-For, hop-by-hop headers) that bypass auth controls or influence routing. Allowlist the keys that are legitimately needed, or document clearly that cfg.headers must only ever be set by trusted config.

💡 Suggested Change

Before:

  const headers: Record<string, string> = {
    "Content-Type": "application/json",
    ...cfg.headers,
  };

After:

  const ALLOWED_EXTRA_HEADERS = new Set(["x-amz-security-token", "x-amzn-requestid"]);
  const safeExtra = Object.fromEntries(
    Object.entries(cfg.headers ?? {}).filter(([k]) => ALLOWED_EXTRA_HEADERS.has(k.toLowerCase())),
  );
  const headers: Record<string, string> = {
    "Content-Type": "application/json",
    ...safeExtra,
  };

---

30. apps/memos-local-openclaw/src/ingest/providers/bedrock.ts (L440-L447)

Unexpected or empty arbitration results are silently swallowed with only a warn log, then default to "SAME". This means a model hallucination or API error produces a silent, potentially wrong downstream decision with no observable metric or thrown error. Consider at minimum recording a metric or counter so that sustained degradation is detectable in production, aligning with how openai.ts handles the same case.

---

31. apps/memos-local-openclaw/src/ingest/providers/bedrock.ts (L414-L421)

The userContent template string is duplicated verbatim in both classifyTopicBedrock and arbitrateTopicSplitBedrock. Any future prompt-format change must be applied in two places, risking drift. Extract it into a shared inline helper or named constant.

💡 Suggested Change

Before:

  const userContent = `TASK:\n${taskState}\n\nMSG:\n${newMessage}`;
  const raw = await bedrockConverseTopic(
    TOPIC_CLASSIFIER_PROMPT,
    userContent,
    cfg,
    60,
    "topic-classifier",
  );

After:

function buildTopicUserContent(taskState: string, newMessage: string): string {
  return `TASK:\n${taskState}\n\nMSG:\n${newMessage}`;
}

// … inside classifyTopicBedrock:
  const userContent = buildTopicUserContent(taskState, newMessage);

---

32. apps/memos-local-openclaw/src/ingest/providers/bedrock.ts (L419-L420)

The maxTokens values 60 (classifier) and 10 (arbitration) are bare numeric literals with no explanation. If the prompt or response format changes, silent truncation will cause parseTopicClassifyResult to fail non-obviously. Declare them as named constants with a brief rationale comment.

💡 Suggested Change

Before:

    60,
    "topic-classifier",

After:

/** Classifier response is a short JSON object; 60 tokens is sufficient. */
const TOPIC_CLASSIFIER_MAX_TOKENS = 60;
/** Arbitration response is a single word (NEW or SAME); 10 tokens is sufficient. */
const TOPIC_ARBITRATION_MAX_TOKENS = 10;

---

33. apps/memos-local-openclaw/src/ingest/providers/bedrock.ts (L362-L364)

The comment says this constant is "scoped to the two functions below", but module-level const in TypeScript is visible to the entire file. The comment is misleading. If true scoping is intended, move the declaration inside bedrockConverseTopic; otherwise correct the comment.

💡 Suggested Change

Before:

// Default Bedrock model for the topic-classifier / arbitration helpers.
// Scoped to the two functions below to avoid churn in pre-existing helpers.
const DEFAULT_BEDROCK_TOPIC_MODEL = "anthropic.claude-3-haiku-20240307-v1:0";

After:

// Default Bedrock model used by bedrockConverseTopic and its two callers.
const DEFAULT_BEDROCK_TOPIC_MODEL = "anthropic.claude-3-haiku-20240307-v1:0";

---

34. apps/memos-local-openclaw/src/ingest/providers/gemini.ts (L369-L371)

The API key is appended as a plain URL query parameter. This causes it to appear in server access logs, network traces, browser history, and Referer headers on any redirect. The Gemini REST API supports the key as a query param by design, but when cfg.endpoint is caller-supplied and may point to a proxy or redirect, the key leaks to intermediate hops. Prefer passing the key in the Authorization or x-goog-api-key request header instead, which is also supported by the Gemini API and does not appear in server-side URL logs.

💡 Suggested Change

Before:

  const u = new URL(endpoint);
  u.searchParams.set("key", cfg.apiKey);
  const url = u.toString();

After:

  const u = new URL(endpoint);
  const url = u.toString();
  const headers: Record<string, string> = {
    "Content-Type": "application/json",
    "x-goog-api-key": cfg.apiKey,
    ...cfg.headers,
  };

---

35. apps/memos-local-openclaw/src/ingest/providers/gemini.ts (L372-L375)

cfg.headers is spread without any sanitization after Content-Type, so a misconfigured caller can silently override Content-Type or inject an Authorization header that supersedes the intended API key. If the suggestion above (x-goog-api-key) is adopted, place the auth header after the spread so caller-supplied headers cannot override it.

💡 Suggested Change

Before:

  const headers: Record<string, string> = {
    "Content-Type": "application/json",
    ...cfg.headers,
  };

After:

  const headers: Record<string, string> = {
    "Content-Type": "application/json",
    ...cfg.headers,
    // Auth header placed after spread to prevent caller override
    "x-goog-api-key": cfg.apiKey,
  };

---

36. apps/memos-local-openclaw/src/ingest/providers/gemini.ts (L388-L391)

The raw response body is embedded verbatim in the error message. Gemini error responses can echo back request fragments or include internal diagnostic detail. This error propagates up through classifyTopicGemini and arbitrateTopicSplitGemini with no catch, so the full body ends up in application logs. Consider truncating the body or stripping it before logging.

💡 Suggested Change

Before:

  if (!resp.ok) {
    const body = await resp.text();
    throw new Error(`Gemini ${errorLabel} failed (${resp.status}): ${body}`);
  }

After:

  if (!resp.ok) {
    const body = await resp.text();
    const truncated = body.length > 200 ? body.slice(0, 200) + "…" : body;
    throw new Error(`Gemini ${errorLabel} failed (${resp.status}): ${truncated}`);
  }

---

37. apps/memos-local-openclaw/src/ingest/providers/gemini.ts (L364)

The default model name is hardcoded inline. This is a business-relevant default that controls cost and capability. It should be lifted to a shared constants file or SummarizerConfig defaults so it is visible and changeable without touching transport logic. Other Gemini helpers in the same file likely duplicate this same default.

---

38. apps/memos-local-openclaw/src/ingest/providers/gemini.ts (L429-L432)

The empty-string guard (if (!answer)) is checked after text.toUpperCase(), which is safe but misleading — toUpperCase() on an empty string returns an empty string, so the guard still fires correctly. The real issue is that this warn fires but execution falls through silently to return "SAME". A consumer reading only the warn message has no signal that "SAME" was returned as the default. Logging the fallback value makes the behavior explicit.

💡 Suggested Change

Before:

  const answer = text.toUpperCase();
  log.debug(`Topic arbitration result: "${answer}"`);
  if (!answer) {
    log.warn("Gemini topic-arbitration returned empty text; defaulting to SAME");

After:

  const answer = text.toUpperCase();
  if (!answer) {
    log.warn("Gemini topic-arbitration returned empty text; defaulting to SAME");
    return "SAME";
  }
  log.debug(`Topic arbitration result: "${answer}"`);
  if (!answer.startsWith("NEW") && !answer.startsWith("SAME")) {
    log.warn(`Gemini topic-arbitration returned unexpected value "${answer}"; defaulting to SAME`);
  }

---

39. apps/memos-local-openclaw/src/ingest/providers/gemini.ts (L393-L394)

The JSON cast is structural-only with no runtime validation. If the Gemini API changes its response envelope or returns an unexpected shape, optional chaining silently produces an empty string, which downstream callers (parseTopicClassifyResult) will then handle as if the model returned nothing — no parse error is ever surfaced. A minimal shape check or Zod schema would make unexpected API changes explicit.

---

40. apps/memos-local-openclaw/src/shared/openrouter.ts (L30)

The OPENROUTER_HOSTS set only contains "openrouter.ai", but the detection logic in isOpenRouter checks hostname, which strips subdomains. A URL like https://api.openrouter.ai/... has hostname api.openrouter.ai, which won't match. Consider checking with endsWith or storing the known domain suffix instead:

return [...OPENROUTER_HOSTS].some(h => hostname === h || hostname.endsWith(`.${h}`));

💡 Suggested Change

Before:

    return OPENROUTER_HOSTS.has(new URL(config.endpoint).hostname.toLowerCase());

After:

    const hostname = new URL(config.endpoint).hostname.toLowerCase();
    return [...OPENROUTER_HOSTS].some(h => hostname === h || hostname.endsWith(`.${h}`));

---

41. apps/memos-local-plugin/adapters/hermes/memos_provider/__init__.py (L1614)

Using daemon=True means this thread will be killed abruptly if the main process exits before the 5-second request completes. Since shutdown() is meant to cleanly finalize an episode, a daemon thread could leave the session unclosed on normal process exit (e.g., if the Hermes gateway shuts down immediately after calling shutdown()). Consider using a non-daemon thread, or joining it with a short timeout so the session close can complete during an orderly shutdown while still bounding the wait.

💡 Suggested Change

Before:

        threading.Thread(target=_close, daemon=True).start()

After:

        t = threading.Thread(target=_close, daemon=False)
        t.start()
        # Optionally join with timeout to allow orderly shutdown without blocking indefinitely
        # t.join(timeout=5.5)

---

42. apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py (L183)

rglob("*.ts") will descend into node_modules/, which can contain tens of thousands of .ts type-declaration files. This makes the staleness scan very slow and almost always returns a newest_source that is newer than dist/bridge.cjs, causing a spurious rebuild on every start.

Exclude node_modules (and dist) from the walk, e.g.:

def _ts_mtimes(root: Path):
    for p in root.rglob("*.ts"):
        if "node_modules" not in p.parts and "dist" not in p.parts:
            yield p.stat().st_mtime

newest_source = max(_ts_mtimes(plugin_root), default=0.0)

💡 Suggested Change

Before:

            (p.stat().st_mtime for p in plugin_root.rglob("*.ts")),

After:

            (p.stat().st_mtime for p in plugin_root.rglob("*.ts")
             if "node_modules" not in p.parts and "dist" not in p.parts),

---

43. apps/memos-local-plugin/adapters/hermes/memos_provider/daemon_manager.py (L210-L212)

When TimeoutExpired is raised, the npm run build child process is still running in the background. subprocess.run does not kill it automatically on timeout. You should kill (and wait for) the process to avoid leaving a dangling build worker, which could later corrupt the output or compete with a subsequent build attempt.

Use result.kill(); result.communicate() in the handler, or switch to a manual Popen pattern that always cleans up:

except subprocess.TimeoutExpired as e:
    if e.proc is not None:  # subprocess.run attaches .proc on timeout
        e.proc.kill()
        e.proc.communicate()
    logger.error("MemOS: bridge rebuild timed out after 120s")
    return False

💡 Suggested Change

Before:

    except subprocess.TimeoutExpired:
        logger.error("MemOS: bridge rebuild timed out after 120s")
        return False

After:

    except subprocess.TimeoutExpired as e:
        if hasattr(e, "proc") and e.proc is not None:
            e.proc.kill()
            e.proc.communicate()
        logger.error("MemOS: bridge rebuild timed out after 120s")
        return False

---

44. apps/memos-local-plugin/agent-contract/memory-core.ts (L521)

includeAllNamespaces on metrics lacks any JSDoc, yet the comment added to listApiLogs in the same diff explicitly notes that the underlying api_logs table has no per-namespace owner column, making the flag a no-op today. Since metrics aggregates from those same tables, the same caveat almost certainly applies here too — but a reader of this interface has no way to know that. Add a JSDoc comment (or at minimum a // no-op today – see listApiLogs comment inline note) so implementors and callers understand the current behaviour and the eventual intent, matching the documentation standard set for listApiLogs.

---

45. apps/memos-local-plugin/agent-contract/memory-core.ts (L372)

includeAllNamespaces is added to listEpisodes without any JSDoc, while the same parameter on listApiLogs (in this same diff) is accompanied by a detailed comment explaining the semantics, the no-op caveat, and the referenced issue (#2131). For consistency and to keep the interface contract self-documenting, add equivalent documentation here — particularly whether the no-op caveat applies to the episodes table as well.

---

46. apps/memos-local-plugin/core/config/index.ts (L91-L93)

The original RangeError from Intl.DateTimeFormat (e.g. "Invalid time zone specified") is swallowed here. Forwarding it gives users a clearer diagnostic — especially when they provide a near-miss like "America/New_york" vs "America/New_York".

Suggested fix: capture and append the underlying error message.

💡 Suggested Change

Before:

  } catch {
    throw new MemosError("config_invalid", `invalid logging.timezone: ${completed.logging.timezone}`);
  }

After:

  } catch (e) {
    const detail = e instanceof Error ? ` (${e.message})` : "";
    throw new MemosError("config_invalid", `invalid logging.timezone: ${completed.logging.timezone}${detail}`);
  }

---

47. apps/memos-local-plugin/core/config/schema.ts (L61-L69)

Two issues with this effort union:

1. "none" conflicts with enabled: false. A user can write enabled: true, effort: "none" and the schema accepts it — serializeOpenRouterReasoning passes both fields through verbatim with no reconciliation, producing a contradictory payload to the API. Consider removing "none" from this union (disabling reasoning is already handled by enabled: false) or add a runtime guard in serializeOpenRouterReasoning that ignores enabled when effort === "none".

2. "xhigh" and "max" are not documented in OpenRouter's public reasoning API (which defines low, medium, high). If passed verbatim, they will likely be silently ignored or rejected. Verify these values against the provider spec, and if they are provider-specific extensions, add a comment citing the source.

---

48. apps/memos-local-plugin/core/config/schema.ts (L605-L606)

No IANA timezone validation is performed before this value is used. isoFromEpochMs in core/time.ts passes it directly to new Intl.DateTimeFormat("en-CA", { timeZone: tz, ... }). An invalid string (e.g., "Europe/Lndon") throws a RangeError at the first log call, crashing logger initialization and potentially the entire plugin startup. Add a schema-level refinement or validate at config-load time, for example:

try { new Intl.DateTimeFormat("en-CA", { timeZone: value }); } catch { throw new Error(`Invalid timezone: ${value}`); }

---

49. apps/memos-local-plugin/core/embedding/types.ts (L34-L35)

openRouter is a required field (no ?), but all other newly added OpenRouter-related fields (providerIgnore, providerOrder) are optional. This is inconsistent and will be a breaking change for any existing code that constructs an EmbeddingConfig object without this field. Consider making it optional with a default of false, or adding a default value in the config resolution layer.

💡 Suggested Change

Before:

  /** Explicitly enable OpenRouter fields for a reverse proxy or CNAME. */
  openRouter: boolean;

After:

  /** Explicitly enable OpenRouter fields for a reverse proxy or CNAME. */
  openRouter?: boolean;

---

50. apps/memos-local-plugin/core/llm/providers/openai.ts (L146-L157)

The OpenRouter reasoning injection block is duplicated verbatim in both the non-streaming and streaming paths. Extracting it into a helper (e.g., applyOpenRouterReasoning(config, body)) would reduce duplication and make future changes to the reasoning serialization logic easier to maintain in one place.

💡 Suggested Change

Before:

    if (applyOpenRouterProviderRouting(config, body)) {
      const reasoning = config.reasoning && serializeOpenRouterReasoning(config.reasoning);
      if (reasoning) body.reasoning = reasoning;
    }

    const headers: Record<string, string> = {};
    if (config.apiKey) {
      headers.Authorization = `Bearer ${config.apiKey}`;
    }
    Object.assign(headers, config.headers);

    const resp = await httpPostStream({

After:

// Extract into a shared helper alongside applyOpenRouterProviderRouting:
function applyOpenRouterReasoning(config: LlmConfig, body: Record<string, unknown>): void {
  if (applyOpenRouterProviderRouting(config, body)) {
    const reasoning = config.reasoning && serializeOpenRouterReasoning(config.reasoning);
    if (reasoning) body.reasoning = reasoning;
  }
}

// Then call it in both paths:
applyOpenRouterReasoning(config, body);

---

51. apps/memos-local-plugin/core/openrouter.ts (L23-L26)

This function has a dual responsibility: it mutates body in-place AND returns a boolean, which is a surprising API shape. Callers may not realize body is modified as a side effect. Consider either (a) returning the provider patch object instead of mutating, letting the caller merge it, or (b) renaming to something that signals mutation (e.g. injectOpenRouterProviderRouting) and documenting the side effect explicitly.

💡 Suggested Change

Before:

export function applyOpenRouterProviderRouting(
  config: OpenRouterRoutingConfig,
  body: Record<string, unknown>,
): boolean {

After:

/**
 * Injects OpenRouter provider-routing fields into `body` in-place.
 * Returns `true` if the endpoint was identified as OpenRouter, `false` otherwise.
 * Note: `body` is mutated directly when routing fields are applied.
 */
export function applyOpenRouterProviderRouting(
  config: OpenRouterRoutingConfig,
  body: Record<string, unknown>,
): boolean {

---

52. apps/memos-local-plugin/core/openrouter.ts (L11)

The set is initialised with a single static string. A plain string constant (const OPENROUTER_HOST = "openrouter.ai") and a simple equality check would be clearer and cheaper. Use a Set only when multiple values are genuinely expected, or add a comment explaining the extension point.

💡 Suggested Change

Before:

const OPENROUTER_HOSTS = new Set(["openrouter.ai"]);

After:

/** Add more hostnames here as additional OpenRouter-compatible hosts are identified. */
const OPENROUTER_HOSTS = new Set(["openrouter.ai"]);

---

53. apps/memos-local-plugin/core/pipeline/deps.ts (L215)

bgReflectLlm is only ever consumed for its .provider and .model metadata in the evaluator block — it is never passed to any subscriber that actually invokes LLM calls. Wrapping deps.reflectLlm in rateLimitLlmClient solely to read two delegating getters creates an unnecessary object allocation and misleads readers into thinking the semaphore is being exercised here.

Consider accessing the metadata directly from deps.reflectLlm:

evaluator: {
  reflectionProvider: deps.reflectLlm?.provider,
  reflectionModel: deps.reflectLlm?.model,
  ...
}

This makes the intent explicit and eliminates the dead wrapper.

💡 Suggested Change

Before:

  const bgReflectLlm = rateLimitLlmClient(deps.reflectLlm, bgLlmSemaphore);

After:

  // bgReflectLlm wrapper removed — metadata is read directly from deps.reflectLlm below

---

54. apps/memos-local-plugin/core/pipeline/deps.ts (L271-L272)

Because bgReflectLlm (the rate-limited wrapper) is the only variable referencing deps.reflectLlm here, using it for metadata while simultaneously noting it is "read-only" creates confusion. If bgReflectLlm is intentionally kept for future use where it would actually make calls, a clarifying comment to that effect would prevent it from being cleaned up as dead code. Otherwise, replace with deps.reflectLlm?.provider / deps.reflectLlm?.model directly.

💡 Suggested Change

Before:

      reflectionProvider: bgReflectLlm?.provider,
      reflectionModel: bgReflectLlm?.model,

After:

      reflectionProvider: deps.reflectLlm?.provider,
      reflectionModel: deps.reflectLlm?.model,

---

55. apps/memos-local-plugin/core/pipeline/orchestrator.ts (L91-L96)

The timeout promise's setTimeout is never cleared when classifyFn() resolves first. After timeoutMs ms the inner promise rejects, but by then Promise.race has already settled and its .catch is no longer listening — so the rejection surfaces as an unhandled promise rejection. In Node.js 15+ with --unhandled-rejections=throw (the default), this can crash the process under sustained load.

Fix: capture the timer handle and clear it when classifyFn() settles:

function classifyWithTimeout(
  classifyFn: () => Promise<RelationDecision>,
  timeoutMs: number,
  log: Logger,
): Promise<RelationDecision> {
  let timer: ReturnType<typeof setTimeout>;
  return Promise.race([
    classifyFn().finally(() => clearTimeout(timer)),
    new Promise<RelationDecision>((_, reject) => {
      timer = setTimeout(() => reject(new Error("classify_timeout")), timeoutMs);
    }),
  ]).catch((err) => {
    // ...
  });
}

---

56. apps/memos-local-plugin/core/pipeline/orchestrator.ts (L96-L107)

The .catch swallows every error thrown by classifyFn() — not just the timeout sentinel — and logs them all as relation.classify_timeout. A network failure, a malformed LLM response, or a programming error inside classify() will be silently treated as a clean timeout, masking real failures and polluting any dashboards or alerts keyed on that log event.

Distinguish the two cases explicitly:

]).catch((err) => {
  const isTimeout = err instanceof Error && err.message === "classify_timeout";
  if (isTimeout) {
    log.warn("relation.classify_timeout", { timeoutMs });
  } else {
    log.error("relation.classify_error", {
      err: err instanceof Error ? err.message : String(err),
    });
  }
  return {
    relation: "new_task" as const,
    confidence: 0,
    reason: isTimeout ? "classify_timeout" : "classify_error",
    signals: [isTimeout ? "classify_timeout" : "classify_error"],
  };
});

---

57. apps/memos-local-plugin/core/storage/migrations/008-feedback-experience-metadata.sql (L2-L5)

The index idx_policies_experience is already defined in 001-initial.sql (line 160) with the same IF NOT EXISTS guard. Since the project comment in 001-initial.sql states that the plugin is pre-release and every install wipes the previous DB, this migration file is now a complete no-op on every code path — fresh installs get the index from 001, and there are no legacy DBs to upgrade.

If incremental upgrades are ever supported in the future, this file's body should be empty (or contain only a comment) to make the no-op intent explicit, rather than re-declaring an index that 001 already owns. Alternatively, if the file must remain for migration version tracking, document clearly that it is intentionally empty.

💡 Suggested Change

Before:

-- All ALTER TABLE columns backported into 001-initial.sql; only indexes remain here.
 
 CREATE INDEX IF NOT EXISTS idx_policies_experience
   ON policies(experience_type, evidence_polarity, updated_at DESC);

After:

-- All ALTER TABLE columns backported into 001-initial.sql.
-- This migration is intentionally a no-op: the index below is already
-- defined in 001-initial.sql and is retained here only to keep the
-- migration version sequence intact.
-- CREATE INDEX IF NOT EXISTS idx_policies_experience
--   ON policies(experience_type, evidence_polarity, updated_at DESC);

---

58. apps/memos-local-plugin/core/storage/repos/episodes.ts (L204)

The limit value is interpolated directly into the SQL string. Even though clampLimit should return a safe integer, this bypasses the parameterized query pattern used everywhere else in the file. If clampLimit ever returns a non-integer (e.g., NaN or a string), this becomes a SQL injection vector. Prefer binding limit as a named parameter for consistency and safety.

💡 Suggested Change

Before:

      const sql = `SELECT ${COLUMNS.join(", ")} FROM episodes ${where} ORDER BY started_at DESC, id DESC LIMIT ${limit}`;

After:

      params.limit = limit;
      const sql = `SELECT ${COLUMNS.join(", ")} FROM episodes ${where} ORDER BY started_at DESC, id DESC LIMIT @limit`;

---

59. apps/memos-local-plugin/core/time.ts (L58)

When opts.offset is false (or omitted) and tz is a non-UTC timezone, the returned string has no timezone indicator at all — it looks like a local time but carries no offset or Z suffix. This makes the output ambiguous: consumers can't tell whether the timestamp is UTC or local time. Consider always appending the offset when a non-UTC tz is provided, or at minimum appending the literal timezone abbreviation. If the no-suffix behavior is intentional, a comment explaining the reasoning would prevent future misuse.

---

60. apps/memos-local-plugin/core/time.ts (L86-L87)

Intl.DateTimeFormat with timeZoneName: "longOffset" is a relatively recent addition (ES2021/Chrome 95+). On older runtimes this will throw or silently fall back. More critically, if fmt.formatToParts returns no timeZoneName part (e.g. unsupported environment or an unusual tz string), zone is undefined and the function silently returns "+00:00", masking the real offset error without any warning or thrown exception. At minimum, log or throw when zone is unexpectedly absent.

💡 Suggested Change

Before:

  const zone = fmt.formatToParts(d).find((part) => part.type === "timeZoneName")?.value;
  if (!zone || zone === "GMT" || zone === "UTC") return "+00:00";

After:

  const zone = fmt.formatToParts(d).find((part) => part.type === "timeZoneName")?.value;
  if (!zone) {
    // timeZoneName:"longOffset" unavailable in this runtime — fall back to UTC offset
    console.warn(`offsetForTimeZone: could not determine offset for tz="${tz}", defaulting to +00:00`);
    return "+00:00";
  }
  if (zone === "GMT" || zone === "UTC") return "+00:00";

---

61. apps/memos-local-plugin/core/time.ts (L61)

dateTimeParts returns Record<string, string>, so callers access parts without any compile-time safety. The fractionalSecond field is accessed with a non-null fallback (?? "000") but all other fields (year, month, day, hour, minute, second) are used without null guards. If any part is unexpectedly missing the result will silently contain undefined in the ISO string. Consider defining a typed interface for the parts object, or adding a guard that throws on missing required fields.

💡 Suggested Change

Before:

function dateTimeParts(d: Date, tz: string): Record<string, string> {

After:

interface DateParts {
  year: string;
  month: string;
  day: string;
  hour: string;
  minute: string;
  second: string;
  fractionalSecond?: string;
}

function dateTimeParts(d: Date, tz: string): DateParts {

---

62. apps/memos-local-plugin/core/util/semaphore.ts (L25-L29)

The release function can be called multiple times since the returned closure has no guard against double-invocation. Each extra call decrements current (floored at 0 by Math.max) and pops an additional waiter, allowing more concurrent holders than limit. Guard against this by tracking whether the release has already been called.

💡 Suggested Change

Before:

  function release() {
    current = Math.max(0, current - 1);
    const next = waiters.shift();
    if (next) next();
  }

After:

  function makeRelease(): () => void {
    let released = false;
    return function release() {
      if (released) return;
      released = true;
      current = Math.max(0, current - 1);
      const next = waiters.shift();
      if (next) next();
    };
  }

---

63. apps/memos-local-plugin/server/routes/metrics.ts (L112-L116)

With includeAllNamespaces: true now fetching across all namespaces, the fixed limit: 5_000 cap is more likely to silently truncate data from older or less-active namespaces, producing an incomplete aggregation in the tool panel. Consider either raising the limit, adding a pagination loop to exhaust all results, or at minimum logging a warning when logs.length === 5_000 to surface the truncation to operators.

💡 Suggested Change

Before:

    const { logs } = await deps.core.listApiLogs({
      limit: 5_000,
      offset: 0,
      includeAllNamespaces: true,
    });

After:

    const { logs } = await deps.core.listApiLogs({
      limit: 5_000,
      offset: 0,
      includeAllNamespaces: true,
    });
    if (logs.length === 5_000) {
      // Warn that results may be truncated across namespaces
      console.warn("[metrics] listApiLogs hit the 5_000 row limit; aggregation may be incomplete");
    }

---

64. apps/memos-local-plugin/server/routes/metrics.ts (L127)

Same truncation risk as listApiLogs above: limit: 2_000 was sized for a single namespace. Fetching across all namespaces with the same cap means recent traces from some namespaces may be silently dropped, skewing the latency panel. Consider raising the limit or adding a similar guard.

💡 Suggested Change

Before:

    const traces = await deps.core.listTraces({ limit: 2_000, offset: 0, includeAllNamespaces: true });

After:

    const traces = await deps.core.listTraces({ limit: 2_000, offset: 0, includeAllNamespaces: true });
    if (traces.length === 2_000) {
      console.warn("[metrics] listTraces hit the 2_000 row limit; aggregation may be incomplete");
    }

---

65. apps/memos-local-plugin/tests/helpers/fake-llm.ts (L67)

The parameter name o in the cast (x: unknown, o?: unknown) => unknown shadows the outer const o declared two lines above (line 60). While this has no runtime effect — the inner o is only part of the type cast expression and is never referenced — it is a misleading naming collision that could confuse readers into thinking the outer o is being rebound. Consider using a distinct name in the cast, e.g. (x: unknown, _opts?: unknown) => unknown, to make the intent clear.

💡 Suggested Change

Before:

        ? await (entry as (x: unknown, o?: unknown) => unknown)(input, opts)

After:

        ? await (entry as (x: unknown, _opts?: unknown) => unknown)(input, opts)

---

66. examples/basic_modules/llm.py (L59)

A whitespace-only value (e.g. export OPENAI_API_KEY=" ") is truthy, so the if _openai_key: guard passes but the API call fails with a cryptic authentication error instead of the intended graceful skip.

Normalize the value before the truthiness check:

💡 Suggested Change

Before:

_openai_key = os.getenv("OPENAI_API_KEY")

After:

_openai_key = (os.getenv("OPENAI_API_KEY") or "").strip() or None

---

67. examples/core_memories/general_textual_memory.py (L19)

OPENAI_BASE_URL is the standard environment variable used by the official OpenAI Python SDK (v1+). Dropping it means users who export OPENAI_BASE_URL (e.g., to point at a proxy, Azure, or a local server) will silently fall back to https://api.openai.com/v1 instead of their intended endpoint. The previous two-level fallback preserved compatibility with both naming conventions. Consider retaining it:

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

---

68. packages/memos-core/src/ingest/providers/anthropic.ts (L369-L374)

The inline comment above says 'cfg.headers is spread FIRST so user-supplied headers cannot override the credential', but the actual code contradicts this: Content-Type is placed before ...cfg.headers, meaning a caller-supplied cfg.headers["Content-Type"] will overwrite it. The credentials (x-api-key, anthropic-version) are correctly protected by appearing last, but the comment is factually wrong about Content-Type. An incorrect comment here is actively misleading — a future reader may trust the comment and believe Content-Type is safe. Either fix the comment to accurately describe what is protected vs. overridable, or move Content-Type after the spread as well if it must also be locked.

💡 Suggested Change

Before:

  const headers: Record<string, string> = {
    "Content-Type": "application/json",
    ...cfg.headers,
    "x-api-key": cfg.apiKey,
    "anthropic-version": "2023-06-01",
  };

After:

  const headers: Record<string, string> = {
    // cfg.headers is spread first; Content-Type, x-api-key and anthropic-version
    // are placed after so they cannot be overridden by caller-supplied headers.
    ...cfg.headers,
    "Content-Type": "application/json",
    "x-api-key": cfg.apiKey,
    "anthropic-version": "2023-06-01",
  };

---

69. packages/memos-core/src/ingest/providers/anthropic.ts (L389-L392)

The raw API response body is interpolated directly into the thrown Error message. Anthropic error payloads can reflect fragments of the original request in their error.message field. If a request includes sensitive context (e.g., user data embedded in taskState/newMessage), that content may end up in error logs or propagated to callers. Truncate or sanitize the body before including it in the error, or limit it to a fixed number of characters.

💡 Suggested Change

Before:

  if (!resp.ok) {
    const body = await resp.text();
    throw new Error(`Anthropic ${errorLabel} failed (${resp.status}): ${body}`);
  }

After:

  if (!resp.ok) {
    const body = await resp.text();
    // Truncate to avoid leaking large/sensitive error payloads into logs.
    const truncated = body.length > 300 ? `${body.slice(0, 300)}…` : body;
    throw new Error(`Anthropic ${errorLabel} failed (${resp.status}): ${truncated}`);
  }

---

70. packages/memos-core/src/ingest/providers/anthropic.ts (L431-L433)

maxTokens: 10 is extremely tight for a prompt that instructs the model to reply 'NEW or SAME'. Any leading whitespace, a brief preamble token, or model-side variation can easily consume those tokens, causing the response to be truncated or empty. When that happens, answer.startsWith("NEW") is false and the function silently returns "SAME" — indistinguishable in logs from a legitimate same-topic result. At minimum, add a log.warn when the raw answer does not equal the expected values, so operators can detect degraded model responses.

💡 Suggested Change

Before:

  const answer = text.toUpperCase();
  log.debug(`Topic arbitration result: "${answer}"`);
  return answer.startsWith("NEW") ? "NEW" : "SAME";

After:

  const answer = text.toUpperCase();
  log.debug(`Topic arbitration result: "${answer}"`);
  if (!answer.startsWith("NEW") && !answer.startsWith("SAME")) {
    log.warn(`Topic arbitration: unexpected response "${answer}", defaulting to SAME`);
  }
  return answer.startsWith("NEW") ? "NEW" : "SAME";

---

71. packages/memos-core/src/ingest/providers/anthropic.ts (L368)

The default model "claude-3-haiku-20240307" is an inline string literal. Anthropic periodically deprecates dated snapshot names, so callers relying on this fallback will start receiving API errors with no clear path to reconfigure. Define this as a named constant (e.g., DEFAULT_ANTHROPIC_TOPIC_MODEL) near the top of the file or alongside the other provider defaults, so it can be updated in one place.

💡 Suggested Change

Before:

  const model = cfg.model ?? "claude-3-haiku-20240307";

After:

  const DEFAULT_ANTHROPIC_TOPIC_MODEL = "claude-3-haiku-20240307";
  const model = cfg.model ?? DEFAULT_ANTHROPIC_TOPIC_MODEL;

---

72. packages/memos-core/src/ingest/providers/gemini.ts (L369-L371)

Security: The API key is embedded in the URL as a query parameter, which exposes it to URL-based logging (server logs, proxy logs, browser history). While this pattern matches the existing judgeDedupGemini function in this file, consider using the x-goog-api-key HTTP header instead to keep secrets out of URLs. This would require a coordinated fix across all Gemini helpers in this file.

---

73. packages/memos-core/src/ingest/providers/gemini.ts (L388-L391)

Security: The raw Gemini API error response body is concatenated directly into the error message. This can expose quota identifiers, project IDs, or other sensitive metadata to application logs. Consider logging the body separately at a controlled log level and throwing a sanitized error message instead.

---

74. packages/memos-core/src/ingest/providers/gemini.ts (L422-L428)

The maxOutputTokens value of 10 is very tight for this operation. While the prompt expects a single word ('NEW' or 'SAME'), if the model produces slightly verbose output or adds punctuation/spaces, the response could be truncated mid-token, resulting in an unrecognized string that silently defaults to 'SAME'. Consider increasing to at least 16-32 tokens to provide a safety margin, and add a truncation guard if needed.

---

75. packages/memos-core/src/ingest/providers/gemini.ts (L429-L436)

Control flow issue: The empty string check logs a warning but does not early-return, causing the code to continue to the final ternary expression. This makes the warning semantically correct but the control flow harder to follow. Consider restructuring with early returns after each warning case for clarity:

if (!answer) {
  log.warn("...");
  return "SAME";
}
if (!answer.startsWith("NEW") && !answer.startsWith("SAME")) {
  log.warn(...);
  return "SAME";
}
return answer.startsWith("NEW") ? "NEW" : "SAME";

💡 Suggested Change

Before:

  const answer = text.toUpperCase();
  log.debug(`Topic arbitration result: "${answer}"`);
  if (!answer) {
    log.warn("Gemini topic-arbitration returned empty text; defaulting to SAME");
  } else if (!answer.startsWith("NEW") && !answer.startsWith("SAME")) {
    log.warn(`Gemini topic-arbitration returned unexpected value "${answer}"; defaulting to SAME`);
  }
  return answer.startsWith("NEW") ? "NEW" : "SAME";

After:

  const answer = text.toUpperCase();
  log.debug(`Topic arbitration result: "${answer}"`);
  if (!answer) {
    log.warn("Gemini topic-arbitration returned empty text; defaulting to SAME");
    return "SAME";
  }
  if (!answer.startsWith("NEW") && !answer.startsWith("SAME")) {
    log.warn(`Gemini topic-arbitration returned unexpected value "${answer}"; defaulting to SAME");
    return "SAME";
  }
  return answer.startsWith("NEW") ? "NEW" : "SAME";

---

76. packages/memos-core/src/ingest/providers/gemini.ts (L397-L402)

Missing JSDoc documentation for this exported function. Consider adding documentation that describes the purpose, parameters (especially the cfg and log types), and return value semantics to improve discoverability and maintainability.

---

77. packages/memos-core/src/ingest/providers/gemini.ts (L415-L420)

Missing JSDoc documentation for this exported function. The return value is a string ('NEW' or 'SAME'), but this contract is not documented. Consider adding JSDoc that clarifies the return value semantics and the purpose of the arbitration step.

---

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

The refactored get_qwen_llm_config silently drops support for QWEN_TEMPERATURE, QWEN_MAX_TOKENS, QWEN_TOP_P, QWEN_TOP_K, and QWEN_REMOVE_THINK_PREFIX env vars that the old inline dict honored. Production deployments that tuned the Qwen LLM via those variables will silently get the hardcoded defaults (e.g. a configured QWEN_TEMPERATURE=1.9 becomes 0.8 with no warning).

This is confirmed by test_qwen_llm_only_uses_model_and_provider_endpoint_env, which asserts the env vars are intentionally ignored. If this consolidation is final, the removed variables should be documented as deprecated/removed in a changelog or migration guide to avoid silent misconfiguration for existing users.

---

79. src/memos/api/config.py (L425-L429)

The old code resolved backup credentials as os.getenv("MEMREADER_GENERAL_API_KEY", os.getenv("OPENAI_API_KEY", "EMPTY")), respecting a dedicated per-endpoint override. The new code reads only from OPENAI_API_KEY/QWEN_API_KEY via _build_provider_llm_config, silently discarding MEMREADER_GENERAL_API_KEY and MEMREADER_GENERAL_API_BASE.

Users who pointed the backup model at a separate provider endpoint via those variables (e.g. a self-hosted proxy with different auth) will silently start using the shared provider credentials, causing all backup calls to fail or hit an unintended endpoint. test_memreader_backup_uses_provider_env_for_general_model confirms this is the new intended behavior, but the change is backwards-incompatible without any deprecation notice.

---

80. src/memos/api/handlers/search_handler.py (L608-L620)

Silent data loss when candidate_limit == 0 (i.e., target_top_k == 0): selected is never populated, so bucket["memories"] is unconditionally overwritten with [], discarding every candidate before the downstream MMR step. A caller signalling 'skip this memory type' with top_k=0 will silently get empty results. Add an early-continue to preserve the original bucket when the limit is zero:

if candidate_limit == 0:
    continue

💡 Suggested Change

Before:

                selected: list[dict[str, Any]] = []
                if candidate_limit > 0:
                    for typed_memories in memories_by_type.values():
                        selected.extend(
                            sorted(
                                typed_memories,
                                key=self._mmr_candidate_score,
                                reverse=True,
                            )[:candidate_limit]
                        )
                    selected.sort(key=self._mmr_candidate_score, reverse=True)

                bucket["memories"] = selected

After:

                selected: list[dict[str, Any]] = []
                if candidate_limit == 0:
                    continue  # preserve existing memories; skip pruning for this bucket
                for typed_memories in memories_by_type.values():
                    selected.extend(
                        sorted(
                            typed_memories,
                            key=self._mmr_candidate_score,
                            reverse=True,
                        )[:candidate_limit]
                    )
                selected.sort(key=self._mmr_candidate_score, reverse=True)

                bucket["memories"] = selected

---

81. src/memos/api/handlers/search_handler.py (L606-L618)

The per-type slicing to candidate_limit is applied independently for each memory type, so if there are N distinct memory_type values in a bucket the final selected list can contain up to N × candidate_limit entries. The docstring promises 'at most twice the final quota for each memory type in each bucket', but in practice the total can be a multiple of that limit. After the per-type selection, apply a global cap on selected to enforce the bucket-level guarantee:

selected = selected[:candidate_limit]

💡 Suggested Change

Before:

                memories_by_type.setdefault(str(memory_type or result_key), []).append(memory)

                selected: list[dict[str, Any]] = []
                if candidate_limit > 0:
                    for typed_memories in memories_by_type.values():
                        selected.extend(
                            sorted(
                                typed_memories,
                                key=self._mmr_candidate_score,
                                reverse=True,
                            )[:candidate_limit]
                        )
                    selected.sort(key=self._mmr_candidate_score, reverse=True)

After:

                memories_by_type.setdefault(str(memory_type or result_key), []).append(memory)

                selected: list[dict[str, Any]] = []
                if candidate_limit == 0:
                    continue
                for typed_memories in memories_by_type.values():
                    selected.extend(
                        sorted(
                            typed_memories,
                            key=self._mmr_candidate_score,
                            reverse=True,
                        )[:candidate_limit]
                    )
                # Global cap: the total across all types must not exceed candidate_limit
                selected = sorted(selected, key=self._mmr_candidate_score, reverse=True)[
                    :candidate_limit
                ]

---

82. src/memos/api/handlers/search_handler.py (L642-L646)

metadata.get("score", ...) uses the fallback only when the score key is absent. If the key is present but its value is None, the fallback to relativity is never reached, and float(None or 0.0) silently returns 0.0. A memory with score=None but a high relativity is incorrectly scored as 0 and may be pruned when it should be retained. Use explicit None checking instead:

💡 Suggested Change

Before:

        score = metadata.get("score", metadata.get("relativity", 0.0))
        try:
            return float(score or 0.0)
        except (TypeError, ValueError):
            return 0.0

After:

        score = metadata.get("score")
        if score is None:
            score = metadata.get("relativity", 0.0)
        try:
            return float(score or 0.0)
        except (TypeError, ValueError):
            return 0.0

---

83. src/memos/api/handlers/search_handler.py (L357-L361)

The return value of _prune_mmr_candidates_by_bucket is ignored. The method mutates results in-place and returns it, creating an inconsistency: if a future refactor introduces a copy inside the method, the caller will silently stop seeing pruned data. Either drop the return value (change the return type to None) or assign it here to make the intent unambiguous.

💡 Suggested Change

Before:

        self._prune_mmr_candidates_by_bucket(
            results,
            text_top_k=text_top_k,
            pref_top_k=pref_top_k,
        )

After:

        results = self._prune_mmr_candidates_by_bucket(
            results,
            text_top_k=text_top_k,
            pref_top_k=pref_top_k,
        )

---

84. src/memos/configs/mem_scheduler.py (L351-L361)

Including MEMSCHEDULER_OPENAI_DEFAULT_MODEL in the OpenAI config detection can lead to initializing an OpenAI config with an empty API key. If only MEMSCHEDULER_OPENAI_DEFAULT_MODEL is set (without OPENAI_API_KEY), from_env() returns a config with api_key="", which then gets written to os.environ["OPENAI_API_KEY"] via set_openai_config_to_environment(). This could cause API calls to fail with empty-key errors. Consider requiring at least OPENAI_API_KEY or OPENAI_API_BASE to be present before initializing the config, since a model override alone doesn't constitute valid OpenAI credentials.

💡 Suggested Change

Before:

        try:
            has_openai_env = any(
                key in os.environ
                for key in [
                    "OPENAI_API_KEY",
                    "OPENAI_API_BASE",
                    "MEMSCHEDULER_OPENAI_DEFAULT_MODEL",
                ]
            )
            if has_openai_env:
                openai_config = OpenAIConfig.from_env()

After:

        try:
            has_openai_env = any(
                key in os.environ
                for key in [
                    "OPENAI_API_KEY",
                    "OPENAI_API_BASE",
                ]
            )
            if has_openai_env:
                openai_config = OpenAIConfig.from_env()

---

85. apps/memos-local-plugin/core/capture/capture.ts (L1477-L1485)

Zero-limit silently disables all LLM reflection. When configuredLimit is 0 (or any value that floors to 0), limit = 0 and the first tryUse call evaluates used >= limit as 0 >= 0 = true, immediately returning false for every invocation. This means the entire reflect phase silently falls back to heuristic/no-LLM output. The design intent of 0 is ambiguous — it could mean "no limit" (unbounded) or "disable all calls". At minimum, 0 should be treated as unbounded (e.g., Infinity) to avoid accidental silent misconfiguration, and an explicit sentinel like -1 or a separate enabled flag should gate disabling.

💡 Suggested Change

Before:

  const limit = Math.max(0, Math.floor(configuredLimit));
  let used = 0;
  let stopped = false;
  let exhaustedWarned = false;
  let terminalWarned = false;

  return {
    tryUse(stage) {
      if (stopped || used >= limit) {

After:

  // Treat 0 / negative as "no limit" (unlimited); use a large sentinel.
  const limit = configuredLimit > 0 ? Math.floor(configuredLimit) : Number.MAX_SAFE_INTEGER;
  let used = 0;
  let stopped = false;
  let exhaustedWarned = false;
  let terminalWarned = false;

  return {
    tryUse(stage) {
      if (stopped || used >= limit) {

---

86. apps/memos-local-plugin/core/capture/capture.ts (L1319-L1328)

Downstream context is always silently dropped in the chunk fallback. The third argument to buildReflectionContexts is chunk.map(() => []) — an array of empty arrays — meaning downstream preview context is never passed for any step when a chunk falls back to per-step scoring. In the non-chunked path, downstreamByStep (computed from the full normalized array in the outer scope) is used. Since runChunkedBatchScoring is a module-level function and not a closure, it has no access to downstreamByStep. This silently degrades reflection quality for all long episodes that hit the chunk-batch path without any warning logged.

💡 Suggested Change

Before:

    return runPerStepScoring(
      chunk,
      llm,
      deps,
      warnings,
      llmCalls,
      episodeId,
      buildReflectionContexts(chunk, taskSummary, chunk.map(() => [])),
      budget,
    );

After:

    // Pass the downstream contexts slice for this chunk.
    // The caller should provide the relevant subslice of downstreamByStep.
    return runPerStepScoring(
      chunk,
      llm,
      deps,
      warnings,
      llmCalls,
      episodeId,
      chunkDownstreamContexts, // passed in from caller who has downstreamByStep
      budget,
    );

---

87. apps/memos-local-plugin/core/capture/capture.ts (L1513-L1517)

Non-MemosError instances are classified as terminal based on substring matching of their .message. Any transient error from a network library, timeout wrapper, or third-party SDK whose message incidentally contains strings like "unauthorized" or "billing" will set stopped = true permanently, halting all remaining reflect LLM calls for the entire episode with no recovery. The stopped flag is never reset. This is especially risky for generic JavaScript errors that wrap lower-level HTTP responses as message strings. Non-MemosError types should be treated as non-terminal by default, or require a much narrower match (e.g., exact status codes).

💡 Suggested Change

Before:

function isTerminalReflectLlmError(err: unknown): boolean {
  if (!(err instanceof MemosError)) {
    const msg = err instanceof Error ? err.message : String(err);
    return terminalMessage(msg);
  }

After:

function isTerminalReflectLlmError(err: unknown): boolean {
  // Only classify MemosError instances as terminal; unknown error types
  // are assumed transient to avoid permanently halting reflection.
  if (!(err instanceof MemosError)) return false;
  if (err.code !== ERROR_CODES.LLM_UNAVAILABLE) return false;
  const details = (err.details ?? {}) as Record<string, unknown>;
  if (details.circuitOpen === true) return true;
  const status = Number(details.status);
  if (status === 401 || status === 402 || status === 403) return true;
  return terminalMessage(err.message);
}

---

88. apps/memos-local-plugin/core/capture/capture.ts (L1212-L1217)

The per_episode batchMode value is silently dropped. The old shouldBatch had an explicit branch if (cfg.batchMode === "per_episode") return true that always forced whole-episode batching regardless of step count. The new function removes that branch, so per_episode now falls through to the threshold expression — identical to auto. Users who configured per_episode expecting their entire episode to be passed as a single batch will now get chunked batching for large episodes without any indication that behavior changed. Add an explicit per_episode branch.

💡 Suggested Change

Before:

function planScoring(cfg: CaptureConfig, stepCount: number, hasLlm: boolean): ScoringPlan {
  if (!hasLlm) return "per_step";
  if (stepCount === 0) return "per_step";
  if (cfg.batchMode === "per_step") return "per_step";
  return stepCount <= Math.max(1, cfg.batchThreshold) ? "batch" : "chunk_batch";
}

After:

function planScoring(cfg: CaptureConfig, stepCount: number, hasLlm: boolean): ScoringPlan {
  if (!hasLlm) return "per_step";
  if (stepCount === 0) return "per_step";
  if (cfg.batchMode === "per_step") return "per_step";
  // per_episode: always pass the whole episode as one batch (legacy behavior).
  if (cfg.batchMode === "per_episode") return "batch";
  // auto: batch if within threshold, otherwise chunk.
  return stepCount <= Math.max(1, cfg.batchThreshold) ? "batch" : "chunk_batch";
}

---

89. apps/memos-local-plugin/core/capture/capture.ts (L1306-L1329)

Nested runConcurrently calls multiply concurrency. The outer call processes up to llmConcurrency chunks simultaneously. When a chunk's runBatchScoring fails and falls back to runPerStepScoring, that function itself calls runConcurrently with the same llmConcurrency value. In the worst case (all chunks fail batch), llmConcurrency × llmConcurrency LLM calls can be in-flight simultaneously — far exceeding the configured cap. The ReflectLlmBudget limits total call count but does not enforce concurrency. Consider passing a reduced inner concurrency (e.g., 1) or sharing a global semaphore across both levels.

💡 Suggested Change

Before:

  const concurrency = Math.max(1, deps.cfg.llmConcurrency);
  const scoredChunks = await runConcurrently(chunks, concurrency, async (chunk): Promise<ScoredStep[]> => {
    const scored = await runBatchScoring(
      chunk,
      llm,
      deps,
      warnings,
      llmCalls,
      episodeId,
      taskSummary,
      budget,
    );
    if (scored.length > 0) return scored;
    return runPerStepScoring(
      chunk,
      llm,
      deps,
      warnings,
      llmCalls,
      episodeId,
      buildReflectionContexts(chunk, taskSummary, chunk.map(() => [])),
      budget,
    );
  });

After:

  const concurrency = Math.max(1, deps.cfg.llmConcurrency);
  // Use concurrency=1 for per-step fallback inside each chunk to avoid
  // multiplying concurrency by the outer chunk parallelism.
  const innerConcurrency = 1;
  const scoredChunks = await runConcurrently(chunks, concurrency, async (chunk): Promise<ScoredStep[]> => {
    const scored = await runBatchScoring(chunk, llm, deps, warnings, llmCalls, episodeId, taskSummary, budget);
    if (scored.length > 0) return scored;
    return runPerStepScoring(
      chunk, llm, deps, warnings, llmCalls, episodeId,
      buildReflectionContexts(chunk, taskSummary, chunk.map(() => [])),
      budget,
      innerConcurrency,
    );
  });

---

90. apps/memos-local-plugin/core/capture/capture.ts (L1000-L1003)

The recovery detection is overly broad. The previous code checked explicitly against RECOVERY_REASONS.DIRTY_REWARD_RESCORE — only that specific recovery type triggered the orphan-insert throttle. The new check typeof meta.recoveryReason === "string" matches any episode with any recovery reason, including future recovery types that may not share the same duplicate-trace risk. This implicitly constrains the design of all future recovery types to either avoid setting recoveryReason, or accept the maxRecoveryOrphanInserts cap. At minimum, document which recovery reasons legitimately need this guard, or maintain an allowlist.

---

91. apps/memos-local-plugin/core/util/rate-limited-llm.ts (L65-L75)

The semaphore slot is acquired before the first chunk is yielded and held for the entire lifetime of the stream — including all idle time while the remote model generates tokens. For slow or long-running streams this monopolizes one concurrency slot for seconds or minutes, starving other callers queued on the same semaphore.

If streaming is only used in latency-insensitive background paths and the slot count (bgLlmConcurrency) is sized for this, it may be acceptable — but the behaviour should be documented explicitly. If there's any latency-sensitive streaming usage in the future, consider releasing the slot immediately after the stream iterator is returned and tracking active streams separately.

---

92. apps/memos-local-plugin/core/util/rate-limited-llm.ts (L41-L51)

The semaphore.acquire() call (same applies to completeJson and stream) will block indefinitely if all slots are occupied by stalled or very slow calls. The Semaphore interface in semaphore.ts exposes no timeout or AbortSignal parameter, so there is currently no way for a caller to bound its wait time or cancel it.

A single hung LLM call could cause all downstream callers to queue silently forever, with no observable error surfaced. Consider adding an optional timeout to Semaphore.acquire(), for example:

acquire(timeoutMs?: number): Promise<() => void>;

and rejecting with a descriptive error when the deadline is exceeded.

---

93. apps/memos-local-plugin/core/util/rate-limited-llm.ts (L85-L87)

The close() method tears down the inner client immediately without waiting for in-flight semaphore-protected calls (complete, completeJson, stream) to finish. If close() is called while any of those methods holds a semaphore slot, the inner client can be destroyed mid-request, potentially causing unhandled promise rejections or silent data loss.

A safe shutdown should drain the semaphore first — i.e., wait until current reaches zero — before forwarding to inner.close(). One approach is to expose a drain() helper on the semaphore that resolves once all acquired slots have been released, then await drain() here before tearing down.

---

94. apps/memos-local-plugin/core/pipeline/memory-core.ts (L663)

A single shared KV cursor key is used by two independent callers: autoRescoreDirtyClosedEpisodes() (periodic) and the init() startup path. Each call to collectDirtyClosedEpisodes() reads the cursor, fetches one page, and then writes the next cursor position. When both callers run within the same startup window — init() at startup and autoRescoreDirtyClosedEpisodes() firing within 30 s — they interleave pages on the same cursor: init() consumes page N and writes cursor N, then autoRescore reads cursor N and consumes page N+1. Neither caller completes a full pass over all dirty episodes in its intended scan.

Since the two consumers have different lifetimes and intents (one-shot init vs. recurring periodic), they should use separate cursor keys, or collectDirtyClosedEpisodes should accept a caller-specific cursor key as a parameter.

---

95. apps/memos-local-plugin/core/pipeline/memory-core.ts (L1570-L1574)

When a stored cursor fails the isDirtyClosedScanCursor validation (non-null but invalid shape), the KV key is silently deleted and the scan restarts from the beginning with no diagnostic log. This makes it impossible to detect cursor corruption or schema-version mismatches in production logs. Add a log.warn or log.debug call before kv.del to surface these resets.

---

96. apps/memos-local-plugin/core/pipeline/memory-core.ts (L3285-L3289)

The limit: 100_000 passed to episodes.list() relies on the repo's internal clampLimit silently capping the fetch to 500 rows — a cross-layer contract that is only documented in the comment above, not enforced here. If clampLimit is relaxed or removed in a future refactor, this silently becomes a full-table scan. Use a named constant (e.g. EPISODE_LIST_UNBOUNDED_FETCH_LIMIT) that matches the actual clamp value, and add an assertion or a known-safe upper bound to make the contract explicit at the call site rather than relying on a side-effect in the repo layer.

---

97. apps/memos-local-plugin/core/pipeline/memory-core.ts (L3909-L3911)

The traces fetch is always unfiltered (all namespaces) regardless of the includeAllNamespaces flag, while totalTurns a few lines below conditionally respects the flag. This creates an inconsistent contract: a caller passing includeAllNamespaces: false will still receive cross-namespace values for sessions, writesToday, embeddings, and dailyWrites, while total is namespace-scoped. The current metrics route always passes includeAllNamespaces: true, so the inconsistency is not currently reachable, but it makes the function signature misleading. Either document that the flag only applies to totalTurns, or thread a namespace filter through the traces.list() call when includeAllNamespaces is false.

---

98. .github/workflows/memos-local-plugin-publish.yml (L293-L299)

GitHub Actions delivers workflow_dispatch boolean inputs as the string 'true'/'false' at runtime, not as actual booleans. This means when a user triggers the workflow manually with dry_run checked:

• inputs.dry_run == true evaluates to false (string 'true' ≠ boolean true) → the notice step is skipped
• inputs.dry_run != true evaluates to true (string 'true' ≠ boolean true) → the publish step executes

Result: a workflow_dispatch dry run silently proceeds to publish to npm.

For workflow_call, boolean inputs retain their boolean type (so the comparison is correct there), but this only applies to the automated post-merge caller, not manual triggers.

Fix: use toJSON(inputs.dry_run) != 'true' which normalizes both string and boolean forms:

if: ${{ toJSON(inputs.dry_run) != 'true' }}

💡 Suggested Change

Before:

      - name: Stop before publish in dry run
        if: ${{ inputs.dry_run == true }}
        run: |
          echo "::notice::dry_run=true; skipped npm publish, tag, GitHub Release, and release PR."

      - name: Publish to npm
        if: ${{ inputs.dry_run != true }}

After:

      - name: Stop before publish in dry run
        if: ${{ toJSON(inputs.dry_run) == 'true' }}
        run: |
          echo "::notice::dry_run=true; skipped npm publish, tag, GitHub Release, and release PR."
          exit 1

      - name: Publish to npm
        if: ${{ toJSON(inputs.dry_run) != 'true' }}

---

99. .github/workflows/memos-local-plugin-publish.yml (L396-L397)

Same workflow_dispatch boolean/string type mismatch as on the publish step. When triggered manually with dry_run=true, inputs.dry_run != true evaluates to true (string 'true' ≠ boolean true), so this step also executes and attempts to create a release tag and PR.

Fix: use toJSON(inputs.dry_run) != 'true' consistently.

💡 Suggested Change

Before:

      - name: Create release tag and PR
        if: ${{ inputs.dry_run != true }}

After:

      - name: Create release tag and PR
        if: ${{ toJSON(inputs.dry_run) != 'true' }}

---

100. .github/workflows/memos-local-plugin-publish.yml (L90-L101)

The prebuild matrix job (darwin-arm64, darwin-x64, linux-x64, win32-x64) only installs dependencies and compiles the native better-sqlite3 binary. It does not use git history, tags, or changelog generation. Adding fetch-depth: 0 fetches the full repository history on all four matrix runners on every run, adding unnecessary time and bandwidth overhead. Full history is only required in the publish job (for tag resolution and release notes generation).

💡 Suggested Change

Before:

      - uses: actions/checkout@v4
        with:
          ref: ${{ inputs.git_ref || github.ref }}
          fetch-depth: 0

      - uses: actions/setup-node@v4
        with:
          node-version: 22

      - name: Install dependencies
        shell: bash
        run: bash ../../.github/scripts/retry.sh --label "npm ci" -- npm ci

After:

      - uses: actions/checkout@v4
        with:
          ref: ${{ inputs.git_ref || github.ref }}

      - uses: actions/setup-node@v4
        with:
          node-version: 22

      - name: Install dependencies
        shell: bash
        run: bash ../../.github/scripts/retry.sh --label "npm ci" -- npm ci

---

101. .github/workflows/memos-local-plugin-publish.yml (L76-L83)

macos-15-intel is not a documented GitHub-hosted runner label. Standard GitHub-hosted macOS runners are macos-14, macos-15, macos-13, etc. If this is not a registered self-hosted runner label in this repository, the darwin-x64 matrix leg will queue indefinitely or fail with a confusing 'no runner available' error. The previous value was macos-15, which is a valid GitHub-hosted label. If the intent is to use the x86-64 architecture on macOS 15, note that the arch -x86_64 Rosetta step on the next line already handles the cross-compilation — a stock macos-15 runner (arm64) running Rosetta can produce the x64 binary without needing a native Intel runner.

---

102. .github/workflows/memos-local-plugin-publish.yml (L76-L81)

Neither build-prebuilds nor publish declares a timeout-minutes limit. The publish job now contains multiple layered retry loops (npm view ×3, npm publish ×3, git ls-remote ×3, gh release ×3, gh pr ×3) each with exponential sleep delays. A hung network call or unresponsive registry could block a paid runner for an unbounded duration. Adding a generous but finite timeout (e.g., 30 minutes for prebuild, 60 minutes for publish) provides a safety bound.

💡 Suggested Change

Before:

  build-prebuilds:
    strategy:
      matrix:
        include:
          - os: macos-14
            platform: darwin-arm64

After:

  build-prebuilds:
    timeout-minutes: 30
    strategy:
      matrix:
        include:
          - os: macos-14
            platform: darwin-arm64

---

103. src/memos/embedders/base.py (L37-L58)

Two issues in this finally block:

1. text_hash(normalized_texts) is called unconditionally. If it raises (e.g. due to unexpected input), the exception it throws will replace any in-flight exception from func, losing the original traceback entirely. Wrap the logging call in a try/except to prevent this.

2. On the failure path (error_type is not None), logger.info is used instead of logger.warning or logger.error. Failures logged at INFO level will be invisible when the log level is set to WARNING or above, silently hiding embedding errors.

Suggested fix:

finally:
    elapsed_ms = (time.perf_counter() - started_at) * 1000
    log_message = (
        "Embedding request model=%s backup_model=%s backup_enabled=%s "
        "batch_size=%d total_chars=%d max_chars=%d text_hash=%s "
        "elapsed_ms=%.2f status=%s"
    )
    try:
        _text_hash = text_hash(normalized_texts)
    except Exception:
        _text_hash = "error"
    log_values = (
        model,
        backup_model,
        backup_enabled,
        len(normalized_texts),
        sum(text_lengths),
        max(text_lengths, default=0),
        _text_hash,
        elapsed_ms,
        status,
    )
    if error_type is None:
        logger.info(log_message, *log_values)
    else:
        logger.warning(log_message + " error_type=%s", *log_values, error_type)

---

104. src/memos/embedders/cache.py (L173-L178)

request_cache is a local reference captured inside the initial with self._lock block. The thread then blocks on future.result() for an unbounded time with the lock released. Because _request_caches is a TTLCache, the entry for request_id can expire and be evicted while the thread is blocked. The write to request_cache[text] = vector then succeeds silently on an orphaned LRUCache object that is no longer in _request_caches, so intra-request deduplication silently stops working for long-running singleflight waits.

Re-fetch the live per-request cache from _request_caches after future.result() returns, inside the lock, rather than reusing the pre-blocking reference.

---

105. src/memos/embedders/cache.py (L95-L96)

__getattr__ is only called when the attribute is not found through normal means. Accessing self._backend inside it will itself trigger __getattr__('_backend') if _backend is not yet in self.__dict__ — for example during unpickling (before __setstate__ restores the dict) or if a subclass calls a method before super().__init__ runs. This causes infinite recursion and a RecursionError.

Guard against the bootstrap case:

def __getattr__(self, name: str) -> Any:
    if name == '_backend':
        raise AttributeError(name)
    return getattr(self._backend, name)

---

106. src/memos/embedders/cache.py (L88)

self._request_cache_text_limit — which controls the maxsize of each per-request LRUCache — is set to cache_max_size (read from MEMOS_EMBEDDING_CACHE_MAX_SIZE, default 4096). This repurposes the global TTL-cache size limit as a per-request text limit. In the worst case this allocates up to _DEFAULT_CACHE_MAX_SIZE × _DEFAULT_REQUEST_CACHE_MAX_REQUESTS (4 096 × 1 024 ≈ 4 M) text slots. If a dedicated per-request limit was intended, introduce a separate env var (e.g. MEMOS_EMBEDDING_REQUEST_CACHE_MAX_TEXTS) with a sensibly smaller default.

---

107. src/memos/embedders/cache.py (L180-L183)

This INFO-level log is emitted on every embed call when optimization is enabled, which in a high-throughput system produces a log line per embedding request. Consider downgrading to DEBUG to avoid flooding production logs and the associated formatting overhead.

---

108. src/memos/embedders/universal_api.py (L85-L86)

The actual exception message e is now dropped from the warning log — only the error type name is recorded. This loses the specific reason for failure (e.g., timeout message, connection refused details, HTTP status), making it significantly harder to diagnose production issues. The old log included both type(e).__name__ and the exception message. Add e as a second %s argument.

💡 Suggested Change

Before:

                        "Embedding request failed error_type=%s; trying backup client",
                        type(e).__name__,

After:

                        "Embedding request failed error_type=%s error=%s; trying backup client",
                        type(e).__name__,
                        e,

---

109. apps/memos-local-plugin/server/routes/overview.ts (L50-L74)

These 7 RPCs are redundant. The metrics() call already in this same Promise.all returns skillStats (total/active/candidate/archived), policyStats (total/active/candidate/archived), and worldModelCount — confirmed in the MemoryCore contract. Issuing 7 extra parallel DB queries duplicates that computation, increases DB load, and introduces a subtle TOCTOU inconsistency: counts from countSkills/countPolicies are snapshotted at a different moment from metrics(), so skillStats.total on the overview response could diverge from metrics.skillStats.total if a record transitions status between the two reads.

Consider removing these 7 calls and deriving the stats directly from the metrics result:

const [health, episodeCount, metrics] = await Promise.all([
  deps.core.health(),
  deps.core.countEpisodes({ includeAllNamespaces: true }),
  deps.core.metrics({ days: 1, includeAllNamespaces: true }),
]);
// then:
const skillStats = metrics.skillStats;   // already has total/active/candidate/archived
const policyStats = metrics.policyStats; // already has total/active/candidate/archived
const worldModelCount = metrics.worldModelCount;

This reduces 10 parallel RPCs to 3 and keeps all aggregate numbers consistent within a single metrics() snapshot. If metrics.skillStats / metrics.policyStats are not yet correctly scoped by includeAllNamespaces, the right fix is to thread the flag through metrics() internals — not to add 6 parallel count calls.

---

110. src/memos/log.py (L99)

The or operator substitutes "unknown" for any falsy memory_type, not just None. An empty string "" is a plausible field value (e.g. a memory record whose memory_type column was stored as an empty string), and it would be silently bucketed as "unknown" instead of "". Use an explicit None check to distinguish between "not set" and "set to an empty/falsy value".

---

111. src/memos/mem_reader/read_multi_modal/multi_modal_parser.py (L63)

Regression: document_parser_llm has no fallback to llm, unlike image_parser_llm. Callers that pass only llm=... (without the new document_parser_llm) will get None forwarded to FileContentParser, silently disabling all document LLM extraction. Apply the same fallback pattern used for image_parser_llm.

💡 Suggested Change

Before:

        self.document_parser_llm = document_parser_llm

After:

        self.document_parser_llm = document_parser_llm if document_parser_llm is not None else llm

---

112. src/memos/mem_scheduler/analyzer/eval_analyzer.py (L57)

OPENAI_API_BASE is a legacy (v0.x SDK / LiteLLM) convention. The current OpenAI Python SDK v1.x reads OPENAI_BASE_URL automatically. A user who follows current OpenAI documentation and sets OPENAI_BASE_URL will have their base URL silently ignored here, causing the client to fall back to the default OpenAI endpoint unexpectedly. Use OPENAI_BASE_URL to match the SDK standard:

base_url=openai_base_url or os.getenv("OPENAI_BASE_URL"),

If OPENAI_API_BASE is intentionally adopted as a project-wide convention (e.g., for LiteLLM/proxy compatibility), document it explicitly so users know not to use OPENAI_BASE_URL.

💡 Suggested Change

Before:

            base_url=openai_base_url or os.getenv("OPENAI_API_BASE"),

After:

            base_url=openai_base_url or os.getenv("OPENAI_BASE_URL"),

---

113. src/memos/mem_reader/read_multi_modal/file_content_parser.py (L121-L127)

When filename was originally empty, line 115 already sets it to os.path.basename(parsed_url.path), which means file_ext is derived from the same raw (percent-encoded) path that url_path_ext will decode. For a URL like https://example.com/doc%2Emd, os.path.basename gives doc%2Emd, so file_ext = ".md" (splitext works on the literal dot). However for https://example.com/doc%2Freadme%2Emd (encoded slash+dot), basename of the raw path /doc%2Freadme%2Emd gives doc%2Freadme%2Emd, and file_ext = ".md" still. The real gap is a URL whose path has no dot but the decoded form reveals an extension — e.g. https://example.com/notes%2Emd. In this case file_ext from filename is ".md" anyway, so url_path_ext is redundant. The fix is correct and safe; just note that the added value is specifically for URLs where the caller-supplied filename argument is empty string and the URL path extension is percent-encoded (e.g. /path/note%2Emd). This is a valid improvement, but the intent could be clearer with a comment.

💡 Suggested Change

Before:

url_path_ext = os.path.splitext(unquote(parsed_url.path))[1].lower()
markdown_extensions = {".md", ".markdown", ".txt"}
if (
    file_ext in markdown_extensions
    or url_path_ext in markdown_extensions
    or self._is_oss_md(url_str)
):

After:

            # Decode percent-encoded path to catch extensions like %2Emd → .md
            # that os.path.splitext on the raw filename would miss.
            url_path_ext = os.path.splitext(unquote(parsed_url.path))[1].lower()
            markdown_extensions = {".md", ".markdown", ".txt"}
            if (
                file_ext in markdown_extensions
                or url_path_ext in markdown_extensions
                or self._is_oss_md(url_str)
            ):

---

114. src/memos/memories/textual/tree_text_memory/retrieve/recall.py (L387-L390)

Using is True for the supports_lightweight_vector_search attribute check is an identity comparison rather than a truthiness check. If any graph store implementation returns a truthy non-bool value (e.g., 1, a non-empty string, or another object) for this attribute, the condition evaluates to False and lightweight search is silently disabled with no warning. The review checklist flags this pattern as a correctness risk.

Use a plain truthiness check to correctly handle any truthy return value:

💡 Suggested Change

Before:

        use_lightweight_search = (
            _env_flag_enabled(_POLARDB_LIGHTWEIGHT_SEARCH_ENV)
            and getattr(self.graph_store, "supports_lightweight_vector_search", False) is True
        )

After:

        use_lightweight_search = (
            _env_flag_enabled(_POLARDB_LIGHTWEIGHT_SEARCH_ENV)
            and bool(getattr(self.graph_store, "supports_lightweight_vector_search", False))
        )

---

115. tests/api/test_mmr_candidate_pruning.py (L67-L72)

The set comparison loses ordering information. _prune_mmr_candidates_by_bucket ends with selected.sort(key=self._mmr_candidate_score, reverse=True), which produces a deterministic score-descending order across memory types. Given the fixture scores (long-1: 0.90, user-1: 0.85, long-2: 0.70, user-2: 0.65), the expected order is fully determined. Using a set means an ordering bug in the production sort would pass this test undetected. Use a list assertion to match the sorted order the implementation guarantees.

💡 Suggested Change

Before:

    assert {item["id"] for item in results["text_mem"][0]["memories"]} == {
        "long-1",
        "long-2",
        "user-1",
        "user-2",
    }

After:

    assert [item["id"] for item in results["text_mem"][0]["memories"]] == [
        "long-1",
        "user-1",
        "long-2",
        "user-2",
    ]

---

116. tests/api/test_llm_provider_config.py (L68-L70)

The env var set here (MEMRADER_MODEL) is never read by get_document_parser_llm_config(). The production implementation only checks DOCUMENT_PARSER_MODEL and MEMREADER_GENERAL_MODEL, so this test returns None trivially regardless of what MEMRADER_MODEL contains. The test never actually verifies that the function doesn't fall back to the main memreader model.

If the intent is to guard against a fallback to the main memreader (which uses a different env var), the test should set whichever env var the main memreader reads (e.g. via get_memreader_config()) and assert the result is still None. As written, the assertion passes vacuously and provides no meaningful coverage.

💡 Suggested Change

Before:

    monkeypatch.setenv("MEMRADER_MODEL", "qwen3-0.6B")

    assert APIConfig.get_document_parser_llm_config() is None

After:

def test_document_parser_model_does_not_fall_back_to_main_memreader(monkeypatch):
    monkeypatch.delenv("DOCUMENT_PARSER_MODEL", raising=False)
    monkeypatch.delenv("MEMREADER_GENERAL_MODEL", raising=False)
    # Explicitly leave MEMRADER_MODEL / any main-memreader env var unset (or set it)
    # to confirm get_document_parser_llm_config returns None when neither
    # DOCUMENT_PARSER_MODEL nor MEMREADER_GENERAL_MODEL is configured.

    assert APIConfig.get_document_parser_llm_config() is None

---

117. tests/api/test_llm_provider_config.py (L118-L120)

The test name states legacy task-scoped env vars (FEEDBACK_API_KEY, FEEDBACK_API_BASE) are ignored, but the test only asserts the provider-level values are present — it never asserts the legacy values are absent. A buggy implementation that set both (e.g. keeping a FEEDBACK_API_KEY key in the returned config alongside the correct api_key) would still pass all current assertions. Adding a negative assertion would make the contract explicit:

assert config["config"]["api_key"] != "legacy-feedback-key"

---

118. tests/embedders/test_base.py (L27-L29)

Missing explicit assertion that logger.info was called before accessing call_args. If the decorator never invokes logger.info, call_args is None and None.args raises AttributeError, producing a confusing test error rather than a clear assertion failure. The same pattern is repeated in the other two test functions.

Add mock_logger.info.assert_called_once() before accessing call_args so a missing log call produces an actionable failure message.

💡 Suggested Change

Before:

    log_args = mock_logger.info.call_args.args
    rendered = log_args[0] % log_args[1:]
    assert "model=embedding-model" in rendered

After:

    mock_logger.info.assert_called_once()
    log_args = mock_logger.info.call_args.args
    rendered = log_args[0] % log_args[1:]
    assert "model=embedding-model" in rendered

---

119. tests/embedders/test_cache.py (L143-L147)

The poll loop starts immediately after executor.submit(second) returns, but the second thread may not have been scheduled yet, so singleflight_joins can legitimately stay 0 for the entire 2-second window before the assertion fires as a spurious failure. singleflight_joins is only incremented once the second thread acquires self._lock and finds the text already in self._inflight, which can only happen after the backend's blocking call has started — but thread scheduling is not guaranteed.

A reliable fix is to block the second thread inside the lock until it has registered a join, or to add a second threading.Event that the CachingEmbedder (or a wrapper in the test) sets just before future.result() is called. A simpler but still robust approach: add a small sleep after backend_started.wait() and before submitting the second task to give the OS time to schedule it, and then increase the singleflight wait deadline accordingly. The current structure can produce intermittent CI failures on slow or heavily loaded runners.

💡 Suggested Change

Before:

        deadline = time.monotonic() + 2
        while embedder.cache_info()["singleflight_joins"] != 1:
            assert time.monotonic() < deadline
            time.sleep(0.01)
        release_backend.set()

After:

        deadline = time.monotonic() + 2
        # Give the OS a moment to schedule the second thread so it can
        # enter CachingEmbedder.embed and register a singleflight join
        # before we start polling; without this the poll may time out
        # spuriously on slow/loaded runners.
        time.sleep(0.05)
        while embedder.cache_info()["singleflight_joins"] != 1:
            assert time.monotonic() < deadline, "timed out waiting for singleflight join"
            time.sleep(0.01)
        release_backend.set()

---

120. tests/memories/textual/test_tree_retriever.py (L133-L136)

The fallback test omits user_name, so it defaults to None. In the production code, user_name is forwarded to both search_by_embedding and get_nodes. If the graph store filters by user_name during fallback, calling with None tests a different (and less realistic) code path than the one exercised by the first test. A bug where user_name is incorrectly threaded through the fallback would not be caught here. Add user_name="cube-a" (or any non-None value) to match the realistic scenario.

💡 Suggested Change

Before:

    results = retriever._vector_recall([[0.1] * 5], "LongTermMemory", top_k=5)

    assert results[0].memory == "fallback content"
    mock_graph_store.get_nodes.assert_called_once()

After:

    results = retriever._vector_recall([[0.1] * 5], "LongTermMemory", top_k=5, user_name="cube-a")

    assert results[0].memory == "fallback content"
    mock_graph_store.get_nodes.assert_called_once()

---

121. tests/memories/textual/test_tree_retriever.py (L136)

The assertion verifies only that get_nodes was called, not what it was called with. The entire correctness of the fallback path is that the node IDs extracted from the lightweight hits are forwarded to get_nodes. Without asserting the call arguments (specifically that node_id was passed), a bug where wrong or missing IDs are forwarded would produce a passing test. Use assert_called_once_with(...) to verify the expected node_id is included in the call.

💡 Suggested Change

Before:

    mock_graph_store.get_nodes.assert_called_once()

After:

    mock_graph_store.get_nodes.assert_called_once_with(
        node_ids=[node_id],
        user_name=mock.ANY,  # or the specific user_name if added above
    )

---

122. tests/graph_dbs/test_search_return_fields.py (L237-L239)

The 8th element 0.8 is the scope = (1 - scope_distance) column appended by the outer SELECT *, (1 - scope_distance) AS scope FROM t query. It is required to pass the expected_columns = 6 + len(return_fields) = 8 guard, but the production code never reads it — the score is derived from row[4] (scope_distance = 0.2), not from this column.

Without a comment, a future reader will likely treat 0.8 as the score source and expect (0.8 + 1) / 2 = 0.9 as the result, conflicting with the pytest.approx(0.6) assertion below. Add an inline comment to make the column layout explicit, e.g.:

cursor.fetchall.return_value = [
    # id, properties, timeline, old_id, scope_distance, return_memory, return_tags, scope=(1-dist)
    (1, None, None, '"node-1"', 0.2, '"hello"', '["tag-a"]', 0.8)
]

---

123. tests/graph_dbs/test_search_return_fields.py (L260-L262)

The expected score 0.6 is computed as (row[4] + 1) / 2 = (scope_distance + 1) / 2 = (0.2 + 1) / 2 = 0.6. However, row[7] = 0.8 in the mock tuple is the scope column (1 - scope_distance), which a reader would naturally expect to be the score source, yielding (0.8 + 1) / 2 = 0.9.

This discrepancy makes the assertion appear wrong at first glance. Add a comment tying the mock distance value to the expected score:

# score = (scope_distance + 1) / 2 = (0.2 + 1) / 2 = 0.6 (row[4] is scope_distance, not scope)
assert results == [
    {"id": "node-1", "score": pytest.approx(0.6), "memory": "hello", "tags": ["tag-a"]}
]

---

124. tests/reranker/test_local_rerankers.py (L101-L105)

next() without a default raises StopIteration if no matching log record is found (e.g., if the logger propagation is blocked or the log message changes). This makes the test error rather than fail with a meaningful message. Pass a default of None and assert it explicitly so the failure is clear.

💡 Suggested Change

Before:

    message = next(
        record.getMessage()
        for record in caplog.records
        if "CosineLocalReranker rerank result" in record.getMessage()
    )

After:

    message = next(
        (
            record.getMessage()
            for record in caplog.records
            if "CosineLocalReranker rerank result" in record.getMessage()
        ),
        None,
    )
    assert message is not None, "Expected 'CosineLocalReranker rerank result' log record was not emitted"

---

125. tests/test_log.py (L126)

The len(rendered) <= len(str(results)) * 0.2 assertion is a fragile heuristic that only holds reliably because the test input contains a 256-element embedding list inflating str(results). If the test fixture is later changed (e.g., the embedding is removed or shrunk), this bound can fail even when the summarizer is correct, or silently pass when it shouldn't. It also adds no new PII-safety signal beyond the explicit not in checks immediately above it. Consider removing this assertion — the four preceding not in checks already constitute a more precise and maintainable verification of the privacy guarantee.

---

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

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (204/204 executed, 1 skipped). memos_github_open_source/smoke: 1/1, memos_local_openclaw/unit: 45/45, memos_local_plugin/changed-python-source: 3/3, memos_python_core/changed-repo-python: 155 passed, 1 skipped. Duration: 19s [advisory, non-gating] AI-generated tests on branch test/auto-gen-47c422590cb9b57f-20260724162555: 110/112 passed, 2 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: dev-v2.0.25