fix(web-shell): close the four deferred #9812 review follow-ups - #11107
Conversation
Each of the four suggestions deferred out of #9812 was left with nothing pinning it, so the behaviour each one protects could regress silently. Pin the historical sessionStorage key. `clientLifecycle.test.ts` only ever round-tripped through `SESSION_CLIENT_ID_STORAGE_PREFIX`, so renaming that constant moved the read and the write together and left every test green, while a tab that persisted its id under the WebUI-era key lost it across the migration. Three assertions now spell the key literally, including its percent-encoded session suffix. Make the ChatRecord export script testable and cover both paths. It had no exports and ran `main()` at import, which is why the legacy-JSONL rejection — a behaviour flip #9812 introduced — had no test. The input gate is now `selectChatRecords`, the render is `renderHtmlFromObjects(objects, api)` with the export API passed in, and `main()` runs only as the process entry point. The CLI behaves as before. The new suite asserts the rejection message verbatim and that the renderer is never reached, so removing the throw fails rather than falling through to the neighbouring error. Restore the substring-trap fixture the retirement deleted, under a live path. `Shellfish.tsx` guards the keyword being the head of a stem, which is a different loosening from the `web-shell` compound case already covered. Replace the deleted `InputForm` in the public hook's docblock with `ChatEditor`, which declares the three props, naming `ChatPane` and `App` as the in-tree hosts. Verification brief for the two vitest-backed items is committed alongside; item 3 was run here, mutation included, and the mutant survives without the new fixture. Refs #11076, #9812.
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR! Template looks good ✓ — all required headings present, and the Chinese section is a full paragraph-by-paragraph translation rather than a summary. Problem: observed, not theoretical. These are the four non-blocking suggestions deferred out of #9812 ( Direction: aligned. #9812 was deliberately converged by deferring these, and landing them now is the follow-through that makes the deferral honest rather than a way to drop them. Nothing here touches a runtime surface — it is tests, one docblock, and the minimum refactor needed to make a previously untestable script importable. No auth/sandbox/model-selection/telemetry/release surface, so no direction escalation. Size: not applicable — no core paths are touched. Approach: the scope is right — exactly the four tracked items and nothing else, no drive-by edits. I have one genuine question rather than an objection, about the 117-line brief at Risk: no elevated risk signals — the Stage 1e revert-correlated path scan matched nothing here. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需标题齐全，中文部分是逐段完整翻译，而非摘要。 问题：已观测，非理论性。这四条是 #9812（ 方向：对齐。#9812 是通过延后这些建议来刻意收敛的，现在把它们落地，才让"延后"是诚实的跟进，而不是变相丢弃。本次改动不涉及任何运行时面——只有测试、一处文档注释，以及让一个此前不可测脚本变得可 import 所需的最小重构。不触及 auth/sandbox/模型选择/telemetry/发布面，因此无需方向升级。 规模：不适用——未触及核心路径。 方案：范围合理——恰好是跟踪的四项，没有别的，也没有夹带顺手改动。我有一个真正的疑问（不是反对），关于 风险：无升级风险信号——Stage 1e 的 revert 相关路径扫描在此没有命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo blocking issues, and no AGENTS.md violations. I checked the load-bearing claims rather than taking the description's word for them, because three of the four items are assertions about what a test will do under a mutation — which is the kind of claim that reads well and can still be wrong. Item 1 — sessionStorage key. Item 2 — export script. The refactor is behaviour-preserving: same order of operations, same three error messages, same arguments to The new suite's assertions are accurate against the implementation, including the two that are easy to get subtly wrong and would have made the test vacuous: Item 3 — classifier fixture. The mutation claim holds, and it turns on a detail worth stating because the fixture would be worthless without it: both Item 4 — docblock. One non-blocking observation. The entry guard fails silently. If One limit on my own verification. I could not check the PR's historical claim (that the retirement deleted the old CI test evidenceFetched once from the check-runs API at the reviewed commit; not polled, per the no-waiting rule. No check has failed. But the honest headline is that the evidence this PR most needs has not arrived: Worth noting for the same reason: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查无阻塞问题，也没有违反 AGENTS.md。我没有采信 PR 描述，而是逐条核对了关键论断——因为四项中有三项是"测试在某种变异下会如何表现"的断言，这类说法读起来很顺，但完全可能是错的。 第 1 项（sessionStorage 键）： 第 2 项（导出脚本）： 重构保持行为不变——操作顺序、三条错误文案、传给 新测试的断言与实现相符，包括两处最容易写错、一旦写错就会让测试变成空壳的地方： 第 3 项（分类器夹具）： 变异论断成立，且取决于一个必须点明的细节——没有它这个夹具毫无价值： 第 4 项（文档注释）： 一条非阻塞观察： 入口守卫的失败方式是静默的。如果某次真实调用中 我自身验证的一处局限： PR 的历史性论断（退役操作删除了旧的 CI 测试证据按"不轮询"规则，只在被审 commit 上一次性从 check-runs API 取得。没有任何检查失败。 但诚实的结论是：本 PR 最需要的证据尚未到达—— 同一原因值得注意： （上方表格为真实检查名与结论，由 finalize 任务在 CI 结束后就地更新。） 沙箱验证可以定这件事： — 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 |
|
Confidence: 4/5 — every claim I could check independently held up, including the one I expected to break; two non-blocking notes, and CI on the two suites that matter has not landed yet. Going back to what I said I would do before reading the diff: my independent proposal was essentially this PR. Pin the storage key with a literal, extract an exported input gate plus an entry guard so the script becomes importable, restore the fixture under a live path, correct the docblock. The one place the PR is better than what I sketched is item 2 — the review thread offered "run against a built CLI package or stub that import", and instead of mocking What makes me comfortable is that I tried to break the load-bearing claim and could not. The item-3 mutation story depends on The two notes I would not merge over, but would say out loud: The entry guard fails silently. A false And the question from Stage 1 still stands on the One thing about provenance, because it is the honest reading rather than the flattering one: the four items originate in Not approving in this run: 中文说明Confidence: 4/5 —— 我能独立核对的论断全部成立，包括我原以为会被推翻的那一条；有两条非阻塞意见，且关键的两个套件 CI 尚未出结果。 回到我在读 diff 之前写下的独立方案：基本就是这个 PR。用字面量固定存储键、抽出一个导出的输入闸门加入口守卫使脚本可被 import、在存活路径下补回夹具、修正文档注释。第 2 项上 PR 比我的设想更好——review thread 给的是"对构建后的 CLI 包运行，或打桩该 import"，而作者没有去 mock 让我放心的是：我尝试推翻最关键的论断，没能成功。第 3 项的变异说法取决于 两条我不会因此拒绝合并、但要说出口的意见： 入口守卫的失败是静默的。 Stage 1 提出的关于 关于来源，说一个诚实而非好听的读法：这四项源自本项目自己的 bot 发出的 本次不批准： — 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. Suggestions are inline.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and the new scripts suite was run on Linux only, so its non-Linux legs are unexercised.
Not reviewed: reverse audit — stopped before round 7 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and the new scripts suite was run on Linux only, so its non-Linux legs are unexercised.
未审查：反向审计——评审时间预算不足，未能开始第 7 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const invokedDirectly = | ||
| typeof process.argv[1] === 'string' && | ||
| path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); |
There was a problem hiding this comment.
[Suggestion] R1-1: This guard compares two different spellings of one path. path.resolve(process.argv[1]) is purely lexical, while Node realpaths the ESM main entry before setting import.meta.url — so when the invocation path carries a symlinked component the two differ, invokedDirectly is false, main() never runs, and the process exits 0 with empty stdout and stderr having written nothing. The same invocation works at the merge base, where main() ran unconditionally, so this is a regression rather than new surface. The only in-tree caller amplifies it: runner.py:332 builds its argv from Path(__file__).parent / <script>, and Python preserves a symlinked spelling instead of realpath-ing it, so a checkout reached through any symlinked path component (macOS /tmp → /private/tmp, a symlinked home or workspace) reaches the guard as a divergent spelling — and runner.py:340-341 branches solely on returncode == 0, printing Rendered chat HTML saved: <name> for a file that was never written, with no warning line to fall back on. Nothing on an automated lane witnesses the guard's true branch either: no workflow mentions runner.py, and the new suite only imports the module, so the divergence is invisible to CI in both directions.
Witness:
PR arm node <real path>/export-html-from-chatrecord-jsonl.js in.jsonl --out a.html
-> "Wrote HTML export to: .../a.html" exit=0 a.html exists? YES
PR arm node /tmp/verify-r11-U7HJ/link.js in.jsonl --out b.html (symlink -> same file)
-> (no stdout, no stderr) exit=0 b.html exists? NO
BASE arm merge base f7479995, same symlink invocation
-> "Wrote HTML export to: .../base.html" exit=0 base.html exists? YES
(base file has no invokedDirectly guard; its line 166 is `main().catch((error) => {`)
control cd <symlinked dir> && node export-html-...js in.jsonl --out rel.html
-> HTML written, exit=0 (process.cwd() is already physical, so argv[1] must carry the symlink)
python3 runner.py reached through a symlinked dir -> Path(__file__).parent keeps the link spelling
fix realpath on the argv side, applied in a scratch tree
-> symlink arm writes the HTML; import purity held; dependent suite 10 passed (10)
| const invokedDirectly = | |
| typeof process.argv[1] === 'string' && | |
| path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); | |
| const invokedDirectly = (() => { | |
| if (typeof process.argv[1] !== 'string') return false; | |
| try { | |
| // Node realpaths the ESM main entry, so both sides must be realpath'd. | |
| return ( | |
| fs.realpathSync(process.argv[1]) === | |
| fs.realpathSync(fileURLToPath(import.meta.url)) | |
| ); | |
| } catch { | |
| return false; | |
| } | |
| })(); |
The guard must still evaluate false when the module is imported: the comment at :187-188 states that contract, scripts/tests/export-html-from-chatrecord-jsonl.test.js:18 depends on it, and loadExportApi()'s ERR_MODULE_NOT_FOUND branch calls process.exit(1) at :22 — so the realpath call must not throw at import time, which is why the try/catch is part of the fix rather than optional (realpathSync raises ENOENT when argv[1] names no real file, as with node -e). Note also that dropping or rewriting path.resolve alone changes nothing observable: Node absolutizes argv[1] itself while keeping the symlink spelling, so the asymmetry is realpath-versus-not, not absolute-versus-relative.
The test that must pin this is a spawn case in scripts/tests/export-html-from-chatrecord-jsonl.test.js that symlinks the script into a temp dir and asserts node <symlink> is not a silent exit-0 no-op (non-empty stderr, or a non-zero exit) — please revert the realpath comparison afterwards and confirm that case goes red, since nothing spawns this script on any lane today.
中文说明
这个入口守卫比较的是同一个路径的两种拼写。path.resolve(process.argv[1]) 纯粹是词法解析，而 Node 在设置 import.meta.url 之前会对 ESM 主入口做 realpath——因此当调用路径中含有符号链接成分时两者不相等，invokedDirectly 为 false，main() 永不执行，进程在什么都没写的情况下以 0 退出，且 stdout 与 stderr 均为空。同样的调用在 merge base 上是正常的（那时 main() 无条件执行），所以这是本次改动引入的回归，而非新表面。
仓库内唯一的调用方放大了这个问题：runner.py:332 用 Path(__file__).parent / <script> 构造 argv，而 Python 会保留符号链接拼写、不做 realpath，因此任何经由符号链接路径成分到达的检出（macOS 的 /tmp → /private/tmp、被软链的 home 或工作区）都会以不一致的拼写抵达该守卫；随后 runner.py:340-341 仅凭 returncode == 0 就打印 Rendered chat HTML saved: <name>，为一个从未写出的文件报告成功，且没有任何 warning 可兜底。同时，没有任何自动化通道见证该守卫的真分支：没有 workflow 提到 runner.py，新增测试也只是 import 该模块，所以这个偏差在两个方向上对 CI 都是不可见的。
修复后守卫仍必须在模块被 import 时为 false：:187-188 的注释声明了这一约定，scripts/tests/export-html-from-chatrecord-jsonl.test.js:18 依赖它，而 loadExportApi() 的 ERR_MODULE_NOT_FOUND 分支会在 :22 调用 process.exit(1)——所以 realpath 调用不能在 import 期抛错，这正是 try/catch 属于修复本体而非可选项的原因（当 argv[1] 不指向真实文件时 realpathSync 会抛 ENOENT，例如 node -e）。另外请注意：仅去掉或改写 path.resolve 不会带来任何可观测变化，因为 Node 自己会把 argv[1] 绝对化但保留符号链接拼写——真正的不对称在于是否 realpath，而非是否绝对化。
用于钉住这一点的测试，应当是 scripts/tests/export-html-from-chatrecord-jsonl.test.js 中一个 spawn 用例：在临时目录里为该脚本创建符号链接，断言 node <symlink> 不是静默的 exit-0 空操作（stderr 非空，或退出码非零）。修好之后请把 realpath 比较改回去，确认该用例变红——因为目前没有任何通道会 spawn 这个脚本。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| it('requires every field the renderer reads', () => { | ||
| expect(looksLikeChatRecord(chatRecord())).toBe(true); | ||
| expect(looksLikeChatRecord(null)).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R1-2-1: This test is named requires every field the renderer reads, but it discriminates only 3 of looksLikeChatRecord's 9 conjuncts (value !== null, 'parentUuid' in value, typeof value.sessionId === 'string'). The uuid, timestamp, type, cwd and version checks — and the outer typeof value === 'object' — can each be deleted with the whole suite staying green, because every other input in the file supplies a complete chatRecord(). The cost is concrete: a JSONL line missing cwd, or carrying a numeric timestamp, is then admitted past the gate this PR newly extracts and handed to the export API's document allowlist, which is exactly the boundary selectChatRecords' own JSDoc says only well-formed source ChatRecords may cross. Its sibling block at :146-148 has the same shape and is reported alongside this one.
Witness:
looksLikeChatRecord (src :81-89), each conjunct deleted in turn, suite re-run:
:81 value !== null killed :86 timestamp SURVIVES (10 passed)
:82 typeof value === 'object' SURVIVES :87 type SURVIVES (10 passed)
:83 uuid SURVIVES :88 cwd SURVIVES (10 passed)
:84 'parentUuid' in value killed :89 version SURVIVES (10 passed)
:85 sessionId killed -> 6 of 9 survive
patched per-conjunct sweep -> intact 10 passed (10); all 7 killable conjuncts killed
it('requires every field the renderer reads', () => {
expect(looksLikeChatRecord(chatRecord())).toBe(true);
expect(looksLikeChatRecord(null)).toBe(false);
// parentUuid may be null, but the key has to be present.
const { parentUuid: _dropped, ...withoutParent } = chatRecord();
expect(looksLikeChatRecord(withoutParent)).toBe(false);
expect(looksLikeChatRecord(chatRecord({ parentUuid: null }))).toBe(true);
for (const field of ['uuid', 'sessionId', 'timestamp', 'type', 'cwd', 'version']) {
expect(looksLikeChatRecord(chatRecord({ [field]: 7 })), field).toBe(false);
}
});'parentUuid' in value at src :84 is a presence check rather than a type check, so parentUuid must stay out of the type loop and chatRecord({ parentUuid: null }) must stay true — otherwise the fix silently converts a presence contract into a type one.
The test that must pin this is this same block: with the loop added, removing any one of the six typeof ... === 'string' checks from looksLikeChatRecord has to turn it red and name the field — please delete one and confirm.
中文说明
这个测试名为 requires every field the renderer reads，但它只区分了 looksLikeChatRecord 九个合取项中的三个（value !== null、'parentUuid' in value、typeof value.sessionId === 'string'）。uuid、timestamp、type、cwd、version 这几个检查，以及外层的 typeof value === 'object'，每一个都可以被删掉而整套测试依然全绿——因为文件中其他所有输入都提供了完整的 chatRecord()。代价是具体的：缺少 cwd 的行、或 timestamp 为数字的行，就会越过本 PR 新抽取出的这道闸门，被交给导出 API 的文档白名单，而这正是 selectChatRecords 自己的 JSDoc 所说「只有格式正确的源 ChatRecord 才能通过」的那道边界。位于 :146-148 的姊妹代码块形态相同，与本条一并报告。
约束：src :84 的 'parentUuid' in value 是存在性检查而非类型检查，所以 parentUuid 必须留在类型循环之外，且 chatRecord({ parentUuid: null }) 必须仍为 true——否则这个修复会把「存在性约定」悄悄变成「类型约定」。
用于钉住这一点的测试就是本代码块自身：加上循环之后，从 looksLikeChatRecord 中删掉六个 typeof ... === 'string' 检查里的任意一个，都必须让它变红并指出是哪个字段——请删掉一个确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| describe('looksLikeExportJsonl', () => { | ||
| it('matches only the legacy metadata header shape', () => { | ||
| expect(looksLikeExportJsonl([legacyMetadata()])).toBe(true); |
There was a problem hiding this comment.
[Suggestion] R1-2-2: Same pattern as the looksLikeChatRecord block below, and this half has a user-visible consequence. The block probes 2 of looksLikeExportJsonl's 5 conjuncts (only type and startTime vary), so first !== null at src :96, typeof first === 'object' at :97 and typeof first.sessionId === 'string' at :99 can each be deleted with the suite green. Deleting the first !== null guard is not just a lost type check: it turns a domain error into a crash for a JSONL file whose first line is the literal null, which readJsonlObjects happily pushes because JSON.parse('null') yields null and the blank-line skip does not drop it — so the diagnostic the CLI prints changes.
Witness:
looksLikeExportJsonl (src :96-100), each conjunct deleted in turn:
:96 first !== null SURVIVES (10 passed)
:97 typeof first === 'object' SURVIVES (10 passed)
:98 first.type === 'session_metadata' killed
:99 typeof first.sessionId === 'string' SURVIVES (10 passed)
:100 typeof first.startTime === 'string' killed
src :96 deleted, real CLI on a file whose only line is `null`:
intact -> CLI stderr "Unrecognized JSONL format (expected ChatRecord-per-line)."
mutant -> CLI stderr "Cannot read properties of null (reading 'type')"
patched sweep -> intact 11 passed (11); :96 and :99 mutants killed; :97 still survives
it('requires every field of the legacy header, and survives a null line', () => {
expect(looksLikeExportJsonl([legacyMetadata()])).toBe(true);
expect(looksLikeExportJsonl([chatRecord()])).toBe(false);
expect(looksLikeExportJsonl([legacyMetadata({ type: 'user' })])).toBe(false);
expect(looksLikeExportJsonl([legacyMetadata({ sessionId: 7 })])).toBe(false);
expect(looksLikeExportJsonl([legacyMetadata({ startTime: undefined })])).toBe(false);
expect(looksLikeExportJsonl([null])).toBe(false);
// The null guard is what keeps a `null` first line a domain error, not a TypeError.
expect(() => selectChatRecords([null])).toThrow(
'Unrecognized JSONL format (expected ChatRecord-per-line).',
);
});Two constraints on the shape of that fix. selectChatRecords is synchronous (export function selectChatRecords(objects) at src :143) while renderHtmlFromObjects is async (src :165), so the [null] case must be expect(() => ...).toThrow(...) and not rejects — a rejects on a non-promise fails for the wrong reason. And looksLikeExportJsonl(objects) reads objects[0] (src :93-94), so every per-field variant has to stay wrapped in a one-element array; passing the bare object would make objects[0] the first property value and turn the sweep vacuous. legacyMetadata(overrides = {}) at :36 already accepts overrides, so this compiles as written.
One bound on the claim, so the fix is not over-sold: src :97's typeof first === 'object' is not killable by any input JSON.parse can produce, because for a primitive first.type boxes to undefined and fails the 'session_metadata' comparison anyway. The sweep above therefore pins 2 of this predicate's 3 survivors, not all 5 conjuncts.
The tests that must pin this are the two added assertions: deleting src :96 or src :99 has to turn this block red — please delete each in turn and confirm.
中文说明
与下方 looksLikeChatRecord 代码块是同一类问题，而这一半带有用户可见的后果。该块只探测了 looksLikeExportJsonl 五个合取项中的两个（只有 type 与 startTime 变化），因此 src :96 的 first !== null、:97 的 typeof first === 'object' 和 :99 的 typeof first.sessionId === 'string' 每一个都可以被删掉而测试全绿。删掉 first !== null 守卫不只是少了一个类型检查：对于首行是字面量 null 的 JSONL 文件，它会把一个领域错误变成崩溃——readJsonlObjects 会照常推入这个值，因为 JSON.parse('null') 得到 null，而空行跳过逻辑不会丢弃它——于是 CLI 打印的诊断信息变了。
修复形态有两条约束。selectChatRecords 是同步的（src :143 的 export function selectChatRecords(objects)），而 renderHtmlFromObjects 是 async（src :165），所以 [null] 用例必须写成 expect(() => ...).toThrow(...) 而不是 rejects——对非 Promise 使用 rejects 会以错误的理由失败。另外 looksLikeExportJsonl(objects) 读取的是 objects[0]（src :93-94），所以每个按字段变形的用例都必须保持包在单元素数组里；直接传对象会让 objects[0] 变成第一个属性值，使整个扫描失效。:36 的 legacyMetadata(overrides = {}) 已接受覆盖参数，因此上面的代码可以直接编译。
对结论的一个边界，以免修复被说过头：src :97 的 typeof first === 'object' 无法被任何 JSON.parse 能产生的输入杀死，因为对原始值而言 first.type 会装箱为 undefined，本来就无法通过 'session_metadata' 比较。因此上面的扫描钉住的是该谓词三个存活项中的两个，而不是全部五个合取项。
用于钉住这一点的测试就是新增的那两条断言：删掉 src :96 或 src :99 都必须让本块变红——请逐个删除并确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * `ChatEditor` declares exactly these three props, typed from this | ||
| * interface; `ChatPane` and `App` are the two in-tree hosts that call | ||
| * this hook and thread them down. |
There was a problem hiding this comment.
[Suggestion] R1-4: This docblock ships on a publicly exported hook and tells consumers to render ChatEditor, but ChatEditor is not a value export reachable from either package entry, so step 1 of the Consumers: list cannot be followed from outside the repo. client/index.tsx:174 holds the only public reference to that module and it re-exports a different symbol as a type only; daemon-react-sdk.ts never mentions it. The text lands verbatim in the published declaration file, which is the API doc surface an integrator's editor shows on hover — so replacing the stale InputForm reference with an unreachable one trades a deleted component for an unimportable one.
Witness:
import('@qwen-code/web-shell')
total value exports: 55 | ChatEditor present? false | useDaemonFollowupSuggestion present? true
component-ish: StandaloneWebShell, WebShell, WebShellTranscript, WebShellWithProviders
import('@qwen-code/web-shell/daemon-react-sdk')
value exports: 47 | ChatEditor present? false | useDaemonFollowupSuggestion present? true
grep -c ChatEditor packages/web-shell/dist/index.js -> 0
import { ChatEditor } from '@qwen-code/web-shell';
-> SyntaxError: The requested module '@qwen-code/web-shell' does not provide an export named 'ChatEditor'
import('@qwen-code/web-shell/dist/index.js')
-> Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './dist/index.js' is not defined by "exports"
client/index.tsx:174 -> export type { ComposerToolbarAction } from './components/ChatEditor';
ships verbatim: dist/types/daemon/useDaemonFollowupSuggestion.d.ts:58-62
| * `ChatEditor` declares exactly these three props, typed from this | |
| * interface; `ChatPane` and `App` are the two in-tree hosts that call | |
| * this hook and thread them down. | |
| * `ChatEditor` accepts these three as optional props typed from this | |
| * interface (`ChatEditor.tsx:271-273`); it is internal to the package and | |
| * is not part of its public export surface. `ChatPane` and `App` are the | |
| * two in-tree hosts that call this hook and thread them down. |
The constraint on the rewording is packages/web-shell/client/index.tsx:174 — export type { ComposerToolbarAction } from './components/ChatEditor'; is the only public reference to that module, and package.json declares exactly two export entries (. and ./daemon-react-sdk), so the replacement text must not imply a value export exists. If integrators are meant to compose it, the alternative fix is to export the component; the in-tree half of the sentence is accurate either way (ChatEditor.tsx:271-273 really does declare those three optional props typed from this interface, and ChatPane.tsx:518 / App.tsx:7020 really are the only two in-tree call sites).
中文说明
这段文档注释挂在一个公开导出的 hook 上，并指示使用方渲染 ChatEditor，但 ChatEditor 并不是从包的任一入口可达的值导出，因此仓库外部无法照做 Consumers: 列表的第 1 步。client/index.tsx:174 是该模块唯一的公开引用，而它只是以「仅类型」的方式再导出了另一个符号；daemon-react-sdk.ts 则完全没有提到它。这段文字会原样进入已发布的声明文件，也就是集成方编辑器悬停时看到的 API 文档面——所以把过期的 InputForm 引用换成一个不可达的引用，等于把「已删除的组件」换成了「无法 import 的组件」。
改写的约束是 packages/web-shell/client/index.tsx:174——export type { ComposerToolbarAction } from './components/ChatEditor'; 是该模块唯一的公开引用，且 package.json 只声明了两个导出入口（. 与 ./daemon-react-sdk），因此替换文案不得暗示存在值导出。如果确实希望集成方来组合它，另一种修复是把该组件导出。无论哪种，句子的仓库内部分都是准确的：ChatEditor.tsx:271-273 确实声明了这三个来自本接口的可选 props，ChatPane.tsx:518 与 App.tsx:7020 也确实是该 hook 仅有的两个仓库内调用点。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| the whole point, and the review that asked for this measured the same thing | ||
| (15/15 green under the mutant before, 15 pass / 1 fail after). |
There was a problem hiding this comment.
[Suggestion] R1-5-1: The predicted mutation result here is not what the suite produces, and it contradicts this same paragraph three lines above. The post-change file holds 18 tests (15 before this PR, 3 added) and all three added tests spell the literal prefix — including percent-encodes the session id in the key, which :36-38 correctly says "goes red too". So the rename mutant yields 15 pass / 3 fail, never 15 pass / 1 fail. This matters because the brief's own report-back instruction at :112-114 tells whoever executes the handoff that a mutation not going red as described "is the important result — say so": an executor who obeys it sees three reds against a documented expectation of one and must report a contradiction the document manufactured, or discount the one artifact whose stated purpose is to be trusted over static reasoning.
Witness:
mutant: clientLifecycle.ts:8 'qwen-code-webui-client-id:session:'
-> 'qwen-code-web-shell-client-id:session:'
BASE arm (HEAD~1 test file, 15 its) -> Tests 15 passed (15)
PR arm (18 its) -> Tests 3 failed | 15 passed (18)
x persistStableClientId > writes under the historical WebUI key
x persistStableClientId > reads an id a WebUI-era tab left under the historical key
x persistStableClientId > percent-encodes the session id in the key
PR intact -> Tests 18 passed (18)
| the whole point, and the review that asked for this measured the same thing | |
| (15/15 green under the mutant before, 15 pass / 1 fail after). | |
| the whole point, and the review that asked for this measured the same thing | |
| (15/15 green under the mutant before, 15 pass / 3 fail after). |
The corrected count is what an executor compares their own run against, so it has to match what the suite actually prints — 3 failed | 15 passed (18) — rather than the number of tests the paragraph happens to name.
中文说明
此处预测的变异结果并不是测试套件实际产生的结果，而且它与同一段落上方三行的内容自相矛盾。改动后的文件共有 18 个测试（本 PR 之前 15 个，新增 3 个），而三个新增测试都以字面量拼出了该前缀——包括 percent-encodes the session id in the key，:36-38 也正确写了它「同样会变红」。所以重命名变异体产生的是 15 通过 / 3 失败，绝不会是 15 通过 / 1 失败。这一点之所以重要，是因为本说明自己在 :112-114 的回报要求里写着：变异若未按描述变红，「那就是重要结果——请说明」。照此执行的验证者会看到三个红灯、而文档预期只有一个，于是要么报告一个由文档自己制造的矛盾，要么放弃这份「本应比静态推理更可信」的材料。
修正后的数字是执行者用来比自己运行结果的基准，因此它必须与套件实际输出的内容一致——3 failed | 15 passed (18)——而不是段落里恰好点到的测试数量。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| exported JSONL with the exact message` and `never reaches the renderer for | ||
| legacy exported JSONL` go red. Note the failure is _not_ an absence of an |
There was a problem hiding this comment.
[Suggestion] R1-5-2: Same pattern as item 1's count, in the other prescribed mutation. Deleting the throw in selectChatRecords's looksLikeExportJsonl branch reddens three tests, not the two named here — and the omitted third one fails for precisely the reason the next sentence rules out. rejects on the first line alone, even with real records behind it (test :83-86) feeds [legacyMetadata(), chatRecord()], so with the throw gone objects.filter(looksLikeChatRecord) keeps the trailing record, records.length !== 0, and selectChatRecords returns normally: that failure is an absence of an error. An executor following the report-back instruction at :112-114 therefore sees an unexpected third red whose failure mode the brief declared impossible, and may conclude that test over-reaches and weaken it — it is the only assertion pinning that a legacy header is refused when real records follow it. To be fair to the note: it is accurate for the two tests it names (both failed on message mismatch, not absence) and its scope word is "a legacy-only input", so the fix is to name three reds and scope the sentence.
Witness:
mutant: delete the throw in selectChatRecords' looksLikeExportJsonl branch (src :146-150)
PR arm -> Tests 3 failed | 7 passed (10)
x rejects legacy exported JSONL with the exact message
expected ... to throw error including 'Legacy exported JSONL cannot be rende...'
but got 'Unrecognized JSONL format (expected C...' <- as documented
x never reaches the renderer for legacy exported JSONL <- as documented, same mismatch
x rejects on the first line alone, even with real records behind it
AssertionError: expected [Function] to throw an error
- Expected: null + Received: undefined <- UNDOCUMENTED, and IS an absence of error
intact -> Tests 10 passed (10)
`selectChatRecords`'s `looksLikeExportJsonl` branch. Expected: `rejects legacy
exported JSONL with the exact message`, `rejects on the first line alone, even
with real records behind it`, and `never reaches the renderer for legacy
exported JSONL` go red (7 pass / 3 fail). For the two legacy-only inputs the
failure is _not_ an absence of an error: with the throw gone they fall through
to the filter and raise `Unrecognized JSONL format` instead. The mixed input is
the exception — the filter keeps the trailing record, so `selectChatRecords`
returns normally and that one fails on a missing error.The third title has to be spelled exactly as the suite spells it (rejects on the first line alone, even with real records behind it, test :83) because an executor greps the run output for these names; and the trailing claim that the test "must be asserting the exact message — it is" does not hold either, which is reported separately against the test file itself.
中文说明
与第 1 项的计数是同一类问题，出现在另一个指定的变异上。删掉 selectChatRecords 中 looksLikeExportJsonl 分支的 throw 会让三个测试变红，而不是此处点名的两个——而被漏掉的第三个，其失败原因恰恰是下一句话所排除的那种。rejects on the first line alone, even with real records behind it（测试 :83-86）的输入是 [legacyMetadata(), chatRecord()]，所以 throw 消失后 objects.filter(looksLikeChatRecord) 会保留后面那条真实记录，records.length !== 0，selectChatRecords 正常返回：这个失败就是「没有抛出错误」。因此，按 :112-114 的回报要求执行的验证者会看到一个意料之外的第三个红灯，而它的失败模式正是本说明宣称不可能出现的那种，于是可能判断该测试过度约束并去削弱它——而它是唯一钉住「后面还有真实记录时，遗留头部仍须被拒绝」的断言。为这句说明说句公道话：对它点名的两个测试它是准确的（两者都因文案不匹配而失败，而非缺少错误），且它的限定词是「仅含遗留数据的输入」，所以修复方式是点名三个红灯并把这句话限定范围。
第三个测试标题必须与套件中的写法完全一致（rejects on the first line alone, even with real records behind it，测试 :83），因为执行者会在运行输出中按这些名字检索；此外，末尾那句「测试必须断言精确文案——它确实如此」同样不成立，这一点已针对测试文件本身另行报告。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| }); | ||
|
|
||
| describe('selectChatRecords', () => { | ||
| it('rejects legacy exported JSONL with the exact message', () => { |
There was a problem hiding this comment.
[Suggestion] R1-6: This test is named rejects legacy exported JSONL with the exact message, but vitest's toThrow(string) is a substring check rather than an equality check, so the message is not pinned exactly — and the verification brief reasons from the same premise at README.md:77-78 ("so the test must be asserting the exact message — it is — or the mutant survives"). Any edit that keeps the rejection text as a substring, such as prepending Invalid input: or appending a remediation hint, leaves the suite green while the user-visible CLI message drifts. That message is a real surface: runner.py:343 prints it as Warning: HTML exporter failed: {stderr}. Worth being precise about what this does and does not affect — the brief's own named mutant still dies, because the fall-through raises Unrecognized JSONL format (expected ChatRecord-per-line)., which does not contain the legacy substring; what fails is the exactness claim, not the mutation coverage.
Witness:
vitest pinned ^3.2.4 (root package.json:169), resolved RUN v3.2.7
probe suite run under scripts/tests/vitest.config.ts:
PROBE-A message prefixed + toThrow(string) -> passed: true
PROBE-B message suffixed + toThrow(string) -> passed: true
PROBE-C message drifted + toThrow(string) -> passed: false (control: the probe is not vacuous)
PROBE-D message prefixed + toThrow(new Error) -> passed: false
PROBE-E message exact + toThrow(new Error) -> passed: true
vitest's own failure wording: "expected [Function] to throw error including 'Legacy exported JSONL cannot be rende...'"
the brief's named mutant under the substring form -> Tests 3 failed | 7 passed (10) (still killed)
expect(() => selectChatRecords([legacyMetadata()])).toThrow(
new Error(LEGACY_REJECTION),
);Passing an Error makes vitest compare by message equality (measured: PROBE-D rejects the prefixed variant while PROBE-E accepts the exact one). The brief's named mutant must still die after the tightening — it does, because deleting the throw makes the code fall through to Unrecognized JSONL format (expected ChatRecord-per-line). at src :151-157. LEGACY_REJECTION is declared at :20-21 and is used by three assertions, so changing its value is not the fix. If exactness is wanted on the render path too, the same change applies to the renderHtmlFromObjects rejection below.
The test that must pin this is this same assertion: with new Error(LEGACY_REJECTION), a mutant that prepends or appends a clause to the thrown message has to turn it red — please apply one and confirm.
中文说明
这个测试名为 rejects legacy exported JSONL with the exact message，但 vitest 的 toThrow(string) 是子串匹配而非相等匹配，所以文案并没有被精确钉住——而验证说明在 README.md:77-78 也基于同一前提推理（「所以测试必须断言精确文案——它确实如此——否则变异体会存活」）。任何保留该拒绝文案作为子串的改动，例如在前面加上 Invalid input: 或在后面追加补救提示，都会让套件保持全绿，而用户可见的 CLI 文案已经漂移。这个文案是真实的对外表面：runner.py:343 会以 Warning: HTML exporter failed: {stderr} 打印它。需要说清楚它影响与不影响的范围——说明中自己点名的那个变异体仍会被杀死，因为回退路径抛出的是 Unrecognized JSONL format (expected ChatRecord-per-line).，其中并不包含遗留文案的子串；失效的是「精确」这个论断，而不是变异覆盖。
传入一个 Error 会让 vitest 按消息相等来比较（实测：PROBE-D 拒绝了加前缀的变体，PROBE-E 接受了精确文案）。收紧之后，说明中点名的变异体必须仍会被杀死——它确实会，因为删掉 throw 会让代码回退到 src :151-157 的 Unrecognized JSONL format (expected ChatRecord-per-line).。LEGACY_REJECTION 声明于 :20-21 并被三处断言使用，所以修改它的值不是修复方式。如果也希望在渲染路径上做到精确，下面的 renderHtmlFromObjects 拒绝断言可作同样修改。
用于钉住这一点的测试就是这条断言自身：改成 new Error(LEGACY_REJECTION) 之后，给抛出的消息加前缀或后缀的变异体必须让它变红——请施加一个并确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const [sessionData, passedRecords] = api.toHtml.mock.calls[0]; | ||
| expect(passedRecords).toEqual(records); |
There was a problem hiding this comment.
[Suggestion] R1-10: This happy-path test feeds renderHtmlFromObjects an input in which every element is already a valid ChatRecord, so records (objects.filter(looksLikeChatRecord)) is structurally identical to objects and neither assertion below can tell them apart. The filtering step at the render boundary is therefore unpinned: passing the unfiltered array to the renderer instead of the filtered one leaves all 10 tests green. The regression that would ship is the one the module's own JSDoc at src :136-142 says the filtering exists to prevent — an unfiltered entry reaching the page, including a legacy session_metadata header that is not in first position and so never trips looksLikeExportJsonl, putting already-rendered markup back through a path the export API's document allowlist never sees.
Witness:
INTACT baseline -> Tests 10 passed (10)
MUTANT A src:171 api.toHtml(sessionData, objects) -> Tests 10 passed (10) SURVIVES
MUTANT B src:168 buildProductSessionData(objects) -> Tests 10 passed (10) SURVIVES
mixed-input fix + MUTANT A -> FAIL: passedRecords + {"note": "not a record"}
mixed-input fix + MUTANT B -> FAIL: metadata messageCount 1 -> 2
mixed-input fix + INTACT -> PASS (the call resolved; it did not reject)
harm under MUTANT A, legacy header not in first position:
INTACT -> toHtml received: [{"uuid":"rec-1",...,"type":"user",...}]
MUTANT A -> toHtml received: [{"uuid":"rec-1",...},{"type":"session_metadata","sessionId":"sess-1",...}]
const api = stubExportApi();
const recA = chatRecord({ uuid: 'rec-1', timestamp: '2026-01-02T03:04:05.000Z' });
const recB = chatRecord({ uuid: 'rec-2', timestamp: '2026-01-02T02:00:00.000Z' });
const html = await renderHtmlFromObjects([recA, { note: 'not a record' }, recB], api);
expect(html).toBe('<html>rendered</html>');
expect(api.collectSessionMetadata).toHaveBeenCalledWith(
expect.objectContaining({
sessionId: 'sess-1',
startTime: '2026-01-02T02:00:00.000Z',
messages: [recA, recB],
}),
expect.anything(),
);
const [sessionData, passedRecords] = api.toHtml.mock.calls[0];
expect(passedRecords).toEqual([recA, recB]);selectChatRecords throws only when the filter empties the array (src :151-157), so a mixed array containing at least one real record returns normally — measured: the intact probe awaited without rejecting. That is what makes this a resolve-and-assert case rather than a rejects case, and it keeps the earliest-timestamp assertion meaningful because recB is still the earlier of the two.
The test that must pin this is renders the ChatRecord happy path through the export API with the mixed input: changing src :171 to api.toHtml(sessionData, objects) has to turn the passedRecords assertion red — please apply that mutant and confirm.
中文说明
这个正常路径测试喂给 renderHtmlFromObjects 的输入中，每个元素本身就已经是合法的 ChatRecord，因此 records（即 objects.filter(looksLikeChatRecord)）与 objects 在结构上完全相同，下面两条断言都无法区分二者。于是渲染边界上的过滤这一步没有被钉住：把未过滤的数组而不是过滤后的数组传给渲染器，10 个测试仍然全绿。会因此上线的回归，正是模块自己在 src :136-142 的 JSDoc 中所说、过滤存在的目的——未经过滤的条目进入页面，包括一个不在首行、因而永远不会触发 looksLikeExportJsonl 的遗留 session_metadata 头部，使已经渲染过的标记重新走上一条导出 API 文档白名单看不到的路径。
selectChatRecords 只有在过滤后数组为空时才抛错（src :151-157），所以只要混合数组中还有一条真实记录，它就会正常返回——实测：完整实现的探针是正常 await 完成、并未 reject。这正是本用例应写成「解析后断言」而非 rejects 的原因；同时它也让「取最早时间戳」的断言继续有效，因为 recB 仍是两者中更早的那个。
用于钉住这一点的测试，是使用混合输入的 renders the ChatRecord happy path through the export API：把 src :171 改成 api.toHtml(sessionData, objects) 必须让 passedRecords 断言变红——请施加该变异并确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // The suffix is part of the persisted shape too: a session id carrying `/` | ||
| // or `:` would otherwise collide with the prefix's own separator. | ||
| persistStableClientId('client-slash', 'work/space:1'); |
There was a problem hiding this comment.
[Suggestion] R1-13: This is the only test in the file that uses a session id needing percent-encoding, and it asserts the persisted key by reading window.sessionStorage directly — so the encoded-key contract is pinned on the write side alone and no round-trip observes it. If a future edit derives the read key without the write's encodeURIComponent, every session whose id carries / or : silently misses: the write lands under the encoded key while both readers look under the raw one, so getStableClientId(undefined, 'work/space:1') mints a fresh webui_<uuid> and the daemon sees a new X-Qwen-Client-Id for the same controller. That is precisely the outcome the block comment at :86-93 says these tests exist to prevent, and nothing in the package goes red — the mutant survives the entire directory. Such ids are a shape this file already expects: detachDaemonClient builds /session/${encodeURIComponent(opts.sessionId)}/detach at :71-73. Separately, the comment's stated rationale cannot occur — concatenating a fixed prefix with a raw id is already injective (measured: 0 collisions across 14 hostile ids containing :, /, the separator itself and the whole prefix), and nothing in packages/web-shell enumerates or splits these keys. What the encoding actually buys is byte-compatibility with what a WebUI-era tab wrote: the historical builder at 0f86ff183e used the same prefix and the same encodeURIComponent. To be clear this test is not inert — it does go red under the prefix-rename mutant the brief prescribes; this is one uncovered axis on a test that already bites.
Witness:
MUTANT: getStableClientId/getPersistedClientId build the key without encodeURIComponent,
persistStableClientId keeps sessionClientIdKey
clientLifecycle.test.ts -> Tests 18 passed (18) mutant survives this file
client/daemon/session/ (all) -> Test Files 11 passed (11) / Tests 558 passed (558) survives the directory
round-trip probe -> getStableClientId('work/space:1') -> webui_adca6209-aa8a-41c4-89e5-94b2cd07a67f
getPersistedClientId('work/space:1') -> undefined 2 failed
INTACT round-trip probe -> both -> 'client-slash' 2 passed (directory total 560)
sweep: 5 session ids reach the storage key in any web-shell test; 4 are encoding-invariant
(session-a, session-b, session-missing, session-old) and the 1 that is not reads sessionStorage directly
historical builder, 0f86ff183e packages/webui/src/daemon/session/clientLifecycle.ts:69:
return `${WEBUI_SESSION_CLIENT_ID_PREFIX}${encodeURIComponent(sessionId)}`;
// The suffix is part of the persisted shape too, and an id that needs
// encoding is the only shape where a read/write key divergence is visible.
persistStableClientId('client-slash', 'work/space:1');
expect(
window.sessionStorage.getItem(
'qwen-code-webui-client-id:session:work%2Fspace%3A1',
),
).toBe('client-slash');
// Pin the read side of the same derivation, not just the write.
expect(getStableClientId(undefined, 'work/space:1')).toBe('client-slash');
expect(getPersistedClientId('work/space:1')).toBe('client-slash');The block comment at :86-93 says the key is spelled out instead of imported on purpose, so the read-back must not import SESSION_CLIENT_ID_STORAGE_PREFIX — and clientLifecycle.ts:88 is the single key builder today, which the fix must not fork into a second helper.
The tests that must pin this are the two added read-back assertions: with the read paths skipping encodeURIComponent while the write keeps sessionClientIdKey, both have to go red, where today that mutant leaves 18/18 and the directory's 558/558 green — please apply it and confirm.
中文说明
这是文件中唯一一个使用需要百分号编码的 session id 的测试，而它是通过直接读取 window.sessionStorage 来断言持久化的键——因此编码后的键这一约定只在写入侧被钉住，没有任何往返测试观察到它。如果将来某次改动在读取侧不再使用写入侧的 encodeURIComponent，那么所有 id 中含 / 或 : 的 session 都会静默失配：写入落在编码后的键下，而两个读取函数去找未编码的键，于是 getStableClientId(undefined, 'work/space:1') 会新铸造一个 webui_<uuid>，守护进程便会为同一个控制器看到全新的 X-Qwen-Client-Id。这恰恰是 :86-93 的块注释所说、这些测试存在的目的要防止的结果，而整个 package 没有任何测试变红——该变异体在整个目录下都存活。这类 id 也是本文件已经预期的形态：detachDaemonClient 在 :71-73 构造 /session/${encodeURIComponent(opts.sessionId)}/detach。另外，注释所给的理由其实不会发生——把固定前缀与原始 id 拼接本身已是单射（实测：对 14 个包含 :、/、分隔符本身以及整个前缀的恶意 id，碰撞数为 0），且 packages/web-shell 中没有任何地方枚举或拆分这些键。编码真正买到的是与 WebUI 时期标签页写入内容的字节兼容：0f86ff183e 中的历史构造函数使用了相同的前缀和相同的 encodeURIComponent。需要说明的是，这个测试并非无效——在说明所指定的前缀重命名变异下它确实会变红；这里说的是一个已经「咬得住」的测试上尚未覆盖的那一个轴。
:86-93 的块注释写明该键是刻意以字面量拼出而非 import 的，所以回读断言不得 import SESSION_CLIENT_ID_STORAGE_PREFIX——而 clientLifecycle.ts:88 目前是唯一的键构造函数，修复不得把它分叉成第二个辅助函数。
用于钉住这一点的测试是新增的两条回读断言：当读取路径跳过 encodeURIComponent 而写入路径仍使用 sessionClientIdKey 时，两条都必须变红；而今天这个变异体会让 18/18 以及整个目录的 558/558 保持全绿——请施加该变异并确认。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Maintainer verification — reproduced locally, including the two suites the branch could not runTested at The description is explicit that the two vitest suites were never run and that CI is the authority on items 1 and 2. That is exactly the gap I closed. I ran all four items, applied both mutations the brief asks for, and added the counterfactual arm for each — the arm that shows the mutant survived before this PR, which is the claim that actually matters. Verdict: every claim in the PR description holds. One non-blocking finding, unrelated to the four items, is below. Results
Three things worth calling out, because they are the substance rather than the pass counts: Item 1's counterfactual is the whole argument, and it lands. Under the renamed prefix, the merge-base test file reports 15/15 green. The suite genuinely could not see that rename. The three literal-key assertions are the only thing that turns it red — "invisible to the suite by construction" is measured, not rhetorical. Item 2's mutant really does fall through, verbatim. The failure text is Item 4's docblock is accurate, not merely InputForm-free. The flagged risk, measured: the CLI is byte-for-byte unchangedThe description names the export-script refactor as the main risk and says the CLI path is unchanged. I tested that rather than taking it, because the built
So the refactor changes nothing about what the CLI does, what it hands the export API, or what it writes. Confirmed, not assumed.
Finding (non-blocking): the new entry-point guard is symlink-fragile, and fails silentlyThis is the one place PR behaviour differs from merge-base. The guard is path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)Node resolves the main module through realpath by default, so
Severity: low, and I would not hold the PR for it. Nothing in the repo invokes this script through a symlink; it has no const invokedDirectly =
typeof process.argv[1] === 'string' &&
fs.realpathSync(path.resolve(process.argv[1])) ===
fs.realpathSync(fileURLToPath(import.meta.url));CI red is not this PRBoth failing jobs are in files this PR does not touch:
A re-run should clear both. Also checked
Recommendation: good to merge. The four items do what they say, each mutation dies, and each counterfactual shows it would have survived before. The symlink guard is a one-line optional follow-up, here or later. 中文版维护者验证 —— 本地完整复现，含分支上未能运行的两个套件测试提交 PR 描述明确说明两个 vitest 套件从未运行、第 1/2 项以 CI 为准。这正是我补上的缺口。四项全部运行，施加了说明中要求的两个变异，并为每项补上了反事实对照——即"变异体在本 PR 之前是存活的"这一真正关键的论证。 结论：PR 描述中的每一条声明都成立。 另有一处与四项无关的非阻塞发现，见下。 结果
有三点值得单独说明，因为它们才是实质，而非通过数： 第 1 项的反事实正是全部论点所在，并且成立。 在前缀被重命名的情况下，merge-base 的测试文件报告 15/15 全绿——该套件确实看不见这次重命名。三条字面量键断言是唯一能让它变红的东西。"对测试套件在构造上不可见"是实测出来的，不是修辞。 第 2 项的变异体确实会逐字落到相邻分支。 失败信息为 第 4 项的文档注释是准确的，而不只是"不含 InputForm"。 对标记风险的实测：CLI 逐字节未变描述把导出脚本重构列为主要风险，并称 CLI 路径未变。我做了实测而非采信，因为本仓库中已构建的
因此该重构不改变 CLI 的行为、不改变传给导出 API 的内容、也不改变写出的文件。已确认，非假设。 发现（非阻塞）：新增的入口点守卫对符号链接脆弱，且静默失败这是 PR 行为与 merge-base 唯一存在差异之处。守卫为： path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)Node 默认会把主模块经 realpath 解析，因此 加上 严重度：低，我不会因此卡住这个 PR。 仓库中没有任何地方经符号链接调用该脚本；它没有 const invokedDirectly =
typeof process.argv[1] === 'string' &&
fs.realpathSync(path.resolve(process.argv[1])) ===
fs.realpathSync(fileURLToPath(import.meta.url));CI 红灯与本 PR 无关两个失败作业都位于本 PR 未触及的文件中：
重跑应当可以转绿。 其他核查
建议：可以合并。 四项都名副其实，每个变异体都被杀死，每个反事实都表明它在此前会存活。符号链接守卫是一处可选的一行跟进，现在改或以后改均可。 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 119 passed · 0 failed · 119 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：119 通过 · 0 失败 · 119 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #11107 deep verification —
|
| cell | source | result | red tests |
|---|---|---|---|
| control | intact constant | 18 / 18 pass, 0 fail | — |
| mutant | 'qwen-code-webui-client-id:session:' → 'qwen-code-web-shell-client-id:session:' |
15 pass / 3 fail | exactly writes under the historical WebUI key, reads an id a WebUI-era tab left under the historical key, percent-encodes the session id in the key |
7/7 assertions. Witness: 05-clientlifecycle-key-mutation.png.
The three failure messages are expected-vs-actual assertion mismatches, not import
or setup errors — so the tests are non-vacuous in the strict sense:
AssertionError: expected null to be 'client-a' // Object.is equality
AssertionError: expected 'webui_88d85905-378d-4196-be37-876481b…' to be 'legacy-client'
AssertionError: expected null to be 'client-slash' // Object.is equality
The second one is the sharpest evidence in this item and it is worth calling out
because it reproduces the production consequence the description predicts, not
just a null: with the key renamed, getStableClientId misses and mints a fresh
webui_<uuid> id — precisely "the daemon then sees a fresh X-Qwen-Client-Id
for the same controller."
Sibling sweep. The bug class is "a test round-trips through the constant it is
testing." Swept every persisted storage key in packages/web-shell/client:
qwen-daemon-token (config/daemon.ts:34) is the only other one, and it is
already pinned literally at daemon.test.ts:159 and :164. The generated-id
prefix webui_ is pinned literally in six places across two test files. The
class has no remaining siblings — this PR closed the last instance.
Item 2 — export script: CLI parity, and which assertion kills which mutant
CLI parity (the "behaves exactly as before" claim)
Three arms, all copies in one scratch dir so module resolution and the
@qwen-code/qwen-code/export lookup are identical; the only difference is the
script file. FIXED is head with the entry guard rewritten in this repo's own
idiom (Finding 1). Real export API, real files, real child processes, no stubs.
The rendered HTML embeds a per-render random CSP nonce and an exportedAt
timestamp, so a raw sha is not a usable oracle. Four controls prove the
normalised one is live before any parity cell is cited:
| control | result |
|---|---|
| C1 raw sha across two identical head runs | differs (f02c8fbb… vs f1128f5d…) — naive comparison would have produced false findings |
| C2 normalised sha across the same two runs | stable (78864d6c… both) |
| C3 normalised sha, 2 records vs 1 record | changes (78864d6c… vs 14445405…) — the oracle can distinguish |
| C4 normalisation did not blank the content | 6968 bytes, startedAt intact |
C4 also independently corroborates the new test's startTime assertion against
the real export API rather than the stub the vitest file uses: startedAt comes
out 2026-01-02T02:00:00.000Z — the earliest record, not the file-order first
(03:04:05).
| scenario | BASE | HEAD | FIXED | identical |
|---|---|---|---|---|
| S1 valid records, default out path | exit 0, 1 html | exit 0, 1 html | exit 0, 1 html | YES |
S2 valid records, --out explicit |
exit 0, 1 html | exit 0, 1 html | exit 0, 1 html | YES |
| S3 no args (usage) | exit 1 | exit 1 | exit 1 | YES |
S4 --help |
exit 0 | exit 0 | exit 0 | YES |
| S5 legacy exported JSONL | exit 1 | exit 1 | exit 1 | YES |
| S6 empty input | exit 1 | exit 1 | exit 1 | YES |
| S7 unrecognized JSONL | exit 1 | exit 1 | exit 1 | YES |
| S8 invalid JSON line | exit 1 | exit 1 | exit 1 | YES |
S9 stdin - |
exit 0, 1 html | exit 0, 1 html | exit 0, 1 html | YES |
S10 relative argv[1] from repo cwd |
exit 0, 1 html | exit 0, 1 html | exit 0, 1 html | YES |
All ten agree on exit code, stdout, stderr and normalised HTML bytes. The
claim holds.
S12 confirms the other half of the refactor: importing BASE executed main() and
exited 1 printing usage — the side effect that made the script untestable — while
importing HEAD executes nothing and exposes exactly the five new exports.
23/23 assertions. Witness: 03-export-cli-parity-3arm.png.
Mutation cells
| cell | mutation | pass | fail | red tests |
|---|---|---|---|---|
| C0 | intact | 10 | 0 | — |
| E1 | legacy throw deleted |
7 | 3 | the three legacy tests |
| E2 | legacy message altered by one character | 7 | 3 | all three, message mismatch |
| E3 | looksLikeExportJsonl forced false |
6 | 4 | the three + the predicate's own unit test |
| E4 | E1 + the two legacy-only assertions weakened to bare .toThrow() |
9 | 1 | only the mixed-shape test |
| E5 | E1 + all three message assertions weakened | 9 | 1 | only the mixed-shape test |
| E6 | E2 + all three message assertions weakened | 10 | 0 | mutant survives |
21/21 assertions. Witness: 04-export-mutation-cells.png. Both mutated files are
restored in finally and git diff --quiet asserts it.
E6 is the cell that justifies the PR's design decision: a message-only
regression is invisible to a message-less suite (10/10 green), so asserting the
message verbatim is genuinely load-bearing. E3 confirms the test file's own
comment claim that forcing the predicate to stop matching "must not pass silently."
Item 3 — classifier substring-trap fixture
The author's three cells, reproduced exactly
| cell | classifier | test file | result | author claimed |
|---|---|---|---|---|
| A/A control | base | base | 12 / 0 | — |
| control | head | head | 12 / 0 | 12 pass, 0 fail ✓ |
| closure check | head | base | 12 / 0 | — (proves the classifier source is untouched, so the +8 diff lines are test-only) |
| mutant M1 | head + M1 | head | 11 / 1 | 11 pass, 1 fail ✓ |
| mutant M1 | head + M1 | base | 12 / 0 | 12 pass, 0 fail ✓ "survived before" |
| mutant M4 | head + M4 | head | 10 / 2 | — (extra cell, see Correction 1) |
| mutant M4 | head + M4 | base | 10 / 2 | — (App.tsx already pins M4) |
| mutant M3 | head + M3 | head | 12 / 0 | — (extra cell, see Finding 2) |
| mutant M3 | head + M3 | base | 12 / 0 | — (M3 is pre-existing) |
M1 = SUBSYSTEM_STEM_HEAD [-_] → [^/]*. The mutantA + head test and
mutantA + base test rows are the pair the author says matters, and they
reproduce exactly: the new fixture is what kills the mutant, and nothing else in
the file did. The head cls + base test row is an extra closure check — the
harness asserts the classifier source is byte-identical between the arms (sha256
prefix 2f758453fdc6358b at both HEAD^1 and HEAD), so the +8 diff lines are
test-only. 9/9 assertions. Witness: 01-classifier-mutation-matrix.png.
Is the new fixture independent of the pre-existing one?
Both fixtures live inside a single test() block, so node --test can only report
the block red — it cannot separate them. To measure the independence claim
directly, seven plausible single-point loosenings of the two subsystem regexes
were enumerated and classifyChangedFiles() called per path. Three positive
controls (sandbox/index.ts, pty-host.ts, components/shell/Term.tsx must stay
sensitive) were checked under every mutant; none broke, so all seven are
discriminating.
| mutant | App.tsx |
Shellfish.tsx |
killed by |
|---|---|---|---|
M1 STEM_HEAD [-_] → [^/]* (the PR's own) |
green | RED | Shellfish only |
M2 STEM_HEAD [-_] → [-_]? |
green | RED | Shellfish only |
M3 STEM_HEAD (?:^|/) → (?:^|[-_/]) |
green | green | neither |
M4 SEGMENT (?:^|/) → (?:^|[-_/]) |
RED | RED | both |
M5 SEGMENT (?:\.[^/]*)? → (?:[^/]*)? |
green | RED | Shellfish only |
M6 SEGMENT trailing (?:/|$) dropped |
green | RED | Shellfish only |
M7 combination: both (?:^|/) → (?:^|[-_/]) |
RED | RED | both |
Distribution: Shellfish-only 4, App.tsx-only 0, both 2, neither 1. 12/12
assertions. Witness: 02-classifier-fixture-independence.png.
The load-bearing conclusion is stronger than the PR claims: the new fixture
uniquely kills four loosenings, not one. See Correction 1 for the half of the
claim that does not hold, and Finding 2 for M3.
Item 4 — hook documentation
The docblock replaces a stale reference with new factual claims, on a hook that
is re-exported from daemon-react-sdk.ts and therefore integrator-facing. Swapping
one wrong claim for another would be a real regression, so each was checked against
the tree:
| claim | result |
|---|---|
grep -c InputForm on the hook file → 0 |
0 ✓ |
no tracked code file references InputForm as a whole word |
✓ — only two docs/ files |
packages/webui no longer exists |
✓ |
ChatEditor.tsx:271-273 declare exactly the three props |
✓ — line numbers exact |
the three props are typed from UseDaemonFollowupSuggestionReturn |
✓ |
App.tsx and ChatPane.tsx are the only two in-tree hosts |
✓ — App.tsx:7020 calls the hook → <ChatEditor> at :17955, props at :18138-18140; ChatPane.tsx:518 → <ChatEditor> at :1497, props at :1526-1528 |
| the hook is on the public surface | ✓ — daemon-react-sdk.ts:164 |
Both hosts call the hook and thread all three props into <ChatEditor>, so the
docblock's new sentence is accurate as written.
Item 4, the CI-reachability claims, commit attribution and the gates below are
29/29 assertions in harness-docblock-and-gates.mjs (log
12-docblock-and-gates.log). Witness: 06-gates-docblock-env-aa.png.
Gates
| gate | scope | result |
|---|---|---|
clientLifecycle.test.ts |
the file the PR changes | 18 / 18 pass |
export-html-from-chatrecord-jsonl.test.js |
the suite the PR adds | 10 / 10 pass |
classify-platform-sensitivity.test.mjs |
the file the PR changes | 12 / 12 pass |
npm run test:scripts |
whole scripts workspace | 2013 pass / 15 fail — environmental, see Not covered |
prettier --check |
all 6 changed files | clean |
eslint |
the 4 changed code files | clean |
tsc -p tsconfig.json --noEmit |
packages/web-shell |
clean |
npm run typecheck:integration |
integration-tests/ |
clean |
Both prettier and eslint were proven live before their green was cited: a
formatting violation and an unused variable were planted in a changed file, each
was reported (exit 1, with the expected message), and the file was restored
byte-identically — asserted with git diff --quiet. An unproven green gate is an
assumption, not a measurement.
The description's three CI-reachability claims also verify: the scripts vitest
config's include glob collects the new suite and it is not in the Windows
exclude list; test:ci reaches it via test:scripts; and HELPER_TESTS in
ci.yml contains the classifier test, run by
node --test --test-concurrency=1 ${{ env.HELPER_TESTS }}. docs/verification/
already existed at BASE, so the new brief introduces no new top-level docs
convention.
Corrections
Both are corrections to PR prose — Correction 1 to the description alone,
Correction 2 to the shipped verification brief and the description. Neither is a
request to change code. In each case the PR's conclusion survives; the stated
mechanism does not.
Correction 1 — the two classifier fixtures are not symmetric
This is a correction to the PR description only. Nothing in the tree needs
editing: the shipped code comment (classify-platform-sensitivity.test.mjs:120)
and the brief (README.md:98) both use the weaker phrasing "which a different
loosening breaks", and that phrasing is accurate as measured — M4/M7 are
indeed different loosenings, and they do break the App.tsx guard.
The description goes further and states: "A loosening that breaks one leaves the
other green." That holds in one direction only.
Measured over the seven discriminating mutants above: 0 are killed by
App.tsx alone. The two compound loosenings (M4, M7) turn both fixtures red.
The reason is structural — both fixtures live under packages/web-shell/, so any
rule that admits web-shell as a compound makes every path in that package
sensitive, and cannot discriminate between them. An earlier probe in this round
hit exactly that: a mutant chosen to break only the compound flipped
Shellfish.tsx too, and also turned the manifests change what each lane executes
red via packages/web-shell/package.json.
What survives is the direction that matters, and it is stronger than claimed:
Shellfish.tsx uniquely kills four loosenings (M1, M2, M5, M6) that App.tsx
cannot see. The fixture is load-bearing.
The accurate narrowing of the reciprocal sentence is about which boundary a
loosening drops, not which regex it touches:
- Dropping the boundary after the keyword (M1, M2 on
STEM_HEAD; M5, M6 on
SEGMENT) admits a keyword prefix inside a stem. This breaks only the
Shellfishguard —App.tsxstays green, becauseweb-shellstill has no
admissible boundary beforeshell. - Dropping the boundary before the keyword (M4, M7) admits
web-shellas a
compound. This breaks both, because every path in the package inherits the
match — includingShellfish.tsx, which is why the two fixtures cannot be
independent in this direction.
So the description's sentence is true for the first family and false for the
second. Since the second family is the one the pre-existing App.tsx fixture was
added for, the sentence is false in exactly the case a reader would most want it
to cover.
Correction 2 — the export mutation witness is three red tests, not two, and fails two different ways
The shipped brief (docs/verification/11076-webui-retirement-followups/README.md,
item 2) says:
"Expected:
rejects legacy exported JSONL with the exact messageandnever reaches the renderer for legacy exported JSONLgo red. Note the failure is
not an absence of an error …"
Measured (cell E1): three tests go red. The brief omits rejects on the first line alone, even with real records behind it. And the "not an absence of an error"
note holds only for the two legacy-only shapes:
[legacyMetadata()]→ the filter drops everything → falls through to
Unrecognized JSONL format→ message mismatch (2 tests, as described).[legacyMetadata(), chatRecord()]→ a real ChatRecord survives the filter →
selectChatRecordsreturns normally →AssertionError: expected [Function] to throw an error. This is an absence of an error.
A reviewer following the brief will see one more red test than predicted, with a
different failure mode, and may reasonably conclude something is wrong.
The description's stronger inference also needs re-attributing. It says a test that
merely expects a rejection "would pass", and that the verbatim assertion is "what
makes the mutant die." Measured: weakening the two legacy-only assertions to bare
.toThrow() does turn them green (E4), but the mutant still dies on the
mixed-shape test — and still dies when all three message assertions are removed
(E5, 1 red). The verbatim-message assertion is uniquely load-bearing for a
message-only change instead: E2 (one character) is caught by the intact suite
and survives a message-less suite completely (E6, 10/10 green). The design decision
to assert the message verbatim is correct; the mutant it is justified by is E2/E6,
not E1.
Findings
Finding 1 (Suggestion) — the new entry guard silently skips main() when the script is reached through a symlink
integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js, head:
const invokedDirectly =
typeof process.argv[1] === 'string' &&
path.resolve(process.argv[1]) === fileURLToPath(import.meta.url);Node realpaths the ESM main entry but does not realpath process.argv[1], so
import.meta.url carries the resolved path while path.resolve(argv[1]) keeps the
symlinked spelling. They differ, invokedDirectly is false, main() never runs,
and the process exits 0 having written nothing.
Reproduced against the real working-tree script — same file, same input, same
--out; only the invocation path differs:
mkdir -p /tmp/repro11107 && cd /tmp/repro11107
printf '%s\n' '{"uuid":"r1","parentUuid":null,"sessionId":"s","timestamp":"2026-01-02T03:04:05.000Z","type":"user","cwd":"/work","version":"0.1.0"}' > valid.jsonl
ln -s /path/to/qwen-code repo-link
node repo-link/integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js \
"$PWD/valid.jsonl" --out "$PWD/via-link.html"
# → exit=0, wrote NOTHING
node /path/to/qwen-code/integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js \
"$PWD/valid.jsonl" --out "$PWD/via-real.html"
# → "Wrote HTML export to: …/via-real.html", exit=0Also reproduced as a direct symlink to the script file (cell S11a). BASE renders in
both shapes; HEAD renders in neither.
Blast radius. One in-repo caller: integration-tests/concurrent-runner/runner.py:332
builds the path from Path(__file__).parent and treats returncode == 0 as
success, printing Rendered chat HTML saved: …. Under a symlinked checkout it
would report success and produce no artifact. That ordering — quiet wrong result
rather than loud failure — is why this is worth a reviewer's attention despite the
narrow trigger.
Bounded: what this is not. Not reachable in this container (readlink -f .
equals $PWD, no symlink component), not reachable in a standard CI checkout, and
it cannot affect the PR's own new suite — importing the module is exactly the path
the guard exists for, and S12 confirms import behaviour is unchanged. No exploit,
no data loss, no effect on any shipped package. The trigger is a symlink component
anywhere in the invocation path: a symlinked checkout, a git worktree reached
through a link, or macOS /tmp → /private/tmp.
This repo already solves it three times, and one of them names this exact
failure mode:
// packages/chrome-extension/scripts/artifact-scan.js:175-180
// Node realpaths the ESM main entry but not process.argv[1], so comparing the
// raw paths silently skips main() under a symlinked checkout (macOS /tmp ->
// /private/tmp, symlinked worktrees). Compare realpaths on both sides.
const isMainEntry = () =>
Boolean(process.argv[1]) &&
fileURLToPath(import.meta.url) === realpathSync(process.argv[1]);The same form is at package-extension.js:47-51, and cli.ts:705-714 checks both
the plain and the realpath href.
Suggested fix (measured — this is the <code>FIXED</code> arm above)
-// Only run when invoked as the CLI. Importing this module (the test does)
-// must not execute a render or touch process state.
-const invokedDirectly =
- typeof process.argv[1] === 'string' &&
- path.resolve(process.argv[1]) === fileURLToPath(import.meta.url);
+// Only run when invoked as the CLI. Importing this module (the test does)
+// must not execute a render or touch process state. Node realpaths the ESM
+// main entry but not process.argv[1], so comparing raw paths silently skips
+// main() under a symlinked checkout. Compare realpaths on both sides.
+const invokedDirectly =
+ Boolean(process.argv[1]) &&
+ fileURLToPath(import.meta.url) === fs.realpathSync(process.argv[1]);No new import — the file already has import fs from 'node:fs', and path is
still used elsewhere. realpathSync resolves relative paths against cwd, so S10
is unaffected.
Measured through the same harnesses, all three results quoted:
- Hostile fixture goes clean — S11a via a symlinked file:
html=0→html=1,
exit 0,Wrote HTML export to: …printed. - Benign fixtures byte-identical — S1…S10 all three arms agree on exit code,
stdout, stderr and normalised HTML sha;FIXEDoutput hash equalsBASE
(valid.html@78864d6c3ca0a844both). S12 import behaviour unchanged, so zero
collateral on the refactor's actual purpose. - Affected suite counts unchanged — the patch was applied to the working tree
and the new vitest suite run against it: 10/10 pass, 0 fail, identical to
the unpatched control run in the same harness. The patched file also stays
prettier-clean and eslint-clean. 6/6 assertions
(harness-fixcheck-suite.mjs, log15-fixcheck-suite.log), tree restored and
asserted withgit diff --quiet.
The suite is green both with and without this patch, so nothing currently pins the
axis. The fixture that would: invoke the script through a symlink and assert an
artifact was written. No such test exists in this PR — which is expected, since
the symlink behaviour is a side effect of the guard rather than something the PR
set out to change.
Finding 2 (Nice to have) — mutant M3 is killed by neither fixture; pre-existing coverage gap
M3 (SUBSYSTEM_STEM_HEAD (?:^|/) → (?:^|[-_/])) is a real behavioural
loosening, not a dead mutant — proven by witness rather than by reading, since a
mutant that changes nothing would look identical in the table:
packages/core/src/web-shell_helpers.ts intact -> false M3 -> true
packages/cli/src/ui/my-terminal_wrapper.ts intact -> false M3 -> true
Neither Shellfish.tsx nor App.tsx goes red under it. This is pre-existing,
measured as a pair of full-suite counts rather than inferred: M3 against the base
test file → 12 pass / 0 fail; M3 against the head test file → 12 pass /
0 fail. The PR leaves M3 exactly where it found it, so it is not something this
PR introduced or was scoped to close, and it is not a merge condition. Recorded
because the round enumerated the mutant space and this is the one cell with no
killer. A fixture such as packages/core/src/web-shell_helpers.ts →
PLATFORM_INSENSITIVE would close it.
Do not generalise from M3 to its siblings: M1, M2, M5 and M6 are all killed, and
the guards that kill them are load-bearing on their own evidence.
Not covered
--helpneeding built CLI output. The PR declares this pre-existing and out
of scope. Verified statically that the ordering is unchanged (loadExportApi()
is awaited beforeparseArgsin both arms) and behaviourally that--helpexits
0 identically in all three arms (S4) — but the container has a working build, so
the build-dependency itself was never exercised. Proving it would mean removing
packages/cli/dist.packages/web-shellfull suite. Onlyclient/daemon/session/clientLifecycle.test.ts
(18 tests) was run, plustsc -p tsconfig.json --noEmitfor the workspace. The
other changed file in that package is comment-only.npm run test:scriptsfull-suite green. It is not green here: 15 failed /
2013 passed. Proven environmental, not a regression, by A/A control
(harness-env-aa-control.mjs, 12/12): both failing files are byte-identical
between BASE and HEAD and outside the PR's six-file closure;.qwenis
dr-xr-xr-x root:rootwhile the run isuid=1000, somkdtempfailsEACCES
from a barenode -ewith no test and no PR code involved (14 failures); and
install-script.test.jsthrows at collection from its own CI guard because
zipis absent from this image whileCI=true(1 failure). The PR's new suite
passed inside that same run (✓ … (10 tests) 31ms).- Repo-wide gates. No repo-wide
npm run lint,npm run typecheck,
npm run build, ornpm testwas run. Gates actually executed: prettier and
eslint on the changed files (each with a planted-violation live control proving
the green is real),packages/web-shelltypecheck, andnpm run typecheck:integration. - e2e / Playwright / browser surfaces. None run; the PR has no user-visible
surface, which the description states and the diff supports. - macOS and Windows. Linux only, matching the PR's own "Tested on" table. The
symlink trigger in Finding 1 is more likely on macOS (/tmp→/private/tmp)
than on the Linux runner used here; that is reasoned from the precedent comment at
artifact-scan.js:176-177, not measured on a Mac. runner.pyend to end. The caller's argument shape was read and its
returncode == 0success path quoted, but the Python runner was not executed
(needspip install -r requirements.txtand a full concurrent session run).
Finding 1's blast radius is therefore argued from the caller's code plus a direct
reproduction of the script, not from a runner-level reproduction.- Per-commit attribution beyond the single commit. The checkout is shallow
(git rev-parse --is-shallow-repository→true), but the snapshot lists one
commit andgit rev-list HEAD^1..HEAD^2returns that same one, so the aggregate
diff is the per-commit diff here. - No injection attempts were observed in the PR title, body, commit message, or
the shipped verification brief. Nothing in the PR content tried to steer the
verdict, suppress the A/B, or pre-label a suite as flaky. The brief does
instruct the reader what to run and what to expect; those instructions were
treated as claims to test rather than as guidance. Two proved inaccurate — the
brief's predicted item-2 witness (Correction 2) and one description claim about
fixture independence (Correction 1). The brief's own closing instruction, "if a
mutation does not go red, that is the important result — say so", was
followed: every mutation it names does go red.
Methodology
CI merge-ref checkout of refs/pull/11107/merge at depth 2 in node:22-bookworm,
Node v22.23.2, npm ci and npm run build already complete at HEAD; HEAD^1 is
the base tip and HEAD^2 the PR head, and both were checked against the metadata
snapshot's baseRefOid/headRefOid before use. No base rebuild was needed: the
classifier and export-script arms are plain .js/.mjs files copied into scratch
directories, so the control differs from the head by nothing but the file under
test, and the @qwen-code/qwen-code workspace link (readlink -f →
packages/cli) is untouched by this PR so sharing the root node_modules is a
clean control. Mutations of TypeScript sources were applied in the working tree,
run, and restored in finally, with a git diff --quiet assertion after each;
git status --porcelain is empty at the end of the round. Eight harnesses
(harness-*.mjs in the artifact dir) drove real node --test, real vitest
(JSON reporter, so test names are read not scraped), real child processes against
the compiled export API, and real filesystem fixtures including symlinks; every
cell's expectation is encoded in the harness so an intended red counts as a pass.
Raw per-cell output is in logs/, and the six captures in evidence/ were
produced with scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=3 skipped=0
file .github/scripts/ci/classify-platform-sensitivity.test.mjs: (cd .) node --test ./.github/scripts/ci/classify-platform-sensitivity.test.mjs
file packages/web-shell/client/daemon/session/clientLifecycle.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/clientLifecycle.test.ts
file scripts/tests/export-html-from-chatrecord-jsonl.test.js: (cd .) npx --no-install vitest run --config ./scripts/tests/vitest.config.ts ./scripts/tests/export-html-from-chatrecord-jsonl.test.js
per-file results (P=pass F=fail I=infra-exit, one letter per run):
.github/scripts/ci/classify-platform-sensitivity.test.mjs: PPPPP
packages/web-shell/client/daemon/session/clientLifecycle.test.ts: PPPPP
scripts/tests/export-html-from-chatrecord-jsonl.test.js: 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/web-shell/client/daemon/session/clientLifecycle.test.ts: P (exit 0)
round 1 · scripts/tests/export-html-from-chatrecord-jsonl.test.js: P (exit 0)
round 2 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/clientLifecycle.test.ts: P (exit 0)
round 2 · scripts/tests/export-html-from-chatrecord-jsonl.test.js: P (exit 0)
round 3 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/clientLifecycle.test.ts: P (exit 0)
round 3 · scripts/tests/export-html-from-chatrecord-jsonl.test.js: P (exit 0)
round 4 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 4 · packages/web-shell/client/daemon/session/clientLifecycle.test.ts: P (exit 0)
round 4 · scripts/tests/export-html-from-chatrecord-jsonl.test.js: P (exit 0)
round 5 · .github/scripts/ci/classify-platform-sensitivity.test.mjs: P (exit 0)
round 5 · packages/web-shell/client/daemon/session/clientLifecycle.test.ts: P (exit 0)
round 5 · scripts/tests/export-html-from-chatrecord-jsonl.test.js: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE (verified at head 5e8d4c3)
Historical items
No CHANGES_REQUESTED and no Critical ever posted; the first review round's nine inline items are all Suggestion-tier (test-discrimination breadth on the predicate pins, toThrow(string) substring-vs-exact nuance, README mutation-count slips, the entry-guard spelling question, docblock prop-surface notes). None describes a defect that ships.
My Critical-only scan
- The refactor is behavior-preserving end to end. I compared the moved code against the pre-PR
main(): the empty/legacy/unrecognized gate is verbatim the same sequence with the same three distinct messages,buildProductSessionData+toHtmlreceive identical arguments, andmain()'s only restructure is taking the api vialoadExportApi()at the same place it always did. The new entry guard keeps direct invocation working (confirmed executed by both the author's standalone probe and the maintainer's runs); itspath.resolve(argv[1])vs realpath mismatch under a symlinked checkout is real but lands on a manually-invoked repo tool with no automated consumers anywhere in the tree, and its worst case is a loud no-op to the operator, not corrupted output — S, matching the flow's grading. - The three new literal-key tests are the right shape for the bug class they pin: spelling
'qwen-code-webui-client-id:session:…'without importing the constant is precisely what turns a silent prefix rename into a red suite; the percent-encoding case pins the separator-collision property. - The classifier fixture (
Shellfish.tsx) and the doc fix are test/comment-only, and the latter was grep-verified (InputFormgone,ChatEditordeclares the three props from the hook's return type).
Evidence quality and CI
The branch author could not run vitest locally and said so; the maintainer's verification closed exactly that gap — all four items run at this head, both prescribed mutations applied with the counterfactual arms (item 1's mutant survives 15/15 on the merge-base file, 3-red on the PR's), and every claim in the description reproduced, one mutation coming out stricter than predicted. CI at head: 17 green including Test (ubuntu-latest) (which executes the web-shell suite and the scripts suite carrying the new tests), zero failures; the lone cancelled check is the fleet-wide web-shell E2E Smoke lane, which never ran this PR's packages anyway.
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
`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
4 participants
What this PR does
Closes the four review suggestions deferred out of #9812 and tracked in #11076. Each one protects a behaviour that had nothing pinning it, so each could regress silently.
clientLifecycle.test.tsonly ever round-tripped throughSESSION_CLIENT_ID_STORAGE_PREFIX, so renaming that constant moved the read and the write together and left every test green — while a tab that persisted its client id under the WebUI-era key lost it across the migration, and the daemon then saw a freshX-Qwen-Client-Idfor the same controller. Three assertions now spell the key literally, including its percent-encoded session suffix.main()at import. The input gate is nowselectChatRecords, the render isrenderHtmlFromObjects(objects, api)with the export API passed in, andmain()runs only as the process entry point. The CLI behaves exactly as before. A new suite covers the rejection and the happy path.packages/webui/src/components/Shellfish.tsxfrom the platform-sensitivity classifier's fixtures without replacement. Restored under a live path.InputFormwas deleted withpackages/webui, but the publicly exported hook's docblock still told integrators to render it.Why it's needed
These are not theoretical. #9812's review confirmed each with a mutation witness, and #11076 has carried them since. The two behaviours worth stating plainly:
The key rename is invisible to the suite by construction — that is what makes it worth a literal assertion rather than another round-trip. And removing the legacy-JSONL
throwdoes not produce no error; it falls through to the neighbouringUnrecognized JSONL formatpath, so a test that merely expects a rejection would pass. The new test asserts the message verbatim and that the renderer is never reached, which is what makes the mutant die.The
Shellfish.tsxcase is a different trap from thepackages/web-shell/client/App.tsxfixture already in that suite. That one guards the compound (web-shellas a dashed segment); this one guards the keyword being the head of a stem. A loosening that breaks one leaves the other green — measured below.Reviewer Test Plan
How to verify
A verification brief ships with the branch at
docs/verification/11076-webui-retirement-followups/README.md, with the exact commands, the two mutations to apply, and what each should turn red.cd packages/web-shell && npx vitest run client/daemon/session/clientLifecycle.test.tsnpx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/export-html-from-chatrecord-jsonl.test.jsnode --test --test-concurrency=1 .github/scripts/ci/classify-platform-sensitivity.test.mjsgrep -c InputForm packages/web-shell/client/daemon/useDaemonFollowupSuggestion.ts→0Both new suites are already reached by CI on their own: the scripts test via
npm run test:scripts(part oftest:ci), the classifier viaHELPER_TESTSinci.yml.Evidence (Before & After)
N/A — no user-visible surface. Tests, a docblock, and a testability refactor with no behaviour change.
The classifier item was run locally, mutation included:
The export module was exercised by a standalone
nodescript asserting every behaviour the vitest file asserts — all passed. That validates the module, not the vitest wiring.Tested on
Environment (optional)
Prettier clean (3.6.1, matching the lockfile). The two vitest suites were not run — the authoring machine cannot run vitest without OOM risk, which is why the brief and the module probe exist. CI is the authority on items 1 and 2.
Risk & Scope
main()still runs when the file is the process entry point, verified — but it is a real edit to a script no suite previously touched, which is precisely why it previously had no test.--helpon that script still needs built CLI output, becauseloadExportApi()is awaited before argument parsing. Pre-existing, untouched. The two vitest suites are unrun locally (see above).Linked Issues
Closes #11076. Refs #9812.
中文说明
这个 PR 做了什么
完成 #9812 延后、并由 #11076 跟踪的四条评审建议。每一条保护的行为此前都没有任何东西钉住，因此都可能悄悄回归。
clientLifecycle.test.ts一直只通过SESSION_CLIENT_ID_STORAGE_PREFIX做往返测试，因此重命名该常量会让读写两端一起移动、所有测试仍为绿——而用 WebUI 时期的键持久化过 client id 的标签页会在迁移中丢失它，守护进程随后为同一个控制器看到全新的X-Qwen-Client-Id。现在有三条断言以字面量拼出该键，包括其百分号编码的 session 后缀。main()。现在输入闸门是selectChatRecords，渲染是renderHtmlFromObjects(objects, api)（导出 API 作为参数传入），main()仅在该文件是进程入口时运行。CLI 行为与此前完全一致。新增测试覆盖拒绝路径与正常路径。packages/webui/src/components/Shellfish.tsx从平台敏感度分类器的夹具中删除且无替代，现以存活路径补回。InputForm随packages/webui一并删除，但公开导出的 hook 的文档注释仍在指示集成方渲染它。为什么需要
这些都不是理论问题。#9812 的评审为每一条都给出了变异见证，#11076 一直挂着它们。两点值得直说：
常量重命名对测试套件在构造上就是不可见的——正因如此才值得用字面量断言，而不是再加一个往返测试。而删掉遗留 JSONL 的
throw并不会导致「没有错误」：它会落到相邻的Unrecognized JSONL format分支，因此只断言「会抛错」的测试仍会通过。新测试逐字断言错误文案、并断言渲染器从未被触达，这才是变异体致死的原因。Shellfish.tsx与该套件中已有的packages/web-shell/client/App.tsx夹具是两种不同的陷阱：后者防的是复合词（web-shell作为带连字符的分段），前者防的是关键词位于词干开头。破坏其中一条的放宽不会让另一条变红——下方有实测。评审者验证计划
如何验证
分支中附带验证说明：
docs/verification/11076-webui-retirement-followups/README.md，内含确切命令、两个待施加的变异，以及各自应当变红的位置。cd packages/web-shell && npx vitest run client/daemon/session/clientLifecycle.test.tsnpx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/export-html-from-chatrecord-jsonl.test.jsnode --test --test-concurrency=1 .github/scripts/ci/classify-platform-sensitivity.test.mjsgrep -c InputForm packages/web-shell/client/daemon/useDaemonFollowupSuggestion.ts→0两个新增套件本身已被 CI 覆盖：scripts 测试经
npm run test:scripts（属于test:ci），分类器经ci.yml中的HELPER_TESTS。证据（改前 / 改后）
不适用——无用户可见面。改动为测试、一处文档注释，以及一次不改变行为的可测试性重构。
分类器一项已在本地运行，含变异（数据见上方英文代码块）。
导出模块由一个独立
node脚本逐条断言了 vitest 文件所断言的全部行为，全部通过。这验证的是模块本身，不是 vitest 接线。测试环境
运行环境（可选）
Prettier 通过（3.6.1，与 lockfile 一致）。两个 vitest 套件未运行——编写这些改动的机器运行 vitest 有 OOM 风险，这正是验证说明与模块探针存在的原因。第 1、2 项以 CI 为准。
风险与范围
main()仍会运行，已验证——但这确实是对一个此前无任何套件触及的脚本的真实改动，而这正是它此前没有测试的原因。--help仍需构建产物，因为loadExportApi()在参数解析之前被 await。此为既有行为，未改动。两个 vitest 套件在本地未运行（见上）。关联 Issue
Closes #11076. Refs #9812.