[claude] Bump SIL.Harmony to rc.228 and fix config-time freeze - #2463
Closed
myieye wants to merge 1 commit into
Closed
[claude] Bump SIL.Harmony to rc.228 and fix config-time freeze#2463myieye wants to merge 1 commit into
myieye wants to merge 1 commit into
Conversation
rc.228 makes CrdtConfig.JsonSerializerOptions a lazy that freezes ChangeTypeListBuilder when built. The old code read it inside ConfigureCrdt before remote-resource change types were registered, throwing "ChangeTypeListBuilder is frozen". Move the JSON options finalization out of ConfigureCrdt into a single PostConfigure (runs after every Configure) so all change types are registered first; ConfigureCrdt is now pure type registration. Also update DataModelSnapshotTests to read change discriminators from the new synthetic $type property, since rc.228 drops built-in IChange PolymorphismOptions for a converter. The verified snapshot is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
💻 FW Lite
myieye
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdates Harmony package versions, changes CRDT JSON options finalization timing, moves remote resource registration, and revises change-model snapshot verification to derive polymorphic discriminators from concrete change types. ChangesCRDT JSON polymorphism and configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
hahn-kev
reviewed
Jul 22, 2026
| var discriminatorProperty = _jsonSerializerOptions.GetTypeInfo(changeType).Properties | ||
| .SingleOrDefault(p => p.Name == CrdtConstants.ChangeDiscriminatorProperty); | ||
| discriminatorProperty.Should().NotBeNull("change type {0} should declare a $type discriminator", changeType); | ||
| var discriminator = discriminatorProperty!.Get!(RuntimeHelpers.GetUninitializedObject(changeType)); |
Collaborator
There was a problem hiding this comment.
we should probably make a helper function that does this, as I think we've done this in other places already and each one is a little different.
Collaborator
Author
|
We want to make bigger changes on the Harmony side, so closing this. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Claude, autonomous]
Bumps SIL.Harmony 0.2.1-rc.211 → rc.228.
rc.228 makes
CrdtConfig.JsonSerializerOptionsa lazy that freezesChangeTypeListBuilderwhen built, so reading it insideConfigureCrdt(before remote-resource change types are registered) threwChangeTypeListBuilder is frozen. Moved the JSON-options finalization into a singlePostConfigurethat runs after everyConfigure;ConfigureCrdtis now pure type registration.DataModelSnapshotTests.VerifyChangeModelsnow reads change discriminators from the synthetic$typeproperty (rc.228 dropped built-inIChangePolymorphismOptionsfor a converter). Verified snapshot is byte-identical, so the registered change set and discriminators are unchanged — on-disk/wire format stays compatible.harmony-sentinel reviewed: no data-loss or format-compat risk (commit hash chain doesn't depend on change payload;
$typename/value/position unchanged).