Bring the added comments back to the prevailing style

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>

Author: allister-beamable

cli/cli/Services/Content/ContentService.cs

@@ -1157,9 +1157,7 @@ public async Task PublishContent(AutoSnapshotType autoSnapshotType, int maxLocal
 				}).ToArray()
 		};
 
-		// The platform derives a content version from the payload it stored, and returns it per item. Index it
-		// so each published file can record which remote payload its local checksum now describes. The same id
-		// comes back once per visibility, carrying the same version.
+		// The platform returns the content version it assigned to each item, repeated once per visibility.
 		var publishedVersions = saveContentResponses
 			.SelectMany(response => response.content)
 			.GroupBy(c => c.id)
@@ -1178,8 +1176,7 @@ public async Task PublishContent(AutoSnapshotType autoSnapshotType, int maxLocal
 			{
 				ContentFile contentFile = c;
 
-				// We just uploaded these exact properties, so the local checksum is now the reference for the
-				// version the platform assigned to them.
+				// We just uploaded these properties, so our checksum describes the version the platform assigned.
 				if (publishedVersions.TryGetValue(contentFile.Id, out var publishedVersion))
 				{
 					contentFile.Reference = new LocalContentReference(contentFile.PropertiesChecksum, publishedVersion);
@@ -1401,10 +1398,8 @@ public async Task<ContentSyncReport> SyncLocalContent(ClientManifestJsonResponse
 			contentFile.Tags = JsonSerializer.SerializeToElement(c.ReferenceContent.tags);
 			contentFile.FetchedFromManifestUid = targetManifestUid;
 
-			// Hash the payload we just downloaded using our own canonicalization, and record that rather than
-			// trusting the publisher-supplied manifest checksum. The file about to be written is serialized from
-			// this same element, so local and reference agree by construction regardless of how the publisher
-			// ordered, escaped or formatted its JSON.
+			// Hash the downloaded payload ourselves rather than trusting the publisher's manifest checksum. The
+			// file we are about to write is serialized from this same element, so the two agree by construction.
 			contentFile.PropertiesChecksum = CalculateChecksum(in contentFile);
 			contentFile.Reference = new LocalContentReference(contentFile.PropertiesChecksum, c.ReferenceContent.version);
 			saveTasks.Add(SaveContentFile(contentFolder, contentFile, cancellationToken));
@@ -1420,8 +1415,7 @@ public async Task<ContentSyncReport> SyncLocalContent(ClientManifestJsonResponse
 				contentFile.Tags = JsonSerializer.SerializeToElement(c.ReferenceContent.tags);
 			}
 
-			// This set also contains locally modified and locally created files, whose local bytes are NOT the
-			// remote ones, so only record a reference for the entries that actually match the target.
+			// This set also holds locally modified and created files, whose bytes are not the remote ones.
 			if (c.ReferenceContent != null && contentFile.GetStatus() == ContentStatus.UpToDate)
 			{
 				contentFile.Reference = new LocalContentReference(contentFile.PropertiesChecksum, c.ReferenceContent.version);
@@ -1987,8 +1981,6 @@ public static IEnumerable<LocalContentManifestEntry> ContentFileToLocalContentMa
 
 	/// <summary>
 	/// Reads the locally derived reference off a parsed content file, if it carries one.
-	/// Absence is meaningful rather than exceptional: files written by an older CLI simply do not have it, and
-	/// resolving that here keeps every downstream reader working with a reference that is whole or not there.
 	/// </summary>
 	private static LocalContentReference? ReadReference(in JsonElement json) =>
 		json.TryGetProperty(ContentFile.JSON_NAME_REFERENCE, out var reference)
@@ -2053,14 +2045,8 @@ public static JsonSerializerOptions GetContentFileSerializationOptions(bool inde
 		{
 			WriteIndented = indent,
 			IncludeFields = true,
-
-			// Pinned deliberately. The bytes these options produce are hashed by CalculateChecksum and the
-			// result is published as the manifest checksum, so this is a wire format rather than a style
-			// preference: changing any value here re-hashes every content item and makes this CLI disagree
-			// with every other CLI version in the realm. JavaScriptEncoder.Default is what System.Text.Json
-			// used implicitly before this was written down, so naming it changes no existing checksum.
+			// Pinned: these bytes get hashed, so a change here re-checksums every content item in the realm.
 			Encoder = JavaScriptEncoder.Default,
-
 			Converters =
 			{
 				new SortedJsonElementConverter(), new SortedSnapshotConverter()
@@ -2241,10 +2227,8 @@ public struct LocalContentFiles
 }
 
 /// <summary>
-/// A checksum this CLI computed over a remote content payload, paired with the platform content version that
-/// says which payload it was. Neither half means anything on its own -- a checksum without its version would
-/// let a stale reference mask a genuine remote change -- so the two are only ever constructed together and an
-/// absent reference is represented by a null, not by empty strings.
+/// A checksum this CLI computed over a remote content payload, paired with the content version identifying
+/// which payload it was. The version is required: without it a stale checksum can mask a real remote change.
 /// </summary>
 [Serializable]
 public struct LocalContentReference
@@ -2262,8 +2246,7 @@ public LocalContentReference(string checksum, string version)
 	}
 
 	/// <summary>
-	/// Reads a reference from the object a content file stores it under, yielding one only when both halves
-	/// are present. A file carrying half a reference is treated as carrying none.
+	/// Reads a reference, yielding one only when both halves are present.
 	/// </summary>
 	public static bool TryRead(in JsonElement json, out LocalContentReference reference)
 	{
@@ -2281,8 +2264,7 @@ public static bool TryRead(in JsonElement json, out LocalContentReference refere
 	}
 
 	/// <summary>
-	/// True when this reference was taken from the very payload <paramref name="remote"/> points at, which is
-	/// the only situation where comparing a local checksum against it means anything.
+	/// True when this reference was taken from the payload <paramref name="remote"/> points at.
 	/// </summary>
 	public bool Describes(ClientContentInfoJson remote) => Version == remote.version;
 }
@@ -2312,9 +2294,9 @@ public struct ContentFile : IEquatable<ContentFile>
 	public string FetchedFromManifestUid;
 
 	/// <summary>
-	/// What the remote payload hashed to when we last synced it, as computed by this CLI rather than supplied
-	/// by whoever published it. Null on files written before this existed, and on files restored from a
-	/// snapshot, both of which fall back to comparing against the manifest checksum.
+	/// What the remote payload hashed to when we last synced it, computed by this CLI rather than supplied by
+	/// the publisher. Null on files written before this existed and on snapshot restores, which fall back to
+	/// the manifest checksum.
 	/// </summary>
 	[JsonPropertyName(JSON_NAME_REFERENCE)]
 	[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
@@ -2333,10 +2315,9 @@ public ContentStatus GetStatus()
 
 	/// <summary>
 	/// Compares the local properties against the remote ones we last synced.
-	/// Prefers <see cref="Reference"/>, which both sides of the comparison canonicalized the same way, and which
-	/// is only usable for the remote payload it was taken from. Otherwise falls back to the publisher-supplied
-	/// manifest checksum, which is the historical behaviour and is only correct when the publisher happened to
-	/// match our serialization.
+	/// Prefers <see cref="Reference"/> when it describes the manifest entry in hand, since both sides of that
+	/// comparison were canonicalized the same way. Otherwise falls back to the publisher-supplied manifest
+	/// checksum, which is the historical behavior.
 	/// </summary>
 	private bool IsPropertiesDiff() => Reference.HasValue && Reference.Value.Describes(ReferenceContent)
 		? Reference.Value.Checksum != PropertiesChecksum
@@ -2470,8 +2451,7 @@ private static void WriteSortedJsonElement(Utf8JsonWriter writer, JsonElement el
 				writer.WriteStartObject();
 				foreach (var property in element.EnumerateObject()
 				         .OrderBy(p => p.Name, StringComparer.OrdinalIgnoreCase)
-				         // OrderBy is a stable sort, so without this two keys that differ only in case would
-				         // keep whatever order they arrived in -- exactly the instability we are removing.
+				         // OrderBy is stable, so without this, keys differing only in case keep arrival order.
 				         .ThenBy(p => p.Name, StringComparer.Ordinal))
 				{
 					writer.WritePropertyName(property.Name);

cli/tests/ContentChecksumTests/ContentChecksumTest.cs

@@ -6,8 +6,7 @@ namespace tests.ContentChecksumTests;
 
 /// <summary>
 /// Guards the canonical form that <see cref="ContentService.CalculateChecksum(string)"/> hashes.
-/// The resulting hash is published as the manifest checksum, so these are wire-format tests: a failure
-/// here means this CLI no longer agrees with content published by any other version of it.
+/// The hash is published as the manifest checksum, so these are wire-format tests.
 /// </summary>
 public class ContentChecksumTest
 {
@@ -42,8 +41,7 @@ public void Checksum_IgnoresNestedFieldOrder()
 	[Test]
 	public void Checksum_IsStableForKeysDifferingOnlyInCase()
 	{
-		// OrderBy is a stable sort, so without an Ordinal tiebreak these two inputs would sort into
-		// different orders and hash differently despite carrying identical values.
+		// Without an Ordinal tiebreak these sort differently despite carrying identical values.
 		var a = ChecksumOf("""{"Damage":{"data":1},"damage":{"data":2}}""");
 		var b = ChecksumOf("""{"damage":{"data":2},"Damage":{"data":1}}""");
 
@@ -72,22 +70,19 @@ public void Checksum_DistinguishesDifferentValues()
 	[Test]
 	public void SerializationOptions_EscapeNonAsciiAsTheyAlwaysHave()
 	{
-		// Pinning the encoder must not have changed the bytes we hash. If this fails, every checksum in
-		// every realm published by an older CLI has just become wrong.
+		// Pinning the encoder must not have changed the bytes we hash.
 		var json = JsonSerializer.Serialize(
 			JsonSerializer.Deserialize<JsonElement>("""{"name":"Café & Co"}"""),
 			ContentService.GetContentFileSerializationOptions(false));
 
-		// JavaScriptEncoder.Default escapes non-ASCII and the HTML-sensitive characters. That is not
-		// pretty, but it is what every previously published checksum was computed over.
+		// JavaScriptEncoder.Default escapes non-ASCII and HTML-sensitive characters, as it always has.
 		Assert.That(json, Is.EqualTo("""{"name":"Caf\u00E9 \u0026 Co"}"""));
 	}
 
 	[Test]
 	public void Checksum_PreservesRawNumberText()
 	{
-		// Numbers are replayed as written rather than normalized, which keeps the float-versus-int
-		// distinction that Unity schemas rely on. Documented here so the behaviour is deliberate.
+		// Numbers are replayed as written, keeping the float-versus-int distinction Unity schemas rely on.
 		var a = ChecksumOf("""{"price":{"data":1.0}}""");
 		var b = ChecksumOf("""{"price":{"data":1}}""");
 

cli/tests/ContentChecksumTests/ContentStatusTest.cs

@@ -11,8 +11,6 @@ namespace tests.ContentChecksumTests;
 
 /// <summary>
 /// Covers what <see cref="ContentFile.GetStatus"/> trusts when deciding that content was modified.
-/// The point of these is that a publisher supplying a checksum we cannot reproduce must not make
-/// value-identical content look modified forever.
 /// </summary>
 public class ContentStatusTest
 {
@@ -63,8 +61,7 @@ private ContentFile MakeFile(string manifestChecksum, LocalContentReference? ref
 	[Test]
 	public void UpToDate_WhenPublisherChecksumIsUnreproducible_ButLocalReferenceMatches()
 	{
-		// The reported bug: an external tool published a checksum over differently ordered JSON. The values
-		// are identical, so this must read as clean.
+		// The reported bug: a publisher's checksum taken over differently ordered JSON. The values match.
 		var file = MakeFile("a-checksum-we-could-never-compute", ReferenceTo(RemoteVersion));
 
 		Assert.That(file.GetStatus(), Is.EqualTo(ContentStatus.UpToDate));
@@ -73,8 +70,7 @@ public void UpToDate_WhenPublisherChecksumIsUnreproducible_ButLocalReferenceMatc
 	[Test]
 	public void Modified_WhenTheRemotePayloadMovedOnSinceWeRecordedOurReference()
 	{
-		// Our locally derived reference describes the payload we downloaded. If the remote has since changed,
-		// that reference says nothing about the new one and must not be allowed to mask a real change.
+		// Our reference describes the payload we downloaded; it must not mask a change to a newer one.
 		var file = MakeFile("a-checksum-we-could-never-compute", ReferenceTo("an-older-remote-version"));
 
 		Assert.That(file.GetStatus(), Is.EqualTo(ContentStatus.Modified));
@@ -91,7 +87,7 @@ public void Modified_WhenLocalPropertiesActuallyDiffer()
 	[Test]
 	public void FallsBackToPublisherChecksum_WhenFileHasNoLocalReference()
 	{
-		// Files written before referenceChecksum existed keep the historical comparison until their next sync.
+		// Files written before the reference existed keep the historical comparison until their next sync.
 		var matching = MakeFile(OurChecksum, null);
 		var notMatching = MakeFile("a-checksum-we-could-never-compute", null);
 
@@ -105,8 +101,7 @@ public void FallsBackToPublisherChecksum_WhenFileHasNoLocalReference()
 	[TestCase("""{}""", TestName = "both missing")]
 	public void ReferenceIsNotRead_WhenEitherHalfIsMissing(string referenceJson)
 	{
-		// Half a reference is not a reference: a checksum without its version cannot say which payload it
-		// describes. Rejecting it here is what makes the half-populated state unrepresentable everywhere else.
+		// Half a reference is not a reference: a checksum without its version names no payload.
 		var json = JsonSerializer.Deserialize<JsonElement>(referenceJson);
 
 		Assert.That(LocalContentReference.TryRead(in json, out _), Is.False);
@@ -115,8 +110,7 @@ public void ReferenceIsNotRead_WhenEitherHalfIsMissing(string referenceJson)
 	[Test]
 	public void SerializedFile_NestsTheReferenceUnderOneKey()
 	{
-		// Pins the on-disk shape. The two halves live under one key so that a file cannot express half a
-		// reference, which is the same invariant LocalContentReference enforces in memory.
+		// Pins the on-disk shape: both halves under one key, so a file cannot express half a reference.
 		var file = MakeFile(null, new LocalContentReference("the-checksum", "the-version"));
 
 		var json = JsonSerializer.Serialize(file, ContentService.GetContentFileSerializationOptions(false));