fix(web-shell): preserve slash command attachments - #10730
Merged
Merged
Conversation
Collaborator
Collaborator
|
Re-run on the current head (
Moving on to code review. 🔍 中文说明在当前 head（
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Re-run review at Code reviewI re-read the whole diff against base code, with the review history in mind. The shape is what I'd have proposed — classify at the two points where the command kind is knowable, reuse the existing attachment resolution — and the points raised in earlier rounds were genuinely addressed, not papered over.
No Critical findings on the current diff. Three open items, none individually blocking:
Testing evidenceUnattended triage never builds or runs PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via the API. The honest state: the unit suite never reached a verdict on this commit. The one red: Everything that exercises this PR's surface completed green on this commit: build,
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The new unit tests pin the classification logic at both layers (they assert the exact parts sent to the model and would fail against the old discard-all behavior) — but as noted above they have not completed on this commit. The end-to-end claim (real browser → daemon → model → transcript attachment reference) rests on the author's single-platform report (macOS, not independently re-run here). Sandboxed verification would settle it: 中文说明代码审查（对照基线代码重读当前完整 diff）：形态与我独立推演一致——在两处可知命令类型的位置分类、复用现有附件解析路径；此前各轮的问题是真修复而非糊弄。运行时侧（Session.ts）在 测试证据：无人值守的 triage 不会构建或运行 PR 代码，以下为通过 API 获取的该提交自身 CI。如实说明：该提交的单测没有跑出结论—— — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 3/5 — clean review of a well-shaped, minimal fix; what I don't have is a completed green unit-suite verdict on this commit, so I can't attest to it yet. Stepping back: this is still the shape of change I'd want for this bug — my independent proposal (classify at the two points where the command kind is knowable, reuse the existing attachment resolution, fail safe when metadata hasn't arrived) is exactly what the PR does, now hardened by two review rounds. The earlier Criticals weren't waved away: the client classifier genuinely mirrors the daemon parser now (I checked it against Why 3/5 and not 4/5:
None of these is a reason to reject. The path to merge is short: a completed green unit run on the head (the author has write access and can re-run the workflow), plus a word on R4-1 and the description sentence. Sandboxed ⏸️ Deferring — no approving this round. No area owner resolved from labels or the owner policy for these paths, so this is deferred without a specific @mention: it needs a completed green CI verdict on the reviewed commit (and ideally the author's call on the two open items) before approval. A maintainer or the author can re-trigger with 中文说明总体评价：仍是该缺陷应有的修复形态——独立推演的方案（两处可知命令类型处分类、复用现有附件解析、元数据未就绪时保守处理）与 PR 一致，且经两轮审查加固。此前的 Critical 不是被敷衍掉的：客户端分类器现已逐行对照 为何 3/5 而非 4/5：其一，该 head 的 CI 没有跑出结论——ubuntu 单测作业在拥挤的自托管 runner 上以平时 4 倍时长运行后被中途取消，本 PR 新增测试未完成。唯一红项看起来是既有键盘时序抖动（位于 diff 未触碰的文件），我不会因此要求修改——但"大概率与本 PR 无关"不等于"已验证全绿"，且本 PR 触及高风险路径（acp-integration），CI 证据是批准的前置条件。其二，R4-1 未关闭—— ⏸️ 暂缓——本轮不批准。这些路径未从标签或所有者策略解析出区域负责人，故不做具体 @mention 升级：需要先在该提交上获得完整绿色的 CI 结论（最好加上作者对两个未闭项的答复）方可批准。CI 落地后维护者或作者可用 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
requested changes
Sep 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 1a": running the new/changed tests (Session.test.ts, actions.test.ts, mappers.test.ts) — the review worktree has no node_modules or built dist , and npm ci + n….
中文说明
未探索到全部深度（达到工具调用预算）："agent 1a"：running the new/changed tests (Session.test.ts, actions.test.ts, mappers.test.ts) — the review worktree has no node_modules or built dist , and npm ci + n…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+13495
to
+13498
| const attachmentBlocks = | ||
| result.resolvedCommand?.kind === CommandKind.BUILT_IN | ||
| ? [] | ||
| : originalPrompt.filter((block) => block.type !== 'text'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-7: The client gate classifies keep/discard by static source, but the daemon only honors kept attachments in the submit_prompt branch of #processSlashCommandResult; any non-built-in command whose action returns message/stream_messages/goal_control (returns null) or unsupported (throws) silently drops attachments the client already uploaded and displayed, with no cleanup. This is reachable today for MCP prompt commands (source: 'mcp-prompt'): the auto-registered help subcommand returns message/info (McpPromptLoader.ts:60-94), and the main action returns message/error on config/parse/invocation failures; parseSlashCommand descends subcommands token by token. Concretely: /gen-report help with a pasted screenshot — the client keeps and uploads it (source is not built-in), the daemon descends to the help subcommand, emits the help text, and ends the turn with zero model calls, so the image reaches no consumer and the upload is orphaned, while the transcript shows it attached to the message.
Witness:
Orphan arm: PROBE-R1-7: prompt → { stopReason: 'end_turn' }; modelCallCount=0
(sendMessageStream never called; help text emitted as agent_message_chunk)
Control arm: PROBE-R1-7-CONTROL: firstSentMessage() =
[ { inlineData: { mimeType: 'image/png', data: 'QUJD' } }, { text: 'Expanded MCP prompt' } ] ✓
(the identical attachment block is forwarded to the model when the result is submit_prompt and dropped when it is message)
Codify the contract for kept attachments whose command result is not submit_prompt: deterministically surface or drop the attachment blocks in the message/stream_messages branches (and remove the corresponding uploaded attachments so they do not orphan), or have the client suppress upload when the invocation resolves to an informational subcommand (_meta.subcommands is already on the wire).
The fix must not widen discard to non-built-in sources generally — the submit_prompt keep behaviour is pinned by the new it.each ('keeps attachments when a %s expands into a model prompt', Session.test.ts:22183) and client-side by 'uploads attachments used by skill slash commands' (actions.test.ts). Please add a Session.test.ts case that mocks handleSlashCommand to resolve an MCP_PROMPT command returning { type: 'message', messageType: 'info', content: 'help' } and calls session.prompt with a text block plus an image block, asserting the chosen behaviour (attachment cleanup/visibility).
中文说明
客户端按静态 source 分类保留/丢弃，但 daemon 只在 #processSlashCommandResult 的 submit_prompt 分支中兑现保留的附件；任何非内置命令，若其 action 返回 message/stream_messages/goal_control（返回 null）或 unsupported（抛错），都会静默丢弃客户端已上传并显示的附件，且没有清理逻辑。这在今天的 MCP prompt 命令（source: 'mcp-prompt'）上可达：自动注册的 help 子命令返回 message/info（McpPromptLoader.ts:60-94），主 action 在配置/解析/调用失败时返回 message/error；parseSlashCommand 会逐级进入子命令。具体场景：/gen-report help 附带粘贴的截图——客户端保留并上传（source 不是内置），daemon 进入 help 子命令、输出帮助文本并以零次模型调用结束本轮，图片没有任何消费者，上传成为孤儿，而会话记录却显示消息带有附件。
修复建议：为结果不是 submit_prompt 的命令明确保留附件的契约——在 message/stream_messages 分支中确定性地呈现或丢弃附件块（并移除对应的已上传附件以免成为孤儿），或者在调用解析到信息型子命令时由客户端抑制上传（_meta.subcommands 已在 wire 上）。
修复不得把丢弃扩大到所有非内置 source——submit_prompt 的保留行为由新增的 it.each（'keeps attachments when a %s expands into a model prompt'，Session.test.ts:22183）和客户端的 'uploads attachments used by skill slash commands'（actions.test.ts）固定。请在 Session.test.ts 中补充用例：mock handleSlashCommand 解析出返回 { type: 'message', messageType: 'info', content: 'help' } 的 MCP_PROMPT 命令，以文本块加图片块调用 session.prompt，并断言所选行为（附件清理/可见性）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Author
There was a problem hiding this comment.
Not changing this in this PR. A non-submit command intentionally performs no model call, while the uploaded attachment remains referenced by the recorded user message and transcript. The client currently receives no resolved-result metadata (message vs submit_prompt) that would let it safely suppress or remove the upload. Adding that protocol and cleanup behavior would widen this focused attachment-loss fix and could erase user-provided transcript evidence, so I am leaving this unresolved for a follow-up design.
qwen-code-ci-bot
previously requested changes
Sep 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D2-5 orphaned uploads for policy-blocked/disabled commands — already reported as R1-7 (comment 3906736070); author declined in this PR and deferred the class to a follow-up design
- D2-6 orphaned uploads for MCP-prompt message results — already reported as R1-7 (comment 3906736070); author declined in this PR and deferred the class to a follow-up design
Not explored to full depth (tool budget reached): "agent 1a": run the web-shell actions.test.ts / mappers.test.ts and CLI Session.test.ts suites to confirm green (worktree has no node_modules ; would require npm ci ….
Test Plan (not a blocker): tests: 190 passed — this review observed 5639, 504 passed; tests: 810 passed — this review observed 5639, 504 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session/Session.ts:13495 — [review] stacked-skill submit_prompt keep branch has no pinning test — deferred (code-age rule: unchanged since round 1)packages/web-shell/client/daemon/session/promptContent.ts:47 — [probe] attachment token glues onto command text on the no_command path — deferred (code-age rule: unchanged since round 1)
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1a"：run the web-shell actions.test.ts / mappers.test.ts and CLI Session.test.ts suites to confirm green (worktree has no node_modules ; would require npm ci …。
Test Plan（非阻断）：tests: 190 passed — this review observed 5639, 504 passed; tests: 810 passed — this review observed 5639, 504 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Author
|
CI note for 3ccf7de: the Ubuntu unit job was cancelled at the two-hour workflow limit while suites were still reporting green, with no test assertion failure. Real daemon E2E reported BUILD SUCCESS (4 tests, 0 failures) and was then cancelled at the 30-minute job limit. The coverage-comment job consequently failed only because the cancelled unit job never uploaded its coverage artifact. These are workflow/runner terminations, not failures caused by this PR; the new head reruns CI. |
Collaborator
Author
|
CI diagnosis for 43616b4: all primary failures share the same upstream compile error during npm prepare: packages/cli/src/ui/opentui/commands-registry.ts rejects the newly added output-style dialog as never. This code came from main commit 1772e14 (#10683), is unrelated to this PR attachment diff, and remains present unchanged in current origin/main 7bd939e. Unit, no-AK integration, web-shell smoke, visual capture, and Real daemon E2E all fail before their tests start; Post Coverage Comment is only a downstream failure because no coverage artifact was produced. Per scope, I am not patching this unrelated main regression in PR #10730. The automatic review job is still running. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D3-7 orphaned uploads for non-submit_prompt results (MCP-prompt help/error paths, hook-blocked skills) — already reported as R1-7 (comment 3906736070); author declined…
- D3-8 orphaned uploads for built-ins filtered out of the acp snapshot (mode filter, standalone policy, denylist) — already reported as R1-7 (comment 3906736070); author…
Not reviewed: build-and-test — the harness test phase never ran: the build chain halted on a pre-existing packages/cli compile error present on main (commands-registry.ts 'output-style' union member without its switch case, byte-identical at the merge base and the current main tip); the CLI Session.test.ts suite including this PR's daemon-side cases was not executed this round; web-shell actions.test.ts (165/165 including mutants) and mappers.test.ts (45/45) were exercised green by verification probes.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/daemon/session/promptContent.ts:18 — [review] Divergent duplicate slash gate strips attachment token linespackages/web-shell/client/daemon/session/mappers.ts:837 — [review] Third drifted copy of the altNames wire parsepackages/cli/src/acp-integration/session/Session.ts:13515 — [review] Stacked-skill keep branch (resolvedCommand undefined) unpinnedpackages/web-shell/client/daemon/session/actions.ts:880 — [review] sendPrompt display-hide for built-ins untestedpackages/web-shell/client/daemon/session/actions.ts:423 — [review] Empty-name guard (bare /) has no coveragepackages/web-shell/client/daemon/session/actions.ts:435 — [review] Zero-builtin loaded snapshot makes fail-closed permanent
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — the harness test phase never ran: the build chain halted on a pre-existing packages/cli compile error present on main (commands-registry.ts 'output-style' union member without its switch case, byte-identical at the merge base and the current main tip); the CLI Session.test.ts suite including this PR's daemon-side cases was not executed this round; web-shell actions.test.ts (165/165 including mutants) and mappers.test.ts (45/45) were exercised green by verification probes。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
Collaborator
Author
|
@qwen-code /review |
Contributor
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
Collaborator
|
Verification report at head Static review — no Critical found
Unit + mutation (local checkout, PR diff applied)
Live daemon A/B (tmux, Same prompt in both arms:
Not approved: no maintainer/ci-bot approval stands at the current head (ci-bot's latest is a zero-finding COMMENTED at One note, not a blocker: earlier CI redness on this PR (per ytahdn's 02:56Z diagnosis) was the upstream |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- orphaned uploads for commands the daemon refuses (policy-blocked / disabled / interactive-only, e.g. /statusline, standalone-blocked /export) — already reported as R1-7 (comment 3906736070); author declined in this PR and deferred the class…
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": line-by-line walk of the useQueuedPrompts drain re-submit (only its shape was grepped — rows carry images / files and re-enter the same gated action, so the…; "agent 1a": no typecheck/build run — I did not verify that record['altNames'] after Array.isArray(record?.['altNames']) in mapCommandMeta satisfies strict narrowing…; "agent 1a": did not verify whether a runtime config-gate flip (e.g. isManagedMemoryAvailable() ) re-publishes available_commands_update , which decides finding 2's reacha…; "agent 1a": did not verify whether the daemon garbage-collects uploaded session attachments that a prompt never consumed, which decides finding 2's orphan claim.; "agent 1c": whether sendPrompt 's post- submitPrompt rejection path already calls removeUploadedAttachments (I verified the abort and upload-failure paths do, at action…, and 3 more.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/daemon/session/actions.ts:435 — [probe] fail-closed bootstrap fallback drops custom/MCP/workflow attachments pre-snapshot (R1-2-mandated tradeoff); PR Risk & Scope wrongly says they 'retain' — fix the description
中文说明
已审查。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：line-by-line walk of the useQueuedPrompts drain re-submit (only its shape was grepped — rows carry images / files and re-enter the same gated action, so the…；"agent 1a"：no typecheck/build run — I did not verify that record['altNames'] after Array.isArray(record?.['altNames']) in mapCommandMeta satisfies strict narrowing…；"agent 1a"：did not verify whether a runtime config-gate flip (e.g. isManagedMemoryAvailable() ) re-publishes available_commands_update , which decides finding 2's reacha…；"agent 1a"：did not verify whether the daemon garbage-collects uploaded session attachments that a prompt never consumed, which decides finding 2's orphan claim.；"agent 1c"：whether sendPrompt 's post- submitPrompt rejection path already calls removeUploadedAttachments (I verified the abort and upload-failure paths do, at action…，另有 3 条。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| const attachmentBlocks = | ||
| result.resolvedCommand?.kind === CommandKind.BUILT_IN | ||
| ? [] | ||
| : originalPrompt.filter((block) => block.type !== 'text'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This new submit_prompt branch forwards the user's non-text blocks (attachments) to the model for any resolved command that is not CommandKind.BUILT_IN. That makes a pre-existing gap in the transcript record reachable. When a custom command or skill named advisor shadows the built-in — a configuration this codebase supports and tests (the advisor shadowing case in Session.test.ts) — Session.prompt skips its early user-message record for advisor (!isSlashInput || slashCommandName !== 'advisor', Session.ts:5341) and uses the deferred /advisor record instead (Session.ts:5435-5451), which calls recordUserMessage without the ...(attachmentReferences ? { attachmentReferences } : {}) the early site passes (Session.ts:5352-5357). So the model receives the image through this branch while the persisted user message carries no attachment references; on reload/resume, transcript-replay projects attachment chips only from payload.attachmentReferences (transcript-replay.ts:717-729), so the message renders as text alone even though the turn used the image. Before this PR the client discarded attachments for every /-prefixed prompt, so attachmentReferences was always undefined here and the omission was inert — this PR is what makes it live.
Witness:
probe (scratch tree, HEAD 103e611) — handleSlashCommand resolves `advisor` as CommandKind.FILE;
prompt '/advisor check my work' + image block; _meta attachmentReferences = [image.png]:
INTACT PR : recordUserMessage = [ '/advisor check my work' ] ← one positional arg, no options → no attachmentReferences
firstSentMessage = [ inlineData(image/png), text ] ← the model DID receive the image
FLIP (mirror the early site's attachmentReferences at the deferred site):
recordUserMessage = [ ..., { displayText, hookContext, attachmentReferences:[{type:image,attachmentId:image.png,...}] } ]
vitest -t 'advisor' → 5 passed
The fix belongs at the deferred /advisor record site (Session.ts:5435-5451), not on this line — read attachmentReferences once above both record sites and forward it in the deferred branch too:
const attachmentReferences = readDaemonAttachmentReferences(
promptMetadata?.[DAEMON_ATTACHMENT_REFERENCES_META_KEY],
);
recorder?.recordUserMessage(promptText, goalTurn?.permit, {
displayText: promptDisplayText ?? promptText,
hookContext: '',
...(attachmentReferences ? { attachmentReferences } : {}),
});The deferred site is reached only when slashCommandName === 'advisor' && shouldRecordSlashCommand, where shouldRecordSlashCommand is !(resolvedCommandInfo?.kind === CommandKind.BUILT_IN && resolvedCommandInfo.name === 'advisor'), so the fix must add references only for the non-BUILT_IN shadow and leave the real built-in /advisor record shape unchanged.
Please add a Session.test.ts case beside the new 'keeps attachments when a %s expands into a model prompt' tests where handleSlashCommand resolves advisor as CommandKind.FILE and the prompt carries an attachment reference in _meta, asserting recordUserMessage was called with expect.objectContaining({ attachmentReferences: [...] }), and confirm it goes red when the deferred-site references are removed.
中文说明
这个新的 submit_prompt 分支会把用户的非文本块（附件）转发给模型，只要解析出的命令不是 CommandKind.BUILT_IN。这使得 transcript 记录中一个既有的缺口变得可达。当名为 advisor 的自定义命令或技能遮蔽内置命令时（本代码库支持并测试了这种配置，见 Session.test.ts 的 advisor 遮蔽用例），Session.prompt 会跳过对 advisor 的早期用户消息记录（!isSlashInput || slashCommandName !== 'advisor'，Session.ts:5341），改用延迟的 /advisor 记录（Session.ts:5435-5451），而后者调用 recordUserMessage 时没有传入早期记录点会传的 ...(attachmentReferences ? { attachmentReferences } : {})（Session.ts:5352-5357）。于是模型通过本分支收到了图片，而持久化的用户消息不携带附件引用；重新加载/恢复会话时，transcript-replay 仅从 payload.attachmentReferences 投影附件标记（transcript-replay.ts:717-729），消息便只显示文本，尽管该轮确实用到了图片。本 PR 之前客户端会对所有以 / 开头的 prompt 丢弃附件，因此这里的 attachmentReferences 始终为 undefined、缺口不会显现——是本 PR 让它变得可达。
证据（探针，HEAD 103e611 的 scratch tree）：将 advisor 解析为 CommandKind.FILE、prompt 为 '/advisor check my work' 且带图片、_meta 附件引用为 [image.png] 时——未修改的 PR：recordUserMessage 只收到一个位置参数（无 options，即无 attachmentReferences），而 firstSentMessage 含 [inlineData(image/png), text]（模型确实收到了图片）；将早期记录点的 attachmentReferences 镜像到延迟记录点后：recordUserMessage 带上 {displayText, hookContext, attachmentReferences:[...]}，vitest -t 'advisor' 5 项通过。
修复应落在延迟的 /advisor 记录点（Session.ts:5435-5451），而非本行——在两处记录点上方统一读取一次 attachmentReferences，并在延迟分支中也透传（代码见英文部分）。延迟记录点仅在 slashCommandName === 'advisor' && shouldRecordSlashCommand 时到达，其中 shouldRecordSlashCommand 为 !(resolvedCommandInfo?.kind === CommandKind.BUILT_IN && resolvedCommandInfo.name === 'advisor')，因此修复只应为非 BUILT_IN 的遮蔽命令补充引用，保持真正的内置 /advisor 记录形态不变。请在 Session.test.ts 中仿照新增的 'keeps attachments when a %s expands into a model prompt' 用例补充一个测试：handleSlashCommand 将 advisor 解析为 CommandKind.FILE、prompt 的 _meta 携带附件引用，断言 recordUserMessage 被调用时带有 expect.objectContaining({ attachmentReferences: [...] })，并确认移除延迟记录点的引用后该测试变红。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
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: 2064 passed · 0 failed · 2064 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2064 通过 · 0 失败 · 2064 总计 抖动门： Verification reportPR #10730 — fix(web-shell): preserve slash command attachmentsVerdict: 中文摘要
Central claim + A/BCentral claim: attachments (images/files) survive to the model when a slash command expands into a model prompt (skill / custom / MCP / workflow / unresolved), while built-in control commands still discard them — enforced at two layers: the Web Shell upload gate ( Layer 1 — runtime (
|
| cell | build | keeps-attachments (skill) | keeps-attachments (custom/FILE) | built-in discards | oracle |
|---|---|---|---|---|---|
| base | 7df5ac6 | red | red | green (trivial) | expected [ { text: 'Expanded skill prompt' } ] to deeply equal [ …(4) ] — the @attachment:///…, inlineData and File: parts are absent |
| head | 103e611 | green | green | green | 3/3 |
Base's submit_prompt case returned normalizePartList(result.content) and ignored originalPrompt entirely, so the two "keeps" tests are the load-bearing flip; the built-in test passes on base trivially (base discarded everything) and pins head against over-forwarding (killed by mutant R2).
Layer 2 — frontend (packages/web-shell/client/daemon/session/actions.ts)
harness-frontend.mjs extracts the head function verbatim (only TS annotations stripped) and runs it against the verbatim base predicate text.trimStart().startsWith('/') over 24 inputs × 4 command-snapshot fixtures. Witness: evidence/01-frontend-ab-matrix-head-vs-base.png. 27/27 green, including:
- central flips:
/price-sheet …,/qc-helper …,/unlisted …,//…,/*…,/var/log/…,/go discard(base)→preserve(head); - invariants:
/compress, alias/summarize,/ compressstay discard on both arms; plain text preserve on both; - fail-closed: unknown
/…before a fully-loaded snapshot (nobuiltin-commandentry yet) discards on head, same as base; - monotonicity: no input flips preserve→discard from base to head.
Same-file A/B with the PR's own tests: head 203/203 green; base 11 behavioral reds (expected "spy" to be called once, but got 0 times — base never uploads for / text) and 5 trivially-green new tests (the no-upload assertions base satisfies by discarding everything). Witness: evidence/03-webshell-ab-base-vs-head.png.
Cross-layer oracle (frontend gate vs the CLI's own isSlashCommand/parseSlashCommand, compiled dist)
For every matrix input, the harness compared the frontend decision against what the daemon would do with the same first text block (untrimmed). Case-sensitivity, whitespace-after-slash, and primary-name-over-alias precedence all agree between the two layers. Exactly two inputs are dangerous (frontend drops the upload, daemon path keeps attachments):
| input | divergence | reachability | status |
|---|---|---|---|
' /compress' (leading whitespace) |
frontend discard; daemon isSlashCommand false → plain prompt keeps |
unreachable via UI: composer trims (ChatPane.tsx const trimmed = text.trim() before sendPrompt) and queued prompts store trimmed (useQueuedPrompts.ts text: annotated?.displayText ?? trimmed) |
pre-existing (base discarded it too); note |
'/price-sheet x' before first available_commands_update |
frontend fail-closed discard; daemon resolves skill and keeps | narrow real race: sendAvailableCommandsUpdate is fire-and-forget at session activation and the snapshot builds async |
pre-existing (base discarded all / input); finding #1 |
Corrections
- The PR body says "Before command metadata is available, unknown commands retain attachments to avoid silent data loss." The code does the opposite in the pre-load window:
return !connection.commands?.some(builtin)discards when no builtin entry has arrived yet (the code comment labels this "fail closed"). The body sentence is accurate only for the loaded-but-unlisted case (/unlistedkeeps). This is a correction to the description, not a request to change code — the fail-closed choice is deliberate and matches the new tests.
Findings
- Suggestion (pre-existing, strictly narrowed by this PR): silent attachment loss in the pre-metadata window. A skill command plus image submitted between connection and the first
available_commands_updateis discarded by the frontend gate and never reaches the runtime gate the PR built. Repro shape:actions.submitPrompt('/price-sheet x', {images:[…]})withconnection.commandsundefined/empty/skills-only →uploadAttachmentnot called (pinned by the PR's own two "before command metadata loads" tests, which assert the discard). Base discarded every/prompt, so the PR strictly shrinks the loss surface; the residual window is narrow (snapshot fires at session activation) but real. If the author wants zero silent loss, the fail-closed branch could upload-and-let-runtime-decide (wasted upload for built-ins) or the composer could hold submissions until the snapshot arrives; both tradeoffs are defensible, hence non-blocking. - Note (pre-existing): leading-whitespace classifier divergence — see table above; unreachable through the shipped UI on both arms.
- Completeness: the empty-name guard is unpinned. Mutant M6 (delete
if (!name) return false;) survives all 158 actions tests, but the in-memory matrix census shows it decides 6 cells (/,/,/\t× UNLOADED/EMPTY): with the guard these preserve, without it they discard. Behavior is correct as shipped; nothing asserts it. A test forsubmitPrompt('/', …)before metadata loads would pin it. Coverage gap, not a defect.
Mutation matrix
Witness: evidence/04-mutation-matrix.png (rows re-verified live per mutant; raw logs mutant-*.log). Positive controls landed in the same file as each mutant and went red (M1 1 red, P1 1 red, R1/R2 red), proving each suite collects the mutated file.
| mutant | guard | suite | result |
|---|---|---|---|
M1 remove //+/* guard |
comment classifier | actions.test.ts | KILLED (1 red: /* crash note */; // still caught by the path-separator guard → redundant defence for //, load-bearing for /*) |
| M2 remove path-separator guard | /var/… classifier |
actions.test.ts | KILLED (2 red) |
| M3 alias-lookup-first | primary-over-alias precedence | actions.test.ts | KILLED (1 red) |
| M4 fail-open before metadata | fail-closed polarity | actions.test.ts | KILLED (5 red: 2 wrong-case + 2 pre-metadata + 1 unknown-after-load) |
| M5 case-insensitive name match | case-sensitive lookup | actions.test.ts | KILLED (1 red: /Compress) |
| M5b case-insensitive alias match | case-sensitive alias lookup | actions.test.ts | KILLED (1 red: /SUMMARIZE) |
| M6 remove empty-name guard | if (!name) |
actions.test.ts | SURVIVED → coverage gap (finding 3) |
| P1 remove altNames mapping | mappers.ts | mappers.test.ts | KILLED (1 red) |
| R1 revert hunk (= base) | whole runtime change | Session.test.ts | KILLED (2 red) — same as A/B base cell |
| R2 drop BUILT_IN condition | built-in gate | Session.test.ts | KILLED (1 red) |
| R3 always discard | keep-path | Session.test.ts | KILLED (2 red) |
| R4 swap attachment/expansion order | part ordering | Session.test.ts | KILLED (2 red) |
11/12 killed; the single survivor is classified (coverage gap), and M1's // row is classified redundant defence (sibling guard closes the same hazard), so no unexplained survivor remains.
Targeted gates
packages/cliSession.test.tsfull file at head: 810/810 (562 s).packages/web-shellchanged files (actions.test.ts,mappers.test.ts) at head: 203/203.packages/web-shellfull suite at head: 5628 passed / 47 failed across 14 UI-component files. Attribution: none of the failing files is touched by the PR; all 14 pass in isolation at head (131 + 331 + 195 = 657 tests green) and the two largest clusters also pass on base (A/A, 131 green) — load-induced flakiness on this shared runner, not a regression. Excluded fromassertions.jsonas a broad sweep; the attribution cells (788 checks) are counted instead.- Typecheck:
packages/web-shellandpackages/cliboth exit 0.
Not covered
- Reviewer Test Plan browser steps (
npm run dev:daemon+ real browser + model): no browser or model credentials in this container. The display half (displayedImages/appendLocalUserMessage) is pinned by the PR'suploads attachments used by skill slash commandstest; the model-receipt half by the runtime A/B. This reproduces the classification and wire behavior, not the browser-rendered transcript. - Runtime cell for
CommandKind.MCP_PROMPTand stacked-skill (resolvedCommandundefined): same!== BUILT_INbranch as the covered SKILL/FILE cells; not asserted individually. - Per-commit attribution: shallow checkout exposes 1 of the snapshot's 6 commits; verified the aggregate
HEAD^1..HEADdiff only. - Windows/Linux browser runs (author's own declared gap).
- The 47 full-suite flakes themselves (see gates; disproven, not fixed).
Methodology
Environment: CI merge-ref checkout (HEAD fd4cc042, base HEAD^1 7df5ac68, PR head HEAD^2 103e6117), node 22.23.2, prebuilt dist/. A/B base cells ran in tmp/base-tree (worktree at HEAD^1) with head's test files copied in; unchanged workspace dist/ and per-package node_modules were symlinked from the main tree (PR touches no dependency or those packages — asserted git diff --name-only empty for them), while the changed files load from the worktree's own source. The frontend matrix executes the verbatim head function extracted from actions.ts and the verbatim base predicate from git show HEAD^1; the cross-layer oracle imports the compiled parseSlashCommand/isSlashCommand from packages/cli/dist. Mutations ran in tmp/mutant-tree (worktree at HEAD), each applied once, suite run, file restored via git checkout -- (tree verified clean after). Raw logs 02-…13-*.log, harnesses harness-frontend.mjs, harness-m6.mjs, harness-mutations.mjs, captures in evidence/. Worktrees removed after capture.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/acp-integration/session/Session.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.test.ts
file packages/web-shell/client/daemon/session/actions.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/actions.test.ts
file packages/web-shell/client/daemon/session/mappers.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/mappers.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/session/Session.test.ts: PPPP
packages/web-shell/client/daemon/session/actions.test.ts: PPP
packages/web-shell/client/daemon/session/mappers.test.ts: PPP
verdict: timeout
summary: only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/session/Session.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
Collaborator
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Collaborator
Verification report — real browser + real daemon (head
|
| Input (image pasted in every row) | Image in message | attachmentReferences in JSONL |
Reached the model |
|---|---|---|---|
/qc-helper what is in the image (skill) |
✅ | image.png |
✅ image_url, 758 B data URL |
/price-sheet … (custom command) |
✅ | image.png |
✅ image_url |
/price-sheet … + notes.txt |
✅ | resource notes.txt |
✅ @attachment:///notes.txt + File: …RIG_FILE_MARKER_ALPHA… |
/compress (built-in) |
❌ | none | ❌ |
/summarize (built-in alias) |
❌ | none | ❌ |
/ compress (space after slash) |
❌ | none | ❌ — and the daemon still resolved it to compress, so the lexical gate matches parseSlashCommand |
/var/log/app.log shows this crash (not a command) |
✅ | image (1).png |
✅ image_url |
The attachment parts arrive before the expanded prompt in the model request, matching [...attachmentParts, ...expandedPrompt].
2. Bug reproduced on base, and both hunks are individually load-bearing
I ran four arms crossing the two halves of the diff:
| Arm | web-shell | Session.ts |
/qc-helper + image → what happens |
|---|---|---|---|
| A | base | base | image lost on all three oracles — the reported bug |
| B | head | head | image displayed, recorded, and delivered — fixed |
| C | base | head | the prompt reaches the daemon with no attachment block, so the fixed runtime has nothing to forward |
| D | head | base | image displayed ✅, JSONL records image.png ✅, model receives nothing ❌ |
Arm D is the interesting one: with only the frontend half applied, the UI and the transcript both claim the attachment was sent while the model never sees it. That is a strictly worse failure mode than today's, so the two hunks must ship together — which they do here.
3. Finding F1 (Suggestion, not a blocker) — the fail-closed window is reachable, and it is not only "unknown" commands
The PR's Risk section says unresolved slash commands fail closed before the command snapshot loads. I reproduced that on the head build, end to end, for a known custom command:
- Same page, same session, same command, same image.
- 1st prompt, submitted as soon as the composer mounts:
/price-sheet RACE1 update these prices→ no image in the message, noattachmentReferences, noimage_urlin the model request. - 2nd prompt, after the snapshot arrived:
/price-sheet WARM update these prices→ all three oracles green.
The window is usually sub-second, but it is not always: in one run — the first page load after a daemon restart — the drop still happened after well over 10 s of interaction. Worth knowing:
- N1:
mapWorkspaceSkills()inclient/daemon/session/mappers.ts— whose doc comment describes it as exactly this session-less bootstrap ("before any session exists the only way to populate skill-backed slash commands") — has no production caller in this tree; onlymappers.test.tsimports it. Pre-existing (from refactor(vscode-ide-companion): migrate chat to WebShell and qwen serve #9811), untouched by this PR, but it is the reasonconnection.commandsisundefineduntil the firstavailable_commands_updateinstead of being pre-seeded. Wiring it up (or adding a session-less command catalog) would shrink this window for real.
Why this is not a merge blocker: on base, every slash-prefixed prompt loses attachments, so this window is strictly narrower than today's behavior — the PR is a large net improvement with no regression. Also note the PR's own reviewer test plan drives /qc-helper, a skill, with human-speed typing, so it will basically never surface this; a follow-up test that submits a custom command in the cold window would pin it.
4. Suites re-run locally at this head
packages/web-shell:actions.test.ts+mappers.test.ts+promptContent.test.ts→ 215/215 pass (158 inactions.test.ts).packages/cli:Session.test.ts -t attachment→ 6 pass, 804 skipped.
Verdict
Behavior verified end-to-end in a real browser against a real daemon: the fix does what the description claims, both hunks are necessary, the built-in and built-in-alias exclusions hold, and no regression appeared in any arm. From my side this is merge-ready; F1/N1 are follow-ups, not gates.
中文说明
验证报告 —— 真实浏览器 + 真实 daemon（head 103e611）
维护者验证。与 @qqqys 的报告互补：他覆盖了静态审查、单测/变异，以及经 HTTP/ACP 接口驱动的 daemon A/B；我这边驱动的是真实 Chrome 里的 Web Shell 界面——本 PR 的大部分改动在 actions.ts/mappers.ts，接口层驱动根本走不到——并且把前端那一半和 runtime 那一半拆开单独验证。
验证台：103e611 的独立 worktree，完整 npm run build，真实 qwen serve，真实 Chrome，隔离 HOME。模型侧是自写的假 OpenAI 网关，逐条落盘请求体；模型 id 用 qwen-vl-max，这样 modalities.image 为真，图片会转成真正的 image_url data URL 而不是 [image: image/png] 占位符。工作区放了 .qwen/skills/qc-helper/SKILL.md（技能）、.qwen/commands/price-sheet.md（自定义命令），加上内置 /compress 及其别名 /summarize。图片/文件通过真实的 paste ClipboardEvent 投进输入框，走的就是用户那条 onPasteCapture。每个用例用三个独立判据：消息里是否渲染出图、会话 JSONL 的 attachmentReferences、网关抓到的模型请求字节。每次测量都记录臂的身份：页面读出的 bundle 哈希（index-C5U-Hv1k.js=head，index-CQPmRIo5.js=base）与 Session.js 里 attachmentBlocks 出现次数（3=head，0=base）。base 取 HEAD^2（7df5ac68），它与 HEAD 的差异恰好只有本 PR 的 8 个文件。假 provider 的两条 Model 'qwen-vl-max' not found toast 是验证台噪音。
1. head 的行为与 PR 描述完全一致：技能命令、自定义命令、自定义命令+文本文件三种情况下图片/文件都保留（模型侧分别拿到 758 字节的 image_url，以及 @attachment:///notes.txt + 文件正文）；内置 /compress、内置别名 /summarize、以及斜杠后带空格的 / compress 三种情况都正确丢弃，且 / compress 在 daemon 侧依然被解析成 compress，说明前端词法门与 parseSlashCommand 对齐；非命令的 /var/log/app.log … 保留图片。附件块排在展开 prompt 之前，与 [...attachmentParts, ...expandedPrompt] 一致。
2. base 复现缺陷，且两处改动各自都是承重件：四条臂交叉验证——A（base/base）三个判据全丢＝复现缺陷；B（head/head）全绿＝修复；C（前端 base + runtime head）prompt 到达 daemon 时已不带附件块，修好的 runtime 无物可转；D（前端 head + runtime base）最关键：界面显示了图、JSONL 也记了 image.png，但模型什么都没收到。也就是说只上前端那一半会得到一个比现状更糟的失败形态（界面和会话记录都在"撒谎"），两处改动必须一起上——本 PR 正是如此。
3. 发现 F1（Suggestion，非阻断）：PR 风险段说的"快照就绪前 fail closed"我在 head 上端到端复现了，而且不止于"未知命令"——对一个已知的自定义命令同样发生：同一页面、同一会话、同一命令、同一张图，输入框一挂载就提交的 /price-sheet RACE1 … 三个判据全丢；快照到达后的 /price-sheet WARM … 三个判据全绿。这个窗口通常在一秒内，但并非总是如此：有一轮（daemon 重启后的首次开页）交互了远超 10 秒仍然丢。相关的 N1：mappers.ts 里的 mapWorkspaceSkills()——它的文档注释写的正是这个"会话前引导"用途——在本树里没有生产调用方，只有 mappers.test.ts 引用它；这是 #9811 遗留的、本 PR 未触及的问题，但它正是 connection.commands 在首个 available_commands_update 之前一直是 undefined 的原因。把它接上（或提供一个无会话的命令目录接口）才能真正收窄这个窗口。
为什么不是合并阻断：base 上任何以 / 开头的 prompt 都会丢附件，所以这个窗口严格窄于现状，本 PR 是净改善且未引入回归。另外 PR 自带的 Reviewer Test Plan 用的是 /qc-helper（技能）加人手输入速度，基本不可能撞上；补一条"冷窗口内提交自定义命令"的用例可以把它钉住。
4. 本地复跑套件：packages/web-shell 的 actions.test.ts + mappers.test.ts + promptContent.test.ts 215/215 通过；packages/cli 的 Session.test.ts -t attachment 6 个通过、804 跳过。
结论：真实浏览器 + 真实 daemon 端到端验证通过，描述属实，两处改动缺一不可，内置命令与内置别名的排除生效，四条臂均未出现回归。我这边认为可以合并；F1/N1 作为后续跟进，不构成门槛。
wenshao
approved these changes
Sep 2, 2026
Collaborator
|
@qwen-code /triage |
wenshao
enabled auto-merge
September 2, 2026 12:50
Collaborator
|
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: 9492 passed · 0 failed · 9492 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：9492 通过 · 0 失败 · 9492 总计 抖动门： Verification reportPR #10730 — fix(web-shell): preserve slash command attachmentsVerdict: 中文 — 判定：✅ 通过 · 可合入（agent 判定）第 2 轮验证。本轮 head / base 与第 1 轮完全相同（同一提交，树哈希一致），所有携带结论均在新容器中重新执行而非引用旧报告，且全部复现。
未覆盖范围与第 1 轮相同（真浏览器/模型步骤、逐 commit 归因等），详见 Not covered。 Previous-finding status (round 1 → this round)
Central claim + A/BCentral claim: attachments (images/files) survive to the model when a slash command expands into a model prompt (skill / custom / MCP / unresolved), while built-in control commands still discard them — enforced at two layers: the Web Shell upload gate ( Layer 1 — runtime (
|
| cell | build | oracle (scripted) | result |
|---|---|---|---|
| base | 7df5ac68 + head's test file |
3 filtered tests must classify: skill/custom keep, built-in discard | 2 behavioral red — AssertionError: expected [ { text: 'Expanded skill prompt' } ] to deeply equal [ …(4) ] (the @attachment:///…, inlineData and File: parts are absent); built-in test trivially green (base discarded everything) |
| head | 103e6117 |
full Session.test.ts |
810/810 green (includes the 3 new tests, exit 0) |
Vacuity: mutant R1 (revert the exact PR hunk ⇒ base behavior) makes the two "keeps" tests fail the intended behavioral assertion with expected-vs-actual output — the tests are load-bearing, not vacuous. Round-1's two Not covered runtime cells are now covered by scratch probes at head (capture evidence/06-runtime-kind-probes.png): CommandKind.MCP_PROMPT keeps, and a stacked-skill submit_prompt with no resolvedCommand keeps — 5/5 green. CommandKind has exactly 4 values (types.ts:312-317), so the runtime gate is now cell-complete.
Layer 2 — Web Shell frontend (packages/web-shell/client/daemon/session/actions.ts, mappers.ts)
Witness: evidence/03-webshell-ab-base-vs-head.png. Raw: 05-webshell-head.log, 06-webshell-base.log.
| cell | build | oracle (scripted) | result |
|---|---|---|---|
| base | 7df5ac68 + head's test files |
203 tests must classify | 11 behavioral red (expected "spy" to be called once, but got 0 times — base never uploads for / text) / 192 green; the other 5 new tests are trivially green on base (no-upload assertions base satisfies by discarding everything) |
| head | 103e6117 |
actions.test.ts + mappers.test.ts |
203/203 green (158 + 45) |
Layer 3 — classification matrix + cross-layer oracle
Witness: evidence/01-frontend-ab-matrix-head-vs-base.png. Harness: harness-frontend.mjs (runs the head function extracted verbatim from actions.ts — only the TS signature annotations stripped — against the verbatim base predicate text.trimStart().startsWith('/'), which the harness proves occurs exactly 3× in the HEAD^1 blob). 141/141 scripted checks green over 24 inputs × 4 snapshot fixtures:
- Central flips (
/price-sheet …,/qc-helper …,/unlisted …,//…,/*…,/var/log/…,/var\log\…,/,/,/\t,//…): discard(base)→preserve(head), 40 cells total (flip census asserted). - Invariants:
/compress, alias/summarize,/ compress,/help,/rememberstay discard on both arms; plain/empty/no-slash text stays preserve (monotonicity asserted for all 96 cells: nothing base preserved became discarded). - Fail-closed window: unknown
/…before any builtin entry arrives (unloaded / empty / skills-only snapshots) discards on head — same as base; pinned by the PR's own two pre-metadata tests. - Cross-layer oracle (compiled
isSlashCommand/parseSlashCommandfrompackages/cli/dist, fed the untrimmed first text block exactly asSession.ts:5275-5281does): at the loaded snapshot, head's frontend agrees with the daemon on 23/24 inputs; the single lexical disagreement is' /compress'(daemon never trims). Dangerous census (frontend drops the upload while the daemon path would keep attachments): F0/F1 = 7 inputs, F2 = 5, F3 = 1 — partitioned into 16 reachable fail-closed-window cells (finding 1) and 4 UI-unreachable whitespace cells (finding 2).
Corrections
- Round 1 corrected a PR-body sentence that said unknown commands retain attachments before command metadata loads while the code fails closed. The body has since been corrected by the author and now matches the code. Nothing further to correct; this is a status note, not a request to change code.
Findings
Ordered by severity; none blocking. All three numbered findings are carried from round 1 and re-measured (status table above).
- Suggestion (pre-existing, strictly narrowed by this PR): silent attachment loss in the pre-metadata window. A prompt the daemon would resolve to a non-built-in command or no command (skill/custom/unknown/wrong-case) loses its attachments if submitted between connection and the first
available_commands_updatethat carries a builtin entry — 16 reachable cells in the matrix census. Repro shape:actions.submitPrompt('/price-sheet x', {images:[…]})withconnection.commandsundefined/empty/skills-only ⇒uploadAttachmentnot called (pinned by the PR's own tests, which assert the discard). Base discarded every/prompt, so the PR strictly shrinks the loss surface; the residual window is narrow (snapshot fires at session activation) but real. Candidate fixes (upload-and-let-runtime-decide, or hold submissions until the snapshot) are tradeoffs, hence non-blocking. - Note (pre-existing): leading-whitespace classifier divergence —
' /compress'is classified built-in-discard by the frontend but is a plain prompt to the daemon. Unreachable through the shipped UI on both arms (trim sites cited in the status table); the other programmatic send paths (SideTaskPanelinitial prompt, App command insertion) are machine-generated text. - Completeness: the empty-name guard is unpinned. Mutant M6 survives all 158 actions tests; the census shows it decides 9 cells. A test for
submitPrompt('/', …)before metadata loads would pin it. Coverage gap, not a defect. - Note (new, sibling sweep): failed skill turns with attachments are excluded from "retry last prompt".
App.tsx:7068still classifies viapreparedPrompt.trimStart().startsWith('/')for retry bookkeeping, so slash turns are never recorded as retryable. Pre-existing and orthogonal to attachment loss (the submitted turn itself is unaffected; the exclusion reads as deliberate for side-effecting control commands), but the PR raises the stakes of the exclusion by giving skill commands attachments. No action required for this PR.
Mutation matrix
Witness: evidence/04-mutation-matrix.png. Raw: mutant-*.log, driver run-mutations.sh, patcher apply-mutant.mjs (each mutant asserted to match its target exactly once). Unmutated controls green in the same invocation: actions.test.ts 158/158, mappers.test.ts 45/45; runtime mutants ran against head Session.test.ts filtered to the attachment tests.
| mutant | guard | suite | result |
|---|---|---|---|
M1 remove //+/* guard |
comment classifier | actions.test.ts | KILLED (1 red: /* crash note */; // still caught by the path-separator guard → redundant defence for //, load-bearing for /*) |
| M2 remove path-separator guard | /var/… classifier |
actions.test.ts | KILLED (2 red: /var/log…, /var\log…) |
| M3 alias-lookup-first | primary-over-alias precedence | actions.test.ts | KILLED (1 red: prefers a primary command name over another command alias) |
| M4 inverted fail-closed polarity | fail-closed polarity | actions.test.ts | KILLED (5 red: wrong-case ×2, pre-metadata ×2, unknown-after-load ×1) |
| M5 case-insensitive name match | case-sensitive lookup | actions.test.ts | KILLED (1 red: /Compress) |
| M5b case-insensitive alias match | case-sensitive alias lookup | actions.test.ts | KILLED (1 red: /SUMMARIZE) |
| M6 remove empty-name guard | if (!name) |
actions.test.ts | SURVIVED → censused coverage gap (finding 3); positive control M1 red in the same file |
| P1 remove altNames mapping | mappers.ts | mappers.test.ts | KILLED (1 red: maps command aliases from available_commands_update metadata) |
| R1 revert hunk (= base) | whole runtime change | Session.test.ts | KILLED (2 red) — identical to the A/B base cell; failure message quotes expected-vs-actual |
| R2 drop BUILT_IN condition | built-in gate | Session.test.ts | KILLED (1 red: built-in test) |
| R3 always discard | keep-path | Session.test.ts | KILLED (2 red) |
| R4 swap attachment/expansion order | part ordering | Session.test.ts | KILLED (2 red — the keeps tests assert exact part order) |
11/12 killed; every kill attributed to its intended test by name; the single survivor is classified by census (coverage gap), and M1's // half is classified redundant defence — no unexplained survivor.
Targeted gates
packages/cliSession.test.tsat head: 810/810, 3 consecutive rounds (gate + 2 flakiness rounds), all exit 0. Coverage collection disabled for these runs (--coverage.enabled=false) — test outcomes are unaffected; this only removes post-run coverage work.packages/web-shellchanged files (actions.test.ts,mappers.test.ts): 203/203, 5 consecutive rounds, all exit 0.packages/web-shellfull suite at head: 5675 passed / 0 failed across 252 files, exit 0. Round 1's 47 load-induced flakes in 14 untouched UI-component files did not reproduce this round — consistent with (and reinforcing) round 1's environmental attribution.- Typecheck:
packages/web-shellexit 0,packages/cliexit 0. - Flakiness gate summary:
Session.test.tsPPP (3 rounds × 810),actions.test.ts+mappers.test.tsPPPPP (5 rounds × 203). All rounds agreed; no intermittent failure of either kind (random or speed-correlated) observed.
Not covered
- Reviewer Test Plan browser steps (
npm run dev:daemon+ real browser + real model): no browser or model credentials in this container. The classification half is pinned by the matrix and the PR's tests, the upload/wire half by the web-shell A/B, the model-receipt half by the runtime A/B and kind probes. This reproduces the classification and wire shape, not the browser-rendered transcript. - Per-commit attribution: the shallow checkout exposes 1 of the snapshot's 6 commits (
git rev-list HEAD^1..HEAD^2= 1 vs 6 in$QWEN_VERIFY_CONTEXT); verified the aggregateHEAD^1..HEADdiff only. - Windows/Linux browser runs (author's own declared gap).
- Repo-wide test/lint gates: not run (out of scope for the affected workspaces).
Methodology
Environment: CI merge-ref checkout (HEAD fd4cc042, base HEAD^1 = 7df5ac68, PR head HEAD^2 = 103e6117 — both OIDs identical to round 1), node v22.23.2, prebuilt dist/, CI=true. Base A/B ran in tmp/base-tree (worktree at HEAD^1) with head's three changed test files copied in; control purity: packages/cli/vitest.config.ts aliases every @qwen-code/* import to sources relative to the config file, so the base run loads base-tree sources for all workspace code; the only head-tree borrow was third-party node_modules (symlinked) plus dist/generated prerequisites demanded by the vitest globalSetup guard — every borrowed package is inside the PR's provably-unchanged closure (git diff --stat HEAD^1..HEAD -- packages/core packages/channels packages/acp-bridge packages/sdk-typescript packages/web-templates empty). Mutations ran in a second scratch worktree at HEAD, one mutant per iteration, each asserted to match exactly once, suite run, file restored via git checkout -- (tree verified clean after); probe tests for MCP_PROMPT/stacked-skill were scratch-only and never touched the PR tree. The cross-layer oracle imports the compiled isSlashCommand/parseSlashCommand from packages/cli/dist/src/…. Assertion counting is at test/cell granularity (each vitest test or harness check = one scripted oracle; expected base-arm reds count as passes per the expected-failure rule). Raw logs: 02-…15-*.log; harnesses: harness-frontend.mjs, harness-m6-census.mjs, apply-mutant.mjs, run-mutations.sh; captures in evidence/. Worktrees removed after capture.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/acp-integration/session/Session.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.test.ts
file packages/web-shell/client/daemon/session/actions.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/actions.test.ts
file packages/web-shell/client/daemon/session/mappers.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/mappers.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/session/Session.test.ts: PPPP
packages/web-shell/client/daemon/session/actions.test.ts: PPP
packages/web-shell/client/daemon/session/mappers.test.ts: PPP
verdict: timeout
summary: only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/session/Session.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
approved these changes
Sep 2, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Tier: Standard — frontend attachment-filter logic + runtime command-kind gate; no persistent format, no auth, no wire-protocol change.
Prior-round critical findings — status at head 103e611
| ID | Original finding | Status |
|---|---|---|
| R1-1 | Frontend classifier was case-insensitive; daemon parseSlashCommand is case-sensitive — wrong-case built-ins discarded silently |
✅ Fixed: frontend now uses exact === match, mirrors parseSlashCommand at commands.ts:88–95 |
| R1-2 | Fail-open before snapshot loads — built-in submitted in unloaded window uploaded image the daemon would discard | ✅ Fixed: fail-closed when connection.commands is undefined or has no built-in entry |
| R1-2 fix-induced | Fail-closed fallback did not apply daemon lexical exclusions; // stack trace, /* crash */, /var/log/… lost attachments |
✅ Fixed: //, /*, and first-token //\ guards added; parameterized tests cover all four forms with commands absent |
| R1-3 | sendPrompt display/upload gate untested — partial revert would silently regress the primary interactive path |
✅ Fixed: classifies %s command attachments on the sendPrompt path test exercises both skip and keep paths |
| R1-4 | No test for unknown command keeping attachments after snapshot loads — flip to discard survives suite green (mutation-witnessed) | ✅ Fixed: uploads attachments for unknown commands after metadata loads added |
| R2-1 | discardsSlashCommandAttachments silently encoded two load-bearing invariants (lexical mirror, built-in-means-loaded) |
✅ Addressed: comment at definition site now names both invariants |
| R2-2 | No test for commands: undefined (genuine not-loaded state) |
✅ Fixed: does not upload a slash command before command metadata loads added |
What I checked
originalPromptscope: parameter of#processSlashCommandResultatSession.ts:13500— correctly scoped, receives full original prompt blocks ✓deferBridgeConversions: truesemantics: established pattern (existing call at line 8417);#resolvePromptreturns parts without bridge conversion;#applyBridgeConversionsIfNeededthen runs on the full[...attachmentParts, ...expandedPrompt]combined array ✓- Case-sensitivity alignment:
discardsSlashCommandAttachmentsuses exact-match===— consistent withparseSlashCommand's two-pass exact-match atcommands.ts:88–95✓ - Fail-closed correctness:
commands === undefined→true(discard); non-empty snapshot with ≥1 built-in → unknown command →false(upload) ✓ - Two-gate coverage: both
submitPromptandsendPromptpaths updated and tested ✓ altNamesmapping:mapCommandMetadefensively filters non-string entries; two-pass primary-name-before-alias lookup mirrors daemon ✓
Deferred / out of scope (no change expected)
- R1-7: Orphaned uploads for non-
submit_promptresults (MCP-prompthelp/error paths, policy-blocked commands) — author deferred to follow-up design; noted but not blocking - Advisor transcript gap (Session.ts:13518 suggestion): shadowed
/advisorpath uses deferred user-message recorder that lacksattachmentReferences— author deferred; attachment is still referenced in the recorded message, just not the structured field
Scope
Source only. NOT reviewed: CONTRIBUTING.md (network error on fetch — no AI ban found in this repo's prior PRs); CLI Session.test.ts suite (compile error on main from commands-registry.ts, unrelated to this PR — qwen-code-ci-bot confirmed actions.test.ts 165/165 and mappers.test.ts 45/45 green). useQueuedPrompts drain re-submit path not traced (would re-enter same gated action with same snapshot; low risk). Runtime attachment GC is outside this PR's scope.
Reviewed with AI assistance.
Collaborator
|
Released in v0.23.0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Preserves image and file attachments when a skill, custom command, MCP prompt, workflow, or unresolved slash command expands into a model prompt. Built-in control commands continue to discard attachments. Command aliases use the same classification as their canonical built-in command.
Why it's needed
Web Shell previously filtered attachments from every prompt beginning with
/, so a user's image could disappear before a skill or user-defined command reached the model. The frontend now avoids uploads only for commands advertised as built-in, while the session runtime uses the resolved command kind as the final authority before model submission.Reviewer Test Plan
How to verify
Start Web Shell with
npm run dev:daemon, paste an image, and submit it with a skill command such as/qc-helper. Confirm the image remains visible in the user message, the model receives it, and the transcript records an attachment reference. Then paste an image and submit/compress; confirm the built-in command message and transcript contain no image. Repeat the second check with a built-in alias such as/summarize.Evidence (Before & After)
Before: skill and custom slash commands silently lost all images and files in Web Shell. After: a real-browser
/qc-helperrun displayed the image, the model identified the Qwen logo, and the JSONL included animage.pngattachment reference; a subsequent/compressrun displayed and recorded only the command text.Tested on
Environment (optional)
macOS,
npm run dev:daemon, local Web Shell in a real browser. Full repository build passed. Focused Web Shell tests: 190 passed. ACP Session tests: 810 passed. Web Shell and CLI typechecks passed.Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
当技能、自定义命令、MCP prompt、workflow 或尚未识别的斜杠命令展开为模型 prompt 时，保留图片和文件附件。内置控制命令仍会丢弃附件；内置命令别名与其主命令使用相同分类。
为什么需要
Web Shell 之前会过滤所有以
/开头 prompt 的附件，导致用户图片在技能或自定义命令到达模型前消失。现在前端仅对明确标记为内置的命令跳过上传，session runtime 则在命令解析后依据最终命令类型决定是否向模型传递附件。Reviewer Test Plan
如何验证
使用
npm run dev:daemon启动 Web Shell，粘贴图片并通过/qc-helper等技能命令发送。确认用户消息中仍显示图片、模型收到图片且会话记录包含附件引用。然后再次粘贴图片并执行/compress，确认内置命令消息和会话记录中都没有图片。再使用/summarize等内置别名重复第二项验证。前后证据
修改前：技能和自定义斜杠命令会在 Web Shell 中静默丢失全部图片和文件。修改后：真机浏览器中的
/qc-helper会显示图片，模型正确识别出 Qwen Logo，JSONL 包含image.png附件引用；后续/compress仅显示和记录命令文本。测试平台
环境（可选）
macOS，
npm run dev:daemon，真实浏览器中的本地 Web Shell。完整仓库构建通过；Web Shell 聚焦测试 190 个通过；ACP Session 测试 810 个通过；Web Shell 和 CLI typecheck 通过。风险与范围
关联 Issue
N/A