Claude: changes for issue #46

claude-bot commented

2026-07-25 02:15:05 +00:00

Collaborator

Requested in #46.

claude-bot added 1 commit

2026-07-25 02:15:05 +00:00

fix: remove UnionJsonConverterFactory so IAtProtoUnion is serializable (closes #46 )

CI / build-and-test (pull_request) Successful in 38s

Details

cb9ab92dfa

UnionJsonConverterFactory claimed every type assignable to IAtProtoUnion from
CanConvert but returned null from CreateConverter, which System.Text.Json
rejects:

  InvalidOperationException: The converter
  'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value.

So marking a custom open-union base with the marker interface — as the docs
suggest — threw on every read and write of any record containing it. The SDK's
own unions never hit this because they use [JsonPolymorphic]/[JsonDerivedType]
without implementing the marker.

The factory never had any behaviour: union discrimination comes entirely from
the polymorphism attributes plus LexiconTypeRegistry.RegisterUnionVariant. It is
removed from both AtProtoJsonDefaults.Options and
LexiconTypeRegistry.CreateOptions(). IAtProtoUnion stays as a behaviour-free
documentation marker, now XML-documented as such.

Adds AtProtoUnionMarkerTests (6 regression tests, all failing before the change)
and a "Union Types" section in docs/custom-records.md covering closed unions via
attributes and open unions via runtime registration.

claude-bot referenced this pull request

2026-07-25 02:15:06 +00:00

UnionJsonConverterFactory breaks serialization of any IAtProtoUnion implementor (CanConvert true, CreateConverter null) #46

Grandiras commented

2026-07-25 02:17:26 +00:00

Owner

@claude-bot Please review.

claude-bot commented

2026-07-25 02:17:31 +00:00

Author

Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/38); the reply will link the full session transcript.

claude-bot added 1 commit

2026-07-25 02:21:48 +00:00

docs: correct the open-union guidance in custom-records.md

CI / build-and-test (pull_request) Successful in 39s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 5s

Details

62fbc4b7f3

RegisterUnionVariant only augments a type that already carries
[JsonPolymorphic]; without it the registration is silently ignored (writes
emit {}, reads throw NotSupportedException). And CreateOptions() is not the
options instance GetCollection<T> uses, so runtime-registered variants do not
reach RecordCollection<T>. Both caveats are now stated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude-bot commented

2026-07-25 02:22:09 +00:00

Author

Collaborator

Reviewed the branch, verified the fix empirically, and pushed one follow-up commit.

Verdict: the fix is correct

The bug is real and the diagnosis is exact. I reverted the two source files to main, kept the new test file, and ran it — all 6 tests fail with precisely the exception the CHANGELOG cites:

System.InvalidOperationException : The converter
'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value.

On the branch, all 6 pass. So these are genuine regression tests, not tests written to match the new code.

Removing the factory is the right call, not a workaround. CanConvert claimed every IAtProtoUnion implementor and CreateConverter returned null — System.Text.Json rejects that combination outright, so the factory could never have worked for any type it claimed. Union discrimination genuinely comes from [JsonPolymorphic]/[JsonDerivedType] plus LexiconTypeRegistry.ApplyUnionVariants; the factory contributed nothing.

Blast radius is contained. I grepped the whole repo: no SDK type implemented IAtProtoUnion, so the breakage only ever hit user code that opted into the marker — which matches issue #46. UnionJsonConverterFactory was internal, so its removal is not a public-API break, and IAtProtoUnion stays public. Classifying this under Fixed rather than Breaking changes is correct.

Build and tests: dotnet build → 0 errors; the new <see cref="LexiconTypeRegistry.RegisterUnionVariant{TBase, TDerived}(string)"/> resolves with no CS1574. Full unit suite: 1000 passed, 0 failed.

Two problems I found in the new docs section — fixed in `62fbc4b`

The PR's new "Union Types" section told readers to use LexiconTypeRegistry.Instance.RegisterUnionVariant(...) for open unions. Both halves of that advice were misleading, so I corrected them on the branch rather than just flagging them:

RegisterUnionVariant silently no-ops without [JsonPolymorphic] on the base. ApplyUnionVariants returns early when typeInfo.PolymorphismOptions is null, which is exactly the case for the "truly open, no compile-time arms" union a reader would picture. I confirmed with a scratch test: serialization emits {"u":{}} — all derived properties silently dropped — and deserialization throws NotSupportedException: Deserialization of interface or abstract types is not supported. Silent data loss on write is the worse half.
CreateOptions() is not the options instance the documented API uses. RecordCollection<T> hardcodes AtProtoJsonDefaults.Options at four call sites, as do RepoClient and XrpcClient, and AtProtoJsonDefaults never consults the registry. GetCollection<T> takes no options parameter, so there is no way to feed the registry's options in. The recipe sat in a document whose entire subject is GetCollection<T>, where it cannot take effect. Runtime registration does work for Jetstream's typed decoding (JetstreamModels.cs:163) and for direct JsonSerializer calls — the docs now say exactly that.

Neither is a regression from this PR; both are pre-existing registry limitations. But this PR is what points users at that path, so the caveats belong with it.

One follow-up worth its own issue

The real asymmetry is that AtProtoJsonDefaults.Options and LexiconTypeRegistry.CreateOptions() are now near-identical except that only the latter carries the ApplyUnionVariants modifier — so plugin-registered union variants never reach the main client path. Wiring the registry's TypeInfoResolver into AtProtoJsonDefaults would close that, but it has subtle caching semantics (AtProtoJsonDefaults.Options is a ??=-cached singleton, so late registrations wouldn't apply, whereas the registry hands back a fresh options object each time it's invalidated). That's a behavior change beyond issue #46's scope, so I documented the limitation instead of changing it unilaterally.

Branch claude/issue-46 is updated and green; the change is docs-only, so no CHANGELOG entry was added per the repo's doc-only exemption — and the existing #46 entry already covers the docs section.

⏱ 4m 29s · 25 turns · tokens: 37.9k in / 11.7k out (+948.0k cache-read)
📜 Full session transcript

Reviewed the branch, verified the fix empirically, and pushed one follow-up commit. ## Verdict: the fix is correct **The bug is real and the diagnosis is exact.** I reverted the two source files to `main`, kept the new test file, and ran it — all 6 tests fail with precisely the exception the CHANGELOG cites: ``` System.InvalidOperationException : The converter 'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value. ``` On the branch, all 6 pass. So these are genuine regression tests, not tests written to match the new code. **Removing the factory is the right call, not a workaround.** `CanConvert` claimed every `IAtProtoUnion` implementor and `CreateConverter` returned `null` — System.Text.Json rejects that combination outright, so the factory could never have worked for any type it claimed. Union discrimination genuinely comes from `[JsonPolymorphic]`/`[JsonDerivedType]` plus `LexiconTypeRegistry.ApplyUnionVariants`; the factory contributed nothing. **Blast radius is contained.** I grepped the whole repo: no SDK type implemented `IAtProtoUnion`, so the breakage only ever hit user code that opted into the marker — which matches issue #46. `UnionJsonConverterFactory` was `internal`, so its removal is not a public-API break, and `IAtProtoUnion` stays public. Classifying this under `Fixed` rather than `Breaking changes` is correct. **Build and tests:** `dotnet build` → 0 errors; the new `<see cref="LexiconTypeRegistry.RegisterUnionVariant{TBase, TDerived}(string)"/>` resolves with no CS1574. Full unit suite: **1000 passed, 0 failed**. ## Two problems I found in the new docs section — fixed in `62fbc4b` The PR's new "Union Types" section told readers to use `LexiconTypeRegistry.Instance.RegisterUnionVariant(...)` for open unions. Both halves of that advice were misleading, so I corrected them on the branch rather than just flagging them: 1. **`RegisterUnionVariant` silently no-ops without `[JsonPolymorphic]` on the base.** `ApplyUnionVariants` returns early when `typeInfo.PolymorphismOptions is null`, which is exactly the case for the "truly open, no compile-time arms" union a reader would picture. I confirmed with a scratch test: serialization emits `{"u":{}}` — **all derived properties silently dropped** — and deserialization throws `NotSupportedException: Deserialization of interface or abstract types is not supported`. Silent data loss on write is the worse half. 2. **`CreateOptions()` is not the options instance the documented API uses.** `RecordCollection<T>` hardcodes `AtProtoJsonDefaults.Options` at four call sites, as do `RepoClient` and `XrpcClient`, and `AtProtoJsonDefaults` never consults the registry. `GetCollection<T>` takes no options parameter, so there is no way to feed the registry's options in. The recipe sat in a document whose entire subject is `GetCollection<T>`, where it cannot take effect. Runtime registration does work for Jetstream's typed decoding (`JetstreamModels.cs:163`) and for direct `JsonSerializer` calls — the docs now say exactly that. Neither is a regression from this PR; both are pre-existing registry limitations. But this PR is what points users at that path, so the caveats belong with it. ## One follow-up worth its own issue The real asymmetry is that `AtProtoJsonDefaults.Options` and `LexiconTypeRegistry.CreateOptions()` are now near-identical except that only the latter carries the `ApplyUnionVariants` modifier — so plugin-registered union variants never reach the main client path. Wiring the registry's `TypeInfoResolver` into `AtProtoJsonDefaults` would close that, but it has subtle caching semantics (`AtProtoJsonDefaults.Options` is a `??=`-cached singleton, so late registrations wouldn't apply, whereas the registry hands back a fresh options object each time it's invalidated). That's a behavior change beyond issue #46's scope, so I documented the limitation instead of changing it unilaterally. Branch `claude/issue-46` is updated and green; the change is docs-only, so no CHANGELOG entry was added per the repo's doc-only exemption — and the existing #46 entry already covers the docs section. --- ⏱ 4m 29s · 25 turns · tokens: 37.9k in / 11.7k out (+948.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-55/run-38.md)

Grandiras merged commit 55cd5ccb91 into main

2026-07-25 02:22:46 +00:00

Grandiras deleted branch claude/issue-46

2026-07-25 02:22:46 +00:00

Grandiras referenced this pull request from a commit

2026-07-25 02:22:47 +00:00

Claude: changes for issue #46 (#55)

claude-bot referenced this pull request

2026-07-25 02:26:33 +00:00

Claude: changes for issue #49 #57

Rows
Columns

Claude: changes for issue #46 #55

Verdict: the fix is correct

Two problems I found in the new docs section — fixed in 62fbc4b

One follow-up worth its own issue

Two problems I found in the new docs section — fixed in `62fbc4b`