feat(web-shell): unify file uploads and references - #9477
Conversation
b20cf2a to
5bcd903
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Thanks for the PR! Template looks good ✓ Problem: This is a feature unification, not a bug fix — the motivation is documented as observed Web Shell behavior (images and files follow different drop paths; attachment preview depends on transient client state, so sent files can need a refresh before they become clickable). That is a real UX gap on an actively developed surface. One note: "Linked Issues" points to #9351, which is a merged PR for a different feature, not an issue describing this problem. Direction: Aligned. The v0.21.14 changelog shows sustained Web Shell investment (#9310, #9311, #9323, #9382, #9407), and a single reference-or-upload drop path backed by persistent attachments is a natural next step. No direct claude-code CHANGELOG counterpart — Web Shell is qwen-code's own surface, so the area-relevance signal stands on the repo's own changelog. Size: The change spans 6 packages (acp-bridge, cli, core, sdk-typescript, web-shell, webui), so the core-module size check applies: 4,502 production-logic lines (60 files) vs 4,787 test lines (28 files). The core-path footprint itself is small ( Approach: The end-to-end scope is mostly justified — one attachment model has to thread store → bridge → routes → SDK → daemon actions → UI → transcript replay consistently, and half-measures would leave the layers disagreeing. Two substantive questions before deeper review:
Risk: Stage 1e high-risk path match: Flagging the compatibility question for discussion, then moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 这是一个功能统一类 PR，不是 bug fix——动机是 Web Shell 中已观察到的行为（图片和文件走不同的拖入路径；附件预览依赖临时客户端状态，已发送文件有时需要刷新后才能点击）。这是一个活跃迭代界面上的真实 UX 缺口。一点说明："Linked Issues" 指向的 #9351 是一个已合并的其他功能 PR，并不是描述本问题的 issue。 方向： 对齐。v0.21.14 的 changelog 显示 Web Shell 在持续投入（#9310、#9311、#9323、#9382、#9407），统一的"引用或上传"拖入路径加持久化附件是自然的下一步。claude-code CHANGELOG 无直接对应——Web Shell 是 qwen-code 自有界面，方向信号来自本仓库自身的 changelog。 规模： 改动横跨 6 个包（acp-bridge、cli、core、sdk-typescript、web-shell、webui），因此适用核心模块规模检查：4,502 行生产逻辑（60 个文件）vs 4,787 行测试（28 个文件）。核心路径本身改动很小（ 方案： 端到端的范围大体合理——同一个附件模型必须贯穿 store → bridge → 路由 → SDK → daemon actions → UI → transcript 回放，半套方案会让各层互相矛盾。深入审查前有两个实质问题：
风险： Stage 1e 高风险路径命中： 先提出兼容性问题讨论，随后进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewThe new attachment layer itself is carefully built — this is not a drive-by rename. Filename handling is traversal-safe (basename normalization plus a strict validator, re-checked as Two things genuinely block my sign-off, both flagged in the gate comment and confirmed by the code: 1. Sessions created on shipped v0.21.14 lose their media references, silently. #9310 merged 2026-08-17 and is listed in the v0.21.14 release notes (published 2026-08-19), so transcripts written by released builds carry 2. SDK public API rename with no bridge. Non-blocking: the per-session 100 MiB and 256-item caps are removed by design (8 MiB per file remains, and the 256-block request fan-out cap is kept — good). Storage now grows until the session is deleted; that tradeoff is acknowledged in the PR body and is a maintainer's call, not a review blocker. Attachment flow (drop to replay)sequenceDiagram
participant P1 as User
participant P2 as Web Shell composer
participant P3 as Daemon attachment routes
participant P4 as Session attachment store
participant P5 as Transcript record
P1->>P2: drop files
P2->>P2: intent dialog, reference or upload
P2->>P3: POST session attachments, bytes plus name header
P3->>P4: putAttachment, dedupe and 8 MiB check
P4-->>P2: reference with attachmentId
P1->>P2: send prompt with references
P2->>P3: prompt carrying attachment references
P3->>P4: assertReferences, resolve to inline bytes
P3->>P5: record attachmentReferences in systemPayload
P5-->>P2: replay, branch, refresh rehydrate rows
Files changed (30 of 88 shown)
Testing evidenceThis is an unattended CI run — triage executed none of the PR's code; the evidence below is the PR's own CI, read through the API at the reviewed commit. The main unit suite and the web-shell visual capture were still running at review time (fetched once, not polled); the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: live drag-and-drop behavior in a real browser, attachment survival across an actual daemon restart, and replay of a genuine v0.21.14-era transcript — none of these is observable from the diff or from unit checks, and the author verified on macOS only. Sandboxed verification would settle this: 中文说明新的附件层本身写得相当仔细——这不是顺手改名。文件名处理防穿越（basename 归一化加严格校验，读取/删除/断言时再次校验 有两点让我无法签字，与门禁评论一致并已在代码中确认：
非阻断：单 session 100 MiB 与 256 项上限按设计移除（保留单文件 8 MiB 与 256 块请求扇出上限——好）。存储随 session 生命周期增长，正文已承认该权衡，属于维护者决策而非审查阻断项。 测试证据：本次为无人值守 CI 运行——triage 未执行 PR 的任何代码；以上证据为通过 API 读取的 PR 自身 CI 结果。主单测套件与 web-shell 视觉截取在审查时仍在运行（只取一次，不轮询）， 未验证：真实浏览器中的拖放行为、真实 daemon 重启后附件是否存活、真实 v0.21.14 时期 transcript 的回放——这些从 diff 和单测中都看不到，且作者只在 macOS 上验证过。沙箱验证可以定案： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — high-quality implementation of a feature I'd want shipped, but it silently abandons data written by the build released two days ago, and the size escalation caps this at a maintainer's decision rather than an auto-approval. Stepping back: my independent take on this problem was the same end-to-end model — one filename-backed attachment store threaded through bridge, routes, SDK, daemon actions, UI, and replay, because half-measures leave those layers disagreeing. Where I'd have diverged is compatibility: the replaced media model was merged as #9310 on 2026-08-17 and shipped in v0.21.14 this morning, so I'd have kept a fallback reader for Everything else earns real credit. The store is written like someone thought about abuse (traversal-safe names validated twice, CI at review time: Security Checks, Qwen Live Host CI, and SDK Java green; the main unit suite and the remaining web-shell runs still in flight (not polled; the finalize job will update the Stage 2 table). Green CI would not change this verdict on its own — the unit suite pins the new shape, not compatibility with shipped transcripts, and the live browser behavior rests on the author's macOS-only testing. ⏸️ Deferring to @wenshao — two things need a human call before this merges:
Author: if you go the compatibility route (or the maintainer confirms the intentional break and the description is updated), re-run 中文说明信心：3/5 —— 实现质量高、方向也是我希望能合入的功能，但它会静默丢弃两天前发布的版本写下的数据，且规模升级（escalation）决定了这必须由维护者拍板，而不是自动批准。 退一步看：我对这个问题的独立方案同样是端到端模型——一个以文件名为标识的附件存储，贯穿 bridge、路由、SDK、daemon actions、UI 和回放，因为半套方案会让这些层互相矛盾。我的分歧点在兼容性：被替换的 media 模型已作为 #9310 于 2026-08-17 合并并随 v0.21.14 发布，因此我会保留对 其余部分值得肯定。存储层写得很防恶意输入（文件名两次校验防穿越、目录 0o700、原子去重、记忆化读取、逐块降级），路由作用域和 SVG 处理谨慎，分支与删除生命周期接线完整，新增测试覆盖可观。六个月后这个统一模型比 media/attachment 双轨更容易维护——等兼容性问题解决后，我会感谢作者做了这件事。 审查时 CI：Security Checks、Qwen Live Host CI、SDK Java 通过；主单测套件与其余 web-shell 任务仍在运行（不轮询，finalize 任务会更新 Stage 2 表格）。即便 CI 全绿也不改变本结论——单测只钉住了新形状，钉不住与已发布 transcript 的兼容性；浏览器实际行为目前只有作者在 macOS 上的自测。 ⏸️ 转交 @wenshao —— 合并前需要人工决策两件事：
作者：如果选择兼容路线（或维护者确认有意断裂且描述已更新），re-run — Qwen Code · qwen3.8-max Reviewed at |
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. |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "session_attachments" |
features[] |
"session_media" |
— |
— Qwen Code · serve A/B
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9477 feat(web-shell): unify file uploads and references
Type: New Feature + Refactor (replaces session_media with session_attachments)
Change size: +6681/-2608 across 88 files
HEAD: 5bcd9032
Prior reviews: none (first review)
Findings Summary
- Critical/Major: 0
- Minor: 0
- Nit: 5 (all non-blocking)
Key Design Observations
A comprehensive, well-engineered rewrite of the attachment subsystem. The core design decisions are sound:
- Filename-as-
attachmentIdeliminates a separate metadata store. MIME type is derived server-side viamimeTypeForName, making references self-describing from the filename alone. - Persistent disk storage (
session-${encodeURIComponent(sessionId)}/) with session-lifecycle cleanup correctly solves the "attachments disappear after daemon restart" problem the PR targets. flag: 'wx'exclusive create for deduplication is the correct OS-level primitive; the retry loop correctly handles concurrent same-name writes without a lock.assertStoredvalidates size + MIME on dispatch — catches corruption (partial writes) and stale references before they reach the model.resolveContentmemo sharing — theMap<string, Promise<ContentBlock>>deduplication ensures each distinct attachment is read from disk once per prompt even if referenced by multiple messages in a batched replay.withAttachmentDegradationMarkerpartial-degradation — appends the unavailable marker to the last text block rather than wholesale-replacing all content; this preserves surviving blocks in mixed prompts.copyFromskipspendingNames— in-progress writes are not copied to branches; this is correct since the branch snapshot is taken at dispatch time.canPreviewAttachmenton HTML/Markdown only — the source/preview toggle is correctly gated to files where rendered output is meaningfully different from raw content.
Nit Findings
Nit-1: assertStored uses statSync (synchronous FS I/O)
sessionAttachments.ts — assertStored calls statSync(path.join(directory, name)) synchronously to validate that the file exists and its size matches the reference. This is called from assertReference/assertReferences, which are invoked in the prompt-dispatch hot path. In a multi-session server, synchronous stat calls block the event loop for the duration of each call. For typical prompt sizes (1–5 attachments, sub-millisecond stat on SSD) the impact is negligible, but an async fs.promises.stat alternative would be strictly better and is consistent with every other I/O in this class.
Nit-2: deduplicatedName retry loop has no iteration bound
sessionAttachments.ts — the for (;;) loop in putAttachment retries with incrementing suffix until it finds an unused name. If a session accumulates many same-named files (e.g., thousands of image.jpeg uploads), the loop could run for many iterations before finding a free slot. In practice the per-file 8 MiB limit and disk constraints cap the count, but an explicit MAX_DEDUP_ATTEMPTS guard (e.g., 9999) with a rejection error would make the failure mode explicit rather than relying on storage exhaustion.
Nit-3: ATTACHMENT_UNAVAILABLE_TEXT has four copies
The string '[Attachment is no longer available]' is duplicated across sessionAttachments.ts (SESSION_ATTACHMENT_UNAVAILABLE_TEXT), normalizer.ts (ATTACHMENT_UNAVAILABLE_TEXT), useQueuedPrompts.ts (MEDIA_UNAVAILABLE_PLACEHOLDER), and DaemonSessionClient.ts (inferred from comment). The existing comment in sessionAttachments.ts calls this out explicitly; a re-export from sessionAttachments.ts through bridgeTypes.ts would let all consumers import the single constant, making future wording changes one-line instead of four.
Nit-4: toDaemonPromptContent throws TypeError for binary files
promptContent.ts — the new guard if (file.text === undefined) throw new TypeError('File attachment content is unavailable') is intentional, but the error message doesn't name the file. If the calling path ever accidentally passes a binary file (no text), the TypeError surfaces to the user as a generic send failure with no actionable hint. Adding file.name to the message ('File attachment content is unavailable: ' + file.name) would aid diagnosis without changing the guard's behavior.
Nit-5: DaemonInputReference.metadata?: unknown is undocumented
sdk-typescript/src/daemon/ui/types.ts — a new optional metadata: unknown field is added to DaemonInputReference without a comment. This is presumably for storing attachment-related metadata on @-file tags (e.g. { attachmentId: '...' }) to enable the composerTag.ts preview routing. A one-line comment stating its purpose and expected shape would clarify intent for maintainers.
Additional Audit Coverage
Areas independently verified:
- Path traversal safety:
safeAttachmentNamecallspath.basename()before all path joins — directory-traversal sequences are stripped at the gate. Windows reserved names (con,prn,aux,nul,com[1–9],lpt[1–9]) are blocked. ✓ putAttachmentclose-during-write:close()zeroespendingItemsand resolves drain waiters;finallychecks!this.closedbefore decrementing — no counter underflow, no double-release. ✓copyFromvsclose()race:copyFromchecksthis.closedafter the drain-waitawait, so aclose()fired during the wait causescopyFromto throw rather than copy into a closed store. ✓ENOENTon copy target: if the target directory is removed between thereaddirandcopyFilecalls, the error handler re-stats the source; if the source is intact the error is rethrown (branch fails cleanly rather than silently). ✓assertReferencesduplicate-ID guard: theseenIdsSet blocks repeated references for the sameattachmentIdin one prompt, preventing unbounded heap growth from one stored blob being resolved N times. ✓resolveContentrejection eviction: failed attachment reads evict their entry frompendingByIdso a transient disk error doesn't permanently poison later lookups for the same ID. ✓mimeTypeForNameTypeScript override:.ts/.tsx/.mts/.ctsare mapped totext/plainrather thanvideo/mp2t; mirrors thenormalizeTextMediaTypebehaviour in the client and prevents TypeScript source files from being stored with a video MIME type. ✓sanitizeAttachmentNamesimplification: removedATTACHMENT_NAME_UNSAFE_RE(was replacing\s,;!?()[]{}with_) while adding path-separator stripping (/^.*[\\/]/). File names with spaces or punctuation now pass through unsanitized — this is correct for the new filename-as-ID design since the server validates names withsafeAttachmentName(Windows reserved chars, control chars, length). ✓useQueuedPromptsupload cleanup removal: the oldremoveUploadedMedia()cleanup on abort/failure paths is intentionally dropped; uploaded attachments are session-persistent rather than transient, so orphaned uploads simply persist until session deletion — consistent with removing the per-session limits. ✓- Mid-turn
session_attachmentscapability gate:useQueuedPromptscorrectly gates mid-turn image injection onsession_attachments(wassession_media) so old daemons without this capability don't receive attachment blocks they would silently drop. ✓ isSessionAttachmentReferencetype guard consistency: bothacp-bridge/sessionAttachments.tsandsdk-typescript/DaemonSessionClient.tsindependently implement this guard; the constraints are equivalent (type ∈ {image, resource}, non-emptyattachmentId, size ≥ 0, images require size > 0, image mimeType must start withimage/). ✓attachmentUriForNameencoding: now usesencodeURIComponent— spaces and non-ASCII in attachment names are safely encoded in@attachment:///tokens embedded in the prompt text. ✓Session.tssize < 0guard: resources allowsize === 0(empty file); only images requiresize > 0. The guardreference['size'] < 0 || (reference['type'] === 'image' && reference['size'] === 0)correctly models both constraints. ✓normalizer.tsuser.file.deltaoffline path: when a transcript record contains a resourceattachmentIdreference that can't be hydrated (offline projection), the new path correctly emits auser.file.deltaevent rather than a plain-text placeholder — the file chip is still rendered and can be clicked. Images without data still fall back to the unavailable text placeholder because they can't be displayed without inline bytes. ✓FileTypeIconcompleteness: covers JSON, code, archive, spreadsheet, presentation, image, audio, video, text extension families;.pdf,.doc,.docx,.rtfare mapped toFileTextIcon; unknown extensions fall back toFileIcon; MIME-type heuristics cover cases where the file has no extension. ✓
Final Verdict
✅ Approve. The session_attachments model is a substantial improvement over the session_media predecessor: persistent storage eliminates the refresh-to-preview issue, the unified drop-intent dialog solves the image-vs-file split, and the clean filename-as-ID design avoids a separate metadata store. The implementation handles the lifecycle edge cases (concurrent writes, close-during-write, branch copy, degraded prompts) correctly. All five findings are non-blocking Nits. Ready to merge.
This review was generated by QoderWork AI
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Review Report — PR #9477: feat(web-shell): unify file uploads and references
Target: QwenLM/qwen-code — codex/web-shell-drop-upload-or-reference → main
HEAD: 5bcd90324d (82 files, +6681/−2608)
Effort: high | 4 parallel subagents by module
🎉 Highlights
-
Thorough media → attachment migration: All references to
sessionMedia,mediaId,session_media,/media/routes have been fully replaced across all packages. Grep confirms zero stale references in the affected trees. The rename is consistent in types, runtime code, tests, and design docs. -
Exemplary path traversal defense:
safeAttachmentNamelayerspath.basename, backslash normalization,.trim(), Windows reserved name rejection, control character filtering, and a byte-length cap. Every entry point re-validatesname === attachmentId. Both upload and download routes set properContent-DispositionandX-Content-Type-Options: nosniffheaders. -
Well-designed race condition handling: The
pendingNamesmap protecting in-flight writes fromremove(), thependingItems/pendingDrainWaitersmechanism forcopyFrom()ordering, theflag: 'wx'exclusive-write retry loop with deduplication, and the memo eviction on transient read failures are all solid and thoroughly tested. -
Comprehensive test coverage: ~25+ tests for
SessionAttachmentStorecovering storage, resolution, deduplication, path traversal rejection, race conditions, memo eviction, degradation, and removal of per-session caps. Bridge and bridgeClient tests verify integration. Web shell tests cover drop intent flow, sidebar preview, file-type icons, keyboard accessibility, workspace file tags, and i18n. -
Clean ArtifactPanel preview hierarchy: The
AttachmentBlobPreview→TextAttachmentPreview/PdfAttachmentPreview/UnsupportedAttachmentPreviewdecomposition cleanly handles each file type. The source/preview toggle for HTML/Markdown attachments is well-implemented.
🔴 Critical — 1 finding
C1: Second drop while intent dialog is open silently discards first drop's files
packages/web-shell/client/components/ChatEditor.tsx:1803
handleUploadDrop calls setPendingDropFiles(files) unconditionally. If the intent dialog is already showing a previous drop, a second drop replaces the first drop's files. When the user clicks "Attach to message" or "Upload", only the second set is acted upon; the first set is silently lost.
Failure scenario: User drops a.txt → dialog opens. Before choosing, user drops b.txt on the same composer. setPendingDropFiles([b.txt]) replaces [a.txt]. User clicks "Attach to message" — only b.txt is attached; a.txt is gone with no warning.
Suggested fix:
if (pendingDropFiles !== null) {
event.preventDefault();
return;
}
setPendingDropFiles(files);🟡 Important — 4 findings
I1: sanitizeAttachmentName no longer strips leading dots
packages/web-shell/client/utils/imageIngestion.ts:182-188
The new sanitizer only strips path prefixes and control characters. Names like .env, .gitignore, .htaccess pass through unchanged. If uploaded to a workspace, they create hidden files that ls won't show by default.
Suggested fix: Add name.replace(/^\./, '_') or document that dot-prefixed names are intentionally preserved.
I2: Image size check changed from aggregate budget to per-file
packages/web-shell/client/utils/imageIngestion.ts:336-342
The old code computed remaining budget by subtracting already-ingested image data lengths. The new code checks each file's raw size independently against MAX_IMAGE_ATTACHMENT_DATA_BYTES (8 MB). Ten 7 MB images all pass (each < 8 MB), totaling ~93 MB base64 payload.
Suggested fix: If the aggregate budget was intentional, restore the remaining-budget computation. If the relaxation is deliberate, document it.
I3: Orphaned attachments accumulate with no cleanup path
packages/webui/src/daemon/session/actions.ts:293-406
The old media code called session.removeMedia() on prompt cancellation, pending prompt removal, and partial batch failure. The new code removes all three cleanup calls. With per-session limits and daemon-wide caps both removed, the only cleanup is permanent session deletion.
Failure scenario: A long-running session where the user repeatedly attaches files and the prompt is rejected. Each attempt uploads 8 MiB-max attachments that are never reclaimed.
Suggested fix: Consider (a) a per-session cumulative cap, or (b) a background sweep on session detach that removes unreferenced attachments.
I4: readFileTransfer accepts and ignores lifecycle parameter
packages/web-shell/client/utils/imageIngestion.ts:381-401
readFileTransfer accepts lifecycle: ReaderLifecycle but never calls lifecycle.onReaderCreated or lifecycle.onReaderSettled. The caller's reader-tracking infrastructure has no visibility into file transfers. Currently harmless (synchronous), but the misleading API invites bugs if file reading becomes async.
Suggested fix: Remove the lifecycle parameter or document that it's intentionally unused.
🟢 Nits — 3 findings
N1: referenceDroppedFiles depends on entire core object
packages/web-shell/client/components/ChatEditor.tsx:1813-1817
useCallback depends on core (new object identity every render), so the callback is recreated every render. Depend on core.ingestFiles instead.
N2: Drop intent dialog mixes Tailwind with CSS modules
packages/web-shell/client/components/ChatEditor.tsx:3527
Inline Tailwind classes (max-h-32 overflow-auto rounded-lg...) alongside CSS modules (styles.xxx) in the same component.
N3: Stale "media" in test descriptions
packages/core/src/services/chatRecordingService.test.ts:353,382 and packages/webui/src/daemon/session/actions.test.ts:1709 — test bodies correctly use "attachment" but descriptions still say "media".
💡 Suggestion — 1 finding
S1: O(n²) indexOf in file-to-reference mapping
packages/webui/src/daemon/session/actions.ts:366-371
files.map uses uploadableFiles.indexOf(file) — O(n) per file, O(n²) total. Build a Map<File, Reference> instead for O(n) and more robust reference identity.
Verdict
🔄 Request Changes — one verified Critical (C1: double-drop silently discards files) must be fixed before merge. The four Important findings are worth addressing but not blocking.
中文版本
审查报告 — PR #9477：feat(web-shell): unify file uploads and references
目标： QwenLM/qwen-code — codex/web-shell-drop-upload-or-reference → main
HEAD： 5bcd90324d（82 文件，+6681/−2608）
工作量： 高 | 4 个子代理按模块并行
🎉 做得好
- 彻底的 media → attachment 迁移：所有
sessionMedia、mediaId、session_media、/media/路由引用已全部替换，grep 确认零残留。 - 路径穿越防御纵深：
safeAttachmentName多层防护（path.basename、反斜杠归一化、Windows 保留名拒绝、控制字符过滤、字节长度上限），每个入口重复校验name === attachmentId。 - 竞态处理设计良好：
pendingNames保护写中文件、flag: 'wx'独占写重试、memo 瞬态失败驱逐，均有测试覆盖。 - 测试覆盖全面：
SessionAttachmentStore25+ 测试，bridge/bridgeClient 集成测试，Web Shell 拖放意图弹窗、侧边栏预览、文件类型图标、键盘可访问性、i18n 均有覆盖。 - ArtifactPanel 预览层次清晰：按文件类型分解为 Text/Pdf/Unsupported 预览组件，HTML/Markdown 源/预览切换实现良好。
🔴 严重 — 1 条
C1：意图弹窗打开时再次拖放会静默丢弃第一次的文件
ChatEditor.tsx:1803 — handleUploadDrop 无条件调用 setPendingDropFiles(files)，第二次拖放覆盖第一次的文件，用户操作后只有第二次的文件被处理，第一次的文件静默丢失。
修复建议：
if (pendingDropFiles !== null) {
event.preventDefault();
return;
}
setPendingDropFiles(files);🟡 重要 — 4 条
I1： sanitizeAttachmentName 不再去除前导点 — .env、.gitignore 等文件名原样通过，上传到工作区后成为隐藏文件。
I2： 图片大小检查从聚合预算改为单文件 — 旧代码按已附加图片总大小计算剩余预算，新代码每张图片独立检查 8 MB。10 张 7 MB 图片都通过，总 base64 约 93 MB。
I3： 孤儿附件无清理路径 — 旧代码在 prompt 取消、待发送移除、部分批量失败时调用 removeMedia，新代码移除了所有这些清理调用。长运行会话中反复附加文件但 prompt 被拒绝时，附件持续累积。
I4： readFileTransfer 接受并忽略 lifecycle 参数 — 调用方的 reader 追踪机制对文件传输无可见性。目前无害（同步），但 API 误导性强。
🟢 小问题 — 3 条
N1： referenceDroppedFiles 依赖整个 core 对象，每次渲染重建。
N2： 拖放意图弹窗混用 Tailwind 和 CSS modules。
N3： 测试描述中残留 "media" 字样（测试体已正确改为 "attachment"）。
💡 建议 — 1 条
S1： files.map 中使用 uploadableFiles.indexOf(file) 为 O(n²)，建议改用 Map<File, Reference>。
结论
🔄 请求修改 — 一个已验证的严重问题（C1：重复拖放静默丢弃文件）必须在合并前修复。4 条重要问题建议处理但不阻断。
|
已在
以下 review 项按既定产品设计保留：前导点文件名保持原名；仅保留单文件 8 MiB 限制，不恢复 session 聚合上限；附件只在删除 session 时清理；弹窗继续使用 Web Shell 已采用的 Tailwind/shadcn 样式。
本地验证通过： |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Re-review Report — PR #9477 (round 2)
HEAD: daca1eed72 (91 files, +6774/−2658)
Previous HEAD: 5bcd90324d (82 files, +6681/−2608)
🎉 修复确认 / Fixes Confirmed
上一轮审查中的以下问题已在本次 head 中修复：
| 编号 | 状态 | 说明 |
|---|---|---|
| 🔴 C1 | ✅ 已修复 | ChatEditor.tsx:1784 添加了 if (pendingDropFiles !== null) { event.preventDefault(); event.stopPropagation(); return; } 守卫，第二次 drop 不再覆盖第一次的文件 |
| 🟡 I4 | ✅ 已修复 | readFileTransfer 的 lifecycle 参数已移除，改为简单的 options: { maxBytes?: number } |
| 🟢 N1 | ✅ 已修复 | referenceDroppedFiles 依赖收窄为 [ingestFiles, pendingDropFiles]，ingestFiles 是稳定的 useCallback |
| 💡 S1 | ✅ 已修复 | 文件去重改用 Set + taken.add(name)，O(1) 查找 |
| 🟡 I2 | ✅ 已记录 | 设计文档 session-attachment-references.md 明确说明 "Sessions have no cumulative attachment size or count limit"，per-file 8 MiB 限制保留 |
🎉 本轮亮点 / New Highlights
- 完整的 media → attachment 迁移: 所有
sessionMedia、mediaId、/media/路由、DAEMON_MEDIA_REFERENCES_META_KEY已从全部 91 个变更文件中彻底清除，grep 确认零残留。类型、运行时代码、测试、设计文档一致。 safeAttachmentName防御纵深:path.basename+ 反斜杠归一化 + Windows 保留名拒绝 + 控制字符过滤 +<>:"|?*特殊字符拒绝 + 字节长度上限。上传和下载路由均做name === attachmentId往返校验。- 并发安全:
pendingNamesmap 保护写入中的文件名、flag: 'wx'排他写入、pendingItems/pendingDrainWaiters排序机制、memo 瞬态读取失败驱逐——全部经过测试覆盖。
🟡 Important — 1 finding
I1（遗留）— sanitizeAttachmentName 不过滤前导点 / Leading dots not stripped
packages/web-shell/client/utils/imageIngestion.ts:182-188
该函数仅剥离路径前缀和控制字符。.env、.gitignore、.htaccess 等名称原样通过。若选择"上传到工作区"，会在工作区中创建 ls 默认不显示的隐藏文件。
sanitizeAttachmentName only strips path prefixes and control characters. Names like .env, .gitignore, .htaccess pass through unchanged. If the user chooses "Upload to workspace", hidden files are created that ls won't show by default.
此外，dedupeAttachmentName（line 195-197）对前导点文件的去重后缀位置不一致：.env 变为 .env (1) 而非 .env (1)（dot > 0 对 index 0 为 false，导致 stem 包含点前缀）。虽然不会崩溃，但与 app.log → app (1).log 的行为不一致。
Additionally, dedupeAttachmentName (line 195-197) inserts the dedup suffix inconsistently for dotfiles: .env becomes .env (1) because dot > 0 is false at index 0, so the stem includes the dot prefix. This differs from the app.log → app (1).log pattern.
建议修复 / Suggested fix:
// sanitizeAttachmentName: strip leading dots after control char removal
export function sanitizeAttachmentName(name: string): string {
const cleaned = name
.replace(/^.*[\\/]/, '')
.trim()
.replace(CONTROL_CHAR_RE, '')
.replace(/^\.+/, '_'); // leading dots → underscore
return cleaned || 'attachment';
}
// dedupeAttachmentName: handle dotfile stem correctly
const dot = name.lastIndexOf('.');
const stem = dot > 0 || (dot === 0 && name.length > 1) ? name.slice(0, dot) : name;
const extension = dot > 0 || (dot === 0 && name.length > 1) ? name.slice(dot) : '';🟡 Important — 1 new finding
I2 — Session 记录先于附件文件删除，清理失败时文件泄漏 / Session record deleted before attachment files; orphaned files on cleanup failure
packages/cli/src/serve/server/session-archive.ts:404-431
deleteDaemonSessions 中，deletePersistedSessionWithLease（line 405-408）先删除 session 记录，然后才调用 bridge.deleteSessionAttachments（line 418）。若 session 记录删除成功但附件删除失败（磁盘 I/O 错误、权限问题），session 记录已不存在（无法重试），但附件目录 <temp>/attachments/session-<id>/ 永久残留。
deletePersistedSessionWithLease runs first (line 405-408), then bridge.deleteSessionAttachments (line 418). If the session record deletion succeeds but attachment deletion fails, the session record is gone (cannot retry), but the attachment directory remains on disk permanently with no way to discover or clean it up.
建议修复 / Suggested fix: 考虑调换顺序（先删附件再删记录），或在错误信息中记录附件目录路径以便运维清理。
Consider reversing the order (delete attachments first, then session record), or record the attachment directory path in the error so an operator can clean up.
🟢 Nit — 3 findings
N1 — 测试描述中残留 "media" / Stale "media" in test descriptions
packages/web-shell/client/hooks/useQueuedPrompts.midTurnReconcile.test.tsx 约 10 处测试描述仍引用 "media"（lines 1999, 2027, 2101, 2159, 2198, 2246, 2302, 2396, 2498）。内部函数名 contentHasDegradedMedia、contentHasUnhydratedMedia、参数 canInjectMidTurnMedia 也保留 "media" 命名。packages/cli/src/serve/server.test.ts:9836 的测试描述 'reports the media route 8 MiB body limit accurately' 实际测试 attachment 路由。
About 10 test descriptions still reference "media". Internal function names (contentHasDegradedMedia, contentHasUnhydratedMedia, canInjectMidTurnMedia) also retain "media" naming.
N2 — MEDIA_CONTENT_MAX_BLOCKS 常量名遗留 / Legacy constant name
packages/cli/src/serve/routes/session.ts:220 — 常量仍名为 MEDIA_CONTENT_MAX_BLOCKS，注释仍说 "Media blocks are resolved into inline bytes at dispatch"。PR 其他位置已全部重命名为 attachment。
The constant and its comment still use "media" naming while the rest of the PR renamed everything to "attachment."
N3 — Drop intent dialog 混用 Tailwind 和 CSS modules / Mixed Tailwind and CSS modules
packages/web-shell/client/components/ChatEditor.tsx:3534-3541 — drop intent dialog 文件列表使用 Tailwind 工具类（max-h-32 overflow-auto rounded-lg...），而 ChatEditor 其他部分使用 CSS modules（styles.*）。
The drop intent dialog file list uses Tailwind utility classes alongside CSS modules, inconsistent with the rest of ChatEditor.
结论 / Verdict
💬 评论 / Comment — 上一轮的 🔴 Critical（C1 第二次 drop 覆盖）已修复。剩余 2 条 Important 均为非阻塞问题：I1（前导点过滤）是上一轮遗留的安全卫生问题，I2（删除顺序）是新增的运维卫生问题。3 条 Nit 为命名一致性和样式混合。整体实现质量高，media → attachment 迁移彻底，安全防御纵深完善，并发处理稳健。
The previous Critical (C1) is fixed. The remaining 2 Important findings are non-blocking hygiene issues: I1 (leading dot stripping) carried over from round 1, I2 (deletion ordering) is new. 3 Nits are naming consistency and style mixing. Overall implementation quality is high — the migration is thorough, security defenses are solid, and concurrency handling is robust.
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.
Unresolved, please confirm:
- [Critical] Triage stage-2 blocker (legacy compatibility): transcript records carrying
systemPayload.mediaReferences/mediaIdwritten by the shipped v0.21.14 build are no longer projected on replay/resume/branch — the drop mechanism is still present… - [Critical] Triage stage-2 blocker (SDK rename):
uploadMedia/removeMedia/DaemonSessionMediaReferenceand the daemon client methods are renamed wholesale without a bridge; whether a published@qwen-code/sdk-typescriptversion exposed the media A… - [Critical] Triage stage-3 deferral: the two items above (plus maintainer awareness of the ~4,500-production-line, 6-package scope) are explicitly awaiting @wenshao's human decision; the author has since corrected the PR description as stage 3 outlined…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": did not trace the webui settle-path consumer of consumeSidechannelMidTurnInjected end-to-end to confirm the exact UI symptom (stale queued chip vs delayed cal….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 3 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：did not trace the webui settle-path consumer of consumeSidechannelMidTurnInjected end-to-end to confirm the exact UI symptom (stale queued chip vs delayed cal…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…upload-or-reference # Conflicts: # packages/sdk-typescript/scripts/build.js
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review (Round 3 — AI Generated)
HEAD: 1726d5427f | 基于前两轮 review 的补充，聚焦一条仍未解决的清理顺序问题。
This review was generated by QoderWork AI
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM (AI Generated)
经三轮审查及作者反馈确认：
- 所有 Critical/Major findings 均已解决或经作者合理说明后关闭
session_media→session_attachments迁移完整彻底，无残留引用- 并发安全（
pendingNames、flag: 'wx'、drain waiter）、路径遍历防御、幂等重试均设计正确且有测试覆盖 - 遗留 Nit（
statSync/ leading-dot 文件名）可作为后续 follow-up 处理，不阻塞合并
✅ Approve
This review was generated by QoderWork AI
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. CI is green, all Critical/Suggestion threads from qwen-code-ci-bot are resolved, and the PR body follows the template with a solid test plan and evidence.
One non-blocking follow-up (still open from round-3 review): in removePersistedSession, the DB record is deleted before deleteSessionAttachments. If attachment deletion throws (EACCES/ENOSPC/transient IO), the record is already gone, so a retry hits "session not found" and the attachment directory leaks permanently. Consider deleting attachments first so a failed delete can be re-run (idempotent). Edge-case, not a merge blocker.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Independent review — verified against HEAD 1726d542
I reviewed this diff independently (security-sensitive storage change crossing four packages): full read of sessionAttachments.ts, the daemon attachment routes, the SDK/UI reference lifecycle, and a byte-level comparison against the deleted sessionMedia design. Verdict: one Major finding on a failure path (inline), one Suggestion (inline), everything else verified clean. Not approving yet — the orphan-accumulation path deserves a response, either a fix or a justification.
What I verified and found correct
- Path traversal closed.
safeAttachmentName(basename + rejects./../trailing dot/Windows reserved/<>:"|?*/control chars/255-byte cap) is enforced on every read/remove/assert path viasafeAttachmentName(id) === id; session directories useencodeURIComponent(sessionId). No client-supplied string reaches a path join un-sanitized. - Dedupe race closed. Writes use
flag: 'wx'(O_EXCL) with EEXIST→suffix retry;pendingNamesblocks concurrentremove, andcopyFromdrains pending writes correctly. - 8 MiB enforced on decoded bytes at the store, raw bytes at the HTTP layer. (Note:
limit: '8mb'is decimal 8,000,000 — slightly stricter than the 8 MiB the client allows; this is a pre-existing line, not new.) - Session deletion cleanup is runtime-correct. Batch deletion resolves the owner runtime per session, refuses cross-workspace ambiguity with 409 rather than falling back to primary, and the archive coordinator's exclusive lock serializes against uploads.
- Route ownership. All three attachment routes resolve through the session-runtime gate: ambiguous → 500, unavailable → 503, not-found → 404, never a silent primary fallback; storage root is injected per bridge at all three creation points.
- Rename is complete. No functional
mediaId/session_mediaconsumers remain (the only hits are unrelated external-channel adapters); old transcripts'mediaReferencesare silently skipped by replay/resume — loads safely, matches the stated no-compat-layer policy. - Degradation paths. Hydration 404/410 → placeholder, transient errors retry later; bridge dispatches degrade per-block with markers instead of rejecting the message; preview failures toast.
assertStoredcross-checks size/mimeType/type against disk, so forged references don't pass. - Preview XSS. GET routes send
Content-Disposition: attachment+nosniff; SVG/HTML previews use source view or a sandboxed iframe withoutallow-same-origin.
Why I'm holding approval
The Major inline is the only substantive gap: the PR's two headline design moves — persistence across restarts and removal of all aggregate caps — turn a pre-existing failure-path leak shape into unbounded, invisible disk growth. It is cheap to close (removeAttachment already exists in the session actions API), and the alternative is that every aborted/rejected mid-turn upload with a restored draft plants up to 8 MiB that nothing references and only session deletion removes.
| throw new TypeError('Attachment name and Content-Type do not match'); | ||
| } | ||
| if (this.closed) throw new Error('Session attachment store is closed'); | ||
| if (this.copying) throw new Error('Session attachments are being copied'); |
There was a problem hiding this comment.
[Suggestion] During a branch copyFrom, putAttachment throws a plain Error, and the upload route only maps RangeError → 413 and TypeError → 400, so this lands in the generic handler as a 500. It is a predictable transient state (a branch copy in flight), and a 500 invites clients to treat it as a server fault rather than retry-after-a-moment. Consider a dedicated error type mapped to 409 (or 503). Non-blocking.
There was a problem hiding this comment.
本轮不修改。该状态只存在于很短的分支复制窗口，当前 500 不会造成数据损坏，客户端重试即可；要改成 409/503 需要新增跨 bridge、route 和 SDK 的公共错误映射。PR 已经过多轮 review，按项目规则非阻断建议留到后续，避免继续扩大本 PR。
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.
Unresolved, please confirm:
- [Critical] Round-1 body blocker (SDK rename): uploadMedia/removeMedia/DaemonSessionMediaReference and the daemon client methods are renamed wholesale without a compat bridge at this head; whether a published @qwen-code/sdk-typescript version exposed t…
- [Critical] Round-1 body blocker (stage-3 deferral): the legacy-compatibility and SDK-rename items (plus maintainer awareness of the ~4,500-production-line, 6-package scope) remain explicitly awaiting @wenshao's human decision — this review cannot rule…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 16: none — no check was cut short.; "agent reverse-audit (round 2)": composer reachability check for attachments + / -prefixed text (App.tsx/ChatPane.tsx call sites not traced to the submit gating — finding filed at Confidence l….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.test.ts:13495 — [review] Deferred (code-age): 'only when' half of the attachment-deletion test unasserted (F7)packages/web-shell/client/App.test.tsx:353 — [review] Deferred (code-age): mock onAttachmentPreview type omits workspacePath — TS2353 (F8)packages/webui/src/daemon/session/actions.ts:1643 — [review] Deferred (code-age): standalone uploadAttachment 'image/*' fallback → invalid name 'image.*' (F9)packages/acp-bridge/src/bridge.ts:9193 — [review] Deferred (code-age): concurrent upload during branch copy 500s (F11)packages/acp-bridge/src/bridge.test.ts:13514 — [review] Deferred (code-age): force-kill test exercises an ephemeral store — retention semantics unpinned (G9)packages/acp-bridge/src/bridge.test.ts:14768 — [review] Deferred (code-age): production branch-copy combination (root set + restored live) untested (G10)packages/acp-bridge/src/bridge.ts:9192 — [review] Deferred (code-age): concurrent side-task branch copy swallowed → branch committed empty (G11)packages/web-shell/client/App.tsx:3451 — [review] Deferred (code-age): preview-only attachment tabs keyed by name only — same-name collision (G12)packages/web-shell/client/components/ChatEditor.module.css:2063 — [review] Deferred (code-age): hybrid touch devices need two taps on the file-chip remove button (G15)packages/acp-bridge/src/sessionAttachments.ts:155 — [review] Deferred (code-age): text-MIME shortcut bypasses the NUL/UTF-8 sniff — UTF-16 mojibake (H1)packages/web-shell/client/components/ChatEditor.test.tsx:1997 — [review] Deferred (code-age): mixed-batch upload-lane assertion deleted (H3)
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 2 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 16：none — no check was cut short.；"agent reverse-audit (round 2)"：composer reachability check for attachments + / -prefixed text (App.tsx/ChatPane.tsx call sites not traced to the submit gating — finding filed at Confidence l…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| '/session/:id/attachments', | ||
| mutate(), | ||
| express.raw({ type: 'image/*', limit: '8mb' }), | ||
| express.raw({ type: '*/*', limit: '8mb' }), |
There was a problem hiding this comment.
[Critical] R2-2: All aggregate attachment-storage bounds were deleted with no replacement. The daemon-wide MAX_RETAINED_SESSION_MEDIA_BYTES (512 MiB) and the per-session 100 MiB / 256-item caps are gone; SessionAttachmentStore enforces only the 8 MiB per-item cap, and attachments persist under <project temp>/attachments/session-<id> until session deletion.
Any authorized client bound to a session can loop 8 MiB uploads with no client-visible stop signal; a buggy or hostile client exhausts the daemon host's disk, where previously it stopped at 100 MB/256 items with a RangeError.
Witness: not run — executing a disk-exhaustion upload loop against the daemon host is destructive; mechanism verified by code inspection (putAttachment enforces the per-item cap only) and diff inspection (all three aggregate bounds deleted).
Note: the design doc ('Sessions have no cumulative attachment size or count limit'), the Risk & Scope section, and your round-1 reply accept this tradeoff explicitly — so this is an explicit-acceptance decision a maintainer should own, not an oversight to rubber-stamp. Suggested fix: reintroduce an aggregate bound (per-session total bytes or count in putAttachment, mapped to 4xx), or have a maintainer explicitly confirm the unbounded-growth tradeoff on the record.
中文说明
所有附件存储的总量上限都被删除且没有替代。daemon 级 MAX_RETAINED_SESSION_MEDIA_BYTES（512 MiB）与单 session 100 MiB / 256 项上限均已移除；SessionAttachmentStore 只保留单文件 8 MiB 上限，附件持久保存在 <project temp>/attachments/session-<id> 下直到 session 被删除。
任何绑定到 session 的授权客户端都可以无限制地循环上传 8 MiB 附件且没有任何可见的停止信号；有 bug 或恶意的客户端可以耗尽 daemon 宿主磁盘——而之前会在 100 MB/256 项时以 RangeError 停止。
证据：未运行——对 daemon 宿主执行磁盘耗尽上传循环是破坏性的；机制通过代码检查（putAttachment 只有单项上限）与 diff 检查（三个总量上限全部被删）确认。
注意：设计文档（"Sessions have no cumulative attachment size or count limit"）、Risk & Scope 以及你在第一轮回复中已明确接受该取舍——因此这是一个应由 maintainer 明确认领的决策，而不是可以默认放过的疏漏。建议修复：恢复总量上限（在 putAttachment 中按 session 总字节或数量检查并映射为 4xx），或由 maintainer 在 PR 上明确确认接受无界增长。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
按已确认的产品要求，本轮不恢复累计容量或数量上限。附件只保留单文件 8 MiB 限制，并随 session 生命周期持久化、删除 session 时清理；100 MiB/256 项以及 daemon 全局累计上限是有意移除的取舍。此项需要 maintainer 认领该产品决策，因此保留线程不 resolve。
| meta: UpdateMetaOptions, | ||
| ): Iterable<TranscriptReplayEmission> { | ||
| const references = payload?.['mediaReferences']; | ||
| const references = payload?.['attachmentReferences']; |
There was a problem hiding this comment.
[Critical] R2-3: The mediaReferences→attachmentReferences rename has no read-side fallback, so transcripts persisted by already-shipped builds (v0.21.14 / recent nightlies, where #9310 recorded systemPayload.mediaReferences with mediaId) lose their attachment display on upgrade — in three consumers: (1) CLI resume — resumeHistoryUtils.ts:445 reads only payload?.attachmentReferences; (2) replay projection — here (projectUserAttachmentReferences); (3) SDK transcript projection — normalizer.ts's isAttachmentReferenceContent now requires attachmentId, so old {type:'image', mediaId} user_message_chunk records emit zero UI events. There is no recording-boundary rewrite (raw journal frames persist and replay verbatim).
Concrete outcome: an old image-only user prompt recorded with displayText:'' + mediaReferences silently disappears from restored history on resume (no text, no blocks → the new UserMessage skips the empty bubble); on replay it projects nothing; in SDK-projected transcripts the bubble vanishes where pre-PR builds showed the '[Attached media is no longer available]' placeholder. An old media-only mid-turn message falls back to the raw internal prefix '[User message received during tool execution]: '. The old bytes were never recoverable across restarts anyway — what is lost is the placeholder/chip display that every other gone-attachment path preserves.
Probe (all three consumers + flip arm): legacy records → Resume: buildResumedHistoryItems returned []; mid-turn: restored text '\n[User message received during tool execution]: '; Replay: zero updates; SDK normalizer: old shape → zero events, while the same content with attachmentId returned the placeholder text event.
This re-asserts the round-1 legacy-compatibility blocker (review 4971964241) with the third consumer now traced. Suggested fix: in all three consumers, when attachmentReferences is absent, accept the legacy mediaReferences/mediaId shapes and project the degraded placeholder (SESSION_ATTACHMENT_UNAVAILABLE_TEXT), so pre-upgrade records keep at least the placeholder the pre-PR builds showed.
中文说明
mediaReferences→attachmentReferences 重命名没有读取端兜底，因此由已发布版本（v0.21.14 / 近期 nightly，#9310 记录的是带 mediaId 的 systemPayload.mediaReferences）持久化的转录在升级后会丢失附件展示——共三个消费者：(1) CLI resume——resumeHistoryUtils.ts:445 只读 payload?.attachmentReferences；(2) replay 投影——即此处（projectUserAttachmentReferences）；(3) SDK 转录投影——normalizer.ts 的 isAttachmentReferenceContent 现在要求 attachmentId，旧的 {type:'image', mediaId} user_message_chunk 记录会产出零个 UI 事件。录制边界没有任何改写（原始 journal 帧原样持久化并原样回放）。
具体结果：旧的纯图片用户消息（displayText:'' + mediaReferences）在 resume 时从恢复的历史中静默消失（无文本、无块 → 新的 UserMessage 跳过空气泡）；replay 时投影不出任何内容；SDK 投影转录中气泡直接消失——而升级前的构建会显示 '[Attached media is no longer available]' 占位文本。旧的纯 media mid-turn 消息则退化为内部原始前缀 '[User message received during tool execution]: '。旧字节本来就无法跨重启恢复——真正丢失的是其他所有附件缺失路径都会保留的占位/附件标签展示。
探针（三个消费者 + 翻转对照）：旧格式记录 → Resume：buildResumedHistoryItems 返回 []；mid-turn：恢复文本为 '\n[User message received during tool execution]: '；Replay：零更新；SDK normalizer：旧形状 → 零事件，而相同内容带 attachmentId 时返回占位文本事件。
本条以第三个消费者被追踪到的事实，再次提出第一轮的 legacy 兼容性 blocker（review 4971964241）。建议修复：在三个消费者中，当 attachmentReferences 缺失时接受旧的 mediaReferences/mediaId 形状并投影降级占位文本（SESSION_ATTACHMENT_UNAVAILABLE_TEXT），使升级前的记录至少保留升级前构建所展示的占位文本。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
按已确认的发布策略，本轮不增加 mediaReferences/mediaId 兼容层。daemon、Web Shell 和 SDK 同批发布，不存在新旧协议组件混用；旧 media 临时文件本来也会随升级重启失效，而 mediaId 方案本身也是本 PR 前序开发阶段引入、并非需要长期兼容的稳定附件格式。因此保持 attachmentReferences 单一路径，线程留给 maintainer 确认。
| const maxBytes = lifecycle.maxBytes ?? MAX_IMAGE_ATTACHMENT_DATA_BYTES; | ||
| for (const candidate of imageCandidates) { | ||
| const candidateBytes = Math.ceil(candidate.file.size / 3) * 4; | ||
| if (estimatedEncodedBytes + candidateBytes > maxEncodedBytes) { | ||
| if (candidate.file.size > maxBytes) { |
There was a problem hiding this comment.
[Critical] R1-6 (re-check: still stands): the cumulative encoded-bytes image budget remains replaced by a per-file raw-size check with no batch or cross-batch accounting, while every accepted image is still eagerly read into base64 and retained in React state before upload. The tab-memory arm of round-1's R1-6 is unchanged in the code at this commit.
Failure scenario: a multi-selection drop of N images each ≤ 8 MiB (e.g. 60×5 MiB — collectDroppedFiles imposes no count limit, unlike the workspace lane's MAX_FILES_PER_BATCH) pins ≈ N×1.33× raw of base64 in the tab (~400 MiB for the example; larger drops push the tab past 1 GiB → freeze/OOM, losing session state); repeated pastes accumulate since each batch gets the full per-file budget.
Witness: not run — browser heap retention does not reproduce in the unit harness; mechanism verified by code inspection (per-file maxBytes loop here, no aggregate counter anywhere in the ingestion lane). The other arm of R1-6 (older-daemon 10 MB JSON body) rests on your atomic-upgrade architecture statement (daemon, Web Shell, SDK ship together — documented in Risk & Scope), which this review accepts as disputing that arm.
You declined restoring cumulative caps citing product requirements (per-file 8 MiB only). Since the mechanism stands in code, a maintainer should own that decision explicitly on the record — or restore an aggregate encoded-size/count budget on the ingest lane.
中文说明
R1-6 复查：仍然存在。累计编码字节预算仍然被按文件原始大小检查取代，没有任何批量/跨批量核算，同时每张被接受的图片仍会被立即读成 base64 并保留在 React 状态中等待上传。第一轮 R1-6 的"标签页内存"分支在本次提交的代码中没有变化。
失败场景：一次多选拖入 N 张每张 ≤ 8 MiB 的图片（例如 60×5 MiB——collectDroppedFiles 没有数量上限，不像工作区通道有 MAX_FILES_PER_BATCH）会在标签页中钉住约 N×1.33 倍原始大小的 base64（例子约 400 MiB；更大的拖入会把标签页推过 1 GiB → 卡死/OOM，丢失会话状态）；重复粘贴会累积，因为每批都拿到完整的单文件预算。
证据：未运行——浏览器堆保留无法在单测环境复现；机制经代码检查确认（此处按文件 maxBytes 循环，整个摄入通道没有任何累计计数器）。R1-6 的另一分支（旧 daemon 的 10 MB JSON body）基于你的原子升级架构声明（daemon、Web Shell、SDK 同包发布——已写入 Risk & Scope），本审查接受该声明足以质疑那一分支。
你以产品要求为由（只保留单文件 8 MiB）拒绝恢复累计上限。由于机制在代码中仍然存在，请 maintainer 在 PR 上明确认领该决策——或在摄入通道恢复累计编码字节/数量预算。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
按已确认的交互要求，本轮不增加批量或跨批次累计图片限制。图片与其他附件统一只保留单文件 8 MiB 上限，多个文件均允许选择引用；恢复累计预算会重新引入用户明确要求移除的批量限制。该内存取舍留给 maintainer 确认，因此不 resolve。
|
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 |
…upload-or-reference
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM (Round 4 — AI Generated)
HEAD: 12426258c2（含 85a261e2fb fix）
经本轮全量复核，所有代码层面的 Critical/Major findings 均已在 85a261e2fb 中修复：
- R2-1 队列内联附件预算恢复 ✅
- R2-4
.file扩展名与 daemon MIME gate 对齐 ✅ - R2-5 dedupe 重试误删已写入附件 ✅
- R2-6
remove()TOCTOU 竞态 ✅ - R2-7
close()/delete()copyFrom 竞态 ✅ - R2-8 mid-turn resource 块路由→bridge 丢弃 ✅
- R2-9 surrogate 正则缺
uflag 删 emoji ✅ - doudouOUC Major enqueue 失败后孤儿附件清理 ✅
- R2-10/11/12 队列水合、bundle ledger、slash echo ✅
剩余开放项（无累计容量上限 / mediaReferences 兼容层 / SDK rename compat bridge）均为产品与发布策略决策，已在 PR summary 中明确说明，需 maintainer 人工确认，不属于代码 correctness 问题，不阻塞合并。
✅ Approve
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM (Round 5 — AI Generated)
HEAD: c9181991a46e — one new commit since the previous approve (12426258c2)
New commit: fix(webui): restore optimistic text prompts
Change: actions.ts — both submitPrompt and enqueueMidTurnMessage paths
The fix pre-appends the optimistic user message before the promptContentWithUploadedAttachments upload step, but only when displayedImages.length === 0 && displayedFiles.length === 0 (text-only prompt). All subsequent appendLocalUserMessage calls (error / abort / rejection / success paths) are guarded by !optimisticMessageAppended to prevent double-append. Both code paths updated symmetrically.
This correctly restores pre-PR behavior where text-only prompts showed the optimistic message immediately, instead of waiting for the async upload pipeline to settle.
CI: All checks green — ubuntu Test ✅ · E2E Smoke ✅ · Serve A/B ✅ · Real daemon E2E ✅ · Desktop Shell (ubuntu + windows) ✅
No new issues introduced. Prior findings unchanged.
✅ Approve
Superseded by newer review rounds (R4/R5 approved); outstanding items are product/release decisions already noted in PR summary.
Conflict: the SDK browser-bundle budget — main bumped 198→199KB for persistent session attachments while this branch needed headroom for the session PR binding types; resolved at 200KB with both reasons recorded. Also fixes a latent type error currently on main: QwenLM#9477 added deleteSessionAttachments to the deleteDaemonSessions bridge contract, but the batch-delete test from QwenLM#9341 still passes a closeSession-only mock; CI did not catch it because the verify check is skipped on main pushes.
…nLM#9551) `deleteDaemonSessions` takes `bridge: Pick<AcpSessionBridge, 'closeSession' | 'deleteSessionAttachments'>` since QwenLM#9477, which updated every mock that existed when it was written. QwenLM#9341 landed in parallel and added one more — "collapses case-variant spellings in one batch to a single delete" — with a `{ closeSession }` bridge. Each PR was green on its own merge ref; main is red combined, so `npm ci` fails the build for every branch cut from it: src/serve/server/session-archive.test.ts(1069,7): error TS2741: Property 'deleteSessionAttachments' is missing in type '{ closeSession: Mock<Procedure> }' Adds the same `vi.fn().mockResolvedValue(undefined)` its neighbours already pass. The test asserts on the delete result, not on the spy, so its meaning is unchanged: 48/48 still pass.
|
Released in v0.21.15. |
5 participants
What this PR does
This PR unifies dropped-image and dropped-file handling in Web Shell. Dropping one or more files now asks whether to reference them in the conversation or upload them into the workspace, and referenced images and files are rendered as separate attachment rows instead of being embedded in the message bubble.
Referenced content is stored as persistent, session-scoped attachments in the Qwen runtime temp area. Attachment identifiers are their stored filenames, duplicate names receive a
(1)-style suffix, the data survives daemon restarts and client reconnects, and it is removed when the owning session is deleted. The per-file 8 MiB limit remains, while the former per-session 100 MiB and 256-item limits are removed.The right sidebar can now preview referenced attachments, completed workspace uploads, and files selected with
@. File-type icons are shared across composer chips, sent messages, and sidebar tabs; Markdown and HTML can switch to their existing rendered preview modes; PDFs and browser-supported formats use an embedded preview; unsupported binary formats show a clear fallback. Directories selected through@are not clickable.Prompt admission, queued and mid-turn messages, transcript recording, replay hydration, session branching, and daemon routes now carry the same attachment references, so sent files are immediately clickable and remain available after refresh or restart.
Why it's needed
The previous behavior treated images as prompt content but files as workspace uploads, even though a user may intend either action for either kind of file. It also made attachment preview depend on transient client state, which caused sent files to become clickable only after refresh and caused attachments to disappear after daemon lifecycle changes.
The earlier media design was image-focused and relied on
mediaId, an in-memory retained-media index, TTL cleanup, and separate media routes. This PR replaces it with one filename-backed attachment model for images and files. A compatibility layer is intentionally not retained: the daemon, Web Shell, and TypeScript SDK are released together as one package, so these protocol changes are upgraded atomically rather than mixed across versions. The old image bytes were process-lifetime data and became invalid when an upgrade restarted the daemon, so there is no durable media payload to migrate; retaining both protocols would only add dead routing and cleanup complexity.Reviewer Test Plan
How to verify
@and confirm it opens in the sidebar. Select a directory and confirm it does not open a file preview.Evidence (Before & After)
Before: images and files followed different drop paths, referenced content was embedded into prompt state, sent attachments could require a refresh before preview worked, and temporary media could disappear through TTL or daemon cleanup.
After: every drop has an explicit reference-or-upload choice, referenced content uses persistent session attachments, attachment rows are separate from message text, and file/image previews open consistently in the right sidebar across send, refresh, and daemon restart.
Automated verification completed locally: full repository build, full workspace typecheck, full repository lint, 661 Web Shell tests, 369 SDK tests, 149 ACP bridge attachment/client tests, 99 Web UI session tests, and the relevant CLI session/server tests. The large CLI server suite's timing-sensitive unrelated cases were also rerun individually and passed.
Tested on
Environment (optional)
Local Node.js workspace build and Vitest/jsdom test environment. Browser automation was not used.
Risk & Scope
session_media, media routes, and themediaIdreference shape are replaced bysession_attachments, attachment routes, and filename-basedattachmentId. No compatibility shim is included because all consumers ship together and the old process-lifetime image data does not survive the daemon restart performed during an upgrade.Linked Issues
Related to #9351.
中文说明
本 PR 做了什么
本 PR 统一了 Web Shell 中拖入图片和文件的处理方式。现在拖入一个或多个文件时，都会询问用户是将其引用到会话中，还是上传到工作区；被引用的图片和文件会作为独立的附件行展示，不再嵌入消息气泡内部。
引用内容会作为持久化、session 级别的附件存放在 Qwen 运行时临时目录中。附件标识直接使用实际存储的文件名，重名文件自动追加
(1)形式的后缀；数据在 daemon 重启和客户端重新连接后仍然保留，只在所属 session 被删除时清理。单文件 8 MiB 限制继续保留，原有的单 session 100 MiB 和 256 项限制已移除。右侧边栏现在可以预览引用附件、已完成的工作区上传，以及通过
@选择的文件。输入框附件、已发送消息和侧边栏 tab 共用按文件类型区分的图标；Markdown 和 HTML 可以切换到已有的渲染预览模式；PDF 和浏览器支持的格式使用内嵌预览；不支持的二进制格式会展示明确的降级提示。通过@选择的目录不可点击。Prompt 准入、排队消息和 mid-turn 消息、会话记录、回放恢复、session 分支以及 daemon 路由现在都传递同一种附件引用，因此文件发送后可以立即点击，并且刷新或重启后仍然可用。
为什么需要
此前图片会作为 prompt 内容处理，而文件会被上传到工作区，但用户对任意一种文件都可能希望执行“引用”或“上传”。同时，附件预览依赖临时客户端状态，导致已发送文件有时必须刷新后才能点击，并且附件可能在 daemon 生命周期变化后丢失。
之前的 media 方案主要面向图片，并依赖
mediaId、内存中的 retained-media 索引、TTL 清理以及独立的 media 路由。本 PR 使用一个基于文件名的统一附件模型替换它，同时覆盖图片和普通文件。这里明确不保留兼容层：daemon、Web Shell 和 TypeScript SDK 会随同一个发行包一起升级，不存在新旧协议消费者混用；旧图片数据只在进程生命周期内有效，升级重启 daemon 时本来就会失效，因此也没有可迁移的持久 media 数据。保留两套协议只会增加无用的路由和清理复杂度。Reviewer 测试计划
如何验证
@选择普通文件并确认它能在侧边栏打开；选择目录并确认不会打开文件预览。证据（修改前后）
修改前：图片和文件走不同的拖入路径，引用内容嵌入 prompt 状态，已发送附件可能需要刷新后才能预览，临时 media 还可能因 TTL 或 daemon 清理而消失。
修改后：每次拖入都明确选择引用或上传，引用内容使用持久化 session 附件，附件行与消息文本分开，并且文件和图片在发送、刷新及 daemon 重启后都能稳定地从右侧边栏预览。
本地自动验证已完成：全仓构建、所有 workspace 类型检查、全仓 lint、661 项 Web Shell 测试、369 项 SDK 测试、149 项 ACP bridge 附件/客户端测试、99 项 Web UI session 测试，以及相关 CLI session/server 测试。CLI 大型 server 测试中与本改动无关且对时间敏感的用例也已单独重跑并通过。
测试平台
环境（可选）
本地 Node.js workspace 构建与 Vitest/jsdom 测试环境。未使用浏览器自动化。
风险与范围
session_media、media 路由和mediaId引用结构被替换为session_attachments、附件路由和基于文件名的attachmentId。因为所有消费者随同一发行包一起升级，且旧的进程级图片数据在 daemon 重启时本来就会失效，所以不提供兼容层或迁移逻辑。关联 Issue
关联 #9351。