feat(web-shell): add web previews with saved delivery history - #11276
Conversation
E2E verification reportVerified on macOS with Node.js 22.22.2 and Chromium. These results apply to the submitted implementation; Windows, Linux, and WebKit were not verified locally. BaselineThe globally installed qwen 0.23.0 was exercised with the real Artifact publisher. Publishing v1 and then v2 from the same source retained one stable descriptor and replaced the output. There was no independent saved version, and the proposed saved-content route returned HTTP 404. Actual publication, persistence, daemon, and browserThe test-script fallback invoked the actual built Artifact publisher, trusted bridge ingestion, artifact store, conversation recorder, JSONL restore, daemon routes, and Web Shell. The conversation was a deterministic authored fixture, not model-generated output. No HTTP responses were mocked and no model requests were sent.
After the final viewer isolation correction, a fresh actual daemon run verified saved v1, saved v2, and a generic HTML artifact. Each rendered its expected heading and working counter. Each then attempted self-navigation to a separate real HTTP listener: all three were blocked by Independent browser review reproduced the policy regression before the correction and confirmed the fix: the old narrow host policy blocked navigation, the broadened host policy with an unwrapped offline viewer allowed it, and the final isolated viewer blocked it. Parent-policy tampering and malicious title probes were also blocked. The review finding was closed with no new findings. All temporary daemons and HTTP listeners were stopped; direct connection probes confirmed their ports were closed. Existing user daemons were left running. Automated coverage
Counts are per focused run and overlap; they are not a unique aggregate. Package unit tests ran from their respective package directories. Browser history suites use daemon API fixtures and do not establish real persistence by themselves; the real-daemon checks above cover that separately. Cross-runtime ownership, matching-file primary-runtime fallback rejection, descriptor validation, and size limits are covered by focused tests, not claimed as additional real-daemon E2E observations. ScopeSaved versions cover newly delivered self-contained HTML, not arbitrary website capture or transient browser state. The existing default 200-record artifact retention limit remains. Snapshot bytes remain local to the owning runtime. 中文摘要：真实发布、持久化、重启恢复、原消息版本打开、异常文件拒绝及最终浏览器隔离均已通过。两份已保存版本与普通 HTML 均可正常交互，导航攻击的目标请求数为零。上表明确区分真实 daemon 验证与模拟 API 的浏览器测试；未验证 Windows、Linux 和 WebKit。 |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
6 |
7 |
— Qwen Code · serve A/B
🖼️ 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 8 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 |
|
Thanks for the PR — this is a substantial one, so here is the gate pass before the code review. Template ✓ — every required section is present, including the before/after evidence table and the Chinese translation. Problem — this is a capability gap rather than a bug, and it is evidenced rather than theoretical: the right panel can render offline HTML but cannot open a running dev server, and the "latest publication URL" model genuinely loses what an earlier turn delivered. The before/after table names a concrete failure (publish v1 then v2 from one source, and v1 is gone). No reproduction is needed for a feature of this shape, so this passes. Direction — aligned. The right panel already carries review / sideTask / terminal tabs and an artifact panel that sandboxes HTML, so a preview tab and per-delivery version history continue an existing trajectory rather than opening a new one. Upstream signal: Anthropic's CHANGELOG has an active Artifact surface (several recent artifact entries, including one about reading an artifact that isn't yours now requiring confirmation), so owner-scoped artifact reads are a live concern in the same area. No direct "web preview panel" reference, but the area is clearly relevant. One thing that does need a human owner rather than a bot verdict: this touches a process-global security policy and public contract — the shell document's Size — core paths are touched (
The author is a repo admin, so this is maintainer-authored and exempt from the two-tier core gate; 922 production lines is also below the 1000-line large-PR advisory. Reporting the numbers for awareness only — nothing here blocks on size. Approach — the mechanics look right, but I would push on scope. This is two separable features plus a third ride-along:
So: if you cut 80% of the scope, the live preview alone would already deliver most of the "iterate on a running page" value at a fraction of the surface. The saved-version half is the more interesting feature but also the one carrying the security and contract weight. Would landing them as two PRs be feasible? Not a blocker — asking because it would make both much easier to review and to revert. Risk — the revert-history path check found no matches, so no elevated-risk-path signal. The real risk is the one the PR names itself: Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 体量不小，先给出门禁结论，再看代码。 模板 ✓ —— 必填章节齐全，包含 before/after 证据表和中文说明。 问题 —— 这是能力缺口而非 bug，而且有实证、不是理论性加固：右侧面板能渲染离线 HTML，但打不开正在运行的开发服务器；「最新发布 URL」的模型确实会丢失早前轮次交付的内容。before/after 表格点名了一个具体失效场景（同一源发布 v1 再发布 v2，v1 就没了）。这类特性不需要复现步骤，此项通过。 方向 —— 对齐。右侧面板已经有 review / sideTask / terminal 标签页，产物面板也已对 HTML 做沙箱渲染，所以预览标签页与按次保存的历史版本是延续既有路线，而不是新开一条。上游信号：Anthropic 的 CHANGELOG 中 Artifact 是活跃面（近期多条 artifact 相关条目，其中一条正是关于读取不属于你的 artifact 现在需要确认），说明「按所有者限定 artifact 读取」在同一领域是当前的关注点。没有直接的「网页预览面板」条目，但该领域确实相关。 有一点需要人来定夺、而不是由机器人下结论：本 PR 触及进程级安全策略与公共契约——shell 文档的 规模 —— 触及核心路径（
作者是仓库 admin，属维护者自建 PR，豁免两级核心门禁；922 行生产代码也低于 1000 行的大 PR 提示线。此处仅为知会而列出数字——不因体量而阻塞。 方案 —— 机制看起来是对的，但我想就范围提个问题。这实际上是两个可拆分的特性，外加一个搭车改动：
所以：如果砍掉 80% 的范围，单是实时预览就已经能以极小的面积交付「在运行中的页面上迭代」的大部分价值。保存历史版本这一半更有意思，但也是承载安全与契约权重的那一半。拆成两个 PR 是否可行？这不是阻塞项——之所以问，是因为拆开后两者都更容易审查、也更容易回滚。 风险 —— 回滚历史的路径检查无命中，因此没有高风险路径信号。真正的风险正是 PR 自己点出的那条： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": did not execute the spec (dev server + browser launch) to observe the frame-ancestors violation's source frame at runtime; the attribution above is establishe…; "agent reverse-audit (round 1)": the README block's live-preview behavioural claims (hot reload, refresh returning to the entry URL, 390px/desktop toggle, cross-origin navigation not synced to …; "agent reverse-audit (round 2)": whether the daemon serves any attacker-influenced HTML at its own origin, and therefore whether framing a workspace.baseUrl origin-alias ( http://127.0.0.1:PO…; "agent reverse-audit (round 1)": did not re-execute the file under CI settings ( CI=1 → non-reused webServer, retries: 2 , forbidOnly ) or concurrently with the sibling new spec web-shell.s….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：did not execute the spec (dev server + browser launch) to observe the frame-ancestors violation's source frame at runtime; the attribution above is establishe…；"agent reverse-audit (round 1)"：the README block's live-preview behavioural claims (hot reload, refresh returning to the entry URL, 390px/desktop toggle, cross-origin navigation not synced to …；"agent reverse-audit (round 2)"：whether the daemon serves any attacker-influenced HTML at its own origin, and therefore whether framing a workspace.baseUrl origin-alias ( http://127.0.0.1:PO…；"agent reverse-audit (round 1)"：did not re-execute the file under CI settings ( CI=1 → non-reused webServer, retries: 2 , forbidOnly ) or concurrently with the sibling new spec web-shell.s…。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
wenshao
removed
the
autofix/takeover
|
🤖 AutoFix crashed before it could evaluate the feedback (attempt 1/100) — it will retry on the next scan. AutoFix failed before producing a verified commit (the run crashed or timed out before it could explain why). 中文说明🤖 AutoFix 在评估反馈之前崩溃（第 1/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34128630004 🧠 Handled by Qwen Code · model/模型 |
|
Review repair and validation for 18f87ca (merged main at 63578c7). The three Critical reports were reproduced before repair and rechecked against actual source modules afterward: all ten HTML compatibility cases pass, a successful publication survives a failed history write through the real transcript → bridge → artifact store, and foreign-session preview reads omit the active session clientId while local reads retain it. The additional ArtifactPanel integration test checks source-session propagation into the SDK call.
The browser tests verify real Vite modules, storage, HMR, widths, frame retention, referrer suppression, blocked popups, direct redirect/script navigation, explicit descendant anti-framing, and immutable-card restore with inline controls. Offline interaction/data image/data WAV checks run under the full response CSP. Session/artifact HTTP responses in these Playwright scenarios are fixtures. The publication/bridge reproductions separately use real implementations; this is not claimed as a complete real-daemon side-task delivery E2E. Earlier real-daemon snapshot/restart evidence and PR screenshots remain available above. The source tree was not committed until two consecutive complete reverse audits were clean after all fixes. The commit hook left the audited tree unchanged (tree a28b1df8ffc8ef7349634341ce5d73521a9b6ae2). An independent reviewer also found no remaining Critical. Each review thread is answered individually; R1-5 (tiny escaper extraction), R1-19 (generic anchor/navigation behavior) and R1-27 (runtime migration/classification contract) remain explicitly discussed rather than falsely marked implemented. The unrelated validator expansion was removed, keeping main’s existing validation contract. Local platform: macOS, Node 22.22.2, Chromium. The post-fix source scripts and these browser scenarios do not rely on model-generated responses. The conflicting AutoFix takeover was paused before work to prevent competing unaudited pushes; the new GitHub CI run is separate from these local results. |
|
Fixed the CI route drift guard failure in c575fdf. The new saved-page content endpoint increases the registered route inventory from 69 to 70. The previous update corrected the telemetry behavior test but missed the separate registration drift guard; this commit updates that remaining count and preserves the exact route-to-catalog equality assertion. Local validation: reproduced the original failure on 18f87ca before changing the test, then verified both telemetry test files pass (69 tests). Root build, typecheck, bundle, focused ESLint and Prettier all pass. The rebuilt CLI starts successfully. Two consecutive reverse-audit passes found no further issue in this correction, and no production code changed. Remote validation is now complete for c575fdf: Qwen Code CI passed, including Linux full unit tests, static checks, no-AK integration tests and the Web Shell browser smoke gate. The original route drift test passed in CI; the CLI suite finished with 28,989 passed and 90 skipped. Web-shell Visuals, Serve A/B, SDK Java and tui-parity also passed. |
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": confirming whether any shipped flow forks or replays a session journal under a runtime base dir that differs from the one that wrote the snapshot bytes (write s…; "agent 1b": none — the walk completed within budget.; chunk 4: I did not execute packages/core vitest for artifact-snapshots.test.ts / artifact-tool.test.ts / html.test.ts ; I verified the new html.test.ts cases by…; "agent reverse-audit (round 4)": none of my checks were cut short — but two verifications I chose not to run are owed scope rather than passes: isTrustedArtifactToolUpdate 's per-batch vs per-…; "agent reverse-audit (round 4)": whether the artifact *list* scope and SavedWebPreview 's clientId-narrowed content read can diverge (list-wide via App.tsx:5388 / 5515 calling listSessionAr…, and 2 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
docs/design/web-shell/web-shell-preview-snapshots.md:49 — [review] The dedup this doc specifies removes the…packages/acp-bridge/src/sessionArtifacts.test.ts:6217 — [review] The restore-trust gate this diff adds depends on the tool…packages/cli/src/serve/routes/session.ts:6375 — [review] The header-absent branch (clientId === undefined) is the…packages/cli/src/serve/web-shell-static.test.ts:24 — [review] The rewritten CSP test is the repo's only oracle for…packages/core/src/services/session-artifact-persistence.ts:116 — [review] Two of the new whitelist's guards - kind !== 'html' and…packages/core/src/tools/artifact/artifact-snapshots.test.ts:67 — [review] The 'oversized' test never reaches the size guard it…packages/core/src/tools/artifact/artifact-snapshots.ts:25 — [review] The snapshot directories are created with no mode, so they…packages/core/src/tools/artifact/artifact-snapshots.ts:30 — [review] The mode: 0o600 on the snapshot file is the only thing…packages/core/src/tools/artifact/artifact-snapshots.ts:59 — [review] The descriptor persists an absolute file:// URL bound to…packages/core/src/tools/artifact/artifact-tool.ts:262 — [review] Nothing in the suite pins that the saved snapshot is the…packages/web-shell/client/App.tsx:4620 — [review] Manually opened preview tabs are all titled…packages/web-shell/client/App.tsx:6343 — [review] Re-clicking an artifact card replaces an already-open…packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3362 — [review] artifactPreviewDocument - a full DOM parse, a serialize,…packages/web-shell/client/components/artifacts/TurnOutputs.tsx:464 — [review] The saved-version card label in the transcript has no test…packages/web-shell/client/components/artifacts/artifactUtils.test.ts:328 — [review] The new test for artifactPreviewDocument never exercises…packages/web-shell/client/components/artifacts/turnOutputSelectors.ts:76 — [review] The saved/latest card dedup compares the raw publishedUrl…packages/web-shell/client/components/artifacts/turnOutputSelectors.ts:84 — [review] The new de-duplication pass mutates the map that…packages/web-shell/client/components/preview/web-preview.test.ts:24 — [review] The credential clause of the preview allow-list guard…packages/web-shell/client/e2e/web-shell.saved-preview.spec.ts:66 — [review] The @smoke spec builds its sandbox fixture in the…
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：confirming whether any shipped flow forks or replays a session journal under a runtime base dir that differs from the one that wrote the snapshot bytes (write s…；"agent 1b"：none — the walk completed within budget.；chunk 4：I did not execute packages/core vitest for artifact-snapshots.test.ts / artifact-tool.test.ts / html.test.ts ; I verified the new html.test.ts cases by…；"agent reverse-audit (round 4)"：none of my checks were cut short — but two verifications I chose not to run are owed scope rather than passes: isTrustedArtifactToolUpdate 's per-batch vs per-…；"agent reverse-audit (round 4)"：whether the artifact *list* scope and SavedWebPreview 's clientId-narrowed content read can diverge (list-wide via App.tsx:5388 / 5515 calling listSessionAr…，另有 2 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
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.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D8-2 retain-on-upsert fault isolation, packages/acp-bridge/src/sessionArtifacts.ts:486 — already reported (round-7 deferral at the same line, review 5159878427)
- D8-3 per-runtime snapshot base-dir wiring unpinned, packages/acp-bridge/src/bridge.ts:7132 — already reported (round-7 deferral at packages/cli/src/serve/run-qwen-serve.ts:6123, review 5159878427)
- D8-8 O_NOFOLLOW flag collapse on Windows, packages/core/src/tools/artifact/artifact-snapshots.ts:92 — already reported (round-7 deferral at artifact-snapshots.ts:97, review 5159878427)
- D8-14 auto-open suppression assertion vacuous on CI, packages/core/src/config/config.test.ts:6040 — already reported (round-7 deferral at the same line, review 5159878427)
- D8-15 English-only design docs, docs/design/web-shell/web-shell-live-preview.md:1 — already reported (round-7 deferral at the same path, review 5159878427)
- D8-16 transient I/O collapsed into the terminal 404 class, packages/cli/src/serve/routes/session.ts:6346 — already reported (round-7 deferral at packages/core/src/tools/artifact/artifact-snapshots.ts:82, review 5159878427)
- D8-25 snapshot write ignores the abort signal, packages/core/src/tools/artifact/artifact-tool.ts:264 — already reported (round-7 deferral at the same line, review 5159878427)
Not reviewed: reverse audit — stopped after the round 1/2 convergence pair: both members reported findings, so the pair is one reporting round and the two-consecutive-dry-rounds criterion was never reached; rounds 3-5 of this plan's 5-round cap did not run. The pair's 26 findings WERE verified in the repair round (10 confirmed high, 5 confirmed low, 11 rejected).
Not reviewed: verifier-incidental findings inc1 (routes/session.ts:6341) and inc2 (local-publisher.ts:36-38) — reported by round-2 verifiers and never ruled on by a verification round, so they stay unverified and terminal-only.
Not reviewed: issue-fidelity — closing-issue discovery unavailable on this runner (gh 2.45.0; the subcommand requires >= 2.72.0 for closingIssuesReferences), so the linked-issue set is UNKNOWN rather than empty; the PR declares no linked issue and the motivating-incident replay did run.
Not reviewed: build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — "Test (windows-latest, Node 22.x)" was skipped in CI, so no platform exercised the Windows-only paths; the O_NOFOLLOW finding was verified against a model of win32 constants, not win32.
Not reviewed: browser e2e — packages/web-shell/client/e2e/web-shell.live-preview.spec.ts, web-shell.pdf-preview.spec.ts and web-shell.saved-preview.spec.ts are excluded from the vitest run and no browser was available to this reviewer, so the PR's browser-level containment evidence was not re-executed as a suite (individual verifier probes did drive real Chromium for three claims).
Not reviewed: test-efficacy probe — all 25 probed files returned inconclusive (reason: no-output, from the vitest global-setup prerequisite guard), so no mutant or hunk survivor was measured and the harness control never ran; the mutation-coverage conclusions in this review come from hand-built verifier probes instead.
Not reviewed: did not verify whether create-sub-session.ts:1131's unpinned new SessionService(boundWorkspace).removeSession(...) runs inside a runWithWorkspaceRuntimeStorage scope at its call site — if it does not, a sub-session deletion resolves the primary runtime and silently no-ops the new snapshot release — stopped at the agent tool budget.
Not reviewed: verifying that config.getSessionId() (the snapshot reference key written at artifact-tool.ts:270) cannot diverge from the SessionArtifactStore's fixed this.sessionId used by every retain/release call — Config.startNewSession() reassigns it — stopped at the agent tool budget.
Not reviewed: whether toolName on a tool-recorded artifact can be caller-supplied through record_artifact params, which decides whether a model-driven record_artifact call can mint a full getWebPreviewSnapshotId descriptor and drive retain/delete against an arbitrary snapshot id — stopped at the agent tool budget.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": determining the numeric limit passed to extractCappedArtifactInputs for tool_call_update artifacts (traced the parameter to bridgeClient.ts:443 / :3334 …; "agent reverse-audit (round 2)": verifying that config.getSessionId() (the snapshot reference key written at artifact-tool.ts:270 ) cannot diverge from the SessionArtifactStore 's fixed th…; "agent reverse-audit (round 2)": running packages/core/src/tools/artifact/html.test.ts to confirm the ten new characterization cases pass — I hand-evaluated each against the unchanged valida…; "agent reverse-audit (round 1)": Firefox behaviour of connect-src 'self' against same-origin ws:// — the installed firefox-1532 build would not launch under this tree's playwright-core …; "agent reverse-audit (round 2)": did not confirm referential stability of useWorkspace().client in daemon-react-sdk (it sits in the effect dep array; I inferred stability from the pre-exist…, and 6 more.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/core/src/services/sessionService.ts:4130 — [probe] Critical [fails-closed] [new-surface] forkSession aborts with a raw ENOENT once a durable snapshot descriptor's bytes are missing, permanently unbranching the sessionpackages/acp-bridge/src/sessionArtifacts.test.ts:6594 — [probe] D8-4 the runtime-capture test passes runtimeBaseDir explicitly, so the construction-time fallback it is named after is never reachedpackages/core/src/services/sessionService.ts:3232 — [probe] D8-5 the snapshot collection scan swallows stream/IO failures with no diagnostic, so the resulting permanent leak is invisiblepackages/core/src/tools/artifact/artifact-snapshots.test.ts:263 — [probe] D8-6 the lease-loss test never asserts the reference directory survived, so the pre-unlink ownership recheck is unpinnedpackages/core/src/tools/artifact/artifact-snapshots.test.ts:78 — [probe] D8-7 the oversize case cannot fail on the size guard it names, because truncate destroys the bytes so the sha256 mismatch rejects firstpackages/acp-bridge/src/sessionArtifacts.ts:565 — [probe] D8-9 the droppedArtifacts operand of the new reclamation call has no test, so removing it ships greenpackages/core/src/services/sessionService.test.ts:5961 — [probe] D8-10 the fork-rollback test cannot discriminate operation-scoped from session-scoped release despite its namepackages/web-shell/client/App.tsx:6485 — [probe] D8-11 no test pins sourceSessionId propagation onto the artifact tab; the App-level test is masked by an identical mock session idpackages/web-shell/client/components/preview/SavedWebPreview.tsx:68 — [probe] D8-12 the result?.key === key staleness guard is load-bearing but no test goes red when it is removedpackages/web-shell/client/e2e/web-shell.saved-preview.spec.ts:161 — [probe] D8-13 the route handler can throw before fulfilling, turning a clear assertion failure into an unexplained 60s timeoutpackages/core/src/services/session-artifact-persistence.test.ts:115 — [probe] D8-17 three getWebPreviewSnapshotId rejection conditions are unpinned; all four mutations survive every suitepackages/core/src/tools/artifact/artifact-snapshots.test.ts:195 — [probe] D8-18 the session-scoped retain tolerance production depends on is never asserted anywhere in the suitepackages/web-shell/client/App.test.tsx:3343 — [probe] D8-19 the only open/persist/reload test asserts nothing about the URL that survived the round trippackages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3390 — [probe] D8-20 the preview document is rebuilt inline on every render while the sibling call site added in the same PR memoizes itpackages/web-shell/vite.config.ts:93 — [probe] D8-21 the dev CSP duplicates ten shipped directives with nothing pinning the copy to the builder it namespackages/web-shell/client/App.tsx:5967 — [probe] D8-22 restored preview tabs mount eagerly while the terminal case two branches below deliberately deferspackages/web-shell/client/e2e/web-shell.live-preview.spec.ts:288 — [probe] D8-23 the 'preserves the working preview' assertion is load-invariant because the fixture rewrites the value on every loadpackages/web-shell/client/e2e/web-shell.live-preview.spec.ts:377 — [probe] D8-24 the recorded-link fixture fails two independent predicate arms, so removing either leaves the suite green
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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：reverse audit — stopped after the round 1/2 convergence pair: both members reported findings, so the pair is one reporting round and the two-consecutive-dry-rounds criterion was never reached; rounds 3-5 of this plan's 5-round cap did not run. The pair's 26 findings WERE verified in the repair round (10 confirmed high, 5 confirmed low, 11 rejected).
未审查（原文为英文）：verifier-incidental findings inc1 (routes/session.ts:6341) and inc2 (local-publisher.ts:36-38) — reported by round-2 verifiers and never ruled on by a verification round, so they stay unverified and terminal-only.
未审查（原文为英文）：issue-fidelity — closing-issue discovery unavailable on this runner (gh 2.45.0; the subcommand requires >= 2.72.0 for closingIssuesReferences), so the linked-issue set is UNKNOWN rather than empty; the PR declares no linked issue and the motivating-incident replay did run.
未审查（原文为英文）：build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — "Test (windows-latest, Node 22.x)" was skipped in CI, so no platform exercised the Windows-only paths; the O_NOFOLLOW finding was verified against a model of win32 constants, not win32.
未审查（原文为英文）：browser e2e — packages/web-shell/client/e2e/web-shell.live-preview.spec.ts, web-shell.pdf-preview.spec.ts and web-shell.saved-preview.spec.ts are excluded from the vitest run and no browser was available to this reviewer, so the PR's browser-level containment evidence was not re-executed as a suite (individual verifier probes did drive real Chromium for three claims).
未审查（原文为英文）：test-efficacy probe — all 25 probed files returned inconclusive (reason: no-output, from the vitest global-setup prerequisite guard), so no mutant or hunk survivor was measured and the harness control never ran; the mutation-coverage conclusions in this review come from hand-built verifier probes instead.
未审查（原文为英文）：did not verify whether create-sub-session.ts:1131's unpinned new SessionService(boundWorkspace).removeSession(...) runs inside a runWithWorkspaceRuntimeStorage scope at its call site — if it does not, a sub-session deletion resolves the primary runtime and silently no-ops the new snapshot release — stopped at the agent tool budget.
未审查（原文为英文）：verifying that config.getSessionId() (the snapshot reference key written at artifact-tool.ts:270) cannot diverge from the SessionArtifactStore's fixed this.sessionId used by every retain/release call — Config.startNewSession() reassigns it — stopped at the agent tool budget.
未审查（原文为英文）：whether toolName on a tool-recorded artifact can be caller-supplied through record_artifact params, which decides whether a model-driven record_artifact call can mint a full getWebPreviewSnapshotId descriptor and drive retain/delete against an arbitrary snapshot id — stopped at the agent tool budget.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：determining the numeric limit passed to extractCappedArtifactInputs for tool_call_update artifacts (traced the parameter to bridgeClient.ts:443 / :3334 …；"agent reverse-audit (round 2)"：verifying that config.getSessionId() (the snapshot reference key written at artifact-tool.ts:270 ) cannot diverge from the SessionArtifactStore 's fixed th…；"agent reverse-audit (round 2)"：running packages/core/src/tools/artifact/html.test.ts to confirm the ten new characterization cases pass — I hand-evaluated each against the unchanged valida…；"agent reverse-audit (round 1)"：Firefox behaviour of connect-src 'self' against same-origin ws:// — the installed firefox-1532 build would not launch under this tree's playwright-core …；"agent reverse-audit (round 2)"：did not confirm referential stability of useWorkspace().client in daemon-react-sdk (it sits in the effect dep array; I inferred stability from the pre-exist…，另有 6 条。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 18 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.2)
wenshao
removed
the
autofix/takeover
…ew-history # Conflicts: # packages/cli/src/acp-integration/acpAgent.test.ts # packages/cli/src/serve/server/telemetry-catalog.test.ts # packages/cli/src/serve/server/telemetry.test.ts # packages/cli/src/serve/web-shell-static.test.ts # packages/cli/src/serve/web-shell-static.ts # packages/web-shell/client/App.tsx
Review follow-up and E2E verification — September 10Merged current main ( Missing snapshots blocking conversation forks — fixed. Reproduced the deferred Critical from review round 8: removing a saved page's file or directory made the entire fork fail with ENOENT. Forking now preserves the conversation and historical descriptor and emits a warning when snapshot storage is missing. Other ownership errors still abort before committing the child. No latest-page fallback was added. Restore record loss — independently reverified. The fix for R3-1 retains the saved-page record after a real ENOTDIR bookkeeping failure. Writing another actual JSONL snapshot and restoring a fresh store preserves its ID, URL, timestamps, tool-call ID and SHA-256; readable original HTML remains readable. Documentation and remaining comments. Both live-preview and saved-version designs now have complete, linked English and Chinese versions, including the missing-storage fork behavior. This resolves D8-15. The remaining Suggestions listed in rounds 7/8 remain acknowledged follow-ups under the repository's late-review scope rule: test-strengthening and fault-injection coverage, transient-I/O presentation, diagnostics, Windows-specific runtime validation, preview memoization/lazy mounting, CSP synchronization and abort handling. The review's explicitly unverified incidental hypotheses are not counted as confirmed defects. The previously closed Criticals were checked against the merged code again; resolved flags alone were not used as evidence. Actual daemon verificationThe test engineer ran the newly built
The installed global CLI does not contain this feature, so it established the unsupported baseline; deterministic recorder fixtures and the rebuilt daemon supplied the before/after proof. This does not claim model-generated publication or Windows execution. Both temporary daemons were stopped and their ports verified closed. Pre-commit checks
The PR description retains all ten images hosted on the wenshao fork assets branch. The reviewer test plan now includes missing-storage forks. 中文说明已合入 main 本轮实际修复了第 8 轮评审中延后的 Critical：快照文件缺失不再阻止整个会话分叉。分叉保留对话和原始历史描述符并发出警告；其他引用错误仍会阻止提交不完整分支。没有增加最新网页回退。 独立复验确认 R3-1 的恢复修复有效：真实 ENOTDIR 错误下，历史记录经过再次写入 JSONL 和新实例恢复后，ID、URL、时间、工具调用和校验和均保留。两份设计文档已补齐完整中英文版本及互链，D8-15 已解决。第 7/8 轮其余建议按仓库后期评审规则保留为后续工作，未验证的推测不计为已确认缺陷。 真实本地 daemon 验证了健康和三种缺失场景均可分叉（201），缺失内容明确返回 404 不可用；ENOTDIR 仍失败且不提交子对话。该验证使用实际持久化记录和 API，不模拟分叉、内容读取或持久化实现；不声称覆盖模型生成或 Windows。临时 daemon 已停止。 提交前完整构建、打包、类型检查、Lint、格式检查、5,397 项相关单元测试和 9 项 Chromium 场景均通过。多轮反向审计、使测试变红的受控变异验证、独立代码审查及真实 daemon 复验均完成后才提交。PR 描述保留 wenshao fork 图床的全部 10 张配图。 |
…QwenLM/qwen-code into codex/web-shell-preview-history
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Address-review round — PR #11276Feedback disposition
No guards, branches, or behaviors were added beyond combining the two merged sides, so no new witnesses were required; every resolved hunk is covered by the pre-existing suites listed below (the CSP directive by Files changed this roundMerge commit Verification
中文说明审查处理轮次 — PR #11276反馈处置
本轮除合并双方外未新增任何守卫、分支或行为,因此无需新增见证测试;每个解决的冲突块都由下列既有套件覆盖(CSP 指令由 本轮改动的文件仅合并提交 验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Follow-up at The CI freshness failure was resolved by incorporating the updated main gate. The next complete unit run then exposed eight reproducible failures in two upstream Web Shell test fixtures: startup now makes an independent branding request, and version tooltips are intentionally absent in the default compact sidebar. This follow-up supplies the existing branding response and allows only that request alongside capability discovery, then sets an expanded width in the two tooltip assertions. The initial session-request gate, single-load/stream checks, default-brand checks, and compact-footer regressions remain intact. No production behavior or workflow was changed by this follow-up. Validation: full repository build and typecheck passed; the complete Web Shell suite passed 302 files / 7,166 tests, without retries. Independent reproduction matched all eight CI failures before the fix; all 20 focused tests, including unchanged compact-footer regressions, passed afterward. Scoped lint/format passed. Two consecutive reverse-audit passes and two independent passes found no further issue before committing. The earlier webpage snapshot fixes and all ten description images are preserved. Remote CI for this exact commit is now successful: Qwen Code CI (full unit suite, static checks, integration tests, desktop checks and both browser gates), Serve A/B, Web-shell Visuals, SDK Java, and TUI parity. The CI log confirms the previously failing loading and branding files pass, and all 7,166 Web Shell tests pass. No unresolved review threads remain. The automatic PR review is still running and the previous changes-requested review has not yet been replaced by approval; the PR is conflict-free, awaiting review approval. 本次补充先同步 main 的新 CI 门禁，再修复完整单测发现的两处上游测试前提不匹配：启动时的独立品牌请求，以及紧凑侧栏隐藏版本提示。仅修改测试，保留会话提前请求、单次恢复和窄栏布局的全部原有断言。全仓构建、类型检查及 Web Shell 全套 302 个文件 / 7,166 项测试通过；提交前两轮反向审计和两轮独立审计均未发现新问题。当前提交的代码 CI 已全部通过，包括全量单测、静态检查、集成测试及两个浏览器检查。当前无合并冲突、无未解决评论线程；自动审查仍在运行，之前的“请求修改”状态仍待审核者更新。 |
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.
Not reviewed: three verifier-incidental candidates the audit loop never reached a verifier for — trailing-dot hostname rejection undocumented (packages/web-shell/client/components/preview/web-preview.ts:33), protectedOrigin matches spellings rather than name resolution (web-preview.ts:9), and the snapshot exemption keyed on a lookup that is undefined while the artifact list loads (packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:1060).
Not reviewed: the three new Playwright specs (web-shell.live-preview / pdf-preview / saved-preview.spec.ts) — no command in this run executes them; packages/web-shell/vitest.config.ts excludes client/e2e/**, so the green 302-file web-shell suite does not include them.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, and this diff adds an fs-flags path (packages/core/src/tools/artifact/artifact-snapshots.ts:94) whose Windows behaviour no run on this Linux host could exercise.
Not explored to full depth (tool budget reached): chunk 18: none — I did not run the Playwright specs (no browser/dev-server setup in this review tree); the static selector and assertion tracing above plus the head-commi…; "agent reverse-audit (round 1)": the default byte cap for GET /file when maxBytes is omitted — I confirmed only the explicit 256 * 1024 bound at packages/cli/src/serve/routes/workspace-fi…; "agent reverse-audit (round 1)": executing web-shell.live-preview.spec.ts under Playwright (browser install + vite dev server) to measure empirically how often the single-shot window.innerWi…; "agent reverse-audit (round 2)": the README claims 390px 手机宽度 and 地址必须使用主机名或 IPv4 were not verified against the web-shell preview code — my grep for 390 returned SVG asset noise and I sto…; "agent reverse-audit (round 2)": the App.test.tsx assertions that consume the new readSessionArtifactContent mock (my chunk ends mid-file at the latestMessageListProps type addition) were l…, and 1 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
docs/design/web-shell/web-shell-live-preview.md:40 — [review] The design record for what the diff itself calls a process-global shell-document policy change claims the other CSP directives are preserved, but the same diff adds a sdocs/design/web-shell/web-shell-live-preview.md:94 — [review] The "Files affected" list omits two files this PR changes that carry the very policies the design's own prose specifies — packages/web-shell/vite.config.ts ("Vite devdocs/design/web-shell/web-shell-preview-snapshots.md:15 — [review] The design states snapshot creation as an unconditional consequence of a successful publication and never states the shipped partial-failure semantics (publication succpackages/acp-bridge/src/sessionArtifacts.test.ts:6244 — [probe] The only test in this chunk that round-trips a saved-webpage record through recordEvent → rebuildSessionArtifactSnapshot → restore asserts that publishedUrl surpackages/acp-bridge/src/sessionArtifacts.test.ts:6563 — [probe] The new suite pins "never reclaim snapshot bytes for a removal that was not durably persisted" for the restore-prune path only ( if (persistenceWarnings.length === 0) ,packages/acp-bridge/src/sessionArtifacts.test.ts:6598 — [probe] The test named uses the runtime captured before the ambient environment changes never lets the ambient environment reach the store: the store() helper always passespackages/acp-bridge/src/sessionArtifacts.ts:1989 — [probe] The new reclamation path is silent twice over — deleteArtifactSnapshot already swallows every error internally (packages/core/src/tools/artifact/artifact-snapshots.tspackages/cli/src/serve/routes/session.ts:6418 — [probe] The bare catch collapses *every* readArtifactSnapshot failure — including transient I/O faults — into the permanent 404 artifact_snapshot_unavailable , and the onlpackages/cli/src/serve/run-qwen-serve.ts:6124 — [review] Nothing pins the three-way identity of the runtime directory that saved-webpage bytes travel through — the child's QWEN_RUNTIME_DIR (where ArtifactTool writes viapackages/core/src/services/session-artifact-persistence.ts:123 — [review] The source clause silently carries a second, unrelated rule — a record with source: 'tool' and no toolName is rejected only because artifact.toolName?.toLowerC…packages/core/src/services/sessionService.test.ts:5795 — [probe] No test pins that deletion-time collection unions the artifacts of *every* artifact record instead of replaying the transcript as a whole, so collapsing the per-recordpackages/core/src/services/sessionService.ts:3274 — [probe] The transcript scan that drives snapshot reclamation swallows every read error with no warning, unlike the sibling usage-salvage read at the same call site, so a sessiopackages/core/src/tools/artifact/artifact-snapshots.test.ts:57 — [review] No test in this file pins the mode: 0o600 that saveArtifactSnapshot sets on the snapshot bytes and on each reference file, so the only thing keeping saved page co…packages/core/src/tools/artifact/artifact-snapshots.test.ts:80 — [review] The "oversized" case cannot fail without the size guard, because line 75 already overwrote the file with 'changed' — the sha256 mismatch rejects it too, so stat.si…packages/core/src/tools/artifact/artifact-snapshots.test.ts:156 — [probe] Every retainArtifactSnapshot call in the entire repo's tests passes an operationId , so the !operationId form used by *both* non-fork production callers — the re…packages/core/src/tools/artifact/artifact-snapshots.test.ts:206 — [review] This test pins only the fork half of retainArtifactSnapshot 's documented two-sided contract ( if (code !== 'EEXIST' && !(code === 'ENOENT' && !operationId)) throw …packages/core/src/tools/artifact/artifact-snapshots.test.ts:290 — [review] The foreign-runtime url case in never deletes outside the exact file the descriptor points at never creates that foreign tree, and its only assertion is that the…packages/core/src/tools/artifact/artifact-snapshots.ts:97 — [review] On Windows constants.O_NOFOLLOW / O_NONBLOCK are undefined and the bitwise OR coerces them to 0, so the open silently collapses to a plain symlink-following O_RDONpackages/core/src/tools/artifact/artifact-tool.test.ts:50 — [probe] The new QWEN_RUNTIME_DIR stub and the new mock storage.getRuntimeBaseDir() are pointed at the *same* directory ( path.join(outDir, 'runtime') ), so the file's new spackages/core/src/tools/artifact/artifact-tool.ts:63 — [probe] The static DESCRIPTION promises snapshot saving and preservation-first tool choice unconditionally, while the behaviour it describes is gated on isArtifactSnapshotsE- …and 8 more (see the run report)
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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：three verifier-incidental candidates the audit loop never reached a verifier for — trailing-dot hostname rejection undocumented (packages/web-shell/client/components/preview/web-preview.ts:33), protectedOrigin matches spellings rather than name resolution (web-preview.ts:9), and the snapshot exemption keyed on a lookup that is undefined while the artifact list loads (packages/web-shell/client/components/artifacts/ArtifactPanel.tsx:1060).
未审查（原文为英文）：the three new Playwright specs (web-shell.live-preview / pdf-preview / saved-preview.spec.ts) — no command in this run executes them; packages/web-shell/vitest.config.ts excludes client/e2e/**, so the green 302-file web-shell suite does not include them.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, and this diff adds an fs-flags path (packages/core/src/tools/artifact/artifact-snapshots.ts:94) whose Windows behaviour no run on this Linux host could exercise.
未探索到全部深度（达到工具调用预算）：chunk 18：none — I did not run the Playwright specs (no browser/dev-server setup in this review tree); the static selector and assertion tracing above plus the head-commi…；"agent reverse-audit (round 1)"：the default byte cap for GET /file when maxBytes is omitted — I confirmed only the explicit 256 * 1024 bound at packages/cli/src/serve/routes/workspace-fi…；"agent reverse-audit (round 1)"：executing web-shell.live-preview.spec.ts under Playwright (browser install + vite dev server) to measure empirically how often the single-shot window.innerWi…；"agent reverse-audit (round 2)"：the README claims 390px 手机宽度 and 地址必须使用主机名或 IPv4 were not verified against the web-shell preview code — my grep for 390 returned SVG asset noise and I sto…；"agent reverse-audit (round 2)"：the App.test.tsx assertions that consume the new readSessionArtifactContent mock (my chunk ends mid-file at the latestMessageListProps type addition) were l…，另有 1 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 28 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.2)
|
Round 9 fixes and conflict resolution are in
Validation: full build, full typecheck, bundle and lint passed; core 326, ACP 1,096, CLI 944, and Web Shell 7,203 tests passed. All three Playwright specs ran explicitly: 9 Chromium scenarios passed, covering live modules/storage/HMR, navigation isolation, original saved-version cards and inline interaction after reopening/reloading, refresh from memory, and PDF blob framing. Browser daemon responses use fixtures; the separate fault-injection probes invoke the real tool, publisher, store, snapshot filesystem, and session fork/deletion implementation. They do not claim a model-driven scheduler E2E. Windows was not exercised on this macOS host. Every reproduced bug went red before its fix. Audit passes 1–3 found issues, pass 4 was clean, and pass 5 caught a Windows-specific test assumption. We disclosed the failure to converge in five passes, replaced the platform-dependent fault injection, and then completed consecutive clean passes 6 and 7 on the final diff. The test engineer independently verified rebuilt artifacts. All five code workflows completed successfully for exactly Convergence disposition: the review’s 28 deferred Suggestions remain recorded in the round 9 review rather than expanding this PR. The snapshot partial-failure wording and relevant lifecycle tests were updated as part of the Critical fixes; the remaining Suggestions are deferred to follow-up work and are not silently treated as implemented. This update does not merge the PR or substitute for maintainer review. 中文：本次处理了 main 冲突及 R9-1/R9-2，并在提交前的反向审计中复现、修复了部分恢复和后续淘汰导致的历史快照误删。两条原评论及新增边界均先有失败证据，再验证修复。第 1–3 遍发现问题，第 4 遍干净，第 5 遍发现 Windows 测试假设不成立；已说明前五遍未收敛，修正后第 6、7 遍连续无新发现；独立测试工程师验证了真实构建产物。build、typecheck、bundle、lint、9,569 个相关单元测试及 9 个 Chromium 场景通过。浏览器场景使用 daemon 数据夹具，另有真实文件、工具、存储和分叉/删除探针，不声称覆盖模型驱动的完整调度流程或 Windows。PR 的 10 张配图保持不变；其余非阻断建议按第 9 轮收敛规则留待后续处理。提交 874042c 的五个代码工作流已全部成功；主 CI 最后的文档浏览器检查 6 项、Web Shell 冒烟场景 73 项通过，独立视觉工作流的 PR 分支 47 项通过。自动审查仍在排队，之前的 CHANGES_REQUESTED 状态未被撤销，PR 未合并。 |
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
|
September 11 conflict and review follow-up — Both ACP conflicts preserve the trusted Web Shell snapshot capability and the upstream Goal-proposal capability before session initialization. All 58 files retain the same authored addition/deletion lines as the previous PR head; this maintenance update does not expand the feature. The original PR description and its 10 screenshots remain unchanged. No new unresolved review threads were present. The latest automatic review result reports a 21,600-second timeout, not a new code finding. The 15 historical Critical threads and the missing-snapshot fork case were checked against the merged implementation, rather than assuming resolved threads prove a fix. Previously recorded Suggestions remain deferred under the repository's late-review convergence rule. No review was dismissed and the PR has not been merged. Before committing, two consecutive independent reverse-audit passes found no new verified Critical; parent review and an additional inspection of the actual validation logs and assertions agreed. Full build, typecheck (including integration), bundle, lint and formatting passed. Package-scoped unit tests passed: core 993, ACP 1,098, CLI 1,088 and Web Shell 1,048 — 4,227 total. The three explicitly selected preview Playwright specs passed all 9 Chromium cases with no retries. The test engineer reran 12 real filesystem/publication/store/fork outcomes using the newly installed dependencies and rebuilt modules. All passed, including failed persistence, partial restore and both cancellation windows. A supplemental mocked ACP lifecycle matrix passed 6 combinations of trusted-parent and Goal capability; actual Config objects passed 8 combinations of recording, snapshot opt-in and Goal host support. Actual Artifact publication of v1 and v2 to the same latest URL created distinct saved versions; after deleting both the source and latest file, both original saved HTML versions remained exactly readable. Fourteen source/output/test/lockfile hashes stayed stable through verification. Browser daemon responses use fixtures; the independent filesystem probes use real backend implementations. Neither is presented as model-driven scheduler E2E or native Windows validation. All probe processes exited normally. All five code workflows completed successfully for exactly 中文说明本次合入 main 没有新的未解决评论。最新自动审查是达到 21,600 秒上限后超时，并未报告新的代码问题。已按当前合并代码重新核对 15 条历史 Critical 及快照缺失时的分叉问题；原有非阻断建议继续按后期收敛规则留待后续，未撤销审查或合并 PR。 提交前两轮独立反向审计连续无新发现，并复核实际日志和断言。完整构建、类型检查、打包、Lint、格式检查、4,227 项相关单元测试和 9 个 Chromium 场景通过。独立测试重新执行的 12 个真实文件场景、6 种 ACP 模拟生命周期组合、8 种真实 Config 组合均通过；真实连续发布两版后，删除源文件及最新发布文件，两份历史 HTML 仍精确保留。14 项源码、产物、测试和锁文件哈希全程稳定。 浏览器场景使用 daemon 数据夹具，后端探针调用真实实现；不声称覆盖模型驱动的完整调度流程或 Windows。提交 2ff9bb8 的五个代码工作流已全部成功，最后的文档浏览器检查 6 项、Web Shell 冒烟场景 74 项通过；视觉检查的 PR 分支 47 项、main 对照 43 项通过。自动审查仍在排队，旧的 CHANGES_REQUESTED 状态未被撤销，PR 未合并。 |
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.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; this diff adds an fs-flags path whose Windows behaviour no run on this Linux host could exercise.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: the three new Playwright specs (web-shell.live-preview / pdf-preview / saved-preview) — packages/web-shell/vitest.config.ts excludes client/e2e/**, so no command in this run executed them.
Not reviewed: test-efficacy probe — harnessValidated was null (the positive control never produced a verdict; all 25 revert probes came back inconclusive because the scratch tree has no built dist), so no revert/mutant/hunk evidence exists for this PR either way.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": the README's live-preview behaviour claims in my chunk (tab-switch state preservation, per-workspace/session URL and width persistence, 390px toggle, cross-orig…; chunk 17: none — I did not run the Playwright suite (no browser/dev-server run in this review), so all test-pass reasoning above is from static reading of the specs and t….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/artifact/artifact-snapshots.ts:250 — [review] A lost ownership race surfaces as bare ENOENT, indistinguishable from absent storage, so the fork caller warns and commits a branch whose saved version is permanently una…packages/core/src/tools/artifact/artifact-tool.ts:66 — [review] The tool DESCRIPTION promises saved-version history unconditionally while the behaviour is gated on isArtifactSnapshotsEnabled()packages/acp-bridge/src/bridge.ts:7157 (+1 locations) — [review] No test joins the artifactSnapshotRuntimeBaseDir wiring to store behaviour; a dropped forwarding degrades to a silent no-oppackages/web-shell/vite.config.ts:101 — [review] The dev-server CSP is a hand-copy of the shipped policy with no drift guard, and the three new acceptance specs assert the copypackages/web-shell/client/components/artifacts/TurnOutputs.tsx:464 — [review] The transcript card's new saved-version label has no test; the fallback leaks the internal token web_preview_snapshot to the userpackages/acp-bridge/src/sessionArtifacts.test.ts:6925 — [review] The runtime-capture test never reaches the ?? Storage.getRuntimeBaseDir() fallback it is named for, so a lazy-getter mutation survives all 172 testspackages/core/src/config/config.test.ts:6081 — [review] The only assertion on the new shouldAutoOpenArtifact snapshot guard cannot fail under CI=true with no DISPLAY, so the guard is untestedpackages/core/src/services/sessionService.ts:3247 — [review] collectSessionArtifactSnapshots re-implements jsonl.read, doubling the transcript read per deletion and swallowing failures with no diagnosticpackages/core/src/services/sessionService.ts:3358 — [review] Archive/unarchive conflict resolution deletes a same-ID transcript copy without reclaiming its snapshots, orphaning them through two successful operationspackages/core/src/services/sessionService.ts:4208 — [review] Fork-rollback reclamation failures are silent at both layers, unlike every sibling cleanup in the same finally blockpackages/web-shell/client/components/artifacts/ArtifactPanel.test.tsx:73 — [review] The new readSessionArtifactContent mock is the only one afterEach never resets, leaking implementations into later testspackages/core/src/tools/artifact/artifact-tool.test.ts:180 — [review] The cancel-test stub permits shouldAutoOpenArtifact and snapshots enabled at once, a configuration the shipped Config forbidspackages/web-shell/client/components/preview/SavedWebPreview.tsx:55 — [review] Every snapshot read failure collapses into one terminal 404, so a transient EMFILE permanently hides the only retry affordancepackages/web-shell/client/components/preview/WebPreviewPanel.tsx:127 — [review] The live preview srcDoc embeds the translated frame title, so a UI-language change rewrites it and force-reloads the framed apppackages/core/src/config/config.ts:7914 — [review] Managed-session publication writes into the process-global ~/.qwen/artifacts while snapshots write into the captured per-session runtimepackages/core/src/config/config.ts:7906 — [review] The live published file:// URL the tool tells the model to share has no renderer in the Web Shell the gate's premise namespackages/core/src/services/sessionService.ts:4175 — [review] No test separates the captured runtime dir from the ambient one at the fork retain and rollback sites; both mutants pass 269 testspackages/core/src/tools/artifact/artifact-snapshots.ts:94 — [review] OR-ing O_NOFOLLOW in by hand collapses to a symlink-following O_RDONLY on Windows, the shape no-follow-open.ts forbidspackages/web-shell/client/App.test.tsx:3729 — [review] The only web_preview_snapshot routing fixture uses a file:// URL, so deleting the new routing clause keeps all 871 tests greenpackages/web-shell/client/components/artifacts/turnOutputSelectors.test.ts:69 — [review] Per-turn savedUrls lets a record_artifact re-attribution resurrect the duplicate card, and no test covers the cross-turn case- …and 1 more (see the run report)
Convergence: round 10 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/acp-bridge/src/sessionArtifacts.ts (findings in round 9; 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.)
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.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; this diff adds an fs-flags path whose Windows behaviour no run on this Linux host could exercise.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：the three new Playwright specs (web-shell.live-preview / pdf-preview / saved-preview) — packages/web-shell/vitest.config.ts excludes client/e2e/**, so no command in this run executed them.
未审查（原文为英文）：test-efficacy probe — harnessValidated was null (the positive control never produced a verdict; all 25 revert probes came back inconclusive because the scratch tree has no built dist), so no revert/mutant/hunk evidence exists for this PR either way.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：the README's live-preview behaviour claims in my chunk (tab-switch state preservation, per-workspace/session URL and width persistence, 390px toggle, cross-orig…；chunk 17：none — I did not run the Playwright suite (no browser/dev-server run in this review), so all test-pass reasoning above is from static reading of the specs and t…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 21 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 10 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/sessionArtifacts.ts（第 9 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
R10 verification report for e180a24. The latest Critical is fixed: a non-strict snapshot-reference I/O failure no longer leaves silently accepted history after the bridge drops its notifications. The four-file change preserves strict rollback, adds focused regressions and synchronizes the English/Chinese design explanation.
The real-I/O test-script fallback executes Artifact publication, store mutations, BridgeClient extraction/ingestion, JSONL persistence/rebuild and SessionService fork handling. Its configuration and event receiver are fixtures. The reference-read failure is native EACCES; the separate unit acquisition-write failures inject ENOSPC before/after a real write. This independent local probe does not claim a full model/daemon/browser run or native Windows filesystem verification. The remote CI checks below are separate executed checks. The unchanged feature-specific nine Chromium scenarios remain documented in the preceding verification report. The latest review's 21 Suggestions remain explicitly deferred under the round-10 convergence posture; this change addresses the Critical without widening the feature. The original bilingual PR description and all ten images hosted on the wenshao fork assets branch are preserved. No merge or review dismissal was performed. All five code CI workflows completed successfully on e180a24:
The main CI unit log contains 23 suite summaries totaling 74,124 passed and 116 skipped tests, including all 178 store regressions. The final browser job passed the six-case transcript document gate and all 74 smoke cases. Main CI's macOS/Windows unit jobs and model-dependent CLI integration job were skipped by the workflow; they are not presented as executed coverage. The PR is mergeable with zero unresolved review threads. The separate automatic code review is still in progress; its previous changes-requested decision has not been dismissed. 中文说明本轮修复了非严格写入时引用 I/O 错误导致的通知与历史记录不一致。修复前真实权限错误得到“通知 0、内存 2、后续落盘 2”；修复后变为 2/2/2，ID 一致且准确报告警告。三种严格模式仍全部回滚，正常及幂等路径不新增警告。 独立验证的 6 个主场景和 12 个历史保护场景全部通过。两个新增回归先在旧代码上失败；修复后 178 个存储测试及合计 1,236 个相关 ACP 测试通过。完整构建、打包、类型检查、lint、格式及差异检查均通过。提交前主审计与独立审计各连续两轮未发现新的 Critical，独立验证期间 23 项源码及产物哈希保持一致。 本轮独立探针使用真实文件系统、产物发布和 JSONL 持久化，事件接收端为测试 fixture；该本地探针没有宣称执行完整模型/daemon/浏览器链路或原生 Windows 文件系统验证。远端 CI 的执行结果单独列于上表。按本轮审查的收敛要求，21 条 Suggestions 保留为后续工作。PR 原有双语描述和 wenshao fork assets 分支托管的 10 张图均保留，未合并 PR 或撤销评审。 新提交的五组代码 CI 已全部成功完成。主 CI 单元测试日志的 23 组摘要合计 74,124 项通过、116 项跳过，其中本轮存储测试 178 项全部执行并通过。最后的会话文档检查 6 项、浏览器冒烟 74 项均通过；视觉检查为 PR 版本 47 项、主分支对照 43 项通过。主 CI 的 macOS/Windows 单测及依赖模型密钥的 CLI 集成任务按工作流跳过，未将其记为已执行覆盖。PR 无冲突，未解决评论线程为 0；自动代码复审仍在进行中，上一轮请求修改的评审状态未被撤销。 |
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.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only no-follow-open behaviour was checked against a constants model, not a run.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 18: prettier --check on the three territory files (the run timed out before producing output, so CJK line-break formatting is unverified — formatting is formatter…; "agent reverse-audit (round 1)": did not read isTrustedManagedParent() 's body ( packages/cli/src/acp-integration/acpAgent.ts :3638), so I confirmed only that the new acpAgent.test.ts case's…; "agent reverse-audit (round 1)": did not open normalizeInput 's metadataBudget: 'persisted' filtering to confirm qwen.snapshot.references is preserved for restored records; I inferred it f…; "agent reverse-audit (round 2)": end-to-end confirmation that the ACP emitter labels the post-settle cancel cancelled (not failed ) in createTranscriptToolCallResultUpdate , and an executed…; "agent reverse-audit (round 1)": none — all six layers above were walked to a conclusion; nothing in chunk 12 was left unread., and 6 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
12 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/core/src/services/sessionService.ts:3267 — [probe] Critical [fails-closed] [new-surface] snapshot bytes unreclaimable whenever the descriptor never reaches a scanned location; no orphan sweeppackages/acp-bridge/src/sessionArtifacts.test.ts:6526 — [review] R11-1: The only parent/fork shared-snapshot test never asserts that the fork acquired its own reference (nor that its remove /eviction actually dropped the record), so every …packages/acp-bridge/src/sessionArtifacts.ts:1176 — [review] R11-2: pendingSnapshotRemovals is a suppress-only deferral set; deferred reclamation is never performed. Store is at maxArtifacts and an upsert batch evicts a web-preview artifac…packages/cli/src/serve/routes/session.ts:6420 — [review] R11-3: content route collapses every FS failure into the terminal artifact_snapshot_unavailable code; client renders it as permanent with no retry. The daemon is at its descriptor lim…packages/core/src/config/config.test.ts:6083 — [review] R11-4: The only assertion covering the new auto-open suppression ( config.ts:8012 if (this.isArtifactSnapshotsEnabled()) return false; ) is satisfied by ambient CI environment, so de…packages/core/src/config/config.ts:7914 — [review] R11-5: Relaxing the non-interactive gate for managed preview sessions neutralizes only the "opens a browser" half of the gate's own stated rationale ("Publishing writes outside the project …packages/core/src/services/sessionService.ts:3314 — [review] R11-6: Every session deletion now JSON-parses each transcript identity a second time, unconditionally, in the same loop that already fully reads the same file for usage salvage. …packages/core/src/services/sessionService.ts:3360 — [review] R11-7: Snapshot reclamation is the first operation that can throw *after* the transcript has been removed and durable-synced, so its ownership-assert failure is reclassified by th…packages/core/src/tools/artifact/artifact-tool.ts:274 — [review] R11-8: The cancel path writes a fully durable snapshot and only then discovers the request was cancelled — signal.aborted is checked after saveArtifactSnapshot returns, so…packages/web-shell/client/components/artifacts/turnOutputSelectors.test.ts:70 — [review] R11-9: The duplicate-card suppression this PR adds builds its savedUrls set **per turn** ( turnOutputSelectors.ts:74-95 ), so a local file:// publi…packages/web-shell/client/components/preview/web-preview.ts:44 — [review] R11-10: The protected-origin comparison is one-directional: it blocks an http: candidate that *upgrades* into a protected https: origin, but never an https: can…packages/web-shell/client/e2e/web-shell.live-preview.spec.ts:390 — [review] R11-11: The new "recorded links open metadata without automatically loading a live frame" test pins frame *absence* with toHaveCount(0) , which resolves on its fir…packages/web-shell/client/e2e/web-shell.saved-preview.spec.ts:79 — [review] R11-12: The browser-level containment test builds the wrapper document in the Node test process, where DOMParser is undefined, so Chromium is handed the output of…packages/acp-bridge/src/bridge.ts:7157 — [review] runtime base dir plumbed write-side vs resolved read-side independently, silent ambient fallbackpackages/acp-bridge/src/sessionArtifacts.test.ts:6655 — [review] it.each(['before','after']) parametrization changes only the spy's internal behaviour - no assertion depends on the parameter, so the corner it exists to cover is unpinnedpackages/core/src/tools/artifact/artifact-snapshots.test.ts:274 — [review] The reclamation path's symlink-containment guard — the four realpath(...) !== path.join(realRoot, ...) comparisons in snapshotDirectory (packages/core/src/tools/…packages/core/src/tools/artifact/artifact-snapshots.ts:93 — [review] Hand-rolled O_NOFOLLOW|O_NONBLOCK expression collapses to a plain symlink-following open on Windows, the exact pattern the repo's no-follow-open helper exists to forbidpackages/web-shell/client/App.tsx:6104 — [review] no sweep removes an already-mounted web_preview tab when webPreviewAvailable flips falsepackages/web-shell/client/App.tsx:6581 — [review] The deterministic web-preview tab id and the replace-vs-append dedupe that depends on it have no testpackages/web-shell/client/components/artifacts/ArtifactPanel.tsx:3643 — [review] The new artifactPreviewDocument wrapper builder is called inline in JSX, so every panel re-render re-parses and re-escapes the entire HTML artifact document - …- …and 1 more (see the run report)
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally; the Windows-only no-follow-open behaviour was checked against a constants model, not a run.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）：chunk 18：prettier --check on the three territory files (the run timed out before producing output, so CJK line-break formatting is unverified — formatting is formatter…；"agent reverse-audit (round 1)"：did not read isTrustedManagedParent() 's body ( packages/cli/src/acp-integration/acpAgent.ts :3638), so I confirmed only that the new acpAgent.test.ts case's…；"agent reverse-audit (round 1)"：did not open normalizeInput 's metadataBudget: 'persisted' filtering to confirm qwen.snapshot.references is preserved for restored records; I inferred it f…；"agent reverse-audit (round 2)"：end-to-end confirmation that the ACP emitter labels the post-settle cancel cancelled (not failed ) in createTranscriptToolCallResultUpdate , and an executed…；"agent reverse-audit (round 1)"：none — all six layers above were walked to a conclusion; nothing in chunk 12 was left unread.，另有 6 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
12 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 21 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
@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: 143 passed · 0 failed · 143 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：143 通过 · 0 失败 · 143 总计 抖动门： Verification report (report.md, truncated)Flakiness gate logEvidence imagesHarness 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 pass at head e180a2418dd55ec4bd942ed95b55d6aec845c6e2. No merge-blocking defect found; previously reported blocking issues verified fixed on the current code.
Historical blocking issues re-verified on this head
- R9-1 (non-strict ingestion losing a batch without rollback) — fixed. In
packages/acp-bridge/src/sessionArtifacts.ts, theretainArtifactSnapshotloop insideupsertManynow catches per-artifact: it rethrows only whenvalidationStrict || persistenceStrict, otherwise it pushesartifact <id> kept without retaining its snapshot: ...and processing continues, so a reference-bookkeeping I/O error can no longer leave the store mutated while the caller drops the batch. Strict paths still reach the outercatch, which now callsrollback()(restoreState(before)+reclaimSnapshotFiles(rollbackArtifacts)) instead of a bare state restore. - R9-2 (snapshot not cleaned up when a cancelled tool result is discarded) — fixed.
packages/core/src/tools/artifact/artifact-tool.tscheckssignal.abortedimmediately aftersaveArtifactSnapshotand, when set, callsdeleteArtifactSnapshot(snapshot, runtimeBaseDir, sessionId)and returns no artifact descriptors, so a cancel landing after publication no longer orphans a reference-holding snapshot. - R4-1 / R3-1 (fork creating a second live owner; reclamation destroying a version the durable log still references) — fixed.
packages/core/src/services/sessionService.tsnow resolvesrebuildSessionArtifactSnapshot(forked, newSessionId)and callsretainArtifactSnapshot(..., operationId)beforefs.promises.link(stagedTranscriptPath, targetPath); a non-ENOENT retain error throws so no partial fork commits, ENOENT warns and keeps the record, and thefinallyblock releases exactly the operation-scoped reference when!committed.removeSessionTranscriptscollects snapshot ids only from the deleted session's own transcripts and releases them throughdeleteArtifactSnapshot, whosermdironreferences/is the last-owner claim (ENOTEMPTY returns early, keepingindex.html). - R1-3 / R3-2 / R2-1 / R3-15 (cross-session read pairing, artifact-URL preview gate, raw host opt-in gate, non-workspace artifact tab) — corresponding threads are resolved and the gates are consistent in the current
SavedWebPreview.tsx/App.tsxcode; the read path is additionally constrained server-side (below).
Current scan
- New read route
GET /session/:id/artifacts/:artifactId/content(packages/cli/src/serve/routes/session.ts) is wrapped inwithOwnerReadSession(..., { cwdBound: true }), requires a parsed client id, resolves the artifact through the owning session's owngetSessionArtifacts, and reads withruntime.sessionRuntimeBaseDir. Failures collapse to a single404 artifact_snapshot_unavailable, and the response setsContent-Disposition: attachment,X-Content-Type-Options: nosniff,Cache-Control: private, no-store. No cross-session or cross-runtime fallback is reachable. - Snapshot read integrity (
artifact-snapshots.ts):readArtifactSnapshotrejects a descriptor whoseurldoes not equal the expectedfile://<runtimeBaseDir>/artifacts/snapshots/<id>/index.html, comparesrealpathagainst the real root, opensO_RDONLY|O_NOFOLLOW|O_NONBLOCK, requiresstat.isFile(), bounds the size byMAX_ARTIFACT_BYTES, and re-verifies the stored sha256 before returning bytes. Tampered or swapped content yields "unavailable" rather than wrong content. - Descriptor classification:
getWebPreviewSnapshotIdrequires a v4-UUIDpreview-managedId,kind === 'html',storage === 'published',artifactType === 'web_preview_snapshot', a well-formed sha256, tool/source consistency, and a matchingfile:URL suffix; every consumer (snapshotDirectory,readArtifactSnapshot) re-anchors the id to the actual runtime root before touching the filesystem, so an unanchored suffix cannot select another runtime's bytes. - Frame-policy widening (
web-shell-static.tsnow emitsframe-src http: https: blob:and dropsloopbackSandboxOrigins): this does not weaken the existing offline HTML viewer or the MCP App sandbox.artifactPreviewDocumentwraps model-authored HTML in a trusted parent whose own meta CSP isARTIFACT_PREVIEW_CSPplusframe-src 'none', and the content frame stayssandbox="allow-scripts"(opaque origin) withreferrerpolicy="no-referrer", so intersected policies still deny nested frames and network access. The MCP App sandbox keeps its own independently served policy (packages/cli/src/serve/mcp-app-sandbox.ts, unchanged here,frame-src 'none'unless the app declares frames). The new live-preview wrapper pinsframe-src ${url.origin}andparseWebPreviewUrlrejects non-http(s), credentialed, trailing-dot, non-LDH hostnames and any origin that normalizes to the shell or daemon origin (including the HTTPS-upgraded spelling).
CI
All non-skipped checks on this head are green (Lint & Static, Test (ubuntu-latest, Node 22.x), Serve A/B, web-shell E2E Smoke, Desktop Shell ubuntu/windows, Integration Tests no-AK, review-pr). The macOS/Windows Node test lanes and Integration Tests (CLI, No Sandbox) are skipped, which is not attributable to this change.
Non-blocking follow-up (already disclosed by the author)
There is still no orphan sweep: snapshot bytes whose descriptor never reaches a scanned transcript location stay on disk. This fails closed, is new surface, and is called out in the PR's Risk & Scope, so it is recorded as follow-up work rather than a merge blocker.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at e180a2418dd55ec4bd942ed95b55d6aec845c6e2 — Partial review — coverage gaps. Comment only; no approval implied.
No additional independently confirmed Critical is reported in the inspected paths. This is not a CLEAN/C=0 verdict or comprehensive-review completion. Base pinned to c46cb85cf21b3b8e8393222fffc670176e0de1da; reviewed current scope, not merely the last-review delta.
Historical reassessment (static):
- R9-1, latest non-strict retain-failure anchor:
packages/acp-bridge/src/sessionArtifacts.tsnow catches each bookkeeping failure, returns an artifact-specific warning and continues the batch; strict modes still enter rollback. Read the before/after-reference-write regressions atsessionArtifacts.test.ts:6655-6721. The earlier rollback-reference anchor now restores the previous state before reclaiming discarded candidates, preserving current and pending-durable ownership. The restore-retain R3-1 anchor similarly keeps the descriptor on bookkeeping failure. - R3-1 reclamation / R4-1 shared-fork ownership: traced explicit captured runtime roots, per-session/operation references, final-reference directory claim, removal/eviction/rewind cleanup, restore-prune persistence gating, physical-transcript deletion collection and fork retain-before-commit/operation-only rollback. Ordinary CLI and recording-disabled sessions do not enable snapshots. These address the cited mechanisms; they do not establish crash recovery or every concurrent lifecycle interleaving. Current fork code intentionally retains an unavailable descriptor on missing storage; an older response claiming all missing bytes prevent fork commit is no longer accurate.
- R1-2 publication-success status, R1-3 foreign-session client ID, both R2-1 anchors (workspace gating and PDF blob CSP), R3-2 hosted live-card dedup and R3-15 workspace-independent saved viewer: current branches address the reported mechanisms. The HTML validator expansion from R1-1 is absent from the production diff. R9-2 now releases the newly saved snapshot when cancellation is observed after save; I have not certified the entire scheduler handoff window.
Boundary tracing: the new content route uses the session-owner read wrapper, registered artifact lookup and the owner's sessionRuntimeBaseDir. The exact URL/realpath/regular-file/size/hash reader does not fall back to the latest publication or another runtime. Read the secondary-root and untrusted-owner route tests. The shared resolver retains its existing single-entry legacy-primary shortcut; I did not exhaustively validate all removed/bootstrap/draining and standalone activation cases. Saved content uses nested opaque script-enabled sandboxing with restrictive parent/content CSP; live preview validates URLs and uses an origin-limited wrapper. These static checks are not a browser-enforcement guarantee.
Coverage limitations: inspected changed snapshot/store/service/tool functions, runtime wiring, SDK read, preview components, artifact panel/App routing, selected unit/route regressions and the saved-preview E2E assertions, plus historical Critical discussions. Not every changed test/documentation section or App lifecycle path was examined. Real browser CSP/navigation, full daemon restart/fork/deletion interleavings, scheduler cancellation after tool return, and all trust/removal lifecycle states remain unverified. No PR code, tests, builds or probes were executed; prior authors' test claims were not reproduced here. Remaining Suggestions stay deferred. Maintainer permission verified; no external-refactor hard gate.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
APPROVE — no live Critical at head e180a2418d. Filing this as the second approval; @qqqys approved at 09:39 and @doudouOUC's pass at 09:54 found no Criticals.
Basis
The round-11 ledger records findings: [], posted: 0 against this exact sha, with flatRounds: 2. All 50 review threads are resolved, and every substantive lane is green (Lint & Static, Test (ubuntu-latest, Node 22.x), Integration Tests (no-AK, No Sandbox)). The 21 deferred items are deferred under the convergence posture and explicitly marked "not a blocker"; the one Critical among them is fails-closed on new surface, so no wrong result is certified and the merge base had neither the surface nor the defect. Author is a maintainer, so the AGENTS.md two-tier core gate does not engage.
What I verified independently
I did not re-review 5,300 lines that have been through eleven rounds. I read the two surfaces where a defect would be silent and security-relevant, and both hold up.
The artifact read path is TOCTOU-safe in the way that matters. artifact-snapshots.ts:86-101 proves the URL matches the expected file, then does a realpath containment check against the realpath'd root, then opens, then handle.stat() on the opened descriptor rather than re-statting the path — so the regular-file and size proofs apply to the bytes actually read, not to a path that could have been swapped in between. The read loop is bounded by stat.size + 1 with a bytesRead === 0 break. That is the correct ordering; the fstat-on-handle is what makes the preceding realpath check non-load-bearing on its own.
The live-preview layering is sound. parseWebPreviewUrl (web-preview.ts:23-56) rejects non-http(s), credentials in the URL, trailing-dot hostnames, and hostnames outside [a-z0-9._-], and refuses both the shell origin and the daemon origin including the https-upgraded form — so the preview cannot be pointed back at the shell itself. The wrapper document (:66-77) then carries default-src 'none'; script-src 'none'; frame-src <the one validated origin>; base-uri 'none'; form-action 'none', with the inner iframe sandboxed and no-referrer. Pinning the child source per instance rather than at the shell is the right place for it.
Notes, none blocking
1. The O_NOFOLLOW comment asserts a guarantee Windows does not provide. artifact-snapshots.ts:94-96 says the flag "refuses a symlink swapped in after the realpath check", but libuv ignores O_NOFOLLOW on win32 — this repo already documents that at acp-integration/live/capture-screen-context.ts:56 and its test at :76, and three sites handle it explicitly with process.platform === 'win32' ? 0 : (fsConstants.O_NOFOLLOW ?? 0) (conversation-workspace.ts:532, standalone-deletion-journal.ts:508 and :563). The defence does not actually collapse, because the realpath containment check and the handle fstat().isFile() both still hold on every platform — but the comment reads as a stronger claim than the code makes on Windows. Worth either a clause acknowledging it or following the win32-guard convention. This overlaps the deferred artifact-snapshots.ts:93 item and @doudouOUC's S2, which looked at O_NONBLOCK as a perf no-op rather than at the symlink half.
2. The shell-level frame-src widening is the real tradeoff here and deserves to be stated plainly. web-shell-static.ts:67 goes to frame-src http: https: blob:, replacing loopbackSandboxOrigins, whose own docblock warned that "wildcard ports would let a compromised shell embed any loopback listener". The per-instance wrapper pinning above is what makes this acceptable for the preview feature, and framing an arbitrary http(s) URL is inherent to what a live web preview is — but the shell's own CSP no longer constrains what a compromised shell can frame, which is strictly wider than the property the deleted helper existed to enforce. Not something to change for this PR; worth a line in the design doc's threat model so the next reader does not assume the old loopback pinning is still in force.
3. The deferred Critical should get a tracking issue. sessionService.ts:3267 — snapshot bytes unreclaimable when the descriptor never reaches a scanned location, with no orphan sweep — is a disk leak rather than a correctness or disclosure fault, so deferring it past this merge is reasonable. It is the one deferred item with a resource cost that grows without bound, and the deferral list is a review artifact rather than a work queue, so it is worth filing before this closes.
Noting for the record that I reviewed against gh pr diff rather than a local merge-base: origin/main on this machine is stale enough that a local three-dot diff pulled in already-merged PRs.
6 participants
What this PR does
Adds a Web preview panel for browser-reachable development URLs, with desktop/mobile widths, refresh, and external opening. Standalone Web Shell enables it for workspace sessions by default; embedded hosts can opt in. Live preview retains the existing workspace requirement; saved HTML versions can also open in standalone and Live sessions without a workspace. The selected URL and width are remembered per session and workspace. Published Artifact links can open in live preview; ordinary recorded links keep their metadata view and external action.
In managed ACP sessions with chat recording enabled, each new self-contained HTML Artifact delivery also saves an independent immutable version. Ordinary CLI publication and recording-disabled sessions do not create history files. Each saved-version card stays with the original message, so closing the panel, refreshing Web Shell, or restarting the daemon still opens the HTML delivered at that time. Later deliveries retain their own cards and timestamps while the existing latest publication URL keeps its current behavior.
Saved content is read through an authenticated, session-owner-scoped route that verifies the stored file and hash. Once a saved version has loaded, Refresh rebuilds it from in-memory HTML without another daemon request. Existing PDF attachments remain frameable in development and production. A failed history save keeps the successful publication available with a warning; transient preview reads offer retry instead of reporting data loss. Saved versions and existing HTML previews share an isolated viewer that allows inline interaction while blocking network access and self-navigation.
Why it's needed
A live development page is useful while iterating, but its URL changes content as the project changes. Keeping only a right-panel tab or the latest publication URL loses what was delivered in an earlier turn. This adds live iteration and a durable record of each HTML delivery in the conversation.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Historical deliveries stay with their original messages. Both screenshots below were captured for this feature after restoring the same recorded session; each card opens its own saved HTML and supports inline interaction.
Saved version 1 remains available after version 2 has been delivered.
Opening the second card shows version 2 without replacing version 1.
Live development preview. A real Vite page runs in the right panel alongside the conversation. Desktop and 390 px mobile views share the same interactive page.
Before: the installed qwen 0.23.0 right panel had no Web preview action
The current repair passes build, bundle, full typecheck, lint, 4,936 selected unit tests, 61 Chromium smoke scenarios and 6 focused preview browser scenarios. Real-daemon checks confirm deletion cleanup and retention for a never-loaded fork after parent deletion. Real-module checks cover persistence rejection, rewind, owner-runtime isolation, same-ID workspaces and managed tool registration. Two controlled mutation groups fail as expected when fork ownership or persistence protection is removed. Two consecutive full reverse audits and an independent review found no remaining Critical. The separate E2E report states the limits of each test method.
Tested on
Environment (optional)
macOS, Node.js 22.22.2, Chromium, and isolated local daemon runtimes. Real-daemon checks used deterministic recorded conversations and actual Artifact publication/storage without mocked HTTP responses or model requests. Temporary test servers were stopped after verification.
Risk & Scope
Linked Issues
None.
中文说明
本 PR 的改动
新增 Web 预览面板，支持浏览器可访问的开发页面 URL，提供桌面/手机宽度切换、刷新和外部打开。独立 Web Shell 的工作区会话默认启用，嵌入式宿主可以选择启用。实时预览沿用现有工作区要求；已保存的 HTML 版本也能在无工作区的 standalone 和 Live 会话中打开。所选 URL 和宽度按会话及工作区保存。Artifact 发布的链接可进入实时预览，普通记录链接保留详情查看和外部打开操作。
在开启会话记录的受管 ACP 会话中，每次通过 Artifact 新交付的自包含 HTML 还会保存一个独立、不可变的版本。普通 CLI 发布和关闭记录的会话不创建历史文件。卡片保留在原始消息中，因此关闭面板、刷新 Web Shell 或重启 daemon 后，仍能打开当时交付的 HTML。后续交付各自保留卡片和时间，现有最新发布 URL 的行为保持不变。
已保存内容通过经过身份验证、按会话所属运行时限定的接口读取，并校验存储文件和哈希。已加载历史版本的刷新直接使用内存中的 HTML，不再请求 daemon。开发与生产环境继续支持 PDF 附件的 iframe 展示。历史版本写入失败时，已成功发布的内容仍可用，并显示提示；临时读取失败会提供重试，不会误报数据丢失。已保存版本与现有 HTML 预览共用隔离查看器，允许内联交互，同时阻止网络访问和页面自身导航。
为什么需要
实时开发页面适合迭代，但随着项目变化，同一 URL 的内容也会改变。如果只保留右侧面板标签或最新发布 URL，就会丢失先前轮次交付的内容。本改动同时提供实时迭代能力，并在会话中持久记录每次 HTML 交付。
审阅者测试计划
如何验证
验证证据（改动前后）
历史交付保留在各自的原始消息中。 下方两张截图来自本功能构建恢复后的同一个会话；每张卡片打开各自保存的 HTML，并支持内联交互。
交付版本 2 后，已保存的版本 1 仍然可以打开。
打开第二张卡片显示版本 2，不会覆盖版本 1。
实时开发预览。 真实 Vite 页面在会话旁边的右侧面板中运行，桌面与 390 px 手机宽度共用同一个可交互页面。
改动前：已安装的 qwen 0.23.0 右侧面板没有 Web 预览入口
当前修复已通过构建、打包、全量类型检查、lint、4,936 个相关单元测试、61 个 Chromium smoke 场景和 6 个预览浏览器场景。真实 daemon 验证了删除清理，以及父会话删除后尚未加载的分支仍保留内容。真实模块验证覆盖持久化失败、回退、所属运行目录隔离、同 ID 跨工作区和受管工具注册。两组受控变异在移除分支引用或持久化保护后按预期失败。连续两轮完整反向审计及独立审查未发现遗留 Critical；独立 E2E 报告明确各验证方式的范围。
已测试平台
环境（可选）
macOS、Node.js 22.22.2、Chromium 及隔离的本地 daemon 运行时。实际 daemon 检查使用确定性的会话记录和真实 Artifact 发布/存储，没有模拟 HTTP 响应，也没有发送模型请求。验证后已停止临时测试服务器。
风险与范围
关联 Issue
无。