Skip to content

feat(memory): add structured on-demand recall - #10178

Closed
ZijianZhang989 wants to merge 1 commit into
QwenLM:mainfrom
ZijianZhang989:codex/model-pull-memory-from-main
Closed

feat(memory): add structured on-demand recall#10178
ZijianZhang989 wants to merge 1 commit into
QwenLM:mainfrom
ZijianZhang989:codex/model-pull-memory-from-main

Conversation

@ZijianZhang989

Copy link
Copy Markdown
Collaborator

What this PR does

This PR evolves managed auto-memory from a flat, body-heavy prompt into a structured push/pull recall protocol. The model receives a complete two-level ref/title tree when the memory corpus changes, a query-focused metadata subtree on relevant turns, and a dedicated tool for exact fetch, keyword or phrase search, and category exploration. Memory bodies are loaded in bounded windows only when metadata is insufficient, with signed cursor continuation and duplicate-window suppression.

Existing memory remains usable throughout migration. Legacy files continue through the existing recall path while a best-effort background migration adds validated frontmatter atomically; the runtime switches to structured recall only after every visible enabled scope is ready, and rolls back the protocol if activation cannot complete safely. Remember, extraction, project dream, and user dream reuse a dynamic canonical keyword vocabulary while preserving the original memory body.

The change also tracks whether fetched body versions are still resident in conversation history. Unchanged resident bodies are not injected again, compacted bodies can be fetched again, and modified files are treated as new versions. Telemetry separates scan, fast selection, model selection, delivery, search, migration, and protocol-transition timing.

Direct file and shell access to managed memory is restricted while structured recall is active so the tool remains the single audited body-loading path. Glob protection covers managed-memory descendants, ancestor dot-globs, brace expansion, and early Node 22 releases without blocking ordinary non-dot globs.

Why it's needed

Flat memory indexes consume prompt tokens even when most entries are irrelevant, provide little hierarchy over a growing corpus, and make precise per-turn recall difficult. Model-only asynchronous selection can also miss the first request or fail independently, while injecting complete bodies eagerly increases latency and context cost.

The structured protocol keeps a stable global map, adds a small query-specific focus layer, and lets the model pull only the body windows needed for the task. The migration gate preserves existing users' memories and recall behavior until the new metadata contract is complete rather than requiring a destructive one-time conversion.

Reviewer Test Plan

How to verify

  1. Start with legacy project and user memory files that do not contain structured frontmatter. Confirm they remain available through legacy recall while background migration runs, their original body bytes are preserved, and structured mode activates only after all enabled visible scopes are ready.
  2. In structured mode, ask two unrelated questions. Confirm the complete ref/title tree is delivered once, each question receives only its focused subtree, and the complete tree is delivered again only after memory metadata changes or the conversation is compacted.
  3. Fetch a memory body twice without compaction and confirm the second result reports that the body is already available. Compact the conversation and confirm the same body can be fetched again. Modify the memory file and confirm the new version can be read even if the old version was resident.
  4. Exercise exact fetch, multi-keyword search, category exploration, cursor continuation, invalid cursors, partial refs, repeated requests, and aggregate body limits. Confirm results remain bounded and copyable refs retain their scope prefix.
  5. In structured mode, confirm direct read, write, list, glob, and shell access to managed-memory paths is denied. Confirm brace-expanded paths and recursive dot-globs are denied, while ordinary patterns such as ./*, ~/*, and {src,docs}/README.md remain usable.
  6. Run the focused Core suites: cd packages/core && npx vitest run src/memory/tree.test.ts src/memory/recall.test.ts src/memory/search-memory.test.ts src/memory/manager.test.ts src/tools/search-memory.test.ts src/core/client.test.ts src/config/config.test.ts src/memory/metadata-migration.test.ts src/memory/user-dream.test.ts src/memory/writer-keyword-vocabulary.test.ts.
  7. Run the complete shell suite on the normal development runtime and Node 22.4.1: cd packages/core && npx vitest run src/tools/shell.test.ts and npx -y node@22.4.1 ../../node_modules/vitest/vitest.mjs run src/tools/shell.test.ts.
  8. Run Core type checking and lint for the changed surfaces.

Evidence (Before & After)

Before: managed memory is exposed primarily through a flat index and selector-selected prompt content, with no audited on-demand body protocol or automatic compatibility transition for legacy metadata.

After: the model receives a revisioned complete tree plus per-turn focused subtrees, reads bounded bodies through the memory tool, avoids duplicate resident content, and transitions from legacy recall only after atomic background metadata migration is complete.

Local verification completed: 1,125 focused Core tests passed; the shell suite passed 331/331 on the development runtime and 331/331 on Node 22.4.1; Core TypeScript and ESLint checks passed.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS, local TypeScript workspace, Node 26.2.0 and Node 22.4.1 compatibility smoke run.

Risk & Scope

  • Main risk or tradeoff: structured recall adds a metadata migration and protocol transition around a mature memory lifecycle; conservative guards intentionally deny ambiguous shell glob patterns that could traverse managed-memory directories.
  • Not validated / out of scope: Windows and Linux were not tested locally; one-shot headless sessions keep best-effort migration semantics and may exit before a migration batch finishes; the existing model-selector manifest budget is unchanged; production tuning of the initial delivery budget remains tracked separately.
  • Breaking changes / migration notes: no manual migration is required. Legacy recall remains active until the visible corpus is fully structured, memory body content is preserved during frontmatter updates, and failed or cancelled migrations are retried on later turns.

Linked Issues

Related to #8998.

中文说明

本 PR 做了什么

本 PR 将托管自动记忆从平铺、正文占比较高的提示上下文升级为结构化的 push/pull 召回协议。记忆语料发生变化时,模型会收到完整的两层 ref/title 主题树;相关轮次会收到面向当前问题的 metadata 子树;同时提供专用工具执行精确 fetch、关键词或短语 search 以及分类 explore。只有 metadata 不足时才会按有界窗口读取记忆正文,并支持签名 cursor 续读和重复窗口抑制。

迁移期间已有记忆始终可用。旧格式文件在 best-effort 后台迁移以原子方式补充经过校验的 frontmatter 时,继续使用原有召回链路;只有所有可见且已启用的 scope 全部就绪后,runtime 才切换到结构化召回;如果协议激活无法安全完成,则回滚到原协议。Remember、Extraction、项目 Dream 和用户 Dream 会复用动态 canonical keyword vocabulary,同时保留原始记忆正文。

本改动还会跟踪已 fetch 的正文版本是否仍存在于对话历史中。未变化且仍驻留的正文不会重复注入;被压缩移除的正文可以重新 fetch;记忆文件发生修改后会被视为新版本。Telemetry 分别记录 scan、fast selection、model selection、delivery、search、migration 和协议切换的耗时。

结构化召回启用时,系统会限制通过普通文件工具和 shell 直接访问托管记忆,使专用工具成为唯一可审计的正文加载路径。Glob 防护覆盖记忆目录内部路径、能够命中祖先目录的 dot-glob、brace expansion 和早期 Node 22 版本,同时不会阻止普通非 dot glob。

为什么需要

平铺记忆索引会在大部分条目与当前问题无关时仍消耗提示 Token,难以表达不断增长语料的层次,也不利于每轮精确召回。仅依赖异步模型 selector 还可能错过首个请求或独立失败,而提前注入完整正文会增加延迟和上下文成本。

结构化协议保留稳定的全局地图,增加较小的当前问题聚焦层,并允许模型只拉取完成任务所需的正文窗口。迁移门禁会在新 metadata contract 完整之前保留已有用户的记忆和召回行为,不要求进行破坏性的一次性转换。

审阅者测试计划

如何验证

  1. 准备不含结构化 frontmatter 的旧项目记忆和用户记忆。确认后台迁移期间仍可通过旧链路召回,原始正文字节保持不变,并且只有所有已启用的可见 scope 就绪后才激活结构化模式。
  2. 在结构化模式下询问两个不相关的问题。确认完整 ref/title 树只交付一次,每个问题只收到自己的 focused subtree,并且只有记忆 metadata 变化或对话压缩后才再次交付完整树。
  3. 在未压缩的情况下连续 fetch 同一记忆正文,确认第二次返回正文已经可用。压缩对话后确认可以再次 fetch。修改记忆文件后,确认旧版本仍驻留时也可以读取新版本。
  4. 验证精确 fetch、多关键词 search、分类 explore、cursor 续读、非法 cursor、不完整 ref、重复请求和正文总预算。确认结果保持有界,并且可复制 ref 保留 scope 前缀。
  5. 在结构化模式下确认普通 read、write、list、glob 和 shell 无法直接访问托管记忆路径。确认 brace 展开路径和递归 dot-glob 被拒绝,同时 ./*~/*{src,docs}/README.md 等普通模式仍可使用。
  6. 运行聚焦 Core 测试:cd packages/core && npx vitest run src/memory/tree.test.ts src/memory/recall.test.ts src/memory/search-memory.test.ts src/memory/manager.test.ts src/tools/search-memory.test.ts src/core/client.test.ts src/config/config.test.ts src/memory/metadata-migration.test.ts src/memory/user-dream.test.ts src/memory/writer-keyword-vocabulary.test.ts
  7. 在正常开发运行时和 Node 22.4.1 下运行完整 shell 测试:cd packages/core && npx vitest run src/tools/shell.test.ts,以及 npx -y node@22.4.1 ../../node_modules/vitest/vitest.mjs run src/tools/shell.test.ts
  8. 对相关改动运行 Core typecheck 和 lint。

证据(修改前与修改后)

修改前:托管记忆主要通过平铺索引和 selector 选中的提示内容暴露,没有经过审计的按需正文协议,也没有兼容旧 metadata 的自动切换过程。

修改后:模型会收到带 revision 的完整树和每轮 focused subtree,通过记忆工具读取有界正文,避免重复驻留内容,并且只有原子后台 metadata 迁移完成后才从旧召回切换。

本地验证结果:1,125 条聚焦 Core 测试通过;shell 测试在开发运行时下 331/331 通过,在 Node 22.4.1 下同样 331/331 通过;Core TypeScript 和 ESLint 检查通过。

测试平台

系统 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS,本地 TypeScript workspace,Node 26.2.0,并使用 Node 22.4.1 做兼容性冒烟测试。

风险与范围

  • 主要风险或取舍:结构化召回在成熟记忆生命周期外围增加了 metadata 迁移和协议切换;保守的安全门禁会有意拒绝可能遍历托管记忆目录的歧义 shell glob。
  • 未验证或不在范围内:Windows 和 Linux 未在本地测试;单轮 headless 会话继续采用 best-effort 迁移语义,可能在一个迁移 batch 完成前退出;现有模型 selector manifest 预算未改变;首轮交付预算的生产调优仍由单独 issue 跟踪。
  • 破坏性变更或迁移说明:不需要手工迁移。可见语料完全结构化之前继续使用旧召回,frontmatter 更新会保留记忆正文,失败或取消的迁移会在后续轮次重试。

关联 Issue

关联 #8998

@ZijianZhang989
ZijianZhang989 force-pushed the codex/model-pull-memory-from-main branch 2 times, most recently from fe89220 to 2bc4533 Compare August 26, 2026 16:18
@ZijianZhang989
ZijianZhang989 force-pushed the codex/model-pull-memory-from-main branch from 2bc4533 to cf48eb7 Compare August 26, 2026 16:19
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

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

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

@ZijianZhang989

Copy link
Copy Markdown
Collaborator Author

Superseded by #10183, which uses an in-repository head branch so the standard pull_request CI can run without fork-workflow approval.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @ZijianZhang989!

  • Template: complete ✓ — all required sections present, bilingual body included.

  • Problem: This is a feature, so no reproduction is expected — but the stated problem is design reasoning, not measured evidence. "Flat memory indexes consume prompt tokens even when most entries are irrelevant" is plausible, yet the repo's own established plan for exactly this area was to measure first: issue Validate the recall initial-turn budget against production delivery telemetry #8998 exists to decide from production delivery telemetry whether a follow-up recall change is justified, and the PR body itself notes that production tuning "remains tracked separately." So today there is no data showing the current protocol is actually failing users. What production signal motivated replacing the protocol now, rather than landing Validate the recall initial-turn budget against production delivery telemetry #8998's measurement first?

  • Direction: Memory recall efficiency is genuinely an active area here (RFC RFC: Reliable auto-memory recall — timing, quality, and telemetry #7040, fix(memory): improve recall reliability and candidate coverage #8716, and feat(core): add memory recall delivery telemetry #7393 — which you authored). The claude-code CHANGELOG has no direct reference to a structured push/pull recall protocol, but the area is relevant. Two concerns before endorsing the direction: (1) this PR pre-empts Validate the recall initial-turn budget against production delivery telemetry #8998's measure-first plan with a wholesale protocol replacement; (2) it touches telemetry contracts and tool/shell-glob denial semantics, which are public-contract surface and need maintainer eyes either way.

  • Size: Core paths touched. 6,860 production logic lines (47 files) vs 6,288 test lines (41 files), no generated/schema files. feat-type PRs are not hard-blocked on size, but at 500+ production lines this is flagged for maintainer awareness, and at 1,000+ the standing advice applies: if any part of this can be split into independently reviewable PRs, please consider it — a PR this size is very hard to review, revert, and reason about.

  • Approach: Honest question before the code review — if 80% of this scope were cut, would the remaining 20% already solve the problem? The core value looks like: a bounded-window on-demand fetch/search tool, plus delivering metadata instead of full bodies. Around that sits a lot of machinery that each carries its own risk and could plausibly be follow-ups: the background migration protocol with rollback, signed cursor continuation, body-residency tracking across compaction, the full shell/glob denial surface for managed memory, and the telemetry split. In particular, denying direct file/shell access to managed memory changes what users and scripts can do with their own files — that deserves its own discussion and PR. Have you considered landing the fetch/search tool on top of the existing recall path first?

  • Risk: packages/core/src/tools/shell.ts matches a high-risk path from this repo's revert-history analysis (one of the paths most correlated with post-merge reverts). That means: full review depth, no skipped enrichments, and CI evidence is required before anyone approves. Note that since the author lacks write access, @qwen-code /verify remains available as a sponsored run — a maintainer's comment approves the head it was written against, and that run carries a pre-execution risk screen and a full workspace wipe; the resulting report should still be read with the same skepticism as the fork's own CI logs.

Escalating to maintainer awareness per the core-module two-tier rule (feat-type, 500+ production lines touching packages/core/src/**), and carrying the shell.ts high-risk flag into code review. Moving on to code review. 🔍

中文说明

感谢贡献,@ZijianZhang989

  • **模板:**完整 ✓ —— 所有必需小节齐全,包含双语正文。

  • 问题:这是功能型 PR,不要求复现——但所述问题是设计推理,而非实测证据。"平铺记忆索引在大部分条目不相关时仍消耗提示 Token"听起来合理,但本仓库对该领域既定的计划是先测量:issue Validate the recall initial-turn budget against production delivery telemetry #8998 的存在就是为了依据生产交付遥测来判定是否有理由做后续召回变更,而且 PR 正文自己也承认生产调优"仍由单独 issue 跟踪"。也就是说,目前没有任何数据表明现有协议正在实际影响用户。是什么生产信号促使现在替换协议,而不是先完成 Validate the recall initial-turn budget against production delivery telemetry #8998 的测量?

  • **方向:**记忆召回效率确实是这里的活跃领域(RFC RFC: Reliable auto-memory recall — timing, quality, and telemetry #7040fix(memory): improve recall reliability and candidate coverage #8716feat(core): add memory recall delivery telemetry #7393——后者正是你所贡献的)。claude-code CHANGELOG 中没有结构化 push/pull 召回协议的直接参照,但该领域本身是相关的。在认可方向之前有两点顾虑:(1) 本 PR 用整体协议替换抢先于 Validate the recall initial-turn budget against production delivery telemetry #8998 的"先测量"计划;(2) 它触及遥测契约和工具/shell-glob 拒绝语义,这些属于公共契约面,无论如何都需要维护者审视。

  • 规模:触及核心路径。生产逻辑 6,860 行(47 个文件)对比测试 6,288 行(41 个文件),无生成/schema 文件。feat 类型 PR 不因规模被硬性阻止,但超过 500 行生产代码需标记给维护者知悉;超过 1,000 行的常规建议同样适用:如果其中任何部分可以拆成可独立评审的 PR,请考虑——这种规模的 PR 非常难以评审、回滚和推理。

  • 方案:代码审查之前的诚实问题——如果砍掉 80% 的范围,剩下的 20% 是否已能解决问题?核心价值看起来是:有界窗口的按需 fetch/search 工具,加上以 metadata 替代完整正文交付。围绕这些还有大量机制,每一项都自带风险,完全可能作为后续 PR:带回滚的后台迁移协议、签名 cursor 续读、跨压缩的正文驻留跟踪、针对托管记忆的完整 shell/glob 拒绝面,以及遥测拆分。特别是拒绝对托管记忆的直接文件/shell 访问,改变了用户和脚本对自己文件能做的事——它值得单独讨论并单开 PR。有没有考虑过先在现有召回链路之上落地 fetch/search 工具?

  • 风险:packages/core/src/tools/shell.ts 命中本仓库回滚历史分析中的高风险路径(与合并后回滚相关性最高的路径之一)。这意味着:完整审查深度、不跳过任何增强项、批准前必须有 CI 证据。注意:由于作者无写权限,@qwen-code /verify 仍可作为sponsored run 使用——维护者的评论会批准其撰写时所对应的 head,该运行带有执行前风险筛查和完整工作区清理;但产出的报告仍应以审视 fork 自身 CI 日志的同等怀疑态度阅读。

依据核心模块两档规则(feat 类型、触及 packages/core/src/** 且生产代码 500+ 行)升级至维护者知悉,并将 shell.ts 高风险标记带入代码审查。进入代码审查。🔍

Qwen Code · qwen3.8-max

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

@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Read-only static review of the diff at the commit below — no PR code was built or executed, per triage rules. Test evidence in this comment comes from the PR's own CI via the API (see the CI section: the PR's CI has not run on this commit at all), plus the author's self-reported local results, which are the author's claim and not independently verified here.

First, what's good: this is careful engineering. The cursor machinery is HMAC-signed with timing-safe comparison and invalidated on file mtime change; body windows and aggregate budgets are bounded; migration writes go through atomicWriteFile; the legacy→structured transition has an explicit prepare/confirm/commit path with rollback and telemetry on every branch; delivery telemetry is deferred until the stream confirms the prompt actually landed; and isManagedMemoryPath resolves symlinks via the nearest existing ancestor, so the path-based denials can't be escaped with a symlink. Test coverage is extensive — 6,288 added test lines across 41 files.

Concerns, in rough order of importance — these are for discussion, not blockers:

  1. The shell lockdown is guidance, not enforcement. isManagedMemoryShellAccess is a best-effort static tokenizer: segments that fail to tokenise are skipped, tokens carrying $VAR indirection are skipped, and runtime path construction ($(...), eval, python -c, xargs, base64-decoded scripts) is invisible to it by design. So the PR description's claim that the tool "remains the single audited body-loading path" overstates the guarantee — the layer steers a well-behaved model, it doesn't enforce anything against intent. That may be fine as defense-in-depth, but the description should say what it actually does, and maintainers should decide whether steering is the right contract here.

  2. No opt-out switch. There is no user-facing setting: once background migration marks all visible scopes ready, structured mode — and with it the denial of direct read/write/ls/glob/shell access to .qwen/memory/**, ~/.qwen/memories/**, and team memory — activates automatically. Users or scripts that read those files directly (this memory system's files are plain markdown by design) get no escape hatch. For a change that denies file access, a config flag to pin legacy mode seems warranted, at least initially.

  3. Migration spends model calls. generateMemoryMetadataWithAgent makes an LLM call per legacy memory file to synthesize frontmatter. It's best-effort and backgrounded, but on a large corpus that is real cost and latency the user didn't ask for; worth documenting prominently.

  4. The scope question stands (from the gate). An incremental version — a bounded fetch/search tool on top of the existing recall path — plausibly delivers most of the token benefit at ~1/10 the size. The migration protocol, residency tracking, whole-tool lockdown surface, and telemetry split could each have been follow-up PRs, each reviewable on its own.

Reuse check: the new engine builds on the existing scan.ts snapshot and atomicWriteFile rather than forking parallel utilities — good. picomatch was already a packages/core dependency; nothing new added.

sequenceDiagram
    participant P1 as User
    participant P2 as GeminiClient turn loop
    participant P3 as MemoryManager recall
    participant P4 as Tree renderer
    participant P5 as search_memory tool
    participant P6 as History residency map
    P1->>P2: sends a query
    P2->>P3: begin recall prefetch
    P3-->>P2: selected docs plus focused subtree
    P2->>P4: render focused subtree
    P4-->>P2: metadata prompt
    Note over P2: full tree added only when the revision changed
    P2-->>P1: answer streams back
    P2->>P5: model calls fetch or search when metadata is not enough
    P5->>P6: check body version already resident
    P6-->>P5: resident, evicted, or changed
    P5-->>P2: bounded body window with signed cursor
Loading
Files changed (28 of 88 shown)
File What changed
packages/core/src/memory/search-memory.ts New engine core: fetch, search, explore modes with bounded body windows and HMAC-signed cursors
packages/core/src/memory/search-memory.test.ts Exhaustive suite for the engine; largest single file in the PR
packages/core/src/memory/manager.ts Orchestrates recall mode, migration scheduling, and body-residency state
packages/core/src/memory/metadata-migration.ts Background legacy-to-frontmatter migration with atomic writes and agent-generated metadata
packages/core/src/memory/metadata-migration.test.ts Migration coverage including conflict and rollback cases
packages/core/src/memory/scan.ts Snapshot scanning shared by legacy and structured paths
packages/core/src/memory/recall.ts Splits legacy prompt building from the structured focused-subtree path
packages/core/src/memory/tree.ts Two-level ref-title tree and focused-subtree renderer
packages/core/src/core/client.ts Delivery orchestration: tree-revision dedup, transition commit and rollback, eviction hooks
packages/core/src/core/client.test.ts Delivery and transition coverage for the client changes
packages/core/src/config/config.ts Recall-mode state machine with prepare, confirm, commit, rollback
packages/core/src/tools/search-memory.ts New model-facing tool exposing fetch, search, explore
packages/core/src/tools/manage-memory.ts Remember and forget maintenance tool for structured mode
packages/core/src/tools/shell.ts Static tokeniser denying shell arguments that can reach managed memory
packages/core/src/tools/glob.ts Filters managed-memory entries from results in structured mode
packages/core/src/tools/ls.ts Denies listing managed-memory directories; schema hint in structured mode
packages/core/src/tools/read-file.ts Denies direct reads of managed-memory bodies
packages/core/src/tools/write-file.ts Denies writes into managed memory
packages/core/src/tools/edit.ts Denies edits inside managed memory
packages/core/src/tools/readManyFiles.ts Hides managed memory from multi-file reads
packages/core/src/tools/tool-registry.ts Registers the two new tools
packages/core/src/memory/prompt.ts Structured main-model prompt contract alongside the legacy one
packages/core/src/memory/paths.ts Adds roots to the managed set plus user metadata and lock paths
packages/core/src/memory/types.ts Tree categories and scope types
packages/core/src/memory/writer-keyword-vocabulary.ts Canonical keyword vocabulary shared by remember, extract, and dream
packages/core/src/services/microcompaction/microcompact.ts Collects and reports resident memory bodies across compaction
packages/core/src/telemetry/types.ts Mode-transition event and new delivery fields
packages/cli/src/ui/commands/memoryCommand.ts CLI memory command surface adjustments
…and 60 more files

Testing

Evidence carried by this run: API-fetched CI signal only. No PR code was built, run, or tested by triage (unattended CI runs never execute PR code).

The PR's CI has not run on the reviewed commit: the API shows zero check-runs and zero pull_request-event workflow runs for it (only the pull_request_target triage orchestration is active — that job does not build or test). For a fork PR this typically means the CI workflows are still awaiting maintainer approval to run. Nothing below should be read as "tests pass".

Check Conclusion
no check-runs registered on this commit
no pull_request workflow runs for this PR
  • Not verified: all runtime behaviour — CI has not executed on this commit.
  • Not verified: Windows and Linux behaviour — the author's "Tested on" table marks both ⚠️ not tested (author's claim, macOS only).
  • Not verified: the author's reported numbers (1,125 focused Core tests, shell suite 331/331 on two runtimes, typecheck/lint green) — self-reported in the PR body, not independently re-run.
  • The finalize workflow will rewrite the table above in place once CI settles on this commit.

Sandboxed verification would settle the behavioural claims: @qwen-code /verify — available as a sponsored run since the author lacks write access (a maintainer's comment approves the head it was written against; the run carries a pre-execution risk screen and a full workspace wipe, and its report should be read with the same skepticism as the fork's own CI logs). Two claims currently rest on nothing but the diff and the author's word: (1) structured delivery actually saves prompt tokens without dropping recall quality the legacy path provided, and (2) the shell/glob denial layer blocks managed-memory access without false-positives on legitimate patterns — the latter untested off macOS entirely. A maintainer can also trigger the isolated @qwen-code /tmux job for TUI-surface evidence, though /verify is the lane that fits this PR's claims.

中文说明

代码审查

仅对下列 commit 的 diff 做静态只读审查——按审查规则,未构建或执行任何 PR 代码。本评论中的测试证据来自 PR 自身的 CI(经 API 获取,见 CI 部分:该 PR 的 CI 尚未在此 commit 上运行过);作者自报的本地结果属于作者声明,未经独立验证。

先说优点:这是细致的工程。游标机制使用 HMAC 签名、时序安全比较,并在文件 mtime 变化时失效;正文窗口与总预算均有界;迁移写入走 atomicWriteFile;legacy→structured 切换具备明确的 prepare/confirm/commit 路径,带回滚且每个分支都有遥测;交付遥测推迟到流确认提示确实注入后才记录;isManagedMemoryPath 通过最近存在祖先解析符号链接,基于路径的拒绝无法用符号链接绕过。测试覆盖充分——41 个文件共新增 6,288 行测试。

顾虑按重要性大致排序——供讨论,不构成阻塞:

  1. Shell 封锁是引导而非强制。 isManagedMemoryShellAccess 是尽力而为的静态分词器:分词失败的片段被跳过,带 $VAR 间接引用的 token 被跳过,运行时构造路径($(...)evalpython -cxargs、base64 解码脚本)按其设计不可见。因此 PR 描述中"该工具保持唯一可审计的正文加载路径"夸大了保证——这一层引导行为良好的模型,对恶意意图不设防。作为纵深防御也许可以接受,但描述应如实说明其实际作用,由维护者判断"引导"是否是这里合适的契约。

  2. 没有退出开关。 没有面向用户的配置项:后台迁移将全部可见 scope 标记就绪后,结构化模式——连同对 .qwen/memory/**~/.qwen/memories/** 与团队记忆的直接 read/write/ls/glob/shell 拒绝——自动生效。直接读取这些文件的用户或脚本(该记忆系统的文件本就是纯 markdown)没有任何逃生通道。对于一个拒绝文件访问的变更,至少初期应提供一个固定使用 legacy 模式的配置开关。

  3. 迁移消耗模型调用。 generateMemoryMetadataWithAgent 会为每个旧格式记忆文件发起一次 LLM 调用以合成 frontmatter。虽是 best-effort 且在后台,但大语料下这是用户并未请求的真实成本与延迟,值得显著说明。

  4. 范围问题依旧成立(见 Gate 评论)。 增量版本——在现有召回链路之上做有界 fetch/search 工具——很可能以约 1/10 的规模交付大部分 Token 收益。迁移协议、驻留跟踪、整工具面的封锁、遥测拆分本可以各自作为后续 PR,逐个独立评审。

复用检查:新引擎基于现有 scan.ts 快照与 atomicWriteFile 构建,没有另起平行工具——好。picomatch 本就是 packages/core 的依赖,未新增。

序列图:用户提问 → GeminiClient 发起召回预取 → 交付 focused subtree(仅当树 revision 变化时附完整树)→ 模型在 metadata 不足时调用 search_memory → 工具核对历史驻留状态后返回带签名游标的有界正文窗口。

测试

本次运行携带的证据:仅 API 获取的 CI 信号。 审查未构建、运行或测试任何 PR 代码(无人值守 CI 运行从不执行 PR 代码)。

该 PR 的 CI 尚未在被审 commit 上运行:API 显示该 commit 上没有任何 check-run,也没有任何 pull_request 事件的 workflow run(唯一活跃的是 pull_request_target 审查编排任务——它不构建也不测试)。对 fork PR 而言,这通常意味着 CI 工作流仍在等待维护者批准运行。下文任何内容都不应读作"测试通过"。

  • 未验证:所有运行时行为——CI 尚未在该 commit 上执行。
  • 未验证:Windows 与 Linux 行为——作者的"测试平台"表将两者标为 ⚠️ 未测试(作者声明,仅 macOS)。
  • 未验证:作者自报的数字(1,125 条聚焦 Core 测试、shell 套件在两种运行时 331/331、typecheck/lint 通过)——来自 PR 正文自述,未独立复跑。
  • CI 在该 commit 上落定后,finalize 工作流会就地重写上方的表格。

沙箱验证可以落地行为性声明:@qwen-code /verify——由于作者无写权限,以 sponsored run 方式提供(维护者的评论批准其撰写时所对应的 head;该运行带执行前风险筛查与完整工作区清理,报告仍应以审视 fork CI 日志的同等怀疑态度阅读)。目前有两个声明仅凭 diff 与作者陈述支撑:(1) 结构化交付确实节省提示 Token 且不损失旧路径提供的召回质量;(2) shell/glob 拒绝层在拦截托管记忆访问的同时不会对合法模式误伤——后者完全未在 macOS 之外测试。维护者也可触发隔离的 @qwen-code /tmux 任务获取 TUI 面证据,但 /verify 更贴合本 PR 的声明。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the implementation quality is genuinely high, but the problem is not grounded in measured evidence, the direction pre-empts this repo's own measure-first plan, and it arrives at roughly ten times the size of the incremental alternative; a maintainer needs to judge the vehicle, and the 3/5 cap here is Stage 0 policy (core paths, 500+ production lines, escalated for maintainer awareness), not a code-quality verdict.

Going back to my independent proposal: for "flat memory indexes spend tokens on irrelevant entries," I would have added one bounded fetch/search tool on top of the existing recall path and trimmed the per-turn injection to metadata — roughly 500–800 lines. This PR is 6,860 production lines. Each piece of added machinery is individually defensible — background migration with rollback, body-residency tracking across compaction, HMAC-signed cursors, a whole-tool lockdown surface, a telemetry split — but together they multiply the review, revert, and maintenance surface, and none of it is justified by measurement yet.

The question I keep coming back to is whether the problem is demonstrated at all. The repo's own plan for this exact area was to measure first: #8998 exists to decide from production delivery telemetry whether a recall change is warranted. The pointed detail is that the author wrote #7393 — the delivery telemetry #8998 is meant to read — so this PR leapfrogs the very instrument they built. That doesn't make the change wrong; it makes the direction a maintainer call, not a gate call.

On execution, to be clear where my reservations are not: the code is built with real care. Signed cursors with timing-safe comparison and mtime invalidation, atomic migration writes, an explicit prepare/confirm/commit transition with rollback and telemetry on every branch, deferred delivery logging, symlink-aware path matching, and 6,288 lines of tests. If a maintainer accepts the direction, the quality bar to maintain is there. My reservations are about necessity and sequencing: no opt-out for the automatic file-access denial, a lockdown that steers rather than enforces, an LLM call per legacy file during migration, and an incremental path that was plausibly available.

On evidence: there is none at runtime. CI has not run on the reviewed commit (the fork's CI workflows have not been approved/triggered), Windows and Linux are untested, and the token-savings and recall-quality claims — the entire "why" of the PR — are substantiated by nothing in the PR or its CI. Approving on that record would be approving a description.

If I had to maintain this in six months: the dual legacy/structured paths plus the migration state machine are real ongoing cost, mitigated by the transition being designed to go one-way once the corpus is migrated. I'm not approving because I ran out of objections — I'm deferring because the central question ("should we do this now, this way, at this size?") is a product judgment the gate cannot make.

中文说明

置信度:3/5 —— 实现质量确实高,但问题缺乏实测证据支撑,方向抢先于本仓库自己的"先测量"计划,且规模约为增量替代方案的十倍;应由维护者判断载体是否合适。此处的 3/5 上限来自 Stage 0 政策(核心路径、生产代码 500+ 行、已升级至维护者知悉),而非代码质量结论。

回到我的独立方案:针对"平铺记忆索引在不相关条目上消耗 Token",我会新增一个有界 fetch/search 工具置于现有召回链路之上,并把每轮注入精简为 metadata——约 500–800 行。本 PR 是 6,860 行生产代码。新增机制的每一部分单独看都有道理——带回滚的后台迁移、跨压缩的正文驻留跟踪、HMAC 签名游标、整工具面的封锁、遥测拆分——但合在一起放大了评审、回滚与维护面,且目前没有任何一项由测量证明必要。

我反复回到的问题是:问题本身是否被证实。本仓库对该领域的既定计划是先测量:#8998 的存在就是为了依据生产交付遥测判定是否有理由做召回变更。耐人寻味的是,作者写了 #7393——即 #8998 要读取的那套交付遥测——因此本 PR 跨越了他们自己建造的测量仪器。这不意味着变更是错的;它意味着方向问题是维护者的判断,而不是 gate 的判断。

关于执行,先说清楚我的顾虑不在哪里:代码构建得相当用心。时序安全比较与 mtime 失效的签名游标、原子迁移写入、带遥测且每个分支都有回滚的明确 prepare/confirm/commit 切换、推迟的交付日志、符号链接感知的路径匹配,以及 6,288 行测试。如果维护者接受该方向,可维护的质量标准是在的。我的顾虑在于必要性与顺序:自动文件访问拒绝没有退出开关、封锁是引导而非强制、迁移期间每个旧文件一次 LLM 调用,以及一条大概率可行的增量路径。

关于证据:运行时证据为零。CI 尚未在被审 commit 上运行(fork 的 CI 工作流未被批准/触发),Windows 与 Linux 未测试,而 Token 节省与召回质量这两项声明——整个 PR 的"为什么"——在 PR 与其 CI 中都没有任何支撑。在这种记录下批准,等于批准一份描述。

如果六个月后由我维护:legacy/structured 双路径加迁移状态机是真实的持续成本,缓解因素是切换被设计为语料迁移完成后单向进行。我不批准,不是因为我说不出反对理由才放行——而是因为核心问题("现在该不该以这种方式、这种规模做?")是 gate 无法替代的产品判断,所以我选择转交。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @yiliang114 — needs a human call on this one.

This PR is not approved and not rejected. Three things put it in the maintainer's court:

  1. Policy: core paths (packages/core/src/**) with 6,860 production logic lines — the two-tier rule escalates any 500+ production-line core feature for maintainer awareness, and this lane never auto-approves it.
  2. Direction: the problem isn't grounded in measured evidence, and Validate the recall initial-turn budget against production delivery telemetry #8998 — the plan to decide from production telemetry whether a recall change is justified — is still open. The questions worth your call: accept a wholesale recall-protocol replacement now, or an incremental bounded fetch/search tool on the existing path first? And should the automatic denial of direct file/shell access to .qwen/memory/** (with no opt-out switch) be gated behind a config flag?
  3. Evidence gap: CI has not run on the reviewed commit (the fork's CI appears to need approval), Windows/Linux are untested, and the token-savings / recall-quality claims have no substantiation yet. If the direction is accepted, @qwen-code /verify is available as a sponsored run to settle the behavioural claims before deeper investment.

The implementation itself is high quality — full notes in the review comments above. @ZijianZhang989, nothing here is a rejection of the work; this is the gate saying the decision is bigger than the gate.

中文说明

⏸️ 转交 @yiliang114 —— 需要人工判断。

本 PR 既未批准也未拒绝。三点原因使其交由维护者决定:

  1. **政策:**触及核心路径(packages/core/src/**)且生产逻辑达 6,860 行——两档规则要求任何 500+ 生产行的核心功能升级至维护者知悉,此通道不会自动批准。
  2. **方向:**问题缺乏实测证据支撑,而 Validate the recall initial-turn budget against production delivery telemetry #8998——依据生产遥测判定是否有理由做召回变更的计划——仍未关闭。值得你判断的问题:现在接受整体召回协议替换,还是先在现有链路上做增量有界 fetch/search 工具?对 .qwen/memory/** 直接文件/shell 访问的自动拒绝(无退出开关)是否应置于配置开关之后?
  3. **证据缺口:**CI 尚未在被审 commit 上运行(fork 的 CI 似乎需要批准),Windows/Linux 未测试,Token 节省/召回质量声明尚无支撑。若方向被接受,可用 @qwen-code /verify(sponsored run)在深入投入前落地行为性声明。

实现本身质量很高——完整笔记见上方审查评论。@ZijianZhang989,这里没有任何否定工作的意思;这只是 gate 在说:这个决定比 gate 的职权更大。

Qwen Code · qwen3.8-max

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.

2 participants