[hahn-kev-bot]

---

AI summary

Summary

Harmony replays every change on every client (and again whenever snapshots are regenerated). A fresh Guid or clock read inside a change type during replay gives each client its own value, which shows up as sync divergence rather than an error. The example sentence JSON patch did exactly that; #2813 fixes it, and this PR adds analyzer rule LX0002 so the next one fails the build instead.

IChange implementation
  instance constructors          -> exempt (run once on the originating client; replay gets serialized values)
  everything else                -> checked
    ApplyChange / NewEntity
    private helpers they call    <- the shape of the original bug
    property / field initializers
    lambdas, local functions, nested helper types, abstract bases

Deny list (matched by declaring type + member name, so members missing on a TFM never match):

Type	Banned
Guid	NewGuid, CreateVersion7
DateTime	Now, UtcNow, Today
DateTimeOffset	Now, UtcNow
TimeProvider	System, GetUtcNow, GetLocalNow, GetTimestamp
Random, RandomNumberGenerator, Stopwatch, Environment	every member
Path	GetRandomFileName, GetTempFileName

 backend/Analyzers/LexboxAnalyzers/
+├── HarmonyChangeTypes.cs                          # shared IChange detection, used by LX0001 and LX0002
 ├── DiagnosticIds.cs                               # + LX0002
 └── Rules/
     ├── ChangeEntityIdConstructorAnalyzer.cs       # now uses HarmonyChangeTypes
+    └── ChangeNonDeterministicCallAnalyzer.cs      # LX0002
 .editorconfig                                      # LX0002 = error

Deliberate exceptions can be suppressed with #pragma warning disable LX0002 plus a justification.

Evidence

• Before: building LcmCrdt on develop with the rule reports exactly one error, at the original Guid.NewGuid() in JsonPatchExampleSentenceChange.ApplyTranslationOp, and nothing else (the Guid.NewGuid() / DateTimeOffset.UtcNow uses in the create-change constructors are correctly exempt).
  After: stacked on Add major FW Lite CRDT model version to regenerate snapshots #2813, which replaces that call with a deterministic translation, LcmCrdt builds with 0 errors.
• LexboxAnalyzers.Tests: 24 passed (9 existing LX0001, 15 new LX0002), covering the helper-method shape of the original bug, initializers, lambdas/local functions/method groups, nested helper types, abstract bases, constructor exemption, deterministic members of the banned types, nameof, no-Harmony compilations, and generated code.

Merge Danger

Door: two-way

Build-time only; no runtime code changes. Revert the rule or lower the severity in .editorconfig to walk it back.

Blast Radius: build

Any backend compilation that references Harmony. Today nothing outside LcmCrdt/Changes implements IChange, and after #2813 that directory is clean, so no existing code is flagged.

Test plan

• dotnet test backend/Analyzers/LexboxAnalyzers.Tests — 24 passed
• dotnet build backend/FwLite/LcmCrdt/LcmCrdt.csproj on develop flags only the known Guid.NewGuid() line; on this branch (merged with Add major FW Lite CRDT model version to regenerate snapshots #2813) it builds with 0 errors
• CI green on the stacked branch

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 56e9353f-12d1-41ad-9139-53bd8a0b9ce7

Commits

Reviewing files that changed from the base of the PR and between a198963 and f4af2ef.

Files selected for processing: 7

• .editorconfig
• backend/Analyzers/LexboxAnalyzers.Tests/ChangeNonDeterministicCallAnalyzerTests.cs
• backend/Analyzers/LexboxAnalyzers/DiagnosticIds.cs
• backend/Analyzers/LexboxAnalyzers/HarmonyChangeTypes.cs
• backend/Analyzers/LexboxAnalyzers/Rules/ChangeEntityIdConstructorAnalyzer.cs
• backend/Analyzers/LexboxAnalyzers/Rules/ChangeNonDeterministicCallAnalyzer.cs
• backend/Analyzers/README.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The pull request adds the LX0002 analyzer for nondeterministic API use in Harmony change types outside instance constructors. It adds shared Harmony change-type detection and updates an existing analyzer to use it. Tests cover reporting and exclusion cases. The rule is configured as an error and documented in the analyzer README.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Possibly related PRs

• sillsdev/languageforge-lexbox#2813: Removes a randomly generated translation ID from JsonPatchExampleSentenceChange; both PRs address nondeterminism in CRDT change logic.

Suggested reviewers: myieye

Merge Risk: Minimal · up to f4af2

No actionable issue is established; the PR is mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: adding an analyzer that bans non-deterministic calls in CRDT change types.
Description check		The description directly explains the analyzer rule, its scope, exclusions, deny list, configuration, tests, and expected build impact.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 26 functions across 5 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each change with care
No stray clock can settle there
A Guid may wait outside the door
While tests inspect each case and more
LX0002 marks the paths it sees
Then hops away beneath the trees}

_{Comment @coderabbitai help to get the list of available commands.}