chore: restore regression coverage after WebUI retirement - #11101
Conversation
…tions Address the remaining review suggestions from the @qwen-code/webui retirement (#9812) and the deferred items tracked in #11076: - Fix the 01-architecture.md adapter diagram path (missing session/). - Correct the followup-suggestions doc: generation fires on end_turn, and headless/SDK daemon clients should opt out to avoid the per-turn LLM cost. - Pin playwright to 1.61.1 to restore version parity with @playwright/test. - Restore file: scheme rejection coverage in Markdown.test.ts. - Pin the historical sessionStorage key prefix with a literal assertion. - Cover the legacy exported JSONL rejection and ChatRecord happy path. - Restore the shell-substring classifier trap with a live path. - Point useDaemonFollowupSuggestion docs at ChatEditor, not the deleted InputForm. - Replace the theme-toggle timing-race assertion and exercise the unhandledrejection fail-closed path in the document browser gate. Closes #11076
github-actions
Bot
added
the
review/self-reported
|
Re-run at Gate: Stage 1-pre has no substantive complaint left — but it still trips, on a parser technicality. #11076 is closed as completed by #11107 (merged), so the gate compares this diff against Last round's asks are met: rebased onto The one line worth fixing: Linked Issues now reads "this PR neither reimplements nor closes #11076". GitHub's closing-reference parser is not intent-aware, so the Problem: observed, not theoretical. Node realpaths Direction: aligned. Restoring coverage that evaporated when a package was deleted is maintenance, not scope creep. Size: 220 lines / 12 files — roughly 41 production/config (25 of those the exporter helper), 73 test, 106 lockfile. No core-module path is touched: the two files under Approach: the scope is right now. The Risk: no Stage 1e match — nothing here touches the paths that correlate with post-merge reverts. Moving on to code review. 🔍 中文说明在 门禁：Stage 1-pre 已经没有实质性意见了——但仍会因为一个解析器的技术细节而触发。 #11076 已由 #11107（已合并）以 completed 关闭，所以门禁会把本 diff 与 上一轮的要求都已满足：已 rebase 到 唯一值得改的一行： Linked Issues 现在写的是「this PR neither reimplements nor closes #11076」。GitHub 的 closing-reference 解析器不判断语义，所以这句否定里嵌着的 问题： 是已观测到的，不是理论性的。Node 会对 ESM 主模块的 方向： 对齐。补回一个包被删除后蒸发掉的覆盖率，属于维护，不是范围蔓延。 规模： 220 行 / 12 个文件——大约 41 行生产/配置（其中 25 行是 exporter helper），73 行测试，106 行 lockfile。没有触及核心模块路径： 方案： 现在范围是合理的。那个不可能失败的 风险： Stage 1e 无命中——这里没有触及与合并后 revert 相关的路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewRead against the base tree at the reviewed commit; nothing in this PR was built or executed here — the test evidence below is the PR's own CI, read through the API. No Critical findings. My independent proposal for "restore the coverage that evaporated with the WebUI retirement" was: pin each lost boundary with a focused test, fix whatever made the module untestable, register the new lane in CI, and keep the committed manifest mirror in sync. That's what this does, so I spent the pass trying to break the assertions rather than looking for a different shape. Every assertion checks out against the implementation it claims to pin. I verified each one rather than taking the diff's word:
Two things worth a look, neither blockingThe symlink case is proven one level below the behaviour it claims. The same bug still sits in the shared helper. Smaller, take or leave: the new test's import closure is TestingEvidence carried here is the PR's own CI at the reviewed commit, read via the check-runs and job-log APIs. I did not build or run anything from this branch. The load-bearing lane is green, and I confirmed the new tests actually executed rather than inferring it from a green check —
The skipped
Sandboxed verification would settle one thing CI cannot: Not verified here: Windows behaviour of the realpath comparison. It is low-risk — both sides derive from the same OS path through the same function, and the exporter is a repo-internal CI helper that Windows lanes invoke with a literal path — but no lane at this head exercises it, and the new symlink test has no 中文说明代码审查在被审 commit 的基线树上静态阅读；本 PR 的任何代码都没有在这里被构建或执行——下面的测试证据来自 PR 自己的 CI，通过 API 读取。 没有 Critical 发现。对「补回随 WebUI 退役而蒸发的覆盖率」这个目标，我自己的方案是：为每个丢失的边界补一个聚焦测试、修掉让模块无法被测的那个问题、把新 lane 注册进 CI、并让提交的 manifest 镜像保持同步。这个 PR 做的正是这件事，所以我把这一轮花在尝试推翻它的断言上，而不是去找另一种形态。 每条断言都对照它声称要钉住的实现核对过了，我没有采信 diff 的自述：
两处值得看一下，都不阻塞软链接场景的证明层次低于它声称的行为。 同一个 bug 还在共享 helper 里。 更小的、可选：新测试的 import 闭包只有 测试这里承载的证据是 PR 自己在被审 commit 上的 CI，通过 check-runs 与 job-log API 读取。我没有从这个分支构建或运行任何东西。 关键的 lane 是绿的，而且我确认了新测试确实执行了，而不是从一个绿勾去推断——
被 skip 的 （上方表格由 finalize 任务在 CI 收敛后原地更新。） 沙箱验证能解决一件 CI 解决不了的事： 此处未验证：realpath 比较在 Windows 上的行为。风险低——两侧都由同一个 OS 路径经同一个函数推导，且 exporter 是仓库内部的 CI helper，Windows lane 用字面路径调用它——但当前 head 上没有 lane 会走到它，而新的软链接测试也没有 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — everything I could check statically checks out and CI substantiates the part that matters; the nits are real but none of them is a reason to hold this back. Stepping back against the proposal I wrote before reading the diff: this is what I would have done, and in two places it is better than what I would have done. Keeping the committed manifest mirror in the same commit as the rule it mirrors is the discipline that stops those two drifting, and registering the new file in The thing I was watching for, given why this PR exists, is a green test that pins nothing — that was the headline finding last round, and it is the failure mode this PR was opened to fix. That block is gone rather than reworded, which is the right resolution. What replaced it in the same file is stronger than what it superseded: The two notes in Stage 2 are the honest reservations, and I want to be clear about their weight. The symlink test proves the predicate rather than the end-to-end write; that is one level below the claim, and the repo already owns a harness that proves the stronger thing. And Six months from now this reads fine: the comments say why (the realpath asymmetry, the mutual-exclusivity invariant, the dep-free lane constraint) instead of narrating what, and every assertion is anchored to a named implementation. That's the part that keeps coverage from rotting again the next time a package is retired. Verdict is approve. CI has one PR workflow still in flight at this commit, so I'm not posting the approval in this run — it will land automatically, pinned to the commit below, once everything on it goes green. One mechanical thing a human needs to do. The one-line body fix from Stage 1 is still open and still worth doing: "neither reimplements nor closes #11076" keeps a live closing reference to an already-closed issue, because GitHub's parser doesn't read intent. Drop the keyword and this gate stops re-firing on every re-run. 中文说明Confidence: 4/5 —— 能静态核对的部分全部对得上，CI 也证实了最关键的那部分；这些瑕疵是真实存在的，但没有一个足以成为压住这个 PR 的理由。 对照我在读 diff 之前写下的方案回看：这就是我会做的，而且有两处比我本来会做的更好。把提交的 manifest 镜像和它所镜像的规则放在同一个 commit 里，是防止两者漂移的纪律；把新文件注册进 考虑到这个 PR 的由来，我最提防的就是一个钉不住任何东西的绿测试——那是上一轮的头号发现，也正是本 PR 要修的失败形态。那个代码块是被删掉而不是被改写，这是正确的处理方式。同一文件里替代它的内容比它取代的更强： Stage 2 里的两条备注是我真实的保留意见，我想把它们的分量说清楚。软链接测试证明的是谓词，而不是端到端的写文件；这比它声称的低一层，而仓库里已经有一个能证明更强命题的 harness。另外 六个月后再看这份代码是没问题的：注释写的是为什么（realpath 的不对称、互斥不变量、dep-free lane 的约束），而不是复述做了什么，每条断言都锚定在一个有名字的实现上。这正是下一次再有包被退役时，覆盖率不会再次腐坏的关键。 结论是 approve。当前 commit 上还有一个 PR workflow 在飞行中，所以这一轮我不发布 approval——它会自动落地，并钉在下面的 commit 上，等该 commit 上的一切都变绿。 有一件机械性的事需要人来做。 Stage 1 里那一行的修改仍然开放，也仍然值得做：「neither reimplements nor closes #11076」会让一个已关闭的 issue 保持有效的关闭引用，因为 GitHub 的解析器不判断语义。去掉这个关键词，这道门禁就不会在每次重跑时继续触发。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 1a": did not execute the two web-shell vitest files ( Markdown.test.ts , clientLifecycle.test.ts ); both were resolved by reading the implementations they assert ag…; "agent 6a": live mutation probe of Finding 1 (delete document-index.html:57 , run the fails closed… integration test under Chromium to observe it stay green) — resolved …; "agent 6a": running packages/web-shell vitest for Markdown.test.ts and clientLifecycle.test.ts — both verified against their implementations by reading instead.; "agent 2": enumerating all 21 stopReason: 'end_turn' producers in packages/cli/src/acp-integration/session/Session.ts to test whether a guard/loop-protection stop also….
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent 1a"：did not execute the two web-shell vitest files ( Markdown.test.ts , clientLifecycle.test.ts ); both were resolved by reading the implementations they assert ag…；"agent 6a"：live mutation probe of Finding 1 (delete document-index.html:57 , run the fails closed… integration test under Chromium to observe it stay green) — resolved …；"agent 6a"：running packages/web-shell vitest for Markdown.test.ts and clientLifecycle.test.ts — both verified against their implementations by reading instead.；"agent 2"：enumerating all 21 stopReason: 'end_turn' producers in packages/cli/src/acp-integration/session/Session.ts to test whether a guard/loop-protection stop also…。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
— qwen3.8-max via Qwen Code /review (v0.23.0)
The browser gate imports expect from vitest and drives playwright's Locator directly, so toHaveClass does not exist on the assertion and typecheck:integration failed with TS2339 on this line. Poll the class attribute through the same expect.poll idiom the neighbouring assertions use, which keeps the retry the assertion was switched to and needs no @playwright/test dependency. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtooas8sln
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 inert synthetic-unhandledrejection block — integration-tests/chat-transcript-document.test.ts:670 — already reported (comment 3940965876)
- R1-3 isMain path-spelling guard — integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js:169 — already reported (comment 3940965877)
- R1-5 stop-reason enumeration in the added parenthetical — docs/users/features/followup-suggestions.md:5 — already reported (comment 3940965878)
- R1-6 "unconditional" wording — docs/users/features/followup-suggestions.md:39 — already reported (comment 3940965880)
- R1-8 legacy-JSONL rejection path not reached — integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:39 — already reported (comment 3940965882)
- R1-9 hand-rolled ChatRecord factory — integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:15 — already reported (comment 3940965884)
- R1-10 untested negative side of the stop-reason gate — docs/users/features/followup-suggestions.md:5 — already reported (comment 3940965886)
- R1-11 node:test file under a vitest-mapped path — integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:13 — already reported (comment 3940965889)
Not explored to full depth (tool budget reached): "agent 1a": none — I did not execute the integration test (no node_modules in this review worktree and the renderer dist/ is unbuilt), so the claim "this block passes w….
中文说明
已审查。 建议见行内评论。
本轮确认的 8 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1a"：none — I did not execute the integration test (no node_modules in this review worktree and the renderer dist/ is unbuilt), so the claim "this block passes w…。
— qwen3.8-max via Qwen Code /review (v0.23.0)
`document-main.tsx` toggles `dark` and `light` on <html> mutually exclusively and nothing else writes `documentElement` classes, so the attribute is always exactly one of them. Matching `/light/` as an unanchored substring would also accept `dark light`, which is the theme-toggle breakage this poll exists to catch. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtow5pgslz
Node realpath-resolves `import.meta.url` for the ESM main module but leaves `process.argv[1]` as invoked, so comparing the raw spellings made a symlinked invocation skip `main()` and exit 0 having written nothing — a silent no-op the harness reads as success. Compare realpaths instead and export the predicate so both directions are pinned. Also extract the input gate into `assertRenderableJsonl` so the legacy exported-JSONL rejection can be asserted directly: the previous tests only pinned the two predicates feeding it, so deleting the guard and losing the remediation hint kept every test green. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtow5pgslz
main landed #11107 ("close the four deferred #9812 review follow-ups") while this branch was open, and it overlaps most of this branch's work. Conflict resolutions: - .github/scripts/ci/classify-platform-sensitivity.test.mjs: both sides appended a different substring-trap fixture to the same list; kept both (main's Shellfish.tsx, this branch's shellCommandProcessor.ts). The classifier itself is unchanged on both sides. - packages/web-shell/client/daemon/useDaemonFollowupSuggestion.ts: main's version is a superset of this branch's (same InputForm -> ChatEditor doc rename, plus the Prettier wrap and an extra note), so took main's. - integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js: main extracted the same input gate as selectChatRecords and added renderHtmlFromObjects, so dropped this branch's duplicate assertRenderableJsonl and kept main's structure. Kept this branch's realpath-based isMainModule guard, because main's inline path.resolve compare still misses a symlinked invocation, and folded main's typeof-string argv1 check into it. - export-html-from-chatrecord-jsonl.test.mjs: the gate is now covered by main's vitest suite (scripts/tests/export-html-from-chatrecord-jsonl.test.js), so this helper test keeps only the main-module cases, including the symlink one that nothing else covers. Verified locally: node --test on both helper test files (3/3 and 12/12 passing), node scripts/check-lockfile.js passing on the merged package.json/package-lock.json (playwright pinned to 1.61.1), no conflict markers, git diff --check clean. The web-shell vitest and Playwright additions were not run here (no node_modules in this worktree); they were checked against the implementations they pin, which main did not touch. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtp15qxqm5
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 inert synthetic-unhandledrejection block — integration-tests/chat-transcript-document.test.ts:671 — already reported (comment 3940965876)
- R1-5 stop-reason enumeration in the added parenthetical — docs/users/features/followup-suggestions.md:5 — already reported (comment 3940965878)
- R1-6 "unconditional" wording — docs/users/features/followup-suggestions.md:39 — already reported (comment 3940965880)
- R1-10 untested negative side of the stop-reason gate — docs/users/features/followup-suggestions.md:5 — already reported (comment 3940965886)
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": did not open react-markdown's defaultUrlTransform (applied upstream at Markdown.tsx:752 ) to confirm what entity/control-character normalization the URL has …; "agent 1b": I did not run the web_shell_e2e_smoke browser gate ( chat-transcript-document.test.ts ) locally — no Chromium installed here — so the strengthened .toBe('lig…; "agent 2": did not execute integration-tests/chat-transcript-document.test.ts (requires a Chromium install) — its new fail-closed block and theme assertion were reviewed….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
.github/scripts/ci/classify-platform-sensitivity.test.mjs:127 — [probe] fixture pins a host-coupled file as PLATFORM_INSENSITIVE.github/workflows/ci.yml:82 — [probe] new suite's only CI path is an unpinned hand-edited list
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：did not open react-markdown's defaultUrlTransform (applied upstream at Markdown.tsx:752 ) to confirm what entity/control-character normalization the URL has …；"agent 1b"：I did not run the web_shell_e2e_smoke browser gate ( chat-transcript-document.test.ts ) locally — no Chromium installed here — so the strengthened .toBe('lig…；"agent 2"：did not execute integration-tests/chat-transcript-document.test.ts (requires a Chromium install) — its new fail-closed block and theme assertion were reviewed…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
The parenthetical this PR added named `interrupted` as a daemon stop reason. The ACP `StopReason` union is `end_turn | max_tokens | max_turn_requests | refusal | cancelled`, so there is no `interrupted`; an interrupted turn reports `cancelled`. Name the four suppressing values the union actually defines so an integrator writing `stopReason === '...'` gets a branch that can fire. "unconditional" was wider than the code and contradicted the condition list on the same page: `Session.ts#maybeEmitFollowupSuggestion` returns early on the stop reason, the todo stop guard, `ui.enableFollowupSuggestions`, PLAN approval mode, a missing chat, and a non-model last history entry. Point at those conditions instead; the cost advice still holds because the `enableFollowupSuggestions === false` return sits before `generatePromptSuggestion` is reached. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtp6vibomd
`persists under the literal historical key prefix`, added on this branch, became a byte-equivalent twin of `writes under the historical WebUI key`, which reached main with #11107 and arrived here through the merge at this PR's head: same `persistStableClientId('client-a', 'session-a')`, same literal `qwen-code-webui-client-id:session:session-a`, same expectation. One fact was reported under two names, so a prefix rename reddened two tests and an auditor could not tell which copy was load-bearing. The surviving test's comment already carries the rename rationale, so dropping the copy loses no coverage. Verified with `cd packages/web-shell && npx vitest run client/daemon/session/clientLifecycle.test.ts`: 19 passed before, 18 passed after. Mutating SESSION_CLIENT_ID_STORAGE_PREFIX to `qwen-code-webshell-client-id:session:` after the dedupe still reddens 3 tests (15 passed), including the surviving `writes under the historical WebUI key`; reverting the mutation returns to 18 passed. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtp6vibomd
The block this PR added after the CDN-failure loop could not reach the
`window.addEventListener('unhandledrejection', showLoadError)` handler its
comment claimed to exercise. `page.route('**/*', route =>
route.abort('blockedbyclient'))` fails the `<script id="transcript-renderer">`
load, the template's capture-phase `error` listener (document-index.html:45-56,
matching `event.target.id === 'transcript-renderer'`) calls `showLoadError()`
first, and `setContent(..., { waitUntil: 'load' })` only resolves after that
subresource has failed - so `data-render-complete='error'` and the alert text
were already in place before `page.evaluate` dispatched anything, and
showLoadError's one-shot guard (`if (document.body.dataset.renderComplete)
return;`, document-index.html:30) made the synthetic rejection a no-op. The
block was a strict duplicate of the loop's own `rendererBody === null`
iteration: identical all-abort route, identical two assertions.
Exercising the handler for real needs a renderer body that passes the
document's pinned SRI integrity and then rejects during boot; no body this gate
can serve passes SRI, so the arm cannot be made honest here. Deleting it
returns the file to origin/main's state for this test - the handler and its
missing coverage are both pre-existing on main (packages/web-templates is not
in this PR's diff), so no coverage this PR was responsible for is lost.
Verified: `npm run typecheck:integration` reports 0 errors in this file (6
remain, all in packages/** against generated/dist paths absent from an unbuilt
worktree: channels/feishu, cli/src/generated/git-commit.js,
web-templates/src/generated/*); `npx prettier --check` and `npx eslint` clean
on the file. The browser gate itself was not executed locally - it needs the
full build chain plus Chromium.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtpfg5ewmp
…ts lane
This PR added
`integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs`,
the first `node:test` file under `integration-tests/`, and wired it into CI
through the `HELPER_TESTS` env list in `.github/workflows/ci.yml`. The tracked
review-context manifest still mapped the whole `integration-tests/**` path to
`recommendedTests: ['integration-tests']`, whose vitest lane collects
`**/*.test.ts` only (`integration-tests/vitest.config.ts:21`), so a future PR
touching only the exporter and its `.mjs` test is told to run a lane that
cannot collect the file - the same silent drop `HELPER_TESTS` exists to
prevent.
Add a narrower rule for `integration-tests/concurrent-runner/**` recommending
`helper-tests`. Matching rules merge rather than first-match, so the lane list
for that directory becomes the union and every other `integration-tests/` path
is untouched. The pinned `expectedManifest` in
`manifest-repository-context.committed.test.ts` moves with it, which is what
that pin is for.
Verified through the real provider (tsx importing
`packages/cli/src/commands/review/lib/manifest-repository-context.ts`) because
the packages/cli vitest globalSetup gate refuses to run on this unbuilt
worktree:
concurrent-runner .mjs -> ["helper-tests","integration-tests"]
concurrent-runner .js -> ["helper-tests","integration-tests"]
chat-transcript-document.test.ts -> ["integration-tests"] (unchanged)
all 10 rules co-matching -> non-null, 80 relatedPaths, and every
`paths` probe matched some rule
Also checked the committed manifest deep-equals the test's pinned literal (10
rules on both sides), and that dropping the new rule breaks that equality, so
the pin is not tautological. `npx prettier --check` and `npx eslint` clean on
both files.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtpfg5ewmp
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 — the triage gate stopped here, on a duplicate-scope check, before code review.
The linked issue is already done. This PR says Closes #11076, but #11076 was closed as completed by #11107 (fix(web-shell): close the four deferred #9812 review follow-ups, merged 2026-09-05). I checked all four tracked items against main, and every one has landed:
| #11076 item | where it lives on main |
|---|---|
literal qwen-code-webui-client-id:session: compat pin |
packages/web-shell/client/daemon/session/clientLifecycle.test.ts |
| legacy exported JSONL rejection + ChatRecord happy path | scripts/tests/export-html-from-chatrecord-jsonl.test.js |
Shellfish.tsx substring-trap fixture on a live path |
.github/scripts/ci/classify-platform-sensitivity.test.mjs (line 126) |
InputForm → ChatEditor / ChatPane in public docs |
packages/web-shell/client/daemon/useDaemonFollowupSuggestion.ts |
So merging this PR would close nothing — the reference is stale.
There is still a real delta, and it is not #11076's scope. None of the following exists on main:
export-html-from-chatrecord-jsonl.js—invokedDirectlyreplaced by an exportedisMainModule()that compares realpaths, plus a newnode:testfile pinning direct / symlinked / imported invocation, plus itsHELPER_TESTSregistration inci.ymlMarkdown.test.ts—file:scheme rejection for bothisSafeHrefandisSafeImageSrcchat-transcript-document.test.ts— the theme-toggle assertion replaced with a poll for exactlylightclassify-platform-sensitivity.test.mjs— one furthershellCommandProcessor.tsfixturedocs/users/features/followup-suggestions.md— the stop-reason gate wording and the headless/SDK opt-out notepackage.json/package-lock.json—playwrightmoved from^1.57.0to an exact1.61.1.qwen/review-context.jsonand its mirrormanifest-repository-context.committed.test.ts— a newintegration-tests/concurrent-runner/**→helper-testsdomain mapping
None of that is redundant work, and I am not asking you to drop it. The problem is that the PR no longer matches its own stated goal: it claims to close an issue that is closed, and what it actually carries is a different set of changes.
What I'd like before this goes further:
- Rebase onto current
main. The merge base (03c0909) is 25 commits behind. It does already contain #11107 — the diff removes theinvokedDirectly/fileURLToPathguard that #11107 added, and the classifier fixture list already carries theShellfish.tsxpath — which is why the overlap correctly disappeared from your later commits. - Reduce the PR to that delta and re-scope it: drop
Closes #11076, and retitle and redescribe it as what it now is — largely follow-ups to the/reviewfindings on this PR, plus a few independent items. - Consider splitting the drive-bys. The
playwrightpin, the followup-suggestions doc rewrite, and thereview-context.jsondomain mapping each stand on their own and would be easier to review and to revert separately.
One note so the two are not confused: a separate CHANGES_REQUESTED from the /review skill also stands on this PR, at the older commit 2a11002 and on different grounds. This comment is the triage duplicate-scope gate.
中文说明
@yiliang114 —— triage 门禁在此停下，原因是重复范围检查，尚未进入代码审查。
关联 issue 已经完成了。 本 PR 写的是 Closes #11076，但 #11076 已由 #11107（fix(web-shell): close the four deferred #9812 review follow-ups，2026-09-05 合并）以 completed 关闭。我把四项待办逐条对照 main 核实过，全部已经落地：
- 字面量
qwen-code-webui-client-id:session:兼容性断言 →packages/web-shell/client/daemon/session/clientLifecycle.test.ts - legacy exported JSONL 拒绝路径 + ChatRecord 正常路径 →
scripts/tests/export-html-from-chatrecord-jsonl.test.js - 使用仍存在的 Web Shell 路径补回
Shellfish.tsxsubstring-trap fixture →.github/scripts/ci/classify-platform-sensitivity.test.mjs（第 126 行） - 公共文档中
InputForm改为ChatEditor/ChatPane→packages/web-shell/client/daemon/useDaemonFollowupSuggestion.ts
因此合并本 PR 不会关闭任何东西——这个关联已经失效。
但仍存在真实的增量，而且不属于 #11076 的范围。 以下内容在 main 上都不存在：
export-html-from-chatrecord-jsonl.js——把invokedDirectly换成导出的isMainModule()（比较 realpath），并新增一个node:test文件钉住直接调用／软链调用／被 import 三种情形，外加ci.yml里的HELPER_TESTS注册Markdown.test.ts——为isSafeHref与isSafeImageSrc补上file:协议拒绝chat-transcript-document.test.ts——主题切换断言改为轮询精确等于lightclassify-platform-sensitivity.test.mjs——再补一条shellCommandProcessor.tsfixturedocs/users/features/followup-suggestions.md——stop-reason 触发条件的措辞，以及 headless/SDK 客户端应关闭的提示package.json/package-lock.json——playwright从^1.57.0改为精确的1.61.1.qwen/review-context.json及其镜像测试manifest-repository-context.committed.test.ts——新增integration-tests/concurrent-runner/**→helper-tests的领域映射
这些都不是重复劳动，我没有要求你丢弃它们。问题在于这个 PR 已经与它自己声明的目标不符：它声称要关闭一个已经关闭的 issue，而实际携带的是另一组改动。
在继续之前希望你做的：
- rebase 到当前
main。merge base（03c0909）已落后 25 个提交。它确实已经包含 #11107——本 diff 删除的正是 #11107 加入的invokedDirectly/fileURLToPath判定，classifier fixture 列表里也已经有Shellfish.tsx路径——这也是为什么后续提交中重叠部分已经正确消失。 - 把 PR 收敛到上述增量并重新定位范围：去掉
Closes #11076，把标题和描述改写成它现在的真实内容——主要是针对本 PR/review发现的跟进，外加几项独立改动。 - 建议拆分顺手带上的改动。
playwright版本固定、followup-suggestions 文档改写、review-context.json领域映射，这三项都能独立成立，分开更容易 review、也更容易回滚。
另外说明一下以免混淆：本 PR 上还有另一条来自 /review skill 的 CHANGES_REQUESTED，它针对的是较早的提交 2a11002，理由也不同。本条是 triage 的重复范围门禁。
— Qwen Code · qwen3.8-max-2026-09-02
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D4-6 unpinned hand-edited HELPER_TESTS list — already reported (review 5123901497, round-3 convergence-posture deferral at .github/workflows/ci.yml:82)
Not explored to full depth (tool budget reached): "agent 3c": I did not execute the vitest suites I reasoned about ( packages/web-shell/client/components/messages/Markdown.test.ts , integration-tests/chat-transcript-docum…; "agent reverse-audit (round 2)": did not execute packages/cli 's manifest-repository-context.committed.test.ts (it resolves the package entry into dist/ , which this worktree has not built)…; "agent reverse-audit (round 2)": did not run the chat-transcript-document.test.ts browser gate myself (needs a chromium download); the expect.poll(...).toBe('light') hunk is assessed from t…; "agent 2": verifying the doc's stop-reason claim ( end_turn -only generation, cancelled / refusal / max_tokens / max_turn_requests excluded) against the daemon's followu…; "agent 6b": did not execute packages/web-shell/client/components/messages/Markdown.test.ts under vitest — verified the two new file: assertions against Markdown.tsx:16…, and 3 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Test Plan (not a blocker): client/daemon/session/clientLifecycle.test.ts — no such file or directory.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
.github/workflows/ci.yml:82 — [test] the 25th HELPER_TESTS entry falsifies the list's own descriptions — ci.yml:832 still says "24-file", ci.yml:78 still scopes it to .github/scriptsintegration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:16 — [probe] the comment's reason for a second test home ("cannot exercise without spawning") is measured false — the graft passes 11/11 and still kills the muta…integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:16 — [review] Closes #11076, but #11107 delivered all four items inside the merge base and this PR's net diff to each is emptyintegration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:17 — [probe] its directory puts this test outside the qwen-triage flake gate — the only one of 25 HELPER_TESTS entries that never runs therepackage.json:161 — [probe] the pin falsifies the two-Playwright-trees premise qwen-triage.yml:3950-3958 states as present fact
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 3c"：I did not execute the vitest suites I reasoned about ( packages/web-shell/client/components/messages/Markdown.test.ts , integration-tests/chat-transcript-docum…；"agent reverse-audit (round 2)"：did not execute packages/cli 's manifest-repository-context.committed.test.ts (it resolves the package entry into dist/ , which this worktree has not built)…；"agent reverse-audit (round 2)"：did not run the chat-transcript-document.test.ts browser gate myself (needs a chromium download); the expect.poll(...).toBe('light') hunk is assessed from t…；"agent 2"：verifying the doc's stop-reason claim ( end_turn -only generation, cancelled / refusal / max_tokens / max_turn_requests excluded) against the daemon's followu…；"agent 6b"：did not execute packages/web-shell/client/components/messages/Markdown.test.ts under vitest — verified the two new file: assertions against Markdown.tsx:16…，另有 3 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
Test Plan（非阻断）：client/daemon/session/clientLifecycle.test.ts — no such file or directory。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Brings this PR up to date with main (00fe690) so it is mergeable again. Two files conflicted; both were mechanical and resolved as a union, so neither side's intent is lost. .github/workflows/ci.yml -- both sides appended to env.HELPER_TESTS. Kept main's whole block (its rewritten comment, the new HELPER_TESTS_DEP_FREE fast-lane list, and its update-ecs-runner-qwen-workflow.test.mjs entry) and appended this PR's entry integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs at the end, so the merged list is exactly main's 25 entries plus this PR's one: 26 entries, no duplicates, every path present on disk. The new suite is deliberately NOT added to HELPER_TESTS_DEP_FREE -- main scopes that list to .github/scripts node-builtin-only suites, and the full-profile helper step still runs the complete HELPER_TESTS list after the dependency install. Also bumped the adjacent "25-file HELPER_TESTS battery" comment to 26-file so the count next to the list stays true; that is the same one-word bump main made for 24 -> 25 when it added its own entry. integration-tests/chat-transcript-document.test.ts -- adjacent but disjoint assertions, not competing rewrites of one. Kept this PR's stricter `await expect.poll(...).toBe('light')` for the <html> class, and kept main's post-#11091 mermaid assertions (`div[class*="mermaidInline"]` count is 0, plus the fence source visible in body text), which replace the old `mermaidInline svg` poll that this PR never touched and that would now fail against main's stubbed-out mermaid. .size-baseline is left unchanged on purpose. It did not conflict (only main moved it, 131378 -> 134426), and the merged ci.yml is 135313 bytes: 887 over that record, inside the gate's 4096-byte GROWTH_ALLOWANCE, so no bump is owed by this PR. 808 of the 887 is pre-existing main-side drift, and the ratchet's design wants that reclaimed on main rather than silently absorbed here. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtpq5ybkn4
|
Merged
The new suite is deliberately not added to I also bumped the adjacent
Two things checked that could have been semantic but weren't:
Verified locally — this environment has no
Left to CI: everything that needs dependencies — Test, Lint, Integration, Desktop Shell and the web-shell E2E lanes. In particular the two assertions that now sit side by side in
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and integration-tests/chat-transcript-document.test.ts's browser gate did not run locally either (needs a Chromium install plus the built export-transcript-document.js asset); its typecheck half was covered locally and npm run typecheck:integration exits 0.
Not explored to full depth (tool budget reached): "agent 6b": did not execute the changed browser gate in integration-tests/chat-transcript-document.test.ts (needs a full workspace build plus a chromium download); the .….
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not reviewed: verification — the review posts findings, but no verifier was launched with a prompt this skill builds — they were ruled on, if at all, without the verdict bar its brief carries.
— [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Test Plan (not a blocker): client/daemon/session/clientLifecycle.test.ts — no such file or directory.
[Critical] An existing blocker on this PR still stands and this review is re-asserting it rather than filing a new claim: the triage gate's CHANGES_REQUESTED (review 5124792569) stopped this PR on a duplicate-scope check. Its ask 1 has landed — the branch merged current main at c5a45ce, so the merge base is no longer 25 commits behind. Its ask 2 has not: the description still carries Closes #11076, and #11076 is CLOSED (closedAt 2026-09-05T23:41:17Z) with all four of its tracked items satisfied at the reviewed commit by files this diff does not touch — the literal compat pin in packages/web-shell/client/daemon/session/clientLifecycle.test.ts, the legacy-JSONL rejection plus happy path in scripts/tests/export-html-from-chatrecord-jsonl.test.js, the Shellfish.tsx classifier fixture at .github/scripts/ci/classify-platform-sensitivity.test.mjs:126 (present in this diff only as unchanged context), and zero remaining InputForm references. Merging therefore fires a close keyword at an already-closed issue and closes nothing, while the description's "this PR restores the rest" and "fixes two stale documentation references" send a reviewer hunting for work the diff does not contain. This is the same ask the triage gate already made; it is not a new finding of this round.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and integration-tests/chat-transcript-document.test.ts's browser gate did not run locally either (needs a Chromium install plus the built export-transcript-document.js asset); its typecheck half was covered locally and npm run typecheck:integration exits 0.
未探索到全部深度（达到工具调用预算）："agent 6b"：did not execute the changed browser gate in integration-tests/chat-transcript-document.test.ts (needs a full workspace build plus a chromium download); the .…。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
未审查：验证——本次评审发布了发现，但没有验证 agent 是用本 skill 构建的 prompt 启动的——这些发现即便被裁定过，也缺失了 brief 承载的裁定标准。
— [unverified] 标记——验证者从未对它们作出裁决，它们不算已确认。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
Test Plan（非阻断）：client/daemon/session/clientLifecycle.test.ts — no such file or directory。
[Critical] An existing blocker on this PR still stands and this review is re-asserting it rather than filing a new claim: the triage gate's CHANGES_REQUESTED (review 5124792569) stopped this PR on a duplicate-scope check. Its ask 1 has landed — the branch merged current main at c5a45ce, so the merge base is no longer 25 commits behind. Its ask 2 has not: the description still carries Closes #11076, and #11076 is CLOSED (closedAt 2026-09-05T23:41:17Z) with all four of its tracked items satisfied at the reviewed commit by files this diff does not touch — the literal compat pin in packages/web-shell/client/daemon/session/clientLifecycle.test.ts, the legacy-JSONL rejection plus happy path in scripts/tests/export-html-from-chatrecord-jsonl.test.js, the Shellfish.tsx classifier fixture at .github/scripts/ci/classify-platform-sensitivity.test.mjs:126 (present in this diff only as unchanged context), and zero remaining InputForm references. Merging therefore fires a close keyword at an already-closed issue and closes nothing, while the description's "this PR restores the rest" and "fixes two stale documentation references" send a reviewer hunting for work the diff does not contain. This is the same ask the triage gate already made; it is not a new finding of this round.
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (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: 106 passed · 0 failed · 106 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：106 通过 · 0 失败 · 106 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11101 — Deep Verification ReportVerdict: 中文摘要结论： A/B 结论（中心主张）：本 PR 唯一的生产代码改动是导出脚本的 main-module 判定（改为比较 realpath）。用两个只差这一个 hunk 的构建做 A/B，以「是否真的写出 HTML 文件」为 oracle：base 在 2 种调用方式中只成功 1 种，head 成功 2 种。base 经软链调用时 依赖固定的 A/B：Playwright 版本一致性无法用 A/B 衡量（两侧代码相同），改用身份比较。按 测试非空转（mutation 矩阵）：12 行全部符合预期（29/29 断言）。要点：以保留接口的方式回退 realpath 语义，恰好只有软链那条测试变红、另两条保持绿 —— 证明新测试是承重的；而「整文件回退」会在 import 阶段就崩，是无效的空转检查（两者都记录在矩阵里）。分类器两个 fixture 不冗余：把 camelCase 边界当作分隔符的变异，只有本 PR 新增的 findings（均非阻塞）：① 未覆盖范围：本 checkout 深度为 2， ScopeCentral claim. One production-code change carries this PR: the exported-JSONL→HTML helper decided whether it was the main module by comparing Secondary claims (both verified).
Out of scope by choice: the full repo test suite; Windows/macOS; per-commit attribution (unreachable, see Not covered); the Central claim A/BTwo arms byte-identical except the main-module hunk (
base wrote 1/2 cells; head wrote 2/2. The base failure mode is the dangerous shape: exit 0, no stdout, no stderr, no artifact — a silent no-op that any harness reading an exit code scores as success. 15/15 assertions, including:
Witness: Secondary claim 1 — the Playwright pinA dependency pin has no A/B (both arms run identical code), so the oracle is an identity comparison. Base topology was reconstructed from
Base 4/4 split, head 4/4 identical. The four sites are The split was a real breakage, not a cosmetic version diff. This container has only Witness: A consequence the PR description does not claim
That premise is falsifiable, so it was measured on both arms (
So this PR removes the condition the workflow documents a workaround for. The workaround itself remains correct and should stay — it resolves the CLI from the harness's own directory, which is robust to hoist-layout changes and still lands on the same package. See Findings F2 for the now-stale comment. Witness: Residual divergence, accounted for. Secondary claim 2 — the restored tests hold down what they claimMutation/vacuity matrix, 12 rows, all matched expectation, 29/29 assertions. Every mutation ran in a scratch copy; the repository tree was never modified. Witness:
Two results deserve emphasis. M1b is the vacuity check a naive round would run, and it is the wrong one. Reverting the whole file removes the The two classifier fixtures are orthogonal, not redundant. M5 (camelCase counts as a compound separator — a plausible completion of a rule that already counts A note on harness discipline: an earlier revision of M5 mutated the regex to
|
| arm | recommendedTests for integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs |
|---|---|
| base | ["integration-tests"] |
| head | ["helper-tests","integration-tests"] |
Also asserted: reversing all 10 rules yields identical output (order-independence proven, not assumed); helper-tests is emitted for integration-tests/concurrent-runner/runner.py and not for integration-tests/chat-transcript-document.test.ts or packages/cli/src/index.ts (no over-reach); the manifest still passes the provider's fail-closed schema validation.
Theme-class assertion (chat-transcript-document.test.ts)
expect(await …getAttribute('class')).toContain('light') → await expect.poll(…).toBe('light'). Two separate questions, both answered (6/6 assertions):
- Strengthening is real.
toContain('light')accepts"dark light","light dark"and even"lightish";.toBe('light')rejects all three and agrees on the correct value. - Exactness is safe.
document-main.tsxhas exactly twodocumentElement.classListwrites, mutually exclusive toggles ondark/light, it is the only writer in the export bundle (git grepcensus overpackages/web-templates/src/export-html/**andpackages/cli/src/ui/utils/export/**), anddocument-index.htmlships<html lang="en" class="dark">— one class, no third token for.toBeto trip on. So the attribute is always exactlydarkorlight, anddark lightis unreachable today: this is defence against a future regression, not a live bug fix.
Reviewer Test Plan, walked step by step
| # | step as written | result |
|---|---|---|
| 1 | node --test .github/scripts/ci/classify-platform-sensitivity.test.mjs |
12/12 pass |
| 2 | node --test integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs |
3/3 pass (direct, symlinked, imported) |
| 3 | npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/export-html-from-chatrecord-jsonl.test.js |
10/10 pass — see Corrections: this file is not added by this PR |
| 4 | cd packages/web-shell && npx vitest run client/components/messages/Markdown.test.ts |
85/85 pass |
| 5 | npm run typecheck && npm run lint |
typecheck exit 0 (including typecheck:integration); lint exit 0 |
| 6 | npm ls playwright @playwright/test |
both 1.61.1, deduped — see the parity table above |
Step 5's typecheck:integration is the gate the PR's second commit says failed with TS2339 on toHaveClass; it is green here.
Every step was performable. In addition, the plan's own out-of-scope item was executed: "the chat-transcript-document.test.ts browser-gate changes require a full build plus a Chromium install, so they were not run locally; CI covers them." This container has both, so the gate was run — 5/5 pass in 37.4 s, the changed test taking 25.5 s (logs/gate-head.txt), with --retry=0 so no retry budget absorbed anything.
Witness for this table and the gates below: 05-reviewer-test-plan-gates.png.
Two further gates the PR's ci.yml change implicates, both green: .github/scripts/check-workflow-size.sh exits 0 with WORKFLOW_SIZE_BASE_SHA=HEAD^1 (ci.yml grew 79 bytes — 135234 → 135313, 1.9% of the 4096-byte baseline budget), and its vitest mirror passes 202/202. The HELPER_TESTS battery claim was checked mechanically rather than by eye: base 25 files / head 26, no duplicates, every listed path exists on disk, and the adjacent comment's 25-file→26-file edit matches the actual count on both sides.
Corrections to the PR description
These are corrections to the description, not requests to change code.
- Test-plan step 3 credits this PR with coverage it does not add.
scripts/tests/export-html-from-chatrecord-jsonl.test.jsis absent fromgit diff HEAD^1..HEAD. Per the merge commit, main extracted the same input gate asselectChatRecordsand addedrenderHtmlFromObjects, and this branch dropped its duplicateassertRenderableJsonl. The suite is genuinely good coverage and the step is worth running — but it landed via main, not here. The body's "Cover the legacy exported JSONL rejection and ChatRecord happy path" (commit 1's message) was superseded by that merge. - The aggregate diff is smaller than the commit list implies. Commit 1 also claims an
01-architecture.mdadapter-diagram path fix and auseDaemonFollowupSuggestiondoc rename; neither file appears inHEAD^1..HEAD, because the merge from main already carried equivalent changes (the merge commit says main's version was a superset and was taken). Commit 1's "Pin the historical sessionStorage key prefix with a literal assertion" was likewise added then removed by commit 7 as a byte-equivalent twin of what fix(web-shell): close the four deferred #9812 review follow-ups #11107 landed — correctly, and the body already says so. A reader auditing per-commit claims against the final diff will find gaps that are merges, not omissions.
Findings
All three are non-blocking. None is a merge condition, and none reproduced a defect in the PR's own behaviour.
F1 — Suggestion: the Playwright parity invariant has no enforcement
Root package.json now pins playwright to exact 1.61.1, while packages/web-shell/package.json declares @playwright/test: "^1.57.0" and packages/mobile-mcp/package.json declares "^1.49.0". The parity this PR restores therefore rests on a caret range happening to resolve to the pinned version today. Nothing holds it: scripts/check-lockfile.js validates lockfile↔package.json consistency (it passes, and would keep passing), and no lint rule, test, or CI step asserts that the two resolve to the same version — a repo-wide grep for a parity guard found none.
Failure scenario: Playwright publishes 1.62.x, someone bumps or regenerates @playwright/test, the lockfile moves it to 1.62.x while root playwright stays exact at 1.61.1, and the chromium revisions split again (1228 vs whatever 1.62 pins) — silently, with every gate green. The PR's own Risk section discloses the tradeoff ("a future Playwright bump must move both together"), so this is about the absence of a mechanism, not an undisclosed risk. Cheapest closing move: pin @playwright/test to the same exact version, or add an assertion to check-lockfile.js that the two resolve identically.
F2 — Nit: this PR falsifies the premise of a comment in qwen-triage.yml
The Install evidence browser step (around line 3950) justifies its directory-scoped CLI resolution with "This lockfile has TWO Playwright trees … which pins a different chromium revision — so npx playwright install would download a browser the harness cannot launch." Measured above: at head both paths resolve to 1.61.1 / chromium 1228, so for the harness path the two-tree premise no longer holds. (A second tree does still exist under packages/mobile-mcp, forced by mobilewright's exact pin, but it is not on any path this step resolves.)
The code should stay — resolving from the harness's own directory is correct regardless and is robust to hoist changes. Only the rationale is now stale, and a future reader will mis-infer why the indirection exists. Note when editing: scripts/tests/qwen-triage-workflow.test.js:5593 asserts expect(capture).toMatch(/from 'playwright'/) against this text, so the comment and that assertion must move together.
F3 — Nit (coverage gap, classified): isMainModule's realpath fallback is untested
M3 (delete the try { fs.realpathSync(argv1) } catch { resolved = path.resolve(argv1) } fallback) survived all 3 tests. Classified per the matrix protocol:
- Not dead code. Driven directly,
isMainModule('/definitely/not/here.js', …)returnsfalseat head and throws with the fallback removed — the clause decides an outcome. - Not redundant defence. No sibling hunk in this PR covers a nonexistent
argv1. - A coverage gap. The negative-case fixture points at
integration-tests/concurrent-runner/runner.py, which exists, sorealpathSyncsucceeds and the catch is never entered. The other negative input,undefined, is stopped earlier by thetypeofguard.
One-line fixture that would pin it: assert.equal(isMainModule(path.join(dir, 'does-not-exist.js'), exporterUrl), false). For the module's own invocation the branch is near-unreachable (Node just loaded argv[1]), so this is worth a test only because the predicate is exported.
Not covered
- Per-commit attribution. The checkout is depth 2 and
git rev-parse --is-shallow-repositoryistrue.git rev-list HEAD^1..HEAD^2returns 1 while the metadata snapshot lists 10 commits — the shallow boundary silently undercounts rather than erroring, so the two were compared explicitly. Only the aggregateHEAD^1..HEADdiff was verified; no per-commit table is presented. Note also that the snapshot'sbaseRefOid(00fe6904…) is older thanHEAD^1(1a86cd6c…), i.e. the merge ref was cut against a newer main;HEAD^1was used as base throughout, as the merge-ref contract requires. packages/climanifest-repository-context.committed.test.tscould not be run green in this container — environmental, and proven so. The container froze.qwen/review-context.jsonas-r--r--r-- root root(directory.qwenisdr-xr-xr-x root root) holding the base content with this PR's new rule removed, while the agent runs asuid=1000(node). The test'sreadCommittedFile()— despite its name — doesreadFileSync(join(repoRoot, …))against the working tree, so it reads the frozen base content and fails 1/5. An attempt to restore the committed content failed withPermission denied; sha256 confirmed the file was never modified (a71b02dc…before and after). The assertion was verified two other ways instead: the committed manifest and the test'sexpectedManifestliteral contain a byte-identical rule object, and the real provider was driven directly on both arms' committed content (11/11, above). A pristinegit worktreeat HEAD was also tried and hit the repo's ownglobalSetupguard for unbuilt workspace packages.- Trial merge into current
main. No network/refs beyond the depth-2 checkout, so the merge this PR would land as could not be re-measured. The head already contains two merges from main (the most recent resolving fix(web-shell): close the four deferred #9812 review follow-ups #11107's overlap), which is the same risk retired from the other direction. - The full repo test suite and the whole 26-file
HELPER_TESTSbattery. Only the files this PR touches or implicates were run (steps 1–4 above, plus the size-ratchet mirror). The PR's own CI covers the rest from a clean checkout. - Windows and macOS. Linux container only; the PR body likewise marks both untested. The realpath fix is POSIX-symlink-shaped — on Windows the equivalent would involve junctions/reparse points, which was not exercised.
- A live
npm installof the pinned dependency. Parity was measured against the already-installed tree plus a hardlink reconstruction of base's lockfile topology, not by resolving1.61.1from the registry. - Browser-gate timing margins.
expect.polluses vitest's default 1000 ms budget against a click-driven class flip. It passed at--retry=0, but the natural flip latency was not measured N times, so no margin assertion is reported. This job runs on a shared, loaded runner — the regime where such a test passes; a fast-machine failure could not be reproduced here by repetition. lintinitially reported 4215 errors, all self-inflicted. Every one was undertmp/— 611 files from a scratchgit worktreeand 22 from this artifact directory — with zero in the repository. The worktree was removed (git worktree remove --force), the artifact directory was staged outside the repo, andnpm run lintwas re-run on the pristine tree: exit 0. Recorded so the first number is not mistaken for a PR result.
Methodology
Linux container (node:22-bookworm lane), Node v22.23.2 / npm 10.9.8, working tree at the merge commit b7d6660c with npm ci + npm run build already completed by the workflow. The PR's effective diff is git diff HEAD^1..HEAD; the verified head is HEAD^2 = c5a45ce4, matching the snapshot's headRefOid.
Every harness is a .mjs file under this artifact directory and drives compiled or source-real code, never a stub of the unit under test: the A/B spawns real node child processes against two exporter copies that differ by exactly one hunk and asserts on the child's exit code, stdout, stderr and the artifact it leaves on disk; the parity harnesses resolve packages with createRequire from each site's own directory and read the chromium revision out of each playwright-core's shipped browsers.json; the manifest harness calls the real manifestRepositoryContextProvider.provide() from packages/cli/dist with each arm's committed manifest supplied through readIdentityFile; the mutation matrix runs node --test and vitest against scratch copies. The base dependency tree was built by hardlink-copying already-installed packages into the layout HEAD^1:package-lock.json describes, then validated entry-by-entry against that lockfile before any cell was trusted — hardlinks rather than symlinks because Node realpath-resolves symlinks and would have escaped the scratch tree back into head's node_modules, the exact confound the A/B exists to avoid.
The one in-place mutation (Markdown.tsx) ran under a bash EXIT trap with sha256 verification before and after; git status --porcelain at the end of the round shows only the pre-existing root-owned .qwen/review-context.json drift, which was never modified (an attempt to restore it was refused by permissions and the sha confirms it is untouched). Scratch worktrees were removed and the artifact directory was staged outside the repository while npm run lint ran, so no scratch file could be attributed to the PR.
The scratch base dependency tree was removed after its cells were captured (it was 32 MB of hardlinks into node_modules); logs/parity-base.txt and logs/workflow-premise.json hold its full output, and parity/run-parity.mjs rebuilds nothing on its own — to re-run the base arm, recreate parity/base-tree/node_modules by hardlink-copying (cp -al) packages/mobile-mcp/node_modules/playwright → playwright, node_modules/playwright-core → playwright-core, node_modules/@playwright/test → @playwright/test, node_modules/playwright → @playwright/test/node_modules/playwright, and node_modules/playwright/node_modules/playwright-core → @playwright/test/node_modules/playwright-core, then validate each version against git show HEAD^1:package-lock.json as the harness's first run did.
Raw per-cell stdout/stderr, build-free gate logs and every harness's JSON summary live in logs/; harnesses in ab/, parity/, manifest/, mutate/; image evidence in evidence/. Assertion counts in assertions.json were produced by aggregate.mjs, which re-parses those recorded logs rather than trusting any summary line, and counts only checks that actually executed — with fail reserved for unexpected outcomes, so base-arm cells that broke exactly as predicted are recorded as passes.
Flakiness gate log
integration test, out of gate scope: integration-tests/chat-transcript-document.test.ts
integration test, out of gate scope: integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs
rounds=5 files=3 skipped=2
file .github/scripts/ci/classify-platform-sensitivity.test.mjs: (cd .) node --test ./.github/scripts/ci/classify-platform-sensitivity.test.mjs
file packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/manifest-repository-context.committed.test.ts
file packages/web-shell/client/components/messages/Markdown.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/Markdown.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
.github/scripts/ci/classify-platform-sensitivity.test.mjs: PPPPP
packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: PPPPP
packages/web-shell/client/components/messages/Markdown.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 2 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 3 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 4 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 4 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 5 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 5 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
CI attribution for the two red checks at head
1.
The same check is cancelled at the same wall on two unrelated PRs, which is what rules out this diff:
Three unrelated diffs, one of which cannot touch the web-shell E2E surface at all, all killed at ~20m2x with no log. That is a job-level timeout / runner ceiling, not a regression. 2. Everything that actually gates is green at this head: So the state of this PR is: 0 unresolved review threads (12/12 resolved), the bot's own triage verification ✅ passed — merge-ready (106/106 scripted assertions, flakiness gate clean over 3 changed test files × 5 rounds), and the standing |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6b": did not verify whether integration-tests/concurrent-runner/runner.py exists — it decides only which branch of isMainModule the third test exercises (the as…; "agent 6b": did not verify the doc's second added sentence ("runs after every turn that meets the conditions above") against the daemon's full condition list — I confirmed …; "agent 6b": did not grep for --preserve-symlinks-main / NODE_OPTIONS across CI and npm scripts, which is what would have turned the unfilable isMainModule candidate a….
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Test Plan (not a blocker): client/daemon/session/clientLifecycle.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js:201 — [review] isMainModule realpaths only argv[1], so under --preserve-symlinks-main — the one configuration where the deleted comparison worked — a symlinked invocat…integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js:193 — [review] a third exported isMainModule with a third arity, while scripts/release-script-utils.js:61 already exports one lacking the realpath step; picking the -u…integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:25 — [probe] nothing pins that the module's own guard calls isMainModule — the guard mutant gives exit 0 with 0 output bytes while the .mjs suite stays 3 pass an…package.json:163 — [review] the exact playwright pin leaves the parity invariant unowned (twin still on a caret, so npm re-nests the deleted tree with no check going red) and falsifies the 'TWO Playwright trees' rationale qwen-triage.yml:39….github/scripts/ci/classify-platform-sensitivity.test.mjs:127 — [review] the only real path in a loop of five invented substring fixtures genuinely branches on os.platform(), so the pin cements the classifier's camelCase false negative unde…integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs:16 — [probe] the comment's reason for a second lane ('cannot exercise without spawning') is measurably false — the assertions graft into the existing vitest suit…
[Critical] R5-1: [certifies-falsely] [new-surface] An existing blocker on this PR still stands and this review is re-asserting it rather than filing a new claim. The PR description still carries Closes #11076, but #11076 is CLOSED (closedAt 2026-09-05T23:41:17Z) and all four of its tracked items are satisfied at the reviewed commit by files this diff does not touch: the literal compat pin at packages/web-shell/client/daemon/session/clientLifecycle.test.ts:98,105,120; the legacy-JSONL rejection plus ChatRecord happy path at scripts/tests/export-html-from-chatrecord-jsonl.test.js:78-85,140 (that file, blob 4a60f165, and selectChatRecords at base line 143 both exist AT THE MERGE BASE — git diff --stat <merge-base> HEAD -- scripts/tests/export-html-from-chatrecord-jsonl.test.js is empty); the Shellfish.tsx classifier fixture at .github/scripts/ci/classify-platform-sensitivity.test.mjs:126, which appears in this diff only as a context line (the sole added line is 127); and zero remaining InputForm references under packages/web-shell. Merging therefore fires a close keyword at an already-closed issue and closes nothing, while the description's '#11107 has since restored the sessionStorage key coverage on main; this PR restores the rest' has no rest to restore and 'fixes two stale documentation references that pointed at renamed or deleted artifacts' describes work the delta does not contain — the only doc file touched is docs/users/features/followup-suggestions.md, whose two hunks are the end_turn stop-reason gate and the headless/SDK opt-out note, and whose merge-base version carries no InputForm or webui reference at all. Meanwhile the real delta (the isMainModule realpath fix and its suite, the file: scheme rejections, the camelCase classifier fixture, the theme-poll assertion, the playwright pin, the concurrent-runner domain mapping) has no issue describing it, so none of it is traceable. This is the same ask the triage gate made at review 5124792569: ask 1 (merge current main) landed at c5a45ce, ask 2 (drop the closing keyword and rescope the description) has not. Witness: GitHub's own closing-reference resolver, reached by raw GraphQL after the platform CLI's --json field proved unavailable — pullRequest(number:11101){ closingIssuesReferences{ nodes{...} } } returned {"state":"OPEN","closingIssuesReferences":{"nodes":[{"number":11076,"state":"CLOSED","closedAt":"2026-09-05T23:41:17Z"}]}}; per item at the reviewed commit, grep -c 'qwen-code-webui-client-id:session:session-a' clientLifecycle.test.ts gives 3 at both head and merge base with the file absent from the 11 diff paths, selectChatRecords sits at BASE line 143, diff hunk @@ -124,6 +124,7 @@ shows Shellfish.tsx as context with 127 the only added line, grep -rn InputForm packages/web-shell --include='*.ts*' | wc -l gives 0, and git merge-base main HEAD gives a4a6225, which is main's tip. The PR body fetched live still carries Closes #11076 in both the English and the Chinese blocks. Fix: drop Closes #11076 from Linked Issues and correct the 'this PR restores the rest' and 'fixes two stale documentation references' sentences, retitling and redescribing the PR as the delta the triage gate enumerated (its seven bullets were each re-verified as absent from main), keeping every code change; if a link is wanted, reference #11076 and #9812 as motivating prior work without a closing keyword. No fix witness applies — the fix is PR metadata and description only, so it adds no guard, branch or behaviour a test can pin.
中文说明
未探索到全部深度（达到工具调用预算）："agent 6b"：did not verify whether integration-tests/concurrent-runner/runner.py exists — it decides only which branch of isMainModule the third test exercises (the as…；"agent 6b"：did not verify the doc's second added sentence ("runs after every turn that meets the conditions above") against the daemon's full condition list — I confirmed …；"agent 6b"：did not grep for --preserve-symlinks-main / NODE_OPTIONS across CI and npm scripts, which is what would have turned the unfilable isMainModule candidate a…。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
Test Plan（非阻断）：client/daemon/session/clientLifecycle.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
[Critical] R5-1: [certifies-falsely] [new-surface] An existing blocker on this PR still stands and this review is re-asserting it rather than filing a new claim. The PR description still carries Closes #11076, but #11076 is CLOSED (closedAt 2026-09-05T23:41:17Z) and all four of its tracked items are satisfied at the reviewed commit by files this diff does not touch: the literal compat pin at packages/web-shell/client/daemon/session/clientLifecycle.test.ts:98,105,120; the legacy-JSONL rejection plus ChatRecord happy path at scripts/tests/export-html-from-chatrecord-jsonl.test.js:78-85,140 (that file, blob 4a60f165, and selectChatRecords at base line 143 both exist AT THE MERGE BASE — git diff --stat <merge-base> HEAD -- scripts/tests/export-html-from-chatrecord-jsonl.test.js is empty); the Shellfish.tsx classifier fixture at .github/scripts/ci/classify-platform-sensitivity.test.mjs:126, which appears in this diff only as a context line (the sole added line is 127); and zero remaining InputForm references under packages/web-shell. Merging therefore fires a close keyword at an already-closed issue and closes nothing, while the description's '#11107 has since restored the sessionStorage key coverage on main; this PR restores the rest' has no rest to restore and 'fixes two stale documentation references that pointed at renamed or deleted artifacts' describes work the delta does not contain — the only doc file touched is docs/users/features/followup-suggestions.md, whose two hunks are the end_turn stop-reason gate and the headless/SDK opt-out note, and whose merge-base version carries no InputForm or webui reference at all. Meanwhile the real delta (the isMainModule realpath fix and its suite, the file: scheme rejections, the camelCase classifier fixture, the theme-poll assertion, the playwright pin, the concurrent-runner domain mapping) has no issue describing it, so none of it is traceable. This is the same ask the triage gate made at review 5124792569: ask 1 (merge current main) landed at c5a45ce, ask 2 (drop the closing keyword and rescope the description) has not. Witness: GitHub's own closing-reference resolver, reached by raw GraphQL after the platform CLI's --json field proved unavailable — pullRequest(number:11101){ closingIssuesReferences{ nodes{...} } } returned {"state":"OPEN","closingIssuesReferences":{"nodes":[{"number":11076,"state":"CLOSED","closedAt":"2026-09-05T23:41:17Z"}]}}; per item at the reviewed commit, grep -c 'qwen-code-webui-client-id:session:session-a' clientLifecycle.test.ts gives 3 at both head and merge base with the file absent from the 11 diff paths, selectChatRecords sits at BASE line 143, diff hunk @@ -124,6 +124,7 @@ shows Shellfish.tsx as context with 127 the only added line, grep -rn InputForm packages/web-shell --include='*.ts*' | wc -l gives 0, and git merge-base main HEAD gives a4a6225, which is main's tip. The PR body fetched live still carries Closes #11076 in both the English and the Chinese blocks. Fix: drop Closes #11076 from Linked Issues and correct the 'this PR restores the rest' and 'fixes two stale documentation references' sentences, retitling and redescribing the PR as the delta the triage gate enumerated (its seven bullets were each re-verified as absent from main), keeping every code change; if a link is wanted, reference #11076 and #9812 as motivating prior work without a closing keyword. No fix witness applies — the fix is PR metadata and description only, so it adds no guard, branch or behaviour a test can pin.
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
State at head Threads: 0 unresolved of 12 (paginated to Checks: 23 pass / 27 skipping / 0 fail, 0 pending. The two reds attributed to infrastructure at the previous head The standing CHANGES_REQUESTED is the bot's own review submitted at 2026-09-07T18:28:02Z on this same head. It is explicitly a partial review — "Not explored to full depth (tool budget reached)" — and it carries no inline findings. Its One correction to that review. It discloses:
That file exists at this head. Verified against the tree at ref
So the Test Plan reference in the description is correct and that disclosure is a sandbox miss on the review's side, not a defect here. The gaps that review does disclose are honest, and none of them was requested as a change: Firing |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (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: 109 passed · 0 failed · 109 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：109 通过 · 0 失败 · 109 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11101 — Deep Verification Report (follow-up round)Verdict: Three findings carry forward and all three still stand (F1 Suggestion, F2 Nit, F3 Nit). One claim the previous round never covered — the 中文摘要结论： 本轮性质：这是跟进轮。上一轮在 head A/B 结论（中心主张，重测）：本 PR 唯一的生产代码改动仍是导出脚本的 main-module 判定（改为比较 realpath）。两个只差这一个 hunk 的构建（改动行逐字比对，2 个 hunk、25 行，无一行越界）做 A/B，以「是否真的写出 HTML」为 oracle：base 2 种调用方式只成功 1 种，head 2/2。base 经软链调用时 本轮新增，两项上一轮没有的证据：
依赖固定的 A/B（重测）：版本一致性无法用 A/B 衡量（两侧代码相同），改用身份比较。按 上一轮 findings 状态（全部重测，三条全部 stands）；本轮另新增 F4（见上）：F1 stands（且本轮把「假设的未来」变成「今天就能复现」：caret 范围今天解析到 1.63.0，其 playwright-core 钉 chromium 1243 ≠ 固定的 1.61.1 的 1228，而 本轮新覆盖的文档主张（并因此新增一条 finding F4）： 未覆盖范围：逐提交归因不可达（深度 2， Previous-finding statusRe-measured at the new head; every number below comes from a harness that ran this round (
| F4 | NEW this round — the doc's new sentence points a daemon reader at a condition list that describes the CLI, not the daemon | Suggestion | new | See Findings below. 5 scripted checks in I agree with all three carried-forward severities as filed. Nothing was declined by the author, so there are no declined rows to adjudicate. F4 is new this round and is described in full under Findings. Witness for all 21 rows below, including the positive control and the candidate fixture: One methodological catch worth recording, because it changes how F3 must be reproduced: my first M3 run killed the mutant, which contradicted the previous round. The cause was my scratch directory, not the code — the suite's negative case points at ScopeCentral claim. One production-code change carries this PR: the exported-JSONL→HTML helper decided whether it was the main module by comparing Secondary claims (both re-verified).
Claim added this round (not covered previously). The Out of scope by choice: the full repo test suite; Windows/macOS; per-commit attribution (unreachable); the Central claim A/BTwo arms side by side in The internal workspace closure is asserted, not assumed:
base wrote 1/2 cells; head wrote 2/2 (22/22 assertions). The base failure mode is the dangerous shape: exit 0, no stdout, no stderr, no artifact — a silent no-op that any harness reading an exit code scores as success. Fidelity, not just "a file appeared": the symlinked artifact carries all 3 transcript blocks ( Normaliser controls. Raw sha256 is an invalid oracle here, and this round measured exactly why: two renders of identical input differ in precisely two literal strings — a fresh CSP nonce appearing in five positions (including Witness: Sibling sweep — the same root cause admits more than one shapeThe reported shape is a symlinked file. The mechanism is "argv[1]'s spelling is not physical", so seven shapes were driven through both arms, with predictions written down before running (20/20 matched).
base silently no-ops on 3 of 7; head on 0 of 7. All 11 writing cells rendered the same document. Two consequences are disproved, and bounding them is the point: S5 does not break base because Node's Witness: The consumer announces an artifact that does not exist
if result.returncode == 0:
self.console.print(f"[dim]Rendered chat HTML saved: {rendered_html_path.name}[/dim]")
else:
self.console.print(f"[yellow]Warning: HTML exporter failed: {result.stderr}[/yellow]")So the base arm's silent no-op is not a missing file — it is a false success message with no warning. This was driven end to end rather than argued: the exporter block (lines 329–348) and
Claim-vs-filesystem agreement: base 1 of 2, head 2 of 2 (10/10 assertions). The head artifact renders the conversation ( Witness: Secondary claim 1 — the Playwright pinA dependency pin has no A/B (both arms run identical code), so the oracle is an identity comparison. Base topology was reconstructed from
base splits 2/2, head agrees 2/2 (18/18 assertions). The census is a fresh Two independent instruments agree on every cell — each package's own live The split was a real breakage, not a cosmetic version diff. This container has only Residual divergence, accounted for. Head keeps Witness: Docs claim (new this round) — the stop-reason enumeration is complete and exactThe rewritten sentence names a export type StopReason = "end_turn" | "max_tokens" | "max_turn_requests" | "refusal" | "cancelled";Exactly five members. The code's gate is an allowlist, if (result.stopReason !== 'end_turn') return;so the complement is exactly the four the doc names — no invented values, none missing. The union is corroborated in three more places: the SDK's runtime zod validator ( Two refinements the enumeration alone does not settle, both scripted (18/18 assertions across docs + manifest):
Witness for this section and the manifest table that follows: Secondary claim 2 — the restored tests and the new routing rule
No over-reach:
Mutation rows re-run this round are in the F3 table above, including the positive control that proves the harness can fail that suite. Reviewer Test Plan, walked step by step
Every step was performable, and step 5's lint half was proven live rather than accepted as green: an unused variable planted in the PR's own new test file was reported ( Step 6's output also carries a pre-existing, unrelated The plan's own out-of-scope item was executed anyway: "the Step 4's Witness for this whole table, including the gates the PR implicates beyond the plan: FindingsNo blocking finding. The three carried-forward findings all stand, with the re-measured evidence in the status table at the top. One new Suggestion (F4) was found this round in the docs edit. F4 — Suggestion (new): the new sentence sends a daemon reader to a list that describes the CLIThis PR adds, verbatim:
The advice itself is correct and worth shipping — it was measured: The problem is the referent. "the conditions above" is the
And the daemon has three conditions the list omits entirely: So a headless/SDK integrator — precisely the reader this new sentence addresses — is told the daemon fires on "every turn that meets the conditions above", and two of those conditions are things the daemon never looks at while three things it does look at are missing from the list. Attribution, kept separate. The bullet list is pre-existing: Minimal suggested fix (preserves the commit's intent; <b>not applied, not measured</b>)Scope the referent instead of widening the list — the sentence only needs the conditions the daemon actually applies: Suggestions will not appear in the CLI's non-interactive mode (e.g., headless/SDK mode). In the daemon, generation is server-side: it runs after every turn that ends with the `end_turn` stop reason and meets the enabled/approval-mode conditions above (the error and pending-dialog conditions are CLI-only), so a headless or SDK client that cannot render the suggestion should set `ui.enableFollowupSuggestions` to `false` to avoid the per-turn LLM cost.Per the contract this is not a measured fix: it was not applied to a scratch copy and driven through a harness, because the oracle here is prose accuracy rather than an executable assertion. The five scripted checks that establish the underlying facts are measured, and are listed in Their suggested closings are unchanged and were not applied, so none is "measured" in the sense the contract requires of a fix I ship — they are reported as findings, not as patches. For completeness, the sharpest form of F1 as measured this round: the invariant is not merely unenforced, it is one lockfile regeneration away from breaking today. What was tested and did NOT hold. No defect was found in this PR's executable behaviour; F4 is a prose inaccuracy in the docs edit, not a code fault. Specifically disproved rather than merely unobserved: the fix does not regress direct or relative invocation (S1/S4/S5/S6 write on both arms, and head+direct normalises to the same digest as base+direct); importing the module still runs nothing on either arm (S8, with cwd left empty); the new routing rule does not over-reach beyond Not covered
Evidence images
MethodologyLinux container ( Every harness is a Mutation work ran in scratch copies under Raw per-cell stdout/stderr and every harness's output live in Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
yiliang114
changed the title
|
Resolved the remaining Critical at head
Per the repository's round-6 convergence rule, the four non-blocking findings from the final verification are deferred rather than widening this PR: Playwright manifest parity enforcement (F1), the stale evidence-browser comment (F2), the missing-path Verification: |
|
@qwen-code /triage |
|
CI follow-up: |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (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: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：143 通过 · 0 失败 · 143 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11101 — Deep Verification Report (follow-up round, 3rd)Verdict: This is a follow-up round. The previous report ran at head The delta since the previous round is two commits: Four findings carry forward and all four still stand (F1 Suggestion, F2 Nit, F3 Nit, F4 Suggestion). One new finding (F5, Suggestion) came out of the sibling sweep the central fix invites: the repo ships three other 中文摘要结论： 本轮性质：第三轮跟进。上一轮 head 本轮 delta：自上轮只多了两个提交 —— 一个 main 合并，以及 中心主张 A/B（重测）：唯一的生产代码改动仍是导出脚本的 main-module 判定改为比较 realpath。两个只差这一个 hunk（4 hunks / −5 / +20 行，逐字比对预期行集）的构建，以「是否真的写出 HTML」为 oracle，驱动 8 种调用形状 × 2 个构建，预测先写下再跑，16/16 命中：base 4/7 写出，head 7/7。base 的三个失败单元格都是最危险的形状 —— exit 0、stdout/stderr 全空、不写任何文件。11 个写出单元格规范化后折叠为同一摘要；真实提交 fixture 渲染出 4 个 block、合成 fixture 渲染出 3 个 block， 上轮 findings 状态（全部重测，四条全部 stands）：F1 stands（caret 今天解析到 1.63.0 → chromium 1243 ≠ 固定 1.61.1 的 1228； 本轮新增 F5（Suggestion）： Reviewer Test Plan：6 步全部可执行且全部通过（12/12、3/3、10/10、85/85、typecheck 0 条 未覆盖范围：逐提交归因不可达（快照 13 个提交， Previous-finding statusRe-measured at the new head; every number comes from a harness that ran this round.
Nothing was declined by the author, so there are no declined rows to adjudicate. I agree with all four carried-forward severities as filed. Witness: ScopeCentral claim (unchanged from last round, re-measured). One production-code change carries this PR: the exported-JSONL→HTML helper decided whether it was the main module by comparing Delta claim (new this round). Secondary claims (re-verified). (1) Pinning Out of scope by choice: the full repo test suite; the whole 26-file Delta A/B — the pnpm lockfile sync is load-bearing, and it is complete
Three arms, differing only in the two files the PR touches inside that gate's input closure. The closure is asserted, not assumed: The shipped bootstrap was run verbatim in each arm, with the registry reachable:
base 0, mid 1, head 0 (14/14 assertions in Two things fall out of that block, and both are asserted rather than read:
What the sync does and does not change. The pnpm-lock resolved version for playwright is Methodology note, recorded because it changed the result. The first offline run was non-discriminating: all three arms died on Witness: Central claim A/B — re-measured at the new headTwo arms differing by exactly the main-module hunk. The changed-line set is compared literally, not by pattern: The internal workspace closure is asserted: Predictions for all eight shapes were written down before running; 16/16 matched.
base wrote 4/7; head wrote 7/7 (60/60 assertions). Every base no-op is the dangerous shape: exit 0, empty stdout, empty stderr, no artifact — a harness that only reads the exit code scores all three as success. Fidelity, not just "a file appeared." All 11 writing cells produce the same byte count and collapse to one normalised digest ( Two consequences are disproved, and bounding them is the point. S5 does not break base because a relative Witness: Secondary claim 1 — the Playwright pinA dependency pin has no A/B (both arms run identical code), so the oracle is an identity comparison — see the F1/F2 rows in the status table and
Reviewer Test Plan, walked step by step
Every step was performable. Step 6's output still carries a pre-existing, unrelated Gates the PR implicates beyond the plan: Step 2's suite is not vacuous (measured, not assumed): reverting only the realpath semantics while keeping the export intact — so no import or compile break — reddens exactly the test it exists for, Witness: FindingsNo blocking finding. The four carried-forward findings all stand (status table above). One new Suggestion (F5) was found this round. F5 — Suggestion (new): three other
|
| implementation | compares | on a symlinked invocation |
|---|---|---|
integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js (this PR) |
realpaths | true |
scripts/release-script-utils.js:61 — imported by generate-changelog.js, generate-release-notes.js, upload-aliyun-oss-assets.js, verify-installation-release.js |
path.resolve(process.argv[1]) === filename, filename = fileURLToPath(importMetaUrl) |
false |
scripts/build-standalone-release.js:178 |
path.resolve(process.argv[1]) === __filename, __filename = fileURLToPath(import.meta.url) |
same expression |
scripts/create-standalone-package.js:219 |
identical to the above | same expression |
Driven live, both helpers imported into one process and one symlinked invocation of the probe:
node /…/sibling/sibling-link.mjs (symlink → sibling-probe.mjs)
argv[1] = …/sibling-link.mjs ← LOGICAL spelling
import.meta = …/sibling-probe.mjs ← realpath-resolved
shared helper (release-script-utils) → false PR's fixed helper → true
direct invocation control: shared → true, fixed → true
So the divergence is measured, not inferred: the shipped shared helper skips the main block on a symlinked invocation and the process exits 0 having done nothing — exactly the failure mode this PR's commit message describes, in a file this PR does not touch. No test anywhere covers it (git grep isMainModule finds no scripts/**/*.test.* hit).
Bounded, and the bound is what keeps this a Suggestion. Every invocation site found by grepping .github/, package.json and Makefile is a plain node scripts/….js (finalize-release.yml:129,196, package.json:91,92,95, web-shell-visuals-publish.yml); 0 reach these through a symlink. The bug is therefore latent, not live.
This is not a request to reuse the shared helper. Its signature differs (isMainModule(importMetaUrl) reads process.argv[1] itself) and it lives in release tooling, so importing it from integration-tests/concurrent-runner/ would create a new cross-tree dependency for a 9-line predicate. The finding is narrower: three copies of a bug this PR just fixed remain in the repo, and the cheapest closing move is a follow-up that fixes scripts/release-script-utils.js in place (the two file-local copies are byte-identical in shape) rather than anything in this PR.
F1, sharpest form as measured this round
The invariant is not merely unenforced, it is one lockfile regeneration away from breaking today: @playwright/test@^1.57.0 resolves to 1.63.0 right now, which pins chromium 1243, while root playwright is exact-pinned at 1.61.1 → 1228. node scripts/check-lockfile.js passes on the pinned tree and would keep passing after such a bump, because it validates lockfile↔manifest consistency and contains no playwright logic. Cheapest closing move remains: pin @playwright/test to the same exact version, or assert the two resolve identically in check-lockfile.js. The PR body already tracks this as #11336.
What was tested and did NOT hold. No defect was found in this PR's executable behaviour. Specifically disproved rather than merely unobserved: the pnpm sync is not partial (pnpm enumerated exactly one mismatch, and head is green with no lockfile rewrite); the realpath fix does not regress direct, relative, symlinked-cwd or hard-link invocation (S1/S4/S5/S6 write on both arms, and head+direct normalises to the same digest as base+direct); importing the module still runs nothing on either arm; the PR's central new test is not vacuous; the two isMainModule helpers agree whenever nothing is symlinked; and the residual second Playwright tree is forced by an upstream exact pin and sits off every path this PR touches.
Not covered
- Per-commit attribution. Depth-2 checkout,
git rev-parse --is-shallow-repository=true.git rev-list HEAD^1..HEAD^2returns 1 while the snapshot lists 13 commits — the shallow boundary undercounts silently rather than erroring, so the two were compared explicitly. The previous round's OIDs (a3ddb060,a4a62258) and the snapshot'sbaseRefOid(6ba04fc2) are all unreachable (git cat-file -tfails), so no diff-against-last-round was possible. Only the aggregateHEAD^1..HEADdiff was verified; the two-commit delta was inferred from the snapshot's commit list plus the aggregate diff growing by exactlypnpm-lock.yaml(1 file, 1 insertion, 1 deletion). npm run lintin full (its first half,eslint .). Not run:tmp/is gitignored but is not ineslint.config.js's globalignores, and this round's artifact directory lives there, so a whole-repoeslint .would report thousands of self-inflicted errors. Lint was scoped instead to all 6 changed lintable files (exit 0) andeslint integration-tests(exit 0).packages/was not linted as a whole; the PR's only changes there are two test files, both linted individually. The lint gate was not proven live this round — the previous round planted a violation and confirmed it was reported; that plant-and-restore was not repeated here, so treat "live" as carried-forward prose, not as re-measured.- The browser gate (
integration-tests/chat-transcript-document.test.ts, whosetoContain('light')→expect.poll(...).toBe('light')change is in this diff). Not re-run this round — budget went to the new pnpm surface. The previous round ran it 5/5 in 29.2 s with--retry=0and confirmed the changed line executed inside a real browser (20 986 ms of the run). The PR's edit to that file is unchanged in the aggregate diff, but per the re-measurement rule this is not re-measured, not "re-measured and unchanged". Its poll timeout margin against the theme transition was never measured on any round, and this runner is shared and loaded — the regime where such a test passes. - Mutation rows for the classifier fixture and
Markdown.tsx. Not re-run (budget). Both suites are green at head (12/12 and 85/85) and neither production file (classify-platform-sensitivity.mjs,Markdown.tsx) appears in this PR's diff. packages/climanifest-repository-context.committed.test.tsin the main working tree. Not re-run, and the A/A worktree control the previous round used was not repeated. The precondition it diagnosed was re-measured and still holds:.qwen/review-context.jsonis-r--r--r-- root rootin the working tree and its content is the base manifest (a scripted read confirms it does not containintegration-tests/concurrent-runner/**), while the agent runs asuid=1000(node);readCommittedFile()reads the working tree, not the index.git statusshows the same single pre-existingM .qwen/review-context.jsondrift. So a failure there would be environmental, but this round asserts only the precondition, not the outcome. The routing rule's effect was therefore not driven through the real provider this round either — the previous round measured base["integration-tests"]→ head["helper-tests","integration-tests"]; treat that as carried-forward prose.- Trial merge into current
main. No refs beyond the depth-2 checkout. - The full repo test suite and the whole 26-file
HELPER_TESTSbattery. Only files this PR touches or implicates were run. Theci.ymlbattery contents were not re-counted this round (the previous round counted base 25 → head 26 mechanically); the diff hunk adding the 26th entry and the matching25-file→26-filecomment edit are both visible ingit diff HEAD^1..HEAD. - Windows and macOS. Linux container only; the PR body marks Windows untested. The realpath fix is POSIX-symlink-shaped — S3 models the macOS
/tmp→/private/tmpcase mechanically, but nothing ran on Darwin and Windows junctions/reparse points were not exercised at all. - A
pnpm installfrom a genuinely cold store. The head arm's online run reused this container's warm/__w/.pnpm-store/v11, and the base arm ran after head had populated it. Both are green, but a first-ever cold install on a runner was not reproduced. The shape of the gate failure is reproduced (real pnpm, real manifests, real lockfiles, verbatim shipped command); the smoke job's behaviour on a fresh runner is the PR's own CI's business, not this round's.
Methodology
Linux container (node:22-bookworm lane), Node v22.23.2 / npm 10.9.8 / corepack pnpm 11.24.0, working tree at the merge commit 7ee68f99 with npm ci + npm run build already completed by the workflow. The PR's effective diff is git diff HEAD^1..HEAD (12 files, 156 insertions, 64 deletions); the verified head is HEAD^2 = 95a35e06.
Every harness is a .mjs/.sh file under this artifact directory and drives compiled or source-real code, never a stub of the unit under test. The pnpm A/B runs the repository's own scripts/setup-worktree.js verbatim — the exact command the smoke workflow runs — in a scratch git worktree at HEAD whose two varying files are supplied from git show HEAD^1:… / git show HEAD:…, and asserts on exit status, pnpm's own error code and diagnosis text, the lockfile's sha256 before and after, and git status --porcelain. The central A/B spawns real node child processes against two exporter copies that differ by exactly one hunk (the changed-line set compared literally against the expected −5/+20) and asserts on exit code, stdout, stderr and the artifact left on disk, across symlinked files, a symlinked directory, a double symlink chain, hard links and relative invocations, plus an independent argv[1]-vs-cwd spelling probe. The parity harnesses read each package's live chromium.executablePath() and its resolved playwright-core/browsers.json as two independent instruments, and lift today's caret resolution from the registry (npm view) and npm pack playwright-core@1.63.0. The mutation work runs in scratch mirrors under mutate/scratch/ that copy runner.py in beside the module to reproduce the repository precondition. The repository tree was never modified.
The one working-tree drift, .qwen/review-context.json, is the container's own root-owned frozen base-content copy, re-measured and described under Not covered. The scratch worktree at pnpm/tree still holds a pnpm-installed node_modules from the head arm.
Raw per-cell output and every harness's log live in logs/ (ab-run.txt, pnpm-frozen-ab.txt, pnpm-online-{base,mid,head}.txt, pnpm-online-assert.txt, findings-f1f2.txt, findings-f3.txt, findings-f5.txt, gates.txt, typecheck.txt, plan-summary.txt); harnesses in ab/, pnpm/, parity/, mutate/; recorded cells in ab/cells.json, pnpm/cells.json, pnpm/online-*.json, parity/f1f2.json, mutate/f3.json, mutate/f5.json; rendered artifacts in ab/artifact-*.html; image evidence in evidence/.
Assertion counts in assertions.json (143 pass / 0 fail / 143 total) were summed from the six harnesses' own recorded pass/fail lines and count only checks that actually executed. fail is reserved for unexpected outcomes, so the base- and mid-arm cells that broke exactly as predicted (silent no-op, ERR_PNPM_OUTDATED_LOCKFILE) are recorded as passes. Four harness-expectation bugs found and fixed during this round are recorded rather than hidden: the offline pnpm discriminator was first asserted as an error code that a cold metadata mirror makes unreachable on every arm; a require used inside --input-type=module -e threw and a catch silently fell back to the hoisted playwright-core, reporting chromium 1208 where the live package says 1228; npm view <spec> version output was parsed with the wrong token; and playwright/cli.js is not in that package's exports map (ERR_PACKAGE_PATH_NOT_EXPORTED), so the require had to be re-anchored at playwright/package.json.
Evidence images
| file | what it witnesses |
|---|---|
01-pnpm-frozen-lockfile-three-arms.png |
the Delta A/B as printed: the shipped bootstrap's exit status per arm, pnpm's own one-line mismatch diagnosis, and the smoke job's cleanliness step |
02-ab-symlinked-invocation-base-vs-head.png |
the central-claim A/B: 8 invocation shapes × 2 arms, base 4/7 vs head 7/7, and the argv[1]-vs-cwd spelling probe that explains S3 vs S5 |
03-mutation-matrix-f3-and-sibling-sweep-f5.png |
the F3 mutation matrix with its same-file positive controls and vacuity row, and the F5 sibling sweep with its live divergence |
04-f1-f2-playwright-parity.png |
F1/F2 re-measured: the enforcement census, today's caret resolution against the pin, and both parity instruments at both sites |
05-reviewer-test-plan-and-gates.png |
every test-plan step's own exit code and counts, plus the gates the PR implicates beyond the plan |
Flakiness gate log
integration test, out of gate scope: integration-tests/chat-transcript-document.test.ts
integration test, out of gate scope: integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs
rounds=5 files=3 skipped=2
file .github/scripts/ci/classify-platform-sensitivity.test.mjs: (cd .) node --test ./.github/scripts/ci/classify-platform-sensitivity.test.mjs
file packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/manifest-repository-context.committed.test.ts
file packages/web-shell/client/components/messages/Markdown.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/Markdown.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
.github/scripts/ci/classify-platform-sensitivity.test.mjs: PPPPP
packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: PPPPP
packages/web-shell/client/components/messages/Markdown.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 2 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 3 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 4 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 4 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
round 5 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/manifest-repository-context.committed.test.ts: P (exit 0)
round 5 · packages/web-shell/client/components/messages/Markdown.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review — chore: restore regression coverage after WebUI retirement
Round 1 · head 95a35e06 · base 6ba04fc2
Scope
Reviewed all 12 changed files at head. Static analysis only — no local execution environment available (Playwright/Chromium browser gate, Node/Vitest suites not run). Cross-checked against all prior rounds from qwen-code-ci-bot.
Not reviewed / not run:
- Browser gate in
integration-tests/chat-transcript-document.test.ts(requires Playwright + built asset) - Windows behavior (not verified locally; CI covers Linux; macOS confirmed by author in PR body)
--preserve-symlinks-main/NODE_OPTIONSsweep across all npm scripts (CI workflow grep returned no matches; edge-case impact only)
Prior-round findings status
All confirmed blockers and suggestions from prior rounds (R1–D4) have been resolved at this head:
| ID | Summary | Status |
|---|---|---|
| R1-1 | toHaveClass incompatible with vitest expect |
Fixed — .poll().toBe('light') |
| R1-2 | Inert synthetic-unhandledrejection block |
Fixed — block deleted |
| R1-3 | isMain path-spelling guard fails on symlinks |
Fixed — isMainModule with realpathSync |
| R1-5 | Stop-reason enumeration named interrupted |
Fixed — cancelled, refusal, max_tokens, max_turn_requests |
| R1-6 | "unconditional" contradicts six early-return conditions | Fixed — wording removed |
| R1-8 | throw in assertRenderableJsonl unreachable from test |
Fixed — assertRenderableJsonl exported and called directly |
| R1-9 | Test factory hand-rolled synthetic ChatRecord shape | Fixed — factory removed; .mjs now covers only isMainModule |
| R1-10 | Stop-reason gate on negative side has no test | Deferred to #11159 — agreed split, not a blocker here |
| R1-11 | node:test .mjs file not routed to correct CI lane |
Fixed — .qwen/review-context.json narrower rule added |
| R2-1 | .toMatch(/light/) matches dark light |
Fixed — .toBe('light') |
| R3-1 | PR body test plan inconsistent with actual .mjs scope |
Fixed — PR body reworded |
| R3-2 | Duplicate persists under the literal historical key prefix block |
Fixed — block deleted |
| D4-6 | HELPER_TESTS hand-edited list lacked the new .mjs |
Fixed — .mjs added, count comment updated 25→26 |
New findings at this head
No new blockers. No new major findings.
Minor observations (body-only, no inline threads per round-1 policy for non-blockers already addressed):
-
shellCommandProcessor.tsin the PLATFORM_INSENSITIVE assertion: the file containsos.platform() === 'win32'and conditional path logic, but the classifier intentionally cannot see into source internals — its own comment documents this and names the nightly scheduled run as the backstop. The test addition correctly pins the existing classifier boundary (camelCase-embedded keyword is not a segment or stem-head match). No action needed here; the design trade-off is explicitly stated in the classifier. -
Playwright pin (
1.61.1exact vs. caret^1.57.0): the@playwright/test/node_modulesoverride that duplicated the 1.61.1 sub-tree is cleanly removed; rootplaywrightresolves to 1.61.1 for both the test runner and the root package. The long-term manifest-level parity enforcement is tracked in #11336 as noted in the PR.
Cross-file checks performed
isMainModule: verifiedrunner.pyexists inintegration-tests/concurrent-runner/— the third test case (non-main path) uses a real file and is sound.isSafeHref/isSafeImageSrc: confirmed at head thatSAFE_HREF_SCHEMES = /^(https?:|mailto:)/i—file:URIs are already rejected; the new tests pin correct existing behavior.- ACP
StopReason: confirmedend_turn,cancelled,max_tokensare used inSession.ts;refusalandmax_turn_requestsappear in the doc as valid non-end_turnsuppressors — consistent withresult.stopReason !== 'end_turn'guard at line 5090. .qwen/review-context.jsonnarrower rule: the manifest-repository-context.ts code merges all matching rules rather than first-match; the committed test snapshot was updated to match; both sides are consistent.- HELPER_TESTS count: old list had 25 entries; new list adds
integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs; comment updated to 26. Count verified by inspection.
Verdict
No blocking findings. No approval blockers.
Approving.
Reviewed with AI assistance.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head 95a35e0615.
The one thing that was blocking this — R5-1 — is resolved in the description at this head, and I checked the description rather than the claim about it. The Closes #11076 keyword is gone; the body now states plainly that "the four items originally tracked in #11076 were completed separately by #11107; this PR neither reimplements nor closes #11076", which is what the earlier rounds asked for twice. That also removes the merge-time hazard (a close keyword firing at an issue that has been closed since 2026-09-05) and the mis-scoped coverage claims, and the deferred non-blocking suggestions now have a home (#11336) instead of living only in review threads.
The code side holds up on its own terms. The earlier Critical about toHaveClass — a @playwright/test web-first matcher used under vitest's expect, which typechecked red and threw Invalid Chai property at runtime — is gone: no toHaveClass remains anywhere under integration-tests/, and the theme check is now await expect.poll(() => page.locator('html').getAttribute('class')).toBe('light') at chat-transcript-document.test.ts:474-476, sitting under a comment that explains why the attribute is exactly one of two values. The typecheck:integration command that CI runs inside the no-AK job is green there (24m38s / 12m39s on this head), so the author's note about errors "remaining locally, all in packages/**" is a local-tree artifact and not this lane.
The package.json → 1.61.1 Playwright pin is exact rather than a range, and the newest commit (95a35e0615) exists only to bring pnpm-lock.yaml in step with it — which is the failure mode #10449's dual-lockfile arrangement creates whenever a pin moves on one side, and it is enforced by the lockfile check that runs in the green Lint & Static lane.
I ran the new lane myself. node --test integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs at this head: 3 passed, 0 failed, including recognizes a main-module invocation through a symlinked path — the exact shape that motivated the isMainModule realpath fix, where a symlinked invocation used to exit successfully having written nothing. The wiring around it is consistent too: the file is appended to HELPER_TESTS in ci.yml:90, the "25-file" comment two lines below became 26, and .qwen/review-context.json gained a matching integration-tests/concurrent-runner/** → helper-tests/node22 route, with classify-platform-sensitivity.test.mjs picking up its one line.
One wording nit, not blocking. The comment at export-html-from-chatrecord-jsonl.test.mjs:15-17 justifies the second lane by saying the vitest suite "cannot exercise [the main-module check] without spawning". That reason is wrong in the direction that matters for future maintainers: the assertions call isMainModule(exporterPath, exporterUrl) with computed arguments, so no spawn is needed and they could live in the existing suite — the real reason for the file is its node:test form and the HELPER_TESTS battery. Say that instead the next time this file is touched, so nobody deletes a "duplicate" on the strength of the current comment.
CI facts. At this head Lint & Static 24m38s and Integration Tests (no-AK) 12m39s are green; Test (ubuntu-latest), review-pr, assign and delay-automatic-review were still queued or running when I submitted, and nothing has reported red. All 12 review threads are resolved, and chiga0's approval is on this head; the page's CHANGES_REQUESTED row is ci-bot's, anchored at the pre-description-fix commit, so a @qwen-code /triage re-run is what clears it.
|
Sandboxed verification: Skipped because the PR is not open for verification (state=MERGED, draft=false). 中文 — 判定：
|
Works the four items tracked in #11336: - Pin Web Shell's @playwright/test to the same exact version as the root playwright dependency, and fail `npm run check:lockfile` when the two direct manifests drift apart or their resolved root trees stop matching the pin. The check deliberately tolerates the packages/mobile-mcp tree: mobilewright depends on playwright and playwright-core at an exact older revision, so no manifest edit in this repository can dedupe it. - Correct the "Install evidence browser" rationale in qwen-triage.yml and its restatement in the workflow test. The two-trees premise no longer holds now that both manifests are pinned; the resolution behavior is unchanged, since resolving from the harness's own directory still keeps the binary tied to the package it imports. - Cover the ENOENT fallback of the exported isMainModule helper. The existing runner.py assertion takes the realpath success path because that file exists, so the fallback stayed unpinned. - Separate the CLI-only from the daemon-only followup suggestion gates in the documentation instead of pointing daemon clients at the blended CLI-oriented list, keeping the headless/SDK opt-out guidance.
|
Released in v0.23.1. |
4 participants
What this PR does
Restores focused regression coverage and supporting behavior left after the
@qwen-code/webuiretirement. It pins the platform-sensitivity substring boundary, rejects localfile:links in rendered Markdown, preserves transcript export when the helper is invoked through a symlink, strengthens the exported-document theme assertion, and routes the new helper test through CI. It also pins the root Playwright dependency to the currently matched browser revision and corrects follow-up-suggestion guidance for daemon clients.Why it's needed
These are delta-specific gaps found while auditing the WebUI retirement in #9812. In particular, a symlinked exporter invocation could exit successfully without writing output, several important boundaries lacked direct regression coverage, and headless daemon clients were not told how to avoid suggestion-generation cost they cannot use. The four items originally tracked in #11076 were completed separately by #11107; this PR neither reimplements nor closes #11076.
Reviewer Test Plan
How to verify
node --test .github/scripts/ci/classify-platform-sensitivity.test.mjsand confirm the shell-substring boundary passes.node --test integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjsand confirm direct, symlinked, and imported main-module cases pass.npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/export-html-from-chatrecord-jsonl.test.jsand confirm legacy JSONL rejection plus the ChatRecord happy path pass.packages/web-shell, runnpx vitest run client/components/messages/Markdown.test.tsand confirmfile:URLs remain rejected.npm run typecheck && npm run lint.npm ls playwright @playwright/testand confirm both resolve to1.61.1in the current lockfile. Long-term manifest-level parity enforcement is tracked in chore: follow up deferred #11101 review suggestions #11336.Evidence (Before & After)
N/A — documentation, tests, a helper bug fix, and a dependency pin; no UI change.
Tested on
Environment (optional)
Local Node 22 checkout; focused Node and Vitest suites were run directly in the worktree. The exported-document browser gate was also verified in the sandboxed PR run linked in the discussion.
Risk & Scope
playwrightto1.61.1trades caret flexibility for the browser-revision parity required by the current lockfile; future manifest-level parity enforcement is tracked in chore: follow up deferred #11101 review suggestions #11336.main.Linked Issues
Context: #9812, #11076, and #11107. Deferred non-blocking review suggestions are tracked in #11336.
中文说明
本 PR 做了什么
补回
@qwen-code/webui退役后仍缺失的聚焦回归覆盖及其必要行为：固定 platform-sensitivity 的子串边界，拒绝渲染 Markdown 中的本地file:链接，保证 transcript exporter 通过软链接调用时仍会执行，加强导出文档的主题断言，并让新增 helper 测试进入 CI。同时把根目录 Playwright 依赖固定到当前匹配的浏览器版本，并修正 daemon 客户端的 follow-up suggestion 使用说明。为什么需要
这些是审计 #9812 的 WebUI 退役工作时发现、且确实属于当前 delta 的缺口。此前通过软链接调用 exporter 时，进程可能成功退出却没有写出文件；若干关键边界缺少直接回归覆盖；headless daemon 客户端也没有被告知如何关闭无法消费却会产生费用的 suggestion generation。#11076 原先跟踪的四项内容已由 #11107 单独完成；本 PR 不重复实现，也不关闭 #11076。
Reviewer Test Plan
如何验证
node --test .github/scripts/ci/classify-platform-sensitivity.test.mjs，确认 shell 子串边界测试通过。node --test integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.test.mjs，确认直接调用、软链接调用和被 import 三种 main-module 场景通过。npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/export-html-from-chatrecord-jsonl.test.js，确认 legacy JSONL 拒绝路径和 ChatRecord 正常路径通过。packages/web-shell中运行npx vitest run client/components/messages/Markdown.test.ts，确认file:URL 仍被拒绝。npm run typecheck && npm run lint。npm ls playwright @playwright/test，确认当前 lockfile 中两者都解析为1.61.1。长期的 manifest 级版本一致性约束由 chore: follow up deferred #11101 review suggestions #11336 跟踪。证据（Before & After）
N/A —— 文档、测试、helper bug 修复和依赖固定；无 UI 变化。
测试环境
环境（可选）
本地 Node 22 checkout；聚焦的 Node 与 Vitest 测试直接在 worktree 中运行。导出文档的浏览器 gate 也已在 PR 讨论中链接的沙箱验证任务里通过。
风险与范围
playwright固定为1.61.1，以放弃 caret 灵活性换取当前 lockfile 所需的浏览器版本一致性；后续的 manifest 级一致性约束由 chore: follow up deferred #11101 review suggestions #11336 跟踪。main的纯合并同步由 CI 覆盖。关联问题
背景参考：#9812、#11076、#11107。延后的非阻塞 review 建议由 #11336 跟踪。