Skip to content

fix(openclaw): preserve batch purge ID selectors - #340

Merged
loveRhythm1990 merged 2 commits into
matrixorigin:mainfrom
MatrixAdventurer:fix/openclaw-batch-purge-20261019
Oct 19, 2026
Merged

loveRhythm1990 merged 2 commits into
matrixorigin:mainfrom
MatrixAdventurer:fix/openclaw-batch-purge-20261019

Conversation

@MatrixAdventurer

@MatrixAdventurer MatrixAdventurer commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

由 LW 授权的 ChatGPT Business 智能体提交。

What type of PR is this?

  • fix (bug fix)
  • test (adding regression coverage)

Which issue(s) this PR fixes

New narrowly scoped OpenClaw HTTP adapter defect. Related #268 concerns cache invalidation, not this request-body mapping; this does not duplicate that fix. No automatic issue closure.

What this PR does / why we need it

The existing Rust MCP memory_purge schema and both embedded/remote implementations accept a single ID or comma-separated IDs. OpenClaw purgeMemory already splits that selector to invalidate caches, but its HTTP adapter sends memoryId: "m1,m2" as {"memory_ids":["m1,m2"]}, looking up one nonexistent combined ID rather than deleting the selected records. Depending on authorization path this can produce a not-found error or no matches; no production deletion outcome is asserted.

  • Split, trim and remove empty CSV fields at the existing HTTP adapter boundary, matching the MCP contract.
  • Reject explicitly supplied empty/all-empty or non-string ID selectors before HTTP. The API treats an empty ID array as absent, so simply forwarding [] together with a topic could accidentally broaden deletion. This fix prevents that.
  • Keep valid IDs + topic together so the API still rejects mutually exclusive selectors; never silently discard one.
  • Invalidate each normalized ID after deleteMemory succeeds, just as purgeMemory does. This also fixes whitespace-padded single-ID invalidation. Failed mutations preserve the cache; unrelated IDs and other users remain cached. The optional tool-description edit was reverted to keep the complete PR at three files.
  • Preserve topic-only, single-ID, reason and auth behavior. No backend authorization, storage, retry, dependency or release changes.

Comma remains a reserved batch separator, as in the existing Rust MCP contract; it is not treated as a literal character in one custom legacy ID. Normal generated memory IDs are UUID/hash identifiers.

Scope

Three files, +189/-2, based on main c551911b308dff1d7c2f0d9458fe1b7aadd12311.
Head 85bafa6c06a6e37404bfe76d38db360320ed7cb6; tree b894b722bc80e50f081a5b1441dcb892777710ff matches the locally tested tree exactly.
Own fork branch MatrixAdventurer:fix/openclaw-batch-purge-20261019; upstream has no dev branch, so target is main.

Verification

Node 24.19.0 / Vitest 4.1.11:

  • Identical new regression suite on unchanged baseline: 11 failed / 2 passed. Three CSV cases prove incorrect request target selection, seven cases prove missing local rejection, and the mixed-selector case proves incorrect ID serialization.
  • Original 13-case regression suite: 13 passed after the transport fix.
  • Review follow-up: byte-identical expanded regression suite on original PR head a610f079: 4 failed / 19 passed, all proving stale cache returns through deleteMemory (CSV, spaced CSV, empty segments, padded single ID).
  • Current expanded regression suite: 23 passed, covering both public deletion methods, retained unrelated/other-user cache entries, and failed deletion preserving cache.
  • Normal npm ci, full npm test: 181 passed across 10 files.
  • npm run test:pack: passed (bundle build, package validation, packed-file check).
  • git diff --check: passed.
  • Independent follow-up review agent Approve: reproduced original-PR red cases, current 23-case regression, full 181-test suite and package checks. All three final changed files reviewed, no blocking findings.

Tests exercise the real TypeScript client/transport and native Request serialization with synthetic fetch responses. The exact-ID matching fixture is not a real server/database, and its purged=0 result is not asserted as the only live-server behavior.

Remaining gates / limitations

  • Fresh CI for current head 85bafa6 is green: OpenClaw test/package, Rust unit tests, Check/Clippy and MatrixOne DB/integration tests, and PR title check. The DB job includes the all-tests step plus restore transaction, branch capability cleanup, and offline restore cleanup regressions. No local Rust/database integration test is claimed.
  • Latest upstream main fda8bd3 adds snapshot listing changes only, with no overlap in the OpenClaw plugin.
  • Extra npx tsc --noEmit is not clean: absent optional host peer openclaw/plugin-sdk causes 42 diagnostics, identically reproduced on the unchanged baseline; no new diagnostics in this patch. The repository's actual OpenClaw CI commands above pass locally. No types or gate configuration were weakened.
  • No merge, deployment or package publication performed.

Review follow-up

Addresses the cache-invalidation finding in commit 85bafa6. Cache IDs now use the same split/trim semantics as HTTP selection; the existing invalidation helper ignores empty segments and advances the cache generation. No force-push, merge, deployment or package publication.

由 LW 授权的 ChatGPT Business 智能体提交。
@loveRhythm1990

Copy link
Copy Markdown
Collaborator

The CSV request-body bug is real, and splitting/validating selectors at the HTTP boundary is appropriate. However, one caller of this shared adapter needs a corresponding fix before LGTM.

[P2, bug] Invalidate all normalized IDs when this change is reached through deleteMemory. At plugins/openclaw/openclaw/http-client.ts:218-225, splitting the selector changes the deletion performed by every memory_purge caller, including MemoriaClient.deleteMemory, which is used by the memory_forget tool. That method still calls invalidateMemoryIds(userId, [params.memoryId]) at client.ts:594. With memoryId: "m1,m2", the server now receives ["m1", "m2"], but only the nonexistent cache key "m1,m2" is invalidated. Subsequent getMemory calls continue returning the deleted records from cache.

I reproduced this through the actual MemoriaClient with synthetic HTTP responses: cache m1 and m2, call deleteMemory with m1,m2, verify that the request deletes both fixture records and reports purged=2, then read m1. It returns the stale cached record instead of null. The identical test using purgeMemory passes. The new tests cover purgeMemory but miss this second caller.

Please use consistent selector normalization for deletion and cache invalidation in deleteMemory as well, or explicitly reject batch selectors at the single-memory boundary before sending anything. Add a regression covering cached records through this caller. Normalized whitespace IDs should follow the same rule.

Validation at a610f07: the existing 171-test OpenClaw suite and test:pack passed locally on Node 25.9.0; the new PR suite against the baseline produced 11 failures and 2 passing controls. The additional caller/cache regression produced 1 failure and 1 passing control. Current upstream OpenClaw and Rust/DB CI are green, but do not cover this regression. All three changed files were reviewed (3/3, none skipped). No live database deletion was performed.

Clear each deleted ID from the client cache after a successful purge.
Cover batch and whitespace selectors through both public deletion methods,
failed mutations, and unrelated user/ID cache preservation.
Revert the optional tool description change to keep the PR to three files.

由 LW 授权的 ChatGPT Business 智能体提交。

Copy link
Copy Markdown
Contributor Author

由 LW 授权的 ChatGPT Business 智能体提交。

Addressed the cache-invalidation finding in commit 85bafa6. deleteMemory now invalidates all split/trimmed IDs after the purge succeeds, matching purgeMemory; the invalidation helper skips empty segments and advances the generation. Reverted the optional tool-description change so the entire PR remains 3 files, +189/-2.

The expanded regression suite reproduces 4 failures / 19 passes on the prior PR head a610f07 (CSV, whitespace, empty segments, and padded single ID all return deleted data from cache); the same suite now passes 23/23. It also checks both callers, unrelated and other-user cache preservation, and failed mutations retaining cached entries. Full local suite 181/181, test:pack and git diff --check pass. Independent review agent repeated red/green and full/package checks: Approve.

Fresh OpenClaw CI and title check are green; Rust/MatrixOne CI is still running. Extra TypeScript checking continues to show exactly the same 42 optional-host-peer diagnostics as the unchanged baseline; no new diagnostics. No local Rust/database integration test, merge, deployment, or package publication is claimed.

@loveRhythm1990

Copy link
Copy Markdown
Collaborator

LGTM on 85bafa6c06a6e37404bfe76d38db360320ed7cb6.

The original HTTP batch-purge bug is real: comma-separated IDs were sent as one literal ID. Splitting, trimming, and rejecting an empty ID selector correctly implements the MCP contract without allowing an invalid ID selector to fall through to topic deletion.

The previously reported cache issue is also resolved: deleteMemory now invalidates each normalized ID after a successful purge, consistent with purgeMemory. The regression tests cover both methods, whitespace and empty segments, preservation of unrelated/user-scoped cache entries, and rejected deletions.

Validation: all 181 OpenClaw tests and package build/verification passed locally. Running the current batch-purge tests against the previous client.ts reproduced four cache failures (19 passed), confirming that the new tests detect the defect. Current CI, including DB tests and the explicit cleanup/restore regressions, is green. Reviewed all three changed files; no further actionable issues found.

@MatrixAdventurer

Copy link
Copy Markdown
Contributor Author

由 LW 授权的 MOI 智能体提交。

补记 CI 完成情况给 @loveRhythm1990(并同步 @XuPeng-SH):先前的 12:35 评论发布时 Rust/MatrixOne CI 还在运行,现在已在未再改动的 head 85bafa6c06a6e37404bfe76d38db360320ed7cb6 上全部通过(状态均为 success / completed):

你在 deleteMemory 缓存失效 P2 意见 中指出的问题已在 commit 85bafa6 处理:失效入口与 purgeMemory 使用一致的 split/trim 规则;扩展后的回归套件在该 head 上 23/23,完整本地套件 181/181(见 此前记录)。PR 规模保持 3 文件 +189/-2,目标分支为 main(memoria 无 dev),未强制推送、未合并、未部署、未关闭 Issue。

状态只以上述实际运行的 GitHub 输出为准;我这边没有做任何新的代码变更。请在方便时复审。

@MatrixAdventurer

Copy link
Copy Markdown
Contributor Author

由 LW 授权的 MOI 智能体提交。

更正上一条评论(#issuecomment-6270090896)中的一处链接笔误:DB Tests 行误用了 Unit Tests 的 job ID 118061774636。DB Tests 的正确链接为 https://github.com/matrixorigin/memoria/actions/runs/39253174149/job/118062735262 (run 39253174149,12:57:42Z success,与所引完成时间一致)。其余各条 job ID、时间戳、结论和 head 85bafa6 均不变。

@loveRhythm1990
loveRhythm1990 merged commit ed63438 into matrixorigin:main Oct 19, 2026
8 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.

3 participants