Skip to content

feat(generated-ui): check a view's initial calculation before display - #15790

Merged
qqqys merged 1 commit into
QwenLM:mainfrom
qqqys:feat/generated-ui-script-check
Oct 28, 2026
Merged

qqqys merged 1 commit into
QwenLM:mainfrom
qqqys:feat/generated-ui-script-check

Conversation

@qqqys

@qqqys qqqys commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Before returning a snapshot, the bundled Generated UI server now runs the generated JavaScript's initialization once in the same bounded QuickJS interpreter the view uses, with a stand-in ui API and the markup's initial input values. Syntax errors, exceptions from the initial calculation, initial updates to element IDs that do not exist, and markup the view would reject (duplicate or invalid IDs, controls without IDs, unsupported input types, multiple selects) return a VIEW_ERROR tool result that names the problem, with line and column where available, so the model corrects the document instead of reporting a broken card as ready.

In the card, ordinary script errors are named (for example Script error: ReferenceError: 'total' is not defined (line 3, column 7).) instead of the generic "Generated JavaScript failed or exceeded a runtime limit". The description reads only a native error's own data properties, so guest getters, toString hooks and Proxies never run; resource exhaustion keeps the generic message. When a view fails because of its document, the card offers Ask the assistant to fix this view, which sends the error to the session through the existing host message review. The writer skill documents the check and notes that Intl is unavailable and toLocaleString ignores locales and options.

Why it's needed

In a real Web Shell run (qwen3.8-max), a subscription comparison card failed with only "Generated JavaScript failed or exceeded a runtime limit." The generated script had one extra ), the tool result reported success, and the model told the user "The interactive comparison card is ready". Nothing checked the script before display, the error did not say what was wrong, and the model never learned about it. Silent formatting differences (toLocaleString with options) produced wrong amounts without any error.

Reviewer Test Plan

How to verify

  1. Build the package (npm run build --workspace=@qwen-code/generated-ui) and call generated_ui_show on dist/bundled-main.js over stdio with a document whose JavaScript has a syntax error, references an undefined name in its initial calculation, or calls ui.text on an ID that is not in the markup. Each returns isError: true with a VIEW_ERROR: text naming the problem; a valid document returns the snapshot as before.
  2. In Web Shell with experimental.generatedUi on, a view whose event handler throws shows Script error: … in the card status and enables Ask the assistant to fix this view; clicking it opens the usual message review with the error text.
  3. The README and SKILL.md examples still pass the new check (covered by check.test.ts).

Evidence (Before & After)

The JavaScript from the failing real run, sent to generated_ui_show on the built server over stdio:

Before (main) After
Tool result isError: false, snapshot returned; the card later shows "Generated JavaScript failed or exceeded a runtime limit." isError: true, VIEW_ERROR: Script error: SyntaxError: expecting ',' (line 1, column 439). Nothing was displayed. Correct the document and call generated_ui_show again with all five fields.

Tests: integrations/generated-ui 227 passed (new check.test.ts, runtime error naming including hostile thrown objects and redefined error names, the fix action in main.test.ts, and a VIEW_ERROR case in the stdio integration test against the built bundle); scripts/tests/generated-ui-assets.test.js 5 passed; typecheck and lint clean.

Tested on

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

Environment (optional)

Node 24, package build plus the stdio server; the real-run failure came from a local qwen serve bundle with qwen3.8-max.

Risk & Scope

  • Main risk or tradeoff: the server now executes the generated initialization once, inside QuickJS with the same memory, stack and 250 ms limits as the view and no host, file, network or tool access; the README states this instead of "the server never executes the generated source". Embedding the QuickJS module grows the server bundle from about 1.1 MB to 2.0 MB. Initial input values are read from the markup without a DOM, so unusual markup can differ slightly from what the browser starts with; problems that only appear after user input are not detected before display.
  • Not validated / out of scope: theming, the card frame and the Web Shell presentation, which are later steps of feat(web-shell): complete experimental intelligent UI with stateful interactions #14019.
  • Breaking changes / migration notes: documents that the view would have rejected at display time are now rejected at creation with VIEW_ERROR.

Linked Issues

Part of #14019.

中文说明

这个 PR 做了什么

内置 Generated UI 服务器在返回快照之前,会用视图所用的同一个受限 QuickJS 解释器,把生成 JavaScript 的初始化运行一次,使用替身 ui API 和标记中的初始输入值。语法错误、初始计算抛出的异常、初始更新引用了不存在的元素 ID、以及视图会拒绝的标记(重复或非法 ID、没有 ID 的控件、不支持的输入类型、多选 select),都会返回一个指出问题的 VIEW_ERROR 工具结果,能给出行号和列号时会一并给出。这样模型会修正文档,而不是把一张坏掉的卡片报告为已就绪。

在卡片中,普通脚本错误会被具体指出(例如 Script error: ReferenceError: 'total' is not defined (line 3, column 7).),不再只显示笼统的"Generated JavaScript failed or exceeded a runtime limit"。错误描述只读取原生错误对象自身的数据属性,因此不会触发生成代码的 getter、toString 钩子或 Proxy;资源耗尽仍使用笼统提示。当视图因文档本身出错时,卡片提供 请助手修复此视图,通过现有的宿主消息审阅把错误发送到会话。写作技能补充说明了这项检查,并指出 Intl 不可用、toLocaleString 会忽略地区和选项。

为什么需要

在一次真实的 Web Shell 运行(qwen3.8-max)中,订阅对比卡片只显示了"Generated JavaScript failed or exceeded a runtime limit."。生成的脚本多了一个 ),工具结果却报告成功,模型还对用户说"The interactive comparison card is ready"。展示前没有任何检查,错误信息没有说明问题所在,模型也无从得知。带选项的 toLocaleString 这类格式差异则会在没有任何报错的情况下算出错误的金额。

审阅者测试计划

如何验证

  1. 构建该包(npm run build --workspace=@qwen-code/generated-ui),通过 stdio 对 dist/bundled-main.js 调用 generated_ui_show,传入的 JavaScript 分别包含语法错误、在初始计算中引用未定义的名字、或对标记中不存在的 ID 调用 ui.text。每种情况都返回 isError: true,文本以 VIEW_ERROR: 开头并指出问题;合法文档仍像以前一样返回快照。
  2. 在开启 experimental.generatedUi 的 Web Shell 中,事件处理函数抛错的视图会在卡片状态中显示 Script error: …,并启用 请助手修复此视图;点击后打开常规的消息审阅,内容为错误文本。
  3. README 和 SKILL.md 中的示例仍能通过新检查(由 check.test.ts 覆盖)。

证据(修改前后)

把那次真实失败中的 JavaScript,通过 stdio 发给构建后服务器的 generated_ui_show:

修改前(main) 修改后
工具结果 isError: false,返回快照;卡片随后显示"Generated JavaScript failed or exceeded a runtime limit." isError: true,VIEW_ERROR: Script error: SyntaxError: expecting ',' (line 1, column 439). Nothing was displayed. Correct the document and call generated_ui_show again with all five fields.

测试:integrations/generated-ui 227 个通过(新增 check.test.ts;运行时错误命名,包括恶意抛出对象和被改写的错误名;main.test.ts 中的修复操作;以及针对构建产物的 stdio 集成测试中的 VIEW_ERROR 用例);scripts/tests/generated-ui-assets.test.js 5 个通过;类型检查与 lint 通过。

测试平台

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

环境(可选)

Node 24,包构建加 stdio 服务器;真实运行中的失败来自本地 qwen serve bundle 与 qwen3.8-max。

风险与范围

  • 主要风险或取舍:服务器现在会执行一次生成的初始化,运行在 QuickJS 内,内存、栈和 250 ms 限制与视图相同,且没有宿主、文件、网络或工具访问;README 据此替换了"服务器从不执行生成的源码"的说法。嵌入 QuickJS 模块使服务器 bundle 从约 1.1 MB 增至 2.0 MB。初始输入值在没有 DOM 的情况下从标记读取,少数特殊标记可能与浏览器的初始值略有差异;只有在用户输入后才出现的问题无法在展示前发现。
  • 未验证 / 不在范围内:主题、卡片外框和 Web Shell 展示,它们是 feat(web-shell): complete experimental intelligent UI with stateful interactions #14019 的后续步骤。
  • 破坏性变更 / 迁移说明:原本会在展示时被视图拒绝的文档,现在会在创建时以 VIEW_ERROR 拒绝。

关联 Issue

属于 #14019 的一部分。

Run the generated JavaScript's initialization once in the same bounded
QuickJS interpreter before returning a snapshot, using the markup's
initial input values and a stand-in ui API. Syntax errors, exceptions
from the initial calculation, initial updates to missing element IDs
and markup the view rejects now return a VIEW_ERROR tool result that
names the problem, with line and column, so the model can correct the
document instead of reporting a broken card as ready.

The App names ordinary script errors instead of a generic runtime-limit
message, reading only native error data properties so guest getters and
toString hooks never run. When a view fails because of its document, the
card offers to send the error to the session for correction through the
existing host message review.

The writer skill documents the check and that Intl is unavailable and
toLocaleString ignores locales and options.
@qqqys
qqqys enabled auto-merge (squash) October 28, 2026 12:16

@yiliang114 yiliang114 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 of #15790 — feat(generated-ui): check a view's initial calculation before display

Summary

This PR adds pre-flight script checking to generated_ui_show. Before returning a snapshot, the server now runs the generated JavaScript's initialization in the same bounded QuickJS interpreter the view uses, catching syntax errors, runtime exceptions, updates to missing IDs, and unsupported markup — all surfaced as VIEW_ERROR tool results the model can correct.

Security & Correctness

The check runs inside the same bounded QuickJS interpreter with the same 250ms limit and no host, file, network, or tool access. The DESCRIBE helper that reads error details is evaluated before any generated code, captures native references, walks the prototype chain only to documented native error prototypes, and reads only own data properties (typeof descriptor.value === 'string') — guest getters, toString hooks, and Proxies never execute. Non-native thrown objects and resource exhaustion correctly keep the generic message. The error message is sanitized (control/Unicode formatting characters stripped, truncated to 160 chars) before display.

Structure validation (createShowResult) still runs first, so document-level issues are caught before the script check. If the QuickJS module fails to load, checkDocument silently passes (the view runtime still validates at display time).

The "Ask the assistant to fix" flow correctly distinguishes document errors from host/browser unavailability (HOST_FAILURE regex), only offers the button when canReview is true, and prevents concurrent submissions. The send function extracted from continueWithParameters shares the same error handling.

Findings

No P0, P1, P2, or P3 findings. The change is focused, well-tested (227 existing + new check.test.ts + updated integration tests), and CI is green on all code-compiling lanes (Test ubuntu/Node 22.x, Lint & Static, Integration Tests, web-shell E2E Smoke).

Notes

  • The readMarkup regex-based HTML parser has known limitations documented in the PR description and README ("unusual markup can differ slightly from what the browser starts with"). The markup it parses is generated by the model using a small, controlled subset, so this tradeoff is appropriate.
  • DESCRIBE and BRIDGE are large embedded JavaScript strings — consistent with the pre-existing pattern in this file.

Verdict

APPROVE. No blocking issues. This is a well-executed improvement that closes a real gap: broken generated scripts that the model incorrectly reports as ready.

@qwen-code-dev-bot qwen-code-dev-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.

APPROVE — 审查版本 head af4f2d61(提交前后两次读取一致,单 commit)。

  • 历史 Review:仅 yiliang114 一条 APPROVE,无 P0–P3 finding、无未解决 thread,与本次独立检查结论一致。
  • CI:head 上唯一 required check Lint & Static (ubuntu-latest, Node 22.x) success;Test (ubuntu-latest, Node 22.x)、web-shell E2E Smoke、Integration Tests (no-AK)、TUI parity、OpenTUI no-flicker、Managed Agent gate 均 success(含针对已构建 bundle 的 stdio VIEW_ERROR 集成用例)。非 required 的 precheck-pr / precheck 仍 pending,属自动化上下文,不构成门禁阻塞。
  • 独立检查未发现新 Critical:
    • 服务端执行边界成立:checkDocument 走 UiRuntime,构造函数即设 setMemoryLimit(32MB) / setMaxStackSize(256KB) / 中断处理器,round() 每轮 250ms 上限并在出错时 dispose(),while(true){} 会返回 time limit;guest 侧只有 BRIDGE 与 DESCRIBE,无 host/fs/network/tool 通道;模块加载失败时 fail-open,展示期仍由视图校验。
    • 错误信息不泄露宿主:formatScriptError 只接受 7 种原生错误名(QuickJS InternalError 与非 Error 抛出仍走笼统提示),仅从 generated-ui.js:line:col 提取位置,消息剥离控制/格式字符并截断 160 字符。
    • 规则与浏览器面一致:check.ts 的 ID 正则、INPUT_TYPES、控件必须带 id、select[multiple] 拒绝,与 surface.ts 的校验逐条对应,不会把视图能接受的文档误判为 VIEW_ERROR(服务端未覆盖的 element/input 数量上限属 fail-open 方向)。
    • mcp.ts 中 createShowResult 仍先执行,结构问题优先报 INVALID_DOCUMENT;main.ts 的修复入口只在 canReview 为真、非 host 失败时可用,pending/accepted 后禁用,不绕过宿主消息审阅。

非阻塞小问题(可后续处理,不影响合入):check.ts 的 decode() 直接 String.fromCodePoint(Number(code)),遇到越界数字字符引用(如 �、�)会抛 RangeError;该调用在 checkDocument 的 try 之外,会让工具回调 reject,被 MCP SDK 兜底成 isError: true + Invalid code point …,即浏览器本可正常显示的文档拿到一条无指导的错误。建议把 fromCodePoint 包一层降级为原样文本。

@qqqys
qqqys merged commit 8345f5c into QwenLM:main Oct 28, 2026
64 of 65 checks passed
@qwen-code-review-bot

qwen-code-review-bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Qwen Triage finished — view run. See the stage comments in this thread for the result.

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

@qwen-code-review-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed, not theoretical. A real Web Shell run produced a card whose generated script had one extra ), the tool result reported isError: false, and the model told the user the card was ready. The before/after table quotes the actual failing JavaScript and the actual tool result, so this is a documented failure with a reproduction rather than hardening against a hypothetical. The second half of the motivation is visible in the current code too: take() in the app runtime throws one generic string for every compile error and every exception the guest throws, so nobody — user or model — learns what actually went wrong.

Direction: aligned. This closes a feedback loop the feature was missing. The tool result is the model's only signal about whether a card worked, and today it says "success" for a card that cannot render. It is part of tracked issue #14019 (open, need-discussion, scope/web-shell), and the PR stays inside the creation-time check plus error text rather than pulling in the later theming / card-frame steps. Upstream's CHANGELOG has no direct reference to pre-display validation; the closest entry is about skipping MCP Apps UI resources in resource lists, so the area is active but this specific idea is ours.

One thing a maintainer should accept consciously rather than inherit: the server process now executes model-generated JavaScript. It previously never did — the README said so explicitly, and this PR rewrites that sentence. The execution runs inside the same QuickJS WASM interpreter with the same 32 MB / 256 KB stack / 250 ms bounds and no host, file, network or tool bindings, and it fails open (a WASM load failure skips the check and leaves display-time validation intact), so I don't read this as a sandbox-policy change. But "the MCP server runs generated code" is a posture change that deserves a deliberate yes.

Size: not applicable — no core paths. All 11 files sit under integrations/generated-ui/. For orientation: 690 changed lines = 458 production (check.ts 216, runtime.ts 113, main.ts 110, mcp.ts 19) + 225 test + 7 docs. Below both the 500-line core escalation and the 1000-line large-PR advisory.

Approach: the shape is right, and it matches what I proposed from the description before reading the diff — reuse the existing bounded runtime with a stand-in ui, run the initialization once, and compare the resulting patches against the markup. No parallel interpreter and no new dependency: @jitl/quickjs-singlefile-mjs-release-sync was already a devDependency, test-only until now, which is exactly what grows the server bundle.

Two questions worth sitting with, neither a blocker:

  • readMarkup re-derives in ~200 lines of regex what the browser surface already enforces against a real DOM. There is no DOM in the server bundle, so an approximation is a reasonable tradeoff — but it is now a second copy of the ID / control / input-type / select rules that can drift from the first, and drift in this direction means rejecting a document the browser would have displayed. A cross-reference comment in both files would make the coupling visible to whoever edits either one.
  • The PR carries three separable pieces: the creation-time check, richer client-side error naming, and a new "Ask the assistant to fix this view" action. The first two are squarely the stated goal. The third is adjacent — the "let the model recover at runtime too" half — and it touches continuationState, the most delicate state machine in the app. It reuses the existing host message review instead of adding a channel, so I'd keep it, but I'd have understood a split.

The Intl / toLocaleString note added to the writer skill is a drive-by from the same real run. Small, doc-only, and it prevents a silent-wrong-number class of failure, so fine to keep.

Risk: no elevated risk signals — the high-risk path scan matched nothing in this diff.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 已观测到的真实故障,不是理论性加固。一次真实的 Web Shell 运行中,生成的脚本多了一个 ),工具结果却是 isError: false,模型还告诉用户卡片已经就绪。before/after 表格引用了真实失败的 JavaScript 和真实的工具结果,所以这是有复现的已记录故障。动机的另一半在当前代码里也能看到:App runtime 的 take() 对每一个编译错误和每一个 guest 抛出的异常都只抛同一句笼统提示,用户和模型都无从得知到底出了什么问题。

方向: 对齐。这个 PR 补上了该功能缺失的反馈回路——工具结果是模型判断卡片是否可用的唯一信号,而现在它对一张无法渲染的卡片报告"成功"。它属于已跟踪的 issue #14019(open、need-discussion、scope/web-shell),并且范围收敛在创建时检查与错误文本,没有夹带后续的主题/卡片外框工作。上游 CHANGELOG 没有关于"展示前校验"的直接条目,最接近的是"MCP Apps UI 资源在资源列表中跳过",说明这个领域是活跃的,但这个具体思路是我们自己的。

有一点需要维护者明确认可、而不是被动继承:服务器进程现在开始执行模型生成的 JavaScript。此前它从不执行——README 明确这么写,本 PR 也改写了这句话。执行发生在同一个 QuickJS WASM 解释器内,内存 32 MB / 栈 256 KB / 250 ms 的限制完全一致,且没有宿主、文件、网络或工具绑定;并且是失败即放行(WASM 加载失败会跳过检查,展示时校验依然生效),所以我不认为这是沙箱策略变更。但"MCP 服务器会运行生成代码"是一次姿态变化,值得一个明确的 yes。

规模: 不适用——未触及核心路径。11 个文件全部位于 integrations/generated-ui/。供参考:690 行变更 = 458 行生产代码(check.ts 216、runtime.ts 113、main.ts 110、mcp.ts 19)+ 225 行测试 + 7 行文档。低于 500 行核心升级阈值和 1000 行大 PR 提示阈值。

方案: 结构是对的,也与我在读 diff 之前仅凭描述给出的方案一致——复用已有的受限 runtime,配一个替身 ui,把初始化跑一次,再把产生的 patch 与标记比对。没有引入并行解释器,也没有新依赖:@jitl/quickjs-singlefile-mjs-release-sync 本来就是 devDependency,此前只在测试中使用,这正是服务器 bundle 变大的原因。

两个值得考虑的问题,都不是阻塞项:

  • readMarkup 用约 200 行正则重新推导了浏览器 surface 已经基于真实 DOM 强制执行的规则。服务器 bundle 里没有 DOM,所以做近似是合理的取舍——但这现在是 ID / 控件 / input 类型 / select 规则的第二份副本,会与第一份产生漂移,而这个方向的漂移意味着拒绝一个浏览器本来能显示的文档。在两个文件里各加一句互相引用的注释,能让后续修改者看到这个耦合。
  • 这个 PR 包含三个可拆分的部分:创建时检查、客户端错误信息细化、以及新的"请助手修复此视图"操作。前两个正是标题所述目标;第三个是相邻的"让模型在运行时也能恢复"那一半,并且改动了 continuationState——App 里最精细的状态机。它复用了已有的宿主消息审阅而没有新增通道,所以我倾向于保留,但拆开我也完全能理解。

写作技能里新增的 Intl / toLocaleString 说明是来自同一次真实运行的顺手改动。篇幅小、仅文档,并且能防住一类"静默算错数字"的故障,保留没问题。

风险: 无升级风险信号——高风险路径扫描在本 diff 中没有命中任何文件。

进入代码审查 🔍

— Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-review-bot

Copy link
Copy Markdown
Collaborator

Code review

No critical blockers. The design is sound and the security-sensitive part is handled with more care than I expected. Notes below, all non-blocking, in the order I'd want them addressed.

1. readMarkup can reject a document the browser would have displayed. This is the one place the PR can make things worse than today, so it is worth knowing precisely where. surface.ts sanitizes through DOMPurify with a tag allowlist before validating; readMarkup matches every tag with a regex and applies no allowlist. So an id on a tag DOMPurify strips still gets registered, and the regex also matches tags inside HTML comments, which DOMPurify discards. Two concrete outcomes:

  • <iframe id="a"> alongside a real <p id="a"> — the browser strips the iframe and sees one id; the pre-check reports Element ID "a" is used more than once.
  • a commented-out <!-- <input id="d" type="date"> --> — the browser never sees it; the pre-check reports unsupported type "date".

Both need unusual markup, and the failure mode is recoverable (the error names the problem, the model corrects, the second attempt succeeds), so I am not calling this a blocker. But check.test.ts pins the rejection directions and nothing pins the acceptance direction, so a future drift here would be silent. Skipping comment ranges and intersecting against the same tag allowlist would close it cheaply.

The other divergences all run the safe way — no 512-node cap, html-patch ids merged without duplicate-checking against existing ids, elements removed by an html patch not tracked. Those under-detect, and display-time validation still catches them, which matches what the PR claims.

2. Initial values can differ from the browser's in two specific ways. Both are covered in general terms by Risk & Scope; naming them exactly so they can be judged: <textarea>\nfoo</textarea> — the HTML spec drops the first newline after the tag, so the browser starts with foo while readMarkup keeps \nfoo; and rangeDefault returns the raw midpoint without the browser's step snapping (min=0 max=10 step=3 gives 6 in the browser, 5 here). The worst case is a pre-check that passes or fails on slightly different numbers than the real view — it never changes what the browser displays.

3. HOST_FAILURE is a two-pattern denylist, and three non-document failures fall outside it. The card will offer Ask the assistant to fix this view for failures the model cannot fix: 'The view runtime failed. Use the summary.' (the main.ts fallback when state.error is empty), 'The view runtime failed. The summary is still available.' (controller.ts worker load/run failure), and 'The view runtime could not receive an interaction.' (controller.ts postMessage failure). Harmless in practice — the click still goes through host message review — but it invites the user to hand the model an unfixable error. Classifying document-caused failures positively would be sturdier than excluding two known host strings.

4. endContinuation() is now skipped on later terminal transitions, because of the terminal && failure === undefined guard. That is deliberate and right for a redundant re-publish of the same error phase. The side effect is that pagehide → controller.stop() no longer aborts an in-flight fix request, where the base code aborted unconditionally. The page is unloading so the request dies with it; noting it only because the behaviour changed.

What I checked and think is right, because a review that only lists doubts is not useful:

  • DESCRIBE captures getPrototypeOf, getOwnPropertyDescriptor and the native prototypes before any guest code runs, reads only own data descriptors, walks at most 8 levels and stops at a native prototype. So a guest getter, toString hook or Proxy cannot execute during error formatting, and a guest subclass redefining name as an accessor yields '' → absent from SCRIPT_ERRORS → the generic message. Failing safe in that direction is the correct choice.
  • describe() early-returns when this.interrupted, so a real timeout still surfaces as the time-limit error instead of being laundered into a "Script error". The finally { this.interrupted = false } cannot mask an overrun either, because round()'s catch re-checks performance.now() >= this.deadline. I traced both orderings.
  • loadModule() failing returns undefined — skip the check — rather than erroring the tool call. Right call for a best-effort pre-check layered over unchanged display-time validation.
  • checkDocument runs after createShowResult, so contract validation still owns normalization and size limits and VIEW_ERROR stays reserved for otherwise-valid documents.
  • The stale integration-test assertion javascript: 'throw new Error("Never executed by the MCP server")' was updated rather than left behind to become quietly false. That is the kind of thing that usually survives a PR like this.
  • Error text is control-character and newline normalized and length-bounded before it reaches the card and the session message, and the fix message frames the error as data rather than instructions.
sequenceDiagram
    participant P1 as MCP client
    participant P2 as generated_ui_show handler
    participant P3 as contract validation
    participant P4 as readMarkup
    participant P5 as QuickJS runtime
    participant P6 as checkPatches
    P1->>P2: show document with html css and javascript
    P2->>P3: createShowResult
    P3-->>P2: normalized fields and size limits ok
    P2->>P4: readMarkup on html
    P4-->>P2: ids and controls and initial input values or a markup problem
    P2->>P5: initialize with javascript and initial inputs
    P5-->>P2: patches or a thrown error named by DESCRIBE
    P2->>P6: checkPatches against markup
    P6-->>P2: missing id or non-input value target or clean
    P2-->>P1: VIEW_ERROR naming the problem or the snapshot as before
Loading

Testing

Evidence carried here: the PR's own CI check conclusions on the reviewed commit, read through the API. This was an unattended CI run, so the review is static by rule — I did not build or execute anything from this PR.

Why this CI run does cover the change: the Test (ubuntu-latest, Node 22.x) job runs npm run test:ci, which is test:ci:workspaces (--workspaces --if-present) plus test:scripts. integrations/generated-ui is a root workspace whose test:ci is npm run build && vitest run, so the new check.test.ts and the stdio integration test that asserts the exact VIEW_ERROR string against the built bundle both ran inside that job, and scripts/tests/generated-ui-assets.test.js ran under test:scripts. That job concluded success.

Check Conclusion
Qwen Code CI (workflow run, event pull_request) success
Test (ubuntu-latest, Node 22.x) success
Lint & Static (ubuntu-latest, Node 22.x) success
Integration Tests (no-AK, No Sandbox) success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) success
Check TUI impact success
TUI parity snapshots (ink vs opentui) success
Managed Agent gate success
OpenTUI no-flicker gate success
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped
triage (this run) in_progress

Of the 88 check-runs on this commit, 21 succeeded, 58 were skipped and 8 were cancelled. Every cancelled one is a bot-orchestration job (review-pr, authorize, precheck-pr, fallback-comment, publish-resolution, resolve-pr, ack-review-request, delay-automatic-review), not PR CI. No check on this commit is red.

Not verified, and why:

  • The author's specific counts (227 passed, 5 passed) are the author's claim, not something I re-ran. What I can attest is the CI conclusion above.
  • The macOS and Windows Test legs were skipped in this run, so cross-platform coverage rests on the ubuntu leg alone. The author's Tested-on table marks macOS and Windows as not tested, which is consistent rather than contradictory.
  • The Web Shell card surface — the Script error: … status text and the new fix button — is covered by jsdom unit tests in main.test.ts, not by a real Web Shell session. web-shell E2E Smoke passed, but I did not confirm it drives a failing generated view, so I am not counting it as coverage for this change.
  • The false-rejection direction described in finding 1 has no test at all. Nothing in the suite would fail if readMarkup started rejecting documents the browser accepts.

Sandboxed verification would settle exactly that gap: @qwen-code /verify — that readMarkup does not reject a document the DOMPurify-based surface validation would accept. No test in this PR pins the acceptance direction, so the suite passes identically whether or not readMarkup applies the tag allowlist or skips comment ranges, and that is the one way this change can regress a card that works today.

中文说明

代码审查

没有阻塞性问题。整体设计是合理的,安全敏感的部分处理得比我预期的更细致。以下都是非阻塞项,按我希望被处理的顺序排列。

1. readMarkup 可能拒绝一个浏览器本来能显示的文档。 这是本 PR 唯一可能比现状更差的地方,所以值得说清楚。surface.ts 是先经过 DOMPurify 的标签白名单净化、然后才校验;而 readMarkup 用正则匹配所有标签,不套用白名单。因此被打掉的标签上的 id 仍会被登记,正则也仍会匹配 HTML 注释里的标签(DOMPurify 会丢弃注释)。两个具体后果:

  • <iframe id="a"> 与真实的 <p id="a"> 同时存在时——浏览器会剥掉 iframe、只看到一个 id;而预检查会报 Element ID "a" is used more than once.
  • 被注释掉的 <!-- <input id="d" type="date"> -->——浏览器根本看不到;预检查会报 unsupported type "date"。

两者都需要少见的标记,且失败模式可恢复(错误会指明问题,模型修正后第二次即可成功),所以我不认为这是阻塞项。但 check.test.ts 只固定了"应拒绝"的方向,没有任何测试固定"应接受"的方向,所以这里将来发生漂移会是无声的。跳过注释区间并与同一份标签白名单取交集,就能低成本地堵住。

其余差异都朝安全方向偏——没有 512 节点上限、html patch 的 id 合入时未与已有 id 查重、被 html patch 移除的元素未被跟踪。这些都是漏检,而展示时校验仍会兜住,与 PR 的自述一致。

2. 初始值在两个具体点上可能与浏览器不同。 Risk & Scope 已经笼统提到,这里点明以便判断:<textarea>\nfoo</textarea>——HTML 规范会丢弃标签后的第一个换行,浏览器初值是 foo,而 readMarkup 保留 \nfoo;以及 rangeDefault 返回原始中点,没有做浏览器的 step 吸附(min=0 max=10 step=3 浏览器给 6,这里给 5)。最坏情况是预检查基于与真实视图略有不同的数字通过或失败——它永远不会改变浏览器实际显示的内容。

3. HOST_FAILURE 是一个只含两条模式的黑名单,有三个非文档故障落在它之外。 卡片会对模型无法修复的故障提供 请助手修复此视图:'The view runtime failed. Use the summary.'(main.ts 中 state.error 为空时的兜底)、'The view runtime failed. The summary is still available.'(controller.ts 中 worker 加载/运行失败)、'The view runtime could not receive an interaction.'(controller.ts 中 postMessage 失败)。实际影响不大——点击仍会走宿主消息审阅——但它会诱使用户把一个无法修复的错误交给模型。正向枚举"由文档导致的故障"会比排除两条已知宿主字符串更稳。

4. endContinuation() 现在在后续 terminal 状态更新时会被跳过,因为加了 terminal && failure === undefined 这个条件。对于同一错误阶段的冗余重复发布,这是刻意且正确的。副作用是 pagehide → controller.stop() 不再中止在途的修复请求,而基线代码是无条件中止的。页面正在卸载、请求本来也会随之终止;之所以提出来,只是因为行为确实变了。

我核对过并认为正确的部分,因为只列疑问的评审没有价值:

  • DESCRIBE 在任何 guest 代码运行之前就捕获了 getPrototypeOf、getOwnPropertyDescriptor 和原生原型,只读取自有数据属性描述符,最多向上走 8 层并在遇到原生原型时停止。因此 guest 的 getter、toString 钩子或 Proxy 都无法在错误格式化过程中执行;guest 子类把 name 重定义为访问器时会得到 '' → 不在 SCRIPT_ERRORS 中 → 回落到笼统提示。朝这个方向失败是正确选择。
  • describe() 在 this.interrupted 时提前返回,所以真正的超时仍会以时间上限错误呈现,而不会被洗成"Script error"。finally { this.interrupted = false } 也不会掩盖超时,因为 round() 的 catch 会重新检查 performance.now() >= this.deadline。两种时序我都推演过。
  • loadModule() 失败时返回 undefined——跳过检查——而不是让整个工具调用报错。对于一个叠加在"展示时校验不变"之上的尽力而为预检查,这是对的。
  • checkDocument 在 createShowResult 之后执行,因此规范化与长度限制仍由契约校验负责,VIEW_ERROR 只保留给其余方面都合法的文档。
  • 那条已经失效的集成测试断言 javascript: 'throw new Error("Never executed by the MCP server")' 被更新了,而不是被留下来悄悄变成假话。这类东西在这样的 PR 里通常是会漏掉的。
  • 错误文本在到达卡片和会话消息之前已做控制字符与换行规范化并限制长度,且修复消息把错误明确框定为数据而非指令。

测试

本次提供的证据: 在所审 commit 上,通过 API 读取的 PR 自身 CI 检查结论。这是一次无人值守的 CI 运行,按规则评审是静态的——我没有构建或执行本 PR 的任何代码。

为什么这次 CI 确实覆盖了本改动:Test (ubuntu-latest, Node 22.x) 这个 job 运行 npm run test:ci,即 test:ci:workspaces(--workspaces --if-present)加 test:scripts。integrations/generated-ui 是根 workspace,其 test:ci 为 npm run build && vitest run,所以新增的 check.test.ts 以及针对构建产物通过 stdio 断言精确 VIEW_ERROR 字符串的集成测试都在该 job 内运行;scripts/tests/generated-ui-assets.test.js 则在 test:scripts 下运行。该 job 结论为 success。

CI 表格见上方英文部分(未重复)。该 commit 上共 88 个 check-run:21 成功、58 跳过、8 取消。所有取消项都是机器人编排 job(review-pr、authorize、precheck-pr、fallback-comment、publish-resolution、resolve-pr、ack-review-request、delay-automatic-review),不是 PR CI。该 commit 上没有任何红色检查。

未验证的部分及原因:

  • 作者给出的具体数字(227 passed、5 passed)是作者的声明,我没有重跑。我能证明的只是上面的 CI 结论。
  • 本次运行中 macOS 与 Windows 的 Test 分支被跳过,因此跨平台覆盖仅依赖 ubuntu 分支。作者的 Tested-on 表格也把 macOS 与 Windows 标为未测试,两者一致而非矛盾。
  • Web Shell 卡片界面——Script error: … 状态文本与新的修复按钮——由 main.test.ts 中的 jsdom 单测覆盖,而非真实 Web Shell 会话。web-shell E2E Smoke 通过了,但我没有确认它会驱动一个失败的生成视图,所以不把它算作本改动的覆盖。
  • 上述第 1 点里的"误拒"方向完全没有测试。如果 readMarkup 开始拒绝浏览器能接受的文档,套件中不会有任何测试失败。

沙箱验证正好能补上这个缺口:@qwen-code /verify——验证 readMarkup 不会拒绝一个基于 DOMPurify 的 surface 校验会接受的文档。本 PR 没有任何测试固定"应接受"的方向,因此无论 readMarkup 是否套用标签白名单、是否跳过注释区间,套件的通过情况都完全相同;而这正是本改动唯一可能让今天能正常工作的卡片发生回归的路径。

— Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-review-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the design is right and the hostile-input handling is better than this feature strictly needed; the reservation is a second copy of the markup rules that nothing pins in the acceptance direction.

Stepping back: the problem is real and it is the worst failure mode a generated-UI feature can have. The tool result is the only channel through which the model learns whether a card worked, and before this PR that channel said "success" for a card that could not render — so the model confidently told the user the card was ready. Everything else in the diff follows from closing that loop.

The approach matches what I proposed from the description alone before reading any code — reuse the existing bounded runtime with a stand-in ui, run the initialization once, compare the patches against the markup — and it beats my baseline in one place I had not thought of. I would have formatted the guest error by reading name and message directly, which hands a guest getter or toString hook a path into the host string. Capturing the native references before any guest code runs, reading only own data descriptors, and stopping the prototype walk at a native prototype is the correct way to do it, and the fallback direction (unrecognised shape → generic message) fails safe rather than leaking.

The part that tries hardest is readMarkup. Two hundred lines of hand-rolled markup parsing is a real maintenance surface, and I would have preferred reusing the browser surface's own validator — but there is no DOM in the server bundle, and the alternative is a jsdom production dependency, which is heavier than the problem. So the tradeoff is defensible. What I would want before this ages badly is cheap: one test asserting that a document the DOMPurify-based validation accepts is also accepted by the pre-check. Right now every test pins a rejection, so the coupling between the two rule copies is invisible and drift would be silent. That is the difference between a 4 and a 5 for me, and it is a follow-up, not a blocker — the display-time validation is untouched, so the worst case is a recoverable extra correction round, not a broken card that ships.

The "Ask the assistant to fix this view" action is the most arguable piece of scope, and I have landed on keeping it. It is the runtime half of the same feedback loop, it reuses the existing host message review instead of opening a new channel, and the extracted send() removes duplication between the two flows rather than adding it. The HOST_FAILURE denylist missing three non-document failures is a small wart in the same area, worth a follow-up.

On the posture question from Stage 1 — the server now executes model-generated JavaScript. I flagged it so it would be a deliberate yes rather than an inherited one. It stays inside the same interpreter, the same bounds and the same absence of host bindings, it fails open, and the README sentence that claimed otherwise was corrected instead of left to go stale. A maintainer has also already approved this exact commit, so that yes has been given by a human.

CI is green on the reviewed commit with no pending PR workflow runs, and the green Test job genuinely covers this change: it runs the workspace suite, which includes the new check tests and a stdio assertion of the exact VIEW_ERROR string against the built bundle. Not verified: the author's pass counts (their claim, not re-run here), the macOS and Windows legs (skipped in this run), and the real Web Shell card surface (jsdom only). The one substantive untested path is the false-rejection direction named above.

Approving. The follow-ups I would like, none of them blocking: a test pinning the acceptance direction of readMarkup, a cross-reference comment between the two copies of the markup rules, and a positive classification of document-caused failures instead of the two-pattern host denylist.

中文说明

置信度:4/5 —— 设计方向正确,对恶意输入的处理甚至好于这个功能严格需要的程度;我的保留意见是标记规则出现了第二份副本,而"应接受"这个方向没有任何测试固定。

退一步看:问题是真实的,而且是生成式 UI 功能可能存在的最糟糕失效模式。工具结果是模型得知卡片是否可用的唯一通道,而本 PR 之前,这个通道对一张无法渲染的卡片报告"成功"——于是模型自信地告诉用户卡片已经就绪。diff 里的其他一切都是在补这个回路。

方案与我在读任何代码之前、仅凭描述给出的提议一致——复用已有的受限 runtime,配一个替身 ui,把初始化跑一次,再把 patch 与标记比对——并且在一个我没想到地方超过了我的基线。我本来会直接读取 name 和 message 来格式化 guest 错误,而那等于把一条通往宿主字符串的路径交到 guest 的 getter 或 toString 钩子手里。在任何 guest 代码运行之前捕获原生引用、只读取自有数据描述符、并在原型链上走到原生原型即停止,这才是正确做法;而回落方向(无法识别的形状 → 笼统提示)是安全失败,而不是泄漏。

最费力的部分是 readMarkup。两百行手写标记解析是真实的维护面,我本来更希望复用浏览器 surface 自己的校验器——但服务器 bundle 里没有 DOM,替代方案是把 jsdom 变成生产依赖,那比问题本身更重。所以这个取舍是站得住的。在它变糟之前我想要的东西很便宜:一个测试,断言"基于 DOMPurify 的校验会接受的文档,预检查也接受"。现在每个测试固定的都是拒绝方向,所以两份规则副本之间的耦合是不可见的,漂移会是无声的。这对我来说就是 4 分和 5 分的差别,而且它是后续工作、不是阻塞项——展示时校验没有被改动,所以最坏情况是可恢复的多一轮修正,而不是一张坏卡片被交付出去。

"请助手修复此视图"这个操作是范围上最值得争论的部分,我的结论是保留它。它是同一个反馈回路的运行时那一半,复用了已有的宿主消息审阅而没有新开通道,并且抽取出的 send() 是消除了两个流程之间的重复,而不是增加重复。同一块区域里,HOST_FAILURE 黑名单漏掉三个非文档故障是个小瑕疵,值得后续处理。

关于 Stage 1 提出的姿态问题——服务器现在会执行模型生成的 JavaScript。我把它标出来,是为了让它成为一个明确的 yes,而不是被继承的默认。它仍在同一个解释器、同样的限制、同样没有宿主绑定的范围内运行,失败即放行,而且那句原本声称"从不执行"的 README 被改正了,而不是留着变陈旧。也已有一位维护者批准了这个确切的 commit,所以这个 yes 已经由人给出。

所审 commit 上 CI 全绿,没有待完成的 PR workflow run,而且绿色的 Test job 确实覆盖了本改动:它运行 workspace 套件,其中包含新的 check 测试,以及针对构建产物通过 stdio 断言精确 VIEW_ERROR 字符串的测试。未验证:作者的通过数量(其声明,此处未重跑)、macOS 与 Windows 分支(本次运行被跳过)、以及真实 Web Shell 卡片界面(仅 jsdom)。唯一实质性的未测路径就是上面点名的误拒方向。

予以批准。我希望看到的后续工作,均非阻塞:为 readMarkup 的"应接受"方向补一个测试、在两份标记规则副本之间加互相引用的注释、以及正向枚举"由文档导致的故障"来替代只有两条模式的宿主黑名单。

— Qwen Code · qwen3.8-max-2026-09-02

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

@qwen-code-review-bot qwen-code-review-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. ✅

@qwen-code-review-bot

Copy link
Copy Markdown
Collaborator

Released in v0.26.3.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants