Skip to content

feat(external-context): Add daemon memory deletion - #11337

Merged
doudouOUC merged 1 commit into
mainfrom
codex/mem0-daemon-delete
Sep 8, 2026
Merged

feat(external-context): Add daemon memory deletion#11337
doudouOUC merged 1 commit into
mainfrom
codex/mem0-daemon-delete

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

Adds an independently enabled, workspace-bound deletion profile for daemon memory. The read helper returns one complete target; an explicitly requested deletion carries the exact ID and original text through ordinary MCP approval, rechecks ID, every configured scope and full text, submits at most one DELETE, and verifies absence. Search now omits overlong IDs instead of returning truncated identifiers.

Why it's needed

The merged daemon writer allows users to save memories, but removing an obsolete or incorrect record still requires a separate administration interface. Search summaries and truncated IDs cannot safely identify the complete deletion target. This change provides a narrow explicit deletion workflow while keeping the default extension search-only.

Reviewer Test Plan

How to verify

Enable the separate deletion server in a trusted workspace with a disposable memory service, default approval mode and an explicit ask rule. Read a target, confirm that approval contains its complete ID and text, reject it and observe zero deletion-side requests. Approve a new call and confirm exactly one target disappears while control records remain. Change the text during approval or use another workspace’s scope and confirm no DELETE occurs. Check that unknown responses, cancellation after submission and transport loss never trigger automatic replay. A repeated call for an absent record must report that no DELETE was submitted.

Evidence (Before & After)

Before: the default reader/writer had no deletion tool, and a 129-character search ID could be returned as another record’s 128-character ID. After: the overlong candidate is omitted; a separately enabled daemon tool reads and checks the complete target before one deletion. Actual Chrome approval of a 4000-code-point target produced one DELETE, and a separate empty-text rejection produced zero GET/DELETE requests, with browser voter IDs distinct from the test driver. The existing approval UI is reused; its extra full-DOM character-by-character assertion did not complete because the control tool timed out.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

macOS with Node v22.22.3, local built daemon/ACP/stdio MCP and loopback synthetic providers. The initial global CLI baseline used qwen 0.23.0 on Node v24.12.0. Build, typecheck and bundle passed; 330 package tests, 67 Web Shell tests and the durable daemon integration test passed. Additional independent verification covered 170 protocol/package cases and 21 daemon scenarios. The extension implementation is unchanged from those independent runs; core build and focused tests were refreshed on the current main base.

Risk & Scope

  • Main risk or tradeoff: the final GET and DELETE are not atomic; an update after the final read may also be deleted. Deployment must verify non-reused IDs and immutable scope, or independent server-side deletion authorization for the fixed scope. Confirmation text enters ordinary tool parameters and transcripts.
  • Not validated / out of scope: real Holo conformance is blocked by the unavailable temporary credential; the preflight sent zero service requests. The extra full-DOM character comparison did not return. Real index propagation, backups, transcript erasure, atomic version deletion, automatic/bulk/cascading deletion, and CLI/TUI integration are out of scope.
  • Breaking changes / migration notes: deletion is off by default and requires its own V5 instance configuration and MCP entry; V2/V3/V4 remain separate. Search records with IDs longer than 128 Unicode code points are now omitted rather than truncated; complete IDs up to 256 allowed ASCII characters remain usable through the explicit tools.

Linked Issues

Follow-up to #11311. No issue is automatically closed.

中文说明

What this PR does

为 daemon 记忆增加独立启用、绑定 workspace 的删除能力。读取辅助工具返回一条完整目标;用户明确请求删除后,精确 ID 和原文通过现有 MCP 审批,执行端重新核对 ID、全部配置 scope 和完整正文,最多提交一次 DELETE 并复核不存在。搜索遇到超长 ID 时改为省略该记录,不再返回截断标识符。

Why it's needed

已合并的 daemon writer 支持保存记忆,但删除过期或错误记录仍需另外使用管理接口。搜索摘要和截断 ID 无法安全标识完整删除目标。本次提供范围明确的显式删除流程,同时保持默认 Extension 仅搜索。

Reviewer Test Plan

How to verify

在可信 workspace 为可丢弃的记忆服务启用独立删除 server,使用默认审批模式及显式 ask 规则。读取目标,确认审批包含完整 ID 和正文,拒绝后观察删除侧零请求。批准新的调用后,确认仅一条目标消失且对照记录保留。在审批期间修改正文或使用其他 workspace 的 scope,确认没有 DELETE。检查未知回执、提交后取消及传输断开均不触发自动重放。对已不存在记录再次调用,应报告未提交 DELETE。

Evidence (Before & After)

变更前:默认 reader/writer 没有删除工具,129 字符的搜索 ID 可能作为另一条记录的 128 字符 ID 返回。变更后:超长候选被省略,独立启用的 daemon 工具先读取并核对完整目标,再执行一次删除。实际 Chrome 批准 4000 code point 目标产生一次 DELETE;另一次空正文拒绝产生零 GET/DELETE,两次浏览器 voter ID 均与测试驱动不同。复用现有审批 UI;额外的 DOM 全文逐字符断言因控制工具超时未完成。

Tested on

OS Status
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

Environment (optional)

macOS、Node v22.22.3,本地构建的 daemon/ACP/stdio MCP 与 loopback 合成 provider。初始全局 CLI 基线使用 Node v24.12.0 上的 qwen 0.23.0。build、typecheck、bundle 通过;330 项包测试、67 项 Web Shell 测试和持久 daemon 集成测试通过。额外独立验收覆盖 170 个协议/打包用例和 21 个 daemon 场景。Extension 实现与独立验收时一致;已在当前 main 基线上重新执行核心构建和相关测试。

Risk & Scope

  • Main risk or tradeoff:最后一次 GET 与 DELETE 不原子,最终读取后发生的更新仍可能被删除。部署必须核实 ID 不复用且 scope 不可迁移,或由服务端独立限制固定 scope 的删除权限。确认原文会进入普通工具参数和会话记录。
  • Not validated / out of scope:真实 Holo 验收因临时凭证不可用而阻塞,预检未发送服务请求;额外 DOM 全文逐字比较没有返回结果。真实索引传播、备份、会话记录擦除、原子版本删除、自动/批量/级联删除和 CLI/TUI 集成不在范围内。
  • Breaking changes / migration notes:默认不启用删除,需要独立 V5 实例配置和 MCP 入口,V2/V3/V4 保持分离。搜索现在省略 ID 超过 128 Unicode code point 的记录,不再截断;不超过 256 个允许 ASCII 字符的完整 ID 仍可用于显式工具。

Linked Issues

接续 #11311,不自动关闭任何 issue。

Add an opt-in workspace-bound deletion profile with exact target reads,
full-text and scope verification, single-shot deletion, and absence checks.
Preserve full record identifiers and cover approval and failure boundaries.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

E2E verification report

PR head: cdcbdbc76aec9154fc89f33d8c15c1ba574b0bd3; refreshed main base: 078b924989148f5ae689e63dbb66590047c693e5.

The deletion implementation was verified with disposable records and loopback services. No real Holo record was created or deleted, and no user daemon or workspace configuration was changed.

Verification Result
Fresh main-base build / typecheck / bundle Passed
Complete extension suite 17 files, 330 tests passed
Existing Web Shell approval/adapter regression 3 files, 67 tests passed
Durable daemon integration test Passed on the new main base
Independent stdio / HTTP / packaged archive matrix 170/170 passed on the unchanged extension implementation
Independent daemon permission/lifecycle matrix 21/21 passed before rebasing; refreshed durable integration test covers workspace ownership and approval on the new main base
Actual Chrome approval 4000-code-point target, one DELETE, distinct browser voter
Actual Chrome rejection Empty-text target, zero GET/DELETE, distinct browser voter

The independent protocol run covered strict configuration/version isolation, an archive unpacked outside the repository and node_modules, complete writer IDs, scope and whole-text checks, forbidden collection paths, finite acknowledgement/absence contracts, bounded UTF-8/JSON bodies, one cumulative deadline, and cancellation without replay. The collection DELETE trap was never hit. A 600 ms total budget with each of three stages delayed by 230 ms returned unknown in approximately 604 ms.

The daemon matrix covered default ask/reject/cancel, invalid and stale votes, all REST SSE subscriptions disconnecting, Hook/YOLO semantics, interleaved workspaces, same-path config reload, restart without local confirmation state, runtime generation changes, permission timeout and MCP process loss after DELETE without unsafe replay. The actual browser votes were separate from the SDK test driver's client IDs. A timed-out browser-control attempt was not counted as a rejection; a fresh reject-only browser run supplied the successful zero-request rejection evidence. Test processes, browser tabs and temporary directories were cleaned up.

Reproduction commands

# Repository root
npm run build
npm run typecheck
npm run bundle

# integrations/external-context-mem0
npx vitest run
npm run lint

# packages/web-shell
npx vitest run client/adapters/transcriptAdapter.test.ts client/components/messages/ToolApproval.test.ts client/components/messages/ToolApproval.test.tsx

# integration-tests
QWEN_SANDBOX=false npx vitest run cli/external-context-mem0-daemon-delete.test.ts --retry=0

The additional independent scripts and local raw reports are working artifacts rather than committed test entry points. The durable stdio and daemon tests above are included in the PR. Tested locally on macOS / Node v22.22.3; Windows and Linux were not run locally.

Limits

Real Holo conformance remains unverified because the previously supplied temporary credential was unavailable; the preflight sent zero service requests. The browser DOM length, leading/trailing text, escaped controls and scroll-to-end were observed, but the additional full character-by-character DOM comparison did not return before the control tool timed out. No atomic version-delete guarantee is claimed: the independent synthetic test demonstrated that an update after the final GET can still be deleted. Service-side ID reuse, scope ownership/authorization, cascading behavior and real search-index propagation require deployment conformance checks.

Two self-audit passes and an independent read-only review found no remaining production defects. An obsolete README statement denying any deletion tool was corrected. Review did not substitute passing tests for inspecting the actual call paths.

中文验收说明

删除实现仅使用可丢弃的合成记录与 loopback 服务验证,没有创建或删除真实 Holo 记录,也未改动用户 daemon 或 workspace 配置。最新 main 基线上 build/typecheck/bundle、330 项完整扩展测试、67 项 Web Shell 回归及持久 daemon 集成测试通过。此前在相同 Extension 实现上完成了 170 项独立 stdio/HTTP/归档矩阵及 21 项 daemon 权限与生命周期矩阵;变更基线后复跑的持久 daemon 测试继续覆盖 workspace 归属和审批。

实际 Chrome 批准 4000 code point 目标产生一次 DELETE,独立空正文 Reject 产生零 GET/DELETE;两次 voter 均来自浏览器而非 SDK 驱动。浏览器控制超时导致的清理取消未计为拒绝成功,已用独立拒绝分支补验。集合 DELETE 陷阱零命中;600ms 总预算、每阶段230ms延迟的用例约604ms返回 unknown。测试进程、浏览器标签及临时目录均已清理。可重复执行的 stdio 和 daemon 回归测试包含在 PR,额外独立脚本与原始报告作为本地工作产物保留。

真实 Holo 因临时凭证不可用尚未验证,预检服务请求为零。浏览器 DOM 长度、首尾、控制字符转义与滚动末尾可见已观察;额外全文逐字比较因控制工具超时未返回,不计为通过。合成测试明确演示 GET→DELETE 非原子窗口;真实服务 ID 复用、scope 权威性/授权、级联行为及索引传播仍须部署验收。仅本地 macOS / Node v22.22.3 已测试,Windows/Linux 未运行。两轮自审与独立只读审查无遗留生产缺陷;README 旧说法已修正。

@doudouOUC
doudouOUC marked this pull request as ready for review September 8, 2026 08:00
@doudouOUC
doudouOUC enabled auto-merge September 8, 2026 08:00
@doudouOUC doudouOUC self-assigned this Sep 8, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required heading is present, and the Risk & Scope section is unusually candid about what was not verified, which is exactly what a reviewer needs.

Problem: real, and I checked it rather than taking the description's word. qwen-extension.json still registers only context_search, and examples/ on main offers only the auto-recall and write settings — so after #11311 landed yesterday you can save a memory and search it, but there is no way to remove one. That gap is genuine. The second half is weaker as a bug: profile.ts does truncate IDs to 128 code points today, so the collision the new test constructs is reachable in code, but Mem0 hands out UUIDs, so a >128-code-point ID is not something a real provider produces. I read that part as hardening that makes IDs safe to use as deletion keys, not as a fix for an observed failure — fine, it's two lines and it's tested, but worth saying plainly which half is which.

Direction: aligned. It completes the writer rather than opening a new front, it's off by default behind its own V5 instance config, its own credential env var and its own MCP entry, and it stays inside integrations/ — nothing under packages/ is touched, so there are no downstream consumers outside the integration and its own integration test.

Size: Stage 0 doesn't apply (no core paths). For orientation: ~590 production lines (~560 of it TypeScript), ~1727 test lines, 140 lines of schema JSON, ~360 lines of design doc + README, 29 lines of fixture. Under the 1000-line advisory, and the test-to-production ratio is about 3:1.

Approach: the shape matches the merged write path file-for-file — delete-config / delete-main / delete-mcp / delete-profile / delete-request-engine — and it reuses applyAuthentication, readBoundedBody and the existing config.ts helpers (resolveRepositoryRoot, isWithinRepository, validateEndpoint, validateStaticPath) instead of growing parallel utilities. That's the right instinct. Three questions before the code review, none of them blockers:

  • This is the third MCP server and the fourth config schema version in one integration. A separate delete-main.js does buy a real trust boundary — an administrator can enable writes without enabling deletion — so I think the split is defensible, but the V2/V3/V4/V5 ladder is getting long and someone will have to hold all of it in their head.
  • The search change drops the whole record when the ID is overlong, so the content disappears from context too. Was keeping the record with the ID withheld considered? An item that can't be used as a deletion target is still useful as reference context. The current itemSchema requires id, so I can see why omitting the record was the minimal edit — just flagging the trade.
  • expectedContent asks the model to reproduce up to 4000 code points byte-exactly, and the tests confirm canonically-equivalent-but-different Unicode is rejected as target_changed. For short memories that's reliable; for long ones I'd expect it to fail often, with no way forward for the user. It fails closed, so it's safe — but is it usable?

Risk: no elevated risk signals — none of the changed files match the high-risk paths. The genuinely unproven part is the confirmed-deletion branch: by the PR's own account it has never run against a real Mem0 service, so the exact-acknowledgement matching and the scope-field echo check are informed by the API's documented shape rather than by a live response. I'll come back to this in the review.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需小标题都在,Risk & Scope 部分对"哪些没有验证"写得异常坦诚,这正是 reviewer 需要的。

问题: 真实存在,我核实过而不是照抄描述。qwen-extension.json 目前仍只注册 context_searchmain 上的 examples/ 也只有 auto-recall 和 write 两份配置 —— 所以昨天 #11311 合入之后,记忆可以保存、可以搜索,但没有任何删除入口。这个缺口是真的。第二部分作为 bug 就弱一些:profile.ts 今天确实会把 ID 截断到 128 个码点,新测试构造的碰撞在代码层面可达,但 Mem0 发的是 UUID,真实服务不会产生超过 128 码点的 ID。我把这部分理解为"让 ID 可以安全用作删除键"的加固,而不是修复已观测到的故障 —— 可以接受,只有两行且有测试,但两部分性质不同,值得说清楚。

方向: 对齐。它补全了 writer,而不是另开一条战线;默认关闭,需要独立的 V5 实例配置、独立凭证环境变量和独立 MCP 入口;改动全部留在 integrations/ 内 —— 没有触及 packages/,因此除集成本身和它自己的集成测试外没有下游消费者。

规模: Stage 0 不适用(未触及核心路径)。供参考:约 590 行生产代码(其中约 560 行 TypeScript)、约 1727 行测试、140 行 schema JSON、约 360 行设计文档 + README、29 行 fixture。低于 1000 行大 PR 建议阈值,测试与生产代码比约 3:1。

方案: 结构与已合并的 write 路径逐文件对应 —— delete-config / delete-main / delete-mcp / delete-profile / delete-request-engine —— 并复用了 applyAuthenticationreadBoundedBody 以及现有 config.ts 辅助函数(resolveRepositoryRootisWithinRepositoryvalidateEndpointvalidateStaticPath),没有另造一套并行工具。这个取向是对的。进入代码审查前有三个问题,都不是阻塞项:

  • 这是同一个集成里的第三个 MCP server 和第四个配置 schema 版本。独立的 delete-main.js 确实换来了真实的信任边界 —— 管理员可以只开写入而不开删除 —— 所以我认为拆分站得住脚,但 V2/V3/V4/V5 这条阶梯越来越长,将来需要有人把全部版本装在脑子里。
  • 搜索侧的改动在 ID 超长时丢弃整条记录,内容也一并从上下文消失。是否考虑过保留记录但隐去 ID?一条不能用作删除目标的记录,作为参考上下文仍然有价值。当前 itemSchema 要求 id 必填,所以我理解为什么丢弃记录是最小改动 —— 只是把这个取舍点出来。
  • expectedContent 要求模型逐字节精确复现最多 4000 个码点,而测试确认"规范等价但不同"的 Unicode 会被判为 target_changed。短记忆可靠;长记忆我预计会频繁失败,且用户无路可走。它是 fail-closed 的,所以安全 —— 但是否可用?

风险: 无升级风险信号 —— 变更文件均未命中高风险路径。真正未被证明的是"确认删除"这条分支:按 PR 自己的说明,它从未对真实 Mem0 服务运行过,所以精确回执匹配和 scope 字段回显校验是依据 API 文档形态而非真实响应设计的。这一点我会在代码审查里再回来谈。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at cdcbdbc76aec9154fc89f33d8c15c1ba574b0bd3 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Code review

I wrote my own proposal before opening the diff: one context_forget(memoryId) tool on a new delete server, ID validated against the charset rule the write engine already uses, DELETE {basePath}{/memories/}{id}, then map 200 → deleted / 404 → not found / anything else → unknown. About 150 production lines, no read tool, no confirmation text, no post-delete check.

The PR is roughly four times that, and after reading it I think most of the extra is load-bearing rather than decorative. The part my version gets wrong: without context_get, the approval prompt would show the user an ID and nothing else. The model's own view of the record is a search summary capped at 1000 characters, so a user approving "delete X" would be approving something they cannot read. Fetching the complete text so it lands in the tool parameters — and therefore in the approval — closes that, and it's the reason the two-tool split exists. The post-delete absence read is also worth its round trip: the provider's DELETE acknowledgement is the weakest signal in the whole chain, and "I asked again and it was gone" is the strongest.

I traced every exit path looking for one that deletes without a verified exact match, or reports deleted without an absence check, and did not find one. Invalid input → not_deleted; abort before submission → cancelled; ID or scope mismatch on the pre-read → unavailable; text mismatch → target_changed; non-200 or unrecognized acknowledgement → unknown; any throw after submission → unknown. DELETE is only reachable after the provider's own GET for that exact ID returned that exact ID, inside the configured scope, with that exact content. Path safety holds too: the ID is charset-restricted to [A-Za-z0-9._:-], . and .. are rejected outright, and validateStaticPath rejects //, %, ?, #, \ and control characters in the configured prefix — the integration test fires ., .., %2e%2e and a/b at the packaged server and asserts zero HTTP requests went out. Credential and endpoint handling is unchanged from the merged writer, and the out-of-workspace startup test asserts stderr leaks neither the path nor the token.

No critical blockers. Three non-blocking things I'd want on the record:

1. The absence check is gated behind the weakest signal in the chain. isDeleteAcknowledgement requires HTTP 200 and a body whose message is exactly Memory deleted successfully or Memory deleted successfully!. Anything else — a 204, a 200 with different prose, a non-JSON body — returns unknown and skips the verification GET entirely; the unit test pins this at toHaveBeenCalledTimes(2), so it's deliberate, and the README documents it. But the verification read can only ever upgrade the answer: if the record is absent you learn deleted, if it's still there you return the same unknown you'd have returned anyway. It can't manufacture a false deleted, because absent still requires a 404 (or a null-200) for that exact ID. Widening the trigger to "any 2xx, then go look" would make the tool work against Mem0-compatible providers that phrase things differently, at the cost of one request inside a deadline that already budgets three. I can see why v1 was drawn narrowly; I just think the narrowness costs more than it protects.

2. The exact-text precondition is the part I'd expect to fail most often in real use. target.content !== expectedContent is a byte-for-byte comparison of up to 4000 code points that the model has to reproduce verbatim from what context_get returned, and the tests confirm canonically-equivalent-but-different Unicode is rejected. For a short memory that's reliable. For a long one, the realistic outcome is a user who asks to delete something, gets not_deleted / target_changed, and has no idea what differed — the reason string doesn't say where the mismatch was. Fails closed, so it's safe, and I don't have a better design that keeps the "approve the actual text" property. But if this turns out to be the common failure in practice, a bounded diff hint in the message would be the cheap fix.

3. context_search can now surface an ID that context_forget refuses to accept. Search only bounds the ID by length (128 code points, any characters), while delete requires the [A-Za-z0-9._:-] charset. A provider handing out an ID with a / or a space would show it in search and then reject it as invalid_input. Unreachable with Mem0's UUIDs and it fails closed, so this is a consistency note, not a defect.

On the search change itself: the new test is well-built — it pins the actual collision (a 129-character ID truncating onto a real 128-character neighbour) and the code-point boundary with emoji, so 128 surrogate-pair characters survive while 129 are dropped. That's the right way to test a Unicode-length rule. My Stage 1 question about losing the record's content along with its ID still stands as a question, not an objection.

Reuse is good: applyAuthentication, readBoundedBody and the config.ts helpers are all imported rather than reimplemented, and delete-config.ts is a faithful mirror of write-config.ts. The one near-duplicate I found — isMemoryId in delete-profile.ts versus the private isIdentifier in write-request-engine.ts — has genuinely different semantics (the former adds the dot-segment rejection), so I wouldn't unify them.

The flow, since the ordering is the design:

sequenceDiagram
    participant P1 as Model
    participant P2 as context_forget
    participant P3 as delete engine
    participant P4 as Mem0 provider
    P1->>P2: memoryId plus complete expectedContent
    P2->>P2: validate ID charset, text bounds, not aborted
    P2->>P3: forget
    Note over P3,P4: one AbortSignal.timeout covers all three calls
    P3->>P4: GET record
    P4-->>P3: id, scope fields, content
    P3->>P3: id matches, every scope field matches, content matches exactly
    P3->>P4: DELETE record
    P4-->>P3: HTTP 200 and exact success message
    P3->>P4: GET record again
    P4-->>P3: 404
    P3-->>P2: deleted
    P2-->>P1: deleted
Loading

Testing evidence

This is an unattended CI run, so I did not build or execute any PR code — everything below is the PR's own CI, read through the API for the reviewed commit. CI has now settled: every lane that covers this PR is green, and one lane that does not is red. I've classified that failure below with the evidence rather than waving it off.

Check Conclusion
Test (ubuntu-latest, Node 22.x) success
Lint & Static (ubuntu-latest, Node 22.x) success
Install (ubuntu-latest) success
Install (macos-latest) success
Install (windows-latest) success
Integration Tests (no-AK, No Sandbox) success
Desktop Shell (ubuntu-22.04) success
Desktop Shell (windows-2022) success
Classify PR / label / assign / authorize / review-config success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) failure — pre-existing infra flake, see below
Integration Tests (CLI, No Sandbox) skipped
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped

About that red check. web-shell E2E Smoke ran 53 Playwright @smoke tests on the self-hosted pool (actions-runner-hk3-16): 50 passed, 2 were classified flaky by Playwright itself, and 1 failed all three attempts — client/e2e/web-shell.history-viewport.spec.ts:189, a row-count predicate that expected <= 2 and received 286 after a 10s timeout. The log carries a vite proxy error on /session/web-shell-e2e-session and connect ECONNREFUSED 127.0.0.1:4170 immediately before it — the suite's own backend stopped accepting connections mid-run. I don't think this PR caused it, and I'd rather show the reasoning than assert it:

  • This PR changes zero files under packages/. Its 22 files are all in integrations/external-context-mem0/, docs/design/ and integration-tests/cli/. There is no path by which a Mem0 delete tool, a design doc, or a CLI integration test file changes web-shell's turn-navigation row count. It touches no lockfile and no dependency, so the install graph is unchanged too.
  • The same test failed on another PR at the same time. fix(serve): configure live-state polling with a five-second default #11339 (fix(serve): configure live-state polling) ran its web-shell E2E Smoke job at 08:14–08:19 against this PR's 08:13–08:17, and failed web-shell.history-viewport.spec.ts:189 as well — plus its sibling at line 168 — with the same ECONNREFUSED 127.0.0.1:4170, and far worse: 11 failed / 11 flaky / 31 passed. That PR touches serve code and no web-shell code either. Two concurrent runs on a shared pool, same backend refusing connections, same tests down.
  • The check is unstable generally: of the last 14 completed Qwen Code CI runs, 6 failed it, and 4 of those 6 are PRs that touch no web-shell code (feat(memory), fix(serve), feat(channels), fix(core)).

So I'm naming it as pre-existing infra noise, not as a finding against this PR. It will still hold the merge gate until it's re-run — that's a workflow condition rather than a code problem, and it deserves its own flaky-test look instead of blocking this diff.

Which of those lanes actually cover this PR matters more than the green ticks, so I checked rather than assuming:

  • The unit lane is doing the real work here. integrations/external-context-mem0 is listed in the root workspaces, so Test (ubuntu-latest, Node 22.x) runs the package's own test:ci — which is npm run build && vitest run, and the vitest config includes everything under src/ ending in .test.ts. That means delete-config.test.ts, delete-mcp.test.ts, delete-request-engine.test.ts and delete.integration.test.ts all executed and passed on Linux, alongside the modified profile.test.ts and manifest.test.ts. Roughly 1255 of the PR's ~1727 test lines ran in that green check.
  • delete.integration.test.ts is not a mock. It esbuilds the package, spawns the real dist/delete-main.js over stdio with StdioClientTransport, and talks to a real http server on 127.0.0.1. It asserts the exact request sequence (GET GET GET GET DELETE GET), that every request carried Token synthetic-token with an empty body, that the three control records survive while the target does not, and that a foreign-scope read returns unavailable without leaking the record's text. It also drives five failure modes — socket drop after DELETE, truncated pre-flight body, truncated delete body, truncated verification body, and a cumulative-deadline case that proves the single timeout spans all three calls — asserting exactly one DELETE and no replay in each. That is genuinely load-bearing coverage, and it ran.
  • The skipped CLI lane is a real gap, not noise. The PR adds integration-tests/cli/external-context-mem0-daemon-delete.test.ts (452 lines), but the integration_cli job is gated on the merge_group event and no merge queue has been enabled since 2026-07-02 — so on a pull_request it is skipped by design and will stay skipped. The Integration Tests (no-AK, No Sandbox) job that did pass runs an explicit file list which does not include this test. So those 452 lines have not executed in any lane on this PR; they'll first run in release CI or on a manual invocation. Worth knowing before merge, and it's a pre-existing workflow condition rather than something this PR introduced.
  • Not verified, and I can't verify it from here: conformance against a real Mem0 service. The author states this plainly in Risk & Scope — the temporary credential was unavailable and the preflight sent zero service requests. Everything above runs against a synthetic provider whose response shape the same PR defines, so the two places a real provider is most likely to diverge (the exact acknowledgement strings, and the requirement that the GET response echo user_id/agent_id/app_id back for the scope check) are asserted only against that fixture. Both divergences fail closed — you'd get unknown or target_unavailable, never a wrong deletion — so this is a "might not work yet" risk, not a safety one.
  • Also not verified: the macOS/Windows unit lanes (skipped) and the approval-UI surface. The author's own evidence is macOS-only and self-reported, and the PR says the full-DOM character-by-character assertion on the approval prompt never completed because the control tool timed out. I'm treating that as the author's claim, not as evidence — the assertion that the approval prompt really does render the complete ID and text is exactly the one that didn't finish.

Sandboxed verification would settle the remaining gaps, and the author has write access so both lanes are open: @qwen-code /tmux is the one I'd run — it would drive the real approval prompt as a user sees it and confirm the complete ID and text actually render there, that rejecting produces zero DELETE-side requests, and it would exercise the 452-line CLI integration lane that CI skipped. @qwen-code /verify would add less than usual here: its A/B arm compares against the base build, and the base has no delete path at all, so the interesting half would be the mock-free wire oracles over the fail-closed branches — which delete.integration.test.ts already covers against real HTTP.

中文说明

代码审查

我在打开 diff 前先写了自己的方案:在新的 delete server 上加一个 context_forget(memoryId) 工具,用 write engine 已有的字符集规则校验 ID,发 DELETE {basePath}{/memories/}{id},然后把 200 映射为 deleted、404 映射为 not found、其余映射为 unknown。大约 150 行生产代码,没有读取工具、没有确认原文、没有删除后复核。

这个 PR 大约是我的四倍,读完之后我认为多出来的部分大多是承重的,而不是装饰。我的版本错在哪里:没有 context_get,审批提示就只会给用户看一个 ID。模型自己对记录的了解只是被截断到 1000 字符的搜索摘要,所以批准"删除 X"的用户其实看不到 X 写了什么。把完整正文取回来、让它进入工具参数(因而进入审批)解决了这一点,这也是拆成两个工具的理由。删除后的"确认不存在"读取同样值这一趟:provider 的 DELETE 回执是整条链上最弱的信号,而"我再问一次,它没了"是最强的。

我把每一条退出路径都走了一遍,找有没有哪条会在未核对精确匹配的情况下删除、或者在没做不存在复核的情况下报 deleted——没有找到。输入非法 → not_deleted;提交前中止 → cancelled;预读时 ID 或 scope 不符 → unavailable;正文不符 → target_changed;非 200 或回执无法识别 → unknown;提交后任何抛错 → unknown。只有在 provider 自己针对该 ID 的 GET 返回了同一个 ID、处于配置的 scope 内、且正文完全一致之后,DELETE 才可达。路径安全也成立:ID 字符集限制为 [A-Za-z0-9._:-]... 直接拒绝,validateStaticPath 拒绝配置前缀中的 //%?#\ 和控制字符——集成测试向打包后的 server 打了 ...%2e%2ea/b,并断言零 HTTP 请求发出。凭证与端点处理与已合并的 writer 一致,越出 workspace 的启动测试断言 stderr 既不泄漏路径也不泄漏 token。

没有关键阻塞项。 三点非阻塞、但我希望记录在案:

1. 不存在复核被挡在整条链最弱的信号后面。 isDeleteAcknowledgement 要求 HTTP 200 响应体的 message 精确等于 Memory deleted successfullyMemory deleted successfully!。其余情况——204、200 但措辞不同、非 JSON 响应体——都返回 unknown 并完全跳过复核 GET;单测用 toHaveBeenCalledTimes(2) 钉住了这一点,所以是有意为之,README 也写了。但复核读取只可能提升结论:记录不在了你就得到 deleted,还在就返回同样的 unknown。它无法伪造 deleted,因为 absent 仍然要求该精确 ID 返回 404(或 null-200)。把触发条件放宽成"任何 2xx,然后去看一眼",就能让工具在措辞不同的 Mem0 兼容服务上可用,代价是在一个本来就为三次调用编列预算的 deadline 里多一次请求。我理解 v1 为什么划得窄;只是我认为这个窄带来的损失大于保护。

2. 精确原文前置条件是实际使用中最容易失败的一环。 target.content !== expectedContent 是对最多 4000 码点做逐字节比较,而模型必须从 context_get 的返回里一字不差地复现它,测试也确认"规范等价但不同"的 Unicode 会被拒。短记忆可靠;长记忆的现实结果是:用户要求删除,得到 not_deleted / target_changed,却完全不知道哪里不一样——reason 字符串不说明差异位置。它是 fail-closed 的,所以安全,而我也没有既能保留"审批看到真实正文"这个性质、又更好的设计。但如果实践中这是最常见的失败,在 message 里给一个有界的差异提示是最便宜的修法。

3. context_search 现在可能给出一个 context_forget 拒绝接受的 ID。 搜索只按长度限制 ID(128 码点,任意字符),而删除要求 [A-Za-z0-9._:-] 字符集。若 provider 发出带 / 或空格的 ID,搜索会展示它,删除会以 invalid_input 拒绝。用 Mem0 的 UUID 不会发生,且 fail-closed,所以这是一致性提示,不是缺陷。

关于搜索侧改动本身:新测试写得好——它既钉住了真实的碰撞(129 字符 ID 截断后撞上真实的 128 字符邻居),也用 emoji 钉住了码点边界,所以 128 个代理对字符保留、129 个被丢弃。这是测试 Unicode 长度规则的正确方式。我在 Stage 1 提的"记录内容随 ID 一起丢失"仍然是个问题,不是反对意见。

复用做得好:applyAuthenticationreadBoundedBodyconfig.ts 的辅助函数都是 import 而非重写,delete-config.ts 忠实对应 write-config.ts。我唯一找到的近似重复——delete-profile.tsisMemoryIdwrite-request-engine.ts 里私有的 isIdentifier——语义确实不同(前者多了点号段拒绝),所以我不建议合并。

测试证据

这是无人值守的 CI 运行,所以我没有构建或执行任何 PR 代码——以下全部是通过 API 读取的该 PR 自己在被审 commit 上的 CI 结果。CI 已经跑完:覆盖本 PR 的通道全绿,一个不覆盖本 PR 的通道是红的。下面我给出证据来归类那次失败,而不是一笔带过。

关于那个红 check。 web-shell E2E Smoke 在自建机器池(actions-runner-hk3-16)上跑了 53 个 Playwright @smoke 测试:50 通过,2 个被 Playwright 自己判为 flaky,1 个三次尝试全失败——client/e2e/web-shell.history-viewport.spec.ts:189,一个行数断言期望 <= 2 却在 10 秒超时后收到 286。日志里紧邻其前的是 /session/web-shell-e2e-session 的 vite 代理错误和 connect ECONNREFUSED 127.0.0.1:4170——测试套件自己的后端在运行途中不再接受连接。我不认为是本 PR 造成的,而我更愿意展示推理而不是直接断言:

  • 本 PR 在 packages/ 下改动了 0 个文件。22 个文件全在 integrations/external-context-mem0/docs/design/integration-tests/cli/。一个 Mem0 删除工具、一份设计文档、或一个 CLI 集成测试文件,没有任何路径能改变 web-shell 的 turn 导航行数。它也没动 lockfile 和任何依赖,安装图同样未变。
  • 同一个测试在同一时间也在另一个 PR 上失败。fix(serve): configure live-state polling with a five-second default #11339fix(serve): configure live-state polling)的 web-shell E2E Smoke job 跑在 08:14–08:19,本 PR 跑在 08:13–08:17,它同样败在 web-shell.history-viewport.spec.ts:189——外加 168 行的同族用例——同样是 ECONNREFUSED 127.0.0.1:4170,而且严重得多:11 失败 / 11 flaky / 31 通过。那个 PR 改的是 serve 代码,也没碰 web-shell。共享机器池上两个并发运行,同一个后端拒绝连接,同一批测试挂掉。
  • 这个 check 整体就不稳定:最近 14 次跑完的 Qwen Code CI 中有 6 次失败,其中 4 次的 PR 完全没碰 web-shell 代码(feat(memory)fix(serve)feat(channels)fix(core))。

所以我把它归为既有的基础设施噪声,而不是本 PR 的问题。它在被重跑之前仍会卡住合并门禁——那是 workflow 状态而非代码问题,应该单独开一个 flaky test 的排查,而不是用它阻塞这个 diff。

哪些通道真正覆盖了这个 PR,比绿色勾更重要,所以我核实过而不是假设:

  • 单测通道在这里干了实事。 integrations/external-context-mem0 在根 workspaces 列表里,所以 Test (ubuntu-latest, Node 22.x) 会跑该包自己的 test:ci——即 npm run build && vitest run,而其 vitest 配置 include 了 src/ 下所有 .test.ts。也就是说 delete-config.test.tsdelete-mcp.test.tsdelete-request-engine.test.tsdelete.integration.test.ts 都在 Linux 上真实执行并通过,连同修改过的 profile.test.tsmanifest.test.ts。该 PR 约 1727 行测试中,约 1255 行跑在了这个绿色 check 里。
  • delete.integration.test.ts 不是 mock。 它用 esbuild 打包,通过 StdioClientTransport 以 stdio 启动真实的 dist/delete-main.js,并与 127.0.0.1 上真实的 http server 通信。它断言精确的请求序列(GET GET GET GET DELETE GET)、每个请求都带 Token synthetic-token 且 body 为空、三条对照记录存活而目标记录消失,以及跨 scope 读取返回 unavailable 且不泄漏记录正文。它还驱动了五种失败模式——DELETE 后断开 socket、预检 body 截断、删除 body 截断、复核 body 截断,以及一个证明单一 timeout 覆盖全部三次调用的累积 deadline 用例——每种都断言恰好一次 DELETE 且无重放。这是真正承重的覆盖,而且它跑过了。
  • 被跳过的 CLI 通道是真实缺口,不是噪声。 该 PR 新增了 integration-tests/cli/external-context-mem0-daemon-delete.test.ts(452 行),但 integration_cli job 的条件是 merge_group 事件,而自 2026-07-02 起就没有启用 merge queue——所以在 pull_request 上它按设计被跳过,并且会一直跳过。真正通过的 Integration Tests (no-AK, No Sandbox) 跑的是一份显式文件清单,其中不含这个测试。因此这 452 行在本 PR 的任何通道里都没有执行过;它们要等到 release CI 或手动触发才会首次运行。合并前值得知道,而且这是既有的 workflow 状态,不是本 PR 引入的。
  • 未验证、我在这里也无法验证: 对真实 Mem0 服务的一致性。作者在 Risk & Scope 里说得很清楚——临时凭证不可用,预检零服务请求。上面的一切都跑在同一个 PR 自定义响应形态的合成 provider 上,所以真实服务最可能分歧的两处(精确回执字符串,以及要求 GET 响应回显 user_id/agent_id/app_id 以供 scope 校验)只对着那个 fixture 断言过。两处分歧都是 fail-closed——你会得到 unknowntarget_unavailable,绝不会删错——所以这是"可能还不能用"的风险,不是安全风险。
  • 同样未验证: macOS/Windows 单测通道(被跳过)和审批 UI 表面。作者自己的证据只在 macOS 上、且是自述的,PR 也说审批提示的 DOM 全文逐字符断言因控制工具超时而没有完成。我把它当作作者的主张而不是证据——"审批提示确实渲染出完整 ID 与正文"这条断言,恰恰是那个没跑完的。

沙箱化验证可以补上剩余缺口,且作者有写权限,两条通道都开放:@qwen-code /tmux 是我会跑的那条——它会以用户视角驱动真实审批提示,确认完整 ID 与正文确实渲染在那里、拒绝后删除侧零请求,同时也会跑 CI 跳过的那 452 行 CLI 集成通道。@qwen-code /verify 在这里增益不如平常:它的 A/B 一环是与 base 构建对比,而 base 根本没有删除路径,所以有价值的那半是针对 fail-closed 分支的 mock-free wire oracle——而那部分 delete.integration.test.ts 已经用真实 HTTP 覆盖了。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at cdcbdbc76aec9154fc89f33d8c15c1ba574b0bd3 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the safety design holds up under tracing and the test coverage is genuinely load-bearing; the reservations are about whether it will work against a real provider, not about whether it's safe.

Stepping back. My independent proposal was a quarter of this size, and I went in expecting to argue that the extra machinery was over-engineering. I can't make that argument honestly after reading it. The piece that changed my mind is the approval prompt: without context_get, a user asked to approve a deletion would be shown an ID and nothing else, while the model's own knowledge of the record is a search summary capped at 1000 characters. You'd be approving the deletion of something you cannot read. That's a real hole in the simple version, and the two-tool split is the minimum that closes it. The post-delete absence read earns its round trip for the same reason — the provider's acknowledgement is the weakest signal available and "I asked again and it was gone" is the strongest.

What I actually verified rather than accepted: I walked every exit path in delete-request-engine.ts looking for one that submits a DELETE without a verified exact match, or one that reports deleted without an absence check. There isn't one. DELETE is reachable only after the provider's own GET for that exact ID echoed that exact ID back, inside every configured scope field, with byte-identical content. Every other outcome — invalid input, pre-submission abort, scope mismatch, text mismatch, non-200, unrecognized acknowledgement, any throw after submission — lands on a status that does not claim success. For a destructive, irreversible operation behind an MCP approval prompt, failing closed on all seven branches is the whole ballgame, and it does.

Six months from now I'd thank whoever maintains this rather than curse them. The design doc and the README spell out the contract, including the parts that don't work yet; delete-config.ts mirrors write-config.ts closely enough that knowing one tells you the other; and it reuses applyAuthentication, readBoundedBody and the existing config.ts helpers instead of growing a parallel set. The maintenance cost is the V2/V3/V4/V5 config ladder and a third MCP server in one integration — real, but bounded and documented, and the separate server buys a trust boundary I think is worth it: an administrator can enable writes without enabling deletion.

The scope is honest. Every change maps to the stated goal, there's no formatting churn or drive-by refactor, and the one edit outside the delete path — dropping overlong IDs from search instead of truncating them — is two lines, is arguably a prerequisite for treating IDs as deletion keys, and comes with a test that pins the actual collision rather than a synthetic one. I'd have preferred it as its own commit so it's revertable separately, but that's a preference, not a finding.

On the pattern question: this is the third PR in a line from the same author (#11246 auto-recall, #11311 writes, this one), all merged, and the author is reviewing their own feature line with self-reported evidence. I checked whether that was wearing down my judgment rather than being evaluated on merit, and I don't think it is — the series is coherent, each PR is separately justified, and this one's verification story is weaker than the last one's only in the sense that the last one had the same limitation. But it's exactly why I'd run /tmux before merge rather than after: the author's own DOM assertion on the approval prompt timed out, so the single most important user-facing claim — that the approval really renders the complete ID and text — is currently resting on their word.

Two things a maintainer should know before merging, neither of which I think blocks it:

  • The red web-shell E2E Smoke check is not this PR's, and I've laid out the evidence in the Stage 2 comment — zero changed files under packages/, the same test failing concurrently on fix(serve): configure live-state polling with a five-second default #11339 with the same ECONNREFUSED on port 4170, and 4 of the last 6 failures of that check on PRs that touch no web-shell code. It will still hold the merge gate, so it needs a re-run, and it deserves its own flaky-test look. My approval is about the code, not a claim that the commit is mergeable right now.
  • The happy path has never met a real Mem0 service. The acknowledgement strings and the requirement that GET echo the scope fields back are both inferred from the documented API shape and asserted only against a fixture this same PR defines. If either is wrong in practice you get unknown or target_unavailable — a feature that doesn't work yet, never a wrong deletion. Worth a follow-up against a real credential, and worth knowing that the narrow acknowledgement matching (finding 1 in Stage 2) is the most likely thing to need loosening when that happens.

Approving, pinned to the commit I reviewed. The three Stage 2 findings are non-blocking and I'd be happy to see them as follow-ups rather than as churn on this diff.

中文说明

Confidence: 4/5 —— 安全设计经得起逐路径推演,测试覆盖是真正承重的;我的保留意见在于它对真实 provider 是否能用,而不在于它是否安全。

退一步看。我自己的方案只有这个的四分之一,我本来是打算论证多出来的机制是过度设计的。读完之后我没法诚实地这么讲。让我改变想法的是审批提示:没有 context_get,被要求批准删除的用户只会看到一个 ID,而模型自己对记录的了解只是被截断到 1000 字符的搜索摘要。那就是在批准删除一个你读不到的东西。这是简化版里真实存在的漏洞,而拆成两个工具是补上它的最小改动。删除后的"确认不存在"读取同样值这一趟——理由相同:provider 的回执是现有最弱的信号,而"我再问一次,它没了"是最强的。

我真正核实过、而不是照单接受的:我把 delete-request-engine.ts 的每一条退出路径都走了一遍,找有没有哪条会在未核对精确匹配的情况下提交 DELETE,或者哪条会在没做不存在复核的情况下报 deleted。一条都没有。只有在 provider 自己针对该精确 ID 的 GET 把同一个 ID 回显回来、且落在每一个已配置的 scope 字段内、且正文逐字节一致之后,DELETE 才可达。其余所有结果——输入非法、提交前中止、scope 不符、正文不符、非 200、回执无法识别、提交后任何抛错——都落在一个不宣称成功的状态上。对一个处于 MCP 审批提示之后、不可逆的破坏性操作来说,七条分支全部 fail-closed 就是全部关键,而它做到了。

六个月后维护它,我会感谢作者而不是骂作者。设计文档和 README 把契约写清楚了,包括现在还不能用的部分;delete-config.tswrite-config.ts 对应得足够紧密,懂一个就懂另一个;它复用 applyAuthenticationreadBoundedBody 和现有 config.ts 辅助函数,而不是另造一套。维护成本是 V2/V3/V4/V5 这条配置阶梯和一个集成里的第三个 MCP server——真实存在,但有界且有文档,而独立 server 换来的信任边界我认为值得:管理员可以只开写入而不开删除。

范围是诚实的。每处改动都对应既定目标,没有格式化噪声或顺手重构;delete 路径之外唯一的那处改动——搜索丢弃超长 ID 而不是截断——只有两行,可以说正是"把 ID 当作删除键"的前提,而且配的测试钉住的是真实碰撞而不是人造场景。我更希望它是一个独立 commit,以便单独回滚,但那是偏好,不是发现。

关于"是否形成了模式":这是同一作者的系列第三篇(#11246 auto-recall、#11311 writes、本篇),全部已合并,而且作者在用自述证据审自己的特性线。我检查过这是否在磨损我的判断而不是按实质评估,我认为没有——这个系列是自洽的,每个 PR 都各自站得住,本篇的验证故事比上一篇弱,也只是在"上一篇有同样局限"这个意义上。但这恰恰是我会在合并前而不是合并后跑 /tmux 的原因:作者自己对审批提示的 DOM 断言超时了,所以最重要的那条面向用户的主张——审批确实渲染出完整 ID 与正文——目前是靠作者的口头保证。

维护者合并前应当知道两件事,我都不认为构成阻塞:

  • 红色的 web-shell E2E Smoke 不是本 PR 造成的,证据我放在 Stage 2 评论里——packages/ 下改动为 0,同一测试在同一时间也在 fix(serve): configure live-state polling with a five-second default #11339 上以同样的 4170 端口 ECONNREFUSED 失败,而该 check 最近 6 次失败里有 4 次的 PR 完全没碰 web-shell 代码。它仍然会卡住合并门禁,所以需要重跑,也值得单独开一个 flaky test 排查。我的批准针对的是代码,不是在宣称这个 commit 现在就可合并。
  • 成功路径从未遇到过真实 Mem0 服务。 回执字符串、以及要求 GET 回显 scope 字段这两点,都是从文档化的 API 形态推断出来、且只对着本 PR 自己定义的 fixture 断言过。如果实践中任一处不对,你会得到 unknowntarget_unavailable——一个还不能用的特性,绝不会是一次错误删除。值得在拿到真实凭证后补一次验证,也值得知道:真到那时候,最可能需要放宽的就是那条狭窄的回执匹配(Stage 2 的发现 1)。

批准,并钉在我审过的那个 commit 上。Stage 2 的三条发现都是非阻塞的,我更乐意看到它们作为后续工作,而不是在这个 diff 上再翻一轮。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at cdcbdbc76aec9154fc89f33d8c15c1ba574b0bd3 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. The three Stage 2 findings are non-blocking follow-ups, and the red web-shell E2E Smoke check is pre-existing infra flake unrelated to this diff (evidence in the Stage 2 comment) — it still needs a re-run to clear the merge gate. ✅

@doudouOUC
doudouOUC requested a review from wenshao September 8, 2026 09:25

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #11337 Review — feat(external-context): Add daemon memory deletion

Tier: Standard(新 MCP delete server + 破坏性工具,仅限 integrations/ 包,packages/ 零改动)


审查结果:无阻塞问题,批准 ✅


已验证

Class 1 — Contract 对称性:CLEAN

  • isMemoryIdvalue.length <= 256 检查长度,允许字符集全为 ASCII([A-Za-z0-9._:-]),每个字符恰好是一个 UTF-16 code unit,所以 .length 与 code point 计数等价。✓
  • isDeletionContentArray.from(value).length <= 4000 正确按 Unicode code point 计数。✓
  • readTargetvalue[idField] !== memoryId 做逐字节精确比对,不宽松。✓
  • DeleteInstanceConfigV5schemaVersion: 5 与 V2/V3/V4 严格分离,独立配置路径,不自动继承 writer/reader 权限。✓

Class 3 — 错误处理:CLEAN

逐条追踪 forget() 的所有出口:

触发条件 结果 是否安全
isMemoryId/isDeletionContent 失败 not_deleted / invalid_input
提交前 signal.aborted not_deleted / cancelled
预读 ID 不匹配 not_deleted / verification_failed
预读 scope 不符 not_deleted / target_unavailable
预读正文不符 not_deleted / target_changed
DELETE 返回非 200 unknown ✓(fail-closed)
DELETE 回执无法识别 unknown ✓(fail-closed)
验证 GET 未返回 absent unknown ✓(不伪造 deleted
提交后任何 throw unknownsubmitted=true 分支)

没有任何路径在未经预读精确核对的情况下执行 DELETE,也没有任何路径在没有 absence 验证的情况下返回 deleted

Class 4 — 授权与路径安全:CLEAN

  • scope 核对循环:只检查 expected !== undefined 的 scope 字段;若响应中缺少或值不符则返回 unavailable,不泄漏内容。✓
  • isWithinRepository 防止 delete server 在 workspace 外运行。✓
  • validateStaticPath 阻止配置 prefix 中的 //%?#\ 和控制字符。✓
  • encodeURIComponent(memoryId) + charset 严格限制,防止路径注入。✓
  • stderr 不泄漏路径和 token(集成测试明确断言)。✓

Class 10 — 代码与设计文档一致性:CLEAN

  • 设计文档:"最多执行一次精确 GET" → get()readTarget 调用一次。✓
  • 设计文档:"最多提交一次 DELETE 并复核不存在" → forget() 在通过全部前置检查后执行一次 DELETE,再做一次验证 GET。✓
  • 设计文档:"执行端重新核对 ID、全部配置 scope 和完整正文" → readTarget() 执行三层核对(ID → scope → content),与描述一致。✓

profile.ts ID 截断修复:CLEAN

id: truncate(source.id, 128) 改为跳过 Array.from(source.id).length > 128 的记录。正确:返回截断 ID 会形成指向另一条记录的假 key,现在改为完整放行或整条省略。新测试钉住了 emoji 码点边界(128 代理对保留、129 丢弃)和真实碰撞场景。✓

累积超时覆盖所有三次 HTTP 调用:SAFE

AbortSignal.any([signal, AbortSignal.timeout(timeoutMs)])request() 中构造一次,被 GET、DELETE、验证 GET 共享。超时在第一次 GET 之前开始,剩余预算覆盖后续调用。若超时在 DELETE 之后触发,外层 catchsubmitted=true 返回 unknown——fail-closed,不声称 deleted。集成测试中 600 ms 总预算 × 3×230 ms 延迟已验证此行为。✓


Cross-check(与 qwen-code-ci-bot 的 APPROVED Review 对比)

CI bot 已作三阶段深度分析(Stage 1-3)。我独立得出的三项 non-blocking 发现与其一致:

  1. isDeleteAcknowledgement 的触发门槛偏窄(仅识别两个精确 message 字符串),导致 204、措辞略有不同的 200 等均跳过验证 GET,返回 unknown——但 absent 分支无法伪造 deleted,fail-closed,安全。
  2. 精确正文前置条件可能成为高频失败点(长记忆的逐码点完全比对),not_deleted / target_changed 的 reason 不给差异位置提示——安全,但可用性折中。
  3. context_search 可能返回 context_forget 拒绝的 ID(charset 限制不同),fail-closed(invalid_input),不是缺陷。

CI bot 的额外识别(452 行 CLI 集成测试因 merge_group 门控未在 CI 中执行、Holo 真实服务未验证)与我的覆盖评估一致。我未发现额外阻塞项。


未审查维度

  • 无本地工作树,未执行测试套件;delete.integration.test.ts 已通过真实 stdio + HTTP server 覆盖所有 fail-closed 分支。
  • 对真实 Holo 服务的协议一致性(临时凭证不可用,作者已在 Risk & Scope 中明确说明)。

Reviewed with AI assistance.

@doudouOUC
doudouOUC added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit ab33974 Sep 8, 2026
109 of 111 checks passed
@doudouOUC
doudouOUC deleted the codex/mem0-daemon-delete branch September 8, 2026 09:41
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.1.

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