Skip to content

fix(openclaw): follow list cursors and look up memories by ID - #274

Merged
loveRhythm1990 merged 3 commits into
matrixorigin:mainfrom
loveRhythm1990:fix/openclaw-list-pagination
Oct 7, 2026
Merged

loveRhythm1990 merged 3 commits into
matrixorigin:mainfrom
loveRhythm1990:fix/openclaw-list-pagination

Conversation

@loveRhythm1990

Copy link
Copy Markdown
Collaborator

Fixes #267

Problem

GET /v1/memories limits each page to 500 rows and returns next_cursor when more rows exist. In API mode, the OpenClaw plugin dropped that cursor:

  • MemoriaHttpTransport.list serialized only items and never requested a second page.
  • MemoriaClient.listMemories set partial from items.length >= scanLimit. When the requested limit was above 500, a truncated page looked complete. A request for 1000 rows came back with 500 rows and partial: false.
  • getMemory searched that same truncated first page, so it returned null for any uncached ID that sat beyond the first 500 rows.
  • stats built on the same list, so activeMemoryCount stopped at 500.

Fix

  • http-client.ts: list follows next_cursor. Each page asks for at most 500 rows, and the loop runs until it has limit rows, the server returns no cursor, or it reaches maxListPages. Until now that setting only multiplied the scan size. The method returns { items, has_more }. It also stops if the cursor does not advance or a page comes back empty; any cursor left over still sets has_more.
  • http-client.ts: a new memory_get dispatch calls the existing GET /v1/memories/{id} endpoint. Like the list, that endpoint returns only active memories.
  • client.ts: listMemories sets partial from has_more, and only falls back to the length check when has_more is missing. getMemory uses memory_get instead of scanning the list. It returns null only when the API says the memory is absent. API errors now propagate, so they no longer read as "missing".

Tests

New file __tests__/list-pagination.test.ts (12 tests). Its fetch mock emulates the server's keyset pagination with the 500-row limit. It covers:

  • the repro from the issue
  • lists longer than 500 rows
  • a total that is exactly the limit, and one that ends exactly on a page boundary
  • the maxListPages limit
  • a cursor that does not advance
  • stats counting past the first page
  • getMemory on a later page, a not-found ID, an error response, and the cache path

helpers.ts gains respondWithHandler so a test can serve different responses depending on the URL.

  • npm test: 118/118 pass.
  • Against the original client.ts and http-client.ts, 7 of the 12 new tests fail.
  • npm run test:pack passes.

🤖 Generated with Claude Code

GET /v1/memories clamps each page to 500 rows and returns next_cursor when
more exist, but the API transport dropped the cursor and the client inferred
completeness from items.length >= requested limit. A 1000-row request
therefore returned 500 rows with partial:false, and getMemory reported IDs
beyond the first page as missing.

- MemoriaHttpTransport.list follows next_cursor up to maxListPages and
  returns has_more, stopping on a non-advancing cursor.
- listMemories derives partial from has_more.
- getMemory calls GET /v1/memories/{id} instead of scanning the list, so a
  null result means the API reported the memory absent.

Fixes matrixorigin#267

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor

Reviewed head 8988b4e. The pagination and direct-ID lookup fit #267. I did not identify a blocking issue in this static review. Three non-blocking suggestions:

  1. In plugins/openclaw/openclaw/http-client.ts:158–165, check for a non-null repeated cursor before appending that page. Currently, a server that repeats a page adds its rows twice before the loop stops, which can inflate list counts and derived stats. Keep has_more true when stopping for this reason, and still append a normal terminal page whose next_cursor is null. Extend the existing stalled-cursor test to assert the returned IDs and count (one item for its current fixture), in addition to the request count and partial flag. This is defensive handling of an anomalous response; a narrow guard should be enough.

  2. Update the old memory_get bounded-scan descriptions in plugins/openclaw/openclaw/index.ts:331–335, config.ts:129–134, and plugins/openclaw/README.md:203–204. They still describe fallback scans and advise rerunning search/list, while this PR uses a direct lookup on cache misses. Also describe maxListPages as a pagination bound for lists/derived stats rather than a memory_get fallback setting.

  3. Make serveMemories in plugins/openclaw/openclaw/tests/list-pagination.test.ts:21–46 route explicitly. It currently returns memory-list data for every non-ID path, including /v1/snapshots and /v1/branches, despite the stats test's comment saying those return empty pages. Return the correct empty response for each of those endpoints, reject unexpected paths, and assert the snapshot/branch counts alongside activeMemoryCount in the stats test.

I have not run the tests or reproduced these against a live service.

— dot (AI-assisted review; static analysis only)

@loveRhythm1990

Copy link
Copy Markdown
Collaborator Author

Review

The issue is valid. list_memories clamps limit to 500 and returns next_cursor (memoria-api/src/routes/memory.rs). The plugin dropped that cursor, inferred partial from items.length >= scanLimit, and getMemory scanned only the first page. All three behaviors match #267.

The change looks correct. npm test on the PR head passes 118/118. I traced through list:

  • pageSize = min(limit - collected, 500) never asks for more than the remaining rows, so items.length > limit can only happen if the server misbehaves.
  • has_more = cursor !== null: the cursor is non-null only when the server sent one and the loop stopped early (limit reached, maxListPages hit, or stalled). If the limit lands exactly on the last page, the server returns next_cursor = null, so partial = false. Correct.
  • Stall guard (next === cursor || pageItems.length === 0): this stops an infinite loop, and the leftover cursor still marks the result partial. 👍
  • getMemory → GET /v1/memories/{id}: the server returns Json(None) for an unknown ID, which goes through "null" → asRecord(null) → null. Real errors propagate, and the only caller (index.ts memory-get tool) already has a try/catch. Like the old list scan, get_from filters is_active = 1, so active-only semantics are kept.

Minor / optional

  1. Validate the returned record in getMemory. request() returns { ok: true } for an empty body. That would pass asRecord, become a memory with an empty ID, and get cached. A cheap guard: if (!record || record.memory_id !== params.memoryId) return null;.
  2. Master key. For master auth, get_memory falls back to find_memory_any_user. If someone configures the plugin with a master key, getMemory can now return another user's memory, which the old scoped list scan could not. That's unlikely given the sk- user-key model, but the guard in (1) won't catch it. If MemoryResponse carries user_id, consider checking it, or document that the plugin expects a user-scoped key.
  3. When memoryType is set, listMemories still filters on the client after scanning up to 2000 rows. /v1/memories already accepts memory_type, so passing it through would avoid both the scan and that limitation. This can be a follow-up.
  4. maxListPages now means "max pages of 500" in the transport, but it is still used as a ×50 multiplier for stats and a ×limit multiplier for filtered lists. Updating the config description/placeholder would help.

LGTM after (1). The other items are optional.

Address review on matrixorigin#274:
- Stop before appending a page whose next_cursor repeats the request
  cursor, so a misbehaving server cannot double-count rows; the leftover
  cursor still marks the result partial.
- getMemory returns null unless the payload is the requested memory, so an
  empty-body response is never cached under a bogus ID.
- Update memory_get and maxListPages descriptions in index.ts, config.ts,
  openclaw.plugin.json and README to match the direct ID lookup.
- Route the test server explicitly (snapshots, branches, unexpected paths)
  and assert snapshot/branch counts and de-duplicated IDs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@loveRhythm1990

Copy link
Copy Markdown
Collaborator Author

Follow-up: review items addressed in ea401d7

Item Status
dot #1: repeated cursor double-counts a page Fixed. The check now runs before appending, so a page whose next_cursor equals the request cursor is dropped and has_more stays true. The stalled-cursor test now also checks the returned IDs and count (1).
dot #2 / my #4: stale memory_get and maxListPages descriptions Fixed in index.ts, config.ts, openclaw.plugin.json and the README. The setting is now described as the per-call page limit (≤500 rows per page) that also bounds stats and client-side type filtering.
dot #3: test server routes every non-ID path to the memory list Fixed. The handler now serves /v1/snapshots and /v1/branches with their real response shapes and throws on any other path. The stats test checks snapshotCount = 0 and branchCount = 1.
my #1: empty-body { ok: true } cached as a memory Fixed. getMemory returns null unless memory_id matches the requested ID. A regression test checks that nothing is cached.
my #2: master-key cross-user lookup Not changed. The plugin's userId is an OpenClaw identity, not the API key owner's ID, so the client has nothing reliable to compare against. The plugin is designed for user-scoped sk- keys.
my #3: push memory_type filtering to the server Deferred. This is independent of #267 and better as a follow-up PR.

npm test: 119/119 pass. npm run test:pack passes. CI Unit Tests, Check & Clippy and test are green, and DB Tests are pending.

@loveRhythm1990
loveRhythm1990 merged commit 7dded43 into matrixorigin:main Oct 7, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OpenClaw API listing discards next_cursor and reports truncated results as complete

2 participants