Skip to content

fix: preserve images in wire codec round-trips (issue #187) - #189

Closed
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-03_wire-image-loss-fixes
Closed

fix: preserve images in wire codec round-trips (issue #187)#189
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-03_wire-image-loss-fixes

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Fixes both silent image-loss gaps reported in #187, plus a wider variant found during triage.

Triage findings (verified on master v0.0.49)

Gap 1 (openai) — the report pointed at the imgs.length === 1 && firstParsed sidecar condition, but current code has an additional filter upstream: allImageParts only collected image parts whose url parses as a data URL (introduced by 626f933 to feed rawOpenaiContentParts). A single remote-URL image therefore never reached the sidecar logic at all (imgs.length === 0). Same end state as reported, but the fix must also remove that filter. Reproducing also showed the gap is wider than reported: due to the same filter, multi-image messages dropped every remote-URL part — [data, remote] round-tripped as [data], not just the single-image case.

Gap 2 (responses) — reproduced exactly as described: an image-only user item produced zero core messages. One nuance: loss was path-dependent — patchResponsesInput kept such items via the layout fallback (slot without coreId → original item), while coreToResponses dropped them; and in either path the untracked item stayed invisible to the compression pipeline and would accumulate unbounded. Tracking fixes both paths.

Changes

openai codec (src/wire/openai.ts)

  • allImageParts now collects every image_url part with a string url (data-URL filter removed).
  • Single-image sidecar keeps the raw part verbatim for any URL shape; base64/mediaType fields are split out only for data URLs (the shape suggested in the issue):
    • 1 image → { rawOpenaiContent } (+ base64 fields iff data URL)
    • 1 images → { rawOpenaiContentParts } (field unchanged, now complete)

  • coreToOpenai needed no change — its precedence chain already prefers verbatim raw over base64 rebuild.
  • Image-only messages keep empty text (pre-existing behavior; switching to a placeholder would shift derived ids for already-working shapes).

responses codec (src/wire/responses.ts)

  • Image-only user items are now tracked: "[image]" placeholder text (anthropic codec precedent) + rawResponsesItem sidecar + data-URL base64 split as before, so they enter the compression space with stable ids.
  • Multi-image-only items (≥2 input_image, no text) also get "[image]" — previously tracked with the "\n" join artifact, which is not real text. This is the only id churn introduced: those items' ids shift once (deterministically); every previously working shape keeps its exact id.
  • coreToResponses re-emits the raw item verbatim while the kernel text still equals canonicalUserText(raw) (content text, or "[image]" when no text part exists); edited/summarized messages rebuild as plain text as before.
  • Placeholder applies only to role: "user"; assistant/system/developer handling is unchanged.

Known trade-offs / residual gaps (out of scope, reporting only)

  • Literal user text "[image]" shares the base id with the placeholder; ClusterCounter disambiguates deterministically (documented message-id trade-off).
  • The "\n" join artifact for non-text parts pre-dates this fix (e.g. [text, image]"t\n") and is unchanged; normalizing it would shift existing ids.
  • Assistant-role image-only response items remain untracked (same class of gap, opposite direction) — needs owner decision.
  • Mirror builders (src/wire/mirror.ts) cannot represent images at all (MirrorBlock has no image type) — pre-existing parity limitation, affects data-URL images too.
  • Separate crash found during review, pre-existing, not touched here: openaiToCore throws TypeError on {type:"image_url", image_url:null} — filed as openaiToCore 崩溃:image_url 为 null 的 image_url part 触发 TypeError #188.

Evidence

  • 9 new regression tests in tests/wire-bili-message-roundtrip.test.ts (5c–5e, 6b–6g): single/multi/mixed remote-URL OpenAI shapes, image-only responses tracking, verbatim round-trip, patchResponsesInput survival + prune-by-id, cross-turn id stability, whitespace-text edge, malformed image_url. All fail on master and pass with this change (verified against pristine HEAD copies).
  • npm run typecheck ✓ · npm test 581/581 ✓ · npm run build
  • Reviewed by two independent agent reviews (correctness/id-stability angle + integration/consumer angle); consumer audit confirmed no other kernel module depends on the changed behavior.

openai codec: allImageParts no longer filters to data: URLs, so a single
remote-URL image gets the rawOpenaiContent sidecar (base64 fields only
for data URLs) and multi-image messages keep every part via
rawOpenaiContentParts instead of dropping all remote-URL images.

responses codec: image-only user items are now tracked as core messages
with the [image] placeholder text (anthropic codec precedent) plus the
rawResponsesItem sidecar; coreToResponses re-emits the raw item verbatim
while the kernel text still equals canonicalUserText, so such items
survive rebuilds and enter the compression space.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-03_wire-image-loss-fixes (27498c6)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-189

Each push to this PR publishes a new version under the pr-189 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr189.tgz
npm install ./package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 独立复核完成(在我这边环境对 PR 分支 2026-09-03_wire-image-loss-fixes @ 27498c6 逐项复现,非只凭描述)。

验证复现(我的环境)

检查 结果
npm run typecheck ✓ exit 0
npm test 581/581 pass
npm run build ✓ tsup + tsc emit 成功
新测试 vs master 源码 8/9 fail on master(见下方注 ①)

做法:把 PR 的测试文件 + master 的三个源码文件(openai/responses/bili-message)拼在一起跑 tests/wire-bili-message-roundtrip.test.ts,确认这些测试确实在复现 bug 而非空转。

正确性分析(逐层)

openai (src/wire/openai.ts)

  • id 派生只 keyed on textsrc/wire/openai.ts:75 deriveMessageId("user","text",text)),与 imgs 无关 → 放开 allImageParts 的 data-URL 过滤不会移动任何 openai 侧 id。已确认。
  • 单 remote-URL 图走 rawOpenaiContent verbatim;coreToOpenai 优先级链(src/wire/openai.ts:203-208)本来就 raw-parts > raw > base64-rebuild,remote URL 命中 raw 分支原样重发。encode 侧零改动成立。
  • image-only(m.text==="")不注入空 text part(if (m.text) falsy),content 恰好一个 image part。符合预期。

responses (src/wire/responses.ts)

  • 锁步成立coreToResponses 的 verbatim 条件改用 canonicalUserText(raw) === message.textsrc/wire/responses.ts:411)。我逐式比对了两处公式——responsesToCoredisplayText = !effText || (role==="user" && hasImage && !hasTextPart) ? "[image]" : effText(:229)与 canonicalUserText!contentText || (hasImage && !hasTextPart) ? "[image]" : contentText(:119)对用户 role 完全同构,且 splitDemotedThinking 只对 assistant 生效、user 侧 effText===contentText 恒等,所以两者不可能漂移。注释里的 "MUST stay in lockstep" 警告是对的,当前也守住了。
  • 两条路径都修到了:跟踪后 layout slot 带上 coreId → patchResponsesInput 里 prune-by-id(source 在、next 不在 → drop)能正确随 turn 移除;未修前靠 !slot.coreId 的 layout fallback 保住了 item 但对压缩管线不可见、会无界累积。floor 里这个判断准确。
  • id churn 范围精确:只有"≥2 图、无文本"user item 从 "\n" join 伪影变为 "[image]"(一次性确定性);单图 item 是新增跟踪(原先 effText=="" 根本没进 msgs);带文本/whitespace/mixed 形状逐字节不变。assistant image-only 仍不跟踪(gate role==="assistant" && text 需 truthy),属你列的残留项,方向一致。
  • 边界抽查均无回归:content:"" / content:[](不跟踪,同前)、whitespace-only text(真实文本保留)、malformed image_url(tracked 但无 sidecar)、assistant demoted-thinking identity space(displayText 从 post-split effText 推导,未被破坏)。

发现(均为非阻塞,记录用)

证据表述不精确:PR 写 "All fail on master and pass with this change",实测 8/9 fail on master——6f(whitespace-only text + image keeps exact text)在 master 上也通过。它是防回归 guard(锁住 fix 不要把 whitespace 误判成占位符),不是 bug 复现测试,价值仍在,只是措辞应改成 "8 reproduce the bug; 6f guards the whitespace edge"。

重复逻辑靠注释同步canonicalUserText(:115)与 responsesToCore 内联的 displayText 三元(:229)是同一段逻辑的两份拷贝,目前靠注释约束同步。现在正确,但未来任一处单独改动会静默破坏 verbatim round-trip。建议抽成单一 helper(入参 contentText/hasImage/hasTextPart)供两处共用,降低漂移面。非本次必须。

"[image]" sentinel 碰撞是新增交互:字面输入 "[image]" 的 user 文本与图片占位符共享 base id,由 ClusterCounter 按出现顺序确定性消歧(test 6d 覆盖了纯图场景)。这是把既有"重复文本 id-shift"类问题拓宽了一点(新增了个常见 token 作为碰撞源),与 anthropic codec 先例一致、且已被文档化取舍覆盖,接受。仅提示:若日后想更稳可考虑低碰撞 sentinel,但那本身又会引入 churn,现状合理。

结论

实现最小、正确、测试充分,三项 CI 全绿,id-churn 范围被精确界定并验证。我这边无阻塞项,可以合并。按规则我不执行 merge —— 请你自行合并:#189

顺带:#188{type:"image_url", image_url:null} 抛 TypeError,src/wire/openai.ts:271)确认预存在、与本 PR 无关,本 PR 未触碰它,处理得当。

@ranxianglei

Copy link
Copy Markdown
Owner Author

Closing as superseded: master src/wire/openai.ts:70-85,193-200 already preserves images through wire round-trips (rawOpenaiContent / imageBase64 / rawOpenaiContentParts), and wire-level historical-image stripping landed separately in #215 (release PR#218). The branch also predates #215 (shows wire-strip-images.test.ts deletions vs master).

@ranxianglei ranxianglei closed this Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant