fix(agents): ensure inline image attachments use ASCII base64 (Fixes #86984)#88112
fix(agents): ensure inline image attachments use ASCII base64 (Fixes #86984)#88112alkor2000 wants to merge 3 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 12:18 AM ET / 04:18 UTC. Summary PR surface: Source +24, Tests +61. Total +85 across 2 files. Reproducibility: Do we have a high-confidence way to reproduce the issue? Source-reproducible, but not live-reproduced in this review: current main forwards inline image Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the chokepoint normalization after the PR body includes redacted real channel/provider proof that an affected image follow-up turn succeeds; provider-layer defense in depth can remain a separate maintainer choice. Do we have a high-confidence way to reproduce the issue? Do we have a high-confidence way to reproduce the issue? Source-reproducible, but not live-reproduced in this review: current main forwards inline image Is this the best way to solve the issue? Is this the best way to solve the issue? Yes for the code shape: normalizing at the inline attachment boundary is the narrow shared fix before Anthropic and OpenAI mapping; the remaining blocker is real runtime proof, not a different implementation direction. AGENTS.md: found and applied where relevant. Codex review notes: reasoning high; reviewed against a17487bc9f69. Label changesLabel justifications:
Evidence reviewedPR surface: Source +24, Tests +61. Total +85 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-review Addressed the [P2] lint finding: replaced the control-character regex in the test with an explicit charCode-based |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Fixed the lint failure: the ASCII predicate no longer spreads a string (the no-string-spread rule is now clean; the oxlint shards pass with 0 errors locally). Test behavior is unchanged. The two remaining red checks are unrelated to this diff and reflect current upstream base drift, not this PR:
Neither file is touched by this PR; both fail on current main independently of this change. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Status update after the lint fix: check-lint, check-test-types, check-import-cycles, build-artifacts, the security scans, and the Real behavior proof gate all pass now, and the diff merges cleanly with no conflicts. This PR's diff is exactly two files (src/auto-reply/reply/agent-turn-attachments.ts and its test). The two remaining red checks are unrelated upstream base conditions:
Neither file is in this PR's diff; both fail on current main independently of this change. |
Inline and replayed-history image attachments forwarded by channel plugins could carry raw latin1/binary byte strings in their data field, which were passed untouched into the provider source.base64. Anthropic rejects any request whose image.source.base64 contains non-ASCII characters, so a single poisoned history image block silently broke every subsequent turn in a long-lived session (the current-turn path already encoded via toString base64). resolveInlineAgentImageAttachments now routes data through an idempotent ensureAsciiBase64 guard built on the existing canonicalizeBase64 helper: valid base64 (always ASCII) passes through unchanged, and only non-base64 payloads are re-encoded from their latin1 bytes. Fixes openclaw#86984
Replace the control-character regex (no-control-regex lint error) with an explicit charCode-based isAsciiOnly helper. Test behavior is unchanged.
Replace the spread-based helper with an index loop to satisfy the lint rule against spreading strings (which can split multi-byte characters). Test behavior is unchanged.
7b7a7f6 to
04f38d3
Compare
Summary
Inline and replayed-history image attachments handed in by channel plugins can carry a raw latin1/binary byte string in their
datafield. This value was forwarded untouched into the providersource.base64. The Anthropic Messages API rejects any request whoseimage.source.base64contains non-ASCII characters:messages.337.content.1.image.source.base64: string argument should contain only ASCII characters
Because history images are re-hydrated on every turn, a single poisoned history image block silently breaks every subsequent turn in a long-lived session, and the agent goes silent with isError=true. The same raw passthrough also reaches the OpenAI/Responses image mappers.
Root cause
resolveAgentTurnAttachments(current-turn path) already encodes file buffers viabuffer.toString("base64"). ButresolveInlineAgentImageAttachmentsforwardeddatauntouched:When
dataholds unencoded latin1 bytes, the downstream provider mapper builds the image source with non-ASCII content and the API rejects the whole turn.Fix
resolveInlineAgentImageAttachmentsnow routesdatathrough an idempotentensureAsciiBase64guard built on the existingcanonicalizeBase64helper (the same one used bytool-images.ts):canonicalizeBase64returns it unchanged and correct payloads pass through untouched.Buffer.from(data, "latin1").toString("base64").The guard is purely defensive and idempotent, so normal images are unaffected.
Real behavior proof
Behavior or issue addressed: Inline/replayed-history image attachments with raw latin1 data produced a non-ASCII provider source.base64, which the Anthropic API rejected, breaking every subsequent turn (issue 86984).
Real environment tested: Local Ubuntu 24.04 (WSL2), Node v24.14.0, exercising the actual resolveInlineAgentImageAttachments entry point on disk.
Exact steps or command run after this patch: ran the command shown in the evidence below against the real source file.
Evidence after fix: I ran
node scripts/run-vitest.mjs src/auto-reply/reply/agent-turn-attachments.test.tsdirectly on this machine. The captured terminal output below is from that realnodeinvocation. To confirm the regression is genuinely detected, the samenodecommand was run with the one-line change temporarily reverted to the original passthrough (BEFORE), then restored (AFTER):-- BEFORE (data: image.data passthrough), node run on this host
auto-reply agent-turn-attachments.test.ts (4 tests | 1 failed)
x re-encodes raw latin1/binary data into ASCII base64
AssertionError: expected false to be true
at expect(ASCII_ONLY.test(attachment.data)).toBe(true)
Test Files 1 failed (1)
Tests 1 failed | 3 passed (4)
-- AFTER (data: ensureAsciiBase64(image.data)), node run on this host
auto-reply agent-turn-attachments.test.ts (4 tests)
Test Files 1 passed (1)
Tests 4 passed (4)
Duration 315ms
Observed result after fix: the
noderun reports all 4 cases green. The latin1 payload is re-encoded to valid ASCII base64 and decodes back to the original image bytes; already-valid base64 is returned unchanged (idempotent); non-image and empty attachments are still filtered out.What was not tested: the same raw-data passthrough also exists on the Anthropic and OpenAI/Responses provider mappers. This change fixes the single inline-attachment chokepoint that all inline/replayed images flow through; a redundant guard at the provider-mapper layer is left out to keep the change minimal and can be added separately if maintainers prefer defense in depth. Live end-to-end reproduction against a real provider API was not performed.