Skip to content

Preserve large-integer precision when decoding FunctionResultContent.Result - #717

Open
PratikDhanave wants to merge 2 commits into
microsoft:mainfrom
PratikDhanaveFork:preserve-large-int-function-result
Open

Preserve large-integer precision when decoding FunctionResultContent.Result#717
PratikDhanave wants to merge 2 commits into
microsoft:mainfrom
PratikDhanaveFork:preserve-large-int-function-result

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

FunctionResultContent.UnmarshalJSON decoded the JSON Result into an any via json.Unmarshal. Go's encoding/json decodes JSON numbers into any as float64, so integers above 2^53 were silently corrupted on the round trip (9007199254740993 became 9007199254740992). Because MarshalJSON emits the exact int64, a Marshal followed by Unmarshal was not idempotent for large-integer tool results, which bites checkpoint save/restore and provider re-serialization.

This change adds a serializedFunctionResultContentForUnmarshal struct whose Result is a json.RawMessage, and decodes it in UnmarshalJSON with a number-preserving decoder (json.NewDecoder(...).UseNumber()). Numeric leaves become json.Number, so large integers stay exact. MarshalJSON is unchanged, mirroring the existing serialized...ForUnmarshal pattern already used in this file for tool-approval content.

Why (parity)

This matches .NET's JsonElement-based preservation of tool-result payloads, where numeric values are not eagerly narrowed to a lossy floating-point representation. It is distinct from #528, which fixed FunctionCallContent.Arguments (a string field); the result payload is an arbitrary any and needed a separate fix.

How it is tested

Added TestFunctionResultContentRoundtripPreservesResult in the canonical message/content_test.go. It marshals a FunctionResultContent, unmarshals it, and re-marshals, asserting the two JSON byte slices are byte-identical and that a large integer (9007199254740993) survives exactly. Small-int and string-result cases guard against regressions. The test fails before the change (round-tripped value is 9.007199254740992e+15) and passes after.

go build ./..., go vet ./message/..., and go test ./message/... all pass.

Copilot AI review requested due to automatic review settings July 24, 2026 02:58
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 24, 2026 02:58
@PratikDhanave
PratikDhanave force-pushed the preserve-large-int-function-result branch from 5a0fcc0 to 24228ce Compare July 24, 2026 03:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes lossy decoding of FunctionResultContent.Result by switching UnmarshalJSON to decode the Result payload with a number-preserving JSON decoder (UseNumber), preventing large integers (> 2^53) from being silently corrupted and making internal marshal/unmarshal round-trips stable for tool results.

Changes:

  • Add an unmarshal-only serialization struct using json.RawMessage for Result.
  • Decode Result via json.Decoder + UseNumber() so numeric leaves become json.Number.
  • Add a unit test ensuring large-int result round-trips preserve exact value and marshaled JSON bytes remain identical.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
message/content.go Decode FunctionResultContent.Result using UseNumber() via a json.RawMessage intermediary to preserve large-integer precision.
message/content_test.go Add a round-trip test that asserts large integer results and re-marshaled JSON remain unchanged.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread message/content.go
Comment on lines 412 to +416
t.ContentHeader = tmp.ContentHeader
t.CallID = tmp.CallID
t.Result = tmp.Result
// Decode the result with a number-preserving decoder so numeric leaves become
// json.Number rather than float64. This keeps large integers exact (values
// above 2^53 would otherwise be corrupted) and makes Marshal/Unmarshal

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in c7d6580: UnmarshalJSON now resets t.Result to nil when tmp.Result is empty and t.Error to nil when tmp.Error is empty, so stale values on a non-zero receiver no longer stick. Added TestFunctionResultContentUnmarshalResetsStaleFields to lock the behavior.

@github-actions

This comment has been minimized.

@github-actions github-actions Bot added parity-approved Go API consistency review found no parity issues public-api-change Pull Request changes public APIs labels Jul 24, 2026
@github-actions

This comment has been minimized.

Decode the JSON result with a number-preserving decoder so numeric
leaves become json.Number instead of float64. Unmarshaling into an
any previously corrupted integers above 2^53 (9007199254740993 became
9007199254740992), so a Marshal followed by Unmarshal was not
idempotent for large-integer tool results, biting checkpoint
save/restore and provider re-serialization.
@PratikDhanave
PratikDhanave force-pushed the preserve-large-int-function-result branch from c7d6580 to e14efb5 Compare July 24, 2026 09:33
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review: ✅ Approved

This PR fixes a JSON round-trip precision bug in FunctionResultContent.UnmarshalJSON by switching to json.NewDecoder(...).UseNumber() so that large integers (above 2^53) are not silently corrupted to float64.

Scope: Internal serialization behavior only. No exported types, fields, or method signatures changed. The Result any field type is unchanged; only the decoded representation of numeric JSON values improves from lossy float64 to exact json.Number.

Cross-repo parity assessment:

  • .NET: Uses JsonElement (System.Text.Json) for tool-result payloads, which preserves numeric values without lossy narrowing to double. The Go fix aligns Go behavior with this .NET property.
  • Python: Tool results are handled as raw Python objects/dicts; Python's json module decodes integers as int natively (no float truncation for whole numbers), so Python already has the correct behavior by default.

This change closes a Go-specific gap relative to both .NET and Python. It is a correctness fix, not a divergence. No parity issues found.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by Go API Consistency Review Agent · 24.2 AIC · ⌖ 5.58 AIC · ⊞ 5.9K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parity-approved Go API consistency review found no parity issues public-api-change Pull Request changes public APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants