Degrade on unknown annotation/region discriminators instead of aborting the message parse - #626
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates JSON decoding for annotations/annotated regions to tolerate unknown discriminator values by preserving them as raw JSON, enabling forward compatibility with newer provider-emitted subtypes.
Changes:
- Add fallback decoding for
Annotations/AnnotatedRegionsto preserve unknown entries as raw JSON. - Introduce
RawAnnotationandRawAnnotatedRegiontypes that round-trip the original JSON on marshal. - Add a unit test validating preservation + round-trip behavior for unknown types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| message/annotation.go | Adds fallback unmarshalling and new raw wrapper types to preserve unknown annotation/region JSON. |
| message/annotation_test.go | Adds a test ensuring unknown annotation/region types are preserved and round-trip correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return []byte("{}"), nil | ||
| } | ||
|
|
||
| func (t *RawAnnotation) kind() annotationKind { return "" } |
There was a problem hiding this comment.
This is intentional and mirrors the existing RawContent.kind() in the same package (message/content.go:497), which likewise returns "". kind() is unexported, is never registered in supportedAnnotations, and is never consulted for Raw types — RawAnnotation.MarshalJSON emits RawRepresentation directly and unmarshalling routes through the fallback, not kind(). Introducing a "raw" sentinel here would diverge from RawContent for no behavioral gain.
| return []byte("{}"), nil | ||
| } | ||
|
|
||
| func (t *RawAnnotatedRegion) kind() annotatedRegionKind { return "" } |
There was a problem hiding this comment.
This is intentional and matches RawContent.kind() (message/content.go:497) and RawAnnotation.kind() in this file. The method is unexported, never used as a map key for Raw types, and MarshalJSON round-trips RawRepresentation directly rather than switching on kind(), so an empty discriminator has no downstream effect.
| var annotations message.Annotations | ||
| if err := json.Unmarshal(data, &annotations); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Good catch — added TestAnnotationEncoding_KnownTypeInvalidPayloadReturnsError in c596d0c. It asserts that a known discriminator (citation / text_span) with a mismatched payload surfaces a decode error rather than being silently downgraded to RawAnnotation/RawAnnotatedRegion. The fallback only triggers for missing/unsupported discriminators, and this test locks that in.
| func unmarshalRawAnnotation(data json.RawMessage) (Annotation, error) { | ||
| return &RawAnnotation{RawRepresentation: append(json.RawMessage(nil), data...)}, nil | ||
| } |
There was a problem hiding this comment.
Kept as-is for consistency: append(json.RawMessage(nil), data...) is the established clone idiom across this package (e.g. message/content.go:493 and the jsonx fallback), and it yields a json.RawMessage directly whereas bytes.Clone returns []byte and would need a conversion back. Switching only these two call sites would introduce inconsistency for no functional change.
| func unmarshalRawAnnotatedRegion(data json.RawMessage) (AnnotatedRegion, error) { | ||
| return &RawAnnotatedRegion{RawRepresentation: append(json.RawMessage(nil), data...)}, nil | ||
| } |
There was a problem hiding this comment.
Same as above — kept append(json.RawMessage(nil), data...) to match the clone idiom used elsewhere in the package (message/content.go:493, jsonx). Functionally equivalent, and it avoids a []byte->json.RawMessage conversion that bytes.Clone would require.
01d96e0 to
844b9dc
Compare
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
parity-approved
844b9dc to
316f9fa
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ng the message parse Annotations and AnnotatedRegions used the no-fallback discriminated-union unmarshaler, so any provider-emitted subtype outside the registered citation/text_span kinds failed the entire enclosing deserialization. Content already tolerates unknown kinds via RawContent; mirror that by adding RawAnnotation/RawAnnotatedRegion fallbacks that preserve and round-trip the original JSON, matching the .NET/Python behavior of not dropping or rejecting unrecognized union members.
c596d0c to
fe7464a
Compare
Parity Review — PR #626: Degrade on unknown annotation/region discriminatorsScope: In scope — adds exported public types New exported API surface:
Parity assessment — ✅ Aligned This PR closes an internal inconsistency in the Go SDK (annotations were stricter than content), and aligns with upstream .NET/Python semantics:
The No cross-repo parity issues found. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
Annotations.UnmarshalJSONandAnnotatedRegions.UnmarshalJSONcalledjsonx.UnmarshalDiscriminatedUnionSlice(the no-fallback variant). With a nil fallback, an unrecognized discriminator returns an error, which fails the entire enclosing message deserialization. Onlycitation/text_spanare registered, so any newer provider-emitted annotation or region subtype is fatal.This switches both to
UnmarshalDiscriminatedUnionSliceWithFallback, adding package-internalRawAnnotationandRawAnnotatedRegiontypes that preserve the original JSON and round-trip it on marshal (returning emptykind()), mirroring howContentsalready preserves unknown content viaRawContent.Why
message/content.goalready tolerates unknown content kinds throughRawContentviaunmarshalRawContent, but annotations did not — an inconsistency that makes forward-compatibility depend on which field a new subtype lands in. Aligning annotations with content matches the .NET/Python SDK semantics of gracefully degrading on unrecognized union members rather than dropping or rejecting them, preserving the raw payload for downstream inspection and faithful re-serialization.Testing
Added
TestAnnotationEncoding_UnknownTypesPreservedAsRawinmessage/annotation_test.go: unmarshals acitationannotation whoseAnnotatedRegionsholds an unknown region type, alongside an unknown top-level annotation type. It asserts no error, that both unknowns are preserved asRawAnnotatedRegion/RawAnnotation, and that they round-trip back to the original JSON.go build ./...,go vet ./message/..., andgo test ./message/...all pass.