Skip to content

fix: reconstruct reverse-mapped tokens split across SSE delta events - #56

Open
rafaelreis-r wants to merge 1 commit into
zacdcook:masterfrom
rafaelreis-r:fix/sse-cross-event-reversemap
Open

fix: reconstruct reverse-mapped tokens split across SSE delta events#56
rafaelreis-r wants to merge 1 commit into
zacdcook:masterfrom
rafaelreis-r:fix/sse-cross-event-reversemap

Conversation

@rafaelreis-r

@rafaelreis-r rafaelreis-r commented May 17, 2026

Copy link
Copy Markdown

Fixes #55.

Problem

reverseMap() only rewrites complete patterns. The streaming SSE response transformer applied it independently per content_block_delta event, so a sanitized identifier split across two delta events (.ocpla then tform) was never reverse-mapped and the sanitized form leaked to the client.

A naive slice-offset attempt makes it worse: it emits the incomplete prefix raw, then when the next event completes the pattern the already-sent bytes cannot be retracted, producing mangled output like .ocplalaw.

Fix

createSseEventTransformer() does a proper streaming replace:

  • streamReverse() keeps a raw, un-emitted buffer per content block + field. Each delta holds back the trailing bytes that could still grow into a pattern (maxPatternLen-1), emits only the safe prefix, and pulls the cut back so no complete occurrence straddles it.
  • The held tail is flushed as synthetic content_block_delta events on content_block_stop; flushAll() covers streams that end without a stop.
  • thinking / redacted_thinking blocks stay byte-identical pass-throughs.
  • Exports loadConfig / reverseMap / applySseReverseMapChunks and guards startServer() behind require.main for unit-testing.

Tests

test/sse-reversemap.test.js (node --test, no new deps) asserts exact reconstructionreconstruct(transform(events)) === reverseMap(wholeInput) — at:

  • every two-way split offset of a text_delta
  • single-character splitting of a text_delta
  • every two-way split offset of an input_json_delta (escaped tool/prop names)
  • a token split across raw TCP chunks
  • flushAll when no content_block_stop arrives
  • thinking / redacted_thinking byte-invariance

All pass.

reverseMap() only rewrites COMPLETE patterns. The streaming SSE response
transformer applied it per content_block_delta event, so a sanitized
identifier split across two delta events (".ocpla" then "tform") was
never reverse-mapped and the sanitized form leaked to the client. An
earlier slice-offset attempt instead emitted incomplete prefixes raw and
produced mangled output (".ocplalaw").

Add createSseEventTransformer() with a proper streaming replace:
- streamReverse() holds back trailing raw bytes that could still grow into
  a pattern, emits only the safe prefix, and pulls the cut back so no
  complete occurrence straddles it;
- the held tail is flushed as synthetic content_block_delta events on
  content_block_stop, and flushAll() covers streams that end with no stop;
- thinking / redacted_thinking blocks remain byte-identical pass-throughs.

Export loadConfig/reverseMap/applySseReverseMapChunks and guard startServer
behind require.main so the transformer is unit-testable.

Adds test/sse-reversemap.test.js: asserts exact reconstruction at every
split offset (two-way and single-character) for text_delta and
input_json_delta, plus TCP-chunk splitting, flushAll, and thinking
byte-invariance.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@rafaelreis-r
rafaelreis-r force-pushed the fix/sse-cross-event-reversemap branch from e489abe to 46b2c28 Compare May 17, 2026 17:45
@zacdcook

Copy link
Copy Markdown
Owner

Hey @rafaelreis-r — solid catch on this bug. I dug into your diagnosis and reproduction, ran your test suite, and confirmed: the per-event reverseMap was silently leaking sanitized tokens whenever Anthropic's tokenizer split an identifier across delta events. Your createSseEventTransformer algorithm (per-block carry, maxPatternLen-1 cut, complete-pattern pullback, flushAll on stop) is correct, and the test suite is comprehensive — I'd want it in regardless of implementation.

One concern before merging, mostly architectural:

The proxy has had a deliberate "raw string manipulation only" principle since v1.0 — no JSON.parse/JSON.stringify on the body. The original reason was Anthropic's classifier rejecting re-serialized request bodies (Unicode escapes, whitespace, and number formatting all subtly change), and the codebase has consistently honored that through every transform (processBody, reverseMap, billing block injection, metadata injection) — it's why everything reads as indexOf + slice + split/join rather than parsed objects.

Your fix uses JSON.parse(dataStr) + JSON.stringify(payload) on every delta event. For the response direction it's functionally safe — the consumer is OpenClaw, not Anthropic's classifier — but it does break the architectural pattern and changes the byte format of every event the proxy forwards (key ordering, whitespace, possibly Unicode escape canonicalization).

I drafted a raw-string variant that achieves identical functional behavior. It keeps your streamReverse algorithm verbatim, but extracts the index/field/value via small string-aware helpers (extractSseIntField, findSseStringField) and decodes/re-encodes only the field value via tiny hand-rolled JSON-string codec helpers (jsonStringDecode, jsonStringEncode) — no payload parsing, no key reordering. All 6 of your tests pass unchanged against it. I also ran a handful of streamed requests through it on my own production deployment — both text and tool-call paths round-tripped correctly, with sanitized identifiers (openclaw, lossless-claw) reverse-mapped accurately from cross-delta token splits. Short validation window so far, but representative of normal traffic.

Happy to either:

  • Send the variant as a follow-up PR against your branch (you'd own the merge with both your original commit and the refactor on top)
  • Drop the diff here for you to pull into this PR if you'd rather keep it as one
  • Or you can push back if you think JSON.parse is the right call here — open to that. The raw-string principle is a strong default but not sacred, and you may have a reason I haven't considered.

Either way, this bug needed fixing and your work is what got us there. Just want us to land on the variant we'll both be happy maintaining long-term.

@rafaelreis-r

Copy link
Copy Markdown
Author

Agreed — let's go with the raw-string variant. You're right that "functionally safe on the response direction" is the weakest possible reason to keep JSON.parse: it holds today because the consumer is OpenClaw, but a re-serialized byte format (key reordering, whitespace, Unicode-escape canonicalization on every delta) is exactly the kind of invisible change that breaks something downstream later and is miserable to trace. And the consistency cost is real — the raw-string principle only works if it's uniform; a half-and-half codebase turns it into folklore the next contributor won't honor. The hard part — streamReverse — stays verbatim either way, so there's no algorithmic loss in ceding the field-extraction layer.

One ask before merge, since this is where the raw-string approach trades risk rather than removing it: JSON.parse is a bulletproof parser, and jsonStringDecode/jsonStringEncode are hand-rolled. I'd want unit tests on those two helpers specifically, covering:

  • \uXXXX and surrogate pairs (astral-plane chars / emoji) — the usual failure spot for hand-rolled codecs
  • \", \\, \/, and control chars (\n \t \b \f \r)
  • a field value that embeds a fake key (e.g. a sanitized identifier literally containing "index":) — to confirm findSseStringField is genuinely string-aware and doesn't match inside a value

If those pass, the variant is strictly better than mine and I'm happy to maintain it.

For logistics: the follow-up PR against my branch works best — keeps my original commit (the bug repro + test suite) with your refactor on top, and we land it as one. Send it over whenever.

Thanks for the careful review on this.

@rafaelreis-r

Copy link
Copy Markdown
Author

I love the AI talk, btw.

On a side note, this bug had my agents go berzerk. They developed alternate .ocplatform workspaces and broke all sorts of scripts, automations, and basic functionality. They truly believed openclaw and ocplatform were the same word.

Happy that we're fixing it, this tool rocks.
--Rafael

@sontakey

Copy link
Copy Markdown

Hope this fixes the .ocplatform issue! Been driving me insane.

@0n1cOn3

0n1cOn3 commented May 24, 2026

Copy link
Copy Markdown

On a side note, this bug had my agents go berzerk. They developed alternate .ocplatform workspaces and broke all sorts of scripts, automations, and basic functionality. They truly believed openclaw and ocplatform were the same word.

That absolutely driven me crazy as well. It also has created a new folder .ocplatform and has put the data i'd needed inside there instead of .openclaw. Simple fix was a Symlink from .ocplatform to .openclaw.

I still see sometimes .ocplatform but I know it will land for sure in the proper folder. In case of automations or scripts - I dont face these issues. Prolly because I have a call in my system prompts to use bash instead of it when it fails. Since it has its own VM with regular backups and safety guidelines, it didn't had bricked anything so far.

avaclaw1 added a commit to avaclaw1/hermes-billing-proxy that referenced this pull request May 25, 2026
…lits

Per-event reverseMap silently leaked sanitized identifiers (e.g. "Claude")
when the upstream tokenizer split them across delta events ("Cla" + "ude"):
neither event matched the pattern, so the bare form reached the client.

Per-content-block raw-text buffer; each delta runs safeCut to hold the
trailing bytes that could still grow into a pattern (maxPatternLen-1, with
pullback when a complete occurrence would straddle the cut), reverse-maps
and emits only the safe prefix as a synthesized text_delta, flushes the
held tail at content_block_stop. tool_use buffering and thinking pass-
through unchanged.

Raw-string field extraction (findSseStringField + jsonStringDecode/Encode)
preserves the proxy-wide "no JSON.parse on bodies" principle.

23 tests under node --test cover every two-way split offset, single-char
chunking, raw-TCP chunk splits, overlapping-pattern resolution
(Claude Code vs Claude), multi-block independence, flushAll on truncated
streams, thinking/redacted_thinking byte-invariance, surrogate-pair codec,
and findSseStringField string-awareness.

Ports upstream zacdcook/openclaw-billing-proxy#56 with the raw-string
variant the thread converged on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@rafaelreis-r

Copy link
Copy Markdown
Author

@0n1cOn3 you can temporarily patch your deployment with this PR. I've been running smoothly for a couple of weeks now

@zacdcook this is in the wild. happy to move forward as per your recommendation. Let me know.

@sontakey

sontakey commented May 25, 2026 via email

Copy link
Copy Markdown

@0n1cOn3

0n1cOn3 commented May 25, 2026

Copy link
Copy Markdown

@0n1cOn3 you can temporarily patch your deployment with this PR. I've been running smoothly for a couple of weeks now

Aight, applying another PR locally hehe.

@hunandy14

Copy link
Copy Markdown

@rafaelreis-r @zacdcook — opened #59 with the raw-string variant the review here asked for.

It keeps #56's streamReverse verbatim (built on the same commit, so the algorithm credit stays with @rafaelreis-r) and only swaps the per-event JSON.parse/JSON.stringify for four hand-rolled codecs, so the SSE path never parses/re-serializes the body. The existing test/sse-reversemap.test.js passes unchanged, plus a new boundary suite. It's a single cherry-pickable commit on top of #56.

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.

SSE reverse-mapping leaks sanitized identifiers split across content_block_delta events

5 participants