feat: support session media references end-to-end - #9310
Merged
Conversation
…age-injection # Conflicts: # packages/acp-bridge/src/bridge.ts # packages/webui/src/daemon/session/actions.test.ts # packages/webui/src/daemon/session/actions.ts
…a test The three new media routes (POST/GET/DELETE /session/:id/media[/:mediaId]) were registered but missing from legacySessionTelemetryRoutes, tripping the route drift guard; add them as handler_resolved like their sibling routes. The mid-turn history-replay expectation now carries the replay meta this PR adds (source: mid_turn_message_injected, qwenDiscreteMessage: true). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Addresses the Critical review findings on the session-media PR: - Reject image/svg+xml uploads and serve stored media with Content-Disposition: attachment and X-Content-Type-Options: nosniff (same-origin XSS vector on the daemon/Web Shell origin). - Keep the retained-media TTL sweep running when the session reaper is disabled (sessionReapIntervalMs <= 0) on the default 60s cadence. - Record only the inline bytes the media references actually cover: the gate now counts image blocks only (references are image-only), and the strip keeps unrelated inline parts (e.g. @-mentioned files). - Show '[User message with attachments]' on TUI resume for image-only mid-turn messages recorded with an empty displayText. - Exempt mid-turn injected echoes from the Web Shell status-noise and plan-JSON filters. - Degrade refresh-rebuilt queue rows to summary-only when media hydration failed, so editing cannot silently discard attachments. - Retry cross-session media removal without the clientId when the daemon rejects the stale persisted id (invalid_client_id). - Register session_media in the integration capabilities baseline. Each fix carries a regression test that fails on the pre-fix code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Addresses the remaining Critical review findings on the session-media PR: - Cap media content blocks at 256 on the mid-turn and prompt routes and resolve each distinct mediaId once per resolveContent call — an unbounded array of duplicate references amplified one small request into gigabytes of heap at dispatch. - Record a '[User message with attachments]' placeholder for inline-media-only mid-turn messages with no references, keeping '' only for the reference shape that replay projects. - Restore the same placeholder for image-only ordinary prompts on TUI resume instead of dropping the message from the restored history. - Treat a mid-turn injected echo as renderable when its items carry a non-empty text block, so the degraded-media echo (messages: [''] plus the placeholder text block) is not discarded as malformed. - Release session media in killSession's force-kill and closing-session fallback branches instead of degrading to the crash-path detach retention. - Remove the unreachable duplicate return in DaemonSessionClient.load(). Each fix carries a regression test that fails on the pre-fix code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Addresses the round-4 Critical review findings on the session-media PR: - Move the mid-turn media-reference validation below the idempotent retry-ack rings so a same-id retry whose media was already removed (delete racing an in-flight POST, or a refresh re-enqueueing from the snapshot) settles idempotently instead of failing with session_media_gone (410). - Keep image/* (unknown mime type) prompt images inline instead of uploading them: the media route matches concrete image types only, so the upload POST 400s and the whole submission hard-failed, regressing pre-upload behavior for untyped images. - Project the degraded-media drain echo's placeholder text block when the echo text is empty, so the Web Shell shows the unavailability notice instead of rendering an empty bubble. Each fix carries a regression test that fails on the pre-fix code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…age-injection # Conflicts: # packages/sdk-typescript/scripts/build.js # packages/webui/src/daemon/session/actions.ts
…age-injection # Conflicts: # packages/sdk-typescript/scripts/build.js # packages/web-shell/client/components/QueuedPromptDisplay.tsx # packages/web-shell/client/hooks/useQueuedPrompts.midTurnReconcile.test.tsx # packages/web-shell/client/hooks/useQueuedPrompts.ts # packages/web-shell/client/midTurnDedup.test.ts # packages/web-shell/client/midTurnDedup.ts # packages/webui/src/daemon/session/actions.ts
- Stop deleting a session-media blob when one queued prompt / mid-turn message referencing it is removed: the store has no reference counting, so siblings, replay metadata, or other clients may still hold the same mediaId. Blobs now live until session close / TTL sweep or an explicit removeSessionMedia. - Reject duplicate mediaId occurrences in one message at assertReferences (covers the prompt and mid-turn admission paths); the serializer expands every reference at dispatch, so repeats amplified one upload into an unbounded payload even though only one read is needed. - Align the mid-turn display-text and reference-persistence gates: compute the same willPersistReferences condition before finalizing displayText, so a partially-referenced message records the attachments placeholder instead of an empty displayText with no references. - Keep the webui mid_turn_message_injected sidechannel alive for degraded image-only echoes whose items carry only the placeholder text block, mirroring the SDK normalizer's hasRenderableItemContent.
…age-injection # Conflicts: # packages/cli/src/serve/routes/session.ts # packages/cli/src/serve/server/telemetry.test.ts
ytahdn
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin,
wenshao and
yiliang114
as code owners
August 17, 2026 03:51
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 9 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
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. |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "session_media" |
— Qwen Code · serve A/B
Collaborator
Collaborator
|
Thanks for the PR — this is a big one, so here is the gate read before the code review. Template ✓ — all required sections present, bilingual body complete. Problem: this is a feature/architecture PR, so the bar is "real user problem", not "fix reproduction". The motivation is concrete and matches how the current inline-base64 design behaves in practice: queued images lost after refresh, drained messages briefly rendering as one merged message, one stale attachment blocking a whole mid-turn drain, and the same bytes repeated across request JSON, SSE snapshots, and replay. No linked issue — if there is one (or a short recording of the refresh-loss behavior), linking it would make the before/after provable instead of asserted. Direction: aligned. Media robustness is an active investment area in the reference agent's changelog (non-blocking clipboard-image reads, image/document stripping against the 32 MB request cap, recalled paste-attachment fixes), so making queued and replayed images survive is on-mission. Note this touches the daemon HTTP surface, the TypeScript SDK's public types, and the ACP bridge protocol — public-contract territory, which on its own warrants maintainer awareness. Size: large cross-package change — 9,724 changed lines, of which 3,003 production (44 files) vs 6,721 test (28 files), zero generated/schema. Per the core-module gate, a Approach: the shape is the right one — upload once into a session-scoped store, move bounded references through queues/snapshots/events, resolve to inline bytes only at the ACP dispatch boundary, capability-gated so legacy clients and daemons keep working. A design doc is included under Risk: Stage 1e flags one high-risk path — Moving on to code review. 🔍 中文说明感谢贡献——这是个大 PR，先给出门禁结论再看代码。 模板 ✓ —— 各节齐全，中英双语完整。 问题： 这是 feature/架构类 PR，标准是"真实用户问题"而非"fix 复现"。动机具体，且与现状吻合：排队图片刷新后丢失、drain 的多条消息短暂合并渲染、一张失效附件阻塞整个 mid-turn drain、base64 在请求 JSON、SSE 快照和 replay 中重复。没有关联 issue——如果有（或者有刷新丢失的录屏），关联上能让 before/after 可证而不是仅靠描述。 方向： 对齐。参考实现的 changelog 显示媒体健壮性是活跃投入方向（剪贴板图片非阻塞读取、32 MB 请求上限下的图片/文档剥离、粘贴附件恢复修复）。注意本 PR 触及 daemon HTTP 接口、SDK 公开类型和 ACP bridge 协议——公开契约区域，仅此一点就需要维护者关注。 规模： 大型跨包改动——共 9,724 行，其中 3,003 行生产代码（44 文件）、6,721 行测试（28 文件）、无生成/schema 文件。按核心模块门禁，这个体量的 方案： 形态正确——上传一次进 session 级存储，队列/快照/事件里只传引用，仅在 ACP 分发边界解析为 inline bytes，capability 门控保证旧客户端和旧 daemon 继续工作。附 风险： Stage 1e 命中一条高风险路径—— 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewI proposed my own design before reading the diff (session-scoped daemon store, upload-once, reference-by-id through queue/snapshot/event paths, resolve to inline bytes at exactly one boundary, capability gate, bounded storage, isolated degradation). The PR matches that shape and goes further on the race-prone edges than my proposal did. No critical blockers found. What I verified in the diff:
Non-blocking notes:
How a media reference flows (upload → drain → dispatch)sequenceDiagram
participant P1 as Web Shell
participant P2 as Daemon HTTP
participant P3 as Media store
participant P4 as Mid-turn queue
participant P5 as ACP bridge
participant P6 as ACP agent
participant P7 as SDK client
P1->>P2: upload image bytes
P2->>P3: put, cap-checked
P3-->>P1: reference (id, mime, size)
P1->>P2: prompt or mid-turn message carrying the reference
P2->>P4: enqueue reference, no base64
P6->>P5: drain between tool batches
P5->>P3: resolve reference to bytes
P5-->>P6: inline content blocks for the agent
P7->>P2: hydrate references from snapshots and events
Files changed (24 of 44 production files shown)
…and 20 more files (tests for every module above, plus a design doc, CSS, i18n strings, and the SDK build script). Test evidence — the PR's own CI (unattended run, PR code not executed locally)All Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The central claims here are behavioural — queued previews survive refresh, drained messages stay separate, one dead media degrades only itself — and the PR's own suite (however thorough, and it is thorough: 6,721 test lines pinning normalizer/sidechannel parity, retry-ack idempotency, and the salvage races) is the author's code testing the author's guards. Sandboxed verification would settle this: Not verified: Windows/Linux manual UI behaviour (author reports macOS only, per the PR body — author's claim, not independently re-run here). 中文说明代码审查： 在读 diff 之前我先独立给出了自己的设计（session 级 daemon 存储、一次上传、引用贯穿队列/快照/事件、仅在单一边界解析为 inline bytes、capability 门控、有界存储、隔离降级）。PR 与这个形态一致，且在竞态边界上比我的方案考虑得更细。未发现阻塞性问题。 已逐项核对：三层容量上限（单文件 8 MiB、单 session 100 MiB/256 项、daemon 全局 512 MiB，经串行化 put 队列执行）+ 3 小时 TTL 清扫 + 关闭/kill/停机立即释放；防放大（同一 mediaId 单次准入、按批 memo 解析、256 块上限、内联字节单独预算）；同源 XSS 已处理（SVG 拒绝 + 归一化媒体类型比较 + nosniff + attachment 下载头）；降级按块隔离、drain 遇非媒体 I/O 失败会重新入队并回滚 settled 环；SDK 区分 404/410 与瞬时失败；Web Shell 让部分水合的行不可编辑，防止刷新静默丢附件；legacy 兼容与 capability 预检齐全。高风险路径（acp-integration Session.ts）已全深度审查：引用严格校验、持久化前剥离已被引用覆盖的 inline part、displayText 与持久化条件严格对齐。 非阻塞建议：占位符文案与引用形状判断在四个包间靠注释同步，未来易漂移；core 的 测试证据： 无人值守运行，未本地执行 PR 代码。被审 commit 的全部 行为性验证缺口： 核心主张（刷新后预览仍在、drain 保持独立、失效媒体只降级自身）目前由 PR 自带测试支撑。可用 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 3/5 — clean static review and green CI, but this is a 3,003-production-line change across daemon, ACP bridge, SDK, and Web Shell, and the Stage 0 core-module escalation caps this at a maintainer's call rather than a bot approval. Stepping back on the whole picture:
⏸️ Deferring to @wenshao — Stage 0 escalation for a cross-package core feature at 3,003 production lines (policy cap, not a defect finding), plus one open judgement call only a maintainer can make: whether to land this as one 72-file PR or ask for a split, and whether to trigger 中文说明置信度：3/5 —— 静态审查干净、CI 全绿，但这是横跨 daemon、ACP bridge、SDK 与 Web Shell 的 3,003 行生产改动，Stage 0 核心模块升级决定了应由维护者拍板，而不是机器人批准。 整体回顾：方案与我的独立设想一致且更完整——复杂度恰恰花在值得的地方（drain 遇 I/O 失败重新入队、重试幂等比较含媒体在内的完整载荷、reconcile 前先抢救在途准入里的图片、降级行与瞬时未水合行区别对待防止编辑丢附件）。六个月后维护这份代码会感谢作者。解决的是真实问题：mid-turn 排队的图片丢失、两条消息合并成一条，都是 Web Shell 可靠性的切身痛点；不再让 base64 穿过 SSE 与 replay 的收益也真实存在。改动基本都在最小必要集内（遥测目录、错误映射、resume 更新是三条新路由的必然配套），唯一的软肋是体量本身——72 个文件一旦合并后出问题，回滚成本高，这正是门禁升级而非批准的原因。 CI 与承诺一致：单元、Serve A/B、web-shell E2E smoke、桌面壳、真实 daemon E2E 全绿，测试确实钉住了刁钻场景。CI 看不到的是活的行为——刷新后预览仍在、两条 drain 行保持两条——目前的手工验证来自作者（仅 macOS）。这正是上文 ⏸️ 转交 @wenshao —— Stage 0 对 3,003 行生产代码的跨包核心 feature 的政策升级（是政策上限，不是缺陷结论），外加一个只有维护者能做的判断：按一个 72 文件 PR 合入还是要求拆分，以及合并前是否触发 — Qwen Code · qwen3.8-max Reviewed at |
chiga0
reviewed
Aug 17, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9310 feat: support session media references end-to-end
Type: New Feature
Change size: +9376/-348 across 72 files
Commits reviewed at HEAD: 516a0936
Findings Summary
- Critical/Major: 0
- Minor: 3
- Nit: 2
Key Observations
This is a well-engineered, end-to-end feature. The layered design (upload once at the HTTP boundary → carry media IDs through transport/SSE/transcript → resolve to inline bytes only at ACP dispatch) cleanly eliminates base64 duplication while preserving full backward compatibility via the session_media capability gate. The security controls (SVG rejection at both upload and block-parse layers, Content-Disposition: attachment, X-Content-Type-Options: nosniff, per-item/per-session/daemon-wide byte caps, duplicate-mediaId guard) are solid. The multi-round fix history shows thorough coverage of edge cases (TOCTOU, degrade-in-place vs re-admit, image-only placeholder, TTL sweep when reaper is disabled).
Three minor issues are worth addressing before shipping.
Cross-Validation
No prior reviews exist on this PR. The following table reflects independent findings:
| Finding | Prior Reviewer | My Assessment |
|---|---|---|
M1: mediaPutQueue global serial queue is unnecessary in Node.js |
— | New — see inline |
M2: Wholesale image/* fallback discards typed images |
— | New — see inline |
M3: hydrateReplaySnapshot fires O(N) concurrent HTTP reads on restore |
— | New — see inline |
N1: parseMediaContentBlock missing upper-bound on mediaId length |
— | New — see inline |
N2: DaemonSessionMediaReference uses Record<string,unknown> intersection |
— | New — see inline |
Additional Audit Coverage
- TOCTOU in
assertReferences→resolveContent: validated that the synchronous gap (promotion pre-filter →sendPrompt) is handled by the in-place degradation path for promoted mid-turn messages. SessionMediaStore.close()race with in-flightput(): traced all concurrent paths (closed before writeFile, closed after writeFile, closed during writeFile); accounting is correct in all branches.mediaPutQueueTOCTOU analysis:SessionMediaStore.put()incrementstotalBytessynchronously before its firstawait; JS single-threading makes the queue redundant — see M1.hydrateBlockrejection safety: confirmed it never propagates (404/410 → placeholder, other errors → raw reference block returned);restore()is safe.extractMediaBlocksfor audio blocks: legacy audio blocks pass through correctly because they arrive via the ACP SDK path that bypassesparseMediaContentBlock.- Capability gate wiring:
session_mediain capabilities.ts,canInjectMidTurnMediainuseQueuedPrompts, andsupportsMediaUploadinpromptContentWithUploadedMediaare all consistently gated. - SVG bypass vectors:
isSvgMimeTypecorrectly handles;charset=…params, case variants, and leading/trailing whitespace; no bypass found. withMediaDegradationMarkeridempotency: theblock.text.endsWith(...)guard at the last text block correctly prevents double-marker in the common replay-append path.
Final Verdict
The implementation is production-ready modulo M1–M3. None of the findings are correctness regressions — all three are trade-offs that constrain performance or capability slightly below what the design allows. Safe to merge with M1–M2 as follow-up issues.
This review was generated by QoderWork AI
chiga0
previously approved these changes
Aug 17, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Re-review at HEAD 516a0936
All findings from the initial review are Minor/Nit — no correctness regressions, no security gaps. The core design (upload-once / carry-by-ID / resolve-at-dispatch, SVG rejection, per-item + per-session + daemon-wide caps, degrade-in-place, capability gate) is sound and the fix history shows thorough edge-case coverage.
Unresolved items (safe to track as follow-up issues):
- M1:
mediaPutQueueglobal serial queue — harmless but unnecessary given JS event-loop semantics; follow-up optimization - M2: Wholesale
image/*fallback — suboptimal for mixed-type batches; follow-up UX improvement - M3:
hydrateReplaySnapshotO(N) concurrent requests on restore — only matters for media-heavy sessions; lazy hydration is a clean follow-up
LGTM ✅
This review was generated by QoderWork AI
yiliang114
reviewed
Aug 17, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Independent verification pass at head 516a0936, separate from the earlier AI review. The storage/route security surface checks out, but I found three issues worth addressing.
Verified clean:
- Media routes use the same
withOwnerMutableSession/withOwnerReadSession+parseClientIdHeadergates as siblings, plus bridge-sideresolveTrustedClientId. - No path traversal: mediaIds are server-generated
randomUUID, paths aremkdtemp (0700)/uuid, reads/writes go through the records map,fs.writeFileuseswx. - SVG rejected at both upload (415) and parse layers; downloads set
Content-Disposition: attachment+nosniff; per-item 8 MiB enforced byexpress.rawlimit AND store; per-session/daemon caps incremented synchronously before any await (no concurrent bypass).
Findings:
-
(P2, potentially P1) Drain re-serializes resolved media per queued message — unbounded amplification. In
bridgeClient.tshandleMidTurnQueueDrain,mediaMemodedups resolution, but each of up toMAX_MID_TURN_QUEUE_DEPTH(20) queued messages still gets a full copy of the resolved base64 blocks in both the drain responseitemsand themid_turn_message_injectedSSE echo. With the 100 MiB / 256-item session store, one adversarial (or just media-heavy) drain can materialize gigabytes of JSON in the daemon and broadcast it to every subscribed client — OOM risk for the whole daemon. The PR's ownMEDIA_CONTENT_MAX_BLOCKScomment flags this hazard for prompts; the drain path reintroduces it across messages. Suggest bounding aggregate resolved bytes per drain (reuse the session byte budget across the batch, degrade overflow), or echoing references + one shared payload. -
(P2) Drain/delete race: a removed mid-turn message can still be injected.
handleMidTurnQueueDrainnow awaits media resolution after splicing the queue. AremoveMidTurnMessagearriving in that window finds an empty queue and a non-promoted message, returns{removed: false}, deletes no media — and the in-flight drain injects the "deleted" message anyway. Secondary: on a non-media resolve failure, the catch pathunshifts drained messages back and strips their ids fromsettledMidTurnMessageIds, which can resurrect ids already settled by concurrent removals. Suggest tracking in-flight drained ids so removal can ack/cancel them, or resolving before splicing. -
(P2) Salvaged promoted image rows stay stuck
summary-only. InuseQueuedPrompts.ts, thepromotedImagessalvage does{ ...prompt, images }without clearingpayloadCompleteness: 'summary-only'. Neither recovery pass can fix it afterwards:applyServerPrompts's clear branch requires!next[existingIndex]!.images(now false), and the snapshot upgrade pass requiresmidTurnMessageId !== undefined(already cleared). Trigger: mid-turn image send, turn settles before drain, one transient media-read failure during hydration. Result: images display but the row is permanently non-editable — user must delete and retype. The tests pinning the summary-only upgrade invariant don't cover the salvage path.
Also a minor note: SessionMediaStore.put itself accepts image/svg+xml (only the HTTP route blocks it) — defense-in-depth gap if a future producer calls the store directly.
Holding approval pending author response on 1 and 2.
Collaborator
Author
|
Addressed the independent review at commit
Validation: all 112 |
yiliang114
approved these changes
Aug 17, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Re-verification at head 5d506485 of the responses to my earlier findings:
1. Drain amplification — fix verified. serializedMediaIds is shared across the whole drain batch and mediaMemo dedupes reads; the SSE echo maps the already-degraded items, so it is bounded by construction. The shared-media test asserts the actual bound (first message carries the image, later ones the marker, and the store read happens exactly once). The OOM amplification I flagged is gone.
2. Drain/delete race — accepted as designed, verified. The splice + settled-ring push are fully synchronous before any await, removeMidTurnMessage is synchronous and misses cleanly with {removed:false} without touching the ring, and the non-media rollback is synchronous inside the catch. No window for double-settle, silent loss, or ring corruption; removed:false while the message injects is the honest claim semantics.
3. Stuck summary-only salvage — accepted, verified unreachable. Admission entries are created payload-complete with a submitting row; the reconciliation that degrades/restores rows deletes the admission in the same synchronous pass, and summary-only is stamped only after the admission is gone. The salvage branch cannot meet summary-only plus recoverable images.
Non-blocking follow-up (found while verifying the fix): admission still accepts cross-message mediaId reuse — assertReferences scopes seenMediaIds to the single content array, and two distinct messages referencing the same stored mediaId both get accepted:true — while the drain now silently degrades the second occurrence to "[Attached media is no longer available]" and drops its mediaReferences metadata. Since the PR contract says cross-message reuse is intentionally unsupported, enforcement belongs at admission (reject the reuse), or at minimum degrade with a distinct marker and retain the reference metadata; as-is, accepted content is dropped with a marker indistinguishable from genuine data loss. Fine to track as a follow-up issue — the daemon-side amplification risk itself is resolved.
LGTM, approving.
chiga0
approved these changes
Aug 17, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Re-review at HEAD 5d506485
Verifying the two new commits in response to yiliang114's findings.
Finding 1 — Drain amplification (P2/P1) → Fixed ✅
Commit fix(daemon): bound repeated media in queue drains adds serializedMediaIds: Set<string> scoped to the entire drain call. Before any media resolution, each message's content is filtered synchronously:
const planned = (item.content ?? []).filter((block) => {
if (!isSessionMediaReference(block)) return true;
if (serializedMediaIds.has(block.mediaId)) {
degraded += 1; // pre-count: marker will be appended
return false; // drop from this message's planned list
}
serializedMediaIds.add(block.mediaId);
return true;
});All paths verified:
serializedMediaIdsis per-drain (declared once, shared across thefor (const item of drained)loop), so the first message wins and subsequent ones are dropped before touchingmediaMemo✅degradedis pre-incremented for filtered blocks →withMediaDegradationMarkeris correctly applied whendegraded > 0✅mediaMemostill dedupes I/O: the filtered blocks never reachresolveContent, so no redundant reads ✅- The OOM risk is gone: one stored blob can serialize into at most one drain response, regardless of how many messages reference it ✅
Test correctly validates the new contract: messages B/C/D get "text\n[Attached media is no longer available]" and read is called exactly once.
Finding 2 — Drain/delete race (P2) → Fixed / accepted as designed ✅
Commit fix(daemon): remove media with deleted queue items adds removeQueuedMedia(entry, content) called eagerly in both removePendingPrompt (queued state) and removeMidTurnMessage. The function is fire-and-forget with error logging.
With this change:
- If
removeMidTurnMessagewins the race (message not yet spliced), the blob is deleted before the drain reaches resolution → drain gets a degraded block, not the original image ✅ - If the drain wins (already spliced),
removeMidTurnMessagecleanly misses with{removed:false}and there's nothing to delete ✅ - No double-settle, no ring corruption in either ordering ✅
Previous tests expecting .toBeDefined() after a delete are correctly inverted to .toBeUndefined(). The "keeps a shared mediaId resolvable for a sibling message" test is removed, which is consistent with the new invariant: cross-message mediaId reuse is not a supported pattern.
Finding 3 — Stuck summary-only salvage (P2) → Verified unreachable ✅
No code change. The salvage branch { ...prompt, images } can't meet payloadCompleteness: 'summary-only' + recoverable images simultaneously: summary-only is stamped only after the submitting admission is removed in a synchronous pass, which is the same pass that deletes the admission row — the salvage path has no images to recover when summary-only is set. Independent of yiliang114's verification, the code path analysis confirms this.
Follow-up noted (non-blocking)
yiliang114 identified that assertReferences scopes seenMediaIds per-content-array, so two messages referencing the same mediaId both pass admission — but the drain silently degrades the 2nd occurrence with [Attached media is no longer available], indistinguishable from genuine data loss, and drops its mediaReferences metadata. The fix makes the daemon safe, but the admission → drain contract gap is worth a follow-up: either reject cross-message reuse at admission, or use a distinct marker and retain reference metadata for the degraded copies.
Cross-Validation
| Finding | yiliang114 | My Assessment |
|---|---|---|
Drain amplification → serializedMediaIds |
Verified ✅ | Confirmed — filter pre-deduplication is correct and bounded |
Drain/delete race → removeQueuedMedia |
Accepted as designed ✅ | Confirmed — race yields degraded content, no corruption |
| Stuck summary-only salvage | Unreachable ✅ | Confirmed — same-pass synchronous admission removal prevents the condition |
| Cross-message reuse: admission accepts / drain degrades | Follow-up | Confirmed gap exists; non-blocking |
All three original findings addressed. Re-approving.
doudouOUC
added a commit
to doudouOUC/qwen-code
that referenced
this pull request
Aug 17, 2026
Resolve the acp-session-bridge type-import collision: keep both BridgePromptContentBlock (session media references, QwenLM#9310) and BridgeSessionCatalogVersion (this PR). Media references pushed the daemon browser bundle to 156 B under the 196 KiB budget; adding this PR's live-state daemon surface tips it over, so bump the guard to 197 KiB with the house precedent comment.
Collaborator
|
Released in v0.21.14. |
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
This PR adds session-scoped media references across the daemon, ACP bridge, TypeScript SDK, and Web Shell. Images are uploaded once and represented by a media ID plus metadata while they move through prompt submission, mid-turn queues, injected-message echoes, reconciliation snapshots, transcripts, and replay. The bridge resolves references to inline bytes only when dispatching content to the ACP agent.
Queued and injected image messages now preserve their previews across refresh, remain separate messages when drained together, and reconcile consistently when a running turn settles. The implementation keeps legacy inline-image and pending-prompt clients compatible, bounds daemon storage and SDK hydration caches, and degrades an unavailable image without blocking later queued messages. Deleting a queued image message also deletes its uploaded media; reusing one media ID across multiple messages is intentionally unsupported.
Why it's needed
Passing base64 image data through JSON, SSE, queue snapshots, and transcript replay duplicates large payloads in memory and over the wire. It also made image state fragile: queued or user-sent images could disappear after refresh, injected image echoes could differ from normal user messages, and stale media could block an entire mid-turn drain. Session media references provide one bounded storage location and a stable replay-safe identity for each uploaded image.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: base64 image bytes were repeated across transport and replay paths; queued images could disappear after refresh, and multiple drained messages could temporarily render as one combined message.
After: transport and replay use bounded session media IDs, queued messages remain distinct, image previews survive refresh, and unavailable media is isolated to the affected message.
Tested on
Environment (optional)
Node.js 22; targeted Vitest suites (2,571 tests), full workspace build, and full workspace typecheck.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 的改动
本 PR 在 daemon、ACP bridge、TypeScript SDK 和 Web Shell 中加入会话级媒体引用。图片只上传一次，在 prompt 发送、mid-turn 队列、插入消息回显、reconciliation snapshot、transcript 和 replay 中以 media ID 与元数据传递；仅在向 ACP agent 分发内容时由 bridge 解析为 inline bytes。
带图片的排队消息和插入消息现在可以在刷新后保留预览，多条消息一起 drain 时仍保持独立，并在运行中的 turn settle 时保持一致的 reconciliation 行为。实现同时兼容旧版 inline 图片和 pending-prompt 客户端，对 daemon 存储与 SDK hydration cache 设置边界，并确保单张不可用图片不会阻塞后续排队消息。删除带图片的排队消息时会同时删除其已上传媒体；本功能明确不支持在多条消息之间复用同一个 media ID。
为什么需要
在 JSON、SSE、队列快照和 transcript replay 中传递 base64 图片会在内存和网络中重复大体积数据，也会让图片状态变得脆弱：排队或用户发送的图片可能在刷新后消失，插入图片的回显可能与普通用户消息不一致，失效媒体还可能阻塞整个 mid-turn drain。会话媒体引用为每张上传图片提供单一、有边界且可安全 replay 的稳定身份。
Reviewer 测试计划
验证方式
证据（改动前后）
改动前：base64 图片数据会在 transport 与 replay 路径中重复；排队图片可能在刷新后消失，多条 drain 消息也可能暂时显示成一条合并消息。
改动后：transport 与 replay 使用有边界的 session media ID，排队消息保持独立，图片预览可在刷新后恢复，不可用媒体只影响对应消息。
测试平台
环境（可选）
Node.js 22；定向 Vitest 测试共 2,571 个、全 workspace build、全 workspace typecheck。
风险与范围
关联 Issue
无