fix(message): normalize empty json.RawMessage tool-call arguments#42
Conversation
An empty-but-non-nil json.RawMessage (e.g. ToolCall.Arguments) is not valid JSON: json.RawMessage.MarshalJSON only special-cases nil -> "null", so encoding/json fails with "error calling MarshalJSON for type json.RawMessage: unexpected end of JSON input" for anything else of length zero. omitempty does not help (it tests the Go zero value, not len == 0). This is the root cause of a goal-supervised worker turn dying mid- stream with exactly that error (session ses_01kx3pvqttfwgbf2n5x1f1y8yh.jsonl), which then stalled and cleared the goal. Add ToolCall.safeArguments (normalizing empty/nil to null) and a ToolCall.MarshalJSON for any direct encoding of a bare ToolCall (e.g. an event's ToolCall field). marshalPart's tagged-union wrapper cannot use the new MarshalJSON directly -- embedding a type that implements json.Marshaler promotes the method onto the wrapper and silently drops its "type" discriminator -- so it reconstructs ToolCall's fields explicitly instead of embedding, applying the same normalization inline.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 4m 32s —— View job Review: normalize empty json.RawMessage tool-call arguments
The core fix is correct and well-targeted. The diagnosis is spot-on: Two points, one substantive: 1. 2. Nothing here blocks the fix — the poisoning bug is genuinely resolved. Point 1 is worth addressing in this PR since it's about the same canonical normalization; a one-line change plus optionally an assertion on the resumed/transcoded value. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ld drift
- safeArguments now normalizes a zero-length Arguments to {} instead of
null, matching the convention both the anthropic and openai transcoders
already use when coercing empty arguments on the wire. A null here
diverged from what actually gets sent, and would not survive a resumed
session's retranscode as a usable arguments value.
- Add TestToolCallEmptyArgumentsRoundTripMatchesTranscodeConvention: a
marshal -> unmarshal -> transcode-path round trip proving the reloaded
Arguments is a parseable empty object, never the literal null.
- Add TestMarshalPartToolCallFieldsMatchStruct: a reflection-based
divergence guard comparing ToolCall's own json-tagged fields against the
keys marshalPart's explicit *ToolCall reconstruction actually emits, so
adding a field to ToolCall without updating that reconstruction fails
with the specific missing key named instead of silently dropping the
field from every persisted tool call.
|
Claude finished @andybons's task in 2m 10s —— View job Review: normalize empty tool-call arguments to
|
…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.
…ge footgun (#43) #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. Co-authored-by: Harness Forensics Agent <harness-agent@example.com>
1 participant
Summary
Forensic fix for a session-poisoning regression caught by the first workload on a fresh binary:
ToolCall.Arguments(json.RawMessage) fails to marshal when empty-but-non-nil —RawMessage.MarshalJSONspecial-cases onlynil, andomitemptytests the Go zero value, notlen == 0. One zero-length arguments payload from a provider poisons the entire history: every subsequent marshal (persist,/message, transcode) fails, and in a goal loop the failure surfaced asgoal.stalledwith a partially-written record. Reproduced verbatim in a red test; empty values now normalize so the poisoned-history state is unrepresentable.Latent in the canonical format since its creation — the goal-retry path merely exercised marshaling enough to trip it. Found and fixed by a box session reading the incident session's log off the Volume (
ses_01kx3q7w0zfbdrxw8e0hfrcdsd), box-pushed.🤖 Generated with Claude Code