feat(serve): allow relocating session attachment storage via env var - #10066
feat(serve): allow relocating session attachment storage via env var#10066ytahdn wants to merge 14 commits into
Conversation
Adds QWEN_SERVE_SESSION_ATTACHMENTS_ROOT, which stores session attachments under <root>/<projectHash>/attachments instead of the runtime temp dir so operators can pin them to a dedicated volume. Reads and removes that miss the configured root fall back to the default dir so pre-switch attachments stay readable and removable; archive cleanup removes both roots. New uploads never shadow a legacy fallback name, and both roots are removed via the same tombstone dance so a deletion racing a session restore cannot sweep up a successor directory.
|
|
|
Re-run after the fix commit
Moving on to code review. 🔍 中文说明修复提交
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe fix commit (
No new findings on this pass. TestingEvidence carried here: the PR's own CI via API, plus the ubuntu Test job's log — no PR code was built or run locally.
Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining end-to-end claim: Not verified: Windows/Linux runtime behavior (those CI lanes no longer run for any PR after #10059; author tested macOS only, per the PR description) and the daemon-restart migration flow end-to-end. 中文说明代码审查修复提交（
本轮无新发现。 测试此处证据：通过 API 读取的 PR 自身 CI，以及 ubuntu Test job 的日志——未在本地构建或运行任何 PR 代码。
CI 明细见上方表格（随 CI 完成自动更新）。 沙箱验证可以定论剩余的端到端声明： 未验证：Windows/Linux 运行时行为（#10059 之后这些 CI 档对所有 PR 都不再运行；按 PR 描述作者仅在 macOS 测试）；端到端的 daemon 重启迁移流程。 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
Confidence: 4/5 — the previous blocker is fixed exactly as prescribed, CI is green where it was red, and the storage work underneath remains the careful, thoroughly tested piece from the first round; what's left is non-blocking (one queued SDK lane, an open product question, macOS-only manual verification). The fix commit is three lines and does precisely what the last review asked: the resolver moved behind the post-listen dynamic import, next to the documented The one item I genuinely cannot settle from the diff is whether an attachments-only knob earns its keep next to Approval deferred until CI lands green on 中文说明置信度：4/5 —— 上次的阻塞项已完全按指示修复，CI 由红转绿，底层的存储工作依然是首轮审查中那份细致、测试充分的工作；剩下的都不阻塞（一个排队中的 SDK 档、一个待定的产品问题、仅 macOS 的手动验证）。 修复提交只有三行，精确做到了上次审查的要求：解析器移到 listen 之后的动态导入处，紧挨已有文档记载的 我真正无法从 diff 定论的一点是：在 批准将推迟到 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes on one mechanical blocker: CI is red on this commit — Test (ubuntu-latest, Node 22.x) fails at check:serve-fast-path-bundle because the new module-scope import in run-qwen-serve.ts (session-attachments-root.ts → Storage from the core package root) drags the core vendor closure into the serve fast-path chunk. A dynamic import at the use sites (the file already documents this pattern) should clear it; then CI needs to run green end-to-end — the unit suite never ran on this commit. Full details and the log excerpt are in the review comments above.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 47 passed · 0 failed · 47 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：47 通过 · 0 失败 · 47 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10066 Deep Verification —
|
| Cell | Oracle | base (0756be0) |
head (da506f7) |
|---|---|---|---|
| A/A default path | put/read/remove/archive on default root | works | identical (assertions equal) |
| read-fallback | read('report.txt') after switch |
undefined |
LEGACY bytes |
| assertReference-fallback | validate a pre-switch reference | throws session_attachment_gone |
validates |
| upload-no-shadow | re-upload a legacy-held name | shadows: id report.txt, old id now serves NEW bytes |
fresh id report (1).txt; legacy bytes intact, old id still serves LEGACY |
| remove-fallback | remove('report.txt') |
false, file stays |
true, file gone |
| archive-both-roots | delete() |
primary removed, legacy dir left behind | both session dirs removed |
| copyFrom-union | branch copy from a switched session | primary only (b.txt lost) |
union of both roots; primary content wins on name collision |
| statSizeStrict-eacces | unreadable fallback dir during upload | n/a (never consulted) | fails with EACCES — refuses to risk shadowing (fail-closed) |
| remove-unlinkable-fallback | read-only fallback dir during remove | removes primary only | fails cleanly with EACCES, both copies intact |
19/19 scripted checks pass; every base cell behaved exactly as the pre-PR code dictates, and every head cell flipped as the PR claims.
Resolver probes (compiled production module dist/src/serve/session-attachments-root.js; witness 02-resolver-probes.png), 13/13 pass:
- env unset → root equals an independently computed
Storage.getProjectTempDir()/attachments, no fallback. - env set → root =
<configured>/<projectHash>/attachments, fallback = old default; two workspaces sharing one configured volume get disjoint sha256 hashes (05ac24…vsf8db21…) — collision isolation holds. ~and~/…expand against the home dir; relative paths resolve againstprocess.cwd(); empty string behaves as unset;~user/…is treated literally (see Findings).new Storage(ws)(oldserver.tsform) ≡new Storage(ws, Storage.getRuntimeBaseDir())(new form) — the substitution is byte-equivalent (constructor default verified inpackages/core/src/config/storage.ts).
Mutation matrix — no vacuous tests (scratch HEAD worktree, one suite per mutant; witness 03-mutation-matrix.png):
| Mutant | Guard removed | Result (sessionAttachments.test.ts / resolver suite) |
|---|---|---|
| control | none | 60/60 and 9/9 green |
| M0 (positive control) | read() payload emptied |
killed, 27 red — harness provably fails the suite, same file |
| M1 | read() fallback reverted |
killed, 2 red (reads from the fallback root…) |
| M2 | dedup occupancy probe removed | killed, 2 red (does not shadow a fallback name…, surfaces a fallback stat error…) |
| M3 | delete() skips fallback tombstone |
killed, 3 red (incl. delete tombstones both roots so a recreated session dir survives) |
| M4 | copyFrom() primary-only |
killed, 1 red (copyFrom merges fallback-held attachments…) |
| M5 | assertStored() ignores fallback |
killed, 1 red (same test pins read() and assertReference() together — verified in test body) |
| M6 | remove() primary-only |
killed, 2 red (removes a fallback attachment…, removes both copies…) |
| M7 | resolver drops <projectHash> |
killed, 5 red (cli suite) |
0 survivors. Layered-guard check: no two hunks defend one hazard indistinguishably — each guard has its own observable and its own killing test, so no combination row was needed.
Targeted gates (affected workspaces only):
| Suite | Result |
|---|---|
packages/acp-bridge sessionAttachments.test.ts |
60/60 (matches PR claim) |
packages/acp-bridge bridge.test.ts |
783/783 (matches PR claim; includes reads attachments from the fallback root when the primary misses at the bridge level) |
packages/cli session-attachments-root.test.ts |
9/9 |
packages/cli process-env-guard.test.ts |
3/3 (allowance entry matches the single process.env read in the new module) |
packages/cli server.test.ts -t "session attachments" |
12/12 (rest of the 1079-test file skipped by the name filter) — the actual POST /session/:id/attachments HTTP routes exercised against createServeApp via supertest |
Reviewer Test Plan: step 1 and step 2 executed with the exact claimed counts. Step 3 (live daemon + Web Shell upload) was verified at every layer below a booted daemon: store and resolver proven against compiled dist, and the HTTP routes themselves are green in server.test.ts's attachments block. The only unexecuted remainder is booting an authenticated daemon with a real ACP child — see Not covered.
Wiring checks (grep-level, not counted as assertions): run-qwen-serve.ts has no static import of the resolver (only the dynamic await import('./session-attachments-root.js') at line 5189 — the state the second commit describes), all three bridge construction sites pass sessionAttachmentsFallbackRoot; server.ts wires both options. Docs (docs/users/qwen-serve.md §Session attachment storage, docs/developers/daemon/17-configuration.md) match measured behavior point-for-point, including the one-way migration warning and the read-once-at-startup note.
Corrections
None needed.
Findings (non-blocking)
~user/…is treated literally —QWEN_SERVE_SESSION_ATTACHMENTS_ROOT=~otheruser/volresolves to<cwd>/~otheruser/vol/…(probe R7). This mirrorsStorage.resolvePathsemantics and violates no documented claim (docs promise only~/~/…expansion), so it is an observation, not a defect.- Fail-closed on an unreadable/unlinkable legacy root (intentional, measured). With the legacy dir
chmod 000, uploads fail with EACCES rather than risking a name shadow (cell 8); with it read-only,remove()throws with both copies intact (cell 9). Both are deliberate per code comments and pinned by tests — an operator whose old temp dir is unreadable gets a loud error instead of silent data loss. Recorded so reviewers know the behavior is by design. assertCanCommitis invoked once per root indelete()(twice when a fallback is configured). Harmless for the current idempotent gate checks; noted for completeness.
Not covered
- Live daemon + Web Shell HTTP upload (the boot-and-click half of Test Plan step 3): starting an authenticated
qwen servewith a real ACP child and uploading through the Web Shell was out of scope. Everything below that layer is measured: store and resolver against compiled dist, the bridge-level API inside the 783-test gate, and the HTTP route handlers inserver.test.ts's attachments block (12/12). This reproduces the handling, not the end-to-end trigger. - Windows path shapes (
~\…branch exists in code; untestable on this Linux runner; PR itself marks Windows/Linux CI as not tested). - Per-commit attribution: metadata lists 2 commits (
e1644bafeat +da506f7fast-path fix) but the depth-2 checkout yieldsgit rev-list --count HEAD^1..HEAD^2 = 1on a shallow repo; verified the aggregateHEAD^1..HEADdiff and the fast-path end-state (no static import). - Repo-wide test/lint gates — only the affected workspaces were run, per scope rules.
- The lenient-
statdegradation path ofassertStored(statSize, which swallows every stat failure): not independently probed — at the harness level a lenient-stat miss is observationally identical to a missing file, so the suite's mocked-statSynctests are the instrument that separates them; they ran green in the 60/60 gate. - No concurrency probing beyond the suite's own tombstone-race test.
Methodology
Environment: the CI verify container (node:22-bookworm, uid 1000), merge-ref checkout at db8fa96b3c, npm ci + npm run build pre-run. A/B: base sources checked out via git worktree add tmp/base-tree HEAD^1; packages/acp-bridge rebuilt there with the repo's own build_package.js (tsc --build, exit 0). Two worktree-depth artifacts were root-caused and fixed, neither attributable to the PR: nested per-package node_modules (e.g. ignore@7.0.5, own ajv) are git-ignored and had to be symlinked in, and core's paths mapping (../../node_modules/@lydell/node-pty/node-pty.d.ts) needed a node_modules symlink at the worktree root. Realpath assertion for control purity: the base dist's @qwen-code/qwen-code-core resolves to the head tree's packages/core, and git diff HEAD^1..HEAD -- packages/core package.json package-lock.json is empty, so no confound. Harnesses (ab-store-harness.mjs, resolver-probe.mjs, mutate.mjs, run-mutations.sh) drove the compiled dist/source directly, mock-free, with fresh sandboxes per cell; raw per-cell logs live in logs/ (ab-store-direct.log, resolver-probe-direct.log, mut-*.log, gate-*.log, base-build*.log). Terminal witnesses rendered by scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/acp-bridge/src/bridge.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/bridge.test.ts
file packages/acp-bridge/src/sessionAttachments.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/sessionAttachments.test.ts
file packages/cli/src/serve/process-env-guard.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/process-env-guard.test.ts
file packages/cli/src/serve/session-attachments-root.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/session-attachments-root.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/acp-bridge/src/bridge.test.ts: PPPPP
packages/acp-bridge/src/sessionAttachments.test.ts: PPPPP
packages/cli/src/serve/process-env-guard.test.ts: PPPPP
packages/cli/src/serve/session-attachments-root.test.ts: PPPPP
verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 1 · packages/acp-bridge/src/sessionAttachments.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/session-attachments-root.test.ts: P (exit 0)
round 2 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 2 · packages/acp-bridge/src/sessionAttachments.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/session-attachments-root.test.ts: P (exit 0)
round 3 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 3 · packages/acp-bridge/src/sessionAttachments.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/session-attachments-root.test.ts: P (exit 0)
round 4 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 4 · packages/acp-bridge/src/sessionAttachments.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/session-attachments-root.test.ts: P (exit 0)
round 5 · packages/acp-bridge/src/bridge.test.ts: P (exit 0)
round 5 · packages/acp-bridge/src/sessionAttachments.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/process-env-guard.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/session-attachments-root.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
| const primary = await this.tryRead(await this.directory(), name); | ||
| if (primary) return primary; | ||
| return await this.tryRead(this.persistentFallbackDirectory, name); |
There was a problem hiding this comment.
[Critical] read() forces await this.directory() — a memoized fs.mkdir of the configured root — before ever consulting the fallback, and tryRead only falls through on ENOENT. When QWEN_SERVE_SESSION_ATTACHMENTS_ROOT points at a volume that is read-only, has bad permissions, is unmounted, or degrades after boot, this lookup rejects before the fallback is consulted and the request 500s even though healthy pre-switch attachments sit in the default runtime temp dir — falsifying the documented promise that "attachments uploaded before the switch remain readable and removable" (those pre-switch bytes live in the default temp dir, which is healthy).
Verified by execution in an isolated scratch tree: with the configured root made unreadable (real EACCES) and the fallback holding 'legacy bytes', the PR code rejects without consulting the fallback (read rejected: EACCES; the post-mkdir degradation arm gives read rejected: EIO, readFile call count: 1). With the forced mkdir removed from the read path the same setup resolves (FIXED: read resolved: "legacy bytes", readFile call count: 2).
Don't force directory creation on the read path: use the memoized persistentDirectory when established and let a primary lookup failure degrade to the fallback read; decide explicitly whether non-ENOENT primary read errors should also fall back (they currently rethrow).
中文说明
read() 在查询 fallback 之前强制执行 await this.directory()（对配置目录做记忆化的 fs.mkdir），且 tryRead 仅在 ENOENT 时才回退。当 QWEN_SERVE_SESSION_ATTACHMENTS_ROOT 指向只读、权限错误、已卸载或启动后损坏的卷时，请求会在查询 fallback 之前直接失败并返回 500——即使切换前上传的健康附件仍在默认运行时临时目录中。这与文档承诺的"切换前上传的附件仍可读取和删除"相矛盾（这些旧附件存放在默认临时目录，它是健康的）。
已在隔离工作树中实测：将配置目录设为不可读（真实 EACCES）、fallback 中写入 'legacy bytes' 后，当前代码不查询 fallback 直接拒绝（read rejected: EACCES；mkdir 成功后卷损坏的场景为 read rejected: EIO, readFile call count: 1）。移除读取路径上的强制 mkdir 后同样场景可正常解析（FIXED: read resolved: "legacy bytes"、readFile call count: 2）。
建议：读取路径不要强制建目录——已建立时使用记忆化的 persistentDirectory，主目录查询失败时降级到 fallback 读取；并明确决定非 ENOENT 的主目录读取错误是否也应回退（目前会直接抛出）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const fallbackHit = | ||
| (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; | ||
| const primaryHit = | ||
| (await this.tryUnlink(await this.directory(), name)) === true; |
There was a problem hiding this comment.
[Critical] remove() unlinks the fallback copy first (the actual mutation), then forces primary directory creation via await this.directory(). If that mkdir throws (EROFS/EACCES on the configured volume), the request fails after the requested deletion was already applied — the client sees failure for a deletion that happened, and every retry keeps 500ing until the volume is repaired.
Verified by execution: PR: remove rejected: EACCES | legacy fallback file still present: false (deletion applied, request failed); with the forced mkdir removed from this path: FIXED: remove resolved: removed=true | legacy fallback file still present: false.
Don't force directory creation on the remove path — e.g. this.persistentDirectory ?? (await this.directory()); tryUnlink already treats ENOENT as absence.
中文说明
remove() 先删除 fallback 副本（真正的变更操作），随后通过 await this.directory() 强制创建主目录。若该 mkdir 抛错（配置卷上的 EROFS/EACCES），请求会在删除已经执行之后才失败——客户端收到的是"删除失败"，但文件其实已删，且在卷修复之前每次重试都会继续 500。
实测：PR: remove rejected: EACCES | legacy fallback file still present: false（删除已生效但请求失败）；移除该路径上的强制 mkdir 后：FIXED: remove resolved: removed=true | legacy fallback file still present: false。
建议：删除路径不要强制建目录——例如 this.persistentDirectory ?? (await this.directory())；tryUnlink 本身已把 ENOENT 当作"不存在"处理。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. | ||
| - **Per-session layout.** Files live under `<root>/<projectHash>/attachments/session-<sessionId>/` in both locations, where `<projectHash>` is the same workspace hash used by the default runtime temp dir; the fallback lookup uses the same session layout in the default dir. Two workspaces pointing at the same configured root stay isolated from each other. | ||
| - **Archive cleanup.** When a session is archived, its attachment directory is removed from both the configured root and the default fallback dir. |
There was a problem hiding this comment.
[Critical] This bullet is false: no archive code path removes attachments. archiveDaemonSessions only receives closeSession (it structurally cannot call deleteSessionAttachments), core archiveSessions only moves transcript/sidecar files, and attachments.close() on a persistent store early-returns without deleting anything. The two-root cleanup exists only on session delete (deleteDaemonSessions → deleteSessionAttachments → store.delete()) — the PR's own bridge test asserting ENOENT in both roots is on that path.
Verified by execution with both roots seeded: after close(): primary present=true, fallback present=true (what the archive path reaches) vs after delete(): primary present=false, fallback present=false. An operator who adopted the env var for a dedicated volume and relies on this bullet gets unbounded growth: every archived session keeps its session-<id>/ directory in both roots until the session is explicitly deleted.
| - **Archive cleanup.** When a session is archived, its attachment directory is removed from both the configured root and the default fallback dir. | |
| - **Delete cleanup.** When a session is deleted, its attachment directory is removed from both the configured root and the default fallback dir. Archiving a session keeps its attachments so they survive unarchive. |
中文说明
该条目与代码不符：归档路径不会删除任何附件。archiveDaemonSessions 只接收 closeSession（结构上无法调用 deleteSessionAttachments），核心 archiveSessions 只移动会话记录/附属文件，持久化存储上的 attachments.close() 会直接提前返回。双目录清理只存在于删除会话路径（deleteDaemonSessions → deleteSessionAttachments → store.delete()）——本 PR 自己的 bridge 测试断言两个目录均 ENOENT 的正是删除路径。
实测（两个目录均有数据）：after close(): primary present=true, fallback present=true（归档路径的实际效果）对比 after delete(): primary present=false, fallback present=false。依赖该条目回收磁盘空间的用户会遇到无限增长：每个归档会话的 session-<id>/ 目录在两个目录中都会保留到会话被显式删除为止。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('still degrades reference validation when the fallback stat fails', async () => { | ||
| const { main, fallback } = await createRoots(); | ||
| const store = new SessionAttachmentStore(main, sessionId, fallback); |
There was a problem hiding this comment.
[Suggestion] This test never fails the fallback stat: mockImplementationOnce intercepts by call order, and assertStored stats the primary root first — so the one-shot EACCES is consumed by the primary stat and swallowed by statSize's catch-all. The fallback stat (the one the test's name claims to fail) runs the real implementation and succeeds; the test passes solely via the size mismatch (reference size: 13 vs a 14-byte file).
Verified by execution: setting the reference to the file's real size: 14 → AssertionError: expected function to throw an error, but it didn't; swapping the fallback branch to statSizeStrict (the exact divergence the two new helpers codify) leaves all 13 fallback-root tests green; a path-conditional mock that really fails the fallback stat flips the outcome (original THROW code=session_attachment_gone, mutant THROW code=EACCES). The implementation is currently correct — this is purely a vacuous test masking that regression.
Arm the fault per call order (ENOENT for the primary stat, EACCES for the fallback) or use a path-conditional mock, and make the reference size match the file so the throw can only come from the degradation path.
中文说明
该测试从未真正让 fallback 的 stat 失败：mockImplementationOnce 按调用顺序拦截，而 assertStored 先对主目录做 stat——因此一次性的 EACCES 被主目录的 stat 消耗，并被 statSize 的兜底 catch 吞掉。测试名称声称要失败的 fallback stat 实际执行的是真实实现且成功；测试通过仅仅因为尺寸不匹配（引用声明 size: 13，文件实际 14 字节）。
实测：把引用改成文件真实尺寸 size: 14 → AssertionError: expected function to throw an error, but it didn't；把 fallback 分支换成 statSizeStrict（正是两个新 helper 所区分的分歧）后全部 13 个 fallback 测试仍然为绿；用按路径条件的 mock 真正让 fallback stat 失败则结果翻转（原始 THROW code=session_attachment_gone，变异体 THROW code=EACCES）。实现目前是正确的——问题纯粹是空测试，无法拦截该回归。
建议：按调用顺序分别注入故障（主目录 stat 抛 ENOENT、fallback 抛 EACCES）或使用按路径条件的 mock，并让引用尺寸与文件一致，使抛错只能来自降级路径。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('keeps the primary readable when the fallback unlink fails', async () => { | ||
| const { main, fallback } = await createRoots(); | ||
| const store = new SessionAttachmentStore(main, sessionId, fallback); |
There was a problem hiding this comment.
[Suggestion] mockRejectedValueOnce rejects whichever fs.unlink arrives first, so swapping the two tryUnlink calls in remove() (primary-first) still satisfies both assertions — the fallback-first ordering the production comment makes load-bearing ("leave the authoritative primary copy intact") is unpinned.
Verified by execution: the swapped-order mutant leaves this test green (Tests 1 passed); a path-conditional probe rejecting only fallback-targeted unlinks distinguishes the arms — original read-after=from primary, mutant read-after=stale fallback copy (the "deleted" attachment resurrects from the fallback).
Reject only unlinks targeting the fallback root, so under a swapped order the primary is really deleted and the read-after returns the stale fallback content.
中文说明
mockRejectedValueOnce 会拒绝最先到达的 fs.unlink 调用，因此把 remove() 中两个 tryUnlink 的顺序对调（主目录优先）后两个断言依然全部通过——生产代码注释中至关重要的"先删 fallback"顺序（"保留权威的主副本"）没有被测试锁定。
实测：对调顺序的变异体下该测试仍为绿（Tests 1 passed）；用只拒绝 fallback 路径 unlink 的按路径条件 probe 可以区分两种顺序——原始顺序 read-after=from primary，变异体 read-after=stale fallback copy（"已删除"的附件从 fallback 复活）。
建议：只拒绝指向 fallback 目录的 unlink，使顺序对调时主副本被真实删除、删除后的读取返回陈旧的 fallback 内容，从而让测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (this.persistentFallbackDirectory) { | ||
| await this.removeDirectoryWithTombstone( | ||
| this.persistentFallbackDirectory, | ||
| options.assertCanCommit, | ||
| ); | ||
| try { | ||
| await fs.rename(directory, tombstone); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') return; | ||
| throw error; | ||
| } | ||
| await fs.rm(tombstone, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
[Suggestion] delete() tombstones the primary directory before the fallback — the inverse of the fallback-first ordering remove() in this same file deliberately documents ("the authoritative primary copy is still intact and remove() can fail cleanly without leaving a deleted attachment readable through the fallback").
Verified by execution (rename mocked to throw EROFS only for the fallback session dir): PR code → delete=REJECTED read-only volume; primaryDir=false; fallbackDir=true; read legacy.txt=legacy data — the primary copy is already destroyed when delete() rejects, and since clients can request session ids, a reused session id serves the archived session's stale fallback bytes. With the fallback block moved first → primaryDir=true; read current.txt=primary data, and all 13 shipped fallback-root tests still pass.
Swap the two blocks so the fallback root is removed first:
if (this.persistentFallbackDirectory) {
await this.removeDirectoryWithTombstone(
this.persistentFallbackDirectory,
options.assertCanCommit,
);
}
const directory =
this.persistentDirectory ??
(await this.directoryPromise?.catch(() => undefined));
if (directory) {
await this.removeDirectoryWithTombstone(
directory,
options.assertCanCommit,
);
}中文说明
delete() 先对主目录做 tombstone 删除、后删 fallback——与同文件中 remove() 刻意文档化的"先删 fallback"顺序相反（注释："权威的主副本保持完好，remove() 可以干净地失败，而不会让已删除的附件仍可经 fallback 读取"）。
实测（mock rename 仅对 fallback 会话目录抛 EROFS）：当前代码 → delete=REJECTED read-only volume; primaryDir=false; fallbackDir=true; read legacy.txt=legacy data——delete() 拒绝时主副本已被销毁；由于客户端可以请求指定会话 id，复用同一会话 id 会读到归档会话遗留的陈旧 fallback 数据。将 fallback 块移到前面后 → primaryDir=true; read current.txt=primary data，且全部 13 个已发布的 fallback 测试仍通过。
建议：交换两个块，先删 fallback 目录（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const configured = process.env[SESSION_ATTACHMENTS_ROOT_ENV]; | ||
| if (!configured) return { root: defaultRoot }; |
There was a problem hiding this comment.
[Suggestion] Only ''/unset is treated as unset here; a whitespace-only or trailing-whitespace value flows into path.resolve and creates a root with a literal space segment. When the typo is fixed and the daemon restarted, everything written to the whitespace root is permanently unreachable — the fallback only covers the default runtime temp dir. The sibling daemon env QWEN_CODE_MEMORY_PROJECT_SCOPE is trimmed before use in this same daemon (launchMemoryProjectScopeValue?.trim() in run-qwen-serve.ts), and the test suite pins the empty-string case but not the whitespace one.
| const configured = process.env[SESSION_ATTACHMENTS_ROOT_ENV]; | |
| if (!configured) return { root: defaultRoot }; | |
| const configured = process.env[SESSION_ATTACHMENTS_ROOT_ENV]?.trim(); | |
| if (!configured) return { root: defaultRoot }; |
and add a test pinning whitespace-as-unset alongside the empty-string case.
中文说明
此处只有 ''/未设置被视为未设置；纯空白或带尾随空格的值会进入 path.resolve，创建出带字面空格的路径段。当拼写修正、守护进程重启后，写入空白路径的所有内容将永久不可达——fallback 只覆盖默认运行时临时目录。同一守护进程中的同类环境变量 QWEN_CODE_MEMORY_PROJECT_SCOPE 在使用前会先 trim（run-qwen-serve.ts 中的 launchMemoryProjectScopeValue?.trim()），且测试只锁定了空字符串场景、未覆盖空白场景。
建议：按上方 suggestion 先 trim 再判空，并补充"空白视为未设置"的测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('prefers the primary root over the fallback', async () => { | ||
| const { main, fallback } = await createRoots(); | ||
| const store = new SessionAttachmentStore(main, sessionId, fallback); |
There was a problem hiding this comment.
[Suggestion] The new fallback block pins primary-over-fallback precedence for read() and remove(), but nothing exercises assertReference in the both-roots-hold-the-same-name state — exactly the post-migration condition this PR manages. assertStored's primary-first stat ordering is what keeps an authoritative primary reference valid while a stale fallback copy exists. A refactor that reorders the two-root lookup (a shared helper listing [fallback, primary], or accepting a size match in either root) ships with every test green, yet prompts holding the authoritative primary reference degrade to session_attachment_gone although read() serves the primary bytes.
After both roots hold the file, also assert store.assertReference(...) passes with the primary reference's size and throws session_attachment_gone when given the stale fallback size.
中文说明
新的 fallback 测试块锁定了 read() 与 remove() 的"主目录优先"，但没有任何测试在"两个目录同时存在同名文件"的状态下执行 assertReference——而这正是本 PR 所管理的迁移后状态。assertStored 先 stat 主目录的顺序，是保证权威主引用在存在陈旧 fallback 副本时仍然有效的关键。若未来重构改变双目录查询顺序（例如合并为按 [fallback, primary] 列举的公共 helper，或任一目录尺寸匹配即通过），所有测试仍会为绿，但持有权威主引用的 prompt 会降级为 session_attachment_gone，尽管 read() 实际提供的是主目录字节。
建议：在两个目录都持有该文件后，补充断言 store.assertReference(...) 以主引用尺寸通过、以陈旧 fallback 尺寸抛出 session_attachment_gone。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const defaultRoot = defaultSessionAttachmentsRoot(workspace, runtimeBaseDir); | ||
| const projectHash = path.basename(path.dirname(defaultRoot)); |
There was a problem hiding this comment.
[Suggestion] Every default/fallback-root assertion in this file is derived by calling defaultSessionAttachmentsRoot itself (even projectHash is extracted from its return value), so the suite cannot detect drift of the one property the fallback exists for — that it equals the legacy layout <runtimeBaseDir>/tmp/<workspace-hash>/attachments. A future change to Storage.getProjectTempDir() (renaming tmp, re-nesting the hash, changing the base) silently moves the fallback while every test stays green: pre-switch attachment reads then fail and delete() leaves the real legacy bytes orphaned.
Add one layout-pinning assertion that does not route through the resolver, e.g. expect(defaultRoot).toBe(path.join(runtimeBaseDir, 'tmp', projectHash, 'attachments')).
中文说明
该文件中所有关于默认目录/fallback 目录的断言都通过调用 defaultSessionAttachmentsRoot 本身推导（连 projectHash 也是从它的返回值中提取），因此测试套件无法检测 fallback 存在意义的唯一属性发生漂移——即 fallback 必须等于旧布局 <runtimeBaseDir>/tmp/<workspace-hash>/attachments。未来对 Storage.getProjectTempDir() 的修改（重命名 tmp、调整 hash 嵌套层级、更换基础目录）会悄悄移动 fallback 而所有测试保持绿色：切换前的附件读取将失败，delete() 会遗留真正的旧数据无人清理。
建议：补充一条不经过解析器的布局锁定断言，例如 expect(defaultRoot).toBe(path.join(runtimeBaseDir, 'tmp', projectHash, 'attachments'))。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
read() and remove() no longer force-create the configured root before consulting the fallback, so a degraded configured volume serves and removes pre-switch attachments from the healthy default dir instead of failing; delete() removes the fallback root first, mirroring remove(), so a failure on the legacy root keeps the primary copy intact; QWEN_SERVE_SESSION_ATTACHMENTS_ROOT is trimmed before use. Corrects the docs to say attachment cleanup happens on session delete, not archive.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #10066All 9 findings from the automated review on Findings and decisionsrv:5026446775 — CI red on rc:3860320277 (Critical) — rc:3860320286 (Critical) — rc:3860320291 (Critical) — docs bullet falsely claims archive removes attachments. Fixed. Verified against the code first: rc:3860320304 (Suggestion) — vacuous fallback-stat test. Fixed. rc:3860320306 (Suggestion) — rc:3860320307 (Suggestion) — rc:3860320316 (Suggestion) — whitespace env values create literal-space roots. Fixed. rc:3860320324 (Suggestion) — rc:3860320336 (Suggestion) — default-root assertions all derived via the resolver. Fixed. Added VerificationRed phase (reproduction): the 6 new behavior witnesses run against the pre-round code first —
中文说明Autofix 评审轮次 — PR #10066
Findings 与处理决定rv:5026446775 — rc:3860320277（Critical）— rc:3860320286（Critical）— rc:3860320291（Critical）— 文档条目错误声称归档会删除附件。 已修复。先对代码核实： rc:3860320304（Suggestion）— fallback stat 测试是空测试。 已修复。 rc:3860320306（Suggestion）— rc:3860320307（Suggestion）— rc:3860320316（Suggestion）— 空白环境变量值会创建带字面空格的目录。 已修复。 rc:3860320324（Suggestion）— rc:3860320336（Suggestion）— 默认目录的所有断言都经由 resolver 本身推导。 已修复。新增 验证红色阶段（复现）：6 个新的行为见证测试先在修改前代码上运行——
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
…nts-root-env # Conflicts: # packages/cli/src/serve/run-qwen-serve.ts
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #10066 (base-conflict resolution round)Feedback triageThe round's Work performed: merged
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/sessionAttachments.ts:488 — [review] D2-3 fallback-read failure mode unpinned and asymmetric to sibling pathspackages/acp-bridge/src/sessionAttachments.test.ts:1588 — [review] D2-4 copyFrom's primary-absent fallback-merge branch (the migration case) is unpinned by any testpackages/acp-bridge/src/sessionAttachments.test.ts:1316 — [review] D2-5 fallback-stat fault test never asserts which path received the faultpackages/acp-bridge/src/sessionAttachments.test.ts:1483 — [review] D2-1 the two 'delete clears …' tests leak their mkdtemp roots into os.tmpdir()packages/cli/src/serve/run-qwen-serve.ts:5264 — [review] D2-2 no test pins the resolver→bridge wiring glue at the four createAcpSessionBridge call sites
中文说明
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| export const SESSION_ATTACHMENTS_ROOT_ENV = | ||
| 'QWEN_SERVE_SESSION_ATTACHMENTS_ROOT'; |
There was a problem hiding this comment.
[Critical] The new daemon-wide env var is not added to PROJECT_ENV_HARDCODED_EXCLUSIONS (packages/cli/src/config/shared-env-keys.ts), so a project .env / .qwen/.env / settings.env file can set it and redirect attachment storage for every workspace the daemon serves — the exact class that denylist exists to block (see the rationale on its existing QWEN_SERVE_NEW_FILE_MODE entry: daemon-wide keys may only come from the daemon's launch environment or a home .env). This PR touches neither shared-env-keys.ts nor fast-path-settings.ts, and the full env loader gates on the same denylist, so no other gate intercepts the key.
Concrete trigger: an attacker repo ships .qwen/.env containing QWEN_SERVE_SESSION_ATTACHMENTS_ROOT=./exfil (relative values are explicitly supported). An operator runs qwen serve inside it — the trust gate passes whenever folder trust is disabled or the folder is unknown (the loader's !== false check), loadServeFastPathEnvironment writes the key into process.env before runQwenServeImpl reads it, and uploads for ALL workspaces land under <repo>/exfil/<hash>/attachments/ — disclosure (content sits in an attacker-controlled, committable tree) and tampering (reads consult the configured root first, so attacker-modified bytes are served back to downloads and model-context dispatch). Unsetting the variable later does not recover the attachments: the fallback only covers the default runtime temp dir.
Verified by execution, driving the real loadServeFastPathEnvironment with an attacker-repo .env:
{"envValueAppliedFromProjectEnvFile":"./exfil","resolvedRoot":"/tmp/probe-r2-7-repo-…/exfil/4b9513…/attachments","rootInsideAttackerRepo":true}
with the key added to PROJECT_ENV_HARDCODED_EXCLUSIONS: {"resolvedRoot":"/tmp/probe-r2-7-runtime/tmp/e759a6…/attachments","rootInsideAttackerRepo":false}.
Fix: add the key to the denylist next to QWEN_SERVE_NEW_FILE_MODE:
// packages/cli/src/config/shared-env-keys.ts — PROJECT_ENV_HARDCODED_EXCLUSIONS
'QWEN_SERVE_NEW_FILE_MODE': …,
// Attachment storage location is a daemon-wide operator decision; a project
// .env must not redirect it across all workspaces.
'QWEN_SERVE_SESSION_ATTACHMENTS_ROOT': 1,中文说明
新的 daemon 级环境变量未加入 PROJECT_ENV_HARDCODED_EXCLUSIONS（packages/cli/src/config/shared-env-keys.ts），因此项目内的 .env / .qwen/.env / settings.env 可以设置它，把该 daemon 服务的所有工作区的附件存储重定向——这正是该拒绝列表要拦截的一类（参见现有 QWEN_SERVE_NEW_FILE_MODE 条目的理由：daemon 级配置只能来自 daemon 启动环境或用户主目录 .env）。本 PR 未改动 shared-env-keys.ts 和 fast-path-settings.ts，完整 env 加载器也以同一拒绝列表为准，因此没有其他关卡会拦截该键。
具体触发：恶意仓库携带 .qwen/.env，内容为 QWEN_SERVE_SESSION_ATTACHMENTS_ROOT=./exfil（明确支持相对值）。运维在该目录运行 qwen serve——当文件夹信任被禁用或文件夹未知时信任检查放行（加载器的 !== false 判断），loadServeFastPathEnvironment 在 runQwenServeImpl 读取之前把该键写入 process.env，所有工作区的上传都会落入 <repo>/exfil/<hash>/attachments/——泄露（内容位于攻击者可控、可提交的目录树中）与篡改（读取优先查配置的根目录，攻击者修改过的字节会被回传给下载和模型上下文）。事后取消该变量也无法找回附件：fallback 只覆盖默认运行时临时目录。
实测（驱动真实的 loadServeFastPathEnvironment）：rootInsideAttackerRepo: true；把该键加入 PROJECT_ENV_HARDCODED_EXCLUSIONS 后恢复默认根目录（rootInsideAttackerRepo: false）。
修复：在 shared-env-keys.ts 的拒绝列表中将该键加在 QWEN_SERVE_NEW_FILE_MODE 旁边（同样理由注释）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Fallback first, mirroring remove(): if the legacy root cannot be | ||
| // removed, the authoritative primary copy must stay intact. | ||
| if (this.persistentFallbackDirectory) { |
There was a problem hiding this comment.
[Critical] delete() removes the fallback root first and rethrows any non-ENOENT fallback failure before touching the primary — but its only caller, deleteDaemonSessions (session-archive.ts:519-549), removes the persisted session row FIRST. With the legacy default dir unwritable (the read-only-volume scenario the comment above this block itself invokes), the whole delete rejects with the session already gone from the catalog: a retry throws SessionNotFoundError, and the session-<id>/ directories remain in BOTH roots permanently — on the dedicated volume this feature tells operators to mount, such orphaned bytes accumulate with no cleanup path. This falsifies the new "Delete cleanup" bullet on the explicit-delete path (the orphan-reap path is a second, distinct mechanism reported separately).
Verified by execution with a real chmod-0555 fallback root on the unmodified PR code:
{"deleteRejected":true,"deleteErrorCode":"EACCES","primaryDirSurvives":true,"primaryBytes":"primary data","fallbackDirSurvives":true}
with a best-effort fallback-removal candidate fix: {"deleteRejected":true,"deleteErrorCode":"EACCES","primaryDirSurvives":false,"primaryBytes":"<gone>"} — the configured root is cleaned, and the request still fails loudly.
Fix: make the fallback removal in delete() best-effort — catch a fallback-side failure, still remove the primary directory, then rethrow/aggregate — so configured-root cleanup is never skipped by a legacy-volume fault. If the strict ordering must stay, narrow the doc bullet instead (state that delete fails and directories are retained when the fallback dir cannot be removed).
中文说明
delete() 先删回退目录，且回退侧任何非 ENOENT 失败都会在触碰主目录之前直接抛出——但它唯一的调用方 deleteDaemonSessions（session-archive.ts:519-549）先删除持久化会话行。当旧的默认目录不可写（正是上方注释自己提到的只读卷场景）时，整个删除在会话已从目录中移除的情况下被拒绝：重试抛 SessionNotFoundError，session-<id>/ 目录在两个根目录中永久残留——在本功能引导运维挂载的专用卷上，这类孤儿字节会无限累积且无任何清理路径。这使得新增的 "Delete cleanup" 条目在显式删除路径上失实（孤儿回收路径是另一个独立机制，已单独报告）。
实测（真实 chmod-0555 回退目录，未改动的 PR 代码）：deleteRejected: true, deleteErrorCode: EACCES, primaryDirSurvives: true；采用"回退删除尽力而为"的候选修复后：主目录被清理（primaryDirSurvives: false），请求仍如实报错。
修复：把 delete() 中的回退目录删除改为尽力而为——捕获回退侧失败、仍然删除主目录、然后再抛出/聚合——使配置根目录的清理永远不会被旧卷故障跳过。若必须保留严格顺序，则收窄文档条目（说明回退目录无法删除时会话删除失败且目录保留）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. | ||
| - **Per-session layout.** Files live under `<root>/<projectHash>/attachments/session-<sessionId>/` in both locations, where `<projectHash>` is the same workspace hash used by the default runtime temp dir; the fallback lookup uses the same session layout in the default dir. Two workspaces pointing at the same configured root stay isolated from each other. | ||
| - **Delete cleanup.** When a session is deleted, its attachment directory is removed from both the configured root and the default fallback dir. Archiving a session keeps its attachments so they survive unarchive. |
There was a problem hiding this comment.
[Critical] This bullet promises more than the code delivers: the orphan-reap deletion path deletes sessions without ever deleting their attachments. deleteDaemonSessionIfOrphan (session-archive.ts:622-656) is typed Pick<AcpSessionBridge, 'killSession' | 'markSessionCatalogChanged'> — it structurally cannot call deleteSessionAttachments — and SessionAttachmentStore.close() on a persistent store early-returns without deleting anything. Sessions reaped as orphans therefore leave their session-<id>/ directories in BOTH roots forever, unreferenced by anything that will ever look them up again. The leak predates this PR for the default tmpdir, but this diff newly asserts the guarantee and extends the leak onto the operator-configured persistent volume, where no temp cleanup will ever sweep it.
Verified by execution with a real bridge: upload → orphan reap completes (sessionCount=0) → bytes still present in the configured root; only the explicit deleteSessionAttachments call — the one the reap path never makes — removes them:
after reap: sessionCount=0; main=["leak.txt"], fallback=ERR:ENOENT — session gone, bytes remain
after deleteSessionAttachments: main=ERR:ENOENT — removed
Concrete trigger: an ACP-HTTP session/new with a {removePersisted: true} ownership receipt uploads attachments while the env var is set, then disconnects orphan-eligible — removeOrphanSession(sessionId, true) reaps it, and the bytes stay.
Fix: in deleteDaemonSessionIfOrphan, after the persisted removal succeeds, call bridge.deleteSessionAttachments(sessionId) (widening its Pick accordingly), mirroring deleteDaemonSessions (session-archive.ts:536-540); or narrow this sentence to explicit delete-request cleanup. The code fix is the better side — the bytes are orphaned either way.
中文说明
该条目的承诺超出了代码实际行为：孤儿回收删除路径删除会话时从不删除附件。deleteDaemonSessionIfOrphan（session-archive.ts:622-656）的类型是 Pick<AcpSessionBridge, 'killSession' | 'markSessionCatalogChanged'>——结构上无法调用 deleteSessionAttachments——而持久化存储上的 SessionAttachmentStore.close() 会直接提前返回、不删除任何内容。因此被当作孤儿回收的会话会在两个根目录中永久留下 session-<id>/ 目录，再无任何东西会引用它们。该泄漏在默认临时目录中在本 PR 之前就存在，但本 diff 新作出了该保证，并把泄漏扩展到运维配置的持久卷——那里没有任何临时清理会回收它。
实测（真实 bridge）：上传 → 孤儿回收完成（sessionCount=0）→ 字节仍在配置根目录中；只有显式的 deleteSessionAttachments 调用（回收路径从不调用）才能删除它们。具体触发：携带 {removePersisted: true} 所有权回执的 ACP-HTTP session/new 在设置环境变量后上传附件、随后以孤儿状态断开——removeOrphanSession(sessionId, true) 回收会话，字节残留。
修复：在 deleteDaemonSessionIfOrphan 的持久化删除成功后调用 bridge.deleteSessionAttachments(sessionId)（相应放宽其 Pick），参照 deleteDaemonSessions（session-archive.ts:536-540）；或把该句收窄为"显式删除请求会清理"。修代码是更好的一侧——否则字节两边都会成为孤儿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
|
||
| Scope and limits: | ||
|
|
||
| - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. |
There was a problem hiding this comment.
[Critical] This bullet overpromises: remove() unlinks the fallback copy first, and tryUnlink rethrows any non-ENOENT error — so when the legacy default dir sits on a read-only or degraded volume, deleting a fallback-held attachment rejects (the HTTP layer maps the unmapped errno to a 500) with the bytes still on disk and still served by the fallback read path, even though this sentence promises they "remain readable and removable". The code behavior is intentional and test-pinned — reporting success would leave "deleted" bytes readable through the fallback — so the sentence, not the code, must change. The sibling sentence in docs/developers/daemon/17-configuration.md promises only "stay readable".
Verified by execution on the unmodified PR code:
CONTROL REMOVE (writable legacy dir): resolved true
READONLY REMOVE: rejected EACCES: permission denied, unlink '/tmp/r2-probe-fallback-…/session-s-…/notes.txt'
READONLY AFTER REMOVE: still on disk, still readable via the fallback
(primary-only removes still succeed — the blast radius is exactly fallback-held attachments.)
| - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. | |
| - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable, and removable while the default fallback dir stays writable — a remove whose legacy copy cannot be unlinked (e.g. a read-only fallback volume) surfaces the error rather than reporting success. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. |
中文说明
该条目承诺过度：remove() 先删除回退副本，且 tryUnlink 对任何非 ENOENT 错误都会重新抛出——因此当旧的默认目录位于只读或损坏的卷上时，删除仅存于回退目录的附件会被拒绝（HTTP 层把未映射的 errno 转为 500），字节仍在磁盘上且仍可通过回退读取路径访问，而本句却承诺"仍可读取和删除"。代码行为是有意为之且有测试钉住——若返回成功，"已删除"的字节仍可通过回退目录读到——所以要改的是这句话而不是代码。docs/developers/daemon/17-configuration.md 中的同义句只承诺 "stay readable"。
实测（未改动的 PR 代码）：可写旧目录 → 删除成功；只读旧目录 → rejected EACCES，字节仍在且仍可读取（仅主目录存在的附件删除仍成功——影响范围恰为回退目录中的附件）。
上方 suggestion 将条目改为：切换前上传的附件仍可读取，且在默认回退目录保持可写时可删除——若旧副本无法删除（如只读回退卷），删除会如实报错而非假装成功。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…rage (#10066) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #10066 (round 3)Addressed all four actionable Critical inline findings from the automated Findings and dispositions[rc:3863704616]
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round report for PR #10066 — no action takenThis round's feedback contains nothing actionable, so no code changes were made and no commit was created. What the round received
About the two listed checksThe only entries under "Failed checks" are Growth statusThe diff-growth window is within budget (source 46 / test 257 net lines vs 400/400 budgets), and no growth audit was required this round. The PR head remains at the last verified commit; the branch is unchanged. 中文说明PR #10066 的 Autofix 轮次报告 — 未采取任何操作本轮反馈中没有任何需要处理的内容，因此未做任何代码修改，也未创建任何提交。 本轮收到的内容
关于列出的两个检查项"Failed checks" 下仅有的条目是 增长状态diff 增长窗口在预算之内（源码净增 46 行 / 测试净增 257 行，预算为 400/400），本轮也不要求增长审计。 PR 头部仍停留在上次已验证的提交；分支保持不变。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D2-2 resolver→bridge wiring test gap at the four createAcpSessionBridge call sites — already recorded in the round-2 review (rv:5031577638) deferred list
- D2-4 copyFrom primary-absent fallback-merge test gap — already recorded in the round-2 review (rv:5031577638) deferred list
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
docs/users/qwen-serve.md:722 — [review] PR description still claims archiving clears both attachment roots
Convergence: round 3 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/acp-bridge/src/sessionAttachments.ts (findings in round 2; 2 more now); docs/users/qwen-serve.md (findings in round 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/sessionAttachments.ts（第 2 轮已出过发现，本轮又有 2 条）；docs/users/qwen-serve.md（第 2 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const fallbackHit = | ||
| (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; |
There was a problem hiding this comment.
[Critical] remove() unlinks the fallback root first, then the primary, and tryUnlink rethrows any non-ENOENT fault without probing existence — on a read-only-mounted legacy volume, unlink fails EROFS at mnt_want_write BEFORE the existence check, so deleting an attachment that exists only in the primary root rejects even though the fallback holds no copy and the integrity rationale (don't resurrect stale fallback bytes) cannot apply. Every single-attachment delete of a post-switch session then fails until the legacy volume is repaired, while whole-session delete() already tolerates the same fault (best-effort fallback). The primary-side mirror at line 607 is the same mechanism in reverse: with the configured root degraded (RO-remounted, dead NFS), deleting a fallback-held attachment unlinks the fallback copy successfully and then faults on the primary unlink — the DELETE route errors, the follow-up GET returns 404 because read() degrades cleanly on the same fault, and every retry fails permanently although nothing remains to delete. Both directions falsify the doc clauses this diff adds ("reads/removes that miss the configured root fall back to the default runtime temp dir", "removable while the default fallback dir stays writable"). Witness (probe on the unmodified PR code; EROFS-on-absent-name modeled via mock — the same semantic the PR's own test 'keeps the primary readable when the fallback unlink fails' uses, because a real RO mount cannot be created on the runner): fallback side — remove('fresh.txt') → rejected EROFS, primaryStillThere=true, only the fallback path attempted; with an existence-probe fix it resolves and the primary is cleaned, all 64 existing tests still green. Primary side — first remove threw EROFS with the fallback bytes already unlinked, GET → 404, retry threw EROFS again; with the fix it resolves. Fix: in tryUnlink (or at both call sites), on a non-ENOENT unlink error stat the path in that root and treat the fault as a miss when the name is absent; rethrow only when the file genuinely exists — applied symmetrically so both entrances get one fix:
let fallbackHit = false;
if (this.persistentFallbackDirectory) {
try {
fallbackHit =
(await this.tryUnlink(this.persistentFallbackDirectory, name)) === true;
} catch (error) {
// Only an actually-present legacy copy can resurrect stale bytes; an
// absent one must not block the remove when its volume faults.
if (
statSizeStrict(path.join(this.persistentFallbackDirectory, name)) !==
undefined
) {
throw error;
}
}
}
const primaryDir = await this.peekDirectory();
let primaryHit = false;
if (primaryDir) {
try {
primaryHit = (await this.tryUnlink(primaryDir, name)) === true;
} catch (error) {
if (statSizeStrict(path.join(primaryDir, name)) !== undefined) {
throw error;
}
}
}
return primaryHit || fallbackHit;Acceptance criterion: new cases in sessionAttachments.test.ts — a primary-only attachment with the mocked fallback unlink throwing EROFS must let remove() resolve true with the primary file gone (plus the symmetric fallback-only case); remove the existence probe from the fix and both must go red, while the existing both-roots-present test stays red-on-primary-deletion.
中文说明
remove() 先删回退目录、再删主目录，且 tryUnlink 对任何非 ENOENT 错误都会重新抛出而不先探测文件是否存在——当旧默认目录所在卷被只读挂载时，unlink 会在存在性检查之前于 mnt_want_write 处返回 EROFS，因此删除仅存在于主目录的附件也会被拒绝，即使回退目录中根本没有副本、注释中的完整性理由（防止陈旧回退字节复活）在此场景也不可能成立。切换后会话的每个单附件删除都会在旧卷修复前一直失败，而整会话 delete() 已经能容忍同样的故障（回退目录尽力而为）。第 607 行的主目录侧是同一机制的镜像：配置根目录损坏（只读重挂载、NFS 失效）时，删除仅存于回退目录的附件会先成功删除回退副本，随后在主目录 unlink 处失败——DELETE 路由报错，后续 GET 却因 read() 在同一故障下正常降级而返回 404，且每次重试都永久失败，尽管已无任何字节可删。两个方向都与本 diff 新增的文档承诺（"未命中配置根目录的读取/删除会回退到默认运行时临时目录"、"在默认回退目录保持可写时可删除"）矛盾。实测（未改动的 PR 代码；"缺失文件也返回 EROFS" 的语义通过 mock 建模——与本 PR 自身测试 'keeps the primary readable when the fallback unlink fails' 使用的语义一致，因为该 runner 无法创建真实只读挂载）：回退侧 remove('fresh.txt') → EROFS 拒绝、primaryStillThere=true、仅尝试了回退路径；加入存在性探测修复后可正常解析且主目录被清理，既有 64 个测试全部仍通过。主目录侧：首次删除在回退字节已被删除的情况下抛 EROFS，GET → 404，重试仍抛 EROFS；修复后可正常解析。修复：在 tryUnlink（或两处调用点）中，对非 ENOENT 的 unlink 错误先在该根目录 stat 该路径，若文件不存在则按"未命中"处理；仅当文件确实存在时才重新抛出——两侧对称应用，一个修复同时关闭两个入口（修复代码见上方英文部分）。验收标准：在 sessionAttachments.test.ts 新增用例——仅主目录存在的附件 + mock 回退侧 unlink 抛 EROFS 时，remove() 应解析为 true 且主目录文件消失（以及对称的"仅回退目录存在"用例）；移除修复中的存在性探测后两个用例必须变红，同时既有的"双目录同名"测试在"先删主目录"变异下仍须保持红色。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Critical] A non-ENOENT readdir failure on the degraded PRIMARY source directory rejects the whole copyFrom before the fallback directory is scanned — so the healthy fallback entries this diff adds to the copy source are never copied on branch, even though read() three lines up explicitly serves them under the identical fault ("A degraded primary root must not hide healthy fallback bytes"). Concrete trigger: a post-migration session whose primary volume degrades after boot (EACCES/EIO on readdir of the primary session dir — the exact fault state the new read tests model); the user branches the session, copyFrom iterates [primary, fallback], the primary readdir throws, and bridge.ts reduces the failure to a stderr line — the branch is created with zero attachments although the legacy fallback bytes are healthy and still served by reads on the source session. Before this diff a primary fault could only lose primary entries; adding the fallback to sourceDirectories is what makes a primary fault now hide the fallback bytes too. Witness (probe with a real, unmocked fault — primary session dir chmod 000 so readdir fails EACCES): PR arm — copyFrom → rejected EACCES, targetReadLegacy=undefined; fix arm (per-directory degrade) — copyFrom → resolved, targetReadLegacy="legacy bytes". Fix: catch the non-ENOENT readdir error per directory and continue to the next source directory, mirroring the read-path degradation; primary-wins conflict order is preserved because a failed directory contributes no copiedNames.
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | |
| throw error; | |
| } | |
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | |
| } catch (error) { | |
| // A degraded primary root must not hide healthy fallback bytes: any | |
| // per-directory readdir fault degrades to the next source directory, | |
| // mirroring the read path (ENOENT — dir absent — takes the same path). | |
| continue; | |
| } |
Acceptance criterion: a new test in the fallback describe block — source store holding legacy.txt only in the fallback root, mock fs.readdir to reject EIO for the primary session dir; assert copyFrom resolves and the target reads legacy.txt. Reverting the continue (rethrowing) must turn it red.
中文说明
主（配置）根目录损坏时，其会话目录上的非 ENOENT readdir 失败会在扫描回退目录之前拒绝整个 copyFrom——本 diff 刚加入复制来源的健康回退条目因此永远不会被复制到分支会话，而上方三行的 read() 在完全相同的故障下却明确会提供这些字节（"损坏的主目录不得遮蔽健康的回退字节"）。具体触发：切换后会话的主卷在启动后损坏（主会话目录 readdir 返回 EACCES/EIO——正是新增读取测试所建模的故障状态）；用户分支该会话时，copyFrom 遍历 [主目录, 回退目录]，主目录 readdir 抛错，bridge.ts 把失败降级为一行 stderr 日志——分支会话在遗留回退字节健康且源会话读取仍正常的情况下被创建为零附件。本 diff 之前，主目录故障只会丢失主目录条目；正是把回退目录加入 sourceDirectories 才使得主目录故障现在也会遮蔽回退字节。实测（真实未 mock 的故障——主会话目录 chmod 000 使 readdir 返回 EACCES）：PR 侧 copyFrom → EACCES 拒绝、targetReadLegacy=undefined；修复侧（按目录降级）→ 解析成功、targetReadLegacy="legacy bytes"。修复：按目录捕获非 ENOENT 的 readdir 错误并继续下一个源目录，与读取路径的降级保持一致；"主目录优先"的冲突顺序不受影响，因为失败的目录不会贡献任何 copiedNames（修复代码见上方 suggestion 块）。验收标准：在 fallback describe 块新增测试——源存储仅回退目录中有 legacy.txt，mock 主会话目录的 fs.readdir 拒绝 EIO；断言 copyFrom 解析成功且目标可读 legacy.txt；把 continue 还原为重新抛出后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // Mirror deleteDaemonSessions: a reaped orphan is never looked up | ||
| // again, and close() on a persistent store deletes nothing — without | ||
| // this the attachment bytes leak from both storage roots. | ||
| await bridge.deleteSessionAttachments(sessionId); |
There was a problem hiding this comment.
[Suggestion] The orphan reaper awaits bridge.deleteSessionAttachments(sessionId) unguarded after a successful persisted removal, but SessionAttachmentStore.delete() deliberately rethrows a fallback-removal fault after primary cleanup (pinned by the 'still clears the primary directory when the fallback removal fails' test) — so a legacy-volume fault rejects an already-successful session removal, unlike deleteDaemonSessions, which wraps the identical call in try/catch and reports via onError; the "Mirror deleteDaemonSessions" comment overstates the equivalence. Concrete consequence: with the env var set and the legacy temp dir on a volume where unlink fails (EACCES/EROFS), the rejection propagates — at the branch-workspace spawn failure path (routes/session.ts ≈3168) the catch branch skips removeUserWorktree/branch rollback (only run when removed === true), leaking the git worktree on disk and pessimistically blocking the workspace on a session id that no longer exists; the .catch(() => false) / .catch(() => {}) call sites swallow the failure with no diagnostic. Pre-diff this path returned true and cleanup proceeded. Witness (probe): PR arm — persistedRowStillExists=false, rejects with the read-only-volume error after the persisted row is already gone; fix arm (try/catch, still return the removal) — resolves true. Fix: wrap the call in try/catch, surface the failure through the same diagnostics deleteDaemonSessions uses, and still return the removal result — the persisted row and primary bytes are already gone, so callers should proceed as removed. (If a fallback failure is truly meant to be fatal here, drop the "Mirror" comment instead, since the mirror catches and reports.)
Acceptance criterion: a new case in session-archive.test.ts beside 'deletes the transcript when killSession resolves true' where deleteSessionAttachments rejects — assert deleteDaemonSessionIfOrphan still resolves true, the session file is gone, and markSessionCatalogChanged fires; remove the try/catch and it must go red.
中文说明
孤儿回收器在持久化删除成功后直接 await bridge.deleteSessionAttachments(sessionId)，未加任何保护；而 SessionAttachmentStore.delete() 有意在清理主目录之后重新抛出回退目录删除失败（由 'still clears the primary directory when the fallback removal fails' 测试钉住）——因此旧卷故障会拒绝一次已经成功的会话删除，这与 deleteDaemonSessions 不同：后者用 try/catch 包住同样的调用并通过 onError 上报。"Mirror deleteDaemonSessions" 注释夸大了两者的等价性。具体后果：设置环境变量且旧临时目录所在卷 unlink 失败（EACCES/EROFS）时，拒绝会向上传播——在分支工作区创建失败路径（routes/session.ts ≈3168）中，catch 分支会跳过 removeUserWorktree/分支回滚（仅在 removed === true 时执行），导致 git worktree 泄漏在磁盘上，并使工作区对一个已不存在的会话 id 保持悲观阻塞；.catch(() => false) / .catch(() => {}) 调用点则无声吞掉失败、无任何诊断信息。本 diff 之前该路径返回 true 且清理正常进行。实测：PR 侧——持久化行已删除（persistedRowStillExists=false）后仍因只读卷错误被拒绝；修复侧（try/catch 后仍返回删除结果）——解析为 true。修复：用 try/catch 包住该调用，用与 deleteDaemonSessions 相同的诊断通道暴露失败，并仍然返回删除结果——持久化行与主目录字节均已删除，调用方应按"已删除"继续。（若回退失败在此确实应视为致命错误，请删去 "Mirror" 注释，因为被镜像的一方是捕获并上报而非传播。）验收标准：在 session-archive.test.ts 中 'deletes the transcript when killSession resolves true' 旁新增 deleteSessionAttachments 拒绝的用例——断言 deleteDaemonSessionIfOrphan 仍解析为 true、会话文件消失且 markSessionCatalogChanged 被调用；移除 try/catch 后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const removal = await deletePersistedSessionWithLease(service, sessionId); | ||
| if (removal.kind !== 'error') { |
There was a problem hiding this comment.
[Suggestion] The new guard that skips deleteSessionAttachments when the persisted orphan removal returns kind: 'error' has no test asserting the skip. The existing lease-conflict test ('throws when the lease is held by another writer') drives exactly this result via SessionWriterConflictError, but holds a resolving deleteSessionAttachments mock and makes no call-count assertion. Deleting the guard condition is a silent data-loss mutation: a transient failure removing the persisted row (lease conflict surfacing as an error result, disk I/O fault) leaves the session row alive but deletes its attachment bytes from both roots, and any later read/restore of the still-persisted session serves dangling attachment references. Witness (mutation run): baseline 65/65 green; mutant with the guard condition deleted — 65/65 still green, the mutation survives the entire existing suite; the proposed test fails on the mutant (expected "spy" to not be called at all, but actually been called 1 times) and passes on the unmodified PR code. Fix: extend session-archive.test.ts with a case where the persisted removal fails — e.g. spy service.removeSession to reject inside the lease mutation — and assert deleteDaemonSessionIfOrphan rejects while deleteSessionAttachments was NOT called and the session file still exists.
Acceptance criterion: that test — removing the kind !== 'error' condition must turn it red (deleteSessionAttachments would be called once).
中文说明
新增的"持久化孤儿删除返回 kind: 'error' 时跳过 deleteSessionAttachments"守卫没有任何测试断言该跳过行为。既有的租约冲突测试（'throws when the lease is held by another writer'）恰好通过 SessionWriterConflictError 触发了这一结果，但其 deleteSessionAttachments mock 是解析成功的，且没有任何调用次数断言。删除该守卫条件是一个静默数据丢失变异：持久化行删除的瞬时失败（租约冲突以错误结果形式出现、磁盘 I/O 故障）会让会话行仍然存活、但其附件字节被从两个根目录删除，之后任何对该仍持久化会话的读取/恢复都会拿到悬空的附件引用。实测（变异运行）：基线 65/65 全绿；删除守卫条件的变异体——65/65 依然全绿，该变异在整个既有测试套件下存活；建议的新测试在变异体上失败（expected "spy" to not be called at all, but actually been called 1 times），在未改动的 PR 代码上通过。修复：在 session-archive.test.ts 中新增持久化删除失败的用例——例如 spy service.removeSession 使其在租约变更中拒绝——并断言 deleteDaemonSessionIfOrphan 被拒绝、deleteSessionAttachments 未被调用且会话文件仍然存在。验收标准：即该测试本身——移除 kind !== 'error' 条件后它必须变红（deleteSessionAttachments 将被调用一次）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
|
||
| Scope and limits: | ||
|
|
||
| - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable, and removable while the default fallback dir stays writable — a remove whose legacy copy cannot be unlinked (e.g. a read-only fallback volume) surfaces the error rather than reporting success. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. |
There was a problem hiding this comment.
[Suggestion] Reviewer Test Plan step 3 instructs verifying the REVERSE of the one-way migration this bullet documents: "Then remove the env var, restart, and confirm the same attachment is still readable (fallback path)". With the env unset, sessionAttachmentsRoots() returns only { root: defaultRoot } — no fallback is constructed (packages/cli/src/serve/session-attachments-root.ts) — so an attachment written to the configured root is unreachable by design and the step always fails. This bullet itself states the reverse direction makes configured-root attachments unreachable, and the PR's Risk & Scope section says the same. A reviewer following step 3 as written uploads with QWEN_SERVE_SESSION_ATTACHMENTS_ROOT set (bytes land under the configured root), unsets the var, restarts, reads the attachment — unreachable — and files a false fallback-regression against correct, test-pinned code, costing a review round. Fix: rewrite step 3 to exercise the shipped direction — upload an attachment with the env unset, then set QWEN_SERVE_SESSION_ATTACHMENTS_ROOT, restart, and confirm new uploads land under the configured root while the pre-switch attachment stays readable/removable via the fallback; note that unsetting the var afterwards makes configured-root attachments unreachable.
中文说明
Reviewer Test Plan 第 3 步要求验证的恰是本条目所记载的单向迁移的反方向："取消该环境变量、重启，并确认同一附件仍可读（回退路径）"。环境变量未设置时，sessionAttachmentsRoots() 只返回 { root: defaultRoot }——不会构造任何回退目录（packages/cli/src/serve/session-attachments-root.ts）——因此写入配置根目录的附件按设计就不可达，该步骤必然失败。本条目自身已写明反向操作会使配置根目录中的附件不可达，PR 的 Risk & Scope 部分也有同样说明。按第 3 步原文操作的评审者会在设置 QWEN_SERVE_SESSION_ATTACHMENTS_ROOT 时上传（字节落入配置根目录），然后取消变量、重启、读取附件——不可达——并对正确且有测试钉住的代码提交一份虚假的"回退回归"报告，浪费一轮评审。修复：把第 3 步改写为实际支持的方向——先在未设置环境变量时上传附件，然后设置 QWEN_SERVE_SESSION_ATTACHMENTS_ROOT 并重启，确认新上传落入配置根目录、同时切换前上传的附件仍可经回退目录读取/删除；并注明此后再取消变量会使配置根目录中的附件不可达。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
🤖 AutoFix ran out of time before finishing (timeout (2700000ms)) (attempt 4/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (2700000ms)）（第 4/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33059559161 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix ran out of time before finishing (timeout (3600000ms)) (attempt 5/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (3600000ms)）（第 5/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33075432245 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Could not produce a passing fix for this feedback (round 6/100) — the verification gate rejected the attempt. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. Autofix round summary — PR #10066 (address-review)Round scope: same-run verification repair. The previous round's commit ( Conflict resolution (
|
…nts-root-env # Conflicts: # packages/acp-bridge/src/sessionAttachments.ts # packages/cli/src/serve/server.ts
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10066 (same-run verification repair)This round carried no review feedback to triage: no reviews, no inline comments, no issue-level comments, and no failed or still-red checks were listed. The actionable items were the two workflow-supplied ones: repair the deterministic rejection ("tests failed in packages/cli") and the base-conflict item. 1. Gate rejection — root cause and fixThe rejection: Diagnosis, from the failure trace itself (its line numbers byte-match the current source): the throw left Why the oracle missed only on the gate hosts: the fixture builds its repository with git calls that inherit the raw process environment, while the implementation's restore/oracle spawns sanitize their environment BY DESIGN — The correlation in the gate run is decisive: all 259 other files passed — including every sibling real-git suite that isolates ( Fix (minimal, test-only, source untouched): wrap Evidence and probes (all in this checkout, git 2.39.5):
2. Base conflict (
|
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D2-2 resolver→bridge wiring test gap at the four createAcpSessionBridge call sites — already recorded in the round-2 review (rv:5031577638) deferred list
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/sessionAttachments.test.ts:1758 — [review] no test pins the fallback tombstone fs.rm — a dropped-rm mutant was measured greenpackages/cli/src/serve/server/session-archive.ts:651 — [review] R3-3 orphan reaper awaits deleteSessionAttachments unguardedpackages/cli/src/serve/server/session-archive.ts:647 — [review] R3-4 no test pins the kind:'error' skip guarddocs/users/qwen-serve.md:720 — [review] R3-5 Reviewer Test Plan step 3 verifies the reverse of the one-way migration
Convergence: round 4 posted 3 inline comment(s), 1 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/acp-bridge/src/sessionAttachments.ts (findings in round 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 3 条行内评论，其中 1 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/sessionAttachments.ts（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const fallbackHit = | ||
| (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; | ||
| const primaryHit = | ||
| (await this.tryUnlink(await this.peekDirectory(), name)) === true; | ||
| return primaryHit || fallbackHit; |
There was a problem hiding this comment.
[Critical] R3-1: remove() unlinks the fallback root first, then the primary, and tryUnlink rethrows any non-ENOENT unlink fault without probing existence — on a read-only or degraded volume, unlink fails (EROFS at the mount write check, EACCES during path walk) BEFORE any existence check, so a fault on a root that holds NO copy aborts the whole remove. With the env set and a post-switch attachment held only in the configured root, a degraded legacy default dir rejects every single-attachment delete of that session even though the fallback holds nothing; mirrored, a pre-switch attachment held only in the fallback unlinks fine and then the primary unlink of an absent name throws — the DELETE rejects after the bytes are already gone, and every retry rejects permanently while read() of the same file degrades gracefully over the identical fault. Both directions falsify the fallback-remove promises this PR adds ("removable while the default fallback dir stays writable" in docs/users/qwen-serve.md; "Reads/removes that miss the configured root fall back to the default runtime temp dir" in docs/developers/daemon/17-configuration.md). Verified by execution on this commit with real filesystem faults:
BUGGY r31a: outcome="rejected EACCES" primaryStillThere=true
BUGGY r31b: first="rejected EACCES" fallbackEntriesAfterFirst=[] second="rejected EACCES" readAfter="undefined (graceful)"
FIXED r31a-erofs: outcome="resolved removed=true" primaryStillThere=false
FIXED r31b: first="resolved removed=true"
In tryUnlink (or symmetrically at both call sites), on a non-ENOENT unlink error stat the path in that root and treat the fault as a miss when the name is absent, rethrowing only when the file genuinely exists; note a stat probe does not cover the lookup-denied variant, so handle that case too. When this lands, add the two probe cases to the fallback root describe block in src/sessionAttachments.test.ts (primary-only copy + EROFS faulting fallback unlink; fallback-only copy + faulting primary unlink) and confirm removing the probe turns both red.
中文说明
remove() 先删回退目录、再删主目录，且 tryUnlink 对任何非 ENOENT 的 unlink 故障不做存在性探测就直接重抛——在只读或降级卷上，unlink 在任何存在性检查之前就失败（只读挂载在 mnt_want_write 处抛 EROFS、路径遍历时抛 EACCES），因此没有副本的根目录出故障也会中止整个删除。设置环境变量后，仅存于配置根的切换后附件：旧的默认目录降级时，该会话的每次单附件删除都会被拒绝——尽管回退目录里什么都没有；反方向同理，仅存于回退目录的切换前附件先被成功删除，随后主侧对不存在文件的 unlink 抛错——字节已删完，DELETE 却被拒绝，且每次重试都永久失败，而同一故障下 read() 能优雅降级。两个方向都与本 PR 新增的回退删除承诺相矛盾（docs/users/qwen-serve.md 的 "removable while the default fallback dir stays writable"、docs/developers/daemon/17-configuration.md 的 "Reads/removes that miss the configured root fall back to the default runtime temp dir"）。已在本提交上用真实文件系统故障实测验证（见上方输出）。
建议：在 tryUnlink（或两个调用处对称地）中，对非 ENOENT 的 unlink 错误在该根目录中 stat 该路径，名字不存在时按"未命中"处理，仅当文件确实存在时才重抛；注意 stat 探测无法覆盖"查找本身被拒绝"的变体，需一并处理。修复后请在 src/sessionAttachments.test.ts 的 fallback root 块中补两个用例（仅主目录副本 + 回退 unlink 抛 EROFS；仅回退副本 + 主侧 unlink 故障），并确认移除探测后两者变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | ||
| throw error; |
There was a problem hiding this comment.
[Critical] R3-2: copyFrom rethrows a non-ENOENT readdir failure on the degraded PRIMARY source directory before the fallback directory is ever scanned — so the healthy fallback entries this diff adds to the copy source are never copied on branch, even though read() twenty lines above explicitly serves them under the identical fault ("A degraded primary root must not hide healthy fallback bytes"). A post-migration session whose configured volume degrades after boot (the exact fault state the new read tests model) branches into a session with ZERO attachments, although the legacy fallback bytes are healthy and still served by reads on the source session — the branch caller in bridge.ts catches the failure, writes one stderr line, and completes the branch. Verified by execution on this commit with a real chmod-000 readdir fault:
BUGGY r32: outcome="rejected EACCES" targetEntries=[] branchReadOfLegacy="undefined"
FIXED r32: outcome="resolved" targetEntries=["legacy.txt"] branchReadOfLegacy="data:legacy-bytes (degraded: true)"
Catch the non-ENOENT readdir error per directory and continue to the next source directory, mirroring the read-path degradation; primary-wins conflict order is preserved because a failed directory contributes no copiedNames. When this lands, add the probe case to the fallback root describe block in src/sessionAttachments.test.ts (legacy bytes held only in the fallback root, primary readdir mocked to reject EIO, copyFrom resolves and the target reads the file) and confirm reverting the continue to a rethrow turns it red.
中文说明
copyFrom 对 PRIMARY 源目录的非 ENOENT readdir 故障直接重抛，在扫描回退目录之前就整体中止——因此本 diff 新加入复制源的健康回退条目在分支时永远不会被复制，而上方约二十行的 read() 在完全相同的故障下明确提供回退字节（"A degraded primary root must not hide healthy fallback bytes"）。切换后会话的配置卷在启动后降级（正是新的读取测试所建模的故障状态）时，分支出的会话附件数为零——尽管旧的回退字节是健康的、且源会话的读取仍能提供它们——bridge.ts 中的分支调用方捕获该失败、写一行 stderr 后即完成分支。已在本提交上用真实 chmod-000 readdir 故障实测验证（见上方输出）。
建议：按目录捕获非 ENOENT 的 readdir 错误并 continue 到下一个源目录，与读取路径的降级保持一致；主目录优先的冲突顺序不受影响，因为失败的目录不会贡献任何 copiedNames。修复后请在 src/sessionAttachments.test.ts 的 fallback root 块中补用例（旧字节仅存于回退根、主目录 readdir mock 为 EIO 拒绝、copyFrom 正常返回且目标可读该文件），并确认把 continue 还原为重抛后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const tombstone = path.join( | ||
| path.dirname(directory), | ||
| `.${path.basename(directory)}.deleting-${randomUUID()}`, | ||
| ); |
There was a problem hiding this comment.
[Critical] R4-1: The fallback-root deletion uses a random-UUID tombstone name per call, defeating the resumability the sibling primary path (removeDirectoryDurably) explicitly documents — its fixed .deleting name exists precisely so "a deletion interrupted between the rename and the removal [can] resume on the next call instead of leaking the tombstone". With the env set, every session deletion runs this helper on the legacy fallback dir; if the daemon is SIGKILLed during fs.rm (the window spans the whole recursive rm), or fs.rm fails mid-way on a large tree, the tombstone with all of the session's deleted bytes survives: both callers deleted the persisted row first, and a retry — which does re-reach this code via the kind: 'notFound' fall-through — computes a FRESH UUID, swallows the ENOENT rename, force-rms a never-existing name, and resolves success. No sweeper for .deleting* exists anywhere in the repo, so user-deleted attachment bytes persist indefinitely on the legacy volume while the delete reported success. Verified by execution on this commit, replaying the crash-leftover state:
Arm A (PR code, UUID leftover + retry): fallbackRootAfterRetry=[".session-s-1.deleting-11111111-…"], crashedTombstoneStillPresent=true (delete() resolved success)
Arm B (one-line fixed-name fix): fallbackRootAfterRetry=[], crashedTombstoneStillPresent=false (retry resumes and cleans up)
Control: the primary-side fixed-name leftover IS resumed by a retry under unmodified code.
Route the fallback deletion through the existing resumable path — call removeDirectoryDurably(this.persistentFallbackDirectory, options.assertCanCommit) in delete() (it no-ops when the directory or its parent is absent, and the two roots' parents are distinct so the fixed tombstone names cannot collide) — or add a startup sweep for leftover .session-*.deleting* entries. When this lands, add a test that plants a crash-state tombstone <fallbackRoot>/.session-<id>.deleting-<uuid> (containing a file) with no session dir present, calls store.delete(), and asserts the planted tombstone is gone — it is red today, since the fresh UUID never matches the planted tombstone.
中文说明
回退根目录的删除每次调用都使用随机 UUID 的 tombstone 名，破坏了兄弟主路径（removeDirectoryDurably）明确记载的可恢复性——后者固定的 .deleting 名正是为了让"在 rename 与删除之间被打断的删除能在下次调用时恢复，而不是泄漏 tombstone"。设置环境变量后，每次会话删除都会对旧回退目录执行该助手；若守护进程在 fs.rm 期间被 SIGKILL（窗口覆盖整个递归 rm），或 fs.rm 在大目录上中途失败，携带该会话全部已删除字节的 tombstone 将残留：两个调用方都已先删除持久行，而重试——确实会经由 kind: 'notFound' 的 fall-through 再次进入这段代码——会计算全新的 UUID、吞掉 ENOENT 的 rename、对从不存在的名字执行 force-rm，然后返回成功。全仓库不存在任何 .deleting* 清扫器，因此用户已删除的附件字节会在旧卷上无限期留存，而删除已报告成功。已在本提交上实测：重放崩溃残留状态（见上方输出：A 组为当前代码，重试后 tombstone 仍在且 delete() 返回成功；B 组为单行"固定名"修复后，重试可恢复并清理；对照组证明主侧固定名残留在当前代码下即可被重试恢复）。
建议：把回退删除改走现有的可恢复路径——在 delete() 中调用 removeDirectoryDurably(this.persistentFallbackDirectory, options.assertCanCommit)（目录或其父目录不存在时它会直接空操作，且两个根的父目录不同，固定 tombstone 名不会冲突）——或者增加启动时对残留 .session-*.deleting* 条目的清扫。修复后请补测试：放置崩溃状态的 tombstone <fallbackRoot>/.session-<id>.deleting-<uuid>（内含文件）且无会话目录，调用 store.delete() 并断言该残留被清除——当前实现下该测试为红（新 UUID 永远匹配不到预置的 tombstone）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D2-1 the two 'delete clears …' tests leak their mkdtemp roots into os.tmpdir() — already recorded in the round-2 review (rv:5031577638) deferred list
- D2-2 resolver→bridge wiring test gap at the four createAcpSessionBridge call sites — already recorded in the round-2 review (rv:5031577638) deferred list
- R3-3 orphan reaper awaits deleteSessionAttachments unguarded — already reported (comment 3869214414) and recorded in the round-4 review (rv:5047904653) deferred list
- R3-4 no test pins the kind:'error' skip guard — already reported (comment 3869214424) and recorded in the round-4 review (rv:5047904653) deferred list
- no test pins the fallback tombstone fs.rm — already recorded in the round-4 review (rv:5047904653) deferred list
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/sessionAttachments.test.ts:1605 — [probe] stat call-order assertions couple to a sibling test's mockRestoredocs/users/qwen-serve.md:722 — [review] archive-keeps-attachments contract pinned by no testpackages/cli/src/serve/server/session-archive.ts:647 — [probe] orphan cleanup skipped though the row deletion actually applied
Convergence: round 5 posted 4 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (1 new). Findings keep coming back to the same files: packages/acp-bridge/src/sessionAttachments.ts (findings in rounds 3, 4; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 4 条行内评论，其中 1 条是首次提出；上一轮发布了 3 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/sessionAttachments.ts（第 3、4 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; | ||
| throw error; |
There was a problem hiding this comment.
[Critical] R3-1: remove() unlinks the fallback root first, then the primary, and tryUnlink rethrows any non-ENOENT unlink fault without probing existence — on a read-only or degraded volume, unlink fails (EROFS at the mount write check, EACCES during path walk) BEFORE any existence check, so a fault on a root that holds NO copy aborts the whole remove. Still stands at c0eedf8a: the attempted fix commit was never pushed (the autofix verification gate rejected that round over an unrelated harness failure), and this code is unchanged since it was verified buggy by execution.
With the env set and a post-switch attachment held only in the configured root, a degraded legacy default dir rejects every single-attachment delete of that session even though the fallback holds nothing; mirrored, deleting a pre-switch attachment held only in the fallback unlinks the bytes fine and then the primary unlink of the absent name throws — the DELETE rejects after the bytes are already gone and every retry rejects permanently, while read() of the same file degrades gracefully over the identical fault. Both directions falsify the fallback-remove promises this PR adds ("removable while the default fallback dir stays writable" in docs/users/qwen-serve.md; "Reads/removes that miss the configured root fall back to the default runtime temp dir" in docs/developers/daemon/17-configuration.md).
Verified by execution on this code:
BUGGY r31a: outcome="rejected EACCES" primaryStillThere=true
BUGGY r31b: first="rejected EACCES" fallbackEntriesAfterFirst=[] second="rejected EACCES" readAfter="undefined (graceful)"
FIXED r31a-erofs: outcome="resolved removed=true" primaryStillThere=false
FIXED r31b: first="resolved removed=true"
In tryUnlink (or symmetrically at both call sites), on a non-ENOENT unlink error stat the path in that root and treat the fault as a miss when the name is absent, rethrowing only when the file genuinely exists; note a stat probe does not cover the lookup-denied variant, so handle that case too:
let fallbackHit = false;
if (this.persistentFallbackDirectory) {
try {
fallbackHit =
(await this.tryUnlink(this.persistentFallbackDirectory, name)) === true;
} catch (error) {
// Only an actually-present legacy copy can resurrect stale bytes; an
// absent one must not block the remove when its volume faults.
if (
statSizeStrict(path.join(this.persistentFallbackDirectory, name)) !==
undefined
) {
throw error;
}
}
}
const primaryDir = await this.peekDirectory();
let primaryHit = false;
if (primaryDir) {
try {
primaryHit = (await this.tryUnlink(primaryDir, name)) === true;
} catch (error) {
if (statSizeStrict(path.join(primaryDir, name)) !== undefined) {
throw error;
}
}
}
return primaryHit || fallbackHit;When this lands, add the two probe cases to the fallback root describe block in src/sessionAttachments.test.ts (primary-only copy + faulting fallback unlink; fallback-only copy + faulting primary unlink) and confirm removing the existence probe turns both red.
中文说明
R3-1 仍然存在：remove() 先删回退目录、再删主目录，且 tryUnlink 对任何非 ENOENT 的 unlink 故障不做存在性探测就直接重抛——在只读或降级卷上，unlink 在任何存在性检查之前就失败（只读挂载在 mnt_want_write 处抛 EROFS、路径遍历时抛 EACCES），因此没有副本的根目录出故障也会中止整个删除。在 c0eedf8a 上该问题依然存在：此前的修复提交因 autofix 验证门禁被一个无关的 harness 失败拒绝而从未推送，此代码自实测确认有 bug 以来未变。
设置环境变量后，仅存于配置根的切换后附件：旧的默认目录降级时，该会话的每次单附件删除都会被拒绝——尽管回退目录里什么都没有；反方向同理，仅存于回退目录的切换前附件先被成功删除，随后主侧对不存在文件的 unlink 抛错——字节已删完，DELETE 却被拒绝，且每次重试都永久失败，而同一故障下 read() 能优雅降级。两个方向都与本 PR 新增的回退删除承诺相矛盾（docs/users/qwen-serve.md 的 "removable while the default fallback dir stays writable"、docs/developers/daemon/17-configuration.md 的 "Reads/removes that miss the configured root fall back to the default runtime temp dir"）。
修复：在 tryUnlink（或两个调用处对称地）中，对非 ENOENT 的 unlink 错误在该根目录中 stat 该路径，名字不存在时按"未命中"处理，仅当文件确实存在时才重抛；注意 stat 探测无法覆盖"查找本身被拒绝"的变体，需一并处理（修复代码见上方英文部分）。修复后请在 src/sessionAttachments.test.ts 的 fallback root 块中补两个用例（仅主目录副本 + 回退 unlink 故障；仅回退副本 + 主侧 unlink 故障），并确认移除探测后两者变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | ||
| throw error; |
There was a problem hiding this comment.
[Critical] R3-2: copyFrom rethrows a non-ENOENT readdir failure on the degraded PRIMARY source directory before the fallback directory is ever scanned — so the healthy fallback entries this diff adds to the copy source are never copied on branch, even though read() above explicitly serves them under the identical fault ("A degraded primary root must not hide healthy fallback bytes"). Still stands at c0eedf8a: the attempted fix commit was never pushed; this code is unchanged. A post-migration session whose configured volume degrades after boot (the exact fault state the new read tests model) branches into a session with ZERO attachments, although the legacy fallback bytes are healthy and still served by reads on the source session — the branch caller in bridge.ts catches the failure, writes one stderr line, and completes the branch. Before this diff a primary fault could only lose primary entries; adding the fallback to sourceDirectories is what makes a primary fault now hide the fallback bytes too.
Verified by execution on this code with a real chmod-000 readdir fault:
BUGGY r32: outcome="rejected EACCES" targetEntries=[] branchReadOfLegacy="undefined"
FIXED r32: outcome="resolved" targetEntries=["legacy.txt"] branchReadOfLegacy="data:legacy-bytes (degraded: true)"
Catch the non-ENOENT readdir error per directory and continue to the next source directory, mirroring the read-path degradation; primary-wins conflict order is preserved because a failed directory contributes no copiedNames.
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | |
| throw error; | |
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | |
| } catch (error) { | |
| // A degraded primary root must not hide healthy fallback bytes: any | |
| // per-directory readdir fault degrades to the next source directory, | |
| // mirroring the read path (ENOENT — dir absent — takes the same path). | |
| continue; | |
| } |
When this lands, add the probe case to the fallback root describe block in src/sessionAttachments.test.ts (legacy bytes held only in the fallback root, primary readdir mocked to reject EIO, copyFrom resolves and the target reads the file) and confirm reverting the continue to a rethrow turns it red.
中文说明
R3-2 仍然存在：copyFrom 对 PRIMARY 源目录的非 ENOENT readdir 故障直接重抛，在扫描回退目录之前就整体中止——因此本 diff 新加入复制源的健康回退条目在分支时永远不会被复制，而上方约二十行的 read() 在完全相同的故障下却明确提供回退字节（"A degraded primary root must not hide healthy fallback bytes"）。在 c0eedf8a 上该问题依然存在：此前的修复提交从未推送，此代码未变。切换后会话的配置卷在启动后降级（正是新增读取测试所建模的故障状态）时，分支出的会话附件数为零——尽管旧的回退字节是健康的、且源会话的读取仍能提供它们——bridge.ts 中的分支调用方捕获该失败、写一行 stderr 后即完成分支。本 diff 之前，主目录故障只会丢失主目录条目；正是把回退目录加入 sourceDirectories 才使得主目录故障现在也会遮蔽回退字节。
修复：按目录捕获非 ENOENT 的 readdir 错误并继续下一个源目录，与读取路径的降级保持一致；"主目录优先"的冲突顺序不受影响，因为失败的目录不会贡献任何 copiedNames（修复代码见上方 suggestion 块）。修复后请在 src/sessionAttachments.test.ts 的 fallback root 块中补用例（旧字节仅存于回退根、主目录 readdir mock 为 EIO 拒绝、copyFrom 正常返回且目标可读该文件），并确认把 continue 还原为重抛后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const fallbackHit = | ||
| (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; | ||
| const primaryHit = | ||
| (await this.tryUnlink(await this.peekDirectory(), name)) === true; |
There was a problem hiding this comment.
[Critical] R5-1: remove()'s pendingNames guard is checked once at entry and never re-checked before the primary unlink — in the fallback-only state this PR introduces, a concurrent putAttachment of the same name can win the gap between the two unlinks and have its just-written bytes deleted by the in-flight remove: a silently lost, successfully-acknowledged upload. With QWEN_SERVE_SESSION_ATTACHMENTS_ROOT set, a session holding image.png only in the fallback root (the pre-switch state this feature exists to migrate) receives a DELETE attachment while a concurrent upload whose auto-generated name collides (routine for images) arrives: reservePendingName lands after remove's entry check, statSizeStrict now sees the fallback name free (remove just unlinked it), writeFile(..., { flag: 'wx' }) succeeds on the primary because the name never existed there, and remove's primary unlink then deletes the new bytes and returns true. The uploader already received a successful reference; every later use degrades to session_attachment_gone. Neither the bridge nor the HTTP layer serializes the two — storeSessionAttachment/removeSessionAttachment call the store directly, and withOwnerMutableSession takes a shared-count lock that explicitly permits concurrent holders of the same sessionId. Pre-PR this shape could not lose bytes: the file existed until the single unlink, so a concurrent put's wx failed EEXIST and took a suffix.
Verified by execution at c0eedf8a — gated probe (window forced open by parking remove() at its primary unlink):
SHIPPED: put_acknowledged {attachmentId: "image.png"} → remove_returned removed=true
→ read_acknowledged_id_after_remove: null
→ assertReference error session_attachment_gone (acknowledged upload silently deleted)
FLIP (primary-first mutant): put took suffix "image (1).png"; read after remove returns the fresh bytes
and ungated (no spies, fresh fixture dirs): the DELETE-first arm lost the acknowledged upload's bytes in 469/500 iterations (repeat run 480/500); the put-first arm lost 0/200 (the entry pendingNames check protects it).
Serialize put and remove rather than reordering — a naive primary-first revert conflicts with the pinned 'keeps the primary readable when the fallback unlink fails' test. E.g. a per-name/per-store mutation lock covering both putAttachment's write phase and remove()'s unlinks, or a remove-held name reservation that putAttachment's dedup loop treats as occupied. When this lands, add a test to src/sessionAttachments.test.ts (seed a fallback-only attachment X, stall remove('X')'s primary unlink until a concurrent putAttachment for X has resolved, assert read('X') returns the newly uploaded bytes) and confirm removing the serialization turns it red.
中文说明
R5-1（本轮新发现）：remove() 的 pendingNames 守卫只在入口检查一次，主目录 unlink 之前不再复查——在本 PR 引入的"仅回退目录"状态下，同名的并发 putAttachment 可以赢得两次 unlink 之间的空隙，使其刚写入的字节被进行中的 remove 删除：一次已成功确认的上传被静默丢失。设置 QWEN_SERVE_SESSION_ATTACHMENTS_ROOT 后，一个仅在回退目录持有 image.png 的会话（本功能要迁移的切换前状态）在收到 DELETE 附件请求的同时，一个自动生成名字冲突的并发上传到达：reservePendingName 落在 remove 的入口检查之后，statSizeStrict 此时看到回退目录中该名字已空闲（remove 刚删掉它），writeFile(..., { flag: 'wx' }) 在主目录成功（该名字在主目录本就不存在），随后 remove 的主目录 unlink 删掉了新写入的字节并返回 true。上传方已收到成功引用，之后每次使用都会降级为 session_attachment_gone。bridge 与 HTTP 层都没有把两者串行化——storeSessionAttachment/removeSessionAttachment 直接调用 store，而 withOwnerMutableSession 拿的是共享计数锁，明确允许同一 sessionId 的并发持有者。本 PR 之前该形态不会丢字节：文件在唯一一次 unlink 之前一直存在，并发 put 的 wx 会得到 EEXIST 并改用后缀名。
已在 c0eedf8a 上实测验证：受控探针（把 remove() 停在主目录 unlink 处强制打开窗口）——当前代码下已确认的上传被静默删除（读回为 null、引用校验报 session_attachment_gone）；翻转变异（主目录优先）后上传存活。无门控竞争测试：DELETE 先进入时 469/500 次丢失已确认上传（复测 480/500）；put 先进入时 0/200（入口 pendingNames 检查提供了保护）。
修复建议：用串行化而非调换顺序——简单地把主目录提到前面会与既有测试 'keeps the primary readable when the fallback unlink fails' 冲突。例如：用按名字/按 store 的互斥锁覆盖 putAttachment 的写入阶段与 remove() 的两次 unlink；或由 remove 持有一个名字预留，putAttachment 的去重循环将其视为已占用。修复后请补测试（预置仅回退目录存在的附件 X，把 remove('X') 的主目录 unlink 挂起直到并发 putAttachment 完成，断言 read('X') 返回新上传的字节），并确认移除串行化后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const tombstone = path.join( | ||
| path.dirname(directory), | ||
| `.${path.basename(directory)}.deleting-${randomUUID()}`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-1: The fallback-root deletion uses a random-UUID tombstone name per call, defeating the resumability the sibling primary path (removeDirectoryDurably) explicitly documents — its fixed .deleting name exists precisely so "a deletion interrupted between the rename and the removal [can] resume on the next call instead of leaking the tombstone". Still stands at c0eedf8a, re-confirmed by probe this round. With the env set, every session deletion runs this helper on the legacy fallback dir; if the daemon is SIGKILLed during fs.rm (the window spans the whole recursive rm), or fs.rm fails mid-way on a large tree, the tombstone with all of the session's deleted bytes survives: both callers deleted the persisted row first, and a retry — which does re-reach this code via the kind: 'notFound' fall-through — computes a FRESH UUID, swallows the ENOENT rename, force-rms a never-existing name, and resolves success. No sweeper for .deleting* exists anywhere in the repo, so user-deleted attachment bytes persist indefinitely on the legacy volume while the delete reported success.
Re-verified by probe at c0eedf8a:
PR arm: AFTER RETRY fallback root: [".session-s-1.deleting-<uuid>"] ← leak survives retry
Fix arm: fallback routed through removeDirectoryDurably → AFTER RETRY fallback root: []
Control: the primary-side fixed-name leftover IS resumed by a retry under unmodified code
Reassessed Critical → Suggestion this round: the leak is gated behind the env var being set + pre-switch fallback bytes + a crash or persistent fs.rm failure in the rename→rm window, lands in the legacy temp root, and has no happy-path correctness or security impact — but it survives retry, and the one-line fixed-name fix cleans it.
Route the fallback deletion through the existing resumable path — call removeDirectoryDurably(this.persistentFallbackDirectory, options.assertCanCommit) in delete() (it no-ops when the directory or its parent is absent, and the two roots' parents are distinct so the fixed tombstone names cannot collide) — or add a startup sweep for leftover .session-*.deleting* entries. When this lands, add a test that plants a crash-state tombstone <fallbackRoot>/.session-<id>.deleting-<uuid> (containing a file) with no session dir present, calls store.delete(), and asserts the planted tombstone is gone — it is red today, since the fresh UUID never matches the planted tombstone.
中文说明
R4-1 仍然存在（本轮经探针复核，严重度由 Critical 重新评估为 Suggestion）：回退根目录的删除每次调用都使用随机 UUID 的 tombstone 名，破坏了兄弟主路径（removeDirectoryDurably）明确记载的可恢复性——后者固定的 .deleting 名正是为了让"在 rename 与删除之间被打断的删除能在下次调用时恢复，而不是泄漏 tombstone"。设置环境变量后，每次会话删除都会对旧回退目录执行该助手；若守护进程在 fs.rm 期间被 SIGKILL（窗口覆盖整个递归 rm），或 fs.rm 在大目录上中途失败，携带该会话全部已删除字节的 tombstone 将残留：两个调用方都已先删除持久行，而重试——确实会经由 kind: 'notFound' 的 fall-through 再次进入这段代码——会计算全新的 UUID、吞掉 ENOENT 的 rename、对从不存在的名字执行 force-rm，然后返回成功。全仓库不存在任何 .deleting* 清扫器，因此用户已删除的附件字节会在旧卷上无限期留存，而删除已报告成功。
重新评估为 Suggestion 的理由：该泄漏需要同时满足"设置了环境变量 + 存在切换前的回退字节 + 在 rename→rm 窗口内崩溃或 fs.rm 持续失败"，且残留位于旧临时目录根，无正常路径的正确性或安全影响——但它能扛过重试，且单行"固定名"修复即可消除。
修复：把回退删除改走现有的可恢复路径——在 delete() 中调用 removeDirectoryDurably(this.persistentFallbackDirectory, options.assertCanCommit)（目录或其父目录不存在时它会直接空操作，且两个根的父目录不同，固定 tombstone 名不会冲突）——或者增加启动时对残留 .session-*.deleting* 条目的清扫。修复后请补测试：放置崩溃状态的 tombstone <fallbackRoot>/.session-<id>.deleting-<uuid>（内含文件）且无会话目录，调用 store.delete()，断言该残留被清除——当前实现下该测试为红（新 UUID 永远匹配不到预置的 tombstone）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- orphan-reaper attachment-delete failure path untested / semantics diverging from the mirrored deleteDaemonSessions — already reported as R3-3 (comment 3869214414)
- resolver→bridge wiring glue at the four createAcpSessionBridge call sites untested — already recorded as D2-2 in the round-2 review (rv:5031577638) deferred list
- kind:'error' skip guard has no witness in the lease-conflict test — already reported as R3-4 (comment 3869214424)
- fallback-root tombstone resumability (random UUID) — already reported as R4-1 (rounds 4-5); carried forward and deferred under the convergence posture this round
- the two 'delete clears …' tests leak their mkdtemp roots into os.tmpdir() — already recorded as D2-1 in the round-2 review (rv:5031577638) deferred list
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/sessionAttachments.ts:867 — [review] R4-1 still stands: fallback-root deletion uses a random-UUID tombstone per call, defeating the resumability of the fixed-name durable path — carried from rounds 4-5, deferred (con…packages/cli/src/serve/session-attachments-root.ts:12 — [probe] D6-1 env-var constant SESSION_ATTACHMENTS_ROOT_ENV never pinned against the documented literal — a value typo ships green and the daemon silently ignores the documented env var
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| try { | ||
| await fs.unlink(filePath); | ||
| await fs.unlink(path.join(directory, name)); | ||
| return true; | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | ||
| return false; | ||
| } | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; | ||
| throw error; |
There was a problem hiding this comment.
[Critical] R3-1: remove() unlinks the fallback root first, then the primary, and tryUnlink rethrows any non-ENOENT unlink fault without probing existence — on a read-only or degraded volume, unlink fails (EROFS at the mount write check, EACCES during path walk) BEFORE any existence check, so a fault on a root that holds NO copy aborts the whole remove. With the env set and a post-switch attachment held only in the configured root, a degraded legacy default dir rejects every single-attachment delete of that session even though the fallback holds nothing; mirrored, deleting a pre-switch attachment held only in the fallback unlinks the bytes fine and then the primary unlink of the absent name throws — the DELETE rejects after the bytes are already gone and every retry rejects permanently, while read() of the same file degrades gracefully over the identical fault. Both directions falsify the fallback-remove promises this PR adds ("removable while the default fallback dir stays writable" in docs/users/qwen-serve.md; "Reads/removes that miss the configured root fall back to the default runtime temp dir" in docs/developers/daemon/17-configuration.md). Still stands at this HEAD: the code is byte-identical to the round-5 commit (the autofix fix attempt was rejected by its verification gate and never pushed); re-traced at 027a2a5d this round.
Verified by execution on this code (round 5):
BUGGY r31a: outcome="rejected EACCES" primaryStillThere=true
BUGGY r31b: first="rejected EACCES" fallbackEntriesAfterFirst=[] second="rejected EACCES" readAfter="undefined (graceful)"
FIXED r31a-erofs: outcome="resolved removed=true" primaryStillThere=false
FIXED r31b: first="resolved removed=true"
In tryUnlink (or symmetrically at both call sites), on a non-ENOENT unlink error stat the path in that root and treat the fault as a miss when the name is absent, rethrowing only when the file genuinely exists; note a stat probe does not cover the lookup-denied variant, so handle that case too:
let fallbackHit = false;
if (this.persistentFallbackDirectory) {
try {
fallbackHit =
(await this.tryUnlink(this.persistentFallbackDirectory, name)) === true;
} catch (error) {
// Only an actually-present legacy copy can resurrect stale bytes; an
// absent one must not block the remove when its volume faults.
if (
statSizeStrict(path.join(this.persistentFallbackDirectory, name)) !==
undefined
) {
throw error;
}
}
}
const primaryDir = await this.peekDirectory();
let primaryHit = false;
if (primaryDir) {
try {
primaryHit = (await this.tryUnlink(primaryDir, name)) === true;
} catch (error) {
if (statSizeStrict(path.join(primaryDir, name)) !== undefined) {
throw error;
}
}
}
return primaryHit || fallbackHit;When this lands, add the two probe cases to the fallback root describe block in src/sessionAttachments.test.ts (primary-only copy + faulting fallback unlink; fallback-only copy + faulting primary unlink) and confirm removing the existence probe turns both red.
中文说明
R3-1 仍然存在：remove() 先删回退目录、再删主目录，且 tryUnlink 对任何非 ENOENT 的 unlink 故障不做存在性探测就直接重抛——在只读或降级卷上，unlink 在任何存在性检查之前就失败（只读挂载在 mnt_want_write 处抛 EROFS、路径遍历时抛 EACCES），因此没有副本的根目录出故障也会中止整个删除。在 027a2a5d 上该问题依然存在：此前的修复提交因 autofix 验证门禁拒绝而从未推送，此代码自实测确认有 bug 以来未变（本轮已重新核对）。
设置环境变量后，仅存于配置根的切换后附件：旧的默认目录降级时，该会话的每次单附件删除都会被拒绝——尽管回退目录里什么都没有；反方向同理，仅存于回退目录的切换前附件先被成功删除，随后主侧对不存在文件的 unlink 抛错——字节已删完，DELETE 却被拒绝，且每次重试都永久失败，而同一故障下 read() 能优雅降级。两个方向都与本 PR 新增的回退删除承诺相矛盾（docs/users/qwen-serve.md 的 "removable while the default fallback dir stays writable"、docs/developers/daemon/17-configuration.md 的 "Reads/removes that miss the configured root fall back to the default runtime temp dir"）。
修复：在 tryUnlink（或两个调用处对称地）中，对非 ENOENT 的 unlink 错误在该根目录中 stat 该路径，名字不存在时按"未命中"处理，仅当文件确实存在时才重抛；注意 stat 探测无法覆盖"查找本身被拒绝"的变体，需一并处理（修复代码见上方英文部分）。修复后请在 src/sessionAttachments.test.ts 的 fallback root 块中补两个用例（仅主目录副本 + 回退 unlink 故障；仅回退副本 + 主侧 unlink 故障），并确认移除探测后两者变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| try { | ||
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Critical] R3-2: copyFrom rethrows a non-ENOENT readdir failure on the degraded PRIMARY source directory before the fallback directory is ever scanned — so the healthy fallback entries this diff adds to the copy source are never copied on branch, even though read() twenty lines above explicitly serves them under the identical fault ("A degraded primary root must not hide healthy fallback bytes"). A post-migration session whose configured volume degrades after boot (the exact fault state the new read tests model) branches into a session with ZERO attachments, although the legacy fallback bytes are healthy and still served by reads on the source session — the branch caller in bridge.ts catches the failure, writes one stderr line, and completes the branch. Before this diff a primary fault could only lose primary entries; adding the fallback to sourceDirectories is what makes a primary fault now hide the fallback bytes too. Still stands at this HEAD: the code is byte-identical to the round-5 commit; re-traced at 027a2a5d this round.
Verified by execution on this code with a real chmod-000 readdir fault (round 5):
BUGGY r32: outcome="rejected EACCES" targetEntries=[] branchReadOfLegacy="undefined"
FIXED r32: outcome="resolved" targetEntries=["legacy.txt"] branchReadOfLegacy="data:legacy-bytes (degraded: true)"
Catch the non-ENOENT readdir error per directory and continue to the next source directory, mirroring the read-path degradation; primary-wins conflict order is preserved because a failed directory contributes no copiedNames.
| try { | |
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; | |
| throw error; | |
| } | |
| try { | |
| entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); | |
| } catch (error) { | |
| // A degraded primary root must not hide healthy fallback bytes: any | |
| // per-directory readdir fault degrades to the next source directory, | |
| // mirroring the read path (ENOENT — dir absent — takes the same path). | |
| continue; | |
| } |
When this lands, add the probe case to the fallback root describe block in src/sessionAttachments.test.ts (legacy bytes held only in the fallback root, primary readdir mocked to reject EIO, copyFrom resolves and the target reads the file) and confirm reverting the continue to a rethrow turns it red.
中文说明
R3-2 仍然存在：copyFrom 对 PRIMARY 源目录的非 ENOENT readdir 故障直接重抛，在扫描回退目录之前就整体中止——因此本 diff 新加入复制源的健康回退条目在分支时永远不会被复制，而上方约二十行的 read() 在完全相同的故障下却明确提供回退字节（"A degraded primary root must not hide healthy fallback bytes"）。在 027a2a5d 上该问题依然存在：此代码自第 5 轮以来未变（本轮已重新核对）。切换后会话的配置卷在启动后降级（正是新增读取测试所建模的故障状态）时，分支出的会话附件数为零——尽管旧的回退字节是健康的、且源会话的读取仍能提供它们——bridge.ts 中的分支调用方捕获该失败、写一行 stderr 后即完成分支。本 diff 之前，主目录故障只会丢失主目录条目；正是把回退目录加入 sourceDirectories 才使得主目录故障现在也会遮蔽回退字节。
修复：按目录捕获非 ENOENT 的 readdir 错误并继续下一个源目录，与读取路径的降级保持一致；"主目录优先"的冲突顺序不受影响，因为失败的目录不会贡献任何 copiedNames（修复代码见上方 suggestion 块）。修复后请在 src/sessionAttachments.test.ts 的 fallback root 块中补用例（旧字节仅存于回退根、主目录 readdir mock 为 EIO 拒绝、copyFrom 正常返回且目标可读该文件），并确认把 continue 还原为重抛后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const primaryHit = | ||
| (await this.tryUnlink(await this.peekDirectory(), name)) === true; |
There was a problem hiding this comment.
[Critical] R5-1: remove()'s pendingNames guard is checked once at entry and never re-checked before the primary unlink — in the fallback-only state this PR introduces, a concurrent putAttachment of the same name can win the gap between the two unlinks and have its just-written bytes deleted by the in-flight remove: a silently lost, successfully-acknowledged upload. With the env set and a session holding image.png only in the fallback root (the pre-switch state this feature exists to migrate), a DELETE attachment arrives while a concurrent upload whose auto-generated name collides (routine for images) arrives: reservePendingName lands after remove's entry check, statSizeStrict now sees the fallback name free (remove just unlinked it), writeFile(..., { flag: 'wx' }) succeeds on the primary because the name never existed there, and remove's primary unlink then deletes the new bytes and returns true. The uploader already received a successful reference; every later use degrades to session_attachment_gone. Neither the bridge nor the HTTP layer serializes the two — storeSessionAttachment/removeSessionAttachment call the store directly, and withOwnerMutableSession takes a shared-count lock that explicitly permits concurrent holders of the same sessionId. Pre-PR this shape could not lose bytes: the file existed until the single unlink, so a concurrent put's wx failed EEXIST and took a suffix. Still stands at this HEAD: the code is byte-identical to the round-5 commit; re-traced at 027a2a5d this round.
Verified by execution at round 5 — gated probe (window forced open by parking remove() at its primary unlink):
SHIPPED: put_acknowledged {attachmentId: "image.png"} → remove_returned removed=true
→ read_acknowledged_id_after_remove: null
→ assertReference error session_attachment_gone (acknowledged upload silently deleted)
FLIP (primary-first mutant): put took suffix "image (1).png"; read after remove returns the fresh bytes
and ungated (no spies, fresh fixture dirs): the DELETE-first arm lost the acknowledged upload's bytes in 469/500 iterations (repeat run 480/500); the put-first arm lost 0/200 (the entry pendingNames check protects it).
Serialize put and remove rather than reordering — a naive primary-first revert conflicts with the pinned 'keeps the primary readable when the fallback unlink fails' test. E.g. a per-name/per-store mutation lock covering both putAttachment's write phase and remove()'s unlinks, or a remove-held name reservation that putAttachment's dedup loop treats as occupied.
When this lands, add a test to src/sessionAttachments.test.ts (seed a fallback-only attachment X, stall remove('X')'s primary unlink until a concurrent putAttachment for X has resolved, assert read('X') returns the newly uploaded bytes) and confirm removing the serialization turns it red.
中文说明
R5-1（第 5 轮发现）仍然存在：remove() 的 pendingNames 守卫只在入口检查一次，主目录 unlink 之前不再复查——在本 PR 引入的"仅回退目录"状态下，同名的并发 putAttachment 可以赢得两次 unlink 之间的空隙，使其刚写入的字节被进行中的 remove 删除：一次已成功确认的上传被静默丢失。设置 QWEN_SERVE_SESSION_ATTACHMENTS_ROOT 后，一个仅在回退目录持有 image.png 的会话（本功能要迁移的切换前状态）在收到 DELETE 附件请求的同时，一个自动生成名字冲突的并发上传到达：reservePendingName 落在 remove 的入口检查之后，statSizeStrict 此时看到回退目录中该名字已空闲（remove 刚删掉它），writeFile(..., { flag: 'wx' }) 在主目录成功（该名字在主目录本就不存在），随后 remove 的主目录 unlink 删掉了新写入的字节并返回 true。上传方已收到成功引用，之后每次使用都会降级为 session_attachment_gone。bridge 与 HTTP 层都没有把两者串行化——storeSessionAttachment/removeSessionAttachment 直接调用 store，而 withOwnerMutableSession 拿的是共享计数锁，明确允许同一 sessionId 的并发持有者。本 PR 之前该形态不会丢字节：文件在唯一一次 unlink 之前一直存在，并发 put 的 wx 会得到 EEXIST 并改用后缀名。在 027a2a5d 上该问题依然存在：代码自第 5 轮以来未变（本轮已重新核对）。
已在第 5 轮实测验证：受控探针（把 remove() 停在主目录 unlink 处强制打开窗口）——当前代码下已确认的上传被静默删除（读回为 null、引用校验报 session_attachment_gone）；翻转变异（主目录优先）后上传存活。无门控竞争测试：DELETE 先进入时 469/500 次丢失已确认上传（复测 480/500）；put 先进入时 0/200（入口 pendingNames 检查提供了保护）。
修复建议：用串行化而非调换顺序——简单地把主目录提到前面会与既有测试 'keeps the primary readable when the fallback unlink fails' 冲突。例如：用按名字/按 store 的互斥锁覆盖 putAttachment 的写入阶段与 remove() 的两次 unlink；或由 remove 持有一个名字预留，putAttachment 的去重循环将其视为已占用。修复后请补测试（预置仅回退目录存在的附件 X，把 remove('X') 的主目录 unlink 挂起直到并发 putAttachment 完成，断言 read('X') 返回新上传的字节），并确认移除串行化后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
ytahdn
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
|
@qwen-code /takeover stop |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-3 orphan reaper propagates attachment-cleanup failure after a successful persisted removal — already reported (comment 3869214414)
- R3-4 the kind:'error' skip guard in deleteDaemonSessionIfOrphan has no test asserting the skip — already reported (comment 3869214424)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — no check was cut short..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/sessionAttachments.ts:701 — [probe] copyFrom both-roots-unreadable guard has no test — a deleted-guard mutant was measured green (75/75 + 806/806)packages/acp-bridge/src/sessionAttachments.test.ts:1956 — [probe] removingNames mechanism is pinned by zero tests — deleting it keeps the suite 75/75 green (positive control flips)packages/cli/src/serve/session-attachments-root.ts:30 — [probe] the ~\ home-expansion branch has no test — deleting the disjunct keeps the suite 12/12 green
Convergence: round 7 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (0 new). Findings keep coming back to the same files: packages/acp-bridge/src/sessionAttachments.ts (findings in rounds 3, 5; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R7-2: docs/users/qwen-serve.md:720 — the added one-way-migration bullet ("attachments uploaded before the switch remain readable, and removable while the default fallback dir stays writable") is wider than the code. remove() probes BOTH roots via hasAttachment before mutating, and hasAttachment rethrows any non-ENOENT stat fault — so a stat-denied CONFIGURED root (permissions revoked post-boot, NFS ESTALE/EACCES/EIO on stat) rejects every remove even when the only copy sits in the healthy writable fallback, although the doc's stated condition holds. The asymmetry is real: read() degrades any primary failure to the fallback (test-pinned), and remove() succeeds on fallback-only copies when the primary is merely absent, but no path removes when the primary is present yet stat-denied. Witness (probe, real chmod'd dirs): configured root stat-denied + attachment seeded only in the writable fallback — read() -> 'legacy bytes' (degrades, per doc) but remove() -> REJECTED code=EACCES at hasAttachment (sessionAttachments.ts:754), fallback bytes intact; configured root absent — remove() RESOLVES removed=true; flip (hasAttachment catch-return-false) — remove resolves. Fix the sentence, not the code (failing closed is defensible — unlinking only the fallback while the primary cannot be verified could leave a readable copy of a deleted attachment): e.g. "removable while the default fallback dir stays writable and the configured root can still be stat-verified; a remove whose configured-root lookup fails surfaces the error instead of degrading to the fallback (reads do degrade)". (Relocated from inline: its line overlaps existing comment 3869214429.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：none — no check was cut short.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 3 条（其中 0 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/sessionAttachments.ts（第 3、5 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R7-2: docs/users/qwen-serve.md:720 — the added one-way-migration bullet ("attachments uploaded before the switch remain readable, and removable while the default fallback dir stays writable") is wider than the code. remove() probes BOTH roots via hasAttachment before mutating, and hasAttachment rethrows any non-ENOENT stat fault — so a stat-denied CONFIGURED root (permissions revoked post-boot, NFS ESTALE/EACCES/EIO on stat) rejects every remove even when the only copy sits in the healthy writable fallback, although the doc's stated condition holds. The asymmetry is real: read() degrades any primary failure to the fallback (test-pinned), and remove() succeeds on fallback-only copies when the primary is merely absent, but no path removes when the primary is present yet stat-denied. Witness (probe, real chmod'd dirs): configured root stat-denied + attachment seeded only in the writable fallback — read() -> 'legacy bytes' (degrades, per doc) but remove() -> REJECTED code=EACCES at hasAttachment (sessionAttachments.ts:754), fallback bytes intact; configured root absent — remove() RESOLVES removed=true; flip (hasAttachment catch-return-false) — remove resolves. Fix the sentence, not the code (failing closed is defensible — unlinking only the fallback while the primary cannot be verified could leave a readable copy of a deleted attachment): e.g. "removable while the default fallback dir stays writable and the configured root can still be stat-verified; a remove whose configured-root lookup fails surfaces the error instead of degrading to the fallback (reads do degrade)". (Relocated from inline: its line overlaps existing comment 3869214429.)
— qwen3.8-max via Qwen Code /review (v0.22.2)
| entry.isFile() && | ||
| !source.pendingNames.has(entry.name) && | ||
| !copiedNames.has(entry.name), |
There was a problem hiding this comment.
[Critical] R7-1: copyFrom's entry filter checks source.pendingNames and copiedNames but never consults source.removingNames — the set this diff introduces. A remove() already in flight when a branch copy starts adds the name to removingNames synchronously before its first await, then crosses several awaited fs steps (peekDirectory, two hasAttachment stats, up to two unlinks); a copyFrom started during that window passes the entry checks (remove does not set copying, and copyFrom drains pending uploads but not in-flight removes), lists the doomed name via readdir before the unlink lands, copies it into the branch, and remove() then completes — a completed deletion is silently resurrected in the branched session. A remove started after copyFrom sets source.copying is rejected by remove()'s this.copying check, so this in-flight-before-copy window is the unguarded one. Neither the bridge nor the HTTP layer serializes the two requests: DELETE attachment and POST branch both run under archiveCoordinator.runSharedMany, which permits concurrent holders of the same sessionId.
Verified by execution (gated probe in an isolated scratch tree, remove('notes.txt') parked at its primary unlink, target.copyFrom(source) run while parked):
HEAD: removed=true | target.read(notes.txt): PRESENT bytes="doomed bytes" | source.read(notes.txt): undefined
FIXED (filter += !source.removingNames.has(entry.name)): target.read(notes.txt): undefined — suite still 75/75
| entry.isFile() && | |
| !source.pendingNames.has(entry.name) && | |
| !copiedNames.has(entry.name), | |
| entry.isFile() && | |
| !source.pendingNames.has(entry.name) && | |
| !source.removingNames.has(entry.name) && | |
| !copiedNames.has(entry.name), |
When this lands, add a test to src/sessionAttachments.test.ts — pause store.remove('img.png') mid-flight with an fs.unlink gate (as the existing concurrency test does), start target.copyFrom(source) while it is suspended, release, and assert remove resolves true AND target.read('img.png') resolves undefined — and confirm removing the filter turns it red.
中文说明
copyFrom 的条目过滤器检查 source.pendingNames 和 copiedNames，却从不查询本 diff 新引入的 source.removingNames。当分支复制开始时已有一个 remove() 在进行中：remove() 在第一个 await 之前同步把名字加入 removingNames，随后要跨越多个 await 的 fs 步骤（peekDirectory、两次 hasAttachment stat、至多两次 unlink）；在这个窗口内启动的 copyFrom 能通过入口检查（remove 不设置 copying，且 copyFrom 只排空待上传项、不等待进行中的删除），会在 unlink 落地之前通过 readdir 列出这个即将被删除的文件并把它复制进分支，随后 remove() 完成——一个已完成的删除在分支会话中被悄悄复活。在 copyFrom 设置 source.copying 之后才开始的 remove 会被 remove() 的 this.copying 检查拒绝，因此这个"删除先于复制开始"的窗口正是无人设防的一处。bridge 与 HTTP 层也没有把两者串行化：DELETE 附件与 POST 分支都运行在 archiveCoordinator.runSharedMany 之下，该锁明确允许同一 sessionId 的并发持有者。
已在隔离工作树中实测（受控探针：把 remove('notes.txt') 停在其主目录 unlink 处，在其悬停期间执行 target.copyFrom(source)）：当前代码下 removed=true 且 target.read(notes.txt) 仍能读到被判删的字节（源侧已为 undefined）——已完成的删除在分支副本中复活；在过滤器中加入 !source.removingNames.has(entry.name) 后 target.read 为 undefined，整套测试仍 75/75 通过。
修复后请补测试：仿照现有并发测试用 fs.unlink 门控把 store.remove('img.png') 停在半空，在其悬停期间启动 target.copyFrom(source)，释放后断言 remove 返回 true 且 target.read('img.png') 为 undefined；移除该过滤条件后测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…nts-root-env Resolve packages/core/src/agents/team/teamHelpers.test.ts: main renamed the vi.mock importOriginal binding from `actual` to `original`, so take main's declaration; this branch only reflowed the old name's type alias.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- removingNames guard has no distinguishing test (the concurrency test pauses remove before the unlinks, so the fallback occupancy probe alone forces the rename) — already recorded in the round-7 review (rv:5053067314) deferred list
- orphan reaper awaits deleteSessionAttachments unguarded after a successful persisted removal — already reported as R3-3 (comment 3869214414); re-derived this round at Critical severity on the same consequence chain R3-3's body already carri…
Not reviewed: build-and-test — Agent 7's run ended before its test phase: install + builds consumed the budget before any suite ran (packages/cli build timed out on infrastructure — the orphaned completion shows tsc compiled the PR clean); targeted suites ran green under review agents (sessionAttachments 75/75; env/fast-path/guard 63/63), but the dimension's own one-build-one-test never completed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
docs/users/qwen-serve.md:721 — [probe] Critical [fails-closed] [new-surface] R7-2: one-way-migration bullet wider than the code — a stat-denied configured root rejects every remove even when the only copy sits in the healthy writable fallba…docs/users/qwen-serve.md:721 — [probe] Critical [fails-closed] [new-surface] one-way-migration bullet's write arm omits that putAttachment stats the fallback root on every upload — a stat-denied legacy fallback dir rejects all new uploads a…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Agent 7's run ended before its test phase: install + builds consumed the budget before any suite ran (packages/cli build timed out on infrastructure — the orphaned completion shows tsc compiled the PR clean); targeted suites ran green under review agents (sessionAttachments 75/75; env/fast-path/guard 63/63), but the dimension's own one-build-one-test never completed。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated; never runs on PR pushes) and its serve-daemon suite did not run locally。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| entry.isFile() && | ||
| !source.pendingNames.has(entry.name) && | ||
| !copiedNames.has(entry.name), |
There was a problem hiding this comment.
[Critical] R7-1: [certifies-falsely] copyFrom's entry filter checks source.pendingNames and copiedNames but never consults source.removingNames — the set this diff introduces. A remove() already in flight when a branch copy starts adds the name to removingNames synchronously before its first await, then crosses several awaited fs steps (peekDirectory, two hasAttachment stats, up to two unlinks); a copyFrom started in that window passes the entry checks (remove does not set copying, and copyFrom drains pending uploads but not in-flight removes), lists the doomed name via readdir before the unlink lands, and copies it into the branch. Neither the bridge nor the HTTP layer serializes the two requests — DELETE attachment and POST branch both run under archiveCoordinator.runSharedMany, which permits concurrent holders of the same sessionId — so a completed deletion is silently resurrected in the branched session: the API certifies removed=true while bytes it no longer serves stay readable through the branch.
Witness:
gated probe, unmodified code — remove('notes.txt') parked at a gated fs.unlink, copyFrom run while parked, then released:
PROBE-R71 removed=true | target.read(notes.txt): PRESENT bytes="doomed bytes" | source.read(notes.txt): undefined
fix-validation arm — filter += !source.removingNames.has(entry.name):
target.read(notes.txt): undefined | source.read(notes.txt): undefined, suite still 75/75
| entry.isFile() && | |
| !source.pendingNames.has(entry.name) && | |
| !copiedNames.has(entry.name), | |
| entry.isFile() && | |
| !source.pendingNames.has(entry.name) && | |
| !source.removingNames.has(entry.name) && | |
| !copiedNames.has(entry.name), |
When this lands, add a test to src/sessionAttachments.test.ts: pause store.remove('img.png') mid-flight with an fs.unlink gate (as the existing concurrency test gates the primary stat), start target.copyFrom(source) while it is suspended, release, and assert remove resolves true AND target.read('img.png') resolves undefined — removing the new filter term must turn that test red.
中文说明
[Critical] R7-1：copyFrom 的条目过滤器检查 source.pendingNames 和 copiedNames，却从不查询本 diff 新引入的 source.removingNames。当分支复制开始时已有一个 remove() 在进行中：它在第一个 await 之前同步把名字加入 removingNames，随后跨越多个 await 的 fs 步骤（peekDirectory、两次 hasAttachment stat、至多两次 unlink）；在这个窗口内启动的 copyFrom 能通过入口检查（remove 不设置 copying，且 copyFrom 只排空待上传项、不等待进行中的删除），会在 unlink 落地之前通过 readdir 列出这个即将被删除的文件并复制进分支。bridge 与 HTTP 层都没有把两个请求串行化——DELETE 附件与 POST 分支都运行在 archiveCoordinator.runSharedMany 之下，允许同一 sessionId 的并发持有者——因此一个已完成的删除会在分支会话中被悄悄复活：API 已认证 removed=true、源侧不再提供该字节，分支中却仍能读到。
实测（受控探针，未改动代码）：把 remove('notes.txt') 停在受门控的 fs.unlink 处，在其悬停期间执行 copyFrom，释放后——removed=true 且 target.read(notes.txt) 仍能读到被判删的字节（源侧已为 undefined）；在过滤器中加入 !source.removingNames.has(entry.name) 后 target.read 为 undefined，整套测试仍 75/75 通过。
修复后请补测试：仿照现有并发测试用 fs.unlink 门控把 store.remove('img.png') 停在半空，在其悬停期间启动 target.copyFrom(source)，释放后断言 remove 返回 true 且 target.read('img.png') 为 undefined——移除新增过滤条件后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
4 participants
What this PR does
Adds a
QWEN_SERVE_SESSION_ATTACHMENTS_ROOTenv var forqwen serveso operators can store session attachments (Web Shell file/image uploads viaPOST /session/:id/attachments) under a directory of their choosing instead of the runtime temp dir — e.g. a dedicated volume that survives daemon restarts and is not subject to temp cleanup. When set, attachments live under<root>/<projectHash>/attachments/session-<sessionId>/, mirroring the default layout's per-workspace hash isolation so multiple workspaces sharing one root never collide.The switch is one-way-safe: reads and removes that miss the configured root fall back to the previous default directory, so attachments uploaded before the switch stay readable and removable; archiving a session clears both roots. A new upload never shadows a legacy fallback copy (the dedup loop treats a fallback-held name as occupied), and deleting both roots uses the same tombstone rename used for the primary, so a deletion racing a session restore cannot sweep up a successor directory. The
process.envguard registration, daemon config table, and user docs are updated alongside.Why it's needed
Session attachments currently live only under
~/.qwen/tmp/<projectHash>/attachments, which is tied to the runtime base dir and offers no way to relocate them. Users who want attachments persisted on a dedicated volume (or outside the home directory entirely) have no supported knob. This PR adds that knob with a safe migration story for existing attachments.Reviewer Test Plan
How to verify
cd packages/acp-bridge && npx vitest run src/sessionAttachments.test.ts src/bridge.test.ts— all pass (60 + 783).cd packages/cli && npx vitest run src/serve/session-attachments-root.test.ts src/serve/process-env-guard.test.ts— all pass (9 + 3).QWEN_SERVE_SESSION_ATTACHMENTS_ROOT=/some/dir, startqwen serve, upload a file through the Web Shell, and confirm the bytes land under/some/dir/<projectHash>/attachments/session-<sessionId>/. Then remove the env var, restart, and confirm the same attachment is still readable (fallback path), and that archiving the session removes both directories.Evidence (Before & After)
N/A — configuration and storage-layout change, no UI.
Tested on
Environment (optional)
Unit tests via vitest; no sandbox needed.
Risk & Scope
statSyncso they are platform-independent.Linked Issues
N/A
中文说明
为
qwen serve新增QWEN_SERVE_SESSION_ATTACHMENTS_ROOT环境变量，允许把 Web Shell 上传的会话附件存储到自定义目录（如独立磁盘卷），默认仍使用运行时临时目录。配置后附件存放于<root>/<projectHash>/attachments/session-<sessionId>/，与默认布局一致保留按工作区 hash 的隔离。迁移安全：读取/删除在配置 root 未命中时回退到切换前的默认目录，旧附件仍可读可删；归档会清理两个目录；新上传不会遮蔽 fallback 中的旧附件；两个目录的删除都使用与主目录相同的 tombstone 改名机制，避免删除与会话恢复竞争时误删新目录。同步更新了 process.env guard 登记、daemon 配置文档和用户文档。