Claude: changes for issue #45

claude-bot commented

2026-07-25 02:37:23 +00:00

Collaborator

Requested in #45.

claude-bot added 1 commit

2026-07-25 02:37:23 +00:00

fix(lexgen): make generated C# compile and type unions/tokens (closes #45 )

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

Details

c46950fd23

Dogfooding `atproto-lexgen csharp` against the published exchange.recipe
Lexicons (recipe.exchange) produced C# that did not compile. Fixes all seven
reported defects plus three more found while verifying against the 395
official atproto Lexicons:

- drop the stray `}` emitted after the file-scoped namespace
- rename members that collide with their enclosing type (CS0542), another
  member, or AtProtoRecord's Type/CreatedAt; sanitize illegal identifiers
- disambiguate identically-named defs in sibling documents (CS0101)
- emit `using ATProtoNet;`/`using ATProtoNet.Models;`/`System.Text.Json`
  only when used, plus `#nullable enable`, and root cross-namespace
  references at `global::`
- make optional members nullable (a bare JsonElement serializes as
  ValueKind.Undefined and throws)
- resolve well-known com.atproto.*/app.bsky.* refs to the SDK's own models
  (SdkTypeMap); unresolvable refs degrade to JsonElement? with a WARN
  instead of a dangling type name
- record defs subclass AtProtoRecord and inherit CreatedAt
- unions of object defs become [JsonPolymorphic] base classes with
  [JsonDerivedType] variants; unions of tokens are typed string
- inline object schemas become nested classes (arrays singularize)
- token families collapse into one static class of constants (recipe.exchange
  defs: 101 classes -> 9)
- case the `atproto` NSID segment as `AtProto`, and map `"type": "number"`
  to double

Also repeats [JsonPropertyName("$type")] on the Type override everywhere:
System.Text.Json does not inherit the attribute onto an override, so the
previously documented pattern wrote both "type" and "$type" into records.

Verified: unit tests (1010 passing, 15 new in CSharpEmitterTests), and
generated output for both the four exchange.recipe schemas and all 395
official atproto Lexicons compiles against the SDK with
TreatWarningsAsErrors, with a serialization round-trip through the union.

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

claude-bot referenced this pull request

2026-07-25 02:37:23 +00:00

lexgen csharp: codegen defects found generating exchange.recipe.* (real-world third-party lexicon) #45

claude-bot commented

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

Author

Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/47); the reply will link the full session transcript. 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-60/run-47.md) — updating every ~5 minutes while the task runs.

claude-bot commented

2026-07-25 14:35:20 +00:00

Author

Collaborator

📝 Retroactive review summary (was stuck as an invisible draft by the APPROVE/APPROVED bug, fixed in v12.2 — verdict was: approve):

Large but well-scoped fix for issue #45: the Lexicon → C# emitter is substantially rewritten (CSharpEmitter, new EmitPlan/SdkTypeMap, TypeMapper additions) to fix a long list of real compile-breaking defects (stray closing brace, CS0542 name collisions, CS0101 duplicate types, unqualified SDK refs, non-nullable JsonElement fallbacks, record defs not extending AtProtoRecord, atproto casing), plus new features (typed unions via [JsonPolymorphic], token-family grouping, nested inline-object classes). The accompanying doc/README/test-fixture updates correctly propagate the [JsonPropertyName("$type")]-on-override fix, which is a legitimate System.Text.Json gotcha (attributes on a base virtual/abstract property are not picked up on the overriding property).

I manually traced the non-trivial algorithmic pieces (EmitPlan's union-signature grouping/overlap handling, sibling-document disambiguation, token-family prefix detection, member/type collision renaming, Shorten/global:: qualification) against the 24 new tests in CSharpEmitterTests.cs and every case checks out — including subtle ones like the licenseAllRights/licenseCreativeCommons* common-prefix computation and the _2fa / kebab-case identifier sanitization. I was not able to execute dotnet test myself in this sandbox (git worktree/dotnet invocations were blocked by the approval gate), so this is a static review, but the logic is internally consistent and the test coverage is unusually thorough for a change of this size.

Two very minor, non-blocking observations not worth a code change on their own: (1) TypeMapper.ToIdentifier's C#-keyword-escaping branch ("@" + identifier) is unreachable in every actual call site, since all callers pass through ToPascalCase first, which capitalizes away every lowercase C# keyword — effectively dead code. (2) A Lexicon union whose refs list includes the very object that declares the union property (self-referential union, not covered by tests) would pass the Kind == "object" variant check and could be treated as an owning union member — plausible in principle but an unlikely real-world Lexicon shape, and the overlap-detection logic still degrades gracefully (falls back to JsonElement? with a WARN) in the tested sibling-overlap case.

⏱ 3m 27s · 14 turns · tokens: 74.3k in / 17.9k out (+1106.8k cache-read)
📜 Full session transcript

📝 **Retroactive review summary** (was stuck as an invisible draft by the APPROVE/APPROVED bug, fixed in v12.2 — verdict was: approve): Large but well-scoped fix for issue #45: the Lexicon → C# emitter is substantially rewritten (`CSharpEmitter`, new `EmitPlan`/`SdkTypeMap`, `TypeMapper` additions) to fix a long list of real compile-breaking defects (stray closing brace, CS0542 name collisions, CS0101 duplicate types, unqualified SDK refs, non-nullable `JsonElement` fallbacks, `record` defs not extending `AtProtoRecord`, `atproto` casing), plus new features (typed unions via `[JsonPolymorphic]`, token-family grouping, nested inline-object classes). The accompanying doc/README/test-fixture updates correctly propagate the `[JsonPropertyName("$type")]`-on-override fix, which is a legitimate System.Text.Json gotcha (attributes on a base virtual/abstract property are not picked up on the overriding property). I manually traced the non-trivial algorithmic pieces (`EmitPlan`'s union-signature grouping/overlap handling, sibling-document disambiguation, token-family prefix detection, member/type collision renaming, `Shorten`/`global::` qualification) against the 24 new tests in `CSharpEmitterTests.cs` and every case checks out — including subtle ones like the `licenseAllRights`/`licenseCreativeCommons*` common-prefix computation and the `_2fa` / `kebab-case` identifier sanitization. I was not able to execute `dotnet test` myself in this sandbox (git worktree/dotnet invocations were blocked by the approval gate), so this is a static review, but the logic is internally consistent and the test coverage is unusually thorough for a change of this size. Two very minor, non-blocking observations not worth a code change on their own: (1) `TypeMapper.ToIdentifier`'s C#-keyword-escaping branch (`"@" + identifier`) is unreachable in every actual call site, since all callers pass through `ToPascalCase` first, which capitalizes away every lowercase C# keyword — effectively dead code. (2) A Lexicon union whose refs list includes the very object that declares the union property (self-referential union, not covered by tests) would pass the `Kind == "object"` variant check and could be treated as an owning union member — plausible in principle but an unlikely real-world Lexicon shape, and the overlap-detection logic still degrades gracefully (falls back to `JsonElement?` with a WARN) in the tested sibling-overlap case. --- ⏱ 3m 27s · 14 turns · tokens: 74.3k in / 17.9k out (+1106.8k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-60/run-47.md)

Grandiras commented

2026-07-25 14:36:23 +00:00

Owner

@claude-bot Please fix the small conflict in the changelog.

claude-bot commented

2026-07-25 14:36:26 +00:00

Author

Collaborator

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

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/50); the reply will link the full session transcript. 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-60/run-50.md) — updating every ~5 minutes while the task runs.

claude-bot added 1 commit

2026-07-25 14:39:15 +00:00

Merge branch 'main' into claude/issue-45

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

Details

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

Details

847d2669e9

Resolves the CHANGELOG.md conflict: both sides appended to the
[Unreleased] "Added"/"Fixed" sections, so all entries are kept.

The PR's "hand-written AtProtoRecord subclasses serialized both type and
$type" bullet overlapped with the Issue #49 entry that landed on main
(which fixes the same bug at the SDK level with a JsonTypeInfo contract
modifier). Reworded it to describe what this PR actually contributes —
emitting [JsonPropertyName("$type")] on the Type override in generator
output, docs, and fixtures, so the documented pattern is also correct
under hand-built JsonSerializerOptions.

Grandiras merged commit f2aee510e3 into main

2026-07-25 14:53:51 +00:00

Grandiras deleted branch claude/issue-45

2026-07-25 14:53:52 +00:00

Grandiras referenced this pull request from a commit

2026-07-25 14:53:53 +00:00

Claude: changes for issue #45 (#60)

claude-bot commented

2026-07-25 14:55:20 +00:00

Author

Collaborator

Conflict resolved and pushed to claude/issue-45 (847d266).

What was conflicting

Only CHANGELOG.md — every other file (docs/custom-records.md, tests/ATProtoNet.Tests/RecordCollectionTests.cs) auto-merged. Both sides had appended entries to the [Unreleased] → Added and Fixed sections, so most of it was a straightforward keep-both:

Added — main's #41 / #39 / #49 bullets plus main's new ### Changed section, then this PR's typed-unions/tokens bullet.
Fixed — the #45 lexgen bullet alongside main's #41 / #49 / #46 bullets.

One judgement call worth flagging

This PR's bullet "Hand-written AtProtoRecord subclasses serialized both type and $type" describes the same bug as the Issue #49 entry that has since landed on main. #49 fixes it properly at the SDK level (AtProtoJsonDefaults.ApplyRecordTypeDiscriminator, a JsonTypeInfo contract modifier wired into the SDK's own serializer options), so a mechanical keep-both would have left the changelog describing one bug twice, one of the two as if still unfixed.

I kept both bullets but reworded this PR's to state what it actually still contributes on top of #49 — the generator, docs, and fixtures now emit [JsonPropertyName("$type")] on the Type override, so the documented pattern is correct even under hand-built JsonSerializerOptions that don't have the #49 modifier registered. That's genuinely complementary, and #49's own entry already calls that attribute "safe", so the two now read consistently. The PR's code changes were left untouched.

Verified

Installed the .NET 10 SDK in the container (it wasn't preinstalled) and ran the canonical pre-merge check against the merged tree:

dotnet build -p:EnableSourceControlManagerQueries=false — 0 errors (1160 warnings, all pre-existing CS1591 missing-XML-doc and NU1902 MessagePack advisories, none from the merge).
dotnet test tests/ATProtoNet.Tests/ — 1049 passed, 0 failed, 0 skipped.

So the semantic overlap between #45 and #49 on AtProtoRecord.Type doesn't actually break anything — the emitted attribute and the contract modifier coexist, and RecordCollectionTests passes with both in play.

⏱ 2m 53s · 21 turns · 📜 Full session transcript

(reply recovered from the transcript — the posting step in runs before v12.3 was broken by a shell-quoting bug)

Conflict resolved and pushed to **`claude/issue-45`** (`847d266`). ## What was conflicting Only `CHANGELOG.md` — every other file (`docs/custom-records.md`, `tests/ATProtoNet.Tests/RecordCollectionTests.cs`) auto-merged. Both sides had appended entries to the `[Unreleased]` → `Added` and `Fixed` sections, so most of it was a straightforward keep-both: - **`Added`** — main's #41 / #39 / #49 bullets plus main's new `### Changed` section, then this PR's typed-unions/tokens bullet. - **`Fixed`** — the #45 lexgen bullet alongside main's #41 / #49 / #46 bullets. ## One judgement call worth flagging This PR's bullet *"Hand-written `AtProtoRecord` subclasses serialized both `type` and `$type`"* describes the **same bug** as the Issue #49 entry that has since landed on main. #49 fixes it properly at the SDK level (`AtProtoJsonDefaults.ApplyRecordTypeDiscriminator`, a `JsonTypeInfo` contract modifier wired into the SDK's own serializer options), so a mechanical keep-both would have left the changelog describing one bug twice, one of the two as if still unfixed. I kept both bullets but reworded this PR's to state what it actually still contributes on top of #49 — the generator, docs, and fixtures now emit `[JsonPropertyName("$type")]` on the `Type` override, so the documented pattern is correct even under hand-built `JsonSerializerOptions` that don't have the #49 modifier registered. That's genuinely complementary, and #49's own entry already calls that attribute "safe", so the two now read consistently. The PR's code changes were left untouched. ## Verified Installed the .NET 10 SDK in the container (it wasn't preinstalled) and ran the canonical pre-merge check against the merged tree: - `dotnet build -p:EnableSourceControlManagerQueries=false` — **0 errors** (1160 warnings, all pre-existing CS1591 missing-XML-doc and NU1902 `MessagePack` advisories, none from the merge). - `dotnet test tests/ATProtoNet.Tests/` — **1049 passed, 0 failed, 0 skipped**. So the semantic overlap between #45 and #49 on `AtProtoRecord.Type` doesn't actually break anything — the emitted attribute and the contract modifier coexist, and `RecordCollectionTests` passes with both in play. --- ⏱ 2m 53s · 21 turns · 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-60/run-50.md) _(reply recovered from the transcript — the posting step in runs before v12.3 was broken by a shell-quoting bug)_

Rows
Columns

Claude: changes for issue #45 #60

What was conflicting

One judgement call worth flagging

Verified