fix(cli): Preserve mid-turn image messages - #5183
Conversation
Preserve image attachments and structured content when user messages are drained during tool execution. Keep text-only behavior compatible while preventing attachment-only or missing-base64 image messages from being acknowledged before they can be replayed safely. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
3 Critical findings (2 ESLint errors blocking CI, 1 logic bug) and 3 Suggestions. — qwen3.7-max via Qwen Code /review
Fix the lint blockers, preserve legacy mid-turn messages when structured items are empty, share the mid-turn prefix helper, and record mid-turn @ command reads without adding UI tool groups. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical re-check: the mid-turn image/message drain issues raised earlier are fixed at HEAD, and I found no remaining critical blockers in this pass.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review found no critical issues. 3 suggestions below. Downgraded from Approve to Comment: CI still running. — qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code review did not complete successfully: Qwen review timed out after 85 minutes. See workflow logs. |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical re-check after the latest mid-turn hardening commit: the previously reviewed image/message drain issues remain fixed, and I found no new critical blocker in the incremental diff.
Bound mid-turn @ resolution with a per-message timeout and surfaced attachment failures to the user before falling back to raw text. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review of latest commit. Previous Critical findings (batch error handling, orphan AbortController) are resolved — the per-message try/catch and withTimeoutSignal(abortSignal, ...) correctly address both concerns. Remaining findings below are Suggestions.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
One additional finding on the Gemini drain loop (supplementing the 12 comments already posted). — Qwen Code /review
Surface mid-turn at-command tool displays, validate ACP inline media MIME prefixes, and add focused coverage for mid-turn message prefixing. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Stop processing drained mid-turn messages once cancellation aborts at-command resolution, avoiding stale fallback notifications and recordings. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Automated Review — PR #5183 (fix(cli): Preserve mid-turn image messages)
All 297 tests pass (CLI + Desktop). TypeScript and lint checks clean.
This PR extends the ACP mid-turn drain to carry structured content blocks (text, image, audio, resource) alongside the legacy text-only messages format, and lets the desktop client forward image attachments mid-turn. The overall design is sound — the structured items path cleanly separates from the legacy messages path, the abort/timeout plumbing is careful, and the resolveAtCommandQuery extraction is a clean refactor.
I found 1 critical issue and 3 suggestions. Inline comments below.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary (Cross-repo lightweight mode)
This PR extends the mid-turn message pipeline to carry structured image/attachment content through CLI ACP, useGeminiStream, atCommandProcessor, and desktop paths. The overall design is sound — the separation between text-only (messages) and structured (items) drain responses, the canOfferMidTurnAttachments gate, and the isContentBlock validator all work together coherently.
The existing review comments from @doudouOUC and the CI bot cover the substantive issues thoroughly. Below is one additional finding from an independent analysis pass.
Reviewed by: qwen3.7-max
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Maintainer local verification — real test build under tmux (red/green + typecheck)I built a worktree at the PR head and ran the actual test suites under Verdict: verified locally. All changed suites pass on all three surfaces; the new tests genuinely guard the fix (clean red/green); types are clean; there are no merge conflicts. The only hard blocker is pending CI. The single open review thread is a non-blocking performance suggestion. Environment
1) Changed suites — GREEN on PR code
CLI total 271 passed; desktop 62 passed / 0 failed. 2) Red/green — the new tests actually guard the fixReverted only the impl files to baseline
So the failures map 1:1 to the PR's new behavior (mid-turn Exact failing tests on the pre-fix baseline (21 total)CLI —
CLI —
CLI — Desktop —
Desktop —
3) Typecheck — clean
4) Cross-surface interop + security (code-read & test-confirmed)
5) Merge readiness
Scope / caveats
Net: ready to merge once CI is green; the single open thread is an optional perf follow-up. 🇨🇳 中文版（点击展开）维护者本地验证 — tmux 下的真实测试构建（red/green + 类型检查）我在 PR head 上建了 worktree，在 Linux 下用 结论：本地验证通过。 三个改动面（CLI 环境
1) 改动套件 — PR 代码上全绿
CLI 合计 271 通过；desktop 62 通过 / 0 失败。 2) Red/green — 新测试确实守护修复只把 实现 文件回退到基线
失败项与 PR 新增行为一一对应（mid-turn 3) 类型检查 — 干净
4) 跨面互通 + 安全（代码阅读 + 测试确认）
5) 可合并性
范围 / 说明
结论：CI 转绿后即可合并；唯一未解决 thread 是可选的性能后续项。 Local verification on Linux (worktree at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Overall this PR makes a solid improvement — the structured content block pipeline for mid-turn images is well-architected and the separation of resolveAtCommandQuery (pure) from handleAtCommand (side effects) is a good refactor.
One inline suggestion noted regarding the randomUUID() fallback in the drain handler.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
| return text.replace(/^(?:[ \t]*\r?\n)+/, ''); | ||
| } | ||
|
|
||
| async function resolveWithAbort<T>( |
There was a problem hiding this comment.
[Suggestion] resolveWithAbort (here) and withTimeoutSignal (in Session.ts) are near-identical abort-race utilities — both implement the same listener-setup, Promise.race, cleanup-in-finally pattern. They differ only in that withTimeoutSignal adds AbortSignal.any with a timeout layer.
Two independently maintained copies of the same logic. A correctness fix or optimization in one must be replicated to the other. Consider extracting a shared utility into packages/cli/src/utils/.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
3 new findings not covered by existing review comments. Build passes, 272 tests pass.
Key concern: The desktop mid-turn queue (qwen-agent.ts) lacks a retry counter for failed entries, unlike the Session.ts ACP path which has MID_TURN_QUEUE_DRAIN_MAX_TIMEOUT_STRIKES. A deterministic buildPromptBlocks failure causes an infinite retry loop.
Also: isEmbeddedResourceResource accepts unbounded text (potential OOM), and recordMidTurnUserMessage now persists base64 image data in chat recording files.
| ?.recordMidTurnUserMessage([part], message); | ||
| return part; | ||
| }); | ||
| ?.recordMidTurnUserMessage(parts, displayText); |
There was a problem hiding this comment.
[Nice to have] recordMidTurnUserMessage(parts, displayText) now receives Part[] containing inlineData with raw base64 image content. The chat recording service writes this to session JSONL, which could significantly inflate recording file sizes (base64 adds ~33% overhead over raw binary) and embed binary blobs in the session history file.
Consider whether the recording service should strip inlineData from persisted parts (replacing with a placeholder or reference) to keep recording files manageable.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
🔬 Maintainer verification — live during-tool-execution TUI reproduction (head
|
| attachments?: FileAttachment[], | ||
| metadata?: MidTurnMessageMetadata, | ||
| ): boolean { | ||
| const trimmed = message.trim(); |
There was a problem hiding this comment.
[Suggestion] enqueueMidTurnMessage pushes to midTurnMessageQueue with no cap on queue length or total base64 payload size. Before this PR, the queue was string[] (text only, a few KB per entry) and SessionManager blocked mid-turn messages carrying attachments. Now the queue carries QueuedMidTurnMessage[] with FileAttachment[] containing base64-encoded images (potentially MBs each). During a long tool execution with no drain boundary, a user could enqueue many large images, all held in memory.
Consider adding a cap:
| const trimmed = message.trim(); | |
| const MAX_QUEUE_BASE64_BYTES = 50 * 1024 * 1024; | |
| const currentSize = this.midTurnMessageQueue.reduce( | |
| (sum, e) => sum + (e.attachments?.reduce((s, a) => s + (a.base64?.length ?? 0), 0) ?? 0), | |
| 0, | |
| ); | |
| const incomingSize = attachments?.reduce((s, a) => s + (a.base64?.length ?? 0), 0) ?? 0; | |
| if (currentSize + incomingSize > MAX_QUEUE_BASE64_BYTES) return false; | |
| const trimmed = message.trim(); |
— qwen3.7-max via Qwen Code /review
| resolvedMidTurnQuery, | ||
| msg, | ||
| ); | ||
| const formatCheck = checkImageFormatsSupport( |
There was a problem hiding this comment.
[Suggestion] checkImageFormatsSupport runs on mid-turn @ command results, but no test in useGeminiStream.test.tsx exercises this path. The existing mid-turn @ test uses image/png (a supported format). A grep for checkImageFormats, hasUnsupportedFormats, and unsupported.*format in the test file returns zero matches.
If checkImageFormatsSupport has a bug in how it inspects Part objects after prefixMidTurnUserMessageParts wraps them, users would get no format-warning notification for mid-turn image attachments even though the pre-submit path (line 1861) correctly warns.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new high-confidence findings after multi-agent review (9 agents + reverse audit). 3 low-confidence items noted for human review (see terminal output). Downgraded from Approve to Comment: CI still running. — qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
5 new suggestions (supplementing the 28 inline comments already posted). Build passes, 274 CLI tests pass. — qwen3.7-max via Qwen Code /review
| const MID_TURN_QUEUE_RESOLVE_TIMEOUT_MS = 10_000; | ||
| const MAX_MID_TURN_DRAIN_ITEMS = 10; | ||
| const MID_TURN_ATTACHMENT_PROCESSING_FAILURE_TEXT = | ||
| '[Attachment could not be processed]'; |
There was a problem hiding this comment.
[Suggestion] MID_TURN_ATTACHMENT_PROCESSING_FAILURE_TEXT is defined here with the identical value '[Attachment could not be processed]' as in packages/desktop/packages/shared/src/agent/qwen-agent.ts:107. The MID_TURN_USER_MESSAGE_PREFIX duplication was already resolved by extracting it into midTurnUserMessage.ts, but this second user-facing string constant was not included in that extraction.
If the failure message needs to change (e.g., for localization or UX polish), two sites must be updated in lockstep. A mismatch produces inconsistent user-facing messages between the CLI ACP path and the desktop path.
Consider extracting to a shared constants module, or at minimum add a // SYNC: also defined in qwen-agent.ts comment at both sites.
— qwen3.7-max via Qwen Code /review
| ]; | ||
| if ( | ||
| message.kind === 'structured' && | ||
| hasInlineMediaContentBlock(message.content) |
There was a problem hiding this comment.
[Suggestion] No test verifies the text-only structured message failure path. The existing failure tests ('keeps later structured mid-turn messages when one resolution fails' and 'adds a fallback marker when audio resolution fails') use items with image or audio content blocks, so hasInlineMediaContentBlock always returns true in failure paths.
The branch where it returns false (text-only structured message fails) is untested. A future refactor could accidentally remove the hasInlineMediaContentBlock guard, causing text-only messages to receive a misleading [Attachment could not be processed] marker on resolution failure.
Add a test case where a structured item has only { type: 'text', text: '...' } content blocks and #resolvePrompt throws. Assert the resulting parts contain the prefix + displayText fallback but do NOT contain the failure marker.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review with qwen3.7-max. Build passes, all 240 CLI tests pass. No new high-confidence issues found beyond the 45+ inline comments from prior rounds.
5 low-confidence findings (needs human review, not posted as inline comments):
- Queue splice precedes ack callback — narrow data-loss window if callback throws
messageIdsparameter can contain raw text/empty strings (defensive fallback paths)- Sequential resolution with no aggregate timeout (200s worst case, unlikely scenario)
- Desktop ack mismatch could cause double delivery (requires unusual ID mismatch)
- Test coverage gaps: 7 untested edge-case branches (unsupported image format, resource block, mixed attachments, etc.)
All prior Critical issues have been addressed. PR looks ready to merge.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
Thanks for the PR, @doudouOUC! Template looks good ✓ — every required heading is present, the reviewer test plan is concrete (how-to-verify, before/after, per-OS matrix, environment, risks), and the bilingual block is complete. Direction. Mid-turn user messages being collapsed to plain strings is a real bug with a user-visible cost: Scope. The PR does three coupled things and all three are needed to actually fix the bug end-to-end:
Cutting any one of those leaves the bug half-fixed on one surface. The ratio is test-heavy (~2500 of the +3350 lines are tests, and red/green from the maintainer's Linux verification confirms they actually guard the new behavior). No drive-by refactors, no scope creep past the stated goal. Two notes, neither blocking:
Moving on to code review. 🔍 中文说明感谢 @doudouOUC 的贡献！ 模板完整 ✓ —— 所有必填标题齐全，reviewer 测试方案具体（如何复现、修复前后对比、按 OS 矩阵、环境、风险），双语折叠块完整。 方向。mid-turn 用户消息被降级为纯字符串，是个真实、用户可感的 bug： 范围。PR 做了三件彼此耦合的事，而三件都是真正把 bug 端到端修掉所必需的：
砍掉其中任何一件，都会在某个面上留下"修了一半"的状态。测试占比高（+3350 行里约 2500 行是测试），而且维护者在 Linux 上的 red/green 验证确认这些测试真的守护了新行为。没有顺手重构，也没有超出既定目标的范围蔓延。 两条非阻塞备注：
进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
|
||
| const messages = this.midTurnMessageQueue.splice(0); | ||
| if (messages.length > 0) { | ||
| const entries = this.midTurnMessageQueue.splice(0); |
There was a problem hiding this comment.
[Critical] Silent message loss when >10 mid-turn messages are queued. splice(0) drains ALL entries from midTurnMessageQueue and onMidTurnMessagesDrained(messageIds) acknowledges ALL of them. But on the CLI side, parseMidTurnDrainResponse caps processing at MAX_MID_TURN_DRAIN_ITEMS = 10 via capMidTurnDrainItems. Entries 11+ are removed from the agent's queue, marked "accepted" in the desktop UI, but never injected into the LLM conversation. Users have no indication their messages were lost.
Suggested fix: Apply the cap in the agent's drain handler before splicing, so unprocessed entries remain queued for the next drain cycle. Alternatively, acknowledge only the entries actually included in the response.
— qwen3.7-max via Qwen Code /review
| @@ -2427,16 +2461,128 @@ export const useGeminiStream = ( | |||
| ? [] | |||
| : (midTurnDrainRef?.current?.() ?? []); | |||
There was a problem hiding this comment.
[Suggestion] UI-side drain has no item cap. midTurnDrainRef?.current?.() returns all drained messages and the for loop iterates over all of them. The ACP path enforces MAX_MID_TURN_DRAIN_ITEMS = 10 in Session.ts, but the UI path has no equivalent limit. While human typing naturally limits queue depth, paste operations or programmatic input could produce larger batches.
| : (midTurnDrainRef?.current?.() ?? []); | |
| const MAX_UI_MID_TURN_DRAIN_ITEMS = 10; | |
| const drained = (midTurnDrainRef?.current?.() ?? []).slice(0, MAX_UI_MID_TURN_DRAIN_ITEMS); |
— qwen3.7-max via Qwen Code /review
| value['mimeType'].startsWith('audio/') && | ||
| typeof value['data'] === 'string' | ||
| ); | ||
| case 'resource_link': |
There was a problem hiding this comment.
[Suggestion] isContentBlock rejects resource_link blocks (return false), but #resolvePrompt at line 4307 fully handles them (resolving URIs via readManyFiles, falling back to @<uri> text). This validation/resolution inconsistency means resource_link content blocks in mid-turn drain items are silently filtered out by getValidMidTurnContentBlocks before #resolvePrompt ever sees them. The ACP spec states "All agents MUST support ContentBlock::ResourceLink in prompts."
| case 'resource_link': | |
| case 'resource_link': | |
| return typeof value['uri'] === 'string' && typeof value['name'] === 'string'; |
— qwen3.7-max via Qwen Code /review
| return typeof value['blob'] === 'string'; | ||
| } | ||
|
|
||
| function hasInlineMediaContentBlock(content: ContentBlock[]): boolean { |
There was a problem hiding this comment.
[Suggestion] hasInlineMediaContentBlock only checks for image or audio types. When #resolvePrompt fails for a message with only resource blocks (no images/audio), this function returns false and the [Attachment could not be processed] marker is NOT appended. The user and model see only the display text with no indication that resource content was lost. Consider broadening the check to cover any non-text content block type.
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| function isEmbeddedResourceResource( |
There was a problem hiding this comment.
[Suggestion] isEmbeddedResourceResource caps text at MAX_MID_TURN_RESOURCE_TEXT_LENGTH = 100_000 characters but the blob field has no size limit. Similarly, isContentBlock doesn't cap data for image/audio blocks. A malicious or buggy ACP client could send arbitrarily large base64 blobs that get decoded and processed without bounds. Consider adding a MAX_MID_TURN_RESOURCE_BLOB_LENGTH constant and validating data.length / blob.length.
— qwen3.7-max via Qwen Code /review
✅ Local runtime verification (Linux, real end-to-end)I ran a real end-to-end verification of this PR on Linux — building the actual Result: both user-facing paths preserve the mid-turn image to the model after this PR, and lose it on the pre-PR baseline. Legacy text-only drains stay backward-compatible. The 275 focused unit tests pass. Environment
1) CLI path — mid-turn
|
| Build | mid-turn message | image_url reaches model? |
mid-turn text? |
|---|---|---|---|
| This PR | @midturn-image.png inspect this image |
✅ yes — resolved file → image_url (our PNG) |
✅ prefixed |
| pre-PR (counterfactual¹) | same | ❌ no — sent as literal @… text, image lost |
✅ literal text |
Continuation request (role: "tool" result is [3]; injected user message is [4]):
This PR — the @ reference is resolved into structured parts and the image is attached:
[4] role=user:
text("\n[User message received during tool execution]: @midturn-image.png ins…")
text("\n--- Content from referenced files ---")
text("\nContent from /…/midturn-image.png:\n")
IMAGE_URL(data:image/png;base64,iVBORw0KGgoAAA…) ← image preserved
text("\n--- End of content ---")
pre-PR — the same message collapses to plain text, no resolution, no image:
[4] role=user:
text("\n[User message received during tool execution]: @midturn-image.png inspect this image")
¹ counterfactual = a one-line surgical toggle that disables only the new mid-turn @-resolution branch, then rebundle (everything else identical to PR head).
2) ACP path — structured mid-turn drain
Session.ts (#drainMidTurnUserMessages / parseMidTurnDrainResponse / #resolvePrompt)
Scenario (real ACP client over stdio): a ClientSideConnection drives initialize → newSession → prompt; the agent runs a tool, then calls the craft/drainMidTurnQueue ext-method, which my client answers with a configurable shape. I assert what lands in the continuation request to the model.
| Build | drain response | image_url reaches model? |
mid-turn text? |
|---|---|---|---|
| This PR | {items:[{content:[text, image]}]} (new) |
✅ yes — image_url (our PNG) |
✅ prefixed |
| This PR | {messages:["…"]} (legacy) |
❌ no (no image sent) | ✅ prefixed — backward-compatible |
| pre-PR | {items:[{content:[text, image]}]} (new) |
❌ no | ❌ whole message dropped |
Decisive evidence — with the PR, the structured items image is injected as a user message right after the tool result:
[4] { "role": "user", "content": [
{ "type": "text", "text": "\n[User message received during tool execution]: inspect this image" },
{ "type": "image_url", "image_url": { "url": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAA…" } }
]}On the pre-PR build the same items response is unrecognized (the old code reads only response.messages), so the continuation contains no injected user message at all (messages.length 5 → 4) — the text and the image are silently lost. This is the bug the PR fixes. The legacy {messages} shape still produces the text-only mid-turn message, so existing clients are unaffected.
3) Focused unit tests (vitest, run on PR source)
✓ src/utils/midTurnUserMessage.test.ts (3 tests)
✓ src/ui/hooks/useGeminiStream.test.tsx (119 tests)
✓ src/acp-integration/session/Session.test.ts (153 tests)
Test Files 3 passed (3) Tests 275 passed (275)
Scope / not covered here
- Desktop Qwen backend (
SessionManager.ts,qwen-agent.ts,backend/types.ts) — the live-attachment forwarding and the missing-base64 → retain-for-next-turn fallback were not runtime-driven: that path needs the desktop server + a desktop/browser client, which is outside this CLI/ACP harness. Itsbun-based unit tests don't resolve standalone in an isolated worktree, so they're left to project CI. Worth a manual desktop pass before merge (the PR marks Windows/Linux untested, too). - The
[Attachment could not be processed]fallback fires only whenclampInlineMediaPartthrows; a valid small image never triggers it, so it's a defensive path best left to the unit tests (which cover it).
Conclusion: the CLI and ACP changes do what the PR says — mid-turn image input is preserved through tool execution and reaches the model, while text-only behavior remains backward-compatible. LGTM on those two paths from a runtime standpoint; recommend a manual desktop spot-check for the third.
🇨🇳 中文版（点击展开）
✅ 本地运行时验证（Linux，真实端到端）
我在 Linux 上对本 PR 做了真实的端到端验证：构建了真正的 dist/cli.js，并通过 mock OpenAI provider（记录发往模型的精确 on-the-wire 请求体）、真实的 ACP ClientSideConnection 以及 基于 tmux pty 的真实 Ink TUI 来驱动它。每条路径都做了 A/B 对照（PR 构建 vs. PR 前基线），从而把结果精确归因到本 PR 的改动。
结论：修复后，两条面向用户的路径都能把 mid-turn 图片保留并送达模型；在 PR 前基线上都会丢失。legacy 纯文本 drain 保持向后兼容。275 个聚焦单测全部通过。
环境
| 系统 | 🐧 Linux（Node v22.22.2，tmux 3.5a） |
| PR head | 855a2686c · base（merge-base）3ff7e7f95 |
| 构建 | npm install → node dist/cli.js（已确认 bundle 含 parseMidTurnDrainResponse / prefixMidTurnUserMessageParts） |
| Provider | 本地 mock OpenAI：第 1 轮返回 run_shell_command tool-call，续轮返回最终文本；model id 用 gpt-4o，因此图片 inlineData 会被序列化成 image_url data URL |
| 测试图片 | 1×1 PNG，base64 以 iVBORw0KGgo… 开头（端到端可 grep） |
1) CLI 路径 — 工具执行期间的 mid-turn @image
useGeminiStream.ts · atCommandProcessor.ts（resolveAtCommandQuery）· midTurnUserMessage.ts
场景（tmux 中的真实 TUI）： 提交 prompt → 模型返回一个长时间运行的 run_shell_command → 在 shell 工具仍在执行时，输入 @midturn-image.png inspect this image 并提交。TUI 显示 ● … / Press ↑ to edit queued messages，说明消息进入了 mid-turn 队列。工具结束后队列被 drain，续轮请求随之发出。
| 构建 | mid-turn 消息 | image_url 是否送达模型？ |
mid-turn 文本？ |
|---|---|---|---|
| 本 PR | @midturn-image.png inspect this image |
✅ 是 — @ 解析为文件 → image_url（我们的 PNG） |
✅ 带前缀 |
| PR 前（对照¹） | 同上 | ❌ 否 — 作为纯文本 @… 发送，图片丢失 |
✅ 纯文本 |
续轮请求（role:"tool" 结果是 [3]，注入的 user 消息是 [4]）：
本 PR — @ 引用被解析为结构化 parts，图片被附上：
[4] role=user:
text("\n[User message received during tool execution]: @midturn-image.png ins…")
text("\n--- Content from referenced files ---")
text("\nContent from /…/midturn-image.png:\n")
IMAGE_URL(data:image/png;base64,iVBORw0KGgoAAA…) ← 图片被保留
text("\n--- End of content ---")
PR 前 — 同一条消息退化为纯文本，没有解析、没有图片：
[4] role=user:
text("\n[User message received during tool execution]: @midturn-image.png inspect this image")
¹ 对照 = 仅用一行手术式开关关掉新的 mid-turn @ 解析分支后重新打包（其余与 PR head 完全一致）。
2) ACP 路径 — 结构化 mid-turn drain
Session.ts（#drainMidTurnUserMessages / parseMidTurnDrainResponse / #resolvePrompt）
场景（stdio 上的真实 ACP 客户端）： 一个 ClientSideConnection 驱动 initialize → newSession → prompt；agent 执行工具后调用 craft/drainMidTurnQueue ext-method，我的客户端用可配置的返回结构回应。然后断言续轮请求里发给模型的内容。
| 构建 | drain 返回 | image_url 是否送达模型？ |
mid-turn 文本？ |
|---|---|---|---|
| 本 PR | {items:[{content:[text, image]}]}（新） |
✅ 是 — image_url（我们的 PNG） |
✅ 带前缀 |
| 本 PR | {messages:["…"]}（legacy） |
❌ 否（不发图片） | ✅ 带前缀 — 向后兼容 |
| PR 前 | {items:[{content:[text, image]}]}（新） |
❌ 否 | ❌ 整条消息被丢弃 |
关键证据 — 修复后，结构化 items 中的图片会作为 user 消息注入在 tool 结果之后：
[4] { "role": "user", "content": [
{ "type": "text", "text": "\n[User message received during tool execution]: inspect this image" },
{ "type": "image_url", "image_url": { "url": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAA…" } }
]}在 PR 前 的构建上，同样的 items 返回无法被识别（旧代码只读 response.messages），续轮里 完全没有注入任何 user 消息（messages.length 5 → 4）——文本和图片都被静默丢弃。这正是本 PR 修复的 bug。而 legacy {messages} 形态仍能产出纯文本的 mid-turn 消息，所以现有客户端不受影响。
3) 聚焦单测（vitest，跑在 PR 源码上）
✓ src/utils/midTurnUserMessage.test.ts (3)
✓ src/ui/hooks/useGeminiStream.test.tsx (119)
✓ src/acp-integration/session/Session.test.ts (153)
Test Files 3 passed (3) Tests 275 passed (275)
范围 / 本次未覆盖
- Desktop Qwen backend（
SessionManager.ts、qwen-agent.ts、backend/types.ts）—— live 附件转发与 缺少 base64 → 留到下一轮 的回退路径未做运行时驱动：该路径需要 desktop server + desktop/浏览器客户端，超出本 CLI/ACP 测试床范围。其bun单测在隔离 worktree 下无法独立解析依赖，故交由项目 CI。建议合并前手动跑一遍 desktop（PR 也标注了 Windows/Linux 未测）。 [Attachment could not be processed]回退仅在clampInlineMediaPart抛错时触发；正常小图片不会触发，属于防御性路径，交由单测覆盖（单测已覆盖）。
结论： CLI 与 ACP 两条改动达到了 PR 的目标 —— mid-turn 图片输入能穿过工具执行并送达模型，同时纯文本行为保持向后兼容。从运行时角度这两条路径 LGTM；建议对第三条（desktop）做一次手动抽查。
Stage 2 — Code review + testingCode review (independent read, then diff)Before looking at the diff I sketched: the bug is that mid-turn user messages are plain strings at three handoff points (CLI Reading the diff against that baseline, the things that matter:
No correctness bugs, no AGENTS.md-level violations, no drive-by refactors past the Testing — independent Linux verification on PR head
|
Stage 3 — Reflection + verdictStepping back: the PR addresses a real, user-visible loss — images (and other structured attachments) silently dropped from mid-turn user messages at three separate handoff points. It's the kind of bug that erodes trust in the tool exactly when the user is relying on it most (typing while the agent is busy), and the linked issue #5036 shows it was hitting real workflows. The approach matches the independent sketch I wrote before reading the diff, and I don't see a materially simpler path. The three surfaces (CLI On the reflection checklist:
Two non-blocking notes (mentioned in Stage 1/2, restated for the record)
ApprovalApproving now. 中文说明Stage 3 — 反思 + 结论退一步看：PR 修的是真实、用户可感的丢失——图片（及其他结构化附件）在三个独立的 mid-turn 交接点被静默丢掉。这种 bug 恰恰在用户最依赖工具时（agent 正忙时输入）侵蚀信任；关联的 #5036 表明它确实命中了真实工作流。 方案与我看 diff 前写下的独立勾画一致，我也没看到更简单的路径。三个面（CLI 反思清单：
两条非阻塞备注（Stage 1/2 已提过，记录在此复述）
审批现在 approve。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Post-merge review (qwen3.7-max): no new Critical findings. One Suggestion on mid-turn message loss during cancellation — see inline.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| if ( | ||
| turnCancelledRef.current || |
There was a problem hiding this comment.
[Suggestion] Mid-turn messages are permanently lost when the user cancels (Ctrl+C) during async @ command resolution. drainQueue() destructively empties the CLI message queue at line ~2462. The for loop below contains await resolveWithAbort(...) calls, which yield control. If cancelled during this window, this guard discards responsesToSend without calling submitQuery — all drained messages are gone from the queue and never reach the model. No log or notification is emitted.
Consider checking cancellation before consuming the drain (so messages stay queued for the next turn), or re-enqueuing unprocessed messages on cancellation:
| turnCancelledRef.current || | |
| // Check cancellation BEFORE draining, not after | |
| if (turnCancelledRef.current || abortControllerRef.current?.signal.aborted) { | |
| return; | |
| } |
— qwen3.7-max via Qwen Code /review
What this PR does
This PR preserves mid-turn user image input across the CLI, ACP, and desktop Qwen backend paths. It lets mid-turn
@file messages resolve into structured parts before tool results are sent, extends the ACP mid-turn drain extension to carry structured content blocks, and forwards live desktop image attachments through the mid-turn queue instead of collapsing those messages to text.Why it's needed
The root cause was that mid-turn messages were represented as plain strings in multiple handoff points. That worked for text, but it lost image parts from
@resolution and desktop attachments during tool execution. It also allowed the desktop session queue to acknowledge some image messages before safe replay when live image data was unavailable. This PR keeps the text-only behavior compatible while preserving structured image content and retaining unsafe image messages for the next turn.Reviewer Test Plan
How to verify
Send a message while tools are still executing that references an image through
@in the CLI; the continuation request after the tool result should include both the prefixed user text and the image inline data. In desktop Qwen mode, send a queued mid-turn message with a live image attachment; the ACP drain should return structured content blocks with text and image data, and the queued message should be acknowledged by stable message id. Send a queued mid-turn message whose image attachment lacks live base64; it should remain queued for the next turn rather than being acknowledged and dropped.Evidence (Before & After)
Before: mid-turn image messages were reduced to text or could be acknowledged before their image data was available, which made the image unavailable to the model. After: focused tests cover CLI
@image resolution during mid-turn tool continuation, ACP structured image block injection, desktop image attachment drain, and the missing-base64 fallback queue path.Tested on
Environment (optional)
macOS local worktree with Node/npm, Bun, Vitest, and GitHub CLI. Validation included focused CLI and desktop tests, TypeScript checks, and the repository build.
Risk & Scope
itemsshape in addition to legacymessages, so reviewers should check clients that implement this extension continue to receive text-only legacy behavior when there are no attachments.messagesresponse remains supported for text-only drains.Linked Issues
References #5036.
中文说明
What this PR does
这个 PR 在 CLI、ACP 和 desktop Qwen backend 的 mid-turn 路径中保留用户图片输入。它会在工具结果发送前解析 mid-turn 的
@文件消息为结构化 parts，扩展 ACP mid-turn drain extension 以传递结构化 content blocks，并把 desktop 的 live 图片附件带入 mid-turn 队列，而不是把这些消息降级成纯文本。Why it's needed
根因是多个 mid-turn 交接点都把消息表示成纯字符串。纯文本场景可以工作，但会丢失
@解析产生的图片 parts 和 desktop 附件。它还可能在 live 图片数据不可用时，让 desktop session queue 提前 ack 图片消息，导致后续无法安全 replay。这个 PR 保持纯文本行为兼容，同时保留结构化图片内容，并把不安全的图片消息留到下一轮处理。Reviewer Test Plan
How to verify
在工具仍在执行时发送一条通过 CLI
@引用图片的消息；工具结果后的 continuation request 应包含带前缀的用户文本和图片 inline data。在 desktop Qwen mode 中，发送一条带 live 图片附件的 mid-turn 队列消息；ACP drain 应返回包含文本和图片数据的结构化 content blocks，并通过稳定 message id ack 队列消息。再发送一条图片附件缺少 live base64 的 mid-turn 队列消息；它应保留在队列中等待下一轮，而不是被 ack 后丢弃。Evidence (Before & After)
修复前：mid-turn 图片消息会被降级成文本，或者在图片数据可用前就被 ack，导致模型拿不到图片。修复后：聚焦测试覆盖了 CLI mid-turn 工具 continuation 中的
@图片解析、ACP 结构化图片 block 注入、desktop 图片附件 drain，以及缺少 base64 时回退保留队列的路径。Tested on
Environment (optional)
macOS 本地 worktree，使用 Node/npm、Bun、Vitest 和 GitHub CLI。验证包括 CLI 和 desktop 的聚焦测试、TypeScript 检查以及仓库构建。
Risk & Scope
messages外也接受结构化itemsshape，因此 review 时应确认实现该 extension 的 client 在无附件纯文本场景下仍保持 legacy 行为。messages响应。Linked Issues
参考 #5036。