Skip to content

feat(web-shell): support upload directory and hard-disable drag-in when fileUploadEnabled=false - #9382

Merged
ytahdn merged 1 commit into
QwenLM:mainfrom
ytahdn:web-shell-upload-folder
Aug 18, 2026
Merged

feat(web-shell): support upload directory and hard-disable drag-in when fileUploadEnabled=false#9382
ytahdn merged 1 commit into
QwenLM:mainfrom
ytahdn:web-shell-upload-folder

Conversation

@ytahdn

@ytahdn ytahdn commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Web Shell file upload gets two changes.

First, fileUploadEnabled={false} now disables file drag-and-drop entirely. Previously a file dragged onto the composer still landed on the inline image/text lane, producing an attachment tag even though no upload happened. Now the composer surface ignores file drags completely — no drag highlight, no drop ingestion, no upload — while clipboard paste of images and text stays enabled. The composer core gates its drag/drop handlers behind a new fileDragEnabled option, and the editor cancels the drop outright, so the browser cannot navigate to a dropped file.

Second, a new fileUploadDirectory prop (relative to the workspace root) sets where drag-and-dropped files upload; the workspace root remains the default. The daemon upload route now materializes a missing target directory — recursively, with a 64-component depth cap — via a new WorkspaceFileSystem.mkdir primitive, so a configured drop folder needs no manual setup.

Why it's needed

Hosts that disable file upload expect drag-in to be fully inert; leaking attachment tags through the inline lane contradicts that. Hosts that do enable upload need a way to route dropped files into a specific folder (e.g. an uploads/ directory) without pre-creating it, which is what the new prop plus auto-created directories provide.

Reviewer Test Plan

How to verify

  1. Run the affected unit suites: cd packages/cli && npx vitest run src/serve/fs/workspace-file-system.test.ts src/serve/routes/workspace-file-write.test.ts and cd packages/web-shell && npx vitest run client/components/ChatEditor.test.tsx client/hooks/useComposerCore.dom.test.tsx client/App.test.tsx.
  2. fileUploadEnabled={false}: drag a file (and separately an image) onto the composer — no drop highlight, no overlay, no tag, no upload strip appears; Ctrl/Cmd+V of an image still attaches it.
  3. fileUploadDirectory="uploads" (folder does not exist): drag a file into the composer — it uploads into uploads/, the directory is created automatically, and the composer inserts an @uploads/<name> reference.
  4. Upload to a path with a parent missing by more than 64 components is rejected with parse_error before any directory is created.

Evidence (Before & After)

N/A — behavior is covered by the unit tests above (drag-in gating, directory plumbing, auto-creation, depth cap).

Tested on

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

Environment (optional)

npm run dev:daemon with the standalone web-shell page, verifying drag-in disable and directory upload end-to-end.

Risk & Scope

  • Main risk or tradeoff: the upload route now creates missing parent directories instead of failing with "parent directory does not exist" — a behavior change for direct API callers, and failed requests (413, abort, 409) may leave an empty directory behind; the widened directory-creation surface is documented in docs/design/web-shell-file-upload.md.
  • Not validated / out of scope: the @ panel upload item still uploads to the browsed directory (not fileUploadDirectory); leading-slash directory values are intentionally rejected as outside the workspace.
  • Breaking changes / migration notes: none for web-shell hosts; uploads to a non-existent directory now succeed instead of returning 400.

Linked Issues

None.

中文说明

本 PR 做了什么

Web Shell 文件上传功能有两项改动。

第一项:fileUploadEnabled={false} 现在会完全禁用文件拖入。此前文件拖到合成器上仍会落入 inline 图片/文本通道,产生附件 tag(尽管并没有真正上传)。现在合成器表面会完全忽略文件拖拽——没有拖拽高亮、没有 drop 摄入、没有上传——而剪贴板粘贴图片和文本仍然可用。合成器核心在新增的 fileDragEnabled 选项后面门控了它的拖拽/drop 处理器,编辑器也会直接取消 drop,避免浏览器导航到被拖入的文件。

第二项:新增 fileUploadDirectory 属性(相对于工作区根目录),用于设置拖入文件的上传目标目录;默认仍是工作区根目录。daemon 上传路由现在会通过新的 WorkspaceFileSystem.mkdir 原语自动创建缺失的目标目录(递归创建,组件数上限 64),因此配置好的上传目录无需手动预先创建。

为什么需要

禁用文件上传的宿主希望拖入完全失效;通过 inline 通道泄漏附件 tag 与该预期矛盾。启用上传的宿主则需要一种方式把拖入的文件路由到指定文件夹(例如 uploads/ 目录)而无需预先创建,这正是新属性加上自动建目录所提供的。

Reviewer 测试计划

如何验证

  1. 运行相关单元测试:cd packages/cli && npx vitest run src/serve/fs/workspace-file-system.test.ts src/serve/routes/workspace-file-write.test.ts 以及 cd packages/web-shell && npx vitest run client/components/ChatEditor.test.tsx client/hooks/useComposerCore.dom.test.tsx client/App.test.tsx
  2. fileUploadEnabled={false}:把文件(以及单独的图片)拖到合成器上——不应出现拖拽高亮、overlay、tag 或上传条;Ctrl/Cmd+V 粘贴图片仍可附加。
  3. fileUploadDirectory="uploads"(目录不存在):拖一个文件进合成器——文件上传到 uploads/,目录被自动创建,合成器插入 @uploads/<name> 引用。
  4. 父目录缺失超过 64 个组件的上传路径会在创建任何目录之前以 parse_error 被拒绝。

Evidence (Before & After)

N/A —— 行为已由上述单元测试覆盖(拖入门控、目录贯通、自动创建、深度上限)。

测试环境

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

运行环境(可选)

npm run dev:daemon 配合独立 web-shell 页面,端到端验证拖入禁用与目录上传。

风险与范围

  • 主要风险或权衡:上传路由现在会创建缺失的父目录,而不是报 "parent directory does not exist"——这对直接调用 API 的调用方是行为变化,且失败请求(413、中断、409)可能留下空目录;目录创建能力拓宽已在 docs/design/web-shell-file-upload.md 中说明。
  • 未验证/不在范围:@ 面板的上传项仍上传到浏览目录(而非 fileUploadDirectory);前导斜杠的目录值会被有意拒绝(视为工作区外)。
  • 破坏性变更/迁移说明:对 web-shell 宿主无破坏性变更;上传到不存在的目录现在成功而不是返回 400。

关联 Issues

无。

…en fileUploadEnabled=false

fileUploadEnabled={false} previously still admitted file drags onto the
inline image/text lane, producing attachment tags with no upload. It now
disables file drag-and-drop entirely — no drag highlight, no drop ingestion,
no upload — while clipboard paste stays enabled. The composer core gates its
drag/drop handlers behind a new fileDragEnabled option; ChatEditor cancels
the drop outright.

A new fileUploadDirectory prop (relative to the workspace root) sets the
drag-upload target directory; the root remains the default. The daemon upload
route now materializes a missing target directory (recursively, depth-capped)
via a new WorkspaceFileSystem.mkdir, so a configured drop folder needs no
manual setup. mkdir follows the existing write-path safeguards: trust gate,
generation guard, path lock, audit, and symlink-swap re-checks per created
component and its parent.
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on aa690a8 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— aa690a8 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: real, and I verified it in the base code rather than taking the description at face value. With fileUploadEnabled={false}, handleUploadDrop falls through to the inline image lane (!uploadEnabledcore.imageTransferHandlers.onDropCapture(event)), so a dropped file does produce an attachment tag even though nothing was uploaded — a structural leak in the existing host-control gate, not a theoretical concern. The fileUploadDirectory half is a feature extension of the existing upload design, grounded in docs/design/web-shell-file-upload.md, which this PR updates in sync.

Direction: aligned. This refines an already-shipped web-shell capability (host control + upload routing) instead of adding a parallel mechanism. No auth/model-selection/telemetry/release surface is touched; the daemon-side change stays inside the existing trust model (capability-gated, trusted workspace, qualified routes). The widened directory-creation surface is explicitly documented in the design doc, including the .git/ component discussion.

Size: 252 production lines (227 code + the audit operation field) vs 317 test lines vs 9 docs lines — comfortably under any escalation threshold. The touched paths (packages/cli/src/serve/fs, serve/routes, packages/web-shell/client) are not core-module paths, but the fs layer is security-sensitive, so the review below reads it at that depth.

Approach: the scope feels right. Gating in the composer core (not just ChatEditor) is the correct layer — the drag highlight and the capture-phase drop ingestion live there, so a ChatEditor-only early return would have left both leaking. The new mkdir primitive follows the established fs-layer pattern (trust check, path lock, generation guard, audit, per-component lstat re-checks for symlink swaps) rather than calling fsp.mkdir({ recursive: true }), which is the right call for this threat model. No unrelated changes spotted.

Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:真实存在,且我已在基础代码中核实(而非仅凭描述)。fileUploadEnabled={false} 时,handleUploadDrop 会落入 inline 图片通道(!uploadEnabledcore.imageTransferHandlers.onDropCapture(event)),因此拖入文件确实会产生附件 tag——尽管并没有发生上传。这是现有宿主控制门控上的结构性泄漏,不是理论性问题。fileUploadDirectory 部分是对现有上传设计的功能扩展,以 docs/design/web-shell-file-upload.md 为依据,本 PR 同步更新了该设计文档。

方向:对齐。这是对已上线的 web-shell 能力(宿主控制 + 上传路由)的完善,而不是新增平行机制。未触及 auth/模型选择/遥测/发布面;daemon 侧改动仍在现有信任模型内(capability 门控、受信任工作区、qualified 路由)。拓宽的建目录能力已在设计文档中明确说明,包括对 .git/ 组件的讨论。

规模:生产代码 252 行(227 行代码 + audit 的 operation 字段),测试 317 行,文档 9 行——远低于任何升级阈值。触及路径(packages/cli/src/serve/fsserve/routespackages/web-shell/client)不属于核心模块路径,但 fs 层安全敏感,下面的代码审查按该深度进行。

方案:范围合理。在合成器核心层(而非仅在 ChatEditor)做门控是正确的层级——拖拽高亮与 capture 阶段的 drop 摄入都在那里,只在 ChatEditor 提前返回无法堵住这两处泄漏。新的 mkdir 原语遵循既有 fs 层模式(信任检查、路径锁、代际守卫、审计、逐组件 lstat 复查以防符号链接掉包),而不是直接调用 fsp.mkdir({ recursive: true })——对该威胁模型而言这是正确选择。未发现无关改动。

风险:无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

I read the full diff against the base code. No blockers found — the implementation is careful and the test coverage is genuinely behavioral, not ceremonial. Notes below.

Daemon side. The new mkdir primitive mirrors the established WorkspaceFileSystem pattern exactly — trust-intent assertion, path lock, generation guard before and after, audit with ignore verdict, recordAndWrap on failure — so it inherits the layer's existing guarantees rather than bypassing them. ensureResolvedDirectory walks up to the deepest existing ancestor, creates each missing component one at a time, tolerates an EEXIST create race by re-checking the winner, and lstat-re-checks every created component (plus each component's parent before the next mkdir) so a symlink swapped in mid-creation surfaces as symlink_escape instead of being followed. There is a residual sub-microsecond window between the parent check and the mkdir within one iteration, but exploiting it requires concurrent write access inside an already-trusted workspace, and the existing write paths carry the same class of race — the re-checks here are defense-in-depth on par with the layer, and the race tests pin the documented semantics. The two interface mocks in bridge-file-system-adapter.test.ts are the only mocks of this interface in the repo, and both were updated.

The route change is placed correctly: the 64-component cap fires before buffering and before the concurrency gate, and mkdir runs only on path_not_found from the directory stat — every other error still rethrows. The re-stat after mkdir keeps the existing "parent path is not a directory" check live. One consequence the PR already discloses: a failed upload (413/abort/409) can leave the freshly created empty directory behind — acceptable for a drop folder, and stated in Risk & Scope.

One suggestion (non-blocking). The depth cap counts dir.split('/') components, but path.dirname on Windows preserves backslashes from a \-separated query path, so such a path would count as a single component and slip past the cap on a Windows daemon. The blast radius is bounded — resolveWithinWorkspace's ancestor walk is independently capped at MAX_ANCESTOR_HOPS = 40, so the auto-creation depth can't exceed ~40 either way, and this requires an authenticated caller in a trusted workspace — but since this cap exists precisely to bound what one request may materialize ahead of the concurrency gate, counting both separators (e.g. dir.split(/[\\/]/)) would make it hold cross-platform.

Web-shell side. The gate sits at the right layer in both places. I verified the wiring in the base code: the composer core's capture-phase drag handlers are spread onto the surface and fire before ChatEditor's drop handler, so gating only in ChatEditor would have left both the drag highlight and the inline-lane ingestion leaking — the new fileDragEnabled option closes both, while ChatEditor's own early return closes the upload lane and the !uploadEnabled → image lane fallthrough. A disabled drop is still preventDefault-ed (the browser can't navigate to the file) but not stopPropagation-ed (host handlers keep working), paste is untouched, and the extra effect clears a stuck drag highlight if a host flips the flag mid-drag. Dependency arrays and the ref-based option plumbing follow the file's existing style.

Tests. The suite pins the behavior rather than the implementation: the route test flips from "rejects a missing parent" to "creates it and uploads into it" (matching the intended behavior change), a 65-component request asserts rejection before any tree exists, the symlink-race tests swap the final component and an intermediate ancestor respectively, and the DOM test asserts an inert drag end-to-end (no highlight, dropEffect stays none, nothing ingested, drop cancelled).

Files changed (14 of 14 shown)
File What changed
docs/design/web-shell-file-upload.md Documents the mkdir primitive and its symlink defense, the 64-component cap, the drag-in hard-disable, and the new directory prop
packages/cli/src/serve/bridge-file-system-adapter.test.ts Adds mkdir to the two interface mocks so they still satisfy the widened type
packages/cli/src/serve/fs/audit.ts Optional operation field so a mkdir is distinguishable from a zero-byte write in the audit trail
packages/cli/src/serve/fs/workspace-file-system.test.ts New mkdir suite: create, recursive, reuse, non-directory and symlink-race rejections, audit record
packages/cli/src/serve/fs/workspace-file-system.ts New mkdir method plus the ensureResolvedDirectory helper with per-component lstat re-checks
packages/cli/src/serve/routes/workspace-file-write.test.ts Flips the missing-parent test to expect creation, adds the depth-cap rejection test
packages/cli/src/serve/routes/workspace-file-write.ts Depth cap before buffering, and recursive mkdir when the directory stat reports path_not_found
packages/web-shell/client/App.test.tsx Asserts the directory prop reaches the composer customization, and is absent when omitted
packages/web-shell/client/App.tsx Declares fileUploadDirectory and threads it through the customization context memo
packages/web-shell/client/components/ChatEditor.test.tsx Tests drag-in disable, upload into the configured directory, and the workspace-root default
packages/web-shell/client/components/ChatEditor.tsx Gates the drop handler on fileUploadEnabled and passes the directory into uploadFiles
packages/web-shell/client/customization.tsx Adds fileUploadDirectory to the customization context type
packages/web-shell/client/hooks/useComposerCore.dom.test.tsx DOM-level test that a disabled file drag stays fully inert
packages/web-shell/client/hooks/useComposerCore.ts New fileDragEnabled option gating the capture-phase drag and drop handlers

Test evidence (PR's own CI — I do not run PR code)

No failures so far. The main unit suite is still in flight at review time — Test (ubuntu-latest, Node 22.x), Serve A/B (ubuntu-latest, Node 22.x), and Capture web-shell visuals — so the table below will be updated in place by the finalize job once CI settles. The macOS/Windows test jobs are skipped, which is normal for fork PRs; everything else that completed is green, including the Desktop Shell builds on both OSes and the secret/CVE scans. The log bodies of any future failure would be treated as untrusted text; classification here rests on check identity and the diff.

Final CI results for aa690a8 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
macos-latest / Java 21 ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle the remaining gap: @qwen-code /verify — that an upload into a non-existent directory succeeds and materializes it (where the base build rejects with 400), and that drag-in is fully inert with upload disabled, is pinned by unit tests here but an A/B run against the base build would prove the diff is load-bearing; the Windows depth-cap note above also rests on a platform this PR's CI never exercises.

Not verified: real-browser drag-and-drop behavior (unit DOM tests only), and Windows/Linux runtime behavior — author reports macOS tested, Windows/Linux untested.

中文说明

代码审查:未发现阻塞问题。daemon 侧新增的 mkdir 原语完整复用了 WorkspaceFileSystem 既有模式(信任断言、路径锁、代际守卫、审计、失败包装),ensureResolvedDirectory 逐级创建缺失组件,每个组件创建后立即 lstat 复查、下一个组件创建前复查其父目录,中途被替换为符号链接会以 symlink_escape 拒绝而不是被跟随;单次迭代内父目录检查与 mkdir 之间残留的极小时间窗与既有写路径同级别,属于该层的纵深防御范畴。路由改动位置正确:64 组件深度上限在缓冲与并发门之前生效,仅在 path_not_found 时建目录,其余错误照旧抛出。唯一的非阻塞建议:深度上限用 dir.split('/') 计数,Windows daemon 上反斜杠路径会绕过计数(影响有限——resolveWithinWorkspaceMAX_ANCESTOR_HOPS = 40 上限独立约束了创建深度),建议改为同时统计两种分隔符。web-shell 侧门控层级正确:capture 阶段的拖拽处理器在合成器核心层,必须在那里门控才能同时堵住高亮与 inline 通道泄漏;禁用的 drop 仍被取消(浏览器不会导航到文件)但不阻断宿主处理器,粘贴不受影响,中途切换标志会清理残留高亮。测试钉住的是行为而非实现。

测试证据:引自 PR 自身的 CI(不运行 PR 代码)。截至目前无失败;主单元测试(ubuntu Test、Serve A/B、web-shell 视觉快照)仍在运行,表格将由 finalize 任务在 CI 落定后原地更新。macOS/Windows 测试任务为 skipped(fork PR 常态),其余已完成检查全部通过。沙盒验证可补齐剩余缺口:@qwen-code /verify 可 A/B 证明"上传到不存在目录会成功并自动创建"这一行为改动确实由本 diff 承载,且上述 Windows 深度上限问题所在平台未被本 PR 的 CI 覆盖。未验证:真实浏览器拖拽行为(仅单元 DOM 测试)、Windows/Linux 运行时行为(作者仅测试 macOS)。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head aa690a8. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 3 render-shaping files:

  • packages/web-shell/client/App.tsx
  • packages/web-shell/client/components/ChatEditor.tsx
  • packages/web-shell/client/customization.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head aa690a8, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, well-tested change that closes a real leak in an existing host-control gate; the only nit is the Windows depth-cap counting, which is bounded anyway.

Stepping back: this PR does one coherent thing — make the web-shell's file-upload host controls actually hold — and it does it at the right layers. Before reading the diff I would have proposed exactly this shape: gate the composer core's capture-phase drag handlers behind the existing flag, cancel drops without letting the browser navigate, and back the new directory prop with a hardened mkdir in the fs layer rather than a route-level fs.mkdir shortcut. The implementation matches that, then goes further than I would have bothered to on the symlink-race defense, and the tests pin the semantics (including both race windows) instead of the plumbing. The behavior change for direct API callers — missing directories now created instead of 400 — is disclosed in the PR body and the design doc, which is the honest way to ship it.

Reservations, all non-blocking: the Windows separator counting noted in my review; empty directories left behind by failed uploads (disclosed, acceptable for a drop folder); and real-browser drag-and-drop remains untested beyond DOM unit tests — the sandboxed /verify lane named above is the way to close that if a maintainer wants it before merge.

Approval is deferred until CI lands green on aa690a8024885696da7f1d2dce256e5daa84ff7b — the main unit suite, Serve A/B, and web-shell visual capture are still in flight at review time. If everything completes green on that commit, the approval follows automatically; if anything lands red or the head moves, this deferral stands and the table in my review comment will say why.

中文说明

总体判断:这是一个内聚的改动——让 web-shell 文件上传的宿主控制真正生效——且层级选择正确。读 diff 之前我的独立方案与此一致:在合成器核心的 capture 阶段门控拖拽处理器、取消 drop 但不让浏览器导航、用 fs 层加固的 mkdir 支撑新的目录属性,而不是在路由里走 fs.mkdir 捷径。实现与方案吻合,且在符号链接竞态防御上做得更深,测试钉住的是语义(包括两个竞态窗口)而非管线。对直接 API 调用方的行为变化(缺失目录从 400 变为自动创建)已在 PR 正文与设计文档中如实披露。

保留意见(均不阻塞):review 中提到的 Windows 分隔符计数问题(影响有界);失败上传可能留下空目录(已披露,对上传目录可接受);真实浏览器拖拽在 DOM 单元测试之外尚未验证——如需在合并前补齐,可由 maintainer 触发上面提到的沙盒 /verify 通道。

批准暂缓至 CI 在 aa690a8024885696da7f1d2dce256e5daa84ff7b 上全部通过——主单元测试、Serve A/B 与 web-shell 视觉快照仍在运行。若该 commit 全绿,批准将自动跟上;若有检查变红或 head 移动,则维持暂缓,review 评论中的 CI 表格会说明原因。

Qwen Code · qwen3.8-max

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — feat(web-shell): support upload directory and hard-disable drag-in

Verdict: Approve — solid implementation with thorough security design and strong test coverage. Two nits below.


Cross-Validation

Finding CI bot My Assessment
General LGTM ✅ APPROVED Confirmed — reviewed independently across all 14 files

The CI bot left no inline findings, so this section tracks my independent audit dimensions against its approval.


Additional Audit Coverage

Independent areas I verified beyond the green-CI signal:

  1. ensureResolvedDirectory security chain — traced the full safeguard sequence for both recursive and non-recursive paths: trust gate → generation guard → path lock (on final path) → per-component lstat re-check immediately after each mkdir → parent lstat re-check before the next mkdir. Concurrent-create races (EEXIST) are caught and re-verified rather than silently swallowed. The two symlink-swap race tests (final component and intermediate ancestor) exercise the real code paths correctly.

  2. fileDragEnabled handler completeness — verified all four drag event handlers in useComposerCore.ts (onDragEnterCapture, onDragOverCapture, onDragLeaveCapture, onDropCapture) consistently gate on fileDragEnabledRef.current. The mid-drag useEffect cleanup handles the case where fileUploadEnabled flips to false while a drag is active (depth counter would otherwise never drain). The ChatEditor's own early-return guard is a correct second layer, not redundancy: the two handlers sit on different elements (outer surface capture vs. editor bubble phase).

  3. Depth cap correctnessdir.split('/').filter(Boolean).length > 64 is a conservative upper bound: . components count but create no directories; .. components count but are rejected by fs.resolve downstream. The cap is a pre-admission coarse filter; the actual security boundary remains fs.resolve. Verified filter(Boolean) correctly handles leading slashes and empty segments.

  4. bridge-file-system-adapter.ts type boundary — confirmed createBridgeFileSystemAdapter returns BridgeFileSystem, not WorkspaceFileSystem, so it does not need a mkdir implementation. The mkdir: vi.fn() additions in the test are for unrelated WorkspaceFileSystem mock objects (needed because the interface now declares mkdir).

  5. Audit record positionrecordAccess is called inside pathLocks.runExclusive, after ensureResolvedDirectory completes and the generation guard is re-asserted. The start timestamp is captured before the lock (consistent with writeBytesAtomic and other write primitives), so durationMs covers the full wall-clock cost including lock contention.

  6. Clipboard paste unaffected — confirmed fileDragEnabled=false only gates onDragEnterCapture / onDragOverCapture / onDragLeaveCapture / onDropCapture; paste (onPaste) is a separate code path not touched by this PR.


Nits

N1 · ChatEditor.tsx:1749 — empty-string fileUploadDirectory passes through ??

uploadFiles(files, fileUploadDirectory ?? '.', insertUploadReference);

?? only substitutes when the value is null | undefined; an explicit '' reaches uploadFiles as an empty string. The server-side behavior (fs.resolve('', 'write')) is not documented and the path through fileUploadAdmission for dir = '' is implicit (resolves to workspace root in practice, but relies on path.dirname turning '' into .). || '.' instead of ?? '.' would make the intent explicit, or a comment noting the empty-string case is intentionally treated as root.

N2 · App.tsx — no client-side warning for leading-slash fileUploadDirectory

The docs and server both reject paths starting with / ("outside the workspace"), but there is no client-side guard. A host accidentally passing '/uploads' would only discover the failure on the first drag-and-drop attempt. A console.warn on mount (when the prop is non-nullish and starts with /) would improve developer-facing DX without any runtime cost.

Neither nit affects correctness or security.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "agent 1b": none.** All planned checks completed within budget..

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):"agent 1b"none.** All planned checks completed within budget.

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +681 to +682
await fs.mkdir(resolvedDir, { recursive: true });
dirStat = await fs.stat(resolvedDir);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-1: Recursive directory creation runs ahead of the upload concurrency gate — this fs.mkdir(..., { recursive: true }) runs in the fileUploadAdmission middleware, registered BEFORE fileUploadConcurrencyGate, so directory materialization is not throttled by MAX_CONCURRENT_UPLOADS = 4, and nothing rolls a created tree back when the request later fails. — Failure scenario: an authenticated client floods POST /file/upload with unique deep paths and oversized/empty bodies → each request materializes up to ~39 missing directories in admission, then 413/429s without ever holding a gate slot; sustained flooding exhausts workspace inodes and every rejected request leaves empty directories behind.

Witness (probe against this commit):

chunked 51 MiB upload to a 30-deep path → 413 file_too_large, directoriesMaterialized: 30, treeExistsAfter413: true
8 concurrent 4 MiB uploads with unique 20-deep trees → statuses {201: 4, 429: 4}; all four 429'd requests had dirCreated: true

Suggested fix: move the fs.mkdir + re-stat out of admission into handlePostFileUpload after the gate lease is acquired (admission keeps the cheap fs.resolve boundary validation), so directory creation is subject to the same concurrency bound and rejected/aborted requests do not materialize trees — or document the accepted aggregate risk in the design doc.

中文说明

[Suggestion] R1-1:递归建目录发生在上传并发门控之前 —— 此处的 fs.mkdir(..., { recursive: true }) 运行在 fileUploadAdmission 中间件中,而该中间件注册在 fileUploadConcurrencyGate 之前,因此目录创建不受 MAX_CONCURRENT_UPLOADS = 4 的限制;请求随后失败时也没有任何回滚,已创建的目录树会被留下。— 失败场景:已认证客户端用唯一的深层路径加超大/空 body 轰炸 POST /file/upload → 每个请求在 admission 阶段创建最多约 39 个缺失目录,然后在从未持有门控槽位的情况下返回 413/429;持续轰炸会耗尽工作区 inode,且每个被拒绝的请求都会留下空目录。

证据(针对本 commit 的探针):分块 51 MiB 上传到 30 层深路径 → 413 file_too_large,directoriesMaterialized: 30,treeExistsAfter413: true;8 个并发 4 MiB 上传(唯一的 20 层深树)→ 状态 {201: 4, 429: 4},四个 429 请求全部 dirCreated: true。

建议修复:把 fs.mkdir + 重新 stat 从 admission 移到获取门控租约之后的 handlePostFileUpload 中(admission 保留低成本的 fs.resolve 边界校验),使目录创建受同样的并发限制,被拒绝/中止的请求不会创建目录树 —— 或者在设计文档中明确记录这一已接受的总体风险。

— qwen3.8-max via Qwen Code /review (v0.21.13)

* body is buffered, so a single request must not be able to spin up an
* unbounded directory tree ahead of the concurrency gate).
*/
const MAX_UPLOAD_DIR_DEPTH = 64;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-2: MAX_UPLOAD_DIR_DEPTH = 64 overlaps inconsistently with the pre-existing MAX_ANCESTOR_HOPS = 40 in fs/paths.ts — resolve's ENOENT fallback (findExistingAncestor) walks at most 40 hops for a not-yet-existing directory, which is exactly the path fs.resolve(dir, 'write') takes here, so the advertised 40-64 create range is a dead zone (effective create range: ≤39 fully-missing components). — Failure scenario: a host-configured drop folder 45 components deep (all missing) passes the static cap (45 ≤ 64), then fs.resolve exhausts its 40-hop budget and the upload fails with 404 path_not_found ("path traversal exceeded 40 hops while finding ancestor") instead of a created directory or the documented 400 depth-cap parse_error.

Witness (probe):

resolveWithinWorkspace on fully-missing dirs: 38/39 → resolved; 40/45/64 → threw path_not_found ("path traversal exceeded 40 hops")
route-level: 39 missing components → 201 created; 40 → 404; 45 → 404 "path traversal exceeded 40 hops while finding ancestor"

Suggested fix: align the two bounds — derive MAX_UPLOAD_DIR_DEPTH from MAX_ANCESTOR_HOPS so they cannot drift, or document the interaction explicitly; optionally add a route test at ~45 missing components pinning whichever behavior is chosen.

中文说明

[Suggestion] R1-2:MAX_UPLOAD_DIR_DEPTH = 64fs/paths.ts 中既有的 MAX_ANCESTOR_HOPS = 40 重叠且不一致 —— resolve 的 ENOENT 回退(findExistingAncestor)对尚不存在的目录最多向上查找 40 跳,而这里的 fs.resolve(dir, 'write') 走的正是该路径,因此宣传的 40-64 创建区间是死区(有效创建范围为 ≤39 个完全缺失的组件)。— 失败场景:宿主配置了 45 层深(全部缺失)的上传目录:静态上限检查通过(45 ≤ 64),随后 fs.resolve 耗尽 40 跳预算,上传以 404 path_not_found("path traversal exceeded 40 hops while finding ancestor")失败,而不是创建目录或返回文档所述的 400 深度上限 parse_error

建议修复:对齐两个边界 —— 从 MAX_ANCESTOR_HOPS 推导 MAX_UPLOAD_DIR_DEPTH 使两者不会漂移,或明确记录这一相互作用;可选地在约 45 个缺失组件处加一个路由测试来固定所选行为。

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +608 to +609
// 65 components exceeds MAX_UPLOAD_DIR_DEPTH; the request must fail
// before any directory tree is materialized.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-3: Inclusive depth-cap boundary untested — this test pins only the 65-component rejection, and the check is > (workspace-file-write.ts:621), so exactly 64 components (the documented maximum) is accepted today but the >>= off-by-one ships green. — Failure scenario: under that mutation, a host whose configured drop folder sits at exactly the cap gets spurious 400 parse_error rejections with no CI signal.

Witness (probe):

exactly 64 components (25 pre-existing + 39 missing) → 201 on current code
mutation `>` → `>=` → the same request flips to 400 parse_error; the full route suite passes 66/66 under the mutation

Suggested fix: add the paired boundary test — upload into a directory with exactly 64 components (with enough pre-existing ancestors to stay under resolve's hop budget) and expect 201 with the tree created.

中文说明

[Suggestion] R1-3:深度上限的闭边界(恰好 64 个组件)没有测试 —— 本测试只固定了 65 个组件被拒绝的情况,而检查用的是 >(workspace-file-write.ts:621),因此恰好 64 个组件(文档规定的最大值)目前会被接受,但 >>= 的 off-by-one 变异可以让全部测试绿灯通过。— 失败场景:在该变异下,宿主配置的上传目录恰好位于上限深度时会收到错误的 400 parse_error 拒绝,且 CI 毫无信号。

建议修复:补充成对的边界测试 —— 上传到恰好 64 个组件的目录(有足够的已存在祖先以保持在 resolve 跳数预算内),期望 201 且目录树被创建。

— qwen3.8-max via Qwen Code /review (v0.21.13)

it('creates a missing directory', async () => {
const r = await h.fs.resolve('new-dir', 'write');
await h.fs.mkdir(r);
const st = await fsp.stat(path.join(h.workspace, 'new-dir'));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-4: Documented 0o755 create mode asserted by no test — the interface JSDoc added by this diff and the design doc both state directories are created at 0o755 (modulo umask), and ensureResolvedDirectory passes { mode: 0o755 } in both create branches, but none of the eight new mkdir tests asserts the resulting mode (the same file pins 0o600 for created files at lines 1272 and 2389-2394). — Failure scenario: mutating the mode to 0o700 or 0o777 ships silently; under umask 002, 0o777 yields group/world-writable daemon-created directories, contradicting the documented contract.

Suggested fix:

// pin umask in the harness so the assert is environment-independent
process.umask(0o022);
// in the create tests (single and recursive):
expect(st.mode & 0o777).toBe(0o755);
中文说明

[Suggestion] R1-4:文档所述的 0o755 创建模式没有任何测试断言 —— 本 diff 新增的接口 JSDoc 和设计文档都声明目录以 0o755(受 umask 影响)创建,ensureResolvedDirectory 的两个创建分支也都传入 { mode: 0o755 },但新增的 8 个 mkdir 测试没有一个断言实际模式(同一文件在 1272 行和 2389-2394 行为创建的文件固定了 0o600)。— 失败场景:把模式变异为 0o7000o777 可以悄无声息地合入;在 umask 002 下 0o777 会产生组/全局可写的 daemon 创建目录,与文档契约矛盾。

建议修复:在测试框架中固定 umask(如 process.umask(0o022)),并在单级与递归创建测试中断言 expect(st.mode & 0o777).toBe(0o755)

— qwen3.8-max via Qwen Code /review (v0.21.13)

}
}

async mkdir(p: ResolvedPath, opts?: { recursive?: boolean }): Promise<void> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-5: mkdir lacks the untrusted-workspace rejection test every sibling primitive has — the trust guard is present below (assertTrustedForIntent(..., 'write')), so this is a missing-test finding, not a missing-guard one; but deleting the guard leaves the whole suite green, while writeTextOverwrite/writeBytesAtomic/edit all pin their rejections (lines 1323, 1726, 1735, 2397). — Failure scenario: a future or regressed caller of fs.mkdir materializes directory trees in untrusted workspaces with no failing test to notice.

Suggested fix — mirror the sibling pattern:

it('denies mkdir with untrusted_workspace', async () => {
  const h = await makeHarness({ trusted: false });
  const r = await h.fs.resolve('new-dir', 'write');
  const err = await h.fs.mkdir(r).catch((e: unknown) => e);
  expect(isFsError(err)).toBe(true);
  expect((err as { kind: string }).kind).toBe('untrusted_workspace');
});
中文说明

[Suggestion] R1-5:mkdir 缺少其他所有变更原语都有的 untrusted-workspace 拒绝测试 —— 信任守卫确实存在(下方的 assertTrustedForIntent(..., 'write')),因此这是缺测试而非缺守卫;但删除该守卫整个测试套件依然绿灯,而 writeTextOverwrite/writeBytesAtomic/edit 都固定了各自的拒绝行为(1323、1726、1735、2397 行)。— 失败场景:未来或回归的 fs.mkdir 调用方会在不受信任的工作区中创建目录树,而没有任何失败的测试能发现。

建议修复:仿照兄弟测试模式补充上面的用例。

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +1945 to +1946
if (fileDragEnabled === false) clearImageDragState();
}, [clearImageDragState, fileDragEnabled]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-6: This mid-drag cleanup effect has no test — deleting it ships green, yet it covers the scenario its own comment names: a host flipping fileUploadEnabled to false mid-drag gates the leave/drop handlers, and OS-originated file drags never fire dragend in the page (the window listener only sees page-originated drags). — Failure scenario: without this effect imageDragActive stays true indefinitely — the composer keeps advertising a drop target that no longer accepts drops, until unmount.

Witness (jsdom probe):

fileDragEnabled=true + dragenter/dragover (Files payload) → dragActiveBeforeFlip: true
flip to false + rerender → dragActiveAfterFlip: false
with the useEffect deleted → dragActiveAfterFlip: true (stuck highlight); 151/151 tests still pass under the mutation

Suggested fix: add a DOM test — mount with file drag enabled, dispatch dragenter with a Files payload and assert imageDragActive === true; re-render with fileDragEnabled: false and assert imageDragActive === false.

中文说明

[Suggestion] R1-6:这个拖拽中途清理 effect 没有测试 —— 删除它所有测试依然绿灯,而它覆盖的正是其注释所述的场景:宿主在拖拽中途把 fileUploadEnabled 翻转为 false 时,leave/drop 处理器被门控,而来自操作系统的文件拖拽不会在页面内触发 dragend(window 监听器只能看到页面内发起的拖拽)。— 失败场景:没有这个 effect,imageDragActive 会一直为 true —— 合成器会持续显示一个不再接受 drop 的放置目标,直到卸载。

建议修复:补一个 DOM 测试 —— 以启用文件拖拽挂载,派发带 Files 负载的 dragenter 并断言 imageDragActive === true;以 fileDragEnabled: false 重新渲染并断言 imageDragActive === false

— qwen3.8-max via Qwen Code /review (v0.21.13)

@ytahdn
ytahdn added this pull request to the merge queue Aug 18, 2026
Merged via the queue into QwenLM:main with commit 721e435 Aug 18, 2026
149 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants