Skip to content

fix(message): guard ProviderData against empty RawMessage entries#43

Merged
andybons merged 1 commit into
mainfrom
fix/rawmessage-systematic
Jul 9, 2026
Merged

fix(message): guard ProviderData against empty RawMessage entries#43
andybons merged 1 commit into
mainfrom
fix/rawmessage-systematic

Conversation

@andybons

@andybons andybons commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Forensics round 2 on the recurring MarshalJSON … unexpected end of JSON input incident. The actual offending field is message.ProviderData (map[string]json.RawMessage) on Reasoning parts — one indirection away from #42's ToolCall.Arguments guard, marshaled unguarded on every persist/journal/hook encode and read by direct map indexing in all three transcoders. A present-but-zero-length provider-data entry reproduces the incident error exactly (red-verified by reverting the fix); goal-supervised sessions hit it repeatedly because they run with reasoning enabled and replay provider data same-family.

Guard applied at the type level mirroring #42's pattern, with the full worker-turn path (transcode + persist) covered by the incident-shaped test, not just the encoder.

Why #42 missed it: its repro matched the error string, not the site — the lesson (forensic fixes must re-run the failing workload shape) goes to AGENTS.md in the next docs pass.

Box forensics session ses_01kx3se8wqexatnesyxmgj5b0q, box-pushed.

🤖 Generated with Claude Code

…ge footgun

#42 normalized ToolCall.Arguments (a bare json.RawMessage field) but left
Reasoning.ProviderData - a map[string]json.RawMessage carrying the exact
same encoding/json footgun one layer of map indirection away - completely
unguarded, which is why 'json: error calling MarshalJSON for type
json.RawMessage: unexpected end of JSON input' recurred on a binary that
already had #42's fix.

Systematic guard, at the message package (mirroring ToolCall's own
safeArguments/MarshalJSON pattern) rather than patching individual call
sites:

- ProviderData.Get(family) is the one choke point every transcoder must use
  to read an entry; it treats a present-but-zero-length entry as absent, so
  a transcoder never unmarshals or replays a poisoned value (anthropic's
  transcoder previously failed with a related but distinct 'bad anthropic
  reasoning data: unexpected end of JSON input' via json.Unmarshal on such
  an entry; openai's transcoder silently emitted a spurious null wire item
  instead of dropping it).
- ProviderData.MarshalJSON drops zero-length entries from the encoded
  object, guarding every direct marshal of a Reasoning part (the session
  log, the server journal, a chat.message plugin hook payload) regardless
  of whether any transcoder's Get ever ran.
- Message.Normalize, called from engine.Session.append (the single ingest
  point every message passes through), deletes zero-length entries from the
  Go value itself at ingest time, so an in-memory session history never
  diverges from what LoadSession would reload (a map with one zero-length
  entry is 'non-empty' to encoding/json's omitempty, which only checks
  length, so leaving the entry in place would make the marshaled shape
  reload-unstable even though it no longer crashes).

Anthropic, OpenAI Responses, and OpenAI-compat transcoders all now read
ProviderData through Get instead of indexing the map directly.

Tests reconstruct the incident at every layer required: message-encoder
unit marshal (message_test.go), full transcode-request marshal for both
anthropic and openai (provider/*/transcode_test.go), and the full
worker-turn path - a scripted provider producing the incident's exact part
shape, run through a real Session.Prompt and PursueGoal turn, persisted to
and reloaded from the session log (engine/store_test.go,
engine/goal_test.go). All reproduce the exact production error before this
fix and pass after it.
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude finished @andybons's task in 4m 10s —— View job


PR Review: guard ProviderData against empty RawMessage entries

  • Read AGENTS.md and gather context
  • Review the ProviderData guard implementation
  • Review transcoder changes (anthropic, openai, openaicompat)
  • Review test coverage
  • Post review feedback

Verdict: LGTM. This correctly root-causes the recurring MarshalJSON … unexpected end of JSON input incident one indirection past #42, and fixes it at the right layer (the message type) so every producer is protected, not just the shipped adapters. I built (go build ./...) and ran the affected packages (message, provider/...) green.

What it does

Three coordinated guards on message.ProviderData (map[string]json.RawMessage), mirroring #42's ToolCall.safeArguments/MarshalJSON pattern:

  • Get(family) — single read choke point; a present-but-zero-length entry reports absent. All three transcoders switch from direct map indexing to Get.
  • MarshalJSON — drops zero-length entries from any direct encode (session log, server journal, plugin hook payload), so no encoder can trip the json.RawMessage footgun.
  • Normalize (called in Session.append) — deletes zero-length entries in place at ingest, so an in-memory message remarshals byte-identically to what LoadSession reloads.

Verification I did on the trickier claims

  • omitempty round-trip equality holds. After Normalize, a map that held only an empty entry is len 0 → omitempty drops the field; the reloaded side has no field → nil map → also dropped. A map with a real entry + an empty one keeps the real entry on both sides. The historyJSON(loaded) == historyJSON(s) assertions are sound.
  • The outside-the-lock Normalize in Session.append is not a race. For the assistant path (engine.go:445-446), s.append(*asst) fully completes Normalize (mutating the shared *Reasoning map) before s.emit(... Message: asst ...) hands the same pointer to subscribers — sequential on one goroutine, so there's a happens-before. No subscriber ever reads the map concurrently with the delete.
  • The openai transcoder fix closes a real latent bug, not just the crash: previously append(json.RawMessage(nil), raw...) normalized an empty entry to nil, which marshals as a spurious null item in the request input list. The test (transcode_test.go) documents and guards exactly this.

Notes (all minor / optional — nothing blocking)

  • openaicompat has no dedicated regression test (anthropic + openai each got one). This is fine here: that branch continues and drops Reasoning regardless of Get's result (transcode.go:213-222), and its own stream never populates ProviderData, so the Get swap is a behavioral no-op kept for consistency. Worth a one-line note in the PR that it's intentionally untested rather than an oversight.
  • MarshalJSON allocates a fresh out map on every encode even in the common all-non-empty case. Negligible (cold persist path, tiny map), but a len==0 / no-empty-entries fast path would avoid it if you want the persist path allocation-free.
  • Doc quality is excellent — the ProviderData and Normalize comments capture why fix(message): normalize empty json.RawMessage tool-call arguments #42 missed this and the omitempty-before-MarshalJSON subtlety precisely. This is the kind of comment that prevents round 3.

Consistent with AGENTS.md: TDD (incident-shaped tests at message, transcoder, persist, and goal-loop levels), red-verification called out, no raw sleeps, golden round-trip comparisons. Good work.
fix/rawmessage-systematic

@andybons andybons merged commit c05fb55 into main Jul 9, 2026
2 checks passed
@andybons andybons deleted the fix/rawmessage-systematic branch July 9, 2026 16:18
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