[linjicong]

Description

clampLimit() in apps/memos-local-plugin/core/storage/repos/_helpers.ts rewrote every explicitly requested page size to the 500-row default page:

export function clampLimit(n: number): number {
  if (!Number.isFinite(n) || n <= 0) return 500;
  return Math.min(Math.trunc(n), 500);   // ← explicit 10_000 became 500
}

Bulk callers therefore silently lost rows:

• metrics() (memory-core.ts) reads traces.list({ limit: 10_000 }) to bucket the viewer's "memory writes per day" chart — it only ever saw the newest 500 traces. On a real production DB (9,362 traces over 52 days, ~250/day) the chart showed data for exactly the last two days (253 + 247 = 500) and zero-filled the rest, while the sessions card reported 14 instead of 163 and embeddings was similarly undercounted.
• World-model maintenance (worldModel.list({ limit: 100_000 })) received 500 rows.

Fix: explicit positive limits are now honored up to a MAX_QUERY_LIMIT ceiling of 100,000 (the largest limit any call site asks for today). The default page for callers that omit limit stays at 500 — no existing call site changes behavior, and unbounded scans remain impossible. No new dependencies.

Relation to #2464 (cc @akushonkamen): same root cause, different contract decision. #2464 additionally changes omitted-limit to mean "all rows" (no LIMIT clause), which fixes more of #2401 but re-opens unbounded-scan risk on very large databases (the #2076 4.2 GB / 518k-row report). This PR is intentionally the minimal, zero-behavior-change subset: it can land independently (or serve as the safe core of that effort) while the omitted-limit semantics are decided separately.

Related Issue (Required): Addresses the explicit-limit half of #2401 (no Fixes — the omitted-limit call sites it also reports are intentionally out of scope here).

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

cd apps/memos-local-plugin && npm ci && npx vitest run tests/unit

• Unit Test
  • New tests/unit/storage/explicit-limit.test.ts: clampLimit honors explicit limits up to MAX_QUERY_LIMIT, invalid input keeps the 500 default, and repo.list({ limit: 10_000 }) over 600 seeded rows (3 distinct days) returns all 600 — including the oldest rows a day-bucketing caller needs — while repo.list() keeps the 500-row default page.
  • New regression in tests/unit/pipeline/memory-core.test.ts ("MemoryCore metrics sees beyond the 500-row page"): 520 traces "now" + 100 traces 3 days earlier; metrics({ days: 7 }) must report the older day bucket as 100 and sessions as 2. Fails on the old code with exactly the shipped symptom (expected +0 to be 100) and passes with this change.
  • Updated the stale assertion in traces-count.test.ts that enshrined the old clamp (list({ limit: 10000 }) → 500) to the new contract (→ 600).
• Test Script Or Test Steps (please provide)
  • npm run lint (tsc --noEmit): clean.
  • Full vitest run tests/unit on this branch: 9 pre-existing failing files, byte-identical to the same run on pristine main (config/paths, install scripts, server http/sse — Windows-environment failures; see also the CI "ENV ISSUE" already reported on fix(plugin): stop clamping storage list queries to 500 rows #2464). This change adds zero new failures and 3 new passing tests.

Checklist

• I have performed a self-review of my own code | 我已自行检查了自己的代码
• I have commented my code in hard-to-understand areas | 我已在难以理解的地方对代码进行了注释
• I have added tests that prove my fix is effective or that my feature works | 我已添加测试以证明我的修复有效或功能正常
• I have created related documentation issue/PR in MemOS-Docs (if applicable) — N/A, no user-facing docs change
• I have linked the issue to this PR (part of 2.0.20 still truncates internal queries at 500 rows: countPolicies/countEpisodes/countSkills/countWorldModels report 500, and no-limit call sites lose rows (#1954 only changed the ceiling) #2401) | 我已将 issue 链接到此 PR（2.0.20 still truncates internal queries at 500 rows: countPolicies/countEpisodes/countSkills/countWorldModels report 500, and no-limit call sites lose rows (#1954 only changed the ceiling) #2401 的一部分）
• I have mentioned the person who will review this PR | 我已提及将审查此 PR 的人（@akushonkamen for the overlapping fix(plugin): stop clamping storage list queries to 500 rows #2464; maintainers please advise）

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2535
Task: 0e7957ecae43a2da
Base: main
Head: fix/plugin-honor-explicit-list-limits

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

---

1. apps/memos-local-plugin/server/routes/params.ts (L21-L23)

Bug: fractional values between 0 and 1 produce limit = 0.

Number("0.9") is 0.9, which passes the parsed <= 0 guard, then Math.trunc(0.9) === 0. The function returns Math.min(0, 500) === 0, so the downstream SQL query becomes LIMIT 0 and returns zero rows.

Fix by truncating before the positivity check so that sub-integer positives are treated as non-positive (i.e., fall back to the default), or by rounding up:

export function parseLimitParam(
  raw: string | null,
  fallback = 50,
): number {
  const parsed = Math.trunc(Number(raw));
  if (!Number.isFinite(parsed) || parsed <= 0) return fallback;
  return Math.min(parsed, MAX_HTTP_PAGE_LIMIT);
}

💡 Suggested Change

Before:

  const parsed = Number(raw);
  if (!Number.isFinite(parsed) || parsed <= 0) return fallback;
  return Math.min(Math.trunc(parsed), MAX_HTTP_PAGE_LIMIT);

After:

  const parsed = Math.trunc(Number(raw));
  if (!Number.isFinite(parsed) || parsed <= 0) return fallback;
  return Math.min(parsed, MAX_HTTP_PAGE_LIMIT);

---

2. apps/memos-local-plugin/core/storage/repos/_helpers.ts (L62-L65)

The api_logs repo's list() method (line 152 in api_logs.ts) still uses its own hardcoded inline clamp:

const limit = Math.max(1, Math.min(500, filter.limit ?? 50));

This does not use clampLimit() or DEFAULT_PAGE_SIZE / MAX_QUERY_LIMIT. This means:

1. The api_logs repo is inconsistent with the new pattern — if a bulk-read caller (e.g., metrics()) passes limit: 10_000, api_logs will silently truncate to 500 while every other repo now correctly honours it up to 100,000.
2. The api_logs inline comment on ApiLogFilter.limit (/** Default 50; max 500 */) will become stale/misleading if the intent is to eventually unify all repos under the new ceiling.

Consider making api_logs.ts consistent by delegating to clampLimit(), or at minimum documenting that it is intentionally bypassed (i.e., the 500 hard-cap here is a deliberate business rule for this specific endpoint).

💡 Suggested Change

Before:

export const MAX_QUERY_LIMIT = 100_000;

/** Default page size when a caller omits `limit`. */
export const DEFAULT_PAGE_SIZE = 500;

After:

// In api_logs.ts list():
const limit = clampLimit(filter.limit ?? 50);

---

3. apps/memos-local-plugin/server/routes/params.ts (L17-L20)

The fallback parameter is not validated before use. If a caller passes fallback = 0 or fallback = -1, the function silently returns an invalid page size that would cause LIMIT 0 (empty result) or a negative LIMIT in SQL. Consider adding a guard: const safeFallback = Number.isFinite(fallback) && fallback > 0 ? Math.trunc(fallback) : 50; and use safeFallback in the return path.

💡 Suggested Change

Before:

export function parseLimitParam(
  raw: string | null,
  fallback = 50,
): number {

After:

export function parseLimitParam(
  raw: string | null,
  fallback = 50,
): number {
  const safeFallback = Number.isFinite(fallback) && fallback > 0 ? Math.trunc(fallback) : 50;

---

4. apps/memos-local-plugin/core/storage/repos/_helpers.ts (L74-L77)

The guard n <= 0 silently returns DEFAULT_PAGE_SIZE (500) for an explicitly passed 0 or any negative value. This can mask bugs in callers that compute a limit incorrectly and end up passing 0 — they will silently receive 500 rows instead of an empty result or an error.

Since the function now allows large bulk limits up to MAX_QUERY_LIMIT, the contract of "invalid means use the default" becomes harder to reason about. Consider throwing (or at least logging a warning) for explicitly invalid inputs so misuse is detectable, reserving the DEFAULT_PAGE_SIZE fallback only for the undefined/null case handled by buildPageClauses.

💡 Suggested Change

Before:

export function clampLimit(n: number): number {
  if (!Number.isFinite(n) || n <= 0) return DEFAULT_PAGE_SIZE;
  return Math.min(Math.trunc(n), MAX_QUERY_LIMIT);
}

After:

export function clampLimit(n: number): number {
  if (!Number.isFinite(n) || n <= 0) {
    // Defensive fallback; callers should validate before reaching here.
    return DEFAULT_PAGE_SIZE;
  }
  return Math.min(Math.trunc(n), MAX_QUERY_LIMIT);
}

Generated by cloud-assistant via Open Code Review.

[linjicong]

Thanks for the review — both points addressed in 7a0f9b8.

Point 2 (duplicated 500 magic number) — accepted verbatim: extracted DEFAULT_PAGE_SIZE = 500, now referenced by both buildPageClauses and clampLimit.

Point 1 (global 100k ceiling too coarse-grained) — accepted, with one amendment to the premise: I audited every untrusted entry before choosing where to fix it, and the actual exposure was not "any caller can fetch 100k rows" but a specific, smaller set:

untrusted entry	downstream	effective max after the first commit
GET /api/v1/traces?limit=	core.listTraces clamps at 10_000	10,000 (doc says "max 500")
GET /api/v1/episodes?limit=	core.listEpisodes — no clamp	100,000
GET /api/v1/policies / world-models?limit=	listPolicies clamps at 500 but its internal over-fetch (limit + offset + 200) was previously hard-capped at 500, now bounded by MAX_QUERY_LIMIT	amplified internal reads
GET /api/v1/api-logs?limit=	core.listApiLogs clamps at 500	500 (unaffected)
GET /api/v1/skills?limit=	route-local Math.min(n, 500)	500 (unaffected)

Rather than threading per-caller caps through clampLimit's signature (which would churn every bulk call site — the exact invasiveness this PR tries to avoid), the boundary is now enforced at the boundary: new server/routes/params.ts exposes parseLimitParam(raw, fallback) + MAX_HTTP_PAGE_LIMIT = 500, and all four exposed routes use it. Every untrusted page is back to its pre-PR 500 ceiling (and GET /api/v1/traces's documented "max 500" is enforced for the first time), while trusted in-process bulk readers (metrics(), world-model maintenance) keep the MAX_QUERY_LIMIT ceiling. MAX_QUERY_LIMIT's doc comment now states that split explicitly.

Side note from the audit: the policies over-fetch (limit + offset + 200) used to silently under-fill pages once offset exceeded ~300 because the repo fetch was clamped at 500 — honoring explicit limits fixes that latent paging bug as a side effect.

Verification — npm run lint clean; vitest run tests/unit: new tests/unit/server/params.test.ts (8 assertions incl. clamp-at-500 and 1e9 input) plus all prior tests pass; full-suite failure set is still byte-identical to pristine main (the 9 pre-existing Windows-environment failures), zero new failures.