Skip to content

feat: wire-level stripHistoricalImages for historical image payloads - #215

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-07_wire-strip-historical-images
Sep 7, 2026
Merged

feat: wire-level stripHistoricalImages for historical image payloads#215
ranxianglei merged 1 commit into
masterfrom
2026-09-07_wire-strip-historical-images

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Adds a protocol-aware primitive to the wire layer for dropping historical image payloads from a request body before the wire rebuild.

What

stripHistoricalImages(body, protocol, keepRecent) removes image parts from every message older than the most recent keepRecent, across all three codecs (anthropic / openai / responses). An image-only message collapses to an "[image]" text placeholder (input_text for Responses) so message count and role ordering stay stable; mixed content keeps its non-image parts; the input reference is returned unchanged when nothing changed. Detection is type-based, so remote-URL images are dropped too — not just data URLs.

Why

Image bytes ride along verbatim on every request even after compression folds the surrounding text — the codecs move images out of CoreMessage.text into sidecars, so the raw payload is forwarded regardless of what got summarized. A host wanting to shed the payload of aged-out turns previously had no shared place to do it. Putting this in the wire layer means "which field carries an image per protocol" has one home, reusable by every host (proxy + in-process adapters) instead of each re-implementing the traversal.

Design split: this is the mechanism only. Whether to strip, how many recent messages to keep, and any overflow interplay remain host policy. keepRecent is a required parameter; no default is baked in here.

Cross-reference

Requested by ranxianglei/billion-context#617 (opt-in strip of historical image payloads during compression). billion-context will call this export once it lands and ships a version; its own PR (#618) currently carries an equivalent in-repo implementation and will be re-pointed at this export after the kernel release.

Pre-flight

  • npm run typecheck — clean
  • npm test — 592 pass, 0 fail
  • npm run build — ok (dist/wire/index.js + .d.ts emitted; barrel re-export confirmed reachable via the /wire subpath)

Ready for review (≥2 agents) ahead of merge. No version bump on this branch — the release flow handles that separately.

Image bytes ride along verbatim on every request even after compression folds
the surrounding text (codecs move images out of CoreMessage.text into sidecars),
so a host that wants to shed the payload of aged-out turns had no shared
primitive. Add one to the wire layer so "which field carries an image" lives in
a single place across hosts.

- src/wire/strip-images.ts: stripHistoricalImages(body, protocol, keepRecent)
  drops image parts from every message older than the most recent keepRecent
  across anthropic/openai/responses. Image-only content collapses to an "[image]"
  text placeholder (input_text for responses) so message count/role stay stable;
  mixed content keeps its non-image parts; returns the input reference unchanged
  when nothing changed. Detection is type-based, so remote-URL images are dropped
  too (not just data URLs).
- Exposed through the acp-kernel/wire barrel (src/wire/index.ts).
- tests/wire-strip-images.test.ts: 10 cases covering all three protocols,
  recent-N preservation, no-op identity, remote-URL stripping, field preservation.

Pre-flight: typecheck clean, full suite 592 pass, build ok.
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-07_wire-strip-historical-images (0d547b3)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-215

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

Option B — Download artifact

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

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review of #215 — reviewer 1/2; a second independent agent review also completed and both converge.

Pre-flight (re-run on branch 2026-09-07_wire-strip-historical-images @ 0d547b3): typecheck clean · npm test 592 pass / 0 fail · build ok (dist/wire/index.js + strip-images.d.ts emitted; barrel export reachable via /wire). Duplicate screen: no prior issue/PR covers wire-level historical-image stripping.

Verdict: Approve-with-nits

No correctness defects in any shape the codecs model. Mechanism/policy split and placement are right. One documentation nit worth fixing before merge.

Correctness — clean. Per-protocol part types + placeholder types match the codecs: openai image_url, anthropic image, responses input_image; placeholders text (openai/anthropic) / input_text (responses). Cutoff math safe (negative/NaN → no-op; keepRecent≥len → no-op; keepRecent=0 → strip all). Reference-stable no-change path returns the input ref untouched, and untouched items keep their exact refs (good for cache-prefix detection). Idempotent (placeholders aren't image parts). String/null/missing containers all no-op. removed counts image parts, so multi-image collapse still reports the true count.

Architecture fit — good. Pure fn, zero imports/deps, no as any, no mutation; barrel-exported; sits pre-ToCore so assignRefsNode stays the sole writer of CoreMessage content. Host owns whether/how-many to strip; no default baked in. Correct home for "which field carries an image per protocol."

Top nit (fix before merge): the header comment overstates id stability

src/wire/strip-images.ts:9-10 claims "content-hash message ids shift once per message … self-healing downstream via orphan-GC." Against the actual id rules (src/wire/message-id.ts + per-codec derivation) that's inaccurate:

  • anthropic: zero shifts — the codec already maps an image block to a core msg whose id derives from literal "[image]" (src/wire/anthropic.ts:121), identical to the placeholder's id.
  • mixed messages (any protocol): zero shifts — ids derive from text only, and stripping leaves text intact.
  • image-only messages: openai user/tool shift "" → "[image]" once; openai assistant and responses user/assistant currently create no core msg, so stripping creates a new one (birth, not shift).
  • "once per message" is not guaranteed: ClusterCounter renumbers a whole identical-identity cluster, so with K near-identical image-only turns a single message can renumber O(K) times as siblings age out one per turn. Bounded & monotonic (stripping is irreversible → churn terminates) but not ≤1. Residual hazard: a block can stay active anchored to a renumbered id that now denotes a different message (src/sync.ts:80-86 only deactivates when zero effective ids are present) — the same id-collision class src/wire/message-id.ts:24-26 already documents as an accepted trade-off for duplicate deletion, not a new break. Orphan-GC heals removals; it does not heal that collision.

Low severity in practice (needs repeated near-identical image-only turns; distinct real images rarely collide), but the comment will mislead future maintainers into believing the impact is trivially bounded. Suggest rewriting lines 9-10 to state: anthropic/mixed are id-stable; image-only openai/responses birth-or-shift; cluster renumbering can exceed one shift per message; the collision hazard is the pre-existing documented one.

Secondary nits (optional, non-blocking — fine as follow-ups)

  • Images nested inside anthropic tool_result.content are not recursed/stripped (only top-level message.content parts). The real API allows image blocks there; typed callers can't hit it (repo type omits them) but raw traffic can. At minimum add a "not covered" note.
  • No dedicated test for multi-image collapse (imgs>1 → single placeholder); all tests use ≤1 image/message.
  • Image-part shape knowledge is duplicated across 4 files with no shared detector — must stay in lockstep. Consider exporting codec detectors later.

Recommendation: the implementation is correct and correctly placed — approve the code. Fix the header comment (top nit, doc-only, ~5 min) before merge, since acp-kernel treats comments as load-bearing; ship the rest as a small follow-up. Merge stays human-only per AGENTS.md — I won't merge.

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