Skip to content

fix(memory): refresh instructions after remember - #6497

Merged
wenshao merged 14 commits into
QwenLM:mainfrom
han-dreamer:fix/remember-refresh-memory
Jul 11, 2026
Merged

fix(memory): refresh instructions after remember#6497
wenshao merged 14 commits into
QwenLM:mainfrom
han-dreamer:fix/remember-refresh-memory

Conversation

@han-dreamer

@han-dreamer han-dreamer commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Refreshes live memory instructions after successful managed-memory writes, so newly written /remember memory can be observed by the active session without restarting.

The shared core helper detects successful write_file / edit operations that touch private managed-memory roots, rebuilds the touched project and/or user memory indexes, and then refreshes hierarchical memory plus the live system instruction. The interactive TUI path runs this refresh after completed tool results are available and before the ToolResult continuation is sent back to the model, so the model can see the updated memory within the same turn. The ACP workspace-memory path refreshes live session configs/clients, and ACP session tool execution also refreshes after successful managed-memory writes.

Refresh failures are best-effort: index rebuild failures are logged and do not prevent the live instruction refresh attempt, and instruction refresh failures do not cause the original remember/tool request to fail.

Why it's needed

Managed /remember writes memory files during a live session, but the active system instruction could remain stale until restart. That meant the memory landed on disk while the model continued to see an old or empty MEMORY.md instruction in subsequent requests.

The earlier command-level refresh approach was too early for the documented look-then-write flow, because the memory file is written by later tool calls rather than by the command parser itself. Refreshing after successful managed-memory tool writes fixes the timing issue and also covers the ACP live-session path.

Reviewer Test Plan

How to verify

Use managed memory and run /remember always use tabs, never spaces. Let the model write the managed memory topic file, then inspect the next model request or continue the session. The active system instruction should include the rebuilt memory index without restarting the CLI.

Also verify the path guard by writing an ordinary non-memory file, such as ./notes.md. That write should not rebuild managed-memory indexes and should not refresh the live memory instruction.

For user-scope managed memory, write into the user memory root. Only the user memory index should refresh; the project memory index should not be rebuilt.

Evidence (Before & After)

Before: on main, the memory file lands on disk, but the main session system instruction remains byte-identical across subsequent requests and can still say that MEMORY.md is empty.

After: this PR refreshes after the managed-memory write completes and before the tool-result continuation is sent back to the model. The rebuilt memory index is included in the active system instruction within the same turn, and ordinary non-memory writes do not trigger spurious refreshes.

Maintainer Linux E2E verification also confirmed the bug reproduces on main, this PR fixes it with the real bundled CLI under tmux, and the path guard holds.

Tested on

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

Environment (optional)

Windows local unit/static checks plus maintainer Linux tmux E2E with the real bundled CLI and a mock OpenAI-compatible server.

Local checks run on Windows:

  • npm run test:ci --workspace=packages/core -- src/memory/refresh.test.ts
  • npm run test:ci --workspace=packages/cli -- src/ui/commands/rememberCommand.test.ts
  • npm run test:ci --workspace=packages/cli -- src/acp-integration/acpAgent.test.ts
  • npx vitest run src/acp-integration/session/Session.test.ts --coverage.enabled=false
  • npm run typecheck --workspace=packages/core
  • npm run typecheck --workspace=packages/cli

Note: local Windows CLI typecheck previously showed an environment/stale-build artifact in unrelated channel/serve ACP bridge types, while changed-file checks and PR CI were clean.

Risk & Scope

  • Main risk or tradeoff: refreshing hierarchical memory can do extra work, so this PR only triggers on successful private project/user managed-memory writes and deliberately ignores ordinary file writes and team-memory writes.
  • Not validated / out of scope: auto-memory extraction can still leave the live system instruction stale; that path is intentionally left as a follow-up for Memory index stale after /remember; memory content lost on compaction #6487.
  • Breaking changes / migration notes: None.

Linked Issues

Refs #6487

中文说明

这个 PR 做了什么

这个 PR 会在 managed-memory 写入成功后刷新当前会话中的 memory instructions,让 /remember 新写入的记忆不需要重启就能被当前活跃会话看到。

共享的 core helper 会检测成功的 write_file / edit 操作是否写入了私有 managed-memory 根目录。如果命中,它会重建被触达的 project 和/或 user memory index,然后刷新 hierarchical memory 以及当前 live system instruction。交互式 TUI 路径会在 tool results 已经完成之后、ToolResult continuation 发回模型之前执行刷新,因此模型可以在同一轮内看到更新后的 memory。ACP workspace-memory 路径会刷新 live session configs/clients,ACP session 的 tool execution 也会在成功写入 managed memory 后执行刷新。

刷新失败是 best-effort 的:index rebuild 失败会被记录日志,但不会阻止继续尝试刷新 live instruction;instruction refresh 失败也不会导致原本的 remember/tool 请求失败。

为什么需要

Managed /remember 会在 live session 中写入 memory 文件,但当前活跃的 system instruction 之前可能一直保持 stale,直到重启才更新。这会导致 memory 已经落盘,但模型在后续请求里仍然看到旧的或者空的 MEMORY.md instruction。

之前 command-level 的刷新方案对文档里的 look-then-write 流程来说太早了,因为 memory 文件不是 command parser 自己写入的,而是后续 tool calls 写入的。现在改成在 successful managed-memory tool writes 之后刷新,可以修复这个时序问题,同时也覆盖 ACP live-session 路径。

Reviewer Test Plan

如何验证

启用 managed memory 后运行 /remember always use tabs, never spaces。让模型写入 managed memory topic file,然后检查下一次模型请求或者继续会话。当前活跃的 system instruction 应该包含 rebuilt memory index,不需要重启 CLI。

同时通过写入普通非 memory 文件验证 path guard,例如 ./notes.md。这个写入不应该触发 managed-memory index rebuild,也不应该刷新 live memory instruction。

对于 user-scope managed memory,写入 user memory root 时应该只刷新 user memory index,不应该重建 project memory index。

证据 Before & After

Before:在 main 上,memory 文件会落盘,但 main session 的 system instruction 在后续请求中保持 byte-identical,并且仍然可能显示 MEMORY.md 是空的。

After:这个 PR 会在 managed-memory write 完成之后、tool-result continuation 发回模型之前刷新。rebuilt memory index 会在同一轮内进入当前活跃的 system instruction,并且普通非 memory 文件写入不会触发误刷新。

维护者的 Linux E2E 验证也确认了该 bug 可以在 main 上复现,本 PR 使用 tmux 中的真实 bundled CLI 修复了该问题,并且 path guard 有效。

测试平台

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

环境

Windows 本地 unit/static checks,加上维护者在 Linux 上使用真实 bundled CLI、tmux 和 mock OpenAI-compatible server 做的 E2E 验证。

Windows 本地运行过的检查:

  • npm run test:ci --workspace=packages/core -- src/memory/refresh.test.ts
  • npm run test:ci --workspace=packages/cli -- src/ui/commands/rememberCommand.test.ts
  • npm run test:ci --workspace=packages/cli -- src/acp-integration/acpAgent.test.ts
  • npx vitest run src/acp-integration/session/Session.test.ts --coverage.enabled=false
  • npm run typecheck --workspace=packages/core
  • npm run typecheck --workspace=packages/cli

说明:Windows 本地 CLI typecheck 之前出现过 unrelated channel/serve ACP bridge types 的环境/旧构建产物问题,但 changed-file checks 和 PR CI 是干净的。

风险和范围

  • 主要风险或取舍:refreshing hierarchical memory 可能带来额外工作量,所以这个 PR 只在成功写入私有 project/user managed-memory 时触发,并且有意忽略普通文件写入和 team-memory 写入。
  • 未验证 / 不在范围内:auto-memory extraction 目前仍然可能让 live system instruction stale;这条路径会作为 Memory index stale after /remember; memory content lost on compaction #6487 的 follow-up 处理。
  • Breaking changes / migration notes:无。

关联 Issue

Refs #6487

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR @han-dreamer!

Template looks good ✓

Problem: references #6487 — managed /remember writes memory to disk but the current session keeps using the stale system instruction until restart. The mechanism is clear: memory files are read into the system instruction at startup, so a mid-session write needs a refresh to become visible. The existing languageCommand.ts already performs the same refreshHierarchicalMemory() + refreshSystemInstruction() sequence after changing output-language.md, which confirms this is a real gap in the remember path. No before/after reproduction provided, but the problem is structurally self-evident.

Direction: aligned with existing patterns. The /language command already does exactly this refresh after updating output-language.md (line 148-149 of languageCommand.ts). Memory visibility without restart is a basic expectation.

Size: 6 production lines + 22 test lines + 0 generated/schema lines. Tiny, focused change in packages/cli/src/ only — no core module paths touched.

Approach: scope is tight and appropriate — two paths (interactive /remember via onComplete hook, ACP workspace memory via direct calls), both addressed. No scope creep, no drive-by refactors. One code review note for Stage 2: in acpAgent.ts, the refresh calls are placed inside the existing try block, so a refresh failure would surface as a remember error. The existing reload command (line ~7904) wraps these same calls in individual try/catch with debugLogger.warn — worth flagging.

Moving on to code review. 🔍

中文说明

感谢贡献 @han-dreamer

模板完整 ✓

问题:关联 #6487 — managed /remember 将记忆写入磁盘但当前会话继续使用过时的 system instruction 直到重启。机制很清晰:memory 文件在启动时读入 system instruction,因此会话中途写入需要刷新才能可见。已有的 languageCommand.ts 在修改 output-language.md 后已经执行了相同的 refreshHierarchicalMemory() + refreshSystemInstruction() 序列,确认这是 remember 路径中的一个真实缺口。未提供 before/after 复现,但问题从结构上是自明的。

方向:与现有模式一致。/language 命令在更新 output-language.md 后已经做了完全相同的刷新(languageCommand.ts 第 148-149 行)。无需重启即可让 memory 可见是一个基本预期。

规模:6 行生产代码 + 22 行测试代码 + 0 行生成/schema 代码。变更非常小且聚焦,仅在 packages/cli/src/ 中,未触及核心模块路径。

方案:范围紧凑合理——两条路径(交互式 /remember 通过 onComplete hook,ACP workspace memory 通过直接调用),都已处理。无范围蔓延,无顺手重构。一个代码审查注意点:在 acpAgent.ts 中,refresh 调用放在了已有的 try 块内,因此 refresh 失败会以 remember 错误的形式呈现。现有的 reload 命令(约第 7904 行)将这些相同的调用分别用 try/catch 包裹并 debugLogger.warn——值得指出。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Before reading the diff, my independent proposal was: use the existing submit_prompt.onComplete hook in the interactive path to trigger refreshHierarchicalMemory() + refreshSystemInstruction() after the remember agent completes, and add the same refresh calls in the ACP path after runManagedRememberByAgent returns — but wrap them in a separate try/catch so refresh failures don't mask as remember failures.

The PR matches this proposal exactly in the interactive path — the onComplete callback in rememberCommand.ts is the right hook, and useGeminiStream.ts already fires it with void ... .catch() error logging, so a refresh failure won't fail the remember operation. ✓

One issue in the ACP path: in acpAgent.ts (line ~5819), the two new refresh calls are placed inside the existing try block that wraps runManagedRememberByAgent. If either refresh throws, the catch block below treats it as a remember failure — it calls extractRememberErrorCode(err) on a refresh error, which will produce a misleading error code and message for the user. The memory was successfully saved, but the user sees a "remember failed" error.

The codebase already has the correct pattern at line ~7904 of the same file — the reload command wraps each refresh call in its own try/catch with debugLogger.warn:

try {
  await config.refreshHierarchicalMemory();
} catch (err) {
  debugLogger.warn(`reload: refreshHierarchicalMemory failed for session ${id}: ${err}`);
}
try {
  await config.getGeminiClient()?.refreshSystemInstruction();
} catch (err) {
  debugLogger.warn(`reload: refreshSystemInstruction failed for session ${id}: ${err}`);
}

The ACP remember path should follow this same pattern. Something like:

const result = await runManagedRememberByAgent({ ... });
try {
  await this.config.refreshHierarchicalMemory();
  await this.config.getGeminiClient()?.refreshSystemInstruction();
} catch (err) {
  debugLogger.warn('workspace memory remember: refresh failed:', err);
}
return result as unknown as Record<string, unknown>;

This keeps the refresh best-effort — the remember itself succeeded, and a stale system instruction is better surfaced as a warning than as a false remember error.

Test Results

Unit tests verified in worktree:

$ cd packages/cli && npx vitest run src/ui/commands/rememberCommand.test.ts
 ✓ src/ui/commands/rememberCommand.test.ts (5 tests) 7ms
 Test Files  1 passed (1)
      Tests  5 passed (5)

$ cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts
 ✓ src/acp-integration/acpAgent.test.ts (191 tests) 8435ms
 Test Files  1 passed (1)
      Tests  191 passed (191)

Prettier: All matched files use Prettier code style!
ESLint: passed (0 warnings) ✓

Real-Scenario Testing

$ qwen -p 'respond with just the word hello' -y
hello

Basic CLI invocation works. However, the /remember managed memory path requires config.isManagedMemoryAvailable() === true, which depends on managed memory infrastructure (memory backend service) not available in this CI environment. The behavioral change — system instruction refresh after remember — is an internal state update not directly observable in terminal output. Unit tests adequately verify the callback wiring.

中文说明

代码审查

在阅读 diff 之前,我的独立方案是:在交互式路径中使用已有的 submit_prompt.onComplete hook,在 remember agent 完成后触发 refreshHierarchicalMemory() + refreshSystemInstruction();在 ACP 路径中在 runManagedRememberByAgent 返回后添加相同的刷新调用——但用单独的 try/catch 包裹,避免 refresh 失败被误报为 remember 失败。

PR 在交互式路径中完全匹配此方案——rememberCommand.ts 中的 onComplete 回调是正确的 hook,useGeminiStream.ts 已经通过 void ... .catch() 触发它,因此 refresh 失败不会导致 remember 操作失败。✓

ACP 路径中的一个问题:acpAgent.ts(约第 5819 行),两个新的 refresh 调用放在了包裹 runManagedRememberByAgent 的已有 try 块内。如果任一 refresh 抛异常,下方的 catch 块会将其视为 remember 失败——对 refresh 错误调用 extractRememberErrorCode(err),会产生误导性的错误代码和消息。记忆已成功保存,但用户看到的是"remember 失败"。

代码库中在同一文件约第 7904 行已有正确模式——reload 命令将每个 refresh 调用用单独的 try/catch + debugLogger.warn 包裹。ACP remember 路径应遵循相同模式。

测试结果

Worktree 中验证的单元测试:5 个 rememberCommand 测试和 191 个 acpAgent 测试全部通过。Prettier 和 ESLint 检查通过。

真实场景测试

基本 CLI 调用正常工作。但 /remember 的 managed memory 路径需要 config.isManagedMemoryAvailable() === true,这依赖于当前 CI 环境中不可用的 managed memory 基础设施。行为变更——remember 后的 system instruction 刷新——是一个在终端输出中不可直接观察到的内部状态更新。单元测试已充分验证了回调的接线。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a focused, well-motivated fix for a real gap — /remember writing memory to disk but the current session not picking it up until restart. The scope is tight (6 production lines), the approach mirrors what /language already does, and the interactive path is handled correctly via the onComplete hook.

The one thing that needs fixing before this can ship is the ACP path error handling. Right now, if refreshHierarchicalMemory() or refreshSystemInstruction() throws after a successful remember, the user sees "remember failed" even though the memory was saved fine. The codebase already has the right pattern in the same file — the reload command at line ~7904 wraps each refresh call in its own try/catch with debugLogger.warn. Moving the refresh calls into a similar wrapper would make the ACP path consistent with the interactive path (which handles this correctly through useGeminiStream's void onComplete().catch(...) pattern).

Everything else looks clean — tests pass, lint passes, the PR body is thorough, and the direction is aligned. Once the ACP error handling is adjusted, this is ready to go.

中文说明

这是一个聚焦且有明确动机的修复——/remember 将记忆写入磁盘但当前会话直到重启才能获取到。范围很小(6 行生产代码),方案与 /language 已有的做法一致,交互式路径通过 onComplete hook 正确处理。

唯一需要在合并前修复的是 ACP 路径的错误处理。目前,如果在成功的 remember 之后 refreshHierarchicalMemory()refreshSystemInstruction() 抛异常,用户会看到"remember 失败",尽管记忆已成功保存。代码库中在同一文件已有正确模式——约第 7904 行的 reload 命令将每个 refresh 调用用单独的 try/catch + debugLogger.warn 包裹。将 refresh 调用移到类似的包裹中,可以让 ACP 路径与交互式路径保持一致(后者通过 useGeminiStreamvoid onComplete().catch(...) 模式正确处理)。

其余一切看起来都很干净——测试通过、lint 通过、PR 描述详尽、方向一致。一旦 ACP 错误处理调整完毕,就可以合并了。

Qwen Code · qwen3.7-max

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good fix for a real gap — just one adjustment needed. In the ACP path (acpAgent.ts), the refresh calls need their own try/catch so a refresh failure doesn't surface as a remember error. The reload command in the same file already has the right pattern. See Stage 2 comment for details. 🙏

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Duplicated refresh pattern across code paths

The refreshHierarchicalMemory() + refreshSystemInstruction() pair appears in at least 5 call sites with 3 different error-handling strategies (none, single try/catch, individual try/catch, Promise.allSettled). A shared helper (e.g., refreshMemoryState(config)) with consistent per-call error handling would eliminate the duplication and prevent the inconsistency from recurring as the codebase evolves.

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/ui/commands/rememberCommand.ts Outdated
Comment thread packages/cli/src/ui/commands/rememberCommand.test.ts

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No review findings. Downgraded from Approve to Comment: CI still running.

This is a focused, well-structured fix. The per-call try/catch error isolation in both the interactive onComplete path and the ACP workspace memory path correctly prevents refresh failures from masking a successful remember operation. Test coverage is solid — both the happy path and resilience-on-failure are verified in both code paths. The sequential ordering of refreshHierarchicalMemory before refreshSystemInstruction respects the data dependency (system instruction reads memory state).

— qwen3.7-max via Qwen Code /review

wenshao added a commit to wenshao/qwen-code that referenced this pull request Jul 8, 2026
wenshao added a commit to wenshao/qwen-code that referenced this pull request Jul 8, 2026
wenshao added a commit to wenshao/qwen-code that referenced this pull request Jul 8, 2026
wenshao added a commit to wenshao/qwen-code that referenced this pull request Jul 8, 2026
@wenshao

wenshao commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

E2E verification report — ⚠️ not ready to merge

I built a real end-to-end harness for this PR (real qwen TUI under tmux, real qwen --acp subprocess driven by a genuine @agentclientprotocol/sdk client, deterministic mock OpenAI server capturing every model request) and A/B'd 3fc9c3f against merge-base faf7c434.

Summary: the diagnosis in #6487 is right and the direction here is right, but neither half of the fix works in the case that matters.

Path Result
Interactive /remember, model writes in one model round fixed (BASE stale → PR fresh)
Interactive /remember, model looks then writes (two rounds) still staleonComplete fires before any tool runs
ACP qwen/control/workspace/memory/remember (the ext method this PR patches) complete no-op
ACP /remember slash command (supportedModes includes 'acp') callback never invoked

Tests, lint, format all pass on Linux (the PR's table said Linux untested): rememberCommand.test.ts 6/6, acpAgent.test.ts 192/192, prettier ✓, eslint --max-warnings 0 on all four changed files ✓, git diff --check ✓. The new tests are non-vacuous — reverting only the two source files to merge-base (keeping the PR's tests) makes exactly the 4 new tests fail.


1. The interactive fix works only when the model writes in its first round

onComplete is fired at useGeminiStream.ts:2616, immediately after processGeminiStreamEvents() returns. That function does not await tool calls — it only schedules them (scheduleToolCalls(...), line 2287, fire-and-forget) and returns Completed. The finally block at :2662 then nulls submitPromptOnCompleteRef, so the callback can never fire again on the ToolResult continuation.

So the refresh runs while the write_file calls are still in flight — or, when the model needs a second round, before they have even been requested.

interactive A/B

In the one-round case the PR wins the race by ~80 ms (the time refreshHierarchicalMemory() spends inside loadServerHierarchicalMemory() before it reaches readAutoMemoryIndex()). Add a second model round and it loses deterministically — reproduced identically across repeated runs:

timing trace

REFRESH_START precedes the first WRITE_START by 55 ms, and readAutoMemoryIndex() gets ENOENT. MEMORY.md is correctly written to disk in every arm — the model just never sees it.

This is not a contrived shape. The managed-memory system prompt explicitly instructs:

Do not write duplicate memories. First check if there is an existing memory in any of your memory directories you can update before writing a new one.

Look-then-write is the prescribed behaviour, so a real model will usually take ≥ 2 rounds — landing on exactly the branch where the fix does nothing. The same applies whenever the write is merely slow: a PreToolUse hook, a large memory tree, a slow FS.

Nothing in the TUI distinguishes the two runs — both print Wrote 2 memories / Saved the memory.:

TUI panes

Why /dream got away with this hook: its onComplete is recordDream — it writes a manual-run timestamp, which is order-insensitive. /remember is the first consumer whose callback must observe the turn's side effects, so it is the first to notice that submit_prompt.onComplete means "the first model stream ended", not "the turn finished".

Suggested direction: don't fire the callback when the just-finished stream scheduled tool calls; carry the ref across the ToolResult continuations and fire it (and don't null it in finally) only when the turn genuinely terminates. Alternatively, sidestep the hook entirely and have the write path itself trigger the refresh when a write lands under a managed-memory root (isAllowedMemoryPath / isUserAutoMemPath already exist) — that variant would also cover the background auto-extraction writes, which #6487 mentions and this PR leaves untouched.


2. Both ACP paths are no-ops

Driving a real qwen --acp subprocess with a genuine ACP client shows zero delta between BASE and PR, on both the ext method and the /remember slash command:

ACP evidence

2a. The ext method refreshes the wrong Config. In acpAgent.ts, this.config is the bootstrap config. Every ACP session builds its own Config (and therefore its own GeminiClient) in newSessionConfig()loadCliConfig(...). Worse, runAcpAgent() initialises the bootstrap config with { skipGeminiInitialization: true }, so its GeminiClient never gets a chat — and refreshSystemInstruction() returns at its first line:

[probe] sessionId 69e12675-b507-487e-a7f8-53146a2ec8ee
REFRESH_PROBE refreshSystemInstruction sid=9d70576f-… hasChat=false     ← bootstrap config, early-returns

await this.config.getGeminiClient()?.refreshSystemInstruction() is therefore guaranteed dead code. The new test passes only because it stubs getGeminiClient() to return a plain object with a refreshSystemInstruction spy — mocking away precisely the thing that fails in production.

acpAgent.ts already contains the correct idiom twice (:6628 for /language, :7904 for the reload path):

await Promise.allSettled([...this.sessions.values()].map(async (s) => {
  const cfg = s.getConfig();
  await cfg.refreshHierarchicalMemory();
  await cfg.getGeminiClient()?.refreshSystemInstruction();
}));

2b. The /remember slash command never runs its callback under ACP. rememberCommand declares supportedModes: ['interactive', 'acp'], but the ACP route goes Session.#processSlashCommandResulthandleCommandResult() (nonInteractiveCliCommands.ts:111), which rebuilds the result as { type, content, modelOverride?, outputHistoryItems? }. onComplete is not part of NonInteractiveSlashCommandResult, so it is silently dropped. In my instrumented ACP runs the callback fires 0 times on this path.

dreamCommand.ts already knows this and works around it explicitly:

if (context.executionMode === 'acp') {
  recordDream().catch(() => {});          // onComplete is dropped in ACP — run it here
  return { type: 'submit_prompt', content: prompt };
}

rememberCommand has no such branch. Either thread onComplete through NonInteractiveSlashCommandResult and await it after the ACP prompt turn completes, or state explicitly that the refresh is interactive-only.


3. Smaller notes

  • void onComplete() is not awaited. With the callback moved to true turn-end this is mostly cosmetic, but the next user turn can currently start while the refresh is still running.
  • Mid-turn system-instruction swap. refreshSystemInstruction() calls chat.setSystemInstruction() on the live chat while the tool loop is still running, so whether the same turn's ToolResult request carries the old or the new system prompt is decided by a race. (It carried the old one in my runs — but nothing enforces that, and a changed prefix mid-turn is a prompt-cache miss.) Firing at turn-end removes this too.
  • refreshHierarchicalMemory() is heavier than it looks. Team memory is off by default, but for users who enable memory.enableTeamMemory + memory.enableTeamMemorySync it rebuilds the team index and runs syncTeamMemory() — a git pull + commit + push — on every successful /remember. It also re-fires the onInstructionsLoaded hook each time. Worth a deliberate decision, not a side effect.
  • The PR body's stated risk ("a refresh failure can make the remember request report failure") is resolved by the two later commits; both paths now isolate each refresh. No issue there.

How to reproduce
workspace   isolated git repo, QWEN_CODE_MEMORY_LOCAL=1 (project memory at <ws>/.qwen/memory)
model       mock OpenAI server on 127.0.0.1:8123, logs every /chat/completions body
settings    security.auth.selectedType=openai, memory.enableManagedAutoMemory=false
            (disables auto-extraction + recall prefetch, so the only variable is the refresh)
A/B         single-file dist swap of packages/cli/dist/src/ui/commands/rememberCommand.js
            and .../acp-integration/acpAgent.js  (esbuild transform of each arm's source)
  1. tmux drives the real TUI: /remember the deploy freeze code is FREEZE7X, then a second turn PROBEMARK list my memory index.
  2. The mock's remember-turn reply emits write_file for the topic doc + MEMORY.md. In the two-round scenario it first emits list_directory, then writes on the next round.
  3. Assertion = does the probe turn's role:system message contain FREEZE7X? One-round PR: yes (sysLen 30246 → 39922, the index escalates from the "currently empty" placeholder to the full protocol + the new entry). Two-round PR: no.
  4. ACP arms: real qwen --acp subprocess + ClientSideConnection; initializesession/new → warm-up prompt → (extMethod('qwen/control/workspace/memory/remember') | /remember prompt) → probe prompt. Compare the session's system message before/after.
  5. Timing trace from temporary appendFileSync probes at Config.refreshHierarchicalMemory(), WriteFileTool.execute() and readAutoMemoryIndex().

Linux test totals on 3fc9c3f: rememberCommand.test.ts 6 passed, acpAgent.test.ts 192 passed.

中文版

E2E 验证报告 — ⚠️ 暂不建议合并

我为这个 PR 搭建了真实的端到端环境(tmux 里跑真实 qwen TUI;用真正的 @agentclientprotocol/sdk 客户端驱动真实的 qwen --acp 子进程;确定性 mock OpenAI server 抓取每一次模型请求),并把 3fc9c3f 与 merge-base faf7c434 做了 A/B 对比。

结论: #6487 的诊断是对的,本 PR 的方向也是对的,但两个改动在真正要紧的场景下都不生效

路径 结果
交互式 /remember,模型一轮内完成写入 修好了(BASE 陈旧 → PR 新鲜)
交互式 /remember,模型先查看再写入(两轮) 仍然陈旧 —— onComplete 在任何工具执行前就触发了
ACP qwen/control/workspace/memory/remember(本 PR 修改的 ext method) 完全空操作
ACP /remember 斜杠命令(supportedModes'acp' 回调根本不会被调用

测试、lint、格式化在 Linux 上全部通过(PR 表格里 Linux 标记为未测试):rememberCommand.test.ts 6/6,acpAgent.test.ts 192/192,prettier ✓,对全部四个改动文件跑 eslint --max-warnings 0 ✓,git diff --check ✓。新增测试并非空洞 —— 保留 PR 的测试、只把两个源文件回退到 merge-base,恰好这 4 个新测试失败。

1. 交互式修复只在模型第一轮就写入时才成立

onCompleteuseGeminiStream.ts:2616 触发,即 processGeminiStreamEvents() 返回之后立刻执行。而该函数并不等待工具调用 —— 它只是调度工具(scheduleToolCalls(...),2287 行,fire-and-forget)然后返回 Completed。紧接着 :2662finallysubmitPromptOnCompleteRef 置空,所以回调在后续的 ToolResult 续跑里再也不会触发。

于是刷新是在 write_file 仍在执行途中跑的;如果模型需要第二轮,刷新甚至发生在写入被请求之前。

单轮场景下 PR 以约 80 ms 的差距赢下这场竞态(refreshHierarchicalMemory() 在到达 readAutoMemoryIndex() 之前先花在 loadServerHierarchicalMemory() 上的时间)。加上第二轮模型往返,它就必然失败 —— 重复运行结果完全一致:REFRESH_START 比第一次 WRITE_START 早 55 ms,readAutoMemoryIndex() 拿到 ENOENT。所有 arm 中 MEMORY.md 都正确落盘,只是模型看不到。

这并非人为构造的场景。managed-memory 系统提示词明确要求:

Do not write duplicate memories. First check if there is an existing memory in any of your memory directories you can update before writing a new one.

"先查看、再写入"正是提示词规定的行为,因此真实模型通常会走 ≥ 2 轮 —— 恰好落在修复失效的分支上。写入变慢时同理:PreToolUse hook、庞大的 memory 目录、慢速文件系统。

TUI 上两次运行毫无差别,都打印 Wrote 2 memories / Saved the memory.

为什么 /dream 用同一个 hook 没出问题: 它的 onCompleterecordDream,只写一条手动运行的时间戳记录,与顺序无关。/remember 是第一个需要观察本轮副作用的使用者,因此也是第一个暴露出 submit_prompt.onComplete 语义其实是"第一段模型流结束"而非"整个回合结束"的命令。

建议方向: 当刚结束的这段流调度了工具调用时,不要触发回调;把 ref 保留到 ToolResult 续跑(同时不要在 finally 里清空),只在回合真正结束时触发。或者干脆绕开这个 hook:在写入路径上,当写入落在 managed-memory 根目录内时触发刷新(isAllowedMemoryPath / isUserAutoMemPath 已经存在)—— 这个方案还能顺带覆盖 #6487 提到、本 PR 未处理的后台自动抽取写入。

2. 两条 ACP 路径都是空操作

用真实 ACP 客户端驱动真实 qwen --acp 子进程,BASE 与 PR 在 ext method 和 /remember 斜杠命令上都是零差异

2a. ext method 刷新了错误的 Config acpAgent.ts 里的 this.configbootstrap config。每个 ACP session 都会在 newSessionConfig()loadCliConfig(...) 中构造自己的 Config(因而也有自己的 GeminiClient)。更关键的是,runAcpAgent(){ skipGeminiInitialization: true } 初始化 bootstrap config,它的 GeminiClient 永远没有 chat,于是 refreshSystemInstruction() 在第一行就返回:

[probe] sessionId 69e12675-b507-487e-a7f8-53146a2ec8ee
REFRESH_PROBE refreshSystemInstruction sid=9d70576f-… hasChat=false     ← bootstrap config,直接 early-return

所以 await this.config.getGeminiClient()?.refreshSystemInstruction() 必然是死代码。新增的测试之所以通过,只是因为它把 getGeminiClient() stub 成了带 refreshSystemInstruction spy 的普通对象 —— 恰好把生产环境中真正会失败的东西 mock 掉了。

acpAgent.ts 里已经有两处正确写法(:6628/language:7904 的 reload 路径):遍历 this.sessions,刷新每个 session 自己的 cfg 与 client。

2b. ACP 下 /remember 的回调根本不会执行。 rememberCommand 声明了 supportedModes: ['interactive', 'acp'],但 ACP 路线是 Session.#processSlashCommandResulthandleCommandResult()nonInteractiveCliCommands.ts:111),它会重建结果对象 { type, content, modelOverride?, outputHistoryItems? }onComplete 不在 NonInteractiveSlashCommandResult 里,因此被静默丢弃。在带探针的 ACP 实测中,该路径上回调触发次数为 0

dreamCommand.ts 已经知道这一点,并做了显式绕行:

if (context.executionMode === 'acp') {
  recordDream().catch(() => {});          // ACP 下 onComplete 会被丢弃,这里直接执行
  return { type: 'submit_prompt', content: prompt };
}

rememberCommand 没有这个分支。要么把 onComplete 透传进 NonInteractiveSlashCommandResult 并在 ACP prompt 回合结束后 await,要么明确声明该刷新仅限交互模式。

3. 其他次要问题

  • void onComplete() 没有被 await。 把回调移到真正的回合结束后,这一点基本只是观感问题;但目前用户的下一轮输入可能在刷新还在跑时就开始了。
  • 回合中途替换 system instruction。 refreshSystemInstruction() 会在工具循环仍在运行时对活跃的 chat 调用 setSystemInstruction(),因此同一回合的 ToolResult 请求带的是旧还是新 system prompt,取决于竞态结果。(我的实测中带的是旧的,但没有任何机制保证这一点;而回合中途改变前缀会导致 prompt cache miss。)改到回合结束触发即可一并消除。
  • refreshHierarchicalMemory() 比看起来重。 team memory 默认关闭,但对启用了 memory.enableTeamMemory + memory.enableTeamMemorySync 的用户,它会在每一次成功的 /remember 之后重建 team 索引并执行 syncTeamMemory() —— 一次 git pull + commit + push。它每次还会重新触发 onInstructionsLoaded hook。这应当是一个明确的决定,而不是副作用。
  • PR 描述中提到的风险("refresh 失败可能导致 remember 报告失败")已被后两个 commit 解决,两条路径现在都做了逐调用隔离,这部分没有问题。

复现方式

  1. 隔离的 git workspace,QWEN_CODE_MEMORY_LOCAL=1(项目 memory 位于 <ws>/.qwen/memory);mock OpenAI server 记录每一个 /chat/completions 请求体;settings 里 memory.enableManagedAutoMemory=false(关闭自动抽取与 recall 预取,使唯一变量就是这次刷新)。
  2. A/B 方式:对 packages/cli/dist/src/ui/commands/rememberCommand.js.../acp-integration/acpAgent.js 做单文件 dist 替换(两个 arm 的源码分别用 esbuild transform 产出)。
  3. tmux 驱动真实 TUI:/remember the deploy freeze code is FREEZE7X,随后第二轮 PROBEMARK list my memory index。断言 = probe 回合的 role:system 消息是否包含 FREEZE7X。单轮 PR:包含(sysLen 30246 → 39922);两轮 PR:不包含。
  4. ACP arm:真实 qwen --acp 子进程 + ClientSideConnectioninitializesession/new → 预热 prompt →(extMethod(...)/remember prompt)→ probe prompt,比较 session 前后的 system 消息。
  5. 时序追踪来自在 Config.refreshHierarchicalMemory()WriteFileTool.execute()readAutoMemoryIndex() 处临时插入的 appendFileSync 探针。

Linux 上 3fc9c3f 的测试总数:rememberCommand.test.ts 6 通过,acpAgent.test.ts 192 通过。

@han-dreamer

Copy link
Copy Markdown
Contributor Author

Thanks for the very thorough E2E verification. I agree with the findings and the current PR is not sufficient as-is.

My understanding is:

  1. The interactive /remember change only works in the lucky one-round write case. In the look-then-write flow, submit_prompt.onComplete fires after the first model stream returns, before the scheduled tool calls / ToolResult continuations have actually completed, so the refresh can run before the memory write lands.

  2. The ACP workspace-memory ext method is refreshing the bootstrap config (this.config) rather than the live session config/client, so it does not update the system instruction seen by the active ACP session.

  3. The ACP /remember slash-command path drops onComplete when converting to the non-interactive slash command result, so the callback is never invoked there.

I’ll stop patching the current callback shape and rework this around the actual runtime paths. I’m going to first trace the real turn lifecycle and ACP session config flow locally, then update the PR with a fix that verifies the memory is visible in the next model request, not just that refresh methods were called.

The two viable directions I see are the same ones you called out: either make the submit-prompt completion hook fire at true turn end, after tool calls and ToolResult continuations, or move the refresh trigger closer to the managed-memory write path and make it update the relevant live session(s). I’ll keep the PR scoped to #6487 and avoid widening into a general shared-helper refactor unless you prefer that direction.

@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Replying to @han-dreamer comment

Thanks for the detailed write-up. A couple of notes from reviewing the code:

On point 3: rememberCommand never sets onComplete in the first place — its return value is just { type: 'submit_prompt', content: ... }. So there's nothing being "dropped" by handleCommandResult. This matters for /dream (which does set onComplete in interactive mode and works around it in ACP mode by calling recordDream() eagerly), but not for /remember.

Direction 2 is the right call. Refreshing after the memory is actually written is event-driven and doesn't require tracking the full agentic loop. Some guidance:

  1. Refresh must target live sessions, not bootstrap config. The current extension method uses this.config (bootstrap/workspace-level). For ACP, per-session configs are created via newSessionConfig — the refresh needs to propagate to those. For interactive, it needs to reach the active useGeminiStream session.
  2. Look at how /dream handles the ACP path (dreamCommand.ts) — it eagerly calls recordDream() in ACP mode instead of relying on onComplete. That pattern is worth referencing, though for /remember the refresh belongs in the write path itself, not in the command.
  3. The createHiddenWorkspaceMemoryConfig proxy (acpAgent.ts:405) suppresses chat recording but is still backed by this.config. If the write happens through this proxy, make sure the subsequent refresh doesn't just re-read the same bootstrap config that was already stale for the session.

Keeping scope to #6487 sounds right.

中文版本

感谢详细的分析。对照代码后有几点补充:

关于论点 3: rememberCommand 本身就没有设置 onComplete——它的返回值只是 { type: 'submit_prompt', content: ... },所以 handleCommandResult 并不存在"丢失"的问题。这个 onComplete 丢弃的问题实际影响的是 /dream(交互模式下设置了 onComplete,ACP 模式下通过提前调用 recordDream() 来规避),对 /remember 不成立。

方向 2 是正确的选择。 在 memory 实际写入之后再刷新是事件驱动的,不需要追踪整个 agentic loop。几点实现指引:

  1. 刷新必须到达活跃会话,而非 bootstrap config。 当前扩展方法使用的是 this.config(工作区/启动级配置)。ACP 模式下,每个会话的 config 通过 newSessionConfig 创建——刷新需要传播到这些会话级配置。交互模式下,需要到达活跃的 useGeminiStream 会话。
  2. 参考 /dream 的 ACP 路径处理方式dreamCommand.ts)——它在 ACP 模式下提前调用 recordDream() 而非依赖 onComplete。这个模式值得参考,但对 /remember 来说,刷新应该在写入路径本身触发,而不是在命令里。
  3. createHiddenWorkspaceMemoryConfig 代理acpAgent.ts:405)虽然抑制了聊天记录,但底层仍然是 this.config。如果写入通过这个代理发生,要确保后续刷新不会只是重新读取同一个对会话来说已经过时的 bootstrap config。

保持范围限定在 #6487 是对的。

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Suggestions — commit 3909b95de

File Issue Suggested fix
packages/core/src/memory/refresh.ts:167 All refresh errors log only via debugLogger (no-op without --debug). Production memory refresh failures are completely silent. Add console.warn or telemetry for refresh failures, or use a logging channel that is always active.

— qwen3.7-max via Qwen Code /review

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No issues found. LGTM! ✅

This is a focused, well-structured fix for the stale memory index after /remember. The centralized refresh helpers in refresh.ts cleanly separate detection, index rebuild, and instruction refresh. Test coverage is thorough across all three integration paths. Build passes, 429 tests pass.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Local verification — real tmux E2E on Linux ✅

I verified this PR end-to-end on Linux by driving the real bundled CLI (dist/cli.js) inside tmux against a mock OpenAI-compatible server that records every request. The fix works, the bug is reproduced on main, and the path guard holds. LGTM to merge, with two non-blocking cleanups and one scope note below.

Because the branch is 57 commits behind main and main has since touched 4 of the same files, I tested the merged state (origin/main + this PR = 61eab4d5c), not the raw branch head. The merge is clean.

BASEorigin/main @ e64010c11
PRorigin/main + b5bda61c3 (merged, clean)
EnvLinux 6.12.63, Node v22.22.2, tmux 3.5a, real dist/cli.js bundle

1. The bug reproduces on main

Session: /remember always use tabs, never spaces. The mock model performs the documented two-step save that the system prompt asks for — write the topic file, then add a pointer line to MEMORY.md. Then an ordinary second user turn is sent, and I inspect the system message the CLI actually put on the wire.

On main the memory lands on disk, but the system instruction is byte-identical (30274 chars) across every main-session request, including turn 2. The model is still being told Your MEMORY.md is currently empty.

base

That is exactly bug 1 of #6487.

2. The PR fixes it — and does so within the same turn

pr

Two things worth calling out, both better than the PR description claims:

  • The refresh lands on call#2, i.e. before the tool result is even submitted back to the model. So the model can act on the new memory immediately, not just on the next user turn.
  • MEMORY.md is auto-rebuilt from the topic file's frontmatter. In my main run the mock model had to spend a second write_file round-trip to create the index by hand; under this PR the index already existed by then, so that round-trip was skipped (6 requests vs 7). A real model that writes the index unconditionally wouldn't save the round-trip, but it also can no longer leave the index stale by forgetting step 2.

Here is the real TUI pane from that run:

cli

3. Path guard holds, and scope classification is correct

I ran two more live sessions against the PR bundle:

  • Control — pre-seed a stale MEMORY.md sentinel, then have the model write ./notes.md (outside the memory root). The sentinel survives, the index is never rebuilt, and the system instruction stays byte-stable. No spurious refresh on ordinary file writes.
  • User scope — write into ~/.qwen/memories/ instead. Only the user index refreshes; the project index correctly stays empty. classifyWrittenMemoryScope does the right thing on a live run, not just in a mock.

guard

4. Tests, non-vacuity, and static checks

All of the PR's suites pass on the merged state — 603 tests:

core  src/memory/refresh.test.ts                      5 passed
cli   src/ui/commands/rememberCommand.test.ts         5 passed
cli   src/ui/hooks/useGeminiStream.test.tsx         159 passed
cli   src/acp-integration/{acpAgent,session/Session}.test.ts   434 passed

More importantly, I checked the new tests are non-vacuous by running them against unmodified origin/main sources:

new test on origin/main
core/src/memory/refresh.test.ts suite cannot load — Cannot find module './refresh.js'
useGeminiStreamrefreshes managed-memory instructions after interactive memory file writes fails (spy never called)
Sessionrefreshes managed memory instructions after successful ACP tool writes fails (spy called 0 times)
acpAgentrefreshes live sessions after workspace memory remember fails (spy called 0 times)
rememberCommand (+6 lines) ⚠️ passes — see finding B

eslint --max-warnings 0 and prettier --check are clean on all five changed source files. npm run typecheck passes with 0 errors for both packages/core and packages/cli on Linux — the baseline typecheck failure noted in the PR description is a local/Windows environment artifact (stale workspace builds), not a real error. CI on the PR is green.


Findings (none blocking)

A. The PR description no longer matches the diff. It describes using the submit_prompt.onComplete hook in rememberCommand.ts, but the shipped diff doesn't touch rememberCommand.ts at all — the interactive path now hooks tool completion in useGeminiStream.ts:2943, via the new packages/core/src/memory/refresh.ts. The stated risk ("a refresh failure can make the remember request report failure even if the memory write itself succeeded") also no longer applies: refreshMemoryInstruction swallows both failures, and index rebuilds are individually .catch()-ed. Since the PR body becomes the merge record, please refresh it.

B. rememberCommand.test.ts (+6 lines) is vacuous. The two added expect(result.onComplete).toBe(undefined) assertions pass unchanged on origin/main (verified). They're leftovers from the abandoned onComplete design and assert the absence of a property nothing ever sets. Suggest deleting them.

C. Scope note — auto-memory extraction still leaves a stale instruction. #6487's bug 1 is "after saving new memory (via /remember or auto-memory extraction)". This PR fixes the /remember and ACP paths only. The extraction path (client.ts:1650MemoryManager.scheduleExtractextract.ts) runs once per user turn, on by default (enableManagedAutoMemory ?? true, config.ts:2043), writes memory files through a subagent, and rebuilds the indexes at extract.ts:188-191 — but nothing in that path calls refreshSystemInstruction(). Repo-wide, the only callers are the new refresh.ts:140, acpAgent.ts:6792/8091, and languageCommand.ts:149.

I confirmed the live half of this on the PR bundle: the extraction subagent ran mid-session (3 captured requests) and wrote an indexable memory file into the memory root, while the main session's system instruction stayed constant at 30294 chars through turn 2. I did not fully exercise the rebuild half — my mock subagent didn't return touchedTopics, so extract.ts skipped its own index rebuild; the stale-instruction conclusion doesn't depend on that. Reasonable as a follow-up alongside the compaction work, but worth saying out loud that Refs #6487 closes roughly half of bug 1.

D. Hardening nit — refreshMemoryAfterManagedWrite is not fully best-effort. The commit history (fix(memory): make ACP remember refresh best-effort, fix(memory): isolate interactive remember refresh failures) shows the intent is that a refresh failure can never break a tool batch. The inner steps honor that, but the entry guards do not: refresh.ts:153-162 calls config.isManagedMemoryAvailable(), config.getProjectRoot() and didWriteManagedMemory() (→ isAllowedMemoryPathgetUserAutoMemoryRoot()) outside any try. Both call sites await it unguarded, and in Session.ts:4310-4311 the call sits in a finally, where a throw would discard the return value and drop the whole batch's tool responses. I did not manage to make it throw, so this is defensive only — but a try { … } catch { return false; } around the body would make the guarantee unconditional and costs nothing.

E. Minor — refreshLiveSessionMemoryInstructions fans out to every live session. acpAgent.ts:4770 refreshes all sessions on any workspace-memory remember; each one re-reads hierarchical memory and calls toolRegistry.warmAll() inside refreshSystemInstruction. Correct (each session re-reads its own memory, so no cross-project bleed), but it's O(sessions) work per remember. Fine at today's session counts; just noting it.


Verdict

LGTM — recommend merge once the PR description is updated (A) and ideally the vacuous assertions are dropped (B). The core behavior change is correct, correctly scoped, index-rebuilding is a genuine bonus, and the guard against spurious refreshes is real. C is a follow-up, D is cheap insurance.

中文版报告

本地验证 —— Linux 上的真实 tmux 端到端测试 ✅

我在 Linux 上做了完整的端到端验证:在 tmux 里跑真实打包产物dist/cli.js),后端接一个会记录每次请求的 mock OpenAI 兼容服务。修复有效,main 上的 bug 能稳定复现,路径守卫也确实生效。 结论是可以合并,另有两个不阻塞的清理项和一个范围说明。

由于该分支落后 main 57 个提交,且 main 之后改动过其中 4 个相同文件,我测试的是合并后的状态origin/main + 本 PR = 61eab4d5c),而不是分支原始 HEAD。合并本身是干净的。

BASE origin/main @ e64010c11
PR origin/main + b5bda61c3(合并后,无冲突)
环境 Linux 6.12.63、Node v22.22.2、tmux 3.5a、真实 dist/cli.js

1. Bug 在 main 上确实复现

会话中执行 /remember always use tabs, never spaces。mock 模型严格按系统提示词要求的两步保存流程操作:先写 topic 文件,再往 MEMORY.md 里加一行指针。随后发送一个普通的第二轮用户消息,我检查 CLI 实际发到网络上的 system 消息。

main 上,记忆确实落盘了,但系统指令在所有主会话请求里逐字节相同(30274 字符),第二轮也不例外。模型收到的仍然是 Your MEMORY.md is currently empty. —— 这正是 #6487 的 bug 1。

2. 本 PR 修好了,而且是在同一轮内生效

有两点比 PR 描述里说的还要好:

  • 刷新发生在 call#2,也就是工具结果还没回传给模型之前。因此模型可以立刻用上新记忆,不必等到下一轮用户输入。
  • MEMORY.md 会依据 topic 文件的 frontmatter 自动重建。在我的 main 运行里,mock 模型不得不额外花一次 write_file 往返来手工创建索引;在本 PR 下索引此时已经存在,这次往返被跳过了(6 次请求 vs 7 次)。如果真实模型无条件写索引,则省不下这次往返 —— 但它也不再可能因为忘记第二步而留下过期索引。

3. 路径守卫有效,scope 分类正确

我又用 PR 产物跑了两个真实会话:

  • 对照组 —— 预先写入一个带哨兵值的过期 MEMORY.md,然后让模型写 ./notes.md(在 memory root 之外)。哨兵值原样保留,索引没有被重建,系统指令逐字节不变。普通文件写入不会触发多余刷新。
  • 用户级 scope —— 改为写入 ~/.qwen/memories/。只有 user 索引被刷新,project 索引正确地保持为空。classifyWrittenMemoryScope 在真实运行中的行为是对的,不只是 mock 里对。

4. 测试、非空洞性与静态检查

合并状态下 PR 的所有测试套件全部通过,共 603 个

core  src/memory/refresh.test.ts                      5 passed
cli   src/ui/commands/rememberCommand.test.ts         5 passed
cli   src/ui/hooks/useGeminiStream.test.tsx         159 passed
cli   src/acp-integration/{acpAgent,session/Session}.test.ts   434 passed

更重要的是,我把新测试放到未修改的 origin/main 源码上跑,验证它们不是空洞测试

新增测试 origin/main
core/src/memory/refresh.test.ts 套件无法加载 —— Cannot find module './refresh.js'
useGeminiStreamrefreshes managed-memory instructions after interactive memory file writes 失败(spy 从未被调用)
Sessionrefreshes managed memory instructions after successful ACP tool writes 失败(spy 调用 0 次)
acpAgentrefreshes live sessions after workspace memory remember 失败(spy 调用 0 次)
rememberCommand(+6 行) ⚠️ 通过 —— 见问题 B

五个改动源文件的 eslint --max-warnings 0prettier --check 均干净。npm run typecheck 在 Linux 上对 packages/corepackages/cli 都是 0 错误 —— PR 描述里提到的 baseline typecheck 失败是本地/Windows 环境问题(workspace 构建产物过期),不是真实错误。PR 的 CI 是绿的。

问题清单(均不阻塞合并)

A. PR 描述已经和 diff 对不上了。 描述里说用 rememberCommand.tssubmit_prompt.onComplete hook,但最终 diff 根本没碰 rememberCommand.ts —— 交互路径现在是在 useGeminiStream.ts:2943 挂到工具完成回调上,经由新增的 packages/core/src/memory/refresh.ts。描述里写的风险("refresh 失败可能导致 remember 请求报告失败,即使写入已成功")也已不适用:refreshMemoryInstruction 吞掉了两处失败,索引重建也各自 .catch() 了。PR 描述会成为合并记录,建议更新。

B. rememberCommand.test.ts 新增的 6 行是空洞断言。 两处 expect(result.onComplete).toBe(undefined)origin/main 上原样通过(已验证)。它们是被放弃的 onComplete 方案的遗留物,断言的是一个谁都不会设置的属性不存在。建议删除。

C. 范围说明 —— 自动记忆抽取路径的系统指令仍然是过期的。 #6487 的 bug 1 原文是*"保存新记忆后(通过 /remember 或自动记忆抽取)"*。本 PR 只修了 /remember 与 ACP 两条路径。抽取路径(client.ts:1650MemoryManager.scheduleExtractextract.ts每个用户轮次跑一次、且默认开启enableManagedAutoMemory ?? trueconfig.ts:2043),通过子代理写入记忆文件,并在 extract.ts:188-191 重建索引 —— 但这条路径上没有任何地方调用 refreshSystemInstruction()。全仓库的调用方只有新增的 refresh.ts:140acpAgent.ts:6792/8091,以及 languageCommand.ts:149

我在 PR 产物上确认了其中可观测的一半:抽取子代理确实在会话中途运行了(捕获到 3 次请求),并往 memory root 写入了一个可被索引的记忆文件,而主会话的系统指令直到第二轮都稳定在 30294 字符。我没有完整跑通重建那一半 —— 我的 mock 子代理没有返回 touchedTopics,因此 extract.ts 跳过了自己的索引重建;不过"系统指令过期"这个结论并不依赖那一步。作为后续跟进(和 compaction 一起)是合理的,但值得明说:Refs #6487 大约只关掉了 bug 1 的一半。

D. 加固建议 —— refreshMemoryAfterManagedWrite 并非完全 best-effort。 提交历史(fix(memory): make ACP remember refresh best-effortfix(memory): isolate interactive remember refresh failures)表明设计意图是"刷新失败绝不能破坏工具批次"。内部步骤做到了,但入口守卫没有:refresh.ts:153-162 在任何 try 之外调用了 config.isManagedMemoryAvailable()config.getProjectRoot()didWriteManagedMemory()(→ isAllowedMemoryPathgetUserAutoMemoryRoot())。两个调用点都是裸 await,而且在 Session.ts:4310-4311 里这个调用位于 finally 中 —— 一旦抛出,就会丢弃 return 值并吞掉整个批次的工具响应。我没能构造出实际抛出的场景,所以这纯属防御性建议;但在函数体外包一层 try { … } catch { return false; } 可以让这个保证变成无条件的,且没有任何代价。

E. 小问题 —— refreshLiveSessionMemoryInstructions 会扇出到所有活跃会话。 acpAgent.ts:4770 在任何一次 workspace memory remember 时刷新全部会话,每个会话都会重新加载分层记忆,并在 refreshSystemInstruction 内部调用 toolRegistry.warmAll()。行为是正确的(每个会话读自己的记忆,不会跨项目串味),但每次 remember 的开销是 O(会话数)。按当前的会话规模没问题,仅作记录。

结论

LGTM —— 建议合并,前提是更新 PR 描述(A),最好再删掉那两处空洞断言(B)。核心行为改动正确、范围收敛得当,索引自动重建是实打实的额外收益,防止误刷新的守卫也确实有效。C 属于后续跟进,D 是低成本的保险。

@han-dreamer

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed verification and summary.

I updated the PR description to match the current implementation and scope, including the same-turn refresh behavior, ACP live-session path, path guard, and the auto-memory extraction follow-up note.

I also removed the obsolete onComplete assertions from rememberCommand.test.ts and added the defensive best-effort guard around refreshMemoryAfterManagedWrite, so unexpected guard/classification failures return false instead of breaking the tool batch.

Re-ran the focused checks locally:

  • npx prettier --write packages/core/src/memory/refresh.ts packages/core/src/memory/refresh.test.ts packages/cli/src/ui/commands/rememberCommand.test.ts
  • git diff --check
  • npx eslint packages/core/src/memory/refresh.ts packages/core/src/memory/refresh.test.ts packages/cli/src/ui/commands/rememberCommand.test.ts --max-warnings 0
  • npm run test:ci --workspace=packages/core -- src/memory/refresh.test.ts
  • npm run test:ci --workspace=packages/cli -- src/ui/commands/rememberCommand.test.ts

wenshao
wenshao previously approved these changes Jul 9, 2026

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No review findings. Downgraded from Approve to Comment: CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x).

— qwen3.7-max via Qwen Code /review

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No blockers. The fix correctly addresses the stale memory instruction gap across all three integration paths. Suggestion-level recommendations are in the Suggestion summary comment below.

— qwen3.7-max via Qwen Code /review

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Conflict Resolution Summary — PR #6497

Branch: fix/remember-refresh-memorymain
Commit: fix(memory): refresh instructions after remember

Conflicted File

packages/cli/src/acp-integration/session/Session.test.ts

Conflicts and Resolutions

Conflict 1 — Spy declarations (line ~46)

Both the PR branch and main added independent vi.hoisted() spy declarations in the same location:

  • HEAD (PR): const refreshMemoryAfterManagedWriteSpy = vi.hoisted(() => vi.fn());
  • main: const transcribeVoiceAudioSpy = vi.hoisted(() => vi.fn());

Resolution: Kept both declarations. They are independent test spies for unrelated features (memory refresh vs. voice audio transcription).

Conflict 2 — beforeEach mock resets (line ~386)

Both sides added mockReset() calls in the beforeEach block at the same location:

  • HEAD (PR): refreshMemoryAfterManagedWriteSpy.mockReset() + .mockResolvedValue(false)
  • main: transcribeVoiceAudioSpy.mockReset()

Resolution: Kept both reset blocks. They correspond to the respective spy declarations and are both needed for test isolation.

Verification

  • No conflict markers remain in the file.
  • The final diff against origin/main shows only the PR's intended additions (refreshMemoryAfterManagedWriteSpy setup and a new test case for memory refresh after ACP tool writes) layered cleanly on top of main's transcribeVoiceAudioSpy changes.
  • No unrelated files were modified.

@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Addendum — re-verified on the conflict-resolved head 1d8bcd033

My round-2 comment was written against 0e94944c3; the branch was force-updated by /resolve right after. Re-ran everything on the head that will actually merge. Verdict unchanged: LGTM.

The resolution itself is correct. Only Session.test.ts conflicted, and both vi.hoisted() spies were kept (refreshMemoryAfterManagedWriteSpy and main's transcribeVoiceAudioSpy). No conflict markers survive anywhere, cb6fbe2bd (the hardening commit) is still an ancestor, refresh.ts still has its try/catch, and rememberCommand.test.ts is still out of the diff. Still 9 files, and refresh.ts / useGeminiStream.ts / acpAgent.ts / core/src/index.ts are byte-identical to what I verified in round 2.

The thing actually worth re-checking: main independently rewrote ~169 lines of Session.ts in the meantime, which is exactly where this PR threads memoryWriteCandidates through the tool loop. A new success-return path added by main would silently skip the refresh. So I audited every return { in runToolCalls / runTool on the merged code:

return site carries candidates why
Session.ts:5409 the one post-execution success return (status === 'success' ? [...])
:4395 :4407 :4428 :4437 :4447 batch-level returns propagating collected candidates
:5497 catch / error path — no successful write
:4056 :4096 :4508 pre-execution bail-outs (loop-detected batch skip, repeated-duplicate drop, loop-detected tool skip) — no tool ran

Wiring is intact; nothing bypasses finally { await refreshMemoryIfNeeded(); }.

Re-ran on the resolved head

core  src/memory/refresh.test.ts                                     6 passed
cli   src/acp-integration/{session/Session,acpAgent}.test.ts       455 passed
cli   src/ui/hooks/useGeminiStream.test.tsx + rememberCommand.test.ts  164 passed
                                                             total  625 passed

The conflicted test (Sessionrefreshes managed memory instructions after successful ACP tool writes) passes on its own too.

Rebuilt dist/cli.js from 1d8bcd033 and re-ran the live tmux sessions — identical to rounds 1 and 2:

  • /remember → project index enters the system instruction at call#2 (same turn, 30270 → 39955) and persists into turn 2.
  • control (./notes.md, outside the memory root) → STALE-INDEX-SENTINEL intact, system instruction byte-stable at 39926 across all three main requests.

eslint --max-warnings 0, prettier --check, and npm run typecheck (core + cli) all clean. PR CI is re-running on the new head; the local equivalents are green.

Nothing further from me — good to merge once CI goes green. Please keep #6487 open for the auto-memory-extraction and compaction halves.

中文版

补充 —— 已在解决冲突后的 HEAD 1d8bcd033 上重新验证 ✅

我第二轮的评论是针对 0e94944c3 写的,紧接着分支就被 /resolve 强制更新了。我在真正将要合并的 HEAD 上重跑了全部验证。结论不变:LGTM。

冲突解决本身是正确的。 只有 Session.test.ts 发生冲突,两个 vi.hoisted() spy 都被保留了(refreshMemoryAfterManagedWriteSpy 与 main 的 transcribeVoiceAudioSpy)。任何地方都没有残留冲突标记,cb6fbe2bd(加固提交)仍是祖先,refresh.tstry/catch 还在,rememberCommand.test.ts 依旧不在 diff 中。仍是 9 个文件,且 refresh.ts / useGeminiStream.ts / acpAgent.ts / core/src/index.ts 与我第二轮验证的版本逐字节一致。

真正需要复查的点: 这期间 main 独立重写了 Session.ts 约 169 行,而那里正是本 PR 把 memoryWriteCandidates 串进工具循环的位置。如果 main 新增了一条成功返回路径,刷新就会被静默跳过。因此我审计了合并后代码中 runToolCalls / runTool 里的每一处 return {

返回点 是否携带 candidates 原因
Session.ts:5409 唯一的执行后成功返回(status === 'success' ? [...]
:4395 :4407 :4428 :4437 :4447 批次级返回,透传已收集的 candidates
:5497 catch / 错误路径,不存在成功写入
:4056 :4096 :4508 执行前的提前返回(loop 检测跳过批次、重复调用丢弃、loop 检测跳过单个工具)—— 没有工具真正执行

接线完好,没有任何路径绕过 finally { await refreshMemoryIfNeeded(); }

在解决冲突后的 HEAD 上重跑

core  src/memory/refresh.test.ts                                     6 passed
cli   src/acp-integration/{session/Session,acpAgent}.test.ts       455 passed
cli   src/ui/hooks/useGeminiStream.test.tsx + rememberCommand.test.ts  164 passed
                                                                合计  625 passed

发生过冲突的那个测试(Sessionrefreshes managed memory instructions after successful ACP tool writes)单独跑也通过。

1d8bcd033 重新构建 dist/cli.js 并重跑真实 tmux 会话 —— 与第一、二轮完全一致:

  • /remember → 项目索引在 call#2 进入系统指令(同一轮内,30270 → 39955),并延续到第二轮。
  • 对照组(./notes.md,位于 memory root 之外)→ STALE-INDEX-SENTINEL 原样保留,三次主请求的系统指令逐字节稳定在 39926

eslint --max-warnings 0prettier --checknpm run typecheck(core + cli)全部干净。PR CI 正在新 HEAD 上重跑,本地等价检查均为绿。

我这边没有其他问题了 —— CI 转绿即可合并。请让 #6487 保持 open,用于跟进 auto-memory extraction 与 compaction 这两半。

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No blockers. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). This is a focused, well-structured fix that correctly addresses the stale memory instruction gap across all three integration paths. The Session.ts try/finally refactoring preserves existing behavior, error isolation is correct, and tests cover the key scenarios. Low-confidence suggestions (status type narrowing, test coverage for early-return paths, JSDoc on exported functions) are noted for human review.

— qwen3.7-max via Qwen Code /review

wenshao
wenshao previously approved these changes Jul 9, 2026
@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestions are inline.

Comment thread packages/core/src/memory/refresh.ts Outdated
}

export interface RefreshMemoryAfterWriteOptions {
rebuildIndexes?: boolean;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] rebuildIndexes option is declared but never set by any caller — every call site falls through to the default true. Unless there is a concrete planned use case for skipping index rebuilds, removing this option keeps the API surface minimal and avoids dead configurability.

Suggested change
rebuildIndexes?: boolean;
export interface RefreshMemoryAfterWriteOptions {
logContext?: string;
}

— qwen3.7-max via Qwen Code /review

config: Config,
options?: Pick<RefreshMemoryAfterWriteOptions, 'logContext'>,
): Promise<void> {
try {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The two independent try/catch blocks here guarantee that refreshSystemInstruction runs even if refreshHierarchicalMemory throws — but no test in refresh.test.ts covers this specific resilience path. The existing "keeps refreshing when index rebuild fails" test covers index-rebuild failure (a different code path inside rebuildWrittenMemoryIndexes). A test should configure config.refreshHierarchicalMemory to reject, then assert config.getGeminiClient().refreshSystemInstruction was still called.

— qwen3.7-max via Qwen Code /review

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@wenshao

wenshao commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge Conflict Resolution Summary — PR #6497

Base branch

main

Conflicts

1. packages/cli/src/acp-integration/acpAgent.ts (import block)

  • PR side (HEAD): Added refreshMemoryInstruction to the value imports from @qwen-code/qwen-code-core.
  • Main side: Added 22 new type imports (AgentParams, ApprovalMode, ChatRecord, Config, etc.) from the same module — a refactor that extracted inline types to explicit type imports.
  • Resolution: Kept both. refreshMemoryInstruction remains as a value import, followed by all 22 type imports from main. Both are independent additions to the same import statement.

2. packages/cli/src/acp-integration/session/Session.ts (import block)

  • PR side (HEAD): Added MemoryWriteCandidate and SubSessionSpawner to the type imports from @qwen-code/qwen-code-core.
  • Main side: Did not add these types (main's version of this import block didn't include them).
  • Resolution: Kept the PR's additions. Both types are actively used throughout Session.ts (20 references to MemoryWriteCandidate/SubSessionSpawner in the file body), so they are required for the PR's feature to compile.

Commit

merge: resolve conflicts with origin/main (import unions) — single conventional commit on qwen-resolve/pr-6497.

ToolArtifact,
VisionBridgeResult,
MemoryWriteCandidate,
SubSessionSpawner,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Unused type import breaks tsc --build.

noUnusedLocals is enabled in tsconfig, and SubSessionSpawner is imported but never used as a type annotation in this file. The call sites (this.config.setSubSessionSpawner(...)) don't require the type to be imported — TypeScript infers the callback parameter type from the method signature.

Suggested change
SubSessionSpawner,
MemoryWriteCandidate,
} from '@qwen-code/qwen-code-core';

— qwen3.7-max via Qwen Code /review

!t.request.isClientInitiated &&
!historyCallIdsWithResponse.has(t.request.callId),
);
await refreshMemoryAfterManagedWrite(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Race condition between concurrent memory refresh paths.

When a save_memory tool and a managed-memory write_file/edit both succeed in the same completedAndReadyToSubmitTools batch:

  1. Line 2930: void performMemoryRefresh() fires as fire-and-forget (un-awaited)
  2. Line 2943: await refreshMemoryAfterManagedWrite(...) blocks

performMemoryRefresh reads memory from disk before the index rebuild, then calls config.setUserMemory(content). If it finishes after the awaited refresh has already pushed a fresh system instruction, it overwrites config.userMemory with stale content — the next turn's system instruction rebuild reads stale memory.

Fix: either await both paths consistently, or guard one when the other fires (e.g., skip performMemoryRefresh when refreshMemoryAfterManagedWrite will run for the same batch).

— qwen3.7-max via Qwen Code /review

if (!didRefreshManagedMemory) {
// Perform the legacy save_memory refresh only when the managed-memory
// write refresh did not already rebuild and publish a fresher state.
void performMemoryRefresh().catch((err) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Test failure: the mock performMemoryRefresh in useGeminiStream.test.tsx is vi.fn() (returns undefined), but this code calls .catch() on the return value, causing TypeError: Cannot read properties of undefined (reading 'catch').

Fix the test mock to return a resolved promise:

Suggested change
void performMemoryRefresh().catch((err) => {
void performMemoryRefresh()?.catch((err) => {

— qwen3.7-max via Qwen Code /review

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 11, 2026
Merged via the queue into QwenLM:main with commit 2b9c92e Jul 11, 2026
39 checks passed
@han-dreamer
han-dreamer deleted the fix/remember-refresh-memory branch July 11, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants