LT-22711: Fix affix process rule corruption when splitting a sense into a new entry

[johnml1135]

Splitting a sense into a new entry no longer corrupts the affix process rule on the copy. MoAffixProcess.PostClone was leaving stray default items on the clone, deleting real ones, or both, depending on how many allomorphs the entry had and what order they were in.

The reviewer's question on a nine-line production diff is why it needs seven tests. Because the old code was wrong in five distinct ways that masked each other, and because three of them only appear in specific entry shapes — a stem allomorph ordered before the affix process, two affix processes on one entry, an entry reached through the WfiMorphBundle failover path. A single happy-path test passes against the broken code. The review worth your time is the replacement rule, not the deletions.

Where to look

• The new rule compares the clone's counts against the source's own (this is the source) and removes exactly the surplus. Every previous version guessed from the clone's shape, and every count heuristic has a case it cannot distinguish.
• The output surplus is recomputed after the input removal, because removing the default input cascades via RemoveObjectSideEffectsInternal into removing the default output. Assuming two independent removals is what deleted real data.
• PostClone is called once per top-level source object but receives the copy map for the whole pass, including every sibling allomorph's clones. Scoping to copyMap[Hvo] is the core of the fix.
• MoveSenseToCopy reaches MoAffixProcess through three call sites, not the two that are obvious; the third (CreateMatchingAllomorphInTargetEntry) fires in the normal case, when a process affix's Form is blank.
• MoAffixProcess still does not implement ICloneableCmObject; it remains on the generic reflection clone path.

Deliberately not here

• MoAffixProcess.SetCloneProperties — would remove the seed-then-strip dance entirely, but needs hand-rolled ContentRA remapping that PhRegularRule does not, and landing it untested is worse than this fix.
• Narrowing PostClone(Dictionary<int, ICmObject>) to the object's own clone. Breaking public API; deserves its own decision.
• PhTerminalUnit.CodesOS and MoStemName.RegionsOC share the vulnerable shape but are unreachable from any clone call site today.

Verification. Full SIL.LCModel.Tests 1716 passed / 0 failed / 18 skipped on net8.0. Against the test-only commit, 4 of 5 reproduction tests fail. net462 was not built or run — a pre-existing IdlImp toolchain issue in this environment, unrelated to this change; CI will be first to exercise it. Requires a package bump in FieldWorks to ship. Fixes LT-22711.

---

Reading this a year from now — start here

The investigation notes and architecture review were deliberately deleted from the branch rather than merged; their content is synthesised below. They were working documents for a one-time analysis.

The short version: MoAffixProcess.SetDefaultValuesAfterInit seeds every new instance with a default PhVariable input and MoCopyFromInput output — needed so the affix process slice works (FWR-1619). CopyObject builds clones through the normal factory, so a clone is born holding those defaults, and the real cloned content is appended after them. PostClone exists solely to strip the seeds back off. It was doing that wrong.

The five defects, and how they masked each other

1. return where continue was meant. The loop walked the entire shared copy map and bailed at the first value that was not an IMoAffixProcess. Unless the affix process happened to sort first, the cleanup never ran at all — leaving a leading default pair, which renders as an untouched, freshly-created rule.
2. Repeated stripping. PostClone fires once per top-level source object but iterated all clones, so with two affix-process allomorphs the second invocation stripped the first one's already-repaired clone, deleting real content.
3. Wrong scope. It should have looked up its own clone rather than iterating the map at all.
4. The cascade. Removing the default InputOS[0] triggers RemoveObjectSideEffectsInternal, which also removes the matching default OutputOS[0]. The unconditional second RemoveAt(0) then deleted a real, surviving output. This one bites even the single-allomorph case, which the original analysis had assumed was safe.
5. Zero real content. An affix process with legitimately empty InputOS/OutputOS still leaked a default pair: a Count > 1 guard cannot tell "genuinely empty" from "one leaked default", since both read as count 1 once the fresh default is seeded.

Defect 4 was found by writing the tests; defect 5 by adversarial review of the first fix.

Decisions, and why

Compare against the source, not the clone. PostClone runs on the source object, so the source's own InputOS.Count/OutputOS.Count are in hand. The clone should end with exactly that many items; anything more is seed. This is not a heuristic and has no undistinguishable case, which is why it subsumes all five defects in one rule instead of patching them individually.

Recompute the output surplus rather than deriving it. The cascade means the two removals are not independent. Measuring OutputOS's surplus after the input removal is the difference between correct and destructive.

Keep MoAffixProcess on the reflection clone path. SetCloneProperties short-circuits both CopyObject passes, so an implementation would have to redo the OutputOS → InputOS ContentRA remapping by hand. PhRegularRule gets away with its implementation because it has no equivalent cross-list reference.

Paths not taken

Narrowing the PostClone signature. PostClone(Dictionary<int, ICmObject>) hands every implementer the entire copy map and thereby invited defects 1–3. Narrowing it to the object's own clone would remove that footgun — but it is a breaking public API change with three implementers, and it would do nothing for the two latent classes below, where the problem is that no hook was written at all. The genuinely categorical fix is to handle seed-then-append inside CopyObject itself.

Fixing the latent classes. PhTerminalUnit.CodesOS and MoStemName.RegionsOC both seed owned-sequence defaults and implement neither ICloneableCmObject nor PostClone — the same shape as this bug. Neither is reachable from any of the 21 CloneLcmObject(s) call sites today, so both are latent. A future "duplicate phoneme" feature would trip it. PhRegularRule is the model to copy.

Surprising findings

The reported symptom was wrong, and this is now settled. The bug was reported as "both entries look right at first, but coming back later one has the old version" — read as a save/live-state problem. A save/reload round-trip test through the real XML backend shows the corruption is present in memory before saving; reload merely persists data that was already wrong. The reporter has since confirmed the corruption was observed before saving as well, which corroborates the test. There is no timing effect at the LCM level, and no evidence of a second live-state or cache defect above LCM — none is needed to explain the symptom.

A third clone path. MoveSenseToCopy clones LexemeFormOA and AlternateFormsOS — and then, via UpdateReferencesForSenseMove → CreateMatchingAllomorphInTargetEntry, can clone the source affix process a second, independent time. It triggers when a WfiMorphBundle references the moved sense's morph and the morph's Form is blank, which is the normal state for a process affix. Confirmed corrupted pre-fix and correctly repaired post-fix; now permanently tested.

One documented worry was unfounded. MoveSenseToCopy clones lexeme form and alternate forms with separate CopyObject instances, hence separate copy maps, which looked like it could mis-remap cross-references. Schema inspection shows MoForm and its subclasses have no reference field crossing between the two. Non-issue.

Evidence

Mutation testing, run rather than argued:

Mutation	Result
Restore whole-map iteration	two-allomorph test red
Restore unconditional second RemoveAt(0)	4 of 5 red
Remove index 1 instead of index 0 (wrong object, right count)	all in-memory tests red

That last one matters: before the tests were strengthened, two of them asserted counts and containment only, and passed while the wrong object was being removed. They now assert identity at each position and reference-equality of each mapping's ContentRA to its exact target slot.

An unrelated pre-existing KeyNotFoundException in LcmAtomicRefPropertyChanged.Undo() surfaces during teardown of the WfiMorphBundle path test. It reproduces identically at the pre-fix and post-fix commits, so it is not caused by this change; the test commits the action handler in a finally so the crash cannot mask a real result.

---

This change is 

[github-actions]

LCM Tests

    16 files  ± 0      16 suites  ±0   2m 13s ⏱️ +7s
 2 882 tests + 7   2 862 ✅ + 7   20 💤 ±0  0 ❌ ±0 
11 476 runs  +28  11 308 ✅ +28  168 💤 ±0  0 ❌ ±0 

Results for commit c6f36b3. ± Comparison against base commit 9fdb060.