Derive content reference checksums locally instead of trusting publishers - #4840
Open
allister-beamable wants to merge 6 commits into
Open
allister-beamable wants to merge 6 commits into
allister-beamable wants to merge 6 commits into
Conversation
Contributor
|
Lightbeam link |
Contributor
|
Lightbeam link |
allister-beamable
marked this pull request as ready for review
September 15, 2026 15:28
Contributor
|
Lightbeam link |
allister-beamable
requested review from
DiasAtBeamable,
Leinnan and
PedroRauizBeamable
September 17, 2026 18:32
…hers Content status was decided by comparing a checksum we computed over our own canonical serialization against a string lifted verbatim out of the remote manifest. Nothing recomputes or validates that string, so the real contract was never "sort your keys" but "reproduce System.Text.Json byte for byte under one undocumented options object, then SHA1 it". A game maker publishing through their own tooling cannot honor that except by accident, and when they do not, every item shows a permanent modified badge that survives a fetch. Content files now carry referenceChecksum, recording what the remote payload hashes to under our canonicalization, alongside referenceVersion, recording which remote payload that hash describes. The version pairing matters: without it a file synced from an older manifest would match its stale reference against a newer manifest entry and report a genuinely changed remote as up to date. Files carrying neither fall back to the historical comparison. Also pin the serializer options that define this canonical form. The encoder is named explicitly as the value System.Text.Json was already using implicitly, so no existing checksum changes; it simply can no longer drift underneath us. The key sort gains an Ordinal tiebreak, because OrderBy is stable and keys differing only in case were otherwise left in whatever order they arrived in, which is the instability the converter exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two stacked TryGetProperty ternaries in an initializer read badly. The absence they encode is meaningful -- a content file written by an older CLI does not carry these fields -- so resolving it at the parse boundary is right; only the syntax was clumsy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two halves are meaningless apart: a checksum without the version it was taken from would let a stale reference mask a genuine remote change. Modelling them as two nullable strings left that illegal state representable and pushed the burden onto a guard at every read. LocalContentReference can only be constructed whole, TryRead yields nothing unless both halves are present, and the file nests them under one key, so half a reference is now unrepresentable rather than defended against. Absence is a null instead of a pair of empty strings, which is also what the on-disk format now says: the key is omitted entirely when there is no reference. The half-populated status test moved down to TryRead, which is the only place that state can still arrive. Two more tests pin the on-disk shape so it cannot drift silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The explanatory comments added with the reference checksum ran far longer than anything around them. ContentService.cs keeps its inline comments to a line or two and its xmldoc to a plain statement of purpose, and the new test files sat beside neighbors carrying almost no comments at all. Most of what came out was argument rather than description: six comments stated the consequence of getting the code wrong, which belongs in the pull request a reviewer reads once, not in source every later reader reads again. Three xmldoc summaries lost a second paragraph justifying the design under a heading that elsewhere only says what a thing does. The version pairing on LocalContentReference and the two comparisons behind IsPropertiesDiff keep their explanations, being the parts a reader cannot recover from the code. Also corrects "behaviour" to "behavior" and a stale reference to referenceChecksum, which has been named reference since it was bundled with its version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
allister-beamable
force-pushed
the
fix/content-reference-checksum
branch
from
September 17, 2026 20:14
3fdbb9e to
14d5289
Compare
Contributor
|
Lightbeam link |
Both sorted converters arrived together in a42d8f0, whose changelog entry records the only rationale: sorting "makes viewing snapshots in VCS-diff-viewers easier." OrdinalIgnoreCase is the choice that keeps the capitals from clumping ahead of the lowercase keys, which is a display preference and nothing more. The property sort was later promoted into the input of a hash without anyone re-deriving its requirements, and a comparer that merely needs to look tidy is not the same as one that needs to be a total order. That half is fixed earlier in this branch. The snapshot sort was left alone because its bytes are not hashed, so a tie there cannot produce a wrong status. It is still worth fixing on its own terms: content ids differing only in case flip-flop according to whatever order the dictionary happened to yield, so an incidental reordering turns a one-line diff into a two-line one. Stability is strictly better than instability even when the only thing at stake is the diff. Plain Ordinal is what either site would use on a blank slate -- total, culture-independent, no tie-breaker needed -- but switching the property sort to it now would reorder the hashed bytes and re-checksum every content item in every realm. The tie-breaker buys totality while leaving every existing checksum alone. The in-code comments stay short and name only the peculiarity, since the reasoning belongs here rather than in the source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Lightbeam link |
The tiebreak added alongside it had no coverage, so a later edit could quietly restore the flip-flop it was meant to remove. Three cases: insertion order does not reach the output, ids differing only in case land the same way regardless of arrival order, and the comparer is still case-insensitive so capitalized ids do not clump ahead of lowercase ones. That last one guards the reason OrdinalIgnoreCase was chosen in the first place, which is otherwise the sort of detail a cleanup would file off as redundant next to the Ordinal tiebreak. Verified the middle case fails without the tiebreak rather than passing either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Lightbeam link |
This branch has not been deployed
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
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.
Closes #4838.
The problem
ContentFile.GetStatus()decided whether content was modified by comparing two values that are not comparable:PropertiesChecksum, computed locally byCalculateChecksumover our own canonical serialization, andReferenceContent.checksum, a string lifted verbatim out of the remote manifest.The second one is whatever the publisher put there. Nothing recomputes or validates it. So the real contract was never "sort your keys" — it was "byte-for-byte reproduce
System.Text.Jsonunder one undocumented options object, then SHA1 it." A Game Maker publishing through their own tooling cannot honor that except by accident, and when they do not, every item shows a permanent "modified" badge that survives a Fetch.Field order is only one of the ways to miss. Others include string escaping (
JavaScriptEncoder.DefaultrendersCafé & CoasCaf\u00E9 \u0026 Co), raw number text (1.0and1both round-trip as written), and keys differing only in case.The change
Stop trusting the publisher's checksum. Content files gain a
referenceobject holding what the remote payload hashed to under our canonicalization, together with the platform content version identifying which payload that was:GetStatus()compares against that checksum when the version matches the manifest entry being compared against, and falls back to the old comparison otherwise. Both sides of the comparison then go through the same serializer, so the result no longer depends on how the publisher formatted its JSON.The version pairing is load-bearing rather than decorative. The platform derives a content version from the payload itself, so it identifies exactly which remote bytes our checksum describes. Without it, a file synced from an older manifest would compare its stale reference against a newer manifest entry, match, and report a genuinely changed remote as up to date — trading a false positive for a false negative, which is much worse. There is a test for exactly that.
The two halves are meaningless apart, so they are never apart.
LocalContentReferencecan only be constructed whole,TryReadyields nothing unless both halves are present, and the JSON nests them under one key, so a half-populated reference is unrepresentable rather than guarded against. Absence is a null, not a pair of empty strings.The reference is written when we download content, when content we publish becomes the new remote (using the version the platform returns on the save response), and when a file that genuinely matches the target has its manifest reference moved forward. Files without one fall back to the old manifest-checksum comparison, so nothing changes for existing workspaces until their next sync.
Pin the serializer options.
GetContentFileSerializationOptionsnow namesEncoder = JavaScriptEncoder.Defaultexplicitly. This is whatSystem.Text.Jsonwas already doing implicitly, so no existing checksum changes — it just can no longer drift with a .NET upgrade, and the comment says why that matters.Make the key sort a total order.
OrderByis stable over input ordering, so two keys differing only in case previously kept whatever order they arrived in, which is exactly the instability the converter exists to remove. Both sorted converters get anOrdinaltiebreak. InSortedJsonElementConverterthe tie was a correctness problem, because its output is hashed. InSortedSnapshotConverterit was not -- snapshot bytes are not hashed -- but an incidental reordering there turns a one-line diff into a two-line one, which defeats the readability the sort was introduced for. No existing checksum changes, since only case-colliding keys reorder.Deliberately not done
1as1.0can still read as modified. Covered by a test that documents the behavior as intentional.& < > ', which means a forced re-sync and skew against teammates on older CLI versions. Worth doing deliberately with a migration, not as a side effect of this fix.Server-side half, not addressed here
Content versions are derived from a hash of unsorted JSON on the platform side as well, so a value-identical republish with different key order mints a new content version, a new stored object, and a new manifest. That means an unstable publisher still causes churn and redundant client downloads even with this fix in place. It is filed separately on the backend tracker; it needs a migration plan because existing stored object keys derive from the current hash, and it is not something the CLI can fix on its own.
Tests
ContentChecksumTestcovers the canonical form as a wire format: top-level and nested field order are ignored, case-only key collisions order deterministically, array order is still significant, different values still differ, the pinned encoder still escapes exactly as before, and raw number text is still preserved.ContentStatusTestcovers whatGetStatus()trusts: an unreproducible publisher checksum no longer marks value-identical content modified, a remote that moved on since we recorded our reference still reads as modified, genuinely different local properties still read as modified, and a file carrying no reference falls back to the historical comparison. It also pins the on-disk shape — the reference nested under one key, and omitted entirely when absent — and checks that a half-written reference is refused at parse time.The tiebreak added to
SortedSnapshotConverteris not separately covered. No existing test pins snapshot key order, and its output is not hashed, so a regression there would show up as diff churn rather than as wrong status. Say the word if you would rather it were pinned.Notes for review
Task.WhenAll, against a sync that issues one CDN request per item. If it ever needs to be cheaper,CalculateChecksumcurrently serializes to astringand then callsEncoding.UTF8.GetBytes; serializing into a pooledArrayBufferWriter<byte>and hashing the span measured about 37% faster at that size.ContentObject.cssits inside#if UNITY_EDITOR, and the CLI is not shipped to devices.🤖 Generated with Claude Code