fix(external-context): Align deletion responses with Mem0 SDK - #11397
Conversation
Accept successful HTTP responses and parse JSON without fixed acknowledgement rules, retaining exact post-delete absence verification. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E test report / E2E 测试报告12/12 scenarios passed using real daemon/MCP processes with default permission handling, a controlled model and a local HTTP provider. This reproduces the previously captured real Holo acknowledgement; no fresh cloud-service requests or credentials were used.
Nine DELETE requests across the suite, at most one per submitted call. Test daemons and temporary directories were removed. Independent focused review found no issues. A first harness run parsed the wrong daemon error-output field; the harness was corrected and the full suite rerun without product changes. 12 项均通过：使用真实 daemon/MCP 默认审批、受控模型和本地 HTTP 服务复现已捕获的 Holo 回执；没有重新访问云实例或使用真实凭证。带 ID 回执、任意 JSON 和 202 均须精确读取确认不存在才返回 deleted；目标仍在或响应/连接失败返回 unknown，不重试。拒绝审批为零请求，scope/正文不匹配仅一次 GET。合计 9 次 DELETE，每次调用最多一次，临时进程及目录已清理。独立审查无发现。首轮测试脚本读取了错误的 daemon 错误输出字段，修正脚本后全套重跑通过，未因此修改产品。 Validation: root build, typecheck and bundle; package lint; final package suite 326 tests across 17 files. macOS / Node.js v22.22.3 / npm 10.9.8. Windows and Linux were not locally tested. PolarDB GET-wrapper and literal-write compatibility are outside this patch. 验证：全仓 build、typecheck、bundle，包内 lint，以及最终 17 个文件共 326 项测试。环境为 macOS / Node.js v22.22.3 / npm 10.9.8，未在本地验证 Windows/Linux。PolarDB 读取包装及原文写入兼容不在本补丁范围。 |
|
Thanks for the PR! Third pass, same head — nothing has landed since Template looks good ✓ — all required sections present, including the Risk & Scope tradeoffs and the bilingual block. Problem: observed, not theoretical. A real Holo deletion returned HTTP 200 with Direction: aligned. This is a follow-up to the just-merged #11337, correcting a contract that landed too narrow rather than adding new surface. The important part is what it does not weaken: Size: the core-module gate does not apply — nothing under Approach: the scope feels right, and I could not find a meaningfully simpler version of it. Dropping the fixed-message check is the minimal correct fix; the alternative — widening the string list to also accept the interpolated form — would have been worse: more branches, still guessing at provider prose. No drive-by refactors either. The design-doc table churn is just Prettier re-padding a column after one cell grew, and the README edits are the user-facing half of the same contract. One honest question, not a blocker, and unchanged from my last pass: an empty-body 2xx — Risk: no elevated risk signals — no revert-correlated paths touched. The redirect-safety property also survives: Moving on to code review. 🔍 中文说明感谢贡献！第三轮审查，head 未变 —— 自 模板完整 ✓ —— 所有必需章节齐全，包含 Risk & Scope 的取舍说明和中英文对照。 问题： 是已观测到的问题，不是理论性加固。真实 Holo 删除返回 HTTP 200 和 方向： 对齐。这是对刚合并的 #11337 的后续修正，纠正的是落地时定得过窄的契约，而不是新增能力。关键在于它没有削弱什么： 规模： 核心模块门禁不适用 —— 没有触及 方案： 范围合理，我找不到明显更简的做法。去掉固定文案判断就是最小的正确修复；另一种选择——把插值形式也加进字符串白名单——反而更糟：分支更多，且仍在猜测服务端文案。也没有夹带顺手重构：设计文档表格的变化只是某个单元格变长后 Prettier 重新对齐列宽，README 的改动是同一契约面向用户的那一半。 一个诚实的疑问，不是阻塞项，与上次相同：空响应体的 2xx —— 也就是 风险： 无升级风险信号 —— 未触及与回滚相关的历史高危路径。重定向安全性也得以保留： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent proposal first. Reading only the title and the "Why it's needed" section, the bug is that a provider's free-text The PR matches (a), and on this head it exceeds it. What I verified rather than taking the description's word for:
Re-verified on this pass, not carried over on trust. The head is unchanged since my last review, so the findings above stand — but I re-checked them against the head sources rather than against my own earlier comment. A correction to my own previous pass, because it is still sitting in this thread. I wrote that the discarded One non-blocking coverage note, carried over. The most alarming newly-accepted shape — HTTP 200 with I skipped both optional enrichments again: no sequence diagram, because the GET → DELETE → GET flow already existed and only one predicate on it changed; no changed-files table, at six files of which two are docs and two are tests. Test evidenceUnattended CI run — I did not build, run, checkout or apply anything from this PR. The evidence below is this PR's own CI, read through the API for commit CI on this commit has now finished: one red check, and it is not this PR's. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The red check, classified with commit-level evidence rather than a shrug. Every link in the chain below was checked, not assumed:
So this is a broken-main window that this PR's merge commit happened to sit inside. Merging current main into the branch clears it; no change to this PR's own code is required. I am calling it pre-existing and not attributable with the commits named — not "known flake". A The suite that actually covers this diff passed inside that same failing job:
The web-shell E2E red my previous pass left open in this thread is now settled. The behavioural claim is now substantiated — and not by me. Last pass I named
A second Real-scenario testing: N/A for this run. This is an unattended CI execution, so nothing was driven in tmux and no terminal capture is pasted — that lane is local-invocation only. Stated plainly rather than papered over: the author's E2E report covers macOS only, with Windows and Linux untested, and says outright that no fresh live Holo or PolarDB acceptance was run after the patch. That is the author's claim, not evidence I re-ran. The verify run reproduces the wire shape the author describes against a local provider — a faithful replay of the reported bytes, not a live Holo call. PolarDB's GET-wrapper and literal-write behaviour are explicitly out of scope. 中文说明代码审查先说我自己的方案。 只看标题和"Why it's needed"，这个 bug 的本质是把服务端自由文本 本 PR 采用的正是 (a)，而且在当前 head 上超出了我的方案。 以下是我逐一核对、而非照抄描述的部分：
本轮重新核对过，不是出于信任而沿用。 自上次审查以来 head 没有变化，所以上述结论依然成立 —— 但我是拿 head 源码重新核对的，而不是拿我自己上一条评论核对的。 对我上一轮审查的一处更正，因为它仍留在本讨论串里。 我曾写那个被丢弃的 一条非阻塞的覆盖度备注，延续上轮。 最令人警惕的新接受形态 —— HTTP 200 且响应体为 我再次跳过了两项可选增强：没有时序图，因为 GET → DELETE → GET 流程本来就有，只改了其中一个判定条件；也没有变更文件表，六个文件里两个是文档、两个是测试。 测试证据无人值守 CI 运行 —— 我没有构建、运行、checkout 或 apply 本 PR 的任何内容。 下面的证据是本 PR 自己的 CI，通过 API 针对提交 该提交上的 CI 现已跑完：一项红灯，而且不是本 PR 造成的。（检查结论表见上方英文区，由 finalize 作业在 CI 结束后原地更新。） 这项红灯的归类，附提交级证据，而不是含糊带过。 下面每一环都是核对过的，不是假设：
因此这是本 PR 的合并提交恰好落在其中的一段"main 已坏"窗口。把当前 main 合并进本分支即可消除，本 PR 自身代码无需任何改动。 我把它归为既存问题、不可归因于本 PR，并且点名了具体提交 —— 而不是称之为"已知 flake"。对未定义标识符的 真正覆盖本 diff 的那套测试，是在同一个失败作业内部跑绿的：
上一轮留在本讨论串里的那项 web-shell E2E 红灯现已定论。 行为性主张现已有实证 —— 而且不是我给的。 上一轮我点名
第二次 真实场景测试：本次运行为 N/A。 这是无人值守 CI 执行，因此没有在 tmux 中驱动产品、也没有粘贴终端截取 —— 那条通道仅限本地调用。此处如实说明未验证的部分，而不是掩盖：作者的 E2E 报告只覆盖 macOS，Windows 与 Linux 未测试，并明确说明打补丁后没有重新做真实 Holo 或 PolarDB 验收。那是作者的主张，不是我复跑的证据。verify 运行复现的是作者所描述的线上字节形态（对接本地服务），是对所报字节的忠实回放，不是对真实 Holo 的调用。PolarDB 的 GET 包装结构与原文写入语义明确不在本次范围内。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the code is right, the behavioural claim is now independently substantiated rather than argued from the diff, and the single red check on this commit is provably main's and not this PR's; what keeps it off 5 is that the branch has to merge current main before that check can go green, plus two notes I am explicitly not blocking on. Stepping back, the picture is unchanged from my last pass and better-evidenced. This is a maintainer correcting a contract they landed hours earlier in #11337, and the correction removes code: +3/−17 production lines, and the function being deleted was the entire source of the bug. I went looking again for the way this could be wrong, and the honest answer is still that the safety model never depended on the thing being changed. The user-facing bug is concrete and worth fixing on its own terms: someone deleted a memory from Holo, the record really was gone, and the tool told them What moved this pass is the evidence, not my reading of the diff. Two things landed since I last wrote here:
Am I approving because it's genuinely good, or because I ran out of reasons to say no? The former. My proposal before reading the diff was to drop the body match entirely and let the HTTP status plus the confirmation GET decide — which is what this does. The alternatives I considered, tolerating an interpolated ID by pattern or making the accepted message set configurable per dialect, both keep guessing at provider prose and add surface to maintain. I could not find a simpler version of this fix, and I looked harder this pass precisely because the answer was already sitting in the thread. Two things on the record, both non-blocking, both already stated in the review above:
Three things I want stated rather than implied:
If I maintain this in six months I'd thank the author. The code got shorter, the two engines in the package now agree on what a successful HTTP response is instead of each having its own idea, the user-facing message tells the truth about what is actually verified, and the docs and README moved in the same commit as the behaviour rather than after it. ✅ Approving, pinned to 中文说明Confidence: 4/5 —— 代码是对的，行为性主张现在有独立实证、而不再是从 diff 推论出来的，而本提交上唯一的红灯可以被证明属于 main、不属于本 PR；没给到 5 分的原因是：该分支必须先合并当前 main，那项检查才能转绿，另有两条我明确不作为阻塞项的备注。 退一步看整体：结论与上一轮相同，但证据更充分。这是一位 maintainer 在修正自己几小时前随 #11337 落地的契约，而这次修正是删代码：生产代码 +3/−17，被删掉的那个函数正是 bug 的全部来源。我又一次去找"这可能错在哪"，诚实的答案仍然是：安全模型从来就不依赖于被改动的这一环。 面向用户的 bug 具体、且本身就值得修：有人从 Holo 删除了一条记忆，记录确实没了，而工具在一个破坏性操作上告诉他 本轮变化的是证据，不是我对 diff 的解读。自我上次发言后落地了两件事：
我批准，是因为它真的好，还是因为我找不到反对理由了？是前者。我在读 diff 之前的方案就是：彻底去掉响应体匹配，交给 HTTP 状态加那次复核 GET 判定 —— 本 PR 做的正是这件事。我考虑过的另外两条路（用模式容忍插入的 ID、或把可接受 message 集合做成按 dialect 可配置）都仍在猜测服务端文案，并且增加了要维护的表面积。我找不到更简的版本；这一轮我查得更用力，恰恰因为答案已经躺在讨论串里了。 两条记录在案的备注，均非阻塞，且都已在上面的审查中说明：
三件我希望明说、而不是暗示的事：
如果六个月后由我来维护这份代码，我会感谢作者。代码变短了，包里两个引擎对"什么算成功的 HTTP 响应"终于达成一致、而不是各自定义，面向用户的文案如实说明了实际校验了什么，文档与 README 也是与行为同一个提交里改的、而不是事后补的。 ✅ 已批准，绑定到 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…s statuses Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 311 passed · 0 failed · 311 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：311 通过 · 0 失败 · 311 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 11397 — deep verificationVerdict: 中文摘要结论： A/B 结论。 本 PR 实际包含两个独立的改动：(A) 与 SDK 对齐的说法已对着 SDK 自己的代码验证。 从 npm 取回 204/空响应/非 JSON 的取舍经测量属于「与 SDK 一致」，不是遗漏。 我原本怀疑这是与被修复 bug 同一类的漏网形状，实测否证了更坏的读法：SDK 的 变异矩阵：9/9 全部被杀，0 存活，对照组 328/328 绿。 每个变异都重建了 门禁（均带存活性证明）。 包内 vitest 在受验 HEAD 树上 17 文件 / 328 用例全绿； 发现（均为 nit，不阻塞），见 Findings 表：一是 未覆盖：仓库级 daemon E2E（PR 声称的「12 个 daemon 场景」）、真实 Holo/PolarDB 验收、Windows、以及逐 commit 归因（depth-2 浅克隆下 1. Scope selectionDiff is 6 files / +92 −77, entirely inside
Central claim. A DELETE that returns a successful HTTP status with arbitrary JSON is no longer rejected by message text or response-field rules; the follow-up exact GET alone decides Secondary claims. (1) The handling matches the official Mem0 client. (2) Malformed/empty bodies, non-success statuses and connection loss still yield The change bundles two independent hunks, so the A/B has four arms rather than two (§2). Out of scope by choice: the repo-level daemon E2E, live providers, Windows (§6). 2. Central claim — four-arm A/BThe PR's engine change is two separable edits:
Arms: A=base (neither), B=hunk A only, C=hunk B only, D=head (both). Each arm is the real Witness: 32 cells × 4 arms × 2 oracles (status, request count) + 5 harness-validity + 6 wire-shape = 267 assertions, 267 passed. Selected cells (
Attribution — what each half buys. G1 is the reported bug: hunk B alone fixes it; hunk A contributes nothing (B= No retry, on any path. Every failure cell sits at exactly 2 requests (GET, DELETE) or 3 (GET, DELETE, GET) — never 4. G5 covers 7 non-success statuses, G8 covers transport loss mid-DELETE, and the body-veto group (§3) covers 8 more; none re-issued a DELETE. Wire oracle, both sides (6 assertions, head arm, G1). The peer saw exactly 3. The SDK-alignment claim, tested against the SDKThe description and README both claim alignment with the official client. That is falsifiable, and the registry was reachable, so I ran the real thing rather than reasoning about it.
if (!response.ok) { const errorData = await response.text(); throw createExceptionFromResponse(...); }
const jsonResponse = await response.json();
return snakeToCamelKeys(jsonResponse);That is hunk A verbatim ( Witness: 11 statuses × 13 bodies = 143 cells, each reduced to one boolean — did this gate accept the DELETE response? — with all three gates reading the same scripted response from the same server:
The 6 HEAD divergences are exactly the two hardenings the README already discloses ("bounded to 1 MiB", "bounded UTF-8 JSON"). Asserted as a set, not eyeballed: the distinct body names where the SDK accepts and HEAD refuses are precisely Set invariants (all asserted, all passed) — these are the safety-critical direction:
Redirect policy diverges deliberately, and in the repo's favour. The SDK calls SDK differential: 19 assertions, 19 passed. The declared 204 / empty-body / non-JSON tradeoff — measured, and the scarier reading does not holdThe description and README both declare that empty responses (including 204) and invalid JSON remain
So yes: on these shapes the tool reports
What survives is a design-choice observation, not a defect: the discarded 4. Vacuity and mutation matrixWitness: Run in
9/9 killed, 0 survivors, control green. No coverage gaps, no dead guards and no redundant-defence rows to adjudicate on the changed surface. Two rows carry the most information:
Every kill quoted its intended behavioural assertion; none was an import, compile or fixture failure. 5. FindingsNo blocking findings. Two nits, both measured, neither requiring action for this PR. N1 (nit) — the discarded
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally: the repo-level daemon delete E2E (integration-tests/cli/external-context-mem0-daemon-delete.test.ts) was never executed by this review; the changed workspace's own real-HTTP delete.integration.test.ts did run green among the 328.
Not explored to full depth (tool budget reached): "agent 1d": none — approximately 7 tool calls used of the ~36 budget; no check was cut short..
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally: the repo-level daemon delete E2E (integration-tests/cli/external-context-mem0-daemon-delete.test.ts) was never executed by this review; the changed workspace's own real-HTTP delete.integration.test.ts did run green among the 328.
未探索到全部深度（达到工具调用预算）："agent 1d"：none — approximately 7 tool calls used of the ~36 budget; no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| DELETE follows the official client's response handling: require a successful | ||
| HTTP status and parse the bounded UTF-8 JSON response without matching message |
There was a problem hiding this comment.
[Suggestion] R2-1: This sentence makes "the official client" the normative basis for the whole DELETE response contract — here and at docs/design/external-context-mem0-explicit-delete.md:135 (官方客户端) — but nothing in the repository pins which client, which version, or which commit. That is out of step with the rest of the same design doc, whose evidence paragraph at line 27 pins every other external claim to a fixed commit (dae67f74 for server/main.py#L443, plus memory/main.py#L1208) and the two Platform reference pages, and lists server-side sources only.
It costs more than tidiness. The rule being justified here is the decision to accept any 2xx, including the 201/202 that it.each([201, 202]) now pins, and design doc line 201 makes that document the acceptance gate ("服务端协议验收通过后才能宣布相应服务支持"). With no pinned client revision, a maintainer asked whether a 204 with an empty body should stay unknown, or whether a later client tolerates a non-JSON success body, has nothing to re-read, and the next change to this rule re-litigates it from scratch — the same unpinned-upstream-prose shape that produced the fixed-message check this PR exists to remove.
The claim itself checks out, so nothing here needs re-scoping: at mem0ai/mem0@main, mem0/client/main.py:405-411 does response.raise_for_status() and then return response.json() — any 2xx accepted, JSON parsed, no message/status/event/cascade/error rules — so the accepted-status set genuinely is the client's. Only the citation is missing. Worth knowing that the client has already moved in adjacent respects the repo nowhere records (a delete_linked parameter, params=, a trailing-slash path), which is exactly what a pin would have surfaced.
Witness:
sweep 官方客户端|official client|client/main\.py|MemoryClient|mem0ai/mem0
-> 3 hits, all prose: README.md:538, design doc:135, design doc:27
-> 0 client artifacts / paths / versions / commits anywhere in the repo
sweep pinned refs (dae67f74|blob/[0-9a-f]{7,40}) over docs/ + the package
-> only design doc:27
authority fetched at the guessed revision `main`:
mem0/client/main.py:405-411
response = self.client.delete(f"/v1/memories/{_encode_path_segment(memory_id)}/", params=params)
response.raise_for_status()
return response.json()
Pin the client the way section 2 pins the server: add the SDK name, repo path and a fixed commit for the delete call being mirrored to the evidence paragraph at docs/design/external-context-mem0-explicit-delete.md:27, and reference it from this sentence. If what is mirrored is only the client's "parse JSON, do not match message text" behaviour rather than its whole accepted-status set, saying exactly that would close the gap too.
The citation has to meet the standard the neighbouring ones set: design doc line 27 pins its sources as "OSS 依据为固定提交 dae67f74 的 server... 它们不是 Holo 部署版本的实现证明" — a fixed commit rather than a floating doc page, and not presented as proof of any deployed service's behaviour.
中文说明
这句话把「官方客户端」确立为整个 DELETE 响应契约的规范依据 —— 此处以及 docs/design/external-context-mem0-explicit-delete.md:135（官方客户端）—— 但仓库里没有任何地方固定它指的是哪个客户端、哪个版本、哪个提交。这与同一份设计文档的其余部分不一致：该文档第 27 行的依据段落把其他每一条外部结论都钉在固定提交上（server/main.py#L443 的 dae67f74，以及 memory/main.py#L1208）和两个 Platform 参考页面上，而且只列了服务端来源。
这不只是整洁问题。此处要论证的规则是「接受任意 2xx」，其中包括 it.each([201, 202]) 新固定下来的 201/202；而设计文档第 201 行使该文档成为验收门禁（「服务端协议验收通过后才能宣布相应服务支持」）。在没有固定客户端版本的情况下，若有维护者要判断「空响应体的 204 是否应保持 unknown」或「更新版本的客户端是否容忍非 JSON 的成功响应体」，将无任何可复查的依据，下一次修改该规则只能从头再论证一遍 —— 这正是本 PR 要移除的固定文案判断当初产生的形态：拿未固定的上游文案当协议常量。
该主张本身是成立的，因此这里不需要重新界定范围：在 mem0ai/mem0@main 上，mem0/client/main.py:405-411 的实现是 response.raise_for_status() 后 return response.json() —— 接受任意 2xx、解析 JSON、不检查 message/status/event/cascade/error 字段 —— 所以「接受的状态集合」确实来自该客户端。缺的只是引用。值得注意的是，该客户端在本仓没有任何记录的相邻方面已经发生变化（delete_linked 参数、params=、路径末尾斜杠），而这恰恰是固定引用本可以提前暴露的。
证据：
sweep 官方客户端|official client|client/main\.py|MemoryClient|mem0ai/mem0
-> 3 hits, all prose: README.md:538, design doc:135, design doc:27
-> 0 client artifacts / paths / versions / commits anywhere in the repo
sweep pinned refs (dae67f74|blob/[0-9a-f]{7,40}) over docs/ + the package
-> only design doc:27
authority fetched at the guessed revision `main`:
mem0/client/main.py:405-411
response = self.client.delete(f"/v1/memories/{_encode_path_segment(memory_id)}/", params=params)
response.raise_for_status()
return response.json()
请像第 2 节固定服务端那样固定客户端：把被对齐的那次 delete 调用所在的 SDK 名称、仓库路径与固定提交补进 docs/design/external-context-mem0-explicit-delete.md:27 的依据段落，并从这句话引用它。如果实际对齐的只是客户端「解析 JSON、不匹配 message 文案」这一行为、而非其整个可接受状态集合，那么把这一点写清楚同样能补上缺口。
该引用需要达到相邻引用的标准：设计文档第 27 行把自己的来源固定为「OSS 依据为固定提交 dae67f74 的 server... 它们不是 Holo 部署版本的实现证明」—— 是固定提交而非会漂移的文档页面，且不作为任何已部署服务行为的证明。
— qwen3.8-max via Qwen Code /review (v0.23.1)
| if (outcome.status === 'deleted') { | ||
| expect(result.structuredContent).toMatchObject({ | ||
| message: |
There was a problem hiding this comment.
[Suggestion] R2-2: The assertion added here branches on outcome.status === 'deleted' inside an it.each that iterates all three forget statuses, under the name renders $status with fixed text and no full-text echo. So the fixed text of not_deleted and unknown is asserted by no test in this package. The one that matters is unknown: its "Do not retry automatically; explicitly read the target to check its current state." clause is the string whose whole purpose is to stop an agent re-issuing a DELETE whose effect is unknown, and it can be deleted with the suite staying green.
The asymmetry is new in this diff rather than pre-existing. Before it, no forget message was asserted at all; this commit added a status-conditional pin inside a test whose name advertises all three statuses, so it pinned one and left the safety-critical two unpinned. The sibling package already pins the same text — integrations/external-context/src/mcp.test.ts:432 and :550 assert toContain('Do not retry automatically.').
Witness:
BASELINE (pristine PR src) : Test Files 17 passed (17) / Tests 328 passed (328)
MUTANT not_deleted -> 'MUTATED not_deleted text.'
unknown -> 'The record may have been deleted.' (safety clause dropped)
arm proven : grep -c "MUTATED not_deleted text." dist/delete-main.js => 1
grep -c "Do not retry automatically; explicitly read the target" dist/delete-main.js => 0
result : Test Files 17 passed (17) / Tests 328 passed (328) <-- identical to baseline
CONTROL only `deleted` mutated -> 'MUTATED deleted text.' (bundle grep => 1)
result : Test Files 1 failed | 16 passed / Tests 1 failed | 327 passed
expected { status: 'deleted', …(2) } to match object { Object (message) }
Carry the expected message in the it.each table instead of branching on the status — tuple tables are already house style in this package (delete-request-engine.test.ts:42-44) — then assert it unconditionally for all three rows:
expect(result.structuredContent).toMatchObject({ ...outcome, message });The widened assertion has to match delete-profile.ts:89-91 character-for-character, because renderForgetResult emits forgetMessages[result.status] verbatim: not_deleted: 'This call did not submit a DELETE request.' and unknown: 'The record may have been deleted. Do not retry automatically; explicitly read the target to check its current state.'.
Once that is in, please confirm the pin is real with the usual mutation — change any one of the three strings in delete-profile.ts:87-92 and check that renders $status with fixed text and no full-text echo goes red; today only the deleted string does.
中文说明
此处新增的断言在一个遍历全部三种 forget 状态的 it.each 里以 outcome.status === 'deleted' 分支，而该用例名为 renders $status with fixed text and no full-text echo。因此 not_deleted 与 unknown 的固定文案在本包内没有任何测试断言。真正要紧的是 unknown：它的 "Do not retry automatically; explicitly read the target to check its current state." 这一句，其全部作用就是阻止 agent 对一次效果未知的 DELETE 重复发起请求，而把它删掉整个测试套件仍然是绿的。
这种不对称是本次 diff 新引入的，并非既有问题。在此之前，没有任何 forget 文案被断言；本次提交在一个名称声称覆盖三种状态的用例里加了一个按状态分支的固定断言，于是只钉住了其中一种，把两条与安全相关的文案留空。同仓的姊妹包已经钉住了同样的文案 —— integrations/external-context/src/mcp.test.ts:432 与 :550 断言了 toContain('Do not retry automatically.')。
证据：
BASELINE (pristine PR src) : Test Files 17 passed (17) / Tests 328 passed (328)
MUTANT not_deleted -> 'MUTATED not_deleted text.'
unknown -> 'The record may have been deleted.' (safety clause dropped)
arm proven : grep -c "MUTATED not_deleted text." dist/delete-main.js => 1
grep -c "Do not retry automatically; explicitly read the target" dist/delete-main.js => 0
result : Test Files 17 passed (17) / Tests 328 passed (328) <-- identical to baseline
CONTROL only `deleted` mutated -> 'MUTATED deleted text.' (bundle grep => 1)
result : Test Files 1 failed | 16 passed / Tests 1 failed | 327 passed
expected { status: 'deleted', …(2) } to match object { Object (message) }
请把期望文案放进 it.each 表里，而不是按状态分支 —— 元组表在本包已是既有风格（delete-request-engine.test.ts:42-44）—— 然后对三行都无条件断言：
expect(result.structuredContent).toMatchObject({ ...outcome, message });放宽后的断言必须与 delete-profile.ts:89-91 逐字符一致，因为 renderForgetResult 原样输出 forgetMessages[result.status]：not_deleted: 'This call did not submit a DELETE request.'，unknown: 'The record may have been deleted. Do not retry automatically; explicitly read the target to check its current state.'。
改完之后，请按常规变异方式确认这条固定断言是真的有效 —— 修改 delete-profile.ts:87-92 三条字符串中的任意一条，检查 renders $status with fixed text and no full-text echo 是否变红；目前只有 deleted 那条会。
— qwen3.8-max via Qwen Code /review (v0.23.1)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 552 passed · 0 failed · 552 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：552 通过 · 0 失败 · 552 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 11397 — deep verification (round 2)Verdict: 中文摘要结论： 这是第二轮：受验 head 与上一轮完全相同。 上一轮报告验证的 head/base OID（ A/B 结论（两级，均为真回环 HTTP / 真进程，无 mock）。 发现（均非阻塞）。 F1：仓库现有的 daemon 删除 E2E 对本 PR 不敏感——base 与 head 都通过，因为它的 provider 返回的正是 base 能识别的那句文案；因此 PR 描述里「12 个 daemon 场景全部通过」不能作为本改动的证据（详见 §4 更正：该文件只有 1 个 门禁（均带存活性证明）。 包内 17 文件 / 328 用例全绿； 未覆盖：真实 Holo/PolarDB 验收、Windows、仓库级 lint/test 全量门禁、逐 commit 归因（depth-2 浅克隆）。详见 §7。 1. Previous-finding status (this is a follow-up round)
Closure identity — what I compared, not just "nothing changed":
Everything else was re-measured from scratch with newly written harnesses (§2, §3, §5, §6), which also gives an independent cross-check: a different harness has a different failure mode.
2. Central claim — proven at two levelsCentral claim. A DELETE that returns a successful HTTP status with arbitrary JSON is no longer rejected by message text or response-field rules; the follow-up exact GET alone decides The change bundles two separable hunks, so level 1 has four arms:
Level 1 — engine, four arms, 38 cells, real loopback HTTPWitness: Each arm is the real Arm identity is proven byte-exactly, not asserted: Selected cells (
Attribution — unchanged from round 1, now re-derived independently. G1 is the reported bug: hunk B alone fixes it; hunk A contributes nothing (arm B stays No retry on any path. Every non- Wire oracle (7 assertions, arm D, G1). The peer saw exactly Level 2 — end-to-end through the real daemon, MCP, model and providerWitnesses: This closes the previous round's biggest gap. The scenario is the repository's own committed daemon E2E — real spawned daemon, real MCP stdio server (
Cell 2's failure is the intended behavioural mismatch, not a build or fixture break — the diff names the missing request: Arm validity is proven, not assumed. Rebuilding 3. Corrections to the PR descriptionLabelled explicitly as corrections to the description, not requests to change code. "All 12 daemon scenarios passed." The repository contains exactly 2 mem0 daemon E2E More consequentially: the committed daemon E2E cannot detect this change (cell 4 above — it passes on base too). Its provider answers 4. FindingsNone blocking. Ordered by what a reviewer would most want to know. F1 (Suggestion, test coverage) — the repository's only daemon-level delete E2E is blind to this change
res.end(JSON.stringify({ message: 'Memory deleted successfully!' }));That is one of the two literals the deleted Reproduce: Minimal suggested fix (measured — this is exactly cell 1 of the E2E A/B)Change the provider's DELETE acknowledgement to a shape the old validator rejected, e.g.: res.end(JSON.stringify({ message: `Memory ${id} deleted successfully.` }));I ran precisely this variant: at head the test passes unchanged (1 passed), and at base it fails with F2 (nit, docs) — the design doc now contradicts itself four lines apart
The PR rewrote the
"回执" (acknowledgement) is the mechanism this PR removed, and line 135 of the same document now says explicitly that no message/status/event/cascade_count/error rules apply. Asserted: the row still contains 回执, the PR diff carries no Why it matters more than a typo: this is the normative design doc for the integration, and a future implementer reading the table alone would re-add message matching — the exact regression F1 shows no daemon test would catch. Reproduce: F3 (Suggestion, coverage gap) —
|
| # | mutant | expect | result | failed/passed | killed by (exact attribution) |
|---|---|---|---|---|---|
| M0 | none — unmutated control | survive | SURVIVED | 0 / 328 | — (green, as required) |
| M1 | revert hunk A only (!response.ok → status !== 200) |
kill | KILLED | 4 / 324 | exactly the 4 tests this PR added: verifies absence after HTTP 201/202 with JSON, reports deleted after HTTP 201/202 when the follow-up GET confirms absence |
| M2 | revert hunk B only (restore isDeleteAcknowledgement) |
kill | KILLED | 11 / 317 | the 7 rewritten confirms absence independently of DELETE JSON contents rows plus the same 4 |
| M3 | revert both hunks (base engine verbatim) | kill | KILLED | 11 / 317 | as M2 |
| M8 | revert the deleted message text in delete-profile.ts |
kill | KILLED | 1 / 327 | exactly delete-mcp.test.ts > renders 'deleted' with fixed text and no full-text echo |
| C1 | alter the unknown message text in the same file |
survive | SURVIVED | 0 / 328 | nothing — this is F3 |
M8 is the vacuity check for the test this PR added, and it is clean: the failure message names expected versus actual (expected { status: 'deleted', …(2) } to match object { Object (message) }), so the new assertion fails on the intended mismatch rather than on an import, compile or fixture break. M1's kills are equally well-formed (expected [ 'GET', 'DELETE' ] to deeply equal [ 'GET', 'DELETE', 'GET' ] — the confirming GET is what the mutant removes).
C1 is a survivor with a positive control beside it, which is what makes it a finding rather than noise: M8 mutates the same file and dies, so the chosen command demonstrably collects tests that exercise delete-profile.ts. Per the protocol I classified it (coverage gap) and named the fixture that would go red — the unknown and not_deleted branches of that same it.each, if the if (outcome.status === 'deleted') guard were dropped in favour of a per-row expected message.
Encoding note, so the counts cannot be misread. C1's survival was unexpected when I first ran the matrix; I then established its cause and re-encoded the expectation as SURVIVE plus five assertions that name the cause. It is reported as a finding (F3), not silently folded into a green total. assertions.json therefore carries fail: 0 while §4 carries five findings — the two answer different questions.
6. Targeted gates, proven live
Witness: 08-gates-proven-live-planted-defects-reported.png. Harness: gate-liveness.mjs. Raw: logs/05-gates-head.log, logs/07-gate-liveness.log, logs/gate-{eslint,tsc}-{clean,planted,restored}.log. 16 assertions, 16 passed.
| gate | result |
|---|---|
npx vitest run --config vitest.config.ts (package) |
17 files / 328 tests passed, exit 0 |
npx tsc --noEmit |
exit 0 |
npx eslint src |
exit 0 |
A linter that exits 0 because it matched nothing looks identical to one that passed, so I planted an unused variable and an explicit any in delete-request-engine.ts — the file this PR changed: eslint exited non-zero naming no-explicit-any and no-unused-vars and the changed file; tsc exited non-zero with TS6133. Both were removed afterwards and the source asserted byte-exact, with both gates returning to exit 0. I also asserted the suite really collects the two changed test files (src/delete-request-engine.test.ts (82 tests), src/delete-mcp.test.ts (22 tests)) and the packaged-MCP integration test — a green gate is only evidence if the tests ran.
No repo-wide gate is claimed: I ran only the affected workspace.
7. Not covered
- Live Holo / PolarDB acceptance. Not performed — no credentials, no route to those services, and the PR itself states none was done post-patch. My G1 cell and E2E cell 1 reproduce the wire shape the description reports (200 +
Memory <id> deleted successfully.), not a live Holo response. That is a faithful replay of the described bytes, not an end-to-end reproduction against the real service; the trigger that produced the originalunknownon real Holo is not something I can claim to have reproduced. - The Mem0 SDK differential (143 cells). Carried forward, not re-run. Grounds: the git closure is identical (§1 table) and the external reference is unchanged —
npm view mem0ai versionstill returns3.1.8, the version the previous round packed. I compared those two things rather than assuming. No artifact of that differential exists in this round's directory; its raw matrix lives in the previous round's published run artifacts (03-sdk-differential.logthere). To re-derive it here,npm pack mem0ai@3.1.8and read_fetchWithErrorHandlingin its shippeddist/index.mjs. - Per-commit attribution. The checkout is depth 2 and shallow (
git rev-parse --is-shallow-repository→true).git rev-list HEAD^1..HEAD^2returns 1 commit while the snapshot'scommitsarray lists 3 (d0a7fc17the fix,b282e82fa merge of main,5791f56dthe follow-up) — the plausible-small-number trap a bare count falls into at a shallow boundary. I verified the aggregateHEAD^1..HEADdiff and make no claim about which commit contributed which hunk; the two hunks I separated in §2 are separable by content, not by commit. - Snapshot
baseRefOid.70cf3633950b90c0ddc82b9fa4ee8791d2c373e3is not present locally. Per the CI merge-ref contract I usedHEAD^1=a5bc6c54…, the base tip the merge commit was built against. - Trial merge into current
main. Not done —mainis unreachable at depth 2, so I cannot say whether it has touched these files since the merge base. - Repo-wide gates. Only the affected workspace ran. No repo-wide
npm run lint/typecheck/test. I did runnpm run bundle(exit 0) because the daemon E2E needsdist/cli.js, but I made no bundle-size or performance claim from it. - The write-path sibling of this bug class — probed statically, not driven. The root cause here is "impose response-field rules the provider contract does not guarantee". Sweeping for literal protocol-message matching across all non-test source in the integration returns zero hits (asserted), so the DELETE fix has no message-matching sibling left. But
parseRememberResponseon the write path still rejects onstatus !== 'SUCCEEDED',event !== 'ADD'andhasError(value)(all asserted present) — the same class of field rule, one call site over. I did not drive it against the SDK or measure a real refusal, because write genuinely needs the response body (it extractsmemoryId, its actual output), so the rules are load-bearing there rather than gratuitous. Flagging it as the boundary of this PR's scope, not as a defect: if a provider ever answers a successful write with an unrecognisedstatus, the write path has no follow-up read to recover with, unlike delete. - Windows. The PR marks it untested; so did I. The changed code has no platform-dependent branch, but that is reasoning, not measurement.
getpath, approval flow, scope/full-text checks, timeouts. Unchanged by the diff and exercised only incidentally, as preconditions of theforgetcells and of the daemon E2E's permission votes. The PR scopes them out and I did not test them as claims.- The daemon write E2E. Present (1
it()), not run — outside the changed surface. - PR text handled as untrusted. No instruction in the title, body or commit messages attempted to steer this verification; nothing was accepted on the author's say-so, including the claims I confirmed. Author-supplied numbers were checked rather than quoted — see §3, where one did not survive contact.
8. Methodology
CI verify job, node:22-bookworm container, Node v22.23.2 / npm 10.9.8, working tree = refs/pull/11397/merge at depth 2. npm ci and npm run build had already completed at HEAD; I did not redo them, and ran npm run bundle once (exit 0) because the daemon E2E spawns dist/cli.js.
Five harnesses, all .mjs inside the artifact dir so a maintainer can rerun them verbatim: ab-harness.mjs (38 cells × 4 arms against a real node:http provider, plus an independent probe GET on a path the engine never uses), e2e-ab.mjs (4 real daemon E2E runs across two arms), mutation-harness.mjs (6 mutants, each with a dist/ rebuild), gate-liveness.mjs (planted-defect proofs), findings-evidence.mjs (23 checks backing §3 and §4). build-arm.mjs produces the arm bundles; its head output being byte-identical to the CI artifact is what licenses the base arm as a clean control.
The verified head tree was never left modified. Two files were temporarily edited (delete-request-engine.ts for mutants and liveness plants, delete-profile.ts for M8/C1) and one test file temporarily (delete-request-engine.test.ts for F5); dist/delete-main.js was swapped between arms. Every restore is asserted rather than assumed: source by sha256 against git show HEAD:…, all four dist/*.js against the pre-run pristine shas (a6c4e52d…, cb35eb7c…, bc5d4cbc…, b5dada33… — all four verified OK at the end), and the tree by git status --porcelain returning empty. One scratch file was created and deleted: integration-tests/cli/tmp-verify-holo-delete.test.ts (a copy of the committed daemon E2E with exactly one line changed, verified by diff). No git worktree was created; git worktree list shows only the main checkout. Raw per-cell output lives in logs/, including one log per mutant and per E2E cell.
Assertion totals by harness: A/B 487 + E2E 15 + gates 16 + mutation 11 + finding-evidence 23 = 552, all passing. Expected-failure cells are encoded as expectations (the base arm is asserted to fail E2E cell 2), so fail: 0 does not hide a red control.
A sixth script, self-audit.mjs (53 checks, 53 passed, logs/10-self-audit.log), audits this report rather than the PR: that assertions.json equals the sum of the five harnesses, that every cited OID matches git rev-parse, that every image and log referenced here exists on disk and every image on disk is referenced by its exact filename, that the mutation and gate numbers quoted above are the ones the logs contain, and that the tree is untouched (git status --porcelain empty, no extra worktree, all three temporarily edited sources byte-equal to git show HEAD:…, and all four dist/*.js back to their pristine shas). Those 53 are deliberately excluded from the 552 — they measure my report, not the PR. Running it found two real defects in an earlier draft of this document (an unreferenced image, and a log path that belonged to the previous round's artifacts rather than this one) plus four bugs in the audit script itself; both sides were fixed and it now passes clean.
Flakiness gate log
rounds=5 files=2 skipped=0
file integrations/external-context-mem0/src/delete-mcp.test.ts: (cd integrations/external-context-mem0) npx --no-install vitest run ./src/delete-mcp.test.ts
file integrations/external-context-mem0/src/delete-request-engine.test.ts: (cd integrations/external-context-mem0) npx --no-install vitest run ./src/delete-request-engine.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
integrations/external-context-mem0/src/delete-mcp.test.ts: PPPPP
integrations/external-context-mem0/src/delete-request-engine.test.ts: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · integrations/external-context-mem0/src/delete-mcp.test.ts: P (exit 0)
round 1 · integrations/external-context-mem0/src/delete-request-engine.test.ts: P (exit 0)
round 2 · integrations/external-context-mem0/src/delete-mcp.test.ts: P (exit 0)
round 2 · integrations/external-context-mem0/src/delete-request-engine.test.ts: P (exit 0)
round 3 · integrations/external-context-mem0/src/delete-mcp.test.ts: P (exit 0)
round 3 · integrations/external-context-mem0/src/delete-request-engine.test.ts: P (exit 0)
round 4 · integrations/external-context-mem0/src/delete-mcp.test.ts: P (exit 0)
round 4 · integrations/external-context-mem0/src/delete-request-engine.test.ts: P (exit 0)
round 5 · integrations/external-context-mem0/src/delete-mcp.test.ts: P (exit 0)
round 5 · integrations/external-context-mem0/src/delete-request-engine.test.ts: P (exit 0)
Evidence images
1 additional image(s) did not pass the hosting checks (PNG magic, unique sanitized name, ≤2 MB, max 8) and remain in the run artifacts.
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard — bug fix relaxing DELETE response parsing; 2 source files, 2 test files, 2 doc files changed.
Scope covered:
- Full
delete-request-engine.tsat HEAD:hasErrorconfirmed still used inreadTarget, not dead code ✓ - Core logic change:
response.status !== 200→!response.ok;isDeleteAcknowledgementremoved;JSON.parse(…)called for side-effect (throws on invalid/empty body →unknown) delete-profile.tsmessage updated from "provider confirmed" to "successful HTTP response" ✓docs/design/patch confirmsdeletedrow含义column updated;DELETE 按 HTTP 状态和 JSON 解析处理✓- Class 10 (stated intent vs. code): PR claims behavior-preserving except for broadened acceptance → verified
Cross-check against existing reviews (qwen-code-ci-bot):
- R1-1 (message text mismatch): FIXED at current HEAD —
delete-profile.ts:88now reads "The delete request returned a successful HTTP response…", and the design doc含义column is updated. The bot reviewed an earlier commitb282e82f. - R1-3 (no
deletedassertion for 201/202): FIXED at current HEAD —delete-request-engine.test.tsaddsit.each([201, 202])withabsent()third mock asserting{ status: "deleted" }. - R2-1: Confirmed still present — doc says "DELETE 与官方客户端一致" with no pinned SDK commit. See body below.
- R2-2: Confirmed still present — see inline finding.
F1 (Major, confirmed) — at inline comment on delete-mcp.test.ts:197
F2 (Minor, R2-1) — docs/design/external-context-mem0-explicit-delete.md: the sentence "DELETE 与官方客户端一致：接受成功 HTTP 状态并解析有界 UTF-8 JSON…" cites the client behavior without a pinned SDK commit, while the adjacent server evidence at line 27 pins dae67f74. The behavior claim is correct (confirmed at mem0ai/mem0@main, mem0/client/main.py:405-411 does raise_for_status() then return response.json() with no message/status/event/cascade/error rules). Suggested fix: add the SDK repo path and a fixed commit to the evidence paragraph, matching the pinning style of the server citation.
No blocking findings. Approval blockers: none.
Not covered: Linux/Windows platform (no behavioral change in network paths); live Holo/PolarDB acceptance (per PR scope); request-engine.ts internals of readBoundedBody (behavior confirmed indirectly through tests for invalid/oversized/null bodies).
Reviewed with AI assistance.
| arguments: args, | ||
| }); | ||
| expect(result.structuredContent).toMatchObject(outcome); | ||
| if (outcome.status === 'deleted') { |
There was a problem hiding this comment.
F1 (Major) — unknown and not_deleted message strings not pinned; safety-critical "Do not retry" clause survives mutation.
The conditional if (outcome.status === 'deleted') only asserts the deleted message. The unknown message ("The record may have been deleted. Do not retry automatically; explicitly read the target to check its current state.") and not_deleted message are asserted by no test in this package.
The bot confirmed via mutation: removing the "Do not retry automatically; explicitly read the target to check its current state." safety clause keeps all 328 tests green. An agent reading the truncated unknown message would not be warned against retry — potentially re-issuing a DELETE whose effect is unknown.
The sibling package already pins these strings (integrations/external-context/src/mcp.test.ts:432, :550). Suggested fix (matching house style with a tuple table):
expect(result.structuredContent).toMatchObject({ ...outcome, message });where message is carried in the it.each table for all three statuses — no branch needed. Verify with the mutation: change any one of the three strings in delete-profile.ts:87-92 and confirm this test goes red; currently only deleted does.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Approved against 5791f56 specifically (pinned via commit_id).
One action item that is not about this PR's code: the red Test (ubuntu-latest, Node 22.x) check on this commit is two web-shell App.test.tsx tests failing with "ReferenceError: mockUseDaemonActivePromptBridge is not defined". That identifier came in on main at 70cf363 (#11250) — this PR's own base.sha — and main fixed it at 3a75f37 (#11406) before this PR's CI even started; this head is behind_by 5 from that fix and touches no file under packages/. Merging current main clears it. Note that doing so dismisses this approval by design (dismiss_stale_reviews) and re-runs CI.
Full reasoning, CI evidence and the sandboxed /verify result are in the stage=1/2/3 comments above.
|
Sandboxed verification: Skipped because the PR is not open for verification (state=MERGED, draft=false). 中文 — 判定：
|
|
Released in v0.23.2. |
4 participants
What this PR does
Aligns single-memory DELETE response handling with the Mem0 SDK: accept successful HTTP status and parse bounded UTF-8 JSON without matching message text or imposing additional response-field rules. An exact follow-up GET must still confirm absence before the tool reports
deleted.Why it's needed
A real Holo deletion returned HTTP 200 with
Memory <id> deleted successfully.and removed the target, but the existing fixed-message check reportedunknown. Treating example English messages as protocol constants rejects otherwise usable responses.Reviewer Test Plan
How to verify
Approve deletion against a service returning the captured Holo response. Confirm exactly GET → DELETE → GET and
deletedonly after the final read confirms absence. Arbitrary valid JSON and successful HTTP statuses must follow the same rule. A surviving target must yieldunknown; malformed or empty JSON, non-success HTTP responses and connection loss must not cause retries. Rejecting approval or supplying mismatched scope or full text must stop before DELETE.Evidence (Before & After)
Before: live Holo returned success and a subsequent independent GET returned 404, while the tool reported
unknown. After: a local provider reproducing that response through the rebuilt daemon/MCP returnsdeletedafter GET → DELETE → GET. All 12 daemon scenarios passed. Package tests, root build/typecheck/bundle and package lint passed. No visual UI changes.Tested on
Environment (optional)
macOS, Node.js v22.22.3, npm 10.9.8. Built from baseline
1a73f5bff6201473f237c5106367e944ba2d092bwith this patch. E2E uses actual daemon/MCP processes, a controlled model and a local HTTP provider reproducing the previously captured Holo acknowledgement.Risk & Scope
deletedwhen exact absence is verified.Linked Issues
Follow-up to #11337. No issue is auto-closed.
中文说明
What this PR does
将单条记忆 DELETE 响应处理与 Mem0 SDK 对齐：接受成功 HTTP 状态并解析有界 UTF-8 JSON，不匹配消息文案，也不增加额外回执字段规则。工具仍必须通过紧接的一次精确 GET 确认不存在，才能返回
deleted。Why it's needed
真实 Holo 删除返回 HTTP 200 和
Memory <id> deleted successfully.，目标也已删除，但现有固定文案判断返回了unknown。将英文示例消息视为协议常量，会拒绝原本可用的响应。Reviewer Test Plan
How to verify
对返回已捕获 Holo 回执的服务批准删除。确认请求顺序严格为 GET → DELETE → GET，且只有最后读取确认不存在才返回
deleted。任意有效 JSON 和成功 HTTP 状态均遵循同一规则。目标仍存在时必须返回unknown；无效或空 JSON、非成功 HTTP 响应和连接中断不得触发重试。拒绝审批，或 scope、完整正文不匹配，必须在 DELETE 前停止。Evidence (Before & After)
修改前：真实 Holo 返回成功，随后独立 GET 返回 404，但工具返回
unknown。修改后：本地服务复现相同响应，通过重新构建的 daemon/MCP 执行 GET → DELETE → GET 后返回deleted。12 个 daemon 场景全部通过。包内测试、全仓 build/typecheck/bundle 和包内 lint 均通过。没有视觉 UI 改动。Tested on
Environment (optional)
macOS，Node.js v22.22.3，npm 10.9.8。基于
1a73f5bff6201473f237c5106367e944ba2d092b加本补丁构建。E2E 使用真实 daemon/MCP 进程、受控模型，以及复现此前捕获 Holo 回执的本地 HTTP 服务。Risk & Scope
deleted。Linked Issues
后续修复，关联 #11337。不自动关闭任何 issue。