Skip to content

fix(web-shell): preview document-classified artifacts - #9760

Merged
ytahdn merged 1 commit into
QwenLM:mainfrom
ytahdn:codex/fix-markdown-artifact-preview
Aug 23, 2026
Merged

fix(web-shell): preview document-classified artifacts#9760
ytahdn merged 1 commit into
QwenLM:mainfrom
ytahdn:codex/fix-markdown-artifact-preview

Conversation

@ytahdn

@ytahdn ytahdn commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Web Shell now recognizes Markdown, HTML, and safe raster images from their workspace path or normalized MIME type before applying a generic document classification. MIME parameters such as charset=utf-8 are normalized consistently through both download routing and the final renderer selection. SVG remains download-only, and known Office, PDF, video, and audio extensions take precedence over conflicting previewable MIME metadata.

Why it's needed

Generated artifacts can be recorded with the broad document kind even when their actual file is Markdown, HTML, or a raster image. The previous kind-first routing replaced those supported previews with a download-only card. This restores the existing format-specific previews while keeping active SVG content and binary document or media formats on the safer download path.

Reviewer Test Plan

How to verify

Open workspace artifacts reported as document for .md, .markdown, .html, .htm, and PNG files and confirm they render as Markdown, sandboxed HTML, and an image respectively. Repeat with extensionless Markdown, HTML, and PNG artifacts whose MIME types include parameters such as charset=utf-8, and confirm they use the same previews. Open SVG, DOCX, XLSX, PDF, and MP4 artifacts, including cases where their MIME metadata claims a previewable text or image type, and confirm they remain download-only without reading the workspace file into a text or image renderer.

Evidence (Before & After)

Before: supported Markdown, HTML, and raster files reported as document displayed only the download detail. SVG reported as image could fall through to a source preview, and a previewable conflicting MIME type could override a download-only extension.

After: supported text and raster formats use their existing previews; SVG and known binary document/media extensions fail closed to the download detail. The new behavior tests fail against the previous main implementation and pass with this change.

Tested on

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

Environment (optional)

Node.js 22; Web Shell Vitest tests, production build, and TypeScript typecheck.

Risk & Scope

  • Main risk or tradeoff: Format-routing precedence changes for workspace artifacts whose broad kind conflicts with their path or MIME metadata; this is covered with positive preview cases and fail-closed conflict cases.
  • Not validated / out of scope: Native previews for PDF, Word, Excel, PowerPoint, audio, video, and SVG rendering remain out of scope. Windows and Linux were not tested locally.
  • Breaking changes / migration notes: No API, schema, configuration, persistence, or dependency migration is required. After upgrading, existing Markdown, HTML, and safe raster workspace artifacts previously shown as download-only may open directly in their existing preview UI. SVG, Office documents, PDF, audio, and video continue to require download. Integrations that intentionally relied on a broad document kind to force download-only behavior should instead use a download-only file extension; supported text and raster formats now follow their actual path or normalized MIME type.

Linked Issues

N/A

中文说明

本 PR 做了什么

Web Shell 现在会先根据 workspace 路径或规范化后的 MIME 类型识别 Markdown、HTML 和安全栅格图片,再处理宽泛的 document 分类。下载路由与最终渲染器选择都会统一规范化 charset=utf-8 等 MIME 参数。SVG 仍然仅下载,已知的 Office、PDF、视频和音频扩展名优先于冲突的可预览 MIME 元数据。

为什么需要

生成的 artifact 即使实际是 Markdown、HTML 或栅格图片,也可能被记录为宽泛的 document kind。此前按 kind 优先的路由会用仅下载卡片替换这些已支持的预览。本次修改恢复已有的格式专用预览,同时让包含主动内容的 SVG 以及二进制文档和媒体格式继续走更安全的下载路径。

Reviewer Test Plan

如何验证

打开被报告为 document.md.markdown.html.htm 和 PNG workspace artifact,确认它们分别渲染为 Markdown、沙箱 HTML 和图片。再使用无扩展名且 MIME 带有 charset=utf-8 等参数的 Markdown、HTML 和 PNG artifact,确认使用相同预览。打开 SVG、DOCX、XLSX、PDF 和 MP4 artifact,包括 MIME 元数据伪装成可预览文本或图片的情况,确认它们保持仅下载,并且不会把 workspace 文件读入文本或图片渲染器。

证据(修改前后)

修改前:被报告为 document 的 Markdown、HTML 和栅格文件只显示下载详情。被报告为 image 的 SVG 可能落入源码预览,可预览的冲突 MIME 还可能覆盖仅下载扩展名。

修改后:支持的文本和栅格格式使用已有预览;SVG 与已知二进制文档和媒体扩展名会安全地进入下载详情。新增行为测试在旧 main 实现上失败,在本次修改后通过。

测试平台

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

环境(可选)

Node.js 22;Web Shell Vitest 测试、生产构建和 TypeScript 类型检查。

风险与范围

  • 主要风险或权衡:当 workspace artifact 的宽泛 kind 与路径或 MIME 元数据冲突时,格式路由优先级会发生变化;正向预览用例和安全失败的冲突用例均已覆盖。
  • 未验证或不在范围内:PDF、Word、Excel、PowerPoint、音频、视频的原生预览以及 SVG 渲染仍不在本 PR 范围内。Windows 和 Linux 未进行本地测试。
  • 破坏性变化或迁移说明:无需迁移 API、schema、配置、持久化数据或依赖。升级后,原先只显示下载的 Markdown、HTML 和安全栅格 workspace artifact 可能会直接进入已有预览 UI。SVG、Office 文档、PDF、音频和视频仍需下载。若集成方此前刻意利用宽泛的 document kind 强制仅下载,应改用下载专用扩展名;受支持的文本和栅格格式现在会按照实际路径或规范化 MIME 类型处理。

关联 Issue

@ytahdn

ytahdn commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator Author

E2E test report

Tested the final branch after rebasing onto the latest upstream main.

  • Document-classified Markdown and HTML artifacts, including extensionless files with MIME parameters, reached their rendered Markdown and sandboxed HTML previews.
  • Document-classified PNG and WebP controls remained previewable.
  • SVG artifacts were download-only for path-based, MIME-only, and conflicting MIME cases.
  • All 22 download-only Office, PDF, video, and audio extensions remained download-only when alternately paired with image/png and text/html MIME metadata; no bypasses were observed.
  • Component-level conflict cases did not read workspace text, create a source editor or iframe, and displayed the download detail.
  • The focused Web Shell suite passed 91/91 tests. The Web Shell production build and TypeScript typecheck passed on macOS with Node.js 22.

The pre-fix implementation was independently replayed in a temporary tracked-tree copy: the new Markdown/HTML behavior tests failed because the files were routed to download-only, while an image-classified SVG fell through to source preview. No user workspace sample files were modified or included.

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: This is an observed routing bug, not a theoretical one. On main, isDownloadOnlyWorkspaceArtifact returns true unconditionally for kind === 'document', so a Markdown, HTML, or PNG workspace artifact recorded under the broad document kind renders only as a download card; symmetrically, kind === 'image' returns false unconditionally, letting an image-classified SVG fall through to a source preview. I confirmed both paths in the current code, and the added tests target exactly these cases.

Direction: Aligned — this restores the format-specific previews Web Shell already has (Markdown renderer, CSP-sandboxed HTML iframe, image viewer) instead of replacing them with a download card, and the fail-closed treatment of SVG, Office, PDF, and media formats matches the existing download-only policy. CHANGELOG (claude-code): no direct reference, but the area is Web Shell's own artifact-preview surface.

Size: Not applicable — no core paths. All 4 files are in packages/web-shell/client/components/artifacts/: 42 production lines (additions + deletions) vs 113 test lines.

Approach: The scope feels right. The new precedence — download-only extensions and SVG first, then recognized previewable path/MIME, then the broad kind — is the minimal change that fixes the misrouting without adding any new renderer, and reusing getArtifactImageMimeType keeps the raster-image allowlist in one place. No unrelated edits in the diff. One natural question: fixing the kind at recording time instead of recognizing the format at render time would also address the root cause, but that would not cover artifacts already recorded or extensionless files with only MIME metadata, so the renderer-side fix stands on its own.

Risk: No elevated risk signals — no high-risk paths matched. The widened HTML preview surface is the pre-existing sandbox="allow-scripts" iframe with the CSP wrapper; this PR routes more artifacts into that existing mechanism without changing it.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:这是一个已观测到的路由 bug,不是理论问题。在 main 上,isDownloadOnlyWorkspaceArtifactkind === 'document' 无条件返回 true,因此以宽泛 document kind 记录的 Markdown、HTML 或 PNG workspace artifact 只会显示下载卡片;对称地,kind === 'image' 无条件返回 false,会让 image 分类的 SVG 落入源码预览。我在当前代码中确认了这两条路径,新增测试正是针对这些情况。

方向:对齐——本 PR 恢复 Web Shell 已有的格式专用预览(Markdown 渲染器、CSP 沙箱 HTML iframe、图片查看器),而不是用下载卡片替换它们;SVG、Office、PDF、媒体格式的安全失败(仅下载)处理与现有仅下载策略一致。CHANGELOG(claude-code):无直接引用,但该领域属于 Web Shell 自身的 artifact 预览面。

规模:不适用——不涉及核心路径。4 个文件均在 packages/web-shell/client/components/artifacts/:42 行生产代码(增+删),113 行测试。

方案:范围合理。新的优先级——仅下载扩展名与 SVG 优先,其次可识别的可预览路径/MIME,最后才是宽泛 kind——是修复误路由的最小改动,且复用 getArtifactImageMimeType 让栅格图片白名单保持在一处,diff 中没有无关改动。一个自然的问题:也可以在记录时修正 kind 而不是在渲染时识别格式,但那无法覆盖已记录的 artifact 或只有 MIME 元数据的无扩展名文件,因此渲染侧修复本身成立。

风险:无升级风险信号——未命中高风险路径。被拓宽的 HTML 预览面是既有的 sandbox="allow-scripts" iframe 加 CSP 包装;本 PR 只是把更多 artifact 路由进这一既有机制,并未改动它。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

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

No blocking findings. Posting as COMMENT because Test (ubuntu-latest, Node 22.x) is still pending — will approve once it lands green.

What this changes:

The rewrite of isDownloadOnlyWorkspaceArtifact establishes a clear priority order:

  1. SVG (by extension or MIME) + known download-only extensions → always download — extension-based, cannot be overridden by MIME metadata.
  2. Recognized image formats, Markdown, and HTML → always preview — overrides the broad document kind that was previously blocking these.
  3. Broad kind classifiers (image/document/pdf/video/audio) → download — fallback for unrecognized formats.
  4. Everything else → not download-only (extensionless, unknown MIME).

Checked:

  • DOWNLOAD_ONLY_EXTENSIONS supersedes isOfficeDocumentPath: DOWNLOAD_ONLY_EXTENSIONS = [...OFFICE_DOCUMENT_EXTENSIONS, '.pdf', '.mp4', …] is a strict superset, so the removal of isOfficeDocumentPath from the kind-fallback block loses no coverage.
  • SVG safeguard is bidirectional: {path:'graphic.svg', mimeType:'text/html'} → extension fires at Priority 1. {path:'safe.png', mimeType:'image/svg+xml'} → MIME fires at Priority 1. Neither can be overridden by the other side.
  • Conflict tests match code: report.docx + text/html MIME.docx in DOWNLOAD_ONLY_EXTENSIONS → download-only before the previewable MIME check. Same for .xlsx, .pdf, .mp4. ✓
  • MIME parameter stripping: normalizeArtifactMimeType('text/html; charset=utf-8')'text/html'. Both isHtmlArtifact and isMarkdownArtifact now handle charset params — the old artifact.mimeType?.toLowerCase() === 'text/markdown' silently failed for parameterized MIME. ✓
  • kind='image' change: previously all image artifacts bypassed download-only. Now only recognized image formats (avif/bmp/gif/ico/jpeg/png/webp — both by MIME and by path extension) reach Priority 2's false; unrecognized formats fall to Priority 3 → download-only. Conservative and intentional.
  • ArtifactPanel.tsx caller (line 2432): passes a full DaemonSessionArtifact which includes mimeType — the new parameter is wired through. ✓
  • Test non-vacuity: old code returned true for kind==='document' unconditionally, so the new previews document-classified Markdown/HTML tests would fail at the readWorkspaceFile assertion with the old implementation. ✓

Not covered: macOS and Windows CI SKIPPING (fork-PR pattern). The changed logic is pure in-memory classification with no platform-dependent paths.

Reviewed with AI assistance.

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

Reviewed against main. The routing change is correct and fails closed:

  • Download-only extensions (Office, PDF, media) and SVG — by extension and by image/svg+xml MIME — are checked first, so a previewable MIME claim can never override them. The removed isOfficeDocumentPath branch is fully subsumed by DOWNLOAD_ONLY_EXTENSIONS (same extension set), so no coverage is lost.
  • The previewable branch reuses getArtifactImageMimeType, keeping the raster-image allowlist in one place; .md/.markdown/.html/.htm paths and normalized text/markdown/text/html MIME join it ahead of the broad-kind fallback.
  • normalizeArtifactMimeType is the right fix for the charset=utf-8 comparisons in isHtmlArtifact/isMarkdownArtifact, and reusing it inside getArtifactImageMimeType removes the duplicated inline parsing instead of adding a third copy.
  • One intentional tightening beyond the headline fix: an image-classified artifact whose MIME is not in the raster allowlist (e.g. image/tiff) now lands on download-only instead of previewing. That matches the stated fail-closed intent, and the HTML preview surface being widened is the pre-existing sandbox="allow-scripts" iframe + CSP wrapper — unchanged here.

No critical issues. Two non-blocking notes:

  1. isOfficeDocumentPath loses its only production caller in this diff — only its own unit test references it now, so it is dead production code. Fine to remove in this PR or a follow-up.
  2. The author reports the new behavior tests fail against pre-fix main and pass with the change, and that rendering was verified end-to-end on macOS — that is the author's claim, not independently re-run here (see the sandboxed-lane note below).

Test evidence (PR's own CI via API — this unattended run never executes PR code)

Final CI results for bc52018 (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
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

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

No failures so far. The two checks that matter most for this PR — the unit suite and the web-shell visual capture — are still running; the Qwen Triage Finalize job rewrites the table above once CI settles. (The in-flight triage / review-pr entries on this commit are bot orchestration, not PR CI.)

Sandboxed verification would settle what CI cannot: @qwen-code /verify — that the new behavior tests actually fail against pre-fix main, which is the load-bearing claim behind "this restores previews", and that the routing delta is what the tests pin; @qwen-code /tmux — for the artifact-panel preview surface itself. The author has write access, so either lane can be triggered directly; until then the end-to-end rendering evidence is the author's self-report from macOS only, not independently re-run.

中文说明

代码审查

对照 main 审查。路由改动正确且安全失败(fail-closed):

  • 仅下载扩展名(Office、PDF、媒体)和 SVG——按扩展名 image/svg+xml MIME——优先检查,可预览的 MIME 声明永远无法覆盖它们。被移除的 isOfficeDocumentPath 分支完全被 DOWNLOAD_ONLY_EXTENSIONS(同一扩展名集合)包含,没有丢失覆盖。
  • 可预览分支复用 getArtifactImageMimeType,栅格图片白名单保持在一处;.md/.markdown/.html/.htm 路径与规范化后的 text/markdown/text/html MIME 在宽泛 kind 兜底之前加入。
  • normalizeArtifactMimeType 是修复 isHtmlArtifact/isMarkdownArtifactcharset=utf-8 比较的正确做法,并在 getArtifactImageMimeType 内复用,消除了重复的内联解析而不是新增第三份。
  • 除标题修复外的一处有意收紧:MIME 不在栅格白名单内的 image 分类 artifact(如 image/tiff)现在走仅下载而不是预览。与声明的失败关闭意图一致;被拓宽的 HTML 预览面是既有的 sandbox="allow-scripts" iframe + CSP 包装——此处未改动。

无严重问题。两条非阻塞备注:

  1. isOfficeDocumentPath 在本 diff 后失去了唯一的生产调用方——现在只有它自己的单元测试引用它,已是生产死代码。可在本 PR 或后续移除。
  2. 作者报告新行为测试在修复前的 main 上失败、在本改动后通过,且渲染已在 macOS 上端到端验证——这是作者的自述,未在此独立复跑(见下方沙箱验证说明)。

测试证据(PR 自身 CI,通过 API 获取——无人值守运行绝不执行 PR 代码)

目前无失败(表格见上方英文部分)。对本 PR 最关键的两项检查——单元测试套件与 web-shell 视觉捕获——仍在运行;Qwen Triage Finalize 任务会在 CI 落定后更新表格。(此提交上进行中的 triage / review-pr 是机器人编排任务,不是 PR CI。)

沙箱验证可以补足 CI 无法覆盖的部分:@qwen-code /verify——验证新行为测试在修复前的 main 上确实失败(这是"恢复预览"结论的关键依据),以及测试钉住的正是路由差异;@qwen-code /tmux——针对 artifact 面板预览面本身。作者有 write 权限,两条通道都可直接触发;在此之前,端到端渲染证据仍是作者在 macOS 上的自述,未独立复跑。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — a small, correct, fail-closed routing fix with test coverage on both directions of the new precedence; only nits are the dead isOfficeDocumentPath helper and the unit suite still in flight.

Stepping back: I would have written this almost exactly the same way. The precedence order — download-only formats first, then recognized previewable formats, then the broad kind — is the minimal change that fixes the misrouting, and reusing getArtifactImageMimeType plus one small normalizer avoids duplicating format knowledge. I verified the bug against main rather than taking the PR's framing on faith: kind === 'document' really did force a download card on Markdown/HTML/PNG workspace files, and an image-classified SVG really could fall through to a source preview. Every line in the diff serves the stated goal — no drive-by changes — and the one intentional behavior shift beyond the headline fix (image-classified artifacts with a non-raster MIME now go download-only) is a tightening in the safe direction. Fixing the kind at recording time remains a worthwhile follow-up, but it would not cover artifacts already recorded or extensionless files identified by MIME alone, so this renderer-side fix stands on its own. In six months this routing function will still read cleanly.

Approval is deferred until CI lands green on bc5201836643a30ff882ccadef81bdaa0a0f7618 — the unit suite and the web-shell visual capture are still running, and I won't attest to a result that doesn't exist yet.

中文说明

置信度:4/5 —— 一个小而正确、安全失败(fail-closed)的路由修复,对新优先级的两个方向都有测试覆盖;仅有的小瑕疵是成为死代码的 isOfficeDocumentPath,以及仍在运行的单元测试套件。

退一步看:我自己大概也会写成几乎一样的样子。优先级顺序——仅下载格式优先,其次是可识别的可预览格式,最后才是宽泛 kind——是修复误路由的最小改动;复用 getArtifactImageMimeType 加一个小的规范化函数,避免了格式知识的重复。我没有轻信 PR 的表述,而是对照 main 验证了 bug:kind === 'document' 确实会把 Markdown/HTML/PNG workspace 文件强制变成下载卡片,image 分类的 SVG 也确实可能落入源码预览。diff 中每一行都服务于既定目标——没有夹带改动——标题修复之外唯一有意的行为变化(MIME 非栅格格式的 image 分类 artifact 现在走仅下载)是向更安全方向的收紧。在记录时修正 kind 仍是值得做的后续工作,但它无法覆盖已记录的 artifact 或仅靠 MIME 识别的无扩展名文件,因此这个渲染侧修复本身成立。六个月后这个路由函数读起来依然清晰。

审批推迟到 CI 在 bc5201836643a30ff882ccadef81bdaa0a0f7618 上变绿之后——单元测试套件与 web-shell 视觉捕获仍在运行,我不会为尚不存在的结果背书。

Qwen Code · qwen3.8-max

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

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

静态/diff 审查,head bc52018(未本地运行测试)。

中文:

总体结论:未发现阻塞性问题。isDownloadOnlyWorkspaceArtifact 重写后的三层判定顺序清晰、fail-closed 在前,charset 归一化在路由与渲染器两侧一致落地。以下两条为确认性/清理性备注。

💡 isOfficeDocumentPath 在生产代码中已无调用点
新的第一层判定用 DOWNLOAD_ONLY_EXTENSIONS(本身即 OFFICE_DOCUMENT_EXTENSIONS 的严格超集,另含 .pdf 与媒体扩展)取代了原先 kind 兜底分支里的 isOfficeDocumentPath(...) 调用,覆盖面没有损失——这一点与测试矩阵中 report.docx + text/htmlreport.xlsx + image/png 等"扩展名压制可预览 MIME"的用例一致。但该函数目前只剩 artifactUtils.test.ts 引用它;若不打算保留为公共工具,可在后续顺手清理或注明保留原因。非阻塞。

💡 下载路径保留原始 MIME(确认,无需修改)
DownloadableWorkspaceArtifact 的下载流程把未归一化的 artifact.mimeType 原样传给 downloadWorkspaceFile,后者仅将其用作 new Blob(chunks, { type: mimeType }) 的类型。这是合理的:Blob type 允许携带 charset 等参数,而所有路由/渲染判定(isDownloadOnlyWorkspaceArtifactisHtmlArtifactisMarkdownArtifactgetArtifactImageMimeType)均已统一走 normalizeArtifactMimeType。PR 描述中"MIME 参数在下载路由与渲染器选择中被一致归一化"的说法经核实成立。

🎉 值得肯定的点

  • 冲突优先级设计:.svg 扩展名 / image/svg+xml MIME / 下载类扩展名置于第一层,任何可预览 MIME 都无法反向覆盖(graphic.svg + text/htmlimage kind + graphic.svg + image/png 等六行冲突用例两个方向都钉住了)。
  • 行为收敛是有意的:kind: 'image' 但 MIME/扩展名均不在白名单时,由旧实现的降级预览收敛为仅下载,与 SVG 策略一致,保守且合理。
  • getArtifactImageMimeType 参数收窄为 Pick<DaemonSessionArtifact, 'mimeType' | 'workspacePath'>,纯派生、无副作用。
  • 渲染器一致性:ArtifactPanel 的 download-only 闸门先于 previewKind 选择执行,冲突 MIME 到不了文本/图片渲染器;新增的 charset MIME 预览测试(text/markdown; charset=utf-8 等)与既有 mock 结构一致,且在旧实现下会于 readWorkspaceFile 断言处失败,非空转测试。

CI 状态:Test (ubuntu-latest, Node 22.x) 与 web-shell 视觉捕获仍在运行,Desktop Shell 双平台已通过,macOS/Windows 测试按 fork 惯例跳过(改动为纯内存分类逻辑,无平台相关路径)。待 CI 全绿后可按惯例批准。此前一轮机器辅助审查亦未发现阻塞项。


English:

Static/diff-based review at head bc52018 (tests not run locally).

Overall: no blocking findings. The three-tier rewrite of isDownloadOnlyWorkspaceArtifact has a clear, fail-closed-first ordering, and charset normalization lands consistently on both the routing and renderer sides. Two confirmatory/cleanup notes follow.

💡 isOfficeDocumentPath has no remaining production callers
The new first tier uses DOWNLOAD_ONLY_EXTENSIONS (a strict superset of OFFICE_DOCUMENT_EXTENSIONS, plus .pdf and the media extensions) in place of the old isOfficeDocumentPath(...) call in the kind-fallback branch, so no coverage is lost — consistent with the "extension overrides previewable MIME" test rows such as report.docx + text/html and report.xlsx + image/png. The function itself is now referenced only by artifactUtils.test.ts; if it isn't being kept as a shared utility, it could be cleaned up or annotated in a follow-up. Non-blocking.

💡 Download path keeps the raw MIME (confirmed, no change needed)
The DownloadableWorkspaceArtifact download flow passes the unnormalized artifact.mimeType straight to downloadWorkspaceFile, which uses it only as the new Blob(chunks, { type: mimeType }) type. That is fine: Blob types may carry parameters such as charset, while every routing/renderer decision (isDownloadOnlyWorkspaceArtifact, isHtmlArtifact, isMarkdownArtifact, getArtifactImageMimeType) now goes through normalizeArtifactMimeType. The PR description's claim that MIME parameters are normalized consistently through download routing and renderer selection holds up on inspection.

🎉 Highlights

  • Conflict precedence: .svg extension / image/svg+xml MIME / download-only extensions sit in tier one, so no previewable MIME can override them — the six SVG conflict rows and four extension-over-MIME rows lock down both directions.
  • Intentional narrowing: kind: 'image' artifacts whose MIME/extension are both unrecognized now fall to download-only instead of the old degraded preview — conservative and consistent with the SVG posture.
  • getArtifactImageMimeType narrowed to Pick<DaemonSessionArtifact, 'mimeType' | 'workspacePath'> — pure derivation, no side effects.
  • Renderer consistency: the ArtifactPanel download-only gate runs before previewKind selection, so conflicting MIME can never reach the text/image renderers; the new charset-MIME preview tests match the existing mock structure and would fail at the readWorkspaceFile assertion under the old implementation, so they are not vacuous.

CI: Test (ubuntu-latest, Node 22.x) and the web-shell visual capture are still running; Desktop Shell passed on both platforms, and macOS/Windows tests are skipped per the fork convention (the change is pure in-memory classification with no platform-dependent paths). Once CI is green this can be approved as usual; a prior AI-assisted round also found no blockers.

@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 bc52018. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

⚠️ No preview: one or more scenarios failed to render on this head — see the workflow run. This is not "no visual change" — a scenario that times out or throws produces no image. Fix the failing scenario (or a genuine regression it caught) and the preview returns on the next push.

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

Qwen Code · web-shell visuals

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@wenshao

wenshao commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

✅ Maintainer local verification: merge-ready

147/147 executed assertions passed at bc5201836643a30ff882ccadef81bdaa0a0f7618.

中文摘要

结论:建议合并。 使用精确 base ec8a8a1a97ff1f2f60a9fc4dce4d6a7c1ef63d23 与 PR head bc5201836643a30ff882ccadef81bdaa0a0f7618 做了真实构建和 A/B。相同的 91 项定向测试在 head 为 91/91,通过;在 base 为 74/91,17 个新增行为断言按预期失败,证明修改是有效且不可被现有行为替代的。独立路由矩阵在 head 为 56/56,在 base 为 48/56;Markdown、HTML、栅格图、SVG 和 MIME 归一化的 8 个关键格发生预期翻转。全部 22 种 Office/PDF/音视频下载专用扩展分别与 text/htmlimage/png 冲突 MIME 组合,共 44/44 个安全边界断言通过。

未发现阻塞问题。Web Shell 生产构建、TypeScript typecheck、ESLint 和 git diff --check 均通过。未覆盖浏览器内人工视觉检查、Windows/Linux 原生环境,以及 PDF/Office/音视频/SVG 的原生预览(本 PR 明确保留为仅下载)。

Central claim

Web Shell must recover existing Markdown, sandboxed HTML, and safe raster-image previews when an artifact is broadly classified as document, while SVG and known Office/PDF/audio/video paths continue to fail closed to download-only even when MIME metadata conflicts. MIME parameters must be normalized consistently in both the download-routing decision and the final renderer selection.

A/B load-bearing proof

The identical PR test specifications were run against the exact base production source and the PR head production source. On the base arm only the two changed test files were overlaid; the implementation remained at the base OID.

Cell Production source Test specification Observable result
Head bc5201836643a30ff882ccadef81bdaa0a0f7618 PR tests 91/91 passed across 2 files
Control ec8a8a1a97ff1f2f60a9fc4dce4d6a7c1ef63d23 Same PR tests 74/91 passed; 17 expected failures

The 17 base-only failures are load-bearing: 10 Markdown/HTML routing assertions, 2 raster-image routing assertions, and 5 SVG fail-closed assertions. No unrelated test in the two focused files regressed on head.

A/B: identical PR tests against base and head

Independent routing matrix

A separate harness imported the real artifactUtils.ts from each arm and did not reuse Vitest fixtures. It exercised uppercase extensions, extensionless artifacts, parameterized/case-varied MIME values, SVG path/MIME conflicts, unknown documents, and every download-only extension paired with both previewable HTML and PNG MIME metadata.

Cell Result Key observation
Head 56/56 passed All supported preview and fail-closed cases matched the expected route
Control 48/56 passed 8 expected mechanism cells remained broken before this PR

The head safety boundary includes 44/44 conflict assertions: 22 Office/PDF/audio/video extensions × text/html and image/png. The component path was also audited: isDownloadOnlyWorkspaceArtifact() is evaluated before WorkspaceFilePreview, so SVG and binary conflicts do not reach text/image reads; final Markdown/HTML selection uses the same normalizeArtifactMimeType() helper.

Independent routing matrix: safe previews and fail-closed conflicts

Findings

No blocking or non-blocking correctness findings.

Targeted gates

  • Web Shell production build: passed after explicitly building its @qwen-code/webui workspace dependency.
  • Web Shell TypeScript typecheck: passed.
  • Web Shell ESLint with --max-warnings 0: passed.
  • Focused artifact tests: 91/91 passed on head.
  • git diff --check: passed.

The first full npm ci prepare attempt failed identically on base and head because Docker Desktop's macOS bind mount denied a recursive copy into packages/cli/dist/src/commands/extensions/examples/mcp-server. This occurred in an unrelated CLI build step after dependency installation and is not attributed to the PR. The affected Web Shell workspace and its direct internal dependency were then built and verified symmetrically.

Not covered

  • Manual browser visual inspection of rendered Markdown, HTML iframe content, and raster pixels.
  • Native Windows and Linux host builds; verification used Linux node:22-bookworm containers on macOS Docker Desktop.
  • Native preview implementations for PDF, Office documents, audio, video, or SVG; this PR intentionally keeps those paths download-only.
  • The complete repository test suite and unrelated workspaces.

Methodology

GitHub metadata resolved base ec8a8a1a97ff1f2f60a9fc4dce4d6a7c1ef63d23 and head bc5201836643a30ff882ccadef81bdaa0a0f7618; fetched merge ref 55834e6bdd7113188c3582f1935be231a6706a91 had those exact parents. PR code was executed only inside credential-free Docker containers with no host SSH, cloud, or GitHub-token mounts. Separate detached worktrees held head/merge and base, and each had its own lockfile installation and generated workspace outputs. Raw build, typecheck, lint, Vitest, and matrix logs plus the independent harness are retained locally under tmp/pr9760-verify-20260823-015340/.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

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

Comment on lines +104 to +105
mimeType === 'text/markdown' ||
mimeType === 'text/html'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The MIME normalization this PR introduces stops at the panel: isRenderedArtifact in TurnOutputs.tsx (~673) still compares the raw artifact.mimeType?.toLowerCase() against 'text/markdown'/'text/html' without stripping parameters, so it now disagrees with the normalized predicates here and in isHtmlArtifact/isMarkdownArtifact for exactly the artifacts this PR makes previewable. A document artifact with mimeType: 'text/markdown; charset=utf-8' and an extensionless path (reports/preview — the exact shape of this PR's own new test) renders as Markdown in the panel, but getArtifactPreviewContent returns undefined for it, so the turn's captured-content fast path is dropped: it first paints the "Loading preview..." placeholder instead of the instantly-rendered captured content, and if the workspace file was removed after the turn, the plain-MIME twin still shows the captured content while the charset variant shows only a load error (verified by probe at this commit: CHARSET getArtifactPreviewContent => undefined vs PLAIN => "# Captured"; with the fix below the charset variant returns the captured content, and TurnOutputs tests stay 19/19 green). Format classification now lives in three unsynchronized copies across two files; at minimum route the TurnOutputs comparison through the new helper (the file already imports from ./artifactUtils):

// packages/web-shell/client/components/artifacts/TurnOutputs.tsx — isRenderedArtifact
const mimeType = normalizeArtifactMimeType(artifact.mimeType);

Ideally extract one shared preview-format classifier in artifactUtils.ts consumed by the gate, the panel's previewKind computation, and TurnOutputs.

中文说明

本 PR 引入的 MIME 规范化止步于面板:TurnOutputs.tsx(约 673 行)中的 isRenderedArtifact 仍然用原始的 artifact.mimeType?.toLowerCase()'text/markdown'/'text/html' 比较,没有去掉参数,因此对于本 PR 恰好变为可预览的那些 artifact,它与此处以及 isHtmlArtifact/isMarkdownArtifact 中已规范化的判断出现了分歧。一个 mimeType: 'text/markdown; charset=utf-8'、无扩展名路径(reports/preview —— 正是本 PR 新增测试所用的形态)的 document artifact 在面板中能渲染为 Markdown,但 getArtifactPreviewContent 对它返回 undefined,导致 turn 快照内容的快速路径被丢弃:首屏只能显示 "Loading preview..." 占位而不是立即渲染的快照内容;如果 workspace 文件在 turn 之后被删除,纯 MIME 的同类 artifact 仍能显示快照内容,而带 charset 的变体只显示加载错误(已在本提交上用探针验证:CHARSET getArtifactPreviewContent => undefined,而 PLAIN => "# Captured";应用下面的修复后,charset 变体也能返回快照内容,TurnOutputs 测试保持 19/19 全绿)。格式分类现在有两份文件中的三处不同步副本;至少应让 TurnOutputs 的比较走新的辅助函数(该文件已从 ./artifactUtils 导入)。更理想的做法是在 artifactUtils.ts 中提取一个共享的可预览格式分类器,由 gate、面板的 previewKind 计算和 TurnOutputs 共同使用。

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

Comment on lines +92 to +94
extension === '.svg' ||
mimeType === 'image/svg+xml' ||
DOWNLOAD_ONLY_EXTENSIONS.has(extension)

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] This diff removed the only production caller of the exported isOfficeDocumentPath — the gate now reads DOWNLOAD_ONLY_EXTENSIONS directly (a superset of the office set), and a repo-wide grep finds the helper referenced only by its definition and its own test. It is dead production code that still looks authoritative (the adjacent "Keep in sync" comment), so a future office-policy edit to isOfficeDocumentPath would be a silent no-op against the gate, with the test giving false comfort that the API is in use. Consider deleting isOfficeDocumentPath and its test assertion in this same change (OFFICE_DOCUMENT_EXTENSIONS stays — DOWNLOAD_ONLY_EXTENSIONS still uses it), or routing the gate through the helper if you want to keep it.

中文说明

本 diff 移除了导出函数 isOfficeDocumentPath 唯一的生产调用方——现在 gate 直接读取 DOWNLOAD_ONLY_EXTENSIONS(它是 office 集合的超集),全仓 grep 显示该辅助函数只被其定义和自身测试引用。它是看起来仍像权威接口(旁边还有 "Keep in sync" 注释)的死代码:未来对 isOfficeDocumentPath 的 office 策略修改对 gate 将是静默无效的操作,而其测试会给人一种该 API 仍在使用的虚假安慰。建议在本次改动中一并删除 isOfficeDocumentPath 及其测试断言(OFFICE_DOCUMENT_EXTENSIONS 保留——DOWNLOAD_ONLY_EXTENSIONS 仍在使用它),或者如果想保留它,就让 gate 经由该辅助函数判断。

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

Comment on lines +110 to +114
artifact.kind === 'image' ||
artifact.kind === 'document' ||
artifact.kind === 'pdf' ||
artifact.kind === 'video' ||
artifact.kind === 'audio' ||
isOfficeDocumentPath(artifact.workspacePath)
artifact.kind === 'audio'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The kind-fallback branch has no effective test: every kind-image row in the new SVG cases also carries a .svg extension (decided earlier by the extension check), every kind-document row resolves via the previewable MIME/extension check above, and the one component-level download-only test uses reports/q3.xlsx, caught by DOWNLOAD_ONLY_EXTENSIONS first. Deleting artifact.kind === 'image' || or artifact.kind === 'document' || leaves the whole artifact suite green — verified by mutation at this commit (91/91 pass with each mutant, and probes confirm both are live: {kind: 'image', workspacePath: 'photo.heic'} and {kind: 'document', workspacePath: 'notes.txt'} flip true → false). If one of those lines ever drops, such artifacts silently flip from download-only to a raw source preview. Rows decided by kind alone would pin the branch:

['image', 'photo.heic', undefined],
['image', 'photo', undefined],
['document', 'notes.txt', undefined],
中文说明

kind 兜底分支没有有效的测试:新增 SVG 用例中所有 kind 为 image 的行都带有 .svg 扩展名(已被前面的扩展名检查判定),所有 kind 为 document 的行都经由上面的可预览 MIME/扩展名检查得出结果,而唯一的组件级仅下载测试使用 reports/q3.xlsx,会先被 DOWNLOAD_ONLY_EXTENSIONS 命中。删除 artifact.kind === 'image' ||artifact.kind === 'document' || 后,整个 artifact 测试套件仍然全绿——已在本提交上做变异验证(每个变异体下 91/91 全部通过,且探针确认两者都是活变异体:{kind: 'image', workspacePath: 'photo.heic'}{kind: 'document', workspacePath: 'notes.txt'} 会由 true 翻转为 false)。一旦其中某行被误删,这类 artifact 会静默地从仅下载变成原始 source 预览。增加仅由 kind 判定的用例行即可钉住该分支。

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

getImageMimeTypeFromPath,
getReviewDownloadMimeType,
isDownloadOnlyWorkspaceArtifact,
normalizeArtifactMimeType,

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] AttachmentBlobPreview in this same file (~2756) still computes its MIME type with an inline copy of the strip-parameters/trim/lowercase pipeline this PR consolidates into normalizeArtifactMimeType — after this change a repo-wide grep finds exactly two normalization sites: the new helper and that inline copy. This PR exists because two divergent normalizations routed the same artifact differently; leaving the attachment copy inline means the next normalization change can miss it and the same drift class reappears for attachment previews. The existing fallback composes cleanly with the helper:

const resolvedMimeType =
  normalizeArtifactMimeType(mimeType || data.type) || 'application/octet-stream';
中文说明

同文件中的 AttachmentBlobPreview(约 2756 行)仍在用一份内联的"去参数/trim/小写"流水线计算 MIME 类型,而本 PR 已将该流水线收敛为 normalizeArtifactMimeType——改动后全仓 grep 只有两处规范化:新的辅助函数和那份内联副本。本 PR 的起因正是两种不一致的规范化把同一个 artifact 路由到了不同结果;保留内联副本意味着下次修改规范化时可能漏掉它,同样的漂移问题会在附件预览上重演。现有的兜底值可以与辅助函数自然组合。

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

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

Copy link
Copy Markdown
Collaborator

Released in v0.22.2.

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.

4 participants