fix(web-shell): pin es2021 floor across both build configs and harden terminal tests - #11758
Conversation
… terminal tests - Resolve the build-config probe through web-shell's own Vite 5 instead of the root-hoisted Vite 7, parameterize it over vite.config.ts and vite.lib.config.ts, and pin build.target itself (R1-1, R1-2). - Share WEB_SHELL_BUILD_TARGET between the app and lib configs so the published lib bundle gets the same es2021 floor (R1-2). - Pin the localized protocol-mismatch notice and the releaseRequested flag in the mismatch test (R1-3, R1-4). - Drop the duplicated node-pty pin count owned by conpty-host.test.ts (R1-5). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the follow-up — I traced the whole chain rather than take the premise on faith, and it holds. Template ✓ all required sections present. The only gap is the template's optional Problem: observed, not theoretical. The masked-regression claim is also correct, and it's the more interesting half. The old probe imported bare Direction: aligned. This is a defect in an already-published public entry point, and the follow-up scope matches what #11748 left behind. The one judgment call a human may want to confirm: the shared constant also raises Size: not applicable. No core paths — the two configs sit at the package root rather than under Approach: scope feels right, and it reuses an existing convention instead of inventing one — The The two new Risk: no elevated risk signals — none of the five files match the high-risk path patterns. Review depth: normal. Moving on to code review. 🔍 中文说明感谢这个后续 PR——我没有直接采信前提，而是把整条链路都验证了一遍，结论是成立的。 模板 ✓ 必需章节齐全。唯一缺少的是模板里可选的 问题： 是已观测到的缺陷，不是理论性加固。 回归测试被掩盖这一点也是对的，而且是更有意思的一半。旧的探测代码 import 的是裸 方向： 一致。这是已发布公共入口上的缺陷，后续范围与 #11748 遗留的部分相符。唯一需要人来确认的判断点：共享常量同时把 规模： 不适用。没有触及核心路径——两个配置文件位于包根目录而非 方案： 范围合理，而且复用了既有约定而非新造一个——
风险： 无升级风险信号——五个文件都不匹配高风险路径模式。审查深度：常规。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewReading only the title and the "Why it's needed" section, here is what I would have done: give the lib config the same explicit That is what this PR does, point for point. I did not find a simpler path it missed, and I found no correctness bug, security hole, regression, or AGENTS.md violation. The four substantive points I checked independently rather than on trust:
Two non-blocking observations, neither of which I'd hold the PR for:
I skipped the sequence diagram and the changed-files table here: this is a five-file, 94-line config-and-test change with no new runtime flow, and the findings above already name every file. A table would just be noise. Test evidenceThis is an unattended CI run, so per the gate rules I did not build, run, or execute anything from this PR — no Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Reading the table:
Sandboxed verification would settle the one claim this diff cannot prove statically: Not verified: the real-scenario tmux capture. This is an unattended CI run, where the gate forbids executing PR-derived code, so the live-behaviour signal comes from the 中文说明代码审查只看标题和 "Why it's needed" 部分，我会这么做：给 lib 配置补上与 app 配置相同的显式 这个 PR 正是这么做的，逐点吻合。我没有找到它遗漏的更简路径，也没有发现正确性缺陷、安全漏洞、回归或违反 AGENTS.md 之处。有四个关键点我是独立核实的，而非采信：
两点不阻塞的观察，我都不会为此卡住这个 PR：
这里我省略了时序图和变更文件表：这是一个五文件、94 行的配置与测试改动，没有新的运行时流程，且上面的发现已逐一点名每个文件。加个表只会是噪音。 测试证据这是无人值守的 CI 运行，所以按 gate 规则我没有构建、运行或执行本 PR 的任何东西——没有 （CI 表格见上方英文部分，由 finalize 任务在 CI 结束后原地更新。） 表格解读：
沙箱验证可以了结本 diff 无法静态证明的那一条主张： 未验证：真实场景 tmux 抓取。这是无人值守 CI 运行，gate 禁止执行 PR 派生代码，所以实时行为信号来自上面点名的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Confidence: 4/5 — every claim in this PR survived independent checking and I found no blocker; the two reservations are small and named below, and the job that actually runs the changed tests had not reported yet. Going back to the proposal I wrote before comparing it to the diff: this PR matches it point for point, and I did not find a simpler path it missed. That is not the usual shape of a follow-up PR — most carry some extra refactor or a half-finished idea alongside the stated fix. This one is 14 production lines and 80 test lines, all four changes map onto a defect I could confirm independently, and there is no formatting churn or drive-by edit anywhere in it. What convinced me is that the premise was checkable rather than merely plausible. I could have taken "the lib bundle is broken by the same lowering bug" on faith; instead the chain resolves cleanly from the lockfile (root Vite 7.3.6 versus web-shell's 5.4.21 — a real divergence, not a hypothetical one) through the lib config's I pushed back on myself about whether the fix is worth its surface, since the population that hits the broken lib terminal is external npm hosts rather than the daemon-served app #11748 already fixed. It is, and not because of that population: the value that does not depend on consumer count is the repaired guard, which is what keeps #11643 from silently returning the next time someone touches either config. On maintenance six months out, I would thank the author rather than curse them. The comments earn their length — the Vite-version divergence, the reason the target must be pinned explicitly, and the fact that the pin count is owned by The two things keeping this at 4 rather than 5, both non-blocking:
Verdict is approve, but I am not posting an approval in this run: CI is still in flight on the reviewed commit, and approving now would attest to a result that does not exist yet. Approval is deferred until CI lands green on 中文说明信心：4/5 —— 本 PR 的每一项主张都经受了独立核查，我没有发现阻塞问题；两点保留意见都很小且已在下面点名，另外真正运行被改测试的那个 job 当时还没给出结果。 回到我在对比 diff 之前写下的方案：这个 PR 与之逐点吻合，我没有找到它遗漏的更简路径。这并不是后续 PR 常见的形态——多数都会在既定修复之外夹带一些额外重构或半成品想法。而这个 PR 是 14 行生产代码加 80 行测试，四处改动都能对应到一个我可以独立确认的缺陷，全程没有格式化噪音或顺手改动。 说服我的是：这个前提是可核查的，而不只是听起来合理。我本可以采信"lib 产物被同一个降级 bug 打坏了"；但整条链路从 lockfile（根 Vite 7.3.6 对 web-shell 的 5.4.21——是真实的版本分叉，不是假设）经 lib 配置的 我也对自己反驳过：既然踩到坏掉的 lib 终端的是外部 npm 宿主、而非 #11748 已修好的 daemon 服务页面，这个修复是否值得它带来的改动面。答案是值得，而且理由不在那部分人群：不依赖消费者数量的价值在于被修好的守卫本身——下次有人改动任一配置时，正是它防止 #11643 悄悄复发。 从六个月后的维护角度看，我会感谢作者而不是埋怨。这些注释对得起它们的篇幅——Vite 版本分叉、为什么必须显式固定 target、以及 pin 计数 归 让它停在 4 分而非 5 分的两点，均不阻塞：
结论是 approve，但本次运行我不会提交批准：CI 在被审查的 commit 上仍在进行，现在批准等于为一个尚不存在的结果背书。批准将延后至 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Independent verification report — measured A/B on the real build pipeline结论 / TL;DR: 未发现 Critical。用真实的 No Critical found. The fix is load-bearing and measured on real artifacts, not inferred from the diff. Gate state, read immediately before posting (head
|
| leg | value |
|---|---|
qwen-code-ci-bot review |
APPROVED @ head, 2026-09-13T07:04:05Z |
| product lanes at head (named, not a rollup) | Test (ubuntu-latest, Node 22.x) ✅ · Lint & Static (ubuntu-latest, Node 22.x) ✅ · web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ · Integration Tests (no-AK, No Sandbox) ✅ · Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ |
| skipped by the route classifier | Test (macos/windows), Integration Tests (CLI, No Sandbox), build-cli |
| check-runs at head | 47 total = 15 success / 31 skipped / 1 in_progress (review-pr, a bot lane — not counted as a product lane) |
| qqqys rows before this comment | reviews 0 · issue comments 0 · inline comments 0 |
Why this report has no tmux arm (declared, not omitted)
Both production files in this PR are browser-bundle build configurations (packages/web-shell/vite.config.ts, vite.lib.config.ts). Their only observable effect is the syntax floor of an emitted bundle that runs in a browser / webview, so no tmux TUI arm can reach the changed code — a tmux run would exercise the CLI's own app build and come back flat in both arms, which is the dangerous polarity (a flat A/B that reads as "no difference"). The substitute below drives the real compiler at the real resolved targets and the real emitted artifact, which is strictly more discriminating for this delta.
Arm A/B — real xterm.mjs, real resolved targets, both esbuild copies
Arm A's target is not a guess: it is what web-shell's own Vite 5.4.21 resolves for the base vite.lib.config.ts (which carries no target), read through resolveConfig: ["es2020","edge88","firefox78","chrome87","safari14"]. Arm B is the PR's WEB_SHELL_BUILD_TARGET = 'es2021'. Probe = the PR's own: _inputHandler.parse('\x1b[?2004$pafter-query') in a JSDOM context.
| arm | esbuild | target | ??=/||=/&&= surviving |
DECRQM probe |
|---|---|---|---|---|
| A (base) | root 0.25.6 |
Vite 5 default (above) | 0 / 0 / 0 (all lowered) | ❌ PARSE_THREW: "i is not defined", replies [], line 0 empty |
| A (base) | web-shell 0.21.5 |
same | 0 / 0 / 0 | ❌ identical throw, byte-identical 347,688-byte output |
| B (PR) | root 0.25.6 |
es2021 |
2 / 17 / 2 preserved | ✅ replies ["\u001b[?2004;2$y"], line 0 after-query |
| B (PR) | web-shell 0.21.5 |
es2021 |
2 / 17 / 2 | ✅ identical, byte-identical 344,620-byte output |
| C (masking control) | root 0.25.6 |
root Vite 7 default ["chrome107","edge107","firefox104","safari16"] |
2 / 17 / 2 | ✅ passes without the pin |
Throw site in arm A, from the stack: Za.requestMode ← Array.<anonymous> ← Ya.parse. The lowered code at that position, as emitted into a real bundle (arm A's own minified transform carries the same construct with a different mangled name, (void 0||(i={}))):
…_[_.PERMANENTLY_RESET = 4] = "PERMANENTLY_RESET"))(void 0 || (n = {}));
let s = this._coreService.decPrivateModes, …i.e. ??= was lowered into an IIFE argument whose assignment target is not in scope there, so the first mode query throws before any reply is emitted. Source xterm.mjs carries 2×??=, 17×||=, 2×&&=.
Arm C is why the test's webShellVite change matters. Resolving the config through the root-hoisted Vite 7 gives a default target that never lowers logical assignments, so the probe passes with the pin removed — a false-green. Resolving through web-shell's own Vite 5 (what this PR's test now does) is the only version of the probe that can fail. Independently confirmed, not taken from the comment.
The shipped artifact carries the same broken shape
The lib bundle built in this checkout from an unpinned vite.lib.config.ts (packages/web-shell/dist/index.js, 7,271,983 B) contains the identical construct at the identical site — (void 0 || (n = {})) immediately before let s = this._coreService.decPrivateModes — with 0 surviving logical-assignment operators anywhere in the file, and requestMode registered as the handler for CSI ? Pd $ p (DECRQM). So arm A is not a synthetic transform artefact: it is what npm hosts receive today. In-repo consumer: packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx imports @qwen-code/web-shell.
What that artifact does and does not cover: the checkout it was built from (b5567bb7a9) also predates the app-build pin, so its assets/ carry the same broken shape — that half is not evidence about this PR, whose base already pins the app build (vite.config.ts:92). Only the lib half is cited, and the lib config is unpinned at both that checkout and this PR's base, which is the state the PR changes.
Coverage checks behind "no Critical"
- Both production files read in full.
vite.config.ts's change is behaviour-preserving (a literal replaced by a shared const of the same value);vite.lib.config.tsgains the floor. No circular import (vite.config.tsdoes not import the lib config) and no duplicate declaration ofWEB_SHELL_BUILD_TARGET(1 declaration + 1 import + 1 use per file). - The strengthened test really runs in the green lane.
scriptsis a project in the rootvitest.config.ts, andTest (ubuntu-latest, Node 22.x)runsnpm run test:scripts→vitest run --config ./scripts/tests/vitest.config.ts. A test-efficacy argument would be fiction otherwise. - "Three builds" claim checks out.
build= app + lib default + lib--mode transcript;build.targetsits outside themodebranch, sodescribe.eachover the two config files does cover all three vite invocations. terminal.notice.protocolMismatchis a real key, not a mock invention. Present in both catalogues (client/i18n.tsx:1472EN ='Terminal protocol changed; restart the daemon and reload this page.',:5169ZH) and consumed atclient/components/terminal/TerminalPanel.tsx:310. The newstringContaining('Terminal protocol changed')assertion therefore pins production text verbatim rather than the mock's own?? keyfallback.- The relaxed pin count is a de-duplication, not a lost tripwire.
scripts/tests/package-assets.test.jsdropstoHaveLength(6)fortoBeGreaterThan(0), andpackages/core/src/services/conpty-host.test.ts:64does assertexpect(Object.keys(corePins)).toHaveLength(6)— so the human re-check the comment names exists at head. - Scope note, not a defect:
dist/transcript.jscontains no xterm at all (0 markers), so the transcript build cannot be hit by this bug; the pin there is floor consistency, which is what its comment claims.
One non-blocking observation (no action requested)
The lib floor rises from es2020/edge88/firefox78/chrome87/safari14 to es2021, which narrows browser support for npm hosts. Measured against the alternative — a bundle that throws on the first mode query — this is a strict improvement, and the app build already carried the same floor at this PR's base (vite.config.ts:92), so the two builds now agree. Recording it only so the support delta is a decision rather than a side effect.
Scope of this comment, as of the repository state read immediately before posting: it is a verification report and carries no approval; the merge gate on this PR is unchanged by it. Arm coverage is exactly what is declared above — the compiler/artifact A/B on the terminal DECRQM path and the enumerated file/test checks. It does not cover the rest of the lib bundle's runtime behaviour, the VS Code webview mount, or a browser-driven render.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6b": verifying the new comment's claim that "both esbuild copies miscompile identically only at the Vite 5 default" — I did not compile xterm.mjs at Vite 5's defau…; "agent reverse-audit (round 1)": I did not run the real vite build --config vite.lib.config.ts and grep the emitted dist/transcript.js / dist/index.js for un-lowered ??= — it is a multi….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): 39 passed — this review observed 7669, 31059, 587 passed.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| 'terminal.notice.protocolMismatch': | ||
| 'Terminal protocol changed; restart the daemon and reload this page.', |
There was a problem hiding this comment.
[Suggestion] R1-1: This pins the restart notice against a copy the test supplies to itself. vi.mock('../../i18n') at line 62 replaces the whole module, so the new assertion can catch a wrong key at the call site but says nothing about whether the real catalogs still carry the string — while the comment above it calls the notice "a named deliverable".
Delete or rename terminal.notice.protocolMismatch in client/i18n.tsx (EN at :1472, ZH at :5169), or lose it in a merge, and this suite stays green. Nothing else catches it either: npm run check-i18n is --workspace=packages/cli, and client/i18n.test.ts pins only two settings.* keys. The shipped terminal then writes the literal text terminal.notice.protocolMismatch into the pane at exactly the moment the user is being told to restart the daemon and reload.
Witness:
grep -rn 'terminal\.notice\.protocolMismatch' -> 4 hits, none a catalog assertion
client/i18n.tsx:1472 (EN catalog)
client/i18n.tsx:5169 (ZH catalog)
TerminalPanel.test.tsx:70 (the vi.mock dictionary)
TerminalPanel.tsx:310 (the call site)
client/i18n.test.ts -> 1 test; FOLLOWUP_SETTING_KEYS pins only
settings.label.ui.enableFollowupSuggestions
settings.description.ui.enableFollowupSuggestions
driven through the real module:
en: protocolMismatch -> "Terminal protocol changed; restart the daemon and reload this page."
en: absent key -> "terminal.notice.thisKeyDoesNotExist"
zh-CN: absent key -> "terminal.notice.thisKeyDoesNotExist"
TerminalPanel.test.tsx -> 22 passed, with no assertion touching i18n.tsx
Keep this assertion — the mock's ?? key fallback does make a wrong call-site key observable, which is real coverage. Add the missing half where the repo already pins this class, in client/i18n.test.ts alongside FOLLOWUP_SETTING_KEYS:
const TERMINAL_NOTICE_KEYS = ['terminal.notice.protocolMismatch'] as const;
it.each(TERMINAL_NOTICE_KEYS)('pins the terminal notice %s', (key) => {
expect(getTranslator('en')(key)).not.toBe(key);
// Compare against the EN copy, not against `key` — see the constraint below.
expect(getTranslator('zh-CN')(key)).not.toBe(getTranslator('en')(key));
});One constraint on that fix: const message = messages[key] ?? EN[key] ?? key; (client/i18n.tsx:7421) falls back to EN, so a zh-CN assertion written as !== key passes with the ZH entry deleted. EN and ZH are module-private, so getTranslator is the only route — which is what the existing test already uses.
The new client/i18n.test.ts case must go red when the key is removed from EN, and red again when it is removed from ZH; today no test in the repo fails for either mutation.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const resolveViteConfig = (webShellVite.resolveConfig ?? | ||
| webShellVite.default?.resolveConfig)!; |
There was a problem hiding this comment.
[Suggestion] R1-2: This branch cannot be taken. createRequire gives Node's CJS loader, which returns packages/web-shell/node_modules/vite/index.cjs's module.exports — a shape with resolveConfig directly and no .default — so the right operand of the ?? never evaluates, the & { default?: … } widening exists only to type it, and the trailing ! silences a nullability the expression can never observe. It does not become reachable under a future Vite 6/7 bump either: require() of an ESM-only Vite yields the namespace, where the named export sits alongside .default.
The cost is small but real. A genuinely missing resolveConfig surfaces as an opaque resolveViteConfig is not a function at the describe.each call site instead of a diagnosable undefined at the require, and the comment above records an interop shape as fact that the resolved Vite does not have. AGENTS.md's Simplicity First rules out error handling for impossible scenarios, and the house review rules name unreachable code as in scope rather than as a formatter nit.
To be explicit about how this got here: the predecessor review on the earlier PR told the author to expect resolveConfig on .default, so this is compliance with review guidance that measurement shows was unnecessary — not an invention, and not a mistake worth any extra scrutiny.
Witness:
vite resolved from web-shell : packages/web-shell/node_modules/vite/index.cjs
version = 5.4.21
typeof resolveConfig = function
typeof .default = undefined
.default?.resolveConfig = undefined
fallback branch taken? no (direct .resolveConfig exists)
root-hoisted vite 7.3.6, same probe: typeof resolveConfig = function
| const resolveViteConfig = (webShellVite.resolveConfig ?? | |
| webShellVite.default?.resolveConfig)!; | |
| const resolveViteConfig = webShellVite.resolveConfig; |
with the cast on line 28 dropping the widening to plain as typeof import('vite'), and the last sentence of the comment ("Vite 5 exposes resolveConfig on .default under some interops, so normalize that.") deleted.
Please keep the createRequire itself — reverting to a bare import { resolveConfig } from 'vite' would resolve root-hoisted Vite 7.3.6 (package-lock.json:25783) instead of web-shell's 5.4.21 (package-lock.json:31764), and only the package-anchored require reproduces the Vite 5 default build.target that lowers xterm's logical assignments. Measured, root esbuild at Vite 7's default [chrome107, edge107, firefox104, safari16] returns OK rather than throwing, so the hoisted copy would leave the string comparison as the only tripwire.
No test can pin this one — both Web Shell production terminal (%s) cases take the left operand before and after, so npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/web-terminal-build.test.ts staying at 2 passed is the whole check.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| describe.each(['vite.config.ts', 'vite.lib.config.ts'])( | ||
| 'Web Shell production terminal (%s)', | ||
| (configFile) => { |
There was a problem hiding this comment.
[Suggestion] R1-3: The matrix covers two config files, but the comment directly above it counts three builds — and resolveViteConfig(inline, 'build') passes no mode, so Vite 5.4.21 falls back to defaultMode = 'development'. That is a mode no real invocation produces: the package's build script runs production and transcript. So the --mode transcript lib build is never observed, even though target sits six lines above an already-existing mode === 'transcript' branch inside the same build: object.
Change vite.lib.config.ts:177 to target: mode === 'transcript' ? 'es2020' : WEB_SHELL_BUILD_TARGET — the exact shape the neighbouring lib.entry ternary at :179-186 already uses — and npm run test:scripts stays green on both cases, because both resolve mode 'development' and see 'es2021'. The real transcript build then ships a bundle lowered by the buggy esbuild pass, and nothing downstream repairs it: dist/transcript.js is inlined verbatim into every /export html document, and that build's own floor is target: ['chrome120'], which keeps ??= rather than re-lowering it.
One correction to the framing, so the gap is not dismissed along with it: dist/transcript.js carries no xterm at all, so this is not the DECRQM freeze. The harm is narrower and still real — the transcript build's options go unobserved by a guard whose comment says it covers three builds. The same correction applies to vite.lib.config.ts's own added clause "Also covers the transcript entry inlined into /export html documents", which names a hazard that bundle does not carry.
Witness:
mode measurement, unmodified PR config:
TEST-CALL lib, no mode -> mode="development" target="es2021" libEntry=["index","daemon-react-sdk"]
REAL lib default -> mode="production" target="es2021" libEntry=["index","daemon-react-sdk"]
REAL lib --mode transcript -> mode="transcript" target="es2021" libEntry=["transcript"]
mutation: vite.lib.config.ts:177 -> target: mode === 'transcript' ? 'es2020' : WEB_SHELL_BUILD_TARGET
PR's 2-case matrix -> Tests 2 passed (2) <- mutant SURVIVES
mutant + suggested 3-case fix -> x expected 'es2020' to be 'es2021'
Tests 1 failed | 2 passed (3) <- fix is load-bearing
mutant reverted, fix applied -> Tests 3 passed (3) <- green on arrival
grep of the built artifacts: dist/transcript.js -> xterm 0 hits, _inputHandler 0, allowProposedApi 0
dist/index.js -> xterm 178, _inputHandler 21, allowProposedApi 4
Add the third invocation and supply the mode in the inline config (the fix spans both the header and the call, so no one-click block):
describe.each([
['vite.config.ts', undefined],
['vite.lib.config.ts', undefined],
['vite.lib.config.ts', 'transcript'],
])('Web Shell production terminal (%s%s)', (configFile, mode) => {
it('answers DECRQM and keeps processing output after minification', async () => {
const config = await resolveViteConfig(
{
root: webShellRoot,
configFile: resolve(webShellRoot, configFile),
...(mode ? { mode } : {}),
},
'build',
);The mode has to go in the inline config, not positionally: measured against Vite 5.4.21, resolveConfig({ root, configFile, mode: 'transcript' }, 'build') returns mode="transcript", libEntry=["transcript"], while passing 'transcript' as the 4th positional argument returns mode="production", libEntry=["index","daemon-react-sdk"] — that parameter is defaultNodeEnv, not an override mode, and never reaches the config factory.
The new third case must go red when vite.lib.config.ts is mutated to target: mode === 'transcript' ? 'es2020' : WEB_SHELL_BUILD_TARGET and stay green on the unmodified config; today the suite is green under that mutation.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // Pin the floor itself: the probe cannot discriminate it (both esbuild | ||
| // copies miscompile identically only at the Vite 5 default), and | ||
| // resolveConfig always fills a default so a dropped `target` would be | ||
| // masked without this assertion. |
There was a problem hiding this comment.
[Suggestion] R1-4: This diff adds two comments that give contradictory accounts of the same guard. Lines 20-25 say the config is now resolved through web-shell's Vite 5 precisely so a dropped target no longer "keep[s] this probe green". Lines 106-107 say resolveConfig always fills a default, so a dropped target "would be masked without this assertion". The second was true of the pre-diff root-Vite-7 resolution it replaced, and is false of the code this diff introduces.
The concrete cost lands on the next maintainer of this file. These lines are the stated justification for the assertion, and they mark the probe half as inert — so an intentional floor bump, which trips toBe('es2021') on both describe.each cases, invites deleting the probe as dead weight. That would remove the only check in the repo that the pinned floor still produces a working xterm bundle rather than a correctly-spelled config value.
Witness:
vite5 default build.target = ["es2020","edge88","firefox78","chrome87","safari14"] (Vite 5.4.21, configFile:false)
root esbuild 0.25.6 @ vite5 default list -> THREW ReferenceError: i is not defined
web-shell esbuild 0.21.5 @ vite5 default list -> THREW ReferenceError: i is not defined
root esbuild 0.25.6 @ es2021 (pinned) -> OK replies=["\u001b[?2004;2$y"] output="after-query"
So a dropped target reddens the case without the assertion. To be fair to the first clause: "the probe cannot discriminate it" is defensible if "it" means the exact floor value — the probe passes at es2021, es2022 and esnext alike, so only the assertion pins the string. That is worth saying explicitly rather than leaving the parenthetical to imply the probe is inert.
| // Pin the floor itself: the probe cannot discriminate it (both esbuild | |
| // copies miscompile identically only at the Vite 5 default), and | |
| // resolveConfig always fills a default so a dropped `target` would be | |
| // masked without this assertion. | |
| // Pin the exact floor as well as the outcome: the probe passes at any | |
| // target >= es2021, so only this assertion names the value. A dropped | |
| // `target` resolves to Vite 5's es2020-era default, which the probe | |
| // below also rejects — but as an opaque `i is not defined` rather than | |
| // as the regression it is. |
The reworded comment must not contradict the sibling comment this same diff adds at lines 20-25 ("the bare vite import here lands on the root-hoisted Vite 7, whose default build.target (chrome107/…) never lowers logical assignments, so a dropped target: 'es2021' would keep this probe green"), which the measurement above confirms is the true account.
Comment-only, so nothing can go red for it — the toBe('es2021') assertion and the probe it annotates are unchanged, and the file runs 2/2 green either way.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| expect(config.build.target).toBe('es2021'); | ||
| const terminal = await builtTerminal(config.build.target); |
There was a problem hiding this comment.
[Suggestion] R1-5: Dropping the || 'esnext' operand turns this call into a strict-mode type error. config.build.target is string | false | string[] under the Vite types this file resolves, and builtTerminal (line 41) accepts only string | string[]. The old || was what narrowed false out of the union, so this diff introduces the error rather than inheriting it.
No lane catches it, which is why it is a Suggestion and not a blocker: root typecheck is npm run typecheck --workspaces --if-present && npm run typecheck:integration and scripts/ is in neither, packages/web-shell/tsconfig.json includes only client/**, eslint has no parserOptions.project here, and vitest transpiles with esbuild. But the root tsc program does cover the file, so every gate stays green while the file is red in any editor's TS server and in the first job that ever typechecks the scripts tree.
Witness:
ARM pre (bc7a186cda:scripts/tests/web-terminal-build.test.ts, line 87)
const terminal = await builtTerminal(config.build.target || 'esnext');
tsc exit=0
ARM post (worktree at 02d8dc11d8, line 109)
const terminal = await builtTerminal(config.build.target);
error TS2345: Argument of type 'string | false | string[]' is not assignable to
parameter of type 'string | string[]'. Type 'boolean' is not assignable to type 'string | string[]'.
flags: --noEmit --strict --skipLibCheck --module nodenext --moduleResolution nodenext --target es2022
flip: fix applied -> exit=0 ; fix reverted -> exit=2
also reproduces under a program extending the repo tsconfig.json with
include: ["scripts/tests/web-terminal-build.test.ts"]
root program coverage: tsc -p tsconfig.json --showConfig -> 9838 files, 22 under scripts/
| expect(config.build.target).toBe('es2021'); | |
| const terminal = await builtTerminal(config.build.target); | |
| expect(config.build.target).toBe('es2021'); | |
| const terminal = await builtTerminal(config.build.target || 'esnext'); |
Two things this fix must not disturb. async function builtTerminal(target: string | string[]) at line 41 forwards straight to esbuild's transform({ target }), which does not accept false either — so narrow the argument, do not widen the parameter. And the assertion on line 108 has to stay before the fallback: a fallback on its own would re-mask a dropped target, since resolveConfig always fills a default.
Type-only, so no test can pin it — tsc --noEmit --strict over the file exiting clean is the witness, and npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/web-terminal-build.test.ts must stay at 2 passed.
— qwen3.8-max via Qwen Code /review (v0.23.3)
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approve — cashes in the verification report above
结论 / TL;DR: 上面的独立验证报告（comment 5652040579，同一 head 02d8dc11d822）结论是「未发现 Critical、改动 load-bearing、双臂 A/B 在真实构建产物上量得」。本次 approve 兑现该结论，未新增审查范围。
Gate legs re-read immediately before this write — every value measured, none inferred:
| leg | value |
|---|---|
| head | 02d8dc11d822846d9f916fbad259da1fa56dae25 — unchanged since the report |
| state | open · merged=false · draft=false |
qwen-code-ci-bot APPROVED |
row 5189938914 @ head, 07:04:05Z, undismissed |
| lanes at head (enumerated, not a rollup) | 122/122 check-runs scanned = 17 success / 105 skipped / 0 failure / 0 in_progress; all five product lanes the report named are green |
| Criticals from any author | 0 |
CHANGES_REQUESTED at any commit |
0 |
⚠️ One disclosure about the effective review set
Between the report and this approval, qwen-code-ci-bot posted a second and different review of the same head — a /review round at 08:42:20Z (row 5190254886, state COMMENTED). GitHub keeps one effective review per author, so that row superseded the bot's own APPROVED: immediately before this write, reviewDecision read REVIEW_REQUIRED and mergeStateStatus read BLOCKED even though the approval row itself is still at head and undismissed.
The superseding round filed 5 findings, all Suggestion severity, 0 Critical (R1-1…R1-5, inline), and did not request changes. So no reviewer retracted anything — the BLOCKED state is a same-author bookkeeping artefact, and this approval is what puts an effective APPROVED back into the set. Recording it so the state change is not later read as a withdrawn approval.
On R1-5 — verified independently, and it is not a build break
R1-5 reports that dropping the || 'esnext' operand turns builtTerminal(config.build.target) into a strict-mode TS2345, and argues no lane catches it. That claim decides whether this is a Suggestion or a merge-blocking compile error, so it was checked against the tree rather than accepted:
- root
typecheck=npm run typecheck --workspaces --if-present && npm run typecheck:integration workspaces=packages/*,packages/channels/*,integrations/*—scripts/is not among them, and there is noscripts/package.json, so--workspacesnever reaches ittypecheck:integration=tsc -p integration-tests/tsconfig.json, which covers onlyintegration-tests/
⇒ the file is outside every typecheck gate, which is consistent with Lint & Static and Test both being green at this head. So R1-5 is a latent type error — red in an editor's TS server and in any future job that typechecks the scripts tree, but not a break of this PR's CI. Non-blocking, and worth fixing in passing since the operand this diff removed was what narrowed false out of string | false | string[].
The other four (R1-1 a mock-supplied i18n copy, R1-2 an unreachable ?? branch, R1-3 matrix/comment coverage of the transcript mode, R1-4 two contradictory comments about the same guard) are test-strength and comment-accuracy notes. None contradicts the report's measurements, and none is merge-blocking.
Scope of this approval, as of the repository state read immediately before posting: it covers the delta at 02d8dc11d822 — the two build-config files and the three test files — and the arms declared in the report above. It does not cover the rest of the lib bundle's runtime behaviour, the VS Code webview mount, or a browser-driven render. A head move voids it: re-derive rather than assume it carries.
|
Released in v0.23.4. |
4 participants
What this PR does
Follow-up to #11748 (merged at bc7a186) closing the five review findings that landed after its last head:
packages/web-shelllib build the same ES2021 floor as the app build.vite.lib.config.tsbundles and minifies the same xterm at Vite 5's default target, which miscompiles its logical assignments, so an npm host renderingTerminalPanelgets a terminal that throws on the first DECRQM query. The target is now a sharedWEB_SHELL_BUILD_TARGETconstant both configs import, so the two builds cannot drift apart.scripts/tests/web-terminal-build.test.ts: it resolves the config through web-shell's own Vite 5 instead of the root-hoisted Vite 7 (whose default target never lowers logical assignments and would mask a droppedtarget), pinsbuild.target === 'es2021'explicitly, and now covers both config files.?? keyfallback rendered the raw key) and asserts no second release-only socket is opened, making thereleaseRequestedflag observable.scripts/tests/package-assets.test.jswith a non-empty guard; the count itself stays owned byconpty-host.test.ts.Why it's needed
#11748 fixed the app-served terminal but left the npm-published lib bundle (
dist/index.js, the package'smain/module/exports["."]) broken by the same esbuild lowering bug, and left the new regression tests unable to catch the regressions they were written for.Reviewer Test Plan
How to verify
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/web-terminal-build.test.ts scripts/tests/package-assets.test.jscd packages/web-shell && npx vitest run client/components/terminal/TerminalPanel.test.tsxtargetfromvite.lib.config.tsturns the lib case of the build test red; removingreleaseRequested = trueturns the mismatch case red (toHaveLength(1)catches the second socket); misspelling the i18n key turns the notice assertion red via the?? keyfallback.Evidence (Before & After)
N/A — test/build-hardening change, no user-visible UI delta.
Tested on
Risk & Scope
dist/transcript.js, which is inlined into every/export htmldocument — ES2021 matches the app bundle's existing floor.Linked Issues
Refs #11748, #11643.