Skip to content

feat(memory): protect pinned files during forked Dream - #7714

Merged
wenshao merged 6 commits into
QwenLM:mainfrom
destire-mio:codex/pinned-memory-dream-protection
Aug 1, 2026
Merged

feat(memory): protect pinned files during forked Dream#7714
wenshao merged 6 commits into
QwenLM:mainfrom
destire-mio:codex/pinned-memory-dream-protection

Conversation

@destire-mio

@destire-mio destire-mio commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This PR adds an opt-in managed-memory permission gate that prevents forked Dream workers from using write_file or edit on files under a top-level pinned/ directory. The gate protects project and optional user-memory roots through case-insensitive literal-path checks and symlink-resolved checks, while ordinary memory paths—including nested directories such as memory/project/pinned/—remain writable.

Forked Dream enables the gate for scheduled cleanup and the workspace-memory Dream endpoint, keeps its existing project-memory-only scope, and receives an instruction to skip pinned content during consolidation. Pinned files remain available to the existing recursive memory index. The permission path also resolves each candidate once, caches stable pinned roots when the scoped configuration is created, and reuses the pinned decision when producing a deny reason.

Why it's needed

Pinned memory is intended for durable information that automated consolidation must not rewrite. Prompt instructions alone are not a deterministic boundary, and a lexical-only check could be bypassed through case variants or symlink aliases. The gate provides an enforcement boundary for forked Dream without making every directory named pinned read-only or changing normal memory indexing and read behavior.

Reviewer Test Plan

How to verify

  1. Create project-memory and user-memory files under a top-level pinned/ directory, then evaluate write_file and edit; both operations should be denied with the matching pinned-memory reason.
  2. Repeat with case variants, symlink aliases, a dangling top-level pinned symlink, and a not-yet-created file below pinned/; the protected paths should remain denied.
  3. Write to an ordinary memory file, a pinned-notes/ directory, and memory/project/pinned/notes.md; these paths should remain allowed.
  4. Confirm read-only shell commands are still handled by the existing memory-scoped shell policy and mutating shell commands remain denied.
  5. Confirm the forked Dream planner enables protectPinnedMemory: true with includeUserMemory: false, while pinned files continue to appear in the recursive memory index.
  6. Run the focused and repository checks:
cd packages/core
npx vitest run src/memory

cd ../..
npm run build
npm run typecheck
npm run bundle

The full memory suite passed 477 tests. Targeted ESLint and Prettier checks plus git diff --check also passed.

Evidence (Before & After)

Behavior Before After
Forked Dream writes Consolidation relied on prompt instructions and could reach pinned files through write_file or edit. Top-level pinned paths are deterministically denied by the scoped permission manager.
Path aliases Case variants and symlink-resolved aliases had no dedicated pinned-memory enforcement. Literal case-insensitive paths, resolved aliases, dangling pinned symlinks, and new descendants are covered.
Ordinary memory There was no pinned-specific gate. Ordinary paths, pinned-notes/, and nested project/pinned/ paths remain writable.
Memory index Pinned files were recursively indexed. The existing indexing behavior is preserved.
Path-resolution cost The initial gate could repeat synchronous path resolution during one permission flow. Candidates resolve once per evaluation, pinned roots are cached at configuration creation, and deny-reason lookup reuses the decision.

Tested on

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

Environment (optional)

macOS on arm64 using the repository npm workspace toolchain.

Risk & Scope

  • Main risk or tradeoff: Path containment must protect only top-level pinned roots without denying legitimate nested directories; focused literal, case-variant, symlink, dangling-symlink, new-path, and boundary tests cover this behavior.
  • Not validated / out of scope: The visible /dream slash command runs as a main-Agent submit_prompt turn and remains prompt-protected rather than using the forked worker's deterministic gate. Explicit /forget, the /memory UI, forked-Dream user-memory access, and local Windows/Linux execution are also out of scope.
  • Breaking changes / migration notes: None. The gate is opt-in, normal memory reads and indexing remain unchanged, and forked Dream retains includeUserMemory: false.

Linked Issues

Refs #6801

中文说明

本 PR 做了什么

本 PR 新增一个可选的受管理记忆权限门禁,阻止 Forked Dream Worker 对顶层 pinned/ 目录下的文件执行 write_fileedit。该门禁通过不区分大小写的字面路径检查和符号链接解析后的路径检查保护项目记忆以及可选的用户记忆根目录;普通记忆路径仍可写,包括 memory/project/pinned/ 这类嵌套目录。

Forked Dream 会在定时清理和 workspace-memory Dream 端点中启用该门禁,保持现有的“仅项目记忆”范围,并在 consolidation 提示中明确跳过 pinned 内容。Pinned 文件仍会被现有递归记忆索引收录。权限路径还会让每个候选路径只解析一次,在创建 scoped 配置时缓存稳定的 pinned 根目录,并在生成拒绝原因时复用 pinned 判断结果。

为什么需要

Pinned memory 用于保存不应被自动 consolidation 改写的持久信息。仅靠提示词不能构成确定性的保护边界,只检查字面路径也可能被大小写变体或符号链接别名绕过。该门禁为 Forked Dream 提供强制保护,同时不会把所有名为 pinned 的目录都变成只读,也不会改变正常的记忆索引和读取行为。

审阅者测试计划

如何验证

  1. 在项目记忆和用户记忆的顶层 pinned/ 目录下创建文件,然后检查 write_fileedit;两种操作都应被拒绝,并返回对应的 pinned-memory 原因。
  2. 使用大小写变体、符号链接别名、指向顶层 pinned 的悬空符号链接,以及 pinned/ 下尚未创建的文件重复验证;这些受保护路径都应继续被拒绝。
  3. 写入普通记忆文件、pinned-notes/ 目录和 memory/project/pinned/notes.md;这些路径应继续允许写入。
  4. 确认只读 Shell 命令仍由现有 memory-scoped Shell 策略处理,具有修改行为的 Shell 命令继续被拒绝。
  5. 确认 Forked Dream Planner 使用 protectPinnedMemory: trueincludeUserMemory: false,同时 pinned 文件仍会进入递归记忆索引。
  6. 运行以下聚焦测试和仓库检查:
cd packages/core
npx vitest run src/memory

cd ../..
npm run build
npm run typecheck
npm run bundle

完整 memory 测试套件共 477 个测试通过;定向 ESLint、Prettier 和 git diff --check 也均通过。

修改前后证据

行为 修改前 修改后
Forked Dream 写入 Consolidation 依赖提示词约束,仍可能通过 write_fileedit 访问 pinned 文件。 顶层 pinned 路径由 scoped permission manager 确定性拒绝。
路径别名 大小写变体和符号链接解析后的别名没有专门的 pinned-memory 强制保护。 覆盖不区分大小写的字面路径、解析后的别名、悬空 pinned 符号链接和新建后代路径。
普通记忆 不存在 pinned 专属门禁。 普通路径、pinned-notes/ 和嵌套 project/pinned/ 路径仍可写。
记忆索引 Pinned 文件会被递归索引。 保留现有索引行为。
路径解析开销 初版门禁可能在同一次权限流程中重复执行同步路径解析。 每次评估只解析一次候选路径;创建配置时缓存 pinned 根目录;拒绝原因查找复用判断结果。

测试平台

操作系统 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 未在本机测试
🐧 Linux ⚠️ 未在本机测试

环境(可选)

macOS arm64,使用仓库的 npm workspace 工具链。

风险与范围

  • 主要风险或取舍:路径包含判断必须只保护顶层 pinned 根目录,不能误伤合法的嵌套目录;聚焦测试覆盖字面路径、大小写变体、符号链接、悬空符号链接、新路径和边界情况。
  • 未验证 / 范围外:可见的 /dream 命令以主 Agent 的 submit_prompt Turn 运行,仍只有提示词保护,不使用 Forked Worker 的确定性门禁。显式 /forget/memory UI、Forked Dream 的用户记忆访问,以及 Windows/Linux 本地执行也不在本 PR 范围内。
  • 破坏性变更 / 迁移说明:无。该门禁为可选功能,正常记忆读取和索引保持不变,Forked Dream 继续使用 includeUserMemory: false

关联 Issue

Refs #6801

@destire-mio
destire-mio marked this pull request as ready for review July 25, 2026 13:10
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Thanks for the PR — and for the rework since the last pass. The head moved (09c1f14cd52193), so this is a fresh review of the current diff.

Template: matches now ✓ — the body uses the expected headings (What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope / Linked Issues) with a bilingual summary.

Problem: real and requested, not theoretical. This implements #6801 (P2, scope/memory): hand-curated documents placed under the memory root get merged, "corrected", or deleted by automated Dream consolidation exactly like auto-generated topics, because the forked worker has write access to the whole root and the recursive indexer already discovers files under pinned/. The issue has a concrete scenario; the missing piece is a deterministic mutation boundary, which is what this adds.

Direction: aligned. Memory is core, and protecting user-curated files from automated consolidation is squarely within the feature's intent. The PR is honest about what it deliberately leaves out — the visible /dream slash command runs on the main Agent and gets the skip instruction but not the deterministic tool gate, flagged as an open maintainer scope question rather than smuggled in.

Size: feat touching packages/core/src/memory/. 139 production logic lines (memory-scoped-agent-config.ts 119, dreamAgentPlanner.ts 14, extractionAgentPlanner.ts 5, paths.ts 1), 395 test lines, 105 docs lines. Well under any threshold — no maintainer escalation needed on size.

Approach: the scope feels right and the change is fail-closed. It adds one opt-in boolean (protectPinnedMemory, default false) to the existing memory-scoped config, a deny check that covers both lexical case-variants and symlink-resolved aliases, prompt instructions across the consolidation/extraction phases, and tests. Only the forked Dream and extraction planners opt in; explicit /remember, /forget, reads, and indexing keep their current behavior. Every edit serves the stated goal — no drive-by changes.

Risk: no elevated signals. None of the changed files match the high-revert paths (no streaming/shell/MCP/LSP/sandbox surface) — this is contained to the memory permission boundary.

Moving on to code review. 🔍

中文说明

感谢贡献,也感谢自上轮以来的修改。head 已更新(09c1f14cd52193),所以这是对当前 diff 的全新审查。

模板:现已符合 ✓ —— 正文使用了预期的标题(What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope / Linked Issues),并附带中文说明。

问题:真实且已被请求,并非理论性问题。本 PR 实现的是 #6801(P2,scope/memory):放在 memory 根目录下的人工维护文档,会和自动生成的主题一样被 Dream 自动整合合并、"纠正"或删除——因为 forked worker 对整个根目录有写权限,而递归索引器本来就会发现 pinned/ 下的文件。issue 里有具体场景;缺失的是一条确定性的写入边界,本 PR 正是补上这一点。

方向:对齐。记忆是核心功能,保护用户手工维护的文件不被自动整合,完全在该功能的意图之内。PR 也诚实地说明了刻意不做的部分——可见的 /dream 斜杠命令运行在主 Agent 上,只获得跳过指令,没有确定性工具门禁,并被明确标记为一个待 maintainer 决定的范围问题,而非偷偷夹带。

规模:feat,触及 packages/core/src/memory/。139 行生产逻辑(memory-scoped-agent-config.ts 119、dreamAgentPlanner.ts 14、extractionAgentPlanner.ts 5、paths.ts 1),395 行测试,105 行文档。远低于任何阈值——无需因规模转交 maintainer。

方案:范围合理,且改动是 fail-closed 的。它在现有 memory-scoped config 上增加一个 opt-in 布尔值(protectPinnedMemory,默认 false)、一个同时覆盖词法大小写变体和符号链接解析别名的拒绝检查、贯穿 consolidation/extraction 各阶段的提示指令,以及测试。只有 forked Dream 和 extraction planner 启用该开关;显式 /remember/forget、读取和索引都保持现有行为。diff 中每处改动都服务于既定目标——没有顺手改动。

风险:无升级信号。改动文件均未命中高回滚路径(不涉及 streaming/shell/MCP/LSP/sandbox 面)——局限于记忆权限边界。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: I'd put a reserved pinned/ constant in paths.ts, add a path-containment deny to the memory-scoped permission manager for write_file/edit under <memoryRoot>/pinned/, make the match case-insensitive (so it can't fail open on a case-insensitive FS) and also catch symlink aliases that resolve back into pinned/, tell the consolidation/extraction agents to skip pinned/, leave reads and the indexer untouched, and test the gate plus the bypass vectors. That is essentially what this PR does.

Findings: no critical blockers. The implementation is clean, idiomatic, and — importantly for a permission boundary — fail-closed: it only ever adds new denials, never a new allow.

  • The allow path is a behavior-preserving refactor. isAllowedMemoryPath now delegates to a new isAllowedResolvedMemoryPath so the candidate is realpath-resolved once and reused; the exported function's semantics are unchanged, which keeps the other consumers (remember.ts, refresh.ts, and the read/grep/ls branch) unaffected.
  • isProtectedPinnedMemoryPath reuses the existing realpathExistingOrNew and isWithinRoot helpers. The dual check is correct: literal case-insensitive containment catches direct paths, case variants, and pinned/ itself being a symlink; resolved containment catches an alias elsewhere in memory that symlinks back in. isWithinRoot does proper segment-boundary comparison, so pinned-notes/ and nested project/pinned/ correctly stay writable.
  • Order is right — the pinned deny runs before the general allow check in evaluateScopedDecision, and getScopedDenyRule mirrors it to produce a descriptive reason. For a pinned/ symlink whose target sits outside memory, the deny rule reports the more fundamental only within <root> reason rather than the pinned one, which matches the test and is the better message.
  • protectPinnedMemory defaults to false and is enabled only by the forked Dream and extraction planners; remember.ts doesn't set it, so explicit remember/forget, reads, and indexing retain current behavior. Pinned roots are snapshotted once at config creation.
  • Tests (395 lines) are load-bearing and cover the exact bypass vectors: case variant, symlink alias, pinned/-as-symlink, dangling top-level pinned symlink, outside-root target, nested project/pinned/, pinned-notes/ prefix, protection-disabled, user-memory scope, indexer inclusion, and the prompt assertions.

Two non-blocking observations: getScopedDenyRule re-resolves the candidate on its own call path (a second realpathSync, harmless since it's separate from evaluate), and the visible /dream slash-command gate is deliberately deferred — documented as an open maintainer scope decision, not a gap in this PR's stated goal.

Test evidence (this PR's own CI — I did not run the code)

Unattended run, so per the gate rules I'm reading the PR's CI signal rather than executing anything. All 46 check-runs on cd52193 are success or skipped — zero failures, zero in-progress. The Linux unit suite and web-shell E2E are green; macOS/Windows/integration are skipped, which is the normal posture for a fork PR.

Check Conclusion
Test (ubuntu-latest, Node 22.x) success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) success
precheck-pr / precheck success
Classify PR / label / route success
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped
Integration Tests (CLI, No Sandbox) skipped

The deny behavior itself is substantiated by that ubuntu suite — the symlink/case tests create real on-disk links and assert deny, so they fail if the gate is removed; this is not a green-suite-that-passes-without-the-diff situation.

Sandboxed verification would still settle one thing the unit suite can't: @qwen-code /verify — that the gate holds in the full forked-Dream/extraction runtime flow, end to end, not only when the permission manager is exercised in isolation. The author's manual testing was macOS-only (their claim, not independently re-run); Linux is covered by CI above, and the case-insensitive literal check is platform-independent and fail-closed, so Windows is low-risk even though untested. This is a fork PR, so /verify would be a sponsored run — a maintainer's @qwen-code /verify approves the head it's written against and runs behind a pre-execution risk screen plus a workspace wipe; read the resulting report with the same skepticism as the fork's own CI logs.

中文说明

代码审查

独立方案: 我会在 paths.ts 放一个保留的 pinned/ 常量,在 memory-scoped permission manager 里对 <memoryRoot>/pinned/ 下的 write_file/edit 加一条路径包含拒绝规则,匹配做成不区分大小写(避免在大小写不敏感的文件系统上 fail open),同时捕获解析后落回 pinned/ 的符号链接别名,提示 consolidation/extraction agent 跳过 pinned/,读取和索引器保持不动,并对门禁和各种绕过路径加测试。这基本就是本 PR 所做的。

结论: 无关键阻塞。实现干净、符合惯例,而且——对一条权限边界来说很关键——是 fail-closed 的:它只新增拒绝,从不新增允许。

  • 允许路径是一次行为保持的重构。isAllowedMemoryPath 现在委托给新的 isAllowedResolvedMemoryPath,使候选路径只 realpath 解析一次并被复用;导出函数的语义不变,从而不影响其他消费方(remember.tsrefresh.ts 以及 read/grep/ls 分支)。
  • isProtectedPinnedMemoryPath 复用了现有的 realpathExistingOrNewisWithinRoot。双重检查是正确的:不区分大小写的字面包含捕获直接路径、大小写变体以及 pinned/ 自身是符号链接的情形;解析后的包含捕获 memory 别处别名符号链接回 pinned/ 的情形。isWithinRoot 做了正确的路径段边界比较,所以 pinned-notes/ 和嵌套的 project/pinned/ 正确地保持可写。
  • 顺序正确——pinned 拒绝在 evaluateScopedDecision 的通用允许检查之前执行,getScopedDenyRule 镜像该逻辑以给出描述性原因。对于目标在 memory 之外pinned/ 符号链接,拒绝规则报告更根本的 only within <root> 原因而非 pinned 原因,与测试一致,也是更好的提示。
  • protectPinnedMemory 默认 false,仅由 forked Dream 和 extraction planner 启用;remember.ts 不设置它,因此显式 remember/forget、读取和索引都保持现有行为。Pinned 根目录在创建配置时快照一次。
  • 测试(395 行)是 load-bearing 的,覆盖了确切的绕过路径:大小写变体、符号链接别名、pinned/ 作为符号链接、悬空的顶层 pinned 符号链接、外部目标、嵌套 project/pinned/pinned-notes/ 前缀、关闭保护、用户记忆范围、索引器收录,以及提示断言。

两个非阻塞观察:getScopedDenyRule 在自己的调用路径上重新解析候选路径(第二次 realpathSync,因与 evaluate 分离而无害);可见的 /dream 斜杠命令门禁被刻意推迟——已作为待 maintainer 决定的范围问题记录在案,并非本 PR 既定目标的缺口。

测试证据(本 PR 自身的 CI——我未运行代码)

无人值守运行,因此按门禁规则我读取的是 PR 的 CI 信号,而非执行任何代码。cd52193 上全部 46 个 check-run 均为 successskipped——零失败、零进行中。Linux 单元测试套件和 web-shell E2E 为绿色;macOS/Windows/集成为 skipped,这是 fork PR 的正常状态。

拒绝行为本身由上述 ubuntu 套件证实——符号链接/大小写测试在磁盘上创建真实链接并断言 deny,因此移除门禁就会失败;这不是"没有 diff 也能通过"的绿色套件。

沙盒验证仍能解决一件单元测试套件无法解决的事:@qwen-code /verify——验证门禁在完整的 forked-Dream/extraction 运行时流程中端到端成立,而不仅仅是在 permission manager 被单独调用时成立。作者的手工测试仅在 macOS 上进行(其自述,未独立复跑);Linux 由上面的 CI 覆盖,且不区分大小写的字面检查与平台无关且 fail-closed,因此 Windows 即便未测试风险也很低。这是一个 fork PR,所以 /verify 将是一次赞助运行——maintainer 的 @qwen-code /verify 会批准其写入时所针对的 head,并在执行前风险筛查与工作区清空之后运行;请以对待 fork 自身 CI 日志同样的审慎态度阅读其报告。

Qwen Code · qwen3.8-max-preview

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid, correctly-scoped, and fail-closed; the only reservations are non-blocking (the deferred /dream slash-command gate is a maintainer scope call, and Windows wasn't validated locally though Linux CI is green).

Stepping back: this is exactly the kind of contribution the gate should let through. The problem is real and was explicitly requested (#6801, P2) — hand-curated memory files get destroyed by automated consolidation today, and the PR closes that with a deterministic boundary instead of another prompt instruction. My independent proposal and the PR's approach are essentially the same; I didn't find a simpler path it missed.

What earns the confidence: the change is fail-closed end to end. It only adds denials — the allow path is a behavior-preserving refactor, the flag defaults off, and only the two automated workers opt in. The bypass vectors that matter for a permission boundary (case variants, symlink aliases, pinned/-as-symlink, dangling symlinks, outside-root targets, lookalike pinned-notes/, nested project/pinned/) are each pinned by a load-bearing test, and the full suite is green on Linux CI. I can name every downstream consumer and none are harmed. In six months I'd thank the author, not curse them — there's a design doc, user docs, and tests that document the intent.

What keeps it off 5/5: the visible /dream slash command still relies on the skip instruction rather than a deterministic gate (honestly flagged as an open question, so a maintainer should confirm that scoping is acceptable before merge), and the author's manual testing was macOS-only. Neither blocks — the slash-command surface is out of this PR's stated scope and CI covers the Linux behavior — but they're the reason this is a strong approve rather than an unqualified one.

Approving, pinned to the reviewed commit. ✅

中文说明

置信度:4/5 —— 扎实、范围正确、且 fail-closed;唯一的保留是非阻塞的(推迟的 /dream 斜杠命令门禁是一个 maintainer 范围决定,Windows 未在本地验证但 Linux CI 为绿色)。

退一步看:这正是门禁应当放行的那类贡献。问题是真实的,且已被明确请求(#6801,P2)——人工维护的记忆文件今天会被自动整合销毁,而本 PR 用一条确定性边界而非又一条提示指令解决了它。我的独立方案与 PR 的方案基本一致;我没有找到它遗漏的更简路径。

赢得这份置信度的是:改动从头到尾都是 fail-closed 的。它只新增拒绝——允许路径是行为保持的重构,开关默认关闭,且只有两个自动 worker 启用。对一条权限边界而言重要的绕过路径(大小写变体、符号链接别名、pinned/ 作为符号链接、悬空符号链接、外部目标、形似的 pinned-notes/、嵌套 project/pinned/)每一项都由一个 load-bearing 测试钉住,且完整套件在 Linux CI 上为绿色。我能点名每一个下游消费方,且无一受损。六个月后我会感谢作者,而非抱怨——有设计文档、用户文档和测试记录了意图。

未到 5/5 的原因:可见的 /dream 斜杠命令仍依赖跳过指令而非确定性门禁(已诚实地标记为开放问题,因此 maintainer 在合并前应确认该范围划定可接受),且作者的手工测试仅在 macOS 上进行。两者都不阻塞——斜杠命令面在本 PR 既定范围之外,CI 覆盖了 Linux 行为——但它们是这是一次"强烈批准"而非"无条件批准"的原因。

批准,固定到所审查的提交。✅

Qwen Code · qwen3.8-max-preview

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— 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.

LGTM, looks ready to ship — CI landed green after the review. ✅

wenshao
wenshao previously approved these changes Jul 25, 2026

@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.

No issues found. LGTM! ✅

— qwen3.8-max-preview via Qwen Code /review

@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.

⚠️ This run could not certify that any of this diff was reviewed. Suggestions are inline. Not reviewed: the entire diff — no agent reported covering it; nobody read it. Not reviewed: every dimension — none of the 12 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against.

— qwen3.8-max-preview via Qwen Code /review

expect(index).toContain('The repo uses pnpm workspaces.');
});

it('includes a valid pinned document in the generated index', async () => {

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 new test 'includes a valid pinned document in the generated index' is inert — the test-efficacy probe confirmed all 8 tests in this file pass identically with the PR's source changes (paths.ts, memory-scoped-agent-config.ts, dreamAgentPlanner.ts) reverted. The indexer already scanned all subdirectories including pinned/ before this PR; the new AUTO_MEMORY_PINNED_DIRNAME constant is consumed by the permission gate in memory-scoped-agent-config.ts, not by the indexer. — Concrete cost: if a future change breaks pinned-directory indexing (e.g., the indexer stops scanning subdirectories or explicitly excludes pinned/), this test would still pass green, and the regression would ship undetected.

Consider either (a) making the test import and assert against AUTO_MEMORY_PINNED_DIRNAME so it fails when the constant is removed or the indexer's subdirectory scanning changes, or (b) moving the pinned-indexing assertion into the permission-gate test suite (memory-scoped-agent-config.test.ts) where the new code actually lives.

— qwen3.8-max-preview via Qwen Code /review

@gwinthis

Copy link
Copy Markdown
Collaborator

Review + Linux E2E verification report (real build, tmux)

Verdict: the deterministic gate is correctly designed and holds up under adversarial probing. And this run produced direct empirical evidence for the PR's own scope caveat: prompt-level protection on the visible /dream demonstrably fails in practice, which strengthens the case for extending the gate (#6801).

Code review reasoning

  1. The option is wired, not dead. Per this repo's read-site rule: protectPinnedMemory is declared, read at three sites in the permission evaluator/deny-rule builder, and set to true by the forked Dream planner. remember/extraction planners intentionally leave it off — reasonable for this PR's scope.
  2. The path check is defense-in-depth done right. Both the lexical path and the symlink-resolved path (realpathSync, with closest-existing-parent resolution for new files) are checked against both project and user pinned roots. Deny-before-allow ordering means the pinned check cannot be shadowed by the general memory-path allowlist.
  3. The dangling-symlink leaf edge is safe by composition. realpathExistingOrNew returns undefined for a dangling symlink leaf; the pinned deny can't fire on it, but the allowlist also requires a resolvable path, so the decision falls through to deny anyway. I verified this empirically (below). Within the threat model it's also unreachable: the worker's shell is read-only and its write tools write text, so it cannot create symlinks.
  4. Honest scoping. The PR explicitly documents that main-Agent /dream gets only the prompt instruction, and defers the per-turn gate to maintainer direction rather than mutating the session-wide permission manager around one tool loop — the right call per the design doc's open question.

Verification evidence (Linux, commit 09c1f14, full build + bundle)

Unit level: memory suite 475/476 passed. The 1 failure (memoryLifecycle.integration.test.ts) is pre-existing and environment-dependent — it fails identically on a non-PR checkout because real ~/.qwen user memories leak into the recall assertion. Worth a separate hermeticity fix; not this PR's fault.

Adversarial probe (temporary test against the PR harness, then removed):

  • Writing a new file through a directory symlink pointing into pinned/deny
  • Overwriting a dangling symlink leaf targeting pinned/missing.mddeny (via allowlist fall-through) ✅

tmux E2E (real bundled CLI, QWEN_CODE_MEMORY_LOCAL=1, seeded memory: pinned/architecture.md + two duplicate topic files + index): ran /dream to completion.

  • Duplicates correctly consolidated into one file and removed.
  • pinned/architecture.md byte-identical after the run (sha256 07c63243… unchanged) ✅
  • But: the rewritten MEMORY.md contains only the consolidated entry — the model dropped the pinned index entry despite the prompt's explicit "Do not intentionally remove existing index entries for valid pinned/ files" instruction. ❌ (self-heals on the next deterministic index rebuild, but it is a live demonstration that instruction-level protection is best-effort)

Implication

The gate (deterministic, tested, symlink-aware) held everywhere it applies. The one observed protection failure happened exactly in the layer that only has prompt-level protection. That is strong empirical support for resolving #6801 in favor of a per-turn deterministic gate for the visible /dream as a follow-up.

@wenshao

wenshao commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Maintainer local verification — real-flow, no mocks

Verified 09c1f14 against merge-base 2049d50 in isolated worktrees. I did not trust the unit tests' {} as Config stub: every result below comes from the actual production stack a forked Dream worker takes — real Config, real tool registry, real WriteFileTool/EditTool/ShellTool, the same createApprovalModeOverride(YOLO) wrapper runForkedAgent applies, real CoreToolScheduler.schedule() → real evaluatePermissionFlow(), against a real temp filesystem. The oracle is the bytes on disk, not a decision string.

Verdict: the fix works and it is load-bearing. The pre-PR configuration demonstrably clobbers a pinned file; the PR configuration does not. I found one robustness gap and one documentation gap that I'd like addressed, neither of which blocks the core behavior.

1. Real-flow E2E — 9/9

real-flow E2E

The important row is the last one. With the options dreamAgentPlanner passed before this PR ({ allowShell: true, includeUserMemory: false }), the identical write_file succeeds and pinned/architecture.md becomes "HIJACKED BY DREAM AGENT". With protectPinnedMemory: true it is denied and the bytes are untouched. That is the whole PR, proven end to end.

Also confirmed there is no over-blocking: ordinary topic writes, brand-new topic files, and MEMORY.md reindexing all still succeed (E2E-4/5/6) — Dream can still do its job.

2. Adversarial probes — 2 findings

adversarial probes

F1 · Medium (non-blocking) — a case-variant path defeats the gate on case-insensitive filesystems

write_file to <memory>/PINNED/architecture.md returns success, and the pinned file's bytes become "HIJACKED\n" (probe P1, on macOS APFS).

Root cause, measured directly:

fs.realpathSync("<t>/pinned")  ->  /private/<t>/pinned
fs.realpathSync("<t>/PINNED")  ->  /private/<t>/PINNED     # realpathSync.native -> "pinned"
path.relative(".../pinned", ".../PINNED/a.md") = "../PINNED/a.md"   -> isWithinRoot false

Both the lexical and the resolved containment checks are byte-exact, and Node's fs.realpathSync preserves the caller's casing (only realpathSync.native canonicalises it). The filesystem, however, is case-insensitive — so the write lands on the very same inode.

Worth noting why this is new rather than pre-existing: isAllowedMemoryPath has the same case behavior, but it is an allow-list, where case-preservation fails closed. isProtectedPinnedMemoryPath is the first deny-list built on these helpers, and there the same property fails open. Default macOS (APFS) and Windows (NTFS) are both affected; on a case-sensitive filesystem PINNED/ is simply a different directory and nothing is clobbered.

I'd call the practical exploitability low — the prompt and the directory listing both spell it lowercase, so a real Dream turn is unlikely to produce PINNED/. But the design doc sells this as deterministic mutation protection, and one character of casing currently defeats it.

Suggested fix, contained to isProtectedPinnedMemoryPath: compare the first path segment beneath the memory root case-insensitively (or resolve via fs.realpathSync.native). On a genuinely case-sensitive filesystem that over-blocks a distinct PINNED/ directory — which is the safe direction for a deny-list.

F2 · Low (docs) — nested pinned/ directories are silently unprotected

memory/project/pinned/notes.md → write succeeds, content replaced (probe P3).

The design doc does say top-level pinned/, so this is intended. My concern is the user-facing wording in docs/users/features/memory.md: "Put hand-curated documents that Dream should preserve under pinned/ in a managed-memory directory". A user who creates memory/project/pinned/ reads that as satisfied, gets no warning, and loses the file at the next Dream. One explicit sentence would close it, e.g. "Only a pinned/ directory at the root of a managed-memory directory is protected — …/project/pinned/ is not."

What held up under attack: dot-segment traversal (project/../pinned/…), doubled separators, trailing /, trailing-slash-on-file, relative file_path, a broken pinned/ symlink, a pinned/ symlink escaping outside the memory root, and write_file aimed at the pinned/ directory itself — all denied, all bytes intact. The shell battery is the one I most expected to break, since the design leans on the pre-existing read-only gate rather than adding anything: 18/18 denied, 0/18 mutated, including sed -i, perl -pi -e, python3 -c, node -e, find -delete, truncate, dd of=, tee, and plain > redirection.

3. Mutation matrix — 8/11 caught

mutation matrix

F3 · Test coverage — 3 surviving mutations (real branches, just unasserted)

I re-ran the probe battery with each survivor applied to confirm none of them is dead code:

  • M2 — the lexical containment branch. Delete it and every test stays green. It is reachable: with pinned/ as a broken symlink (probe P6) the deterministic deny disappears and the write only fails incidentally with ENOENT: … mkdir '…/memory/pinned'. A test mirroring P6 would pin it.
  • M5 / M6 — the two pinned memory is read-only deny messages. Delete them and every test stays green, but the message the model actually receives degrades to ManagedAutoMemory(write_file: only within …/.qwen/memory) — which is false, since the path is within that root. A confusing deny message is exactly the kind of thing that makes a model retry in a loop. One assertion on findMatchingDenyRule each.

Not blockers, but all three are a few lines.

4. Regression gates on the PR head

gate result
npx vitest run src/memory 475 passed, +4 net tests vs. base (matches the 4 new it() blocks)
npx tsc --noEmit (core) exit 0
npx eslint (6 changed files) exit 0, no output
npx prettier --check (8 changed files) All matched files use Prettier code style
Note on the 1 failing test

memoryLifecycle.integration.test.ts fails identically on the merge-base (471 passed | 1 failed there, 475 passed | 1 failed here). It is an environment artifact — the real ~/.qwen/memories on my machine leaks into the recall assertion. Not caused by this PR.

5. Scope claims in the PR description — all check out

  • /dream really is ungated. packages/cli/src/ui/commands/dreamCommand.ts returns { type: 'submit_prompt' } on the main Agent, so no per-turn permission override applies. Accurate as described.
  • /dream really does receive the skip instruction. MemoryManager.buildConsolidationPrompt (manager.ts:1480) delegates to the same buildConsolidationTaskPrompt, so it picks up all three new task-prompt lines. The two new system-prompt rules are forked-worker-only — worth knowing when you decide Feature: pinned/ memory directory - read-only files protected from /dream consolidation #6801.
  • /forget is genuinely unaffected. forget.ts deletes via fs.unlink / atomicWriteFile directly, never through the permission manager, so the documented "you can still remove them with /forget" holds.

Recommendation

Merge-worthy on behavior. I'd like F1 as a follow-up commit (a few lines in one function) since it undercuts the "deterministic" framing, F2 as a one-sentence doc clarification, and F3 as two small tests. None of them changes the verdict that this PR fixes a real data-loss path.

Harnesses are reproducible on request — three temporary vitest files under packages/core/src/memory/ plus a mutation script; nothing was committed to the branch.

中文版本(点击展开)

维护者本地验证 — 真实链路,无 mock

在隔离 worktree 中以 09c1f14 对比 merge-base 2049d50 验证。我没有采信单测里的 {} as Config 桩:下面所有结论都跑在 forked Dream worker 真实走的生产链路上 —— 真实 Config、真实工具注册表、真实 WriteFileTool/EditTool/ShellTool、与 runForkedAgent 相同的 createApprovalModeOverride(YOLO) 包装、真实 CoreToolScheduler.schedule() → 真实 evaluatePermissionFlow(),落在真实临时文件系统上。判定依据是磁盘上的字节,而不是某个决策字符串。

结论:修复有效且确实起作用。 合入前的配置能够真实覆盖 pinned 文件,本 PR 的配置则不会。我发现一个健壮性缺口和一个文档缺口,两者都不阻塞核心行为。

1. 真实链路 E2E — 9/9 通过

关键是最后一行:使用本 PR 之前 dreamAgentPlanner 所传的参数({ allowShell: true, includeUserMemory: false }),同一个 write_file 成功执行pinned/architecture.md 变成了 "HIJACKED BY DREAM AGENT";加上 protectPinnedMemory: true 后被拒绝,字节保持不变。这就是本 PR 的全部价值,端到端得到证明。

同时确认没有误伤:普通主题文件写入、新建主题文件、MEMORY.md 重建索引均仍然成功(E2E-4/5/6),Dream 仍能正常工作。

2. 对抗性探测 — 2 个发现

F1 · 中等(不阻塞)— 大小写变体路径可绕过该防护(大小写不敏感文件系统)

<memory>/PINNED/architecture.md 写入返回 success,pinned 文件字节变为 "HIJACKED\n"(探测 P1,macOS APFS)。

直接实测的根因:

fs.realpathSync("<t>/pinned")  ->  /private/<t>/pinned
fs.realpathSync("<t>/PINNED")  ->  /private/<t>/PINNED     # realpathSync.native -> "pinned"
path.relative(".../pinned", ".../PINNED/a.md") = "../PINNED/a.md"   -> isWithinRoot false

字面比较与 realpath 比较都是逐字节的,而 Node 的 fs.realpathSync 会保留调用方的大小写(只有 realpathSync.native 会归一化)。但文件系统是大小写不敏感的 —— 于是写入落到了同一个 inode 上。

值得说明这为什么是问题而非既有问题:isAllowedMemoryPath 有同样的大小写特性,但它是白名单,大小写保留会导致 fail-closed(安全侧);isProtectedPinnedMemoryPath 是基于这些 helper 的第一个黑名单,同样的特性变成了 fail-open。默认的 macOS(APFS)与 Windows(NTFS)都受影响;在大小写敏感的文件系统上 PINNED/ 只是另一个目录,不会覆盖任何内容。

实际可利用性我判断较低 —— 提示词和目录列表都是小写 pinned,真实 Dream 轮次不太可能产出 PINNED/。但设计文档把这个能力定位为确定性的写入防护,而目前一个字符的大小写差异就能绕过它。

建议修复(仅限 isProtectedPinnedMemoryPath 内部):对 memory 根目录下的第一段路径做大小写不敏感比较,或改用 fs.realpathSync.native 解析。在真正大小写敏感的文件系统上这会多拦截一个独立的 PINNED/ 目录 —— 对黑名单而言这是安全的方向。

F2 · 低(文档)— 嵌套 pinned/ 目录静默失去保护

memory/project/pinned/notes.md → 写入成功,内容被替换(探测 P3)。

设计文档确实写了顶层 pinned/,所以这是预期行为。我担心的是 docs/users/features/memory.md 的面向用户措辞:「Put hand-curated documents that Dream should preserve under pinned/ in a managed-memory directory」。用户若创建 memory/project/pinned/,会认为已满足条件,却得不到任何提示,并在下一次 Dream 时丢失文件。补一句明确说明即可,例如:「只有位于 managed-memory 目录根部的 pinned/ 受保护,…/project/pinned/ 不受保护。」

经受住攻击的部分: 点号回溯(project/../pinned/…)、重复分隔符、结尾斜杠、文件路径带结尾斜杠、相对 file_path、断链的 pinned/ 符号链接、指向 memory 根之外的 pinned/ 符号链接、以及直接对 pinned/ 目录本身写入 —— 全部被拒绝,字节全部完好。我最担心会被突破的是 shell 这一环(设计上直接依赖既有的只读闸门,本 PR 未做加固):18/18 全部拒绝,0/18 发生改动,覆盖 sed -iperl -pi -epython3 -cnode -efind -deletetruncatedd of=tee 以及普通的 > 重定向。

3. 变异矩阵 — 11 项中捕获 8 项

F3 · 测试覆盖 — 3 项变异存活(都是真实分支,只是缺断言)

我对每个存活项都重跑了探测集,确认它们都不是死代码:

  • M2 — 字面包含分支。 删掉它,全部测试依然绿。它确实可达:当 pinned/ 是断链符号链接时(探测 P6),确定性拒绝消失,写入只是偶然地以 ENOENT: … mkdir '…/memory/pinned' 失败。补一个对应 P6 的用例即可锁定。
  • M5 / M6 — 两条 pinned memory is read-only 拒绝消息。 删掉它们全部测试依然绿,但模型实际收到的消息会退化为 ManagedAutoMemory(write_file: only within …/.qwen/memory) —— 这是错误的,因为该路径确实在那个根目录内。误导性的拒绝消息恰恰是会让模型反复重试的那类问题。各补一条对 findMatchingDenyRule 的断言即可。

都不是阻塞项,但三项加起来也就几行。

4. PR head 上的回归闸门

闸门 结果
npx vitest run src/memory 475 通过,相对 base 净增 4 个用例(与新增的 4 个 it() 吻合)
npx tsc --noEmit(core) exit 0
npx eslint(6 个改动文件) exit 0,无输出
npx prettier --check(8 个改动文件) 全部符合 Prettier 风格

关于那 1 个失败用例:memoryLifecycle.integration.test.ts 在 merge-base 上同样失败(那边是 471 通过 | 1 失败,这边是 475 通过 | 1 失败)。这是环境产物 —— 我本机真实的 ~/.qwen/memories 泄漏进了 recall 断言。与本 PR 无关。

5. PR 描述中的范围声明 — 全部核实无误

  • /dream 确实没有确定性闸门。 packages/cli/src/ui/commands/dreamCommand.ts 在主 Agent 上返回 { type: 'submit_prompt' },不存在每轮的权限覆盖。描述准确。
  • /dream 确实收到了 skip 指令。 MemoryManager.buildConsolidationPromptmanager.ts:1480)委托给同一个 buildConsolidationTaskPrompt,因此拿到了全部三条新增的任务提示行。两条新增的系统提示规则则仅限 forked worker —— 在决定 Feature: pinned/ memory directory - read-only files protected from /dream consolidation #6801 时值得注意这一差异。
  • /forget 确实不受影响。 forget.ts 直接用 fs.unlink / atomicWriteFile 删除,完全不经过 permission manager,因此文档中「仍可用 /forget 删除」的说法成立。

建议

从行为上看可以合入。我希望把 F1 作为后续提交处理(单个函数内的几行改动),因为它削弱了「确定性」这一定位;F2 补一句文档说明;F3 补两个小测试。这些都不改变本 PR 修复了一条真实数据丢失路径这一结论。

验证脚手架可按需复现 —— packages/core/src/memory/ 下三个临时 vitest 文件加一个变异脚本,未向分支提交任何内容。

@destire-mio
destire-mio dismissed stale reviews from wenshao and qwen-code-ci-bot via 67c90a9 July 26, 2026 04:37

@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.

⚠️ Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Reviewed.

— 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. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment on lines +285 to +291
if (
opts.protectPinnedMemory &&
isProtectedPinnedMemoryPath(ctx.filePath, projectRoot, {
includeUserMemory: opts.includeUserMemory,
})
) {
return 'ManagedAutoMemory(edit: pinned memory is read-only)';

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] isProtectedPinnedMemoryPath is called a second time for the same context in the deny path — first by evaluateScopedDecision (which returned 'deny'), then again here by getScopedDenyRule via findMatchingDenyRule. — Failure scenario: each invocation performs synchronous realpathSync syscalls (up to 3 per call). The entire pinned-path check runs twice for every denied pinned-path operation.

Suggested fix: cache the pinned-check result so getScopedDenyRule does not re-evaluate what evaluateScopedDecision already computed, or restructure evaluate to return both the decision and the deny reason atomically.

— qwen3.7-max via Qwen Code /review

Comment on lines 242 to 252
if (
opts.protectPinnedMemory &&
isProtectedPinnedMemoryPath(ctx.filePath, projectRoot, {
includeUserMemory: opts.includeUserMemory,
})
) {
return 'deny';
}
return isAllowedMemoryPath(ctx.filePath, projectRoot, {
includeUserMemory: opts.includeUserMemory,
})

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] When protectPinnedMemory is true and the path is NOT in pinned (the common case for Dream writes to ordinary memory files), realpathExistingOrNew(filePath) runs twice on the same file — once inside isProtectedPinnedMemoryPath and again inside isAllowedMemoryPath. — Failure scenario: a Dream consolidation session producing 50 write operations to ordinary memory paths executes 50 extra synchronous realpathSync calls, each blocking the event loop.

Suggested fix: resolve the candidate path once before calling either function and pass the pre-resolved path as an optional parameter.

— qwen3.7-max via Qwen Code /review

Comment on lines +123 to +128
const resolvedPinnedRoot = realpathExistingOrNew(literalPinnedRoot);
return (
!!resolvedCandidate &&
!!resolvedPinnedRoot &&
isWithinRootCaseInsensitive(resolvedCandidate, resolvedPinnedRoot)
);

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] realpathExistingOrNew(literalPinnedRoot) resolves the pinned root's real path inside the .some() loop, once per memory root, on every write_file/edit evaluation. The pinned root directories are stable for the session's lifetime but are re-resolved from disk each time. — Failure scenario: during a Dream session with 2 memory roots and 100 tool evaluations, realpathSync is called ~200 times on stable directory paths that don't change.

Suggested fix: compute the resolved pinned root once at config creation time (in createMemoryScopedAgentConfig) and store it in the closure, or memoize it per memory root.

— qwen3.7-max via Qwen Code /review

ZijianZhang989
ZijianZhang989 previously approved these changes Jul 28, 2026

@ZijianZhang989 ZijianZhang989 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

@ZijianZhang989 ZijianZhang989 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.

— qwen3.7-max via Qwen Code /review

Comment on lines +168 to +173
await expect(
protectedPm.evaluate({
toolName: ToolNames.WRITE_FILE,
filePath: path.join(memoryRoot, 'pinned-notes', 'ordinary.md'),
}),
).resolves.toBe('allow');

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] Missing test for nested pinned/ directory remaining writable

The design doc explicitly states "nested directories such as memory/project/pinned/ are ordinary writable memory". The existing pinned-notes prefix-boundary test guards against over-broad matching of a path component that merely starts with pinned, but there is no assertion covering a legitimate nested directory named pinned/ inside a topic folder (e.g. memory/project/pinned/notes.md).

Failure scenario: a future refactor broadens the containment check (e.g. matching pinned/ as a path component anywhere rather than only at the memory root). A Dream consolidation session silently denies edits to memory/project/pinned/notes.md — a legitimate topic file — and no test breaks.

Suggested change
await expect(
protectedPm.evaluate({
toolName: ToolNames.WRITE_FILE,
filePath: path.join(memoryRoot, 'pinned-notes', 'ordinary.md'),
}),
).resolves.toBe('allow');
await expect(
protectedPm.evaluate({
toolName: ToolNames.WRITE_FILE,
filePath: path.join(memoryRoot, 'pinned-notes', 'ordinary.md'),
}),
).resolves.toBe('allow');
await expect(
protectedPm.evaluate({
toolName: ToolNames.WRITE_FILE,
filePath: path.join(memoryRoot, 'project', 'pinned', 'notes.md'),
}),
).resolves.toBe('allow');

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Maintainer re-verification — round 2, hardening commit 67c90a9f

Follow-up to my previous report: verified the new head e5f2636 (= 67c90a9f "harden pinned path protection" + a clean merge of main) with the same methodology — the actual production stack a forked Dream worker takes (real Config, real tool registry, the createApprovalModeOverride(YOLO) wrapper runForkedAgent applies, real CoreToolScheduler.schedule()evaluatePermissionFlow()), oracle = bytes on disk. This round the focus is the delta: do F1/F2/F3 actually die?

Verdict: all three findings from round 1 are fixed and each fix is pinned by a test that provably fails without it. Merge-ready from my side.

1. F1 (case-variant bypass) — fixed; A/B-proven on a real case-insensitive filesystem

Round 1 demonstrated F1 on macOS APFS. This round I built the regression environment on Linux — an ext4 -O casefold loopback mount (chattr +F, fs.existsSync(<memory>/PINNED/x) = true) — and ran the identical probe on both sides:

F1 A/B on casefold ext4

  • Pre-hardening 09c1f14, protection ON: read PINNED/architecture.md, then write_file / edit it → both succeed, and the pinned file's bytes are replaced (263af03ef287…aafdcce62a25… / 2bee135729d6…). F1 reproduced on Linux, same inode-aliasing mechanism as on APFS.
  • PR head: both requests are denied (execution_denied), bytes byte-identical, and the model receives the specific reason: ManagedAutoMemory(write_file: pinned memory is read-only).

One nuance worth recording: without a prior read, the pre-fix write was stopped only by the incidental read-before-write check (edit_requires_prior_read), not by the gate — further evidence the gate itself failed open. Dream reads files during consolidation, so read-then-write is the realistic flow, and there the bytes were clobbered.

The fix lowercases the whole path on both sides of the containment check, which is broader than the segment-only compare I suggested — the consequence is that on a genuinely case-sensitive filesystem a distinct PINNED/ directory is now also denied. That is the safe over-block direction for a deny-list, and it is documented (docs/users/features/memory.md: "Dream workers match the reserved directory name case-insensitively"). I probed the collision boundary: a file named Pinned.md at the memory root is still writable, so the over-block stops at the reserved directory name. (Scope note: I probed ASCII case variants; the reserved name is ASCII, where JS toLowerCase and filesystem casefolding agree.)

2. Adversarial battery — 13/13 on case-sensitive ext4, 5/5 on casefold, zero over-blocking

probe battery

All round-1 deny classes hold on the hardened code (lowercase, case variants, symlink alias, dot-segment traversal, dangling pinned/ symlink, shell redirect), and every legitimate Dream operation still succeeds (topic consolidation, new topic files, MEMORY.md reindex, pinned-notes/ similar prefix, Pinned.md name-collision file).

3. F3 (surviving mutations) — new tests kill all of them, plus the F1 fix itself

mutation matrix + gates

Re-ran the mutation matrix against memory-scoped-agent-config.test.ts at head; 4/4 killed:

mutant round 1 now killed by
revert case-insensitive compare (undo F1 fix) n/a killed new PINNED/ deny assertion
M2 — delete literal-containment branch survived killed new "dangling top-level pinned symlink" test
M5 — gut edit pinned deny message survived killed new findMatchingDenyRule assertion
M6 — gut write_file pinned deny message survived killed new findMatchingDenyRule assertions (both pinned tests)

4. F2 (docs gap) — closed

docs/users/features/memory.md:108 now states explicitly that only the top-level pinned/ directory is protected, that nested memory/project/pinned/ is ordinary writable memory, and that matching is case-insensitive. The design doc got the matching sentences too.

5. Regression gates

gate result
npx vitest run src/memory at head 477 passed (33 files, 0 failed)
test-merge into current origin/main (fceb755) + same suite merge clean, 477 passed
npx tsc --noEmit (core), head vs 09c1f14 baseline byte-identical error sets — zero new errors (the single shared error is a stale-node_modules turndown artifact of my environment, unrelated to the PR)
npx eslint (6 changed .ts files) exit 0
npx prettier --check (8 changed files) all match

The env-dependent memoryLifecycle.integration.test.ts failure I noted in round 1 does not reproduce here (isolated HOME), consistent with it being a hermeticity issue, not a product one.

Recommendation

Everything I asked for in round 1 landed, each with a test that fails if the fix regresses. The /dream slash-command scope is unchanged and still correctly deferred to #6801. LGTM — good to merge.

Harness is a single tsx script (drives the real scheduler flow per probe in a fresh project root) plus a mutation script; nothing was committed to the branch. Reproducible on request.

中文版本(点击展开)

维护者复验 — 第二轮,加固提交 67c90a9f

延续上一份报告的方法学,对新 head e5f2636(= 67c90a9f "harden pinned path protection" + 干净合并 main)复验:走 forked Dream worker 的真实生产链路(真实 Config、真实工具注册表、runForkedAgent 所用的 createApprovalModeOverride(YOLO) 包装、真实 CoreToolScheduler.schedule()evaluatePermissionFlow()),判定依据是磁盘字节。本轮聚焦增量:F1/F2/F3 是否真正被修复。

结论:第一轮的三个发现全部修复,且每个修复都有对应测试锁定(去掉修复该测试必然失败)。我这边认为可以合并。

1. F1(大小写变体绕过)— 已修复;在真实大小写不敏感文件系统上完成 A/B 证明

第一轮在 macOS APFS 上演示了 F1。本轮我在 Linux 上搭建了回归环境 —— ext4 -O casefold loopback 挂载(chattr +Ffs.existsSync(<memory>/PINNED/x) = true),两侧跑同一探针:

  • 加固前 09c1f14(保护开启): 先读 PINNED/architecture.mdwrite_file / edit → 两者均成功,pinned 文件字节被替换(263af03ef287…aafdcce62a25… / 2bee135729d6…)。F1 在 Linux 复现,与 APFS 上相同的同 inode 别名机制。
  • PR head: 两个请求均被拒绝execution_denied),字节逐字节一致,且模型收到明确原因:ManagedAutoMemory(write_file: pinned memory is read-only)

一个值得记录的细节:不带预读时,加固前的写入只是被 read-before-write 校验(edit_requires_prior_read)偶然拦下,而不是权限门 —— 这进一步证明权限门本身是 fail-open 的。Dream 在整合时会先读文件,因此"先读后写"才是真实流程,而该流程下字节确实被覆盖。

修复对包含性检查两侧做了整路径小写化,比我建议的"仅比较该段"更宽 —— 后果是在真正大小写敏感的文件系统上,一个独立的 PINNED/ 目录现在也会被拒绝。对黑名单而言这是安全的过拦截方向,且已写入文档(docs/users/features/memory.md:"Dream workers match the reserved directory name case-insensitively")。我探测了碰撞边界:memory 根下名为 Pinned.md 的文件仍可写,过拦截止步于保留目录名。(范围说明:我只探测了 ASCII 大小写变体;保留名是 ASCII,在此范围 JS toLowerCase 与文件系统 casefold 行为一致。)

2. 对抗性探测 — 大小写敏感 ext4 13/13、casefold 5/5,零误伤

第一轮的全部拒绝类别在加固代码上依然成立(小写、大小写变体、符号链接别名、点号回溯、断链 pinned/ 符号链接、shell 重定向),且 Dream 的全部正当操作仍然成功(主题整合、新建主题、MEMORY.md 重建索引、pinned-notes/ 相似前缀、Pinned.md 同名文件)。

3. F3(存活变异)— 新测试全部击杀,包括 F1 修复本身

对 head 重跑变异矩阵(memory-scoped-agent-config.test.ts),4/4 击杀:

变异 第一轮 本轮 击杀者
回退大小写不敏感比较(撤销 F1 修复) n/a 击杀 新增 PINNED/ 拒绝断言
M2 — 删除字面包含分支 存活 击杀 新增"dangling top-level pinned symlink"用例
M5 — 破坏 edit pinned 拒绝消息 存活 击杀 新增 findMatchingDenyRule 断言
M6 — 破坏 write_file pinned 拒绝消息 存活 击杀 新增 findMatchingDenyRule 断言(两个 pinned 用例)

4. F2(文档缺口)— 已关闭

docs/users/features/memory.md:108 现已明确:只有 managed-memory 根部的顶层 pinned/ 受保护,嵌套的 memory/project/pinned/ 是普通可写内存,且匹配是大小写不敏感的。设计文档也补充了对应句子。

5. 回归闸门

闸门 结果
head 上 npx vitest run src/memory 477 通过(33 个文件,0 失败)
测试性合并到当前 origin/mainfceb755)+ 同一套件 合并干净,477 通过
npx tsc --noEmit(core),head 对比 09c1f14 基线 错误集逐字节一致 —— 零新增错误(两侧共有的唯一报错是我环境 node_modules 陈旧导致的 turndown 产物,与 PR 无关)
npx eslint(6 个改动 .ts 文件) exit 0
npx prettier --check(8 个改动文件) 全部符合

第一轮提到的环境相关 memoryLifecycle.integration.test.ts 失败在本机不复现(隔离 HOME),与其为封闭性问题、而非产品问题的判断一致。

建议

第一轮提出的每一项都已落地,且各自带有回归即失败的测试。/dream 斜杠命令的范围未变,仍正确地留给 #6801。LGTM,可以合并。

验证脚手架为单个 tsx 脚本(每个探针在全新 project root 中驱动真实调度链路)加一个变异脚本;未向分支提交任何内容,可按需复现。

@yiliang114

Copy link
Copy Markdown
Collaborator

⚠️ Failed to process this request. Please re-mention the bot to retry.

Copy link
Copy Markdown
Contributor Author

The remaining review suggestions have been addressed in 942fbdd75:

  • resolve each candidate path once per evaluation
  • cache the resolved pinned roots at config creation
  • reuse the pinned decision when producing the deny reason
  • verify that nested memory/project/pinned/ remains writable

The focused and full memory tests, build, typecheck, bundle, ESLint, and Prettier all pass. I have also updated the PR body to match the current repository template while preserving the documented /dream scope boundary.

The automatic review rerun failed before checkout because the self-hosted runner could not remove .qwen/e2e-tests (EACCES), which appears unrelated to this PR. Could a maintainer please rerun the review and confirm or resolve the addressed threads?

@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. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max-preview via Qwen Code /review

@wenshao

wenshao commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Review — feat(memory): protect pinned files during forked Dream

Reviewed at fbecd38be4 (merge of main into codex/pinned-memory-dream-protection). I ran the three touched test files and mutation-tested the new gate in an isolated worktree rather than reading the diff alone.

Overview

Adds an opt-in protectPinnedMemory flag to createMemoryScopedAgentConfig, enabled only by the forked Dream planner. When set, edit/write_file are denied for paths under a top-level pinned/ directory in a managed-memory root, via three independent sub-checks: literal containment (case-insensitive), symlink-resolved containment, and a WeakMap reuse of the decision when producing the deny reason. Pinned files stay readable and stay in the recursive index; the Dream prompt gains three skip rules.

The design is sound and the enforcement is real. My one substantive concern is scope, not correctness.


What I verified locally

Check Result
vitest run src/memory/{memory-scoped-agent-config,dreamAgentPlanner,indexer}.test.ts 35/35 pass
Prettier on all 8 changed files clean
Mutation M1 — drop toLowerCase() in isWithinRootCaseInsensitive killed (case-variant assertion)
Mutation M2 — return false for the resolved-alias branch killed (2 tests: alias + pinned-dir-symlink)
Mutation M3 — short-circuit the literal branch killed (dangling-symlink test)
Shell surface: is the gate bypassable via redirection? noWRITE_REDIRECT_OPERATORS (shellAstParser.ts:161) covers > >> &> &>> >|, so echo x > pinned/f.md is not read-only ⇒ denied
Un-gated mutating tool in the Dream tool list? no — list is exactly read_file, grep_search, glob, list_directory, run_shell_command, write_file, edit (dreamAgentPlanner.ts:112-119)
Is the pinnedDecisionCache WeakMap dead code? nopermissionFlow.ts:78 and :104 pass the same pmCtx object identity to evaluate then findMatchingDenyRule

All three sub-checks are independently load-bearing — good test design, not a redundant belt-and-braces.

TOCTOU probe (a concern the snapshot-at-creation optimization invites — it holds up): the PR caches pinnedRoots when the config is built. I probed creating pinned/ after config creation, and creating a symlink alias into it after config creation. Both still deny — because realpathExistingOrNew on a not-yet-existing pinned/ snapshots the would-be resolved path (nearest existing ancestor + literal suffix), so a later-created alias still resolves into it. Worth a one-line comment at createPinnedMemoryRoots, since it is load-bearing and non-obvious.


Findings

1. Medium — the other automated memory writer has neither the gate nor the prompt rule

extractionAgentPlanner.ts:260 builds its scoped config as:

const scopedConfig = createMemoryScopedAgentConfig(config, projectRoot, {
  allowShell: true,          // ← no protectPinnedMemory
});

Facts I confirmed:

  • The extractor carries write_file and edit (extractionAgentPlanner.ts:276-284), and covers both project and user memory roots (includeUserMemory defaults to true).
  • With those exact options, evaluate({ write_file, <memoryRoot>/pinned/architecture.md })allow, and editallow. (This PR's own unprotectedPm assertion at the end of the big new test says the same thing.)
  • grep -i pinned extractionAgentPlanner.ts remember.tszero hits. No gate, and no prompt rule either.
  • Its prompt actively steers toward rewriting existing files: "Prefer updating an existing memory file over creating a duplicate. Check both directories for an existing entry before creating a new one." (extractionAgentPlanner.ts:159)
  • And this PR deliberately keeps pinned files in the index — the new indexer.test.ts case asserts [Canonical Architecture](pinned/architecture.md) lands in MEMORY.md. So the extractor reads that index and sees the pinned file precisely as an update candidate.

Net effect: extraction runs after every qualifying session — far more often than Dream — and can rewrite the exact file this feature promises to preserve. The design doc's "Extraction and explicit remember operations retain their current behavior" is an explicit scope call, but docs/users/features/memory.md doesn't carry that caveat; it says "Put hand-curated documents that Dream should preserve under pinned/", which a user will reasonably read as "protected".

Suggested resolution, in preference order:

  1. Set protectPinnedMemory: true at extractionAgentPlanner.ts:260 and add one prompt line. Two lines. The gate already handles the user-memory root, so extraction's default includeUserMemory: true is covered for free — and it makes the ~/.qwen/memories/pinned/ example in the new docs section actually enforced by a production caller (today no production caller enables protectPinnedMemory with includeUserMemory: true, so that path is test-only).
  2. Or, if extraction is intentionally out of scope, say so in docs/users/features/memory.md next to the existing /dream note — the doc already carries one honest caveat, so a second one fits the established tone.

I'd leave explicit /remember unprotected either way: that's user-initiated intent, not an automated rewrite.

2. Low / informational — hardlink aliases are outside what path resolution can cover

Probe: hardlink memory/project/alias.mdmemory/pinned/architecture.md, then evaluate({ write_file, alias })allow, and writing through the alias changed the pinned file's bytes (same inode). fs.realpathSync cannot see hardlinks, so this is not fixable by any path-resolution scheme.

Not a blocker, and not a doc error — the design doc correctly scopes its claim to "aliases that resolve through a symlink into pinned/". Practically unreachable for the Dream worker anyway: write_file/edit can't create hardlinks and the read-only shell gate blocks ln. Flagging it only so the boundary is written down before someone reuses this gate under a stronger threat model.

3. Low — the pinnedDecisionCache WeakMap costs more than it saves

It isn't dead code (verified above), but the ledger is thin:

  • Saves: one realpathSync — on a tool call that is already being denied.
  • Costs: two extra params threaded through evaluateScopedDecision and getScopedDenyRule; a defaulted third parameter on isProtectedPinnedMemoryPath that performs I/O (surprising in a default initializer); and a cached boolean that can silently disagree with a recomputation if the filesystem shifts between the two calls.
  • The other findMatchingDenyRule call site (coreToolScheduler.ts:2142) builds a fresh { toolName } literal, so it is always a cache miss — the cache helps exactly one of two call sites.

Dropping the WeakMap and just calling isProtectedPinnedMemoryPath(ctx.filePath, pinnedRoots) in getScopedDenyRule removes ~15 lines, drops the defaulted-I/O param, and makes both call sites structurally identical. pinnedRoots threading is fine as-is — it matches how projectRoot/opts already flow.

4. Low — isWithinRootCaseInsensitive is broader than the design doc describes

The doc says "Match the reserved top-level directory name case-insensitively", but the implementation lowercases both full absolute paths. This is fail-closed — lowercasing both sides can only add matches, never remove them — and I could not construct a false-allow; the only reachable over-deny is a case variant of pinned itself directly under the memory root, which is the intent. String.prototype.toLowerCase is locale-independent (unlike toLocaleLowerCase), so the Turkish dotless-ı class of bug doesn't apply.

Worth two lines of comment on the helper stating exactly that, because the next reader will ask both questions.


Nits

  • dreamAgentPlanner.test.ts — the new rm <pinned> SHELL assertion passes on main too. It's pre-existing read-only-shell behavior, not evidence of the new gate. Fine as a regression guard, but add a comment saying so, or a future reader will conclude the pinned gate covers shell.
  • The 170-line protects pinned memory and aliases… case bundles ~15 assertions across three different permission managers (protectedPm, allMemoryPm, unprotectedPm). The first failure masks the rest — M1 above surfaced as one opaque red test. Splitting out the allMemoryPm (user-memory) and unprotectedPm (negative control) arms into their own it()s would localize failures and matches the single-concern style of the file's other cases. The negative control in particular deserves its own name — it's the assertion that proves the flag is doing the work.
  • Constant usage is inconsistent across the three test files. indexer.test.ts and dreamAgentPlanner.test.ts import AUTO_MEMORY_PINNED_DIRNAME; memory-scoped-agent-config.test.ts hardcodes 'pinned'. Either convention is defensible — pick one per file.
  • getScopedDenyRule: the pinned branch is duplicated verbatim for EDIT and WRITE_FILE. Could collapse into one block keyed off ctx.toolName, mirroring how allowedRoots is computed once above.
  • Deny-reason precedence: if pinned/ is a symlink pointing outside memory, a write to the resolved target now reports ManagedAutoMemory(edit: pinned memory is read-only) rather than the more accurate only within <root>. Both deny, so cosmetic.
  • Design doc placement is correctdocs/design/YYYY-MM-DD-*.md matches the existing 187 files, and design docs aren't indexed anywhere, so nothing to register.
  • Windows path-alias forms (8.3 short names, trailing dots, ADS) are untested, consistent with the PR body's "not tested locally". pinned is 6 chars so no 8.3 alias is generated; low risk.

Verdict

Correctness of what's implemented: good. Test quality: above average — every sub-check is mutation-verified load-bearing, and the negative control is present. Security posture: the mutating surface reachable by the forked Dream worker (write_file, edit, redirection-capable shell) is genuinely closed.

Requesting one change: resolve finding #1 — either enable the flag for the extraction agent (2 lines, and it makes the user-memory pinned example real) or add the caveat to docs/users/features/memory.md. Without one of those, pinned/ is protected against the rarer writer and open to the frequent one, and the user-facing doc doesn't say which.

Findings #2#4 and the nits are non-blocking.

中文说明

审阅 — feat(memory): protect pinned files during forked Dream

fbecd38be4main 合入 codex/pinned-memory-dream-protection)上审阅。我在隔离 worktree 中实际运行了三个改动的测试文件,并对新门禁做了变异测试,而不是只读 diff。

概述

createMemoryScopedAgentConfig 新增可选 protectPinnedMemory 开关,仅由 Forked Dream Planner 启用。启用后,对管理记忆根目录下顶层 pinned/ 目录中的路径拒绝 edit/write_file,通过三个相互独立的子检查实现:字面路径包含(不区分大小写)、符号链接解析后包含、以及用 WeakMap 在生成拒绝原因时复用判断结果。Pinned 文件仍可读、仍进入递归索引;Dream 提示词新增三条跳过规则。

设计是合理的,强制保护是真实生效的。我唯一实质性的顾虑是范围,而不是正确性。

本地验证结果

检查项 结果
vitest run src/memory/{memory-scoped-agent-config,dreamAgentPlanner,indexer}.test.ts 35/35 通过
对 8 个改动文件跑 Prettier 通过
变异 M1 — 去掉 isWithinRootCaseInsensitive 里的 toLowerCase() 被杀死(大小写变体断言)
变异 M2 — 解析别名分支改为 return false 被杀死(2 个测试:别名 + pinned 目录符号链接)
变异 M3 — 短路字面路径分支 被杀死(悬空符号链接测试)
Shell 层面能否用重定向绕过门禁? 不能WRITE_REDIRECT_OPERATORSshellAstParser.ts:161)覆盖 > >> &> &>> >|,因此 echo x > pinned/f.md 不算只读 ⇒ 被拒绝
Dream 工具列表中是否有未被门禁覆盖的写工具? 没有 — 列表正好是 read_file, grep_search, glob, list_directory, run_shell_command, write_file, editdreamAgentPlanner.ts:112-119
pinnedDecisionCache 这个 WeakMap 是不是死代码? 不是permissionFlow.ts:78:104同一个 pmCtx 对象先传给 evaluate 再传给 findMatchingDenyRule

三个子检查各自都是必要的(都能被变异杀死),测试设计良好,不是冗余的多重保险。

TOCTOU 探测("创建时快照 pinned 根目录"这个优化会引出的疑问 — 结论是成立的): PR 在构建配置时缓存 pinnedRoots。我测试了在配置创建之后才创建 pinned/,以及在配置创建之后才创建指向它的符号链接别名。两种情况仍然 deny — 因为对尚不存在的 pinned/realpathExistingOrNew 快照的是"将来的"解析路径(最近的已存在祖先 + 字面后缀),所以后创建的别名仍会解析进去。建议在 createPinnedMemoryRoots 处加一行注释,因为这个行为是承重的且不直观。

发现的问题

1. 中等 — 另一个自动记忆写入者既没有门禁也没有提示词规则

extractionAgentPlanner.ts:260 的 scoped 配置是:

const scopedConfig = createMemoryScopedAgentConfig(config, projectRoot, {
  allowShell: true,          // ← 没有 protectPinnedMemory
});

我确认的事实:

  • Extractor 同时带 write_file editextractionAgentPlanner.ts:276-284),并且覆盖项目和用户两个记忆根目录(includeUserMemory 默认为 true)。
  • 用这组完全相同的选项,evaluate({ write_file, <memoryRoot>/pinned/architecture.md })alloweditallow。(本 PR 自己新增测试末尾的 unprotectedPm 断言其实说的就是同一件事。)
  • grep -i pinned extractionAgentPlanner.ts remember.ts零命中。既没有门禁,也没有提示词规则。
  • 它的提示词还主动引导去改写已有文件:"Prefer updating an existing memory file over creating a duplicate. Check both directories for an existing entry before creating a new one."extractionAgentPlanner.ts:159
  • 而本 PR 有意让 pinned 文件留在索引里 — 新增的 indexer.test.ts 断言 [Canonical Architecture](pinned/architecture.md) 会进入 MEMORY.md。于是 Extractor 读到该索引,正好把 pinned 文件当成待更新候选。

综合效果:Extraction 在每次符合条件的会话后都会运行 —— 频率远高于 Dream —— 而它可以改写本功能承诺要保护的那个文件。设计文档里 "Extraction and explicit remember operations retain their current behavior" 是一个明确的范围决定,但 docs/users/features/memory.md 没有带上这个说明;它写的是 "Put hand-curated documents that Dream should preserve under pinned/",用户会合理地理解为"受保护"。

建议的处理方式,按优先级:

  1. extractionAgentPlanner.ts:260 设置 protectPinnedMemory: true,并加一行提示词。 两行改动。门禁已经能处理用户记忆根目录,所以 Extraction 默认的 includeUserMemory: true 顺带就被覆盖了 —— 同时也让新文档里 ~/.qwen/memories/pinned/ 这个例子真正被强制执行(目前没有任何生产调用方同时启用 protectPinnedMemoryincludeUserMemory: true,所以那条路径只存在于测试中)。
  2. 或者,如果确实有意把 Extraction 排除在范围外,就在 docs/users/features/memory.md 里紧挨现有 /dream 注记写清楚 —— 文档已经带了一条诚实的说明,再加一条风格上完全吻合。

两种做法下我都建议显式 /remember 保持不受保护:那是用户主动意图,不是自动改写。

2. 低 / 参考性 — 硬链接别名超出路径解析能覆盖的范围

探测:把 memory/project/alias.md 硬链接到 memory/pinned/architecture.md,然后 evaluate({ write_file, alias })allow,并且通过别名写入确实改变了 pinned 文件的内容(同一个 inode)。fs.realpathSync 看不到硬链接,所以任何基于路径解析的方案都无法修复。

不是阻塞项,也不是文档错误 —— 设计文档正确地把结论限定为 "通过符号链接解析进 pinned/ 的别名"。对 Dream Worker 而言实际上也不可达:write_file/edit 无法创建硬链接,只读 Shell 门禁挡掉 ln。提出来只是为了在有人以更强威胁模型复用这个门禁之前,把边界写下来。

3. 低 — pinnedDecisionCache 这个 WeakMap 的代价大于收益

它不是死代码(前面已验证),但账面很薄:

  • 省下: 一次 realpathSync —— 而且是在一个本来就要被拒绝的工具调用上。
  • 代价: 两个额外参数穿过 evaluateScopedDecisiongetScopedDenyRuleisProtectedPinnedMemoryPath 上一个会执行 I/O 的默认参数(放在默认初始化式里很意外);以及一个缓存布尔值,如果两次调用之间文件系统发生变化,它可能与重新计算的结果静默不一致。
  • 另一个 findMatchingDenyRule 调用点(coreToolScheduler.ts:2142)构造的是全新的 { toolName } 字面量,所以永远缓存未命中 —— 这个缓存只对两个调用点中的一个有用。

去掉 WeakMap、在 getScopedDenyRule 里直接调用 isProtectedPinnedMemoryPath(ctx.filePath, pinnedRoots),可以少约 15 行、去掉那个执行 I/O 的默认参数,并让两个调用点结构一致。pinnedRoots 的参数传递保持现状没问题 —— 它和现有的 projectRoot/opts 传递方式一致。

4. 低 — isWithinRootCaseInsensitive 的范围比设计文档描述的更宽

文档说 "不区分大小写地匹配保留的顶层目录",但实现把两条完整绝对路径都转成了小写。这是 fail-closed 的 —— 两边同时转小写只会增加匹配,不会减少 —— 我也构造不出 false-allow;唯一可达的过度拒绝是记忆根目录下 pinned 本身的大小写变体,而那正是预期行为。String.prototype.toLowerCase 与 locale 无关(不同于 toLocaleLowerCase),所以土耳其语无点 ı 那类 bug 不适用。

建议在该 helper 上加两行注释把这两点写清楚,因为下一个读者一定会问这两个问题。

小问题

  • dreamAgentPlanner.test.ts 新增的 rm <pinned> SHELL 断言在 main 上同样通过。 它验证的是既有的只读 Shell 行为,不是新门禁的证据。作为回归保护没问题,但请加注释说明,否则后来的读者会以为 pinned 门禁覆盖了 Shell。
  • 那个 170 行的 protects pinned memory and aliases… 用例把约 15 个断言、跨三个不同 permission managerprotectedPmallMemoryPmunprotectedPm)打包在一起。第一个失败会掩盖其余 —— 上面的 M1 表现为一个笼统的红色测试。把 allMemoryPm(用户记忆)和 unprotectedPm(负对照)两块拆成各自的 it(),能定位失败,也更符合该文件其他用例的单一关注点风格。负对照尤其值得有自己的名字 —— 它是证明这个开关真正起作用的断言。
  • 三个测试文件里常量用法不一致。 indexer.test.tsdreamAgentPlanner.test.ts 导入了 AUTO_MEMORY_PINNED_DIRNAMEmemory-scoped-agent-config.test.ts 硬编码 'pinned'。两种约定都说得通 —— 但每个文件内部请统一。
  • getScopedDenyRule:pinned 分支在 EDITWRITE_FILE 下逐字重复。 可以按 ctx.toolName 合并成一块,和上面 allowedRoots 只计算一次的写法保持一致。
  • 拒绝原因的优先级: 如果 pinned/ 是指向记忆目录外部的符号链接,对解析目标的写入现在会报 ManagedAutoMemory(edit: pinned memory is read-only),而不是更准确的 only within <root>。两者都拒绝,所以只是措辞问题。
  • 设计文档位置正确 —— docs/design/YYYY-MM-DD-*.md 与现有 187 个文件一致,且设计文档没有任何索引需要登记。
  • Windows 路径别名形式(8.3 短名、结尾点、ADS)未测试,与 PR 描述中"未在本机测试"一致。pinned 只有 6 个字符,不会生成 8.3 别名,风险很低。

结论

已实现部分的正确性:良好。测试质量:高于平均 —— 每个子检查都经变异验证是承重的,负对照也在。安全态势:Forked Dream Worker 可触及的写入面(write_fileedit、能重定向的 Shell)确实被封住了。

请求一处修改:处理发现 #1 —— 要么为 Extraction Agent 启用该开关(两行,并让用户记忆的 pinned 示例变成真的),要么在 docs/users/features/memory.md 里加上说明。否则 pinned/ 只防住了较少见的写入者,对高频写入者是敞开的,而用户文档没有说清是哪一个。

发现 #2#4 及小问题均不阻塞。

@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. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

中文说明

已审查。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

— qwen3.8-max-preview via Qwen Code /review

@wenshao

wenshao commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Local verification report — real end-to-end, not mocked

I built a local harness to verify this PR against a real forked Dream agent and a real filesystem, because every test in the PR asserts on a PermissionManager.evaluate() return value — none of them proves that a denied decision actually stops a write from landing on disk. Verified at head cd5219355d, A/B against merge-base 289fe94c9e, on Linux x64 / Node 22.

Verdict: the boundary does what the PR claims. I'm happy to merge it. Details, evidence and a few non-blocking notes below.


1. The decisive test — a real Dream agent trying to overwrite pinned memory

I drove planManagedAutoMemoryDreamByAgentrunForkedAgentAgentHeadless → real ToolRegistry / WriteFileTool / EditTool / CoreToolScheduler, with a scripted fake OpenAI model that deliberately issues six mutations. The oracle is the bytes on disk, not a permission decision.

real forked Dream agent vs pinned memory

merge-base 289fe94 PR cd52193
write_filepinned/architecture.md overwritten blocked
editpinned/deep/nested.md overwritten blocked
write_file<symlink>/architecture.md overwritten blocked
write_filepinned/brand-new.md (new) created not created
run_shell_command rm -f pinned/… blocked (pre-existing) blocked
write_fileproject/ordinary.md (control) wrote wrote

4 of 4 pinned mutations land before the PR; 0 of 4 after. The control write lands in both arms — without it, "nothing was written" would be a vacuous pass. The agent is told exactly why:

Tool "write_file" is denied by permission rules.
Matching deny rule: "ManagedAutoMemory(write_file: pinned memory is read-only)".

2. Path-containment probe matrix — real symlinks, real dangling links

25 probes against a real temp memory tree through the real createMemoryScopedAgentConfig permission manager. B* = advertised behaviour; A* = adversarial aliases, several of which the PR's own tests don't cover (file-symlink into a pinned file, two-hop symlink chain, dot-segment / .. traversal, double slash, trailing slash, the pinned directory itself).

path containment probe matrix

Everything lands where it should, and the three "must stay writable" cases (pinned-notes/, nested project/pinned/, MEMORY.md) are unaffected.

I also ran three configurations the PR never exercises — all correct:

result
QWEN_CODE_MEMORY_LOCAL=1 (<project>/.qwen/memory) — pinned, case variant, ordinary deny / deny / allow ✅
pinned/ created after the scoped config snapshotted its roots deny ✅ (literal containment carries it)
memory root reached through a symlink — pinned via link path and via real path deny / deny ✅

(In that last case an ordinary file is also denied, but that is pre-existing resolveTrustedMemoryRoot anchor-guard behaviour — identical on the merge-base, not caused by this PR.)

3. The second line of defence, and residual surface

The docs claim the pre-existing read-only shell policy blocks command-line mutation. I threw 17 mutation vectors at it — rm, >, >>, tee, sed -i, cp, mv, truncate, dd, find -delete, xargs rm, sh -c, $(…), python3 -c open(...,'w'), perl -pi, ln -sf, chmod. All 17 denied; the two read-only controls (cat, ls) still allowed. Relative, backslash-separated, empty and undefined paths all fail closed, and rebuildManagedAutoMemoryIndex still indexes pinned documents.

shell battery and residual surface

4. Repository checks

check result
npx vitest run src/memory (packages/core) 33 files, 482 tests passed
npx tsc --noEmit -p packages/core/tsconfig.json clean
npx eslint --max-warnings 0 on the 4 changed source files clean

Non-blocking notes

  1. Hard links are genuinely not covered — confirmed empirically (probe A6: allow). This matches the design doc exactly, and the worker can't create one (ln is denied, and it only has write_file/edit), so it needs a pre-existing user-created hard link. No action needed; I'm recording it because it's now measured rather than assumed.

  2. A symlink placed inside pinned/ doesn't protect its target. Writing the link path is denied, but writing the ordinary memory file it points at is allowed — so content reachable through pinned/ can still be rewritten if a user pins by symlink. One sentence in docs/users/features/memory.md ("pin files, not symlinks to files elsewhere in memory") would close the expectation gap.

  3. The gate is keyed to two tool names. evaluateScopedDecision handles EDIT and WRITE_FILE; anything else falls to default. I confirmed notebook_edit and save_memory return default from the scoped manager. Neither is in the Dream or extraction tool list today, so this is not reachable — but note that a future mutating tool added to those lists would silently bypass both the pinned gate and the existing memory-root confinement. A short comment next to the tool list, or a test asserting the mutating-tool set, would make that trap visible. (tool-names.ts already warns about exactly this class of manual-allowlist drift.)

  4. Windows has no signal yet. The author marked Windows/Linux "not tested locally", and on this repo Test (windows-latest) / Test (macos-latest) are skipping on PR runs — the first Windows result arrives in the merge queue. Case-insensitive matching is precisely the Windows-relevant behaviour here, so it's worth watching the queue run rather than assuming green. I've covered Linux above.

  5. Cosmetic: for a dangling symlink in ordinary memory pointing into pinned/, the decision is correctly deny but the reason reported is ManagedAutoMemory(write_file: only within <root>) rather than the pinned-specific message. Right outcome, slightly misleading explanation.

  6. /dream remains prompt-only, as the PR states — dreamCommand returns submit_prompt, so it runs on the main Agent with no per-turn gate. Since that's the surface users actually invoke by hand, the note in the docs is doing real work; agreed it's a separate cross-surface permission design and shouldn't block this.

How to reproduce

The E2E harness is ~150 lines: create a temp project root with QWEN_CODE_MEMORY_BASE_DIR pointed at a temp dir, seed memory/pinned/… plus a symlink alias, start integration-tests/fake-openai-server.ts (scripted to emit read_file calls first so edit's read-before-write precondition is met, then the mutation batch), build a real Config with generationConfig: { authType: 'openai', apiKey, baseUrl } seeded directly (the env-var fallback lives in the CLI layer, not in Config), then call planManagedAutoMemoryDreamByAgent and diff the files. Run it with node --import tsx from a worktree with node_modules symlinked — no build required, since tsx drives packages/core/src directly. Running the identical script in a merge-base worktree gives the before column.

中文说明

本地验证报告 —— 真实端到端,非 Mock

我在本地搭建了验证环境,用真实的 Forked Dream Agent 和真实文件系统来验证本 PR。原因是:PR 中所有测试断言的都是 PermissionManager.evaluate() 的返回值,没有任何一个测试能证明「拒绝决策真的阻止了写入落盘」。验证基于 head cd5219355d,与 merge-base 289fe94c9e 做 A/B 对比,环境为 Linux x64 / Node 22。

结论:该边界确实实现了 PR 所声称的效果,我同意合并。 证据与若干非阻塞说明如下。


1. 决定性测试 —— 真实 Dream Agent 尝试覆盖 pinned memory

我驱动了完整真实链路:planManagedAutoMemoryDreamByAgentrunForkedAgentAgentHeadless → 真实 ToolRegistry / WriteFileTool / EditTool / CoreToolScheduler,并用一个脚本化的 fake OpenAI 模型故意发起 6 次改写。判定依据是磁盘上的字节内容,而不是权限判定的返回值。

real forked Dream agent vs pinned memory

merge-base 289fe94 PR cd52193
write_filepinned/architecture.md 被覆盖 已拦截
editpinned/deep/nested.md 被覆盖 已拦截
write_file<符号链接>/architecture.md 被覆盖 已拦截
write_filepinned/brand-new.md(新文件) 被创建 未创建
run_shell_command rm -f pinned/… 已拦截(原有策略) 已拦截
write_fileproject/ordinary.md(对照组) 写入成功 写入成功

修改前 4 次 pinned 改写全部成功;修改后 0 次成功。对照组写入在两个分支都成功——如果没有这个对照,「没有任何文件被写入」就可能是一个空洞的通过。Agent 收到的拒绝原因也很明确:

Tool "write_file" is denied by permission rules.
Matching deny rule: "ManagedAutoMemory(write_file: pinned memory is read-only)".

2. 路径包含判定矩阵 —— 真实符号链接、真实悬空链接

在真实临时 memory 目录树上,通过真实的 createMemoryScopedAgentConfig 权限管理器跑了 25 个探针。B* 为 PR 声称的行为;A* 为对抗性别名,其中多项是 PR 自带测试未覆盖的(指向 pinned 文件的文件符号链接、两跳符号链接、... 路径段、双斜杠、结尾斜杠、pinned 目录本身)。

path containment probe matrix

结果全部符合预期,三个「必须保持可写」的场景(pinned-notes/、嵌套 project/pinned/MEMORY.md)也未受影响。

我还额外验证了 PR 从未覆盖的三种配置,结果均正确:

结果
QWEN_CODE_MEMORY_LOCAL=1<project>/.qwen/memory)—— pinned / 大小写变体 / 普通文件 deny / deny / allow ✅
scoped 配置快照 pinned 根目录之后才创建 pinned/ deny ✅(字面路径包含判定兜住了)
memory 根目录本身是符号链接 —— 分别用链接路径和真实路径访问 pinned deny / deny ✅

(最后一种情况下普通文件也会被拒绝,但这是 resolveTrustedMemoryRoot anchor guard 的既有行为——merge-base 上表现完全一致,并非本 PR 引入。)

3. 第二道防线与残余攻击面

文档声称既有的只读 Shell 策略可以阻止命令行改写。我用 17 种改写手段做了测试:rm>>>teesed -icpmvtruncateddfind -deletexargs rmsh -c$(…)python3 -c open(...,'w')perl -piln -sfchmod。17 项全部被拒绝;两个只读对照(catls)仍然放行。相对路径、反斜杠分隔符、空路径和 undefined 路径均 fail-closed,rebuildManagedAutoMemoryIndex 也依然会收录 pinned 文档。

shell battery and residual surface

4. 仓库检查

检查项 结果
npx vitest run src/memory(packages/core) 33 个文件,482 个测试通过
npx tsc --noEmit -p packages/core/tsconfig.json 通过
对 4 个改动源文件执行 npx eslint --max-warnings 0 通过

非阻塞说明

  1. 硬链接确实不在保护范围内 —— 已实测确认(探针 A6:allow)。这与设计文档完全一致,且 Worker 无法自行创建硬链接(ln 被拒绝,它只有 write_file/edit),必须依赖用户预先创建的硬链接。无需处理;我记录下来只是因为现在这一点是被测量出来的,而不是假设出来的。

  2. 放在 pinned/ 内部的符号链接无法保护其指向的目标。 写链接路径会被拒绝,但直接写它指向的那个普通 memory 文件是允许的——因此如果用户用符号链接的方式 pin 文件,通过 pinned/ 可达的内容仍可能被改写。建议在 docs/users/features/memory.md 加一句说明(「请 pin 文件本身,而不是指向 memory 中其他位置的符号链接」)来消除预期落差。

  3. 该门禁是按工具名硬编码的。 evaluateScopedDecision 只处理 EDITWRITE_FILE,其余一律落到 default。我确认了 notebook_editsave_memory 在 scoped 管理器下返回 default。这两个工具目前都不在 Dream / extraction 的工具列表里,所以当前不可达——但需要注意:将来若给这些 Agent 增加任何有写能力的工具,会同时绕过 pinned 门禁既有的 memory 根目录限制。建议在工具列表旁加一行注释,或加一个断言「可改写工具集合」的测试,把这个坑显式化。(tool-names.ts 本身就警告过这一类手工 allowlist 漂移问题。)

  4. Windows 目前没有任何信号。 作者标注了 Windows/Linux「未在本机测试」,而本仓库的 Test (windows-latest) / Test (macos-latest) 在 PR 流水线上是 skipping——第一个 Windows 结果要到 merge queue 才会出现。而大小写不敏感匹配恰恰是这里与 Windows 最相关的行为,所以建议盯一下 merge queue 的那次运行,不要默认它是绿的。Linux 侧我已覆盖。

  5. 一处文案问题: 对于普通 memory 目录下指向 pinned/ 的悬空符号链接,判定结果正确为 deny,但返回的原因是 ManagedAutoMemory(write_file: only within <root>),而不是 pinned 专属的那条消息。结果正确,解释略有误导。

  6. /dream 仍然只有提示词保护,与 PR 描述一致 —— dreamCommand 返回的是 submit_prompt,因此运行在主 Agent 上,没有 per-turn 门禁。由于这恰恰是用户会手动调用的入口,文档里的那条 Note 是有实际价值的;我同意这属于独立的跨界面权限设计,不应阻塞本 PR。

复现方法

E2E harness 约 150 行:创建临时 project root,把 QWEN_CODE_MEMORY_BASE_DIR 指向临时目录,预置 memory/pinned/… 以及一个符号链接别名;启动 integration-tests/fake-openai-server.ts(脚本先发 read_file 调用以满足 edit 的「先读后写」前置条件,再发改写批次);直接用 generationConfig: { authType: 'openai', apiKey, baseUrl } 构造真实 Config(环境变量回退逻辑在 CLI 层,不在 Config 里);然后调用 planManagedAutoMemoryDreamByAgent 并 diff 文件内容。用 node --import tsx 在一个 node_modules 做了软链的 worktree 里运行即可,无需 build —— tsx 直接驱动 packages/core/src。把同一个脚本放到 merge-base 的 worktree 里跑,就得到对比表格中的「修改前」那一列。


Verified with Claude Code (Claude Opus 5, 1M context).

@wenshao

wenshao commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Aug 1, 2026
Merged via the queue into QwenLM:main with commit 4dc50b1 Aug 1, 2026
61 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.3.

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.

6 participants