fix(memory): scan uncapped when selecting forget candidates - #9530
Conversation
Recall moved to the uncapped scanner in QwenLM#8716; forget did not. A document ranked past the 200-document cap could be recalled and injected into the prompt but never forgotten. Forget now scans uncapped, so its candidate universe matches recall's. The model-selection prompt renders every candidate, so it gets its own bound of 400: literal query matches first, then the most recently modified remainder. The heuristic fallback keeps scanning the full uncapped list. Indexer, status, and extraction stay capped on purpose, and the two design docs that recorded forget as capped now say otherwise. Refs: QwenLM#9378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Re-run on
Moving on to code review. 🔍 中文说明在
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-run on My independent baseline before reading the diff: switch forget to the same uncapped scanners recall uses; give the model-selection prompt its own bound since it renders every candidate inline — literal query matches first, and both scopes kept represented so a uniformly-newer scope cannot starve the other; give the unconfirmed deletion path its own ceiling, since it substring-matches the whole store with no confirmation; leave indexer/status/extraction capped. The PR matches that proposal. Verified against
No blockers. Non-blocking notes: the Suggestion-level findings accumulated across the review rounds (duplicated test factories/fixtures, Test evidence — the PR's own CI (this review never runs PR code)CI results for
Everything green on Not verified by CI alone, stated plainly: the >400-candidate model path is exercised in-suite only through a mocked Also noted, attributed as the maintainer's own evidence (not independently re-run here): @wenshao posted a local A/B verification on 中文说明代码审查在 读 diff 前我的独立方案:把 forget 切到与 recall 相同的无上限扫描器;为模型选择 prompt 单独设上限(每个候选都会内联渲染)——字面匹配优先、两个 scope 都保留席位,避免整体更新的 scope 挤掉另一个;为未确认删除路径单独设上限(它会对整个存储做子串匹配且无确认);indexer/status/extraction 维持上限。PR 与该方案一致。 已在审查 worktree 中对照
无阻塞问题。非阻塞备注:review 各轮积累的 Suggestion 级发现(测试工厂/夹具重复、 测试证据——来自 PR 自身的 CI(本评审不运行 PR 代码)
CI 本身未能覆盖、如实说明:超过 400 候选的模型路径在套件内只通过 mock 的 另注明,以下为维护者本人的证据(本评审未独立复跑):@wenshao 在 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — fresh full pass on the final diff: a confirmed regression fixed with a minimal, well-bounded change and a negative-control integration test; only nits remain (Chinese The re-run on The deciding evidence is the negative-control integration test: a real 201-document tree, target ranked 201st by mtime, capped-scan → recall → forget-selection → delete driven on real files — and it fails cleanly against the base code. CI is fully green on this exact commit, including the previously red Dependency CVE audit (repo-wide at the time, unrelated to this diff). Maintainer @wenshao has already approved and posted an independent local A/B verification (71/71 scripted assertions); my fresh pass agrees with that read. Not verified, stated plainly: the bound's behavior at real scale on the model path is mocked in-suite (acknowledged in the PR's own Risk & Scope), and Windows/macOS coverage rests on the merge queue per One thread-hygiene note: the automated round-1 review left a Approving, pinned to the reviewed commit. ✅ 中文说明置信度:4/5 —— 对最终 diff 做了完整的新评审:以最小、边界清晰的改动修复了已确认的回归,并配有阴性对照集成测试;只剩小瑕疵(PR 正文仍缺中文 在 决定性证据是阴性对照集成测试:真实 201 文档树、目标按 mtime 排第 201 位、在真实文件上驱动 带上限扫描 → recall → forget 选择 → 删除 的完整路径——并且在 base 代码上干净地失败。CI 在此 commit 上全绿,包括之前变红的 Dependency CVE audit(当时是仓库范围问题,与本 diff 无关)。维护者 @wenshao 已批准并发布了独立的本地 A/B 验证(71/71 脚本化断言);我的全新评审与该判断一致。 未验证部分,如实说明:模型路径在真实规模下的上限行为在套件内是 mock 的(PR 自身的 Risk & Scope 已承认);Windows/macOS 覆盖按 一条线程卫生说明:第 1 轮自动 review 留下了一条 8 月 20 日的 批准,钉在所评审的 commit 上。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): 575 tests pass — this review observed 20640, 1590, 22030, 1612, 494, 3799, 542 passed; Tests 575 passed — this review observed 20640, 1590, 22030, 1612, 494, 3799, 542 passed; Tests 20697 passed — this review observed 20640, 1590, 22030, 1612, 494, 3799, 542 passed; 2 passed — this review observed 20640, 1590, 22030, 1612, 494, 3799, 542 passed; 20694 passed — this review observed 20640, 1590, 22030, 1612, 494, 3799, 542 passed.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| rest.sort((a, b) => b.mtimeMs - a.mtimeMs); | ||
| return [...matched, ...rest].slice(0, MAX_MODEL_FORGET_CANDIDATES); |
There was a problem hiding this comment.
[Critical] The new global 400-candidate budget drops the per-scope visibility guarantee the removed capped scans provided. The old scanners handed the model up to 200 newest documents per scope (200 project + 200 user); selectModelForgetCandidates now ranks both scopes into one mtime-ordered 400 budget, so a scope whose entries are all older than the other scope's 400th-newest gets zero representation in the prompt. Combined with the fallback firing only on model error — a successful-but-empty selection returns strategy: 'none' directly — such entries become unselectable and therefore unforgettable, while uncapped recall can still inject them.
Concrete shape: a store with 400+ project entries all newer than the user-level entries, and a semantic forget request targeting an old user entry. An A/B probe against the merge base shows the regression:
BASE: {"totalIdsInPrompt":203,"userIdsInPrompt":3,"targetInPrompt":true}
PR: {"totalIdsInPrompt":400,"userIdsInPrompt":0,"projectIdsInPrompt":400,"targetInPrompt":false}
The Risk & Scope note that a semantic miss on >400-entry trees is "no worse than today" does not hold for this scope-skewed shape — the old code guaranteed each scope seats in the prompt regardless of cross-scope recency.
Two possible fixes: re-establish per-scope representation (e.g. split MAX_MODEL_FORGET_CANDIDATES into per-scope quotas, 200/200 with unused quota redistributed, before the recency fill), or fall through to selectByHeuristic(candidates, query, limit) over the full uncapped list when the model returns zero selections.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| } | ||
| } | ||
| rest.sort((a, b) => b.mtimeMs - a.mtimeMs); | ||
| return [...matched, ...rest].slice(0, MAX_MODEL_FORGET_CANDIDATES); |
There was a problem hiding this comment.
[Suggestion] When more than 400 entries literally match the query, this slice truncates matched itself — in scan order (user scope first, then project), since matched is never recency-sorted unlike rest — and nothing logs the truncation. That contradicts the docstring promise "without dropping anything the heuristic fallback would have matched": the heuristic uses the identical predicate over the full list, and because the model path succeeded the fallback never runs, so the dropped literal matches survive deletion while recall can still inject them.
Probe with 450 literal matches and a successful model call dropped project entries at scan positions 200–249 even though their mtimes (201–250) were newer than every kept project entry (max 200): {"promptIdCount":400,"droppedProjectCount":50,"strategy":"model"}.
Sort matched by mtimeMs descending as well so truncation is deterministic, emit a debugLogger line when the bound actually drops candidates, and soften the docstring to say matches beyond the budget are dropped.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| * would have matched: literal query matches go in first, then the most | ||
| * recently modified remainder fills the budget. A semantic-only match ranked | ||
| * past the budget can still be missed by the model path; the heuristic | ||
| * fallback below keeps scanning the full uncapped list. |
There was a problem hiding this comment.
[Suggestion] This comment presents the heuristic fallback as the mitigation for matches ranked past the budget, but in production the fallback fires only when the side query throws — a successful model selection returning zero ids short-circuits with strategy: 'none' and never consults the uncapped list, which is exactly the case the comment names. (And even when the fallback does fire, literal-substring matching cannot catch a semantic-only match.) The behaviour itself is disclosed in the PR's Risk & Scope, so this is about the comment saying what the code does not do.
| * fallback below keeps scanning the full uncapped list. | |
| * fallback below rescues only the model-failure path; a successful-but-empty | |
| * selection returns 'none' without scanning the full list. |
— qwen3.8-max via Qwen Code /review (v0.21.14)
| Superseded for Forget: issue #9378 moved Forget to the uncapped scanner so a | ||
| recallable entry is always removable. Indexer, Status, and Extraction remain |
There was a problem hiding this comment.
[Suggestion] This records an unconditional guarantee ("a recallable entry is always removable") that the code does not provide: the A/B probe on the sibling thread shows a recallable entry that is not removable on a scope-skewed store, and >400 literal matches can also fall off the prompt. Issue (1) in the linked thread asked for the asymmetry to be closed or explicitly documented and bounded — this documents an unconditional closure instead of the bounded one (the bounding only lives in the transient PR description).
| Superseded for Forget: issue #9378 moved Forget to the uncapped scanner so a | |
| recallable entry is always removable. Indexer, Status, and Extraction remain | |
| Superseded for Forget: issue #9378 moved Forget to the uncapped scanner so | |
| literal matches always reach the model first within the 400-candidate prompt | |
| bound; semantic-only matches ranked past the bound can still be missed. | |
| Indexer, Status, and Extraction remain |
(The following capped. line completes the sentence.)
— qwen3.8-max via Qwen Code /review (v0.21.14)
| if (candidates.length <= MAX_MODEL_FORGET_CANDIDATES) { | ||
| return candidates; | ||
| } | ||
| const queryLower = query.replace(/\s+/g, ' ').trim().toLowerCase(); |
There was a problem hiding this comment.
[Suggestion] This normalisation and the literal-match predicate below duplicate the same operations already in selectByHeuristic (same file). The docstring invariant — the bound drops nothing the heuristic would have matched — depends on the two copies staying in sync, and no test pins the equivalence: if the heuristic predicate ever changes (fold rules, an extra matched field), the bound copy silently keeps dropping heuristic-matchable candidates from the model prompt, visible only in stores with >400 candidates where the bound engages. A shared helper used by both call sites would make the invariant structural.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| scanAllAutoMemoryTopicDocuments(projectRoot), | ||
| scanAllUserAutoMemoryTopicDocuments(), |
There was a problem hiding this comment.
[Suggestion] Forget is now the only member of this scan family without the best-effort guard its siblings carry: recall.ts wraps scanAllUserAutoMemoryTopicDocuments().catch(...) to [] with a warning, and extractionAgentPlanner.ts does the same for its user scan. scan.ts swallows only ENOENT, so if ~/.qwen/memories exists but is unreadable (EACCES/ELOOP), this Promise.all rejects and /forget fails outright even for a project-level memory, while recall and extraction degrade to project-only. The missing guard predates this diff, but these are the lines being modified, and the new comment aligns forget with the family whose pattern includes the guard.
| scanAllAutoMemoryTopicDocuments(projectRoot), | |
| scanAllUserAutoMemoryTopicDocuments(), | |
| scanAllAutoMemoryTopicDocuments(projectRoot), | |
| scanAllUserAutoMemoryTopicDocuments().catch((error: unknown) => { | |
| debugLogger.warn( | |
| `User-level auto-memory scan failed; project-level forget continues: ${error}`, | |
| ); | |
| return []; | |
| }), |
— qwen3.8-max via Qwen Code /review (v0.21.14)
Review round 1. The 400-candidate bound ranked both scopes into one recency budget, so a store whose project entries are all newer than its user entries seated no user memory at all. The capped scanners this replaced ran per scope, so each scope always had seats. That made an old user entry unselectable by the model while recall could still inject it, which is the same asymmetry the PR set out to close. Each scope now keeps a 200-candidate quota and whatever a smaller scope leaves is handed to the other. Within a scope, literal query matches rank first and both groups are ordered newest first, so truncation is deterministic instead of scan-order, and the bound logs when it drops candidates. Also from review: the query normalisation and match predicate are now shared with selectByHeuristic so the two cannot drift; the user scan gets the best-effort guard recall.ts and extractionAgentPlanner.ts already carry; and the docstring and design docs no longer claim an unconditional guarantee the bound does not provide. Three tests, each verified against the mutation it is meant to catch: global ranking drops the user ids, an ascending sort drops the newest filler, and handing the fallback the bounded list returns 400 of 450 matches. Refs: QwenLM#9378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): 575 tests pass — this review observed 20642, 1590, 22027, 1612, 494, 3799, 542 passed; Tests 575 passed — this review observed 20642, 1590, 22027, 1612, 494, 3799, 542 passed; Tests 20697 passed — this review observed 20642, 1590, 22027, 1612, 494, 3799, 542 passed; 2 passed — this review observed 20642, 1590, 22027, 1612, 494, 3799, 542 passed; 20694 passed — this review observed 20642, 1590, 22027, 1612, 494, 3799, 542 passed.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // outright. `scan.ts` swallows only ENOENT. | ||
| scanAllUserAutoMemoryTopicDocuments().catch((error: unknown) => { |
There was a problem hiding this comment.
[Suggestion] This catch turns a user-scope scan failure into a silent skip of the entire user scope while /forget (CLI and ACP) still reports a complete success/no-match result. Pre-PR the same failure surfaced as a loud error: the rejection propagated out of selectManagedAutoMemoryForgetCandidates, which forgetCommand.ts renders as Failed to process /forget: … and ACP as forget_failed. Now the only record is a debugLogger.warn gated behind QWEN_DEBUG_LOG_FILE — invisible by default — and neither result type carries a degraded-scan field. The comment's rationale argues "don't fail outright"; nothing argues for "report as complete". The direction matters: recall's equivalent catch costs a missed injection, but forget's leaves requested deletions unperformed while the user is told the operation finished.
Concrete shape: ~/.qwen/memories root-owned after a sudo run on a shared Linux box (EACCES on readdir). A user asks to forget a user-scoped entry; the scan rejects, the catch yields [], and /forget prints "No managed auto-memory entries matched" while the entry stays on disk.
witness (probe, chmod-000 user root under QWEN_CODE_MEMORY_BASE_DIR):
BASELINE user-query: {"strategy":"heuristic","matches":["…/user/secret-pref.md"]}
SELECTION user-only query: {"strategy":"none","matches":[]} // once unreadable
flip (catch reverted): rejected: code=EACCES: permission denied, scandir …
Suggested fix: surface the degradation in the result — e.g. add skippedScopes?: AutoMemoryStorageScope[] (with the error message) to AutoMemoryForgetSelectionResult/AutoMemoryForgetResult, set it in this catch, and have forgetCommand.ts and the ACP summary append "user-level memory was not searched: " when it is non-empty.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // Best-effort, as in recall.ts and extractionAgentPlanner.ts: an | ||
| // unreadable `~/.qwen/memories` must not make project-level forget fail | ||
| // outright. `scan.ts` swallows only ENOENT. | ||
| scanAllUserAutoMemoryTopicDocuments().catch((error: unknown) => { |
There was a problem hiding this comment.
[Suggestion] The new best-effort degradation path has no test — every other new behavior in this diff (prompt bound, per-scope quota, literal-first ranking, full-list heuristic fallback) got one. The existing mockRejectedValue tests in forget.test.ts only reject runSideQuery, never the scan.
If a future refactor drops this .catch() (e.g. re-aligning with the plain Promise.all pattern used before this PR), a permission error reading ~/.qwen/memories (EACCES — scan.ts swallows only ENOENT) would make /forget reject outright instead of continuing with project-level candidates, and nothing in the suite would catch the regression.
witness (probe): user scan rejecting + model failing still resolves with project-only
heuristic matches on PR code; removing the .catch() flips the probe to
`× … EACCES: permission denied` (1 failed) while no existing test changes state.
Add a unit test mocking scanAllUserAutoMemoryTopicDocuments to reject, asserting selectManagedAutoMemoryForgetCandidates still resolves with the project-scope matches.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // outright. `scan.ts` swallows only ENOENT. | ||
| scanAllUserAutoMemoryTopicDocuments().catch((error: unknown) => { | ||
| debugLogger.warn( | ||
| `User-level auto-memory scan failed; project-level forget continues: ${ |
There was a problem hiding this comment.
[Suggestion] This is the third hand-rolled copy of the same catch → warn → [] wrapper: recall.ts:413 is character-identical except the log word, and extractionAgentPlanner.ts:117 is the same shape over the capped scanner. The "user-memory unreadable → degrade to project-only, never fail" policy is owned by each caller instead of the producer, and it has already drifted: rebuildUserAutoMemoryIndex (indexer.ts:256) calls the user scanner with no catch, so the same unreadable ~/.qwen/memories hard-fails an index rebuild while recall/forget/extraction degrade gracefully. (forgetManagedAutoMemoryMatches wraps that rebuild in try/catch, so the failure bites other rebuild callers first — that narrows but does not remove the drift.)
If the policy ever changes (broader error class, log level, telemetry), every copy must be edited in lockstep; missing one makes forget or recall fail hard on an EACCES while the other flows degrade. The next consumer moving off the capped scanner copies this block a fourth time, or omits it and reintroduces the failure mode the comment above exists to prevent.
Give the policy one owner in scan.ts: either the user-scoped scanner swallows non-ENOENT read failures the way listMarkdownFiles already swallows ENOENT (with the warn log), or export a single scanAllUserAutoMemoryTopicDocumentsBestEffort() all callers use.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| const queryLower = normalizeForgetQuery(query); | ||
| const matches = candidates |
There was a problem hiding this comment.
[Suggestion] The heuristic fallback now runs over the full uncapped store, and on the programmatic path the limit is unbounded: workspaceMemoryForget (acpAgent.ts) validates only non-empty + byte size, and forgetManagedAutoMemoryEntries passes limit: Number.MAX_SAFE_INTEGER. When the model side-query fails (8-second timeout, rate limit, offline), this substring-matches every project AND cross-project user entry and forgetManagedAutoMemoryMatches deletes with no confirmation anywhere in the chain. The uncapped scan and full-list fallback are deliberate, but that rationale argues completeness, not an unbounded blast radius: pre-PR the capped scanners bounded this same failure at the newest 200 documents per scope. /forget is insulated by its default limit of 5; the manager/ACP path is not.
witness (probe, real scanner + real fs, only runSideQuery mocked to fail):
query='e' → removedEntries=5 survivors=0 touchedScopes=["user","project"]
with a minimum-length guard → removedEntries=0 survivors=5
Keep the full list for matching, but bound the deletion: cap heuristic matches in the forget pipeline, or return a "too many matches, refine the query" result above a threshold; a minimum query length on the unbounded API path would also close the single-character wipe.
— qwen3.8-max via Qwen Code /review (v0.21.14)
| A broader manageability pass is separate from this recall-only change. | ||
| Superseded for Forget: issue #9378 moved Forget to the uncapped scanner, with | ||
| a per-scope bound on the model-selection prompt so literal matches in each | ||
| scope reach the model first; semantic-only matches ranked past that bound can |
There was a problem hiding this comment.
[Suggestion] This sentence understates the bound: literal matches ranked past a scope's seat count (200 quota + up to 200 redistributed spare) are also missed, because a successful model selection never consults the full list. In the probe below, 200 of 300 literal matches were offered, the model succeeded, and runSideQueryCalls=1 proves the full list was never consulted — /forget reports success while ~100 literal matches survive, still injectable by recall's uncapped scan (the concrete cost when the query is scrubbing a leaked secret saved into many entries). The bounded-prompt tradeoff itself is defensible and documented; only this wording is inaccurate — the sibling doc says neutrally "Entries past that bound are not offered to the model".
witness (probe, 300 project docs containing the query + 200 user docs, spare 0):
strategy=model promptProjectIdLines=200 (of 300 literal matches)
deletedCodewordEntries=200 totalMatches=400 runSideQueryCalls=1
| scope reach the model first; semantic-only matches ranked past that bound can | |
| reach the model first; any match — literal or semantic — ranked past that bound can |
— qwen3.8-max via Qwen Code /review (v0.21.14)
| const take = ranked.map((scopeRanked) => | ||
| Math.min(scopeRanked.length, MAX_MODEL_FORGET_CANDIDATES_PER_SCOPE), |
There was a problem hiding this comment.
[Suggestion] The per-scope quota — the centerpiece guarantee of this PR — is never exercised with both scopes over quota, so its value survives mutation. Every new test has one scope empty (the 500-doc bound test, both heuristic tests) or far under quota (3 user docs), where spare redistribution fully compensates any quota value.
witness: mutating this take cap to 150 (MAX kept at 400) leaves all 15 tests green;
a 300-user + 300-project store seats [200, 200] under the original but [250, 150]
under the mutant — the 50 newest non-matching project docs silently lose the seats
the design doc and the constant's docstring promise them.
The suggested test fails against the mutant: expected a length of 200 but got 250.
Add a unit test with both scopes over 200 (e.g. 300 user + 300 project, no literal matches) asserting exactly 200 id: user: lines and 200 id: project: lines in the prompt.
— qwen3.8-max via Qwen Code /review (v0.21.14)
…an guard Review round 2, all suggestions. MemoryManager.forget passed limit: MAX_SAFE_INTEGER and deletes without confirmation. With an uncapped scan and a heuristic fallback that substring matches the whole store, a one-character query matched nearly every entry in both scopes, where the capped scanners had held that same failure to one scan's worth of candidates. The limit is now the prompt bound, restoring the old ceiling. Round 1 added a best-effort catch on the user scan. That was wrong on two counts: scan.ts caps after reading and ordering the whole tree, so uncapping adds no read exposure to justify it, and swallowing the failure made forget report "no entries matched" for a scope it never read, then act on that answer by deleting. Reverted, with a comment saying why forget differs from recall here: a missed injection is recoverable, a missed deletion is not. normalizeForgetQuery now delegates to normalizeSummary so query matching and the post-selection re-match cannot drift apart, and one design-doc sentence no longer implies only semantic matches fall off the bound. Two tests, each verified against its mutation: the quota split is now exercised with both scopes over quota, where dropping it to 150 seats 250 project entries instead of 200; and the delete ceiling fails at 401 removals if the unbounded limit comes back. Refs: QwenLM#9378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6c": none — no check was cut short..
Test Plan (not a blocker): 575 tests pass — this review observed 1590, 22027, 1612, 494, 3799, 542 passed; Tests 575 passed — this review observed 1590, 22027, 1612, 494, 3799, 542 passed; Tests 20697 passed — this review observed 1590, 22027, 1612, 494, 3799, 542 passed; 2 passed — this review observed 1590, 22027, 1612, 494, 3799, 542 passed; 20694 passed — this review observed 1590, 22027, 1612, 494, 3799, 542 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/memory/memoryLifecycle.integration.test.ts:356 — [review] new forget integration test duplicates ~45 lines of fixture from the adjacent recall test; copies already driftedpackages/core/src/memory/forget.ts:217 — [probe] recency ordering within the literal-matched set is unpinned; flipping the sort to oldest-first survives the whole suite (mutation probe)
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // short query matches nearly everything. The capped scanners this replaced | ||
| // held the same failure to one scan's worth of candidates; keep that | ||
| // ceiling rather than letting an uncapped scan widen it. | ||
| { ...options, limit: MAX_MODEL_FORGET_CANDIDATES }, |
There was a problem hiding this comment.
[Suggestion] The model path codifies a per-scope seat guarantee (selectModelForgetCandidates), but the heuristic fallback this limit is handed to — which fires on side-query failure/timeout over the full uncapped list — is a plain filter-then-slice in candidate order, and listIndexedForgetCandidates pushes every user entry before any project entry. With more than 400 matches, one scope can therefore take all 400 deletion seats and the other zero. Pre-PR this path could not truncate (capped 200-per-scope scanners plus a MAX_SAFE_INTEGER limit), so every visible match in both scopes was deleted.
Concrete trigger: a store with 450 user-scope entries and 50 project-scope entries all containing the query text; an ACP workspace-memory forget while runSideQuery is unavailable (outage or its 8 s timeout). The fallback deletes 400 user entries and zero project entries, recall keeps injecting the surviving project entries, and the response still reports 400 successful removals. Reproduced with a scratch-tree probe:
removedEntries=400 removedByScope={"user":400,"project":0} projectSurvivors=50/50
systemMessage: "Managed auto-memory forgot 400 entries"
Suggested fix: before slicing, run the matched set through the same per-scope quota logic selectModelForgetCandidates uses (200 seats per scope, spare redistributed). One refinement, verified in the probe's fixed arm: the redistribution budget must be the caller's limit, not min(limit, MAX_MODEL_FORGET_CANDIDATES) — the latter breaks the behaviour pinned by 'gives the heuristic fallback the full list, not the bounded one'.
— qwen3.8-max via Qwen Code /review (v0.21.14)
…e ceiling Review round 3. The deletion ceiling added last round truncated the heuristic fallback in candidate order, and listIndexedForgetCandidates pushes every user entry ahead of every project entry. With 450 matching user entries and 50 matching project ones and the side query down, forget deleted 400 user entries, zero project ones, and reported success. That is the reachability asymmetry this PR exists to remove, moved into the delete path. The per-scope allocation the model prompt already used is now shared with the heuristic, so each scope keeps its share of the limit and a smaller scope's unused seats go to the other. The ceiling is also its own constant now rather than an alias of the prompt bound. Resizing the model prompt is a cost decision and resizing this is a blast-radius decision; sharing one constant let the first silently widen the second. Two tests, each checked against its mutation: the 450-user/50-project shape returns zero project matches under a plain slice, and oldest-first ranking inside a scope drops that scope's newest entry from the prompt. Refs: QwenLM#9378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…'s own order Cross-review found both new tests mutation-survivable. Every case used a 400 limit, so hard-coding a 200 per-scope quota instead of deriving it from the budget still passed, and the recency case let the side query succeed, so it pinned the model prompt's ranking rather than selectByHeuristic's own comparator. One case at limit 5 with the side query failing covers both: it asserts the 3/2 split, which only holds if the quota comes from the budget, and that each scope contributes its newest entry, which fails if the comparator is reversed. Both mutants verified failing. Refs: QwenLM#9378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): 581 tests pass — this review observed 20569, 1590, 22030, 1612, 494, 3799, 542 passed; Tests 581 passed — this review observed 20569, 1590, 22030, 1612, 494, 3799, 542 passed; Tests 20704 passed — this review observed 20569, 1590, 22030, 1612, 494, 3799, 542 passed; 2 passed — this review observed 20569, 1590, 22030, 1612, 494, 3799, 542 passed; 20694 passed — this review observed 20569, 1590, 22030, 1612, 494, 3799, 542 passed.
— qwen3.8-max via Qwen Code /review (v0.21.15)
| .filter((candidate) => candidate.storageScope === scope) | ||
| .sort((a, b) => b.mtimeMs - a.mtimeMs), |
There was a problem hiding this comment.
[Suggestion] This mtime-descending comparator is the one piece of logic shared by the model-prompt path and the heuristic path that was not shared: rankScopeForPrompt declares it as the named byRecency (around line 230) while selectByHeuristic re-types it inline here. This increment extracted everything else the two paths share — normalizeForgetQuery, matchesForgetQuery, and allocatePerScope, whose call-site comment says "Same per-scope split the model path uses" — precisely so the two cannot drift, but the ordering stayed typed in two places. A future ordering change (a filename tie-break, or a direction flip) applied to only one site makes the model prompt rank candidates differently from the order the heuristic fallback deletes in, and nothing fails: the two orderings are pinned by separate tests ('keeps the newest of a scope…' pins the prompt order, 'scales the per-scope split…' pins this one), so a one-sided change that updates its own test passes CI and silently leaves the sibling site stale.
Hoist one module-level comparator, e.g.:
const byMtimeMsDesc = (
a: IndexedForgetCandidate,
b: IndexedForgetCandidate,
) => b.mtimeMs - a.mtimeMs;and use it in both rankScopeForPrompt (replacing byRecency) and here (.sort(byMtimeMsDesc)).
— qwen3.8-max via Qwen Code /review (v0.21.15)
… deletion Review round 4, both suggestions. The mtime comparator was the last thing the model path and the heuristic path each typed for themselves, after this branch had already hoisted the query normaliser, the match predicate and the per-scope allocator so the two could not drift. Each site has its own test, so a one-sided ordering change would have updated its own test, passed CI, and left the sibling stale. Now one definition. The deletion cap also bound silently. The prompt bound warns when it truncates; the path that actually deletes did not, so a forget that removed 400 of 500 matches reported success and left no record of why recall kept injecting the rest. It now says so. No test for the new warning: it is a debug log line, and asserting on it would pin the wording rather than the behaviour. Refs: QwenLM#9378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 1a": running packages/core/src/memory/forget.test.ts — the review worktree has no node_modules , so executing it would require a full monorepo install; left to CI…; "agent 5": live run of packages/core/src/memory/forget.test.ts and memoryLifecycle.integration.test.ts (no node_modules in the review worktree or parent checkout; would re….
Test Plan (not a blocker): 582 tests pass — this review observed 20647, 1590, 22027, 1612, 494, 3799, 542 passed; Tests 582 passed — this review observed 20647, 1590, 22027, 1612, 494, 3799, 542 passed; Tests 20704 passed — this review observed 20647, 1590, 22027, 1612, 494, 3799, 542 passed; 2 passed — this review observed 20647, 1590, 22027, 1612, 494, 3799, 542 passed; 20694 passed — this review observed 20647, 1590, 22027, 1612, 494, 3799, 542 passed.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/memory/forget.ts:55 — [review] Scope count is encoded three times in forget.ts and can silently driftpackages/core/src/memory/forget.ts:139 — [probe] Deliberately loud user-scope scan failure has no test pinning it
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
The red It is also the only thing standing between this PR and an approving review. The automatic review reached Approve and was downgraded by that one check:
Every finding from review rounds 1 through 4 is addressed as of 46824dd. All other checks are green, including the full ubuntu test job. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D6-2 scope list/ordering duplicated between FORGET_SCOPES and sortTouchedScopes — already deferred in round 5 (forget.ts:55)
- D6-3 loud user-scope scan failure has no test pinning it — already deferred in round 5 (forget.ts:139)
- D6-4 silent truncation when the new bounds bind — already reported (comment 3833175831, forget.ts:658)
Not explored to full depth (tool budget reached): "agent 1c": none — all checks above completed within the tool budget (~15 calls)..
Test Plan (not a blocker): 582 tests pass — this review observed 20785, 1679, 22938, 1641, 495, 4063, 595 passed; Tests 582 passed — this review observed 20785, 1679, 22938, 1641, 495, 4063, 595 passed; Tests 20704 passed — this review observed 20785, 1679, 22938, 1641, 495, 4063, 595 passed; 2 passed — this review observed 20785, 1679, 22938, 1641, 495, 4063, 595 passed; 20694 passed — this review observed 20785, 1679, 22938, 1641, 495, 4063, 595 passed.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/memory/forget.test.ts:402 — [review] candidate factories and 500-doc noise generators pasted across the new forget tests
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R7-1 candidate-document factories duplicated five ways in forget.test.ts — already deferred in round 6 (forget.test.ts:402)
- R7-2 beyond-cap integration fixture duplicated from the sibling recall test — already deferred in round 3 (memoryLifecycle.integration.test.ts:356)
- R7-4 deletion-cap truncation only debug-logged, never surfaced to callers — already reported (comment 3833175831, forget.ts:658)
- R7-5 MAX_MODEL_FORGET_CANDIDATES hardcodes the scope count (* 2) — already deferred in round 5 (forget.ts:55)
Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short..
Test Plan (not a blocker): 582 tests pass — this review observed 21053, 1685, 23536, 1654, 495, 4190, 610 passed; Tests 582 passed — this review observed 21053, 1685, 23536, 1654, 495, 4190, 610 passed; Tests 20704 passed — this review observed 21053, 1685, 23536, 1654, 495, 4190, 610 passed; 2 passed — this review observed 21053, 1685, 23536, 1654, 495, 4190, 610 passed; 20694 passed — this review observed 21053, 1685, 23536, 1654, 495, 4190, 610 passed.
— qwen3.8-max via Qwen Code /review (v0.22.0)
Local maintainer verification — ✅ merge-ready (71/71 scripted assertions, 0 unexpected failures)Full local A/B verification of this PR against the base build, run on a real environment (macOS, Node 24, production code paths with real files — no scan mocks). Verified head Central claim, proven load-bearing (A/B). 200 filler docs + 1 target with an epoch mtime (ranks 201st under the capped scanner), driven through the production recall → forget-select → forget-delete path on both builds:
The #9378 asymmetry reproduces exactly on base and is closed on head. Evidence: Unconfirmed deletion ceiling. 450 user + 50 project docs all matching a one-token query: head deletes exactly 400 (350 user + 50 project — every project seat kept by the per-scope allocation); base deletes 250 (200 user + 50 project — the old cap artifact). Model-selection prompt bound — the gap an earlier review round flagged as mock-only coverage. This round drives the real
Two things worth spelling out: on stores at ≤ ~200 docs/scope the cap and the bound render identical 400-shape prompts (the change is invisible at steady state, by design), and on base a pre-cap entry was unreachable by the model path even with a perfect model — it never reached the wire. Sibling boundary. "Successful-but-empty selection is not rescued by the heuristic" — confirmed on both arms: strategy short-circuits to Vacuity + mutation matrix (each mutation applied to head's
No surviving mutants — every guard this PR adds is pinned by its own test. Gates. Not covered: full Evidence
中文摘要结论:可合并(merge-ready)——71/71 脚本断言全部通过,无一意外失败;在本地真实环境(macOS、Node 24、真实文件、无扫描 mock)完成 base↔head 的 A/B 深度验证。
|
|
@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: 37 passed · 0 failed · 37 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:37 通过 · 0 失败 · 37 总计 抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9530 Deep Verification —
|
| cell | arm | oracle | result |
|---|---|---|---|
| capped scan control | base + head | capped scanner returns exactly 200 docs, excludes target | 200, excluded — both arms |
| recall positive control | base + head | resolveRelevantAutoMemoryPromptForQuery selects target |
selected — both arms |
| forget selection | base | heuristic selection for 'overflow-zephyr-7040' |
0 matches — bug reproduces |
| forget selection + delete | head | same | target selected; file deleted; 1 entry removed |
Secondary claims, same harness style (bounds-harness.ts, both arms):
| cell | arm | oracle | result |
|---|---|---|---|
| per-scope split (450 user + 50 project matches, limit 400, heuristic path) | head | match counts per scope | 400 total = 350 user / 50 project; newest user seated, oldest truncated |
| same | base | same | 250 total = 200 user (scan cap) / 50 project |
unconfirmed deletion ceiling (405 matching docs via forgetManagedAutoMemoryEntries, no config) |
head | deletions stop at 400 | 400 removed, oldest 5 survive (newest-first order) |
| same | base | same | 200 removed (capped scan was the only ceiling), 205 survive |
Witnesses: 01-ab-rank201-forget-base-vs-head.png, 02-bounds-split-and-cap-base-vs-head.png.
Caller mapping verified by grep: /forget command takes the confirmed path
(selectForgetCandidates default limit 5, then forgetMatches); ACP's
acpAgent.ts takes manager.forget → forgetManagedAutoMemoryEntries → the new
400 ceiling. Recall never scans the team tier (only the indexer does), so
project+user is exactly recall's universe — the asymmetry named in the PR is
closed with no remaining scope gap.
Corrections
Two numbers in the PR body do not reproduce at the verified head (neither is a
code problem):
- "36 files, 582 tests" for
npx vitest run src/memory— the verified head has
36 files, 584 tests (all passing, 0 skipped). The PR adds 10 tests over
base (forget.test.ts 11→20, memoryLifecycle 2→3; base suite = 574), so the
body's 582 is a stale tally from an earlier review round. - "596 passed | 1 skipped (597) / 20704 passed" for full packages/core — the
verified head has 605 files / 21138 tests (the final Aug-24 main merge
landed after those numbers were written). In this container 70 tests fail —
but byte-identically at base (see Findings §1); the PR contributes zero
head-only failures.
Findings
1. (info, not a defect) 70 full-suite failures are environmental, proven pre-existing
Full packages/core at head: 8 files / 70 tests fail (logger, ide-client,
skill-manager, subagent-manager, installationManager, memoryDiscovery,
rulesDiscovery, file-token-storage). Re-running the identical 8 files at
base (worktree 9b27184, same container): same 8 files, same 70 failures,
failing test names byte-identical (junit diff: 0 head-only, 0 base-only).
Failure messages show the cause: tests pin mock home paths like
/home/test/.qwen/ide/8080.lock that this container's real home
(/__w/_temp/verify-agent-home) does not produce. Not attributable to the PR.
2. (nit) truncation warnings are observable only in debug logs
The new deletion-bound warning and its sibling prompt-bound warning both go
through createDebugLogger('MEMORY_FORGET').warn, which is a silent no-op unless
a debug session is active and QWEN_DEBUG_LOG_FILE is enabled
(debugLogger.ts:38-43,222-226). In a default run, an unconfirmed forget that
matches 450 entries and deletes 400 still returns success with nothing surfacing
the 50 survivors to the caller (removedEntries is the only signal). This is
parity with the pre-existing prompt-bound warning and matches the round-4
commit's own framing ("it is a debug log line"), so it is not a merge condition —
but anyone later treating the warn as an audit trail should know its visibility
ceiling.
3. (note, verified clean) hermeticity of the new test
The new integration test redirects QWEN_CODE_MEMORY_BASE_DIR before any scan
because forget deletes files. Empirically confirmed: running the integration file
under a fake $HOME on both arms creates nothing outside the temp dirs. (A
0-byte ~/.qwen/memories/MEMORY.md observed in the container home was traced to
the verification agent's own CLI session scaffolding — createDefaultAutoMemoryIndex()
returns '' — not to any test run; the fake-HOME probes on both arms prove the
tests themselves do not leak.)
Mutation matrix (vacuity + guard load-bearingness)
Suite forget.test.ts baseline unmutated: 20/20 green; integration file at
head: 3/3 green. Each mutant reverts one guard the PR introduces; all were
applied to packages/core/src/memory/forget.ts and restored afterwards
(git status clean after every round).
| # | mutation | killed by | observed failure (behavioral) |
|---|---|---|---|
| M0 | scan calls reverted to capped scanners | forgets a topic beyond the general 200-document scan cap |
expected [] to include '/tmp/…/overflow-target.md' — 1 failed | 2 passed; recall test stayed green |
| M1 | heuristic allocatePerScope → plain matched.slice(0, limit) |
2 tests | expected [] to have a length of 50 but got +0 (project seats lost); limit-5 split length 3, got 5 |
| M2 | unconfirmed limit → Number.MAX_SAFE_INTEGER |
1 test | expected [ Array(401) ] to have a length of 400 but got 401 |
| M3 | prompt bound → single global mtime ranking | 3 tests | missing id: project:reference/overflow.md; missing id: user:user/old-0.md; even-split 100 vs 200 |
| M4 | per-scope quota 200 → 150 | 4 tests | prompt sizes 300 vs 400; scope counts 150 vs 200 |
| M5 | positive control: byMtimeMsDesc comparator reversed |
3 tests | missing newest ids (noise-498, doc-299) — proves the harness can fail this suite, in the mutated file itself |
No survivors. M3/M4 red sets are slightly wider than the "intended" test per
mutation because the prompt-budget tests share the same mechanism
(allocatePerScope / the quota constant) — kills, not unexpected failures.
Witness: 04-mutation-matrix-and-gates.png.
Targeted gates
| gate | result |
|---|---|
npx vitest run src/memory (packages/core) |
36 files, 584 passed, 0 failed, 0 skipped |
npx vitest run full packages/core |
605 files, 21058 passed, 70 failed — all 70 byte-identical at base (Finding 1) |
npm run typecheck -w @qwen-code/qwen-code-core (tsc --noEmit) |
exit 0 |
eslint on the 4 changed .ts files |
exit 0; live-gate proof: a planted probe file with an unused var + any produced exactly 2 errors, then was removed |
Not covered
- Live model selection path: no API access in the sandbox. The prompt bound
(selectModelForgetCandidates) was exercised through the PR's own
runSideQuery-mocked suite plus my M3/M4/M5 reverts, not a real side query.
The heuristic + deletion paths are proven mock-free above. - Per-commit attribution: depth-2 checkout makes only the head commit
reachable (git rev-list HEAD^1..HEAD^2= 1 of the 8 snapshot commits). The
aggregateHEAD^1..HEADdiff was verified; per-commit claims (e.g. which
review round added which guard) were not. - Issue Recall/forget scan-cap asymmetry: documents beyond the 200-doc cap can be recalled but never forgotten #9378 text: unavailable offline; verified against the asymmetry as
stated in the PR body and commit messages. - Scan-cost scaling ladder: forget now parses entries for docs past the cap.
scan.tsalready reads and stats every file before slicing (verified by
readingscanAutoMemoryDocumentsFromRoot), so I/O is unchanged and only entry
parsing grows — the same cost recall pays every turn. No synthetic 20k-doc
timing was run; forget is user-initiated. - Repo-wide gates (
npm run preflight, full lint, other packages' tests),
Windows/macOS platform matrix, and live ACP/CLI E2E of/forget. - Indexer/status/extraction stay capped — accepted by design; not probed
beyond reading the call sites and the updated design docs. - Empty/whitespace query edge:
forgetManagedAutoMemoryEntriestrims and
short-circuits empty queries (read-verified); direct
selectManagedAutoMemoryForgetCandidates('')ranks everything as "literal"
(includes('')) — harmless, not separately asserted.
Methodology
Environment: CI verify container (node:22-bookworm), Node v22.23.2, vitest
3.2.7. Working tree = refs/pull/9530/merge (depth 2); base control =
git worktree add tmp/base-tree HEAD^1 at 9b27184. Harnesses
(ab-harness.ts, bounds-harness.ts, kept in this artifact dir) import the
production TypeScript directly from the tree under test via tsx — no stubs of
code under test, real temp memory trees, QWEN_CODE_MEMORY_BASE_DIR redirected
to temp so user-scope scans never touch a real home. Base-arm control validity:
forget.ts and its whole import closure are repo-relative (./, ../) — no
workspace-symlink crossings — and each run prints realpath of the loaded
forget.ts (base runs resolved into tmp/base-tree); packages/core's nested
node_modules (ajv v8 et al.) was symlinked into the base worktree, which is a
clean control because the diff touches zero package.json/lockfile paths. For
the 8-file base A/A, head's dist/ was symlinked only to satisfy the vitest
globalSetup guard after confirming none of the 8 files imports the package
entry. Mutations were applied with mutate.mjs (refuses to run unless the
target snippet matches exactly once) and restored with git checkout after each
round. Raw logs in logs/ (per-arm harness output, memory-suite summary, full
core suite tail, 8-file base run); evidence images in evidence/.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/core/src/memory/forget.test.ts: (cd packages/core) npx --no-install vitest run ./src/memory/forget.test.ts
file packages/core/src/memory/memoryLifecycle.integration.test.ts: (cd packages/core) npx --no-install vitest run ./src/memory/memoryLifecycle.integration.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/memory/forget.test.ts: PPPPP
packages/core/src/memory/memoryLifecycle.integration.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 · packages/core/src/memory/forget.test.ts: P (exit 0)
round 1 · packages/core/src/memory/memoryLifecycle.integration.test.ts: P (exit 0)
round 2 · packages/core/src/memory/forget.test.ts: P (exit 0)
round 2 · packages/core/src/memory/memoryLifecycle.integration.test.ts: P (exit 0)
round 3 · packages/core/src/memory/forget.test.ts: P (exit 0)
round 3 · packages/core/src/memory/memoryLifecycle.integration.test.ts: P (exit 0)
round 4 · packages/core/src/memory/forget.test.ts: P (exit 0)
round 4 · packages/core/src/memory/memoryLifecycle.integration.test.ts: P (exit 0)
round 5 · packages/core/src/memory/forget.test.ts: P (exit 0)
round 5 · packages/core/src/memory/memoryLifecycle.integration.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.22.2. |








What this PR does
Forget now scans the same document universe recall does.
listIndexedForgetCandidatesmoves from the capped scanners to the uncapped ones, so an entry recall can inject is an entry forget can remove.Two things had to be bounded to make that safe, because both were previously bounded only as a side effect of the 200-document scan cap.
The model-selection prompt.
buildForgetSelectionPromptinterpolates every candidate inline, so it takes its own bound of 400. Each scope keeps a 200-candidate quota and whatever a smaller scope leaves unused goes to the other. Within a scope, literal query matches rank first and both groups are ordered newest first. The per-scope split matters: ranking both scopes into one recency budget would seat zero user entries on a store whose project entries are all newer, which is the same unreachability this PR exists to remove.The unconfirmed deletion path.
forgetManagedAutoMemoryEntries, whichMemoryManager.forgetand ACP use, deletes without confirmation and passedlimit: Number.MAX_SAFE_INTEGER. With an uncapped scan and a heuristic fallback that substring-matches the whole store, a one-character query matched nearly everything. It now passesMAX_UNCONFIRMED_FORGET_DELETIONS, a constant of its own rather than an alias of the prompt bound: resizing the model prompt is a cost decision, resizing this is a blast-radius decision. The heuristic fallback shares the same per-scope allocation, so a limit cannot be consumed entirely by one scope.Indexer, status and extraction stay capped deliberately. The extraction planner renders each document into a forked agent's task prompt, so an uncapped scan there would grow that prompt without bound; a comment at the call site says so, and the agent still holds
read_file/grep/glob/lsto reach anything past the cap. The two design docs that recorded forget as capped now record what it actually does.Why it's needed
#8716 moved recall to
scanAllAutoMemoryTopicDocuments/scanAllUserAutoMemoryTopicDocuments. Forget stayed on the 200-document capped scanner.Private-tier scans order by mtime descending and cut at
MAX_SCANNED_MEMORY_FILES(scan.ts:162), so any document older than the 200 most recently modified was invisible tolistIndexedForgetCandidates. It produced no candidate, so neitherselectByModelnorselectByHeuristiccould return it andforgetManagedAutoMemoryMatchesnever deleted it. Recall read that same document uncapped and injected it.The result was memory that could be recalled and injected but never forgotten. A user asking to forget it was told nothing matched, and it kept arriving in the prompt on later turns. The asymmetry did not exist before #8716, when both sides shared the capped scan.
Reviewer Test Plan
How to verify
Expected: 36 files, 582 tests pass.
The end-to-end case is
forgets a topic beyond the general 200-document scan capinmemoryLifecycle.integration.test.ts. It builds a real temporary memory tree of 200 filler documents plus one target with an epoch mtime so it ranks 201st, then asserts in order that the capped scanner returns 200 documents without the target, thatresolveRelevantAutoMemoryPromptForQueryselects it anyway, thatselectManagedAutoMemoryForgetCandidatesalso returns it, and thatforgetManagedAutoMemoryMatchesdeletes the file. No scan mock: it drives the production path and asserts on real files. It setsQWEN_CODE_MEMORY_BASE_DIRto a temp dir, because forget deletes files and the user-level scan would otherwise reach the real~/.qwen/memories.forget.test.tscovers the bounds. Each of these was checked against the mutation it is meant to catch, and the mutant failure is quoted below:Evidence (Before & After)
Negative control for the original bug. The new integration test against unmodified
forget.ts, production file restored fromorigin/mainand the test kept:Recall finds it, forget returns an empty match list. That is the bug.
Every bound is separately controlled. Reverting each guarantee in turn:
With the fix:
The same full suite on restored upstream sources gives 20694 passed, also exit 0. The difference is the tests this PR adds, so nothing here is pre-existing breakage and nothing regressed.
npm run preflightis green.Tested on
Verified on macOS, Node 22.22.3. Typecheck and lint clean. Windows and Linux via CI.
Environment (optional)
Unit and integration tests only, no live API call. Node 22.
Risk & Scope
/forgetnow parses and ranks the whole project and user memory tree instead of the first 200 documents of each. Scan I/O is unchanged, becausescan.tsalready enumerates, reads and stats every document before slicing; what grows is entry parsing and ranking. Recall already pays the same cost on every turn, and forget is user-initiated.MEMORY.mdindex and from/memory statuscounts. Only the recall-versus-forget asymmetry named in the issue is closed.forget.test.tsretargets its./scan.jsmock from the capped functions to the uncapped ones and spreadsimportOriginalrather than replacing the module. Without that the mock would export noscanAll*and every test in the file would fail on an undefined call.entryIndexvalues still resolve,forgetManagedAutoMemoryMatchesremoves the valid subset and does not summary-rematch the stale one. That predates this diff.Linked Issues
Refs #9378
AI-assisted: implemented with Claude Opus 5, revised across three rounds of the repo's review bot and three rounds of independent cross-model review (GPT-5.6-sol via Codex).