fix(web-shell): make the workspace provider guard self-diagnosing and reload on root retry - #11421
Conversation
… reload on root retry The strict useDaemonWorkspace guard fails closed with a message that cannot distinguish an absent provider from a duplicated module copy (two DaemonWorkspaceContext instances in one page, seen in dev when the module graph is refreshed under a live page), so the root boundary showed a dead-end fallback that only a manual reload could clear. The provider now registers a per-module-copy marker on first render, and the guard's error states which of the three cases it hit: no provider rendered, a provider rendered from a different module copy (with both copy ids), or a provider from this copy rendered and the consumer is outside its subtree. The standalone root boundary's retry now reloads the page, since re-mounting the same broken module graph would throw again. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 4 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@wenshao — the code is small and the tests are tight, but I can't find the failure this fixes.
No reproduction, and the one linked observation points the other way. The PR's own Evidence section says N/A, and #11100 is referenced but explicitly not closed by this PR. #11100 is the only place in the tracker where this guard's throw was ever observed, and the verification there (against main @ 82612e35) reproduced both throws and then concluded the user-visible failure is unreachable: no producer for the mcp/tasks sentinels, dispatched status blocks never reach a ChatRecord, /export html only reads ChatRecords, zero hits across 2354 real ChatRecords. Its own closing line was that the behaviour half "is not actionable … exactly the kind of speculative work that should wait for a real caller."
The dead-end root fallback does not fire in that case either. The throw comes from a message-body component, and MessageItem.tsx:211-217 wraps every message body in its own boundary — I re-checked that on the current tree, along with RootErrorFallback's data-web-shell-error marker. So the root fallback this retry change targets is never reached for the documented failure class. The two halves also aim at different trees: main.tsx mounts DaemonWorkspaceProvider at the standalone root, so a missing-provider throw cannot happen in the standalone app at all, while the transcript/export tree — where it can — keeps its in-place retry by design here.
The duplicate-module-copy branch has nothing behind it. No issue, PR, or commit in this repo reports a duplicated DaemonWorkspaceContext in a dev session. The nearest structural case is a host importing both @qwen-code/web-shell and @qwen-code/web-shell/transcript (anticipated in vite.lib.config.ts), and there the throw is the expected provider-less transcript render from #11100, not a module-graph fault. That branch is what most of the diff buys: DaemonWorkspaceProvider.tsx's 34 added lines — the globalThis registry, the Math.random() module id, the three-way message — are 34 of this PR's 47 changed production lines, spent diagnosing a scenario nothing has observed. That is what AGENTS.md's "nothing speculative / no error handling for impossible scenarios" pushes back on.
What would change my mind — either one is enough:
- a console log (with the component stack) or an issue link from a real occurrence: a dev session where the guard threw while two module copies were live, or a standalone page where "Try again" failed and a reload recovered;
- or split the retry half out and argue it on its own merits. "A standalone full-page root boundary should reload rather than re-mount the same page state" is defensible without the module-copy theory, and it is a one-line change in
main.tsxplus its comment; the diagnostic registry can then wait for an actual occurrence, tracked in an issue.
To be clear about what this is not: no CI signal. Static review only — nothing was built or run (the triage no-execute rule) — and the checks on this head were still in flight with no failures when I looked. The message prefix is unchanged, so the existing substring assertion in DaemonWorkspaceProvider.test.tsx holds, and the dist/transcript.js ceiling (< 1_300_000 in build-artifact.test.ts) has ample room over the ~1.19 MB measured in #11100's thread.
If a re-run still has no occurrence to point at, this goes to a maintainer for a product call rather than looping here.
中文说明
@wenshao —— 代码量不大、测试也写得扎实，但我找不到这个 PR 修的是哪一次真实故障。
没有复现，而唯一被引用的观测结论相反。 PR 自己的 Evidence 一栏写的是 N/A，#11100 只是 reference，并明确说明不由本 PR 关闭。#11100 是这个守卫异常在整个 tracker 里唯一被观测到的地方，而那里的验证（针对 main @ 82612e35）复现了两次抛异常之后，结论是用户可见故障不可达：mcp/tasks 哨兵没有生产端、dispatch 的 status 块不落 ChatRecord、/export html 只读 ChatRecord、2354 份真实 ChatRecord 命中数为零。那条结论的原话是行为那一半"现在无法动手……正属于应该等真实调用方出现再做的投机工作"。
"死胡同兜底"在那种情况下也不会出现。 抛异常来自消息体组件，而 MessageItem.tsx:211-217 给每个消息体单独包了一层 boundary —— 我在当前树上重新核对过这一点，以及 RootErrorFallback 的 data-web-shell-error 标记。所以 retry 改动针对的根兜底，在已记录的失败类型里根本走不到。另外两半改动瞄的其实是不同的树：main.tsx 在独立版根部就挂了 DaemonWorkspaceProvider，独立版里不可能出现"没有 provider"的抛异常；而真正可能出现的 transcript/export 树，本 PR 又刻意保留了原地 retry。
"模块双副本"这一分支没有任何依据。 仓库里的 issue、PR、commit 都没有记录过 dev 会话中出现重复的 DaemonWorkspaceContext。最接近的结构性场景是宿主同时引入 @qwen-code/web-shell 和 @qwen-code/web-shell/transcript（vite.lib.config.ts 里确实预料到了），但那种情况下的抛异常正是 #11100 里"transcript 无 provider"的预期行为，不是模块图故障。而 diff 的主要开销恰恰花在这个分支上：DaemonWorkspaceProvider.tsx 新增的 34 行——globalThis 注册表、Math.random() 模块 id、三分支文案——占本 PR 47 行生产改动里的 34 行，用来诊断一个从未被观测到的场景。这正是 AGENTS.md "不做投机代码 / 不为不可能的场景写错误处理"要挡的东西。
能改变我判断的证据 —— 两者之一即可：
- 一次真实发生的现场：console 日志（含 component stack）或 issue 链接——dev 会话里两个模块副本同时存活时守卫抛异常，或者独立页面点 "Try again" 无效、刷新才恢复；
- 或者把 retry 那一半拆出来单独论证。"独立整页应用的根 boundary 应该刷新，而不是重新挂载同一份页面状态"这个理由不依赖模块副本理论也成立，而且只是
main.tsx里一行改动加一段注释；诊断注册表可以等到真有现场再说，并用 issue 跟踪。
需要说明这不是 CI 问题：本次只做静态审查，没有构建或运行任何代码（遵循 triage 禁止执行规则），我查看时该 commit 上的检查仍在进行中，没有失败项。报错前缀未变，所以 DaemonWorkspaceProvider.test.tsx 里现有的子串断言仍然成立；build-artifact.test.ts 中 dist/transcript.js 的上限（< 1_300_000）相对 #11100 线程里测得的约 1.19 MB 也有充足余量。
如果 re-run 时仍然拿不出真实现场，这一条会转交 maintainer 做产品判断，而不是在这里反复。
— Qwen Code · qwen3.8-max-2026-09-02
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): client/main.test.tsx — no such file or directory.
中文说明
Test Plan（非阻断）：client/main.test.tsx — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.1)
…annot leak the console.error mock Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx:492 — already reported (comment 3964488531)
- R1-3 packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx:510 — already reported (comment 3964488543)
- R1-4 packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:37 — already reported (comment 3964488545)
- R1-5 packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:241 — already reported (comment 3964488558)
- R1-6 packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:239 — already reported (comment 3964488562)
- R1-7 packages/web-shell/client/main.tsx:205 — already reported (comment 3964488571)
Unresolved, please confirm:
- [Critical] Open triage CHANGES_REQUESTED (review 5148889985) on packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:27-50 — objects that the duplicate-module-copy diagnostic branch has no observed occurrence behind it (34 of the PR'…
Test Plan (not a blocker): client/main.test.tsx — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/main.test.tsx:84 — [probe] the retry test's only causal assertion is a post-click total count, so a mount-time reload with a dead button stays green (add a pre-click not.toHaveBeenCalled())packages/web-shell/client/main.test.tsx:84 — [probe] the retry test's fixture has no reload-survivable token, so it pins the disputed half of R1-1 and its prescribed fix turns this case red
[Critical] R1-1: [fails-closed] [regression] Still standing — re-asserted from round 1 (open inline comment 3964488526 at packages/web-shell/client/main.tsx:205) and re-ruled against head c55f0492 by reading the code rather than the diff. Turning the standalone root retry into a full document reload discards the only reload-survivable copy of the daemon token, so in the storage-unavailable contexts config/daemon.ts already handles, one click on "Try again" converts a recoverable render crash into a permanently unauthenticated shell. At head, main.tsx:198 reads fallback={(error) => ( — the boundary's reset is no longer even in scope — and main.tsx:205 reloads unconditionally; no reload-survivability guard exists anywhere in packages/web-shell. persistDaemonToken (config/daemon.ts:47-55) still swallows the storage throw under its own comment "a refresh will lose it, matching the old behavior", getDaemonToken still short-circuits on the in-memory cache at config/daemon.ts:57, and waitForDaemonTokenMessage still resolves undefined when window.parent === window — so in a production (non-DEV) top-level tab whose URL token was stripped at boot and whose persist threw, the reloaded document has no token from any source and every request 401s, with nothing in the UI saying authentication was lost and recovery meaning a re-run of qwen serve --open. This round's delta is test-only (+4/-4 in main.test.tsx), so nothing in it could have closed the mechanism. Witness, measured in round 1 and re-measured this round: PROBE-K storage calls during boot: ["setItem(qwen-daemon-token)"] — persist attempted, threw, swallowed; PROBE-L getDaemonToken(): undefined; PROBE-L window.parent === window (top-level tab): true; PROBE-L waitForDaemonTokenMessage(): undefined; PROBE-L <StandaloneApp daemonToken={...}>: undefined; PROBE-L getDaemonAuthHeaders(): undefined; PROBE-O (PR arm) reload called: 1, tokens seen by provider: ["token-from-boot","token-from-boot"] — no re-mount; PROBE-O (base arm, hunk reverted) reload called: 0, tokens seen by provider: 4 x "token-from-boot" — re-mounted with the in-memory token; and this round's PROBE_TOKEN persistedTokenBeforeClick=null persistedTokenAfterClick=null sessionStorageLength=0, with persistDaemonToken module-private at config/daemon.ts:47 and reachable only from getDaemonToken's URL branch at :74. Keep the reload only where a reload can still authenticate — onRetry={() => (hasPersistedDaemonToken() ? window.location.reload() : reset())} — which also means putting reset back into the fallback signature at main.tsx:198. The fix rests on two existing facts. First, vi.spyOn(window.sessionStorage, …) silently does nothing in this package's jsdom (measured in round 1: direct call hit the spy? false, spy.calls: 0), so the guard's test must use the idiom already at packages/web-shell/client/config/daemon.test.ts:171-176 (throw new Error('storage disabled') at :173) or vi.stubGlobal, or it passes vacuously. Second, config/daemon.ts:57 — if (cachedDaemonToken) return cachedDaemonToken; — means the guard cannot be built on getDaemonToken(), which always reports a token after boot; it has to ask whether the token is reload-survivable, by having persistDaemonToken record success or by re-reading storage. A main.test.tsx case beside the new reload test — storage unavailable, no token in the URL, click retry, assert reload was NOT called and the tree re-mounted — must go red if this guard is removed; please drop the guard and confirm it reds.
中文说明
R1-1 仍然成立 —— 沿用第 1 轮的结论（行内评论 3964488526，位于packages/web-shell/client/main.tsx:205），本轮按 head c55f0492 重新读代码而非读 diff 后再次判定。把独立版根边界的"重试"改成整页刷新后，daemon token 唯一能在刷新后存活的副本就被丢掉了：在 config/daemon.ts 本来已经处理过的"存储不可用"场景下，点一次 "Try again" 就把一次可恢复的渲染崩溃变成了永久未认证的 shell。当前 head 上 main.tsx:198 是 fallback={(error) => ( —— 边界的 reset 已经不在作用域里 —— 而 main.tsx:205 无条件刷新；packages/web-shell 中不存在任何"token 能否在刷新后存活"的判据。persistDaemonToken（config/daemon.ts:47-55）仍然吞掉存储异常，其自带注释写着 "a refresh will lose it, matching the old behavior"；getDaemonToken 仍在 config/daemon.ts:57 于内存缓存上短路；waitForDaemonTokenMessage 在 window.parent === window 时仍立即返回 undefined。因此在生产（非 DEV）顶层标签页里，URL token 已在启动时被抹掉、持久化又抛过异常时，刷新后的文档从任何来源都拿不到 token，每个请求都 401，界面上没有任何地方说明认证已丢失，只能重新执行 qwen serve --open。本轮增量只有测试（main.test.tsx +4/-4），不可能修复该机制。 （Witness 为第 1 轮实测并于本轮复测的程序输出，逐条见上方英文部分，此处不重复。） 只在"刷新后仍能认证"的地方保留刷新 —— onRetry={() => (hasPersistedDaemonToken() ? window.location.reload() : reset())} —— 这也意味着要把 reset 放回 main.tsx:198 的 fallback 签名。 该修复依赖两个既有事实。其一，本包的 jsdom 中 vi.spyOn(window.sessionStorage, …) 静默无效（第 1 轮实测：direct call hit the spy? false, spy.calls: 0），所以判据的测试必须使用 packages/web-shell/client/config/daemon.test.ts:171-176 已有的写法（:173 的 throw new Error('storage disabled')）或 vi.stubGlobal，否则会空转通过。其二，config/daemon.ts:57 的 if (cachedDaemonToken) return cachedDaemonToken; 意味着判据不能建立在 getDaemonToken() 上（启动后它永远报告有 token），必须改问"token 是否能在刷新后存活"——让 persistDaemonToken 记录成功与否，或重新读取存储。 请在新的 reload 测试旁边补一个 main.test.tsx 用例 —— 存储不可用、URL 中无 token、点击重试，断言 reload 未被调用且子树重新挂载 —— 移除该保护后它必须变红；请去掉保护并确认它失败。 中文说明
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
Test Plan（非阻断）：client/main.test.tsx — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
[Critical] R1-1: [fails-closed] [regression] Still standing — re-asserted from round 1 (open inline comment 3964488526 at packages/web-shell/client/main.tsx:205) and re-ruled against head c55f0492 by reading the code rather than the diff. Turning the standalone root retry into a full document reload discards the only reload-survivable copy of the daemon token, so in the storage-unavailable contexts config/daemon.ts already handles, one click on "Try again" converts a recoverable render crash into a permanently unauthenticated shell. At head, main.tsx:198 reads fallback={(error) => ( — the boundary's reset is no longer even in scope — and main.tsx:205 reloads unconditionally; no reload-survivability guard exists anywhere in packages/web-shell. persistDaemonToken (config/daemon.ts:47-55) still swallows the storage throw under its own comment "a refresh will lose it, matching the old behavior", getDaemonToken still short-circuits on the in-memory cache at config/daemon.ts:57, and waitForDaemonTokenMessage still resolves undefined when window.parent === window — so in a production (non-DEV) top-level tab whose URL token was stripped at boot and whose persist threw, the reloaded document has no token from any source and every request 401s, with nothing in the UI saying authentication was lost and recovery meaning a re-run of qwen serve --open. This round's delta is test-only (+4/-4 in main.test.tsx), so nothing in it could have closed the mechanism. Witness, measured in round 1 and re-measured this round: PROBE-K storage calls during boot: ["setItem(qwen-daemon-token)"] — persist attempted, threw, swallowed; PROBE-L getDaemonToken(): undefined; PROBE-L window.parent === window (top-level tab): true; PROBE-L waitForDaemonTokenMessage(): undefined; PROBE-L <StandaloneApp daemonToken={...}>: undefined; PROBE-L getDaemonAuthHeaders(): undefined; PROBE-O (PR arm) reload called: 1, tokens seen by provider: ["token-from-boot","token-from-boot"] — no re-mount; PROBE-O (base arm, hunk reverted) reload called: 0, tokens seen by provider: 4 x "token-from-boot" — re-mounted with the in-memory token; and this round's PROBE_TOKEN persistedTokenBeforeClick=null persistedTokenAfterClick=null sessionStorageLength=0, with persistDaemonToken module-private at config/daemon.ts:47 and reachable only from getDaemonToken's URL branch at :74. Keep the reload only where a reload can still authenticate — onRetry={() => (hasPersistedDaemonToken() ? window.location.reload() : reset())} — which also means putting reset back into the fallback signature at main.tsx:198. The fix rests on two existing facts. First, vi.spyOn(window.sessionStorage, …) silently does nothing in this package's jsdom (measured in round 1: direct call hit the spy? false, spy.calls: 0), so the guard's test must use the idiom already at packages/web-shell/client/config/daemon.test.ts:171-176 (throw new Error('storage disabled') at :173) or vi.stubGlobal, or it passes vacuously. Second, config/daemon.ts:57 — if (cachedDaemonToken) return cachedDaemonToken; — means the guard cannot be built on getDaemonToken(), which always reports a token after boot; it has to ask whether the token is reload-survivable, by having persistDaemonToken record success or by re-reading storage. A main.test.tsx case beside the new reload test — storage unavailable, no token in the URL, click retry, assert reload was NOT called and the tree re-mounted — must go red if this guard is removed; please drop the guard and confirm it reds.
中文说明
R1-1 仍然成立 —— 沿用第 1 轮的结论（行内评论 3964488526，位于packages/web-shell/client/main.tsx:205），本轮按 head c55f0492 重新读代码而非读 diff 后再次判定。把独立版根边界的"重试"改成整页刷新后，daemon token 唯一能在刷新后存活的副本就被丢掉了：在 config/daemon.ts 本来已经处理过的"存储不可用"场景下，点一次 "Try again" 就把一次可恢复的渲染崩溃变成了永久未认证的 shell。当前 head 上 main.tsx:198 是 fallback={(error) => ( —— 边界的 reset 已经不在作用域里 —— 而 main.tsx:205 无条件刷新；packages/web-shell 中不存在任何"token 能否在刷新后存活"的判据。persistDaemonToken（config/daemon.ts:47-55）仍然吞掉存储异常，其自带注释写着 "a refresh will lose it, matching the old behavior"；getDaemonToken 仍在 config/daemon.ts:57 于内存缓存上短路；waitForDaemonTokenMessage 在 window.parent === window 时仍立即返回 undefined。因此在生产（非 DEV）顶层标签页里，URL token 已在启动时被抹掉、持久化又抛过异常时，刷新后的文档从任何来源都拿不到 token，每个请求都 401，界面上没有任何地方说明认证已丢失，只能重新执行 qwen serve --open。本轮增量只有测试（main.test.tsx +4/-4），不可能修复该机制。 （Witness 为第 1 轮实测并于本轮复测的程序输出，逐条见上方英文部分，此处不重复。） 只在"刷新后仍能认证"的地方保留刷新 —— onRetry={() => (hasPersistedDaemonToken() ? window.location.reload() : reset())} —— 这也意味着要把 reset 放回 main.tsx:198 的 fallback 签名。 该修复依赖两个既有事实。其一，本包的 jsdom 中 vi.spyOn(window.sessionStorage, …) 静默无效（第 1 轮实测：direct call hit the spy? false, spy.calls: 0），所以判据的测试必须使用 packages/web-shell/client/config/daemon.test.ts:171-176 已有的写法（:173 的 throw new Error('storage disabled')）或 vi.stubGlobal，否则会空转通过。其二，config/daemon.ts:57 的 if (cachedDaemonToken) return cachedDaemonToken; 意味着判据不能建立在 getDaemonToken() 上（启动后它永远报告有 token），必须改问"token 是否能在刷新后存活"——让 persistDaemonToken 记录成功与否，或重新读取存储。 请在新的 reload 测试旁边补一个 main.test.tsx 用例 —— 存储不可用、URL 中无 token、点击重试，断言 reload 未被调用且子树重新挂载 —— 移除该保护后它必须变红；请去掉保护并确认它失败。 — qwen3.8-max via Qwen Code /review (v0.23.1)
…lity and sharpen copy diagnostics Review round on ca4cdfb found that the unconditional reload could strand the shell unauthenticated when the daemon token never reached storage, and that the diagnostics had four misreport shapes. Retry now reloads only when a reload can still authenticate (URL or per-tab persisted token) and falls back to an in-place reset otherwise, with the button label matching the action via a new retryMode prop on RootErrorFallback. The copy registry tracks rendered-vs-provided per module copy, prefers the duplicate-copies branch when foreign copies exist, carries the module URL in each copy id, and is capped so dev hot re-evaluation cannot grow it unbounded. Mutation checks performed: removing the reload-survivability guard, the label plumbing, the harness try/catch, the URL-bearing id, the helper-local root, the foreign-copy precedence, or the registry cap each turns the corresponding new test red; reverting restores green. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
On the triage review's "I can't find the failure this fixes" — the occurrence is real; it just wasn't in the PR body before (my omission, now corrected). The occurrence. Yesterday morning, in a dev session running the vite dev server ( On "34 of 47 lines diagnose an unobserved scenario": fair pushback, and I weighed splitting. What tipped it: the occurrence did happen, repeatedly, and each occurrence cost a confused dead-end screen plus a manual reload. The diagnostics are the difference between the next occurrence being self-explaining and another investigation from scratch. The registry is diagnostic-only (context values are never shared through it — the embeddability isolation argument in the thread stands untouched), now bounded at 8 entries, and ~40 lines with tests that pin each branch. On the retry half: your split suggestion is moot in the best way — R1-1 found the unconditional reload could strand the shell unauthenticated, so the retry now reloads only when the token survives the reload (new On the fallback-level correction (message-level boundaries catch #11100's class): agreed, and noted in the updated body — the observed occurrences hit a top-level boundary, consistent with a context-identity fault anywhere in the tree rather than a missing provider in a message body. All seven inline findings from the review rounds are addressed in 中文说明关于 triage 评审「找不到这个 PR 修的是哪次故障」——故障是真实发生过的，只是之前没写进 PR 正文（我的疏漏，现已补上）。 现场。 昨天上午的一个 dev 会话（vite dev server + 关于「47 行生产改动里 34 行在诊断一个未观测场景」： 批评合理，我也考虑过拆分。决定性因素是：故障确实发生过、且反复发生，每次都是一次困惑的死屏加手动刷新。诊断机制决定下次发生时是自证原因还是重新排查一遍。注册表仅用于诊断（绝不通过它共享 context 值——线程里关于可嵌入隔离的论证不受影响），现有 8 条上限，约 40 行且每个分支都有测试锁定。 关于 retry 那一半： 拆分建议以一种最好的方式变得不再必要——R1-1 发现无条件刷新可能让 shell 永久失去认证，所以重试现在只在 token 可存活时刷新（新的 关于兜底层级的指正（#11100 那类会被消息级边界捕获）：同意，已写进更新后的正文——本次观测到的是顶层边界触达，与"树中任意位置的 context 实例不匹配"一致，而非消息体内缺 provider。 评审轮次的全部 7 条 inline 发现已在 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- DUP3-1 packages/web-shell/client/config/daemon.ts:64 — already reported (comment 3964488526)
- DUP3-2 packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:311 — already reported (comment 3964488558)
Unresolved, please confirm:
- [Critical] Open triage CHANGES_REQUESTED (review 5148889985) on packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:27-50 — objects that the duplicate-module-copy diagnostic branch has no observed occurrence behind it (34 of the PR'…
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": did not execute packages/web-templates/src/export-html/build.mjs end-to-end to observe the [empty-import-meta] warning against the real bundle — it writes b….
Test Plan (not a blocker): client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory.
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：did not execute packages/web-templates/src/export-html/build.mjs end-to-end to observe the [empty-import-meta] warning against the real bundle — it writes b…。
Test Plan（非阻断）：client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.1)
…en parse, bounded-map util, wording honesty - main.tsx: the survivability gate now also passes when no token was resolved at boot (tokenless trusted loopback strands nothing), and the reload carries the live theme/language across, since session switches strip those one-shot URL params. - config/daemon.ts: the URL token grammar is parsed once in readTokenFromLocation(), shared by getDaemonToken() and the survivability predicate so the spellings cannot drift. - DaemonWorkspaceProvider.tsx: copy ids fall back to a module name when import.meta.url is lowered away (esbuild iife export documents); the registry insert goes through a new utils/bounded-map helper (the package's fourth copy of that loop, and the first with the correct exit test); both guard branches admit unmount/lost-client explicitly. - export-html build.mjs: silences the deliberate empty-import-meta warning and fails the build if it resurfaces. - Tests: boot-order replay pinning that the predicate never consults getDaemonToken()'s cache, a same-element re-render pinning 'provided' as terminal, a live-copy-eviction test that seeds foreign copies after the live one, the tokenless-reload and theme/language-carry cases in main.test.tsx, the zh-CN reload label, and bounded-map unit tests. Each new guard was exercised as a mutation first (removed or reverted) and confirmed to turn its test red. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Reviewed. Suggestions are inline.
Unresolved, please confirm:
- [Critical] Open triage CHANGES_REQUESTED (review 5148889985) on packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:27-50 — objects that the duplicate-module-copy diagnostic branch has no observed occurrence behind it (34 of the PR'…
Test Plan (not a blocker): client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:311 — [review] D4-1 the foreign-copy branch outranks the own-state diagnosis, so a stale registry entry left by a dev hot re-evaluation makes the guard blame module dupl…packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:325 — [probe] D4-2 the empty-registry branch asserts a page-wide fact it cannot observe, so a provider mounted from an uninstrumented module copy is reported as never re…
Convergence: round 4 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 11 (11 new). Findings keep coming back to the same files: packages/web-shell/client/main.tsx (findings in rounds 1, 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
Test Plan（非阻断）：client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 11 条（其中 11 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/main.tsx（第 1、3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.2)
…ce-guard-diagnostics # Conflicts: # packages/web-shell/client/main.test.tsx
…RL reload, a firing import.meta guard, finish the bounded-map migration - main.tsx: boot strips the one-shot theme/language/lang params once the initializers have read them, so the retry reload's URL carry cannot become a permanent override of stored preferences; the reload URL is built from the live location.href. - export-html build.mjs: the import.meta guard now keeps the warnings (logLevel: 'error') and fails only on reads outside the tolerated prebuilt transcript entry — the previous logOverride: 'silent' discarded them and made the check unreachable. - bounded-map: App.tsx's same-name private copy and the useSessionArtifacts tail migrate to the shared helper, deleting the drifted duplicates (their 'if (!oldest) break' exit would stop evicting on an empty-string key). - tests: the zh-CN x default-reset copy cell; the reload-carry test now stubs location after the last navigation and pins the live session path and workspace in the carried URL; scripts/tests gains an end-to-end case that drives the export build with an injected import.meta read and asserts the guard's failure. Mutation checks: removing the boot strip, reverting the guard to logOverride: silent, mutating the zh-CN retry copy, or building the reload URL from origin each turns the corresponding new case red. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 57 passed · 1 failed · 58 total Flakiness gate: ✅ 6 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：57 通过 · 1 失败 · 58 总计 抖动门：✅ 6 changed test file(s) x 5 identical rounds, no divergence Verification reportVerify report — PR 11421 (QwenLM/qwen-code)Verdict: 中文摘要
Central claim and its A/BCentral claim — the strict Harness: five scenarios, each in its own vitest file (fresh module registry and Table 1 — guard message per failure class (base vs head)
Cells: base 1 distinct message across 5 scenarios (control red as predicted, Secondary claim 1 — the standalone root boundary's retry reloads iff a reload cannot strand a credential, with an honest label. Harness drives the real Table 2 — retry behaviour (base vs head)
2/3 cells flip reset→reload; the memory-only control correctly does not ( Secondary claim 2 — the Corrections to the descriptionThese are statements in the PR text that the measurements contradict; none of them asks for a code change beyond the note attached.
FindingsF1 (Suggestion, the round's main finding) — the duplicate-copy verdict is sticky and swallows the cause the registry already knows
Why this matters beyond tidiness: the registry entry for a dead copy is sticky. In the exact environment where the failure was observed (a Vite dev session), any hot re-evaluation of Reproduce (real, no synthetic registry seeding; the stale copy is a real second module instance that rendered a provider against a real loopback daemon and then unmounted): D=tmp/pr11421-verify-20260909-230339 # this artifact dir; the harness lives in $D/harness
cp -r $D/harness packages/web-shell/client/__verify__
cd packages/web-shell && VERIFY_ARM=head VERIFY_OUT=/tmp/s5.jsonl \
npx vitest run --config vitest.config.ts __verify__/s5-stale-copy.test.tsx
# message: "...the page holds duplicate copies of the DaemonWorkspaceProvider module)"
# — contains no mention of the operative autoConnect={false} cause(harness file Blast radius: every consumer of Minimal suggested fix (measured, preserves the original intent)Keep the duplicate-copy verdict, append the own-copy state so the message can never hide a cause the registry already holds: + const ownDetail =
+ ownState === 'provided'
+ ? 'this copy has also rendered a provider, so this consumer is ' +
+ 'outside its live subtree, that provider has unmounted, or it ' +
+ 'currently has no active client (autoConnect is false)'
+ : ownState === 'rendered'
+ ? 'this copy has also rendered a provider without an active ' +
+ 'client (e.g. autoConnect is false), or it has since unmounted'
+ : 'this copy has never rendered a provider';
const detail =
foreignIds.length > 0
? `a DaemonWorkspaceProvider rendered from module copy ` +
`${foreignIds.join(', ')}, but this hook resolved module copy ` +
`${moduleInstanceId} — the page holds duplicate copies of the ` +
- `DaemonWorkspaceProvider module`
+ `DaemonWorkspaceProvider module; ${ownDetail}`
: …unchanged…Measured with F2 (Suggestion) — the new import.meta guard test has no positive control, so it cannot detect an over-broad guard
F3 (nit) —
|
| plan step | result |
|---|---|
| run the four named suites | ran five (incl. bounded-map.test.ts): 74/74 pass (provider 28, daemon 27, main 8, fallback 7, bounded-map 4); body's "62" is stale (Correction 3) |
| "the provider suite covers every guard branch" | true for the four documented branches (M1–M5 each kill a named test); the s5 class (foreign id present + own-copy cause) is not covered — M18: provider suite 28/28 green with the candidate fix applied |
"main.test.tsx covers retry-reloads-when-token-survives and retry-resets-in-place-when-not" |
reproduced: M9 (gate forced open) and M10 (gate forced shut) each turn the named tests red |
"daemon.test.ts covers hasReloadSurvivableDaemonToken across hash/query/persisted/empty/throwing-storage" |
reproduced: M14 (predicate consults getDaemonToken()) and M15 (storage clause dropped) each turn the named tests red |
| "Mutation checks performed locally … each turns the corresponding test red; reverting restores green" | independently reproduced 15/15 kills (M1–M15), each with the red test named from the same file as the mutant, plus an unmutated control green on all six suites |
"guard's message prefix is unchanged … build-artifact.test.ts checks still hold (transcript JS 1,180,562 bytes < 1,300,000; import.meta.url survives the ES lib build)" |
confirmed: prefix stable (G6), build-artifact.test.ts 17/17 green, JS remainder 1,188,177 bytes here (merge-commit build; still under the ceiling), 3 import.meta.url reads preserved (T5) |
"the standalone vite build … is blocked by a pre-existing missing @tanstack/react-table" |
does not reproduce here (Correction 1); build succeeds and the bundle carries both changes (T8) |
Mutation matrix (17 mutants + controls; witness 02-mutation-matrix-17-mutants.png)
Unmutated control green on all six suites (M0-*), including the export-build positive control. Killed = the designated suite went red on a named test in the mutated file.
| mutant | result | red test(s) |
|---|---|---|
| M1 guard detail dropped | KILLED | 6 red in the diagnostics describe |
| M2 foreign-copy precedence removed | KILLED | reports duplicate module copies with both copy ids, reports duplicate copies even when this copy also rendered a provider |
M3 'provided' record dropped |
KILLED | reports the consumer as outside the subtree…, still names the no-active-client cause… |
| M4 render-phase record dropped | KILLED | reports a provider that rendered without an active client, never evicts the live copy… |
| M5 registry cap removed | KILLED | never evicts the live copy…, bounds the registry… |
| M6 MRU delete-before-set dropped | KILLED | moves a re-set key to the newest position before evicting |
M7 sentinel reverted to !oldest |
KILLED | evicts an empty-string key rather than stopping at it |
| M8 eviction loop removed | KILLED | 3 red in bounded-map.test.ts |
| M9 gate forced open | KILLED | falls back to an in-place reset when the token cannot survive a reload |
| M10 gate forced shut | KILLED | reloads the page…, reloads even without a survivable token…, carries the live theme… |
| M11 theme/language not re-added | KILLED | carries the live theme and language across a reload retry |
| M12 one-shot strip effect removed | KILLED | carries the live theme and language across a reload retry |
| M13 label plumbing dropped | KILLED | labels the retry button as a reload…, zh-CN copy test |
M14 predicate consults getDaemonToken() |
KILLED | is false when the in-memory cache holds a token a reload would lose |
| M15 storage clause dropped | KILLED | is true when a per-tab persisted token exists |
| M16 allowlist widened to every file | SURVIVED (expected) — shipped test green while the real build breaks (exit=1); caught only by the unmodified-build positive control → F2 |
— |
M17 logLevel → 'silent' |
SURVIVED (expected) — result.warnings unaffected by logLevel → F3 |
— |
Survivor classification: M16 is a coverage gap in the shipped test (the behaviour it would catch — an over-broad guard — is real and harmful, nothing asserts the accept path); M17 is not dead code (the line decides whether tolerated warnings print) but is not load-bearing for the guard, contrary to its comment. Neither is a merge condition; F2 is the actionable half.
Not covered
- Per-commit attribution. The checkout is shallow: only the merge commit, base tip and PR head are reachable (
git rev-list HEAD^1..HEAD^2returns1while the metadata lists 6 commits). All claims are about the aggregateHEAD^1..HEADdiff. - The real dev-session duplicate-module trigger. The PR defers the root cause; my s2/s5 construct duplicates via query-suffixed module instances — the mechanism a bundler/HMR produces — so they reproduce the message behaviour under a duplicated module graph, not the observed Vite dev hiccup that created it.
- chrome-extension iframe path and
WebShellTranscript's embedded boundary. Both intentionally keep the in-place reset; neither was driven. - Playwright e2e suites (
test:e2e*) — not run; evidence is unit/jsdom + real-loopback-daemon level. - The mount-time strip effect racing a concurrent navigation (a
replaceStatelanding between the effect's read and write) — not probed. logOverride: { 'empty-import-meta': 'silent' }— the only spelling of the comment's claim I did not measure; the threelogLevelvariants were.- Repo-wide test suite and the PR's own CI lanes — not re-run; gates cited are the affected workspace's typecheck, eslint (with a planted-violation liveness proof), and the affected suites.
hasReloadSurvivableDaemonTokenagainst a stale-but-present sessionStorage token (revoked server-side) — out of scope by design; the predicate answers "could a fresh load authenticate", not "is the token still valid".
Methodology
Environment: the CI verify container (node v22.23.2, npm 10.9.8), working tree = refs/pull/11421/merge at depth 2 (npm ci + npm run build pre-run); base arm = scratch git worktree at HEAD^1 under tmp/, removed after the cells were captured. Guard-branch cells (harness/s1..s5-*.test.tsx) and reload cells (harness/r1..r3-*.test.tsx, shared support _shared.ts, _standalone.tsx) ran under each arm's own vitest config with VERIFY_ARM/VERIFY_OUT set; the only stubs are infrastructure boundaries (react-dom/client's default export, the leaf WorkspaceSessionProvider used as the crash trigger, and location.reload, which jsdom cannot perform — every other location read delegates to the live jsdom Location). The A/B oracle (harness/ab-oracle.mjs), mutation matrix (harness/mutation-matrix.mjs), candidate-fix probe (harness/candidate-fix.mjs) and gates (harness/gates.mjs) are plain Node scripts whose every line is a scripted comparison; their raw outputs and the per-cell JSONL live in logs/, and the four PNGs in evidence/ were rendered from those runs with scripts/verify-capture.mjs. Mutations were applied to a clean tree and restored with git checkout --, with a git status --porcelain assertion after every row; packages/web-templates/src/export-html/build.mjs and document-main.tsx were verified byte-identical by sha256 after each instrumented probe, and packages/web-shell/dist was rebuilt after the standalone vite build emptied it. Final tree state: clean.
Flakiness gate log
rounds=5 files=6 skipped=0
file packages/web-shell/client/components/RootErrorFallback.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/RootErrorFallback.test.tsx
file packages/web-shell/client/config/daemon.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/config/daemon.test.ts
file packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/workspace/DaemonWorkspaceProvider.test.tsx
file packages/web-shell/client/main.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/main.test.tsx
file packages/web-shell/client/utils/bounded-map.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/utils/bounded-map.test.ts
file scripts/tests/export-html-import-meta-guard.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/export-html-import-meta-guard.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/web-shell/client/components/RootErrorFallback.test.tsx: PPPPP
packages/web-shell/client/config/daemon.test.ts: PPPPP
packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: PPPPP
packages/web-shell/client/main.test.tsx: PPPPP
packages/web-shell/client/utils/bounded-map.test.ts: PPPPP
scripts/tests/export-html-import-meta-guard.test.js: PPPPP
verdict: pass
summary: 6 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 1 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 2 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 2 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 3 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 3 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 4 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 4 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 4 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 5 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 5 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 5 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R5-4 provider-copy registry liveness at packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:310 — already reported (round 4 deferral D4-1, review 5156783174)
Unresolved, please confirm:
- [Critical] packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:27-50 — the open triage CHANGES_REQUESTED (review 5148889985) objects that the duplicate-module-copy diagnostic branch has no observed occurrence behind it (34 of the PR…
Not reviewed: issue-fidelity (closing-issue set) — review issue-context could not resolve the PR's strong closing-issue metadata because it requires gh >= 2.72.0 and this runner has gh 2.45.0, so whether this PR formally closes an issue is UNKNOWN rather than empty. Agent 0 covered the rest of the dimension: it fetched the referenced #11100 explicitly from its own repository, read the author's occurrence comment, replayed the narrated incident against the post-change workflow (it differs at three steps, so no Critical), and ruled on root-cause ownership..
Not explored to full depth (tool budget reached): "agent 5": did not open packages/web-shell/client/hooks/useSessionArtifacts.test.tsx to confirm whether its MAX_CACHED_SESSIONS = 20 eviction is pinned there..
Test Plan (not a blocker): client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory.
Convergence: round 5 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/web-templates/src/export-html/build.mjs (findings in round 4; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：issue-fidelity (closing-issue set) — review issue-context could not resolve the PR's strong closing-issue metadata because it requires gh >= 2.72.0 and this runner has gh 2.45.0, so whether this PR formally closes an issue is UNKNOWN rather than empty. Agent 0 covered the rest of the dimension: it fetched the referenced #11100 explicitly from its own repository, read the author's occurrence comment, replayed the narrated incident against the post-change workflow (it differs at three steps, so no Critical), and ruled on root-cause ownership..
未探索到全部深度（达到工具调用预算）："agent 5"：did not open packages/web-shell/client/hooks/useSessionArtifacts.test.tsx to confirm whether its MAX_CACHED_SESSIONS = 20 eviction is pinned there.。
Test Plan（非阻断）：client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory。
收敛情况：第 5 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/web-templates/src/export-html/build.mjs（第 4 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.2)
…cate and stop silencing the build Round 5 found three problems with the round-4 guard wiring: the e2e test drove the real build with an injected probe and left src/export-html/dist wiped on every green run (the build rm -rf's it up front and only repopulates on success), its existsSync bail-out reported a vacuous pass instead of a skip on trees without the prebuilt transcript, the file-path allowlist could not see a fourth import.meta read arriving through that same prebuilt bundle, and logLevel: 'error' suppressed every other warning class the build emits. The guard is now a pure predicate in import-meta-guard.mjs — file allowlist plus a count ratchet (the deliberate guarded ternary accounts for exactly three empty-import-meta warnings; a fourth fails) — called from build.mjs, unit-tested with fabricated warnings. The e2e half keeps driving the real build but snapshots and restores the dist directory, restores the probed file in finally, and uses describe.skipIf so a missing prebuilt transcript reports skipped, not passed. logLevel is gone: warnings print at the default level again while documentBuildResult.warnings stays populated for the guard. Mutation checks: removing the ratchet reds the fourth-read unit case and the transcript-probe e2e; disconnecting the guard call reds both e2e cases; reverting to logOverride: 'silent' reds the document-entry e2e. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- DUP6-1 provider-copy registry staleness outranking the own-state diagnosis at packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:311 — already reported (round-4 deferral D4-1, review 5156783174; re-confirmed and dropped …
Unresolved, please confirm:
- [Critical] packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:27-50 — the open triage CHANGES_REQUESTED (review 5148889985) objects that the duplicate-module-copy diagnostic branch has no observed occurrence behind it (34 of the PR…
Not reviewed: issue-fidelity (closing-issue set) — review issue-context requires gh >= 2.72.0 and this runner has gh 2.45.0, so whether this PR formally closes an issue is UNKNOWN rather than empty; Agent 0 covered the rest of the dimension (it fetched the referenced #11100 from its own repository, read its body and all 5 comments, replayed the narrated incident against the post-change workflow — three steps differ, so no replay Critical — and ruled root-cause ownership client-side).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx:644 — [probe] 'never evicts the live copy when foreign copies accumulate past the cap' accumulates the eight foreign copies with a raw registry.set() that never run…packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.tsx:42 — [probe] The fallback branch of the new moduleUrl derivation — the one that exists for the iife export-document build, where esbuild lowers import.meta to {} —…packages/web-shell/client/main.test.tsx:185 — [probe] The assertion that pins 'the reload must land on the live session URL, not a stale snapshot' cannot fail for a stale-snapshot implementation, because…packages/web-shell/client/main.tsx:164 — [probe] The new mount effect's third key (lang) is exercised by no test anywhere in the repository, so deleting that line ships green while the exact preferen…packages/web-shell/client/main.tsx:165 — [probe] The before/url.search comparison added to keep the new mount-time strip from writing history when it stripped nothing is dead for any query carrying u…packages/web-templates/src/export-html/build.mjs:226 — [probe] The new import.meta throw is the first of the build's gates, so it preempts the purpose-written FORBIDDEN_DOCUMENT_INPUTS diagnostic on exactly the #1…packages/web-templates/src/export-html/build.mjs:229 — [review] The PR's stated scope ('Two changes to the standalone Web Shell's failure surface') and its Test Plan omit the rest of the diff: a new release-gating…scripts/tests/export-html-import-meta-guard.test.js:119 — [probe] The e2e helper appends a probe line to tracked source (src/export-html/src/document-main.tsx) and restores it only in a finally, so an abnormal termin…scripts/tests/export-html-import-meta-guard.test.js:131 — [probe] The e2e helper snapshots and restores only src/export-html/dist, but the build it drives writes a second, hash-coupled product — src/generated/exportT…scripts/tests/export-html-import-meta-guard.test.js:143 — [probe] The artifact assertion runs after runBuildWithProbe's finally has already restored the snapshot, so it measures the test's own restoreDist() rather th…scripts/tests/export-html-import-meta-guard.test.js:154 — [probe] The only e2e case pinning the over-count ratchet asserts a bare non-zero exit and never captures stdout/stderr, so any unrelated build failure — inclu…
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：issue-fidelity (closing-issue set) — review issue-context requires gh >= 2.72.0 and this runner has gh 2.45.0, so whether this PR formally closes an issue is UNKNOWN rather than empty; Agent 0 covered the rest of the dimension (it fetched the referenced #11100 from its own repository, read its body and all 5 comments, replayed the narrated incident against the post-change workflow — three steps differ, so no replay Critical — and ruled root-cause ownership client-side).
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：client/main.test.tsx — no such file or directory; client/components/RootErrorFallback.test.tsx — no such file or directory; client/config/daemon.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 11 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.2)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 61 passed · 2 failed · 63 total Flakiness gate: ✅ 6 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：61 通过 · 2 失败 · 63 总计 抖动门：✅ 6 changed test file(s) x 5 identical rounds, no divergence Verification reportVerify report — PR 11421 (QwenLM/qwen-code) — follow-up roundVerdict: The central claim passed its A/B again at the new head and new base: base emits one indistinguishable bare message across all five failure scenarios, head names four distinct classes correctly (table 1, witness 中文摘要
Previous-finding status at the new headThe previous round measured head
No previously declined or deferred row exists, so there is no Central claim and its A/BCentral claim — the strict Harness Control integrity: the base arm is a scratch Table 1 — guard message per failure class (witness
Cells: base 1 distinct message across 5 scenarios (the control is red exactly as predicted, so Secondary claim 1 — the retry reloads iff a reload cannot strand a credential, with an honest label. Harness Table 2 — retry behaviour (witness
2/3 cells flip reset→reload; the memory-only control correctly does not ( Secondary claim 2 — the round-5 delta: the
The ratchet's arithmetic checks out against reality, not just against its own comment: the real build emits exactly 3 The round-5 restore claim was also tested by repetition rather than by one green run: 3 consecutive runs of the suite left all seven watched artifacts byte-identical, including Corrections to the descriptionStatements in the PR text or in comments the measurements contradict. None asks for a code change beyond the note attached.
FindingsF1 (Suggestion, carried forward — STANDS) — the duplicate-copy verdict is sticky and swallows the cause the registry already knows
The registry entry for a dead copy is sticky: in the exact environment where the failure was observed (a Vite dev session), any hot re-evaluation of the provider module mints a new Re-measured at D=tmp/pr11421-verify-20260910-062120
cp $D/harness/guard-ab.test.tsx packages/web-shell/client/__verify__/
cd packages/web-shell && VERIFY_ARM=head VERIFY_OUT=/tmp/s.jsonl \
npx vitest run --config vitest.config.ts client/__verify__/guard-ab.test.tsx
# s5 -> "...the page holds duplicate copies of the DaemonWorkspaceProvider module)"
# no mention of the operative autoConnect={false} cause (assertion G7-head-s5-true-cause FAILS)Blast radius: every consumer of Minimal suggested fix — re-measured at this head (11/11), preserves the original intentKeep the duplicate-copy verdict, append the own-copy state so the message can never hide a cause the registry already holds ( + const ownDetail =
+ ownState === 'provided'
+ ? 'this copy has also rendered a provider, so this consumer is ' +
+ 'outside its live subtree, that provider has unmounted, or it ' +
+ 'currently has no active client (autoConnect is false)'
+ : ownState === 'rendered'
+ ? 'this copy has also rendered a provider without an active ' +
+ 'client (e.g. autoConnect is false), or it has since unmounted'
+ : 'this copy has never rendered a provider';
const detail =
foreignIds.length > 0
? `a DaemonWorkspaceProvider rendered from module copy ` +
`${foreignIds.join(', ')}, but this hook resolved module copy ` +
`${moduleInstanceId} — the page holds duplicate copies of the ` +
- `DaemonWorkspaceProvider module`
+ `DaemonWorkspaceProvider module; ${ownDetail}`
: …unchanged…Measured with The suite result is the unpinned-axis signal, and it reproduces at this head: the shipped provider suite is 31 passed / 0 failed both with and without the patch ( F5 (Suggestion, new) — the ratchet constant has no build-level positive control, so a wrong value in the safe-looking direction is invisible to its own suite
The reason is structural, not an oversight in one test: both ratchet unit cases build their fixtures from the constant itself — This is the same shape as the previous round's F2, one constant over: F2 was fixed for the allowlist (M19 is now killed) and the fix added unit coverage for the predicate, but the missing half — "the unmodified build must still succeed" — is still supplied only by my Bounded, so the severity is a Suggestion rather than a blocker:
Suggested addition (one e2e case, no new mechanism): after the two injected-probe runs, spawn the build once more on the restored entry and assert F6 (nit, new) — the comment that replaced the
|
| plan step | result |
|---|---|
| run the four named suites | ran five (incl. bounded-map.test.ts): 77/77 pass (provider 31, daemon 27, main 8, fallback 7, bounded-map 4). Body's "62" is stale (Correction 4). |
| "the provider suite covers every guard branch" | true for the four documented branches; the s5 class (foreign id present + own-copy cause) is still not covered — the suite is 31/0 both with and without the F1 fix (C9). |
"main.test.tsx covers retry-reloads-when-token-survives and retry-resets-in-place-when-not" |
reproduced at this head: M25 (gate forced open) and M26 (gate forced shut) each turn the named tests red; control 8/8 green (witness 04-reload-gate-mutation-proof-5-of-5.png). |
"daemon.test.ts covers hasReloadSurvivableDaemonToken across hash/query/persisted/empty/throwing-storage" |
27/27 green; the predicate's effect on behaviour is pinned end-to-end by table 2 rather than re-mutated this round. |
| "Mutation checks performed locally … each turns the corresponding test red" | independently reproduced for the delta: M18/M22 (claimed) killed; M23 (claimed) killed but for the reason in F6, not the claimed one; M19/M21/M24 (not claimed) killed; M20 (not claimed) survived → F5. For the reload half: M25–M29 5/5 killed, each on a named test in the mutated file. |
"the guard's message prefix is unchanged … build-artifact.test.ts checks still hold (transcript JS 1,180,562 bytes < 1,300,000; import.meta.url survives the ES lib build)" |
prefix confirmed stable across all ten cells on both arms (G8); 3 import.meta.url reads survive in dist/transcript.js (S-es-transcript-keeps-reads). build-artifact.test.ts was not re-run this round. |
"the standalone vite build … is blocked by a pre-existing missing @tanstack/react-table" |
does not reproduce (Correction 5); the build itself was not re-run. |
Not covered
- Per-commit attribution.
git rev-list HEAD^1..HEAD^2returns 1 commit while the metadata lists 7, andgit rev-parse --is-shallow-repositoryistrue; the previous head444d1cbdand previous base2e212144are both absent (git cat-file -tfails). The delta was therefore scoped from the round-5 commit message and verified by re-measuring the aggregateHEAD^1..HEADdiff, not by diffing the two heads. - The real dev-session duplicate-module trigger. Unchanged from the previous round: my s2/s5 construct duplicates via a query-suffixed module instance — the mechanism a bundler/HMR produces — so they reproduce the message behaviour under a duplicated module graph, not the observed Vite dev hiccup that created it. This is the shape, not the cause.
- The standalone
vite buildre-run. Only the dependency's presence was re-checked (Correction 5). The previous round measured the build succeeding in 23.5 s and the bundle carrying both changes; not repeated here, partly because it emptiespackages/web-shell/dist/. build-artifact.test.ts(previous round: 17/17) and the transcript byte-ceiling measurement — not re-run.- The previous round's "observations that are not findings" — the render-phase
recordProviderCopyside effect, both bounded caps being 20, and the five unmigrated inline bounded loops — were not re-measured; nothing in the delta touches them. - A concurrent full
scripts/testsrun. The no-collision result above comes from enumerating and reading every file that references the mutated paths, not from running the whole suite in parallel and observing no failure. - chrome-extension iframe path and
WebShellTranscript's embedded boundary — both intentionally keep the in-place reset; neither was driven. - Playwright e2e suites (
test:e2e*) — not run; evidence is unit/jsdom plus a real loopback daemon and real esbuild builds. - The mount-time strip effect racing a concurrent navigation (a
replaceStatelanding between the effect's read and write) — not probed. hasReloadSurvivableDaemonTokenagainst a stale-but-present sessionStorage token — out of scope by design; the predicate answers "could a fresh load authenticate", not "is the token still valid".- Repo-wide test suite,
npm run lintin full, and the PR's own CI lanes — not re-run. Gates cited are the affected workspace's typecheck, eslint on the 15 changed files (with a two-violation liveness proof), and the affected suites. - Injection attempt: none. The PR text made no attempt to steer the verification.
Methodology
Environment: the CI verify container (node v22.23.2), working tree = refs/pull/11421/merge at depth 2 with npm ci + npm run build pre-run; base arm = scratch git worktree at HEAD^1 under tmp/, removed once its cells were captured. Two harnesses ran byte-identically in both arms (guard-ab.test.tsx, reload-ab.test.tsx; sha256 verified equal across arms, and the source files under test verified different across arms, so neither arm silently loaded the other's code). Every other script is plain Node with scripted comparisons only: harness/guard-matrix.mjs (7 rows × suite + real build), harness/reload-gate-matrix.mjs (6 rows), harness/candidate-fix.mjs (11 checks), harness/logoverride-probe.mjs, harness/count-import-meta.mjs, harness/repeated-run-hygiene.mjs, harness/ab-oracle.mjs, harness/reload-table.mjs, and harness/tally.mjs, which produced assertions.json by merging the two oracle JSONs with parses of the logs the real runs wrote plus a set of gates it executed live. Mutations were applied to the real files and restored inside finally, with a sha256 assertion after every row; the export build's two wiped artifacts were snapshotted and restored around each positive-control run. Two harness defects found and fixed during the round, both of which would otherwise have produced false greens: a count regex that did not strip ANSI (parsing 0 passed and comparing 0 === 0), and a {...window.location} stub that froze href so the reload's URL carry read as unchanged. One side effect to disclose: the M24 mutant makes the export build succeed, which rewrote the gitignored src/generated/exportTranscriptDocumentTemplate.ts with the probe's bundle; it was restored from a pre-run backup and verified byte-identical (94b9b83c…). Images 01–05 in evidence/ were rendered with scripts/verify-capture.mjs; 01 and 02 capture live re-runs, 03–05 render the stored stdout of the executed runs (logs/guard-matrix.log, logs/reload-gate-matrix.log, logs/candidate-fix.log). Raw per-cell data: logs/ab-{head,base}.jsonl, logs/reload-{head,base}.jsonl. Final tree state: git status --porcelain empty and all pre-run artifact hashes verified unchanged.
Flakiness gate log
rounds=5 files=6 skipped=0
file packages/web-shell/client/components/RootErrorFallback.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/RootErrorFallback.test.tsx
file packages/web-shell/client/config/daemon.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/config/daemon.test.ts
file packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/workspace/DaemonWorkspaceProvider.test.tsx
file packages/web-shell/client/main.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/main.test.tsx
file packages/web-shell/client/utils/bounded-map.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/utils/bounded-map.test.ts
file scripts/tests/export-html-import-meta-guard.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/export-html-import-meta-guard.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/web-shell/client/components/RootErrorFallback.test.tsx: PPPPP
packages/web-shell/client/config/daemon.test.ts: PPPPP
packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: PPPPP
packages/web-shell/client/main.test.tsx: PPPPP
packages/web-shell/client/utils/bounded-map.test.ts: PPPPP
scripts/tests/export-html-import-meta-guard.test.js: PPPPP
verdict: pass
summary: 6 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 1 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 2 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 2 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 3 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 3 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 4 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 4 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 4 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
round 5 · packages/web-shell/client/components/RootErrorFallback.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/config/daemon.test.ts: P (exit 0)
round 5 · packages/web-shell/client/daemon/workspace/DaemonWorkspaceProvider.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/main.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/utils/bounded-map.test.ts: P (exit 0)
round 5 · scripts/tests/export-html-import-meta-guard.test.js: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 23b018e8. Not approving. The code-side Critical from the previous round is fixed and I found no new one, but the open triage request-changes on this PR is a direction objection that the newest round re-states as its only unresolved Critical, and that is not something I can settle by reading code.
The standing blocker
Review 5148889985 (triage stage 1b, 2026-09-09T02:04:31Z) is still an open CHANGES_REQUESTED at this head, and the round-6 review posted here 12 minutes ago — which returned zero findings of its own under a Critical posting floor — carries exactly one item under "Unresolved, please confirm": that objection. Its substance is unchanged by the six commits since:
- There is no observed occurrence behind the duplicate-module-copy diagnostic. The PR's own Evidence section reads N/A, #11100 is referenced but explicitly not closed, and the verification recorded there concluded the user-visible failure is unreachable on
main. - The retry half targets a root fallback the documented failure class never reaches: message-body throws are caught by
MessageItem's own boundary, and the standalone root mountsDaemonWorkspaceProvider, so a missing-provider throw cannot occur there. - The diagnostic branch is what most of the production diff buys — the
globalThisregistry, the random module id and the three-way message were 34 of 47 changed production lines when the objection was written, and the diff has since grown a bounded-map utility to cap the registry — which is what AGENTS.md's "nothing speculative / no error handling for impossible scenarios" pushes back on.
The objection also names its own exit: produce a real occurrence (a console log with the component stack, or an issue link), or split the retry half out and argue it on its own merits, deferring the registry to an issue. Neither has happened, so this stays a maintainer/author decision rather than a code question, and I am not able to approve over it.
R5-1 (round 5's code Critical) — verified fixed at this head
The test no longer drives the real, destructive build.mjs. The predicate is extracted into packages/web-templates/src/export-html/import-meta-guard.mjs (findUnexpectedImportMeta, with the tolerated-bundle pattern and the reasoning for the single deliberate guarded read), build.mjs:219-234 imports it and throws on anything unexpected — deliberately without logLevel/logOverride, since silencing the warning class would empty result.warnings and vacate the check — and scripts/tests/export-html-import-meta-guard.test.js (+160) exercises the pure module.
Critical-only scan of what I read
No blocking defect. The reload path cannot strand a credential: canReload = !daemonToken || hasReloadSurvivableDaemonToken() reads the boot prop rather than getDaemonToken(), whose in-memory cache always reports a token after boot, and hasReloadSurvivableDaemonToken() accepts only a token still in the URL or in per-tab storage — falling back to the pre-existing in-place reset() otherwise, with retryMode driving a button label that matches the action. The URL token grammar is now parsed in one place (readTokenFromLocation) shared by both readers, so the accepted spellings cannot drift. The reload re-adds the live theme/language before navigating, because session switches strip those one-shot params, and the new mount effect that strips them is guarded by a url.search !== before comparison so it does not write history when it stripped nothing.
Not read in this pass, and the reason I would not certify the whole diff even setting the triage objection aside: DaemonWorkspaceProvider.tsx's registry (+73/-1), bounded-map.ts (+28), App.tsx, RootErrorFallback.tsx and useSessionArtifacts.ts. All 26 inline threads are resolved and the remaining round-6 items are deferred probes at Suggestion level.
CI at this head
Every substantive check passed — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), both Desktop Shell jobs, the web-shell visual capture, web-shell E2E Smoke and review-pr. The rollup reads FAILURE only because two route checks were cancelled. Round 6's Test Plan lines reporting no such file or directory for three test files are a harness path problem on the reviewer's side, not a defect here.
Next step
Answer the triage objection rather than the code: attach a real occurrence for the duplicate-module-copy guard, or split the reload-on-root-retry change out (defensible on its own, and a small diff in main.tsx) and move the diagnostic registry into a tracked issue. Once that is settled the code side needs nothing further from this round.
|
Answering the standing triage objection directly, as the person who filed the occurrence: it is real, and this PR is the fix for it. On the morning of Sep 9, in a dev session running the vite dev server for the standalone shell against What I did not capture was the console component stack, so the duplicate-module-copy mechanism remains an inference from the static shape of the tree — the standalone entry always mounts On the offered alternative of splitting the retry half out: after the review rounds, the two halves are load-bearing on the same incident — the retry performs the recovery that worked, and the diagnostics exist because the incident's mechanism is not yet proven. I'd like to land them together. The occurrence account above is also now in the PR body under "Why it's needed". |
|
Thanks for coming back with the occurrence instead of letting this sit — that's what unblocks it. Template ✓ — every required heading present, with a full Chinese mirror. Problem. Observed, not theoretical. The standalone root boundary showed this guard's message three times in one dev session, the in-place retry could not clear it, and a manual reload recovered every time. That is exactly the second shape the original Stage 1b objection asked for ("a standalone page where 'Try again' failed and a reload recovered"), so I'm treating that objection as answered and withdrawing it. One honest limit, which the body states itself: no component stack was captured, so duplicate module copies is the residual inference — not a proven mechanism. That doesn't undermine the retry half, but it does cap how much weight the registry half can carry. See Approach. Direction. Aligned. "A standalone full-page root boundary should reload rather than re-mount the same broken page state" stands on its own, and it now also stands on a correctness fix: an unconditional reload would strand the shell unauthenticated when the token lives only in module memory. The CHANGELOG has no direct reference to a web-shell error boundary; the nearest signal is the embedding-side recovery fixes (blank sidebar after a window reload), so the area is relevant rather than directly precedented. Size. 1052 changed lines = 316 production + 736 test. It spans Approach. Two things I'd push on. Neither blocks. The diff is three changes, not two. The body describes the guard diagnostics and the reload-on-retry, but there's a third: a new release-gating check in the export-document build ( That third change is the entire cost of one expression. The gate exists because the diagnostic id reads The Risk. No high-risk-path match. Two notes for whoever merges: the unit matrix ran on ubuntu only (the windows/macos Moving on to code review. 🔍 中文说明感谢你补上了真实现场，而不是让这个 PR 挂着——这正是解锁它的关键。 模板 ✓ —— 所有必需标题齐全，并带完整中文镜像。 问题。 是已观测的，不是理论性的。独立版根边界在一个 dev 会话里三次显示了这个守卫的报错，原地重试无法清除，每次只能靠手动刷新恢复。这正是最初 Stage 1b 异议所要的第二种形状（"独立页面中 'Try again' 无效、刷新才恢复"），因此我认定该异议已被回答并撤回。有一个诚实的限度（正文自己也写了）：当时没抓到 component stack，所以"模块双副本"是排除法剩下的推断，而非已证实的机制。这不影响重试那一半，但确实限制了注册表那一半能承载的分量，见「方案」。 方向。 对齐。"独立整页的根边界应当刷新，而不是重新挂载同一副坏掉的页面状态"本身就站得住；而且它现在还多了一层正确性修复：无条件刷新会在 token 仅存于模块内存时让 shell 永久失去认证。CHANGELOG 里没有直接对应 web-shell 错误边界的条目，最接近的信号是嵌入端的恢复类修复（窗口 reload 后侧边栏空白），所以这个领域是相关而非有直接先例。 规模。 1052 行改动 = 316 行生产代码 + 736 行测试。跨 方案。 两点想推一下，都不阻断。 这个 diff 是三处改动，不是两处。 正文写了守卫诊断和重试即刷新，但还有第三处：导出文档构建里新增的发布门禁（ 而这第三处改动，代价完全来自一个表达式。 这道门禁之所以存在，是因为诊断 id 读了 无论如何， 风险。 无高风险路径命中。给合并者的两点提示：单测矩阵只跑了 ubuntu（windows/macos 的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI read the diff against the head commit and wrote down my own approach first. Mine was thinner — reload-on-retry gated on token survivability, plus one module-scoped boolean to say whether this copy ever rendered a provider — so I looked hard for where the fuller version earns its size. It mostly does: the cross-copy branch genuinely needs shared state, and there is no way to detect a foreign copy without No Critical findings. Three Suggestions, all deferrable. 1. An assertion in the export-html e2e test measures the test, not the build. 2. Foreign-copy precedence masks the own-state diagnosis. In the guard, 3. The Test Plan's verify command covers 4 of the 6 suites this diff touches — it omits What I verified so you don't have to re-check it:
One thing worth saying out loud: Test evidenceFrom the PR's own CI on the reviewed commit — nothing was built or executed here (triage no-execute rule). Per-suite counts are quoted from the
The last row matters: 7 tests means the
132 check-runs on the commit, no failures; the skips above are the repo's normal matrix and orchestration shape, not something this PR caused. No Not verified: that a real browser reload actually recovers the observed dev-session crash. No lane can settle that, because the trigger isn't reproducible on demand — the diagnostic registry is the instrument that will answer it on the next occurrence, which is a fair reason to ship it. Sandboxed verification would settle the narrower claim: 中文说明代码审查我先在不看 diff 的情况下写下了自己的方案，再对照阅读。我的方案更薄——重试即刷新（以 token 可存活为前提），加上一个模块级布尔量表示"本副本是否渲染过 provider"——所以我重点看了更完整的这一版是否对得起它的体量。大部分是对的得起的：跨副本分支确实需要共享状态，没有 无 Critical。三条 Suggestion，均可延后。 1. export-html 端到端测试里有一条断言测的是测试自己，不是构建。 2. 外来副本优先会盖掉本副本状态的诊断。 守卫里 3. 测试计划的验证命令覆盖了 6 个受影响套件中的 4 个，漏了 我已核实、你不必再查的部分：
有一点值得明说： 测试证据证据来自本 PR 自己在被审提交上的 CI——这里没有构建或执行任何代码（triage 不执行规则）。逐套件数字引自 该提交上共 132 条 check-run，无失败；表中的 skipped 是仓库正常的矩阵与编排形态，不是本 PR 造成的。没有 未验证：真实浏览器里的刷新是否确实能从那次 dev 会话的崩溃中恢复。没有任何验证通道能确定这一点，因为触发条件无法按需复现——诊断注册表本身就是下次发生时用来回答它的仪器，这也是它值得随 PR 一起合入的合理理由。 沙箱验证可以确定更窄的那个主张： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — solid, safe where it matters, and the one thing I'd cut is named rather than blocking. My independent proposal was thinner than this diff: reload-on-retry gated on token survivability, plus a single module-scoped boolean for "did this copy ever provide". Comparing the two, the extra machinery mostly earns its place — you cannot detect a foreign module copy without state shared across copies, so The half I have no doubt about is the retry. A dead-end error screen whose only recovery is a manual reload is a real paper cut, and the fix is now better than the one-line version I'd have written: the review rounds caught that an unconditional reload strands the shell unauthenticated when the token lives only in module memory, and the gate that came out of it is fail-closed in every path I could trace — including the postMessage case, which persists nothing and therefore correctly takes the in-place reset. Turning "reload is obviously right" into "reload is right except where it would lose your credential" is the substantive improvement in this PR, and it landed because the review pushed, not despite it. On the objection I raised in round 1: I'm withdrawing it, and I want to be explicit about why, because the reviewDecision is about to flip. I asked for one of two things — a real occurrence, or a split. You supplied the occurrence, in the exact shape I said would change my mind, and you were straight that the component stack is missing and the duplicate-copy mechanism is therefore inferred rather than proven. A gate that keeps its objection after being handed what it asked for is not being rigorous, it's just being stubborn. The residual uncertainty is real but it is not a reason to hold the PR: the registry exists precisely to convert that uncertainty into an answer next time, and the retry half stands without it. Six rounds in, the diff is 6.4× what it started as, and AGENTS.md says land only Critical fixes past round five and defer the rest. There is no standing Critical that is a code defect — rounds 5 and 6 both label their Critical as my own unresolved Stage 1b objection, which is now resolved. So the three Suggestions in Stage 2 are deferred, recorded there rather than dropped: the vacuous artifact assertion in the export-html e2e test, the foreign-copy precedence masking the own-state diagnosis (carried as DUP6-1 since round 4, deliberately pinned by a test), and the Test Plan command missing two of the six suites touched. None of them would change my mind about merging; the first is worth a follow-up because an assertion that cannot fail for the reason it states is worse than no assertion. Would I thank or curse whoever wrote this in six months? Thank them for the token-survivability gate and the CI is green on the reviewed commit with nothing in flight, so this approval is pinned to 中文说明信心：4/5 —— 扎实、在关键处是安全的，唯一我想砍掉的部分已点名，但不构成阻断。 我自己的方案比这个 diff 更薄：重试即刷新（以 token 可存活为前提），再加一个模块级布尔量表示"本副本是否 provide 过"。两者对照，多出来的机制大部分对得起它的体积——不借助跨副本共享的状态就无法发现外来模块副本，所以 我毫不怀疑的是重试那一半。一个只能靠手动刷新才能恢复的死胡同报错界面是真实的体验刺点，而这个修复比我原本会写的一行版本更好：评审轮次发现无条件刷新会在 token 仅存于模块内存时让 shell 永久失去认证，由此得到的门禁在我能追踪到的每条路径上都是失败即关闭的——包括 postMessage 那条：它不做任何持久化，因此正确地走了原地重置。把"刷新显然对"变成"刷新是对的，除非它会丢掉你的凭证"，是这个 PR 里实质性的改进；而且它是评审推动出来的结果，不是绕开评审得到的。 关于我在第 1 轮提出的异议：我撤回它，并且想明确说明原因，因为 reviewDecision 即将翻转。我当时要求两件事之一——一个真实发生现场，或者拆分。你提供了现场，且正好是我说过会改变我判断的那种形状；同时你也坦白说明 component stack 缺失，因此双副本机制是推断而非已证实。一个在拿到自己所要的东西之后仍保留异议的门禁，不是严谨，只是固执。残留的不确定性是真实的，但它不是扣住这个 PR 的理由：注册表存在的意义正是在下次发生时把这份不确定性变成答案，而重试那一半不依赖它也能成立。 到第 6 轮，diff 已是初始的 6.4 倍，而 AGENTS.md 说超过 5 轮只合入 Critical 修复、其余延后。目前没有仍然成立的、属于代码缺陷的 Critical——第 5、6 轮都把它们的 Critical 标注为我自己那条未解决的 Stage 1b 异议，而它现已解决。所以 Stage 2 里的三条 Suggestion 予以延后，记录在案而非丢弃：export-html 端到端测试里那条空转的产物断言、外来副本优先盖掉本副本状态诊断（自第 4 轮起以 DUP6-1 携带，且被测试刻意锁定）、以及测试计划命令漏掉 6 个受影响套件中的 2 个。这三条都不会改变我对合并的判断；第一条值得开一个后续，因为一条无法因其声称的原因而失败的断言，比没有断言更糟。 六个月后我会感谢还是抱怨写这段代码的人？会感谢 token 可存活门禁和 被审提交上的 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. ✅ The retry/token-survivability half is fail-closed on every path I could trace, and CI is green on this commit. My earlier request-changes (no observed occurrence) is withdrawn — see the Stage 3 note. Three Suggestions deferred per the round-5+ posture, recorded in Stage 2.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at 23b018e8 (head). C=0 — no new HEAD-verified Critical. All 26 review threads on this PR are resolved, and round 6 posted 0 fresh findings, so this round is a verification pass rather than a new sweep. Nothing already reported is repeated below.
What I verified, and how
setBoundedMapEntry extraction is behaviour-preserving. Every one of the five App.tsx call sites (:4348, :4387, :4404, :4417, :5824) and the useSessionArtifacts.ts one passes its original cap — MAX_ARTIFACT_PANEL_SESSION_STATES and MAX_CACHED_SESSIONS, both still 20. No call site silently lost its budget. The loop-exit change from if (!oldest) to if (oldest === undefined) can only differ for an empty-string key, and every key in the family is built as <cwd>\0<sessionId> or import.meta.url#<rand>, so it is a strict correctness improvement with no reachable behaviour delta.
readTokenFromLocation() is a faithful extraction. The old code produced '' for "no token" and gated on if (fromUrl); the new one produces undefined and gates the same way. #token= with an empty value still falls through to ?token= and then to undefined, exactly as before, so hasReloadSurvivableDaemonToken()'s !== undefined test is equivalent to a truthiness test and cannot report a token that isn't there.
canReload cannot strand the shell unauthenticated. I traced all four boot paths against persistDaemonToken's call sites: URL-borne token (persisted → reload survivable), waitForDaemonTokenMessage / extension iframe (memory only → canReload false → in-place reset, matching the PR's stated scope note), tokenless trusted loopback (!daemonToken → reload, nothing to strand), and sessionStorage-throwing private mode (memory only → reset). The reload URL is built from window.location.href, so a surviving #token= fragment is carried through searchParams.set untouched.
retryMode is a live switch, not a dead one. Written at main.tsx:244, read at RootErrorFallback.tsx:110, and deliberately left unset by the two embeddable call sites (index.tsx:69, WebShellTranscript.tsx:315) — with RootErrorFallback.test.tsx:127 pinning that default in a comment. hasReloadSurvivableDaemonToken likewise has a real read site.
The guard message prefix really is safe. must be used within DaemonWorkspaceProvider appears in exactly two places repo-wide: the throw itself and a substring toThrow at DaemonWorkspaceProvider.test.tsx:494. No exact-match or anchored-regex assertion exists anywhere that the added (${detail}) suffix could break.
replaceState(null, ...) discards nothing. No consumer in the repository reads history.state, so both the new mount-time strip and the reload path are safe on that axis.
Test placement is right, not a style miss. import-meta-guard.mjs lives in packages/web-templates, which has no test script, no vitest config, and no test files at all. scripts/tests/ is the only location wired into npm run test:ci, so that is where the test has to live for it to run.
One finding I raised against myself and then withdrew
I want to record this so it does not get re-litigated in a round 7. A reviewer looking at scripts/tests/export-html-import-meta-guard.test.js will notice that the module-scope mkdtempSync snapshot root is deleted by the file-level afterEach after every test — including the five pure unit tests — and conclude that the second e2e case's snapshotDist() must fail ENOENT, leaving the release-gated dist/ wiped. That is wrong, on two independent counts. cpSync(src, dest, { recursive: true }) creates missing parent directories, so the copy into the deleted root succeeds and the snapshot is rebuilt per case; I confirmed this with a direct probe against a removed mkdtempSync root. And the CI Test lane on this exact head settles it empirically:
✓ scripts/tests/export-html-import-meta-guard.test.js (7 tests) 797ms
✓ export-html import.meta guard (end-to-end) > fails the build when an import.meta read lands in the document entry 381ms
✓ export-html import.meta guard (end-to-end) > fails the build on a fourth import.meta read inside the prebuilt transcript bundle 406ms
Both e2e cases ran — not skipped — and passed. The same measurement retires the spawnSync timeout: 120_000 vs. the suite's testTimeout: 90_000 mismatch as a live flake risk: the observed runtimes are 381 ms and 406 ms, three orders of magnitude inside the ceiling. The unsnapshotted second product is a non-issue too — packages/web-templates/src/generated/ is gitignored, so the e2e half cannot dirty a worktree.
Ruling on the open blocking review
Per the project's own rule that an approval is a claim rather than a default, the one unresolved Critical on this PR is the round-1 triage CHANGES_REQUESTED (review 5148889985), which objects that the duplicate-module-copy diagnostic has no observed occurrence behind it. My ruling is partially answered, remainder is a maintainer product call — not a code defect, and not something further review rounds can settle:
- Its "no reproduction" limb is now answered in kind: the PR body carries a narrated occurrence (root boundary showing this guard three times in one dev morning, in-place retry ineffective, manual reload recovering each time). It is an author narrative rather than the console component stack the triage asked for, and the PR says so plainly — the mechanism is labelled inferred, not proven.
- Its structural limb ("a missing-provider throw cannot happen in the standalone app at all") is consistent with, not contradicted by, that occurrence: both sides agree the throw is unreachable while a single module graph is coherent, which is precisely why the duplicate-copy hypothesis exists and why the diagnostic branch is written to confirm or refute it on the next occurrence.
- Its alternative remedy — split the retry half out and let the registry wait for a real occurrence — was not taken; the diff instead grew to include the export-build
import.metaguard, which the PR description's stated scope does not mention.
Whether roughly 250 production lines of diagnostics for an inferred mechanism clears "nothing speculative" is a judgment about product intent, and the triage itself routed it to a maintainer rather than looping. I am approving on the engineering axis only: this approval does not dismiss that objection, and should not be read as resolving it. A maintainer still owns the shape call.
Convergence note
This is round 6+, with 11 items already recorded as deferred. Per the project's convergence rule, only Critical fixes should land from here — and I have none to add. The deferred set is worth a follow-up issue so it is not silently dropped, in particular the two that are genuinely about diagnostic quality rather than test strength: the stale-id precedence that lets foreignIds report "duplicate copies" for a hot-re-evaluation ghost (already reported as D4-1/DUP6-1), and the new import.meta throw preempting the purpose-written FORBIDDEN_DOCUMENT_INPUTS message on exactly the regression that guard was written for. Neither blocks.
中文说明
在 23b018e8（head）上完成审查。C=0 — 未发现新的、经 HEAD 验证的 Critical。 PR 上 26 条 review thread 全部已解决，第 6 轮也已经是 0 条新发现，因此本轮是验证性复核，不重复任何已报告项。
已验证的内容： setBoundedMapEntry 抽取是行为保持的（App.tsx 五处调用与 useSessionArtifacts.ts 均沿用原有上限 20，!oldest → oldest === undefined 仅在空字符串 key 下有差异，而所有调用点的 key 都不可能为空）；readTokenFromLocation() 的抽取与旧逻辑等价（空值 token 的短路行为一致）；canReload 在四条 boot 路径下都不会让 shell 在刷新后失去认证，且刷新 URL 基于 location.href 构造，会原样保留可能存活的 #token= 片段；retryMode 与 hasReloadSurvivableDaemonToken 都有真实读取点，不是无人设置的死开关；守卫消息前缀在全仓库只有一处子串断言，新增的 (${detail}) 后缀不会破坏任何精确匹配；仓库内无任何 history.state 读取方，两处 replaceState(null, ...) 是安全的；packages/web-templates 没有任何测试设施，把测试放在 scripts/tests/ 是唯一能被 npm run test:ci 执行的位置，不算风格问题。
一条我自己提出又撤回的发现（记录在此以免第 7 轮重复）： 有人会认为 export-html-import-meta-guard.test.js 中文件级 afterEach 在每个测试后删除 mkdtempSync 快照根目录，会导致第二个 e2e 用例 snapshotDist() 抛 ENOENT 并留下被清空的发布产物。这是错的，有两重独立证据：cpSync(src, dest, { recursive: true }) 会创建缺失的父目录（我对已删除的 mkdtempSync 根目录直接做了探针验证）；且本 head 的 CI Test lane 显示该文件 7 个测试全绿，两个 e2e 用例确实执行（未被 skip）、分别耗时 381ms 与 406ms。同一组测量也消除了 spawnSync 120s 超时与套件 90s testTimeout 不一致的实际 flake 风险。此外 packages/web-templates/src/generated/ 已被 gitignore，e2e 部分不会污染工作树。
对未决阻断性审查的裁定： 唯一未决的 Critical 是第 1 轮 triage 的 CHANGES_REQUESTED（review 5148889985），质疑重复模块副本诊断分支背后没有实际观测到的现场。我的裁定是部分已回应，剩余部分属于 maintainer 的产品判断——不是代码缺陷，也不是继续增加审查轮次能解决的问题：「无复现」这一条已由 PR 描述中的现场叙述回应（一个上午三次、原地重试无效、手动刷新恢复），但那是作者叙述而非 triage 要求的 component stack，PR 自己也明确标注机制是推断而非证实；「独立应用树里不可能触发」这一条与该现场并不矛盾，双方都认为模块图一致时不可能触发，这正是重复副本假设存在的理由；而 triage 给出的替代方案（拆出刷新那一半、诊断注册表等真实现场）未被采纳，diff 反而扩大到包含导出构建的 import.meta 守卫，且 PR 描述的范围并未提及这一半。约 250 行生产代码用于诊断一个推断中的机制是否符合「不做投机代码」，属于产品意图判断，triage 本身也已将其转交 maintainer。我仅在工程层面批准：本次批准不构成对该异议的撤销，也不应被理解为已解决它，改动形态的最终判断仍归 maintainer。
收敛提示： 已是第 6 轮以上、11 条延后项在册。按项目收敛规则，从这里开始只应落地 Critical 修复，而我没有新的 Critical。建议把延后项开一个 follow-up issue 以免被静默丢弃，尤其是两条真正关于诊断质量而非测试强度的：foreignIds 中的过期 id 会让热重载残留被误报为「重复模块副本」（已作为 D4-1/DUP6-1 报告），以及新的 import.meta 抛错会在该守卫本要防的那个回归上抢先于专门写的 FORBIDDEN_DOCUMENT_INPUTS 诊断。两者都不阻断。
…-css One both-side change: packages/web-templates/src/export-html/build.mjs. main's #11421 added the import.meta guard (a new import plus the post-build findUnexpectedImportMeta throw) while this branch added the transcript CSS entry filter import at the same spot. Both imports are kept and the guard block is untouched. The budget constants stay at this branch's JS-only values (1,870,000 warning / 1,930,000 max against a measured 1,833,894 bytes of renderer JS), and the comment now also records main's pre-split combined measurement of 4,133,282 bytes that #11372 raised them for, so that history is not lost by the split. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Released in v0.23.3. |
4 participants
What this PR does
Two changes to the standalone Web Shell's failure surface. First, the strict
useDaemonWorkspaceguard now reports which situation it detected, using a small diagnostic registry (keyed onglobalThis, never used to share context): each copy of the provider module records a URL-bearing id and whether it rendered with a live client, so the error distinguishes "no provider has rendered" / "a provider rendered from a different module copy (duplicate module instances, both ids included)" / "this copy rendered but the consumer is outside its live subtree" / "this copy rendered without an active client (autoConnect={false})". The registry is capped so dev hot re-evaluation cannot grow it unbounded. Second, the standalone root error boundary's retry now reloads the page — but only when a reload cannot strand a credential: when no token was resolved at boot (tokenless trusted loopback) or when a token survives in the URL / per-tab storage (a newhasReloadSurvivableDaemonToken(), sharing one URL-grammar parse withgetDaemonToken()); otherwise it falls back to the previous in-place boundary reset, The reload carries the live theme/language into the URL first (session switches strip those one-shot params), and the button label matches the action ("Reload page" / "重新加载" vs "Try again" / "重试").Why it's needed
Observed occurrence (the reason this PR exists): in a dev session (
vitedev server serving the standalone shell againstqwen serve), the root boundary showed this guard's error three times in one morning; an in-place retry could not help and a manual page reload recovered each time. The raw guard message being visible on screen pins a DEV build, and static analysis shows the guard cannot fire in the standalone tree while a single module graph is coherent — the provider wraps the whole app and always produces a context value — which points at a duplicatedDaemonWorkspaceContext(two copies of the provider module live in one page). No console component stack was captured, so the mechanism is inferred, not proven; the diagnostic branch exists precisely to confirm or refute it on the next occurrence instead of guessing again. The reload-on-retry half exists because boundary reset re-mounts the same broken module graph and throws again — reload was the only recovery that worked. Review then found the unconditional version could strand the shell unauthenticated (token only in memory, storage persist failed, URL token stripped at boot), so the reload is now gated on token survivability; the in-memory-token case keeps the old re-mount behavior, which does recover.Reviewer Test Plan
How to verify
Run the touched unit suites from
packages/web-shell:npx vitest run client/daemon/workspace/DaemonWorkspaceProvider.test.tsx client/main.test.tsx client/components/RootErrorFallback.test.tsx client/config/daemon.test.ts(62 tests, all green locally). The provider suite covers every guard branch (no provider / foreign copy / same copy but outside / rendered-without-client / duplicate takes precedence when both copies rendered / registry dedup across recomputes / registry cap);main.test.tsxcovers retry-reloads-when-token-survives and retry-resets-in-place-when-not (asserting reload was not called and the tree re-mounted);daemon.test.tscovershasReloadSurvivableDaemonTokenacross hash/query/persisted/empty/throwing-storage. Mutation checks performed locally: removing the reload-survivability guard, the label plumbing, the shared harness's try/catch, the URL-bearing id, the helper-local root (with an afterEach leak probe), the foreign-copy precedence, or the registry cap each turns the corresponding test red; reverting restores green. Reviewers can also confirm the guard's message prefix is unchanged, so the existing substring assertions and thebuild-artifact.test.tschecks still hold (transcript JS measured at 1,180,562 bytes, under the 1,300,000 ceiling;import.meta.urlsurvives the ES lib build).Evidence (Before & After)
N/A for visuals (no layout change; the fallback looks identical apart from the retry label when it reloads). Behavior evidence: the observed failure was "root fallback with this guard's message, recoverable only by reload", seen 3x in one dev session; after this PR the same screen's retry performs that recovery itself when safe, and the error message now names the failure class.
Tested on
Environment (optional)
npx vitest rununderpackages/web-shell; eslint/prettier clean on the changed files;vite build --config vite.lib.config.ts --mode transcriptsucceeds with the guard intact in the bundle. (The standalonevite buildin this worktree is blocked by a pre-existing missing@tanstack/react-tableinstall, unrelated to this diff.)Risk & Scope
globalThisand is diagnostic-only (context values are never shared through it), bounded to 8 entries.WebShellTranscript's embedded boundary intentionally keeps its in-place retry; thechrome-extensioniframe path depends on the sidepanel re-posting the token on iframe load and takes the reset path.Linked Issues
References #11100 (the strict-guard failure class in provider-less transcript rendering; not closed by this PR — its "no provider has rendered" branch is exactly what that issue's export-document scenario would hit).
中文说明
这个 PR 做了什么
对独立 Web Shell 的失败兜底做了两处改动。第一，严格的
useDaemonWorkspace守卫现在会报告它命中的具体情形：通过一个小型诊断注册表（挂在globalThis上，绝不用于共享 context），Provider 模块的每个副本记录一个带 URL 的 id 以及它是否带着可用 client 渲染过，从而让错误信息区分「从未渲染过 Provider」/「Provider 来自另一个模块副本（模块双实例，消息带两个 id）」/「本副本渲染过但消费者在其存活子树之外」/「本副本渲染了但没有活跃 client（autoConnect={false}）」。注册表有上限，dev 热重载不会让它无限增长。第二，独立版根错误边界的「重试」现在会刷新页面——但只在刷新不可能丢失凭证的前提下（启动时未解析到 token 的免认证回环场景，或 token 存在于 URL / 按标签页持久化存储中——由新的hasReloadSurvivableDaemonToken()判定，与getDaemonToken()共享同一份 URL 解析）；否则回退为原来的原地边界重置。刷新前会把当前的 theme/language 写回 URL（会话切换会把这些一次性参数抹掉），且按钮文案与行为一致（「Reload page」/「重新加载」 对 「Try again」/「重试」）。为什么需要
真实发生过的现场（本 PR 的起因）：在一个 dev 会话里（vite dev server 提供独立 shell、代理到
qwen serve），根边界一个上午出现了三次这个守卫的报错；原地重试无效，每次只能手动刷新页面恢复。屏幕上能直接看到守卫原文这一点锁定了当时是 DEV 构建；而静态分析表明，在模块图一致的独立应用树里这个守卫不可能触发——Provider 包着整个应用且总是产出 context 值——因此指向DaemonWorkspaceContext双实例（同一页面里存在两份 Provider 模块）。当时没有抓到 console 的 component stack，所以这个机制是推断而非已证实；诊断分支的意义正在于下次发生时直接证实或证伪，而不是再猜一次。重试即刷新那一半存在的原因是：边界重置只会重新挂载同一副坏掉的模块图并再次抛错——刷新是唯一有效的恢复手段。评审随后发现无条件刷新在 token 只存在于内存（持久化失败、URL token 启动时被抹掉）时会让 shell 永久失去认证，因此刷新现在以 token 可存活为前提；内存 token 的场景保留原来的重新挂载行为，那条路径本来就能恢复。Reviewer 测试计划
如何验证
在
packages/web-shell下运行受影响的单测：npx vitest run client/daemon/workspace/DaemonWorkspaceProvider.test.tsx client/main.test.tsx client/components/RootErrorFallback.test.tsx client/config/daemon.test.ts（62 个测试，本地全绿）。Provider 套件覆盖守卫的全部分支（无 Provider / 外来副本 / 本副本但子树外 / 渲染了但无 client / 两个副本都渲染过时重复副本优先 / 重复渲染下去重 / 注册表上限）；main.test.tsx覆盖「token 可存活时重试刷新」和「不可存活时原地重置」（断言未调用 reload 且子树重新挂载）；daemon.test.ts覆盖hasReloadSurvivableDaemonToken的 hash/query/持久化/无 token/存储抛异常五种情形。本地做过的变异检查：分别移除刷新存活保护、标签 plumbing、共享 harness 的 try/catch、带 URL 的 id、helper 局部 root（配合 afterEach 泄漏探针）、外来副本优先分支、注册表上限，对应的测试都会变红；还原后恢复绿色。Reviewer 也可以确认守卫消息前缀未变，现有子串断言和build-artifact.test.ts的检查仍然成立（transcript JS 实测 1,180,562 字节，低于 1,300,000 上限；import.meta.url在 ES lib 构建中保留）。证据（Before & After）
视觉上 N/A（布局无变化，仅重试按钮文案在刷新时不同）。行为证据：观测到的故障是「根兜底显示该守卫报错、只能刷新恢复」，一个 dev 会话里出现 3 次；本 PR 之后同一个界面的重试在安全时会自行完成这次恢复，且报错信息会直接指明失败类别。
测试环境
运行环境（可选）
在
packages/web-shell下npx vitest run；改动文件 eslint/prettier 干净；vite build --config vite.lib.config.ts --mode transcript构建成功且守卫完整保留在产物中。（本 worktree 的独立版vite build被既有的@tanstack/react-table未安装问题阻塞，与本 diff 无关。）风险与范围
globalThis上、仅用于诊断（绝不通过它共享 context 值），上限 8 条。WebShellTranscript的嵌入式边界刻意保留原地重试；chrome-extensioniframe 路径依赖 sidepanel 在 iframe load 时重发 token，走的是重置路径。关联 Issue
引用 #11100（无 Provider 渲染 transcript 时的同类严格守卫失败；本 PR 不关闭它——该 issue 的导出文档场景命中的正是「no provider has rendered」这个分支）。